On Wed, Feb 28, 2024 at 10:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Feb 28, 2024 at 3:26 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > Here is the patch which addresses the above comments. Also optimized
> > the test a little bit. Now we use pg_sync_replication_slots() function
> > instead of worker to test the operator-redirection using search-patch.
> > This has been done to simplify the test case and reduce the added
> > time.
> >
>
> I have slightly adjusted the comments in the attached, otherwise, LGTM.
>
> --
- 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)
{
...
======
Kind Regards,
Peter Smith.
Fujitsu Australia
