On 3 August 2017 at 11:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the review.
>
> On 2017/08/03 13:54, Amit Khandekar wrote:
>> On 2 August 2017 at 11:51, Amit Langote wrote:
>>> On 2017/08/02 1:33, Amit Khandekar wrote:
>>>> Instead of callers of map_partition_varattnos() reporting error, we
>>>> can have map_partition_varattnos() itself report error. Instead of the
>>>> found_whole_row parameter of map_partition_varattnos(), we can have
>>>> error_on_whole_row parameter. So callers who don't expect whole row,
>>>> would pass error_on_whole_row=true to map_partition_varattnos(). This
>>>> will simplify the resultant code a bit.
>>>
>>> So, I think it's only the callers of
>>> map_partition_varattnos() who know whether finding whole-row vars is an
>>> error and *what kind* of error.
>>
>> Ok. Got it. Thanks for the explanation.
>>
>> How about making found_whole_row as an optional parameter ?
>> map_partition_varattnos() would populate it only if its not NULL. This
>> way, callers who don't bother about whether it is found or not don't
>> have to declare a variable and pass its address. In current code,
>> ExecInitModifyTable() is the one who has to declare found_whole_row
>> without needing it.
>
> Sure, done.
Looks good.
> But while implementing that, I avoided changing anything in
> map_variable_attnos(), such as adding a check for not-NULLness of
> found_whole_row. The only check added is in map_partition_varattnos().
Yes, I agree. That is anyway not relevant to the fix.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company