Re: [PATCH] Hooks at XactCommand level

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. 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.

<digression>

> The objective of this new callback is to be able to call user-defined
> code before any new statement is executed. For example it can call a
> rollback to savepoint if there was an error in the previous transaction
> statement, which allow to implements Rollback at Statement Level at
> server side using a PostgreSQL extension, see [1] .

Urgh. Isn't this re-making the same mistake we made years ago, namely
trying to implement autocommit on the server side? I fear this will
be a disaster even larger than that was, because if it's an extension
then pretty much no applications will be prepared for the new semantics.
I strongly urge you to read the discussions that led up to f85f43dfb,
and to search the commit history before that for mentions of
"autocommit", to see just how extensive the mess was.

I spent a little time trying to locate said discussions; it's harder
than it should be because we didn't have the practice of citing email
threads in the commit log at the time. I did find

https://www.postgresql.org/message-id/flat/Pine.LNX.4.44.0303172059170.1975-100000%40peter.localdomain#7ae26ed5c1bfbf9b22a420dfd8b8e69f

which seems to have been the proximate decision, and here are
a few threads talking about all the messes that were created
for JDBC etc:

https://www.postgresql.org/message-id/flat/3D793A93.7030000%40xythos.com#4a2e2d9bdf2967906a6e0a75815d6636
https://www.postgresql.org/message-id/flat/3383060E-272E-11D7-BA14-000502E740BA%40wellsgaming.com
https://www.postgresql.org/message-id/flat/Law14-F37PIje6n0ssr00000bc1%40hotmail.com

Basically, changing transactional semantics underneath clients is
a horrid idea. Having such behavior in an extension rather than
the core doesn't make it less horrid. If we'd designed it to act
that way from day one, maybe it'd have been fine. But as things
stand, we are quite locked into the position that this has to be
managed on the client side.

</digression>

That point doesn't necessarily invalidate the value of having
some sort of hook in this general area. But I would kind of like
to see another use-case, because I don't believe in this one.

regards, tom lane