Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails - Mailing list pgsql-hackers

From Tender Wang
Subject Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Date
Msg-id CAHewXNk8hn_Kq=3gb583O1U1WU=T+VMRF8NDZfTa35wxcJ+xcA@mail.gmail.com
Whole thread Raw
In response to Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
List pgsql-hackers


Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年10月27日周日 05:47写道:
On 2024-Oct-25, Alvaro Herrera wrote:

> On 2024-Oct-25, Tender Wang wrote:
>
> > Thanks for reporting. I can reproduce this memory invalid access on HEAD.
> > After debuging codes, I found the root cause.
> >  In DetachPartitionFinalize(), below code:
> > att = TupleDescAttr(RelationGetDescr(partRel),
> >                                attmap->attnums[conkey[i] - 1] - 1);
> >
> > We should use confkey[i] -1 not conkey[i] - 1;
>
> Wow, how embarrasing, now that's one _really_ stupid bug and it's
> totally my own.  Thanks for the analysis!  I'll get this patched soon.

Actually, now that I look at this again, I think this proposed fix is
wrong; conkey/confkey confusion is not what the problem is.  Rather, the
problem is that we're applying a tuple conversion map when none should
be applied.  So the fix is to remove "attmap" altogether.  One thing
that didn't appear correct to me was that the patch was changing one
constraint name so that it appeared that the constrained columns were
"id, p_id" but that didn't make sense: they are "p_id, p_jd" instead.

Yeah, The constrained name  change made me think about if the patch is correct.
After your explanation, I have understood it.

Then I realized that you're wrong that it's the referenced side that
we're processing: what we're doing there is generate the fk_attrs list,
which is the list of constrained columns (not the list of referenced
columns, which is pk_attrs).

I also felt like modifying the fkpart12 test rather than adding a
separate fkpart13, so I did that.

So here's a v2 for this.  (Commit message needs love still, but it's a
bit late here.)


The v2 LGTM.
BTW, while reviewing the v2 patch, I found the parentConTup in foreach(cell, fks) block
didn't need it anymore. We can remove the related codes. 

--
Thanks,
Tender Wang

pgsql-hackers by date:

Previous
From: Alexander Lakhin
Date:
Subject: Re: Statistics Import and Export
Next
From: jian he
Date:
Subject: Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row