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

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