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

Let last_insert_id optional and flexible. #2393

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

langyo
Copy link
Contributor

@langyo langyo commented Oct 14, 2024

PR Info

I'm sorry for submitting such a PR so presumptuously. I found that the type of last_insert_id is flawed.

  • For Postgres, a valid last_insert_id will not be returned anyway, because Postgres may directly use a non-integer primary key.
  • The primary key specified by the table may not be able to auto-increment. For cases where auto-increment is not possible, we should not return a specific ID.
  • For proxy connection type databases, whether to return a valid last_insert_id is entirely up to the user. If the user cannot get a valid ID from the actual database transferred (for example, for databases such as surrealdb, their IDs are UUIDs rather than integers), it is unreasonable to return a 0 directly.

This change is likely to affect a considerable number of downstream libraries, so it is a breaking change. Whether to make such a change requires discussion and decision.

Breaking Changes

  • Let last_insert_id optional.
  • Let last_insert_id follows the primary key's type.
  • Allow initialize custom primary key on server.

I am not in a hurry to merge now. I am trying to make more radical changes to synchronize last_insert_id directly with the type corresponding to PrimaryKey instead of hard-coded u64 type. It may adapt to more complex table structures.

In addition to the fact that this field should be optional, I also consider the default generation of PrimaryKey. As mentioned in another discussion I cited, what we actually need is not necessarily a query logic that "may not return last_insert_id", but that we can generate the primary key by SeaORM instead of the database engine if necessary.

Combining these two requirements, we get a last_insert_id that can be generated in advance and returned normally without hard-coding it as u64. Not only meet the needs of primary keys that may not necessarily return u64 type, but also specify the "default value" of this primary key which is generated by us in advance rather than provided by the database engine itself.

Finally, let's review the scenarios where this is applicable. One type is to use such as UUID, ULID, GUID as the primary key of the database table. The other type is to use the creation time as the index of the database table. Both of these types belong to the categories that the database engine cannot guarantee to have their corresponding implementations. We should indirectly provide auxiliary implementations through ORM.

By the way, it stands to reason that we should also provide a shortcut to achieve the implementation of the key is_deleted and the key updated_at, although I think this may need to be discussed separately. They are used to implement soft deletion and optimistic locking respectively.

@langyo langyo force-pushed the upgrade_exec_result branch 3 times, most recently from 8e19e10 to 8ed9b89 Compare October 14, 2024 02:33
@langyo langyo marked this pull request as ready for review October 14, 2024 02:55
@langyo
Copy link
Contributor Author

langyo commented Oct 17, 2024

cc @billy1624 @tyt2y3
Please take a look at whether this modification is feasible if you have anytime..
Maybe it's still in time to modify it before it's commercially available on a large scale... Otherwise I wouldn't be in such a hurry.

@langyo langyo changed the title Let last_insert_id optional. Let last_insert_id optional and flexible. Oct 17, 2024
@langyo langyo marked this pull request as draft October 17, 2024 15:53
@langyo langyo marked this pull request as ready for review October 23, 2024 01:13
Temporarily circumvent the problem of not being able to correctly query the newly inserted elements caused by the upstream warehouse.
gluesql/gluesql#1579
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

Successfully merging this pull request may close these issues.

1 participant