Re: Synchronizing slots from primary to standby

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