Le 14/07/2021 à 21:26, Tom Lane a écrit :
Gilles Darold <gilles@darold.net> writes:
I have renamed the patch and the title of this proposal registered in
the commitfest "Xact/SubXact event callback at command start" to reflect
the last changes that do not include new hooks anymore.
Hmm, it doesn't seem like this has addressed my concern at all.
The callbacks are still effectively defined as "run during
start_xact_command", so they're not any less squishy semantically
than they were before. The point of my criticism was that you
should move the call site to someplace that's more organically
connected to execution of commands.
Another thing I'm not too pleased with in this formulation is that it's
very unclear what the distinction is between XACT_EVENT_COMMAND_START
and SUBXACT_EVENT_COMMAND_START. AFAICS, *every* potential use-case
for this would have to hook into both callback chains, and most likely
would treat the two events alike.
Please find in attachment the new version v2 of the patch, I hope this time I have well understood your advices. My apologies for this waste of time.
I have moved the call to the callback out of start_xact_command() and limit his call into exec_simple_query() and c_parse_exemessage(). There is other call to start_xact_command() elsewhere but actually these two places are enough for what I'm doing with the extensions. I have updated the extension test cases to check the behavior when autocommit is on or off, error in execute of prepared statement and error in update where current of cursor. But there is certainly a case that I have missed.
Other calls of start_xact_command() are in exec_bind_message(), exec_execute_message(), exec_describe_statement_message(), exec_describe_portal_message and PostgresMain. In my test they are either not called or generates duplicates calls to the callback with exec_simple_query() and c_parse_exemessage().
Also CallXactStartCommand() will only use one event XACT_EVENT_COMMAND_START and only do a single call:
CallXactCallbacks(XACT_EVENT_COMMAND_START);
Plus, as you note, the goalposts have suddenly been moved for the
amount of overhead it's okay to have in an XactCallback or SubXactCallback
function. So that might cause problems for unrelated code. It's probably
better to not try to re-use that infrastructure.
About this maybe I was not clear in my bench, the overhead is not introduced by the patch on the callback, there is no overhead. But by the rollback at statement level extension. In case this was clear but you think that we must not reuse this callback infrastructure do you mean that I should fallback to a hook?
Best regard,
--
Gilles Darold
http://www.darold.net/
