Date: Wed, 20 Nov 2024 17:11:00 -0800
From: Noah Misch <noah@leadboat.com>
To: "Aya Iwata (Fujitsu)" <iwata.aya@fujitsu.com>
Cc: "pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: 'WARNING:  you don't own a lock of type ExclusiveLock' is
 printed during the GRANT command on PostgreSQL16.5
Message-ID: <20241121011100.c3.nmisch@google.com>
References: 
 <OS7PR01MB11964507B5548245A7EE54E70EA212@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <20241120062742.b0.nmisch@google.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="BDIroXaFzUAhfEr3"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20241120062742.b0.nmisch@google.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241121011100.c3.nmisch%40google.com>
Precedence: bulk


--BDIroXaFzUAhfEr3
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Tue, Nov 19, 2024 at 10:27:42PM -0800, Noah Misch wrote:
> On Wed, Nov 20, 2024 at 05:11:07AM +0000, Aya Iwata (Fujitsu) wrote:
> > During our tests on November release, 
> > we discovered a "WARNING:  you don't own a lock of type ExclusiveLock" that occurred at a strange time in GRANT command.

> > This is only possible when psql executes each command individually.
> > $ psql -d postgres -c "CREATE USER nmluser1;"
> > CREATE ROLE
> > $ psql -d postgres -c "CREATE TABLESPACE test_tblspace LOCATION '/xxx /xxx/pgsql16.5/tbs';"
> > CREATE TABLESPACE
> > $ psql -d postgres -c "CREATE DATABASE enc_testdb;"
> > CREATE DATABASE
> > $ psql -d postgres -c "GRANT ALL PRIVILEGES ON DATABASE postgres TO nmluser1;"
> > GRANT
> > $ psql -d postgres -c "GRANT ALL PRIVILEGES ON DATABASE enc_testdb TO nmluser1;"
> > GRANT
> > $ psql -d postgres -c "GRANT CREATE ON TABLESPACE test_tblspace TO nmluser1;"
> > WARNING:  you don't own a lock of type ExclusiveLock
> > GRANT

Did you ever see the WARNING with a DATABASE command, or just with a
TABLESPACE command?

> Thanks for the report.  This seems reasonably likely to come from a defect in
> commit 51ff46d.  I will look into it.

Yes, that commit had us reading cc_relisshared before that field got
initialized.  I can reproduce the WARNING in v16+, with GRANT TABLESPACE.
Before a cache's initialization cc_relisshared==false, so
SearchSysCacheLocked1() used the wrong locktag.  The attached patch is about
right.  At a minimum, I still need to work on the details in its log message.

--BDIroXaFzUAhfEr3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="inplace250-catcache-init-SearchSysCacheLocked1-v1.patch"

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Avoid "you don't own a lock of type ExclusiveLock" in GRANT TABLESPACE.
    
    FIXME look for other user-visible implications:
    - other commands that can reach this warning
    - commands that actually need the protection of the LOCKTAG_TUPLE and
      don't get it as a result of setting the wrong dboid in the locktag.
      These would be commands involving pg_database.
    
    This happened because SearchSysCacheLocked1() read cc_relisshared before
    cache initialization, so the locktag for a shared catalog incorrectly
    contained dboid==MyDatabaseId.  Only shared catalogs were affected.
    
    Back-patch to v13 (all supported versions).  This has no known impact
    before v16, where ExecGrant_common() first appeared.  Earlier branches
    have a separate ExecGrant_Tablespace() that doesn't use LOCKTAG_TUPLE,
    and ExecGrant_Database() seems to get a correctly-initialized cache
    FIXMEconfirm.  Even so it's not very future-proof to assume v15 won't
    get impact in the future.
    
    Reported by Aya Iwata.  Reviewed by FIXME.
    
    Discussion: https://postgr.es/m/OS7PR01MB11964507B5548245A7EE54E70EA212@OS7PR01MB11964.jpnprd01.prod.outlook.com

diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index f41b1c2..f7f4f56 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -287,11 +287,9 @@ HeapTuple
 SearchSysCacheLocked1(int cacheId,
 					  Datum key1)
 {
+	CatCache   *cache = SysCache[cacheId];
 	ItemPointerData tid;
 	LOCKTAG		tag;
-	Oid			dboid =
-		SysCache[cacheId]->cc_relisshared ? InvalidOid : MyDatabaseId;
-	Oid			reloid = cacheinfo[cacheId].reloid;
 
 	/*----------
 	 * Since inplace updates may happen just before our LockTuple(), we must
@@ -343,8 +341,15 @@ SearchSysCacheLocked1(int cacheId,
 
 		tid = tuple->t_self;
 		ReleaseSysCache(tuple);
-		/* like: LockTuple(rel, &tid, lockmode) */
-		SET_LOCKTAG_TUPLE(tag, dboid, reloid,
+
+		/*
+		 * Do like LockTuple(rel, &tid, lockmode).  While cc_relisshared won't
+		 * change from one iteration to another, it may have been a temporary
+		 * "false" until our first SearchSysCache1().
+		 */
+		SET_LOCKTAG_TUPLE(tag,
+						  cache->cc_relisshared ? InvalidOid : MyDatabaseId,
+						  cache->cc_reloid,
 						  ItemPointerGetBlockNumber(&tid),
 						  ItemPointerGetOffsetNumber(&tid));
 		(void) LockAcquire(&tag, lockmode, false, false);
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out
index dd535d4..a90e39e 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -927,6 +927,11 @@ ALTER INDEX testschema.part_a_idx SET TABLESPACE pg_default;
 -- Fail, not empty
 DROP TABLESPACE regress_tblspace;
 ERROR:  tablespace "regress_tblspace" is not empty
+-- Adequate cache initialization before GRANT
+\c -
+BEGIN;
+GRANT ALL ON TABLESPACE regress_tblspace TO PUBLIC;
+ROLLBACK;
 CREATE ROLE regress_tablespace_user1 login;
 CREATE ROLE regress_tablespace_user2 login;
 GRANT USAGE ON SCHEMA testschema TO regress_tablespace_user2;
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql
index c8b8378..dfe3db0 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -396,6 +396,12 @@ ALTER INDEX testschema.part_a_idx SET TABLESPACE pg_default;
 -- Fail, not empty
 DROP TABLESPACE regress_tblspace;
 
+-- Adequate cache initialization before GRANT
+\c -
+BEGIN;
+GRANT ALL ON TABLESPACE regress_tblspace TO PUBLIC;
+ROLLBACK;
+
 CREATE ROLE regress_tablespace_user1 login;
 CREATE ROLE regress_tablespace_user2 login;
 GRANT USAGE ON SCHEMA testschema TO regress_tablespace_user2;

--BDIroXaFzUAhfEr3--