Refs #24638 -- Added support for SQL comments in queries.

[hannseman]

Ticket 24638

This PR tries to bring back life into this ticket which last stalled in https://github.com/django/django/pull/4495. I think it would be a useful addition to Django for adding traceability into stuff like logged slow queries.

I stuck with using the C-style block comment delimiters (/**/) instead of the double-dash (--) as the later require a line break to mark the end of the comment and that felt off since to my knowledge we previously haven't output any line breaks in queries. The downside of using the block comments it that they are harder to sanitize but recursively stripping the delimiters using a regex should be sufficient.

Except for sanitation of the input this PR changes so that the comments ends up at the end of the statement, it also allows chaining of comments. This aligns the behaviour with how Ruby on Rails does it. The sqlcommenter specification used for distributed tracing also expects comments at the end of the statement.

Adding the comment at the end of the statement stops this method from being used to add optimizer hints on supporting databases but I think that should be treated as a separate ticket.

[hannseman]

@hannseman Thanks for this patch 👍 One of potential uses mentioned in the ticket and the previous PR is to pass hints (see Oracle and MySQL docs), however they're ignored when don't follow a DELETE, INSERT, MERGE, SELECT, or UPDATE keyword. I think we should put comments after these keywords.

My reasoning was that optimizer hints could be implemented as a separate feature but I see the benefit of getting support for those using the same API. Maybe outweighs the more expected (in my mind) position in the end of the statement when used as a pure comment. I'll move the comments to follow the initial keywords.

[hannseman]

Updated to add the comments after SELECT and UPDATE, supporting DELETE proved to be a rabbit hole..

Edit: Found Adam's implementation of this. And notably the comment about executable comments on MariaDB and MySQL (🤯). Not sure if this is might be acceptable with a warning in the docs otherwise I think that we should only support "pure" comments by adding a space after /*, so that comment("!50101 +1) would end up as /* !50101 +1 */, and maybe moving the comments back to the end of the statement. That should be safe from any SQL injections.

A safer way to handle optimizer hints could be to add it as it's own method (or as an arg like; hint=True) which automatically adds /*+, maybe something like: comment("MAX_EXECUTION_TIME(60000)", hint=True) could result in the comment /*+ MAX_EXECUTION_TIME(60000) */ being added after SELECT and UPDATE. This means that we would not support the executable comment stuff like /*! and /*M!.

This is sort of how rails handle the same issue with optimizer_hints and annotate.

@felixxm any thoughts?

[felixxm]

Updated to add the comments after SELECT and UPDATE, supporting DELETE proved to be a rabbit hole..

Edit: Found Adam's implementation of this. And notably the comment about executable comments on MariaDB and MySQL (exploding_head). Not sure if this is might be acceptable with a warning in the docs otherwise I think that we should only support "pure" comments by adding a space after /*, so that comment("!50101 +1) would end up as /* !50101 +1 */, and maybe moving the comments back to the end of the statement. That should be safe from any SQL injections.

A safer way to handle optimizer hints could be to add it as it's own method (or as an arg like; hint=True) which automatically adds /*+, maybe something like: comment("MAX_EXECUTION_TIME(60000)", hint=True) could result in the comment /*+ MAX_EXECUTION_TIME(60000) */ being added after SELECT and UPDATE. This means that we would not support the executable comment stuff like /*! and /*M!.

This is sort of how rails handle the same issue with optimizer_hints and annotate.

@felixxm any thoughts?

Agreed, let's add a whitespace after /* and before */. We can add support for hints in a separate method.

[hannseman]

Agreed, let's add a whitespace after /* and before */. We can add support for hints in a separate method.

Sounds good, what do you think about the position of the comment, keep it after the first keyword or at the end of the statement?

[felixxm]

Agreed, let's add a whitespace after /* and before */. We can add support for hints in a separate method.

Sounds good, what do you think about the position of the comment, keep it after the first keyword or at the end of the statement?

The end is probably better.

[ngnpope]

The end is probably better.

Generally I would agree.

I can think of a case where it would be useful to be able to stick it at the beginning, however. Quite often a query can be too long and is truncated, e.g. in pg_stat_activity. It could be useful to have a flag to put it at the very start?

[adamchainz]

I think the front is better, for the reason Nick says. Monitoring tools like innotop show the first N characters of a query. Seeing "SELECT /* example.views */" in such tools is IMO one of the key use cases for this feature.

[hannseman]

I think the front is better, for the reason Nick says. Monitoring tools like innotop show the first N characters of a query. Seeing "SELECT /* example.views */" in such tools is IMO one of the key use cases for this feature.

Appreciate the input Adam! Yeah, I can see the benefit of keeping the comments close the beginning of the statement. I'm fine with keeping the comment there as long as the space after /* makes sure that no instances of + or ! are interpreted as having any special meaning and from my quick testing they are not.

mysql> select 1 /*! SELECT 2; */;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'SELECT 2' at line 1
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '*/' at line 1

mysql> select 1 /* ! SELECT 2; */;
+---+
| 1 |
+---+
| 1 |
+---+
1 row in set (0.00 sec)

[felixxm]

@hannseman Do you have time to keep working on this?

[felixxm]

Closing due to inactivity.