From e357430ad63a53b6a7261c67aff3194973486061 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 14 Oct 2026 22:49:19 +0000 Subject: [PATCH] fix: a rollup depends on its source, and dropping a rollup takes its partials and catalog rows (#1376) pgcolumnar.rollup held the view, its partials table and the source as bare OIDs. DROP TABLE of the source succeeded and left the rollup unrefreshable for good (42P01 on the dead OID; with the OID reused, another table's rows folded silently); DROP VIEW left the partials and the catalog rows behind. An AFTER INSERT OR UPDATE trigger on pgcolumnar.rollup now records the view on the source (NORMAL, as a view over the source would) and the partials on the view (AUTO). It is a trigger rather than a call in create_rollup so a restore that writes the rows back records them again (#1377). The view's rule already depends on the partials, so this is not an INTERNAL dependency. The trigger asks for SELECT on the source, as creating a view over it would: the catalog's row-level security only asks that the writer own the view. The object-access hook deletes a dropped rollup view's rows. rollup gains five checks in both twins, four red on main. Mutations without the normal dependency, the auto dependency, the hook cleanup or the SELECT check each redden their own arms. entry_point_privilege exempts the trigger function, which cannot be called directly. how-to.md says what dropping the source or the rollup does. cluster_tests 551 -> 552. Fixes #1376. The trigger also requires its writer to own the view and the partials, the rollup to be a view and the partials a table, and refuses an UPDATE that changes what a row names (#1406 review): a drop deletes AUTO dependents with no ownership check, so a row naming another role's table as its partials handed that table to the writer's DROP VIEW. Three more arms in both twins, red on the first version. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019VvWuX1zUwk6Y513waRd4N --- docs/how-to.md | 5 + pgcolumnar--1.0-alpha4--1.0-alpha5.sql | 8 ++ pgcolumnar--1.0-alpha5.sql | 8 ++ src/columnar_metadata.c | 30 +++++ src/columnar_metadata.h | 1 + src/columnar_tableam.c | 143 ++++++++++++++++++++++ test/check_ledger.tsv | 8 ++ test/check_ledger_budget.txt | 18 ++- test/entry_point_privilege.sh | 6 +- test/pytest/TESTS.md | 1 + test/pytest/expected_tests.txt | 4 +- test/pytest/test_entry_point_privilege.py | 5 +- test/pytest/test_rollup.py | 115 +++++++++++++++++ test/rollup.sh | 72 +++++++++++ 14 files changed, 420 insertions(+), 4 deletions(-) diff --git a/docs/how-to.md b/docs/how-to.md index dfcea6e1c..dc8104e4e 100644 --- a/docs/how-to.md +++ b/docs/how-to.md @@ -663,6 +663,11 @@ If the source is rewritten, for example by `pgcolumnar.vacuum`, `refresh_rollup` refuses with `55000`. If the rollup was current before the rewrite, run `pgcolumnar.rebase_rollup` to record the rewritten table and continue. +**A rollup depends on its source.** `DROP TABLE` of the source is refused while +a rollup reads from it, as it is for a view. `DROP TABLE ... CASCADE` drops the +rollup too. To remove only the rollup, run `DROP VIEW` on it, which also drops +its partials table and its catalog rows. + Run `refresh_rollup` at READ COMMITTED. A role that is not a superuser needs `USAGE` on schema `pgcolumnar`, which a superuser grants. Each role then sees and refreshes only the rollups that it owns. diff --git a/pgcolumnar--1.0-alpha4--1.0-alpha5.sql b/pgcolumnar--1.0-alpha4--1.0-alpha5.sql index e4ef6d201..bb2bee9a7 100644 --- a/pgcolumnar--1.0-alpha4--1.0-alpha5.sql +++ b/pgcolumnar--1.0-alpha4--1.0-alpha5.sql @@ -1222,6 +1222,14 @@ CREATE POLICY rollup_group_owner ON pgcolumnar.rollup_group (SELECT c.relowner FROM pg_class c WHERE c.oid = rollup), 'USAGE')); GRANT SELECT, INSERT, UPDATE, DELETE ON pgcolumnar.rollup, pgcolumnar.rollup_group TO PUBLIC; +-- The view depends on its source, and the partials on the view, recorded by this +-- trigger whenever a rollup row is written, a restore included (#1376, #1377). +CREATE FUNCTION pgcolumnar.rollup_record_dependencies() RETURNS trigger + LANGUAGE C AS 'MODULE_PATHNAME', 'pgcolumnar_rollup_record_dependencies'; +CREATE TRIGGER rollup_dependencies + AFTER INSERT OR UPDATE OF rollup, partials, source ON pgcolumnar.rollup + FOR EACH ROW EXECUTE FUNCTION pgcolumnar.rollup_record_dependencies(); + /* * The source's row groups, with each one's deleted count. SECURITY DEFINER * because row_group and delete_vector carry no GRANT; the caller must be able to diff --git a/pgcolumnar--1.0-alpha5.sql b/pgcolumnar--1.0-alpha5.sql index b697ee9cb..dd037cf2c 100644 --- a/pgcolumnar--1.0-alpha5.sql +++ b/pgcolumnar--1.0-alpha5.sql @@ -2412,6 +2412,14 @@ CREATE POLICY rollup_group_owner ON pgcolumnar.rollup_group (SELECT c.relowner FROM pg_class c WHERE c.oid = rollup), 'USAGE')); GRANT SELECT, INSERT, UPDATE, DELETE ON pgcolumnar.rollup, pgcolumnar.rollup_group TO PUBLIC; +-- The view depends on its source, and the partials on the view, recorded by this +-- trigger whenever a rollup row is written, a restore included (#1376, #1377). +CREATE FUNCTION pgcolumnar.rollup_record_dependencies() RETURNS trigger + LANGUAGE C AS 'MODULE_PATHNAME', 'pgcolumnar_rollup_record_dependencies'; +CREATE TRIGGER rollup_dependencies + AFTER INSERT OR UPDATE OF rollup, partials, source ON pgcolumnar.rollup + FOR EACH ROW EXECUTE FUNCTION pgcolumnar.rollup_record_dependencies(); + /* * The source's row groups, with each one's deleted count. SECURITY DEFINER * because row_group and delete_vector carry no GRANT; the caller must be able to diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 6c3263945..bd0120efb 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -4159,6 +4159,36 @@ PgColumnarDeleteOptions(Oid relid) table_close(rel, RowExclusiveLock); } +/* + * PgColumnarForgetRollup + * Delete a dropped rollup view's rows from pgcolumnar.rollup and + * pgcolumnar.rollup_group (#1376). Both are keyed by the view's OID in their + * first column. Before this, DROP VIEW left both behind, keyed to a dead OID + * that nothing could reach. + */ +void +PgColumnarForgetRollup(Oid viewOid) +{ + static const char *const catalogs[] = {"rollup", "rollup_group"}; + int i; + + for (i = 0; i < lengthof(catalogs); i++) + { + Relation rel = open_columnar_table(catalogs[i], RowExclusiveLock); + ScanKeyData key[1]; + SysScanDesc scan; + HeapTuple tuple; + + ScanKeyInit(&key[0], 1, BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(viewOid)); + scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + while (HeapTupleIsValid(tuple = systable_getnext(scan))) + CatalogTupleDelete(rel, &tuple->t_self); + systable_endscan(scan); + table_close(rel, RowExclusiveLock); + } +} + /* * PgColumnarIsColumnarRelation * Whether a relation uses the columnar table access method AND has diff --git a/src/columnar_metadata.h b/src/columnar_metadata.h index 905be11cd..ae389471b 100644 --- a/src/columnar_metadata.h +++ b/src/columnar_metadata.h @@ -83,6 +83,7 @@ extern NativeBloomMetadata *PgColumnarReadBloomForColumn(uint64 storageId, Snapshot snapshot); extern void PgColumnarDeleteOptions(Oid relid); +extern void PgColumnarForgetRollup(Oid viewOid); extern List *PgColumnarReadSortBy(Oid relid); diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index c5a0744ed..83ffed3c8 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -32,6 +32,10 @@ #include "catalog/storage.h" #include "commands/defrem.h" #include "commands/extension.h" +#include "commands/trigger.h" +#include "catalog/dependency.h" +#include "access/htup_details.h" +#include "utils/acl.h" #include "commands/vacuum.h" #include "executor/executor.h" #include "executor/tuptable.h" @@ -3715,6 +3719,134 @@ pgcolumnar_process_utility(PlannedStmt *pstmt, const char *queryString, } } +/* + * pgcolumnar_forget_dropped_rollup + * Delete the catalog rows of a dropped view if it was a rollup (#1376). Gated + * as the options cleanup below is: the hook fires in every database, including + * one without the extension, and DROP EXTENSION drops the extension's own views + * while the catalogs themselves are going. + */ +static void +pgcolumnar_forget_dropped_rollup(Oid viewOid) +{ + Oid columnarNsp = get_namespace_oid(COLUMNAR_SCHEMA_NAME, true); + Oid rollupOid = OidIsValid(columnarNsp) ? + get_relname_relid("rollup", columnarNsp) : InvalidOid; + Oid extOid = get_extension_oid("pgcolumnar", true); + + if (!OidIsValid(rollupOid)) + return; + if (OidIsValid(extOid) && + getExtensionOfObject(RelationRelationId, viewOid) == extOid) + return; + PgColumnarForgetRollup(viewOid); +} + +/* + * pgcolumnar_rollup_record_dependencies + * AFTER INSERT OR UPDATE trigger on pgcolumnar.rollup (#1376). The catalog + * holds the view, its partials table and the source as bare OIDs, so nothing + * stopped DROP TABLE of the source, after which the rollup could never be + * refreshed again (or, with the OID reused, folded another table's rows), and + * DROP VIEW left the partials and the catalog rows behind. It records: + * - the view on the source, NORMAL, as a view over the source would have: + * DROP TABLE of the source is refused, and CASCADE takes the rollup; + * - the partials on the view, AUTO: dropping the view takes its partials. + * The view's own rule already depends on the partials, which is why this is + * not an INTERNAL dependency: that would make DROP TABLE of the partials + * redirect to the view instead of refusing. + * + * A trigger rather than a call in create_rollup, so a restore that writes the + * catalog rows back records the dependencies again (#1377). The caller must + * hold SELECT on the source, as creating a view over it would require. + */ +PG_FUNCTION_INFO_V1(pgcolumnar_rollup_record_dependencies); + +Datum +pgcolumnar_rollup_record_dependencies(PG_FUNCTION_ARGS) +{ + TriggerData *trigdata = (TriggerData *) fcinfo->context; + HeapTuple tuple; + TupleDesc desc; + bool isnull; + Oid view; + Oid partials; + Oid source; + ObjectAddress viewAddr; + ObjectAddress partialsAddr; + ObjectAddress sourceAddr; + + if (!CALLED_AS_TRIGGER(fcinfo) || + !TRIGGER_FIRED_AFTER(trigdata->tg_event) || + !TRIGGER_FIRED_FOR_ROW(trigdata->tg_event)) + elog(ERROR, "pgcolumnar_rollup_record_dependencies: not called as an AFTER ROW trigger"); + + desc = RelationGetDescr(trigdata->tg_relation); + + /* + * NOTHING CHANGES WHAT A ROW NAMES ONCE IT IS WRITTEN (#1406 review). No + * function issues such an UPDATE, and re-recording would have to remove the + * old row's dependencies too; a no-op SET of the same values is let through. + */ + if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) + { + int att; + + for (att = 1; att <= 3; att++) + { + bool oldnull; + bool newnull; + Oid before = DatumGetObjectId(heap_getattr(trigdata->tg_trigtuple, att, + desc, &oldnull)); + Oid after = DatumGetObjectId(heap_getattr(trigdata->tg_newtuple, att, + desc, &newnull)); + + if (before != after) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("a rollup's view, partials and source cannot be changed"))); + } + return PointerGetDatum(NULL); + } + + tuple = trigdata->tg_trigtuple; + view = DatumGetObjectId(heap_getattr(tuple, 1, desc, &isnull)); + partials = DatumGetObjectId(heap_getattr(tuple, 2, desc, &isnull)); + source = DatumGetObjectId(heap_getattr(tuple, 3, desc, &isnull)); + + /* + * THE WRITER MUST OWN WHAT IT TIES TOGETHER (#1406 review). A drop deletes + * AUTO dependents with no ownership check, so a row naming another role's + * table as its partials would hand that table to the writer's DROP VIEW. The + * catalog's row-level security asks only that the writer own the view. + */ + if (get_rel_relkind(view) != RELKIND_VIEW) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("a rollup must be a view"))); + if (get_rel_relkind(partials) != RELKIND_RELATION) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("a rollup's partials must be a table"))); + if (!COLUMNAR_TABLE_OWNERCHECK(view)) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_VIEW, get_rel_name(view)); + if (!COLUMNAR_TABLE_OWNERCHECK(partials)) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, get_rel_name(partials)); + if (pg_class_aclcheck(source, GetUserId(), ACL_SELECT) != ACLCHECK_OK) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for table %s", get_rel_name(source)))); + + ObjectAddressSet(viewAddr, RelationRelationId, view); + ObjectAddressSet(partialsAddr, RelationRelationId, partials); + ObjectAddressSet(sourceAddr, RelationRelationId, source); + + recordDependencyOn(&viewAddr, &sourceAddr, DEPENDENCY_NORMAL); + recordDependencyOn(&partialsAddr, &viewAddr, DEPENDENCY_AUTO); + + return PointerGetDatum(NULL); +} + static void pgcolumnar_object_access(ObjectAccessType access, Oid classId, Oid objectId, int subId, void *arg) @@ -3729,6 +3861,17 @@ pgcolumnar_object_access(ObjectAccessType access, Oid classId, Oid objectId, { Relation rel; + /* + * A dropped rollup view takes its catalog rows with it (#1376). Its + * partials table goes by the auto dependency the rollup catalog's trigger + * records; these rows have no dependency of their own to go by. + */ + if (get_rel_relkind(objectId) == RELKIND_VIEW) + { + pgcolumnar_forget_dropped_rollup(objectId); + return; + } + /* * Ordinary tables AND materialized views. A columnar matview has * storage of its own, so it can carry an options row -- and until diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 8f6eb65d4..75d030e9d 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -2376,6 +2376,8 @@ rewrite_storage_oid rewrite_storage_oid the repair gate does not read a partitio rewrite_storage_oid rewrite_storage_oid the repair gate does not read a partition the ALTER did not touch: lp DETACH PARTITION lp_2 15;16;17;18;19 2026-10-12 the repair gate opens every inheritor with NoLock again, so it reads the metapage of a partition the ALTER never locked (an Assert on assert builds) rewrite_storage_oid rewrite_storage_oid the repair gate does not read a partition the ALTER did not touch: lp DETACH PARTITION lp_2 CONCURRENTLY 15;16;17;18;19 2026-10-12 the repair gate opens every inheritor with NoLock again, so it reads the metapage of a partition the ALTER never locked (an Assert on assert builds) rollup rollup * with anything but count is refused 15;16;17;18;19 2026-10-13 create_rollup allowing * with any function +rollup rollup DROP TABLE of a rollup's source is refused while the rollup exists (2BP01, #1376) 15;16;17;18;19 2026-10-14 a rollup's view, partials and source with no dependency between them, and no catalog cleanup (main before #1376's fix) +rollup rollup DROP VIEW of a rollup takes its partials and its catalog rows with it (#1376) 15;16;17;18;19 2026-10-14 a rollup's view, partials and source with no dependency between them, and no catalog cleanup (main before #1376's fix) rollup rollup a delete in a bucket that holds retired history is refused, and the rollup is unchanged 15;16;17;18;19 2026-10-13 refresh_rollup not storing the retired groups' ranges as history;refresh_rollup recording new groups without their time range;refresh_rollup without the history refusal rollup rollup a delete in a bucket whose history the same refresh retires is refused, and the rollup is unchanged 15;16;17;18;19 2026-10-13 refresh_rollup blocking on stored history only, not on groups the same refresh retires;refresh_rollup recording new groups without their time range;refresh_rollup without the history refusal rollup rollup a delete in a group that expire then retires is refused, and the rollup is unchanged 15;16;17;18;19 2026-10-13 expire recording a retired group's deleted count as 0;refresh_rollup not refusing a retiring group whose deleted count moved @@ -2390,8 +2392,12 @@ rollup rollup a refresh at REPEATABLE READ is refused 15;16;17;18;19 2026-10-13 rollup rollup a refresh folds a bulk append 15;16;17;18;19 2026-10-13 refresh_rollup folding every group, ignoring the recorded snapshot;refresh_rollup folding only the first new group;row_tid mapping 290 rows a block instead of the AM's rollup rollup a refresh in a zone with a DST change recomputes a whole day bucket 15;16;17;18;19 2026-10-13 refresh_rollup adding the width in the session's zone;refresh_rollup recomputing every bucket once any group changed;refresh_rollup recording new groups without their time range;refresh_rollup refusing any change but appends (phase 1, main before this branch) rollup rollup a refresh with nothing new folds nothing 15;16;17;18;19 2026-10-13 refresh_rollup folding every group, ignoring the recorded snapshot;refresh_rollup folding only the first new group +rollup rollup a role cannot name another role's table as its rollup's partials (42501), and that table survives its view's drop (#1376) 15;16;17;18;19 2026-10-14 the rollup trigger with no ownership, kind or no-change checks (8a7fab2, #1406 review) +rollup rollup a role cannot tie a rollup row to a table it cannot read (42501, #1376) 15;16;17;18;19 2026-10-14 a rollup's view, partials and source with no dependency between them, and no catalog cleanup (main before #1376's fix) rollup rollup a role that cannot read the source cannot create a rollup of it 15;16;17;18;19 2026-10-13 get_storage_id without its SELECT check, so create_rollup no longer refuses a caller that cannot read the source rollup rollup a rollup kept in a heap table refreshes and equals a full recompute 15;16;17;18;19 2026-10-13 create_rollup ignoring access_method;refresh_rollup folding only the first new group;row_tid mapping 290 rows a block instead of the AM's +rollup rollup a rollup row whose rollup is not a view is refused (42809, #1376) 15;16;17;18;19 2026-10-14 the rollup trigger with no ownership, kind or no-change checks (8a7fab2, #1406 review) +rollup rollup a rollup row's view, partials and source cannot be changed once written (0A000, #1376) 15;16;17;18;19 2026-10-14 the rollup trigger with no ownership, kind or no-change checks (8a7fab2, #1406 review) rollup rollup a rollup with NULL keys and NULL timestamps equals a full recompute 15;16;17;18;19 2026-10-13 refresh_rollup combining min with greatest;refresh_rollup folding every group, ignoring the recorded snapshot;refresh_rollup folding only the first new group;refresh_rollup matching keys with = instead of IS NOT DISTINCT FROM;row_tid mapping 290 rows a block instead of the AM's rollup rollup a sum over a column that cannot be folded is refused 15;16;17;18;19 2026-10-14 create_rollup without its cannot-be-folded check rollup rollup a table's owner, not a superuser, creates a rollup of it 15;16;17;18;19 2026-10-13 the rollup catalogs without their grants @@ -2405,6 +2411,7 @@ rollup rollup after compact_rewrite the rollup equals a full recompute 15;16;17; rollup rollup aggregate functions and columns of different lengths are refused 15;16;17;18;19 2026-10-13 create_rollup without the equal-length check rollup rollup an aggregate column the source does not have is refused 15;16;17;18;19 2026-10-13 create_rollup without the column-exists check rollup rollup an aggregate that does not combine is refused 15;16;17;18;19 2026-10-13 create_rollup without the five-function allow-list +rollup rollup and DROP TABLE ... CASCADE takes the rollup's view, partials and catalog rows with it (#1376) 15;16;17;18;19 2026-10-14 a rollup's view, partials and source with no dependency between them, and no catalog cleanup (main before #1376's fix) rollup rollup and after a delete the rollup equals a full recompute 15;16;17;18;19 2026-10-13 rebase_rollup recording no time ranges;refresh_rollup recomputing every bucket once any group changed;refresh_rollup refusing any change but appends (phase 1, main before this branch) rollup rollup and after retention an append still folds 15;16;17;18;19 2026-10-13 refresh_rollup folding every group, ignoring the recorded snapshot;refresh_rollup folding only the first new group;refresh_rollup not dropping groups retention retired;row_tid mapping 290 rows a block instead of the AM's rollup rollup and after the bulk append the rollup equals a full recompute 15;16;17;18;19 2026-10-13 refresh_rollup combining min with greatest;refresh_rollup folding every group, ignoring the recorded snapshot;refresh_rollup folding only the first new group;refresh_rollup recording the folded groups from a fresh read after the fold;row_tid mapping 290 rows a block instead of the AM's @@ -2430,6 +2437,7 @@ rollup rollup and the rollup still equals a full recompute 15;16;17;18;19 2026-1 rollup rollup another role cannot add a group row to the owner's rollup (42501) 15;16;17;18;19 2026-10-14 the rollup catalog policies given WITH CHECK (true) rollup rollup another role cannot read the source's groups through rollup_source_groups (42501) 15;16;17;18;19 2026-10-14 rollup_source_groups without its require_caller_select guard rollup rollup another role sees none of its rollup catalog rows 15;16;17;18;19 2026-10-13 the rollup catalogs without row-level security;the rollup catalogs without their grants +rollup rollup both rollups over the source refreshed, so each has a view, partials and catalog rows 15;16;17;18;19 never - rollup rollup each row group's TID range holds exactly that group's rows 15;16;17;18;19 2026-10-13 row_tid mapping 290 rows a block instead of the AM's rollup rollup nor its retired groups through rollup_source_retired (42501) 15;16;17;18;19 2026-10-14 rollup_source_retired without its require_caller_select guard rollup rollup premise: and the fixture has NULL-key and NULL-bucket groups 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index ee8b6d854..3616eb7f5 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -2210,4 +2210,20 @@ suites_not_covered 234 # never 2099 + 1 - 14 = 2086 # dated 644 + 14 = 658 # 2086 + 658 == 2744 -checks_never_observed_red 2086 +# +# #1376, a rollup and its source are dropped together (jdatcmd): 2086 -> 2087. rollup +# gains eight checks from five-major runs (90/90 on each of 15-19). Four are dated by +# main before the fix, where the view, partials and source had no dependency between +# them and nothing cleaned the catalog rows: DROP VIEW left the partials and rows, +# DROP TABLE of the source succeeded, CASCADE then had nothing to drop, and a role +# could tie a table it cannot read to its own view. Three more are dated by the first +# version of the trigger (#1406 review): it let a role name another role's table as +# its partials (and dropped that table with the role's view), a non-view as its +# rollup, and change a written row. The control that both rollups refreshed arrives +# never. +# +# ledger rows 2744 + 8 = 2752 +# never 2086 + 1 = 2087 +# dated 658 + 7 = 665 +# 2087 + 665 == 2752 +checks_never_observed_red 2087 diff --git a/test/entry_point_privilege.sh b/test/entry_point_privilege.sh index ea4e9c8a6..b7a28ea29 100755 --- a/test/entry_point_privilege.sh +++ b/test/entry_point_privilege.sh @@ -67,13 +67,17 @@ SQLFILE="$_epp_root/pgcolumnar--$_epp_ver.sql" # row_tid maps a row number to a TID by arithmetic (#1294): it # takes no relation and reads nothing. A version that # took a relation would need the SELECT check the others have. +# rollup_record_dependencies a TRIGGER function (#1376): PostgreSQL refuses to call it +# directly, and it runs only on writes to pgcolumnar.rollup, +# whose row-level security asks that the writer own the +# view; it checks SELECT on the source itself (rollup.sh). # # Kept on ONE line and matched after whitespace normalisation. A multi-line list # here is matched by `case " $EXEMPT " in *" $fn "*`, and the name sitting at a # line end is followed by a newline rather than a space, so it silently fails to # match and reads as an unbucketed function. That is not hypothetical: it is what # the first run of this suite reported for parquet_fdw_validator. -EXEMPT="columnar_handler parquet_fdw_handler parquet_fdw_validator iceberg_fdw_handler iceberg_fdw_validator iceberg_catalog_fdw_validator parquet_schema read_parquet file_split_offsets iceberg_current_snapshot iceberg_data_files iceberg_scan iceberg_rest_table_location iceberg_rest_scan iceberg_rest_namespaces iceberg_rest_tables read_avro_manifest read_manifest_list row_tid" +EXEMPT="columnar_handler parquet_fdw_handler parquet_fdw_validator iceberg_fdw_handler iceberg_fdw_validator iceberg_catalog_fdw_validator parquet_schema read_parquet file_split_offsets iceberg_current_snapshot iceberg_data_files iceberg_scan iceberg_rest_table_location iceberg_rest_scan iceberg_rest_namespaces iceberg_rest_tables read_avro_manifest read_manifest_list row_tid rollup_record_dependencies" EXEMPT="$(echo $EXEMPT)" # Relation-taking entry points with NO privilege check of any kind on main. diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index de625bea7..f80083149 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -6689,6 +6689,7 @@ Phase 1 removal proofs, each red on its arm: no `OFFSET 0` (the cost arm), no is | test | what it holds | | --- | --- | | `test_rollup_phase_one` | the rollup equals a full recompute after the first refresh, appends with late rows, a bulk append, and a rebase; a refresh with nothing new folds nothing; rows committed during a refresh are folded by the next, once; two concurrent refreshes fold each row once; zones agree; a refresh after a small append reads under a fifth of the source; a rewritten source is refused until `rebase_rollup`; after a delete, an update, a straddling append, `compact_rewrite` or a deleted NULL-ts row the rollup equals a full recompute, rewriting no partial outside the changed groups' range; a recompute that would reach retained history is refused with `0A000` and changes nothing; a day bucket is recomputed whole across a DST switch; a heap rollup behaves the same; REPEATABLE READ is refused; an owner who is not a superuser creates and refreshes a rollup, and another role sees none of it; bad definitions are refused with `22023` | +| `test_a_rollup_and_its_source_are_dropped_together` | the view depends on its source, as a view over it would (#1376): `DROP TABLE` of the source is refused (`2BP01`), `CASCADE` takes the view, its partials table and its catalog rows, and `DROP VIEW` takes the partials and the rows. A role that cannot read a table cannot tie a rollup row to it (`42501`), so it cannot block that table's drop. Nor can it name another role's table as its partials (`42501`, and that table survives its view's drop), or a non-view as its rollup (`42809`); and a written row's view, partials and source cannot be changed (`0A000`) | ## 98. test_timeseries_recipes.py: the gap-fill, LOCF and interpolation recipes answer what they claim diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index c00a69f00..a7a1befab 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -661,4 +661,6 @@ guard_tests 433 # cluster_tests 559 -> 562 for #1341's three privilege twins: test_debug_hook_privilege.py, # test_vacuum_lock_privilege.py and test_vm_privilege.py, one test each, by collection over # the cluster files on this tree. -cluster_tests 562 +# cluster_tests 562 -> 563 for #1376's test_a_rollup_and_its_source_are_dropped_together in +# test_rollup.py, by collection over the cluster files on this tree. +cluster_tests 563 diff --git a/test/pytest/test_entry_point_privilege.py b/test/pytest/test_entry_point_privilege.py index a86c81396..19c1210bb 100644 --- a/test/pytest/test_entry_point_privilege.py +++ b/test/pytest/test_entry_point_privilege.py @@ -31,7 +31,10 @@ "iceberg_fdw_validator", "iceberg_catalog_fdw_validator", "parquet_schema", "read_parquet", "file_split_offsets", "iceberg_current_snapshot", "iceberg_data_files", "iceberg_scan", "iceberg_rest_table_location", "iceberg_rest_scan", "iceberg_rest_namespaces", - "iceberg_rest_tables", "read_avro_manifest", "read_manifest_list", "row_tid"} + "iceberg_rest_tables", "read_avro_manifest", "read_manifest_list", "row_tid", + # A trigger function (#1376): never called directly, it runs on writes to + # pgcolumnar.rollup and checks SELECT on the source itself (test_rollup.py). + "rollup_record_dependencies"} # Relation-taking entry points with no privilege check of any kind: none, and a # name added here is a claim that something is ungated. KNOWN_UNGUARDED: set = set() diff --git a/test/pytest/test_rollup.py b/test/pytest/test_rollup.py index 5c7bd6b46..c729831e7 100644 --- a/test/pytest/test_rollup.py +++ b/test/pytest/test_rollup.py @@ -549,3 +549,118 @@ def refused(name, width, funcs, cols): "rebase_rollup of a table that is not a rollup is refused (42704)") expect.num(_one(conn, "SELECT count(*) FROM pg_class WHERE relname IN ('q_m', 'q_p', 'q_l', 'q_c', 'q_s', 'q_t', 'q_g', 'q_f')"), 0, "and no refused rollup left a table behind") + + +def test_a_rollup_and_its_source_are_dropped_together(pgc_conn, pgc_cluster, expect): + """The catalog held the source and the view as bare OIDs (#1376). DROP TABLE of the + source succeeded and left the rollup unrefreshable for good (42P01 on the dead OID, + or a reused OID folded silently); DROP VIEW left the partials table and the catalog + rows behind. Now the view depends on the source as a view over it would, so the drop + is refused and CASCADE takes the rollup; and dropping the view takes the rest of it.""" + conn = pgc_conn + + def ddl(sql): + try: + with conn.cursor() as cur: + cur.execute(sql) + return "ok" + except psycopg.Error as e: + return e.sqlstate or "?" + + def left(oid, name): + return _one(conn, f"SELECT (SELECT count(*) FROM pg_class WHERE relname IN ('{name}', '{name}__partials'))" + f" || '|' || (SELECT count(*) FROM pgcolumnar.rollup WHERE rollup::oid = {oid})" + f" || '|' || (SELECT count(*) FROM pgcolumnar.rollup_group WHERE rollup::oid = {oid})") + + with conn.cursor() as cur: + cur.execute("CREATE TABLE lsrc (ts timestamptz, v int) USING pgcolumnar") + cur.execute(f"INSERT INTO lsrc SELECT '{ORIGIN}'::timestamptz + g * interval '7 minutes', g" + " FROM generate_series(1, 500) g") + for lr in ("lr_a", "lr_b"): + cur.execute(f"SELECT pgcolumnar.create_rollup('{lr}', 'lsrc', 'ts', '1 hour', '{ORIGIN}'," + " '{}', '{count}', '{*}')") + cur.execute(f"SELECT pgcolumnar.refresh_rollup('{lr}')") + a_oid = _one(conn, "SELECT 'lr_a'::regclass::oid") + b_oid = _one(conn, "SELECT 'lr_b'::regclass::oid") + + sums = _one(conn, "SELECT (SELECT sum(count) FROM lr_a) || '|' || (SELECT sum(count) FROM lr_b)") + a_parts = "|".join(left(a_oid, "lr_a").split("|")[:2]) + expect.text(f"{sums}|{a_parts}", "500|500|2|1", + "both rollups over the source refreshed, so each has a view, partials and catalog rows") + expect.text(f"{ddl('DROP VIEW lr_b')}|{left(b_oid, 'lr_b')}", "ok|0|0|0", + "DROP VIEW of a rollup takes its partials and its catalog rows with it (#1376)") + expect.text(f"{ddl('DROP TABLE lsrc')}|{_one(conn, 'SELECT count(*) FROM pg_class WHERE relname = ' + chr(39) + 'lsrc' + chr(39))}", + "2BP01|1", "DROP TABLE of a rollup's source is refused while the rollup exists (2BP01, #1376)") + expect.text(f"{ddl('DROP TABLE lsrc CASCADE')}|{left(a_oid, 'lr_a')}", "ok|0|0|0", + "and DROP TABLE ... CASCADE takes the rollup's view, partials and catalog rows with it (#1376)") + + # The trigger that records those dependencies writes to pg_depend for whoever writes + # a rollup row, and the catalog's row-level security only asks that they own the + # view. So it asks what a view over the source would: SELECT on the source. + schema = _one(conn, "SELECT current_schema()") + with conn.cursor() as cur: + cur.execute("DO $$ BEGIN CREATE ROLE lr_stranger_py LOGIN; " + "EXCEPTION WHEN duplicate_object THEN NULL; END $$") + cur.execute(f'GRANT USAGE, CREATE ON SCHEMA "{schema}" TO lr_stranger_py') + cur.execute("GRANT USAGE ON SCHEMA pgcolumnar TO lr_stranger_py") + cur.execute("CREATE TABLE lr_victim (ts timestamptz, v int) USING pgcolumnar") + cur.execute("REVOKE ALL ON lr_victim FROM PUBLIC") + db = _one(conn, "SELECT current_database()") + try: + with psycopg.connect(f"host=127.0.0.1 port={pgc_cluster.port} user=lr_stranger_py dbname={db}", + autocommit=False) as c: + c.execute(f'SET search_path TO "{schema}", public') + c.execute("CREATE TABLE lr_mine (bucket timestamptz) USING pgcolumnar") + c.execute("CREATE VIEW lr_my_view AS SELECT bucket FROM lr_mine") + c.execute("INSERT INTO pgcolumnar.rollup (rollup, partials, source, source_storage, ts_column," + " width, origin, group_by, agg_functions, agg_columns) VALUES ('lr_my_view', 'lr_mine'," + f" 'lr_victim', 0, 'ts', '1 hour', '{ORIGIN}', '{{}}', '{{count}}', '{{*}}')") + c.commit() + tie = "ok" + except psycopg.Error as e: + tie = e.sqlstate or "?" + expect.text(f"{tie}|{ddl('DROP TABLE lr_victim')}", "42501|ok", + "a role cannot tie a rollup row to a table it cannot read (42501, #1376)") + + # The partials go with the view by an AUTO dependency, and a drop deletes AUTO + # dependents with no ownership check. So the trigger requires the writer to own + # the partials too, and the rollup to be a view and the partials a table. + with conn.cursor() as cur: + cur.execute("CREATE TABLE lr_others (bucket timestamptz) USING pgcolumnar") + cur.execute("GRANT SELECT ON lr_others TO lr_stranger_py") + cur.execute("CREATE TABLE lr_src2 (ts timestamptz, v int) USING pgcolumnar") + cur.execute("GRANT SELECT ON lr_src2 TO lr_stranger_py") + + def row(view, partials): + return ("INSERT INTO pgcolumnar.rollup (rollup, partials, source, source_storage, ts_column, width," + f" origin, group_by, agg_functions, agg_columns) VALUES ('{view}', '{partials}', 'lr_src2', 0," + f" 'ts', '1 hour', '{ORIGIN}', '{{}}', '{{count}}', '{{*}}')") + + def as_stranger(*stmts): + try: + with psycopg.connect(f"host=127.0.0.1 port={pgc_cluster.port} user=lr_stranger_py dbname={db}", + autocommit=False) as c: + c.execute(f'SET search_path TO "{schema}", public') + for st in stmts: + c.execute(st) + c.commit() + return "ok" + except psycopg.Error as e: + return e.sqlstate or "?" + + named = as_stranger("CREATE TABLE lr_p2 (bucket timestamptz) USING pgcolumnar", + "CREATE VIEW lr_v2 AS SELECT bucket FROM lr_p2", row("lr_v2", "lr_others")) + dropped = as_stranger("DROP VIEW IF EXISTS lr_v2") + survives = _one(conn, "SELECT count(*) FROM pg_class WHERE relname = 'lr_others'") + expect.text(f"{named}|{dropped}|{survives}", "42501|ok|1", + "a role cannot name another role's table as its rollup's partials (42501), and that table survives its view's drop (#1376)") + expect.text(as_stranger("CREATE TABLE lr_notview (bucket timestamptz) USING pgcolumnar", + "CREATE TABLE lr_p4 (bucket timestamptz) USING pgcolumnar", row("lr_notview", "lr_p4")), + "42809", "a rollup row whose rollup is not a view is refused (42809, #1376)") + with conn.cursor() as cur: + cur.execute("CREATE TABLE lr_p5 (bucket timestamptz) USING pgcolumnar") + cur.execute("CREATE VIEW lr_v5 AS SELECT bucket FROM lr_p5") + cur.execute("CREATE TABLE lr_p6 (bucket timestamptz) USING pgcolumnar") + cur.execute(row("lr_v5", "lr_p5")) + expect.text(ddl("UPDATE pgcolumnar.rollup SET partials = 'lr_p6' WHERE rollup = 'lr_v5'::regclass"), "0A000", + "a rollup row's view, partials and source cannot be changed once written (0A000, #1376)") diff --git a/test/rollup.sh b/test/rollup.sh index 2ef271f42..3483c1d14 100755 --- a/test/rollup.sh +++ b/test/rollup.sh @@ -461,4 +461,76 @@ check "rebase_rollup of a table that is not a rollup is refused (42704)" \ check_num "and no refused rollup left a table behind" \ "$(q "SELECT count(*) FROM pg_class WHERE relname IN ('r_m', 'r_p', 'r_l', 'r_c', 'r_s', 'r_t', 'r_g', 'r_f');")" "0" +# ---- a rollup and its source are dropped together (#1376) ---------------------- +# +# The catalog held the source and the view as bare OIDs. DROP TABLE of the source +# succeeded and left the rollup unrefreshable for good (42P01 on the dead OID, or a +# reused OID folded silently); DROP VIEW left the partials table and the catalog rows +# behind. Now the view depends on the source as a view over it would, so the drop is +# refused and CASCADE takes the rollup; and dropping the view takes the rest of it. +q "CREATE TABLE lsrc (ts timestamptz, v int) USING pgcolumnar; + INSERT INTO lsrc SELECT '$ORIGIN'::timestamptz + g * interval '7 minutes', g FROM generate_series(1, 500) g;" >/dev/null +for lr in lr_a lr_b; do + psql_run "SELECT pgcolumnar.create_rollup('$lr', 'lsrc', 'ts', '1 hour', '$ORIGIN', '{}', '{count}', '{*}');" >/dev/null + psql_run "SELECT pgcolumnar.refresh_rollup('$lr');" >/dev/null +done +lr_a_oid="$(q "SELECT 'lr_a'::regclass::oid")" +lr_b_oid="$(q "SELECT 'lr_b'::regclass::oid")" +lr_left() { # lr_left OID NAME -> "||" + q "SELECT (SELECT count(*) FROM pg_class WHERE relname IN ('$2', '$2__partials')) || '|' || + (SELECT count(*) FROM pgcolumnar.rollup WHERE rollup::oid = $1) || '|' || + (SELECT count(*) FROM pgcolumnar.rollup_group WHERE rollup::oid = $1)" +} +check "both rollups over the source refreshed, so each has a view, partials and catalog rows" \ + "$(q "SELECT (SELECT sum(count) FROM lr_a) || '|' || (SELECT sum(count) FROM lr_b)")|$(lr_left "$lr_a_oid" lr_a | cut -d'|' -f1,2)" "500|500|2|1" +check "DROP VIEW of a rollup takes its partials and its catalog rows with it (#1376)" \ + "$(sqlstate "DROP VIEW lr_b")|$(lr_left "$lr_b_oid" lr_b)" "ok|0|0|0" +check "DROP TABLE of a rollup's source is refused while the rollup exists (2BP01, #1376)" \ + "$(sqlstate "DROP TABLE lsrc")|$(q "SELECT count(*) FROM pg_class WHERE relname = 'lsrc'")" "2BP01|1" +check "and DROP TABLE ... CASCADE takes the rollup's view, partials and catalog rows with it (#1376)" \ + "$(sqlstate "SET client_min_messages = warning; DROP TABLE lsrc CASCADE")|$(lr_left "$lr_a_oid" lr_a)" "ok|0|0|0" +# The trigger that records those dependencies writes to pg_depend for whoever writes +# a rollup row, and the row-level security on the catalog only asks that they own +# the view. So it asks what a view over the source would: SELECT on the source. +# Without that, a role could tie a table it cannot read to a view of its own and +# block the table owner's DROP TABLE. +psql_run "DROP ROLE IF EXISTS lr_stranger; CREATE ROLE lr_stranger LOGIN; + GRANT USAGE, CREATE ON SCHEMA public TO lr_stranger; GRANT USAGE ON SCHEMA pgcolumnar TO lr_stranger; + CREATE TABLE lr_victim (ts timestamptz, v int) USING pgcolumnar; REVOKE ALL ON lr_victim FROM PUBLIC;" +lr_tie="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U lr_stranger -d "$PGC_DB" \ + -X -At -v ON_ERROR_STOP=1 -v VERBOSITY=sqlstate -c "CREATE TABLE lr_mine (bucket timestamptz) USING pgcolumnar; + CREATE VIEW lr_my_view AS SELECT bucket FROM lr_mine; + INSERT INTO pgcolumnar.rollup (rollup, partials, source, source_storage, ts_column, width, origin, + group_by, agg_functions, agg_columns) + VALUES ('lr_my_view', 'lr_mine', 'lr_victim', 0, 'ts', '1 hour', '$ORIGIN', '{}', '{count}', '{*}');" 2>&1 >/dev/null)" +check "a role cannot tie a rollup row to a table it cannot read (42501, #1376)" \ + "$(st="$(sed -n 's/^ERROR: *\([0-9A-Z]\{5\}\).*/\1/p' <<<"$lr_tie" | head -1)"; echo "${st:-${lr_tie:+other}}")|$(sqlstate "DROP TABLE lr_victim")" "42501|ok" +# The partials table goes with the view by an AUTO dependency, and a drop deletes AUTO +# dependents with no ownership check. So the trigger requires the writer to own the +# partials too, and the rollup to be a view and the partials a table (#1406 review): +# otherwise a role names another role's table as its partials and drops its own view. +as_stranger() { # as_stranger SQL -> ok, the SQLSTATE it fails with, or other + local out st + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U lr_stranger -d "$PGC_DB" \ + -X -At -v ON_ERROR_STOP=1 -v VERBOSITY=sqlstate -c "$1" 2>&1 >/dev/null)" + [ -z "$out" ] && { echo ok; return; } + st="$(sed -n 's/^ERROR: *\([0-9A-Z]\{5\}\).*/\1/p' <<<"$out" | head -1)" + echo "${st:-other}" +} +psql_run "CREATE TABLE lr_others (bucket timestamptz) USING pgcolumnar; GRANT SELECT ON lr_others TO lr_stranger; + CREATE TABLE lr_src2 (ts timestamptz, v int) USING pgcolumnar; GRANT SELECT ON lr_src2 TO lr_stranger;" +lr_row() { # lr_row VIEW PARTIALS -> a rollup row over lr_src2 + echo "INSERT INTO pgcolumnar.rollup (rollup, partials, source, source_storage, ts_column, width, origin, + group_by, agg_functions, agg_columns) VALUES ('$1', '$2', 'lr_src2', 0, 'ts', '1 hour', '$ORIGIN', '{}', '{count}', '{*}');" +} +check "a role cannot name another role's table as its rollup's partials (42501), and that table survives its view's drop (#1376)" \ + "$(as_stranger "CREATE TABLE lr_p2 (bucket timestamptz) USING pgcolumnar; CREATE VIEW lr_v2 AS SELECT bucket FROM lr_p2; $(lr_row lr_v2 lr_others)")|$(as_stranger "SET client_min_messages = warning; DROP VIEW IF EXISTS lr_v2")|$(q "SELECT count(*) FROM pg_class WHERE relname = 'lr_others'")" \ + "42501|ok|1" +check "a rollup row whose rollup is not a view is refused (42809, #1376)" \ + "$(as_stranger "CREATE TABLE lr_notview (bucket timestamptz) USING pgcolumnar; CREATE TABLE lr_p4 (bucket timestamptz) USING pgcolumnar; $(lr_row lr_notview lr_p4)")" "42809" +psql_run "CREATE TABLE lr_p5 (bucket timestamptz) USING pgcolumnar; CREATE VIEW lr_v5 AS SELECT bucket FROM lr_p5; + CREATE TABLE lr_p6 (bucket timestamptz) USING pgcolumnar; $(lr_row lr_v5 lr_p5)" +check "a rollup row's view, partials and source cannot be changed once written (0A000, #1376)" \ + "$(sqlstate "UPDATE pgcolumnar.rollup SET partials = 'lr_p6' WHERE rollup = 'lr_v5'::regclass")" "0A000" + pgc_summary