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

Michael Paquier <michael(at)paquier(dot)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/