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: qualify table name for COMMENT ON INDEX, COLUMN event logs #58472

Merged
merged 1 commit into from
Jan 6, 2021
Merged

sql: qualify table name for COMMENT ON INDEX, COLUMN event logs #58472

merged 1 commit into from
Jan 6, 2021

Conversation

the-ericwang35
Copy link
Contributor

Fixes #57740.

Release note (bug fix): qualify table name for COMMENT ON INDEX, COLUMN.

Previously, event logs were not capturing the qualified table names
for COMMENT ON INDEX and COMMENT ON COLUMN commands.
This PR changes the event logs to use the qualified table name.
Tests were also added for these changes.

Fixes #57740.

Release note (bug fix): qualify table name for COMMENT ON INDEX, COLUMN.

Previously, event logs were not capturing the qualified table names
for COMMENT ON INDEX and COMMENT ON COLUMN commands.
This PR changes the event logs to use the qualified table name.
Tests were also added for these changes.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/sql/logictest/testdata/logic_test/event_log, line 941 at r1 (raw file):

WHERE "eventType" = 'comment_on_index'
----
1  {"Comment": "This is an index.", "EventType": "comment_on_index", "IndexName": "b_index", "Statement": "COMMENT ON INDEX \"\".\"\".b_index IS 'This is an index.'", "TableName": "defaultdb.public.a", "User": "root"}

this ""."".b_index is suspect. I think I understand why it's happening but it's somewhat unfortunate. I think we want to update the way that tree.TableIndexName formats itself around here:

// 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('.')
}
.

The issue here is that the TableIndexName doesn't end up being an annotated node so we don't end up knowing how to resolve this appropriately. In the short term, I think I'm in favor of omitting the ""."". prefix when formatting this statement if the prefix is empty regardless of the flags. In the longer term I'd like us to annotate the node to properly resolve the prefix. Also, the logic to not include a prefix in that node except when we don't know the object name is particularly bizarre.

I'm thinking that for now this is what it is and is not touched by this code but I filed #58496 to follow up.

@the-ericwang35
Copy link
Contributor Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Jan 6, 2021

Build succeeded:

@craig craig bot merged commit 9fa3e82 into cockroachdb:master Jan 6, 2021
@the-ericwang35 the-ericwang35 deleted the ericwang/qualify_table_name branch January 6, 2021 16:26
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.

sql: event log entry for COMMENT ON INDEX,COLUMN does not properly qualify its TableName payload
3 participants