On 2019/01/11 11:07, Amit Langote wrote:
> On 2019/01/11 6:47, David Rowley wrote:
>> On Fri, 11 Jan 2019 at 06:56, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>> Pushed 0001 with some minor tweaks, thanks.
>>
>> Thanks for pushing. I had looked at 0001 last night and there wasn't
>> much to report, just:
>>
>> v12 0001
>>
>> 1. I see you've moved translate_col_privs() out of prepunion.c into
>> appendinfo.c. This required making it an external function, but it's
>> only use is in inherit.c, so would it not be better to put it there
>> and keep it static?
>
> Actually, I *was* a bit puzzled where to put it. I tend to agree with you
> now that it might be define it locally within inherit.c as it might not be
> needed elsewhere. Let's hear what Alvaro thinks. I'm attaching a patch
> anyway.
>
>> 2. The following two lines I think need to swap their order.
>>
>> +#include "utils/rel.h"
>> +#include "utils/lsyscache.h"
>
> Oops, fixed.
>
>> Both are pretty minor details but thought I'd post them anyway.
>
> Thank you for reporting.
>
> Attached find the patch.
Looking around a bit more, I started thinking even build_child_join_sjinfo
doesn't belong in appendinfo.c (just to be clear, it was defined in
prepunion.c before this commit), so maybe we should move it to joinrels.c
and instead export adjust_child_relids that's required by it from
appendinfo.c. There's already adjust_child_relids_multilevel in
appendinfo.h, so having adjust_child_relids next to it isn't too bad. At
least not as bad as appendinfo.c exporting build_child_join_sjinfo for
joinrels.c to use.
I have updated the patch.
Thanks,
Amit
