Skip to content

fix(plan): expose subscription index metadata - #27778

Open
Lundomn wants to merge 3 commits into
matrixorigin:mainfrom
Lundomn:fix/issue-27759
Open

fix(plan): expose subscription index metadata#27778
Lundomn wants to merge 3 commits into
matrixorigin:mainfrom
Lundomn:fix/issue-27759

Conversation

@Lundomn

@Lundomn Lundomn commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #27759

Fixes #27759

What this PR does / why we need it:

  • Route SHOW INDEX catalog scans for subscription tables through the publisher using the complete subscription metadata.
  • Route direct information_schema.STATISTICS reads through the publisher while keeping unrelated catalog tables in the same query on the subscriber.
  • Rewrite publisher schema names back to the subscription database and constrain mo_tables scans to tables exposed by the publication.
  • Cover the SQL shapes used by Connector/J getIndexInfo() and getPrimaryKeys(), including server-prepared statements.
  • Add planner unit tests and publication/subscription BVT coverage, including unpublished-table isolation and ambiguous multi-schema predicates.

Testing

  • go test ./pkg/sql/plan
  • go test -race ./pkg/sql/plan
  • go vet ./pkg/sql/plan
  • make config
  • make err-check
  • make build
  • pub_sub4.sql: 34/34 statements passed on the committed head; the unfixed baseline reproduced the four expected metadata failures.
  • MySQL Connector/J 8.0.33 metadata integration passed with useInformationSchema and useServerPrepStmts both enabled and disabled.
  • Affected-package statement coverage: 79.1%; changed production statements: 86.1%.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review of exact head baae324, including the complete diff, issue #27759, planner/catalog routing and publication-isolation paths, tests, and CI state. The targeted implementation tests, full pkg/sql/plan tests, vet, build-only check, focused race tests (20 iterations), and diff check pass. I found three blocking query-shape correctness gaps in the new scope selection; two are reproduced by exact-head planner counterexamples inline.

Comment thread pkg/sql/plan/subscription_metadata.go Outdated
// emits exactly this shape for getIndexInfo and getPrimaryKeys.
func (builder *QueryBuilder) enterSubscriptionMetadataScope(stmt *tree.Select) (func(), error) {
previous := builder.subscriptionMetadataScope
if previous != nil || !selectReadsInformationSchemaStatistics(stmt) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: subscriptionMetadataScope is builder-wide, and this early return makes every nested SELECT inherit the first STATISTICS instance's publisher. Repro with subscriptions sub_a (publisher account A) and sub_b (publisher account B): SELECT s.index_name FROM information_schema.statistics s WHERE s.table_schema='sub_a' AND EXISTS (SELECT 1 FROM information_schema.statistics t WHERE t.table_schema='sub_b'). Both mo_indexes scans are planned with A's PubInfo; the inner view output is rewritten to sub_a, so its table_schema='sub_b' predicate cannot match and valid outer rows disappear. I reproduced the exact-head plan: expected publisher IDs {A,B}, actual {A}. Each STATISTICS occurrence/query block needs its own scope; a parent scope cannot suppress discovery for the nested block.

Comment thread pkg/sql/plan/subscription_metadata.go Outdated
}

databaseName, allowDefaultDatabase := selectTableSchemaEquality(stmt)
if databaseName == "" && allowDefaultDatabase {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: treating DefaultDatabase() as the target whenever TABLE_SCHEMA is absent or non-literal changes both INFORMATION_SCHEMA and prepared-statement semantics. USE sub_db; SELECT ... FROM information_schema.statistics is an account-wide metadata query, not an implicit TABLE_SCHEMA='sub_db'; this path replaces the subscriber scan with one publisher, drops local-schema rows, and relabels every publisher row as sub_db. It also fixes a TABLE_SCHEMA=? route at PREPARE time: prepare under local_db then execute with @schema='sub_db' still scans the subscriber (the original zero-row bug), while prepare under sub_db then execute with @schema='local_db' stays on the publisher and returns no local rows. A server-prepared statement may be executed with a catalog different from the connection default or reused with different catalogs. Please avoid inferring an absent/dynamic schema predicate from the current DB; this needs a parameter-aware/runtime route or a plan that preserves all applicable subscriber/subscription sources.

Comment thread pkg/sql/plan/subscription_metadata.go Outdated
if databaseName, ok := tableSchemaLiteral(typed.Right, typed.Left); ok {
return databaseName, false
}
case *tree.OrExpr, *tree.XorExpr, *tree.NotExpr:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: any OR/XOR/NOT anywhere in the WHERE tree is treated as schema ambiguity, even when a separate top-level conjunct already fixes the statistics alias to one schema. Repro: SELECT s.index_name FROM information_schema.statistics s WHERE s.table_schema='sub_db' AND (s.index_name='PRIMARY' OR s.non_unique=1). This is unambiguously scoped to sub_db, but on the exact head scope selection is rejected and the mo_indexes scan has nil PubInfo, so subscription indexes disappear. I reproduced this with a failing planner counterexample. Only boolean branches that can change the effective TABLE_SCHEMA constraint should invalidate the scope; unrelated predicates must not discard a proven schema conjunct.

@aunjgr aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head baae32422345ae179b21e92c644ce09b0c7717d9 against merge base e5f26b66195373f9bff512ac9f428788d1c07e8c.

Three blocking routing errors remain:

  1. subscriptionMetadataScope is builder-wide and enterSubscriptionMetadataScope returns immediately when a parent scope exists. A nested STATISTICS query targeting a different subscription therefore inherits the parent's publisher and can incorrectly return no rows or mislabeled rows. Scope must be owned per query block/STATISTICS occurrence.

  2. An absent or non-literal TABLE_SCHEMA constraint is replaced with DefaultDatabase(). INFORMATION_SCHEMA.STATISTICS without a schema predicate is account-wide, while TABLE_SCHEMA=? is execution-dependent. Preparing/executing under different databases or parameter values therefore fixes the wrong publisher at plan time. Do not infer either case from the connection default; preserve all applicable sources or introduce a runtime-capable route.

  3. tableSchemaConjunct marks any OR/XOR/NOT subtree ambiguous even when an independent top-level conjunct already proves one schema, e.g. table_schema='sub_db' AND (index_name='PRIMARY' OR non_unique=1). That drops a valid subscription scope and hides indexes. Only boolean branches that can alter the TABLE_SCHEMA constraint should invalidate it.

Exact-head CI and focused tests are green, but the current tests do not cover these query-block, prepared, and unrelated-boolean shapes.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep-reviewed exact head 8ba796c against base 28769ff, including the complete planner/frontend/BVT diff and all earlier review findings. The per-query-block/sibling scope, unrelated-boolean, and execute-time prepared-parameter defects are now addressed, but one fundamental metadata correctness gap remains.

[P1] Account-wide and semantically equivalent STATISTICS queries still omit every subscription index. The new BVT explicitly locks in select ... from information_schema.statistics where table_name = visible_t returning 0. A subscription database is a visible schema of the subscriber, so an account-wide INFORMATION_SCHEMA query must include its published table metadata; falling back to subscriber-only catalog scans leaves the original issue unresolved for clients that do not provide TABLE_SCHEMA.

The route is also selected only from a literal/parameter equality found in the same query block WHERE AST. Equivalent valid forms such as ... statistics s join ... on s.table_schema = sub_db or an outer filter over a derived STATISTICS query have no local WHERE constraint during configureSubscriptionMetadataScopes; they retain subscriber scans and return empty subscription metadata. This makes correctness depend on predicate syntax/placement rather than relational semantics.

Please preserve all visible subscriber and subscription sources when the schema is absent/ambiguous, and route constraints discovered through equivalent query shapes (or introduce a runtime/catalog abstraction that does not require syntax-specific single-publisher replacement). Add public regressions for account-wide, JOIN-ON, and derived-table forms while retaining unpublished-table isolation. Exact-head CI being green does not close this because the committed account-wide oracle currently expects the wrong zero-row result.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep-reviewed exact head 0d253a7 against merge base b789245. The redesign correctly fixes the earlier account-wide, JOIN ON, derived-table, sibling/nested, and prepared-parameter query-shape omissions, and preserves unpublished-table filtering. Two temporal correctness gaps remain:

[P1] GetSubscriptionMetas(snapshot) does not read mo_subs at the requested snapshot. GetSubscriptionMeta immediately above clones the transaction when snapshot.TS is older, but the new account-wide method only changes TenantID and then calls getOrCreateBackExec, whose cached BackgroundExec is explicitly kept on the current session transaction. bindSubscriptionStatisticsView uses this current subscription list to construct branches while each branch is bound with the historical snapshot. Therefore a historical INFORMATION_SCHEMA.STATISTICS query can combine a present-day branch set with historical catalog data: subscriptions created after the snapshot can be exposed as schemas at that snapshot, while subscriptions that existed then but were later dropped/withdrawn disappear. Please enumerate mo_subs at the same snapshot TS (via a snapshot txn or MO_TS query) and add create/drop-across-snapshot regressions.

[P1] Prepared statements retain a plan-time snapshot of the complete subscription set. Each STATISTICS occurrence is expanded into one branch per subscription during BuildPlan. NotCacheable prevents the generic SQL plan cache, but the prepared execution path does not rebuild on NotCacheable: shouldRebuildPreparePlan checks only schema/FK changes, and subscription validation iterates only ObjectRefs already present in PreparePlan.Schemas. A subscription created after PREPARE is absent from that list, so EXECUTE with TABLE_SCHEMA set to the new subscription can keep returning zero rows indefinitely; conversely membership/status changes not represented by an existing dependency can remain stale. The new prepared test only verifies a static list at PREPARE time. Please make subscription-set membership an explicit invalidation dependency or rebuild this plan class, and test PREPARE -> create/drop/withdraw subscription -> EXECUTE.

Performance/unhappy-path note: plan construction is O(number of visible subscriptions) per STATISTICS occurrence and intentionally reaches 64 full view expansions in the new test. This is bounded by account metadata only, not by the query predicate; repeated STATISTICS references multiply plan/catalog scans. It does not introduce a wait/leak/log-storm path, but the fix should avoid worsening this expansion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: SHOW INDEX and JDBC key metadata are empty for subscription tables

5 participants