Обсуждение: BUG #17504: psql --single-transaction -vON_ERROR_STOP=1 still commits after client-side error

The following bug has been logged on the website:

Bug reference:      17504
Logged by:          Christoph Berg
Email address:      christoph.berg@credativ.de
PostgreSQL version: 14.3
Operating system:   any
Description:

A customer is reporting the following script has not the intended effect:

Preparation:
psql -c 'create table bar (id int)'
psql -c 'insert into bar values(1)'
-- bar now contains "1"

Test script truncate-and-copy.sql:
truncate bar;
\copy bar from bar.txt
select * from bar;

psql --single-transaction -vON_ERROR_STOP=1 -Xf truncate-and-copy.sql
TRUNCATE TABLE
psql:truncate-and-copy.sql:2: error: bar.txt: No such file or directory

The last "select" is correctly not executed anymore, i.e. the script
execution is correcty aborted, but bar is now empty, i.e. `psql -1` did
commit even when the script was erroring out:
psql -c 'select * from bar'
 id 
────
(0 rows)

The expectation would be that the script is either fully committed, or not
at all.

The problem is not limited to \copy; \i has the same problem.
A workaround is to drop -1, and use an explicit transaction in the script.

Christoph

Re: PG Bug reporting form
> The problem is not limited to \copy; \i has the same problem.
> A workaround is to drop -1, and use an explicit transaction in the script.

\connect correctly aborts the whole operation.

I would suggest that all client-side errors should be handled as fatal
when both --single-transaction and -vON_ERROR_STOP=1 are in effect.

Christoph

At Mon, 30 May 2022 17:18:28 +0200, Christoph Berg <christoph.berg@credativ.de> wrote in 
> Re: PG Bug reporting form
> > The problem is not limited to \copy; \i has the same problem.
> > A workaround is to drop -1, and use an explicit transaction in the script.
> 
> \connect correctly aborts the whole operation.
> 
> I would suggest that all client-side errors should be handled as fatal
> when both --single-transaction and -vON_ERROR_STOP=1 are in effect.

The code looks like just a thinko that "COMMIT" works fine even if the
given commands have ended in failure. But actually it doesn't for
client-side failure.

diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index ddff903915..2261f78f81 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -426,7 +426,9 @@ main(int argc, char *argv[])
 
                if (options.single_txn)
                {
-                       if ((res = PSQLexec("COMMIT")) == NULL)
+                       res = PSQLexec(successResult == EXIT_SUCCESS ?
+                                                  "COMMIT" : "ROLLBACK");
+                       if (res == NULL)
                        {
                                if (pset.on_error_stop)
                                {

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, May 31, 2022 at 11:39:27AM +0900, Kyotaro Horiguchi wrote:
> The code looks like just a thinko that "COMMIT" works fine even if the
> given commands have ended in failure. But actually it doesn't for
> client-side failure.
>
> diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
> index ddff903915..2261f78f81 100644
> --- a/src/bin/psql/startup.c
> +++ b/src/bin/psql/startup.c
> @@ -426,7 +426,9 @@ main(int argc, char *argv[])
>
>                 if (options.single_txn)
>                 {
> -                       if ((res = PSQLexec("COMMIT")) == NULL)
> +                       res = PSQLexec(successResult == EXIT_SUCCESS ?
> +                                                  "COMMIT" : "ROLLBACK");
> +                       if (res == NULL)
>                         {
>                                 if (pset.on_error_stop)
>                                 {

Yeah, it seems a bit strange to commit the changes if an error happens
on the client side, and the docs are a bit blurry about that because
it has never been considered, I guess.  This would not happen with a
failure in the backend as COMMIT would just map to a ROLLBACK
automatically.

The change that you are sending would enforce this policy as Christoph
would like.  Some tests would be nice to check such behaviors, say in
001_basic.pl, but we also need to be careful when sending down queries
with psql expected to fail because of SIGPIPE (c757a3d, 6d41dd0).  The
docs need a refresh, they mention now that COMMIT is sent after the
last command but that would not be the case anymore with this patch if
there is a client-side error.
--
Michael

Thanks for the suggestions!

At Thu, 2 Jun 2022 14:39:41 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> The change that you are sending would enforce this policy as Christoph
> would like.  Some tests would be nice to check such behaviors, say in

Added a test.

> 001_basic.pl, but we also need to be careful when sending down queries
> with psql expected to fail because of SIGPIPE (c757a3d, 6d41dd0).  The

This does not need to send a byte after the client-side failure. The
connection is rather living after failure in this case.

> docs need a refresh, they mention now that COMMIT is sent after the
> last command but that would not be the case anymore with this patch if
> there is a client-side error.

Agreed. Does the following work?

 > It causes psql to issue a BEGIN command before the first such option
 > and a COMMIT command after the last one, thereby wrapping all the
 > commands into a single transaction.
+> If any of the commands fails, a ROLLBACK command is sent instead.
 > This ensures that either all the commands complete successfully, or
 > no changes are applied.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jun 03, 2022 at 05:56:03PM +0900, Kyotaro Horiguchi wrote:
> At Thu, 2 Jun 2022 14:39:41 +0900, Michael Paquier <michael@paquier.xyz> wrote in
>> 001_basic.pl, but we also need to be careful when sending down queries
>> with psql expected to fail because of SIGPIPE (c757a3d, 6d41dd0).  The
>
> This does not need to send a byte after the client-side failure. The
> connection is rather living after failure in this case.

Sure, but I don't think that we can reliably test the case where one
of the switches triggers a backend-side error, which is what my point
is about.  I have added a note about that, with a couple of extra
tests with -f, with both success and failure scenarios.  And we'd
better add -X to all the commands as well.  These are added only on
HEAD as the test file is rather new.

>> It causes psql to issue a BEGIN command before the first such option
>> and a COMMIT command after the last one, thereby wrapping all the
>> commands into a single transaction.
>> If any of the commands fails, a ROLLBACK command is sent instead.
>> This ensures that either all the commands complete successfully, or
>> no changes are applied.

Looks fine to me, so applied down to 10.
--
Michael

At Mon, 6 Jun 2022 11:15:44 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Jun 03, 2022 at 05:56:03PM +0900, Kyotaro Horiguchi wrote:
> > At Thu, 2 Jun 2022 14:39:41 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> >> 001_basic.pl, but we also need to be careful when sending down queries
> >> with psql expected to fail because of SIGPIPE (c757a3d, 6d41dd0).  The
> > 
> > This does not need to send a byte after the client-side failure. The
> > connection is rather living after failure in this case.
> 
> Sure, but I don't think that we can reliably test the case where one
> of the switches triggers a backend-side error, which is what my point
> is about.  I have added a note about that, with a couple of extra
> tests with -f, with both success and failure scenarios.  And we'd
> better add -X to all the commands as well.  These are added only on
> HEAD as the test file is rather new.

Mmm, sorry about my laziness, and thanks for improving the coverage.

> >> It causes psql to issue a BEGIN command before the first such option
> >> and a COMMIT command after the last one, thereby wrapping all the
> >> commands into a single transaction.
> >> If any of the commands fails, a ROLLBACK command is sent instead.
> >> This ensures that either all the commands complete successfully, or
> >> no changes are applied.
> 
> Looks fine to me, so applied down to 10.

(Yes!)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jun 6, 2022 at 2:11 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> > >> It causes psql to issue a BEGIN command before the first such option
> > >> and a COMMIT command after the last one, thereby wrapping all the
> > >> commands into a single transaction.
> > >> If any of the commands fails, a ROLLBACK command is sent instead.
> > >> This ensures that either all the commands complete successfully, or
> > >> no changes are applied.
> >
> > Looks fine to me, so applied down to 10.
>
> (Yes!)

I am slightly concerned that this behavior change could break
somebody's stuff when they upgrade to the next minor release. It does
not seem impossible that someone could be knowingly relying on the old
behavior.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Jun 06, 2022 at 01:05:46PM -0700, David G. Johnston wrote:
> We've done worse when dealing with obvious bugs contrary to the
> documentation and that prevent unsafe behavior.  People relying on the
> defined behavior to safely fail when encountering unexpected failure
> producing situations that hopefully never happen get the priority over
> poorly written code that somehow relies on errors not respecting the single
> transaction promise.

Yeah, I think that the past behavior was a bit crazy.  On a
client-side error, psql reports a failure with an error code but would
commit any changes that happened before the command that failed with
the underlying commit created by --single-transaction, while
discarding any future commands.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Jun 03, 2022 at 05:56:03PM +0900, Kyotaro Horiguchi wrote:
>>> It causes psql to issue a BEGIN command before the first such option
>>> and a COMMIT command after the last one, thereby wrapping all the
>>> commands into a single transaction.
>>> If any of the commands fails, a ROLLBACK command is sent instead.
>>> This ensures that either all the commands complete successfully, or
>>> no changes are applied.

> Looks fine to me, so applied down to 10.

While preparing release notes, I had to study this commit to figure
out what it actually did, and I do not think it is right yet.  It
definitely does not do what is claimed above, ie rollback if "any"
of the steps fails.  As coded, it rolls back only if the *last*
step fails.  Now, if you've got ON_ERROR_STOP set, then the loop
terminates after an error so there's no difference ... but if you
do not, I think this behavior is not what's expected.

ISTM that an appropriate fix would be to remember if any command
failed, not just the last one.

            regards, tom lane

I wrote:
> ISTM that an appropriate fix would be to remember if any command
> failed, not just the last one.

Alternatively, one could argue that once we've decided to issue
ROLLBACK, there's not really any reason to continue performing
additional -c/-f actions, so that a sufficient fix would be to
get rid of the loop's ON_ERROR_STOP conditionality:

-            if (successResult != EXIT_SUCCESS && pset.on_error_stop)
+            if (successResult != EXIT_SUCCESS)
                 break;

But I'm not sure that that argument is bulletproof.  If we are
considering a client-side failure then the server may still think
the transaction is fine, in which case we should continue to
perform actions that could have client-side side effects; or
at least, not doing so is a potentially significant behavioral
change.

In any case, I now agree with Robert's upthread objection that
this should not have been back-patched.  It's a nontrivial
behavioral change and it's not clear that it's 100% without
downsides.  I particularly do not want to ship 14.4 with this,
because we really need a clean release with no new regressions.

            regards, tom lane

On June 11, 2022 1:12:11 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In any case, I now agree with Robert's upthread objection that
> this should not have been back-patched.  It's a nontrivial
> behavioral change and it's not clear that it's 100% without
> downsides.  I particularly do not want to ship 14.4 with this,
> because we really need a clean release with no new regressions.

Okay. An issue here is that I don't have my laptop at hand until 14.4 wraps, which would be around Monday morning my
time.Could somebody do that instead? 
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On June 11, 2022 1:12:11 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In any case, I now agree with Robert's upthread objection that
>> this should not have been back-patched.  It's a nontrivial
>> behavioral change and it's not clear that it's 100% without
>> downsides.  I particularly do not want to ship 14.4 with this,
>> because we really need a clean release with no new regressions.

> Okay. An issue here is that I don't have my laptop at hand until 14.4 wraps, which would be around Monday morning my
time.Could somebody do that instead? 

OK, I can take care of it.

            regards, tom lane

On June 11, 2022 4:47:09 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> OK, I can take care of it.

Thanks. I'll look at the rest later.
--
Michael

On Fri, Jun 10, 2022 at 12:12:11PM -0400, Tom Lane wrote:
> Alternatively, one could argue that once we've decided to issue
> ROLLBACK, there's not really any reason to continue performing
> additional -c/-f actions, so that a sufficient fix would be to
> get rid of the loop's ON_ERROR_STOP conditionality:
>
> -            if (successResult != EXIT_SUCCESS && pset.on_error_stop)
> +            if (successResult != EXIT_SUCCESS)
>                  break;

This suggestion does not look right to me with --single-transaction.
If not using ON_ERROR_STOP, I think that we should still loop through
all the switches given by the caller and force a COMMIT all the time
because the intention is that we don't care about failures while
processing.  This gives me the attached as a result for HEAD, where we
would issue a ROLLBACK only when using ON_ERROR_STOP to stop
immediately if there is a backend-side error or a client-side error.
I have expanded the tests where not using ON_ERROR_STOP, with errors
triggered at the end of the set of switches.

Now, do we really want to introduce this new behavior on HEAD?  We are
post-beta so this does not me make me really comfortable if both
Robert and you don't like the change, even if the behavior of
--single-transaction/ON_ERROR_STOP on client-side failure is weird to
me and others from upthread.  If you'd prefer see the original logic
in place, I don't mind putting the code in its previous shape.
However, I would like to keep all those TAP tests because we have zero
coverage in this area, and we have the base file to be able to do so
now.
--
Michael

At Mon, 13 Jun 2022 15:45:05 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Jun 10, 2022 at 12:12:11PM -0400, Tom Lane wrote:
> > Alternatively, one could argue that once we've decided to issue
> > ROLLBACK, there's not really any reason to continue performing
> > additional -c/-f actions, so that a sufficient fix would be to
> > get rid of the loop's ON_ERROR_STOP conditionality:
> > 
> > -            if (successResult != EXIT_SUCCESS && pset.on_error_stop)
> > +            if (successResult != EXIT_SUCCESS)
> >                  break;
> 
> This suggestion does not look right to me with --single-transaction.
> If not using ON_ERROR_STOP, I think that we should still loop through
> all the switches given by the caller and force a COMMIT all the time
> because the intention is that we don't care about failures while
> processing.  This gives me the attached as a result for HEAD, where we

Agreed. It is actually a bug that on_error_stop is ignored here.

> would issue a ROLLBACK only when using ON_ERROR_STOP to stop
> immediately if there is a backend-side error or a client-side error.
> I have expanded the tests where not using ON_ERROR_STOP, with errors
> triggered at the end of the set of switches.


-            res = PSQLexec((successResult == EXIT_SUCCESS) ?
.
+            res = PSQLexec((successResult != EXIT_SUCCESS && pset.on_error_stop) ?

Thanks!

> Now, do we really want to introduce this new behavior on HEAD?  We are
> post-beta so this does not me make me really comfortable if both
> Robert and you don't like the change, even if the behavior of
> --single-transaction/ON_ERROR_STOP on client-side failure is weird to
> me and others from upthread.  If you'd prefer see the original logic
> in place, I don't mind putting the code in its previous shape.
> However, I would like to keep all those TAP tests because we have zero
> coverage in this area, and we have the base file to be able to do so
> now.

+1 for at least add the tests.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Mon, 13 Jun 2022 15:45:05 +0900, Michael Paquier <michael@paquier.xyz> wrote in
>> This suggestion does not look right to me with --single-transaction.
>> If not using ON_ERROR_STOP, I think that we should still loop through
>> all the switches given by the caller and force a COMMIT all the time
>> because the intention is that we don't care about failures while
>> processing.  This gives me the attached as a result for HEAD, where we

> Agreed. It is actually a bug that on_error_stop is ignored here.

Sounds plausible to me too.

>> Now, do we really want to introduce this new behavior on HEAD?  We are
>> post-beta so this does not me make me really comfortable if both
>> Robert and you don't like the change, even if the behavior of
>> --single-transaction/ON_ERROR_STOP on client-side failure is weird to
>> me and others from upthread.

I think it's fine to commit this to HEAD.  The reason for reverting
in the back branches was exactly that we'd already changed the behavior;
and what we now see is that it's still buggy and we need to change it
some more.  That's an entirely appropriate thing to be doing in beta.

            regards, tom lane

On Mon, Jun 13, 2022 at 10:52:16AM -0400, Tom Lane wrote:
> I think it's fine to commit this to HEAD.  The reason for reverting
> in the back branches was exactly that we'd already changed the behavior;
> and what we now see is that it's still buggy and we need to change it
> some more.  That's an entirely appropriate thing to be doing in beta.

Okay, let's do so then on HEAD.  I'll wait a bit more, in case others
have an opinion to offer on the matter.
--
Michael

On Tue, Jun 14, 2022 at 10:31:21AM +0900, Michael Paquier wrote:
> Okay, let's do so then on HEAD.  I'll wait a bit more, in case others
> have an opinion to offer on the matter.

Well, done.  One thing that I find a bit surprising in all that is the
lack of consistency in the handling of the return code of psql when
the last switch fails when not using ON_ERROR_STOP.  For example, psql
fails if the last switch is a slash command from -c, but succeeds if
the last switch is a slash command in a file from -f that exists.
This comes down to the way failures are passed down from MainLoop() so
I am not sure if this is worth worrying about and nobody has
complained about that AFAIK, but I have added some extra tests to at
least document everything I could think about to track changes, in
case somebody plays with this code in the future.
--
Michael

Re: Michael Paquier
> Well, done.  One thing that I find a bit surprising in all that is the 
> lack of consistency in the handling of the return code of psql when
> the last switch fails when not using ON_ERROR_STOP.

TBH ON_ERROR_STOP itself feels very brittle to me. Besides the fact
that `psql -vON_ERROR_STOP=1` looks very ugly, it's only one typo away
from breaking silently. `psql -vSTOP_ON_ERROR=1` (or -vON_EROR_STOP=1)
looks good as well, but doesn't work.

There should be a proper command line switch for it like
`psql --on-error-stop` since typoing that errors. (The short options
-eEsSq are already taken, so I'd propose psql -Q.)

Christoph
-- 
Senior Consultant, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley
Unser Umgang mit personenbezogenen Daten unterliegt folgenden
Bestimmungen: https://www.credativ.de/datenschutz

Re: Michael Paquier
> On Tue, Jun 14, 2022 at 10:31:21AM +0900, Michael Paquier wrote:
> > Okay, let's do so then on HEAD.  I'll wait a bit more, in case others
> > have an opinion to offer on the matter.
> 
> Well, done.  One thing that I find a bit surprising in all that is the 
> lack of consistency in the handling of the return code of psql when
> the last switch fails when not using ON_ERROR_STOP.  For example, psql
> fails if the last switch is a slash command from -c, but succeeds if
> the last switch is a slash command in a file from -f that exists.
> This comes down to the way failures are passed down from MainLoop() so
> I am not sure if this is worth worrying about and nobody has
> complained about that AFAIK, but I have added some extra tests to at
> least document everything I could think about to track changes, in
> case somebody plays with this code in the future.

Is there anything left to do to fix up the --single_transaction +
ON_ERROR_STOP case here? From reading the code, it now does what it
should do. What is left to do for backpatching it?

Christoph

On Mon, Jul 04, 2022 at 04:38:26PM +0200, Christoph Berg wrote:
> Is there anything left to do to fix up the --single_transaction +
> ON_ERROR_STOP case here? From reading the code, it now does what it
> should do. What is left to do for backpatching it?

As per upthread, Tom and Robert do not seem to agree on this point.
And it is true that this creates a behavior change.  At this stage, it
could be a good idea to gather more votes.
--
Michael