On Sun, Jul 30, 2017 at 8:07 AM, Jeevan Ladhe <jeevan.la...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
