Skip to content

Commit d2976d4

Browse files
michae2michae2
committed
sql: ignore some session variables in RESET ALL
Looking at `src/backend/utils/misc/guc_tables.c` in PG source code, there are a few session variables flagged with `GUC_NO_RESET_ALL` that should not be affected by RESET ALL. This commit adds a `NoResetAll` flag to the following session variables that overlap with PG: - `is_superuser` - `role` - `session_authorization` - `transaction_isolation` - `transaction_read_only` This commit also adds the flag to a couple CRDB-only session variables that seem similar: - `transaction_priority` - `transaction_status` Most of these variables are read-only, and so were not affected by RESET ALL, but the new flag makes the intended behavior clear in case any of them do become writable in the future. By fixing RESET ALL, this change also makes DISCARD ALL work correctly when `default_transaction_use_follower_reads` is enabled. Fixes: #124150 Release note (bug fix): The RESET ALL statement is fixed to no longer affect the following session variables: - `is_superuser` - `role` - `session_authorization` - `transaction_isolation` - `transaction_priority` - `transaction_status` - `transaction_read_only` This better matches PostgreSQL behavior for RESET ALL. By fixing RESET ALL, the DISCARD ALL statement is also fixed to work correctly when `default_transaction_use_follower_reads` is enabled.
1 parent dd7f87c commit d2976d4

File tree

7 files changed

+144
-4
lines changed

7 files changed

+144
-4
lines changed

‎pkg/ccl/logictestccl/testdata/logic_test/read_committed

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -639,3 +639,24 @@ statement ok
639639
ROLLBACK
640640

641641
subtest end
642+
643+
# Verify that RESET ALL does not change transaction_isolation.
644+
645+
statement ok
646+
BEGIN ISOLATION LEVEL READ COMMITTED
647+
648+
query T
649+
SHOW transaction_isolation
650+
----
651+
read committed
652+
653+
statement ok
654+
RESET ALL
655+
656+
query T
657+
SHOW transaction_isolation
658+
----
659+
read committed
660+
661+
statement ok
662+
COMMIT

‎pkg/sql/discard.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ func (n *discardNode) startExec(params runParams) error {
4444

4545
// RESET ALL
4646
if err := params.p.resetAllSessionVars(params.ctx); err != nil {
47-
4847
return err
4948
}
5049

‎pkg/sql/logictest/testdata/logic_test/discard

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,3 +223,62 @@ SELECT currval('discard_seq')
223223
query T rowsort
224224
SELECT table_name FROM [SHOW TABLES FROM pg_temp]
225225
----
226+
227+
# Check that DISCARD still works with follower reads.
228+
229+
query T
230+
SET search_path = bar, public; SHOW search_path
231+
----
232+
bar, public
233+
234+
query T
235+
SET timezone = 'Europe/Amsterdam'; SHOW timezone
236+
----
237+
Europe/Amsterdam
238+
239+
statement ok
240+
PREPARE a AS SELECT 1
241+
242+
statement ok
243+
CREATE SEQUENCE discard_seq START WITH 10
244+
245+
statement ok
246+
CREATE TEMP TABLE tempy (a int);
247+
248+
query T rowsort
249+
SELECT table_name FROM [SHOW TABLES FROM pg_temp]
250+
----
251+
tempy
252+
253+
statement ok
254+
SET default_transaction_use_follower_reads = on
255+
256+
# DISCARD should be allowed, even with AOST set.
257+
statement ok
258+
DISCARD ALL
259+
260+
# The DISCARD ALL should have reset default_transaction_use_follower_reads.
261+
query T
262+
SHOW default_transaction_use_follower_reads
263+
----
264+
off
265+
266+
query T
267+
SHOW search_path
268+
----
269+
"$user", public
270+
271+
query T
272+
SHOW timezone
273+
----
274+
UTC
275+
276+
statement error prepared statement \"a\" does not exist
277+
DEALLOCATE a
278+
279+
statement error pgcode 55000 pq: currval of sequence "test.public.discard_seq" is not yet defined in this session
280+
SELECT currval('discard_seq')
281+
282+
query T rowsort
283+
SELECT table_name FROM [SHOW TABLES FROM pg_temp]
284+
----

‎pkg/sql/logictest/testdata/logic_test/reset

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,3 +63,42 @@ query T
6363
RESET time zone; SHOW TIME ZONE
6464
----
6565
UTC
66+
67+
# Verify that RESET ALL does not change transaction_read_only.
68+
69+
statement ok
70+
BEGIN TRANSACTION READ ONLY
71+
72+
query T
73+
SHOW transaction_read_only
74+
----
75+
on
76+
77+
statement ok
78+
RESET ALL
79+
80+
query T
81+
SHOW transaction_read_only
82+
----
83+
on
84+
85+
statement ok
86+
COMMIT
87+
88+
# Verify that RESET ALL works with follower reads.
89+
90+
statement ok
91+
SET default_transaction_use_follower_reads = on
92+
93+
query T
94+
SHOW default_transaction_use_follower_reads
95+
----
96+
on
97+
98+
statement ok
99+
RESET ALL
100+
101+
query T
102+
SHOW default_transaction_use_follower_reads
103+
----
104+
off

‎pkg/sql/logictest/testdata/logic_test/set_role

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,16 @@ SHOW is_superuser
134134
----
135135
on
136136

137+
# Verify that RESET ALL does not change is_superuser.
138+
139+
statement ok
140+
RESET ALL
141+
142+
query T
143+
SHOW is_superuser
144+
----
145+
on
146+
137147
# Check testuser cannot transition to other users as it has no privileges.
138148
user testuser
139149

‎pkg/sql/set_var.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,8 @@ func (p *planner) resetAllSessionVars(ctx context.Context) error {
205205
if v.Set == nil && v.RuntimeSet == nil && v.SetWithPlanner == nil {
206206
continue
207207
}
208-
// For Postgres compatibility, Don't reset `role` here.
209-
if varName == "role" {
208+
// For Postgres compatibility, don't reset some settings here.
209+
if v.NoResetAll {
210210
continue
211211
}
212212
hasDefault, defVal := getSessionVarDefaultString(

‎pkg/sql/vars.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@ type sessionVar struct {
7373
// Hidden indicates that the variable should not show up in the output of SHOW ALL.
7474
Hidden bool
7575

76+
// NoResetAll indicates that the variable should not be reset by RESET
77+
// ALL. (For the PG equivalent, see the GUC_NO_RESET_ALL flag in
78+
// src/backend/utils/misc/guc_tables.c of PG source.)
79+
NoResetAll bool
80+
7681
// Get returns a string representation of a given variable to be used
7782
// either by SHOW or in the pg_catalog table.
7883
Get func(evalCtx *extendedEvalContext, kv *kv.Txn) (string, error)
@@ -1214,6 +1219,7 @@ var varGen = map[string]sessionVar{
12141219
),
12151220

12161221
`is_superuser`: {
1222+
NoResetAll: true,
12171223
Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
12181224
return formatBoolAsPostgresSetting(evalCtx.SessionData().IsSuperuser), nil
12191225
},
@@ -1378,6 +1384,7 @@ var varGen = map[string]sessionVar{
13781384

13791385
// See https://www.postgresql.org/docs/current/sql-set-role.html.
13801386
`role`: {
1387+
NoResetAll: true,
13811388
Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
13821389
if evalCtx.SessionData().SessionUserProto == "" {
13831390
return username.NoneRole, nil
@@ -1618,7 +1625,8 @@ var varGen = map[string]sessionVar{
16181625
// See pg sources src/backend/utils/misc/guc.c. The variable is defined
16191626
// but is hidden from SHOW ALL.
16201627
`session_authorization`: {
1621-
Hidden: true,
1628+
Hidden: true,
1629+
NoResetAll: true,
16221630
Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
16231631
return evalCtx.SessionData().User().Normalized(), nil
16241632
},
@@ -1731,6 +1739,7 @@ var varGen = map[string]sessionVar{
17311739
// been executed yet.
17321740
// See https://github.com/postgres/postgres/blob/REL_10_STABLE/src/backend/utils/misc/guc.c#L3401-L3409
17331741
`transaction_isolation`: {
1742+
NoResetAll: true,
17341743
Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
17351744
level := tree.FromKVIsoLevel(evalCtx.Txn.IsoLevel())
17361745
return strings.ToLower(level.String()), nil
@@ -1762,20 +1771,23 @@ var varGen = map[string]sessionVar{
17621771

17631772
// CockroachDB extension.
17641773
`transaction_priority`: {
1774+
NoResetAll: true,
17651775
Get: func(evalCtx *extendedEvalContext, txn *kv.Txn) (string, error) {
17661776
return txn.UserPriority().String(), nil
17671777
},
17681778
},
17691779

17701780
// CockroachDB extension.
17711781
`transaction_status`: {
1782+
NoResetAll: true,
17721783
Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
17731784
return evalCtx.TxnState, nil
17741785
},
17751786
},
17761787

17771788
// See https://www.postgresql.org/docs/10/static/hot-standby.html#HOT-STANDBY-USERS
17781789
`transaction_read_only`: {
1790+
NoResetAll: true,
17791791
GetStringVal: makePostgresBoolGetStringValFn("transaction_read_only"),
17801792
Set: func(_ context.Context, m sessionDataMutator, s string) error {
17811793
b, err := paramparse.ParseBoolVar("transaction_read_only", s)

0 commit comments

Comments
 (0)