On Fri, 27 Jun 2025 18:53:02 +0700
Daniil Davydov <3danissimo@gmail.com> wrote:
> Hi,
>
> On Thu, Jun 5, 2025 at 5:21 PM Yugo Nagata <nagata@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 <nagata@sraoss.co.jp>
