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

MySQL/MariaDB: Use strict SQL mode instead of just ANSI_QUOTES #624

Closed
wants to merge 2 commits into from

Conversation

lippserd
Copy link
Member

@lippserd lippserd commented Jul 31, 2023

This PR

  • introduces Quoter that provides utility functions for quoting table names and columns, where the quotation mark depends on the database driver used
  • and replaces the ANSI_QUOTES SQL mode with TRADITIONAL which enables strict mode.

Previously, setting ANSI_QUOTES overrode the server defaults, which were most likely stricter. Also Vitess, which will be supported in the near future, does not support ANSI_QUOTES.

refs #606

Quoter provides utility functions for quoting table names and columns,
where the quotation mark depends on the database driver used. This is in
preparation for using strict SQL mode instead of just ANSI_QUOTES. Also
Vitess, which will be supported in the near future, does not support
ANSI_QUOTES.
@lippserd lippserd added this to the 1.1.1 milestone Jul 31, 2023
@cla-bot cla-bot bot added the cla/signed label Jul 31, 2023
@lippserd lippserd changed the title MySQL: Use strict SQL mode instead of just ANSI_QUOTES MySQL/MariaDB: Use strict SQL mode instead of just ANSI_QUOTES Jul 31, 2023
Comment on lines +64 to +65
// Set strict SQL mode, i.e. trigger an error if an incorrect value is inserted into a column.
config.Params = map[string]string{"sql_mode": "TRADITIONAL"}
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do differently compared to not setting sql_mode here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to explicitly enable strict mode instead of letting something else decide it.

Copy link
Member

Choose a reason for hiding this comment

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

MariaDB's default SQL_MODE depends on the MariaDB version and, even with current MariaDB versions, does not equal TRADITIONAL.

MariaDB [icingadb]> SELECT @@SQL_MODE, @@GLOBAL.SQL_MODE\G
*************************** 1. row ***************************
       @@SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
@@GLOBAL.SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
1 row in set (0.000 sec)

MariaDB [icingadb]> SET sql_mode = 'TRADITIONAL';
Query OK, 0 rows affected (0.001 sec)

MariaDB [icingadb]> SELECT @@SQL_MODE, @@GLOBAL.SQL_MODE\G
*************************** 1. row ***************************
       @@SQL_MODE: STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
@@GLOBAL.SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
1 row in set (0.000 sec)

Furthermore, MySQL's default SQL_MODE is also something else.

Fortunately, both MySQL's as well as MariaDB's TRADITIONAL mode maps to the same mode list.
Thus, explicitly setting the SQL_MODE to TRADITIONAL sounds like a good idea.

@@ -32,6 +32,7 @@ type DB struct {
logger *logging.Logger
tableSemaphores map[string]*semaphore.Weighted
tableSemaphoresMu sync.Mutex
quoter *Quoter
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering: why hide this inside a member instead of having the functions directly available on DB (maybe by embedding the struct/pointer)? Would make the calls a bit nicer and the functions wouldn't look out of place there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't think of that tbh.

// QuoteColumnList quotes the given columns into a single comma concatenated string
// so that they can be safely used as a column list for SELECT and INSERT statements.
func (q *Quoter) QuoteColumnList(columns []string) string {
return fmt.Sprintf("%[1]s%s%[1]s", q.quoteCharacter, strings.Join(columns, q.quoteCharacter+", "+q.quoteCharacter))
Copy link
Contributor

Choose a reason for hiding this comment

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

Mixing numbered and unnumbered placeholders looks strange. At least I can't tell without double-checking which argument an unnumbered placeholder would refer to after some numbered ones.

So maybe at least go with %[1]s%[2]s%[1]s here.

But it's probably easier to not use printf here at all, q.quoteCharacter + strings.Join(columns, q.quoteCharacter+", "+q.quoteCharacter)) + q.quoteCharacter is shorter, even though it repeats the name quoteCharacter yet another time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a few tests for this file should be easy by initializing with &Quoter{"`"}. Testing NewQuoter() would probably be annoying without a database connection.

Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Using MySQL quotes on both Vitess and, while on it, on MySQL is absolutely legit. For 1.2.0.

@@ -61,7 +61,8 @@ func (d *Database) Open(logger *logging.Logger) (*icingadb.DB, error) {

config.DBName = d.Database
config.Timeout = time.Minute
config.Params = map[string]string{"sql_mode": "ANSI_QUOTES"}
// Set strict SQL mode, i.e. trigger an error if an incorrect value is inserted into a column.
config.Params = map[string]string{"sql_mode": "TRADITIONAL"}
Copy link
Member

Choose a reason for hiding this comment

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

For 1.1.1 we should just set TRADITIONAL,ANSI_QUOTES here. This is a bugfix release. PoC:

MariaDB [idb]> SET SQL_MODE='TRADITIONAL';
Query OK, 0 rows affected (0,003 sec)

MariaDB [idb]> SET SQL_MODE='TRADITIONAL,ANSI_QUOTES';
Query OK, 0 rows affected (0,003 sec)

MariaDB [idb]> select @@SQL_MODE\G
*************************** 1. row ***************************
@@SQL_MODE: ANSI_QUOTES,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
1 row in set (0,003 sec)

MariaDB [idb]> SET SQL_MODE='TRADITIONAL,ANSI_QUOTES,LOLCAT';
ERROR 1231 (42000): Variable 'sql_mode' can't be set to the value of 'LOLCAT'
MariaDB [idb]>

@julianbrost
Copy link
Contributor

I mean the missing of strict mode is not really bug in itself. Enabling it, at best, changes nothing, at worst, it exposes other bugs. In doubt, exposing would mean crashing the Icinga DB process. So better just do this as a whole in 1.2.0.

@julianbrost julianbrost modified the milestones: 1.1.1, 1.2.0 Aug 1, 2023
@julianbrost julianbrost requested a review from oxzi March 7, 2024 09:10
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

I have successfully tested this PR and a rebased version on the main branch.

Should the SQL_MODE also be reflected in the schema.sql file?

Otherwise, there is nothing I would add to the already written comments.

Comment on lines +64 to +65
// Set strict SQL mode, i.e. trigger an error if an incorrect value is inserted into a column.
config.Params = map[string]string{"sql_mode": "TRADITIONAL"}
Copy link
Member

Choose a reason for hiding this comment

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

MariaDB's default SQL_MODE depends on the MariaDB version and, even with current MariaDB versions, does not equal TRADITIONAL.

MariaDB [icingadb]> SELECT @@SQL_MODE, @@GLOBAL.SQL_MODE\G
*************************** 1. row ***************************
       @@SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
@@GLOBAL.SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
1 row in set (0.000 sec)

MariaDB [icingadb]> SET sql_mode = 'TRADITIONAL';
Query OK, 0 rows affected (0.001 sec)

MariaDB [icingadb]> SELECT @@SQL_MODE, @@GLOBAL.SQL_MODE\G
*************************** 1. row ***************************
       @@SQL_MODE: STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
@@GLOBAL.SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
1 row in set (0.000 sec)

Furthermore, MySQL's default SQL_MODE is also something else.

Fortunately, both MySQL's as well as MariaDB's TRADITIONAL mode maps to the same mode list.
Thus, explicitly setting the SQL_MODE to TRADITIONAL sounds like a good idea.

@lippserd lippserd removed this from the 1.1.2 milestone Mar 18, 2024
@lippserd
Copy link
Member Author

lippserd commented Mar 18, 2024

#699 sets TRADITIONAL in addition to ANSI_QUOTES to enable strict mode, while this PR eliminates the latter by introducing driver-specific quoting. I've removed this PR from the next release and will leave it open until we decide whether to support Vitess, which would be the only reason to change quoting.

@oxzi oxzi added this to the 1.3.0 milestone Jul 30, 2024
@julianbrost
Copy link
Contributor

This PR

  • introduces Quoter that provides utility functions for quoting table names and columns, where the quotation mark depends on the database driver used
  • and replaces the ANSI_QUOTES SQL mode with TRADITIONAL which enables strict mode.

Accidentally disabling strict mode was already fixed in #699

So only the other point remains which seems to only be relevant if we'd want to so support Vitess (#606).

@julianbrost julianbrost removed this from the 1.3.0 milestone Jul 31, 2024
@lippserd
Copy link
Member Author

lippserd commented Oct 2, 2024

I doubt that these changes will be necessary in the near future, so I am closing this PR.

@lippserd lippserd closed this Oct 2, 2024
@lippserd lippserd deleted the quoting branch October 2, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants