WIP: Unforced AutoFields to be primary keys

[claudep]

[claudep]

Is the Windows build currently broken?

[smithdc1]

It is. I can reproduce failures locally with a fresh environment. I'll try and investigate.

[nessita]

Is the Windows build currently broken?

Most test runs are broken, apparently due to a bug in python 3.11.4

[nessita]

@claudep Is there any ticket or conversation about this work that could provide me with some context for doing the review? Thanks!

[claudep]

I guess I should add WIP prefix. Here's the related forum thread: https://forum.djangoproject.com/t/rationale-for-unique-autofields/23122

[shangxiao]

Hey Claude, thanks for getting something started on this.

Just a couple of notes to add to the topic:

• I got the impression from the forum thread & previous discussions that separating "auto fields" from "is a primary key" was part of the intention 🤔 I'd also like to see an auto field that isn't a primary key by default.
• I'd personally like to see auto fields get a start value. This is probably a separate feature though.

[claudep]

I'd also like to see an auto field that isn't a primary key by default.

Could this be a follow-up work or would you like to see that also in this patch?

[claudep]

@felixxm, @nessita, @charettes, what do you think this proposal still need before being accepted?

[claudep]

Basically, I think that that creating a custom field in contrib just because we are making too strict controls in core looks weird to me. But I'll try to rework this patch, as with the useful tip you gave, it might be less invasive than currently.

[charettes]

@claudep you might also be interested in https://github.com/django/django/commit/f333e3513e8bdf5ffeb6eeb63021c230082e6f95#diff-1c8b882c73bfda668d5451d4578c97191b0ebc0f088d0c0ba2296ab89b428c44R1053 which just landed through #16860 (ticket-16860).

AutoField on Postgres is backend by a id INT GENERATED ALWAYS AS IDENTITY so maybe the following would work

auto2 = GeneratedField(
    RawSQL("IDENTITY"), db_persist=True, output_field=IntegerField()
)

[nessita]

@felixxm, @nessita, @charettes, what do you think this proposal still need before being accepted?

I'm sorry I haven't provided feedback yet, is just that we are preparing for the 5.0 feature freeze next Mon and I haven't got time to review this in detail. From the latest comments, it feels like this may need another pass to make it a PG specific field? (at least for now). Am I reading this right that this may be targeting now 5.1?

Thanks! Natalia.

[shangxiao]

Ah didn't realise the generated fields was ready for 5.0 ❤️

It sounds like it should just work™️ and would not only allow multiple "auto fields" but will also allow me to specify options for the sequence like a start value, increment, etc.

[charettes]

I tested out the above and it unfortunately doesn't work out of the box because IDENTITY must replace the STORED | VIRTUAL suffix as well as avoid double parentheses wrapping.

In order to make it work with GeneratedFields we'd have to introduce a new Identity expression that gets special cased by the schema alteration framework to omit the (...) AS STORED | VIRTUAL.

Makes me wonder if the interface around db_persist should have been replaced by a Stored and Virtual expression instead to make Identity a first class citizen.

e.g.

class GeneratedExpression(Expression):
   pass

class Identity(GeneratedExpression):
    template = "IDENTITY %(opts)s"

    def __init__(self, sequence_opts, output_field=None):
         ....


class Stored(GeneratedExpression):
   template =  "(%(expressions)s) AS STORED"

GeneratedField(F("foo") + F("bar"))  # Equivalent of GeneratedField(F("foo") + F("bar"), db_persist=None)
GeneratedField(Stored(F("foo") + F("bar")))
GeneratedField(Virtual(F("foo") + F("bar")))
GeneratedField(Identity())

[claudep]

Promising... I guess it is still time to adapt the new functionality for such use cases before 5.0, right?

[shangxiao]

@LilyFoote no "problem" per se more just lamenting the fact that we apparently can't declare identity columns with it ^ 😭😂

It seemed so fortuitously convenient too.

[felixxm]

Makes me wonder if the interface around db_persist should have been replaced by a Stored and Virtual expression instead to make Identity a first class citizen.

Personally, I don't think it's worth complicating and stretching the current API where it should be able to implement it via something like:

class IdentityField(Field):
    db_returning = True

    def db_type(self, connection):
        return "INT GENERATED ALWAYS AS IDENTITY"

[shangxiao]

Personally, I don't think it's worth complicating and stretching the current API where it should be able to implement it via something like:

class IdentityField(Field):
    db_returning = True

    def db_type(self, connection):
        return "INT GENERATED ALWAYS AS IDENTITY"

@felixxm I don't think it's that simple though 🤔 Unless the behaviour of inserts & updates changed for ticket 31300, my understanding is that db_returning doesn't affect these, ie insert will still try and use the column for this field in the VALUES?

[shangxiao]

Hey folks, I have some good news (sort of 🙂):

I was able to use the newly added db_default to achieve something similar to GENERATED BY DEFAULT AS IDENTITY.

class Foo(Model):
    autoincrement = IntegerField(db_default=RawSQL('nextval(your_sequence)', …))

We still need to create the sequence but that's easy enough.

You can't do GENERATED ALWAYS AS IDENTITY though BY DEFAULT is good enough for me so I'm pleased because there are no more complex annoying workarounds for me to achieve this 👍