-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
workload/tpcc: use multiple column families to avoid contention #30624
workload/tpcc: use multiple column families to avoid contention #30624
Conversation
Also FWIW, my investigation in #30213 revealed that transaction restarts were directly correlated with 99th percentile latency in TPC-C. The graphs were almost identical. Tracing confirmed this, as I could see that nearly all long-running txns were restarted at least once. The situation I saw most often in the traces was I mentioned above that:
I'll clarify that intra-txn contention will still lead to transaction retries due to the read-modify-write operation that they often perform when incrementing counters (see #22778 and #29431), but these are performed as the first statement of the |
This is kind of a crazy idea, but this change would actually allow us to remove the client-side retry loops for all txns except the |
I manually verified that without foreign keys, this change, in conjunction with the fixes for #18168 and #30618, break the dependency between In my limited performance testing on my laptop with However, the dependency between the txns was not broken with fks enabled. This is because of #30852. |
42290a4
to
de1f234
Compare
@rohany @solongordon see the last point in @nvanbenschoten's last paragraph. Once the optimizer plans FKs, solving #38280 for lookup joins should break the last dependency here. |
Actually, #30618 hasn't been solved yet either (making sure UPDATE RETURNING doesn't fetch too many column families). I think that's on the optimizer team, though. @RaduBerinde is this issue on the horizon? |
Not really, but I can prioritize it. |
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. This change passes down the minimal required return cols to SQL so it knows to only return columns that's requested. This fixes the output column calculation done by opt and makes sure the execPlan's output columns are the same as output cols of the opt plan. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. This change passes down the minimal required return cols to SQL so it knows to only return columns that's requested. This fixes the output column calculation done by opt and makes sure the execPlan's output columns are the same as output cols of the opt plan. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. This change passes down the minimal required return cols to SQL so it knows to only return columns that's requested. This fixes the output column calculation done by opt and makes sure the execPlan's output columns are the same as output cols of the opt plan. Fixes cockroachdb#30618. Unblocks cockroachdb#30624. Release note: None
38594: opt: fetch minimal set of columns on returning mutations r=ridwanmsharif a=ridwanmsharif Previously, we used to fetch all columns when a mutation contained a `RETURNING` clause. This is an issue because it forces us to retrieve unnecessary data and creates extra contention. This change adds logic to compute the minimal set of required columns and fetches only those. Fixes #30618. Unblocks #30624. Release note: None Co-authored-by: Ridwan Sharif <ridwan@cockroachlabs.com>
804dd41
to
d753ea0
Compare
This commit adds multiple column families to the `warehouse`, `district`, and `customer` tables. These column families split static columns up from columns that are modified by transactions. This effectively removes all contention points between the `NewOrder` and `Payment` transactions. The transactions will still contend with other instances of their same txn type, but that's less of an issue because they should end up being serialized early on, meaning than intra-txn contention should rarely lead to transaction retries (for instance, NewOrder serializes on district.d_next_o_id immediately). These transactions are run about 90% of the time, so this should be a performance win, especially without wait times. Release justification: testing only
d753ea0
to
6b11918
Compare
I'm reviving this PR, with slight modifications to the column families so that this all works properly with the handling column families in the optimizer, in foreign key checks, and with implicit SFU locking. With those changes in place, this change achieves its goal of removing all contention points between the This showed up very prominently in some of the tests I've been running recently. Specifically, I found that a great way to measure the impact of contention on a workload is to run the workload on our very own high-performance, in-memory database – Or course, standard TPC-C with wait times and proper durability isn't nearly as contended, but avoiding dependencies between these transactions still helps. We see this in a complimentary PR I'm about to open. @jordanlewis do you mind giving this another pass? |
@jordanlewis friendly ping on this, now that we're close to the end of the stability period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry for missing this last time.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, though I didn't review the column family choices carefully myself . Are you going to regenerate the fixtures and so on? That's probably the most annoying thing about this change, but important, right?
Reviewed 6 of 6 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained
Thanks for taking a look!
Nope! We recently ripped out all uses of TPC-C fixtures in perf-related tests in favor of IMPORT, specifically so that they no longer need to be regenerated whenever we make minor tweaks like this. The need to regenerate fixtures was a major impediment to tuning the workload, and that didn't seem well justified. bors r+ |
Build succeeded: |
This reverts commit 6b11918. Fixes cockroachdb#55954. Fixes cockroachdb#55953. Fixes cockroachdb#55952. Fixes cockroachdb#55951. Fixes cockroachdb#55949. In cockroachdb#30624, we split various column families up in the TPC-C workload. This had the unintended consequence of breaking CDC roachtests, because CDC does not support multiple column families per row: ``` CHANGEFEEDs are currently supported on tables with exactly 1 column family: warehouse has 2 ``` This PR reverts that commit. I intend to come back to this and introduce a `--families` flag to TPC-C (like we have to YCSB) that can be used to selectively toggle column families on or off. However, I'll do this after cockroachdb#51897 lands, because it's going to be more effort than it's worth to conditionally set this flag based on whether the version of workload used in the test supports it or not.
55983: Revert "tpcc: use multiple column families to avoid contention" r=nvanbenschoten a=nvanbenschoten This reverts commit 6b11918. Fixes #55954. Fixes #55953. Fixes #55952. Fixes #55951. Fixes #55949. In #30624, we split various column families up in the TPC-C workload. This had the unintended consequence of breaking CDC roachtests, because CDC does not support multiple column families per row: ``` CHANGEFEEDs are currently supported on tables with exactly 1 column family: warehouse has 2 ``` This PR reverts that commit. I intend to come back to this and introduce a `--families` flag to TPC-C (like we have to YCSB) that can be used to selectively toggle column families on or off. However, I'll do this after #51897 lands, because it's going to be more effort than it's worth to conditionally set this flag based on whether the version of workload used in the test supports it or not. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
This commit adds multiple column families to the
warehouse
,district
, andcustomer
tables. These column families splitstatic columns up from columns that are modified by transactions.
This effectively removes all contention points between the
NewOrder
and
Payment
transactions. The transactions will still contend withother instances of their same txn type, but that's less of an issue
because they should end up being serialized early on, meaning than
intra-txn contention should rarely lead to transaction retries (for
instance, NewOrder serializes on district.d_next_o_id immediately).
These transactions are run about 90% of the time, so this should be a
performance win, especially without wait times.
Release justification: testing only