On Mon, Jan 22, 2024 at 1:11 PM Bertrand Drouvot <bertranddrouvot...@gmail.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.