Re: Synchronizing slots from primary to standby

On Wed, Apr 3, 2024 at 9:04 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > I'd just rename LogicalSlotAdvanceAndCheckSnapState(XLogRecPtr
> > moveto, bool *found_consistent_snapshot) to
> > pg_logical_replication_slot_advance(XLogRecPtr moveto, bool
> > *found_consistent_snapshot) and use it. If others don't like this, I'd
> > at least turn pg_logical_replication_slot_advance(XLogRecPtr moveto) a
> > static inline function.
> >
> Yeah, we can do that but it is not a performance-sensitive routine so
> don't know if it is worth it.

Okay for what the patch has right now. No more bikeshedding from me on this.

> > > The slotsync worker needs to advance the slots from different databases in
> > > fast_forward. So, we need to skip this check in fast_forward mode. The analysis can
> > > be found in [3].
> > -    if (slot->data.database != MyDatabaseId)
> > +    /*
> > +     * We need to access the system tables during decoding to build the
> > +     * logical changes unless we are in fast_forward mode where no changes are
> > +     * generated.
> > +     */
> > +    if (slot->data.database != MyDatabaseId && !fast_forward)
> >          ereport(ERROR,
> >
> > It's not clear from the comment that we need it for a slotsync worker
> > to advance the slots from different databases. Can this be put into
> > the comment? Also, specify in the comment, why this is safe?
> >
> It is not specific to slot sync worker but specific to fast_forward
> mode. There is already a comment "We need to access the system tables
> during decoding to build the logical changes unless we are in
> fast_forward mode where no changes are generated." telling why it is
> safe. The point is we need database access to access system tables
> while generating the logical changes and in fast-forward mode, we
> don't generate logical changes so this check is not required. Do let
> me if you have a different understanding or if my understanding is
> incorrect.

Understood. Thanks. Just curious, why isn't a problem for the existing
fast_forward mode callers pg_replication_slot_advance and
LogicalReplicationSlotHasPendingWal?

I quickly looked at v8, and have a nit, rest all looks good.

+        if (DecodingContextReady(ctx) && found_consistent_snapshot)
+            *found_consistent_snapshot = true;

Can the found_consistent_snapshot be checked first to help avoid the
function call DecodingContextReady() for pg_replication_slot_advance
callers?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com