Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Fingerprint() to make it work correctly with MAX_EXECUTION_TIME statement #63

Open
wants to merge 2 commits into
base: PMM-2.0
Choose a base branch
from

Conversation

yehornaumenko
Copy link

@yehornaumenko yehornaumenko commented Dec 28, 2024

https://perconadev.atlassian.net/browse/PMM-13757

Function func Fingerprint(q string) string does not work correctly with SELECT /*+ MAX_EXECUTION_TIME(<number>) */ statement.

For queries where column names are wrapped into backticks ``, MAX_EXECUTION_TIME(123) statement is preserved with a number. The number is not replaced by ?.

For queries where column names are not wrapped into backticks, it works correctly and does not copy MAX_EXECUTION_TIME(<number>) to a fingerprint at all.

This PR:

  1. Adds tests for above-mentioned cases
  2. Adds a fix that make the Fingerprint() function work correctly with MAX_EXECUTION_TIME()

Copy link
Member

@ademidoff ademidoff left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution 👍

@yehornaumenko
Copy link
Author

Just to be clear
I've chosen PMM-2.0 as a target branch because it is the one used in percona/pmm

@ademidoff
Copy link
Member

Just to be clear I've chosen PMM-2.0 as a target branch because it is the one used in percona/pmm

Yes, that's fine - it's the project default branch.

@yehornaumenko
Copy link
Author

yehornaumenko commented Jan 7, 2025

Is there anything further needed from my side to get the PR merged?
PS Thank you for the review!

@ademidoff
Copy link
Member

ademidoff commented Jan 23, 2025

Is there anything further needed from my side to get the PR merged? PS Thank you for the review!

Nothing more, it's ready for testing as is. Unfortunately, we have to wait until we release our next product version, which is what prevents us from merging this PR sooner.

Apologies for that and thanks a lot!

@yehornaumenko
Copy link
Author

Got it.
Thank you!

@marat-saddarov
Copy link

Any hints about when the next product version will be/planned to be released would be very much appreciated :)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants