Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements

Hello, Matthias!

Thanks for the review!

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

Fixed. I have also run pgindent over stir.c/stir.h
and added the STIR typedefs to typedefs.list.

> > + 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).

STIR indexes are intentionally always created unlogged, regardless of
the persistence of the table - an auxiliary index has no reason to
survive a crash. Added some comments about it, including the relpersistence
parameter of index_create.

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

Yes, made that error.

> 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).

Nice catch!

Fixed by writing the metapage through bulk-write (like btbuildempty
does), which WAL-logs and syncs init-fork pages cregardless of
relation persistence. Also added a TAP test for that.

> 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

Yes, I'll revisit it later in v38. Also, rd_amcache may be helpfull here.
For now I haven't seen it as a hot spot in any benchmarks I was running.

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

Done.

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

Removed, some stale artifact.

> 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.

Done.

> I don't see it being used in any non-false manner in this patch; maybe
> postpone this field to 0003?

Moved to 0004 - the first patch that actually uses it.

> And put it ahead of ii_ParallelWorkers,
> it has padding bytes available that otherwise would go unused.

Done.

> 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.

Done.

> 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 call

Done, but perhaps too much in an AI-driven way; I will revalidate later.

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

Rebased everything.

0007 probably shares some complaints about snapshot resetting as in
your previous review - I'll address those complaints first in another patch.

Best regards,
Mikhail.