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: Sun, 29 Dec 2019 15:12:16 +0300
Message-ID: <175c2760666a78205e053207794c0f8f@postgrespro.ru> (raw)
In-Reply-To: <682b1960-cb89-8738-dbdf-8033662fb80f@postgrespro.ru>
References: <20191225.130337.442597881772691026.horikyota.ntt@gmail.com>
<ada09c82-df58-8204-9296-283514adeb6e@postgrespro.ru>
<24b9a157-d95e-96c2-2fde-9cf73f05008c@postgrespro.ru>
<20191226.173349.2159296357637287391.horikyota.ntt@gmail.com>
<682b1960-cb89-8738-dbdf-8033662fb80f@postgrespro.ru>
On 2019-12-26 16:35, Alexey Kondratov wrote:
>
> Another concern is that ReplicationSlotIsDirty is added with the only
> one user. It also cannot be used by SaveSlotToPath due to the
> simultaneous usage of both flags dirty and just_dirtied there.
>
> In that way, I hope that we should call ReplicationSlotSave
> unconditionally in the pg_replication_slot_advance, so slot will be
> saved or not automatically based on the slot->dirty flag. In the same
> time, ReplicationSlotsComputeRequiredXmin and
> ReplicationSlotsComputeRequiredLSN should be called by anyone, who
> modifies xmin and LSN fields in the slot. Otherwise, currently we are
> getting some leaky abstractions.
>
It seems that there was even a race in the order of actions inside
pg_replication_slot_advance, it did following:
- ReplicationSlotMarkDirty();
- ReplicationSlotsComputeRequiredXmin(false);
- ReplicationSlotsComputeRequiredLSN();
- ReplicationSlotSave();
1) Mark slot as dirty, which actually does nothing immediately, but
setting dirty flag;
2) Do compute new global required LSN;
3) Flush slot state to disk.
If someone will utilise old WAL and after that crash will happen between
steps 2) and 3), then we start with old value of restart_lsn, but
without required WAL. I do not know how to properly reproduce it without
gdb and power off, so the chance is pretty low, but still it could be a
case.
Logical slots were not affected again, since there was a proper
operations order (with comments) and slot flushing routines inside
LogicalConfirmReceivedLocation.
Thus, in the attached patch I have decided to do not perform slot
flushing in the pg_replication_slot_advance at all and do it in the
pg_physical_replication_slot_advance instead, as it is done in the
LogicalConfirmReceivedLocation.
Since this bugfix have not moved forward during the week, I will put it
on the 01.2020 commitfest. Kyotaro, if you do not object I will add you
as a reviewer, as you have already gave a lot of feedback, thank you for
that!
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Attachments:
[text/x-diff] v3-0001-Make-physical-slot-advance-to-be-persistent.patch (4.0K, ../175c2760666a78205e053207794c0f8f@postgrespro.ru/2-v3-0001-Make-physical-slot-advance-to-be-persistent.patch)
download | inline diff:
From c9446bfc60ae125adea476bcc5b7d50f4bafa38b Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Sun, 29 Dec 2019 14:38:13 +0300
Subject: [PATCH v3] Make physical slot advance to be persistent
Starting from v11 pg_replication_slot_advance result was not persistent
after restart. This patch fixes that bug by proper slot flushing if it
was modified.
---
src/backend/replication/logical/logical.c | 10 ++++++--
src/backend/replication/slotfuncs.c | 31 +++++++++++++++--------
2 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.cindex 7e06615864..bdb82da39b 100644--- a/src/backend/replication/logical/logical.c+++ b/src/backend/replication/logical/logical.c@@ -1049,7 +1049,10 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
SpinLockRelease(&MyReplicationSlot->mutex);
- /* first write new xmin to disk, so we know what's up after a crash */+ /*+ * First write new xmin and/or LSN to disk, so we know what's up+ * after a crash.+ */
if (updated_xmin || updated_restart)
{
ReplicationSlotMarkDirty();
@@ -1057,6 +1060,10 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
elog(DEBUG1, "updated xmin: %u restart: %u", updated_xmin, updated_restart);
}
+ /* Compute global required LSN if restart_lsn was changed */+ if (updated_restart)+ ReplicationSlotsComputeRequiredLSN();+
/*
* Now the new xmin is safely on disk, we can let the global value
* advance. We do not take ProcArrayLock or similar since we only
@@ -1070,7 +1077,6 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
SpinLockRelease(&MyReplicationSlot->mutex);
ReplicationSlotsComputeRequiredXmin(false);
- ReplicationSlotsComputeRequiredLSN();
}
}
else
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.cindex ba08ad405f..9d0e99afb8 100644--- a/src/backend/replication/slotfuncs.c+++ b/src/backend/replication/slotfuncs.c@@ -370,6 +370,19 @@ pg_physical_replication_slot_advance(XLogRecPtr moveto)
MyReplicationSlot->data.restart_lsn = moveto;
SpinLockRelease(&MyReplicationSlot->mutex);
retlsn = moveto;
++ /*+ * Dirty the slot as we updated data that is meant to be persistent+ * between restarts, flush it and re-compute global required LSN.+ *+ * If we change the order of operations and do not flush slot before+ * required LSN computing, then there will be a race. Least recent WAL+ * segments may be already utilized, so if we crash and start again with+ * old restart_lsn there will be no WAL to proceed properly.+ */+ ReplicationSlotMarkDirty();+ ReplicationSlotSave();+ ReplicationSlotsComputeRequiredLSN();
}
return retlsn;
@@ -463,6 +476,10 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto)
if (ctx->reader->EndRecPtr != InvalidXLogRecPtr)
{
+ /*+ * It also takes care about computing and updating global+ * required xmin and LSN if needed.+ */
LogicalConfirmReceivedLocation(moveto);
/*
@@ -564,7 +581,10 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
(uint32) (moveto >> 32), (uint32) moveto,
(uint32) (minlsn >> 32), (uint32) minlsn)));
- /* Do the actual slot update, depending on the slot type */+ /*+ * Do the actual slot update, depending on the slot type. Slot will be+ * marked as dirty and flushed by pg_*_replication_slot_advance if needed.+ */
if (OidIsValid(MyReplicationSlot->data.database))
endlsn = pg_logical_replication_slot_advance(moveto);
else
@@ -573,15 +593,6 @@ 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))- {- ReplicationSlotMarkDirty();- ReplicationSlotsComputeRequiredXmin(false);- ReplicationSlotsComputeRequiredLSN();- ReplicationSlotSave();- }-
ReplicationSlotRelease();
/* Return the reached position. */
base-commit: 0ce38730ac72029f3f2c95ae80b44f5b9060cbcc
--
2.19.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: <175c2760666a78205e053207794c0f8f@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