Re: Unify parallel worker handling for index builds and instrumentation

On Mon, Jun 01, 2026 at 01:31:00PM -0500, Sami Imseih wrote:
>> 0001: Use the new Instrumentation struct to handle WAL and Buffer usage together
>
> This one is a straightforward refactor. Only comment is to remove the bare block
>
> - InstrEndParallelQuery(&buffer_usage[ParallelWorkerNumber],
> -
> &wal_usage[ParallelWorkerNumber]);
> + {
> + Instrumentation *worker_instr;
> +
> + worker_instr = shm_toc_lookup(toc,
> PARALLEL_KEY_INSTRUMENTATION, false);
> + InstrEndParallelQuery(&worker_instr[ParallelWorkerNumber]);
> + }

Instrumentation is a structure larger than WalUsage and BufferUsage
combined, and we don't care about all these other fields for the
parallel workers. Sure, this cuts code, but this also increases the
DSM and memory footprint.

0002 relies on 0001, so same mixed feeling regarding it.

>> 0003: New helpers to handle query text for parallel workers
>
> +/*
> + * Restore the query text in a worker: set debug_query_string and report it as
> + * the current activity. The key is looked up with missing_ok, so this is a
> + * no-op (leaving debug_query_string NULL) when the leader stored no text.
> + */
> +void
> +RestoreParallelQueryText(shm_toc *toc, uint64 key)
> +{
> + debug_query_string = shm_toc_lookup(toc, key, true);
> + pgstat_report_activity(STATE_RUNNING, debug_query_string);
> +}
>
> I went back-forth on this since it combines 2 different ideas, setting
> a debug_query_string and reporting activity. I think this is OK after
> thinking about it a bit, and the whole point is to deduplicate code.
>
> "no-op (leaving debug_query_string NULL) " The comment here is
> not accurate. I would drop the "no-op" part since it is setting a NULL
> query string. right?

Hmm. This one looks like it has nice advantages. For one, less
specific logic related to debug_query_string in all the main callbacks
of these parallel workers, copied in three different places.

>> 0004: Introduces shared parallel index build helper functions
>>
>> Of these, 0004 is where I have the most uncertainty personally (what's
>> a good abstraction here?), but wanted to share this to spark a
>> conversation on how we could improve this.
>
> I think this is a nice deduplication as well. So I am also +1 to this.

Less duplication with the levels of locks assigned and less
duplication regarding the levels of the snapshots retrieved makes this
approach quite beneficial in the long-term. That reads nice.
--
Michael