Skip to content
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

IStorage: Require lastTransaction() to invalidate DB before returning #313

Closed
wants to merge 1 commit into from

Conversation

navytux
Copy link
Contributor

@navytux navytux commented Jun 8, 2020

Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
#290:

  • DB refreshes a Connection for new transaction;

  • zstor.lastTransaction() is called to obtain database view for this connection.

  • objects in live-cache for this Connection are invalidated with
    invalidations that were queued through DB.invalidate() calls from
    storage.

  • if lastTransaction does not guarantee that all DB invalidations for
    transactions with ID ≤ returned tid have been completed, it can be
    that:

    incomplete set of objects are invalidated in live cache

    i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

#307 (comment)
https://lab.nexedi.com/nexedi/neoppod/commit/5870c5d7

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gidzit, @jimfulton

navytux added a commit to navytux/ZODB that referenced this pull request Jun 8, 2020
… ...

... and to provide correct "current" view for load().

This is ZODB4 backport of commit 9a1d29f
(zopefoundation#313)

It has been amended with ZODB4-specific

      It is guaranteed that after lastTransaction returns, "current" view of
      the storage as observed by load() is ≥ returned tid.

because on ZODB<5 - contrary to ZODB5 - there is "load current" for
which correct semantic must be also provided:
zopefoundation#307 (comment)

Original description follows:

---- 8< ----

Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
zopefoundation#290:

- DB refreshes a Connection for new transaction;
- zstor.lastTransaction() is called to obtain database view for this connection.
- objects in live-cache for this Connection are invalidated with
  invalidations that were queued through DB.invalidate() calls from
  storage.
- if lastTransaction does not guarantee that all DB invalidations for
  transactions with ID ≤ returned tid have been completed, it can be
  that:

	incomplete set of objects are invalidated in live cache

  i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

zopefoundation#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

- already holds for FileStorage (no database mutations from outside of
  single process);
- is already true for ZEO4 and ZEO5
  zopefoundation#307 (review)
  zopefoundation#307 (comment)
- holds for RelStorage because it implements IMVCCStorage natively;
- is *not* currently true for NEO because NEO sets zstor.last_tid before
  calling DB.invalidate:

  https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

  zopefoundation#307 (comment)
  https://lab.nexedi.com/nexedi/neoppod/commit/5870c5d7

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
navytux referenced this pull request in navytux/ZODB Jun 8, 2020
Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
zopefoundation#290:

- DB refreshes a Connection for new transaction;
- zstor.lastTransaction() is called to obtain database view for this connection.
- objects in live-cache for this Connection are invalidated with
  invalidations that were queued through DB.invalidate() calls from
  storage.
- if lastTransaction does not guarantee that all DB invalidations for
  transactions with ID ≤ returned tid have been completed, it can be
  that:

	incomplete set of objects are invalidated in live cache

  i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

zopefoundation#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

- already holds for FileStorage (no database mutations from outside of
  single process);
- is already true for ZEO4 and ZEO5
  zopefoundation#307 (review)
  zopefoundation#307 (comment)
- holds for RelStorage because it implements IMVCCStorage natively;
- is *not* currently true for NEO because NEO sets zstor.last_tid before
  calling DB.invalidate:

  https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

  zopefoundation#307 (comment)
  https://lab.nexedi.com/nexedi/neoppod/commit/5870c5d7

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
@jmuchemb
Copy link
Member

jmuchemb commented Jun 8, 2020

I want to close it. I see no reason to work on this separately to #307.

Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
zopefoundation#290:

- DB refreshes a Connection for new transaction;
- zstor.lastTransaction() is called to obtain database view for this connection.
- objects in live-cache for this Connection are invalidated with
  invalidations that were queued through DB.invalidate() calls from
  storage.
- if lastTransaction does not guarantee that all DB invalidations for
  transactions with ID ≤ returned tid have been completed, it can be
  that:

	incomplete set of objects are invalidated in live cache

  i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

zopefoundation#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

- already holds for FileStorage (no database mutations from outside of
  single process);
- is already true for ZEO4 and ZEO5
  zopefoundation#307 (review)
  zopefoundation#307 (comment)
- holds for RelStorage because it implements IMVCCStorage natively;
- is *not* currently true for NEO because NEO sets zstor.last_tid before
  calling DB.invalidate:

  https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

  zopefoundation#307 (comment)
  https://lab.nexedi.com/nexedi/neoppod/commit/5870c5d7

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
navytux added a commit to navytux/ZODB that referenced this pull request Jun 9, 2020
… ...

... and to provide correct "current" view for load().

This is ZODB4 backport of commit b32d52f
(zopefoundation#313)

It has been amended with ZODB4-specific

      It is guaranteed that after lastTransaction returns, "current" view of
      the storage as observed by load() is ≥ returned tid.

because on ZODB<5 - contrary to ZODB5 - there is "load current" for
which correct semantic must be also provided:
zopefoundation#307 (comment)

Original description follows:

---- 8< ----

Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
zopefoundation#290:

- DB refreshes a Connection for new transaction;
- zstor.lastTransaction() is called to obtain database view for this connection.
- objects in live-cache for this Connection are invalidated with
  invalidations that were queued through DB.invalidate() calls from
  storage.
- if lastTransaction does not guarantee that all DB invalidations for
  transactions with ID ≤ returned tid have been completed, it can be
  that:

	incomplete set of objects are invalidated in live cache

  i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

zopefoundation#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

- already holds for FileStorage (no database mutations from outside of
  single process);
- is already true for ZEO4 and ZEO5
  zopefoundation#307 (review)
  zopefoundation#307 (comment)
- holds for RelStorage because it implements IMVCCStorage natively;
- is *not* currently true for NEO because NEO sets zstor.last_tid before
  calling DB.invalidate:

  https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

  zopefoundation#307 (comment)
  https://lab.nexedi.com/nexedi/neoppod/commit/5870c5d7

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
@navytux
Copy link
Contributor Author

navytux commented Jun 9, 2020

Updated patch with the following interdiff due to @jmuchemb notice:

--- a/src/ZODB/interfaces.py
+++ b/src/ZODB/interfaces.py
@@ -687,7 +687,7 @@ def lastTransaction():
         """Return the id of the last committed transaction.

         Returned tid is ID of last committed transaction as observed from
-        some time _before_ lastTransaction call was made. In particular for
+        some time _before_ lastTransaction call returns. In particular for
         client-sever case, lastTransaction can return cached view of storage
         that was learned some time ago.

@navytux
Copy link
Contributor Author

navytux commented Jul 31, 2020

The interface was changed via different text as part of #307 (4a6b028#diff-881ceb274f9e538d4144950eefce8682R685-R695). That change was discussed here: #307 (comment). Closing.

@navytux navytux closed this Jul 31, 2020
@navytux navytux deleted the y/istorage-lasttid branch July 31, 2020 08:50
navytux added a commit to navytux/ZODB that referenced this pull request Jul 31, 2020
…eturning ...

... and to provide correct "current" view for load().

This is ZODB4 backport of zopefoundation#313,
which itself is just a more explicit language of
zopefoundation@4a6b0283#diff-881ceb274f9e538d4144950eefce8682R685-R695

It has been amended with ZODB4-specific

      It is guaranteed that after lastTransaction returns, "current" view of
      the storage as observed by load() is ≥ returned tid.

because on ZODB<5 - contrary to ZODB5 - there is "load current" for
which correct semantic must be also provided:
zopefoundation#307 (comment)

Original description follows:

---- 8< ----

Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
zopefoundation#290:

- DB refreshes a Connection for new transaction;
- zstor.lastTransaction() is called to obtain database view for this connection.
- objects in live-cache for this Connection are invalidated with
  invalidations that were queued through DB.invalidate() calls from
  storage.
- if lastTransaction does not guarantee that all DB invalidations for
  transactions with ID ≤ returned tid have been completed, it can be
  that:

	incomplete set of objects are invalidated in live cache

  i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

zopefoundation#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

- already holds for FileStorage (no database mutations from outside of
  single process);
- is already true for ZEO4 and ZEO5
  zopefoundation#307 (review)
  zopefoundation#307 (comment)
- holds for RelStorage because it implements IMVCCStorage natively;
- is *not* currently true for NEO because NEO sets zstor.last_tid before
  calling DB.invalidate:

  https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

  zopefoundation#307 (comment)
  https://lab.nexedi.com/nexedi/neoppod/commit/a7d101ec
  https://lab.nexedi.com/nexedi/neoppod/commit/a7d101ec#note_112238
  https://lab.nexedi.com/nexedi/neoppod/commit/a7d101ec#note_113331

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
navytux added a commit to navytux/ZODB that referenced this pull request Nov 30, 2020
…eturning ...

... and to provide correct "current" view for load().

This is ZODB4 backport of zopefoundation#313,
which itself is just a more explicit language of
zopefoundation@4a6b0283#diff-881ceb274f9e538d4144950eefce8682R685-R695

It has been amended with ZODB4-specific

      It is guaranteed that after lastTransaction returns, "current" view of
      the storage as observed by load() is ≥ returned tid.

because on ZODB<5 - contrary to ZODB5 - there is "load current" for
which correct semantic must be also provided:
zopefoundation#307 (comment)

Original description follows:

---- 8< ----

Because if lastTransaction() returns tid, for which local database
handle has not yet been updated with invalidations, it could lead to
data corruption due to concurrency issues similar to
zopefoundation#290:

- DB refreshes a Connection for new transaction;
- zstor.lastTransaction() is called to obtain database view for this connection.
- objects in live-cache for this Connection are invalidated with
  invalidations that were queued through DB.invalidate() calls from
  storage.
- if lastTransaction does not guarantee that all DB invalidations for
  transactions with ID ≤ returned tid have been completed, it can be
  that:

	incomplete set of objects are invalidated in live cache

  i.e. data corruption.

This particular data corruption has been hit when working on core of
ZODB and was not immediately noticed:

zopefoundation#307 (review)

this fact justifies the importance of explicitly stating what IStorage
guarantees are / must be in the interface.

This guarantee

- already holds for FileStorage (no database mutations from outside of
  single process);
- is already true for ZEO4 and ZEO5
  zopefoundation#307 (review)
  zopefoundation#307 (comment)
- holds for RelStorage because it implements IMVCCStorage natively;
- is *not* currently true for NEO because NEO sets zstor.last_tid before
  calling DB.invalidate:

  https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124

However NEO is willing to change and already prepared the fix to provide
this guarantee because of data corruption scenario that can happen
without it:

  zopefoundation#307 (comment)
  https://lab.nexedi.com/nexedi/neoppod/commit/a7d101ec
  https://lab.nexedi.com/nexedi/neoppod/commit/a7d101ec#note_112238
  https://lab.nexedi.com/nexedi/neoppod/commit/a7d101ec#note_113331

In other words all storages that, to my knowledge, are in current use
are either already providing specified semantic, or will be shortly
fixed to provide it.

This way we can fix up the interface and make the semantic clear.

/cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants