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
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 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.c
index 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. */
