Hi Bertrand-san,
> As f19c0ecca introduced online enabling and disabling of data checksums, I
> think that it would make sense to do the same for wal_log_hints.
+1 on the general direction.
> The pg_rewind check on ControlFile->wal_log_hints (which reflects
> the current state at rewind time) remains sufficient.
I'm not sure this holds. In fact, [1] seems to say the opposite:
> It does not really matter if the node used log_hint_bits set to
> false in its latest state (Node to-be-rewinded might have been
> restarted after WAL forked).
What pg_rewind actually requires is that the effective value was
continuously on from the divergence point to the end of the target's
WAL. pg_control only keeps the latest value, so an off period after
the divergence goes undetected:
1. Run with wal_log_hints = off (data checksums off).
2. The standby is promoted (divergence).
3. On the old primary, a hint-bit-only page update happens (not WAL-logged).
4. wal_log_hints is set to on via reload (pg_control now says on).
5. The old primary is cleanly shut down and pg_rewind is run.
The check passes, but the page from step 3 never shows up in
extractPageMap() and is not copied from the source. Combined with the
source's CLOG, the leftover hint bit yields incorrect tuple visibility.
I was able to reproduce this: with a transaction held open across the
divergence (insert before the last common checkpoint, commit after the
divergence), the rewound node shows a row that does not exist on the
source, due to a leftover HEAP_XMIN_COMMITTED hint. I can provide this
as a TAP test. Note that
012_wal_log_hints.pl in the patch exercises
exactly this sequence (diverge while off, reload to on, pg_rewind
succeeds) -- the command succeeds, but the result is not consistent.
Strictly speaking, a similar gap can already be created today by
restarting with a different value, but a reload makes it much easier
to hit.
How about making pg_rewind fail if the WAL range it already scans in
extractPageMap() contains an XLOG_PARAMETER_CHANGE record with
wal_log_hints = off? Current pg_control saying on, plus no change
record in the scanned range, guarantees the value was on for the whole
range. This is essentially the tracking that [1] already suggested on
the pg_rewind side, which does not seem to have been implemented.
[1]
https://postgr.es/m/CAB7nPqSXYs4Jg-KJMy8xiM4sTqkgKvHyDrCoojhXzRSKiW57+g@mail.gmail.comRegards,
Tatsuya Kawata
