Re: Fix missing FORMAT when deparsing JSON_ARRAY(query)

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Richard Guo <guofenglinux(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, John Naylor <johncnaylorls(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Fix missing FORMAT when deparsing JSON_ARRAY(query)
Date: 2026-07-24 02:23:53
Message-ID: 1E0ED705-2B67-4FE9-A50A-B22C2FF7609F@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Jul 24, 2026, at 08:45, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Thu, Jul 23, 2026 at 11:59 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Actually, since I'm sitting here at the AI workshop, I thought I'd
>> try using Claude Code to generate a patch. Here's what it came up
>> with, after only light steering by me (I told it to factor the patch
>> this way, rather than making independent v19 and v20 patches).
>> I would have used a much shorter test case, and some of the comments
>> seem overly verbose too, but otherwise not bad.
>
> I just took a brief look at this bug and concluded that the root cause
> is that ForPortionOfExpr carries the range column in two redundant
> forms: rangeVar and range_name. At deparse time, ruleutils.c prints
> the range_name, which can be stale after RENAME.
>
> And the patch Claude came up with here makes sense to me. The
> rangeVar field already holds everything we need to print the
> up-to-date name, so removing the range_name field makes sense.
>
> A nit:
> 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.

I have a few more nitpicks:

1 - 0001
```
/*
* get_for_portion_of - print FOR PORTION OF if needed
+ *
+ * rte is the result relation's RTE, which we need to resolve the range
+ * column's current name.
+ *
* XXX: Newlines would help here, at least when pretty-printing. But then the
* 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)
```

Maybe there's no need to add an explanation of rte to the header comment, since none of the other arguments are described there. I remember Tom mentioning on one of my previous patches that documenting only some of the arguments might not be encouraged, see [1].

2 - 0002
```
+ * Note that the range column is identified only by rangeVar. We must not
+ * record its name here, because this node can be stored on disk (in a rule
+ * or a SQL function body) and the column could be renamed afterwards.
*----------
```

I'm not sure it's a good idea to mention rules and SQL functions specifically in this comment, because that could create a maintenance burden if things change in the future.

[1] Discussion: https://postgr.es/m/1734390.1768939652@sss.pgh.pa.us

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Yilin Zhang 2026-07-24 03:05:41 Re: explain plans for foreign servers
Previous Message Fujii Masao 2026-07-24 02:06:42 Collect ALTER PUBLICATION commands for event triggers