On Mon, Jul 31, 2023 at 5:04 PM Tomas Vondra <tomas.von...@enterprisedb.com> wrote: > > On 7/31/23 11:25, Amit Kapila wrote: > > On Sat, Jul 29, 2023 at 5:53 PM Tomas Vondra > > <tomas.von...@enterprisedb.com> wrote: > >> > >> On 7/28/23 14:44, Ashutosh Bapat wrote: > >>> On Wed, Jul 26, 2023 at 8:48 PM Tomas Vondra > >>> <tomas.von...@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.
