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: planColumns() is missing TableID information during a merge join #71891

Closed
yuzefovich opened this issue Oct 22, 2021 · 1 comment · Fixed by #72071
Closed

sql: planColumns() is missing TableID information during a merge join #71891

yuzefovich opened this issue Oct 22, 2021 · 1 comment · Fixed by #72071
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team

Comments

@yuzefovich
Copy link
Member

If we add the following diff to row_description PG test

diff --git a/pkg/sql/pgwire/testdata/pgtest/row_description b/pkg/sql/pgwire/testdata/pgtest/row_description
index f18071d10c..8d40c5de7e 100644
--- a/pkg/sql/pgwire/testdata/pgtest/row_description
+++ b/pkg/sql/pgwire/testdata/pgtest/row_description
@@ -135,6 +135,35 @@ ReadyForQuery
 {"Type":"CommandComplete","CommandTag":"SELECT 1"}
 {"Type":"ReadyForQuery","TxStatus":"I"}
 
+send
+Query {"String": "SELECT a, tab1_a FROM tab2 INNER MERGE JOIN tab1 ON a = tab1_a WHERE a = 1"}
+----
+
+# With postgres we don't control the table OID.
+until ignore_table_oids noncrdb_only
+RowDescription
+----
+{"Type":"RowDescription","Fields":[{"Name":"a","TableOID":0,"TableAttributeNumber":1,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"tab1_a","TableOID":0,"TableAttributeNumber":2,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
+
+until crdb_only
+RowDescription
+----
+{"Type":"RowDescription","Fields":[{"Name":"a","TableOID":52,"TableAttributeNumber":1,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"tab1_a","TableOID":53,"TableAttributeNumber":2,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
+
+until ignore_table_oids
+ReadyForQuery
+----
+{"Type":"DataRow","Values":[{"text":"1"},{"text":"1"}]}
+{"Type":"CommandComplete","CommandTag":"SELECT 1"}
+{"Type":"ReadyForQuery","TxStatus":"I"}
+
+until ignore_table_oids
+ReadyForQuery
+----
+{"Type":"DataRow","Values":[{"text":"1"},{"text":"1"}]}
+{"Type":"CommandComplete","CommandTag":"SELECT 1"}
+{"Type":"ReadyForQuery","TxStatus":"I"}
+
 send
 Query {"String": "CREATE TABLE tab3 (a INT8 PRIMARY KEY, b CHAR(8))"}
 ----

it'll fail with

    --- FAIL: TestPGTest/row_description (0.23s)
        datadriven.go:75: 
            testdata/pgtest/row_description:170: RowDescription
            expected:
            {"Type":"RowDescription","Fields":[{"Name":"a","TableOID":52,"TableAttributeNumber":1,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"tab1_a","TableOID":53,"TableAttributeNumber":2,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
            
            found:
            {"Type":"RowDescription","Fields":[{"Name":"a","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"tab1_a","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}

For some reason we're losing some of the information about the columns returned from the query.

@yuzefovich yuzefovich added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Oct 22, 2021
@blathers-crl blathers-crl bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team labels Oct 22, 2021
@otan
Copy link
Contributor

otan commented Oct 27, 2021

This bug involves something around here:

cols = planner.curPlan.main.planColumns()
}
if err := ex.initStatementResult(ctx, res, stmt.AST, cols); err != nil {

initStatementResult relies on planColumns() containing the correct table ID information, which then populates this result.

If you insert:

diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go
index cc8146d786..7659323a2d 100644
--- a/pkg/sql/conn_executor_exec.go
+++ b/pkg/sql/conn_executor_exec.go
@@ -1010,6 +1010,9 @@ func (ex *connExecutor) dispatchToExecutionEngine(
        if stmt.AST.StatementReturnType() == tree.Rows {
                cols = planner.curPlan.main.planColumns()
        }
+       for _, col := range cols {
+               fmt.Printf("columns: %s %s\n", col.Name, col.TableID)
+       }
        if err := ex.initStatementResult(ctx, res, stmt.AST, cols); err != nil {
                res.SetError(err)
                return nil

you will see that the columns have no tableid attached:

columns: a %!s(descpb.ID=0)
columns: tab1_a %!s(descpb.ID=0)
  a | tab1_a
----+---------
(0 rows)

i think the queries team / optimiser should dig further here (as the information comes from the plan)

@otan otan changed the title pgwire: missing TableOID in RowDescription with a merge join sql: planColumns() is missing TableID information during a merge join Oct 27, 2021
@yuzefovich yuzefovich self-assigned this Oct 28, 2021
@craig craig bot closed this as completed in 5e87a89 Nov 4, 2021
@craig craig bot closed this as completed in #72071 Nov 4, 2021
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants