Re: Unify parallel worker handling for index builds and instrumentation

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

In response to

Browse pgsql-hackers by date

  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)