Re: Error on failed COMMIT

> 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. [...]
> I am really struggling to see why this is anything but a bug in the JDBC
driver

As Dave wrote, the problem here isn't with the driver, but with framework
or user-code which swallows the initial exception and allows code to
continue to the commit. Npgsql (and I'm sure the JDBC driver too) does
surface PostgreSQL errors as exceptions, and internally tracks the
transaction status provided in the CommandComplete message. That means
users have the ability - but not the obligation - to know about failed
transactions, and some frameworks or user coding patterns could lead to a
commit being done on a failed transaction.

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

This is the main point here IMHO, and I don't think it's a question of
convenience, or of behavior that should vary across drivers.

If we think the current *user-visible* behavior is problematic (commit on
failed transaction completes without throwing), then the only remaining
question is where this behavior should be fixed - at the server or at the
driver. As I wrote above, from the user's perspective it makes no
difference - the change would be identical (and just as breaking) either
way. So while drivers *could* implement the new behavior, what advantages
would that have over doing it at the server? Some disadvantages do seem
clear (repetition of the logic across each driver - leading to
inconsistency across drivers, changing semantics at the driver by turning a
non-error into an exception...).

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

If we are assuming that most user code is already written to avoid
committing on failed transactions (by tracking transaction state etc.),
then making this change at the server wouldn't affect those applications;
the only applications affected would be those that do commit on failed
transactions today, and it could be argued that those are likely to be
broken today (since drivers today don't really expose the rollback in an
accessible/discoverable way).

Shay

On Mon, Feb 24, 2020 at 3:31 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, Feb 23, 2020 at 11:11 AM Shay Rojansky <roji(at)roji(dot)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
>