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
WT-3493 wt_verbose_dump_txn should display timestamp information #3580
Conversation
Make __wt_timestamp_to_hex_string() visible, add support for zero timestamps (returning "0"). Use __wt_timestamp_to_hex_string() to add timestamp information to the __wt_verbose_dump_txn() function. Change btree debug code that prints out a WT_UPDATE timestamp to call __wt_timestamp_to_hex_string() as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#ifdef HAVE_TIMESTAMPS | ||
WT_RET(__wt_timestamp_to_hex_string( | ||
session, hex_timestamp[0], &txn_global->commit_timestamp)); | ||
WT_RET(__wt_msg(session, "commit timestamp: %s", hex_timestamp[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will be output when WT_VERB_EVICT_STUCK is set and won't care about the newly added WT_VERB_TIMESTAMP. If WT_VERB_TIMESTAMP is set and WT_VERB_EVICT_STUCK not, we won't see these. I think that is okay, thinking out loud here.
@keithbostic, I think the change to return a "0" string is okay -- queries should get |
WT_RET(__wt_msg(session, "oldest_is_pinned: %s", | ||
txn_global->oldest_is_pinned ? "yes" : "no")); | ||
WT_RET(__wt_msg(session, "stable_is_pinned: %s", | ||
txn_global->stable_is_pinned ? "yes" : "no")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor nit: Did you see how it looked with related fields displayed near each other rather than types of fields grouped? I.e. if I'm debugging something related to the oldest timestamp, it might be more useful to have oldest_timestamp
, has_oldest_timestamp
and oldest_is_pinned
all printed together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sueloverso, please don't hesitate to re-order to make this more useful. (I haven't done any debugging in the timestamp code, so I used the field ordering from the include file as a first approximation of what should be displayed together, and of course alignment issues might will skew how things are grouped there.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To wrap up, I spoke with @milkie who motivated the original ticket and he thinks that the current ordering is most useful to start.
@michaelcahill, there's one minor change here that might be of interest. I changed
__wt_timestamp_to_hex_string()
to return the string "0" in the case of a zero timestamp.That's potentially interesting if
__wt_txn_global_query_timestamp()
calls that function with a zero timestamp as we'll return "0" instead of the empty string. I think that's a bug fix, if anything (unless we document somewhere that an empty string is the same as a "0" string), but wanted to call it to your attention.