[roomserver]Store transaction ID and prevent reprocessing of events #446
Conversation
| auth_event_nids BIGINT[] NOT NULL | ||
| auth_event_nids BIGINT[] NOT NULL, | ||
| -- Apply uniqueness constraint on event_id and transaction_id | ||
| CONSTRAINT roomserver_event_id_transaction_id_unique UNIQUE (event_id, transaction_id) |
There was a problem hiding this comment.
This is saying that we want the tuple (event_id, txn_id) to be unique, which would allow us to have duplicated event_ids, which is not what we want.
| // Check if an transaction ID exists | ||
| // This is used to determine if the room event is processed/processing already | ||
| CheckTransactionID( | ||
| ctx context.Context, TransactionID string, |
There was a problem hiding this comment.
A transaction ID is only unique up to a user and device, so we'll need to also pass in the sender and device_id.
| event_id TEXT NOT NULL, | ||
| -- Transaction ID for this event | ||
| -- Required to ensure that an event is not processed again | ||
| transaction_id TEXT NOT NULL, |
There was a problem hiding this comment.
We'll also need to store at least the device_id and probably the sender (though the sender can be computed from the json)
| const checkExistsTransactionIDSQL = "" + | ||
| "SELECT EXISTS(" + | ||
| "SELECT 1 FROM roomserver_events WHERE transaction_id = $1" + | ||
| ")" |
There was a problem hiding this comment.
I'd probably remove the SELECT EXISTS (..) portion and simply count the number of rows returned. Another option is SELECT COUNT(*) FROM roomserver_events WHERE transaction_id = $1. In general its usually clearer to try and avoid using subqueries where possible.
|
Unfortunately, I forgot two things that makes things a bit more annoying:
The second point should be relatively easy to do. However the first point is more annoying, since the events table currently stores neither the sender user_id nor device ID. Given that, I think I've changed my mind and we should probably have it as a a separate table (like you originally suggested). |
|
Oh, and also: whenever you add new queries (especially when using new columns), its a good idea to stop and consider whether it needs a new index to be added. For example, currently in this PR we're adding a query to select based on |
APwhitehat
force-pushed
the
roomserver_txnID_idempotent
branch
2 times, most recently
from
6f4b3f5 to
ac6a910
Compare
| "INSERT INTO roomserver_transactions (transaction_id, device_id, user_id, event_id)" + | ||
| " VALUES ($1, $2, $3, $4)" + | ||
| " ON CONFLICT ON CONSTRAINT roomserver_transaction_unique" + | ||
| " DO NOTHING" |
There was a problem hiding this comment.
Why do we want to noop if the row already exists?
Reconfiguration of postgresql is required to test this.
Signed-off-by: Anant Prakash <[email protected]>