Thread: Re: [HACKERS] Question on win32 semaphore simulation
pgman wrote: > Qingqing Zhou wrote: > > > > > > On Sun, 7 May 2006, Bruce Momjian wrote: > > > > > > > > > > Leave 'em alone. That code has zero field validation, and should > > > > certainly not get shipped until it's survived a beta-test cycle. > > > > > > Uh, this is a bug fix, and the patch I am asking about is not the Win32 > > > semaphore reimplementation but a more limited fix. > > > > Sorry for the late reply. Maybe more intensive tests are needed? Since > > this bug seems could not lead data corruption, we can wait till next bug > > report and let the user test it then decide to apply? > > Well we did have a bug report by Peter Brant, and a test by him with the > patch that fixes it, so it seems it should be applied. The URLs I > posted had that information. Correction. It only goes into 8.0.X and 8.1.X. CVS HEAD has a rewritten file. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Well we did have a bug report by Peter Brant, and a test by him with the
>> patch that fixes it, so it seems it should be applied. The URLs I
>> posted had that information.
> Correction. It only goes into 8.0.X and 8.1.X. CVS HEAD has a
> rewritten file.
That seems a bit scary --- patching stable branches with a patch that
will get zero testing in HEAD? I think some more testing besides
Peter's would be smart first. What if the patch behaves differently
on different versions of Windows, for instance?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Well we did have a bug report by Peter Brant, and a test by him with the > >> patch that fixes it, so it seems it should be applied. The URLs I > >> posted had that information. > > > Correction. It only goes into 8.0.X and 8.1.X. CVS HEAD has a > > rewritten file. > > That seems a bit scary --- patching stable branches with a patch that > will get zero testing in HEAD? I think some more testing besides > Peter's would be smart first. What if the patch behaves differently > on different versions of Windows, for instance? We can wait for someone else to report the problem. I think it only happened on Win2003 SP1. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Ah, sorry for the late response (and for any confusion), but the only thing I tested was Qingqing's rewritten semaphore implementation. I didn't test the proposed bug fixes to the existing semaphore implementation. We've never been able reproduce (or even trigger) the original "sem_ctl failed" error in a testing environment so it would be hard to say if the changes to win32/sema.c have an impact on it or not. On the other hand, win32_sema.c seems to solve the pgbench lockups reported earlier by Jim N. and it successfully completes a reasonably brutal stress test with real world data and real world queries (which at least is a good indication that it basically works). Pete >>> Bruce Momjian <pgman@candle.pha.pa.us> 05/11/06 4:08 am >>> Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Well we did have a bug report by Peter Brant, and a test by him with the > >> patch that fixes it, so it seems it should be applied. The URLs I > >> posted had that information.
Peter Brant wrote: > Ah, sorry for the late response (and for any confusion), but the only > thing I tested was Qingqing's rewritten semaphore implementation. I > didn't test the proposed bug fixes to the existing semaphore > implementation. Oh, OK. The email mentioned the semaphore patch without specifying which one, so I assume it was the shorter one. > We've never been able reproduce (or even trigger) the original "sem_ctl > failed" error in a testing environment so it would be hard to say if the > changes to win32/sema.c have an impact on it or not. On the other hand, > win32_sema.c seems to solve the pgbench lockups reported earlier by Jim > N. and it successfully completes a reasonably brutal stress test with > real world data and real world queries (which at least is a good > indication that it basically works). OK, let's consider the item closed. We didn't backpatch the new win32_sema.c file to 8.1.X or 8.0.X, so let'see if we get more reports. Thanks. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Peter Brant wrote:
>> We've never been able reproduce (or even trigger) the original "sem_ctl
>> failed" error in a testing environment so it would be hard to say if the
>> changes to win32/sema.c have an impact on it or not. On the other hand,
>> win32_sema.c seems to solve the pgbench lockups reported earlier by Jim
>> N. and it successfully completes a reasonably brutal stress test with
>> real world data and real world queries (which at least is a good
>> indication that it basically works).
> OK, let's consider the item closed. We didn't backpatch the new
> win32_sema.c file to 8.1.X or 8.0.X, so let'see if we get more reports.
Based on that, backpatching the new win32_sema.c implementation is
probably more defensible than applying the proposed smaller patch
anyway; it's survived more testing.
My inclination is to do nothing to the back branches, but if we get more
field reports of trouble with them, maybe that's what to do. (I'd be
happier if 8.2 gets through beta first, as I'm still a bit worried about
the do-all-Windows-versions-act-the-same bit.)
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Peter Brant wrote: > >> We've never been able reproduce (or even trigger) the original "sem_ctl > >> failed" error in a testing environment so it would be hard to say if the > >> changes to win32/sema.c have an impact on it or not. On the other hand, > >> win32_sema.c seems to solve the pgbench lockups reported earlier by Jim > >> N. and it successfully completes a reasonably brutal stress test with > >> real world data and real world queries (which at least is a good > >> indication that it basically works). > > > OK, let's consider the item closed. We didn't backpatch the new > > win32_sema.c file to 8.1.X or 8.0.X, so let'see if we get more reports. > > Based on that, backpatching the new win32_sema.c implementation is > probably more defensible than applying the proposed smaller patch > anyway; it's survived more testing. > > My inclination is to do nothing to the back branches, but if we get more > field reports of trouble with them, maybe that's what to do. (I'd be > happier if 8.2 gets through beta first, as I'm still a bit worried about > the do-all-Windows-versions-act-the-same bit.) Agreed. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +