Re: logical decoding and replication of sequences, take 2

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.

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.

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.

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?

--
With Regards,
Amit Kapila.