From e809d4eb6ffcadeb805f382ad8f7b040d708f4ba Mon Sep 17 00:00:00 2001 From: Minghui Yang Date: Tue, 1 Oct 2024 03:47:31 +0000 Subject: [PATCH] [BACKPORT 2.20][#24217] YSQL: fill definition of a shell type requires catalog version increment Summary: Steps to repro: 1. In one ysqlsh session: ``` yugabyte=# CREATE TYPE base_type; -- create shell type CREATE TYPE yugabyte=# CREATE FUNCTION base_type_in(cstring) RETURNS base_type yugabyte-# LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2in'; NOTICE: return type base_type is only a shell CREATE FUNCTION yugabyte=# CREATE FUNCTION base_type_out(base_type) RETURNS cstring yugabyte-# LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2out'; NOTICE: argument type base_type is only a shell CREATE FUNCTION yugabyte=# CREATE FUNCTION base_type_recv(internal) RETURNS base_type yugabyte-# LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2recv'; NOTICE: return type base_type is only a shell CREATE FUNCTION yugabyte=# CREATE FUNCTION base_type_send(base_type) RETURNS bytea yugabyte-# LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2send'; NOTICE: argument type base_type is only a shell CREATE FUNCTION ``` 2. In a second session: ``` yugabyte=# CREATE TABLE default_test (f1 base_type, f2 int); ERROR: type "base_type" is only a shell LINE 1: CREATE TABLE default_test (f1 base_type, f2 int); ``` This ERROR is expected. 3. Back to the first session to fill the definition of the shell type ``` yugabyte=# CREATE TYPE base_type ( yugabyte(# INPUT = base_type_in, yugabyte(# OUTPUT = base_type_out, yugabyte(# RECEIVE = base_type_recv, yugabyte(# SEND = base_type_send, yugabyte(# LIKE = smallint, yugabyte(# CATEGORY = 'N', yugabyte(# PREFERRED = FALSE, yugabyte(# DELIMITER = ',', yugabyte(# COLLATABLE = FALSE yugabyte(# ); -- fill definition CREATE TYPE ``` 4. Back to the second session ``` yugabyte=# CREATE TABLE default_test (f1 base_type, f2 int); ERROR: type "base_type" is only a shell LINE 1: CREATE TABLE default_test (f1 base_type, f2 int); ``` This ERROR is not expected because the first session has already filled the definition of the shell type. To fix this bug I added code to detect the update of an existing shell type and make this DDL to increment the catalog version. A new unit test is added. Jira: DB-13103 Original commit: 17c45ffd231473228c89e7bd25d2b64dfb2a2571 / D38575 Test Plan: ./yb_build.sh debug --cxx-test pgwrapper_pg_libpq-test --gtest_filter=PgLibPqTest.FillShellTypeDefinition Reviewers: kfranz, mihnea Reviewed By: kfranz Subscribers: yql Differential Revision: https://phorge.dev.yugabyte.com/D38697 --- src/postgres/src/backend/catalog/pg_type.c | 8 ++++ .../src/backend/utils/misc/pg_yb_utils.c | 11 ++++- src/postgres/src/include/pg_yb_utils.h | 2 + src/yb/yql/pgwrapper/pg_libpq-test.cc | 44 +++++++++++++++++++ 4 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/postgres/src/backend/catalog/pg_type.c b/src/postgres/src/backend/catalog/pg_type.c index 779061723861..1db38a2a45b3 100644 --- a/src/postgres/src/backend/catalog/pg_type.c +++ b/src/postgres/src/backend/catalog/pg_type.c @@ -451,6 +451,14 @@ TypeCreate(Oid newTypeOid, replaces); CatalogTupleUpdate(pg_type_desc, &tup->t_self, tup); + if (IsYugaByteEnabled()) + /* + * Update existing shell type requires catalog version increment + * so that if a session has cached the shell type it can get + * refreshed to get the newly defined type. + */ + YBAddModificationAspects(true /* is_catalog_version_increment */, + false /* is_breaking_catalog_change */); typeObjectId = HeapTupleGetOid(tup); 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 cde1850116e6..3475408231e4 100644 --- a/src/postgres/src/backend/utils/misc/pg_yb_utils.c +++ b/src/postgres/src/backend/utils/misc/pg_yb_utils.c @@ -1515,6 +1515,13 @@ void YbSetIsGlobalDDL() { ddl_transaction_state.is_global_ddl = true; } +void +YBAddModificationAspects(bool is_catalog_version_increment, + bool is_breaking_catalog_change) { + ddl_transaction_state.is_catalog_version_increment |= is_catalog_version_increment; + ddl_transaction_state.is_breaking_catalog_change |= is_breaking_catalog_change; +} + void YBIncrementDdlNestingLevel(bool is_catalog_version_increment, bool is_breaking_catalog_change) @@ -1533,8 +1540,8 @@ YBIncrementDdlNestingLevel(bool is_catalog_version_increment, * The is_catalog_version_increment and is_breaking_catalog_change flags * should only be set if it is not already true. */ - ddl_transaction_state.is_catalog_version_increment |= is_catalog_version_increment; - ddl_transaction_state.is_breaking_catalog_change |= is_breaking_catalog_change; + YBAddModificationAspects(is_catalog_version_increment, + is_breaking_catalog_change); } void diff --git a/src/postgres/src/include/pg_yb_utils.h b/src/postgres/src/include/pg_yb_utils.h index 19910856225f..f64fe50c5a0a 100644 --- a/src/postgres/src/include/pg_yb_utils.h +++ b/src/postgres/src/include/pg_yb_utils.h @@ -627,6 +627,8 @@ bool YBIsInitDbAlreadyDone(); int YBGetDdlNestingLevel(); void YbSetIsGlobalDDL(); +void YBAddModificationAspects(bool is_catalog_version_increment, + bool is_breaking_catalog_change); void YBIncrementDdlNestingLevel(bool is_catalog_version_increment, bool is_breaking_catalog_change); void YBDecrementDdlNestingLevel(); diff --git a/src/yb/yql/pgwrapper/pg_libpq-test.cc b/src/yb/yql/pgwrapper/pg_libpq-test.cc index 4702a26e4851..7eb737b1a33c 100644 --- a/src/yb/yql/pgwrapper/pg_libpq-test.cc +++ b/src/yb/yql/pgwrapper/pg_libpq-test.cc @@ -4240,5 +4240,49 @@ TEST_F(PgPostmasterExitTest, SignalIdleBackendOnPostmasterDeath) { ASSERT_OK(TestPostmasterExit()); } +TEST_F(PgLibPqTest, FillShellTypeDefinition) { + PGConn conn1 = ASSERT_RESULT(Connect()); + PGConn conn2 = ASSERT_RESULT(Connect()); + // Create a shell type. + ASSERT_OK(conn1.Execute( + R"#( +CREATE TYPE base_type; -- create shell type +CREATE FUNCTION base_type_in(cstring) RETURNS base_type + LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2in'; +CREATE FUNCTION base_type_out(base_type) RETURNS cstring + LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2out'; +CREATE FUNCTION base_type_recv(internal) RETURNS base_type + LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2recv'; +CREATE FUNCTION base_type_send(base_type) RETURNS bytea + LANGUAGE internal IMMUTABLE STRICT PARALLEL SAFE AS 'int2send'; + )#")); + + // Using a shell type as column type is an error. Note that shell type is now + // cached in conn2. + ASSERT_NOK(conn2.Execute("CREATE TABLE default_test (f1 base_type, f2 int)")); + + // Fill the definition of the shell type. + ASSERT_OK(conn1.Execute( + R"#( +CREATE TYPE base_type ( + INPUT = base_type_in, + OUTPUT = base_type_out, + RECEIVE = base_type_recv, + SEND = base_type_send, + LIKE = smallint, + CATEGORY = 'N', + PREFERRED = FALSE, + DELIMITER = ',', + COLLATABLE = FALSE +); -- fill definition + )#")); + // Wait for heartbeat to propagate the new catalog version to trigger + // catalog cache refresh on conn2. + SleepFor(2s); + + // Now that the shell type definition is filled, we should not get an error any more. + ASSERT_OK(conn2.Execute("CREATE TABLE default_test (f1 base_type, f2 int)")); +} + } // namespace pgwrapper } // namespace yb