Thread: Re: API stability
At Tue, 5 Apr 2022 10:01:56 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > I think as the person who committed that patch I'm on the hook to fix > this if nobody else would like to do it, but let me ask whether > Kyotaro Horiguchi would like to propose a patch, since the original > patch did, and/or whether you would like to propose a patch, as the > person reporting the issue. I'd like to do that. Let me see. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
me> I'd like to do that. Let me see. At Tue, 5 Apr 2022 10:29:03 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Tue, Apr 5, 2022 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > What I think you need to do is: > > > > 1. In the back branches, revert delayChkpt to its previous type and > > semantics. Squeeze a separate delayChkptEnd bool in somewhere > > (you can't change the struct size either ...). > > > > 2. In HEAD, rename the field to something like delayChkptFlags, > > to ensure that any code touching it has to be inspected and updated. > > Well, we can do it that way, I suppose. The change is easy on head, but is it better use uint8 instead of int for delayChkptFlags? In the back branches, we have, on gcc/Linux/x86-64, 14's PGPROC is 880 bytes and has gaps: - 6 bytes after statusFlag - 4 bytes after syncRepState - 2 bytes after subxidStatus - 3 bytes after procArrayGroupMember - 3 bytes after clogGroupMember - 3 bytes after fpVXIDLock It seems that we can place the new variable in the first place above, since the two are not bonded together, or at least in less tightly bonded than other candidates. 13's PGPROC is 856 bytes and has a 7 bytes gap after delayChkpt. Versions Ealier than 13 have delayChkpt in PGXACT (12 bytes). It is tightly packed and dones't have a room for a new member. Can we add the new flag to PGPROC instead of PGXACT? 12 and 11's PGPROC is 848 bytes and has gaps: - 4 bytes after syncRepState - 3 bytes after procArrayGroupMember - 3 bytes after clogGroupMember - 4 bytes after clogGroupMemberPage - 3 bytes after fpVXIDLock 10's PGPROC is 816 bytes and has gaps: - 4 bytes after cvWaitLink - 4 bytes after syncRepState - 3 bytes after procArrayGroupMember - 3 bytes after fpVXIDLock So if we don't want to move any member in PGPROC, we do: 14: after statusFlags. 13: after delayChkpt. 12-10: after syncRepState (and before syncRepLinks). If we allow to shift some members, the new flag can be placed more saner place. 14: after delayChkpt ((uint8)statusFlags moves forward by 1 byte) 13: after delayChkpt (no member moves) 12-10: after subxids ((bool)procArrayGroupMember moves forward by 1 byte) I continue working on the last direction above. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Wed, 06 Apr 2022 14:30:37 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > So if we don't want to move any member in PGPROC, we do: > > 14: after statusFlags. > 13: after delayChkpt. > 12-10: after syncRepState (and before syncRepLinks). > > If we allow to shift some members, the new flag can be placed more > saner place. > > 14: after delayChkpt ((uint8)statusFlags moves forward by 1 byte) > 13: after delayChkpt (no member moves) > 12-10: after subxids ((bool)procArrayGroupMember moves forward by 1 byte) > > I continue working on the last direction above. Hmm. That is ABI break. I go with the first way. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Wed, 06 Apr 2022 15:31:53 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Wed, 06 Apr 2022 14:30:37 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > So if we don't want to move any member in PGPROC, we do: > > > > 14: after statusFlags. > > 13: after delayChkpt. > > 12-10: after syncRepState (and before syncRepLinks). > > > > If we allow to shift some members, the new flag can be placed more > > saner place. > > > > 14: after delayChkpt ((uint8)statusFlags moves forward by 1 byte) > > 13: after delayChkpt (no member moves) > > 12-10: after subxids ((bool)procArrayGroupMember moves forward by 1 byte) > > > > I continue working on the last direction above. > > Hmm. That is ABI break. I go with the first way. By the way, the patch for -14 changed the sigunature of two public functions. -GetVirtualXIDsDelayingChkpt(int *nvxids) +GetVirtualXIDsDelayingChkpt(int *nvxids, int type) -HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) +HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type) Do I need to restore the signature? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Wed, 06 Apr 2022 15:53:32 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Wed, 06 Apr 2022 15:31:53 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > At Wed, 06 Apr 2022 14:30:37 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > > So if we don't want to move any member in PGPROC, we do: > > > > > > 14: after statusFlags. > > > 13: after delayChkpt. > > > 12-10: after syncRepState (and before syncRepLinks). > > > > > > If we allow to shift some members, the new flag can be placed more > > > saner place. > > > > > > 14: after delayChkpt ((uint8)statusFlags moves forward by 1 byte) > > > 13: after delayChkpt (no member moves) > > > 12-10: after subxids ((bool)procArrayGroupMember moves forward by 1 byte) > > > > > > I continue working on the last direction above. > > > > Hmm. That is ABI break. I go with the first way. > > By the way, the patch for -14 changed the sigunature of two public > functions. > > -GetVirtualXIDsDelayingChkpt(int *nvxids) > +GetVirtualXIDsDelayingChkpt(int *nvxids, int type) > > -HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) > +HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type) > > Do I need to restore the signature? For master, renamed delayChkpt to delayChkptFlags and changed it to uint8. For 14, restored delayChkpt to bool and added delayChkptEnd into a gap in PGPROC, then restored the signature of the twofunctions above to before the patch. Then added a new functions ..DelayingChkptEnd(). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Attachment
On 2022-Apr-06, Kyotaro Horiguchi wrote: > For master, renamed delayChkpt to delayChkptFlags and changed it to > uint8. For code documentation purposes, I think it is slightly better to use bits8 than uint8 for variables where you're storing independent bit flags. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)