|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
В Вс, 13/03/2022 в 07:05 -0700, Zhihong Yu пишет:
> 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:
> + 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.
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
I don't see the need to make it longer.
But if someone supports your point, I will not mind to changing
> + 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?
|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|