Обсуждение: Re: Improve error handling for invalid slots and ensure a same 'inactive_since' time for inactive slots

Hi Nisha,

Some review comments for patch v1-0001.

======
src/backend/replication/logical/slotsync.c

ReplSlotSyncWorkerMain:

1.
+ /* Use same inactive_since time for all slots */
+ now = GetCurrentTimestamp();
+
  LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);

  for (int i = 0; i < max_replication_slots; i++)
@@ -1537,10 +1540,6 @@ update_synced_slots_inactive_since(void)
  /* The slot must not be acquired by any process */
  Assert(s->active_pid == 0);

- /* Use the same inactive_since time for all the slots. */
- if (now == 0)
- now = GetCurrentTimestamp();
-

AFAICT, this code was *already* ensuring to use the same
'inactive_since' even before your patch. The only difference is now
you are getting the timestamp value up-front instead of a deferred
assignment.

So why did you change this (and the code of RestoreSlotFromDisk) to do
the up-front assignment? Instead, you could have chosen to just leave
this code as-is, and then modify the RestoreSlotFromDisk code to match
it.

FWIW, I do prefer what you have done here because it is simpler, but I
just wondered about the choice because I think some people worry about
GetCurrentTimestamp overheads and try to avoid calling that wherever
possible.

======
src/backend/replication/slot.c

2. What about other loops?

AFAICT there are still some other loops where the inactive_since
timestamps might differ.

e.g. How about this logic in slot.c:

InvalidateObsoleteReplicationSlots:

LOOP:
for (int i = 0; i < max_replication_slots; i++)
{
  ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];

  calls InvalidatePossiblyObsoleteSlot(...)
      which calls ReplicationSlotRelease(...)
          which assigns now = GetCurrentTimestamp();
              then slot->inactive_since = now;
}

~

So, should you also assign a 'now' value outside this loop and pass
that timestamp down the calls so they eventually all get assigned the
same? I don't know, but I guess at least that would require much fewer
unnecessary calls to GetCurrentTimestamp so that may be a good thing.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Jan 29, 2025 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for patch v1-0001.
>
> ======
> src/backend/replication/logical/slotsync.c
>
> ReplSlotSyncWorkerMain:
>
> 1.
> + /* Use same inactive_since time for all slots */
> + now = GetCurrentTimestamp();
> +
>   LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
>
>   for (int i = 0; i < max_replication_slots; i++)
> @@ -1537,10 +1540,6 @@ update_synced_slots_inactive_since(void)
>   /* The slot must not be acquired by any process */
>   Assert(s->active_pid == 0);
>
> - /* Use the same inactive_since time for all the slots. */
> - if (now == 0)
> - now = GetCurrentTimestamp();
> -
>
> AFAICT, this code was *already* ensuring to use the same
> 'inactive_since' even before your patch. The only difference is now
> you are getting the timestamp value up-front instead of a deferred
> assignment.
>

I find the code without a patch better as it may sometimes skip to
call GetCurrentTimestamp().

> So why did you change this (and the code of RestoreSlotFromDisk) to do
> the up-front assignment? Instead, you could have chosen to just leave
> this code as-is, and then modify the RestoreSlotFromDisk code to match
> it.
>
> FWIW, I do prefer what you have done here because it is simpler, but I
> just wondered about the choice because I think some people worry about
> GetCurrentTimestamp overheads and try to avoid calling that wherever
> possible.
>
> ======
> src/backend/replication/slot.c
>
> 2. What about other loops?
>
> AFAICT there are still some other loops where the inactive_since
> timestamps might differ.
>
> e.g. How about this logic in slot.c:
>
> InvalidateObsoleteReplicationSlots:
>
> LOOP:
> for (int i = 0; i < max_replication_slots; i++)
> {
>   ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
>
>   calls InvalidatePossiblyObsoleteSlot(...)
>       which calls ReplicationSlotRelease(...)
>           which assigns now = GetCurrentTimestamp();
>               then slot->inactive_since = now;
> }
>
> ~
>
> So, should you also assign a 'now' value outside this loop and pass
> that timestamp down the calls so they eventually all get assigned the
> same? I don't know, but I guess at least that would require much fewer
> unnecessary calls to GetCurrentTimestamp so that may be a good thing.
>

I don't see this as an optimization worth the effort of changing the
code. This gets called infrequently enough to matter. The same is true
for the code in RestoreSlotFromDisk().

So, overall, I think we should just reject the 0001 patch and focus on 0002.

--
With Regards,
Amit Kapila.

On Wed, Jan 29, 2025 at 7:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jan 29, 2025 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Some review comments for patch v1-0001.
> >
> > ======
> > src/backend/replication/logical/slotsync.c
> >
> > ReplSlotSyncWorkerMain:
> >
> > 1.
> > + /* Use same inactive_since time for all slots */
> > + now = GetCurrentTimestamp();
> > +
> >   LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> >
> >   for (int i = 0; i < max_replication_slots; i++)
> > @@ -1537,10 +1540,6 @@ update_synced_slots_inactive_since(void)
> >   /* The slot must not be acquired by any process */
> >   Assert(s->active_pid == 0);
> >
> > - /* Use the same inactive_since time for all the slots. */
> > - if (now == 0)
> > - now = GetCurrentTimestamp();
> > -
> >
> > AFAICT, this code was *already* ensuring to use the same
> > 'inactive_since' even before your patch. The only difference is now
> > you are getting the timestamp value up-front instead of a deferred
> > assignment.
> >
>
> I find the code without a patch better as it may sometimes skip to
> call GetCurrentTimestamp().
>
> > So why did you change this (and the code of RestoreSlotFromDisk) to do
> > the up-front assignment? Instead, you could have chosen to just leave
> > this code as-is, and then modify the RestoreSlotFromDisk code to match
> > it.
> >
> > FWIW, I do prefer what you have done here because it is simpler, but I
> > just wondered about the choice because I think some people worry about
> > GetCurrentTimestamp overheads and try to avoid calling that wherever
> > possible.
> >
> > ======
> > src/backend/replication/slot.c
> >
> > 2. What about other loops?
> >
> > AFAICT there are still some other loops where the inactive_since
> > timestamps might differ.
> >
> > e.g. How about this logic in slot.c:
> >
> > InvalidateObsoleteReplicationSlots:
> >
> > LOOP:
> > for (int i = 0; i < max_replication_slots; i++)
> > {
> >   ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
> >
> >   calls InvalidatePossiblyObsoleteSlot(...)
> >       which calls ReplicationSlotRelease(...)
> >           which assigns now = GetCurrentTimestamp();
> >               then slot->inactive_since = now;
> > }
> >
> > ~
> >
> > So, should you also assign a 'now' value outside this loop and pass
> > that timestamp down the calls so they eventually all get assigned the
> > same? I don't know, but I guess at least that would require much fewer
> > unnecessary calls to GetCurrentTimestamp so that may be a good thing.
> >
>
> I don't see this as an optimization worth the effort of changing the
> code. This gets called infrequently enough to matter. The same is true
> for the code in RestoreSlotFromDisk().
>

My understanding was that the purpose of this patch was not anything
to do with "optimisations" per se, but rather it was (like the
$SUBJECT says) to ensure the *same* 'active_since' timestamp value
gets assigned.

E.g the change to RestoreSlotFromDisk() was to prevent multiple slots
from all getting assigned different 'active_since' values that differ
by only 1 or 2 milliseconds because that would look strange to anyone
inspecting those 'active_since' values.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Jan 30, 2025 at 5:23 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> My understanding was that the purpose of this patch was not anything
> to do with "optimisations" per se, but rather it was (like the
> $SUBJECT says) to ensure the *same* 'active_since' timestamp value
> gets assigned.
>
> E.g the change to RestoreSlotFromDisk() was to prevent multiple slots
> from all getting assigned different 'active_since' values that differ
> by only 1 or 2 milliseconds because that would look strange to anyone
> inspecting those 'active_since' values.
>

I see your point but not sure whether it will matter in practice
unless the number of slots is large. I feel the second patch discussed
here is a clear improvement as it helps centralize the logic to give
ERRORs for invalid slots. This is useful especially when we are
thinking of adding more reasons for slot invalidation. So, we should
put our energy into getting the 0002 patch proposed here committed and
the related patch to add a new reason for slot invalidation.

--
With Regards,
Amit Kapila.

Dear Nisha,

Thanks for creating a patch!

> Removed patch v1-0001. Please find the attached version 2 of 0002,
> which is now v2-0001.

ISMT error_if_invalid is set to true when the slot is using and set to false when dropping.
One exception is the slot_sync, but it has already had mechanism to handle such a slot.

I confirmed RaiseSlotInvalidationError() is removed based on comments.
I ran regression tests on my env and passed.

In total the patch looks good to me.

----------
Best regards,
Haato Kuroda

Some review comments for patch v4-0001.

======
src/backend/replication/logical/logical.c

CreateDecodingContext:

1.
Assert(MyReplicationSlot->data.invalidated == RS_INVAL_NONE);
Assert(MyReplicationSlot->data.restart_lsn != InvalidXLogRecPtr);

~

These lines are adjacent to the patch change. It's inconsistent to
refer to 'MyReplicationSlot' here. Everywhere else in this function
deliberated using variable 'slot' to keep the code shorter. In passing
we should change this too.

======
src/backend/replication/slot.c

2.
+ *
+ * An error is raised if error_if_invalid is true and the slot is found to
+ * be invalid. It should always be set to true, except when we are temporarily
+ * acquiring the slot and doesn't intend to change it.
  */

typo /and doesn't intend to change it/and don't intend to change it/

On Fri, Jan 31, 2025 at 1:02 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 30 Jan 2025 at 16:02, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I have made minor changes in the attached. The main change is to raise
> > an ERROR before we broadcast to let everybody know we've modified this
> > slot. There is no point in broadcasting if the slot is unusable.
> >
> > >   - Updated the test files to match the new error message.
> > >
> >
> > -   /ERROR:  cannot alter invalid replication slot "vacuum_full_inactiveslot"/
> > +   /ERROR:  can no longer access replication slot "vacuum_full_inactiveslot"/
> >
> > - "can no longer get changes from replication slot
> > \"shared_row_removal_activeslot\""
> > + "can no longer access replication slot \"shared_row_removal_activeslot\""
> >
> > With the above changes, we made the ERROR message generic which was
> > required to raise it from the central place. This looks reasonable to
> > me. The other option that occurred to me is to write it as: "can no
> > longer use replication slot "vacuum_full_inactiveslot" to access WAL
> > or modify it" but I find the one used currently in the patch better as
> > this is longer and may need modification in future if we start using
> > slot for something else.
>
> One of the test was failing because MyReplicationSlot was not set to s
> before erroring out which resulted in:
> TRAP: failed Assert("s->data.persistency == RS_TEMPORARY"), File:
> "slot.c", Line: 777, PID: 280121
> postgres: primary: walsender vignesh [local]
> START_REPLICATION(ExceptionalCondition+0xbb)[0x578612cd9289]
> postgres: primary: walsender vignesh [local]
> START_REPLICATION(ReplicationSlotCleanup+0x12b)[0x578612a32348]
> postgres: primary: walsender vignesh [local]
> START_REPLICATION(WalSndErrorCleanup+0x5e)[0x578612a41995]
>
> Fixed it by setting MyReplicationSlot just before erroring out so that
> WalSndErrorCleanup function will have access to it. I also moved the
> error reporting above as there is no need to check for slot is active
> for pid in case of invalidated slots. The attached patch has the
> changes for the same.
>

3.
+ /* We made this slot active, so it's ours now. */
+ MyReplicationSlot = s;
+
+ /* Invalid slots can't be modified or used before accessing the WAL. */
+ if (error_if_invalid && s->data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("can no longer access replication slot \"%s\"",
+    NameStr(s->data.name)),
+ errdetail("This replication slot has been invalidated due to \"%s\".",
+   SlotInvalidationCauses[s->data.invalidated]));
+

This change looked dubious. I understand that it was done to avoid a
TRAP, but I am not sure if this is the correct fix. At the point of
that ereport ERROR this slot doesn't yet belong to us because we are
still determining *can* we acquire this slot or not, so I felt it
doesn't seem correct to have the MyReplicationSlot code/comment ("it's
ours now") come before the ERROR.

Furthermore, having the code/comment "We made this slot active, so
it's ours now" ahead of the other code/comment "... but it's already
active in another process..." is contradictory.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jan 31, 2025 at 6:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Anyway, please consider it. The recovery and subscription TAP test are
> working for me.
>

Your fix looks good to me. I have pushed the patch along with that. Thanks.

--
With Regards,
Amit Kapila.