Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Started by Boszormenyi Zoltanover 18 years ago51 messagespatches
Jump to latest
#1Boszormenyi Zoltan
zb@cybertec.at

Hi,

I am working on this TODO item:

* Consider allowing 64-bit integers and floats to be passed by value on
64-bit platforms

Also change 32-bit floats (float4) to be passed by value at the same
time.

For genbki.sh, to correctly determine whether HAVE_LONG_INT_64
is defined, the attached bugfix is needed in the configure.in machinery.
This way the pg_config.h actually conforms to the comments for
HAVE_LONG_INT_64 and HAVE_LONG_LONG_INT_64.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

fix-64-bit-long-int-defines.patchtext/x-patch; charset=iso-8859-1; name=fix-64-bit-long-int-defines.patchDownload+6-6
#2Boszormenyi Zoltan
zb@cybertec.at
In reply to: Boszormenyi Zoltan (#1)
int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Hi,

I got around to (almost) finalize the patch. What it does:
- fixes the configure define described in my previous mail
- when the value of HAVE_LONG_INT_64 == 1, the following types
are passed by value: int8, float8, time, timestamp, timestamptz
The time[stamp[tz]] types caused segfaults in the regression test
if their attbyval setting was different from int8/float8, so it's
really needed.
I am not sure there's another type that needs a similat switch, the
type regression
tests are running fine.
- In the HAVE_LONG_INT_64 == 1 case, Int64GetDatum() becomes a
casting macro instead of a function, some others become functions.
The implementation of DatumGetFloat4()/Float4GetDatum()/etc functions may
change into an inline or change internals.
- float4 is now unconditionally passed by value
- 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.
- removed deprecated DatumGetFloat32/Float32GetDatum/etc macros, they aren't
used anywhere in the core and contrib source.

There is only one regression, in the tsearch tests. Some selects in
tsearch now
return no records but they don't segfault. I couldn't hunt the bug down yet,
not sure I will have time in the near future for that.

Comments welcome.

Best regards,
Zoltán Böszörményi

Zoltan Boszormenyi írta:

Hi,

I am working on this TODO item:

* Consider allowing 64-bit integers and floats to be passed by value on
64-bit platforms

Also change 32-bit floats (float4) to be passed by value at the same
time.

For genbki.sh, to correctly determine whether HAVE_LONG_INT_64
is defined, the attached bugfix is needed in the configure.in machinery.
This way the pg_config.h actually conforms to the comments for
HAVE_LONG_INT_64 and HAVE_LONG_LONG_INT_64.

Best regards,
Zoltán Böszörményi

------------------------------------------------------------------------

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

pg84-passedbyval.patch.gzapplication/x-tar; name=pg84-passedbyval.patch.gzDownload
#3Bruce Momjian
bruce@momjian.us
In reply to: Boszormenyi Zoltan (#2)
Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

"Zoltan Boszormenyi" <zb@cybertec.at> 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!

#4Boszormenyi Zoltan
zb@cybertec.at
In reply to: Bruce Momjian (#3)
Re: 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" <zb@cybertec.at> 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/

Attachments:

pg84-passedbyval-v2.patch.gzapplication/x-tar; name=pg84-passedbyval-v2.patch.gzDownload
#5Bruce Momjian
bruce@momjian.us
In reply to: Boszormenyi Zoltan (#2)
Re: 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!

#6Boszormenyi Zoltan
zb@cybertec.at
In reply to: Bruce Momjian (#5)
Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Gregory Stark írta:

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.

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).

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.

OK, this would also make the patch smaller.
Is pg_config_manual.h good for this setting?
Or which header would you suggest?

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.

I am looking into it.

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?

You can't mix 32-bit executables with 64-bit shared libraries, well,
without tricks.
I don't see any problem here.

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.

Thanks for commenting and encouragement.

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

#7Boszormenyi Zoltan
zb@cybertec.at
In reply to: Boszormenyi Zoltan (#6)
Re: Re: 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:

Gregory Stark írta:

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.

I am looking into it.

Do you think it's actually needed?
PG_MODULE_MAGIC_DATA contains the server version
the external module was compiled for. This patch won't go to
older versions, so it's already protected from the unconditional
float4 change. And because you can't mix server and libraries
with different bitsize, it's protected from the conditional int64,
float8, etc. changes.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

#8Boszormenyi Zoltan
zb@cybertec.at
In reply to: Boszormenyi Zoltan (#4)
Re: 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/

#9Bruce Momjian
bruce@momjian.us
In reply to: Boszormenyi Zoltan (#7)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

"Zoltan Boszormenyi" <zb@cybertec.at> writes:

Zoltan Boszormenyi írta:

Gregory Stark írta:

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.

I am looking into it.

Do you think it's actually needed?
PG_MODULE_MAGIC_DATA contains the server version
the external module was compiled for. This patch won't go to
older versions, so it's already protected from the unconditional
float4 change. And because you can't mix server and libraries
with different bitsize, it's protected from the conditional int64,
float8, etc. changes.

Right, it does seem like we're conservative about adding things to
PG_MODULE_MAGIC. As long as this is hard coded based on HAVE_LONG_INT_64 then
we don't strictly need it. And I don't see much reason to make this something
the user can override.

If there are modules which use the wrong macros and assume certain other data
types are pass-by-reference when they're not then they're going to fail
regardless of what version of postgres they're compiled against anyways.

So I would say in response to your other query to _not_ use pg_config_manual.h
which is intended for variables which the user can override. If you put
anything there then we would have to worry about incompatibilities.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

#10Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Boszormenyi Zoltan (#1)
Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Zoltan Boszormenyi wrote:

For genbki.sh, to correctly determine whether HAVE_LONG_INT_64
is defined, the attached bugfix is needed in the configure.in machinery.
This way the pg_config.h actually conforms to the comments for
HAVE_LONG_INT_64 and HAVE_LONG_LONG_INT_64.

I think this part of the patch can be committed right away.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

#11Boszormenyi Zoltan
zb@cybertec.at
In reply to: Bruce Momjian (#9)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Gregory Stark írta:

"Zoltan Boszormenyi" <zb@cybertec.at> writes:

Zoltan Boszormenyi írta:

Gregory Stark írta:

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.

I am looking into it.

Do you think it's actually needed?
PG_MODULE_MAGIC_DATA contains the server version
the external module was compiled for. This patch won't go to
older versions, so it's already protected from the unconditional
float4 change. And because you can't mix server and libraries
with different bitsize, it's protected from the conditional int64,
float8, etc. changes.

Right, it does seem like we're conservative about adding things to
PG_MODULE_MAGIC. As long as this is hard coded based on HAVE_LONG_INT_64 then
we don't strictly need it. And I don't see much reason to make this something
the user can override.

If there are modules which use the wrong macros and assume certain other data
types are pass-by-reference when they're not then they're going to fail
regardless of what version of postgres they're compiled against anyways.

So I would say in response to your other query to _not_ use pg_config_manual.h
which is intended for variables which the user can override. If you put
anything there then we would have to worry about incompatibilities

OK, third version, the #define INT64PASSBYVAL is used now
for less #ifdef'd code, I used postgres.h for the defines for now.

As for the tsearch problem, it seems that bth tsearch and gist in general
uses the "internal" type for passing pointers to different datatypes around
for modifying them in-place, float4 among them. So, somewhere tsearch
or gist uses hardcoded passed-by-ref where it really a float4, not
"internal".
Someone with more knowledge about tsearch could look into this...

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

pg84-passedbyval-v3.patch.gzapplication/x-tar; name=pg84-passedbyval-v3.patch.gzDownload
#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Boszormenyi Zoltan (#6)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Zoltan Boszormenyi <zb@cybertec.at> writes:

Gregory Stark �rta:

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.

Noted.

Just for the record: the reason configure is in CVS is to avoid
requiring users of CVS checkouts to have autoconf installed.

Perhaps we should rethink that, but in any case there's no point
in submitting manual diffs to configure (or any other generated
file). Best practice is to just remind the committer that the
generated file needs to be regenerated.

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".

OK, this would also make the patch smaller.
Is pg_config_manual.h good for this setting?

I'd go for having a #define like that, but what is the reason to set it
in pg_config_manual.h? Seems like the configure script should set it.

regards, tom lane

#13Magnus Hagander
magnus@hagander.net
In reply to: Bruce Momjian (#5)
Re: 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

#14Boszormenyi Zoltan
zb@cybertec.at
In reply to: Tom Lane (#12)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Tom Lane wrote:

Zoltan Boszormenyi <zb@cybertec.at> writes:

Gregory Stark írta:

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.

Noted.

Just for the record: the reason configure is in CVS is to avoid
requiring users of CVS checkouts to have autoconf installed.

Perhaps we should rethink that, but in any case there's no point
in submitting manual diffs to configure (or any other generated
file). Best practice is to just remind the committer that the
generated file needs to be regenerated.

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".

OK, this would also make the patch smaller.
Is pg_config_manual.h good for this setting?

I'd go for having a #define like that, but what is the reason to set it
in pg_config_manual.h? Seems like the configure script should set it.

Obviously. :-) Thanks.

And:

Magnus Hagander wrote:

Changes to genbki.sh also have to be "mirrored" in the msvc build
scripts (Genbki.pm) in most cases...

//Magnus

Thanks for the info, I modified this file as well.
Please review the change, I am not a Perl expert and
I don't have a Windows build environment.

New patch is attached.

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

pg84-passedbyval-v4.patch.gzapplication/x-tar; name=pg84-passedbyval-v4.patch.gzDownload
#15Boszormenyi Zoltan
zb@cybertec.at
In reply to: Tom Lane (#12)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Tom Lane wrote:

Zoltan Boszormenyi <zb@cybertec.at> writes:

Gregory Stark írta:

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.

Noted.

Just for the record: the reason configure is in CVS is to avoid
requiring users of CVS checkouts to have autoconf installed.

Perhaps we should rethink that, but in any case there's no point
in submitting manual diffs to configure (or any other generated
file). Best practice is to just remind the committer that the
generated file needs to be regenerated.

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".

OK, this would also make the patch smaller.
Is pg_config_manual.h good for this setting?

I'd go for having a #define like that, but what is the reason to set it
in pg_config_manual.h? Seems like the configure script should set it.

Obviously. :-) Thanks.

And:

Magnus Hagander wrote:

Changes to genbki.sh also have to be "mirrored" in the msvc build
scripts (Genbki.pm) in most cases...

//Magnus

Thanks for the info, I modified this file as well.
Please review the change, I am not a Perl expert and
I don't have a Windows build environment.

New patch is attached.

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

pg84-passedbyval-v4.patch.gzapplication/x-tar; name=pg84-passedbyval-v4.patch.gzDownload
#16Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Boszormenyi Zoltan (#15)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

I don't think
my $int64passbyval = "(?($real64 = 1)t|f)";

works. Perhaps

my $int64passbyval = $real64 ? 't' : 'f';

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Attachments:

pg84-passedbyval-v4.patch.gzapplication/x-tar; name=pg84-passedbyval-v4.patch.gzDownload
#17Boszormenyi Zoltan
zb@cybertec.at
In reply to: Alvaro Herrera (#16)
Re: Re: int8/float8/time/timestamp[tz]/float4 passed by value, was Re: Fix HAVE_LONG[_LONG]_INT_64 to really define to 1

Alvaro Herrera írta:

I don't think
my $int64passbyval = "(?($real64 = 1)t|f)";

works. Perhaps

my $int64passbyval = $real64 ? 't' : 'f';

Thanks. Modified patch attached.

Stupid question follows. Now that float4 is passed by value
unconditionally, is it worth modifying the *penalty() functions
in GIST/TSearch to just use PG_RETURN_FLOAT4()?
Or the implicit "modify the float4 value at the caller site and
return the same pointer I got as 3rd parameter" is an internal API
set in stone? Modifying them to have only 2 parameters
(the 3rd one was an implicit OUT parameter anyway) and
omitting the pointer dereference might give a small speedup.

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

pg84-passedbyval-v5.patch.gzapplication/x-tar; name=pg84-passedbyval-v5.patch.gzDownload
#18Boszormenyi Zoltan
zb@cybertec.at
In reply to: Boszormenyi Zoltan (#17)
float4/float8/int64 passed by value with tsearch fixup

Hi,

I tried to split the previous patch up to see where the tsearch regression
comes from. So, it turned out that:
- float4 conversion is risk free (patch #1)
- float8 conversion is okay, too, if coupled with time[stamp[tz]] conversion
(patch #2) but with int64 timestamps enabled, the next one is also
needed:
- int64 conversion (patch #3) is mostly okay but it is the one that's
causing
the tsearch regression

I looked at the tsearch code and found only one thing that can be
suspicious, namely:

typedef uint64 TSQuerySign;

TSQuerySign is handled almost everywhere as an allocated,
passed-by-reference value. I converted it with the attached patch (#4)
so it uses Int64GetDatum() and DatumGetInt64() functions internally
and the regression went away. Please, consider applying all four patches.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Attachments:

01-pg84-passedbyval-float4.patch.gzapplication/x-tar; name=01-pg84-passedbyval-float4.patch.gzDownload
02-pg84-passedbyval-float8.patch.gzapplication/x-tar; name=02-pg84-passedbyval-float8.patch.gzDownload
03-pg84-passedbyval-int64.patch.gzapplication/x-tar; name=03-pg84-passedbyval-int64.patch.gzDownload
04-pg84-passedbyval-tsearch.patch.gzapplication/x-tar; name=04-pg84-passedbyval-tsearch.patch.gzDownload
#19Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Boszormenyi Zoltan (#18)
Re: float4/float8/int64 passed by value with tsearch fixup

Zoltan Boszormenyi wrote:

- float4 conversion is risk free (patch #1)

I applied this #1 patch. It needed some further adjustments; in
particular contrib/btree_gist regression check was crashing, and
utils/fmgr/README needed updating.

With contrib/seg also adjusted to use float4 instead of float32, and
thus the last usage of float32 gone, I am now wondering if it would be a
good idea to remove the float32 and float32data definitions in c.h.

Thanks to Zoltan for the patch.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#20Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#19)
Re: float4/float8/int64 passed by value with tsearch fixup

Alvaro Herrera wrote:

With contrib/seg also adjusted to use float4 instead of float32, and
thus the last usage of float32 gone, I am now wondering if it would be a
good idea to remove the float32 and float32data definitions in c.h.

Ok, the buildfarm is going yellow over this change. On "cardinal" I see
this:

*** ./expected/seg.out	Fri Apr 18 20:45:38 2008
--- ./results/seg.out	Fri Apr 18 20:59:40 2008
***************
*** 1069,1218 ****
  SELECT seg_lower(s), seg_center(s), seg_upper(s)
  FROM test_seg WHERE s @> '11.2..11.3' OR s IS NULL ORDER BY s;
   seg_lower | seg_center | seg_upper 
! -----------+------------+-----------
!  -Infinity |  -Infinity |        40
!  -Infinity |  -Infinity |        82
!  -Infinity |  -Infinity |        90
!          1 |          7 |        13
!        1.3 |       6.65 |        12
!          2 |       6.75 |      11.5
!        2.1 |       6.95 |      11.8
!        2.3 |   Infinity |  Infinity
!        2.3 |   Infinity |  Infinity
!        2.4 |       6.85 |      11.3
!        2.5 |          7 |      11.5
!        2.5 |       7.15 |      11.8
!        2.6 |   Infinity |  Infinity
!        2.7 |       7.35 |        12
!          3 |   Infinity |  Infinity
!          3 |       30.5 |        58
!        3.1 |        7.3 |      11.5
!        3.5 |        7.5 |      11.5
!        3.5 |       7.85 |      12.2
!          4 |          8 |        12
!          4 |   Infinity |  Infinity
!          4 |          8 |        12
!          4 |       7.85 |      11.7
!          4 |       8.25 |      12.5
!          4 |        8.5 |        13
!          4 |         32 |        60
!          4 |   Infinity |  Infinity
!        4.2 |       7.85 |      11.5
!        4.2 |       7.95 |      11.7
!        4.5 |       8.25 |        12
!        4.5 |          8 |      11.5
!        4.5 |       8.25 |        12
!        4.5 |       8.25 |        12
!        4.5 |        8.5 |      12.5
!        4.5 |      59.75 |       115
!        4.7 |       8.25 |      11.8
!        4.8 |       8.15 |      11.5
!        4.8 |        8.2 |      11.6
!        4.8 |       8.65 |      12.5
!        4.8 |   Infinity |  Infinity
!        4.9 |       8.45 |        12
!        4.9 |   Infinity |  Infinity
!          5 |       8.25 |      11.5
!          5 |        8.5 |        12
!          5 |       17.5 |        30
!          5 |        8.2 |      11.4
!          5 |       8.25 |      11.5
!          5 |        8.3 |      11.6
!          5 |       8.35 |      11.7
!          5 |        8.5 |        12
!          5 |        8.5 |        12
!          5 |        8.5 |        12
!        5.2 |       8.35 |      11.5
!        5.2 |        8.6 |        12
!       5.25 |      8.625 |        12
!        5.3 |        8.4 |      11.5
!        5.3 |       9.15 |        13
!        5.3 |      47.65 |        90
!        5.3 |   Infinity |  Infinity
!        5.4 |   Infinity |  Infinity
!        5.5 |        8.5 |      11.5
!        5.5 |        8.6 |      11.7
!        5.5 |       8.75 |        12
!        5.5 |       8.75 |        12
!        5.5 |          9 |      12.5
!        5.5 |        9.5 |      13.5
!        5.5 |   Infinity |  Infinity
!        5.5 |   Infinity |  Infinity
!        5.7 |   Infinity |  Infinity
!        5.9 |   Infinity |  Infinity
!          6 |       8.75 |      11.5
!          6 |          9 |        12
!          6 |       8.75 |      11.5
!          6 |        9.5 |        13
!          6 |       8.75 |      11.5
!        6.1 |       9.05 |        12
!        6.1 |   Infinity |  Infinity
!        6.2 |       8.85 |      11.5
!        6.3 |   Infinity |  Infinity
!        6.5 |          9 |      11.5
!        6.5 |       9.25 |        12
!        6.5 |       9.25 |        12
!        6.5 |   Infinity |  Infinity
!        6.6 |   Infinity |  Infinity
!        6.7 |        9.1 |      11.5
!        6.7 |   Infinity |  Infinity
!       6.75 |   Infinity |  Infinity
!        6.8 |   Infinity |  Infinity
!        6.9 |       9.55 |      12.2
!        6.9 |      48.45 |        90
!        6.9 |   Infinity |  Infinity
!          7 |       9.25 |      11.5
!          7 |       9.25 |      11.5
!          7 |       9.25 |      11.5
!          7 |   Infinity |  Infinity
!       7.15 |   Infinity |  Infinity
!        7.2 |      10.35 |      13.5
!        7.3 |      48.65 |        90
!        7.3 |   Infinity |  Infinity
!        7.3 |   Infinity |  Infinity
!        7.4 |       9.75 |      12.1
!        7.4 |   Infinity |  Infinity
!        7.5 |        9.5 |      11.5
!        7.5 |       9.75 |        12
!        7.5 |   Infinity |  Infinity
!        7.7 |        9.6 |      11.5
!        7.7 |   Infinity |  Infinity
!       7.75 |   Infinity |  Infinity
!          8 |       9.85 |      11.7
!          8 |         10 |        12
!          8 |       10.5 |        13
!        8.2 |   Infinity |  Infinity
!        8.3 |   Infinity |  Infinity
!        8.5 |         10 |      11.5
!        8.5 |       10.5 |      12.5
!        8.5 |   Infinity |  Infinity
!        8.6 |       53.8 |        99
!        8.7 |         10 |      11.3
!        8.7 |       10.2 |      11.7
!        8.9 |       10.2 |      11.5
!          9 |       10.5 |        12
!          9 |      10.15 |      11.3
!          9 |      10.25 |      11.5
!          9 |       10.5 |        12
!          9 |   Infinity |  Infinity
!        9.2 |       10.6 |        12
!        9.4 |       10.8 |      12.2
!        9.5 |      10.75 |        12
!        9.5 |      10.85 |      12.2
!        9.5 |   Infinity |  Infinity
!        9.6 |      10.55 |      11.5
!        9.7 |       10.6 |      11.5
!        9.7 |      10.85 |        12
!        9.8 |      11.15 |      12.5
!         10 |       10.8 |      11.6
!         10 |      10.75 |      11.5
!         10 |      11.25 |      12.5
!         10 |      11.25 |      12.5
!       10.2 |         11 |      11.8
!       10.5 |         11 |      11.5
!       10.5 |         11 |      11.5
!       10.5 |         12 |      13.5
!       10.7 |       11.5 |      12.3
             |            |          
  (144 rows)
--- 1069,1218 ----
  SELECT seg_lower(s), seg_center(s), seg_upper(s)
  FROM test_seg WHERE s @> '11.2..11.3' OR s IS NULL ORDER BY s;
    seg_lower   |  seg_center  |  seg_upper   
! --------------+--------------+--------------
!  -3.40315e-06 | -3.40315e-06 | -3.40315e-06
!  -3.40286e-06 | -3.40286e-06 | -3.40286e-06
!  -3.40296e-06 | -3.40296e-06 | -3.40296e-06
!  -3.40056e-06 | -3.40056e-06 | -3.40056e-06
!  -3.39789e-06 | -3.39789e-06 | -3.39789e-06
!  -3.39752e-06 | -3.39752e-06 | -3.39752e-06
!  -3.39735e-06 | -3.39735e-06 | -3.39735e-06
!  -3.40043e-06 | -3.40043e-06 | -3.40043e-06
!  -3.39825e-06 | -3.39825e-06 | -3.39825e-06
!  -3.39722e-06 | -3.39722e-06 | -3.39722e-06
!  -3.39715e-06 | -3.39715e-06 | -3.39715e-06
!  -3.39714e-06 | -3.39714e-06 | -3.39714e-06
!    -3.397e-06 |   -3.397e-06 |   -3.397e-06
!  -3.39695e-06 | -3.39695e-06 | -3.39695e-06
!  -3.40275e-06 | -3.40275e-06 | -3.40275e-06
!  -3.39669e-06 | -3.39669e-06 | -3.39669e-06
!  -3.39641e-06 | -3.39641e-06 | -3.39641e-06
!  -3.39605e-06 | -3.39605e-06 | -3.39605e-06
!  -3.39604e-06 | -3.39604e-06 | -3.39604e-06
!   -3.4025e-06 |  -3.4025e-06 |  -3.4025e-06
!  -3.40257e-06 | -3.40257e-06 | -3.40257e-06
!  -3.39517e-06 | -3.39517e-06 | -3.39517e-06
!  -3.39504e-06 | -3.39504e-06 | -3.39504e-06
!  -3.39503e-06 | -3.39503e-06 | -3.39503e-06
!  -3.39503e-06 | -3.39503e-06 | -3.39503e-06
!  -3.39489e-06 | -3.39489e-06 | -3.39489e-06
!  -3.39509e-06 | -3.39509e-06 | -3.39509e-06
!  -3.39464e-06 | -3.39464e-06 | -3.39464e-06
!  -3.39463e-06 | -3.39463e-06 | -3.39463e-06
!  -3.40236e-06 | -3.40236e-06 | -3.40236e-06
!  -3.39408e-06 | -3.39408e-06 | -3.39408e-06
!  -3.39361e-06 | -3.39361e-06 | -3.39361e-06
!   -3.3936e-06 |  -3.3936e-06 |  -3.3936e-06
!  -3.39387e-06 | -3.39387e-06 | -3.39387e-06
!  -3.39408e-06 | -3.39408e-06 | -3.39408e-06
!  -3.39333e-06 | -3.39333e-06 | -3.39333e-06
!  -3.39309e-06 | -3.39309e-06 | -3.39309e-06
!  -3.39308e-06 | -3.39308e-06 | -3.39308e-06
!  -3.39307e-06 | -3.39307e-06 | -3.39307e-06
!  -3.39311e-06 | -3.39311e-06 | -3.39311e-06
!  -3.39279e-06 | -3.39279e-06 | -3.39279e-06
!  -3.39287e-06 | -3.39287e-06 | -3.39287e-06
!  -3.39274e-06 | -3.39274e-06 | -3.39274e-06
!  -3.39273e-06 | -3.39273e-06 | -3.39273e-06
!  -3.39272e-06 | -3.39272e-06 | -3.39272e-06
!  -3.39258e-06 | -3.39258e-06 | -3.39258e-06
!  -3.39257e-06 | -3.39257e-06 | -3.39257e-06
!  -3.39257e-06 | -3.39257e-06 | -3.39257e-06
!  -3.39256e-06 | -3.39256e-06 | -3.39256e-06
!  -3.39255e-06 | -3.39255e-06 | -3.39255e-06
!  -3.39227e-06 | -3.39227e-06 | -3.39227e-06
!  -3.39228e-06 | -3.39228e-06 | -3.39228e-06
!  -3.39186e-06 | -3.39186e-06 | -3.39186e-06
!  -3.39161e-06 | -3.39161e-06 | -3.39161e-06
!  -3.39155e-06 | -3.39155e-06 | -3.39155e-06
!  -3.39148e-06 | -3.39148e-06 | -3.39148e-06
!  -3.39147e-06 | -3.39147e-06 | -3.39147e-06
!  -3.39128e-06 | -3.39128e-06 | -3.39128e-06
!  -3.39153e-06 | -3.39153e-06 | -3.39153e-06
!  -3.39126e-06 | -3.39126e-06 | -3.39126e-06
!  -3.39106e-06 | -3.39106e-06 | -3.39106e-06
!  -3.39105e-06 | -3.39105e-06 | -3.39105e-06
!  -3.39105e-06 | -3.39105e-06 | -3.39105e-06
!  -3.39074e-06 | -3.39074e-06 | -3.39074e-06
!  -3.39104e-06 | -3.39104e-06 | -3.39104e-06
!  -3.39103e-06 | -3.39103e-06 | -3.39103e-06
!   -3.3911e-06 |  -3.3911e-06 |  -3.3911e-06
!  -3.39841e-06 | -3.39841e-06 | -3.39841e-06
!   -3.3904e-06 |  -3.3904e-06 |  -3.3904e-06
!   -3.3896e-06 |  -3.3896e-06 |  -3.3896e-06
!  -3.38943e-06 | -3.38943e-06 | -3.38943e-06
!  -3.38936e-06 | -3.38936e-06 | -3.38936e-06
!  -3.38925e-06 | -3.38925e-06 | -3.38925e-06
!  -3.38924e-06 | -3.38924e-06 | -3.38924e-06
!  -3.39837e-06 | -3.39837e-06 | -3.39837e-06
!  -3.38867e-06 | -3.38867e-06 | -3.38867e-06
!  -3.38885e-06 | -3.38885e-06 | -3.38885e-06
!  -3.38815e-06 | -3.38815e-06 | -3.38815e-06
!  -3.38805e-06 | -3.38805e-06 | -3.38805e-06
!  -3.38735e-06 | -3.38735e-06 | -3.38735e-06
!  -3.38735e-06 | -3.38735e-06 | -3.38735e-06
!  -3.38705e-06 | -3.38705e-06 | -3.38705e-06
!  -3.38742e-06 | -3.38742e-06 | -3.38742e-06
!  -3.38694e-06 | -3.38694e-06 | -3.38694e-06
!  -3.38672e-06 | -3.38672e-06 | -3.38672e-06
!  -3.38673e-06 | -3.38673e-06 | -3.38673e-06
!  -3.38624e-06 | -3.38624e-06 | -3.38624e-06
!  -3.38618e-06 | -3.38618e-06 | -3.38618e-06
!  -3.38581e-06 | -3.38581e-06 | -3.38581e-06
!  -3.38561e-06 | -3.38561e-06 | -3.38561e-06
!  -3.38583e-06 | -3.38583e-06 | -3.38583e-06
!   -3.3988e-06 |  -3.3988e-06 |  -3.3988e-06
!  -3.38541e-06 | -3.38541e-06 | -3.38541e-06
!  -3.38512e-06 | -3.38512e-06 | -3.38512e-06
!  -3.38548e-06 | -3.38548e-06 | -3.38548e-06
!  -3.39831e-06 | -3.39831e-06 | -3.39831e-06
!   -3.3848e-06 |  -3.3848e-06 |  -3.3848e-06
!  -3.38396e-06 | -3.38396e-06 | -3.38396e-06
!  -3.38423e-06 | -3.38423e-06 | -3.38423e-06
!   -3.3981e-06 |  -3.3981e-06 |  -3.3981e-06
!   -3.3839e-06 |  -3.3839e-06 |  -3.3839e-06
!  -3.38392e-06 | -3.38392e-06 | -3.38392e-06
!   -3.3836e-06 |  -3.3836e-06 |  -3.3836e-06
!   -3.3836e-06 |  -3.3836e-06 |  -3.3836e-06
!  -3.38369e-06 | -3.38369e-06 | -3.38369e-06
!  -3.38304e-06 | -3.38304e-06 | -3.38304e-06
!  -3.38309e-06 | -3.38309e-06 | -3.38309e-06
!   -3.3827e-06 |  -3.3827e-06 |  -3.3827e-06
!  -3.38223e-06 | -3.38223e-06 | -3.38223e-06
!  -3.38222e-06 | -3.38222e-06 | -3.38222e-06
!  -3.38207e-06 | -3.38207e-06 | -3.38207e-06
!   -3.3819e-06 |  -3.3819e-06 |  -3.3819e-06
!  -3.38171e-06 | -3.38171e-06 | -3.38171e-06
!  -3.38131e-06 | -3.38131e-06 | -3.38131e-06
!  -3.38141e-06 | -3.38141e-06 | -3.38141e-06
!  -3.38147e-06 | -3.38147e-06 | -3.38147e-06
!  -3.38122e-06 | -3.38122e-06 | -3.38122e-06
!  -3.38118e-06 | -3.38118e-06 | -3.38118e-06
!  -3.38117e-06 | -3.38117e-06 | -3.38117e-06
!  -3.38084e-06 | -3.38084e-06 | -3.38084e-06
!  -3.38079e-06 | -3.38079e-06 | -3.38079e-06
!   -3.3807e-06 |  -3.3807e-06 |  -3.3807e-06
!  -3.38069e-06 | -3.38069e-06 | -3.38069e-06
!  -3.38068e-06 | -3.38068e-06 | -3.38068e-06
!  -3.38077e-06 | -3.38077e-06 | -3.38077e-06
!  -3.38057e-06 | -3.38057e-06 | -3.38057e-06
!   -3.3805e-06 |  -3.3805e-06 |  -3.3805e-06
!  -3.39849e-06 | -3.39849e-06 | -3.39849e-06
!  -3.39848e-06 | -3.39848e-06 | -3.39848e-06
!  -3.38048e-06 | -3.38048e-06 | -3.38048e-06
!  -3.38036e-06 | -3.38036e-06 | -3.38036e-06
!  -3.38032e-06 | -3.38032e-06 | -3.38032e-06
!   -3.3803e-06 |  -3.3803e-06 |  -3.3803e-06
!  -3.38026e-06 | -3.38026e-06 | -3.38026e-06
!  -3.40282e-06 | -3.40282e-06 | -3.40282e-06
!  -3.38017e-06 | -3.38017e-06 | -3.38017e-06
!  -3.38017e-06 | -3.38017e-06 | -3.38017e-06
!  -3.38016e-06 | -3.38016e-06 | -3.38016e-06
!  -3.38014e-06 | -3.38014e-06 | -3.38014e-06
!  -3.40054e-06 | -3.40054e-06 | -3.40054e-06
!  -3.38008e-06 | -3.38008e-06 | -3.38008e-06
!  -3.38007e-06 | -3.38007e-06 | -3.38007e-06
!  -3.38004e-06 | -3.38004e-06 | -3.38004e-06
                |              |             
  (144 rows)

So clearly the data is not being passed around as expected.

Please note that this is a completely new test -- prior to this commit,
these functions were not being called at all. So it is quite possible
that they have been failed for long.

I assume this is just some dumb portability mistake on my part ... or
perhaps the fact that the functions are still using v0 fmgr convention?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#20)
#22Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#21)
#23Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#22)
#24Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#23)
#25Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#24)
#26Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#25)
#27Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#26)
#28Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#27)
#29Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#27)
#30Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#29)
#31Boszormenyi Zoltan
zb@cybertec.at
In reply to: Alvaro Herrera (#25)
#32Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Boszormenyi Zoltan (#31)
#33Boszormenyi Zoltan
zb@cybertec.at
In reply to: Alvaro Herrera (#32)
#34Tom Lane
tgl@sss.pgh.pa.us
In reply to: Boszormenyi Zoltan (#33)
#35Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#26)
#36Boszormenyi Zoltan
zb@cybertec.at
In reply to: Tom Lane (#34)
#37Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#28)
#38Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#19)
#39Tom Lane
tgl@sss.pgh.pa.us
In reply to: Boszormenyi Zoltan (#36)
#40Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#37)
#41Boszormenyi Zoltan
zb@cybertec.at
In reply to: Tom Lane (#39)
#42Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#35)
#43Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#37)
#44Boszormenyi Zoltan
zb@cybertec.at
In reply to: Bruce Momjian (#43)
#45Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#43)
#46Tom Lane
tgl@sss.pgh.pa.us
In reply to: Boszormenyi Zoltan (#18)
#47Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#5)
#48Guillaume Smet
guillaume.smet@gmail.com
In reply to: Tom Lane (#46)
#49Tom Lane
tgl@sss.pgh.pa.us
In reply to: Guillaume Smet (#48)
#50Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#45)
#51Guillaume Smet
guillaume.smet@gmail.com
In reply to: Tom Lane (#49)