On Sun, Feb  3, 2013 at 07:19:14AM +0000, Amit kapila wrote:
> 
> On Saturday, February 02, 2013 9:08 PM Robert Haas wrote:
> On Fri, Feb 1, 2013 at 12:04 AM, Amit Kapila <amit.kap...@huawei.com> wrote:
> >> I think user should be aware of effect before using SET commands, as these 
> >> are used at various levels (TRANSACTION, SESSION, ...).
> 
> > Ideally, sure.  But these kinds of mistakes are easy to make.  
> 
>   You mean to say that after using SET Transaction, user can think below 
> statements will
>   use modified transaction property. But I think if user doesn't understand 
> by default
>   transaction will be committed after the statement execution, he might 
> expect after 
>   few statements he can rollback.
> 
> > That's why LOCK and DECLARE CURSOR already emit errors in this case - why
> > should this one be any different?
> 
> IMO, I think error should be given when it is not possible to execute current 
> statement.
> 
> Not only LOCK,DECLARE CURSOR but SAVEPOINT and some other statements also 
> give the same error,
> so if we want to throw error for such behavior, we can find all such similar 
> statements 
> (SET TRANSACTION, SET LOCAL, etc) and do it for all.
> 
> This can be helpful to some users, but not sure if such behavior (statement 
> can be executed but it will not have any sense) 
> can be always detectable and maintaible.

I have created the attached patch which issues an error when SET
TRANSACTION and SET LOCAL are used outside of transactions:

        test=> set transaction isolation level serializable;
        ERROR:  SET TRANSACTION can only be used in transaction blocks
        test=> reset transaction isolation level;
        ERROR:  RESET TRANSACTION can only be used in transaction blocks

        test=> set local effective_cache_size = '3MB';
        ERROR:  SET LOCAL can only be used in transaction blocks
        test=> set local effective_cache_size = default;
        ERROR:  SET LOCAL can only be used in transaction blocks

-- 
  Bruce Momjian  <br...@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
new file mode 100644
index b1023c4..0da041b
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** standard_ProcessUtility(Node *parsetree,
*** 688,694 ****
  			break;
  
  		case T_VariableSetStmt:
! 			ExecSetVariableStmt((VariableSetStmt *) parsetree);
  			break;
  
  		case T_VariableShowStmt:
--- 688,694 ----
  			break;
  
  		case T_VariableSetStmt:
! 			ExecSetVariableStmt((VariableSetStmt *) parsetree, isTopLevel);
  			break;
  
  		case T_VariableShowStmt:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 7d297bc..168cd95
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** flatten_set_variable_args(const char *na
*** 6240,6246 ****
   * SET command
   */
  void
! ExecSetVariableStmt(VariableSetStmt *stmt)
  {
  	GucAction	action = stmt->is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET;
  
--- 6240,6246 ----
   * SET command
   */
  void
! ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
  {
  	GucAction	action = stmt->is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET;
  
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6248,6253 ****
--- 6248,6255 ----
  	{
  		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),
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6257,6263 ****
  									 0);
  			break;
  		case VAR_SET_MULTI:
- 
  			/*
  			 * Special-case SQL syntaxes.  The TRANSACTION and SESSION
  			 * CHARACTERISTICS cases effectively set more than one variable
--- 6259,6264 ----
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6269,6274 ****
--- 6270,6277 ----
  			{
  				ListCell   *head;
  
+ 				RequireTransactionChain(isTopLevel, "SET TRANSACTION");
+ 
  				foreach(head, stmt->args)
  				{
  					DefElem    *item = (DefElem *) lfirst(head);
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6313,6318 ****
--- 6316,6323 ----
  			{
  				A_Const    *con = (A_Const *) linitial(stmt->args);
  
+ 				RequireTransactionChain(isTopLevel, "SET TRANSACTION");
+ 
  				if (stmt->is_local)
  					ereport(ERROR,
  							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
*************** ExecSetVariableStmt(VariableSetStmt *stm
*** 6326,6332 ****
--- 6331,6343 ----
  					 stmt->name);
  			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,
  									 (superuser() ? PGC_SUSET : PGC_USERSET),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
new file mode 100644
index 99211c1..89ee40c
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** extern void SetPGVariable(const char *na
*** 334,340 ****
  extern void GetPGVariable(const char *name, DestReceiver *dest);
  extern TupleDesc GetPGVariableResultDesc(const char *name);
  
! extern void ExecSetVariableStmt(VariableSetStmt *stmt);
  extern char *ExtractSetVariableArgs(VariableSetStmt *stmt);
  
  extern void ProcessGUCArray(ArrayType *array,
--- 334,340 ----
  extern void GetPGVariable(const char *name, DestReceiver *dest);
  extern TupleDesc GetPGVariableResultDesc(const char *name);
  
! extern void ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel);
  extern char *ExtractSetVariableArgs(VariableSetStmt *stmt);
  
  extern void ProcessGUCArray(ArrayType *array,
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
new file mode 100644
index 7b5a624..203fa6e
*** a/src/test/regress/expected/guc.out
--- b/src/test/regress/expected/guc.out
*************** SELECT '2006-08-13 12:34:56'::timestampt
*** 29,34 ****
--- 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 
  -------------------
*************** SHOW vacuum_cost_delay;
*** 36,41 ****
--- 37,43 ----
  (1 row)
  
  SET LOCAL datestyle = 'SQL';
+ ERROR:  SET LOCAL can only be used in transaction blocks
  SHOW datestyle;
   DateStyle 
  -----------
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to