Date: Thu, 11 Sep 2025 11:57:29 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Smolkin Grigory <smallkeen@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: race condition in pg_class
Message-ID: <20250911185729.ad.nmisch@google.com>
References: 
 <CAMp+ueZQz3yDk7qg42hk6-9gxniYbp-=bG2mgqecErqR5gGGOA@mail.gmail.com>
 <20231027044404.7e.nmisch@google.com>
 <20231027184832.9b.nmisch@google.com>
 <1596629.1698435146@sss.pgh.pa.us>
 <20231027214946.79.nmisch@google.com>
 <1617596.1698446455@sss.pgh.pa.us>
 <20231027232612.99.nmisch@google.com>
 <20231102030915.d3.nmisch@google.com>
 <20240512232923.aa.nmisch@google.com>
 <1802710.1757608564@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="xZJ/d+OgWc8sEYA8"
Content-Disposition: inline
In-Reply-To: <1802710.1757608564@sss.pgh.pa.us>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250911185729.ad.nmisch%40google.com>
Precedence: bulk


--xZJ/d+OgWc8sEYA8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Sep 11, 2025 at 12:36:04PM -0400, Tom Lane wrote:
> inplace010-tests-v1.patch from this message, committed as
> 0844b3968, contains this bit:
> 
> new file mode 100644
> index 00000000000..0367c0e37ab
> --- /dev/null
> +++ b/src/test/regress/sql/database.sql
> @@ -0,0 +1,17 @@
> +CREATE DATABASE regression_tbd
> +   ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0;
> +ALTER DATABASE regression_tbd RENAME TO regression_utf8;
> +ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace;
> +ALTER DATABASE regression_utf8 RESET TABLESPACE;
> +ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123;
> +
> 
> It emerges that the "ALTER DATABASE regression_utf8 RESET TABLESPACE"
> command is a complete no-op [1].  I am guessing that that was not the
> behavior you had in mind, and am wondering if we are losing any test
> coverage thereby.  Did you have a specific reason for manipulating the
> TABLESPACE property and not some random GUC setting?

I have no specific notes or memories about that RESET TABLESPACE, but I likely
wanted "SET TABLESPACE pg_default".  RESET TABLESPACE may still have the
effect of loading a catcache entry, as a later comment says:

-- Test PgDatabaseToastTable.  Doing this with GRANT would be slow.
BEGIN;
UPDATE pg_database
SET datacl = array_fill(makeaclitem(10, 10, 'USAGE', false), ARRAY[5e5::int])
WHERE datname = 'regression_utf8';
-- load catcache entry, if nothing else does
ALTER DATABASE regression_utf8 RESET TABLESPACE;
ROLLBACK;


That originated to exercise catcache.c code specific to toast_flatten_tuple()
for an inplace-updated catalog.  (Commit af8cd16 later removed the
inplace-specific code.)  SET TABLESPACE rejects transaction blocks, so that
would need another way to load a catcache entry.  I'm inclined to change it as
attached.  This doesn't reduce database.sql test coverage of dbcommands.c or
catcache.c, so I'll bet it doesn't lose anything vs. the original database.sql
commit.  Some check-tests runs showed it adding database.sql coverage of
catcache.c:1908-1911 and catcache.c:1983-1984, so that's a bonus.

> [1] https://www.postgresql.org/message-id/flat/30783e-68c28a00-9-41004480%40130449754

Do you plan to back-patch that?  That should dictate whether I back-patch the
test change.

--xZJ/d+OgWc8sEYA8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="no-reset-tablespace-v1.patch"

diff --git a/src/test/regress/expected/database.out b/src/test/regress/expected/database.out
index 4cbdbdf..6b879b0 100644
--- a/src/test/regress/expected/database.out
+++ b/src/test/regress/expected/database.out
@@ -2,7 +2,7 @@ CREATE DATABASE regression_tbd
 	ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0;
 ALTER DATABASE regression_tbd RENAME TO regression_utf8;
 ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace;
-ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ALTER DATABASE regression_utf8 SET TABLESPACE pg_default;
 ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123;
 -- Test PgDatabaseToastTable.  Doing this with GRANT would be slow.
 BEGIN;
@@ -10,7 +10,7 @@ UPDATE pg_database
 SET datacl = array_fill(makeaclitem(10, 10, 'USAGE', false), ARRAY[5e5::int])
 WHERE datname = 'regression_utf8';
 -- load catcache entry, if nothing else does
-ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ALTER DATABASE regression_utf8 RENAME TO regression_rename_rolled_back;
 ROLLBACK;
 CREATE ROLE regress_datdba_before;
 CREATE ROLE regress_datdba_after;
diff --git a/src/test/regress/sql/database.sql b/src/test/regress/sql/database.sql
index 46ad263..4ef3612 100644
--- a/src/test/regress/sql/database.sql
+++ b/src/test/regress/sql/database.sql
@@ -2,7 +2,7 @@ CREATE DATABASE regression_tbd
 	ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0;
 ALTER DATABASE regression_tbd RENAME TO regression_utf8;
 ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace;
-ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ALTER DATABASE regression_utf8 SET TABLESPACE pg_default;
 ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123;
 
 -- Test PgDatabaseToastTable.  Doing this with GRANT would be slow.
@@ -11,7 +11,7 @@ UPDATE pg_database
 SET datacl = array_fill(makeaclitem(10, 10, 'USAGE', false), ARRAY[5e5::int])
 WHERE datname = 'regression_utf8';
 -- load catcache entry, if nothing else does
-ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ALTER DATABASE regression_utf8 RENAME TO regression_rename_rolled_back;
 ROLLBACK;
 
 CREATE ROLE regress_datdba_before;

--xZJ/d+OgWc8sEYA8--