pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
20+ messages / 4 participants
[nested] [flat]
* BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-25 04:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 20+ messages in thread
From: PG Bug reporting form @ 2024-01-25 04:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18310
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 16.1
Operating system: Ubuntu 22.04
Description:
[ this bug reported to fix defects discovered while working on bug #18297
]
The following query:
CREATE ROLE u;
DROP ROLE u, u;
fails with
ERROR: tuple already updated by self
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.
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.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-26 02:51 Michael Paquier <michael@paquier.xyz>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 20+ messages in thread
From: Michael Paquier @ 2024-01-26 02:51 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.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.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 = 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.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/create_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;
--
2.43.0
[application/pgp-signature] signature.asc (832B, ../../ZbMeI3EyyDlP-Agv@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-26 03:23 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-26 03:23 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> 于2024年1月26日周五 10:51写道：
> 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.
>
Agreed. +1
>
> > 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
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-26 03:33 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-26 03:33 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> 于2024年1月26日周五 10:51写道：
> 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.
>
I have an idea.
How about using list *res replace int *res in getTokenTypes(), so we can
use list_append_unique().
In outer function, use list *tokens to replace int *tokens, and ntokens =
list_length(tokens) not list_length(stmt->tokentype).
--
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-26 03:41 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2024-01-26 03:41 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Fri, Jan 26, 2024 at 11:33:24AM +0800, Tender Wang wrote:
> How about using list *res replace int *res in getTokenTypes(), so we can
> use list_append_unique().
> In outer function, use list *tokens to replace int *tokens, and ntokens =
> list_length(tokens) not list_length(stmt->tokentype).
Yeah, I was wondering about this code path. If you feel strongly
about that, would you like to write a patch?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbMp3H9oVvubWkR5@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-26 04:54 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-26 04:54 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> 于2024年1月26日周五 11:41写道：
> On Fri, Jan 26, 2024 at 11:33:24AM +0800, Tender Wang wrote:
> > How about using list *res replace int *res in getTokenTypes(), so we can
> > use list_append_unique().
> > In outer function, use list *tokens to replace int *tokens, and ntokens =
> > list_length(tokens) not list_length(stmt->tokentype).
>
> Yeah, I was wondering about this code path. If you feel strongly
> about that, would you like to write a patch?
>
Ok, I try to fix it.
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-26 06:21 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 2 replies; 20+ messages in thread
From: Tender Wang @ 2024-01-26 06:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> 于2024年1月26日周五 12:54写道：
>
>
> Michael Paquier <michael@paquier.xyz> 于2024年1月26日周五 11:41写道：
>
>> On Fri, Jan 26, 2024 at 11:33:24AM +0800, Tender Wang wrote:
>> > How about using list *res replace int *res in getTokenTypes(), so we can
>> > use list_append_unique().
>> > In outer function, use list *tokens to replace int *tokens, and ntokens
>> =
>> > list_length(tokens) not list_length(stmt->tokentype).
>>
>> Yeah, I was wondering about this code path. If you feel strongly
>> about that, would you like to write a patch?
>>
>
> Ok, I try to fix it.
>
As I said before, return List looks like not complicated to solve this
issue.
I found another problem, it didn't report NOTICE if SQL has IF EXISTS, for
example:
postgres=# alter text search configuration ispell_tst drop mapping if
exists for test;
ERROR: token type "test" does not exist
So I change the func getTokenTypes() interface and remove
DropConfigurationMapping() error report into getTokenTypes().
--
>> Michael
>>
>
>
> --
> Tender Wang
> OpenPie: https://en.openpie.com/
>
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
[application/octet-stream] 0001-Fix-reporting-error-when-process-duplicate-token-typ.patch (7.9K, ../../CAHewXNk5yeUo2XPaqTYSfbkKhEBfKd4VYa7NHv2MzV-mJ9ULpA@mail.gmail.com/3-0001-Fix-reporting-error-when-process-duplicate-token-typ.patch)
download | inline diff:
From a826ab300507721861fced5642dc509872219d55 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Fri, 26 Jan 2024 14:01:52 +0800
Subject: [PATCH] Fix reporting error when process duplicate token type.
Now we don't Remove Duplicates in tokenname list, so we may
see this error "tuple already updated by self".
By the way, it should report NOTICE not ERROR if IF EXISTS specified
when we DROP MAPPING. So change the getTokenTypes() interface.
---
src/backend/commands/tsearchcmds.c | 87 +++++++++++----------------
src/test/regress/expected/tsdicts.out | 5 ++
src/test/regress/sql/tsdicts.sql | 7 +++
3 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 07671720ea..dc76f7a465 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -1194,20 +1194,19 @@ AlterTSConfiguration(AlterTSConfigurationStmt *stmt)
/*
* Translate a list of token type names to an array of token type numbers
*/
-static int *
-getTokenTypes(Oid prsId, List *tokennames)
+static List *
+getTokenTypes(Oid prsId, AlterTSConfigurationStmt *stmt)
{
TSParserCacheEntry *prs = lookup_ts_parser_cache(prsId);
LexDescr *list;
- int *res,
- i,
- ntoken;
+ List *res = NIL;
+ int ntoken;
ListCell *tn;
+ List *tokennames = stmt->tokentype;
ntoken = list_length(tokennames);
if (ntoken == 0)
- return NULL;
- res = (int *) palloc(sizeof(int) * ntoken);
+ return NIL;
if (!OidIsValid(prs->lextypeOid))
elog(ERROR, "method lextype isn't defined for text search parser %u",
@@ -1217,7 +1216,6 @@ getTokenTypes(Oid prsId, List *tokennames)
list = (LexDescr *) DatumGetPointer(OidFunctionCall1(prs->lextypeOid,
(Datum) 0));
- i = 0;
foreach(tn, tokennames)
{
String *val = lfirst_node(String, tn);
@@ -1229,18 +1227,26 @@ getTokenTypes(Oid prsId, List *tokennames)
{
if (strcmp(strVal(val), list[j].alias) == 0)
{
- res[i] = list[j].lexid;
+ res = list_append_unique_int(res, list[j].lexid);
found = true;
break;
}
j++;
}
if (!found)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("token type \"%s\" does not exist",
- strVal(val))));
- i++;
+ {
+ if (!stmt->missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("token type \"%s\" does not exist",
+ strVal(val))));
+ else
+ {
+ ereport(NOTICE,
+ (errmsg("token type \"%s\" does not exist, skipping",
+ strVal(val))));
+ }
+ }
}
return res;
@@ -1261,8 +1267,8 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
int i;
int j;
Oid prsId;
- int *tokens,
- ntoken;
+ List *tokens = NIL;
+ int ntoken;
Oid *dictIds;
int ndict;
ListCell *c;
@@ -1272,15 +1278,15 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
cfgId = tsform->oid;
prsId = tsform->cfgparser;
- tokens = getTokenTypes(prsId, stmt->tokentype);
- ntoken = list_length(stmt->tokentype);
+ tokens = getTokenTypes(prsId, stmt);
+ ntoken = list_length(tokens);
if (stmt->override)
{
/*
* delete maps for tokens if they exist and command was ALTER
*/
- for (i = 0; i < ntoken; i++)
+ foreach(c, tokens)
{
ScanKeyInit(&skey[0],
Anum_pg_ts_config_map_mapcfg,
@@ -1289,7 +1295,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyInit(&skey[1],
Anum_pg_ts_config_map_maptokentype,
BTEqualStrategyNumber, F_INT4EQ,
- Int32GetDatum(tokens[i]));
+ Int32GetDatum(lfirst_int(c)));
scan = systable_beginscan(relMap, TSConfigMapIndexId, true,
NULL, 2, skey);
@@ -1346,9 +1352,9 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
{
bool tokmatch = false;
- for (j = 0; j < ntoken; j++)
+ foreach(c, tokens)
{
- if (cfgmap->maptokentype == tokens[j])
+ if (cfgmap->maptokentype == lfirst_int(c))
{
tokmatch = true;
break;
@@ -1401,7 +1407,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
/*
* Insertion of new entries
*/
- for (i = 0; i < ntoken; i++)
+ foreach(c, tokens)
{
for (j = 0; j < ndict; j++)
{
@@ -1411,7 +1417,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
slot[slotCount]->tts_tupleDescriptor->natts * sizeof(bool));
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapcfg - 1] = ObjectIdGetDatum(cfgId);
- slot[slotCount]->tts_values[Anum_pg_ts_config_map_maptokentype - 1] = Int32GetDatum(tokens[i]);
+ slot[slotCount]->tts_values[Anum_pg_ts_config_map_maptokentype - 1] = Int32GetDatum(lfirst_int(c));
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapseqno - 1] = Int32GetDatum(j + 1);
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapdict - 1] = ObjectIdGetDatum(dictIds[j]);
@@ -1455,23 +1461,18 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyData skey[2];
SysScanDesc scan;
HeapTuple maptup;
- int i;
Oid prsId;
- int *tokens;
+ List *tokens = NIL;
ListCell *c;
tsform = (Form_pg_ts_config) GETSTRUCT(tup);
cfgId = tsform->oid;
prsId = tsform->cfgparser;
- tokens = getTokenTypes(prsId, stmt->tokentype);
+ tokens = getTokenTypes(prsId, stmt);
- i = 0;
- foreach(c, stmt->tokentype)
+ foreach(c, tokens)
{
- String *val = lfirst_node(String, c);
- bool found = false;
-
ScanKeyInit(&skey[0],
Anum_pg_ts_config_map_mapcfg,
BTEqualStrategyNumber, F_OIDEQ,
@@ -1479,7 +1480,7 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyInit(&skey[1],
Anum_pg_ts_config_map_maptokentype,
BTEqualStrategyNumber, F_INT4EQ,
- Int32GetDatum(tokens[i]));
+ Int32GetDatum(lfirst_int(c)));
scan = systable_beginscan(relMap, TSConfigMapIndexId, true,
NULL, 2, skey);
@@ -1487,29 +1488,9 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
while (HeapTupleIsValid((maptup = systable_getnext(scan))))
{
CatalogTupleDelete(relMap, &maptup->t_self);
- found = true;
}
systable_endscan(scan);
-
- if (!found)
- {
- if (!stmt->missing_ok)
- {
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("mapping for token type \"%s\" does not exist",
- strVal(val))));
- }
- else
- {
- ereport(NOTICE,
- (errmsg("mapping for token type \"%s\" does not exist, skipping",
- strVal(val))));
- }
- }
-
- i++;
}
EventTriggerCollectAlterTSConfig(stmt, cfgId, NULL, 0);
diff --git a/src/test/regress/expected/tsdicts.out b/src/test/regress/expected/tsdicts.out
index c804293142..af69197306 100644
--- a/src/test/regress/expected/tsdicts.out
+++ b/src/test/regress/expected/tsdicts.out
@@ -687,3 +687,8 @@ CREATE TEXT SEARCH DICTIONARY tsdict_case
"AffFile" = ispell_sample
);
ERROR: unrecognized Ispell parameter: "DictFile"
+-- test duplicate token type
+CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst ALTER MAPPING FOR word, word WITH ispell;
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst DROP MAPPING IF EXISTS FOR hello;
+NOTICE: token type "hello" does not exist, skipping
diff --git a/src/test/regress/sql/tsdicts.sql b/src/test/regress/sql/tsdicts.sql
index ddc6c7f445..d4302abebd 100644
--- a/src/test/regress/sql/tsdicts.sql
+++ b/src/test/regress/sql/tsdicts.sql
@@ -251,3 +251,10 @@ CREATE TEXT SEARCH DICTIONARY tsdict_case
"DictFile" = ispell_sample,
"AffFile" = ispell_sample
);
+
+-- test duplicate token type
+CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
+
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst ALTER MAPPING FOR word, word WITH ispell;
+
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst DROP MAPPING IF EXISTS FOR hello;
--
2.25.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-27 17:32 Yongtao Huang <yongtaoh2022@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Yongtao Huang @ 2024-01-27 17:32 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi Tender Wang,
I think your code changes *LGTM*. Thanks for your contribution.
But I meet these errors when I git apply your patch, so I do some tiny
formate modification.
Please refer to the patch which is still authored by you.
``` bash
$ git apply 0001-Fix-reporting-error-when-process-duplicate-token-typ.patch
0001-Fix-reporting-error-when-process-duplicate-token-typ.patch:38: indent
with spaces.
List *tokennames = stmt->tokentype;
0001-Fix-reporting-error-when-process-duplicate-token-typ.patch:73: indent
with spaces.
{
0001-Fix-reporting-error-when-process-duplicate-token-typ.patch:74: indent
with spaces.
if (!stmt->missing_ok)
0001-Fix-reporting-error-when-process-duplicate-token-typ.patch:75: indent
with spaces.
ereport(ERROR,
0001-Fix-reporting-error-when-process-duplicate-token-typ.patch:76: indent
with spaces.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
warning: squelched 9 whitespace errors
warning: 14 lines add whitespace errors.
```
Regrads
Yongtao Huang
Broadcom Greenplum
Tender Wang <tndrwang@gmail.com> 于2024年1月26日周五 14:21写道：
>
>
> Tender Wang <tndrwang@gmail.com> 于2024年1月26日周五 12:54写道：
>
>>
>>
>> Michael Paquier <michael@paquier.xyz> 于2024年1月26日周五 11:41写道：
>>
>>> On Fri, Jan 26, 2024 at 11:33:24AM +0800, Tender Wang wrote:
>>> > How about using list *res replace int *res in getTokenTypes(), so we
>>> can
>>> > use list_append_unique().
>>> > In outer function, use list *tokens to replace int *tokens, and
>>> ntokens =
>>> > list_length(tokens) not list_length(stmt->tokentype).
>>>
>>> Yeah, I was wondering about this code path. If you feel strongly
>>> about that, would you like to write a patch?
>>>
>>
>> Ok, I try to fix it.
>>
>
> As I said before, return List looks like not complicated to solve this
> issue.
> I found another problem, it didn't report NOTICE if SQL has IF EXISTS, for
> example:
>
> postgres=# alter text search configuration ispell_tst drop mapping if
> exists for test;
> ERROR: token type "test" does not exist
>
> So I change the func getTokenTypes() interface and remove
> DropConfigurationMapping() error report into getTokenTypes().
>
>
> --
>>> Michael
>>>
>>
>>
>> --
>> Tender Wang
>> OpenPie: https://en.openpie.com/
>>
>
>
> --
> Tender Wang
> OpenPie: https://en.openpie.com/
>
Attachments:
[application/octet-stream] 0001-Fix-error-when-process-duplicate-token-in-tsearch.patch (7.5K, ../../CAOe1Go1N1XHYMXOq6+0=Awcpngmc9ei+3Jou2Om6o=giW+3pvw@mail.gmail.com/3-0001-Fix-error-when-process-duplicate-token-in-tsearch.patch)
download | inline diff:
From 8e4ae9249c70ca5b142962664b621af416de903e Mon Sep 17 00:00:00 2001
From: Yongtao Huang <yongtah2022@gmail.com>
Date: Sun, 28 Jan 2024 01:13:14 +0800
Subject: [PATCH] Fix error when process duplicate token in tsearch
Author: Tender Wang <tndrwang@gmail.com>
---
src/backend/commands/tsearchcmds.c | 85 ++++++++++++++---------------------
src/test/regress/expected/tsdicts.out | 5 +++
src/test/regress/sql/tsdicts.sql | 7 +++
3 files changed, 45 insertions(+), 52 deletions(-)
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 0767172..0e82695 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -1194,20 +1194,19 @@ AlterTSConfiguration(AlterTSConfigurationStmt *stmt)
/*
* Translate a list of token type names to an array of token type numbers
*/
-static int *
-getTokenTypes(Oid prsId, List *tokennames)
+static List *
+getTokenTypes(Oid prsId, AlterTSConfigurationStmt *stmt)
{
TSParserCacheEntry *prs = lookup_ts_parser_cache(prsId);
LexDescr *list;
- int *res,
- i,
- ntoken;
+ List *res = NIL;
+ int ntoken;
ListCell *tn;
+ List *tokennames = stmt->tokentype;
ntoken = list_length(tokennames);
if (ntoken == 0)
- return NULL;
- res = (int *) palloc(sizeof(int) * ntoken);
+ return NIL;
if (!OidIsValid(prs->lextypeOid))
elog(ERROR, "method lextype isn't defined for text search parser %u",
@@ -1217,7 +1216,6 @@ getTokenTypes(Oid prsId, List *tokennames)
list = (LexDescr *) DatumGetPointer(OidFunctionCall1(prs->lextypeOid,
(Datum) 0));
- i = 0;
foreach(tn, tokennames)
{
String *val = lfirst_node(String, tn);
@@ -1229,18 +1227,26 @@ getTokenTypes(Oid prsId, List *tokennames)
{
if (strcmp(strVal(val), list[j].alias) == 0)
{
- res[i] = list[j].lexid;
+ res = list_append_unique_int(res, list[j].lexid);
found = true;
break;
}
j++;
}
if (!found)
- ereport(ERROR,
+ {
+ if (!stmt->missing_ok)
+ ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("token type \"%s\" does not exist",
- strVal(val))));
- i++;
+ errmsg("token type \"%s\" does not exist",
+ strVal(val))));
+ else
+ {
+ ereport(NOTICE,
+ (errmsg("token type \"%s\" does not exist, skipping",
+ strVal(val))));
+ }
+ }
}
return res;
@@ -1261,8 +1267,8 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
int i;
int j;
Oid prsId;
- int *tokens,
- ntoken;
+ List *tokens = NIL;
+ int ntoken;
Oid *dictIds;
int ndict;
ListCell *c;
@@ -1272,15 +1278,15 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
cfgId = tsform->oid;
prsId = tsform->cfgparser;
- tokens = getTokenTypes(prsId, stmt->tokentype);
- ntoken = list_length(stmt->tokentype);
+ tokens = getTokenTypes(prsId, stmt);
+ ntoken = list_length(tokens);
if (stmt->override)
{
/*
* delete maps for tokens if they exist and command was ALTER
*/
- for (i = 0; i < ntoken; i++)
+ foreach(c, tokens)
{
ScanKeyInit(&skey[0],
Anum_pg_ts_config_map_mapcfg,
@@ -1289,7 +1295,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyInit(&skey[1],
Anum_pg_ts_config_map_maptokentype,
BTEqualStrategyNumber, F_INT4EQ,
- Int32GetDatum(tokens[i]));
+ Int32GetDatum(lfirst_int(c)));
scan = systable_beginscan(relMap, TSConfigMapIndexId, true,
NULL, 2, skey);
@@ -1346,9 +1352,9 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
{
bool tokmatch = false;
- for (j = 0; j < ntoken; j++)
+ foreach(c, tokens)
{
- if (cfgmap->maptokentype == tokens[j])
+ if (cfgmap->maptokentype == lfirst_int(c))
{
tokmatch = true;
break;
@@ -1401,7 +1407,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
/*
* Insertion of new entries
*/
- for (i = 0; i < ntoken; i++)
+ foreach(c, tokens)
{
for (j = 0; j < ndict; j++)
{
@@ -1411,7 +1417,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
slot[slotCount]->tts_tupleDescriptor->natts * sizeof(bool));
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapcfg - 1] = ObjectIdGetDatum(cfgId);
- slot[slotCount]->tts_values[Anum_pg_ts_config_map_maptokentype - 1] = Int32GetDatum(tokens[i]);
+ slot[slotCount]->tts_values[Anum_pg_ts_config_map_maptokentype - 1] = Int32GetDatum(lfirst_int(c));
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapseqno - 1] = Int32GetDatum(j + 1);
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapdict - 1] = ObjectIdGetDatum(dictIds[j]);
@@ -1455,23 +1461,18 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyData skey[2];
SysScanDesc scan;
HeapTuple maptup;
- int i;
Oid prsId;
- int *tokens;
+ List *tokens = NIL;
ListCell *c;
tsform = (Form_pg_ts_config) GETSTRUCT(tup);
cfgId = tsform->oid;
prsId = tsform->cfgparser;
- tokens = getTokenTypes(prsId, stmt->tokentype);
+ tokens = getTokenTypes(prsId, stmt);
- i = 0;
- foreach(c, stmt->tokentype)
+ foreach(c, tokens)
{
- String *val = lfirst_node(String, c);
- bool found = false;
-
ScanKeyInit(&skey[0],
Anum_pg_ts_config_map_mapcfg,
BTEqualStrategyNumber, F_OIDEQ,
@@ -1479,7 +1480,7 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyInit(&skey[1],
Anum_pg_ts_config_map_maptokentype,
BTEqualStrategyNumber, F_INT4EQ,
- Int32GetDatum(tokens[i]));
+ Int32GetDatum(lfirst_int(c)));
scan = systable_beginscan(relMap, TSConfigMapIndexId, true,
NULL, 2, skey);
@@ -1487,29 +1488,9 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
while (HeapTupleIsValid((maptup = systable_getnext(scan))))
{
CatalogTupleDelete(relMap, &maptup->t_self);
- found = true;
}
systable_endscan(scan);
-
- if (!found)
- {
- if (!stmt->missing_ok)
- {
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("mapping for token type \"%s\" does not exist",
- strVal(val))));
- }
- else
- {
- ereport(NOTICE,
- (errmsg("mapping for token type \"%s\" does not exist, skipping",
- strVal(val))));
- }
- }
-
- i++;
}
EventTriggerCollectAlterTSConfig(stmt, cfgId, NULL, 0);
diff --git a/src/test/regress/expected/tsdicts.out b/src/test/regress/expected/tsdicts.out
index c804293..af69197 100644
--- a/src/test/regress/expected/tsdicts.out
+++ b/src/test/regress/expected/tsdicts.out
@@ -687,3 +687,8 @@ CREATE TEXT SEARCH DICTIONARY tsdict_case
"AffFile" = ispell_sample
);
ERROR: unrecognized Ispell parameter: "DictFile"
+-- test duplicate token type
+CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst ALTER MAPPING FOR word, word WITH ispell;
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst DROP MAPPING IF EXISTS FOR hello;
+NOTICE: token type "hello" does not exist, skipping
diff --git a/src/test/regress/sql/tsdicts.sql b/src/test/regress/sql/tsdicts.sql
index ddc6c7f..d4302ab 100644
--- a/src/test/regress/sql/tsdicts.sql
+++ b/src/test/regress/sql/tsdicts.sql
@@ -251,3 +251,10 @@ CREATE TEXT SEARCH DICTIONARY tsdict_case
"DictFile" = ispell_sample,
"AffFile" = ispell_sample
);
+
+-- test duplicate token type
+CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
+
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst ALTER MAPPING FOR word, word WITH ispell;
+
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst DROP MAPPING IF EXISTS FOR hello;
--
1.8.3.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-28 23:18 Michael Paquier <michael@paquier.xyz>
parent: Yongtao Huang <yongtaoh2022@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2024-01-28 23:18 UTC (permalink / raw)
To: Yongtao Huang <yongtaoh2022@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs@lists.postgresql.org
On Sun, Jan 28, 2024 at 01:32:08AM +0800, Yongtao Huang wrote:
> Hi Tender Wang,
>
> I think your code changes *LGTM*. Thanks for your contribution.
Yes, incorrect indentation is something I have noticed in the previous
patches you have sent to the lists: all the new lines have an
incorrect indentation. See here, where it is mentioned that the code
base uses 4 column tab spacing with tabs preserved:
https://www.postgresql.org/docs/devel/source-format.html
FWIW, I am OK for simple patches, but this is going to make reviews
harder when it comes to read more complicated patches that touch more
code paths.
> Broadcom Greenplum
Glad to see you're still alive and around.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbbgvVe_xbi-1U6n@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-28 23:34 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Michael Paquier @ 2024-01-28 23:34 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Fri, Jan 26, 2024 at 11:23:08AM +0800, Tender Wang wrote:
> Michael Paquier <michael@paquier.xyz> 于2024年1月26日周五 10:51写道：
>> 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.
>
> Agreed. +1
Thanks, I've applied this one down v16 as of 50b797dc99ec. v15 and
older do not use a two-pass logic in DropRole() so one would fail with
a simpler "role does not exist", which is kind of correct in its own
way because the second role cannot be scanned in pg_authid after the
CCI of the first one, so I have let it be in older versions. It's not
like somebody complained about that in the past, either, as far as I
recall.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbbkaguEsLj42Spb@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-29 03:27 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-29 03:27 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Yongtao Huang <yongtaoh2022@gmail.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> 于2024年1月29日周一 07:18写道：
> On Sun, Jan 28, 2024 at 01:32:08AM +0800, Yongtao Huang wrote:
> > Hi Tender Wang,
> >
> > I think your code changes *LGTM*. Thanks for your contribution.
>
Thanks for reviewing this patch and fixing the indentation.
>
> Yes, incorrect indentation is something I have noticed in the previous
> patches you have sent to the lists: all the new lines have an
> incorrect indentation. See here, where it is mentioned that the code
> base uses 4 column tab spacing with tabs preserved:
> https://www.postgresql.org/docs/devel/source-format.html
>
> FWIW, I am OK for simple patches, but this is going to make reviews
> harder when it comes to read more complicated patches that touch more
> code paths.
>
Sorry, my mistake. Thanks for reminding. Yongtao has help me fix the
indentation issue in his patch,
and I don't have new code to add.
> > Broadcom Greenplum
>
> Glad to see you're still alive and around.
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 01:14 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Michael Paquier @ 2024-01-30 01:14 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Yongtao Huang <yongtaoh2022@gmail.com>; pgsql-bugs@lists.postgresql.org
On Mon, Jan 29, 2024 at 11:27:15AM +0800, Tender Wang wrote:
> Michael Paquier <michael@paquier.xyz> 于2024年1月29日周一 07:18写道：
>> Yes, incorrect indentation is something I have noticed in the previous
>> patches you have sent to the lists: all the new lines have an
>> incorrect indentation. See here, where it is mentioned that the code
>> base uses 4 column tab spacing with tabs preserved:
>> https://www.postgresql.org/docs/devel/source-format.html
>>
>> FWIW, I am OK for simple patches, but this is going to make reviews
>> harder when it comes to read more complicated patches that touch more
>> code paths.
>>
>
> Sorry, my mistake. Thanks for reminding. Yongtao has help me fix the
> indentation issue in his patch,
> and I don't have new code to add.
The latest version posted on [1] was still not right. I got a lot of
complaints from a simple `git diff --check`. I am just looking at the
patch, no need to send something else.
[1]: https://www.postgresql.org/message-id/CAHewXNk5yeUo2XPaqTYSfbkKhEBfKd4VYa7NHv2MzV-mJ9ULpA@mail.gmail...
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbhNktmvYQXQRAob@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 06:28 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2024-01-30 06:28 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Fri, Jan 26, 2024 at 02:21:12PM +0800, Tender Wang wrote:
> As I said before, return List looks like not complicated to solve this
> issue.
> I found another problem, it didn't report NOTICE if SQL has IF EXISTS, for
> example:
>
> postgres=# alter text search configuration ispell_tst drop mapping if
> exists for test;
> ERROR: token type "test" does not exist
It seems to me that there is a misunderstanding here, because this
query is correct depending on the parser used by a tsearch
configuration, and the code. See details below.
> So I change the func getTokenTypes() interface and remove
> DropConfigurationMapping() error report into getTokenTypes().
+ if (!stmt->missing_ok)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("token type \"%s\" does not exist",
+ strVal(val))));
+ else
+ {
+ ereport(NOTICE,
+ (errmsg("token type \"%s\" does not exist, skipping",
+ strVal(val))));
+ }
[ ... ]
- if (!found)
- {
- if (!stmt->missing_ok)
- {
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("mapping for token type \"%s\" does not exist",
- strVal(val))));
- }
- else
- {
- ereport(NOTICE,
- (errmsg("mapping for token type \"%s\" does not exist, skipping",
- strVal(val))));
- }
- }
Your patch looks incorrect to me, and changes existing behaviors
because of the changes you have done in both getTokenTypes() *and*
DropConfigurationMapping() because the error handling you have removed
can be reached when attempting to drop a token that is included in the
parser but has *no* mapping. For example, reusing the test of your
patch, this would include a bunch of tokens:
CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
\dF+ dup_ispell_tst
On HEAD, attempting to drop a token that does not exist for the
configuration fails, because the defined token is not part of the
parser:
=# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
DROP MAPPING IF EXISTS FOR not_here;
ERROR: 22023: token type "not_here" does not exist
But if a token is dropped the configuration with the parser supporting
it, we'd get that:
=# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
DROP MAPPING FOR word;
ALTER TEXT SEARCH CONFIGURATION
=# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
DROP MAPPING IF EXISTS FOR word;
NOTICE: 00000: mapping for token type "word" does not exist, skipping
With the patch and the same queries as previously, we now get that:
=# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
DROP MAPPING IF EXISTS FOR not_here;
NOTICE: 00000: token type "not_here" does not exist, skipping
=# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
DROP MAPPING IF EXISTS FOR word;
ALTER TEXT SEARCH CONFIGURATION
=# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
DROP MAPPING IF EXISTS FOR word;
ALTER TEXT SEARCH CONFIGURATION
This is incorrect for two reasons:
- In the first query, the mapping "not_here" is not part of the
parser, and this pattern *has to* be a hard ERROR even on IF EXISTS.
- The token "word" is part in the parser, and we finish by incorrectly
reporting that it gets dropped, missing the NOTICE from the IF EXISTS.
So the patch makes a worse experience the user, because he/she then
does not know if the mapping has been dropped or not, while not
knowing anymore if the mapping was part of the parser used by the
configuration.
The root of the problem is that we should fail for an unknown token
name and we skip a failure if the mapping does not exist with IF
EXISTS, while still knowing the name of the token to be able to report
it correctly, and while handling duplicates in getTokenTypes(). So
the patch should *not* change the error checks at all.
I think that we should tweak getTokenTypes() so as we return *two*
Lists, one for the IDs and a second with the token names, then use
forboth() in DropConfigurationMapping() with the two lists and a
simple foreach in MakeConfigurationMapping() when overriding the
mappings, while getTokenTypes() checks if a number is in the first
list before adding the name of a token in the second list. Or we
could use a simple list with pointers to a local structure, but the
second list is only needed by DropConfigurationMapping(). That would
enforce a correct order of the token names and numbers, at least.
I would be tempted to just use one List with a structure (number,
token_name). It makes the checks for duplicates O(N^2) but we will
never have hundreds of mapping entries in these DDL queries.
What's your pick?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbiXEa_zkw3ZYoga@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 07:29 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-30 07:29 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> 于2024年1月30日周二 14:28写道：
> On Fri, Jan 26, 2024 at 02:21:12PM +0800, Tender Wang wrote:
> > As I said before, return List looks like not complicated to solve this
> > issue.
> > I found another problem, it didn't report NOTICE if SQL has IF EXISTS,
> for
> > example:
> >
> > postgres=# alter text search configuration ispell_tst drop mapping if
> > exists for test;
> > ERROR: token type "test" does not exist
>
> It seems to me that there is a misunderstanding here, because this
> query is correct depending on the parser used by a tsearch
> configuration, and the code. See details below.
>
Yeah, you are right.
>
> > So I change the func getTokenTypes() interface and remove
> > DropConfigurationMapping() error report into getTokenTypes().
>
> + if (!stmt->missing_ok)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("token type \"%s\" does not exist",
> + strVal(val))));
> + else
> + {
> + ereport(NOTICE,
> + (errmsg("token type \"%s\" does not exist,
> skipping",
> + strVal(val))));
> + }
> [ ... ]
> - if (!found)
> - {
> - if (!stmt->missing_ok)
> - {
> - ereport(ERROR,
> - (errcode(ERRCODE_UNDEFINED_OBJECT),
> - errmsg("mapping for token type \"%s\" does not
> exist",
> - strVal(val))));
> - }
> - else
> - {
> - ereport(NOTICE,
> - (errmsg("mapping for token type \"%s\" does not
> exist, skipping",
> - strVal(val))));
> - }
> - }
>
> Your patch looks incorrect to me, and changes existing behaviors
> because of the changes you have done in both getTokenTypes() *and*
> DropConfigurationMapping() because the error handling you have removed
> can be reached when attempting to drop a token that is included in the
> parser but has *no* mapping. For example, reusing the test of your
> patch, this would include a bunch of tokens:
> CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
> \dF+ dup_ispell_tst
>
> On HEAD, attempting to drop a token that does not exist for the
> configuration fails, because the defined token is not part of the
> parser:
> =# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
> DROP MAPPING IF EXISTS FOR not_here;
> ERROR: 22023: token type "not_here" does not exist
> But if a token is dropped the configuration with the parser supporting
> it, we'd get that:
> =# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
> DROP MAPPING FOR word;
> ALTER TEXT SEARCH CONFIGURATION
> =# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
> DROP MAPPING IF EXISTS FOR word;
> NOTICE: 00000: mapping for token type "word" does not exist, skipping
>
> With the patch and the same queries as previously, we now get that:
> =# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
> DROP MAPPING IF EXISTS FOR not_here;
> NOTICE: 00000: token type "not_here" does not exist, skipping
> =# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
> DROP MAPPING IF EXISTS FOR word;
> ALTER TEXT SEARCH CONFIGURATION
> =# ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst
> DROP MAPPING IF EXISTS FOR word;
> ALTER TEXT SEARCH CONFIGURATION
>
> Thanks for your explanation.
> This is incorrect for two reasons:
> - In the first query, the mapping "not_here" is not part of the
> parser, and this pattern *has to* be a hard ERROR even on IF EXISTS.
> - The token "word" is part in the parser, and we finish by incorrectly
> reporting that it gets dropped, missing the NOTICE from the IF EXISTS.
>
> So the patch makes a worse experience the user, because he/she then
> does not know if the mapping has been dropped or not, while not
> knowing anymore if the mapping was part of the parser used by the
> configuration.
>
> The root of the problem is that we should fail for an unknown token
> name and we skip a failure if the mapping does not exist with IF
> EXISTS, while still knowing the name of the token to be able to report
> it correctly, and while handling duplicates in getTokenTypes(). So
> the patch should *not* change the error checks at all.
>
>
Yeah, it works for MakeConfigurationMapping() when just returns list, but
for DropConfigurationMapping(),
it need to know the non-exist token name. So it doesn't work if we just
return oid list in getTokenTypes().
When I try to fix the problem, I found that getTokenType() will report
ERROR and DropConfigurationMapping()
will report ERROR too. I felt a little confused at that time.
I misunderstund the behavior in getTokenTypes() and in
DropConfigurationMapping(). I only realized it can fix the reported issue.
I think that we should tweak getTokenTypes() so as we return *two*
> Lists, one for the IDs and a second with the token names, then use
> forboth() in DropConfigurationMapping() with the two lists and a
> simple foreach in MakeConfigurationMapping() when overriding the
> mappings, while getTokenTypes() checks if a number is in the first
> list before adding the name of a token in the second list. Or we
> could use a simple list with pointers to a local structure, but the
> second list is only needed by DropConfigurationMapping(). That would
> enforce a correct order of the token names and numbers, at least.
> I would be tempted to just use one List with a structure (number,
> token_name). It makes the checks for duplicates O(N^2) but we will
> never have hundreds of mapping entries in these DDL queries.
>
Hmm, I agree with you.
>
> What's your pick?
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 07:42 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2024-01-30 07:42 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Tue, Jan 30, 2024 at 03:29:46PM +0800, Tender Wang wrote:
> Michael Paquier <michael@paquier.xyz> 于2024年1月30日周二 14:28写道：
> I misunderstund the behavior in getTokenTypes() and in
> DropConfigurationMapping(). I only realized it can fix the reported issue.
Well, I don't think you should blame yourself, so no worries we are
all here to learn :)
tsdicts.sql has little to no coverage of the various grammar flavors
we are discussing here, so we should try to close the hole with all
the expectations I've just guessed while analyzing the code and the
patch. So it is a
problem of fixing the root issue as much as expanding the regression
tests with token names and IF EXISTS.
>> I think that we should tweak getTokenTypes() so as we return *two*
>> Lists, one for the IDs and a second with the token names, then use
>> forboth() in DropConfigurationMapping() with the two lists and a
>> simple foreach in MakeConfigurationMapping() when overriding the
>> mappings, while getTokenTypes() checks if a number is in the first
>> list before adding the name of a token in the second list. Or we
>> could use a simple list with pointers to a local structure, but the
>> second list is only needed by DropConfigurationMapping(). That would
>> enforce a correct order of the token names and numbers, at least.
>> I would be tempted to just use one List with a structure (number,
>> token_name). It makes the checks for duplicates O(N^2) but we will
>> never have hundreds of mapping entries in these DDL queries.
>>
>
> Hmm, I agree with you.
Perhaps you'd like to give it a second try? Based on my suggestion,
you just need to englobe the tokens retrieved into a single structure
like that:
typedef struct
{
int num;
char *name;
} TSTokenTypes;
Then handle a List of these (could be as well an array with its length
returned as an output argument of getTokenTypes(), to keep a
non-duplicated list of tokens with a strict mapping between token name
and token number.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbioUp1xRNnkRfkN@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 08:02 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-30 08:02 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> 于2024年1月30日周二 15:42写道：
> On Tue, Jan 30, 2024 at 03:29:46PM +0800, Tender Wang wrote:
> > Michael Paquier <michael@paquier.xyz> 于2024年1月30日周二 14:28写道：
> > I misunderstund the behavior in getTokenTypes() and in
> > DropConfigurationMapping(). I only realized it can fix the reported
> issue.
>
> Well, I don't think you should blame yourself, so no worries we are
> all here to learn :)
>
> tsdicts.sql has little to no coverage of the various grammar flavors
> we are discussing here, so we should try to close the hole with all
> the expectations I've just guessed while analyzing the code and the
> patch. So it is a
> problem of fixing the root issue as much as expanding the regression
> tests with token names and IF EXISTS.
>
> >> I think that we should tweak getTokenTypes() so as we return *two*
> >> Lists, one for the IDs and a second with the token names, then use
> >> forboth() in DropConfigurationMapping() with the two lists and a
> >> simple foreach in MakeConfigurationMapping() when overriding the
> >> mappings, while getTokenTypes() checks if a number is in the first
> >> list before adding the name of a token in the second list. Or we
> >> could use a simple list with pointers to a local structure, but the
> >> second list is only needed by DropConfigurationMapping(). That would
> >> enforce a correct order of the token names and numbers, at least.
> >> I would be tempted to just use one List with a structure (number,
> >> token_name). It makes the checks for duplicates O(N^2) but we will
> >> never have hundreds of mapping entries in these DDL queries.
> >>
> >
> > Hmm, I agree with you.
>
> Perhaps you'd like to give it a second try? Based on my suggestion,
> you just need to englobe the tokens retrieved into a single structure
> like that:
> typedef struct
> {
> int num;
> char *name;
> } TSTokenTypes;
>
> Then handle a List of these (could be as well an array with its length
> returned as an output argument of getTokenTypes(), to keep a
> non-duplicated list of tokens with a strict mapping between token name
> and token number.
>
Thanks a lot. I want to give it a second try. I will send a patch based on
your suggestion.
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 08:14 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2024-01-30 08:14 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Tue, Jan 30, 2024 at 04:02:06PM +0800, Tender Wang wrote:
> Thanks a lot. I want to give it a second try. I will send a patch based on
> your suggestion.
Sure, feel free.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Zbiv_y06kN5_nuqY@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-30 11:51 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 20+ messages in thread
From: Tender Wang @ 2024-01-30 11:51 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
I refactor previes patch based on your suggestion, please review the new
attached patch.
Michael Paquier <michael@paquier.xyz> 于2024年1月30日周二 16:15写道：
> On Tue, Jan 30, 2024 at 04:02:06PM +0800, Tender Wang wrote:
> > Thanks a lot. I want to give it a second try. I will send a patch based
> on
> > your suggestion.
>
> Sure, feel free.
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
[application/octet-stream] 0001-Fix-tuple-already-updated-by-self-error-when-duplica.patch (8.6K, ../../CAHewXNnMT1tYsxnnjUSAxMpi2v2DPR6-hABpCBgWcSJxCJrfEw@mail.gmail.com/3-0001-Fix-tuple-already-updated-by-self-error-when-duplica.patch)
download | inline diff:
From 4a0e73a36073d5d6d3381a257387441195516e49 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Tue, 30 Jan 2024 19:20:00 +0800
Subject: [PATCH] Fix "tuple already updated by self" error when duplicate
token type name in ALTER TEXT SEARCH CONFIGURATION xxx ALTER MAPPING FOR.
Currently, we don't remove duplicated token type name in getTokenTypes().
Above error will report in MakeConfigurationMapping() when meets duplicated
token type name.
We remove duplicated token type name in getTokenTypes() and return List * not
int *. DropConfigurationMapping() need token type name when reports error,
so the returned List point to a structue, which not only has token type
numbuer, but has the token type name.
---
src/backend/commands/tsearchcmds.c | 98 +++++++++++++++++++--------
src/test/regress/expected/tsdicts.out | 3 +
src/test/regress/sql/tsdicts.sql | 5 ++
3 files changed, 76 insertions(+), 30 deletions(-)
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 07671720ea..3905dd2897 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -48,6 +48,12 @@
#include "utils/rel.h"
#include "utils/syscache.h"
+/* result structure for getTokenTypes() */
+typedef struct
+{
+ int num; /* token type number */
+ char *name; /* token type name */
+} TSTokenTypes;
static void MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
HeapTuple tup, Relation relMap);
@@ -1192,22 +1198,47 @@ AlterTSConfiguration(AlterTSConfigurationStmt *stmt)
}
/*
- * Translate a list of token type names to an array of token type numbers
+ * Test whether a token type name is present in a TSTokenTypes list.
+ *
+ * We return "true" if a name is already in the list.
+ */
+static bool
+token_type_name_present(char *token_name, List *tokens)
+{
+ ListCell *c;
+ bool found = false;
+
+ foreach(c, tokens)
+ {
+ TSTokenTypes *ts = (TSTokenTypes *) lfirst(c);
+
+ if (strcmp(token_name, ts->name) == 0)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ return found;
+}
+
+/*
+ * Translate a list of token type names to an array of token type numbers.
+ *
+ * We remove duplicated entry in tokennames list, and return a list of structure.
*/
-static int *
+static List *
getTokenTypes(Oid prsId, List *tokennames)
{
TSParserCacheEntry *prs = lookup_ts_parser_cache(prsId);
LexDescr *list;
- int *res,
- i,
- ntoken;
+ List *result = NIL;
+ int ntoken;
ListCell *tn;
ntoken = list_length(tokennames);
if (ntoken == 0)
- return NULL;
- res = (int *) palloc(sizeof(int) * ntoken);
+ return NIL;
if (!OidIsValid(prs->lextypeOid))
elog(ERROR, "method lextype isn't defined for text search parser %u",
@@ -1217,19 +1248,25 @@ getTokenTypes(Oid prsId, List *tokennames)
list = (LexDescr *) DatumGetPointer(OidFunctionCall1(prs->lextypeOid,
(Datum) 0));
- i = 0;
foreach(tn, tokennames)
{
String *val = lfirst_node(String, tn);
bool found = false;
int j;
+ /* Skip if the token type name is present in the list */
+ if (token_type_name_present(strVal(val), result))
+ continue;
+
j = 0;
while (list && list[j].lexid)
{
if (strcmp(strVal(val), list[j].alias) == 0)
{
- res[i] = list[j].lexid;
+ TSTokenTypes *ts = (TSTokenTypes *) palloc(sizeof(TSTokenTypes));
+ ts->num = list[j].lexid;
+ ts->name = pstrdup(strVal(val));
+ result = lappend(result, ts);
found = true;
break;
}
@@ -1240,10 +1277,9 @@ getTokenTypes(Oid prsId, List *tokennames)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("token type \"%s\" does not exist",
strVal(val))));
- i++;
}
- return res;
+ return result;
}
/*
@@ -1261,8 +1297,8 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
int i;
int j;
Oid prsId;
- int *tokens,
- ntoken;
+ List *tokens = NIL;
+ int ntoken;
Oid *dictIds;
int ndict;
ListCell *c;
@@ -1273,15 +1309,17 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
prsId = tsform->cfgparser;
tokens = getTokenTypes(prsId, stmt->tokentype);
- ntoken = list_length(stmt->tokentype);
+ ntoken = list_length(tokens);
if (stmt->override)
{
/*
* delete maps for tokens if they exist and command was ALTER
*/
- for (i = 0; i < ntoken; i++)
+ foreach(c, tokens)
{
+ TSTokenTypes *ts = (TSTokenTypes *) lfirst(c);
+
ScanKeyInit(&skey[0],
Anum_pg_ts_config_map_mapcfg,
BTEqualStrategyNumber, F_OIDEQ,
@@ -1289,7 +1327,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyInit(&skey[1],
Anum_pg_ts_config_map_maptokentype,
BTEqualStrategyNumber, F_INT4EQ,
- Int32GetDatum(tokens[i]));
+ Int32GetDatum(ts->num));
scan = systable_beginscan(relMap, TSConfigMapIndexId, true,
NULL, 2, skey);
@@ -1346,9 +1384,11 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
{
bool tokmatch = false;
- for (j = 0; j < ntoken; j++)
+ foreach(c, tokens)
{
- if (cfgmap->maptokentype == tokens[j])
+ TSTokenTypes *ts = (TSTokenTypes *) lfirst(c);
+
+ if (cfgmap->maptokentype == ts->num)
{
tokmatch = true;
break;
@@ -1401,8 +1441,10 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
/*
* Insertion of new entries
*/
- for (i = 0; i < ntoken; i++)
+ foreach(c, tokens)
{
+ TSTokenTypes *ts = (TSTokenTypes *) lfirst(c);
+
for (j = 0; j < ndict; j++)
{
ExecClearTuple(slot[slotCount]);
@@ -1411,7 +1453,7 @@ MakeConfigurationMapping(AlterTSConfigurationStmt *stmt,
slot[slotCount]->tts_tupleDescriptor->natts * sizeof(bool));
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapcfg - 1] = ObjectIdGetDatum(cfgId);
- slot[slotCount]->tts_values[Anum_pg_ts_config_map_maptokentype - 1] = Int32GetDatum(tokens[i]);
+ slot[slotCount]->tts_values[Anum_pg_ts_config_map_maptokentype - 1] = Int32GetDatum(ts->num);
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapseqno - 1] = Int32GetDatum(j + 1);
slot[slotCount]->tts_values[Anum_pg_ts_config_map_mapdict - 1] = ObjectIdGetDatum(dictIds[j]);
@@ -1455,9 +1497,8 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyData skey[2];
SysScanDesc scan;
HeapTuple maptup;
- int i;
Oid prsId;
- int *tokens;
+ List *tokens = NIL;
ListCell *c;
tsform = (Form_pg_ts_config) GETSTRUCT(tup);
@@ -1466,10 +1507,9 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
tokens = getTokenTypes(prsId, stmt->tokentype);
- i = 0;
- foreach(c, stmt->tokentype)
+ foreach(c, tokens)
{
- String *val = lfirst_node(String, c);
+ TSTokenTypes *ts = (TSTokenTypes *) lfirst(c);
bool found = false;
ScanKeyInit(&skey[0],
@@ -1479,7 +1519,7 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ScanKeyInit(&skey[1],
Anum_pg_ts_config_map_maptokentype,
BTEqualStrategyNumber, F_INT4EQ,
- Int32GetDatum(tokens[i]));
+ Int32GetDatum(ts->num));
scan = systable_beginscan(relMap, TSConfigMapIndexId, true,
NULL, 2, skey);
@@ -1499,17 +1539,15 @@ DropConfigurationMapping(AlterTSConfigurationStmt *stmt,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("mapping for token type \"%s\" does not exist",
- strVal(val))));
+ ts->name)));
}
else
{
ereport(NOTICE,
(errmsg("mapping for token type \"%s\" does not exist, skipping",
- strVal(val))));
+ ts->name)));
}
}
-
- i++;
}
EventTriggerCollectAlterTSConfig(stmt, cfgId, NULL, 0);
diff --git a/src/test/regress/expected/tsdicts.out b/src/test/regress/expected/tsdicts.out
index c804293142..94aed1164b 100644
--- a/src/test/regress/expected/tsdicts.out
+++ b/src/test/regress/expected/tsdicts.out
@@ -687,3 +687,6 @@ CREATE TEXT SEARCH DICTIONARY tsdict_case
"AffFile" = ispell_sample
);
ERROR: unrecognized Ispell parameter: "DictFile"
+-- test duplicate token type name
+CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst ALTER MAPPING FOR word, word WITH ispell;
diff --git a/src/test/regress/sql/tsdicts.sql b/src/test/regress/sql/tsdicts.sql
index ddc6c7f445..88db4f4516 100644
--- a/src/test/regress/sql/tsdicts.sql
+++ b/src/test/regress/sql/tsdicts.sql
@@ -251,3 +251,8 @@ CREATE TEXT SEARCH DICTIONARY tsdict_case
"DictFile" = ispell_sample,
"AffFile" = ispell_sample
);
+
+-- test duplicate token type name
+CREATE TEXT SEARCH CONFIGURATION dup_ispell_tst (COPY=english);
+
+ALTER TEXT SEARCH CONFIGURATION dup_ispell_tst ALTER MAPPING FOR word, word WITH ispell;
--
2.25.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-31 04:25 Michael Paquier <michael@paquier.xyz>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Michael Paquier @ 2024-01-31 04:25 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Tue, Jan 30, 2024 at 07:51:45PM +0800, Tender Wang wrote:
> I refactor previes patch based on your suggestion, please review the new
> attached patch.
Thanks for the new patch. I've found the logic to be basically OK,
minus edits with the structure, the routine names and some comments.
Unfortunately, the set of regression tests was too shy because this
only tested for the case of duplicated tokens when overridding
mappings, while missing:
- ADD MAPPING that failed on a constraint failure.
- DROP MAPPING that failed with an deletion failure.
- The case of IF EXISTS with tokens supported or not supported by a
configuration's parser. That was not related to the report of this
patch, but as we've discussed it is very easy to miss the historical
distinction between the way this clause is handled.
In short, there was zero coverage for these code paths. See
DropConfigurationMapping() that was completely red, for one:
https://coverage.postgresql.org/src/backend/commands/tsearchcmds.c.gcov.html
This should be marked as covered pretty soon. I was also hesitating
about the addition of a test with REPLACE, actually..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZbnLp-sHGq389yOD@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self
@ 2024-01-31 08:11 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 20+ messages in thread
From: Tender Wang @ 2024-01-31 08:11 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Thanks for your work on this issue, and thanks for pushing it, again.
Michael Paquier <michael@paquier.xyz> 于2024年1月31日周三 12:25写道：
> On Tue, Jan 30, 2024 at 07:51:45PM +0800, Tender Wang wrote:
> > I refactor previes patch based on your suggestion, please review the new
> > attached patch.
>
> Thanks for the new patch. I've found the logic to be basically OK,
> minus edits with the structure, the routine names and some comments.
>
> Unfortunately, the set of regression tests was too shy because this
> only tested for the case of duplicated tokens when overridding
> mappings, while missing:
> - ADD MAPPING that failed on a constraint failure.
> - DROP MAPPING that failed with an deletion failure.
> - The case of IF EXISTS with tokens supported or not supported by a
> configuration's parser. That was not related to the report of this
> patch, but as we've discussed it is very easy to miss the historical
> distinction between the way this clause is handled.
>
> In short, there was zero coverage for these code paths. See
> DropConfigurationMapping() that was completely red, for one:
>
> https://coverage.postgresql.org/src/backend/commands/tsearchcmds.c.gcov.html
>
> This should be marked as covered pretty soon. I was also hesitating
> about the addition of a test with REPLACE, actually..
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2024-01-31 08:11 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-25 04:00 BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self PG Bug reporting form <noreply@postgresql.org>
2024-01-26 02:51 ` Michael Paquier <michael@paquier.xyz>
2024-01-26 03:23 ` Tender Wang <tndrwang@gmail.com>
2024-01-28 23:34 ` Michael Paquier <michael@paquier.xyz>
2024-01-26 03:33 ` Tender Wang <tndrwang@gmail.com>
2024-01-26 03:41 ` Michael Paquier <michael@paquier.xyz>
2024-01-26 04:54 ` Tender Wang <tndrwang@gmail.com>
2024-01-26 06:21 ` Tender Wang <tndrwang@gmail.com>
2024-01-27 17:32 ` Yongtao Huang <yongtaoh2022@gmail.com>
2024-01-28 23:18 ` Michael Paquier <michael@paquier.xyz>
2024-01-29 03:27 ` Tender Wang <tndrwang@gmail.com>
2024-01-30 01:14 ` Michael Paquier <michael@paquier.xyz>
2024-01-30 06:28 ` Michael Paquier <michael@paquier.xyz>
2024-01-30 07:29 ` Tender Wang <tndrwang@gmail.com>
2024-01-30 07:42 ` Michael Paquier <michael@paquier.xyz>
2024-01-30 08:02 ` Tender Wang <tndrwang@gmail.com>
2024-01-30 08:14 ` Michael Paquier <michael@paquier.xyz>
2024-01-30 11:51 ` Tender Wang <tndrwang@gmail.com>
2024-01-31 04:25 ` Michael Paquier <michael@paquier.xyz>
2024-01-31 08:11 ` Tender Wang <tndrwang@gmail.com>
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