New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimistic locking #658

Closed
tuukkamustonen opened this Issue Jul 15, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@tuukkamustonen

tuukkamustonen commented Jul 15, 2015

I'm not sure what is the correct way to implement support optimistic locking. As usual, I want to hold a version column that I increment on updates, while also ensuring that I am updating the row with the current version.

I assume I could implement this with something like:

Layer.update(name=self.name, label=self.label, version=self.version + 1).where((id=self.id) & (version=self.version))

But here, I need to explicitly list all the fields/values in update(...). Is there some shortcut?

@coleifer

This comment has been minimized.

Show comment
Hide comment
@coleifer

coleifer Jul 16, 2015

Owner

The typical way to handle this is to select the record you're modifying FOR UPDATE (see docs). Here's a blog post you might also find useful. Note that this only works with Postgresql.

Owner

coleifer commented Jul 16, 2015

The typical way to handle this is to select the record you're modifying FOR UPDATE (see docs). Here's a blog post you might also find useful. Note that this only works with Postgresql.

@coleifer coleifer closed this Jul 16, 2015

@tuukkamustonen

This comment has been minimized.

Show comment
Hide comment
@tuukkamustonen

tuukkamustonen Jul 16, 2015

@coleifer FOR UPDATE is for pessimistic locking, while I explicitly need optimistic locking here. A user who opens up data for modification might think for a long time (while modifying a form) and even vanish without never submitting the changes. So, optimistic locking suits my use case better.

It sounds like there's no builtin support for this. Would you implement it as I sketched above or do you have a more sophisticated approach?

tuukkamustonen commented Jul 16, 2015

@coleifer FOR UPDATE is for pessimistic locking, while I explicitly need optimistic locking here. A user who opens up data for modification might think for a long time (while modifying a form) and even vanish without never submitting the changes. So, optimistic locking suits my use case better.

It sounds like there's no builtin support for this. Would you implement it as I sketched above or do you have a more sophisticated approach?

@coleifer

This comment has been minimized.

Show comment
Hide comment
@coleifer

coleifer Jul 16, 2015

Owner

I don't have any plans on adding this to Peewee itself, so you can choose the implementation that best suits your use-case.

Owner

coleifer commented Jul 16, 2015

I don't have any plans on adding this to Peewee itself, so you can choose the implementation that best suits your use-case.

@tuukkamustonen

This comment has been minimized.

Show comment
Hide comment
@tuukkamustonen

tuukkamustonen Dec 7, 2015

@coleifer I finally have to implement this. If I made a PR for it, would consider merging it into peewee? It could work like:

class MyModel(peewee.Model):
    version = IntegerVersionField()

m = MyModel()
m.save()
m1 = MyModel.get()
m2 = MyModel.get()
m1.save()
m2.save()  # would throw error

m2.save(optimistic=False)  # temporarily disable locking

Or it could be:

class MyModel(peewee.Model):
    version = IntegerField(default=1)

    class Meta:
        version_field = 'version'

I don't have strong opinion on the API, I just wish it was included in peewee and would not break during version upgrades. This touches part of core functionalities (save()) so I'm not sure if this could go into playhouse?

It shouldn't need too many modifications to Model.save(), the core idea would be something like:

        if self._meta.version_field:
            field_dict[self._meta.version_field] += 1
            rows = self.update(**field_dict).where(self._pk_expr() & (self._meta.fields[self._meta.version_field] == getattr(self, self._meta.version_field))).execute()
            if rows != 1:
                raise Exception("Something to indicate that update did not succeed. The cause is probably in version mismatch, but it could be that object with PK was no longer there :|")
        else:
            rows = self.update(**field_dict).where(self._pk_expr()).execute()

Maybe something more sophisticated than getattr etc.

I am personally doing locking based on integer version field, but it could be timestamp, hash, or anything.

What do you think?

tuukkamustonen commented Dec 7, 2015

@coleifer I finally have to implement this. If I made a PR for it, would consider merging it into peewee? It could work like:

class MyModel(peewee.Model):
    version = IntegerVersionField()

m = MyModel()
m.save()
m1 = MyModel.get()
m2 = MyModel.get()
m1.save()
m2.save()  # would throw error

m2.save(optimistic=False)  # temporarily disable locking

Or it could be:

class MyModel(peewee.Model):
    version = IntegerField(default=1)

    class Meta:
        version_field = 'version'

I don't have strong opinion on the API, I just wish it was included in peewee and would not break during version upgrades. This touches part of core functionalities (save()) so I'm not sure if this could go into playhouse?

It shouldn't need too many modifications to Model.save(), the core idea would be something like:

        if self._meta.version_field:
            field_dict[self._meta.version_field] += 1
            rows = self.update(**field_dict).where(self._pk_expr() & (self._meta.fields[self._meta.version_field] == getattr(self, self._meta.version_field))).execute()
            if rows != 1:
                raise Exception("Something to indicate that update did not succeed. The cause is probably in version mismatch, but it could be that object with PK was no longer there :|")
        else:
            rows = self.update(**field_dict).where(self._pk_expr()).execute()

Maybe something more sophisticated than getattr etc.

I am personally doing locking based on integer version field, but it could be timestamp, hash, or anything.

What do you think?

@coleifer

This comment has been minimized.

Show comment
Hide comment
@coleifer

coleifer Dec 9, 2015

Owner

That's pretty cool, but honestly I think that is just enough beyond the
scope that I'd rather it live as its own project. By all means you can
create your own base middle class that overrides save.

There is enough complexity there that I think I could get bogged down
supporting exotic edge cases or who knows, and really I want to focus on
peewee.
On Dec 7, 2015 2:39 AM, "Tuukka Mustonen" notifications@github.com wrote:

@coleifer https://github.com/coleifer I finally have to implement this.
If I made a PR for it, would consider merging it into peewee? It could work
like:

class MyModel(peewee.Model):
version = IntegerVersionField()

m = MyModel()
m.save()
m1 = MyModel.get()
m2 = MyModel.get()
m1.save()
m2.save() # would throw error

m2.save(optimistic=False) # temporarily disable locking

Or it could be:

class MyModel(peewee.Model):
version = IntegerField(default=1)

class Meta:
    version_field = 'version'

I don't have strong opinion on the API, I just wish it was included in
peewee and would not break during version upgrades. This touches part of
core functionalities (save()) so I'm not sure if this could go into
playhouse?

It shouldn't need too many modifications to Model.save(), the core idea
would be something like:

    if self._meta.version_field:
        field_dict[self._meta.version_field] += 1
        rows = self.update(**field_dict).where(self._pk_expr() & (self._meta.fields[self._meta.version_field] == getattr(self, self._meta.version_field))).execute()
        if rows != 1:
            raise Exception("Something to indicate that update did not succeed. The cause is probably in version mismatch, but it could be that object with PK was no longer there :|")
    else:
        rows = self.update(**field_dict).where(self._pk_expr()).execute()

Maybe something more sophisticated than getattr etc.

I am personally doing locking based on integer version field, but it could
be timestamp, hash, or anything.

What do you think?


Reply to this email directly or view it on GitHub
#658 (comment).

Owner

coleifer commented Dec 9, 2015

That's pretty cool, but honestly I think that is just enough beyond the
scope that I'd rather it live as its own project. By all means you can
create your own base middle class that overrides save.

There is enough complexity there that I think I could get bogged down
supporting exotic edge cases or who knows, and really I want to focus on
peewee.
On Dec 7, 2015 2:39 AM, "Tuukka Mustonen" notifications@github.com wrote:

@coleifer https://github.com/coleifer I finally have to implement this.
If I made a PR for it, would consider merging it into peewee? It could work
like:

class MyModel(peewee.Model):
version = IntegerVersionField()

m = MyModel()
m.save()
m1 = MyModel.get()
m2 = MyModel.get()
m1.save()
m2.save() # would throw error

m2.save(optimistic=False) # temporarily disable locking

Or it could be:

class MyModel(peewee.Model):
version = IntegerField(default=1)

class Meta:
    version_field = 'version'

I don't have strong opinion on the API, I just wish it was included in
peewee and would not break during version upgrades. This touches part of
core functionalities (save()) so I'm not sure if this could go into
playhouse?

It shouldn't need too many modifications to Model.save(), the core idea
would be something like:

    if self._meta.version_field:
        field_dict[self._meta.version_field] += 1
        rows = self.update(**field_dict).where(self._pk_expr() & (self._meta.fields[self._meta.version_field] == getattr(self, self._meta.version_field))).execute()
        if rows != 1:
            raise Exception("Something to indicate that update did not succeed. The cause is probably in version mismatch, but it could be that object with PK was no longer there :|")
    else:
        rows = self.update(**field_dict).where(self._pk_expr()).execute()

Maybe something more sophisticated than getattr etc.

I am personally doing locking based on integer version field, but it could
be timestamp, hash, or anything.

What do you think?


Reply to this email directly or view it on GitHub
#658 (comment).

@arikfr

This comment has been minimized.

Show comment
Hide comment
@arikfr

arikfr Dec 9, 2015

Personally I would love to see this as part of peewee, as it's something I found useful when I was using Ruby's ActiveRecord. Until now I wasn't doing optimistic locking in my peewee based projects, just because I was lazy to implement it myself...

@tuukkamustonen if you end up implementing it in a separate lib, please post the link here.
And about your suggested implementation - I suggest using a dedicated exception, so it can be properly handled by using code.

arikfr commented Dec 9, 2015

Personally I would love to see this as part of peewee, as it's something I found useful when I was using Ruby's ActiveRecord. Until now I wasn't doing optimistic locking in my peewee based projects, just because I was lazy to implement it myself...

@tuukkamustonen if you end up implementing it in a separate lib, please post the link here.
And about your suggested implementation - I suggest using a dedicated exception, so it can be properly handled by using code.

@tconkling

This comment has been minimized.

Show comment
Hide comment
@tconkling

tconkling Mar 7, 2016

@tuukkamustonen: just throwing a +1 here - if this is something you've already written, I'd love to see the code!

tconkling commented Mar 7, 2016

@tuukkamustonen: just throwing a +1 here - if this is something you've already written, I'd love to see the code!

@tuukkamustonen

This comment has been minimized.

Show comment
Hide comment
@tuukkamustonen

tuukkamustonen Mar 7, 2016

@arikfr @tconkling Well, I am just pasting here the code I'm running atm. Maybe @coleifer wants to sanity check it, too.

I am not running it in production, nor do I have extensive test suite for it. It's just something I threw in for now, and I actually had it back in my head to ask for comments at some point.

The big drawback is that this code is pretty much bound to a specific peewee version, because it re-writes the whole save method. Of course, one could rename it differently, but, well.

A model is defined by:

class MyModel(LockableModel):

    class Meta(object):
        version_field = 'version'

    version = IntegerField(default=1)

It only supports numeric locking field.

And the actual implementation (my only additions are the two blocks with # Comments):

class ConcurrentModification(PeeweeException):
    """Indicates that entity has been changed in the database between loading and saving."""

class LockableModel(peewee.Model):

    # The base was copied from peewee 2.7.4
    def save(self, force_insert=False, only=None, optimistic=True):
        field_dict = dict(self._data)
        if self._meta.primary_key is not False:
            pk_field = self._meta.primary_key
            pk_value = self._get_pk_value()
        else:
            pk_field = pk_value = None
        if only:
            field_dict = self._prune_fields(field_dict, only)
        elif getattr(self._meta, 'only_save_dirty', False) and not force_insert:
            field_dict = self._prune_fields(
                field_dict,
                self.dirty_fields)
            # Empty dict is ok with optimistic locking (to simply increment version)
            # XXX: Is this really needed? There is no use case for this!
            if not field_dict and (
                not getattr(self._meta, 'version_field', None) or (
                    getattr(self._meta, 'version_field', None) and not optimistic
                )
            ):
                self._dirty.clear()
                return False

        self._populate_unsaved_relations(field_dict)
        if pk_value is not None and not force_insert:
            if self._meta.composite_key:
                for pk_part_name in pk_field.field_names:
                    field_dict.pop(pk_part_name, None)
            else:
                field_dict.pop(pk_field.name, None)

            # Locking
            if optimistic and getattr(self._meta, 'version_field', None):
                # Add and increment version field, if it was not manually given. This is the normal behavior
                if self._meta.version_field in field_dict:
                    raise ValueError('You must not modify version field manually while using locking!')
                field_dict[self._meta.version_field] = self._data[self._meta.version_field] + 1
                rows = self.update(**field_dict).where(
                    self._pk_expr() &
                    (
                        self._meta.fields[self._meta.version_field] ==
                        getattr(self, self._meta.version_field)
                    )
                ).execute()
                if rows == 0:
                    entity = self.select().where(self._pk_expr()).get()  # throws DoesNotExist if not found, but it should...
                    raise ConcurrentModification(
                        'Tried to save entity with version of `{}` but database contains entry with version of `{}`!'
                        .format(field_dict[self._meta.version_field], getattr(entity, self._meta.version_field))
                    )
                # Also set correct version on this instance, if update succeeded
                setattr(self, self._meta.version_field, field_dict[self._meta.version_field])

            else:
                rows = self.update(**field_dict).where(self._pk_expr()).execute()
        elif pk_field is None:
            self.insert(**field_dict).execute()
            rows = 1
        else:
            pk_from_cursor = self.insert(**field_dict).execute()
            if pk_from_cursor is not None:
                pk_value = pk_from_cursor
            self._set_pk_value(pk_value)
            rows = 1
        self._dirty.clear()
        return rows

So, calling save() does the usual optimistic locking stuff, and calling save(optimistic=False) disables the feature.

tuukkamustonen commented Mar 7, 2016

@arikfr @tconkling Well, I am just pasting here the code I'm running atm. Maybe @coleifer wants to sanity check it, too.

I am not running it in production, nor do I have extensive test suite for it. It's just something I threw in for now, and I actually had it back in my head to ask for comments at some point.

The big drawback is that this code is pretty much bound to a specific peewee version, because it re-writes the whole save method. Of course, one could rename it differently, but, well.

A model is defined by:

class MyModel(LockableModel):

    class Meta(object):
        version_field = 'version'

    version = IntegerField(default=1)

It only supports numeric locking field.

And the actual implementation (my only additions are the two blocks with # Comments):

class ConcurrentModification(PeeweeException):
    """Indicates that entity has been changed in the database between loading and saving."""

class LockableModel(peewee.Model):

    # The base was copied from peewee 2.7.4
    def save(self, force_insert=False, only=None, optimistic=True):
        field_dict = dict(self._data)
        if self._meta.primary_key is not False:
            pk_field = self._meta.primary_key
            pk_value = self._get_pk_value()
        else:
            pk_field = pk_value = None
        if only:
            field_dict = self._prune_fields(field_dict, only)
        elif getattr(self._meta, 'only_save_dirty', False) and not force_insert:
            field_dict = self._prune_fields(
                field_dict,
                self.dirty_fields)
            # Empty dict is ok with optimistic locking (to simply increment version)
            # XXX: Is this really needed? There is no use case for this!
            if not field_dict and (
                not getattr(self._meta, 'version_field', None) or (
                    getattr(self._meta, 'version_field', None) and not optimistic
                )
            ):
                self._dirty.clear()
                return False

        self._populate_unsaved_relations(field_dict)
        if pk_value is not None and not force_insert:
            if self._meta.composite_key:
                for pk_part_name in pk_field.field_names:
                    field_dict.pop(pk_part_name, None)
            else:
                field_dict.pop(pk_field.name, None)

            # Locking
            if optimistic and getattr(self._meta, 'version_field', None):
                # Add and increment version field, if it was not manually given. This is the normal behavior
                if self._meta.version_field in field_dict:
                    raise ValueError('You must not modify version field manually while using locking!')
                field_dict[self._meta.version_field] = self._data[self._meta.version_field] + 1
                rows = self.update(**field_dict).where(
                    self._pk_expr() &
                    (
                        self._meta.fields[self._meta.version_field] ==
                        getattr(self, self._meta.version_field)
                    )
                ).execute()
                if rows == 0:
                    entity = self.select().where(self._pk_expr()).get()  # throws DoesNotExist if not found, but it should...
                    raise ConcurrentModification(
                        'Tried to save entity with version of `{}` but database contains entry with version of `{}`!'
                        .format(field_dict[self._meta.version_field], getattr(entity, self._meta.version_field))
                    )
                # Also set correct version on this instance, if update succeeded
                setattr(self, self._meta.version_field, field_dict[self._meta.version_field])

            else:
                rows = self.update(**field_dict).where(self._pk_expr()).execute()
        elif pk_field is None:
            self.insert(**field_dict).execute()
            rows = 1
        else:
            pk_from_cursor = self.insert(**field_dict).execute()
            if pk_from_cursor is not None:
                pk_value = pk_from_cursor
            self._set_pk_value(pk_value)
            rows = 1
        self._dirty.clear()
        return rows

So, calling save() does the usual optimistic locking stuff, and calling save(optimistic=False) disables the feature.

@tconkling

This comment has been minimized.

Show comment
Hide comment
@tconkling

tconkling Mar 7, 2016

Cool, I'll play with this myself!

It seems like something @coleifer could possibly support by breaking out some of the save() functionality out into several other functions that custom subclasses, like your optimistic-locking model, could override?

tconkling commented Mar 7, 2016

Cool, I'll play with this myself!

It seems like something @coleifer could possibly support by breaking out some of the save() functionality out into several other functions that custom subclasses, like your optimistic-locking model, could override?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment