Thread: [PATCH] Hooks at XactCommand level

Hi,

Based on a PoC reported in a previous thread [1] I'd like to propose new
hooks around transaction commands. The objective of this patch is to
allow PostgreSQL extension to act at start and end (including abort) of
a SQL statement in a transaction.

The idea for these hooks is born from the no go given to Takayuki
Tsunakawa's patch[2] proposing an in core implementation of
statement-level rollback transaction and the pg_statement_rollback
extension[3] that we have developed at LzLabs. The extension
pg_statement_rollback has two limitation, the first one is that the
client still have to call the ROLLBACK TO SAVEPOINT when an error is
encountered and the second is that it generates a crash when PostgreSQL
is compiled with assert that can not be fixed at the extension level.

Although that I have not though about other uses for these hooks, they
will allow a full server side statement-level rollback feature like in
commercial DBMSs like DB2 and Oracle. This feature is very often
requested by users that want to migrate to PostgreSQL.


SPECIFICATION
==================================================


There is no additional syntax or GUC, the patch just adds three new hooks:


* start_xact_command_hook called at end of the start_xact_command()
function.
* finish_xact_command called in finish_xact_command() just before
CommitTransactionCommand().
* abort_current_transaction_hook called after an error is encountered at
end of AbortCurrentTransaction().

These hooks allow an external plugins to execute code related to the SQL
statements executed in a transaction.


DESIGN
==================================================


Nothing more to add here.


CONSIDERATIONS AND REQUESTS
==================================================


An extension using these hooks that implements the server side rollback
at statement level feature is attached to demonstrate the interest of
these hooks. If we want to support this feature the extension could be
added under the contrib/ directory.

Here is an example of use of these hooks through the
pg_statement_rollbackv2 extension:

    LOAD 'pg_statement_rollbackv2.so';
    LOAD
    SET pg_statement_rollback.enabled TO on;
    SET
    CREATE SCHEMA testrsl;
    CREATE SCHEMA
    SET search_path TO testrsl,public;
    SET
    BEGIN;
    BEGIN
    CREATE TABLE tbl_rsl(id integer, val varchar(256));
    CREATE TABLE
    INSERT INTO tbl_rsl VALUES (1, 'one');
    INSERT 0 1
    WITH write AS (INSERT INTO tbl_rsl VALUES (2, 'two') RETURNING id,
val) SELECT * FROM write;
     id | val
    ----+-----
      2 | two
    (1 row)

    UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1; -- >>>>> will fail
    psql:simple.sql:14: ERROR:  invalid input syntax for type integer: "two"
    LINE 1: UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1;
                    ^
    SELECT * FROM tbl_rsl; -- Should show records id 1 + 2
     id | val
    ----+-----
      1 | one
      2 | two
    (2 rows)

    COMMIT;
    COMMIT

As you can see the failing UPDATE statement has been rolled back and we
recover the state of the transaction just before the statement without
any client savepoint and rollback to savepoint action.


I'll add this patch to Commitfest 2021-01.


Best regards


[1]
https://www.postgresql-archive.org/Issue-with-server-side-statement-level-rollback-td6162387.html
[2]
https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F6A9286%40G01JPEXMBYT05
[3] https://github.com/darold/pg_statement_rollbackv2

--
Gilles Darold
http://www.darold.net/

Le 12/03/2021 à 06:55, Julien Rouhaud a écrit :
> Hi,
>
> On Tue, Dec 08, 2020 at 11:15:12AM +0100, Gilles Darold wrote:
>> Based on a PoC reported in a previous thread [1] I'd like to propose new
>> hooks around transaction commands. The objective of this patch is to
>> allow PostgreSQL extension to act at start and end (including abort) of
>> a SQL statement in a transaction.
>>
>> The idea for these hooks is born from the no go given to Takayuki
>> Tsunakawa's patch[2] proposing an in core implementation of
>> statement-level rollback transaction and the pg_statement_rollback
>> extension[3] that we have developed at LzLabs. The extension
>> pg_statement_rollback has two limitation, the first one is that the
>> client still have to call the ROLLBACK TO SAVEPOINT when an error is
>> encountered and the second is that it generates a crash when PostgreSQL
>> is compiled with assert that can not be fixed at the extension level.
> This topic came up quite often on the mailing list, the last being from Álvaro
> at [1].  I think there's a general agreement that customers want that feature,
> won't stop asking for it, and many if not all forks ended up implementing it.
>
> I would still prefer if he had a way to support if in vanilla postgres, with of
> course all possible safeguards to avoid an epic fiasco.


I have added Alvarro and Takayuki to the thread, this patch is inspired
from their proposals. I wrote this patch after reading the thread and
concluding that a core implementation doesn't seems to make the
consensus and that this feature could be available to users through an
extension.


> I personally think that Álvaro's previous approach, giving the ability to
> specify the rollback behavior in the TransactionStmt grammar, would be enough
> (I mean without the GUC part) to cover realistic and sensible usecases, which
> is where the client fully decides whether there's a statement level rollback or
> not.  One could probably build a custom module on top of that to introduce some
> kind of GUC to change the behavior more globally if it wants to take that risk.


Yes probably, with this patch I just want to propose an external
implementation of the feature. The extension implementation "just"
require these three hooks to propose the same feature as if it was
implemented in vanilla postgres. The feature can be simply enabled or
disabled by a custom user defined variable before a transaction is
started or globaly for all transaction.


> If such an approach is still not wanted for core inclusion, then I'm in favor
> of adding those hooks.  There's already a published extension that tries to
> implement that (for complete fairness I'm one of the people to blame), but as
> Gilles previously mentioned this is very hackish and the currently available
> hooks makes it very hard if not impossible to have a perfect implementation.
> It's clear that people will never stop to try doing it, so at least let's make
> it possible using a custom module.
>
> It's also probably worthwhile to mention that the custom extension implementing
> server side statement level rollback wasn't implemented because it wasn't
> doable in the client side, but because the client side implementation was
> causing a really big overhead due to the need of sending the extra commands,
> and putting it on the server side lead to really significant performance
> improvement.

Right, the closer extension to reach this feature is the extension we
develop at LzLabs [2] but it still require a rollback to savepoint at
client side in case of error. The extension [3] using these hooks
doesn't have this limitation, everything is handled server side.


>> Although that I have not though about other uses for these hooks, they
>> will allow a full server side statement-level rollback feature like in
>> commercial DBMSs like DB2 and Oracle. This feature is very often
>> requested by users that want to migrate to PostgreSQL.
> I also thought about it, and I don't really see other possible usage for those
> hooks.


Yes I have not a lot of imagination too for possible other use for these
hooks but I hope that in itself this feature can justify them. I just
though that if we expose the query_string at command_start hook we could
allow its modification by external modules, but this is surely the worst
idea I can produce.


>> There is no additional syntax or GUC, the patch just adds three new hooks:
>>
>>
>> * start_xact_command_hook called at end of the start_xact_command()
>> function.
>> * finish_xact_command called in finish_xact_command() just before
>> CommitTransactionCommand().
>> * abort_current_transaction_hook called after an error is encountered at
>> end of AbortCurrentTransaction().
>>
>> These hooks allow an external plugins to execute code related to the SQL
>> statements executed in a transaction.
> The only comment I have for those hooks is for the
> abort_current_transaction_hook.  AbortCurrentTransaction() can be called
> recursively, so should the hook provide some more information about the
> CurrentTransactionState, like the blockState, or is
> GetCurrentTransactionNestLevel() enough to act only for the wanted calls?


I don't think we need to pass any information at least for the rollback
at statement level extension. All information needed are accessible and
actually at abort_current_transaction_hook we only toggle a boolean to
fire the rollback.


I have rebased the patch.


Thanks for the review.


[1]
https://www.postgresql.org/message-id/20181207192006.rf4tkfl25oc6pqmv@alvherre.pgsql

[2] https://github.com/lzlabs/pg_statement_rollback/

[3] https://github.com/darold/pg_statement_rollbackv2


--
Gilles Darold
LzLabs GmbH
http://www.lzlabs.com/

On Fri, Mar 19, 2021 at 11:02:29PM +0100, Gilles Darold wrote:
> Le 12/03/2021 à 06:55, Julien Rouhaud a écrit :
> >
> 
> I don't think we need to pass any information at least for the rollback
> at statement level extension. All information needed are accessible and
> actually at abort_current_transaction_hook we only toggle a boolean to
> fire the rollback.

That's what I thought but I wanted to be sure.

So, I have nothing more to say about the patch itself.  At that point, I guess
that we can't keep postponing that topic, and we should either:

- commit this patch, or Álvaro's one based on a new grammar keyword for BEGIN
  (maybe without the GUC if that's the only hard blocker), assuming that there
  aren't any technical issue with those

- reject this patch, and I guess set in stone that vanilla postgres will
  never allow that.

Given the situation I'm not sure if I should mark the patch as Ready for
Committer or not.  I'll leave it as-is for now as Álvaro is already in Cc.

> I have rebased the patch.

Thanks!

On Sat, Mar 20, 2021 at 06:33:24PM +0800, Julien Rouhaud wrote:
> 
> So, I have nothing more to say about the patch itself.  At that point, I guess
> that we can't keep postponing that topic, and we should either:
> 
> - commit this patch, or Álvaro's one based on a new grammar keyword for BEGIN
>   (maybe without the GUC if that's the only hard blocker), assuming that there
>   aren't any technical issue with those
> 
> - reject this patch, and I guess set in stone that vanilla postgres will
>   never allow that.
> 
> Given the situation I'm not sure if I should mark the patch as Ready for
> Committer or not.  I'll leave it as-is for now as Álvaro is already in Cc.

I just switched the patch to Ready for Committer.

Hello,

As far as I am concerned, I am totally awaiting for this kind of feature exposed here, for one single reason at this
time: the extension pg_statement_rollback will be much more valuable with the ability of processing "rollback to
savepoint"without the need for explicit instruction from client side (and this patch is giving this option).
 
The way the improvement is suggested here seems to be clever enough to allow many interesting behaviours from
differentskinds of extensions.
 

Thank you,

Nicolas CHAHWEKILIAN <leptitstagiaire@gmail.com> writes:
> As far as I am concerned, I am totally awaiting for this kind of feature
> exposed here, for one single reason at this time : the extension
> pg_statement_rollback will be much more valuable with the ability of
> processing "rollback to savepoint" without the need for explicit
> instruction from client side (and this patch is giving this option).

What exactly do these hooks do that isn't done as well or better
by the RegisterXactCallback and RegisterSubXactCallback mechanisms?
Perhaps we need to define some additional event types for those?
Or pass more data to the callback functions?

I quite dislike inventing a hook that's defined as "run during
start_xact_command", because there is basically nothing that's
not ad-hoc about that function: it's internal to postgres.c
and both its responsibilities and its call sites have changed
over time.  I think anyone hooking into that will be displeased
by the stability of their results.

BTW, per the cfbot the patch doesn't even apply right now.

            regards, tom lane

Nicolas CHAHWEKILIAN <leptitstagiaire@gmail.com> writes:

As far as I am concerned, I am totally awaiting for this kind of feature
exposed here, for one single reason at this time : the extension
pg_statement_rollback will be much more valuable with the ability of
processing "rollback to savepoint" without the need for explicit
instruction from client side (and this patch is giving this option).

What exactly do these hooks do that isn't done as well or better
by the RegisterXactCallback and RegisterSubXactCallback mechanisms?
Perhaps we need to define some additional event types for those?
Or pass more data to the callback functions?

Sorry it take me time to recall the reason of the hooks. Actually the problem is that the callbacks are not called when a statement is executed after an error so that we fall back to error:

    ERROR:  current transaction is aborted, commands ignored until end of transaction block

For example with the rollback at statement level extension:

BEGIN;
UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1; -- >>>>> will fail
LOG:  statement: UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1;
ERROR:  invalid input syntax for type integer: "two"
LINE 1: UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1;
                                ^
UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1; -- >>>>> will fail again
LOG:  statement: UPDATE tbl_rsl SET id = 'two', val = 2 WHERE id = 1;
ERROR:  current transaction is aborted, commands ignored until end of transaction block
SELECT * FROM tbl_rsl; -- Should show records id 1 + 2
LOG:  statement: SELECT * FROM tbl_rsl;
ERROR:  current transaction is aborted, commands ignored until end of transaction block

With the exention and the hook on start_xact_command() we can continue and execute all the following statements.

I have updated the patch to only keep the hook on start_xact_command(), as you've suggested the other hooks can be replaced by the use of the xact callback. The extension has also been updated for testing the feature, available here https://github.com/darold/pg_statement_rollbackv2

I quite dislike inventing a hook that's defined as "run during
start_xact_command", because there is basically nothing that's
not ad-hoc about that function: it's internal to postgres.c
and both its responsibilities and its call sites have changed
over time.  I think anyone hooking into that will be displeased
by the stability of their results.

Unfortunately I had not found a better solution, but I just tried with placing the hook in function BeginCommand() in src/backend/tcop/dest.c and the extension is working as espected. Do you think it would be a better place?In this case I can update the patch. For this feature we need a hook that is executed before any command even if the transaction is in abort state to be able to inject the rollback to savepoint, maybe I'm not looking at the right place to do that.

Thanks

-- 
Gilles Darold
http://www.darold.net/

Le 01/07/2021 à 18:47, Tom Lane a écrit :
> Nicolas CHAHWEKILIAN <leptitstagiaire@gmail.com> writes:
>> As far as I am concerned, I am totally awaiting for this kind of feature
>> exposed here, for one single reason at this time : the extension
>> pg_statement_rollback will be much more valuable with the ability of
>> processing "rollback to savepoint" without the need for explicit
>> instruction from client side (and this patch is giving this option).
> What exactly do these hooks do that isn't done as well or better
> by the RegisterXactCallback and RegisterSubXactCallback mechanisms?
> Perhaps we need to define some additional event types for those?
> Or pass more data to the callback functions?
>
> I quite dislike inventing a hook that's defined as "run during
> start_xact_command", because there is basically nothing that's
> not ad-hoc about that function: it's internal to postgres.c
> and both its responsibilities and its call sites have changed
> over time.  I think anyone hooking into that will be displeased
> by the stability of their results.


Sorry Tom, it seems that I have totally misinterpreted your comments,
google translate was not a great help for my understanding but Julien
was. Thanks Julien.


I'm joining a new patch v4 that removes the need of any hook and adds a
new events XACT_EVENT_COMMAND_START and SUBXACT_EVENT_COMMAND_START that
can be cautch in the xact callbacks when a new command is to be executed.


--
Gilles Darold
http://www.darold.net/

Hi,


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.


Here is the new description corresponding to the current patch.


This patch allow to execute user-defined code for the start of any
command through a xact registered callback. It introduce two new events
in XactEvent and SubXactEvent enum called respectively
XACT_EVENT_COMMAND_START and SUBXACT_EVENT_COMMAND_START. The callback
is not called if a transaction is not started.


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


The patch compile and regressions tests with assert enabled passed
successfully.

There is no regression test for this feature but extension at [1] has
been used for validation of the new callback.


The patch adds insignificant overhead by looking at an existing callback
definition but clearly it is the responsibility to the developer to
evaluate the performances impact of its user-defined code for this
callback as it will be called before each statement. Here is a very
simple test using pgbench -c 20 -j 8 -T 30

     tps = 669.930274 (without user-defined code)
     tps = 640.718361 (with user-defined code from extension [1])

the overhead for this extension is around 4.5% which I think is not so
bad good for such feature (internally it adds calls to RELEASE +
SAVEPOINT before each write statement execution and in case of error a
ROLLBACK TO savepoint).


[1] https://github.com/darold/pg_statement_rollbackv2

--
Gilles Darold
http://www.darold.net/

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

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.

I would like to move it closer to the command execution but the only place I see would be in BeginCommand() which actually is waiting for code to execute, for the moment this function does nothing. I don't see another possible place after start_xact_command() call. All my attempt to inject the callback after start_xact_command() result in a failure. If you see an other place I will be please to give it a test.

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.

Actually XACT_EVENT_COMMAND_START occurs only after the call BEGIN, when a transaction starts, whereas SUBXACT_EVENT_COMMAND_START occurs in all subsequent statement execution of this transaction. This helps to perform different actions following the event. In the example extension only SUBXACT_EVENT_COMMAND_START is used but for example I could use event XACT_EVENT_COMMAND_START to not send a RELEASE savepoint as there is none. I detect this case differently but this could be an improvement in the extension.

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

Yes I have suffered of this implementation for server side autocommit, it was reverted in PG 7.4 if I remember well. I'm old enough to remember that :-). I'm also against restoring this feature inside PG core but the fact that the subject comes again almost every 2 years mean that there is a need on this feature. This is why I'm proposing to be able to use an extension for those who really need the feature, with all the associated warning.

For example in my case the first time I was needing this feature was to emulate the behavior of DB2 that allows rollback at statement level. This is not exactly autocommit because the transaction still need to be validated or rolledback at end, this is just that an error will not invalidate the full transaction but just the failing statement. I think that this is different. Actually I have an extension that is doing that for most of the work but we still have to send the ROLLBACK TO savepoint at client side which is really a performances killer and especially painful to implement with JDBC Exception blocks.

Recently I was working on an Oracle to PostgreSQL migration and want to implement an other Oracle feature like that is heavily used when importing data from different sources into a data warehouse. It's very common in the Oracle world to batch data import inside a transaction and log silently the errors into a dedicated table to be processed later. "Whatever" (this concern only certain errors) happens you continue to import the data and DBAs will check what to fix and will re-import the records in error. Again, I have an extension that is doing that but we still have to generate the ROLLBACK TO at client side. This can be avoided with this proposal and will greatly simplify the code at client side.

We all know the problems of such server side implementation but once you have implemented it at client side and you are looking for better performances it's obvious that this kind of extension could help. The other solution is to move to a proprietary PostgreSQL fork which is surely not what we want.

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

I have sited two use-case, they are both based on the rollback at statement level feature. I'm pretty sure that there is several other use-cases that escape my poor imagination. IMHO the possibility to offer the rollback at statement level feature through an extension should be enough but if anyone have other use-case I will be pleased to create an extension to test it :-)

-- 
Gilles Darold
http://www.darold.net/

Le 15/07/2021 à 09:44, Gilles Darold a écrit :
> 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.
>
> I would like to move it closer to the command execution but the only
> place I see would be in BeginCommand() which actually is waiting for
> code to execute, for the moment this function does nothing. I don't
> see another possible place after start_xact_command() call. All my
> attempt to inject the callback after start_xact_command() result in a
> failure. If you see an other place I will be please to give it a test.
>

Looks like I have not well understood again, maybe you want me to move
the callback just after the start_xact_command() so that it is not
"hidden" in the "run during
start_xact_command". Ok, I will do that.


--
Gilles Darold
http://www.darold.net/

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/

Gilles Darold <gilles@darold.net> writes:
> [ 00001-startcommand_xact_callback-v2.diff ]

I've not read this version of the patch, but I see from the cfbot's
results that it's broken postgres_fdw.  I recall that postgres_fdw
uses the XactCallback and SubXactCallback mechanisms, so I'm betting
this means that you've changed the semantics of those callbacks in
an incompatible way.  That's probably not a great idea.  We could
fix postgres_fdw, but there are more than likely some external
modules that would also get broken, and that is supposed to be a
reasonably stable API.

            regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> On 2021-07-30 13:58:51 -0400, Tom Lane wrote:
>> I've not read this version of the patch, but I see from the cfbot's
>> results that it's broken postgres_fdw.

> I think this may partially be an issue with the way that postgres_fdw
> uses the callback than with the patch. It disconnects from the server
> *regardless* of the XactEvent passed to the callback. That makes it
> really hard to extend the callback mechanism to further events...

Perhaps.  Nonetheless, I thought upthread that adding these events
as Xact/SubXactCallback events was the wrong design, and I still
think that.  A new hook would be a more sensible way.

> I'm *very* unconvinced it makes sense to implement a feature like this
> in an extension / that we should expose API for that purpose. To me the
> top-level transaction state is way too tied to our internals for it to
> be reasonably dealt with in an extension.

Yeah, that's the other major problem --- the use-case doesn't seem
very convincing.  I'm not even sold on the goal, let alone on trying
to implement it by hooking into these particular places.  I think
that'll end up being buggy and fragile as well as not very performant.

            regards, tom lane

I'm *very* unconvinced it makes sense to implement a feature like this
in an extension / that we should expose API for that purpose. To me the
top-level transaction state is way too tied to our internals for it to
be reasonably dealt with in an extension.

Yeah, that's the other major problem --- the use-case doesn't seem
very convincing.  I'm not even sold on the goal, let alone on trying
to implement it by hooking into these particular places.  I think
that'll end up being buggy and fragile as well as not very performant.

I'm more favorable than you on the overall goal. Migrations to PG are a
frequent and good thing and as discussed before, lots of PG forks ended
up implementing a version of this. Clearly there's demand.

Sorry for the response delay. I have though about adding this odd hook to be able to implement this feature through an extension because I don't think this is something that should be implemented in core. There were also patches proposals which were all rejected.

We usually implement the feature at client side which is imo enough for the use cases. But the problem is that this a catastrophe in term of performances. I have done a small benchmark to illustrate the problem. This is a single process client on the same host than the PG backend.

For 10,000 tuples inserted with 50% of failures and rollback at statement level handled at client side:

        Expected: 5001, Count:  5001
        DML insert took: 13 wallclock secs ( 0.53 usr +  0.94 sys =  1.47 CPU)

Now with statement at rollback level handled at server side using the hook and the extension:

        Expected: 5001, Count:  5001
        DML insert took:  4 wallclock secs ( 0.27 usr +  0.32 sys =  0.59 CPU)

And with 100,000 tuples this is worst. Without the extension:

        Expected: 50001, Count:  50001
        DML insert took: 1796 wallclock secs (14.95 usr + 20.29 sys = 35.24 CPU)

with server side Rollback at statement level:

        Expected: 50001, Count:  50001
        DML insert took: 372 wallclock secs ( 4.85 usr +  5.45 sys = 10.30 CPU)

I think this is not so uncommon use cases and that could shows the interest of such extension.

However, I think a proper implementation would require a substantial
amount of effort. Including things like optimizing the subtransaction
logic so that switching the feature on doesn't lead to xid wraparound
issues. Adding odd hooks doesn't move us towards a real solution imo.

I would like to help on this part but unfortunately I have no idea on how we can improve that.

Best regards,

-- 
Gilles Darold

Le 30/07/2021 à 23:49, Tom Lane a écrit :
> Andres Freund <andres@anarazel.de> writes:
>> On 2021-07-30 13:58:51 -0400, Tom Lane wrote:
>>> I've not read this version of the patch, but I see from the cfbot's
>>> results that it's broken postgres_fdw.
>> I think this may partially be an issue with the way that postgres_fdw
>> uses the callback than with the patch. It disconnects from the server
>> *regardless* of the XactEvent passed to the callback. That makes it
>> really hard to extend the callback mechanism to further events...
> Perhaps.  Nonetheless, I thought upthread that adding these events
> as Xact/SubXactCallback events was the wrong design, and I still
> think that.  A new hook would be a more sensible way.
>
>> I'm *very* unconvinced it makes sense to implement a feature like this
>> in an extension / that we should expose API for that purpose. To me the
>> top-level transaction state is way too tied to our internals for it to
>> be reasonably dealt with in an extension.
> Yeah, that's the other major problem --- the use-case doesn't seem
> very convincing.  I'm not even sold on the goal, let alone on trying
> to implement it by hooking into these particular places.  I think
> that'll end up being buggy and fragile as well as not very performant.


I've attached the new version v5 of the patch that use a hook instead of
the use of a xact callback. Compared to the first implementation calls
to the hook have been extracted from the start_xact_command() function.
The test extension have also be updated.


If I understand well the last discussions there is no chance of having
this hook included. If there is no contrary opinion I will withdraw the
patch from the commitfest. However thank you so much to have taken time
to review this proposal.


Best regards,

--
Gilles Darold

Le 13/08/2021 à 11:58, Andres Freund a écrit :
> Hi,
>
> On 2021-08-10 10:12:26 +0200, Gilles Darold wrote:
>> Sorry for the response delay. I have though about adding this odd hook to be
>> able to implement this feature through an extension because I don't think
>> this is something that should be implemented in core. There were also
>> patches proposals which were all rejected.
>>
>> We usually implement the feature at client side which is imo enough for the
>> use cases. But the problem is that this a catastrophe in term of
>> performances. I have done a small benchmark to illustrate the problem. This
>> is a single process client on the same host than the PG backend.
>>
>> For 10,000 tuples inserted with 50% of failures and rollback at statement
>> level handled at client side:
>>
>>         Expected: 5001, Count:  5001
>>         DML insert took: 13 wallclock secs ( 0.53 usr +  0.94 sys =  1.47
>> CPU)
> Something seems off here. This suggests every insert took 2.6ms. That
> seems awfully long, unless your network latency is substantial. I did a
> quick test implementing this in the naive-most way in pgbench, and I get
> better times - and there's *lots* of room for improvement.
>
> I used a pgbench script that sent the following:
> BEGIN;
> SAVEPOINT insert_fail;
> INSERT INTO testinsert(data) VALUES (1);
> ROLLBACK TO SAVEPOINT insert_fail;
> SAVEPOINT insert_success;
> INSERT INTO testinsert(data) VALUES (1);
> RELEASE SAVEPOINT insert_success;
> {repeat 5 times}
> COMMIT;
>
> I.e. 5 failing and 5 succeeding insertions wrapped in one transaction. I
> get >2500 tps, i.e. > 25k rows/sec. And it's not hard to optimize that
> further - the {ROLLBACK TO,RELEASE} SAVEPOINT; SAVEPOINT; INSERT can be
> sent in one roundtrip. That gets me to somewhere around 40k rows/sec.
>
>
> BEGIN;
>
> \startpipeline
> SAVEPOINT insert_fail;
> INSERT INTO testinsert(data) VALUES (1);
> \endpipeline
>
> \startpipeline
> ROLLBACK TO SAVEPOINT insert_fail;
> SAVEPOINT insert_success;
> INSERT INTO testinsert(data) VALUES (1);
> \endpipeline
>
> \startpipeline
> RELEASE SAVEPOINT insert_success;
> SAVEPOINT insert_fail;
> INSERT INTO testinsert(data) VALUES (1);
> \endpipeline
>
> \startpipeline
> ROLLBACK TO SAVEPOINT insert_fail;
> SAVEPOINT insert_success;
> INSERT INTO testinsert(data) VALUES (1);
> \endpipeline
>
> {repeat last two blocks three times}
> COMMIT;
>
> Greetings,
>
> Andres Freund


I have written a Perl script to mimic what I have found in an Oracle
batch script to import data in a table. I had this use case in a recent
migration the only difference is that the batch was written in Java.


$dbh->do("BEGIN") or die "FATAL: " . $dbh->errstr . "\n";
my $start = new Benchmark;
my $sth = $dbh->prepare("INSERT INTO t1 VALUES (?, ?)");
exit 1 if (not defined $sth);
for (my $i = 0; $i <= 10000; $i++)
{
        $dbh->do("SAVEPOINT foo") or die "FATAL: " . $dbh->errstr . "\n";
        # Generate a duplicate key each two row inserted
        my $val = $i;
        $val = $i-1 if ($i % 2 != 0);
        unless ($sth->execute($val, 'insert '.$i)) {
                $dbh->do("ROLLBACK TO foo") or die "FATAL: " .
$dbh->errstr . "\n";
        } else {
                $dbh->do("RELEASE foo") or die "FATAL: " . $dbh->errstr
. "\n";
        }
}
$sth->finish();
my $end = new Benchmark;

$dbh->do("COMMIT;");

my $td = timediff($end, $start);
print "DML insert took: " . timestr($td) . "\n";


The timing reported are from my personal computer, there is no network
latency, it uses localhost. Anyway, the objective was not to bench the
DML throughput but the overhead of the rollback at statement level made
at client side versus server side. I guess that you might have the same
speed gain around x3 to x5 or more following the number of tuples?


The full script can be found here
https://github.com/darold/pg_statement_rollbackv2/blob/main/test/batch_script_example.pl


Cheers,

--
Gilles Darold

I have changed the status of this proposal as rejected.


To resume the final state of this proposal there is no consensus on the
interest to add a hook on start xact commands. Also the only useful case
for this hook was to be able to have a server side automatic rollback at
statement level. It can be regrettable because I don't think that
PostgreSQL will have such feature before a long time (that's probably
better) and a way to external implementation through an extension would
be helpful for migration from other RDBMS like DB2 or Oracle. The only
ways to have this feature is to handle the rollback at client side using
savepoint, which is at least 3 times slower than a server side
implementation, or not use such implementation at all. Outside not being
performant it doesn't scale due to txid wraparound. And the last way is
to use a proprietary forks of PostgreSQL, some are proposing this  feature.

--
Gilles Darold
http://www.darold.net/