pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Dmitry Koval <d.koval@postgrespro.ru>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: andrewbille@gmail.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end
Date: Mon, 7 Feb 2022 00:21:39 +0300
Message-ID: <6e31c4ef-de9a-960b-1a21-e3b6ff0fa9c2@postgrespro.ru> (raw )
In-Reply-To: <CAFiTN-sCr6mYg8A_BBiRz0N68M9QY3uoBWHhbPgHDM-goLJGwg@mail.gmail.com >
References: <17385-9ee529fb091f0ce5@postgresql.org >
<b22395d2-d432-f211-0bed-1f466df75152@postgrespro.ru >
<CAD21AoCkwsb_Hz6JYbG0X8Jf_VEMheiib0FXpR2kc3UATVCPNQ@mail.gmail.com >
<CAFiTN-sYZ0Cuyeygp7z8xfoFPhqTAbtnV+k2W9OHp8s0xus5vw@mail.gmail.com >
<f97d670d-2b03-9d47-6b64-09e6ed2f4d84@postgrespro.ru >
<CAFiTN-sCr6mYg8A_BBiRz0N68M9QY3uoBWHhbPgHDM-goLJGwg@mail.gmail.com >
Hi,
I a bit changed Masahiko Sawada's patch with using Dilip Kumar's
recommendations (see it in attachment).
About testing.
The most simple path to testing of command "RESET <GUC_string_variable>"
(with error) that I found:
----
1) need to modify "postgresql.conf". Add string:
default_table_access_method = 'heapXXX'
2) start PostgreSQL;
3) start "psql" and execute command:
RESET default_table_access_method;
After that we get an error:
ERROR: invalid value for parameter "default_table_access_method": "heapXXX"
DETAIL: Table access method "heapXXX" does not exist.
Without attached patch command "RESET default_table_access_method;"
returns no error.
----
Probably no reason to create new tap-test for this case...
With best regards,
Dmitry Koval.
Attachments:
v2_0001-call-check-hook-on-reset.patch (text/plain, 7.5K) download
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f505413a7f..a6c35ee21f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7611,19 +7611,27 @@ set_config_option(const char *name, const char *value,
&newval_union, &newextra))
return 0;
}
- else if (source == PGC_S_DEFAULT)
+ else
{
- newval = conf->boot_val;
+ if (source != PGC_S_DEFAULT)
+ newval = conf->reset_val;
+ else
+ newval = conf->boot_val;
+
if (!call_bool_check_hook(conf, &newval, &newextra,
source, elevel))
return 0;
- }
- else
- {
- newval = conf->reset_val;
- newextra = conf->reset_extra;
- source = conf->gen.reset_source;
- context = conf->gen.reset_scontext;
+
+ if (source != PGC_S_DEFAULT)
+ {
+ /* Release newextra as we use reset_extra */
+ if (newextra)
+ free(newextra);
+
+ newextra = conf->reset_extra;
+ source = conf->gen.reset_source;
+ context = conf->gen.reset_scontext;
+ }
}
if (prohibitValueChange)
@@ -7705,19 +7713,30 @@ set_config_option(const char *name, const char *value,
&newval_union, &newextra))
return 0;
}
- else if (source == PGC_S_DEFAULT)
+ else
{
- newval = conf->boot_val;
+ if (source != PGC_S_DEFAULT)
+ newval = conf->reset_val;
+ else
+ newval = conf->boot_val;
+
if (!call_int_check_hook(conf, &newval, &newextra,
source, elevel))
return 0;
- }
- else
- {
- newval = conf->reset_val;
- newextra = conf->reset_extra;
- source = conf->gen.reset_source;
- context = conf->gen.reset_scontext;
+
+ if (source != PGC_S_DEFAULT)
+ {
+ /* Release newextra as we use reset_extra */
+ if (newextra)
+ {
+ Assert(!extra_field_used(&conf->gen, newextra));
+ free(newextra);
+ }
+
+ newextra = conf->reset_extra;
+ source = conf->gen.reset_source;
+ context = conf->gen.reset_scontext;
+ }
}
if (prohibitValueChange)
@@ -7799,19 +7818,27 @@ set_config_option(const char *name, const char *value,
&newval_union, &newextra))
return 0;
}
- else if (source == PGC_S_DEFAULT)
+ else
{
- newval = conf->boot_val;
+ if (source != PGC_S_DEFAULT)
+ newval = conf->reset_val;
+ else
+ newval = conf->boot_val;
+
if (!call_real_check_hook(conf, &newval, &newextra,
source, elevel))
return 0;
- }
- else
- {
- newval = conf->reset_val;
- newextra = conf->reset_extra;
- source = conf->gen.reset_source;
- context = conf->gen.reset_scontext;
+
+ if (source != PGC_S_DEFAULT)
+ {
+ /* Release newextra as we use reset_extra */
+ if (newextra)
+ free(newextra);
+
+ newextra = conf->reset_extra;
+ source = conf->gen.reset_source;
+ context = conf->gen.reset_scontext;
+ }
}
if (prohibitValueChange)
@@ -7893,12 +7920,19 @@ set_config_option(const char *name, const char *value,
&newval_union, &newextra))
return 0;
}
- else if (source == PGC_S_DEFAULT)
+ else
{
- /* non-NULL boot_val must always get strdup'd */
- if (conf->boot_val != NULL)
+ const char *confval;
+
+ if (source != PGC_S_DEFAULT)
+ confval = conf->reset_val;
+ else
+ confval = conf->boot_val;
+
+ /* non-NULL value must always get strdup'd */
+ if (confval)
{
- newval = guc_strdup(elevel, conf->boot_val);
+ newval = guc_strdup(elevel, confval);
if (newval == NULL)
return 0;
}
@@ -7911,17 +7945,29 @@ set_config_option(const char *name, const char *value,
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 (source != PGC_S_DEFAULT)
+ {
+ /* Release newextra as we use reset_extra */
+ if (newextra)
+ free(newextra);
+
+ /*
+ * Release newval (after guc_strdup) before
+ * overwriting
+ */
+ if (newval)
+ free(newval);
+
+ /*
+ * 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)
@@ -8018,19 +8064,27 @@ set_config_option(const char *name, const char *value,
&newval_union, &newextra))
return 0;
}
- else if (source == PGC_S_DEFAULT)
+ else
{
- newval = conf->boot_val;
+ if (source != PGC_S_DEFAULT)
+ newval = conf->reset_val;
+ else
+ newval = conf->boot_val;
+
if (!call_enum_check_hook(conf, &newval, &newextra,
source, elevel))
return 0;
- }
- else
- {
- newval = conf->reset_val;
- newextra = conf->reset_extra;
- source = conf->gen.reset_source;
- context = conf->gen.reset_scontext;
+
+ if (source != PGC_S_DEFAULT)
+ {
+ /* Release newextra as we use reset_extra */
+ if (newextra)
+ free(newextra);
+
+ newextra = conf->reset_extra;
+ source = conf->gen.reset_source;
+ context = conf->gen.reset_scontext;
+ }
}
if (prohibitValueChange)
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 61862d595d..1f301f3033 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -40,6 +40,38 @@ SELECT * FROM aggtest;
42 | 324.78
(4 rows)
+-- Test that we disallow changing transaction characteristics after the
+-- first snapshot acquisition.
+BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; -- ok
+SELECT count(*) FROM xacttest; -- ok
+ count
+-------
+ 5
+(1 row)
+
+RESET transaction_isolation; -- fail
+ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any query
+COMMIT;
+BEGIN TRANSACTION READ ONLY; -- ok
+SELECT count(*) FROM xacttest; -- ok
+ count
+-------
+ 5
+(1 row)
+
+RESET transaction_read_only; -- fail
+ERROR: transaction read-write mode must be set before any query
+COMMIT;
+BEGIN TRANSACTION DEFERRABLE; -- ok
+SELECT count(*) FROM xacttest; -- ok
+ count
+-------
+ 5
+(1 row)
+
+RESET transaction_deferrable; -- fail
+ERROR: SET TRANSACTION [NOT] DEFERRABLE must be called before any query
+COMMIT;
-- Read-only tests
CREATE TABLE writetest (a int);
CREATE TEMPORARY TABLE temptest (a int);
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 8886280c0a..b4b9b43f77 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -33,6 +33,22 @@ SELECT oid FROM pg_class WHERE relname = 'disappear';
-- should have members again
SELECT * FROM aggtest;
+-- Test that we disallow changing transaction characteristics after the
+-- first snapshot acquisition.
+BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; -- ok
+SELECT count(*) FROM xacttest; -- ok
+RESET transaction_isolation; -- fail
+COMMIT;
+
+BEGIN TRANSACTION READ ONLY; -- ok
+SELECT count(*) FROM xacttest; -- ok
+RESET transaction_read_only; -- fail
+COMMIT;
+
+BEGIN TRANSACTION DEFERRABLE; -- ok
+SELECT count(*) FROM xacttest; -- ok
+RESET transaction_deferrable; -- fail
+COMMIT;
-- Read-only tests
view thread (44+ messages) latest in thread
Message-ID: <6e31c4ef-de9a-960b-1a21-e3b6ff0fa9c2@postgrespro.ru>
Permalink: ../6e31c4ef-de9a-960b-1a21-e3b6ff0fa9c2@postgrespro.ru/
Also on: postgresql.org/message-id/6e31c4ef-de9a-960b-1a21-e3b6ff0fa9c2@postgrespro.ru
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: d.koval@postgrespro.ru, dilipbalaut@gmail.com, sawada.mshk@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end
In-Reply-To: <6e31c4ef-de9a-960b-1a21-e3b6ff0fa9c2@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox