Re: logical decoding and replication of sequences, take 2

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.

--
With Regards,
Amit Kapila.