On Sun, Nov 25, 2018 at 05:49:42PM +0300, Nikolay Shaplov wrote:
> 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."
The definition of PGXS is here:
https://www.postgresql.org/docs/11/extend-pgxs.html
> 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)
I am not sure that documenting NO_PGXS makes much sense for extensions,
which have normally no need of the surrounding code. If you have
suggestions of improvements for the existing docs, please feel free to
think about a patch and then post it. Docs improvements are a
never-ending task.
> 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.
ISOLATION_OPTS and REGRESS_OPTS already allow to pass down custom
options, and are more extensible than the _CONF variants proposed here.
Keeping the number of options low is not a bad idea either.
> 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?
Good point. This can be cleaned up too, so done.
> 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 :-)
Thanks, I have re-checked, and the thing is working as I would expect.
So committed.
--
Michael