On Fri, Jul 28, 2023 at 6:12 PM Tomas Vondra <tomas.von...@enterprisedb.com> wrote: > > On 7/28/23 11:42, Amit Kapila wrote: > > 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. > > > > 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. -- With Regards, Amit Kapila.
