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

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

At the moment, I'm inclined to commit v1 once v18 development opens up.  We
can consider any additional adjustments separately.

-- 
nathan