On Sun, Jul 30, 2017 at 8:07 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
> Hi Ashutosh,
>
> 0003 patch
>>
>> + parentRel = heap_open(parentOid, AccessExclusiveLock);
>> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
>> should not heap_open() the parent relation. But this patch still calls
>> heap_open() without giving any counter argument. Also I don't see
>> get_default_partition_oid() using Relation anywhere. If you remove that
>> heap_open() please remove following heap_close().
>
>
> I think the patch 0004 exactly does what you have said here, i.e. it gets
> rid of the heap_open() and heap_close().
> The question might be why I kept the patch 0004 a separate one, and the
> answer is I wanted to make it easier for review, and also keeping it that
> way would make it bit easy to work on a different approach if needed.
>
The reviewer has to review two different set of changes to the same
portion of the code. That just doubles the work. I didn't find that
simplifying review. As I have suggested earlier, let's define
get_default_partition_oid() only once, mostly in or before 0003 patch.
Having it in a separate patch would allow you to change its
implementation if needed.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company