Repository navigation
fix(frontend): replay publications after snapshot database restore - #30675
Conversation
…arser (matrixorigin#30607) RESTORE DATABASE ... FROM PITR reads the restored database's publications through getPubInfoWithPitrFormat, which projected mo_pubs as (pub_name, database_name, ...). createPubByPitr then parsed the rows with extractPubInfosFromExecResult, which expects the 12-column layout starting with (account_id, account_name). The pub name landed in the first column and restore aborted with "strconv.ParseInt: parsing \"<pub_name>\": invalid syntax", so a published database could never be restored via PITR (while the same restore succeeded for unpublished databases). Make the PITR query project the same 12-column layout as the extractor, matching getAllPubInfosBySnapshotName's already-correct snapshot query. Regression tests pin the projection against getPubInfoSql and verify a realistic row parses into the right PubInfo fields; both fail on main.
…atrixorigin#30610) RESTORE DATABASE <db> {snapshot=...} drops the database before recreating it, and that drop erases the mo_pubs rows targeting the database: every subscriber then breaks permanently on 'there is no publication' while 'show subscriptions' still lists the dangling databases. The PITR path rebuilds the publications after its database restore (createPubByPitr), so the two restore stacks disagreed and only the snapshot one lost data. Replay the snapshot-time publications at the end of the database restore in both snapshot stacks — the clone-based restoreToDatabaseOrTable (the RESTORE DATABASE {snapshot} mainline) and the from-TS restoreDatabaseFromTS — through a shared wrapper that: - reuses the PITR 'read mo_pubs at {MO_TS} / re-CREATE PUBLICATION' semantics, replaying with the stored owner and creator; - stays silent on cross-account restores: the source account kept its database and publications there, so replaying would collide; - never runs for table-level restores, which do not drop the database and its publications in the first place. Verified live on a fresh single-node instance with the issue flow (account acc1, publication pubS -> acc1, subscriber spb_sub, snapshot, post-snapshot insert, restore): the publication row survives, the data rolls back, and the subscriber keeps serving. Two complete independent cycles stay green. Existing snapshot-restore UT fixtures learn the new mo_pubs projection query; a new wrapper UT pins the same-account replay, the cross-account no-op and error propagation.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Evidence at head
|
|
mo-bot review · Commit: Review completed. Read the assessment. |
|
mo-bot review · Commit: "interrupted" |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: a8dd8082ae563fb8f779ad41ba5210e2426a23d6
Assessment: request_changes
Reviewed the complete delta and supplied comments. Replay ownership and backend reuse are reasonable; prior quoting, skipped-database and empty-list concerns are addressed. One reproduced regression remains: explicitly publishing an external table prevents database restore.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
- P2
pkg/frontend/snapshot_restore_with_ts.go:566: Create a database containing an ordinary table and external table ext_t, publish TABLE ext_t ACCOUNT ALL, take a database snapshot, then restore it.
Impact: Bulk restore deliberately skips external tables, but publication replay validates the unchanged historical selection. The missing ext_t aborts and rolls back the entire restore, preventing recovery of ordinary tables. Reconcile replay with the external-table exclusion policy while preserving explicit empty selections, and add regression coverage.
Evidence: Real SQL probe TestReviewSnapshotExternalPublication/external_selection fails on the candidate with "table 'ext_t' not exists", leaving two ordinary rows after rollback. The identical merge-base probe succeeds, restoring one row. Candidate controls with no publication, ALL tables, and an ordinary-table selection all pass. Retained logs: external-candidate.log and external-base.log.
Validation:
-
Reviewed all 15 changed files, restore/PITR/cluster callers, transaction cleanup, authorization, serialization, and test contracts.
-
Isolation preflight passed. Verified 192 native inputs against candidate and base, plus seven reused build outputs against the build owner's receipt and log.
-
Candidate probe: three controls passed, external selection failed; checks/1792596324791694213-20.json records exit 1. Base probe: all four passed; checks/1792596383849604362-3.json records exit 0.
-
Reconciled commands, clean source fingerprints, logs, identical probe bytes and base overlays. Evidence retained under /home/xupeng/.local/state/mo-bot/jobs/148480debcda396ee9412082/attempt-1, including review-evidence-audit.json.
-
Frozen exact-head CI reports successful Ubuntu UT, Linux/arm64 SCA, shared build and proxy/pessimistic BVT.
-
Performance tracing found added catalog/subscriber work confined to publication replay; ordinary SELECT/DML paths gain no work.
Limitations:
-
The focused reproduction used single-CN real SQL; no additional race, multi-CN or performance benchmark runs were performed.
-
local_validation contained no retained author gates; raw CI logs and the linked issue body were absent. CI conclusions do not establish coverage of the reproduced boundary.
|
Addressed the external-table replay P2 in 3be8d01. The replay helper now reads source-tenant table metadata at the restore timestamp and applies the existing bulk-restore exclusion policy to explicit publication selections. Only intentionally omitted external-table names are removed. Missing ordinary tables still fail normal creation validation, and historical-read errors abort before catalog writes. An external-only selection remains Reproduced the reported failure with real tenant publisher/subscriber SQL on the reviewed production source: external-only and mixed selections abort RESTORE DATABASE with Validation on the final source (Go 1.27.1, darwin/arm64, repository-native provenance verified):
Local SQL uses the single-CN fixture; no full-repository, distributed-BVT, cluster-restore SQL or throughput benchmark is claimed. Both this PR's reviewed head and #30719's reviewed head had successful applicable CI when inspected, so there was no failed old run to retry. The new push has triggered fresh CI; its results are pending, not claimed green. Please re-review. |
|
Merged current main (
Updated the upstream PITR mode-resolution fixture to include the historical table read that now precedes mode resolution, retaining the exact rejection and adding an exact no-write query assertion. Unit tests still cover external-only/mixed/unknown selections and source metadata immutability. Extended the existing real MySQL restore fixture, without another cluster, with two view-scope controls. A complete view/table dependency set restores twice and remains queryable by the subscriber. A publication whose view was subsequently redefined to require an unselected private table is rejected twice; the two live rows and publication selection remain unchanged after rollback. The previous 17 restore/subscriber scenarios remain covered, including external-only and empty selections. Validation on Go 1.27.1 / darwin-arm64, using the repository wrapper and freshly rebuilt, provenance-verified native artifacts:
Local SQL validation uses the canonical single-CN fixture; no new distributed BVT, cluster-restore SQL, full-repository UT or performance benchmark is claimed. The latest external-table reviewer finding was already repaired in |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 45730bd08bd20183f70fe2785dbbcf8844ad917b
Assessment: request_changes
Reviewed the complete 16-file delta and restore/publication callers. Coordinator-owned replay, recreation gating and reuse of the creation backend are sound; earlier reported defects are addressed. One confirmed authorization regression remains: replay can publish a different, previously private table by lowercasing its saved name.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
- P1
pkg/frontend/publication_replay.go:26: With lower_case_table_names=0, create distinct tables PublishedT and publishedt, publish only PublishedT to another account, snapshot the database, then RESTORE DATABASE.
Impact: The hard-coded parser mode 1 changes PublishedT to publishedt. Restore succeeds but grants the subscriber access to the previously private lowercase table. Without that colliding table, restore instead fails and rolls back. Preserve catalog identifier bytes during replay and add case-sensitive collision and noncollision regressions.
Evidence: TestReviewPublicationCaseReplayV3/case_collision verified private-table access was denied before restore, then observed table_list="publishedt" and a successful subscriber read of private value 901 afterward. The noncollision case failed with "table 'publishedt' not exists" and retained two live rows. Both controls passed against the merge-base production overlay. Candidate receipt: checks/1792648580367420366-3.json (exit 1); base receipt: checks/1792648610039050219-3.json (exit 0). Logs: candidate-case-v3.jsonl and base-case-v3.jsonl.
Validation:
-
Artifacts are retained under /home/xupeng/.local/state/mo-bot/jobs/bac1210cab099de45d8da8be/attempt-1. evidence-audit.json reconciles commands, source fingerprints, exit receipts, logs and overlay hashes; source remained unchanged.
-
Isolation preflight passed. After the repository wrapper rejected incompatible host artifacts, make cgo built frozen-source prerequisites. Independently verified 194 native inputs and 13 output hashes.
-
Focused frontend/pubsub tests passed: focused-tests.jsonl, receipt checks/1792648532928084812-3.json. All 19 added real-client SQL scenarios passed in 12.916s: existing-sql-tests.jsonl, receipt checks/1792648600425470836-3.json.
-
The final four-case public-SQL probe reproduced two consequences of the identifier defect; unpublished and ordinary-name controls passed. All four cases passed against verified merge-base production sources.
-
git diff --check passed. Performance tracing found additional historical metadata scans and publication/subscriber work confined to restore; ordinary SELECT/DML paths gain no work.
Limitations:
-
Local SQL execution used one embedded CN. Frozen CI metadata reports successful UT, coverage, static analysis and multi-CN BVT checks; raw CI logs and retained author-validation receipts were not supplied.
-
No local race, full-repository suite or performance benchmark was run. Initial probe setup failures were retained separately and excluded from the finding.
-
The linked issue body was not supplied; its contract was assessed through the frozen PR description, comments and regression tests.
|
@XuPeng-SH Addressed the case-sensitive publication replay P1 from review 5609853967 in commit e653b52. Replay now constructs the internal CREATE PUBLICATION AST directly from canonical catalog fields for every selection shape. It no longer formats SQL and reparses it with hard-coded mode 1. This preserves identifier bytes without changing the ordinary backend's table/view scope validation, historical principals, subscriber handling, or ALL-versus-empty semantics. The new real-client SQL regressions cover both Local validation:
Transparency: the first full frontend run hit the unchanged Both PRs' required CI was green before these review fixes. The pushed commit starts fresh checks; the local results above are not a claim that those new checks have completed. |
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 1eadf5008d3e984f50a07c9548d9e32aabad594b against base/merge-base 6cbe375b49696021d72778baa292a66287f080bd, including the increment from my last audited head 4c453e7f5ae5e08497b3b2db76ffe05e4e151d41. Read all16 changed files,9 historical reviews,22 discussion comments,2 inline comments/author reply and the unresolved thread. No remaining blocking finding.
My previous empty-authorization counterexample was rerun byte-for-byte: restore now succeeds and restores1 row (1.46s). Also verified all21 real-client scenarios: empty grants deny old/new subscribers until reauthorized; empty table selections stay empty; external-only/mixed filtering never widens to ALL; case-sensitive collision/noncollision preserves selected names and denies private-table access; incomplete view scope fails atomically. Comment/quoted-identifier and external-FK skip repairs remain covered. The old inline thread is left unresolved in GitHub, but its failure is demonstrably fixed.
Traced coordinator recreation ownership, FK/view ordering, source timestamp/tenant and historical principals, ordinary creation validation, subscriber rebinding, rollback/locks and AST cleanup. Each helper frees its AST on success/error; restore failure reaches the existing bounded transaction cleanup. Public grammar/authorization checks are not bypassed.
Independent exact-head evidence:35 focused frontend/pubsub roots pass; canonical LOAD + unchanged prior counterexample + all21 SQL scenarios pass in one process (23.948s); probe vet, formatting and diff checks pass. Reused verified CI39749370567: SCA/UT and complete non-race coverage producer exit0 with no failure event, plus both compose BVT groups. Raw PROXY/mo-tester results explicitly show the new regression102/102, restore_pub_sub354/354, and cluster/privilege controls all passing. No author-only execution is relabeled as mine, and no new physical archive/upgrade/performance claim is made.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 1eadf5008d3e984f50a07c9548d9e32aabad594b
Assessment: request_changes
Reviewed the complete 16-file delta and historical concerns. Coordinator ownership, shared creation validation, and authorization preservation are sound, but replay misses an existing intentional view omission and introduces a confirmed database-restore regression.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
- P2
pkg/frontend/publication_replay.go:86: Create a valid FULLTEXT view v_ft over docs, publish TABLE docs,v_ft, drop its FULLTEXT index, take a database snapshot, modify docs, then RESTORE DATABASE from that snapshot.
Impact: Restore deliberately skips the now-unusable view, but publication replay removes only external tables from its selection. Validation then rejects the missing v_ft and rolls back the entire database restore. Propagate the restore owner's known omitted-view set into replay while retaining rejection of unexpectedly missing objects.
Evidence: The identical real-client probe passes on merge-base 6cbe375 and restores one row. The candidate returns "table 'v_ft' not exists" and retains two rows after rollback; no-publication and ALL-tables controls pass. Retained under /home/xupeng/.local/state/mo-bot/jobs/b0b646bd7887a4db8cdbfb09/attempt-1: review_unservable_publication_test.go, candidate-unservable-probe.jsonl, base-unservable-probe.jsonl, and the audited exact-base overlay. Receipts: 1792667728926398164-9.json (candidate exit 1), 1792667832381642605-21.json (base exit 0).
Validation:
-
Command/source/exit receipts, retained logs, probe hashes, and base overlays reconciled in attempt-1/review-evidence-audit.json; audit receipt 1792667910986605084-3.json passed. Source remained clean.
-
Isolation preflight passed. Verified current native prerequisites against 192 input files and 46 retained output hashes.
-
All 21 added SQL scenarios plus the existing LOAD regression passed, covering case-sensitive access, empty grants/selections, external tables, quoted names/comments, repeated restore, and scope-validation rollback.
-
Independent cluster and PITR client probes passed, including recreated publisher account IDs, current database IDs, and subscriber access restrictions.
-
Frontend: 2,582 root tests passed, one skipped, and one unrelated export test failed because the checkout is read-only. That test passed with an audited test-only output-path overlay. All 11 pubsub roots and 11 focused frontend race roots passed.
-
Configured incremental golangci-lint reported zero issues; changed Go formatting and git diff checks passed.
Limitations:
-
No local multi-CN run or performance benchmark was performed. Historical table enumeration adds restore-only work per explicit publication; ordinary SELECT/DML paths are unchanged.
-
Frozen CI statuses and author summaries were contextual evidence, not substitutes for audited execution. The race selector exercised frontend tests only.
|
Fixed the omitted-view replay regression in Publication replay now receives the restore coordinator's database-qualified set of views deliberately omitted as unservable. It filters a historical selection only when the historical object is a view in that set (or an external table already intentionally skipped by bulk restore). Unexpectedly missing objects still reach ordinary publication validation and fail. ALL, empty selections, authorization, source tenant/timestamp and ordinary CREATE validation are unchanged. Runtime-discovered unservable views are recorded by the view owner too. The real-client regression creates a valid FULLTEXT view and publication/subscription, drops its FULLTEXT index before the snapshot, and restores twice. It verifies restored rows, omitted view metadata, the retained publication selection, subscriber access, and denial after the omitted view's name is reused. No-publication and ALL controls are included. Validation on this change:
The new head is mergeable and fresh CI has started. These are local embedded real-client regressions, not a claim of multi-CN or physical cross-version archive testing. Please re-review. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 1180cd9361046a1b7cd52560d4e0a7c0c7cba28a
Assessment: approve
Reviewed the complete 16-file delta, callers and supplied review concerns. Coordinator-owned replay, explicit recreation gating and reuse of publication validation are coherent. The omitted-view repair preserves authorization, empty selections and rejection of unexpectedly missing objects. No actionable findings remain. Added catalog work is confined to restore paths.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
Validation:
-
Verified frozen head, clean source, complete patch, commands, source fingerprints, terminal receipts and retained logs. Audit: attempt-1/review-audit.json; receipt checks/1792676699570174695-4.json, exit 0.
-
Isolation preflight passed. Independently verified 192 native inputs and 46 retained build outputs before reuse.
-
37 focused frontend/pubsub test roots passed: focused-tests.jsonl; receipt 1792676266108955696-3.json.
-
All 24 real-client publication replay scenarios plus the existing LOAD regression passed on the shared single-CN fixture, including repeated restore, subscriber restrictions and omitted-name reuse: sql-tests.jsonl; receipt 1792676358468921535-3.json.
-
Owning-package execution passed 2,594 test roots, with one existing skip and one export-test failure caused by writing into the read-only checkout. That unchanged test passed with a path-only overlay directing output to private /tmp; receipts 1792676382469669881-3.json and 1792676472696084313-5.json.
-
Reconciled retained cluster/PITR probe receipts, logs and diagnostic hashes. Their tested external-selection and identity-rebinding paths remain semantically unchanged by this revision.
-
Formatting and diff checks passed. Frozen current-head CI reports successful UT, coverage, SCA and both multi-CN BVT lanes.
Limitations:
-
Raw CI logs, retained author gates and the separate linked issue body were not supplied; author summaries were not treated as independently audited executions.
-
No new local race, multi-CN, BVT-harness or performance run was performed. Local public SQL validation used one CN; no throughput or restore-latency improvement is claimed.
|
@mergify refresh |
✅ Pull request refreshed |
|
Investigated run 39797131945 / UT job 119887273624 at exact head There are three independent failing tests, not a coverage-threshold failure:
The checkpoint telemetry records no OOM kill. Coverage UT (exit 0), coverage aggregation, SCA, and both required BVT jobs passed at the failing head. No publication-replay functional failure is reported by this job. Local verification (macOS arm64, not the Linux/x86 runner):
Only the morpc test fixture was changed. No SQL expectation was relaxed, no test was skipped, and no global/production cluster timeout was increased. A fresh full CI result is still required; these local results do not establish that the new head is CI-green. |
Merge Queue Status
This pull request spent 1 hour 6 minutes 3 seconds in the queue, with no time running CI. ReasonPull request #30675 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #30610
What this PR does / why we need it:
RESTORE DATABASE <db> {snapshot="..."}drops and recreates the database, removing its publication rows without restoring them. The publisher's data returns but existing subscribers fail withthere is no publication.Replay snapshot-time publications at the top-level database restore coordinator, after ordinary tables, deferred foreign-key tables and views have finished. Reuse the PITR publication recreation path so the current database identity is resolved and the stored publication principals are retained. Keep the same-account guard; table and cross-account restores do not replay publications.
Do not replay publications inside shared database workers. Account/cluster restores also call those workers; doing so changed account behavior and collided with the cluster's existing final publication phase. Cluster restore continues to recreate publications once, followed by subscriptions and identity-bearing catalogs. This PR does not change the existing account-restore publication contract or suppress duplicate-key errors.
Update only the full-text database-restore golden that expected the missing-publication defect. Drop the restored publication before fixture database cleanup, preventing leaked databases from breaking later SHOW DATABASES tests. Add a public SQL regression with a real subscriber, foreign-key tables, a view, table-only restore and repeated database restore; add UT guards for restore-level ownership and shared workers.
Verification
pkg/frontendsuite: passed (-short -tags=matrixone_test -count=1, 15.103s).go vet ./pkg/frontend: passed.-race -count=1, 3.246s).pkg/frontend: passed, zero issues (molint and configured golangci linters).ignoresetting, zero failures. Coverage includes original duplicate-publication cluster cases, account controls, table/repeated DB restore, FK/view ordering, full-text subscriber rows and later SHOW DATABASES controls. No account/cluster golden changes were needed.pub_fulltext_table,pub_full01and the new fixture publication are zero.Validation was on macOS arm64 / Go 1.27.1 with matching CGo dependencies, not the Linux multi-CN Compose lane. Full remote CI remains pending; local results do not claim full Compose CI passed. Repair revision:
dc5953a8a8bf480beac8c45920ad793d12b964b8.