MIME-Version: 1.0
References: <CACxu=vJaKFNsYxooSnW1wEgsAO5u_v1XYBacfVJ14wgJV_PYeg@mail.gmail.com>
 <1342498.1729444411@sss.pgh.pa.us> <CACxu=vLXvpzN4X3k+9jsMt6ujuOvFVUSkA80t_cROSsF4y2jQQ@mail.gmail.com>
 <1445998.1729482404@sss.pgh.pa.us> <CACxu=vKEF8Qa-OaADFxf0uMg-xw6gH_CNCWd2s+xaqh-gY4=xg@mail.gmail.com>
 <2062830.1729625620@sss.pgh.pa.us> <2265411.1729699470@sss.pgh.pa.us>
 <CACxu=v++HNmss59yGUDkRny7g=M8tZ2YXF07AUXqKVGqcSfxGQ@mail.gmail.com>
 <2354718.1729737539@sss.pgh.pa.us> <2581216.1729794746@sss.pgh.pa.us>
 <CACxu=v+dn37zr8gx5xNP-EZY3OLtGLTHrbx_ZkCQc40HpyMLKA@mail.gmail.com>
 <1948345.1730500073@sss.pgh.pa.us> <CACxu=vL7i_U_iSNpREe8eCAMEyKHuPpk9THRpBhk+ar0U1EdOw@mail.gmail.com>
 <CACxu=vKLc6f5N8_DR58LKkE1eohWSxTvThTeGsLm7p7QH1aFBA@mail.gmail.com>
 <CACxu=vJf2S=ysun_h=zmYNu6oUM47+egbpX5mMC0X9BJK=EQwQ@mail.gmail.com>
 <CACxu=vK+S6BXN8ZYyBvqQBWrcwHXqtue1-ZuKO3+XtHGBYcDUQ@mail.gmail.com>
 <CAFj8pRCd6xcH-AYEyHFdGdU89O9JjZ-v-pyQnOwd9zNJkCEdhQ@mail.gmail.com>
 <3797606.1732045516@sss.pgh.pa.us> <1417389.1736964543@sss.pgh.pa.us>
 <CACxu=vKModwqSpEjpn-ur1uMAEYkcd-i8CvNsH-z9=dk7F7YQw@mail.gmail.com>
 <3363452.1737483125@sss.pgh.pa.us> <0AC229FA-A3F1-43FD-B0DC-A46A73FEAFF7@yandex-team.ru>
 <931398.1737905825@sss.pgh.pa.us> <38A31221-C1C4-4846-9709-D66ACD76E87A@yandex-team.ru>
 <46876.1737918281@sss.pgh.pa.us> <CALT9ZEGy5+GNQUtR80BPL7k-Qi47sqwPLnN_dOaQUW5UbxcTGA@mail.gmail.com>
 <3682021.1738288421@sss.pgh.pa.us> <256915.1738533419@sss.pgh.pa.us> <CACxu=vJ250gdGF0HPzBEjCX+dDWVvum3P4Vuxg+B_VJ7x69h8A@mail.gmail.com>
In-Reply-To: <CACxu=vJ250gdGF0HPzBEjCX+dDWVvum3P4Vuxg+B_VJ7x69h8A@mail.gmail.com>
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Mon, 3 Feb 2025 15:42:04 +0400
Message-ID: <CALT9ZEE1+e2pn5F9qTrc0n5DqD++EzzoyoKG8aS2xA8_yXUxFA@mail.gmail.com>
Subject: Re: Using Expanded Objects other than Arrays from plpgsql
To: Michel Pelletier <pelletier.michel@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andrey Borodin <x4mmm@yandex-team.ru>, 
	Pavel Stehule <pavel.stehule@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALT9ZEE1%2Be2pn5F9qTrc0n5DqD%2B%2BEzzoyoKG8aS2xA8_yXUxFA%40mail.gmail.com>
Precedence: bulk

Hi, Tom

On Mon, 3 Feb 2025 at 07:02, Michel Pelletier
<pelletier.michel@gmail.com> wrote:
>
> On Sun, Feb 2, 2025 at 1:57=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> wrote=
:
>>
>> I wrote:
>> > Hmm, it seemed to still apply for me.  But anyway, I needed to make
>> > the other changes, so here's v4.
>>
>> PFA v5.  The new 0001 patch refactors the free_xxx infrastructure
>> to create plpgsql_statement_tree_walker(), and then in what's now
>> 0003 we can use that instead of writing a lot of duplicate code.
>
>
> Thanks Tom!  These patches apply for me and all my tests and benchmarks a=
re still good.

I've looked at the patches v4 and v5.
v5 logic of patch 0003 is much more understandable with refactoring
free_* functions to a walker. So I think it's much better than v4 even
regarding the principle have not changed.

Using support functions in 0005 complicates things. But I think it's
justified by extensibility and benchmarks demonstrated by Michel above
in the thread.

Overall patch to me looks well-structured, beneficial for performance
and extensibility and it looks good to me.

Minor notes on the patches:

If dump_* functions could use the newly added walker, the code would
look better. I suppose the main complication is that dump_* functions
contain a lot of per-statement prints/formatting. So maybe a way to
implement this is to put these statements into the existing tree
walker i.e. plpgsql_statement_tree_walker_impl() and add an argument
bool dump_debug into it. So in effect called with dump_debug=3Dfalse
plpgsql_statement_tree_walker_impl() would walk silent, and with
dump_debug=3Dfalse it would walk and print what is supposed to be
printed currently in dump_* functions. Maybe there are other problems
besides this?

For exec_check_rw_parameter():

I think renaming expr->expr_simple_expr to sexpr saves few bytes but
doesn't makes anything simpler, so if possible I'd prefer using just
expr->expr_simple_expr with necessary casts. Furtermore in this
function mostly we use cast results fexpr, opexpr and sbsref of
expr->expr_simple_expr that already has separate names.

Transferring target param as int paramid looks completely ok. But we
have unconditional checking Assert(paramid =3D=3D expr->target_param + 1),
so it looks as a redundant split as of now. Do we plan a true split
and removal of this assert in the future?

Thanks for creating and working on this patch!
Regards,
Pavel Borisov
Supabase