no partition pruning when partitioning using array type

On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I noticed that if you partition using a array type column, partition
pruning using constraint exclusion fails to work due to a minor problem.

Example:

create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');
create table p1 partition of p for values in ('{2, 3}', '{4, 5}');

explain select a from p where a = '{1}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])

explain select a from p where a = '{2, 3}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
(5 rows)

In the case of array type partition key, make_partition_op_expr() will
have to put a RelabelType node on top of the partition key Var, after
having selected an = operator from the array_ops family. The RelabelType
causes operator_predicate_proof() to fail to consider predicate leftop and
clause leftop as equal, because only one of them ends up having the
RelabelType attached to it.

As a simple measure, the attached patch teaches operator_predicate_proof()
to strip RelabelType nodes from all the nodes it compares using equal().
I also added a relevant test in partition_prune.sql.

I guess the question is whether that's guaranteed to be safe. I spent
a little bit of time thinking about it and I don't see a problem. The
function is careful to check that the opclasses and collations of the
OpExprs are compatible, and it is the behavior of the operator that is
in question here, not the column type, so your change seems OK to me.
But I hope somebody else will also study this, because this stuff is
fairly subtle and I would not like to be the one who breaks it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017/12/09 3:46, Robert Haas wrote:

On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I noticed that if you partition using a array type column, partition
pruning using constraint exclusion fails to work due to a minor problem.

Example:

create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');
create table p1 partition of p for values in ('{2, 3}', '{4, 5}');

explain select a from p where a = '{1}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])

explain select a from p where a = '{2, 3}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
(5 rows)

In the case of array type partition key, make_partition_op_expr() will
have to put a RelabelType node on top of the partition key Var, after
having selected an = operator from the array_ops family. The RelabelType
causes operator_predicate_proof() to fail to consider predicate leftop and
clause leftop as equal, because only one of them ends up having the
RelabelType attached to it.

As a simple measure, the attached patch teaches operator_predicate_proof()
to strip RelabelType nodes from all the nodes it compares using equal().
I also added a relevant test in partition_prune.sql.

I guess the question is whether that's guaranteed to be safe. I spent
a little bit of time thinking about it and I don't see a problem. The
function is careful to check that the opclasses and collations of the
OpExprs are compatible, and it is the behavior of the operator that is
in question here, not the column type, so your change seems OK to me.
But I hope somebody else will also study this, because this stuff is
fairly subtle and I would not like to be the one who breaks it.

Thanks for taking a look at it.

I will try to say a little more on why it seems safe. RelabelType node
exists (if any) on top of a given expression node only to denote that the
operator for which the node is an input will interpret its result as of
the type RelableType.resulttype, instead of the node's original type. No
conversion of values actually occurs before making any decisions that this
function is in charge of making, because the mismatching types in question
are known to be binary coercible. Or more to the point, the operator that
will be used in the proof will give correct answers for the values without
having to do any conversion of values. IOW, it's okay if we simply drop
the RelabelType, because it doesn't alter in any way the result of the
proof that operator_predicate_proof() performs.

That said, I've to come think in this particular case that the
partitioning code that generates the predicate expression should be a bit
smarter about the various types it manipulates such that RelabelType won't
be added in the first place. In contrast, make_op(), that generates an
OpExpr from the parser representation of a = '{1}' appearing in the
query's WHERE clause, won't add the RelabelType because the underlying
type machinery that it invokes is able to conclude that that's
unnecessary. The original patch may still be worth considering as a
solution though.

I hope someone else chimes in as well. :)

Thanks,
Amit

On 2017/12/11 14:31, Amit Langote wrote:

On 2017/12/09 3:46, Robert Haas wrote:

On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote wrote:

I noticed that if you partition using a array type column, partition
pruning using constraint exclusion fails to work due to a minor problem.

I guess the question is whether that's guaranteed to be safe. I spent
a little bit of time thinking about it and I don't see a problem. The
function is careful to check that the opclasses and collations of the
OpExprs are compatible, and it is the behavior of the operator that is
in question here, not the column type, so your change seems OK to me.
But I hope somebody else will also study this, because this stuff is
fairly subtle and I would not like to be the one who breaks it.

Thanks for taking a look at it.

I will try to say a little more on why it seems safe. RelabelType node
exists (if any) on top of a given expression node only to denote that the
operator for which the node is an input will interpret its result as of
the type RelableType.resulttype, instead of the node's original type. No
conversion of values actually occurs before making any decisions that this
function is in charge of making, because the mismatching types in question
are known to be binary coercible. Or more to the point, the operator that
will be used in the proof will give correct answers for the values without
having to do any conversion of values. IOW, it's okay if we simply drop
the RelabelType, because it doesn't alter in any way the result of the
proof that operator_predicate_proof() performs.

That said, I've to come think in this particular case that the
partitioning code that generates the predicate expression should be a bit
smarter about the various types it manipulates such that RelabelType won't
be added in the first place. In contrast, make_op(), that generates an
OpExpr from the parser representation of a = '{1}' appearing in the
query's WHERE clause, won't add the RelabelType because the underlying
type machinery that it invokes is able to conclude that that's
unnecessary. The original patch may still be worth considering as a
solution though.

I hope someone else chimes in as well. :)

Bug #15042 [1]/messages/by-id/151747550943.1247.2111555422760537959@wrigleys.postgresql.org seems to be caused by this same problem. There, a
RelabelType node is being slapped (by the partitioning code) on a Var node
of a partition key on enum.

Attached updated patch.

Thanks,
Amit

[1]: /messages/by-id/151747550943.1247.2111555422760537959@wrigleys.postgresql.org
/messages/by-id/151747550943.1247.2111555422760537959@wrigleys.postgresql.org

On Thu, Feb 1, 2018 at 4:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I hope someone else chimes in as well. :)

Bug #15042 [1] seems to be caused by this same problem. There, a
RelabelType node is being slapped (by the partitioning code) on a Var node
of a partition key on enum.

Attached updated patch.

Can I get anyone else to weigh in on whether this is likely to be
safe? Paging people who understand constraint exclusion...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018/02/02 0:20, Robert Haas wrote:

On Thu, Feb 1, 2018 at 4:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

I hope someone else chimes in as well. :)

Bug #15042 [1] seems to be caused by this same problem. There, a
RelabelType node is being slapped (by the partitioning code) on a Var node
of a partition key on enum.

Attached updated patch.

Can I get anyone else to weigh in on whether this is likely to be
safe? Paging people who understand constraint exclusion...

Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).

It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.

So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.

Thanks,
Amit

On 2018/03/01 17:16, Amit Langote wrote:

Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).

It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.

So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.

I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.

In HEAD, since we already fixed that case in e5dcbb88a15d [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.

Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this. Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed. Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.

Thanks,
Amit

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d

On 05/08/2018 02:18 AM, Amit Langote wrote:

On 2018/03/01 17:16, Amit Langote wrote:

Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).

It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.

So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.

I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.

In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.

Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this. Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed. Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.

Thanks,
Amit

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d

Since this email we have branched off REL_11_STABLE. Do we want to
consider this as a bug fix for Release 11? If so, should we add it to
the open items list?

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-May-08, Amit Langote wrote:

I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.

Interesting patchset. Didn't read your previous v2, v3 versions; I only
checked your latest, v1 (???).

I'm wondering about the choice of OIDs in the new test. I wonder if
it's possible to get ANYNONARRAY (or others) by way of having a
polymorphic function that passes its polymorphic argument in a qual. I
suppose it won't do anything in v10, or will it? Worth checking :-)
Why not use IsPolymorphicType? Also, I think it'd be good to have tests
for all these cases (even in v10), just to make sure we don't break it
going forward. At least the array case is clearly broken today ...
A test for the RECORDOID case would be particularly welcome, since it's
somehow different from the other cases. (I didn't understand why did
you remove the test in the latest version.)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018/07/07 0:13, Andrew Dunstan wrote:

On 05/08/2018 02:18 AM, Amit Langote wrote:

On 2018/03/01 17:16, Amit Langote wrote:

Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).

It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes.  Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases.  That is, in the case of having an array, record,
enum, and range type as the partition key.  No other part of the system
ends up adding one as far as I can see.  Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.

So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases.  Please find it attached.

I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD.  In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.

In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved.  The change can be seen in
the updated test output for create_table test.

Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this.  Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed.  Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.

Thanks,
Amit

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d

Since this email we have branched off REL_11_STABLE. Do we want to
consider this as a bug fix for Release 11? If so, should we add it to the
open items list?

The code being fixed with the latest patch is not new in PG 11, it's
rather PG 10 code. That code affects how pruning works in PG 10 (used to
affect PG 11 until we rewrote the partition pruning code). I think it's a
good idea to apply this patch to all branches, as the code is not specific
to partition pruning and has its benefits even for HEAD and PG 11.

For PG 11 and HEAD, the benefit of this patch can be thought of as just
cosmetic, because it only affects the ruleutils.c's deparse output for
partition constraint expressions when showing it in the psql output for
example.

In PG 10, it directly affects the planner behavior whereby having a
RelabelType redundantly in a partition constraint expression limits the
planner's ability to do partition pruning with it.

Thanks,
Amit

Thanks for taking a look.

On 2018/07/07 9:19, Alvaro Herrera wrote:

On 2018-May-08, Amit Langote wrote:

I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.

Interesting patchset. Didn't read your previous v2, v3 versions; I only
checked your latest, v1 (???).

Sorry, I think I messed up version numbering there.

I'm wondering about the choice of OIDs in the new test. I wonder if
it's possible to get ANYNONARRAY (or others) by way of having a
polymorphic function that passes its polymorphic argument in a qual. I
suppose it won't do anything in v10, or will it? Worth checking :-)> Why not use IsPolymorphicType?

Hmm, so IsPolymorphicType() test covers all of these pseudo-types except
RECORDOID. I rewrote the patch to use IsPolymorphicType.

I'm not able to think of a case where the partition constraint expression
would have to contain ANYNONARRAY though.

Also, I think it'd be good to have tests
for all these cases (even in v10), just to make sure we don't break it
going forward.

So, I had proposed this patch in last December, because partition pruning
using constraint exclusion was broken for these types and still is in PG
10. I have added the tests back in the patch for PG 10 to test that
partition pruning (using constraint exclusion) works for these cases. For
PG 11 and HEAD, we took care of that in e5dcbb88a15 (Rework code to
determine partition pruning procedure), so there does not appear to be any
need to add tests for pruning there.

At least the array case is clearly broken today ...
A test for the RECORDOID case would be particularly welcome, since it's
somehow different from the other cases. (I didn't understand why did
you remove the test in the latest version.)

I have added the tests in the patch for PG 10.

I have marked both patches as v5. One patch is for PG 10 and the other
applies unchanged to both HEAD and PG 11 branches.

Thanks,
Amit

On 2018-Jul-09, Amit Langote wrote:

On 2018/07/07 9:19, Alvaro Herrera wrote:

On 2018-May-08, Amit Langote wrote:

I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.

Interesting patchset. Didn't read your previous v2, v3 versions; I only
checked your latest, v1 (???).

Sorry, I think I messed up version numbering there.

Well, I later realized that you had labelled the master version v4 and
the pg10 version v1, which made sense since you hadn't produced any
patch for pg10 before that ...

I'm wondering about the choice of OIDs in the new test. I wonder if
it's possible to get ANYNONARRAY (or others) by way of having a
polymorphic function that passes its polymorphic argument in a qual. I
suppose it won't do anything in v10, or will it? Worth checking :-)> Why not use IsPolymorphicType?

Hmm, so IsPolymorphicType() test covers all of these pseudo-types except
RECORDOID. I rewrote the patch to use IsPolymorphicType.

I think that's good.

I'm not able to think of a case where the partition constraint expression
would have to contain ANYNONARRAY though.

I was about to give up trying to construct a case for this, when I
noticed this behavior (in pg10):

create or replace function f(anyelement) returns anynonarray immutable language plpgsql as $$
begin
return $1;
end;
$$;
create table pt (a int) partition by range (f(a));
create table pt1 partition of pt for values from (0) to (100);
create table pt2 partition of pt for values from (100) to (200);

and then pruning doesn't work:
alvherre=# explain select * from pt where a = 150;
QUERY PLAN
───────────────────────────────────────────────────────────
Append (cost=0.00..83.75 rows=26 width=4)
-> Seq Scan on pt1 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = 150)
-> Seq Scan on pt2 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = 150)
(5 filas)

The same occurs in 11 and master. I think this is because the
polymorphic type is resolved for the function ahead of time (at
table creation time); partexprs shows

({FUNCEXPR :funcid 35757 :funcresulttype 23 :funcretset false :funcvariadic false :funcformat 0 :funccollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1 :varoattno 1 :location 46}) :location 44})

where the ":funcresulttype 23" bit indicates that the function is
returning type integer, which I find a bit odd. I think if we were to
leave it as funcresulttype anynonarray, pruning would work. Not sure
yet where is that done.

Also, I think it'd be good to have tests
for all these cases (even in v10), just to make sure we don't break it
going forward.

So, I had proposed this patch in last December, because partition pruning
using constraint exclusion was broken for these types and still is in PG
10. I have added the tests back in the patch for PG 10 to test that
partition pruning (using constraint exclusion) works for these cases. For
PG 11 and HEAD, we took care of that in e5dcbb88a15 (Rework code to
determine partition pruning procedure), so there does not appear to be any
need to add tests for pruning there.

Right.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

The same occurs in 11 and master. I think this is because the
polymorphic type is resolved for the function ahead of time (at
table creation time); partexprs shows

({FUNCEXPR :funcid 35757 :funcresulttype 23 :funcretset false :funcvariadic false :funcformat 0 :funccollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1 :varoattno 1 :location 46}) :location 44})

where the ":funcresulttype 23" bit indicates that the function is
returning type integer, which I find a bit odd. I think if we were to
leave it as funcresulttype anynonarray, pruning would work.

... at the cost of breaking many other things.

regards, tom lane

Another thing I realized when testing this is that partitioning over a
domain doesn't work very nicely (tested in 10 and master):

create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);

results in:

ERROR: specified value cannot be cast to type overint for column "a"
L�NEA 1: create table pt1 partition of pt for values from (0) to (100...
^
DETALLE: The cast requires a non-immutable conversion.
SUGERENCIA: Try putting the literal value in single quotes.

I tried to do what the HINT says, but it fails in the same way. I also
tried to add casts, but those are rejected as syntax errors.

Tracing it down, turns out that transformPartitionBoundValue gets from
coerce_to_target_type a CoerceToDomain node. It then tries to apply
expression_planner() to simplify the expression, but that one doesn't
want anything to do with a domain coercion (for apparently good reasons,
given other comments in that file). However, if we take out the
expression_planner() and replace it with a call to
strip_implicit_coercions(), not only it magically starts working, but
also the regression tests start failing with the attached diff, which
seems a Good Thing to me.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tracing it down, turns out that transformPartitionBoundValue gets from
coerce_to_target_type a CoerceToDomain node. It then tries to apply
expression_planner() to simplify the expression, but that one doesn't
want anything to do with a domain coercion (for apparently good reasons,
given other comments in that file).

Quite. Suppose you did

create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);

and the system took it, and then you did

alter domain overint add check (value > 100);

What happens now?

However, if we take out the
expression_planner() and replace it with a call to
strip_implicit_coercions(), not only it magically starts working, but
also the regression tests start failing with the attached diff, which
seems a Good Thing to me.

Why would you find that to be a good thing? The prohibition against
mutable coercions seems like something we need here, for more or less
the same reason in the domain example.

regards, tom lane

On 2018-Jul-09, Tom Lane wrote:

Suppose you did

create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);

and the system took it, and then you did

alter domain overint add check (value > 100);

What happens now?

It scans the table to check whether any values violate that condition,
and raises an error if they do:

alvherre=# alter domain overint add check (value > 100);
ERROR: column "a" of table "ppt1" contains values that violate the new constraint

This looks sensible behavior to me.

Now if you don't have any values that violate the new constraint, then
the constraint can be created just fine, and you now have a partition
that can never accept any values. But that doesn't seem like a terrible
problem.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-May-08, Amit Langote wrote:

In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.

Actually, even in 11/master it also fixes this case:

alvherre=# explain update p set a = a || a where a = '{1}';
QUERY PLAN
──────────────────────────────────────────────────────────
Update on p (cost=0.00..54.03 rows=14 width=38)
Update on p1
Update on p2
-> Seq Scan on p1 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
(7 filas)

Because UPDATE uses the predtest.c prune code, not partprune. So it's
not just some ruleutils beautification.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2018-Jul-09, Tom Lane wrote:

Suppose you did

create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);

and the system took it, and then you did

alter domain overint add check (value > 100);

What happens now?

It scans the table to check whether any values violate that condition,
and raises an error if they do:

alvherre=# alter domain overint add check (value > 100);
ERROR: column "a" of table "ppt1" contains values that violate the new constraint

This looks sensible behavior to me.

And what about those partition bound values? They are now illegal
for the domain, so I would expect a dump/reload to fail, regardless
of whether there are any values in the table.

regards, tom lane

On 2018-Jul-10, Alvaro Herrera wrote:

alvherre=# explain update p set a = a || a where a = '{1}';
QUERY PLAN
──────────────────────────────────────────────────────────
Update on p (cost=0.00..54.03 rows=14 width=38)
Update on p1
Update on p2
-> Seq Scan on p1 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
(7 filas)

Because UPDATE uses the predtest.c prune code, not partprune. So it's
not just some ruleutils beautification.

I added this test, modified some comments, and pushed.

Thanks for the patch.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Jul-10, Tom Lane wrote:

And what about those partition bound values? They are now illegal
for the domain, so I would expect a dump/reload to fail, regardless
of whether there are any values in the table.

Hmm, true.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services