Обсуждение: 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