Add support for conflict_target: {:unsafe_fragment, _}

[ewildgoose]

Sqlite does support ON CONFLICT DO upsert feature, eg:
https://www.sqlite.org/lang_UPSERT.html

As part of this I synced a couple of function definitions to match current upstream postgrex as part of this. This is a bit stark as they use a different formatting option. Does this fit with your goals?

Note my goal here was to be able to do something like:

Repo.insert(
  %__MODULE__{
    device_id: device_id,
    sim_id: sim_id,
    device_type: device_type
  },
  on_conflict: {:replace_all_except, [:id]},
  conflict_target: {:unsafe_fragment, ~s<("device_id",coalesce("sim_id", ""))>},
  returning: true

Where the goal is to simulate a unique constraint on a column which accepts nulls (default for sqlite is to allow multiple "unique" rows in a nullable column)

[warmwaffles]

Did mix format make this get all aligned on the same column?

[ewildgoose]

For me with the default I think not... The lines I carefully copy/pasted from postgrex. No special reason other than I kind of consider it the main upstream implementation and I guess the closer this file is to the upstream the easier to merge in changes going forward?

https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/postgres/connection.ex#L176

I wonder if they use a custom .formatter file, didn't think to check? For me also, the formatter will add extra row spaces between each of these clauses, but their formatting has them all bunched together?

[warmwaffles]

This looks good to me. Are there tests we could add to the integration suite to ensure this is doing what we want it to do? Or rather are there already existing integration tests that we can enable to test against?

[warmwaffles]

I approved the test suite to run. @ewildgoose feel free to push changes and what not.

[ewildgoose]

OK, so this looks like I've introduced some bug, probably syncing with (what I'll call upstream)

Error is:
:erlang.iolist_to_binary([[], ["UPDATE ", [[34, "users", 34]], " AS ", [117 | "0"], " SET "], [[:name, " = ", 63]], [], []])

I assume it's the improper list [117 | "0"] which is breaking things?

I've copied a couple of what I thought were indifferent changes from "upstream" where they appear to do a list cons when returning a value, rather than returning the longer list. I would hazard a guess there is somewhere sending a value not wrapped in a list, which then gets turned into an improper list

I could remove some of the changes, but I wonder if you can guide on what you see as "upstream"? Is it work a slightly larger sync change to make our code look more substantially similar to postgrex? (ease of comparison/digression being the only motivation)

[warmwaffles]

Can you link me to the sections you copied upstream? I want to take a look at the blame and do some digging.

[ewildgoose]

Hi, see comment above. Main link is this:
https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/postgres/connection.ex#L176

I had to merge it because of the extra clause we have forbidding using named contraints.

I also added the error message at the base of the file.

Then I jumped to a wrong conclusion, which is why I synced "update_fields" as well.

The only useful functional line that I've added is line 667:

  defp conflict_target({:unsafe_fragment, fragment}),
    do: [fragment, ?\s]

The rest is just syncing with ecto_sql (which may or may not be desired?)
Edit: Sorry, I keep saying "postgrex", when I really mean ecto_sql/postgres

[warmwaffles]

Sorry, I keep saying "postgrex", when I really mean ecto_sql/postgres

Don't worry it confuses me as well.

[ewildgoose]

I presume it's this change causing the issue (or the update_fields() change, but can't see how that causes it?)

-    [
-      cte,
-      prefix,
-      fields,
-      join,
-      where,
-      returning(query, sources)
-    ]
+    [cte, prefix, fields, join, where | returning(query, sources)]