Обсуждение: Random inconsistencies in GiST support function declarations

I was idly trying to improve the just-added index AM amvalidate()
functions by having them verify the expected signatures (argument
and result types) of opclass support functions.  opr_sanity currently
does this for btree, hash, and spgist functions, but not for other
cases; it'd be useful IMO if we had more complete coverage on that.

However, I was soon rudely reminded of the reason that opr_sanity
doesn't check GiST support functions, which is that their declarations
in pg_proc are wildly inconsistent.  An example is that the strategy
number argument of GiST consistent functions is stated in the
documentation to be smallint, which is the way that many of the contrib
modules declare that, and all of the consistent functions I've looked
at use PG_GETARG_UINT16() to fetch it ... but most of the built-in
consistent functions declare the argument as integer not smallint.
(This has no impact on what happens at runtime, since the GiST core
code pays no attention to what the functions are declared as.)

There's not a lot of sanity to the declarations of union functions
either, and I've not even gotten to the other seven support functions.

I think it'd be a good idea to clean these things up, rather than weaken
the amvalidate() tests to the point where they'll accept all the existing
weirdnesses.  And the documentation needs to match reality more closely
in any case.

Fixing the pg_proc entries in HEAD seems like no big deal, but some of
the errors are in contrib modules.  If we wanted to be really clean
about that, we'd have to bump those modules' extension versions, which
is a pain in the rear.  Since this has no user-visible impact AFAICS
(the support functions not being callable from SQL), I'm inclined to
just fix the incorrect declarations in-place.  I think it's sufficient
if the contrib modules pass amvalidate checks in future, I don't feel
a need to make existing installations pass.

Any objections?
        regards, tom lane

On Mon, Jan 18, 2016 at 2:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fixing the pg_proc entries in HEAD seems like no big deal, but some of
> the errors are in contrib modules.  If we wanted to be really clean
> about that, we'd have to bump those modules' extension versions, which
> is a pain in the rear.  Since this has no user-visible impact AFAICS
> (the support functions not being callable from SQL), I'm inclined to
> just fix the incorrect declarations in-place.  I think it's sufficient
> if the contrib modules pass amvalidate checks in future, I don't feel
> a need to make existing installations pass.
>
> Any objections?
>
>                         regards, tom lane
>

This work (9ff60273e35cad6e9) seems have broken pg_upgrade when
tsearch2 is installed.

On an empty 9.4 instance with nothing but tsearch2 installed, using
HEAD's pg_upgrade gives this error:

pg_restore: creating OPERATOR CLASS "public.gin_tsvector_ops"
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 3037; 2616 19016
OPERATOR CLASS gist_tp_tsquery_ops jjanes
pg_restore: [archiver (db)] could not execute query: ERROR:  function
gtsquery_consistent(internal, internal, integer, oid, internal) does
not exist   Command was: CREATE OPERATOR CLASS "gist_tp_tsquery_ops"   FOR TYPE "pg_catalog"."tsquery" USING "gist" AS
STORAGE bigint ,   OPE...
 

Cheers,

Jeff

Jeff Janes <jeff.janes@gmail.com> writes:
> This work (9ff60273e35cad6e9) seems have broken pg_upgrade when
> tsearch2 is installed.

> On an empty 9.4 instance with nothing but tsearch2 installed, using
> HEAD's pg_upgrade gives this error:

> pg_restore: creating OPERATOR CLASS "public.gin_tsvector_ops"
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 3037; 2616 19016
> OPERATOR CLASS gist_tp_tsquery_ops jjanes
> pg_restore: [archiver (db)] could not execute query: ERROR:  function
> gtsquery_consistent(internal, internal, integer, oid, internal) does
> not exist
>     Command was: CREATE OPERATOR CLASS "gist_tp_tsquery_ops"
>     FOR TYPE "pg_catalog"."tsquery" USING "gist" AS
>     STORAGE bigint ,
>     OPE...

Ah, drat.  I guess we'll have to continue to provide pg_proc entries with
the old signatures to support pg_upgrade.  Will fix.
        regards, tom lane