MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_b69b40b38c7150409d8b94f807a61f28"
Content-Transfer-Encoding: 7bit
Date: Tue, 09 Jun 2020 21:01:13 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>, Dimitri Fontaine
 <dim@tapoueh.org>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 pgsql-hackers@postgresql.org, simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
In-Reply-To: <20200609171904.kpltxxvjzislidks@alap3.anarazel.de>
References: <20191226.173349.2159296357637287391.horikyota.ntt@gmail.com>
 <682b1960-cb89-8738-dbdf-8033662fb80f@postgrespro.ru>
 <175c2760666a78205e053207794c0f8f@postgrespro.ru>
 <20200109.153607.1891950504261689123.horikyota.ntt@gmail.com>
 <298e5b24-2628-3be0-840c-9ed6a45127ed@postgrespro.ru>
 <20200120064540.GA57042@paquier.xyz>
 <20200120190014.h4pvoep2t44foy4y@alap3.anarazel.de>
 <20200121050730.GF2552@paquier.xyz> <20200128080114.GB145179@paquier.xyz>
 <20200609171904.kpltxxvjzislidks@alap3.anarazel.de>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <82163ef4a380c5255e5004659d02559e@postgrespro.ru>
Precedence: bulk

--=_b69b40b38c7150409d8b94f807a61f28
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

On 2020-06-09 20:19, Andres Freund wrote:
> Hi,
> 
> On 2020-01-28 17:01:14 +0900, Michael Paquier wrote:
>> So attached is an updated patch which addresses the problem just by
>> marking a physical slot as dirty if any advancing is done.  Some
>> documentation is added about the fact that an advance is persistent
>> only at the follow-up checkpoint.  And the tests are fixed to not use
>> a fake LSN but instead advance to the latest LSN position produced.
> 
>> 
>> -	/* Update the on disk state when lsn was updated. */
>> -	if (XLogRecPtrIsInvalid(endlsn))
>> -	{
>> -		ReplicationSlotMarkDirty();
>> -		ReplicationSlotsComputeRequiredXmin(false);
>> -		ReplicationSlotsComputeRequiredLSN();
>> -		ReplicationSlotSave();
>> -	}
>> -
> 
> I am quite confused by the wholesale removal of these lines. That 
> wasn't
> in previous versions of the patch. As far as I can tell not calling
> ReplicationSlotsComputeRequiredLSN() for the physical slot leads to the
> global minimum LSN never beeing advanced, and thus WAL reserved by the
> slot not being removable.  Only if there's some independent call to
> ReplicationSlotsComputeRequiredLSN() XLogSetReplicationSlotMinimumLSN()
> will be called, allowing for slots to advance.
> 
> I realize this stuff has been broken since the introduction in
> 9c7d06d6068 (due to the above being if (XLogRecPtrIsInvalid()) rather
> than if (!XLogRecPtrIsInvalid()) , but this seems to make it even 
> worse?
> 

Yes, there was a ReplicationSlotsComputeRequiredLSN() call inside 
pg_physical_replication_slot_advance() in the v5 of the patch:

@@ -370,6 +370,11 @@ pg_physical_replication_slot_advance(XLogRecPtr 
moveto)
  		MyReplicationSlot->data.restart_lsn = moveto;
  		SpinLockRelease(&MyReplicationSlot->mutex);
  		retlsn = moveto;
+
+		ReplicationSlotMarkDirty();
+
+		/* We moved retart_lsn, update the global value. */
+		ReplicationSlotsComputeRequiredLSN();

But later it has been missed and we have not noticed that.

I think that adding it back as per attached will be enough.

> 
> 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.


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company
--=_b69b40b38c7150409d8b94f807a61f28
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=fix-ph-repl-slot-advance-lsn-recompute.diff
Content-Disposition: attachment;
 filename=fix-ph-repl-slot-advance-lsn-recompute.diff;
 size=545

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3RmdW5jcy5jIGIvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vc2xvdGZ1bmNzLmMKaW5kZXggMWI5MjlhNjAzZS4uOGU1NDNlMjc2
ZiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vc2xvdGZ1bmNzLmMKKysrIGIv
c3JjL2JhY2tlbmQvcmVwbGljYXRpb24vc2xvdGZ1bmNzLmMKQEAgLTQxOSw2ICs0MTksMTEgQEAg
cGdfcGh5c2ljYWxfcmVwbGljYXRpb25fc2xvdF9hZHZhbmNlKFhMb2dSZWNQdHIgbW92ZXRvKQog
CQkgKiBjcmFzaCwgYnV0IHRoaXMgbWFrZXMgdGhlIGRhdGEgY29uc2lzdGVudCBhZnRlciBhIGNs
ZWFuIHNodXRkb3duLgogCQkgKi8KIAkJUmVwbGljYXRpb25TbG90TWFya0RpcnR5KCk7CisKKwkJ
LyoKKwkJICogV2UgYWR2YW5jZWQgcmV0YXJ0X2xzbiwgdXBkYXRlIHRoZSBnbG9iYWwgbWluaW11
bSByZXF1aXJlZCB2YWx1ZS4KKwkJICovCisJCVJlcGxpY2F0aW9uU2xvdHNDb21wdXRlUmVxdWly
ZWRMU04oKTsKIAl9CiAKIAlyZXR1cm4gcmV0bHNuOwo=
--=_b69b40b38c7150409d8b94f807a61f28--