#12308 closed New feature (fixed)
Adding tablespace support to postgres backends
Reported by: | Travis Cline | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | anssi.kaariainen@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The postgres backend(s) should support tablespaces.
Attachments (9)
Change History (33)
by , 15 years ago
Attachment: | postgres_tablespace_support.diff added |
---|
comment:1 by , 15 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
comment:2 by , 15 years ago
Resolution: | duplicate |
---|---|
Status: | closed → reopened |
#6148 does not reference tablespaces.
comment:3 by , 15 years ago
comment:4 by , 15 years ago
I had looked at it before, should have mentioned that it wasn't actually a dupe before to avoid confusion.
comment:5 by , 15 years ago
Question from a newbie: Isn't implementing Operations.tablespace_sql()
also needed so the tablespace support is complete (i.e. not only allowing to set a default tablaspace with the DEFAULT_TABLESPACE
setting but also allow models and field to override it with the Meta.db_tablespace
option)?
comment:6 by , 15 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Accepted for the same reason #6148 was accepted.
Patch needs improvement, though. If we're going to allow tablespaces, it shouldn't be just as a default value. You should be able to override on a per-model basis.
by , 14 years ago
Attachment: | t12308_3.diff added |
---|
Rewritten tests to use lower level connection.creation.sql_create_model api, instead of calling sql management command
by , 14 years ago
Attachment: | t12308_4.diff added |
---|
Removed some whitespace and unnecessary empty set argument
comment:7 by , 14 years ago
Cc: | added |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:8 by , 14 years ago
Cc: | removed |
---|---|
Owner: | changed from | to
Status: | reopened → new |
comment:9 by , 14 years ago
Status: | new → assigned |
---|
comment:10 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:11 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
t12308_4.diff fails to apply cleanly on to trunk
comment:12 by , 13 years ago
UI/UX: | unset |
---|
MySQL supports tablespaces since version 5.1 (catching up, eh?)
http://dev.mysql.com/doc/refman/5.1/en/create-tablespace.html
It may be possible to add support for tablespaces in MySQL at the same time with limited additional work.
comment:13 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Since nothing happened in a year, I take the freedom of reassigning the ticket to myself.
I'm interested in this because I'd like to be able to put the databases in a ramfs on the CI server.
comment:14 by , 13 years ago
Patch needs improvement: | unset |
---|
Tablespaces were temorarily supported by MySQL 5.1 with MySQL NDB Cluster, but they were removed in MySQL 5.5. It doesn't make sense for Django to support them.
I had initially written a patch that includes MySQL support. I later realized that it was not worth the effort. The final patch implements tablespace support in PostgreSQL only.
Apparently, there were no tests for the tablespaces; I have added some. I've added a feature in the BaseDatabaseFeatures
class: supports_tablespaces
. Thus, I can run the appropriate tests, depending on whether the database supports tablespaces. Tests pass under SQLite, PostgreSQL and MySQL.
I noticed that the implementation wasn't conforming to the documentation when ManyToManyField
s were involved. I fixed that bug.
I also cleaned up some related code to avoid spurious spaces and newlines in the generated SQL.
Finally, I refactored the documentation. It used to be in the Oracle notes. It's now a standalone page.
by , 13 years ago
Attachment: | 12308-with-mysql-support-dont-use.patch added |
---|
by , 13 years ago
Attachment: | 12308.patch added |
---|
comment:15 by , 13 years ago
I generated the patch with svn diff
as usual but patch -p0 < 12308.patch
doesn't create tests/modeltests/tablespaces/__init__.py
. You may need to create it manually after applying the patch.
I'm unable to run the tests under Oracle because of #16694.
comment:16 by , 13 years ago
Cc: | added |
---|
On quick look improving the testing of settings.DEFAULT_TABLESPACE would be a plus. Maybe an approach like in modeltests/invalid_models could work? For this kind of a feature testing the syncdb etc would be a big plus but I don't see any way to make that work...
Other notes:
- docs: "These is useful for setting" does not pass my (admittedly very buggy) English parser.
I will try to do a full review today.
comment:17 by , 13 years ago
Patch needs improvement: | set |
---|
Some comments. I can't break it => good!
There are some places which do something like this:
if tablespace: tablespace_sql = self.connection.ops.tablespace_sql(tablespace) if tablespace_sql: tablespace_sql = ' ' + tablespace_sql else: tablespace_sql = ''
If I am not mistaken, this could be written as:
if tablespace and connection_supports_tablespaces: # sorry for the pseudo-code tablespace_sql = ' ' + self.connection.ops.tablespace_sql(tablespace) else: tablespace_sql = ''
IMHO that makes it a little easier to see what is happening. The default implementation of tablespace_sql() could then raise NotImplementedError now that there is a flag which tells if the backend supports SQL. Currently the flag isn't used at all except in tests. There could be an external backend that currently implements tablespace_sql() but will have supports_tablespaces flag False when used in Django 1.4 installation. That backend would use tablespaces, but not support them.
If I am not totally mistaken, the db_tablespace kwarg for models.ManyToManyField does not cause the intermediary table to be created in that db_tablespace. The db_tablespace of the model containing the ManyToManyField is used. Wouldn't it be better that one could use the db_tablespace in ManyToManyField to define the tablespace of the intermediary model? However, that seems to be existing behavior and could be solved in another ticket.
Otherwise looks good. IMHO patch needs improvement based on the added supports_tablespaces flag which then is not used otherwise than in tests.
comment:18 by , 13 years ago
Thanks for your review!
Regarding the confusing code pattern, in fact, the two blocks aren't equivalent. The goal of this construct is to add a space at the beginning of tablespace_sql
only when it isn't the empty string. With your version, if self.connection.ops.tablespace_sql(tablespace)
returns ''
, then tablespace_sql
is set to ' '
, and this results in an extra space in the final SQL.
Currently, Django contains this pattern:
if tablespace: sql = self.connection.ops.tablespace_sql(tablespace) if sql: tablespace_sql = ' ' + sql else: tablespace_sql = '' else: tablespace_sql = ''
Which my patch condenses to:
if tablespace: tablespace_sql = self.connection.ops.tablespace_sql(tablespace) if tablespace_sql: tablespace_sql = ' ' + tablespace_sql else: tablespace_sql = ''
Would it be more explicit like this?
if tablespace: tablespace_sql = self.connection.ops.tablespace_sql(tablespace) else: tablespace_sql = '' if tablespace_sql: tablespace_sql = ' ' + tablespace_sql
(It's slightly less efficient because the second if
isn't necessary when tablespace == ''
.)
Should I just keep the original pattern?
No strong feelings here, I settled for the shorter version because they all looked equivalent to me...
Regarding the "features" flags, some are computed by a the confirm()
method, which is only called from create_test_db()
. Therefore, I think that connection.features
should only be used for the tests (@skipIfDBFeature
/ @skipUnlessDBFeature
). This rule isn't written anywhere, but it's comforted by the fact that connection.features
isn't used anywhere within django.db.backends
.
Full story: in my initial implementation (with MySQL), support for transactions was computed by confirm()
because it depended on the version of MySQL, and I then used features.supports_transactions
to generate the appropriate SQL. It worked perfectly in the tests, and failed everywhere else because confirm()
wasn't called, so features.supports_transaction
was always None
.
I leave "Patch needs improvement" set until we reach an agreement on the first point :)
follow-up: 20 comment:19 by , 13 years ago
Patch needs improvement: | unset |
---|
The pattern you use is fine if the "supports_tablespaces" should not be used when creating the SQL. I was thinking that your code is checking the support for tablespaces by checking if the tablespace_sql() method returns an empty string, while there is also a flag which tells you that. And that seemed wrong. After your explanation that doesn't seem wrong any more.
So, at this point I have the following items:
- trivial typo in documentation: "These is useful for setting ...".
- As mentioned by author, the patch lacks
__init__
.py for the tests/modeltests/tablespaces/ directory.
So, I guess the patch is ready for committer who can do the final polish. I won't mark this ready for checkin, as I have no possibility to test this on Oracle, nor do I know how to actually compile the documentation. Maybe I am taking the "ready for checkin" a bit too literally? :)
comment:20 by , 13 years ago
Replying to akaariai:
Maybe I am taking the "ready for checkin" a bit too literally? :)
You aren't; "ready for checkin" means "if I were a core developer, I'd commit this patch as is".
Unless the problem is trivial (like a typo in the docs), there must be an exact patch, to avoid wasting the time of the core developer who commits the patch.
I ran the tests under Oracle 10g, and they failed because tablespace names are capitalized by the Oracle backend. I fixed this bug and re-ran the tests under SQLite, PostgreSQL, MySQL and Oracle; they pass everywhere.
I'm uploading a new patch that adresses this problem, your two remaining remarks, and also removes a trailing space. To represent the new, empty file, I created a diff in git format, but it still doesn't work with patch -p1
, use git apply
instead.
by , 13 years ago
Attachment: | 12308.2.patch added |
---|
by , 13 years ago
Attachment: | 12308.3.patch added |
---|
comment:21 by , 13 years ago
Patch updated to address a bad interaction with the proxy_model_inheritance
(thanks Ramiro).
I stole the app cache cleanup code from tests/modeltests/invalid_models/tests.py
.
#11685 asked for the same thing and was closed as a dupe of an even earlier ticket. Please search the tracker before opening new tickets.