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

On 1/9/15, 8:51 PM, Kohei KaiGai wrote:
> 2015-01-10 9:56 GMT+09:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>:
>> On 1/9/15, 6:54 PM, Jim Nasby wrote:
>>>
>>> On 1/9/15, 6:44 PM, Petr Jelinek wrote:
>>>>>>
>>>>>>
>>>>> Yep, I had a same impression when I looked at the code first time,
>>>>> however, it is defined as below. Not a manner of custom-scan itself.
>>>>>
>>>>> /*
>>>>> * ==========
>>>>> * Scan nodes
>>>>> * ==========
>>>>> */
>>>>> typedef struct Scan
>>>>> {
>>>>> Plan plan;
>>>>> Index scanrelid; /* relid is index into the range table
>>>>> */
>>>>> } Scan;
>>>>>
>>>>
>>>> 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.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com