Re: [bug fix] Savepoint-related statements terminates connection - Mailing list pgsql-hackers

Please add this to the next commitfest.

I think there's some misunderstanding between exec_simple_query() and
the way we manage transaction block state machine.

In exec_simple_query()952      * We'll tell PortalRun it's a top-level command iff there's
exactly one953      * raw parsetree.  If more than one, it's effectively a
transaction block954      * and we want PreventTransactionChain to reject unsafe
commands. (Note:955      * we're assuming that query rewrite cannot add commands that are956      * significant to
PreventTransactionChain.)957     */958     isTopLevel = (list_length(parsetree_list) == 1);
 

it assumes that a multi-statement command is a transaction block. But
for every statement in this multi-statement, we toggle between
TBLOCK_STARTED and TBLOCK_DEFAULT never entering TBLOCK_INPROGRESS as
expected by a transaction block. It looks like we have to fix this
transaction block state machine for multi-statement commands. One way
to fix it is to call finish_xact_command() in exec_simple_query() at
958 when it sees that it's a transaction block. I am not sure if
that's correct. We have to at least fix the comment above or even stop
setting isTopLevel for mult-statement commands.

I don't think the fix in the patch is on the right track, since
RequireTransactionChain() is supposed to do exactly what the patch
intends to do.
3213 /*
3214  *  RequireTransactionChain
3215  *
3216  *  This routine is to be called by statements that must run inside
3217  *  a transaction block, because they have no effects that persist past
3218  *  transaction end (and so calling them outside a transaction block
3219  *  is presumably an error).  DECLARE CURSOR is an example.

Incidently we allow cursor operations in a multi-statement command
psql -d postgres -c "select 1; declare curs cursor for select * from
pg_class; fetch from curs;"  relname    | relnamespace | reltype | reloftype | relowner | relam
| relfilenode | reltablespace | relpages | reltuples | relallvisible |
reltoastre
lid | relhasindex | relisshared | relpersistence | relkind | relnatts
| relchecks | relhasoids | relhaspkey | relhasrules | relhastriggers |
relhassubc
lass | relrowsecurity | relforcerowsecurity | relispopulated |
relreplident | relispartition | relfrozenxid | relminmxid |
relacl
| reloptions | relpartbound

--------------+--------------+---------+-----------+----------+-------+-------------+---------------+----------+-----------+---------------+-----------

----+-------------+-------------+----------------+---------+----------+-----------+------------+------------+-------------+----------------+-----------

-----+----------------+---------------------+----------------+--------------+----------------+--------------+------------+-----------------------------
+------------+--------------pg_statistic |           11 |   11258 |         0 |       10 |     0
|        2619 |             0 |       16 |       388 |            16 |        2
840 | t           | f           | p              | r       |       26
|         0 | f          | f          | f           | f              |
f    | f              | f                   | t              | n    | f              |          547 |          1 |
{ashutosh=arwdDxt/ashutosh}
|            |
(1 row)

Then the question is why not to allow savepoints as well? For that we
have to fix transaction block state machine.

On Fri, Mar 31, 2017 at 12:40 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> Hello,
>
> I found a trivial bug that terminates the connection.  The attached patch fixes this.
>
>
> PROBLEM
> ========================================
>
> Savepoint-related statements in a multi-command query terminates the connection unexpectedly, as follows.
>
> $ psql -d postgres -c "SELECT 1; SAVEPOINT sp"
> FATAL:  DefineSavepoint: unexpected state STARTED
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> connection to server was lost
>
>
> CAUSE
> ========================================
>
> 1. In exec_simple_query(), isTopLevel is set to false.
>
>         isTopLevel = (list_length(parsetree_list) == 1);
>
> Then it is passed to PortalRun().
>
>                 (void) PortalRun(portal,
>                                                  FETCH_ALL,
>                                                  isTopLevel,
>                                                  receiver,
>                                                  receiver,
>                                                  completionTag);
>
> 2. The isTopLevel flag is passed through ProcessUtility() to RequireTransactionChain().
>
>                                                         RequireTransactionChain(isTopLevel, "SAVEPOINT");
>
>
> 3. CheckTransactionChain() returns successfully here:
>
>         /*
>          * inside a function call?
>          */
>         if (!isTopLevel)
>                 return;
>
>
> 4. Finally, unexpectedly called DefineSavepoint() reports invalid transaction block state.
>
>                         /* These cases are invalid. */
>                 case TBLOCK_DEFAULT:
>                 case TBLOCK_STARTED:
> ...
>                         elog(FATAL, "DefineSavepoint: unexpected state %s",
>                                  BlockStateAsString(s->blockState));
>
>
>
> SOLUTION
> ========================================
>
> The manual page says "Savepoints can only be established when inside a transaction block."  So just check for it.
>
> https://www.postgresql.org/docs/devel/static/sql-savepoint.html
>
>
> RESULT AFTER FIX
> ========================================
>
> $ psql -d postgres -c "SELECT 1; SAVEPOINT sp"
> ERROR:  SAVEPOINT can only be used in transaction blocks
>
>
> Regards
> Takayuki Tsunakawa
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company