Re: [PATCH] Hooks at XactCommand level

Le 14/07/2021 à 21:26, Tom Lane a écrit :
> Gilles Darold <gilles(at)darold(dot)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. Myapologies 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 areenough 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/