Re: fork/exec patch: pre-CreateProcess finalization - Mailing list pgsql-patches

Claudio Natoli <claudio.natoli@memetrics.com> writes:
>> I don't follow your thinking here.  The information that has to be
>> reloaded *must* be passed across the fork/exec boundary in
>> either case.

> Yes. But not all of it.

I'll throw that same argument back at you: some of the information
needed for PostgresMain's command line is available to the postmaster.
But not all of it.  Therefore we have to change things around.  I'm
proposing solving this by introducing a new layer of code that's
essentially separate from the non-fork-exec case.  Your solution seems
much messier, and it doesn't have any redeeming social value (that I can
see) to justify the mess.

> How about things like, for instance, canAcceptConnections(). To make this
> call successfully in a fork/exec'd child would take a bit of work.

And your point is what?  As long as you grant that the fork must occur
before we begin talking to the client, this information will have to be
passed down somehow (offhand, a command-line argument for the result of
canAcceptConnections() seems fairly reasonable).  Obscuring the division
between "sub-postmaster" code and PostgresMain isn't going to save you
from that.

> What I was suggesting with b) was to format up the command line for the
> items prefixed by * in the postmaster,
> do the fork (or fork/exec), and then run the authentication in, say
> PostgresMain. (Note: this is essentially what the fork/exec case currently
> does).

Yeah, I noticed.  If I'd been paying more attention, the already-applied
patch wouldn't have gotten in either, because it already has created
what I consider an unacceptable amount of coupling between PostgresMain
and the sub-postmaster code.  If it's still like that when the dust
settles then I will go in and change it myself.  PostgresMain has no
business doing *any* of the stuff that is currently #ifdef EXEC_BACKEND
in postgres.c.  It should certainly not be running authentication, and I
see no reason for it to be messing with restoring state that should have
been inherited from the postmaster.  That just creates fragility and
order-of-operations dependency.  As an example, it doesn't seem like a
good idea to me that reloading postmaster GUC variables happens after
scanning of PostgresMain command line options, because the option
processing both uses and sets GUC values.  It is not hard to point out
three or four bugs associated with that alone, including security
violations (since we don't know at this point whether the user is a
superuser and thus don't know what he's allowed to set).  In general,
PostgresMain assumes it is started with the inherited environment
already set up, and I don't see a good argument for changing that.

> Now, what I think you are suggesting (correct me if I'm wrong), is that we
> should pass along whatever we need to in order to be able to setup the
> fork/exec'd process to run BackendFork more or less unchanged.

I don't mind making localized changes like passing in the result of
canAcceptConnections, but by and large, yes: I want to run BackendFork
pretty much as-is.  I see no strong reason to do differently, and I am
convinced that we will spend the next six months ferreting out startup
bugs if we make wholesale revisions in the order in which things are
done.

            regards, tom lane