sql: audit all usages of Query to use iterator pattern #59339
Conversation
|
This change is |
yuzefovich
force-pushed
the
internal-executor-extra
branch
9 times, most recently
from
c97aef6
to
d1a5870
Compare
yuzefovich
force-pushed
the
internal-executor-extra
branch
from
b84a9a8
to
c6c79f5
Compare
yuzefovich
force-pushed
the
internal-executor-extra
branch
4 times, most recently
from
ae5091e
to
472e94d
Compare
yuzefovich
force-pushed
the
internal-executor-extra
branch
5 times, most recently
from
b28b143
to
6251b7b
Compare
yuzefovich
force-pushed
the
internal-executor-extra
branch
4 times, most recently
from
2baee69
to
dafd8f0
Compare
yuzefovich
changed the title
yuzefovich
requested review from
irfansharif,
rafiss and
otan
and removed request for
irfansharif and
rafiss
|
@irfansharif @otan @rafiss github suggested you as reviewers. Could one of you take a look at this please? |
|
CCing @RichardJCai too as it may touch his idea in #60507 |
|
(Looking at the code areas, I'm definitely not familiar with any of it - so I'll defer elsewhere. Sorry!) |
Similarly to the previous commit, here we audit all usages of `Query` method of the internal executor to take advantage of the iterator API wherever possible (or switching to `Exec` or `QueryRow`). `QueryBuffered` has been added to the interface too. The only place where it would be beneficial to use the iterator pattern but it is not done currently is for `SHOW STATISTICS` statement - in there, we have a panic-catcher which works only on the assumption of not updating any of the shared state (which the iterator API contradicts). Refactoring that part is left as a TODO. Release justification: low-risk update to existing functionality. Release note: None
yuzefovich
force-pushed
the
internal-executor-extra
branch
from
dafd8f0
to
10ede95
Compare
There was a problem hiding this comment.
Reviewed 22 of 22 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @otan, @rafiss, @RichardJCai, and @yuzefovich)
pkg/ccl/backupccl/backup_planning.go, line 961 at r1 (raw file):
// Include all tenants. // TODO(tbg): make conditional on cluster setting. tenantRows, err = p.ExecCfg().InternalExecutor.QueryBuffered(
This could be problematic with many tenants, right?
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/ccl/backupccl/backup_planning.go, line 961 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
This could be problematic with many tenants, right?
Yeah, possibly, but it didn't seem trivial to refactor, so I'll leave it to Bulk IO friends. cc @dt @adityamaru
|
Build failed (retrying...): |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
Similarly to the previous commit (dbc8676), here we audit all usages of
Querymethod of the internal executor to take advantage of the iterator API
wherever possible (or switching to
ExecorQueryRow).QueryBufferedhas been added to the interface too.The only place where it would be beneficial to use the iterator pattern
but it is not done currently is for
SHOW STATISTICSstatement - inthere, we have a panic-catcher which works only on the assumption of not
updating any of the shared state (which the iterator API contradicts).
Refactoring that part is left as a TODO.
Fixes: #48595.
Release justification: low-risk update to existing functionality.
Release note: None