Skip to content

Spr 865 fs snapshot validation#573

Merged
garthgoodson merged 16 commits into
mainfrom
SPR-865-fs-snapshot-validation
Aug 25, 2025
Merged

Spr 865 fs snapshot validation#573
garthgoodson merged 16 commits into
mainfrom
SPR-865-fs-snapshot-validation

Conversation

@ella-springtail

Copy link
Copy Markdown
Contributor

Finished snapshot validation.

@linear

linear Bot commented Aug 7, 2025

Copy link
Copy Markdown
SPR-865 Filesystem check utility

Create a utility that walks the system tables and verifies that every extent in every live table is reachable. For now we don't need to read in each row of each extent, as the extent header has a checksum that validates the extent data. In the future we may want to validate the schema of the extent against each row.

To start with lets validate the tables found in the tableroots table by iterating through the tablenames table. (see system_tables.hh for table definitions)

The tablenames table contains:

  • tablename
  • table ID
  • XID
  • exists flag

If an exists flag is false it indicates that the table was deleted at that XID and all prior XIDs are no longer valid, so should not be checked. The exists flag may become true at a later XID if the table is recreated.

The tableroots table contains:

  • table ID
  • index ID
  • XID
  • extent ID

Validate a table as follows:

  • Find table in tablenames table
  • Once a table has been found in the tablenames table and is known to be valid it can be looked up in the tableroots table (see: service.cc::_get_roots_info() and look at the un-cached path).
  • One can do a primary key lookup and get a table object. (from table roots table)
  • That table object can be iterated to find all extent IDs.
  • Once an extent ID is found, using the original table ID, a table object can be constructed and one can call table->read_extent_from_disk(eid); this will self-validate the extent data. one can iterate through the table rows thus fetching each extent. Ideally we would only iterate over the extents and not the rows.
    • Easier way to iterate through all the extents of the table:
      1. With the table object at an XID, one can get end_offset in the table_stats table
      2. Using the end_offset, call read_extent_from_disk to get the actual extent and next_offset
      3. Repeat step 2 until the extent object returned is null

In future, we can also then validate the other system tables and the indexes for a table.

@rsdcbabu rsdcbabu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, I tried running on my system with vacuum and could see we are validating all the metadata, but for data - we are verifying extents only at the last xid. Can we verify extents between cutoff_xid and last xid, may be as a separate PR?
Basically we have to re-run FSCheck with all xids between cutoff_xid/0 and max_xid.

@rsdcbabu

Copy link
Copy Markdown
Contributor

Just adding a note here based on my observations:

  1. With vacuum false: It works fine looking through all xids if the flag is passed.
  2. With vacuum true: It works as expected only if the system is running, this is because vacuumer's get cutoff xid method calls XidMgrClient::get_instance()->get_committed_xid. If the system doesnt run, it gets stuck at that call.

@craigsoules craigsoules left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A couple of minor comments... let me know once you've reviewed and I can approve.

Comment thread include/storage/vacuumer.hh Outdated
Comment on lines +29 to +31
if (!vacuumer_enabled || _max_xid >= cutoff_xid) {
_db_id_to_cutoff_xid.insert(std::make_pair(db_id, cutoff_xid));
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It looks like other parts of this code assume that this map is always populated via calls to _db_id_to_cutoff_xid.at(), but it seems like it might be possible that this doesn't get populated under the right conditions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is correct. Basically we setup max_xid and vacuumer gives us cutoff xid per database. If max_xid is less thant cutoff xid for some database, then this database won't be evaluated because we won't have the data to work with.

@rsdcbabu

Copy link
Copy Markdown
Contributor

Just adding a note here based on my observations:

  1. With vacuum false: It works fine looking through all xids if the flag is passed.
  2. With vacuum true: It works as expected only if the system is running, this is because vacuumer's get cutoff xid method calls XidMgrClient::get_instance()->get_committed_xid. If the system doesnt run, it gets stuck at that call.

@craigsoules do you think we want to run the check when the system is not running?

@garthgoodson

Copy link
Copy Markdown
Contributor

Just adding a note here based on my observations:

  1. With vacuum false: It works fine looking through all xids if the flag is passed.
  2. With vacuum true: It works as expected only if the system is running, this is because vacuumer's get cutoff xid method calls XidMgrClient::get_instance()->get_committed_xid. If the system doesnt run, it gets stuck at that call.

@craigsoules do you think we want to run the check when the system is not running?

@rsdcbabu The plan was to only run when not running. As the vacuumer could move past that point while the check is running. Unless we can block the vacuumer if a check is running.

@rsdcbabu

Copy link
Copy Markdown
Contributor

Just adding a note here based on my observations:

  1. With vacuum false: It works fine looking through all xids if the flag is passed.
  2. With vacuum true: It works as expected only if the system is running, this is because vacuumer's get cutoff xid method calls XidMgrClient::get_instance()->get_committed_xid. If the system doesnt run, it gets stuck at that call.

@craigsoules do you think we want to run the check when the system is not running?

@rsdcbabu The plan was to only run when not running. As the vacuumer could move past that point while the check is running. Unless we can block the vacuumer if a check is running.

@garthgoodson @ella-springtail ok in that case, we may not be able to use the method in the vacuumer as it requires xid_client to be able to get the last committed xid. We can as well pass the cutoff xid in the param itself?

@garthgoodson

Copy link
Copy Markdown
Contributor

Just adding a note here based on my observations:

  1. With vacuum false: It works fine looking through all xids if the flag is passed.
  2. With vacuum true: It works as expected only if the system is running, this is because vacuumer's get cutoff xid method calls XidMgrClient::get_instance()->get_committed_xid. If the system doesnt run, it gets stuck at that call.

@craigsoules do you think we want to run the check when the system is not running?

@rsdcbabu The plan was to only run when not running. As the vacuumer could move past that point while the check is running. Unless we can block the vacuumer if a check is running.

@garthgoodson @ella-springtail ok in that case, we may not be able to use the method in the vacuumer as it requires xid_client to be able to get the last committed xid. We can as well pass the cutoff xid in the param itself?

Two potential issues:

  1. isn't it per db?
  2. how do we get it to pass it into the command line?

@rsdcbabu

Copy link
Copy Markdown
Contributor

Just adding a note here based on my observations:

  1. With vacuum false: It works fine looking through all xids if the flag is passed.
  2. With vacuum true: It works as expected only if the system is running, this is because vacuumer's get cutoff xid method calls XidMgrClient::get_instance()->get_committed_xid. If the system doesnt run, it gets stuck at that call.

@craigsoules do you think we want to run the check when the system is not running?

@rsdcbabu The plan was to only run when not running. As the vacuumer could move past that point while the check is running. Unless we can block the vacuumer if a check is running.

@garthgoodson @ella-springtail ok in that case, we may not be able to use the method in the vacuumer as it requires xid_client to be able to get the last committed xid. We can as well pass the cutoff xid in the param itself?

Two potential issues:

  1. isn't it per db?
  2. how do we get it to pass it into the command line?

@garthgoodson Maybe I should keep track of cutoff xid ran by vacuumer in the redis per db? that may simplify this, but yeah there is a redis-call overhead.

@rsdcbabu rsdcbabu self-requested a review August 23, 2025 04:10
LOG_INFO("Verifying database {}:{} with first_xid = {} and cuttoff_xid = {}",
db_id, db_name, first_xid, cutoff_xid);

uint64_t start_xid = (first_xid < cutoff_xid)? cutoff_xid : first_xid;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if all_xids is false, start_xid is initialized with LATEST_XID, resulting in the following:
./src/sys_tbl_mgr/file_system_check | grep "Verifying data" [2025-08-23 04:09:07.771 +00:00] [info] [file_system_check.cc:397:_check_db] [thread 254662] Verifying database 1:springtail with first_xid = 18446744073709551615 and cuttoff_xid = 1115 [2025-08-23 04:09:07.771 +00:00] [info] [file_system_check.cc:402:_check_db] [thread 254662] Verifying database 1:springtail iteration max_xid = 18446744073709551615

and so extents are not validated

@rsdcbabu rsdcbabu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Saw a minor bug wrt start_xid if all_xids are not passed, added my comment inline.

@rsdcbabu rsdcbabu self-requested a review August 23, 2025 04:12
@rsdcbabu

Copy link
Copy Markdown
Contributor

Saw a minor bug wrt start_xid if all_xids are not passed, added my comment inline.

Fixed in the last commit, it works:

if I dont pass all_xids:  checks everything at last XID
if I pass all_xids: checks at last XID and checks system tables at last_XID+1 and exit

@sonarqubecloud

Copy link
Copy Markdown

@garthgoodson garthgoodson merged commit fe606fc into main Aug 25, 2025
5 checks passed
@garthgoodson garthgoodson deleted the SPR-865-fs-snapshot-validation branch August 25, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants