Thread: Partition Check not updated when insert into a partition
Hi, When directly INSERT INTO partition, postgres will invoke ExecPartitionCheck which will execute its parent's and grandparent's partition constraint check. From the code, the whole constraint check is saved in relcache::rd_partcheck. For a multi-level partition, for example: table 'A' is partition of table 'B', and 'B' is also partition of table 'C'. After I 'ALTER TABLE C DETACH B', I thought partition constraint check of table 'C' does not matter anymore if INSERT INTO table 'A'. But it looks like the relcache of 'A' is not invalidated after detaching 'B'. And the relcache::rd_partcheck still include the partition constraint of table 'C'. Note If I invalidate the table 'A''s relcache manually, then next time the relcache::rd_partcheck will be updated to the expected one which does not include partition constraint check of table 'C'. (ATTACH partition has the same behaviour that relcache::rd_partcheck will not be updated immediately) Does it work as expected ? I didn't find some explanation from the doc. (sorry if I missed something). Best regards, houzj
On Wednesday, June 23, 2021 12:16 PM I wrote: > When directly INSERT INTO partition, postgres will invoke ExecPartitionCheck > which will execute its parent's and grandparent's partition constraint check. > From the code, the whole constraint check is saved in relcache::rd_partcheck. > > For a multi-level partition, for example: table 'A' is partition of table 'B', and 'B' > is also partition of table 'C'. After I 'ALTER TABLE C DETACH B', I thought > partition constraint check of table 'C' does not matter anymore if INSERT INTO > table 'A'. But it looks like the relcache of 'A' is not invalidated after detaching 'B'. > And the relcache::rd_partcheck still include the partition constraint of table 'C'. > Note If I invalidate the table 'A''s relcache manually, then next time the > relcache::rd_partcheck will be updated to the expected one which does not > include partition constraint check of table 'C'. > (ATTACH partition has the same behaviour that relcache::rd_partcheck will not > be updated immediately) An DETACH PARTITION example which shows the relcache::rd_partcheck is not invalidated immediately is: ----- parttable1 -> parttable2-> parttable3 create table parttable1 (a int, b int, c int) partition by list(a); create table parttable2 (a int, b int, c int) partition by list(b); create table parttable3 (a int, b int, c int); alter table parttable1 attach partition parttable2 for values in (1); alter table parttable2 attach partition parttable3 for values in (1); ----- -----INSERT a tuple into parttable3 which does not satisfy parttable1's partition constraint -----we will get an error ----- insert into parttable3 values(2,1,1); ERROR: new row for relation "parttable3" violates partition constraint DETAIL: Failing row contains (2, 1, 1). ----- ----- parttable1 is no longer the grandparent of parttable3. ----- I thought the partition constraint of parttable1 does not matter anymore ----- alter table parttable1 detach partition parttable2; ----- -----INSERT a tuple into parttable3 which does not satisfy parttable1's partition constraint ----- *** I expect a successful insertion, but it returns an error again. *** ----- insert into parttable3 values(2,1,1); ERROR: new row for relation "parttable3" violates partition constraint DETAIL: Failing row contains (2, 1, 1). RECONNECT ----- -----Reconnect the postgres which will invalidate the relcache ----- INSERT a tuple into parttable3 which does not satisfy parttable1's partition constraint ----- We succeeded this time as expected. ----- insert into parttable3 values(2,1,1); INSERT 0 1 Best regards, houzj
On 2021-Jun-23, houzj.fnst@fujitsu.com wrote:
> For a multi-level partition, for example: table 'A' is partition of table
> 'B', and 'B' is also partition of table 'C'. After I 'ALTER TABLE C DETACH B',
> I thought partition constraint check of table 'C' does not matter anymore if
> INSERT INTO table 'A'. But it looks like the relcache of 'A' is not invalidated
> after detaching 'B'. And the relcache::rd_partcheck still include the partition
> constraint of table 'C'. Note If I invalidate the table 'A''s relcache manually,
> then next time the relcache::rd_partcheck will be updated to the expected
> one which does not include partition constraint check of table 'C'.
Hmm, if I understand correctly, this means that we need to invalidate
relcache for all partitions of the partition being detached. Maybe like
in the attached WIP ("XXX VERY CRUDE XXX DANGER EATS DATA") patch, which
solves what you complained about, but I didn't run any other tests.
(Also, in the concurrent case I think this should be done during the
first transaction, so this patch is wrong for it.)
Did you have a misbehaving test for the ATTACH case?
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)
Attachment
On Tuesday, July 13, 2021 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2021-Jun-23, houzj.fnst@fujitsu.com wrote:
>
> > For a multi-level partition, for example: table 'A' is partition of
> > table 'B', and 'B' is also partition of table 'C'. After I 'ALTER
> > TABLE C DETACH B', I thought partition constraint check of table 'C'
> > does not matter anymore if INSERT INTO table 'A'. But it looks like
> > the relcache of 'A' is not invalidated after detaching 'B'. And the
> > relcache::rd_partcheck still include the partition constraint of table
> > 'C'. Note If I invalidate the table 'A''s relcache manually, then next
> > time the relcache::rd_partcheck will be updated to the expected one which
> does not include partition constraint check of table 'C'.
>
> Hmm, if I understand correctly, this means that we need to invalidate relcache
> for all partitions of the partition being detached. Maybe like in the attached
> WIP ("XXX VERY CRUDE XXX DANGER EATS DATA") patch, which solves what
> you complained about, but I didn't run any other tests.
> (Also, in the concurrent case I think this should be done during the first
> transaction, so this patch is wrong for it.)
>
> Did you have a misbehaving test for the ATTACH case?
Thanks for the response.
Yes, I think the following example of ATTACH doesn't work as expected.
---------------------------------------------------------------
create table parttable1 (a int, b int, c int) partition by list(a);
create table parttable2 (a int, b int, c int) partition by list(b);
create table parttable3 (a int, b int, c int);
alter table parttable2 attach partition parttable3 for values in (1);
-----
----- INSERT a tuple into parttable3
----- Cache the partitioncheck in relcache::rd_partcheck
-----
insert into parttable3 values(1, 1, 0);
----- Attach a new top parent
alter table parttable1 attach partition parttable2 for values in (1);
-----
----- INSERT a tuple which doesn't satisfy the new top parent(parttable1)'s partitioncheck
----- But the INSERT will succeed which looks not as expected.
-----
insert into parttable3 values(999, 1, 0);
-----
----- And when I reconnect to clean the cache
----- INSERT a tuple which doesn't satisfy the new top parent(parttable1)'s partitioncheck
----- INSERT will fail due to partition check violation.
-----
insert into parttable3 values(999, 1, 0);
Best regards,
Hou zhijie
Sorry that I missed this thread.
On Wed, Jul 14, 2021 at 11:16 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
> On Tuesday, July 13, 2021 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2021-Jun-23, houzj.fnst@fujitsu.com wrote:
> >
> > > For a multi-level partition, for example: table 'A' is partition of
> > > table 'B', and 'B' is also partition of table 'C'. After I 'ALTER
> > > TABLE C DETACH B', I thought partition constraint check of table 'C'
> > > does not matter anymore if INSERT INTO table 'A'. But it looks like
> > > the relcache of 'A' is not invalidated after detaching 'B'. And the
> > > relcache::rd_partcheck still include the partition constraint of table
> > > 'C'. Note If I invalidate the table 'A''s relcache manually, then next
> > > time the relcache::rd_partcheck will be updated to the expected one which
> > does not include partition constraint check of table 'C'.
> >
> > Hmm, if I understand correctly, this means that we need to invalidate relcache
> > for all partitions of the partition being detached. Maybe like in the attached
> > WIP ("XXX VERY CRUDE XXX DANGER EATS DATA") patch, which solves what
> > you complained about, but I didn't run any other tests.
> > (Also, in the concurrent case I think this should be done during the first
> > transaction, so this patch is wrong for it.)
> >
> > Did you have a misbehaving test for the ATTACH case?
>
> Thanks for the response.
Thank you both.
> Yes, I think the following example of ATTACH doesn't work as expected.
Yeah, need the fix for the ATTACH case too.
Couple more things:
* We must invalidate not just the "direct" partitions of the table
being attached/detached, but also any indirect ones, because all of
their partition constraints would need to contain (or no longer
contain) the root parent's partition constraint.
* I think we should lock the partitions before sending the
invalidation. The ATTACH code already locks the descendents for a
different purpose, but DETACH doesn't, so the latter needs to be fixed
to match.
I've updated Alvaro's patch to address these points. Maybe, we should
also add these cases to the regression and isolation suites?
--
Amit Langote
EDB: http://www.enterprisedb.com
Attachment
On Thu, Aug 5, 2021 at 11:32 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Wed, Jul 14, 2021 at 11:16 AM houzj.fnst@fujitsu.com > > On Tuesday, July 13, 2021 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > Did you have a misbehaving test for the ATTACH case? > > > > Thanks for the response. > > Thank you both. > > > Yes, I think the following example of ATTACH doesn't work as expected. > > Yeah, need the fix for the ATTACH case too. > > Couple more things: > > * We must invalidate not just the "direct" partitions of the table > being attached/detached, but also any indirect ones, because all of > their partition constraints would need to contain (or no longer > contain) the root parent's partition constraint. > > * I think we should lock the partitions before sending the > invalidation. The ATTACH code already locks the descendents for a > different purpose, but DETACH doesn't, so the latter needs to be fixed > to match. > > I've updated Alvaro's patch to address these points. Maybe, we should > also add these cases to the regression and isolation suites? Apparently, I had posted a version of the patch that didn't even compile. I have fixed that in the attached and also added regression tests. Adding this to the next CF. -- Amit Langote EDB: http://www.enterprisedb.com
Attachment
I have reviewed the patch and it looks good to me. However I have one comment.
+ foreach(l, attachrel_children)
+ {
+ Oid partOid = lfirst_oid(l);
+
+ CacheInvalidateRelcacheByRelid(partOid);
+ }
Can we avoid using the extra variable 'partOid' and directly pass
'lfirst_oid(l)' to CacheInvalidateRelcacheByRelid().
Thanks & Regards,
Nitin Jadhav
On Fri, Aug 27, 2021 at 2:50 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Aug 5, 2021 at 11:32 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Jul 14, 2021 at 11:16 AM houzj.fnst@fujitsu.com
> > > On Tuesday, July 13, 2021 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > > Did you have a misbehaving test for the ATTACH case?
> > >
> > > Thanks for the response.
> >
> > Thank you both.
> >
> > > Yes, I think the following example of ATTACH doesn't work as expected.
> >
> > Yeah, need the fix for the ATTACH case too.
> >
> > Couple more things:
> >
> > * We must invalidate not just the "direct" partitions of the table
> > being attached/detached, but also any indirect ones, because all of
> > their partition constraints would need to contain (or no longer
> > contain) the root parent's partition constraint.
> >
> > * I think we should lock the partitions before sending the
> > invalidation. The ATTACH code already locks the descendents for a
> > different purpose, but DETACH doesn't, so the latter needs to be fixed
> > to match.
> >
> > I've updated Alvaro's patch to address these points. Maybe, we should
> > also add these cases to the regression and isolation suites?
>
> Apparently, I had posted a version of the patch that didn't even compile.
>
> I have fixed that in the attached and also added regression tests.
>
> Adding this to the next CF.
>
> --
> Amit Langote
> EDB: http://www.enterprisedb.com
Hi, hackers!
-- We've reviewed patch v3 and found it right. Completely agree that in case we attach/detach partition relcaches for all child relations (if they exist) need invalidation.
Installcheck world succeeds after the patch. Tests from the patch fail as they should when run on the master branch. Found no problems.
Overall patch is good and I'd recommend it to be committed.
We've made v4 patch according to Nitin's advice and tested it, but still have no objections to patch v3. Each can be committed, equally good.
Big thanks to you, Álvaro and Amit for working on this!
Attachment
On Wed, Oct 6, 2021 at 10:40 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> Hi, hackers!
>
> We've reviewed patch v3 and found it right. Completely agree that in case we attach/detach partition relcaches for
allchild relations (if they exist) need invalidation.
> Installcheck world succeeds after the patch. Tests from the patch fail as they should when run on the master branch.
Foundno problems.
>
> Overall patch is good and I'd recommend it to be committed.
>
> We've made v4 patch according to Nitin's advice and tested it, but still have no objections to patch v3. Each can be
committed,equally good.
Thanks Pavel, Nitin for your reviews.
I was looking again at the following hunk in the patch and started
wondering if the lockmode for the children in
DetachPartitionFinalize() shouldn't be the same as used for the parent
mentioned in the DETACH PARTITION command:
@@ -18150,6 +18168,26 @@ DetachPartitionFinalize(Relation rel,
Relation partRel, bool concurrent,
* included in its partition descriptor.
*/
CacheInvalidateRelcache(rel);
+
+ /*
+ * If the partition we just detached is partitioned itself, invalidate
+ * relcache for all descendent partitions too to ensure that their
+ * rd_partcheck expression trees are rebuilt; must lock partitions
+ * before doing so.
+ */
+ if (partRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ List *partRel_children =
+ find_all_inheritors(RelationGetRelid(partRel),
+ AccessExclusiveLock, NULL);
The lock taken on the parent is either ShareUpdateExclusiveLock or
AccessExclusiveLock depending on whether CONCURRENTLY is specified or
not. Maybe that should be considered also when locking the children.
I've updated the patch that way. (Also, reintroduced the slightly
longer commit message that I had added in v3. :))
--
Amit Langote
EDB: http://www.enterprisedb.com
Attachment
The lock taken on the parent is either ShareUpdateExclusiveLock or
AccessExclusiveLock depending on whether CONCURRENTLY is specified or
not. Maybe that should be considered also when locking the children.
I've updated the patch that way. (Also, reintroduced the slightly
longer commit message that I had added in v3. :))
Thanks Amit, for your work!
I am little bit reluctant to the change you made in v5. As per https://www.postgresql.org/docs/14/sql-altertable.html:
> If
CONCURRENTLY is specified, ... the second transaction acquires SHARE UPDATE EXCLUSIVE on the partitioned table and ACCESS EXCLUSIVE on the partition, and the detach process completes.In comment to find_all_inheritors():
> The specified lock type is acquired on all child relations (but not on the given rel; caller should already have locked it)
So I conclude that it is done in a right way in v3 with ACCESS_EXCLUSIVE lock.
Also I'd recommend removing the link to a discussion from the test. Anyway we have link in a commit message.
-- Report: https://postgr.es/m/OS3PR01MB5718DA1C4609A25186D1FBF194089%40OS3PR01MB5718.jpnprd01.prod.outlook.com
--
Hi Pavel, On Fri, Oct 15, 2021 at 5:02 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: >> The lock taken on the parent is either ShareUpdateExclusiveLock or >> AccessExclusiveLock depending on whether CONCURRENTLY is specified or >> not. Maybe that should be considered also when locking the children. >> >> I've updated the patch that way. (Also, reintroduced the slightly >> longer commit message that I had added in v3. :)) > > > Thanks Amit, for your work! > > I am little bit reluctant to the change you made in v5. As per https://www.postgresql.org/docs/14/sql-altertable.html: > > > If CONCURRENTLY is specified, ... the second transaction acquires SHARE UPDATE EXCLUSIVE on the partitioned table andACCESS EXCLUSIVE on the partition, and the detach process completes. > > In comment to find_all_inheritors(): > > > The specified lock type is acquired on all child relations (but not on the given rel; caller should already have lockedit) > > So I conclude that it is done in a right way in v3 with ACCESS_EXCLUSIVE lock. Oops, you're right. I had failed to notice when reading the code that the second transaction takes an AccessExclusiveLock on the target partition. Reverted back to how this was in v3. > Also I'd recommend removing the link to a discussion from the test. Anyway we have link in a commit message. > -- Report: https://postgr.es/m/OS3PR01MB5718DA1C4609A25186D1FBF194089%40OS3PR01MB5718.jpnprd01.prod.outlook.com Yeah, maybe the link is unnecessary in the test comment, so removed. Though, I do occasionally see one of those in the test files (try `git grep https src/test`). Thanks again. -- Amit Langote EDB: http://www.enterprisedb.com
Attachment
Oops, you're right. I had failed to notice when reading the code that
the second transaction takes an AccessExclusiveLock on the target
partition. Reverted back to how this was in v3.
> Also I'd recommend removing the link to a discussion from the test. Anyway we have link in a commit message.
> -- Report: https://postgr.es/m/OS3PR01MB5718DA1C4609A25186D1FBF194089%40OS3PR01MB5718.jpnprd01.prod.outlook.com
Yeah, maybe the link is unnecessary in the test comment, so removed.
Though, I do occasionally see one of those in the test files (try `git
grep https src/test`).
Thanks! I don't see problems anymore. The patch is RFC now.
On 2021-Oct-18, Pavel Borisov wrote: > > Yeah, maybe the link is unnecessary in the test comment, so removed. > > Though, I do occasionally see one of those in the test files (try `git > > grep https src/test`). Yeah, for example the test stanza just above says "test case for but 16242". Sadly there's no bug number to quote here. Anyway, an interested reader will still be able to get to this thread via "git blame" when this get committed, which seems enough. > Thanks! I don't see problems anymore. The patch is RFC now. Thanks. I'm looking at it now. I notice that if I take out the code fix and keep the tests, I only see the ATTACH side of the problem have a failure; I expected to see a failure for DETACH too. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
On 2021-Oct-18, Alvaro Herrera wrote: > Thanks. I'm looking at it now. I notice that if I take out the code > fix and keep the tests, I only see the ATTACH side of the problem have a > failure; I expected to see a failure for DETACH too. Ah, no, the test covers both cases; it's just that if it fails the first time, it'll fail to fail the second time. But if I run it separately, and make it succeed the first time, then the second one will fail as expected. This becomes better visible by adding \c in a few places, but I don't think it's necessary to add it to the committed test -- I'm taking the code as Amit submitted. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ Maybe there's lots of data loss but the records of data loss are also lost. (Lincoln Yeoh)
Pushed now to all branches. Thanks much! -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "The important things in the world are problems with society that we don't understand at all. The machines will become more complicated but they won't be more complicated than the societies that run them." (Freeman Dyson)
On Tue, Oct 19, 2021 at 7:14 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Pushed now to all branches. Thanks much! Thanks Álavro. -- Amit Langote EDB: http://www.enterprisedb.com