Re: Auxiliary Processes and MyAuxProc - Mailing list pgsql-hackers

On Thu, Oct 3, 2019 at 1:31 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-09-30 15:13:18 -0400, Mike Palmiotto wrote:
> > Attached is the reworked and rebased patch set. I put the hook on top
> > and a separate commit for each process type. Note that avworker and
> > avlauncher were intentionally left together. Let me know if you think
> > those should be split out as well.
>
> I've not looked at this before, nor the thread. Just jumping here
> because of the reference to this patch you made in another thread...
>
> In short, I think this is far from ready, nor am I sure this is going in
> the right direction. There's a bit more detail about where I think this
> ought to go interspersed below (in particular search for PgSubProcess,
> EXEC_BACKEND).

Great, thanks for the review!

>
>
>
> > From 205ea86dde898f7edac327d46b2b43b04721aadc Mon Sep 17 00:00:00 2001
> > From: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > Date: Mon, 30 Sep 2019 14:42:53 -0400
> > Subject: [PATCH 7/8] Add backends to process centralization
>
> .oO(out of order attached patches, how fun). 7, 8, 6, 5, 4, 2, 3, 1...

Yeah, hm. Not really sure why/how that happened.

>
>
>
> > From 2a3a35f2e80cb2badcb0efbce1bad2484e364b7b Mon Sep 17 00:00:00 2001
> > From: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > Date: Fri, 27 Sep 2019 12:28:19 -0400
> > Subject: [PATCH 1/8] Add ForkProcType infrastructure
>
> A better explanation would be nice. It's hard to review non-trivial
> patches that have little explanation as to what they're
> achieving. Especially when there's some unrelated seeming changes.

I should have maintained the initial patch description from the last
cut -- there is a lot more detail there. I will pull that back in
after addressing the other concerns. :)

>
>
> > diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
> > index 9238fbe98d..9f3dad1c6d 100644
> > --- a/src/backend/bootstrap/bootstrap.c
> > +++ b/src/backend/bootstrap/bootstrap.c
> > @@ -70,6 +70,7 @@ static void cleanup(void);
> >   */
> >
> >  AuxProcType MyAuxProcType = NotAnAuxProcess; /* declared in miscadmin.h */
> > +ForkProcType MyForkProcType = NoForkProcess; /* declared in fork_process.h */
> >
> >  Relation     boot_reldesc;           /* current relation descriptor
> >  */
>
> IMO, before this stuff gets massaged meaningfully, we ought to move code
> like this (including AuxiliaryProcessMain etc) somewhere where it makes
> sense. If we're going to rejigger things, it ought to be towards a more
> understandable architecture, rather than keeping weird quirks around.

Sounds good. I'll address this in the next round.

>
> I'm inclined to think that this better also include properly
> centralizing a good portion of the EXEC_BACKEND code. Building
> infrastructure that's supposed to be long-lived with that being left as
> is just seems like recipe for yet another odd framework.

+1

>
>
> > @@ -538,11 +539,11 @@ static void ShmemBackendArrayAdd(Backend *bn);
> >  static void ShmemBackendArrayRemove(Backend *bn);
> >  #endif                                                       /* EXEC_BACKEND */
> >
> > -#define StartupDataBase()            StartChildProcess(StartupProcess)
> > -#define StartBackgroundWriter() StartChildProcess(BgWriterProcess)
> > -#define StartCheckpointer()          StartChildProcess(CheckpointerProcess)
> > -#define StartWalWriter()             StartChildProcess(WalWriterProcess)
> > -#define StartWalReceiver()           StartChildProcess(WalReceiverProcess)
> > +#define StartupDataBase()            StartChildProcess(StartupFork)
> > +#define StartBackgroundWriter() StartChildProcess(BgWriterFork)
> > +#define StartCheckpointer()          StartChildProcess(CheckpointerFork)
> > +#define StartWalWriter()             StartChildProcess(WalWriterFork)
> > +#define StartWalReceiver()           StartChildProcess(WalReceiverFork)
>
> FWIW, I'd just rip this out, it adds exactly nothing but one more place
> to change.

+1

>
>
> > +/*
> > + * PrepAuxProcessFork
> > + *
> > + * Prepare a ForkProcType struct for the auxiliary process specified by
>
> ForkProcData, not ForkProcType
>
>
> > + * AuxProcType. This does all prep related to av parameters and error messages.
> > + */
> > +static void
> > +PrepAuxProcessFork(ForkProcData *aux_fork)
> > +{
> > +     int                     ac = 0;
> > +
> > +     /*
> > +      * Set up command-line arguments for subprocess
> > +      */
> > +     aux_fork->av[ac++] = pstrdup("postgres");
>
> There's no documentation as to what 'av' is actually supposed to mean. I
> assume auxvector or such, but if so that's neither obvious, nor
> necessarily good.

Got it, better variable names here.

>
>
> > +#ifdef EXEC_BACKEND
> > +     aux_fork->av[ac++] = pstrdup("--forkboot");
> > +     aux_fork->av[ac++] = NULL;                      /* filled in by postmaster_forkexec */
> > +#endif
>
> What's the point of pstrdup()ing constant strings here? Afaict you're
> not freeing them ever?

The idea was supposed to be that the prep work lives up until the fork
happens and then is freed en masse with the rest of the MemoryContext.
Perhaps there was some oversight here. I'll revisit this and others in
the next pass.

>
>
> > +     aux_fork->av[ac++] = psprintf("-x%d", MyForkProcType);
> > +
> > +     aux_fork->av[ac] = NULL;
> > +     Assert(ac < lengthof(*aux_fork->av));
>
> Unless I miss something, the lengthof here doesn't do anythign
> useful. ForkProcData->av is defined as:
>
> > +typedef struct ForkProcData
> > +{
> > +   char                   **av;
>
> which I think means you're computing sizeof(char*)/sizeof(char)?

I think this might be a leftover assertion from the previous
infrastructure. Agreed that this is probably useless.

>
>
>
> > +     aux_fork->ac = ac;
> > +     switch (MyForkProcType)
> > +     {
> > +             case StartupProcess:
> > +                     aux_fork->type_desc = pstrdup("startup");
> > +                     break;
> > +             case BgWriterProcess:
> > +                     aux_fork->type_desc = pstrdup("background writer");
> > +                     break;
> > +             case CheckpointerProcess:
> > +                     aux_fork->type_desc = pstrdup("checkpointer");
> > +                     break;
> > +             case WalWriterProcess:
> > +                     aux_fork->type_desc = pstrdup("WAL writer");
> > +                     break;
> > +             case WalReceiverProcess:
> > +                     aux_fork->type_desc = pstrdup("WAL receiver");
> > +                     break;
> > +             default:
> > +                     aux_fork->type_desc = pstrdup("child");
> > +                     break;
> > +     }
> > +
> > +     aux_fork->child_main = AuxiliaryProcessMain;
>
> I'm really not in love in having details like the descriptors of
> processes, the types of processes, the mapping from MyForkProcType to
> AuxProcType in multiple places.  I feel this is adding even more
> confusion to this, rather than reducing it.
>
> I feel like we should define the properties of the different types of
> processes in *one* place, rather multiple. Define one array of
> structs that contains information about the different types of
> processes, rather than distributing that knowledge all over.

Agreed here. Thanks for the suggestion.

>
>
> > @@ -4477,11 +4530,11 @@ BackendRun(Port *port)
> >  pid_t
> >  postmaster_forkexec(int argc, char *argv[])
> >  {
> > -     Port            port;
> > -
> >       /* This entry point passes dummy values for the Port variables */
> > -     memset(&port, 0, sizeof(port));
> > -     return internal_forkexec(argc, argv, &port);
> > +     if (!ConnProcPort)
> > +             ConnProcPort = palloc0(sizeof(*ConnProcPort));
> > +
> > +     return internal_forkexec(argc, argv);
> >  }
>
> What's the point of creating a dummy ConnProcPort? And when is this
> being pfreed? I mean previously this at least used a stack variable that
> was automatically being released by the time postmaster_forkexec
> returned, now it sure looks like a permanent leak to me.

Will address this in the next pass.

>
> Also, why is it a good idea to rely *more* on data being implicitly
> passed by stuffing things into global variables? That strikes me as

It is certainly not. This will be addressed in the next pass, thank you!

>
>
> >               /* in parent, fork failed */
> > -             int                     save_errno = errno;
> > +             int save_errno = errno;
> >
> > -             errno = save_errno;
> > -             switch (type)
> > -             {
> > -                     case StartupProcess:
> > -                             ereport(LOG,
> > -                                             (errmsg("could not fork startup process: %m")));
> > -                             break;
> > -                     case BgWriterProcess:
> > -                             ereport(LOG,
> > -                                             (errmsg("could not fork background writer process: %m")));
> > -                             break;
> > -                     case CheckpointerProcess:
> > -                             ereport(LOG,
> > -                                             (errmsg("could not fork checkpointer process: %m")));
> > -                             break;
> > -                     case WalWriterProcess:
> > -                             ereport(LOG,
> > -                                             (errmsg("could not fork WAL writer process: %m")));
> > -                             break;
> > -                     case WalReceiverProcess:
> > -                             ereport(LOG,
> > -                                             (errmsg("could not fork WAL receiver process: %m")));
> > -                             break;
> > -                     default:
> > -                             ereport(LOG,
> > -                                             (errmsg("could not fork process: %m")));
> > -                             break;
> > -             }
> > +             errno =  save_errno;
> > +             ereport(LOG,
> > +                     (errmsg("could not fork %s process: %m", fork_data->type_desc)));
>
> Previously the process type was translatable, now it is not
> anymore. You'd probably have to solve that somehow, perhaps by
> translating the type_desc once, when setting it.

+1

>
>
> > @@ -1113,7 +1113,7 @@ process_startup_options(Port *port, bool am_superuser)
> >               av = (char **) palloc(maxac * sizeof(char *));
> >               ac = 0;
> >
> > -             av[ac++] = "postgres";
> > +             av[ac++] = pstrdup("postgres");
> >
> >               pg_split_opts(av, &ac, port->cmdline_options);
>
> Again, what's with all these conversions to dynamic memory allocations?
>
>
>
> >  /* Flags to tell if we are in an autovacuum process */
> > -static bool am_autovacuum_launcher = false;
> > -static bool am_autovacuum_worker = false;
> > +bool am_autovacuum_launcher = false;
> > +bool am_autovacuum_worker = false;
>
> I'm against exposing a multitude of am_* variables for each process
> type, especially globally.  If we're building proper infrastructure
> here, why do we need multiple variables, rather than exactly one
> indicating the process type?

Noted.

>
>
>
> > @@ -347,88 +343,53 @@ static void avl_sigusr2_handler(SIGNAL_ARGS);
> >  static void avl_sigterm_handler(SIGNAL_ARGS);
> >  static void autovac_refresh_stats(void);
> >
> > -
> > -
> >  /********************************************************************
> >   *                                     AUTOVACUUM LAUNCHER CODE
> >   ********************************************************************/
>
> Spurious whitespace changes (in a few other places too).

Noted.

>
>
> > +void
> > +PrepAutoVacProcessFork(ForkProcData *autovac_fork)
> >  {
>
> I'm really not in love with keeping "fork" in all these, given that
> we're not actually going to fork in some cases. Why do we not just call
> it subprocess and get rid of the "fork" and "forkexec" suffixes?

Sounds good to me.

>
>
>
>
> > +/*
> > + * shmemSetup
> > + *
> > + * Helper function for a child to set up shmem before
> > + * executing.
> > + *
> > + * aux_process - set to true if an auxiliary process.
> > + */
> > +static void
> > +shmemSetup(bool aux_process)
> > +{
> > +     /* Restore basic shared memory pointers */
> > +     InitShmemAccess(UsedShmemSegAddr);
> > +
> > +     /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
> > +     if (aux_process)
> > +             InitAuxiliaryProcess();
> > +     else
> > +             InitProcess();
> > +
> > +     /* Attach process to shared data structures */
> > +     CreateSharedMemoryAndSemaphores();
> > +}
>
> Not quite convinced this is a useful abstraction.  I'm inclined to think
> that this again should use something more like one array of
> per-subprocess properties.
>
>
> Imagine if we had an array like
>
> typedef struct PgSubProcess
> {
>         const char *name;
>         const char *translated_name;
>         bool            needs_shmem;
>         bool            needs_aux_proc;
>         bool            needs_full_proc;
>         SubProcessEntryPoint entrypoint;
> } PgSubProcess;
>
> PgSubProcess process_types[] = {
>     {.name = "startup process", .needs_shmem = true, .needs_aux_proc = true, .entrypoint = StartupProcessMain},
>     {.name = "background writer", .needs_shmem = true, .needs_aux_proc = true, .entrypoint = BackgroundWriterMain},
>     {.name = "autovacuum launcher", .needs_shmem = true, .needs_aux_proc = true, .entrypoint = AutoVacLauncherMain},
>     {.name = "backend", .needs_shmem = true, .needs_full_proc = true, , .entrypoint = PostgresMain},

Great idea, thanks!

>
>
> > From efc6f0531b71847c6500062b5a0fe43331a6ea7e Mon Sep 17 00:00:00 2001
> > From: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > Date: Fri, 27 Sep 2019 19:13:53 -0400
> > Subject: [PATCH 3/8] Add centralized stat collector
> >
> > ---
> >  src/backend/postmaster/pgstat.c       | 88 ++++++---------------------
> >  src/backend/postmaster/postmaster.c   |  5 +-
> >  src/include/pgstat.h                  |  4 +-
> >  src/include/postmaster/fork_process.h |  1 +
> >  4 files changed, 26 insertions(+), 72 deletions(-)
> >
> > diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
> > index 011076c3e3..73d953aedf 100644
> > --- a/src/backend/postmaster/pgstat.c
> > +++ b/src/backend/postmaster/pgstat.c
> > @@ -280,10 +280,6 @@ static instr_time total_func_time;
> >   * Local function forward declarations
> >   * ----------
> >   */
> > -#ifdef EXEC_BACKEND
> > -static pid_t pgstat_forkexec(void);
> > -#endif
> > -
> >  NON_EXEC_STATIC void PgstatCollectorMain(int argc, char *argv[]) pg_attribute_noreturn();
> >  static void pgstat_exit(SIGNAL_ARGS);
> >  static void pgstat_beshutdown_hook(int code, Datum arg);
> > @@ -689,53 +685,26 @@ pgstat_reset_all(void)
> >       pgstat_reset_remove_files(PGSTAT_STAT_PERMANENT_DIRECTORY);
> >  }
> >
> > -#ifdef EXEC_BACKEND
> >
> >  /*
> > - * pgstat_forkexec() -
> > - *
> > - * Format up the arglist for, then fork and exec, statistics collector process
> > - */
> > -static pid_t
> > -pgstat_forkexec(void)
> > -{
> > -     char       *av[10];
> > -     int                     ac = 0;
> > -
> > -     av[ac++] = "postgres";
> > -     av[ac++] = "--forkcol";
> > -     av[ac++] = NULL;                        /* filled in by postmaster_forkexec */
> > -
> > -     av[ac] = NULL;
> > -     Assert(ac < lengthof(av));
> > -
> > -     return postmaster_forkexec(ac, av);
> > -}
> > -#endif                                                       /* EXEC_BACKEND */
> > -
> > -
> > -/*
> > - * pgstat_start() -
> > + * PrepPgstatCollectorFork
> >   *
> >   *   Called from postmaster at startup or after an existing collector
> >   *   died.  Attempt to fire up a fresh statistics collector.
> >   *
> > - *   Returns PID of child process, or 0 if fail.
> > - *
> > - *   Note: if fail, we will be called again from the postmaster main loop.
> >   */
> > -int
> > -pgstat_start(void)
> > +void
> > +PrepPgstatCollectorFork(ForkProcData *pgstat_fork)
> >  {
> > +     int                             ac = 0;
> >       time_t          curtime;
> > -     pid_t           pgStatPid;
> >
> >       /*
> >        * Check that the socket is there, else pgstat_init failed and we can do
> >        * nothing useful.
> >        */
> >       if (pgStatSock == PGINVALID_SOCKET)
> > -             return 0;
> > +             return;
> >
> >       /*
> >        * Do nothing if too soon since last collector start.  This is a safety
> > @@ -746,45 +715,20 @@ pgstat_start(void)
> >       curtime = time(NULL);
> >       if ((unsigned int) (curtime - last_pgstat_start_time) <
> >               (unsigned int) PGSTAT_RESTART_INTERVAL)
> > -             return 0;
> > +             return;
> >       last_pgstat_start_time = curtime;
> >
> > -     /*
> > -      * Okay, fork off the collector.
> > -      */
> >  #ifdef EXEC_BACKEND
> > -     switch ((pgStatPid = pgstat_forkexec()))
> > -#else
> > -     switch ((pgStatPid = fork_process()))
> > -#endif
> > -     {
> > -             case -1:
> > -                     ereport(LOG,
> > -                                     (errmsg("could not fork statistics collector: %m")));
> > -                     return 0;
> > -
> > -#ifndef EXEC_BACKEND
> > -             case 0:
> > -                     /* in postmaster child ... */
> > -                     InitPostmasterChild();
> > -
> > -                     /* Close the postmaster's sockets */
> > -                     ClosePostmasterPorts(false);
> > -
> > -                     /* Drop our connection to postmaster's shared memory, as well */
> > -                     dsm_detach_all();
> > -                     PGSharedMemoryDetach();
> > -
> > -                     PgstatCollectorMain(0, NULL);
> > -                     break;
> > +     pgstat_fork->av[ac++] = pstrdup("postgres");
> > +     pgstat_fork->av[ac++] = pstrdup("--forkcol");
> > +     pgstat_fork->av[ac++] = NULL;                   /* filled in by postmaster_forkexec */
> >  #endif
> >
> > -             default:
> > -                     return (int) pgStatPid;
> > -     }
> > +     pgstat_fork->ac = ac;
> > +     Assert(pgstat_fork->ac < lengthof(*pgstat_fork->av));
> >
> > -     /* shouldn't get here */
> > -     return 0;
> > +     pgstat_fork->type_desc = pstrdup("statistics collector");
> > +     pgstat_fork->child_main = PgstatCollectorMain;
> >  }
> >
> >  void
> > @@ -4425,12 +4369,16 @@ pgstat_send_bgwriter(void)
> >   * ----------
> >   */
> >  NON_EXEC_STATIC void
> > -PgstatCollectorMain(int argc, char *argv[])
> > +PgstatCollectorMain(pg_attribute_unused() int argc, pg_attribute_unused() char *argv[])
> >  {
> >       int                     len;
> >       PgStat_Msg      msg;
> >       int                     wr;
> >
> > +     /* Drop our connection to postmaster's shared memory, as well */
> > +     dsm_detach_all();
> > +     PGSharedMemoryDetach();
> > +
> >       /*
> >        * Ignore all signals usually bound to some action in the postmaster,
> >        * except SIGHUP and SIGQUIT.  Note we don't need a SIGUSR1 handler to
> > diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
> > index 35cd1479b9..37a36387a3 100644
> > --- a/src/backend/postmaster/postmaster.c
> > +++ b/src/backend/postmaster/postmaster.c
> > @@ -549,6 +549,7 @@ static void ShmemBackendArrayRemove(Backend *bn);
> >  #define StartWalReceiver()           StartChildProcess(WalReceiverFork)
> >  #define StartAutoVacLauncher()               StartChildProcess(AutoVacLauncherFork)
> >  #define StartAutoVacWorker()         StartChildProcess(AutoVacWorkerFork)
> > +#define pgstat_start()                       StartChildProcess(PgstatCollectorFork)
> >
> >  /* Macros to check exit status of a child process */
> >  #define EXIT_STATUS_0(st)  ((st) == 0)
> > @@ -5459,7 +5460,9 @@ StartChildProcess(ForkProcType type)
> >               case AutoVacWorkerFork:
> >                       PrepAutoVacProcessFork(fork_data);
> >                       break;
> > -
> > +             case PgstatCollectorFork:
> > +                     PrepPgstatCollectorFork(fork_data);
> > +                     break;
> >               default:
> >                       break;
> >       }
> > diff --git a/src/include/pgstat.h b/src/include/pgstat.h
> > index fe076d823d..00e95caa60 100644
> > --- a/src/include/pgstat.h
> > +++ b/src/include/pgstat.h
> > @@ -15,6 +15,7 @@
> >  #include "libpq/pqcomm.h"
> >  #include "port/atomics.h"
> >  #include "portability/instr_time.h"
> > +#include "postmaster/fork_process.h"
> >  #include "postmaster/pgarch.h"
> >  #include "storage/proc.h"
> >  #include "utils/hsearch.h"
> > @@ -1235,10 +1236,11 @@ extern Size BackendStatusShmemSize(void);
> >  extern void CreateSharedBackendStatus(void);
> >
> >  extern void pgstat_init(void);
> > -extern int   pgstat_start(void);
> >  extern void pgstat_reset_all(void);
> >  extern void allow_immediate_pgstat_restart(void);
> >
> > +extern void PrepPgstatCollectorFork(ForkProcData *pgstat_fork);
> > +
> >  #ifdef EXEC_BACKEND
> >  extern void PgstatCollectorMain(int argc, char *argv[]) pg_attribute_noreturn();
> >  #endif
> > diff --git a/src/include/postmaster/fork_process.h b/src/include/postmaster/fork_process.h
> > index 1f319fc98f..16ca8be968 100644
> > --- a/src/include/postmaster/fork_process.h
> > +++ b/src/include/postmaster/fork_process.h
> > @@ -26,6 +26,7 @@ typedef enum
> >     WalReceiverFork,  /* end of Auxiliary Process Forks */
> >     AutoVacLauncherFork,
> >     AutoVacWorkerFork,
> > +   PgstatCollectorFork,
> >
> >     NUMFORKPROCTYPES                  /* Must be last! */
> >  } ForkProcType;
>
>
>
>
> > ---
> >  src/backend/postmaster/postmaster.c   | 309 +++++++++++++-------------
> >  src/include/postmaster/fork_process.h |   5 +
> >  src/include/postmaster/postmaster.h   |   1 -
> >  3 files changed, 160 insertions(+), 155 deletions(-)
> >
> > diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
> > index 8f862fcd64..b55cc4556d 100644
> > --- a/src/backend/postmaster/postmaster.c
> > +++ b/src/backend/postmaster/postmaster.c
> > @@ -144,6 +144,7 @@ static Backend *ShmemBackendArray;
> >
> >  BackgroundWorker *MyBgworkerEntry = NULL;
> >  RegisteredBgWorker *CurrentBgWorker = NULL;
> > +static Backend          *MyBackend;
> >  static int                   child_errno;
>
> This is another global variable that strikes me as a bad idea. Whereas
> MyBgworkerEntry is only set in the bgworker itself, you appear to set
> MyBackend *in postmaster*:

Okay, will address this in the next pass.

>
> > +/*
> > + * PrepBackendFork
> > + *
> > + * Prepare a ForkProcType struct for starting a Backend.
> > + * This does all prep related to av parameters and error messages.
> > +*/
> > +static void
> > +PrepBackendFork(ForkProcData *backend_fork)
> > +{
> > +     int                     ac = 0;
> > +
> > +     /*
> > +      * Create backend data structure.  Better before the fork() so we can
> > +      * handle failure cleanly.
> > +      */
> > +     MyBackend = (Backend *) malloc(sizeof(Backend));
> > +     if (!MyBackend)
> > +     {
> > +             ereport(LOG,
> > +                             (errcode(ERRCODE_OUT_OF_MEMORY),
> > +                              errmsg("out of memory")));
> > +     }
>
> That's totally not ok, we'll constantly overwrite the variable set by
> another process, and otherwise have now obsolete values in there. And
> even worse, you leave it dangling in some cases:

Got it. Will address in the next pass.

>
> > +     /*
> > +      * Compute the cancel key that will be assigned to this backend. The
> > +      * backend will have its own copy in the forked-off process' value of
> > +      * MyCancelKey, so that it can transmit the key to the frontend.
> > +      */
> > +     if (!RandomCancelKey(&MyCancelKey))
> > +     {
> > +             free(MyBackend);
> > +             ereport(LOG,
> > +                             (errcode(ERRCODE_INTERNAL_ERROR),
> > +                              errmsg("could not generate random cancel key")));
> > +     }
>
>
> Nor do I see where you're actually freeing that memory in postmaster, in
> the successful case.  Previously it was freed in postmaster in the
> success case
>
> > -#ifdef EXEC_BACKEND
> > -     pid = backend_forkexec(port);
> > -#else                                                        /* !EXEC_BACKEND */
> > -     pid = fork_process();
> > -     if (pid == 0)                           /* child */
> > -     {
> > -             free(bn);
> > -
>
> but now it appears  it's only freed in the failure case:

Good catch. Will fix.

>
> > +/*
> > + *    BackendFailFork
> > + *
> > + *    Backend cleanup in case a failure occurs forking a new Backend.
> > + */
> > +static void BackendFailFork(pg_attribute_unused() int argc, pg_attribute_unused() char *argv[])
> > +{
> > +     if (!MyBackend->dead_end)
> > +             (void) ReleasePostmasterChildSlot(MyBackend->child_slot);
> > +     free(MyBackend);
> > +
> > +     report_fork_failure_to_client(MyProcPort, child_errno);
> > +}
>
>
>
> > From 718c6598e9e02e1ce9ade99afb3c8948c67ef5ae Mon Sep 17 00:00:00 2001
> > From: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > Date: Tue, 19 Feb 2019 15:29:33 +0000
> > Subject: [PATCH 8/8] Add a hook to allow extensions to set worker metadata
> >
> > This patch implements a hook that allows extensions to modify a worker
> > process' metadata in backends.
> >
> > Additional work done by Yuli Khodorkovskiy <yuli@crunchydata.com>
> > Original discussion here:
https://www.postgresql.org/message-id/flat/CAMN686FE0OdZKp9YPO%3DhtC6LnA6aW4r-%2Bjq%3D3Q5RAoFQgW8EtA%40mail.gmail.com
> > ---
> >  src/backend/postmaster/fork_process.c | 11 +++++++++++
> >  src/include/postmaster/fork_process.h |  4 ++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/src/backend/postmaster/fork_process.c b/src/backend/postmaster/fork_process.c
> > index a9138f589e..964c04d846 100644
> > --- a/src/backend/postmaster/fork_process.c
> > +++ b/src/backend/postmaster/fork_process.c
> > @@ -21,6 +21,14 @@
> >  #include <openssl/rand.h>
> >  #endif
> >
> > +/*
> > + * This hook allows plugins to get control of the child process following
> > + * a successful fork_process(). It can be used to perform some action in
> > + * extensions to set metadata in backends (including special backends) upon
> > + * setting of the session user.
> > + */
> > +ForkProcess_post_hook_type ForkProcess_post_hook = NULL;
> > +
> >  #ifndef WIN32
> >  /*
> >   * Wrapper for fork(). Return values are the same as those for fork():
> > @@ -113,6 +121,9 @@ fork_process(void)
> >  #ifdef USE_OPENSSL
> >               RAND_cleanup();
> >  #endif
> > +
> > +             if (ForkProcess_post_hook)
> > +                     (*ForkProcess_post_hook) ();
> >       }
>
>
> >       return result;
> > diff --git a/src/include/postmaster/fork_process.h b/src/include/postmaster/fork_process.h
> > index 631a2113b5..d756fcead7 100644
> > --- a/src/include/postmaster/fork_process.h
> > +++ b/src/include/postmaster/fork_process.h
> > @@ -58,4 +58,8 @@ extern pid_t fork_process(void);
> >  extern pid_t postmaster_forkexec(ForkProcData *fork_data);
> >  #endif
> >
> > +/* Hook for plugins to get control after a successful fork_process() */
> > +typedef void (*ForkProcess_post_hook_type) ();
> > +extern PGDLLIMPORT ForkProcess_post_hook_type ForkProcess_post_hook;
> > +
> >  #endif                                                       /* FORK_PROCESS_H */
> > --
> > 2.23.0
> >
>
> Why do we want libraries to allow to hook into processes like the
> startup process etc?

There are a number of OS-level process manipulations that this could
afford you as an extension developer. For instance, you could roll
your own seccomp implementation (to limit syscalls per-process-type,
perhaps?), perform some setcap magic, or some other security-related
magic.

Thanks for your patience with this patchset. I understand it's still
very immature. Hopefully all of the concerns raised will be adjusted
in the follow-up patches.



-- 
Mike Palmiotto
https://crunchydata.com