MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 10 Jun 2020 20:57:17 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: michael@paquier.xyz, andres@anarazel.de, dim@tapoueh.org,
 pgsql-hackers@postgresql.org, simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
In-Reply-To: <20200610.173844.78906193765823414.horikyota.ntt@gmail.com>
References: <20200609171904.kpltxxvjzislidks@alap3.anarazel.de>
 <82163ef4a380c5255e5004659d02559e@postgrespro.ru>
 <20200610065353.GA358163@paquier.xyz>
 <20200610.173844.78906193765823414.horikyota.ntt@gmail.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <876a782d320a52b0c54d6d48372de17d@postgrespro.ru>
Precedence: bulk

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