sql: move notifying StatsRefresh of new table to post-commit hook#47718
Conversation
|
This change is |
|
@andreimatei this still needs testing but can you validate the approach? cc @rytaft |
|
Fixes #46268. |
rytaft
left a comment
There was a problem hiding this comment.
Thanks!
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
andreimatei
left a comment
There was a problem hiding this comment.
LGTM
Reviewable status:
pkg/sql/conn_executor.go, line 2321 at r1 (raw file):
} // notifyStatsRefresherOfNewTables is call in the post commit hook to inform
is called
pkg/sql/conn_executor.go, line 2321 at r1 (raw file):
} // notifyStatsRefresherOfNewTables is call in the post commit hook to inform
I don't really know what the "post commit hook" is. I'd just say that it's called on txn commit.
pkg/sql/conn_executor.go, line 2326 at r1 (raw file):
func (ex *connExecutor) notifyStatsRefresherOfNewTables(ctx context.Context) { for _, desc := range ex.extraTxnState.tables.getNewTables() { // The CREATE STATISTICS run for an async CTAS query is initiated by the
nit: I'd take the opportunity to clarify this comment, appending a "so we don't do it here".
Prior to this change, the StatsRefresher was being notified of a new table during the execution of the createTable node, before the creating transaction had committed. Prior to cockroachdb#46170, the StatsRefresher was likely to block on the intent of the creating transaction. After that change, the StatsRefresher might not discover the new table because the creating transaction is much more likely to get pushed further into the future, past the read of the stats refresher. Release note (bug fix): Fix rare bug where stats were not automatically generated for a new table.
680cd25 to
1030de5
Compare
ajwerner
left a comment
There was a problem hiding this comment.
I checked and this seems to get exercised a reasonable amount already. LMK if you want a test somewhere and what you have in mind.
Reviewable status:
pkg/sql/conn_executor.go, line 2321 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
is called
Done.
pkg/sql/conn_executor.go, line 2321 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I don't really know what the "post commit hook" is. I'd just say that it's called on txn commit.
Done.
andreimatei
left a comment
There was a problem hiding this comment.
lgtm
Reviewable status:
ajwerner
left a comment
There was a problem hiding this comment.
TFTR!
bors r=andreimatei
Reviewable status:
Build succeeded |
Prior to this change, the StatsRefresher was being notified of a new table
during the execution of the createTable node, before the creating transaction
had committed. Prior to #46170, the StatsRefresher was likely to block on the
intent of the creating transaction. After that change, the StatsRefresher might
not discover the new table because the creating transaction is much more likely
to get pushed further into the future, past the read of the stats refresher.
Release note (bug fix): Fix rare bug where stats were not automatically
generated for a new table.