diff --git a/CHANGELOG.md b/CHANGELOG.md index dd2dedcd8b..54da359741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`server_id` identity now carries engine and port, without re-keying anything that exists** ([#2218]) - the storage name was derived from host, database and read-only intent only, so a SQL Server and a PostgreSQL instance on ONE host collided into a single identity and interleaved their histories, as did two PostgreSQL instances distinguished only by port - both of which [#2213] made first-class configuration. Engine and port are now discriminators. **The interesting constraint is what could NOT change.** Lite derives `server_id` FRESH at runtime from the shared `ServerIdHelper.BuildStorageName`, everywhere, and has no stored-id fallback the way Darling does - so altering what that function returns for an EXISTING server would re-key it in Lite and orphan all of its collected history, silently: the same class of harm as [#2158], arrived at from the other direction. So the new parameters are OPTIONAL and append nothing at their defaults, which keeps Lite's three-argument call byte-identical - verified against a re-statement of the pre-change rule rather than against hand-written strings, so it holds for any input and not just the cases someone thought to list. A SQL Server entry that DOES pass them is unchanged too, which is what lets Darling pass `Engine` and `Port` unconditionally instead of branching: `Engine` folds to no token for SQL Server, and `Port` is a PostgreSQL-only field that stays 0 there because SQL Server carries a non-default port inside the host as `host,1433` and is therefore already discriminated by the host string. Darling's already-registered PostgreSQL targets do not re-key either, for a different reason: their id comes from the store and is only ever derived for an entry with no row yet - which is why [#2158] (identity assigned, never re-derived) was a prerequisite for this rather than a sibling of it. Every spelling of the engine folds to one token (`postgres`, `PostgreSQL`, `pg`, any casing), so a colleague's capitalisation cannot mint a second identity for one instance - a split history nothing downstream could diagnose - and an unrecognised engine appends nothing rather than being interpolated raw, so a typo cannot mint one either. Suffix order is fixed (engine, port, then `:RO`) so two callers supplying the same facts cannot produce two names. + - **A registration connected to a database it does not name now says so** ([#2228]) - identity is registration-derived and was never checked against the connection, so a registration whose Initial Catalog is absent, misspelled or overridden lands in a different database and every collected row is stored under that registration's identity while describing somewhere else - indefinitely, with nothing anywhere saying so. When a sibling registration names that same database, both collect it and its history exists twice under two identities: [#2220]'s report of byte-identical deadlock graphs under six `server_id`s, one real incident alerting six times. Both engine probes now also return the database the connection ACTUALLY reached - `DB_NAME()` on SQL Server, `current_database()` on PostgreSQL - and the worker compares it against the registration on every connect. **Why this has to happen at connect and not in the registry**: [#2158] established that identity is assigned rather than derived, which fixes the re-key class but cannot touch this one, because the two registrations here genuinely DIFFER in configuration - no amount of care in hashing config can tell that they resolve to one database. Only the server can answer what a connection reached. The message names what is at stake (mis-attributed rows, and duplication when a sibling names the same database) and both places the setting lives, because the log line is the whole diagnosis. Logged at ERROR on the TRANSITION rather than per connect: a mismatch is a standing misconfiguration that persists until someone edits the registration, so repeating it every reconnect would bury the one line that matters, which is how a tripwire gets trained past and stops working; the recovery is logged too, so an operator who fixes it has confirmation rather than silence. **Silent in three cases on purpose**, each a false positive that would have made the feature useless: a registration naming NO database is server-scoped by design and meant to land wherever the login defaults, so it would otherwise fire on every correctly-configured server-scoped registration in the fleet; a null probe answer is unknown rather than mismatched; and comparison is case-insensitive, which is what SQL Server database names are. `DB_NAME()` needs no DMV, so it does not reintroduce the VIEW DATABASE STATE dependency [#1535] removed, and both columns are APPENDED because every read in those probes is positional. The registration-time half of the issue - refusing a new registration whose (host, actual database) pair matches an existing one - is not in this: it needs a connection during Add, which is a different surface. - **Editing a server's address keeps its identity, so its collected history stays attached** ([#2158]) - the Add/Edit save re-derived `server_id` from host/database/read-only-intent on EVERY save, so fixing a hostname typo wrote a row under a new id and deleted the old one. That left the registry tidy and every `collect.*` row keyed to the old id orphaned with nothing pointing at it, which is why it went unnoticed: nothing looks broken, the server simply reads as though it had never been monitored. An edit now keeps the row's identity and rewrites the address in place; derivation runs only on Add, where there is no history to lose. **Why assigned rather than derived, argued from consequences rather than preference**: three issues pull on this identity and they pull in opposite directions. This one says a config edit must not change it. [#2228] says two different configs that resolve to the same real database must not be two identities - which no config-derived hash can decide, because the configs genuinely differ. [#2218] says two instances on one host need distinguishing, which wants MORE fields in the derivation and so makes this bug strictly worse. Only one shape satisfies all three: identity is allocated once and never recomputed, the derived address is a lookup key rather than the identity, and what the target actually IS comes from the connection. The groundwork was already in - `StoredServerId` made the store authoritative and `DarlingServerConnector` reads `config.ServerId` rather than re-hashing - so the Viewer's save was the last place identity was recomputed. **Two things the fix had to carry with it.** The collision guard used to look a derived id up in the registry, which only works while every row's id equals the hash of its own address; left alone, an edit could point a second registration at an address another server already monitors and the guard would never fire, because the id it looked up belongs to nobody. It now matches on the ADDRESS columns (`IS NOT DISTINCT FROM` for the nullable database, since `=` never matches NULL and every server-scoped registration would otherwise read as "address free") and compares ids afterwards, which is what still lets a rename or a credential change through. And the darling.json reconcile matched on id alone, so after an edit it would report a server that IS monitored as absent and advise re-adding it - wrong advice, on every start, about the one server the operator had just fixed; it now matches id OR name, either-or rather than name-only so a same-named sibling cannot hide a genuinely unmonitored server. diff --git a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs index 302ae027be..3fb4ebed34 100644 --- a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs +++ b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs @@ -54,7 +54,8 @@ public sealed class PostgresEngineGateBehaviorTests { /// /// The HOST is what identity derives from, which is the trap this test tripped over first: - /// MonitoredServer.StorageName is BuildStorageName(Host, Database, ReadOnlyIntent) — NOT + /// MonitoredServer.StorageName is BuildStorageName(Host, Database, ReadOnlyIntent, Engine, Port) + /// (#2218 added the last two) — NOT /// Name — and RunAnalyzeNowAsync finds a server by hashing that. A server_id hashed from /// anything else simply is not found, and the gate then returns "server not monitored" rather than the /// arm under test. Unique hosts so neither case can collide with a real server's analysis_state row. @@ -74,9 +75,17 @@ depend on test ordering. */ /// the ONLY thing that can decide whether it is dispatched. private const string SnapshotCollector = "wait_stats"; - /// Derived through the SAME helper the worker uses, so the test cannot drift from the lookup. - private static int ServerIdFor(string host) => - ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(host, null, false)); + /// + /// Derived through the SAME helper the worker uses, so the test cannot drift from the lookup. + /// + /// #2218 is that drift happening: the ENGINE joined the derivation, and because this helper hashed + /// without it, a PostgreSQL host produced an id the product never uses — so the gate returned "server not + /// monitored" rather than the arm under test, which is the same trap the class comment above describes for + /// Name vs Host. The engine is defaulted to null so the SQL Server call sites stay unchanged, + /// exactly as the shared helper does it. + /// + private static int ServerIdFor(string host, string? engine = null) => + ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(host, null, false, engine, 0)); [Fact] public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_AndDoesNotRunThePass() @@ -86,7 +95,7 @@ public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_And "Set DARLING_TEST_PG to a Postgres connection string to run the analyze_now engine-gate test."); await using var postgres = NpgsqlDataSource.Create(connectionString!); - var serverId = ServerIdFor(PgHost); + var serverId = ServerIdFor(PgHost, "postgres"); /* Fabricated worker, the CollectorMemoryKnobTests.SweepGate idiom: the real ctor wants a host's worth of dependencies, and the gate under test reads exactly three fields. Reflection because pinning the @@ -268,7 +277,7 @@ public async Task SnapshotNow_AgainstAPostgresTarget_DispatchesNoSqlServerCollec "Set DARLING_TEST_PG to a Postgres connection string to run the snapshot_now engine-gate test."); await using var postgres = NpgsqlDataSource.Create(connectionString!); - var serverId = ServerIdFor(PgSnapshotHost); + var serverId = ServerIdFor(PgSnapshotHost, "postgres"); var bodySucceeded = false; try @@ -356,7 +365,7 @@ private static ServerRuntime PostgresRuntime( ConnectionString = $"Host={PgHost};Database=postgres;Username=monitor", Target = new CollectorTargetInfo { Engine = engine }, StorageName = PgHost, - ServerId = ServerIdFor(PgHost), + ServerId = ServerIdFor(PgHost, "postgres"), }; private static void SetField(DarlingWorker worker, string name, object value) => diff --git a/Darling/Darling.Tests/PostgresTargetConfigTests.cs b/Darling/Darling.Tests/PostgresTargetConfigTests.cs index 20b1bfd548..3e25b03f64 100644 --- a/Darling/Darling.Tests/PostgresTargetConfigTests.cs +++ b/Darling/Darling.Tests/PostgresTargetConfigTests.cs @@ -177,8 +177,18 @@ public void DerivesStorageIdentityThroughTheSharedRule() server.Database = "segments_horizon"; server.ReadOnlyIntent = true; + /* #2218: ":pg" sits between the database and ":RO". A PostgreSQL instance and a SQL Server on one host + used to derive ONE server_id and interleave their histories; the engine token is what separates them, + and its position is fixed so two callers with the same facts cannot produce two names. */ Assert.Equal( - "segments-multi-1.cluster-x.us-east-1.rds.amazonaws.com:segments_horizon:RO", + "segments-multi-1.cluster-x.us-east-1.rds.amazonaws.com:segments_horizon:pg:RO", + server.StorageName); + + /* A port appends after the engine when one is set — the second half of #2218, for two PostgreSQL + instances on one host. */ + server.Port = 6432; + Assert.Equal( + "segments-multi-1.cluster-x.us-east-1.rds.amazonaws.com:segments_horizon:pg:6432:RO", server.StorageName); } diff --git a/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs b/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs index 6bfe39e9c8..0325ad7093 100644 --- a/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs +++ b/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs @@ -245,7 +245,11 @@ public async Task TheLoadedServerCarriesTheStoresOwnId_NotAFreshHash() /* The seed writes the derivation, which is what makes every existing store migration-free. Assert it rather than assume it -- if this ever stops holding, the "no data moves" claim stops holding too. */ - var seededId = Derived("identity-host", "identityDb", readOnlyIntent: true); + /* #2218: derived from the SERVER'S OWN StorageName rather than from a re-statement of the rule here. + This test previously hashed (host, database, readOnlyIntent) by hand, which silently stopped matching + the moment the derivation grew the engine and port the seeded server actually carries — and the + failure reads as "the seed wrote the wrong id" rather than "the test's copy of the rule is stale". */ + var seededId = ServerIdHelper.GetDeterministicHashCode(seeded.StorageName); Assert.Equal(seededId, await ReadServerIdByNameAsync(dataSource, "identity-roundtrip", ct)); /* Now the thing production cannot produce yet: re-key the row so the stored id disagrees with the hash diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs index f4757e3b03..d8a145d112 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs @@ -1172,11 +1172,20 @@ public sealed class MonitoredServer public string DisplayName => string.IsNullOrWhiteSpace(Name) ? Host : Name; /// - /// The canonical storage identity (host[:database][:RO]) — hashed to server_id via the shared - /// ServerIdHelper, so this Darling entry derives the same id Lite would for the same server. + /// The canonical storage identity (host[:database][:pg][:port][:RO]) — hashed to server_id via the + /// shared ServerIdHelper, so this Darling entry derives the same id Lite would for the same server. + /// + /// #2218: engine and port are passed so a PostgreSQL instance cannot collide with a SQL Server on the + /// same host, and two PostgreSQL instances on one host cannot collide with each other. Both are inert for a + /// SQL Server entry and the resulting name is byte-identical to what it was before — Engine folds to + /// no token for SQL Server, and Port is a PostgreSQL-only field that stays 0 there, because SQL + /// Server carries a non-default port inside Host as host,1433 and is therefore already + /// discriminated by the host string. That is why no conditional is needed here: the defaults ARE the + /// backwards-compatible case. /// [JsonIgnore] - public string StorageName => PerformanceMonitor.Common.ServerIdHelper.BuildStorageName(Host, Database, ReadOnlyIntent); + public string StorageName => PerformanceMonitor.Common.ServerIdHelper.BuildStorageName( + Host, Database, ReadOnlyIntent, Engine, Port); /// /// config_monitored_servers.server_id as READ FROM THE STORE, or null for an entry that has no diff --git a/Lite.Tests/ServerIdentityEngineAndPortTests.cs b/Lite.Tests/ServerIdentityEngineAndPortTests.cs new file mode 100644 index 0000000000..217655b8ca --- /dev/null +++ b/Lite.Tests/ServerIdentityEngineAndPortTests.cs @@ -0,0 +1,177 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor Lite. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using PerformanceMonitor.Common; +using Xunit; + +namespace Lite.Tests; + +/// +/// #2218: the storage name carries engine and port, without re-keying anything that already exists. +/// +/// The defect. server_id was derived from host, database and read-only intent only. So a SQL +/// Server and a PostgreSQL instance on ONE host collided into a single identity and interleaved their histories, +/// and so did two PostgreSQL instances distinguished only by port — both of which #2213 made first-class +/// configuration. +/// +/// Why the new discriminators are OPTIONAL, which is the whole reason this is safe. Lite derives +/// server_id FRESH at runtime from this function, everywhere, and has no stored-id fallback: +/// RemoteCollectorService.GetServerNameForStorage hashes it on every read. So any change to what this +/// returns for an existing server re-keys it in Lite and orphans all of its collected history, silently — which +/// is the same class of harm as #2158, arrived at from the other direction. Appending nothing at the parameter +/// defaults is what keeps Lite's three-argument call byte-identical. +/// +/// These tests are therefore mostly about what did NOT change. This file lives in Lite.Tests deliberately: +/// Lite is the SKU with no stored-id safety net, so it is the one whose invariant needs guarding. +/// +public sealed class ServerIdentityEngineAndPortTests +{ + /// + /// The pre-#2218 implementation, verbatim, as the oracle. Comparing against a re-statement of the rule + /// rather than against hard-coded strings is what makes "nothing re-keyed" checkable for any input, not + /// just the handful someone thought to write down. + /// + private static string PreviousImplementation(string serverName, string? databaseName, bool readOnlyIntent) + { + var name = string.IsNullOrWhiteSpace(databaseName) ? serverName : serverName + ":" + databaseName; + return readOnlyIntent ? name + ":RO" : name; + } + + /// + /// THE INVARIANT: Lite's three-argument call is byte-identical to what it produced before, so no Lite + /// server re-keys and no Lite history is orphaned. Asserted on the derived server_id too, because + /// that — not the string — is what the stored rows are keyed by. + /// + [Theory] + [InlineData("SQLPROD01", null, false)] + [InlineData("SQLPROD01", "SalesDB", false)] + [InlineData("SQLPROD01", null, true)] + [InlineData("SQLPROD01", "SalesDB", true)] + [InlineData("host.contoso.com,1433", null, false)] + [InlineData("host,49152", "db", true)] + [InlineData("azure.database.windows.net", "AdventureWorks", false)] + public void TheThreeArgumentCallIsUnchanged_SoNothingReKeys(string host, string? database, bool readOnlyIntent) + { + var expected = PreviousImplementation(host, database, readOnlyIntent); + + Assert.Equal(expected, ServerIdHelper.BuildStorageName(host, database, readOnlyIntent)); + Assert.Equal( + ServerIdHelper.GetDeterministicHashCode(expected), + ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(host, database, readOnlyIntent))); + } + + /// + /// And a SQL Server entry that DOES pass the new arguments is also unchanged — which is what lets Darling + /// pass Engine and Port unconditionally instead of branching at the call site. Port is + /// a PostgreSQL-only field (SQL Server carries a non-default port inside the host as host,1433, so it + /// is already discriminated there), so 0 is the SQL Server case. + /// + [Theory] + [InlineData("SQLPROD01", null, false)] + [InlineData("SQLPROD01", "SalesDB", true)] + [InlineData("host.contoso.com,1433", null, false)] + public void ASqlServerEntryPassingEngineAndPortIsAlsoUnchanged(string host, string? database, bool readOnlyIntent) + { + Assert.Equal( + PreviousImplementation(host, database, readOnlyIntent), + ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, "sqlserver", 0)); + } + + /// + /// An engine that is blank, SQL Server, or unrecognized appends NOTHING. The unrecognized case matters as + /// much as the others: a typo in engine must not mint a fresh identity for a server that already has + /// one, which is exactly what interpolating the raw value would do. + /// + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("sqlserver")] + [InlineData("SQLSERVER")] + [InlineData("SqlServer")] + [InlineData("mysql")] + [InlineData("postgre")] + public void AnEngineThatIsNotPostgresAppendsNothing(string? engine) + { + Assert.Equal("H:D", ServerIdHelper.BuildStorageName("H", "D", false, engine, 0)); + } + + /// THE FIX, half one: PostgreSQL and SQL Server on one host are now two identities. + [Fact] + public void PostgresAndSqlServerOnOneHostNoLongerCollide() + { + var sqlServer = ServerIdHelper.BuildStorageName("box01", null, false, "sqlserver", 0); + var postgres = ServerIdHelper.BuildStorageName("box01", null, false, "postgres", 0); + + Assert.Equal("box01", sqlServer); + Assert.Equal("box01:pg", postgres); + Assert.NotEqual( + ServerIdHelper.GetDeterministicHashCode(sqlServer), + ServerIdHelper.GetDeterministicHashCode(postgres)); + } + + /// THE FIX, half two: two PostgreSQL instances on one host, told apart by port. + [Fact] + public void TwoPostgresInstancesOnOneHostNoLongerCollide() + { + var first = ServerIdHelper.BuildStorageName("box01", null, false, "postgres", 5432); + var second = ServerIdHelper.BuildStorageName("box01", null, false, "postgres", 5433); + + Assert.NotEqual(first, second); + Assert.NotEqual( + ServerIdHelper.GetDeterministicHashCode(first), + ServerIdHelper.GetDeterministicHashCode(second)); + } + + /// + /// Every spelling of the engine folds to ONE token, so an operator writing "PostgreSQL" where a + /// colleague wrote "postgres" does not get a second identity for the same instance — a split history + /// caused by capitalisation, which nothing downstream could diagnose. + /// + [Theory] + [InlineData("postgres")] + [InlineData("PostgreSQL")] + [InlineData("Postgres")] + [InlineData("POSTGRESQL")] + [InlineData("postgresql")] + [InlineData("pg")] + [InlineData("PG")] + [InlineData(" postgres ")] + public void EverySpellingOfPostgresFoldsToOneIdentity(string engine) + { + Assert.Equal("box01:pg", ServerIdHelper.BuildStorageName("box01", null, false, engine, 0)); + } + + /// + /// The suffix order is fixed — engine, then port, then :RO. Two callers supplying the same facts + /// must not be able to produce two names, and the read-only marker stays last so the existing convention + /// (and anything that reads a name by eye) is preserved. + /// + [Theory] + [InlineData("h", "d", true, "postgres", 5433, "h:d:pg:5433:RO")] + [InlineData("h", null, true, "postgres", 0, "h:pg:RO")] + [InlineData("h", null, false, null, 5433, "h:5433")] + [InlineData("h", "d", false, "postgres", 0, "h:d:pg")] + public void TheSuffixOrderIsEngineThenPortThenReadOnly( + string host, string? database, bool readOnlyIntent, string? engine, int port, string expected) + { + Assert.Equal(expected, ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, engine, port)); + } + + /// + /// A zero or negative port appends nothing. Zero is the real default (the field means "the driver's + /// default"), and a negative value is nonsense that must not become part of an identity. + /// + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void AnAbsentOrNonsensePortAppendsNothing(int port) + { + Assert.Equal("h", ServerIdHelper.BuildStorageName("h", null, false, null, port)); + } +} diff --git a/PerformanceMonitor.Common/Services/ServerIdHelper.cs b/PerformanceMonitor.Common/Services/ServerIdHelper.cs index 9ffba5e78c..e5e0521b28 100644 --- a/PerformanceMonitor.Common/Services/ServerIdHelper.cs +++ b/PerformanceMonitor.Common/Services/ServerIdHelper.cs @@ -6,6 +6,8 @@ * Licensed under the MIT License. See LICENSE file in the project root for full license information. */ +using System; + namespace PerformanceMonitor.Common; /// @@ -47,13 +49,81 @@ public static int GetDeterministicHashCode(string value) /// read-write connections to the same host. Extracted verbatim from Lite's /// RemoteCollectorService.GetServerNameForStorage; every SKU MUST build storage names /// through this one implementation so the same server derives the same id everywhere. + /// + /// #2218 — engine and port, and why they are OPTIONAL rather than required. The name carried + /// neither, so a SQL Server and a PostgreSQL instance on one host collided into a single server_id and + /// interleaved their histories, as did two instances distinguished only by port. Both are now discriminators + /// — but only when they are actually present, and that is a correctness requirement, not tidiness. + /// + /// Lite derives server_id FRESH at runtime, everywhere, from this function, and has no stored-id + /// concept to fall back on: RemoteCollectorService.GetServerNameForStorage hashes it on every read. + /// So any change to what this returns for an EXISTING server re-keys that server in Lite and orphans all of + /// its collected history, silently. Making the new parameters optional — and appending nothing at their + /// defaults — is what keeps Lite's three-argument call byte-identical to what it produced before. The same + /// protection covers Darling's SQL Server targets, which pass no port. + /// + /// Darling's already-registered PostgreSQL and explicit-port servers do not re-key either, for a + /// different reason: their id comes from the store (StoredServerId), which is authoritative and is + /// only ever DERIVED for an entry that has no row yet. So the new discriminators change what a FRESH + /// registration derives, never what an existing one is called — which is the property that made #2158 + /// (identity assigned, not re-derived) a prerequisite for this rather than a sibling of it. + /// + /// Engine is folded to a short token rather than interpolated raw so an operator writing + /// "PostgreSQL", "postgres" or "Postgres" gets ONE identity instead of three. Only + /// non-SQL-Server engines append anything, since SQL Server is the historical default and appending for it + /// would re-key every server in both SKUs. /// - public static string BuildStorageName(string serverName, string? databaseName, bool readOnlyIntent) + public static string BuildStorageName( + string serverName, + string? databaseName, + bool readOnlyIntent, + string? engine = null, + int port = 0) { var name = string.IsNullOrWhiteSpace(databaseName) ? serverName : serverName + ":" + databaseName; + /* Engine BEFORE port and both before :RO, so the suffix order is fixed regardless of which + discriminators a caller supplies — two callers passing the same facts in a different order must + not produce two identities. */ + var engineToken = EngineToken(engine); + if (engineToken is not null) + { + name += ":" + engineToken; + } + + if (port > 0) + { + name += ":" + port.ToString(System.Globalization.CultureInfo.InvariantCulture); + } + return readOnlyIntent ? name + ":RO" : name; } + + /// + /// The identity token for an engine, or null when it contributes nothing — which is the case for SQL + /// Server and for an unspecified engine (#2218). + /// + /// Null for SQL Server is load-bearing: it is the historical default, so emitting a token for it + /// would change every existing server's storage name in both SKUs and re-key the lot. Unrecognized values + /// also return null rather than being interpolated raw — a typo must not mint a new identity for a server + /// that already has one, and the engine gate elsewhere already rejects an unknown engine loudly. + /// + private static string? EngineToken(string? engine) + { + if (string.IsNullOrWhiteSpace(engine)) + { + return null; + } + + var trimmed = engine.Trim(); + if (trimmed.StartsWith("postgres", StringComparison.OrdinalIgnoreCase) + || trimmed.Equals("pg", StringComparison.OrdinalIgnoreCase)) + { + return "pg"; + } + + return null; + } }