Date: Fri, 7 Feb 2020 17:19:33 +0100
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>,
 pgsql-bugs@lists.postgresql.org
Subject: Re: FK violation in partitioned table after truncating a referenced
 partition
Message-ID: <20200207171933.77aaaba6@firost>
In-Reply-To: <20200207150432.GA21902@alvherre.pgsql>
References: <20200207113203.3e26b28d@firost>
	<20200207150432.GA21902@alvherre.pgsql>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 7 Feb 2020 12:04:32 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> On 2020-Feb-07, Jehan-Guillaume de Rorthais wrote:
> 
> > Well, when reading myself, I found a bug in my algorithm. When looking for
> > parent constraints harvested during the first loop, I wasn't looking on
> > pg_contraint.oid, but on conparentid again. So instead of gathering parent
> > constraints to add the parent relation to the list of oids, I was only
> > adding siblings constraints. Here the fix:
> > 
> >     ScanKeyInit(&key,
> >   - 			Anum_pg_constraint_conparentid,
> >   + 			Anum_pg_constraint_oid
> >     			BTEqualStrategyNumber, F_OIDEQ,
> >     			ObjectIdGetDatum(parent));
> >     
> >   -  fkeyScan = systable_beginscan(fkeyRel, ConstraintParentIndexId,
> >   +  fkeyScan = systable_beginscan(fkeyRel, ConstraintOidIndexId,
> >     							   true, NULL,
> > 1, &key);  
> 
> Doh, of course.  I should have seen that.
> 
> Here's another take at the formulation; IMO the loop is more obvious
> this way, with a flag to restart from the top rather than keeping track
> of the list length.  But essentially this is your algorithm.

Yes, I recognize my algo with some cosmetic improvements, this obvious restart
flag I should have thought about and some welcomed code comments. I agree this
is more clear. Thanks!

Maybe I would just add:

 /*
  * If this constraint has a parent constraint which we have not seen
  * yet, keep track of it for the second loop, below.
+ * Tracking parent constraint allows to climb up to the top-level
+ * level constraint and look for all possible relation referencing 
+ * the partioned table.
  */

> I couldn't find any fault in this.

great!

> It would be nice if the cascaded truncation was more precise, ie. only
> truncate the referencing partitions that overlap the ranges covered by the
> referenced partition being truncated.

Yes, I was wondering about that when I was working on the first version of the
patch. It seems like a dedicated partitioning syntax when looking at other
RDBMSs. Eg. "PARTITION BY REFERENCE (col)" and "TRUNCATE PARTITION":

https://oracle-base.com/articles/12c/cascade-functionality-for-truncate-partition-and-exchange-partition-12cr1

> But that seems more difficult to achieve, as well as less clearly defined; if
> you really want something like that, I think you can detach the referenced
> partition.

This is out of the scope of this bug fix in my humble opinion. This would be a
whole new feature, even if it could be done without a new syntax.

Regards,