sqlbase, distsql: (memory) row container fixes #17302
Conversation
The `sv` was left over from `sorterValues`; renaming to `mc`.
While working on another change, I encountered a bug (exposed by `TestDiskContainer`): we are passing a `memRowContainer` by value and modifying it. This results in incorrect memory accounting (the memory account is embedded in the structure). The fix is easy - we just need to pass a pointer. To prevent similar bugs, we disallow copying of `RowContainer` (using `NoCopy`). The `MakeRowContainer` function is replaced with an `Init` method.
|
This change is |
|
Reviewed 1 of 1 files at r1, 8 of 8 files at r2, 3 of 3 files at r3. pkg/sql/logictest/testdata/logic_test/distsql_agg, line 417 at r3 (raw file):
I never liked this test. Glad to see it go! pkg/sql/sqlbase/row_container.go, line 308 at r3 (raw file):
This section could use a comment saying what's happening (roughly an adaptation of the commit message from r3). Comments from Reviewable |
RaduBerinde
force-pushed
the
row-container-stuff
branch
from
d7f9a69 to
ba1f768
Compare
|
Thanks for the reviews! Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion. pkg/sql/sqlbase/row_container.go, line 308 at r3 (raw file): Previously, arjunravinarayan (Arjun Narayan) wrote…
Done. Comments from Reviewable |
This wasn't critical before because we were always popping rows during a draining (followed by a call to `Clear` or `Close`). With the buffering router, we will be alternating between popping rows and adding more rows, so we need to release the memory. Removing a distsql_agg "memory exceeded" test - it inadvertently used tuples and wasn't run via distsql, and it now times out with local SQL (it no longer exceeds the budget).
RaduBerinde
force-pushed
the
row-container-stuff
branch
from
ba1f768 to
9712cc2
Compare
|
Reviewed 1 of 1 files at r4. Comments from Reviewable |
for observability This patch introduces 2 new system privileges VIEWDEBUG and VIEWCLUSTERMETADATA. VIEWDEBUG will now be used to gate taking traces and viewing debug endpoints. VIEWCLUSTERMETADATA will now be used to gate the node and range reports. The patch also introduces 3 new builtin roles: crdb_internal_cluster_activity_reader which has the system privilege VIEWACTIVITY. crdb_internal_cluster_activity_writer which has the system privilege CANCELQUERY. crdb_internal_cluster_metadata_reader which has the system privileges VIEWCLUSTERMETADATA, VIEWCLUSTERSETTINGS, and VIEWDEBUG. Resolves cockroachdb#17301, cockroachdb#17302, cockroachdb#17312, cockroachdb#17313, cockroachdb#17316 Release note (sql change): add VIEWDEBUG and VIEWCLUSTERMETADATA system privileges. Add cluster_activity_reader, cluster_activity_writer, cluster_metadata_operator builtin roles.
distsql: rename receiver for memRowContainer methods
The
svwas left over fromsorterValues; renaming tomc.sqlbase: disallow copying of RowContainers
While working on another change, I encountered a bug (exposed by
TestDiskContainer): we are passing amemRowContainerby value and modifyingit. This results in incorrect memory accounting (the memory account is embedded
in the structure).
The fix is easy - we just need to pass a pointer. To prevent similar bugs, we
disallow copying of
RowContainer(usingNoCopy). TheMakeRowContainerfunction is replaced with an
Initmethod.sqlbase: release memory when popping rows from RowContainer
This wasn't critical before because we were always popping rows during a
draining (followed by a call to
ClearorClose). With the buffering router,we will be alternating between popping rows and adding more rows, so we need to
release the memory.
Removing a distsql_agg "memory exceeded" test - it inadvertently used tuples and
wasn't run via distsql, and it now times out with local SQL (it no longer
exceeds the budget).