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

On 11/16/24 08:26, Masahiko Sawada wrote:
> On Fri, Nov 15, 2024 at 9:48 AM Tomas Vondra <tomas@vondra.me> wrote:
>>
>>
>>
>> On 11/15/24 18:40, Masahiko Sawada wrote:
>>> 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.
>>>
>>
>> I'm not particularly attached to how I did this in my WIP patch, it was
>> simply the simplest way to make it work for experimentation. I'd imagine
>> it'd be best to just mirror how LogicalIncreaseXminForSlot() does this.
> 
> I was going to push it next Monday but we're going to have
> out-of-cycle minor releases next week and we don't have to wait for
> February minor releases given that we already agreed with the current
> fix. So I pushed the fix.
> 

Thanks. I see you only backpatched to 13, but I believe 12 will be
rewrapped too. So maybe backpatch to 12 too?

regards

-- 
Tomas Vondra