Re: BUG #18310: Some SQL commands fail to process duplicate objects with error: tuple already updated by self

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