Re: [bug fix] Savepoint-related statements terminates connection

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 one
953 * raw parsetree. If more than one, it's effectively a
transaction block
954 * and we want PreventTransactionChain to reject unsafe
commands. (Note:
955 * we're assuming that query rewrite cannot add commands that are
956 * 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(dot)takay(at)jp(dot)fujitsu(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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