Re: logical decoding and replication of sequences, take 2

On 7/29/23 06:54, Amit Kapila wrote:
> On Fri, Jul 28, 2023 at 6:12 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>> On 7/28/23 11:42, Amit Kapila wrote:
>>> On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra
>>> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>>>
>>>> On 7/26/23 09:27, Amit Kapila wrote:
>>>>> On Wed, Jul 26, 2023 at 9:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>>
>>>> Anyway, I was thinking about this a bit more, and it seems it's not as
>>>> difficult to use the page LSN to ensure sequences don't go backwards.
>>>>
>>>
>>> While studying the changes for this proposal and related areas, I have
>>> a few comments:
>>> 1. I think you need to advance the origin if it is changed due to
>>> copy_sequence(), otherwise, if the sync worker restarts after
>>> SUBREL_STATE_FINISHEDCOPY, then it will restart from the slot's LSN
>>> value.
>>>
>>
>> True, we want to restart at the new origin_startpos.
>>
>>> 2. Between the time of SYNCDONE and READY state, the patch can skip
>>> applying non-transactional sequence changes even if it should apply
>>> it. The reason is that during that state change
>>> should_apply_changes_for_rel() decides whether to apply change based
>>> on the value of remote_final_lsn which won't be set for
>>> non-transactional change. I think we need to send the start LSN of a
>>> non-transactional record and then use that as remote_final_lsn for
>>> such a change.
>>
>> Good catch. remote_final_lsn is set in apply_handle_begin, but that
>> won't happen for sequences. We're already sending the LSN, but
>> logicalrep_read_sequence ignores it - it should be enough to add it to
>> LogicalRepSequence and then set it in apply_handle_sequence().
>>
>
> As per my understanding, the LSN sent is EndRecPtr of record which is
> the beginning of the next record (means current_record_end + 1). For
> comparing the current record, we use the start_position of the record
> as we do when we use the remote_final_lsn via apply_handle_begin().
>
>>>
>>> 3. For non-transactional sequence change apply, we don't set
>>> replorigin_session_origin_lsn/replorigin_session_origin_timestamp as
>>> we are doing in apply_handle_commit_internal() before calling
>>> CommitTransactionCommand(). So, that can lead to the origin moving
>>> backwards after restart which will lead to requesting and applying the
>>> same changes again and for that period of time sequence can go
>>> backwards. This needs some more thought as to what is the correct
>>> behaviour/solution for this.
>>>
>>
>> I think saying "origin moves backwards" is a bit misleading. AFAICS the
>> origin position is not actually moving backwards, it's more that we
>> don't (and can't) move it forwards for each non-transactional change. So
>> yeah, we may re-apply those, and IMHO that's expected - the sequence is
>> allowed to be "ahead" on the subscriber.
>>
>
> But, if this happens then for a period of time the sequence will go
> backwards relative to what one would have observed before restart.
>

That is true, but is it really a problem? This whole sequence decoding
thing was meant to allow logical failover - make sure that after switch
to the subscriber, the sequences don't generate duplicate values. From
this POV, the sequence going backwards (back to the confirmed origin
position) is not an issue - it's still far enough (ahead of publisher).

Is that great / ideal? No, I agree with that. But it was considered
acceptable and good enough for the failover use case ...

The only idea how to improve that is we could keep the non-transactional
changes (instead of applying them immediately), and then apply them on
the nearest "commit". That'd mean it's subject to the position tracking,
and the sequence would not go backwards, I think.

So every time we decode a commit, we'd check if we decoded any sequence
changes since the last commit, and merge them (a bit like a subxact).

This would however also mean sequence changes from rolled-back xacts may
not be replictated. I think that'd be fine, but IIRC Andres suggested
it's a valid use case.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company