On Thu, Feb 22, 2024 at 3:44 PM Bertrand Drouvot <bertranddrouvot...@gmail.com> wrote: > > Hi, > > Thanks! > > Some random comments about v92_001 (Sorry if it has already been discussed > up-thread):
Thanks for the feedback. The patch is pushed 15 minutes back. I will prepare a top-up patch for your comments. > 1 === > > + * We do not update the 'synced' column from true to false here > > Worth to mention from which system view the 'synced' column belongs to? Sure, I will change it. > 2 === (Nit) > > +#define MIN_WORKER_NAPTIME_MS 200 > +#define MAX_WORKER_NAPTIME_MS 30000 /* 30s */ > > [MIN|MAX]_SLOTSYNC_WORKER_NAPTIME_MS instead? It is used only in slotsync.c so > more a Nit. Okay, will change it, > 3 === > > res = walrcv_exec(wrconn, query, SLOTSYNC_COLUMN_COUNT, slotRow); > - > if (res->status != WALRCV_OK_TUPLES) > > Line removal intended? I feel the current style is better, where we do not have space between the function call and return value checking. > 4 === > > + if (wal_level < WAL_LEVEL_LOGICAL) > + { > + ereport(ERROR, > + errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("slot synchronization requires > wal_level >= \"logical\"")); > + return false; > + } > > I think the return is not needed here as it won't be reached due to the > "ERROR". > Or should we use "elevel" instead of "ERROR"? It was suggested to raise ERROR for wal_level validation, please see [1]. But yes, I will remove the return value. Thanks for catching this. > 5 === > > + * operate as a superuser. This is safe because the slot sync worker > does > + * not interact with user tables, eliminating the risk of executing > + * arbitrary code within triggers. > > Right. I did not check but if we are using operators in our remote SPI calls > then it would be worth to ensure they are coming from the pg_catalog schema? > Using something like "OPERATOR(pg_catalog.=)" using "=" as an example. Can you please elaborate this one, I am not sure if I understood it. [1]: https://www.postgresql.org/message-id/CAD21AoB2ipSzQb5-o5pEYKie4oTPJTsYR1ip9_wRVrF6HbBWDQ%40mail.gmail.com thanks Shveta