From: Michael Paquier <michael@paquier.xyz>
To: exclusion@gmail.com
To: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
Date: Fri, 26 Jan 2024 11:51:15 +0900
Message-ID: <ZbMeI3EyyDlP-Agv@paquier.xyz> (raw)
In-Reply-To: <18310-1eb233c5908189c8@postgresql.org>
References: <18310-1eb233c5908189c8@postgresql.org>
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=english);
> CREATE TEXT SEARCH DICTIONARY ispell (Template=ispell,
> DictFile=ispell_sample, AffFile=ispell_sample);
> ALTER TEXT SEARCH CONFIGURATION ispell_tst ALTER MAPPING FOR word, word
> WITH ispell;
> fails with the same
> ERROR: tuple already updated by self
>
> 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
Attachments:
[text/x-diff] 0001-Fix-DROP-ROLE-when-specifying-duplicated-roles.patch (3.1K, ../ZbMeI3EyyDlP-Agv@paquier.xyz/2-0001-Fix-DROP-ROLE-when-specifying-duplicated-roles.patch)
download | inline diff:
From 8a82a8813b31ead6f37d2b1576239c769bdacbda Mon Sep 17 00:00:00 2001
From: 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.cindex 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 = NIL;+ List *role_oids = NIL;
if (!have_createrole_privilege())
ereport(ERROR,
@@ -1119,7 +1119,6 @@ DropRole(DropRoleStmt *stmt)
ScanKeyData scankey;
SysScanDesc sscan;
Oid roleid;
- ObjectAddress *role_address;
if (rolspec->roletype != ROLESPEC_CSTRING)
ereport(ERROR,
@@ -1260,21 +1259,16 @@ DropRole(DropRoleStmt *stmt)
*/
CommandCounterIncrement();
- /* Looks tentatively OK, add it to the list. */- role_address = palloc(sizeof(ObjectAddress));- role_address->classId = AuthIdRelationId;- role_address->objectId = roleid;- role_address->objectSubId = 0;- role_addresses = lappend(role_addresses, role_address);+ /* Looks tentatively OK, add it to the list if not there yet. */+ role_oids = list_append_unique_oid(role_oids, roleid);
}
/*
* Second pass over the roles to be removed.
*/
- foreach(item, role_addresses)+ foreach(item, role_oids)
{
- ObjectAddress *role_address = lfirst(item);- Oid roleid = role_address->objectId;+ Oid roleid = 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/expected/create_role.outindex 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/create_role.sqlindex 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;
--
2.43.0
[application/pgp-signature] signature.asc (832B, ../ZbMeI3EyyDlP-Agv@paquier.xyz/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, 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
In-Reply-To: <ZbMeI3EyyDlP-Agv@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox