At Sun, 13 Feb 2022 17:35:38 +0300, Yura Sokolov <y.soko...@postgrespro.ru> wrote in > В Пн, 07/02/2022 в 13:52 +0530, Dilip Kumar пишет: > > Right. I think the patch looks fine to me. > > > > Good day. > > I've looked to the patch. Personally I'd prefer dynamically resize > xip array. But I think there is issue with upgrade if replica source > is upgraded before destination, right?
I don't think it is relevant. I think we don't convey snapshot via streaming replication. But I think that expanding xip or subxip is wrong, since it is tied with ProcArray structure. (Even though we abuse the arrays in some situations, like this). > Concerning patch, I think more comments should be written about new > usage case for `takenDuringRecovery`. May be this field should be renamed > at all? I don't feel the need to rename it so much. It just signals that "this snapshot is in the form for recovery". The more significant reason is that I don't come up better name:p And the comment is slightly modified and gets a pointer to detailed comment. + * Although this snapshot is not acutally taken during recovery, all XIDs + * are stored in subxip. See GetSnapshotData for the details of that form + * of snapshot. > And there are checks for `takenDuringRecovery` in `heapgetpage` and > `heapam_scan_sample_next_tuple`. Are this checks affected by the change? > Neither the preceding discussion nor commit message answer me. The snapshot works correctly, but for the heapgetpage case, it foces all_visible to be false. That unnecessarily prevents visibility check from skipping. An annoying thing in SnapBuildInitialSnapshot is that we don't know the number of xids before looping over the xid range, and we don't want to bother sorting top-level xids and subxids unless we have to do so. Is it better that we hassle in SnapBuildInitialSnapshot to create a !takenDuringRecovery snapshot? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
