MIME-Version: 1.0
References: <ZmCAtVaOrHpf31PJ@nathan>
In-Reply-To: <ZmCAtVaOrHpf31PJ@nathan>
From: Neil Conway <neil.conway@gmail.com>
Date: Wed, 5 Jun 2024 12:22:03 -0400
Message-ID: <CAOW5sYZY3sdSxzaRwQCMMNtsZ_1jN+AFzB82k58==Vome-G11w@mail.gmail.com>
Subject: Re: small pg_dump code cleanup
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="0000000000004ad42d061a26f849"
Archived-At: <https://www.postgresql.org/message-id/CAOW5sYZY3sdSxzaRwQCMMNtsZ_1jN%2BAFzB82k58%3D%3DVome-G11w%40mail.gmail.com>
Precedence: bulk

--0000000000004ad42d061a26f849
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 5, 2024 at 11:14=E2=80=AFAM Nathan Bossart <nathandbossart@gmai=
l.com>
wrote:

>  In fact, many of the functions in this area don't actually need to

return anything, so we can trim some code and hopefully reduce confusion a
> bit.  Patch attached.
>

Nice cleanup! Two minor comments:

(1) Names like `getXXX` for these functions suggest to me that they return
a value, rather than side-effecting. I realize some variants continue to
return a value, but the majority no longer do. Perhaps a name like
lookupXXX() or readXXX() would be clearer?

(2) These functions malloc() a single ntups * sizeof(struct) allocation and
then index into it to fill-in each struct before entering it into the hash
table. It might be more straightforward to just malloc each individual
struct.

Neil

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

<div dir=3D"ltr"><div dir=3D"ltr"></div><div class=3D"gmail_quote"><div dir=
=3D"ltr" class=3D"gmail_attr">On Wed, Jun 5, 2024 at 11:14=E2=80=AFAM Natha=
n Bossart &lt;<a href=3D"mailto:nathandbossart@gmail.com">nathandbossart@gm=
ail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"=
margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-lef=
t:1ex">=C2=A0In fact, many of the functions in this area don&#39;t actually=
 need to</blockquote><blockquote class=3D"gmail_quote" style=3D"margin:0px =
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
return anything, so we can trim some code and hopefully reduce confusion a<=
br>
bit.=C2=A0 Patch attached.<br></blockquote><div><br></div><div>Nice cleanup=
! Two minor comments:</div><div><br></div><div>(1) Names like `getXXX` for =
these functions suggest=C2=A0to me that they return a value, rather than si=
de-effecting. I realize some variants continue to return a value, but the m=
ajority no longer do. Perhaps a name like lookupXXX() or readXXX() would be=
 clearer?</div><div><br></div><div>(2) These functions malloc() a single nt=
ups * sizeof(struct) allocation and then index into it to fill-in each stru=
ct before entering it into the hash table. It might be more straightforward=
 to just malloc each individual struct.</div><div><br></div><div>Neil</div>=
<div><br></div></div></div>

--0000000000004ad42d061a26f849--