[4.1] fix publication table lookup for quoted database names - #25532
Conversation
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? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the 4.1-dev port. The fix is the right direction: the table-level publication path validates explicit table lists via showTablesFromDb, and that SQL needs identifier quoting rather than raw concatenation. The caller passes a database identifier value, not a prebuilt SQL fragment, so sqlquote.Ident(dbName) is appropriate and does not affect DATABASE * because genPubTablesStr returns before calling showTablesFromDb for TABLE *.
Coverage looks adequate for this bug: the unit test covers non-ASCII names and embedded backticks at the generated SQL boundary, and the BVT covers create publication, alter publication, subscriber visibility, and the negative case that an unpublished table remains hidden. CI is green for UT, BVT, coverage, upgrade, and benchmark jobs.
No blocking findings.
Merge Queue Status
This pull request spent 1 hour 15 minutes 26 seconds in the queue, including 1 hour 15 minutes 10 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
issue matrixorigin/matrixflow#11551
4.1-dev port of #25518.
What this PR does / why we need it:
This ports the publication/subscription fix to 4.1-dev.
Root cause: table-level publication refresh builds
show tables from <db>by raw string concatenation. Database names that require identifier quoting, such as Chinese database names or names containing backticks, produce invalid SQL or the wrong identifier.Changes:
showTablesFromDbwithsqlquote.Ident.Validation:
make buildgo test -v -count=1 -timeout 120s ./pkg/frontend -run 'Test_showTablesFromDbQuotesDatabaseName'with MatrixOne CGo flagsgit diff --check upstream/4.1-dev...HEADNote: local BVT execution against a temporary 4.1 MO did not complete because the local temporary MO process exited during mo-tester cleanup. The added BVT case itself already passed on the main PR CI/local run, and this PR includes it for 4.1 CI coverage.