Re: [PATCHES] fork/exec patch: CreateProcess calls for Win32
Patch applied. Thanks. --- Claudio Natoli wrote: > > For application to HEAD, pending community review. > > Drops in the CreateProcess calls for Win32 (essentially wrapping up the > fork/exec portion of the port), and fixes a handful of whitespace issues > (that no doubt I'm to blame for; my apologies) in the source base. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 7: don't forget to increase your free space map settings -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Done and attached.
---
Tom Lane wrote:
> Bruce Momjian <[EMAIL PROTECTED]> 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
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: src/backend/postmaster/postmaster.c
===
RCS file: /cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v
retrieving revision 1.356
diff -c -c -r1.356 postmaster.c
*** src/backend/postmaster/postmaster.c 9 Jan 2004 23:11:39 - 1.356
--- src/backend/postmaster/postmaster.c 9 Jan 2004 23:25:36 -
***
*** 2447,2457
status = ProcessStartupPacket(port, false);
if (status != STATUS_OK)
- {
- ereport(LOG,
- (errmsg("connection startup failed")));
proc_exit(0);
- }
/*
* Now that we have the user and database name, we can set the process
--- 2447,2453
---(end of broadcast)---
TIP 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian <[EMAIL PROTECTED]> 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
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote:
> Jan Wieck <[EMAIL PROTECTED]> 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
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: src/backend/postmaster/postmaster.c
===
RCS file: /cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v
retrieving revision 1.355
diff -c -c -r1.355 postmaster.c
*** src/backend/postmaster/postmaster.c 7 Jan 2004 18:56:27 - 1.355
--- src/backend/postmaster/postmaster.c 9 Jan 2004 23:08:01 -
***
*** 2450,2456
{
ereport(LOG,
(errmsg("connection startup failed")));
! proc_exit(status);
}
/*
--- 2450,2456
{
ereport(LOG,
(errmsg("connection startup failed")));
! proc_exit(0);
}
/*
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote:
> Jan Wieck <[EMAIL PROTECTED]> 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
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: CreateProcess calls for Win32
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --- Claudio Natoli wrote: > > For application to HEAD, pending community review. > > Drops in the CreateProcess calls for Win32 (essentially wrapping up the > fork/exec portion of the port), and fixes a handful of whitespace issues > (that no doubt I'm to blame for; my apologies) in the source base. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 7: don't forget to increase your free space map settings -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> writes: > Actually, if I was going to argue anything, I'd say that if a backend goes > nuts and zeroes the whole shmem segment you've probably some bigger things > to worry about (Aside: Would postgres actually recover from such an > occurence? BTW, I'd be pretty impressed if it did, but not all that > surprised ;-). It should, although there are limits (for instance, if someone is actively writing out a page of WAL at the same time the bogus backend comes by and zeroes that buffer, you might lose WAL entries for already-committed transactions, which would be unhappy-making). As a developer, though, I crash backends all the time, and I can say that this mechanism is indeed pretty robust. The postmaster never goes down (what, never? well, hardly ever) and it's *extremely* seldom that a crash results in on-disk corruption, because the postmaster generally manages to kill the other backends before any corruption in shared memory gets propagated to disk. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: > What it comes down to is I don't want the postmaster to be keeping its > own state in shared memory --- that is, the array must be write-only > memory as far as the postmaster is concerned. If we eliminate the Ok. Well, I can appreciate things from that point of view. > postmaster's private DLList of backends, then the postmaster becomes > that much more vulnerable to corruption from a backend bug that leads to > trashing shared memory. To take just one example: backend A goes nuts, > zeroes the whole shmem segment, and then dumps core. How is the > postmaster going to kill the other backends so that it can begin the > recovery process? If there's no record of their PIDs left anywhere, > it's got a problem. The postmaster *needs* its own copy of that DLList. > > You might object that backend bugs could clobber the array and thus > interfere with cancel request processing, but that's not nearly as > critical a function. Actually, if I was going to argue anything, I'd say that if a backend goes nuts and zeroes the whole shmem segment you've probably some bigger things to worry about (Aside: Would postgres actually recover from such an occurence? BTW, I'd be pretty impressed if it did, but not all that surprised ;-). But I'll happily concede the point, and prove it by knocking up a patch for it over the weekend (unless anyone else particularly wants to). Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> writes: > I wasn't suggesting that we somehow put the Dllist in shared memory. I was > advocating completely replacing the BackendList dlllist with this array, > under both Win32 and *nix case. Right, that's what I understood you to be proposing, and I dislike it on robustness grounds. One of the most important functions of the postmaster is to survive backend crashes --- as soon as you put any of its state where backends could clobber it, you compromise that function. regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> I think the simplest way to make this work is to use an array that's >> 2*MaxBackend items long (corresponding to the max number of children the >> postmaster will fork). > Actually, now that I think about it, is even that big enough. There is a > reason BackendList is a list. In pathological situations, the postmaster > could be made to fork a much larger number than 2*MaxBackend simultaneous > children, although only this many will be allowed to become backends. > (I guess we could check the port->canAcceptConnections value, and not add > the backend to the array when == CAC_TOOMANY). Probably we could rearrange the logic to test for too-many-children before we even do the fork. It'd be a little uglier that way, but not out of the question. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> writes: > Tom Lane: >> Per Jan's comment, there is no need to mess with the existing >> datastructure. I'd think more of *copying* the dllist into some array >> in shared memory. This is code that would only need to exist in the >> Windows port. > Also, let's get back to why we want this: to handle processCancelRequest in > the Win32 case. If this array is in Windows only, then we'll obviously need > two implementations of the processCancelRequest logic. [ shrug... ] If that's the only redundant code we end up with in this port, we'll have done pretty durn well. What it comes down to is I don't want the postmaster to be keeping its own state in shared memory --- that is, the array must be write-only memory as far as the postmaster is concerned. If we eliminate the postmaster's private DLList of backends, then the postmaster becomes that much more vulnerable to corruption from a backend bug that leads to trashing shared memory. To take just one example: backend A goes nuts, zeroes the whole shmem segment, and then dumps core. How is the postmaster going to kill the other backends so that it can begin the recovery process? If there's no record of their PIDs left anywhere, it's got a problem. The postmaster *needs* its own copy of that DLList. You might object that backend bugs could clobber the array and thus interfere with cancel request processing, but that's not nearly as critical a function. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian: > > Tom Lane: > > > Per Jan's comment, there is no need to mess with the existing > > > datastructure. I'd think more of *copying* the dllist into some array > > > in shared memory. This is code that would only need to exist in the > > > Windows port. > > > > (I thought Jan was referring to the PGPROC struct) > > > > This just seems a little odd to me. I mean, they are going to be basically > > identical (they'll even use the same struct!). > > Seems you should use an array of "struct bkend" or "Backend" as storage > in shared memory. I think the problem with Dllist is that it is > dynamically allocated and I don't see how that could be done cleanly in > shared memory. I think that's why the array idea seems best. I wasn't suggesting that we somehow put the Dllist in shared memory. I was advocating completely replacing the BackendList dlllist with this array, under both Win32 and *nix case. (Is what I've been saying sound different in this light?) AFAICS, dumping the BackendList dlllist, and just completely replacing it with "PidCancel[]" in shared memory would be a pretty small hit, and give us the same code under *nix + Win32. > Looking up they key would have to be different for fork and fork/exec. Only if we choose to have this new PidCancel array, in addition to the existing BackendList, which looks to me to be entirely redundant. Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli wrote: > > Tom Lane: > > Per Jan's comment, there is no need to mess with the existing > > datastructure. I'd think more of *copying* the dllist into some array > > in shared memory. This is code that would only need to exist in the > > Windows port. > > (I thought Jan was referring to the PGPROC struct) > > This just seems a little odd to me. I mean, they are going to be basically > identical (they'll even use the same struct!). Seems you should use an array of "struct bkend" or "Backend" as storage in shared memory. I think the problem with Dllist is that it is dynamically allocated and I don't see how that could be done cleanly in shared memory. I think that's why the array idea seems best. > Also, let's get back to why we want this: to handle processCancelRequest in > the Win32 case. If this array is in Windows only, then we'll obviously need > two implementations of the processCancelRequest logic. > > Or I'm missing something... Looking up they key would have to be different for fork and fork/exec. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: > I think the simplest way to make this work is to use an array that's > 2*MaxBackend items long (corresponding to the max number of children the > postmaster will fork). Actually, now that I think about it, is even that big enough. There is a reason BackendList is a list. In pathological situations, the postmaster could be made to fork a much larger number than 2*MaxBackend simultaneous children, although only this many will be allowed to become backends. (I guess we could check the port->canAcceptConnections value, and not add the backend to the array when == CAC_TOOMANY). > There is no race condition possible for the ProcessCancelRequest case --- > the sub-postmaster that spawned an active backend must have found its > entry before it could have sent the cancel key to the client, so any > valid cancel request from a client must reference an already-existing > entry in the array. Make sense. Great. Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane: > Per Jan's comment, there is no need to mess with the existing > datastructure. I'd think more of *copying* the dllist into some array > in shared memory. This is code that would only need to exist in the > Windows port. (I thought Jan was referring to the PGPROC struct) This just seems a little odd to me. I mean, they are going to be basically identical (they'll even use the same struct!). Also, let's get back to why we want this: to handle processCancelRequest in the Win32 case. If this array is in Windows only, then we'll obviously need two implementations of the processCancelRequest logic. Or I'm missing something... Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian <[EMAIL PROTECTED]> writes: > Uh, why would you need more than maxbackends? Can't it be indexed by > slot number --- each backend has a slot? Maybe I am missing something. The postmaster has no way to know what slot number each backend will get. For that matter, a sub-postmaster doesn't know yet either. I think the simplest way to make this work is to use an array that's 2*MaxBackend items long (corresponding to the max number of children the postmaster will fork). Establish the convention that unused entries are zero. Then: 1. On forking a child, the postmaster scans the array for a free (zero) slot, and stashes the cancel key and PID there (in that order). 2. On receiving a child-termination report, the postmaster scans the array for the corresponding entry, and zeroes it out (PID first). (Obviously these algorithms could be improved if they turn out to be bottlenecks, but for the first cut KISS is applicable.) 3. To find or check a cencel key, a sub-postmaster scans the array looking for the desired PID (either its own, or the one it got from an incoming cancel request message). There is a potential race condition if a sub-postmaster scans the array before the postmaster has been able to store its PID there. I think it is sufficient for the sub-postmaster to sleep a few milliseconds and try again if it can't find its own PID in the array. There is no race condition possible for the ProcessCancelRequest case --- the sub-postmaster that spawned an active backend must have found its entry before it could have sent the cancel key to the client, so any valid cancel request from a client must reference an already-existing entry in the array. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> writes: > Just to be clear, this would involve turning the BackendList dlllist into an > array in shared memory, right? If so, a couple of questions: Per Jan's comment, there is no need to mess with the existing datastructure. I'd think more of *copying* the dllist into some array in shared memory. This is code that would only need to exist in the Windows port. > - the postmaster makes all calls referencing this list, with the exception > of processCancelRequest, correct? The postmaster writes the array (and really would have no need to read it). Sub-postmasters would need to read the array, either to check an incoming cancel request or to get the current-session key value to pass back to the client. I don't think that PostgresMain or any subsidiary routine would ever need to touch it. regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli wrote:
>
> I wrote:
> > Just to be clear, this would involve turning the BackendList dlllist into
> an
> > array in shared memory, right? If so, a couple of questions:
>
> Bruce Momjian wrote:
> > I assumed a much simpler solution. I thought we would just have:
> >
> > struct {
> > pid_t pid;
> > int cancel_key;
> > } PidCancel[maxbackend];
> >
> > in shared memory and we would just sequentially scan looking for a pid
> > match? Is that wrong?
>
> Isn't that basically "turning the BackendList dlllist into an array in
> shared memory"? And I don't think that an array length of maxbackend is
> enough.
Uh, why would you need more than maxbackends? Can't it be indexed by
slot number --- each backend has a slot? Maybe I am missing something.
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
I wrote:
> Just to be clear, this would involve turning the BackendList dlllist into
an
> array in shared memory, right? If so, a couple of questions:
Bruce Momjian wrote:
> I assumed a much simpler solution. I thought we would just have:
>
> struct {
> pid_t pid;
> int cancel_key;
> } PidCancel[maxbackend];
>
> in shared memory and we would just sequentially scan looking for a pid
> match? Is that wrong?
Isn't that basically "turning the BackendList dlllist into an array in
shared memory"? And I don't think that an array length of maxbackend is
enough.
Cheers,
Claudio
---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em
ailpolicy.html
---(end of broadcast)---
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli wrote:
>
> > I think they are saying put the cancel key inside the existing shared
> > memory segment.
>
> Ok. Thanks.
>
> > I don't know when we actually attach to the main shared
> > memory sigment in the child, but it would have to be sooner than when we
> > need the cancel key.
>
> Yes. Currently, it happens too late.
>
> I'll put my hand up to have a go at this fixing this (and getting
> processCancelRequest to work under Win32/EXEC_BACKEND at the same time), if
> no one else particularly cares to.
>
> Just to be clear, this would involve turning the BackendList dlllist into an
> array in shared memory, right? If so, a couple of questions:
> - what is a suitably large size for this array (2 * MaxBackends, ala
> canAcceptConnections?)
> - the postmaster makes all calls referencing this list, with the exception
> of processCancelRequest, correct? So, as Tom was alluding to, no locking is
> required (or desired!), and we'll just need to be careful not to introduce a
> race condition into processCancelRequest.
I assumed a much simpler solution. I thought we would just have:
struct {
pid_t pid;
int cancel_key;
} PidCancel[maxbackend];
in shared memory and we would just sequentially scan looking for a pid
match? Is that wrong?
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian wrote: Claudio Natoli wrote: Tom Lane writes: > Actually, on further reflection a separate array to store PIDs and cancel keys is probably a better idea. [snip] > I still think it's unnecessary to make a separate shmem segment for it, though. Why is that? Don't we need the backends to have access to it to do a cancel request? I think I've missed something here... I think they are saying put the cancel key inside the existing shared memory segment. I don't know when we actually attach to the main shared memory sigment in the child, but it would have to be sooner than when we need the cancel key. I said move it into the PGPROC structure. And keep the pid in both, the PGPROC structure and postmaster local memory. The backend attaches to the shared memory during AttachSharedMemoryAndSemaphores() ... where else? Jan -- #==# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #== [EMAIL PROTECTED] # ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
> I think they are saying put the cancel key inside the existing shared > memory segment. Ok. Thanks. > I don't know when we actually attach to the main shared > memory sigment in the child, but it would have to be sooner than when we > need the cancel key. Yes. Currently, it happens too late. I'll put my hand up to have a go at this fixing this (and getting processCancelRequest to work under Win32/EXEC_BACKEND at the same time), if no one else particularly cares to. Just to be clear, this would involve turning the BackendList dlllist into an array in shared memory, right? If so, a couple of questions: - what is a suitably large size for this array (2 * MaxBackends, ala canAcceptConnections?) - the postmaster makes all calls referencing this list, with the exception of processCancelRequest, correct? So, as Tom was alluding to, no locking is required (or desired!), and we'll just need to be careful not to introduce a race condition into processCancelRequest. Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli wrote: > > Tom Lane writes: > > Actually, on further reflection a separate array to store PIDs and > cancel keys is probably a better idea. > [snip] > > I still think it's unnecessary to make a separate shmem segment for it, > though. > > Why is that? Don't we need the backends to have access to it to do a cancel > request? I think I've missed something here... I think they are saying put the cancel key inside the existing shared memory segment. I don't know when we actually attach to the main shared memory sigment in the child, but it would have to be sooner than when we need the cancel key. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane writes: > Actually, on further reflection a separate array to store PIDs and cancel keys is probably a better idea. [snip] > I still think it's unnecessary to make a separate shmem segment for it, though. Why is that? Don't we need the backends to have access to it to do a cancel request? I think I've missed something here... Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: Jan Wieck <[EMAIL PROTECTED]> writes: It doesn't hurt to keep the locations and code as much in sync as possible. I think Tom's idea to move the information into the PGPROC entry is the winner and does not need any OS specific handling. Actually, on further reflection a separate array to store PIDs and cancel keys is probably a better idea. If we put this stuff in PGPROC then the postmaster will need to be able to obtain the ProcStructLock (or whatever it's called this week) in order to examine/modify that data structure. That gets us into the usual concerns about backend bugs locking up the postmaster, etc. But if it's a separate array then we can just have the rule that no one but the postmaster gets to write in it. I still think it's unnecessary to make a separate shmem segment for it, though. I'd like to avoid the additional shmem segment if possible. The postmaster can keep the stuff he needs in local memory. I did not mean to rip everything out of postmaster local memory, and that little bit of redundancy does not hurt. The pid's of processes aren't likely to change very often. Jan -- #==# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #== [EMAIL PROTECTED] # ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Jan Wieck <[EMAIL PROTECTED]> writes: > It doesn't hurt to keep the locations and code as much in sync as > possible. I think Tom's idea to move the information into the PGPROC > entry is the winner and does not need any OS specific handling. Actually, on further reflection a separate array to store PIDs and cancel keys is probably a better idea. If we put this stuff in PGPROC then the postmaster will need to be able to obtain the ProcStructLock (or whatever it's called this week) in order to examine/modify that data structure. That gets us into the usual concerns about backend bugs locking up the postmaster, etc. But if it's a separate array then we can just have the rule that no one but the postmaster gets to write in it. I still think it's unnecessary to make a separate shmem segment for it, though. regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian wrote: Tom Lane wrote: > Claudio Natoli wrote: >> The only things I've thought of so far are: >> a) sticking the PID/cancel key list in shared mem [yeech] >> b) rearranging the entire cancel handling to occur in the postmaster [double >> yeech] (a) seems like the lesser of the available evils (unless someone has a bright idea for a plan C). Bruce Momjian <[EMAIL PROTECTED]> writes: > I think we need to move in the direction of a separate fork/exec-only > shared memory segment that holds the pids and cancel keys for all the > backends. That doesn't seem worth the trouble. I'd be inclined to just stick the cancel keys in the PGPROC structures (I believe the PIDs are already in there). The original motivation for keeping them only in postmaster local memory was to keep backend A's cancel key away from the prying eyes of backend B, but is there really any security added? Anyone who can inspect PGPROC hardly needs to know the cancel key --- he can just issue a SIGINT (or worse) directly to the target backend. Agreed. I was going for a separate one just to be paranoid. This will only be done for exec(), so I don't see a problem for normal Unix use anyway. It doesn't hurt to keep the locations and code as much in sync as possible. I think Tom's idea to move the information into the PGPROC entry is the winner and does not need any OS specific handling. Jan -- #==# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #== [EMAIL PROTECTED] # ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: > > Claudio Natoli wrote: > >> The only things I've thought of so far are: > >> a) sticking the PID/cancel key list in shared mem [yeech] > >> b) rearranging the entire cancel handling to occur in the postmaster [double > >> yeech] > > (a) seems like the lesser of the available evils (unless someone has a > bright idea for a plan C). > > Bruce Momjian <[EMAIL PROTECTED]> writes: > > I think we need to move in the direction of a separate fork/exec-only > > shared memory segment that holds the pids and cancel keys for all the > > backends. > > That doesn't seem worth the trouble. I'd be inclined to just stick the > cancel keys in the PGPROC structures (I believe the PIDs are already in > there). The original motivation for keeping them only in postmaster > local memory was to keep backend A's cancel key away from the prying > eyes of backend B, but is there really any security added? Anyone who > can inspect PGPROC hardly needs to know the cancel key --- he can just > issue a SIGINT (or worse) directly to the target backend. Agreed. I was going for a separate one just to be paranoid. This will only be done for exec(), so I don't see a problem for normal Unix use anyway. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
> Claudio Natoli wrote: >> The only things I've thought of so far are: >> a) sticking the PID/cancel key list in shared mem [yeech] >> b) rearranging the entire cancel handling to occur in the postmaster [double >> yeech] (a) seems like the lesser of the available evils (unless someone has a bright idea for a plan C). Bruce Momjian <[EMAIL PROTECTED]> writes: > I think we need to move in the direction of a separate fork/exec-only > shared memory segment that holds the pids and cancel keys for all the > backends. That doesn't seem worth the trouble. I'd be inclined to just stick the cancel keys in the PGPROC structures (I believe the PIDs are already in there). The original motivation for keeping them only in postmaster local memory was to keep backend A's cancel key away from the prying eyes of backend B, but is there really any security added? Anyone who can inspect PGPROC hardly needs to know the cancel key --- he can just issue a SIGINT (or worse) directly to the target backend. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli wrote: > > > 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 ... > > Good question (the Win32/EXEC_BACKEND case is #def'd out to issue an > altogether unhelpful abort(), so I know it is there). > > The only things I've thought of so far are: > a) sticking the PID/cancel key list in shared mem [yeech] > b) rearranging the entire cancel handling to occur in the postmaster [double > yeech] As I remember, the only per-backend value to be passed is the cancel key, and seeing that this is going to be a problem for postmaster too, I think we need to move in the direction of a separate fork/exec-only shared memory segment that holds the pids and cancel keys for all the backends. I will update the Win32 TODO list to mention this issue in more detail. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: > 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). It's already been applied. Your push in the SubPostmaster direction was really useful; a lot cleaner, and fixed things that I wasn't even familiar enough with the code to know were broken. Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
> 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 ... Good question (the Win32/EXEC_BACKEND case is #def'd out to issue an altogether unhelpful abort(), so I know it is there). The only things I've thought of so far are: a) sticking the PID/cancel key list in shared mem [yeech] b) rearranging the entire cancel handling to occur in the postmaster [double yeech] Any better ideas? Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
> Whoever changed this: My fault. Thanks for the catch Jan. >proc_exit(status); Should be proc_exit(0). My apologies, Claudio PS. I take it there is no regression test for this. Would this be a difficult one to knock up? --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Tom Lane wrote:
> Jan Wieck <[EMAIL PROTECTED]> 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
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Jan Wieck <[EMAIL PROTECTED]> 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
---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
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 href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(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. # #== [EMAIL PROTECTED] # ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
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 http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
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 However, I have never seen that version, and Google doesn't show it either: http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&c2coff=1&group=comp.databases.postgresql.patches Anyway, Tom has looked at your newest patch and it looks good to him. These are all marked not for application so we will just wait for you to finalize it. --- Claudio Natoli wrote: > > [patch edited + resubmitted for review; not for committing] > > Hi Tom, > > figuring that, on balance, you are in fact going to prefer to take a > potential future hit in duplicated work (in passing context in the fork/exec > case) over moving the client auth code to PostgresMain, I've just gone ahead > and made a patch that implements your BackendFork ideas... > > Please let me know: > > * if the changes to the BackendFork / SubPostmasterMain logic are more or > less what you envisaged, and if you are content with them [Note: we can also > roll BackendInit back into BackendFork, making BackendFork (now BackendRun?) > pretty much exactly as it was before the fork/exec changes began] > > * if you are content with the above, whether or not you think I ought to do > the same for the SSDataBase logic. I'm hoping for a negative, as the #ifdef > logic is not as convoluted as that originally presented in BackendFork (ie. > to me, it looks like overkill in this case), but anticipating otherwise :-) > > Also: > * are you ok with the pgstat changes (I'm guessing yes, as you haven't > mentioned them, and since these changes are pretty similar to what you > suggested for BackendFork) > > Cheers, > Claudio > > > > > > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 6: Have you searched our list archives? > >http://archives.postgresql.org -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --- Claudio Natoli wrote: > > [patch edited + resubmitted for review; not for committing] > > Hi Tom, > > figuring that, on balance, you are in fact going to prefer to take a > potential future hit in duplicated work (in passing context in the fork/exec > case) over moving the client auth code to PostgresMain, I've just gone ahead > and made a patch that implements your BackendFork ideas... > > Please let me know: > > * if the changes to the BackendFork / SubPostmasterMain logic are more or > less what you envisaged, and if you are content with them [Note: we can also > roll BackendInit back into BackendFork, making BackendFork (now BackendRun?) > pretty much exactly as it was before the fork/exec changes began] > > * if you are content with the above, whether or not you think I ought to do > the same for the SSDataBase logic. I'm hoping for a negative, as the #ifdef > logic is not as convoluted as that originally presented in BackendFork (ie. > to me, it looks like overkill in this case), but anticipating otherwise :-) > > Also: > * are you ok with the pgstat changes (I'm guessing yes, as you haven't > mentioned them, and since these changes are pretty similar to what you > suggested for BackendFork) > > Cheers, > Claudio > > > > > > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 6: Have you searched our list archives? > >http://archives.postgresql.org -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
[minor corrections (some duplication in arg passing); still under discussion/review] Cheers, Claudio > -Original Message- > From: Claudio Natoli [mailto:[EMAIL PROTECTED] > Sent: Saturday, 27 December 2003 9:27 PM > To: 'Tom Lane' > Cc: '[EMAIL PROTECTED]' > Subject: Re: [PATCHES] fork/exec patch: pre-CreateProcess > finalization > > > > [patch edited + resubmitted for review; not for committing] > > Hi Tom, > > figuring that, on balance, you are in fact going to prefer to take a > potential future hit in duplicated work (in passing context > in the fork/exec > case) over moving the client auth code to PostgresMain, I've > just gone ahead > and made a patch that implements your BackendFork ideas... > > Please let me know: > > * if the changes to the BackendFork / SubPostmasterMain logic > are more or > less what you envisaged, and if you are content with them > [Note: we can also > roll BackendInit back into BackendFork, making BackendFork > (now BackendRun?) > pretty much exactly as it was before the fork/exec changes began] > > * if you are content with the above, whether or not you think > I ought to do > the same for the SSDataBase logic. I'm hoping for a negative, > as the #ifdef > logic is not as convoluted as that originally presented in > BackendFork (ie. > to me, it looks like overkill in this case), but anticipating > otherwise :-) > > Also: > * are you ok with the pgstat changes (I'm guessing yes, as you haven't > mentioned them, and since these changes are pretty similar to what you > suggested for BackendFork) > > Cheers, > Claudio > > > > > > > --- > Certain disclaimers and policies apply to all email sent from > Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.me > metrics.com/em > ailpolicy.html > > > --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html diff5c3.out Description: Binary data ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
> > 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. > [snip] > > > 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. Check out the last patch I sent. It implements your ideas for BackendFork'ing in the fork/exec case, and pretty much lets us revert the previous changes to postgres.c and BackendFork (which I agree is a good thing). Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
[patch edited + resubmitted for review; not for committing] Hi Tom, figuring that, on balance, you are in fact going to prefer to take a potential future hit in duplicated work (in passing context in the fork/exec case) over moving the client auth code to PostgresMain, I've just gone ahead and made a patch that implements your BackendFork ideas... Please let me know: * if the changes to the BackendFork / SubPostmasterMain logic are more or less what you envisaged, and if you are content with them [Note: we can also roll BackendInit back into BackendFork, making BackendFork (now BackendRun?) pretty much exactly as it was before the fork/exec changes began] * if you are content with the above, whether or not you think I ought to do the same for the SSDataBase logic. I'm hoping for a negative, as the #ifdef logic is not as convoluted as that originally presented in BackendFork (ie. to me, it looks like overkill in this case), but anticipating otherwise :-) Also: * are you ok with the pgstat changes (I'm guessing yes, as you haven't mentioned them, and since these changes are pretty similar to what you suggested for BackendFork) Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html diff5c2.out Description: Binary data ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
> > 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. Yes. But not all of it. Sure, PGDATA is a trivial example of something that's gotta get across one way or another. I take no argument with that. How about things like, for instance, canAcceptConnections(). To make this call successfully in a fork/exec'd child would take a bit of work. Of course, there is a work-around... pass the value into BackendFork (or, in the fork/exec case, on the command line). But, ISTM that, do this a few times, and you might as well have just had the postmaster format up the argument list. > 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. Hmm.. that's not the way I see it. > > 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. Ok. That, in the context of where it was written, instead of "can't" should have read "can no longer ... [in the absence of passing a great deal of additional context]". Mea culpa. Anyway, moving along... > 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. Ah. OK! Now I see where we are talking at cross-purposes. Forking after client auth was NOT what I was trying to suggest at all. Let's have a look at BackendFork. Here's what it needs to set up the arg list for PostgresMain, or otherwise uses: * PreAuthDelay * canAcceptConnections() * ExtraOptions * debugbuf * DataDir (in EXEC_BACKEND case) BackendInit (ie. client authentication) port->cmdline_options port->proto port->database_name port->user_name 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). 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 prefer the former option, as there is less duplication. Should anything be changed/added to the items required for BackendFork'ing, no changes would be required to the fork/exec code. This will not necessarily be true if we need to pass a bunch of (additional) context, as in the latter case. ISTM this would let us make BackendFork *very* simple, and pretty much identical in both the normal and fork/exec cases, for the cost of pushing the authenication step across to PostgresMain (non stand-alone case), which is what the fork/exec case does now anyway! What do you think? Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Thanks for your comments Tom. Tom Lane writes: > Claudio Natoli <[EMAIL PROTECTED]> 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 construc
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
I said: > 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. It occurs to me that you probably need some concrete suggestions about what to do. Here's one. There are really three separate sections in this code: the postmaster (which no longer does much except fork off a subprocess on receipt of a new connection), the "sub-postmaster" which does client authentication, and the backend. Historical note: formerly the sub-postmaster actions were done in the parent postmaster process, with a sort of poor man's threading logic to allow the postmaster to interleave multiple client authentication operations. We had to change that because various libraries like SSL, PAM, and Kerberos weren't amenable to pseudo-threading. It is worth maintaining a clean distinction between sub-postmaster and backend because when we launch a standalone backend, we don't want the sub-postmaster part of the code. The command line syntax accepted by PostgresMain() is largely designed for the convenience of the standalone backend case, with a couple of simple hacks for the under-postmaster case. 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. 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. In the Unix case we don't bother compiling SubPostmasterMain, we just fork and go straight to BackendFork as the code does now. In essence, this design makes SubPostmasterMain responsible for emulating Unix-style fork(), ie, fork without loss of static variable contents. It would need to reload as many variables as we needed. I think this is a cleaner approach since it creates a layer of code which is simply there in one case and not there in the other, rather than making arbitrary changes in the layers above and below depending on #ifdefs. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch: pre-CreateProcess finalization
Claudio Natoli <[EMAIL PROTECTED]> 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. 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. If you want to change them both, let's first see the reason why it's necessary. regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch: pgstat + BootstrapMain
Patch applied. Thanks. --- Claudio Natoli wrote: > > For application to HEAD, pending community review. > > Continued rearrangement to permit pgstat + BootstrapMain processes to be > fork/exec'd, in the same mode as the previous patch for backends. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 6: Have you searched our list archives? > >http://archives.postgresql.org -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch: pgstat + BootstrapMain
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --- Claudio Natoli wrote: > > For application to HEAD, pending community review. > > Continued rearrangement to permit pgstat + BootstrapMain processes to be > fork/exec'd, in the same mode as the previous patch for backends. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 6: Have you searched our list archives? > >http://archives.postgresql.org -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch
Patch applied. Thanks. I had a little trouble patching shmem.c but got it fixed. --- Claudio Natoli wrote: > > Resubmission, incorporating Neil Conway's comments and some minor > corrections. > > > -Original Message- > > From: Bruce Momjian [mailto:[EMAIL PROTECTED] > > Sent: Wednesday, 17 December 2003 11:12 AM > > To: Claudio Natoli > > Cc: '[EMAIL PROTECTED]' > > Subject: Re: [PATCHES] fork/exec patch > > > > > > > > Patch withdrawn. Author will resubmit new version. > > > > -- > > - > > > > Claudio Natoli wrote: > > > > > > This patch is the next step towards (re)allowing fork/exec. > > > > > > Bruce, I've cleaned up the parts we discussed, and, pending > > objections from > > > anyone else, it is ready for application to HEAD. > > > > > > Cheers, > > > Claudio > > > > > > --- > > > Certain disclaimers and policies apply to all email sent > > from Memetrics. > > > For the full text of these disclaimers and policies see > > > > > > > href="http://www.memetrics.com/emailpolicy.html";>http://www.me > > metrics.com/em > > > ailpolicy.html > > > > > > > > > > [ Attachment, skipping... ] > > > > > > > > ---(end of > > broadcast)--- > > > TIP 4: Don't 'kill -9' the postmaster > > > > -- > > Bruce Momjian| http://candle.pha.pa.us > > [EMAIL PROTECTED] | (610) 359-1001 > > + If your life is a hard drive, | 13 Roberts Road > > + Christ can be your backup.| Newtown Square, > > Pennsylvania 19073 > > > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --- Claudio Natoli wrote: > > Resubmission, incorporating Neil Conway's comments and some minor > corrections. > > > -Original Message- > > From: Bruce Momjian [mailto:[EMAIL PROTECTED] > > Sent: Wednesday, 17 December 2003 11:12 AM > > To: Claudio Natoli > > Cc: '[EMAIL PROTECTED]' > > Subject: Re: [PATCHES] fork/exec patch > > > > > > > > Patch withdrawn. Author will resubmit new version. > > > > -- > > - > > > > Claudio Natoli wrote: > > > > > > This patch is the next step towards (re)allowing fork/exec. > > > > > > Bruce, I've cleaned up the parts we discussed, and, pending > > objections from > > > anyone else, it is ready for application to HEAD. > > > > > > Cheers, > > > Claudio > > > > > > --- > > > Certain disclaimers and policies apply to all email sent > > from Memetrics. > > > For the full text of these disclaimers and policies see > > > > > > > href="http://www.memetrics.com/emailpolicy.html";>http://www.me > > metrics.com/em > > > ailpolicy.html > > > > > > > > > > [ Attachment, skipping... ] > > > > > > > > ---(end of > > broadcast)--- > > > TIP 4: Don't 'kill -9' the postmaster > > > > -- > > Bruce Momjian| http://candle.pha.pa.us > > [EMAIL PROTECTED] | (610) 359-1001 > > + If your life is a hard drive, | 13 Roberts Road > > + Christ can be your backup.| Newtown Square, > > Pennsylvania 19073 > > > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
Claudio Natoli wrote: > > [Thought I replied to this already] > > > I am now thinking we have to remove pgsql/data/pgsql_tmp > > unconditionally: > > [snip] > > The reason is that if they stop a postmaster that is > > fork/exec, install > > a non-exec postmaster, and restart, we should still clear out that > > directory. I guess what i am saying is that I don't want to tie the > > directory format to the exec() case of the binary. > > Could do. On the other hand, it is a directory for a small number (usually > zero) of tmp files. > > More pertitently, is *anyone* even going to use fork/exec? Whilst there is > no reason (yet) why someone couldn't, other than for development, why would > anyone want to? I've only really been seeing it as a stepping stone to > pushing the Win32 port out... Agreed. Forget my idea. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch
[Thought I replied to this already] > I am now thinking we have to remove pgsql/data/pgsql_tmp > unconditionally: > [snip] > The reason is that if they stop a postmaster that is > fork/exec, install > a non-exec postmaster, and restart, we should still clear out that > directory. I guess what i am saying is that I don't want to tie the > directory format to the exec() case of the binary. Could do. On the other hand, it is a directory for a small number (usually zero) of tmp files. More pertitently, is *anyone* even going to use fork/exec? Whilst there is no reason (yet) why someone couldn't, other than for development, why would anyone want to? I've only really been seeing it as a stepping stone to pushing the Win32 port out... Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch
Claudio Natoli wrote:
>
> Resubmission, incorporating Neil Conway's comments and some minor
> corrections.
I am now thinking we have to remove pgsql/data/pgsql_tmp
unconditionally:
***
*** 1217,1224
{
while ((db_de = readdir(db_dir)) != NULL)
{
! if (strcmp(db_de->d_name, ".") == 0 ||
! strcmp(db_de->d_name, "..") == 0)
continue;
snprintf(temp_path, sizeof(temp_path),
--- 1212,1223
{
while ((db_de = readdir(db_dir)) != NULL)
{
! if (strcmp(db_de->d_name, ".") == 0
! #ifndef EXEC_BACKEND
! /* no PG_TEMP_FILES_DIR in DataDir in non EXEC_BACKEND
case */
! || strcmp(db_de->d_name, "..") == 0
! #endif
! )
continue;
The reason is that if they stop a postmaster that is fork/exec, install
a non-exec postmaster, and restart, we should still clear out that
directory. I guess what i am saying is that I don't want to tie the
directory format to the exec() case of the binary.
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
Patch withdrawn. Author will resubmit new version. --- Claudio Natoli wrote: > > This patch is the next step towards (re)allowing fork/exec. > > Bruce, I've cleaned up the parts we discussed, and, pending objections from > anyone else, it is ready for application to HEAD. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
Claudio Natoli wrote: > > (circa line 335 of ipc/shmem.c:) > > [snip] > > Doesn't this function still acquire ShmemIndexLock? (i.e. why was this > comment changed?) > > AFAICS this is just whitespace differences. > > With the exception of that missing "break" (Bruce, I guess it goes without > saying, but could you please remove that line from the patch before > applying... and again "Thank you Neil"), these are stylistic/cosmetic and > effect the EXEC_BACKEND code only. > > Would a follow-up patch to correct these, along with the next step of the > fork/exec changes, be acceptable? Claudio, let's go for a new version of the patch so everyone can see that is being applied. Thanks. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] fork/exec patch
Neil Conway <[EMAIL PROTECTED]> writes: > My next question would have been to ask whether switching to a > spinlock here will be a performance problem. In looking at the code, > it seems we only hold the ShmemIndexLock for a long time (hundreds of > instructions & multiple system calls) while bootstrapping the shmem > index hash table itself. Otherwise, the lock is acquired and released > quickly, and even then it is only done during backend initialization, > so there shouldn't be much contention for it. Is this analysis > correct? Yes, at least that was the theory I was working from when I suggested Claudio do it this way ... regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] fork/exec patch
Hi Neil, > > No. This isn't necessary (and what action would it take in any > > case?). > > It should write a log message. I'm not sure why this /shouldn't/ be > done: if an operation fails, we should log that failure. This is > standard practise. Fair enough. Will do (although, I'd point out that there are more than a few places in the existing code where unlink is called without error checking, but that isn't justification for not doing it here). > >> Doesn't this function still acquire ShmemIndexLock? (i.e. why was > >> this comment changed?) > > > > AFAICS this is just whitespace differences. > > Read it again. Here's the whole diff hunk: > [snip] > The code acquires ShmemIndexLock, performs some computations, and then > notes that "ShmemLock is held" in a comment before returning. ISTM > that is plainly wrong. [I did, again. Twice just now. And still didn't see what you were trying to point out. And then...] Ah. Yep. Typo. Will fix (I was experimenting with using the ShmemLock, instead of creating a new ShmemIndexLock, and forgot to change the comment back). > > [ the only other suggested changes are ] stylistic/cosmetic and > > effect the EXEC_BACKEND code only. > > Yeah, my apologies for nitpicking... Not at all. I want this done well. Thank you for any input. Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch
Claudio Natoli <[EMAIL PROTECTED]> writes:
>> You should probably check the return value of unlink().
>
> No. This isn't necessary (and what action would it take in any
> case?).
It should write a log message. I'm not sure why this /shouldn't/ be
done: if an operation fails, we should log that failure. This is
standard practise.
>> Doesn't this function still acquire ShmemIndexLock? (i.e. why was
>> this comment changed?)
>
> AFAICS this is just whitespace differences.
Read it again. Here's the whole diff hunk:
*** 320,340
strncpy(item.key, name, SHMEM_INDEX_KEYSIZE);
item.location = BAD_LOCATION;
! LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);
if (!ShmemIndex)
{
/*
* If the shmem index doesn't exist, we are bootstrapping: we must
* be trying to init the shmem index itself.
*
!* Notice that the ShmemIndexLock is held until the shmem index has
* been completely initialized.
*/
Assert(strcmp(name, "ShmemIndex") == 0);
Assert(ShmemBootstrap);
*foundPtr = FALSE;
! return ShmemAlloc(size);
}
/* look it up in the shmem index */
--- 335,367
strncpy(item.key, name, SHMEM_INDEX_KEYSIZE);
item.location = BAD_LOCATION;
! SpinLockAcquire(ShmemIndexLock);
if (!ShmemIndex)
{
+ if (IsUnderPostmaster)
+ {
+ /* Must be initializing a (non-standalone) backend */
+ Assert(strcmp(name, "ShmemIndex") == 0);
+ Assert(ShmemBootstrap);
+ Assert(ShmemIndexAlloc);
+ *foundPtr = TRUE;
+ }
+ else
+ {
/*
* If the shmem index doesn't exist, we are bootstrapping: we
must
* be trying to init the shmem index itself.
*
!* Notice that the ShmemLock is held until the shmem index has
* been completely initialized.
*/
Assert(strcmp(name, "ShmemIndex") == 0);
Assert(ShmemBootstrap);
*foundPtr = FALSE;
! ShmemIndexAlloc = ShmemAlloc(size);
! }
! return ShmemIndexAlloc;
}
The code acquires ShmemIndexLock, performs some computations, and then
notes that "ShmemLock is held" in a comment before returning. ISTM
that is plainly wrong.
> [ the only other suggested changes are ] stylistic/cosmetic and
> effect the EXEC_BACKEND code only.
Yeah, my apologies for nitpicking...
-Neil
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch
Tom Lane <[EMAIL PROTECTED]> writes: > That one I can answer --- it's a bootstrapping issue: we can't use > LWLocks until we have a PGProc (*MyProc), and we can't set that up > without looking in the ShmemIndex for the related data structures. > So ShmemIndex needs to use a more primitive lock type. Fair enough. My next question would have been to ask whether switching to a spinlock here will be a performance problem. In looking at the code, it seems we only hold the ShmemIndexLock for a long time (hundreds of instructions & multiple system calls) while bootstrapping the shmem index hash table itself. Otherwise, the lock is acquired and released quickly, and even then it is only done during backend initialization, so there shouldn't be much contention for it. Is this analysis correct? > I don't want to make these things public, because we don't really > want any other modules accessing them. I agree, both ways are non-optimal for different reasons. Can anyone else see a better way to do this? -Neil ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch
[Thanks to Tom + Bruce]
For the remaining comments...
> The number of "FIXME" or "This is ugly" comments doesn't ease my mind
> about this patch :-) How many of these issues do you plan to resolve?
All of them, as we go along. Treat this as a first step.
> - break;
>
> case 'd': /* debug level
>*/
> {
>
>Why was this change made? If you actually intend to fall through the
> case here, please make it clear via a comment.
I can't believe that got through! It is an editing mistake, pure and simple.
Thank you for catching it. [bashes head against wall]
> + #define get_tmp_backend_var_file_name(buf,id) \
> + Assert(DataDir);\
> + sprintf((buf), \
> + "%s/%s/%s.backend_var.%d", \
> + DataDir,\
> + PG_TEMP_FILES_DIR, \
> + PG_TEMP_FILE_PREFIX,\
> + (id))
>
> This macro needs to be enclosed in a do {} while (0) block. Also,
> what does the "var" in the name of this macro refer to?
"var" refers to "variable". Will add a do while block in a following patch.
> + #define get_tmp_backend_var_dir(buf)\
> + sprintf((buf),"%s/%s",DataDir,PG_TEMP_FILES_DIR)
>
> This seems a little pointless, IMHO.
True.
> + static void
> [snip]
> ereport(FATAL) seems too strong, doesn't it?
Possibly.
> + read_var(LWLockArray,fp);
> + read_var(ProcStructLock,fp);
> + read_var(pgStatSock,fp);
> +
> + /* Release file */
> + FreeFile(fp);
> + unlink(filename);
>
> You should probably check the return value of unlink().
No. This isn't necessary (and what action would it take in any case?). If it
doesn't unlink the file, tough. A backend crash could also leave these files
on the disk. Like the other pgtmp files they'll get cleaned up on postmaster
restart.
> (circa line 335 of ipc/shmem.c:)
> [snip]
> Doesn't this function still acquire ShmemIndexLock? (i.e. why was this
comment changed?)
AFAICS this is just whitespace differences.
With the exception of that missing "break" (Bruce, I guess it goes without
saying, but could you please remove that line from the patch before
applying... and again "Thank you Neil"), these are stylistic/cosmetic and
effect the EXEC_BACKEND code only.
Would a follow-up patch to correct these, along with the next step of the
fork/exec changes, be acceptable?
Cheers,
Claudio
---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em
ailpolicy.html
---(end of broadcast)---
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] fork/exec patch
Neil Conway <[EMAIL PROTECTED]> writes: > Why did you change ShmemIndexLock from an LWLock to a spinlock? That one I can answer --- it's a bootstrapping issue: we can't use LWLocks until we have a PGProc (*MyProc), and we can't set that up without looking in the ShmemIndex for the related data structures. So ShmemIndex needs to use a more primitive lock type. This is actually the way the code used to do it; I changed the lock type when the opportunity presented itself, but if we're going to support fork/exec again then we have to go back to how it was done before. Your other comments seem pretty germane to me, except for > I wonder whether it is cleaner to make these properly public, rather > than using the NON_EXEC_STATIC #ifdef... (I'm not necessarily saying > it is, I'm just tossing it out there). I don't want to make these things public, because we don't really want any other modules accessing them. NON_EXEC_STATIC is pretty ugly, but it does guarantee that we will soon find out about any unintended cross-module references, because they won't compile on Unix systems. If you've got an idea about a cleaner way to do it, I'm all ears ... regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch
Neil Conway wrote: > + /* > + * The following need to be available to the read/write_backend_variables > + * functions > + */ > + extern XLogRecPtr RedoRecPtr; > + extern XLogwrtResult LogwrtResult; > + extern slock_t *ShmemLock; > + extern slock_t *ShmemIndexLock; > + extern void *ShmemIndexAlloc; > + typedef struct LWLock LWLock; > + extern LWLock *LWLockArray; > + extern slock_t *ProcStructLock; > + extern int pgStatSock; > > I wonder whether it is cleaner to make these properly public, rather > than using the NON_EXEC_STATIC #ifdef... (I'm not necessarily saying > it is, I'm just tossing it out there). This was my idea. Rather than making statics as globals, they are global only for exec() builds. This seemed safest. They need to be extern for exec() because we need to reference them in a function that writes all the postmaster globals to a file. We could have had a write function in every C file that needed it and call the write function from postmaster.c, but that seems like too much bloat. If we make them extern in all builds we lose the safety of a static. Your other comments are good and I will wait for Claudio to respond. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch
Claudio Natoli <[EMAIL PROTECTED]> writes:
> This patch is the next step towards (re)allowing fork/exec.
I've included a few comments on the patch below. Unfortunately I only
had time to briefly look over the code...
Why did you change ShmemIndexLock from an LWLock to a spinlock?
The number of "FIXME" or "This is ugly" comments doesn't ease my mind
about this patch :-) How many of these issues do you plan to resolve?
Index: src/backend/tcop/postgres.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
retrieving revision 1.379
diff -c -w -r1.379 postgres.c
*** src/backend/tcop/postgres.c 1 Dec 2003 22:15:37 - 1.379
--- src/backend/tcop/postgres.c 13 Dec 2003 06:18:44 -
***
*** 2133,2139
case 'D': /* PGDATA directory */
if (secure)
potential_DataDir = optarg;
- break;
case 'd': /* debug level */
{
Why was this change made? If you actually intend to fall through the
case here, please make it clear via a comment.
+ /*
+ * The following need to be available to the read/write_backend_variables
+ * functions
+ */
+ extern XLogRecPtr RedoRecPtr;
+ extern XLogwrtResult LogwrtResult;
+ extern slock_t *ShmemLock;
+ extern slock_t *ShmemIndexLock;
+ extern void *ShmemIndexAlloc;
+ typedef struct LWLock LWLock;
+ extern LWLock *LWLockArray;
+ extern slock_t *ProcStructLock;
+ extern intpgStatSock;
I wonder whether it is cleaner to make these properly public, rather
than using the NON_EXEC_STATIC #ifdef... (I'm not necessarily saying
it is, I'm just tossing it out there).
+ #define get_tmp_backend_var_file_name(buf,id) \
+ Assert(DataDir);\
+ sprintf((buf), \
+ "%s/%s/%s.backend_var.%d", \
+ DataDir,\
+ PG_TEMP_FILES_DIR, \
+ PG_TEMP_FILE_PREFIX,\
+ (id))
This macro needs to be enclosed in a do {} while (0) block. Also,
what does the "var" in the name of this macro refer to?
+ #define get_tmp_backend_var_dir(buf) \
+ sprintf((buf),"%s/%s",DataDir,PG_TEMP_FILES_DIR)
This seems a little pointless, IMHO.
+ static void
+ write_backend_variables(pid_t pid, Port *port)
+ {
+ charfilename[MAXPGPATH];
+ FILE*fp;
+ get_tmp_backend_var_file_name(filename,pid);
+
+ /* Open file */
+ fp = AllocateFile(filename, PG_BINARY_W);
+ if (!fp)
+ {
+ /* As per OpenTemporaryFile... */
+ char dirname[MAXPGPATH];
+ get_tmp_backend_var_dir(dirname);
+ mkdir(dirname, S_IRWXU);
+
+ fp = AllocateFile(filename, PG_BINARY_W);
+ if (!fp)
+ {
+ ereport(FATAL,
+ (errcode_for_file_access(),
+ errmsg("could not write to file \"%s\": %m",
filename)));
+ return;
+ }
+ }
ereport(FATAL) seems too strong, doesn't it?
+ read_var(LWLockArray,fp);
+ read_var(ProcStructLock,fp);
+ read_var(pgStatSock,fp);
+
+ /* Release file */
+ FreeFile(fp);
+ unlink(filename);
You should probably check the return value of unlink().
(circa line 335 of ipc/shmem.c:)
/*
* If the shmem index doesn't exist, we are bootstrapping: we
must
* be trying to init the shmem index itself.
*
!* Notice that the ShmemLock is held until the shmem index has
* been completely initialized.
*/
Doesn't this function still acquire ShmemIndexLock? (i.e. why was this
comment changed?)
-Neil
---(end of broadcast)---
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] fork/exec patch
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --- Claudio Natoli wrote: > > This patch is the next step towards (re)allowing fork/exec. > > Bruce, I've cleaned up the parts we discussed, and, pending objections from > anyone else, it is ready for application to HEAD. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch
> > > Why not use an anonymous pipe to send data from the parent to the > > > child process? > > > > Doesn't that require the postmaster to stay around to feed that > > information into the pipe or can the postmaster just shove the data > > and continue on, and how do the old pipes get cleaned up? > > I think that one can just output the data and close that end > of the pipe. > But i've not looked at win32 the last 5 years or so, I could be wrong. For anonymous pipes, large writes will block (pipes created with CreatePipe()). For named pipes (which can be given unique names - see CreateNamedPipe()), you can use Overlapped IO (MS speak for async IO), and then forget about it. Or rather, register a callback that will do a CloseHandle() on the pipe, so you don't leak handles. (Of course, the child process has to do CloseHandle() on the other end of the pipe). A way to do this using named pipes would be: a) Have the postmaster listen on a named pipe always (along with general connections). b) Have the clients use CallNamedPipe() to hit the postmasters known pipe name (the actual pipe name can be passed on the commandline). The postmaster then sends everything down the pipe. c) The postmaster only closes the pipe when it shuts down. The same pipe endpoint is reused all the time. > Does windows have a temp filesystem where the temp files are > not actually > written out on disk? It's still ugly but better then hitting > a disk all > the time. You can specify the FILE_ATTRIBUTE_TEMPORARY parameter to CreateFile(). This does not *guarantee* it will not go to disk, but it allows the system to store it in RAM. Small files will never hit disk. Large ones will when the memory manager figures it needs the space for something else. > > Also has to work on Unix too for testing. > > Everything can not work in unix, CreateProcess() and fork() > are different. > However, the pipe solution can be mimiced in unix, but it > will not be the > same code since the api's are different. So that does not give much. If you want to use any of the ways that "windows were made to use", they probably won't be compatible with Unix. Might be better off starting with something simple and once everything else works move to something more Windows specific (which can then be tested isolated from all the other changes). FWIW, the most common way to do this on windows is the "create anonymous memory mapped region, duplicate with INHERITABLE flag, and use it as shared memory". You then call DuplicateHandle() specifying read-only access to make sure the child process cannot write in the parent process' memory. If you want to use unique memory regions for each process (everytime you fork), do: CreateFileMapping(ALL_ACCESS, ANONYMOUS) MapViewOfFile() --> write all data to mapped region UnmapViewOfFile() DuplicateHandle(DUPLICATE_CLOSE_SOURCE, READ_ACCESS) --> exec and specify handle Child process does MapViewOfFile() to read. When done, UnmapViewOfFile() and CloseHandle(). On that CloseHandle(), the memory will be freed. If you want shared memory, just don't specify DUPLICATE_CLOSE_SOURCE, instead have the postmaster shut it down. Another option is to used named shared memory (specifying a name and security attribute, still mapping the pagefile so you don't need an actual file), in which case the child processes just use OpenFileMapping() (replaces DuplicateHandle()). DuplicateHandle() is the better way unelss you need to access it from a non-child process, though. //Magnus ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Agreed, added to the Win32 status page: > > * remove per-backend parameter file and move into shared memory > > [itch] I'm not sure that's an answer either; see my comments about how > the postmaster shouldn't depend on the contents of shared memory being > valid. > > We could get away with the postmaster having a write-only relationship > to shared memory (put value of variable X into predetermined location > Y), but I don't think that helps. It doesn't work for variable-size > values --- we certainly don't want the postmaster dependent on memory > allocation structures being valid within shared memory --- and what > about locks? Do you want the postmaster writing shared values without > taking a lock, or relying on shared-memory lock structures to be valid > enough to not lock it up or crash it? My answer to either of those is > "no way, Jose" ... > > Writing temp files may actually be a cleaner solution than writing > shared memory, once we take these considerations into account. My gripe > about race conditions was "I want to see how you solve this", and wasn't > intended to mean "I don't think that is soluble". Read my idea that shared memory for signals might be required, and a separate shared memory segment might be used for parameter passing too. I added a question mark to the win32 TODO item, so we can keep as an open item. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > Agreed, added to the Win32 status page: > * remove per-backend parameter file and move into shared memory [itch] I'm not sure that's an answer either; see my comments about how the postmaster shouldn't depend on the contents of shared memory being valid. We could get away with the postmaster having a write-only relationship to shared memory (put value of variable X into predetermined location Y), but I don't think that helps. It doesn't work for variable-size values --- we certainly don't want the postmaster dependent on memory allocation structures being valid within shared memory --- and what about locks? Do you want the postmaster writing shared values without taking a lock, or relying on shared-memory lock structures to be valid enough to not lock it up or crash it? My answer to either of those is "no way, Jose" ... Writing temp files may actually be a cleaner solution than writing shared memory, once we take these considerations into account. My gripe about race conditions was "I want to see how you solve this", and wasn't intended to mean "I don't think that is soluble". regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] fork/exec patch
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > I don't think we ever discussed it, but it seemed logical and a minimal > > change to the code. We already have a GUC write of non-default values > > for exec and no one had issues with that. > > You can hardly claim that "no one had issues with that". I complained > about it and I think other people did too. It's a messy, ugly approach; > moreover we have no field experience that says it's reliable. > > It may be the least messy, ugly approach available, but I concur with > Neil's wish to at least look for other answers. Absolutely. I am not happy with the GUC file either, but can't see a better way right now. I have already documented your concern about the GUC race condition issue on the Win32 status page so we will not forget about it. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch
Claudio Natoli wrote: > > For example, couldn't we write this data into a particular location in > > shared memory, and then pass that location to the child? That is still > > ugly, slow, and prone to failure (shmem being statically sized), but > > ISTM that the proposed implementation already possesses those > > attributes :-) > > I agree that this is a better implementation. > > Bruce, do we implement this now, or just hold it as something to revisit > down the track? I'm all for leaving it as is. > > Moreover, in general, how do we handle things like this? IMHO, I'd rather > live with a few kludges (that don't impact the *nix code) until the Windows > port is actually a reality, and then reiterate (having the discussions as we > go along, however, is necessary). Perhaps adding a TO_REVISIT section to > your Win32 Status Report page? > > Or do people have strong leanings towards "fix as you go along"? Just feels > like that way could see us getting bogged down making things "perfect" > instead of advancing the port... Let's get it working first. I have added an item to the Win32 status page so we will not forget it. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] fork/exec patch
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > On Sun, Dec 14, 2003 at 06:53:22PM -0500, Tom Lane wrote: > >> You can hardly claim that "no one had issues with that". > > > Don't the FSM and the system catalog cache use a similar mechanism? > > FSM uses a backing file to hold information over a database shutdown > (write once during shutdown, read once during startup). That's a little > different from once per backend fork. Also, there are no race > conditions to worry about, and finally the system does not *require* the > backing file to be either present or correct. > > The catalog cache uses a file that typically gets updated once per > VACUUM. Again, the file does not have to be present, nor correct. > There are mechanisms in place to deal with the cases (including race > conditions) where it's broken or obsolete. > > I have not looked at the proposed fork/exec code in any detail, but > IIUC it will be *necessary* that the intermediate file be present, and > correct. I think a minimum requirement for accepting this solution is a > sketch of how race conditions will be dealt with (ie, postmaster changes > its own value of some variable immediately after making the temp file). > I don't necessarily say that the first-cut patch has to get it right, > but we'd better understand how we will get to where it is right. Agreed, added to the Win32 status page: * remove per-backend parameter file and move into shared memory -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
Alvaro Herrera <[EMAIL PROTECTED]> writes: > On Sun, Dec 14, 2003 at 06:53:22PM -0500, Tom Lane wrote: >> You can hardly claim that "no one had issues with that". > Don't the FSM and the system catalog cache use a similar mechanism? FSM uses a backing file to hold information over a database shutdown (write once during shutdown, read once during startup). That's a little different from once per backend fork. Also, there are no race conditions to worry about, and finally the system does not *require* the backing file to be either present or correct. The catalog cache uses a file that typically gets updated once per VACUUM. Again, the file does not have to be present, nor correct. There are mechanisms in place to deal with the cases (including race conditions) where it's broken or obsolete. I have not looked at the proposed fork/exec code in any detail, but IIUC it will be *necessary* that the intermediate file be present, and correct. I think a minimum requirement for accepting this solution is a sketch of how race conditions will be dealt with (ie, postmaster changes its own value of some variable immediately after making the temp file). I don't necessarily say that the first-cut patch has to get it right, but we'd better understand how we will get to where it is right. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
On Sun, Dec 14, 2003 at 06:53:22PM -0500, Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > I don't think we ever discussed it, but it seemed logical and a minimal > > change to the code. We already have a GUC write of non-default values > > for exec and no one had issues with that. > > You can hardly claim that "no one had issues with that". I complained > about it and I think other people did too. It's a messy, ugly approach; > moreover we have no field experience that says it's reliable. Don't the FSM and the system catalog cache use a similar mechanism? -- Alvaro Herrera () "Limítate a mirar... y algun día veras" ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > I don't think we ever discussed it, but it seemed logical and a minimal > change to the code. We already have a GUC write of non-default values > for exec and no one had issues with that. You can hardly claim that "no one had issues with that". I complained about it and I think other people did too. It's a messy, ugly approach; moreover we have no field experience that says it's reliable. It may be the least messy, ugly approach available, but I concur with Neil's wish to at least look for other answers. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] fork/exec patch
On Mon, 15 Dec 2003, Claudio Natoli wrote: > Moreover, in general, how do we handle things like this? IMHO, I'd rather > live with a few kludges (that don't impact the *nix code) until the Windows > port is actually a reality As long as it does not hurt the unix code it's not a big problem as I see it. The usual open source solution is that since no one else writes the code, you can do it the way you think works the best. To change this in the future does not mean that everything else has to be rewritten which is good. It does also not mean that one can not discuss the implementation. A fair amount of discussion is always good. -- /Dennis ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch
Hi all, Dennis Bjorklund wrote: > > Also has to work on Unix too for testing. > > Everything can not work in unix, CreateProcess() and fork() > are different. True (but CreateProcess and "fork followed by exec" are pretty close). I think what Bruce is implying is that, ideally, we'd like to keep things as close as possible between Unix fork/exec and Windows code bases, so that: * it remains possible to advance the Windows port under a *nix dev environment and * should (when!) issues arise in the Windows implementation, it will be easier to identify and debug Neil Conway wrote: > For example, couldn't we write this data into a particular location in > shared memory, and then pass that location to the child? That is still > ugly, slow, and prone to failure (shmem being statically sized), but > ISTM that the proposed implementation already possesses those > attributes :-) I agree that this is a better implementation. Bruce, do we implement this now, or just hold it as something to revisit down the track? I'm all for leaving it as is. Moreover, in general, how do we handle things like this? IMHO, I'd rather live with a few kludges (that don't impact the *nix code) until the Windows port is actually a reality, and then reiterate (having the discussions as we go along, however, is necessary). Perhaps adding a TO_REVISIT section to your Win32 Status Report page? Or do people have strong leanings towards "fix as you go along"? Just feels like that way could see us getting bogged down making things "perfect" instead of advancing the port... Comments? Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em ailpolicy.html ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch
On Sun, 14 Dec 2003, Bruce Momjian wrote: > > Why not use an anonymous pipe to send data from the parent to the child > > process? > > Doesn't that require the postmaster to stay around to feed that > information into the pipe or can the postmaster just shove the data and > continue on, and how do the old pipes get cleaned up? I think that one can just output the data and close that end of the pipe. But i've not looked at win32 the last 5 years or so, I could be wrong. > Seems messy. Maybe, but to me the solution where you write to files are much more ugly. If one does not like pipes, there are other ipc mechanisms that does not involve creating, reading and deleting a file on each connect. Does windows have a temp filesystem where the temp files are not actually written out on disk? It's still ugly but better then hitting a disk all the time. > Also has to work on Unix too for testing. Everything can not work in unix, CreateProcess() and fork() are different. However, the pipe solution can be mimiced in unix, but it will not be the same code since the api's are different. So that does not give much. -- /Dennis ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] fork/exec patch
Dennis Bjorklund wrote: > On Sun, 14 Dec 2003, Bruce Momjian wrote: > > > change to the code. We already have a GUC write of non-default values > > for exec and no one had issues with that. Of course, this one is > > per-backend. > > > > Yea, we could use shared memory for this too, but I don't see a problem > > with using the file system. > > Why not use an anonymous pipe to send data from the parent to the child > process? That is a common way to handle this problem in win32 (and in unix > by the way). The parent sets up the pipe and the child process inherits > the handle, and after that the child and parent can excange information in > private. Doesn't that require the postmaster to stay around to feed that information into the pipe or can the postmaster just shove the data and continue on, and how do the old pipes get cleaned up? Seems messy. Also has to work on Unix too for testing. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch
On Sun, 14 Dec 2003, Bruce Momjian wrote: > change to the code. We already have a GUC write of non-default values > for exec and no one had issues with that. Of course, this one is > per-backend. > > Yea, we could use shared memory for this too, but I don't see a problem > with using the file system. Why not use an anonymous pipe to send data from the parent to the child process? That is a common way to handle this problem in win32 (and in unix by the way). The parent sets up the pipe and the child process inherits the handle, and after that the child and parent can excange information in private. -- /Dennis ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fork/exec patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > I don't think we ever discussed it, but it seemed logical and a minimal > change to the code. We already have a GUC write of non-default values > for exec and no one had issues with that. For the record, I think that is ugly as well :-) Anyway, I'm not necessarily arguing that using shmem is the right way to go here -- that was merely an off-the-cuff suggestion. I'm just saying that whatever solution we end up with, ISTM we can do better than writing out + reading in a file for /every/ new connection. -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] fork/exec patch
Neil Conway wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Let me add that Claudio is doing a fantastic job on this. The > > changes are minimal and clean. I think the writing of a per-backend > > temp file has allowed this patch to be smaller than it might have > > been. > > Did we REALLY conclude that the best way to work around the lack of > fork() on Win32 is by writing variables out to disk and reading them > back in? Frankly, that seems like an enormous kludge. > > For example, couldn't we write this data into a particular location in > shared memory, and then pass that location to the child? That is still > ugly, slow, and prone to failure (shmem being statically sized), but > ISTM that the proposed implementation already possesses those > attributes :-) I don't think we ever discussed it, but it seemed logical and a minimal change to the code. We already have a GUC write of non-default values for exec and no one had issues with that. Of course, this one is per-backend. Yea, we could use shared memory for this too, but I don't see a problem with using the file system. Older releases of PostgreSQL read from postgresql.conf or pg_hba.conf or other files for every connection so I don't see that using the file system is going to be that slow. Of course, we removed those file reads because they were slow, but they were also much larger and more complex in requiring parsing and stuff, while this is just a list of binary values. We also have a GUC dump file that is read by exec too. The downside of shared memory is that you would need the postmaster to write into shared memory and you have to allocate enough shared memory for all backends, but clearly it could be done. You could just pass the backend slot number to the child and the child could read from the offset. Not sure how to cleanly do the GUC dump file because it is of variable length depending on the number of GUC variables changed. I guess the big question is whether it is worth doing in shared memory. We also would have to pass the shared memory address to the child, and make sure the child knew the proper offset in shared memory to find its values. Of course, stuff that is variable length would be a problem in shared memory, and we have some of those for the GUC defaults. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] fork/exec patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > Let me add that Claudio is doing a fantastic job on this. The > changes are minimal and clean. I think the writing of a per-backend > temp file has allowed this patch to be smaller than it might have > been. Did we REALLY conclude that the best way to work around the lack of fork() on Win32 is by writing variables out to disk and reading them back in? Frankly, that seems like an enormous kludge. For example, couldn't we write this data into a particular location in shared memory, and then pass that location to the child? That is still ugly, slow, and prone to failure (shmem being statically sized), but ISTM that the proposed implementation already possesses those attributes :-) (/me goes off to re-read the archives on this issue...) -Neil ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] fork/exec patch
Let me provide a summary of this patch because I reviewed the first version. The patch basically is a slight rearrangement of the code to allow fork/exec on Unix, with the ultimate goal of doing CreateProcess on Win32. The changes are: o Write out postmaster global variables and per-backend variables to be read by the exec'ed backend o Mark some static variables as global when exec is used so then can be dumped from postmaster.c, marked NON_EXEC_STATIC o Remove value passing with -p now that we have per-backend file o Move some pointer storage out of shared memory for easier dumping. o Modified pgsql_temp directory cleanup to handle per-database directories and the backend exec directory under datadir. Let me add that Claudio is doing a fantastic job on this. The changes are minimal and clean. I think the writing of a per-backend temp file has allowed this patch to be smaller than it might have been. --- Claudio Natoli wrote: > > This patch is the next step towards (re)allowing fork/exec. > > Bruce, I've cleaned up the parts we discussed, and, pending objections from > anyone else, it is ready for application to HEAD. > > Cheers, > Claudio > > --- > Certain disclaimers and policies apply to all email sent from Memetrics. > For the full text of these disclaimers and policies see > href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em > ailpolicy.html > > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
