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: disable stats injection within an explicit transaction #46567

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Mar 25, 2020

Fixes #42421.

Release justification: Disables a situation that could lead to a
deadlock.

Release note (sql change): Disables to ability to inject stats
within an explicit transaction.

@rohany rohany requested a review from a team as a code owner March 25, 2020 15:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

I don't think this works. There are two problems:

  • the table stats cache is not scoped per txn; with this change, if the txn that injected stats gets aborted, the new stats will still stay in the cache and will be accessible. They are actually accessible to other queries even before the transaction commits or aborts.
  • We want the stats cache to use a separate transaction which can be auto-retried on its own. We don't want a user transaction to get aborted because the table stats hit a conflict (because a table stat was recently refreshed).

I think we should just not support injecting statistics inside a transaction (or do the injection insert using a separate transaction so that it's not tied to the outer transaction).

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)

@rohany
Copy link
Contributor Author

rohany commented Mar 25, 2020

I see, I didn't consider those effects. That makes sense.

I think we should just not support injecting statistics inside a transaction (or do the injection insert using a separate transaction so that it's not tied to the outer transaction).

I'll play around with these ideas.

@rohany rohany force-pushed the inject-stats-hang branch 2 times, most recently from 61cdfc6 to e101d21 Compare March 25, 2020 17:46
@rohany rohany changed the title sql: fix hang when injecting stats and operating on table in same txn sql: disable stats injection within an explicit transaction Mar 25, 2020
@rohany
Copy link
Contributor Author

rohany commented Mar 25, 2020

PTAL

Running the injection in a separate transaction leads to the same deadlock it seems. So I went ahead and disabled stats injection in explicit transactions.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Great, thanks for the fix!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany and @rytaft)


pkg/sql/logictest/testdata/logic_test/alter_table, line 1120 at r1 (raw file):

statement error pq: cannot inject statistics in an explicit transaction
ALTER TABLE inject_stats INJECT STATISTICS '[
  {

[nit] this can probably just be []. We definitely don't need these crazy counts :)

Fixes cockroachdb#42421.

Release justification: Disables a situation that could lead to a
deadlock.

Release note (sql change): Disables to ability to inject stats
within an explicit transaction.
@rohany
Copy link
Contributor Author

rohany commented Mar 25, 2020

lol true :D

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 25, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Mar 25, 2020

Build succeeded

@craig craig bot merged commit ffd0a52 into cockroachdb:master Mar 25, 2020
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: hang in txn with INJECT STATISTICS
3 participants