On Tue, 1 Jul 2025 18:56:11 +0700
Daniil Davydov <3daniss...@gmail.com> wrote:
> Hi,
>
> On Tue, Jul 1, 2025 at 5:47 PM Yugo Nagata <nag...@sraoss.co.jp> wrote:
> >
> > On Mon, 30 Jun 2025 18:32:47 +0700
> > Daniil Davydov <3daniss...@gmail.com> wrote:
> >
> > > On Mon, Jun 30, 2025 at 3:47 PM Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > > >
> > > > I agree with Tom Lane that the behavior is expected, although it would
> > > > be better
> > > > if the error message were more user-friendly. We could improve it by
> > > > checking the
> > > > unique constraint before calling index_insert in CatalogIndexInsert.
> > > >
> > >
> > > As far as I understand, unique constraint checking is specific for
> > > each index access method.
> > > Thus, to implement the proposed idea, you will have to create a
> > > separate callback for check_unique function.
> > > It doesn't seem like a very neat solution, but there aren't many other
> > > options left.
> >
> > I believe check_exclusion_or_unique_constraint() can be used independently
> > of
> > a specific index access method.
> >
> > > I would suggest intercepting the error (via PG_CATCH), and if it has
> > > the ERRCODE_UNIQUE_VIOLATION code, change the error message (more
> > > precisely, throw another error with the desired message).
> > > If we caught an error during the CatalogTupleInsert call, we can be
> > > sure that the problem is in concurrent execution, because before the
> > > insertion, we checked that such a tuple does not exist.
> > >
> > > What do you think? And in general, are you going to fix this behavior
> > > within this thread?
> >
> > Initially, I wasn't planning to do so, but I gave it a try and wrote a
> > patch to fix the issue based on my idea.
>
> Thanks for the patch! Some comments on it :
> 1)
> I found two typos :
> + if (HeapTupleIsHeapOenly(heapTuple) && !onlySummarized)
> and
> + sartisfied = check_unique_constraint(heapRelation,
Thank you for pointing out them. Fixed.
> 2)
> 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.
> That is why I suggested handling unique violations exactly inside
> ProcedureCreate - the only place where we can be sure about reasons of
> error.
If we were to fix the error message outside of CatalogIndexInsert, we would
need to
modify CatalogTupleInsert, CatalogTupleUpdate, and related functions to allow
them to
report the failure appropriately.
You suggested using PG_TRY/PG_CATCH, but these do not suppress the error
message from
the btree code, so this approach seems not to fully address the issue.
Moreover, the places affected are not limited to ProcedureCreate, for example,
concurrent CREATE TABLE commands can also lead to the same situation, and
possibly
other commands as well. Therefore, I think it would be sufficient if the
improved message in CatalogIndexInsert makes sense on its own.
Regards,
Yugo Nagata
--
Yugo Nagata <nag...@sraoss.co.jp>
>From 40fc14a0a15aa85bcaf863007e972c8fe6250fe2 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Tue, 1 Jul 2025 18:36:01 +0900
Subject: [PATCH v7 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.
---
src/backend/catalog/indexing.c | 24 ++++++++++++++++++++++++
src/backend/executor/execIndexing.c | 19 +++++++++++++++++++
src/include/executor/executor.h | 5 +++++
3 files changed, 48 insertions(+)
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 bdf862b2406..889573feb6b 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 104b059544d..2653e85e5ea 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -749,6 +749,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
--
2.43.0
>From 7c9146f221a253aa3a150f6b6d5600ed0225e4e1 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Thu, 5 Jun 2025 15:32:37 +0900
Subject: [PATCH v7 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 +-
2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0dcd6ee817e..712b7ded6d2 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3194,6 +3194,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,
@@ -3211,11 +3212,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:
@@ -4486,6 +4499,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,
@@ -4503,11 +4517,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
--
2.43.0
>From 8e5aa491ee6754d55001f568557454fcfd65eb41 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Mon, 31 Mar 2025 20:17:07 +0900
Subject: [PATCH v7 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 208f4bcc5b6f12ae95b2c2981786a441263b8a98 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
Date: Mon, 31 Mar 2025 18:46:58 +0900
Subject: [PATCH v7 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 | 40 ++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 5fdcf24d5f8..734508c7f58 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,47 @@ 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;
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, oldproc->oid, 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 +609,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
