MIME-Version: 1.0
References: <19519-fe02d8ff679d834d@postgresql.org>
 <CAFC+b6rLNvDzzYxrfLdq7CWbbouHcuOxi-mjEOW1r3qy2uAPMQ@mail.gmail.com>
 <CA+UBfan1QukfNDPvb2YpPG-ME4vUduBfbbe5uWptXDryZbtwVA@mail.gmail.com>
 <aktM919ANQGYZcsj@paquier.xyz>
 <CAEze2WhCRX86ZY2QN7vuGc3wsAyOJbArFSc0i5KtMVnsguA7Kw@mail.gmail.com>
 <akw336Xpi9Ck9-Oz@paquier.xyz>
In-Reply-To: <akw336Xpi9Ck9-Oz@paquier.xyz>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Tue, 7 Jul 2026 01:51:32 +0200
Message-ID: 
 <CAEze2WgBjhf6KTKuJ-UqHHGj9Z66f62grmJgZ1EazvMH9HU+XA@mail.gmail.com>
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
To: Michael Paquier <michael@paquier.xyz>
Cc: Imran Zaheer <imran.zhir@gmail.com>,
 Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
 Konstantin Knizhnik <knizhnik@garret.ru>
Content-Type: multipart/alternative; boundary="000000000000dcc9560655f9f49e"
Archived-At: 
 <https://www.postgresql.org/message-id/CAEze2WgBjhf6KTKuJ-UqHHGj9Z66f62grmJgZ1EazvMH9HU%2BXA%40mail.gmail.com>
Precedence: bulk

--000000000000dcc9560655f9f49e
Content-Type: text/plain; charset="UTF-8"

On Tue, 7 Jul 2026, 01:19 Michael Paquier, <michael@paquier.xyz> wrote:

> On Mon, Jul 06, 2026 at 08:21:41PM +0200, Matthias van de Meent wrote:
> > So, in effect, this is an issue that's quite similar to the CIC/RIC
> > bug of PG14<14.4, except that
> >
> > 1.) in this case it's SnapshotAny instead of a
> > registered-but-invisible MVCC snapshot;
> > 2.) the issue in this case only happens when toast pointers were
> > cleaned up and then are dereferenced, rather than HOT chains updated
> > and reclaimed since the scan started; and finally
> > 3.) this issue dates back a very long time, likely since TOAST, but
> > defininitely since PG14 introduced the GlobalVisTestFor apis.
>
> You mean 042b584c7f7d, revert of d9d076222f5b.  That rings a bell.
>

Indeed


> > So I think the only way to fix this is either
> > 1.) we update every maintenance process that makes use of SnapshotAny
> > and could access TOAST tables to handle errors caused by missing TOAST
> > data when the base relation's tuple is RECENTLY_DEAD, and treat those
> > as "tuple cleanup has already started, we just weren't aware of it
> > yet, so ignore this tuple" (as I mentioned in [1]), or
>
> That seems like an option worth exploring to me, but I doubt that
> we'll be able to get something that can be backpatched due to how
> invasive it is.  I strongly suspect that it would require at least one
> table AM change to cope with the fact that we want to let the callers
> be OK with TOAST chunks missing in some contexts when grabbing a toast
> slice.  That's perhaps for the best if we think about potential
> regressions, this is scary and old enough that we may still be OK by
> living without a backpatch.
>

I suspect that for indexing we're lucky, in that we have full control
internally over which tuple data get fed into IndexBuildCallback. We can
make sure only fully detoasted attributes get passed on, and can be
implemented with some (a lot) of TRY/CATCH work in or around
FormIndexDatum. That said, I don't think that's very nice, and I'm also a
bit concerned about leaking things (memory, buffers, ...) in such an
approach.

For REPACK/VACUUM FULL/CLUSTER, I think we can do something similar, given
that this code too has a central loop processing the tuple data, though I
suspect it'll need a bit more work than the reindex one.

> 2.) we set the xmin of the CI/R backend to the current OldestXmin for
> > the relation(s) we're currently processing, and so block all cleanup
> > of the relevant (toast) data for the duration of this statement or
> > whenever we check for a newer xmin for the relation (whilst ignoring
> > our current one).
>
> Err, I don't think that would be safe anyway?  It's rather uncommon in
> practice in schemas, but we could trigger function calls, like index
> expressions, domains, whatever, that access data of other tables while
> processing one table with a SnapshotAny.  The CIC revert of 14.4 was
> about such cases.
>

True, but in indexes that is forbidden: mislabeling of non-INMUTABLE
functions causes broken indexes every day, and we can't be expected to make
it work there. Similarly, for REPACK it shouldn't evaluate any
non-indexable expressions, right?

Either way, if we can register a snapshot in the REPACK/INDEX backend whose
xmin is (*correctly*) registered with the OldestNonRemovableTransactionId()
of the relation, then concurrent cleanup won't happen for toast, and we'd
also avoid dereferencing those dangling toast pointers. We won't need to
actually use that snapshot for any scans; it'll avoid the issue as long as
our backend advertises an xmin <= the OldestXmin used in maintenance to
hold back pruning/vacuum on the relation.

(we can adjust that xmin every once in a while with a newer
OldestNonRemovableTransactionId (excl. our own backend) as long as we also
adjust the OldestXmin cutoff for the SnapshotAny visibility tests, but I
don't suggest to backpatch that even newer system, it'd add significant
complications to an already complicated system)


- Matthias

ps. sorry for the formatting, I'm writing this on my phone.

--000000000000dcc9560655f9f49e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"auto"><div><br><br><div class=3D"gmail_quote gmail_quote_contai=
ner"><div dir=3D"ltr" class=3D"gmail_attr">On Tue, 7 Jul 2026, 01:19 Michae=
l Paquier, &lt;<a href=3D"mailto:michael@paquier.xyz">michael@paquier.xyz</=
a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0p=
x 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On=
 Mon, Jul 06, 2026 at 08:21:41PM +0200, Matthias van de Meent wrote:<br>
&gt; So, in effect, this is an issue that&#39;s quite similar to the CIC/RI=
C<br>
&gt; bug of PG14&lt;14.4, except that<br>
&gt;<br>
&gt; 1.) in this case it&#39;s SnapshotAny instead of a<br>
&gt; registered-but-invisible MVCC snapshot;<br>
&gt; 2.) the issue in this case only happens when toast pointers were<br>
&gt; cleaned up and then are dereferenced, rather than HOT chains updated<b=
r>
&gt; and reclaimed since the scan started; and finally<br>
&gt; 3.) this issue dates back a very long time, likely since TOAST, but<br=
>
&gt; defininitely since PG14 introduced the GlobalVisTestFor apis.<br>
<br>
You mean 042b584c7f7d, revert of d9d076222f5b.=C2=A0 That rings a bell.<br>=
</blockquote></div></div><div dir=3D"auto"><br></div><div dir=3D"auto">Inde=
ed</div><div dir=3D"auto"><br></div><div dir=3D"auto"><div class=3D"gmail_q=
uote gmail_quote_container"><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1e=
x">
<br>
&gt; So I think the only way to fix this is either<br>
&gt; 1.) we update every maintenance process that makes use of SnapshotAny<=
br>
&gt; and could access TOAST tables to handle errors caused by missing TOAST=
<br>
&gt; data when the base relation&#39;s tuple is RECENTLY_DEAD, and treat th=
ose<br>
&gt; as &quot;tuple cleanup has already started, we just weren&#39;t aware =
of it<br>
&gt; yet, so ignore this tuple&quot; (as I mentioned in [1]), or<br>
<br>
That seems like an option worth exploring to me, but I doubt that<br>
we&#39;ll be able to get something that can be backpatched due to how<br>
invasive it is.=C2=A0 I strongly suspect that it would require at least one=
<br>
table AM change to cope with the fact that we want to let the callers<br>
be OK with TOAST chunks missing in some contexts when grabbing a toast<br>
slice.=C2=A0 That&#39;s perhaps for the best if we think about potential<br=
>
regressions, this is scary and old enough that we may still be OK by<br>
living without a backpatch.<br></blockquote></div></div><div dir=3D"auto"><=
br></div><div dir=3D"auto">I suspect that for indexing we&#39;re lucky, in =
that we have full control internally over which tuple data get fed into Ind=
exBuildCallback. We can make sure only fully detoasted attributes get passe=
d on, and=C2=A0can be implemented=C2=A0with some (a lot) of TRY/CATCH work =
in or around FormIndexDatum. That said, I don&#39;t think that&#39;s very n=
ice, and I&#39;m also a bit concerned about leaking things (memory, buffers=
, ...) in such an approach.</div><div dir=3D"auto"><br></div><div dir=3D"au=
to">For REPACK/VACUUM FULL/CLUSTER, I think we can do something similar, gi=
ven that this code too has a central loop processing the tuple data, though=
 I suspect it&#39;ll need a bit more work than the reindex one.</div><div d=
ir=3D"auto"><br></div><div dir=3D"auto"><div class=3D"gmail_quote gmail_quo=
te_container"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px=
 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
&gt; 2.) we set the xmin of the CI/R backend to the current OldestXmin for<=
br>
&gt; the relation(s) we&#39;re currently processing, and so block all clean=
up<br>
&gt; of the relevant (toast) data for the duration of this statement or<br>
&gt; whenever we check for a newer xmin for the relation (whilst ignoring<b=
r>
&gt; our current one).<br>
<br>
Err, I don&#39;t think that would be safe anyway?=C2=A0 It&#39;s rather unc=
ommon in<br>
practice in schemas, but we could trigger function calls, like index<br>
expressions, domains, whatever, that access data of other tables while<br>
processing one table with a SnapshotAny.=C2=A0 The CIC revert of 14.4 was<b=
r>
about such cases.<br></blockquote></div></div><div dir=3D"auto"><br></div><=
div dir=3D"auto">True, but in indexes that is forbidden: mislabeling of non=
-INMUTABLE functions causes broken indexes every day, and we can&#39;t be e=
xpected to make it work there. Similarly, for REPACK it shouldn&#39;t evalu=
ate any non-indexable expressions, right?</div><div dir=3D"auto"><br></div>=
<div dir=3D"auto">Either way, if we can register a snapshot in the REPACK/I=
NDEX backend whose xmin is (*correctly*) registered with the OldestNonRemov=
ableTransactionId() of the relation, then concurrent cleanup won&#39;t happ=
en for toast, and we&#39;d also avoid dereferencing those dangling toast po=
inters. We won&#39;t need to actually use that snapshot for any scans; it&#=
39;ll avoid the issue as long as our backend advertises an xmin &lt;=3D the=
 OldestXmin used in maintenance to hold back pruning/vacuum on the relation=
.</div><div dir=3D"auto"><br></div><div dir=3D"auto">(we can adjust that xm=
in every once in a while with a newer OldestNonRemovableTransactionId (excl=
. our own backend) as long as we also adjust the OldestXmin cutoff for the =
SnapshotAny visibility tests, but I don&#39;t suggest to backpatch that eve=
n newer system, it&#39;d add significant complications to an already compli=
cated system)</div><div dir=3D"auto"><br></div><div dir=3D"auto"><br></div>=
<div dir=3D"auto">- Matthias</div><div dir=3D"auto"><br></div><div dir=3D"a=
uto">ps. sorry for the formatting, I&#39;m writing this on my phone.</div><=
/div>

--000000000000dcc9560655f9f49e--