Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Two ALTER commands on a single line fail #108253

Closed
smcvey opened this issue Aug 5, 2023 · 3 comments · Fixed by #109774
Closed

Two ALTER commands on a single line fail #108253

smcvey opened this issue Aug 5, 2023 · 3 comments · Fixed by #109774
Assignees
@annrpom
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@smcvey
Copy link
Contributor

smcvey commented Aug 5, 2023 •
edited by exalate-issue-sync bot
Loading

Describe the problem

Having two ALTER commands on the same line produces the error message:

ERROR: unexpected value: raw_bytes:"z\311\346;\003\020\200\200\200@\030\200\200\200\200\002\"\004\010\220\277\005(\003P\000X\000x\001" timestamp:<>

To Reproduce

root@localhost:26257/defaultdb> ALTER RANGE default CONFIGURE ZONE USING gc.ttlseconds = 90000; ALTER RANGE default CONFIGURE ZONE USING gc.ttlseconds = 90000;                                                                                             
CONFIGURE ZONE 1
ERROR: unexpected value: raw_bytes:"z\311\346;\003\020\200\200\200@\030\200\200\200\200\002\"\004\010\220\277\005(\003P\000X\000x\001" timestamp:<>

However, both ALTER commands do take effect despite the error message. For example, running this will show both 70000 and 60000:

ALTER RANGE default CONFIGURE ZONE USING gc.ttlseconds = 70000; SHOW ZONE CONFIGURATIONS; ALTER RANGE default CONFIGURE ZONE USING gc.ttlseconds = 60000; SHOW ZONE CONFIGURATIONS;

Expected behavior
I would expect both ALTERS to return two CONFIGURE ZONE 1 messages.

Environment:

  • CockroachDB version: All 23.1.x

Additional context
Low impact, just the ominous error message.

Jira issue: CRDB-30386

Epic CRDB-27601
@smcvey smcvey added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 5, 2023
@dikshant dikshant added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Aug 6, 2023
@dikshant

This comment was marked as outdated.

Sign in to view
@rafiss
Copy link
Collaborator

rafiss commented Aug 9, 2023

Internal slack discussion: https://cockroachlabs.slack.com/archives/CHVV403F0/p1691270552695919

CPut /Table/5/1/0/2/1 -> {\x00\x00\x00\x00\x03\x10\x80\x80\x80@\x18\x80\x80\x80\x80\x02\"\x04\b\x90\xbf\x05(\x01P\x00X\x00x\x01 0,0}, expValue: {\x00\x00\x00\x00\x03\x10\x80\x80\x80@\x18\x80\x80\x80\x80\x02\"\x03\b\xc0p(\x01P\x00X\x00x\x01 0,0}
CPut /Table/5/1/0/2/1 -> {\x00\x00\x00\x00\x03\x10\x80\x80\x80@\x18\x80\x80\x80\x80\x02\"\x04\b\x90\xbf\x05(\x01P\x00X\x00x\x01 0,0}, expValue: {\x00\x00\x00\x00\x03\x10\x80\x80\x80@\x18\x80\x80\x80\x80\x02\"\x03\b\xc0p(\x01P\x00X\x00x\x01 0,0}

It looks like the expected values are the same for both statements’ CPuts. Where does the ALTER SET ZONE CONFIGURATION stmt pull the expected value from? If it’s from gossip or some other source that won’t read the first uncommitted write, then this behavior would be explained.

@rafiss rafiss added the db-cy-23 label Aug 9, 2023
@rafiss
Copy link
Collaborator

rafiss commented Aug 9, 2023

Faizan pointed out that it might be related to caching we added to the Collection.

@annrpom annrpom self-assigned this Aug 22, 2023
annrpom added a commit to annrpom/cockroach that referenced this issue Aug 30, 2023
@annrpom
sql: disregard check for RootNameSpaceID
4e1d2ac 
Previously, a bug occured where two `ALTER RANGE default CONFIGURE
ZONE...` statements in a single transaction would produce a CPut
failure, even though both statements do take effect. This was due
to a guard in the code that blocked us from adding the first zone
config to the uncommitted cache. TODO.

TODO(before merge): confirm that the `default` range has RootNamespaceID

Epic: none
Release note (bug fix): two `ALTER RANGE default CONFIGURE ZONE`
statements on the same line no longer displays an error.
Fixes: cockroachdb#108253
@annrpom annrpom mentioned this issue Aug 30, 2023
Merged
annrpom added a commit to annrpom/cockroach that referenced this issue Sep 7, 2023
@annrpom
sql: disregard checks in *ZoneConfig*Batch
2d9a10e 
Previously, a bug occured where a transactional schema change,
 `ALTER RANGE default CONFIGURE ZONE...`, statement would
produce a CPut failure, even though both statements do take effect.
This was due to a guard in the code that blocked us from adding the
first zone config to the uncommitted cache, causing the expValues
being updated to the KV batch to be the same for both statements.
This patch addresses the issue by removing the check and allowing
for `default` and psuedotables (like system ranges) to be added to
the uncommitted cache.

Epic: none
Release note (bug fix): two `ALTER RANGE default CONFIGURE ZONE`
statements on the same line no longer displays an error.
Fixes: cockroachdb#108253
annrpom added a commit to annrpom/cockroach that referenced this issue Sep 7, 2023
@annrpom
sql: disregard checks in *ZoneConfig*Batch
a9588e9 
Previously, a bug occured where a transactional schema change,
 `ALTER RANGE default CONFIGURE ZONE...`, statement would
produce a CPut failure, even though both statements do take effect.
This was due to a guard in the code that blocked us from adding the
first zone config to the uncommitted cache, causing the expValues
being updated to the KV batch to be the same for both statements.
This patch addresses the issue by removing the check and allowing
for `default` and psuedotables (like system ranges) to be added to
the uncommitted cache.

Epic: none
Release note (bug fix): two `ALTER RANGE default CONFIGURE ZONE`
statements on the same line no longer displays an error.
Fixes: cockroachdb#108253
craig bot pushed a commit that referenced this issue Sep 8, 2023
@annrpom @dhartunian
Merge #108824 #109346 #109694 #109774 #110250
9db41b3 
108824: sql: implement datetime builtins r=rafiss,chrisseto a=annrpom

Previously, `make_date`, `make_timestamp`, and `make_timestamptz`
built-ins were not implemented. In addition, a new signature for
`date_trunc` was not implemented. This was inadequate because it caused
an incompatibility issue for tools that needed these datetime functions.
To address this, this patch adds said datetime built-ins. Note that behaviors
do not align with postgres when negative years are inpits, as we adhere
to ISO 8601 standard.

Epic: none
Fixes: #108448

Release note (sql change): Datetime built-ins (make_date,
make_timestamp, and make_timestamptz) are implemented - allowing for
the creation of timestamps, timestamps with time zones, and dates. In
addition, date_trunc now allows for a timestamp to be truncated in a
provided timezone (to a provided precision).

109346: kvcoord: Pace rangefeed client goroutine creation r=miretskiy a=miretskiy

Acquire catchup scan quota prior to goroutine creation in order to pace the goroutine creation rate.

This change results in nice and smooth growth in
goroutine count, thus reducing the pressure on goroutine scheduler, which in turn reduces the impact on SQL latency during changefeed startup.

This change also improves observability in rangefeed client by introducing new counters:
 * `distsender.rangefeed.retry.<reason>`: counter keeping track of the number of ranges that ecountered a retryable error of particular type (e.g. slow counsumer, range split, etc).

Observability also enhanced by adding a column to
 `crdb_internal.active_rangefeed` virtual table augment to indicate
if the range is currently in catchup scan mode.

Fixes #98842

Release note (enterprise change): Pace rangefeed goroutine creation
rate to improve scheduler latency.  Improve observability by adding
additional metrics indicating the reason for rangefeed restart
as well as additional column in the `crdb_internal.active_rangefeed`
table to indicate if the range is currently in catchup scan mode.

109694: ui: all xhr paths from db console are now relative r=santamaura,zachlite,j82w a=dhartunian

This commit removes all prefix `/` characters from request paths in the DB Console and Cluster UI codebases. This ensures that if the DB Console is proxied at a subpath the requests continue to work as expected and are relative to the correct base path.

Resolves: cockroachdb/helm-charts#228
Resolves: #91429

Epic CRDB-21265

Release note (ops change, ui change): The DB Console now constructs client-side requests using relative URLs instead of absolute ones. This enables proxying of the DB Console at arbitrary subpaths.

109774: sql,keys: disregard checks in *ZoneConfig*Batch r=rafiss a=annrpom

Previously, a bug occured where a transactional schema change,
 `ALTER RANGE default CONFIGURE ZONE...`, statement would
produce a CPut failure, even though both statements do take effect.
This was due to a guard in the code that blocked us from adding the
first zone config to the uncommitted cache, causing the expValues
being updated to the KV batch to be the same for both statements.
This patch addresses the issue by removing the check and allowing
for `default` and psuedotables (like system ranges) to be added to
the uncommitted cache.

Epic: none
Release note (bug fix): two `ALTER RANGE default CONFIGURE ZONE` 
statements on the same line no longer displays an error. 
Fixes: #108253

110250: changefeedccl: Fix data race in lagging spans metric r=miretskiy a=miretskiy

Fix a race bug in lagging spans metric.

Fixes: #110235

Release note: None

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
@craig craig bot closed this as completed in e56312f Sep 8, 2023
annrpom added a commit to annrpom/cockroach that referenced this issue Sep 11, 2023
@annrpom
sql: disregard checks in *ZoneConfig*Batch
619c437 
Previously, a bug occured where a transactional schema change,
 `ALTER RANGE default CONFIGURE ZONE...`, statement would
produce a CPut failure, even though both statements do take effect.
This was due to a guard in the code that blocked us from adding the
first zone config to the uncommitted cache, causing the expValues
being updated to the KV batch to be the same for both statements.
This patch addresses the issue by removing the check and allowing
for `default` and psuedotables (like system ranges) to be added to
the uncommitted cache.

Epic: none
Release note (bug fix): two `ALTER RANGE default CONFIGURE ZONE`
statements on the same line no longer displays an error.
Fixes: cockroachdb#108253
Release justification: small bugfix
@annrpom annrpom mentioned this issue Sep 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@annrpom annrpom

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
SQL Foundations
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql,keys: disregard checks in *ZoneConfig*Batch annrpom/cockroach
4 participants
@rafiss @dikshant @smcvey @annrpom