[Improve](auditlog) audit log print real sql in prepared statement by HexyinUESTC · Pull Request #43038 · apache/doris

HexyinUESTC · 2024-10-31T19:44:31Z

What problem does this PR solve?

Use the "execute *** using *** /*original sql = */" in the audit log instead of "execute *** using ***".
Add a CommandType parameter to the audit log.
When the prepared statement is ready, it should log OK instead of NOOP.
Fixed a bug where only the last statement in multiple executions was logged multiple times, while previous statements were not logged.

Issue Number: close #42553

Problem Summary:

Check List (For Committer)

Test
- Regression test
- Unit Test
- Manual test (add detailed scripts or steps below)
- No need to test or manual test. Explain why:
  - This is a refactor/code format and no logic has been changed.
  - Previous test can cover this change.
  - No colde files have been changed.
  - Other reason
Behavior changed:
- No.
- Yes.
Does this need documentation?
- No.
- Yes.
Release note

None

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

doris-robot · 2024-10-31T19:44:36Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

eldenmoon · 2024-11-01T04:15:04Z

fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java

            context.getMysqlChannel().sendOnePacket(serializer.toByteBuffer());
        }
        context.getMysqlChannel().flush();
-        context.getState().setNoop();


what happend if setOK? you should distingwish diffent behavior bettwen NOOP and OK

After verification, I found that changing it to OK would cause issues. Therefore, when logging, I added a validation: if the status is com_stmt_prepare and there are no errors, it is recorded as OK.

I saw the issue and this setting, and I thought this setNoop was a bug, so I didn’t think it through carefully. Thank you for the reminder

eldenmoon · 2024-11-01T04:17:00Z

fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java


    public AuditEventBuilder getAuditEventBuilder() {
-        return auditEventBuilder;
+        return new AuditEventBuilder();


do not new AuditEventBuilder() here, new AuditEventBuilder for prepared statement EXECUTE

Thank you for your correction, I have modified this part by performing a deep copy of the auditevent object to prevent the statement recorded in the audit log from being overwritten.

eldenmoon · 2024-11-01T04:20:38Z

fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java

+            if (!expr.isNullLiteral()) {
+                value = expr.toString();
+            }
+            origStmt = origStmt.replaceFirst("\\?", value);


what if literal contains "?" for example , select * from tbl where key = "123?"

Thank you for your correction.I will modify it to skip ? enclosed in quotes.

eldenmoon · 2024-11-01T04:27:12Z

fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java

+        for (Map.Entry<PlaceholderId, Expression> entry : sortedEntries) {
+            Expression expr = entry.getValue();
+            String value = "";
+            if (!expr.isNullLiteral()) {


why special treat null value？

If expr is a NullLiteral, toString() will return "NULL".

eldenmoon · 2024-11-01T06:27:33Z

fe/fe-core/src/main/java/org/apache/doris/plugin/AuditEvent.java

        public AuditEvent build() {
-            return this.auditEvent;
+            AuditEvent copy = new AuditEvent();
+            copy.type = auditEvent.type;


do not modify this. you should call reset some where when handle EXEUCTE command

Thank you for your correction

eldenmoon · 2024-11-01T06:29:00Z

fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java

    }

+    private String parseRealSql(String origStmt, Map<PlaceholderId, Expression> idExpressionMap) {
+        if (idExpressionMap.isEmpty()) {


I think it's tricky to rewrite the sql, and i think maybe another way, we could print EXETUE xxx USING XXX , and add another filed in audit log to print the prepared sql like select * from tbl where a = ?

eldenmoon

LGTM

eldenmoon · 2024-11-01T07:50:21Z

run buildall

eldenmoon

LGTM

github-actions · 2024-11-01T07:51:16Z

PR approved by at least one committer and no changes requested.

github-actions · 2024-11-01T07:51:18Z

PR approved by anyone and no changes requested.

xiaokang

LGTM

eldenmoon · 2024-11-04T02:12:15Z

run buildall

eldenmoon

LGTM

github-actions · 2024-11-04T02:32:35Z

PR approved by at least one committer and no changes requested.

xiaokang · 2024-11-04T06:55:38Z

fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java

            executor.execute();
            if (ctx.getSessionVariable().isEnablePreparedStmtAuditLog()) {
                stmtStr = executeStmt.toSql();
+                stmtStr = stmtStr + "/*originalSql = " + prepareCommand.getOriginalStmt().originStmt + "*/";


add blank before /*originalSql for user friendly.

xiaokang · 2024-11-04T06:56:31Z

fe/fe-core/src/main/java/org/apache/doris/plugin/AuditEvent.java

    public String ctl = "";
    @AuditField(value = "Db")
    public String db = "";
+    @AuditField(value = "CommandType")


@eldenmoon Will it cause sede compatibility problem ?

no, adding fields will not cause compatibility problem

eldenmoon

LGTM

github-actions · 2024-11-04T14:44:36Z

PR approved by at least one committer and no changes requested.

eldenmoon · 2024-11-05T02:02:09Z

run buildall

xiaokang

LGTM

…43038) 1. Use the "execute *** using *** /*original sql = */" in the audit log instead of "execute *** using ***". 2. Add a CommandType parameter to the audit log. 3. When the prepared statement is ready, it should log OK instead of NOOP

…statement (#43321) Cherry-picked from #43038 Co-authored-by: He xueyu <112704062+HexyinUESTC@users.noreply.github.com>

…statement (apache#43321) Cherry-picked from apache#43038 Co-authored-by: He xueyu <112704062+HexyinUESTC@users.noreply.github.com>

…pache#43038) 1. Use the "execute *** using *** /*original sql = */" in the audit log instead of "execute *** using ***". 2. Add a CommandType parameter to the audit log. 3. When the prepared statement is ready, it should log OK instead of NOOP

…44588) cherry-pick from #43038 Co-authored-by: He xueyu <112704062+HexyinUESTC@users.noreply.github.com>

eldenmoon reviewed Nov 1, 2024

View reviewed changes

HexyinUESTC closed this Nov 1, 2024

HexyinUESTC deleted the issue_fix branch November 1, 2024 06:00

HexyinUESTC reopened this Nov 1, 2024

eldenmoon reviewed Nov 1, 2024

View reviewed changes

HexyinUESTC force-pushed the issue_fix branch from ba98233 to db0b8b5 Compare November 1, 2024 06:59

HexyinUESTC changed the title ~~audit log print real sql in prepared statement~~ [Improve](auditlog) audit log print real sql in prepared statement Nov 1, 2024

HexyinUESTC force-pushed the issue_fix branch 2 times, most recently from 656edf6 to 2ef96e3 Compare November 1, 2024 07:35

eldenmoon previously approved these changes Nov 1, 2024

View reviewed changes

HexyinUESTC dismissed eldenmoon’s stale review via e57e441 November 1, 2024 07:37

HexyinUESTC force-pushed the issue_fix branch from 2ef96e3 to e57e441 Compare November 1, 2024 07:37

eldenmoon previously approved these changes Nov 1, 2024

View reviewed changes

eldenmoon added dev/3.0.x dev/2.1.x labels Nov 1, 2024

github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 1, 2024

github-actions bot added the reviewed label Nov 1, 2024

xiaokang previously approved these changes Nov 1, 2024

View reviewed changes

HexyinUESTC dismissed stale reviews from xiaokang and eldenmoon via d7585e7 November 2, 2024 03:58

github-actions bot removed the approved Indicates a PR has been approved by one committer. label Nov 2, 2024

eldenmoon previously approved these changes Nov 4, 2024

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 4, 2024

xiaokang reviewed Nov 4, 2024

View reviewed changes

“hxy” added 2 commits November 4, 2024 21:18

audit log print real sql in prepared statement

275a58e

fix some bug

6c99373

HexyinUESTC dismissed eldenmoon’s stale review via 6c99373 November 4, 2024 13:20

HexyinUESTC force-pushed the issue_fix branch from d7585e7 to 6c99373 Compare November 4, 2024 13:20

github-actions bot removed the approved Indicates a PR has been approved by one committer. label Nov 4, 2024

eldenmoon approved these changes Nov 4, 2024

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 4, 2024

xiaokang approved these changes Nov 6, 2024

View reviewed changes

eldenmoon merged commit 8c55f2b into apache:master Nov 6, 2024

github-actions bot mentioned this pull request Nov 6, 2024

branch-3.0: [Improve](auditlog) audit log print real sql in prepared statement #43321

Merged

github-actions bot added the cherry-pick-conflict-in-2.1.x label Nov 6, 2024

dataroaring pushed a commit that referenced this pull request Nov 7, 2024

branch-3.0: [Improve](auditlog) audit log print real sql in prepared …

6121a1c

…statement (#43321) Cherry-picked from #43038 Co-authored-by: He xueyu <112704062+HexyinUESTC@users.noreply.github.com>

dataroaring added dev/3.0.3-merged kind/behavior-changed and removed dev/3.0.x labels Nov 7, 2024

eldenmoon mentioned this pull request Nov 26, 2024

[Improve](auditlog) audit log print real sql in prepared statement #44588

Merged

yiguolei pushed a commit that referenced this pull request Nov 26, 2024

[Improve](auditlog) audit log print real sql in prepared statement (#…

4113c11

…44588) cherry-pick from #43038 Co-authored-by: He xueyu <112704062+HexyinUESTC@users.noreply.github.com>

yiguolei added dev/2.1.8-merged and removed dev/2.1.x cherry-pick-conflict-in-2.1.x labels Nov 26, 2024

Conversation

HexyinUESTC commented Oct 31, 2024 • edited by eldenmoon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

Check List (For Committer)

Check List (For Reviewer who merge this PR)

Uh oh!

doris-robot commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eldenmoon Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eldenmoon left a comment

Choose a reason for hiding this comment

Uh oh!

eldenmoon commented Nov 1, 2024

Uh oh!

eldenmoon left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 1, 2024

Uh oh!

github-actions bot commented Nov 1, 2024

Uh oh!

xiaokang left a comment

Choose a reason for hiding this comment

Uh oh!

eldenmoon commented Nov 4, 2024

Uh oh!

eldenmoon left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eldenmoon Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eldenmoon left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 4, 2024

Uh oh!

eldenmoon commented Nov 5, 2024

Uh oh!

xiaokang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

HexyinUESTC commented Oct 31, 2024 •

edited by eldenmoon

Loading

eldenmoon Nov 1, 2024 •

edited

Loading

eldenmoon Nov 4, 2024 •

edited

Loading