Hash submitters in the deduplication key [DPP-347] #9417

kamil-da · 2021-04-14T16:53:58Z

Motivation

Submitting a command with a large number of submitters results in:

org.postgresql.util.PSQLException: ERROR: index row size 6512 exceeds maximum 2712 for index "participant_command_submissions_pkey"
  Hint: Values larger than 1/3 of a buffer page cannot be indexed.
Consider a function index of an MD5 hash of the value, or use full text indexing.

due to how we construct deduplication keys. Concatenating a large number of submitters causes exceeding Postgres limits.

Main changes

Made the deduplication key containing a hash of submitters instead of a concatenated string of submitters.

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

rautenrieth-da

Any change in com.daml.platform.store.dao needs to be mirrored in com.daml.platform.store.appendonlydao, can you please edit JdbcLedgerDao there accordingly?

rautenrieth-da · 2021-04-16T05:46:41Z

.../ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/ValueLimitsIT.scala

+        .update(
+          _.commands.deduplicationTime := deduplicationTime.asProtobuf
+        )
+      _ <- ledger.submit(request)


Shouldn't this test assert that the dummy contract was successfully created?

rautenrieth-da · 2021-04-16T05:51:29Z

.../ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/ValueLimitsIT.scala

+  /** Postgres has a limit on the index row size of 2712.
+    * We test that a large submitters number doesn't cause deduplication failure because of that limit.
+    * THIS TEST CASE DOES NOT TEST ANY OTHER FAILURES THAT MAY BE CAUSED BY A LARGE SUBMITTERS NUMBER
+    */


This description is not accurate:

The test uses the ledger API and not the internals of the indexer, so you don't really know what kind of failures it tests for.

The description mentions a limit of Postgres, but you don't know whether the test runs against a Postgres index database.

You are not checking any property of command deduplication, you are just submitting a request with a large number of parties. I would name the test "Submit commands with a large number of submitters".

@rautenrieth-da Thank you for the review!
I've modified the test suite according to your suggestions. I've also added a change to a database index that was blocking submissions due to index row size limits.

rautenrieth-da

Left some minor comments. You still need to do the change in com.daml.platform.store.appendonlydao though.

rautenrieth-da · 2021-04-19T08:07:24Z

.../ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/ValueLimitsIT.scala

+      _ <- ledger.submit(request)
+      _ <- Delayed.by(1.second)(())


Suggested change

_ <- ledger.submit(request)

_ <- Delayed.by(1.second)(())

_ <- ledger.submitAndWait(request)

rautenrieth-da · 2021-04-19T08:09:58Z

.../ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/ValueLimitsIT.scala

+          commands = DummyWithAnnotation(parties.head, "First submission").create.command,
+        )
+        .update(
+          _.commands.deduplicationTime := deduplicationTime.asProtobuf


Why do you need to set a custom deduplication time?

…ures

CHANGELOG_BEGIN - [Integration Kit] - deduplication key will contain hashed submitters instead of concatenated submitters CHANGELOG_END

Do not include submitters in the index to avoid issues with index row size limit

The suite will be restored when database index fixes for large submitters sets are implemented

kamil-da · 2021-04-19T18:13:26Z

@rautenrieth-da I've adapted the PR according to your review and the discussion during the meeting.

implemented the feature for appendonlydao package
removed changes to the database index - will come in a follow-up PR
removed ValueLimitsIT suite - will come in a follow-up PR

For this PR I've replaced API-level testing with simple unit testing because it's currently impossible to make API-level tests pass without making changes to database indexes.

rautenrieth-da

Thank you for deduplicating the deduplication logic 🙂

rautenrieth-da · 2021-04-20T08:12:52Z

...pant-integration-api/src/test/suite/scala/platform/store/dao/DeduplicationKeyMakerSpec.scala

+
+    "make a deduplication key with a limited length for a large number of submitters" in {
+      val submitters = (1 to 50).map(_ => aParty()).toList
+      val MaxKeyLength = 200


This constant seems arbitrary, but the value is fine. It's smaller than the maximum length for indexed string columns in both Postgres and Oracle (which was the reason for this change). Maybe add a comment?

Good point - I'll comment on motivation for this limit.

@remyhaemmerle-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @remyhaemmerle-da is in charge of this release. Commit log: ``` cf2be78 Support rollback nodes in BlindingInfo (#9445) adde90b KV: do not use "Effects" part of InputsAndEffects (#9433) 75fa86a Additional metrics for mutable contract state cache (#9444) d8c34a4 Rename normalization-related params in the integrity checker config (#9455) e335244 Hash submitters in the deduplication key [DPP-347] (#9417) 43ffa42 Test for duplicate contracts when querying on behalf of multiple parties (#9443) 7644844 Document daml ledger export script (#9446) 3193027 DPP-334 Manually partition the event table (#9394) 14661a8 Clearer variable name for subtype evidence (#9442) 40c85d8 Release v1.13.0-snapshot.20210419.6730.0.8c3a8c04 (#9439) 1cb907c KV: do not use "Inputs" part of InputsAndEffects (#9429) ``` Changelog: ``` - [Integration Kit] - deduplication key will contain hashed submitters instead of concatenated submitters * [Daml export] Refer to the "Ledger Export" chapter under the "Early Access Features" for a description of the new Daml ledger export command. This is an early access feature. ``` CHANGELOG_BEGIN CHANGELOG_END

kamil-da marked this pull request as ready for review April 15, 2021 15:09

kamil-da requested review from gerolf-da, mziolekda, nmarton-da, rautenrieth-da and tudor-da as code owners April 15, 2021 15:09

rautenrieth-da requested changes Apr 16, 2021

View reviewed changes

rautenrieth-da requested changes Apr 19, 2021

View reviewed changes

kamil-da added 13 commits April 19, 2021 11:34

A ledger-api-test-tool test case for too large deduplication key fail…

507d242

…ures

Hashing submitters in the deduplication key

b30b098

CHANGELOG_BEGIN - [Integration Kit] - deduplication key will contain hashed submitters instead of concatenated submitters CHANGELOG_END

Separate ValueLimitsIT test suite for the ledger-api-test-tool

2d2cc4b

Excluded ValueLimitsIT for sandbox and ledger-on-sql conformance tests

8ea6c8e

Checking contract creation result in the test case

3c33fcf

Update index on participant_command_completions table

9ef8dcb

Do not include submitters in the index to avoid issues with index row size limit

Added a delay for more consistent results

1ecaf9c

Fixed submitting a command

0b579a6

Simplified the test case

cd49103

Reverted changing indexes

3008af9

Dedicated object for making deduplication keys

5b0fae8

Ported hashing deduplication keys to the appendonlydao

4883383

Removed ValueLimitIT suite.

368b6d6

The suite will be restored when database index fixes for large submitters sets are implemented

kamil-da force-pushed the kamil-da/DPP-347-hash-deduplication-key branch from f79025e to 368b6d6 Compare April 19, 2021 18:09

rautenrieth-da approved these changes Apr 20, 2021

View reviewed changes

Additional unit test for hashing deduplication keys

3bf7912

kamil-da merged commit e335244 into main Apr 20, 2021

kamil-da deleted the kamil-da/DPP-347-hash-deduplication-key branch April 20, 2021 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hash submitters in the deduplication key [DPP-347] #9417

Hash submitters in the deduplication key [DPP-347] #9417

kamil-da commented Apr 14, 2021 •

edited

Loading

rautenrieth-da left a comment

rautenrieth-da Apr 16, 2021

kamil-da Apr 16, 2021

rautenrieth-da Apr 16, 2021

kamil-da Apr 16, 2021

rautenrieth-da left a comment

rautenrieth-da Apr 19, 2021

rautenrieth-da Apr 19, 2021

kamil-da commented Apr 19, 2021

rautenrieth-da left a comment

rautenrieth-da Apr 20, 2021

kamil-da Apr 20, 2021

	_ <- ledger.submit(request)
	_ <- Delayed.by(1.second)(())
	_ <- ledger.submitAndWait(request)

Hash submitters in the deduplication key [DPP-347] #9417

Hash submitters in the deduplication key [DPP-347] #9417

Conversation

kamil-da commented Apr 14, 2021 • edited Loading

Motivation

Main changes

Pull Request Checklist

rautenrieth-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rautenrieth-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamil-da commented Apr 19, 2021

rautenrieth-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamil-da commented Apr 14, 2021 •

edited

Loading