Re: [PATCH] src/test/modules/dummy_index -- way to test reloptionsfrom inside of access method

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.

From 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=80, col1=2, col2=2, col3=4);
ALTER INDEX bloomidx SET (length=100);

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 = kinds;
    newoption->namelen = strlen(name);
    newoption->type = type;
+   newoption->lockmode = 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