Date: Fri, 7 Feb 2020 11:32:03 +0100
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: FK violation in partitioned table after truncating a referenced
 partition
Message-ID: <20200207113203.3e26b28d@firost>
In-Reply-To: <20200206230129.GA9760@alvherre.pgsql>
References: <20200206223015.GA32754@alvherre.pgsql>
 <20200206230129.GA9760@alvherre.pgsql>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/mv_X5Ubk18+7hJI71UOlhTs"
Precedence: bulk

--MP_/mv_X5Ubk18+7hJI71UOlhTs
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

Thank you for the review and thoughts Alvaro.

On Thu, 6 Feb 2020 20:01:29 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> On 2020-Feb-06, Alvaro Herrera wrote:
> 
> > On 2020-Feb-06, Alvaro Herrera wrote:
> >   
> > > I agree that patching heap_truncate_find_FKs is a reasonable way to fix.
> > > I propose a slightly different formulation: instead of the loop that you
> > > have, we can just use the second loop, and add more parent constraints
> > > to the list if any constraint we scan in turn has a parent constraint.
> > > So we don't repeat the whole thing, but only that second loop.  
> > 
> > Hmm, this doesn't actually work; I modified your test case and I see
> > that my code fails to do the right thing.  
> 
> Yeah, AFAICS both algorithms posted so far (yours and mine) are wrong.

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);

[...]
> ENOTIME to complete it now, though ... also: I'm not sure about having
> heap_truncate_find_FKs() acquire the locks on partitions;

ExecuteTruncate and ExecuteTruncateGuts are responsible to open and
lock relations. It might be messy or racy between those and
heap_truncate_find_FKs if the later open/lock or open/nolock while looking for
relations.

> but what happens if there's a concurrent detach?

Not sure. Are you talking about the referenced or referencing side?

> This is a larger can of worms than I imagined.  Maybe a simpler solution
> is to say that you cannot truncate a partition; if you want that,
> truncate the topmost relation.

I thought about this as well, but it might be a feature regression in a minor
version.

> No functionality seems lost with that restriction, or is it?

It does. When truncating a partition, you left untouched other siblings. You
did not truncate the whole partioned table. this is the last query in my
original test.

I added some more words to the doc about this. Please, find in attachment a new
version of bug fix proposal.

Regards,

--MP_/mv_X5Ubk18+7hJI71UOlhTs
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0001-v2-Fix-TRUNCATE-on-a-partition-to-apply-CASCADE-to-part.patch