Skip to content

fix: retire old storage catalog rows on TRUNCATE - #867

Draft
OffgridwithJD wants to merge 1 commit into
mainfrom
audit/truncate-metadata-leak
Draft

fix: retire old storage catalog rows on TRUNCATE#867
OffgridwithJD wants to merge 1 commit into
mainfrom
audit/truncate-metadata-leak

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Summary

  • TRUNCATE installs a new relfilenode and storage id. Catalog rows for the retired id were left behind because DROP only deletes the current id.
  • Rewrite now deletes the old storage tree (including projections) while the previous fork is still attached.

Test coverage

  • test/truncate_cleanup.sh
  • test/drop_cleanup.sh (unchanged, still passes)

Made with Cursor

Co-authored-by: Cursor <cursoragent@cursor.com>
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

The premise is real — reproduced

I did not take the leak from the description. Measured on pg18a, unpatched main,
one columnar table, counting the columnar catalog directly:

after INSERT     row_group=1 column_chunk=2 zone_map=6 bloom=2 storage=1   distinct storage ids: 1
after TRUNCATE   row_group=1 column_chunk=2 zone_map=6 bloom=2 storage=1   distinct storage ids: 1
after re-INSERT  row_group=2 column_chunk=4 zone_map=10 bloom=4 storage=2  distinct storage ids: 2
after DROP       row_group=1 column_chunk=2 zone_map=6 bloom=2 storage=1   distinct storage ids: 1

LEFTOVER ROWS AFTER DROP: 1.

So the retired storage survives the TRUNCATE, a second id appears alongside it on
the next write, and DROP removes only the current one. A catalog row outlives the
table that owned it, and it leaks once per TRUNCATE rather than once per table.
zone_map and bloom leak with it. The description is accurate.

Worth putting these numbers in the suite header: they are what makes the fix
checkable by someone who did not write it.

The one thing I would want pinned

set_new_filelocator is not reached only by TRUNCATE, and the guard is what
decides whether an unrelated caller loses its catalog rows:

oldsrel = RelationGetSmgr(rel);
if (smgrexists(oldsrel, MAIN_FORKNUM) &&
    smgrnblocks(oldsrel, MAIN_FORKNUM) >= 2)
    pgcolumnar_delete_storage_tree(PgColumnarStorageId(rel));

Reading PostgreSQL's callers, the rewrite paths I would worry about —
ALTER TABLE ... ALTER COLUMN TYPE, VACUUM FULL, CLUSTER — all build a new
heap via make_new_heap and swap, so the callback runs against a relation with
no existing main fork and the guard is false. CREATE TABLE likewise. That
reasoning says the change is confined to TRUNCATE, and it matches the comment.

But that is me reading call sites, not running them. The suite asserts TRUNCATE.
I would add one arm for a rewrite that must NOT lose its data — the cheapest is:

CREATE TABLE r (id int, v text) USING pgcolumnar;
INSERT INTO r SELECT g, 'x'||g FROM generate_series(1,5000) g;
ALTER TABLE r ALTER COLUMN v TYPE varchar(64);   -- full rewrite
-- must still be 5000, and the rows must still be readable

If the guard ever misfires on a rewrite, the failure mode is silent data loss
rather than a leak, so it is the arm worth having. drop_cleanup.sh passing does
not cover it, because nothing there rewrites.

Two smaller notes:

  • The helper is a clean extraction — the object_access path and the new caller
    now share one implementation rather than two copies that could drift.
  • smgrnblocks(...) >= 2 encodes "metapage + reserved" as a bare literal. A
    named constant, or a comment naming the block layout it depends on, would keep
    it honest if the metapage ever grows.

Reviewed as OffgridwithJD. Not approving — same account as the author.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants