Auxiliary Processes and MyAuxProc

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Auxiliary Processes and MyAuxProc

Mike Palmiotto
Greetings,

I've been looking through the startup code for postmaster forks and
saw a couple of mechanisms used. Most forks seem to be using
StartChildProcess with a MyAuxProc emumerator, but then some
(autovacuum launcher/worker, syslogger, bgworker, archiver) are forked
through their own start functions.

I noticed some implication in the pgsql-hackers archives [1] that
non-AuxProcs are as such because they don't need access to shared
memory. Is this an accurate explanation?

For some context, I'm trying to come up with a patch to set the
process identifier (MyAuxProc/am_autovacuumworker/launcher,
am_archiver, etc.) immediately after forking, so we can provide
process context info to a hook in early startup. I wanted to make sure
to do things as cleanly as possible.

With the current startup architecture, we have to touch multiple
entrypoints to achieve the desired effect. Is there any particular
reason we couldn't fold all of the startup processes into the
StartChildProcess code and assign MyAuxProc for processes that don't
currently have one, or is this a non-starter?

Thank you for your time.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com


[1] https://www.postgresql.org/message-id/20181127.175949.06807946.horiguchi.kyotaro%40lab.ntt.co.jp

Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Tom Lane-2
Mike Palmiotto <[hidden email]> writes:
> I've been looking through the startup code for postmaster forks and
> saw a couple of mechanisms used. Most forks seem to be using
> StartChildProcess with a MyAuxProc emumerator, but then some
> (autovacuum launcher/worker, syslogger, bgworker, archiver) are forked
> through their own start functions.

> I noticed some implication in the pgsql-hackers archives [1] that
> non-AuxProcs are as such because they don't need access to shared
> memory. Is this an accurate explanation?

That was the original idea, but it hasn't stood the test of time very
well.  In particular, the AuxProcType enum only works for child
processes that there's supposed to be exactly one of, so we haven't
used it for autovac workers or bgworkers, although those certainly
have to be connected to shmem (hm, maybe that's not true for all
types of bgworker, not sure).  The autovac launcher is kind of a
weird special case --- I think it could have been included in AuxProcType,
but it wasn't, possibly to minimize the cosmetic difference between it
and autovac workers.

> For some context, I'm trying to come up with a patch to set the
> process identifier (MyAuxProc/am_autovacuumworker/launcher,
> am_archiver, etc.) immediately after forking,

Don't we do that already?

> With the current startup architecture, we have to touch multiple
> entrypoints to achieve the desired effect. Is there any particular
> reason we couldn't fold all of the startup processes into the
> StartChildProcess code and assign MyAuxProc for processes that don't
> currently have one, or is this a non-starter?

If memory serves, StartChildProcess already was an attempt to unify
the treatment of postmaster children.  It's possible that another
round of unification would be productive, but I think you'll find
that there are random small differences in requirements that'd
make it messy.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Mon, Feb 25, 2019 at 1:29 PM Tom Lane <[hidden email]> wrote:
>
> Mike Palmiotto <[hidden email]> writes:
> <snip>
>
> > For some context, I'm trying to come up with a patch to set the
> > process identifier (MyAuxProc/am_autovacuumworker/launcher,
> > am_archiver, etc.) immediately after forking,
>
> Don't we do that already?

Kind of. The indicators are set in the Main functions after
InitPostmasterChild and some other setup. In my little bit of digging
I found InitPostmasterChild to be the best place for a centralized
"early start-up hook." Is there a better place you can think of
off-hand?

> <snip>
>
> If memory serves, StartChildProcess already was an attempt to unify
> the treatment of postmaster children.  It's possible that another
> round of unification would be productive, but I think you'll find
> that there are random small differences in requirements that'd
> make it messy.

It kind of seemed like it, but I noticed the small differences in
requirements, which made me a bit hesitant. I'll go ahead and see what
I can do and submit the patch for consideration.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Mon, Feb 25, 2019 at 1:41 PM Mike Palmiotto
<[hidden email]> wrote:

>
> <snip>
> >
> > If memory serves, StartChildProcess already was an attempt to unify
> > the treatment of postmaster children.  It's possible that another
> > round of unification would be productive, but I think you'll find
> > that there are random small differences in requirements that'd
> > make it messy.
>
> It kind of seemed like it, but I noticed the small differences in
> requirements, which made me a bit hesitant. I'll go ahead and see what
> I can do and submit the patch for consideration.
I'm considering changing StartChildProcess to take a struct with data
for forking/execing each different process. Each different backend
type would build up the struct and then pass it on to
StartChildProcess, which would handle each separately. This would
ensure that the fork type is set prior to InitPostmasterChild and
would provide us with the information necessary to do what we need in
the InitPostmasterChild_hook.

Attached is a patch to fork_process.h which shows roughly what I'm
thinking. Does this seem somewhat sane as a first step?




--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

fork-proc-type.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Yuli Khodorkovskiy
On Mon, Feb 25, 2019 at 5:25 PM Mike Palmiotto
<[hidden email]> wrote:

>
> On Mon, Feb 25, 2019 at 1:41 PM Mike Palmiotto
> <[hidden email]> wrote:
> >
> > <snip>
> > >
> > > If memory serves, StartChildProcess already was an attempt to unify
> > > the treatment of postmaster children.  It's possible that another
> > > round of unification would be productive, but I think you'll find
> > > that there are random small differences in requirements that'd
> > > make it messy.
> >
> > It kind of seemed like it, but I noticed the small differences in
> > requirements, which made me a bit hesitant. I'll go ahead and see what
> > I can do and submit the patch for consideration.
>
> I'm considering changing StartChildProcess to take a struct with data
> for forking/execing each different process. Each different backend
> type would build up the struct and then pass it on to
> StartChildProcess, which would handle each separately. This would
> ensure that the fork type is set prior to InitPostmasterChild and
> would provide us with the information necessary to do what we need in
> the InitPostmasterChild_hook.
>
> Attached is a patch to fork_process.h which shows roughly what I'm
> thinking. Does this seem somewhat sane as a first step?
>
All,

Mike and I have written two patches to solve the issues
discussed in this thread.

The first patch centralizes the startup of workers and extends
worker identification that was introduced by AuxProcType. The worker
id can then be leveraged by extensions for identification of each
process.

The second patch adds a new hook that allows extensions to modify a worker
process' metadata in backends.

These patches should make future worker implementation more
straightforward as there is now one function to call that sets up
each worker. There is also code cleanup and removal of startup
redundancies.

Please let me know your thoughts. I look forward to your feedback.

Thanks,

Yuli

Add-a-hook-to-allow-extensions-to-set-worker-metadat.patch (2K) Download Attachment
Refactor-child-process-startup.patch (95K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Alvaro Herrera-9
0002 seems way too large (and it doesn't currently apply).  Is there
something we can do to make it more manageable?

I think it would be better to put your 0001 in second place rather than
first, since your other patch doesn't use it AFAICS and it adds
functionality that has not yet received approval [or even been
discussed], while the other is necessary refactoring.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Thu, Sep 26, 2019 at 9:49 AM Alvaro Herrera <[hidden email]> wrote:
>
> 0002 seems way too large (and it doesn't currently apply).  Is there
> something we can do to make it more manageable?

Initially we were thinking of submitting one patch for the
centralization work and then separate patches per backend type. We
opted not to go that route, mainly because of the number of resulting
patches (there were somewhere around 13 total, as I remember). If it
makes sense, we can go ahead and split the patches up in that fashion
after rebasing.

>
> I think it would be better to put your 0001 in second place rather than
> first, since your other patch doesn't use it AFAICS and it adds
> functionality that has not yet received approval [or even been
> discussed], while the other is necessary refactoring.

Sounds good.

--
Mike Palmiotto
https://crunchydata.com


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Alvaro Herrera-9
On 2019-Sep-26, Mike Palmiotto wrote:

> On Thu, Sep 26, 2019 at 9:49 AM Alvaro Herrera <[hidden email]> wrote:
> >
> > 0002 seems way too large (and it doesn't currently apply).  Is there
> > something we can do to make it more manageable?
>
> Initially we were thinking of submitting one patch for the
> centralization work and then separate patches per backend type. We
> opted not to go that route, mainly because of the number of resulting
> patches (there were somewhere around 13 total, as I remember). If it
> makes sense, we can go ahead and split the patches up in that fashion
> after rebasing.

Well, I think it would be easier to manage as split patches, yeah.
I think it'd be infrastructure that needs to be carefully reviewed,
while the other ones are mostly boilerplate.  If I were the committer
for it, I would push that initial patch first immediately followed by
conversion of some process that's heavily exercised in buildfarm, wait
until lack of trouble is evident, followed by a trickle of pushes to
adapt the other processes.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Thu, Sep 26, 2019 at 10:56 AM Alvaro Herrera
<[hidden email]> wrote:

>
> On 2019-Sep-26, Mike Palmiotto wrote:
>
> > On Thu, Sep 26, 2019 at 9:49 AM Alvaro Herrera <[hidden email]> wrote:
> > >
> > > 0002 seems way too large (and it doesn't currently apply).  Is there
> > > something we can do to make it more manageable?
> >
> > Initially we were thinking of submitting one patch for the
> > centralization work and then separate patches per backend type. We
> > opted not to go that route, mainly because of the number of resulting
> > patches (there were somewhere around 13 total, as I remember). If it
> > makes sense, we can go ahead and split the patches up in that fashion
> > after rebasing.
>
> Well, I think it would be easier to manage as split patches, yeah.
> I think it'd be infrastructure that needs to be carefully reviewed,
> while the other ones are mostly boilerplate.  If I were the committer
> for it, I would push that initial patch first immediately followed by
> conversion of some process that's heavily exercised in buildfarm, wait
> until lack of trouble is evident, followed by a trickle of pushes to
> adapt the other processes.

Thanks for the feedback! I've rebased and tested on my F30 box with
and without EXEC_BACKEND. Just working on splitting out the patches
now and will post the new patchset as soon as that's done (hopefully
sometime tomorrow).

--
Mike Palmiotto
https://crunchydata.com


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Thu, Sep 26, 2019 at 6:03 PM Mike Palmiotto
<[hidden email]> wrote:

>
> On Thu, Sep 26, 2019 at 10:56 AM Alvaro Herrera
> <[hidden email]> wrote:
> <snip>
> >
> > Well, I think it would be easier to manage as split patches, yeah.
> > I think it'd be infrastructure that needs to be carefully reviewed,
> > while the other ones are mostly boilerplate.  If I were the committer
> > for it, I would push that initial patch first immediately followed by
> > conversion of some process that's heavily exercised in buildfarm, wait
> > until lack of trouble is evident, followed by a trickle of pushes to
> > adapt the other processes.
>
> Thanks for the feedback! I've rebased and tested on my F30 box with
> and without EXEC_BACKEND. Just working on splitting out the patches
> now and will post the new patchset as soon as that's done (hopefully
> sometime tomorrow).
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.

Tested again with and without EXEC_BACKEND. Note that you'll need to
set randomize_va_space to 0 to disable ASLR in order to avoid
occasional failure in the EXEC_BACKEND case.

Please let me know if anything else jumps out to you.

Thanks,
--
Mike Palmiotto
https://crunchydata.com

0007-Add-backends-to-process-centralization.patch (17K) Download Attachment
0008-Add-a-hook-to-allow-extensions-to-set-worker-metadat.patch (2K) Download Attachment
0006-Add-bgworker-to-process-centralization.patch (34K) Download Attachment
0005-Add-syslogger-to-process-centralization.patch (11K) Download Attachment
0004-Add-archiver-to-process-centralization.patch (8K) Download Attachment
0002-Add-centralized-autovac-launcher-worker.patch (15K) Download Attachment
0003-Add-centralized-stat-collector.patch (8K) Download Attachment
0001-Add-ForkProcType-infrastructure.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Andres Freund
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).



> From 205ea86dde898f7edac327d46b2b43b04721aadc Mon Sep 17 00:00:00 2001
> From: Mike Palmiotto <[hidden email]>
> 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...



> From 2a3a35f2e80cb2badcb0efbce1bad2484e364b7b Mon Sep 17 00:00:00 2001
> From: Mike Palmiotto <[hidden email]>
> 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.


> 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.

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.


> @@ -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.


> +/*
> + * 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.


> +#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?


> + 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)?



> + 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.


> @@ -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.

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


>   /* 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.


> @@ -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?



> @@ -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).


> +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?




> +/*
> + * 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},


> From efc6f0531b71847c6500062b5a0fe43331a6ea7e Mon Sep 17 00:00:00 2001
> From: Mike Palmiotto <[hidden email]>
> 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*:

> +/*
> + * 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:

> + /*
> + * 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:

> +/*
> + * 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 <[hidden email]>
> 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 <[hidden email]>
> 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?


Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Thu, Oct 3, 2019 at 1:31 PM Andres Freund <[hidden email]> 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 <[hidden email]>
> > 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 <[hidden email]>
> > 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 <[hidden email]>
> > 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 <[hidden email]>
> > 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 <[hidden email]>
> > 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


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Andres Freund
Hi,

On 2019-10-03 14:33:26 -0400, Mike Palmiotto wrote:

> > > +#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.

Well, two things:
1) That'd *NEVER* be more efficient that just referencing constant
   strings. Unless you pfree() the variable, and there sometimes can be
   constant, and sometimes non-constant strings, there is simply no
   reason to ever pstrdup a constant string.
2) Which context are you talking about? Are you thinking of? A forked
   process doing MemoryContextDelete(PostmasterContext); doesn't free
   that memory in postmaster.


> > > @@ -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.

Color me unconvinced.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Michael Paquier-2
On Thu, Oct 03, 2019 at 11:39:37AM -0700, Andres Freund wrote:
> Color me unconvinced.

The latest comments of the thread have not been addressed yet. so I am
marking the patch as returned with feedback.  If you think that's not
correct, please feel free to update the status of the patch.  If you
do so, please provide at the same time a rebased version of the patch,
as it is failing to apply on HEAD.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Sat, Nov 30, 2019 at 9:15 PM Michael Paquier <[hidden email]> wrote:
>
> On Thu, Oct 03, 2019 at 11:39:37AM -0700, Andres Freund wrote:
> > Color me unconvinced.
>
> The latest comments of the thread have not been addressed yet. so I am
> marking the patch as returned with feedback.  If you think that's not
> correct, please feel free to update the status of the patch.  If you
> do so, please provide at the same time a rebased version of the patch,
> as it is failing to apply on HEAD.

I've finally had some time to update the patchset with an attempt at
addressing Andres' concerns. Note that the current spin has a bug
which does not allow syslogger to run properly. Additionally, it is
squashed into one patch again.  I intend to split the patch out into
separate functional patches and fix the syslogger issue, but wanted to
get this on the ticket for the open CommitFest before it closes. I'll
go ahead and re-add it and will be pushing updates as I have them.

I intend to give this patchset the time it deserves, so will be much
more responsive this go-around.

Thanks,

--
Mike Palmiotto
https://crunchydata.com

0002-Add-a-hook-to-allow-extensions-to-set-worker-metadat.patch (2K) Download Attachment
0001-Introduce-subprocess-infrastructure.patch (118K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Sat, Feb 29, 2020 at 9:51 PM Mike Palmiotto
<[hidden email]> wrote:

>
> On Sat, Nov 30, 2019 at 9:15 PM Michael Paquier <[hidden email]> wrote:
> >
> > On Thu, Oct 03, 2019 at 11:39:37AM -0700, Andres Freund wrote:
> > > Color me unconvinced.
> >
> > The latest comments of the thread have not been addressed yet. so I am
> > marking the patch as returned with feedback.  If you think that's not
> > correct, please feel free to update the status of the patch.  If you
> > do so, please provide at the same time a rebased version of the patch,
> > as it is failing to apply on HEAD.
>
> I've finally had some time to update the patchset with an attempt at
> addressing Andres' concerns. Note that the current spin has a bug
> which does not allow syslogger to run properly. Additionally, it is
> squashed into one patch again.  I intend to split the patch out into
> separate functional patches and fix the syslogger issue, but wanted to
> get this on the ticket for the open CommitFest before it closes. I'll
> go ahead and re-add it and will be pushing updates as I have them.
Okay, here is an updated and rebased patch that passes all regression
tests with and without EXEC_BACKEND. This also treats more of the
issues raised by Andres.

I still need to do the following:
- split giant patch out into separate functional commits
- add translated process descriptions
- fix some variable names here and there (notably the PgSubprocess
struct ".progname" member -- that's a misnomer.
- address some valgrind findings

I should be able to split this out into smaller commits sometime today
and will continue iterating to scratch the other items off the list.

Thanks,
--
Mike Palmiotto
https://crunchydata.com

0002-Add-a-hook-to-allow-extensions-to-set-worker-metadat.patch (2K) Download Attachment
0001-Introduce-subprocess-infrastructure.patch (141K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Mon, Mar 2, 2020 at 9:53 AM Mike Palmiotto
<[hidden email]> wrote:
>
> On Sat, Feb 29, 2020 at 9:51 PM Mike Palmiotto
> <[hidden email]> wrote:
> >
<snip>

> Okay, here is an updated and rebased patch that passes all regression
> tests with and without EXEC_BACKEND. This also treats more of the
> issues raised by Andres.
>
> I still need to do the following:
> - split giant patch out into separate functional commits
> - add translated process descriptions
> - fix some variable names here and there (notably the PgSubprocess
> struct ".progname" member -- that's a misnomer.
> - address some valgrind findings
>
> I should be able to split this out into smaller commits sometime today
> and will continue iterating to scratch the other items off the list.
I've addressed all of the points above (except splitting the base
patch up) and rebased on master. I didn't squash before generating the
patches so it may be easier to read the progress here, or annoying
that there are so many attachments.

This patchset hits all of Andres' points with the exception of the
reshuffling of functions like AuxiliaryProcessMain, but it does get
rid of a lot of direct argv string comparison. I'll reorganize a bit
when I re-write the patchset and will likely move
AuxiliaryProcessMain, along with the Backend subprocess functions but
just wanted to get this up here in the meantime.

There is quite a lot to be gained from this patchset in the form of
re-organization and removal of redundant code-paths/globals. I see
this as a good first step and am willing to continue to push it
forward with any new suggestions.

Thanks,
--
Mike Palmiotto
https://crunchydata.com

0008-Rename-subprocess-init-to-prep.patch (7K) Download Attachment
0011-Use-MyProcPort-instead-of-adding-ConnProcPort.patch (6K) Download Attachment
0007-Translate-descriptions-and-drop-unused-fields.patch (8K) Download Attachment
0010-Update-StartSubprocess-comments.patch (1K) Download Attachment
0009-Move-Backend-specific-code-down.patch (12K) Download Attachment
0006-Fix-up-subprocess-init-logic-and-docs.patch (2K) Download Attachment
0005-Drop-MyBackend-and-handle-backend-fork-failure.patch (14K) Download Attachment
0003-Rearrange-subprocess.c-headers.patch (1K) Download Attachment
0004-Fix-allocation-of-MySubprocess.patch (1K) Download Attachment
0002-Get-rid-of-pstrdup-for-fork-exec-args.patch (1K) Download Attachment
0001-Introduce-subprocess-infrastructure.patch (141K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Mike Palmiotto
On Tue, Mar 3, 2020 at 11:56 PM Mike Palmiotto
<[hidden email]> wrote:

>
> On Mon, Mar 2, 2020 at 9:53 AM Mike Palmiotto
> <[hidden email]> wrote:
> >
> > On Sat, Feb 29, 2020 at 9:51 PM Mike Palmiotto
> > <[hidden email]> wrote:
> > >
> <snip>
> > Okay, here is an updated and rebased patch that passes all regression
> > tests with and without EXEC_BACKEND. This also treats more of the
> > issues raised by Andres.
> >
> > I still need to do the following:
> > - split giant patch out into separate functional commits
> > - add translated process descriptions
> > - fix some variable names here and there (notably the PgSubprocess
> > struct ".progname" member -- that's a misnomer.
> > - address some valgrind findings
> >
> > I should be able to split this out into smaller commits sometime today
> > and will continue iterating to scratch the other items off the list.
>
> I've addressed all of the points above (except splitting the base
> patch up) and rebased on master. I didn't squash before generating the
> patches so it may be easier to read the progress here, or annoying
> that there are so many attachments.
>
> This patchset hits all of Andres' points with the exception of the
> reshuffling of functions like AuxiliaryProcessMain, but it does get
> rid of a lot of direct argv string comparison. I'll reorganize a bit
> when I re-write the patchset and will likely move
> AuxiliaryProcessMain, along with the Backend subprocess functions but
> just wanted to get this up here in the meantime.
>
> There is quite a lot to be gained from this patchset in the form of
> re-organization and removal of redundant code-paths/globals. I see
> this as a good first step and am willing to continue to push it
> forward with any new suggestions.
The patchset is now split out. I've just noticed that Peter Eisentraut
included some changes for a generic MyBackendType, which I should have
been aware of. I was unable to rebase due to these changes, but can
fold these patches into that framework if others think it's
worthwhile.

The resulting diffstat is as follows:

% git diff 24d85952a57b16090ca8ad9cf800fbdd9ddd104f --shortstat
 29 files changed, 1029 insertions(+), 1201 deletions(-)

--
Mike Palmiotto
https://crunchydata.com

0001-Add-subprocess-infrastructure.patch (20K) Download Attachment
0002-Use-centralized-StartSubprocess-for-aux-procs.patch (20K) Download Attachment
0003-Add-AutoVacLauncherType-to-subprocess-struct.patch (10K) Download Attachment
0004-Add-AutoVacuumWorkerType-to-subprocess-struct.patch (8K) Download Attachment
0005-Add-PgstatCollectorType-to-subprocess-struct.patch (15K) Download Attachment
0006-Add-PgArchiverType-to-subprocess-struct.patch (10K) Download Attachment
0007-Add-SysLoggerType-to-subprocess-struct.patch (23K) Download Attachment
0008-Add-BgWorkerType-to-subprocess-struct.patch (35K) Download Attachment
0009-Add-Backends-to-subprocess-struct.patch (21K) Download Attachment
0010-Add-WalSenderType-to-subprocess-struct.patch (1K) Download Attachment
0011-Get-rid-of-st_beckendType-and-use-MySubprocess.patch (9K) Download Attachment
0012-Move-away-from-argv-string-comparison.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Justin Pryzby
On Tue, Mar 17, 2020 at 02:50:19PM -0400, Mike Palmiotto wrote:
> The patchset is now split out. I've just noticed that Peter Eisentraut
> included some changes for a generic MyBackendType, which I should have
> been aware of. I was unable to rebase due to these changes, but can
> fold these patches into that framework if others think it's
> worthwhile.

I don't have many comments on the patch, but it's easy enough to rebase.

I think maybe you'll want to do something more with this new function:
GetBackendTypeDesc()

+       /* Don't panic. */

+1

--
Justin

v2-0001-Add-subprocess-infrastructure.patch (14K) Download Attachment
v2-0002-Use-centralized-StartSubprocess-for-aux-procs.patch (15K) Download Attachment
v2-0003-Add-AutoVacLauncherType-to-subprocess-struct.patch (7K) Download Attachment
v2-0004-Add-AutoVacuumWorkerType-to-subprocess-struct.patch (6K) Download Attachment
v2-0005-Add-PgstatCollectorType-to-subprocess-struct.patch (12K) Download Attachment
v2-0006-Add-PgArchiverType-to-subprocess-struct.patch (7K) Download Attachment
v2-0007-Add-SysLoggerType-to-subprocess-struct.patch (17K) Download Attachment
v2-0008-Add-BgWorkerType-to-subprocess-struct.patch (26K) Download Attachment
v2-0009-Add-Backends-to-subprocess-struct.patch (16K) Download Attachment
v2-0010-Add-WalSenderType-to-subprocess-struct.patch (1K) Download Attachment
v2-0011-Get-rid-of-st_beckendType-and-use-MySubprocess.patch (4K) Download Attachment
v2-0012-Move-away-from-argv-string-comparison.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Auxiliary Processes and MyAuxProc

Alvaro Herrera-9
On 2020-Mar-17, Justin Pryzby wrote:

> +static PgSubprocess process_types[] = {
> + {
> + .desc = "checker",
> + .entrypoint = CheckerModeMain
> + },
> + {
> + .desc = "bootstrap",
> + .entrypoint = BootstrapModeMain
> + },

Maybe this stuff can be resolved using a technique like rmgrlist.h or
cmdtaglist.h.  That way it's not in two separate files.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


12