Re: BufferAlloc: don't take two simultaneous locks

From: Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru>
To: Zhihong Yu <zyu(at)yugabyte(dot)com>
Cc: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, michail(dot)nikolaev(at)gmail(dot)com, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, Andres Freund <andres(at)anarazel(dot)de>, Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>
Subject: Re: BufferAlloc: don't take two simultaneous locks
Date: 2022-03-13 22:27:47
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

В Вс, 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


> + * We also reset the usage_count since any recency of use of the old
> recency of use -> recent use


> +BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse)
> Later on, there is code:
> Can flag (such as HASH_REUSE) be passed to BufTableDelete() instead of bool ? That way, flag can be used directly in the above place.

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?


Yura Sokolov

Attachment Content-Type Size
v7-bufmgr-lock-improvements.patch text/x-patch 27.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2022-03-13 22:31:03 Re: On login trigger: take three
Previous Message Nathan Bossart 2022-03-13 21:58:58 Re: add checkpoint stats of snapshot and mapping files of pg_logical dir