postgres crash on CURSORS

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.

regards, tom lane

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.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

-----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

"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

-----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 */

"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

-----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

"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

-----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

"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