Обсуждение: Re: incorrect exit code from psql with single transaction + violation of deferred FK constraint
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
Index: src/bin/psql/command.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.216
diff -c -c -r1.216 command.c
*** src/bin/psql/command.c 26 Feb 2010 02:01:17 -0000 1.216
--- src/bin/psql/command.c 7 Mar 2010 17:40:47 -0000
***************
*** 1731,1740 ****
--- 1731,1752 ----
pset.inputfile = filename;
if (single_txn)
+ {
res = PSQLexec("BEGIN", false);
+ if (!res)
+ return EXIT_FAILURE;
+ PQclear(res);
+ }
+
result = MainLoop(fd);
+
if (single_txn)
+ {
res = PSQLexec("COMMIT", false);
+ if (!res)
+ return EXIT_FAILURE;
+ PQclear(res);
+ }
fclose(fd);
pset.inputfile = oldfilename;
Bruce Momjian <bruce@momjian.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do Index: src/bin/psql/command.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v retrieving revision 1.216 diff -c -c -r1.216 command.c *** src/bin/psql/command.c 26 Feb 2010 02:01:17 -0000 1.216 --- src/bin/psql/command.c 8 Mar 2010 00:29:26 -0000 *************** *** 1731,1740 **** --- 1731,1752 ---- pset.inputfile = filename; if (single_txn) + { res = PSQLexec("BEGIN", false); + if (!res) + return EXIT_USER; + PQclear(res); + } + result = MainLoop(fd); + if (single_txn) + { res = PSQLexec("COMMIT", false); + if (!res) + return EXIT_USER; + PQclear(res); + } fclose(fd); pset.inputfile = oldfilename;
BBruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do Index: src/bin/psql/command.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v retrieving revision 1.216 diff -c -c -r1.216 command.c *** src/bin/psql/command.c 26 Feb 2010 02:01:17 -0000 1.216 --- src/bin/psql/command.c 8 Mar 2010 00:46:10 -0000 *************** *** 1731,1740 **** pset.inputfile = filename; if (single_txn) ! res = PSQLexec("BEGIN", false); result = MainLoop(fd); if (single_txn) ! res = PSQLexec("COMMIT", false); fclose(fd); pset.inputfile = oldfilename; --- 1731,1758 ---- pset.inputfile = filename; if (single_txn) ! { ! if ((res = PSQLexec("BEGIN", false)) == NULL) ! { ! if (pset.on_error_stop) ! return EXIT_USER; ! } ! else ! PQclear(res); ! } ! result = MainLoop(fd); + if (single_txn) ! { ! if ((res = PSQLexec("COMMIT", false)) == NULL) ! { ! if (pset.on_error_stop) ! return EXIT_USER; ! } ! else ! PQclear(res); ! } fclose(fd); pset.inputfile = oldfilename;
Bruce Momjian wrote: > BBruce Momjian wrote: > > Tom Lane wrote: > > > Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
Bruce Momjian <bruce@momjian.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
Bruce Momjian <bruce@momjian.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do