Chao Li <[email protected]> writes:
> On Jul 24, 2026, at 08:45, Richard Guo <[email protected]> wrote:
>> I'm not sure about this change in 0002:
>>
>> tle = makeTargetEntry((Expr *) rangeTLEExpr, range_attno,
>> - forPortionOf->range_name, false);
>> + pstrdup(NameStr(attr->attname)), false);
>>
>> forPortionOf is a ForPortionOfClause, and range_name is a valid field.
> I agree. ForPortionOfExpr.range_name is being removed, but forPortionOf is of
> type ForPortionOfClause whose range_name is valid.
Agreed, this change is unnecessary, and inconsistent with nearby
uses of forPortionOf->range_name.
> I have a few more nitpicks:
Yeah, both of those were comments I thought were unnecessary.
I thought the test case was overdone too: there's not really any
need AFAICS to test the SQL-function-deparse case separately.
Another odd thing when I looked closer is that Claude did the
core fix in ruleutils.c with
+ range_name = get_rte_attribute_name(rte,
+ forPortionOf->rangeVar->varattno);
rather than
+ range_name = get_attname(rte->relid,
+ forPortionOf->rangeVar->varattno,
+ false);
which is the way that target column names are fetched in the adjacent
get_update_query_targetlist_def code. I think get_rte_attribute_name
ends up being equivalent, since the UPDATE target rel can't have any
column aliases, but it's inconsistent and formally incorrect. Even
odder, I distinctly recall that Claude's first draft (before I told
it to partition the patch differently) did this correctly and even
called out the analogy to get_update_query_targetlist_def in its
commit message. Memo to self: LLMs are nondeterministic.
Anyway, I've actually reviewed the attached v2 and think it's solid.
regards, tom lane
From 385d2e492da7b9aaa87e6f8f0b333c3cb2c8904b Mon Sep 17 00:00:00 2001
From: Tom Lane <[email protected]>
Date: Fri, 24 Jul 2026 12:19:52 -0400
Subject: [PATCH v2] Deparse FOR PORTION OF using the range column's current
name.
Commit 8e72d914c recorded the range column's name in ForPortionOfExpr
and used that for deparsing FOR PORTION OF. This gives the wrong
answer if the ForPortionOfExpr is saved in a rule or SQL function and
then the column gets renamed. Drop the ForPortionOfExpr.range_name
field; instead fetch the current column name from the catalogs when
needed.
Also drop ForPortionOfState.fp_rangeName, which wasn't being used
anywhere.
Full disclosure: an earlier draft of this patch was made with
Claude Opus 4.8.
XXX: don't forget catversion bump
Reported-by: John Naylor <[email protected]>
Author: Tom Lane <[email protected]>
Reviewed-by: Richard Guo <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/canwcazyfepj5oi45gi4q9y6lya4_oiaxxunnwe-1ym-i0ff...@mail.gmail.com
Backpatch-through: 19
---
src/backend/executor/nodeModifyTable.c | 2 --
src/backend/optimizer/plan/planner.c | 4 ++-
src/backend/parser/analyze.c | 1 -
src/backend/utils/adt/ruleutils.c | 15 ++++++---
src/include/nodes/execnodes.h | 1 -
src/include/nodes/primnodes.h | 1 -
src/test/regress/expected/for_portion_of.out | 33 ++++++++++++++++++++
src/test/regress/sql/for_portion_of.sql | 16 ++++++++++
8 files changed, 63 insertions(+), 10 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index b9781eb3b95..1dbf0ffff9e 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -5641,7 +5641,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
/* Create state for FOR PORTION OF operation */
fpoState = makeNode(ForPortionOfState);
- fpoState->fp_rangeName = forPortionOf->range_name;
fpoState->fp_rangeType = forPortionOf->rangeType;
fpoState->fp_rangeAttno = forPortionOf->rangeVar->varattno;
fpoState->fp_targetRange = targetRange;
@@ -5928,7 +5927,6 @@ ExecInitForPortionOf(ModifyTableState *mtstate, EState *estate,
leafState = makeNode(ForPortionOfState);
- leafState->fp_rangeName = fpoState->fp_rangeName;
leafState->fp_rangeType = fpoState->fp_rangeType;
leafState->fp_targetRange = fpoState->fp_targetRange;
map = ExecGetChildToRootMap(resultRelInfo);
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 3225185d16f..a0ff9159ae0 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -871,7 +871,9 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot use generated column \"%s\" in FOR PORTION OF",
- forPortionOf->range_name)));
+ get_attname(rte->relid,
+ forPortionOf->rangeVar->varattno,
+ false))));
}
/*
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 562e4facd74..581457c69c9 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1606,7 +1606,6 @@ transformForPortionOfClause(ParseState *pstate,
else
result->rangeTargetList = NIL;
- result->range_name = forPortionOf->range_name;
result->location = forPortionOf->location;
result->targetLocation = forPortionOf->target_location;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1b44b7a78d2..0e9f311dd79 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -527,6 +527,7 @@ static void get_rte_alias(RangeTblEntry *rte, int varno, bool use_as,
static void get_column_alias_list(deparse_columns *colinfo,
deparse_context *context);
static void get_for_portion_of(ForPortionOfExpr *forPortionOf,
+ RangeTblEntry *rte,
deparse_context *context);
static void get_from_clause_coldeflist(RangeTblFunction *rtfunc,
deparse_columns *colinfo,
@@ -7556,7 +7557,7 @@ get_update_query_def(Query *query, deparse_context *context)
generate_relation_name(rte->relid, NIL));
/* Print the FOR PORTION OF, if needed */
- get_for_portion_of(query->forPortionOf, context);
+ get_for_portion_of(query->forPortionOf, rte, context);
/* Print the relation alias, if needed */
get_rte_alias(rte, query->resultRelation, false, context);
@@ -7763,7 +7764,7 @@ get_delete_query_def(Query *query, deparse_context *context)
generate_relation_name(rte->relid, NIL));
/* Print the FOR PORTION OF, if needed */
- get_for_portion_of(query->forPortionOf, context);
+ get_for_portion_of(query->forPortionOf, rte, context);
/* Print the relation alias, if needed */
get_rte_alias(rte, query->resultRelation, false, context);
@@ -13478,12 +13479,18 @@ get_rte_alias(RangeTblEntry *rte, int varno, bool use_as,
* alias and SET will be on their own line with a leading space.
*/
static void
-get_for_portion_of(ForPortionOfExpr *forPortionOf, deparse_context *context)
+get_for_portion_of(ForPortionOfExpr *forPortionOf, RangeTblEntry *rte,
+ deparse_context *context)
{
if (forPortionOf)
{
+ char *range_name;
+
+ range_name = get_attname(rte->relid,
+ forPortionOf->rangeVar->varattno,
+ false);
appendStringInfo(context->buf, " FOR PORTION OF %s",
- quote_identifier(forPortionOf->range_name));
+ quote_identifier(range_name));
/*
* Try to write it as FROM ... TO ... if we received it that way,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index e64fd8c7ea3..e95ac3eda35 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -476,7 +476,6 @@ typedef struct ForPortionOfState
{
NodeTag type;
- char *fp_rangeName; /* the column named in FOR PORTION OF */
Oid fp_rangeType; /* the base type (not domain) of the FOR
* PORTION OF expression */
int fp_rangeAttno; /* the attno of the range column */
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index cacef7d4151..adff8d4c682 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -2433,7 +2433,6 @@ typedef struct ForPortionOfExpr
{
NodeTag type;
Var *rangeVar; /* Range column */
- char *range_name; /* Range name */
Node *targetFrom; /* FOR PORTION OF FROM bound, if given */
Node *targetTo; /* FOR PORTION OF TO bound, if given */
Node *targetRange; /* FOR PORTION OF bounds as a range/multirange */
diff --git a/src/test/regress/expected/for_portion_of.out b/src/test/regress/expected/for_portion_of.out
index 0e217f104ef..3c592f90e70 100644
--- a/src/test/regress/expected/for_portion_of.out
+++ b/src/test/regress/expected/for_portion_of.out
@@ -2255,6 +2255,39 @@ SELECT * FROM fpo_rule ORDER BY f1;
(2 rows)
DROP TABLE fpo_rule;
+-- Deparsing FOR PORTION OF must use the range column's current name,
+-- not the name it had when the rule was created.
+CREATE TABLE fpo_rename (f1 bigint, f2 int4range);
+CREATE TABLE fpo_rename_src (x int);
+CREATE RULE fpo_rename_rule1 AS ON UPDATE TO fpo_rename_src
+ DO INSTEAD UPDATE fpo_rename FOR PORTION OF f2 FROM 3 TO 6 SET f1 = 2;
+CREATE RULE fpo_rename_rule2 AS ON DELETE TO fpo_rename_src
+ DO INSTEAD DELETE FROM fpo_rename FOR PORTION OF f2 (int4range(3, 6));
+\d+ fpo_rename_src
+ Table "public.fpo_rename_src"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ x | integer | | | | plain | |
+Rules:
+ fpo_rename_rule1 AS
+ ON UPDATE TO fpo_rename_src DO INSTEAD UPDATE fpo_rename FOR PORTION OF f2 FROM 3 TO 6 SET f1 = 2
+ fpo_rename_rule2 AS
+ ON DELETE TO fpo_rename_src DO INSTEAD DELETE FROM fpo_rename FOR PORTION OF f2 (int4range(3, 6))
+
+ALTER TABLE fpo_rename RENAME COLUMN f1 TO ff1;
+ALTER TABLE fpo_rename RENAME COLUMN f2 TO ff2;
+\d+ fpo_rename_src
+ Table "public.fpo_rename_src"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ x | integer | | | | plain | |
+Rules:
+ fpo_rename_rule1 AS
+ ON UPDATE TO fpo_rename_src DO INSTEAD UPDATE fpo_rename FOR PORTION OF ff2 FROM 3 TO 6 SET ff1 = 2
+ fpo_rename_rule2 AS
+ ON DELETE TO fpo_rename_src DO INSTEAD DELETE FROM fpo_rename FOR PORTION OF ff2 (int4range(3, 6))
+
+DROP TABLE fpo_rename, fpo_rename_src;
-- UPDATE/DELETE FOR PORTION OF on a GENERATED VIRTUAL range column:
CREATE TABLE fpo_gen_virtual (
a int,
diff --git a/src/test/regress/sql/for_portion_of.sql b/src/test/regress/sql/for_portion_of.sql
index a8d29a76b22..5f7b04fdf6b 100644
--- a/src/test/regress/sql/for_portion_of.sql
+++ b/src/test/regress/sql/for_portion_of.sql
@@ -1495,6 +1495,22 @@ SELECT * FROM fpo_rule ORDER BY f1;
DROP TABLE fpo_rule;
+-- Deparsing FOR PORTION OF must use the range column's current name,
+-- not the name it had when the rule was created.
+CREATE TABLE fpo_rename (f1 bigint, f2 int4range);
+CREATE TABLE fpo_rename_src (x int);
+CREATE RULE fpo_rename_rule1 AS ON UPDATE TO fpo_rename_src
+ DO INSTEAD UPDATE fpo_rename FOR PORTION OF f2 FROM 3 TO 6 SET f1 = 2;
+CREATE RULE fpo_rename_rule2 AS ON DELETE TO fpo_rename_src
+ DO INSTEAD DELETE FROM fpo_rename FOR PORTION OF f2 (int4range(3, 6));
+
+\d+ fpo_rename_src
+ALTER TABLE fpo_rename RENAME COLUMN f1 TO ff1;
+ALTER TABLE fpo_rename RENAME COLUMN f2 TO ff2;
+\d+ fpo_rename_src
+
+DROP TABLE fpo_rename, fpo_rename_src;
+
-- UPDATE/DELETE FOR PORTION OF on a GENERATED VIRTUAL range column:
CREATE TABLE fpo_gen_virtual (
a int,
--
2.52.0
