Re: Adding support for Default partition in partitioning

Hi,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.

On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI <
horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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.
>
> - Considering on how canSkipPartConstraintValidation is called, I
> *think* that RelationProvenValid() would be better. (But this
> would be disappear by rebasing..)
>
> I think RelationProvenValid() is bit confusing in the sense that, it does
not
imply the meaning that some constraint is being checke

> - 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.
>
> Yes, this will be taken care with default partition for range.

> - 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")
>
> Agree, probably this can be taken as a separate refactoring patch.
Currently,
for in case of default I have got rid of "overlap", and now use of "with"
and
that is also used just for code simplification.

> - 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?
>
> I think the current error message is more clearer. Agree that there might
be
sort of confusion if it's due to addition or attach partition, but we have
already stretched the message longer. I am open to suggestions here.

> - In check_default_allows_bound, the iteration over partitions is
> quite similar to what validateCheckConstraint does. Can we
> somehow share validateCheckConstraint with this function?
>
> May be we can, but I think again this can also be categorized as
refactoring
patch and done later maybe. Your thoughts?

> - 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)
>
> Currently we do not emit any warning when attaching a foreign table as a
non-default partition having rows that do not match its partition
constraints
and we still let attach the partition.
But, I agree that we should emit such a warning, I added a code to do this.

> - 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.
>
> Removed the check for partqualstate.

> - In gram.y, the nonterminal for list spec clause is still
> "ForValues". It seems somewhat strange. partition_spec or
> something would be better.
>
> Done.
Thanks for catching this, I agree with you.
I have changed the name to PartitionBoundSpec.

> - 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.
>
> I think this should be taken separately.

Thanks,
Jeevan Ladhe

Refs:
[1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X%
2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com