On Thu, Mar 19, 2020 at 4:57 PM Andres Freund <andres@anarazel.de> wrote:
> On 2020-03-19 11:35:41 +0100, Peter Eisentraut wrote:
> > On 2020-03-18 17:07, Mike Palmiotto wrote:
> > > On Wed, Mar 18, 2020 at 11:26 AM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> > > >
> > > > On Wed, Mar 18, 2020 at 10:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > > > Also, I saw this was failing tests both before and after my rebase.
> > > > >
> > > > > http://cfbot.cputube.org/
> > > > > https://ci.appveyor.com/project/postgresql-cfbot/postgresql/builds/31535161
> > > > > https://ci.appveyor.com/project/postgresql-cfbot/postgresql/builds/31386446
> > > >
> > > > Good catch, thanks. Will address this as well in the next round. Just
> > > > need to set up a Windows dev environment to see if I can
> > > > reproduce/fix.
I'm still working on wiring up an AppVeyor instance, as seemingly
builds don't work on any of the default Azure/Visual Studio images. In
the meantime, I've fixed some spurious whitespace changes and the
compile error for non-EXEC_BACKEND. I'm posting a new version to keep
Travis happy at least while I keep working on that. Sorry for the
delay.
> On a green field, I would say that there should be one or two C
> arrays-of-structs defining subprocesses. And most behaviour should be
> channeled through that.
>
> struct PgProcessType
> {
> const char* name;
> PgProcessBeforeShmem before_shmem;
> PgProcessEntry entrypoint;
> uint8:1 only_one_exists;
> uint8:1 uses_shared_memory;
> uint8:1 client_connected;
> uint8:1 database_connected;
> ...
> };
Only some of these are currently included in the process_types struct,
but this demonstrates the extensibility of the architecture and room
for future centralization. Do you see any items in this set that
aren't currently included but are must-haves for this round?
> Then there should be a single startup routine for all postmaster
> children. Since most of the startup is actually shared between all the
> different types of processes, we can declutter it a lot.
Agreed.
> When starting a process postmaster would just specify the process type,
> and if relevant, an argument (struct Port for backends, whatever
> relevant for bgworkers etc) . Generic code should handle all the work
> until the process type entry point - and likely we should move more work
> from the individual process types into generic code.
>
> If a process is 'only_one_exists' (to be renamed), the generic code
> would also (in postmaster) register the pid as
> subprocess_pids[type] = pid;
> which would make it easy to only have per-type code in the few locations
> that need to be aware, instead of many locations in
> postmaster.c. Perhaps also some shared memory location.
All of this sounds good.
> Coming back to earth, from green field imagination land: I think the
> patchset does go in that direction (to be fair, I think I outlined
> something like the above elsewhere in this thread in a discussion with
> Mike). And that's good.
Thanks for chiming in. Is there anything else you think needs to go in
this version to push things along -- besides fixing Windows builds, of
course?
Regards,
--
Mike Palmiotto
https://crunchydata.com