Thread: MERGE issues around inheritance

Hi,

In [1] I added some verification to projection building, to check if the
tupleslot passed to ExecBuildProjectionInfo() is compatible with the target
list.  One query in merge.sql [2] got flagged.

Trying to debug that issue, I found another problem. This leaves us with:

1) w/ inheritance INSERTed rows are not returned by RETURNING. This seems to
   just generally not work with MERGE

2) w/ inheritance the projection for insert that ExecInitMerge() builds
   sometimes uses mismatching columns between the slot passed to
   ExecBuildProjectionInfo() and tgtslot.  The only reason this doesn't seem
   to immediately crash is 1).

   E.g. the attached repro-merge-inheritance.sql triggers [3] if the attached
   debugging patch is applied.


I wouldn't be surprised if there were other issues around inheritance with
mismatched column (order), but I didn't look further.


Greetings,

Andres Freund

[1] https://postgr.es/m/smj2mopf4t654xinuyyq4azik2d5zp2g3lsv2o7vficegqq5c5%40v35iq4llqjpw

[2] MERGE into measurement m
 USING new_measurement nm ON
      (m.city_id = nm.city_id and m.logdate=nm.logdate)
WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE
WHEN MATCHED THEN UPDATE
     SET peaktemp = greatest(m.peaktemp, nm.peaktemp),
        unitsales = m.unitsales + coalesce(nm.unitsales, 0)
WHEN NOT MATCHED THEN INSERT
     (city_id, logdate, peaktemp, unitsales)
   VALUES (city_id, logdate, peaktemp, unitsales);

triggers

2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  type mismatch: resno 1: slot (type: 0,
name:........pg.dropped.1........) vs expr (type: 23, name: city_id)
 
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL:     {TARGETENTRY
       :expr
          {VAR
          :varno -1
          :varattno 3
          :vartype 23
          :vartypmod -1
          :varcollid 0
          :varnullingrels (b)
          :varlevelsup 0
          :varreturningtype 0
          :varnosyn 2
          :varattnosyn 1
          :location 379
          }
       :resno 1
       :resname city_id
       :ressortgroupref 0
       :resorigtbl 0
       :resorigcol 0
       :resjunk false
       }

2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  type mismatch: resno 2: slot (type: 23,
name:peaktemp) vs expr (type: 1082, name: logdate)
 
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL:     {TARGETENTRY
       :expr
          {VAR
          :varno -1
          :varattno 4
          :vartype 1082
          :vartypmod -1
          :varcollid 0
          :varnullingrels (b)
          :varlevelsup 0
          :varreturningtype 0
          :varnosyn 2
          :varattnosyn 2
          :location 388
          }
       :resno 2
       :resname logdate
       :ressortgroupref 0
       :resorigtbl 0
       :resorigcol 0
       :resjunk false
       }

2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  type mismatch: resno 3: slot (type: 1082,
name:logdate) vs expr (type: 23, name: peaktemp)
 
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL:     {TARGETENTRY
       :expr
          {VAR
          :varno -1
          :varattno 1
          :vartype 23
          :vartypmod -1
          :varcollid 0
          :varnullingrels (b)
          :varlevelsup 0
          :varreturningtype 0
          :varnosyn 2
          :varattnosyn 3
          :location 397
          }
       :resno 3
       :resname peaktemp
       :ressortgroupref 0
       :resorigtbl 0
       :resorigcol 0
       :resjunk false
       }

2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  type mismatch: resno 5: slot (type: 23,
name:unitsales) vs expr (type: 25, name: last_action)
 
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING:  TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL:     {TARGETENTRY
       :expr
          {CONST
          :consttype 25
          :consttypmod -1
          :constcollid 100
          :constlen -1
          :constbyval false
          :constisnull false
          :location 418
          :constvalue 16 [ 64 0 0 0 109 101 114 103 101 45 105 110 115 101 114 116
           ]
          }
       :resno 5
       :resname last_action
       :ressortgroupref 0
       :resorigtbl 0
       :resorigcol 0
       :resjunk false
       }

[3] 2025-05-21 11:24:55.111 EDT [1838296][client backend][0/16:0][psql] WARNING:  type mismatch: resno 1: slot (type:
1700,name: dropme) vs expr (type: 25, name: key)
 
2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] WARNING:  TargetEntry:
2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] DETAIL:     {TARGETENTRY
       :expr
          {VAR
          :varno -1
          :varattno 1
          :vartype 25
          :vartypmod -1
          :varcollid 100
          :varnullingrels (b)
          :varlevelsup 0
          :varreturningtype 0
          :varnosyn 2
          :varattnosyn 1
          :location 179
          }
       :resno 1
       :resname key
       :ressortgroupref 0
       :resorigtbl 0
       :resorigcol 0
       :resjunk false
       }

On 2025-May-21, Andres Freund wrote:

> Hi,
> 
> In [1] I added some verification to projection building, to check if the
> tupleslot passed to ExecBuildProjectionInfo() is compatible with the target
> list.  One query in merge.sql [2] got flagged.
> 
> Trying to debug that issue, I found another problem. This leaves us with:
> 
> 1) w/ inheritance INSERTed rows are not returned by RETURNING. This seems to
>    just generally not work with MERGE

Hmm, curious.  One thing to observe is that the original source tuple is
in the child table, but the tuple inserted by MERGE ends up in the
parent table.  I'm guessing that the code gets confused as to the
relation that the tuple in the returned slot comes from, and that
somehow makes it somehow not "see" the tuple to process for RETURNING?
I dunno.  CC'ing Dean, who is more familiar with this code than I am.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)

On Sun, 25 May 2025 at 13:06, Tender Wang <tndrwang@gmail.com> wrote:
>
> For a partitioned table, we must pass rootResultRelInfo to ExecInsert(). I added the check before calling
ExecInsert()
> If it is a partitioned table, we continue to pass rootResultRelInfo. Otherwise, we pass resultRelInfo.
> Please see the attached diff file. The patch passed all regression test cases.
>

No, I don't think that's the right fix. I'm looking at it now, and I
think I have a fix, but it's more complicated than that. I'll post an
update later.

Regards,
Dean

On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sun, 25 May 2025 at 13:06, Tender Wang <tndrwang@gmail.com> wrote:
> >
> > For a partitioned table, we must pass rootResultRelInfo to ExecInsert(). I added the check before calling
ExecInsert()
> > If it is a partitioned table, we continue to pass rootResultRelInfo. Otherwise, we pass resultRelInfo.
> > Please see the attached diff file. The patch passed all regression test cases.
> >
>
> No, I don't think that's the right fix. I'm looking at it now, and I
> think I have a fix, but it's more complicated than that. I'll post an
> update later.
>

The reason that MERGE must pass rootResultRelInfo to ExecInsert() for
a plain-inheritance table dates back to 387f9ed0a08. As that commit
demonstrates, it is possible for the parent to be excluded from the
plan, and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo.

So there are indeed two related bugs here:

1. The projection built by ExecInitMerge() in the INSERT case may use
a tuple slot that's not compatible with the target table.

2. ExecInitModifyTable() does not initialize the WCO lists or
RETURNING list for rootResultRelInfo, so those never get executed.

As it happens, it is possible to construct cases where (1) causes a
crash, even without WCO's or a RETURNING list (see the first test case
in the attached patch), so this bug goes all the way back to v15,
where MERGE was introduced.

So I think we need to do something like the attached.

There is perhaps scope to reduce the code duplication between this and
ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
think it's best to leave that for now.

Regards,
Dean

On Mon, May 26, 2025 at 4:11 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
>
> 2. ExecInitModifyTable() does not initialize the WCO lists or
> RETURNING list for rootResultRelInfo, so those never get executed.
>
> As it happens, it is possible to construct cases where (1) causes a
> crash, even without WCO's or a RETURNING list (see the first test case
> in the attached patch), so this bug goes all the way back to v15,
> where MERGE was introduced.
>
> So I think we need to do something like the attached.
>
> There is perhaps scope to reduce the code duplication between this and
> ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
> think it's best to leave that for now.

+ Relation rootRelation = rootRelInfo->ri_RelationDesc;
+ Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc;
+ int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;

firstResultRel may equal (==) to rootRelation,
in that case, we don't need to call map_variable_attnos?

we can
        returningList = linitial(node->returningLists);
        if (rel != firstResultRel)
        {
            /* Convert any Vars in it to contain the root's attno's */
            part_attmap =
                build_attrmap_by_name(RelationGetDescr(rel),
                                      RelationGetDescr(firstResultRel),
                                      false);
            returningList = (List *)
                map_variable_attnos((Node *) returningList,
                                    firstVarno, 0,
                                    part_attmap,
                                    RelationGetForm(rel)->reltype,
                                    &found_whole_row);
        }
(i am not sure that will make code less readable).


we can unconditionally call ExecBuildProjectionInfo for rootRelInfo
within ExecInitModifyTable instead of in ExecInitMerge.
right after

        /*
         * Build a projection for each result rel.
         */
        resultRelInfo = mtstate->resultRelInfo;
        foreach(l, returningLists)
        {
            List       *rlist = (List *) lfirst(l);
            resultRelInfo->ri_returningList = rlist;
            resultRelInfo->ri_projectReturning =
                ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
                                        resultRelInfo->ri_RelationDesc->rd_att);
            resultRelInfo++;
        }

This would make related initiation logic stay together, also harmless (i think).
what do you think?

On Mon, 26 May 2025 at 10:30, jian he <jian.universality@gmail.com> wrote:
>
> + Relation rootRelation = rootRelInfo->ri_RelationDesc;
> + Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc;
> + int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;
>
> firstResultRel may equal (==) to rootRelation,
> in that case, we don't need to call map_variable_attnos?

Good point. I think that's by far the most common case, so that seems
like a worthwhile optimisation. v2 attached.


> we can unconditionally call ExecBuildProjectionInfo for rootRelInfo
> within ExecInitModifyTable instead of in ExecInitMerge.
> right after
>
>         /*
>          * Build a projection for each result rel.
>          */
>         resultRelInfo = mtstate->resultRelInfo;
>         foreach(l, returningLists)
>         {
>             List       *rlist = (List *) lfirst(l);
>             resultRelInfo->ri_returningList = rlist;
>             resultRelInfo->ri_projectReturning =
>                 ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
>                                         resultRelInfo->ri_RelationDesc->rd_att);
>             resultRelInfo++;
>         }
>
> This would make related initiation logic stay together, also harmless (i think).
> what do you think?

Well it would have to be done after calling ExecInitMerge() to set up
rootRelInfo->ri_returningList, but I don't think it really makes sense
to do it there. The patch intentionally only does it for a MERGE into
an inherited table when there are INSERT actions, and this also allows
the new code to be more consistent with ExecInitPartitionInfo().

Regards,
Dean

On Mon, 26 May 2025 at 07:46, Tender Wang <tndrwang@gmail.com> wrote:
>
> Hi Dean,
>
>  "it is possible for the parent to be excluded from the
> plan and so all of the entries in the resultRelInfo array may be for
> different relations than rootResultRelInfo."
>
> I didn't fully understand the above sentence.  Can you give me more information or an example?
> If the parent is excluded from the plan, the first entry in the resultRelInfo array will not be the parent but some
survivingchild.
 

There's an example in the updated regression tests. A non-inherited
CHECK constraint on the parent causes the planner to exclude the
parent from the relations being scanned and from the resultRelInfo
array, so the first resultRelInfo entry is for a child relation.

Regards,
Dean

On Wed, 28 May 2025 at 11:37, Tender Wang <tndrwang@gmail.com> wrote:
>
> Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年5月28日周三 18:26写道：
>>
>> Unless there are any further comments, I plan to commit it in a day or so.
>
> I don't have any other comments. It looks good for me.
>

Thanks for looking. I have committed this now.

Regards,
Dean