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

add incarnation to transcriptStore spans and items #7482

Closed
warner opened this issue Apr 22, 2023 · 4 comments · Fixed by #7506
Closed

add incarnation to transcriptStore spans and items #7482

warner opened this issue Apr 22, 2023 · 4 comments · Fixed by #7506
Assignees
Labels
enhancement New feature or request SwingSet package: SwingSet vaults_triage DO NOT USE
Milestone

Comments

@warner
Copy link
Member

warner commented Apr 22, 2023

What is the Problem Being Solved?

Our vat lifetimes are first broken up into "incarnations", separated by upgrades. When the vat is first created, we're in incarnation = 0. The first E(adminFacet).upgrade() that happens to the vat changes that to incarnation = 1, etc.

Note that this is a larger unit than a "span". Each time we write out a heap snapshot, we start a new span. There will be multiple spans within a single incarnation, if the vat worker uses heap snapshots, and it receives enough deliveries to justify a snapshot.

The new incarnation number is visible to userspace in the result of the upgrade() call ({ incarnatonNumber }). The old value appears in the disconnectObject with which all the old incarnation's outstanding promises are rejected: { name: 'vatUpgraded', upgradeMessage, incarnationNumber }.

A month-ish ago we realized that it would be nice if this value were also in the database. We'd like to be able to quickly extract all the transcript items from a single incarnation, and currently there is no convenient way to learn where the upgrade points were. In a pinch, you could scan the whole transcript for startVat deliveries, and then deduce that the upgrade must have happened about the same time, but we'd prefer something more deliberate/accurate, and which wouldn't require additional queries.

Description of the Design

So we'd like to change the transcriptItems table from:

    CREATE TABLE IF NOT EXISTS transcriptItems (
      vatID TEXT,
      position INTEGER,
      item TEXT,
      PRIMARY KEY (vatID, position)
    )

to:

    CREATE TABLE IF NOT EXISTS transcriptItems (
      vatID TEXT,
      incarnation INTEGER,
      position INTEGER,
      item TEXT,
      PRIMARY KEY (vatID, position)
    )

and the transcriptSpans table from:

    CREATE TABLE IF NOT EXISTS transcriptSpans (
      vatID TEXT,
      startPos INTEGER, -- inclusive
      endPos INTEGER, -- exclusive
      hash TEXT, -- cumulative hash of this item and previous cumulative hash
      isCurrent INTEGER CHECK (isCurrent = 1),
      PRIMARY KEY (vatID, startPos),
      UNIQUE (vatID, isCurrent)
    )

to:

    CREATE TABLE IF NOT EXISTS transcriptSpans (
      vatID TEXT,
      incarnation INTEGER,
      startPos INTEGER, -- inclusive
      endPos INTEGER, -- exclusive
      hash TEXT, -- cumulative hash of this item and previous cumulative hash
      isCurrent INTEGER CHECK (isCurrent = 1),
      PRIMARY KEY (vatID, startPos),
      UNIQUE (vatID, isCurrent)
    )

plus we should add an index to support querying by vatid+incarnation:

CREATE INDEX IF NOT EXISTS name
 ON transcriptItems (vatID, incarnation, position);
CREATE INDEX IF NOT EXISTS name
 ON transcriptSpans (vatID, incarnation, startPos);

The incarnation number should be initialized to 0 during initTranscript, adding it to the initial span record. Each time we add a new transcript entry (addItem()), we should fetch the incarnation from the current span and include it in the new transcriptItems row. It might be more efficient to do this with SQL query syntax than with multiple queries, or maybe getCurrentSpanBounds() should return the current incarnation along with everything else, and then include it in the call to sqlAddItem.

We should add a boolean rolloverIncarnation argument to rolloverSpan, and increase the incarnation number iff that argument is true. So the old span will be the last span of the old incarnation, and the new span record will use the next higher incarnation number. We'll call rolloverSpan(vatID, true) from vatKeeper.dropSnapshotAndResetTranscript(), which is called from vat-warehouse resetWorker(), which is called from kernel.js processUpgradeVat(). These functions should return the new incarnation number.

vatKeeper.js > getIncarnationNumber currently tracks this in the kvStore. This should change to track it in the DB, getting it from transcriptStore.getCurrentSpanBounds(). VatKeeper's incIncarnationNumber should be removed. The kernel.js code that called it (from processUpgradeVat) should be removed, and replaced with the return value from resetWorker.

Security Considerations

None, we're just adding an extra counter.

Scaling Considerations

None, the additional data is minimal. This will make it easier to delete old transcript items while retaining the ones needed for a sleeper-agent only-since-last-upgrade kind of upgrade, if/when we want to do that.

Release Priority Considerations

DB schema changes are always easier to make before deployment than after. This is a low-risk change that is highly likely to help us out in the future, and @FUDCo has some spare time, so we think we can get it landed before the release.

Test Plan

Unit tests in swing-store which call rolloverSpan with both true and false, followed by addItem and getCurrentSpanBounds() calls to check that the incarnation number is as expected.

Confirm that the incarnation numbers that appear in the vat-upgrade tests match the ones in the DB, probably by having test/upgrade/test-upgrade.js snoop the transcriptStore.getCurrentSpanBounds() before/after the upgrade.

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Apr 22, 2023
@warner warner added this to the Vaults EVP milestone Apr 22, 2023
@mhofman
Copy link
Member

mhofman commented Apr 23, 2023

I think we can get away with not including the incarnation number in the transcript items table. I am not aware of any case where we query the items directly without first looking at bounds in the spans table.

@warner
Copy link
Member Author

warner commented Apr 23, 2023

hm, true. I kinda like the idea of having them there, because it could make some future queries simpler to express. But it does add a tiny bit of complication to the addItem call.

@ivanlei ivanlei added the vaults_triage DO NOT USE label Apr 24, 2023
@FUDCo
Copy link
Contributor

FUDCo commented Apr 24, 2023

I have a couple of nits to pick with this design.

I think we should omit the index -- queries based on incarnation are going to be relatively rare and when they happen they will be in contexts that are not particularly performance sensitive. However, with an explicit index we'd be paying the cost of maintaining/updating that index on every database write to the transcript. If, in the highly unlikely event that we are experiencing some kind of perf bottleneck due to frequent incarnation-entangled queries, we can just add the index then, since doing so would be a transparent and backwards compatible change.

Second, I don't think it's a win to remove management of the incarnation number from the vatKeeper. All the logic there is proven and debugged, and kvStore is exactly the right place to be keeping one-off specialized values such as the incarnation counter. Decisions about bumping the incarnation number necessarily have to happen outside the swingStore anyway, since upgrades are initiated exogenously. The comment that

This should change to track it in the DB

is a bit misleading since the kvStore is in the DB. Replacing the current vatKeeper mechanism with something internal to the swingStore would add a bunch of complication with no particular compensating benefit.

@warner
Copy link
Member Author

warner commented Apr 24, 2023

Chatting with @FUDCo now:

  • ok, let's omit the index
  • I convinced him to let transcriptStore own the incarnation number
    • vatKeeper.getIncarnationNumber(), used only by kernel.js processVatUpgrade, can do transcriptStore.getCurrentSpanInfo().incarnation
    • vatKeeper.dropSnapshotAndResetTranscript gets renamed to something like startNewIncarnation, and returns the new incarnation number
      • we add transcriptStore.rolloverIncarnation(vatID) instead of a boolean "also roll the incarnation" flag to rolloverSpan(), to be more self-documenting
    • vatWarehouse.resetWorker (which calls that) might get renamed too, and also returns the new incarnation number
    • processVatUpgrade stops calling vatKeeper.incIncarnationNumber, relying on startNewIncarnation doing the bump
  • we take advantage of this opportunity to make incarnation numbers start at 0, instead of the 1 established by initializeVatState writing it into the kvstore
    • remove vatID.incarnationNumber from the kvStore

Hm, not necessarily as part of this task, but we might consider removing vatID.nextDeliveryNum from the kvstore too, and leaning further into having transcriptStore own that. That would change the implementation of vatKeeper nextDeliveryNum to use a getCurrentSpanInfo too, and would further reduce duplication of transcript counter state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SwingSet package: SwingSet vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants