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 > >