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] Double Backslash example patch
Peter Eisentraut wrote: > David Fetter wrote: > > Please find enclosed a patch exemplifying typical use of the ARE > > Class-Shorthand Escapes??. I believe it will help intrepid regex > > users. :) > > If you want to explain something, it's normally better to create an > example just for that point, instead of piggybacking it onto another > example. Else you just make it harder for people to recognize the > relevant information. > > For instance, in the case you want to patch the example aims to show how > patterns are not anchored, as opposed to the pattern for the LIKE > operator. What does the escape syntax of the pattern have to do with > that? He is reminding folks about double-backslashes in a relivant place --- makes sense to me. -- 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] Double Backslash example 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. --- David Fetter wrote: > Kind people, > > Please find enclosed a patch exemplifying typical use of the ARE > Class-Shorthand Escapes®. I believe it will help intrepid regex > users. :) > > Cheers, > D > -- > David Fetter [EMAIL PROTECTED] http://fetter.org/ > phone: +1 510 893 6100cell: +1 415 235 3778 [ 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 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] YA Doc patch
Peter Eisentraut wrote: > David Fetter wrote: > > I hope this one actually does what Tom said. It appears to work :) > > Probably, but that does not seem to be the right place in the > documentation for this. The location you propose explains EXTRACT, and > we should leave it at that. His patch puts the new test into the 'epoch' func.sgml section, not the EXTRACT section. It just so happens there are EXTRACT examples using epoch. -- 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] YA Doc 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. --- David Fetter wrote: > Kind people, > > I hope this one actually does what Tom said. It appears to work :) > > Cheers, > D > -- > David Fetter [EMAIL PROTECTED] http://fetter.org/ > phone: +1 510 893 6100cell: +1 415 235 3778 [ 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 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Unix timestamp -> timestamp, per Tom Lane :)
Peter indicated it was not valid docbook and wanted only the relivant parts, but it looked OK to me. Peter? 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. --- David Fetter wrote: > Folks, > > 'nother one. Thanks very much, Tom! :) > > Cheers, > D > -- > David Fetter [EMAIL PROTECTED] http://fetter.org/ > phone: +1 510 893 6100cell: +1 415 235 3778 [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 5: Have you checked our extensive FAQ? > >http://www.postgresql.org/docs/faqs/FAQ.html -- 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] ISO 8601 "Time Intervals" of the "format with time-unit
Peter Eisentraut wrote: > Bruce Momjian wrote: > > Peter Eisentraut wrote: > > > Bruce Momjian wrote: > > > > > I keep reading about open issues, and deprecating certain > > > > > things, and patch removed, and patch readded. What is going > > > > > on? > > > > > > > > I think the patch just added is OK, no? > > > > > > I don't know, but earlier the identical patch was rejected by you. > > > > I thought he made an adjustment so no backward compatibility was > > broken. > > Then I wouldn't have said "identical". OK, can anyone raise an objection to the patch. The new description means to me that he addressed our concerns and that my original hesitation was unwarranted. -- 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] ISO 8601 "Time Intervals" of the "format with time-unit
Bruce Momjian wrote: > Peter Eisentraut wrote: > > Bruce Momjian wrote: > > > > I keep reading about open issues, and deprecating certain > > > > things, and patch removed, and patch readded. What is going > > > > on? > > > > > > I think the patch just added is OK, no? > > > > I don't know, but earlier the identical patch was rejected by you. > > I thought he made an adjustment so no backward compatibility was > broken. Then I wouldn't have said "identical". ---(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] ISO 8601 "Time Intervals" of the "format with time-unit
Peter Eisentraut wrote: > Bruce Momjian wrote: > > > I keep reading about open issues, and deprecating certain things, > > > and patch removed, and patch readded. What is going on? > > > > I think the patch just added is OK, no? > > I don't know, but earlier the identical patch was rejected by you. I thought he made an adjustment so no backward compatibility was broken. -- 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] ISO 8601 "Time Intervals" of the "format with time-unit
Bruce Momjian wrote: > > I keep reading about open issues, and deprecating certain things, > > and patch removed, and patch readded. What is going on? > > I think the patch just added is OK, no? I don't know, but earlier the identical patch was rejected by you. ---(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] ISO 8601 "Time Intervals" of the "format with time-unit
Peter Eisentraut wrote: > Bruce Momjian wrote: > > 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. > > I keep reading about open issues, and deprecating certain things, and > patch removed, and patch readded. What is going on? I think the patch just added is OK, no? -- 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] ISO 8601 "Time Intervals" of the "format with time-unit
Bruce Momjian wrote: > 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. I keep reading about open issues, and deprecating certain things, and patch removed, and patch readded. What is going on? ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] ISO 8601 "Time Intervals" of the "format with time-unit
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. --- Ron Mayer wrote: > > -Original Message- > > > > Is this ready for application? It looks good to me. However, there is > > an "Open issues" section. > > In my mind there were two categories of open issues > a) ones that are 100% backward (such as the comment about > outputting this format) > and > b) ones that aren't (such as deprecating the current > postgresql shorthand of > '1Y1M'::interval = 1 year 1 minute > in favor of the ISO-8601 > 'P1Y1M'::interval = 1 year 1 month. > > Attached is a patch that addressed all the discussed issues that > did not break backward compatability, including the ability to > output ISO-8601 compliant intervals by setting datestyle to > iso8601basic. > >Ron [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 8: explain analyze is your friend -- 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
