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

Variable references like @@GLOBAL.sql_mode cause syntax errors #104

Closed
brandonpayton opened this issue Apr 27, 2024 · 3 comments · Fixed by #109
Closed

Variable references like @@GLOBAL.sql_mode cause syntax errors #104

brandonpayton opened this issue Apr 27, 2024 · 3 comments · Fixed by #109
Assignees
Labels
bug Something isn't working

Comments

@brandonpayton
Copy link
Member

An example query is:

SELECT @@session.max_allowed_packet

Within WordPress Playground this is leading to an error message like:

SQLSTATE[HY000]: General error: 1 unrecognized token: "@"

Related to WordPress/wordpress-playground#1272

@brandonpayton brandonpayton added the bug Something isn't working label Apr 27, 2024
@brandonpayton brandonpayton self-assigned this Apr 27, 2024
@brandonpayton
Copy link
Member Author

Plan: Follow up with a PR containing a failing test and then work on a fix.

@brandonpayton
Copy link
Member Author

Some notes from @adamziel:

In the SQLite plugin we should either:

  • Inspect each consumed token to see if it's a SQL variable, like @@session.max_allowed_packet, and then substitute it for something SQLite understand. Upside: It would work for all queries. Downside: Consuming a token could potentially consume three, if @@session, ., and max_allowed_packet are all separate tokens. Because of that, it could be better to...
  • Do a second pass over parsed queries (or maybe just SELECT queries?) to substitute these variables with values. This could potentially use regexps or strcspn as strings should be replaced with :parameters at that stage, although that requires double checking if @ signs can be legally used outside of strings and variable names.
  • Is there even a third solution?

In v1 we could use a hardcoded mapping of variable -> value. In v2 we could create a SQLite database table and source the data from there, also supporting SET.

(from this GH comment)

Random thought:
Would there be any value in allowing the default values of these variables to be filterable in WP?

If so, we'd need to be careful not to override values assigned via SET.

@brandonpayton
Copy link
Member Author

Notes about @@ references in MariaDB SQL parsing

To help figure out whether there is any additional SQL syntax that includes @@, I ran the preprocessor to remove inline comments from MariaDB SQL parsing to reduce the number of references:

for FILE in $(ls ../sql); do echo "$FILE"; gcc-13 -fpreprocessed -dD -E "../sql/$FILE" > "$FILE"; done

And the following references remained under the mariadb/sql dir:

sys_vars.cc:       DEPRECATED("'@@debug_dbug'"));
sys_vars.cc:       DEPRECATED("'@@mrr_buffer_size'"));
sys_vars.cc:   "@@skip_replication=1 on the master. Default REPLICATE (no events are "
sys_vars.cc:   "@@skip_replication=1 will be filtered on the master and never be sent to "
sys_vars.cc:       "storage_engine", "Alias for @@default_storage_engine. Deprecated",
sys_vars.cc:       "@@skip_replication flag set. Such events will not be replicated by "
sys_vars.cc:       DEPRECATED("'@@wsrep_sync_wait=1'"));
log_event.cc:    my_b_printf(file,"SET @@session.pseudo_thread_id=%lu%s\n",
log_event.cc:                       "@@session.foreign_key_checks", &need_comma);
log_event.cc:                       "@@session.sql_auto_is_null", &need_comma);
log_event.cc:                       "@@session.unique_checks", &need_comma);
log_event.cc:                       "@@session.autocommit", &need_comma);
log_event.cc:                       "@@session.check_constraint_checks", &need_comma);
log_event.cc:    my_b_printf(file,"SET @@session.sql_mode=%s%s\n",
log_event.cc:    my_b_printf(file,"SET @@session.auto_increment_increment=%lu, @@session.auto_increment_offset=%lu%s\n",
log_event.cc:                "@@session.character_set_client=%d,"
log_event.cc:                "@@session.collation_connection=%d,"
log_event.cc:                "@@session.collation_server=%d"
log_event.cc:      my_b_printf(file,"SET @@session.time_zone='%s'%s\n",
log_event.cc:    my_b_printf(file, "SET @@session.lc_time_names=%d%s\n",
log_event.cc:      my_b_printf(file, "SET @@session.collation_database=%d%s\n",
log_event.cc:      my_b_printf(file, "SET @@session.collation_database=DEFAULT%s\n",
log_event.cc:    my_b_printf(&cache,"%sSET @@session.pseudo_thread_id=%lu%s\n",
log_event.cc:                  "/*!100101 SET @@session.skip_parallel_replication=%u*/%s\n",
log_event.cc:      my_b_printf(&cache, "/*!100001 SET @@session.gtid_domain_id=%u*/%s\n",
log_event.cc:      my_b_printf(&cache, "/*!100001 SET @@session.server_id=%u*/%s\n",
log_event.cc:      my_b_printf(&cache, "/*!100001 SET @@session.gtid_seq_no=%s*/%s\n",
log_event.cc:  my_b_printf(&cache, "SET @@RAND_SEED1=%s, @@RAND_SEED2=%s%s\n",
set_var.cc:    strxnmov(buf1, sizeof(buf1)-1, "@@", name.str, 0);
item_func.cc:    str->append(STRING_WITH_LEN("@@"));
item_func.cc:  return mark_unsupported_function("@@", var->name.str, arg, VCOL_SESSION_FUNC);
sql_table.cc:    memcpy(to + length, "@@@", 4);
sql_parse.cc:    end= strxmov(buff, "@@session.", var_name, NullS);
sql_mode.cc:                          "Expression depends on the @@%s value %s",
slave.cc:                          STRING_WITH_LEN("SELECT @@GLOBAL.COLLATION_SERVER")) &&
slave.cc:    if (!mysql_real_query(mysql, STRING_WITH_LEN("SELECT @@GLOBAL.TIME_ZONE")) &&
slave.cc:    const char query[]= "SET @master_binlog_checksum= @@global.binlog_checksum";
slave.cc:                   "Setting master-side filtering of @@skip_replication failed "
slave.cc:                            "of @@skip_replication events."));
slave.cc:          "with the @@skip_replication flag.";
slave.cc:                         STRING_WITH_LEN("SELECT @@GLOBAL.gtid_domain_id")) ||
slave.cc:                   "Get master @@GLOBAL.gtid_domain_id failed with error: %s",
slave.cc:          "it tries to SELECT @@GLOBAL.gtid_domain_id.";

Planning to review these matches later.

brandonpayton added a commit that referenced this issue May 1, 2024
The purpose of this PR is to avoid errors when selecting MySQL system variables. Fixes #104.

Related to:
WordPress/wordpress-playground#1272 -
"UpdraftPlus plugins doesn't fully work in Playground."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant