Hi, On 2021-08-05 12:50:15 -0700, Andres Freund wrote: > On 2021-08-03 16:50:24 +0900, Kyotaro Horiguchi wrote: > > > - My first attempt at PostgresMainSingle() separated the single/multi user > > > cases a bit more than the code right now, by having a > > > PostgresMainCommon() > > > which was called by PostgresMainSingle(), PostgresMain(). *Common only > > > started with the MessageContext allocation, which did have the > > > advantage of > > > splitting out a few of the remaining conditional actions in > > > PostgresMain() > > > (PostmasterContext, welcome banner, Log_disconnections). But lead to a > > > bit > > > more duplication. I don't really have an opinion on what's better. > > > > I'm not sure how it looked like, but isn't it reasonable that quickdie > > and log_disconnections(). handle IsUnderPostmaster instead? Or for > > log_disconnections, Log_disconnections should be turned off at > > standalone-initialization? > > I was wondering about log_disconnections too. The conditional addition of the > callback is all that forces log_disconnections to be PGC_SU_BACKEND rather > than PGC_SUSET too. So I agree that moving a check for Log_disconnections and > IsUnderPostmaster into log_disconnections is a good idea.
I did that, and it didn't seem a clear improvement.. > > > - I had to move the PgStartTime computation to a bit earlier for single > > > user > > > mode. That seems to make sense to me anyway, given that postmaster does > > > so > > > fairly early too. > > > > > > Any reason that'd be a bad idea? > > > > > > Arguably it should even be a tad earlier to be symmetric. > > > > Why don't you move the code for multiuser as earlier as standalone does? > > I think it's the other way round - right now the standalone case is much later > than the multiuser case. Postmaster determines PgStartTime after creating > shared memory, just before starting checkpointer / startup process - whereas > single user mode in HEAD does it just before accepting input for the first > time. Did that in the attached. I've attached the three remaining patches, after some more polish. Unless somebody argues against I plan to commit these soon-ish. Greetings, Andres Freund
>From 33d48f51d57b088d2d0f10f9a73e26157f98e0dd Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 8 Sep 2021 10:33:29 -0700 Subject: [PATCH v2 1/3] process startup: Initialize PgStartTime earlier in single user mode. An upcoming patch splits single user mode handling out of PostgresMain(). The startup time only needs to be determined in single user mode. Currently the initialization happens late, which makes the split a bit harder. As postmaster determines the time earlier it makes sense to move the time for single user mode to a roughly similar point in time. Author: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6y...@alap3.anarazel.de --- src/backend/tcop/postgres.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 58b5960e27d..32d11e53b4d 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4052,6 +4052,11 @@ PostgresMain(int argc, char *argv[], InitializeMaxBackends(); CreateSharedMemoryAndSemaphores(); + + /* + * Remember stand-alone backend startup time + */ + PgStartTime = GetCurrentTimestamp(); } /* @@ -4161,12 +4166,6 @@ PostgresMain(int argc, char *argv[], initStringInfo(&row_description_buf); MemoryContextSwitchTo(TopMemoryContext); - /* - * Remember stand-alone backend startup time - */ - if (!IsUnderPostmaster) - PgStartTime = GetCurrentTimestamp(); - /* * POSTGRES main processing loop begins here * -- 2.32.0.rc2
>From 1348c819f64a6e582d3cc00584ca55db5bc692e2 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Mon, 2 Aug 2021 00:38:53 -0700 Subject: [PATCH v2 2/3] process startup: Do InitProcess() at the same time regardless of EXEC_BACKEND. An upcoming patch splits single user mode into its own function. This makes that easier. Split out for easier review / testing. Author: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6y...@alap3.anarazel.de --- src/backend/postmaster/postmaster.c | 9 +++++++++ src/backend/tcop/postgres.c | 17 +++++------------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index b2fe420c3cf..96e94a55f27 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -4239,6 +4239,15 @@ BackendStartup(Port *port) /* Perform additional initialization and collect startup packet */ BackendInitialize(port); + /* + * Create a per-backend PGPROC struct in shared memory. We must do + * this before we can use LWLocks. In the !EXEC_BACKEND case (here) + * this could be delayed a bit further, but EXEC_BACKEND needs to do + * stuff with LWLocks before PostgresMain(), so we do it here as well + * for symmetry. + */ + InitProcess(); + /* And run the backend */ BackendRun(port); } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 32d11e53b4d..1dd5800d6b3 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4057,20 +4057,13 @@ PostgresMain(int argc, char *argv[], * Remember stand-alone backend startup time */ PgStartTime = GetCurrentTimestamp(); - } - /* - * Create a per-backend PGPROC struct in shared memory, except in the - * EXEC_BACKEND case where this was done in SubPostmasterMain. We must do - * this before we can use LWLocks (and in the EXEC_BACKEND case we already - * had to do some stuff with LWLocks). - */ -#ifdef EXEC_BACKEND - if (!IsUnderPostmaster) + /* + * Create a per-backend PGPROC struct in shared memory. We must do + * this before we can use LWLocks. + */ InitProcess(); -#else - InitProcess(); -#endif + } /* Early initialization */ BaseInit(); -- 2.32.0.rc2
>From b863a63613c56036eecdf03f7aebe5e5fb45d8df Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 8 Sep 2021 12:19:50 -0700 Subject: [PATCH v2 3/3] process startup: Split single user code out of PostgresMain(). Author: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6y...@alap3.anarazel.de --- src/include/tcop/tcopprot.h | 5 +- src/backend/main/main.c | 5 +- src/backend/postmaster/postmaster.c | 8 +- src/backend/tcop/postgres.c | 148 +++++++++++++++------------- 4 files changed, 88 insertions(+), 78 deletions(-) diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index 968345404e5..5c77075aedd 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -75,8 +75,9 @@ extern void ProcessClientWriteInterrupt(bool blocked); extern void process_postgres_switches(int argc, char *argv[], GucContext ctx, const char **dbname); -extern void PostgresMain(int argc, char *argv[], - const char *dbname, +extern void PostgresSingleUserMain(int argc, char *argv[], + const char *username) pg_attribute_noreturn(); +extern void PostgresMain(const char *dbname, const char *username) pg_attribute_noreturn(); extern long get_stack_depth_rlimit(void); extern void ResetUsage(void); diff --git a/src/backend/main/main.c b/src/backend/main/main.c index 3a2a0d598cd..ad84a45e28c 100644 --- a/src/backend/main/main.c +++ b/src/backend/main/main.c @@ -192,9 +192,8 @@ main(int argc, char *argv[]) else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0) GucInfoMain(); else if (argc > 1 && strcmp(argv[1], "--single") == 0) - PostgresMain(argc, argv, - NULL, /* no dbname */ - strdup(get_user_name_or_exit(progname))); + PostgresSingleUserMain(argc, argv, + strdup(get_user_name_or_exit(progname))); else PostmasterMain(argc, argv); /* the functions above should not return */ diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 96e94a55f27..cc3e245562a 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -4515,19 +4515,13 @@ BackendInitialize(Port *port) static void BackendRun(Port *port) { - char *av[2]; - const int ac = 1; - - av[0] = "postgres"; - av[1] = NULL; - /* * Make sure we aren't in PostmasterContext anymore. (We can't delete it * just yet, though, because InitPostgres will need the HBA data.) */ MemoryContextSwitchTo(TopMemoryContext); - PostgresMain(ac, av, port->database_name, port->user_name); + PostgresMain(port->database_name, port->user_name); } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 1dd5800d6b3..8d003ea9ce7 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3915,40 +3915,30 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx, } -/* ---------------------------------------------------------------- - * PostgresMain - * postgres main loop -- all backends, interactive or otherwise start here +/* + * PostgresSingleUserMain + * Entry point for single user mode. argc/argv are the command line + * arguments to be used. * - * argc/argv are the command line arguments to be used. (When being forked - * by the postmaster, these are not the original argv array of the process.) - * dbname is the name of the database to connect to, or NULL if the database - * name should be extracted from the command line arguments or defaulted. - * username is the PostgreSQL user name to be used for the session. - * ---------------------------------------------------------------- + * Performs single user specific setup then calls PostgresMain() to actually + * process queries. Single user mode specific setup should go here, rather + * than PostgresMain() or InitPostgres() when reasonably possible. */ void -PostgresMain(int argc, char *argv[], - const char *dbname, - const char *username) +PostgresSingleUserMain(int argc, char *argv[], + const char *username) { - int firstchar; - StringInfoData input_message; - sigjmp_buf local_sigjmp_buf; - volatile bool send_ready_for_query = true; - bool idle_in_transaction_timeout_enabled = false; - bool idle_session_timeout_enabled = false; + const char *dbname = NULL; - /* Initialize startup process environment if necessary. */ - if (!IsUnderPostmaster) - InitStandaloneProcess(argv[0]); + Assert(!IsUnderPostmaster); - SetProcessingMode(InitProcessing); + /* Initialize startup process environment. */ + InitStandaloneProcess(argv[0]); /* * Set default values for command-line options. */ - if (!IsUnderPostmaster) - InitializeGUCOptions(); + InitializeGUCOptions(); /* * Parse command-line options. @@ -3966,12 +3956,74 @@ PostgresMain(int argc, char *argv[], progname))); } - /* Acquire configuration parameters, unless inherited from postmaster */ - if (!IsUnderPostmaster) - { - if (!SelectConfigFiles(userDoption, progname)) - proc_exit(1); - } + /* Acquire configuration parameters */ + if (!SelectConfigFiles(userDoption, progname)) + proc_exit(1); + + /* + * Validate we have been given a reasonable-looking DataDir and change + * into it. + */ + checkDataDir(); + ChangeToDataDir(); + + /* + * Create lockfile for data directory. + */ + CreateDataDirLockFile(false); + + /* read control file (error checking and contains config ) */ + LocalProcessControlFile(false); + + /* Initialize MaxBackends */ + InitializeMaxBackends(); + + CreateSharedMemoryAndSemaphores(); + + /* + * Remember stand-alone backend startup time + */ + PgStartTime = GetCurrentTimestamp(); + + /* + * Create a per-backend PGPROC struct in shared memory. We must do this + * before we can use LWLocks. + */ + InitProcess(); + + /* + * Now that sufficient infrastructure has been initialized, PostgresMain() + * can do the rest. + */ + PostgresMain(dbname, username); +} + + +/* ---------------------------------------------------------------- + * PostgresMain + * postgres main loop -- all backends, interactive or otherwise loop here + * + * dbname is the name of the database to connect to, username is the + * PostgreSQL user name to be used for the session. + * + * NB: Single user mode specific setup should go to PostgresSingleUserMain() + * if reasonably possible. + * ---------------------------------------------------------------- + */ +void +PostgresMain(const char *dbname, const char *username) +{ + int firstchar; + StringInfoData input_message; + sigjmp_buf local_sigjmp_buf; + volatile bool send_ready_for_query = true; + bool idle_in_transaction_timeout_enabled = false; + bool idle_session_timeout_enabled = false; + + AssertArg(dbname != NULL); + AssertArg(username != NULL); + + SetProcessingMode(InitProcessing); /* * Set up signal handlers. (InitPostmasterChild or InitStandaloneProcess @@ -4029,42 +4081,6 @@ PostgresMain(int argc, char *argv[], * platforms */ } - if (!IsUnderPostmaster) - { - /* - * Validate we have been given a reasonable-looking DataDir (if under - * postmaster, assume postmaster did this already). - */ - checkDataDir(); - - /* Change into DataDir (if under postmaster, was done already) */ - ChangeToDataDir(); - - /* - * Create lockfile for data directory. - */ - CreateDataDirLockFile(false); - - /* read control file (error checking and contains config ) */ - LocalProcessControlFile(false); - - /* Initialize MaxBackends (if under postmaster, was done already) */ - InitializeMaxBackends(); - - CreateSharedMemoryAndSemaphores(); - - /* - * Remember stand-alone backend startup time - */ - PgStartTime = GetCurrentTimestamp(); - - /* - * Create a per-backend PGPROC struct in shared memory. We must do - * this before we can use LWLocks. - */ - InitProcess(); - } - /* Early initialization */ BaseInit(); -- 2.32.0.rc2
