В письме от 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 :-)
signature.asc
Description: This is a digitally signed message part.
