Hi Kuroda-san,
> Hmm, I'm not excited to modify the exposed data structure yet, unless there is a
> real issue.
Agreed. It was only a readability suggestion, not a fix for a real failure.
I checked again. CheckXidAlive is set only at the start of each change in
ReorderBufferProcessTXN(), before that change's catalog access. It is cleared
only after the change loop, or on abort, where ResetLogicalStreamingState()
also resets the new depth counter. So it does not change while a scan is open,
except on the error path, which v1 already handles. v1 looks complete to me.
Thanks for the patch.
Regards,
Jiří Kavalík
Hi Jiří,
Thanks for the test. I understood that no issues were found for now.
> One question while reading the patch, not a problem I could trigger: the depth
> is incremented in systable_beginscan* and decremented in systable_endscan* only
> if CheckXidAlive is valid, and that is evaluated separately at each end. If
> CheckXidAlive changed while a scan was open, the counter would be off by one.
> I could not find a path where that happens. Error paths look fine, since
> AbortTransaction/AbortSubTransaction call ResetLogicalStreamingState(). If a
> SysScanDesc field is acceptable despite the header concern, remembering in the
> scan whether it was counted would make the pairing explicit.
Hmm, I'm not excited to modify the exposed data structure yet, unless there is a
real issue. Per my analysis, SysScanDescData only contains pointers (8 bytes),
it does not have any paddings. This meant we need to modify a size of the data
structure, it might cause failures somewhere.
Also, the existing code has the same possibility while turning on/off bsysscan,
right? So I feel it's already accepted.
(Of course, we must fix if it causes a real failure)
Best regards,
Hayato Kuroda
FUJITSU LIMITED
-- S pozdravem
Jiří Kavalík
Comgate a.s.
Gočárova třída 1754/48b, 500 02 Hradec Králové
