Re: logical decoding and replication of sequences, take 2

On Mon, Jul 31, 2023 at 5:04 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 7/31/23 11:25, Amit Kapila wrote:
> > On Sat, Jul 29, 2023 at 5:53 PM Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
> >>
> >> On 7/28/23 14:44, Ashutosh Bapat wrote:
> >>> On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra
> >>> <tomas.vondra@enterprisedb.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.
> >>>> The 0005 change does that, by:
> >>>>
> >>>> 1) adding pg_sequence_state, that returns both the sequence state and
> >>>>    the page LSN
> >>>>
> >>>> 2) copy_sequence returns the page LSN
> >>>>
> >>>> 3) tablesync then sets this LSN as origin_startpos (which for tables is
> >>>>    just the LSN of the replication slot)
> >>>>
> >>>> AFAICS this makes it work - we start decoding at the page LSN, so that
> >>>> we  skip the increments that could lead to the sequence going backwards.
> >>>>
> >>>
> >>> I like this design very much. It makes things simpler than complex.
> >>> Thanks for doing this.
> >>>
> >>
> >> I agree it seems simpler. It'd be good to try testing / reviewing it a
> >> bit more, so that it doesn't misbehave in some way.
> >>
> >
> > Yeah, I also think this needs a review. This is a sort of new concept
> > where we don't use the LSN of the slot (for cases where copy returned
> > a larger value of LSN) or a full_snapshot created corresponding to the
> > sync slot by Walsender. For the case of the table, we build a full
> > snapshot because we use that for copying the table but why do we need
> > to build that for copying the sequence especially when we directly
> > copy it from the sequence relation without caring for any snapshot?
> >
>
> We need the slot to decode/apply changes during catchup. The main
> subscription may get ahead, and we need to ensure the WAL is not
> discarded or something like that. This applies even if the initial sync
> step does not use the slot/snapshot directly.
>

AFAIK, none of these needs a full_snapshot (see usage of
SnapBuild->building_full_snapshot). The full_snapshot tracks both
catalog and non-catalog xacts in the snapshot where we require to
track non-catalog ones because we want to copy the table using that
snapshot. It is relatively expensive to build a full snapshot and we
don't do that unless it is required. For the current usage of this
patch, I think using CRS_NOEXPORT_SNAPSHOT would be sufficient.

-- 
With Regards,
Amit Kapila.