Hi, Tom! On Mon, 3 Feb 2025 at 21:53, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Pavel Borisov <pashkin.e...@gmail.com> writes: > > 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? > > I'm not thrilled with that idea, mainly because it would add overhead > to the performance-relevant cases (mark and free) to benefit a rarely > used debugging feature. I'm content to leave the debug code out of > this for now --- it seems to me that it's serving a different master > and doesn't have to be unified with the other routines. Makes sense.
> > 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. > > Hmm, I thought it looked cleaner like this, but I agree beauty > is in the eye of the beholder. Anybody else have a preference? > > > 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? > > We've already fetched the target variable using the paramid (cf > plpgsql_param_eval_var_check), so I think checking that the > expression does match it seems like a useful sanity check. > Agreed, it shouldn't ever not match, but that's why that's just > an Assert. There are no problems with these. Regards, Pavel Borisov
