From: | Tender Wang <tndrwang(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Alexander Lakhin <exclusion(at)gmail(dot)com>, Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Guillaume Lelarge <guillaume(at)lelarge(dot)info> |
Subject: | Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails |
Date: | 2024-10-27 11:00:44 |
Message-ID: | CAHewXNk8hn_Kq=3gb583O1U1WU=T+VMRF8NDZfTa35wxcJ+xcA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | jian he | 2024-10-27 11:32:45 | Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row |
Previous Message | Alexander Lakhin | 2024-10-27 11:00:00 | Re: Statistics Import and Export |