fix (sync-service): let Postgres handle user-provided root table name #1776
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KyleAMathews
reviewed
Oct 1, 2024
packages/sync-service/lib/electric/postgres/inspector/direct_inspector.ex
Outdated
Show resolved
Hide resolved
…on from table name.
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
KyleAMathews
approved these changes
Oct 3, 2024
There was a problem hiding this comment.
I gave a brief look through (gotta run to airport in a sec) and it looks great.
One thing we talked about yesterday also is including the sql statement in errors we send back — I'll make a separate issue for that for some follow-on work — #1790
| assert %{"root_table" => ["table not found"]} = Jason.decode!(conn.resp_body) | ||
| assert %{ | ||
| "root_table" => [ | ||
| ~s|Table "nonexistent" does not exist. If the table name contains capitals or special characters you must quote it.| |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #1764 according to the approach that was decided by @KyleAMathews, namely to adhere to Postgres' way of handling identifiers. Therefore, this PR modifies Electric such that it passes the user-provided root table to Postgres which then returns the actual relation (i.e. schema and table name). Which we then use throughout the rest of the code as before.
We extended the ETS inspector with a write-through cache that caches the relation. For any incoming request, it tries to fetch the relation for that table from the cache. If not found, it loads it from PG and stores it in the cache. Note that many table name spellings can refer to the same relation, e.g.
users,USERS,public.users, etc. all refer to the same relation{public, users}. For each different spelling, we will load the relation from PG and store it in the cache. Subsequent requests then load it from cache. We also cache the inverse (mapping relation to the different table names) in order to efficiently clean up.