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 #27452 -- Added SerialField and BigSerialField to contrib.postgres. #7525

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
10 participants
@codingjoe
Copy link
Contributor

commented Nov 6, 2016

@codingjoe codingjoe force-pushed the codingjoe:issues/27452 branch from 76a6a50 to 1e0986c Nov 6, 2016

Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved docs/ref/contrib/postgres/fields.txt Outdated
Show resolved Hide resolved tests/postgres_tests/test_serialfield.py Outdated
Show resolved Hide resolved tests/postgres_tests/test_serialfield.py Outdated
@wengole

This comment has been minimized.

Copy link

commented Nov 7, 2016

I hate to throw a spanner in the works, but this would be a lot simple if/when this is merged: #7515

Something along the lines of

class SerialField(model.Field):
    def __init__(self, *args, **kwargs):
        kwargs['readonly'] = True
        super(SerialField, self).__init__(*args, **kwargs)
   ...
@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

@wengole yes, that would be cool, but whoever comes last hast to refactor ;) It is really only a couple of lines in both cases.

@codingjoe codingjoe force-pushed the codingjoe:issues/27452 branch 2 times, most recently from f55aa79 to 7104fa1 Nov 7, 2016

Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved django/contrib/postgres/fields/serial.py
Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved django/contrib/postgres/fields/serial.py Outdated
Show resolved Hide resolved tests/postgres_tests/test_serialfield.py Outdated
Show resolved Hide resolved tests/postgres_tests/test_serialfield.py Outdated
Show resolved Hide resolved tests/postgres_tests/test_serialfield.py Outdated
@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2016

I changed a couple of things, I don't know tho, if this field is ready year. Currently every insert would result in an extra query, where the field should use RETURNING. I'm not too familiar with the db backends, to implement that properly.

@codingjoe codingjoe force-pushed the codingjoe:issues/27452 branch from edd4851 to 5d1af7a Nov 14, 2016

@ewjoachim

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

Well, I'm currently writing a DEP targetting precisely this problem. Serial is just a part of a more generic problem we need solved once and for all. Do you want to participate on the DEP ?

@ewjoachim

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

(For the record : #7515 (comment) )

@aouaki

This comment has been minimized.

Copy link

commented Mar 15, 2017

Hey guys,
Any news on this PR? We would love to use it in production and had to copy the implementation 🤓.

Thanks,
Arthur

@syurchenko

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

We are waiting for this PR to be merged. Very useful

@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2017

@syurchenko I will resume my work on this once #6385 is merged.
There is also a lager design issue, that is still to be resolved. Currently this field would execute an additional query for each insert. To be honest I know how to solve this in prostgres, I need to look into the ORM again, to see how I can make it work in Django.

@timgraham timgraham changed the title Added #27452 -- Added SerialField and BigSerialField to contrib.prostgres Fixed #27452 -- Added SerialField and BigSerialField to contrib.postgres. Jun 14, 2017

@aaronlelevier

This comment has been minimized.

Copy link

commented Sep 20, 2017

@codingjoe #6385 is merged. Can I give you a bump to resume work on this? I know I'd like to see this feature land! We use uuid's but need serial int ids for other uses, so this would be a great addition to the postgres contrib. Thanks.

@aaronlelevier

This comment has been minimized.

Copy link

commented Sep 21, 2017

@codingjoe I ended up going off of what you had done in order to build a custom SerialField for our project. It worked great. If you want help with this PR and just don't have time to do it now, I could take a crack at it. Let me know?

I did add the below method, or else it wasn't creating the field in the database. But other than that, I pretty much went off of what you had:

class SerialField(Field):
    # ...
    def db_type(self, connection):
        return 'serial'
@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@aronysidoro yes sure, be my guest to try. I do have more time now, since my other big PR is merged, but I have another one too.
The big problem here is to add the RETURNING statement, that you only have a single query per insert.

@aaronlelevier

This comment has been minimized.

Copy link

commented Sep 24, 2017

@codingjoe ok, I am going to try. So, it sounds like the big problem would be when using the SerialField with a bulk insert? I didn't test for that, so I will test for that as well. Thanks for pointing that out.

@codingjoe codingjoe closed this Sep 25, 2017

@aaronlelevier

This comment has been minimized.

Copy link

commented Dec 26, 2017

@codingjoe I have looked into this several times. I just want to follow up. So far, I haven't seen where a single RETURNING statement could be injected.

One place I looked at in particular was django.db.models.query.QuerySet.bulk_create here:

https://github.com/django/django/blob/master/django/db/models/query.py#L458

It looks like the code is looking for objects without a pk to bulk return the ids, but in the case of a Postgres SerialField, it could be any field, not just the pk field.

It seems like the code would need to use the NEXTVAL regardless. If that's true, then I wouldn't have much to add to your PR because it did everything necessary.

Any thoughts?

@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

Hi @aaronlelevier I know there isn't a simple way to inject this.
What I think needs to be done is:
Create a method to get a set of all fields that have a database default and should have a returning value, than rewrite those functions:

def return_insert_id(self):
return "RETURNING %s", ()

def fetch_returned_insert_ids(self, cursor):
"""
Given a cursor object that has just performed an INSERT...RETURNING
statement into a table that has an auto-incrementing ID, return the
list of newly created IDs.
"""
return [item[0] for item in cursor.fetchall()]

and this one:

def fetch_returned_insert_id(self, cursor):
"""
Given a cursor object that has just performed an INSERT...RETURNING
statement into a table that has an auto-incrementing ID, return the
newly created ID.
"""
return cursor.fetchone()[0]

They all need to be able to not only unpack a single but all values for the fields that are specified within your method.

btw, that method should return the fields in a deterministic order.

Does that help you?

@aaronlelevier

This comment has been minimized.

Copy link

commented Dec 30, 2017

@codingjoe thank you for your reply. I am going to look into your recommendations above.

Yes, it definitely helps. I was looking at Django 1.11, but I see the PR that you linked above is from Nov 29th, so there's new code then for some of the logic that SerialField could use that I hadn't seen.

Great, thank you.

@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

@codingjoe codingjoe force-pushed the codingjoe:issues/27452 branch 4 times, most recently from 8708d4b to 487285c Aug 23, 2018

@codingjoe codingjoe force-pushed the codingjoe:issues/27452 branch from 487285c to ddedea4 Aug 25, 2018

@codingjoe

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2018

Thanks for the review @charettes, but this will have to wait until after #9983 anyways. I will close the PR for now and open it, once #9983 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.