Re: logical decoding and replication of sequences, take 2

On 7/28/23 11:42, Amit Kapila wrote:
> On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 7/26/23 09:27, Amit Kapila wrote:
>>> On Wed, Jul 26, 2023 at 9:37 AM Amit Kapila <amit.kapila16@gmail.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().

> 
> 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.

I don't see a way to improve this, except maybe having a separate LSN
for non-transactional changes (for each origin).

> 4. BTW, while checking this behaviour, I noticed that the initial sync
> worker for sequence mentions the table in the LOG message: "LOG:
> logical replication table synchronization worker for subscription
> "mysub", table "s" has finished". Won't it be better here to refer to
> it as a sequence?
> 

Thanks, I'll fix that.


regards

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