On Fri, Mar 29, 2024 at 6:36 AM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > > Attach a new version patch which fixed an un-initialized variable issue and > added some comments. >
The other approach to fix this issue could be that the slotsync worker get the serialized snapshot using pg_read_binary_file() corresponding to restart_lsn and writes those at standby. But there are cases when we won't have such a file like (a) when we initially create the slot and reach the consistent_point, or (b) also by the time the slotsync worker starts to read the remote snapshot file, the snapshot file could have been removed by the checkpointer on the primary (if the restart_lsn of the remote has been advanced in this window). So, in such cases, we anyway need to advance the slot. I think these could be optimizations that we could do in the future. Few comments: ============= 1. - if (slot->data.database != MyDatabaseId) + if (slot->data.database != MyDatabaseId && !fast_forward) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("replication slot \"%s\" was not created in this database", @@ -526,7 +527,7 @@ CreateDecodingContext(XLogRecPtr start_lsn, * Do not allow consumption of a "synchronized" slot until the standby * gets promoted. */ - if (RecoveryInProgress() && slot->data.synced) + if (RecoveryInProgress() && slot->data.synced && !IsSyncingReplicationSlots()) Add comments at both of the above places. 2. +extern XLogRecPtr pg_logical_replication_slot_advance(XLogRecPtr moveto, + bool *found_consistent_point); + This API looks a bit awkward as the functionality doesn't match the name. How about having a function with name LogicalSlotAdvanceAndCheckReadynessForDecoding(moveto, ready_for_decoding) with the same functionality as your patch has for pg_logical_replication_slot_advance() and then invoke it both from pg_logical_replication_slot_advance and slotsync.c. The function name is too big, we can think of a shorter name. Any ideas? -- With Regards, Amit Kapila.