On Wed, Apr 3, 2024 at 9:36 AM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Wed, Apr 3, 2024 at 9:04 AM Amit Kapila <amit.kapil...@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? >
We call those after connecting to the database and the slot also belongs to that database whereas during synchronization of slots standby. the slots could be from different databases. > 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? > Okay, changed. Additionally, I have updated the comments and commit message. I'll push this patch after some more testing. -- With Regards, Amit Kapila.
v9-0001-Ensure-that-the-sync-slots-reach-a-consistent-sta.patch
Description: Binary data
