| From: | Nikhil Sontakke <nikkhils(at)gmail(dot)com> |
|---|---|
| To: | Matt Blewitt <mble(at)planetscale(dot)com> |
| Cc: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Fix JSON_SERIALIZE() coercion placeholder type for jsonb input |
| Date: | 2026-08-26 08:07:08 |
| Message-ID: | CANgU5ZdYMuWx1ew96ObKtNNnFh9CJJFLJ5TGi2wH3Qo2dWQGsQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Matt,
I reviewed the patch and tested it against current master.
I built the patch with assertions enabled and ran the full core regression
suite. All 245 tests passed. I also tested:
- typed jsonb parameters in prepared statements
- default text, varchar, bytea, and string-domain results
- NULL jsonb input
- use inside a view and subsequent deparsing
These all behaved as expected. I did not find any security, WAL/recovery,
logical-replication, or performance concerns introduced by the patch.
I have one comment:
Regression tests
To me, some of the additional coverage seems redundant:
- The RETURNING int and RETURNING jsonb cases are rejected before
reaching the changed code.
- The RETURNING jsonb error is already tested immediately above with a
non-jsonb input.
- The new EXPLAIN cases verify deparsing but do not reveal which
coercion function was selected.
So, I think the test addition could be reduced to one default text case and
one RETURNING bytea case.
Otherwise, the implementation looks correct. It's simpler than converting
every jsonb input to json every time.
Regards,
Nikhil
On Tue, Mar 17, 2026 at 12:18 AM Matt Blewitt <mble(at)planetscale(dot)com> wrote:
> Hi Zsolt,
>
> Thanks for testing that out and confirming it looks good against master.
> Submitted to commitfest for further review and integration consideration.
>
> Matt
>
> On Fri, Mar 13, 2026 at 11:20 PM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>
> wrote:
>
>> Hello!
>>
>> This is a simple fix and it does what it says, it looks good to me.
>>
>> I did test it with a few more queries and compared it against master,
>> all looks good.
>>
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | vignesh C | 2026-08-26 08:22:39 | Re: Proposal: Conflict log history table for Logical Replication |
| Previous Message | Tender Wang | 2026-08-26 08:04:07 | Re: More partition pruning bugs with multi-column RANGE partitions |