Re: Add extension options to control TAP and isolation tests

В письме от 23 ноября 2018 09:43:45 пользователь Michael Paquier написал:

> > So, I continued exploring your patch. First I carefully read changes in
> > pgxs.mk. The only part of it I did not understand is
> >
> > .PHONY: submake
> >
> > -submake:
> > ifndef PGXS
> >
> > +submake:
> > +ifdef REGRESS
> >
> > $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X)
> >
> > endif
> >
> > +ifdef ISOLATION
> > + $(MAKE) -C $(top_builddir)/src/test/isolation all
> > +endif
> > +endif # PGXS
>
> This is to make sure that the necessary tools are compiled before
> running the related tests. "submake:" needs to be moved out actually.
> Thinking about it a bit more, we should also remove the "ifdef REGRESS"
> and "ifdef ISOLATION" because there are some dependencies here. For
> example TAP tests call pg_regress to initialize connection policy. TAP
> tests may also use isolation_tester or such.

Can you add some comments in this part of pgxs.mk that explains this thing
about pre-building needed tools? This will make understanding it more easy...

>
> > I suppose it is because the purpose of PGXS is never explained, not in the
> > documentation, not in the code comments, and PGXS := $(shell $(PG_CONFIG)
> > -- pgxs) sounds like some strange magic spell, that explains nothing. In
> > what cases it is defined? In what cases it is not defined? What exactly
> > does it store? Can we make things more easy to understand here?
>
> That's part of a larger scope which is here:
> https://www.postgresql.org/docs/11/extend-pgxs.html

I've carefully read this documentation. And did not get clear explanation of
what is the true purpose of PGXS environment variable. Only

"The last three lines should always be the same. Earlier in the file, you
assign variables or add custom make rules."

May be it should bot be discussed in the doc, but it should be mentioned in
pgxs.mk . I guess both PGXS nad NO_PGXS should be explicitly described, in
order to make the rest of the code more readable. (As for me I now have some
intuitive understanding of it's nature, but it would be better to have strict
explanation)

So about test_decoding

contrib/test_decoding/Makefile:

EXTRA_INSTALL=contrib/test_decoding

This sounds a bit strange to me. Why in make file for <some_extantions> we
should explicitly specify that this <some_extantions> is needed for tests. It
is obviously needed! Man, we are testing it!! ;-)

I would suspect that is should be added somewhere in pgxs.mk code, when it is
needed. Or this is not as obvious and trivial as I see it?

I guess it came from
-submake-test_decoding:
- $(MAKE) -C $(top_builddir)/contrib/test_decoding

but still there is no logic in it.

+REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
+ISOLATION_OPTS = --temp-config
$(top_srcdir)/contrib/test_decoding/logical.conf

When I read these lines an idea about ISOLATION_CONF and REGRESS_CONF
variables came into my mind. That are transformed into proper _OPTS in pgxs.mk
? Since there is only one use case, may be it do not worth it. So I just speak
this thought aloud without any intention to make it reality.

- $(MAKE) -C $(top_builddir)/src/test/regress all
is replaced with
$(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X)
in pgxs.mk. I hope changing "all" to "pg_regress$(X)" is ok, because I do not
understand it.

I've also greped for "pg_isolation_regress_check" and found that it is also
used in src/test/modules/snapshot_too_old that also uses PGXS (at least as an
option) should not we include it in your patch too?

So I think that is it. Since Artur said, he successfully used your TAP patch
in other extensions, I will not do it, considering it is already checked. If
you think it is good to recheck, I can do it too :-)