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

Not ignoring comments on "use database" statements #4598

Closed
Abigail opened this issue Aug 5, 2024 · 3 comments · Fixed by #4605
Closed

Not ignoring comments on "use database" statements #4598

Abigail opened this issue Aug 5, 2024 · 3 comments · Fixed by #4605

Comments

@Abigail
Copy link

Abigail commented Aug 5, 2024

I find that the following works as expected:

use db /* COMMENT */

it will connect to the database db.

However, if there is a newline in the comment:

use db /*
   COMMENT */

it will try to switch to the database db /* COMMENT */, which obviously not what should happen.

The other types of comment aren't ignored either:

use db -- COMMENT
use db # COMMENT

result in attempts to switch to database db -- COMMENT and db # COMMENT.

I have not seen this issue with other statements than the use statement, but I haven't done an exhaustive search.

@JavierJF
Copy link
Collaborator

JavierJF commented Aug 5, 2024

Hi @Abigail,

the issue template is not a form to scrap and replace with free-writing. There are several details required to follow up on this, all of them missing, but that would be present if the template was filled. When opening an issue, please, at least make sure the points on the issue template are filled.

Thanks, Javier.

@Abigail
Copy link
Author

Abigail commented Aug 5, 2024

Sorry, I don't know all the details being asked.

You can just ignore my bug report, or, alternatively, investigate the regexp on line 6139 of lib/MySQL_Session.cpp. The regexp is:

     /\\*.*\\*/

which is problematic in three ways:

  1. . matches any character, but a newline.
  2. The regexp is too greedy, meaning that if you have two comments on the same line (like SQL1 /* comment */ SQL2 /* comment */) it replaces the code between the comments (replacing /* comment */ SQL2 /* comment */ by a space).
  3. It doesn't match -- or # style comments.

May I suggest something like

     (?:(?:#|--(?=\s))[^\n]*\n|/\*(?:(?>[^*]+)|\*(?!/))*(?:;|\*/))

instead?

@JavierJF
Copy link
Collaborator

JavierJF commented Aug 6, 2024

Hi @Abigail,

You can just ignore my bug report, or, alternatively, investigate the regexp on line 6139 of lib/MySQL_Session.cpp. The regexp is:

What an interesting wording you used, I find myself at a crossroads, only two ways ahead! I understand this is your not so nice way of presenting your findings. Let me be clear, my request was not because there was the need of extra info for a regular contributor to find the obvious cause of the behavior, or to change a simple regex, into a sightly more complex one. A couple of git blames would tell you that the change was intentional, and that the behavior is well known, and was changed like this by design for the time being. Giving you more context, for several parsing related issues, we solve the most common and pressing cases, and (this has changed overtime) we might take a different approach for fixing these kind of parsing issues all together, or maybe, while waiting for this more ideal solution, we might use more complex regexes (like the one you provided).

The point of asking those details, as you can probably guess, is procedural. Those details are generic, and help with issue categorization, even without knowledge of the codebase, something that universal and common to all projects (even outside software). So please, just for the sake of curiosity, and maybe improving the template, which one of this highly technical details, was not possible to share?

If you are submitting a reproducible bug report, please provide:
- [ ] A clear description of the issue  // you already did this
- [ ] ProxySQL version // ??
- [ ] OS version // ??
- [ ] The steps to reproduce the issue // ??
- [ ] The **full** ProxySQL error log (default location: `/var/lib/proxysql/proxysql.log`) // copy paste?

Regards, Javier.

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 a pull request may close this issue.

2 participants