Hi,
On Sat, Aug 27, 2022 at 01:17:45PM +0200, Pavel Stehule wrote:
>
> after some thinking I think that instead of sequence I can use LSN. The
> combination oid, LSN should be unique forever
Yeah I was about suggesting doing that instead of a sequence, so +1 for that
approach!
I've been spending a bit of time trying to improve the test coverage on the
protection for concurrently deleted and recreated variables, and thought that a
new isolation test should be enough. I'm attaching a diff (in .txt extension)
that could be applied to 009-regress-tests-for-session-variables.patch, but
while working on that I discovered a few problems.
First, the pg_debug_show_used_session_variables() function reports what's
currently locally known, but there's no guarantee that
AcceptInvalidationMessages() will be called prior to its execution. For
instance if you're in a transaction and already hold a lock on the function and
execute it again.
It therefore means that it can display that a locally cached variable isn't
dropped and still holds a value, while it's not the case. While it may be
surprising, I think that's still the wanted behavior as you want to know what
is the cache state. FTR this is tested in the last permutation in the attached
patch (although the expected output contains the up-to-date information, so you
can see the failure).
But if invalidation are processed when calling the function, the behavior seems
surprising as far as I can see the cleanup seems to be done in 2 steps: mark t
he hash entry as removed and then remove the hash entry. For instance:
(conn 1) CREATE VARIABLE myvar AS text;
(conn 1) LET myvar = 'something';
(conn 2) DROP VARIABLE myvar;
(conn 1) SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
schema | name | removed
--------+-------+---------
public | myvar | t
(1 row)
(conn 1) SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
schema | name | removed
--------+------+---------
(0 rows)
Why are two steps necessary here, and is that really wanted?
Finally, I noticed that it's quite easy to get cache lookup failures when using
transactions. AFAICS it's because the current code first checks in the local
cache (which often isn't immediately invalidated when in a transaction),
returns an oid (of an already dropped variable), then the code acquires a lock
on that non-existent variable, which internally accepts invalidation after the
lock is acquired. The rest of the code can then fail with some "cache lookup
error" in the various functions as the invalidation has now been processed.
This is also tested in the attached isolation test.
I think that using a retry approach based on SharedInvalidMessageCounter change
detection, like RangeVarGetRelidExtended(), in IdentifyVariable() should be
enough to fix that class of problem, but maybe some other general functions
would need similar protection too.
While looking at the testing, I also noticed that the main regression tests
comments are now outdated since the new (and more permissive) approach for
dropped variable detection. For instance:
+ ALTER TYPE public.svar_test_type DROP ATTRIBUTE c;
+ -- should to fail
+ SELECT public.svar;
+ svar
+ ---------
+ (10,20)
+ (1 row)
+
+ ALTER TYPE public.svar_test_type ADD ATTRIBUTE c int;
+ -- should to fail too (different type, different generation number);
+ SELECT public.svar;
+ svar
+ ----------
+ (10,20,)
+ (1 row)
I'm also unsure if this one is showing a broken behavior or not:
+ CREATE VARIABLE public.avar AS int;
+ -- should to fail
+ SELECT avar FROM xxtab;
+ avar
+ ------
+ 10
+ (1 row)
+
+ -- should be ok
+ SELECT public.avar FROM xxtab;
+ avar
+ ------
+
+ (1 row)
For reference, with the code as-is I get the following diff when testing the
attached isolation test:
---
/Users/rjuju/git/postgresql/src/test/isolation/expected/session-variable.out
2022-08-29 15:41:11.000000000 +0800
+++
/Users/rjuju/git/pg/pgmaster_debug/src/test/isolation/output_iso/results/session-variable.out
2022-08-29 15:42:17.000000000 +0800
@@ -16,21 +16,21 @@
step let: LET myvar = 'test';
step val: SELECT myvar;
myvar
-----
test
(1 row)
step s1: BEGIN;
step drop: DROP VARIABLE myvar;
step val: SELECT myvar;
-ERROR: column or variable "myvar" does not exist
+ERROR: cache lookup failed for session variable 16386
step sr1: ROLLBACK;
starting permutation: let val dbg drop create dbg val
step let: LET myvar = 'test';
step val: SELECT myvar;
myvar
-----
test
(1 row)
@@ -68,20 +68,16 @@
schema|name |removed
------+-----+-------
public|myvar|f
(1 row)
step drop: DROP VARIABLE myvar;
step create: CREATE VARIABLE myvar AS text
step dbg: SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
schema|name |removed
------+-----+-------
-public|myvar|t
+public|myvar|f
(1 row)
step val: SELECT myvar;
-myvar
------
-
-(1 row)
-
+ERROR: cache lookup failed for session variable 16389
step sr1: ROLLBACK;
>From 9a572f21b37670aa8372e654c209fbccc9ca44d7 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
Date: Mon, 29 Aug 2022 16:38:52 +0800
Subject: [PATCH] Add isolation test for session variables.
---
.../isolation/expected/session-variable.out | 87 +++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/session-variable.spec | 34 ++++++++
3 files changed, 122 insertions(+)
create mode 100644 src/test/isolation/expected/session-variable.out
create mode 100644 src/test/isolation/specs/session-variable.spec
diff --git a/src/test/isolation/expected/session-variable.out
b/src/test/isolation/expected/session-variable.out
new file mode 100644
index 0000000000..d8c817f05a
--- /dev/null
+++ b/src/test/isolation/expected/session-variable.out
@@ -0,0 +1,87 @@
+Parsed test spec with 2 sessions
+
+starting permutation: let val drop val
+step let: LET myvar = 'test';
+step val: SELECT myvar;
+myvar
+-----
+test
+(1 row)
+
+step drop: DROP VARIABLE myvar;
+step val: SELECT myvar;
+ERROR: column or variable "myvar" does not exist
+
+starting permutation: let val s1 drop val sr1
+step let: LET myvar = 'test';
+step val: SELECT myvar;
+myvar
+-----
+test
+(1 row)
+
+step s1: BEGIN;
+step drop: DROP VARIABLE myvar;
+step val: SELECT myvar;
+ERROR: column or variable "myvar" does not exist
+step sr1: ROLLBACK;
+
+starting permutation: let val dbg drop create dbg val
+step let: LET myvar = 'test';
+step val: SELECT myvar;
+myvar
+-----
+test
+(1 row)
+
+step dbg: SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
+schema|name |removed
+------+-----+-------
+public|myvar|f
+(1 row)
+
+step drop: DROP VARIABLE myvar;
+step create: CREATE VARIABLE myvar AS text
+step dbg: SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
+schema|name |removed
+------+-----+-------
+public|myvar|t
+(1 row)
+
+step val: SELECT myvar;
+myvar
+-----
+
+(1 row)
+
+
+starting permutation: let val s1 dbg drop create dbg val sr1
+step let: LET myvar = 'test';
+step val: SELECT myvar;
+myvar
+-----
+test
+(1 row)
+
+step s1: BEGIN;
+step dbg: SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
+schema|name |removed
+------+-----+-------
+public|myvar|f
+(1 row)
+
+step drop: DROP VARIABLE myvar;
+step create: CREATE VARIABLE myvar AS text
+step dbg: SELECT schema, name, removed FROM
pg_debug_show_used_session_variables();
+schema|name |removed
+------+-----+-------
+public|myvar|t
+(1 row)
+
+step val: SELECT myvar;
+myvar
+-----
+
+(1 row)
+
+step sr1: ROLLBACK;
diff --git a/src/test/isolation/isolation_schedule
b/src/test/isolation/isolation_schedule
index 529a4cbd4d..96ecf93e32 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -107,3 +107,4 @@ test: cluster-conflict-partition
test: truncate-conflict
test: serializable-parallel
test: serializable-parallel-2
+test: session-variable
diff --git a/src/test/isolation/specs/session-variable.spec
b/src/test/isolation/specs/session-variable.spec
new file mode 100644
index 0000000000..a138f0449e
--- /dev/null
+++ b/src/test/isolation/specs/session-variable.spec
@@ -0,0 +1,34 @@
+#
+
+setup
+{
+ CREATE VARIABLE myvar AS text;
+}
+
+teardown
+{
+ DROP VARIABLE IF EXISTS myvar;
+}
+
+session s1
+step s1 { BEGIN; }
+step let { LET myvar = 'test'; }
+step val { SELECT myvar; }
+step dbg { SELECT schema, name, removed FROM
pg_debug_show_used_session_variables(); }
+step sr1 { ROLLBACK; }
+
+
+session s2
+step drop { DROP VARIABLE myvar; }
+step create { CREATE VARIABLE myvar AS text }
+
+# Concurrent drop of a known variable should lead to an error
+permutation let val drop val
+# Same, but with an explicit transaction
+permutation let val s1 drop val sr1
+# Concurrent drop/create of a known variable should lead to empty variable
+permutation let val dbg drop create dbg val
+# Concurrent drop/create of a known variable should lead to empty variable
+# We need a transaction to make sure that we won't accept invalidation when
+# calling the dbg step after the concurrent drop
+permutation let val s1 dbg drop create dbg val sr1
--
2.37.0
