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
