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

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