Add --recompress-plan-dim: convert pre-V54 plan-dim text rows to gzip (#2076) - #2077
Conversation
…#2076) V54 (#2069) gzips new plan content only; existing rows convert by GC attrition, and a STABLE plan's row never retires (every sighting refreshes last_seen) -- so a long-running store keeps a permanent text tail at the lz4 rate. This verb is the operator-paced rewrite the migration deliberately refused to do implicitly, following the --collapse-legacy-slices pattern: runs when a person decides, --dry-run first (which measures the real ratio on a sample of the store's own pending rows before anything is written). No outage: the service stays up throughout. 1,000-row batches, one transaction each, one unnest-array UPDATE per batch; the fetch predicate (text NOT NULL AND gz NULL) is the resume point, so an interrupted run continues from the remainder and a completed one converges to a no-op. Every row's gzip bytes are round-trip verified against the original text before the text is nulled; failures keep their text and are reported (exit 1). Digest and last_seen are both deliberately untouched: identity rides the uncompressed text, and recompression is not a sighting -- stamping the watermark would push GC-eligible rows a full retention window forward. Wiring: verb predicate + IsKnownVerb (the #1581 reflection tripwire covers the classifier automatically), help text, Program dispatch with the Windows/DPAPI guard and --dry-run parse the sibling verbs use. Tests: pure pins (table constant, fetch/update/survey shapes, no last_seen in the update, PG dialect) and a gated live test seeding text + gz rows, then pinning verified conversion, original-text round-trip, untouched last_seen and gz rows, and second-run convergence. Disclosed limit (CHANGELOG): converts live content; the relation's file does not shrink -- freed space is reused internally, and a later VACUUM FULL/repack returns it to the volume if wanted. Closes #2076 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tiny fixtures compressed LARGER than their text (gzip framing beats a 60-char payload), failing the smaller-than-text assertion. Real plans are KBs; the fixtures now carry ~6 KB of operator-block bulk so the assertion tests the property it names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /* Every fetched row either converted or failed verification. If ALL failed, stop rather than | ||
| spin on the same rows forever — the fetch would return them again. */ | ||
| if (good.Length == 0) |
There was a problem hiding this comment.
Correctness: a single fully-failing batch permanently stops the whole run, not just that batch — and re-running doesn't recover.
FetchBatchSql has no ORDER BY, and nothing in the loop excludes previously-attempted digests, so if a batch of up to 1,000 rows all fail round-trip verification, good.Length == 0 and the method breaks out of the entire ConvertAsync call — not just that batch. If those failing rows happen to be physically clustered early in the heap (e.g. a bulk-loaded batch that shares some encoding quirk causing lossy UTF‑8 round-tripping, such as unpaired surrogates), every subsequent pending row in the table — potentially millions of good, convertible rows — is never even attempted, in this run or any future run, because:
query_plan_xml IS NOT NULL AND query_plan_gz IS NULLstill matches the failed rows (they were never converted), and- with no
ORDER BY, a plain re-fetch after a restart will very likely return the same first-scanned batch again, fail the same way, and abort again at the exact same point.
This contradicts the doc comment on ConvertAsync ("Safe to interrupt and re-run at any point") and the CLI's own guidance ("Investigate before re-running") — re-running doesn't make progress past the bad cluster, it just repeats the same abort.
Suggested fix: don't abort the whole run on a fully-failed batch. Either (a) continue the loop past it while tracking already-verify-failed digests to exclude from subsequent fetches within the same run (e.g. AND NOT digest = ANY($2::bytea[])), or (b) only break when no progress at all has been made across several consecutive attempts, rather than after the very first all-bad batch. As written, one contiguous bad cluster can silently cap conversion of the whole dimension.
| await using var transaction = await connection.BeginTransactionAsync(cancellationToken); | ||
| await using var update = new NpgsqlCommand(UpdateBatchSql, connection, transaction); | ||
| update.Parameters.Add(new NpgsqlParameter | ||
| { | ||
| NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Bytea, | ||
| Value = good.Select(row => row.Digest).ToArray(), | ||
| }); | ||
| update.Parameters.Add(new NpgsqlParameter | ||
| { | ||
| NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Bytea, | ||
| Value = good.Select(row => row.Gzip).ToArray(), | ||
| }); | ||
| await update.ExecuteNonQueryAsync(cancellationToken); |
There was a problem hiding this comment.
Minor: unlike other long-running batch operations in this project (PgMigrations.MigrationCommandTimeoutSeconds, TimescaleSupport.SetupTimeoutSeconds/BackfillTimeoutSeconds, RollupBackfill.SliceTimeoutSeconds, DarlingRetention.DeleteTimeoutSeconds), neither the fetch nor this batch UPDATE sets an explicit CommandTimeout, so both fall back to Npgsql's 30s default. A 1,000-row bytea-array UPDATE moving up to ~130 MB (per the BatchSize doc comment above) plus the full-table COUNT(*)/pg_total_relation_size scan in SurveyAsync on a 100+ GB dimension could plausibly exceed 30s under load/contention, turning a routine batch into a spurious "Conversion failed mid-run" error. Worth an explicit elevated timeout to match the convention used elsewhere for comparable batch/DDL work.
Review:
|
1 participant
Closes #2076.
What
An optional operator verb for stores that collected before V54: converts the plan dimension's existing text rows to the gzip form new plans already use (#2069), in bounded batches, while the service runs — no stop, no outage.
--dry-runsurveys the store and measures the real compression ratio on a sample of its own pending rows before anything is written.Why a verb (and not the migration)
Attrition converts only rows that die, and a stable plan's row never dies — every sighting refreshes
last_seen, so a long-running store keeps a permanent text tail at the lz4 rate (~15 KB/plan vs ~10 KB measured). Rewriting the store's largest table is an operator decision (--collapse-legacy-slicesrationale), not a migration side effect.Safety properties
query_plan_xml IS NOT NULL AND query_plan_gz IS NULL) is the resume point; each 1,000-row batch is one transaction; a second run converges to a no-op (pinned live).last_seenuntouched: identity rides the uncompressed text (Gzip the plan-XML dimension content at the application level — measured 14.0x vs lz4's 8.9x on live content #2069's rule), and recompression is not a sighting — stamping the watermark would silently extend retention (pinned: the UPDATE contains nolast_seen).ON CONFLICT ... SET last_seen; the UPDATE re-checksquery_plan_xml IS NOT NULLunder the row lock.Wiring
Verb predicate +
IsKnownVerb(the #1581 reflection tripwire picks up the classifier by name), help text, Program dispatch with the same Windows/DPAPI guard and--dry-runparse as the sibling verbs.Tests
Pure pins: table constant is the one compressed-content dim, fetch/update/survey statement shapes, one-unnest-statement batch, no
last_seenin the update, PG dialect. Gated live test: seeds three text rows (incl. non-ASCII) + one gz row, then pins verified conversion, original-text round-trip throughDecompressContent, untouchedlast_seen(t0) and gz bytes, and second-run convergence.Disclosed limit
Converts live content — the relation file does not shrink (PostgreSQL reuses the freed space internally). Returning space to the volume is a separate one-time
VACUUM FULL/repack, the operator's call. New installs never need this verb.🤖 Generated with Claude Code