Re: Using Expanded Objects other than Arrays from plpgsql

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 PM 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 are 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=false
plpgsql_statement_tree_walker_impl() would walk silent, and with
dump_debug=false 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 == 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