Re: Error on failed COMMIT

On Sun, Feb 23, 2020 at 11:11 AM Shay Rojansky <roji@roji.org> wrote:
> I'd like to second Dave on this, from the .NET perspective - actual client access is done via standard drivers in almost all cases, and these drivers generally adhere to database API abstractions (JDBC for Java, ADO.NET for .NET, and so on). AFAIK, in almost all such abstractions, commit can either complete (implying success) or throw an exception - there is no third way to return a status code. It's true that a driver may expose NOTICE/WARNING messages via some other channel (Npgsql emits .NET events for these), but this is a separate message "channel" that is disconnected API-wise from the commit; this makes the mechanism very "undiscoverable".

I'm still befuddled here. First, to repeat what I said before, the
COMMIT only returns a ROLLBACK command tag if there's been a previous
ERROR. So, if you haven't ignored the prior ERROR, you should be fine.
Second, there's nothing to keep the driver itself from translating
ROLLBACK into an exception, if that's more convenient for some
particular driver. Let's go back to Bernhard's example upthred:

composeTransaction() {
    Connection con = getConnection(); // implicitly "begin"
    try {
       insertFrameworkLevelState(con);
       insertApplicationLevelState(con);
       con.commit();
       publishNewState();
    } catch (Throwable ex) {
       con.rollback();
    }
}

If insertFrameworkLevelState() or insertApplicationLevelState()
perform database operations that fail, then an exception should be
thrown and we should end up at con.rollback(), unless there is an
internal catch block inside those functions that swallows the
exception, or unless the JDBC driver ignores the error from the
server. If those things succeed, then COMMIT could still fail with an
ERROR but it shouldn't return ROLLBACK. But, for extra security,
con.commit() could be made to throw an exception if the command tag
returned by COMMIT is not COMMIT. It sounds like Dave doesn't want to
do that, but it would solve this problem without requiring a server
behavior change.

Actually, an even better idea might be to make the driver error out
when the transaction is known to be in a failed state when you enter
con.commit(). The server does return an indication after each command
as to whether the session is in a transaction and whether that
transaction is in a failed state. That's how the %x escape sequence
just added to the psql prompt works. So, suppose the JDBC driver
tracked that state like libpq does. insertFrameworkLevelState() or
insertApplicationLevelState() throws an exception, which is internally
swallowed. Then you reach con.commit(), and it says, nope, can't do
that, we're in a failed state, and so an exception is thrown. Then
when we reach con.rollback() we're still inside a transaction, it gets
rolled back, and everything works just as expected.

Or, alternatively, the JDBC driver could keep track of the fact that
it had thrown an exception ITSELF, without paying any attention to
what the server told it, and if it saw con.commit() after raising an
exception, it could raise another exception (or re-raise the same
one). That would also fix it.

> Asking drivers to do this at the client have the exact same breakage impact as the server change, since the user-visible behavior changes in the same way (the change is just shifted from server to driver). What's worse is that every driver now has to reimplement the same new logic, and we'd most probably end up with some drivers doing it in some languages, and others not doing it in others (so behavioral differences).

Well, it seems quite possible that there are drivers and applications
that don't have this issue; I've never had a problem with this
behavior, and I've been using PostgreSQL for something like two
decades, and I believe that the sketch above could be used to get the
desired behavior in current releases of PostgreSQL with no server code
change. If we did change the server behavior, it seems unlikely that
every driver would adjust their behavior to the new server behavior
all at once and that they would all get it right while also all
preserving backward compatibility with current releases in case a
newer driver is used with an older server. I don't think that's
likely. What would probably happen is that many drivers would ignore
the change, leaving applications to cope with the differences between
server versions, and some would change the driver behavior
categorically, breaking compatibility with older server versions, and
some would make mistakes in implementing support for the new behavior.
And maybe we would also find that the new behavior isn't ideal for
everybody any more than the current behavior is ideal for everybody.

I am really struggling to see why this is anything but a bug in the
JDBC driver. The problem is that the application doesn't know that the
transaction has failed, but the server has returned not one, not two,
but three indications of failure. First, it returned an error, which I
guess the JDBC driver turns into an exception - but it does not,
before throwing that exception, remember that the current transaction
is failed. Second, it will thereafter report that the transaction is
in a failed state, both immediately after the error and upon every
subsequent operation that does not get the server out of the
transaction. It sounds like the JDBC driver ignores this information.
Third, the attempt at COMMIT will return a ROLLBACK command tag, which
Dave said that the driver does ignore. That's a lot of stuff that the
driver could do but isn't doing. So what this boils down to, from my
perspective, is not that the driver behavior in the face of errors
can't be made correct with the existing semantics, but that the driver
would find it more convenient if PostgreSQL reported those errors in a
somewhat different way. I think that's a fair criticism, but I don't
think it's a sufficient reason to change the behavior.

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