[PATH] Idle in transaction cancellation

Andres Freund <andres@anarazel.de> wrote:

Here is a proposed patch which enables cancellation of $subject.

Cool. Some enhancements we'd like to do to Serializable Snapshot
Isolation (SSI), should the base patch make it in, would require
this capability.

Currently it does *not* report any special error message to the
client if it

starts sending commands in an (unbekownst to it) failed
transaction, but just the normal "25P02: current transaction is
aborted..." message.

It shouldn't be hard to add that and I will propose a patch if
people would like it (I personally am not very interested, but I
can see people validly wanting it)

For SSI purposes, it would be highly desirable to be able to set the
SQLSTATE and message generated when the canceled transaction
terminates.

but I would like to have some feedback on the patch first.

I'll take a look when I can, but it may be a couple weeks from now.

-Kevin

On Tue, Oct 19, 2010 at 10:18 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

Andres Freund <andres@anarazel.de> wrote:

Here is a proposed patch which enables cancellation of $subject.

I'll take a look when I can, but it may be a couple weeks from now.

How about adding it to
https://commitfest.postgresql.org/action/commitfest_view/open ?

Seems like a good feature, at least from 20,000 feet.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On Tuesday 19 October 2010 16:18:29 Kevin Grittner wrote:

Andres Freund <andres@anarazel.de> wrote:

Here is a proposed patch which enables cancellation of $subject.

Cool. Some enhancements we'd like to do to Serializable Snapshot
Isolation (SSI), should the base patch make it in, would require
this capability.

Currently it does *not* report any special error message to the
client if it

starts sending commands in an (unbekownst to it) failed
transaction, but just the normal "25P02: current transaction is
aborted..." message.

It shouldn't be hard to add that and I will propose a patch if
people would like it (I personally am not very interested, but I
can see people validly wanting it)

For SSI purposes, it would be highly desirable to be able to set the
SQLSTATE and message generated when the canceled transaction
terminates.

Ok, I implemented that capability, but the patch feels somewhat wrong to me,
so its a separate patch on top the others:

* it intermingles logic between elog.c and postgres.c far too much - requiring
exporting variables which should not get exported. And it would still need
more to allow some sensible Assert()s. I.e. Assert(DoingCommandRead) would
need to be available in elog.c to avoid somebody using the re-throwing
capability out of place....

* You wont see an error if the next command after the IDLE in transaction is a
COMMIT/ROLLBACK. I don’t see any sensible way around that.

* the copied edata lives in TopMemoryContext and gets only reset in the next
reported error, after the re-raised error was reported.

That’s how it looks like to raise one such error right now:

error = ERROR
if (DoingCommandRead)
{
silent_error_while_idle = true;
error |= LOG_NO_CLIENT|LOG_RE_THROW_AFTER_SYNC;
}

...

ereport(error,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to user request")));

One could mingle together LOG_NO_CLIENT|LOG_RE_THROW_AFTER_SYNC into a macro
and set silent_error_while_idle in elog.c, but I don’t see many callsites
coming up, so I think its better to be explicit.

Ill set this up for the next commitfest, I don't think I can do much more
without further input.

Andres

Andres Freund <andres@anarazel.de> wrote:

* You wont see an error if the next command after the IDLE in
transaction is a COMMIT/ROLLBACK. I don*t see any sensible way
around that.

Well, on a ROLLBACK I'm not sure it's a problem. On a COMMIT,
couldn't you call a function to check for it in CommitTransaction
and PrepareTransaction?

-Kevin

On Tuesday 02 November 2010 18:33:15 Kevin Grittner wrote:

Andres Freund <andres@anarazel.de> wrote:

* You wont see an error if the next command after the IDLE in
transaction is a COMMIT/ROLLBACK. I don*t see any sensible way
around that.

Well, on a ROLLBACK I'm not sure it's a problem. On a COMMIT,
couldn't you call a function to check for it in CommitTransaction
and PrepareTransaction?

Sure, throwing an error somewhere wouldnt be that hard. But at the moment a
COMMIT is always successfull (and just reporting a ROLLBACK in a failed txn).
Doesn't seem to be something to changed lightly.

Does anybody have any idea why COMMIT is allowed there? Seems pretty strange
to me.

Andres

Excerpts from Andres Freund's message of mar nov 02 18:36:19 -0300 2010:

On Tuesday 02 November 2010 18:33:15 Kevin Grittner wrote:

Andres Freund <andres@anarazel.de> wrote:

* You wont see an error if the next command after the IDLE in
transaction is a COMMIT/ROLLBACK. I don*t see any sensible way
around that.

Well, on a ROLLBACK I'm not sure it's a problem. On a COMMIT,
couldn't you call a function to check for it in CommitTransaction
and PrepareTransaction?

Sure, throwing an error somewhere wouldnt be that hard. But at the moment a
COMMIT is always successfull (and just reporting a ROLLBACK in a failed txn).
Doesn't seem to be something to changed lightly.

If the user calls COMMIT, it calls EndTransactionBlock, which ends up
calling AbortTransaction. Can't you do it at that point? (Says he who
hasn't looked at the patch at all)

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Andres Freund <andres@anarazel.de> wrote:

On Tuesday 02 November 2010 18:33:15 Kevin Grittner wrote:

Well, on a ROLLBACK I'm not sure it's a problem. On a COMMIT,
couldn't you call a function to check for it in CommitTransaction
and PrepareTransaction?

Sure, throwing an error somewhere wouldnt be that hard. But at the
moment a COMMIT is always successfull (and just reporting a
ROLLBACK in a failed txn).
Doesn't seem to be something to changed lightly.

Well, I'm looking at using this with the Serializable Snapshot
Isolation (SSI) patch, which can throw an error from a COMMIT, if
the commit completes the conditions necessary for an anomaly to
occur (i.e., the committing transaction is on the rw-conflict *out*
side of a pivot, and it is the first in the set to commit). If we
succeed in enhancing SSI to use lists of rw-conflicted transactions
rather than its current techniques, we might be able to (and find it
desirable to) always commit in that circumstance and roll back some
*other* transaction which is part of the problem. Of course, that
other transaction might be idle at the time, and the next thing *it*
tries to execute *might* be a COMMIT.

So if the SSI patch goes in, there will always be some chance that a
COMMIT can fail, but doing it through the mechanism your patch
provides could improve performance, because we could guarantee that
nobody ever has a serialization failure without first successfully
committing a transaction which wrote to one or more tables. If a
commit fails due to SSI, it is highly desirable that the error use
the serialization failure SQLSTATE, so that an application framework
can know that it is reasonable to retry the transaction.

Does anybody have any idea why COMMIT is allowed there? Seems
pretty strange to me.

So that the "failed transaction" state can be cleared. The
transaction as a whole has failed, but you don't want the connection
to become useless.

-Kevin

On Tuesday 02 November 2010 22:59:15 Kevin Grittner wrote:

Does anybody have any idea why COMMIT is allowed there? Seems
pretty strange to me.

So that the "failed transaction" state can be cleared. The
transaction as a whole has failed, but you don't want the connection
to become useless.

Well, allowing ROLLBACK is enought there, isnt it?

Andres Freund <andres@anarazel.de> writes:

On Tuesday 02 November 2010 22:59:15 Kevin Grittner wrote:

Does anybody have any idea why COMMIT is allowed there? Seems
pretty strange to me.

So that the "failed transaction" state can be cleared. The
transaction as a whole has failed, but you don't want the connection
to become useless.

Well, allowing ROLLBACK is enought there, isnt it?

The client has no reason to think the transaction has failed, so what
it's going to send is COMMIT, not ROLLBACK. From the point of view of
the client, this should look like its COMMIT failed (or in general its
next command failed); not like there was some magic action-at-a-distance
on the state of its transaction.

Now you could argue that if you send COMMIT, and that fails, you should
have to send ROLLBACK to get to an idle state ... but that's not how
this ever worked before, and I don't think it's what the SQL standard
expects either. After a COMMIT, you're out of the transaction either
way.

regards, tom lane

Andres Freund <andres@anarazel.de> wrote:

Ok, I implemented that capability

I applied all three patches with minor offsets, and it builds, but
several regression tests fail. I backed out the patches in reverse
order and confirmed that while the regression tests pass without
any of these patches, they fail with just the first, the first and
the second, or all three patches.

If you're not seeing the same thing there, I'll be happy to provide
the details.

-Kevin

I wrote:

I applied all three patches with minor offsets, and it builds, but
several regression tests fail.

Sorry, after sending that I realized I hadn't done a make distclean.
After that it passes. Please ignore the previous post.

-Kevin

Andres Freund <andres@anarazel.de> wrote:

On Tuesday 19 October 2010 16:18:29 Kevin Grittner wrote:

For SSI purposes, it would be highly desirable to be able to set
the SQLSTATE and message generated when the canceled transaction
terminates.

Ok, I implemented that capability, but the patch feels somewhat
wrong to me,

so its a separate patch on top the others:

The patch is in context diff format, applies with minor offsets,
compiles and passes regression tests.

I have to admit that after reading the patch, I think I previously
misunderstood the scope of it. Am I correct in reading that the
main thrust of this is to improve error handling on standbys? Is
there any provision for one backend to cause a *different* backend
which is idle in a transaction to terminate cleanly when it attempts
to process its next statement? (That is what I was hoping to find,
for use in the SSI patch.)

Anyway, if the third patch file is only there because of my request,
I think it might be best to focus on the first two as a solution for
the standby issues this was originally meant to address, and then to
look at an API for the usage I have in mind after that is settled.

-Kevin

On Thursday 02 December 2010 00:48:53 Kevin Grittner wrote:

Andres Freund <andres@anarazel.de> wrote:

On Tuesday 19 October 2010 16:18:29 Kevin Grittner wrote:

For SSI purposes, it would be highly desirable to be able to set
the SQLSTATE and message generated when the canceled transaction
terminates.

Ok, I implemented that capability, but the patch feels somewhat
wrong to me,

so its a separate patch on top the others:

The patch is in context diff format, applies with minor offsets,
compiles and passes regression tests.

I have to admit that after reading the patch, I think I previously
misunderstood the scope of it. Am I correct in reading that the
main thrust of this is to improve error handling on standbys? Is
there any provision for one backend to cause a *different* backend
which is idle in a transaction to terminate cleanly when it attempts
to process its next statement? (That is what I was hoping to find,
for use in the SSI patch.)

Do you wan't to terminate it immediately or on next statement?

You might want to check out SendProcSignal() et al.

Anyway, if the third patch file is only there because of my request,
I think it might be best to focus on the first two as a solution for
the standby issues this was originally meant to address, and then to
look at an API for the usage I have in mind after that is settled.

Besides that I dont like the implementation very much, I think its generally a
good idea...

Andres Freund <andres@anarazel.de> wrote:

Do you wan't to terminate it immediately or on next statement?

I want to have one backend terminate the transaction on another
backend as soon as practicable. If a query is active, it would be
best if it was canceled. It appears that if it is "idle in
transaction" there is a need to wait for the next request. It would
be a big plus for the backend requesting the cancellation to be able
to specify the SQLSTATE, message, etc., used by the other backend on
failure.

You might want to check out SendProcSignal() et al.

Will take a look.

Besides that I dont like the implementation very much, I think its
generally a good idea...

OK. While browsing around, I'll try to think of an alternative
approach, but this is new territory for me -- I've been learning
about areas in the code at need so far....

-Kevin

Andres Freund <andres@anarazel.de> wrote:

On Thursday 02 December 2010 00:48:53 Kevin Grittner wrote:

Is there any provision for one backend to cause a *different*
backend which is idle in a transaction to terminate cleanly when
it attempts to process its next statement?

You might want to check out SendProcSignal() et al.

Yeah, that was the missing link for me. Thanks!

Anyway, if the third patch file is only there because of my
request, I think it might be best to focus on the first two as a
solution for the standby issues this was originally meant to
address, and then to look at an API for the usage I have in mind
after that is settled.

Besides that I dont like the implementation very much, I think its
generally a good idea...

Is it sane to leave the implementation of this for the specific
areas which need it (like SSI), or do you think a generalized API
for it is needed?

I'll look at it more closely tonight, but at first scan it appears
that just reserving one flag for PROCSIG_SERIALIZATION_FAILURE (or
PROCSIG_SSI_CANCELLATION?) would allow me to code up the desired
behavior in a function called from procsignal_sigusr1_handler. I
can arrange for passing any needed detail through the SSI-controlled
structures somehow. Would that allow you to skip the parts you
didn't like?

It looks as though this is something which could easily be split off
as a separate patch within the SSI effort.

-Kevin

Excerpts from Andres Freund's message of sáb oct 30 05:49:21 -0300 2010:

Ill set this up for the next commitfest, I don't think I can do much more
without further input.

Are you reserving about 20 bits for levels, and 12 for flags? Given the
relatively scarce growth of levels, I think we could live with about 6
or 7 bits for level, rest for flags.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thursday 02 December 2010 22:21:37 Alvaro Herrera wrote:

Excerpts from Andres Freund's message of sáb oct 30 05:49:21 -0300 2010:

Ill set this up for the next commitfest, I don't think I can do much
more without further input.

Are you reserving about 20 bits for levels, and 12 for flags? Given the
relatively scarce growth of levels, I think we could live with about 6
or 7 bits for level, rest for flags.

The number I picked was absolutely arbitrary I admit. Neither did I think it
would be likely to see more levels, nor did I forsee many flags, so I just
chose some number I liked in that certain moment ;-)

Andres

"Kevin Grittner" wrote:

Andres Freund wrote:

On Thursday 02 December 2010 00:48:53 Kevin Grittner wrote:

Is there any provision for one backend to cause a *different*
backend which is idle in a transaction to terminate cleanly when
it attempts to process its next statement?

You might want to check out SendProcSignal() et al.

Yeah, that was the missing link for me. Thanks!

OK, I have confirmed that the patch set Andres posted compiles and
passes regression tests (both `make check` and `make
installcheck-world`), and that the API provided works for at least
one purpose -- allowing SSI conflict detection to cancel an a
transaction other than the one on which the failure is detected, to
allow a guarantee that there will be no rollbacks without first
having a successful commit of a transaction which wrote data.

I created a "skip" branch in my git repo to hold the work for this
test. A single commit there which includes all three of Andres's
patches plus the code needed to implement the SSI feature is at:

http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=
commitdiff;h=0e1e86830b49dab51ff80619a580527260f4e186

I now wish I'd committed Andres's patches separately first. If
anyone likes, I could try to put something together to separate
things out like that.

I did notice one picky thing while working on this -- the left brace
for some if statements wasn't placed consistently with surrounding
code. Rather than:

if (x)
{
a();
b();
}

the patch tends to do this:

if (x){
a();
b();
}

I agree with Andres that the API to use it is a little bit spread out
and non-obvious, but I don't see a nicer way to do it. Maybe someone
else can think of something.

I didn't test the patch in relation to the original purpose (for
which only the first two of the three patch files should be needed)
-- error handling with hot standby. I'm hoping that someone familiar
with the issues there can test that part of it.

-Kevin

On Sat, Oct 30, 2010 at 4:49 AM, Andres Freund <andres@anarazel.de> wrote:

Here is a proposed patch which enables cancellation of $subject.

Disclaimer: This isn't my area of expertise, so take the below with a
grain or seven of salt.

It sort of looks to me like the LOG_NO_CLIENT error flag and the
silent_error_while_idle flag are trying to cooperate to get the effect
of throwing an error without actually throwing an error. I'm
wondering if it would be at all sensible to do that more directly by
making ProcessInterrupts() call AbortCurrentTransaction() in this
case.

Assuming that works at all, it would presumably mean that the client
would thereafter get something like this:

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

...which might be thought unhelpful. But that could be fixed either
by modifying errdetail_abort() or perhaps even by abstracting the
"current transaction is aborted, commands..." message into a function
that could produce an entirely different message if on either the
first or all calls within a given transaction.

I'm not sure if this would work, or if it's better. I'm just throwing
it out there, because the current approach looks a little grotty to
me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company