Skip to content

treat the type of 'dual' table as 'TypeReference'#5268

Merged
sougou merged 2 commits intovitessio:masterfrom
tinyblink:handle-sql-with-pseudo-table-properly
Oct 24, 2019
Merged

treat the type of 'dual' table as 'TypeReference'#5268
sougou merged 2 commits intovitessio:masterfrom
tinyblink:handle-sql-with-pseudo-table-properly

Conversation

@tinyblink
Copy link

@tinyblink tinyblink commented Oct 6, 2019

JDBC Driver may produce the following sql which includes pseudo table 'dual':

  • select @@session.tx_read_only from dual

  • select @@session.tx_isolation from dual

  • select 1 from dual

  • ... etc.

The default behavior of vtgate to treat those sql queries is routing them to a default shard, which will significantly increase the burden on a specific vttablet.
why not treating the route type of those sql queries as SelectDBA? Since these sql queries will be executed on any shard and will not result in any hotspot among vttablets.
image

@tinyblink tinyblink requested a review from sougou as a code owner October 6, 2019 18:06
@tinyblink tinyblink force-pushed the handle-sql-with-pseudo-table-properly branch from eee755c to 35fe4b1 Compare October 6, 2019 18:11
@tinyblink
Copy link
Author

tinyblink commented Oct 7, 2019

@sougou In this case can we return response from cache or just return a canned response?

failed test
***database calls should be substituted***
"select database() from dual"
{
  "Original": "select database() from dual",
  "Instructions": {
    "Opcode": "SelectUnsharded",
    "Keyspace": {
      "Name": "main",
      "Sharded": false
    },
    "Query": "select database() from dual",
    "FieldQuery": "select database() from dual where 1 != 1"
  }
}
 ***last_insert_id for unsharded route***
"select last_insert_id() from main.dual"
{
  "Original": "select last_insert_id() from main.dual",
  "Instructions": {
    "Opcode": "SelectUnsharded",
    "Keyspace": {
      "Name": "main",
      "Sharded": false
    },
    "Query": "select last_insert_id() from dual",
    "FieldQuery": "select last_insert_id() from dual where 1 != 1"
  }
}
***select from dual on unqualified keyspace***
"select @@session.auto_increment_increment from dual"
{
  "Original": "select @@session.auto_increment_increment from dual",
  "Instructions": {
    "Opcode": "SelectUnsharded",
    "Keyspace": {
      "Name": "main",
      "Sharded": false
    },
    "Query": "select @@session.auto_increment_increment from dual",
    "FieldQuery": "select @@session.auto_increment_increment from dual where 1 != 1"
  }
}
***select from dual on sharded keyspace***
"select @@session.auto_increment_increment from user.dual"
{
  "Original": "select @@session.auto_increment_increment from user.dual",
  "Instructions": {
    "Opcode": "SelectEqualUnique",
    "Keyspace": {
      "Name": "user",
      "Sharded": true
    },
    "Query": "select @@session.auto_increment_increment from dual",
    "FieldQuery": "select @@session.auto_increment_increment from dual where 1 != 1",
    "Vindex": "binary",
    "Values": ["\u0000"]
  }
}

@sougou
Copy link
Contributor

sougou commented Oct 11, 2019

I agree in principle. However, we also need to remove the dual table from the vschema. We also have to make sure that the new approach doesn't break any existing use cases that may already be in use.

Another alternative is to categorize dual as a reference table, and change the logic to pick a random shard in such cases.

@sougou
Copy link
Contributor

sougou commented Oct 12, 2019

This is an interesting approach, but it will work incorrectly for non-idempotent expressions like last_insert_id() or rand().

I think changing dual to be a Reference table will be the better approach. However, we should continue to fail last_insert_id() like we already do. We should add a test to show that it fails for dual.

@tinyblink tinyblink changed the title treat the route of sql with pseudo table as SelectDBA treat the route of sql queries with 'dual table as SelectReference Oct 13, 2019
@tinyblink tinyblink changed the title treat the route of sql queries with 'dual table as SelectReference treat the route of sql queries with 'dual' table as SelectReference Oct 13, 2019
@tinyblink
Copy link
Author

tinyblink commented Oct 13, 2019

@sougou Thanks for your advice. I've updated this PR, and only changed a small piece of logic. I also add a test case for last_insert_id().
However, the CI failed unexpectedly, is it an external error or did I miss something?

Signed-off-by: wh <wanghao.seed@gmail.com>
@tinyblink tinyblink force-pushed the handle-sql-with-pseudo-table-properly branch from 77aa432 to 27b5aae Compare October 14, 2019 14:41
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Sorry about not being clear on this one. The change should actually be much simpler. All you have to do is change this function: https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/vindexes/vschema.go#L367 to create dual as a reference table instead of one that's pinned to keyspace id 0.

Signed-off-by: wh <wanghao.seed@gmail.com>
@tinyblink tinyblink force-pushed the handle-sql-with-pseudo-table-properly branch from 32bb9e9 to 001ec07 Compare October 20, 2019 05:59
@tinyblink tinyblink changed the title treat the route of sql queries with 'dual' table as SelectReference treat the type of 'dual' table as 'TypeReference' Oct 20, 2019
@tinyblink
Copy link
Author

@sougou I was just thinking of not changing exist test cases. But what you have mentioned above are totally right, I've fixed it.

@sougou sougou merged commit 67fa436 into vitessio:master Oct 24, 2019
@systay systay mentioned this pull request Nov 7, 2019
JavierR14 pushed a commit to JavierR14/vitess that referenced this pull request Dec 4, 2019
…th-pseudo-table-properly"

This reverts commit 67fa436, reversing
changes made to 3ef22e7.
eeSeeGee pushed a commit to eeSeeGee/vitess that referenced this pull request May 22, 2020
…th-pseudo-table-properly"

This reverts commit 67fa436, reversing
changes made to 3ef22e7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants