Re: merging duplicate definitions of adjust_relid_set

On Fri, Mar 17, 2017 at 1:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> While reviewing Ashutosh Bapat's partitionwise join code, I noticed
>> he'd run up against the problem that adjust_relid_set() is defined as
>> static in two different source files, and he wanted to call it from a
>> third file. I didn't much like his solution to that problem, which
>> was to rename one of them and make that definition non-static; I think
>> it would be better to keep the existing name and stop defining it in
>> multiple places. However, I discovered that there wasn't really an
>> obviously-good place to put the function; neither prepunion.c nor
>> rewriteManip.c, the two files that contain static versions as of now,
>> seem like an appropriate place from which to expose it, and I didn't
>> find anything else that I was wildly in love with, either. The
>> attached patch puts it in var.c, because it didn't look horrible and I
>> thought it wasn't worth creating a new file just for this.
>
>> Objections, better ideas?
>
> I think it might be better to define it as a fundamental Bitmapset
> operation in nodes/bitmapset.c, along the lines of
>
> Bitmapset *bms_replace_member(const Bitmapset *bms, int member, int repl);
>
> This API would probably require giving up the property of not copying the
> set unless it changes, but I doubt that that's performance critical.

I thought of that, but I wasn't sure it was good in terms of clarity
to replace a function that works in terms of Relids with one that
works in terms of Bitmapset *. I know they're the same, so it's just
a style thing.

But actually, I think my email was premature. Actually, his patch
series first changes adjust_relid_set to take different arguments so
that it can do multiple translations at once, and then a later patch
in the series renames it. So there's actually no need to merge the
definitions, because one of them ends up going away anyway. So, uh,
never mind.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company