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
