FB8-117: Add thread ID option for SQL SET command #1012

inikep · 2019-04-03T15:13:55Z

Summary:

Jira issue: https://jira.percona.com/browse/FB8-117

Reference Patch: 19f5aad

Added thread ID option to the SQL SET command. The syntax of the command is as follows:

SET SESSION [THREAD_ID] var_name = var_value [, [SESSION [THREAD_ID]] var_name = var_value]

Notes:

The thread ID can be specified only with the SESSION keyword
If no thread ID is provided, it is assumed to be the current thread
Similarly, if own thread ID is provided, it is equivalent to SET var_name = var_value
If the thread with given ID does not exist, it will fail
For commands like SET SESSION THREAD_ID var1 = var_value1, var2 = var_value2, var3 = var_value3 the SESSION THREAD_ID will apply to all three variables as explained in https://dev.mysql.com/doc/refman/5.6/en/set-variable.html

sql/set_var.cc

laurynas-biveinis · 2019-04-04T11:07:39Z

sql/sql_yacc.yy

@@ -450,7 +450,7 @@ void warn_about_deprecated_national(THD *thd)
  1. We do not accept any reduce/reduce conflicts
  2. We should not introduce new shift/reduce conflicts any more.
 */
-%expect 108
+%expect 109


I guess here (and in the high priority DDL PR) no easy way to avoid this?

We can fix this by reworking the grammar rules so that the thread id in option_type_with_thread_id is non-optional. It looks like we're confusing the start_option_value_list_following_option_type rule, because it doesn't know if it needs to reduce option_type to option_type_with_thread_id.

@inikep, as per @lth's suggestion, can something like the following grammar rules be used instead and can the parse tree be able to support it easily? It basically removes option_type from option_type_with_thread_id and explicitly adds productions for option_type_with_thread_id to the appropriate locations that have productions for option_type. Changes to your existing rules would be:

option_type_with_thread_id: LOCAL_SYM opt_var_tid | SESSION_SYM opt_var_tid ; start_option_value_list_following_option_type: option_type option_value_following_option_type option_value_list_continued | option_type_with_thread_id option_value_following_option_type option_value_list_continued | option_type TRANSACTION_SYM transaction_characteristics ; option_value: option_type option_value_following_option_type | option_type_with_thread_id option_value_following_option_type | option_value_no_option_type ;

Note I'm only including the grammar rules here without any code associated on those rules. The above change, when compiled with bison, does not have any additional shift/reduce conflicts.

mysql-test/t/set_thread_var.test

sql/parse_tree_nodes.h

sql/set_var.cc

inikep · 2019-04-05T10:31:00Z

PR was updated.

percona-ysorokin

LGTM

facebook-github-bot

@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hermanlee

Most of the code looks good. The changes to the parse tree code seems to work, but I'm wondering if it would be simpler to add the thread_id value to the option_type structure directly instead of create a new one.

hermanlee · 2019-05-14T22:30:55Z

sql/set_var.cc

+      }
+
+      Security_context *sctx = thd->security_context();
+      if (!sctx->check_access(SUPER_ACL) &&


Would it make sense to cache the result of the first security_context check? It doesn't seem like the query result could change if it's a pass. Setting multiple variables is probably rare anyway, so maybe this is not worth considering.

This code is called only with SET SESSION THREAD_ID and it's not called without THREAD_ID. The optimization will make sense only for SET SESSION THREAD_ID var_name1 = var_value1, SET SESSION THREAD_ID var_name2 = var_value2 or more values.

Yes. The use case I was thinking of would have been trying to update one variable in all of outstanding sessions, so the call would be:

SET SESSION thread_id1 var_name = var_value, SESSION thread_id2 var_name = var_value, SESSION thread_id3 var_name = var_value;

But it's minor anyhow.

sql/parse_tree_nodes.h

hermanlee · 2019-05-23T23:51:36Z

sql/set_var.cc

+      }
+
+      Security_context *sctx = thd->security_context();
+      if (!sctx->check_access(SUPER_ACL) &&


Yes. The use case I was thinking of would have been trying to update one variable in all of outstanding sessions, so the call would be:

SET SESSION thread_id1 var_name = var_value, SESSION thread_id2 var_name = var_value, SESSION thread_id3 var_name = var_value;

But it's minor anyhow.

sql/parse_tree_nodes.h

lth

I mainly have a question about the change in behaviour in the security checks.

lth · 2019-06-04T16:39:05Z

sql/set_var.cc

+          !sctx->has_global_grant(STRING_WITH_LEN("SYSTEM_VARIABLES_ADMIN"))
+               .first) {
+        mysql_mutex_unlock(&change_thd->LOCK_thd_data);
+        my_error(ER_SHOW_DENIED_ERROR, MYF(0), thd_id);


I don't know if the change of behaviour here is intentional, but previously, we just checked if the user was an owner of the thread, instead of checking for specific privileges. Now the error is kind of misleading, because you'll get "You are not owner of thread", when we should really be getting something from ER_SPECIFIC_ACCESS_DENIED_ERROR.

I spoke with @hermanlee and the change in behaviour is probably okay.

However, I think we should use ER_SPECIFIC_ACCESS_DENIED_ERROR instead so that the error is less confusing.

lth · 2019-06-04T16:41:10Z

sql/sql_yacc.yy

@@ -450,7 +450,7 @@ void warn_about_deprecated_national(THD *thd)
  1. We do not accept any reduce/reduce conflicts
  2. We should not introduce new shift/reduce conflicts any more.
 */
-%expect 108
+%expect 109


We can fix this by reworking the grammar rules so that the thread id in option_type_with_thread_id is non-optional. It looks like we're confusing the start_option_value_list_following_option_type rule, because it doesn't know if it needs to reduce option_type to option_type_with_thread_id.

lth · 2019-06-04T16:46:03Z

sql/parse_tree_helpers.cc

@@ -358,12 +359,14 @@ bool sp_create_assignment_instr(THD *thd, const char *expr_end_ptr) {
  }

  /* Remember option_type of the currently parsed LEX. */
-  enum_var_type inner_option_type = lex->option_type;
+  const enum_var_type inner_option_type = lex->option_type;


Not too important, but I wonder if this code path actually works? It'd be nice to have simple test for set statements in stored procedures.

It doesn't work because of an upstream bug:
https://bugs.mysql.com/bug.php?id=95734

Jira issue: https://jira.percona.com/browse/FB8-117 Reference Patch: facebook@19f5aad Summary: Added thread ID option to the SQL SET command. The syntax of the command is as follows: `SET SESSION [THREAD_ID] var_name = var_value [, [SESSION [THREAD_ID]] var_name = var_value]` Notes: - The thread ID can be specified only with the SESSION keyword - If no thread ID is provided, it is assumed to be the current thread - Similarly, if own thread ID is provided, it is equivalent to `SET var_name = var_value` - If the thread with given ID does not exist, it will fail - For commands like `SET SESSION THREAD_ID var1 = var_value1, var2 = var_value2, var3 = var_value3` the SESSION THREAD_ID will apply to all three variables as explained in https://dev.mysql.com/doc/refman/5.6/en/set-variable.html fbshipit-source-id: efe2126

facebook-github-bot · 2019-06-11T11:57:28Z

@inikep has updated the pull request. Re-import the pull request

inikep · 2019-06-11T12:06:30Z

PR was updated with the following changes:

ER_SHOW_DENIED_ERROR was replaced with ER_SPECIFIC_ACCESS_DENIED_ERROR
The grammar was changed according to the proposal from @hermanlee and appropriate classes were updated

facebook-github-bot

@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Jira issue: https://jira.percona.com/browse/FB8-117 Reference Patch: 19f5aad Added thread ID option to the SQL SET command. The syntax of the command is as follows: `SET SESSION [THREAD_ID] var_name = var_value [, [SESSION [THREAD_ID]] var_name = var_value]` Notes: - The thread ID can be specified only with the SESSION keyword - If no thread ID is provided, it is assumed to be the current thread - Similarly, if own thread ID is provided, it is equivalent to `SET var_name = var_value` - If the thread with given ID does not exist, it will fail - For commands like `SET SESSION THREAD_ID var1 = var_value1, var2 = var_value2, var3 = var_value3` the SESSION THREAD_ID will apply to all three variables as explained in https://dev.mysql.com/doc/refman/5.6/en/set-variable.html Pull Request resolved: #1012 Reviewed By: lth Differential Revision: D15773495 Pulled By: lth fbshipit-source-id: 24798ca

…cebook#1012) Summary: Jira issue: https://jira.percona.com/browse/FB8-117 Reference Patch: facebook@19f5aad Added thread ID option to the SQL SET command. The syntax of the command is as follows: `SET SESSION [THREAD_ID] var_name = var_value [, [SESSION [THREAD_ID]] var_name = var_value]` Notes: - The thread ID can be specified only with the SESSION keyword - If no thread ID is provided, it is assumed to be the current thread - Similarly, if own thread ID is provided, it is equivalent to `SET var_name = var_value` - If the thread with given ID does not exist, it will fail - For commands like `SET SESSION THREAD_ID var1 = var_value1, var2 = var_value2, var3 = var_value3` the SESSION THREAD_ID will apply to all three variables as explained in https://dev.mysql.com/doc/refman/5.6/en/set-variable.html Pull Request resolved: facebook#1012 Reviewed By: lloyd Differential Revision: D15773495 Pulled By: lth

facebook-github-bot added the CLA Signed label Apr 3, 2019

laurynas-biveinis suggested changes Apr 4, 2019

View reviewed changes

percona-ysorokin suggested changes Apr 4, 2019

View reviewed changes

inikep force-pushed the FB8-117 branch 3 times, most recently from 74ae309 to 2b59862 Compare April 5, 2019 10:24

inikep force-pushed the FB8-117 branch from 2b59862 to a3ff0d4 Compare April 5, 2019 10:41

percona-ysorokin approved these changes Apr 8, 2019

View reviewed changes

inikep changed the title ~~WIP FB8-117: Add thread ID option for SQL SET command~~ FB8-117: Add thread ID option for SQL SET command Apr 17, 2019

facebook-github-bot reviewed Apr 23, 2019

View reviewed changes

hermanlee reviewed May 15, 2019

View reviewed changes

sql/parse_tree_nodes.h Outdated Show resolved Hide resolved

hermanlee approved these changes May 23, 2019

View reviewed changes

lth reviewed Jun 4, 2019

View reviewed changes

inikep force-pushed the FB8-117 branch from a3ff0d4 to 6ffcd1e Compare June 11, 2019 11:57

facebook-github-bot reviewed Jun 11, 2019

View reviewed changes

facebook-github-bot reviewed Jun 12, 2019

View reviewed changes

hermanlee closed this Jun 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FB8-117: Add thread ID option for SQL SET command #1012

FB8-117: Add thread ID option for SQL SET command #1012

inikep commented Apr 3, 2019 •

edited by hermanlee

Loading

laurynas-biveinis Apr 4, 2019

lth Jun 4, 2019

hermanlee Jun 8, 2019

inikep Jun 11, 2019

inikep commented Apr 5, 2019

percona-ysorokin left a comment

facebook-github-bot left a comment

hermanlee left a comment

hermanlee May 14, 2019

inikep May 21, 2019

hermanlee May 23, 2019

hermanlee May 23, 2019

lth left a comment

lth Jun 4, 2019

lth Jun 4, 2019

inikep Jun 11, 2019

lth Jun 4, 2019

lth Jun 4, 2019

inikep Jun 11, 2019

facebook-github-bot commented Jun 11, 2019

inikep commented Jun 11, 2019

facebook-github-bot left a comment

facebook-github-bot left a comment

FB8-117: Add thread ID option for SQL SET command #1012

FB8-117: Add thread ID option for SQL SET command #1012

Conversation

inikep commented Apr 3, 2019 • edited by hermanlee Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inikep commented Apr 5, 2019

percona-ysorokin left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

hermanlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 11, 2019

inikep commented Jun 11, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

inikep commented Apr 3, 2019 •

edited by hermanlee

Loading