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.

Sign up for GitHub

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

Jump to bottom

tree: formatting of TableIndexName seems wrong #58496

Closed
ajwerner opened this issue Jan 6, 2021 · 6 comments · Fixed by #61776
Closed

tree: formatting of TableIndexName seems wrong #58496

ajwerner opened this issue Jan 6, 2021 · 6 comments · Fixed by #61776
Assignees
@fqazi
Labels
A-logging In and around the logging infrastructure. A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Jan 6, 2021

Describe the problem

A table index name with no table specified will end up being formatted like:

""."".<index name> when using the FQName flag for formatting. That is bogus and doesn't refer to an index. There's also logic for formatting the prefix in this bogus is we have a schema, but it's not clear when that can happen.

See the below code:

cockroach/pkg/sql/sem/tree/table_name.go

Lines 191 to 195 in c08088d

// The table is not specified. The schema/catalog can still be specified.
if n.Table.ExplicitSchema || ctx.alwaysFormatTablePrefix() {
ctx.FormatNode(&n.Table.ObjectNamePrefix)
ctx.WriteByte('.')
}
.

Preferred Solution

Ideally we'd annotate the TableIndexName tree node as at some point during planning we do end up resolving this index to a real table. If we had proper annotations then in the event log context which motivated this issue we'd be able to properly format the name. On the way to that solution, we should stop writing this bogus format which will be interpreted as an object name for an index.

Additional context

This arose in the context of #58472 to fix #57740.
@ajwerner ajwerner added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-logging In and around the logging infrastructure. labels Jan 6, 2021
@ajwerner
Copy link
Contributor Author

ajwerner commented Jan 6, 2021

cc @knz

@knz
Copy link
Contributor

knz commented Jan 6, 2021

How is this a logging issue and not a sql experience issue?

@ajwerner
Copy link
Contributor Author

ajwerner commented Jan 6, 2021 •
edited
Loading

How is this a logging issue and not a sql experience issue?

Updated.

Offline we confirmed that the existing logic when the table is missing is totally bogus. I'm inclined to fix the bug before following through on the annotation as that is somewhat more involved.

@knz
Copy link
Contributor

knz commented Jan 6, 2021

yes that would be nice

@knz knz added the A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect label Jan 6, 2021
@rafiss
Copy link
Collaborator

rafiss commented Mar 3, 2021

@ajwerner @the-ericwang35 just curious -- are there plans to fix the FQName formatting in time for 21.1?

@ajwerner
Copy link
Contributor Author

ajwerner commented Mar 3, 2021

Hmm this feels like a good thing for @fqazi to pick up. I’m going to tentatively assign him.

@ajwerner ajwerner assigned fqazi and unassigned the-ericwang35 Mar 3, 2021
@fqazi fqazi mentioned this issue Mar 10, 2021
Merged
fqazi added a commit to fqazi/cockroach that referenced this issue Mar 10, 2021
@fqazi
sql: formatting of TableIndexName can be wrong in certain contexts
beed9a4 
Fixes: cockroachdb#58496

Previously, the object name information for TableIndexName,
was not populated in multiple contexts leading to incorrect
formatting of index names in event log messages. To address
this, this patch modifies code paths for resolving table
indexes to add this information during the planning phase.

Release note: None
craig bot pushed a commit that referenced this issue Mar 11, 2021
@fqazi @ajwerner
Merge #61776 #61830
a86cf54 
61776: sql: formatting of `TableIndexName` can be wrong in certain contexts r=postamar a=fqazi

Fixes: #58496

Previously, the object name information for TableIndexName,
was not populated in multiple contexts leading to incorrect
formatting of index names in event log messages. To address
this, this patch modifies code paths for resolving table
indexes to add this information during the planning phase.

Release note: None

61830: bench/ddl_analysis: de-flake, clean up format, improve usability r=ajwerner a=ajwerner

See individual commits.

Fixes #57771.

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
@craig craig bot closed this as completed in 6218515 Mar 11, 2021
knz pushed a commit to knz/cockroach that referenced this issue May 21, 2021
@fqazi @knz
sql: formatting of TableIndexName can be wrong in certain contexts
2c6f1af 
Fixes: cockroachdb#58496

Previously, the object name information for TableIndexName,
was not populated in multiple contexts leading to incorrect
formatting of index names in event log messages. To address
this, this patch modifies code paths for resolving table
indexes to add this information during the planning phase.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fqazi fqazi

Labels
A-logging In and around the logging infrastructure. A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: formatting of TableIndexName can be wrong in certain contexts fqazi/cockroach
5 participants
@knz @rafiss @fqazi @ajwerner @the-ericwang35