Fix concurrency errors that could cause corruption or dropped updates #1092
Conversation
…lipselink.transaction.join-existing Set the Isolation level in both a new EclipseLink SessionCustomizer as well as providing default postgresql.conf and injecting it in the getting-started docker-compose. Set eclipselink.transaction.join-existing in persistence.xml to force reads to go through the same write connection per EntityManager session, otherwise reads are not consistent with writes in a transaction. Only call rollback() if the transaction is still active, otherwise we get 500 server errors.
…eateTableLike/updateTableLike Without this fix, even if the underlying database layer does the right thing in resolving concurrency conflicts, the API call will incorrectly return a 200 success. This isn't too common of a race condition under normal operation since BasePolarisCatalog does redundant state-checking right before the transaction (which maybe should be removed in the future for performance), but in theory this bug indeed could've caused dropped writes under high concurrency.
dennishuo
requested review from
MonkeyCanCode,
RussellSpitzer,
collado-mike,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
|
@jbonofre I'm trying to fix the license documentation by including the following in the main However, it looks like our main Alternatively, if PostgreSQL license is messy to incorporate especially if it has implications on downstream users of Polaris, I could rewrite the config update to be something where the Additionally, the file |
a note that the sample conf file can be found at the postgres github mirror, and then only specify the settings we immediately care about.
|
Okay, thinking about it more, including the whole sample conf file for postgres might be too cluttered anyways, so instead I just added a link to the postgres github repo where the sample conf file can be looked at for anything who wants to customize their postgres settings easily, and then only include the particular config changes we care about. |
…zer for attribution of the PolarisEclipseLinkSessionCustomizer.java code.
… start successfully.
| max_wal_size = 1GB | ||
| min_wal_size = 80MB |
There was a problem hiding this comment.
Are these just defaults, or where did these come from?
There was a problem hiding this comment.
Yeah they were in the postgres docker image. Originally I copy pasted the whole file but it was too cluttered so I just literally filtered to keep only the non-commented-out default settings.
| tr.rollback(); | ||
| // For some transaction conflict errors, the transaction will already no longer be active; | ||
| // if it's still active, explicitly rollback. | ||
| if (tr.isActive()) { |
There was a problem hiding this comment.
Good point. I guess that might be the reason for 500 errors in my old test with Serializable isolation.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
…apache#1092) * Explicitly set transaction isolation level to SERIALIZABLE and set eclipselink.transaction.join-existing Set the Isolation level in both a new EclipseLink SessionCustomizer as well as providing default postgresql.conf and injecting it in the getting-started docker-compose. Set eclipselink.transaction.join-existing in persistence.xml to force reads to go through the same write connection per EntityManager session, otherwise reads are not consistent with writes in a transaction. Only call rollback() if the transaction is still active, otherwise we get 500 server errors. * Fix handling of non-SUCCESS ReturnStatus in BasePolarisCatalog for createTableLike/updateTableLike Without this fix, even if the underlying database layer does the right thing in resolving concurrency conflicts, the API call will incorrectly return a 200 success. This isn't too common of a race condition under normal operation since BasePolarisCatalog does redundant state-checking right before the transaction (which maybe should be removed in the future for performance), but in theory this bug indeed could've caused dropped writes under high concurrency. * Instead of including the entire sample postgres.conf file, just include a note that the sample conf file can be found at the postgres github mirror, and then only specify the settings we immediately care about. * Add a link to the EclipseLink tutorial for injecting a SessionCustomizer for attribution of the PolarisEclipseLinkSessionCustomizer.java code. * Fix format * Add some other required default postgres.conf settings so that it can start successfully.
4 participants
This fixes #1076
Explicitly set transaction isolation level to SERIALIZABLE and set eclipselink.transaction.join-existing
Set the Isolation level in both a new EclipseLink SessionCustomizer as well as providing
default postgresql.conf and injecting it in the getting-started docker-compose.
Set eclipselink.transaction.join-existing in persistence.xml to force reads to go through
the same write connection per EntityManager session, otherwise reads are not consistent with
writes in a transaction.
Only call rollback() if the transaction is still active, otherwise we get 500 server errors.
Fix handling of non-SUCCESS ReturnStatus in BasePolarisCatalog for createTableLike/updateTableLike
Without this fix, even if the underlying database layer does the right thing in resolving
concurrency conflicts, the API call will incorrectly return a 200 success.
This isn't too common of a race condition under normal operation since BasePolarisCatalog
does redundant state-checking right before the transaction (which maybe should be removed
in the future for performance), but in theory this bug indeed could've caused dropped writes
under high concurrency.