On Thu, Jul 11, 2019 at 3:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > Attached updated patch. Thanks again.
>
> Pushed with a bit of further cleanup
Thanks a lot.
> --- most notably, the way
> you had execute_sql_string(), it was still leaking any cruft
> ProcessUtility might generate. We can fix that by running
> ProcessUtility in the per-statement context too.
Ah, I was thinking only about planning.
> I also dropped the optimization for a single/last statement in
> execute_sql_string(), and simplified it to just always create
> and delete a child context. This was based on a couple of
> thoughts. The norm in this code path is that there's multiple
> statements, probably lots of them, so that the percentage savings
> from getting rid of one context creation is likely negligible.
> Also, unlike exec_simple_query, we *don't* know that the outer
> context is due to be cleared right afterwards. Since
> execute_sql_string() can run multiple times in one extension
> command, in principle we could get bloat from not cleaning up
> after the last command of each string. Admittedly, it's not
> likely that you'd have so many strings involved that that
> amounts to a lot, but between following upgrade-script chains
> and cascaded module loads, there could be more than a couple.
> So it seems like the argument for saving a context creation is
> much weaker here than in exec_simple_query.
Agreed.
> I tried to improve the comments too. I noticed that the bit about
> "(again, these must outlive the execution context)" seemed to be
> a dangling reference --- whatever previous comment it was referring
> to is not to be found anymore. So I made that self-contained.
Thanks.
Regards,
Amit
