Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #20960 - moved DEFAULT_TABLESPACE to DATABASES #17902

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

bcail
Copy link
Contributor

@bcail bcail commented Feb 23, 2024

No description provided.

def test_default_index_tablespace(self):
index_tablespace = "default_index_tablespace"
databases = copy.deepcopy(settings.DATABASES)
databases["default"]["OPTIONS"]["DEFAULT_INDEX_TABLESPACE"] = index_tablespace
Copy link
Member

@charettes charettes Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong here but I think these options should live at the same level as OPTIONS and not be nested within OPTIONS?

In other words, I think they should be defined as

DATABASES = {
    "default": {
        "ENGINE": "...",
        ...,
        "DEFAULT_TABLESPACE": "..."
        "DEFAULT_INDEX_TABLESPACE": "...",
     }
}

As OPTIONS is meant to be per-backend flags that are usually passed directly to the underlying client bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @charettes. The ticket comment seems to suggest they should be inside "OPTIONS", but I'm fine with whatever. @aaugustin do you have any comments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I haven't been involved in developing Django for several years now and I'm unable to help there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, thanks.

I'll update the code to move these options up to the same level as OPTIONS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charettes @felixxm @shangxiao I've updated the code to move those options up.

@bcail bcail force-pushed the issue-20960-move-default-tablespace-settings branch from 87672cb to cfc948e Compare February 27, 2024 20:58
Comment on lines +279 to +285
db_tablespace = None
if settings.DATABASES[self.connection.alias].get("DEFAULT_TABLESPACE"):
db_tablespace = settings.DATABASES[self.connection.alias][
"DEFAULT_TABLESPACE"
]
elif model._meta.db_tablespace:
db_tablespace = model._meta.db_tablespace

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way the DEFAULT_TABLESPACE has a higher priority than the model's Meta tablespace.

I think it's because of the DEFAULT_TABLESPACE assignment in options, but it might lead to unexpected behaviour if someone is not using the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants