[PG19] eager aggregation gives wrong results because of bpchar_ops

I'm inclined to use the newer patch in v19, rather than trusting
the runtime return-false method.

Sounds good to me. 0001 and 0002 apply to REL_19_STABLE as they are.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

I used Opus to go over the new features in PG19, and it found wrong
results from eager aggregation on a bpchar column with no length.

I went through all the data types that allege equalimage support
carefully, and found another problem: oidvector is claimed to
behave this way, but it does not. In particular, btoidvectorcmp
intentionally pays no attention to the array lower bound field.

While oidvectors would normally have a lower bound of 0, it's
not hard at all to make one that doesn't:

regression=# select array_lower(array[12,34]::oidvector, 1);
array_lower
-------------
1
(1 row)

So we find that these things are equal per btoidvectorcmp:

regression=# select array[12,34]::oidvector = '12 34'::oidvector;
?column?
----------
t
(1 row)

but they are not actually bitwise equal:

regression=# select array[12,34]::oidvector::oid[] = '12 34'::oidvector::oid[];
?column?
----------
f
(1 row)

So my first instinct is that we'd better rescind equalimage support
for oidvector too. But then pg_proc_proname_args_nsp_index and
perhaps some other system catalog indexes will start failing amcheck
in existing installations. That'd likely cause enough trouble to
outweigh the safety argument, especially since I really doubt that
it's possible to get a nonstandard oidvector value into a catalog
without doing superuser-y things.

So I'm not quite sure what to do about this. Maybe rescind in
HEAD/v19, and do nothing in the back branches?

By the by, I'm not especially happy about the errhint that Noah
added in 5f27b5f84:

+                     has_interval_ops
+                     ? errhint("This is known of \"interval\" indexes last built on a version predating 2023-11.")
+                     : 0));

This seems to me to have passed its sell-by date somewhere around
2024. I see that Shihao's patch tries to emulate that, but doing so
for three or so different types seems to me like it will be a complete
mess, and I see no real value in it. The outcome is the same
regardless of cause: you'd better REINDEX.

regards, tom lane

On Tue, Oct 6, 2026 at 12:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I went through all the data types that allege equalimage support
carefully, and found another problem: oidvector is claimed to
behave this way, but it does not. In particular, btoidvectorcmp
intentionally pays no attention to the array lower bound field.

Thanks for going to the trouble. I'll prepare a fix for these issues.

While oidvectors would normally have a lower bound of 0, it's
not hard at all to make one that doesn't:

So my first instinct is that we'd better rescind equalimage support
for oidvector too. But then pg_proc_proname_args_nsp_index and
perhaps some other system catalog indexes will start failing amcheck
in existing installations. That'd likely cause enough trouble to
outweigh the safety argument, especially since I really doubt that
it's possible to get a nonstandard oidvector value into a catalog
without doing superuser-y things.

So I'm not quite sure what to do about this. Maybe rescind in
HEAD/v19, and do nothing in the back branches?

+1. Once we rescind oidvector support for deduplication, amcheck will
reliably flag every affected index's metapage as corrupt -- regardless
of the actual contents of the index. That doesn't seem worth it, all
things considered.

By the by, I'm not especially happy about the errhint that Noah
added in 5f27b5f84:

+                     has_interval_ops
+                     ? errhint("This is known of \"interval\" indexes last built on a version predating 2023-11.")
+                     : 0));

This seems to me to have passed its sell-by date somewhere around
2024.

I'll remove that on master in passing. Also, I'll remove the relevant
pg_amproc.dat entries on 19 and master, but leave them alone on the
backbranches (backbranches will have to teach btequalimage and
btvarstrequalimage to disallow deduplication with these unsafe
opclasses).

--
Peter Geoghegan

On Tue, Oct 6, 2026 at 1:05 PM Peter Geoghegan <pg@bowt.ie> wrote:

Thanks for going to the trouble. I'll prepare a fix for these issues.

Attached are lightly edited versions of Shihao Zhong's bug fix
patches, incorporating the revisions I agreed to earlier today.

Note again that the patch that's intended for the released back
branches deliberately ignores the oidvector issue. Also, there are no
changes to system catalog contents on released branches (just 19 and
master), which isn't how Noah's similar 2023 interval_ops commit did
things. Shihao followed that precedent, but I don't think it's
appropriate to change the contents of the catalogs that initdb
creates, unless it's truly unavoidable.

If there are no objections, I'll commit these in the next couple of days.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

Attached are lightly edited versions of Shihao Zhong's bug fix
patches, incorporating the revisions I agreed to earlier today.

Note again that the patch that's intended for the released back
branches deliberately ignores the oidvector issue. Also, there are no
changes to system catalog contents on released branches (just 19 and
master), which isn't how Noah's similar 2023 interval_ops commit did
things. Shihao followed that precedent, but I don't think it's
appropriate to change the contents of the catalogs that initdb
creates, unless it's truly unavoidable.

Patches LGTM. I could go along with removing those entries from the
.dat files in the older branches, but not doing so is fine too.

regards, tom lane

On Tue, Oct 6, 2026 at 3:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Patches LGTM. I could go along with removing those entries from the
.dat files in the older branches, but not doing so is fine too.

I pushed this, but see a XversionUpgrade-REL_18_STABLE-REL_19_STABLE
buildfarm failure on crake. It looks like crake's amcheck run
correctly complains when a bpchar index is upgraded. This must be due
to it using a stale version of REL_18_STABLE with the latest
REL_19_STABLE.

I imagine that the failure will stop soon, once crake starts using a
REL_18_STABLE with the fix. Does that sound right?

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

I pushed this, but see a XversionUpgrade-REL_18_STABLE-REL_19_STABLE
buildfarm failure on crake. It looks like crake's amcheck run
correctly complains when a bpchar index is upgraded. This must be due
to it using a stale version of REL_18_STABLE with the latest
REL_19_STABLE.

Yeah, it's visibly run 19 and HEAD before the older branches, which
seems odd. Usually the BF client does the branches back-to-front.

regards, tom lane

On Tue, Oct 6, 2026 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Actually, we do have a precedent for fixing this: it's to drop
tables or indexes that the newer version gets indigestion on.
See adjust_database_contents in AdjustUpgrade.pm.

I just found that myself; commit b352d3d8 had to do this.

I'm about to go to dinner, but I can work on fixing this afterwards
if you don't want to.

I'll see where I get with following this precedent now.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

On Tue, Oct 6, 2026 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm about to go to dinner, but I can work on fixing this afterwards
if you don't want to.

I'll see where I get with following this precedent now.

If you like, but my experience is that getting this right is pretty
painful if you don't have a cross-version-upgrade-testing BF animal
to experiment on. I do ...

regards, tom lane

On Tue, Oct 6, 2026 at 8:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

If you like, but my experience is that getting this right is pretty
painful if you don't have a cross-version-upgrade-testing BF animal
to experiment on. I do ...

Something like the attached seems like it'd do the job.

But, yeah, realistically, the hard part is proper testing and
validation, which I lack the ability to do myself.

--
Peter Geoghegan

Hi Peter, Tom,

Thanks for pushing this and for taking care of the upgrade test failure.

I was not around when the crake failure came up, so I want to explain.
I work a day job in EST, and I do PostgreSQL work
mostly outside those hours. I usually read the list in the evening, and
a reply from me can take until then. It is not that I am ignoring the
thread.

I saw Peter's 0001-Fix-XversionUpgrade-allequalimage-failures.patch, and
the AdjustUpgrade.pm approach looks right to me. Peter said validating
it is the hard part. Let me try to play with this patch.

Thanks,
Shihao

Hi Peter,

Something like the attached seems like it'd do the job.

I tried it. I took a pg_dumpall from a 13 cluster after the
regression tests, ran 002_pg_upgrade.pl on master with it, and then
ran amcheck on the upgraded cluster. Without the patch amcheck
complains with the following. With the patch it is clean.

`ERROR: index "ctl_bpchar" metapage incorrectly indicates that
deduplication is safe`

Thanks,
Shihao