On Thu, Feb 8, 2024 at 8:01 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Thu, Feb 8, 2024 at 12:08 PM Peter Smith <smithpb2...@gmail.com> wrote: > > > > Here are some review comments for patch v80_2-0001. > > Thanks for the feedback Peter. Addressed the comments in v81. > Attached patch001 for early feedback. Rest of the patches need > rebasing and thus will post those later. > > It also addresses comments by Amit in [1].
Thank you for updating the patch! Here are random comments: --- + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot use replication slot \"%s\" for logical" + " decoding", NameStr(slot->data.name)), + errdetail("This slot is being synced from the primary server."\), + errhint("Specify another replication slot.")); + I think it's better to use "synchronized" instead of "synced" for consistency with other places. --- We can create a temporary failover slot on the primary, but such slot is not synchronized. Do we want to disallow creating it? --- + + /* + * Register the callback function to clean up the shared memory of slot + * synchronization. + */ + SlotSyncInitialize(); I think it would have a wider impact than expected. IIUC this callback is needed only for processes who calls synchronize_slots(). Why do we want all processes to register this callback? --- + if (!valid) + ereport(ERROR, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("bad configuration for slot synchronization"), + /* translator: second %s is a GUC variable name */ + errdetail("The primary server slot \"%s\" specified by \"%s\" i\s not valid.", + PrimarySlotName, "primary_slot_name")); + I think that the detail message is not appropriate since the primary_slot_name could actually be a valid name. I think we can rephrase it to something like "The replication slot %s specified by %s does not exist on the primary server". Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com