On Fri, 23 May 2025 10:37:42 +0800
jian he <jian.universal...@gmail.com> wrote:
> On Thu, May 22, 2025 at 10:25 AM jian he <jian.universal...@gmail.com> wrote:
> >
> hi.
> earlier, i didn't check patch 0002.
>
> i think in AlterFunction add
> /* Lock the function so nobody else can do anything with it. */
> LockDatabaseObject(ProcedureRelationId, funcOid, 0, AccessExclusiveLock);
>
> right after
> funcOid = LookupFuncWithArgs(stmt->objtype, stmt->func, false);
>
> should be fine.
Thank you. That makes sense because we can reduce redundant call of
SearchSysCacheCopy1
and HeapTupleIsValid. I've attached a updated patch.
Regards,
Yugo Nagata
--
Yugo Nagata <nag...@sraoss.co.jp>
>From b5b79dffb96760b47632cc9d325a034f5cc020e9 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 v2 2/2] Prevent internal error at concurrent ALTER FUNCTION
---
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 b9fd7683abb..a41eec9d48e 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"
@@ -1380,9 +1381,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 094fd8d212e01634f74afcac8a9b8b0d26201813 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 v2 1/2] Prevent internal error at concurrent CREATE OR REPLACE
FUNCTION
---
src/backend/catalog/pg_proc.c | 38 +++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index fe0490259e9..3a15374b261 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -35,6 +35,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"
@@ -355,24 +356,45 @@ 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)));
+
+ /* 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);
@@ -557,7 +579,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
