Обсуждение: [PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method

Hi!

This patch introduce a dummy_index access method module, that does not do any 
indexing at all, but allow to test reloptions from inside of access method 
extension.

This patch is part of my bigger work on reloptions refactoring.

It came from 
https://www.postgresql.org/message-id/20190220060832.GI15532%40paquier.xyz 
thread where I suggested to add a "enum" reloption type, and we came to 
conclusion that we need to test how this new option works for access method 
created from extension (it does not work in the same way as in-core access 
methods) . But we can't add this option to bloom index, so we need an index 
extension that can be freely used for tests.

So I created src/test/modules/dummy_index, it does no real indexing, but it 
has all types of reloptions that can be set (reloption_int, reloption_real, 
reloption_bool, reloption_string and reloption_string2). It also has set of 
boolean GUC variables that enables test output concerning certain reloption:
(do_test_reloption_int, do_test_reloption_real, do_test_reloption_bool and 
do_test_reloption_string and  do_test_reloption_string2) also set 
do_test_reloptions to true to get any output at all.
Dummy index will print this output when index is created, and when record is 
inserted (this needed to check if your ALTER TABLE did well)
Then you just use normal regression tests: turns on test output, sets some 
reloption and check test output, that it properly reaches the access method 
internals.

While writing this module I kept in mind the idea that this module can be also 
used for other am-related tests, so I separated the code into two parts: 
dummy_index.c has only code related to implementation of an empty access 
method, and all code related to reloptions tests  were stored into 
direloptions.c. So in future somebody can add di[what_ever_he_wants].c whith 
his own tests code, add necessary calls to dummy_index.c, create some GUC 
variables, and has his own feature tested.

So I kindly ask you to review and commit this module, so I would be able to 
continue my work on reloptions refactoring...

Thanks! 

On Mon, Mar 18, 2019 at 10:41:13PM +0300, Nikolay Shaplov wrote:
> So I created src/test/modules/dummy_index, it does no real indexing, but it
> has all types of reloptions that can be set (reloption_int, reloption_real,
> reloption_bool, reloption_string and reloption_string2). It also has set of
> boolean GUC variables that enables test output concerning certain reloption:
> (do_test_reloption_int, do_test_reloption_real, do_test_reloption_bool and
> do_test_reloption_string and  do_test_reloption_string2) also set
> do_test_reloptions to true to get any output at all.
> Dummy index will print this output when index is created, and when record is
> inserted (this needed to check if your ALTER TABLE did well)
> Then you just use normal regression tests: turns on test output, sets some
> reloption and check test output, that it properly reaches the access method
> internals.

Thanks for doing the effort to split that stuff.  This looks like an
interesting base template for anybody willing to look after some
basics with index AMs, like what's done for FDWs with blackhole_fdw.
Perhaps the name should be dummy_am_index or dummy_index_am?
dummy_index does not sound bad either.

> While writing this module I kept in mind the idea that this module can be also
> used for other am-related tests, so I separated the code into two parts:
> dummy_index.c has only code related to implementation of an empty access
> method, and all code related to reloptions tests  were stored into
> direloptions.c. So in future somebody can add di[what_ever_he_wants].c whith
> his own tests code, add necessary calls to dummy_index.c, create some GUC
> variables, and has his own feature tested.

Here are some comments.  I think that this could be simplified
further more.

The README file could have a more consistent format with the rest.
See for example dummy_seclabel/README.  You could add a small
example with its usage.

Is there any point in having string_option2?  String reloptions are
already tested with string_option.  Also => s/Seconf/Second/.

s/valudate/validate/.

+-- Test behavior of second string option (there can be issues with second one)
What are those issues?

+       } else
+       {
Code format does not follow the Postgres guidelines.  You could fix
all that with an indent run.

The ranges of the different values are not tested, wouldn't it be
better to test that as well?

The way the test is configured with the strong dependencies between
the reloption types and the GUCs is much bug-prone I think.  All of
that is present only to print a couple of WARNING messages with
specific values of values.  So, why not removing the GUCs and the
printing logic which shows a subset of values?  Please note that these
are visible directly via pg_class.reloptions.  So we could shave quite
some code.

Please note that the compilation of the module fails.
nodes/relation.h maybe is access/relation.h?  You may want to review
all that.
--
Michael

В письме от вторник, 19 марта 2019 г. 16:09:13 MSK пользователь Michael
Paquier написал:

> > So I created src/test/modules/dummy_index, it does no real indexing, but
> > it
> > has all types of reloptions that can be set (reloption_int,
> > reloption_real,
> > reloption_bool, reloption_string and reloption_string2). It also has set
> > of
> > boolean GUC variables that enables test output concerning certain
> > reloption: (do_test_reloption_int, do_test_reloption_real,
> > do_test_reloption_bool and do_test_reloption_string and
> > do_test_reloption_string2) also set
> > do_test_reloptions to true to get any output at all.
> > Dummy index will print this output when index is created, and when record
> > is inserted (this needed to check if your ALTER TABLE did well)
> > Then you just use normal regression tests: turns on test output, sets some
> > reloption and check test output, that it properly reaches the access
> > method
> > internals.
>
> Thanks for doing the effort to split that stuff.  This looks like an
> interesting base template for anybody willing to look after some
> basics with index AMs, like what's done for FDWs with blackhole_fdw.
I am not sure it is good template. Most methods are empty, and does not show
any example of how it should work.
If I am to create a template I would try to create index that just do seq scan
of indexed values. It would have all code index must have, but the code of the
index algorithm iteslf would be minimal. But it is another task.

> Perhaps the name should be dummy_am_index or dummy_index_am?
> dummy_index does not sound bad either.
Actually I do not see any reason to do it, all indexes in postgres are
implemented as access methods, so it sounds as double naming for me. But I
actually do not care about this name, if you think adding _am is better, so I
did it.
But i did not remove .c file names and did not change di- suffix to dia- in the
code. Is it ok for you?

> > While writing this module I kept in mind the idea that this module can be
> > also used for other am-related tests, so I separated the code into two
> > parts: dummy_index.c has only code related to implementation of an empty
> > access method, and all code related to reloptions tests  were stored into
> > direloptions.c. So in future somebody can add di[what_ever_he_wants].c
> > whith his own tests code, add necessary calls to dummy_index.c, create
> > some GUC variables, and has his own feature tested.
>
> Here are some comments.  I think that this could be simplified
> further more.
>
> The README file could have a more consistent format with the rest.
> See for example dummy_seclabel/README.  You could add a small
> example with its usage.
Good notion. Fixed it.

> Is there any point in having string_option2?  String reloptions are
> already tested with string_option.
There are two reasons for that:
1. We should test both behavior with validation function, and without one. For
this we need two options, because we can change this in runtime
2. The implementation of string functions is a bit tricky. It allocates some
more memory after the Option structure, and string values are placed there. It
works well with one string option, but I was not sure that is works properly
for two of them. I can imagine a bug that will show itself only with a second
option.  So we anyway should test two.

> Also => s/Seconf/Second/.
> s/valudate/validate/.
Thanks. I tried my best with aspell, but still missed something.

> +-- Test behavior of second string option (there can be issues with second
> one) What are those issues?
This issues are listed in README. And also I've written them above. To prevent
confusion I've removed this issue notion.  :-) One who want to know more, can
read README file ;-)

> +       } else
> +       {
> Code format does not follow the Postgres guidelines.  You could fix
> all that with an indent run.
Oups, it's my favorite codestyle, I fall back to it when does not pay
attention. I've reindented the code, a good idea. Should come to it myself....

> The ranges of the different values are not tested, wouldn't it be
> better to test that as well?

My idea was to test only things that can't be tested in regression tests.
Ranges are tested in regression tests ( I also wrote that tests) and it is
better to leave it there.

But the question is good, I would mention it in README file, to make it
clear....

> The way the test is configured with the strong dependencies between
> the reloption types and the GUCs is much bug-prone I think.  All of
> that is present only to print a couple of WARNING messages with
> specific values of values.  So, why not removing the GUCs and the
> printing logic which shows a subset of values?
I am afraid that we will get a mess that will work well, but it would be
difficult for a human to find any logic in the output. And sooner or later we
will need it, when something will go wrong and somebody will try to find out
why.
So it is better to test one option at a time, and that's why mute test output
for other options.

> Please note that these
> are visible directly via pg_class.reloptions.  So we could shave quite
> some code.
Values from pg_class are well tested in regression test. My point here is to
check that they reach index internal as expected. And there is a long way
between pg_class.reloptions and index internals.

> Please note that the compilation of the module fails.
> nodes/relation.h maybe is access/relation.h?  You may want to review
> all that.
Hm... I do not quite understand how it get there and why in worked for me
before. But I changed it to nodes/pathnodes.h It was here because it is needed
for PlannerInfo symbol.

PS. Sorry for long delays. I do not always have much time to do postgres...

Hi Nikolay,

> On 3 Apr 2019, at 20:54, Nikolay Shaplov <dhyan@nataraj.su> wrote:
>
> В письме от вторник, 19 марта 2019 г. 16:09:13 MSK пользователь Michael
> Paquier написал:
>
>> Thanks for doing the effort to split that stuff.  This looks like an
>> interesting base template for anybody willing to look after some
>> basics with index AMs, like what's done for FDWs with blackhole_fdw.
> I am not sure it is good template. Most methods are empty, and does not show
> any example of how it should work.

I think it would probably not be a good template — not for a a solid start point.

There is value in having something that has all the relevant method signatures, just to save someone the bother of
crawlingdocs, or scraping other contrib/ examples for copy/paste snippets. But I think it should really be a different
thing.It would be a distraction to litter such a template with custom reloptions clutter. 

I guess that assumes it is possible to create a realistic AM without configurable options. I’m guessing it should be.
Butperhaps such situations are rarer than I imagine…? 

Better than an empty template, though, would be a concrete, but minimal, implementation of an INDEX/AM. I find it
difficultto see how you get something clear and concise, while trying to simultaneously serve both INDEX/AM template
andreloptions testing needs. 

>> Please note that these
>> are visible directly via pg_class.reloptions.  So we could shave quite
>> some code.
> Values from pg_class are well tested in regression test. My point here is to
> check that they reach index internal as expected. And there is a long way
> between pg_class.reloptions and index internals.

I had the same thought. But on quick inspection — and perhaps I have missed something — I don’t see that /custom/
reloptionsare really tested at all by the regression tests. 

So I do think verifying an extension’s custom reloptions exposure would be valuable.

I guess you might argue that it’s the regression test suite that should properly test that exposure mechanism. I kind
ofagree. :-) But I think that argument falls for similar reasons you cite for your initiative — i.e., it’s basically
prettyhard to set up the situation where any kind of custom reloption would ever be reported. 

Hope that is useful feedback.

denty.

On Thu, Jun 27, 2019 at 10:17 AM Dent John <denty@qqdd.eu> wrote:
> > On 3 Apr 2019, at 20:54, Nikolay Shaplov <dhyan@nataraj.su> wrote:
> > В письме от вторник, 19 марта 2019 г. 16:09:13 MSK пользователь Michael
> > Paquier написал:
> >
> >> Thanks for doing the effort to split that stuff.  This looks like an
> >> interesting base template for anybody willing to look after some
> >> basics with index AMs, like what's done for FDWs with blackhole_fdw.
> > I am not sure it is good template. Most methods are empty, and does not show
> > any example of how it should work.
>
> [review]

Hi Nikolay,

While moving this to the September CF, I noticed this failure:

test reloptions                   ... FAILED       32 ms

--- /home/travis/build/postgresql-cfbot/postgresql/src/test/modules/dummy_index_am/expected/reloptions.out
2019-08-01 08:06:16.580197980 +0000
+++ /home/travis/build/postgresql-cfbot/postgresql/src/test/modules/dummy_index_am/results/reloptions.out
2019-08-01 08:11:57.817493999 +0000
@@ -13,12 +13,14 @@
 CREATE INDEX test_idx ON tst USING dummy_index_am (i) WITH (int_option = 5);
 WARNING:  int_option = 5
 ALTER INDEX test_idx SET (int_option = 3);
+ERROR:  unrecognized lock mode: 2139062143
 INSERT INTO tst VALUES(1);
-WARNING:  int_option = 3
+WARNING:  int_option = 5
 ALTER INDEX test_idx SET (bool_option = false);
 ALTER INDEX test_idx RESET (int_option);
+ERROR:  unrecognized lock mode: 2139062143
 INSERT INTO tst VALUES(1);
-WARNING:  int_option = 10
+WARNING:  int_option = 5
 DROP INDEX test_idx;
 SET dummy_index.do_test_reloption_int to false;
 -- Test behavior of real option (default and non default values)
@@ -48,9 +50,10 @@
 INSERT INTO tst VALUES(1);
 WARNING:  bool_option = 1
 ALTER INDEX test_idx SET (int_option = 5, bool_option = false);
+ERROR:  unrecognized lock mode: 2139062143
 ALTER INDEX test_idx RESET (bool_option);
 INSERT INTO tst VALUES(1);
-WARNING:  bool_option = 1
+WARNING:  No reloptions is set, default values will be chosen in module runtime
 DROP INDEX test_idx;
 SET dummy_index.do_test_reloption_bool to false;
 -- Test behavior of string option (default and non default values + validate
@@ -68,12 +71,12 @@
 WARNING:  Validating string option 'Valid_value'
 WARNING:  string_option = 'Valid_value'
 ALTER INDEX test_idx SET (string_option = "Valid_value_2", int_option = 5);
-WARNING:  Validating string option 'Valid_value_2'
+ERROR:  unrecognized lock mode: 2139062143
 INSERT INTO tst VALUES(1);
-WARNING:  string_option = 'Valid_value_2'
+WARNING:  string_option = 'Valid_value'
 ALTER INDEX test_idx RESET (string_option);
 INSERT INTO tst VALUES(1);
-WARNING:  string_option = 'DefaultValue'
+WARNING:  No reloptions is set, default values will be chosen in module runtime
 DROP INDEX test_idx;
 SET dummy_index.do_test_reloption_string to false;
 -- Test behavior of second string option
@@ -87,11 +90,12 @@
 "Some_value");
 WARNING:  string_option2 = 'Some_value'
 ALTER INDEX test_idx SET (string_option2 = "Valid_value_2", int_option = 5);
+ERROR:  unrecognized lock mode: 2139062143
 INSERT INTO tst VALUES(1);
-WARNING:  string_option2 = 'Valid_value_2'
+WARNING:  string_option2 = 'Some_value'
 ALTER INDEX test_idx RESET (string_option2);
 INSERT INTO tst VALUES(1);
-WARNING:  string_option2 = 'SecondDefaultValue'
+WARNING:  No reloptions is set, default values will be chosen in module runtime
 DROP INDEX test_idx;
 SET dummy_index.do_test_reloption_string2 to false;
 SET dummy_index.do_test_reloptions to false;

--
Thomas Munro
https://enterprisedb.com

В Fri, 2 Aug 2019 11:12:35 +1200
Thomas Munro <thomas.munro@gmail.com> пишет:

> While moving this to the September CF, I noticed this failure:
>
> test reloptions                   ... FAILED       32 ms

Do you have any idea, how to reproduce this? I tried this patch on
current master, and did not get result you are talking about.
Is it still there for you BTW?

On Thu, Sep 19, 2019 at 7:58 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:
> В Fri, 2 Aug 2019 11:12:35 +1200
> Thomas Munro <thomas.munro@gmail.com> пишет:
>
> > While moving this to the September CF, I noticed this failure:
> >
> > test reloptions                   ... FAILED       32 ms
>
> Do you have any idea, how to reproduce this? I tried this patch on
> current master, and did not get result you are talking about.
> Is it still there for you BTW?

Hi Nikolay,

Yeah, it's still happening on Travis:

https://travis-ci.org/postgresql-cfbot/postgresql/builds/586714100

Although the "reloptions" tests passes when it's run as part of the
regular test schedule (ie "make check"), the patch also runs it from
src/test/modules/dummy_index_am/Makefile ("REGRESS = reloptions"), and
when it runs in that context it fails.  Cfbot is simply running "make
check-world".

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:

    frame #4: 0x00000001051e6572 postgres`elog_finish(elevel=22,
fmt="unrecognized lock mode: %d") at elog.c:1365:2
    frame #5: 0x0000000104ff033a
postgres`LockAcquireExtended(locktag=0x00007ffeeb14bc28,
lockmode=2139062143, sessionLock=false, dontWait=false,
reportMemoryError=true, locallockp=0x00007ffeeb14bc20) at lock.c:756:3
    frame #6: 0x0000000104fedaed postgres`LockRelationOid(relid=16397,
lockmode=2139062143) at lmgr.c:116:8
    frame #7: 0x0000000104c056f2
postgres`RangeVarGetRelidExtended(relation=0x00007fbd0f000b58,
lockmode=2139062143, flags=0,
callback=(postgres`RangeVarCallbackForAlterRelation at
tablecmds.c:14834), callback_arg=0x00007fbd0f000d60) at
namespace.c:379:4
    frame #8: 0x0000000104d4b14d
postgres`AlterTableLookupRelation(stmt=0x00007fbd0f000d60,
lockmode=2139062143) at tablecmds.c:3445:9
    frame #9: 0x000000010501ff8b
postgres`ProcessUtilitySlow(pstate=0x00007fbd10800d18,
pstmt=0x00007fbd0f0010b0, queryString="ALTER INDEX test_idx SET
(int_option = 3);", context=PROCESS_UTILITY_TOPLEVEL,
params=0x0000000000000000, queryEnv=0x0000000000000000,
dest=0x00007fbd0f0011a0, completionTag="") at utility.c:1111:14
    frame #10: 0x000000010501f480
postgres`standard_ProcessUtility(pstmt=0x00007fbd0f0010b0,
queryString="ALTER INDEX test_idx SET (int_option = 3);",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000,
queryEnv=0x0000000000000000, dest=0x00007fbd0f0011a0,
completionTag="") at utility.c:927:4

AlterTableGetLockLevel() returns that crazy lockmode value, becase it
calls AlterTableGetRelOptionsLockLevel(), I suspect with a garbage
defList, but I didn't dig further.

--
Thomas Munro
https://enterprisedb.com

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

В письме от четверг, 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...

On Thu, Sep 19, 2019 at 02:13:23PM +0300, Nikolay Shaplov wrote:
> What a good catch! dummy_index already proved to be useful ;-)

Yes, the testing around custom reloptions is rather poor now, so this
module has value.  I still don't like much its shape though, so I
began hacking on it for integration, and I wanted to get that part
down in this CF :)

There may be other issues, but let's sort out that later if anything
shows up.

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

I guess so, but with table AMs introduced in 12, I would suspect that
we are going to have much more use cases popping out, and that these
use cases would be very happy to have the possibility to lower the
lock level needed to set a custom reloption.  I would like to get that
fixed and back-patched separately.  As it is not especially clear for
everybody here in a thread dedicated to a test module that we are
discussing about a backend-side bug, I am going to spawn a new thread
with a proper patch.  Perhaps I missed something as well, so it would
be good to get more input on that.

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

Well, another thing would be to extend the existing routines so as
they take an extra argument to be able to enforce the lockmode, which
is something that can be done without a large rewrite of the whole
facility, and the change is less invasive so it would have better
chances to get into core.  I don't mind changing those APIs on HEAD by
the way as long as the breakage involves a clean compilation failure
and I don't think they are the most popular extension APIs ever.
Perhaps others don't have the same line of thoughts, but let's see.
--
Michael

On Fri, Sep 20, 2019 at 09:16:58AM +0900, Michael Paquier wrote:
> On Thu, Sep 19, 2019 at 02:13:23PM +0300, Nikolay Shaplov wrote:
>> What a good catch! dummy_index already proved to be useful ;-)
>
> Yes, the testing around custom reloptions is rather poor now, so this
> module has value.  I still don't like much its shape though, so I
> began hacking on it for integration, and I wanted to get that part
> done in this CF :)

So...  I have looked at the patch of upthread in details, and as I
suspected the module is over-designed.  First, on HEAD the coverage of
reloptions.c is 86.6%, with your patch we get at 94.1%, and with the
attached I reach 95.1% thanks to the addition of a string parameter
with a NULL default value and a NULL description, for roughly half the
code size.

The GUCs are also basically not necessary, as you can just replace the
various WARNING calls (please don't call elog on anything which can be
reached by the user!) by lookups at reloptions in pg_class.  Once this
is removed, the whole code gets more simple, and there is no point in
having neither a separate header nor a different set of files and the
size of the whole module gets really reduced.

I still need to do an extra pass on the code (particularly the AM
part), but I think that we could commit that.  Please note that I
included the fix for the lockmode I sent today so as the patch can be
tested:
https://www.postgresql.org/message-id/20190920013831.GD1844@paquier.xyz

Thoughts?
--
Michael

On Fri, Sep 20, 2019 at 08:58:27PM +0900, Michael Paquier wrote:
> I still need to do an extra pass on the code (particularly the AM
> part), but I think that we could commit that.  Please note that I
> included the fix for the lockmode I sent today so as the patch can be
> tested:
> https://www.postgresql.org/message-id/20190920013831.GD1844@paquier.xyz

I looked at that over the last couple of days, and done as attached.
Well, the actual module is in 0003.  I have added more comments to
document the basic AM calls so as it can easier be used as a template
for some other work, and tweaked a couple of things.  0001 and 0002
are just the patches from the other thread to address the issues with
the lock mode of custom reloptions.
--
Michael

On 2019-Sep-24, Michael Paquier wrote:

> I looked at that over the last couple of days, and done as attached.
> Well, the actual module is in 0003.  I have added more comments to
> document the basic AM calls so as it can easier be used as a template
> for some other work, and tweaked a couple of things.  0001 and 0002
> are just the patches from the other thread to address the issues with
> the lock mode of custom reloptions.

0003 looks useful, thanks for completing it.  I think it would be a good
idea to test invalid values for each type of reloption too (passing
floating point to integers, strings to floating point, and so on).
If you can get this pushed, I'll push the enum reloptions on top.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В Fri, 20 Sep 2019 20:58:27 +0900
Michael Paquier <michael@paquier.xyz> пишет:


Sorry I am really slow to answer... I hope it is not too late.

> The GUCs are also basically not necessary, as you can just replace the
> various WARNING calls (please don't call elog on anything which can be
> reached by the user!) by lookups at reloptions in pg_class.  Once this
> is removed, the whole code gets more simple, and there is no point in
> having neither a separate header nor a different set of files and the
> size of the whole module gets really reduced.

Reading options from pg_class is not a good idea. We already do this in
reloption regression test. Here the thing is almost same...

My point of testing was to read these values from bytea right from
inside of the module. This is not exactly the same value as in pg_class.
It _should_ be the same. But nobody promised is _is_ the same. That is
why I was reading it right from relotions in-memory bytea, the same way
real access methods do it.

And then we came to a GUC variables. Because it we have five reloptions
and we print them all each time we change something, there would be
quite huge output.
It is ok when everything goes well. Comparing with 'expected' is cheap.
But is something goes wrong, then it would be very difficult to find
proper place in this output to deal with it.
So I created GUCs so we can get only one output in a row, not a whole
bunch.

These are my points.

В письме от вторник, 24 сентября 2019 г. 9:25:54 MSK пользователь Alvaro
Herrera написал:

> 0003 looks useful, thanks for completing it.  I think it would be a good
> idea to test invalid values for each type of reloption too (passing
> floating point to integers, strings to floating point, and so on).

We already do it in reloption regression tests.

My idea was to test here only the things that can't be tested in regression
tests, on in real indexes like bloom.

On 2019-Sep-24, Nikolay Shaplov wrote:

> В письме от вторник, 24 сентября 2019 г. 9:25:54 MSK пользователь Alvaro 
> Herrera написал:
> 
> > 0003 looks useful, thanks for completing it.  I think it would be a good
> > idea to test invalid values for each type of reloption too (passing
> > floating point to integers, strings to floating point, and so on).
> 
> We already do it in reloption regression tests.
> 
> My idea was to test here only the things that can't be tested in regression 
> tests, on in real indexes like bloom.

I suppose that makes sense.  But of course when I push enum reloptions
I will have to add such a test, since bloom does not have one.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 24, 2019 at 04:49:11PM +0300, Nikolay Shaplov wrote:
> And then we came to a GUC variables. Because it we have five reloptions
> and we print them all each time we change something, there would be
> quite huge output.

Well, that depends on how you design your tests.  The first versions
of the patch overdid it, and those GUCs have IMO little place for a
module aimed as well to be a minimized referential template focused on
testing some portions of the backend code.

> It is ok when everything goes well. Comparing with 'expected' is cheap.
> But is something goes wrong, then it would be very difficult to find
> proper place in this output to deal with it.
> So I created GUCs so we can get only one output in a row, not a whole
> bunch.

I am still not convinced that this is worth the complication.  Your
point is that you want to make *on-demand* and *user-visible* the set
of options stored in rd_options after filling in the relation options
using the static table used in the AM.

One way to do that could be to have a simple wrapper function which
could be called at SQL level to do those checks, or you could issue a
NOTICE with all the data filled in amoptions() or even ambuild(),
though the former makes the most sense as we fill in the options
there.

One thing that I think would value in the module would be to show how
a custom string option can be properly parsed when doing some
decisions in the AM.  Now we store an offset in the static table, and
one needs to do a small dance with it to fetch the actual option
value.

This can be guessed easily as for example gist has a string option
with "buffering", but we could document that better in the dummy
template, say like that:
@@ -206,6 +210,15 @@ dioptions(Datum reloptions, bool validate)
    fillRelOptions((void *) rdopts, sizeof(DummyIndexOptions), options, numoptions,
                   validate, di_relopt_tab,  lengthof(di_relopt_tab));

+   option_string_val = (char *) rdopts + rdopts->option_string_val_offset;
+   option_string_null = (char *) rdopts + rdopts->option_string_null_offset;
+   ereport(NOTICE,
+           (errmsg("table option_int %d, option_real %f, option_bool %s, "
+                   "option_string_val %s, option_option_null %s",
+                   rdopts->option_int, rdopts->option_real,
+                   rdopts->option_bool ? "true" : "false",
+                   option_string_val ? option_string_val : "NULL",
+                   option_string_null ? option_string_null : "NULL")));

The patch I have in my hands now is already doing a lot, so I am
discarding that part for now.  And we can easily improve it
incrementally.

(One extra thing which is also annoying with the current interface is
that we don't actually pass down the option name within the validator
function for string options like GUCs, so you cannot know on which
option you work on if a module generates logs, I'll send an extra
patch for that on a separate thread.)
--
Michael

On Tue, Sep 24, 2019 at 12:38:30PM -0300, Alvaro Herrera wrote:
> On 2019-Sep-24, Nikolay Shaplov wrote:
>> В письме от вторник, 24 сентября 2019 г. 9:25:54 MSK пользователь Alvaro
>> Herrera написал:
>>> 0003 looks useful, thanks for completing it.  I think it would be a good
>>> idea to test invalid values for each type of reloption too (passing
>>> floating point to integers, strings to floating point, and so on).
>>
>> We already do it in reloption regression tests.
>>
>> My idea was to test here only the things that can't be tested in regression
>> tests, on in real indexes like bloom.
>
> I suppose that makes sense.  But of course when I push enum reloptions
> I will have to add such a test, since bloom does not have one.

Good point.  We rely now on the GUC parsing for reloptions, so having
cross-checks about what patterns are allowed or not is a good idea for
all reloption types.  I have added all that, and committed the
module.  The amount of noise generated by the string validator routine
was a bit annoying, so I have silenced them where they don't really
matter (basically everything except the initial creation).
--
Michael