Gilles Darold <gil...@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. 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
