Re: [HACKERS] Adding support for Default partition in partitioning

On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> Some more comments on the latest set of patches.
>>
>> In heap_drop_with_catalog(), we heap_open() the parent table to get the
>> default partition OID, if any. If the relcache doesn't have an entry for the
>> parent, this means that the entry will be created, only to be invalidated at
>> the end of the function. If there is no default partition, this all is
>> completely unnecessary. We should avoid heap_open() in this case. This also
>> means that get_default_partition_oid() should not rely on the relcache entry,
>> but should growl through pg_inherit to find the default partition.
>
> I am *entirely* unconvinced by this line of argument.  I think we want
> to open the relation the first time we touch it and pass the Relation
> around thereafter.

If this would be correct, why heap_drop_with_catalog() without this
patch just locks the parent and doesn't call a heap_open(). I am
missing something.

> Anything else is prone to accidentally failing to
> have the relation locked early enough,

We are locking the parent relation even without this patch, so this
isn't an issue.

> or looking up the OID in the
> relcache multiple times.

I am not able to understand this in the context of default partition.
After that nobody else is going to change its partitions and their
bounds (since both of those require heap_open on parent which would be
stuck on the lock we hold.). So, we have to check only once if the
table has a default partition. If it doesn't, it's not going to
acquire one unless we release the lock on the parent i.e at the end of
transaction. If it has one, it's not going to get dropped till the end
of the transaction for the same reason. I don't see where we are
looking up OIDs multiple times.


>
>> +    defaultPartOid = get_default_partition_oid(rel);
>> +    if (OidIsValid(defaultPartOid))
>> +        ereport(ERROR,
>> +                (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> +                 errmsg("there exists a default partition for table
>> \"%s\", cannot attach a new partition",
>> +                        RelationGetRelationName(rel))));
>> +
>> Should be done before heap_open on the table being attached. If we are not
>> going to attach the partition, there's no point in instantiating its relcache.
>
> No, because we should take the lock before examining any properties of
> the table.

There are three tables involved here. "rel" which is the partitioned
table. "attachrel" is the table being attached as a partition to "rel"
and defaultrel, which is the default partition table. If there exists
a default partition in "rel" we are not allowing "attachrel" to be
attached to "rel". If that's the case, we don't need to examine any
properties of "attachrel" and hence we don't need to instantiate
relcache of "attachrel". That's what the comment is about.
ATExecAttachPartition() receives "rel" as an argument which has been
already locked and opened. So, we can check the existence of default
partition right at the beginning of the function.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company