Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)

On 11/01/15 08:56, Kohei KaiGai wrote:
> 2015-01-11 10:40 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>:
>>>>>>
>>>>>> Yeah there are actually several places in the code where "relid" means
>>>>>> index in range table and not oid of relation, it still manages to
>>>>>> confuse
>>>>>> me. Nothing this patch can do about that.
>>>>>
>>>>> Well, since it's confused 3 of us now... should we change it (as a
>>>>> separate patch)? I'm willing to do that work but don't want to waste
>>>>> time if
>>>>> it'll just be rejected.
>>>>>
>>>>> Any other examples of this I should fix too?
>>>>
>>>> Sorry, to clarify... any other items besides Scan.scanrelid that I should
>>>> fix?
>>>>
>>> This naming is a little bit confusing, however, I don't think it "should"
>>> be
>>> changed because this structure has been used for a long time, so reworking
>>> will prevent back-patching when we find bugs around "scanrelid".
>>
>> We can still backpatch; it just requires more work. And how many bugs do we
>> actually expect to find around this anyway?
>>
>> If folks think this just isn't worth fixing fine, but I find the
>> backpatching argument rather dubious.
>>
> Even though here is no problem around Scan structure itself, a bugfix may
> use the variable name of "scanrelid" to fix it. If we renamed it on v9.5, we
> also need a little adjustment to apply this bugfix on prior versions.
> It seems to me a waste of time for committers.
>

I tend to agree, especially as there is multiple places in code this
would affect - RelOptInfo and RestrictInfo have same issue, etc.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services