Hi, On Thu, Feb 29, 2024 at 10:43:07AM +1100, Peter Smith wrote: > - if (logical) > + /* > + * Set always-secure search path for the cases where the connection is > + * used to run SQL queries, so malicious users can't get control. > + */ > + if (logical || !replication) > { > PGresult *res; > > I found this condition a bit confusing. According to the > libpqrcv_connect function comment: > > * This function can be used for both replication and regular connections. > * If it is a replication connection, it could be either logical or physical > * based on input argument 'logical'. > > IIUC that comment is saying the 'replication' flag is like the main > categorization and the 'logical' flag is like a subcategory (for when > 'replication' is true). Therefore, won't the modified check be better > to be written the other way around? This will also be consistent with > the way the Assert was written. > > SUGGESTION > if (!replication || logical) > { > ...
Thanks for the review! Yeah, that makes sense from a categorization point of view. Out of curiosity, I checked which condition returns true most of the time by: Looking at the walrcv_connect calls: logical: 6 times !replication: 2 times (only for sync slot related stuff) Looking at a check-world coverage: logical: 1006 times !replication: 16 times So according to the above, using what has been proposed initially: " if (logical || !replication) " provides the benefit to avoid the second check on !replication most of the time (at least during check-world). Of course it also all depends if the slot sync feature (the only one that makes use of !replication) is used or not. Based on the above, I did prefer the original proposal but I think we can keep what has been pushed (Peter's proposal). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com