Re: problems with foreign keys on partitioned tables

[Amit Langote]

On 2019/01/25 2:18, Alvaro Herrera wrote:
> On 2019-Jan-24, Amit Langote wrote:
> 
>> A few hunks of the originally proposed patch are attached here as 0001,
>> especially the part which fixes ATAddForeignKeyConstraint to pass the
>> correct value of connoninherit to CreateConstraintEntry (which should be
>> false for partitioned tables).  With that change, many tests start failing
>> because of the above bug.  That patch also adds a test case like the one
>> above, but it fails along with others due to the bug.  Patch 0002 is the
>> aforementioned simpler fix to make the errors (existing and the newly
>> added) go away.
> 
> Cool, thanks.  I made a bunch of fixes -- I added an elog(ERROR) check
> to ensure a constraint whose parent is being set does not already have a
> parent; that seemed in line with the new asserts that check the
> coninhcount.  I also moved those asserts, changing the spirit of what
> they checked.  Also: I wasn't sure about stopping recursion for legacy
> inheritance in ATExecDropConstraint() for non-check constraints, so I
> changed that to occur in partitioned only.  Also, stylistic fixes.

Thanks for the fixes and committing.

> I was mildly surprised to realize that the my_fkey constraint on
> fk_part_1_1 is gone after dropping fkey on its parent, since it was
> declared locally when that table was created.  However, it makes perfect
> sense in retrospect, since we made it dependent on its parent.  I'm not
> terribly happy about this, but I don't quite see a way to make it better
> that doesn't require much more code than is warranted.

Fwiw, CHECK constraints behave that way too.  OTOH, detaching a partition
preserves all the constraints, even the ones that were never locally
defined on the partition.

>> These patches apply unchanged to the PG 11 branch.
> 
> Yeah, only if you tried to compile, it would have complained about
> table_close() ;-)

Oops, sorry.  I was really in a hurry that day as the dinnertime had passed.

Thanks,
Amit

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

On 2019-Jan-24, Amit Langote wrote:

> A few hunks of the originally proposed patch are attached here as 0001,
> especially the part which fixes ATAddForeignKeyConstraint to pass the
> correct value of connoninherit to CreateConstraintEntry (which should be
> false for partitioned tables).  With that change, many tests start failing
> because of the above bug.  That patch also adds a test case like the one
> above, but it fails along with others due to the bug.  Patch 0002 is the
> aforementioned simpler fix to make the errors (existing and the newly
> added) go away.

Cool, thanks.  I made a bunch of fixes -- I added an elog(ERROR) check
to ensure a constraint whose parent is being set does not already have a
parent; that seemed in line with the new asserts that check the
coninhcount.  I also moved those asserts, changing the spirit of what
they checked.  Also: I wasn't sure about stopping recursion for legacy
inheritance in ATExecDropConstraint() for non-check constraints, so I
changed that to occur in partitioned only.  Also, stylistic fixes.

I was mildly surprised to realize that the my_fkey constraint on
fk_part_1_1 is gone after dropping fkey on its parent, since it was
declared locally when that table was created.  However, it makes perfect
sense in retrospect, since we made it dependent on its parent.  I'm not
terribly happy about this, but I don't quite see a way to make it better
that doesn't require much more code than is warranted.

> These patches apply unchanged to the PG 11 branch.

Yeah, only if you tried to compile, it would have complained about
table_close() ;-)

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

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

On 2019-Jan-25, Amit Langote wrote:

> On Fri, Jan 25, 2019 at 12:30 AM Amit Langote  wrote:

> > Doesn't the following performDeletion() at the start of
> > ATExecDropConstraint(), through findDependentObject()'s own recursion,
> > take care of deleting *all* constraints, including those of?
> 
> Meant to say: "...including those of the grandchildren?"
> 
> > /*
> >  * Perform the actual constraint deletion
> >  */
> > conobj.classId = ConstraintRelationId;
> > conobj.objectId = con->oid;
> > conobj.objectSubId = 0;
> >
> > performDeletion(, behavior, 0);

Of course it does when the dependencies are set up -- but in the
approach we just gave up on, those dependencies would not exist.
Anyway, my motivation was that performMultipleDeletions has the
advantage that it collects all objects to be dropped before deleting
anyway, and so the error that a constraint was dropped in a previous
recursion step would not occur.

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

Re: problems with foreign keys on partitioned tables

[Amit Langote]

On Fri, Jan 25, 2019 at 12:30 AM Amit Langote  wrote:
>
> On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera
>  wrote:
> > On 2019-Jan-24, Amit Langote wrote:
> >
> > > Thinking more on this, my proposal to rip dependencies between parent and
> > > child constraints altogether to resolve the bug I initially reported is
> > > starting to sound a bit overambitious especially considering that we'd
> > > need to back-patch it (the patch didn't even consider index constraints
> > > properly, creating a divergence between the behaviors of inherited foreign
> > > key constraints and inherited index constraints).  We can pursue it if
> > > only to avoid bloating the catalog for what can be achieved with little
> > > bit of additional code in tablecmds.c, but maybe we should refrain from
> > > doing it in reaction to this particular bug.
> >
> > While studying your fix it occurred to me that perhaps we could change
> > things so that we first collect a list of objects to drop, and only when
> > we're done recursing perform the deletion, as per the attached patch.
> > However, this fails for the test case in your 0001 patch (but not the
> > one you show in your email body), because you added a stealthy extra
> > ingredient to it: the constraint in the grandchild has a different name,
> > so when ATExecDropConstraint() tries to search for it by name, it's just
> > not there, not because it was dropped but because it has never existed
> > in the first place.
>
> Doesn't the following performDeletion() at the start of
> ATExecDropConstraint(), through findDependentObject()'s own recursion,
> take care of deleting *all* constraints, including those of?

Meant to say: "...including those of the grandchildren?"

> /*
>  * Perform the actual constraint deletion
>  */
> conobj.classId = ConstraintRelationId;
> conobj.objectId = con->oid;
> conobj.objectSubId = 0;
>
> performDeletion(, behavior, 0);

Thanks,
Amit

Re: problems with foreign keys on partitioned tables

[Amit Langote]

On Fri, Jan 25, 2019 at 12:08 AM Alvaro Herrera
 wrote:
> On 2019-Jan-24, Amit Langote wrote:
>
> > Thinking more on this, my proposal to rip dependencies between parent and
> > child constraints altogether to resolve the bug I initially reported is
> > starting to sound a bit overambitious especially considering that we'd
> > need to back-patch it (the patch didn't even consider index constraints
> > properly, creating a divergence between the behaviors of inherited foreign
> > key constraints and inherited index constraints).  We can pursue it if
> > only to avoid bloating the catalog for what can be achieved with little
> > bit of additional code in tablecmds.c, but maybe we should refrain from
> > doing it in reaction to this particular bug.
>
> While studying your fix it occurred to me that perhaps we could change
> things so that we first collect a list of objects to drop, and only when
> we're done recursing perform the deletion, as per the attached patch.
> However, this fails for the test case in your 0001 patch (but not the
> one you show in your email body), because you added a stealthy extra
> ingredient to it: the constraint in the grandchild has a different name,
> so when ATExecDropConstraint() tries to search for it by name, it's just
> not there, not because it was dropped but because it has never existed
> in the first place.

Doesn't the following performDeletion() at the start of
ATExecDropConstraint(), through findDependentObject()'s own recursion,
take care of deleting *all* constraints, including those of?

/*
 * Perform the actual constraint deletion
 */
conobj.classId = ConstraintRelationId;
conobj.objectId = con->oid;
conobj.objectSubId = 0;

performDeletion(, behavior, 0);

> Unless I misunderstand, this means that your plan to remove those
> catalog tuples won't work at all, because there is no way to find those
> constraints other than via pg_depend if they have different names.

Yeah, that's right.  Actually, I gave up on developing the patch
further based on that approach (no-dependencies approach) when I
edited the test to give the grandchild constraint its own name.

> I'm leaning towards the idea that your patch is the definitive fix and
> not just a backpatchable band-aid.

Yeah, I think so too.

Thanks,
Amit

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

Hello

On 2019-Jan-24, Amit Langote wrote:

> Thinking more on this, my proposal to rip dependencies between parent and
> child constraints altogether to resolve the bug I initially reported is
> starting to sound a bit overambitious especially considering that we'd
> need to back-patch it (the patch didn't even consider index constraints
> properly, creating a divergence between the behaviors of inherited foreign
> key constraints and inherited index constraints).  We can pursue it if
> only to avoid bloating the catalog for what can be achieved with little
> bit of additional code in tablecmds.c, but maybe we should refrain from
> doing it in reaction to this particular bug.

While studying your fix it occurred to me that perhaps we could change
things so that we first collect a list of objects to drop, and only when
we're done recursing perform the deletion, as per the attached patch.
However, this fails for the test case in your 0001 patch (but not the
one you show in your email body), because you added a stealthy extra
ingredient to it: the constraint in the grandchild has a different name,
so when ATExecDropConstraint() tries to search for it by name, it's just
not there, not because it was dropped but because it has never existed
in the first place.

Unless I misunderstand, this means that your plan to remove those
catalog tuples won't work at all, because there is no way to find those
constraints other than via pg_depend if they have different names.

I'm leaning towards the idea that your patch is the definitive fix and
not just a backpatchable band-aid.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 747acbd2b9..29860acaa2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -417,7 +417,7 @@ static void CloneFkReferencing(Relation pg_constraint, Relation parentRel,
 static void ATExecDropConstraint(Relation rel, const char *constrName,
 	 DropBehavior behavior,
 	 bool recurse, bool recursing,
-	 bool missing_ok, LOCKMODE lockmode);
+	 bool missing_ok, LOCKMODE lockmode, ObjectAddresses *objsToDelete);
 static void ATPrepAlterColumnType(List **wqueue,
 	  AlteredTableInfo *tab, Relation rel,
 	  bool recurse, bool recursing,
@@ -4160,12 +4160,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		case AT_DropConstraint: /* DROP CONSTRAINT */
 			ATExecDropConstraint(rel, cmd->name, cmd->behavior,
  false, false,
- cmd->missing_ok, lockmode);
+ cmd->missing_ok, lockmode, NULL);
 			break;
 		case AT_DropConstraintRecurse:	/* DROP CONSTRAINT with recursion */
 			ATExecDropConstraint(rel, cmd->name, cmd->behavior,
  true, false,
- cmd->missing_ok, lockmode);
+ cmd->missing_ok, lockmode, NULL);
 			break;
 		case AT_AlterColumnType:	/* ALTER COLUMN TYPE */
 			address = ATExecAlterColumnType(tab, rel, cmd, lockmode);
@@ -9219,7 +9219,8 @@ static void
 ATExecDropConstraint(Relation rel, const char *constrName,
 	 DropBehavior behavior,
 	 bool recurse, bool recursing,
-	 bool missing_ok, LOCKMODE lockmode)
+	 bool missing_ok, LOCKMODE lockmode,
+	 ObjectAddresses *objsToDelete)
 {
 	List	   *children;
 	ListCell   *child;
@@ -9237,6 +9238,12 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 
 	conrel = heap_open(ConstraintRelationId, RowExclusiveLock);
 
+	if (!recursing)
+	{
+		Assert(objsToDelete == NULL);
+		objsToDelete = new_object_addresses();
+	}
+
 	/*
 	 * Find and drop the target constraint
 	 */
@@ -9290,13 +9297,13 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 		}
 
 		/*
-		 * Perform the actual constraint deletion
+		 * Register the constraint for deletion
 		 */
 		conobj.classId = ConstraintRelationId;
 		conobj.objectId = HeapTupleGetOid(tuple);
 		conobj.objectSubId = 0;
 
-		performDeletion(, behavior, 0);
+		add_exact_object_address(, objsToDelete);
 
 		found = true;
 	}
@@ -9402,7 +9409,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 /* Time to delete this child constraint, too */
 ATExecDropConstraint(childrel, constrName, behavior,
 	 true, true,
-	 false, lockmode);
+	 false, lockmode, objsToDelete);
 			}
 			else
 			{
@@ -9435,6 +9442,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 		heap_close(childrel, NoLock);
 	}
 
+	if (!recursing)
+		performMultipleDeletions(objsToDelete, behavior, 0);
+
 	heap_close(conrel, RowExclusiveLock);
 }
 

Re: problems with foreign keys on partitioned tables

[Amit Langote]

Hi,

On 2019/01/24 6:13, Alvaro Herrera wrote:
> On 2019-Jan-22, Amit Langote wrote:
>> Done.  See the attached patches for HEAD and PG 11.
> 
> I'm not quite sure I understand why the one in DefineIndex needs the
> deps but nothing else does.  I fear that you added that one just to
> appease the existing test that breaks otherwise, and I worry that with
> that addition we're papering over some other, more fundamental bug.

Thinking more on this, my proposal to rip dependencies between parent and
child constraints altogether to resolve the bug I initially reported is
starting to sound a bit overambitious especially considering that we'd
need to back-patch it (the patch didn't even consider index constraints
properly, creating a divergence between the behaviors of inherited foreign
key constraints and inherited index constraints).  We can pursue it if
only to avoid bloating the catalog for what can be achieved with little
bit of additional code in tablecmds.c, but maybe we should refrain from
doing it in reaction to this particular bug.

I've updated the patch that implements a much simpler fix for this
particular bug.  Just to reiterate, the following illustrates the bug:

create table pk (a int primary key);
create table p (a int references pk) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p detach partition p1;
alter table p1 drop constraint p_a_fkey;
ERROR:  constraint "p_a_fkey" of relation "p11" does not exist

The error occurs because ATExecDropConstraint when recursively called on
p11 cannot find the constraint as the dependency mechanism already dropped
it.  The new fix is to return from ATExecDropConstraint without recursing
if the constraint being dropped is index or foreign constraint.

A few hunks of the originally proposed patch are attached here as 0001,
especially the part which fixes ATAddForeignKeyConstraint to pass the
correct value of connoninherit to CreateConstraintEntry (which should be
false for partitioned tables).  With that change, many tests start failing
because of the above bug.  That patch also adds a test case like the one
above, but it fails along with others due to the bug.  Patch 0002 is the
aforementioned simpler fix to make the errors (existing and the newly
added) go away.

These patches apply unchanged to the PG 11 branch.

Thanks,
Amit
From 8eb5ce74e8656b517ad5a4e960b70de0ff3bedd5 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 24 Jan 2019 21:29:17 +0900
Subject: [PATCH 1/2] Fix ATAddForeignKeyConstraint to use correct value of
 connoinherit

While at it, add some Asserts to ConstraintSetParentConstraint to
assert the correct value of coninhcount.

Many tests fail, but the next patch should take care of them.
---
 src/backend/catalog/pg_constraint.c   | 11 +--
 src/backend/commands/tablecmds.c  |  5 -
 src/test/regress/expected/foreign_key.out | 18 --
 src/test/regress/sql/foreign_key.sql  | 15 ++-
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c 
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..d2b8489b6c 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -785,6 +785,12 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid 
parentConstrId)
{
constrForm->conislocal = false;
constrForm->coninhcount++;
+   /*
+* An inherited foreign key constraint can never have more than 
one
+* parent, because inheriting foreign keys is only allowed for
+* partitioning where multiple inheritance is disallowed.
+*/
+   Assert(constrForm->coninhcount == 1);
constrForm->conparentid = parentConstrId;
 
CatalogTupleUpdate(constrRel, >t_self, newtup);
@@ -797,8 +803,9 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid 
parentConstrId)
else
{
constrForm->coninhcount--;
-   if (constrForm->coninhcount <= 0)
-   constrForm->conislocal = true;
+   /* See the above comment. */
+   Assert(constrForm->coninhcount == 0);
+   constrForm->conislocal = true;
constrForm->conparentid = InvalidOid;
 
deleteDependencyRecordsForClass(ConstraintRelationId, 
childConstrId,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 738c178107..fea4d99735 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7251,6 +7251,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo 
*tab, Relation rel,
boolold_check_ok;
ObjectAddress address;
ListCell   *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
+   /* Partitioned table's foreign 

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

On 2019-Jan-22, Amit Langote wrote:

> On 2019/01/22 8:30, Alvaro Herrera wrote:
> > Hi Amit,
> > 
> > Will you please rebase 0002?  Please add your proposed tests cases to
> > it, too.
> 
> Done.  See the attached patches for HEAD and PG 11.

I'm not quite sure I understand why the one in DefineIndex needs the
deps but nothing else does.  I fear that you added that one just to
appease the existing test that breaks otherwise, and I worry that with
that addition we're papering over some other, more fundamental bug.

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

Re: problems with foreign keys on partitioned tables

[Amit Langote]

On 2019/01/22 8:30, Alvaro Herrera wrote:
> Hi Amit,
> 
> Will you please rebase 0002?  Please add your proposed tests cases to
> it, too.

Done.  See the attached patches for HEAD and PG 11.

Thanks,
Amit

From 432c4551990d0da1c77b6b9523296b0a2a0a5119 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 22 Jan 2019 13:20:54 +0900
Subject: [PATCH] Do not track foreign key inheritance by dependencies

Inheritance information maintained in pg_constraint is enough to
prevent a child constraint to be dropped and for them to be dropped
when the parent constraint is dropped.  So, do not create
dependencies between the parent foreign key constraint and its
children.

Also, fix ATAddForeignKeyConstraint to set connoniherit on the parent
constraint correctly.
---
 src/backend/catalog/pg_constraint.c   | 27 +--
 src/backend/commands/indexcmds.c  | 22 --
 src/backend/commands/tablecmds.c  | 24 
 src/test/regress/expected/foreign_key.out | 17 +++--
 src/test/regress/sql/foreign_key.sql  | 14 +-
 5 files changed, 73 insertions(+), 31 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c 
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..855d57c65a 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -762,8 +762,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
  * ConstraintSetParentConstraint
  * Set a partition's constraint as child of its parent table's
  *
- * This updates the constraint's pg_constraint row to show it as inherited, and
- * add a dependency to the parent so that it cannot be removed on its own.
+ * This updates the constraint's pg_constraint row to change its inheritance
+ * properties.
  */
 void
 ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
@@ -772,8 +772,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid 
parentConstrId)
Form_pg_constraint constrForm;
HeapTuple   tuple,
newtup;
-   ObjectAddress depender;
-   ObjectAddress referenced;
 
constrRel = table_open(ConstraintRelationId, RowExclusiveLock);
tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId));
@@ -785,25 +783,26 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid 
parentConstrId)
{
constrForm->conislocal = false;
constrForm->coninhcount++;
+
+   /*
+* An inherited foreign key constraint can never have more than 
one
+* parent, because inheriting foreign keys is only allowed for
+* partitioning where multiple inheritance is disallowed.
+*/
+   Assert(constrForm->coninhcount == 1);
+
constrForm->conparentid = parentConstrId;
 
CatalogTupleUpdate(constrRel, >t_self, newtup);
-
-   ObjectAddressSet(referenced, ConstraintRelationId, 
parentConstrId);
-   ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
-
-   recordDependencyOn(, , 
DEPENDENCY_INTERNAL_AUTO);
}
else
{
constrForm->coninhcount--;
-   if (constrForm->coninhcount <= 0)
-   constrForm->conislocal = true;
+   /* See the above comment. */
+   Assert(constrForm->coninhcount == 0);
+   constrForm->conislocal = true;
constrForm->conparentid = InvalidOid;
 
-   deleteDependencyRecordsForClass(ConstraintRelationId, 
childConstrId,
-   
ConstraintRelationId,
-   
DEPENDENCY_INTERNAL_AUTO);
CatalogTupleUpdate(constrRel, >t_self, newtup);
}
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3edc94308e..6c8aa5d149 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -972,8 +972,26 @@ DefineIndex(Oid relationId,
/* Attach index to parent and 
we're done. */
IndexSetParentIndex(cldidx, 
indexRelationId);
if (createdConstraintId != 
InvalidOid)
-   
ConstraintSetParentConstraint(cldConstrOid,
-   
  createdConstraintId);
+   {
+   ObjectAddress depender;
+   ObjectAddress 
referenced;
+
+   

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

Hi Amit,

Will you please rebase 0002?  Please add your proposed tests cases to
it, too.

Thanks,

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

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

Pushed now, thanks.

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

Re: problems with foreign keys on partitioned tables

[Amit Langote]

On Sat, Jan 19, 2019 at 7:16 AM Alvaro Herrera  wrote:
> Thanks, this is better.  There were a few other things I didn't like, so
> I updated it.  Mostly, two things:
>
> 1. I didn't like a seqscan on pg_trigger, so I turned that into an
> indexed scan on the constraint OID, and then the other two conditions
> are checked in the returned tuples.  Also, what's the point on
> duplicating code and checking how many you deleted?  Just delete them
> all.

Yeah, I didn't quite like what that code looked like, but it didn't
occur to me that there's an index on tgconstraint.

It looks much better now.

> 2. I didn't like the ABI break, and it wasn't necessary: you can just
> call createForeignKeyActionTriggers directly.  That's much simpler.

OK.

> I also added tests.  While running them, I noticed that my previous
> commit was broken in terms of relcache invalidation.  I don't really
> know if this is a new problem with that commit, or an existing one.  The
> fix is 0001.

Looks good.

Thanks,
Amit

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

On 2019-Jan-18, Amit Langote wrote:

> OK, I agree.  I have updated the patch to make things work that way.

Thanks, this is better.  There were a few other things I didn't like, so
I updated it.  Mostly, two things:

1. I didn't like a seqscan on pg_trigger, so I turned that into an
indexed scan on the constraint OID, and then the other two conditions
are checked in the returned tuples.  Also, what's the point on
duplicating code and checking how many you deleted?  Just delete them
all.

2. I didn't like the ABI break, and it wasn't necessary: you can just
call createForeignKeyActionTriggers directly.  That's much simpler.

I also added tests.  While running them, I noticed that my previous
commit was broken in terms of relcache invalidation.  I don't really
know if this is a new problem with that commit, or an existing one.  The
fix is 0001.

Haven't got around to your 0002 yet.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 2047f054facf3ffaff31e36591279b9bd76ca53c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 18 Jan 2019 19:06:40 -0300
Subject: [PATCH 1/2] Add missing relcache reset

Adding a foreign key to a partitioned table requires applying a relcache
reset to each partition, so that the newly added FK is correctly
recorded in its entry on next rebuild.

With the previous coding of ATAddForeignKeyConstraint, this may not have
been necessary, but with the one after 0325d7a5957b, it is.

Noted while testing another bugfix.
---
 src/backend/commands/tablecmds.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1a1ac698e5..5a27f64aa7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7700,6 +7700,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 
 			childtab->constraints = lappend(childtab->constraints, newcon);
 
+			/*
+			 * Also invalidate the partition's relcache entry, so that its
+			 * FK list gets updated on next rebuild.
+			 */
+			CacheInvalidateRelcache(partition);
+
 			heap_close(partition, lockmode);
 		}
 	}
-- 
2.11.0

>From 42b96db12aafea22285893e1d96c63a7f684abfd Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 18 Jan 2019 19:10:29 -0300
Subject: [PATCH 2/2] Fix action triggers for FK constraints on detached
 partitions

---
 src/backend/commands/tablecmds.c | 72 +++-
 src/test/regress/sql/foreign_key.sql | 20 +-
 2 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5a27f64aa7..02a55ed3d6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7858,6 +7858,10 @@ CloneFkReferencing(Relation pg_constraint, Relation parentRel,
 			ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell);
 			Form_pg_constraint partConstr;
 			HeapTuple	partcontup;
+			Relation	trigrel;
+			HeapTuple	trigtup;
+			SysScanDesc scan;
+			ScanKeyData key;
 
 			attach_it = true;
 
@@ -7909,7 +7913,39 @@ CloneFkReferencing(Relation pg_constraint, Relation parentRel,
 
 			ReleaseSysCache(partcontup);
 
-			/* looks good!  Attach this constraint */
+			/*
+			 * Looks good!  Attach this constraint.  Note that the action
+			 * triggers are no longer needed, so remove them.  We identify
+			 * them because they have our constraint OID, as well as being
+			 * on the referenced rel.
+			 */
+			trigrel = heap_open(TriggerRelationId, RowExclusiveLock);
+			ScanKeyInit(,
+		Anum_pg_trigger_tgconstraint,
+		BTEqualStrategyNumber, F_OIDEQ,
+		ObjectIdGetDatum(fk->conoid));
+
+			scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true,
+	  NULL, 1, );
+			while ((trigtup = systable_getnext(scan)) != NULL)
+			{
+Form_pg_trigger	trgform = (Form_pg_trigger) GETSTRUCT(trigtup);
+
+if (trgform->tgconstrrelid != fk->conrelid)
+	continue;
+if (trgform->tgrelid != fk->confrelid)
+	continue;
+
+deleteDependencyRecordsForClass(TriggerRelationId,
+trgform->oid,
+ConstraintRelationId,
+DEPENDENCY_INTERNAL);
+CatalogTupleDelete(trigrel, >t_self);
+			}
+
+			systable_endscan(scan);
+			heap_close(trigrel, RowExclusiveLock);
+
 			ConstraintSetParentConstraint(fk->conoid, parentConstrOid);
 			CommandCounterIncrement();
 			attach_it = true;
@@ -15080,19 +15116,51 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	}
 	heap_close(classRel, RowExclusiveLock);
 
-	/* Detach foreign keys */
+	/*
+	 * Detach any foreign keys that are inherited.  This includes creating
+	 * additional action triggers.
+	 */
 	fks = copyObject(RelationGetFKeyList(partRel));
 	foreach(cell, fks)
 	{
 		ForeignKeyCacheInfo *fk = lfirst(cell);
 		HeapTuple	contup;
+		Form_pg_constraint conform;
+		Constraint *fkconstraint;
 
 

Re: problems with foreign keys on partitioned tables

[Amit Langote]

On 2019/01/18 7:54, Alvaro Herrera wrote:
> On 2019-Jan-09, Amit Langote wrote:
> 
>> 1. Foreign keys of partitions stop working correctly after being detached
>> from the parent table
> 
>> This happens because the action triggers defined on the PK relation (pk)
>> refers to p as the referencing relation.  On detaching p1 from p, p1's
>> data is no longer accessible to that trigger.
> 
> Ouch.
> 
>> To fix this problem, we need create action triggers on PK relation
>> that refer to p1 when it's detached (unless such triggers already
>> exist which might be true in some cases).  Attached patch 0001 shows
>> this approach.
> 
> Hmm, okay.  I'm not in love with the idea that such triggers might
> already exist -- seems unclean.  We should remove redundant action
> triggers when we attach a table as a partition, no?

OK, I agree.  I have updated the patch to make things work that way.  With
the patch:

create table pk (a int primary key);
create table p (a int references pk) partition by list (a);

-- this query shows the action triggers on the referenced rel ('pk'), name
-- of the constraint that the trigger is part of and the foreign key rel
-- ('p', etc.)

select tgrelid::regclass as pkrel, c.conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname │ fkrel
───┼───┼───
 pk│ p_a_fkey  │ p
 pk│ p_a_fkey  │ p
(2 rows)

create table p1 (
   a int references pk,
   foreign key (a) references pk (a) ON UPDATE CASCADE ON DELETE CASCADE
DEFERRABLE,
   foreign key (a) references pk (a) MATCH FULL ON UPDATE CASCADE ON
DELETE CASCADE
) partition by list (a);

-- p1_a_fkey on 'p1' is equivalent to p_a_fkey on 'p', but they're not
-- attached yet

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───┼┼───
 pk│ p_a_fkey   │ p
 pk│ p_a_fkey   │ p
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey2 │ p1
(8 rows)

create table p11 (like p1, foreign key (a) references pk);

-- again, p11_a_fkey, p1_a_fkey, and p_a_fkey are equivalent

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───┼┼───
 pk│ p_a_fkey   │ p
 pk│ p_a_fkey   │ p
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p11_a_fkey │ p11
 pk│ p11_a_fkey │ p11
(10 rows)


alter table p1 attach partition p11 for values in (1);

-- p1_a_fkey and p11_a_fkey merged, so triggers for the latter dropped

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───┼┼───
 pk│ p_a_fkey   │ p
 pk│ p_a_fkey   │ p
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey2 │ p1
(8 rows)

-- p_a_fkey and p1_a_fkey merged, so triggers for the latter dropped

alter table p attach partition p1 for values in (1);

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───┼┼───
 pk│ p_a_fkey   │ p
 pk│ p_a_fkey   │ p
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey2 │ p1
(6 rows)


alter table p detach partition p1;

-- p1_a_fkey needs its own triggers again

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───┼┼───
 pk│ p_a_fkey   │ p
 pk│ p_a_fkey   │ p
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey  │ p1
 pk│ p1_a_fkey  │ p1
(8 rows)

alter table p1 detach partition p11;

-- p11_a_fkey needs its own triggers again

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───┼┼───
 pk│ p_a_fkey   │ p
 pk│ p_a_fkey   │ p
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey1 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey2 │ p1
 pk│ p1_a_fkey  │ p1
 pk│ 

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

On 2019-Jan-09, Amit Langote wrote:

> 1. Foreign keys of partitions stop working correctly after being detached
> from the parent table

> This happens because the action triggers defined on the PK relation (pk)
> refers to p as the referencing relation.  On detaching p1 from p, p1's
> data is no longer accessible to that trigger.

Ouch.

> To fix this problem, we need create action triggers on PK relation
> that refer to p1 when it's detached (unless such triggers already
> exist which might be true in some cases).  Attached patch 0001 shows
> this approach.

Hmm, okay.  I'm not in love with the idea that such triggers might
already exist -- seems unclean.  We should remove redundant action
triggers when we attach a table as a partition, no?

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

Re: problems with foreign keys on partitioned tables

[Alvaro Herrera]

Hi Amit

On 2019-Jan-09, Amit Langote wrote:

> I noticed a couple of problems with foreign keys on partitioned tables.

Ouch, thanks for reporting.  I think 0001 needs a bit of a tweak in pg11
to avoid an ABI break -- I intend to study this one and try to push
early next week.  I'm going to see about pushing 0002 shortly,
adding some of your tests.

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

problems with foreign keys on partitioned tables

[Amit Langote]

Hi,

I noticed a couple of problems with foreign keys on partitioned tables.

1. Foreign keys of partitions stop working correctly after being detached
from the parent table

create table pk (a int primary key);
create table p (a int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p add foreign key (a) references pk (a);

-- these things work correctly
insert into p values (1);
ERROR:  insert or update on table "p11" violates foreign key constraint
"p_a_fkey"
DETAIL:  Key (a)=(1) is not present in table "pk".
insert into pk values (1);
insert into p values (1);
delete from pk where a = 1;
ERROR:  update or delete on table "pk" violates foreign key constraint
"p_a_fkey" on table "p"
DETAIL:  Key (a)=(1) is still referenced from table "p".

-- detach p1, which preserves the foreign key key
alter table p detach partition p1;
create table p12 partition of p1 for values in (2);

-- this part of the foreign key on p1 still works
insert into p1 values (2);
ERROR:  insert or update on table "p12" violates foreign key constraint
"p_a_fkey"
DETAIL:  Key (a)=(2) is not present in table "pk".

-- but this seems wrong
delete from pk where a = 1;
DELETE 1

-- because
select * from p1;
 a
───
 1
(1 row)

This happens because the action triggers defined on the PK relation (pk)
refers to p as the referencing relation.  On detaching p1 from p, p1's
data is no longer accessible to that trigger.  To fix this problem, we
need create action triggers on PK relation that refer to p1 when it's
detached (unless such triggers already exist which might be true in some
cases).  Attached patch 0001 shows this approach.

2. Foreign keys of a partition cannot be dropped in some cases after
detaching it from the parent.

create table p (a int references pk) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p detach partition p1;

-- p1's foreign key is no longer inherited, so should be able to drop it
alter table p1 drop constraint p_a_fkey ;
ERROR:  constraint "p_a_fkey" of relation "p11" does not exist

This happens because by the time ATExecDropConstraint tries to recursively
drop the p11's inherited foreign key constraint (which is what normally
happens for inherited constraints), the latter has already been dropped by
dependency management.  I think the foreign key inheritance related code
doesn't need to add dependencies for something that inheritance recursion
can take of and I can't think of any other reason to have such
dependencies around.  I thought maybe they're needed for pg_dump to work
correctly, but apparently not so.

Interestingly, the above problem doesn't occur if the constraint is added
to partitions by inheritance recursion.

create table p (a int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p add foreign key (a) references pk (a);
alter table p detach partition p1;
alter table p1 drop constraint p_a_fkey ;
ALTER TABLE

Looking into it, that happens to work *accidentally*.

ATExecDropInherit() doesn't try to recurse, which prevents the error in
this case, because it finds that the constraint on p1 is marked NO INHERIT
(non-inheritable), which is incorrect.  The value of p1's constraint's
connoinherit (in fact, other inheritance related properties too) is
incorrect, because ATAddForeignKeyConstraint doesn't bother to set them
correctly.  This is what the inheritance properties of various copies of
'p_a_fkey' looks like in the catalog in this case:

-- case 1: foreign key added to partitions recursively
create table p (a int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p add foreign key (a) references pk (a);
select conname, conrelid::regclass, conislocal, coninhcount, connoinherit
from pg_constraint where conname like 'p%fkey%';
 conname  │ conrelid │ conislocal │ coninhcount │ connoinherit
──┼──┼┼─┼──
 p_a_fkey │ p│ t  │   0 │ t
 p_a_fkey │ p1   │ t  │   0 │ t
 p_a_fkey │ p11  │ t  │   0 │ t
(3 rows)

In this case, after detaching p1 from p, p1's foreign key's coninhcount
turns to -1, which is not good.

alter table p detach partition p1;
select conname, conrelid::regclass, conislocal, coninhcount, connoinherit
from pg_constraint where conname like 'p%fkey%';
 conname  │ conrelid │ conislocal │ coninhcount │ connoinherit
──┼──┼┼─┼──
 p_a_fkey │ p│ t  │   0 │ t
 p_

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Robert Haas]

On Fri, Jun 29, 2018 at 12:06 PM, Alvaro Herrera
 wrote:
> Okay, pushed that way.
>
> We can redo this if/when we add support for cloning BEFORE row triggers,
> which are going to need the trigger link info, I suspect.

Yeah, having an explicit representation seems less likely to be
fragile.  Following dependencies might not work if there's an extra
dependency that you aren't expecting for some reason.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Thu, Jul 5, 2018 at 6:44 AM, Robert Haas  wrote:
>
> Well, as far as I know, it's up to me which parts of your emails I want to
> quote in my reply. I did read this part. It did not change my opinion.  My
> fundamental objection to your proposal is that I think it is too wordy. I
> think users will generally know whether they are using partitioning or
> inheritance, and if they don't it's not hard to figure out.   I don't think
> quoting only part of what you wrote made the quote misleading, but it did
> allow me to express my opinion.

I would usually believe that people have read complete email before
replying. But when the reply raises a question without quoting a
portion of my mail which I think has the answer, I am confused.
There's no straight forward method to avoid that confusion given it's
written and turn based communication. But I try to get clarity on that
confusion.

> I understand that you don't agree, which is
> fine, but I stand by my position.
>

I am fine with disagreement, now that I know why there's disagreement.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Tue, Jul 3, 2018 at 12:41 AM Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> > rhaas=# drop table foo;
> > ERROR:  table "foo" does not exist
> > HINT: Try dropping a table with a different name that does exist, or
> > first create this table before trying to drop it.
>
> Again a wrong example and wrong comparison. I think I was clear about
> the problem when I wrote


I don't think this is a question of "right" vs. "wrong".  You are certainly
entitled to your opinion, but I believe that I am entitled to mine, too.

--
> When there was only a single way, i.e table
> inheritance, to "inherit" things users could probably guess that. But
> now there are multiple ways to inherit things, we have to help user a
> bit more. The user might figure out that ti's a partition of a table,
> so the constraint is inherited from the partitioned table. But it will
> help if we give a hint about from where the constraint was inherited
> like the error message itself reads like "can not drop constraint
> "p_a_check" on relation "p1" inherited from "partitioned table's name"
> OR a hint "you may drop constraint "p_a_check" on the partitioned
> table "partitioned table's name".
> --
>
> For some reason you have chosen to remove this from the email and
> commented on previous part of it.


Well, as far as I know, it's up to me which parts of your emails I want to
quote in my reply. I did read this part. It did not change my opinion.  My
fundamental objection to your proposal is that I think it is too wordy. I
think users will generally know whether they are using partitioning or
inheritance, and if they don't it's not hard to figure out.   I don't think
quoting only part of what you wrote made the quote misleading, but it did
allow me to express my opinion. I understand that you don't agree, which is
fine, but I stand by my position.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Tue, Jul 3, 2018 at 8:19 AM, Robert Haas  wrote:
> On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat
>  wrote:
>> This constraint was added to the partitioned table and inherited from
>> there. If user wants to drop that constraint for some reason, this
>> error message doesn't help. The error message tells why he can't drop
>> it, but doesn't tell, directly or indirectly, the user what he should
>> do in order to drop it.
>
> That doesn't really sound like an actual problem to me.  If the error
> is that the constraint is inherited, that suggests that the solution
> is to find the place from which it got inherited and drop it there.
> And that's in fact what you have to do.  What's the problem?  I mean,
> we could add a hint, but it's possible to make yourself sound dumb by
> giving hints that are basically obvious implications from the error
> message.  Nobody wants this sort of thing:
>
> rhaas=# drop table foo;
> ERROR:  table "foo" does not exist
> HINT: Try dropping a table with a different name that does exist, or
> first create this table before trying to drop it.

Again a wrong example and wrong comparison. I think I was clear about
the problem when I wrote

--
When there was only a single way, i.e table
inheritance, to "inherit" things users could probably guess that. But
now there are multiple ways to inherit things, we have to help user a
bit more. The user might figure out that ti's a partition of a table,
so the constraint is inherited from the partitioned table. But it will
help if we give a hint about from where the constraint was inherited
like the error message itself reads like "can not drop constraint
"p_a_check" on relation "p1" inherited from "partitioned table's name"
OR a hint "you may drop constraint "p_a_check" on the partitioned
table "partitioned table's name".
--

For some reason you have chosen to remove this from the email and
commented on previous part of it.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/07/03 11:49, Robert Haas wrote:
> On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat
>  wrote:
>> This constraint was added to the partitioned table and inherited from
>> there. If user wants to drop that constraint for some reason, this
>> error message doesn't help. The error message tells why he can't drop
>> it, but doesn't tell, directly or indirectly, the user what he should
>> do in order to drop it.
> 
> That doesn't really sound like an actual problem to me.  If the error
> is that the constraint is inherited, that suggests that the solution
> is to find the place from which it got inherited and drop it there.
> And that's in fact what you have to do.  What's the problem?  I mean,
> we could add a hint, but it's possible to make yourself sound dumb by
> giving hints that are basically obvious implications from the error
> message.  Nobody wants this sort of thing:
> 
> rhaas=# drop table foo;
> ERROR:  table "foo" does not exist
> HINT: Try dropping a table with a different name that does exist, or
> first create this table before trying to drop it.
> 
> A hint here wouldn't be as silly as that, but I think it is
> unnecessary.  I doubt there's likely to be much confusion here.

I have to agree with that.  "cannot drop inherited ..." conveys enough for
a user to find out more.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat
 wrote:
> This constraint was added to the partitioned table and inherited from
> there. If user wants to drop that constraint for some reason, this
> error message doesn't help. The error message tells why he can't drop
> it, but doesn't tell, directly or indirectly, the user what he should
> do in order to drop it.

That doesn't really sound like an actual problem to me.  If the error
is that the constraint is inherited, that suggests that the solution
is to find the place from which it got inherited and drop it there.
And that's in fact what you have to do.  What's the problem?  I mean,
we could add a hint, but it's possible to make yourself sound dumb by
giving hints that are basically obvious implications from the error
message.  Nobody wants this sort of thing:

rhaas=# drop table foo;
ERROR:  table "foo" does not exist
HINT: Try dropping a table with a different name that does exist, or
first create this table before trying to drop it.

A hint here wouldn't be as silly as that, but I think it is
unnecessary.  I doubt there's likely to be much confusion here.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/07/02 14:46, Ashutosh Bapat wrote:
> This constraint was added to the partitioned table and inherited from
> there. If user wants to drop that constraint for some reason, this
> error message doesn't help. The error message tells why he can't drop
> it, but doesn't tell, directly or indirectly, the user what he should
> do in order to drop it. When there was only a single way, i.e table
> inheritance, to "inherit" things users could probably guess that. But
> now there are multiple ways to inherit things, we have to help user a
> bit more. The user might figure out that ti's a partition of a table,
> so the constraint is inherited from the partitioned table. But it will
> help if we give a hint about from where the constraint was inherited
> like the error message itself reads like "can not drop constraint
> "p_a_check" on relation "p1" inherited from "partitioned table's name"
> OR a hint "you may drop constraint "p_a_check" on the partitioned
> table "partitioned table's name". It might even suffice to say
> "partition p1" instead "relation p1" so that the user gets a clue.

It might be a good idea to mention if the table is a partition in the HINT
message.  ERROR message can remain the same.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Fri, Jun 29, 2018 at 6:35 PM, Tomas Vondra
 wrote:
>
>
> On 06/29/2018 02:30 PM, Robert Haas wrote:
>>
>> On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote
>>  wrote:
>>>
>>> By the way, picking up on the word "inherited" in the error message shown
>>> above, I wonder if you decided against using similar terminology
>>> intentionally.
>>
>>
>> Good question.
>>
>>> I guess the thinking there is that the terminology being
>>> used extensively with columns and constraints ("inherited column/check
>>> constraint cannot be dropped", etc.) is just a legacy of partitioning
>>> sharing implementation with inheritance.
>>
>>
>> It seems to me that we can talk about things being inherited by
>> partitions even if we're calling the feature partitioning, rather than
>> inheritance.  Maybe that's confusing, but then again, maybe it's not
>> that confusing.
>>
>
> my 2c: I don't think it's confusing. Inheritance is a generic concept, not
> attached exclusively to the "table inheritance". If you look at docs from
> other databases, you'll see "partition inherits" all the time.

I generally agree with this. But I think we need to think more in the
context of the particular example Amit gave.

-- quoting Amit's example,

   Table "public.p1"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   |  |
 b  | integer |   |  |
Partition of: p FOR VALUES IN (1)
Check constraints:
"p1_b_check" CHECK (b >= 0)
"p_a_check" CHECK (a >= 0)

alter table p1 drop constraint p_a_check;
ERROR:  cannot drop inherited constraint "p_a_check" of relation "p1"

--unquote

This constraint was added to the partitioned table and inherited from
there. If user wants to drop that constraint for some reason, this
error message doesn't help. The error message tells why he can't drop
it, but doesn't tell, directly or indirectly, the user what he should
do in order to drop it. When there was only a single way, i.e table
inheritance, to "inherit" things users could probably guess that. But
now there are multiple ways to inherit things, we have to help user a
bit more. The user might figure out that ti's a partition of a table,
so the constraint is inherited from the partitioned table. But it will
help if we give a hint about from where the constraint was inherited
like the error message itself reads like "can not drop constraint
"p_a_check" on relation "p1" inherited from "partitioned table's name"
OR a hint "you may drop constraint "p_a_check" on the partitioned
table "partitioned table's name". It might even suffice to say
"partition p1" instead "relation p1" so that the user gets a clue.

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

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Alvaro Herrera]

On 2018-Jun-29, Amit Langote wrote:

> On 2018/06/29 6:23, Peter Eisentraut wrote:
> > On 6/28/18 22:52, Alvaro Herrera wrote:
> >>> Couldn't psql chase the pg_depend links to find inherited triggers?
> >>
> >> Yeah, I thought this would be exceedingly ugly, but apparently it's not
> >> *that* bad -- see the attached patch, which relies on the fact that
> >> triggers don't otherwise depend on other triggers.  We don't use this
> >> technique elsewhere in psql though.
> > 
> > Yeah, relying on pg_depend to detect relationships between catalog
> > objects is a bit evil.  We do use this for detecting sequences linked to
> > tables, which also has its share of problems.  Ideally, there would be a
> > column in pg_trigger, perhaps, that makes this link explicit.  But we
> > are looking here for a solution without catalog changes, I believe.
> 
> +1 if that gets the job done.

Okay, pushed that way.

We can redo this if/when we add support for cloning BEFORE row triggers,
which are going to need the trigger link info, I suspect.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Tomas Vondra]

On 06/29/2018 02:30 PM, Robert Haas wrote:

On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote
 wrote:

By the way, picking up on the word "inherited" in the error message shown
above, I wonder if you decided against using similar terminology
intentionally.


Good question.


I guess the thinking there is that the terminology being
used extensively with columns and constraints ("inherited column/check
constraint cannot be dropped", etc.) is just a legacy of partitioning
sharing implementation with inheritance.


It seems to me that we can talk about things being inherited by
partitions even if we're calling the feature partitioning, rather than
inheritance.  Maybe that's confusing, but then again, maybe it's not
that confusing.



my 2c: I don't think it's confusing. Inheritance is a generic concept, 
not attached exclusively to the "table inheritance". If you look at docs 
from other databases, you'll see "partition inherits" all the time.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote
 wrote:
> By the way, picking up on the word "inherited" in the error message shown
> above, I wonder if you decided against using similar terminology
> intentionally.

Good question.

> I guess the thinking there is that the terminology being
> used extensively with columns and constraints ("inherited column/check
> constraint cannot be dropped", etc.) is just a legacy of partitioning
> sharing implementation with inheritance.

It seems to me that we can talk about things being inherited by
partitions even if we're calling the feature partitioning, rather than
inheritance.  Maybe that's confusing, but then again, maybe it's not
that confusing.

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

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Amit Langote]

On 2018/06/29 6:23, Peter Eisentraut wrote:
> On 6/28/18 22:52, Alvaro Herrera wrote:
>>> Couldn't psql chase the pg_depend links to find inherited triggers?
>>
>> Yeah, I thought this would be exceedingly ugly, but apparently it's not
>> *that* bad -- see the attached patch, which relies on the fact that
>> triggers don't otherwise depend on other triggers.  We don't use this
>> technique elsewhere in psql though.
> 
> Yeah, relying on pg_depend to detect relationships between catalog
> objects is a bit evil.  We do use this for detecting sequences linked to
> tables, which also has its share of problems.  Ideally, there would be a
> column in pg_trigger, perhaps, that makes this link explicit.  But we
> are looking here for a solution without catalog changes, I believe.

+1 if that gets the job done.

Thanks,
Amit

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Peter Eisentraut]

On 6/28/18 22:52, Alvaro Herrera wrote:
>> Couldn't psql chase the pg_depend links to find inherited triggers?
> 
> Yeah, I thought this would be exceedingly ugly, but apparently it's not
> *that* bad -- see the attached patch, which relies on the fact that
> triggers don't otherwise depend on other triggers.  We don't use this
> technique elsewhere in psql though.

Yeah, relying on pg_depend to detect relationships between catalog
objects is a bit evil.  We do use this for detecting sequences linked to
tables, which also has its share of problems.  Ideally, there would be a
column in pg_trigger, perhaps, that makes this link explicit.  But we
are looking here for a solution without catalog changes, I believe.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Peter Eisentraut]

On 6/27/18 23:01, Alvaro Herrera wrote:
> Another angle is that we're already in beta3 and there may be concerns
> about altering catalog definition this late in the cycle.  Anybody?
> Maybe psql can just match tgisinternal triggers by name, and if one
> match occurs then we assume it's a clone that should be listed as a
> partition trigger.

Couldn't psql chase the pg_depend links to find inherited triggers?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/28 7:58, Alvaro Herrera wrote:
> On 2018-Jun-18, Robert Treat wrote:
>> So +1 for thinking we do need to worry about it. I'm not exactly sure
>> how we want to expose that info; with \d+ we list various "Partition
>> X:" sections, perhaps adding one for "Partition triggers:" would be
>> enough, although I am inclined to think it needs exposure at the \d
>> level. One other thing to consider is firing order of said triggers...
>> if all parent level triggers fire before child level triggers then the
>> above separation is straightforward, but if the execution order is, as
>> I suspect, mixed, then it becomes more complicated.
> 
> The firing order is alphabetical across the whole set, i.e. parent/child
> triggers are interleaved.  See the regression test output at the bottom
> of this email.
> 
> I looked into adding a "Partition triggers" section because it seemed a
> nice idea, but looking at the code I realized it's a bit tough because
> we already split triggers in "categories": normal triggers, disabled
> triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c
> line 2795).  Since the trigger being in a partition is orthogonal to
> that classification, we would end up with eight groups.  Not sure that's
> great.
> 
> The attached patch (catversion bump not included -- beware of server
> crash if you run it without initdb'ing) keeps the categories the same.

Thanks for creating the patch.

> So with my previous example setup, you can see the duplicate triggers in
> psql:
> 
> alvherre=# \d child 
>Table "public.child"
>  Column │  Type   │ Collation │ Nullable │ Default 
> ┼─┼───┼──┼─
>  a  │ integer │   │  │ 
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
> trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
> trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
> 
> and as soon as you try to drop the second one, you'll learn the truth
> about it:
> 
> alvherre=# drop trigger trig_p on child;
> ERROR:  cannot drop trigger trig_p on table child because trigger trig_p on 
> table parent requires it
> SUGERENCIA:  You can drop trigger trig_p on table parent instead.
> Time: 1,443 ms
> 
> I say this is sufficient.

Yeah.  We do same with constraints for example:

create table p (a int check (a >= 0), b int) partition by list (a);
create table p1 partition of p (b check (b >= 0)) for values in (1);
\d p1
 Table "public.p1"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   |  |
 b  | integer |   |  |
Partition of: p FOR VALUES IN (1)
Check constraints:
"p1_b_check" CHECK (b >= 0)
"p_a_check" CHECK (a >= 0)

alter table p1 drop constraint p_a_check;
ERROR:  cannot drop inherited constraint "p_a_check" of relation "p1"

More specifically, inherited triggers are not listed separately.

By the way, picking up on the word "inherited" in the error message shown
above, I wonder if you decided against using similar terminology
intentionally.  I guess the thinking there is that the terminology being
used extensively with columns and constraints ("inherited column/check
constraint cannot be dropped", etc.) is just a legacy of partitioning
sharing implementation with inheritance.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-18, Robert Treat wrote:

> One of the things I was thinking about while reading this thread is
> that the scenario of creating "duplicate" triggers on a table meaning
> two triggers doing the same thing is entirely possible now but we
> don't really do anything to prevent it, which is Ok. I've never seen
> much merit in the argument "people should test" (it assumes a certain
> infallibility that just isn't true) but we've also generally been
> pretty good about exposing what is going on so people can debug this
> type of unexpected behavior.
> 
> So +1 for thinking we do need to worry about it. I'm not exactly sure
> how we want to expose that info; with \d+ we list various "Partition
> X:" sections, perhaps adding one for "Partition triggers:" would be
> enough, although I am inclined to think it needs exposure at the \d
> level. One other thing to consider is firing order of said triggers...
> if all parent level triggers fire before child level triggers then the
> above separation is straightforward, but if the execution order is, as
> I suspect, mixed, then it becomes more complicated.

The firing order is alphabetical across the whole set, i.e. parent/child
triggers are interleaved.  See the regression test output at the bottom
of this email.

I looked into adding a "Partition triggers" section because it seemed a
nice idea, but looking at the code I realized it's a bit tough because
we already split triggers in "categories": normal triggers, disabled
triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c
line 2795).  Since the trigger being in a partition is orthogonal to
that classification, we would end up with eight groups.  Not sure that's
great.

The attached patch (catversion bump not included -- beware of server
crash if you run it without initdb'ing) keeps the categories the same.
So with my previous example setup, you can see the duplicate triggers in
psql:

alvherre=# \d child 
   Table "public.child"
 Column │  Type   │ Collation │ Nullable │ Default 
┼─┼───┼──┼─
 a  │ integer │   │  │ 
Partition of: parent FOR VALUES FROM (0) TO (100)
Triggers:
trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()

and as soon as you try to drop the second one, you'll learn the truth
about it:

alvherre=# drop trigger trig_p on child;
ERROR:  cannot drop trigger trig_p on table child because trigger trig_p on 
table parent requires it
SUGERENCIA:  You can drop trigger trig_p on table parent instead.
Time: 1,443 ms

I say this is sufficient.


-- Verify that triggers fire in alphabetical order
create table parted_trig (a int) partition by range (a);
create table parted_trig_1 partition of parted_trig for values from (0) to 
(1000)
   partition by range (a);
create table parted_trig_1_1 partition of parted_trig_1 for values from (0) to 
(100);
create table parted_trig_2 partition of parted_trig for values from (1000) to 
(2000);
create trigger zzz after insert on parted_trig for each row execute procedure 
trigger_notice();
create trigger mmm after insert on parted_trig_1_1 for each row execute 
procedure trigger_notice();
create trigger aaa after insert on parted_trig_1 for each row execute procedure 
trigger_notice();
create trigger bbb after insert on parted_trig for each row execute procedure 
trigger_notice();
create trigger qqq after insert on parted_trig_1_1 for each row execute 
procedure trigger_notice();
insert into parted_trig values (50), (1500);
NOTICE:  trigger aaa on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger bbb on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger mmm on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger qqq on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger zzz on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger bbb on parted_trig_2 AFTER INSERT for ROW
NOTICE:  trigger zzz on parted_trig_2 AFTER INSERT for ROW

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 57519fe8d6..9542856d6a 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -815,11 +815,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char 
*queryString,
 
/*
 * Build the new pg_trigger tuple.
-*
-* When we're creating a trigger in a partition, we mark it as internal,
-* even though we don't do the isInternal magic in this function.  This
-* makes the triggers in partitions identical to the ones in the
-* partitioned tables, except that they are marked internal.
 */
memset(nulls, false, sizeof(nulls));
 
@@ -829,7 +824,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
values[Anum_pg_trigger_tgfoid - 1] = 

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Alvaro Herrera]

On 2018-Jun-28, David Rowley wrote:

> On 28 June 2018 at 09:01, Alvaro Herrera  wrote:
> > Another angle is that we're already in beta3 and there may be concerns
> > about altering catalog definition this late in the cycle.  Anybody?
> > Maybe psql can just match tgisinternal triggers by name, and if one
> > match occurs then we assume it's a clone that should be listed as a
> > partition trigger.
> 
> Are catversion bumps really a huge problem in beta?  Looks like there
> were quite a few during the v10 cycle.

Hm, I remember they were somewhat of a big deal.  Maybe it's not so
anymore and just can pg_upgrade easily?  Of course, once a beta contains
one, and subsequent ones before the next beta are "free".

Your patch for partition pruning will also require one, by the looks, so
I guess it's pretty much settled ...

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

Re: Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[David Rowley]

On 28 June 2018 at 09:01, Alvaro Herrera  wrote:
> Another angle is that we're already in beta3 and there may be concerns
> about altering catalog definition this late in the cycle.  Anybody?
> Maybe psql can just match tgisinternal triggers by name, and if one
> match occurs then we assume it's a clone that should be listed as a
> partition trigger.

Are catversion bumps really a huge problem in beta?  Looks like there
were quite a few during the v10 cycle.

$ git log REL_10_BETA2..REL_10_BETA3 --oneline --
src\include\catalog\catversion.h | wc -l
  1

$ git log REL_10_BETA1..REL_10_BETA2 --oneline --
src\include\catalog\catversion.h | wc -l
  9

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Listing triggers in partitions (was Re: Remove mention in docs that foreign keys on partitioned tables)

[Alvaro Herrera]

Gmail users: this comes from 
https://postgr.es/m/20180627191819.6g73wu7ck23fdhv6@alvherre.pgsql

On 2018-Jun-27, Alvaro Herrera wrote:

> On 2018-Jun-19, Amit Langote wrote:
> 
> > In CreateTrigger(), 86f575948c7 did this.
> > 
> > -values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
> > +values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
> > in_partition);
> > 
> > I'm not sure why it had to be done, but undoing this change doesn't seem
> > to break any regression tests (neither those added by 86f575948c7 nor of
> > the partitioned table foreign key commit).  Did we really intend to change
> > the meaning of tginternal like this here?
> 
> I'm pretty sure pg_dump breaks if you turn that flag off for triggers in
> partitions, because pg_dump is going to emit commands to create those
> triggers, and those fail.

After idlying some more on this, I think one possibility is to add
another column to pg_trigger to differentiate triggers that should not
be shown by psql nor dumped, from triggers that should be shown by psql
but not dumped.

Two possibilities:
a) a boolean flag, "trgpartition" or something like that, indicating
that this is a trigger in a partition.  pg_dump does not dump such
triggers, but psql shows them.

b) an OID, pointing to the parent trigger.  This could theoretically
help a future implementation of BEFORE ROW triggers, as discussed
before (if trigger trgparent has already run for this row, don't run the
current trigger).  However, it's easy to revamp pg_trigger definition in
pg12 to add this column and remove the boolean one, so unless there is
some other immediate use for trgparent then there's no point.

Another angle is that we're already in beta3 and there may be concerns
about altering catalog definition this late in the cycle.  Anybody?
Maybe psql can just match tgisinternal triggers by name, and if one
match occurs then we assume it's a clone that should be listed as a
partition trigger.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-19, Amit Langote wrote:

> In CreateTrigger(), 86f575948c7 did this.
> 
> -values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
> +values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
> in_partition);
> 
> I'm not sure why it had to be done, but undoing this change doesn't seem
> to break any regression tests (neither those added by 86f575948c7 nor of
> the partitioned table foreign key commit).  Did we really intend to change
> the meaning of tginternal like this here?

I'm pretty sure pg_dump breaks if you turn that flag off for triggers in
partitions, because pg_dump is going to emit commands to create those
triggers, and those fail.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Mon, Jun 18, 2018 at 10:29 PM, Alvaro Herrera
 wrote:
> On 2018-Jun-18, Ashutosh Bapat wrote:
>
>> That's a wrong comparison. Inheritance based partitioning works even
>> after declarative partitioning feature is added. So, users can
>> continue using inheritance based partitioning if they don't want to
>> move to declarative partitioning. That's not true here. A user creates
>> a BEFORE ROW trigger on each partition that mimics partitioned table
>> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
>> partitioned table will cascade the trigger down to each partition. If
>> that fails to recognize that there is already an equivalent trigger, a
>> partition table will get two triggers doing the same thing silently
>> without any warning or notice. That's fine if the trigger changes the
>> salaries to $50K but not OK if each of those increases salary by 10%.
>> The database has limited ability to recognize what a trigger is doing.
>
> I agree with Robert that nobody in their right minds would be caught by
> this problem by adding new triggers without thinking about it and
> without testing them.  If you add a partitioned-table-level trigger to
> raise salaries by 10% but there was already one in the partition level
> that does the same thing, you'll readily notice that salaries have been
> increased by 21% instead.
>
> So like Robert I'm inclined to not change the wording in the
> documentation.
>
> What does worry me a little bit now, reading this discussion, is whether
> we've made the triggers in partitions visible enough.  We'll have this
> problem once we implement BEFORE ROW triggers as proposed, and I think
> we already have this problem now.  Let's suppose a user creates a
> duplicated after trigger:
>
> create table parent (a int) partition by range (a);
> create table child partition of parent for values from (0) to (100);
> create function noise() returns trigger language plpgsql as $$ begin raise 
> notice 'nyaa'; return null; end; $$;
> create trigger trig_p after insert on parent for each row execute procedure 
> noise();
> create trigger trig_c after insert on child for each row execute procedure 
> noise();
>
> Now let's try it:
>
> alvherre=# insert into child values (1);
> NOTICE:  nyaa
> NOTICE:  nyaa
> INSERT 0 1
>
> OK, so where does that one come from?
>
> alvherre=# \d child
> Tabla «public.child»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
> trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
>
> Hmm, there's only one trigger here, why does it appear twice?  To find
> out, you have to know where to look:
>
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal
> ┼─┼──
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
>
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
>
> alvherre=# \d parent
>Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │
> Partition key: RANGE (a)
> Triggers:
> trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
>
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers

That's a very good description of the problem people will face. Thanks
for elaborating it this way.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Tue, Jun 19, 2018 at 3:51 AM, Robert Treat  wrote:
>
> So +1 for thinking we do need to worry about it. I'm not exactly sure
> how we want to expose that info; with \d+ we list various "Partition
> X:" sections, perhaps adding one for "Partition triggers:" would be
> enough, although I am inclined to think it needs exposure at the \d
> level.

Something like this or what Alvaro proposed will be helpful.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

Hi.

On 2018/06/19 1:59, Alvaro Herrera wrote:
> What does worry me a little bit now, reading this discussion, is whether
> we've made the triggers in partitions visible enough.  We'll have this
> problem once we implement BEFORE ROW triggers as proposed, and I think
> we already have this problem now.  Let's suppose a user creates a
> duplicated after trigger:
> 
> create table parent (a int) partition by range (a);
> create table child partition of parent for values from (0) to (100);
> create function noise() returns trigger language plpgsql as $$ begin raise 
> notice 'nyaa'; return null; end; $$;
> create trigger trig_p after insert on parent for each row execute procedure 
> noise();
> create trigger trig_c after insert on child for each row execute procedure 
> noise();
> 
> Now let's try it:
> 
> alvherre=# insert into child values (1);
> NOTICE:  nyaa
> NOTICE:  nyaa
> INSERT 0 1
> 
> OK, so where does that one come from?
> 
> alvherre=# \d child
> Tabla «public.child»
>  Columna │  Tipo   │ Collation │ Nullable │ Default 
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │ 
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
> trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
> 
> Hmm, there's only one trigger here, why does it appear twice?  To find
> out, you have to know where to look:
> 
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal 
> ┼─┼──
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
> 
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
> 
> alvherre=# \d parent
>Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default 
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │ 
> Partition key: RANGE (a)
> Triggers:
> trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
> 
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers

In CreateTrigger(), 86f575948c7 did this.

-values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
+values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
in_partition);

I'm not sure why it had to be done, but undoing this change doesn't seem
to break any regression tests (neither those added by 86f575948c7 nor of
the partitioned table foreign key commit).  Did we really intend to change
the meaning of tginternal like this here?

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Treat]

On Mon, Jun 18, 2018 at 12:59 PM, Alvaro Herrera
 wrote:
> On 2018-Jun-18, Ashutosh Bapat wrote:
>
>> That's a wrong comparison. Inheritance based partitioning works even
>> after declarative partitioning feature is added. So, users can
>> continue using inheritance based partitioning if they don't want to
>> move to declarative partitioning. That's not true here. A user creates
>> a BEFORE ROW trigger on each partition that mimics partitioned table
>> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
>> partitioned table will cascade the trigger down to each partition. If
>> that fails to recognize that there is already an equivalent trigger, a
>> partition table will get two triggers doing the same thing silently
>> without any warning or notice. That's fine if the trigger changes the
>> salaries to $50K but not OK if each of those increases salary by 10%.
>> The database has limited ability to recognize what a trigger is doing.
>
> I agree with Robert that nobody in their right minds would be caught by
> this problem by adding new triggers without thinking about it and
> without testing them.  If you add a partitioned-table-level trigger to
> raise salaries by 10% but there was already one in the partition level
> that does the same thing, you'll readily notice that salaries have been
> increased by 21% instead.
>
> So like Robert I'm inclined to not change the wording in the
> documentation.
>
> What does worry me a little bit now, reading this discussion, is whether
> we've made the triggers in partitions visible enough.  We'll have this
> problem once we implement BEFORE ROW triggers as proposed, and I think
> we already have this problem now.  Let's suppose a user creates a
> duplicated after trigger:
>
> create table parent (a int) partition by range (a);
> create table child partition of parent for values from (0) to (100);
> create function noise() returns trigger language plpgsql as $$ begin raise 
> notice 'nyaa'; return null; end; $$;
> create trigger trig_p after insert on parent for each row execute procedure 
> noise();
> create trigger trig_c after insert on child for each row execute procedure 
> noise();
>
> Now let's try it:
>
> alvherre=# insert into child values (1);
> NOTICE:  nyaa
> NOTICE:  nyaa
> INSERT 0 1
>
> OK, so where does that one come from?
>
> alvherre=# \d child
> Tabla «public.child»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
> trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
>
> Hmm, there's only one trigger here, why does it appear twice?  To find
> out, you have to know where to look:
>
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal
> ┼─┼──
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
>
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
>
> alvherre=# \d parent
>Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │
> Partition key: RANGE (a)
> Triggers:
> trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
>
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers
>
> Or maybe this is not something to worry about?
>

One of the things I was thinking about while reading this thread is
that the scenario of creating "duplicate" triggers on a table meaning
two triggers doing the same thing is entirely possible now but we
don't really do anything to prevent it, which is Ok. I've never seen
much merit in the argument "people should test" (it assumes a certain
infallibility that just isn't true) but we've also generally been
pretty good about exposing what is going on so people can debug this
type of unexpected behavior.

So +1 for thinking we do need to worry about it. I'm not exactly sure
how we want to expose that info; with \d+ we list various "Partition
X:" sections, perhaps adding one for "Partition triggers:" would be
enough, although I am inclined to think it needs exposure at the \d
level. One other thing to consider is firing order of said triggers...
if all parent level triggers fire before child level triggers then the
above separation is straightforward, but if the execution order is, as
I suspect, mixed, then it becomes more complicated.


Robert Treat
http://xzilla.net

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[David G. Johnston]

On Mon, Jun 18, 2018 at 9:59 AM, Alvaro Herrera 
wrote:

>
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal
> ┼─┼──
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
>
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
>
> alvherre=# \d parent
>Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─┼─┼───┼──┼─
>  a   │ integer │   │  │
> Partition key: RANGE (a)
> Triggers:
> trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
>
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers
>
> Or maybe this is not something to worry about?


For the main internal trigger, foreign key, we don't show the trigger on
the relevant table but we do indicate its effect by showing the presence of
the foreign key.  We likewise need to show the effect of the inherited
trigger on the child table.  When viewing the output the display order of
invocation should be retained (is it that way now?) as a primary goal -
with the directed or inherited nature presentation dependent upon that.
i.e., I would like to see "Parent Triggers:" and "Triggers:" sections if
possible but if trig_c is going to be invoked before trig_p that won't work
and having a single "Triggers:" section with "(parent)" somewhere in the
trigger info printout would be preferred.

David J.

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-18, Ashutosh Bapat wrote:

> That's a wrong comparison. Inheritance based partitioning works even
> after declarative partitioning feature is added. So, users can
> continue using inheritance based partitioning if they don't want to
> move to declarative partitioning. That's not true here. A user creates
> a BEFORE ROW trigger on each partition that mimics partitioned table
> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
> partitioned table will cascade the trigger down to each partition. If
> that fails to recognize that there is already an equivalent trigger, a
> partition table will get two triggers doing the same thing silently
> without any warning or notice. That's fine if the trigger changes the
> salaries to $50K but not OK if each of those increases salary by 10%.
> The database has limited ability to recognize what a trigger is doing.

I agree with Robert that nobody in their right minds would be caught by
this problem by adding new triggers without thinking about it and
without testing them.  If you add a partitioned-table-level trigger to
raise salaries by 10% but there was already one in the partition level
that does the same thing, you'll readily notice that salaries have been
increased by 21% instead.

So like Robert I'm inclined to not change the wording in the
documentation.

What does worry me a little bit now, reading this discussion, is whether
we've made the triggers in partitions visible enough.  We'll have this
problem once we implement BEFORE ROW triggers as proposed, and I think
we already have this problem now.  Let's suppose a user creates a
duplicated after trigger:

create table parent (a int) partition by range (a);
create table child partition of parent for values from (0) to (100);
create function noise() returns trigger language plpgsql as $$ begin raise 
notice 'nyaa'; return null; end; $$;
create trigger trig_p after insert on parent for each row execute procedure 
noise();
create trigger trig_c after insert on child for each row execute procedure 
noise();

Now let's try it:

alvherre=# insert into child values (1);
NOTICE:  nyaa
NOTICE:  nyaa
INSERT 0 1

OK, so where does that one come from?

alvherre=# \d child
Tabla «public.child»
 Columna │  Tipo   │ Collation │ Nullable │ Default 
─┼─┼───┼──┼─
 a   │ integer │   │  │ 
Partition of: parent FOR VALUES FROM (0) TO (100)
Triggers:
trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()

Hmm, there's only one trigger here, why does it appear twice?  To find
out, you have to know where to look:

alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
 tgname │ tgrelid │ tgisinternal 
┼─┼──
 trig_p │ parent  │ f
 trig_p │ child   │ t
 trig_c │ child   │ f
(3 filas)

So there is a trigger in table child, but it's hidden because
tgisinternal.  Of course, you can see it if you look at the parent's
definition:

alvherre=# \d parent
   Tabla «public.parent»
 Columna │  Tipo   │ Collation │ Nullable │ Default 
─┼─┼───┼──┼─
 a   │ integer │   │  │ 
Partition key: RANGE (a)
Triggers:
trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
Number of partitions: 1 (Use \d+ to list them.)

I think it'd be useful to have a list of triggers that have been
inherited from ancestors, or maybe simply a list of internal triggers

Or maybe this is not something to worry about?

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Mon, Jun 18, 2018 at 1:20 AM, Ashutosh Bapat
 wrote:
> That's a wrong comparison. Inheritance based partitioning works even
> after declarative partitioning feature is added. So, users can
> continue using inheritance based partitioning if they don't want to
> move to declarative partitioning. That's not true here. A user creates
> a BEFORE ROW trigger on each partition that mimics partitioned table
> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
> partitioned table will cascade the trigger down to each partition. If
> that fails to recognize that there is already an equivalent trigger, a
> partition table will get two triggers doing the same thing silently
> without any warning or notice. That's fine if the trigger changes the
> salaries to $50K but not OK if each of those increases salary by 10%.
> The database has limited ability to recognize what a trigger is doing.

I don't even know what to say about that.  You are correct that if a
user creates a new trigger on a partitioned table and doesn't check
what happens to any existing triggers that they already have, bad
things might happen.  But that seems like a pretty stupid thing to do.
If you make *any* kind of critical change to your production database
without testing it, bad things may happen.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Sat, Jun 16, 2018 at 9:29 AM, Robert Haas  wrote:
>
> By that logic, we should not have suggested that anyone use table
> inheritance, because they would have to change their configuration
> when we implemented table partitioning.  Indeed, switching from table
> inheritance to table partitioning is much more intrusive than dropping
> triggers on individual partitions and adding one on the partitioned
> table.  The latter only requires DDL on existing objects, but the
> former requires creating entirely new objects and moving all of your
> data.

That's a wrong comparison. Inheritance based partitioning works even
after declarative partitioning feature is added. So, users can
continue using inheritance based partitioning if they don't want to
move to declarative partitioning. That's not true here. A user creates
a BEFORE ROW trigger on each partition that mimics partitioned table
level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
partitioned table will cascade the trigger down to each partition. If
that fails to recognize that there is already an equivalent trigger, a
partition table will get two triggers doing the same thing silently
without any warning or notice. That's fine if the trigger changes the
salaries to $50K but not OK if each of those increases salary by 10%.
The database has limited ability to recognize what a trigger is doing.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Thu, Jun 14, 2018 at 9:45 AM, Ashutosh Bapat
 wrote:
>> It sounds like you want to try to hide from users the fact that they
>> can create triggers on the individual partitions.
>
> No. I never said that in my mails (see [1], [2]) I object to the
> explicit suggestion that they can/should create BEFORE ROW triggers on
> partitions since those are not supported on the partitioned table.
>
>>  I think that's the
>> wrong approach.  First of all, it's not going to work, because people
>> are going to find out about it and do it anyway.  Secondly, the
>> documentation's job is to tell you about the system's capabilities,
>> not conceal them from you.
>
> Neither is documentation's job to "suggest" something that can lead to
> problems in future.

Well, perhaps what this boils down to is that I don't agree that it
can lead to problems in the future.  If the user creates a trigger on
each partition, whether they are all the same or some are different
from others, they can stick with that configuration in future
releases, too.  I don't think we're going to remove the ability to
have separate triggers on each partition; we're just going to add, as
an additional possibility, the ability to have a trigger on the
partitioned table that cascades to the individual partitions.  It's
true that if people want to get the benefit of that feature, they'll
have to change the configuration, but so what?  That's true of many
new features, and I don't think it's right to characterize that as a
problem.

By that logic, we should not have suggested that anyone use table
inheritance, because they would have to change their configuration
when we implemented table partitioning.  Indeed, switching from table
inheritance to table partitioning is much more intrusive than dropping
triggers on individual partitions and adding one on the partitioned
table.  The latter only requires DDL on existing objects, but the
former requires creating entirely new objects and moving all of your
data.

I think that the existing wording is fine and there's really no need
to change anything.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/14 22:45, Ashutosh Bapat wrote:
> On Thu, Jun 14, 2018 at 6:49 PM, Robert Haas  wrote:
>> It sounds like you want to try to hide from users the fact that they
>> can create triggers on the individual partitions.
> 
> No. I never said that in my mails (see [1], [2]) I object to the
> explicit suggestion that they can/should create BEFORE ROW triggers on
> partitions since those are not supported on the partitioned table.

I understand Ashutosh's worry as follows:

A workaround for the limitation that BR triggers cannot be defined on
partitioned tables yet is to define that *same* trigger on all partitions,
which works from the beginning (PG 10), so that gets the job done.  But...
some users may however fail to ensure that the trigger's definition is
exactly alike for each partition, especially they are likely to make each
trigger call a different function, although each of those functions may
have the same body of code.  When in some future release, one is able to
define the trigger on the partitioned table and does so, the trigger will
end up being created again on each partition, because the existing trigger
on each partition (after upgrading) would be different due to a different
function being called in each.

I proposed that we reword the sentence that describes this particular
limitation to warn users about that.  See if the attached patch is any
good for that.

Thanks,
Amit
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 0258391154..781536c44b 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3349,8 +3349,10 @@ ALTER TABLE measurement ATTACH PARTITION 
measurement_y2008m02
 
  
   
-   BEFORE ROW triggers, if necessary, must be defined
-   on individual partitions, not the partitioned table.
+   Creating BEFORE ROW triggers on partitioned tables
+   is not supported.  A workaround is to create such a trigger on 
individual
+   partitions, but make sure that its definition is identical across all
+   partitions, including the function that's called from the trigger.
   
  
 

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Rui DeSousa]

> On Jun 14, 2018, at 9:19 AM, Robert Haas  wrote:
> 
>  anyone who wants a BEFORE trigger has a good reason
> for wanting it.

I have used before triggers to enforce the immutability of a column.

i.e. 

  if (new.member_key != old.member_key) then
raise exception 'Unable to change member_key, column is immutable';
  end if
  ; 

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Thu, Jun 14, 2018 at 6:49 PM, Robert Haas  wrote:
> On Wed, Jun 6, 2018 at 7:51 AM, Ashutosh Bapat
>  wrote:
>> On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera
>>  wrote:
>>
>>>  - BEFORE row triggers are not supported
>>
>> I think this is fine. The existing wording suggests that the user
>> creates the triggers on the partitioned table, and that will be
>> supported always, which can lead to problems. With this description,
>> if the user finds out that BEFORE triggers are supported on partitions
>> and creates those, and we implement something to support those on
>> partitioned table, s/he will have to change the implementation
>> accordingly.
>
> It sounds like you want to try to hide from users the fact that they
> can create triggers on the individual partitions.

No. I never said that in my mails (see [1], [2]) I object to the
explicit suggestion that they can/should create BEFORE ROW triggers on
partitions since those are not supported on the partitioned table.

>  I think that's the
> wrong approach.  First of all, it's not going to work, because people
> are going to find out about it and do it anyway.  Secondly, the
> documentation's job is to tell you about the system's capabilities,
> not conceal them from you.

Neither is documentation's job to "suggest" something that can lead to
problems in future.

[1] 
https://www.postgresql.org/message-id/cafjfprfzcvnul0l3_qafqr5xfn-eynbmow4gf-2moo7gnaz...@mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAFjFpRfzCVnuL0L3_qAFqr5xFN-EynbMoW4gf-2moO7gNazADA%40mail.gmail.com

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Wed, Jun 6, 2018 at 7:51 AM, Ashutosh Bapat
 wrote:
> On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera
>  wrote:
>
>>  - BEFORE row triggers are not supported
>
> I think this is fine. The existing wording suggests that the user
> creates the triggers on the partitioned table, and that will be
> supported always, which can lead to problems. With this description,
> if the user finds out that BEFORE triggers are supported on partitions
> and creates those, and we implement something to support those on
> partitioned table, s/he will have to change the implementation
> accordingly.

It sounds like you want to try to hide from users the fact that they
can create triggers on the individual partitions.  I think that's the
wrong approach.  First of all, it's not going to work, because people
are going to find out about it and do it anyway.  Secondly, the
documentation's job is to tell you about the system's capabilities,
not conceal them from you.  Third, any speculation about what upgrade
problems people might have in the future is just that: speculation.
As the saying goes, it's tough to make predictions, especially about
the future.

To put that another way, if we really think nobody should do this, we
should prohibit it, not leave it out of the documentation.  But I
think prohibiting it would be a terrible idea: it would break upgrades
from existing releases and inconvenience users to no good purpose.
Very few, if any, users say, well, I don't really need a trigger on
this table, but PostgreSQL is really quite a bit faster than I need it
to be, so I think I'll add one anyway just to slow things down.  We
should assume that anyone who wants a BEFORE trigger has a good reason
for wanting it.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Mon, Jun 4, 2018 at 5:40 PM, Tom Lane  wrote:
>> I think, in general, that we should try to pick semantics that make a
>> partitioned table behave like an unpartitioned table, provided that
>> all triggers are defined on the partitioned table itself.
>
> Well, then we lose the property Alvaro wanted, namely that if an
> application chooses to insert directly into a partition, that's
> just an optimization that changes no behavior (as long as it picked
> the right partition).  Maybe this can be dodged by propagating
> parent trigger definitions to the children, but it's going to be
> complicated I'm afraid.

Isn't this basically what I already proposed in
http://postgr.es/m/CA+TgmoYQD1xSM7=xry6rv2a-w43gkpcth76f3nsp5o2sgwe...@mail.gmail.com
?

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Thu, Jun 14, 2018 at 1:54 PM, Amit Langote
 wrote:
> On 2018/06/12 22:22, Ashutosh Bapat wrote:
>> -- create triggers, user may create different trigger functions one
>> for each partition, unless s/he understands that the tables can share
>> trigger functions
>> create function trig_t1p1() returns trigger as $$ begin return new;
>> end;$$ language plpgsql;
>> create trigger trig_t1p1_1 before update on t1p1 execute procedure 
>> trig_t1p1();
>> create trigger trig_t1p1_2 before update on t1p1 execute procedure 
>> trig_t1p1();
>>
>> create function trig_t1p2() returns trigger as $$ begin return new;
>> end;$$ language plpgsql;
>> create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();
>>
>> When this schema gets upgraded to v12 or whatever version supports
>> BEFORE ROW triggers on a partitioned table and the user tries to
>> create a trigger on a partitioned table
>> 1. somehow the code has to detect that there are already triggers on
>> the partitions so no need to create new triggers on the partitions. I
>> don't see how this can be done, unless the user is smart enough to use
>> the same trigger function for all partitions.
>>
>> 2. user has to drop those triggers and trigger functions and create
>> trigger on the partitioned table.
>>
>> May be I am underestimating users but I am sure there will be some
>> users who will end up with scenario.
>
> Hmm, if a user *knowingly* makes triggers on different partitions call
> different functions, then they wouldn't want to use the feature to create
> the trigger on partitioned tables, because that feature implies same
> trigger definition for all partitions.

How many users would do that *knowingly*?

>
> Maybe, just as a reminder to users, we could mention in the docs that it
> is in fact possible to call the same function from different triggers
> (that is, triggers defined on different partitions) and that's what they
> should do if they intend to later use the feature to define that same
> trigger on the partitioned table.

+1 for something like this, but wording is important.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/12 22:22, Ashutosh Bapat wrote:
> -- create triggers, user may create different trigger functions one
> for each partition, unless s/he understands that the tables can share
> trigger functions
> create function trig_t1p1() returns trigger as $$ begin return new;
> end;$$ language plpgsql;
> create trigger trig_t1p1_1 before update on t1p1 execute procedure 
> trig_t1p1();
> create trigger trig_t1p1_2 before update on t1p1 execute procedure 
> trig_t1p1();
> 
> create function trig_t1p2() returns trigger as $$ begin return new;
> end;$$ language plpgsql;
> create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();
> 
> When this schema gets upgraded to v12 or whatever version supports
> BEFORE ROW triggers on a partitioned table and the user tries to
> create a trigger on a partitioned table
> 1. somehow the code has to detect that there are already triggers on
> the partitions so no need to create new triggers on the partitions. I
> don't see how this can be done, unless the user is smart enough to use
> the same trigger function for all partitions.
> 
> 2. user has to drop those triggers and trigger functions and create
> trigger on the partitioned table.
> 
> May be I am underestimating users but I am sure there will be some
> users who will end up with scenario.

Hmm, if a user *knowingly* makes triggers on different partitions call
different functions, then they wouldn't want to use the feature to create
the trigger on partitioned tables, because that feature implies same
trigger definition for all partitions.

Maybe, just as a reminder to users, we could mention in the docs that it
is in fact possible to call the same function from different triggers
(that is, triggers defined on different partitions) and that's what they
should do if they intend to later use the feature to define that same
trigger on the partitioned table.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Sat, Jun 9, 2018 at 3:48 AM, Alvaro Herrera  wrote:
> On 2018-Jun-08, Alvaro Herrera wrote:
>
>> Actually, the column order doesn't matter for a trigger function,
>> because these don't refer to columns by number but by name.  So unless
>> users write trigger functions in C and use hardcoded column numbers
>> (extremely unlikely), I think this is not an issue.  In other words, in
>> the interesting cases the trigger functions are the same for all
>> partitions -- therefore upgrading from separate per-partition triggers
>> to one master trigger in the partitioned table is not going to be that
>> difficult, ISTM.
>
> One last thing.  This wording has been there since pg10; the only thing
> that changed is that it now says "BEFORE ROW triggers" instead of "Row
> triggers".  I don't think leaving it there one more year is going to get
> us too much grief, TBH.

I am worried about following scenario

-- create partitioned table
create table t1 (a int, b int) partition by range(a);

-- create some tables which will be attached to the partitioned table in future
create table t1p1 (like t1);
create table t1p2 (like t1);

-- those tables have indexes
create index t1p1_a on t1p1(a);
create index t1p2_a on t1p2(a);

-- attach partitions
alter table t1 attach partition t1p1 for values from (0) to (100);
alter table t1 attach partition t1p2 for values from (100) to (200);

-- create index on partitioned table, it doesn't create new indexes on
t1p1 and t1p2 since there are already indexes on a
create index t1_a on t1(a);

-- create triggers, user may create different trigger functions one
for each partition, unless s/he understands that the tables can share
trigger functions
create function trig_t1p1() returns trigger as $$ begin return new;
end;$$ language plpgsql;
create trigger trig_t1p1_1 before update on t1p1 execute procedure trig_t1p1();
create trigger trig_t1p1_2 before update on t1p1 execute procedure trig_t1p1();

create function trig_t1p2() returns trigger as $$ begin return new;
end;$$ language plpgsql;
create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();

When this schema gets upgraded to v12 or whatever version supports
BEFORE ROW triggers on a partitioned table and the user tries to
create a trigger on a partitioned table
1. somehow the code has to detect that there are already triggers on
the partitions so no need to create new triggers on the partitions. I
don't see how this can be done, unless the user is smart enough to use
the same trigger function for all partitions.

2. user has to drop those triggers and trigger functions and create
trigger on the partitioned table.

May be I am underestimating users but I am sure there will be some
users who will end up with scenario.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-08, Alvaro Herrera wrote:

> Actually, the column order doesn't matter for a trigger function,
> because these don't refer to columns by number but by name.  So unless
> users write trigger functions in C and use hardcoded column numbers
> (extremely unlikely), I think this is not an issue.  In other words, in
> the interesting cases the trigger functions are the same for all
> partitions -- therefore upgrading from separate per-partition triggers
> to one master trigger in the partitioned table is not going to be that
> difficult, ISTM.

One last thing.  This wording has been there since pg10; the only thing
that changed is that it now says "BEFORE ROW triggers" instead of "Row
triggers".  I don't think leaving it there one more year is going to get
us too much grief, TBH.

I'm going to leave this as an open item for one or two more weeks and
see if there's more support for Ashutosh's PoV; but if not, my proposal
is to close it without changing anything further.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-07, Ashutosh Bapat wrote:

> On Thu, Jun 7, 2018 at 10:58 AM, Amit Langote
>  wrote:

> > I don't understand why you think it's too troublesome to let the users
> > know that there is some way to use BR triggers with partitioning.  We
> > didn't do that for indexes, for example, before PG 11 introduced the
> > ability to create them on partitioned tables.
> 
> By looking at the index keys it's easy to decide whether the two
> indexes are same. When we add an index on a partitioned table in v11,
> we skip creating an index on the partition if there exists an index
> similar to the one being created. So, a user can have indexes on
> partitions created in v10, upgrade to v11 and create an index on the
> partitioned table. Nothing changes. But that's not true for a trigger.
> It's not easy to check whether two triggers are same or not unless the
> underlying function is same. User may or may not be using same trigger
> function for all the partitions, which is more true, if the column
> order differs between partitions. So, if the user has created triggers
> on the partitions in v10, upgrades to v11, s/he may have to drop them
> all and recreate the trigger on the partitioned table.

Actually, the column order doesn't matter for a trigger function,
because these don't refer to columns by number but by name.  So unless
users write trigger functions in C and use hardcoded column numbers
(extremely unlikely), I think this is not an issue.  In other words, in
the interesting cases the trigger functions are the same for all
partitions -- therefore upgrading from separate per-partition triggers
to one master trigger in the partitioned table is not going to be that
difficult, ISTM.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Thu, Jun 7, 2018 at 10:58 AM, Amit Langote
 wrote:
> On 2018/06/07 14:17, Ashutosh Bapat wrote:
>>> that is, users can find out about that feature by themselves by
>>> trying it out?
>>
>> I didn't understand that part.
>>
>> Probably we just say that BEFORE ROW triggers are not supported on a
>> partitioned table. It's good enough not to suggest it ourselves. If
>> users find out that they can create triggers on partitions and use it
>> that way, they may or may not have to change their implementation
>> later when we start supporting those. But then we didn't suggest that
>> they do it that way.
>


> I don't understand why you think it's too troublesome to let the users
> know that there is some way to use BR triggers with partitioning.  We
> didn't do that for indexes, for example, before PG 11 introduced the
> ability to create them on partitioned tables.

By looking at the index keys it's easy to decide whether the two
indexes are same. When we add an index on a partitioned table in v11,
we skip creating an index on the partition if there exists an index
similar to the one being created. So, a user can have indexes on
partitions created in v10, upgrade to v11 and create an index on the
partitioned table. Nothing changes. But that's not true for a trigger.
It's not easy to check whether two triggers are same or not unless the
underlying function is same. User may or may not be using same trigger
function for all the partitions, which is more true, if the column
order differs between partitions. So, if the user has created triggers
on the partitions in v10, upgrades to v11, s/he may have to drop them
all and recreate the trigger on the partitioned table.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/07 14:17, Ashutosh Bapat wrote:
>> that is, users can find out about that feature by themselves by
>> trying it out?
> 
> I didn't understand that part.
> 
> Probably we just say that BEFORE ROW triggers are not supported on a
> partitioned table. It's good enough not to suggest it ourselves. If
> users find out that they can create triggers on partitions and use it
> that way, they may or may not have to change their implementation
> later when we start supporting those. But then we didn't suggest that
> they do it that way.

I don't understand why you think it's too troublesome to let the users
know that there is some way to use BR triggers with partitioning.  We
didn't do that for indexes, for example, before PG 11 introduced the
ability to create them on partitioned tables.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Thu, Jun 7, 2018 at 7:51 AM, Amit Langote
 wrote:
> On 2018/06/06 20:51, Ashutosh Bapat wrote:
>> The existing wording suggests that the user
>> creates the triggers on the partitioned table, and that will be
>> supported always, which can lead to problems.
>
> Do you mean the following wording
>
> "BEFORE ROW triggers, if necessary, must be defined on individual
> partitions, not the partitioned table."
>
> suggests that user *can* create triggers on the partitioned table?  I
> guess I can see how one may read it that way.  How about we make it say
> something like:
>
> "BEFORE ROW triggers are not supported on partitioned tables; a user can
> still define them, if necessary, on individual partitions though."

Whichever way said, I am reading it like "We don't support BEFORE ROW
triggers on partitioned tables, but you can have them by creating
those on partitions". That is going to be a trouble for us.

>
>> With this description,
>> if the user finds out that BEFORE triggers are supported on partitions
>> and creates those, and we implement something to support those on
>> partitioned table, s/he will have to change the implementation
>> accordingly.
>
> So you mean that the existing wording *encourages* users to use the
> feature to create BR triggers on individual partitions whereas it
> shouldn't,

right.

> that is, users can find out about that feature by themselves by
> trying it out?

I didn't understand that part.

Probably we just say that BEFORE ROW triggers are not supported on a
partitioned table. It's good enough not to suggest it ourselves. If
users find out that they can create triggers on partitions and use it
that way, they may or may not have to change their implementation
later when we start supporting those. But then we didn't suggest that
they do it that way.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/06 20:51, Ashutosh Bapat wrote:
> The existing wording suggests that the user
> creates the triggers on the partitioned table, and that will be
> supported always, which can lead to problems.

Do you mean the following wording

"BEFORE ROW triggers, if necessary, must be defined on individual
partitions, not the partitioned table."

suggests that user *can* create triggers on the partitioned table?  I
guess I can see how one may read it that way.  How about we make it say
something like:

"BEFORE ROW triggers are not supported on partitioned tables; a user can
still define them, if necessary, on individual partitions though."

> With this description,
> if the user finds out that BEFORE triggers are supported on partitions
> and creates those, and we implement something to support those on
> partitioned table, s/he will have to change the implementation
> accordingly.

So you mean that the existing wording *encourages* users to use the
feature to create BR triggers on individual partitions whereas it
shouldn't, that is, users can find out about that feature by themselves by
trying it out?  I think the revised wording I proposed above isn't too
encouraging.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera
 wrote:

>  - BEFORE row triggers are not supported

I think this is fine. The existing wording suggests that the user
creates the triggers on the partitioned table, and that will be
supported always, which can lead to problems. With this description,
if the user finds out that BEFORE triggers are supported on partitions
and creates those, and we implement something to support those on
partitioned table, s/he will have to change the implementation
accordingly.

>
> The following caveat applies to partitioned tables:
>  - When an UPDATE causes a row to move ..."
>

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/06 2:08, Alvaro Herrera wrote:
> On 2018-Jun-05, Amit Langote wrote:
> 
>> On 2018/06/05 16:41, Ashutosh Bapat wrote:
>>> On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
>>>  wrote:
 On 2018/06/05 1:25, Alvaro Herrera wrote:
> In the meantime, my inclination is to fix the documentation to point out
> that AFTER triggers are allowed but BEFORE triggers are not.

 Wasn't that already fixed by bcded2609ade6?
> 
> Ah, yes, so it's already OK.
> 
>>> Thought correct that suggestion is problematic for upgrades. Probably
>>> users will create triggers using same function on all the partitions.
>>> After the upgrade when we start supporting BEFORE TRIGGER on
>>> partitioned table, the user will have to drop these triggers and
>>> create one trigger on the partitioned table to have the trigger to be
>>> applicable to the new partitions created.
>>
>> Hmm yes, maybe there is scope for rewording then.
> 
> Reading that subsection in its entirety, I'm not sure how to do better
> -- it's all about restrictions that currently exist and we think we
> could do better in the future, with the exception of the one about an
> UPDATE/DELETE not seeing the updated version after a row migrating to
> another partition.  One idea would be to split it into its own list of
> issues; something like:
> 
> "The following limitations apply, and might be lifted in the future:
>  - no way to create exclusion constraint
>  - foreign keys cannot refer to partitioned tables
>  - BEFORE row triggers are not supported
> 
> The following caveat applies to partitioned tables:
>  - When an UPDATE causes a row to move ..."
> 
> In other words, make a distinction of things we expect to change soon
> (which might be too optimistic), vs. others we don't expect to change.
> 
> Or we could leave it alone; any behavior change would be called out in
> the future version's release notes anyway.  This is what I propose.

That works for me. :)

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-05, Amit Langote wrote:

> On 2018/06/05 16:41, Ashutosh Bapat wrote:
> > On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
> >  wrote:
> >> On 2018/06/05 1:25, Alvaro Herrera wrote:
> >>> In the meantime, my inclination is to fix the documentation to point out
> >>> that AFTER triggers are allowed but BEFORE triggers are not.
> >>
> >> Wasn't that already fixed by bcded2609ade6?

Ah, yes, so it's already OK.

> > Thought correct that suggestion is problematic for upgrades. Probably
> > users will create triggers using same function on all the partitions.
> > After the upgrade when we start supporting BEFORE TRIGGER on
> > partitioned table, the user will have to drop these triggers and
> > create one trigger on the partitioned table to have the trigger to be
> > applicable to the new partitions created.
> 
> Hmm yes, maybe there is scope for rewording then.

Reading that subsection in its entirety, I'm not sure how to do better
-- it's all about restrictions that currently exist and we think we
could do better in the future, with the exception of the one about an
UPDATE/DELETE not seeing the updated version after a row migrating to
another partition.  One idea would be to split it into its own list of
issues; something like:

"The following limitations apply, and might be lifted in the future:
 - no way to create exclusion constraint
 - foreign keys cannot refer to partitioned tables
 - BEFORE row triggers are not supported

The following caveat applies to partitioned tables:
 - When an UPDATE causes a row to move ..."

In other words, make a distinction of things we expect to change soon
(which might be too optimistic), vs. others we don't expect to change.

Or we could leave it alone; any behavior change would be called out in
the future version's release notes anyway.  This is what I propose.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/05 16:41, Ashutosh Bapat wrote:
> On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
>  wrote:
>> On 2018/06/05 1:25, Alvaro Herrera wrote:
>>> In the meantime, my inclination is to fix the documentation to point out
>>> that AFTER triggers are allowed but BEFORE triggers are not.
>>
>> Wasn't that already fixed by bcded2609ade6?
>>
>> We don't say anything about AFTER triggers per se, but the following under
>> the limitations of partitioned tables:
>>
>> BEFORE ROW triggers, if necessary, must be defined on individual
>> partitions, not the partitioned table.
> 
> Thought correct that suggestion is problematic for upgrades. Probably
> users will create triggers using same function on all the partitions.
> After the upgrade when we start supporting BEFORE TRIGGER on
> partitioned table, the user will have to drop these triggers and
> create one trigger on the partitioned table to have the trigger to be
> applicable to the new partitions created.

Hmm yes, maybe there is scope for rewording then.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/06/05 1:25, Alvaro Herrera wrote:
> In the meantime, my inclination is to fix the documentation to point out
> that AFTER triggers are allowed but BEFORE triggers are not.

Wasn't that already fixed by bcded2609ade6?

We don't say anything about AFTER triggers per se, but the following under
the limitations of partitioned tables:

BEFORE ROW triggers, if necessary, must be defined on individual
partitions, not the partitioned table.

Thanks,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
 wrote:
> On 2018/06/05 1:25, Alvaro Herrera wrote:
>> In the meantime, my inclination is to fix the documentation to point out
>> that AFTER triggers are allowed but BEFORE triggers are not.
>
> Wasn't that already fixed by bcded2609ade6?
>
> We don't say anything about AFTER triggers per se, but the following under
> the limitations of partitioned tables:
>
> BEFORE ROW triggers, if necessary, must be defined on individual
> partitions, not the partitioned table.

Thought correct that suggestion is problematic for upgrades. Probably
users will create triggers using same function on all the partitions.
After the upgrade when we start supporting BEFORE TRIGGER on
partitioned table, the user will have to drop these triggers and
create one trigger on the partitioned table to have the trigger to be
applicable to the new partitions created.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[David G. Johnston]

On Mon, Jun 4, 2018 at 2:40 PM, Tom Lane  wrote:

> > I think, in general, that we should try to pick semantics that make a
> > partitioned table behave like an unpartitioned table, provided that
> > all triggers are defined on the partitioned table itself.
>
> Well, then we lose the property Alvaro wanted, namely that if an
> application chooses to insert directly into a partition, that's
> just an optimization that changes no behavior (as long as it picked
> the right partition).  Maybe this can be dodged by propagating
> parent trigger definitions to the children, but it's going to be
> complicated I'm afraid.
>

​Can we give the user the option - adding a before trigger to the
partitioned table forces one to forgo the ability to directly insert into
the partitions?

​David J.

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Tom Lane]

Robert Haas  writes:
> On Mon, Jun 4, 2018 at 4:50 PM, Tom Lane  wrote:
>> Perhaps, but I'm having a hard time wrapping my mind around what the
>> semantics ought to be.  If a trigger on partition A changes the keys
>> so that the row shouldn't have gone into A at all, what then?  That
>> trigger should never have fired, eh?

> Causality is for wimps.  :-)

Heh.

> I think, in general, that we should try to pick semantics that make a
> partitioned table behave like an unpartitioned table, provided that
> all triggers are defined on the partitioned table itself.

Well, then we lose the property Alvaro wanted, namely that if an
application chooses to insert directly into a partition, that's
just an optimization that changes no behavior (as long as it picked
the right partition).  Maybe this can be dodged by propagating
parent trigger definitions to the children, but it's going to be
complicated I'm afraid.

regards, tom lane

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Peter Eisentraut]

On 6/4/18 16:50, Tom Lane wrote:
> Perhaps, but I'm having a hard time wrapping my mind around what the
> semantics ought to be.  If a trigger on partition A changes the keys
> so that the row shouldn't have gone into A at all, what then?  That
> trigger should never have fired, eh?

The insert will result in an error and everything is rolled back, so you
do kind of get the "should never have" behavior.

It seems we ultimately have to answer the question whether we want
BEFORE triggers executed before or after tuple routing.  Both behaviors
might be useful, so perhaps we'll need two kinds of triggers.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane  wrote:
> Could we solve it by saying that triggers on partitioned tables aren't
> allowed to change the partitioning values?  (Or at least, not allowed
> to change them in a way that changes the target partition.)

That seems like a somewhat-unfortunate restriction.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-04, Tom Lane wrote:

> Alvaro Herrera  writes:
> > On 2018-Jun-04, Tom Lane wrote:
> >> ... why doesn't the same problem apply to AFTER triggers that are attached
> >> to the inheritance parent?
> 
> > With a BEFORE trigger, running the trigger might change the target
> > partition, which has the potential for all kinds of trouble.
> 
> Got it.  That seems like not just an implementation restriction, but
> a pretty fundamental issue.
> 
> Could we solve it by saying that triggers on partitioned tables aren't
> allowed to change the partitioning values?  (Or at least, not allowed
> to change them in a way that changes the target partition.)

Yes, that would be one way forward.  In fact, IIUC what happens today if
you remove the restriction (as Amit tested and reported upthread[1]) is
that this already causes an error, because tuple routing is not re-done
after BEFORE triggers are run.

I was thinking it would be good to leave the option open (i.e. forbid
BEFORE triggers altogether) so that in the future we could implement
that case too, and avoid a backwards-incompatible behavior change.
Thinking about it again, this may not be a problem:  if you write a
trigger that works (it doesn't change the target partition) then when
forward-porting it to a version that does allow the target partition to
change, your trigger doesn't change behavior.  So maybe it's okay to
remove the restriction.  I'll test more tomorrow.

[1] https://postgr.es/m/1824bda1-0c47-abc4-8b97-e37414c52...@lab.ntt.co.jp

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Tom Lane]

Alvaro Herrera  writes:
> On 2018-Jun-04, Tom Lane wrote:
>> ... why doesn't the same problem apply to AFTER triggers that are attached
>> to the inheritance parent?

> With a BEFORE trigger, running the trigger might change the target
> partition, which has the potential for all kinds of trouble.

Got it.  That seems like not just an implementation restriction, but
a pretty fundamental issue.

Could we solve it by saying that triggers on partitioned tables aren't
allowed to change the partitioning values?  (Or at least, not allowed
to change them in a way that changes the target partition.)

regards, tom lane

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-04, Tom Lane wrote:

> Alvaro Herrera  writes:
> > This kind of inconsistency is what I wanted to avoid.  One of the
> > guiding principles here was that a partitioned table behaves just like a
> > regular table does; in particular, inserting directly into a partition
> > is an application-level optimization that must behave exactly like it
> > would if the insert had gone into its parent table (unless the user
> > explicitly makes it not so).  If we make an insertion into the partition
> > *not* fire the trigger that would have been fired by inserting into the
> > partitioned table, that's a bug.  I couldn't see a way to have the
> > BEFORE trigger handle that nicely, so I decided to punt on that feature.
> 
> Reasonable, but ...
> 
> > In the meantime, my inclination is to fix the documentation to point out
> > that AFTER triggers are allowed but BEFORE triggers are not.
> 
> ... why doesn't the same problem apply to AFTER triggers that are attached
> to the inheritance parent?

The trigger is not executed as attached to the parent; it's only
executed as attached to the partition itself.  So you create it in the
parent, and clone so that all partitions have it; when you run the
command, only the cloned trigger is run, so in effect the trigger runs
exactly once.  Because the trigger runs *after* the target partition has
been selected, and the trigger cannot change that outcome (ie. it
cannot move the row to another partition) this works fine.

With a BEFORE trigger, running the trigger might change the target
partition, which has the potential for all kinds of trouble.  Also,
there's room to say that the before trigger should run before partition
routing is even invoked (hence the idea of running the triggers in the
partitioned table rather than the partition).  But in that case we must
not run the trigger again when executing the insertion in the chosen
partition; and we must do *something* if the user executes the command
targetting the partition rather than the parent table.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Tom Lane]

Alvaro Herrera  writes:
> This kind of inconsistency is what I wanted to avoid.  One of the
> guiding principles here was that a partitioned table behaves just like a
> regular table does; in particular, inserting directly into a partition
> is an application-level optimization that must behave exactly like it
> would if the insert had gone into its parent table (unless the user
> explicitly makes it not so).  If we make an insertion into the partition
> *not* fire the trigger that would have been fired by inserting into the
> partitioned table, that's a bug.  I couldn't see a way to have the
> BEFORE trigger handle that nicely, so I decided to punt on that feature.

Reasonable, but ...

> In the meantime, my inclination is to fix the documentation to point out
> that AFTER triggers are allowed but BEFORE triggers are not.

... why doesn't the same problem apply to AFTER triggers that are attached
to the inheritance parent?

regards, tom lane

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-May-03, Robert Haas wrote:

> The asymmetry doesn't seem horrible to me on its own merits, but it
> would lead to some odd behavior: suppose you defined a BEFORE ROW
> trigger and an AFTER ROW trigger on the parent, and then inserted one
> row into the parent table and a second row directly into a partition.
> It seems like the BEFORE ROW trigger would fire only for the parent
> insert, but the AFTER ROW trigger would fire in both cases.
> 
> What seems like a better idea is to have the BEFORE ROW trigger get
> cloned to each partition just as we do with AFTER ROW triggers, but
> arrange things so that if it already got fired for the parent table,
> it doesn't get fired again after tuple routing.  This would be a bit
> tricky to get correct in cases where there are multiple levels of
> partitioning involved, but it seems doable.

Hmm.  I'm adding this to the open items list.  I'll study this next
week.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Robert Haas]

On Wed, May 2, 2018 at 9:17 AM, Ashutosh Bapat
 wrote:
> On Wed, May 2, 2018 at 11:56 AM, Amit Langote
>  wrote:
>> But one could very well argue that BEFORE ROW triggers on the
>> parent should run before performing the tuple routing and not be cloned to
>> individual partitions, in which case the result would not have been the
>> same.  Perhaps that's the motivation behind leaving this to be considered
>> later.
>
> +1 for that. We should associate before row triggers with the
> partitioned table and not with the partitions. Those should be
> executed before tuple routing (for that partition level) kicks in. But
> then that would be asymetric with AFTER row trigger behaviour. From
> this POV, pushing AFTER row triggers to the individual partitions
> doesn't look good.

The asymmetry doesn't seem horrible to me on its own merits, but it
would lead to some odd behavior: suppose you defined a BEFORE ROW
trigger and an AFTER ROW trigger on the parent, and then inserted one
row into the parent table and a second row directly into a partition.
It seems like the BEFORE ROW trigger would fire only for the parent
insert, but the AFTER ROW trigger would fire in both cases.

What seems like a better idea is to have the BEFORE ROW trigger get
cloned to each partition just as we do with AFTER ROW triggers, but
arrange things so that if it already got fired for the parent table,
it doesn't get fired again after tuple routing.  This would be a bit
tricky to get correct in cases where there are multiple levels of
partitioning involved, but it seems doable.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Peter Eisentraut]

On 5/2/18 02:26, Amit Langote wrote:
> You're right.  I think that's what you were also saying on the other
> thread, in reply to which I directed you to this thread.  I very clearly
> missed that BEFORE ROW triggers are still disallowed.

fixed

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Wed, May 2, 2018 at 11:56 AM, Amit Langote
 wrote:
> But one could very well argue that BEFORE ROW triggers on the
> parent should run before performing the tuple routing and not be cloned to
> individual partitions, in which case the result would not have been the
> same.  Perhaps that's the motivation behind leaving this to be considered
> later.

+1 for that. We should associate before row triggers with the
partitioned table and not with the partitions. Those should be
executed before tuple routing (for that partition level) kicks in. But
then that would be asymetric with AFTER row trigger behaviour. From
this POV, pushing AFTER row triggers to the individual partitions
doesn't look good.

Other question that comes to my mind is what happens when rows are
inserted into a partition directly. Do we execute BEFORE trigger on
the partitioned table?

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/05/02 14:17, Ashutosh Bapat wrote:
> On Tue, May 1, 2018 at 5:20 PM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> On 4/26/18 05:03, Amit Langote wrote:
>>> On 2018/04/26 13:01, David Rowley wrote:
>>>> The attached small patch removes the mention that partitioned tables
>>>> cannot have foreign keys defined on them.
>>>>
>>>> This has been supported since 3de241db
>>>
>>> I noticed also that the item regarding row triggers might be obsolete as
>>> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
>>> care of that.
>>
>> committed both
> 
> AFAIK we still don't support BEFORE ROW triggers, so removing that
> entry altogether is misleading.

You're right.  I think that's what you were also saying on the other
thread, in reply to which I directed you to this thread.  I very clearly
missed that BEFORE ROW triggers are still disallowed.

create trigger br_insert_trig before insert on p for each row execute
procedure br_insert_trigfunc();
ERROR:  "p" is a partitioned table
DETAIL:  Partitioned tables cannot have BEFORE / FOR EACH ROW triggers.

Here is a patch that adds back a line to state this particular limitation.
Sorry about the earlier misleading patch.

Fwiw, I am a bit surprised to see the error, but that's irrelevant now.  I
see that 86f575948c7 added the following comment in CreateTrigger() above
the check that causes above error.

/*
 * BEFORE triggers FOR EACH ROW are forbidden, because they would
 * allow the user to direct the row to another partition, which
 * isn't implemented in the executor.
 */

But if that's the only reason, I think it might be too restrictive.
Allowing them would not lead to something wrong happening, afaics.  To
illustrate, I temporarily removed the check to allow BR triggers to be
created on the parent and thus being cloned to each partition:

create table p (a int) partition by list (a);
create table p1 partition of p for values in (1);

create or replace function br_insert_trigfunc() returns trigger as
$$ begin new.a := new.a + 1; return new; end $$
language plpgsql;

create trigger br_insert_trig before insert on p for each row execute
procedure br_insert_trigfunc();

insert into p values (1, 'a') returning *;
ERROR:  new row for relation "p1" violates partition constraint
DETAIL:  Failing row contains (2, a).

Since the same error would occur if the trigger were instead defined
directly on the partition (which *is* allowed), maybe users could live
with this.  But one could very well argue that BEFORE ROW triggers on the
parent should run before performing the tuple routing and not be cloned to
individual partitions, in which case the result would not have been the
same.  Perhaps that's the motivation behind leaving this to be considered
later.

Thanks,
Amit
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 0c8eb48a24..0b1948f069 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3340,6 +3340,12 @@ ALTER TABLE measurement ATTACH PARTITION 
measurement_y2008m02
version.
   
  
+ 
+  
+   BEFORE ROW triggers, if necessary, must be defined
+   on individual partitions, not the partitioned table.
+  
+ 
 
 
 

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Tue, May 1, 2018 at 5:20 PM, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> On 4/26/18 05:03, Amit Langote wrote:
>> On 2018/04/26 13:01, David Rowley wrote:
>>> The attached small patch removes the mention that partitioned tables
>>> cannot have foreign keys defined on them.
>>>
>>> This has been supported since 3de241db
>>
>> I noticed also that the item regarding row triggers might be obsolete as
>> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
>> care of that.
>
> committed both

AFAIK we still don't support BEFORE ROW triggers, so removing that
entry altogether is misleading.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/05/01 20:50, Peter Eisentraut wrote:
> On 4/26/18 05:03, Amit Langote wrote:
>> On 2018/04/26 13:01, David Rowley wrote:
>>> The attached small patch removes the mention that partitioned tables
>>> cannot have foreign keys defined on them.
>>>
>>> This has been supported since 3de241db
>>
>> I noticed also that the item regarding row triggers might be obsolete as
>> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
>> care of that.
> 
> committed both

Thank you.

Regards,
Amit

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[David Rowley]

On 1 May 2018 at 23:50, Peter Eisentraut
 wrote:
> committed both

Thanks!

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Peter Eisentraut]

On 4/26/18 05:03, Amit Langote wrote:
> On 2018/04/26 13:01, David Rowley wrote:
>> The attached small patch removes the mention that partitioned tables
>> cannot have foreign keys defined on them.
>>
>> This has been supported since 3de241db
> 
> I noticed also that the item regarding row triggers might be obsolete as
> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
> care of that.

committed both

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[David Rowley]

On 26 April 2018 at 21:03, Amit Langote  wrote:
> I noticed also that the item regarding row triggers might be obsolete as
> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
> care of that.

Thanks. I walked right past that one.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Amit Langote]

On 2018/04/26 13:01, David Rowley wrote:
> The attached small patch removes the mention that partitioned tables
> cannot have foreign keys defined on them.
> 
> This has been supported since 3de241db

I noticed also that the item regarding row triggers might be obsolete as
of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
care of that.

Thanks,
Amit
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 89735b4804..c2e4ec9ab9 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3315,8 +3315,7 @@ ALTER TABLE measurement ATTACH PARTITION 
measurement_y2008m02
  
   
While primary keys are supported on partitioned tables, foreign
-   keys referencing partitioned tables are not supported, nor are foreign
-   key references from a partitioned table to some other table.
+   keys referencing partitioned tables are not supported.
   
  
 
@@ -3340,13 +3339,6 @@ ALTER TABLE measurement ATTACH PARTITION 
measurement_y2008m02
version.
   
  
-
- 
-  
-   Row triggers, if necessary, must be defined on individual partitions,
-   not the partitioned table.
-  
- 
 
 
 

Remove mention in docs that foreign keys on partitioned tables are not supported

[David Rowley]

The attached small patch removes the mention that partitioned tables
cannot have foreign keys defined on them.

This has been supported since 3de241db

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


foreign_keys_on_partitioned_tables_docfix.patch
Description: Binary data

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Alvaro Herrera wrote:
> After wasting some time trying to resolve
> "minor last minute issues", I decided to reduce the scope for now: in
> the current patch, it's allowed to have foreign keys in partitioned
> tables, but it is not possible to have foreign keys that point to
> partitioned tables.  I have split out some preliminary changes that
> intended to support FKs referencing partitioned tables; I intend to
> propose that for early v12, to avoid spending any more time this
> commitfest on that.

Hello,

I won't be able to work on foreign keys pointing to partitioned tables
for the next commitfest, so if somebody is interested in seeing it
supported, I applaud they working on it and I offer a bit of time to
discuss it, if they're so inclined:

CREATE TABLE pktab (a int PRIMARY KEY) PARTITION BY RANGE (a);
... create some partitions ...

CREATE TABLE fktab (a int REFERENCES pktab);


Thanks,

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

Re: pgsql: Foreign keys on partitioned tables

[Robert Haas]

On Fri, Apr 6, 2018 at 4:49 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
> Robert Haas wrote:
>> On Wed, Apr 4, 2018 at 1:03 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org> 
>> wrote:
>> > Foreign keys on partitioned tables
>> >
>> > Author: Álvaro Herrera
>> > Discussion: 
>> > https://postgr.es/m/20171231194359.cvojcour423ulha4@alvherre.pgsql
>> > Reviewed-by: Peter Eisentraut
>>
>> The commit message here was so brief that I had to read the
>> documentation to figure out exactly what this feature was.
>
> I wrote three draft commit messages, and they all seemed to be saying
> something so obvious (just repeating the commit title) that I decided
> not to repeat myself.  Evidently that was a mistake.

Mainly I couldn't tell which direction(s) had been permitted without
looking within.

Not a huge deal, just mentioning it.

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

Re: pgsql: Foreign keys on partitioned tables

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Apr 4, 2018 at 1:03 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org> 
> wrote:
> > Foreign keys on partitioned tables
> >
> > Author: Álvaro Herrera
> > Discussion: 
> > https://postgr.es/m/20171231194359.cvojcour423ulha4@alvherre.pgsql
> > Reviewed-by: Peter Eisentraut
> 
> The commit message here was so brief that I had to read the
> documentation to figure out exactly what this feature was.

I wrote three draft commit messages, and they all seemed to be saying
something so obvious (just repeating the commit title) that I decided
not to repeat myself.  Evidently that was a mistake.

> In so doing, I ran across this, which seems to need some cleanup:
> 
> +  Also, while it's possible to define PRIMARY KEY
> +  constraints on partitioned tables, it is not supported to create 
> foreign
> +  keys cannot that reference them.  This restriction will be lifted in a
> +  future release.
> 
> Generally, I think we're better off not committing to doing things in
> a future release because we never really know what will happen in the
> future,

True.  I removed that sentence, leaving a "yet" that hints to the future
without making (I hope) too much of a promise.

> but the biggest problem here is that "it is not supported to
> create foreign keys cannot that reference them" doesn't make any
> sense.  I think you mean something like "creating foreign keys that
> reference a partitioned table is not supported".

Yeah, I edited this a few times and evidently one word from some
previous iteration ("cannot") escaped deletion.  I liked your wording so
I used it.

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

Re: pgsql: Foreign keys on partitioned tables

[Robert Haas]

On Wed, Apr 4, 2018 at 1:03 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
> Foreign keys on partitioned tables
>
> Author: Álvaro Herrera
> Discussion: https://postgr.es/m/20171231194359.cvojcour423ulha4@alvherre.pgsql
> Reviewed-by: Peter Eisentraut

The commit message here was so brief that I had to read the
documentation to figure out exactly what this feature was.  In so
doing, I ran across this, which seems to need some cleanup:

+  Also, while it's possible to define PRIMARY KEY
+  constraints on partitioned tables, it is not supported to create foreign
+  keys cannot that reference them.  This restriction will be lifted in a
+  future release.

Generally, I think we're better off not committing to doing things in
a future release because we never really know what will happen in the
future, but the biggest problem here is that "it is not supported to
create foreign keys cannot that reference them" doesn't make any
sense.  I think you mean something like "creating foreign keys that
reference a partitioned table is not supported".

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

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Robert Haas wrote:

> I suspect that this leads to bugs under concurrency, something to do
> with crosscheck_snapshot, but I couldn't say exactly what the problem
> is off the top of my head.   My hope is that partitioning might be
> immune on the strength of knowing that any given tuple could only be
> present in one particular partition, but that might be wishful
> thinking.

Speaking of crosscheck_snapshot, I just noticed that the case of FKs
with repeatable read or serializable snapshot seems not to be covered by
tests at all, judging from the coverage report:

2635 : /*
2636 :  * In READ COMMITTED mode, we just need to use an 
up-to-date regular
2637 :  * snapshot, and we will see all rows that could be 
interesting. But in
2638 :  * transaction-snapshot mode, we can't change the 
transaction snapshot. If
2639 :  * the caller passes detectNewRows == false then 
it's okay to do the query
2640 :  * with the transaction snapshot; otherwise we use a 
current snapshot, and
2641 :  * tell the executor to error out if it finds any 
rows under the current
2642 :  * snapshot that wouldn't be visible per the 
transaction snapshot.  Note
2643 :  * that SPI_execute_snapshot will register the 
snapshots, so we don't need
2644 :  * to bother here.
2645 :  */
26463026 : if (IsolationUsesXactSnapshot() && detectNewRows)
2647 : {
2648   0 : CommandCounterIncrement();  /* be sure all my 
own work is visible */
2649   0 : test_snapshot = GetLatestSnapshot();
2650   0 : crosscheck_snapshot = GetTransactionSnapshot();
2651 : }
2652 : else
2653 : {
2654 : /* the default SPI behavior is okay */
26553026 : test_snapshot = InvalidSnapshot;
26563026 : crosscheck_snapshot = InvalidSnapshot;
2657 : }
https://coverage.postgresql.org/src/backend/utils/adt/ri_triggers.c.gcov.html

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

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Thanks, pushed.
> 
> This has broken the selinux regression tests, evidently because it
> removed ONLY from the emitted FK test queries.  While we could change
> the expected results, I would first like to hear a defense of why that
> change is a good idea.  It seems highly likely to be the wrong thing
> for non-partitioned cases.

Yeah, there ain't one, because this was a reversal mistake.  I restored
that ONLY.  (There were two ONLYs in the original query; I initially
removed both, and then went over the file and included them
conditionally on the table not being a partitioned one, based on review
comments.  In this line I restored one conditionally but failed to
realize I should have been restoring the other unconditionally.)

Pushed a fix blind.  Let's see if it appeases rhinoceros.

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

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 4/3/18 15:11, Alvaro Herrera wrote:
> > 0003 is the main patch, which is a bit changed from v4, notably to cover
> > your review comments:
> 
> Looks good now.

Thanks, pushed.

I added a couple of test cases for ON UPDATE/DELETE and MATCH PARTIAL,
after noticing that ri_triggers.c could use some additional coverage
after deleting the parts of it that did not correspond to partitioned
tables.  I think it is possible to keep adding tests, if someone really
wanted to.

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

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Robert Haas wrote:
> On Sun, Dec 31, 2017 at 2:43 PM, Alvaro Herrera
>  wrote:
> > This patch removes all the ONLY markers from queries in ri_triggers.c.
> > That makes the queries work for the new use case, but I haven't figured
> > if it breaks things for other use cases.  I suppose not, since regular
> > inheritance isn't supposed to allow foreign keys in the first place, but
> > I haven't dug any further.
> 
> I suspect that this leads to bugs under concurrency, something to do
> with crosscheck_snapshot, but I couldn't say exactly what the problem
> is off the top of my head.   My hope is that partitioning might be
> immune on the strength of knowing that any given tuple could only be
> present in one particular partition, but that might be wishful
> thinking.

I think you're thinking of this problem: if I insert a row in
partitioned table F, and simultaneously remove the referenced row from
table P, it is possible that we fail to reject the insertion in some
corner-case scenario.  I suppose it's not completely far-fetched, if P
is partitioned.  I don't see any way in which it could be a problem if
only F is partitioned.

For the record: in the patch I'm about to push, I did not implement
foreign key references to partitioned tables.  So it should be safe.
More thought may be needed to implement the other direction.  Offhand, I
don't see a problem, but I may well be wrong.

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

Re: Foreign keys and partitioned tables

[Peter Eisentraut]

On 4/3/18 15:11, Alvaro Herrera wrote:
> 0003 is the main patch, which is a bit changed from v4, notably to cover
> your review comments:

Looks good now.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Peter Eisentraut wrote:

> > 0002 is a fixup for a bug in the row triggers patch: I had a restriction
> > earlier that triggers declared internal were not cloned, and I seem to
> > have lost it in rebase.  Reinstate it.
> 
> Hmm, doesn't cause any test changes?

Here's a test case:

create table t (a int) partition by range (a);
create table t1 partition of t for values from (0) to (1000);
alter table t add constraint uniq unique (a) deferrable;
create table t2 partition of t for values from (1000) to (2000);
create table t3 partition of t for values from (2000) to (3000) partition by 
range (a);
create table t33 partition of t3 for values from (2000) to (2100);

Tables t and t1 have one trigger; tables t2 and t3 have two triggers;
table t33 has three triggers:

alvherre=# select tgrelid::regclass, count(*) from pg_trigger where 
tgrelid::regclass in ('t', 't1', 't2', 't3', 't33') group by tgrelid;
 tgrelid │ count 
─┼───
 t   │ 1
 t1  │ 1
 t2  │ 2
 t3  │ 2
 t33 │ 3
(5 filas)

These triggers probably all do the same thing, so there is no
correctness issue -- only speed.  I suppose it's not impossible to
construct a case that shows some actual breakage -- I just don't know
how.

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

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

Alvaro Herrera wrote:
> While adding some more tests for the "action" part (i.e. updates and
> deletes on the referenced table) I came across a bug that was causing
> the server to crash ... but it's actually a preexisting bug in an
> assert.  The fix is in 0001.

Yeah, it's a live bug that only manifests on Assert-enabled builds.
Here's an example:

create table pk (a int, b int, c int, d int primary key);
create table fk (d int references pk);
insert into pk values (1, 2, 3, 4);
insert into fk values (4);
delete from pk;

Will fix

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

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

While adding some more tests for the "action" part (i.e. updates and
deletes on the referenced table) I came across a bug that was causing
the server to crash ... but it's actually a preexisting bug in an
assert.  The fix is in 0001.

0002 I already posted: don't clone row triggers that are declared
internal.  As you noted, there is no test that changes because of this.
I haven't tried yet; the only case that comes to mind is doing something
with a deferred unique constraint.  Anyway, it was clear to me from the
beginning that cloning internal triggers was not going to work for the
FK constraints.

0003 is the main patch, which is a bit changed from v4, notably to cover
your review comments:

Peter Eisentraut wrote:

> > -  tables and permanent tables.
> > +  tables and permanent tables.  Also note that while it is permitted to
> > +  define a foreign key on a partitioned table, declaring a foreign key
> > +  that references a partitioned table is not allowed.
> >   
> 
> Maybe use "possible" or "supported" instead of "allowed" and "permitted"
> in this and similar cases.

Fixed.

> @@ -7226,12 +7235,23 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab,
> Relation rel,
>  * numbers)
>  */
> if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +   {
> +   /* fix recursion in ATExecValidateConstraint to enable this case */
> +   if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
> +   ereport(ERROR,
> +   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +errmsg("cannot add NOT VALID foreign key to
> relation \"%s\"",
> +   RelationGetRelationName(pkrel;
> +   }
> 
> Maybe this error message should be more along the lines of "is not
> supported yet".

I added errdetail("This feature is not yet supported on partitioned tables.")))

> Also, this restriction should perhaps be mentioned in
> the documentation additions that we have been discussing.

Added a note in ALTER TABLE.

> 
> The first few hunks in ri_triggers.c (the ones that refer to the
> pktable) are apparently for the postponed part of the feature, foreign
> keys referencing partitioned tables.  So I think those hunks should be
> dropped from this patch.

Yeah, reverted.

> The removal of the ONLY for the foreign key queries also affects
> inherited tables, in undesirable ways.  For example, consider this
> script: [...]

> With the patch, this will have deleted the (111, 1) record in test2a,
> which it did not do before.
> 
> I think the trigger functions need to look up whether the table is a
> partitioned table and decide whether the use ONLY based on that.
> (This will probably also apply to the PK side, when that is
> implemented.)

Updated this.  I added you test script to inherit.sql.

> 
> In pg_dump, maybe this can be refined:
> 
> +   /*
> +* For partitioned tables, it doesn't work to emit constraints
> as not
> +* inherited.
> +*/
> +   if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
> +   only = "";
> +   else
> +   only = "ONLY ";
> 
> We use ONLY for foreign keys on inherited tables, but foreign keys are
> not inherited anyway, so this is at best future-proofing.  We could
> just drop the ONLY unconditionally.  Or at least explain this more.

Yeah, I admit this is a bit weird.  I expanded the comment but didn't
change the code:

/*
 * Foreign keys on partitioned tables are always declared as 
inheriting
 * to partitions; for all other cases, emit them as applying 
ONLY
 * directly to the named table, because that's how they work for
 * regular inherited tables.
 */

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From d5313eea2e0196631d3269f453eb3bad86e5eca5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 3 Apr 2018 13:58:49 -0300
Subject: [PATCH v5 1/3] Ancient bug fix

---
 src/backend/utils/adt/ri_triggers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/ri_triggers.c 
b/src/backend/utils/adt/ri_triggers.c
index 3bb708f863..d0fe65cea9 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -514,7 +514,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
boolresult;
 
/* Only called for non-null rows */
-   Assert(ri_NullCheck(RelationGetDescr(fk_rel), old_row, riinfo, true) == 
RI_KEYS_NONE_NULL);

Re: Foreign keys and partitioned tables

[Peter Eisentraut]

Comments on the code:

@@ -7226,12 +7235,23 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab,
Relation rel,
 * numbers)
 */
if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+   {
+   /* fix recursion in ATExecValidateConstraint to enable this case */
+   if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
+   ereport(ERROR,
+   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("cannot add NOT VALID foreign key to
relation \"%s\"",
+   RelationGetRelationName(pkrel;
+   }

Maybe this error message should be more along the lines of "is not
supported yet".  Also, this restriction should perhaps be mentioned in
the documentation additions that we have been discussing.


The first few hunks in ri_triggers.c (the ones that refer to the
pktable) are apparently for the postponed part of the feature, foreign
keys referencing partitioned tables.  So I think those hunks should be
dropped from this patch.

The removal of the ONLY for the foreign key queries also affects
inherited tables, in undesirable ways.  For example, consider this
script:

create table test1 (a int primary key);
insert into test1 values (1), (2), (3);

create table test2 (x int primary key, y int references test1 on delete
cascade);
insert into test2 values (11, 1), (22, 2), (33, 3);

create table test2a () inherits (test2);
insert into test2a values (111, 1), (222, 2);

delete from test1 where a = 1;

select * from test1 order by 1;
select * from test2 order by 1, 2;

With the patch, this will have deleted the (111, 1) record in test2a,
which it did not do before.

I think the trigger functions need to look up whether the table is a
partitioned table and decide whether the use ONLY based on that.
(This will probably also apply to the PK side, when that is
implemented.)


In pg_dump, maybe this can be refined:

+   /*
+* For partitioned tables, it doesn't work to emit constraints
as not
+* inherited.
+*/
+   if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+   only = "";
+   else
+   only = "ONLY ";

We use ONLY for foreign keys on inherited tables, but foreign keys are
not inherited anyway, so this is at best future-proofing.  We could
just drop the ONLY unconditionally.  Or at least explain this more.


Other than that, it looks OK.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Foreign keys and partitioned tables

[Peter Eisentraut]

On 3/31/18 18:21, Alvaro Herrera wrote:
> Yeah, I started by putting what I thought was going to be just ALTER
> TABLE in that test, then moved to the other file and added what I
> thought were more complete tests there and failed to move stuff to
> alter_table.  Honestly, I think these should mostly all belong in
> foreign_key,

right

>   
> -  Partitioned tables do not support EXCLUDE or
> -  FOREIGN KEY constraints; however, you can define
> -  these constraints on individual partitions.
> +  Partitioned tables do not support EXCLUDE 
> constraints;
> +  however, you can define these constraints on individual partitions.
> +  Also, while it's possible to define PRIMARY KEY
> +  constraints on partitioned tables, it is not supported to create 
> foreign
> +  keys cannot that reference them.  This restriction will be lifted in a

This doesn't read correctly.

> +  future release.
>   

> -  tables and permanent tables.
> +  tables and permanent tables.  Also note that while it is permitted to
> +  define a foreign key on a partitioned table, declaring a foreign key
> +  that references a partitioned table is not allowed.
>   

Maybe use "possible" or "supported" instead of "allowed" and "permitted"
in this and similar cases.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services