Thread: [PATCH] PostgresNode.pm enhancements, pg_lsn helper, and some more recovery tests

Every test I write with the TAP framework for recovery seems to need
to wait for one node to catch up to another or examine replication
slot state. So: attached is a patch to add helper methods for these
tasks.

Also add a new pg_lsn.pm helper class to deal with PostgreSQL's
awkward representation of LSNs in perl. Made extra fun by our use of
Perl 5.8.8 with no new modules, so we can't rely on having 64-bit
integers. Provides sensible LSN comparisons. I'll be using this in
coming patches that have to make assertions about differences between
LSNs, and I think it's sensible to have anyway. Incorporates tests for
the class.

Finally, add some coverage of physical replication slots to recovery tests.

Backpatch to 9.6 desirable, since we seem to be doing that for TAP
infrastructure.

These three are related enough, and all only touch the TAP framework,
so I've bundled them in a series.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 14 November 2016 at 15:01, Craig Ringer <craig@2ndquadrant.com> wrote:

> These three are related enough, and all only touch the TAP framework,
> so I've bundled them in a series.

CF entry https://commitfest.postgresql.org/12/883/

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Hello, Craig.

I noticed that these patches have a "Needs review" status and decided to
take a look. Surprisingly I didn't manage to find many defects.

* I like this idea in general;
* Code is test covered and documented well;
* All tests pass;
* No warnings during compilation and/or tests run;
* I didn't find any typos;
* Code style seems to be OK;

Though are you sure you want to name a class like_this instead of LikeThis?
It's quite uncommon in Perl code and usually used only for packages like
"strict" and "warnings". However I prefer no to insist on changes like
this since it's a matter of taste.

The bottom line --- this code looks good to me. If there is nothing you
would like to change in the last moment I suggest to change the status
to "Ready for committer".

--
Best regards,
Aleksander Alekseev

On 14 November 2016 at 07:01, Craig Ringer <craig@2ndquadrant.com> wrote:
> Every test I write with the TAP framework for recovery seems to need
> to wait for one node to catch up to another or examine replication
> slot state. So: attached is a patch to add helper methods for these
> tasks.
>
> Also add a new pg_lsn.pm helper class to deal with PostgreSQL's
> awkward representation of LSNs in perl. Made extra fun by our use of
> Perl 5.8.8 with no new modules, so we can't rely on having 64-bit
> integers. Provides sensible LSN comparisons. I'll be using this in
> coming patches that have to make assertions about differences between
> LSNs, and I think it's sensible to have anyway. Incorporates tests for
> the class.
>
> Finally, add some coverage of physical replication slots to recovery tests.
>
> Backpatch to 9.6 desirable, since we seem to be doing that for TAP
> infrastructure.
>
> These three are related enough, and all only touch the TAP framework,
> so I've bundled them in a series.

Good to see more tests going in.

Bit confused... can't see a caller for wait_for_slot_catchup() and the
slot tests don't call it AFAICS.

Also can't see anywhere the LSN stuff is used either.

No specific problems with the code, just don't want to commit code
that isn't used anywhere, yet.

I want to commit the extra tests soon, as a stronger test for my
recovery.conf changes.

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

On 2 January 2017 at 20:17, Simon Riggs <simon@2ndquadrant.com> wrote:

> Bit confused... can't see a caller for wait_for_slot_catchup() and the
> slot tests don't call it AFAICS.

The recovery tests for decoding on standby will use it. I can delay
adding it until then.

> Also can't see anywhere the LSN stuff is used either.

Removed after discussion with Michael in the logical decoding on standby thread.

I'll be posting a new patch series there shortly, which removes
pg_lsn.pm and modifies the PostgresNode.pm changes per discussion. If
you're able to commit the updated PostgresNode.pm and new standby
tests that'd be very handy.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 3 January 2017 at 12:36, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 2 January 2017 at 20:17, Simon Riggs <simon@2ndquadrant.com> wrote:
>
>> Bit confused... can't see a caller for wait_for_slot_catchup() and the
>> slot tests don't call it AFAICS.
>
> The recovery tests for decoding on standby will use it. I can delay
> adding it until then.
>
>> Also can't see anywhere the LSN stuff is used either.
>
> Removed after discussion with Michael in the logical decoding on standby thread.
>
> I'll be posting a new patch series there shortly, which removes
> pg_lsn.pm and modifies the PostgresNode.pm changes per discussion. If
> you're able to commit the updated PostgresNode.pm and new standby
> tests that'd be very handy.

To keep things together, I've followed up on the logical decoding on
standby thread that now incorporates all these patches.


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 4 January 2017 at 12:39, Craig Ringer <craig@2ndquadrant.com> wrote:

> To keep things together, I've followed up on the logical decoding on
> standby thread that now incorporates all these patches.

Attached are the two patches discussed upthread, in their
proposed-for-commit form, as requested by Simon.

These correspond to patches 0001 and 0004 of the logical decoding on
standby series at
https://www.postgresql.org/message-id/CAMsr+YEzC=-+eV09A=ra150FjtkmTqT5Q70PiqBwytbOR3cshg@mail.gmail.com
.

This subset is tracked as https://commitfest.postgresql.org/12/883/ .

When committed I will update the decoding on standby series to omit
these pre-requisite patches.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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 4 January 2017 at 08:16, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 4 January 2017 at 12:39, Craig Ringer <craig@2ndquadrant.com> wrote:
>
>> To keep things together, I've followed up on the logical decoding on
>> standby thread that now incorporates all these patches.
>
> Attached are the two patches discussed upthread, in their
> proposed-for-commit form, as requested by Simon.
>
> These correspond to patches 0001 and 0004 of the logical decoding on
> standby series at
> https://www.postgresql.org/message-id/CAMsr+YEzC=-+eV09A=ra150FjtkmTqT5Q70PiqBwytbOR3cshg@mail.gmail.com
> .
>
> This subset is tracked as https://commitfest.postgresql.org/12/883/ .
>
> When committed I will update the decoding on standby series to omit
> these pre-requisite patches.

Committed, thanks.

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

On Thu, Jan 5, 2017 at 1:58 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 4 January 2017 at 08:16, Craig Ringer <craig@2ndquadrant.com> wrote:
>> When committed I will update the decoding on standby series to omit
>> these pre-requisite patches.
>
> Committed, thanks.

I was planning a round of reviews of those patches, but you were faster than me.

So now looking at what has been committed.

The perldoc documentation is broken for the new routines. The commits
have added patterns like that:
=pod $node->routine_name
But what needs to be done is to use =pod and =item, like that:
=pod
=item $node->routine_name

+Look up xlog positions on the server:
This has better be "*WAL* positions". There is another reference with
xlog (see recent threads about renaming those functions for example).

+* insert position (master only, error on replica)
+* write position (master only, error on replica)
+* flush position
+* receive position (always undef on master)
+* replay position
Replay position is always undefined on primary, let's document it in
the description of the routine. And flush position generates an error
for a standby.

The documentation of $node->lsn is generated like that, with a very
unfriendly list of modes:
    $node->lsn(mode)
        Look up xlog positions on the server:

        * insert position (master only, error on replica) * write position
        (master only, error on replica) * flush position * receive position
        (always undef on master) * replay position
A trick that I have found here is to add a space before the '*'.

It may be a good idea to run perltidy on top of that to be honest...

Attached is a patch to fix all those small issues.
-- 
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 5 January 2017 at 04:50, Michael Paquier <michael.paquier@gmail.com> wrote:

> The perldoc documentation is broken for the new routines.
...
> Attached is a patch to fix all those small issues.

Thanks, looks good, will apply.


> It may be a good idea to run perltidy on top of that to be honest...

Should we add that to the makefile? I guess start a new thread if you think so.

Anything that helps me check its correct is a good thing AFAICS.

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