Thread: no partition pruning when partitioning using array type

Hi.

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.

Thoughts?

Thanks,
Amit

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] 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]
https://www.postgresql.org/message-id/151747550943.1247.2111555422760537959%40wrigleys.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] 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

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
:inputcollid0 :args ({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1
:varoattno1 :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
:inputcollid0 :args ({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1
:varoattno1 :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

On 2018-Jul-09, Tom Lane wrote:

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

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

By the way, while playing with a partition on type money and replacing
expression_planner() with strip_implicit_coercions(), the stored
partition bounds are completely broken -- they end up as literals of
type integer rather than money, so any insert at all into the partition
fails (even if the value is nominally the same).  So clearly it's not a
change we want.

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

On 2018/07/11 3:18, Alvaro Herrera wrote:
> 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.

That's true.  Shame I totally missed that.

Thanks,
Amit

On 2018/07/11 4:48, Alvaro Herrera wrote:
> 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.

Thank you for committing.

Regards,
Amit

On 2018/07/11 4:50, Alvaro Herrera wrote:
> 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.

There is a patch to overhaul how partition bound values are parsed:

https://commitfest.postgresql.org/18/1620/

With that patch, the error you reported upthread goes away (that is, one
can successfully create partitions), but the problem that Tom mentioned
above then appears.

What's the solution here then?  Prevent domains as partition key?

Thanks,
Amit

On 2018-Jul-11, Amit Langote wrote:

> What's the solution here then?  Prevent domains as partition key?

Maybe if a domain is used in a partition key somewhere, prevent
constraints from being added?

Another thing worth considering: are you prevented from dropping a
domain that's used in a partition key?  If not, you get an ugly message
when you later try to drop the table.

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

On 2018/07/11 13:12, Alvaro Herrera wrote:
> On 2018-Jul-11, Amit Langote wrote:
> 
>> What's the solution here then?  Prevent domains as partition key?
> 
> Maybe if a domain is used in a partition key somewhere, prevent
> constraints from being added?

Maybe, but I guess you mean only prevent adding such a constraint
after-the-fact.

If a domain has a constraint before creating any partitions based on the
domain, then partition creation command would check that the partition
bound values satisfy those constraints.

> Another thing worth considering: are you prevented from dropping a
> domain that's used in a partition key?  If not, you get an ugly message
> when you later try to drop the table.

Yeah, I was about to write a message about that.  I think we need to teach
RemoveAttributeById, which dependency.c calls when dropping the
referencing domain with cascade option, to abort if the attribute passed
to it belongs to the partition key of the input relation.

I tried that in the attached, but not sure about the order of messages
that appear in the output of DROP DOMAIN .. CASCADE.  It contains a NOTICE
message followed by an ERROR message.

Thanks,
Amit

On 2018-Jul-11, Amit Langote wrote:

> On 2018/07/11 13:12, Alvaro Herrera wrote:
> > On 2018-Jul-11, Amit Langote wrote:
> > 
> >> What's the solution here then?  Prevent domains as partition key?
> > 
> > Maybe if a domain is used in a partition key somewhere, prevent
> > constraints from being added?
> 
> Maybe, but I guess you mean only prevent adding such a constraint
> after-the-fact.

Yeah, any domain constraints added before won't be a problem.  Another
angle on this problem is to verify partition bounds against the domain
constraint being added; if they all pass, there's no reason to reject
the constraint.

But I worry that Tom was using domain constraints as just an example of
a more general problem that we can get into.  

> If a domain has a constraint before creating any partitions based on the
> domain, then partition creation command would check that the partition
> bound values satisfy those constraints.

Of course.

> > Another thing worth considering: are you prevented from dropping a
> > domain that's used in a partition key?  If not, you get an ugly message
> > when you later try to drop the table.
> 
> Yeah, I was about to write a message about that.  I think we need to teach
> RemoveAttributeById, which dependency.c calls when dropping the
> referencing domain with cascade option, to abort if the attribute passed
> to it belongs to the partition key of the input relation.

Actually, here's another problem: why are we letting a column on a
domain become partition key, if you'll never be able to create a
partition?  It seems that for pg11 the right reaction is to check
the partition key and reject this case.

I'm not sure of this implementation -- I couldn't find any case where we
reject the deletion on the function called from doDeletion (and we don't
have a preliminary phase on which to check for this, either).  Maybe we
need a pg_depend entry to prevent this, though it seems a little silly.
Maybe we should add a preliminary verification phase in
deleteObjectsInList(), where we invoke object-type-specific checks.

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

On 2018/07/12 2:33, Alvaro Herrera wrote:
> On 2018-Jul-11, Amit Langote wrote:
> 
>> On 2018/07/11 13:12, Alvaro Herrera wrote:
>>> On 2018-Jul-11, Amit Langote wrote:
>>>
>>>> What's the solution here then?  Prevent domains as partition key?
>>>
>>> Maybe if a domain is used in a partition key somewhere, prevent
>>> constraints from being added?
>>
>> Maybe, but I guess you mean only prevent adding such a constraint
>> after-the-fact.
> 
> Yeah, any domain constraints added before won't be a problem.  Another
> angle on this problem is to verify partition bounds against the domain
> constraint being added; if they all pass, there's no reason to reject
> the constraint.

That's an idea.  It's not clear to me how easy it is to get hold of all
the partition bounds that contain domain values.  How do we get from the
domain on which a constraint is being added to the relpartbound which
would contain the partition bound value of the domain?

> But I worry that Tom was using domain constraints as just an example of
> a more general problem that we can get into.  
> 
> 
>>> Another thing worth considering: are you prevented from dropping a
>>> domain that's used in a partition key?  If not, you get an ugly message
>>> when you later try to drop the table.
>>
>> Yeah, I was about to write a message about that.  I think we need to teach
>> RemoveAttributeById, which dependency.c calls when dropping the
>> referencing domain with cascade option, to abort if the attribute passed
>> to it belongs to the partition key of the input relation.
> 
> Actually, here's another problem: why are we letting a column on a
> domain become partition key, if you'll never be able to create a
> partition?  It seems that for pg11 the right reaction is to check
> the partition key and reject this case.

Yeah, that seems much safer, and given how things stand today, no users
would complain if we do this.

> I'm not sure of this implementation -- I couldn't find any case where we
> reject the deletion on the function called from doDeletion (and we don't
> have a preliminary phase on which to check for this, either).  Maybe we
> need a pg_depend entry to prevent this, though it seems a little silly.
> Maybe we should add a preliminary verification phase in
> deleteObjectsInList(), where we invoke object-type-specific checks.

Doing pre-check based fix had crossed my mind, but I didn't try hard to
pursue it.  If we decide to just prevent domain partition keys, we don't
need to try too hard now to come up with a nice implementation for this,
right?

Thanks,
Amit

On 2018-Jul-12, Amit Langote wrote:

> On 2018/07/12 2:33, Alvaro Herrera wrote:

> > Yeah, any domain constraints added before won't be a problem.  Another
> > angle on this problem is to verify partition bounds against the domain
> > constraint being added; if they all pass, there's no reason to reject
> > the constraint.
> 
> That's an idea.  It's not clear to me how easy it is to get hold of all
> the partition bounds that contain domain values.  How do we get from the
> domain on which a constraint is being added to the relpartbound which
> would contain the partition bound value of the domain?

Well, we already get all table columns using a domain when the domain
gets a new constraint; I suppose it's just a matter of verifying for
each column whether it's part of the partition key, and then drill down
from there.  (I'm not really familiar with that part of the catalog.)

> > Actually, here's another problem: why are we letting a column on a
> > domain become partition key, if you'll never be able to create a
> > partition?  It seems that for pg11 the right reaction is to check
> > the partition key and reject this case.
> 
> Yeah, that seems much safer, and given how things stand today, no users
> would complain if we do this.

Agreed.

> > I'm not sure of this implementation -- I couldn't find any case where we
> > reject the deletion on the function called from doDeletion (and we don't
> > have a preliminary phase on which to check for this, either).  Maybe we
> > need a pg_depend entry to prevent this, though it seems a little silly.
> > Maybe we should add a preliminary verification phase in
> > deleteObjectsInList(), where we invoke object-type-specific checks.
> 
> Doing pre-check based fix had crossed my mind, but I didn't try hard to
> pursue it.  If we decide to just prevent domain partition keys, we don't
> need to try too hard now to come up with a nice implementation for this,
> right?

Absolutely.

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

On 2018/07/26 1:41, Alvaro Herrera wrote:
> On 2018-Jul-12, Amit Langote wrote:
>> On 2018/07/12 2:33, Alvaro Herrera wrote:
>>> Yeah, any domain constraints added before won't be a problem.  Another
>>> angle on this problem is to verify partition bounds against the domain
>>> constraint being added; if they all pass, there's no reason to reject
>>> the constraint.
>>
>> That's an idea.  It's not clear to me how easy it is to get hold of all
>> the partition bounds that contain domain values.  How do we get from the
>> domain on which a constraint is being added to the relpartbound which
>> would contain the partition bound value of the domain?
> 
> Well, we already get all table columns using a domain when the domain
> gets a new constraint; I suppose it's just a matter of verifying for
> each column whether it's part of the partition key, and then drill down
> from there.  (I'm not really familiar with that part of the catalog.)

Possibly doable, but maybe leave it for another day.

>>> Actually, here's another problem: why are we letting a column on a
>>> domain become partition key, if you'll never be able to create a
>>> partition?  It seems that for pg11 the right reaction is to check
>>> the partition key and reject this case.
>>
>> Yeah, that seems much safer, and given how things stand today, no users
>> would complain if we do this.
> 
> Agreed.
> 
>>> I'm not sure of this implementation -- I couldn't find any case where we
>>> reject the deletion on the function called from doDeletion (and we don't
>>> have a preliminary phase on which to check for this, either).  Maybe we
>>> need a pg_depend entry to prevent this, though it seems a little silly.
>>> Maybe we should add a preliminary verification phase in
>>> deleteObjectsInList(), where we invoke object-type-specific checks.
>>
>> Doing pre-check based fix had crossed my mind, but I didn't try hard to
>> pursue it.  If we decide to just prevent domain partition keys, we don't
>> need to try too hard now to come up with a nice implementation for this,
>> right?
> 
> Absolutely.

OK, attached is a patch for that.

Thanks,
Amit