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

On 11/15/24 18:40, Masahiko Sawada wrote:
> 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.
>

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.

regards

--
Tomas Vondra