volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.66k stars 539 forks source link

Upserts with partial indexes #856

Open tonyhb opened 3 years ago

tonyhb commented 3 years ago

Allow upserting using partial indexes. Right now, one must supply column names for upserts. If an application has a partial index (ie. in the case of null values), upserts fail.

Consider the following unique index:

CREATE UNIQUE INDEX contacts_segments_uniqueness_live ON contacts_segments (contact_id, segment_id) WHERE deleted_at IS NULL

We ensure that a contact can only be in a single segment once, and that deleted at IS NULL for this row. This enforces one "live" (not soft deleted) row. To use this, we must supply a suffix to the upsert:

ON CONFLICT ("contact_id", "segment_id") WHERE "deleted_at" IS NULL DO UPDATE...

See: https://dba.stackexchange.com/questions/151431/postgresql-upsert-issue-with-null-values

We cant seem to currently do this.

What version of SQLBoiler are you using (sqlboiler --version)?

4.2.0

What is your database and version (eg. Postgresql 10)

Postgresql 12

tonyhb commented 3 years ago

Note: you can get around this with ON CONFLICT DO NOTHING and using nil as the upsert columns as long as postgres can automatically figure out the right unique index to use.

It's low priority, but it would still be nice to specify the full columns and optional constraint for the index - or even index name - if that's valid.

aarondl commented 3 years ago

Well given there's a workaround I probably won't be touching this. Super happy to accept a PR for it though, would be nice to clean it up.

tonyhb commented 3 years ago

Noted! Was filing an issue for awareness. Likely won't have time this year, but maybe I can help out with a PR at some point :)

tonyhb commented 3 years ago

Bitten by this again haha. Im wondering if we can add a where constraint to upsert in the form of a new method for backwards compatibility:

user.UpsertWhere(ctx, db, true, conflictCols, boil.Infer(), boil.Infer(), user.Where(user.DeletedAt.IsNull())

Or some other order of arguments :)

This would:

  1. Fix this problem entirely, because we can use the correct unique constraint with the where specification
  2. Maintain backwards compatibility so that we don't break people's code, and can release with a minor bump.

@aarondl if you're open to it I can work on a PR?

aarondl commented 3 years ago

@tonyhb I feel like this isn't common enough to warrant an additional method bloat for every model for every postgres user. WDYT?

Is there a different approach we could use that's more automatic inside the existing code?

glenjamin commented 3 years ago

If .Upsert() accepted ...QueryMods (or similar for just Where clauses?) then I think it wouldn't be a breaking API change?

I've just run into a similar but distinct case where I want to set the WHERE clause of the DO UPDATE, to check that the non-indexed fields are compatible or not (and then I need to be able to tell whether the update was applied).

My current workaround is to use DO NOTHING and then do another query in the same transaction.

tonyhb commented 3 years ago

This one comes back to bite every few months. Agree that adding a variadic QueryMods would be a good idea.

aarondl commented 2 years ago

Fine with adding a second Upsert method with the idea of providing the where clause. We shouldn't touch the current one because it's fine for most use cases and that function signature will only cause confusion:

yourtallness commented 2 years ago

Thus, so far no way to specify conflict conditions other than with columns, right?

I have a null-able foreign key which I'd like to be able to COALESCE to make unique when checking for a conflict while upserting.

aarondl commented 2 years ago

@yourtallness Correct. In this scenario I'd say it's best to use a raw query.