BUG #1118: Misleading Commit message

PostgreSQL Bugs List wrote:

Description: Misleading Commit message

Details:

In a block transaction, whether or not there were errors in the transaction
issuing a commit; returns a COMMIT confirmation.

In a block transaction, after a statement fails, all other statements have
the (frustratingly) nice message ERROR: current transaction is aborted,
commands ignored until end of transaction block

It seems to me, it would be helpful to change the commit message to reflect
a ROLLBACK if the transaction is not actually committed.

Let me know if you want an example :-)

PS: the bug tracking thingo thinks my email (elein@varlena.com) is not a
valid address.
Funny I get a whole lotta postgres mail there...

Uh, the tag indicates the COMMIT completed, not that it was a success.
If we throw an error on a COMMIT, people willl think we did not close
the transacction, and if we return a ROLLBACK, they will think they
issued a rollback. I don't think we can easily change the current
behavior.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

PostgreSQL Bugs List wrote:

In a block transaction, whether or not there were errors in the transaction
issuing a commit; returns a COMMIT confirmation.

Uh, the tag indicates the COMMIT completed, not that it was a success.

The current philosophy on command tags is "the tag is the same as the
command actually issued". However we are talking about breaking that
rule for EXECUTE, and if we do that, it's hard to say that we should
continue to enforce the rule for COMMIT. It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

If we throw an error on a COMMIT, people willl think we did not close
the transacction,

... which we wouldn't have. That won't work.

and if we return a ROLLBACK, they will think they issued a rollback.

Which, in effect, is what they did. Is it likely that this would break
any clients? The intention of the current design rule is that clients
can match the tag against the command they issued, but I don't know of
any client code that actually does that.

In any case, we already have some inconsistencies:

regression=# begin;
BEGIN
regression=# end;
COMMIT
regression=# begin;
BEGIN
regression=# abort;
ROLLBACK
regression=#

so it seems that in some cases we're already following a rule more like
"the tag is the same as the command actually *executed*".

I started out not wanting to make this change either, but the more
I think about it the harder it is to hold that position.

regards, tom lane

On Sun, Mar 28, 2004 at 10:23:26AM -0500, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

PostgreSQL Bugs List wrote:

In a block transaction, whether or not there were errors in the transaction
issuing a commit; returns a COMMIT confirmation.

Uh, the tag indicates the COMMIT completed, not that it was a success.

The current philosophy on command tags is "the tag is the same as the
command actually issued". However we are talking about breaking that
rule for EXECUTE, and if we do that, it's hard to say that we should
continue to enforce the rule for COMMIT. It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

If we throw an error on a COMMIT, people willl think we did not close
the transacction,

... which we wouldn't have. That won't work.

and if we return a ROLLBACK, they will think they issued a rollback.

Which, in effect, is what they did. Is it likely that this would break
any clients? The intention of the current design rule is that clients
can match the tag against the command they issued, but I don't know of
any client code that actually does that.

In any case, we already have some inconsistencies:

regression=# begin;
BEGIN
regression=# end;
COMMIT
regression=# begin;
BEGIN
regression=# abort;
ROLLBACK
regression=#

so it seems that in some cases we're already following a rule more like
"the tag is the same as the command actually *executed*".

I started out not wanting to make this change either, but the more
I think about it the harder it is to hold that position.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

The message could be something like:
COMMIT: Transaction rolled back due to errors

That way, it would reflect both the command and the action.
But I am concerned about the information rather than
the exact message if someone has better ideas.

My reason for submitting the bug was as Tom stated:

It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

A commit that fails does not commit. It rolls back.

In general, this would make it friendlier for new people and
space cadets that don't notice the last statement failed :-)

Elein
elein@varlena.com

Do we want to add this to TODO:

* Issue an extra message when COMMIT completes an failed transaction

---------------------------------------------------------------------------

elein wrote:

On Sun, Mar 28, 2004 at 10:23:26AM -0500, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

PostgreSQL Bugs List wrote:

In a block transaction, whether or not there were errors in the transaction
issuing a commit; returns a COMMIT confirmation.

Uh, the tag indicates the COMMIT completed, not that it was a success.

The current philosophy on command tags is "the tag is the same as the
command actually issued". However we are talking about breaking that
rule for EXECUTE, and if we do that, it's hard to say that we should
continue to enforce the rule for COMMIT. It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

If we throw an error on a COMMIT, people willl think we did not close
the transacction,

... which we wouldn't have. That won't work.

and if we return a ROLLBACK, they will think they issued a rollback.

Which, in effect, is what they did. Is it likely that this would break
any clients? The intention of the current design rule is that clients
can match the tag against the command they issued, but I don't know of
any client code that actually does that.

In any case, we already have some inconsistencies:

regression=# begin;
BEGIN
regression=# end;
COMMIT
regression=# begin;
BEGIN
regression=# abort;
ROLLBACK
regression=#

so it seems that in some cases we're already following a rule more like
"the tag is the same as the command actually *executed*".

I started out not wanting to make this change either, but the more
I think about it the harder it is to hold that position.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

The message could be something like:
COMMIT: Transaction rolled back due to errors

That way, it would reflect both the command and the action.
But I am concerned about the information rather than
the exact message if someone has better ideas.

My reason for submitting the bug was as Tom stated:

It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

A commit that fails does not commit. It rolls back.

In general, this would make it friendlier for new people and
space cadets that don't notice the last statement failed :-)

Elein
elein@varlena.com

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Do we want to add this to TODO:

* Issue an extra message when COMMIT completes a failed transaction

---------------------------------------------------------------------------

elein wrote:

On Sun, Mar 28, 2004 at 10:23:26AM -0500, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

PostgreSQL Bugs List wrote:

In a block transaction, whether or not there were errors in the transaction
issuing a commit; returns a COMMIT confirmation.

Uh, the tag indicates the COMMIT completed, not that it was a success.

The current philosophy on command tags is "the tag is the same as the
command actually issued". However we are talking about breaking that
rule for EXECUTE, and if we do that, it's hard to say that we should
continue to enforce the rule for COMMIT. It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

If we throw an error on a COMMIT, people willl think we did not close
the transacction,

... which we wouldn't have. That won't work.

and if we return a ROLLBACK, they will think they issued a rollback.

Which, in effect, is what they did. Is it likely that this would break
any clients? The intention of the current design rule is that clients
can match the tag against the command they issued, but I don't know of
any client code that actually does that.

In any case, we already have some inconsistencies:

regression=# begin;
BEGIN
regression=# end;
COMMIT
regression=# begin;
BEGIN
regression=# abort;
ROLLBACK
regression=#

so it seems that in some cases we're already following a rule more like
"the tag is the same as the command actually *executed*".

I started out not wanting to make this change either, but the more
I think about it the harder it is to hold that position.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

The message could be something like:
COMMIT: Transaction rolled back due to errors

That way, it would reflect both the command and the action.
But I am concerned about the information rather than
the exact message if someone has better ideas.

My reason for submitting the bug was as Tom stated:

It would clearly be useful
for a COMMIT that ends a failed transaction to report ROLLBACK instead.

A commit that fails does not commit. It rolls back.

In general, this would make it friendlier for new people and
space cadets that don't notice the last statement failed :-)

Elein
elein@varlena.com

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

While Alvarro, et al are messing with transaction syntax
this would be a good time to clarify this message.

--elein

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Do we want to add this to TODO:
* Issue an extra message when COMMIT completes a failed transaction

No --- it's (a) wordy and (b) not responsive to the original complaint,
which was that a client that examines command completion tags will be
easily misled. We should either change the command tags or do nothing.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Do we want to add this to TODO:
* Issue an extra message when COMMIT completes a failed transaction

No --- it's (a) wordy and (b) not responsive to the original complaint,
which was that a client that examines command completion tags will be
easily misled. We should either change the command tags or do nothing.

I am not excited about changing the command tag.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Do we want to add this to TODO:
* Issue an extra message when COMMIT completes a failed transaction

No --- it's (a) wordy and (b) not responsive to the original complaint,
which was that a client that examines command completion tags will be
easily misled. We should either change the command tags or do nothing.

I am not excited about changing the command tag.

I was not either to start with, but the more I think about it, the more
I think it would be a good idea.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Do we want to add this to TODO:
* Issue an extra message when COMMIT completes a failed transaction

No --- it's (a) wordy and (b) not responsive to the original complaint,
which was that a client that examines command completion tags will be
easily misled. We should either change the command tags or do nothing.

I am not excited about changing the command tag.

I was not either to start with, but the more I think about it, the more
I think it would be a good idea.

What tag would we use? ABORT?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I am not excited about changing the command tag.

I was not either to start with, but the more I think about it, the more
I think it would be a good idea.

What tag would we use? ABORT?

No, ROLLBACK, which is what you get when you give the "expected"
command.

regression=# begin;
BEGIN
regression=# select 1/0;
ERROR: division by zero
regression=# abort; -- or rollback;
ROLLBACK

regression=# begin;
BEGIN
regression=# select 1/0;
ERROR: division by zero
regression=# commit;
COMMIT

I think the above is fairly misleading; it would be better to say
ROLLBACK to indicate that we had in fact canceled the transaction.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Do we change tags in any other commands?

MOVE, FETCH, EXECUTE ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Do we change tags in any other commands?

MOVE, FETCH, EXECUTE ...

Ah, yes, I remember we changed EXECUTE recently to return the tag of
what we executed. How do we modify MOVE/FETCH tags? I can't remember.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

MOVE, FETCH, EXECUTE ...

Ah, yes, I remember we changed EXECUTE recently to return the tag of
what we executed. How do we modify MOVE/FETCH tags? I can't remember.

I was just looking to see what cases ProcessUtility allowed to change
the tag. I think that what the code does is just to append the row
count, which you could argue isn't "changing the tag". But really,
is returning "UPDATE 0" vs "UPDATE 1" any different conceptually from
what we are talking about here? It's still using the tag to pass back
info about what the command actually did.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

MOVE, FETCH, EXECUTE ...

Ah, yes, I remember we changed EXECUTE recently to return the tag of
what we executed. How do we modify MOVE/FETCH tags? I can't remember.

I was just looking to see what cases ProcessUtility allowed to change
the tag. I think that what the code does is just to append the row
count, which you could argue isn't "changing the tag". But really,
is returning "UPDATE 0" vs "UPDATE 1" any different conceptually from
what we are talking about here? It's still using the tag to pass back
info about what the command actually did.

Yes, the count is what I remember changing, and as I remember the goal
was to have us return MOVE 0 if you do MOVE 1 at the end of a cursor.
We already return counts for INSERT/UPDATE/DELETE, but the racial issue
with MOVE was that the count returned might not match the count
supplied.

Therefore, I don't see MOVE/FETCH as the same issue as ROLLBACK.
EXECUTE is closer, and I think new for 7.5, but the interesting part
there is that you should always get back something different from
EXECUTE, while with COMMIT it would change only when you have an aborted
transaction.

As I remember, the big issue was how often applications are looking and
comparing these tags to take actions. I think we should return ROLLBACK
on COMMIT failure and we can see if we get any problem reports during
beta.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

As I remember, the big issue was how often applications are looking and
comparing these tags to take actions. I think we should return ROLLBACK
on COMMIT failure and we can see if we get any problem reports during
beta.

Good enough; I'll make it happen.

regards, tom lane

FYI: I'm agreeing w/Tom who is agreeing with me.

The tag change should be good. I do hope people are
not relying on seeing COMMIT when the transaction
rolled back. It does not seem that in this case
they would.

elein

elein wrote:

FYI: I'm agreeing w/Tom who is agreeing with me.

The tag change should be good. I do hope people are
not relying on seeing COMMIT when the transaction
rolled back. It does not seem that in this case
they would.

If it is a problem, hopefully we will hear about it during beta. I will
mention is as a backward-compatibility issue in the release notes.

---------------------------------------------------------------------------

elein

On Sat, Jul 10, 2004 at 04:13:49PM -0400, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

As I remember, the big issue was how often applications are looking and
comparing these tags to take actions. I think we should return ROLLBACK
on COMMIT failure and we can see if we get any problem reports during
beta.

Good enough; I'll make it happen.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

elein wrote:

FYI: I'm agreeing w/Tom who is agreeing with me.

The tag change should be good. I do hope people are
not relying on seeing COMMIT when the transaction
rolled back. It does not seem that in this case
they would.

If it is a problem, hopefully we will hear about it during beta. I will
mention is as a backward-compatibility issue in the release notes.

The JDBC driver when talking the V2 protocol (unusual for a recent
server, but possible) looks for either COMMIT or ROLLBACK when looking
for end-of-transaction, so it should be fine.

-O