Thread: [HACKERS] Fix bloom WAL tap test

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

The patch doesn't apply against master:

fabrizio@macanudo:/d/postgresql (master) 
$ git apply /tmp/wal-check-on-bloom-check.patch
error: contrib/bloom/Makefile: already exists in working directory

Regards,

Fabrízio Mello

The new status of this patch is: Waiting on Author

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Sep 29, 2017 at 10:32 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Wed, Sep 6, 2017 at 5:06 PM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>>
>> On Wed, Sep 6, 2017 at 4:08 PM, Alexander Korotkov
>> <a.korotkov@postgrespro.ru> wrote:
>>>
>>> I just realized that these lines of contrib/bloom/t/001_wal.pl don't
>>> check that queries give same results on master and standby.  They just check
>>> that *return codes* of psql are equal.
>>>
>>>> # Run test queries and compare their result
>>>> my $master_result = $node_master->psql("postgres", $queries);
>>>> my $standby_result = $node_standby->psql("postgres", $queries);
>>>> is($master_result, $standby_result, "$test_name: query result matches");
>>>
>>>
>>> Attached patch fixes this problem by using safe_psql() which returns psql
>>> output instead of return code.  For safety, this patch replaces psql() with
>>> safe_psql() in other places too.
>>>
>>> I think this should be backpatched to 9.6 as bugfix.
>>
>>
>> Also, it would be nice to call wal-check on bloom check (now wal-check
>> isn't called even on check-world).
>> See attached patch.  My changes to Makefile could be cumbersome.  Sorry
>> for that, I don't have much experience with them...
>
>
> This topic didn't receive any attention yet.  Apparently, it's because of
> in-progress commitfest and upcoming release.
> I've registered it on upcoming commitfest as bug fix.
> https://commitfest.postgresql.org/15/1313/
>

I understood the necessity of this patch and reviewed two patches.

For /fix-bloom-wal-check.patch, it looks good to me. I found no
problem. But for wal-check-on-bloom-check.patch, if you want to run
wal-check even when running 'make check' or 'make check-world', we can
just add wal-check test as follows?

diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
index 13bd397..c1566d4 100644
--- a/contrib/bloom/Makefile
+++ b/contrib/bloom/Makefile
@@ -20,5 +20,7 @@ include $(top_builddir)/src/Makefile.globalinclude $(top_srcdir)/contrib/contrib-global.mkendif

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

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 8, 2017 at 1:49 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Tue, Nov 7, 2017 at 4:26 PM, Fabrízio Mello <fabriziomello@gmail.com>
> wrote:
>> The patch doesn't apply against master:
>>
>> fabrizio@macanudo:/d/postgresql (master)
>> $ git apply /tmp/wal-check-on-bloom-check.patch
>> error: contrib/bloom/Makefile: already exists in working directory
>
> Apparently I sent patches whose are ok for "patch -p1", but not ok for "git
> apply".
> Sorry for that.   I resubmit both of them.

I would not worry about that as long as there is a way to apply
patches. git apply fails to easily to be honest, while patch -p1 is
more permissive. I use the latter extensively by the way, because it
has the merit to not be a PITA.
--
Michael


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 8, 2017 at 1:58 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Tue, Nov 7, 2017 at 4:34 PM, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
>> I understood the necessity of this patch and reviewed two patches.
>
> Good, thank you.

That's clearly a bug fix.

>> diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
>> index 13bd397..c1566d4 100644
>> --- a/contrib/bloom/Makefile
>> +++ b/contrib/bloom/Makefile
>> @@ -20,5 +20,7 @@ include $(top_builddir)/src/Makefile.global
>>  include $(top_srcdir)/contrib/contrib-global.mk
>>  endif
>>
>> +check: wal-check
>> +
>>  wal-check: temp-install
>>         $(prove_check)
>
>
> I've tried this version Makefile.  And I've seen the only difference: when
> tap tests are enabled, this version of Makefile runs tap tests before
> regression tests.  While my version of Makefile runs tap tests after
> regression tests.  That seems like more desirable behavior for me.  But it
> would be sill nice to simplify Makefile.

Why do you care about the order of actions? There is no dependency
between each test and it seems to me that it should remain so to keep
a maximum flexibility. This does not sacrifice coverage as well. In
short, Sawada-san's suggestion looks like a thing to me. One piece
that it is missing though is that installcheck triggers only the
SQL-based tests, and it should also trigger the TAP tests. So I think
that you should instead do something like that:

--- a/contrib/bloom/Makefile
+++ b/contrib/bloom/Makefile
@@ -20,5 +20,12 @@ include $(top_builddir)/src/Makefile.globalinclude $(top_srcdir)/contrib/contrib-global.mkendif

+installcheck: wal-installcheck
+
+check: wal-check
+
+wal-installcheck:
+   $(prove_installcheck)
+wal-check: temp-install   $(prove_check)

This works for me as I would expect it should. Could you update the
patch? That's way more simple than having to replicate again rules
like regresscheck and regressioncheck-install. I am switching the
patch back to "waiting on author" for now.

I can see that src/test/modules/commit_ts is missing the shot as well,
and I think that's the case as well of other test modules like
pg_dump's suite..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 8, 2017 at 11:20 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 1:58 AM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>> On Tue, Nov 7, 2017 at 4:34 PM, Masahiko Sawada <sawada.mshk@gmail.com>
>> wrote:
>>> I understood the necessity of this patch and reviewed two patches.
>>
>> Good, thank you.
>
> That's clearly a bug fix.
>
>>> diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
>>> index 13bd397..c1566d4 100644
>>> --- a/contrib/bloom/Makefile
>>> +++ b/contrib/bloom/Makefile
>>> @@ -20,5 +20,7 @@ include $(top_builddir)/src/Makefile.global
>>>  include $(top_srcdir)/contrib/contrib-global.mk
>>>  endif
>>>
>>> +check: wal-check
>>> +
>>>  wal-check: temp-install
>>>         $(prove_check)
>>
>>
>> I've tried this version Makefile.  And I've seen the only difference: when
>> tap tests are enabled, this version of Makefile runs tap tests before
>> regression tests.  While my version of Makefile runs tap tests after
>> regression tests.  That seems like more desirable behavior for me.  But it
>> would be sill nice to simplify Makefile.
>
> Why do you care about the order of actions? There is no dependency
> between each test and it seems to me that it should remain so to keep
> a maximum flexibility. This does not sacrifice coverage as well. In
> short, Sawada-san's suggestion looks like a thing to me. One piece
> that it is missing though is that installcheck triggers only the
> SQL-based tests, and it should also trigger the TAP tests.

+1

> So I think
> that you should instead do something like that:
>
> --- a/contrib/bloom/Makefile
> +++ b/contrib/bloom/Makefile
> @@ -20,5 +20,12 @@ include $(top_builddir)/src/Makefile.global
>  include $(top_srcdir)/contrib/contrib-global.mk
>  endif
>
> +installcheck: wal-installcheck
> +
> +check: wal-check
> +
> +wal-installcheck:
> +   $(prove_installcheck)
> +
>  wal-check: temp-install
>     $(prove_check)
>
> This works for me as I would expect it should.

Looks good to me too.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 9, 2017 at 7:51 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Wed, Nov 8, 2017 at 5:46 AM, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
>> > So I think
>> > that you should instead do something like that:
>> >
>> > --- a/contrib/bloom/Makefile
>> > +++ b/contrib/bloom/Makefile
>> > @@ -20,5 +20,12 @@ include $(top_builddir)/src/Makefile.global
>> >  include $(top_srcdir)/contrib/contrib-global.mk
>> >  endif
>> >
>> > +installcheck: wal-installcheck
>> > +
>> > +check: wal-check
>> > +
>> > +wal-installcheck:
>> > +   $(prove_installcheck)
>> > +
>> >  wal-check: temp-install
>> >     $(prove_check)
>> >
>> > This works for me as I would expect it should.
>>
>> Looks good to me too.
>
> OK, then so be it :)

Thanks for the new version. This one, as well as the switch to
psql_safe in https://www.postgresql.org/message-id/CAPpHfduxgEYF_0BTs-mxGC4=w5sw8rnUbq9BSTp1Wq7=NwrWDA@mail.gmail.com
are good for a committer lookup IMO.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Nov 9, 2017 at 7:51 PM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>> OK, then so be it :)

> Thanks for the new version. This one, as well as the switch to
> psql_safe in https://www.postgresql.org/message-id/CAPpHfduxgEYF_0BTs-mxGC4=w5sw8rnUbq9BSTp1Wq7=NwrWDA@mail.gmail.com
> are good for a committer lookup IMO.

The safe_psql change is a clear bug fix, so I've pushed it.

However, as far as adding the TAP test to the standard test suite
goes, I've got two complaints about this patch:

1. It doesn't (I think, can't test) do anything to run the test on
Windows.

2. The TAP test is enormously slower than the standard test.  On my
development workstation, "make installcheck" in contrib/bloom takes
about 0.5 sec; this patch increases that to 11.6 sec.  I'm not too
happy about that as a developer, and even less so as an owner of some
fairly slow buildfarm critters.  I'd say that this might be the
reason the TAP test wasn't part of the standard tests to begin with.

Is there anything we can do to cut the runtime of the TAP test to
the point where running it by default wouldn't be so painful?
        regards, tom lane


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:
> Is there anything we can do to cut the runtime of the TAP test to
> the point where running it by default wouldn't be so painful?

As an experiment, I tried simply cutting the size of the test table 10X:

diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
index 1b319c9..566abf9 100644
--- a/contrib/bloom/t/001_wal.pl
+++ b/contrib/bloom/t/001_wal.pl
@@ -57,7 +57,7 @@ $node_standby->start;$node_master->safe_psql("postgres", "CREATE EXTENSION
bloom;");$node_master->safe_psql("postgres","CREATE TABLE tst (i int4, t text);");$node_master->safe_psql("postgres", 
-"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,100000) i;"
+"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,10000)
i;");$node_master->safe_psql("postgres",   "CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);"); 
@@ -72,7 +72,7 @@ for my $i (1 .. 10)    test_index_replay("delete $i");    $node_master->safe_psql("postgres", "VACUUM
tst;");   test_index_replay("vacuum $i"); 
-    my ($start, $end) = (100001 + ($i - 1) * 10000, 100000 + $i * 10000);
+    my ($start, $end) = (10001 + ($i - 1) * 1000, 10000 + $i * 1000);    $node_master->safe_psql("postgres","INSERT
INTOtst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series($start,$end) i;"    ); 

This about halved the runtime of the TAP test, and it changed the coverage
footprint not at all according to lcov.  (Said coverage is only marginally
better than what we get without running the bloom TAP test, AFAICT.)

It seems like some effort could be put into both shortening this test
and improving the amount of code it exercises.
        regards, tom lane


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 13, 2017 at 7:13 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Fri, Nov 10, 2017 at 9:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wrote:
>> > Is there anything we can do to cut the runtime of the TAP test to
>> > the point where running it by default wouldn't be so painful?
>>
>> As an experiment, I tried simply cutting the size of the test table 10X:
>>
>> diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
>> index 1b319c9..566abf9 100644
>> --- a/contrib/bloom/t/001_wal.pl
>> +++ b/contrib/bloom/t/001_wal.pl
>> @@ -57,7 +57,7 @@ $node_standby->start;
>>  $node_master->safe_psql("postgres", "CREATE EXTENSION bloom;");
>>  $node_master->safe_psql("postgres", "CREATE TABLE tst (i int4, t
>> text);");
>>  $node_master->safe_psql("postgres",
>> -"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series(1,100000) i;"
>> +"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series(1,10000) i;"
>>  );
>>  $node_master->safe_psql("postgres",
>>         "CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 =
>> 3);");
>> @@ -72,7 +72,7 @@ for my $i (1 .. 10)
>>         test_index_replay("delete $i");
>>         $node_master->safe_psql("postgres", "VACUUM tst;");
>>         test_index_replay("vacuum $i");
>> -       my ($start, $end) = (100001 + ($i - 1) * 10000, 100000 + $i *
>> 10000);
>> +       my ($start, $end) = (10001 + ($i - 1) * 1000, 10000 + $i * 1000);
>>         $node_master->safe_psql("postgres",
>>  "INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series($start,$end) i;"
>>         );
>>
>> This about halved the runtime of the TAP test, and it changed the coverage
>> footprint not at all according to lcov.  (Said coverage is only marginally
>> better than what we get without running the bloom TAP test, AFAICT.)
>>
>> It seems like some effort could be put into both shortening this test
>> and improving the amount of code it exercises.
>
> Thank you for committing patch which fixes tap test.
> I'll try to improve coverage of this test and reduce its run time.

I am marking this CF entry as committed, as the switch to psql_safe
has been committed. In order to improve the run time and coverage of
the tests. Let's spawn a new thread.
-- 
Michael