Adding tablespace support to postgres backends

[Travis Cline]

The postgres backend(s) should support tablespaces.

[Karen Tracey]

#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.

[Travis Cline <travis.cline@…>]

#6148 does not reference tablespaces.

[Alex Gaynor]

Note that I originally refereed Travis to #6148 but as he pointed out that doesn't reference tablespaces, so I asked him to file this new ticket. If #6148 should encompass that was well the ticket should be amended to note this.

[Travis Cline <travis.cline@…>]

I had looked at it before, should have mentioned that it wasn't actually a dupe before to avoid confusion.

[Ramiro Morales]

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)?

[Russell Keith-Magee]

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.

[Andrii Kurinnyi]

Patch for current trunk r13359

[Andrii Kurinnyi]

added tests

[Andrii Kurinnyi]

Rewritten tests to use lower level connection.creation.sql_create_model api, instead of calling sql management command

[Andrii Kurinnyi]

Removed some whitespace and unnecessary empty set argument

[patchhammer]

t12308_4.diff fails to apply cleanly on to trunk

[Aymeric Augustin]

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.

[Aymeric Augustin]

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.

[Aymeric Augustin]

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 ManyToManyFields 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.

[Aymeric Augustin]

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.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

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.

[Aymeric Augustin]

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 :)

[Anssi Kääriäinen]

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? :)

[Aymeric Augustin]

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.

[Aymeric Augustin]

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.

[Aymeric Augustin]

In [16987]:

Fixed #12308 -- Added tablespace support to the PostgreSQL backend.

[Aymeric Augustin]

In [16988]:

Fixed a test failure introduced at r16987. Refs #12308.

[Aymeric Augustin]

In [16990]:

Modified the tablespaces tests so that they no longer rely on settings.DEFAULT_INDEX_TABLESPACE being empty. Refs #12308.