Hi, Tom On Mon, 3 Feb 2025 at 07:02, Michel Pelletier <pelletier.mic...@gmail.com> wrote: > > On Sun, Feb 2, 2025 at 1:57 PM Tom Lane <t...@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
