diff --git a/docs/adr/0057-box-schema-versioning-and-migrations.md b/docs/adr/0057-box-schema-versioning-and-migrations.md
index af91e7c510..7dcc1fc022 100644
--- a/docs/adr/0057-box-schema-versioning-and-migrations.md
+++ b/docs/adr/0057-box-schema-versioning-and-migrations.md
@@ -223,7 +223,7 @@ The asymmetry across backends is documented per-catalog: MSSQL/MySQL/SQLite outb
| Backend | Strategy | Why no extra serialization |
|---|---|---|
| MSSQL | `IF NOT EXISTS` + `CREATE TABLE`; catch 2714 | Statement-terminating under default `XACT_ABORT OFF` — loser's tx stays usable |
-| PostgreSQL | `CREATE TABLE IF NOT EXISTS`; catch 23505 / 42P07 / 42710 | pg_class check and pg_type insert are not atomic |
+| PostgreSQL | `CREATE TABLE IF NOT EXISTS` inside a `SAVEPOINT`; catch 23505 / 42P07 / 42710 and `ROLLBACK TO SAVEPOINT` | pg_class check and pg_type insert are not atomic. Postgres marks the whole transaction aborted on the error even when it is caught, so without the savepoint the next statement fails with 25P02 |
| MySQL | `CREATE TABLE IF NOT EXISTS` | `MDL_EXCLUSIVE` metadata lock makes the statement atomic |
| SQLite | `CREATE TABLE IF NOT EXISTS` | `BEGIN IMMEDIATE` writer lock already serialises concurrent runners |
diff --git a/docs/adr/0060-multi-tenancy-migration-history-scope.md b/docs/adr/0060-multi-tenancy-migration-history-scope.md
index 1f4795081f..96b6f9c832 100644
--- a/docs/adr/0060-multi-tenancy-migration-history-scope.md
+++ b/docs/adr/0060-multi-tenancy-migration-history-scope.md
@@ -250,6 +250,12 @@ Structural changes precede behavioural ones, in separate commits (design princip
These corrections are reflected in `specs/0029-multi-tenancy-migrations/tasks.md` (S1, T3) and do not change the decision's substance (single resolver, opt-in scope, upgrade-safe default).
+**2026-10-12 (D5 as implemented, MSSQL and PostgreSQL):**
+
+3. **The seed runs on every `PerSchema` provision, not only when the per-schema history table is first created.** D5's "One-time" bullet gates the seed on a pre-create probe. That gate is wrong when more than one box shares the per-schema history table: the second box type to flip (e.g. the inbox after the outbox) finds the table already created by the first, skips its seed, and the bootstrap path then stamps a fresh `bootstrap: detected at V_latest` row — re-running the migration ledger and breaking FR5. The table-level gate is therefore dropped. The seed runs whenever the resolved history schema differs from the backend default; the per-row `NOT EXISTS` guard makes a steady-state run a zero-row `INSERT … SELECT`, and the cost is that one round-trip per provision. The consequence for the "Cross-schema read permission" risk is that read access to the legacy table is needed for the lifetime of the `PerSchema` deployment, not just at flip time.
+
+4. **A failure to read the legacy history table throws `ConfigurationException`.** Silently absorbing it would let an empty seed through, and the next provision would bootstrap-stamp a fresh row, again breaking FR5; the throw rolls back the surrounding transaction so no partial per-schema state is left. The catch is filtered to the provider's permission-denied codes (PostgreSQL `42501`; SQL Server `229`/`230`), and wraps both the legacy-existence probe and the seed `INSERT … SELECT`, so a deadlock, dropped connection or timeout propagates unchanged rather than being misreported as "grant SELECT on the legacy table".
+
## References
- Requirements: [specs/0029-multi-tenancy-migrations/requirements.md](../../specs/0029-multi-tenancy-migrations/requirements.md)
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlAdvisoryLock.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlAdvisoryLock.cs
index 38891da906..069ac332f8 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlAdvisoryLock.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlAdvisoryLock.cs
@@ -39,41 +39,24 @@ namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
/// The deadline is measured against an injectable ; the default
/// reads
/// (monotonic), so a wall-clock jump (NTP correction during a long lock wait, leap-second
-/// smear, container clock skew on VM resume) cannot collapse or extend the budget. Tests
-/// inject a fake provider to drive the deadline check deterministically.
+/// smear, container clock skew on VM resume) cannot collapse or extend the budget.
///
/// Lock-key derivation: the per-call key (typically BrighterMigration_<schema>.<table>
-/// from the runner) is combined with the Brighter namespace constant into the composite
-/// "74726:<lockKey>", hashed with SHA-256, and the first 8 bytes of the digest are read
-/// fixed big-endian (via BinaryPrimitives.ReadInt64BigEndian) into a signed long.
-/// That single 64-bit value is passed to the single-argument pg_try_advisory_lock(bigint) /
-/// pg_advisory_unlock(bigint) overloads; there is no hashtext call and the namespace
-/// is folded into the hash input rather than passed as a separate SQL argument. The birthday-bound
-/// collision probability across distinct lock keys is therefore ~1 in 2^64, hardened from the
-/// ~1 in 2^32 of the prior hashtext → int4 scheme and aligned with the 64-bit SHA-256
-/// principle the MySQL long-form fallback in MySqlMigrationLockName follows. Any collision
-/// merely serialises two migrations on a shared advisory lock (correctness preserved, only the
-/// concurrency boundary widens), and the per-deployment population is typically < 100 box tables.
-/// Fixing big-endian keeps the derived key identical on every host, OS, and culture (deterministic);
-/// see ADR 0062.
+/// from the runner) is prefixed with a Brighter namespace as "74726:<lockKey>", hashed
+/// with SHA-256, and the first 8 bytes of the digest are read big-endian into the signed
+/// bigint passed to pg_try_advisory_lock / pg_advisory_unlock. Reading
+/// big-endian keeps the key identical on every host. A collision between two lock keys
+/// (about 1 in 2^64) only serialises two migrations on one lock. See ADR 0062.
///
///
public class PostgreSqlAdvisoryLock : IPostgreSqlAdvisoryLock
{
- // Brighter-specific advisory-lock namespace, folded into the hash input by DeriveLockKey (the
- // composite prefix "74726:") rather than passed as a separate SQL argument. The single
- // 64-bit advisory-lock space is partitioned per box table by the lock-key string; the namespace
- // distinguishes Brighter's locks from any other application using the same composite scheme. The
- // numeric value is "BRIG" interpreted as ASCII (74726 decimal); not load-bearing — collision
- // with another application's choice is diagnostic not correctness.
+ // Prefixed to every lock key before hashing, to keep Brighter's advisory locks apart from
+ // other applications' (ADR 0062).
private const int BRIGHTER_LOCK_NAMESPACE = 74726;
private readonly TimeProvider _timeProvider;
- // Derives the 64-bit advisory-lock key for a Brighter lock key. The namespace is folded into a
- // composite ("74726:"), hashed with SHA-256, and the first 8 bytes are read fixed
- // big-endian into a signed long (ADR 0062). Both AcquireAsync and ReleaseAsync route through
- // this single helper, so they always derive byte-identical keys for the same lockKey.
private static long DeriveLockKey(string lockKey)
{
var composite = $"{BRIGHTER_LOCK_NAMESPACE}:{lockKey}";
@@ -85,8 +68,7 @@ private static long DeriveLockKey(string lockKey)
/// Initialises a new .
///
/// Source of timestamps for the acquisition deadline. Defaults
- /// to (monotonic). Tests pass
- /// FakeTimeProvider from Microsoft.Extensions.TimeProvider.Testing.
+ /// to (monotonic).
public PostgreSqlAdvisoryLock(TimeProvider? timeProvider = null)
{
_timeProvider = timeProvider ?? TimeProvider.System;
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxDetectionHelper.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxDetectionHelper.cs
index fb824d08a5..5258087111 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxDetectionHelper.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxDetectionHelper.cs
@@ -55,14 +55,8 @@ public class PostgreSqlBoxDetectionHelper :
private readonly ILogger _logger;
- ///
- /// Resolves and quotes the schema that physically holds the history table for a read: null
- /// (Global default) → "public"; otherwise the configured SchemaName supplied by the
- /// runner under . Folded via
- /// so it matches the case the runner created, after
- /// Identifiers.AssertSafe validation since the result is interpolated into the query
- /// text (no parameterisation of identifiers in SQL).
- ///
+ // Null means the Global default, "public". Quoting folds the name to the case the runner
+ // created it with; it is validated first because it is interpolated into the SQL.
private static string QuotedHistorySchema(string? historySchema)
{
var schema = historySchema ?? DefaultSchemaName;
@@ -72,10 +66,7 @@ private static string QuotedHistorySchema(string? historySchema)
///
/// Initialises the detection helper with an optional logger. When unspecified, falls back
- /// to . Existing callers that use the
- /// parameterless form continue to work — the logger is currently only consumed for the
- /// rare UndefinedTable-swallow Debug emission in
- /// ; the helper remains a safe DI singleton.
+ /// to .
///
public PostgreSqlBoxDetectionHelper(ILogger? logger = null)
{
@@ -85,7 +76,7 @@ public PostgreSqlBoxDetectionHelper(ILogger? logger = null)
///
/// Returns true if a table with the given name exists in the given schema.
///
- /// Optional. Null is substituted with "public" per ADR 0057 §A.1.
+ /// Optional. Null is substituted with "public".
public async Task DoesTableExistAsync(
NpgsqlConnection connection, string tableName, string? schemaName,
CancellationToken cancellationToken = default,
@@ -96,18 +87,9 @@ public async Task DoesTableExistAsync(
command.CommandText = @"
SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
WHERE TABLE_SCHEMA = @SchemaName AND TABLE_NAME = @TableName)";
- // information_schema.tables stores PG-folded (lowercase) names. Normalize so a
- // mixed-case configured value (e.g. default "Outbox") matches the stored "outbox".
- // The internal __BrighterMigrationHistory system-table existence check does NOT
- // route through this method — its CREATE DDL quotes the name and therefore
- // case-preserves it in pg_class, so DoesHistoryExistAsync inlines a literal-name
- // lookup that bypasses the fold normalization done here.
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
command.Parameters.AddWithValue("@TableName", PgIdentifier.Normalize(tableName));
- // Pattern-match rather than (bool)raw! so a driver returning null surfaces as a named
- // InvalidOperationException instead of a bare NullReferenceException — Npgsql has
- // returned null from ExecuteScalarAsync under server-side errors in the wild.
var raw = await command.ExecuteScalarAsync(cancellationToken);
return raw is bool b
? b
@@ -120,47 +102,25 @@ SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
/// given box table.
///
/// Optional. The box-table schema used to filter history rows. Null
- /// is substituted with "public" per ADR 0057 §A.1.
+ /// is substituted with "public".
/// Optional. The physical schema holding the history table; null
- /// resolves to "public" (today's behaviour).
+ /// resolves to "public", the Global default.
public async Task DoesHistoryExistAsync(
NpgsqlConnection connection, string tableName, string? schemaName, string? historySchema,
CancellationToken cancellationToken = default,
NpgsqlTransaction? transaction = null)
{
- // The physical history schema: Global → "public" (today's behaviour); PerSchema → the
- // configured SchemaName passed by the runner. Both sides fold the same way (Normalize for
- // the information_schema param, Quote for the qualified table ref) so a mixed-case configured
- // value reaches the same physical schema the runner created.
var foldedHistorySchema = PgIdentifier.Normalize(historySchema ?? DefaultSchemaName);
var quotedHistorySchema = QuotedHistorySchema(historySchema);
- // System-table existence: __BrighterMigrationHistory is created via quoted DDL
- // (`CREATE TABLE IF NOT EXISTS ."__BrighterMigrationHistory"`), so its name
- // is case-PRESERVED in pg_class. Routing through DoesTableExistAsync would lowercase
- // the lookup to `'__brightermigrationhistory'` and miss. Inline the literal-name check,
- // parameterising only the (folded) schema.
- using (var existsCmd = connection.CreateCommand())
- {
- if (transaction != null) existsCmd.Transaction = transaction;
- existsCmd.CommandText = @"
-SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
-WHERE TABLE_SCHEMA = @HistorySchema AND TABLE_NAME = '__BrighterMigrationHistory')";
- existsCmd.Parameters.AddWithValue("@HistorySchema", foldedHistorySchema);
- var existsRaw = await existsCmd.ExecuteScalarAsync(cancellationToken);
- var historyTableExists = existsRaw is bool b && b;
- if (!historyTableExists)
- return false;
- }
+ if (!await HistoryTableExistsAsync(connection, foldedHistorySchema, cancellationToken, transaction))
+ return false;
using var command = connection.CreateCommand();
if (transaction != null) command.Transaction = transaction;
command.CommandText = $@"
SELECT COUNT(1) FROM {quotedHistorySchema}.""__BrighterMigrationHistory""
WHERE ""BoxTableName"" = @BoxTableName AND ""SchemaName"" = @SchemaName";
- // History rows are stored with PG-folded (lowercase) identifiers by the runner so
- // that lookups remain consistent across mixed-case configured values; normalize
- // here too. See PostgreSqlBoxMigrationRunner.InsertHistory.
command.Parameters.AddWithValue("@BoxTableName", PgIdentifier.Normalize(tableName));
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
@@ -175,17 +135,6 @@ SELECT COUNT(1) FROM {quotedHistorySchema}.""__BrighterMigrationHistory""
}
catch (PostgresException ex) when (ex.SqlState == PostgresErrorCodes.UndefinedTable)
{
- // TOCTOU: another connection dropped __BrighterMigrationHistory between our
- // existence check and this count query. In production this cannot happen — the
- // history table is created once and never dropped — but parallel tests do drop
- // it deliberately, and a "table dropped between two queries" outcome is
- // semantically equivalent to "no history". Returning false lets the caller fall
- // through to the runner, which re-creates the history table under its lock.
- //
- // Per PR #4039 reviewer Nit (item 4485697019): Debug-level log so the swallow is
- // observable if a future contributor adds a DROP-history operation outside test
- // setup. Production should never see this; if it does, the deployment has a
- // history-table-management bug worth surfacing.
_logger.LogDebug(
ex,
"PostgreSqlBoxDetectionHelper.DoesHistoryExistAsync: swallowed UndefinedTable on COUNT(1) after positive existence check — __BrighterMigrationHistory was dropped between the two queries. Returning false. In production this indicates a history-table-management bug; in parallel tests it is expected because tests drop the table deliberately.");
@@ -193,21 +142,35 @@ SELECT COUNT(1) FROM {quotedHistorySchema}.""__BrighterMigrationHistory""
}
}
+ // The history table is created with a quoted name, so its case is preserved; this cannot use
+ // DoesTableExistAsync, which folds the name to lowercase.
+ private static async Task HistoryTableExistsAsync(
+ NpgsqlConnection connection, string foldedHistorySchema,
+ CancellationToken cancellationToken, NpgsqlTransaction? transaction)
+ {
+ using var command = connection.CreateCommand();
+ if (transaction != null) command.Transaction = transaction;
+ command.CommandText = @"
+SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
+WHERE TABLE_SCHEMA = @HistorySchema AND TABLE_NAME = '__BrighterMigrationHistory')";
+ command.Parameters.AddWithValue("@HistorySchema", foldedHistorySchema);
+ var raw = await command.ExecuteScalarAsync(cancellationToken);
+ return raw is bool exists && exists;
+ }
+
///
/// Returns the highest migration version recorded in history for the given box table,
/// or 0 if no rows exist.
///
/// Optional. The box-table schema used to filter history rows. Null
- /// is substituted with "public" per ADR 0057 §A.1.
+ /// is substituted with "public".
/// Optional. The physical schema holding the history table; null
- /// resolves to "public" (today's behaviour).
+ /// resolves to "public", the Global default.
public async Task GetMaxVersionAsync(
NpgsqlConnection connection, string tableName, string? schemaName, string? historySchema,
CancellationToken cancellationToken = default,
NpgsqlTransaction? transaction = null)
{
- // Read max version from the same physical history schema the runner writes to: Global →
- // "public"; PerSchema → the configured SchemaName, folded identically via PgIdentifier.Quote.
var quotedHistorySchema = QuotedHistorySchema(historySchema);
using var command = connection.CreateCommand();
if (transaction != null) command.Transaction = transaction;
@@ -228,7 +191,7 @@ SELECT COALESCE(MAX(""MigrationVersion""), 0) FROM {quotedHistorySchema}.""__Bri
/// Reads the column name set for the given table from information_schema.columns.
/// Comparison is case-sensitive; PostgreSQL returns unquoted identifiers folded to lowercase.
///
- /// Optional. Null is substituted with "public" per ADR 0057 §A.1.
+ /// Optional. Null is substituted with "public".
public async Task> GetTableColumnsAsync(
NpgsqlConnection connection, string tableName, string? schemaName,
CancellationToken cancellationToken = default,
@@ -248,7 +211,7 @@ public async Task> GetTableColumnsAsync(
///
///
/// Selects the discriminator: headerbag for outbox, commandbody for inbox.
- /// Optional. Null is substituted with "public" per ADR 0057 §A.1.
+ /// Optional. Null is substituted with "public".
public async Task DetectCurrentVersionAsync(
NpgsqlConnection connection, string tableName, string? schemaName,
BoxType boxType, IReadOnlyList migrations,
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxMigrationRunner.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxMigrationRunner.cs
index 2a2fa11d28..0634c45030 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxMigrationRunner.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxMigrationRunner.cs
@@ -45,11 +45,8 @@ namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
public class PostgreSqlBoxMigrationRunner : SqlBoxMigrationRunner
{
private const string MIGRATION_HISTORY_TABLE = "__BrighterMigrationHistory";
- // The history table is global — one row per (SchemaName, BoxTableName, MigrationVersion)
- // tracking migrations across every box-table schema. It always lives in "public" regardless
- // of the connection's search_path or the configured box schema; without explicit
- // qualification an unqualified CREATE/SELECT/INSERT would land in whichever schema appears
- // first on search_path, scattering history rows across the cluster.
+ // The Global history schema. Qualified explicitly, so history cannot land in whichever schema
+ // is first on the connection's search_path.
private const string HISTORY_TABLE_SCHEMA = "public";
private readonly IPostgreSqlAdvisoryLock _advisoryLock;
@@ -74,10 +71,7 @@ public PostgreSqlBoxMigrationRunner(
}
///
- /// Convenience ctor used by the AddPostgreSqlOutbox/AddPostgreSqlInbox
- /// registration extensions: synthesises a default
- /// so the extension method doesn't have to resolve it from the container before the catalog
- /// is in scope.
+ /// Initialises the runner with a default .
///
public PostgreSqlBoxMigrationRunner(
IAmABoxMigrationCatalog catalog,
@@ -112,73 +106,58 @@ protected override Task> CreateUni
=> Task.FromResult>(
new PostgreSqlProvisioningUnitOfWork(connection, _advisoryLock, Logger));
- // Include the schema in the lock key so two same-named tables in different schemas
- // (e.g. public.Outbox and billing.Outbox) acquire distinct advisory locks. Without
- // the schema qualifier they would share a lock and serialize unnecessarily — matches
- // the MSSQL runner's lockResource shape. Lowercase the identifiers so callers that
- // configure the same physical table with different casings (e.g. "Outbox" vs
- // "outbox" — both folded to `outbox` by PG) hash to the same lock key.
+ ///
+ ///
+ /// Includes the schema, so same-named tables in different schemas (public.Outbox,
+ /// billing.Outbox) take different locks; and folds both names to lowercase, so
+ /// differently-cased configurations of one physical table share a lock.
+ ///
protected override string LockResourceFor(string? schemaName, string tableName)
=> $"BrighterMigration_{PgIdentifier.Normalize(schemaName ?? HISTORY_TABLE_SCHEMA)}.{PgIdentifier.Normalize(tableName)}";
- ///
- /// Resolves and quotes the schema that physically holds the history table for this run, folded
- /// via so it matches the case PostgreSQL stores. Under
- /// this is "public" (today's behaviour); under
- /// it is the configured (non-null) SchemaName. The
- /// detection helper folds the same value the same way, so write and read never diverge.
- ///
- private string QuotedHistorySchema()
+ // Never null here: DefaultHistorySchema is "public", and the base rejects PerSchema without a
+ // SchemaName before migrating.
+ private string ValidatedHistorySchema()
{
- // The `!` is safe: DefaultHistorySchema returns the non-null "public" const on this runner
- // and the D3 guard rejects PerSchema with a null SchemaName before MigrateAsync proceeds,
- // so ResolveHistorySchema() is provably non-null here. If a future refactor breaks that
- // invariant, NRE points at this line.
- var schema = ResolveHistorySchema()!;
- Identifiers.AssertSafe(schema, nameof(schema));
- return PgIdentifier.Quote(schema);
+ var resolvedHistorySchema = ResolveHistorySchema()!;
+ Identifiers.AssertSafe(resolvedHistorySchema, nameof(resolvedHistorySchema));
+ return resolvedHistorySchema;
}
+ // Quoted, and so folded, the same way the detection helper reads it.
+ private string QuotedHistorySchema() => PgIdentifier.Quote(ValidatedHistorySchema());
+
protected override async Task EnsureHistoryTableAsync(
NpgsqlConnection connection, NpgsqlTransaction? transaction, string? schemaName, string tableName,
CancellationToken cancellationToken)
{
- // Resolve the physical history schema once. Under PerSchema it's the configured (non-null)
- // SchemaName; under Global it is the backend default ("public"). The detection helper folds
- // the same value the same way (PgIdentifier.Quote / Normalize), so write and read never
- // diverge. AssertSafe guards against any unsafe schema literal leaking into the inline DDL.
- // The `!` is safe: DefaultHistorySchema returns the non-null "public" const on this runner
- // and the D3 guard rejects PerSchema with a null SchemaName before MigrateAsync proceeds,
- // so ResolveHistorySchema() is provably non-null here.
- var resolvedHistorySchema = ResolveHistorySchema()!;
- Identifiers.AssertSafe(resolvedHistorySchema, nameof(resolvedHistorySchema));
+ var resolvedHistorySchema = ValidatedHistorySchema();
var historySchemaQuoted = PgIdentifier.Quote(resolvedHistorySchema);
var historySchemaFolded = PgIdentifier.Normalize(resolvedHistorySchema);
- // D5 (ADR 0060): on a PerSchema run that follows a Global predecessor, copy this tenant's
- // prior history rows from the legacy public.__BrighterMigrationHistory so the runner does
- // NOT mis-detect the box as needing a bootstrap re-stamp (which would silently re-run
- // migration accounting and break FR5). Skipped when the folded resolved history schema
- // equals the folded backend default (Global, or PerSchema with SchemaName == "public") —
- // then per-schema and legacy are the same table and there is nothing to copy.
- //
- // PR #4155 reviewer bug fix: an earlier implementation gated the seed on whether the
- // per-schema __BrighterMigrationHistory existed before this run, so the second box-type to
- // flip (e.g. inbox after outbox) found the table already created by the first flip and
- // skipped the seed — its legacy row never landed and the bootstrap path stamped a fresh
- // "bootstrap: detected at V_latest" entry. The table-level gate has been removed: the
- // seed's per-row NOT EXISTS PK guard already makes it idempotent (a fresh PerSchema
- // install with no legacy table is a no-op because the seed's SELECT FROM legacy returns
- // zero rows; a re-run on already-seeded data inserts zero rows because the composite PK
- // matches). The cost is one extra `INSERT ... SELECT` round-trip on the steady-state
- // PerSchema path, which is the same cost the first-box-type flip already pays today.
- var needsSeedCheck = !string.Equals(historySchemaFolded, HISTORY_TABLE_SCHEMA, StringComparison.Ordinal);
+ var historyIsSeparateFromLegacy = !string.Equals(historySchemaFolded, HISTORY_TABLE_SCHEMA, StringComparison.Ordinal);
- using (var command = connection.CreateCommand())
+ await CreateHistoryTableAsync(connection, transaction, historySchemaQuoted, cancellationToken);
+
+ if (historyIsSeparateFromLegacy)
{
- command.Transaction = transaction;
- command.CommandText = $@"
+ await SeedHistoryFromLegacyAsync(
+ connection, transaction, historySchemaQuoted, resolvedHistorySchema,
+ schemaName ?? resolvedHistorySchema, tableName, cancellationToken);
+ }
+ }
+
+ // A provisioner for another box can race this CREATE (ADR 0057 §3). Postgres aborts the whole
+ // transaction on the loser's duplicate error even when it is caught, so the CREATE runs inside
+ // a savepoint that the catch rolls back to.
+ private async Task CreateHistoryTableAsync(
+ NpgsqlConnection connection, NpgsqlTransaction? transaction, string historySchemaQuoted,
+ CancellationToken cancellationToken)
+ {
+ using var command = connection.CreateCommand();
+ command.Transaction = transaction;
+ command.CommandText = $@"
CREATE TABLE IF NOT EXISTS {historySchemaQuoted}.""{MIGRATION_HISTORY_TABLE}"" (
""MigrationVersion"" INT NOT NULL,
""SchemaName"" VARCHAR(256) NOT NULL DEFAULT 'public',
@@ -187,76 +166,38 @@ protected override async Task EnsureHistoryTableAsync(
""AppliedAt"" TIMESTAMPTZ NOT NULL DEFAULT NOW(),
PRIMARY KEY (""SchemaName"", ""BoxTableName"", ""MigrationVersion"")
)";
- // Postgres marks the outer transaction as aborted on a catalog-level duplicate-type
- // error from CREATE TABLE IF NOT EXISTS regardless of whether we catch it. Without a
- // savepoint the catch below leaves the transaction "poisoned" — RedetectStateAsync's
- // next statement then fails with 25P02 even though our swallow was intentional. A
- // SAVEPOINT around the CREATE narrows the abort to the inner sub-transaction, so
- // ROLLBACK TO SAVEPOINT restores a usable transaction state on the racing path.
+ if (transaction is not null)
+ {
+ await transaction.SaveAsync(HistoryTableSavepoint, cancellationToken);
+ }
+ try
+ {
+ await command.ExecuteNonQueryAsync(cancellationToken);
if (transaction is not null)
{
- await transaction.SaveAsync(HistoryTableSavepoint, cancellationToken);
- }
- try
- {
- await command.ExecuteNonQueryAsync(cancellationToken);
- if (transaction is not null)
- {
- await transaction.ReleaseAsync(HistoryTableSavepoint, cancellationToken);
- }
+ await transaction.ReleaseAsync(HistoryTableSavepoint, cancellationToken);
}
- catch (PostgresException ex) when (
- ex.SqlState == PostgresErrorCodes.UniqueViolation
- || ex.SqlState == PostgresErrorCodes.DuplicateTable
- || ex.SqlState == PostgresErrorCodes.DuplicateObject)
+ }
+ catch (PostgresException ex) when (
+ ex.SqlState == PostgresErrorCodes.UniqueViolation
+ || ex.SqlState == PostgresErrorCodes.DuplicateTable
+ || ex.SqlState == PostgresErrorCodes.DuplicateObject)
+ {
+ if (transaction is not null)
{
- // TOCTOU on Postgres catalog: another connection raced our CREATE TABLE IF NOT EXISTS
- // between the existence check (pg_class) and the type insert (pg_type), or between
- // the type insert and a duplicate-relation check. The error surfaces as one of:
- // 23505 (UniqueViolation) on pg_type_typname_nsp_index — most common
- // 42P07 (DuplicateTable) — relation already exists
- // 42710 (DuplicateObject) — type/constraint already exists
- // In every case the history table now exists with the schema we intended to create
- // (the racing session ran the same DDL), which is the post-condition we wanted.
- if (transaction is not null)
- {
- // Restore the transaction from the aborted sub-transaction back to the pre-CREATE
- // state. Subsequent statements (RedetectStateAsync etc.) then execute against a
- // live transaction instead of failing with 25P02.
- await transaction.RollbackAsync(HistoryTableSavepoint, cancellationToken);
- }
-
- // Surface the swallow so operators investigating "did two replicas race here?" have
- // a signal — Debug-level log (carrying the actual SqlState so the three race shapes
- // can be distinguished after the fact) + Activity event on the migration span (when
- // one is active). Silent swallow was the prior behaviour; per PR #4039 review item #7
- // the race is real and the swallow is intentional, but operators got no signal that
- // two racers serialised here.
- Logger.LogDebug(ex,
- "{HistoryTable} already created by racing session (Postgres SqlState {SqlState})",
- MIGRATION_HISTORY_TABLE, ex.SqlState);
- Activity.Current?.AddEvent(new ActivityEvent(
- BrighterSemanticConventions.BoxMigrationEventHistoryTableRaceSwallowed));
+ await transaction.RollbackAsync(HistoryTableSavepoint, cancellationToken);
}
- }
- // Run the D5 seed on every PerSchema provision (subject to needsSeedCheck above). The seed
- // carries a NOT EXISTS guard against the composite PK so re-fires are harmless — the
- // important invariant is that EVERY box-type that flips gets a chance to seed its own
- // (SchemaName, BoxTableName)-filtered row, not just the first one to land in a freshly
- // created per-schema history table.
- if (needsSeedCheck)
- {
- await SeedHistoryFromLegacyAsync(
- connection, transaction, historySchemaQuoted, resolvedHistorySchema,
- schemaName ?? resolvedHistorySchema, tableName, cancellationToken);
+ Logger.LogDebug(ex,
+ "{HistoryTable} already created by racing session (Postgres SqlState {SqlState})",
+ MIGRATION_HISTORY_TABLE, ex.SqlState);
+ Activity.Current?.AddEvent(new ActivityEvent(
+ BrighterSemanticConventions.BoxMigrationEventHistoryTableRaceSwallowed));
}
}
- // EXISTS probe used both for the D5 pre-create check on the per-schema table and the
- // legacy-existence check inside SeedHistoryFromLegacyAsync. The schema parameter must already
- // be folded via PgIdentifier.Normalize — information_schema.tables stores the case PG actually
- // saved, so an unfolded compare would miss tables created from mixed-case identifiers.
+ // foldedSchema must already be folded by PgIdentifier.Normalize: information_schema holds the
+ // case PostgreSQL stored, so an unfolded name would miss a table created from a mixed-case one.
private async Task DoesHistoryTableExistAsync(
NpgsqlConnection connection, NpgsqlTransaction? transaction, string foldedSchema,
CancellationToken cancellationToken)
@@ -272,18 +213,11 @@ private async Task DoesHistoryTableExistAsync(
return result is bool exists && exists;
}
- // Copies this tenant's prior history rows from the legacy public history table into the
- // per-schema history table (ADR 0060 D5). Runs on every PerSchema provision where the resolved
- // history schema differs from the backend default: the per-row NOT EXISTS PK guard makes
- // steady-state runs (no new legacy rows) a zero-row no-op, while still allowing each box-type
- // that flips later to seed its own (SchemaName, BoxTableName) row when needed (PR #4155 fix).
- // Filtered to this tenant (SchemaName + BoxTableName, folded the same way the runner stamped
- // them on the Global INSERT path) so a multi-tenant Global deployment doesn't bleed rows across
- // tenants. All five columns are copied — Description is NOT NULL with no default and AppliedAt
- // preserves the original install/bootstrap timestamp, so the post-flip row is indistinguishable
- // from a row written by the original Global-scope provision. NOT EXISTS on the composite PK makes the
- // seed idempotent. Skipped when no legacy table exists — this is the first-ever provision (no
- // Global predecessor) and there is nothing to copy.
+ // Copies this box's rows from the legacy public history table into the per-schema one
+ // (ADR 0060 D5). Runs on every PerSchema provision, not only the first: each box type that
+ // flips must seed its own rows, and the NOT EXISTS guard makes a re-run copy nothing. All five
+ // columns are copied, so a seeded row is indistinguishable from the original. No legacy table
+ // means no Global predecessor, so nothing to copy.
private async Task SeedHistoryFromLegacyAsync(
NpgsqlConnection connection, NpgsqlTransaction? transaction,
string perSchemaQuoted, string perSchemaBare, string boxSchema, string boxTableName,
@@ -299,12 +233,6 @@ private async Task SeedHistoryFromLegacyAsync(
}
catch (PostgresException ex) when (IsLegacyHistoryReadPermissionDenied(ex))
{
- // PR #4155 reviewer #2: information_schema.tables is row-filtered to objects the role
- // has SOME privilege on (so a missing-grant probe normally just returns zero rows, not
- // 42501), but wrapping with the same when-filter as the INSERT below keeps the
- // documented "any legacy-history-read failure surfaces as a ConfigurationException"
- // contract honest. Non-permission failures (connectivity, syntax) propagate untouched;
- // the outer UoW transaction still rolls back.
throw BuildLegacyHistoryReadDeniedException(legacyQuoted, perSchemaQuoted, ex);
}
if (!legacyExists)
@@ -312,12 +240,6 @@ private async Task SeedHistoryFromLegacyAsync(
return;
}
- // perSchemaQuoted was already AssertSafe-d + Quote-d by the caller. legacySchema is the
- // compile-time const HISTORY_TABLE_SCHEMA ("public") — trivially safe — so no AssertSafe
- // call is needed. If a future refactor turns legacySchema into a non-const derived value
- // (e.g. an operator-configurable legacy-schema override) restore the AssertSafe check.
- // boxSchema is operator-supplied and must be validated before its folded form is passed as a
- // parameter (Normalize doesn't sanitise — AssertSafe does).
Identifiers.AssertSafe(boxSchema, nameof(boxSchema));
int rowsCopied;
@@ -344,32 +266,12 @@ SELECT 1 FROM {perSchemaQuoted}.""{MIGRATION_HISTORY_TABLE}"" AS tgt
}
catch (PostgresException ex) when (IsLegacyHistoryReadPermissionDenied(ex))
{
- // Reviewer #1 hardening (must, not should): any failure reading the legacy history
- // table — typically tenant-isolated credentials lacking SELECT on public — must
- // surface as a ConfigurationException with a clear cause. Silently absorbing the
- // failure would let an empty seed go through; the next provision would then
- // bootstrap-stamp a fresh row, effectively re-running the migration ledger and
- // breaking FR5. The throw rolls back the surrounding transaction so no partial
- // per-schema state is left behind.
- //
- // PR #4155 reviewer #2 tightening: the catch is filtered to the SELECT-permission
- // SqlState (42501 / insufficient_privilege) so a deadlock victim, connection drop,
- // statement timeout, or future-refactor syntax error doesn't get misreported as
- // "grant SELECT on the legacy table". Unmatched PostgresExceptions propagate
- // unchanged; the outer UoW transaction still rolls back.
throw BuildLegacyHistoryReadDeniedException(legacyQuoted, perSchemaQuoted, ex);
}
}
if (rowsCopied > 0)
{
- // Spec 0029 NF5/AC7 (ADR 0060 D6, reviewer #3): structured fields RowCount + BoxTable +
- // LegacySchema + TargetSchema each appear as a separate placeholder so a structured log
- // sink can filter on individual fields. Target/legacy schemas are passed bare (without
- // the PgIdentifier.Quote double-quote) so the values are stable join keys against other
- // operator dashboards that report unquoted schema identifiers. The existing legacy-
- // history-seeded Activity event additionally carries the row count as a tag so a
- // trace-store query can size flip impact without parsing event names.
Logger.LogInformation(
"Seeded {RowCount} legacy history row(s) for {BoxTable} from {LegacySchema} to {TargetSchema}",
rowsCopied, boxTableName, legacySchema, perSchemaBare);
@@ -383,14 +285,9 @@ SELECT 1 FROM {perSchemaQuoted}.""{MIGRATION_HISTORY_TABLE}"" AS tgt
}
}
- // PR #4155 reviewer #2: narrow the seed catch so the operator-facing "grant SELECT on the
- // legacy history table" message only fires when the provider actually raised a SELECT-
- // permission error. 42501 (insufficient_privilege) is the canonical SqlState PostgreSQL
- // raises for missing SELECT/INSERT on a relation. Other PostgresExceptions (connectivity
- // 08xxx, deadlock 40P01, statement_timeout 57014, future-refactor syntax errors 42xxx-other)
- // propagate untouched.
+ // Only a missing privilege means "grant SELECT"; any other failure propagates unchanged.
private static bool IsLegacyHistoryReadPermissionDenied(PostgresException ex) =>
- ex.SqlState == "42501";
+ ex.SqlState == PostgresErrorCodes.InsufficientPrivilege;
private static ConfigurationException BuildLegacyHistoryReadDeniedException(
string legacyQuoted, string perSchemaQuoted, Exception inner) =>
@@ -413,12 +310,6 @@ protected override async Task RunFreshPathAsync(
{
var effectiveSchema = schemaName ?? HISTORY_TABLE_SCHEMA;
- // Execute the V_latest-shape DDL sourced from IAmABoxMigrationCatalog.FreshInstallDdl
- // (the live builder DDL — typically OutboxBuilder.GetDDL(...) for outbox /
- // InboxBuilder.GetDDL(...) for inbox). We stamp directly at V_latest with a
- // "fresh install" marker — the V2..V_latest ALTERs in the chain would be no-ops on the
- // V_latest-shape table, so we skip the chain entirely (spec 0027 R1 fresh-install fast
- // path per ADR §3).
await ExecuteDdlAsync(connection, transaction!, freshInstallDdl, cancellationToken);
await InsertHistoryRowAsync(
@@ -512,12 +403,6 @@ private async Task InsertHistoryRowAsync(
INSERT INTO {QuotedHistorySchema()}.""{MIGRATION_HISTORY_TABLE}"" (""MigrationVersion"", ""SchemaName"", ""BoxTableName"", ""Description"")
VALUES (@Version, @SchemaName, @BoxTableName, @Description)";
command.Parameters.AddWithValue("@Version", version);
- // Store PG-folded (lowercase) identifiers so history rows match the case of the
- // physical table PG actually created — and so lookups via the detection helper
- // (which normalizes the same way) hit the same row regardless of the configured
- // casing. Existing rows written before this normalization remain in the table but
- // become unreachable on read; the ALTER chain is idempotent (`ADD COLUMN IF NOT
- // EXISTS`) so a re-run against an existing table is a no-op.
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName));
command.Parameters.AddWithValue("@BoxTableName", PgIdentifier.Normalize(tableName));
command.Parameters.AddWithValue("@Description", description);
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxProvisioningExtensions.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxProvisioningExtensions.cs
index 69aeec08fb..63cf4123ca 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxProvisioningExtensions.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxProvisioningExtensions.cs
@@ -18,10 +18,6 @@ public static BoxProvisioningOptions AddPostgreSqlOutbox(
this BoxProvisioningOptions options,
IAmARelationalDatabaseConfiguration configuration)
{
- // MigrationLockTimeout is read inside the registration lambda so it stays late-bound:
- // the configure delegate may set the timeout AFTER calling Add*Outbox/Add*Inbox and
- // the change still takes effect (registrations only run once the configure delegate
- // has fully completed inside UseBoxProvisioning).
options.Add(services =>
{
RegisterSharedRoleImpls(services);
@@ -147,11 +143,7 @@ public static BoxProvisioningOptions AddPostgreSqlInbox(
return options;
}
- // Detection helper + payload validator are shared across Outbox and Inbox extensions;
- // TryAddSingleton makes the registration idempotent so calling both Add*Outbox and
- // Add*Inbox does not produce duplicate singletons. Catalogue registration is per
- // box-type and lives in the call-site (Outbox catalogue from Add*Outbox; Inbox
- // catalogue from Add*Inbox) per ADR 0058 §A.4 Alternatives.
+ // Shared by the outbox and inbox registrations; TryAdd makes calling both safe.
private static void RegisterSharedRoleImpls(IServiceCollection services)
{
services.TryAddSingleton();
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlInboxMigrationCatalog.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlInboxMigrationCatalog.cs
index af1a6e5a1c..1ab7da0932 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlInboxMigrationCatalog.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlInboxMigrationCatalog.cs
@@ -35,17 +35,15 @@ namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
/// PostgreSQL inbox skips the ContextKey catch-up the other backends carry. Unlike
/// MSSQL/MySQL/SQLite — which have a V1 baseline followed by a V2 that adds ContextKey
/// (commit 787c31c52, Oct 2018) — the PostgreSQL inbox was born with ContextKey +
-/// composite primary key (CommandId, ContextKey) in PR #1401 (Nov 2023, commit
-/// 1cdc04b60). No pre-ContextKey PostgreSQL inbox ever shipped (ADR 0057 "Alternatives →
+/// composite primary key (CommandId, ContextKey) (Nov 2023, commit 1cdc04b60). No pre-ContextKey PostgreSQL inbox ever shipped (ADR 0057 "Alternatives →
/// E"), so V1's already includes contextkey.
-/// Spec 0027 (#2541) then adds a V2 that introduces causationid via an idempotent
+/// A V2 then introduces causationid via an idempotent
/// ALTER TABLE ... ADD COLUMN IF NOT EXISTS, so the chain is V1→V2 — exactly one version
/// behind the other three (which run V1→V2→V3).
///
/// V1's UpScript is the literal historical baseline DDL extracted from commit
-/// 1cdc04b60 — the first PostgreSQL inbox builder shape. Spec 0027 R1 split "live
-/// builder DDL" away from V1.UpScript: the fresh-install fast path (ADR 0057 §3) now
-/// sources its DDL from . V1.UpScript is the historical
+/// 1cdc04b60 — the first PostgreSQL inbox builder shape. The fresh-install fast path
+/// (ADR 0057 §3) takes its DDL from instead. V1.UpScript is the historical
/// reference for legacy bootstrap; V2's ADD COLUMN IF NOT EXISTS causationid is safe to
/// re-execute on chain replay via Postgres's native idempotency clause.
///
@@ -58,15 +56,9 @@ public class PostgreSqlInboxMigrationCatalog : IAmABoxMigrationCatalog
{
private const string DefaultSchema = "public";
- // Literal historical PostgreSQL inbox DDL extracted from commit 1cdc04b60 (Nov 2023, PR
- // #2560). First-shipped state already carried ContextKey with a composite PRIMARY KEY on
- // (CommandId, ContextKey) — see the V1-only/born-past-V1 note in the class remarks.
- // {0} = quoted, lowercased table identifier (via PgIdentifier).
- // Lowercase-then-quote: PG case-folds unquoted identifiers to lowercase at parse time, so
- // the legacy unquoted form created e.g. `inbox` from a configured `"Inbox"`. Quoting the
- // already-lowercased form ("inbox") resolves to the same physical table as the legacy
- // unquoted form, AND lets reserved-keyword names (User, Order, …) parse cleanly. All
- // PG catalogs and builders share this fold-then-quote convention via PgIdentifier.
+ // From commit 1cdc04b60 (Nov 2023); born with ContextKey, see the class remarks.
+ // {0} is the table name folded to lowercase then quoted: that names the same table the
+ // original unquoted DDL created, and lets reserved words (Order, User) parse.
private const string V1HistoricalDdl =
"""
CREATE TABLE {0}
@@ -86,9 +78,6 @@ public string FreshInstallDdl(IAmARelationalDatabaseConfiguration configuration)
Identifiers.AssertSafe(
configuration.InBoxTableName,
nameof(IAmARelationalDatabaseConfiguration.InBoxTableName));
- // Pass SchemaName so the builder schema-qualifies the CREATE TABLE IF NOT EXISTS.
- // Per PR #4039 reviewer item M4-1 (F1b). See the outbox catalog for the full
- // rationale — the same fix applies symmetrically to the inbox.
if (configuration.SchemaName is not null)
{
Identifiers.AssertSafe(
@@ -105,7 +94,7 @@ public string FreshInstallDdl(IAmARelationalDatabaseConfiguration configuration)
///
/// Returns the migration list for the PostgreSQL inbox: V1 (the literal historical
/// PostgreSQL inbox DDL, composite primary key on (CommandId, ContextKey)) followed
- /// by V2 (idempotent ADD COLUMN IF NOT EXISTS causationid, Spec 0027 #2541).
+ /// by V2 (idempotent ADD COLUMN IF NOT EXISTS causationid).
///
/// The relational database configuration.
/// An ordered list of migrations from V1 to V2.
@@ -140,11 +129,7 @@ public IReadOnlyList All(IAmARelationalDatabaseConfiguration c
];
}
+ // Column names are emitted bare: the catalog's are already lowercase (ADR 0057 §1).
private static string AddColumn(string schema, string table, string column, string type) =>
- // schema/table are lowercase-then-quoted via PgIdentifier so the ALTER targets the same
- // physical table that V1's CREATE produced — and so reserved-keyword names parse cleanly.
- // Column names are sourced from the catalog and are already ADR 0057 §1 lowercase, so they
- // are emitted bare. Postgres's native ADD COLUMN IF NOT EXISTS keeps the migration safe to
- // re-execute on chain replay.
$"ALTER TABLE {PgIdentifier.QuoteQualified(schema, table)} ADD COLUMN IF NOT EXISTS {column} {type} NULL;";
}
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlOutboxMigrationCatalog.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlOutboxMigrationCatalog.cs
index 326712aa94..f6c80e18cc 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlOutboxMigrationCatalog.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlOutboxMigrationCatalog.cs
@@ -34,10 +34,9 @@ namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
///
///
/// V1's UpScript is the literal historical baseline DDL — the first PostgreSQL
-/// outbox builder shape (commit 3c30343fa, July 2019). Spec 0027 R1 split "live
-/// builder DDL" away from V1.UpScript: the fresh-install fast path (ADR 0057 §3) now
-/// sources its DDL from , so V1.UpScript is free to carry the
-/// honest historical shape. V2..V7 are conditional ALTER TABLE ... ADD COLUMN IF NOT
+/// outbox builder shape (commit 3c30343fa, July 2019). The fresh-install fast path
+/// (ADR 0057 §3) takes its DDL from instead, so V1.UpScript
+/// carries the historical shape. V2..V7 are conditional ALTER TABLE ... ADD COLUMN IF NOT
/// EXISTS statements per ADR 0057 §5 — Postgres's native idempotency clause keeps each
/// migration safe to re-execute on chain replay.
///
@@ -51,7 +50,7 @@ namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
///
/// are lowercase per ADR 0057 §1 so that
/// detection-by-information_schema.columns (which returns Postgres's folded lowercase
-/// column names) matches without case mismatch. V8 adds causationid (Spec 0027, #2541)
+/// column names) matches without case mismatch. V8 adds causationid
/// plus a replay index on it. The accumulated columns across V1..V8 plus
/// the Postgres housekeeping (id, messageid) equal the live builder's column
/// set — verified by the drift test in tests/Paramore.Brighter.PostgresSQL.Tests/BoxProvisioning.
@@ -75,16 +74,9 @@ public class PostgreSqlOutboxMigrationCatalog : IAmABoxMigrationCatalog
private static readonly string[] s_v7AddedColumns = ["dataref", "specversion"];
private static readonly string[] s_v8AddedColumns = ["causationid"];
- // Literal historical PostgreSQL outbox DDL extracted from commit 3c30343fa (July 2019).
- // First-shipped state already carried Dispatched — see the born-past-V1 note in the
- // class remarks. {0} = quoted, lowercased table identifier (via PgIdentifier).
- // Lowercase-then-quote: PG case-folds unquoted identifiers to lowercase at parse time,
- // so historical tables created via the unquoted builder DDL live as e.g. `outbox` in
- // pg_class regardless of the configured `OutBoxTableName = "Outbox"`. Quoting the
- // already-lowercased form ("outbox") resolves to the same physical table as the legacy
- // unquoted form would have, AND unlocks reserved-keyword names (Order, User, Group, …)
- // that the unquoted V2..V7 ALTERs would otherwise reject as syntax errors. All catalogs
- // and builders in this chain apply the same fold-then-quote rule via PgIdentifier.
+ // From commit 3c30343fa (July 2019); born with Dispatched, see the class remarks.
+ // {0} is the table name folded to lowercase then quoted: that names the same table the
+ // original unquoted DDL created, and lets reserved words (Order, User) parse.
private const string V1HistoricalDdl =
"""
CREATE TABLE {0}
@@ -106,11 +98,6 @@ public string FreshInstallDdl(IAmARelationalDatabaseConfiguration configuration)
Identifiers.AssertSafe(
configuration.OutBoxTableName,
nameof(IAmARelationalDatabaseConfiguration.OutBoxTableName));
- // Pass SchemaName so the builder schema-qualifies the CREATE TABLE IF NOT EXISTS —
- // otherwise the table lands in the connection's search_path default (typically
- // `public`) regardless of the configured SchemaName, leaving V2..V7 ALTERs (which
- // use `{schema}.{table}`) targeting a table that does not exist on subsequent runs.
- // Per PR #4039 reviewer item M4-1 (F1b).
if (configuration.SchemaName is not null)
{
Identifiers.AssertSafe(
@@ -228,17 +215,11 @@ private static IReadOnlyCollection Cumulative(int upToVersion)
private static string AddColumns(string schema, string table, params (string Column, string Type)[] columns) =>
string.Join(Environment.NewLine, columns.Select(c => AddColumn(schema, table, c.Column, c.Type)));
+ // Column names are emitted bare: the catalog's are already lowercase (ADR 0057 §1).
private static string AddColumn(string schema, string table, string column, string type) =>
- // schema/table are lowercase-then-quoted via PgIdentifier so the ALTER targets the
- // same physical table that V1's CREATE produced — and so reserved-keyword names
- // (Order, User, Group, …) parse cleanly. Column names are sourced from the catalog
- // and are already ADR 0057 §1 lowercase, so they are emitted bare.
$"ALTER TABLE {PgIdentifier.QuoteQualified(schema, table)} ADD COLUMN IF NOT EXISTS {column} {type} NULL;";
- // Idempotent replay index (Spec 0027, #2541) on causationid via Postgres's native
- // CREATE INDEX IF NOT EXISTS. The index name folds the table identifier to lowercase
- // (PgIdentifier.Normalize) and is emitted bare so it matches Postgres's natural case-fold;
- // the indexed table is lowercase-then-quoted via PgIdentifier like the ALTER targets above.
+ // The index name is the folded table name, emitted bare to match Postgres's own case-folding.
private static string AddCausationIndex(string schema, string table) =>
$"CREATE INDEX IF NOT EXISTS idx_{PgIdentifier.Normalize(table)}_causationid " +
$"ON {PgIdentifier.QuoteQualified(schema, table)} (causationid);";
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlPayloadModeValidator.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlPayloadModeValidator.cs
index adc49e4aa1..ecbefa239e 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlPayloadModeValidator.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlPayloadModeValidator.cs
@@ -61,8 +61,6 @@ public async Task ValidateAsync(
command.CommandText = @"
SELECT data_type FROM information_schema.columns
WHERE table_schema = @SchemaName AND table_name = @TableName AND column_name = @ColumnName";
- // information_schema.columns stores PG-folded (lowercase) identifiers. Normalize
- // configured values so mixed-case defaults match the stored folded form.
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(resolvedSchema));
command.Parameters.AddWithValue("@TableName", PgIdentifier.Normalize(tableName));
command.Parameters.AddWithValue("@ColumnName", PgIdentifier.Normalize(columnName));
diff --git a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlProvisioningUnitOfWork.cs b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlProvisioningUnitOfWork.cs
index 8eea0d6e22..025275f556 100644
--- a/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlProvisioningUnitOfWork.cs
+++ b/src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlProvisioningUnitOfWork.cs
@@ -50,14 +50,6 @@ public class PostgreSqlProvisioningUnitOfWork(
///
public async Task BeginAsync(string lockResource, TimeSpan lockTimeout, CancellationToken cancellationToken)
{
- // Per ADR 0058 §B.1: the advisory lock MUST be acquired before the transaction is
- // opened because pg_advisory_lock is session-scoped — the lock outlives any
- // transaction on the connection and is released either by an explicit
- // pg_advisory_unlock or when the connection closes. Acquiring the lock outside any
- // transaction makes the contention window explicit and decouples the lock's lifetime
- // from any in-flight tx. Reverse ordering would still acquire a session-scoped lock
- // (Postgres permits advisory-lock acquisition inside a tx), but the sequencing would
- // be misleading and breaks the §B.1 contract.
logger.LogTrace("Beginning Postgres provisioning UoW for resource {LockResource}", lockResource);
_lockResource = lockResource;
await advisoryLock.AcquireAsync(connection, lockResource, lockTimeout, cancellationToken);
@@ -67,73 +59,43 @@ public async Task BeginAsync(string lockResource, TimeSpan lockTimeout, Cancella
}
catch
{
- // Per ADR 0058 §B.3 atomic-Begin: BeginTransactionAsync threw AFTER the session-
- // scoped lock was acquired. The §B.3 runner contract places BeginAsync OUTSIDE
- // its try/catch (pinned by When_relational_box_migration_runner_base_begin_async_throws...),
- // so RollbackAsync will not run — the lock would leak until connection close
- // (bounded but messy). Release the lock at source so atomic-Begin holds: a
- // BeginAsync that throws guarantees no resources are held. _lockResource is
- // cleared so a subsequent best-effort RollbackAsync is a clean no-op.
- try
- {
- // Pass CancellationToken.None — NOT the caller's token — mirroring §B.3
- // rollback semantics: if BeginTransactionAsync threw because the caller's
- // token was cancelled (or cancelled during this catch block), reusing that
- // token here short-circuits pg_advisory_unlock with OperationCanceledException
- // before the SQL is dispatched, and the session-scoped lock leaks to
- // connection close. The release SQL is a single round-trip and bounded by the
- // connection state; running it untokened guarantees the unlock attempt reaches
- // the server even when the caller has already abandoned the operation.
- await advisoryLock.ReleaseAsync(connection, lockResource, CancellationToken.None);
- }
- catch (Exception releaseEx)
- {
- // Release-on-cleanup MUST NOT replace the original BeginTransactionAsync
- // exception. Log + swallow; the session-scoped lock is released by the server
- // when the connection closes.
- logger.LogWarning(
- releaseEx,
- "Postgres provisioning UoW: pg_advisory_unlock threw while cleaning up after BeginTransactionAsync failure for lock resource '{LockResource}' — release skipped, the session will release the lock when the connection closes.",
- lockResource);
- }
+ await ReleaseLockAfterFailedBeginAsync(lockResource);
_lockResource = null;
throw;
}
}
+ // A BeginAsync that throws must hold nothing, and the runner will not call RollbackAsync
+ // (ADR 0058 §B.3). Uses CancellationToken.None because a cancelled caller token may be why
+ // Begin failed, and must not stop the unlock reaching the server. Never throws, so the Begin
+ // failure is what the caller sees; the server releases the lock when the connection closes.
+ private async Task ReleaseLockAfterFailedBeginAsync(string lockResource)
+ {
+ try
+ {
+ await advisoryLock.ReleaseAsync(connection, lockResource, CancellationToken.None);
+ }
+ catch (Exception releaseEx)
+ {
+ logger.LogWarning(
+ releaseEx,
+ "Postgres provisioning UoW: pg_advisory_unlock threw while cleaning up after BeginTransactionAsync failure for lock resource '{LockResource}' — release skipped, the session will release the lock when the connection closes.",
+ lockResource);
+ }
+ }
+
///
public async Task CommitAsync(CancellationToken cancellationToken)
{
- // Per ADR 0058 §B.1: lock release is EXPLICIT for Postgres because pg_advisory_lock is
- // session-scoped — it is NOT auto-released on transaction commit (the MSSQL pattern,
- // where sp_getapplock with @LockOwner='Transaction' rides the tx lifetime, does not
- // apply here). Commit the tx first, then call pg_advisory_unlock; both must succeed
- // in this happy-path branch.
if (_transaction is null) return;
await _transaction.CommitAsync(cancellationToken);
var held = await advisoryLock.ReleaseAsync(connection, _lockResource!, cancellationToken);
- if (!held)
- {
- // Per ADR 0057 §5b: pg_advisory_unlock returns false when this session does not
- // currently hold the named lock — a diagnostic anomaly because the UoW just
- // acquired it. Surface it as a Warning naming the lock resource so the operator
- // can correlate against the runner's table-level context, and continue.
- logger.LogWarning(
- "Postgres provisioning UoW: pg_advisory_unlock returned false for lock resource '{LockResource}' — the lock was not held by this session at release. This is likely a Brighter defect — please report it.",
- _lockResource);
- }
+ if (!held) LogLockNotHeld();
}
///
public async Task RollbackAsync(CancellationToken cancellationToken)
{
- // Per ADR 0058 §B.3: best-effort. The runner may call RollbackAsync after a thrown
- // CommitAsync (the transaction may already be in a finalised state by then —
- // committed-but-client-side-failed, or zombied by a broken connection); calling
- // _transaction.RollbackAsync on a finalised tx throws InvalidOperationException
- // ("Cannot rollback; this transaction has been completed..."). RollbackAsync MUST NOT
- // throw — log a Warning and continue so the lock is still released. Disposal-style
- // semantics: BOTH halves of the unwind are attempted regardless.
if (_transaction is not null)
{
try
@@ -152,20 +114,10 @@ public async Task RollbackAsync(CancellationToken cancellationToken)
try
{
var held = await advisoryLock.ReleaseAsync(connection, _lockResource, cancellationToken);
- if (!held)
- {
- logger.LogWarning(
- "Postgres provisioning UoW: pg_advisory_unlock returned false for lock resource '{LockResource}' — the lock was not held by this session at release. This is likely a Brighter defect — please report it.",
- _lockResource);
- }
+ if (!held) LogLockNotHeld();
}
catch (Exception ex)
{
- // pg_advisory_unlock executes SQL on the same connection — if the connection is
- // dead (mid-migration driver fault, ObjectDisposedException, NpgsqlException)
- // ReleaseAsync throws. RollbackAsync MUST NOT throw — otherwise the runner's
- // catch path (catch { RollbackAsync(...); throw; }) replaces the original
- // migration exception with this cleanup-side failure, masking the real cause.
logger.LogWarning(
ex,
"Postgres provisioning UoW: pg_advisory_unlock threw for lock resource '{LockResource}' — release skipped, the session will release the lock when the connection closes.",
@@ -174,6 +126,11 @@ public async Task RollbackAsync(CancellationToken cancellationToken)
}
}
+ private void LogLockNotHeld() =>
+ logger.LogWarning(
+ "Postgres provisioning UoW: pg_advisory_unlock returned false for lock resource '{LockResource}' — the lock was not held by this session at release. This is likely a Brighter defect — please report it.",
+ _lockResource);
+
///
public ValueTask DisposeAsync()
{