Re: BufferAlloc: don't take two simultaneous locks

On Sun, Mar 13, 2022 at 3:27 PM Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru>
wrote:

> В Вс, 13/03/2022 в 07:05 -0700, Zhihong Yu пишет:
> >
> > Hi,
> > In the description:
> >
> > There is no need to hold both lock simultaneously.
> >
> > both lock -> both locks
>
> Thanks.
>
> > + * We also reset the usage_count since any recency of use of the old
> >
> > recency of use -> recent use
>
> Thanks.
>
> > +BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse)
> >
> > Later on, there is code:
> >
> > + reuse ? HASH_REUSE : HASH_REMOVE,
> >
> > Can flag (such as HASH_REUSE) be passed to BufTableDelete() instead of
> bool ? That way, flag can be used directly in the above place.
>
> No.
> BufTable* functions are created to abstract Buffer Table from dynahash.
> Pass of HASH_REUSE directly will break abstraction.
>
> > + long nalloced; /* number of entries initially allocated
> for
> >
> > nallocated isn't very long. I think it would be better to name the field
> nallocated 'nallocated'.
>
> It is debatable.
> Why not num_allocated? allocated_count? number_of_allocations?
> Same points for nfree.
> `nalloced` is recognizable and unambiguous. And there are a lot
> of `*alloced` in the postgresql's source, so this one will not
> be unusual.
>
> I don't see the need to make it longer.
>
> But if someone supports your point, I will not mind to changing
> the name.
>
> > + sum += hashp->hctl->freeList[i].nalloced;
> > + sum -= hashp->hctl->freeList[i].nfree;
> >
> > I think it would be better to calculate the difference between nalloced
> and nfree first, then add the result to sum (to avoid overflow).
>
> Doesn't really matter much, because calculation must be valid
> even if all nfree==0.
>
> I'd rather debate use of 'long' in dynahash at all: 'long' is
> 32bit on 64bit Windows. It is better to use 'Size' here.
>
> But 'nelements' were 'long', so I didn't change things. I think
> it is place for another patch.
>
> (On the other hand, dynahash with 2**31 elements is at least
> 512GB RAM... we doubtfully trigger problem before OOM killer
> came. Does Windows have an OOM killer?)
>
> > Subject: [PATCH 3/3] reduce memory allocation for non-partitioned
> dynahash
> >
> > memory allocation -> memory allocations
>
> For each dynahash instance single allocation were reduced.
> I think, 'memory allocation' is correct.
>
> Plural will be
> reduce memory allocations for non-partitioned dynahashes
> ie both 'allocations' and 'dynahashes'.
> Am I wrong?
>
> Hi,
bq. reduce memory allocation for non-partitioned dynahash

It seems the following is clearer:

reduce one memory allocation for every non-partitioned dynahash

Cheers