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.
> 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.
> Thanks for creating and working on this patch!
Thanks for reviewing it!
regards, tom lane
