> -----Original Message-----
> From: pgsql-hackers-owner@hub.org [mailto:pgsql-hackers-owner@hub.org]On
> Behalf Of Bruce Momjian
> 
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > I have found I can crash the backend with the following queries:
> > > 	test=> BEGIN WORK;
> > > 	BEGIN
> > > 	test=> DECLARE bigtable_cursor CURSOR FOR
> > test-> SELECT * FROM bigtable;
> > > 	SELECT
> > > 	test=> FETCH 3 prior FROM bigtable_cursor;
> > > 	ERROR:  parser: parse error at or near "prior"
> > > 	test=> FETCH prior FROM bigtable_cursor;
> > > 	pqReadData() -- backend closed the channel unexpectedly.
> > > 	        This probably means the backend terminated abnormally
> > > 	        before or while processing the request.
> > > 	The connection to the server was lost. Attempting reset: Succeeded.
> > 
> > Problem appears to be due to trying to bull ahead with processing after
> > the current transaction has been aborted by an error.  The immediate
> > cause is these lines in postgres.c:
> > 
> >             /*
> >              * We have to set query SnapShot in the case of 
> FETCH or COPY TO.
> >              */
> >             if (nodeTag(querytree->utilityStmt) == T_FetchStmt ||
> >                 (nodeTag(querytree->utilityStmt) == T_CopyStmt && 
> >                 ((CopyStmt 
> *)(querytree->utilityStmt))->direction != FROM))
> >                 SetQuerySnapshot();
> > 
> > which are executed without having bothered to check for aborted state.
> > I think this code should be removed from postgres.c, and the
> > SetQuerySnapshot call instead made from the Fetch and Copy arms of the
> > switch statement in ProcessUtility() (utility.c), after doing
> > CHECK_IF_ABORTED in each case.
> 
> Yes, I saw this code and got totally confused of how to fix it.
>

Is it bad to check ABORTED after yyparse() in parser.c ?

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp

Wed, 5 Apr 2000 13:02:06 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: postgres crash on CURSORS
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>>>> which are executed without having bothered to check for aborted state.
>>>> I think this code should be removed from postgres.c, and the
>>>> SetQuerySnapshot call instead made from the Fetch and Copy arms of the
>>>> switch statement in ProcessUtility() (utility.c), after doing
>>>> CHECK_IF_ABORTED in each case.

> Is it bad to check ABORTED after yyparse() in parser.c ?

Yes.  Try to execute an END (a/k/a ABORT, ROLLBACK, ...)

The check for abort state has to happen in the appropriate paths of
execution, not in the parser.  Not all statements should reject on
abort state.

			regards, tom lane

Wed, 05 Apr 2000 00:04:07 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: postgres crash on CURSORS 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Sent: Wednesday, April 05, 2000 1:04 PM

>
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> >>>> which are executed without having bothered to check for
> aborted state.
> >>>> I think this code should be removed from postgres.c, and the
> >>>> SetQuerySnapshot call instead made from the Fetch and Copy
> arms of the
> >>>> switch statement in ProcessUtility() (utility.c), after doing
> >>>> CHECK_IF_ABORTED in each case.
>
> > Is it bad to check ABORTED after yyparse() in parser.c ?
>
> Yes.  Try to execute an END (a/k/a ABORT, ROLLBACK, ...)
>
> The check for abort state has to happen in the appropriate paths of
> execution, not in the parser.  Not all statements should reject on
> abort state.
>

Are there any statements which should be executable on abort state
except ROLLBACK/COMMIT ?
The following is a sample patch for parser.c.

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp

Index: parser.c
===================================================================
RCS file: /home/cvs/pgcurrent/backend/parser/parser.c,v
retrieving revision 1.2
diff -c -r1.2 parser.c
*** parser.c    2000/01/26 09:58:32     1.2
--- parser.c    2000/04/05 03:54:31
***************
*** 16,21 ****
--- 16,24 ----
  #include "parser/analyze.h"
  #include "parser/gramparse.h"
  #include "parser/parser.h"
+ #include "nodes/parsenodes.h"
+ #include "access/xact.h"
+ #include "parse.h"

  #if defined(FLEX_SCANNER)
  extern void DeleteBuffer(void);
***************
*** 48,53 ****
--- 51,82 ----
        parser_init(typev, nargs);
--- 51,82 ----
        parser_init(typev, nargs);
        yyresult = yyparse();

+       /* To avoid doing processing within an aborted transaction block. */
+       if (!yyresult && IsAbortedTransactionBlockState())
+       {
+               Node    *node = lfirst(parsetree);
+
+               if (IsA(node, TransactionStmt))
+               {
+                       TransactionStmt *stmt=(TransactionStmt *)node;
+                       switch (stmt->command)
+                       {
+                               case ROLLBACK:
+                               case COMMIT:
+                                       break;
+                               default:
+                                       yyresult = -1;
+                                       break;
+                       }
+               }
+               else
+                       yyresult = -1;
+               if (yyresult)
+               {
+                       elog(NOTICE, "(transaction already aborted): %s",
+                               "queries ignored until END");
+               }
+       }
  #if defined(FLEX_SCANNER)
        DeleteBuffer();
  #endif         /* FLEX_SCANNER */


Wed, 5 Apr 2000 15:00:19 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: postgres crash on CURSORS 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> The check for abort state has to happen in the appropriate paths of
>> execution, not in the parser.  Not all statements should reject on
>> abort state.

> Are there any statements which should be executable on abort state
> except ROLLBACK/COMMIT ?

I dunno ... but offhand, I see no really good reason for checking this
in the parser rather than the way it's done now.  Presumably only
utility statements would be candidates for exemption from abort checks,
so checking in the switch statement in ProcessUtility makes sense to
me --- that way the knowledge of the semantics of a given utility
statement is localized.

> The following is a sample patch for parser.c.

The specific patch you propose is definitely inferior to the currently-
committed code, because it does not deal properly with COMMIT/ROLLBACK
appearing within a list of queries.  If we are in abort state and
the submitted query string is

	SELECT foo ; ROLLBACK ; SELECT bar

it seems to me that the correct response is to reject the first select
and process the second.  The currently committed code does so, but
your patch would fail.

			regards, tom lane

Wed, 05 Apr 2000 02:10:52 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: postgres crash on CURSORS 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> >> The check for abort state has to happen in the appropriate paths of
> >> execution, not in the parser.  Not all statements should reject on
> >> abort state.
> 
> > Are there any statements which should be executable on abort state
> > except ROLLBACK/COMMIT ?
> 
> I dunno ... but offhand, I see no really good reason for checking this
> in the parser rather than the way it's done now.  Presumably only
> utility statements would be candidates for exemption from abort checks,
> so checking in the switch statement in ProcessUtility makes sense to
> me --- that way the knowledge of the semantics of a given utility
> statement is localized.
>

Current abort check seems too late.
For example,is the following behavior preferable ?

=# begin;
BEGIN
=# aaa;
ERROR:  parser: parse error at or near "aaa"
=# select * from aaaa;
ERROR:  Relation 'aaaa' does not exist
	?? existence check ?? Why ??

reindex=# select * from t; -- t is a existent table
NOTICE:  (transaction aborted): queries ignored until END
*ABORT STATE*

> > The following is a sample patch for parser.c.
> 
> The specific patch you propose is definitely inferior to the currently-
> committed code, because it does not deal properly with COMMIT/ROLLBACK
> appearing within a list of queries.  If we are in abort state and
> the submitted query string is
> 
> 	SELECT foo ; ROLLBACK ; SELECT bar
> 
> it seems to me that the correct response is to reject the first select
> and process the second.  The currently committed code does so, but
> your patch would fail.
>

It seems pg_parse_and_plan() returns NIL plan_list and NIL
querytree_list in this case.

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp 

Wed, 5 Apr 2000 17:05:19 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: postgres crash on CURSORS 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> Current abort check seems too late.
> For example,is the following behavior preferable ?

> =# begin;
> BEGIN
> =# aaa;
> ERROR:  parser: parse error at or near "aaa"
> =# select * from aaaa;
> ERROR:  Relation 'aaaa' does not exist
> 	?? existence check ?? Why ??

> reindex=# select * from t; -- t is a existent table
> NOTICE:  (transaction aborted): queries ignored until END
> *ABORT STATE*

Hmm.  The error of course appears because we perform parsing and
rewriting before checking for abort. I am not sure whether the rewriter
can introduce begin/commit commands at present, but I would not like to
design the front-end processing on the assumption that it can never do so.
So I think we have to run the query that far before we think about
aborting.

>> If we are in abort state and
>> the submitted query string is
>> 
>> SELECT foo ; ROLLBACK ; SELECT bar
>> 
>> it seems to me that the correct response is to reject the first select
>> and process the second.  The currently committed code does so, but
>> your patch would fail.

> It seems pg_parse_and_plan() returns NIL plan_list and NIL
> querytree_list in this case.

You're not looking at current CVS ;-)

			regards, tom lane

Wed, 05 Apr 2000 11:04:13 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: postgres crash on CURSORS 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> 
> >> If we are in abort state and
> >> the submitted query string is
> >> 
> >> SELECT foo ; ROLLBACK ; SELECT bar
> >> 
> >> it seems to me that the correct response is to reject the first select
> >> and process the second.  The currently committed code does so, but
> >> your patch would fail.
> 
> > It seems pg_parse_and_plan() returns NIL plan_list and NIL
> > querytree_list in this case.
> 
> You're not looking at current CVS ;-)
>

Sorry,I see your change now.

Unfortunately I've never used multiple query and understand
little about it. For example,how to know using libpq that the first
select was ignored ?

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp 

Thu, 6 Apr 2000 10:37:14 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: postgres crash on CURSORS 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
>>>>> If we are in abort state and
>>>>> the submitted query string is
>>>>> 
>>>>> SELECT foo ; ROLLBACK ; SELECT bar
>>>>> 
>>>>> it seems to me that the correct response is to reject the first select
>>>>> and process the second.

> Unfortunately I've never used multiple query and understand
> little about it. For example,how to know using libpq that the first
> select was ignored ?

If you use PQexec then you can't really tell, because you'll only get
back the last command's result.  If you use PQsendQuery/PQgetResult
then you'll get back multiple PGresults from a multi-query string, and
you can examine each one to see if it was executed or not.

			regards, tom lane

Thu, 06 Apr 2000 17:42:31 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: postgres crash on CURSORS 