MIME-Version: 1.0
References: 
 <CA+HiwqF4C0ws3cO+z5cLkPuvwnAwkSp7sfvgGj3yQ=Li6KNMqA@mail.gmail.com>
 <CA+HiwqGM6nvAV5O+=Nr+BXMPWOma0oeCRVzVP0XiLE8zX5TVAg@mail.gmail.com>
 <CA+HiwqGMaovCUgDbGxVGnK0Mrivr+ph3YE2Ws+47-ugyPb4f7g@mail.gmail.com>
 <CAFj8pRDaiBe_GOLk_yyYHTtPiDAAaLOM8u1-=Q3ZgXBTH+1igg@mail.gmail.com>
 <CA+HiwqGA5Ay_MR0eJEEbt4j6WrVh4F+AasTp8yCbs5aJLOJn6Q@mail.gmail.com>
 <CAEG8a3JM=NoqiTK0V6S9FNxZPvy1+C5F7rfafTtPKBVWnunL-g@mail.gmail.com>
 <CA+HiwqEyiLCY6MTLbOJXDdLNNQLaURYHvdW797MQgbjEK9od4Q@mail.gmail.com>
 <CAEG8a3+VBpwPf1Rm-ECD90whM9b3YnGhux5CVXdsL6khiBfzRQ@mail.gmail.com>
 <CA+HiwqF2UHzF0sKCp-F2a-U29rqh_9ZPy=f1h+Fh_=M8efj3pg@mail.gmail.com>
 <CAEG8a3L9Ew-WL8sxLROVOcypeaENPmd8qCmMvz4geoGL1TDGCA@mail.gmail.com>
 <CAEG8a3+nUFQo4sdPQF9xy0J73J8RFJ5U9A5+_kMosGDaZ+1sXA@mail.gmail.com>
 <2BE661BA-D909-4093-BF78-DB9B0C099337@gmail.com>
 <CAEG8a3JyKdizWvYsF+z_mA1BKy=dpW11iKVMOG=bk6Tbz6M1Bw@mail.gmail.com>
 <CAEG8a3+Hf4tvvbts29_k_AFhWQmRYfEo_SW4C5FY_140iKghBw@mail.gmail.com>
 <CA+HiwqFV-PY-3BxM6j5TaAiC3AwedDxo-6vwRSbvygg3zF+xAQ@mail.gmail.com>
 <CA+HiwqHpaisS-e+0gAgzh92qZAFxncAJMmmTRZZN=efoeTPgOg@mail.gmail.com>
 <CA+HiwqFwZ6WLRbY8R7VC7JVp5Jot6ktZOkr9wDxTjoK=W1SgdQ@mail.gmail.com>
 <CA+HiwqF_Kz=R8juHJBiOATvabWSOugK4VaGOxoJ_n=E2c7UM9w@mail.gmail.com>
 <CA+HiwqHCB7kcbspkhaLN9enoj5x=ehzhFM4PXDgWUUP8Px41GA@mail.gmail.com>
 <77FA04FE-1F84-4DA1-8855-8BBFD8CC889A@gmail.com>
 <CA+HiwqHpVtP485wEKuXdOkdoZWhvVvfFH40-og07Jp3MPx21eg@mail.gmail.com>
 <CAEG8a3JWHkJSXe9nNcVK7wnYKUEqWuMGFDhy5BynB_9tEjmEUg@mail.gmail.com>
 <CA+HiwqFjfumKrWy03q5M309xJJVYt0WgGfH6AZ8BjFhSwppwsQ@mail.gmail.com>
In-Reply-To: 
 <CA+HiwqFjfumKrWy03q5M309xJJVYt0WgGfH6AZ8BjFhSwppwsQ@mail.gmail.com>
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Tue, 31 Mar 2026 23:54:29 +0800
Message-ID: 
 <CAEG8a3JjP1LaKSv-r3AMJLRyLMzENJrKshWsDvDouMPM_sizmA@mail.gmail.com>
Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3
To: Amit Langote <amitlangote09@gmail.com>
Cc: Chao Li <li.evan.chao@gmail.com>, Haibo Yan <tristan.yim@gmail.com>,
	Pavel Stehule <pavel.stehule@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Tomas Vondra <tomas@vondra.me>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAEG8a3JjP1LaKSv-r3AMJLRyLMzENJrKshWsDvDouMPM_sizmA%40mail.gmail.com>
Precedence: bulk

Hi Amit,

On Tue, Mar 31, 2026 at 8:15=E2=80=AFPM Amit Langote <amitlangote09@gmail.c=
om> wrote:
>
> On Tue, Mar 31, 2026 at 7:57=E2=80=AFPM Junwang Zhao <zhjwpku@gmail.com> =
wrote:
> > On Tue, Mar 31, 2026 at 5:17=E2=80=AFPM Amit Langote <amitlangote09@gma=
il.com> wrote:
> > > On Tue, Mar 31, 2026 at 6:09=E2=80=AFPM Chao Li <li.evan.chao@gmail.c=
om> wrote:
> > > > > On Mar 30, 2026, at 19:15, Amit Langote <amitlangote09@gmail.com>=
 wrote:
> > > > > Kept looking at 0002 and found a couple of things to improve or c=
hange
> > > > > my thoughts about.  I decided to move the permission check from f=
ast
> > > > > path cache entry creation into ri_FastPathBatchFlush(), alongside=
 the
> > > > > snapshot, so that permission changes between flushes are respecte=
d
> > > > > rather than checked once at batch start; the check happens for ev=
ery
> > > > > row in the SPI and non-batched fast path.  Also, improved comment=
s in
> > > > > a few places to mention design decisions better.
> > > > >
> > > > > 0001 is mostly unchanged from v11 except I updated its commit mes=
sage
> > > > > to explain why only RI_FKey_check is covered and not the action
> > > > > triggers as the topic has come up in previous threads about this
> > > > > topic.
> > > > >
> > > > > Still planning to commit 0001 tomorrow.
> > > > >
> > > > > --
> > > > > Thanks, Amit Langote
> > > > > <v12-0001-Add-fast-path-for-foreign-key-constraint-checks.patch><=
v12-0002-Batch-FK-rows-and-use-SK_SEARCHARRAY-for-fast-pa.patch>
> > > >
> > > > Hi Amit,
> > > >
> > > > While reading the recent commits, I saw that 0001 has been pushed a=
s 2da86c1ef9b5446e0e22c0b6a5846293e58d98e3. However, I also just noticed a =
use-after-free issue in ri_LoadConstraintInfo(). It dereferences conForm af=
ter ReleaseSysCache(tup), which is unsafe. I am attaching a tiny patch to f=
ix that.
> > >
> > > Thanks.  I noticed that too and pushed the fix an hour ago:
> > >
> > > https://www.postgresql.org/message-id/E1w7U6V-002H6n-0o%40gemulon.pos=
tgresql.org
> > >
> > > --
> > > Thanks, Amit Langote
> >
> > prion is happy now, the fix works, thanks.
>
> Yep, good.
>
> Because I noticed a use-after-free with prion, I thought to check our
> preparedness for CLOBBER_CACHE_ALWAYS and found issues in both the
> committed patch (and similar code in 0002): riinfo going stale inside
> ri_FastPathCheck() after relation opens and dangling fpmeta pointer
> after riinfo invalidation.  0001 fixes those; I'll apply it tomorrow
> morning.

+       if (riinfo->fpmeta =3D=3D NULL)
+       {
+               /* Reload to ensure it's valid. */
+               riinfo =3D ri_LoadConstraintInfo(riinfo->constraint_id);

I was thinking of wrapping the reload in a conditional check like
`!riinfo->valid`, since `riinfo` can be valid even when `fpmeta =3D=3D NULL=
`.
However,  `if (riinfo->fpmeta =3D=3D NULL)` should rarely be true, so the
unconditional reload is harmless, and the code is cleaner.

+1 to the fix.

>
> 0002 is the rebased batching patch.

The change of RI_FastPathEntry from storing riinfo to fk_relid
makes sense to me. I'll do another review on 0002 tomorrow.

>
> --
> Thanks, Amit Langote


--=20
Regards
Junwang Zhao