tidb icon indicating copy to clipboard operation
tidb copied to clipboard

parser: Add option to allow additional MariaDB syntax

Open dveeden opened this issue 1 month ago • 15 comments

What problem does this PR solve?

Issue Number: close #64635

Problem Summary:

This is used in https://github.com/pingcap/tiflow/pull/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

  • [x] Unit test
  • [ ] Integration test
  • [x] 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
  • [x] 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?~~

dveeden avatar Nov 21 '25 12:11 dveeden

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[bot] avatar Nov 21 '25 12:11 ti-chi-bot[bot]

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.

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.

tiprow[bot] avatar Nov 21 '25 12:11 tiprow[bot]

/retest

dveeden avatar Nov 23 '25 14:11 dveeden

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

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.

tiprow[bot] avatar Nov 23 '25 14:11 tiprow[bot]

/ok-to-test

dveeden avatar Nov 23 '25 14:11 dveeden

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.

dveeden avatar Nov 23 '25 14:11 dveeden

that we only support in DM, but not in TiDB

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

lance6716 avatar Nov 23 '25 14:11 lance6716

Codecov Report

:x: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 68.7493%. Comparing base (e5fb979) to head (e486e12). :warning: Report is 5 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #64625        +/-   ##
================================================
- Coverage   70.8767%   68.7493%   -2.1275%     
================================================
  Files          1890       1868        -22     
  Lines        516251     508463      -7788     
================================================
- Hits         365902     349565     -16337     
- Misses       125886     136573     +10687     
+ Partials      24463      22325      -2138     
Flag Coverage Δ
integration 41.6299% <ø> (-6.5110%) :arrow_down:
unit 65.9915% <40.0000%> (+0.3618%) :arrow_up:

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

Components Coverage Δ
dumpling 52.8700% <ø> (ø)
parser ∅ <ø> (∅)
br 39.1136% <ø> (-20.2392%) :arrow_down:
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 23 '25 15:11 codecov[bot]

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 avatar Nov 23 '25 15:11 dveeden

/cc @D3Hunter

lance6716 avatar Nov 28 '25 03:11 lance6716

/retest

dveeden avatar Nov 30 '25 11:11 dveeden

[LGTM Timeline notifier]

Timeline:

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

ti-chi-bot[bot] avatar Dec 08 '25 03:12 ti-chi-bot[bot]

/assign @yudongusa

lance6716 avatar Dec 10 '25 12:12 lance6716

@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

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.

ti-chi-bot[bot] avatar Dec 10 '25 12:12 ti-chi-bot[bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: D3Hunter, lance6716 Once this PR has been reviewed and has the lgtm label, please assign bb7133, 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.

Needs approval from an approver in each of these files:

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

ti-chi-bot[bot] avatar Dec 16 '25 11:12 ti-chi-bot[bot]

/retest

dveeden avatar Dec 16 '25 13:12 dveeden

/retest

dveeden avatar Dec 16 '25 15:12 dveeden

/retest

lance6716 avatar Dec 17 '25 01:12 lance6716