| From: | Lukas Fittl <lukas(at)fittl(dot)com> |
|---|---|
| To: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Sami Imseih <samimseih(at)gmail(dot)com> |
| Subject: | Re: Unify parallel worker handling for index builds and instrumentation |
| Date: | 2026-10-31 20:21:03 |
| Message-ID: | CAP53PkxEZPeT_G7u-1qF-=ouBvMxUNka-bN7cJowkv7gms9uCg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue, Jun 2, 2026 at 5:38 PM Lukas Fittl <lukas(at)fittl(dot)com> wrote:
> Maybe for now I can re-order this patch series to lead with the other
> patches, and we could defer this part a bit until we have clarity what
> we're doing re: stack-based instrumentation.
>
> I'll respond on the other comments separately later this week. Thanks
> to you both for the quick feedback!
Well, that took a bit longer - whoops!
See attached v2:
Per prior discussion, to focus on the bigger win here, I've dropped
the original 0001 and 0002 patches for now (though thank you Sami for
reviewing) and might bring them back at a later time when some of the
instrumentation work goes in (or clearly doesn't). However, I've added
two new bug fix patches in their place:
New 0001 "Fix query ID reporting in parallel workers for GIN index
builds" - split out as Sami suggested.
New 0002 "Fix broken HOT chain handling in parallel BRIN and GIN index builds"
- I discovered this whilst thinking through how pgvector also had an
extra field that needed the ParallelIndexBuildShared mutex (see at the
end of this email) - I initially thought that both "havedead" and
"brokenhotchain" were B-tree specific, but I don't think that's
actually true - instead "brokenhotchain" / ii_BrokenHotChain is a
general purpose field that must be handled correctly by any index AM
that points at heap tuples
- This should be backpatched to 17 (parallel BRIN index builds) and 18
(parallel GIN index builds) respectively, since in my read it's an
actual bug: A transaction holding a snapshot from before the build
started could use the new BRIN/GIN index and get incorrect results
Per the discussion I've also flipped the order of the other two,
making 0003 "Unify parallel index build infrastructure", and 0004
"Unify parallel worker handling for query text".
New 0005 - "Unify parallel index build scan handling" - an additional
proposal of how we could more structurally prevent the issue that 0002
fixes by introducing a shared ParallelIndexBuildScan() helper.
I've also addressed the other minor feedback items on what are now
0003/0004, and additionally:
- Forward declare ParallelTableScanDescData in parallel_index_build.h
to avoid including access/relscan.h and storage/lockdefs.h
- Add a helper function ParallelIndexBuildAbort to deal with the abort
case in a consistent way
- Have ParallelIndexBuildCreateContext take a library_name argument,
matching CreateParallelContext, to allow extensions to use this
infrastructure more easily (see pgvector case below)
- Rework how lock modes are set, so the caller doesn't have to pass them back in
- Rework the query text logic to avoid making two strlen calls (we can
keep the pre-existing pattern with a local variable to store the
length from the estimation)
To put this to the test I've also sketched out how pgvector can update
to this infrastructure at [0], as an example of an out-of-core index
AM that supports parallel index builds. Some notes from that:
- Because which fields are updated under the ParallelIndexBuildShared
mutex isn't configurable, pgvector has to do an additional
SpinLockAcquire/SpinLockRelease for updating its "inertia" field
before calling ParallelIndexBuildReportScanDone (which will report the
other fields) -- I don't think that in itself is a problem (and don't
have a good idea to solve it), but we kind of have the same in-core
"quirk", which is why havedead is on ParallelIndexBuildShared despite
being B-Tree specific
- pgvector didn't report back worker WAL/buffer usage, and the
ParallelIndexBuildEnd function added here now requires that, which I
think is acceptable
- pgvector also suffered from the same queryid reporting bug as GIN,
which is fixed by using the new infrastructure
- pgvector also did not report back broken HOT chains encountered
during parallel index builds
As expected, this is a net code reduction for pgvector, when ignoring
the need to have older Postgres versions supported (the shim is ~330
lines that they'd have to carry for a few years).
Thanks,
Lukas
[0]: https://github.com/lfittl/pgvector/pull/1
--
Lukas Fittl
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-Fix-query-ID-reporting-in-parallel-workers-for-GI.patch | application/octet-stream | 2.2 KB |
| v2-0005-Unify-parallel-index-build-scan-handling.patch | application/octet-stream | 37.3 KB |
| v2-0002-Fix-broken-HOT-chain-handling-in-parallel-BRIN-an.patch | application/octet-stream | 10.7 KB |
| v2-0004-Unify-parallel-worker-handling-for-query-text.patch | application/octet-stream | 13.0 KB |
| v2-0003-Unify-parallel-index-build-infrastructure.patch | application/octet-stream | 51.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tom Lane | 2026-10-31 20:24:10 | Re: Reject wholerow references where ColumnRef is forbidden |
| Previous Message | Ishjaap Singh | 2026-10-31 20:04:10 | psql: Don't offer USING INDEX after REPACK (CONCURRENTLY) or (ANALYZE) |