Simple implementation of insert_or_update and try_insert_or_update

[rafaelGuerreiro]

Description of Changes

Implementing insert_or_update and try_insert_or_update following the issue guideline, this is a naive implementation that actually has to first find and then decide whether it's insert or update.

It's not clear to me how we want this feature to behave with auto_inc ids, if the id is 0, should we simply call insert?

I think I also need to provide the C# implementation, I'd like some guidance on that side.

This resolves #2612

API and ABI breaking changes

It introduces 2 public methods, which are marked with the unstable feature until we're set on the API.

Expected complexity level and risk

Complexity for this solution is 1, but if we want to have it performed in a single operation, it might be a 3, I'm not sure. I tried diving into the internals and wasn't too sure on the right approach.

Testing

I need guidance on testing.

[gefjon]

Thanks for this!

It's not clear to me how we want this feature to behave with auto_inc ids, if the id is 0, should we simply call insert?

It's not necessary to specifically handle the case where an auto-inc column is supplied and zero. In this case, the row cannot be already present, so the upsert operation will behave like an insert.

I think I also need to provide the C# implementation, I'd like some guidance on that side.

I would not block this PR on a C# implementation, but I would be very happy to have one included if you're able.

It introduces 2 public methods, which are marked with the unstable feature until we're set on the ABI.

This is an API. We use "ABI" (Application Binary Interface) to refer to our set of WASM imports and exports, and the memory representations they use to communicate row and column values between the host and the WASM code.

Complexity for this solution is 1, but if we want to have it performed in a single operation, it might be a 3, I'm not sure. I tried diving into the internals and wasn't too sure on the right approach.

We definitely prefer to implement this purely in WASM the way you've done, rather than implementing a new ABI / import function. We only define new ABIs when we really need to, either because profiling has shown a clear performance bottleneck, or because it's not possible to implement the functionality within WASM at all.

I need guidance on testing.

This PR is simple enough that I'd be willing to accept it with minimal testing. Add a call to the new functions somewhere in modules/module-test (and module-test-cs if you implement the C# version) so that we know the methods continue to exist and typecheck, and then do some amount of manual testing that the methods behave the way you expect. If you were interested in learning more about our testing setup, we could talk you through adding a new smoketest, but that'd be purely voluntary.

[rafaelGuerreiro]

@gefjon, thank you for reviewing and explaining all of that. I really appreciate it and I'll make all the changes.

I'll skip C# in this PR because I'm more comfortable with Rust (as in, if it compiles, it's probably correct). Also, since it's marked as unstable, we can raise another PR to make it available in C# once it becomes stable.

It's not necessary to specifically handle the case where an auto-inc column is supplied and zero. In this case, the row cannot be already present, so the upsert operation will behave like an insert.

Is it possible that a column becomes auto-inc after the row 0 was inserted? In this case, it would definitely break the auto-inc behaviour because a row with id 0 might already exist, and that same row is always updated in the current implementation.

[gefjon]

Is it possible that a column becomes auto-inc after the row 0 was inserted? In this case, it would definitely break the auto-inc behaviour because a row with id 0 might already exist, and that same row is always updated in the current implementation.

That's hypothetically possible, but it's pathological, and I don't think it needs or deserves special handling. update will also behave oddly if passed a zero value for an auto_inc column when a row with zero exists: it will delete the zero row, then compute the next sequence value and perform the auto-inc substitution in the new row.

[rafaelGuerreiro]

@gefjon, all of that makes sense to me. I've applied all the changes.

[gefjon]

Looks great, thanks!

[bfops]

It looks like the tests are failing due to build issues. I don't personally have context on how to fix these, or how to plumb the new methods through to the right places.

[rafaelGuerreiro]

It looks like the tests are failing due to build issues. I don't personally have context on how to fix these, or how to plumb the new methods through to the right places.

I think this is because I flagged these methods with the "unstable" flag. Let me see if I can get it to work on the test with the unstable over there as well.