Re: Prevent internal error at concurrent CREATE OR REPLACE FUNCTION

On Fri, 27 Jun 2025 18:53:02 +0700
Daniil Davydov <3danissimo(at)gmail(dot)com> wrote:

> Hi,
>
> On Thu, Jun 5, 2025 at 5:21 PM Yugo Nagata <nagata(at)sraoss(dot)co(dot)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 <nagata(at)sraoss(dot)co(dot)jp>