From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: andres@anarazel.de
Cc: dim@tapoueh.org
Cc: pgsql-hackers@postgresql.org
Cc: simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
Date: Thu, 18 Jun 2020 21:46:28 +0300
Message-ID: <0a14a258081aea401222498d402d129b@postgrespro.ru> (raw)
In-Reply-To: <20200616072727.GA2361@paquier.xyz>
References: <20200609171904.kpltxxvjzislidks@alap3.anarazel.de>
<82163ef4a380c5255e5004659d02559e@postgrespro.ru>
<20200610065353.GA358163@paquier.xyz>
<20200610.173844.78906193765823414.horikyota.ntt@gmail.com>
<876a782d320a52b0c54d6d48372de17d@postgrespro.ru>
<20200616072727.GA2361@paquier.xyz>
On 2020-06-16 10:27, Michael Paquier wrote:
> On Wed, Jun 10, 2020 at 08:57:17PM +0300, Alexey Kondratov wrote:
>> New test reproduces this issue well. Left it running for a couple of
>> hours
>> in repeat and it seems to be stable.
>
> Thanks for testing. I have been thinking about the minimum xmin and
> LSN computations on advancing, and actually I have switched the
> recomputing to be called at the end of pg_replication_slot_advance().
> This may be a waste if no advancing is done, but it could also be an
> advantage to enforce a recalculation of the thresholds for each
> function call. And that's more consistent with the slot copy, drop
> and creation.
>
Sorry for a bit late response, but I see a couple of issues with this
modified version of the patch in addition to the waste call if no
advancing is done, mentioned by you:
1. Both ReplicationSlotsComputeRequiredXmin() and
ReplicationSlotsComputeRequiredLSN() may have already been done in the
LogicalConfirmReceivedLocation() if it was a logical slot. It may be
fine and almost costless to do it twice, but it looks untidy for me.
2. It seems that we do not need ReplicationSlotsComputeRequiredXmin() at
all if it was a physical slot, since we do not modify xmin in the
pg_physical_replication_slot_advance(), doesn't it?
That's why I wanted (somewhere around v5 of the patch in this thread) to
move all dirtying and recomputing calls to the places, where xmin / lsn
slot modifications are actually done —
pg_physical_replication_slot_advance() and
LogicalConfirmReceivedLocation(). LogicalConfirmReceivedLocation()
already does this, so we only needed to teach
pg_physical_replication_slot_advance() to do the same.
However, just noted that LogicalConfirmReceivedLocation() only does
ReplicationSlotsComputeRequiredLSN() if updated_xmin flag was set, which
looks wrong from my perspective, since updated_xmin and updated_restart
flags are set separately.
That way, I would solve this all as per attached, which works well for
me, but definitely worth of a better testing.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Attachments:
[text/x-diff] slots-advancing.diff (1.6K, ../0a14a258081aea401222498d402d129b@postgrespro.ru/2-slots-advancing.diff)
download | inline diff:diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.cindex 61902be3b0..2ceb078418 100644--- a/src/backend/replication/logical/logical.c+++ b/src/backend/replication/logical/logical.c@@ -1081,8 +1081,14 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
SpinLockRelease(&MyReplicationSlot->mutex);
ReplicationSlotsComputeRequiredXmin(false);
- ReplicationSlotsComputeRequiredLSN();
}
++ /*+ * Now recompute the minimum required LSN across all slots if+ * restart_lsn of the slot has been updated.+ */+ if (updated_restart)+ ReplicationSlotsComputeRequiredLSN();
}
else
{
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.cindex 06e4955de7..d9c19c07e5 100644--- a/src/backend/replication/slotfuncs.c+++ b/src/backend/replication/slotfuncs.c@@ -419,6 +419,12 @@ pg_physical_replication_slot_advance(XLogRecPtr moveto)
* crash, but this makes the data consistent after a clean shutdown.
*/
ReplicationSlotMarkDirty();
++ /*+ * Recompute the minimum required LSN across all slots to adjust with+ * the advancing done.+ */+ ReplicationSlotsComputeRequiredLSN();
}
return retlsn;
@@ -621,13 +627,6 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
values[0] = NameGetDatum(&MyReplicationSlot->data.name);
nulls[0] = false;
- /*- * Recompute the minimum LSN and xmin across all slots to adjust with the- * advancing potentially done.- */- ReplicationSlotsComputeRequiredXmin(false);- ReplicationSlotsComputeRequiredLSN();-
ReplicationSlotRelease();
/* Return the reached position. */
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, michael@paquier.xyz, horikyota.ntt@gmail.com, andres@anarazel.de, dim@tapoueh.org, simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
In-Reply-To: <0a14a258081aea401222498d402d129b@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