Skip to content

A rewrite through set_new_filelocator must re-record projections, as the compaction paths already do (#876) #887

Description

@jdatcmd

The fix for #876. That issue reports the symptom — a declared projection reads as 42704 after a
rewrite — and 1.0-alpha3 shipped only a HINT naming pgcolumnar.rebuild_projections(). This is
the work that removes the need for the hint.

The gap is one function, and there is already a working model beside it

Four sites call PgColumnarNextStorageId(). Three of them handle projections correctly:

site function re-records?
columnar_vacuum.c:1361 pgcolumnar_compact_relation yesPgColumnarInsertProjectionRow at :1362
columnar_vacuum.c:1542 pgcolumnar_compact_relation_zorder yes — at :1543
columnar_projection.c:251 pgcolumnar_add_projection n/a — mints the projection's own storage
columnar_tableam.c:845 pgcolumnar_relation_set_new_filelocator no

So this is not a design question. Two paths already re-record projections under the new storage
id, in this same tree, and the table-AM rewrite callback does not.

Measured on main 9628414, each operation on a freshly built table

operation                      storage id            read_projection
pgcolumnar.vacuum()            changes               ok
pgcolumnar.compact_rewrite()   unchanged             ok
pgcolumnar.vacuum_sorted()     unchanged             ok
pgcolumnar.recluster()         unchanged             ok
CLUSTER ... USING              unchanged             ok
TRUNCATE                       changes               42704
ALTER TABLE ... ALTER COLUMN   changes               42704
                   ... TYPE

Every failure goes through pgcolumnar_relation_set_new_filelocator. Every operation that
either keeps the storage id or re-records survives.

A note for anyone reading #876's reproduction, because it looks like it contradicts the
table above: that reproduction is sequential. It runs TRUNCATE and then
pgcolumnar.vacuum(), so the vacuum row inherits breakage the TRUNCATE already caused. Isolated
on its own table, pgcolumnar.vacuum() is fine. #876 is not wrong; it is cumulative.

What to do

In pgcolumnar_relation_set_new_filelocator, after minting the new storage id and writing the
metapage, re-record the relation's projections under the new id — the same thing
pgcolumnar_compact_relation does at columnar_vacuum.c:1362.

pgcolumnar.rebuild_projections() already contains a working implementation of the
re-recording, so there may be a shared helper here rather than a third copy.

Tests it needs, and one trap

Write the arm first and watch it go red. TRUNCATE and ALTER COLUMN ... TYPE are the two
demonstrated shapes; both belong in the suite.

The trap: an arm that only asserts read_projection returns rows will pass on a tree where
the projection was re-recorded but is EMPTY. Assert the row count and the contents, not merely
that the call did not raise. The projection must hold the same live rows as the base table after
the rewrite, which is what test/projections.sh already compares elsewhere with
pgc_set_hash.

Also worth an arm: the retired storage's projection rows should not survive alongside the new
ones. #867 fixed the equivalent leak for the storage tree.

Once this lands

The HINT added in 1.0-alpha3 becomes stale for these paths and should be narrowed or removed,
and RELEASE_NOTES/docs entries that describe the workaround will need revisiting. The known
issue in RELEASE_NOTES_1.0-alpha3.md says #876 "can affect you today", which is the sentence
that would change.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions