Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization

Hi Bruce,

> Claudio, where are we on this patch? I see an even newer version in the
archives:
>
>
> http://archives.postgresql.org/pgsql-patches/2003-12/msg00361.php

[Weird you and Google groups "missed" it!]

> Anyway, Tom has looked at your newest patch and it looks good to him.

I'm happy with the patch from the link above being committed if Tom has no
more comments. Was only waiting for a final nod from him.

Once it is in the source tree, give me a couple days and I'll fire off a
patch with the actual CreateProcess calls... and then we are off into Win32
signal land [shudder].

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 wrote:
>
> Hi Bruce,
>
> > Claudio, where are we on this patch? I see an even newer version in the
> archives:
> >
> >
> > http://archives.postgresql.org/pgsql-patches/2003-12/msg00361.php
>
> [Weird you and Google groups "missed" it!]

No, seems only _I_ missed it. The google display for groups:

http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&group=comp.databases.postgresql.patches

Doesn't show individual messages but lists threads, _and_ it shows the
date of the last message in the thread, rather than the first. That's
how I missed it. How I missed it my mailbox, I have no idea. Sorry.

> > Anyway, Tom has looked at your newest patch and it looks good to him.
>
> I'm happy with the patch from the link above being committed if Tom has no
> more comments. Was only waiting for a final nod from him.

Thanks. Applied.

> Once it is in the source tree, give me a couple days and I'll fire off a
> patch with the actual CreateProcess calls... and then we are off into Win32
> signal land [shudder].

Great.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Claudio Natoli wrote:

Something after 2003/11/20 enhanced the query cancel handling. Namely,
CVS tip now responds to a query cancel with a postmaster restart
canceling all queries. Could the fork/exec stuff be responsible for this?

Jan

> Hi Bruce,
>
>> Claudio, where are we on this patch? I see an even newer version in the
> archives:
>>
>>
>> http://archives.postgresql.org/pgsql-patches/2003-12/msg00361.php
>
> [Weird you and Google groups "missed" it!]
>
>
>> Anyway, Tom has looked at your newest patch and it looks good to him.
>
> I'm happy with the patch from the link above being committed if Tom has no
> more comments. Was only waiting for a final nod from him.
>
> Once it is in the source tree, give me a couple days and I'll fire off a
> patch with the actual CreateProcess calls... and then we are off into Win32
> signal land [shudder].
>
> 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>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck(at)Yahoo(dot)com #

Jan Wieck <JanWieck(at)Yahoo(dot)com> writes:
> Something after 2003/11/20 enhanced the query cancel handling. Namely,
> CVS tip now responds to a query cancel with a postmaster restart
> canceling all queries. Could the fork/exec stuff be responsible for this?

Whoever changed this:

status = ProcessStartupPacket(port, false);

if (status != STATUS_OK)
return 0; /* cancel request processed, or error */

to this:

status = ProcessStartupPacket(port, false);

if (status != STATUS_OK)
{
ereport(LOG,
(errmsg("connection startup failed")));
proc_exit(status);
}

is responsible. May we have an explanation of the thought process,
if any?

regards, tom lane

Tom Lane wrote:
> Jan Wieck <JanWieck(at)Yahoo(dot)com> writes:
> > Something after 2003/11/20 enhanced the query cancel handling. Namely,
> > CVS tip now responds to a query cancel with a postmaster restart
> > canceling all queries. Could the fork/exec stuff be responsible for this?
>
> Whoever changed this:
>
> status = ProcessStartupPacket(port, false);
>
> if (status != STATUS_OK)
> return 0; /* cancel request processed, or error */
>
> to this:
>
> status = ProcessStartupPacket(port, false);
>
> if (status != STATUS_OK)
> {
> ereport(LOG,
> (errmsg("connection startup failed")));
> proc_exit(status);
> }
>
> is responsible. May we have an explanation of the thought process,
> if any?

My guess is that this used to proc_exit the postgres backend, but now
proc_exits the postmaster, but that is only a guess.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> My guess is that this used to proc_exit the postgres backend, but now
> proc_exits the postmaster, but that is only a guess.

No. This is post-fork (and had better remain so). The change causes
the sub-postmaster that has just finished handling a cancel request
to exit with nonzero status, which the postmaster then interprets
(correctly) as a child process crash.

BTW, how are we going to do cancels in Windows-land? The sub-postmaster
isn't gonna have access to the postmaster's list of child PIDs and
cancel keys ...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > My guess is that this used to proc_exit the postgres backend, but now
> > proc_exits the postmaster, but that is only a guess.
>
> No. This is post-fork (and had better remain so). The change causes
> the sub-postmaster that has just finished handling a cancel request
> to exit with nonzero status, which the postmaster then interprets
> (correctly) as a child process crash.
>
> BTW, how are we going to do cancels in Windows-land? The sub-postmaster
> isn't gonna have access to the postmaster's list of child PIDs and
> cancel keys ...

I think the postmaster will have access to the cancel key and pid. It
should work similar to Unix. However, I do have a win32 TODO item on
"test cancel key", so we will not forget about it.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > My guess is that this used to proc_exit the postgres backend, but now
> > proc_exits the postmaster, but that is only a guess.
>
> No. This is post-fork (and had better remain so). The change causes
> the sub-postmaster that has just finished handling a cancel request
> to exit with nonzero status, which the postmaster then interprets
> (correctly) as a child process crash.
>
> BTW, how are we going to do cancels in Windows-land? The sub-postmaster
> isn't gonna have access to the postmaster's list of child PIDs and
> cancel keys ...

When you say sub-postmaster, you mean we fork, then process the cancel
request? Seems we might need special handling in there, yea.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> When you say sub-postmaster, you mean we fork, then process the cancel
> request? Seems we might need special handling in there, yea.

We fork before we even read the request. Otherwise an attacker can
trivially lock up the postmaster by sending a partial startup packet.

I've just noticed another serious bit of breakage in CVS tip (though
this might be fixed by Claudio's pending patch that reverts a lot of
the code rearrangements). There is a reason why the 7.4 code does
on_exit_reset *immediately* after fork(), and it's not acceptable to
rearrange the code so that that happens at some random later time.
Otherwise, any problem in between leads to the child process executing
the postmaster's on_exit routines, with such pleasant side effects as
destroying the shmem segment. For similar reasons, you don't get to
postpone setting IsUnderPostmaster true --- elog pays attention to the
value of that flag, and will do the wrong thing if called in a child
process that doesn't yet have it set.

regards, tom lane

Tom Lane wrote:
> Jan Wieck <JanWieck(at)Yahoo(dot)com> writes:
> > Something after 2003/11/20 enhanced the query cancel handling. Namely,
> > CVS tip now responds to a query cancel with a postmaster restart
> > canceling all queries. Could the fork/exec stuff be responsible for this?
>
> Whoever changed this:
>
> status = ProcessStartupPacket(port, false);
>
> if (status != STATUS_OK)
> return 0; /* cancel request processed, or error */
>
> to this:
>
> status = ProcessStartupPacket(port, false);
>
> if (status != STATUS_OK)
> {
> ereport(LOG,
> (errmsg("connection startup failed")));
> proc_exit(status);
> }
>
> is responsible. May we have an explanation of the thought process,
> if any?

Claudio specified the attached fix, which I have applied.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> Jan Wieck <JanWieck(at)Yahoo(dot)com> writes:
> > Something after 2003/11/20 enhanced the query cancel handling. Namely,
> > CVS tip now responds to a query cancel with a postmaster restart
> > canceling all queries. Could the fork/exec stuff be responsible for this?
>
> Whoever changed this:
>
> status = ProcessStartupPacket(port, false);
>
> if (status != STATUS_OK)
> return 0; /* cancel request processed, or error */
>
> to this:
>
> status = ProcessStartupPacket(port, false);
>
> if (status != STATUS_OK)
> {
> ereport(LOG,
> (errmsg("connection startup failed")));
> proc_exit(status);
> }
>
> is responsible. May we have an explanation of the thought process,
> if any?

Claudio specified the attached fix, which I have applied (this time).

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Claudio specified the attached fix, which I have applied (this time).

The ereport must vanish back into its black hole, also.
ProcessStartupPacket has already issued any appropriate log message.

> *** 2450,2456 ****
> {
> ereport(LOG,
> (errmsg("connection startup failed")));
> ! proc_exit(status);
> }

> /*
> --- 2450,2456 ----
> {
> ereport(LOG,
> (errmsg("connection startup failed")));
> ! proc_exit(0);
> }

> /*

regards, tom lane

Done and attached.

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Claudio specified the attached fix, which I have applied (this time).
>
> The ereport must vanish back into its black hole, also.
> ProcessStartupPacket has already issued any appropriate log message.
>
> > *** 2450,2456 ****
> > {
> > ereport(LOG,
> > (errmsg("connection startup failed")));
> > ! proc_exit(status);
> > }
>
> > /*
> > --- 2450,2456 ----
> > {
> > ereport(LOG,
> > (errmsg("connection startup failed")));
> > ! proc_exit(0);
> > }
>
> > /*
>
> regards, tom lane
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073