Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

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

postgres_tablespace_support.diff (854 bytes) - added by Travis Cline <travis.cline@…> 7 years ago.
t12308_1.diff (648 bytes) - added by Andrii Kurinnyi 6 years ago.
Patch for current trunk r13359
t12308_2.diff (3.3 KB) - added by Andrii Kurinnyi 6 years ago.
added tests
t12308_3.diff (3.5 KB) - added by Andrii Kurinnyi 6 years ago.
Rewritten tests to use lower level connection.creation.sql_create_model api, instead of calling sql management command
t12308_4.diff (3.5 KB) - added by Andrii Kurinnyi 6 years ago.
Removed some whitespace and unnecessary empty set argument
12308-with-mysql-support-dont-use.patch (26.8 KB) - added by Aymeric Augustin 5 years ago.
12308.patch (22.8 KB) - added by Aymeric Augustin 5 years ago.
12308.2.patch (22.8 KB) - added by Aymeric Augustin 5 years ago.
12308.3.patch (23.4 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (33)

Changed 7 years ago by Travis Cline <travis.cline@…>

comment:1 Changed 7 years ago by Karen Tracey

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: duplicate
Status: newclosed

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

comment:2 Changed 7 years ago by Travis Cline <travis.cline@…>

Resolution: duplicate
Status: closedreopened

#6148 does not reference tablespaces.

comment:3 Changed 7 years ago by 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.

comment:4 Changed 7 years ago by Travis Cline <travis.cline@…>

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

comment:5 Changed 7 years ago by 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)?

comment:6 Changed 7 years ago by Russell Keith-Magee

Has patch: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

Changed 6 years ago by Andrii Kurinnyi

Attachment: t12308_1.diff added

Patch for current trunk r13359

Changed 6 years ago by Andrii Kurinnyi

Attachment: t12308_2.diff added

added tests

Changed 6 years ago by Andrii Kurinnyi

Attachment: t12308_3.diff added

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

Changed 6 years ago by Andrii Kurinnyi

Attachment: t12308_4.diff added

Removed some whitespace and unnecessary empty set argument

comment:7 Changed 6 years ago by Andrii Kurinnyi

Cc: prigun@… added
Needs tests: unset
Patch needs improvement: unset

comment:8 Changed 6 years ago by Andrii Kurinnyi

Cc: prigun@… removed
Owner: changed from nobody to Andrii Kurinnyi
Status: reopenednew

comment:9 Changed 6 years ago by Andrii Kurinnyi

Status: newassigned

comment:10 Changed 5 years ago by Matt McClanahan

Severity: Normal
Type: New feature

comment:11 Changed 5 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

t12308_4.diff fails to apply cleanly on to trunk

comment:12 Changed 5 years ago by Aymeric Augustin

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 Changed 5 years ago by Aymeric Augustin

Owner: changed from Andrii Kurinnyi to Aymeric Augustin
Status: assignednew

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 Changed 5 years ago by Aymeric Augustin

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

Changed 5 years ago by Aymeric Augustin

Changed 5 years ago by Aymeric Augustin

Attachment: 12308.patch added

comment:15 Changed 5 years ago by 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.

comment:16 Changed 5 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… 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 Changed 5 years ago by Anssi Kääriäinen

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 Changed 5 years ago by 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 :)

comment:19 Changed 5 years ago by Anssi Kääriäinen

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 in reply to:  19 Changed 5 years ago by 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.

Last edited 5 years ago by Aymeric Augustin (previous) (diff)

Changed 5 years ago by Aymeric Augustin

Attachment: 12308.2.patch added

Changed 5 years ago by Aymeric Augustin

Attachment: 12308.3.patch added

comment:21 Changed 5 years ago by 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.

comment:22 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [16987]:

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

comment:23 Changed 5 years ago by Aymeric Augustin

In [16988]:

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

comment:24 Changed 5 years ago by Aymeric Augustin

In [16990]:

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

Note: See TracTickets for help on using tickets.
Back to Top