Re: fork/exec patch: pre-CreateProcess finalization

Thanks for your comments Tom.

Tom Lane writes:
> Claudio Natoli <claudio(dot)natoli(at)memetrics(dot)com> writes:
> > This has required some reworking of the existing code base, particularly
to
> > BackendFork (which now actually does the fork()). As such, I've been
> > anticipating that this will be the most controversial of
> the fork/exec patches, so critique away :-)
>
> You haven't explained why that's necessary. Given the fact that this
> patch seems to hugely complicate the postmaster logic --- not so much
> either path individually, but the messy #ifdef interleaving of two
> radically different programs --- I am inclined to reject it on style
> grounds alone.

I have to agree, as I wasn't particularly happy with the BackendFork section
of this patch. You didn't really seem to comment on the pgstat changes,
which were much cleaner, so perhaps (like me) you were happier with these
changes.

So, could you give me an indication as to whether or not making changes to
the BackendFork logic as done for the pgstat logic (specifically, replacing
fork() call with a new, argument marshalling routine to do fork/exec) would
be more acceptable? The reason I avoided this in the BackendFork case was,
predominantly, code duplication. Creating a new "BackendForkExec" would, I
think, result in a lot of similarity between the two routines. [yes,
BackendFork is poorly named, although I'd still think of moving the fork()
call into BackendFork anyway, thereby justifying its name. But, much more
importantly, for symmetry with the code format that will inevitably arise in
the fork/exec case; see below]

> We should either find a way to make the fork/exec path more like the
> existing code, or bite the bullet and change them both. Doing it the
> way you have here will create an unmaintainable mess. I'm not prepared
> to work on a postmaster in which a step as basic as fork() happens in
> two different places depending on an #ifdef.

["unmaintainable mess": yeah, I had the exact same thought when I'd
finished. "No one's ever going to be able to rework this properly".]

I'm afraid that, short of removing all SubPostmaster actions from
BackendFork, the need to do fork in two different places (at least,
physically, if not logically different) will remain. After fork/exec'ing,
the child process can't recover the context needed to create the argument
list which it'll need to build up the PostgresMain arg list.

So, in summary, I see two solutions:

a) do something similar to BackendFork as done for pgstat, specifically:
- move the fork call into BackendFork
- create a fork/exec equivalent to BackendFork
- #ifdef the call to the appropriate function in BackendStartup
PRO: child is forked in the same logical place
CON: possible duplication in code between BackendFork + BackendForkExec

b) delay the fork() call to the end of BackendFork in both fork/exec and
normal cases, turning it into solely an argument formatter for PostgresMain
- move BackendInit, port->cmdline_options parsing + MemoryContext
calls into PostgresMain
PRO: forking in same (physical) location in code; no duplication
CON: additional complexity at the start of PostgresMain to avoid these
calls in stand-alone backend case

Which is the lesser of two evils? FWIW, I prefer the latter, as it makes the
normal + fork/exec cases effectively identical, for a little added
complexity to PostgresMain.

There's also the alternative you outlined, but I think it is less workable
than the above options:

> Now it seems to me that a lot of the mess in your latest patch comes
> from trying to set up the backend command string before you've forked,
> and therefore before you've done any of the sub-postmaster actions.
> That's not gonna work.

I agree that this is where the mess comes from, but I think this (setting up
backend command string before fork) is *exactly* what needs to occur in the
fork/exec case (leading into the Win32 CreateProcess calls). If we want this
code to work in the Win32 case, we basically can't do any processing between
the fork + exec calls, and I think recovering all the context to do the
command string marshalling post fork/exec is untenable.

> What I'd suggest is adding a new entry point from main.c, say
> SubPostmasterMain(), with its own command line syntax. Perhaps
> postmaster -fork ... additional args ...
> (where -fork is what cues main.c where to dispatch to, and the
> additional args are just those that need to be passed from the
> postmaster to the sub-postmaster, without any of the additional
> information that will be learned by the sub-postmaster during
> client authentication.)
>
> In the Windows case the postmaster would fork/exec to this routine,
> which would re-load as much of the postmaster context as was needed
> and then call BackendFork (which we really oughta rename to something
> else). BackendFork would execute the client auth process and then
> simply call PostgresMain with a correctly constructed argument list.

Here's the critical difference in our thinking:

ISTM that we'll have to go to a lot of work to get the fork/exec
case to re-load the context required to format up the argument list to
PostgresMain. Certainly, it is a lot more work than allowing the postmaster
itself (not the forked child) to create the argument list, albeit moving
auth to PostgresMain.

Ok. So, in conclusion we are in agreement on at least one thing: the current
patch sucks.

But, I suspect we may have an impasse on a sole crucial point: setting up
the backend command string before forking. I'm all for it, and you seem
pretty much against it.

So, I guess I could try to bring you around with another, cleaner patch
using one of the alternatives listed above. But, before doing that, I guess
I should give you a chance to:
a) try to convince me otherwise
b) indicate which of the two alternatives you (even grudgingly :-) prefer
(or another alternative in light of the above)

Again, thanks for your comments, and looking forward to your reply (so that,
hopefully, I can get cracking on a new patch).

Cheers,
Claudio

---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Claudio Natoli <claudio(dot)natoli(at)memetrics(dot)com> writes:
> Ok. So, in conclusion we are in agreement on at least one thing: the current
> patch sucks.

Good, I'm glad we see eye to eye on that ;-)

> Here's the critical difference in our thinking:

> ISTM that we'll have to go to a lot of work to get the fork/exec
> case to re-load the context required to format up the argument list to
> PostgresMain. Certainly, it is a lot more work than allowing the postmaster
> itself (not the forked child) to create the argument list, albeit moving
> auth to PostgresMain.

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.
For example, there is no alternative to telling the backend process
where PGDATA is: it's got to be passed down some way or another. The
Unix implementation assumes it can just inherit the value of a static
variable, while the Windows version will have to do something else.
I take no position here on whether we pass it as a command line item,
or through a temporary file, or whatever: it has to be passed down.

The reason your patch is messy is that the PostgresMain command line
string involves both information passed down from the postmaster and
information acquired from the client's connection-request packet.
We could consider redesigning that command line behavior, but we don't
really want to since it would impact the standalone-backend case. So
I'm proposing substituting two levels of command-line processing within
the exec'd backend process. The first level is responsible for
transferring information inherited from the postmaster (which we are
going to have to do *anyway*, somewhere, and this provides a nice
localized place to do it) and then the second level is fully compatible
with the standalone-backend case and the Unix-fork case.

> I'm afraid that, short of removing all SubPostmaster actions from
> BackendFork, the need to do fork in two different places (at least,
> physically, if not logically different) will remain.

I am not by any means saying that the fork() call has to stay exactly
where it is in the existing code. We clearly want to refactor things
so that fork() is close to the invocation of FooMain() (or equivalently
exec() in the Windows case). I think it's a historical artifact that
these steps got separated in the existing code base.

> After fork/exec'ing,
> the child process can't recover the context needed to create the argument
> list which it'll need to build up the PostgresMain arg list.

If that's literally true, then we are screwed because things cannot
work. Pardon me for doubting this statement. All that information
*must* be available in the child, once it has analyzed the information
it must be able to collect from the postmaster and performed the client
authentication handshake.

> So, in summary, I see two solutions:

I still think there's a third answer: create an intermediate layer
that's responsible for recovering the information that has to be
inherited from the parent process. I don't say this requires zero
rearrangement of existing code, I just say it's a more maintainable
design than either of the alternatives you suggest.

In particular, this alternative is quite unworkable:

> b) delay the fork() call to the end of BackendFork in both fork/exec and
> normal cases, turning it into solely an argument formatter for PostgresMain

We can *not* postpone the fork() until after client authentication.
That loses all of the work that's been done since circa 7.1 to eliminate
cases where the postmaster gets blocked waiting for a single client to
respond. SSL, PAM, and I think Kerberos all create denial-of-service
risks if we do that.

I believe most of what you are presently struggling with revolves
around the fact that the fork() got pushed up to happen before client
authentication, while the interface to PostgresMain didn't change.
I do not want to alter either of those decisions, however. What we need
is to provide an alternative exec-able interface that encapsulates the
processing that now occurs between these steps.

regards, tom lane