| From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
|---|---|
| To: | Nikolay Samokhvalov <nik(at)postgres(dot)ai> |
| Cc: | pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andrey Borodin <amborodin(at)acm(dot)org>, Kirk Wolak <wolakk(at)gmail(dot)com> |
| Subject: | Re: PG19: two RI fast-path issues found while testing the batching revert |
| Date: | 2026-09-11 09:25:28 |
| Message-ID: | CA+HiwqGAq2fqXDSOUzE-uv4-LNMDcu6zHCF8co6=aRBZaTnoQg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Sep 11, 2026 at 9:33 AM Nikolay Samokhvalov <nik(at)postgres(dot)ai> wrote:
>
> On Thu, Sep 10, 2026 at 4:41 PM Amit Langote wrote:
> > Looking at these now. The first issue is clearly a fast-path code
> > problem. The 2nd one interacts with the existing non-fast-path code so
> > I'll need to check if the bug predates fast-path.
>
> Thanks Amit. In case helpful, here are two proposed fixes, with
> regression tests.
>
> Built and tested with assertions; regression and isolation suites pass.
> An independent agent reviewed and tested both, catching a cleanup issue
> that's now fixed. I didn't have time to fully study the patches manually,
> but my harness tested them thoroughly.
Thanks, Nik. Attached are updated patches incorporating your fixes.
For 0001, SPI's FOR KEY SHARE also requires UPDATE privilege on at
least one column. I've used ExecCheckOneRelPerms() to cover that along
with column-level SELECT. The tests exercise both per-row and batched
checks, including rejection without UPDATE and acceptance with UPDATE
on an unrelated column.
For #2, I reproduced the stale cast cache on 18.6 by warming it with
an UPDATE of a committed row before replacing the cast. I've adjusted
the tests to use committed rows, since same-transaction rows bypass
the key comparison. The nested case now also uses UPDATE to exercise
the comparison cache on older branches.
The cleanup strategy in 0002 deserves some discussion. It retains
invalidated call information until transaction end because a cast can
invalidate the cache and re-enter RI checks while an outer comparison
still uses it. I've carried that approach into the backpatch, but this
means introducing AtEOXact_RI() on pre-19 branches. I'd welcome closer
review before settling on that strategy. Could we replace the dead
list and explicit cleanup with reparenting to TopTransactionContext
when an entry is invalidated? That would avoid the new hook, but needs
checking against invalidation timing.
There are separate versions of 0001 and 0002 for master and
REL_19_STABLE. The two versions of 0002 contain the same fix and
tests, adapted to each branch's surrounding code. A shared version of
0002 applies to branches 14 through 18, which have no fast-path code.
--
Thanks, Amit Langote
| Attachment | Content-Type | Size |
|---|---|---|
| master-v2-0001-Fix-RI-fast-path-permission-checks.patch | application/octet-stream | 12.0 KB |
| master-v2-0002-Invalidate-RI-call-information-when-casts-change.patch | application/octet-stream | 21.0 KB |
| REL_19_STABLE-v2-0001-Fix-RI-fast-path-permission-checks.patch | application/octet-stream | 11.6 KB |
| REL_19_STABLE-v2-0002-Invalidate-RI-call-information-when-casts-change.patch | application/octet-stream | 22.0 KB |
| PG14-18-v2-0001-Invalidate-RI-call-information-when-casts-change.patch | application/octet-stream | 17.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | vignesh C | 2026-09-11 09:43:13 | Re: Review items for EXCEPT TABLE publication |
| Previous Message | Álvaro Herrera | 2026-09-11 09:14:03 | Re: fixes for a few GUC descriptions |