On Thu, 2 Jul 2020 at 03:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Khandekar <amitdkhan.pg@gmail.com> writes:
> > There are a couple of function call overheads I observed in pl/pgsql
> > code : exec_stmt() and exec_cast_value(). Removing these overheads
> > resulted in some performance gains.
>
> I took a look at the 0001/0002 patches (not 0003 as yet). I do not
> like 0001 too much. The most concrete problem with it is that
> you broke translatability of the error messages, cf the first
> translatability guideline at [1].
Yeah, I thought we can safely use %s for proper nouns such as "trigger
procedure" or "function" as those would not be translated. But looks
like even if they won't be translated, the difference in word order
among languages might create problems with this.
> While that could be fixed by passing
> the entire message not just part of it, I don't see anything that we're
> gaining by moving that stuff into exec_toplevel_block in the first place.
> Certainly, passing a string that describes what will happen *after*
> exec_toplevel_block is just weird. I think what you've got here is
> a very arbitrary chopping-up of the existing code based on chance
> similarities of the existing callers. I think we're better off to make
> exec_toplevel_block be as nearly as possible a match for exec_stmts'
> semantics.
I thought some of those things that I kept in exec_toplevel_block() do
look like they belong to a top level function. But what you are saying
also makes sense : better to keep it similar to exec_stmts.
>
> Hence, I propose the attached 0001 to replace 0001/0002. This should
> be basically indistinguishable performance-wise, though I have not
> tried to benchmark.
Thanks for the patches. Yeah, performance-wise it does look similar;
but anyways I tried running, and got similar performance numbers.
> Note that for reviewability's sake, I did not
> reindent the former body of exec_stmt, though we'd want to do that
> before committing.
Right.
>
> Also, 0002 is a small patch on top of that to avoid redundant saves
> and restores of estate->err_stmt within the loop in exec_stmts. This
> may well not be a measurable improvement, but it's a pretty obvious
> inefficiency in exec_stmts now that it's refactored this way.
0002 also makes sense.