Date: Sun, 21 Apr 2019 20:34:08 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Strange coding in mvdistinct.c
Message-ID: <20190421183408.lj7r4yr5icqrpu5r@development>
References: <29785.1555365602@sss.pgh.pa.us>
 <30400.1555366344@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <30400.1555366344@sss.pgh.pa.us>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk

On Mon, Apr 15, 2019 at 06:12:24PM -0400, Tom Lane wrote:
>Oh, and as I continue to grep, I found this in dependencies.c:
>
>            dependencies = (MVDependencies *) repalloc(dependencies,
>                                                       offsetof(MVDependencies, deps)
>                                                       + dependencies->ndeps * sizeof(MVDependency));
>
>I'm pretty sure this is an actual bug: the calculation should be
>
>                       offsetof(MVDependencies, deps)
>                       + dependencies->ndeps * sizeof(MVDependency *));
>
>because deps is an array of MVDependency* not MVDependency.
>
>This would lead to an overallocation not underallocation, and it's
>probably pretty harmless because ndeps can't get too large (I hope;
>if it could, this would have O(N^2) performance problems).  Still,
>you oughta fix it.
>
>(There's a similar calculation later in the file that gets it right.)
>

I've pushed a fix correcting those issues - both for mvndistinct and
functional dependencies. I've reworked the macros used to compute the
serialized sizes not to use offsetof(), which however made them pretty
useless for other purposes. So I've made them private by moving them to
the .c files where they are moved them to the .c files.

I don't think we need to backpatch this - we're not going to add new
fields in backbranches, so there's little danger of introducing padding.
And I'm not sure we want to remove the macros, although it's unlikely
anyone else uses them.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services