Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 26 Jan 2024 11:51:15 +0900
From: Michael Paquier <michael@paquier.xyz>
To: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18310: Some SQL commands fail to process duplicate objects
 with error: tuple already updated by self
Message-ID: <ZbMeI3EyyDlP-Agv@paquier.xyz>
References: <18310-1eb233c5908189c8@postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="tbWwLzi8+J4jw/cG"
Content-Disposition: inline
In-Reply-To: <18310-1eb233c5908189c8@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/ZbMeI3EyyDlP-Agv%40paquier.xyz>
Precedence: bulk


--tbWwLzi8+J4jw/cG
Content-Type: multipart/mixed; boundary="JeLYFFwAVgfeHuZW"
Content-Disposition: inline


--JeLYFFwAVgfeHuZW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 25, 2024 at 04:00:01AM +0000, PG Bug reporting form wrote:
> The following query:
> CREATE ROLE u;
> DROP ROLE u, u;
> fails with
> ERROR:  tuple already updated by self

Pas glop.

> Whilst all the other DROP commands, accepting a list of objects (namely,
> AGGREGATE, DOMAIN, EXTENSION, FOREIGN DATA WRAPPER, FOREIGN TABLE,
> FUNCTION, INDEX, MATERIALIZED VIEW, OPERATOR, PROCEDURE, ROUTINE,
> SEQUENCE, SERVER, STATISTICS, TABLE, TYPE, VIEW), handle such duplicates
> with no error.

DROP ROLE has it own parsing node and its own dropping logic because
of ACL checks and such, while the others used the unified route with
dependency.c & friends.

For this job, I think user.c is just overengineered and could be
simplified to use a list of OIDs.  There is no need for ObjectAddress
or even ObjectAddresses: all of them are created with
AuthIdRelationId as class ID, and the second loop just fetches the
role IDs retrieved from the ObjectAddresses created in the first loop.
We don't use the individual ObjectAddress either.  On top of all that,
it makes the code a bit easier to follow.

> Also, the following ALTER query:
> CREATE TEXT SEARCH CONFIGURATION ispell_tst (COPY=3Denglish);
> CREATE TEXT SEARCH DICTIONARY ispell (Template=3Dispell,
>  DictFile=3Dispell_sample, AffFile=3Dispell_sample);
> ALTER TEXT SEARCH CONFIGURATION ispell_tst ALTER MAPPING FOR word, word
>  WITH ispell;
> fails with the same
> ERROR:  tuple already updated by self
>=20
> Again, I re-checked all the other ALTER commands, that accept a list of
> objects, and could not find other ones producing the same error.

This one comes down to the list of manipulation of the list for token
names, where tsearchcmds.c has a few assumptions about the way to do
its job across multiple catalog scans.  We could adjust the code so as
the list of String nodes is rebuilt with some list_append_unique(),
but I cannot convince myself that this is worth the complication.
--
Michael

--JeLYFFwAVgfeHuZW
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Fix-DROP-ROLE-when-specifying-duplicated-roles.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 8a82a8813b31ead6f37d2b1576239c769bdacbda Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 26 Jan 2024 11:30:56 +0900
Subject: [PATCH] Fix DROP ROLE when specifying duplicated roles

This would cause failures with "tuple already updated by self", blah.

Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/18310-1eb233c5908189c8@postgresql.org
Backpatch-through: 12
---
 src/backend/commands/user.c               | 16 +++++-----------
 src/test/regress/expected/create_role.out |  3 ++-
 src/test/regress/sql/create_role.sql      |  3 ++-
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 7a9c177b21..6839d44a2f 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1093,7 +1093,7 @@ DropRole(DropRoleStmt *stmt)
 	Relation	pg_authid_rel,
 				pg_auth_members_rel;
 	ListCell   *item;
-	List	   *role_addresses =3D NIL;
+	List	   *role_oids =3D NIL;
=20
 	if (!have_createrole_privilege())
 		ereport(ERROR,
@@ -1119,7 +1119,6 @@ DropRole(DropRoleStmt *stmt)
 		ScanKeyData scankey;
 		SysScanDesc sscan;
 		Oid			roleid;
-		ObjectAddress *role_address;
=20
 		if (rolspec->roletype !=3D ROLESPEC_CSTRING)
 			ereport(ERROR,
@@ -1260,21 +1259,16 @@ DropRole(DropRoleStmt *stmt)
 		 */
 		CommandCounterIncrement();
=20
-		/* Looks tentatively OK, add it to the list. */
-		role_address =3D palloc(sizeof(ObjectAddress));
-		role_address->classId =3D AuthIdRelationId;
-		role_address->objectId =3D roleid;
-		role_address->objectSubId =3D 0;
-		role_addresses =3D lappend(role_addresses, role_address);
+		/* Looks tentatively OK, add it to the list if not there yet. */
+		role_oids =3D list_append_unique_oid(role_oids, roleid);
 	}
=20
 	/*
 	 * Second pass over the roles to be removed.
 	 */
-	foreach(item, role_addresses)
+	foreach(item, role_oids)
 	{
-		ObjectAddress *role_address =3D lfirst(item);
-		Oid			roleid =3D role_address->objectId;
+		Oid			roleid =3D lfirst_oid(item);
 		HeapTuple	tuple;
 		Form_pg_authid roleform;
 		char	   *detail;
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/e=
xpected/create_role.out
index 7117e943c2..46d4f9efe9 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -251,7 +251,8 @@ DROP INDEX tenant_idx;
 DROP TABLE tenant_table;
 DROP VIEW tenant_view;
 DROP SCHEMA regress_tenant2_schema;
-DROP ROLE regress_tenant;
+-- check for duplicated drop
+DROP ROLE regress_tenant, regress_tenant;
 DROP ROLE regress_tenant2;
 DROP ROLE regress_rolecreator;
 DROP ROLE regress_role_admin;
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/cr=
eate_role.sql
index 12582a3cc2..4491a28a8a 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -206,7 +206,8 @@ DROP INDEX tenant_idx;
 DROP TABLE tenant_table;
 DROP VIEW tenant_view;
 DROP SCHEMA regress_tenant2_schema;
-DROP ROLE regress_tenant;
+-- check for duplicated drop
+DROP ROLE regress_tenant, regress_tenant;
 DROP ROLE regress_tenant2;
 DROP ROLE regress_rolecreator;
 DROP ROLE regress_role_admin;
--=20
2.43.0


--JeLYFFwAVgfeHuZW--

--tbWwLzi8+J4jw/cG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWzHiMACgkQnvQgOdby
QH1olA/+PFYXGc3nIxGh69hWGIH7wk/PXwTDmLgZdOB34E4stCmuO4lerlVVOQOO
lkeZIHDhbtPX7MW2Do9q+Uc3lgNqIvzdgDBJ+0mK4Jz/lt801I8YkVlEWJM98wQC
b1K8v0NLrhVuebg30BMAygbUR4WL9T6sZXyLTn7zdsLt8IQ4JSFCdZwEQ3NjacBr
/7pOAL33JSSeoKCFe8TerUlz9MqpkRWKCJEZTHCuE7UbVy7Byl7dG7MMxnwQ1Db0
6EgzpuFTRC74ZrRXJaeyAbORc+H2kNSFnBY1TxNLBadDRZEaDfTUDB5gPQ8tH5tE
/b1hqBinMFWHHVCKuhtD2hdfMRlKId21yXLWIb5nfbFGpuPn7hFHBlY3aF71hh4h
g+YOsgV9h49ZJVYNdlMAWf7QGqHn5f33Ooxka47bfMAtu9CU95QvmDQb7xpPJCsy
pBbAMikxbtGlCjxIaGZxkME9sEP/dMGKDEmN7CPicS+YSCYI7qnKgL7uz7rv8A+U
9YhN8F8Ij4axpjAkMNkgSXYhMkxjAXi+26WI+kjR/pDrWjd6kSRAr8jrpUZA2iWn
ixPT4ut4uW83xMvaj6TdE5ftXaE8j6yC3U0bjYvx/rBYoWFvFwzOaVIEK/867n2S
D4Hvct0MYL1DkMvrUh/7+xyu45h2IStdOigo2AHva05V0/m46yA=
=Ldkx
-----END PGP SIGNATURE-----

--tbWwLzi8+J4jw/cG--