Обсуждение: Add extension options to control TAP and isolation tests

Hi all,

On a recent thread of pgsql-committers has been discussed the fact that
we lacked a bit of infrastructure to allow extensions to control
isolation and TAP tests:
https://www.postgresql.org/message-id/20180905174527.GA2726@paquier.xyz

Attached is a patch which is the result of the previous thread, where a
couple of variables are added for extension authors:
- ISOLATION, similar to REGRESS for pg_regress, which lists isolation
tests.
- ISOLATION_OPTS, which can be used to pass an option set to
pg_isolation_regress.
- TAP_TESTS, a switch to enable running TAP tests.

This results in a bit of cleanup in the Makefile of modules we ship with
upstream:
- modules/brin
- modules/commit_ts
- modules/test_pg_dump
- contrib/bloom, where the TAP tests were actually not running.
- contrib/oid2name
- contrib/test_decoding, which keeps the same feature set, and is
heavily cleaned up (it missed for example the use of the existing
NO_INSTALLCHECK).
- contrib/vacuumlo

Thoughts?
--
Michael

On Wed, Sep 05, 2018 at 06:48:49PM -0700, Michael Paquier wrote:
> On a recent thread of pgsql-committers has been discussed the fact that
> we lacked a bit of infrastructure to allow extensions to control
> isolation and TAP tests:
> https://www.postgresql.org/message-id/20180905174527.GA2726@paquier.xyz
>
> Attached is a patch which is the result of the previous thread, where a
> couple of variables are added for extension authors:
> - ISOLATION, similar to REGRESS for pg_regress, which lists isolation
> tests.
> - ISOLATION_OPTS, which can be used to pass an option set to
> pg_isolation_regress.
> - TAP_TESTS, a switch to enable running TAP tests.

Tom, Alvaro, any thoughts on the proposed patch?  Please note that one
thing which is missing, and that I left on purpose so as the buildfarm
client does not need any extra tweaks, is support for those options in
src/tools/msvc.  It is already possible to run easily any TAP test suite
using vcregress taptest $dir, and test_decoding has some special
handling in the buildfarm code to run isolation tests.  It seems to me
that the amount of cleanup done by the initial patch in all the
Makefiles justifies its existence, and I could always follow-up with a
second patch for MSVC if needed.
--
Michael

Nice cleanup. Also, I like the ability to enable/control more types of tests easily from the Makefile. What are the
nextsteps for this patch? 

On Fri, Nov 09, 2018 at 03:14:00PM +0000, Adam Berlin wrote:
> Nice cleanup. Also, I like the ability to enable/control more types of
> tests easily from the Makefile. What are the next steps for this
> patch?

Thanks.  It seems to me that a complete review is still in order,
particularly regarding the new makefile option names.  And then, if
everybody caring about the topic is happy with the way the patch is
shaped, it can be carried over to being committed, which would be most
likely something I'll do.
--
Michael

В письме от 10 ноября 2018 09:14:19 пользователь Michael Paquier написал:

> > Nice cleanup. Also, I like the ability to enable/control more types of
> > tests easily from the Makefile. What are the next steps for this
> > patch?
>
> Thanks.  It seems to me that a complete review is still in order,
> particularly regarding the new makefile option names.  And then, if
> everybody caring about the topic is happy with the way the patch is
> shaped, it can be carried over to being committed, which would be most
> likely something I'll do.

Is it ok, if I join the reviewing? I like test, especially TAP one, you know
;-)
Since you are much more experienced in postgres then me, I'd try to understand
how does the patch work, try to use if for writing more TAP test, and will
report problems and thoughts I came across while doing that.

So far while first reading the patch I came to following two

1.
--- a/src/test/modules/brin/.gitignore
+++ b/src/test/modules/brin/.gitignore
@@ -1,3 +1,3 @@
 # Generated subdirectories
-/isolation_output/
+/output_iso/
 /tmp_check/

For me name "output_iso" means nothing. iso is something about CD/DVD or about
standards. I would not guess that iso stands for isolation if I did not know
it already. isolation_output is more sensible: I have heard that there are
some isolation tests, this must be something about it. May be it would be
better to change it to isolation_output everywhere instead of changing to
output_iso

2.

--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1293,6 +1293,34 @@ include $(PGXS)
       </listitem>
      </varlistentry>
.
+     <varlistentry>
+      <term><varname>ISOLATION</varname></term>
+      <listitem>
+       <para>
+        list of isolation test cases
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
+      <term><varname>ISOLATION_OPTS</varname></term>
+      <listitem>
+       <para>
+        additional switches to pass to
+        <application>pg_isolation_regress</application>
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
+      <term><varname>TAP_TESTS</varname></term>
+      <listitem>
+       <para>
+        switch defining if TAP tests need to be run
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><varname>NO_INSTALLCHECK</varname></term>
       <listitem>

I tried to find definition in documentation what does "isolation test" exactly
means, but did not find. There is some general words about TAP tests in main
postgres documentation
https://www.postgresql.org/docs/11/regress-tap.html,
but I would not understand anything from it if I did not already know how it
works.

In current extend-pgxs documentation there is some explanation about
regression test, it sensible enough. Since TAP and isolation tests are
introduced now, there should be same short explanation for both of them.

And also it would be good to add links from extend-pgxs to regress-tap and
regress saying that for more info about these tests one can look at postgres
doc, because they work in a similar way.

That's all so far. I'll look more into it later...

--
Do code for fun.

On Tue, Nov 20, 2018 at 07:30:39PM +0300, Nikolay Shaplov wrote:
> Is it ok, if I join the reviewing? I like test, especially TAP one, you know
> ;-)
>
> Since you are much more experienced in postgres then me, I'd try to
> understand how does the patch work, try to use if for writing more TAP
> test, and will report problems and thoughts I came across while doing that.

Thanks for bumping in the field.

> For me name "output_iso" means nothing. iso is something about CD/DVD or about
> standards. I would not guess that iso stands for isolation if I did not know
> it already. isolation_output is more sensible: I have heard that there are
> some isolation tests, this must be something about it. May be it would be
> better to change it to isolation_output everywhere instead of changing to
> output_iso

That's just a default configuration used by the isolation tester.
That's not much bothering with in my opinion for this patch, as the
patch is here to make sure that the default configuration gets used
where it had better be used.  Changing this default value would be of
course doable technically, but that's around for years to changing it
does not seem like good idea.

> I tried to find definition in documentation what does "isolation test" exactly
> means, but did not find. There is some general words about TAP tests in main
> postgres documentation
> https://www.postgresql.org/docs/11/regress-tap.html,
> but I would not understand anything from it if I did not already know how it
> works.

Those are mentioned here as part of the additional test suites:
https://www.postgresql.org/docs/11/regress-run.html#id-1.6.20.5.5

> In current extend-pgxs documentation there is some explanation about
> regression test, it sensible enough. Since TAP and isolation tests are
> introduced now, there should be same short explanation for both of
> them.

I see your point here, and it is true that documentation ought to be
better.  So I have written out a new set of paragraphs which explain the
whereabouts of the new variables a bit more in depth in the section of
extend-pgxs.

> And also it would be good to add links from extend-pgxs to regress-tap and
> regress saying that for more info about these tests one can look at postgres
> doc, because they work in a similar way.

I have added a reference to regress-tap in one of the new paragraphs.
Linking the existing stuff to point to "regress" is a separate issue
though, and while pointing to the TAP section is adapted as its
guidelines are rather general, I am not sure which one would make the
most sense though.
--
Michael

В письме от 21 ноября 2018 09:39:53 пользователь Michael Paquier написал:

> > For me name "output_iso" means nothing. iso is something about CD/DVD or
> > about standards. I would not guess that iso stands for isolation if I did
> > not know it already. isolation_output is more sensible: I have heard that
> > there are some isolation tests, this must be something about it. May be
> > it would be better to change it to isolation_output everywhere instead of
> > changing to output_iso
> That's just a default configuration used by the isolation tester.
> That's not much bothering with in my opinion for this patch, as the
> patch is here to make sure that the default configuration gets used
> where it had better be used.  Changing this default value would be of
> course doable technically, but that's around for years to changing it
> does not seem like good idea.

Ok. If it is long time tradition, let it be :-)

> > I tried to find definition in documentation what does "isolation test"
> > exactly means, but did not find. There is some general words about TAP
> > tests in main postgres documentation
> > https://www.postgresql.org/docs/11/regress-tap.html,
> > but I would not understand anything from it if I did not already know how
> > it works.
>
> Those are mentioned here as part of the additional test suites:
> https://www.postgresql.org/docs/11/regress-run.html#id-1.6.20.5.5

Oh thanks (I feel urge to start editing documentation right now. I will
surpress it, and do it, I hope, later.)
May I also ask a question, I came across. It is not part of the patch, but
nevertheless...
If I look in the code, regression test are sql files with expected output that
are in src/test/regress. If I look in the documentation, all the tests are
regression tests including TAP tests.
https://www.postgresql.org/docs/11/regress.html

what is the right way to look at it?

> > In current extend-pgxs documentation there is some explanation about
> > regression test, it sensible enough. Since TAP and isolation tests are
> > introduced now, there should be same short explanation for both of
> > them.
>
> I see your point here, and it is true that documentation ought to be
> better.  So I have written out a new set of paragraphs which explain the
> whereabouts of the new variables a bit more in depth in the section of
> extend-pgxs.

Oh thanks! Now it is much more clear.

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

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?

2. As far as you said that TAP tests for bloom were never executed,
I guess I should just ignore

-
-wal-check: temp-install
-   (prove_check)

as a part of wrong way to execute TAP tests. (I fond no traces of "wal-check"
string in the code, so I guess this build target is  an invention of bloom
authors)

3. The rest are trivial, except changes for contrib/test_decoding/ and
src/test/modules/brin I will explore them in my next postgres-dev time slot
and then my review work will be done :-)

--
Do code for fun.

On Thu, Nov 22, 2018 at 10:01:26PM +0300, Nikolay Shaplov wrote:
> В письме от 21 ноября 2018 09:39:53 пользователь Michael Paquier написал:
>> Those are mentioned here as part of the additional test suites:
>> https://www.postgresql.org/docs/11/regress-run.html#id-1.6.20.5.5
>
> Oh thanks (I feel urge to start editing documentation right now. I will
> surpress it, and do it, I hope, later.)

If you have a patch to improve the existing docs, please feel free to
submit those on a different thread.  If you have suggestions about
improving this patch's docs, of course please feel free to suggest them
here!

> May I also ask a question, I came across. It is not part of the patch, but
> nevertheless...
> If I look in the code, regression test are sql files with expected output that
> are in src/test/regress. If I look in the documentation, all the tests are
> regression tests including TAP tests.
> https://www.postgresql.org/docs/11/regress.html
>
> what is the right way to look at it?

That's for the main regression test suite within src/test/regress, TAP
is also a regression test suite, the page of the link reflects the
general set of test suites available.

> 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.

> 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

> 2. As far as you said that TAP tests for bloom were never executed,
> I guess I should just ignore
>
> -
> -wal-check: temp-install
> -   (prove_check)
>
> as a part of wrong way to execute TAP tests. (I fond no traces of "wal-check"
> string in the code, so I guess this build target is  an invention of bloom
> authors)

Yes.  It seems that it was useful for debugging at this time, but this
will rot if let as-is...  I am pretty sure that most people don't use
that wrapper.

> 3. The rest are trivial, except changes for contrib/test_decoding/ and
> src/test/modules/brin I will explore them in my next postgres-dev time slot
> and then my review work will be done :-)

Thanks for the input, Nikolay!
--
Michael

Hello,

On 21.11.2018 03:39, Michael Paquier wrote:
> I have added a reference to regress-tap in one of the new paragraphs.
> Linking the existing stuff to point to "regress" is a separate issue
> though, and while pointing to the TAP section is adapted as its
> guidelines are rather general, I am not sure which one would make the
> most sense though.
> --
> Michael

The patch is very useful. Using TAP_TESTS is more convenient and clearer 
than adding wal-check target. Every time I was adding TAP tests for a 
extension I had to remember that I should add wal-check.

After applying the patch all tests pass, there wasn't any error.

Also I tested it in one of our extension which has TAP tests. 
installcheck and check work as expected.

I think the patch can be marked as "Ready for Committer".

But there is a problem that you need to copy your extension to the 
contrib directory if you want to run TAP tests. I tried to run TAP test 
of the extension outside of PostgreSQL source directory. And it failed 
to run the test. It is because `prove_installcheck` redefines 
`top_builddir` and `PG_REGRESS`:

cd ./ && TESTDIR='/home/artur/source/pg/rum' 
PATH="/home/artur/progs/pgsql/bin:$PATH" PGPORT='65432' 
top_builddir='/home/artur/source/pg/rum//home/artur/progs/pgsql/lib/pgxs/src/makefiles/../..' 
PG_REGRESS='/home/artur/source/pg/rum//home/artur/progs/pgsql/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress'

/usr/sbin/prove -I 
/home/artur/progs/pgsql/lib/pgxs/src/makefiles/../../src/test/perl/ -I 
./  t/*.pl
t/001_wal.pl .. Bailout called.  Further testing stopped:  system 
/home/artur/source/pg/rum//home/artur/progs/pgsql/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress 
failed

Unfortunately I didn't find the way to run it, maybe I miss something. 
It can be fixed by an additional patch I attached. I think I can create 
an entry in the future commitfest or it can be joined into your patch.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Fri, Nov 23, 2018 at 05:29:00PM +0300, Arthur Zakirov wrote:
> The patch is very useful. Using TAP_TESTS is more convenient and clearer
> than adding wal-check target. Every time I was adding TAP tests for a
> extension I had to remember that I should add wal-check.

wal-check is a custom option part of contrib/bloom/ which is not aimed
at spreading around.

> After applying the patch all tests pass, there wasn't any error.
>
> Also I tested it in one of our extension which has TAP tests. installcheck
> and check work as expected.

Thanks.

> But there is a problem that you need to copy your extension to the contrib
> directory if you want to run TAP tests. I tried to run TAP test of the
> extension outside of PostgreSQL source directory. And it failed to run the
> test. It is because `prove_installcheck` redefines `top_builddir` and
> `PG_REGRESS`:

I have tested that as well with one of my custom extensions, which has
some TAP tests, and the following Makefile additions:
TAP_TESTS = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
subdir = contrib/EXTENSION_NAME_HERE
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif

Running make clean at the root of the tree, then running make check from
contrib/EXTENSION_NAME_HERE works for me.

> Unfortunately I didn't find the way to run it, maybe I miss something. It
> can be fixed by an additional patch I attached. I think I can create an
> entry in the future commitfest or it can be joined into your patch.

The previous version of the patch I sent make the build of
src/test/regress dependent on if REGRESS is set, but I missed the fact
that TAP tests also call pg_regress, which is the error you are seeing.
The attached patch will be able to work.

Thanks all for the reviews, I'll do a last lookup on Monday my time and
I'll try to get that committed by then.  That's a nice cleanup of the
tree.
--
Michael

В письме от 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 :-)

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

В письме от 26 ноября 2018 08:53:20 Вы написали:

> > 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
Can you please provide the quotation? I see there PGXS mentioned several times
as "a build infrastructure for extensions" and as an environment variable it
is mentioned only in code sample


PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

So for me there is no explanation. Or it is hard to find (that is also bad)

If we are done with your patch, can we still finish this line of discussion in
order to create another small patch concerning PGXS-env-var comment?

On Mon, Nov 26, 2018 at 10:50:35AM +0300, Nikolay Shaplov wrote:
> I see there PGXS mentioned several times as "a build infrastructure
> for extensions" and as an environment variable it is mentioned only in
> code sample

As a variable PGXS stands for the location of the makefile which holds
all the basic rules an extension can use, as pointed for example by
pg_config --pgxs when it comes to extensions compiled out of the tree.
--
Michael

On Fri, Nov 23, 2018 at 09:43:45AM +0900, Michael Paquier wrote:
> That's for the main regression test suite within src/test/regress, TAP
> is also a regression test suite, the page of the link reflects the
> general set of test suites available.

The main patch had to be reverted a couple of days ago via 1d7dd18, and
I have been digging and fixing the set of issues the buildfarm has been
complaining about since.  There were two problems:
- MSVC scripts were not able to handle modules with NO_INSTALLCHECK
defined, and vcregress.pl runs with the assumption that installcheck is
used.  Support for that has been added in 431f159.  Some modules have
been also lacking NO_INSTALLCHECK.
- contrib/bloom/ are unstable.  I have not spent time investigating the
actual root issue, but spawned a thread with the authors of the tests:
https://www.postgresql.org/message-id/20181126025125.GH1776@paquier.xyz

I am attaching the patch I would like to commit to close this thread,
which has as single change TAP_TESTS commented out in contrib/bloom/ to
not explode the buildfarm.  Any objections to that?
--
Michael

On 29.11.2018 05:00, Michael Paquier wrote:
> The main patch had to be reverted a couple of days ago via 1d7dd18, and
> I have been digging and fixing the set of issues the buildfarm has been
> complaining about since.  There were two problems:

Thank you for working on the patch.

I run world-check. It works perfectly. I think the patch is in good shape.

> I am attaching the patch I would like to commit to close this thread,
> which has as single change TAP_TESTS commented out in contrib/bloom/ to
> not explode the buildfarm.  Any objections to that?

As far as I understand bloom TAP tests are stable on some platforms, 
such as linux. Can be TAP test disabled only for some specific 
platforms? For example (as far as I know 'darwin' refers to MacOS):

ifeq (,$(filter win32 darwin,$(PORTNAME)))
TAP_TESTS = 1
endif

PS. But after rereading the Makefile I see that the problem here is to 
get $(PORTNAME). It is defined in Makefile.global, which is included 
below defining TAP_TESTS variable.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Fri, Nov 30, 2018 at 02:18:04PM +0300, Arthur Zakirov wrote:
> As far as I understand bloom TAP tests are stable on some platforms, such as
> linux. Can be TAP test disabled only for some specific platforms?

That can be done with PORTNAME.  However I am doubting that those tests
are even reliable on Linux..  So it would be much better to understand
the root issue before enabling them.
--
Michael

On Thu, Nov 29, 2018 at 11:00:11AM +0900, Michael Paquier wrote:
> I am attaching the patch I would like to commit to close this thread,
> which has as single change TAP_TESTS commented out in contrib/bloom/ to
> not explode the buildfarm.  Any objections to that?

And I gave this stuff another shot with d3c09b9.  Fingers crossed.
--
Michael