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

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