remove partColsUpdated
This was first added in commit 2f17844 (v11), and AFAICT it was only used
for a variable named update_tuple_routing_needed in ExecInitModifyTable(),
which was removed by commit c5b7ba4 (v14). Any objections to removing it
now?
--
nathan
Attachments:
v1-0001-remove-partColsUpdated.patchtext/plain; charset=us-asciiDownload
From 3f73736a755854c59051e0135c229758c8c18096 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 15 Oct 2025 10:40:18 -0500
Subject: [PATCH v1 1/1] remove partColsUpdated
---
src/backend/optimizer/plan/createplan.c | 4 ----
src/backend/optimizer/plan/planner.c | 2 --
src/backend/optimizer/util/inherit.c | 10 ----------
src/backend/optimizer/util/pathnode.c | 4 ----
src/include/nodes/pathnodes.h | 4 ----
src/include/nodes/plannodes.h | 2 --
src/include/optimizer/pathnode.h | 1 -
7 files changed, 27 deletions(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index c9dba7ff346..63fe6637155 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -311,7 +311,6 @@ static ProjectSet *make_project_set(List *tlist, Plan *subplan);
static ModifyTable *make_modifytable(PlannerInfo *root, Plan *subplan,
CmdType operation, bool canSetTag,
Index nominalRelation, Index rootRelation,
- bool partColsUpdated,
List *resultRelations,
List *updateColnosLists,
List *withCheckOptionLists, List *returningLists,
@@ -2676,7 +2675,6 @@ create_modifytable_plan(PlannerInfo *root, ModifyTablePath *best_path)
best_path->canSetTag,
best_path->nominalRelation,
best_path->rootRelation,
- best_path->partColsUpdated,
best_path->resultRelations,
best_path->updateColnosLists,
best_path->withCheckOptionLists,
@@ -7010,7 +7008,6 @@ static ModifyTable *
make_modifytable(PlannerInfo *root, Plan *subplan,
CmdType operation, bool canSetTag,
Index nominalRelation, Index rootRelation,
- bool partColsUpdated,
List *resultRelations,
List *updateColnosLists,
List *withCheckOptionLists, List *returningLists,
@@ -7047,7 +7044,6 @@ make_modifytable(PlannerInfo *root, Plan *subplan,
node->canSetTag = canSetTag;
node->nominalRelation = nominalRelation;
node->rootRelation = rootRelation;
- node->partColsUpdated = partColsUpdated;
node->resultRelations = resultRelations;
if (!onconflict)
{
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index e8ea78c0c97..342d782c74b 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -744,7 +744,6 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name,
else
root->wt_param_id = -1;
root->non_recursive_path = NULL;
- root->partColsUpdated = false;
/*
* Create the top-level join domain. This won't have valid contents until
@@ -2127,7 +2126,6 @@ grouping_planner(PlannerInfo *root, double tuple_fraction,
parse->canSetTag,
parse->resultRelation,
rootRelation,
- root->partColsUpdated,
resultRelations,
updateColnosLists,
withCheckOptionLists,
diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c
index 856d5959d10..6d5225079f8 100644
--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -335,16 +335,6 @@ expand_partitioned_rtentry(PlannerInfo *root, RelOptInfo *relinfo,
/* A partitioned table should always have a partition descriptor. */
Assert(partdesc);
- /*
- * Note down whether any partition key cols are being updated. Though it's
- * the root partitioned table's updatedCols we are interested in,
- * parent_updatedCols provided by the caller contains the root partrel's
- * updatedCols translated to match the attribute ordering of parentrel.
- */
- if (!root->partColsUpdated)
- root->partColsUpdated =
- has_partition_attrs(parentrel, parent_updatedCols, NULL);
-
/* Nothing further to do here if there are no partitions. */
if (partdesc->nparts == 0)
return;
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index bca51b4067b..44ac5312edd 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3612,8 +3612,6 @@ create_lockrows_path(PlannerInfo *root, RelOptInfo *rel,
* 'canSetTag' is true if we set the command tag/es_processed
* 'nominalRelation' is the parent RT index for use of EXPLAIN
* 'rootRelation' is the partitioned/inherited table root RTI, or 0 if none
- * 'partColsUpdated' is true if any partitioning columns are being updated,
- * either from the target relation or a descendent partitioned table.
* 'resultRelations' is an integer list of actual RT indexes of target rel(s)
* 'updateColnosLists' is a list of UPDATE target column number lists
* (one sublist per rel); or NIL if not an UPDATE
@@ -3630,7 +3628,6 @@ create_modifytable_path(PlannerInfo *root, RelOptInfo *rel,
Path *subpath,
CmdType operation, bool canSetTag,
Index nominalRelation, Index rootRelation,
- bool partColsUpdated,
List *resultRelations,
List *updateColnosLists,
List *withCheckOptionLists, List *returningLists,
@@ -3696,7 +3693,6 @@ create_modifytable_path(PlannerInfo *root, RelOptInfo *rel,
pathnode->canSetTag = canSetTag;
pathnode->nominalRelation = nominalRelation;
pathnode->rootRelation = rootRelation;
- pathnode->partColsUpdated = partColsUpdated;
pathnode->resultRelations = resultRelations;
pathnode->updateColnosLists = updateColnosLists;
pathnode->withCheckOptionLists = withCheckOptionLists;
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 01dbbca2753..30d889b54c5 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -593,9 +593,6 @@ struct PlannerInfo
bool *isAltSubplan pg_node_attr(read_write_ignore);
bool *isUsedSubplan pg_node_attr(read_write_ignore);
- /* Does this query modify any partition key columns? */
- bool partColsUpdated;
-
/* PartitionPruneInfos added in this query's plan. */
List *partPruneInfos;
@@ -2609,7 +2606,6 @@ typedef struct ModifyTablePath
bool canSetTag; /* do we set the command tag/es_processed? */
Index nominalRelation; /* Parent RT index for use of EXPLAIN */
Index rootRelation; /* Root RT index, if partitioned/inherited */
- bool partColsUpdated; /* some part key in hierarchy updated? */
List *resultRelations; /* integer list of RT indexes */
List *updateColnosLists; /* per-target-table update_colnos lists */
List *withCheckOptionLists; /* per-target-table WCO lists */
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 77ec2bc10b2..7cdd2b51c94 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -338,8 +338,6 @@ typedef struct ModifyTable
Index nominalRelation;
/* Root RT index, if partitioned/inherited */
Index rootRelation;
- /* some part key in hierarchy updated? */
- bool partColsUpdated;
/* integer list of RT indexes */
List *resultRelations;
/* per-target-table update_colnos lists */
diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h
index da60383c2aa..955e9056858 100644
--- a/src/include/optimizer/pathnode.h
+++ b/src/include/optimizer/pathnode.h
@@ -281,7 +281,6 @@ extern ModifyTablePath *create_modifytable_path(PlannerInfo *root,
Path *subpath,
CmdType operation, bool canSetTag,
Index nominalRelation, Index rootRelation,
- bool partColsUpdated,
List *resultRelations,
List *updateColnosLists,
List *withCheckOptionLists, List *returningLists,
--
2.39.5 (Apple Git-154)
On Oct 15, 2025, at 23:50, Nathan Bossart <nathandbossart@gmail.com> wrote:
This was first added in commit 2f17844 (v11), and AFAICT it was only used
for a variable named update_tuple_routing_needed in ExecInitModifyTable(),
which was removed by commit c5b7ba4 (v14). Any objections to removing it
now?--
nathan
<v1-0001-remove-partColsUpdated.patch>
Looks like this is only one assignment to it and nobody reads it. I don’t see a reason to retain it. Maybe back patch through 14?
I tried to build, and “make check” with this patch, and everything looks good.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Chao Li <li.evan.chao@gmail.com> writes:
On Oct 15, 2025, at 23:50, Nathan Bossart <nathandbossart@gmail.com> wrote:
This was first added in commit 2f17844 (v11), and AFAICT it was only used
for a variable named update_tuple_routing_needed in ExecInitModifyTable(),
which was removed by commit c5b7ba4 (v14). Any objections to removing it
now?
Looks like this is only one assignment to it and nobody reads it. I don’t see a reason to retain it.
I just had a look through https://codesearch.debian.net/ and couldn't
find any evidence that any extensions are using it, so +1 to remove.
Maybe back patch through 14?
Certainly not. That would cause an ABI break for any extension that
touches later fields in PlannerInfo or ModifyTable. We don't expect
extensions to get recompiled for minor releases.
regards, tom lane
On Wed, Oct 15, 2025 at 08:18:41PM -0400, Tom Lane wrote:
Chao Li <li.evan.chao@gmail.com> writes:
On Oct 15, 2025, at 23:50, Nathan Bossart <nathandbossart@gmail.com> wrote:
This was first added in commit 2f17844 (v11), and AFAICT it was only used
for a variable named update_tuple_routing_needed in ExecInitModifyTable(),
which was removed by commit c5b7ba4 (v14). Any objections to removing it
now?Looks like this is only one assignment to it and nobody reads it. I don’t see a reason to retain it.
I just had a look through https://codesearch.debian.net/ and couldn't
find any evidence that any extensions are using it, so +1 to remove.
Yes, that feels OK. I've also spent some time looking at what github
is able to refer to, but I can only find traces of forked code or
cloned repositories in the open, without specific uses for it. +1.
--
Michael
On Wed, Oct 15, 2025 at 08:18:41PM -0400, Tom Lane wrote:
Chao Li <li.evan.chao@gmail.com> writes:
On Oct 15, 2025, at 23:50, Nathan Bossart <nathandbossart@gmail.com> wrote:
This was first added in commit 2f17844 (v11), and AFAICT it was only used
for a variable named update_tuple_routing_needed in ExecInitModifyTable(),
which was removed by commit c5b7ba4 (v14). Any objections to removing it
now?Looks like this is only one assignment to it and nobody reads it. I
don’t see a reason to retain it.I just had a look through https://codesearch.debian.net/ and couldn't
find any evidence that any extensions are using it, so +1 to remove.
Thanks, will go commit it now.
Maybe back patch through 14?
Certainly not. That would cause an ABI break for any extension that
touches later fields in PlannerInfo or ModifyTable. We don't expect
extensions to get recompiled for minor releases.
Even if it wasn't an ABI break, there's very little upside to back-patching
small stuff like this that's not hurting anything. The risk/reward ratio
is not favorable.
--
nathan