--recompress-plan-dim ends with VACUUM FULL so the saving reaches the volume (#2076) - #2078
Conversation
… volume (#2076) Shipping the conversion without the compaction leaves operators with a file parked at its high-water mark and a df that never moved -- the freed space is real but internal, and "go discover VACUUM FULL" is not a process. When the conversion CONVERGES with zero verify failures the verb now rewrites the dimension to its live content: - Preflighted against free disk on the store's own volume (the sampled compacted-size estimate x1.2 headroom; the reused --backfill-rollups free-space resolver). Refused with numbers when it will not fit or the volume is unmeasurable -- the conversion stays committed either way, and --vacuum-full retries the compaction alone. - Disclosed as the one exclusive-lock step: collections queue for its duration (typically minutes); the service does not stop. - --no-vacuum-full skips it for operators scheduling the lock window separately; --vacuum-full compacts an already-converted store (the fleet store converted by the pre-compaction build is exactly this). - Mid-run arrivals leave pending rows: compaction defers until a run actually converges, and says so. - Flag grammar is a PURE parser (ParseRecompressArgs) that REFUSES unknown --flags instead of treating a typo as a config path. Tests: VacuumFullSql targets exactly the plan dim; the estimate SAMPLES the gzip average (LIMIT) rather than detoasting the whole dimension; the parser grammar pins including the typo refusal; the live test now runs VACUUM FULL after convergence and proves a converted row still decompresses to its original text after the rewrite. Closes #2076 (follow-up) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (after.Total > 0 && after.Pending > 0) | ||
| { | ||
| output.WriteLine($" {after.Pending:N0} row(s) arrived mid-run and are still text — re-run to convert them;"); | ||
| output.WriteLine(" compaction will run when the conversion converges."); | ||
| return 0; | ||
| } | ||
|
|
||
| return await CompactPlanDimAsync(connection, output, error, cancellationToken); |
There was a problem hiding this comment.
A failed post-run survey silently becomes "converged, nothing pending" rather than "unknown state."
A few lines up (~3276), when SurveyAsync throws, the catch sets after = default (Total = 0, Pending = 0) and only logs to error. Execution then reaches this guard:
if (after.Total > 0 && after.Pending > 0) // false when `after` is the default struct
{
...
return 0;
}
return await CompactPlanDimAsync(connection, output, error, cancellationToken);Since after.Total > 0 is false for the default struct, the "rows arrived mid-run, don't compact yet" guard is skipped and control falls straight into CompactPlanDimAsync (VACUUM FULL) as if the conversion had cleanly converged — even though we actually don't know whether new text rows arrived during the run, because the check that would tell us failed. Given VACUUM FULL takes an ACCESS EXCLUSIVE lock and this PR is otherwise careful to refuse rather than assume ("an unmeasurable volume refuses rather than shrugs"), treating a failed post-run survey the same as a successful zero-pending result seems like the wrong default. Consider returning a non-zero exit when the post-run survey fails, instead of falling through to compaction.
| public const string EstimateCompactedSql = """ | ||
| SELECT | ||
| pg_relation_size('query_plan_dim') | ||
| + pg_indexes_size('query_plan_dim') | ||
| + (SELECT COUNT(*) FROM query_plan_dim) | ||
| * COALESCE((SELECT AVG(octet_length(query_plan_gz))::bigint | ||
| FROM (SELECT query_plan_gz FROM query_plan_dim | ||
| WHERE query_plan_gz IS NOT NULL LIMIT 500) AS sample), 0) | ||
| """; |
There was a problem hiding this comment.
The sample feeding the disk-preflight average is LIMIT 500 with no ORDER BY/TABLESAMPLE — it's whatever 500 rows Postgres happens to return first (typically physical/insertion order), not a random cross-section. If plan size correlates with insertion order in any way (e.g. digests from earlier SQL Server versions/collection eras behaving differently than recent ones, or the oldest-surviving digests being systematically the smallest/simplest "always-stable" plans), the average gzip size — and therefore the compacted-size estimate that gates the "REFUSING: not enough free disk" check — could be systematically off in either direction.
Underestimating is the concerning direction: the 1.2x headroom is a fixed multiplier on a potentially-biased base, so a lowball estimate could let CompactPlanDimAsync proceed into a multi-hour VACUUM FULL that then runs the volume out of space mid-rewrite — a much messier failure mode than the "REFUSING" preflight this code is designed to hit instead. Worth considering TABLESAMPLE SYSTEM/BERNOULLI or ORDER BY random() for the sample, or at least a larger headroom multiplier given the estimate's only "one significant digit" per the doc comment.
| if (vacuumMode == RecompressVacuumMode.Force && !dryRun) | ||
| { | ||
| return await CompactPlanDimAsync(connection, output, error, cancellationToken); | ||
| } | ||
|
|
||
| output.WriteLine(" (Pass --vacuum-full to compact a previously-converted store whose file still"); | ||
| output.WriteLine(" sits at its pre-conversion high-water mark.)"); | ||
| return 0; |
There was a problem hiding this comment.
Minor UX inconsistency: if the store is already fully converted and the user passes --dry-run --vacuum-full together, vacuumMode == Force && !dryRun is false (because dryRun is true), so this falls through to printing "(Pass --vacuum-full to compact a previously-converted store...)" — even though the user already passed --vacuum-full. The hint should probably acknowledge that dry-run is what's suppressing the compaction here, rather than suggesting a flag that's already set.
| var before = (await PlanDimRecompression.SurveyAsync(connection, cancellationToken)).RelationBytes; | ||
| output.WriteLine(); | ||
| output.WriteLine($" Compacting (VACUUM FULL): rewriting ~{estimated / (1024.0 * 1024 * 1024):N1} GB of live content."); | ||
| output.WriteLine(" This takes an EXCLUSIVE lock on the plan dimension — collections queue until it"); | ||
| output.WriteLine(" finishes (typically minutes; the service does not need to stop)."); | ||
|
|
||
| var stopwatch = System.Diagnostics.Stopwatch.StartNew(); | ||
| try | ||
| { | ||
| await PlanDimRecompression.VacuumFullAsync(connection, cancellationToken); | ||
| } | ||
| catch (Exception ex) when (ex is not OperationCanceledException) | ||
| { | ||
| error.WriteLine($" VACUUM FULL failed: {ex.Message}"); | ||
| error.WriteLine(" The original relation is untouched (the rewrite is transactional); the conversion"); | ||
| error.WriteLine(" is committed. Re-run with --vacuum-full to retry the compaction."); | ||
| return 1; | ||
| } | ||
|
|
||
| var after = (await PlanDimRecompression.SurveyAsync(connection, cancellationToken)).RelationBytes; | ||
| output.WriteLine($" COMPACTED in {stopwatch.Elapsed:hh\\:mm\\:ss} — " + | ||
| $"{before / (1024.0 * 1024 * 1024):N1} GB -> {after / (1024.0 * 1024 * 1024):N1} GB on disk."); |
There was a problem hiding this comment.
The SurveyAsync calls used for the before/after size report (lines 3363 and 3382) aren't wrapped in try/catch, unlike every other database call in this method (config load, connect, survey, sample, convert, estimate, free-space resolve all catch and print a friendly message). If the "after" survey throws here — e.g. a connection hiccup right after a multi-hour VACUUM FULL finally completes — the exception propagates unhandled out of RecompressPlanDimAsync (there's no top-level catch in Program.cs), so the operator gets a raw stack trace instead of the "COMPACTED" confirmation, even though the compaction itself actually succeeded. Not a data-safety issue, but it breaks the otherwise-consistent error-handling pattern in this file for what should be a purely cosmetic reporting step.
Review summaryFollow-up to #2076/#2077, adding a closing Left 4 inline comments, roughly in order of concern:
No SQL injection, secrets, or missing-index-DMV concerns — all statements here are static consts with no user-input interpolation. |
1 participant
Follow-up to #2077, per the design decision that the shipped process for existing installs must include the compaction — not leave operators with a file at its high-water mark and a df that never moved.
What changes
When the conversion converges with zero verification failures, the verb now finishes with
VACUUM FULL query_plan_dim, rewriting the dimension to its live content and returning the freed space to the volume.--backfill-rollupsfree-space resolver. Refusal comes with the numbers; an unmeasurable volume refuses rather than shrugs. The conversion stays committed either way.--no-vacuum-full: skip, for operators scheduling the lock window separately.--vacuum-full: compact an already-converted store whose file still sits at the high-water mark (the production fleet store, converted by the pre-compaction build, is exactly this case).ParseRecompressArgs) that refuses unknown--flags— a typo like--vaccum-fullmust not be silently treated as a config path.Tests
VacuumFullSqlpins to exactly the plan dim (a broader VACUUM FULL would rewrite hypertables TimescaleDB manages itself); the estimate pins its LIMIT-sampling; the parser grammar pins including the typo refusal; the live test runs the realVACUUM FULLafter convergence and proves a converted row still decompresses byte-for-byte after the rewrite.🤖 Generated with Claude Code