JDBC: Optimize writeEntity calls #1496

singhpk234 · 2025-04-29T18:56:23Z

About the change

Optimize writeEntity calls by extracting the calls out of transaction. please ref to benchmarks below for the motivation for the change.

Note : Handling transaction failures and retries are not in the scope of this pr. This is purely for optimization purpose.

Running Branchmark :

started the jdbc implementation backed Service : https://github.com/apache/polaris/pull/1470/files#diff-44e2226f0bc5712cf1cd511efd94242a08c9760b947809c7ebe76f121f480c76

Updated creds via

--- a/benchmarks/src/gatling/resources/benchmark-defaults.conf
+++ b/benchmarks/src/gatling/resources/benchmark-defaults.conf
@@ -28,11 +28,11 @@ http {
 auth {
   # OAuth2 client ID for authentication
   # Required: Must be provided in configuration
-  client-id = null
+  client-id = root

   # OAuth2 client secret for authentication
   # Required: Must be provided in configuration
-  client-secret = null
+  client-secret = s3cr3t
 }

go to benchmark tools and run the following benchmarks :

# Dataset creation
./gradlew gatlingRun --simulation org.apache.polaris.benchmarks.simulations.CreateTreeDataset

# Read/Update operations
./gradlew gatlingRun --simulation org.apache.polaris.benchmarks.simulations.ReadUpdateTreeDataset

# Read-only operations
./gradlew gatlingRun --simulation org.apache.polaris.benchmarks.simulations.ReadTreeDataset

withRunInTransaction

========================================================================================================================
---- Global Information -------------------------------------------------------------|---Total---|-----OK----|----KO----
> request count                                                                      |    30,108 |    30,092 |        16
> min response time (ms)                                                             |         1 |         1 |         7
> max response time (ms)                                                             |       603 |       603 |       243
> mean response time (ms)                                                            |        17 |        17 |        99
> response time std deviation (ms)                                                   |        23 |        23 |        74
> response time 25th percentile (ms)                                                 |         6 |         6 |        44
> response time 50th percentile (ms)                                                 |        11 |        11 |        76
> response time 75th percentile (ms)                                                 |        22 |        22 |       171
> response time 99th percentile (ms)                                                 |        96 |        94 |       243
> mean throughput (rps)                                                              |      83.4 |     83.36 |      0.04
---- Response Time Distribution ----------------------------------------------------------------------------------------
> OK: t < 800 ms                                                                                         30,092 (99.95%)
> OK: 800 ms <= t < 1200 ms                                                                                   0     (0%)
> OK: t >= 1200 ms                                                                                            0     (0%)
> KO                                                                                                         16  (0.05%)
---- Errors ------------------------------------------------------------------------------------------------------------
> status.find.is(200), but actually found 500                                                                15 (93.75%)
> status.find.is(204), but actually found 500                                                                 1  (6.25%)
========================================================================================================================

without runInTransaction

========================================================================================================================
---- Global Information -------------------------------------------------------------|---Total---|-----OK----|----KO----
> request count                                                                      |    30,009 |    30,001 |         8
> min response time (ms)                                                             |         1 |         1 |        10
> max response time (ms)                                                             |       293 |       293 |        89
> mean response time (ms)                                                            |        16 |        16 |        33
> response time std deviation (ms)                                                   |        16 |        16 |        27
> response time 25th percentile (ms)                                                 |         6 |         6 |        16
> response time 50th percentile (ms)                                                 |        12 |        12 |        20
> response time 75th percentile (ms)                                                 |        23 |        23 |        60
> response time 99th percentile (ms)                                                 |        77 |        76 |        89
> mean throughput (rps)                                                              |     83.13 |     83.11 |      0.02
---- Response Time Distribution ----------------------------------------------------------------------------------------
> OK: t < 800 ms                                                                                         30,001 (99.97%)
> OK: 800 ms <= t < 1200 ms                                                                                   0     (0%)
> OK: t >= 1200 ms                                                                                            0     (0%)
> KO                                                                                                          8  (0.03%)
---- Errors ------------------------------------------------------------------------------------------------------------
> status.find.is(200), but actually found 500                                                                 6    (75%)
> status.find.is(204), but actually found 500                                                                 2    (25%)
========================================================================================================================

dimas-b · 2025-04-29T20:28:00Z

@singhpk234 : could you give a bit more details on the error that prompted this PR?

flyrain

The number is reasonable and solid. The error rate dropped and overall response time dropped as well. Thanks @singhpk234 !

...n/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java

singhpk234 · 2025-04-29T20:57:09Z

@dimas-b here is the logs for he same :

2025-04-29 20:35:59.540 UTC [1] LOG:  listening on Unix socket "/var/run/postgresql/.s.PGSQL.5432"
2025-04-29 20:35:59.545 UTC [63] LOG:  database system was shut down at 2025-04-29 20:35:59 UTC
2025-04-29 20:35:59.559 UTC [1] LOG:  database system is ready to accept connections
2025-04-29 20:39:14.685 UTC [175] ERROR:  could not serialize access due to read/write dependencies among transactions
2025-04-29 20:39:14.685 UTC [175] DETAIL:  Reason code: Canceled on conflict out to pivot 894, during read.
2025-04-29 20:39:14.685 UTC [175] HINT:  The transaction might succeed if retried.
2025-04-29 20:39:14.685 UTC [175] STATEMENT:  SELECT entity_version, to_purge_timestamp, internal_properties, catalog_id, purge_timestamp, sub_type_code, create_timestamp, last_update_timestamp, parent_id, name, id, drop_timestamp, properties, grant_records_version, type_code FROM POLARIS_SCHEMA.ENTITIES WHERE (catalog_id, id) IN ((0, 8213068413194098796),(0, 2285716631442680836),(0, 3182916641487612758),(3182916641487612758, 3957433893749870399),(0, 0),(3182916641487612758, 6835667964051947808)) AND realm_id = 'POLARIS'
2025-04-29 20:39:14.688 UTC [84] ERROR:  could not serialize access due to read/write dependencies among transactions
2025-04-29 20:39:14.688 UTC [84] DETAIL:  Reason code: Canceled on identification as a pivot, during commit attempt.
2025-04-29 20:39:14.688 UTC [84] HINT:  The transaction might succeed if retried.
2025-04-29 20:39:14.688 UTC [84] STATEMENT:  COMMIT
2025-04-29 20:39:14.869 UTC [173] ERROR:  could not serialize access due to read/write dependencies among transactions

flyrain

+1 on handle retry in another PR.

...n/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java

eric-maynard · 2025-04-30T18:57:25Z

...n/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java

@@ -143,12 +138,12 @@ private void persistEntity(
      @Nonnull PolarisCallContext callCtx,
      @Nonnull PolarisBaseEntity entity,
      PolarisBaseEntity originalEntity,
-      Statement statement)
+      CheckedFunction<String, Integer> fn)


Do we really need such a generic type here? Could this be updateFunction?

Are you suggesting making this always String, Integer ? or rename fn to updateFunction, I didn't made the name explicit updateFunction as we do both insert and update :P all DMLs. is kind of done statement.exceuteUpdate

I see. I didn't realize it's used for the different functions in Statement. Fundamentally I guess it's something like:

public class QueryAction { /** Returns the number of affected rows */ public int apply(String statement); }

But using a @FunctionalInterface makes sense. For the name, maybe we can just call it as queryConsumer or queryAction or something to make it clear

flyrain · 2025-04-30T19:12:23Z

.../src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/CheckedFunction.java

+@FunctionalInterface
+interface CheckedFunction<T, R> {
+  R apply(T t) throws SQLException;
+}


To be honest, I don't think it improves the code. It feels like a case of overengineering. Personally, I still prefer the if/else approach, it's simple, clear, and gets the job done. I'm okay with this though.

It seems like T is always String and R is always int?

+1 to @eric-maynard ' comment. Also, this interface can probably be an inner class in JdbcBasePersistenceImpl... it is not meant for sharing, is it?

I wanted this functional interface to be used across, in case such more use case comes hence i kept generics and moved its to its own class, if we want to address this for this particular use case then defining it within the JdbcBasePersistenceImpl makes sense and removing generic also make sense.

dimas-b

extracting the calls out of transaction (in the description) sounds confusing to me. As far as I can tell all DML in Postgres runs in a transaction. This PR merely changes the method of executing that transaction over the JDBC API.

dimas-b · 2025-04-30T19:34:56Z

.../src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/CheckedFunction.java

+@FunctionalInterface
+interface CheckedFunction<T, R> {
+  R apply(T t) throws SQLException;
+}


+1 to @eric-maynard ' comment. Also, this interface can probably be an inner class in JdbcBasePersistenceImpl... it is not meant for sharing, is it?

eric-maynard

LGTM, much simpler now

dimas-b · 2025-04-30T22:03:39Z

@singhpk234 : could you re-run the perf. tests on the final state of this PR and re-compare with main before this change?

singhpk234 requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995 and jbonofre as code owners April 29, 2025 18:56

github-project-automation bot added this to Basic Kanban Board Apr 29, 2025

singhpk234 requested review from vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners April 29, 2025 18:56

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Apr 29, 2025

flyrain previously approved these changes Apr 29, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Apr 29, 2025

dimas-b reviewed Apr 29, 2025

View reviewed changes

...n/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java Outdated Show resolved Hide resolved

...n/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java Outdated Show resolved Hide resolved

Remove transaction from atomic writes

577211c

singhpk234 dismissed flyrain’s stale review via 577211c April 29, 2025 20:57

singhpk234 force-pushed the feature/remove-transaction branch from 30ead8c to 577211c Compare April 29, 2025 20:57

singhpk234 requested a review from flyrain April 29, 2025 20:59

flyrain previously approved these changes Apr 29, 2025

View reviewed changes

dimas-b reviewed Apr 29, 2025

View reviewed changes

...n/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java Outdated Show resolved Hide resolved

singhpk234 changed the title ~~JDBC: Remove transaction from atomic writes~~ JDBC: Optimize writeEntity calls Apr 30, 2025

singhpk234 dismissed flyrain’s stale review via e6a4e89 April 30, 2025 16:50

singhpk234 force-pushed the feature/remove-transaction branch from e6a4e89 to 046233f Compare April 30, 2025 16:52

eric-maynard reviewed Apr 30, 2025

View reviewed changes

flyrain reviewed Apr 30, 2025

View reviewed changes

dimas-b previously approved these changes Apr 30, 2025

View reviewed changes

singhpk234 dismissed dimas-b’s stale review via 4f6b1cc April 30, 2025 21:03

singhpk234 force-pushed the feature/remove-transaction branch 2 times, most recently from 4f6b1cc to 4f105bd Compare April 30, 2025 21:15

eric-maynard previously approved these changes Apr 30, 2025

View reviewed changes

remove if-else

3e4ec1f

singhpk234 dismissed eric-maynard’s stale review via 3e4ec1f April 30, 2025 21:23

singhpk234 force-pushed the feature/remove-transaction branch from 4f105bd to 3e4ec1f Compare April 30, 2025 21:23

eric-maynard enabled auto-merge (squash) April 30, 2025 21:27

eric-maynard approved these changes Apr 30, 2025

View reviewed changes

eric-maynard merged commit d3b24d2 into apache:main Apr 30, 2025
6 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Apr 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDBC: Optimize writeEntity calls #1496

JDBC: Optimize writeEntity calls #1496

singhpk234 commented Apr 29, 2025 •

edited

Loading

dimas-b commented Apr 29, 2025

flyrain left a comment

singhpk234 commented Apr 29, 2025

flyrain left a comment

eric-maynard Apr 30, 2025

singhpk234 Apr 30, 2025

eric-maynard Apr 30, 2025

flyrain Apr 30, 2025 •

edited

Loading

eric-maynard Apr 30, 2025

dimas-b Apr 30, 2025

singhpk234 Apr 30, 2025

dimas-b left a comment

dimas-b Apr 30, 2025

eric-maynard left a comment

dimas-b commented Apr 30, 2025

JDBC: Optimize writeEntity calls #1496

JDBC: Optimize writeEntity calls #1496

Conversation

singhpk234 commented Apr 29, 2025 • edited Loading

About the change

Running Branchmark :

withRunInTransaction

without runInTransaction

dimas-b commented Apr 29, 2025

flyrain left a comment

Choose a reason for hiding this comment

singhpk234 commented Apr 29, 2025

flyrain left a comment

Choose a reason for hiding this comment

eric-maynard Apr 30, 2025

Choose a reason for hiding this comment

singhpk234 Apr 30, 2025

Choose a reason for hiding this comment

eric-maynard Apr 30, 2025

Choose a reason for hiding this comment

flyrain Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

eric-maynard Apr 30, 2025

Choose a reason for hiding this comment

dimas-b Apr 30, 2025

Choose a reason for hiding this comment

singhpk234 Apr 30, 2025

Choose a reason for hiding this comment

dimas-b left a comment

Choose a reason for hiding this comment

dimas-b Apr 30, 2025

Choose a reason for hiding this comment

eric-maynard left a comment

Choose a reason for hiding this comment

dimas-b commented Apr 30, 2025

singhpk234 commented Apr 29, 2025 •

edited

Loading

flyrain Apr 30, 2025 •

edited

Loading