From e28824fae114277a394ab7193fa0ab6337329eb9 Mon Sep 17 00:00:00 2001 From: Peter Smith Date: Thu, 9 Sep 2021 13:12:17 +1000 Subject: [PATCH v28] Row filter validation - replica identity This patch introduces some additional row filter validation. Currently it is implemented only for the publish mode "delete" and it validates that any columns referenced in the filter expression must be part of REPLICA IDENTITY or Primary Key. Also updated test code. Discussion: https://www.postgresql.org/message-id/CAA4eK1Kyax-qnVPcXzODu3JmA4vtgAjUSYPUK1Pm3vBL5gC81g%40mail.gmail.com --- src/backend/catalog/dependency.c | 58 ++++++++++++++++++ src/backend/catalog/pg_publication.c | 74 +++++++++++++++++++++++ src/include/catalog/dependency.h | 6 ++ src/test/regress/expected/publication.out | 97 +++++++++++++++++++++++++++++-- src/test/regress/sql/publication.sql | 78 ++++++++++++++++++++++++- src/test/subscription/t/025_row_filter.pl | 7 +-- 6 files changed, 310 insertions(+), 10 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 91c3e97..e81f093 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -1554,6 +1554,64 @@ ReleaseDeletionLock(const ObjectAddress *object) } /* + * Find all the columns referenced by the row-filter expression and return what + * is found as a list of RfCol. This list is used for row-filter validation. + */ +List * +rowfilter_find_cols(Node *expr, Oid relId) +{ + find_expr_references_context context; + RangeTblEntry rte; + int ref; + List *rfcol_list = NIL; + + context.addrs = new_object_addresses(); + + /* We gin up a rather bogus rangetable list to handle Vars */ + MemSet(&rte, 0, sizeof(rte)); + rte.type = T_RangeTblEntry; + rte.rtekind = RTE_RELATION; + rte.relid = relId; + rte.relkind = RELKIND_RELATION; /* no need for exactness here */ + rte.rellockmode = AccessShareLock; + + context.rtables = list_make1(list_make1(&rte)); + + /* Scan the expression tree for referenceable objects */ + find_expr_references_walker(expr, &context); + + /* Remove any duplicates */ + eliminate_duplicate_dependencies(context.addrs); + + /* Build/Return the list of columns referenced by this Row Filter */ + for (ref = 0; ref < context.addrs->numrefs; ref++) + { + ObjectAddress *thisobj = context.addrs->refs + ref; + + if (thisobj->classId == RelationRelationId) + { + RfCol *rfcol; + + /* + * The parser already took care of ensuring columns must be from + * the correct table. + */ + Assert(thisobj->objectId == relId); + + rfcol = palloc(sizeof(RfCol)); + rfcol->name = get_attname(thisobj->objectId, thisobj->objectSubId, false); + rfcol->attnum = thisobj->objectSubId; + + rfcol_list = lappend(rfcol_list, rfcol); + } + } + + free_object_addresses(context.addrs); + + return rfcol_list; +} + +/* * recordDependencyOnExpr - find expression dependencies * * This is used to find the dependencies of rules, constraint expressions, diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index a1ea0f8..ff2f28d 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -139,6 +139,77 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS) PG_RETURN_BOOL(result); } +/* + * Walk the parse-tree to decide if the row-filter is valid or not. + */ +static void +rowfilter_expr_checker(Publication *pub, Node *rfnode, Relation rel) +{ + Oid relid = RelationGetRelid(rel); + char *relname = RelationGetRelationName(rel); + + /* + * Rule: + * + * If the publish operation contains "delete" then only columns that + * are allowed by the REPLICA IDENTITY rules are permitted to be used in + * the row-filter WHERE clause. + * + * TODO - check later for publish "update" case. + */ + if (pub->pubactions.pubdelete) + { + char replica_identity = rel->rd_rel->relreplident; + + if (replica_identity == REPLICA_IDENTITY_FULL) + { + /* + * FULL means all cols are in the REPLICA IDENTITY, so all cols are + * allowed in the row-filter too. + */ + } + else + { + List *rfcols; + ListCell *lc; + Bitmapset *bms_okcols; + + /* + * Find what are the cols that are part of the REPLICA IDENTITY. + * Note that REPLICA IDENTIY DEFAULT means primary key or nothing. + */ + if (replica_identity == REPLICA_IDENTITY_DEFAULT) + bms_okcols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY); + else + bms_okcols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY); + + /* + * Find what cols are referenced in the row filter WHERE clause, + * and validate that each of those referenced cols is allowed. + */ + rfcols = rowfilter_find_cols(rfnode, relid); + foreach(lc, rfcols) + { + RfCol *rfcol = lfirst(lc); + char *colname = rfcol->name; + int attnum = rfcol->attnum; + + if (!bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber, bms_okcols)) + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), + errmsg("cannot add relation \"%s\" to publication", + relname), + errdetail("Row filter column \"%s\" is not part of the REPLICA IDENTITY", + colname))); + } + } + + bms_free(bms_okcols); + list_free_deep(rfcols); + } + } +} /* * Insert new publication / relation mapping. @@ -204,6 +275,9 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, /* Fix up collation information */ assign_expr_collations(pstate, whereclause); + + /* Validate the row-filter. */ + rowfilter_expr_checker(pub, whereclause, targetrel); } /* Form a tuple. */ diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h index 2885f35..2c7310e 100644 --- a/src/include/catalog/dependency.h +++ b/src/include/catalog/dependency.h @@ -151,6 +151,12 @@ extern void performDeletion(const ObjectAddress *object, extern void performMultipleDeletions(const ObjectAddresses *objects, DropBehavior behavior, int flags); +typedef struct RfCol { + char *name; + int attnum; +} RfCol; +extern List *rowfilter_find_cols(Node *expr, Oid relId); + extern void recordDependencyOnExpr(const ObjectAddress *depender, Node *expr, List *rtable, DependencyType behavior); diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 156f14c..aa97e4d 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -163,13 +163,15 @@ CREATE TABLE testpub_rf_tbl2 (c text, d integer); CREATE TABLE testpub_rf_tbl3 (e integer); CREATE TABLE testpub_rf_tbl4 (g text); SET client_min_messages = 'ERROR'; -CREATE PUBLICATION testpub5 FOR TABLE testpub_rf_tbl1, testpub_rf_tbl2 WHERE (c <> 'test' AND d < 5); +-- Firstly, test using the option publish="insert" because the row filter +-- validation of referenced columns is less strict than for delete/update. +CREATE PUBLICATION testpub5 FOR TABLE testpub_rf_tbl1, testpub_rf_tbl2 WHERE (c <> 'test' AND d < 5) WITH (publish = "insert"); RESET client_min_messages; \dRp+ testpub5 Publication testpub5 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root --------------------------+------------+---------+---------+---------+-----------+---------- - regress_publication_user | f | t | t | t | t | f + regress_publication_user | f | t | f | f | f | f Tables: "public.testpub_rf_tbl1" "public.testpub_rf_tbl2" WHERE (((c <> 'test'::text) AND (d < 5))) @@ -179,7 +181,7 @@ ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl3 WHERE (e > 1000 AND e < 200 Publication testpub5 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root --------------------------+------------+---------+---------+---------+-----------+---------- - regress_publication_user | f | t | t | t | t | f + regress_publication_user | f | t | f | f | f | f Tables: "public.testpub_rf_tbl1" "public.testpub_rf_tbl2" WHERE (((c <> 'test'::text) AND (d < 5))) @@ -190,7 +192,7 @@ ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl2; Publication testpub5 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root --------------------------+------------+---------+---------+---------+-----------+---------- - regress_publication_user | f | t | t | t | t | f + regress_publication_user | f | t | f | f | f | f Tables: "public.testpub_rf_tbl1" "public.testpub_rf_tbl3" WHERE (((e > 1000) AND (e < 2000))) @@ -201,7 +203,7 @@ ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (e > 300 AND e < 500) Publication testpub5 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root --------------------------+------------+---------+---------+---------+-----------+---------- - regress_publication_user | f | t | t | t | t | f + regress_publication_user | f | t | f | f | f | f Tables: "public.testpub_rf_tbl3" WHERE (((e > 300) AND (e < 500))) @@ -229,6 +231,91 @@ DROP TABLE testpub_rf_tbl4; DROP PUBLICATION testpub5; DROP OPERATOR =#>(integer, integer); DROP FUNCTION testpub_rf_func(integer, integer); +-- ====================================================== +-- More row filter tests for validating column references +CREATE TABLE rf_tbl_abcd_nopk(a int, b int, c int, d int); +CREATE TABLE rf_tbl_abcd_pk(a int, b int, c int, d int, PRIMARY KEY(a,b)); +-- Case 1. REPLICA IDENTITY DEFAULT (means use primary key or nothing) +-- 1a. REPLICA IDENTITY is DEFAULT and table has a PK. +-- ok - "a" is a PK col +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99); +RESET client_min_messages; +DROP PUBLICATION testpub6; +-- ok - "b" is a PK col +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (b > 99); +RESET client_min_messages; +DROP PUBLICATION testpub6; +-- fail - "c" is not part of the PK +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +ERROR: cannot add relation "rf_tbl_abcd_pk" to publication +DETAIL: Row filter column "c" is not part of the REPLICA IDENTITY +-- fail - "d" is not part of the PK +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (d > 99); +ERROR: cannot add relation "rf_tbl_abcd_pk" to publication +DETAIL: Row filter column "d" is not part of the REPLICA IDENTITY +-- 1b. REPLICA IDENTITY is DEFAULT and table has no PK +-- fail - "a" is not part of REPLICA IDENTITY +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); +ERROR: cannot add relation "rf_tbl_abcd_nopk" to publication +DETAIL: Row filter column "a" is not part of the REPLICA IDENTITY +-- Case 2. REPLICA IDENTITY FULL +ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY FULL; +ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY FULL; +-- ok - "c" is in REPLICA IDENTITY now even though not in PK +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; +-- ok - "a" is in REPLICA IDENTITY now +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; +-- Case 3. REPLICA IDENTITY NOTHING +ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY NOTHING; +ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY NOTHING; +-- fail - "a" is in PK but it is not part of REPLICA IDENTITY NOTHING +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99); +ERROR: cannot add relation "rf_tbl_abcd_pk" to publication +DETAIL: Row filter column "a" is not part of the REPLICA IDENTITY +-- fail - "c" is not in PK and not in REPLICA IDENTITY NOTHING +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +ERROR: cannot add relation "rf_tbl_abcd_pk" to publication +DETAIL: Row filter column "c" is not part of the REPLICA IDENTITY +-- fail - "a" is not in REPLICA IDENTITY NOTHING +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); +ERROR: cannot add relation "rf_tbl_abcd_nopk" to publication +DETAIL: Row filter column "a" is not part of the REPLICA IDENTITY +-- Case 4. REPLICA IDENTITY INDEX +ALTER TABLE rf_tbl_abcd_pk ALTER COLUMN c SET NOT NULL; +CREATE UNIQUE INDEX idx_abcd_pk_c ON rf_tbl_abcd_pk(c); +ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY USING INDEX idx_abcd_pk_c; +ALTER TABLE rf_tbl_abcd_nopk ALTER COLUMN c SET NOT NULL; +CREATE UNIQUE INDEX idx_abcd_nopk_c ON rf_tbl_abcd_nopk(c); +ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY USING INDEX idx_abcd_nopk_c; +-- fail - "a" is in PK but it is not part of REPLICA IDENTITY INDEX +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99); +ERROR: cannot add relation "rf_tbl_abcd_pk" to publication +DETAIL: Row filter column "a" is not part of the REPLICA IDENTITY +-- ok - "c" is not in PK but it is part of REPLICA IDENTITY INDEX +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; +-- fail - "a" is not in REPLICA IDENTITY INDEX +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); +ERROR: cannot add relation "rf_tbl_abcd_nopk" to publication +DETAIL: Row filter column "a" is not part of the REPLICA IDENTITY +-- ok - "c" is part of REPLICA IDENTITY INDEX +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (c > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; +DROP TABLE rf_tbl_abcd_pk; +DROP TABLE rf_tbl_abcd_nopk; +-- ====================================================== -- Test cache invalidation FOR ALL TABLES publication SET client_min_messages = 'ERROR'; CREATE TABLE testpub_tbl4(a int); diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 331b821..8552b36 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -98,7 +98,9 @@ CREATE TABLE testpub_rf_tbl2 (c text, d integer); CREATE TABLE testpub_rf_tbl3 (e integer); CREATE TABLE testpub_rf_tbl4 (g text); SET client_min_messages = 'ERROR'; -CREATE PUBLICATION testpub5 FOR TABLE testpub_rf_tbl1, testpub_rf_tbl2 WHERE (c <> 'test' AND d < 5); +-- Firstly, test using the option publish="insert" because the row filter +-- validation of referenced columns is less strict than for delete/update. +CREATE PUBLICATION testpub5 FOR TABLE testpub_rf_tbl1, testpub_rf_tbl2 WHERE (c <> 'test' AND d < 5) WITH (publish = "insert"); RESET client_min_messages; \dRp+ testpub5 ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl3 WHERE (e > 1000 AND e < 2000); @@ -125,6 +127,80 @@ DROP PUBLICATION testpub5; DROP OPERATOR =#>(integer, integer); DROP FUNCTION testpub_rf_func(integer, integer); +-- ====================================================== +-- More row filter tests for validating column references +CREATE TABLE rf_tbl_abcd_nopk(a int, b int, c int, d int); +CREATE TABLE rf_tbl_abcd_pk(a int, b int, c int, d int, PRIMARY KEY(a,b)); + +-- Case 1. REPLICA IDENTITY DEFAULT (means use primary key or nothing) +-- 1a. REPLICA IDENTITY is DEFAULT and table has a PK. +-- ok - "a" is a PK col +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99); +RESET client_min_messages; +DROP PUBLICATION testpub6; +-- ok - "b" is a PK col +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (b > 99); +RESET client_min_messages; +DROP PUBLICATION testpub6; +-- fail - "c" is not part of the PK +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +-- fail - "d" is not part of the PK +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (d > 99); +-- 1b. REPLICA IDENTITY is DEFAULT and table has no PK +-- fail - "a" is not part of REPLICA IDENTITY +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); + +-- Case 2. REPLICA IDENTITY FULL +ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY FULL; +ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY FULL; +-- ok - "c" is in REPLICA IDENTITY now even though not in PK +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; +-- ok - "a" is in REPLICA IDENTITY now +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; + +-- Case 3. REPLICA IDENTITY NOTHING +ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY NOTHING; +ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY NOTHING; +-- fail - "a" is in PK but it is not part of REPLICA IDENTITY NOTHING +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99); +-- fail - "c" is not in PK and not in REPLICA IDENTITY NOTHING +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +-- fail - "a" is not in REPLICA IDENTITY NOTHING +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); + +-- Case 4. REPLICA IDENTITY INDEX +ALTER TABLE rf_tbl_abcd_pk ALTER COLUMN c SET NOT NULL; +CREATE UNIQUE INDEX idx_abcd_pk_c ON rf_tbl_abcd_pk(c); +ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY USING INDEX idx_abcd_pk_c; +ALTER TABLE rf_tbl_abcd_nopk ALTER COLUMN c SET NOT NULL; +CREATE UNIQUE INDEX idx_abcd_nopk_c ON rf_tbl_abcd_nopk(c); +ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY USING INDEX idx_abcd_nopk_c; +-- fail - "a" is in PK but it is not part of REPLICA IDENTITY INDEX +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99); +-- ok - "c" is not in PK but it is part of REPLICA IDENTITY INDEX +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; +-- fail - "a" is not in REPLICA IDENTITY INDEX +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99); +-- ok - "c" is part of REPLICA IDENTITY INDEX +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (c > 99); +DROP PUBLICATION testpub6; +RESET client_min_messages; + +DROP TABLE rf_tbl_abcd_pk; +DROP TABLE rf_tbl_abcd_nopk; +-- ====================================================== -- Test cache invalidation FOR ALL TABLES publication SET client_min_messages = 'ERROR'; diff --git a/src/test/subscription/t/025_row_filter.pl b/src/test/subscription/t/025_row_filter.pl index 6428f0d..dc9becc 100644 --- a/src/test/subscription/t/025_row_filter.pl +++ b/src/test/subscription/t/025_row_filter.pl @@ -19,6 +19,8 @@ $node_subscriber->start; $node_publisher->safe_psql('postgres', "CREATE TABLE tab_rowfilter_1 (a int primary key, b text)"); $node_publisher->safe_psql('postgres', + "ALTER TABLE tab_rowfilter_1 REPLICA IDENTITY FULL;"); +$node_publisher->safe_psql('postgres', "CREATE TABLE tab_rowfilter_2 (c int primary key)"); $node_publisher->safe_psql('postgres', "CREATE TABLE tab_rowfilter_3 (a int primary key, b boolean)"); @@ -223,9 +225,7 @@ $node_publisher->wait_for_catchup($appname); # - INSERT (1700, 'test 1700') YES, because 1700 > 1000 and 'test 1700' <> 'filtered' # - UPDATE (1600, NULL) NO, row filter evaluates to false because NULL is not <> 'filtered' # - UPDATE (1601, 'test 1601 updated') YES, because 1601 > 1000 and 'test 1601 updated' <> 'filtered' -# - DELETE (1700) NO, row filter contains column b that is not part of -# the PK or REPLICA IDENTITY and old tuple contains b = NULL, hence, row filter -# evaluates to false +# - DELETE (1700) YES, because 1700 > 1000 and 'test 1700' <> 'filtered' # $result = $node_subscriber->safe_psql('postgres', @@ -234,7 +234,6 @@ is($result, qq(1001|test 1001 1002|test 1002 1600|test 1600 1601|test 1601 updated -1700|test 1700 1980|not filtered), 'check replicated rows to table tab_rowfilter_1'); # Publish using root partitioned table -- 1.8.3.1