Hi, Thanks everyone for noticing this. It is indeed very obviously broken. :(
On Fri, Dec 23, 2022 at 11:26 AM David Rowley <dgrowle...@gmail.com> wrote: > On Fri, 23 Dec 2022 at 15:21, Richard Guo <guofengli...@gmail.com> wrote: > > > > On Thu, Dec 22, 2022 at 5:22 PM David Rowley <dgrowle...@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
v1-0001-postgres_fdw-test-update-of-multi-level-partition.patch
Description: Binary data