On Thu, Feb 8, 2024 at 8:01 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Feb 8, 2024 at 12:08 PM Peter Smith <smithpb2250@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
