On Tue, Dec 22, 2015 at 10:43 PM, Robert Haas <
robertmhaas@gmail.com> wrote:
>
> On Mon, Dec 21, 2015 at 1:27 AM, Amit Kapila <
amit.kapila16@gmail.com> wrote:
> > On Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <
robertmhaas@gmail.com> wrote:
> >>
> >> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <
amit.kapila16@gmail.com>
> >> wrote:
> >>
> >> >> Some random comments:
> >> >>
> >> >> - TransactionGroupUpdateXidStatus could do just as well without
> >> >> add_proc_to_group. You could just say if (group_no >= NUM_GROUPS)
> >> >> break; instead. Also, I think you could combine the two if statements
> >> >> inside the loop. if (nextidx != INVALID_PGPROCNO &&
> >> >> ProcGlobal->allProcs[nextidx].clogPage == proc->clogPage) break; or
> >> >> something like that.
> >> >>
> >
> > Changed as per suggestion.
> >
> >> >> - memberXid and memberXidstatus are terrible names. Member of what?
> >> >
> >> > How about changing them to clogGroupMemberXid and
> >> > clogGroupMemberXidStatus?
> >>
> >> What we've currently got for group XID clearing for the ProcArray is
> >> clearXid, nextClearXidElem, and backendLatestXid. We should try to
> >> make these things consistent. Maybe rename those to
> >> procArrayGroupMember, procArrayGroupNext, procArrayGroupXid
> >>
> >
> > Here procArrayGroupXid sounds like Xid at group level, how about
> > procArrayGroupMemberXid?
> > Find the patch with renamed variables for PGProc
> > (rename_pgproc_variables_v1.patch) attached with mail.
>
> I sort of hate to make these member names any longer, but I wonder if
> we should make it procArrayGroupClearXid etc.
If we go by this suggestion, then the name will look like:
PGProc
{
..
bool procArrayGroupClearXid, pg_atomic_uint32 procArrayGroupNextClearXid,
TransactionId procArrayGroupLatestXid;
..
PROC_HDR
{
..
pg_atomic_uint32 procArrayGroupFirstClearXid;
..
}
I think whatever I sent in last patch were better. It seems to me it is
better to add some comments before variable names, so that anybody
referring them can understand better and I have added comments in
attached patch rename_pgproc_variables_v2.patch to explain the same.
> Otherwise it might be
> confused with some other time of grouping of PGPROCs.
>
Won't procArray prefix distinguish it from other type of groupings?
About CLogControlLock patch, yesterday I noticed that SLRU code
can return error in some cases which can lead to hang for group
members, as once group leader errors out, there is no one to wake
them up. However on further looking into code, I found out that
this path (TransactionIdSetPageStatus()) is always called in critical
section (RecordTransactionCommit() ensures the same), so any
ERROR in this path will be converted to PANIC which don't require
any wakeup mechanism for group members. In any case, if you
find any other way which can lead to error (not being converted to
PANIC), I have already handled the error case in the attached patch
(group_update_clog_error_handling_v4.patch) and if you also don't
find any case, then previous patch stands good.