Re: [PG19] eager aggregation gives wrong results because of bpchar_ops - Mailing list pgsql-hackers

From Tom Lane
Subject Re: [PG19] eager aggregation gives wrong results because of bpchar_ops
Date
Msg-id 1421797.1791305506@sss.pgh.pa.us
Whole thread
In response to [PG19] eager aggregation gives wrong results because of bpchar_ops  (shihao zhong <zhong950419@gmail.com>)
Responses Re: [PG19] eager aggregation gives wrong results because of bpchar_ops
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Masahiko Sawada
Date:
Subject: Re: parallel autovacuum: Propagate track_cost_delay_timing to parallel workers
Next
From: Peter Geoghegan
Date:
Subject: Re: [PG19] eager aggregation gives wrong results because of bpchar_ops