Add postgres interpreter for UserStore#4951
Conversation
zebot
added
the
ok-to-test
akshaymankar
force-pushed
the
user-pg-migration
branch
8 times, most recently
from
7735235 to
edefb6e
Compare
akshaymankar
force-pushed
the
user-pg-migration
branch
3 times, most recently
from
58edc5f to
272456f
Compare
akshaymankar
force-pushed
the
user-pg-migration
branch
8 times, most recently
from
b5cd210 to
68b2d01
Compare
akshaymankar
force-pushed
the
user-pg-migration
branch
9 times, most recently
from
68ef4d0 to
b2719f5
Compare
This makes postgres behave differently from Cassandra, but this is more correct.
… users These actions shouldn't work for deleted users.
Co-authored-by: Sven Tennie <sven.tennie@gmail.com>
akshaymankar
force-pushed
the
user-pg-migration
branch
from
e4edb26 to
389ae24
Compare
| -- If a user is found in deletedUsers and normal users, prefer the deleted | ||
| -- user. |
There was a problem hiding this comment.
Isn't this a data inconsistency that should be logged?
There was a problem hiding this comment.
Good idea, logging this at warn level now.
| lookupHandleImpl :: (PGConstraints r) => Handle -> Sem r (Maybe UserId) | ||
| lookupHandleImpl h = runStatement h selectUserIdByHandleStatement | ||
|
|
||
| selectUserIdByHandleStatement :: Hasql.Statement Handle (Maybe UserId) |
There was a problem hiding this comment.
You may reduce the scope of this. Putting helper functions in where blocks seems to be your pattern 😄
| @@ -0,0 +1,64 @@ | |||
| CREATE TABLE wire_user ( | |||
There was a problem hiding this comment.
You're using the name wire_user instead of user by intention, right? Probably to avoid future name clashes?
There was a problem hiding this comment.
In postgresql user is a keyword, and so needs to be quoted. This makes writing queries very error prone. So I decided to use a different name.
| select :: Hasql.Statement (UserId) (Maybe (Maybe Password, Maybe AccountStatus)) | ||
| select = | ||
| dimapPG | ||
| [maybeStatement|SELECT password :: bytea?, account_status :: integer? FROM wire_user WHERE id = $1 :: uuid|] |
There was a problem hiding this comment.
In wire_user the password is defined as password text. I hope this won't cause troubles when read with bytea?
There was a problem hiding this comment.
Whoops, its a mistake. Fixed now.
| language = COALESCE($6 :: text?, language), | ||
| country = COALESCE($7 :: text?, country), |
There was a problem hiding this comment.
Is this equivalent to the Cassandra behaviour?
userLocaleUpdate :: PrepQuery W (Language, Maybe Country, UserId) ()
userLocaleUpdate = "UPDATE user SET language = ?, country = ? WHERE id = ?"
In case of a pre-existing value for country, don't we keep it here? E.g. what will happen if I switch from en_EN to de?
There was a problem hiding this comment.
This is a very good catch! I'll add a test first ❤️
| WHERE id = $1 :: uuid | ||
| |] | ||
|
|
||
| getIndexUsersPaginatedImpl :: (PGConstraints r) => Int32 -> Maybe UserId -> Sem r (PageWithState UserId IndexUser) |
There was a problem hiding this comment.
If I'm getting this correctly, this function is also used to finally create tombstones in ElasticSearch (finally in indexUserToDoc).
But, we do not query deleted_user here. Is this correct?
| Transaction.statement userRow insertUser | ||
| Transaction.statement new.id deleteAssetsStatement | ||
| Transaction.statement (mkAssetRows new.id new.assets) insertAssetsStatement | ||
| for_ mbConv $ \(convId, mTeamId) -> do |
There was a problem hiding this comment.
createUserImpl of Cassandra also has the Maybe new.service in the equation. Should we adjust this guard?
| [resultlessStatement|UPDATE wire_user SET searchable = $2 :: boolean WHERE id = $1 :: uuid|] | ||
|
|
||
| deleteServiceUserImpl :: (PGConstraints r) => ProviderId -> ServiceId -> BotId -> Sem r () | ||
| deleteServiceUserImpl _ _ bid = |
There was a problem hiding this comment.
These unused parameters may deserve a FUTUREWORK: for the time after Cassandra migration.
supersven
left a comment
supersven
left a comment
There was a problem hiding this comment.
This is looking mostly looking great, however in my opinion:
- The questions regarding potentially slightly different behaviour of Postgresql vs. Cassandra queries should be sorted out.
- It would probably be good to have a third pair of eyeballs check the queries (comparing them to the Cassandra implementation).
As I'll be on vacation, please feel free to resolve my comments if you think they've been addressed.
… and deleted_user tables
…cases for locale update The test are failing for Postgres due to a bug.
Override country with `NULL` when it is not provided
3 participants
https://wearezeta.atlassian.net/browse/WPB-22747
Checklist
changelog.d