#12308: Adding tablespace support to postgres backends -------------------------------------+------------------------------------- Reporter: tclineks | Owner: aaugustin Type: New | Status: new feature | Component: Database layer Milestone: | (models, ORM) Version: SVN | Severity: Normal Resolution: | Keywords: Triage Stage: Accepted | Has patch: 1 Needs documentation: 0 | Needs tests: 0 Patch needs improvement: 1 | Easy pickings: 0 UI/UX: 0 | -------------------------------------+-------------------------------------
Comment (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 :) -- Ticket URL: <https://code.djangoproject.com/ticket/12308#comment:18> Django <https://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.