On Sat, Nov 9, 2013 at 02:15:52PM -0500, Robert Haas wrote:
> On Fri, Nov 8, 2013 at 5:36 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > [ I'm so far behind ... ]
> >
> > Bruce Momjian <br...@momjian.us> writes:
> >> Applied. Thank you for all your suggestions.
> >
> > I thought the suggestion had been to issue a *warning*. How did that
> > become an error? This patch seems likely to break applications that
> > may have just been harmlessly sloppy about when they were issuing
> > SETs and/or what flavor of SET they use. We don't for example throw
> > an error for START TRANSACTION with an open transaction or COMMIT or
> > ROLLBACK without one --- how can it possibly be argued that these
> > operations are more dangerous than those cases?
> >
> > I'd personally have voted for using NOTICE.
>
> Well, LOCK TABLE throws an error, so it's not without precedent.
OK, I dug into all cases where commands that are meaningless outside of
transactions currently throw an error; they are:
DECLARE
LOCK
ROLLBACK TO SAVEPOINT
SET LOCAL*
SET CONSTRAINTS*
SET TRANSACTION*
SAVEPOINT
The stared items are new in 9.4. Here is the current behavior:
test=> LOCK lkjasdf;
ERROR: LOCK TABLE can only be used in transaction blocks
test=> SET LOCAL x = 3;
ERROR: SET LOCAL can only be used in transaction blocks
test=> SAVEPOINT lkjsafd;
ERROR: SAVEPOINT can only be used in transaction blocks
test=> ROLLBACK TO SAVEPOINT asdf;
ERROR: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
Notice that they do _not_ check their arguments; they just throw
errors. With this patch they issue warnings and evaluate their
arguments:
test=> LOCK lkjasdf;
WARNING: LOCK TABLE can only be used in transaction blocks
ERROR: relation "lkjasdf" does not exist
test=> SET LOCAL x = 3;
WARNING: SET LOCAL can only be used in transaction blocks
ERROR: unrecognized configuration parameter "x"
test=> SAVEPOINT lkjsafd;
WARNING: SAVEPOINT can only be used in transaction blocks
SAVEPOINT
test=> ROLLBACK TO SAVEPOINT asdf;
WARNING: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
ROLLBACK
However, SAVEPOINT/ROLLBACK throw weird errors when they are evaluated
outside a multi-statement transaction, so their arguments are not
evaluated. This might be why they were originally marked as errors.
A patch to issue only warnings is attached. In a way this change
improves the code by throwing errors only when the commands are invalid,
rather than just useless. You could argue that ROLLBACK TO SAVEPOINT
should throw an error because no savepoint name is valid in that
context.
--
Bruce Momjian <br...@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/doc/src/sgml/ref/declare.sgml b/doc/src/sgml/ref/declare.sgml
new file mode 100644
index d500faa..7b8c6b6
*** a/doc/src/sgml/ref/declare.sgml
--- b/doc/src/sgml/ref/declare.sgml
*************** DECLARE <replaceable class="parameter">n
*** 179,187 ****
created by this command can only be used within the current
transaction. Thus, <command>DECLARE</> without <literal>WITH
HOLD</literal> is useless outside a transaction block: the cursor would
! survive only to the completion of the statement. Therefore
! <productname>PostgreSQL</productname> reports an error if such a
! command is used outside a transaction block.
Use
<xref linkend="sql-begin"> and
<xref linkend="sql-commit">
--- 179,185 ----
created by this command can only be used within the current
transaction. Thus, <command>DECLARE</> without <literal>WITH
HOLD</literal> is useless outside a transaction block: the cursor would
! survive only to the completion of the statement.
Use
<xref linkend="sql-begin"> and
<xref linkend="sql-commit">
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
new file mode 100644
index 95d6767..6a0ad20
*** a/doc/src/sgml/ref/lock.sgml
--- b/doc/src/sgml/ref/lock.sgml
*************** LOCK [ TABLE ] [ ONLY ] <replaceable cla
*** 168,176 ****
<para>
<command>LOCK TABLE</> is useless outside a transaction block: the lock
! would remain held only to the completion of the statement. Therefore
! <productname>PostgreSQL</productname> reports an error if <command>LOCK</>
! is used outside a transaction block.
Use
<xref linkend="sql-begin"> and
<xref linkend="sql-commit">
--- 168,174 ----
<para>
<command>LOCK TABLE</> is useless outside a transaction block: the lock
! would remain held only to the completion of the statement.
Use
<xref linkend="sql-begin"> and
<xref linkend="sql-commit">
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 6290c9d..aaaf186
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*************** SET [ SESSION | LOCAL ] TIME ZONE { <rep
*** 111,117 ****
Specifies that the command takes effect for only the current
transaction. After <command>COMMIT</> or <command>ROLLBACK</>,
the session-level setting takes effect again.
! <productname>PostgreSQL</productname> reports an error if
<command>SET LOCAL</> is used outside a transaction block.
</para>
</listitem>
--- 111,117 ----
Specifies that the command takes effect for only the current
transaction. After <command>COMMIT</> or <command>ROLLBACK</>,
the session-level setting takes effect again.
! <productname>PostgreSQL</productname> generates a warning if
<command>SET LOCAL</> is used outside a transaction block.
</para>
</listitem>
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index 895a5fd..2fd228c
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*************** SET CONSTRAINTS { ALL | <replaceable cla
*** 99,108 ****
<para>
This command only alters the behavior of constraints within the
! current transaction. Thus, if you execute this command outside of a
! transaction block
! (<command>BEGIN</command>/<command>COMMIT</command> pair), it will
! generate an error.
</para>
</refsect1>
--- 99,105 ----
<para>
This command only alters the behavior of constraints within the
! current transaction and is useless outside a transaction block.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index 391464a..a568ab7
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*************** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 183,194 ****
<title>Notes</title>
<para>
- If <command>SET TRANSACTION</command> is executed without a prior
- <command>START TRANSACTION</command> or <command>BEGIN</command>,
- it will generate an error.
- </para>
-
- <para>
It is possible to dispense with <command>SET TRANSACTION</command>
by instead specifying the desired <replaceable
class="parameter">transaction_modes</replaceable> in
--- 183,188 ----
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
new file mode 100644
index 0591f3f..4e492c4
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*************** PreventTransactionChain(bool isTopLevel,
*** 2949,2955 ****
}
/*
! * RequireTransactionChain
*
* This routine is to be called by statements that must run inside
* a transaction block, because they have no effects that persist past
--- 2949,2955 ----
}
/*
! * IsTransactionChain
*
* This routine is to be called by statements that must run inside
* a transaction block, because they have no effects that persist past
*************** PreventTransactionChain(bool isTopLevel,
*** 2963,2996 ****
*
* isTopLevel: passed down from ProcessUtility to determine whether we are
* inside a function.
! * stmtType: statement type name, for error messages.
*/
! void
! RequireTransactionChain(bool isTopLevel, const char *stmtType)
{
/*
* xact block already started?
*/
if (IsTransactionBlock())
! return;
/*
* subtransaction?
*/
if (IsSubTransaction())
! return;
/*
* inside a function call?
*/
if (!isTopLevel)
! return;
! ereport(ERROR,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
errmsg("%s can only be used in transaction blocks",
stmtType)));
}
/*
--- 2963,2997 ----
*
* isTopLevel: passed down from ProcessUtility to determine whether we are
* inside a function.
! * stmtType: statement type name, for warning messages.
*/
! bool
! IsTransactionChain(bool isTopLevel, const char *stmtType)
{
/*
* xact block already started?
*/
if (IsTransactionBlock())
! return true;
/*
* subtransaction?
*/
if (IsSubTransaction())
! return true;
/*
* inside a function call?
*/
if (!isTopLevel)
! return true;
! ereport(WARNING,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
errmsg("%s can only be used in transaction blocks",
stmtType)));
+ return false;
}
/*
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
new file mode 100644
index 5c3f42c..6be3fa1
*** a/src/backend/commands/portalcmds.c
--- b/src/backend/commands/portalcmds.c
*************** PerformCursorOpen(PlannedStmt *stmt, Par
*** 66,72 ****
* user-visible effect).
*/
if (!(cstmt->options & CURSOR_OPT_HOLD))
! RequireTransactionChain(isTopLevel, "DECLARE CURSOR");
/*
* Create a portal and copy the plan and queryString into its memory.
--- 66,72 ----
* user-visible effect).
*/
if (!(cstmt->options & CURSOR_OPT_HOLD))
! IsTransactionChain(isTopLevel, "DECLARE CURSOR");
/*
* Create a portal and copy the plan and queryString into its memory.
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
new file mode 100644
index 6a7bf0d..0566e59
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** standard_ProcessUtility(Node *parsetree,
*** 461,490 ****
ListCell *cell;
char *name = NULL;
! RequireTransactionChain(isTopLevel, "SAVEPOINT");
!
! foreach(cell, stmt->options)
{
! DefElem *elem = lfirst(cell);
!
! if (strcmp(elem->defname, "savepoint_name") == 0)
! name = strVal(elem->arg);
}
-
- Assert(PointerIsValid(name));
-
- DefineSavepoint(name);
}
break;
case TRANS_STMT_RELEASE:
! RequireTransactionChain(isTopLevel, "RELEASE SAVEPOINT");
! ReleaseSavepoint(stmt->options);
break;
case TRANS_STMT_ROLLBACK_TO:
! RequireTransactionChain(isTopLevel, "ROLLBACK TO SAVEPOINT");
! RollbackToSavepoint(stmt->options);
/*
* CommitTransactionCommand is in charge of
--- 461,491 ----
ListCell *cell;
char *name = NULL;
! if (IsTransactionChain(isTopLevel, "SAVEPOINT"))
{
! foreach(cell, stmt->options)
! {
! DefElem *elem = lfirst(cell);
!
! if (strcmp(elem->defname, "savepoint_name") == 0)
! name = strVal(elem->arg);
! }
!
! Assert(PointerIsValid(name));
!
! DefineSavepoint(name);
}
}
break;
case TRANS_STMT_RELEASE:
! if (IsTransactionChain(isTopLevel, "RELEASE SAVEPOINT"))
! ReleaseSavepoint(stmt->options);
break;
case TRANS_STMT_ROLLBACK_TO:
! if (IsTransactionChain(isTopLevel, "ROLLBACK TO SAVEPOINT"))
! RollbackToSavepoint(stmt->options);
/*
* CommitTransactionCommand is in charge of
*************** standard_ProcessUtility(Node *parsetree,
*** 749,760 ****
* Since the lock would just get dropped immediately, LOCK TABLE
* outside a transaction block is presumed to be user error.
*/
! RequireTransactionChain(isTopLevel, "LOCK TABLE");
LockTableCommand((LockStmt *) parsetree);
break;
case T_ConstraintsSetStmt:
! RequireTransactionChain(isTopLevel, "SET CONSTRAINTS");
AfterTriggerSetState((ConstraintsSetStmt *) parsetree);
break;
--- 750,761 ----
* Since the lock would just get dropped immediately, LOCK TABLE
* outside a transaction block is presumed to be user error.
*/
! IsTransactionChain(isTopLevel, "LOCK TABLE");
LockTableCommand((LockStmt *) parsetree);
break;
case T_ConstraintsSetStmt:
! IsTransactionChain(isTopLevel, "SET CONSTRAINTS");
AfterTriggerSetState((ConstraintsSetStmt *) parsetree);
break;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 54d8078..2437007
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6274,6280 ****
case VAR_SET_VALUE:
case VAR_SET_CURRENT:
if (stmt->is_local)
! RequireTransactionChain(isTopLevel, "SET LOCAL");
(void) set_config_option(stmt->name,
ExtractSetVariableArgs(stmt),
(superuser() ? PGC_SUSET : PGC_USERSET),
--- 6274,6280 ----
case VAR_SET_VALUE:
case VAR_SET_CURRENT:
if (stmt->is_local)
! IsTransactionChain(isTopLevel, "SET LOCAL");
(void) set_config_option(stmt->name,
ExtractSetVariableArgs(stmt),
(superuser() ? PGC_SUSET : PGC_USERSET),
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6295,6301 ****
{
ListCell *head;
! RequireTransactionChain(isTopLevel, "SET TRANSACTION");
foreach(head, stmt->args)
{
--- 6295,6301 ----
{
ListCell *head;
! IsTransactionChain(isTopLevel, "SET TRANSACTION");
foreach(head, stmt->args)
{
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6346,6352 ****
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("SET LOCAL TRANSACTION SNAPSHOT is not implemented")));
! RequireTransactionChain(isTopLevel, "SET TRANSACTION");
Assert(IsA(con, A_Const));
Assert(nodeTag(&con->val) == T_String);
ImportSnapshot(strVal(&con->val));
--- 6346,6352 ----
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("SET LOCAL TRANSACTION SNAPSHOT is not implemented")));
! IsTransactionChain(isTopLevel, "SET TRANSACTION");
Assert(IsA(con, A_Const));
Assert(nodeTag(&con->val) == T_String);
ImportSnapshot(strVal(&con->val));
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6357,6367 ****
break;
case VAR_SET_DEFAULT:
if (stmt->is_local)
! RequireTransactionChain(isTopLevel, "SET LOCAL");
/* fall through */
case VAR_RESET:
if (strcmp(stmt->name, "transaction_isolation") == 0)
! RequireTransactionChain(isTopLevel, "RESET TRANSACTION");
(void) set_config_option(stmt->name,
NULL,
--- 6357,6367 ----
break;
case VAR_SET_DEFAULT:
if (stmt->is_local)
! IsTransactionChain(isTopLevel, "SET LOCAL");
/* fall through */
case VAR_RESET:
if (strcmp(stmt->name, "transaction_isolation") == 0)
! IsTransactionChain(isTopLevel, "RESET TRANSACTION");
(void) set_config_option(stmt->name,
NULL,
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
new file mode 100644
index 835f6ac..690b50d
*** a/src/include/access/xact.h
--- b/src/include/access/xact.h
*************** extern bool IsTransactionOrTransactionBl
*** 244,250 ****
extern char TransactionBlockStatusCode(void);
extern void AbortOutOfAnyTransaction(void);
extern void PreventTransactionChain(bool isTopLevel, const char *stmtType);
! extern void RequireTransactionChain(bool isTopLevel, const char *stmtType);
extern bool IsInTransactionChain(bool isTopLevel);
extern void RegisterXactCallback(XactCallback callback, void *arg);
extern void UnregisterXactCallback(XactCallback callback, void *arg);
--- 244,250 ----
extern char TransactionBlockStatusCode(void);
extern void AbortOutOfAnyTransaction(void);
extern void PreventTransactionChain(bool isTopLevel, const char *stmtType);
! extern bool IsTransactionChain(bool isTopLevel, const char *stmtType);
extern bool IsInTransactionChain(bool isTopLevel);
extern void RegisterXactCallback(XactCallback callback, void *arg);
extern void UnregisterXactCallback(XactCallback callback, void *arg);
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
new file mode 100644
index 203fa6e..4f0065c
*** a/src/test/regress/expected/guc.out
--- b/src/test/regress/expected/guc.out
*************** SELECT '2006-08-13 12:34:56'::timestampt
*** 29,35 ****
-- SET LOCAL has no effect outside of a transaction
SET LOCAL vacuum_cost_delay TO 50;
! ERROR: SET LOCAL can only be used in transaction blocks
SHOW vacuum_cost_delay;
vacuum_cost_delay
-------------------
--- 29,35 ----
-- SET LOCAL has no effect outside of a transaction
SET LOCAL vacuum_cost_delay TO 50;
! WARNING: SET LOCAL can only be used in transaction blocks
SHOW vacuum_cost_delay;
vacuum_cost_delay
-------------------
*************** SHOW vacuum_cost_delay;
*** 37,43 ****
(1 row)
SET LOCAL datestyle = 'SQL';
! ERROR: SET LOCAL can only be used in transaction blocks
SHOW datestyle;
DateStyle
-----------
--- 37,43 ----
(1 row)
SET LOCAL datestyle = 'SQL';
! WARNING: SET LOCAL can only be used in transaction blocks
SHOW datestyle;
DateStyle
-----------
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
new file mode 100644
index 5c84ec5..45bbe8a
*** a/src/test/regress/expected/transactions.out
--- b/src/test/regress/expected/transactions.out
*************** SELECT a FROM savepoints WHERE a BETWEEN
*** 363,373 ****
DROP TABLE savepoints;
-- only in a transaction block:
SAVEPOINT one;
! ERROR: SAVEPOINT can only be used in transaction blocks
ROLLBACK TO SAVEPOINT one;
! ERROR: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
RELEASE SAVEPOINT one;
! ERROR: RELEASE SAVEPOINT can only be used in transaction blocks
-- Only "rollback to" allowed in aborted state
BEGIN;
SAVEPOINT one;
--- 363,373 ----
DROP TABLE savepoints;
-- only in a transaction block:
SAVEPOINT one;
! WARNING: SAVEPOINT can only be used in transaction blocks
ROLLBACK TO SAVEPOINT one;
! WARNING: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
RELEASE SAVEPOINT one;
! WARNING: RELEASE SAVEPOINT can only be used in transaction blocks
-- Only "rollback to" allowed in aborted state
BEGIN;
SAVEPOINT one;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
