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

sql: parse roundtrip #30141

Closed
maddyblue opened this issue Sep 12, 2018 · 8 comments
Closed

sql: parse roundtrip #30141

maddyblue opened this issue Sep 12, 2018 · 8 comments
Assignees
Labels
A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@maddyblue
Copy link
Contributor

maddyblue commented Sep 12, 2018

Parse followed by Format is not idempotent: "WITH ident ( EXTRACT , INTERLEAVE , ident , ORDINALITY , ident ) AS ( IMPORT TABLE ident . FULL FROM INTERVAL ( PLACEHOLDER ) WITH ident = PLACEHOLDER ) , SCHEMA ( BOOL , BIGINT ) AS ( RESTORE ident FROM IS ) DELETE FROM ident . RETURNING . ILIKE * ORDER BY PRIMARY KEY TRIM . ident . GROUPING DESC RETURNING NOTHING" -> "WITH ident ("extract", interleave, ident, "ordinality", ident) AS (IMPORT TABLE ident.full FROM INTERVAL 'placeholder' WITH ident = 'placeholder') , schema (bool, "bigint") AS (RESTORE TABLE ident FROM 'is') DELETE FROM ident.returning.ilike ORDER BY PRIMARY KEY "trim".ident.grouping DESC RETURNING NOTHING" != "WITH ident ("extract", interleave, ident, "ordinality", ident) AS (IMPORT TABLE ident.full FROM INTERVAL 'placeholder' WITH ident = 'placeholder') , schema (bool, "bigint") AS (RESTORE TABLE ident FROM 'is' WITH ident = 'placeholder') DELETE FROM ident.returning.ilike ORDER BY PRIMARY KEY "trim".ident.grouping DESC RETURNING NOTHING"

@maddyblue
Copy link
Contributor Author

Simplified to:

WITH
a AS (backup table t to $1 with a),
y  AS (RESTORE TABLE u FROM $1)
select 1;

Also:

WITH
a AS (RESTORE TABLE u FROM $1),
y  AS (IMPORT TABLE t FROM csv $1 WITH i)
select 1;

Problem: the WITH options are being copied from the import/backup to the restore. I can't get any other combination to produce the bug (two restores, backup+import).

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect labels Sep 13, 2018
@knz
Copy link
Contributor

knz commented Sep 13, 2018

Wow nice find.

@maddyblue
Copy link
Contributor Author

Yeah this is so weird I don't even know where to look but it feels like a pointer is being duplicated somewhere? Maybe WITH is doing something bad? I have no idea.

@maddyblue
Copy link
Contributor Author

This just gets weirder and weirder.

Works fine:

WITH 
 a AS (backup table t to $1 with a = 'aoe', c = 'oaeu')
,b AS (backup table t to $1 with a)
,y AS (RESTORE TABLE u FROM $1 )
select 1;

Starting to add yacc to the suspect list.

@knz
Copy link
Contributor

knz commented Sep 13, 2018

I think I found it: the empty rules in opt_with_options and opt_with_clause should not be {} but instead {$$.val = nil}.

Can you try and see what happens?

Maybe we should also audit the other empty clauses throughout the grammar.

@jordanlewis
Copy link
Member

Ugh... that's my fault. Sorry :( I didn't know that you had to explicitly nil stuff out!

@knz
Copy link
Contributor

knz commented Sep 13, 2018

a yacc-generated parser is a giant switch in a loop and the data structure is a stack implemented as indexes into an array. Each occurrence of $$.val is really thestack[curpos].val. Unless you nil things out the previous value remains.

(That's true of all yacc generators that don't provide destructors for values. Bison and Lemon provide destructors, for example, go-yacc and old Bison don't)

@knz
Copy link
Contributor

knz commented Sep 13, 2018

I'll make the patch!

craig bot pushed a commit that referenced this issue Sep 13, 2018
29692: ui: various glue fixes r=vilterp,couchand a=benesch

This PR restores the "no UI installed" message in short binaries:

![image](https://user-images.githubusercontent.com/882976/45196553-e713b880-b22a-11e8-928a-06c7a2da0f63.png)

I came across a few minor nits that seemed worth fixing too.

30135: sql: add '2.0' setting for distsql r=jordanlewis a=jordanlewis

The 2.0 setting for distsql (both a cluster setting and a session
setting) instructs the executor to use the 2.0 method of determining how
to execute a query: the query runs via local sql unless the query is
both distributable and recommended to be distributed, in which case it
runs via the distsql and is actually distributed.

Release note (sql change): add the '2.0' value for both the distsql
session setting and the sql.defaults.distsql cluster setting, which
instructs the database to use the 2.0 'auto' behavior for determining
whether queries run via distsql or not.

30148: storage: add new metrics for the RaftEntryCache r=nvanbenschoten a=nvanbenschoten

Four new metrics are introduced:
- `raft.entrycache.bytes`
- `raft.entrycache.size`
- `raft.entrycache.accesses`
- `raft.entrycache.hits`

30163: kv: Don't evict from leaseholder cache on context cancellations r=a-robinson a=a-robinson

This was a major contributor to the hundreds of NotLeaseHolderErrors per
second that we see whenever we run tpc-c at scale. A non-essential batch
request like a QueryTxn would get cancelled, causing the range to be
evicted from the leaseholder cache and the next request to that range to
have to guess at the leaseholder.

This is effectively an extension of #26764 that we should have thought to inspect more closely at the time.

Actually fixes #23543, which was not fully fixed before. Although I still haven't seen the errors drop all the way to 0, so I'm letting tpc-c 10k continue to run for a while longer to verify that they do. They are continuing to decrease about 15 minutes in. I don't think getting to 0 will be possible because there are still occasional splits and lease transfers), but it looks like it should be able to get down to single digit errors per second from the hundreds it was at before this change.

Also, avoid doing unnecessary sorting by latency of replicas in the dist_sender in the common case when we know who the leaseholder is and plan on sending our request there.

30197: sql/parser: fix the action for empty rules r=knz a=knz

Fixes #30141.


Co-authored-by: Nikhil Benesch <[email protected]>
Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Alex Robinson <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig craig bot closed this as completed in #30197 Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

3 participants