On Wed, 20 Aug 2025 17:01:56 +0900 Yugo Nagata <[email protected]> wrote:
> On Thu, 17 Jul 2025 14:09:14 +0900 > Yugo Nagata <[email protected]> wrote: > > > On Fri, 4 Jul 2025 14:58:05 +0900 > > Yugo Nagata <[email protected]> wrote: > > > > > On Fri, 4 Jul 2025 10:48:26 +0700 > > > Daniil Davydov <[email protected]> wrote: > > > > > > > Hi, > > > > > > > > On Thu, Jul 3, 2025 at 9:18 PM Yugo Nagata <[email protected]> wrote: > > > > > > > > > > On Tue, 1 Jul 2025 18:56:11 +0700 > > > > > Daniil Davydov <[email protected]> wrote: > > > > > > > > > > > CatalogIndexInsert is kinda "popular" function. It can be called in > > > > > > different situations, not in all of which a violation of unique > > > > > > constraint means an error due to competitiveness. > > > > > > > > > > > > For example, with this patch such a query : "CREATE TYPE mood AS > > > > > > ENUM > > > > > > ('happy', 'sad', 'happy');" > > > > > > Will throw this error : "operation failed due to a concurrent > > > > > > command" > > > > > > Of course, it isn't true > > > > > > > > > > You're right — this error is not caused by a concurrent command. > > > > > However, I believe the error message in cases like creating an ENUM > > > > > type with > > > > > duplicate labels could be improved to explain the issue more clearly, > > > > > rather > > > > > than just reporting it as a unique constraint violation. > > > > > > > > > > In any case, a unique constraint violation in a system catalog is not > > > > > necessarily > > > > > due to concurrent DDL. Therefore, the error message shouldn't suggest > > > > > that as the > > > > > only cause. Instead, it should clearly report the constraint > > > > > violation as the primary > > > > > issue, and mention concurrent DDL as just one possible explanation in > > > > > HINT. > > > > > > > > > > I've updated the patch accordingly to reflect this direction in the > > > > > error message. > > > > > > > > > > ERROR: operation failed due to duplicate key object > > > > > DETAIL: Key (proname, proargtypes, pronamespace)=(fnc, , 2200) > > > > > already exists in unique index pg_proc_proname_args_nsp_index. > > > > > HINT: Another command might have created a object with the same key > > > > > in a concurrent session. > > > > > > > > > > However, as a result, the message ends up being similar to the > > > > > current one raised > > > > > by the btree code, so the overall improvement in user-friendliness > > > > > might be limited. > > > > > > > > > > > > > Thanks for updating the patch! > > > > +1 for adding such a hint for this error. I've attached rebase patches. Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
>From efb50c882964c7962483e55563a666cffafdebe6 Mon Sep 17 00:00:00 2001 From: Yugo Nagata <[email protected]> Date: Tue, 1 Jul 2025 18:36:01 +0900 Subject: [PATCH v10 4/4] Improve error reporting for unique key violations in system catalogs Previously, when a unique constraint violation occurred in a system catalog, typically due to a concurrent session creating an object with the same key, a low-level error like the following was raised by nbtree code: ERROR: duplicate key value violates unique constraint ... However, this message is not very user-friendly, as users are not directly inserting rows into the system catalogs. This commit improves the error reporting by generating a more descriptive and user-facing error message in such cases, making it easier to understand the cause of the failure and its likely relation to concurrent DDL activity. --- contrib/test_decoding/expected/replorigin.out | 5 ++-- src/backend/catalog/indexing.c | 24 +++++++++++++++++++ src/backend/executor/execIndexing.c | 19 +++++++++++++++ src/include/executor/executor.h | 5 ++++ .../expected/syscache-update-pruned.out | 2 +- 5 files changed, 52 insertions(+), 3 deletions(-) diff --git a/contrib/test_decoding/expected/replorigin.out b/contrib/test_decoding/expected/replorigin.out index 29a9630c900..e46c9f17d4a 100644 --- a/contrib/test_decoding/expected/replorigin.out +++ b/contrib/test_decoding/expected/replorigin.out @@ -39,8 +39,9 @@ SELECT pg_replication_origin_create('regress_test_decoding: regression_slot'); -- ensure duplicate creations fail SELECT pg_replication_origin_create('regress_test_decoding: regression_slot'); -ERROR: duplicate key value violates unique constraint "pg_replication_origin_roname_index" -DETAIL: Key (roname)=(regress_test_decoding: regression_slot) already exists. +ERROR: could not create object because a conflicting object already exists +DETAIL: Key (roname)=(regress_test_decoding: regression_slot) conflicts with existing entry in unique index pg_replication_origin_roname_index. +HINT: Another session might have created an object with the same key concurrently. -- ensure inactive origin cannot be set as session one if pid is specified SELECT pg_replication_origin_session_setup('regress_test_decoding: regression_slot', -1); ERROR: cannot use PID -1 for inactive replication origin with ID 1 diff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.c index 25c4b6bdc87..5d91eeae16f 100644 --- a/src/backend/catalog/indexing.c +++ b/src/backend/catalog/indexing.c @@ -164,6 +164,30 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple, values, isnull); + /* Check if a concurrent command inserted an entry with the same key */ + if (index->rd_index->indisunique && IsCatalogRelation(heapRelation)) + { + bool satisfied; + EState *estate = CreateExecutorState(); + + BuildSpeculativeIndexInfo(index, indexInfo); + satisfied = check_unique_constraint(heapRelation, + index, indexInfo, + &(heapTuple->t_self), values, isnull, + estate); + + if (!satisfied) + { + char *key_desc = BuildIndexValueDescription(index, values, isnull); + ereport(ERROR, + (errcode(ERRCODE_UNIQUE_VIOLATION), + errmsg("could not create object because a conflicting object already exists"), + errdetail("Key %s conflicts with existing entry in unique index %s.", + key_desc, RelationGetRelationName(index)), + errhint("Another session might have created an object with the same key concurrently."))); + } + } + /* * The index AM does the rest. */ diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index ca33a854278..f87ab861b44 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -965,6 +965,25 @@ check_exclusion_constraint(Relation heap, Relation index, CEOUC_WAIT, false, NULL); } +/* + * Check for violation of a unique constraint + * + * This is a dumbed down version of check_exclusion_or_unique_constraint + * for external callers. They don't need all the special modes. + */ +bool +check_unique_constraint(Relation heap, Relation index, + IndexInfo *indexInfo, + ItemPointer tupleid, + const Datum *values, const bool *isnull, + EState *estate) +{ + return check_exclusion_or_unique_constraint(heap, index, indexInfo, tupleid, + values, isnull, + estate, false, + CEOUC_WAIT, true, NULL); +} + /* * Check existing tuple's index values to see if it really matches the * exclusion condition against the new_values. Returns true if conflict. diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 3248e78cd28..3d1decb54bf 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -752,6 +752,11 @@ extern void check_exclusion_constraint(Relation heap, Relation index, ItemPointer tupleid, const Datum *values, const bool *isnull, EState *estate, bool newIndex); +extern bool check_unique_constraint(Relation heap, Relation index, + IndexInfo *indexInfo, + ItemPointer tupleid, + const Datum *values, const bool *isnull, + EState *estate); /* * prototypes from functions in execReplication.c diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned.out b/src/test/modules/injection_points/expected/syscache-update-pruned.out index 231545a6cbb..79fd4bff43e 100644 --- a/src/test/modules/injection_points/expected/syscache-update-pruned.out +++ b/src/test/modules/injection_points/expected/syscache-update-pruned.out @@ -46,7 +46,7 @@ step wakegrant4: SELECT FROM injection_points_wakeup('heap_update-before-pin'); <waiting ...> step grant1: <... completed> -ERROR: duplicate key value violates unique constraint "pg_class_oid_index" +ERROR: could not create object because a conflicting object already exists step wakegrant4: <... completed> starting permutation: snap3 cachefill1 at2 mkrels4 r3 waitprunable4 vac4 grant1 wakeinval4 at4 wakegrant4 inspect4 -- 2.43.0
>From 99fc9302e85790b41bce49f9a1e6d4adf81fe886 Mon Sep 17 00:00:00 2001 From: Yugo Nagata <[email protected]> Date: Thu, 5 Jun 2025 15:32:37 +0900 Subject: [PATCH v10 3/4] Improve error reporting for concurrent updates on system catalog tuples Previously, when multiple sessions attempted to modify the same system catalog tuple concurrently due to insufficient locking, DDL commands could fail with an internal error: ERROR: tuple concurrently updated This commit improves the behavior by reporting a more appropriate and user-facing error message in such cases, making it easier for users to understand the cause of the failure. --- src/backend/access/heap/heapam.c | 34 ++++++++++++++++--- .../expected/syscache-update-pruned.out | 2 +- .../expected/syscache-update-pruned_1.out | 2 +- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index ed0c0c2dc9f..7e300559a7d 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3202,6 +3202,7 @@ simple_heap_delete(Relation relation, ItemPointer tid) { TM_Result result; TM_FailureData tmfd; + bool is_catalog = IsCatalogRelation(relation); result = heap_delete(relation, tid, GetCurrentCommandId(true), InvalidSnapshot, @@ -3219,11 +3220,23 @@ simple_heap_delete(Relation relation, ItemPointer tid) break; case TM_Updated: - elog(ERROR, "tuple concurrently updated"); + if (is_catalog) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("operation failed due to a concurrent command"), + errdetail("Another command modified the same object in a concurrent session."))); + else + elog(ERROR, "tuple concurrently updated"); break; case TM_Deleted: - elog(ERROR, "tuple concurrently deleted"); + if (is_catalog) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("operation failed due to a concurrent command"), + errdetail("Another command dropped the same object in a concurrent session."))); + else + elog(ERROR, "tuple concurrently deleted"); break; default: @@ -4494,6 +4507,7 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup, TM_Result result; TM_FailureData tmfd; LockTupleMode lockmode; + bool is_catalog = IsCatalogRelation(relation); result = heap_update(relation, otid, tup, GetCurrentCommandId(true), InvalidSnapshot, @@ -4511,11 +4525,23 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup, break; case TM_Updated: - elog(ERROR, "tuple concurrently updated"); + if (is_catalog) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("operation failed due to a concurrent command"), + errdetail("Another command modified the same object in a concurrent session."))); + else + elog(ERROR, "tuple concurrently updated"); break; case TM_Deleted: - elog(ERROR, "tuple concurrently deleted"); + if (is_catalog) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("operation failed due to a concurrent command"), + errdetail("Another command dropped the same object in a concurrent session."))); + else + elog(ERROR, "tuple concurrently deleted"); break; default: diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned.out b/src/test/modules/injection_points/expected/syscache-update-pruned.out index a6a4e8db996..231545a6cbb 100644 --- a/src/test/modules/injection_points/expected/syscache-update-pruned.out +++ b/src/test/modules/injection_points/expected/syscache-update-pruned.out @@ -20,7 +20,7 @@ step wakegrant4: SELECT FROM injection_points_wakeup('heap_update-before-pin'); <waiting ...> step grant1: <... completed> -ERROR: tuple concurrently deleted +ERROR: operation failed due to a concurrent command step wakegrant4: <... completed> starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 mkrels4 wakegrant4 diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned_1.out b/src/test/modules/injection_points/expected/syscache-update-pruned_1.out index 4dca2b86bc8..e94a8b8cdeb 100644 --- a/src/test/modules/injection_points/expected/syscache-update-pruned_1.out +++ b/src/test/modules/injection_points/expected/syscache-update-pruned_1.out @@ -73,7 +73,7 @@ step wakegrant4: SELECT FROM injection_points_wakeup('heap_update-before-pin'); <waiting ...> step grant1: <... completed> -ERROR: tuple concurrently updated +ERROR: operation failed due to a concurrent command step wakegrant4: <... completed> step inspect4: SELECT relhastriggers, relhassubclass FROM pg_class -- 2.43.0
>From e739cc916fc0d224d865531f12e7b476be5fa697 Mon Sep 17 00:00:00 2001 From: Yugo Nagata <[email protected]> Date: Mon, 31 Mar 2025 20:17:07 +0900 Subject: [PATCH v10 2/4] Prevent internal error caused by concurrent ALTER FUNCTION Previously, concurrent ALTER FUNCTION commands could fail with an internal error "tuple concurrently updated". This occurred because multiple sessions attempted to modify the same catalog tuple simultaneously. To prevent this, ensure that an exclusive lock on the function object is acquired earlier in the process. Additionally, if the target function is dropped by another session while waiting for the lock, an appropriate error is raised to indicate the object no longer exists. --- src/backend/commands/functioncmds.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 0335e982b31..5bb03153c0d 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -61,6 +61,7 @@ #include "parser/parse_func.h" #include "parser/parse_type.h" #include "pgstat.h" +#include "storage/lmgr.h" #include "tcop/pquery.h" #include "tcop/utility.h" #include "utils/acl.h" @@ -1383,9 +1384,21 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt) ObjectAddressSet(address, ProcedureRelationId, funcOid); + /* Lock the function so nobody else can do anything with it. */ + LockDatabaseObject(ProcedureRelationId, funcOid, 0, AccessExclusiveLock); + + /* + * It is possible that by the time we acquire the lock on function, + * concurrent DDL has removed it. We can test this by checking the + * existence of function. We get the tuple again to avoid the risk + * of function definition getting changed. + */ tup = SearchSysCacheCopy1(PROCOID, ObjectIdGetDatum(funcOid)); - if (!HeapTupleIsValid(tup)) /* should not happen */ - elog(ERROR, "cache lookup failed for function %u", funcOid); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("function \"%s\" does not exist", + NameListToString(stmt->func->objname))); procForm = (Form_pg_proc) GETSTRUCT(tup); -- 2.43.0
>From a1b049c1c29e5b45d1371fb72106b82d816ae126 Mon Sep 17 00:00:00 2001 From: Yugo Nagata <[email protected]> Date: Mon, 31 Mar 2025 18:46:58 +0900 Subject: [PATCH v10 1/4] Prevent internal error at concurrent CREATE OR REPLACE FUNCTION Previously, concurrent CREATE OR REPLACE FUNCTION commands could fail with an internal error "tuple concurrently updated". This occurred because multiple sessions attempted to modify the same catalog tuple simultaneously. To prevent this, ensure that an exclusive lock on the function object is acquired earlier in the process. Additionally, if the target function is dropped by another session while waiting for the lock, a new function is created instead. --- src/backend/catalog/pg_proc.c | 41 ++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c index b89b9ccda0e..b459badf074 100644 --- a/src/backend/catalog/pg_proc.c +++ b/src/backend/catalog/pg_proc.c @@ -34,6 +34,7 @@ #include "parser/parse_coerce.h" #include "pgstat.h" #include "rewrite/rewriteHandler.h" +#include "storage/lmgr.h" #include "tcop/pquery.h" #include "tcop/tcopprot.h" #include "utils/acl.h" @@ -383,24 +384,48 @@ ProcedureCreate(const char *procedureName, tupDesc = RelationGetDescr(rel); /* Check for pre-existing definition */ - oldtup = SearchSysCache3(PROCNAMEARGSNSP, - PointerGetDatum(procedureName), - PointerGetDatum(parameterTypes), - ObjectIdGetDatum(procNamespace)); + oldtup = SearchSysCacheCopy3(PROCNAMEARGSNSP, + PointerGetDatum(procedureName), + PointerGetDatum(parameterTypes), + ObjectIdGetDatum(procNamespace)); if (HeapTupleIsValid(oldtup)) { /* There is one; okay to replace it? */ Form_pg_proc oldproc = (Form_pg_proc) GETSTRUCT(oldtup); - Datum proargnames; - bool isnull; - const char *dropcmd; + Oid procoid = oldproc->oid; if (!replace) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_FUNCTION), errmsg("function \"%s\" already exists with same argument types", procedureName))); + + heap_freetuple(oldtup); + + /* Lock the function so nobody else can do anything with it. */ + LockDatabaseObject(ProcedureRelationId, procoid, 0, AccessExclusiveLock); + + /* + * It is possible that by the time we acquire the lock on function, + * concurrent DDL has removed it. We can test this by checking the + * existence of function. We get the tuple again to avoid the risk + * of function definition getting changed. + */ + oldtup = SearchSysCacheCopy3(PROCNAMEARGSNSP, + PointerGetDatum(procedureName), + PointerGetDatum(parameterTypes), + ObjectIdGetDatum(procNamespace)); + } + + if (HeapTupleIsValid(oldtup)) + { + + Form_pg_proc oldproc = (Form_pg_proc) GETSTRUCT(oldtup); + Datum proargnames; + bool isnull; + const char *dropcmd; + if (!object_ownercheck(ProcedureRelationId, oldproc->oid, proowner)) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_FUNCTION, procedureName); @@ -585,7 +610,7 @@ ProcedureCreate(const char *procedureName, tup = heap_modify_tuple(oldtup, tupDesc, values, nulls, replaces); CatalogTupleUpdate(rel, &tup->t_self, tup); - ReleaseSysCache(oldtup); + heap_freetuple(oldtup); is_update = true; } else -- 2.43.0
