Re: run GUC check hooks on RESET

*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***************
*** 600,616 **** check_XactIsoLevel(char **newval, void **extra, GucSource source)
  
  	if (newXactIsoLevel != XactIsoLevel)
  	{
! 		if (FirstSnapshotSet)
  		{
  			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 			GUC_check_errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query");
  			return false;
  		}
! 		/* We ignore a subtransaction setting it to the existing value. */
! 		if (IsSubTransaction())
  		{
  			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 			GUC_check_errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction");
  			return false;
  		}
  		/* Can't go to serializable mode while recovery is still active */
--- 600,616 ----
  
  	if (newXactIsoLevel != XactIsoLevel)
  	{
! 		/* We ignore a subtransaction setting it to the existing value. */
! 		if (IsSubTransaction())
  		{
  			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 			GUC_check_errmsg("cannot set transaction isolation level in a subtransaction");
  			return false;
  		}
! 		if (FirstSnapshotSet)
  		{
  			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 			GUC_check_errmsg("transaction isolation level must be set before any query");
  			return false;
  		}
  		/* Can't go to serializable mode while recovery is still active */
***************
*** 665,677 **** check_transaction_deferrable(bool *newval, void **extra, GucSource source)
  	if (IsSubTransaction())
  	{
  		GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 		GUC_check_errmsg("SET TRANSACTION [NOT] DEFERRABLE cannot be called within a subtransaction");
  		return false;
  	}
  	if (FirstSnapshotSet)
  	{
  		GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 		GUC_check_errmsg("SET TRANSACTION [NOT] DEFERRABLE must be called before any query");
  		return false;
  	}
  
--- 665,677 ----
  	if (IsSubTransaction())
  	{
  		GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 		GUC_check_errmsg("cannot set transaction deferrable state within a subtransaction");
  		return false;
  	}
  	if (FirstSnapshotSet)
  	{
  		GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
! 		GUC_check_errmsg("transaction deferrable state must be set before any query");
  		return false;
  	}
  
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 5042,5047 **** config_enum_get_options(struct config_enum * record, const char *prefix,
--- 5042,5051 ----
   *
   * If value is NULL, set the option to its default value (normally the
   * reset_val, but if source == PGC_S_DEFAULT we instead use the boot_val).
+  * When we reset a value we can't assume that the old value is valid, but must
+  * call the check hook if present.  This is because the validity of a change
+  * might depend on context.  For example, transaction isolation may not be
+  * changed after the transaction has run a query, even by a RESET command.
   *
   * action indicates whether to set the value globally in the session, locally
   * to the current top transaction, or just for the duration of a function call.
***************
*** 5287,5302 **** set_config_option(const char *name, const char *value,
  								 name)));
  						return 0;
  					}
- 					if (!call_bool_check_hook(conf, &newval, &newextra,
- 											  source, elevel))
- 						return 0;
  				}
  				else if (source == PGC_S_DEFAULT)
  				{
  					newval = conf->boot_val;
- 					if (!call_bool_check_hook(conf, &newval, &newextra,
- 											  source, elevel))
- 						return 0;
  				}
  				else
  				{
--- 5291,5300 ----
***************
*** 5306,5311 **** set_config_option(const char *name, const char *value,
--- 5304,5313 ----
  					context = conf->gen.reset_scontext;
  				}
  
+ 				if (!call_bool_check_hook(conf, &newval, &newextra,
+ 										  source, elevel))
+ 					return 0;
+ 
  				if (prohibitValueChange)
  				{
  					if (*conf->variable != newval)
***************
*** 5391,5406 **** set_config_option(const char *name, const char *value,
  										newval, name, conf->min, conf->max)));
  						return 0;
  					}
- 					if (!call_int_check_hook(conf, &newval, &newextra,
- 											 source, elevel))
- 						return 0;
  				}
  				else if (source == PGC_S_DEFAULT)
  				{
  					newval = conf->boot_val;
- 					if (!call_int_check_hook(conf, &newval, &newextra,
- 											 source, elevel))
- 						return 0;
  				}
  				else
  				{
--- 5393,5402 ----
***************
*** 5410,5415 **** set_config_option(const char *name, const char *value,
--- 5406,5415 ----
  					context = conf->gen.reset_scontext;
  				}
  
+ 				if (!call_int_check_hook(conf, &newval, &newextra,
+ 										 source, elevel))
+ 					return 0;
+ 
  				if (prohibitValueChange)
  				{
  					if (*conf->variable != newval)
***************
*** 5492,5507 **** set_config_option(const char *name, const char *value,
  										newval, name, conf->min, conf->max)));
  						return 0;
  					}
- 					if (!call_real_check_hook(conf, &newval, &newextra,
- 											  source, elevel))
- 						return 0;
  				}
  				else if (source == PGC_S_DEFAULT)
  				{
  					newval = conf->boot_val;
- 					if (!call_real_check_hook(conf, &newval, &newextra,
- 											  source, elevel))
- 						return 0;
  				}
  				else
  				{
--- 5492,5501 ----
***************
*** 5511,5516 **** set_config_option(const char *name, const char *value,
--- 5505,5514 ----
  					context = conf->gen.reset_scontext;
  				}
  
+ 				if (!call_real_check_hook(conf, &newval, &newextra,
+ 										  source, elevel))
+ 					return 0;
+ 
  				if (prohibitValueChange)
  				{
  					if (*conf->variable != newval)
***************
*** 5591,5603 **** set_config_option(const char *name, const char *value,
  					 */
  					if (conf->gen.flags & GUC_IS_NAME)
  						truncate_identifier(newval, strlen(newval), true);
- 
- 					if (!call_string_check_hook(conf, &newval, &newextra,
- 												source, elevel))
- 					{
- 						free(newval);
- 						return 0;
- 					}
  				}
  				else if (source == PGC_S_DEFAULT)
  				{
--- 5589,5594 ----
***************
*** 5610,5635 **** set_config_option(const char *name, const char *value,
  					}
  					else
  						newval = NULL;
- 
- 					if (!call_string_check_hook(conf, &newval, &newextra,
- 												source, elevel))
- 					{
- 						free(newval);
- 						return 0;
- 					}
  				}
  				else
  				{
! 					/*
! 					 * strdup not needed, since reset_val is already under
! 					 * guc.c's control
! 					 */
! 					newval = conf->reset_val;
  					newextra = conf->reset_extra;
  					source = conf->gen.reset_source;
  					context = conf->gen.reset_scontext;
  				}
  
  				if (prohibitValueChange)
  				{
  					/* newval shouldn't be NULL, so we're a bit sloppy here */
--- 5601,5630 ----
  					}
  					else
  						newval = NULL;
  				}
  				else
  				{
! 					/* non-NULL reset_val must always get strdup'd */
! 					if (conf->reset_val != NULL)
! 					{
! 						newval = guc_strdup(elevel, conf->reset_val);
! 						if (newval == NULL)
! 							return 0;
! 					}
! 					else
! 						newval = NULL;
  					newextra = conf->reset_extra;
  					source = conf->gen.reset_source;
  					context = conf->gen.reset_scontext;
  				}
  
+ 				if (!call_string_check_hook(conf, &newval, &newextra,
+ 											source, elevel))
+ 				{
+ 					free(newval);
+ 					return 0;
+ 				}
+ 
  				if (prohibitValueChange)
  				{
  					/* newval shouldn't be NULL, so we're a bit sloppy here */
***************
*** 5721,5736 **** set_config_option(const char *name, const char *value,
  							pfree(hintmsg);
  						return 0;
  					}
- 					if (!call_enum_check_hook(conf, &newval, &newextra,
- 											  source, elevel))
- 						return 0;
  				}
  				else if (source == PGC_S_DEFAULT)
  				{
  					newval = conf->boot_val;
- 					if (!call_enum_check_hook(conf, &newval, &newextra,
- 											  source, elevel))
- 						return 0;
  				}
  				else
  				{
--- 5716,5725 ----
***************
*** 5740,5745 **** set_config_option(const char *name, const char *value,
--- 5729,5738 ----
  					context = conf->gen.reset_scontext;
  				}
  
+ 				if (!call_enum_check_hook(conf, &newval, &newextra,
+ 										  source, elevel))
+ 					return 0;
+ 
  				if (prohibitValueChange)
  				{
  					if (*conf->variable != newval)
*** a/src/test/regress/expected/transactions.out
--- b/src/test/regress/expected/transactions.out
***************
*** 53,58 **** SELECT * FROM writetest; -- ok
--- 53,90 ----
  SET TRANSACTION READ WRITE; --fail
  ERROR:  transaction read-write mode must be set before any query
  COMMIT;
+ SET default_transaction_read_only = FALSE;
+ SET default_transaction_isolation = 'read committed';
+ BEGIN;
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ ONLY; -- ok
+ SELECT * FROM writetest; -- ok
+  a 
+ ---
+ (0 rows)
+ 
+ RESET transaction_read_only; --fail
+ ERROR:  transaction read-write mode must be set before any query
+ COMMIT;
+ BEGIN;
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ ONLY; -- ok
+ SELECT * FROM writetest; -- ok
+  a 
+ ---
+ (0 rows)
+ 
+ RESET transaction_isolation; --fail
+ ERROR:  transaction isolation level must be set before any query
+ COMMIT;
+ BEGIN;
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ ONLY; -- ok
+ SELECT * FROM writetest; -- ok
+  a 
+ ---
+ (0 rows)
+ 
+ ROLLBACK; -- ok
+ RESET default_transaction_read_only;
+ RESET default_transaction_isolation;
  BEGIN;
  SET TRANSACTION READ ONLY; -- ok
  SET TRANSACTION READ WRITE; -- ok
***************
*** 391,396 **** SELECT 1;			-- this should work
--- 423,480 ----
          1
  (1 row)
  
+ -- test rollbacks and resets of GUC in transactions
+ CREATE SCHEMA albion;
+ SET search_path = "$user",public,albion;
+ DROP SCHEMA albion;
+ SHOW search_path;
+        search_path       
+ -------------------------
+  "$user", public, albion
+ (1 row)
+ 
+ BEGIN;
+ CREATE SCHEMA camelot;
+ SET search_path = "$user",public,camelot;
+ DROP SCHEMA camelot;
+ SAVEPOINT bedivere;
+ CREATE SCHEMA avalon;
+ SET search_path = "$user",public,avalon;
+ DROP SCHEMA avalon;
+ SHOW search_path;
+        search_path       
+ -------------------------
+  "$user", public, avalon
+ (1 row)
+ 
+ ROLLBACK TO SAVEPOINT bedivere;
+ SHOW search_path;
+        search_path        
+ --------------------------
+  "$user", public, camelot
+ (1 row)
+ 
+ RESET search_path;
+ SHOW search_path;
+   search_path   
+ ----------------
+  "$user",public
+ (1 row)
+ 
+ ROLLBACK;
+ SHOW search_path;
+        search_path       
+ -------------------------
+  "$user", public, albion
+ (1 row)
+ 
+ RESET search_path;
+ SHOW search_path;
+   search_path   
+ ----------------
+  "$user",public
+ (1 row)
+ 
  -- check non-transactional behavior of cursors
  BEGIN;
  	DECLARE c CURSOR FOR SELECT unique2 FROM tenk1 ORDER BY unique2;
*** a/src/test/regress/sql/transactions.sql
--- b/src/test/regress/sql/transactions.sql
***************
*** 45,50 **** SELECT * FROM writetest; -- ok
--- 45,73 ----
  SET TRANSACTION READ WRITE; --fail
  COMMIT;
  
+ SET default_transaction_read_only = FALSE;
+ SET default_transaction_isolation = 'read committed';
+ 
+ BEGIN;
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ ONLY; -- ok
+ SELECT * FROM writetest; -- ok
+ RESET transaction_read_only; --fail
+ COMMIT;
+ 
+ BEGIN;
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ ONLY; -- ok
+ SELECT * FROM writetest; -- ok
+ RESET transaction_isolation; --fail
+ COMMIT;
+ 
+ BEGIN;
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ ONLY; -- ok
+ SELECT * FROM writetest; -- ok
+ ROLLBACK; -- ok
+ 
+ RESET default_transaction_read_only;
+ RESET default_transaction_isolation;
+ 
  BEGIN;
  SET TRANSACTION READ ONLY; -- ok
  SET TRANSACTION READ WRITE; -- ok
***************
*** 252,257 **** BEGIN;
--- 275,303 ----
  COMMIT;
  SELECT 1;			-- this should work
  
+ -- test rollbacks and resets of GUC in transactions
+ CREATE SCHEMA albion;
+ SET search_path = "$user",public,albion;
+ DROP SCHEMA albion;
+ SHOW search_path;
+ BEGIN;
+ CREATE SCHEMA camelot;
+ SET search_path = "$user",public,camelot;
+ DROP SCHEMA camelot;
+ SAVEPOINT bedivere;
+ CREATE SCHEMA avalon;
+ SET search_path = "$user",public,avalon;
+ DROP SCHEMA avalon;
+ SHOW search_path;
+ ROLLBACK TO SAVEPOINT bedivere;
+ SHOW search_path;
+ RESET search_path;
+ SHOW search_path;
+ ROLLBACK;
+ SHOW search_path;
+ RESET search_path;
+ SHOW search_path;
+ 
  -- check non-transactional behavior of cursors
  BEGIN;
  	DECLARE c CURSOR FOR SELECT unique2 FROM tenk1 ORDER BY unique2;