Re: Adding support for Default partition in partitioning

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp
Cc: robertmhaas(at)gmail(dot)com, jeevan(dot)ladhe(at)enterprisedb(dot)com, ashutosh(dot)bapat(at)enterprisedb(dot)com, memissemerson(at)gmail(dot)com, rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com, rahilasyed90(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Adding support for Default partition in partitioning
Date: 2017-06-16 08:16:08
Message-ID: 20170616.171608.49564604.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello, I'd like to review this but it doesn't fit the master, as
Robert said. Especially the interface of predicate_implied_by is
changed by the suggested commit.

Anyway I have some comment on this patch with fresh eyes. I
believe the basic design so my comment below are from a rather
micro viewpoint.

At Thu, 15 Jun 2017 16:01:53 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <a1267081-6e9a-e570-f6cf-34ff801bf503(at)lab(dot)ntt(dot)co(dot)jp>
> Oops, I meant to send one more comment.
>
> On 2017/06/15 15:48, Amit Langote wrote:
> > BTW, I noticed the following in 0002
> + errmsg("there exists a default partition for table \"%s\", cannot
> add a new partition",
>
> This error message style seems novel to me. I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
>
> Note that the comment applies to both DefineRelation and
> ATExecAttachPartition.

- Considering on how canSkipPartConstraintValidation is called, I
*think* that RelationProvenValid() would be better. (But this
would be disappear by rebasing..)

- 0002 changes the interface of get_qual_for_list, but left
get_qual_for_range alone. Anyway get_qual_for_range will have
to do the similar thing soon.

- In check_new_partition_bound, "overlap" and "with" is
completely correlated with each other. "with > -1" means
"overlap = true". So overlap is not useless. ("with" would be
better to be "overlap_with" or somehting if we remove
"overlap")

- The error message of check_default_allows_bound is below.

"updated partition constraint for default partition \"%s\"
would be violated by some row"

This looks an analog of validateCheckConstraint but as my
understanding this function is called only when new partition
is added. This would be difficult to recognize in the
situation.

"the default partition contains rows that should be in
the new partition: \"%s\""

or something?

- In check_default_allows_bound, the iteration over partitions is
quite similar to what validateCheckConstraint does. Can we
somehow share validateCheckConstraint with this function?

- In the same function, skipping RELKIND_PARTITIONED_TABLE is
okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
good. I think at least some warning should be emitted.

"Skipping foreign tables in the defalut partition. It might
contain rows that should be in the new partition." (Needs
preventing multple warnings in single call, maybe)

- In the same function, the following condition seems somewhat
strange in comparison to validateCheckConstraint.

> if (partqualstate && ExecCheck(partqualstate, econtext))

partqualstate won't be null as long as partition_constraint is
valid. Anyway (I'm believing that) an invalid constraint
results in error by ExecPrepareExpr. Therefore 'if
(partqualstate' is useless.

- In gram.y, the nonterminal for list spec clause is still
"ForValues". It seems somewhat strange. partition_spec or
something would be better.

- This is not a part of this patch, but in ruleutils.c, the error
for unknown paritioning strategy is emitted as following.

> elog(ERROR, "unrecognized partition strategy: %d",
> (int) strategy);

The cast is added because the strategy is a char. I suppose
this is because strategy can be an unprintable. I'd like to see
a comment if it is correct.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2017-06-16 08:16:39 Re: Get stuck when dropping a subscription during synchronizing table
Previous Message Konstantin Knizhnik 2017-06-16 08:06:39 Re: WIP: Data at rest encryption