Relational JDBC: Reuse connection with execute select when within transaction #3334
Conversation
sfc-gh-prsingh
force-pushed
the
feature/jdbc-fix-transaction
branch
from
0516439 to
6b013ce
Compare
...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
Outdated
Show resolved
Hide resolved
sfc-gh-prsingh
force-pushed
the
feature/jdbc-fix-transaction
branch
from
fd91797 to
da34869
Compare
sfc-gh-prsingh
force-pushed
the
feature/jdbc-fix-transaction
branch
from
da34869 to
192d787
Compare
There was a problem hiding this comment.
LGTM 👍 Thanks, @singhpk234 !
Yet... CI failed 🤔
| * @param <T> : Business entity class | ||
| * @throws SQLException : Exception during query execution | ||
| */ | ||
| private <T> List<T> executeSelectWithConnection( |
There was a problem hiding this comment.
nit: this method is called from only one place... why not inline?
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| PolarisBaseEntity existingEntity = | ||
| lookupEntityByName( | ||
| callCtx, | ||
| entity.getCatalogId(), | ||
| entity.getParentId(), | ||
| entity.getTypeCode(), | ||
| entity.getName()); | ||
| // This happens in two scenarios: | ||
| // 1. PRIMARY KEY violated | ||
| // 2. UNIQUE CONSTRAINT on (realm_id, catalog_id, parent_id, type_code, name) violated | ||
| // With SERIALIZABLE isolation, the conflicting entity may _not_ be visible and | ||
| // existingEntity can be null, which would cause an NPE in | ||
| // EntityAlreadyExistsException.message(). | ||
| throw new EntityAlreadyExistsException( | ||
| existingEntity != null ? existingEntity : entity, e); |
There was a problem hiding this comment.
PG aborts the transaction when it encounters the error :
If a statement within a transaction raises an exception, the entire transaction is
aborted and all statements are rolled back. The transaction remains in an aborted state,
and no further SQL commands will be executed until a ROLLBACK command is issued.
hence a lookup after write failure is not possible, the tests failed because of this.
I changed to read before write attempt lets say we are working with SERIALIZABLE isolation would work now. I see and think more it was intentional :'( as the comment says, to do it this way worst case was we not able to get existing entity which would have been fine.
Now lookup before before write is gonna cause some perf issue (but only for certain cases) and wrapping writeEntity would elevate the scenario more, will need to run some benchmark before we check it in,
There was a problem hiding this comment.
Thx for the research! That sounds like a good plan 👍
There was a problem hiding this comment.
Alternatively, we could explicitly start a new Tx for lookupEntityByName only on failure... WDYT?
There was a problem hiding this comment.
precisely presently thats what we are doing in the current code, the connection is a separate connection than the one used in the transaction in cases of writeEntities or a completely different connection in writeEntity i am also debating the same is it worth it, i will run benchmark for this 50:50 R/W to see how much we deviate from prev numbers as recorded here : #1517 (comment)
3 participants
About the changes
While reading code, i realized this particular check was not happening in the transaction i.e let say we observe a constrain violation because within a transaction we added some entries, we would not be able to find which entity caused the SQL constraint violation in the first place, we might get incorrect entity in the exception, though the exception remains the same.
The proposed fix just uses the same conenction which is being used in the transaction and uses that to do the lookup to make sure the entity is found correctly.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)