Re: Physical replication slot advance is not persistent

On 2020-06-10 11:38, Kyotaro Horiguchi wrote:
> At Wed, 10 Jun 2020 15:53:53 +0900, Michael Paquier
> <michael@paquier.xyz> wrote in
>> > > I find it really depressing how much obviously untested stuff gets
>> > > added in this area.
>> >
>> > Prior to this patch pg_replication_slot_advance was not being tested
>> > at all.
>> > Unfortunately, added tests appeared to be not enough to cover all
>> > cases. It
>> > seems that the whole machinery of WAL holding and trimming is worth
>> > to be
>> > tested more thoroughly.
>> 
>> I think that it would be interesting if we had a SQL representation of
>> the contents of XLogCtlData (wanted that a couple of times).  Now we
>> are actually limited to use a checkpoint and check that past segments
>> are getting recycled by looking at the contents of pg_wal.  Doing that
>> here does not cause the existing tests to be much more expensive as we
>> only need one extra call to pg_switch_wal(), mostly.  Please see the
>> attached.
> 
> The test in the patch looks fine to me and worked well for me.
> 
> Using smaller wal_segment_size (1(MB) worked for me) reduces the cost
> of the check, but I'm not sure it's worth doing.
> 

New test reproduces this issue well. Left it running for a couple of 
hours in repeat and it seems to be stable.

Just noted that we do not need to keep $phys_restart_lsn_pre:

my $phys_restart_lsn_pre = $node_master->safe_psql('postgres',
	"SELECT restart_lsn from pg_replication_slots WHERE slot_name = 
'$phys_slot';"
);
chomp($phys_restart_lsn_pre);

we can safely use $current_lsn used for pg_replication_slot_advance(), 
since reatart_lsn is set as is there. It may make the test a bit simpler 
as well.


-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company