Tom Lane <t...@sss.pgh.pa.us> wrote:
> The main thing I would be worried about is whether you're sure
> that you have separated the RESET-as-a-command case from the cases
> where we actually are rolling back to a previous state.
It looks good to me. I added a few regression tests for that.
Robert Haas <robertmh...@gmail.com> wrote:
> +1 for such a comment.
Added.
The attached patch includes these. If it seems close, I'd be happy
to come up with a version for the 9.1 branch. Basically it looks
like that means omitting the changes to variable.c (which only
changed message text and made the order of steps on related GUCs
more consistent), and capturing a different out file for the
expected directory.
-Kevin
*** 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;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
