Race condition in Table.update_or_insert()

[nextghost]

Table.update_or_insert() and Table.validate_and_update_or_insert() should use _for_update=True when checking whether the record already exists. Otherwise there is a risk that the record will be deleted or (if it does not exist) inserted by a concurrent thread, leading to either database error or inconsistent data.

Adapters which don't support for_update locking should just silently ignore the parameter instead of raising exception (or at most log a warning).

[niphlod]

All it's done within a single transaction. No way it's going to lead to inconsistencies. For_update just locks the record for everyone else, which is not the common scenario. Il 05 gen 2017 16:35, "nextghost" <notifications@github.com> ha scritto: Table.update_or_insert() and Table.validate_and_update_or_insert() should use _for_update=True when checking whether the record already exists. Otherwise there is a risk that the record will be deleted or (if it does not exist) inserted by a concurrent thread, leading to either database error or inconsistent data. Adapters which don't support for_update locking should just silently ignore the parameter instead of raising exception (or at most log a warning). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#439>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHdB2z4RPNrmAtTtYdzkoxgS6V5y4Z6ks5rPQ2kgaJpZM4Lbzs_> .

[gi0baro]

@nextghost I agree with @niphlod, this should not be considered an issue.

[nextghost]

@niphlod What? That's not how transactions work. Unless you set the transaction level to "serializable" (the most restrictive), transactions don't protect you from read-write race conditions in any way. Otherwise they provide only safe error recovery, protection from write-write race conditions and maybe a little bit of read consistency for SELECT. You need to use for_update locks to ensure read-write consistency. That's why the damn lock exists in the first place.

Consider this example:

db.define_table('assets',
    Field('ownerref', type='reference owner'),
    Field('item', type='string'))
db.assets.insert(ownerref=1, item='chair')

Thread A: record = db.assets(ownerref=1, item='chair')
Thread B: db((db.assets.ownerref == 1) & (db.assets.item == 'chair')).update(ownerref=2)
Thread B: db.commit()
Thread A: record.update_record(item='dog')
Thread A: db.commit()

Now person 2 owns a dog, not a chair which he was supposed to get. Thread A is what Table.update_or_insert() currently does if you provide custom key.

[gi0baro]

@nextghost My considerations:

1. Let's not mix atomic updates with non-atomic ones. db(somequery).update(values) and record.update_record(values) are totally different operations; for instance if you have to update a bank account you should write

db(db.accounts.id == someid).update(amount=db.accounts.amount - transaction_value)

because this

account = db.accounts(someid)
account.update_record(amount=account.amount-transaction_value)

won't protect you in any case, even with a record lock, from updating a value which is not correct.

2. I understand the problem on update_or_insert but I think locking is not the answer. The risk is to introduce deadlocks and it's quite hard to guess the side effects. Moreover, every database use its own way to handle this, for example Postgres 9.5 introduced the UPSERT condition. Before that you could use IF conditions in postgres, but in this case everyone suggest to lock the entire tables to avoid race conditions, which feels like not a solution under my pov.

3. I agree to find a solution for this, but I think the best option is to rewrite the update_or_insert method differently, to try making it more atomic, and be honest with developers about the consistency of that operation. I'm for the thing "if you want to lock something, you write it on your own because you know what you're doing".

@niphlod comments?

[gi0baro]

@nextghost @niphlod a way could be rewrite the update_or_insert method to actually behave like an update or insert, rather than select-and-update or insert, like:

now:
- select
- if exists then update (non atomic)
- else insert

better flow:
- update (atomic)
- if not updated then insert

[nextghost]

@gi0baro @niphlod

1. I am aware of the difference but it is irrelevant for this issue. My example above will produce the exact same result no matter whether Thread B uses db().update() or row.update_record() on some previously select()ed row. Also, the main concern in your example are rounding errors and programmer mistake. Acquiring a FOR UPDATE lock on the record will ensure that concurrent threads cannot modify account.amount in the database until the end of transaction.
2. There is no risk of introducing new deadlocks. SELECT ... FOR UPDATE and UPDATE ... both apply the exact same type of lock on the exact same set of rows. If using SELECT ... FOR UPDATE in Table.update_or_insert() would cause deadlock, the current implementation will deadlock anyway when control flow reaches the subsequent update_record(). UPSERT in PostgreSQL is merely a performance optimization.
3. If you skip the initial SELECT and start with UPDATE instead, you won't be able to limit it to just one record. At least not with the limited update() features that PyDAL currently supports.

[niphlod]

I'm out of this conversation.
Transaction atomicity is what every "relational" database assures. Each player (be it a reader or a writer) gets or acts on a consistent state of truth.
Isolation levels are what rule about what kind of truth to return. A framework that is eminently used in Web frameworks can't seriously vouch to serialize db access by default.
Asking for a method to lock the record and serialize all access to it IMHO is out of anyone's league.

[mdipierro]

nextghost is right that this is an issue. We should change the behavior though. I proposed we simply allow to specify a _lock argument to update_or_insert. if people want, they will use it. Or it could be a parameter of the adapter. Now strong opinion.

[gi0baro]

@nextghost this is not for arguing, but:

• the type of lock is not the same. An UPDATE locks the involved records just during the update, while SELECT FOR UPDATE locks the records between different operations, so IMHO everything else should wait until completion. For instance, in your example if you change the first line with a FOR UPDATE, thread B cannot update the record until A execute update_record. See https://www.postgresql.org/docs/9.5/static/sql-select.html#SQL-FOR-UPDATE-SHARE for more info about locking on postgres.
• Table.__call__ simply select the first record using first(), so is barely an assurance of the fact there's just one record involved when we use it inside update_or_insert. It's simply the first corresponding to the query. Which is a debatable behavior. Is the first record in which order? And if you run an update_or_insert instruction with conditions that are not unique, then is the code which is wrong, not the library beyond.

Again, looking around I can't find any real cases where people are using FOR UPDATE to do an update or insert operation. On postgresql I would do something like this:

LOOP
    UPDATE sometable SET b = 'bar' WHERE a = 'foo';
    IF found THEN
        RETURN;
    END IF;
    BEGIN
        INSERT INTO sometable(a,b) VALUES ('foo', 'bar');
        RETURN;
    EXCEPTION WHEN unique_violation THEN
    END;
END LOOP;

on mysql I would do something like this:

INSERT 
    INTO sometable(a, b)
    VALUES ('foo', 'bar') 
    ON DUPLICATE KEY UPDATE
    a = 'foo', 
    b = 'bar';

and on any other database we support I have no idea.

@mdipierro I'm still to change update_or_insert to perform an update and an insert, avoiding the select. Seems the best behaviour to me. As @niphlod said, pydal is mainly used on web projects, so locking is not at option for me neither.

[mdipierro]

I like your proposal Giovanni. I think it solves the same problem in a more elegant way.

[nextghost]

@mdipierro Using whatever atomic version of update_or_insert() each database engine provides would be nice. But I think that adding an argument controlling the lock is not much of a solution. Anybody who knows what they're doing will set the lock on as soon as they find out that the parameter exists. Because the impact on performance will be negligible. The record gets locked either way and locking it a fraction of a millisecond earlier makes no difference. Everybody else will have to deal with random and completely unnecessary errors.

@niphlod The record will get locked by update_record() whether you like it or not.

@gi0baro Would you care to quote the specific part of PostgreSQL docs which says that rows modified by UPDATE will be unlocked early? Or you could just run this snippet of code to see for yourself that rows modified by UPDATE stay locked until the end of transaction:

connstr = 'FIXME'
def create_tables(db):
    db.define_table('lock_test', Field('foo', type="string"))
print('Create tables')
db1=DAL(connstr)
create_tables(db1)
db1.lock_test.insert(foo='foo')
db1.commit()
db2=DAL(connstr)
create_tables(db2)
db2.commit()

try:
    print('Update 1')
    db1(db1.lock_test).update(foo='bar')
    print('Update 2')
    db2(db2.lock_test).update(foo='baz')
    print('Commits')
    db1.commit()
    db2.commit()
    print('Results:')
    print(db1(db1.lock_test).select(db1.lock_test.ALL))
except:
    db1.rollback()
    db2.rollback()
    raise
finally:
    db1.lock_test.drop()
    db1.commit()

On the other hand, this snippet of code won't deadlock because Table.__call__() uses limitby=(0,1) to read (and lock) only the first record found.

connstr = 'FIXME'
def create_tables(db):
    db.define_table('lock_test', Field('foo', type="string"))
print('Create tables')
db1=DAL(connstr)
create_tables(db1)
first = db1.lock_test.insert(foo='foo')
second = db1.lock_test.insert(foo='foo')
db1.commit()
db2=DAL(connstr)
create_tables(db2)
db2.commit()

try:
    row = db1.lock_test(foo='foo', _for_update=True, _orderby=db1.lock_test.id)
    print(row)
    row2 = db2(db2.lock_test.id == second).select(db2.lock_test.ALL, for_update=True)
    print(row2)
    db1.commit()
    db2.commit()
except:
    db1.rollback()
    db2.rollback()
    raise
finally:
    db1.lock_test.drop()
    db1.commit()

For instance, in your example if you change the first line with a FOR UPDATE, thread B cannot update the record until A execute update_record.

Correction: Thread B cannot update it until Thread A commits the transaction. And that's the whole damn point why the record needs to get locked in the first place.

[gi0baro]

@nextghost I never said with UPDATE the record will be released earlier, I said that I see no point to lock it earlier. Again on your example: If you use a SELECT FOR UPDATE in thread A, and somehow the thread hangs before proceeding with the UPDATE and COMMIT or ROLLBACK, the thread B won't be able to perform the UPDATE. Practically speaking: if something goes wrong in one of the two threads, in one case you have chance to complete at least one operation, in the other one you will miss them both. That's all.

Considering all the examples, I still miss why this:

row = db.foo(bar='foo', _for_update=True)
if row:
    row.update_record(bar='bar')
    return None
return db.foo.insert(bar='bar')

is more correct than this:

updated = db(db.foo.bar == 'foo').update(bar='bar')
if not updated:
    return db.foo.insert(bar='bar')
return None

under your point of view.
And if db1(foo.bar == 'foo').count() > 1 is not an answer to my question, 'cause I already said I can't consider "updating just the first record corresponding the query" correct compared to "update all the records corresponding the query". If this is the condition, then the operation is wrong by principle, not by how is executed.

Finally: If we can't achieve an agreement, I can only propose to present two different PRs and call the team to vote for one or another.