Thread: A problem in ExecModifyTable
Hi hackers,
Recently, I noticed a great patch in pg 14.
"Rework planning and execution of UPDATE and DELETE. (86dc90056dfdbd9d1b891718d2e5614e3e432f35)"
This changes the DML execution of the partitioned table and makes it more friendly.
But I am very confused about the following changes:
```
+ relkind = resultRelInfo->ri_RelationDesc->rd_rel->relkind;
+ if (relkind == RELKIND_RELATION ||
+ relkind == RELKIND_MATVIEW ||
+ relkind == RELKIND_PARTITIONED_TABLE)
{
- char relkind;
- Datum datum;
- bool isNull;
-
- relkind = resultRelInfo->ri_RelationDesc->rd_rel->relkind;
- if (relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW)
- {
- datum = ExecGetJunkAttribute(slot,
- junkfilter->jf_junkAttNo,
- &isNull);
- /* shouldn't ever get a null result... */
```According to my understanding, the parent table of a partitioned table does not store any tuples.
Then why is "relkind = = RELKIND_PARTITIONED_TABLE" suddenly added here ?
There is no comment on this point in the code.
Can you answer my confusion? Be deeply grateful.
Regards & Thanks Adger
On Tue, 17 Aug 2021 at 15:56, 李杰(慎追) <adger.lj@alibaba-inc.com> wrote: > According to my understanding, the parent table of a partitioned table does not store any tuples. > Then why is "relkind = = RELKIND_PARTITIONED_TABLE" suddenly added here ? We'll need some sort of ResultRelInfo in the case that all partitions are pruned. Using the one for the partitioned table is convenient. I believe that's required if there are any statement-level triggers on the partitioned table or if there's a RETURNING clause. > There is no comment on this point in the code. > Can you answer my confusion? Be deeply grateful. Yeah maybe. It's not exactly close by, but one in grouping_planner mentions this: /* * We managed to exclude every child rel, so generate a * dummy one-relation plan using info for the top target * rel (even though that may not be a leaf target). * Although it's clear that no data will be updated or * deleted, we still need to have a ModifyTable node so * that any statement triggers will be executed. (This * could be cleaner if we fixed nodeModifyTable.c to allow * zero target relations, but that probably wouldn't be a * net win.) */ David
Hi, David
Your answer explains that we still need to ModifyTable node without Leaf partitions.
You are right about this.
But you can review the source code again,
```
/*
* Fetch rows from subplan, and execute the required table modification
* for each row.
*/ for (;;)
{ ... /* No more tuples to process? */
if (TupIsNull(planSlot))
break; ....
/*
* For UPDATE/DELETE, fetch the row identity info for the tuple to be
* updated/deleted. For a heap relation, that's a TID; otherwise we
* may have a wholerow junk attr that carries the old tuple in toto.
* Keep this in step with the part of ExecInitModifyTable that sets up
* ri_RowIdAttNo.
*/
if (operation == CMD_UPDATE || operation == CMD_DELETE)
{
char relkind;
Datum datum;
bool isNull;
relkind = resultRelInfo->ri_RelationDesc->rd_rel->relkind;
if (relkind == RELKIND_RELATION ||
relkind == RELKIND_MATVIEW ||
relkind == RELKIND_PARTITIONED_TABLE) ...
}
```
We can see that if the scanned tuple is NULL, it will break.
Therefore, this step will never be achieved that is to extract ctid.
>We'll need some sort of ResultRelInfo in the case that all partitions
>are pruned.
In this case, the tuple returned should be null.
>Using the one for the partitioned table is convenient. I
>believe that's required if there are any statement-level triggers on
>the partitioned table or if there's a RETURNING clause.
>believe that's required if there are any statement-level triggers on
>the partitioned table or if there's a RETURNING clause.
If the tuple to be modified is null, you do not need to process RETURNING clause.
statement-level triggers cannot use tuple, and triggers on partitioned tables cannot have transition tables.
I set Assert(relkind! = RELKIND_PARTITIONED_TABLE); in the code, But it never arrives.
Did I not understand your expression clearly?
Could you provide me with a case to verify it?
Thank you very much for your answer.
Regards & Thanks Adger
On Tue, 17 Aug 2021 at 19:06, 李杰(慎追) <adger.lj@alibaba-inc.com> wrote: > Your answer explains that we still need to ModifyTable node without Leaf partitions. > You are right about this. > > But you can review the source code again, I'd been looking at the code in ExecInitModifyTable() that's the same as what you pasted thinking you meant that. I think for the check for partitioned tables in ExecModifyTable() then it's likely just dead code. It seems to be causing a bit of confusion though, so might be worth doing something about. Copied in Tom to see what he thinks as it's one of his. David
David Rowley <dgrowleyml@gmail.com> writes:
> I'd been looking at the code in ExecInitModifyTable() that's the same
> as what you pasted thinking you meant that. I think for the check for
> partitioned tables in ExecModifyTable() then it's likely just dead
> code. It seems to be causing a bit of confusion though, so might be
> worth doing something about. Copied in Tom to see what he thinks as
> it's one of his.
Yeah, it's dead code in the sense that we should never reach these
if-blocks with a partitioned table. I believe the reason I made it
like that is that the logic is concerned with which style of row identity
applies to a particular relkind, and in the planner we treat partitioned
tables as requiring this style of row identity, so that the right things
happen for their children. So the answer is basically "for consistency
with add_row_identity_columns".
Maybe we could instead have add_row_identity_columns do nothing for
a partitioned table, but I'm unconvinced of that.
regards, tom lane