Subject: Re: Physical replication slot advance is not persistent
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org, michael@paquier.xyz, simon@2ndquadrant.com
References: <059cc53a-8b14-653a-a24d-5f867503b0ee@postgrespro.ru>
 <20191225.130337.442597881772691026.horikyota.ntt@gmail.com>
 <ada09c82-df58-8204-9296-283514adeb6e@postgrespro.ru>
Message-ID: <24b9a157-d95e-96c2-2fde-9cf73f05008c@postgrespro.ru>
Date: Wed, 25 Dec 2019 20:28:04 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <ada09c82-df58-8204-9296-283514adeb6e@postgrespro.ru>
Content-Type: multipart/mixed;
 boundary="------------DE9D0831EFFA818A7581E569"
Content-Language: en-US
Precedence: bulk

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

On 25.12.2019 16:51, Alexey Kondratov wrote:
> On 25.12.2019 07:03, Kyotaro Horiguchi wrote:
>> As the result I think what is needed there is just checking if the
>> returned lsn is equal or larger than moveto. Doen't the following
>> change work?
>>
>> -    if (XLogRecPtrIsInvalid(endlsn))
>> +    if (moveto <= endlsn)
>
> Yep, it helps with physical replication slot persistence after 
> advance, but the whole validation (moveto <= endlsn) does not make 
> sense for me. The value of moveto should be >= than minlsn == 
> confirmed_flush / restart_lsn, while endlsn == retlsn is also always 
> initialized with confirmed_flush / restart_lsn. Thus, your condition 
> seems to be true in any case, even if it was no-op one, which we were 
> intended to catch.
>
> I will recheck everything again and try to come up with something 
> during this week.

If I get it correctly, then we already keep previous slot position in 
the minlsn, so we just have to compare endlsn with minlsn and treat 
endlsn <= minlsn as a no-op without slot state flushing.

Attached is a patch that does this, so it fixes the bug without 
affecting any user-facing behavior. Detailed comment section and DEBUG 
output are also added. What do you think now?

I have also forgotten to mention that all versions down to 11.0 should 
be affected with this bug.


Regards

-- 
Alexey Kondratov

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


--------------DE9D0831EFFA818A7581E569
Content-Type: text/x-patch; charset=UTF-8;
 name="v2-0001-Make-physical-replslot-advance-persistent.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v2-0001-Make-physical-replslot-advance-persistent.patch"

From e08299ddf92abc3fb4e802e8b475097fa746c458 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Wed, 25 Dec 2019 20:12:42 +0300
Subject: [PATCH v2] Make physical replslot advance persistent

---
 src/backend/replication/slotfuncs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 6683fc3f9b..bc5c93b089 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -573,9 +573,17 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
 	values[0] = NameGetDatum(&MyReplicationSlot->data.name);
 	nulls[0] = false;
 
-	/* Update the on disk state when lsn was updated. */
-	if (XLogRecPtrIsInvalid(endlsn))
+	/*
+	 * Update the on disk state when LSN was updated.  Here we rely on the facts
+	 * that: 1) minlsn is initialized with restart_lsn and confirmed_flush LSN for
+	 * physical and logical replication slot respectively, and 2) endlsn is set in
+	 * the same way by pg_*_replication_slot_advance, but after advance.  Thus,
+	 * endlsn <= minlsn is treated as a no-op.
+	 */
+	if (endlsn > minlsn)
 	{
+		elog(DEBUG1, "flushing replication slot '%s' state",
+							NameStr(MyReplicationSlot->data.name));
 		ReplicationSlotMarkDirty();
 		ReplicationSlotsComputeRequiredXmin(false);
 		ReplicationSlotsComputeRequiredLSN();

base-commit: 8ce3aa9b5914d1ac45ed3f9bc484f66b3c4850c7
-- 
2.17.1


--------------DE9D0831EFFA818A7581E569--