Date: Fri, 11 Mar 2022 15:30:30 +0900 (JST)
Message-Id: <20220311.153030.1881495127262295773.horikyota.ntt@gmail.com>
To: y.sokolov@postgrespro.ru
Cc: pgsql-hackers@lists.postgresql.org, michail.nikolaev@gmail.com,
 x4mmm@yandex-team.ru, andres@anarazel.de, simon.riggs@enterprisedb.com
Subject: Re: BufferAlloc: don't take two simultaneous locks
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <e70ea8843dcc94cdb8e7892c5140d683f75e4baf.camel@postgrespro.ru>
References: <CANbhV-H-Nd9B_dhvUSTKfHvLNNjCe58U-=ENCMqHnuxe9sbiKg@mail.gmail.com>
	<a033d901ed0120ab5bf7562d03a5bea3e33dacb6.camel@postgrespro.ru>
	<e70ea8843dcc94cdb8e7892c5140d683f75e4baf.camel@postgrespro.ru>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=koi8-r
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/20220311.153030.1881495127262295773.horikyota.ntt%40gmail.com>
Precedence: bulk

At Thu, 03 Mar 2022 01:35:57 +0300, Yura Sokolov <y.sokolov@postgrespro=
.ru> wrote in =

> =F7 =F7=D4, 01/03/2022 =D7 10:24 +0300, Yura Sokolov =D0=C9=DB=C5=D4:=

> > Ok, here is v4.
> =

> And here is v5.
> =

> First, there was compilation error in Assert in dynahash.c .
> Excuse me for not checking before sending previous version.
> =

> Second, I add third commit that reduces HASHHDR allocation
> size for non-partitioned dynahash:
> - moved freeList to last position
> - alloc and memset offset(HASHHDR, freeList[1]) for
>   non-partitioned hash tables.
> I didn't benchmarked it, but I will be surprised if it
> matters much in performance sence.
> =

> Third, I put all three commits into single file to not
> confuse commitfest application.

Thanks!  I looked into dynahash part.

 struct HASHHDR
 {
-	/*
-	 * The freelist can become a point of contention in high-concurrency =
hash

Why did you move around the freeList?


-	long		nentries;		/* number of entries in associated buckets */
+	long		nfree;			/* number of free entries in the list */
+	long		nalloced;		/* number of entries initially allocated for

Why do we need nfree?  HASH_ASSING should do the same thing with
HASH_REMOVE.  Maybe the reason is the code tries to put the detached
bucket to different free list, but we can just remember the
freelist_idx for the detached bucket as we do for hashp.  I think that
should largely reduce the footprint of this patch.

-static void hdefault(HTAB *hashp);
+static void hdefault(HTAB *hashp, bool partitioned);

That optimization may work even a bit, but it is not irrelevant to
this patch?

+		case HASH_REUSE:
+			if (currBucket !=3D NULL)
+			{
+				/* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
+				Assert(DynaHashReuse.hashp =3D=3D NULL);
+				Assert(DynaHashReuse.element =3D=3D NULL);

I think all cases in the switch(action) other than HASH_ASSIGN needs
this assertion and no need for checking both, maybe only for element
would be enough.

regards.

-- =

Kyotaro Horiguchi
NTT Open Source Software Center