Skip to content

Let the delta decide that an index needs a concurrent build - #594

Open
erdtsieck wants to merge 1 commit into
JasperFx:masterfrom
erdtsieck:feat/concurrent-index-on-alter
Open

erdtsieck wants to merge 1 commit into
JasperFx:masterfrom
erdtsieck:feat/concurrent-index-on-alter

Conversation

@erdtsieck

Copy link
Copy Markdown
Contributor

The problem

Whether an index has to be built without blocking writes is not a property of the index. It is whether the table is being created — nothing to scan, and the statement belongs inline in the CREATE TABLE script — or altered, where there are rows to scan and ACCESS EXCLUSIVE for the whole build is a write outage rather than a migration.

Today that decision is made where the index is declared, through IsConcurrent, so every consumer has to anticipate it per index. Marten already carries BuildHStoreTagIndexConcurrently for one index on mt_events (JasperFx/marten#5268) and I was about to add a second flag for another (marten#5453) before it became obvious that a third index would want a third flag.

TableDelta already knows which of the two cases it is in.

What this does

PostgresqlMigrator.BuildIndexesConcurrentlyOnAlter. With it on, an index added to — or changed on — a table that already exists is built concurrently without anyone marking it:

// TableDelta.WriteUpdate
var concurrently = rules is PostgresqlMigrator { BuildIndexesConcurrentlyOnAlter: true };

foreach (var indexDefinition in Indexes.Missing)
    writer.WriteLine(indexDefinition.ToCreateSql(Expected, concurrently || indexDefinition.IsConcurrent));

The create path is deliberately untouched: WriteCreateStatement still writes what the index itself declares, so an empty table is not paid for with a concurrent build it cannot benefit from — and, on a partitioned parent, one PostgreSQL would refuse outright.

ToCreateSql(Table, bool) is the seam, with the caller's answer authoritative in both directions; ToCreateSql(Table) keeps taking it from IsConcurrent, so nothing changes for existing callers.

Why it is off by default

It changes what a generated patch script is, not just how fast it runs:

  • a concurrent build cannot run inside a transaction, so the script stops being runnable as one block (applying it through PostgresqlMigrator is unaffected — that already splits on the index-creation markers and each statement auto-commits);
  • a failed concurrent build leaves an invalid index behind that has to be dropped before a retry, where a transactional migration rolls back clean;
  • someone with a small table and a maintenance window may genuinely prefer the blocking build.

I would argue for on-by-default eventually — a blocking build against a live table is never what anyone wanted — but that is your call to make, not something to slip in. Say the word and I will flip it.

Tests

  • adding_an_index_to_an_existing_table — concurrent with the option on, blocking with it off, and the create path left blocking even with the option on.
  • Two additions to IndexDefinitionTests pinning that the concurrently argument decides in both directions.
  • Full Weasel.Postgresql.Tests suite green locally: 987 passed, 3 skipped, against PostgreSQL 17.

Knock-on, if you take it

BuildHStoreTagIndexConcurrently becomes redundant, and the docs that tell people to set it (Marten's docs/events/dcb.md) would want a pass. I have left both alone here — happy to follow up in Marten once you have decided what you want this to look like.

Whether an index has to be built without blocking writes is not a
property of the index. It is whether the table is being created --
nothing to scan, and the statement belongs inline in the CREATE TABLE
script -- or altered, where there are rows to scan and ACCESS EXCLUSIVE
for the whole build is a write outage. Today that decision is made where
the index is declared, through IsConcurrent, so every consumer has to
anticipate it per index: Marten carries BuildHStoreTagIndexConcurrently
for one index on mt_events and is about to add a second flag for
another.

TableDelta already knows which of the two it is. PostgresqlMigrator gets
BuildIndexesConcurrentlyOnAlter, and an index added to (or changed on) a
table that exists is then built concurrently without being marked. The
create path is untouched: WriteCreateStatement still writes what the
index itself declares, so an empty table is not paid for with a
concurrent build it cannot benefit from -- and, on a partitioned parent,
one PostgreSQL would refuse outright.

Off by default, because it changes what a generated patch script is and
not just how fast it runs: a concurrent build cannot run inside a
transaction, so the script stops being runnable as one block, and a
failed build leaves an invalid index that has to be dropped before a
retry. Applying through PostgresqlMigrator is unaffected -- it already
splits on the index-creation markers and each statement auto-commits.

ToCreateSql(Table, bool) is the seam, with the caller's answer
authoritative in both directions; ToCreateSql(Table) keeps taking it
from IsConcurrent.
erdtsieck added a commit to erdtsieck/marten that referenced this pull request Sep 15, 2026
EnableEventTypeIndex is recommended for large event stores -- its own
docs list "projection rebuilds time out" and "millions of events" as the
symptoms -- but it is added as a plain CREATE INDEX, which holds ACCESS
EXCLUSIVE on mt_events for the whole build. So the stores the option
exists for are exactly the ones that cannot afford to create it, and
under UseTenantPartitionedEvents there is no CREATE INDEX CONCURRENTLY
to fall back on by hand either: PostgreSQL refuses CONCURRENTLY on a
partitioned parent.

The optimizing page now says so, and the index name is a constant beside
HStoreTagIndexName, so the way out it documents -- ignore it and own it
-- does not need a string literal that has to be kept in step with
Marten.

Deliberately no third thing here. The first draft added a
BuildEventTypeIndexConcurrently flag mirroring
BuildHStoreTagIndexConcurrently, which would have been the second
per-index flag on one table; whether an index needs a concurrent build
is a property of the moment rather than of the index, and the delta is
where that is known. JasperFx/weasel#594 proposes it there instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant