Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c) - Mailing list pgsql-hackers

Hi,

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

On Fri, Dec 23, 2022 at 11:26 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Fri, 23 Dec 2022 at 15:21, Richard Guo <guofenglinux@gmail.com> wrote:
> >
> > On Thu, Dec 22, 2022 at 5:22 PM David Rowley <dgrowleyml@gmail.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