Re: Synchronizing slots from primary to standby

On Mon, Jan 22, 2024 at 1:11 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>

Thanks for sharing the feedback.

>
> > Then we also discussed whether extending libpqwalreceiver's connect
> > API is a good idea and whether we need to further extend it in the
> > future. As far as I can see, slotsync worker's primary requirement is
> > to execute SQL queries which the current API is sufficient, and don't
> > see something that needs any drastic change in this API. Note that
> > tablesync worker that executes SQL also uses these APIs, so we may
> > need something in the future for either of those. Then finally we need
> > a slotsync worker to also connect to a database to use SQL and fetch
> > results.
> >
>
> On my side the nits concerns about using the libpqrcv_connect / walrcv_connect are:
>
> - cosmetic: the "rcv" do not really align with the sync slot worker
>

But note that the same API is even used for apply worker as well. One
can think that this is a connection used to receive WAL or slot_info.

minor comments on the patch:
=======================
1.
+ /* First time slot update, the function must return true */
+ Assert(local_slot_update(remote_slot));

Isn't moving this code to Assert in update_and_persist_slot() wrong?
It will make this function call no-op in non-assert builds?

2.
+ ereport(LOG,
+ errmsg("newly locally created slot \"%s\" is sync-ready now",

I think even without 'locally' in the above LOG message, it is clear.

3.
+/*
+ * walrcv_get_dbinfo_for_failover_slots_fn
+ *
+ * Run LIST_DBID_FOR_FAILOVER_SLOTS on primary server to get the
+ * list of unique DBIDs for failover logical slots
+ */
+typedef List *(*walrcv_get_dbinfo_for_failover_slots_fn)
(WalReceiverConn *conn);

This looks like a leftover from the previous version of the patch.

--
With Regards,
Amit Kapila.