pgsql: Fix assorted inconsistencies in GIN opclass support function dec

Fix assorted inconsistencies in GIN opclass support function declarations.

GIN had some minor issues too, mostly using "internal" where something
else would be more appropriate. I went with the same approach as in
9ff60273e35cad6e, namely preferring the opclass' indexed datatype for
arguments that receive an operator RHS value, even if that's not
necessarily what they really are.

Again, this is with an eye to having a uniform rule for ginvalidate()
to check support function signatures.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/dbe2328959e12701fade6b500ad411271923d6e4

Modified Files
--------------
contrib/hstore/hstore--1.3.sql | 12 ++++-----
contrib/intarray/intarray--1.1.sql | 8 +++---
contrib/tsearch2/tsearch2--1.0.sql | 4 +--
doc/src/sgml/gin.sgml | 48 ++++++++++++++++++++----------------
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.h | 22 ++++++++---------
6 files changed, 51 insertions(+), 45 deletions(-)

On Wed, Jan 20, 2016 at 6:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> contrib/hstore/hstore--1.3.sql | 12 ++++-----
> contrib/intarray/intarray--1.1.sql | 8 +++---
> contrib/tsearch2/tsearch2--1.0.sql | 4 +--
>

Hmm... Is it correct to change function signatures without extension
version bump? pg_upgraded clusters would remain with old version of these
functions. Once we have instances with same extension version but with
different signatures of its functions, there is no correct way to refer
these functions in future. I think we should do the version bump in this
case.
The same for 9ff60273e35cad6e9d3a4adf59d5c2455afe9d9e.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> On Wed, Jan 20, 2016 at 6:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> contrib/hstore/hstore--1.3.sql | 12 ++++-----
>> contrib/intarray/intarray--1.1.sql | 8 +++---
>> contrib/tsearch2/tsearch2--1.0.sql | 4 +--

> Hmm... Is it correct to change function signatures without extension
> version bump? pg_upgraded clusters would remain with old version of these
> functions. Once we have instances with same extension version but with
> different signatures of its functions, there is no correct way to refer
> these functions in future. I think we should do the version bump in this
> case.

It doesn't really matter, though, because there simply isn't any need to
refer to these functions from SQL. They are only useful as opclass
support functions.

But this is likely moot anyway, because of the need to bump all the
contrib modules' versions in order to install parallel-safety labels on
their functions. (I wonder why that isn't on the open-items list.)

regards, tom lane

On Mon, May 2, 2016 at 5:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
>> On Wed, Jan 20, 2016 at 6:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> contrib/hstore/hstore--1.3.sql | 12 ++++-----
>>> contrib/intarray/intarray--1.1.sql | 8 +++---
>>> contrib/tsearch2/tsearch2--1.0.sql | 4 +--
>
>> Hmm... Is it correct to change function signatures without extension
>> version bump? pg_upgraded clusters would remain with old version of these
>> functions. Once we have instances with same extension version but with
>> different signatures of its functions, there is no correct way to refer
>> these functions in future. I think we should do the version bump in this
>> case.
>
> It doesn't really matter, though, because there simply isn't any need to
> refer to these functions from SQL. They are only useful as opclass
> support functions.
>
> But this is likely moot anyway, because of the need to bump all the
> contrib modules' versions in order to install parallel-safety labels on
> their functions. (I wonder why that isn't on the open-items list.)

Because it was argued by Noah that this was 9.7 work.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-05-02 17:09:48 -0400, Tom Lane wrote:
> But this is likely moot anyway, because of the need to bump all the
> contrib modules' versions in order to install parallel-safety labels on
> their functions. (I wonder why that isn't on the open-items list.)

I think the concensus when discussing that was that that'd essentially
be a "feature", and thus too late for 9.6. I'm a bit doubtful about
that position, but I also don't have a strong opinion the other way round.

On 2016-05-02 17:12:41 -0400, Robert Haas wrote:
> On Mon, May 2, 2016 at 5:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > But this is likely moot anyway, because of the need to bump all the
> > contrib modules' versions in order to install parallel-safety labels on
> > their functions. (I wonder why that isn't on the open-items list.)
>
> Because it was argued by Noah that this was 9.7 work.

Thinking about this again, I think that's not a good approach: It'll
mean we've to do very similar testing in 9.7 again. I'd rather label as
much as possible in one release.

On Mon, May 2, 2016 at 5:15 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-05-02 17:12:41 -0400, Robert Haas wrote:
>> On Mon, May 2, 2016 at 5:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
>> > But this is likely moot anyway, because of the need to bump all the
>> > contrib modules' versions in order to install parallel-safety labels on
>> > their functions. (I wonder why that isn't on the open-items list.)
>>
>> Because it was argued by Noah that this was 9.7 work.
>
> Thinking about this again, I think that's not a good approach: It'll
> mean we've to do very similar testing in 9.7 again. I'd rather label as
> much as possible in one release.

Yeah, I somewhat agree, but there's also no patch for this, yet.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 3, 2016 at 12:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > On Wed, Jan 20, 2016 at 6:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> contrib/hstore/hstore--1.3.sql | 12 ++++-----
> >> contrib/intarray/intarray--1.1.sql | 8 +++---
> >> contrib/tsearch2/tsearch2--1.0.sql | 4 +--
>
> > Hmm... Is it correct to change function signatures without extension
> > version bump? pg_upgraded clusters would remain with old version of
> these
> > functions. Once we have instances with same extension version but with
> > different signatures of its functions, there is no correct way to refer
> > these functions in future. I think we should do the version bump in this
> > case.
>
> It doesn't really matter, though, because there simply isn't any need to
> refer to these functions from SQL. They are only useful as opclass
> support functions.
>

What if we'll want to reuse some on these functions in new opclass?
Assumption that we don't need to refer these functions from SQL seems
dangerous to me.
If pg_upgraded instances are OK to leave with old function definitions, why
do we bother about new instances?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> On Tue, May 3, 2016 at 12:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> It doesn't really matter, though, because there simply isn't any need to
>> refer to these functions from SQL.

> What if we'll want to reuse some on these functions in new opclass?

What context do you imagine that happening in? Surely no other extension
would attempt to re-use them. If, say, hstore itself wanted to reuse
these functions, that would be in a new version of hstore--n.n.sql, so
there's no problem.

regards, tom lane

On Tue, May 3, 2016 at 4:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > On Tue, May 3, 2016 at 12:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> It doesn't really matter, though, because there simply isn't any need to
> >> refer to these functions from SQL.
>
> > What if we'll want to reuse some on these functions in new opclass?
>
> What context do you imagine that happening in? Surely no other extension
> would attempt to re-use them. If, say, hstore itself wanted to reuse
> these functions, that would be in a new version of hstore--n.n.sql, so
> there's no problem.
>

Let's consider particular example: gin_extract_hstore() function.

In PostgreSQL 9.6 hstore 1.3 it's gin_extract_hstore(hstore, internal)
RETURNS internal.
In PostgreSQL 9.5 hstore 1.3 it's gin_extract_hstore(internal, internal)
RETURNS internal.
After pg_upgrade 9.5 => 9.6 it will be still gin_extract_hstore(internal,
internal) RETURNS internal.

Thus, in 9.6 hstore 1.3 we can meet either

1. gin_extract_hstore(internal, internal) RETURNS internal
2. gin_extract_hstore(hstore, internal) RETURNS internal

Then, if we want to reuse this function in hstore 1.4, we will need to
write a migration script hstore--1.3--1.4.sql. How can we refer
gin_extract_hstore() function from this script?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company