Fix 3319 - add unique constraint on identity table

[mmzeeman]

Description

Fix #3319

Add a unique constraint on rsc_id, type and key on the identity table. This ensures that only one entry is added as intended.

Added test cases to check this behavior.

Added an update routine to remove duplicate entries, and add the constraint to an existing database.

Checklist

• documentation updated
• tests added
• no BC breaks

[mworrell]

Shouldn't we check here for an error like: #error{ codename = unique_violation } ?
And if so, check if it is because of the (Id, Type, Key) constraint?

[mmzeeman]

I tried that. When you catch the unique_violoation and retry the transaction it will also fail, because there is another concurrent transaction which is touching the same row. So maybe the best thing is to just return the error.

[mworrell]

Ah, the locking problem where we can't lock the key for update yet, because it doesn't exist yet.

Either a table lock, backing off for a couple of msec, or a transactional lock is needed in these situations.
I will check the options. In the paysub module I am now using a retry and a "select ... for update", which seems to work fine for now, but I see the problem with the race condition, especially if the insert takes longer.

[mworrell]

Forgot, since PostgreSQL 9.5 the "INSERT ... ON CONFLICT .." exists. Maybe this is a good moment to start using it in the core:

https://www.postgresql.org/docs/9.5/sql-insert.html

As the oldest supported PostgreSQL version is now 11 we might want to up our minimum PostgreSQL version to 9.5?

[mmzeeman]

Ah... we could extend z_db:insert/3 with some options...

Something like this?

z_db:insert(identity, #{ rsc_id => ..., ...}, 
            #{ on_conflict => <<"ON CONSTAINT(identity_rsc_id_type_key_unique)">>,
               action => <<"DO NOTHING">> },
            Context),

[mmzeeman]

Merging would only work correctly when the only properties are real column, and not something which ends up in a props column.

[mworrell]

I was thinking in two different directions. One is an option to handle the conflict with an update {on_conflict, update}. Second option is to add an extra function upsert, next to insert and update.

[mmzeeman]

Sounds logical... At the moment this would be impossible for erlang props columns of course. Maybe something for 1.1? We could make something to merge json props then.

[mworrell]

Yes, and in the meantime we can just have a "last one wins" strategy.

[mmzeeman]

This is an exceptional situation somehow caused by the way our app passes the apple device token, with QoS 1, to the server (In order to support apple push notifications). Somehow two publishes happen at almost the same time. I'm not sure how yet. Usually the identity is already there.

[mworrell]

@mmzeeman added a "for update" to the select

[mworrell]

@mmzeeman Shall we merge this as-is?