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
