From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: Neil Conway <neil.conway@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: small pg_dump code cleanup
In-reply-to: <ZmCUYtIhD5wEQR4s@nathan>
References: <ZmCAtVaOrHpf31PJ@nathan> <CAOW5sYZY3sdSxzaRwQCMMNtsZ_1jN+AFzB82k58==Vome-G11w@mail.gmail.com> <ZmCUYtIhD5wEQR4s@nathan>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Wed, 05 Jun 2024 11:37:54 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <892530.1717610334.1@sss.pgh.pa.us>
Date: Wed, 05 Jun 2024 13:58:54 -0400
Message-ID: <892531.1717610334@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/892531.1717610334%40sss.pgh.pa.us>
Precedence: bulk

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Jun 05, 2024 at 12:22:03PM -0400, Neil Conway wrote:
>> (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())?

Personally I see nothing much wrong with leaving them as getXXX.

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

I think that would be quite an invasive change; it would require
many hundreds of edits like

-		finfo[i].dobj.objType = DO_FUNC;
+		finfo->dobj.objType = DO_FUNC;

which aside from being tedious would create a back-patching hazard.
So I'm kind of -0.1 or so.

Another angle to this is that Coverity and possibly other tools tend
to report that these functions leak these allocations, apparently
because they don't notice that pointers into the allocations get
stored in hash tables by a subroutine.  I'm not sure if making this
change would make that worse or better.  If we really want to change
it, that might be worth checking somehow before we jump.

			regards, tom lane