On Sat, Jul 25, 2015 at 10:30 PM, Ildus Kurbangaliev < i.kurbangal...@postgrespro.ru> wrote:
> > > On Jul 24, 2015, at 10:02 PM, Robert Haas <robertmh...@gmail.com> wrote: > > > > Also, the patch should not invent a new array similar but not quite > > identical to LockTagTypeNames[]. > > > > This is goofy: > > > > + if (tranche_id > 0) > > + result->tranche = tranche_id; > > + else > > + result->tranche = LW_USERDEFINED; > > > > If we're going to make everybody pass a tranche ID when they call > > LWLockAssign(), then they should have to do that always, not sometimes > > pass a tranche ID and otherwise pass 0, which is actually a valid > > tranche ID but to which you've given a weird special meaning here. > > I'd also name the constants differently, like LWLOCK_TRANCHE_XXX or > > something. LW_ is a somewhat ambiguous prefix. > > > > The idea of tranches is really that each tranche is an array of items > > each of which contains 1 or more lwlocks. Here you are intermingling > > different tranches. I guess that won't really break anything but it > > seems ugly. > > Maybe it will be better to split LWLockAssign to two functions then, keep > name > LWLockAssign for user defined locks and other function with tranche_id. > > > On Jul 25, 2015, at 1:54 PM, Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > That anyway he has to do it either you go by defining individual arrays > > or having unified WaitEventType enum for individual arrays he has to > > find out that array. Another thing is with that you can just encapsulate > > this information in one byte in structure PgBackendStatus, rather than > > using more number of bytes (4 bytes) and I think the function for > reporting > > Waitevent will be much more simplified. > > In my way there are no special meaning for names. Array with names > located in lwlock.c and lock.c, and can be used for other things (for > example > tracing). One byte sounds good only for this case. Do you mean to say that you need more than 256 events? I am not sure if we can add that many events without adding performance penalty in some path. The original idea was proposed for one-byte and the patch was written considering the same, now you are planning to extend (which is okay), but modifying it without any prior consent is what slightly a matter of concern. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
