incorrect exit code from psql with single transaction + violation of deferred FK constraint

Hi

I've noticed that executing a sql script such with psql with -1
-vON_ERROR_STOP=on where the script causes a deferred foreign key
constraint to be violated returns 0 rather than the expected 3. I have
reproduced this in psql 8.4.1, 8.3.3 and 8.2.9, which does lead me to
wonder whether it is expected behaviour. However...

Sample code to reproduce:

-- test.sql
create table foo (id int primary key, foo_id int);
alter table foo add constraint fk1 foreign key (foo_id) references
foo(id) deferrable initially deferred;
insert into foo select 1,2;

for which:

psql -1 -vON_ERROR_STOP=on -f test.sql

returns 0 (but with message detailing the constraint violation)

psql -vON_ERROR_STOP=on -f test.sql

returns 3 (as expected).

However, with the constraint immediate, i.e.

-- test.sql
create table foo (id int primary key, foo_id int);
alter table foo add constraint fk1 foreign key (foo_id) references foo(id);
insert into foo select 1,2;

psql -1 -vON_ERROR_STOP=on -f test.sql

and

psql -vON_ERROR_STOP=on -f test.sql

both return 3 (which is the expected behaviour on my reading of the docs).

Also, interestingly, if I wrap the first script in begin; ... commit; I
always get 3 returned.

Thanks,

Dominic.

Dominic Bevacqua
Director
BPM Logic Ltd.
http://www.bpmlogic.com

Dominic, sorry you didn't get any reply to your bug report from October,
but it was picked up by Robert Haas in January:

http://archives.postgresql.org/pgsql-hackers/2010-01/msg00478.php

and is now listed as an outstanding 9.0 bug:

http://wiki.postgresql.org/wiki/PostgreSQL_9.0_Open_Items

What your bug report illustrates is that the BEGIN/COMMIT commands that
are used for psql -1 are not properly checking for return values. In
fact we are not even properly clearing their libpq result structures.
The "deferrable initially deferred" clause is causing the file to fail
on the commit, and lacking proper error checking, you are getting a zero
return value from psql.

The attached patch checks for the proper return from BEGIN/COMMIT, and
properly frees the libpq structures. In testing, this does return 3 as
you expected.

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

Dominic Bevacqua wrote:
> Hi
>
> I've noticed that executing a sql script such with psql with -1
> -vON_ERROR_STOP=on where the script causes a deferred foreign key
> constraint to be violated returns 0 rather than the expected 3. I have
> reproduced this in psql 8.4.1, 8.3.3 and 8.2.9, which does lead me to
> wonder whether it is expected behaviour. However...
>
> Sample code to reproduce:
>
> -- test.sql
> create table foo (id int primary key, foo_id int);
> alter table foo add constraint fk1 foreign key (foo_id) references
> foo(id) deferrable initially deferred;
> insert into foo select 1,2;
>
> for which:
>
> psql -1 -vON_ERROR_STOP=on -f test.sql
>
> returns 0 (but with message detailing the constraint violation)
>
> psql -vON_ERROR_STOP=on -f test.sql
>
> returns 3 (as expected).
>
> However, with the constraint immediate, i.e.
>
> -- test.sql
> create table foo (id int primary key, foo_id int);
> alter table foo add constraint fk1 foreign key (foo_id) references foo(id);
> insert into foo select 1,2;
>
> psql -1 -vON_ERROR_STOP=on -f test.sql
>
> and
>
> psql -vON_ERROR_STOP=on -f test.sql
>
> both return 3 (which is the expected behaviour on my reading of the docs).
>
> Also, interestingly, if I wrap the first script in begin; ... commit; I
> always get 3 returned.
>
> Thanks,
>
> Dominic.
>
> Dominic Bevacqua
> Director
> BPM Logic Ltd.
> http://www.bpmlogic.com
>
>
>
>
>
>
>
>
>

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> The attached patch checks for the proper return from BEGIN/COMMIT, and
> properly frees the libpq structures. In testing, this does return 3 as
> you expected.

Really? It looks to me like you'd get exit(1). Maybe that's the right
thing, but MainLoop itself seems to return EXIT_USER not EXIT_FAILURE
when it gets an error.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > The attached patch checks for the proper return from BEGIN/COMMIT, and
> > properly frees the libpq structures. In testing, this does return 3 as
> > you expected.
>
> Really? It looks to me like you'd get exit(1). Maybe that's the right
> thing, but MainLoop itself seems to return EXIT_USER not EXIT_FAILURE
> when it gets an error.

Sorry, you are right. I must have mis-read my tests. Updated patch
attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

BBruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > The attached patch checks for the proper return from BEGIN/COMMIT, and
> > > properly frees the libpq structures. In testing, this does return 3 as
> > > you expected.
> >
> > Really? It looks to me like you'd get exit(1). Maybe that's the right
> > thing, but MainLoop itself seems to return EXIT_USER not EXIT_FAILURE
> > when it gets an error.
>
> Sorry, you are right. I must have mis-read my tests. Updated patch
> attached.

I thought some more about it and realized I had to check for the
on-error-exit flag too. Updated patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Bruce Momjian wrote:
> BBruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > > The attached patch checks for the proper return from BEGIN/COMMIT, and
> > > > properly frees the libpq structures. In testing, this does return 3 as
> > > > you expected.
> > >
> > > Really? It looks to me like you'd get exit(1). Maybe that's the right
> > > thing, but MainLoop itself seems to return EXIT_USER not EXIT_FAILURE
> > > when it gets an error.
> >
> > Sorry, you are right. I must have mis-read my tests. Updated patch
> > attached.
>
> I thought some more about it and realized I had to check for the
> on-error-exit flag too. Updated patch attached.

Applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> I thought some more about it and realized I had to check for the
>> on-error-exit flag too. Updated patch attached.

> Applied.

Shouldn't that be back-patched?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> >> I thought some more about it and realized I had to check for the
> >> on-error-exit flag too. Updated patch attached.
>
> > Applied.
>
> Shouldn't that be back-patched?

Uh, well, it is going to change the behavior of back branches, and
because we only got one report of the bug which has existed since 8.2, I
didn't want to risk it. Should I?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> Shouldn't that be back-patched?

> Uh, well, it is going to change the behavior of back branches, and
> because we only got one report of the bug which has existed since 8.2, I
> didn't want to risk it. Should I?

I would say that the odds of the initial BEGIN failing are negligible
anyway, so what it boils down to is whether a failure on the final
COMMIT needs to be reported. Seems to me the answer is "yes", and the
only reason we haven't had more complaints is that not too many people
have actually relied on the exit status. Anyone who *does* look at the
exit status is not going to be happy with the current behavior.

In short: it's a bug, fix it.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> Shouldn't that be back-patched?
>
> > Uh, well, it is going to change the behavior of back branches, and
> > because we only got one report of the bug which has existed since 8.2, I
> > didn't want to risk it. Should I?
>
> I would say that the odds of the initial BEGIN failing are negligible
> anyway, so what it boils down to is whether a failure on the final
> COMMIT needs to be reported. Seems to me the answer is "yes", and the
> only reason we haven't had more complaints is that not too many people
> have actually relied on the exit status. Anyone who *does* look at the
> exit status is not going to be happy with the current behavior.
>
> In short: it's a bug, fix it.

OK, done.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do