| 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/
| 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 |