Re: [PATCHES] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
Gregory Stark [EMAIL PROTECTED] writes: 4) Your problems with tsearch and timestamp etc raise an interesting problem. We don't need to mark this in pg_control because it's a purely a run-time issue and doesn't affect on-disk storage. Just for the record, that's wrong. It's true that on-disk data isn't affected, but the system catalog contents are, and they'd better match what the postgres binary is going to do. However it does affect ABI compatibility with modules. Perhaps it should be added to PG_MODULE_MAGIC_DATA. Very good point, especially now that it's configurable. Included in committed patch. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
I haven't had time to look through the patch, but reading Gregs comments I noted: 2) The genbki.sh change could be a bit tricky for multi-platform builds (ie OSX). I don't really see an alternative so it's just something to note for the folks setting that up (Hi Dave). Changes to genbki.sh also have to be mirrored in the msvc build scripts (Genbki.pm) in most cases... //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
Zoltan Boszormenyi [EMAIL PROTECTED] writes: - the int8inc(), int2_sum() and int4_sum() used pointers directly from the Datums for performance, that code path is now commented out, the other code path is correct for the AggState and !AggState runs and correct every time and now because of the passbyval nature of int8, the !AggState version is not slower than using the pointer directly. Does this mean count() and sum() are slower on a 32-bit machine? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training! - Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
Hi, Gregory Stark írta: Zoltan Boszormenyi [EMAIL PROTECTED] writes: - the int8inc(), int2_sum() and int4_sum() used pointers directly from the Datums for performance, that code path is now commented out, the other code path is correct for the AggState and !AggState runs and correct every time and now because of the passbyval nature of int8, the !AggState version is not slower than using the pointer directly. Does this mean count() and sum() are slower on a 32-bit machine? If you mean slower than on a 64-bit machine then yes. If you mean slower than before, then no. I didn't express myself correctly. The original code path is not commented out, it is just conditionally compiled. BTW I found the tsearch bug, it was a missing conversion of float4 in gistproc.c, it was an unfortunate detail that this didn't cause a segfault, it woul have been easier to find. Now there are no failing regression tests. -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH http://www.postgresql.at/ pg84-passedbyval-v2.patch.gz Description: Unix tar archive -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
Ok, ignore my previous message. I've read the patch now and that's not an issue. The old code path is not commented out, it's #ifdef'd conditionally on HAVE_LONG_INT_64 is right (well it seems right, it's a bit hard to tell in patch form). A few comments: 1) Please don't include configure in your patch. I don't know why it's checked into CVS but it is so that means manually removing it from any patch. It's usually a huge portion of the diff so it's worth removing. 2) The genbki.sh change could be a bit tricky for multi-platform builds (ie OSX). I don't really see an alternative so it's just something to note for the folks setting that up (Hi Dave). Actually there is an alternative but I prefer the approach you've taken. The alternative would be to have a special value in the catalogs for 8-bit maybe-pass-by-value data types and handle the check at run-time. Another alternative would be to have initdb fix up these values in C code instead of fixing them directly in the bki scripts. That seems like more hassle than it's worth though and a bigger break with the rest. 3) You could get rid of a bunch of #ifndef HAVE_LONG_INT_64 snippets by having a #define like INT64PASSBYVALUE which is defined to be either true or false. It might start getting confusing having three different defines for the same thing though. But personally I hate having more #ifdefs than necessary, it makes it hard to read the code. 4) Your problems with tsearch and timestamp etc raise an interesting problem. We don't need to mark this in pg_control because it's a purely a run-time issue and doesn't affect on-disk storage. However it does affect ABI compatibility with modules. Perhaps it should be added to PG_MODULE_MAGIC_DATA. Actually, why isn't sizeof(Datum) in there already? Do we have any protection against loading 64-bit compiled modules in a 32-bit server or vice versa? But generally this is something I've been wanting to do for a while and basically the same approach I would have taken. It seems sound to me. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1
Zoltan Boszormenyi írta: BTW I found the tsearch bug, it was a missing conversion of float4 in gistproc.c, it was an unfortunate detail that this didn't cause a segfault, it woul have been easier to find. Now there are no failing regression tests. Disregards this patch, the bugfix for tsearch is not real. -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH http://www.postgresql.at/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
