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