Fix missing FORMAT when deparsing JSON_ARRAY(query)

On Wed, Jul 22, 2026 at 2:22 PM Chao Li <li.evan.chao@gmail.com> wrote:

Hi,

While testing "[8d829f5a0] Fix JSON_ARRAY(query) empty set handling and view deparsing”, I found that the departing may omit the FORMAT JSON clause.

Here is a simple repro:
```
evantest=# create view v as
evantest-# select json_array(select '{"a": 1}'::text format json) as j;
CREATE VIEW
evantest=# select * from v;
j
------------
[{"a": 1}]
(1 row)

evantest=# select pg_get_viewdef('v'::regclass, true);
pg_get_viewdef
---------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
(1 row)

evantest=# select pg_get_viewdef('v'::regclass, false);
pg_get_viewdef
---------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
(1 row)

evantest=# SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
j
----------------
["{\"a\": 1}"]
(1 row)
```

As shown above, I defined the view with FORMAT JSON, but the deparsed SQL has lost that clause. Running the deparsed SELECT produces a different result from selecting from the view because FORMAT JSON is missing.

Currently, JsonConstructorExpr does not store the JsonFormat information. To fix this problem, we need to add a JsonFormat field to JsonConstructorExpr. Please see the attached patch for details.

With the fix:
```
evantest=# select pg_get_viewdef('v'::regclass, true);
pg_get_viewdef
---------------------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text FORMAT JSON RETURNING json) AS j;
(1 row)

evantest=# SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text FORMAT JSON RETURNING json) AS j;
j
------------
[{"a": 1}]
(1 row)
```

Thanks for the patch — I reviewed and tested it, and it looks correct.

One thing that seems missing: since JsonConstructorExpr is stored in
pg_rewrite (and in SQL-function bodies), adding a field changes the
stored node representation,
so this should bump CATALOG_VERSION_NO?

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

--
Regards,
Ewan Young

On Jul 23, 2026, at 15:25, Ewan Young <kdbase.hack@gmail.com> wrote:

On Wed, Jul 22, 2026 at 2:22 PM Chao Li <li.evan.chao@gmail.com> wrote:

Hi,

While testing "[8d829f5a0] Fix JSON_ARRAY(query) empty set handling and view deparsing”, I found that the departing may omit the FORMAT JSON clause.

Here is a simple repro:
```
evantest=# create view v as
evantest-# select json_array(select '{"a": 1}'::text format json) as j;
CREATE VIEW
evantest=# select * from v;
j
------------
[{"a": 1}]
(1 row)

evantest=# select pg_get_viewdef('v'::regclass, true);
pg_get_viewdef
---------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
(1 row)

evantest=# select pg_get_viewdef('v'::regclass, false);
pg_get_viewdef
---------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
(1 row)

evantest=# SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
j
----------------
["{\"a\": 1}"]
(1 row)
```

As shown above, I defined the view with FORMAT JSON, but the deparsed SQL has lost that clause. Running the deparsed SELECT produces a different result from selecting from the view because FORMAT JSON is missing.

Currently, JsonConstructorExpr does not store the JsonFormat information. To fix this problem, we need to add a JsonFormat field to JsonConstructorExpr. Please see the attached patch for details.

With the fix:
```
evantest=# select pg_get_viewdef('v'::regclass, true);
pg_get_viewdef
---------------------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text FORMAT JSON RETURNING json) AS j;
(1 row)

evantest=# SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text FORMAT JSON RETURNING json) AS j;
j
------------
[{"a": 1}]
(1 row)
```

Thanks for the patch — I reviewed and tested it, and it looks correct.

Thank you very much for the review.

One thing that seems missing: since JsonConstructorExpr is stored in
pg_rewrite (and in SQL-function bodies), adding a field changes the
stored node representation,
so this should bump CATALOG_VERSION_NO?

You are right, CATALOG_VERSION_NO needs to be bumped. As a general rule, however, the committer will do that before pushing, so submitted patches should never touch CATALOG_VERSION_NO.

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

On Thu, Jul 23, 2026 at 4:24 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 23, 2026, at 15:25, Ewan Young <kdbase.hack@gmail.com> wrote:

On Wed, Jul 22, 2026 at 2:22 PM Chao Li <li.evan.chao@gmail.com> wrote:

Hi,

While testing "[8d829f5a0] Fix JSON_ARRAY(query) empty set handling and view deparsing”, I found that the departing may omit the FORMAT JSON clause.

Here is a simple repro:
```
evantest=# create view v as
evantest-# select json_array(select '{"a": 1}'::text format json) as j;
CREATE VIEW
evantest=# select * from v;
j
------------
[{"a": 1}]
(1 row)

evantest=# select pg_get_viewdef('v'::regclass, true);
pg_get_viewdef
---------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
(1 row)

evantest=# select pg_get_viewdef('v'::regclass, false);
pg_get_viewdef
---------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
(1 row)

evantest=# SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text RETURNING json) AS j;
j
----------------
["{\"a\": 1}"]
(1 row)
```

As shown above, I defined the view with FORMAT JSON, but the deparsed SQL has lost that clause. Running the deparsed SELECT produces a different result from selecting from the view because FORMAT JSON is missing.

Currently, JsonConstructorExpr does not store the JsonFormat information. To fix this problem, we need to add a JsonFormat field to JsonConstructorExpr. Please see the attached patch for details.

With the fix:
```
evantest=# select pg_get_viewdef('v'::regclass, true);
pg_get_viewdef
---------------------------------------------------------------------------------------
SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text FORMAT JSON RETURNING json) AS j;
(1 row)

evantest=# SELECT JSON_ARRAY( SELECT '{"a": 1}'::text AS text FORMAT JSON RETURNING json) AS j;
j
------------
[{"a": 1}]
(1 row)
```

Thanks for the patch — I reviewed and tested it, and it looks correct.

Thank you very much for the review.

One thing that seems missing: since JsonConstructorExpr is stored in
pg_rewrite (and in SQL-function bodies), adding a field changes the
stored node representation,
so this should bump CATALOG_VERSION_NO?

You are right, CATALOG_VERSION_NO needs to be bumped. As a general rule, however, the committer will do that before pushing, so submitted patches should never touch CATALOG_VERSION_NO.

Thanks for explaining.
Other than that, the patch looks good to me.

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

--
Regards,
Ewan Young

On Wed, Jul 22, 2026 at 3:22 PM Chao Li <li.evan.chao@gmail.com> wrote:

Currently, JsonConstructorExpr does not store the JsonFormat information. To fix this problem, we need to add a JsonFormat field to JsonConstructorExpr. Please see the attached patch for details.

I considered recovering the format from func, as the arg that carries
the format lives there. However, reaching it requires walking a
complex CoalesceExpr tree to find the JsonValueExpr, which seems too
fragile.

So, recording the input FORMAT in JsonConstructorExpr for deparsing
seems like the most feasible approach.

Regarding the patch, I don't think we need to handle format in
expression_tree_mutator/walker. I also don't think we need to
copyObject the format. Other than that, the patch LGTM.

I plan to push the attached patch soon barring any objections.

- Richard

On Jul 23, 2026, at 16:57, Richard Guo <guofenglinux@gmail.com> wrote:

On Wed, Jul 22, 2026 at 3:22 PM Chao Li <li.evan.chao@gmail.com> wrote:

Currently, JsonConstructorExpr does not store the JsonFormat information. To fix this problem, we need to add a JsonFormat field to JsonConstructorExpr. Please see the attached patch for details.

I considered recovering the format from func, as the arg that carries
the format lives there. However, reaching it requires walking a
complex CoalesceExpr tree to find the JsonValueExpr, which seems too
fragile.

So, recording the input FORMAT in JsonConstructorExpr for deparsing
seems like the most feasible approach.

Regarding the patch, I don't think we need to handle format in
expression_tree_mutator/walker. I also don't think we need to
copyObject the format. Other than that, the patch LGTM.

Agreed, looks like format is immutable, so copy is not needed.

I plan to push the attached patch soon barring any objections.

- Richard
<v2-0001-Fix-deparsing-of-JSON_ARRAY-subquery-with-a-FORMA.patch>

V2 looks good.

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

On Thu, Jul 23, 2026 at 4:57 AM Richard Guo <guofenglinux@gmail.com> wrote:

I plan to push the attached patch soon barring any objections.

No objection, but I'd like to encourage submitters to dig deeper when
they find things like this. Deparsing is relevant to work I'm doing,
and I wanted to see if we can detect this kind of omission earlier. It
turns out my work doesn't enable that, but while looking into it
Claude offered to search for other bugs of this kind and found this:

CREATE TABLE emp (id int, valid daterange, salary int);
CREATE TABLE t (x int);

CREATE RULE r AS ON UPDATE TO t DO INSTEAD
UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01'
SET salary = 1;

-- rename column
ALTER TABLE emp RENAME COLUMN valid TO validity;

INSERT INTO emp VALUES (1, daterange('2019-01-01','2022-01-01'), 0);
UPDATE t SET x = 0; -- fires the rule

=# select * from emp ;
id | validity | salary
----+-------------------------+--------
1 | [2020-01-01,2021-01-01) | 1
1 | [2019-01-01,2020-01-01) | 0
1 | [2021-01-01,2022-01-01) | 0
(3 rows)

dump and restore:

ERROR: column "valid" of relation "emp" does not exist
LINE 2: ...ic.t DO INSTEAD UPDATE public.emp FOR PORTION OF valid FROM...

I don't have time to look at a fix right now, but I wanted to flag it
now in case it makes sense to fix these together.

--
John Naylor
Amazon Web Services

John Naylor <johncnaylorls@gmail.com> writes:

Claude offered to search for other bugs of this kind and found this:

CREATE TABLE emp (id int, valid daterange, salary int);
CREATE TABLE t (x int);

CREATE RULE r AS ON UPDATE TO t DO INSTEAD
UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01'
SET salary = 1;

-- rename column
ALTER TABLE emp RENAME COLUMN valid TO validity;

dump and restore:

ERROR: column "valid" of relation "emp" does not exist
LINE 2: ...ic.t DO INSTEAD UPDATE public.emp FOR PORTION OF valid FROM...

Yeah, you can demonstrate this bug without the dump-and-restore step:

postgres=# \d+ t
Table "public.t"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
x | integer | | | | plain | | |
Rules:
r AS
ON UPDATE TO t DO INSTEAD UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01' SET salary = 1
Access method: heap

postgres=# ALTER TABLE emp RENAME COLUMN valid TO validity;
ALTER TABLE
postgres=# \d+ t
Table "public.t"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
x | integer | | | | plain | | |
Rules:
r AS
ON UPDATE TO t DO INSTEAD UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01' SET salary = 1
Access method: heap

I didn't check the code, but it looks like someone thought it'd be a
good idea to record the FOR PORTION OF target column by name rather
than column number. That'll be an initdb-forcing catalog change, so
good thing we found it now.

regards, tom lane

On Jul 23, 2026, at 21:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:

John Naylor <johncnaylorls@gmail.com> writes:

Claude offered to search for other bugs of this kind and found this:

CREATE TABLE emp (id int, valid daterange, salary int);
CREATE TABLE t (x int);

CREATE RULE r AS ON UPDATE TO t DO INSTEAD
UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01'
SET salary = 1;

-- rename column
ALTER TABLE emp RENAME COLUMN valid TO validity;

dump and restore:

ERROR: column "valid" of relation "emp" does not exist
LINE 2: ...ic.t DO INSTEAD UPDATE public.emp FOR PORTION OF valid FROM...

Yeah, you can demonstrate this bug without the dump-and-restore step:

postgres=# \d+ t
Table "public.t"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
x | integer | | | | plain | | |
Rules:
r AS
ON UPDATE TO t DO INSTEAD UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01' SET salary = 1
Access method: heap

postgres=# ALTER TABLE emp RENAME COLUMN valid TO validity;
ALTER TABLE
postgres=# \d+ t
Table "public.t"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
x | integer | | | | plain | | |
Rules:
r AS
ON UPDATE TO t DO INSTEAD UPDATE emp FOR PORTION OF valid FROM '2020-01-01' TO '2021-01-01' SET salary = 1
Access method: heap

I didn't check the code, but it looks like someone thought it'd be a
good idea to record the FOR PORTION OF target column by name rather
than column number. That'll be an initdb-forcing catalog change, so
good thing we found it now.

regards, tom lane

If nobody works on this problem tonight, I can try to debug it tomorrow.

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

Chao Li <li.evan.chao@gmail.com> writes:

On Jul 23, 2026, at 21:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I didn't check the code, but it looks like someone thought it'd be a
good idea to record the FOR PORTION OF target column by name rather
than column number. That'll be an initdb-forcing catalog change, so
good thing we found it now.

If nobody works on this problem tonight, I can try to debug it tomorrow.

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.

It was Claude's idea to have 2 patches at all: it thought we should
avoid a post-beta2 catversion bump in v19. I might agree with it
except I see catversion was bumped just a few days ago in v19,
so there's no benefit in not doing so again.

regards, tom lane

On Thu, Jul 23, 2026 at 11:59 PM Tom Lane <tgl@sss.pgh.pa.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.

It was Claude's idea to have 2 patches at all: it thought we should
avoid a post-beta2 catversion bump in v19. I might agree with it
except I see catversion was bumped just a few days ago in v19,
so there's no benefit in not doing so again.

Yeah, since catversion was already bumped post-beta2 in commit
372b8d1ad, I see no reason why we can't bump it again here.

- Richard

On Jul 24, 2026, at 08:45, Richard Guo <guofenglinux@gmail.com> wrote:

On Thu, Jul 23, 2026 at 11:59 PM Tom Lane <tgl@sss.pgh.pa.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]Discussion: /messages/by-id/1734390.1768939652@sss.pgh.pa.us.

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: /messages/by-id/1734390.1768939652@sss.pgh.pa.us

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

Chao Li <li.evan.chao@gmail.com> writes:

On Jul 24, 2026, at 08:45, Richard Guo <guofenglinux@gmail.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