Skip to content

cli/sql: delay writing the history file to the end#88272

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
knz:20220920-delay-write-history
Sep 20, 2022
Merged

cli/sql: delay writing the history file to the end#88272
craig[bot] merged 1 commit intocockroachdb:masterfrom
knz:20220920-delay-write-history

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Sep 20, 2022

Fixes #54679.

Previously, the shell would write to the history file in-between each input.

This was incurring noticeable delays if the history file was large.

This patch fixes that by delaying the file write until the end. This is akin to the behavior of unix shell.

There is no user-visible change except in the case of a crash of the shell itself, which at this point has become extremely rare.

Release note: None

Previously, the shell would write to the history file in-between each
input.

This was incurring noticeable delays if the history file was
large.

This patch fixes that by delaying the file write until the end.
This is akin to the behavior of unix shell.

There is no user-visible change except in the case of a crash of the
shell itself, which at this point has become extremely rare.

Release note: None
@knz knz requested review from a team and DrewKimball September 20, 2022 18:40
@knz knz requested a review from a team as a code owner September 20, 2022 18:40
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this so quickly!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @knz)


pkg/cli/clisqlshell/sql.go line 2181 at r1 (raw file):

				// large and we don't want the excess I/O latency to be interleaved
				// in-between every command.
				c.ins.SetAutoSaveHistory(histFile, false)

Just to make sure, saving the history to the file doesn't flush the in-memory data right? So this change shouldn't increase memory usage.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Sep 20, 2022

Just to make sure, saving the history to the file doesn't flush the in-memory data right? So this change shouldn't increase memory usage.

yes correct.

Copy link
Copy Markdown
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @knz)

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Sep 20, 2022

TFYR!

bors r=DrewKimball

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 20, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 20, 2022

Build succeeded:

@craig craig bot merged commit 77c412d into cockroachdb:master Sep 20, 2022
@knz knz deleted the 20220920-delay-write-history branch September 21, 2022 01:27
knz added a commit to knz/cockroach that referenced this pull request Sep 22, 2022
In light of PR cockroachdb#88272, we don't need the limit to be as low as 1000.
This commit increases it back to 10000, which is the common default in
unix shells and the screen/tmux utilities.

Release note: None
craig bot pushed a commit that referenced this pull request Sep 22, 2022
87808: norm: do not fold floor division with unit denominator r=msirek a=msirek

Fixes #87605

The floor division operator `//` is like division but drops the
fractional portion of the result. Normalization rule FoldDivOne
rewrites `(column // 1)` as `(column)` which is incorrect if the
data in `column` has fractional digits, as those digits should be
dropped.

The solution is to only fold `(column // 1)` when `column` is one of
the integer types.

Release note (bug fix): This patch fixes incorrect results from
the floor division operator, `//`, when the numerator is non-constant
and the denominator is the constant 1.

88082: *: audit operation names in new schema changer r=Xiang-Gu a=Xiang-Gu

This PR made non-functional changes related to new schema changer ops:
1. Rename all status changing operation on Index and Column to comform
to the pattern: `Make[StatusA][Index/Column][StatusB]`

2. Rename a few constant/field/function names to be consistent with the
principle of "being explicit":
  2.1. Rename operation name `XxxGcXxx` to `XxxGCXxx`;
  2.2. Rename field name in operations `DescID` to `DescriptorID`;
  2.3. Rename descpb.DELETE_AND_WRITE_ONLY to descpb.WRITE_ONLY;
  2.4. Rename sql.RunningStatusDeleteAndWriteOnly to sql.RunningStatusWriteOnly

3. Seveal comments enhencement

4. Rename `DELETE_AND_WRITE_ONLY` to `WRITE_ONLY` in some files under
docs/.

Fixes #84583

Release note: None

88459: kv: fix spelling of "interceptor" in file name r=nvanbenschoten a=nvanbenschoten

All other file names and code objects have the correct spelling.

Release justification: None.

Release note: None.

88468: kvserver/batcheval: ignore UseRangeTombstonesForPointDeletes knob sometimes r=ajwerner a=ajwerner

We want to ignore the point deletion knob for cases where we're deleting data from system tables watched by rangefeeds. Not doing so can lead to missed updates. This is particularly critical for cluster settings.

Fixes #87575
Fixes #87479
Fixes #88202

Release note: None

88475: workload/schemachange/mixed-version: avoid index visibility r=fqazi a=fqazi

Fixes: #87768

Previously, we were generating statements with invisible indexes in mixed version environments, which could lead to statements generated not supported on older versions. To address this, this patch will prevent the generation of create table/ create index statements with invisible indexes.

Release note: None

88476: multitenant: add TestTenants() helper to TestServer r=ajstorm a=msbutler

This patch adds a helper method for accessing information about the tenant(s) that were probabilistically created with the TestServer.

Release note: None

88478: cli/sql: increase maxHistEntries r=ajwerner a=knz

As discussed this morning.

In light of PR #88272, we don't need the limit to be as low as 1000. This commit increases it back to 10000, which is the common default in unix shells and the screen/tmux utilities.


Co-authored-by: Mark Sirek <sirek@cockroachlabs.com>
Co-authored-by: Xiang Gu <xiang@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cockroachdb cli client is slower than.. itself

3 participants