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

Composite primary keys #820

Open
ooliver1 opened this issue May 4, 2023 · 11 comments
Open

Composite primary keys #820

ooliver1 opened this issue May 4, 2023 · 11 comments

Comments

@ooliver1
Copy link

ooliver1 commented May 4, 2023

Setting primary_key for multiple columns only looks at the last-most column.

@dantownsend
Copy link
Member

dantownsend commented May 5, 2023

You're right - we don't support this currently. Hopefully we can add this soon 👍

@ooliver1
Copy link
Author

ooliver1 commented May 5, 2023

I would be more than willing to help contribute to that! It is currently the only thing I have found stopping me from using this (over ormar + alembic, ormar doesnt support composite PK either but it can be tricked into allowing it).

@sinisaos
Copy link
Member

sinisaos commented May 6, 2023

@dantownsend This is just my opinion and I don't think the implementation of composite primary keys will be easy at all because Piccolo depends on one primary key per table (often generated by Piccolo itself) and all joins in Piccolo apps will have to change. If I understand correctly, a composite primary keys is mainly used to uniquely identify each row in a table. Can this be achieved with unique constraints (UniqueTogether when implemented)? Django ORM does not support composite PK (nor do some other ORMs). Migrations are also problematic with composite PK. I found an interesting article about this problem and alternatives from Deepkit ORM. I hope that makes sense.

@dantownsend
Copy link
Member

@sinisaos Thanks for that article. I do agree that it's tricky. I've been thinking about how it could potentially be done.

Defining the composite primary key

Firstly, there would be two options for defining the composite primary key. Either this:

class Manager(Table):
    first_name = Varchar(primary_key=True)
    last_name = Varchar(primary_key=True)
    age = Integer()

Or this (which is probably better, as it's easier for us to detect that it's deliberate, and someone didn't accidentally mark multiple columns as primary keys):

class Manager(Table):
    first_name = Varchar()
    last_name = Varchar()
    age = Integer()

    pk = PrimaryKey(first_name, last_name)

Defining the composite foreign key

We also need a way of defining foreign keys which reference the composite primary key. I think the best way to achieve this is:

class Band(Table):
    name = Varchar()
    manager_first_name = Varchar()
    manager_last_name = Varchar()

    manager_fk = CompositeForeignKey({
        manager_first_name: Manager.first_name,
        manager_last_name: Manager.last_name
    })

We need something like CompositeForeignKey because Piccolo lets you traverse foreign keys. So we can do:

# Lets get the band's name, and their manager's age:
await Band.select(Band.name, Band.manager_fk.age.as_alias('manager_age'))

Other considerations

Migrations

Getting it to work with migrations might be tricky ... but probably doable.

Piccolo admin

We would need some kind of special widget in Piccolo Admin. Currently, for foreign keys, when you click on the field it opens the foreign key selector widget:

Screenshot 2023-05-06 at 22 41 26

But with composite primary keys / foreign keys, we would need something different, where the foreign key selector populates multiple columns.

OR, we just treat them as normal fields, and don't provide the foreign key selector widget. The user just has to enter the correct values manually.

Final thoughts ...

It's definitely a fair chunk of work, and poses some interesting challenges. @sinisaos and @ooliver1 What do you think of what I've outlined here?

The easiest work around is to have a surrogate primary key (a UUID or something), and then a composite unique constraint on the columns instead. This is something we're actively working on, and might be a better starting point.

@ooliver1
Copy link
Author

ooliver1 commented May 6, 2023

I appreciate the big thought put into this.

As for that article, the few reasons I have needed composite PKs (and I get why they aren't a good idea most of the time) is when an object has multiple fields that identify it, and that's it. The only way to have one key would be an amalgamation of those fields in one key (1_abc_true for example), so as to not have unnecessary storage usage, a composite PK does better.

A separate attribute for primary keys would be good, for the same reason you mentioned.
Migrations should be fine, since you aren't forced into using a composite primary key, and the documentation can simply encourage different design patterns if possible.

@sinisaos
Copy link
Member

sinisaos commented May 7, 2023

Thanks for detailing how the api for composite primary key should look like.

class Manager(Table):
    first_name = Varchar()
    last_name = Varchar()
    age = Integer()

    pk = PrimaryKey(first_name, last_name)

I also agree that would be a better option.

Piccolo admin

But with composite primary keys / foreign keys, we would need something different, where the foreign key selector populates multiple columns.

OR, we just treat them as normal fields, and don't provide the foreign key selector widget. The user just has to enter the correct values manually.

I'm not sure, but I think that forms will also be a problem, because we don't expose the primary key in forms (autogenerated) and I don't know how it should look for composite primary keys.

Final thoughts ...

The easiest work around is to have a surrogate primary key (a UUID or something), and then a composite unique constraint on the columns instead. This is something we're actively working on, and might be a better starting point.

I agree, and I also think it's a valid and much easier alternative that we can start using immediately after implementing unique constraints (UniqueTogheter).

@dantownsend
Copy link
Member

Some other thoughts ...

With Piccolo Admin we identify a row by it's primary key in the URL. For example /movie/1/. With composite primary key we would have to concatenate the values or something. /movie/name-year/.

Postgres supports generated columns. I've never actually tried this, but it might be possible to use the generated column as the primary key (with the value being a concatenation of other columns). In Postgres though, generated columns still use up storage, so might not make sense.

The plan of action seems like:

  1. Lets get composite unique constraints finished asap, as it's a good solution if a surrogate primary key is acceptable.
  2. We can add CompositeForeignKey and CompositePrimaryKey classes later on, but without Piccolo Admin support initially.
  3. Add Piccolo Admin support.

@satyamjay-iitd
Copy link

Since composite pks are not supported as of now, what I have done is:-
Wrote migration script with raw sql to drop the primary key constraint on id, and add my own composite pk constraint.
I dont know if this is a good idea.

@dantownsend
Copy link
Member

@satyamjay-iitd yeah, that's the best approach for now 👍

@MoSheikh
Copy link

Hi, is this still on the roadmap @dantownsend? Thanks!

@sinisaos
Copy link
Member

@MoSheikh I don't know if this is still on Daniel roadmap, but with this PR it will be possible to use a surrogate primary key as described in previous comments (similar to how Django uses it). Composite PK and Surrogate PK are valid options, but I think that surrogate PK with composite unique constraint is a less complicated option that is fully valid and will be able to be used as soon as PR is merged.

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

No branches or pull requests

5 participants