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