Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-32627

Spider: Add CONNECTION parsing of SQLDriverConnect format when the wrapper is odbc

Details

    Attachments

      Issue Links

        Activity

          ycp Yuchen Pei created issue -
          ycp Yuchen Pei made changes -
          Field Original Value New Value
          serg Sergei Golubchik made changes -
          Fix Version/s 11.4 [ 29301 ]
          Fix Version/s 11.3 [ 28565 ]
          ralf.gebhardt Ralf Gebhardt made changes -
          Component/s Storage Engine - Spider [ 14994 ]
          Component/s Storage Engine - Spider [ 10132 ]
          Fix Version/s Future [ 29511 ]
          Fix Version/s 11.4 [ 29301 ]
          Key MDEV-32066 MENT-1981
          Project MariaDB Server [ 10000 ] MariaDB Enterprise [ 11500 ]
          ycp Yuchen Pei made changes -
          Component/s Storage Engine - Spider [ 10132 ]
          Component/s Storage Engine - Spider [ 14994 ]
          Fix Version/s Future [ 29511 ]
          Key MENT-1981 MDEV-32627
          Project MariaDB Enterprise [ 11500 ] MariaDB Server [ 10000 ]
          ycp Yuchen Pei made changes -
          Fix Version/s 11.5 [ 29506 ]
          ycp Yuchen Pei made changes -
          Fix Version/s 11.6 [ 29515 ]
          Fix Version/s 11.5 [ 29506 ]
          ralf.gebhardt Ralf Gebhardt made changes -
          ralf.gebhardt Ralf Gebhardt made changes -
          Component/s Storage Engine - Spider/ODBC [ 20101 ]
          Component/s Storage Engine - Spider [ 10132 ]
          ycp Yuchen Pei added a comment -

          Since MDEV-31146 is blocked until 2028, we need to consider alternatives. Perhaps we could add a spider system variable that toggles the parsing of CONNECTION between the old format and the SQLDriverConnect format. Perhaps we could default to the SQLDriverConnect format when table options are specified using engine specific options, just like we do not parse COMMENT strings when this happens.

          ycp Yuchen Pei added a comment - Since MDEV-31146 is blocked until 2028, we need to consider alternatives. Perhaps we could add a spider system variable that toggles the parsing of CONNECTION between the old format and the SQLDriverConnect format. Perhaps we could default to the SQLDriverConnect format when table options are specified using engine specific options, just like we do not parse COMMENT strings when this happens.
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei added a comment -

          An initial draft

          upstream/10.4-enterprise-mdev-32627 74f05b0fbc92926552719f045defd2ce4c7d6859
          MDEV-32627 [poc] Spider: use CONNECTION string in SQLDriverConnect
           
          Adding a switch spider_pass_connection which, if set to true, will
          pass CONNECTION string to SQLDriverConnect(), instead of parsing it.
          When this happens, nothing else will be used to construct the connect
          string to pass to SQLDriverConnect().
          

          ycp Yuchen Pei added a comment - An initial draft upstream/10.4-enterprise-mdev-32627 74f05b0fbc92926552719f045defd2ce4c7d6859 MDEV-32627 [poc] Spider: use CONNECTION string in SQLDriverConnect   Adding a switch spider_pass_connection which, if set to true, will pass CONNECTION string to SQLDriverConnect(), instead of parsing it. When this happens, nothing else will be used to construct the connect string to pass to SQLDriverConnect().

          why do you need a switch? both current connection string (url) and ODBC DSN have very distinct syntax, you can distinguish them automatically. It if starts from "mysql://" — it's a url. If it has semicolon-separated key=value pair — it's ODBC DSN.

          serg Sergei Golubchik added a comment - why do you need a switch? both current connection string (url) and ODBC DSN have very distinct syntax, you can distinguish them automatically. It if starts from "mysql://" — it's a url. If it has semicolon-separated key=value pair — it's ODBC DSN.
          ycp Yuchen Pei added a comment - - edited

          > why do you need a switch? both current connection string (url) and ODBC DSN have very distinct syntax, you can distinguish them automatically. It if starts from "mysql://" — it's a url.

          No, this has nothing to do with "mysql://" url. The existing parsing of CONNECTION string is the same as the parsing of the COMMENT string, i.e. in the format of 'srv "s1 s2", tbl "t1 t2"' etc. It is not trivial to distinguish this from the SQLDriverConnect format, specified at https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqldriverconnect-function?view=sql-server-ver16&redirectedfrom=MSDN#comments

          See also https://sqlprotocoldoc.blob.core.windows.net/productionsqlarchives/MS-ODBCSTR/%5bMS-ODBCSTR%5d.pdf

          ycp Yuchen Pei added a comment - - edited > why do you need a switch? both current connection string (url) and ODBC DSN have very distinct syntax, you can distinguish them automatically. It if starts from "mysql://" — it's a url. No, this has nothing to do with "mysql://" url. The existing parsing of CONNECTION string is the same as the parsing of the COMMENT string, i.e. in the format of 'srv "s1 s2", tbl "t1 t2"' etc. It is not trivial to distinguish this from the SQLDriverConnect format, specified at https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqldriverconnect-function?view=sql-server-ver16&redirectedfrom=MSDN#comments See also https://sqlprotocoldoc.blob.core.windows.net/productionsqlarchives/MS-ODBCSTR/%5bMS-ODBCSTR%5d.pdf
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei made changes -
          Fix Version/s 11.4 [ 29301 ]
          Fix Version/s 11.6 [ 29515 ]
          ycp Yuchen Pei added a comment -

          Changed fixversion to 11.4, assuming ES 11.4 (fix version for the ES part of this ticket MENT-2070) is based on CS 11.4.

          ycp Yuchen Pei added a comment - Changed fixversion to 11.4, assuming ES 11.4 (fix version for the ES part of this ticket MENT-2070) is based on CS 11.4.

          ouch. So FEDERATED uses the url synax for CONNECTION string, but SPIDER has a different one, I didn't realize that.

          Still, SPIDER current syntax is

          keyword "value" [ , keyword "value" [ ... ] ]
          

          while ODBC syntax is

          keyword = value [ ; keyword = value [ ... ]]
          

          so it looks like you can distinguish by the first non-blank character after the first keyword.

          serg Sergei Golubchik added a comment - ouch. So FEDERATED uses the url synax for CONNECTION string, but SPIDER has a different one, I didn't realize that. Still, SPIDER current syntax is keyword "value" [ , keyword "value" [ ... ] ] while ODBC syntax is keyword = value [ ; keyword = value [ ... ]] so it looks like you can distinguish by the first non-blank character after the first keyword.
          ycp Yuchen Pei added a comment - - edited

          > so it looks like you can distinguish by the first non-blank character after the first keyword.

          Sure, we can do that, assuming that keyword in odbc can be identified trivially using a regex "[a-zA-Z0-9_]+".

          Two further considerations:

          • Because of MDEV-28856 / MDEV-28861, if table options are used or spider_ignore_comments is true, then the COMMENT and CONNECTION parsing are disabled. In this case we will pass CONNECTION to odbc automatically if it is nonempty
          • In CS where spider/odbc is not supported, we do not pass CONNECTION to odbc at all
          ycp Yuchen Pei added a comment - - edited > so it looks like you can distinguish by the first non-blank character after the first keyword. Sure, we can do that, assuming that keyword in odbc can be identified trivially using a regex " [a-zA-Z0-9_] +". Two further considerations: Because of MDEV-28856 / MDEV-28861 , if table options are used or spider_ignore_comments is true, then the COMMENT and CONNECTION parsing are disabled. In this case we will pass CONNECTION to odbc automatically if it is nonempty In CS where spider/odbc is not supported, we do not pass CONNECTION to odbc at all
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          ycp Yuchen Pei added a comment -

          Hi holyfoot, ptal thanks

          3a4d448b3af upstream/bb-11.4-mdev-32640 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect
          

          ycp Yuchen Pei added a comment - Hi holyfoot , ptal thanks 3a4d448b3af upstream/bb-11.4-mdev-32640 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect
          ycp Yuchen Pei made changes -
          Assignee Yuchen Pei [ JIRAUSER52627 ] Alexey Botchkov [ holyfoot ]
          Status In Progress [ 3 ] In Review [ 10002 ]

          One comment on the patch.
          otherwise ok to push.

          holyfoot Alexey Botchkov added a comment - One comment on the patch. otherwise ok to push.
          holyfoot Alexey Botchkov made changes -
          Assignee Alexey Botchkov [ holyfoot ] Yuchen Pei [ JIRAUSER52627 ]
          Status In Review [ 10002 ] Stalled [ 10000 ]
          ycp Yuchen Pei added a comment -

          Thanks holyfoot, while working on MENT-796 I realised that the patches of both this ticket and MENT-2070 may need to be revised and have been working on it as I prepare the patches for MENT-796. Will address the comment too.

          ycp Yuchen Pei added a comment - Thanks holyfoot , while working on MENT-796 I realised that the patches of both this ticket and MENT-2070 may need to be revised and have been working on it as I prepare the patches for MENT-796. Will address the comment too.
          ycp Yuchen Pei added a comment - - edited

          Hi holyfoot, I've adopted your suggestion and removed the init values. I also reworked these changes a bit based on what I learned while working on MENT-796. ptal thanks (in the branch 11.4-enterprise-ment-796):

          53724aa9c07 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect
          10d82255caa MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword
          2f23e084670 MDEV-32627 Spider: add tests for connection param overriding

          The main differences, compared to the previous patch, is that I added two commits that fix bugs relating to empty strings and merging of connection info from several sources.

          Am also sending you an updated patch for MENT-2070.

          Also applied to CS (in the branch bb-11.4-mdev-32627) as

          d0517a0c9a1 bb-11.4-mdev-32627 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect
          3b774099b66 MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword
          31107625b2f MDEV-32627 Spider: add tests for connection param overriding

          ycp Yuchen Pei added a comment - - edited Hi holyfoot , I've adopted your suggestion and removed the init values. I also reworked these changes a bit based on what I learned while working on MENT-796. ptal thanks (in the branch 11.4-enterprise-ment-796): 53724aa9c07 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect 10d82255caa MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword 2f23e084670 MDEV-32627 Spider: add tests for connection param overriding The main differences, compared to the previous patch, is that I added two commits that fix bugs relating to empty strings and merging of connection info from several sources. Am also sending you an updated patch for MENT-2070. Also applied to CS (in the branch bb-11.4-mdev-32627) as d0517a0c9a1 bb-11.4-mdev-32627 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect 3b774099b66 MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword 31107625b2f MDEV-32627 Spider: add tests for connection param overriding
          ycp Yuchen Pei made changes -
          Assignee Yuchen Pei [ JIRAUSER52627 ] Alexey Botchkov [ holyfoot ]
          Status Stalled [ 10000 ] In Review [ 10002 ]

          ok to push.
          Just fix the comment indentation.

          holyfoot Alexey Botchkov added a comment - ok to push. Just fix the comment indentation.
          holyfoot Alexey Botchkov made changes -
          Assignee Alexey Botchkov [ holyfoot ] Yuchen Pei [ JIRAUSER52627 ]
          Status In Review [ 10002 ] Stalled [ 10000 ]
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei added a comment - - edited

          Thanks for the review. Amended the commits to:

          db5bf1cb322 upstream/bb-11.4-mdev-32627 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect
          96e722eecfe MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword
          9279608e87c MDEV-32627 Spider: add tests for connection param overriding
          

          For 11.4-ES:

          7f007e41331 11.4-enterprise-mdev-32627 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect
          4c0e52c369b MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword
          43fa5931391 MDEV-32627 Spider: add tests for connection param overriding
          

          As part of MENT-2070, this ticket is now blocked by TODO-4629.

          Meanwhile I will push the bottom commit, which simply adds a test, to 10.5.

          ycp Yuchen Pei added a comment - - edited Thanks for the review. Amended the commits to: db5bf1cb322 upstream/bb-11.4-mdev-32627 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect 96e722eecfe MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword 9279608e87c MDEV-32627 Spider: add tests for connection param overriding For 11.4-ES: 7f007e41331 11.4-enterprise-mdev-32627 MDEV-32627 Spider: use CONNECTION string in SQLDriverConnect 4c0e52c369b MDEV-32627 Distinguish between absence of a keyword and empty value for the keyword 43fa5931391 MDEV-32627 Spider: add tests for connection param overriding As part of MENT-2070, this ticket is now blocked by TODO-4629. Meanwhile I will push the bottom commit, which simply adds a test, to 10.5.
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei added a comment - - edited

          A fixup commit (in bb-10.5-ycp) that should be pushed when pushing this ticket

          bef1cf17290 MDEV-32627 [fixup] Spider: Fix conn key length
          

          Without this fix, the MDEV-29962 fix would cause a hang in spider.spider_fixes.

          ycp Yuchen Pei added a comment - - edited A fixup commit (in bb-10.5-ycp) that should be pushed when pushing this ticket bef1cf17290 MDEV-32627 [fixup] Spider: Fix conn key length Without this fix, the MDEV-29962 fix would cause a hang in spider.spider_fixes.
          ycp Yuchen Pei added a comment -

          Pushed the bottom two commits and the fixup commit to 10.5 as MDEV-34359

          ycp Yuchen Pei added a comment - Pushed the bottom two commits and the fixup commit to 10.5 as MDEV-34359
          ycp Yuchen Pei added a comment -

          pending testing of MENT-2070 done

          ycp Yuchen Pei added a comment - pending testing of MENT-2070 done
          ycp Yuchen Pei made changes -
          ycp Yuchen Pei added a comment -

          pushed 18d3f63a4e406e2a445e9a62520cf08c0f5b6030 to 11.4

          ycp Yuchen Pei added a comment - pushed 18d3f63a4e406e2a445e9a62520cf08c0f5b6030 to 11.4
          ycp Yuchen Pei made changes -
          Fix Version/s 11.4.4 [ 29907 ]
          Fix Version/s 11.4 [ 29301 ]
          Resolution Fixed [ 1 ]
          Status Stalled [ 10000 ] Closed [ 6 ]

          People

            ycp Yuchen Pei
            ycp Yuchen Pei
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Git Integration

                Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.