-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
sql: support synchronous_commit and enable_seqscan as dummy no-op #52168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @otan)
pkg/sql/unsupported_vars.go, line 96 at r1 (raw file):
"enable_mergejoin", "enable_nestloop", //"enable_seqscan",
[nit] add a space after //
pkg/sql/logictest/testdata/logic_test/set, line 360 at r1 (raw file):
query T noticetrace SET synchronous_commit = off; SET enable_seqscan = false
what about also adding a test for enabling these (setting to true)? Would be good to add a test to SHOW
the current value as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @otan)
pkg/sql/logictest/testdata/logic_test/set, line 380 at r2 (raw file):
query T noticetrace SET synchronous_commit = off; SET enable_seqscan = true
[nit] maybe you meant synchronous_commit = on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)
pkg/sql/logictest/testdata/logic_test/set, line 360 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
what about also adding a test for enabling these (setting to true)? Would be good to add a test to
SHOW
the current value as well.
caught a few things, oops. thanks for the suggestion
pkg/sql/logictest/testdata/logic_test/set, line 380 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] maybe you meant synchronous_commit = on?
ye!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
These vars are set by `osm2pgsql` and `ogr2ogr` respectively. These default to the ON state, the OFF state affects performance but not correctness. Release note (sql change): Support the setting and getting of the `synchronous_commit` and `enable_seqscan` variables, which do not affect any performance characteristics. These are no-ops enabled to allow certain tools to work.
tftr! bors r=rytaft |
Build succeeded: |
These vars are set by
osm2pgsql
andogr2ogr
respectively. Thesedefault to the ON state, the OFF state affects performance but not
correctness.
Touches #51818.
Release note (sql change): Support the setting and getting of the
synchronous_commit
andenable_seqscan
variables, which do not affectany performance characteristics. These are no-ops enabled to allow
certain tools to work.