Re: [PATCH] Hooks at XactCommand level

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/