Repository navigation
Conversation
03e0e63 to
9542a89
Compare
|
I think this feature will be great, but I wonder if we could remove the Or would that make the migration paths with JSON-based tables too difficult? In that case, maybe we can consider another change later (potentially breaking or opt-in) to remove |
|
I like that idea.
I have changed tables to no longer include a |
ac9f509 to
ffb6603
Compare
30a3119 to
98cebcf
Compare
09f191b to
f9719b6
Compare
0c6d667 to
41715bf
Compare
b967acf to
bb78750
Compare
There was a problem hiding this comment.
On a high level, I like the approach here, and it seems like a good start for opt-in functionality that users can start testing out.
Schema changes
I wonder if we should call powersync_trigger_resync() automatically when direct tables are modified? I believe that should cover most inconsistencies covered by schema changes, but I didn't check every path.
That could also potentially allow using more specific type affinities: If the affinity is changed, we can just re-sync. But in practice that could be tricky - we may need to re-create the table since we can't ALTER a column type. So not sure whether that's worth it.
As a side-note, a BLOB column on a normal table should have roughly the same behavior as an ANY column on a strict table. The ANY does make the intention more explicit though.
Specific issues
Some specific issues picked up by Codex below. Not all introduced directly by this PR, but the scope increases with direct tables. These are copied from Codex directly, but confirmed manually:
Syncing or migrating integers can lose precision
Direct tables now use PreparedPendingStatement, whose numeric binding tries as_f64() first—even for integers. For example, 9007199254740993 becomes 9007199254740992. Smaller integers also acquire SQLite’s REAL storage type. Bind integer representations before falling back to doubles. See direct-table sync integration (crates/core/src/sync/sync_local.rs:434) and numeric binding (crates/core/src/sync/sync_local.rs:578).
Rejected writes can still modify direct tables
The generated triggers run AFTER the write and use RAISE(FAIL) for insert-only violations and ID changes. SQLite preserves changes already made when FAIL fires. Consequently, UPDATE users SET id = ... can change the ID, report an error, and skip recording CRUD. Insert-only updates/deletes have the same problem. Use ABORT or validate in BEFORE triggers. See trigger generation (crates/core/src/schema/raw_table.rs:189) and ID validation (crates/core/src/utils/sql_buffer.rs:98).
| buffer.push_str(") ON CONFLICT (id) DO UPDATE SET "); | ||
| let mut do_update = buffer.comma_separated(); | ||
|
|
||
| // Generate an "x" = ? for all synced columns to update them without affecting local-only | ||
| // columns. | ||
| for (i, column) in self.column_names().enumerate() { | ||
| let entry = do_update.element(); | ||
| let _ = entry.identifier().write_str(column); | ||
| let _ = write!(entry, " = ?{}", i + data_start_index); | ||
| } |
There was a problem hiding this comment.
This generates an invalid statement if the table contains only an id.
| results.push(ExistingTable { | ||
| internal_name: internal_name.to_owned(), | ||
| name: internal_name.to_owned(), | ||
| local_only: sql.contains("local-only"), |
There was a problem hiding this comment.
This can also match a column called "local-only". Although that's unlikely with the dash in the name, maybe we can match a more specific comment?
|
|
||
| if let Some(ref schema) = self.direct { | ||
| buffer.push_str(&table_columns_to_json_object( | ||
| &self.internal_name, |
There was a problem hiding this comment.
The current implementation doesn't handle "special" table names that need to be quoted here, even though we do quote them in other places. An example is a table called order, which may break due to it being a SQL keyword.
This adds a new type of table next to the existing ones based on JSON data + views (easy to manage and migrate, slower to query) and raw tables (fast queries and full customizability at the expense of a higher setup complexity). Direct tables (name tbd, we can decide once we add SDK bindings) have the shape of raw tables with proper SQLite columns that can be queried without decoding JSON on reads.
A direct table has a shape like this:
The sync client treats this table like raw tables, automatically inferring statements to sync into it. There is no view to generate, but we generate triggers to record writes in
ps_crud.As an alternative to this table shape, I have considered keeping a
ps_data__-like table that just addsGENERATED ALWAYS AScolumns. This might seem simpler, but then you wouldn't be able to update these columns directly so we'd have to keep generating views andINSTEAD OFtriggers. The current pattern is closer to the shape one would expect SQLite tables to have.Direct tables are currently an opt-in, but we could make them the default in a future major release. An important difference in behavior is that JSON-based tables have
INSTEAD OFtriggers for tables thatCASTvalues to their expected type. Direct tables don't have triggers, so inserting into them gives you the usual SQLite behavior where epxressions have a type affinity, but no enforced type. It is thus possible to sync, insert and update "wrong" types into direct tables. I have adoptedSTRICTtables withANYcolumns for this reason: The type is dynamic either way, and this simplifies migrations because we don't have to care about changed column types.Limitations
A limitation is that direct tables don't support the
view_nameoption (because they're not based on views, this would always require an expensive migration to change table names). We can revisit this later, by creating a wrapping view around the table when theview_nameoption is given.Alternatively, we could introduce a way to set a custom target name to match
ps_oplogrows for direct tables, which roughly matches what we do for raw tables.Benchmarks
Despite the fact that we have to parse rows now, enabling direct tables seems to improve
sync_local_performance_test.dart. Using a full run with unique ids:1940ms Reads: 1291079 + 0 | Writes: 69031 + 0.1408ms (-27.4%) Reads: 1286404 (-3.6%) + 0 | Writes: 42228 + 0 (-38.8%). This is an in-memory test with an instrumented VFS so times don't mean much, but this needing fewer writes should translate to better sync performance.Since direct tables have the same structure as raw tables, they should result in similar performance benefits for queries (I did not benchmark direct tables for this specifically).
Migrations and tests
Raw tables already give us the infrastructure to sync into arbitrary tables, so most of this PR deals with migrations related to direct tables. The following things are considered and tested:
ps_untyped.AI use: Reviewed with Claude Code.