Date: Wed, 5 Jun 2024 11:37:54 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Neil Conway <neil.conway@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: small pg_dump code cleanup
Message-ID: <ZmCUYtIhD5wEQR4s@nathan>
References: <ZmCAtVaOrHpf31PJ@nathan>
 <CAOW5sYZY3sdSxzaRwQCMMNtsZ_1jN+AFzB82k58==Vome-G11w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAOW5sYZY3sdSxzaRwQCMMNtsZ_1jN+AFzB82k58==Vome-G11w@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZmCUYtIhD5wEQR4s%40nathan>
Precedence: bulk

On Wed, Jun 05, 2024 at 12:22:03PM -0400, Neil Conway wrote:
> Nice cleanup! Two minor comments:

Thanks for taking a look.

> (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?

What about collectXXX() to match similar functions in pg_dump.c (e.g.,
collectRoleNames(), collectComments(), collectSecLabels())?

> (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.

That'd increase the number of allocations quite significantly, but I'd be
surprised if that was noticeable outside of extreme scenarios.  At the
moment, I'm inclined to leave these as-is for this reason and because I
doubt it'd result in much cleanup, but I'll yield to the majority opinion
here.

-- 
nathan