Content-Type: multipart/mixed; boundary="------------SSsu0LYVMtCnDZDtow005KO0"
Message-ID: <a4f477e0-5d17-466b-a8cc-e0d905d1161b@postgrespro.ru>
Date: Wed, 28 May 2025 19:12:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Alexey Makhmutov <a.makhmutov@postgrespro.ru>
Subject: Re: Standby server with cascade logical replication could not be
 properly stopped under load
To: Michael Paquier <michael@paquier.xyz>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
 shveta malik <shveta.malik@gmail.com>, Ajin Cherian <itsajin@gmail.com>,
 pgsql-bugs@lists.postgresql.org
References: <52138028-7246-421c-9161-4fa108b88070@postgrespro.ru>
 <CAJpy0uA9kw9WLWETPg+upwzBu145vMB7sPZwBuR_vDrbazeuag@mail.gmail.com>
 <d9e1c765-7f83-4015-ade3-16506854d0b1@postgrespro.ru>
 <aC9DXHt/MwFCW6Is@ip-10-97-1-34.eu-west-3.compute.internal>
 <f2575904-acfe-4037-9748-61fdb4a7abe4@postgrespro.ru>
 <aDa83q7wJt6II3Iq@paquier.xyz>
Content-Language: en-US
In-Reply-To: <aDa83q7wJt6II3Iq@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/a4f477e0-5d17-466b-a8cc-e0d905d1161b%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------SSsu0LYVMtCnDZDtow005KO0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/28/25 10:35, Michael Paquier wrote:
> Wouldn't it be better to extend GetStandbyFlushRecPtr() so as the
> caller can directly decide if they want to include in the maths the
> last LSN flushed by the WAL receiver, depending on the code path where
> the routine is called?  That makes for a larger patch, of course, but
> it removes the need to look at MyWalSnd inside
> GetStandbyFlushRecPtr().

Well, this is a great question, because it made me thinking whether we 
need to change behavior of THIS function at all. Extending 
'GetStandbyFlushRecPtr' to properly support logical replication will 
just make it equal to the 'GetXLogReplayRecPtr'. So, basically we just 
need to replace invocation of 'GetStandbyFlushRecPtr' with 
'GetXLogReplayRecPtr' and this should solve the problem.

The only invoker of 'GetStandbyFlushRecPtr' outside of walsender.c is 
slot synchronization routine, which expects to get 'physical' position 
anyway, so it does not need to be changed. All other invocations are 
performed from the 'walsender.c' file itself. Obviously, both 
'StartReplication' (physical) and 'XLogSendPhysical' functions need 
current logic, so we can skip them. The 'XLogSendLogical' need to be 
changed to use 'GetXLogReplayRecPtr' (to get consistent behavior with 
WalSndWaitForWal and actually fix the original problem discussed in this 
thread). This leaves us with only single function to consider: 
'IdentifySystem'. This function could be called both in case of physical 
and logical replication, but I assume that we need to return flushed WAL 
position in it as well, as it's described in the documentation for this 
command. This also seems to be the right behavior, as primary goal for 
'identify' command is to inform client about the potential replication 
start point and we definitely can start replication from the flushed 
position (as we already has this record), just with some delay (once our 
recovery get us to this point). But maybe somebody with more knowledge 
of the logical replication expectation could confirm this.

The fix seems to be very simple in this case (assuming the logic above 
is correct): we only replace 'GetStandbyFlushRecPtr' call in 
'XLogSendLogical' with the 'GetXLogReplayRecPtr' and that's it. Attached 
is the modified patch. It could be applied on top of 16, 17 and master 
versions.

As for the test: do you want this case with shutdown to be added to the 
existing test plan for standby logical recovery? The condition to 
trigger the problem is just to have a flushed and replayed positions 
pointing to different values during standby shutdown. I could try to add 
it to the test suit, but it could take some time.

Thanks,
Alexey

--------------SSsu0LYVMtCnDZDtow005KO0
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-Use-only-replayed-position-as-target-flush-point-4-pg-master.patch"
Content-Disposition: attachment;
 filename*0="0001-Use-only-replayed-position-as-target-flush-point-4-pg-m";
 filename*1="aster.patch"
Content-Transfer-Encoding: base64

RnJvbSAxZDkxZTA2N2ZhZDBiYzY4YjU1ZDZkNzcxY2MwMzA3ODY4MWRlNzgwIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBBbGV4ZXkgTWFraG11dG92IDxhLm1ha2htdXRvdkBw
b3N0Z3Jlc3Byby5ydT4KRGF0ZTogV2VkLCAyOCBNYXkgMjAyNSAxNzo0NTo1MiArMDMwMApT
dWJqZWN0OiBbUEFUQ0hdIFVzZSBvbmx5IHJlcGxheWVkIHBvc2l0aW9uIGFzIHRhcmdldCBm
bHVzaCBwb2ludCBmb3IgY2FzY2FkZWQKIGxvZ2ljYWwgcmVwbGljYXRpb24KCkZvciBwaHlz
aWNhbCB3YWxzZW5kZXIgdGhlIEdldFN0YW5kYnlGbHVzaFJlY1B0ciBmdW5jdGlvbiByZXR1
cm5zIHBvc2l0aW9uCm9mIHRoZSBsYXRlc3Qgc3RvcmVkIFdBTCByZWNvcmQsIGFzIHdlIGNh
biBzdHJlYW0gZGF0YSB0byBkb3duc3RyZWFtIGluc3RhbmNlCndpdGhvdXQgd2FpdGluZyBm
b3IgaXQgYmVpbmcgbG9jYWxseSBhcHBsaWVkLiBIb3dldmVyLCBmb3IgY2FzY2FkZWQgbG9n
aWNhbApyZXBsaWNhdGlvbiB3ZSBjYW4gc2VudCBkYXRhIHRvIGRvd25zdHJlYW0gY2xpZW50
IG9ubHkgdXAgdG8gdGhlIGxvY2FsbHkKYXBwbGllZCBwb3NpdGlvbi4gVGhpcyBkaXN0aW5j
dGlvbiBpcyBpbXBvcnRhbnQgd2hlbiB3ZSBkZWNpZGUgd2hldGhlcgp3YWxzZW5kZXIgaGFz
IHNlbnQgYWxsIHRoZSBhdmFpbGFibGUgZGF0YSB0byBjbGllbnQgKCdjYXVnaHQgdXAnKS4g
RHVyaW5nIHRoZQpzaHV0ZG93biBwcm9jZXNzIHdhbHNlbmRlciBiYWNrZW5kcyBhcmUgYWxs
b3dlZCB0byB3b3JrIHVudGlsIHRoZXkgcmVhY2ggdGhlCidjYXVnaHQgdXAnIHN0YXRlLCB3
aGlsZSByZWNvdmVyeSBwcm9jZXNzIGlzIGFscmVhZHkgZGVhY3RpdmF0ZWQsIHNvIHRoZQph
cHBsaWVkIHBvc2l0aW9uIHdvbid0IGJlIG1vdmluZy4gSW4gdGhpcyBjYXNlIHdhbHNlbmRl
ciBmb3IgbG9naWNhbApyZXBsaWNhdGlvbiBzaG91bGQgd29yayBvbmx5IHVudGlsIGl0IGNh
dWdodCB1cCB3aXRoIHRoZSBsYXRlc3QgYXBwbGllZCByZWNvcmQsCm90aGVyd2lzZSBpdCB3
aWxsIHN0dWNrIGluIHRoZSBpbmZpbml0ZSBsb29wIGFuZCBpbmhpYml0IGluc3RhbmNlIHNo
dXRkb3duLgpUbyBhdm9pZCBzdWNoIHByb2JsZW1zIFhMb2dTZW5kTG9naWNhbCBtZXRob2Qg
c2hvdWxkIHVzZSBHZXRYTG9nUmVwbGF5UmVjUHRyCmZ1bmN0aW9uIHJhdGhlciB0aGFuIEdl
dFN0YW5kYnlGbHVzaFJlY1B0ci4KLS0tCiBzcmMvYmFja2VuZC9yZXBsaWNhdGlvbi93YWxz
ZW5kZXIuYyB8IDExICsrKysrKysrKystCiAxIGZpbGUgY2hhbmdlZCwgMTAgaW5zZXJ0aW9u
cygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0
aW9uL3dhbHNlbmRlci5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vd2Fsc2VuZGVyLmMK
aW5kZXggOWZhOGJlYjYxMDMuLmM3ZDQ3NWI5OWI2IDEwMDY0NAotLS0gYS9zcmMvYmFja2Vu
ZC9yZXBsaWNhdGlvbi93YWxzZW5kZXIuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlv
bi93YWxzZW5kZXIuYwpAQCAtMzQ0OSw4ICszNDQ5LDE3IEBAIFhMb2dTZW5kTG9naWNhbCh2
b2lkKQogCWlmIChmbHVzaFB0ciA9PSBJbnZhbGlkWExvZ1JlY1B0ciB8fAogCQlsb2dpY2Fs
X2RlY29kaW5nX2N0eC0+cmVhZGVyLT5FbmRSZWNQdHIgPj0gZmx1c2hQdHIpCiAJeworCQkv
KgorCQkgKiBGb3IgY2FzY2FkZWQgbG9naWNhbCByZXBsaWNhdGlvbiB3ZSBuZWVkIHRvIHVz
ZSByZXBsYXkgcG9zaXRpb24KKwkJICogcmF0aGVyIHRoYW4gZmx1c2hlZCBvbmUsIGFzIHdl
IGNvdWxkIG5vdCBkZWNvZGUgYW5kIHN0cmVhbSBkYXRhCisJCSAqIHdoaWNoIGhhcyBub3Qg
eWV0IGJlZW4gYXBwbGllZCBsb2NhbGx5LiBUaGlzIGRpc3RpbmN0aW9uIGlzCisJCSAqIGVz
cGVjaWFsbHkgaW1wb3JhbnQgZHVyaW5nIHNodXRkb3duIHNlcXVlbmNlLCBhcyBjYXVnaHQg
dXAgc3RhdGUgaXMKKwkJICogY2FsY3VsYXRlZCBiYXNlZCBvbiBmbHVzaFB0ciBhbmQgdXNp
bmcgZmx1c2ggcG9zaXRpb24gaW4gdGhpcworCQkgKiBjb250ZXh0IHdpbGwgY2F1c2UgaW5k
ZWZpbml0ZSB3YWl0IGluIHdhbHNlbmRlciBmb3IgcmVjb3JkcyB3aGljaAorCQkgKiB3b24n
dCBiZSBhcHBsaWVkIGR1ZSB0byBzaHV0ZG93bi4KKwkJICovCiAJCWlmIChhbV9jYXNjYWRp
bmdfd2Fsc2VuZGVyKQotCQkJZmx1c2hQdHIgPSBHZXRTdGFuZGJ5Rmx1c2hSZWNQdHIoTlVM
TCk7CisJCQlmbHVzaFB0ciA9IEdldFhMb2dSZXBsYXlSZWNQdHIoTlVMTCk7CiAJCWVsc2UK
IAkJCWZsdXNoUHRyID0gR2V0Rmx1c2hSZWNQdHIoTlVMTCk7CiAJfQotLSAKMi40OS4wCgo=


--------------SSsu0LYVMtCnDZDtow005KO0--