From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz
Cc: simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
Date: Wed, 25 Dec 2019 20:28:04 +0300
Message-ID: <24b9a157-d95e-96c2-2fde-9cf73f05008c@postgrespro.ru> (raw)
In-Reply-To: <ada09c82-df58-8204-9296-283514adeb6e@postgrespro.ru>
References: <059cc53a-8b14-653a-a24d-5f867503b0ee@postgrespro.ru>
<20191225.130337.442597881772691026.horikyota.ntt@gmail.com>
<ada09c82-df58-8204-9296-283514adeb6e@postgrespro.ru>
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
Attachments:
[text/x-patch] v2-0001-Make-physical-replslot-advance-persistent.patch (1.4K, ../24b9a157-d95e-96c2-2fde-9cf73f05008c@postgrespro.ru/2-v2-0001-Make-physical-replslot-advance-persistent.patch)
download | inline diff:
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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, horikyota.ntt@gmail.com, michael@paquier.xyz, simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
In-Reply-To: <24b9a157-d95e-96c2-2fde-9cf73f05008c@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox