Preserve HEAP_COMBOCID when restoring t_cid from WAL

[knizhnik]

Problem

See #8499

Summary of changes

Save HEAP_COMBOCID flag in WAL and do not clear it in redo handlers.

Related Postgres PRs:
neondatabase/postgres#457
neondatabase/postgres#458
neondatabase/postgres#459

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

2144 tests run: 2075 passed, 0 failed, 69 skipped (full report)

---

Flaky tests (2)

Postgres 15

• test_branch_and_gc: release

Postgres 14

• test_branch_and_gc: release

Code coverage* (full report)

• functions: 32.3% (7162 of 22161 functions)
• lines: 50.3% (57930 of 115176 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
2b569a4 at 2024-08-13T20:13:25.776Z :recycle:}

[MMeent]

LGTM

[hlinnaka]

Hmm, so let's look at the case that you UPDATE a row that has been inserted earlier in the same transaction. In that case:

• the old tuple needs to be stamped with a combo cid that represents the cmin and cmax combination. It needs to have the HEAP_COMBOCID flag set.
• the new tuple needs to be stamped with the command id of the updating command, as cmin. It must not have the HEAP_COMBOCID flag set.

The combocid stored on the old tuple, and the cmin stored on the new tuple are fundamentally two different values. But we only have one t_cid field in the WAL record. That's bad news :-(. I don't see how this can possibly work without changing the WAL record format.

[hlinnaka]

I added a test that fails without the fixes from this PR, and passes with them. It only covers the DELETE case, but it should be straightforward to exercise the other cases with a copy-paste of this test.

[knizhnik]

The combocid stored on the old tuple, and the cmin stored on the new tuple are fundamentally two different values. But we only have one t_cid field in the WAL record. That's bad news :-(. I don't see how this can possibly work without changing the WAL record format.

Sorry, I do not understand it. We really have two t_cid in HEAP_UPDATE WAL record: one in record itself and another in xl_neon_heap_header.

[hlinnaka]

The combocid stored on the old tuple, and the cmin stored on the new tuple are fundamentally two different values. But we only have one t_cid field in the WAL record. That's bad news :-(. I don't see how this can possibly work without changing the WAL record format.

Sorry, I do not understand it. We really have two t_cid in HEAP_UPDATE WAL record: one in record itself and another in xl_neon_heap_header.

Oh ok. Good, never mind then :-).

[hlinnaka]

This looks correct. TODOs:

• Add tests to cover each of the cases, INSERT, UPDATE, DELETE, LOCK, MULTI_INSERT
• There's a funny case within heap_update, where it writes a LOCK record for the tuple first, if it has a toast tuple. Not sure of the details, but it looks correct. Let's just make sure we have a test to cover that too.
• There's some unrelated smgrexists() changes in the postgres branches. Need to rebase, I guess

The compute_infobits function is also used by the LOCK_UPDATED record. That means that we will set the XLHL_COMBOCID flag on those records too, even though we don't use the custom neon rmgr for that. That's a bit dirty. So what happens if you feed such a record to vanilla Postgres? It will work, but vanilla fix_infomask_from_infobits() won't set the COMBOCID flag. That's OK, the COMBOCID flag is irrelevant in that scenario.

[knizhnik]

Can somebody also approve correspondent Postgres PRs?