MIME-Version: 1.0
References: 
 <CA+hUKGJ2-eaDqAum5bxhpMNhvuJmRDZxB_Tow0n-gse+HG0Yig@mail.gmail.com>
 <0F9ECBA9-1DF2-4C38-BB58-B17EEE6C7B3A@yesql.se>
 <CA+hUKGJhOtjQH+wjCodtJzHAFCAPYyt6Qm9E1mUoJph42qo1hg@mail.gmail.com>
 <20210311185847.zwb2rf3n4wzelkba@alap3.anarazel.de>
 <CA+hUKGLqmksGRKQRF6qkih6jV_d9qVrrk4wh5F_EgTXyRGeCyw@mail.gmail.com>
 <CA+hUKGKztHEWm676csTFjYzortziWmOcf8HDss2Zr0muZ2xfEg@mail.gmail.com>
 <CALNJ-vRSvudAbdY-DChFv8C0-dJYD3FamX4FpW9A2xivKuB83A@mail.gmail.com>
 <CA+hUKGKWR3_-sqTE8BSrSy0kp0WJeKQ1ffyYxEKuykA7vmG9uQ@mail.gmail.com>
 <CA+hUKGLAfouP-rUpik45Z_KPM5+oxrX_564bLfiZPX4f25MYbA@mail.gmail.com>
In-Reply-To: 
 <CA+hUKGLAfouP-rUpik45Z_KPM5+oxrX_564bLfiZPX4f25MYbA@mail.gmail.com>
From: John Naylor <john.naylor@enterprisedb.com>
Date: Mon, 28 Jun 2021 15:13:06 -0400
Message-ID: 
 <CAFBsxsFSTSrrhAyFQo6Kz4vP40iXyvUdvzU0LvHa4LVyK-eDfA@mail.gmail.com>
Subject: Re: A qsort template
To: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000002c80ec05c5d8486c"
Archived-At: 
 <https://www.postgresql.org/message-id/CAFBsxsFSTSrrhAyFQo6Kz4vP40iXyvUdvzU0LvHa4LVyK-eDfA%40mail.gmail.com>
Precedence: bulk

--0000000000002c80ec05c5d8486c
Content-Type: text/plain; charset="UTF-8"

On Wed, Jun 16, 2021 at 1:55 AM Thomas Munro <thomas.munro@gmail.com> wrote:
[v2 patch]

Hi Thomas,

I plan to do some performance testing with VACUUM, ANALYZE etc soon, to see
if I can detect any significant differences.

I did a quick check of the MacOS/clang binary size (no debug symbols):

master:    8108408
0001-0009: 8125224

Later, I'll drill down into the individual patches and see if anything
stands out.

There were already some comments for v2 upthread about formatting and an
overflow hazard, but I did find a few more things to ask about:

- For my curiosity, there are a lot of calls to qsort/qunique in the tree
-- without having looked exhaustively, do these patches focus on cases
where there are bespoke comparator functions and/or hot code paths?

- Aside from the qsort{_arg} precedence, is there a practical reason for
keeping the new global functions in their own files?

- 0002 / 0004

+/* Search and unique functions inline in header. */

The functions are pretty small, but is there some advantage for inlining
these?

- 0003

#include "lib/qunique.h" is not needed anymore.

This isn't quite relevant for the current patch perhaps, but I'm wondering
why we don't already call bsearch for RelationHasSysCache() and
RelationSupportsSysCache().

- 0008

+#define ST_COMPARE(a, b, cxt) \
+ DatumGetInt32(FunctionCall2Coll(&cxt->flinfo, cxt->collation, *a, *b))

This seems like a pretty heavyweight comparison, so I'm not sure inlining
buys us much, but it seems also there are fewer branches this way. I'll
come up with a test and see what happens.

--
John Naylor
EDB: http://www.enterprisedb.com

--0000000000002c80ec05c5d8486c
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">On Wed, Jun 16, 2021 at 1:55 AM Thomas Munro &lt;<a href=
=3D"mailto:thomas.munro@gmail.com">thomas.munro@gmail.com</a>&gt; wrote:<br=
>[v2 patch]<br><br><div>Hi Thomas,<br><br>I plan to do some performance tes=
ting with VACUUM, ANALYZE etc soon, to see if I can detect any significant =
differences.<br><br>I did a quick check of the MacOS/clang binary size (no =
debug symbols):<br><br>master: =C2=A0 =C2=A08108408<br>0001-0009: 8125224<b=
r><br>Later, I&#39;ll drill down into the individual patches and see if any=
thing stands out.<br><br>There were already some comments for v2 upthread a=
bout formatting and an overflow hazard, but I did find a few more things to=
 ask about:<br><br>- For my curiosity, there are a lot of calls to qsort/qu=
nique in the tree -- without having looked exhaustively, do these patches f=
ocus on cases where there are bespoke comparator functions and/or hot code =
paths?<br><br>- Aside from the qsort{_arg} precedence, is there a practical=
 reason for keeping the new global functions in their own files?<br><br>- 0=
002 / 0004 <br><br>+/* Search and unique functions inline in header. */<br>=
<br>The functions are pretty small, but is there some advantage for inlinin=
g these?<br><br>- 0003<br><br>#include &quot;lib/qunique.h&quot; is not nee=
ded anymore.<br><br>This isn&#39;t quite relevant for the current patch per=
haps, but I&#39;m wondering why we don&#39;t already call bsearch for Relat=
ionHasSysCache() and RelationSupportsSysCache().<br><br>- 0008<br><br>+#def=
ine ST_COMPARE(a, b, cxt) \<br>+	DatumGetInt32(FunctionCall2Coll(&amp;cxt-&=
gt;flinfo, cxt-&gt;collation, *a, *b))<br><br>This seems like a pretty heav=
yweight comparison, so I&#39;m not sure inlining buys us much, but it seems=
 also there are fewer branches this way. I&#39;ll come up with a test and s=
ee what happens.<br><br>--<br>John Naylor<br>EDB: <a href=3D"http://www.ent=
erprisedb.com">http://www.enterprisedb.com</a></div></div>

--0000000000002c80ec05c5d8486c--