Skip to content

Conversation

zhangxu19830126
Copy link
Contributor

@zhangxu19830126 zhangxu19830126 commented Jul 9, 2025

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #16438

What this PR does / why we need it:

make nternal sql executor support prepared statement


PR Type

Enhancement


Description

  • Add prepared statement support to internal SQL executor

  • Enable parameter binding for SQL statements with placeholders

  • Implement parameter handling in StatementOption and compilation

  • Add comprehensive test coverage for prepared statements


Changes diagram

flowchart LR
  A["StatementOption"] -- "WithParams()" --> B["Parameter Storage"]
  B -- "HasParams()" --> C["SQL Executor"]
  C -- "SetPrepareParams()" --> D["Process Context"]
  D -- "BuildPlan(prepared=true)" --> E["Plan Compilation"]
  E -- "ResetPreparePlan()" --> F["Execution"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
sql_executor.go
Core prepared statement execution logic                                   

pkg/sql/compile/sql_executor.go

  • Add parameter detection and processing logic
  • Implement prepared statement plan building
  • Modify compilation context to handle parameters
  • Add conditional plan reset for prepared statements
  • +51/-16 
    options.go
    Parameter handling methods for StatementOption                     

    pkg/util/executor/options.go

  • Add HasParams() method to check parameter existence
  • Implement Params() method to convert strings to vector
  • Add WithParams() method for parameter binding
  • +27/-0   
    types.go
    StatementOption structure enhancement                                       

    pkg/util/executor/types.go

  • Add params field to StatementOption struct
  • Remove outdated comment about SQL statement limitations
  • +1/-2     
    Tests
    test_utils.go
    Test utility for result counting                                                 

    pkg/tests/testutils/test_utils.go

    • Add ReadCount utility function for result processing
    +13/-0   
    txn_executor_test.go
    Prepared statement integration tests                                         

    pkg/tests/txnexecutor/txn_executor_test.go

  • Add comprehensive test for prepared statement parameters
  • Test parameter binding with INSERT and SELECT operations
  • Verify both transaction and non-transaction execution
  • +65/-2   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    qodo-merge-pro bot commented Jul 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    SQL injection:
    The prepared statement implementation accepts string parameters that are directly converted to vectors without proper validation or sanitization. While the use of prepared statements generally helps prevent SQL injection, the current implementation doesn't validate parameter types or values, which could potentially be exploited if malicious input is passed through the WithParams() method.

    ⚡ Recommended focus areas for review

    Resource Leak

    The vector created by statementOption.Params() is deferred for cleanup, but if plan.BuildPlan() or plan.ResetPreparePlan() fails early, the defer may not execute properly, potentially causing memory leaks.

    if statementOption.HasParams() {
    	vec := statementOption.Params(exec.s.mp)
    	proc.SetPrepareParams(vec)
    	prepared = true
    	defer vec.Free(proc.Mp())
    }
    Error Handling

    In the prepared statement build function, plan.ResetPreparePlan() uses the original compileContext instead of the newly created context from the current execution, which could lead to stale context usage.

    _, _, err = plan.ResetPreparePlan(compileContext, pn)
    if err != nil {
    Memory Management

    The Params() method creates a new vector but doesn't provide clear ownership semantics. The caller is responsible for freeing the vector, but this isn't documented and could lead to memory leaks if not handled properly.

    func (opts StatementOption) Params(
    	mp *mpool.MPool,
    ) *vector.Vector {
    	vec := vector.NewVec(types.T_varchar.ToType())
    	vector.AppendStringList(
    		vec,
    		opts.params,
    		make([]bool, len(opts.params)),
    		mp,
    	)
    	return vec
    }

    Copy link
    Contributor

    mergify bot commented Jul 10, 2025

    This pull request has been removed from the queue for the following reason: checks failed.

    The merge conditions cannot be satisfied due to failing checks:

    You may have to fix your CI before adding the pull request to the queue again.
    If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
    If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

    @mergify mergify bot merged commit 5a7999f into matrixorigin:main Jul 10, 2025
    19 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants