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

On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
>>
>> This also means that we have to test PREPARED statements involving
>> default partition. Any addition/deletion/attach/detach of other partition
>> should invalidate those cached statements.
>
>
> Will add this in next version of patch.

My earlier statement requires a clarification. By "PREPARED statements
involving default partition." I mean PREPAREd statements with direct
access to the default partition, without going through the partitioned
table.

>
>>
>> The code in check_default_allows_bound() to check whether the default
>> partition
>> has any rows that would fit new partition looks quite similar to the code
>> in
>> ATExecAttachPartition() checking whether all rows in the table being
>> attached
>> as a partition fit the partition bounds. One thing that
>> check_default_allows_bound() misses is, if there's already a constraint on
>> the
>> default partition refutes the partition constraint on the new partition,
>> we can
>> skip the scan of the default partition since it can not have rows that
>> would
>> fit the new partition. ATExecAttachPartition() has code to deal with a
>> similar
>> case i.e. the table being attached has a constraint which implies the
>> partition
>> constraint. There may be more cases which check_default_allows_bound()
>> does not
>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>> it's
>> better to somehow take out the common code into a function and use it. We
>> will
>> have to deal with a difference through. The first one would throw an error
>> when
>> finding a row that satisfies partition constraints whereas the second one
>> would
>> throw an error when it doesn't find such a row. But this difference can be
>> handled through a flag or by negating the constraint. This would also take
>> care
>> of Amit Langote's complaint about foreign partitions. There's also another
>> difference that the ATExecAttachPartition() queues the table for scan and
>> the
>> actual scan takes place in ATRewriteTable(), but there is not such queue
>> while
>> creating a table as a partition. But we should check if we can reuse the
>> code to
>> scan the heap for checking a constraint.
>>
>> In case of ATTACH PARTITION, probably we should schedule scan of default
>> partition in the alter table's work queue like what
>> ATExecAttachPartition() is
>> doing for the table being attached. That would fit in the way alter table
>> works.
>
>
> I am still working on this.
> But, about your comment here:
> "if there's already a constraint on the default partition refutes the
> partition
> constraint on the new partition, we can skip the scan":
> I am so far not able to imagine such a case, since default partition
> constraint
> can be imagined something like "minus infinity to positive infinity with
> some finite set elimination", and any new non-default partition being added
> would simply be a set of finite values(at-least in case of list, but I think
> range
> should not also differ here). Hence one cannot imply the other here.
> Possibly,
> I might be missing something that you had visioned when you raised the flag,
> please correct me if I am missing something.

I am hoping that this has been clarified in other mails in this thread
between you and Amul.

>
>>
>>      /* Generate the main expression, i.e., keyCol = ANY (arr) */
>>      opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
>> -                                    keyCol, (Expr *) arr);
>> +                                    keyCol, (Expr *) arr,
>> spec->is_default);
>>                  /* Build leftop = ANY (rightop) */
>>                  saopexpr = makeNode(ScalarArrayOpExpr);
>> The comments in both the places need correction, as for default partition
>> the
>> expression will be keyCol <> ALL(arr).
>
>
> Done.

Please note that this changes, if you construct the constraint as
!(keycol = ANY[]).

>
>> We have RelationGetPartitionDesc() for
>> that. Probably we should also add Asserts to check that every pointer in
>> the
>> long pointer chain is Non-null.
>
>
> I am sorry, but I did not understand which chain you are trying to point
> here.

The chain of pointers: a->b->c->d is a chain of pointers.

>
>>
>> @@ -2044,7 +2044,7 @@ psql_completion(const char *text, int start, int
>> end)
>>          COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
>>      /* Limited completion support for partition bound specification */
>>      else if (TailMatches3("ATTACH", "PARTITION", MatchAny))
>> -        COMPLETE_WITH_CONST("FOR VALUES");
>> +        COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT");
>>      else if (TailMatches2("FOR", "VALUES"))
>>          COMPLETE_WITH_LIST2("FROM (", "IN (");
>>
>> @@ -2483,7 +2483,7 @@ psql_completion(const char *text, int start, int
>> end)
>>          COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_partitioned_tables,
>> "");
>>      /* Limited completion support for partition bound specification */
>>      else if (TailMatches3("PARTITION", "OF", MatchAny))
>> -        COMPLETE_WITH_CONST("FOR VALUES");
>> +        COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT");
>> Do we include psql tab completion in the main feature patch? I have not
>> seen
>> this earlier. But appreciate taking care of these defails.
>
>
> I am not sure about this. If needed I can submit a patch to take care of
> this later, but
> as of now I have not removed this from the patch.

I looked at Amul's patch. He has tab completion changes for HASH
partitions and those were suggested by Robert. So, keep those changes
in this patch. Sorry for misunderstanding on my part.

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