Re: [HACKERS] [PATCHES] Bad bug in fopen() wrapper code
Magnus, is this the right fix? Well, actually msdn states: Return Value If successful, _setmode returns the previous translation mode. A return value of -1 indicates an error So, shouldn't we be testing for -1 instead of 0 ? The thing is probably academic, since _setmode is only supposed to fail on invalid file handle or invalid mode. So basically, given our code, it should only fail if filemode is (O_BINARY | O_TEXT) both flags set. Andreas ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
[PATCHES] 7.4, 8.0 branches @ itanium2 icc
Having recently tried to build 7.4, and 8.0 branches on Itanium2 with ICC
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=dugongdt=2006-09-29%2015:59:09
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=dugongdt=2006-09-29%2015:59:43
I encountered the same error as people before:
http://archives.postgresql.org/pgsql-hackers/2005-03/msg00084.php
But the patch from this thread wasn't applied.
I don't know whether it's worth fixing that for old branches ... ,
but I send the patches...
Is it worth applying ?
Regards,
Sergey
***
Sergey E. Koposov
Max Planck Institute for Astronomy/Sternberg Astronomical Institute
Tel: +49-6221-528-349
Web: http://lnfm1.sai.msu.ru/~math
E-mail: [EMAIL PROTECTED]Index: s_lock.h
===
RCS file: /projects/cvsroot/pgsql/src/include/storage/s_lock.h,v
retrieving revision 1.133.4.5
diff -c -r1.133.4.5 s_lock.h
*** s_lock.h29 Aug 2005 00:41:44 - 1.133.4.5
--- s_lock.h3 Oct 2006 10:23:50 -
***
*** 176,181
--- 176,183
#define TAS(lock) tas(lock)
+ #ifndef __INTEL_COMPILER
+
static __inline__ int
tas(volatile slock_t *lock)
{
***
*** 189,194
--- 191,209
return (int) ret;
}
+ #else /* __INTEL_COMPILER */
+
+ static __inline__ int
+ tas(volatile slock_t *lock)
+ {
+ int ret;
+
+ ret = _InterlockedExchange(lock,1); /* this is a xchg asm macro */
+
+ return ret;
+ }
+
+ #endif /* __INTEL_COMPILER */
#endif /* __ia64__ || __ia64 */
Index: s_lock.h
===
RCS file: /projects/cvsroot/pgsql/src/include/storage/s_lock.h,v
retrieving revision 1.115.2.1
diff -c -r1.115.2.1 s_lock.h
*** s_lock.h4 Nov 2003 09:43:56 - 1.115.2.1
--- s_lock.h3 Oct 2006 10:38:28 -
***
*** 117,122
--- 117,124
#if defined(__ia64__) || defined(__ia64)
#define TAS(lock) tas(lock)
+ #ifndef __INTEL_COMPILER
+
static __inline__ int
tas(volatile slock_t *lock)
{
***
*** 131,136
--- 133,151
return (int) ret;
}
+ #else /* __INTEL_COMPILER */
+
+ static __inline__ int
+ tas(volatile slock_t *lock)
+ {
+ int ret;
+
+ ret = _InterlockedExchange(lock,1); /* this is a xchg asm macro */
+
+ return ret;
+ }
+
+ #endif /* __INTEL_COMPILER */
#endif /* __ia64__ || __ia64 */
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] Bad bug in fopen() wrapper code
Zeugswetter Andreas DCP SD [EMAIL PROTECTED] writes: If successful, _setmode returns the previous translation mode. A return value of -1 indicates an error So, shouldn't we be testing for -1 instead of 0 ? I think the usual convention is to test for 0, unless there are other negative return values that are legal. This is doubtless a silly cycle-shaving habit (on nearly all machines, test against 0 is a bit more compact than test against other constants), but it is a widespread habit anyway, and if you sometimes do it one way and sometimes another you just create a distraction for readers. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] 7.4, 8.0 branches @ itanium2 icc
Sergey E. Koposov [EMAIL PROTECTED] writes: Having recently tried to build 7.4, and 8.0 branches on Itanium2 with ICC 7.4 is not going to work with ICC anyway without considerably more extensive changes (eg, configure hacking). It might make sense to apply this patch to 8.0 but I can't get all that excited about it. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] 7.4, 8.0 branches @ itanium2 icc
On Tue, 3 Oct 2006, Tom Lane wrote: Sergey E. Koposov [EMAIL PROTECTED] writes: Having recently tried to build 7.4, and 8.0 branches on Itanium2 with ICC 7.4 is not going to work with ICC anyway without considerably more extensive changes (eg, configure hacking). It might make sense to apply this patch to 8.0 but I can't get all that excited about it. It worked for me with both 7.4 8.0 after applying my patch. (and make check worked ...) ( at least with icc 9.1.x) Regards, Sergey *** Sergey E. Koposov Max Planck Institute for Astronomy/Sternberg Astronomical Institute Tel: +49-6221-528-349 Web: http://lnfm1.sai.msu.ru/~math E-mail: [EMAIL PROTECTED] ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
[PATCHES] MSVC build broken (again)
The code around errcode is definitly messy. In CVS now, it actually renames *our* errcode() function to __msvc_errcode, and exports this from postgres.exe. This is definitly very borken. The check for _MSC_VER 1400 won't come true until Microsoft releases the next verison of Visual Studio - VS2005 is 1400, not 1400. Attached patch fixes this. Tested on MSVC and on Mingw. //Magnus vcbuild.diff Description: vcbuild.diff ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
[PATCHES] pgevent fixes
Two fixes: 1) Make vcbuild actually build the pgevent dll. 2) Change the pgevent DLL file so it doens't specify ordinal for the functions. You're not supposed to do that. You're actually supposed to declare them as PRIVATE as well, but mingw doesn't support that. VC++ will throw a warning and not an error though, so we can live with it. //Magnus pgevent.diff Description: pgevent.diff ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] guc units cleanup
Patch applied. Thanks. --- ITAGAKI Takahiro wrote: The attached patch changes units of the some default values in postgresql.conf. - shared_buffers = 32000kB = 32MB - temp_buffers = 8000kB = 8MB - wal_buffers = 8 = 64kB The code of initdb was a bit modified to write MB-unit values. Values greater than 8000kB are rounded out to MB. GUC_UNIT_XBLOCKS is added for wal_buffers. It is like GUC_UNIT_BLOCKS, but uses XLOG_BLCKSZ instead of BLCKSZ. Also, I cleaned up the test of GUC_UNIT_* flags in preparation to add more unit flags in less bits. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center [ Attachment, skipping... ] ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] minor improvements in messages
Patch applied. Thanks. --- Euler Taveira de Oliveira wrote: Hi, Attached is a patch to make some sentences consistent with similar ones. -- Euler Taveira de Oliveira http://www.timbira.com/ [ Attachment, skipping... ] ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] scripts/common.c minor memory leak
Andrew Dunstan wrote:
Martijn van Oosterhout wrote:
Just a minor thing. In yesno_prompt(), the value is resp is allocated
memory that is never freed.
File: src/bin/scripts/common.c
Line: 218
Not terribly important though, it's not used in critical utilities, but
it's used often.
Found by coverity.
It is surely not the only memory leak. We know there are some and in
most cases (like this) they aren't worth the trouble to clean up. If it
were used in psql or the backend I'd be worried, but it isn't, so I'm not.
I have applied the attached patch to fix this. One reason I think it is
good to fix this is because it illustrates poor use of simple_prompt(),
that might be copied by others.
--
Bruce Momjian [EMAIL PROTECTED]
EnterpriseDBhttp://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/bin/scripts/common.c
===
RCS file: /cvsroot/pgsql/src/bin/scripts/common.c,v
retrieving revision 1.22
diff -c -c -r1.22 common.c
*** src/bin/scripts/common.c 22 Sep 2006 19:51:14 - 1.22
--- src/bin/scripts/common.c 3 Oct 2006 21:40:52 -
***
*** 208,227
{
char prompt[256];
for (;;)
{
char *resp;
- /* translator: This is a question followed by the translated options for yes and no. */
- snprintf(prompt, sizeof(prompt), _(%s (%s/%s) ),
- _(question), _(PG_YESLETTER), _(PG_NOLETTER));
resp = simple_prompt(prompt, 1, true);
if (strcmp(resp, _(PG_YESLETTER)) == 0)
return true;
else if (strcmp(resp, _(PG_NOLETTER)) == 0)
return false;
printf(_(Please answer \%s\ or \%s\.\n),
_(PG_YESLETTER), _(PG_NOLETTER));
}
--- 208,235
{
char prompt[256];
+ /* translator: This is a question followed by the translated options for yes and no. */
+ snprintf(prompt, sizeof(prompt), _(%s (%s/%s) ),
+ _(question), _(PG_YESLETTER), _(PG_NOLETTER));
+
for (;;)
{
char *resp;
resp = simple_prompt(prompt, 1, true);
if (strcmp(resp, _(PG_YESLETTER)) == 0)
+ {
+ free(resp);
return true;
+ }
else if (strcmp(resp, _(PG_NOLETTER)) == 0)
+ {
+ free(resp);
return false;
+ }
+ free(resp);
printf(_(Please answer \%s\ or \%s\.\n),
_(PG_YESLETTER), _(PG_NOLETTER));
}
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] [PATCHES] Bad bug in fopen() wrapper code
I agree that this code is both wrong and unreadable (although in practice the _setmode will probably never fail, which is why our attention hasn't been drawn to it). Is someone going to submit a patch? I'm hesitant to change the code myself since I'm not in a position to test it. I can look at fixing that. Is it something we want to do for 8.2, or wait until 8.3? If there's a hidden bug, I guess 8.2? Magnus, is this the right fix? Claudio sent me some small updates to the patch; updated version attached. Tested, passes all regression tests on MingW. //Magnus ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] vcbuild bison check
Magnus Hagander [EMAIL PROTECTED] writes: Attached patch adds a version check for bison when running the vc++ build. Shouldn't it be looking for 2.1 as well? regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] MSVC build broken (again)
Magnus Hagander [EMAIL PROTECTED] writes: The code around errcode is definitly messy. In CVS now, it actually renames *our* errcode() function to __msvc_errcode, and exports this from postgres.exe. This is definitly very borken. Would it be possible to move the whole crtdefs.h block into win32.h? This would cause it to be included after stdio.h and friends, which maybe is too late, but taking it out of c.h would be a lot cleaner. regards, tom lane ---(end of broadcast)--- TIP 1: 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] vcbuild bison check
Attached patch adds a version check for bison when running the vc++ build. Shouldn't it be looking for 2.1 as well? 2.1 is the broken one. It seemd it was fixed in 2.2, but 2.2 isn't realeased for win32 from what I cna tell. //Magnus ---(end of broadcast)--- TIP 1: 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] MSVC build broken (again)
The code around errcode is definitly messy. In CVS now, it actually renames *our* errcode() function to __msvc_errcode, and exports this from postgres.exe. This is definitly very borken. Would it be possible to move the whole crtdefs.h block into win32.h? This would cause it to be included after stdio.h and friends, which maybe is too late, but taking it out of c.h would be a lot cleaner. Nope, it needs to go before stdio.h and friends, unfortunatly. //Magnus ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] MSVC build broken (again)
Magnus Hagander [EMAIL PROTECTED] writes: Would it be possible to move the whole crtdefs.h block into win32.h? Nope, it needs to go before stdio.h and friends, unfortunatly. OK, patch committed as-is then. The whole thing still looks awfully icky though, particularly the way pg_config_os.h is included in one place for WIN32 and a different place everywhere else. Would it make sense to split win32.h into two files, one that's included in the normal pg_config_os.h place and one included after the system includes? regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] vcbuild bison check
Magnus Hagander [EMAIL PROTECTED] writes: Attached patch adds a version check for bison when running the vc++ build. Shouldn't it be looking for 2.1 as well? 2.1 is the broken one. Exactly. So we should reject it. It seemd it was fixed in 2.2, but 2.2 isn't realeased for win32 from what I cna tell. What's that got to do with rejecting 2.1? regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] [PATCHES] Bad bug in fopen() wrapper code
Applied.
---
Bruce Momjian wrote:
Bruce Momjian wrote:
Magnus Hagander wrote:
Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags O_APPEND)) 0
||
(fileFlags (O_TEXT | O_BINARY) (_setmode(fd, fileFlags
(O_TEXT
| O_BINARY)) 0)))
Without having studied it closely, it might also highlight a bug
on
failure of the second clause -- if the _setmode fails, shouldn't
_close be called instead of CloseHandle, and -1 returned?
(CloseHandle would still be called on failure of the
_open_osfhandle,
obviously)
I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in a
position to test it.
I can look at fixing that. Is it something we want to do for 8.2, or
wait until 8.3? If there's a hidden bug, I guess 8.2?
Magnus, is this the right fix?
Claudio sent me some small updates to the patch; updated version
attached.
--
Bruce Momjian [EMAIL PROTECTED]
EnterpriseDBhttp://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
[ text/x-diff is unsupported, treating like TEXT/PLAIN ]
Index: src/port/open.c
===
RCS file: /cvsroot/pgsql/src/port/open.c,v
retrieving revision 1.15
diff -c -c -r1.15 open.c
*** src/port/open.c 24 Sep 2006 17:19:53 - 1.15
--- src/port/open.c 3 Oct 2006 01:16:43 -
***
*** 105,113
}
/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((long) h, fileFlags O_APPEND)) 0 ||
! (fileFlags (O_TEXT | O_BINARY) (_setmode(fd, fileFlags
(O_TEXT | O_BINARY)) 0)))
CloseHandle(h); /* will not affect errno */
return fd;
}
--- 105,119
}
/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((long) h, fileFlags O_APPEND)) 0)
CloseHandle(h); /* will not affect errno */
+ else if (fileFlags (O_TEXT | O_BINARY)
+ _setmode(fd, fileFlags (O_TEXT | O_BINARY)) 0)
+ {
+ _close(fd);
+ return -1;
+ }
+
return fd;
}
--
Bruce Momjian [EMAIL PROTECTED]
EnterpriseDBhttp://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Numeric overflow problem + patch
Patch applied. Thanks. --- David Fetter wrote: On Thu, Sep 28, 2006 at 11:16:56PM +0200, Martijn van Oosterhout wrote: On Thu, Sep 28, 2006 at 05:11:43PM -0400, Tom Lane wrote: David Fetter [EMAIL PROTECTED] writes: ! DETAIL: A field with precision 4, scale 4 must have an absolute value less than 1. [ becomes ] ! DETAIL: A field with precision 4, scale 4 must have an absolute value less than 1 - 5 * 10^-5. This strikes me as overly pedantic. The message needs to be clear, and the proposed change will just confuse people. I don't know if the code can detect the difference, but a message like: A field with precision 4, scale 4 must *round to* an absolute value less than 1 Since that more accurately describes the actual problem. Have a ncie day, Per your suggestion, how about this patch? Cheers, D -- David Fetter [EMAIL PROTECTED] http://fetter.org/ phone: +1 415 235 3778AIM: dfetter666 Skype: davidfetter Remember to vote! [ Attachment, skipping... ] ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] pgevent fixes
Patch applied. Thanks. --- Magnus Hagander wrote: Two fixes: 1) Make vcbuild actually build the pgevent dll. 2) Change the pgevent DLL file so it doens't specify ordinal for the functions. You're not supposed to do that. You're actually supposed to declare them as PRIVATE as well, but mingw doesn't support that. VC++ will throw a warning and not an error though, so we can live with it. //Magnus Content-Description: pgevent.diff [ Attachment, skipping... ] ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] 7.4, 8.0 branches @ itanium2 icc
On Tue, 3 Oct 2006, Tom Lane wrote: Sergey E. Koposov [EMAIL PROTECTED] writes: Having recently tried to build 7.4, and 8.0 branches on Itanium2 with ICC 7.4 is not going to work with ICC anyway without considerably more extensive changes (eg, configure hacking). It might make sense to apply this patch to 8.0 but I can't get all that excited about it. I guess that fix is just no-cost fix. I don't see any problems that may arise from it. So my personal opinion is that it should be applied... Regards, Sergey *** Sergey E. Koposov Max Planck Institute for Astronomy/Sternberg Astronomical Institute Tel: +49-6221-528-349 Web: http://lnfm1.sai.msu.ru/~math E-mail: [EMAIL PROTECTED] ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] putting CHECK_FOR_INTERRUPTS in qsort_comparetup()
Charles Duffy [EMAIL PROTECTED] writes: The patch puts a CHECK_FOR_INTERRUPTS in qsort_comparetup. Just to close out this thread: this is now done for 8.2, per discussion here: http://archives.postgresql.org/pgsql-hackers/2006-10/msg00144.php regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
