Re: Synchronizing slots from primary to standby

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