Optimize PostgreSQL metadata passthrough - #31
Conversation
8a2bff8 to
a2acfd3
Compare
bill-ph
left a comment
There was a problem hiding this comment.
The redesign is substantially better: this now targets posthog/v1.5.3, uses the existing passthrough manager, and removes the parallel adapter/factory/version matrix. The table-size override also looks appropriately contained.
I am still requesting changes for two reasons:
- PostgreSQL expiration is currently broken for dead inlined tables; the catalog CI failures reproduce the invalid
DROP TABLE IF EXISTS %I.%Icommand. - The full
DeleteSnapshotsoverride still duplicates cleanup policy. A merge with currentupstream/v1.5-variegatais textually clean, but it already misses the new inlined-file-deletion-table cleanup from upstream commitc812f6e6. This is exactly the silent semantic drift periodic syncing needs to avoid.
Textual sync cost is much improved: the PR adds no current v1.5 conflicts and only two conflict paths beyond the existing fork when compared with upstream main. The remaining sync risk is concentrated in expiration policy. Please fix the concrete failure and either derive the PostgreSQL batch from shared cleanup builders/policy or add a comparably robust parity mechanism before merging.
|
Addressed the review in d20f1d3: PostgreSQL now overrides only the narrow snapshot-row deletion primitive, while the shared DeleteSnapshots coordinator owns all cleanup policy, paths, inline tables, dependencies, and cache invalidation. This removes the invalid %I batch entirely and incorporates the upstream c812f6e inlined-delete-table cleanup in the shared path. Added PostgreSQL parity coverage for relocated relative paths and physical inline data/delete tables, plus the upstream regression. Fresh post-fix benchmark artifacts are in e6848cd and the PR description has been updated. @bill-ph could you please re-review? |
|
CI follow-up: the two red catalog jobs were caused by the test gate declaring DUCKLAKE_POSTGRES, which postgres.json already defines. ecf471c switches the test to the established DUCKLAKE_CI workflow gate used by the neighboring PostgreSQL tests. The exact parser path now gets past the former duplicate-variable error, and the focused DEV PostgreSQL run passes all 49 assertions. Fresh CI is running on ecf471c. All four outdated review threads are resolved. |
Summary
DuckLakeMetadataManager::DeleteSnapshotscoordinator as the sole owner of cleanup policyThis is based on
posthog/v1.5.3(49ec0dc8). The implementation uses the fork's existingPassthroughQuery/PassthroughExecutearchitecture and v1.5 transaction state. It does not add aPostgresFastmanager or restore a second commit architecture.Following review, PostgreSQL now overrides only the narrow
DeleteSnapshotRowsprimitive. The generic cleanup coordinator handles everything else, so future upstream cleanup-policy changes apply automatically. This also incorporates upstreamc812f6e6's cleanup of physical inlined-delete tables and adds parity regressions for both inline table types and relocated relative paths.Xlarge benchmark results
Rerun in-cluster against PostgreSQL 18.3 after the review fixes at
d20f1d38. Every trial records actual per-table fixture counts before timing and logical result cardinality. The committed artifact contains 384 unique, alternating trials: 192 pristine v1.5.3 and 192 feature trials.Unchanged operations remain within approximately 4% and are treated as noise. Full methodology and per-table row-count breakdowns are in
benchmark/postgres/RESULTS.mdandbenchmark/postgres/results/release.jsonl.Validation
posthog/v1.5.3: 95,310 assertions in 468 test cases passed (11 skipped)(variant, operation, scale, trial)keys, zero errors/duplicates; every aggregate fixture count equals its stored per-table breakdownThe PostgreSQL test config sets the regression's environment gate so PostHog catalog CI runs it with the official v1.5.3 PostgreSQL extension.