| From: | Clemenza Zhang <zxlmgsps2(at)gmail(dot)com> |
|---|---|
| To: | Matthias van de Meent <boekewurm(at)gmail(dot)com> |
| Cc: | Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org> |
| Subject: | Re: Bug: Whole-row var in indexes corrupts indexes after DDL |
| Date: | 2026-09-09 05:23:38 |
| Message-ID: | CAL9_+FFi8kGsqyQ6r8Aq+NaUqDWR=o_r9ji8wiaqJSvwp_zwFA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Matthias,
I tested v4 on current master and added a few tests beyond the existing
relfilenode checks.
The results look good so far.
For whole-row expression indexes, I tested table-shape changes through
ADD COLUMN, DROP COLUMN, and ALTER COLUMN TYPE. In each case the index
was rebuilt, and a lookup using the rebuilt whole-row expression index
returned the expected row. For the ADD COLUMN case I also checked the
plan explicitly and confirmed that the query used the expression
index, rather than getting the correct result through a sequential
scan.
I also checked a negative case: an ordinary index on an unrelated
column was not rebuilt when other columns were added or dropped.
The new whole-row dependency is present in pg_depend with
WholeRowAttrNumber as expected.
I retested the object-address issue from the previous version. The
table whole-row case now survives the
pg_identify_object_as_address() -> pg_get_object_address() round trip.
I also tested the corresponding foreign-table whole-row case, and that
round trip succeeds as well. An ordinary table-column round trip still
works, so I did not see a regression in the existing attribute path.
I also exercised a whole-row Var through an RTE_FUNCTION and did not
see the failure discussed earlier.
So I have not found any new correctness issue in these tests.
One possible test-coverage suggestion: the current indexing regression
test checks that relfilenode changes after ADD COLUMN, ALTER TYPE, and
DROP COLUMN. It might be worth also checking the result of an actual
lookup through the rebuilt expression index, since stale index contents
were the user-visible failure mode of the original bug.
Likewise, since v4 adds OBJECT_WHOLE_ROW specifically to address the
object-address issue, the table whole-row round-trip case might be
worth adding to the object-address regression tests.
Regards,
Clemenza Zhang
On Wed, Sep 9, 2026 at 3:08 AM Matthias van de Meent
<boekewurm(at)gmail(dot)com> wrote:
>
> On Fri, 4 Sept 2026 at 20:16, Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> >
> > Hi Matthias,
> >
> > I found one remaining issue in the object-address changes in v2/v3.
> > pg_identify_object_as_address() does not produce a value that can be
> > passed back to pg_get_object_address():
> >
> > WITH a AS (
> > SELECT *
> > FROM pg_identify_object_as_address(
> > 'pg_class'::regclass, 'wr_multi'::regclass, -32768)
> > )
> > SELECT pg_get_object_address(type, object_names, object_args)
> > FROM a;
> >
> > ERROR: unrecognized object type "table whole row"
> >
> > The documentation describes these functions as inverses.
>
> I'm not convinced they are that. pg_identify_object_as_address() will
> happily produce "<relkind> column" output, and with several relkinds
> these don't survive a round trip (specifically known failing cases:
> indexes, views; for specifics check objectaddress.c's ObjectTypeMap).
>
> But regardless, I've added support for decoding "table whole row" and
> "foreign table whole row", which were the only objects that also
> supported "<relkind> column", so that the hole isn't any smaller than
> that of direct attribute references.
>
> > I think we
> > either need to make "table whole row" an addressable object type, or use
> > another representation that pg_get_object_address() already understands.
> > This case would also make a useful addition to the object_address test.
>
> I've added a OBJECT_WHOLE_ROW type, treated like OBJECT_ATTRIBUTE but
> slightly differently, and wired it into some relevant places.
>
> > RememberWholeRowDependentForRebuilding() is a void function, but:
> >
> > + return RememberAllDependentForRebuilding(tab, subtype, rel,
> >
> > I think you can just call RememberAllDependentForRebuilding() without a
> > return.
>
> Adjusted.
>
> ----------------------------------------
>
> Attached is v4, which I believe addresses the feedback received so
> far. It does not fix the pre-existing ATPostAlterTypeCleanup issue.
>
>
> Kind regards,
>
> Matthias van de Meent
> Databricks (https://www.databricks.com)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Henson Choi | 2026-09-09 05:33:28 | Re: Row pattern recognition |
| Previous Message | Michael Paquier | 2026-09-09 05:23:35 | Re: pgbench: \gset and \aset should store SQL NULL as the null value |