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.kapila16@gmail.com> wrote:
>
> 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.

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