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.

Sign up for GitHub

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

Jump to bottom

feat: support AUTO_ID_CACHE #56

Merged
merged 4 commits into from
Oct 19, 2023
Merged

feat: support AUTO_ID_CACHE #56

merged 4 commits into from
Oct 19, 2023

Conversation

wd0517
Copy link
Collaborator

@wd0517 wd0517 commented Oct 13, 2023 •
edited
Loading

Refer to #52

This PR plans to support AUTO_ID_CACHE in Django by declaring it in the model's meta option.

Note: tidb_auto_id_cache only affects table creation.

For example:

class Test(models.Model):
    a = models.TextField()

    class Meta:
        tidb_auto_id_cache = 1

This PR also removed functions.py because we have already reverted DatabaseWrapper.vendor to mysql, so there is no need to patch the as_tidb function anymore.

@wd0517 wd0517 marked this pull request as draft October 13, 2023 14:17
@wd0517 wd0517 linked an issue Oct 16, 2023 that may be closed by this pull request
Support AUTO_ID_CACHE #52
Closed
@wd0517 wd0517 marked this pull request as ready for review October 16, 2023 10:18
dveeden
dveeden reviewed Oct 18, 2023
View reviewed changes
Copy link
Contributor

@dveeden dveeden left a comment

Choose a reason for hiding this comment

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

As AUTO_ID_CACHE=1 makes things more MySQL compatible... should we consider making that the default? As this isn't the default in TiDB I don't think we should, but maybe others have different opinions on this?

django_tidb/schema.py
sql, params = super().table_sql(model)
tidb_auto_id_cache = getattr(model._meta, "tidb_auto_id_cache", None)
if tidb_auto_id_cache is not None:
sql += " AUTO_ID_CACHE %s" % tidb_auto_id_cache
Copy link
Contributor

Choose a reason for hiding this comment

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

What if tidb_auto_id_cache isn't a number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The user will receive an error thrown by django-tidb or TiDB, Django does not provide a way to validate meta option, its built-in option can also result in error when got invalid value.

@zhangyangyu
Copy link
Member

Do we need the tidb prefix? I think it's TiDB unique and already under django-tidb scope?

@zhangyangyu
Copy link
Member

Seems after v6.4.0 make it default doesn't hurt anything.

@wd0517
Copy link
Collaborator Author

wd0517 commented Oct 18, 2023

Do we need the tidb prefix? I think it's TiDB unique and already under django-tidb scope?

I think has tidb prefix can help application developers understand that this attribute belongs to tidb.

@wd0517
Copy link
Collaborator Author

wd0517 commented Oct 18, 2023

I have added a TIDB_DEFAULT_AUTO_ID_CACHE = 1 which can be modified in settings.py.

@wd0517
Copy link
Collaborator Author

wd0517 commented Oct 18, 2023

I revert the above change, because AUTO_ID_CACHE=1 have bad performance before TiDB v6.4.0, if we set a default auto_id_cache in django-tidb, it will make things become complex.

@wd0517 wd0517 merged commit 1ad2675 into pingcap:main Oct 19, 2023
10 checks passed
@wd0517 wd0517 deleted the feat-auto-id-cache branch October 19, 2023 02:22
wd0517 added a commit that referenced this pull request Oct 19, 2023
@wd0517
feat: support AUTO_ID_CACHE (#56)
1aad5e2
wd0517 added a commit that referenced this pull request Oct 19, 2023
@wd0517
feat: support AUTO_ID_CACHE (#56)
dc382d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dveeden dveeden dveeden left review comments

@zhangyangyu zhangyangyu zhangyangyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support AUTO_ID_CACHE
3 participants
@wd0517 @zhangyangyu @dveeden