On Fri, Feb 23, 2024 at 7:41 PM Bertrand Drouvot
<bertranddrouvot...@gmail.com> wrote:
>
> Hi,
> > I think to set secure search path for remote connection, the standard
> > approach
> > could be to extend the code in libpqrcv_connect[1], so that we don't need
> > to schema
> > qualify all the operators in the queries.
> >
> > And for local connection, I agree it's also needed to add a
> > SetConfigOption("search_path", "" call in the slotsync worker.
> >
> > [1]
> > libpqrcv_connect
> > ...
> > if (logical)
> > ...
> > res = libpqrcv_PQexec(conn->streamConn,
> >
> > ALWAYS_SECURE_SEARCH_PATH_SQL);
> >
>
> Agree, something like in the attached? (it's .txt to not disturb the CF bot).
Thanks for the patch, changes look good. I have corporated it in the
patch which addresses the rest of your comments in [1]. I have
attached the patch as .txt
[1]:
https://www.postgresql.org/message-id/ZdcejBDCr%2BwlVGnO%40ip-10-97-1-34.eu-west-3.compute.internal
thanks
Shveta
From f1489f783748e85749a576cf21921e37137efd2a Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.ma...@gmail.com>
Date: Thu, 22 Feb 2024 17:11:25 +0530
Subject: [PATCH v1] Top up Patch for commit 93db6cbda0
---
src/backend/access/transam/xlogrecovery.c | 15 ++++++++-------
.../libpqwalreceiver/libpqwalreceiver.c | 2 +-
src/backend/replication/logical/slotsync.c | 19 +++++++++++--------
3 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index d73a49b3e8..9d907bf0e4 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1472,13 +1472,14 @@ FinishWalRecovery(void)
* Shutdown the slot sync worker to drop any temporary slots acquired by
* it and to prevent it from keep trying to fetch the failover slots.
*
- * We do not update the 'synced' column from true to false here, as any
- * failed update could leave 'synced' column false for some slots. This
- * could cause issues during slot sync after restarting the server as a
- * standby. While updating the 'synced' column after switching to the
new
- * timeline is an option, it does not simplify the handling for the
- * 'synced' column. Therefore, we retain the 'synced' column as true
after
- * promotion as it may provide useful information about the slot origin.
+ * We do not update the 'synced' column in 'pg_replication_slots' system
+ * view from true to false here, as any failed update could leave
'synced'
+ * column false for some slots. This could cause issues during slot sync
+ * after restarting the server as a standby. While updating the 'synced'
+ * column after switching to the new timeline is an option, it does not
+ * simplify the handling for the 'synced' column. Therefore, we retain
the
+ * 'synced' column as true after promotion as it may provide useful
+ * information about the slot origin.
*/
ShutDownSlotSync();
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 04271ee703..a30528a5f6 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -271,7 +271,7 @@ libpqrcv_connect(const char *conninfo, bool replication,
bool logical,
errhint("Target server's authentication method
must be changed, or set password_required=false in the subscription
parameters.")));
}
- if (logical)
+ if (logical || !replication)
{
PGresult *res;
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index 36773cfe73..5bb9e5d8b3 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -105,10 +105,10 @@ bool sync_replication_slots = false;
* (within a MIN/MAX range) according to slot activity. See
* wait_for_slot_activity() for details.
*/
-#define MIN_WORKER_NAPTIME_MS 200
-#define MAX_WORKER_NAPTIME_MS 30000 /* 30s */
+#define MIN_SLOTSYNC_WORKER_NAPTIME_MS 200
+#define MAX_SLOTSYNC_WORKER_NAPTIME_MS 30000 /* 30s */
-static long sleep_ms = MIN_WORKER_NAPTIME_MS;
+static long sleep_ms = MIN_SLOTSYNC_WORKER_NAPTIME_MS;
/* The restart interval for slot sync work used by postmaster */
#define SLOTSYNC_RESTART_INTERVAL_SEC 10
@@ -924,12 +924,9 @@ ValidateSlotSyncParams(int elevel)
* in this case regardless of elevel provided by caller.
*/
if (wal_level < WAL_LEVEL_LOGICAL)
- {
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("slot synchronization requires wal_level
>= \"logical\""));
- return false;
- }
/*
* A physical replication slot(primary_slot_name) is required on the
@@ -1082,7 +1079,7 @@ wait_for_slot_activity(bool some_slot_updated)
* No slots were updated, so double the sleep time, but not
beyond the
* maximum allowable value.
*/
- sleep_ms = Min(sleep_ms * 2, MAX_WORKER_NAPTIME_MS);
+ sleep_ms = Min(sleep_ms * 2, MAX_SLOTSYNC_WORKER_NAPTIME_MS);
}
else
{
@@ -1090,7 +1087,7 @@ wait_for_slot_activity(bool some_slot_updated)
* Some slots were updated since the last sleep, so reset the
sleep
* time.
*/
- sleep_ms = MIN_WORKER_NAPTIME_MS;
+ sleep_ms = MIN_SLOTSYNC_WORKER_NAPTIME_MS;
}
rc = WaitLatch(MyLatch,
@@ -1215,6 +1212,12 @@ ReplSlotSyncWorkerMain(int argc, char *argv[])
*/
sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
+ /*
+ * Set always-secure search path, so malicious users can't redirect user
+ * code (e.g. operators).
+ */
+ SetConfigOption("search_path", "", PGC_SUSET, PGC_S_OVERRIDE);
+
dbname = CheckAndGetDbnameFromConninfo();
/*
--
2.34.1
