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

В письме от четверг, 19 сентября 2019 г. 17:32:03 MSK пользователь Michael
Paquier написал:

> > 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.
For some reason it does not reproduce on my dev environment, but it not really
important, since the core of the problem is found.
>
> 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);

What a good catch! dummy_index already proved to be useful ;-)


> 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?

Yes I think AccessExclusiveLock is quite good default I think. Especially in
the case when these options are not really used in real world ;-)

> Changing the relopts APIs in
> back-branches is a no-go of course, but we could improve that for
> 13~.

As you know I have plans for rewriting options engine and there would be same
options code both for core Access Methods and for options for AM from
extensions. So there would be API for setting lockmode...
But the way it is going right now, I am not sure it will reviewed to reach
13...


PS. Michael, who will submit this lock mode patch? Hope you will do it? It
should go separately from dummy_index for sure...