Thanks Ashutosh and Kyotaro for reviewing further.
I shall address your comments in next version of my patch.
Regards,
Jeevan Ladhe
On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
> 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...@lab.ntt.co.jp> wrote in <a1267081-6e9a-e570-f6cf-
> 34ff801bf...@lab.ntt.co.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
>
>
