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

On Thu, Nov 14, 2024 at 10:16 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Nov 14, 2024 at 7:08 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.

Indeed, I prefer your idea. I"ve attached the updated patch. I'll push
it early next week unless there are further comments.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com