Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Support for SET statements. #1073

Merged
merged 25 commits into from Aug 16, 2020
Merged

Support for SET statements. #1073

merged 25 commits into from Aug 16, 2020

Conversation

lmwnshn
Copy link
Contributor

@lmwnshn lmwnshn commented Aug 11, 2020

SET talks directly to the settings manager, poking the values.

With parser SET support yoinked from @thepinetree, thanks!

Known issues

  • SettingsManager assumes that 1 setting = 1 constant value expression. This is not necessarily the case in postgres, which can have SET foo TO bar,baz.
  • Legacy? ActionContext and SettingsCallback stuff is not being used by DBMain or throughout the system.

Example usage

Support for invalid values (type mismatch):

terrier=# set record_buffer_segment_size to a;
ERROR:  invalid value for parameter "record_buffer_segment_size": "a"
terrier=# \errverbose
ERROR:  22023: invalid value for parameter "record_buffer_segment_size": "a"
LOCATION:  /home/cmudb/CLionProjects/terrier/src/settings/settings_manager.cpp:272

Support for invalid values (out of range):

terrier=# set record_buffer_segment_size to -1;
ERROR:  -1 is outside the valid range for parameter "record_buffer_segment_size" (1 .. 1000000)
terrier=# \errverbose
ERROR:  22023: -1 is outside the valid range for parameter "record_buffer_segment_size" (1 .. 1000000)
LOCATION:  /home/cmudb/CLionProjects/terrier/src/settings/settings_manager.cpp:140

Support for valid value but runtime error:

terrier=# set record_buffer_segment_size to 1;
ERROR:  Callback failed after setting "record_buffer_segment_size" from "100000" to "1", revert succeeded.
terrier=# \errverbose
ERROR:  XX000: Callback failed after setting "record_buffer_segment_size" from "100000" to "1", revert succeeded.
LOCATION:  /home/cmudb/CLionProjects/terrier/src/settings/settings_manager.cpp:140

Support for valid value being set (I don't know how to show a setting, you can see it changed in the error message):

terrier=# set record_buffer_segment_size to 100001;
SET
terrier=# set record_buffer_segment_size to 1;
ERROR:  Callback failed after setting "record_buffer_segment_size" from "100001" to "1", revert succeeded.

terrier=# set record_buffer_segment_size to 5;
SET
terrier=# set record_buffer_segment_size to 1;
ERROR:  Callback failed after setting "record_buffer_segment_size" from "5" to "1", revert succeeded.

Also added support for DEFAULT. There was a bug with the settings manager that wouldn't let you define more than one string setting, that was fixed.

@lmwnshn lmwnshn added in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. feature Adds a requested feature labels Aug 11, 2020
Copy link
Contributor

@thepinetree thepinetree left a comment

Choose a reason for hiding this comment

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

Did a pass. Looks good too me.

@mbutrovich mbutrovich added ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. and removed in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. labels Aug 13, 2020
mbutrovich
mbutrovich previously approved these changes Aug 14, 2020
Copy link
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

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

Investigating JUnit failures...

@mbutrovich mbutrovich added in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. and removed ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. labels Aug 14, 2020
@mbutrovich mbutrovich dismissed their stale review August 14, 2020 18:08

Traffic Cop is broken.

@mbutrovich mbutrovich self-requested a review August 14, 2020 18:08
@mbutrovich mbutrovich added ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. and removed in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. labels Aug 14, 2020
@codecov
Copy link

codecov bot commented Aug 16, 2020

Codecov Report

Merging #1073 into master will decrease coverage by 0.24%.
The diff coverage is 58.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1073      +/-   ##
==========================================
- Coverage   81.47%   81.22%   -0.25%     
==========================================
  Files         650      649       -1     
  Lines       42978    42281     -697     
==========================================
- Hits        35015    34343     -672     
+ Misses       7963     7938      -25     
Impacted Files Coverage Δ
...lude/parser/expression/constant_value_expression.h 96.29% <ø> (ø)
src/include/traffic_cop/traffic_cop.h 100.00% <ø> (ø)
...rc/parser/expression/constant_value_expression.cpp 86.48% <0.00%> (-8.15%) ⬇️
src/parser/postgresparser.cpp 84.96% <42.85%> (-0.54%) ⬇️
src/traffic_cop/traffic_cop.cpp 72.24% <46.66%> (-8.72%) ⬇️
src/network/postgres/postgres_network_commands.cpp 72.66% <60.00%> (-7.57%) ⬇️
src/settings/settings_manager.cpp 76.10% <64.70%> (+6.24%) ⬆️
src/include/main/db_main.h 94.04% <100.00%> (+0.41%) ⬆️
src/include/network/network_util.h 100.00% <100.00%> (ø)
src/include/parser/variable_set_statement.h 93.75% <100.00%> (+4.27%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4251c18...c68ed42. Read the comment docs.

Copy link
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

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

LGTM

@mbutrovich mbutrovich added ready-to-merge This PR is ready to be merged. Mark PRs with this. and removed ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. labels Aug 16, 2020
@lmwnshn lmwnshn merged commit 3b16bea into cmu-db:master Aug 16, 2020
@lmwnshn lmwnshn deleted the set_sql branch August 17, 2020 01:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Adds a requested feature ready-to-merge This PR is ready to be merged. Mark PRs with this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants