Frontend peek sequencing -- don't bail out after plan
#34491
Conversation
ggevay
force-pushed
the
frontend-peek-plan-bailouts
branch
2 times, most recently
from
af67b6a to
fad4008
Compare
ggevay
requested review from
SangJunBak and
aljoscha
and removed request for
SangJunBak
ggevay
force-pushed
the
frontend-peek-plan-bailouts
branch
from
f3785cc to
12683a8
Compare
| _ => { | ||
| debug!( | ||
| "Bailing out from try_frontend_peek_inner, because EXPLAIN FILTER PUSHDOWN is not for a SELECT query" | ||
| "Bailing out from try_frontend_peek_inner, because EXPLAIN FILTER PUSHDOWN is not for a (non-BROKEN) SELECT query" |
There was a problem hiding this comment.
maybe "is for BROKEN SELECT query" instead of is not for a non-broken? 😅
There was a problem hiding this comment.
I'll fix this in my next PR (statement logging: #34305), to avoid some merge conflicts, if that's ok.
| tx: oneshot::Sender<Result<ExecuteResponse, AdapterError>>, | ||
| }, | ||
|
|
||
| /// Execute a side-effecting function from the frontend peek path. |
There was a problem hiding this comment.
IMO, we don't have to mention here that it's from the frontend peek path, eventually everything will come "from the frontend". But maybe you have plans to clean this up holistically already once the work is done
There was a problem hiding this comment.
Yes, this is captured in the
"There are lots of refactorings that we could/should do after the old peek sequencing is deleted."
todo item in https://github.com/MaterializeInc/database-issues/issues/9593.
aljoscha
left a comment
aljoscha
left a comment
There was a problem hiding this comment.
looks good! I had some inline suggestions
|
Thank you! |
2 participants
This is a pre-requisite for adding statement logging to the frontend peek sequencing (#34305):
We'll want to log the beginning of statement execution before planning (the
mz_sql::plan::plancall). This means that we can't have any bailout points from the frontend peek sequencing to the old peek sequencing after planning, because in that case both the old and the new peek sequencing would log the beginning of statement execution, resulting in a duplicate entry (and one of them getting stuck in an unfinished state forever).Therefore, this PR attends to all 4 reasons that currently cause us to bail out after planning, see the 4 commit messages.
The relevant tests for
pg_cancel_backendare:test_pg_cancel_backendalter-sink.td,cancel-subscribe.tdThe relevant tests for
waiting_on_startup_appendsare:test_mz_sessions,test_pg_cancel_dropped_roletest-http-race-condition,test-mz-subscriptionswindow_funcs.sltI've ran all of these tests locally with frontend peek sequencing turned both on and off.