Code

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#12308 closed New feature (fixed)

Adding tablespace support to postgres backends

Reported by: tclineks Owned by: aaugustin
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@…> 5 years ago.
t12308_1.diff (648 bytes) - added by andrewsk 4 years ago.
Patch for current trunk r13359
t12308_2.diff (3.3 KB) - added by andrewsk 4 years ago.
added tests
t12308_3.diff (3.5 KB) - added by andrewsk 4 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 andrewsk 4 years ago.
Removed some whitespace and unnecessary empty set argument
12308-with-mysql-support-dont-use.patch (26.8 KB) - added by aaugustin 3 years ago.
12308.patch (22.8 KB) - added by aaugustin 3 years ago.
12308.2.patch (22.8 KB) - added by aaugustin 3 years ago.
12308.3.patch (23.4 KB) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (33)

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

comment:1 Changed 5 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

#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 5 years ago by Travis Cline <travis.cline@…>

  • Resolution duplicate deleted
  • Status changed from closed to reopened

#6148 does not reference tablespaces.

comment:3 Changed 5 years ago by Alex

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 5 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 5 years ago by ramiro

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 4 years ago by russellm

  • Has patch set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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.

Changed 4 years ago by andrewsk

Patch for current trunk r13359

Changed 4 years ago by andrewsk

added tests

Changed 4 years ago by andrewsk

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

Changed 4 years ago by andrewsk

Removed some whitespace and unnecessary empty set argument

comment:7 Changed 4 years ago by andrewsk

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

comment:8 Changed 4 years ago by andrewsk

  • Cc prigun@… removed
  • Owner changed from nobody to andrewsk
  • Status changed from reopened to new

comment:9 Changed 4 years ago by andrewsk

  • Status changed from new to assigned

comment:10 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

t12308_4.diff fails to apply cleanly on to trunk

comment:12 Changed 3 years ago by aaugustin

  • 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 3 years ago by aaugustin

  • Owner changed from andrewsk to aaugustin
  • Status changed from assigned to 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 Changed 3 years ago by aaugustin

  • 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 3 years ago by aaugustin

Changed 3 years ago by aaugustin

comment:15 Changed 3 years ago by aaugustin

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 3 years ago by akaariai

  • 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 3 years ago by akaariai

  • 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 3 years ago by aaugustin

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 follow-up: Changed 3 years ago by akaariai

  • 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 3 years ago by aaugustin

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 3 years ago by aaugustin (previous) (diff)

Changed 3 years ago by aaugustin

Changed 3 years ago by aaugustin

comment:21 Changed 3 years ago by aaugustin

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 3 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

In [16987]:

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

comment:23 Changed 3 years ago by aaugustin

In [16988]:

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

comment:24 Changed 3 years ago by aaugustin

In [16990]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.