On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra <tomas.von...@enterprisedb.com> wrote: > > On 7/26/23 09:27, Amit Kapila wrote: > > On Wed, Jul 26, 2023 at 9:37 AM Amit Kapila <amit.kapil...@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. 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.