Re: [PATCHES] Reorganization of spinlock defines
Done. --- Peter Eisentraut wrote: > Bruce Momjian writes: > > > I can make the change, but --without seemed to more closely match > > because it was like readline where you didn't have it, and had to say so > > specifically. I don't see how we can say --disable because this is case > > were we clearly don't have spinlocks to enable, no? > > ./configure --help | grep --relevant-stuff > > Optional Features: > --disable-FEATURE do not include FEATURE (same as --enable-FEATURE=no) > --enable-FEATURE[=ARG] include FEATURE [ARG=yes] > > Optional Packages: > --with-PACKAGE[=ARG]use PACKAGE [ARG=yes] > --without-PACKAGE do not use PACKAGE (same as --with-PACKAGE=no) > > Readline is a package, spinlocks are a feature. > > -- > Peter Eisentraut [EMAIL PROTECTED] > > > ---(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 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Reorganization of spinlock defines
OK. You'da boss. :-) --- Peter Eisentraut wrote: > Bruce Momjian writes: > > > I can make the change, but --without seemed to more closely match > > because it was like readline where you didn't have it, and had to say so > > specifically. I don't see how we can say --disable because this is case > > were we clearly don't have spinlocks to enable, no? > > ./configure --help | grep --relevant-stuff > > Optional Features: > --disable-FEATURE do not include FEATURE (same as --enable-FEATURE=no) > --enable-FEATURE[=ARG] include FEATURE [ARG=yes] > > Optional Packages: > --with-PACKAGE[=ARG]use PACKAGE [ARG=yes] > --without-PACKAGE do not use PACKAGE (same as --with-PACKAGE=no) > > Readline is a package, spinlocks are a feature. > > -- > Peter Eisentraut [EMAIL PROTECTED] > -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Reorganization of spinlock defines
Peter Eisentraut wrote: > Bruce Momjian writes: > > > o adds a configure option --without-spinlocks to allow > > non-spinlock compiles > > --disable-spinlocks please. I can make the change, but --without seemed to more closely match because it was like readline where you didn't have it, and had to say so specifically. I don't see how we can say --disable because this is case were we clearly don't have spinlocks to enable, 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 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Reorganization of spinlock defines
Bruce Momjian writes: > I can make the change, but --without seemed to more closely match > because it was like readline where you didn't have it, and had to say so > specifically. I don't see how we can say --disable because this is case > were we clearly don't have spinlocks to enable, no? ./configure --help | grep --relevant-stuff Optional Features: --disable-FEATURE do not include FEATURE (same as --enable-FEATURE=no) --enable-FEATURE[=ARG] include FEATURE [ARG=yes] Optional Packages: --with-PACKAGE[=ARG]use PACKAGE [ARG=yes] --without-PACKAGE do not use PACKAGE (same as --with-PACKAGE=no) Readline is a package, spinlocks are a feature. -- Peter Eisentraut [EMAIL PROTECTED] ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] Reorganization of spinlock defines
Bruce Momjian wrote:
> Prompted by confusion over Itanium/Opterion, I have written a patch to
> improve the way we define spinlocks for platforms and cpu's. It
> basically decouples the OS from the CPU spinlock code. In almost all
> cases, the spinlock code cares only about the compiler and CPU, not the
> OS.
>
> The patch:
>
> o defines HAS_TEST_AND_SET inside each spinlock routine, not in
> platform-specific files
> o moves slock_t defines into the spinlock routines
> o remove NEED_{CPU}_TAS_ASM define because it is no longer needed
> o reports a compile error if spinlocks are not defined
> o adds a configure option --without-spinlocks to allow
> non-spinlock compiles
OK, this applied patch implements the last two items. The earlier items
will have to wait for 7.5.
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: configure
===
RCS file: /cvsroot/pgsql-server/configure,v
retrieving revision 1.295
diff -c -c -r1.295 configure
*** configure 7 Sep 2003 16:49:41 - 1.295
--- configure 12 Sep 2003 16:05:13 -
***
*** 869,874
--- 869,875
--with-rendezvous build with Rendezvous support
--with-openssl[=DIR]build with OpenSSL support [/usr/local/ssl]
--without-readline do not use Readline
+ --without-spinlocks do not use Spinlocks
--without-zlib do not use Zlib
--with-gnu-ld assume the C compiler uses GNU ld default=no
***
*** 3494,3499
--- 3495,3530
#
+ # Spinlocks
+ #
+
+
+
+ # Check whether --with-spinlocks or --without-spinlocks was given.
+ if test "${with_spinlocks+set}" = set; then
+ withval="$with_spinlocks"
+
+ case $withval in
+ yes)
+ :
+ ;;
+ no)
+ :
+ ;;
+ *)
+ { { echo "$as_me:$LINENO: error: no argument expected for --with-spinlocks
option" >&5
+ echo "$as_me: error: no argument expected for --with-spinlocks option" >&2;}
+{ (exit 1); exit 1; }; }
+ ;;
+ esac
+
+ else
+ with_spinlocks=yes
+
+ fi;
+
+
+ #
# Zlib
#
***
*** 3523,3529
fi;
-
#
# Elf
#
--- 3554,3559
***
*** 6060,6065
--- 6090,6108
{ (exit 1); exit 1; }; }
fi
+ fi
+
+ if test "$with_spinlocks" = yes; then
+
+ cat >>confdefs.h <<\_ACEOF
+ #define HAVE_SPINLOCKS 1
+ _ACEOF
+
+ else
+ { echo "$as_me:$LINENO: WARNING:
+ *** Not using spinlocks will cause poor performance." >&5
+ echo "$as_me: WARNING:
+ *** Not using spinlocks will cause poor performance." >&2;}
fi
if test "$with_krb4" = yes ; then
Index: configure.in
===
RCS file: /cvsroot/pgsql-server/configure.in,v
retrieving revision 1.286
diff -c -c -r1.286 configure.in
*** configure.in7 Sep 2003 16:38:05 - 1.286
--- configure.in12 Sep 2003 16:05:15 -
***
*** 522,533
[ --without-readline do not use Readline])
#
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
-
#
# Elf
#
--- 522,538
[ --without-readline do not use Readline])
#
+ # Spinlocks
+ #
+ PGAC_ARG_BOOL(with, spinlocks, yes,
+ [ --without-spinlocks do not use Spinlocks])
+
+ #
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
#
# Elf
#
***
*** 676,681
--- 681,693
If you have zlib already installed, see config.log for details on the
failure. It is possible the compiler isn't looking in the proper directory.
Use --without-zlib to disable zlib support.])])
+ fi
+
+ if test "$with_spinlocks" = yes; then
+ AC_DEFINE(HAVE_SPINLOCKS, 1, [Define to 1 if you have spinlocks.])
+ else
+ AC_MSG_WARN([
+ *** Not using spinlocks will cause poor performance.])
fi
if test "$with_krb4" = yes ; then
Index: doc/src/sgml/installation.sgml
===
RCS file: /cvsroot/pgsql-server/doc/src/sgml/installation.sgml,v
retrieving revision 1.141
diff -c -c -r1.141 installation.sgml
*** doc/src/sgml/installation.sgml 11 Sep 2003 21:42:20 - 1.141
--- doc/src/sgml/installation.sgml 12 Sep 2003 16:05:17 -
***
*** 900,905
--- 900,917
+--without-spinlocks
+
+
+ Allows source builds to succeed without CPU spinlock support.
+ Lack of spinlock support will produce poor performance.
+ This option is to be used only by platforms wi
Re: [PATCHES] Reorganization of spinlock defines
Bruce Momjian writes: > o adds a configure option --without-spinlocks to allow > non-spinlock compiles --disable-spinlocks please. -- Peter Eisentraut [EMAIL PROTECTED] ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
On Thu, 11 Sep 2003, Bruce Momjian wrote: > I just learned from Larry that Unixware defines intel as i386, not > __i386 or __i386__, at least of the native SCO compiler that he uses. could we put something in the various port files to standardize this? ie. in unixware.h, add somethinglike: #ifdef i386 #define __i386__ #endif just so that 'special cases' are centralized in the ports file, and the mainstream code doesn't have: #if defined(i386) || defined(__i386) || defined(__i386__) ? ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
--On Thursday, September 11, 2003 23:13:54 -0400 Tom Lane <[EMAIL PROTECTED]> wrote: Larry Rosenman <[EMAIL PROTECTED]> writes: Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all= =20 care). Unfixably? Or just a small oversight? I'm actually not worried about platforms that are actively being tested. It's the stuff that hasn't been confirmed recently that is going to be at risk. I'm seeing failures with the 2nd patch as well. Seems like it's not liking UnixWare's cc defines. the documentation is at: http://www.lerctr.org:8458/ the cc man page is at: http://www.lerctr.org:8458/en/man/html.1/cc.1.html Tom, You still have an account here. Bruce, if you'd like an account, that is easily arranged. LER regards, tom lane -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: [EMAIL PROTECTED] US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749 pgp0.pgp Description: PGP signature
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Larry Rosenman <[EMAIL PROTECTED]> writes: > > Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all= > > =20 > > care). > > Unfixably? Or just a small oversight? > > I'm actually not worried about platforms that are actively being tested. > It's the stuff that hasn't been confirmed recently that is going to be > at risk. I sent him a new patch just now. -- 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: [HACKERS] [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Larry Rosenman <[EMAIL PROTECTED]> writes: > > Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all= > > =20 > > care). > > Unfixably? Or just a small oversight? Updated patch now works on Unixware. -- 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: [HACKERS] [PATCHES] Reorganization of spinlock defines
Marc G. Fournier wrote: > > On Thu, 11 Sep 2003, Bruce Momjian wrote: > > > I just learned from Larry that Unixware defines intel as i386, not > > __i386 or __i386__, at least of the native SCO compiler that he uses. > > could we put something in the various port files to standardize this? ie. > in unixware.h, add somethinglike: > > #ifdef i386 > #define __i386__ > #endif > > just so that 'special cases' are centralized in the ports file, and the > mainstream code doesn't have: > > #if defined(i386) || defined(__i386) || defined(__i386__) Yep, that's what Tom wants and I am doing that now. I sent a patch to Larry for testing. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] Reorganization of spinlock defines
Bruce Momjian <[EMAIL PROTECTED]> writes: > The problem with waiting for 7.5 is that we will have no error reporting > when our non-spinlock code is being executed, and with Opteron/Itanium, > it seems like a good time to get it working. Well, as long as you're prepared to reduce the list of known supported platforms to zero as of 7.4beta3, and issue a fresh call for port reports. But it seems to me that this is mostly a cosmetic cleanup and therefore not the kind of thing to be doing late in beta. Couldn't we do something that affects only Opteron/Itanium and doesn't take a chance on breaking everything else? regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
--On Friday, September 12, 2003 00:00:43 -0400 Tom Lane <[EMAIL PROTECTED]> wrote: Larry Rosenman <[EMAIL PROTECTED]> writes: Please, only the first two. Make the Unixware template add __i386__. Don't add assumptions about valid user-namespace symbols. that's reasonable. At least until 64-bit UnixWare. :-) Even then, I'd prefer to put the necessary kluge into template/unixware or Makefile.unixware or port/unixware.h, rather than add a risky assumption globally. Sure, and 64-bit UnixWare is 2 years out any way. Hopefully we can get reasonableness by then. I've already sent a whine-a-gram to the compiler guys at SCO. LER regards, tom lane -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: [EMAIL PROTECTED] US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749 pgp0.pgp Description: PGP signature
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
--On Friday, September 12, 2003 00:06:49 -0400 Tom Lane <[EMAIL PROTECTED]> wrote: Larry Rosenman <[EMAIL PROTECTED]> writes: I've already sent a whine-a-gram to the compiler guys at SCO. Prolly you thought of this already, but: getting them to *add* an implicit #define of __i386__ should be plenty easy compared to getting them to *remove* the one for i386. And while I think they should remove the latter, the immediate problem would be solved as soon as they add the former. sure, and I expect that's what they may do. We'll see what they say. LER regards, tom lane -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: [EMAIL PROTECTED] US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749 pgp0.pgp Description: PGP signature
Re: [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > The problem with waiting for 7.5 is that we will have no error reporting > > when our non-spinlock code is being executed, and with Opteron/Itanium, > > it seems like a good time to get it working. > > Well, as long as you're prepared to reduce the list of known supported > platforms to zero as of 7.4beta3, and issue a fresh call for port reports. I haven't collected any known reports yet. That happens later, I thought, nearer to RC1. > But it seems to me that this is mostly a cosmetic cleanup and therefore > not the kind of thing to be doing late in beta. Couldn't we do > something that affects only Opteron/Itanium and doesn't take a chance > on breaking everything else? Yes, but to throw an error if spinlocks aren't found, we need this patch. We would have to test for Opteron in all the platforms that test for specific CPU's but don't test for opteron, and might support opterion/itanium, but even then, we don't have any way of getting a report of a failure. Yea, I should have thought of this before beta1, but now that I see the bug reports, seems we have to go this way. -- 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] Reorganization of spinlock defines
Bruce Momjian <[EMAIL PROTECTED]> writes: > I guess we could splatter a test for Itanium and Opterion in every port > that could possibly use it, but then again, if we fall back to not > finding it for some reason, we don't get a report because we silently > fall back to semaphores. That's what has me worried, that if we don't > do it, we will not know what platforms really aren't working properly. Agreed, the silent fallback to semaphores isn't such a hot idea in hindsight. But the part of the patch that requires a configure option to use that code path could be applied without touching anything else. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Reorganization of spinlock defines
New patch attached.
---
Bruce Momjian wrote:
> Prompted by confusion over Itanium/Opterion, I have written a patch to
> improve the way we define spinlocks for platforms and cpu's. It
> basically decouples the OS from the CPU spinlock code. In almost all
> cases, the spinlock code cares only about the compiler and CPU, not the
> OS.
>
> The patch:
>
> o defines HAS_TEST_AND_SET inside each spinlock routine, not in
> platform-specific files
> o moves slock_t defines into the spinlock routines
> o remove NEED_{CPU}_TAS_ASM define because it is no longer needed
> o reports a compile error if spinlocks are not defined
> o adds a configure option --without-spinlocks to allow
> non-spinlock compiles
>
> Looking at the patch, I realize this is how we should have done it all
> along.
>
> It would be nice to report the lack of spinlocks in configure, rather
> than during the compile, but I can't compile s_lock.h and test for
> HAS_TEST_AND_SET until configure completes.
>
> I plan to apply this to 7.4.
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: configure
===
RCS file: /cvsroot/pgsql-server/configure,v
retrieving revision 1.295
diff -c -c -r1.295 configure
*** configure 7 Sep 2003 16:49:41 - 1.295
--- configure 12 Sep 2003 04:38:16 -
***
*** 869,874
--- 869,875
--with-rendezvous build with Rendezvous support
--with-openssl[=DIR]build with OpenSSL support [/usr/local/ssl]
--without-readline do not use Readline
+ --without-spinlocks do not use Spinlocks
--without-zlib do not use Zlib
--with-gnu-ld assume the C compiler uses GNU ld default=no
***
*** 3494,3499
--- 3495,3530
#
+ # Spinlocks
+ #
+
+
+
+ # Check whether --with-spinlocks or --without-spinlocks was given.
+ if test "${with_spinlocks+set}" = set; then
+ withval="$with_spinlocks"
+
+ case $withval in
+ yes)
+ :
+ ;;
+ no)
+ :
+ ;;
+ *)
+ { { echo "$as_me:$LINENO: error: no argument expected for --with-spinlocks
option" >&5
+ echo "$as_me: error: no argument expected for --with-spinlocks option" >&2;}
+{ (exit 1); exit 1; }; }
+ ;;
+ esac
+
+ else
+ with_spinlocks=yes
+
+ fi;
+
+
+ #
# Zlib
#
***
*** 3523,3529
fi;
-
#
# Elf
#
--- 3554,3559
***
*** 6060,6065
--- 6090,6108
{ (exit 1); exit 1; }; }
fi
+ fi
+
+ if test "$with_spinlocks" = yes; then
+
+ cat >>confdefs.h <<\_ACEOF
+ #define HAVE_SPINLOCKS 1
+ _ACEOF
+
+ else
+ { echo "$as_me:$LINENO: WARNING:
+ *** Not using spinlocks will cause poor performance." >&5
+ echo "$as_me: WARNING:
+ *** Not using spinlocks will cause poor performance." >&2;}
fi
if test "$with_krb4" = yes ; then
Index: configure.in
===
RCS file: /cvsroot/pgsql-server/configure.in,v
retrieving revision 1.286
diff -c -c -r1.286 configure.in
*** configure.in7 Sep 2003 16:38:05 - 1.286
--- configure.in12 Sep 2003 04:38:18 -
***
*** 522,533
[ --without-readline do not use Readline])
#
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
-
#
# Elf
#
--- 522,538
[ --without-readline do not use Readline])
#
+ # Spinlocks
+ #
+ PGAC_ARG_BOOL(with, spinlocks, yes,
+ [ --without-spinlocks do not use Spinlocks])
+
+ #
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
#
# Elf
#
***
*** 676,681
--- 681,693
If you have zlib already installed, see config.log for details on the
failure. It is possible the compiler isn't looking in the proper directory.
Use --without-zlib to disable zlib support.])])
+ fi
+
+ if test "$with_spinlocks" = yes; then
+ AC_DEFINE(HAVE_SPINLOCKS, 1, [Define to 1 if you have spinlocks.])
+ else
+ AC_MSG_WARN([
+ *** Not using spinlocks will cause poor performance.])
fi
if test "$with_krb4" = yes ; then
Index: doc/src/sgml/installation.sgml
===
RCS file: /cvsroot/pgsql-server/doc/src/sgml/installation.sgml,v
retrieving revision 1.141
diff -c -c -r1.141 installation.sgml
*** doc/src/sgml/installation.sgml 11 Sep 2003 21:42:20 - 1.141
--- doc/src/sgml/installation.sgml 12 Sep 2003 04:38:21 -
*
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Marc G. Fournier wrote: > > > On Thu, 11 Sep 2003, Bruce Momjian wrote: > > > Well, the problem was that we defined HAS_TEST_AND_SET inside the ports. > > I guess we could splatter a test for Itanium and Opterion in every port > > that could possibly use it, but then again, if we fall back to not > > finding it for some reason, we don't get a report because we silently > > fall back to semaphores. That's what has me worried, that if we don't > > do it, we will not know what platforms really aren't working properly. > > >From what I understand, "not working properly" means slow, not broken, no? > Which means ppl could submit a problem report and it could be fixed for > v7.4.1 ... its not so much 'not working properly' as it is 'not optimal > performance' ... Right, though I am not sure people will know _slow_ configuration vs. PostgreSQL is slow. -- 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] Reorganization of spinlock defines
On Thu, 11 Sep 2003, Bruce Momjian wrote: > Well, the problem was that we defined HAS_TEST_AND_SET inside the ports. > I guess we could splatter a test for Itanium and Opterion in every port > that could possibly use it, but then again, if we fall back to not > finding it for some reason, we don't get a report because we silently > fall back to semaphores. That's what has me worried, that if we don't > do it, we will not know what platforms really aren't working properly. >From what I understand, "not working properly" means slow, not broken, no? Which means ppl could submit a problem report and it could be fixed for v7.4.1 ... its not so much 'not working properly' as it is 'not optimal performance' ... ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Larry Rosenman <[EMAIL PROTECTED]> writes: > I've already sent a whine-a-gram to the compiler guys at SCO. Prolly you thought of this already, but: getting them to *add* an implicit #define of __i386__ should be plenty easy compared to getting them to *remove* the one for i386. And while I think they should remove the latter, the immediate problem would be solved as soon as they add the former. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Larry Rosenman <[EMAIL PROTECTED]> writes: >> Please, only the first two. Make the Unixware template add __i386__. >> Don't add assumptions about valid user-namespace symbols. > that's reasonable. At least until 64-bit UnixWare. :-) Even then, I'd prefer to put the necessary kluge into template/unixware or Makefile.unixware or port/unixware.h, rather than add a risky assumption globally. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
--On Thursday, September 11, 2003 23:42:53 -0400 Tom Lane <[EMAIL PROTECTED]> wrote: Bruce Momjian <[EMAIL PROTECTED]> writes: Looking at the code, I wonder if we already have folks not using spinlocks, and not even knowing it. I don't think problem reports will be limited to new platforms. Very likely --- I heard from someone recently who was trying to run HPUX/Itanium. After we got past the hard-wired assumption that HPUX == HPPA, it was still giving lousy performance for lack of spinlocks. I like the part of the patch that is in-your-face about that. I just learned from Larry that Unixware defines intel as i386, not __i386 or __i386__, at least of the native SCO compiler that he uses. [blink] Namespace infringement 'r us, huh? Yeah. I **DO** have SCO's ear on it, but I don't know how far I'll get, plus there are TONS of pre-whenever-we-get-it-fixed out there. I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for consistency. It is only done in one place in the patch, so that should be good. Please, only the first two. Make the Unixware template add __i386__. Don't add assumptions about valid user-namespace symbols. that's reasonable. At least until 64-bit UnixWare. :-) (announced at SCOForum). regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: [EMAIL PROTECTED] US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749 pgp0.pgp Description: PGP signature
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Looking at the code, I wonder if we already have folks not using > > spinlocks, and not even knowing it. I don't think problem reports will > > be limited to new platforms. > > Very likely --- I heard from someone recently who was trying to run > HPUX/Itanium. After we got past the hard-wired assumption that HPUX > == HPPA, it was still giving lousy performance for lack of spinlocks. > I like the part of the patch that is in-your-face about that. > > > I just learned from Larry that Unixware defines intel as i386, not > > __i386 or __i386__, at least of the native SCO compiler that he uses. > > [blink] Namespace infringement 'r us, huh? > > > I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for > > consistency. It is only done in one place in the patch, so that should > > be good. > > Please, only the first two. Make the Unixware template add __i386__. > Don't add assumptions about valid user-namespace symbols. Roger! -- 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: [HACKERS] [PATCHES] Reorganization of spinlock defines
Bruce Momjian <[EMAIL PROTECTED]> writes: > Looking at the code, I wonder if we already have folks not using > spinlocks, and not even knowing it. I don't think problem reports will > be limited to new platforms. Very likely --- I heard from someone recently who was trying to run HPUX/Itanium. After we got past the hard-wired assumption that HPUX == HPPA, it was still giving lousy performance for lack of spinlocks. I like the part of the patch that is in-your-face about that. > I just learned from Larry that Unixware defines intel as i386, not > __i386 or __i386__, at least of the native SCO compiler that he uses. [blink] Namespace infringement 'r us, huh? > I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for > consistency. It is only done in one place in the patch, so that should > be good. Please, only the first two. Make the Unixware template add __i386__. Don't add assumptions about valid user-namespace symbols. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Yes, we could do just the configure warning, then plaster tests into the > > port files to try to hit all the opteron/itanium cases. I am a little > > concerned that this might throw up a bunch of problem cases that we will > > patching for a while. > > Probably so --- but we'd only be breaking new platforms that people are > starting to use, not old ones that might not be getting tested > regularly. Looking at the code, I wonder if we already have folks not using spinlocks, and not even knowing it. I don't think problem reports will be limited to new platforms. > Understand that I'm not dead set against applying this patch for 7.4. > (On a code-cleanliness point of view I favor it.) What I want is some > open discussion about the risks and benefits before we decide. Sure, and I am not pushing the patch. I am just saying it would have been ideal a few weeks ago --- I am not sure if we are worse off with or without it. I just learned from Larry that Unixware defines intel as i386, not __i386 or __i386__, at least of the native SCO compiler that he uses. What the code used to do is define NEED_I386_TAS_ASM unconditionally on some platforms (negating the need to test for a compiler symbol) or test for each platform compiler symbol (and not test all possible ways it could be specified), like FreeBSD did. That's why things are so messy. I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for consistency. It is only done in one place in the patch, so that should be good. -- 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: [HACKERS] [PATCHES] Reorganization of spinlock defines
Bruce Momjian <[EMAIL PROTECTED]> writes: > Yes, we could do just the configure warning, then plaster tests into the > port files to try to hit all the opteron/itanium cases. I am a little > concerned that this might throw up a bunch of problem cases that we will > patching for a while. Probably so --- but we'd only be breaking new platforms that people are starting to use, not old ones that might not be getting tested regularly. Understand that I'm not dead set against applying this patch for 7.4. (On a code-cleanliness point of view I favor it.) What I want is some open discussion about the risks and benefits before we decide. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > I guess we could splatter a test for Itanium and Opterion in every port > > that could possibly use it, but then again, if we fall back to not > > finding it for some reason, we don't get a report because we silently > > fall back to semaphores. That's what has me worried, that if we don't > > do it, we will not know what platforms really aren't working properly. > > Agreed, the silent fallback to semaphores isn't such a hot idea in > hindsight. But the part of the patch that requires a configure option > to use that code path could be applied without touching anything else. Yes, we could do just the configure warning, then plaster tests into the port files to try to hit all the opteron/itanium cases. I am a little concerned that this might throw up a bunch of problem cases that we will patching for a while. -- 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: [HACKERS] [PATCHES] Reorganization of spinlock defines
Larry Rosenman <[EMAIL PROTECTED]> writes: > Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all= > =20 > care). Unfixably? Or just a small oversight? I'm actually not worried about platforms that are actively being tested. It's the stuff that hasn't been confirmed recently that is going to be at risk. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Reorganization of spinlock defines
"Marc G. Fournier" <[EMAIL PROTECTED]> writes: > On Thu, 11 Sep 2003, Tom Lane wrote: >> Well, as long as you're prepared to reduce the list of known supported >> platforms to zero as of 7.4beta3, and issue a fresh call for port reports. > I didn't think we had done that yet ... had we? called for port reports, > that is ... ? We hadn't, no. My point is that in the past we've continued to list platforms as supported if we've had a successful report in the past release or two. Fooling with the spinlock code is delicate enough that I'd want to insist on moving everything to the "unsupported" category until we get a success report with the modified code. Maybe we should just do that. It's likely that the only platforms that end up marked unsupported are ones that no one cares about any more anyway. But I think we have to realize that this is not a trivial set of changes, even if it looks like it "should work". (Which it does, just for the record. I'm just feeling paranoid because of where we are in the release cycle.) regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Reorganization of spinlock defines
--On Thursday, September 11, 2003 23:46:56 -0300 "Marc G. Fournier" <[EMAIL PROTECTED]> wrote: On Thu, 11 Sep 2003, Tom Lane wrote: Bruce Momjian <[EMAIL PROTECTED]> writes: > The problem with waiting for 7.5 is that we will have no error > reporting when our non-spinlock code is being executed, and with > Opteron/Itanium, it seems like a good time to get it working. Well, as long as you're prepared to reduce the list of known supported platforms to zero as of 7.4beta3, and issue a fresh call for port reports. I didn't think we had done that yet ... had we? called for port reports, that is ... ? But it seems to me that this is mostly a cosmetic cleanup and therefore not the kind of thing to be doing late in beta. Couldn't we do something that affects only Opteron/Itanium and doesn't take a chance on breaking everything else? I just went through the whole patch myself, and as much as I like the overall simplification, I tend to agree with Tom here on questioning the requirement to do suck a massive change so late in the end cycle ... is there no smaller bandaid that can be applied to handle the Opteron/Itanium issue for v7.4, with the "cleanup patch" being applied right away after v7.4? Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all care). LER -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: [EMAIL PROTECTED] US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749 pgp0.pgp Description: PGP signature
Re: [PATCHES] Reorganization of spinlock defines
On Thu, 11 Sep 2003, Bruce Momjian wrote: > Yes, but to throw an error if spinlocks aren't found, we need this > patch. We would have to test for Opteron in all the platforms that test > for specific CPU's but don't test for opteron, and might support > opterion/itanium, but even then, we don't have any way of getting a > report of a failure. 'K, but apparently right now we are broken on Opteron/Itanium without this patch ... so, to fix, we either: a. add appropriate tests to the individual port files based on individual failure reports (albeit not clean, definitely safer), or: b. we do massive, sweeping changes to the whole HAVE_TEST_AND_SET detection code (definitely cleaner, but has potential of breaking more then it fixes) :( personally, as late in the cycle as we are, I think that a. is the wiser move for v7.4, with b. being something that should happen as soon as possible once we've branched and start working on v7.5 ... ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Reorganization of spinlock defines
Marc G. Fournier wrote: > > But it seems to me that this is mostly a cosmetic cleanup and therefore > > not the kind of thing to be doing late in beta. Couldn't we do > > something that affects only Opteron/Itanium and doesn't take a chance > > on breaking everything else? > > I just went through the whole patch myself, and as much as I like the > overall simplification, I tend to agree with Tom here on questioning the > requirement to do suck a massive change so late in the end cycle ... is > there no smaller bandaid that can be applied to handle the Opteron/Itanium > issue for v7.4, with the "cleanup patch" being applied right away after > v7.4? Well, the problem was that we defined HAS_TEST_AND_SET inside the ports. I guess we could splatter a test for Itanium and Opterion in every port that could possibly use it, but then again, if we fall back to not finding it for some reason, we don't get a report because we silently fall back to semaphores. That's what has me worried, that if we don't do it, we will not know what platforms really aren't working properly. Take FreeBSD for example, that couldn't find Opteron. It lists every cpu like this: #if defined(__i386__) #define NEED_I386_TAS_ASM #define HAS_TEST_AND_SET typedef unsigned char slock_t; #endif #if defined(__sparc__) #define NEED_SPARC_TAS_ASM #define HAS_TEST_AND_SET typedef unsigned char slock_t; #endif We would have to add an opteron/itanium to port that does this, but if we miss some opteron/itanium define, we might never know because of the silent fallback. I don't care if we save it for 7.5 --- I just don't know how we will be sure we have things working properly without it. We could apply it tomorrow and see how things look on Monday. -- 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] Reorganization of spinlock defines
On Thu, 11 Sep 2003, Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > The problem with waiting for 7.5 is that we will have no error reporting > > when our non-spinlock code is being executed, and with Opteron/Itanium, > > it seems like a good time to get it working. > > Well, as long as you're prepared to reduce the list of known supported > platforms to zero as of 7.4beta3, and issue a fresh call for port reports. I didn't think we had done that yet ... had we? called for port reports, that is ... ? > But it seems to me that this is mostly a cosmetic cleanup and therefore > not the kind of thing to be doing late in beta. Couldn't we do > something that affects only Opteron/Itanium and doesn't take a chance > on breaking everything else? I just went through the whole patch myself, and as much as I like the overall simplification, I tend to agree with Tom here on questioning the requirement to do suck a massive change so late in the end cycle ... is there no smaller bandaid that can be applied to handle the Opteron/Itanium issue for v7.4, with the "cleanup patch" being applied right away after v7.4? ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Prompted by confusion over Itanium/Opterion, I have written a patch to > > improve the way we define spinlocks for platforms and cpu's. > > The main.c part of the patch strikes me as irrelevant to the claimed > purpose and unlikely to accomplish anything except breaking things. > Do you have a system the main.c "__alpha" code is relevant to, on which > to test that you did not break it? > > Other than that, it looks like basically a good idea. But... I was going to have an alpha guy test it --- that was the one change I wasn't sure about. We did test for __alpha__ all over the port/*.h files, so it wasn't clear which alpha's were being hit. I can throw in a comment and skip that part --- not sure. > > I plan to apply this to 7.4. > > This seems like living dangerously. You do realize that this patch will > invalidate our trust that the code works on every single supported > platform? I think beta3 may be a bit late in the game for this sort of > thing, because we've already gotten a good bit of the testing we can > expect to get for lesser-used platforms during this beta cycle. > > At the very least I'd like to see the decision discussed on -hackers > and not buried in -patches. The problem with waiting for 7.5 is that we will have no error reporting when our non-spinlock code is being executed, and with Opteron/Itanium, it seems like a good time to get it working. We had the FreeBSD report with not finding Opteron/Itanium, and that's what got me going. Also, if it doesn't find the spinlock code, it will report an error, so we should flesh this all out as we collect supported platforms, which we haven't started yet. -- 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] Reorganization of spinlock defines
Bruce Momjian <[EMAIL PROTECTED]> writes: > Prompted by confusion over Itanium/Opterion, I have written a patch to > improve the way we define spinlocks for platforms and cpu's. The main.c part of the patch strikes me as irrelevant to the claimed purpose and unlikely to accomplish anything except breaking things. Do you have a system the main.c "__alpha" code is relevant to, on which to test that you did not break it? Other than that, it looks like basically a good idea. But... > I plan to apply this to 7.4. This seems like living dangerously. You do realize that this patch will invalidate our trust that the code works on every single supported platform? I think beta3 may be a bit late in the game for this sort of thing, because we've already gotten a good bit of the testing we can expect to get for lesser-used platforms during this beta cycle. At the very least I'd like to see the decision discussed on -hackers and not buried in -patches. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
[PATCHES] Reorganization of spinlock defines
Prompted by confusion over Itanium/Opterion, I have written a patch to
improve the way we define spinlocks for platforms and cpu's. It
basically decouples the OS from the CPU spinlock code. In almost all
cases, the spinlock code cares only about the compiler and CPU, not the
OS.
The patch:
o defines HAS_TEST_AND_SET inside each spinlock routine, not in
platform-specific files
o moves slock_t defines into the spinlock routines
o remove NEED_{CPU}_TAS_ASM define because it is no longer needed
o reports a compile error if spinlocks are not defined
o adds a configure option --without-spinlocks to allow
non-spinlock compiles
Looking at the patch, I realize this is how we should have done it all
along.
It would be nice to report the lack of spinlocks in configure, rather
than during the compile, but I can't compile s_lock.h and test for
HAS_TEST_AND_SET until configure completes.
I plan to apply this to 7.4.
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: configure
===
RCS file: /cvsroot/pgsql-server/configure,v
retrieving revision 1.295
diff -c -c -r1.295 configure
*** configure 7 Sep 2003 16:49:41 - 1.295
--- configure 12 Sep 2003 01:36:28 -
***
*** 869,874
--- 869,875
--with-rendezvous build with Rendezvous support
--with-openssl[=DIR]build with OpenSSL support [/usr/local/ssl]
--without-readline do not use Readline
+ --without-spinlocks do not use Spinlocks
--without-zlib do not use Zlib
--with-gnu-ld assume the C compiler uses GNU ld default=no
***
*** 3494,3499
--- 3495,3530
#
+ # Spinlocks
+ #
+
+
+
+ # Check whether --with-spinlocks or --without-spinlocks was given.
+ if test "${with_spinlocks+set}" = set; then
+ withval="$with_spinlocks"
+
+ case $withval in
+ yes)
+ :
+ ;;
+ no)
+ :
+ ;;
+ *)
+ { { echo "$as_me:$LINENO: error: no argument expected for --with-spinlocks
option" >&5
+ echo "$as_me: error: no argument expected for --with-spinlocks option" >&2;}
+{ (exit 1); exit 1; }; }
+ ;;
+ esac
+
+ else
+ with_spinlocks=yes
+
+ fi;
+
+
+ #
# Zlib
#
***
*** 3523,3529
fi;
-
#
# Elf
#
--- 3554,3559
***
*** 6060,6065
--- 6090,6108
{ (exit 1); exit 1; }; }
fi
+ fi
+
+ if test "$with_spinlocks" = yes; then
+
+ cat >>confdefs.h <<\_ACEOF
+ #define HAVE_SPINLOCKS 1
+ _ACEOF
+
+ else
+ { echo "$as_me:$LINENO: WARNING:
+ *** Not using spinlocks will cause poor performance." >&5
+ echo "$as_me: WARNING:
+ *** Not using spinlocks will cause poor performance." >&2;}
fi
if test "$with_krb4" = yes ; then
Index: configure.in
===
RCS file: /cvsroot/pgsql-server/configure.in,v
retrieving revision 1.286
diff -c -c -r1.286 configure.in
*** configure.in7 Sep 2003 16:38:05 - 1.286
--- configure.in12 Sep 2003 01:36:31 -
***
*** 522,533
[ --without-readline do not use Readline])
#
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
-
#
# Elf
#
--- 522,538
[ --without-readline do not use Readline])
#
+ # Spinlocks
+ #
+ PGAC_ARG_BOOL(with, spinlocks, yes,
+ [ --without-spinlocks do not use Spinlocks])
+
+ #
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
#
# Elf
#
***
*** 676,681
--- 681,693
If you have zlib already installed, see config.log for details on the
failure. It is possible the compiler isn't looking in the proper directory.
Use --without-zlib to disable zlib support.])])
+ fi
+
+ if test "$with_spinlocks" = yes; then
+ AC_DEFINE(HAVE_SPINLOCKS, 1, [Define to 1 if you have spinlocks.])
+ else
+ AC_MSG_WARN([
+ *** Not using spinlocks will cause poor performance.])
fi
if test "$with_krb4" = yes ; then
Index: doc/src/sgml/installation.sgml
===
RCS file: /cvsroot/pgsql-server/doc/src/sgml/installation.sgml,v
retrieving revision 1.141
diff -c -c -r1.141 installation.sgml
*** doc/src/sgml/installation.sgml 11 Sep 2003 21:42:20 - 1.141
--- doc/src/sgml/installation.sgml 12 Sep 2003 01:36:34 -
***
*** 900,905
--- 900,915
+--without-spinlocks
+
+
+ Allows source builds to succeed
