sql: 100x performance degradation in pgjdbc query to fetch columns #55140
Labels
A-sql-memmon
SQL memory monitoring
A-sql-vtables
Virtual tables - pg_catalog, information_schema etc
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Comments
rafiss
added
C-bug
|
I think it's the same as #53416. |
|
Yeah, the query matches the signature of #53416 (comment), thanks for the report. |
|
No problem. I still have the cluster so I'll attach the debug zip here at least until I can figure out all the repro steps. Nice catch recognizing that query string signature! |
rafiss
changed the title
|
Actually it looks like the query is from PGJDBC's getColumns, meaning that any Java ORM/app could hit this. I'm still working on figuring out what initial conditions cause this query to fail. |
|
OK I found a reliable repro. Basically, just creating a lot of columns will cause the issue. Steps:
1100 rows returned but 1375 are expected. Here is the explain output of the problematic query. |
|
Nice repro, works for me |
|
It seems like v20.1.3 also has incorrect results. It also returns 1100 rows instead of 1375 rows. Interestingly, even though v20.1 and v20.2 both have the wrong results, v20.1 completes the getColumns query in 69ms while v20.2 takes ~7 seconds. |
This was referenced Oct 2, 2020
|
20.1 didn't have the But, the query you pasted also doesn't have a constraint on the name of the table - is that what you're testing in 20.2 and 20.1 both? |
|
#48226 is the PR that added vtable lookup join |
|
That's right -- none of the queries should have a constraint on the table name. Hmmm Is my math off? Postgres is also returning 1100 rows. whoops. |
|
OK, so I guess there's not a correctness issue in v20.1 or v20.2. But v20.2 does have the memory accounting issue in the logs: |
|
Makes sense - release binaries don't crash on things like this, merely reporting to sentry. Development binaries crash instead, to make it easier to notice. |
|
This is looking like a weirder than normal memory account issue. We are trying to shrink by 25418 bytes, but only have 25416 bytes available. The small difference makes nervous that there was some kind of mutation to the data being accounted for, rather than a double close or whatever, in which situation we'd see a much larger delta. |
|
Yeah, I noted the thing about not crashing on official releases above. And just to close the loop, my math was wrong because I thought I was making 5 columns per table, but I was actually making 4 columns. Confusingly, I named them So the goals of this issue should be:
I'm guessing those two could be related? |
|
I'm starting to believe my theory about mutated rows. Here is the log of the sizes of all row additions and subtractions from the lookup join node. I used the following modified query that uses only the vtable lookup join that crashes: The numbers in this log sum to zero, as expected. But, I also captured the row sizes from the perspective of the row container, which only actually edits the memory account every 64th removed row due to chunking. Here is the log of those row sizes: Summing up the subtractions in the second log with the additions in the first, we get -2 bytes, the same as the discrepancy in the error. This means that, in between some calls to |
|
Here is the log of the rows removed. The first half ("fake" at the end of the line) is the output of |
|
Forgot to include the log. |
|
The rows on the output side are also out of order... something seems really messed up |
|
It was a memory aliasing issue, a bug in the this only explains the crash, not the slowdown, though. |
jordanlewis
added a commit
to jordanlewis/cockroach
that referenced
this issue
Oct 5, 2020
This commit adds a new function called ConstantWithTestValue that is designed to be used to initialize "magic constants", like batch sizes, that cause edge conditions in the code. ConstantWithTestValue should be used to initialize "magic constants" that should be varied during test scenarios to check for bugs at boundary conditions. When built with the test_constants build tag, the test value will be used. In all other cases, the production value will be used. An example of a "magic constant" that behaves this way is a batch size. Batch sizes tend to present testing problems, because often the logic that deals with what to do when a batch is finished is less likely to be exercised by simple unit tests that don't use enough data to fill up a batch. For example, instead of writing: ``` const batchSize = 64 ``` you should write: ``` var batchSize = util.ConstantWithTestValue(64, 1) ``` This will give your code a batch size of 1 in the test_constants build configuration, increasing the amount of exercise the edge conditions get. This commit also adds several uses of `ConstantWithTestValue` in the SQL package. One of them in particular (the datum row container chunk size) was tailored to catch cockroachdb#55140. Release note: None
rytaft
added a commit
to rytaft/cockroach
that referenced
this issue
Oct 22, 2020
This commit adds a cost equal to 10*randIOCostFactor for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. This commit also includes some fixes to the test catalog to provide better support for testing virtual tables with indexes. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
craig bot
pushed a commit
that referenced
this issue
Oct 23, 2020
55833: opt: add cost for table descriptor fetch during virtual scan r=rytaft a=rytaft This commit adds a cost equal to `10*randIOCostFactor` for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. Fixes #55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables. Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
craig bot
pushed a commit
that referenced
this issue
Oct 24, 2020
55833: opt: add cost for table descriptor fetch during virtual scan r=rytaft a=rytaft This commit adds a cost equal to `10*randIOCostFactor` for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. Fixes #55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables. Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
rafiss
pushed a commit
to rafiss/cockroach
that referenced
this issue
Oct 26, 2020
This commit adds a cost equal to 10*randIOCostFactor for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. This commit also includes some fixes to the test catalog to provide better support for testing virtual tables with indexes. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
|
@rytaft apologies for noticing this late, but I just tried running my reproduction above (#55140 (comment)) and I found that the query is still taking around 6 seconds when I test with the latest version from I'm going to re-open and hopefully we can diagnose this further. |
|
Hmm -- I did run those reproduction steps when I was working on this PR and confirmed that the new plan was significantly faster than the old one (at least an order of magnitude). Are you sure that what you're seeing is plan-related? |
|
Here is the plan I am getting now (taking 6 seconds to execute) Here is explain analyze output. It looks like the EXPLAIN format itself has changed, but it also does seem like this is different plan... There is only one virtual table lookup join now, but it seems to be taking 5.6 seconds. Not sure what's going on (or what is expected) yet. |
rafiss
pushed a commit
to rafiss/cockroach
that referenced
this issue
Nov 3, 2020
This commit adds a cost equal to 10*randIOCostFactor for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. This commit also includes some fixes to the test catalog to provide better support for testing virtual tables with indexes. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
rytaft
added a commit
to rytaft/cockroach
that referenced
this issue
Nov 5, 2020
This commit adds a cost equal to 10*randIOCostFactor for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. This commit also includes some fixes to the test catalog to provide better support for testing virtual tables with indexes. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
rafiss
pushed a commit
to rafiss/cockroach
that referenced
this issue
Nov 5, 2020
This commit adds a cost equal to 10*randIOCostFactor for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. This commit also includes some fixes to the test catalog to provide better support for testing virtual tables with indexes. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
rytaft
added a commit
to rytaft/cockroach
that referenced
this issue
Nov 6, 2020
This commit adds a cost equal to 10*randIOCostFactor for each virtual scan in order to represent the cost of fetching table descriptors. This cost is especially important when perfoming lookup joins, because the descriptors may need to be fetched on each lookup. As a result of this change, the optimizer is much less likely to plan a lookup join into a virtual table. This commit also includes some fixes to the test catalog to provide better support for testing virtual tables with indexes. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
rytaft
added a commit
to rytaft/cockroach
that referenced
this issue
Nov 6, 2020
This commit bumps the cost of each virtual scan to 25*randIOCostFactor from its previous value of 10*randIOCostFactor. This new value threads the needle so that a lookup join will still be chosen if the predicate is very selective, but the plan for the PGJDBC query identified in cockroachdb#55140 no longer includes lookup joins. Fixes cockroachdb#55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables.
craig bot
pushed a commit
that referenced
this issue
Nov 12, 2020
55808: kvserver: skip non-live nodes when considering candidates for transfers r=tbg a=knz (This PR is forked off #55460 to simplify the discussion. I believe there's no discussion left here? Maybe I can merge it directly?) Fixes #55440. Prior to this patch, 3 components could attempt to transfer a replica to a node currently being drained: - the store rebalancer, which rebalances replicas based on disk usage and QPS. - the allocator, to place new replicas. - the allocator, to rebalance replicas depending on load. This commit introduces a consideration for node liveness when building the list of candidates, to detect whether a target node is acceptable. Any node that is not LIVE according to its liveness status is not considered for a transfer. Release note (bug fix): In some cases CockroachDB would attempt to transfer ranges to nodes in the process of being decommissioned or being shut down; this could cause disruption the moment the node did actually terminate. This bug has been fixed. It had been introduced some time before v2.0. 56334: kvserver: use messages on NotLeaseholderErrors everywhere r=andreimatei a=andreimatei NLHE permits custom messages in it, but the field was rarely used. This patch makes every instance where we instantiate the error provide a message, since this error comes from a wide variety of conditions. Release note: None 56345: opt: increase cost for table descriptor fetch during virtual scan r=rytaft a=rytaft This commit bumps the cost of each virtual scan to `25*randIOCostFactor` from its previous value of `10*randIOCostFactor`. This new value threads the needle so that a lookup join will still be chosen if the predicate is very selective, but the plan for the PGJDBC query identified in #55140 no longer includes lookup joins. Fixes #55140 Release note (performance improvement): Adjusted the cost model in the optimizer so that the optimizer is less likely to plan a lookup join into a virtual table. Performing a lookup join into a virtual table is expensive, so this change will generally result in better performance for queries involving joins with virtual tables. 56525: bazel: Move third party repositories to c-deps/REPOSITORIES.bzl r=otan a=alan-mas bazel: Move third party repositories to c-deps/REPOSITORIES.bzl This is one of the Bazel re-factoring that we are working on and it is about to move third party repositories out of root WORKSPACE. fixes #56053 Best practices is to separate external dependencies and it also hides the repo WORKSPACE from being used by other directories. We are creating a new .bzl file inside c-deps with all the external dependencies and then load then inside our root WORKSPACE Release note: None 56589: sql: resolve error due to drop table after schema change in same txn r=ajwerner a=jayshrivastava Previously, if a drop table statement was executed in a transaction following other schema changes to the table in the same transaction, an error would occur. This error was due to the drop table statement marking previous jobs as succeeded and then proceeding to modify them. This change ensures that drop table statement will delete all existing jobs from the job cache so that it does not interfere with previous jobs. Release note (sql change): A table can successfully be dropped in a transaction following other schema changes to the table in the same transaction. This resolves one of the issues in #56235 56597: colflow: fix recent misuse of two slices in the flow setup r=yuzefovich a=yuzefovich We've recently added the reusing of metadataSourcesQueue and toClose slices in order to reduce some allocations. However, the components that are using those slices don't make a deep copy, and as a result, we introduced a bug in which we were breaking the current contract. This commit fixes the issue by going back to the old method (with slight difference in that we currently delay any allocations unlike previously when we allocated a slice with capacity of 1). Release note: None (no release with this bug) 56598: tree: introduce concept of "default" collation r=rafiss a=otan Resolves #54989 Release note (sql change): Introduced a pg_collation of "default". Strings now return the "default" collation OID in the pg_attribute table (this was previously en_US). The "default" collation is also visible on the pg_collation virtual table. 56602: roachpb: remove various `(gogoproto.equal)` options r=nvanbenschoten a=tbg The first commit explains why some cleanup was necessary, the others are the result of spending a little extra time cleaning up "unnecessarily". There are plenty of Equals left to clean up, but the returns were diminishing. The important part is that when additions to the KV API are made, nobody will be forced to add the `equal` option any more. - roachpb: don't generate Equal() on Error - roachpb: remove more `Equal` methods - roachpb: remove (gogoproto.equal) from api.proto - roachpb: mostly remove (gogoproto.equal) from data.proto - roachpb: remove Value.Equal - kvserverpb: remove Equal from ReplicatedEvalResult Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Rebecca Taft <becca@cockroachlabs.com> Co-authored-by: Alanmas <acostas.alan@gmail.com> Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com> Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the problem
I was running the liquibase integration tests and encountered a panic:
To Reproduce
Ran the Liquibase integration tests on a fresh cluster.
This is the query that caused the panic:
But running just the query on a fresh cluster is fine. I haven't figured out what preceding steps would make this panic.
Environment:
Using CockroachDB master (sha 33c1bb6)
The text was updated successfully, but these errors were encountered: