From 63c32a70bc9aabd70af32116f9ba071d79045a3c Mon Sep 17 00:00:00 2001 From: Kai Franz Date: Mon, 30 Sep 2024 23:19:37 -0700 Subject: [PATCH] [BACKPORT 2.20][#23686] YSQL: Build relcache foreign key list from YB catcache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Original commit: 3e93354184529cbee6df27337b5a41d78e7eced0 / D37595 Today, when preloading is enabled (either `ysql_catalog_preload_additional_tables` or `ysql_catalog_preload_additional_table_list` is set), we preload the relcache, meaning that we load the relcache entry for every relation in one shot. When we do relcache preloading, we currently don't load foreign key lists—instead, we loading them on-demand. This means on a given connection, the first time each table with a foreign key is referenced, that connection has to fetch the foreign key list from the master leader. With this revision, we make two changes: (1) Modify `RelationGetFKeyList` so that it reads foreign keys from the Yugabyte-only `pg_constraint` catcache `YBCONSTRAINTRELIDTYPIDNAME`. Each time `RelationGetFKeyList` is invoked, it will do a `SearchCatCacheList` on this cache with the partial key `conrelid`. (2) Whenever we preload the `pg_constraint` cache, we also preload the list cache for the partial key `conrelid`. This has the effect that any calls to `RelationGetFKeyList` after preloading will be able to read from the cached list and will not have to go to master. (A lookup for a table created on a different connection after preloading will still need to go to master). Both of these changes are controlled by a new GUC `yb_enable_fkey_catcache` (+tserver gflag wrapper) that is on by default. Jira: DB-12593 Test Plan: ``` ./yb_build.sh --cxx-test pg_catalog_perf-test --gtest_filter "*ForeignKeyRelcachePreloadTest*" ``` Reviewers: myang Reviewed By: myang Subscribers: yql Differential Revision: https://phorge.dev.yugabyte.com/D38520 --- .../java/org/yb/pgsql/TestPgForeignKey.java | 38 +++++++++ .../src/backend/utils/cache/relcache.c | 38 ++++++--- .../src/backend/utils/cache/syscache.c | 21 +++++ src/postgres/src/backend/utils/misc/guc.c | 11 +++ .../src/backend/utils/misc/pg_yb_utils.c | 1 + src/postgres/src/include/pg_yb_utils.h | 5 ++ src/postgres/src/include/utils/guc.h | 1 + .../src/test/regress/expected/yb_dml.out | 75 ++++++++++++++++++ src/postgres/src/test/regress/sql/yb_dml.sql | 78 +++++++++++++++++++ src/yb/yql/pgwrapper/pg_catalog_perf-test.cc | 32 ++++++++ src/yb/yql/pgwrapper/pg_wrapper.cc | 3 + 11 files changed, 292 insertions(+), 11 deletions(-) diff --git a/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgForeignKey.java b/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgForeignKey.java index 814221156f84..62b711f3f346 100644 --- a/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgForeignKey.java +++ b/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgForeignKey.java @@ -322,4 +322,42 @@ public void testInsertConcurrencySerializableIsolation() throws Exception { testInsertConcurrency(Connection.TRANSACTION_SERIALIZABLE); } + @Test + public void testAddForeignKeyConcurrent() throws Exception { + try (Statement stmt = connection.createStatement(); + Connection ddlConnection = getConnectionBuilder().connect(); + Statement ddlStmt = ddlConnection.createStatement()) { + // Create two tables + ddlStmt.execute("CREATE TABLE parent(k int PRIMARY KEY)"); + ddlStmt.execute("CREATE TABLE child(k int)"); + + // Insert data into parent table + stmt.execute("INSERT INTO parent VALUES (1), (2), (3)"); + + // Start inserting data into child table + stmt.execute("INSERT INTO child VALUES (1)"); + stmt.execute("INSERT INTO child VALUES (2)"); + + // On DDL connection, add foreign key constraint + ddlStmt.execute("ALTER TABLE child ADD CONSTRAINT fk_child_parent " + + "FOREIGN KEY (k) REFERENCES parent(k)"); + + // Try inserting a valid row on DML connection + stmt.execute("INSERT INTO child VALUES (3)"); + + // Try inserting an invalid row on DML connection + runInvalidQuery(stmt, + "INSERT INTO child VALUES (4)", + "violates foreign key constraint \"fk_child_parent\""); + + // Try inserting an invalid row on the DDL connection + runInvalidQuery(ddlStmt, + "INSERT INTO child VALUES (4)", + "violates foreign key constraint \"fk_child_parent\""); + + // Verify the data + assertQuery(stmt, "SELECT k FROM child ORDER BY k", + new Row(1), new Row(2), new Row(3)); + } + } } diff --git a/src/postgres/src/backend/utils/cache/relcache.c b/src/postgres/src/backend/utils/cache/relcache.c index a53792c1e508..895f649d2997 100644 --- a/src/postgres/src/backend/utils/cache/relcache.c +++ b/src/postgres/src/backend/utils/cache/relcache.c @@ -90,12 +90,14 @@ #include "storage/smgr.h" #include "utils/array.h" #include "utils/builtins.h" +#include "utils/catcache.h" #include "utils/datum.h" #include "utils/fmgroids.h" #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/partcache.h" +#include "utils/relcache.h" #include "utils/relmapper.h" #include "utils/resowner_private.h" #include "utils/snapmgr.h" @@ -1907,7 +1909,7 @@ YbCompleteAttrProcessingImpl(const YbAttrProcessorState *state) { pfree(constr); relation->rd_att->constr = NULL; - } + } /* Fetch rules and triggers that affect this relation */ if (relation->rd_rel->relhasrules) @@ -2354,7 +2356,7 @@ YbGetPrefetchableTableInfoImpl(YbPFetchTable table) { RelationRelationId, RELOID, RELNAMENSP }; case YB_PFETCH_TABLE_PG_CONSTRAINT: return (YbPFetchTableInfo) - { ConstraintRelationId, CONSTROID, YB_INVALID_CACHE_ID }; + { ConstraintRelationId, CONSTROID, YBCONSTRAINTRELIDTYPIDNAME }; case YB_PFETCH_TABLE_PG_DATABASE: return (YbPFetchTableInfo) { DatabaseRelationId, DATABASEOID, YB_INVALID_CACHE_ID }; @@ -6145,6 +6147,8 @@ RelationGetFKeyList(Relation relation) HeapTuple htup; List *oldlist; MemoryContext oldcxt; + YbCatCListIterator iterator; + bool use_catcache; /* Quick exit if we already computed the list. */ if (relation->rd_fkeyvalid) @@ -6163,17 +6167,24 @@ RelationGetFKeyList(Relation relation) */ result = NIL; - /* Prepare to scan pg_constraint for entries having conrelid = this rel. */ - ScanKeyInit(&skey, + use_catcache = IsYugaByteEnabled() && yb_enable_fkey_catcache; + + if (use_catcache) + { + iterator = YbCatCListIteratorBegin(SearchSysCacheList1(YBCONSTRAINTRELIDTYPIDNAME, RelationGetRelid(relation))); + } + else + { + /* Prepare to scan pg_constraint for entries having conrelid = this rel. */ + ScanKeyInit(&skey, Anum_pg_constraint_conrelid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(relation))); - - conrel = heap_open(ConstraintRelationId, AccessShareLock); - conscan = systable_beginscan(conrel, ConstraintRelidTypidNameIndexId, true, + conrel = heap_open(ConstraintRelationId, AccessShareLock); + conscan = systable_beginscan(conrel, ConstraintRelidTypidNameIndexId, true, NULL, 1, &skey); - - while (HeapTupleIsValid(htup = systable_getnext(conscan))) + } + while (HeapTupleIsValid(htup = use_catcache ? YbCatCListIteratorGetNext(&iterator) : systable_getnext(conscan))) { Form_pg_constraint constraint = (Form_pg_constraint) GETSTRUCT(htup); ForeignKeyCacheInfo *info; @@ -6197,8 +6208,13 @@ RelationGetFKeyList(Relation relation) result = lappend(result, info); } - systable_endscan(conscan); - heap_close(conrel, AccessShareLock); + if (use_catcache) + YbCatCListIteratorFree(&iterator); + else + { + systable_endscan(conscan); + heap_close(conrel, AccessShareLock); + } /* Now save a copy of the completed list in the relcache entry. */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); diff --git a/src/postgres/src/backend/utils/cache/syscache.c b/src/postgres/src/backend/utils/cache/syscache.c index 4a123b8ae4cb..040980a21cca 100644 --- a/src/postgres/src/backend/utils/cache/syscache.c +++ b/src/postgres/src/backend/utils/cache/syscache.c @@ -1269,6 +1269,26 @@ YbPreloadCatalogCache(int cache_id, int idx_cache_id) } break; } + case CONSTROID: + { + /* + * Add a cache list for YBCONSTRAINTRELIDTYPIDNAME for lookup by conrelid only. + */ + if (!yb_enable_fkey_catcache) + { + is_add_to_list_required = false; + break; + } + if (dest_list) + { + HeapTuple ltp = llast(dest_list); + Form_pg_constraint ltp_struct = (Form_pg_constraint) GETSTRUCT(ltp); + Form_pg_constraint ntp_struct = (Form_pg_constraint) GETSTRUCT(ntp); + if (ntp_struct->conrelid != ltp_struct->conrelid) + dest_list = NIL; + } + break; + } default: is_add_to_list_required = false; break; @@ -1302,6 +1322,7 @@ YbPreloadCatalogCache(int cache_id, int idx_cache_id) switch(cache_id) { case PROCOID: + case CONSTROID: Assert(idx_cache); dest_cache = idx_cache; break; diff --git a/src/postgres/src/backend/utils/misc/guc.c b/src/postgres/src/backend/utils/misc/guc.c index 942fc1bb9871..843b7846cf7c 100644 --- a/src/postgres/src/backend/utils/misc/guc.c +++ b/src/postgres/src/backend/utils/misc/guc.c @@ -2395,6 +2395,17 @@ static struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, + { + {"yb_enable_fkey_catcache", PGC_USERSET, DEVELOPER_OPTIONS, + gettext_noop("Enable preloading of foreign key information into the relation cache."), + NULL, + GUC_NOT_IN_SAMPLE + }, + &yb_enable_fkey_catcache, + true, + NULL, NULL, NULL + }, + /* End-of-list marker */ { {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL diff --git a/src/postgres/src/backend/utils/misc/pg_yb_utils.c b/src/postgres/src/backend/utils/misc/pg_yb_utils.c index 5c154c8e20cf..cde1850116e6 100644 --- a/src/postgres/src/backend/utils/misc/pg_yb_utils.c +++ b/src/postgres/src/backend/utils/misc/pg_yb_utils.c @@ -1346,6 +1346,7 @@ bool yb_enable_base_scans_cost_model = false; int yb_wait_for_backends_catalog_version_timeout = 5 * 60 * 1000; /* 5 min */ bool yb_prefer_bnl = false; bool yb_explain_hide_non_deterministic_fields = false; +bool yb_enable_fkey_catcache = true; //------------------------------------------------------------------------------ // YB Debug utils. diff --git a/src/postgres/src/include/pg_yb_utils.h b/src/postgres/src/include/pg_yb_utils.h index 989d9d7056a5..19910856225f 100644 --- a/src/postgres/src/include/pg_yb_utils.h +++ b/src/postgres/src/include/pg_yb_utils.h @@ -526,6 +526,11 @@ extern bool yb_prefer_bnl; */ extern bool yb_explain_hide_non_deterministic_fields; +/* + * Enable preloading of foreign key information into the relation cache. + */ +extern bool yb_enable_fkey_catcache; + //------------------------------------------------------------------------------ // GUC variables needed by YB via their YB pointers. extern int StatementTimeout; diff --git a/src/postgres/src/include/utils/guc.h b/src/postgres/src/include/utils/guc.h index 8c87f92723a0..cc8263f4c04a 100644 --- a/src/postgres/src/include/utils/guc.h +++ b/src/postgres/src/include/utils/guc.h @@ -265,6 +265,7 @@ extern bool yb_bnl_optimize_first_batch; extern bool yb_bnl_enable_hashing; extern bool yb_lock_pk_single_rpc; +extern bool yb_enable_fkey_catcache; extern int temp_file_limit; diff --git a/src/postgres/src/test/regress/expected/yb_dml.out b/src/postgres/src/test/regress/expected/yb_dml.out index 4bdbf71eb37c..cf28130cdbf5 100644 --- a/src/postgres/src/test/regress/expected/yb_dml.out +++ b/src/postgres/src/test/regress/expected/yb_dml.out @@ -18,3 +18,78 @@ EXPLAIN (COSTS OFF) INSERT INTO GH_22967 VALUES ((EXISTS(SELECT 1) in (SELECT tr (6 rows) INSERT INTO GH_22967 VALUES ((EXISTS(SELECT 1) in (SELECT true))::INT4), (-10); +-- Test that foreign key constraints are enforced +CREATE TABLE customers ( + customer_id SERIAL PRIMARY KEY, + name VARCHAR(100) NOT NULL, + email VARCHAR(100) UNIQUE NOT NULL +); +CREATE TABLE orders ( + order_id SERIAL PRIMARY KEY, + order_date DATE NOT NULL, + amount DECIMAL(10, 2) NOT NULL, + customer_id INTEGER NOT NULL, + FOREIGN KEY (customer_id) REFERENCES customers(customer_id) +); +INSERT INTO customers (name, email) VALUES +('Alice Johnson', 'alice@example.com'), +('Bob Smith', 'bob@example.com'); +INSERT INTO orders (order_date, amount, customer_id) VALUES +('2023-10-01', 250.00, 1), +('2023-10-02', 450.50, 2); +-- Attempt to insert an order with a non-existent customer +INSERT INTO orders (order_date, amount, customer_id) VALUES +('2023-10-03', 300.00, 3); +ERROR: insert or update on table "orders" violates foreign key constraint "orders_customer_id_fkey" +DETAIL: Key (customer_id)=(3) is not present in table "customers". +-- Attempt to delete a customer that still has orders +DELETE FROM customers WHERE customer_id = 2; +ERROR: update or delete on table "customers" violates foreign key constraint "orders_customer_id_fkey" on table "orders" +DETAIL: Key (customer_id)=(2) is still referenced from table "orders". +-- Test cascading deletes +DROP TABLE orders; +CREATE TABLE orders_cascade ( + order_id SERIAL PRIMARY KEY, + order_date DATE NOT NULL, + amount DECIMAL(10, 2) NOT NULL, + customer_id INTEGER NOT NULL, + FOREIGN KEY (customer_id) REFERENCES customers(customer_id) ON DELETE CASCADE +); +INSERT INTO orders_cascade (order_date, amount, customer_id) VALUES +('2023-10-01', 250.00, 1), +('2023-10-02', 450.50, 2); +DELETE FROM customers WHERE customer_id = 2; +SELECT * FROM orders_cascade; + order_id | order_date | amount | customer_id +----------+------------+--------+------------- + 1 | 10-01-2023 | 250.00 | 1 +(1 row) + +-- Test adding foreign key constraint using ALTER TABLE ADD CONSTRAINT +CREATE TABLE customers_test ( + customer_id SERIAL PRIMARY KEY, + name VARCHAR(100) NOT NULL +); +CREATE TABLE orders_test ( + order_id SERIAL PRIMARY KEY, + customer_id INTEGER, + name VARCHAR(100) NOT NULL +); +-- Add foreign key constraint using ALTER TABLE +ALTER TABLE orders_test +ADD CONSTRAINT fk_orders_customers +FOREIGN KEY (customer_id) REFERENCES customers_test(customer_id); +-- Insert valid data +INSERT INTO customers_test (name) VALUES ('Customer 1'), ('Customer 2'); +INSERT INTO orders_test (customer_id, name) VALUES (1, 'Order 1'), (2, 'Order 2'); +-- Attempt to insert a child with a non-existent customer +INSERT INTO orders_test (customer_id, name) VALUES (3, 'Order 3'); +ERROR: insert or update on table "orders_test" violates foreign key constraint "fk_orders_customers" +DETAIL: Key (customer_id)=(3) is not present in table "customers_test". +-- Attempt to delete a customer that still has orders +DELETE FROM customers_test WHERE customer_id = 2; +ERROR: update or delete on table "customers_test" violates foreign key constraint "fk_orders_customers" on table "orders_test" +DETAIL: Key (customer_id)=(2) is still referenced from table "orders_test". +-- Test that invalidation of the foreign key cache works +ALTER TABLE orders_test DROP CONSTRAINT fk_orders_customers; +INSERT INTO orders_test (customer_id, name) VALUES (3, 'Order 3'); diff --git a/src/postgres/src/test/regress/sql/yb_dml.sql b/src/postgres/src/test/regress/sql/yb_dml.sql index 8fd3188e9cc2..2be808ee7098 100644 --- a/src/postgres/src/test/regress/sql/yb_dml.sql +++ b/src/postgres/src/test/regress/sql/yb_dml.sql @@ -9,3 +9,81 @@ CREATE TABLE GH_22967 (k INT4 PRIMARY KEY); EXPLAIN (COSTS OFF) INSERT INTO GH_22967 VALUES ((EXISTS(SELECT 1) in (SELECT true))::INT4), (-10); INSERT INTO GH_22967 VALUES ((EXISTS(SELECT 1) in (SELECT true))::INT4), (-10); + + +-- Test that foreign key constraints are enforced +CREATE TABLE customers ( + customer_id SERIAL PRIMARY KEY, + name VARCHAR(100) NOT NULL, + email VARCHAR(100) UNIQUE NOT NULL +); + +CREATE TABLE orders ( + order_id SERIAL PRIMARY KEY, + order_date DATE NOT NULL, + amount DECIMAL(10, 2) NOT NULL, + customer_id INTEGER NOT NULL, + FOREIGN KEY (customer_id) REFERENCES customers(customer_id) +); + +INSERT INTO customers (name, email) VALUES +('Alice Johnson', 'alice@example.com'), +('Bob Smith', 'bob@example.com'); + +INSERT INTO orders (order_date, amount, customer_id) VALUES +('2023-10-01', 250.00, 1), +('2023-10-02', 450.50, 2); + +-- Attempt to insert an order with a non-existent customer +INSERT INTO orders (order_date, amount, customer_id) VALUES +('2023-10-03', 300.00, 3); + +-- Attempt to delete a customer that still has orders +DELETE FROM customers WHERE customer_id = 2; + +-- Test cascading deletes +DROP TABLE orders; +CREATE TABLE orders_cascade ( + order_id SERIAL PRIMARY KEY, + order_date DATE NOT NULL, + amount DECIMAL(10, 2) NOT NULL, + customer_id INTEGER NOT NULL, + FOREIGN KEY (customer_id) REFERENCES customers(customer_id) ON DELETE CASCADE +); +INSERT INTO orders_cascade (order_date, amount, customer_id) VALUES +('2023-10-01', 250.00, 1), +('2023-10-02', 450.50, 2); +DELETE FROM customers WHERE customer_id = 2; +SELECT * FROM orders_cascade; + + +-- Test adding foreign key constraint using ALTER TABLE ADD CONSTRAINT +CREATE TABLE customers_test ( + customer_id SERIAL PRIMARY KEY, + name VARCHAR(100) NOT NULL +); + +CREATE TABLE orders_test ( + order_id SERIAL PRIMARY KEY, + customer_id INTEGER, + name VARCHAR(100) NOT NULL +); + +-- Add foreign key constraint using ALTER TABLE +ALTER TABLE orders_test +ADD CONSTRAINT fk_orders_customers +FOREIGN KEY (customer_id) REFERENCES customers_test(customer_id); + +-- Insert valid data +INSERT INTO customers_test (name) VALUES ('Customer 1'), ('Customer 2'); +INSERT INTO orders_test (customer_id, name) VALUES (1, 'Order 1'), (2, 'Order 2'); + +-- Attempt to insert a child with a non-existent customer +INSERT INTO orders_test (customer_id, name) VALUES (3, 'Order 3'); + +-- Attempt to delete a customer that still has orders +DELETE FROM customers_test WHERE customer_id = 2; + +-- Test that invalidation of the foreign key cache works +ALTER TABLE orders_test DROP CONSTRAINT fk_orders_customers; +INSERT INTO orders_test (customer_id, name) VALUES (3, 'Order 3'); diff --git a/src/yb/yql/pgwrapper/pg_catalog_perf-test.cc b/src/yb/yql/pgwrapper/pg_catalog_perf-test.cc index 0534998eee31..9faff75869ee 100644 --- a/src/yb/yql/pgwrapper/pg_catalog_perf-test.cc +++ b/src/yb/yql/pgwrapper/pg_catalog_perf-test.cc @@ -606,4 +606,36 @@ TEST_F_EX(PgCatalogPerfTest, ASSERT_OK(conn.Execute("INSERT INTO t VALUES (my_func(1))")); } +TEST_F_EX(PgCatalogPerfTest, ForeignKeyRelcachePreloadTest, PgPreloadAdditionalCatBothTest) { + auto ddl_conn = ASSERT_RESULT(Connect()); + ASSERT_OK(ddl_conn.Execute("CREATE TABLE primary_table(k INT PRIMARY KEY)")); + ASSERT_OK( + ddl_conn.Execute("CREATE TABLE foreign_table(k INT, region INT) PARTITION BY LIST (region)")); + // Add 10 child tables for the foreign_table + for (int i = 1; i <= 10; ++i) { + ASSERT_OK(ddl_conn.ExecuteFormat( + "CREATE TABLE foreign_table_$0 PARTITION OF foreign_table FOR VALUES IN ($0)", i)); + } + ASSERT_OK( + ddl_conn.Execute("ALTER TABLE foreign_table ADD CONSTRAINT fk_foreign_table_primary_table " + "FOREIGN KEY (k) REFERENCES primary_table(k)")); + // Dummy connection to build the relcache init file + { auto unused_conn = ASSERT_RESULT(Connect()); } + + auto select_conn = ASSERT_RESULT(Connect()); + + const auto startup_rpc_count = ASSERT_RESULT(RPCCountOnStartUp()); + ASSERT_EQ(startup_rpc_count, 3); + + const auto select_rpc_count = + ASSERT_RESULT(RPCCountAfterCacheRefresh([&](PGConn* conn) -> Status { + RETURN_NOT_OK(conn->Fetch( + "SELECT * FROM primary_table JOIN foreign_table ON primary_table.k = foreign_table.k")); + return Status::OK(); + })); + // With yb_enable_fkey_catcache turned off, we would see more than 8 RPCs + // because we have to look up the foreign keys from master. + ASSERT_EQ(select_rpc_count, 8); +} + } // namespace yb::pgwrapper diff --git a/src/yb/yql/pgwrapper/pg_wrapper.cc b/src/yb/yql/pgwrapper/pg_wrapper.cc index 9d7a7b7325ae..a7a0de9b83ba 100644 --- a/src/yb/yql/pgwrapper/pg_wrapper.cc +++ b/src/yb/yql/pgwrapper/pg_wrapper.cc @@ -215,6 +215,9 @@ DEFINE_NON_RUNTIME_bool(enable_ysql_conn_mgr_stats, true, DEFINE_test_flag(bool, ysql_yb_enable_replication_commands, false, "Enable logical replication commands for Publication and Replication Slots"); +DEFINE_RUNTIME_PG_FLAG(bool, yb_enable_fkey_catcache, true, + "Enable preloading of foreign key information into the relation cache."); + static bool ValidateXclusterConsistencyLevel(const char* flagname, const std::string& value) { if (value != "database" && value != "tablet") { fprintf(