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/



--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

-
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

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 notefor
> 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/



--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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/



--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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.

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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