Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Thu, Sep 14, 2017 at 12:59 AM, Amit Langote
 wrote:
> Since Jeevan Ladhe mentioned this patch [1] earlier this week, sending the
> rebased patches here for consideration.  Actually there are only 2 patches
> now, because 0002 above is rendered unnecessary by ecfe59e50fb [2].

Committed 0001 and back-patched to v10.

Your 0002 and the patch from Jeevan Ladhe to which you refer seem to
be covering closely related subjects.  When I apply either patch by
itself, the regression tests pass; when I apply both together, they
fail.  Could you and Jeevan sort that out?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/08/07 11:05, Amit Langote wrote:
> By the way, bulk of 0004 is refactoring which it seems is what Jeevan's
> default partition patch set also includes as one of the patches [1].  It
> got a decent amount review from Ashutosh.  I broke it down into a separate
> patch, so that the patch to add the new feature is its own tiny patch.
> 
> I also spotted a couple of comments referring to attachRel that we just
> recently renamed.
> 
> So, attached are:
> 
> 0001: s/attachRel/attachrel/g
> 0002: Refactoring to introduce a PartConstraintImpliedByRelConstraint
> 0003: Add the feature to skip the scan of individual leaf partitions

Since Jeevan Ladhe mentioned this patch [1] earlier this week, sending the
rebased patches here for consideration.  Actually there are only 2 patches
now, because 0002 above is rendered unnecessary by ecfe59e50fb [2].

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAOgcT0MWwG8WBw8frFMtRYHAgDD=tpt6u7wcso_l2k0kypm...@mail.gmail.com

[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ecfe59e50fb
From 55e1e14a821de541c2d24c152c193bf57eb91d43 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 7 Aug 2017 10:45:39 +0900
Subject: [PATCH 1/2] Typo: attachRel is now attachrel

---
 src/backend/commands/tablecmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 96354bdee5..563bcda30c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13779,7 +13779,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 * Prevent circularity by seeing if rel is a partition of attachrel. (In
 * particular, this disallows making a rel a partition of itself.)
 *
-* We do that by checking if rel is a member of the list of attachRel's
+* We do that by checking if rel is a member of the list of attachrel's
 * partitions provided the latter is partitioned at all.  We want to 
avoid
 * having to construct this list again, so we request the strongest lock
 * on all partitions.  We need the strongest lock, because we may decide
-- 
2.11.0

From a7d0d781bd9e3730f90d902d0e09abf79962f872 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 7 Aug 2017 10:51:47 +0900
Subject: [PATCH 2/2] Teach ATExecAttachPartition to skip validation in more
 cases

In cases where the table being attached is a partitioned table and
the table itself does not have constraints that would allow validation
on the whole table to be skipped, we can still skip the validations
of individual partitions if they each happen to have the requisite
constraints.

Per an idea of Robert Haas', with code refactoring suggestions from
Ashutosh Bapat.
---
 src/backend/commands/tablecmds.c  | 10 ++
 src/test/regress/expected/alter_table.out | 13 +
 src/test/regress/sql/alter_table.sql  | 10 ++
 3 files changed, 33 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 563bcda30c..901eea7fe2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13678,6 +13678,16 @@ ValidatePartitionConstraints(List **wqueue, Relation 
scanrel,
/* There can never be a whole-row reference here */
if (found_whole_row)
elog(ERROR, "unexpected whole-row reference 
found in partition key");
+
+   /* Check if we can we skip scanning this part_rel. */
+   if (PartConstraintImpliedByRelConstraint(part_rel, 
my_partconstr))
+   {
+   ereport(INFO,
+   (errmsg("partition constraint 
for table \"%s\" is implied by existing constraints",
+   
RelationGetRelationName(part_rel;
+   heap_close(part_rel, NoLock);
+   continue;
+   }
}
 
/* Grab a work queue entry. */
diff --git a/src/test/regress/expected/alter_table.out 
b/src/test/regress/expected/alter_table.out
index 0478a8ac60..e3415837b6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3464,6 +3464,19 @@ ERROR:  updated partition constraint for default 
partition would be violated by
 -- should be ok after deleting the bad row
 DELETE FROM part5_def_p1 WHERE b = 'y';
 ALTER TABLE part_5 ATTACH PARTITION part5_p1 FOR VALUES IN ('y');
+-- If the partitioned table being attached does not have a constraint that
+-- would allow validation scan to be skipped, but an individual partition
+-- does, then the partition's validation scan is skipped.  Note that the
+-- following leaf partition only allows rows that 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/08/05 11:05, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 8:45 PM, Amit Langote
>  wrote:
>>> 0003 needs a rebase.
>>
>> Rebased patch attached.
> 
> Committed.

Thank you.

> I think 0004 is a new feature, so I'm leaving that for v11.

Sure.

By the way, bulk of 0004 is refactoring which it seems is what Jeevan's
default partition patch set also includes as one of the patches [1].  It
got a decent amount review from Ashutosh.  I broke it down into a separate
patch, so that the patch to add the new feature is its own tiny patch.

I also spotted a couple of comments referring to attachRel that we just
recently renamed.

So, attached are:

0001: s/attachRel/attachrel/g
0002: Refactoring to introduce a PartConstraintImpliedByRelConstraint
0003: Add the feature to skip the scan of individual leaf partitions

Totally fine if you postpone 0002 and 0003 to when the tree opens up for
PG 11.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAOgcT0OARciE2X%2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com
From 26e7205fd7c35c0b497c1a7c31152393d3551b23 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 7 Aug 2017 10:45:39 +0900
Subject: [PATCH 1/3] Typo: attachRel is now attachrel

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

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1b8d4b3d17..d27c43bdc7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13491,7 +13491,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 * Prevent circularity by seeing if rel is a partition of attachrel. (In
 * particular, this disallows making a rel a partition of itself.)
 *
-* We do that by checking if rel is a member of the list of attachRel's
+* We do that by checking if rel is a member of the list of attachrel's
 * partitions provided the latter is partitioned at all.  We want to 
avoid
 * having to construct this list again, so we request the strongest lock
 * on all partitions.  We need the strongest lock, because we may decide
@@ -13746,7 +13746,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
{
/*
 * Adjust the constraint that we constructed 
above for
-* attachRel so that it matches this 
partition's attribute
+* attachrel so that it matches this 
partition's attribute
 * numbers.
 */
my_partconstr = 
map_partition_varattnos(my_partconstr, 1,
-- 
2.11.0

From fc6de6ab2800d509ec2af35c96722672e47e99cc Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 15 Jun 2017 19:22:31 +0900
Subject: [PATCH 2/3] Some refactoring of code in ATExecAttachPartition()

Take the code to check using table's constraints if the partition
constraint validation can be skipped and put it into a separate
function PartConstraintImpliedByRelConstraint().
---
 src/backend/commands/tablecmds.c | 187 +--
 1 file changed, 101 insertions(+), 86 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d27c43bdc7..818335ffe9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -473,6 +473,8 @@ static void CreateInheritance(Relation child_rel, Relation 
parent_rel);
 static void RemoveInheritance(Relation child_rel, Relation parent_rel);
 static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel,
  PartitionCmd *cmd);
+static bool PartConstraintImpliedByRelConstraint(Relation partrel,
+ List *partConstraint);
 static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name);
 
 
@@ -13422,15 +13424,12 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
Relationattachrel,
catalog;
List   *attachrel_children;
-   TupleConstr *attachrel_constr;
-   List   *partConstraint,
-  *existConstraint;
+   List   *partConstraint;
SysScanDesc scan;
ScanKeyData skey;
AttrNumber  attno;
int natts;
TupleDesc   tupleDesc;
-   boolskip_validate = false;
ObjectAddress address;
const char *trigger_name;
boolfound_whole_row;
@@ -13625,88 +13624,10 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
elog(ERROR, "unexpected whole-row reference found in partition 
key");
 
/*
-* Check if we can do away with 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Thu, Aug 3, 2017 at 8:45 PM, Amit Langote
 wrote:
>> 0003 needs a rebase.
>
> Rebased patch attached.

Committed.  I think 0004 is a new feature, so I'm leaving that for v11.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/08/04 3:29, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 1:04 AM, Amit Langote
>  wrote:
>> Alright, attached updated 0001 does that.
> 
> Committed 0001 and 0002.

Thanks.

> 0003 needs a rebase.

Rebased patch attached.

Thanks,
Amit
From f069845c027acc36aab4790d6d6afbf50bba803e Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 1 Aug 2017 10:58:38 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code

If the table being attached has attnos different from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to structural inequality of the corresponding Var expressions in the
the partition constraint and those in the table's check constraint
expressions.  Fix this by changing the partition constraint's expressions
to bear the partition's attnos.

Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.

Reported by: Ashutosh Bapat
Report: 
https://postgr.es/m/CAFjFpReT_kq_uwU_B8aWDxR7jNGE%3DP0iELycdq5oupi%3DxSQTOw%40mail.gmail.com
---
 src/backend/commands/tablecmds.c  | 40 ++-
 src/test/regress/expected/alter_table.out | 45 +++
 src/test/regress/sql/alter_table.sql  | 38 ++
 3 files changed, 111 insertions(+), 12 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7859ef13ac..1b8d4b3d17 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13433,6 +13433,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
boolskip_validate = false;
ObjectAddress address;
const char *trigger_name;
+   boolfound_whole_row;
 
attachrel = heap_openrv(cmd->name, AccessExclusiveLock);
 
@@ -13614,6 +13615,16 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.
+*/
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachrel,
+   
 rel, _whole_row);
+   /* There can never be a whole-row reference here */
+   if (found_whole_row)
+   elog(ERROR, "unexpected whole-row reference found in partition 
key");
+
+   /*
 * Check if we can do away with having to scan the table being attached 
to
 * validate the partition constraint, by *proving* that the existing
 * constraints of the table *imply* the partition predicate.  We include
@@ -13712,8 +13723,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
Relationpart_rel;
-   Expr   *constr;
-   boolfound_whole_row;
+   List   *my_partconstr = partConstraint;
 
/* Lock already taken */
if (part_relid != RelationGetRelid(attachrel))
@@ -13732,18 +13742,24 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
continue;
}
 
+   if (part_rel != attachrel)
+   {
+   /*
+* Adjust the constraint that we constructed 
above for
+* attachRel so that it matches this 
partition's attribute
+* numbers.
+*/
+   my_partconstr = 
map_partition_varattnos(my_partconstr, 1,
+   
part_rel, attachrel,
+   
_whole_row);
+   /* There can never be a whole-row reference 
here */
+   if (found_whole_row)
+   elog(ERROR, "unexpected whole-row 
reference found in partition key");
+   }
+
/* Grab a work queue entry. */
tab = ATGetQueueEntry(wqueue, part_rel);
-
-   /* Adjust constraint to match this partition */
-  

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Thu, Aug 3, 2017 at 1:04 AM, Amit Langote
 wrote:
> Alright, attached updated 0001 does that.

Committed 0001 and 0002.  0003 needs a rebase.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/08/02 20:35, Robert Haas wrote:
> On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote
>  wrote:
>> I too dislike the shape of attachRel.  How about we rename attachRel to
>> attachrel?  So, attachrel_children, attachrel_constr, etc.  It's still
>> long though... :)
> 
> OK, I can live with that, I guess.

Alright, attached updated 0001 does that.

About the other hunk, it seems we will have to go with the following
structure after all;

if (a)
  if (b)
 c();
  d();

Note that we were missing that there is a d(), which executes if a is
true.  c() executes *only* if b is true.  So I left that hunk unchanged,
viz. the following:

 /*
- * Skip if it's a partitioned table.  Only RELKIND_RELATION
- * relations (ie, leaf partitions) need to be scanned.
+ * Skip if the partition is itself a partitioned table.  We can
+ * only ever scan RELKIND_RELATION relations.
  */
-if (part_rel != attachRel &&
-part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 {
-heap_close(part_rel, NoLock);
+if (part_rel != attachrel)
+heap_close(part_rel, NoLock);
 continue;
 }


You might ask why the earlier code worked if there was this kind of
logical bug - accident; even if we failed skipping attachRel, the AT
rewrite phase which is in charge of actually scanning the table knows to
skip the partitioned tables, so no harm would be done.

Thanks,
Amit
From 4558c1b31f10e5446a22850a7f8b3d80d082330d Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 1 Aug 2017 10:12:39 +0900
Subject: [PATCH 1/4] Cosmetic fixes for code in ATExecAttachPartition

---
 src/backend/commands/tablecmds.c | 125 +++
 1 file changed, 61 insertions(+), 64 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..ecfc7e48c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13419,10 +13419,10 @@ ComputePartitionAttrs(Relation rel, List *partParams, 
AttrNumber *partattrs,
 static ObjectAddress
 ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 {
-   RelationattachRel,
+   Relationattachrel,
catalog;
-   List   *childrels;
-   TupleConstr *attachRel_constr;
+   List   *attachrel_children;
+   TupleConstr *attachrel_constr;
List   *partConstraint,
   *existConstraint;
SysScanDesc scan;
@@ -13434,22 +13434,22 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
ObjectAddress address;
const char *trigger_name;
 
-   attachRel = heap_openrv(cmd->name, AccessExclusiveLock);
+   attachrel = heap_openrv(cmd->name, AccessExclusiveLock);
 
/*
 * Must be owner of both parent and source table -- parent was checked 
by
 * ATSimplePermissions call in ATPrepCmd
 */
-   ATSimplePermissions(attachRel, ATT_TABLE | ATT_FOREIGN_TABLE);
+   ATSimplePermissions(attachrel, ATT_TABLE | ATT_FOREIGN_TABLE);
 
/* A partition can only have one parent */
-   if (attachRel->rd_rel->relispartition)
+   if (attachrel->rd_rel->relispartition)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("\"%s\" is already a partition",
-   
RelationGetRelationName(attachRel;
+   
RelationGetRelationName(attachrel;
 
-   if (OidIsValid(attachRel->rd_rel->reloftype))
+   if (OidIsValid(attachrel->rd_rel->reloftype))
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("cannot attach a typed table as 
partition")));
@@ -13462,7 +13462,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
ScanKeyInit(,
Anum_pg_inherits_inhrelid,
BTEqualStrategyNumber, F_OIDEQ,
-   ObjectIdGetDatum(RelationGetRelid(attachRel)));
+   ObjectIdGetDatum(RelationGetRelid(attachrel)));
scan = systable_beginscan(catalog, InheritsRelidSeqnoIndexId, true,
  NULL, 1, );
if (HeapTupleIsValid(systable_getnext(scan)))
@@ -13475,11 +13475,11 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
ScanKeyInit(,
Anum_pg_inherits_inhparent,
BTEqualStrategyNumber, F_OIDEQ,
- 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote
 wrote:
> I too dislike the shape of attachRel.  How about we rename attachRel to
> attachrel?  So, attachrel_children, attachrel_constr, etc.  It's still
> long though... :)

OK, I can live with that, I guess.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/08/02 10:27, Robert Haas wrote:
> On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote
>  wrote:
>> Since ATExecAttachPartition() deals with the possibility that the table
>> being attached itself might be partitioned, someone reading the code might
>> find it helpful to get some clue about whose partitions/children a
>> particular piece of code is dealing with -  AT's target table's (rel's) or
>> those of the table being attached (attachRel's)? IMHO, attachRel_children
>> makes it abundantly clear that it is in fact the partitions of the table
>> being attached that are being manipulated.
> 
> True, but it's also long and oddly capitalized and punctuated.  Seems
> like a judgement call which way is better, but I'm allergic to
> fooBar_baz style names.

I too dislike the shape of attachRel.  How about we rename attachRel to
attachrel?  So, attachrel_children, attachrel_constr, etc.  It's still
long though... :)

>>> -if (part_rel != attachRel &&
>>> -part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>>> +if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>>>  {
>>> -heap_close(part_rel, NoLock);
>>> +if (part_rel != attachRel)
>>> +heap_close(part_rel, NoLock);
>>>
>>> This works out to a cosmetic change, I guess, but it makes it worse...
>>
>> Not sure what you mean by "makes it worse".  The comment above says that
>> we should skip partitioned tables from being scheduled for heap scan.  The
>> new code still does that.  We should close part_rel before continuing to
>> consider the next partition, but mustn't do that if part_rel is really
>> attachRel.  The new code does that too.  Stylistically worse?
> 
> Yeah.  I mean, do you write:
> 
> if (a)
>if (b)
>c();
> 
> rather than
> 
> if (a && b)
> c();
> 
> ?
Hmm, The latter is better.  I guess I just get confused with long &&, ||,
() chains.

If you're fine with the s/attachRel/attachrel/g suggestion above, I will
update the patch along with reverting to if (a && b) c().

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Tue, Aug 1, 2017 at 9:23 PM, Amit Langote
 wrote:
> Since ATExecAttachPartition() deals with the possibility that the table
> being attached itself might be partitioned, someone reading the code might
> find it helpful to get some clue about whose partitions/children a
> particular piece of code is dealing with -  AT's target table's (rel's) or
> those of the table being attached (attachRel's)? IMHO, attachRel_children
> makes it abundantly clear that it is in fact the partitions of the table
> being attached that are being manipulated.

True, but it's also long and oddly capitalized and punctuated.  Seems
like a judgement call which way is better, but I'm allergic to
fooBar_baz style names.

>> -if (part_rel != attachRel &&
>> -part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>> +if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>>  {
>> -heap_close(part_rel, NoLock);
>> +if (part_rel != attachRel)
>> +heap_close(part_rel, NoLock);
>>
>> This works out to a cosmetic change, I guess, but it makes it worse...
>
> Not sure what you mean by "makes it worse".  The comment above says that
> we should skip partitioned tables from being scheduled for heap scan.  The
> new code still does that.  We should close part_rel before continuing to
> consider the next partition, but mustn't do that if part_rel is really
> attachRel.  The new code does that too.  Stylistically worse?

Yeah.  I mean, do you write:

if (a)
   if (b)
   c();

rather than

if (a && b)
c();

?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for reviewing.

On 2017/08/02 2:54, Robert Haas wrote:
> On Mon, Jul 31, 2017 at 11:10 PM, Amit Langote
>  wrote:
>> OK, these cosmetic changes are now in attached patch 0001.
> 
> Regarding 0001:
> 
> -List   *childrels;
> +List   *attachRel_children;
> 
> I sorta don't see why this is necessary, or better.

Since ATExecAttachPartition() deals with the possibility that the table
being attached itself might be partitioned, someone reading the code might
find it helpful to get some clue about whose partitions/children a
particular piece of code is dealing with -  AT's target table's (rel's) or
those of the table being attached (attachRel's)? IMHO, attachRel_children
makes it abundantly clear that it is in fact the partitions of the table
being attached that are being manipulated.

>  /* It's safe to skip the validation scan after all */
>  if (skip_validate)
> +{
> +/* No need to scan the table after all. */
> 
> The existing comment should be removed along with adding the new one, I think.

Oops, fixed.

> -if (part_rel != attachRel &&
> -part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>  {
> -heap_close(part_rel, NoLock);
> +if (part_rel != attachRel)
> +heap_close(part_rel, NoLock);
> 
> This works out to a cosmetic change, I guess, but it makes it worse...

Not sure what you mean by "makes it worse".  The comment above says that
we should skip partitioned tables from being scheduled for heap scan.  The
new code still does that.  We should close part_rel before continuing to
consider the next partition, but mustn't do that if part_rel is really
attachRel.  The new code does that too.  Stylistically worse?

Thanks,
Amit
From d67fac574abb085db33e00711ded0515d71d99eb Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 1 Aug 2017 10:12:39 +0900
Subject: [PATCH 1/4] Cosmetic fixes for code in ATExecAttachPartition

---
 src/backend/commands/tablecmds.c | 33 +++--
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..1307dc5893 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13421,7 +13421,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 {
RelationattachRel,
catalog;
-   List   *childrels;
+   List   *attachRel_children;
TupleConstr *attachRel_constr;
List   *partConstraint,
   *existConstraint;
@@ -13490,9 +13490,9 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 * Prevent circularity by seeing if rel is a partition of attachRel. (In
 * particular, this disallows making a rel a partition of itself.)
 */
-   childrels = find_all_inheritors(RelationGetRelid(attachRel),
-   
AccessShareLock, NULL);
-   if (list_member_oid(childrels, RelationGetRelid(rel)))
+   attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
+   
 AccessShareLock, NULL);
+   if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
 errmsg("circular inheritance not allowed"),
@@ -13684,19 +13684,16 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
skip_validate = true;
}
 
-   /* It's safe to skip the validation scan after all */
if (skip_validate)
+   {
+   /* No need to scan the table after all. */
ereport(INFO,
(errmsg("partition constraint for table \"%s\" 
is implied by existing constraints",

RelationGetRelationName(attachRel;
-
-   /*
-* Set up to have the table be scanned to validate the partition
-* constraint (see partConstraint above).  If it's a partitioned table, 
we
-* instead schedule its leaf partitions to be scanned.
-*/
-   if (!skip_validate)
+   }
+   else
{
+   /* Constraints proved insufficient, so we need to scan the 
table. */
List   *all_parts;
ListCell   *lc;
 
@@ -13721,17 +13718,17 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
part_rel = attachRel;
 
/*
-* Skip if it's a partitioned table.  Only 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Mon, Jul 31, 2017 at 11:10 PM, Amit Langote
 wrote:
> OK, these cosmetic changes are now in attached patch 0001.

Regarding 0001:

-List   *childrels;
+List   *attachRel_children;

I sorta don't see why this is necessary, or better.

 /* It's safe to skip the validation scan after all */
 if (skip_validate)
+{
+/* No need to scan the table after all. */

The existing comment should be removed along with adding the new one, I think.

-if (part_rel != attachRel &&
-part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 {
-heap_close(part_rel, NoLock);
+if (part_rel != attachRel)
+heap_close(part_rel, NoLock);

This works out to a cosmetic change, I guess, but it makes it worse...

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for taking a look at this.

On 2017/08/01 6:26, Robert Haas wrote:
> On Wed, Jul 26, 2017 at 9:50 PM, Amit Langote
>  wrote:
>> At least patch 0001 does address a bug.  Not sure if we can say that 0002
>> addresses a bug; it implements a feature that might be a
>> nice-to-have-in-PG-10.
> 
> I think 0001 is actually several fixes that should be separated:

Agreed.

> 
> - Cosmetic fixes, like renaming childrels to attachRel_children,
> adding a period after "Grab a work queue entry", and replacing the if
> (skip_validate) / if (!skip_validate) blocks with if (skip_validate) {
> ... } else { ... }.

OK, these cosmetic changes are now in attached patch 0001.

> 
> - Taking AccessExclusiveLock initially to avoid a lock upgrade hazard.

This in 0002.

> 
> - Rejiggering things so that we apply map_partition_varattnos() to the
> partConstraint in all relevant places.

And this in 0003.

> Regarding the XXX, we currently require AccessExclusiveLock for the
> addition of a CHECK constraint, so I think it's best to just do the
> same thing here.  We can optimize later, but it's probably not easy to
> come up with something that is safe and correct in all cases.
Agreed.  Dropped the XXX part in the comment.


0004 is what used to be 0002 before (checking partition constraints of
individual leaf partitions to skip their scans).  Attached here just in case.

Thanks,
Amit
From 614b0a51e4f820da81ec11b01ca79508c12415f7 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 1 Aug 2017 10:12:39 +0900
Subject: [PATCH 1/4] Cosmetic fixes for code in ATExecAttachPartition

---
 src/backend/commands/tablecmds.c | 32 +++-
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..2f7ef53caf 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13421,7 +13421,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 {
RelationattachRel,
catalog;
-   List   *childrels;
+   List   *attachRel_children;
TupleConstr *attachRel_constr;
List   *partConstraint,
   *existConstraint;
@@ -13490,9 +13490,9 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 * Prevent circularity by seeing if rel is a partition of attachRel. (In
 * particular, this disallows making a rel a partition of itself.)
 */
-   childrels = find_all_inheritors(RelationGetRelid(attachRel),
-   
AccessShareLock, NULL);
-   if (list_member_oid(childrels, RelationGetRelid(rel)))
+   attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
+   
 AccessShareLock, NULL);
+   if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
 errmsg("circular inheritance not allowed"),
@@ -13686,17 +13686,15 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 
/* It's safe to skip the validation scan after all */
if (skip_validate)
+   {
+   /* No need to scan the table after all. */
ereport(INFO,
(errmsg("partition constraint for table \"%s\" 
is implied by existing constraints",

RelationGetRelationName(attachRel;
-
-   /*
-* Set up to have the table be scanned to validate the partition
-* constraint (see partConstraint above).  If it's a partitioned table, 
we
-* instead schedule its leaf partitions to be scanned.
-*/
-   if (!skip_validate)
+   }
+   else
{
+   /* Constraints proved insufficient, so we need to scan the 
table. */
List   *all_parts;
ListCell   *lc;
 
@@ -13721,17 +13719,17 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
part_rel = attachRel;
 
/*
-* Skip if it's a partitioned table.  Only 
RELKIND_RELATION
-* relations (ie, leaf partitions) need to be scanned.
+* Skip if the partition is itself a partitioned table. 
 We can
+* only ever scan RELKIND_RELATION relations.
 */
-   if (part_rel != attachRel &&
-   part_rel->rd_rel->relkind == 
RELKIND_PARTITIONED_TABLE)
+   if (part_rel->rd_rel->relkind == 
RELKIND_PARTITIONED_TABLE)
{
- 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Wed, Jul 26, 2017 at 9:50 PM, Amit Langote
 wrote:
> At least patch 0001 does address a bug.  Not sure if we can say that 0002
> addresses a bug; it implements a feature that might be a
> nice-to-have-in-PG-10.

I think 0001 is actually several fixes that should be separated:

- Cosmetic fixes, like renaming childrels to attachRel_children,
adding a period after "Grab a work queue entry", and replacing the if
(skip_validate) / if (!skip_validate) blocks with if (skip_validate) {
... } else { ... }.

- Taking AccessExclusiveLock initially to avoid a lock upgrade hazard.

- Rejiggering things so that we apply map_partition_varattnos() to the
partConstraint in all relevant places.

Regarding the XXX, we currently require AccessExclusiveLock for the
addition of a CHECK constraint, so I think it's best to just do the
same thing here.  We can optimize later, but it's probably not easy to
come up with something that is safe and correct in all cases.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for looking at this again.

On 2017/07/26 23:31, Ashutosh Bapat wrote:
> On Wed, Jul 12, 2017 at 7:17 AM, Amit Langote
>  wrote:
>>
>> Thanks for the review.  Patch updated taking care of the comments.
> 
> The patches still apply and compile. make check runs well. I do not
> have any further review comments. Given that they address a bug,
> should we consider those for v10?

At least patch 0001 does address a bug.  Not sure if we can say that 0002
addresses a bug; it implements a feature that might be a
nice-to-have-in-PG-10.

Attaching rebased patches.

Thanks,
Amit
From 3edfe27c6a972b09fa9ec369e7dc33d1014bfef8 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 14 Jun 2017 11:32:01 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code

If the table being attached has attnos different from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to structural inequality of the corresponding Var expressions in the
the partition constraint and those in the table's check constraint
expressions.  Fix this by mapping the partition constraint's expressions
to bear the partition's attnos.

Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.
---
 src/backend/commands/tablecmds.c  | 87 +++
 src/test/regress/expected/alter_table.out | 45 
 src/test/regress/sql/alter_table.sql  | 38 ++
 3 files changed, 137 insertions(+), 33 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..8047c9a7bc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13421,7 +13421,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 {
RelationattachRel,
catalog;
-   List   *childrels;
+   List   *attachRel_children;
TupleConstr *attachRel_constr;
List   *partConstraint,
   *existConstraint;
@@ -13489,10 +13489,25 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
/*
 * Prevent circularity by seeing if rel is a partition of attachRel. (In
 * particular, this disallows making a rel a partition of itself.)
+*
+* We do that by checking if rel is a member of the list of attachRel's
+* partitions provided the latter is partitioned at all.  We want to 
avoid
+* having to construct this list again, so we request the strongest lock
+* on all partitions.  We need the strongest lock, because we may decide
+* to scan them if we find out that the table being attached (or its 
leaf
+* partitions) may contain rows that violate the partition constraint.
+* If the table has a constraint that would prevent such rows, which by
+* definition is present in all the partitions, we need not scan the
+* table, nor its partitions.  But we cannot risk a deadlock by taking a
+* weaker lock now and the stronger one only when needed.
+*
+* XXX - Do we need to lock the partitions here if we already have the
+* strongest lock on attachRel?  The information we need here to check
+* for circularity cannot change without taking a lock on attachRel.
 */
-   childrels = find_all_inheritors(RelationGetRelid(attachRel),
-   
AccessShareLock, NULL);
-   if (list_member_oid(childrels, RelationGetRelid(rel)))
+   attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
+   
 AccessExclusiveLock, NULL);
+   if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
 errmsg("circular inheritance not allowed"),
@@ -13603,6 +13618,13 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.
+*/
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+   
 rel);
+
+   /*
 * Check if we can do away with having to scan the table being attached 
to
 * validate the partition 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Wed, Jul 12, 2017 at 7:17 AM, Amit Langote
 wrote:
>
> Thanks for the review.  Patch updated taking care of the comments.

The patches still apply and compile. make check runs well. I do not
have any further review comments. Given that they address a bug,
should we consider those for v10?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/07/11 19:49, Ashutosh Bapat wrote:
> On Tue, Jul 4, 2017 at 9:51 AM, Amit Langote
>  wrote:
> 
>>
>> Attached updated patches.
> 
> There's an extra "we" in
> +* Note that attachRel's OID is in this list.  If it's partitioned, we
> +* we don't need to schedule it to be scanned (would be a noop anyway
> 
> And some portions of the comment before find_all_inheritors() in
> ATExecAttachPartition() look duplicated in portions of the code that
> check constraints on the table being attached and each of its leaf
> partition.
> 
> Other than that the patches look good to me.

Thanks for the review.  Patch updated taking care of the comments.

Regards,
Amit
From b7eb9a2bc0d5742f826da979e0bda5d4d2c25472 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 14 Jun 2017 11:32:01 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code

If the table being attached has attnos different from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to structural inequality of the corresponding Var expressions in the
the partition constraint and those in the table's check constraint
expressions.  Fix this by mapping the partition constraint's expressions
to bear the partition's attnos.

Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.
---
 src/backend/commands/tablecmds.c  | 87 +++
 src/test/regress/expected/alter_table.out | 45 
 src/test/regress/sql/alter_table.sql  | 38 ++
 3 files changed, 137 insertions(+), 33 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..8047c9a7bc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13421,7 +13421,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 {
RelationattachRel,
catalog;
-   List   *childrels;
+   List   *attachRel_children;
TupleConstr *attachRel_constr;
List   *partConstraint,
   *existConstraint;
@@ -13489,10 +13489,25 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
/*
 * Prevent circularity by seeing if rel is a partition of attachRel. (In
 * particular, this disallows making a rel a partition of itself.)
+*
+* We do that by checking if rel is a member of the list of attachRel's
+* partitions provided the latter is partitioned at all.  We want to 
avoid
+* having to construct this list again, so we request the strongest lock
+* on all partitions.  We need the strongest lock, because we may decide
+* to scan them if we find out that the table being attached (or its 
leaf
+* partitions) may contain rows that violate the partition constraint.
+* If the table has a constraint that would prevent such rows, which by
+* definition is present in all the partitions, we need not scan the
+* table, nor its partitions.  But we cannot risk a deadlock by taking a
+* weaker lock now and the stronger one only when needed.
+*
+* XXX - Do we need to lock the partitions here if we already have the
+* strongest lock on attachRel?  The information we need here to check
+* for circularity cannot change without taking a lock on attachRel.
 */
-   childrels = find_all_inheritors(RelationGetRelid(attachRel),
-   
AccessShareLock, NULL);
-   if (list_member_oid(childrels, RelationGetRelid(rel)))
+   attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
+   
 AccessExclusiveLock, NULL);
+   if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
 errmsg("circular inheritance not allowed"),
@@ -13603,6 +13618,13 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.
+*/
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+   
 rel);
+
+   /*
 * Check if we can 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Tue, Jul 4, 2017 at 9:51 AM, Amit Langote
 wrote:

>
> Attached updated patches.

There's an extra "we" in
+* Note that attachRel's OID is in this list.  If it's partitioned, we
+* we don't need to schedule it to be scanned (would be a noop anyway

And some portions of the comment before find_all_inheritors() in
ATExecAttachPartition() look duplicated in portions of the code that
check constraints on the table being attached and each of its leaf
partition.

Other than that the patches look good to me.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for the review.

On 2017/07/03 20:13, Ashutosh Bapat wrote:
> Thanks for working on the previous comments. The code really looks good now.
> On Fri, Jun 23, 2017 at 2:29 PM, Amit Langote
>  wrote:
>>
>>> Don't we need an exclusive lock to
>>> make sure that the constraints are not changed while we are validating 
>>> those?
>>
>> If I understand your question correctly, you meant to ask if we don't need
>> the strongest lock on individual partitions while looking at their
>> constraints to prove that we don't need to scan them.  We do and we do
>> take the strongest lock on individual partitions even today in the second
>> call to find_all_inheritors().  We're trying to eliminate the second call
>> here.
> 
> The comment seems to imply that we need strongest lock only when we
> "scan" the table/s.
> 
> Some more comments on 0001
> - * Prevent circularity by seeing if rel is a partition of attachRel. (In
> + * Prevent circularity by seeing if rel is a partition of attachRel, (In
>   * particular, this disallows making a rel a partition of itself.)
> The sentence outside () doesn't have a full-stop. I think the original
> construct was better.

Yep, fixed.

> + * We want to avoid having to construct this list again, so we request 
> the
>
> "this list" is confusing here since the earlier sentence doesn't mention any
> list at all. Instead we may reword it as "We will need the list of children
> later to check whether any of those have a row which would not fit the
> partition constraints. So, take the strongest lock ..."

It was confusing for sure, so rewrote.

>  * XXX - Do we need to lock the partitions here if we already have the
>  * strongest lock on attachRel?  The information we need here to check
>  * for circularity cannot change without taking a lock on attachRel.
>
> I wondered about this. Do we really need an exclusive lock to check whether
> partition constraint is valid? May be we can compare this condition with ALTER
> TABLE ... ADD CONSTRAINT since the children will all get a new constraint
> effectively. So, exclusive lock it is.

Actually, the XXX comment is about whether we need to lock the children at
all when checking the circularity of inheritance, that is, not about
whether we need lock to check the partition constraint is valid.

> Assert(linitial_oid(attachRel_children) ==
> RelationGetRelid(attachRel));
> if (attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> attachRel_children = list_delete_first(attachRel_children);
>
> Is it necessary for this code to have OID of the relation being attached as 
> the
> first one? You could simply call list_delete_oid() instead of
> list_delete_first(). If for any reason find_all_inheritors() changes the 
> output
> order, this assertion and code would need a change.\

I concluded that removing attachRel's OID from attachRel_children is
pointless, considering that we have to check for attachRel in the loop
below anyway.  The step of removing the OID wasn't helping much.

>>> The name skipPartConstraintValidation() looks very specific to the case at
>>> hand. The function is really checking whether existing constraints on the 
>>> table
>>> can imply the given constraints (which happen to be partition constraints). 
>>> How
>>> about PartConstraintImpliedByRelConstraint()? The idea is to pick a general
>>> name so that the function can be used for purposes other than skipping
>>> validation scan in future.
>>
>> I liked this idea, so done.
> 
> + * skipPartConstraintValidation
> +PartConstraintImpliedByRelConstraint(Relation partrel, List *partConstraint)
> Different function names in prologue and the definition.

Oops, fixed.

> +if (predicate_implied_by(partConstraint, existConstraint, true))
> +return true;
> +
> +/* Tough luck. */
> +return false;
>
> why not to just return the return value of predicate_implied_by() as is?

Sigh.  Done. :)

> With this we can actually handle constr == NULL a bit differently.
> +if (constr == NULL)
> +return false;
>
> To be on safer side, return false when partConstraint is not NULL. If both the
> relation constraint and partConstraint are both NULL, the first does imply the
> other. Or may be leave that decision to predicate_implied_by(), which takes
> care of it right at the beginning of the function.

Rearranged code considering this comment.  Let's let
predicate_implied_by() make ultimate decisions about logical implication. :)

> 
> + * For each leaf partition, check if it we can skip the validation
>
> An extra "it".

Fixed.

> 
> + * Note that attachRel's OID is in this list.  Since we already
> + * determined above that its validation scan cannot be skipped, we
> + * need not check that again in the loop below.  If it's partitioned,
>
> I don't see code to skip checking whether scan can be 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

Thanks for working on the previous comments. The code really looks good now.

On Fri, Jun 23, 2017 at 2:29 PM, Amit Langote
 wrote:
>
>> Don't we need an exclusive lock to
>> make sure that the constraints are not changed while we are validating those?
>
> If I understand your question correctly, you meant to ask if we don't need
> the strongest lock on individual partitions while looking at their
> constraints to prove that we don't need to scan them.  We do and we do
> take the strongest lock on individual partitions even today in the second
> call to find_all_inheritors().  We're trying to eliminate the second call
> here.

The comment seems to imply that we need strongest lock only when we
"scan" the table/s.

Some more comments on 0001
- * Prevent circularity by seeing if rel is a partition of attachRel. (In
+ * Prevent circularity by seeing if rel is a partition of attachRel, (In
  * particular, this disallows making a rel a partition of itself.)
The sentence outside () doesn't have a full-stop. I think the original
construct was better.

+ * We want to avoid having to construct this list again, so we request the
"this list" is confusing here since the earlier sentence doesn't mention any
list at all. Instead we may reword it as "We will need the list of children
later to check whether any of those have a row which would not fit the
partition constraints. So, take the strongest lock ..."


 * XXX - Do we need to lock the partitions here if we already have the
 * strongest lock on attachRel?  The information we need here to check
 * for circularity cannot change without taking a lock on attachRel.
I wondered about this. Do we really need an exclusive lock to check whether
partition constraint is valid? May be we can compare this condition with ALTER
TABLE ... ADD CONSTRAINT since the children will all get a new constraint
effectively. So, exclusive lock it is.

Assert(linitial_oid(attachRel_children) ==
RelationGetRelid(attachRel));
if (attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
attachRel_children = list_delete_first(attachRel_children);
Is it necessary for this code to have OID of the relation being attached as the
first one? You could simply call list_delete_oid() instead of
list_delete_first(). If for any reason find_all_inheritors() changes the output
order, this assertion and code would need a change.\

>
>> Comments on 0002 patch.
>> Thanks for the refactoring. The code looks really good now.
>
> Thanks.
>
>> The name skipPartConstraintValidation() looks very specific to the case at
>> hand. The function is really checking whether existing constraints on the 
>> table
>> can imply the given constraints (which happen to be partition constraints). 
>> How
>> about PartConstraintImpliedByRelConstraint()? The idea is to pick a general
>> name so that the function can be used for purposes other than skipping
>> validation scan in future.
>
> I liked this idea, so done.

+ * skipPartConstraintValidation
+PartConstraintImpliedByRelConstraint(Relation partrel, List *partConstraint)
Different function names in prologue and the definition.

>
>>  * This basically returns if the partrel's existing constraints, which
>> returns "true". Add "otherwise returns false".
>>
>> if (constr != NULL)
>> {
>> ...
>> }
>> return false;
>> May be you should return false when constr == NULL (I prefer !constr, but
>> others may have different preferences.). That should save us one level of
>> indentation. At the end just return whatever predicate_implied_by() returns.
>
> Good suggestion, done.

+if (predicate_implied_by(partConstraint, existConstraint, true))
+return true;
+
+/* Tough luck. */
+return false;
why not to just return the return value of predicate_implied_by() as is?

With this we can actually handle constr == NULL a bit differently.
+if (constr == NULL)
+return false;
To be on safer side, return false when partConstraint is not NULL. If both the
relation constraint and partConstraint are both NULL, the first does imply the
other. Or may be leave that decision to predicate_implied_by(), which takes
care of it right at the beginning of the function.

+ * For each leaf partition, check if it we can skip the validation
An extra "it".

+ * Note that attachRel's OID is in this list.  Since we already
+ * determined above that its validation scan cannot be skipped, we
+ * need not check that again in the loop below.  If it's partitioned,
I don't see code to skip checking whether scan can be skipped for relation
being attached. The loop below this comments executes for every unpartitioned
table in the list of OIDs returned. Thus for an unpartitioned relation being
attached, it will try to compare the constraints again. Am I correct?

+ * comparing it to similarly-processed 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for the review again.

On 2017/06/22 19:55, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 4:06 PM, Amit Langote
>  wrote:
>>
>> Anyway, I tried to implement the refactoring in patch 0002, which is not
>> all of the patch 0001 that Jeevan posted.  Please take a look.  I wondered
>> if we should emit a NOTICE when an individual leaf partition validation
>> can be skipped?
> 
> Yes. We emit an INFO for the table being attached. We want to do the
> same for the child. Or NOTICE In both the places.

Actually, I meant INFO.

>> No point in adding a new test if the answer to that is
>> no, I'd think.

Updated the patch 0002 so that an INFO message is printed for each leaf
partition and a test for the same.

>> Attaching here 0001 which fixes the bug (unchanged from the previous
>> version) and 0002 which implements the refactoring (and the feature to
>> look at the individual leaf partitions' constraints to see if validation
>> can be skipped.)
> 
> Comments on 0001 patch.
> + *
> + * We request an exclusive lock on all the partitions, because we may
> + * decide later in this function to scan them to validate the new
> + * partition constraint.
> Does that mean that we may not scan the partitions later, in which the 
> stronger
> lock we took was not needed. Is that right?

Yes.  I wrote that comment thinking only about the deadlock hazard which
had then occurred to me, so the text somehow ended up being reflective of
that thinking.  Please see the new comment, which hopefully is more
informative.

> Don't we need an exclusive lock to
> make sure that the constraints are not changed while we are validating those?

If I understand your question correctly, you meant to ask if we don't need
the strongest lock on individual partitions while looking at their
constraints to prove that we don't need to scan them.  We do and we do
take the strongest lock on individual partitions even today in the second
call to find_all_inheritors().  We're trying to eliminate the second call
here.

With the current code, we take AccessShareLock in the first call when
checking the circularity of inheritance.  Then if attachRel doesn't have
the constraint to avoid the scan, we decide to look at individual
partitions (their rows, not constraints, as of now) when we take
AccessExclusiveLock.  That might cause a deadlock (was able to reproduce
one using the debugger).

> if (!skip_validate)
> May be we should turn this into "else" condition of the "if" just above.

Yes, done.

> +/*
> + * We already collected the list of partitions, including the table
> + * named in the command itself, which should appear at the head of 
> the
> + * list.
> + */
> +Assert(list_length(attachRel_children) >= 1 &&
> +linitial_oid(attachRel_children) == RelationGetRelid(attachRel));
> Probably the Assert should be moved next to find_all_inheritors(). But I don't
> see much value in this comment and the Assert at this place.

I agree, removed both.

> 
> +/* Skip the original table if it's partitioned. */
> +if (part_relid == RelationGetRelid(attachRel) &&
> +attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +continue;
> +
> 
> There isn't much point in checking this for every child in the loop. Instead,
> we should remove attachRel from the attachRel_children if there are more than
> one elements in the list (which means the attachRel is partitioned, may be
> verify that by Asserting).

Rearranged code considering these comments.

> Comments on 0002 patch.
> Thanks for the refactoring. The code looks really good now.

Thanks.

> The name skipPartConstraintValidation() looks very specific to the case at
> hand. The function is really checking whether existing constraints on the 
> table
> can imply the given constraints (which happen to be partition constraints). 
> How
> about PartConstraintImpliedByRelConstraint()? The idea is to pick a general
> name so that the function can be used for purposes other than skipping
> validation scan in future.

I liked this idea, so done.

>  * This basically returns if the partrel's existing constraints, which
> returns "true". Add "otherwise returns false".
> 
> if (constr != NULL)
> {
> ...
> }
> return false;
> May be you should return false when constr == NULL (I prefer !constr, but
> others may have different preferences.). That should save us one level of
> indentation. At the end just return whatever predicate_implied_by() returns.

Good suggestion, done.

Attached updated patches.

Thanks,
Amit
From ee034a3bcb25a8b516220636134fe3ed38796cfe Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 14 Jun 2017 11:32:01 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code

If the table being attached has different attnos from the parent for
the partitioning columns 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Thu, Jun 15, 2017 at 4:06 PM, Amit Langote
 wrote:
>
> Anyway, I tried to implement the refactoring in patch 0002, which is not
> all of the patch 0001 that Jeevan posted.  Please take a look.  I wondered
> if we should emit a NOTICE when an individual leaf partition validation
> can be skipped?

Yes. We emit an INFO for the table being attached. We want to do the
same for the child. Or NOTICE In both the places.

> No point in adding a new test if the answer to that is
> no, I'd think.
>
> Attaching here 0001 which fixes the bug (unchanged from the previous
> version) and 0002 which implements the refactoring (and the feature to
> look at the individual leaf partitions' constraints to see if validation
> can be skipped.)

Comments on 0001 patch.
+ *
+ * We request an exclusive lock on all the partitions, because we may
+ * decide later in this function to scan them to validate the new
+ * partition constraint.
Does that mean that we may not scan the partitions later, in which the stronger
lock we took was not needed. Is that right? Don't we need an exclusive lock to
make sure that the constraints are not changed while we are validating those?

if (!skip_validate)
May be we should turn this into "else" condition of the "if" just above.

+/*
+ * We already collected the list of partitions, including the table
+ * named in the command itself, which should appear at the head of the
+ * list.
+ */
+Assert(list_length(attachRel_children) >= 1 &&
+linitial_oid(attachRel_children) == RelationGetRelid(attachRel));
Probably the Assert should be moved next to find_all_inheritors(). But I don't
see much value in this comment and the Assert at this place.

+/* Skip the original table if it's partitioned. */
+if (part_relid == RelationGetRelid(attachRel) &&
+attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+continue;
+

There isn't much point in checking this for every child in the loop. Instead,
we should remove attachRel from the attachRel_children if there are more than
one elements in the list (which means the attachRel is partitioned, may be
verify that by Asserting).

Comments on 0002 patch.
Thanks for the refactoring. The code looks really good now.

The name skipPartConstraintValidation() looks very specific to the case at
hand. The function is really checking whether existing constraints on the table
can imply the given constraints (which happen to be partition constraints). How
about PartConstraintImpliedByRelConstraint()? The idea is to pick a general
name so that the function can be used for purposes other than skipping
validation scan in future.

 * This basically returns if the partrel's existing constraints, which
returns "true". Add "otherwise returns false".

if (constr != NULL)
{
...
}
return false;
May be you should return false when constr == NULL (I prefer !constr, but
others may have different preferences.). That should save us one level of
indentation. At the end just return whatever predicate_implied_by() returns.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/15 18:05, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 2:30 PM, Amit Langote
>  wrote:
>> On 2017/06/15 17:53, Ashutosh Bapat wrote:
>>> On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote:
> Both of the above comments are not related to the bug that is being 
> fixed, but
> they apply to the same code where the bug exists. So instead of fixing it
> twice, may be we should expand the scope of this work to cover other
> refactoring needed in this area. That might save us some rebasing and 
> commits.

 Are you saying that the patch posted on that thread should be brought over
 and discussed here?
>>>
>>> Not the whole patch, but that one particular comment, which applies to
>>> the existing code in ATExecAttachPartition(). If we fix the existing
>>> code in ATExecAttachPartition(), the refactoring patch there will
>>> inherit it when rebased.
>>
>> Yes, I too meant only the refactoring patch, which I see as patch 0001 in
>> the series of patches that Jeevan posted with the following message:
>>
>> https://www.postgresql.org/message-id/CAOgcT0NeR%3D%2BTMRTw6oq_5WrJF%2B_xG91k_nGUub29Lnv5-qmQHw%40mail.gmail.com
> 
> I think we don't need to move that patch over to here, unless you see
> that some of that refactoring is useful here. I think, we should
> continue this thread and patch independent of what happens there. If
> and when this patch gets committed, that patch will need to be
> refactored.

I do see it as useful refactoring and a way to implement a feature (which
is perhaps something worth including into v10?)  It's just that the patch
I have posted here fixes bugs, which it would be nice to get committed first.

Anyway, I tried to implement the refactoring in patch 0002, which is not
all of the patch 0001 that Jeevan posted.  Please take a look.  I wondered
if we should emit a NOTICE when an individual leaf partition validation
can be skipped?  No point in adding a new test if the answer to that is
no, I'd think.

Attaching here 0001 which fixes the bug (unchanged from the previous
version) and 0002 which implements the refactoring (and the feature to
look at the individual leaf partitions' constraints to see if validation
can be skipped.)

Thanks,
Amit
From 941ef679635e6848d72edde4721c9d0ac4e9ff45 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 14 Jun 2017 11:32:01 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code

If the table being attached has different attnos from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to the structural inequality of the corresponding Var expressions
in the partition constraint and those in the table's check constraint
expressions.  Fix this by mapping the partition constraint's expressions
to bear the partition's attnos.

Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.

Another minor fix:

Avoid creating an AT work queue entry for the table being attached if
it's partitioned.  Current coding does not lead to that happening.
---
 src/backend/commands/tablecmds.c  | 59 +++
 src/test/regress/expected/alter_table.out | 45 +++
 src/test/regress/sql/alter_table.sql  | 38 
 3 files changed, 120 insertions(+), 22 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b4425bc6af..981b7ae902 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13412,7 +13412,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
 {
RelationattachRel,
catalog;
-   List   *childrels;
+   List   *attachRel_children;
TupleConstr *attachRel_constr;
List   *partConstraint,
   *existConstraint;
@@ -13479,10 +13479,14 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
/*
 * Prevent circularity by seeing if rel is a partition of attachRel. (In
 * particular, this disallows making a rel a partition of itself.)
+*
+* We request an exclusive lock on all the partitions, because we may
+* decide later in this function to scan them to validate the new
+* partition constraint.
 */
-   childrels = find_all_inheritors(RelationGetRelid(attachRel),
-   
AccessShareLock, NULL);
-   if (list_member_oid(childrels, RelationGetRelid(rel)))
+   attachRel_children = 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Thu, Jun 15, 2017 at 2:30 PM, Amit Langote
 wrote:
> On 2017/06/15 17:53, Ashutosh Bapat wrote:
>> On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote:
 Both of the above comments are not related to the bug that is being fixed, 
 but
 they apply to the same code where the bug exists. So instead of fixing it
 twice, may be we should expand the scope of this work to cover other
 refactoring needed in this area. That might save us some rebasing and 
 commits.
>>>
>>> Are you saying that the patch posted on that thread should be brought over
>>> and discussed here?
>>
>> Not the whole patch, but that one particular comment, which applies to
>> the existing code in ATExecAttachPartition(). If we fix the existing
>> code in ATExecAttachPartition(), the refactoring patch there will
>> inherit it when rebased.
>
> Yes, I too meant only the refactoring patch, which I see as patch 0001 in
> the series of patches that Jeevan posted with the following message:
>
> https://www.postgresql.org/message-id/CAOgcT0NeR%3D%2BTMRTw6oq_5WrJF%2B_xG91k_nGUub29Lnv5-qmQHw%40mail.gmail.com

I think we don't need to move that patch over to here, unless you see
that some of that refactoring is useful here. I think, we should
continue this thread and patch independent of what happens there. If
and when this patch gets committed, that patch will need to be
refactored.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/15 17:53, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote:
>>> Both of the above comments are not related to the bug that is being fixed, 
>>> but
>>> they apply to the same code where the bug exists. So instead of fixing it
>>> twice, may be we should expand the scope of this work to cover other
>>> refactoring needed in this area. That might save us some rebasing and 
>>> commits.
>>
>> Are you saying that the patch posted on that thread should be brought over
>> and discussed here?
> 
> Not the whole patch, but that one particular comment, which applies to
> the existing code in ATExecAttachPartition(). If we fix the existing
> code in ATExecAttachPartition(), the refactoring patch there will
> inherit it when rebased.

Yes, I too meant only the refactoring patch, which I see as patch 0001 in
the series of patches that Jeevan posted with the following message:

https://www.postgresql.org/message-id/CAOgcT0NeR%3D%2BTMRTw6oq_5WrJF%2B_xG91k_nGUub29Lnv5-qmQHw%40mail.gmail.com

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote
 wrote:
> Thanks for the review.
>
> On 2017/06/15 16:08, Ashutosh Bapat wrote:
>> On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote:
>>> If we end up having to perform the validation scan and the table being
>>> attached is a partitioned table, we will scan its leaf partitions.  Each
>>> of those leaf partitions may have different attribute numbers for the
>>> partitioning columns, so we will need to do the mapping, which actually we
>>> do even today.  With this patch however, we apply mapping to the generated
>>> partition constraint so that it no longer bears the original parent's
>>> attribute numbers but those of the table being attached.  Down below where
>>> we map to the leaf partition's attribute numbers, we still pass the root
>>> partitioned table as the parent.  But it may so happen that the attnos
>>> appearing in the Vars can no longer be matched with any of the root
>>> table's attribute numbers, resulting in the following code in
>>> map_variable_attnos_mutator() to trigger an error:
>>>
>>> if (attno > context->map_length || context->attno_map[attno - 1] == 0)
>>> elog(ERROR, "unexpected varattno %d in expression to be mapped",
>>>  attno);
>>>
>>> Consider this example:
>>>
>>> root: (a, b, c) partition by list (a)
>>> intermediate: (b, c, ..dropped.., a) partition by list (b)
>>> leaf: (b, c, a) partition of intermediate
>>>
>>> When attaching intermediate to root, we will generate the partition
>>> constraint and after mapping, its Vars will have attno = 4.  When trying
>>> to map the same for leaf, we currently do map_partition_varattnos(expr, 1,
>>> leaf, root).  So, the innards of map_variable_attnos will try to look for
>>> an attribute with attno = 4 in root which there isn't, so the above error
>>> will occur.  We should really be passing intermediate as parent to the
>>> mapping routine.  With the previous patch's approach, we would pass root
>>> as the parent along with partConstraintOrig which would bear the root
>>> parent's attnos.
>>
>> Thanks for the explanation. So, your earlier patch did map Vars
>> correctly for the leaf partitions of the table being attached.
>
> Yes I think.
>
>>> Please find attached the updated patch.  In addition to the already
>>> described fixes, the patch also rearranges code so that a redundant AT
>>> work queue entry is avoided.  (Currently, we end up creating one for
>>> attachRel even if it's partitioned, although it's harmless because
>>> ATRewriteTables() knows to skip partitioned tables.)
>>
>> We are calling find_all_inheritors() on attachRel twice in this function, 
>> once
>> to avoid circularity and second time for scheduling a scan. Why can't call it
>> once and reuse the result?
>
> Hmm, avoiding calling it twice would be a good idea.
>
> Also, I noticed that there might be a deadlock hazard here due to the
> lock-strength-upgrade thing happening here across the two calls.  In the
> first call to find_all_inheritors(), we request AccessShareLock, whereas
> in the second, an AccessExclusiveLock.  That ought to be fixed anyway I'd
> think.
>
> So, the first (and the only after this change) call will request an
> AccessExclusiveLock, even though we may not scan the leaf partitions if a
> suitable constraint exists on the table being attached.  Note that
> previously, we would not have exclusive-locked the leaf partitions in such
> a case, although it was deadlock-prone/buggy anyway.
>
> The updated patch includes this fix.
>
>> On the default partitioning thread [1] Robert commented that we should try to
>> avoid queueing the subpartitions which have constraints that imply the new
>> partitioning constraint. I think that comment applies to this code, which the
>> refactoring patch has moved into a function. If you do this, instead of
>> duplicating the code to gather existing constraints, please create a function
>> for gathering constraints of a given relation and use it for the table being
>> attached as well as its partitions. Also, we should avoid matching
>> constraints for the table being attached twice, when it's not
>> partitioned.
>
> I guess you are talking about the case where the table being attached
> itself does not have a check constraint that would help avoid the scan,
> but its individual leaf partitions (if any) may.

Right.

>
>> Both of the above comments are not related to the bug that is being fixed, 
>> but
>> they apply to the same code where the bug exists. So instead of fixing it
>> twice, may be we should expand the scope of this work to cover other
>> refactoring needed in this area. That might save us some rebasing and 
>> commits.
>
> Are you saying that the patch posted on that thread should be brought over
> and discussed here?

Not the whole patch, but that one particular comment, which applies to
the existing code in ATExecAttachPartition(). If we fix the existing
code in ATExecAttachPartition(), the 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for the review.

On 2017/06/15 16:08, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote:
>> If we end up having to perform the validation scan and the table being
>> attached is a partitioned table, we will scan its leaf partitions.  Each
>> of those leaf partitions may have different attribute numbers for the
>> partitioning columns, so we will need to do the mapping, which actually we
>> do even today.  With this patch however, we apply mapping to the generated
>> partition constraint so that it no longer bears the original parent's
>> attribute numbers but those of the table being attached.  Down below where
>> we map to the leaf partition's attribute numbers, we still pass the root
>> partitioned table as the parent.  But it may so happen that the attnos
>> appearing in the Vars can no longer be matched with any of the root
>> table's attribute numbers, resulting in the following code in
>> map_variable_attnos_mutator() to trigger an error:
>>
>> if (attno > context->map_length || context->attno_map[attno - 1] == 0)
>> elog(ERROR, "unexpected varattno %d in expression to be mapped",
>>  attno);
>>
>> Consider this example:
>>
>> root: (a, b, c) partition by list (a)
>> intermediate: (b, c, ..dropped.., a) partition by list (b)
>> leaf: (b, c, a) partition of intermediate
>>
>> When attaching intermediate to root, we will generate the partition
>> constraint and after mapping, its Vars will have attno = 4.  When trying
>> to map the same for leaf, we currently do map_partition_varattnos(expr, 1,
>> leaf, root).  So, the innards of map_variable_attnos will try to look for
>> an attribute with attno = 4 in root which there isn't, so the above error
>> will occur.  We should really be passing intermediate as parent to the
>> mapping routine.  With the previous patch's approach, we would pass root
>> as the parent along with partConstraintOrig which would bear the root
>> parent's attnos.
> 
> Thanks for the explanation. So, your earlier patch did map Vars
> correctly for the leaf partitions of the table being attached.

Yes I think.

>> Please find attached the updated patch.  In addition to the already
>> described fixes, the patch also rearranges code so that a redundant AT
>> work queue entry is avoided.  (Currently, we end up creating one for
>> attachRel even if it's partitioned, although it's harmless because
>> ATRewriteTables() knows to skip partitioned tables.)
> 
> We are calling find_all_inheritors() on attachRel twice in this function, once
> to avoid circularity and second time for scheduling a scan. Why can't call it
> once and reuse the result?

Hmm, avoiding calling it twice would be a good idea.

Also, I noticed that there might be a deadlock hazard here due to the
lock-strength-upgrade thing happening here across the two calls.  In the
first call to find_all_inheritors(), we request AccessShareLock, whereas
in the second, an AccessExclusiveLock.  That ought to be fixed anyway I'd
think.

So, the first (and the only after this change) call will request an
AccessExclusiveLock, even though we may not scan the leaf partitions if a
suitable constraint exists on the table being attached.  Note that
previously, we would not have exclusive-locked the leaf partitions in such
a case, although it was deadlock-prone/buggy anyway.

The updated patch includes this fix.

> On the default partitioning thread [1] Robert commented that we should try to
> avoid queueing the subpartitions which have constraints that imply the new
> partitioning constraint. I think that comment applies to this code, which the
> refactoring patch has moved into a function. If you do this, instead of
> duplicating the code to gather existing constraints, please create a function
> for gathering constraints of a given relation and use it for the table being
> attached as well as its partitions. Also, we should avoid matching
> constraints for the table being attached twice, when it's not
> partitioned.

I guess you are talking about the case where the table being attached
itself does not have a check constraint that would help avoid the scan,
but its individual leaf partitions (if any) may.

> Both of the above comments are not related to the bug that is being fixed, but
> they apply to the same code where the bug exists. So instead of fixing it
> twice, may be we should expand the scope of this work to cover other
> refactoring needed in this area. That might save us some rebasing and commits.

Are you saying that the patch posted on that thread should be brought over
and discussed here?  I tend to agree if that helps avoid muddying the
default partition discussion with this refactoring work.

> 
> +/*
> + * Adjust the constraint to match this partition.
> + *
> + * Since partConstraint contains attachRel's attnos due to the
> + * mapping we did just before attempting the proof above, we pass
> + * 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote
 wrote:
> Thanks for taking a look.
>
> On 2017/06/14 20:06, Ashutosh Bapat wrote:
>> On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote
>>  wrote:
>>>
>>> By the way, I mentioned an existing problem in one of the earlier emails
>>> on this thread about differing attribute numbers in the table being
>>> attached causing predicate_implied_by() to give up due to structural
>>> inequality of Vars.  To clarify: table's check constraints will bear the
>>> table's attribute numbers whereas the partition constraint generated using
>>> get_qual_for_partbound() (the predicate) bears the parent's attribute
>>> numbers.  That results in Var arguments of the expressions passed to
>>> predicate_implied_by() not matching and causing the latter to return
>>> failure prematurely.  Attached find a patch to fix that that applies on
>>> top of your patch (added a test too).
>>
>> +* Adjust the generated constraint to match this partition's attribute
>> +* numbers.  Save the original to be used later if we decide to proceed
>> +* with the validation scan after all.
>> +*/
>> +   partConstraintOrig = copyObject(partConstraint);
>> +   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
>> +rel);
>> +
>> If the partition has different column order than the parent, its heap will 
>> also
>> have different column order. I am not able to understand the purpose of using
>> original constraints for validation using scan. Shouldn't we just use the
>> mapped constraint expressions?
>
> Actually, I dropped the approach of using partConstraintOrig altogether
> from the latest updated patch.  I will explain the problem I was trying to
> solve with that approach, which is now replaced in the new patch by, I
> think, a more correct solution.
>

I agree.

> If we end up having to perform the validation scan and the table being
> attached is a partitioned table, we will scan its leaf partitions.  Each
> of those leaf partitions may have different attribute numbers for the
> partitioning columns, so we will need to do the mapping, which actually we
> do even today.  With this patch however, we apply mapping to the generated
> partition constraint so that it no longer bears the original parent's
> attribute numbers but those of the table being attached.  Down below where
> we map to the leaf partition's attribute numbers, we still pass the root
> partitioned table as the parent.  But it may so happen that the attnos
> appearing in the Vars can no longer be matched with any of the root
> table's attribute numbers, resulting in the following code in
> map_variable_attnos_mutator() to trigger an error:
>
> if (attno > context->map_length || context->attno_map[attno - 1] == 0)
> elog(ERROR, "unexpected varattno %d in expression to be mapped",
>  attno);
>
> Consider this example:
>
> root: (a, b, c) partition by list (a)
> intermediate: (b, c, ..dropped.., a) partition by list (b)
> leaf: (b, c, a) partition of intermediate
>
> When attaching intermediate to root, we will generate the partition
> constraint and after mapping, its Vars will have attno = 4.  When trying
> to map the same for leaf, we currently do map_partition_varattnos(expr, 1,
> leaf, root).  So, the innards of map_variable_attnos will try to look for
> an attribute with attno = 4 in root which there isn't, so the above error
> will occur.  We should really be passing intermediate as parent to the
> mapping routine.  With the previous patch's approach, we would pass root
> as the parent along with partConstraintOrig which would bear the root
> parent's attnos.

Thanks for the explanation. So, your earlier patch did map Vars
correctly for the leaf partitions of the table being attached.

>
> Please find attached the updated patch.  In addition to the already
> described fixes, the patch also rearranges code so that a redundant AT
> work queue entry is avoided.  (Currently, we end up creating one for
> attachRel even if it's partitioned, although it's harmless because
> ATRewriteTables() knows to skip partitioned tables.)

We are calling find_all_inheritors() on attachRel twice in this function, once
to avoid circularity and second time for scheduling a scan. Why can't call it
once and reuse the result?

On the default partitioning thread [1] Robert commented that we should try to
avoid queueing the subpartitions which have constraints that imply the new
partitioning constraint. I think that comment applies to this code, which the
refactoring patch has moved into a function. If you do this, instead of
duplicating the code to gather existing constraints, please create a function
for gathering constraints of a given relation and use it for the table being
attached as well as its partitions. Also, we should avoid matching
constraints for the table being attached twice, when it's not

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

Thanks for taking a look.

On 2017/06/14 20:06, Ashutosh Bapat wrote:
> On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote
>  wrote:
>>
>> By the way, I mentioned an existing problem in one of the earlier emails
>> on this thread about differing attribute numbers in the table being
>> attached causing predicate_implied_by() to give up due to structural
>> inequality of Vars.  To clarify: table's check constraints will bear the
>> table's attribute numbers whereas the partition constraint generated using
>> get_qual_for_partbound() (the predicate) bears the parent's attribute
>> numbers.  That results in Var arguments of the expressions passed to
>> predicate_implied_by() not matching and causing the latter to return
>> failure prematurely.  Attached find a patch to fix that that applies on
>> top of your patch (added a test too).
> 
> +* Adjust the generated constraint to match this partition's attribute
> +* numbers.  Save the original to be used later if we decide to proceed
> +* with the validation scan after all.
> +*/
> +   partConstraintOrig = copyObject(partConstraint);
> +   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
> +rel);
> +
> If the partition has different column order than the parent, its heap will 
> also
> have different column order. I am not able to understand the purpose of using
> original constraints for validation using scan. Shouldn't we just use the
> mapped constraint expressions?

Actually, I dropped the approach of using partConstraintOrig altogether
from the latest updated patch.  I will explain the problem I was trying to
solve with that approach, which is now replaced in the new patch by, I
think, a more correct solution.

If we end up having to perform the validation scan and the table being
attached is a partitioned table, we will scan its leaf partitions.  Each
of those leaf partitions may have different attribute numbers for the
partitioning columns, so we will need to do the mapping, which actually we
do even today.  With this patch however, we apply mapping to the generated
partition constraint so that it no longer bears the original parent's
attribute numbers but those of the table being attached.  Down below where
we map to the leaf partition's attribute numbers, we still pass the root
partitioned table as the parent.  But it may so happen that the attnos
appearing in the Vars can no longer be matched with any of the root
table's attribute numbers, resulting in the following code in
map_variable_attnos_mutator() to trigger an error:

if (attno > context->map_length || context->attno_map[attno - 1] == 0)
elog(ERROR, "unexpected varattno %d in expression to be mapped",
 attno);

Consider this example:

root: (a, b, c) partition by list (a)
intermediate: (b, c, ..dropped.., a) partition by list (b)
leaf: (b, c, a) partition of intermediate

When attaching intermediate to root, we will generate the partition
constraint and after mapping, its Vars will have attno = 4.  When trying
to map the same for leaf, we currently do map_partition_varattnos(expr, 1,
leaf, root).  So, the innards of map_variable_attnos will try to look for
an attribute with attno = 4 in root which there isn't, so the above error
will occur.  We should really be passing intermediate as parent to the
mapping routine.  With the previous patch's approach, we would pass root
as the parent along with partConstraintOrig which would bear the root
parent's attnos.

Please find attached the updated patch.  In addition to the already
described fixes, the patch also rearranges code so that a redundant AT
work queue entry is avoided.  (Currently, we end up creating one for
attachRel even if it's partitioned, although it's harmless because
ATRewriteTables() knows to skip partitioned tables.)

Thanks,
Amit
From 2b25013e69d262d3c2cd83cbf7f7219d0cb2d96e Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 14 Jun 2017 11:32:01 +0900
Subject: [PATCH] Cope with differing attnos in ATExecAttachPartition code

If the table being attached has different attnos from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to the structural inequality of the corresponding Var expressions
in the partition constraint and those in the table's check constraint
expressions.  Fix this by mapping the partition constraint's expressions
to bear the partition's attnos.

Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.

Another minor fix:

Avoid creating an AT work queue entry for the table being attached if
it's partitioned.  Current coding does not lead to that 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Wed, Jun 14, 2017 at 6:15 AM, Ashutosh Bapat
 wrote:
> PFA patch set addressing comments by Tom and Amit.

LGTM.  Committed.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote
 wrote:
>
> By the way, I mentioned an existing problem in one of the earlier emails
> on this thread about differing attribute numbers in the table being
> attached causing predicate_implied_by() to give up due to structural
> inequality of Vars.  To clarify: table's check constraints will bear the
> table's attribute numbers whereas the partition constraint generated using
> get_qual_for_partbound() (the predicate) bears the parent's attribute
> numbers.  That results in Var arguments of the expressions passed to
> predicate_implied_by() not matching and causing the latter to return
> failure prematurely.  Attached find a patch to fix that that applies on
> top of your patch (added a test too).

+* Adjust the generated constraint to match this partition's attribute
+* numbers.  Save the original to be used later if we decide to proceed
+* with the validation scan after all.
+*/
+   partConstraintOrig = copyObject(partConstraint);
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+rel);
+
If the partition has different column order than the parent, its heap will also
have different column order. I am not able to understand the purpose of using
original constraints for validation using scan. Shouldn't we just use the
mapped constraint expressions?

BTW I liked the idea; this way we can keep part_6 in sync with list_parted2
even when the later changes and still manage to have different order of
attributes. Although the CHECK still assumes that there is a column "a" but
that's fine I guess.
+CREATE TABLE part_6 (
+   c int,
+   LIKE list_parted2,
+   CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 6)
+);
+ALTER TABLE part_6 DROP c;


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

PFA patch set addressing comments by Tom and Amit.

0001 is same as Robert's patch.

On Wed, Jun 14, 2017 at 7:20 AM, Robert Haas  wrote:
> On Tue, Jun 13, 2017 at 5:28 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> OK, I think I see the problem here.  predicate_implied_by() and
>>> predicate_refuted_by() differ in what they assume about the predicate
>>> evaluating to NULL, but both of them assume that if the clause
>>> evaluates to NULL, that's equivalent to false.  So there's actually no
>>> option to get the behavior we want here, which is to treat both
>>> operands using CHECK-semantics (null is true) rather than
>>> WHERE-semantics (null is false).
>>
>>> Given that, Ashutosh's proposal of passing an additional flag to
>>> predicate_implied_by() seems like the best option.  Here's a patch
>>> implementing that.
>>
>> I've not reviewed the logic changes in predtest.c in detail, but
>> I think this is a reasonable direction to go in.  Two suggestions:
>>
>> 1. predicate_refuted_by() should grow the extra argument at the
>> same time.  There's no good reason to be asymmetric.
>
> OK.

0002 has these changes.

>
>> 2. It might be clearer, and would certainly be shorter, to call the
>> extra argument something like "null_is_true".
>
> I think it's pretty darn important to make it clear that the argument
> only applies to the clauses supplied as axioms, and not to the
> predicate to be proven; if you want to control how the *predicate* is
> handled with respect to nulls, change your selection as among
> predicate_implied_by() and predicate_refuted_by().  For that reason, I
> disesteem null_is_true, but I'm open to other suggestions.

The extern functions viz. predicate_refuted_by() and
predicate_implied_by() both accept restrictinfo_list and so the new
argument gets name restrictinfo_is_check, which is fine. But the
static minions have the corresponding argument named clause but the
new argument isn't named clause_is_check. I think it would be better
to be consistent everywhere and use either clause or restrictinfo.

0004 patch does that, it renames restrictinfo_list as clause_list and
the boolean argument as clause_is_check.

0003 addresses comments by Amit Langote.

In your original patch, if restrictinfo_is_check is true, we will call
operator_predicate_proof(), which does not handle anything other than
an operator expression. So, it's going to return false from that
function. Looking at the way argisrow is handled in that function, it
looks like we don't want to pass NullTest expression to
operator_predicate_proof(). 0005 handles the boolean flag in the same
way as argisrow is handled.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


extend-predicate-implied-by-v2.tar.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/14 5:36, Robert Haas wrote:
> On Tue, Jun 13, 2017 at 10:24 AM, Robert Haas  wrote:
>> I think that's going to come as an unpleasant surprise to more than
>> one user.  I'm not sure exactly how we need to restructure things here
>> so that this works properly, and maybe modifying
>> predicate_implied_by() isn't the right thing at all; for instance,
>> there's also predicate_refuted_by(), which maybe could be used in some
>> way (inject NOT?).  But I don't much like the idea that you copy and
>> paste the partitioning constraint into a CHECK constraint and that
>> doesn't work.  That's not cool.
> 
> OK, I think I see the problem here.  predicate_implied_by() and
> predicate_refuted_by() differ in what they assume about the predicate
> evaluating to NULL, but both of them assume that if the clause
> evaluates to NULL, that's equivalent to false.  So there's actually no
> option to get the behavior we want here, which is to treat both
> operands using CHECK-semantics (null is true) rather than
> WHERE-semantics (null is false).
> 
> Given that, Ashutosh's proposal of passing an additional flag to
> predicate_implied_by() seems like the best option.  Here's a patch
> implementing that.

I tried this patch and it seems to work correctly.

Some comments on the patch itself:

The following was perhaps included for debugging?

+#include "nodes/print.h"

I think the following sentence in a comment near the affected code in
ATExecAttachPartition() needs to be removed.

 *
 * There is a case in which we cannot rely on just the result of the
 * proof.

We no longer need the Bitmapset not_null_attrs.  So, the line declaring it
and the following line can be removed:

not_null_attrs = bms_add_member(not_null_attrs, i);

I thought I would make these changes myself and send the v2, but realized
that you might be updating it yourself based on Tom's comments, so didn't.

By the way, I mentioned an existing problem in one of the earlier emails
on this thread about differing attribute numbers in the table being
attached causing predicate_implied_by() to give up due to structural
inequality of Vars.  To clarify: table's check constraints will bear the
table's attribute numbers whereas the partition constraint generated using
get_qual_for_partbound() (the predicate) bears the parent's attribute
numbers.  That results in Var arguments of the expressions passed to
predicate_implied_by() not matching and causing the latter to return
failure prematurely.  Attached find a patch to fix that that applies on
top of your patch (added a test too).

Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3ca23c8ef5..7fa054f56a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13416,6 +13416,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
List   *childrels;
TupleConstr *attachRel_constr;
List   *partConstraint,
+  *partConstraintOrig,
   *existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13581,6 +13582,15 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.  Save the original to be used later if we decide to proceed
+* with the validation scan after all.
+*/
+   partConstraintOrig = copyObject(partConstraint);
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+   
 rel);
+
+   /*
 * Check if we can do away with having to scan the table being attached 
to
 * validate the partition constraint, by *proving* that the existing
 * constraints of the table *imply* the partition predicate.  We include
@@ -13715,7 +13725,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
tab = ATGetQueueEntry(wqueue, part_rel);
 
/* Adjust constraint to match this partition */
-   constr = linitial(partConstraint);
+   constr = linitial(partConstraintOrig);
tab->partition_constraint = (Expr *)
map_partition_varattnos((List *) constr, 1,

part_rel, rel);
diff --git a/src/test/regress/expected/alter_table.out 
b/src/test/regress/expected/alter_table.out
index 13d6a4b747..ec67b4cc73 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3347,6 +3347,16 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a;
 ALTER TABLE part_5 ADD CONSTRAINT 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/13 23:24, Robert Haas wrote:
> On Mon, Jun 12, 2017 at 4:09 AM, Amit Langote
>  wrote:
>> On 2017/06/09 20:49, Ashutosh Bapat wrote:
>>> May be we should pass a flag to predicate_implied_by() to handle NULL
>>> behaviour for CHECK constraints. Partitioning has shown that it needs
>>> to use predicate_implied_by() for comparing constraints and there may
>>> be other cases that can come up in future. Instead of handling it
>>> outside predicate_implied_by() we may want to change it under a flag.
>>
>> IMHO, it may not be a good idea to modify predtest.c to suit the
>> partitioning code's needs.  The workaround of checking that NOT NULL
>> constraints on partitioning columns exist seems to me to be simpler than
>> hacking predtest.c to teach it about the new behavior.
> 
> On the plus side, it might also work correctly.  I mean, the problem
> with what you've done here is that (a) you're completely giving up on
> expressions as partition keys and (b) even if no expressions are used
> for partitioning, you're still giving up unless there are NOT NULL
> constraints on the partitions.  Now, maybe that doesn't sound so bad,
> but what it means is that if you copy-and-paste the partition
> constraint into a CHECK constraint on a new table, you can't skip the
> validation scan when attaching it:
> 
> rhaas=# create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> rhaas=# create table foo1 partition of foo for values from (0) to (10);
> CREATE TABLE
> rhaas=# \d+ foo1
> Table "public.foo1"
>  Column |  Type   | Collation | Nullable | Default | Storage  | Stats
> target | Description
> +-+---+--+-+--+--+-
>  a  | integer |   |  | | plain|  |
>  b  | text|   |  | | extended |  |
> Partition of: foo FOR VALUES FROM (0) TO (10)
> Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 10))
> 
> rhaas=# drop table foo1;
> DROP TABLE
> rhaas=# create table foo1 (like foo, check ((a IS NOT NULL) AND (a >=
> 0) AND (a < 10)));
> CREATE TABLE
> rhaas=# alter table foo attach partition foo1 for values from (0) to (10);
> ALTER TABLE
> 
> I think that's going to come as an unpleasant surprise to more than
> one user.  I'm not sure exactly how we need to restructure things here
> so that this works properly, and maybe modifying
> predicate_implied_by() isn't the right thing at all; for instance,
> there's also predicate_refuted_by(), which maybe could be used in some
> way (inject NOT?).  But I don't much like the idea that you copy and
> paste the partitioning constraint into a CHECK constraint and that
> doesn't work.  That's not cool.

I agree with this argument.  I just tried the patch you posted in the
other email and I like how easy it makes the life for users in that they
can just look at the partition constraint of an existing partition (thanks
to 1848b73d45!) and frame the check constraint of the new partition to
attach accordingly.

IOW, +1 from me to the Ashutosh's idea.

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Tue, Jun 13, 2017 at 5:28 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> OK, I think I see the problem here.  predicate_implied_by() and
>> predicate_refuted_by() differ in what they assume about the predicate
>> evaluating to NULL, but both of them assume that if the clause
>> evaluates to NULL, that's equivalent to false.  So there's actually no
>> option to get the behavior we want here, which is to treat both
>> operands using CHECK-semantics (null is true) rather than
>> WHERE-semantics (null is false).
>
>> Given that, Ashutosh's proposal of passing an additional flag to
>> predicate_implied_by() seems like the best option.  Here's a patch
>> implementing that.
>
> I've not reviewed the logic changes in predtest.c in detail, but
> I think this is a reasonable direction to go in.  Two suggestions:
>
> 1. predicate_refuted_by() should grow the extra argument at the
> same time.  There's no good reason to be asymmetric.

OK.

> 2. It might be clearer, and would certainly be shorter, to call the
> extra argument something like "null_is_true".

I think it's pretty darn important to make it clear that the argument
only applies to the clauses supplied as axioms, and not to the
predicate to be proven; if you want to control how the *predicate* is
handled with respect to nulls, change your selection as among
predicate_implied_by() and predicate_refuted_by().  For that reason, I
disesteem null_is_true, but I'm open to other suggestions.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Tom Lane]

Robert Haas  writes:
> OK, I think I see the problem here.  predicate_implied_by() and
> predicate_refuted_by() differ in what they assume about the predicate
> evaluating to NULL, but both of them assume that if the clause
> evaluates to NULL, that's equivalent to false.  So there's actually no
> option to get the behavior we want here, which is to treat both
> operands using CHECK-semantics (null is true) rather than
> WHERE-semantics (null is false).

> Given that, Ashutosh's proposal of passing an additional flag to
> predicate_implied_by() seems like the best option.  Here's a patch
> implementing that.

I've not reviewed the logic changes in predtest.c in detail, but
I think this is a reasonable direction to go in.  Two suggestions:

1. predicate_refuted_by() should grow the extra argument at the
same time.  There's no good reason to be asymmetric.

2. It might be clearer, and would certainly be shorter, to call the
extra argument something like "null_is_true".

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Tue, Jun 13, 2017 at 10:24 AM, Robert Haas  wrote:
> I think that's going to come as an unpleasant surprise to more than
> one user.  I'm not sure exactly how we need to restructure things here
> so that this works properly, and maybe modifying
> predicate_implied_by() isn't the right thing at all; for instance,
> there's also predicate_refuted_by(), which maybe could be used in some
> way (inject NOT?).  But I don't much like the idea that you copy and
> paste the partitioning constraint into a CHECK constraint and that
> doesn't work.  That's not cool.

OK, I think I see the problem here.  predicate_implied_by() and
predicate_refuted_by() differ in what they assume about the predicate
evaluating to NULL, but both of them assume that if the clause
evaluates to NULL, that's equivalent to false.  So there's actually no
option to get the behavior we want here, which is to treat both
operands using CHECK-semantics (null is true) rather than
WHERE-semantics (null is false).

Given that, Ashutosh's proposal of passing an additional flag to
predicate_implied_by() seems like the best option.  Here's a patch
implementing that.

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


extend-predicate-implied-by-v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Mon, Jun 12, 2017 at 4:09 AM, Amit Langote
 wrote:
> On 2017/06/09 20:49, Ashutosh Bapat wrote:
>> May be we should pass a flag to predicate_implied_by() to handle NULL
>> behaviour for CHECK constraints. Partitioning has shown that it needs
>> to use predicate_implied_by() for comparing constraints and there may
>> be other cases that can come up in future. Instead of handling it
>> outside predicate_implied_by() we may want to change it under a flag.
>
> IMHO, it may not be a good idea to modify predtest.c to suit the
> partitioning code's needs.  The workaround of checking that NOT NULL
> constraints on partitioning columns exist seems to me to be simpler than
> hacking predtest.c to teach it about the new behavior.

On the plus side, it might also work correctly.  I mean, the problem
with what you've done here is that (a) you're completely giving up on
expressions as partition keys and (b) even if no expressions are used
for partitioning, you're still giving up unless there are NOT NULL
constraints on the partitions.  Now, maybe that doesn't sound so bad,
but what it means is that if you copy-and-paste the partition
constraint into a CHECK constraint on a new table, you can't skip the
validation scan when attaching it:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# create table foo1 partition of foo for values from (0) to (10);
CREATE TABLE
rhaas=# \d+ foo1
Table "public.foo1"
 Column |  Type   | Collation | Nullable | Default | Storage  | Stats
target | Description
+-+---+--+-+--+--+-
 a  | integer |   |  | | plain|  |
 b  | text|   |  | | extended |  |
Partition of: foo FOR VALUES FROM (0) TO (10)
Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 10))

rhaas=# drop table foo1;
DROP TABLE
rhaas=# create table foo1 (like foo, check ((a IS NOT NULL) AND (a >=
0) AND (a < 10)));
CREATE TABLE
rhaas=# alter table foo attach partition foo1 for values from (0) to (10);
ALTER TABLE

I think that's going to come as an unpleasant surprise to more than
one user.  I'm not sure exactly how we need to restructure things here
so that this works properly, and maybe modifying
predicate_implied_by() isn't the right thing at all; for instance,
there's also predicate_refuted_by(), which maybe could be used in some
way (inject NOT?).  But I don't much like the idea that you copy and
paste the partitioning constraint into a CHECK constraint and that
doesn't work.  That's not cool.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/09 20:49, Ashutosh Bapat wrote:
> May be we should pass a flag to predicate_implied_by() to handle NULL
> behaviour for CHECK constraints. Partitioning has shown that it needs
> to use predicate_implied_by() for comparing constraints and there may
> be other cases that can come up in future. Instead of handling it
> outside predicate_implied_by() we may want to change it under a flag.

IMHO, it may not be a good idea to modify predtest.c to suit the
partitioning code's needs.  The workaround of checking that NOT NULL
constraints on partitioning columns exist seems to me to be simpler than
hacking predtest.c to teach it about the new behavior.

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/09 20:47, Ashutosh Bapat wrote:
> Thanks for the long explanation. I guess, this should be written in
> comments somewhere in the code there. I see following comment in
> ATExecAttachPartition()
> --
>  *
>  * There is a case in which we cannot rely on just the result of the
>  * proof.
>  */
> 
> --
> 
> I guess, this comment should be expanded to explain what you wrote above.

Tried in the attached updated patch.

Thanks,
Amit
From a98ea14dd3aa57f06d7066ee996c54f42a3014ac Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 8 Jun 2017 14:01:34 +0900
Subject: [PATCH] Fixes around partition constraint handling when attaching

Failure to map attribute numbers in the partition key expressions to
to partition's would cause predicate_implied_by() to unnecessarily
return 'false', in turn causing the failure to skip the validation
scan.

Conversely, failure to map partition's NOT NULL column's attribute
numbers to parent's might cause incorrect conclusion about which
columns of the partition being attached must have NOT NULL constraint
defined on them.

Rearrange code and comments a little around this area to make things
clearer.
---
 src/backend/commands/tablecmds.c  | 122 +++---
 src/test/regress/expected/alter_table.out |  32 
 src/test/regress/sql/alter_table.sql  |  31 
 3 files changed, 126 insertions(+), 59 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9909..13fbed9431 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13409,6 +13409,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
List   *childrels;
TupleConstr *attachRel_constr;
List   *partConstraint,
+  *partConstraintOrig,
   *existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13574,14 +13575,37 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.  Save the original to be used later if we decide to proceed
+* with the validation scan after all.
+*/
+   partConstraintOrig = copyObject(partConstraint);
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+   
 rel);
+
+   /*
 * Check if we can do away with having to scan the table being attached 
to
 * validate the partition constraint, by *proving* that the existing
 * constraints of the table *imply* the partition predicate.  We include
-* the table's check constraints and NOT NULL constraints in the list of
-* clauses passed to predicate_implied_by().
-*
-* There is a case in which we cannot rely on just the result of the
-* proof.
+* only the table's check constraints in the list of "restrictions" 
passed
+* to predicate_implied_by() and do not include the NullTest expressions
+* corresponding to any NOT NULL constraints that the table might have,
+* because we do not want to depend on predicate_implied_by's proof for
+* the requirement (if any) that there not be any null values in the
+* partition keys of the existing rows.  As written in the comments at
+* the top of predicate_implied_by_simple_clause(), the restriction 
clause
+* that it receives is assumed to be a WHERE restriction wherein NULL
+* result means 'false' so that any rows that it selects must have 
non-null
+* value in the respective column (an implicit NOT NULL constraint).  
But
+* in this case, we are passing the list of restrictions that are 
table's
+* constraints wherein NULL result means 'true', so even rows with null
+* values in the respective columns would have been admitted into the
+* table, whereas, per aforementioned, predicate_implied_by() proof 
would
+* say there cannot be any null values in those columns based on those
+* restrictions.  The only way to confirm NOT NULLness then is by 
looking
+* for explicit NOT NULL constraints on the partition key columns.  If 
any
+* partition key is an expression, for which there cannot be a NOT NULL
+* constraint, we simply give up on skipping the scan.
 */
attachRel_constr = tupleDesc->constr;
existConstraint = NIL;
@@ -13589,42 +13613,36 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
{
int num_check = attachRel_constr->num_check;
int i;
+   AttrNumber *parent_attnos;

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

May be we should pass a flag to predicate_implied_by() to handle NULL
behaviour for CHECK constraints. Partitioning has shown that it needs
to use predicate_implied_by() for comparing constraints and there may
be other cases that can come up in future. Instead of handling it
outside predicate_implied_by() we may want to change it under a flag.

On Fri, Jun 9, 2017 at 11:43 AM, Amit Langote
 wrote:
> On 2017/06/08 18:43, Amit Langote wrote:
>> On 2017/06/08 1:44, Robert Haas wrote:
>>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>>>  wrote:
 In ATExecAttachPartition() there's following code

 13715 partnatts = get_partition_natts(key);
 13716 for (i = 0; i < partnatts; i++)
 13717 {
 13718 AttrNumber  partattno;
 13719
 13720 partattno = get_partition_col_attnum(key, i);
 13721
 13722 /* If partition key is an expression, must not skip
 validation */
 13723 if (!partition_accepts_null &&
 13724 (partattno == 0 ||
 13725  !bms_is_member(partattno, not_null_attrs)))
 13726 skip_validate = false;
 13727 }

 partattno obtained from the partition key is the attribute number of
 the partitioned table but not_null_attrs contains the attribute
 numbers of attributes of the table being attached which have NOT NULL
 constraint on them. But the attribute numbers of partitioned table and
 the table being attached may not agree i.e. partition key attribute in
 partitioned table may have a different position in the table being
 attached. So, this code looks buggy. Probably we don't have a test
 which tests this code with different attribute order between
 partitioned table and the table being attached. I didn't get time to
 actually construct a testcase and test it.
>>
>> There seem to be couple of bugs here:
>>
>> 1. When partition's key attributes differ in ordering from the parent,
>>predicate_implied_by() will give up due to structural inequality of
>>Vars in the expressions.  By fixing this, we can get it to return
>>'true' when it's really so.
>>
>> 2. As you said, we store partition's attribute numbers in the
>>not_null_attrs bitmap, but then check partattno (which is the parent's
>>attribute number which might differ) against the bitmap, which seems
>>like it might produce incorrect result.  If, for example,
>>predicate_implied_by() set skip_validate to true, and the above code
>>failed to set skip_validate to false where it should have, then we
>>would wrongly end up skipping the scan.  That is, rows in the partition
>>will contain null values whereas the partition constraint does not
>>allow it.  It's hard to reproduce this currently, because with
>>different ordering of attributes, predicate_refute_by() never returns
>>true (as mentioned in 1 above), so skip_validate does not need to be
>>set to false again.
>>
>> Consider this example:
>>
>> create table p (a int, b char) partition by list (a);
>>
>> create table p1 (b char not null, a int check (a in (1)));
>> insert into p1 values ('b', null);
>>
>> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
>> which matches key attribute of the parent, that is 1, corresponding to
>> column a.  Hence we end up wrongly concluding that p1's partition key
>> column does not allow nulls.
>
> [ ... ]
>
>> I am working on a patch to fix the above mentioned issues and will post
>> the same no later than Friday.
>
> And here is the patch.
>
> Thanks,
> Amit



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Fri, Jun 9, 2017 at 10:31 AM, Amit Langote
 wrote:
> On 2017/06/08 19:25, Ashutosh Bapat wrote:
>> On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote
 I think this code could be removed entirely in light of commit
 3ec76ff1f2cf52e9b900349957b42d28128b7bc7.
>>>
>>> I am assuming you think that because now we emit IS NOT NULL constraint
>>> internally for any partition keys that do not allow null values (including
>>> all the keys of range partitions as of commit
>>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7).  But those IS NOT NULL
>>> constraint expressions are inconclusive as far as the application of
>>> predicate_implied_by() to determine if we can skip the scan is concerned.
>>> So even if predicate_implied_by() returned 'true', we cannot conclude,
>>> just based on that result, that there are not any null values in the
>>> partition keys.
>>
>> I am not able to understand this. Are you saying that
>> predicate_implied_by() returns true even when it's not implied when
>> NOT NULL constraints are involved? That sounds like a bug in
>> predicate_implied_by(), which should be fixed instead of adding code
>> to pepper over it?
>
> No, it's not a bug of predicate_implied_by().  I meant to say
> predicate_implied_by() isn't exactly designed for ATExecAttachPartition's
> purpose, especially its treatment of IS NOT NULL constraints is not
> suitable for this application.  To prove that the table cannot contain
> NULLs when it shouldn't because of the partition constraint, we must look
> for explicit NOT NULL constraints on the partition key columns, instead of
> relying on the predicate_implied_by()'s proof.  See the following
> explanation for why that is so (or at least I think is so):
>
> There is this text in the header comment of
> predicate_implied_by_simple_clause(), which is where the individual pairs
> of expressions are compared and/or passed to operator_predicate_proof(),
> which mentions that the treatment of IS NOT NULL predicate is based on the
> assumption that 'restrictions' that are passed to predicate_implied_by()
> are a query's WHERE clause restrictions, *not* CHECK constraints that are
> checked when inserting data into a table.
>
>  * When the predicate is of the form "foo IS NOT NULL", we can conclude that
>  * the predicate is implied if the clause is a strict operator or function
>  * that has "foo" as an input.  In this case the clause must yield NULL when
>  * "foo" is NULL, which we can take as equivalent to FALSE because we know
>  * we are within an AND/OR subtree of a WHERE clause.  (Again, "foo" is
>  * already known immutable, so the clause will certainly always fail.)
>  * Also, if the clause is just "foo" (meaning it's a boolean variable),
>  * the predicate is implied since the clause can't be true if "foo" is NULL.
>
> As mentioned above, note the following part: which we can take as
> equivalent to FALSE because we know we are within an AND/OR subtree of a
> WHERE clause.
>
> Which is not what we are passing to predicate_implied_by() in
> ATExecAttachPartition().  We are passing it the table's CHECK constraint
> clauses which behave differently for the NULL result on NULL input - they
> *allow* the row to be inserted.  Which means that there will be rows with
> NULLs in the partition key, even if predicate_refuted_by() said that there
> cannot be.  We will end up *wrongly* skipping the validation scan if we
> relied on just the predicate_refuted_by()'s result.  That's why there is
> code to check for explicit NOT NULL constraints on the partition key
> columns.  If there are, it's OK then to assume that all the partition
> constraints are satisfied by existing constraints.  One more thing: if any
> partition key happens to be an expression, which there cannot be NOT NULL
> constraints for, we just give up on skipping the scan, because we don't
> have any declared knowledge about whether those keys are also non-null,
> which we want for partitions that do not accept null values.
>
> Does that make sense?

Thanks for the long explanation. I guess, this should be written in
comments somewhere in the code there. I see following comment in
ATExecAttachPartition()
--
 *
 * There is a case in which we cannot rely on just the result of the
 * proof.
 */

--

I guess, this comment should be expanded to explain what you wrote above.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/08 18:43, Amit Langote wrote:
> On 2017/06/08 1:44, Robert Haas wrote:
>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>>  wrote:
>>> In ATExecAttachPartition() there's following code
>>>
>>> 13715 partnatts = get_partition_natts(key);
>>> 13716 for (i = 0; i < partnatts; i++)
>>> 13717 {
>>> 13718 AttrNumber  partattno;
>>> 13719
>>> 13720 partattno = get_partition_col_attnum(key, i);
>>> 13721
>>> 13722 /* If partition key is an expression, must not skip
>>> validation */
>>> 13723 if (!partition_accepts_null &&
>>> 13724 (partattno == 0 ||
>>> 13725  !bms_is_member(partattno, not_null_attrs)))
>>> 13726 skip_validate = false;
>>> 13727 }
>>>
>>> partattno obtained from the partition key is the attribute number of
>>> the partitioned table but not_null_attrs contains the attribute
>>> numbers of attributes of the table being attached which have NOT NULL
>>> constraint on them. But the attribute numbers of partitioned table and
>>> the table being attached may not agree i.e. partition key attribute in
>>> partitioned table may have a different position in the table being
>>> attached. So, this code looks buggy. Probably we don't have a test
>>> which tests this code with different attribute order between
>>> partitioned table and the table being attached. I didn't get time to
>>> actually construct a testcase and test it.
> 
> There seem to be couple of bugs here:
> 
> 1. When partition's key attributes differ in ordering from the parent,
>predicate_implied_by() will give up due to structural inequality of
>Vars in the expressions.  By fixing this, we can get it to return
>'true' when it's really so.
> 
> 2. As you said, we store partition's attribute numbers in the
>not_null_attrs bitmap, but then check partattno (which is the parent's
>attribute number which might differ) against the bitmap, which seems
>like it might produce incorrect result.  If, for example,
>predicate_implied_by() set skip_validate to true, and the above code
>failed to set skip_validate to false where it should have, then we
>would wrongly end up skipping the scan.  That is, rows in the partition
>will contain null values whereas the partition constraint does not
>allow it.  It's hard to reproduce this currently, because with
>different ordering of attributes, predicate_refute_by() never returns
>true (as mentioned in 1 above), so skip_validate does not need to be
>set to false again.
> 
> Consider this example:
> 
> create table p (a int, b char) partition by list (a);
> 
> create table p1 (b char not null, a int check (a in (1)));
> insert into p1 values ('b', null);
> 
> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
> which matches key attribute of the parent, that is 1, corresponding to
> column a.  Hence we end up wrongly concluding that p1's partition key
> column does not allow nulls.

[ ... ]

> I am working on a patch to fix the above mentioned issues and will post
> the same no later than Friday.

And here is the patch.

Thanks,
Amit
From fef05d64bfad370f79e9fc305eee3d430a8f5263 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 8 Jun 2017 14:01:34 +0900
Subject: [PATCH] Fixes around partition constraint handling when attaching

Failure to map attribute numbers in the partition key expressions to
to partition's would cause predicate_implied_by() to unnecessarily
return 'false', in turn causing the failure to skip the validation
scan.

Conversely, failure to map partition's NOT NULL column's attribute
numbers to parent's might cause incorrect conclusion about which
columns of the partition being attached must have NOT NULL constraint
defined on them.

Rearrange code and comments a little around this area to make things
clearer.
---
 src/backend/commands/tablecmds.c  | 109 ++
 src/test/regress/expected/alter_table.out |  32 +
 src/test/regress/sql/alter_table.sql  |  31 +
 3 files changed, 113 insertions(+), 59 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9909..481bc97155 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13409,6 +13409,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
List   *childrels;
TupleConstr *attachRel_constr;
List   *partConstraint,
+  *partConstraintOrig,
   *existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13574,14 +13575,24 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this 

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/08 19:25, Ashutosh Bapat wrote:
> On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote
>>> I think this code could be removed entirely in light of commit
>>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7.
>>
>> I am assuming you think that because now we emit IS NOT NULL constraint
>> internally for any partition keys that do not allow null values (including
>> all the keys of range partitions as of commit
>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7).  But those IS NOT NULL
>> constraint expressions are inconclusive as far as the application of
>> predicate_implied_by() to determine if we can skip the scan is concerned.
>> So even if predicate_implied_by() returned 'true', we cannot conclude,
>> just based on that result, that there are not any null values in the
>> partition keys.
> 
> I am not able to understand this. Are you saying that
> predicate_implied_by() returns true even when it's not implied when
> NOT NULL constraints are involved? That sounds like a bug in
> predicate_implied_by(), which should be fixed instead of adding code
> to pepper over it?

No, it's not a bug of predicate_implied_by().  I meant to say
predicate_implied_by() isn't exactly designed for ATExecAttachPartition's
purpose, especially its treatment of IS NOT NULL constraints is not
suitable for this application.  To prove that the table cannot contain
NULLs when it shouldn't because of the partition constraint, we must look
for explicit NOT NULL constraints on the partition key columns, instead of
relying on the predicate_implied_by()'s proof.  See the following
explanation for why that is so (or at least I think is so):

There is this text in the header comment of
predicate_implied_by_simple_clause(), which is where the individual pairs
of expressions are compared and/or passed to operator_predicate_proof(),
which mentions that the treatment of IS NOT NULL predicate is based on the
assumption that 'restrictions' that are passed to predicate_implied_by()
are a query's WHERE clause restrictions, *not* CHECK constraints that are
checked when inserting data into a table.

 * When the predicate is of the form "foo IS NOT NULL", we can conclude that
 * the predicate is implied if the clause is a strict operator or function
 * that has "foo" as an input.  In this case the clause must yield NULL when
 * "foo" is NULL, which we can take as equivalent to FALSE because we know
 * we are within an AND/OR subtree of a WHERE clause.  (Again, "foo" is
 * already known immutable, so the clause will certainly always fail.)
 * Also, if the clause is just "foo" (meaning it's a boolean variable),
 * the predicate is implied since the clause can't be true if "foo" is NULL.

As mentioned above, note the following part: which we can take as
equivalent to FALSE because we know we are within an AND/OR subtree of a
WHERE clause.

Which is not what we are passing to predicate_implied_by() in
ATExecAttachPartition().  We are passing it the table's CHECK constraint
clauses which behave differently for the NULL result on NULL input - they
*allow* the row to be inserted.  Which means that there will be rows with
NULLs in the partition key, even if predicate_refuted_by() said that there
cannot be.  We will end up *wrongly* skipping the validation scan if we
relied on just the predicate_refuted_by()'s result.  That's why there is
code to check for explicit NOT NULL constraints on the partition key
columns.  If there are, it's OK then to assume that all the partition
constraints are satisfied by existing constraints.  One more thing: if any
partition key happens to be an expression, which there cannot be NOT NULL
constraints for, we just give up on skipping the scan, because we don't
have any declared knowledge about whether those keys are also non-null,
which we want for partitions that do not accept null values.

Does that make sense?

Thanks,
Amit

PS: Also interesting to note is the difference in behavior between
ExecQual() and ExecCheck() on NULL result.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Ashutosh Bapat]

On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote
 wrote:
> On 2017/06/08 1:44, Robert Haas wrote:
>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>>  wrote:
>>> In ATExecAttachPartition() there's following code
>>>
>>> 13715 partnatts = get_partition_natts(key);
>>> 13716 for (i = 0; i < partnatts; i++)
>>> 13717 {
>>> 13718 AttrNumber  partattno;
>>> 13719
>>> 13720 partattno = get_partition_col_attnum(key, i);
>>> 13721
>>> 13722 /* If partition key is an expression, must not skip
>>> validation */
>>> 13723 if (!partition_accepts_null &&
>>> 13724 (partattno == 0 ||
>>> 13725  !bms_is_member(partattno, not_null_attrs)))
>>> 13726 skip_validate = false;
>>> 13727 }
>>>
>>> partattno obtained from the partition key is the attribute number of
>>> the partitioned table but not_null_attrs contains the attribute
>>> numbers of attributes of the table being attached which have NOT NULL
>>> constraint on them. But the attribute numbers of partitioned table and
>>> the table being attached may not agree i.e. partition key attribute in
>>> partitioned table may have a different position in the table being
>>> attached. So, this code looks buggy. Probably we don't have a test
>>> which tests this code with different attribute order between
>>> partitioned table and the table being attached. I didn't get time to
>>> actually construct a testcase and test it.
>
> There seem to be couple of bugs here:
>
> 1. When partition's key attributes differ in ordering from the parent,
>predicate_implied_by() will give up due to structural inequality of
>Vars in the expressions.  By fixing this, we can get it to return
>'true' when it's really so.
>
> 2. As you said, we store partition's attribute numbers in the
>not_null_attrs bitmap, but then check partattno (which is the parent's
>attribute number which might differ) against the bitmap, which seems
>like it might produce incorrect result.  If, for example,
>predicate_implied_by() set skip_validate to true, and the above code
>failed to set skip_validate to false where it should have, then we
>would wrongly end up skipping the scan.  That is, rows in the partition
>will contain null values whereas the partition constraint does not
>allow it.  It's hard to reproduce this currently, because with
>different ordering of attributes, predicate_refute_by() never returns
>true (as mentioned in 1 above), so skip_validate does not need to be
>set to false again.
>
> Consider this example:
>
> create table p (a int, b char) partition by list (a);
>
> create table p1 (b char not null, a int check (a in (1)));
> insert into p1 values ('b', null);
>
> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
> which matches key attribute of the parent, that is 1, corresponding to
> column a.  Hence we end up wrongly concluding that p1's partition key
> column does not allow nulls.
>
>> I think this code could be removed entirely in light of commit
>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7.
>
> I am assuming you think that because now we emit IS NOT NULL constraint
> internally for any partition keys that do not allow null values (including
> all the keys of range partitions as of commit
> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7).  But those IS NOT NULL
> constraint expressions are inconclusive as far as the application of
> predicate_implied_by() to determine if we can skip the scan is concerned.
> So even if predicate_implied_by() returned 'true', we cannot conclude,
> just based on that result, that there are not any null values in the
> partition keys.

I am not able to understand this. Are you saying that
predicate_implied_by() returns true even when it's not implied when
NOT NULL constraints are involved? That sounds like a bug in
predicate_implied_by(), which should be fixed instead of adding code
to pepper over it?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/06/08 1:44, Robert Haas wrote:
> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>  wrote:
>> In ATExecAttachPartition() there's following code
>>
>> 13715 partnatts = get_partition_natts(key);
>> 13716 for (i = 0; i < partnatts; i++)
>> 13717 {
>> 13718 AttrNumber  partattno;
>> 13719
>> 13720 partattno = get_partition_col_attnum(key, i);
>> 13721
>> 13722 /* If partition key is an expression, must not skip
>> validation */
>> 13723 if (!partition_accepts_null &&
>> 13724 (partattno == 0 ||
>> 13725  !bms_is_member(partattno, not_null_attrs)))
>> 13726 skip_validate = false;
>> 13727 }
>>
>> partattno obtained from the partition key is the attribute number of
>> the partitioned table but not_null_attrs contains the attribute
>> numbers of attributes of the table being attached which have NOT NULL
>> constraint on them. But the attribute numbers of partitioned table and
>> the table being attached may not agree i.e. partition key attribute in
>> partitioned table may have a different position in the table being
>> attached. So, this code looks buggy. Probably we don't have a test
>> which tests this code with different attribute order between
>> partitioned table and the table being attached. I didn't get time to
>> actually construct a testcase and test it.

There seem to be couple of bugs here:

1. When partition's key attributes differ in ordering from the parent,
   predicate_implied_by() will give up due to structural inequality of
   Vars in the expressions.  By fixing this, we can get it to return
   'true' when it's really so.

2. As you said, we store partition's attribute numbers in the
   not_null_attrs bitmap, but then check partattno (which is the parent's
   attribute number which might differ) against the bitmap, which seems
   like it might produce incorrect result.  If, for example,
   predicate_implied_by() set skip_validate to true, and the above code
   failed to set skip_validate to false where it should have, then we
   would wrongly end up skipping the scan.  That is, rows in the partition
   will contain null values whereas the partition constraint does not
   allow it.  It's hard to reproduce this currently, because with
   different ordering of attributes, predicate_refute_by() never returns
   true (as mentioned in 1 above), so skip_validate does not need to be
   set to false again.

Consider this example:

create table p (a int, b char) partition by list (a);

create table p1 (b char not null, a int check (a in (1)));
insert into p1 values ('b', null);

Note that not_null_attrs for p1 will contain 1 corresponding to column b,
which matches key attribute of the parent, that is 1, corresponding to
column a.  Hence we end up wrongly concluding that p1's partition key
column does not allow nulls.

> I think this code could be removed entirely in light of commit
> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7.

I am assuming you think that because now we emit IS NOT NULL constraint
internally for any partition keys that do not allow null values (including
all the keys of range partitions as of commit
3ec76ff1f2cf52e9b900349957b42d28128b7bc7).  But those IS NOT NULL
constraint expressions are inconclusive as far as the application of
predicate_implied_by() to determine if we can skip the scan is concerned.
So even if predicate_implied_by() returned 'true', we cannot conclude,
just based on that result, that there are not any null values in the
partition keys.

The code in question is there to check if there are explicit NOT NULL
constraints on the partition keys.  It cannot be true for expression keys,
so we give up on skip_validate in that case anyway.  But if 1) there are
no expression keys, 2) all the partition key columns of the table being
attached have NOT NULL constraint, and 3) predicate_implied_by() returned
'true', we can skip the scan.

Thoughts?

I am working on a patch to fix the above mentioned issues and will post
the same no later than Friday.

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
 wrote:
> In ATExecAttachPartition() there's following code
>
> 13715 partnatts = get_partition_natts(key);
> 13716 for (i = 0; i < partnatts; i++)
> 13717 {
> 13718 AttrNumber  partattno;
> 13719
> 13720 partattno = get_partition_col_attnum(key, i);
> 13721
> 13722 /* If partition key is an expression, must not skip
> validation */
> 13723 if (!partition_accepts_null &&
> 13724 (partattno == 0 ||
> 13725  !bms_is_member(partattno, not_null_attrs)))
> 13726 skip_validate = false;
> 13727 }
>
> partattno obtained from the partition key is the attribute number of
> the partitioned table but not_null_attrs contains the attribute
> numbers of attributes of the table being attached which have NOT NULL
> constraint on them. But the attribute numbers of partitioned table and
> the table being attached may not agree i.e. partition key attribute in
> partitioned table may have a different position in the table being
> attached. So, this code looks buggy. Probably we don't have a test
> which tests this code with different attribute order between
> partitioned table and the table being attached. I didn't get time to
> actually construct a testcase and test it.

I think this code could be removed entirely in light of commit
3ec76ff1f2cf52e9b900349957b42d28128b7bc7.

Amit?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers