fix: stop storing TimelineMetadata in index_part.json as bytes

[koivunej]

We've stored metadata as bytes within the index_part.json for since long fixed reasons. #7693 added support for reading out normal json serialization of the TimelineMetadata.

Change the serialization to only write TimelineMetadata as json for going forward, keeping the backwards compatibility to reading the metadata as bytes. Because of failure to include alias = "metadata" in #7693, one more follow-up is required to make the switch from the old name to "metadata": <json>, but that affects only the field name in serialized format.

In documentation and naming, an effort is made to add enough warning signs around TimelineMetadata so that it will receive no changes in the future. We can just add those fields to IndexPart directly instead. Changing TimelineMetadata now carries all of the backward compatibility risks without offering any benefits over changing IndexPart, where we do have a clear strategy (soft json (de)serialization).

Additionally the path to cleaning up metadata.rs is documented in the metadata.rs module comment. If we must extend TimelineMetadata before that, the duplication suggested in review comment is the way to go.

[github-actions]

3198 tests run: 3056 passed, 0 failed, 142 skipped (full report)

---

Flaky tests (1)

Postgres 14

• test_subscriber_restart: release

Code coverage* (full report)

• functions: 31.5% (6602 of 20948 functions)
• lines: 48.5% (51082 of 105414 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
1891124 at 2024-06-11T12:35:02.476Z :recycle:}

[koivunej]

Failures on Postgres 16

• test_forward_compatibility: release, debug

Failures on Postgres 15

• test_forward_compatibility: release, debug

Failures on Postgres 14

• test_forward_compatibility: release, debug

Again, as expected 🎉

[problame]

The metadata.rs is hard to read for me, maybe because I didn't review the first PR, but, really, I think it's because you tried hard to not duplicate the field definitions.

I wonder if we could rename-on-read the field to just metadata and allow soft-deserialize-style format evolution for the JSON value.

The price would be to duplicate the fields initially. New fields would only go into the serde_current_json's TimelineMetadata.

Like so:

mod metadata {

  enum TimelineMetadata {
    disk_consistent_lsn: Lsn,
    prev_record_lsn: Option<Lsn>,
    ancestor_timeline: Option<TimelineId>,
    ancestor_lsn: Lsn,
    latest_gc_cutoff_lsn: Lsn,
    initdb_lsn: Lsn,
    pg_version: u32,
  }

  impl Deserialize for TimelineMetadata {
    // if it's a serde seq then
    //    - deser to Vec<u8>
    //    - decode using serde_legacy_bincode::TimelineMetadata::from_bytes
    // if it's a serde map then
    //    - deser a serde_current_json::TimelineMetadata
    //    - move fields into Self
  }

  impl Serialize for TimelineMetadata {
    // always ser using serde_current_json::TimelineMetadata::serialize
  }

  mod serde_legacy_bincode {
    //! DO NOT CHANGE, HERE ONLY FOR BACKWARDS-COMPATIBILITY.
    //! This format was abandoned because it does not support format evolution and is painful to read by humans.

    #[derive(Deserialize, Serialize)]
    struct TimelineMetadata {
      // DO NOT CHANGE, SEE MODULE COMMENT
      disk_consistent_lsn: Lsn,
      prev_record_lsn: Option<Lsn>,
      ancestor_timeline: Option<TimelineId>,
      ancestor_lsn: Lsn,
      latest_gc_cutoff_lsn: Lsn,
      initdb_lsn: Lsn,
      pg_version: u32,
      // DO NOT CHANGE, SEE MODULE COMMENT 
    }
    // the bincode impl
  }

  mod serde_current_json {
    //! The format that we're currently writing. Supports format evolution through soft deserialization.

    // See note at bottom of struct for how to add new fields.
    
    #[derive(Deserialize, Serialize)]
    struct TimelineMetadata {
      disk_consistent_lsn: Lsn,
      prev_record_lsn: Option<Lsn>,
      ancestor_timeline: Option<TimelineId>,
      ancestor_lsn: Lsn,
      latest_gc_cutoff_lsn: Lsn,
      initdb_lsn: Lsn,
      pg_version: u32, 
      // When adding a field here, ensure backwards-compatibility and add my_new_field_update_on_read code.
      // And remember to bump `LATEST_VERSION` in IndexPart.
    }
  }

}

[koivunej]

I wonder if we could rename-on-read the field to just metadata and allow soft-deserialize-style format evolution for the JSON value.

We had a call about this. We can.

I didn't think of this while being too focused on declaring metadata.rs a radioactive no-go zone. However, it doesn't have to be final, and none of the changes done here or in the previous PR make that approach more difficult. I added some thoughts to the module docs in 1891124 about how this would happen. In addition to upgrading all index_part.json files to the latest version, we need to wait 1 month from the last upgrade to allow for s3 recovery to pass before cleaning up the module.

I also linked your proposal comment to the PR description to be included with the commit message.