Ability to specify WITH when creating a table + fixes WITH when creating an index
#11296
Conversation
| ] | ||
| ) | ||
| ) | ||
| with_opts = [] |
There was a problem hiding this comment.
how about creating a function for this?
There was a problem hiding this comment.
yeah, i agree, its fixed
| @@ -936,7 +984,19 @@ def test_create_index_with_with(self): | |||
| schema.CreateIndex(idx3), | |||
| "CREATE INDEX test_idx3 ON testtbl " | |||
| "USING gist (data) " | |||
| "WITH (buffering = off)", | |||
| "WITH (buffering = 'off')", | |||
There was a problem hiding this comment.
passing this as string seems accepted, but this kinda seems like a breaking change, so not sure this should be a v2 change.
for 2.1 should be ok, but maybe not on 2.0.
what do you think mike @zzzeek
There was a problem hiding this comment.
works equally well on versions 11-15
https://sqlize.online/sql/psql11/e3e71f14c6fe2cc19090c22ebad1c2fb/
https://sqlize.online/sql/psql12/e3e71f14c6fe2cc19090c22ebad1c2fb/
https://sqlize.online/sql/psql13/e3e71f14c6fe2cc19090c22ebad1c2fb/
https://sqlize.online/sql/psql14/e3e71f14c6fe2cc19090c22ebad1c2fb/
https://sqlize.online/sql/psql15/e3e71f14c6fe2cc19090c22ebad1c2fb/
There was a problem hiding this comment.
This case sure, but this does not make it not a breaking change:
Something like {'foo': 'true', 'bar': 'null'} would likely not work now, since true and null would be passed as strings, not true the bool and null the missing value
There was a problem hiding this comment.
Yes, from that perspective it's a breaking change
Then we'll wait for 2.1
There was a problem hiding this comment.
-1 on quoting "off" , the syntax at https://www.postgresql.org/docs/current/sql-createindex.html shows it without quotes.
There was a problem hiding this comment.
string 'off' is also accepted.
But I too would be for taking string to mean sql text as is (current behavior)
If people want to pass arbitrary things we can document that literal() will be properly compiled, so we could support
{'foo': 'this is a literal statement fragment', 'this is compiled': literal(True)}
There was a problem hiding this comment.
We are looking to support lots more keywords here, with different kinds of expectations, we either have to look at the type of value and format exactly as would be seen in the examples at https://www.postgresql.org/docs/current/sql-createindex.html or we need to (and I might prefer this) build up a list of all known PG keywords for both indexes and tables and include typing information with each . no backwards-incompat changes
| postgresql_using="brin", | ||
| postgresql_with={ | ||
| "pages_per_range": 1, | ||
| "autosummarize": None, |
There was a problem hiding this comment.
-1 on {"autosummarize": None} . looking at docs here: https://www.postgresql.org/docs/current/sql-createindex.html this is a boolean. so this should accept 'off' / 'on'
| @@ -936,7 +984,19 @@ def test_create_index_with_with(self): | |||
| schema.CreateIndex(idx3), | |||
| "CREATE INDEX test_idx3 ON testtbl " | |||
| "USING gist (data) " | |||
| "WITH (buffering = off)", | |||
| "WITH (buffering = 'off')", | |||
There was a problem hiding this comment.
-1 on quoting "off" , the syntax at https://www.postgresql.org/docs/current/sql-createindex.html shows it without quotes.
…fied or no values are specified Fixes: sqlalchemy#11122
4651d4f to
e1f79a2
Compare
Description
Adding the ability to specify storage parameters via with + fix for specifying index storage parameters (should it be split into two different PRs?).
Fixes: #11122
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!