Skip to content

Conversation

@dveeden
Copy link
Contributor

@dveeden dveeden commented Nov 21, 2025

What problem does this PR solve?

Issue Number: close #64635

Problem Summary:

This is used in pingcap/tiflow#12404

What changed and how does it work?

This adds SetMariaDB() to the parser, which can then be checked when parsing privilege names.

package main

import (
	"fmt"

	"github.com/pingcap/tidb/pkg/parser"
	_ "github.com/pingcap/tidb/pkg/types/parser_driver"
)

func main() {
	stmt := "GRANT BINLOG MONITOR ON *.* TO 'aaa'@'bbb'"
	p := parser.New()
	p.SetMariaDB(true)
	r, err := p.ParseOneStmt(stmt, "", "")
	if err != nil {
		panic(err)
	}
	fmt.Printf("result: %#v\n", r)
}
result: &ast.GrantStmt{stmtNode:ast.stmtNode{node:ast.node{utf8Text:"", enc:(*charset.encodingUTF8)(0x2c88420), once:(*sync.Once)(0xc00032d600), text:"GRANT BINLOG MONITOR ON *.* TO 'aaa'@'bbb'", offset:0}}, Privs:[]*ast.PrivElem{(*ast.PrivElem)(0xc000417960)}, ObjectType:1, Level:(*ast.GrantLevel)(0xc000309800), Users:[]*ast.UserSpec{(*ast.UserSpec)(0xc000483668)}, AuthTokenOrTLSOptions:[]*ast.AuthTokenOrTLSOption{}, WithGrant:false}

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Questions

  1. Should this only do BINLOG MONITOR and add other permissions later on when needed? Or add them now?
  2. The keywords added for this might show up in information_schema.keywords. Should we filter these out?

@ti-chi-bot
Copy link

ti-chi-bot bot commented Nov 21, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 21, 2025
@tiprow
Copy link

tiprow bot commented Nov 21, 2025

Hi @dveeden. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dveeden dveeden force-pushed the mariadb_priv_syntax branch from c882d56 to 7628bc3 Compare November 23, 2025 13:32
@dveeden dveeden changed the title WIP: Add option to allow MariaDB syntax parser: Add option to allow additional MariaDB syntax Nov 23, 2025
@dveeden dveeden marked this pull request as ready for review November 23, 2025 13:59
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2025
@dveeden dveeden force-pushed the mariadb_priv_syntax branch from 7628bc3 to 23b80b1 Compare November 23, 2025 14:14
@dveeden
Copy link
Contributor Author

dveeden commented Nov 23, 2025

/retest

@tiprow
Copy link

tiprow bot commented Nov 23, 2025

@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dveeden
Copy link
Contributor Author

dveeden commented Nov 23, 2025

/ok-to-test

@ti-chi-bot ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 23, 2025
Copy link
Contributor

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

And in fact SEQUENCE is a MariaDB syntax, https://docs.pingcap.com/tidb/stable/sql-statement-create-sequence/#mysql-compatibility so maybe add it to the comment of SetMariaDB that for compatibility it's always enabled.

@dveeden dveeden force-pushed the mariadb_priv_syntax branch from 23b80b1 to da41ec3 Compare November 23, 2025 14:41
@dveeden
Copy link
Contributor Author

dveeden commented Nov 23, 2025

And in fact SEQUENCE is a MariaDB syntax, https://docs.pingcap.com/tidb/stable/sql-statement-create-sequence/#mysql-compatibility so maybe add it to the comment of SetMariaDB that for compatibility it's always enabled.

The SEQUENCE came from MariaDB, but is now accepted TiDB syntax.

The BINLOG MONITOR that we need to parse for DM is MariaDB syntax that we only support in DM, but not in TiDB. Or do you think we should do that as well? Note that not all MariaDB permissions are aliases, some are unique.

@lance6716
Copy link
Contributor

that we only support in DM, but not in TiDB

I think we can narrow the name like SetMariaDBPrivCompatible, SetDMMode or something

@codecov
Copy link

codecov bot commented Nov 23, 2025

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.6108%. Comparing base (ce06e5d) to head (3d624fd).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #64625        +/-   ##
================================================
- Coverage   70.7518%   68.6108%   -2.1411%     
================================================
  Files          1895       1873        -22     
  Lines        518098     509982      -8116     
================================================
- Hits         366564     349903     -16661     
- Misses       126999     137687     +10688     
+ Partials      24535      22392      -2143     
Flag Coverage Δ
integration 41.5645% <ø> (-6.6056%) ⬇️
unit 65.8777% <40.0000%> (+0.3569%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.8700% <ø> (ø)
parser ∅ <ø> (∅)
br 38.4766% <ø> (-19.7496%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dveeden
Copy link
Contributor Author

dveeden commented Nov 23, 2025

that we only support in DM, but not in TiDB

I think we can narrow the name like SetMariaDBPrivCompatible, SetDMMode or something

I think the right name is difficult

  • SetDMMode(bool) (or SetDmMode(bool)?) : What if we want to use this at some point in Dumpling/CDC/etc? Also other open source projects could use the parser for various reasons and then it doesn't make sense to name it to something DM related.
  • SetMariaDBPrivCompatible(bool): Might be ok.
  • SetDialect(dialect...): Seems overly complicated.
  • SetAdditionalMariaDBSyntaxMode(): Might be ok.
  • MoreMariaDB() / LessMariaDB(): confusing

Maybe the solution is to have a ok-ish name and a good and complete comment?

@dveeden dveeden force-pushed the mariadb_priv_syntax branch from 077c39e to 1c6c913 Compare November 23, 2025 20:23
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 24, 2025
@lance6716
Copy link
Contributor

/cc @D3Hunter

@ti-chi-bot ti-chi-bot bot requested a review from D3Hunter November 28, 2025 03:36
@dveeden dveeden requested a review from D3Hunter November 30, 2025 10:50
@dveeden
Copy link
Contributor Author

dveeden commented Nov 30, 2025

/retest

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 8, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 8, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-11-24 14:58:59.486822308 +0000 UTC m=+542103.136016765: ☑️ agreed by lance6716.
  • 2025-12-08 03:12:53.996948867 +0000 UTC m=+837918.810726439: ☑️ agreed by D3Hunter.

@lance6716
Copy link
Contributor

/assign @yudongusa

@ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 10, 2025

@lance6716: GitHub didn't allow me to assign the following users: yudongusa.

Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @yudongusa

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dveeden
Copy link
Contributor Author

dveeden commented Dec 16, 2025

/retest

2 similar comments
@dveeden
Copy link
Contributor Author

dveeden commented Dec 16, 2025

/retest

@lance6716
Copy link
Contributor

/retest

@lance6716
Copy link
Contributor

ptal @bb7133 @yudongusa

@dveeden
Copy link
Contributor Author

dveeden commented Dec 22, 2025

/retest

@dveeden dveeden removed the request for review from tiancaiamao December 22, 2025 10:53
@dveeden
Copy link
Contributor Author

dveeden commented Dec 22, 2025

/retest

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 22, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bb7133, D3Hunter, hawkingrei, lance6716
Once this PR has been reviewed and has the lgtm label, please assign d3hunter, yudongusa for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:
  • OWNERS [D3Hunter,bb7133,hawkingrei,lance6716]
  • pkg/parser/OWNERS [D3Hunter,bb7133]

    Need more approvers for rest parts.

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dveeden
Copy link
Contributor Author

dveeden commented Dec 22, 2025

/retest

1 similar comment
@dveeden
Copy link
Contributor Author

dveeden commented Dec 23, 2025

/retest

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

Labels

lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parser: Extend MariaDB support

5 participants