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

add signature of datastore_update_bsatn host call #2102

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jan 9, 2025

Description of Changes

Adds the ABI datastore_update_bsatn, defining the signature in bindings-sys and in the host.
However, the ABI is not used by the bindings crate (todo) and the InstanceEnv::update method added also is todo!().

This PR is opened so that we can discuss and decide on the ABI signature and semantics.

Some design choices made here:

  1. Sequences are generated for the row provided, matching the semantics of insert. This does have a minor adverse perf impact. If it turns that this is a problem, we can nix that. There are also avenues for optimizing sequence generation by e.g., storing a (ColList, Box<[SequenceId]>) directly in a Table to avoid going to the TableSchema.
  2. The search for the row-to-up-date is done after generating and writing back sequence values.
  3. Unique constraints that aren't the one in the index are checked. This is not really optional, as it would otherwise allow unique constraints to be violated.
  4. When a row wasn't found, an error is returned.

API and ABI breaking changes

None.

Expected complexity level and risk

3, its not used yet, but we are deciding on a signature and ABI call semantics in this PR.

Testing

Nothing to test yet. This has not been integrated yet.

@Centril Centril force-pushed the centril/update-abi-shell branch 2 times, most recently from 6a64505 to 9e87928 Compare January 9, 2025 18:36
@Centril Centril changed the title add signature of datastore_update_bsatn host call add signature of datastore_update_bsatn host call Jan 9, 2025
@Centril Centril marked this pull request as ready for review January 9, 2025 18:40
@Centril Centril requested review from coolreader18 and gefjon January 9, 2025 18:40
@Centril Centril force-pushed the centril/update-abi-shell branch from 8268c03 to de45917 Compare January 13, 2025 16:05
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Sequences are generated for the row provided, matching the semantics of insert.

I agree with this choice. I think we do not want to expose users to a way to accidentally skip auto-inc processing.

The search for the row-to-up-date is done after generating and writing back sequence values.

🤷 I don't think it really matters. AFAICT this is only observably different when updating by a #[unique] #[auto_inc] column and supplying 0 as that column's value in the new row, which is a nonsense thing to do.

Unique constraints that aren't the one in the index are checked. This is not really optional, as it would otherwise allow unique constraints to be violated.

I agree with this assessment.

crates/bindings-sys/src/lib.rs Outdated Show resolved Hide resolved
@Centril Centril enabled auto-merge January 13, 2025 16:31
@Centril Centril added this pull request to the merge queue Jan 13, 2025
Merged via the queue into master with commit 5ad2a2e Jan 13, 2025
9 checks passed
@Centril Centril deleted the centril/update-abi-shell branch January 13, 2025 18:20
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.

2 participants