Re: logical replication: restart_lsn can go backwards (and more), seems broken since 9.4

On Thu, Nov 14, 2024 at 7:08 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Sure. I've attached the updated patch. I just added the commit message.
>

@@ -1815,6 +1818,8 @@ LogicalIncreaseRestartDecodingForSlot(XLogRecPtr
current_lsn, XLogRecPtr restart
  confirmed_flush = slot->data.confirmed_flush;
  SpinLockRelease(&slot->mutex);

+ spin_released = true;
+
  elog(DEBUG1, "failed to increase restart lsn: proposed %X/%X, after
%X/%X, current candidate %X/%X, current after %X/%X, flushed up to
%X/%X",
  LSN_FORMAT_ARGS(restart_lsn),
  LSN_FORMAT_ARGS(current_lsn),
@@ -1823,6 +1828,9 @@ LogicalIncreaseRestartDecodingForSlot(XLogRecPtr
current_lsn, XLogRecPtr restart
  LSN_FORMAT_ARGS(confirmed_flush));
  }

+ if (!spin_released)
+ SpinLockRelease(&slot->mutex);

This coding pattern looks odd to me. We can consider releasing
spinlock in the other two if/else if checks. I understand it is a
matter of individual preference, so, if you and or others prefer the
current way, that is also fine with me. Other than this, the patch
looks good to me.

--
With Regards,
Amit Kapila.