fix(electric): Sync rows that were in the table prior to electrification #1108
Conversation
There was a problem hiding this comment.
This probably works for the case you're testing, but I hit a related problem in my permissions e2e test.
In this test if I swap the order of the inserts and electrification I get an error later in the test when inserting into the project_memberships table caused by logical replication message with nil tags.
Details here: https://linear.app/electric-sql/issue/VAX-1750/inserting-into-a-table-fails-when-electrifying-a-table-with-existing
might be worth adding an e2e test that reproduces this situation
| end | ||
|
|
||
| defp unwrap_quotes("\"" <> _ = name), do: String.slice(name, 1..-2//1) |
There was a problem hiding this comment.
to be entirely over-defensive about it we should also be replacing any "" with ". Wouldn't String.trim_trailing\2be a little more explicit about what we're doing here too?
There was a problem hiding this comment.
Technically not equivalent given that a double-quote inside the quoted string will be represented by ""
There was a problem hiding this comment.
I started hating this "defensive" function definition the moment wrote it. I have since changed the approach to use unquoted {<schema>, <table>} tuples and only produce quoted identifiers immediately prior to using them in an SQL string.
|
@magnetised Good catch! We could try addressing this issue in a general way by creating a view over the shadow table that would return default values for all columns if a shadow row is missing. But then those defaults would need to work in different contexts where shadow rows are queried. Right now we just have these two places in the code where only @icehaunter thoughts? |
|
I think addressing it in those places is good enough |
alco
force-pushed
the
alco/vax-1768-initial-subscription-data
branch
from
10e51a9
to
50ad6e2
Compare
alco
force-pushed
the
alco/vax-1768-initial-subscription-data
branch
4 times, most recently
from
8ec5298
to
bbe03f8
Compare
| schema_version | ||
| |> SchemaVersion.table!(layer.target_table) | ||
| |> Schema.single_table_info(schema_version.schema) | ||
|
|
||
| columns = Enum.map_join(table.columns, ", ", &~s|this."#{&1.name}"|) |
There was a problem hiding this comment.
we should maybe document a restriction that electrified tables or columns in electrified tables can't contain " in their names... seems a) reasonable and b) that it would save us a lot of time defensively escaping quotes everywhere...
There was a problem hiding this comment.
I'm not sure about the reasonable part. People have all kinds of weird things in their schemas when those are generated by a tool instead of being written by hand.
We should pick some SQL fragment builder utility and use it pervasively. Switching to using Ecto's query API is one option. That will get easier to do if #1136 is accepted it merged (once it's done).
| ] | ||
| |> Enum.reject(&(is_nil(&1) or &1 == "")) | ||
| |> Enum.intersperse(" AND ") | ||
|
|
||
| where = if where_clauses != [], do: ["WHERE ", where_clauses], else: "" | ||
|
|
||
| user_table = {table_info.schema, table_info.name} |
There was a problem hiding this comment.
isn't this just layer.target_table?
When a table is electrified, its newly created shadow table is empty. If the table had any rows prior to electrification, they would remain invisible to Electric due to the JOIN between the user table and its shadow table performed when querying for initial subscription data.
alco
force-pushed
the
alco/vax-1768-initial-subscription-data
branch
from
7856b97
to
13e3b72
Compare
When a table is electrified, its newly created shadow table is empty. If the user table had any rows prior to electrification, they would remain invisible to Electric due to the JOIN between the user table and its shadow table performed when querying for initial subscription data.
Fixes #1106, VAX-1750.