Re: Logical decoding on standby

On 22 December 2016 at 13:43, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> So, for 0001:
> --- a/src/test/perl/PostgresNode.pm
> +++ b/src/test/perl/PostgresNode.pm
> @@ -93,6 +93,7 @@ use RecursiveCopy;
> use Socket;
> use Test::More;
> use TestLib ();
> +use pg_lsn qw(parse_lsn);
> use Scalar::Util qw(blessed);
> This depends on 0002, so the order should be reversed.

Will do. That was silly.

I think I should probably also move the standby tests earlier, then
add a patch to update them when the results change.

> +sub lsn
> +{
> + my $self = shift;
> + return $self->safe_psql('postgres', 'select case when
> pg_is_in_recovery() then pg_last_xlog_replay_location() else
> pg_current_xlog_insert_location() end as lsn;');
> +}
> The design of the test should be in charge of choosing which value it
> wants to get, and the routine should just blindly do the work. More
> flexibility is more useful to design tests. So it would be nice to
> have one routine able to switch at will between 'flush', 'insert',
> 'write', 'receive' and 'replay modes to get values from the
> corresponding xlog functions.

Fair enough. I can amend that.

> - die "error running SQL: '$$stderr'\nwhile running '@psql_params'"
> + die "error running SQL: '$$stderr'\nwhile running
> '@psql_params' with sql '$sql'"
> if $ret == 3;
> That's separate from this patch, and definitely useful.

Yeah. Slipped through. I don't think it really merits a separate patch
though tbh.

> + if (!($mode eq 'restart' || $mode eq 'confirmed_flush')) {
> + die "valid modes are restart, confirmed_flush";
> + }
> + if (!defined($target_lsn)) {
> + $target_lsn = $self->lsn;
> + }
> That's not really the style followed by the perl scripts present in
> the code regarding the use of the brackets. Do we really need to care
> about the object type checks by the way?

Brackets: will look / fix.

Type checks (not in quoted snippet above): that's a convenience to let
you pass a PostgresNode instance or a string name. Maybe there's a
more idiomatic Perl-y way to write it. My Perl is pretty dire.

> Regarding wait_for_catchup, there are two ways to do things. Either
> query the standby like in the way 004_timeline_switch.pl does it or
> the way this routine does. The way of this routine looks more
> straight-forward IMO, and other tests should be refactored to use it.
> In short I would make the target LSN a mandatory argument, and have
> the caller send a standby's application_name instead of a PostgresNode
> object, the current way to enforce the value of $standby_name being
> really confusing.

Hm, ok. I'll take a look. Making LSN mandatory so you have to pass
$self->lsn is ok with me.

> + my %valid_modes = ( 'sent' => 1, 'write' => 1, 'flush' => 1,
> 'replay' => 1 );
> What's actually the point of 'sent'?

Pretty useless, but we expose it in Pg, so we might as well in the tests.

> + my @fields = ('plugin', 'slot_type', 'datoid', 'database',
> 'active', 'active_pid', 'xmin', 'catalog_xmin', 'restart_lsn');
> + my $result = $self->safe_psql('postgres', 'SELECT ' . join(', ',
> @fields) . " FROM pg_catalog.pg_replication_slots WHERE slot_name =
> '$slot_name'");
> + $result = undef if $result eq '';
> + # hash slice, see http://stackoverflow.com/a/16755894/398670 .
> Couldn't this portion be made more generic? Other queries could
> benefit from that by having a routine that accepts as argument an
> array of column names for example.

Yeah, probably. I'm not sure where it should live though - TestLib.pm ?

Not sure if there's an idomatic way to pass a string (in this case
queyr) in Perl with a placeholder for interpolation of values (in this
case columns). in Python you'd pass it with pre-defined
%(placeholders)s for %.

> Now looking at 0002....
> One whitespace:
> $ git diff HEAD~1 --check
> src/test/perl/pg_lsn.pm:139: trailing whitespace.
> +=cut

Will fix.

> pg_lsn sounds like a fine name, now we are more used to camel case for
> module names. And routines are written as lower case separated by an
> underscore.

Unsure what the intent of this is.

> +++ b/src/test/perl/t/002_pg_lsn.pl
> @@ -0,0 +1,68 @@
> +use strict;
> +use warnings;
> +use Test::More tests => 42;
> +use Scalar::Util qw(blessed);
> Most of the tests added don't have a description. This makes things
> harder to debug when tracking an issue.
>
> It may be good to begin using this module within the other tests in
> this patch as well. Now do we actually need it? Most of the existing
> tests I recall rely on the backend's operators for the pg_lsn data
> type, so this is actually duplicating an exiting facility. And all the
> values are just passed as-is.

I added it mainly for ordered tests of whether some expected lsn had
passed/increased. But maybe it makes sense to just call into the
server and let it evaluate such tests.

> +++ b/src/test/perl/t/001_load.pl
> @@ -0,0 +1,9 @@
> +use strict;
> +use warnings;
> +use Test::More tests => 5;
> I can guess the meaning of this test, having a comment on top of it to
> explain the purpose of the test is good practice though.

Will.

> Looking at 0004...
> +Disallows pg_recvlogial from internally retrying on error by passing --no-loop.
> s/pg_recvlogial/pg_recvlogical

Thanks.

> +sub pg_recvlogical_upto
> +{
> This looks like a good idea for your tests.

Yeah, and likely others too as we start doing more with logical
replication in future.

> +my $endpos = $node_master->safe_psql('postgres', "SELECT location
> FROM pg_logical_slot_peek_changes('test_slot', NULL, NULL) ORDER BY
> location DESC LIMIT 1;");
> +diag "waiting to replay $endpos";
> On the same wave as the pg_recvlogical wrapper, you may want to
> consider some kind of wrapper at SQL level calling the slot functions.

I'd really rather beg off that until needed later. The SQL functions
are simple to invoke from PostgresNode::psql in the mean time; not so
much so with pg_recvlogical.

> And finally 0006.
> +$node_standby_1->append_conf('recovery.conf', "primary_slot_name =
> $slotname_1\n");
> +$node_standby_1->append_conf('postgresql.conf',
> "wal_receiver_status_interval = 1\n");
> +$node_standby_1->append_conf('postgresql.conf', "max_replication_slots = 4\n");
> No need to call multiple times this routine.
>
> Increasing the test coverage is definitely worth it.

Thanks.

I'll follow up with amendments. I've also implemented Petr's
suggestion to allow explicit omission of a snapshot on slot creation.

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