Re: Physical replication slot advance is not persistent - Mailing list pgsql-hackers

Hello.

At Sun, 29 Dec 2019 15:12:16 +0300, Alexey Kondratov <a.kondratov@postgrespro.ru> wrote in 
> 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.

Sounds reasonable.

> 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.

In the first place we advance required LSN for every reply message but
save slot data only at checkpoint on physical repliation.  Such a
strict guarantee seems too much.

Or we might need to save dirty slots just before the required LSN goes
into the next segment, but it would be a separate issue.

> Logical slots were not affected again, since there was a proper
> operations order (with comments) and slot flushing routines inside
> LogicalConfirmReceivedLocation.

copy_replication_slot doen't follow that, but the function can go into
the similar situation from a bit different cause. If the required LSN
had been advanced by a move of the original slot before the function
recomputes the required LSN, there could be a case where the new slot
is missing required WAL segment. But that is a defferent issue, too.

> 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.

That causes a logical slot not being saved when only confirmed_flush
was changed. (I'm not sure about that the slot would be saved twice if
other than confirmed_flush had been changed..)

> 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!

I'm fine with that.


+        /* Compute global required LSN if restart_lsn was changed */
+        if (updated_restart)
+            ReplicationSlotsComputeRequiredLSN();
..
-            ReplicationSlotsComputeRequiredLSN();


I seems intentional, considering performance, based on the same
thought as the comment of PhysicalConfirmReceivedLocation.

I think we shouldn't touch the paths used by replication protocol. And
don't we focus on how we make a change of a replication slot from SQL
interface persistent?  It seems to me that generaly we don't need to
save dirty slots other than checkpoint, but the SQL function seems
wanting the change to be saved immediately.

As the result, please find the attached, which is following only the
first paragraph cited above.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index bb69683e2a..1aacb937c7 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -370,6 +370,11 @@ pg_physical_replication_slot_advance(XLogRecPtr moveto)
         MyReplicationSlot->data.restart_lsn = moveto;
         SpinLockRelease(&MyReplicationSlot->mutex);
         retlsn = moveto;
+
+        ReplicationSlotMarkDirty();
+
+        /* We moved retart_lsn, update the global value. */
+        ReplicationSlotsComputeRequiredLSN();
     }
 
     return retlsn;
@@ -573,14 +578,8 @@ 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();
-    }
+    /* update the on disk state if needed. */
+    ReplicationSlotSave();
 
     ReplicationSlotRelease();