Re: no partition pruning when partitioning using array type

[Amit Langote]

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
From 042aa582f717ceb695a1ab60517c2e9f7d04704b Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 26 Jul 2018 11:07:51 +0900
Subject: [PATCH v1] Disallow domain type partition key

---
 src/backend/commands/tablecmds.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eb2d33dd86..871c831cd2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13693,6 +13693,12 @@ ComputePartitionAttrs(Relation rel, List *partParams, 
AttrNumber *partattrs,
atttype = attform->atttypid;
attcollation = attform->attcollation;
ReleaseSysCache(atttuple);
+
+   /* Prevent using domains as a partition key. */
+   if (get_typtype(atttype) == TYPTYPE_DOMAIN)
+   ereport(ERROR,
+   
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot use 
domain type column as partition key")));
}
else
{
@@ -13703,6 +13709,12 @@ ComputePartitionAttrs(Relation rel, List *partParams, 
AttrNumber *partattrs,
atttype = exprType(expr);
attcollation = exprCollation(expr);
 
+   /* Prevent using domains as a partition key. */
+   if (get_typtype(atttype) == TYPTYPE_DOMAIN)
+   ereport(ERROR,
+   
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot use 
domain type expression as partition key")));
+
/*
 * Strip any top-level COLLATE clause.  This ensures 
that we treat
 * "x COLLATE y" and "(x COLLATE y)" alike.
-- 
2.11.0

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Amit Langote]

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

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Amit Langote]

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
From aca92efe08a45c7645720bf7c47ee5ca221f0a80 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 11 Jul 2018 10:26:55 +0900
Subject: [PATCH v1] Prevent RemoveAttributeById from dropping partition key

dependency.c is currently be able to drop whatever is referencing
the partition key column, because RemoveAttributeById lacks guards
checks to see if it's actually dropping a partition key.
---
 src/backend/catalog/heap.c | 29 +
 src/test/regress/expected/create_table.out | 10 ++
 src/test/regress/sql/create_table.sql  |  8 
 3 files changed, 47 insertions(+)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d223ba8537..b69b9d9436 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1585,6 +1585,35 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
else
{
/* Dropping user attributes is lots harder */
+   boolis_expr;
+
+   /*
+* Prevent dropping partition key attribute, making whatever 
that's
+* trying to do this fail.
+*/
+   if (has_partition_attrs(rel,
+bms_make_singleton(attnum - 
FirstLowInvalidHeapAttributeNumber),
+_expr))
+   {
+   if (!is_expr)
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+errmsg("cannot drop column 
\"%s\" of table \"%s\"",
+   
NameStr(attStruct->attname),
+   
RelationGetRelationName(rel)),
+errdetail("Column \"%s\" is 
named in the partition key of \"%s\"",
+  
NameStr(attStruct->attname),
+  
RelationGetRelationName(rel;
+   else
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+errmsg("cannot drop column 
\"%s\" of table \"%s\"",
+   
NameStr(attStruct->attname),
+   
RelationGetRelationName(rel)),
+errdetail("Column \"%s\" is 
referenced in the partition key expression of \"%s\"",
+  
NameStr(attStruct->attname),
+  
RelationGetRelationName(rel;
+   }
 
/* Mark the attribute as dropped */
attStruct->attisdropped = true;
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index 8fdbca1345..ba32d781d1 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -909,3 +909,13 @@ ERROR:  cannot create a temporary relation as partition of 
permanent relation "p
 create temp table temp_part partition of temp_parted default; -- ok
 drop table perm_parted cascade;
 drop table temp_parted cascade;
+-- test domain partition key behavior
+create domain ct_overint as int;
+create table ct_domainpartkey (a ct_overint) partition by range (a);
+-- errors because partition key column cannot be dropped
+drop domain ct_overint cascade;
+NOTICE:  drop cascades to column a of table ct_domainpartkey
+ERROR:  

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Amit Langote]

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

Re: no partition pruning when partitioning using array type

[Amit Langote]

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

Re: no partition pruning when partitioning using array type

[Amit Langote]

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

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

On 2018-Jul-09, Tom Lane wrote:

> Alvaro Herrera  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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Tom Lane]

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

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Tom Lane]

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

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
*** /pgsql/source/REL_10_STABLE/src/test/regress/expected/create_table.out  
2018-07-09 13:13:57.397491338 -0400
--- 
/home/alvherre/Code/pgsql/build/REL_10_STABLE/src/test/regress/results/create_table.out
 2018-07-09 17:43:00.794556357 -0400
***
*** 496,507 
a money
  ) PARTITION BY LIST (a);
  CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ERROR:  specified value cannot be cast to type money for column "a"
- LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-^
- DETAIL:  The cast requires a non-immutable conversion.
- HINT:  Try putting the literal value in single quotes.
  CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
  DROP TABLE moneyp;
  -- immutable cast should work, though
  CREATE TABLE bigintp (
--- 496,503 
a money
  ) PARTITION BY LIST (a);
  CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
  CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+ ERROR:  relation "moneyp_10" already exists
  DROP TABLE moneyp;
  -- immutable cast should work, though
  CREATE TABLE bigintp (

==

Re: no partition pruning when partitioning using array type

[Tom Lane]

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

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Amit Langote]

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
From 1ec0c064adc34c12ae5615f0f2bca27a9c61c30f Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 8 May 2018 14:31:37 +0900
Subject: [PATCH v4] Fix how get_partition_operator looks up the operator

Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved.  Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there.  That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
 src/backend/catalog/partition.c| 47 ++
 src/test/regress/expected/create_table.out |  2 +-
 src/test/regress/expected/inherit.out  | 98 ++
 src/test/regress/sql/inherit.sql   | 43 +
 4 files changed, 161 insertions(+), 29 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 17704f36b9..1f50b29a3f 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1176,40 +1176,31 @@ get_partition_operator(PartitionKey key, int col, 
StrategyNumber strategy,
Oid operoid;
 
/*
-* First check if there exists an operator of the given strategy, with
-* this column's type as both its lefttype and righttype, in the
-* partitioning operator family specified for the column.
+* Get the operator in the partitioning operator family using the 
operator
+* class declared input type as both its lefttype and righttype.
 */
operoid = get_opfamily_member(key->partopfamily[col],
- 
key->parttypid[col],
- 
key->parttypid[col],
+ 
key->partopcintype[col],
+ 
key->partopcintype[col],
  strategy);
+   if (!OidIsValid(operoid))
+   elog(ERROR, "missing operator %d(%u,%u) in partition opfamily 
%u",
+strategy, key->partopcintype[col], 
key->partopcintype[col],
+key->partopfamily[col]);
 
/*
-* If one doesn't exist, we must resort to using an operator in the same
-* operator family but with the operator class declared input type.  It 
is
-* OK to do so, 

Re: no partition pruning when partitioning using array type

[Amit Langote]

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

Re: no partition pruning when partitioning using array type

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Andrew Dunstan]

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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: no partition pruning when partitioning using array type

[Amit Langote]

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
From 9f0dff4e6f3c7e8204a73ef0734e38056de08571 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 8 May 2018 14:31:37 +0900
Subject: [PATCH v1] Fix how get_partition_operator looks up the operator

Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved.  Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there.  That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
 src/backend/catalog/partition.c| 49 +-
 src/test/regress/expected/create_table.out |  2 +-
 2 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 17704f36b9..5603a5d992 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1176,40 +1176,33 @@ get_partition_operator(PartitionKey key, int col, 
StrategyNumber strategy,
Oid operoid;
 
/*
-* First check if there exists an operator of the given strategy, with
-* this column's type as both its lefttype and righttype, in the
-* partitioning operator family specified for the column.
+* Get the operator in the partitioning operator family using the 
operator
+* class declared input type as both its lefttype and righttype.
 */
operoid = get_opfamily_member(key->partopfamily[col],
- 
key->parttypid[col],
- 
key->parttypid[col],
+ 
key->partopcintype[col],
+ 
key->partopcintype[col],
  strategy);
+   if (!OidIsValid(operoid))
+   elog(ERROR, "missing operator %d(%u,%u) in partition opfamily 
%u",
+strategy, key->partopcintype[col], 
key->partopcintype[col],
+key->partopfamily[col]);
 
/*
-* If one doesn't exist, we must resort to using an operator in the same
-* operator family but with the operator class declared input type.  It 
is
-* OK to do so, because the column's type is known to be 
binary-coercible
-* with the 

Re: no partition pruning when partitioning using array type

[Amit Langote]

On 2018/02/02 0:20, Robert Haas wrote:
> On Thu, Feb 1, 2018 at 4:42 AM, Amit Langote
>  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
From b0ebef9526a2abb162e877bc3b73d8194ffb1d2f Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 8 Dec 2017 19:09:31 +0900
Subject: [PATCH v3] In get_partition_operator(), avoid RelabelType in some
 cases

We never have pg_am catalog entries with "real" array, enum, record,
range types as leftop and rightop types.  Instead, operators manipulating
such types have entries with "pseudo-types" listed as leftop and rightop
types.  For example, for enums, all operator entries are marked with
anyenum as their leftop and rightop types.  So, if we pass "real" type
OIDs to get_opfamily_member(), we get back an InvalidOid for the
aforementioned type categories.  We'd normally ask the caller of
get_partition_operator to add a RelabelType in such case.  But we don't
need to in these cases, as the rest of the system doesn't.

This fixes the issue that the constraint exclusion code would reject
matching partition key quals with restrictions due to these extraneous
RelabelType nodes surrounding partition key expressions generated
by partition.c.

Also, ruleutils.c can resolve the operator names properly when deparsing
an internally generated partition qual expressions.
---
 src/backend/catalog/partition.c   | 12 ++-
 src/test/regress/expected/create_table.out|  2 +-
 src/test/regress/expected/partition_prune.out | 45 +++
 src/test/regress/sql/partition_prune.sql  | 18 +++
 4 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fcf763..cf71ba174e 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1605,7 +1605,17 @@ get_partition_operator(PartitionKey key, int col, 
StrategyNumber strategy,
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
 strategy, key->partopcintype[col], 
key->partopcintype[col],
 key->partopfamily[col]);
-   *need_relabel = true;
+
+   /*
+* For certain type categories, there don't exist pg_amop 
entries with
+* the given type OID as the operator's left and right operand 
type.
+* Instead, the entries have corresponding pseudo-types listed 
as the
+* left and right operand type; for example, anyenum for an 
enum type.
+* For such cases, it's not necessary to add the RelabelType 
node,
+* because other parts of the sytem won't add one either.
+*/
+   if (get_typtype(key->partopcintype[col]) != TYPTYPE_PSEUDO)
+   *need_relabel = true;
}
else
*need_relabel = false;
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index 39a963888d..7764da3152 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -880,6 +880,6 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN 
('{1}', '{2}');
 
+---+---+--+-+--+--+-
  a  | integer[] |   |  | | extended |  
| 
 Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray 
OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray 

Re: no partition pruning when partitioning using array type

[Robert Haas]

On Thu, Feb 1, 2018 at 4:42 AM, Amit Langote
 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

Re: no partition pruning when partitioning using array type

[Amit Langote]

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
From c4e6a456309e97f0391b2bf8b417b8a71cfac778 Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 8 Dec 2017 19:09:31 +0900
Subject: [PATCH v2] Teach operator_predicate_proof() to strip RelabelType

---
 src/backend/optimizer/util/predtest.c | 13 +---
 src/test/regress/expected/partition_prune.out | 45 +++
 src/test/regress/sql/partition_prune.sql  | 18 +++
 3 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/src/backend/optimizer/util/predtest.c 
b/src/backend/optimizer/util/predtest.c
index 8106010329..64b0926641 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -1407,6 +1407,11 @@ static const StrategyNumber BT_refute_table[6][6] = {
{none, none, BTEQ, none, none, none}/* NE */
 };
 
+/* Strip expr of the surrounding RelabelType, if any. */
+#define strip_relabel(expr) \
+   ((Node *) (IsA((expr), RelabelType) \
+   ? ((RelabelType *) (expr))->arg \
+   : (expr)))
 
 /*
  * operator_predicate_proof
@@ -1503,10 +1508,10 @@ operator_predicate_proof(Expr *predicate, Node *clause, 
bool refute_it)
/*
 * We have to match up at least one pair of input expressions.
 */
-   pred_leftop = (Node *) linitial(pred_opexpr->args);
-   pred_rightop = (Node *) lsecond(pred_opexpr->args);
-   clause_leftop = (Node *) linitial(clause_opexpr->args);
-   clause_rightop = (Node *) lsecond(clause_opexpr->args);
+   pred_leftop = strip_relabel(linitial(pred_opexpr->args));
+   pred_rightop = strip_relabel(lsecond(pred_opexpr->args));
+   clause_leftop = strip_relabel(linitial(clause_opexpr->args));
+   clause_rightop = strip_relabel(lsecond(clause_opexpr->args));
 
if (equal(pred_leftop, clause_leftop))
{
diff --git a/src/test/regress/expected/partition_prune.out 
b/src/test/regress/expected/partition_prune.out
index 348719bd62..cf25c8ec86 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1088,4 +1088,49 @@ explain (costs off) select * from boolpart where a is 
not unknown;
  Filter: (a IS NOT UNKNOWN)
 (7 rows)
 
+-- array type list partition key
+create table arrpart (a int[]) partition by list (a);
+create 

Re: no partition pruning when partitioning using array type

[Amit Langote]

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

Re: no partition pruning when partitioning using array type

[Robert Haas]

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