| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | Andres Freund <andres(at)anarazel(dot)de> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Decoupling our alignment assumptions about int64 and double |
| Date: | 2026-02-02 21:46:31 |
| Message-ID: | 1119616.1770068791@sss.pgh.pa.us |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
I pushed the att_align_nominal() refactoring, since that seems
to be a win whether we go forward with the rest of this or not.
Attached a v3 of the main patch (now again numbered 0001), plus
new work in 0002 that adds the two new alignment values to
pg_control and insists on a match before pg_upgrade'ing.
As I mentioned, this'd result in not being able to pg_upgrade
from old versions to v19 on AIX, though everywhere else it
should be fine.
0001 is the same as before except for one change: I modified the
test in CreateCast that checks whether a binary-compatible cast
is allowable. I did that because I hit errors in cross-version
upgrade testing. The regression tests do this:
create type int8alias1 (..., like int8);
create cast (int8 as int8alias1) without function;
which seems perfectly legit, but the cast fails dump/reload.
That's because we don't remember the "like" clause as such,
so the dump of int8alias1 from an existing branch will say
alignment = double, and then that doesn't match int8's new
alignment = int64.
Of course we could fix the cross-version tests by teaching
AdjustUpgrade.pm to drop these casts, but I think this is
telling us about a real usability gotcha that is likely
to bite users. A closely related issue is the change I'd
already had to make in regression/sql/float8.sql to not
try to cast bigint to float8 without function; that used
to work but doesn't with the new typalign values.
What I did below is to instead insist on a match of the represented
alignment requirement:
- typ1align != typ2align)
+ typalign_to_alignby(typ1align) != typalign_to_alignby(typ2align))
I'm not totally sold on that particular formulation, however.
For one thing it feels a bit too loose: on most machines it'd
allow alignment = max to match int64 or double, and I'm not
sure we want that. For another, it makes the allowable casts
platform-dependent. With the v3 code, we could skip changing
float8.sql on most platforms, but it'd still fail on AIX.
An alternative I've been thinking about, but haven't quite
convinced myself about, is: for pass-by-value types, why do
we need to insist on alignment match at all? Once the value
is in Datum format it doesn't matter, and for purposes of
a binary-compatible cast the input and output are always in
Datum format. So I was thinking about
- typ1align != typ2align)
+ (typ1align != typ2align && !typ1byval))
which'd allow all these cases to pass without SQL-code changes.
Is there a hole in that?
An even weaker restriction would be "allow if alignby matches
or if !byval", but I'm not sure we need to go there in practice.
It'd re-introduce the problem of the test being platform dependent,
which doesn't seem great.
I'd expect to squash these into one commit in the end, but I kept
0002 separate for ease of review, since 0001 is the same as before
except for the CreateCast diff.
Thoughts?
regards, tom lane
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Decouple-our-alignment-assumptions-about-int64-an.patch | text/x-diff | 51.2 KB |
| v3-0002-Add-new-alignment-info-to-pg_control-and-check-it.patch | text/x-diff | 16.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nathan Bossart | 2026-02-02 21:46:35 | Re: [patch] Add process title to test_shm_mq worker |
| Previous Message | Peter Smith | 2026-02-02 21:19:47 | Re: use the malloc macros in pg_dump.c |