Date: Thu, 19 Sep 2019 17:32:03 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Nikolay Shaplov <dhyan@nataraj.su>, Dent John <denty@qqdd.eu>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>
Subject: Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions
 from inside of access method
Message-ID: <20190919083203.GC21144@paquier.xyz>
References: <17071942.m9zZutALE6@x200m>
 <20190319070913.GE2899@paquier.xyz>
 <1562184.o3gKksHyEI@x200m>
 <6EED2D0C-6FC9-4958-BA6C-632E19210531@QQdd.eu>
 <CA+hUKG+7RWOvnvZ1H+e2iUfkXNA17RYO1upoGZm8Tsaet7HbDQ@mail.gmail.com>
 <1862640.9qQ6WrGjfI@x200m>
 <CA+hUKG+8294zbN_k=QDgcW2TFia5TqsTR8nJLyYU+_C8vUTmrg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="kVXhAStRUZ/+rrGn"
Content-Disposition: inline
In-Reply-To: 
 <CA+hUKG+8294zbN_k=QDgcW2TFia5TqsTR8nJLyYU+_C8vUTmrg@mail.gmail.com>
Precedence: bulk


--kVXhAStRUZ/+rrGn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Sep 19, 2019 at 10:51:09AM +1200, Thomas Munro wrote:
> Let's see if I can see this on my Mac... yep, with "make -C
> src/test/modules/dummy_index_am directory check" I see a similar
> failure with "ERROR:  unrecognized lock mode: 2139062143".  I changed
> that to a PANIC and got a core file containing this stack:

A simple make check on the module reproduces the failure, so that's
hard to miss.

=46rom what I can see it is not a problem caused directly by this
module, specifically knowing that this test module is actually copying
what bloom is doing when declaring its reloptions.  Take this example:
CREATE EXTENSION bloom;
CREATE TABLE tbloom AS
    SELECT
      (random() * 1000000)::int as i1,
      (random() * 1000000)::int as i2,
      (random() * 1000000)::int as i3,
      (random() * 1000000)::int as i4,
      (random() * 1000000)::int as i5,
      (random() * 1000000)::int as i6
    FROM
   generate_series(1,100);
CREATE INDEX bloomidx ON tbloom USING bloom (i1,i2,i3)
       WITH (length=3D80, col1=3D2, col2=3D2, col3=3D4);
ALTER INDEX bloomidx SET (length=3D100);

And then you get that:
ERROR:  XX000: unrecognized lock mode: 2139062143
LOCATION:  LockAcquireExtended, lock.c:756

So the options are registered in the relOpts array managed by
reloptions.c but the data is not properly initialized.  Hence when
looking at the lock needed we have an option match, but the lock
number is incorrect, causing the failure.  It looks like there is no
direct way to enforce the lockmode used for a reloption added via
add_int_reloption which does the allocation to add the option to
add_reloption(), but enforcing the value to be initialized fixes the
issue:
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -658,6 +658,7 @@ allocate_reloption(bits32 kinds, int type, const
char *name, const char *desc)
    newoption->kinds =3D kinds;
    newoption->namelen =3D strlen(name);
    newoption->type =3D type;
+   newoption->lockmode =3D AccessExclusiveLock;
    MemoryContextSwitchTo(oldcxt);

I would think that initializing that to a sane default is something
that we should do anyway, or is there some trick allowing the
manipulation of relOpts I am missing?  Changing the relopts APIs in
back-branches is a no-go of course, but we could improve that for
13~.

While reading through the code, I found some extra issues...  Here are
some comments about them.

+++ b/src/test/modules/dummy_index_am/Makefile
@@ -0,0 +1,21 @@
+# contrib/bloom/Makefile
Incorrect copy-paste here.

+extern IndexBulkDeleteResult *dibulkdelete(IndexVacuumInfo *info,
+            IndexBulkDeleteResult *stats, IndexBulkDeleteCallback callback,
+            void *callback_state);
All the routines defining the index AM can just be static, so there is
no point to complicate dummy_index.h with most of its contents.

Some routines are missing a (void) in their declaration when the
routines have no arguments.  This can cause warnings.

No sure I see the point of the various GUC with the use of WARNING
messages to check the sanity of the parameters.  I find that awkward.
--
Michael

--kVXhAStRUZ/+rrGn
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2DPQMACgkQnvQgOdby
QH0m+Q//T8PK6H3OkgrQJ4FN2rknzphOmkhfrs1GHKSEINtyKTBLHvji26VmKJql
IbpDHDPnZCWXf9KCDd2cajS0SqAGjoggw5fSUf6Q6XZu3HmLpGoSyGDzkofhTg24
zkqmnf9YRYaRKUB/aQFKm4Q/9RYwWEnmPcr5qSZQQ2D0Ix93TyFGg7nf00dwi/Df
DYSb1cU08BMfd36LUsg+Rzig2OXk47f7wVpRcvQiottI4mohqKeADrPPNyEKgVhO
iy1u6CcZ5EvHWyiqjPE9DImcnJSHdh3fmdbzAuiTkmk8X6ax2YsUJ8I09EM6LEpR
SkeNPFGXvOX/zLrHfFFoI4hCg2UK7kCPcdPQHW4rFjEGKQ3fJTSMNPDddcei4KfV
/Ij4ekyh5KWWWMDEJT9R0kpLXLjxRZivBsBIDeyquUoZV2gEj1zhh6onWmTYvmy0
jdOTTrJX/WLVGETWtZEFIK2IqrESqHv3On1gWOkbRF/fmc2q7DVLA2qFjpAAUa2y
Ew4z4r/8Zrh85BV8GoxqGFAwCAMLmuAm/Q5tz4jhntyd+4vPajBq/2CNvvr+413Y
cjVF7ohZk++UgVVdgeaWz4q/K7Y4OKEgLV2fYq+rXaYpX/ZN5f0smWR2G5gPUtJP
ToHJ1wc1OcwYOvD/V3Yv7L5K/HWHTzzwBJQxUz9d84eE9y63VFQ=
=XrNZ
-----END PGP SIGNATURE-----

--kVXhAStRUZ/+rrGn--