Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements

On Mon, 29 Jun 2026 at 23:49, Mihail Nikalayeu
<mihailnikalayeu(at)gmail(dot)com> wrote:
>
> Hello!
>
> Just rebased.

Here a partial review. I'm ignoring 0001, as I don't think you suggest
it should be committed.

patch 0002:

"git am" correctly raised some whitespace warnings around spaces vs tabs:

$ git am ~/Downloads/v36-0002-Add-STIR-access-method-and-flags-related-to-auxi.patch
[...]
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.

> +++ b/src/backend/access/stir/stir.c
> + Assert(!RelationNeedsWAL(index));

Why is this so frequently asserted? STIR can be created on any
relation, and AFAIK indexes should have the same persistence settings
as their tables (I suspect assertions could fail if this isn't
maintained).

> +bool
> +stirvalidate(Oid opclassoid)
[...]

Shouldn't this assert() that there are no operators in the opclass/opfamily?

> +void stirbuildempty(Relation index)
> +{
> + StirInitMetapage(index, INIT_FORKNUM);
> +}

This doesn't WAL-log the data in INIT_FORKNUM, and I think that's
still required for non-temp relations without WAL (i.e. unlogged
relations, such as the STIR index).

> +bool
> +stirinsert(Relation index, Datum *values, bool *isnull,
> + ItemPointer ht_ctid, Relation heapRel,
> + IndexUniqueCheck checkUnique,
> + bool indexUnchanged,
> + struct IndexInfo *indexInfo)

The logic in this function effectively serializes all STIR insertions
with exclusive locks, because every insertion and new page requires an
exclusive lock on the same pages.

I think it'd be better if the insertions were distributed across a few
pages a few pages have been filled, to avoid this source of lock
contention. This would be especially useful for unlogged tables' {RE,
CREATE }INDEX CONCURRENTLY.

> +++ b/src/include/access/genam.h
> @@ -58,6 +58,7 @@ typedef struct IndexVacuumInfo
> bool estimated_count; /* num_heap_tuples is an estimate */
> int message_level; /* ereport level for progress messages */
> double num_heap_tuples; /* tuples remaining in heap */
> + bool validate_index; /* validating concurrently built index? */
> BufferAccessStrategy strategy; /* access strategy for reads */

Let's put the new boolean with the other booleans, so between
estimated_count and message_level, that'll save 8 bytes of padding.

> +++ b/src/include/access/reloptions.h

You add a new RELOPT_KIND_STIR variant to enum relopt_kind, but it's
never used. Do we need the new variant?

> +++ b/src/include/access/stir.h
[...]
> +/* Scan strategies */
> +#define STIR_NSTRATEGIES 1

This should be 0; we can't allow any operators to be registered to
this AM; they don't make sense when we don't carry data.

> +++ b/src/include/nodes/execnodes.h
> -
> + /* is auxiliary for concurrent index build? */
> + bool ii_Auxiliary;

I don't see it being used in any non-false manner in this patch; maybe
postpone this field to 0003? And put it ahead of ii_ParallelWorkers,
it has padding bytes available that otherwise would go unused.

> +++ b/src/test/regress/expected/opr_sanity.out
> @@ -2131,9 +2131,10 @@ FROM pg_opclass AS c1
> WHERE NOT EXISTS(SELECT 1 FROM pg_amop AS a1
> WHERE a1.amopfamily = c1.opcfamily
> AND binary_coercible(c1.opcintype, a1.amoplefttype));
> - opcname | opcfamily
> ----------+-----------
> -(0 rows)
> + opcname | opcfamily
> +----------+-----------
> + stir_ops | 5558
> +(1 row)

I agree with this diff, but you'll have to add an explanation in the
comment above the query, indicating why stir_ops should be allowed to
not have matching operators in its opclasses.

--------

0003

I think the `datum` case could benefit from defining different
functions for byval, sized-byref, and variable-byref types, because
it'll reduce the compiled size (and branching) of the callbacks.

--------

0004 doesn't apply anymore, and I'll be out until after the weekend,
so further reviews will arrive not until next week.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)