Hi, On Thu, Feb 22, 2024 at 04:01:34PM +0530, shveta malik wrote: > 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.
Yeah, saw that after I send the comments ;-) > I will > prepare a top-up patch for your comments. Thanks! > > 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. Yeah, thanks, ERROR makes sense here. > > 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. Suppose that in synchronize_slots() the query would be: const char *query = "SELECT slot_name, plugin, confirmed_flush_lsn," " restart_lsn, catalog_xmin, two_phase, failover," " database, conflict_reason" " FROM pg_catalog.pg_replication_slots" " WHERE failover and NOT temporary and 1 = 1"; Then my comment is to rewrite it to: const char *query = "SELECT slot_name, plugin, confirmed_flush_lsn," " restart_lsn, catalog_xmin, two_phase, failover," " database, conflict_reason" " FROM pg_catalog.pg_replication_slots" " WHERE failover and NOT temporary and 1 OPERATOR(pg_catalog.=) 1"; to ensure the operator "=" is coming from the pg_catalog schema. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com