Skip to content

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.

@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.

3 participants