Обсуждение: [HACKERS] Coverage improvements of src/bin/pg_basebackup and pg_receivewal --endpos

Hi all,

The coverage of pg_basebackup is reaching 50%, which is not bad:
https://coverage.postgresql.org/src/bin/pg_basebackup/index.html
In this set pg_receivewal.c is the bad student with less than 20%.

There are a couple of causes behind that, with no tests like:
- option interactions like --create-slot and --drop-slot.
- replication slot drop.
- end of streaming.
- check handling of history files.
- looking at compressed and uncompressed WAL data.
- restart of pg_receivewal on an existing folder.
- pg_basebackup's tests don't use the progress report.

While it is possible to tackle some of those issues independently,
like pg_basebackup stuff, it seems to me that it would be helpful to
make the tests more deterministic by having an --endpos option for
pg_receivewal, similarly to what exists already in pg_recvlogical.

Thoughts?
-- 
Michael

On Wed, Jun 7, 2017 at 9:20 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> While it is possible to tackle some of those issues independently,
> like pg_basebackup stuff, it seems to me that it would be helpful to
> make the tests more deterministic by having an --endpos option for
> pg_receivewal, similarly to what exists already in pg_recvlogical.
>
> Thoughts?

I have just played with that, and attached is a patch to implement the
so-said option with a basic set of tests, increasing code coverage of
pg_receivewal.c from 15% to 60%. I'll park that in the next CF of
September.
-- 
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 6/9/17 02:08, Michael Paquier wrote:
> On Wed, Jun 7, 2017 at 9:20 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> While it is possible to tackle some of those issues independently,
>> like pg_basebackup stuff, it seems to me that it would be helpful to
>> make the tests more deterministic by having an --endpos option for
>> pg_receivewal, similarly to what exists already in pg_recvlogical.
>>
>> Thoughts?
> 
> I have just played with that, and attached is a patch to implement the
> so-said option with a basic set of tests, increasing code coverage of
> pg_receivewal.c from 15% to 60%. I'll park that in the next CF of
> September.

Looks like a good idea.  A couple of thoughts:

- I think the tests should be written in the style of
$node->command_fails instead of just command_fails.  At least, that's
how it's done in the pg_basebackup tests.

- I think the tests will fail if libz support is not built.  Again,
pg_basebackup doesn't have tests for that.  So maybe omit that and
address it later.

- The variable exit_on_stop seems redundant with time_to_abort.  They do
the same thing, so maybe your patch could reuse it.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 5, 2017 at 10:19 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 6/9/17 02:08, Michael Paquier wrote:
>> I have just played with that, and attached is a patch to implement the
>> so-said option with a basic set of tests, increasing code coverage of
>> pg_receivewal.c from 15% to 60%. I'll park that in the next CF of
>> September.
>
> Looks like a good idea.  A couple of thoughts:

Thanks.

> - I think the tests should be written in the style of
> $node->command_fails instead of just command_fails.  At least, that's
> how it's done in the pg_basebackup tests.

Good idea.

> - I think the tests will fail if libz support is not built.  Again,
> pg_basebackup doesn't have tests for that.  So maybe omit that and
> address it later.

Let's address it now. This can be countered by querying pg_config()
before running the command of pg_receivexwal which uses --compress.

> - The variable exit_on_stop seems redundant with time_to_abort.  They do
> the same thing, so maybe your patch could reuse it.

Yes, that's doable. time_to_abort does not seem a variable name
adapted to me though if both are combined, so I have renamed that to
time_to_stop, which maps more with the fact that stop can be also
willingly wanted without a SIGINT.

Attached is a new version of the patch.
-- 
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 9/6/17 00:54, Michael Paquier wrote:
>> - I think the tests will fail if libz support is not built.  Again,
>> pg_basebackup doesn't have tests for that.  So maybe omit that and
>> address it later.
> 
> Let's address it now. This can be countered by querying pg_config()
> before running the command of pg_receivexwal which uses --compress.

I have committed this patch, after a perltidy run, but the way the libz
detection was implemented was a bit too hackish for me, so I have
omitted that for now.  I think a more robust way would be to parse
Makefile.global, perhaps by a function in TestLib, so it can be reused
in other tests.  (Even more robust would be to write out an actual Perl
file with configuration information somewhere, but maybe we don't need
to go there yet.)  Or maybe have the respective make variables exported
when make check is called (could be included in the prove_check
variable?).  Anyway, some more pondering could lead to a more general
solution.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

On Tue, Sep 12, 2017 at 5:57 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I have committed this patch, after a perltidy run, but the way the libz
> detection was implemented was a bit too hackish for me, so I have
> omitted that for now.

Thanks.

> I think a more robust way would be to parse
> Makefile.global, perhaps by a function in TestLib, so it can be reused
> in other tests.  (Even more robust would be to write out an actual Perl
> file with configuration information somewhere, but maybe we don't need
> to go there yet.)  Or maybe have the respective make variables exported
> when make check is called (could be included in the prove_check
> variable?).  Anyway, some more pondering could lead to a more general
> solution.

There is always room for improvement,
-- 
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 9/11/17 18:21, Michael Paquier wrote:
> On Tue, Sep 12, 2017 at 5:57 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> I have committed this patch, after a perltidy run, but the way the libz
>> detection was implemented was a bit too hackish for me, so I have
>> omitted that for now.
> 
> Thanks.
> 
>> I think a more robust way would be to parse
>> Makefile.global, perhaps by a function in TestLib, so it can be reused
>> in other tests.  (Even more robust would be to write out an actual Perl
>> file with configuration information somewhere, but maybe we don't need
>> to go there yet.)  Or maybe have the respective make variables exported
>> when make check is called (could be included in the prove_check
>> variable?).  Anyway, some more pondering could lead to a more general
>> solution.
> 
> There is always room for improvement,

I interpret that as that you are not working on that right now, so I've
closed this patch.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

On Tue, Sep 12, 2017 at 9:19 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/11/17 18:21, Michael Paquier wrote:
>> On Tue, Sep 12, 2017 at 5:57 AM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> I think a more robust way would be to parse
>>> Makefile.global, perhaps by a function in TestLib, so it can be reused
>>> in other tests.  (Even more robust would be to write out an actual Perl
>>> file with configuration information somewhere, but maybe we don't need
>>> to go there yet.)  Or maybe have the respective make variables exported
>>> when make check is called (could be included in the prove_check
>>> variable?).  Anyway, some more pondering could lead to a more general
>>> solution.
>>
>> There is always room for improvement,
>
> I interpret that as that you are not working on that right now, so I've
> closed this patch.

Indeed, I have no plans for that now. There are enough patches to look
at these days. My apologies for not making that clear.
-- 
Michael


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