Thread: Prevent internal error at concurrent CREATE OR REPLACE FUNCTION

Hi,

I found that multiple sessions concurrently execute CREATE OR REPLACE FUNCTION
for a same function, the error "tuple concurrently updated" is raised. This is
an internal error output by elog, also the message is not user-friendly.

I've attached a patch to prevent this internal error by locking an exclusive
lock before the command and get the read tuple after acquiring the lock.
Also, if the function has been removed during the lock waiting, the new entry
is created.

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>

On Mon, 31 Mar 2025 20:00:57 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> Hi,
> 
> I found that multiple sessions concurrently execute CREATE OR REPLACE FUNCTION
> for a same function, the error "tuple concurrently updated" is raised. This is
> an internal error output by elog, also the message is not user-friendly.
> 
> I've attached a patch to prevent this internal error by locking an exclusive
> lock before the command and get the read tuple after acquiring the lock.
> Also, if the function has been removed during the lock waiting, the new entry
> is created.

I also found the same error is raised when concurrent ALTER FUNCTION commands are
executed. I've added a patch to fix this in the similar way.

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>

Hi,


On Thu, 22 May 2025 10:25:58 +0800
jian he <jian.universality@gmail.com> wrote:

Thank you for looking into it.

> + /* 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));
> 
> we already called LockDatabaseObject, concurrent DDL can
> not do DROP FUNCTION or ALTER FUNCTION.
> so no need to call SearchSysCacheCopy3 again?

The function may be dropped *before* we call LockDatabaseObject.
SearchSysCacheCopy3 is called for check this. 
Plese see AlterPublication() as a similar code example.

> 
> @@ -553,11 +575,13 @@ ProcedureCreate(const char *procedureName,
>   replaces[Anum_pg_proc_proowner - 1] = false;
>   replaces[Anum_pg_proc_proacl - 1] = false;
> 
> +
> +
>   /* Okay, do it... */
> no need to add these two new lines.

I'll remove the lines. Thanks.

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>

On Fri, 23 May 2025 10:37:42 +0800
jian he <jian.universality@gmail.com> wrote:

> On Thu, May 22, 2025 at 10:25 AM jian he <jian.universality@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 <nagata@sraoss.co.jp>

On Tue, May 27, 2025 at 1:35 AM Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> > + /* 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));
> >
> > we already called LockDatabaseObject, concurrent DDL can
> > not do DROP FUNCTION or ALTER FUNCTION.
> > so no need to call SearchSysCacheCopy3 again?
>
> The function may be dropped *before* we call LockDatabaseObject.
> SearchSysCacheCopy3 is called for check this.
> Plese see AlterPublication() as a similar code example.
>

I am wondering, can we do it the following way for v2-0001?


    /* Check for pre-existing definition */
    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);
        if (!replace)
            ereport(ERROR,
                    (errcode(ERRCODE_DUPLICATE_FUNCTION),
                     errmsg("function \"%s\" already exists with same
argument types",
                            procedureName)));
        /*
         * 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.
         */
        if (!ConditionalLockDatabaseObject(ProcedureRelationId,
oldproc->oid, 0, AccessExclusiveLock))
            oldtup = SearchSysCacheCopy3(PROCNAMEARGSNSP,
                                        PointerGetDatum(procedureName),
                                        PointerGetDatum(parameterTypes),
                                        ObjectIdGetDatum(procNamespace));
    }