Make it able to ignore WAL related VersionEdits in older versions #7873

ghost · 2021-01-16T06:49:26Z

Although the tags for WalAddition, WalDeletion are after kTagSafeIgnoreMask, to actually be able to skip these entries in older versions of RocksDB, we require that they are encoded with their encoded size as the prefix. This requirement is not met in the current codebase, so a downgraded DB may fail to open if these entries exist in the MANIFEST.

If a DB wants to downgrade, and its MANIFEST contains WalAddition or WalDeletion, it can set track_and_verify_wals_in_manifest to false, then restart twice, then downgrade. On the first restart, a new MANIFEST will be created with a WalDeletion indicating that all previously tracked WALs are removed from MANIFEST. On the second restart, since there is no tracked WALs in MANIFEST now, a new MANIFEST will be created with neither WalAddition nor WalDeletion. Then the DB can downgrade.

Tags for BlobFileAddition, BlobFileGarbage also have the same problem, but this PR focuses on solving the problem for WAL edits.

Test Plan:
Added a VersionEditTest::IgnorableTags unit test to verify all entries with tags larger than kTagSafeIgnoreMask can actually be skipped and won't affect parsing of other entries.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ghost · 2021-01-16T07:33:39Z

db/version_edit.cc

@@ -217,22 +217,30 @@ bool VersionEdit::EncodeTo(std::string* dst) const {

  for (const auto& blob_file_addition : blob_file_additions_) {
    PutVarint32(dst, kBlobFileAddition);
-    blob_file_addition.EncodeTo(dst);
+    std::string encoded;


@ltamasi This breaks backward compatibility. But without doing so, users of BlobDB cannot downgrade. What do you think?

I'll remove changes related to BlobDB from this PR to make it focus on WAL edits.

@ltamasi This breaks backward compatibility. But without doing so, users of BlobDB cannot downgrade. What do you think?

Nice catch @Cheng-Chang, and thanks for letting me know! In hindsight, the blob file related tags shouldn't have been put in the "ignorable" range since if you have blob files and downgrade to a version that cannot read these, you won't be able to read your data. Fortunately, since these are related to the integrated BlobDB project (which is WIP), we can still move these to the "non-ignorable" range. I'll take care of that.

facebook-github-bot · 2021-01-16T07:37:32Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

anand1976 · 2021-01-16T23:16:37Z

As @ltamasi points out for the BlobDB entries, wouldn't this change cause problems when upgrading? Maybe we could introduce new tags with the correct format, and retain the existing ones to be able to decode?

ghost · 2021-01-19T20:17:40Z

As @ltamasi points out for the BlobDB entries, wouldn't this change cause problems when upgrading? Maybe we could introduce new tags with the correct format, and retain the existing ones to be able to decode?

Then when we do the downgrade to a version older than 6.15, it still fails to be decoded unless we restart DB twice to remove the entries. I'm thinking maybe we can fix the format here. During upgrade, if there is decoding issue, the user can restart DB twice to get rid of the existing WAL related entries, and then upgrade.

facebook-github-bot · 2021-01-19T20:52:27Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-01-19T21:02:00Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr

Should we be pessimistic and call them "kWal{Addition,Deletion}2"? It also helps since "new" is ambiguous -- in the case of kNewFile, "new" means the file is being added, I think.

ajkr · 2021-01-19T23:46:06Z

Also can we have a "bug fixes" release note that mentions the downgrade instructions for users who ran 6.15.1 with WAL tracking enabled?

facebook-github-bot · 2021-01-19T23:50:55Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

ghost · 2021-01-19T23:52:09Z

Also can we have a "bug fixes" release note that mentions the downgrade instructions for users who ran 6.15.1 with WAL tracking enabled?

The wal tracking is only released in 6.16. In 6.15, there is a comment on the option indicating that it's not ready for use yet. MyRocks accidentally enabled it in 6.15.

I think we'll mention this in 6.16.1's release note.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr · 2021-01-20T00:11:57Z

MyRocks accidentally enabled it in 6.15.

Got it, I missed this detail earlier, ignore my suggestion then here and questions in group chat

facebook-github-bot · 2021-01-20T03:28:08Z

@Cheng-Chang merged this pull request in e449482.

) Summary: Although the tags for `WalAddition`, `WalDeletion` are after `kTagSafeIgnoreMask`, to actually be able to skip these entries in older versions of RocksDB, we require that they are encoded with their encoded size as the prefix. This requirement is not met in the current codebase, so a downgraded DB may fail to open if these entries exist in the MANIFEST. If a DB wants to downgrade, and its MANIFEST contains `WalAddition` or `WalDeletion`, it can set `track_and_verify_wals_in_manifest` to `false`, then restart twice, then downgrade. On the first restart, a new MANIFEST will be created with a `WalDeletion` indicating that all previously tracked WALs are removed from MANIFEST. On the second restart, since there is no tracked WALs in MANIFEST now, a new MANIFEST will be created with neither `WalAddition` nor `WalDeletion`. Then the DB can downgrade. Tags for `BlobFileAddition`, `BlobFileGarbage` also have the same problem, but this PR focuses on solving the problem for WAL edits. Pull Request resolved: #7873 Test Plan: Added a `VersionEditTest::IgnorableTags` unit test to verify all entries with tags larger than `kTagSafeIgnoreMask` can actually be skipped and won't affect parsing of other entries. Reviewed By: ajkr Differential Revision: D25935930 Pulled By: cheng-chang fbshipit-source-id: 7a02fdba4311d6084328c14aed110a26d08c3efb

ajkr · 2021-01-20T17:58:01Z

I think we'll mention this in 6.16.1's release note.

It makes sense. I hope the release note will appear on master branch as well, so that it's clear upgrading from 6.16.0 to a newer major/minor version also gets the fix, not just upgrading from 6.16.0 to 6.16.1 or a later patch version.

ltamasi · 2021-01-20T18:28:35Z

I think we'll mention this in 6.16.1's release note.

It makes sense. I hope the release note will appear on master branch as well, so that it's clear upgrading from 6.16.0 to a newer major/minor version also gets the fix, not just upgrading from 6.16.0 to 6.16.1 or a later patch version.

@Cheng-Chang Seems to me you've also cherry-picked this into 6.17, so I think we should also call it out in the 6.17.0 section of the changelog (on master and 6.17.fb as well).

…cebook#7873) Summary: Although the tags for `WalAddition`, `WalDeletion` are after `kTagSafeIgnoreMask`, to actually be able to skip these entries in older versions of RocksDB, we require that they are encoded with their encoded size as the prefix. This requirement is not met in the current codebase, so a downgraded DB may fail to open if these entries exist in the MANIFEST. If a DB wants to downgrade, and its MANIFEST contains `WalAddition` or `WalDeletion`, it can set `track_and_verify_wals_in_manifest` to `false`, then restart twice, then downgrade. On the first restart, a new MANIFEST will be created with a `WalDeletion` indicating that all previously tracked WALs are removed from MANIFEST. On the second restart, since there is no tracked WALs in MANIFEST now, a new MANIFEST will be created with neither `WalAddition` nor `WalDeletion`. Then the DB can downgrade. Tags for `BlobFileAddition`, `BlobFileGarbage` also have the same problem, but this PR focuses on solving the problem for WAL edits. Pull Request resolved: facebook#7873 Test Plan: Added a `VersionEditTest::IgnorableTags` unit test to verify all entries with tags larger than `kTagSafeIgnoreMask` can actually be skipped and won't affect parsing of other entries. Reviewed By: ajkr Differential Revision: D25935930 Pulled By: cheng-chang fbshipit-source-id: 7a02fdba4311d6084328c14aed110a26d08c3efb

facebook-github-bot added the CLA Signed label Jan 16, 2021

ghost requested review from ltamasi and anand1976 January 16, 2021 06:49

be able to ignore blob and WAL related VersionEdits in older versions

c009190

facebook-github-bot reviewed Jan 16, 2021

View reviewed changes

ghost commented Jan 16, 2021

View reviewed changes

ghost changed the title ~~Make it able to ignore blob and WAL related VersionEdits in older versions~~ Make it able to ignore WAL related VersionEdits in older versions Jan 16, 2021

remove changes to blob related edits

1a2f02e

use new tags

65f6c2e

facebook-github-bot reviewed Jan 19, 2021

View reviewed changes

remove printf

b76199d

facebook-github-bot reviewed Jan 19, 2021

View reviewed changes

ajkr approved these changes Jan 19, 2021

View reviewed changes

anand1976 approved these changes Jan 19, 2021

View reviewed changes

rename tags

160f99f

facebook-github-bot reviewed Jan 19, 2021

View reviewed changes

facebook-github-bot closed this in e449482 Jan 20, 2021

facebook-github-bot added the Merged label Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it able to ignore WAL related VersionEdits in older versions #7873

Make it able to ignore WAL related VersionEdits in older versions #7873

ghost commented Jan 16, 2021 •

edited by ghost

Loading

facebook-github-bot left a comment

ghost Jan 16, 2021

ghost Jan 16, 2021

ltamasi Jan 19, 2021 •

edited

Loading

facebook-github-bot commented Jan 16, 2021

anand1976 commented Jan 16, 2021

ghost commented Jan 19, 2021 •

edited by ghost

Loading

facebook-github-bot commented Jan 19, 2021

facebook-github-bot left a comment

facebook-github-bot commented Jan 19, 2021

facebook-github-bot left a comment

ajkr left a comment

ajkr commented Jan 19, 2021

facebook-github-bot commented Jan 19, 2021

ghost commented Jan 19, 2021 •

edited by ghost

Loading

facebook-github-bot left a comment

ajkr commented Jan 20, 2021

facebook-github-bot commented Jan 20, 2021

ajkr commented Jan 20, 2021

ltamasi commented Jan 20, 2021

Make it able to ignore WAL related VersionEdits in older versions #7873

Make it able to ignore WAL related VersionEdits in older versions #7873

Conversation

ghost commented Jan 16, 2021 • edited by ghost Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

ghost Jan 16, 2021

Choose a reason for hiding this comment

ghost Jan 16, 2021

Choose a reason for hiding this comment

ltamasi Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Jan 16, 2021

anand1976 commented Jan 16, 2021

ghost commented Jan 19, 2021 • edited by ghost Loading

facebook-github-bot commented Jan 19, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 19, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

ajkr commented Jan 19, 2021

facebook-github-bot commented Jan 19, 2021

ghost commented Jan 19, 2021 • edited by ghost Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

ajkr commented Jan 20, 2021

facebook-github-bot commented Jan 20, 2021

ajkr commented Jan 20, 2021

ltamasi commented Jan 20, 2021

ghost commented Jan 16, 2021 •

edited by ghost

Loading

ltamasi Jan 19, 2021 •

edited

Loading

ghost commented Jan 19, 2021 •

edited by ghost

Loading

ghost commented Jan 19, 2021 •

edited by ghost

Loading