Thread: Support for QNX6, POSIX IPC and PTHREAD-style locking
Here is the patch which adds following things to 7.2: 1. Support for QNX6 (builds cleanly on stock installation and passes all regression tests). 2. HAVE_POSIX_IPC feature, which if enabled switches implementation of IpcSemaphoreXXX() (ipc.[ch]) to POSIX semaphores and mmap(). Enabled on QNX6 but should be useful for lot of platforms. Since IpcSemaphoreCreate() really assumed SysV semaphores, I had to change its prototype *when* this feature is enabled. That function is called from proc.c and spin.c, which were patched accordingly for POSIX case (with #ifdef guards). 3. USE_PTHREAD_MUTEXES feature, which if enabled implements S_LOCK stuff with PTHREAD mutexes. It is useful (better than spinlocks) on non-SMP systems when overhead of kernel call is small compared to overhead of scheduling. Enabled on QNX6. 4. USE_PTHREAD_SPINLOCKS feature which if enabled implements S_LOCK stuff with PTHREAD spinlocks (may not be available on all platforms). It might be better on SMP systems when hardware does not support TAS (in which case SysV semaphores would be used as of now and it is hard to be worse than that). MIPS systems come to mind (and QNX6 runs on them, so it will be enabled on QNX6 in such cases). I haven't put checks for (2), (3) or (4) into configure to not break supported platforms in unexpected ways. Benefits of (3) and (4) are really OS and hardware dependent, so if you think they could be useful for your platform, they have to be enabled in appropriate OS-specific header. Same for HAVE_POSIX_IPC, but that one in fact can be useful on lot of platforms. I've seen benchmark suggesting POSIX semaphores are 4 times faster on Linux than SysV ones. It certainly applies to QNX4 as well and makes emulation of SysV stuff unnecessary. I believe it could be extended to support platforms like Darwin and BeOS which currently also rely on SysV emulation. If there's interest from involved people, we could come up with a better unified abstraction model and implementations for all supported platforms... I've been warned it might be considered too 'major' for 7.2, but please look at the patch before judging. I tried my best to not break existing stuff, all changes are only activated when explicitly enabled, QNX6-specific or obviously compatible (the only 'unguarded' changes are typecasts for things like SemId = -1, since its type is pointer in case of POSIX and fix for broken QNX qsort() which I believe is already commited into CVS). I've spent considerable time doing this and would really appreciate if it went into 7.2. Code builds and runs clean with or without any of above features enabled. The patch generated as recursive GNU-diff between original 7.2b2 and patched (+ make distclean) top level directories, like 'diff -crP pgsql-original pgsql-patched'. It is big mostly because it contains whole new files for QNX6 (-P treats missing files as empty). regards, - igor
Attachment
> Here is the patch which adds following things to 7.2: > > 1. Support for QNX6 (builds cleanly on stock installation and passes all > regression tests). There is no way we can put this in 7.2. We will keep it for 7.3. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
I want to comment on this patch. The author is not happy to have all his QNX6 work wait for 7.3 so I suggested he split the patch in two: one part that is seen only by QNX and regression stuff, and a second patch that adds Posix semaphore support. I think the first part can be put into 7.2 if we can all be sure it is seen only by QNX. FYI, we support QNX4, but not QNX6 at this point. --------------------------------------------------------------------------- > Here is the patch which adds following things to 7.2: > > 1. Support for QNX6 (builds cleanly on stock installation and passes all > regression tests). > > 2. HAVE_POSIX_IPC feature, which if enabled switches implementation of > IpcSemaphoreXXX() (ipc.[ch]) to POSIX semaphores and mmap(). Enabled on QNX6 > but should be useful for lot of platforms. Since IpcSemaphoreCreate() really > assumed SysV semaphores, I had to change its prototype *when* this feature > is enabled. That function is called from proc.c and spin.c, which were > patched accordingly for POSIX case (with #ifdef guards). > > 3. USE_PTHREAD_MUTEXES feature, which if enabled implements S_LOCK stuff > with PTHREAD mutexes. It is useful (better than spinlocks) on non-SMP > systems when overhead of kernel call is small compared to overhead of > scheduling. Enabled on QNX6. > > 4. USE_PTHREAD_SPINLOCKS feature which if enabled implements S_LOCK stuff > with PTHREAD spinlocks (may not be available on all platforms). It might be > better on SMP systems when hardware does not support TAS (in which case SysV > semaphores would be used as of now and it is hard to be worse than that). > MIPS systems come to mind (and QNX6 runs on them, so it will be enabled on > QNX6 in such cases). > > I haven't put checks for (2), (3) or (4) into configure to not break > supported platforms in unexpected ways. Benefits of (3) and (4) are really > OS and hardware dependent, so if you think they could be useful for your > platform, they have to be enabled in appropriate OS-specific header. > > Same for HAVE_POSIX_IPC, but that one in fact can be useful on lot of > platforms. I've seen benchmark suggesting POSIX semaphores are 4 times > faster on Linux than SysV ones. It certainly applies to QNX4 as well and > makes emulation of SysV stuff unnecessary. I believe it could be extended to > support platforms like Darwin and BeOS which currently also rely on SysV > emulation. If there's interest from involved people, we could come up with a > better unified abstraction model and implementations for all supported > platforms... > > I've been warned it might be considered too 'major' for 7.2, but please look > at the patch before judging. I tried my best to not break existing stuff, > all changes are only activated when explicitly enabled, QNX6-specific or > obviously compatible (the only 'unguarded' changes are typecasts for things > like SemId = -1, since its type is pointer in case of POSIX and fix for > broken QNX qsort() which I believe is already commited into CVS). I've spent > considerable time doing this and would really appreciate if it went into > 7.2. Code builds and runs clean with or without any of above features > enabled. > > The patch generated as recursive GNU-diff between original 7.2b2 and patched > (+ make distclean) top level directories, like 'diff -crP pgsql-original > pgsql-patched'. > It is big mostly because it contains whole new files for QNX6 (-P treats > missing files as empty). > > regards, > - igor > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
----- Original Message ----- From: "Bruce Momjian" <pgman@candle.pha.pa.us> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com> Cc: <pgsql-patches@postgresql.org> Sent: Friday, November 23, 2001 2:14 PM Subject: Re: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > > I want to comment on this patch. The author is not happy to have all > his QNX6 work wait for 7.3 so I suggested he split the patch in two: > one part that is seen only by QNX and regression stuff, and a second > patch that adds Posix semaphore support. I think the first part can be > put into 7.2 if we can all be sure it is seen only by QNX. We had some more discussion with Bruce on the nature of changes. Essentially, my point is that all the changes will only be seen on QNX6, with or without Posix semaphore stuff, unless other platforms will explicitly enable new code. I could simply use #ifdef __QNX__ everywhere and not have this discussion, but since Posix semaphore code could be usable for other platforms, I decided to use a 'feature key' rather than 'platform key'. So the code is guarded by #ifdef HAVE_POSIX_IPC and can be enabled on platform-specific basis. The patch will only enable it on QNX6 so it is as 'tight' as '#ifdef __QNX__' would be. Eventually maintainers of other platforms can try it and enable if they like. To prove my point I tried to build patched version on Linux 2.2 (Suse 6.4) and Solaris (2.5/Sparc and 8/Intel). It compiled well and passed all regression tests except geometry (which fails due to usual numeric precision mismatches). That of course still does not cover *all* platforms. I could probably try to check on AIX and HP/UX but not all supported platforms anyway. I believe however that if there were problems with patch, either Linux or Solaris would expose them. I also tried to use Posix semaphores on those platforms and it worked perfectly well on Solaris 8 (it needed proper flags to compiler though). On Linux Posix semaphores can not be shared between processes, so it is of no use for postgres (unless new Linux versions fix that issue). Guess it comes down to vote. I can also do what Bruce suggested (separate patches) if I still did not convince everyone that patch won't hurt as is. Please vote which way you like it (none, separate QNX6 & Posix sem, combined). regards, - igor > > FYI, we support QNX4, but not QNX6 at this point. > > -------------------------------------------------------------------------- - > > > Here is the patch which adds following things to 7.2: > > > > 1. Support for QNX6 (builds cleanly on stock installation and passes all > > regression tests). > > > > 2. HAVE_POSIX_IPC feature, which if enabled switches implementation of > > IpcSemaphoreXXX() (ipc.[ch]) to POSIX semaphores and mmap(). Enabled on QNX6 > > but should be useful for lot of platforms. Since IpcSemaphoreCreate() really > > assumed SysV semaphores, I had to change its prototype *when* this feature > > is enabled. That function is called from proc.c and spin.c, which were > > patched accordingly for POSIX case (with #ifdef guards). > > > > 3. USE_PTHREAD_MUTEXES feature, which if enabled implements S_LOCK stuff > > with PTHREAD mutexes. It is useful (better than spinlocks) on non-SMP > > systems when overhead of kernel call is small compared to overhead of > > scheduling. Enabled on QNX6. > > > > 4. USE_PTHREAD_SPINLOCKS feature which if enabled implements S_LOCK stuff > > with PTHREAD spinlocks (may not be available on all platforms). It might be > > better on SMP systems when hardware does not support TAS (in which case SysV > > semaphores would be used as of now and it is hard to be worse than that). > > MIPS systems come to mind (and QNX6 runs on them, so it will be enabled on > > QNX6 in such cases). > > > > I haven't put checks for (2), (3) or (4) into configure to not break > > supported platforms in unexpected ways. Benefits of (3) and (4) are really > > OS and hardware dependent, so if you think they could be useful for your > > platform, they have to be enabled in appropriate OS-specific header. > > > > Same for HAVE_POSIX_IPC, but that one in fact can be useful on lot of > > platforms. I've seen benchmark suggesting POSIX semaphores are 4 times > > faster on Linux than SysV ones. It certainly applies to QNX4 as well and > > makes emulation of SysV stuff unnecessary. I believe it could be extended to > > support platforms like Darwin and BeOS which currently also rely on SysV > > emulation. If there's interest from involved people, we could come up with a > > better unified abstraction model and implementations for all supported > > platforms... > > > > I've been warned it might be considered too 'major' for 7.2, but please look > > at the patch before judging. I tried my best to not break existing stuff, > > all changes are only activated when explicitly enabled, QNX6-specific or > > obviously compatible (the only 'unguarded' changes are typecasts for things > > like SemId = -1, since its type is pointer in case of POSIX and fix for > > broken QNX qsort() which I believe is already commited into CVS). I've spent > > considerable time doing this and would really appreciate if it went into > > 7.2. Code builds and runs clean with or without any of above features > > enabled. > > > > The patch generated as recursive GNU-diff between original 7.2b2 and patched > > (+ make distclean) top level directories, like 'diff -crP pgsql-original > > pgsql-patched'. > > It is big mostly because it contains whole new files for QNX6 (-P treats > > missing files as empty). > > > > regards, > > - igor > > > > [ Attachment, skipping... ] > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 >
"Igor Kovalenko" <Igor.Kovalenko@motorola.com> writes:
> We had some more discussion with Bruce on the nature of changes.
> Essentially, my point is that all the changes will only be seen on QNX6,
> with or without Posix semaphore stuff, unless other platforms will
> explicitly enable new code.
No, the point is that the Posix semaphore stuff is a major change to a
critical and delicate part of Postgres. It's too late in the 7.2 beta
cycle for such a change to receive the review and testing it needs.
We'll gladly consider it as a non-QNX-specific improvement for 7.3.
We won't ship it in 7.2, QNX-only or otherwise, because we don't have
any confidence in it yet (and no, we don't want to delay 7.2 release
for it either).
While I agree that a SysV emulation layer is ugly, it's by now a
well-tested technology: we've got several other platforms that do it
that way and have been known to work for a few releases. I have some
confidence that that same approach can be transposed into QNX6 and be
trustworthy enough to release with limited testing. I don't yet have
any confidence in changes at higher levels.
Also, I think that your patch as it stands is really only a halfway
measure; it makes the code uglier rather than cleaner. We probably
ought to revamp the higher-level APIs in such a way that either Posix
or SysV semaphores can be used to implement the APIs reasonably cleanly.
I'd prefer to design those changes and revise semaphore handling once,
not tweak it a little in this release and some more in the next.
Perhaps what this really means is that it's too late to think of
supporting QNX6 in PG 7.2 at all, and that we should target it for 7.3
instead. Considering that we're hoping to put out a final candidate
tarball next week, this train has pretty much left the station already.
regards, tom lane
> Perhaps what this really means is that it's too late to think of > supporting QNX6 in PG 7.2 at all, and that we should target it for 7.3 > instead. Considering that we're hoping to put out a final candidate > tarball next week, this train has pretty much left the station already. Agreed. I think a good solution would be to put a patch on an ftp server and have that available for QNX6 users to apply to 7.2.X. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com> Cc: <pgsql-patches@postgresql.org> Sent: Saturday, November 24, 2001 11:47 AM Subject: Re: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > "Igor Kovalenko" <Igor.Kovalenko@motorola.com> writes: > > We had some more discussion with Bruce on the nature of changes. > > Essentially, my point is that all the changes will only be seen on QNX6, > > with or without Posix semaphore stuff, unless other platforms will > > explicitly enable new code. > > No, the point is that the Posix semaphore stuff is a major change to a > critical and delicate part of Postgres. It's too late in the 7.2 beta > cycle for such a change to receive the review and testing it needs. I have feeling we're talking different languages. Can someone explain me how does it need lot of review and testing if all changes are #ifdef-ed and invisible to all currently supported platforms? I just want to understand that point... > While I agree that a SysV emulation layer is ugly, it's by now a > well-tested technology: we've got several other platforms that do it > that way and have been known to work for a few releases. I have some > confidence that that same approach can be transposed into QNX6 and be > trustworthy enough to release with limited testing. I don't yet have > any confidence in changes at higher levels. Does that mean you'd be ok with patch if it did not have Posix semaphores and worked through sysV emulation? I could easily take that part out. We already had similar discussion with Bruce and he was also objecting in similar way until I explained the patch piece by piece and demonstrated why I think it is harmless. Consider following pseudocode: #ifdef HAVE_POSIX_IPC // new code #else // unmodified old code #endif How does that shatter your confidence? Please educate me. > Also, I think that your patch as it stands is really only a halfway > measure; it makes the code uglier rather than cleaner. We probably > ought to revamp the higher-level APIs in such a way that either Posix > or SysV semaphores can be used to implement the APIs reasonably cleanly. > I'd prefer to design those changes and revise semaphore handling once, > not tweak it a little in this release and some more in the next. Theoretically, yes and so far this is the only valid argument that I see. Indeed it would be better to revamp IpcSemaphoreXXX() API to cover SysV, Posix and BeOS semaphores. I wanted to do that, but I did not because I anticipated concerns about changing existing code. So my goal was to change as little of it as possible. I hoped it would help me to get it into 7.2, but now you're pointing to it as to a mistake ;) In practice you have to start with something and it is not always bad idea to start with smaller steps. You may keep thinking about higher goals for a year and nothing will happen until someone willing to spend his time on that comes in and does it. I already spent mine and even went out of my way to test it on various platforms. What I get for that is people refusing to accept the patch without even reading it. That's really encouraging ... > Perhaps what this really means is that it's too late to think of > supporting QNX6 in PG 7.2 at all, and that we should target it for 7.3 > instead. Considering that we're hoping to put out a final candidate > tarball next week, this train has pretty much left the station already. I think I asked for formal vote. Delaying it for 7.3 means somebody will have to essentially redo it and I did not see any volunteers popping up yet. regards, - igor
"Igor Kovalenko" <Igor.Kovalenko@motorola.com> writes:
>> No, the point is that the Posix semaphore stuff is a major change to a
>> critical and delicate part of Postgres. It's too late in the 7.2 beta
>> cycle for such a change to receive the review and testing it needs.
> I have feeling we're talking different languages. Can someone explain me how
> does it need lot of review and testing if all changes are #ifdef-ed and
> invisible to all currently supported platforms?
It needs review and testing because we're not convinced that it's right.
We're not interested in shipping a possibly-unreliable QNX6 port just to
have a QNX6 port.
Also, patches that introduce a bunch of #ifdefs into what had been
non-system-specific code are disliked on general principles around this
project: they make the code harder to read and less maintainable over
the long run. In that sense the patch is going in the wrong direction.
There needs to be some work done on restructuring the existing code to
preserve readability.
I do believe that it's a good idea to support Posix semaphores; that's
been in the wind for awhile, and it's clear that QNX6 is not the only
platform that would benefit. We will take up this code, in some form,
in 7.3. But I don't think it's a wise idea to cram it into 7.2.
7.2 is already two months behind schedule, and I don't want to risk
any more delays in this release cycle.
> ... What I get for that is people refusing to
> accept the patch without even reading it. That's really encouraging ...
I *have* read it. More than once. I'm not saying you've done bad work.
It's a great starting point, in fact. But I don't want to apply it
as-is, and I don't want to hold up 7.2 anymore in order to get QNX6
support into 7.2.
regards, tom lane
----- Original Message ----- From: "Tom Lane" <tgl@sss.pgh.pa.us> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com> Cc: <pgsql-patches@postgresql.org> Sent: Sunday, November 25, 2001 12:08 PM Subject: Re: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > "Igor Kovalenko" <Igor.Kovalenko@motorola.com> writes: > >> No, the point is that the Posix semaphore stuff is a major change to a > >> critical and delicate part of Postgres. It's too late in the 7.2 beta > >> cycle for such a change to receive the review and testing it needs. > > > I have feeling we're talking different languages. Can someone explain me how > > does it need lot of review and testing if all changes are #ifdef-ed and > > invisible to all currently supported platforms? > > It needs review and testing because we're not convinced that it's right. > We're not interested in shipping a possibly-unreliable QNX6 port just to > have a QNX6 port. You're mixing issues related to QNX6 port per say and to Posix semaphores in that statement. There is nothing more unreliable (even potentially) in QNX6 port than in some other platforms. Implementation of Posix semaphores could be considered non-proven, but that's separate issue. I already sent separate patch which deals only with QNX6 support (no Posix semaphores, no Pthread locking). It works through sysV emulation and should not be considered any less reliable than QNX4 port (emulation code was essentially inherited from there). Apparently it is still awaiting approval to be distributed. Bruce has copy though. > Also, patches that introduce a bunch of #ifdefs into what had been > non-system-specific code are disliked on general principles around this > project: they make the code harder to read and less maintainable over > the long run. In that sense the patch is going in the wrong direction. > There needs to be some work done on restructuring the existing code to > preserve readability. You can not avoid #ifdefs completely. At some point somewhere you're bound to have them. Of course it is better to have them encapsulated on lower level. I believe that's what I did, except for those in proc.c & spin.c but I already explained why I did that. So I don't think the patch goes in wrong direction, it just goes half-way due to [intentional] compromise. It could be structured better, but only at the cost of making more changes. > I do believe that it's a good idea to support Posix semaphores; that's > been in the wind for awhile, and it's clear that QNX6 is not the only > platform that would benefit. We will take up this code, in some form, > in 7.3. But I don't think it's a wise idea to cram it into 7.2. > 7.2 is already two months behind schedule, and I don't want to risk > any more delays in this release cycle. > > > ... What I get for that is people refusing to > > accept the patch without even reading it. That's really encouraging ... > > I *have* read it. More than once. I'm not saying you've done bad work. > It's a great starting point, in fact. But I don't want to apply it > as-is, and I don't want to hold up 7.2 anymore in order to get QNX6 > support into 7.2. Bruce said he'd accept patch without Posix semaphores. And I asked to vote 3-way (whole patch, qnx6-support-only or none). What you're saying appears to be 'none' but you're not making it clear if that's 'no to whole patch', 'no to posix semaphores', 'no to qnx6-support-only patch' or 'no to anything'. My understanding at this point is, there should not be problem with applying QNX6-support-only patch, as long as it works within existing framework and does not introduce non-proven code anywhere. That's what new version of patch is. regards, - igor
> Bruce said he'd accept patch without Posix semaphores. And I asked to vote > 3-way (whole patch, qnx6-support-only or none). What you're saying appears > to be 'none' but you're not making it clear if that's 'no to whole patch', > 'no to posix semaphores', 'no to qnx6-support-only patch' or 'no to > anything'. > > My understanding at this point is, there should not be problem with applying > QNX6-support-only patch, as long as it works within existing framework and > does not introduce non-proven code anywhere. That's what new version of > patch is. I will vote for the QNX-only version. However, I will not apply any patch unless there is agreement from the group. It is up the group to decide the issue. However,even though I will vote for the QNX-only version, I am doing this only because you feel so strongly something should be in 7.2. My personal opinion is that this entire patch should wait for 7.3. It is very late in 7.2 development and even a reviewed patch of this complexity causes more risk than benefit. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
----- Original Message ----- From: "Bruce Momjian" <pgman@candle.pha.pa.us> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com> Cc: <pgsql-patches@postgresql.org> Sent: Sunday, November 25, 2001 6:34 PM Subject: Re: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > > Bruce said he'd accept patch without Posix semaphores. And I asked to vote > > 3-way (whole patch, qnx6-support-only or none). What you're saying appears > > to be 'none' but you're not making it clear if that's 'no to whole patch', > > 'no to posix semaphores', 'no to qnx6-support-only patch' or 'no to > > anything'. > > > > My understanding at this point is, there should not be problem with applying > > QNX6-support-only patch, as long as it works within existing framework and > > does not introduce non-proven code anywhere. That's what new version of > > patch is. > > I will vote for the QNX-only version. However, I will not apply any > patch unless there is agreement from the group. > > It is up the group to decide the issue. > > However,even though I will vote for the QNX-only version, I am doing > this only because you feel so strongly something should be in 7.2. My > personal opinion is that this entire patch should wait for 7.3. It is > very late in 7.2 development and even a reviewed patch of this > complexity causes more risk than benefit. Fair enough. I am asking you to get something into 7.2 for practical reasons. Sooner people will get something, sooner someone will uncover problems is there are any. That would allow to have reasonable confidence by the time 7.3 rolled out. Of course there are betas for such purpose, but people prefer to work with releases, so I simply want it to have larger exposure. I also want clearer test case - forcing people to use [potentially unstable] alphas/betas on QNX6 would distort the picture - if there will be problems it won't be very clear whether they are related to port or to the base code. You can mark QNX6 support as 'experimental' if that will help anything. thanks, - igor
Igor Kovalenko writes: > Fair enough. I am asking you to get something into 7.2 for practical > reasons. Sooner people will get something, sooner someone will uncover > problems is there are any. That would allow to have reasonable confidence by > the time 7.3 rolled out. The patch looks mostly harmless in the sense that it doesn't break anything else, although some parts are clearly bogus, such as the patches to 'configure' and 'resultmap' and some "comment out if you want xyz" comments where there's nothing to comment out nearby. (Plus, commenting stuff in and out in makefiles is not an acceptable practice to spread.) CFLAGS in template/qnx6 should probably be -O2 unless you have reasons to do otherwise, which should be documented. LIBS= has no business in the template file. Overriding CC as done in port/qnx6/Makefile is not valid. The SHLIB_LINK line in Makefile.shlib is not possibly correct. (The same goes for most of the other SHLIB_LINK lines there, btw.) These issues are "mostly harmless", but they would need to be fixed. My mind on this is that we hope to put out the first *release candidate* this week, which means, "if there are no more serious bugs, this is the final release". This would mean that this patch would receive virtually *no* testing before release. Surely I trust your word that says that this patch makes PostgreSQL run correctly on your system, and it doesn't look like it'll break anything else. But this kind of reasoning is not responsible. PostgreSQL is, for better or worse, not developed by proving that the code is theoretically correct; we allocate for extensive beta testing because we know we need extensive beta testing. Exceptions are always made, but a new feature has never qualified for such an exception. "There will always be another release." -- Peter Eisentraut peter_e@gmx.net
My reply went only to Peter. Can't get used to the fact that this list sends replies to author rather than list ;) ----- Original Message ----- From: "Peter Eisentraut" <peter_e@gmx.net> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com> Cc: "Bruce Momjian" <pgman@candle.pha.pa.us>; <pgsql-patches@postgresql.org> Sent: Monday, November 26, 2001 1:15 PM Subject: Re: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > Igor Kovalenko writes: > > > Fair enough. I am asking you to get something into 7.2 for practical > > reasons. Sooner people will get something, sooner someone will uncover > > problems is there are any. That would allow to have reasonable confidence by > > the time 7.3 rolled out. > > The patch looks mostly harmless in the sense that it doesn't break > anything else, although some parts are clearly bogus, I am all for making it 'proper' if you elaborate a bit. > such as the patches > to 'configure' and 'resultmap' How those are bogus? Configure needs to recognize system, otherwise it will say Postgres has not been ported to your OS .... Patch to resultmap is not as bogus as it may seem to you. Trouble is, the QNX4 port plays against QNX6 there, by 'fake' matches which are not appropriate for QNX6. That happens because host ID string (as produced by config.guess) contains common part on both systems. Since I did not want to touch other ports, I had little choice but to force matching with proper files on QNX6, even if they are default files (otherwise QNX4-specific ones were matched, producing bogus 'failures' of tests). You don't think I would put things in there for no reason, would you? > and some "comment out if you want xyz" > comments where there's nothing to comment out nearby. (Plus, commenting > stuff in and out in makefiles is not an acceptable practice to spread.) Those comments are leftover from Posix semaphores stuff. Those makefiles would not need to be patched if Posix semaphores were used, but patches are needed otherwise. If you can tell me how to handle that more elegantly, I am all ears. Since now there's no posix semaphores in the patch, those comments can be wiped out, I just did not think it would be such a bad sin to have them there. Assuming there will be Posix semaphores eventually, some comments would need to be there again... > CFLAGS in template/qnx6 should probably be -O2 unless you have reasons to > do otherwise, which should be documented. It is somewhat problematic on QNX. Takes a lot of memory to compile and is prone to 'internal compiler errors'. The default is -O anyway. > LIBS= has no business in the > template file. I did not invent it. Template was copied from QNX4, how I was supposed to know what has business there and what not. Univel and Windows have LIBS in template too. > Overriding CC as done in port/qnx6/Makefile is not valid. This one is my bad... > The SHLIB_LINK line in Makefile.shlib is not possibly correct. (The same > goes for most of the other SHLIB_LINK lines there, btw.) These issues are SHLIB_LINK was copied from other platforms, most of which have -lc there. Following the logic in comments, I added -lm -lsocket since they are certainly needed on QNX6. If that's not possibly correct, you should educate us all. > "mostly harmless", but they would need to be fixed. > > My mind on this is that we hope to put out the first *release candidate* > this week, which means, "if there are no more serious bugs, this is the > final release". This would mean that this patch would receive virtually > *no* testing before release. Surely I trust your word that says that this > patch makes PostgreSQL run correctly on your system, and it doesn't look > like it'll break anything else. But this kind of reasoning is not > responsible. PostgreSQL is, for better or worse, not developed by proving > that the code is theoretically correct; we allocate for extensive beta > testing because we know we need extensive beta testing. The patch was being tested for last couple of weeks by several people. The fact that is passes regression tests should give some confidence too, otherwise what is the point to have them at all. > Exceptions are > always made, but a new feature has never qualified for such an exception. > > "There will always be another release." Sure. I think I made it clear enough why I want it in 7.2. I also took away the most interesting part of patch since there were some valid logical objections against including it as is. I thought that would clear the way, but now I am presented with unbeatable 'this is untested' reason. Who do you think would be testing it if it was included in 7.3 or 8.0 for that matter? Not you, I suspect. That would be same old me probably and why do you think few weeks of testing by me and my fellows then will be somehow better than same testing we've done now? regards, - igor
On Mon, Nov 26, 2001 at 09:39:42PM -0600, Igor Kovalenko wrote: > > > Exceptions are > > always made, but a new feature has never qualified for such an exception. > > > > "There will always be another release." > > Sure. I think I made it clear enough why I want it in 7.2. I also took away > the most interesting part of patch since there were some valid logical > objections against including it as is. I thought that would clear the way, > but now I am presented with unbeatable 'this is untested' reason. Who do you > think would be testing it if it was included in 7.3 or 8.0 for that matter? > Not you, I suspect. That would be same old me probably and why do you think > few weeks of testing by me and my fellows then will be somehow better than > same testing we've done now? Because if it _does_ break other peoples builds, it'll happen in an unstable devlopment tree, which constitutes testing, and everyone just says 'hmm, fix that'. If it gets in now, there's a chance it'll break someone's build _no matter how much you've tested it_. If that happens in a _stable release_, now we have a problem. PostgreSQL has a strong track record of no 'brown paper bag' bugs in stable releases (well, almost none). You seldom see a minor version number over 3 or 4, for that reason. Logically 'proving' that your patch _couldn't possibly_ cause any problems has no impact on the matter: there are stranger build environments out there than you've dreamed of, and someone has ported PostgreSQL to most of them. At this point, I'd suggest you yield gracefully: the core developers don't want it right now, but have read your patch and given you valuable feedback, to improve you submission for 7.3 (in about two weeks). Ross -- Ross Reedstrom, Ph.D. reedstrm@rice.edu Executive Director phone: 713-348-6166 Gulf Coast Consortium for Bioinformatics fax: 713-348-6182 Rice University MS-39 Houston, TX 77005
"Ross J. Reedstrom" <reedstrm@rice.edu> writes:
> ... submission for 7.3 (in about two weeks).
That's probably overly optimistic; if things don't slip any more,
it's still unlikely that we'll open the 7.3 branch for development
before the first of the year. Traditionally we don't make the branch
immediately after release, because there are always a few bugs
justifying a dot-1 release that somehow sneak through beta testing.
It's easier to deal with these if we don't have to double-patch both
current and stable branches.
However, the fact that we always end up having to make a dot-1 release
is not for lack of trying. The dot-0 release is *supposed* to be
bulletproof.
regards, tom lane
On Tue, Nov 27, 2001 at 10:49:31AM -0500, Tom Lane wrote: > "Ross J. Reedstrom" <reedstrm@rice.edu> writes: > > ... submission for 7.3 (in about two weeks). > > That's probably overly optimistic; if things don't slip any more, > it's still unlikely that we'll open the 7.3 branch for development > before the first of the year. Traditionally we don't make the branch > immediately after release, because there are always a few bugs > justifying a dot-1 release that somehow sneak through beta testing. > It's easier to deal with these if we don't have to double-patch both > current and stable branches. I guess I was repeating something from earlier up the thread. However, given how long this beta has been, we _might_ find very few things to fix, so the 'double-patch' burden might be small. <finger crossed> I know there are a lot of people holding patches for 7.3, as well. It might be good to try to get it opened before the inter-holiday week, since people often manage to find some free time then. > > However, the fact that we always end up having to make a dot-1 release > is not for lack of trying. The dot-0 release is *supposed* to be > bulletproof. And you've never got up to .16, that's for sure. ;-) Ross
> On Tue, Nov 27, 2001 at 10:49:31AM -0500, Tom Lane wrote: > > "Ross J. Reedstrom" <reedstrm@rice.edu> writes: > > > ... submission for 7.3 (in about two weeks). > > > > That's probably overly optimistic; if things don't slip any more, > > it's still unlikely that we'll open the 7.3 branch for development > > before the first of the year. Traditionally we don't make the branch > > immediately after release, because there are always a few bugs > > justifying a dot-1 release that somehow sneak through beta testing. > > It's easier to deal with these if we don't have to double-patch both > > current and stable branches. > > I guess I was repeating something from earlier up the thread. However, > given how long this beta has been, we _might_ find very few things > to fix, so the 'double-patch' burden might be small. <finger crossed> > I know there are a lot of people holding patches for 7.3, as well. It > might be good to try to get it opened before the inter-holiday week, > since people often manage to find some free time then. Yes, exactly what I suspect, and also my personal target date for a branch. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Let me add that the number of patches that go into .1 and later releases > > is decreasing, so we may be more aggresive this time in branching the > > development tree earlier this time. > > Maybe. Beta has been so quiet that I figure either (a) we've got a real > solid release here, or (b) hardly anyone's beta testing. If it's (a) > then the post-release period will be quiet too. If it's (b) ... > > If things are still quiet two weeks after release then I'd be ready > to make the branch then. If not, we'll know we need more time. I can't believe people aren't testing the beta. They have jumped all over betas in the past. The only reason people wouldn't be testing is because we are getting less popular, and I _know_ that isn't true. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> "Ross J. Reedstrom" <reedstrm@rice.edu> writes: > > ... submission for 7.3 (in about two weeks). > > That's probably overly optimistic; if things don't slip any more, > it's still unlikely that we'll open the 7.3 branch for development > before the first of the year. Traditionally we don't make the branch > immediately after release, because there are always a few bugs > justifying a dot-1 release that somehow sneak through beta testing. > It's easier to deal with these if we don't have to double-patch both > current and stable branches. > > However, the fact that we always end up having to make a dot-1 release > is not for lack of trying. The dot-0 release is *supposed* to be > bulletproof. Let me add that the number of patches that go into .1 and later releases is decreasing, so we may be more aggresive this time in branching the development tree earlier this time. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Let me add that the number of patches that go into .1 and later releases
> is decreasing, so we may be more aggresive this time in branching the
> development tree earlier this time.
Maybe. Beta has been so quiet that I figure either (a) we've got a real
solid release here, or (b) hardly anyone's beta testing. If it's (a)
then the post-release period will be quiet too. If it's (b) ...
If things are still quiet two weeks after release then I'd be ready
to make the branch then. If not, we'll know we need more time.
regards, tom lane
----- Original Message ----- From: "Ross J. Reedstrom" <reedstrm@rice.edu> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com> Cc: <pgsql-patches@postgresql.org> Sent: Tuesday, November 27, 2001 9:21 AM Subject: Re: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > On Mon, Nov 26, 2001 at 09:39:42PM -0600, Igor Kovalenko wrote: > > > > > Exceptions are > > > always made, but a new feature has never qualified for such an exception. > > > > > > "There will always be another release." > > > > Sure. I think I made it clear enough why I want it in 7.2. I also took away > > the most interesting part of patch since there were some valid logical > > objections against including it as is. I thought that would clear the way, > > but now I am presented with unbeatable 'this is untested' reason. Who do you > > think would be testing it if it was included in 7.3 or 8.0 for that matter? > > Not you, I suspect. That would be same old me probably and why do you think > > few weeks of testing by me and my fellows then will be somehow better than > > same testing we've done now? > > Because if it _does_ break other peoples builds, it'll happen in an > unstable devlopment tree, which constitutes testing, and everyone just > says 'hmm, fix that'. If it gets in now, there's a chance it'll break > someone's build _no matter how much you've tested it_. If that happens > in a _stable release_, now we have a problem. PostgreSQL has a strong > track record of no 'brown paper bag' bugs in stable releases (well, > almost none). You seldom see a minor version number over 3 or 4, for > that reason. Logically 'proving' that your patch _couldn't possibly_ > cause any problems has no impact on the matter: there are stranger build > environments out there than you've dreamed of, and someone has ported > PostgreSQL to most of them. Gotta love this argument. If what you're saying has any real ground, it just means Postgres source structure and build process sucks beyond anything I've seen before. It SHOULD BE possible to logically prove that addition of new platform does not break others. If not, you've got yourself problem without my patch. > At this point, I'd suggest you yield gracefully: the core developers > don't want it right now, but have read your patch and given you valuable > feedback, to improve you submission for 7.3 (in about two weeks). <bow><bow><bow> At this point, I suggest core developers will ask me when/if they feel like they want another submission from me. <bow><bow><bow> Merry X-mas & happy new year everyone. <yield><yield><yield> - igor
> > At this point, I'd suggest you yield gracefully: the core developers > > don't want it right now, but have read your patch and given you valuable > > feedback, to improve you submission for 7.3 (in about two weeks). > > <bow><bow><bow> > At this point, I suggest core developers will ask me when/if they > feel like > they want another submission from me. > <bow><bow><bow> > > Merry X-mas & happy new year everyone. > > <yield><yield><yield> > - igor *sigh* I don't know if I really should be replying to a troll post, but what the heck... Look, Igor - noone's saying that your code isn't any good. They're just saying that it's too late in the development cycle to add it. That's the rules, and they apply to everyone. Your code would be quite happily accepted into 7.3, so get over it. I've submitted a few patches in my time, in fact just before the 7.1 release - and my stuff was held until 7.2. And yes, I'm kind of happy about seeing my code in a release. However, I didn't complain when my patch was put on hold, or the core developers gave me lots of feedback about issues with my patch. Chris
> <bow><bow><bow>
> At this point, I suggest core developers will ask me when/if they feel like
> they want another submission from me.
> <bow><bow><bow>
> Merry X-mas & happy new year everyone.
> <yield><yield><yield>
:))
I've followed this thread mostly after the fact, after perhaps
contributing to Igor's hopes that this might get into 7.2. I think that
pushing this off to 7.3 (or *perhaps* 7.2.1 for QNX6-only changes) is
the Right Thing.
We are *always* interested in patches, but actually applying them during
final betas is not a good thing in general. Participating during the
development cycle is welcome, and the Posix semaphores are likely to
pique several people's interest. Jump in again a few weeks after 7.2 is
released.
- Thomas
----- Original Message ----- From: "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> To: "Igor Kovalenko" <Igor.Kovalenko@motorola.com>; "Ross J. Reedstrom" <reedstrm@rice.edu> Cc: <pgsql-patches@postgresql.org> Sent: Tuesday, November 27, 2001 8:17 PM Subject: RE: [PATCHES] Support for QNX6, POSIX IPC and PTHREAD-style locking > > > At this point, I'd suggest you yield gracefully: the core developers > > > don't want it right now, but have read your patch and given you valuable > > > feedback, to improve you submission for 7.3 (in about two weeks). > > > > <bow><bow><bow> > > At this point, I suggest core developers will ask me when/if they > > feel like > > they want another submission from me. > > <bow><bow><bow> > > > > Merry X-mas & happy new year everyone. > > > > <yield><yield><yield> > > - igor > > *sigh* I don't know if I really should be replying to a troll post, but what > the heck... > > Look, Igor - noone's saying that your code isn't any good. They're just > saying that it's too late in the development cycle to add it. That's the > rules, and they apply to everyone. Your code would be quite happily > accepted into 7.3, so get over it. The trouble is, there seems to be NO rules. 'Too late' does not qualify as rule, IMHO. Not as formal one anyway and informal ones are hard to follow precisely. There should be formal mechanism for acceptance of patches, like 'it should satisfy criterias x,y,z build at least on platforms X,Y,Z' etc. I did not even see much of the vote. There was bunch of people who gave me 'mixed signals' and every single one of them had tendency to speak on behalf of all. OTOH, not a single one of them apparently has tried to apply the patch on his platform to actually help me verify the patch. I did that for 3 platforms and I was sort of assuming if everyone does it at least for his one, it should be demonstrated in no time that patch is harmless. That did not happened, so I was jumping through a lot of hoops only to be presented with new ones. Never good enough... So I was bitter not about delaying or being 'rejected'. It was about lack of cooperation from you fellas. > I've submitted a few patches in my time, in fact just before the 7.1 > release - and my stuff was held until 7.2. And yes, I'm kind of happy about > seeing my code in a release. However, I didn't complain when my patch was > put on hold, or the core developers gave me lots of feedback about issues > with my patch. There was not lots of feedback in my view. The only meaningful thing was said by Tom, about better doing new abstraction for semaphores, than putting #ifdefs into higher levels. But I knew that myself, so it was not hard for me to agree. Bunch of remarks made by Peter were almost all unjustified. He quickly decided that some parts are 'bogus' and never followed up on them. Funny thing, there was in fact an oversight in the PTHREAD mutexes code for locks, but nobody picked that up ;) Don't worry, I will 'get over', and this is not intended to be a troll. Miscommunications happen and this might be lesson for me and others. regards, - igor
"Igor Kovalenko" <Igor.Kovalenko@motorola.com> writes:
> The trouble is, there seems to be NO rules. 'Too late' does not qualify as
> rule, IMHO. Not as formal one anyway and informal ones are hard to follow
> precisely. There should be formal mechanism for acceptance of patches,
No, we don't have formal rules, and it's unlikely that you'll convince
many people in this project that we should. The one formal rule that
we do adhere to is "no new features during beta". I'm not sure if we've
ever really decided whether a new port is a new feature, but I'd lean
to the view that it is.
I realize that it seemed that you were facing increasingly higher
demands; in fact you were, because the closer we get to release the
less inclined we are to apply non-essential patches. It was strictly
a schedule thing. I'm still not sure that you've quite absorbed the
point that we're trying to have a release candidate out *this week*.
Two weeks ago there was more flexibility, but we're down to the wire
now and only important bug fixes are going in.
In retrospect we should probably have told you to start with that there
was no point in trying to get QNX6 support into 7.2. A localized patch
(only adding port/qnx6/ directory and contents, no changes to shared
files) perhaps would have been accepted two weeks ago, but by the time
you had refined your submission down to that we were a lot closer to
release than we were at the start of the discussion.
The fault for the miscommunication is probably mine, in that I didn't
make it clear to you in the beginning that time was of the essence.
I would like to apologize for that and move on. Let's focus on how we
can make 7.3 better, rather than worrying about what might or might not
have gotten into 7.2 if the calendar had been a little different.
regards, tom lane
Tom Lane wrote: > > The fault for the miscommunication is probably mine, in that I didn't > make it clear to you in the beginning that time was of the essence. > I would like to apologize for that and move on. Thanks. Actually you were one most helpful with the port. I also actually understood quite well that time was important. It was moderation what killed the time. Distribution of first patch was delayed by almost a week and second version was still delayed by few days... I don't want to blame anyone for those delays, but wondering why the heck you need manual approvals here. You require people to subscribe, so what's the point? regards, - igor
> In retrospect we should probably have told you to start with that there
> was no point in trying to get QNX6 support into 7.2. A localized patch
> (only adding port/qnx6/ directory and contents, no changes to shared
> files) perhaps would have been accepted two weeks ago, but by the time
> you had refined your submission down to that we were a lot closer to
> release than we were at the start of the discussion.
The minute the first patch appeared, I said it was too late and would be
in 7.3. and put it in the queue:
http://candle.pha.pa.us/cgi-bin/pgpatches2
Igor didn't like that so I gave ideas to see if it could be trimmed down
and slipped in. I thought trimmed-down version might make it, but even
then, the odds were against it. I said I would pitch it to the group as
best I could.
At a certain point, even the work of verifying a patch is 100% safe is
too much work, and we are better off moving on.
Igor, it is not a personal thing about you or the patch. It is just the
mechanics of getting a release out. We can't make everyone happy, and
you are seeing that now.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Igor Kovalenko wrote:
> Here is the patch which adds following things to 7.2:
>
> 1. Support for QNX6 (builds cleanly on stock installation and passes all
> regression tests).
>
> 2. HAVE_POSIX_IPC feature, which if enabled switches implementation of
> IpcSemaphoreXXX() (ipc.[ch]) to POSIX semaphores and mmap(). Enabled on QNX6
> but should be useful for lot of platforms. Since IpcSemaphoreCreate() really
> assumed SysV semaphores, I had to change its prototype *when* this feature
> is enabled. That function is called from proc.c and spin.c, which were
> patched accordingly for POSIX case (with #ifdef guards).
>
> 3. USE_PTHREAD_MUTEXES feature, which if enabled implements S_LOCK stuff
> with PTHREAD mutexes. It is useful (better than spinlocks) on non-SMP
> systems when overhead of kernel call is small compared to overhead of
> scheduling. Enabled on QNX6.
>
> 4. USE_PTHREAD_SPINLOCKS feature which if enabled implements S_LOCK stuff
> with PTHREAD spinlocks (may not be available on all platforms). It might be
> better on SMP systems when hardware does not support TAS (in which case SysV
> semaphores would be used as of now and it is hard to be worse than that).
> MIPS systems come to mind (and QNX6 runs on them, so it will be enabled on
> QNX6 in such cases).
>
> I haven't put checks for (2), (3) or (4) into configure to not break
> supported platforms in unexpected ways. Benefits of (3) and (4) are really
> OS and hardware dependent, so if you think they could be useful for your
> platform, they have to be enabled in appropriate OS-specific header.
>
> Same for HAVE_POSIX_IPC, but that one in fact can be useful on lot of
> platforms. I've seen benchmark suggesting POSIX semaphores are 4 times
> faster on Linux than SysV ones. It certainly applies to QNX4 as well and
> makes emulation of SysV stuff unnecessary. I believe it could be extended to
> support platforms like Darwin and BeOS which currently also rely on SysV
> emulation. If there's interest from involved people, we could come up with a
> better unified abstraction model and implementations for all supported
> platforms...
>
> I've been warned it might be considered too 'major' for 7.2, but please look
> at the patch before judging. I tried my best to not break existing stuff,
> all changes are only activated when explicitly enabled, QNX6-specific or
> obviously compatible (the only 'unguarded' changes are typecasts for things
> like SemId = -1, since its type is pointer in case of POSIX and fix for
> broken QNX qsort() which I believe is already commited into CVS). I've spent
> considerable time doing this and would really appreciate if it went into
> 7.2. Code builds and runs clean with or without any of above features
> enabled.
>
> The patch generated as recursive GNU-diff between original 7.2b2 and patched
> (+ make distclean) top level directories, like 'diff -crP pgsql-original
> pgsql-patched'.
> It is big mostly because it contains whole new files for QNX6 (-P treats
> missing files as empty).
>
> regards,
> - igor
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026