Thread: fork/exec
Looking at postmaster.c, my head started to spin a little. I think I understood that exec case or not, by the time we get to BackendRun we have already done all the fork/exec action. Have I read this correctly? (This code is getting rather intricate. A Readme file might be nice. Just a thought) cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> (This code is getting rather intricate.
It needs rewritten. The hacks inserted for ExecBackend have largely
destroyed the former structure (such as it was), and the lack of any
commentary added with said hacks didn't help. I am thinking of tackling
that rewrite once the dust has settled.
regards, tom lane
> Looking at postmaster.c, my head started to spin a little. I think I > understood that exec case or not, by the time we get to BackendRun we > have already done all the fork/exec action. Have I read this correctly? Yes. In the normal case, fork() then BackendRun. In the EXEC_BACKEND case, fork/exec (or CreateProcess), which then invokes BackendRun via SubPostmasterMain. > (This code is getting rather intricate. A Readme file might be nice. Just a thought) Which bits in particular? 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>
> It needs rewritten. The hacks inserted for ExecBackend have largely > destroyed the former structure (such as it was), and the lack of any > commentary added with said hacks didn't help. I am thinking > of tackling that rewrite once the dust has settled. Anything in particular? With your SubPostmaster suggestion, the split of BackendRun (ne: BackendFork) and BackendInit could now be undone (and I had a TODO item to ask the list if such a reversion was desirable, and provide said patch)... but other than that I don't see where any former structure re: backend creation has been destroyed. 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.natoli@memetrics.com> writes:
>> It needs rewritten. The hacks inserted for ExecBackend have largely
>> destroyed the former structure (such as it was), and the lack of any
>> commentary added with said hacks didn't help. I am thinking
>> of tackling that rewrite once the dust has settled.
> Anything in particular?
> With your SubPostmaster suggestion, the split of BackendRun (ne:
> BackendFork) and BackendInit could now be undone (and I had a TODO item to
> ask the list if such a reversion was desirable, and provide said patch)...
I had intended to do that, but in general it seems like there's more
jumping around now. Here's an example that annoyed me recently when
I was looking at IsUnderPostmaster uses. At about line 370 of
bootstrap.c:
/* Acquire configuration parameters */ if (IsUnderPostmaster) {
#ifdef EXEC_BACKEND read_nondefault_variables(); read_backend_variables(backendID,NULL);
SSDataBaseInit(xlogop);
#endif } else ProcessConfigFile(PGC_POSTMASTER);
What does SSDataBaseInit have to do with acquiring configuration
parameters? (Answer: nothing.) Why is it being called here at all,
and why only in the EXEC_BACKEND case? Sure can't figure that out from
the uncommented code. Is it a good idea to do it? I rather doubt it
--- that routine does on_exit_reset, which would lose any previously
registered exit callbacks, which is something that should only happen
immediately after fork(), not as far down in BootstrapMain as this is
being done.
But Andrew's complaint already had some merit even before this work
started. We probably ought to rear back and contemplate the overall
structure of postmaster.c and postgres.c, and see what we can do to make
it more comprehensible. I don't have any immediate thoughts about what
to do, just a bee in my bonnet that it needs to be done. Any ideas?
regards, tom lane
Tom Lane wrote: > What does SSDataBaseInit have to do with acquiring configuration > parameters? (Answer: nothing.) Why is it being called here at all, > and why only in the EXEC_BACKEND case? Sure can't figure > that out from > the uncommented code. Is it a good idea to do it? I rather doubt it > --- that routine does on_exit_reset, which would lose any previously > registered exit callbacks, which is something that should only happen > immediately after fork(), not as far down in BootstrapMain as this is > being done. That is a remaining wart. Having fixed a similar pattern in the backend creation, I asked whether or not fixing the Bootstrap process creation was overkill, and took no answer to be in the affirmative... > But Andrew's complaint already had some merit even before this work > started. We probably ought to rear back and contemplate the overall > structure of postmaster.c and postgres.c, and see what we can > do to make it more comprehensible. I don't have any immediate thoughts > about what to do, just a bee in my bonnet that it needs to be done. Any ideas? If you like, I'll quickly provide a reversion of the BackendRun/Init split, and perhaps implement a "SubBootstrapMain"-ism... but this doesn't really help things overall. Is it at least a better point to start from? 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.natoli@memetrics.com> writes:
> If you like, I'll quickly provide a reversion of the BackendRun/Init split,
> and perhaps implement a "SubBootstrapMain"-ism... but this doesn't really
> help things overall. Is it at least a better point to start from?
I'd say think first and code later. Do we want to reunify those
routines, or is there some purpose in having a split? Can't see
what offhand, but the whole problem is this code has gotten away
from us complexity-wise. We should first think about what the
structure ought to be.
One thought I had was that there should be a common subroutine to do the
stuff that a new postmaster subprocess needs to do immediately at
startup. This includes setting IsUnderPostmaster, MyProcPid,
on_exit_reset in the fork() case, reloading global variables in the
exec() case, and maybe a couple other things I've forgotten. We've
allowed that code to get duplicated across several places now. I had
been thinking that this was solely an issue for the exec case (and thus
that hiding it in a SubPostmasterMain layer would solve the problem),
but really there are several critical things that have to happen there
in the fork case as well. We could have a PostmasterChildSetup routine
that is called in both cases, and behaves slightly differently in each
case, but once it returns the overall state of the new subprocess is the
same in both cases.
regards, tom lane
> One thought I had was that there should be a common subroutine to do the > stuff that a new postmaster subprocess needs to do immediately at > startup. This includes setting IsUnderPostmaster, MyProcPid, > on_exit_reset in the fork() case, reloading global variables in the > exec() case, and maybe a couple other things I've forgotten. We've > allowed that code to get duplicated across several places now. Nice. Could perhaps also get it to do the ugly and oft repeated setitimer(ITIMER_PROF, &prof_itimer, NULL) and beos_backend_startup() bits, which currently contribute greatly in making the post-fork code unclear. Flip you for it? :-) 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.natoli@memetrics.com> writes:
> Nice. Could perhaps also get it to do the ugly and oft repeated
> setitimer(ITIMER_PROF, &prof_itimer, NULL) and beos_backend_startup() bits,
> which currently contribute greatly in making the post-fork code unclear.
Good point.
> Flip you for it? :-)
Go for it, I've got plenty on my plate already ...
regards, tom lane
Claudio Natoli said: > >> Looking at postmaster.c, my head started to spin a little. I think I >> understood that exec case or not, by the time we get to BackendRun we >> have already done all the fork/exec action. Have I read this >> correctly? > > Yes. In the normal case, fork() then BackendRun. In the EXEC_BACKEND > case, fork/exec (or CreateProcess), which then invokes BackendRun via > SubPostmasterMain. > > >> (This code is getting rather intricate. A Readme file might be nice. > Just a thought) > > Which bits in particular? > My concern was that I wanted to make sure that whatever I did was available to the backend, and in the presence of exec/CreateProcess I would need either to make sure it got passed to the backend if done before, or to make sure that it was done after. That's why I wanted to have the sequence of events clear in my head. cheers andrew