WT-10862 Support read-only fast-truncate data format change in MongoDB 6.0 release #9396

mm-ng · 2023-07-13T01:07:03Z

No description provided.

mm-ng · 2023-07-14T05:29:11Z

@agorrod @kommiharibabu Would appreciate any feedback you might have on this approach. I'm also not sure if this is the right point in time to think about upgrade/downgrade testing. If so, I can look into doing some manual checks.

src/include/connection.h

src/include/cell_inline.h

agorrod

Thanks Monica - this looks like it's in the right direction.

I've got a question about the implementation, and I wonder about testing. Does our existing compatibility testing cover this backport, or do we need to add something specific?

agorrod · 2023-07-17T01:15:36Z

src/include/cell_inline.h

@@ -845,6 +848,28 @@ __wt_cell_unpack_safe(WT_SESSION_IMPL *session, const WT_PAGE_HEADER *dsk, WT_CE
        break;
    }

+    /*
+     * Unpack any fast-truncate information. Note that there is no way to write fast-truncate


Should this just skip the fast truncate information, rather than unpacking it?

I guess it's a question of whether information that was generated by a newer release should be parsed out and maybe, therefore, alter decisions made at runtime.

It feels as though it is going to be hard to test that combination (i.e: run with a newer version, generate fast truncate information, downgrade, read fast truncate information, ensure correct decisions are being made).

So, I'd be inclined towards ignoring the information as early as possible, rather than using it to populate data structures.

I agree with Alex, but I believe we need to unpack it, otherwise, we may get the problem. Instead of saving the unpacked data into the unpack_addr, just save them into a local variable and ignore them.

Yes, I don't believe it's possible to simply skip the information - I think it could disrupt other information that is being unpacked as it would affect the position of the pointer. I can definitely unpack it into a local WT_PAGE_DELETED structure rather than into a structure inside the unpack_addr.

Should we move this logic into a dedicated function?

@quchenhao Would there be any specific advantages of having a separate function for this? We're only unpacking the information once.

Just to make the code cleaner.

mm-ng · 2023-07-17T03:41:25Z

Thanks Monica - this looks like it's in the right direction.

I've got a question about the implementation, and I wonder about testing. Does our existing compatibility testing cover this backport, or do we need to add something specific?

I don't think our existing compatibility testing would cover this scenario. From what I understand, our testing takes changes to the develop branch and tests them against older branches.

It feels as though it is going to be hard to test that combination (i.e: run with a newer version, generate fast truncate information, downgrade, read fast truncate information, ensure correct decisions are being made).

Yes, I think any sort of testing may have to be manual. I've done a basic test of generating fast truncate information with a newer version, downgrading, and then attempting to verify the database file. I didn't run into any problems but I also suspect it might not be hitting the unpack logic.

kommiharibabu · 2023-07-17T04:31:07Z

src/include/cell_inline.h

+        page_del.prepare_state = 0;            /* No prepare can have been in progress. */
+        page_del.previous_state = WT_REF_DISK; /* The leaf page is on disk. */
+        page_del.committed = 1;                /* There is no running transaction. */
+
+        /* Avoid a stale transaction ID on restart. */
+        if (dsk->write_gen <= S2BT(session)->base_write_gen &&
+          !F_ISSET(session, WT_SESSION_DEBUG_DO_NOT_CLEAR_TXN_ID))
+            page_del.txnid = WT_TXN_NONE;


This code is no longer required as we are just unpacking the existing page_del info from the cell.

I think we still need this information in the downgraded version. If the fast truncate is stable and visible, the downgraded version should be able to read that and return not found for any data in the subtree. I don't think we can directly ignore this information. @kommiharibabu @agorrod @mm-ng I think this change may not be enough. In addition, we also need the code in 6.0 to cleanup the fast truncated tree.

I assume we will always downgrade from a clean shut down. Therefore, there is no need to handle the fast truncate information in rts. But I think we better to still do that unless we have a way to prevent the user to directly downgrade from an unclean shutdown.

@quchenhao My intention for this change was only to be able to read database files from newer versions which may or may not write the additional fast-truncate information. There would be no guarantee of whether all the fast-truncate behavior would be the same as newer versions.

We should address the issue of a potential disk leak elsewhere, as it should also be backported to older versions and I feel that it is separate from this change. The disk leak should not be an issue currently as MongoDB does not write out fast-truncate information. I've created a new ticket WT-11321 to track the issue, and it should be completed before WT-10307 but doesn't need to be completed for this current ticket.

kommiharibabu · 2023-07-17T04:31:26Z

src/include/cell.h

@@ -197,6 +198,8 @@ struct __wt_cell_unpack_addr {
    WT_CELL_COMMON_FIELDS;

    WT_TIME_AGGREGATE ta; /* Address validity window */
+
+    WT_PAGE_DELETED page_del; /* Fast-truncate information */


As per the latest code change, this variable also no longer needed.

quchenhao · 2023-07-17T04:50:47Z

src/include/cell_inline.h

+        WT_RET(__wt_vunpack_uint(&p, end == NULL ? 0 : WT_PTRDIFF(end, p), &page_del.timestamp));
+        WT_RET(
+          __wt_vunpack_uint(&p, end == NULL ? 0 : WT_PTRDIFF(end, p), &page_del.durable_timestamp));
+        page_del.prepare_state = 0;            /* No prepare can have been in progress. */


We should use WT_PREPARE_INIT flag here.

quchenhao · 2023-07-17T04:51:59Z

src/include/cell_inline.h

+          __wt_vunpack_uint(&p, end == NULL ? 0 : WT_PTRDIFF(end, p), &page_del.durable_timestamp));
+        page_del.prepare_state = 0;            /* No prepare can have been in progress. */
+        page_del.previous_state = WT_REF_DISK; /* The leaf page is on disk. */
+        page_del.committed = 1;                /* There is no running transaction. */


Should use true instead of 1.

kommiharibabu

LGTM. Thanks Monica. I want you to perform some manual downgrade/upgrade scenario before merging the change.

mm-ng · 2023-07-20T03:12:20Z

LGTM. Thanks Monica. I want you to perform some manual downgrade/upgrade scenario before merging the change.

Thanks Hari. I have performed some manual upgrade/downgrade testing and have described the steps taken in the Jira ticket.

@agorrod @quchenhao let me know if you have any further thoughts on the PR, otherwise I think this can be merged soon.

quchenhao

lgtm

agorrod

I'd be more comfortable knowing that there was automated testing covering this scenario somewhere.

I'm guessing that automated testing actually belongs in develop (or latest), since it involves running with that version as well as this older version. Could you create a new ticket that captures upgrade/downgrade testing with fast truncate aimed at ensuring this backport is correct?

agorrod · 2023-07-20T03:30:51Z

src/btree/row_modify.c

-                  ((*upd_entry)->type == WT_UPDATE_TOMBSTONE && (*upd_entry)->txnid == WT_TS_NONE &&
-                    (*upd_entry)->start_ts == WT_TS_NONE)) ||
+                  ((*upd_entry)->type == WT_UPDATE_TOMBSTONE &&
+                    (*upd_entry)->txnid == WT_TXN_NONE && (*upd_entry)->start_ts == WT_TS_NONE)) ||


Nice catch - do we need to be concerned about this? It looks like a bug regardless of the backport here?

Yeah, it looks like a typo that I decided to just fix here. I think it's actually ok because WT_TS_NONE and WT_TXN_NONE are actually the same value in the codebase, so I don't know if we should call it a bug. It's just confusing to read in the condition.

agorrod · 2023-07-20T03:33:17Z

src/include/cell_inline.h

+    /*
+     * Unpack any fast-truncate information. Note that there is no way to write fast-truncate
+     * information to disk in versions before 6.1, but this information may still exist in the
+     * database files if we are downgrading from newer versions.


I think this deserves another sentence along the lines of "If the information is present, skip past it and ignore the values. That is the equivalent of assuming the content is globally visible, which matches the existing behavior in 6.1."

@agorrod I don't quite understand the second part of your sentence when you say it matches the existing behavior in 6.1. Do you mean that by ignoring the fast truncate information in 6.0, and is equivalent to how 6.1 treats globally visible truncates?

Oh, I think I just had my versions wrong. I meant ignoring version information is equivalent to the version that doesn't ever include visibility when writing out truncated content.

@agorrod I've updated the comment in 20f851a, let me know if you'd like me to make any more changes to the wording.

mm-ng · 2023-07-20T04:00:38Z

I'd be more comfortable knowing that there was automated testing covering this scenario somewhere.

I'm guessing that automated testing actually belongs in develop (or latest), since it involves running with that version as well as this older version. Could you create a new ticket that captures upgrade/downgrade testing with fast truncate aimed at ensuring this backport is correct?

I can definitely create a ticket to add in automated testing for fast-truncate if that is what you are after.

Edit: Created WT-11331.

agorrod

lgtm

mm-ng · 2023-07-21T01:34:49Z

evergreen merge

mm-ng added 4 commits July 13, 2023 00:52

Add fast truncate runtime variable for standalone releases

2aa9fe6

Add fast truncate flag to page header

f03a570

Expand cell size to account for fast truncate info

8f4e900

Support fast truncate information read only

386006b

mm-ng marked this pull request as ready for review July 14, 2023 05:19

mm-ng changed the title ~~WT-10862 (read support only)~~ WT-10862 Support read-only truncate data format change in MongoDB 6.0 release Jul 14, 2023

mm-ng requested review from kommiharibabu and agorrod July 14, 2023 05:22

kommiharibabu reviewed Jul 16, 2023

View reviewed changes

src/include/connection.h Outdated Show resolved Hide resolved

src/include/cell_inline.h Outdated Show resolved Hide resolved

mm-ng added 2 commits July 17, 2023 00:50

Remove unnecessary variables and variable changes

7adba88

Run s_all

e3bba69

agorrod reviewed Jul 17, 2023

View reviewed changes

Unpack page delete info into local variable

3096cb1

kommiharibabu reviewed Jul 17, 2023

View reviewed changes

quchenhao reviewed Jul 17, 2023

View reviewed changes

Remove unnecessary variables and information

67bfbb2

kommiharibabu approved these changes Jul 18, 2023

View reviewed changes

mm-ng requested review from agorrod and quchenhao July 20, 2023 03:12

quchenhao approved these changes Jul 20, 2023

View reviewed changes

agorrod reviewed Jul 20, 2023

View reviewed changes

mm-ng added 2 commits July 20, 2023 05:02

Update comment in cell unpacking function

20f851a

Run s_all

427c7d0

mm-ng requested a review from agorrod July 20, 2023 22:58

mm-ng changed the title ~~WT-10862 Support read-only truncate data format change in MongoDB 6.0 release~~ WT-10862 Support read-only fast-truncate data format change in MongoDB 6.0 release Jul 21, 2023

agorrod approved these changes Jul 21, 2023

View reviewed changes

mm-ng merged commit 6b52dd5 into mongodb-6.0 Jul 21, 2023
5 checks passed

mm-ng deleted the wt-10862-truncate-read-support-only branch July 21, 2023 04:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WT-10862 Support read-only fast-truncate data format change in MongoDB 6.0 release #9396

WT-10862 Support read-only fast-truncate data format change in MongoDB 6.0 release #9396

mm-ng commented Jul 13, 2023

mm-ng commented Jul 14, 2023

agorrod left a comment

agorrod Jul 17, 2023

kommiharibabu Jul 17, 2023 •

edited

mm-ng Jul 17, 2023

quchenhao Jul 17, 2023

mm-ng Jul 17, 2023

quchenhao Jul 17, 2023

mm-ng commented Jul 17, 2023

kommiharibabu Jul 17, 2023

quchenhao Jul 17, 2023 •

edited

mm-ng Jul 17, 2023

mm-ng Jul 18, 2023

kommiharibabu Jul 17, 2023

quchenhao Jul 17, 2023

quchenhao Jul 17, 2023

kommiharibabu left a comment

mm-ng commented Jul 20, 2023

quchenhao left a comment

agorrod left a comment

agorrod Jul 20, 2023

mm-ng Jul 20, 2023 •

edited

agorrod Jul 20, 2023

mm-ng Jul 20, 2023

agorrod Jul 20, 2023

mm-ng Jul 20, 2023

mm-ng commented Jul 20, 2023 •

edited

agorrod left a comment

mm-ng commented Jul 21, 2023

WT-10862 Support read-only fast-truncate data format change in MongoDB 6.0 release #9396

WT-10862 Support read-only fast-truncate data format change in MongoDB 6.0 release #9396

Conversation

mm-ng commented Jul 13, 2023

mm-ng commented Jul 14, 2023

agorrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommiharibabu Jul 17, 2023 • edited

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

mm-ng commented Jul 17, 2023

Choose a reason for hiding this comment

quchenhao Jul 17, 2023 • edited

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

Choose a reason for hiding this comment

kommiharibabu left a comment

Choose a reason for hiding this comment

mm-ng commented Jul 20, 2023

quchenhao left a comment

Choose a reason for hiding this comment

agorrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm-ng Jul 20, 2023 • edited

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

mm-ng commented Jul 20, 2023 • edited

agorrod left a comment

Choose a reason for hiding this comment

mm-ng commented Jul 21, 2023

kommiharibabu Jul 17, 2023 •

edited

quchenhao Jul 17, 2023 •

edited

mm-ng Jul 20, 2023 •

edited

mm-ng commented Jul 20, 2023 •

edited