Accept invalidation messages before the query starts inside a transaction
Hi,
During the PGDAY.Spain'2024 Yurii Rashkovskii found out boring behaviour
related to functions versioning:
If you open a transaction in one session and execute the function, then
replace this function with a new version or drop it in a concurrent
session; the first session doesn't see any changes: it will use the old
version of the function in the following query.
If you execute the function without explicit transaction, subsequent
query execution will employ the new version of the function.
It happens because of documented behaviour [1]https://www.postgresql.org/docs/16/mvcc-caveats.html, which doesn't guarantee
isolation levels for internal access to the system catalogues. But it
looks more consistent for a user to see changes the same way with and
without explicit transactions, doesn't it? At least, an extension
developer may be confident that after updating to a new version, no one
user will employ the old version of the extension's function and not
think about the consequences it may cause.
I don't know whether to classify this as a bug. The sketch of the patch
with an example isolation test is attached.
[1]: https://www.postgresql.org/docs/16/mvcc-caveats.html
--
regards, Andrei Lepikhov
Attachments:
0001-Accept-invalidation-messages-before-the-start-of-a-q.patchtext/plain; charset=UTF-8; name=0001-Accept-invalidation-messages-before-the-start-of-a-q.patchDownload
From 67b15acb4db396a24deab0efefb602227050ed63 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Wed, 11 Sep 2024 21:16:59 +0200
Subject: [PATCH] Accept invalidation messages before the start of a query in
transaction
---
src/backend/tcop/postgres.c | 3 +++
.../isolation/expected/function-replace.out | 20 ++++++++++++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/function-replace.spec | 26 +++++++++++++++++++
4 files changed, 50 insertions(+)
create mode 100644 src/test/isolation/expected/function-replace.out
create mode 100644 src/test/isolation/specs/function-replace.spec
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8bc6bea113..4ed8d6f786 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -73,6 +73,7 @@
#include "tcop/utility.h"
#include "utils/guc_hooks.h"
#include "utils/injection_point.h"
+#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -2774,6 +2775,8 @@ start_xact_command(void)
!get_timeout_active(CLIENT_CONNECTION_CHECK_TIMEOUT))
enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
client_connection_check_interval);
+
+ AcceptInvalidationMessages();
}
static void
diff --git a/src/test/isolation/expected/function-replace.out b/src/test/isolation/expected/function-replace.out
new file mode 100644
index 0000000000..b1393be4bf
--- /dev/null
+++ b/src/test/isolation/expected/function-replace.out
@@ -0,0 +1,20 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_exec_fn s2_replace s1_exec_fn s2_drop s1_exec_fn s1_end
+step s1_exec_fn: SELECT test_fn();
+test_fn
+-------
+ 1
+(1 row)
+
+step s2_replace: CREATE OR REPLACE FUNCTION test_fn() RETURNS integer LANGUAGE sql AS $$ SELECT 2; $$;
+step s1_exec_fn: SELECT test_fn();
+test_fn
+-------
+ 2
+(1 row)
+
+step s2_drop: DROP FUNCTION test_fn;
+step s1_exec_fn: SELECT test_fn();
+ERROR: function test_fn() does not exist
+step s1_end: END;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4d..91a8ad67a6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -115,3 +115,4 @@ test: serializable-parallel-2
test: serializable-parallel-3
test: matview-write-skew
test: lock-nowait
+test: function-replace
diff --git a/src/test/isolation/specs/function-replace.spec b/src/test/isolation/specs/function-replace.spec
new file mode 100644
index 0000000000..54ce156d29
--- /dev/null
+++ b/src/test/isolation/specs/function-replace.spec
@@ -0,0 +1,26 @@
+#
+# Accept invalidation messages before the start of a new query inside the
+# transaction
+#
+
+setup
+{
+ CREATE FUNCTION test_fn() RETURNS integer LANGUAGE sql AS $$ SELECT 1; $$;
+}
+
+session s1
+setup { BEGIN; }
+step s1_exec_fn { SELECT test_fn(); }
+step s1_end { END; }
+
+session s2
+step s2_drop { DROP FUNCTION test_fn; }
+step s2_replace { CREATE OR REPLACE FUNCTION test_fn() RETURNS integer LANGUAGE sql AS $$ SELECT 2; $$; }
+
+permutation
+ s1_exec_fn
+ s2_replace
+ s1_exec_fn
+ s2_drop
+ s1_exec_fn
+ s1_end
\ No newline at end of file
--
2.46.0
Andrei Lepikhov <lepihov@gmail.com> writes:
I don't know whether to classify this as a bug. The sketch of the patch
with an example isolation test is attached.
This seems like an extremely strange place (and an extremely
brute-force way) to insert an AcceptInvalidationMessages call.
Under what circumstances wouldn't we do one or more AIMs later
on, eg while acquiring locks?
regards, tom lane
On 12 Sep 2024, at 00:50, Andrei Lepikhov <lepihov@gmail.com> wrote:
It happens because of documented behaviour [1], which doesn't guarantee isolation levels for internal access to the system catalogues.
As far as I understood you are proposing not isolation guaranties, but allowed isolation anomaly.
I think we do not guaranty anomalies.
Best regards, Andrey Borodin.
On Wednesday, September 11, 2024, Andrei Lepikhov <lepihov@gmail.com> wrote:
I don't know whether to classify this as a bug.
Seems we need to add another sentence to that final paragraph. Something
like:
However, once an object is accessed within a transaction its definition is
cached for the duration of the transaction. Of particular note are routine
bodies modified using create or replace. If a replacement gets committed
mid-transaction the old code body will still be executed for the remainder
of the transaction.
David J.
On 12/9/2024 08:31, David G. Johnston wrote:
On Wednesday, September 11, 2024, Andrei Lepikhov <lepihov@gmail.com
<mailto:lepihov@gmail.com>> wrote:I don't know whether to classify this as a bug.
[1] https://www.postgresql.org/docs/16/mvcc-caveats.html
<https://www.postgresql.org/docs/16/mvcc-caveats.html>Seems we need to add another sentence to that final paragraph.
Something like:However, once an object is accessed within a transaction its definition
is cached for the duration of the transaction. Of particular note are
routine bodies modified using create or replace. If a replacement gets
committed mid-transaction the old code body will still be executed for
the remainder of the transaction.
I'm not sure we need any changes here yet unless Yurii provides an
example of a real issue. But for the record, let me explain the initial
reason in more detail.
Extensions have always been challenging to maintain because they include
hook calls from the core and stored procedures visible in databases
where the extension was created. At the same time, we should control the
UPDATE/DROP/CREATE extension commands. And don't forget, we have some
data in shared memory as well that can't be easily versioned.
Our primary goal is to establish a stable point that can provide
extension developers (and users who call their functions) with a
reliable guarantee that we are up to date with all the changes made to
the extension.
Last week, Michael Paquer's patch introduced sys caches and invalidation
callbacks to watch such actions. But curiously, while we just execute
some extension's functions like:
SELECT pg_extension.smth()
we don't lock any table, don't apply invalidation messages at all, don't
have an information about updating/altering/deletion of our extension's
objects!
--
regards, Andrei Lepikhov