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

[Divulgence pruning] Prune immediate divulgence [DPP-513] #10691

Merged
merged 5 commits into from
Aug 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,11 @@ class ParticipantPruningIT extends LedgerTestSuite {
divulgence.exerciseCanFetch(_, contract),
)

_ <- pruneAtCurrentOffset(participant, bob, pruneAllDivulgedContracts = false)
_ <- pruneAtCurrentOffset(
participant,
bob,
pruneAllDivulgedContracts = false,
)

// Bob can still see the divulged contract
_ <- participant.exerciseAndGetContract[Dummy](
Expand All @@ -562,7 +566,11 @@ class ParticipantPruningIT extends LedgerTestSuite {
// Archive the divulged contract
_ <- participant.exercise(alice, contract.exerciseArchive)

_ <- pruneAtCurrentOffset(participant, bob, pruneAllDivulgedContracts = false)
_ <- pruneAtCurrentOffset(
participant,
bob,
pruneAllDivulgedContracts = false,
)

_ <- participant
.exerciseAndGetContract[Dummy](
Expand Down Expand Up @@ -595,15 +603,7 @@ class ParticipantPruningIT extends LedgerTestSuite {
divulgence.exerciseDivulge(_, contract),
)

_ <- divulgencePruneAndCheck(
alice,
bob,
alpha,
beta,
contract,
divulgence,
disclosureVisibility = false,
)
_ <- divulgencePruneAndCheck(alice, bob, alpha, beta, contract, divulgence)
} yield ()
})

Expand All @@ -624,15 +624,7 @@ class ParticipantPruningIT extends LedgerTestSuite {
divulgeNotDiscloseTemplate.exerciseDivulgeNoDisclose(_, divulgence),
)

_ <- divulgencePruneAndCheck(
alice,
bob,
alpha,
beta,
contract,
divulgence,
disclosureVisibility = false,
)
_ <- divulgencePruneAndCheck(alice, bob, alpha, beta, contract, divulgence)
} yield ()
})

Expand All @@ -649,15 +641,8 @@ class ParticipantPruningIT extends LedgerTestSuite {
alice,
divulgence.exerciseCreateAndDisclose,
)
_ <- divulgencePruneAndCheck(
alice,
bob,
alpha,
beta,
contract,
divulgence,
disclosureVisibility = true,
)

_ <- divulgencePruneAndCheck(alice, bob, alpha, beta, contract, divulgence)
} yield ()
})

Expand All @@ -679,8 +664,6 @@ class ParticipantPruningIT extends LedgerTestSuite {
beta: ParticipantTestContext,
contract: Primitive.ContractId[Contract],
divulgence: binding.Primitive.ContractId[Divulgence],
// TODO Divulgence pruning: Remove when immediate divulgence pruning is implemented
disclosureVisibility: Boolean,
)(implicit ec: ExecutionContext) =
for {
// Check that Bob can fetch the contract
Expand Down Expand Up @@ -715,24 +698,12 @@ class ParticipantPruningIT extends LedgerTestSuite {

_ <- pruneAtCurrentOffset(beta, bob, pruneAllDivulgedContracts = true)

// TODO Divulgence pruning: Check ACS equality before and after pruning
// TODO Divulgence pruning: Remove the true-clause of the if-statement below
// when disclosure pruning is implemented.
_ <-
if (disclosureVisibility) {
beta
.exerciseAndGetContract[Dummy](
bob,
divulgence.exerciseCanFetch(_, contract),
)
} else {
beta
.exerciseAndGetContract[Dummy](
bob,
divulgence.exerciseCanFetch(_, contract),
)
.mustFail("Bob cannot access the divulged contract after the second pruning")
}
_ <- beta
.exerciseAndGetContract[Dummy](
bob,
divulgence.exerciseCanFetch(_, contract),
)
.mustFail("Bob cannot access the divulged contract after the second pruning")
} yield ()

private def populateLedgerAndGetOffsets(participant: ParticipantTestContext, submitter: Party)(
Expand Down Expand Up @@ -783,13 +754,22 @@ class ParticipantPruningIT extends LedgerTestSuite {

private def pruneAtCurrentOffset(
participant: ParticipantTestContext,
party: Party,
localParty: Party,
pruneAllDivulgedContracts: Boolean,
)(implicit ec: ExecutionContext): Future[Unit] =
for {
offset <- participant.currentEnd()
// Dummy needed to prune at this offset
_ <- participant.create(party, Dummy(party))
_ <- participant.create(localParty, Dummy(localParty))

acsBeforePruning <- participant.activeContracts(localParty)
_ <- participant.prune(offset, pruneAllDivulgedContracts = pruneAllDivulgedContracts)
} yield ()
acsAfterPruning <- participant.activeContracts(localParty)

} yield {
assert(
acsBeforePruning == acsAfterPruning,
s"Active contract set comparison before and after pruning failed: $acsBeforePruning vs $acsAfterPruning",
)
}
}
2 changes: 1 addition & 1 deletion ledger/ledger-on-memory/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ conformance_test(
"--verbose",
"--include=ParticipantPruningIT",
# Disable tests targeting only append-only schema functionality
"--exclude=ParticipantPruningIT:PRLocalAndNonLocalRetroactiveDivulgences,ParticipantPruningIT:PRRetroactiveDivulgences",
"--exclude=ParticipantPruningIT:PRLocalAndNonLocalRetroactiveDivulgences,ParticipantPruningIT:PRRetroactiveDivulgences,ParticipantPruningIT:PRDisclosureAndRetroactiveDivulgence",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normalization of these conformance test will be done in a subsequent PR. For clarity/conciseness, in this PR, I blacklist the tests that cannot run on single-participant setups.

],
)

Expand Down
10 changes: 8 additions & 2 deletions ledger/ledger-on-sql/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ da_scala_test_suite(
"--verbose",
"--include=ParticipantPruningIT",
# Disable tests targeting only append-only schema functionality
"--exclude=ParticipantPruningIT:PRLocalAndNonLocalRetroactiveDivulgences,ParticipantPruningIT:PRRetroactiveDivulgences",
"--exclude=ParticipantPruningIT:PRLocalAndNonLocalRetroactiveDivulgences,ParticipantPruningIT:PRRetroactiveDivulgences,ParticipantPruningIT:PRDisclosureAndRetroactiveDivulgence",
],
),
conformance_test(
Expand Down Expand Up @@ -441,6 +441,8 @@ conformance_test(
test_tool_args = [
"--verbose",
"--include=ParticipantPruningIT",
# Disable tests targeting only multi-participant setups
"--exclude=ParticipantPruningIT:PRDisclosureAndRetroactiveDivulgence",
],
)

Expand All @@ -459,6 +461,8 @@ conformance_test(
test_tool_args = [
"--verbose",
"--include=ParticipantPruningIT",
# Disable tests targeting only multi-participant setups
"--exclude=ParticipantPruningIT:PRDisclosureAndRetroactiveDivulgence",
],
)

Expand All @@ -477,6 +481,8 @@ conformance_test(
test_tool_args = [
"--verbose",
"--include=ParticipantPruningIT",
# Disable tests targeting only multi-participant setups
"--exclude=ParticipantPruningIT:PRDisclosureAndRetroactiveDivulgence",
],
)

Expand All @@ -493,7 +499,7 @@ conformance_test(
"--verbose",
"--include=ParticipantPruningIT",
# Disable tests targeting only append-only schema functionality
"--exclude=ParticipantPruningIT:PRLocalAndNonLocalRetroactiveDivulgences,ParticipantPruningIT:PRRetroactiveDivulgences",
"--exclude=ParticipantPruningIT:PRLocalAndNonLocalRetroactiveDivulgences,ParticipantPruningIT:PRRetroactiveDivulgences,ParticipantPruningIT:PRDisclosureAndRetroactiveDivulgence",
],
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import scalaz.syntax.tag._
private[backend] trait CommonStorageBackend[DB_BATCH] extends StorageBackend[DB_BATCH] {

private val logger: ContextualizedLogger = ContextualizedLogger.get(this.getClass)

// Ingestion

private val SQL_DELETE_OVERSPILL_ENTRIES: List[SqlQuery] =
Expand All @@ -55,6 +54,8 @@ private[backend] trait CommonStorageBackend[DB_BATCH] extends StorageBackend[DB_
SQL("DELETE FROM party_entries WHERE ledger_offset > {ledger_offset}"),
)

def queryStrategy: QueryStrategy

override def initializeIngestion(
connection: Connection
): Option[ParameterStorageBackend.LedgerEnd] = {
Expand Down Expand Up @@ -479,6 +480,7 @@ private[backend] trait CommonStorageBackend[DB_BATCH] extends StorageBackend[DB_

// Events

// TODO Cleanup: Extract to [[EventStorageBackendTemplate]]
def pruneEvents(
pruneUpToInclusive: Offset,
pruneAllDivulgedContracts: Boolean,
Expand Down Expand Up @@ -524,6 +526,35 @@ private[backend] trait CommonStorageBackend[DB_BATCH] extends StorageBackend[DB_
)"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Event pruning would be probably more well suited in the EventStorageBackendTemplate, especiallly since it using templates now. (if you are under time pressure never mind, we can do it in the cleanup epic)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I'll add a TODO to it and please let's leave it for the cleanup epic.

}(connection, loggingContext)

if (pruneAllDivulgedContracts) {
tudor-da marked this conversation as resolved.
Show resolved Hide resolved
val pruneAfterClause = {
// We need to distinguish between the two cases since lexicographical comparison
// in Oracle doesn't work with '' (empty strings are treated as NULLs) as one of the operands
participantAllDivulgedContractsPrunedUpToInclusive(connection) match {
case Some(pruneAfter) => cSQL"and event_offset > $pruneAfter"
case None => cSQL""
}
}

pruneWithLogging(queryDescription = "Immediate divulgence events pruning") {
SQL"""
-- Immediate divulgence pruning
delete from participant_events_create c
where event_offset <= $pruneUpToInclusive
-- Only prune create events which did not have a locally hosted party before their creation offset
and not exists (
select 1
from party_entries p
where p.typ = 'accept'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this the issue with the string comparison? if yes, would be nice to add a short comment that without this some fields got empty which lead to oracle issues etcetcetc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are referring to the Oracle-specific blocker that I've mentioned offline, it was the comparison participant_all_divulged_contracts_pruned_up_to_inclusive (which was nullable/empty-stringed) to the event_offset. And in Oracle '' does not compare lexicogrphically as expected.

Copy link
Contributor Author

@tudor-da tudor-da Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which makes me wonder: this is a very common comparison that we're doing. howcome it did not pop out in other places 🤔 ?

and p.ledger_offset <= c.event_offset
and #${queryStrategy.isTrue("p.is_local")}
and #${queryStrategy.arrayContains("c.flat_event_witnesses", "p.party")}
)
$pruneAfterClause
"""
}(connection, loggingContext)
}

pruneWithLogging(queryDescription = "Exercise (consuming) events pruning") {
SQL"""
-- Exercise events (consuming)
Expand All @@ -541,6 +572,14 @@ private[backend] trait CommonStorageBackend[DB_BATCH] extends StorageBackend[DB_
}(connection, loggingContext)
}

private def participantAllDivulgedContractsPrunedUpToInclusive(
connection: Connection
): Option[Offset] =
SQL"select participant_all_divulged_contracts_pruned_up_to_inclusive from parameters"
.as(offset("participant_all_divulged_contracts_pruned_up_to_inclusive").?.single)(
connection
)

private def pruneWithLogging(queryDescription: String)(query: SimpleSql[Row])(
connection: Connection,
loggingContext: LoggingContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,12 @@ trait QueryStrategy {
* @return the function name
*/
def booleanOrAggregationFunction: String = "bool_or"

/** Predicate which tests if the element referenced by the `elementColumnName`
* is in the array from column `arrayColumnName`
*/
def arrayContains(arrayColumnName: String, elementColumnName: String): String

/** Boolean predicate */
def isTrue(booleanColumnName: String): String
}
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ private[backend] object H2StorageBackend
.map(p => cSQL"array_contains(#$columnName, '#${p.toString}')")
.mkComposite("(", " or ", ")")

override def arrayContains(arrayColumnName: String, elementColumnName: String): String =
s"array_contains($arrayColumnName, $elementColumnName)"

override def isTrue(booleanColumnName: String): String = booleanColumnName
}

override def queryStrategy: QueryStrategy = H2QueryStrategy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ private[backend] object OracleStorageBackend
s"""case when ($column = $value) then 1 else 0 end"""

override def booleanOrAggregationFunction: String = "max"

override def arrayContains(arrayColumnName: String, elementColumnName: String): String =
s"EXISTS (SELECT 1 FROM JSON_TABLE($arrayColumnName, '$$[*]' columns (value PATH '$$')) WHERE value = $elementColumnName)"

override def isTrue(booleanColumnName: String): String = s"$booleanColumnName = 1"
}

override def queryStrategy: QueryStrategy = OracleQueryStrategy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ private[backend] object PostgresStorageBackend
cSQL"#$columnName::text[] && $partiesArray::text[]"
}

override def arrayContains(arrayColumnName: String, elementColumnName: String): String =
s"$elementColumnName = any($arrayColumnName)"

override def isTrue(booleanColumnName: String): String = booleanColumnName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about defining this in QueryStrategy itself and overriding only in Oracle?

Copy link
Contributor Author

@tudor-da tudor-da Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm bearish on overriding, in general, and here, for two lines shaved, I would keep them clear. Traversing inheritance levels for understanding functionality is not my piece of cake. I'd change it if you think this degrades coherence in the implementation, though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we have to override in both Oracle and H2 when defining in QueryStrategy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you were probably talking only about isTrue() @nmarton-da . Sorry, I thought about both isTrue() and arrayContains().

Copy link
Contributor Author

@tudor-da tudor-da Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nmarton-da I'll merge this as it is. If we'll reach a different conclusion on this topic, I'll amend it in a separate PR.

}

override def queryStrategy: QueryStrategy = PostgresQueryStrategy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ private[backend] object StorageBackendTestValues {
def dtoPartyEntry(
offset: Offset,
party: String = someParty,
isLocal: Boolean = true,
): DbDto.PartyEntry = DbDto.PartyEntry(
ledger_offset = offset.toHexString,
recorded_at = someTime,
Expand All @@ -78,7 +79,7 @@ private[backend] object StorageBackendTestValues {
display_name = Some(party),
typ = JdbcLedgerDao.acceptType,
rejection_reason = None,
is_local = Some(true),
is_local = Some(isLocal),
)

def dtoPackage(offset: Offset): DbDto.Package = DbDto.Package(
Expand Down
Loading