From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: Andres Freund <andres@anarazel.de>, a.wicht@gmail.com,
        pgsql-bugs@lists.postgresql.org,
        Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Subject: Re: BUG #15990: PROCEDURE throws "SQL Error [XX000]: ERROR: no known
 snapshots" with PostGIS geometries
In-reply-to: <957186.1620784871@sss.pgh.pa.us>
References: <15990-eee2ac466b11293d@postgresql.org>
 <20190904105618.j5l6fhyesmprmstf@alap3.anarazel.de>
 <912290.1620774094@sss.pgh.pa.us> <957186.1620784871@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 11 May 2021 22:01:11 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1032523.1620789939.0@sss.pgh.pa.us>
Date: Tue, 11 May 2021 23:25:57 -0400
Message-ID: <1032535.1620789957@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1032535.1620789957%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1032523.1620789939.1@sss.pgh.pa.us>

I wrote:
> I think the bigger-picture question is, if we're trying to detoast
> as the first step in a new transaction of a procedure, where's the
> guarantee that the TOAST data still exists to be fetched?  For sure
> we aren't holding any locks that would stop VACUUM from reclaiming
> recently-dead TOAST rows.

Yeah.  So here's a patch I actually believe in to some extent, based
on Konstantin's idea.

> I'm still wondering why plpgsql-toast.spec is failing to show the
> problem, too.

The answer to that is that it's not testing commit-inside-a-cursor-loop,
and if it were, it'd still fail to show the problem because its test
table contains just one row.

Interestingly, if you try the test case added here without adding the
code patch, you get a "missing chunk number ... for toast value ..."
error, not "no known snapshots".  I think that's because the test
case has additional commands after the COMMIT, causing the transaction
snapshot to get re-established.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-commit-inside-cursor-loop.patch";
	charset="us-ascii"
Content-ID: <1032523.1620789939.2@sss.pgh.pa.us>
Content-Description: fix-commit-inside-cursor-loop.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b4c70aaa7f..27d6ea7551 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5826,6 +5826,17 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_s=
tmt_forq *stmt,
 	 */
 	PinPortal(portal);
 =

+	/*
+	 * In a non-atomic context, we dare not prefetch, even if it would
+	 * otherwise be safe.  Aside from any semantic hazards that that might
+	 * create, if we prefetch toasted data and then the user commits the
+	 * transaction, the toast references could turn into dangling pointers.
+	 * (Rows we haven't yet fetched from the cursor are safe, because the
+	 * PersistHoldablePortal mechanism handles this scenario.)
+	 */
+	if (!estate->atomic)
+		prefetch_ok =3D false;
+
 	/*
 	 * Fetch the initial tuple(s).  If prefetching is allowed then we grab a
 	 * few more rows to avoid multiple trips through executor startup
diff --git a/src/test/isolation/expected/plpgsql-toast.out b/src/test/isol=
ation/expected/plpgsql-toast.out
index fc557da5e7..4f216b94b6 100644
--- a/src/test/isolation/expected/plpgsql-toast.out
+++ b/src/test/isolation/expected/plpgsql-toast.out
@@ -192,3 +192,46 @@ pg_advisory_unlock
 t              =

 s1: NOTICE:  length(r) =3D 6002
 step assign5: <... completed>
+
+starting permutation: lock assign6 vacuum unlock
+pg_advisory_unlock_all
+
+               =

+pg_advisory_unlock_all
+
+               =

+step lock: =

+    SELECT pg_advisory_lock(1);
+
+pg_advisory_lock
+
+               =

+step assign6: =

+do $$
+  declare
+    r record;
+  begin
+    insert into test1 values (2, repeat('bar', 3000));
+    insert into test1 values (3, repeat('baz', 4000));
+    for r in select test1.b from test1 loop
+      delete from test1;
+      commit;
+      perform pg_advisory_lock(1);
+      raise notice 'length(r) =3D %', length(r::text);
+    end loop;
+  end;
+$$;
+ <waiting ...>
+step vacuum: =

+    VACUUM test1;
+
+step unlock: =

+    SELECT pg_advisory_unlock(1);
+
+pg_advisory_unlock
+
+t              =

+s1: NOTICE:  length(r) =3D 6002
+s1: NOTICE:  length(r) =3D 9002
+s1: NOTICE:  length(r) =3D 12002
+step assign6: <... completed>
diff --git a/src/test/isolation/specs/plpgsql-toast.spec b/src/test/isolat=
ion/specs/plpgsql-toast.spec
index fe7090addb..bd2948b3d3 100644
--- a/src/test/isolation/specs/plpgsql-toast.spec
+++ b/src/test/isolation/specs/plpgsql-toast.spec
@@ -112,6 +112,25 @@ do $$
 $$;
 }
 =

+# committing inside a cursor loop presents its own hazards
+step "assign6"
+{
+do $$
+  declare
+    r record;
+  begin
+    insert into test1 values (2, repeat('bar', 3000));
+    insert into test1 values (3, repeat('baz', 4000));
+    for r in select test1.b from test1 loop
+      delete from test1;
+      commit;
+      perform pg_advisory_lock(1);
+      raise notice 'length(r) =3D %', length(r::text);
+    end loop;
+  end;
+$$;
+}
+
 session "s2"
 setup
 {
@@ -135,3 +154,4 @@ permutation "lock" "assign2" "vacuum" "unlock"
 permutation "lock" "assign3" "vacuum" "unlock"
 permutation "lock" "assign4" "vacuum" "unlock"
 permutation "lock" "assign5" "vacuum" "unlock"
+permutation "lock" "assign6" "vacuum" "unlock"

------- =_aaaaaaaaaa0--