Re: multi-column range partition constraint

On 2017/05/10 12:08, Robert Haas wrote:
> On Mon, May 8, 2017 at 2:59 AM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Yes, disallowing this in the first place is the best thing to do.
>> Attached patch 0001 implements that. With the patch:
>
> Committed.

Thanks.

> With regard to 0002, some of the resulting constraints are a bit more
> complicated than seems desirable:
>
> create table foo1 partition of foo for values from (unbounded,
> unbounded, unbounded) to (1, unbounded, unbounded);
> yields:
> Partition constraint: CHECK (((a < 1) OR (a = 1) OR (a = 1)))
>
> It would be better not to have (a = 1) in there twice, and better
> still to have the whole thing as (a <= 1).
>
> create table foo2 partition of foo for values from (3, 4, 5) to (6, 7,
> unbounded);
> yields:
> Partition constraint: CHECK ((((a > 3) OR ((a = 3) AND (b > 4)) OR ((a
> = 3) AND (b = 4) AND (c >= 5))) AND ((a < 6) OR ((a = 6) AND (b < 7))
> OR ((a = 6) AND (b = 7)))))
>
> The first half of that (for the lower bound) is of course fine, but
> the second half could be written better using <=, like instead of
>
> ((a = 6) AND (b < 7)) OR ((a = 6) AND (b = 7))
> you could have:
> ((a = 6) AND (b <= 7)
>
> This isn't purely cosmetic because the simpler constraint is probably
> noticeably faster to evaluate.

I think that makes sense. I modified things such that a simpler
constraint expression as you described above results in the presence of
UNBOUNDED values.

> I think you should have a few test cases like this in the patch - that
> is, cases where some but not all bounds are finite.

Added tests like this in insert.sql and then in the second patch as well.

>
>> BTW, is it strange that the newly added pg_get_partition_constraintdef()
>> requires the relcache entry to be created for the partition and all of its
>> ancestor relations up to the root (I mean the fact that the relcache entry
>> needs to be created at all)? I can see only one other function,
>> set_relation_column_names(), creating the relcache entry at all.
>
> I suggest that you display this information only when "verbose" is set
> - i.e. \d+ not just \d. I don't intrinsically care think that forcing
> the relcache entry to be built here, but note that it means this will
> block when the parent is locked. Between that and the fact that this
> information will only sometimes be of interest, restricting it to \d+
> seems preferable.

OK, done.

> Next update on this issue by Thursday 5/11.

Attached updated patches.

Thanks,
Amit