sql: don't run EXPLAIN ANALYZE via pausable portals #155655
Conversation
yuzefovich
added
backport-25.3.x
|
This change is |
yuzefovich
force-pushed
the
disable-portal-analyze
branch
from
4c723cc to
54ceb43
Compare
yuzefovich
added
the
backport-25.2.x
|
|
||
| subtest explain_analyze | ||
|
|
||
| # Regression test that EXPLAIN ANALYZE doesn't run throught the pausable portal |
There was a problem hiding this comment.
| # Regression test that EXPLAIN ANALYZE doesn't run throught the pausable portal | |
| # Regression test that EXPLAIN ANALYZE doesn't run through the pausable portal |
This commit disables running EXPLAIN ANALYZE stmts via pausable portals model even if it's enabled. Previously, this led to a crash since different assumptions of how EXPLAIN ANALYZE is handled at the connExecutor level were broken, but it also doesn't make much sense to actually support this. After all, the result rows can only be produced _after_ the query execution effectively completed, which contradicts the spirit of the pausable portal feature. I briefly looked into making it work, but it doesn't seem worth it. There is a fundamental contradiction right now that we defer finishing the instrumentationHelper (which populates the output of EXPLAIN ANALYZE), and it currently happens after we close the commandResult in which we can write the output. Release note (bug fix): Previously, CockroachDB would crash when executing EXPLAIN ANALYZE statements with the pausable portal model (meaning that it was run via the extended PGWire protocol using Parse, Bind, Execute, when `multiple_active_portals_enabled` session variable is enabled). The bug has been present since 23.2 and is now fixed.
yuzefovich
force-pushed
the
disable-portal-analyze
branch
from
54ceb43 to
a4d8381
Compare
There was a problem hiding this comment.
Note that if someone actually tries to run EXPLAIN ANALYZE via pausable portal, pause it, run some other statements via pausable portals, and then resume the EXPLAIN ANALYZE, then the behavior could be confusing (we probably will error on the first other stmt), but given the feature is in the preview, it seems acceptable to me.
TFTR!
bors r+
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @ZhouXing19)
|
|
||
| subtest explain_analyze | ||
|
|
||
| # Regression test that EXPLAIN ANALYZE doesn't run throught the pausable portal |
|
Build succeeded: |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #137597: branch-release-25.2, branch-release-25.3, branch-release-25.4. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from a4d8381 to blathers/backport-release-25.2-155655: POST https://api.github.com/repos/yuzefovich/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 25.2.x failed. See errors above. error creating merge commit from a4d8381 to blathers/backport-release-25.3-155655: POST https://api.github.com/repos/yuzefovich/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 25.3.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
155655: sql: don't run EXPLAIN ANALYZE via pausable portals r=yuzefovich a=yuzefovich This commit disables running EXPLAIN ANALYZE stmts via pausable portals model even if it's enabled. Previously, this led to a crash since different assumptions of how EXPLAIN ANALYZE is handled at the connExecutor level were broken, but it also doesn't make much sense to actually support this. After all, the result rows can only be produced _after_ the query execution effectively completed, which contradicts the spirit of the pausable portal feature. I briefly looked into making it work, but it doesn't seem worth it. There is a fundamental contradiction right now that we defer finishing the instrumentationHelper (which populates the output of EXPLAIN ANALYZE), and it currently happens after we close the commandResult in which we can write the output. Fixes: cockroachdb#137597. Release note (bug fix): Previously, CockroachDB would crash when executing EXPLAIN ANALYZE statements with the pausable portal model (meaning that it was run via the extended PGWire protocol using Parse, Bind, Execute, when `multiple_active_portals_enabled` session variable is enabled). The bug has been present since 23.2 and is now fixed. Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
3 participants
This commit disables running EXPLAIN ANALYZE stmts via pausable portals model even if it's enabled. Previously, this led to a crash since different assumptions of how EXPLAIN ANALYZE is handled at the connExecutor level were broken, but it also doesn't make much sense to actually support this. After all, the result rows can only be produced after the query execution effectively completed, which contradicts the spirit of the pausable portal feature.
I briefly looked into making it work, but it doesn't seem worth it. There is a fundamental contradiction right now that we defer finishing the instrumentationHelper (which populates the output of EXPLAIN ANALYZE), and it currently happens after we close the commandResult in which we can write the output.
Fixes: #137597.
Release note (bug fix): Previously, CockroachDB would crash when executing EXPLAIN ANALYZE statements with the pausable portal model (meaning that it was run via the extended PGWire protocol using Parse, Bind, Execute, when
multiple_active_portals_enabledsession variable is enabled). The bug has been present since 23.2 and is now fixed.