Allow workers to override datallowconn

Started by Magnus Haganderalmost 8 years ago21 messages

magnus@hagander.net

almost 8 years ago

1 attachment(s)

In working on the checksumhelper patch, we came across wanting a background
worker to be allowed to bypass datallowconn for a database. Right now we
didn't take care of that, and just said "you have to ALTER TABLE" first.

Specifically for this usecase that is OK, but not paticularly user
friendly. And I think this is a use that can also be useful for other
things.

Attached is a patch that adds new Override versions of the functions to
connect to a database from a background worker.

Another option would be to just add the parameter directly to the regular
connection function, and not create separate functions. But that would make
it an incompatible change. And since background workers are commonly used
in extensions, that would break a lot of extensions out there. I figured
it's probably not worth doing that, and thus added the new functions. What
do others think about that?

Are there any other caveats in doing that this actually makes it dangerous
to just allow bypassing it for extensions?

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Attachments:

worker_override_allowconn.ctext/x-csrc; charset=US-ASCII; name=worker_override_allowconn.cDownload

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 28ff2f0979..1430894ad2 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -498,7 +498,7 @@ BootstrapModeMain(void)
 	 */
 	InitProcess();
 
-	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL);
+	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
 
 	/* Initialize stuff for bootstrap-file processing */
 	for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 702f8d8188..a52178f7d3 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -477,7 +477,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 	InitProcess();
 #endif
 
-	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL);
+	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
 
 	SetProcessingMode(NormalProcessing);
 
@@ -1679,7 +1679,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 		 * Note: if we have selected a just-deleted database (due to using
 		 * stale stats info), we'll fail and exit here.
 		 */
-		InitPostgres(NULL, dbid, NULL, InvalidOid, dbname);
+		InitPostgres(NULL, dbid, NULL, InvalidOid, dbname, false);
 		SetProcessingMode(NormalProcessing);
 		set_ps_display(dbname, false);
 		ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f3ddf828bb..1ded418960 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -434,6 +434,9 @@ static void StartAutovacuumWorker(void);
 static void MaybeStartWalReceiver(void);
 static void InitPostmasterDeathWatchHandle(void);
 
+static void BackgroundWorkerInitializeConnectionByOidInternal(Oid dboid, Oid useroid, bool override_allow_connections);
+static void BackgroundWorkerInitializeConnectionInternal(const char *dbname, const char *username, bool override_allow_connections);
+
 /*
  * Archiver is allowed to start up at the current postmaster state?
  *
@@ -5587,6 +5590,18 @@ MaxLivePostmasterChildren(void)
 void
 BackgroundWorkerInitializeConnection(const char *dbname, const char *username)
 {
+	BackgroundWorkerInitializeConnectionInternal(dbname, username, false);
+}
+
+void
+BackgroundWorkerInitializeConnectionOverride(const char *dbname, const char *username)
+{
+	BackgroundWorkerInitializeConnectionInternal(dbname, username, true);
+}
+
+static void
+BackgroundWorkerInitializeConnectionInternal(const char *dbname, const char *username, bool override_allow_connections)
+{
 	BackgroundWorker *worker = MyBgworkerEntry;
 
 	/* XXX is this the right errcode? */
@@ -5595,7 +5610,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username)
 				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 				 errmsg("database connection requirement not indicated during registration")));
 
-	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL);
+	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, override_allow_connections);
 
 	/* it had better not gotten out of "init" mode yet */
 	if (!IsInitProcessingMode())
@@ -5610,6 +5625,18 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username)
 void
 BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid)
 {
+	BackgroundWorkerInitializeConnectionByOidInternal(dboid, useroid, false);
+}
+
+void
+BackgroundWorkerInitializeConnectionByOidOverride(Oid dboid, Oid useroid)
+{
+	BackgroundWorkerInitializeConnectionByOidInternal(dboid, useroid, true);
+}
+
+void
+BackgroundWorkerInitializeConnectionByOidInternal(Oid dboid, Oid useroid, bool override_allow_connections)
+{
 	BackgroundWorker *worker = MyBgworkerEntry;
 
 	/* XXX is this the right errcode? */
@@ -5618,7 +5645,7 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid)
 				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 				 errmsg("database connection requirement not indicated during registration")));
 
-	InitPostgres(NULL, dboid, NULL, useroid, NULL);
+	InitPostgres(NULL, dboid, NULL, useroid, NULL, override_allow_connections);
 
 	/* it had better not gotten out of "init" mode yet */
 	if (!IsInitProcessingMode())
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6dc2095b9a..6b5a32b25b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3774,7 +3774,7 @@ PostgresMain(int argc, char *argv[],
 	 * it inside InitPostgres() instead.  In particular, anything that
 	 * involves database access should be there, not here.
 	 */
-	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL);
+	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, false);
 
 	/*
 	 * If the PostmasterContext is still around, recycle the space; we don't
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 484628987f..83640e3159 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -66,7 +66,7 @@
 static HeapTuple GetDatabaseTuple(const char *dbname);
 static HeapTuple GetDatabaseTupleByOid(Oid dboid);
 static void PerformAuthentication(Port *port);
-static void CheckMyDatabase(const char *name, bool am_superuser);
+static void CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections);
 static void InitCommunication(void);
 static void ShutdownPostgres(int code, Datum arg);
 static void StatementTimeoutHandler(void);
@@ -290,7 +290,7 @@ PerformAuthentication(Port *port)
  * CheckMyDatabase -- fetch information from the pg_database entry for our DB
  */
 static void
-CheckMyDatabase(const char *name, bool am_superuser)
+CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections)
 {
 	HeapTuple	tup;
 	Form_pg_database dbform;
@@ -326,7 +326,7 @@ CheckMyDatabase(const char *name, bool am_superuser)
 		/*
 		 * Check that the database is currently allowing connections.
 		 */
-		if (!dbform->datallowconn)
+		if (!dbform->datallowconn && !override_allow_connections)
 			ereport(FATAL,
 					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 					 errmsg("database \"%s\" is not currently accepting connections",
@@ -563,7 +563,7 @@ BaseInit(void)
  */
 void
 InitPostgres(const char *in_dbname, Oid dboid, const char *username,
-			 Oid useroid, char *out_dbname)
+			 Oid useroid, char *out_dbname, bool override_allow_connections)
 {
 	bool		bootstrap = IsBootstrapProcessingMode();
 	bool		am_superuser;
@@ -1006,7 +1006,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	 * user is a superuser, so the above stuff has to happen first.)
 	 */
 	if (!bootstrap)
-		CheckMyDatabase(dbname, am_superuser);
+		CheckMyDatabase(dbname, am_superuser, override_allow_connections);
 
 	/*
 	 * Now process any command-line switches and any additional GUC variable
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index a4574cd533..d06f71aceb 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -419,7 +419,7 @@ extern AuxProcType MyAuxProcType;
 extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
 extern void InitializeMaxBackends(void);
 extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username,
-			 Oid useroid, char *out_dbname);
+			 Oid useroid, char *out_dbname, bool override_allow_connections);
 extern void BaseInit(void);
 
 /* in utils/init/miscinit.c */
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 0c04529f47..83e15c1311 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -141,9 +141,11 @@ extern PGDLLIMPORT BackgroundWorker *MyBgworkerEntry;
  * only shared catalogs can be accessed.
  */
 extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username);
+extern void BackgroundWorkerInitializeConnectionOverride(const char *dbname, const char *username);
 
 /* Just like the above, but specifying database and user by OID. */
 extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid);
+extern void BackgroundWorkerInitializeConnectionByOidOverride(Oid dboid, Oid useroid);
 
 /* Block/unblock signals in a background worker process */
 extern void BackgroundWorkerBlockSignals(void);

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Magnus Hagander (#1)

Re: Allow workers to override datallowconn

Magnus Hagander <magnus@hagander.net> writes:

Attached is a patch that adds new Override versions of the functions to
connect to a database from a background worker.

Another option would be to just add the parameter directly to the regular
connection function, and not create separate functions. But that would make
it an incompatible change. And since background workers are commonly used
in extensions, that would break a lot of extensions out there. I figured
it's probably not worth doing that, and thus added the new functions. What
do others think about that?

Meh. We change exported APIs in new major versions all the time. As
long as it's just a question of an added parameter, people can deal
with it. You could take the opportunity to future-proof a little by
making this option be the first bit in a flags parameter, so that at
least future boolean option additions don't require another API break
or a whole new set of redundant functions.

Are there any other caveats in doing that this actually makes it dangerous
to just allow bypassing it for extensions?

Don't think so; we autovacuum such DBs anyway don't we?

regards, tom lane

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Tom Lane (#2)

1 attachment(s)

Re: Allow workers to override datallowconn

On Thu, Feb 22, 2018 at 7:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Attached is a patch that adds new Override versions of the functions to
connect to a database from a background worker.

Another option would be to just add the parameter directly to the regular
connection function, and not create separate functions. But that would

make

it an incompatible change. And since background workers are commonly used
in extensions, that would break a lot of extensions out there. I figured
it's probably not worth doing that, and thus added the new functions.

What

do others think about that?

Meh. We change exported APIs in new major versions all the time. As
long as it's just a question of an added parameter, people can deal
with it. You could take the opportunity to future-proof a little by
making this option be the first bit in a flags parameter, so that at
least future boolean option additions don't require another API break
or a whole new set of redundant functions.

Fair enough. In that case, something like the attached?

Are there any other caveats in doing that this actually makes it dangerous

to just allow bypassing it for extensions?

Don't think so; we autovacuum such DBs anyway don't we?

Yes. We set the freeze ages to 0 but we do run on it.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Attachments:

worker_override_allowconn.ctext/x-csrc; charset=US-ASCII; name=worker_override_allowconn.cDownload

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index f99f9c07af..bb28e237d1 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -445,7 +445,7 @@ autoprewarm_database_main(Datum main_arg)
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 				 errmsg("could not map dynamic shared memory segment")));
-	BackgroundWorkerInitializeConnectionByOid(apw_state->database, InvalidOid);
+	BackgroundWorkerInitializeConnectionByOid(apw_state->database, InvalidOid, 0);
 	block_info = (BlockInfoRecord *) dsm_segment_address(seg);
 	pos = apw_state->prewarm_start_idx;
 
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 9d4efc0f8f..1d631b7275 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1324,7 +1324,8 @@ ParallelWorkerMain(Datum main_arg)
 
 	/* Restore database connection. */
 	BackgroundWorkerInitializeConnectionByOid(fps->database_id,
-											  fps->authenticated_user_id);
+											  fps->authenticated_user_id,
+											  0);
 
 	/*
 	 * Set the client encoding to the database encoding, since that is what
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 28ff2f0979..1430894ad2 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -498,7 +498,7 @@ BootstrapModeMain(void)
 	 */
 	InitProcess();
 
-	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL);
+	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
 
 	/* Initialize stuff for bootstrap-file processing */
 	for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 702f8d8188..a52178f7d3 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -477,7 +477,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 	InitProcess();
 #endif
 
-	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL);
+	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
 
 	SetProcessingMode(NormalProcessing);
 
@@ -1679,7 +1679,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 		 * Note: if we have selected a just-deleted database (due to using
 		 * stale stats info), we'll fail and exit here.
 		 */
-		InitPostgres(NULL, dbid, NULL, InvalidOid, dbname);
+		InitPostgres(NULL, dbid, NULL, InvalidOid, dbname, false);
 		SetProcessingMode(NormalProcessing);
 		set_ps_display(dbname, false);
 		ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f3ddf828bb..6f1b621f70 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5585,7 +5585,7 @@ MaxLivePostmasterChildren(void)
  * Connect background worker to a database.
  */
 void
-BackgroundWorkerInitializeConnection(const char *dbname, const char *username)
+BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags)
 {
 	BackgroundWorker *worker = MyBgworkerEntry;
 
@@ -5595,7 +5595,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username)
 				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 				 errmsg("database connection requirement not indicated during registration")));
 
-	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL);
+	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, (flags & BGWORKER_BYPASS_ALLOWCONN) != 0);
 
 	/* it had better not gotten out of "init" mode yet */
 	if (!IsInitProcessingMode())
@@ -5608,7 +5608,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username)
  * Connect background worker to a database using OIDs.
  */
 void
-BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid)
+BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
 {
 	BackgroundWorker *worker = MyBgworkerEntry;
 
@@ -5618,7 +5618,7 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid)
 				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 				 errmsg("database connection requirement not indicated during registration")));
 
-	InitPostgres(NULL, dboid, NULL, useroid, NULL);
+	InitPostgres(NULL, dboid, NULL, useroid, NULL, (flags & BGWORKER_BYPASS_ALLOWCONN) != 0);
 
 	/* it had better not gotten out of "init" mode yet */
 	if (!IsInitProcessingMode())
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 2da9129562..6ef333b725 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -901,7 +901,7 @@ ApplyLauncherMain(Datum main_arg)
 	 * Establish connection to nailed catalogs (we only ever access
 	 * pg_subscription).
 	 */
-	BackgroundWorkerInitializeConnection(NULL, NULL);
+	BackgroundWorkerInitializeConnection(NULL, NULL, 0);
 
 	/* Enter main loop */
 	for (;;)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 04985c9f91..448b5a4c35 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -1525,7 +1525,8 @@ ApplyWorkerMain(Datum main_arg)
 
 	/* Connect to our database. */
 	BackgroundWorkerInitializeConnectionByOid(MyLogicalRepWorker->dbid,
-											  MyLogicalRepWorker->userid);
+											  MyLogicalRepWorker->userid,
+											  0);
 
 	/* Load the subscription into persistent memory context. */
 	ApplyContext = AllocSetContextCreate(TopMemoryContext,
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6dc2095b9a..6b5a32b25b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3774,7 +3774,7 @@ PostgresMain(int argc, char *argv[],
 	 * it inside InitPostgres() instead.  In particular, anything that
 	 * involves database access should be there, not here.
 	 */
-	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL);
+	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, false);
 
 	/*
 	 * If the PostmasterContext is still around, recycle the space; we don't
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 484628987f..83640e3159 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -66,7 +66,7 @@
 static HeapTuple GetDatabaseTuple(const char *dbname);
 static HeapTuple GetDatabaseTupleByOid(Oid dboid);
 static void PerformAuthentication(Port *port);
-static void CheckMyDatabase(const char *name, bool am_superuser);
+static void CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections);
 static void InitCommunication(void);
 static void ShutdownPostgres(int code, Datum arg);
 static void StatementTimeoutHandler(void);
@@ -290,7 +290,7 @@ PerformAuthentication(Port *port)
  * CheckMyDatabase -- fetch information from the pg_database entry for our DB
  */
 static void
-CheckMyDatabase(const char *name, bool am_superuser)
+CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections)
 {
 	HeapTuple	tup;
 	Form_pg_database dbform;
@@ -326,7 +326,7 @@ CheckMyDatabase(const char *name, bool am_superuser)
 		/*
 		 * Check that the database is currently allowing connections.
 		 */
-		if (!dbform->datallowconn)
+		if (!dbform->datallowconn && !override_allow_connections)
 			ereport(FATAL,
 					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 					 errmsg("database \"%s\" is not currently accepting connections",
@@ -563,7 +563,7 @@ BaseInit(void)
  */
 void
 InitPostgres(const char *in_dbname, Oid dboid, const char *username,
-			 Oid useroid, char *out_dbname)
+			 Oid useroid, char *out_dbname, bool override_allow_connections)
 {
 	bool		bootstrap = IsBootstrapProcessingMode();
 	bool		am_superuser;
@@ -1006,7 +1006,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	 * user is a superuser, so the above stuff has to happen first.)
 	 */
 	if (!bootstrap)
-		CheckMyDatabase(dbname, am_superuser);
+		CheckMyDatabase(dbname, am_superuser, override_allow_connections);
 
 	/*
 	 * Now process any command-line switches and any additional GUC variable
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index a4574cd533..d06f71aceb 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -419,7 +419,7 @@ extern AuxProcType MyAuxProcType;
 extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
 extern void InitializeMaxBackends(void);
 extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username,
-			 Oid useroid, char *out_dbname);
+			 Oid useroid, char *out_dbname, bool override_allow_connections);
 extern void BaseInit(void);
 
 /* in utils/init/miscinit.c */
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 0c04529f47..f1d08a3866 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -140,10 +140,13 @@ extern PGDLLIMPORT BackgroundWorker *MyBgworkerEntry;
  * If dbname is NULL, connection is made to no specific database;
  * only shared catalogs can be accessed.
  */
-extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username);
+extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags);
 
 /* Just like the above, but specifying database and user by OID. */
-extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid);
+extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags);
+
+/* Flags to BackgroundWorkerInitializeConnection et al */
+#define BGWORKER_BYPASS_ALLOWCONN 1
 
 /* Block/unblock signals in a background worker process */
 extern void BackgroundWorkerBlockSignals(void);

Andres Freund

andres@anarazel.de

almost 8 years ago

In reply to: Magnus Hagander (#1)

Re: Allow workers to override datallowconn

Hi,

On 2018-02-22 19:01:35 +0100, Magnus Hagander wrote:

In working on the checksumhelper patch, we came across wanting a background
worker to be allowed to bypass datallowconn for a database. Right now we
didn't take care of that, and just said "you have to ALTER TABLE" first.

I suspect you mean ALTER DATABASE, rather than table? ;)

I wonder if we don't want this to be a slightly more generic
facility. E.g. pg_upgrade has the same need. Perhaps we whould
superuser-only connection-establishment only option that allows
connecting to a database even if datallowconn = false, and then
additionally allow to pass connection arguments to
BackgroundWorkerInitializeConnection()? It seems fairly reasonable to
want to establish different GUCs via normal GUC-y mechanisms when
establishing a bgworker connection...

Regards,

Andres

Simon Riggs

simon@2ndquadrant.com

almost 8 years ago

In reply to: Tom Lane (#2)

Re: Allow workers to override datallowconn

On 22 February 2018 at 18:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Are there any other caveats in doing that this actually makes it dangerous
to just allow bypassing it for extensions?

Don't think so; we autovacuum such DBs anyway don't we?

Yeh, there is already precedent that should mean it is easy/default
for background workers to ignore datallowcon.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Andres Freund (#4)

Re: Allow workers to override datallowconn

On Thu, Feb 22, 2018 at 8:17 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-02-22 19:01:35 +0100, Magnus Hagander wrote:

In working on the checksumhelper patch, we came across wanting a

background

worker to be allowed to bypass datallowconn for a database. Right now we
didn't take care of that, and just said "you have to ALTER TABLE" first.

I suspect you mean ALTER DATABASE, rather than table? ;)

Um. Yes :)

I wonder if we don't want this to be a slightly more generic
facility. E.g. pg_upgrade has the same need. Perhaps we whould
superuser-only connection-establishment only option that allows
connecting to a database even if datallowconn = false, and then
additionally allow to pass connection arguments to
BackgroundWorkerInitializeConnection()? It seems fairly reasonable to
want to establish different GUCs via normal GUC-y mechanisms when
establishing a bgworker connection...

In a background worker you can just set the parameter using
SetConfigOption(), no? That seems a lot easier than turning things in to a
kv pair and back...

I can see the point for having such a parameter for pg_upgrade, but I'm not
sure we'd necessarily want to overload them.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Andres Freund

andres@anarazel.de

almost 8 years ago

In reply to: Magnus Hagander (#6)

Re: Allow workers to override datallowconn

Hi,

On 2018-02-22 20:24:03 +0100, Magnus Hagander wrote:

In a background worker you can just set the parameter using
SetConfigOption(), no? That seems a lot easier than turning things in to a
kv pair and back...

Sure, but, it doesn't seem bad to offer the option to only allow this
for code running as superuser.

I can see the point for having such a parameter for pg_upgrade, but I'm not
sure we'd necessarily want to overload them.

What's the argument against?

Greetings,

Andres Freund

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Andres Freund (#7)

Re: Allow workers to override datallowconn

On Thu, Feb 22, 2018 at 8:26 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-02-22 20:24:03 +0100, Magnus Hagander wrote:

In a background worker you can just set the parameter using
SetConfigOption(), no? That seems a lot easier than turning things in to

a

kv pair and back...

Sure, but, it doesn't seem bad to offer the option to only allow this
for code running as superuser.

I can see the point for having such a parameter for pg_upgrade, but I'm

not

sure we'd necessarily want to overload them.

What's the argument against?

Complexity for the bgw usecase. It now has to construct a key/value pair
with proper escaping (well, for this one flag it would be easy, but if we
do that wouldn't we also support the other config params? Were you thinking
we'd have *just* tihs one?). Basically taking a data that you have in a
"structured format" in the btw already turning it to a string and then
parsing it back into structured data in the same string.

I think it'd be cleaner to let the bgw initializer pass those as flags. A
"user connection" parameter could still use the booelan in InitPostgres()
of course, and not invent a new things there, but the entry point API could
stay simpler.

I haven't actually looked into what it would look like, so it could be that
I'm overestimating what it'd mean of course.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Andres Freund

andres@anarazel.de

almost 8 years ago

In reply to: Magnus Hagander (#8)

Re: Allow workers to override datallowconn

On 2018-02-22 20:30:02 +0100, Magnus Hagander wrote:

Complexity for the bgw usecase. It now has to construct a key/value pair
with proper escaping (well, for this one flag it would be easy, but if we
do that wouldn't we also support the other config params? Were you thinking
we'd have *just* tihs one?). Basically taking a data that you have in a
"structured format" in the btw already turning it to a string and then
parsing it back into structured data in the same string.

The serialization problem seems out of scope, I don't see why this code
would need to deal with it. Receiving the options would be pretty easy,
we pretty much have the relevant code for PGOPTIONS handling.

The more important part I think is that we solve it via a GUC that can
be used outside of bgworkers. Whether we require setting it via
set_config() for bgworkers or have option parsing doesn't matter that
much.

I think it'd be cleaner to let the bgw initializer pass those as flags. A
"user connection" parameter could still use the booelan in InitPostgres()
of course, and not invent a new things there, but the entry point API could
stay simpler.

I'm pretty strongly against having a special case flag for bgw
initialization for this.

Greetings,

Andres Freund

#10

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Magnus Hagander (#8)

Re: Allow workers to override datallowconn

Magnus Hagander <magnus@hagander.net> writes:

On Thu, Feb 22, 2018 at 8:26 PM, Andres Freund <andres@anarazel.de> wrote:

What's the argument against?

Complexity for the bgw usecase.

They'd be completely different implementations and code paths, no?

For pg_upgrade to use such a thing it'd need to be a connection parameter
of some sort (implying, eg, infrastructure in libpq), while for a bgworker
there's no such animal as connection parameters because there's no
connection.

Certainly what pg_upgrade has to do is a bit ugly, but you'd be adding
an awful lot of code to get rid of a small amount of code. Doesn't
seem like a great tradeoff. Even if it is a good tradeoff, it seems
entirely unrelated to the bgworker's problem.

regards, tom lane

#11

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Andres Freund (#9)

Re: Allow workers to override datallowconn

Andres Freund <andres@anarazel.de> writes:

The more important part I think is that we solve it via a GUC that can
be used outside of bgworkers.

Are you proposing an "ignore_datallowconn" GUC? That's a remarkably
bad idea. We don't have infrastructure that would allow it to be set
at an appropriate scope. I can't imagine any good use-case for allowing
it to be on globally; you'd want it to be per-session (or per-bgworker).
But I don't think there's any way to change the system default setting
in time for the setting to take effect during connection startup or
bgworker startup.

Magnus' most recent patch in this thread seems like a fine answer for
bgworkers. You've moved the goalposts into the next county, and the
design you're proposing to satisfy that goal is lousy. It will end
up being a large amount of additional work with no benefit except
being able to use an arguably less ugly (but almost certainly no
shorter) datallowconn override method in pg_upgrade.

regards, tom lane

#12

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Tom Lane (#11)

1 attachment(s)

Re: Allow workers to override datallowconn

On Thu, Feb 22, 2018 at 9:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

The more important part I think is that we solve it via a GUC that can
be used outside of bgworkers.

Are you proposing an "ignore_datallowconn" GUC? That's a remarkably
bad idea. We don't have infrastructure that would allow it to be set
at an appropriate scope. I can't imagine any good use-case for allowing
it to be on globally; you'd want it to be per-session (or per-bgworker).
But I don't think there's any way to change the system default setting
in time for the setting to take effect during connection startup or
bgworker startup.

I hacked up an attempt to do this. It does seem to work in the very simple
case, but it does requiring changing the order in InitPostgres() to load
the startup packet before validating those.

PFA WIP, which also shows how to do it in the background worker.

This one also lets me do

PGOPTIONS="-c ignore_connection_restrictionon" psql template0

to bypass the restriction, which is what pg_upgrade would basically do.

Magnus' most recent patch in this thread seems like a fine answer for

bgworkers. You've moved the goalposts into the next county, and the
design you're proposing to satisfy that goal is lousy. It will end
up being a large amount of additional work with no benefit except
being able to use an arguably less ugly (but almost certainly no
shorter) datallowconn override method in pg_upgrade.

*If* the moving of the startup packet processing to one step earlier in
InitPostgres is safe, then it is actually less code this way right now.
From a quick look I think it's safe to move it, but I haven't looked in
detail.

I also haven't checked if this actually helps in the pg_upgrade case, but
if bypassing datallowconn is what's needed there then it shuld.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Attachments:

override_datallowconn2.patchtext/x-patch; charset=US-ASCII; name=override_datallowconn2.patchDownload

diff --git a/src/backend/postmaster/checksumhelper.c b/src/backend/postmaster/checksumhelper.c
index 44535f9976..997bffa416 100644
--- a/src/backend/postmaster/checksumhelper.c
+++ b/src/backend/postmaster/checksumhelper.c
@@ -586,6 +588,8 @@ ChecksumHelperWorkerMain(Datum arg)
 	ereport(DEBUG1,
 			(errmsg("Checksum worker starting for database oid %d", dboid)));
 
+	SetConfigOption("ignore_connection_restriction", "true", PGC_SU_BACKEND, PGC_S_OVERRIDE);
+
 	BackgroundWorkerInitializeConnectionByOid(dboid, InvalidOid);
 
 	/*
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 484628987f..7034697e1b 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -326,7 +326,7 @@ CheckMyDatabase(const char *name, bool am_superuser)
 		/*
 		 * Check that the database is currently allowing connections.
 		 */
-		if (!dbform->datallowconn)
+		if (!dbform->datallowconn && !IgnoreDatAllowConn)
 			ereport(FATAL,
 					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 					 errmsg("database \"%s\" is not currently accepting connections",
@@ -999,14 +999,6 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	/* set up ACL framework (so CheckMyDatabase can check permissions) */
 	initialize_acl();
 
-	/*
-	 * Re-read the pg_database row for our database, check permissions and set
-	 * up database-specific GUC settings.  We can't do this until all the
-	 * database-access infrastructure is up.  (Also, it wants to know if the
-	 * user is a superuser, so the above stuff has to happen first.)
-	 */
-	if (!bootstrap)
-		CheckMyDatabase(dbname, am_superuser);
 
 	/*
 	 * Now process any command-line switches and any additional GUC variable
@@ -1016,6 +1008,15 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	if (MyProcPort != NULL)
 		process_startup_options(MyProcPort, am_superuser);
 
+	/*
+	 * Re-read the pg_database row for our database, check permissions and set
+	 * up database-specific GUC settings.  We can't do this until all the
+	 * database-access infrastructure is up.  (Also, it wants to know if the
+	 * user is a superuser, so the above stuff has to happen first.)
+	 */
+	if (!bootstrap)
+		CheckMyDatabase(dbname, am_superuser);
+
 	/* Process pg_db_role_setting options */
 	process_settings(MyDatabaseId, GetSessionUserId());
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 039b63bb05..8ac6c7eefe 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -461,6 +461,7 @@ bool		row_security;
 bool		check_function_bodies = true;
 bool		default_with_oids = false;
 bool		session_auth_is_superuser;
+bool		IgnoreDatAllowConn;
 
 int			log_min_error_statement = ERROR;
 int			log_min_messages = WARNING;
@@ -1648,6 +1649,18 @@ static struct config_bool ConfigureNamesBool[] =
 	},
 
 	{
+		{"ignore_connection_restriction", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
+			gettext_noop("Ignores the datallowconn restriction on database."),
+			NULL,
+			GUC_NOT_IN_SAMPLE
+		},
+		&IgnoreDatAllowConn,
+		false,
+		NULL, NULL, NULL
+	},
+
+
+	{
 		{"lo_compat_privileges", PGC_SUSET, COMPAT_OPTIONS_PREVIOUS,
 			gettext_noop("Enables backward compatibility mode for privilege checks on large objects."),
 			gettext_noop("Skips privilege checks when reading or modifying large objects, "
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index a4574cd533..666508cfaf 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -424,6 +424,7 @@ extern void BaseInit(void);
 
 /* in utils/init/miscinit.c */
 extern bool IgnoreSystemIndexes;
+extern bool IgnoreDatAllowConn;
 extern PGDLLIMPORT bool process_shared_preload_libraries_in_progress;
 extern char *session_preload_libraries_string;
 extern char *shared_preload_libraries_string;

#13

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Magnus Hagander (#12)

Re: Allow workers to override datallowconn

Magnus Hagander <magnus@hagander.net> writes:

I hacked up an attempt to do this. It does seem to work in the very simple
case, but it does requiring changing the order in InitPostgres() to load
the startup packet before validating those.

I doubt that's safe. It requires, to name just one thing, an assumption
that no processing done in process_startup_options has any need to know
the database encoding, which is established by CheckMyDatabase. Thus
for instance, if any GUC settings carried in the startup packet include
non-ASCII characters, the wrong things will happen.

You could possibly make it work with more aggressive refactoring, but
I remain of the opinion that this is a fundamentally bad idea anyhow.
A GUC of this kind is just ripe for abuse, and I don't think it's solving
any problem we really need solved.

regards, tom lane

#14

Andres Freund

andres@anarazel.de

almost 8 years ago

In reply to: Tom Lane (#13)

Re: Allow workers to override datallowconn

On 2018-02-22 15:24:50 -0500, Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

I hacked up an attempt to do this. It does seem to work in the very simple
case, but it does requiring changing the order in InitPostgres() to load
the startup packet before validating those.

I doubt that's safe. It requires, to name just one thing, an assumption
that no processing done in process_startup_options has any need to know
the database encoding, which is established by CheckMyDatabase. Thus
for instance, if any GUC settings carried in the startup packet include
non-ASCII characters, the wrong things will happen.

I think those are effectively ascii only anyway. We process them with
pretty much ascii (or well 8 byte ascii compatible) only logic
afaict. C.f. pg_split_opts().

You could possibly make it work with more aggressive refactoring, but
I remain of the opinion that this is a fundamentally bad idea anyhow.
A GUC of this kind is just ripe for abuse, and I don't think it's
solving any problem we really need solved.

How's that any less safe than allowing to load libraries, disabling
system indexes, and reams of other things we allow via GUCs?

Greetings,

Andres Freund

#15

Andres Freund

andres@anarazel.de

almost 8 years ago

In reply to: Tom Lane (#13)

Re: Allow workers to override datallowconn

Hi,

On 2018-02-22 15:24:50 -0500, Tom Lane wrote:

You could possibly make it work with more aggressive refactoring, but
I remain of the opinion that this is a fundamentally bad idea anyhow.
A GUC of this kind is just ripe for abuse, and I don't think it's solving
any problem we really need solved.

Oh, and I actually find this a thing that I needed a couple times
before. Databases used as templates are occasionally useful. To fill
their contents one usually wants to connect to them occasionally, but
most of the time connections ought to not be allowed so creating a
database with them as a template actually works. It's solvable via
other means, but not conveniently.

Greetings,

Andres Freund

#16

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Tom Lane (#13)

1 attachment(s)

Re: Allow workers to override datallowconn

On Thu, Feb 22, 2018 at 9:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

I hacked up an attempt to do this. It does seem to work in the very

simple

case, but it does requiring changing the order in InitPostgres() to load
the startup packet before validating those.

I doubt that's safe. It requires, to name just one thing, an assumption
that no processing done in process_startup_options has any need to know
the database encoding, which is established by CheckMyDatabase. Thus
for instance, if any GUC settings carried in the startup packet include
non-ASCII characters, the wrong things will happen.

You could possibly make it work with more aggressive refactoring, but
I remain of the opinion that this is a fundamentally bad idea anyhow.
A GUC of this kind is just ripe for abuse, and I don't think it's solving
any problem we really need solved.

Here's another attempt at moving this one forward. Basically this adds a
new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
any other parameters are ignored (without error). For now, only the
ignore_connection_restriction is allowed at this stage. Then it runs
CheckMyDatabase(), and after that it runs through all the parameters again,
now with the GUC_S_CLIENT source as usual, which will now process all
other variables.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Attachments:

override_datallowconn3.patchtext/x-patch; charset=US-ASCII; name=override_datallowconn3.patchDownload

diff --git a/src/backend/postmaster/checksumhelper.c b/src/backend/postmaster/checksumhelper.c
index 44535f9976..997bffa416 100644
--- a/src/backend/postmaster/checksumhelper.c
+++ b/src/backend/postmaster/checksumhelper.c
@@ -586,6 +588,8 @@ ChecksumHelperWorkerMain(Datum arg)
 	ereport(DEBUG1,
 			(errmsg("Checksum worker starting for database oid %d", dboid)));
 
+	SetConfigOption("ignore_connection_restriction", "true", PGC_SU_BACKEND, PGC_S_OVERRIDE);
+
 	BackgroundWorkerInitializeConnectionByOid(dboid, InvalidOid);
 
 	/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6dc2095b9a..f08669ddb3 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3358,7 +3358,7 @@ get_stats_option_name(const char *arg)
  */
 void
 process_postgres_switches(int argc, char *argv[], GucContext ctx,
-						  const char **dbname)
+						  const char **dbname, bool early_processing)
 {
 	bool		secure = (ctx == PGC_POSTMASTER);
 	int			errs = 0;
@@ -3376,6 +3376,10 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx,
 			argc--;
 		}
 	}
+	else if (early_processing)
+	{
+		gucsource = PGC_S_CLIENT_EARLY;
+	}
 	else
 	{
 		gucsource = PGC_S_CLIENT;	/* switches came from client */
@@ -3641,7 +3645,7 @@ PostgresMain(int argc, char *argv[],
 	/*
 	 * Parse command-line options.
 	 */
-	process_postgres_switches(argc, argv, PGC_POSTMASTER, &dbname);
+	process_postgres_switches(argc, argv, PGC_POSTMASTER, &dbname, false);
 
 	/* Must have gotten a database name, or have a default (the username) */
 	if (dbname == NULL)
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 484628987f..2b00cdebdb 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -73,7 +73,7 @@ static void StatementTimeoutHandler(void);
 static void LockTimeoutHandler(void);
 static void IdleInTransactionSessionTimeoutHandler(void);
 static bool ThereIsAtLeastOneRole(void);
-static void process_startup_options(Port *port, bool am_superuser);
+static void process_startup_options(Port *port, bool am_superuser, bool early_processing);
 static void process_settings(Oid databaseid, Oid roleid);
 
 
@@ -326,7 +326,7 @@ CheckMyDatabase(const char *name, bool am_superuser)
 		/*
 		 * Check that the database is currently allowing connections.
 		 */
-		if (!dbform->datallowconn)
+		if (!dbform->datallowconn && !IgnoreDatAllowConn)
 			ereport(FATAL,
 					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 					 errmsg("database \"%s\" is not currently accepting connections",
@@ -811,7 +811,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	{
 		/* process any options passed in the startup packet */
 		if (MyProcPort != NULL)
-			process_startup_options(MyProcPort, am_superuser);
+			process_startup_options(MyProcPort, am_superuser, false);
 
 		/* Apply PostAuthDelay as soon as we've read all options */
 		if (PostAuthDelay > 0)
@@ -999,6 +999,10 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	/* set up ACL framework (so CheckMyDatabase can check permissions) */
 	initialize_acl();
 
+	/* Process "early" GUCs in startup packet */
+	if (MyProcPort != NULL)
+		process_startup_options(MyProcPort, am_superuser, true);
+
 	/*
 	 * Re-read the pg_database row for our database, check permissions and set
 	 * up database-specific GUC settings.  We can't do this until all the
@@ -1014,7 +1018,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	 * because we didn't know if client is a superuser.
 	 */
 	if (MyProcPort != NULL)
-		process_startup_options(MyProcPort, am_superuser);
+		process_startup_options(MyProcPort, am_superuser, false);
 
 	/* Process pg_db_role_setting options */
 	process_settings(MyDatabaseId, GetSessionUserId());
@@ -1051,7 +1055,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
  * settings passed in the startup packet.
  */
 static void
-process_startup_options(Port *port, bool am_superuser)
+process_startup_options(Port *port, bool am_superuser, bool early_processing)
 {
 	GucContext	gucctx;
 	ListCell   *gucopts;
@@ -1086,7 +1090,7 @@ process_startup_options(Port *port, bool am_superuser)
 
 		Assert(ac < maxac);
 
-		(void) process_postgres_switches(ac, av, gucctx, NULL);
+		(void) process_postgres_switches(ac, av, gucctx, NULL, early_processing);
 	}
 
 	/*
@@ -1105,10 +1109,11 @@ process_startup_options(Port *port, bool am_superuser)
 		value = lfirst(gucopts);
 		gucopts = lnext(gucopts);
 
-		SetConfigOption(name, value, gucctx, PGC_S_CLIENT);
+		SetConfigOption(name, value, gucctx, early_processing ? PGC_S_CLIENT_EARLY : PGC_S_CLIENT);
 	}
 }
 
+
 /*
  * Load GUC settings from pg_db_role_setting.
  *
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 039b63bb05..1653b6a63d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -461,6 +461,7 @@ bool		row_security;
 bool		check_function_bodies = true;
 bool		default_with_oids = false;
 bool		session_auth_is_superuser;
+bool		IgnoreDatAllowConn;
 
 int			log_min_error_statement = ERROR;
 int			log_min_messages = WARNING;
@@ -1648,6 +1649,19 @@ static struct config_bool ConfigureNamesBool[] =
 	},
 
 	{
+		{"ignore_connection_restriction", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
+			gettext_noop("Ignores the datallowconn restriction on database."),
+			NULL,
+			GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_ALLOW_EARLY
+
+		},
+		&IgnoreDatAllowConn,
+		false,
+		NULL, NULL, NULL
+	},
+
+
+	{
 		{"lo_compat_privileges", PGC_SUSET, COMPAT_OPTIONS_PREVIOUS,
 			gettext_noop("Enables backward compatibility mode for privilege checks on large objects."),
 			gettext_noop("Skips privilege checks when reading or modifying large objects, "
@@ -6086,6 +6100,11 @@ set_config_option(const char *name, const char *value,
 								name)));
 				return 0;
 			}
+			/* Ignore, but not reject, if we are in early processing but param not allowed */
+			if (source == PGC_S_CLIENT_EARLY && 
+				(record->flags & GUC_ALLOW_EARLY) == 0)
+				return 0;
+
 			/* FALL THRU to process the same as PGC_BACKEND */
 		case PGC_BACKEND:
 			if (context == PGC_SIGHUP)
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index a4574cd533..666508cfaf 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -424,6 +424,7 @@ extern void BaseInit(void);
 
 /* in utils/init/miscinit.c */
 extern bool IgnoreSystemIndexes;
+extern bool IgnoreDatAllowConn;
 extern PGDLLIMPORT bool process_shared_preload_libraries_in_progress;
 extern char *session_preload_libraries_string;
 extern char *shared_preload_libraries_string;
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index 63b4e4864d..3443485bab 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -75,7 +75,8 @@ extern void ProcessClientReadInterrupt(bool blocked);
 extern void ProcessClientWriteInterrupt(bool blocked);
 
 extern void process_postgres_switches(int argc, char *argv[],
-						  GucContext ctx, const char **dbname);
+						  GucContext ctx, const char **dbname,
+									  bool early_processing);
 extern void PostgresMain(int argc, char *argv[],
 			 const char *dbname,
 			 const char *username) pg_attribute_noreturn();
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2e03640c0b..b2bad5d455 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -117,7 +117,8 @@ typedef enum
 	PGC_S_OVERRIDE,				/* special case to forcibly set default */
 	PGC_S_INTERACTIVE,			/* dividing line for error reporting */
 	PGC_S_TEST,					/* test per-database or per-user setting */
-	PGC_S_SESSION				/* SET command */
+	PGC_S_SESSION,				/* SET command */
+	PGC_S_CLIENT_EARLY			/* from client before encoding etc set */
 } GucSource;
 
 /*
@@ -229,6 +230,8 @@ typedef enum
 
 #define GUC_UNIT				(GUC_UNIT_MEMORY | GUC_UNIT_TIME)
 
+#define GUC_ALLOW_EARLY       0x100000  /* allow applying GUC before setting encodings etc */
+
 
 /* GUC vars that are actually declared in guc.c, rather than elsewhere */
 extern bool log_duration;

#17

Tom Lane

tgl@sss.pgh.pa.us

almost 8 years ago

In reply to: Magnus Hagander (#16)

Re: Allow workers to override datallowconn

Magnus Hagander <magnus@hagander.net> writes:

Here's another attempt at moving this one forward. Basically this adds a
new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
any other parameters are ignored (without error). For now, only the
ignore_connection_restriction is allowed at this stage. Then it runs
CheckMyDatabase(), and after that it runs through all the parameters again,
now with the GUC_S_CLIENT source as usual, which will now process all
other variables.

Ick. This is an improvement over the other way of attacking the problem?
I do not think so.

regards, tom lane

#18

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Tom Lane (#17)

Re: Allow workers to override datallowconn

On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Here's another attempt at moving this one forward. Basically this adds a
new GucSource being GUC_S_CLIENT_EARLY. It now runs through the

parameters

once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In

this

source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be

set,

any other parameters are ignored (without error). For now, only the
ignore_connection_restriction is allowed at this stage. Then it runs
CheckMyDatabase(), and after that it runs through all the parameters

again,

now with the GUC_S_CLIENT source as usual, which will now process all
other variables.

Ick. This is an improvement over the other way of attacking the problem?
I do not think so.

Nope, I'm far from sure that it is. I just wanted to show what it'd look
like.

I personally think the second patch (the one adding a parameter to
BackendWorkerInitializeConnection) is the cleanest one. It doesn't solve
Andres' problem, but perhaps that should be the job of a different patch.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

#19

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Magnus Hagander (#18)

Re: Allow workers to override datallowconn

On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <magnus@hagander.net>
wrote:

On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Here's another attempt at moving this one forward. Basically this adds a
new GucSource being GUC_S_CLIENT_EARLY. It now runs through the

parameters

once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In

this

source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be

set,

any other parameters are ignored (without error). For now, only the
ignore_connection_restriction is allowed at this stage. Then it runs
CheckMyDatabase(), and after that it runs through all the parameters

again,

now with the GUC_S_CLIENT source as usual, which will now process all
other variables.

Ick. This is an improvement over the other way of attacking the problem?
I do not think so.

Nope, I'm far from sure that it is. I just wanted to show what it'd look
like.

I personally think the second patch (the one adding a parameter to
BackendWorkerInitializeConnection) is the cleanest one. It doesn't solve
Andres' problem, but perhaps that should be the job of a different patch.

FWIW, I just realized that thue poc patch that adds the GUC also breaks a
large part of the regression tests. As a side-effect of it breaking how
DateStyle works. That's obviously a fixable problem, but it seems not worth
spending time on if that's not the way forward anyway.

Andres, do you have any other ideas of directions to look that would make
you withdraw your objection? I'm happy to try to write up a patch that
solves it in a way that everybody can agree with. But right now it seems to
be stuck between one way that's strongly objected to by you and one way
that's strongly objected to by Tom. And I'd rather not have that end up
with not getting the problem solved at all for *any* of the usecases...

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

#20

Tomas Vondra

tomas.vondra@2ndquadrant.com

almost 8 years ago

In reply to: Magnus Hagander (#19)

Re: Allow workers to override datallowconn

On 03/02/2018 12:16 PM, Magnus Hagander wrote:

On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <magnus@hagander.net
<mailto:magnus@hagander.net>> wrote:

On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:

Magnus Hagander <magnus@hagander.net
<mailto:magnus@hagander.net>> writes:

Here's another attempt at moving this one forward. Basically this adds a
new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
any other parameters are ignored (without error). For now, only the
ignore_connection_restriction is allowed at this stage. Then it runs
CheckMyDatabase(), and after that it runs through all the parameters again,
now with the GUC_S_CLIENT source as usual, which will now process all
other variables.

Ick. This is an improvement over the other way of attacking the
problem?
I do not think so.

Nope, I'm far from sure that it is. I just wanted to show what it'd
look like.

I personally think the second patch (the one adding a parameter to
BackendWorkerInitializeConnection) is the cleanest one. It doesn't
solve Andres' problem, but perhaps that should be the job of a
different patch.

FWIW, I just realized that thue poc patch that adds the GUC also breaks
a large part of the regression tests. As a side-effect of it breaking
how DateStyle works. That's obviously a fixable problem, but it seems
not worth spending time on if that's not the way forward anyway.

Andres, do you have any other ideas of directions to look that would
make you withdraw your objection? I'm happy to try to write up a patch
that solves it in a way that everybody can agree with. But right now it
seems to be stuck between one way that's strongly objected to by you and
one way that's strongly objected to by Tom. And I'd rather not have that
end up with not getting the problem solved at all for *any* of the
usecases...

My 2c: I think we should just go with the simplest solution, that is the
patch sent on 22/2 19:54 (i.e. BackgroundWorkerInitializeConnectionByOid
with an extra parameter).

It would be nice to have something more generic that also works for the
Andres' use case, but the patches submitted to this thread are not
particularly pretty. Also, Tom suggested there may be safety issues when
the GUCs are processed earlier - I agree Andres is right the GUCs are
effectively ASCII-only so the encoding is not an issue, but perhaps
there are other issues (Tom suggested this merely as an example).

So I agree with Magnus, the extra flag seems to be perfectly fine for
bgworkers, and I'd leave the more generic solution for a future patch if
anyone wants to hack on it.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#21

Magnus Hagander

magnus@hagander.net

almost 8 years ago

In reply to: Tomas Vondra (#20)

Re: Allow workers to override datallowconn

On Thu, Mar 29, 2018 at 4:39 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

On 03/02/2018 12:16 PM, Magnus Hagander wrote:

On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <magnus@hagander.net
<mailto:magnus@hagander.net>> wrote:

On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:

Magnus Hagander <magnus@hagander.net
<mailto:magnus@hagander.net>> writes:

Here's another attempt at moving this one forward. Basically

this adds a

new GucSource being GUC_S_CLIENT_EARLY. It now runs through

the parameters

once before CheckMyDatabase, with source set to

GUC_S_CLIENT_EARLY. In this

source, *only* parameters that are flagged as GUC_ALLOW_EARLY

will be set,

any other parameters are ignored (without error). For now,

only the

ignore_connection_restriction is allowed at this stage. Then

it runs

CheckMyDatabase(), and after that it runs through all the

parameters again,

now with the GUC_S_CLIENT source as usual, which will now

process all

other variables.

Ick. This is an improvement over the other way of attacking the
problem?
I do not think so.

Nope, I'm far from sure that it is. I just wanted to show what it'd
look like.

I personally think the second patch (the one adding a parameter to
BackendWorkerInitializeConnection) is the cleanest one. It doesn't
solve Andres' problem, but perhaps that should be the job of a
different patch.

FWIW, I just realized that thue poc patch that adds the GUC also breaks
a large part of the regression tests. As a side-effect of it breaking
how DateStyle works. That's obviously a fixable problem, but it seems
not worth spending time on if that's not the way forward anyway.

Andres, do you have any other ideas of directions to look that would
make you withdraw your objection? I'm happy to try to write up a patch
that solves it in a way that everybody can agree with. But right now it
seems to be stuck between one way that's strongly objected to by you and
one way that's strongly objected to by Tom. And I'd rather not have that
end up with not getting the problem solved at all for *any* of the
usecases...

My 2c: I think we should just go with the simplest solution, that is the
patch sent on 22/2 19:54 (i.e. BackgroundWorkerInitializeConnectionByOid
with an extra parameter).

It would be nice to have something more generic that also works for the
Andres' use case, but the patches submitted to this thread are not
particularly pretty. Also, Tom suggested there may be safety issues when
the GUCs are processed earlier - I agree Andres is right the GUCs are
effectively ASCII-only so the encoding is not an issue, but perhaps
there are other issues (Tom suggested this merely as an example).

So I agree with Magnus, the extra flag seems to be perfectly fine for
bgworkers, and I'd leave the more generic solution for a future patch if
anyone wants to hack on it.

With no further feedback on this, I have pushed this version of the patch
(rebased). Thanks!

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>