Making type Datum be 8 bytes everywhere

Started by Tom Lane9 months ago32 messageshackers
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

In a discussion on Discord (in the PG #core-hacking channel,
which unfortunately is inaccessible to non-members), Andres
and Robert complained about the development/maintenance costs
of continuing to support 32-bit platforms. Here is a modest
proposal to reduce those costs without going so far as to
entirely desupport such platforms: let's require them to use
8-byte Datums even though that's probably not a native data
type for them. That lets us get rid of logic to support the
!USE_FLOAT8_BYVAL case, and allows a few other simplifications.

The attached patch switches to 8-byte Datums everywhere, but
doesn't make any effort to remove the now-dead code. I made
it just as a proof-of-concept that this can work. It compiled
cleanly and passed check-world for me on a 32-bit FreeBSD
image.

I've not looked into the performance consequences. We probably
should at least try to measure that, though I'm not sure what
our threshold of pain would be for deciding not to do this.

Thoughts?

regards, tom lane

Attachments:

v0-0001-Make-type-Datum-be-8-bytes-wide-everywhere.patchtext/x-diff; charset=us-ascii; name=v0-0001-Make-type-Datum-be-8-bytes-wide-everywhere.patchDownload+18-19
#2Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#1)
Re: Making type Datum be 8 bytes everywhere

Hi,

On 2025-07-17 20:09:57 -0400, Tom Lane wrote:

In a discussion on Discord (in the PG #core-hacking channel,
which unfortunately is inaccessible to non-members), Andres
and Robert complained about the development/maintenance costs
of continuing to support 32-bit platforms. Here is a modest
proposal to reduce those costs without going so far as to
entirely desupport such platforms: let's require them to use
8-byte Datums even though that's probably not a native data
type for them. That lets us get rid of logic to support the
!USE_FLOAT8_BYVAL case, and allows a few other simplifications.

The attached patch switches to 8-byte Datums everywhere, but
doesn't make any effort to remove the now-dead code.

Thanks for writing that!

I made it just as a proof-of-concept that this can work. It compiled
cleanly and passed check-world for me on a 32-bit FreeBSD image.

Interestingly it generates a *lot* of warnings here when building for 32 bit
with gcc. One class of complaints is about DatumGetPointer() and
PointerGetDatum() casting between different sizes:

../../../../../home/andres/src/postgresql/src/include/postgres.h: In function 'DatumGetPointer':
../../../../../home/andres/src/postgresql/src/include/postgres.h:320:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
320 | return (Pointer) X;
| ^
../../../../../home/andres/src/postgresql/src/include/postgres.h: In function 'PointerGetDatum':
../../../../../home/andres/src/postgresql/src/include/postgres.h:330:16: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
330 | return (Datum) X;
| ^

And then there's a set of complains due to code converting from NULL to Datum
without going through PointerGetDatum():
../../../../../home/andres/src/postgresql/src/include/access/htup_details.h: In function 'fastgetattr':
../../../../../home/andres/src/postgresql/src/include/access/htup_details.h:887:32: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
887 | return (Datum) NULL;

../../../../../home/andres/src/postgresql/src/include/access/itup.h: In function 'index_getattr':
../../../../../home/andres/src/postgresql/src/include/access/itup.h:157:32: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
157 | return (Datum) NULL;

A third, not quite as verbose set is random code in .c files missing uses of
DatumGetPointer(). There are lot of those.

A fourth class is passing a Datum to VAR* macros. They're a bit too verbose to
paste here, but it's just a variation of the above. I'm not really sure what
our intended use of them is, do we intend to pass pointers or datums to the
macros? I suspect we'd have to move the casts into the varlena macros,
otherwise we'll have to add DatumGetPointer() uses all over.

One of these days I should again try the experiment of making Datum into a
struct, to automatically catch omissions of datum <-> native type. Having them
be silent most of the time really sucks. I suspect that if we get the
64bit-datum-on-32bit-platform code to be warning-free, it'd get a lot easier
to struct-ify Datum. I don't recall the details, but I suspect that all the
varlena macros etc were the problem with that.

I've not looked into the performance consequences. We probably
should at least try to measure that, though I'm not sure what
our threshold of pain would be for deciding not to do this.

From my POV the threshold would have to be rather high for backend code. Less
so in libpq, but that's not affected here.

Greetings,

Andres Freund

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#2)
Re: Making type Datum be 8 bytes everywhere

Andres Freund <andres@anarazel.de> writes:

On 2025-07-17 20:09:57 -0400, Tom Lane wrote:

I made it just as a proof-of-concept that this can work. It compiled
cleanly and passed check-world for me on a 32-bit FreeBSD image.

Interestingly it generates a *lot* of warnings here when building for 32 bit
with gcc.

Oh, that's annoying. I tested it with

$ cc --version
FreeBSD clang version 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a303)
Target: i386-unknown-freebsd13.1
Thread model: posix
InstalledDir: /usr/bin

which is slightly back-rev but not that old. Which gcc did you use?

One class of complaints is about DatumGetPointer() and
PointerGetDatum() casting between different sizes:

../../../../../home/andres/src/postgresql/src/include/postgres.h: In function 'DatumGetPointer':
../../../../../home/andres/src/postgresql/src/include/postgres.h:320:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
320 | return (Pointer) X;
| ^
../../../../../home/andres/src/postgresql/src/include/postgres.h: In function 'PointerGetDatum':
../../../../../home/andres/src/postgresql/src/include/postgres.h:330:16: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
330 | return (Datum) X;
| ^

We might be able to silence those with intermediate casts to uintptr_t,
perhaps?

And then there's a set of complains due to code converting from NULL to Datum
without going through PointerGetDatum():
887 | return (Datum) NULL;

This code just strikes me as tin-eared. Our normal locution for
that is "(Datum) 0", and I see no reason to deviate.

A third, not quite as verbose set is random code in .c files missing uses of
DatumGetPointer(). There are lot of those.

Yeah, that's stuff we ought to fix anyway. Same with VAR* macros.

One of these days I should again try the experiment of making Datum into a
struct, to automatically catch omissions of datum <-> native type. Having them
be silent most of the time really sucks.

Perhaps. I'd be a little sad if the "(Datum) 0" notation stops
working, because there are sure a lot of those.

I've not looked into the performance consequences. We probably
should at least try to measure that, though I'm not sure what
our threshold of pain would be for deciding not to do this.

From my POV the threshold would have to be rather high for backend code. Less
so in libpq, but that's not affected here.

I don't know if it's "rather high" or not, but that seems like
the gating factor that ought to be checked before putting in
more work.

regards, tom lane

#4Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#3)
Re: Making type Datum be 8 bytes everywhere

Hi,

On 2025-07-18 13:24:32 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2025-07-17 20:09:57 -0400, Tom Lane wrote:

I made it just as a proof-of-concept that this can work. It compiled
cleanly and passed check-world for me on a 32-bit FreeBSD image.

Interestingly it generates a *lot* of warnings here when building for 32 bit
with gcc.

Oh, that's annoying. I tested it with

$ cc --version
FreeBSD clang version 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a303)
Target: i386-unknown-freebsd13.1
Thread model: posix
InstalledDir: /usr/bin

which is slightly back-rev but not that old. Which gcc did you use?

That was gcc 14.

One class of complaints is about DatumGetPointer() and
PointerGetDatum() casting between different sizes:

../../../../../home/andres/src/postgresql/src/include/postgres.h: In function 'DatumGetPointer':
../../../../../home/andres/src/postgresql/src/include/postgres.h:320:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
320 | return (Pointer) X;
| ^
../../../../../home/andres/src/postgresql/src/include/postgres.h: In function 'PointerGetDatum':
../../../../../home/andres/src/postgresql/src/include/postgres.h:330:16: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
330 | return (Datum) X;
| ^

We might be able to silence those with intermediate casts to uintptr_t,
perhaps?

Yep, that does the trick.

I've not looked into the performance consequences. We probably
should at least try to measure that, though I'm not sure what
our threshold of pain would be for deciding not to do this.

From my POV the threshold would have to be rather high for backend code. Less
so in libpq, but that's not affected here.

I don't know if it's "rather high" or not, but that seems like
the gating factor that ought to be checked before putting in
more work.

The hard bit would be to determine what workload to measure. Something like
pgbench probably won't suffer meaningfully, there's just not enough passing of
values around.

For a bit I thought it'd need to be a workload that does a lot of int4 math or
such, but I doubt the overhead of it matters sufficiently there.

Then I realized that the biggest issue probably would be a query that does a
lot of tuple deforming of 4 byte values, while not actually accessing them?

Can you think of a worse workload than that?

Greetings,

Andres Freund

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#4)
Re: Making type Datum be 8 bytes everywhere

Andres Freund <andres@anarazel.de> writes:

On 2025-07-18 13:24:32 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

One class of complaints is about DatumGetPointer() and
PointerGetDatum() casting between different sizes:

We might be able to silence those with intermediate casts to uintptr_t,
perhaps?

Yep, that does the trick.

Cool, thanks for checking.

I've not looked into the performance consequences. We probably
should at least try to measure that, though I'm not sure what
our threshold of pain would be for deciding not to do this.

The hard bit would be to determine what workload to measure. Something like
pgbench probably won't suffer meaningfully, there's just not enough passing of
values around.

I'm disinclined to put in a huge amount of effort looking for the
worst case. We established long ago that we weren't going to
optimize for 32-bit anymore. So as long as this doesn't completely
tank performance on 32-bit, I'm satisfied. I'd almost say that
if standard pgbench doesn't notice the change, that's good enough.

regards, tom lane

#6Nazir Bilal Yavuz
byavuz81@gmail.com
In reply to: Tom Lane (#5)
Re: Making type Datum be 8 bytes everywhere

Hi,

Thank you for working on this!

On Wed, 23 Jul 2025 at 22:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm disinclined to put in a huge amount of effort looking for the
worst case. We established long ago that we weren't going to
optimize for 32-bit anymore. So as long as this doesn't completely
tank performance on 32-bit, I'm satisfied. I'd almost say that
if standard pgbench doesn't notice the change, that's good enough.

I did a basic pgbench benchmark on a 32-bit build and there is no change.

$ pgbench -i -s 100 test
$ pgbench -c 16 -j 16 -b $type -T 150 test

TPS results are:

select-only:
master: 215654
patched: 215751

simple-update:
master: 4454
patched: 4446

tpcb-like:
master: 4094
patched: 4128

--
Regards,
Nazir Bilal Yavuz
Microsoft

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nazir Bilal Yavuz (#6)
Re: Making type Datum be 8 bytes everywhere

Nazir Bilal Yavuz <byavuz81@gmail.com> writes:

On Wed, 23 Jul 2025 at 22:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm disinclined to put in a huge amount of effort looking for the
worst case. We established long ago that we weren't going to
optimize for 32-bit anymore. So as long as this doesn't completely
tank performance on 32-bit, I'm satisfied. I'd almost say that
if standard pgbench doesn't notice the change, that's good enough.

I did a basic pgbench benchmark on a 32-bit build and there is no change.

Thanks for doing that! For me, that's enough evidence to move
forward.

regards, tom lane

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#2)
Re: Making type Datum be 8 bytes everywhere

[ getting back to looking at this ]

Andres Freund <andres@anarazel.de> writes:

On 2025-07-17 20:09:57 -0400, Tom Lane wrote:

The attached patch switches to 8-byte Datums everywhere, but
doesn't make any effort to remove the now-dead code.

Thanks for writing that!
Interestingly it generates a *lot* of warnings here when building for 32 bit
with gcc.

Yeah, I can reproduce that if I use gcc. Interesting that casting
between pointer and different-size integer is a default warning on gcc
but not clang. The major stumbling block to cleaning that up seems
to be what you noted:

A fourth class is passing a Datum to VAR* macros. They're a bit too verbose to
paste here, but it's just a variation of the above. I'm not really sure what
our intended use of them is, do we intend to pass pointers or datums to the
macros? I suspect we'd have to move the casts into the varlena macros,
otherwise we'll have to add DatumGetPointer() uses all over.

Right, we have for a long time not worried about whether VARDATA and
the allied macros are being fed a pointer or a Datum. I recall that
somebody tried to make those macros into static inlines awhile back,
and failed because of the lack of clarity about how to declare their
arguments. I think the way forward here is to tackle that head-on
and split the top-level macros into two static inlines, along the
lines of
VARDATA(Pointer ptr)
and
VARDATA_D(Datum dat)
where the _D versions are simply DatumGetPointer and then call the
non-D versions.

I'm giving the traditional names to the Pointer variants because it
turns out that way more places would have to change if we do it the
other way: in a rough count, about 50 versus about 1700. (This is
counting only the core backend.) Beyond that, though, bikeshedding
on the naming is welcome.

One of these days I should again try the experiment of making Datum into a
struct, to automatically catch omissions of datum <-> native type.

It looks like the main remaining thing we'd need to change for that
to be possible is to replace "(Datum) 0" with some macro, say
"INVALID_DATUM". I don't especially want to do that though: there
are upwards of 600 occurrences in our tree, so the consequences for
back-patch hazards seem to me to outweigh the benefit.

regards, tom lane

#9Peter Eisentraut
peter_e@gmx.net
In reply to: Andres Freund (#2)
Re: Making type Datum be 8 bytes everywhere

On 18.07.25 18:26, Andres Freund wrote:

One of these days I should again try the experiment of making Datum into a
struct, to automatically catch omissions of datum <-> native type. Having them
be silent most of the time really sucks. I suspect that if we get the
64bit-datum-on-32bit-platform code to be warning-free, it'd get a lot easier
to struct-ify Datum. I don't recall the details, but I suspect that all the
varlena macros etc were the problem with that.

Patch posted here for demonstration:
/messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org

#10Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#8)
Re: Making type Datum be 8 bytes everywhere

On 30.07.25 18:06, Tom Lane wrote:

Right, we have for a long time not worried about whether VARDATA and
the allied macros are being fed a pointer or a Datum. I recall that
somebody tried to make those macros into static inlines awhile back,
and failed because of the lack of clarity about how to declare their
arguments.

I don't know if that was me, but I have posted a patch about this now:
/messages/by-id/928ea48f-77c6-417b-897c-621ef16685a6@eisentraut.org

I think the way forward here is to tackle that head-on
and split the top-level macros into two static inlines, along the
lines of
VARDATA(Pointer ptr)
and
VARDATA_D(Datum dat)
where the _D versions are simply DatumGetPointer and then call the
non-D versions.

I'm giving the traditional names to the Pointer variants because it
turns out that way more places would have to change if we do it the
other way: in a rough count, about 50 versus about 1700. (This is
counting only the core backend.) Beyond that, though, bikeshedding
on the naming is welcome.

In my patch, I just added the missing DatumGetPointer() calls, which
seemed easy enough.

There is precedent for having two different functions, though, like
att_addlength_pointer() and att_addlength_datum().

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#10)
Re: Making type Datum be 8 bytes everywhere

Peter Eisentraut <peter@eisentraut.org> writes:

On 30.07.25 18:06, Tom Lane wrote:

I'm giving the traditional names to the Pointer variants because it
turns out that way more places would have to change if we do it the
other way: in a rough count, about 50 versus about 1700. (This is
counting only the core backend.) Beyond that, though, bikeshedding
on the naming is welcome.

In my patch, I just added the missing DatumGetPointer() calls, which
seemed easy enough.

I had an earlier patch version that also did that, but it seemed
kind of verbose to me: adding "_D" is much shorter than adding
"DatumGetPointer()", and fewer parens seems good for readability.

One interesting thing I noted is that in some modules we already
were applying DatumGetPointer where needed (mostly, at least).
The patch I just posted in your other thread also simplifies those
cases to use the "_D" notation, which makes it longer than strictly
necessary. But I think consistency of notation is good.

There is precedent for having two different functions, though, like
att_addlength_pointer() and att_addlength_datum().

Yeah ... those two macros could stand to be cleaned up too, per
the notes in their comments. But I don't think we need to fix
that today.

regards, tom lane

#12Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#1)
Re: Making type Datum be 8 bytes everywhere

On 18.07.25 02:09, Tom Lane wrote:

The attached patch switches to 8-byte Datums everywhere, but
doesn't make any effort to remove the now-dead code.

Is the plan to support only exactly Datums of size 8, or Datums of size
at least 8?

There are some optimistic conditionals like

#if SIZEOF_DATUM >= 8

I don't expect it to work right now with larger sizes, but maybe it
should? (uuid pass-by-value!?!)

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#12)
Re: Making type Datum be 8 bytes everywhere

Peter Eisentraut <peter@eisentraut.org> writes:

On 18.07.25 02:09, Tom Lane wrote:

The attached patch switches to 8-byte Datums everywhere, but
doesn't make any effort to remove the now-dead code.

Is the plan to support only exactly Datums of size 8, or Datums of size
at least 8?

My plan was to hard-wire it at 8 permanently. It's pretty hard to
believe that there will be hardware on which sizeof(Datum) == 16
would be a reasonable choice anytime while people are still using C.
You'd want 16-byte registers and native operations, and I don't
see any manufacturers headed in that direction.

It could be reasonable to keep provisions for that as long as we still
have active hardware and testing for two different sizes of Datum.
However, once we kill off testing of sizeof(Datum) == 4, I think the
code will acquire hard assumptions that sizeof(Datum) == 8 pretty
soon. If I were upset with that prospect I would not be proposing
this change.

regards, tom lane

#14Michael Paquier
michael@paquier.xyz
In reply to: Tom Lane (#11)
Re: Making type Datum be 8 bytes everywhere

On Thu, Jul 31, 2025 at 10:27:37AM -0400, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

In my patch, I just added the missing DatumGetPointer() calls, which
seemed easy enough.

I had an earlier patch version that also did that, but it seemed
kind of verbose to me: adding "_D" is much shorter than adding
"DatumGetPointer()", and fewer parens seems good for readability.

I have mixed feelings about that, but as long as one is easily able to
detect that they should not pass a Datum. At the end, I'm kind of
OK-ish with the addition of the _D flavors to have a shortcut for the
VARDATA/DatumGetPointer() patterns, as an option. So I'd put +0.5.
--
Michael

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Michael Paquier (#14)
Re: Making type Datum be 8 bytes everywhere

I have just realized that this proposal has a rather nasty defect.
Per the following comment in spgist_private.h:

* If the prefix datum is of a pass-by-value type, it is stored in its
* Datum representation, that is its on-disk representation is of length
* sizeof(Datum). This is a fairly unfortunate choice, because in no other
* place does Postgres use Datum as an on-disk representation; it creates
* an unnecessary incompatibility between 32-bit and 64-bit builds. But the
* compatibility loss is mostly theoretical since MAXIMUM_ALIGNOF typically
* differs between such builds, too. Anyway we're stuck with it now.

This means we cannot change sizeof(Datum), nor reconsider the
pass-by-value classification of any datatype, without potentially
breaking pg_upgrade of some SP-GiST indexes on 32-bit machines.

Now, it looks like this doesn't affect any in-core SP-GiST opclasses.
The only one using a potentially affected type is kd_point_ops which
uses a float8 prefix. That'll have been stored in regular on-disk
format on a 32-bit machine, but if we redefine it as being stored
in 64-bit-Datum format, nothing actually changes. The case that
would be problematic is a prefix type that's 4 bytes or less, and
I don't see any.

A quick search of Debian Code Search doesn't find any extensions
that look like they are using small pass-by-value prefixes either.
So maybe we can get away with just changing this, but it's worrisome.

On the positive side, even if there are any SP-GiST opclasses that
are at risk, the population of installations using them on 32-bit
installs has got to be pretty tiny. And the worst-case answer is
that you'd have to reindex such indexes after pg_upgrade.

BTW, I don't think we can teach pg_upgrade to check for this
hazard, because the SP-GiST APIs are such that the data type
used for prefixes isn't visible at the SQL level.

Do we think that making this change is valuable enough to justify
taking such a risk?

regards, tom lane

#16Joe Conway
mail@joeconway.com
In reply to: Tom Lane (#15)
Re: Making type Datum be 8 bytes everywhere

On 8/8/25 21:14, Tom Lane wrote:

I have just realized that this proposal has a rather nasty defect.
Per the following comment in spgist_private.h:

* If the prefix datum is of a pass-by-value type, it is stored in its
* Datum representation, that is its on-disk representation is of length
* sizeof(Datum). This is a fairly unfortunate choice, because in no other
* place does Postgres use Datum as an on-disk representation; it creates
* an unnecessary incompatibility between 32-bit and 64-bit builds. But the
* compatibility loss is mostly theoretical since MAXIMUM_ALIGNOF typically
* differs between such builds, too. Anyway we're stuck with it now.

This means we cannot change sizeof(Datum), nor reconsider the
pass-by-value classification of any datatype, without potentially
breaking pg_upgrade of some SP-GiST indexes on 32-bit machines.

Now, it looks like this doesn't affect any in-core SP-GiST opclasses.
The only one using a potentially affected type is kd_point_ops which
uses a float8 prefix. That'll have been stored in regular on-disk
format on a 32-bit machine, but if we redefine it as being stored
in 64-bit-Datum format, nothing actually changes. The case that
would be problematic is a prefix type that's 4 bytes or less, and
I don't see any.

A quick search of Debian Code Search doesn't find any extensions
that look like they are using small pass-by-value prefixes either.
So maybe we can get away with just changing this, but it's worrisome.

On the positive side, even if there are any SP-GiST opclasses that
are at risk, the population of installations using them on 32-bit
installs has got to be pretty tiny.

I bet it is indistinguishable from zero...

And the worst-case answer is that you'd have to reindex such indexes
after pg_upgrade.

...and this seems like a reasonable answer if anyone pops up.

BTW, I don't think we can teach pg_upgrade to check for this
hazard, because the SP-GiST APIs are such that the data type
used for prefixes isn't visible at the SQL level.

Do we think that making this change is valuable enough to justify
taking such a risk?

yes +1

--
Joe Conway
PostgreSQL Contributors Team
Amazon Web Services: https://aws.amazon.com

#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Joe Conway (#16)
Re: Making type Datum be 8 bytes everywhere

Joe Conway <mail@joeconway.com> writes:

On 8/8/25 21:14, Tom Lane wrote:

I have just realized that this proposal has a rather nasty defect.
...
On the positive side, even if there are any SP-GiST opclasses that
are at risk, the population of installations using them on 32-bit
installs has got to be pretty tiny.

I bet it is indistinguishable from zero...

That's my bet too.

And the worst-case answer is that you'd have to reindex such indexes
after pg_upgrade.

...and this seems like a reasonable answer if anyone pops up.

Do we think that making this change is valuable enough to justify
taking such a risk?

yes +1

I've now fleshed out the patch series with some cleanup of code that's
been rendered dead. The 0001 patch is nearly the same as before,
but thanks to all the work Peter did, it doesn't trigger a pile of
gcc warnings (at least, not for me).

regards, tom lane

Attachments:

v1-0001-Make-type-Datum-be-8-bytes-wide-everywhere.patchtext/x-diff; charset=us-ascii; name=v1-0001-Make-type-Datum-be-8-bytes-wide-everywhere.patchDownload+22-26
v1-0002-Grab-the-low-hanging-fruit-from-forcing-sizeof-Da.patchtext/x-diff; charset=us-ascii; name*0=v1-0002-Grab-the-low-hanging-fruit-from-forcing-sizeof-Da.p; name*1=atchDownload+44-222
v1-0003-Grab-the-low-hanging-fruit-from-forcing-USE_FLOAT.patchtext/x-diff; charset=us-ascii; name*0=v1-0003-Grab-the-low-hanging-fruit-from-forcing-USE_FLOAT.p; name*1=atchDownload+84-291
#18Andrew Dunstan
andrew@dunslane.net
In reply to: Joe Conway (#16)
Re: Making type Datum be 8 bytes everywhere

On 2025-08-09 Sa 7:45 AM, Joe Conway wrote:

On 8/8/25 21:14, Tom Lane wrote:

I have just realized that this proposal has a rather nasty defect.
Per the following comment in spgist_private.h:

  * If the prefix datum is of a pass-by-value type, it is stored in its
  * Datum representation, that is its on-disk representation is of
length
  * sizeof(Datum).  This is a fairly unfortunate choice, because in
no other
  * place does Postgres use Datum as an on-disk representation; it
creates
  * an unnecessary incompatibility between 32-bit and 64-bit builds. 
But the
  * compatibility loss is mostly theoretical since MAXIMUM_ALIGNOF
typically
  * differs between such builds, too.  Anyway we're stuck with it now.

This means we cannot change sizeof(Datum), nor reconsider the
pass-by-value classification of any datatype, without potentially
breaking pg_upgrade of some SP-GiST indexes on 32-bit machines.

Now, it looks like this doesn't affect any in-core SP-GiST opclasses.
The only one using a potentially affected type is kd_point_ops which
uses a float8 prefix.  That'll have been stored in regular on-disk
format on a 32-bit machine, but if we redefine it as being stored
in 64-bit-Datum format, nothing actually changes.  The case that
would be problematic is a prefix type that's 4 bytes or less, and
I don't see any.

A quick search of Debian Code Search doesn't find any extensions
that look like they are using small pass-by-value prefixes either.
So maybe we can get away with just changing this, but it's worrisome.

On the positive side, even if there are any SP-GiST opclasses that
are at risk, the population of installations using them on 32-bit
installs has got to be pretty tiny.

I bet it is indistinguishable from zero...

And the worst-case answer is that you'd have to reindex such indexes
after pg_upgrade.

...and this seems like a reasonable answer if anyone pops up.

BTW, I don't think we can teach pg_upgrade to check for this
hazard, because the SP-GiST APIs are such that the data type
used for prefixes isn't visible at the SQL level.

Do we think that making this change is valuable enough to justify
taking such a risk?

yes +1

Agree to all the above

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

#19Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#17)
Re: Making type Datum be 8 bytes everywhere

On 09.08.25 18:44, Tom Lane wrote:

I've now fleshed out the patch series with some cleanup of code that's
been rendered dead.

These patches look mechanically correct to me.

For patch 0002:

The various additional uses of *GetDatum and DatumGet* could be applied
as a separate patch. That would make the remaining patch clearer as
mostly just removing dead code.

In tupmacs.h, I think the two sites with

case sizeof(Datum):

should be rewritten using

case sizeof(int64):

to match the other cases. Otherwise, this code ends up looking
mysteriously asymmetric. (And the related code in pg_type.c as well.)

The remaining mentions of "SIZEOF_DATUM" in gistproc.c and network.c
could be replaced by "sizeof(Datum)". Then we could eventually remove
SIZEOF_DATUM altogether. (Maybe DatumBigEndianToNative() would better
live in a different header file at that point, not sure.)

For patch 0003:

I would also remove most of the remaining uses of FLOAT8PASSBYVAL,
especially where it is used in relation with INT8OID, which is just
endlessly confusing.

We should also get rid of these things in the control file and the ABI
magic structs, but that could be done as separate patches. It would
probably require a separate round of thinking about compatibility,
upgrading, etc.

I'm also thinking, as a follow-on project, we could get rid of typbyval
and require that typbyval == (typlen > 0 && typlen <= 8). Something to
think about.

#20Andres Freund
andres@anarazel.de
In reply to: Peter Eisentraut (#19)
Re: Making type Datum be 8 bytes everywhere

Hi,

On 2025-08-12 08:30:43 +0200, Peter Eisentraut wrote:

I'm also thinking, as a follow-on project, we could get rid of typbyval and
require that typbyval == (typlen > 0 && typlen <= 8). Something to think
about.

We currently have types that aren't typbyval despite fitting those criteria:

postgres[1606972][1]=# SELECT oid::regtype, typlen FROM pg_type WHERE typlen > 0 and typlen <= 8 and not typbyval;
┌──────────┬────────┐
│ oid │ typlen │
├──────────┼────────┤
│ tid │ 6 │
│ macaddr │ 6 │
│ macaddr8 │ 8 │
└──────────┴────────┘
(3 rows)

Greetings,

Andres Freund

#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#20)
#22Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#19)
#23Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#22)
#24Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#23)
#25Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Tom Lane (#24)
#26Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tomas Vondra (#25)
#27Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Tom Lane (#26)
#28Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tomas Vondra (#27)
#29Ranier Vilela
ranier.vf@gmail.com
In reply to: Tom Lane (#26)
#30Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#1)
#31Tom Lane
tgl@sss.pgh.pa.us
In reply to: Ranier Vilela (#29)
#32Ranier Vilela
ranier.vf@gmail.com
In reply to: Tom Lane (#31)