Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

Hi,

Thanks everyone for noticing this. It is indeed very obviously broken. :(

On Fri, Dec 23, 2022 at 11:26 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> On Fri, 23 Dec 2022 at 15:21, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> >
> > On Thu, Dec 22, 2022 at 5:22 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> >> I still think we should have a test to ensure this is actually
> >> working. Do you want to write one?
> >
> >
> > I agree that we should have a test. According to the code coverage
> > report, the recursion part of this function is never tested. I will
> > have a try to see if I can come up with a proper test case.
>
> I started having a go at writing one yesterday. I only got as far as
> the following.
> It looks like it'll need a trigger or something added to the foreign
> table to hit the code path that would be needed to trigger the issue,
> so it'll need more work. It might be a worthy starting point.

I was looking at this last night and found that having a generated
column in the table, but not a trigger, helps hit the buggy code.
Having a generated column in the foreign partition prevents a direct
update and thus hitting the following block of
postgresPlanForeignModify():

else if (operation == CMD_UPDATE)
{
int col;
RelOptInfo *rel = find_base_rel(root, resultRelation);
Bitmapset *allUpdatedCols = get_rel_all_updated_cols(root, rel);

col = -1;
while ((col = bms_next_member(allUpdatedCols, col)) >= 0)
{
/* bit numbers are offset by FirstLowInvalidHeapAttributeNumber */
AttrNumber attno = col + FirstLowInvalidHeapAttributeNumber;

if (attno <= InvalidAttrNumber) /* shouldn't happen */
elog(ERROR, "system-column update is not supported");
targetAttrs = lappend_int(targetAttrs, attno);
}
}

If you add a trigger, which does help with getting a non-direct
update, the code block above this one is executed, so
get_rel_all_updated_cols() isn't called.

Attached shows a test case I was able to come up with that I can see
is broken by a61b1f74 though passes after applying Richard's patch.
What's broken is that deparseUpdateSql() outputs a remote UPDATE
statement with the wrong SET column list, because the wrong attribute
numbers would be added to the targetAttrs list by the above code block
after the buggy multi-level translation in ger_rel_all_updated_cols().

Thanks for writing the patch, Richard.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com