Thread: [HACKERS] merging duplicate definitions of adjust_relid_set
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? -- 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
Attachment
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
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