Re: Fix missing FORMAT when deparsing JSON_ARRAY(query)

Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> writes:
> On Jul 24, 2026, at 08:45, Richard Guo <guofenglinux(at)gmail(dot)com> 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