merging duplicate definitions of adjust_relid_set

Robert Haas <robertmhaas@gmail.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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Mar 17, 2017 at 1:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers