On Fri, 27 Jun 2025 18:53:02 +0700
Daniil Davydov <3daniss...@gmail.com> wrote:
> Hi,
>
> On Thu, Jun 5, 2025 at 5:21 PM Yugo Nagata <nag...@sraoss.co.jp> wrote:
> >
> > I've attached updated patches.
> >
>
> I have some comments on v4-0001 patch :
Thank you for your comments!
> 1)
> heap_freetuple should be called for every tuple that we get from
> SearchSysCacheCopy3.
> But if tuple is valid after the first SearchSysCacheCopy3, we
> overwrite the old pointer (by the second SearchSysCacheCopy3 call) and
> forget to free it.
> I suggest adding heap_freetuple call before the second SearchSysCacheCopy3
> call.
Good catches. Fixed.
> 2)
> + Form_pg_proc oldproc = (Form_pg_proc) GETSTRUCT(oldtup);
> + Datum proargnames;
> + bool isnull;
> + const char *dropcmd;
> Strange alignment. I guess you should keep the same alignment as in
> deleted declarations.
Fixed.
I've attached patches including these fixes.
> 3)
> This patch fixes postgres behavior if I first create a function and
> then try to CREATE OR REPLACE it in concurrent transactions.
> But if the function doesn't exist and I try to call CREATE OR REPLACE
> in concurrent transactions, I will get an error.
> I wrote about it in this thread [1] and Tom Lane said that this
> behavior is kinda expected.
> Just in case, I decided to mention it here anyway - perhaps you will
> have other thoughts on this matter.
>
> [1]
> https://www.postgresql.org/message-id/flat/CAJDiXghv2JF5zbLyyybokWKM%2B-GYsTG%2Bhw7xseLNgJOJwf0%2B8w%40mail.gmail.com
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.
Regards,
Yugo Nagata
--
Yugo Nagata <nag...@sraoss.co.jp>
>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 v5 3/3] 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 v5 2/3] 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 v5 1/3] 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
