Re: [PATCH] Fix JSON_SERIALIZE() coercion placeholder type for jsonb input

Thanks Matt,

It all LGTM now. I think this patch can now be marked as "Ready For
Committer".

Regards,
Nikhil

On Thu, Aug 27, 2026 at 6:50 PM Matt Blewitt <mble(at)planetscale(dot)com> wrote:

> Hi Nikhil,
>
> Thanks for the follow-up review. Attached is v3 with the stricter argument
> assertion, explicit Node * cast, and implicit typmod-coercion test. I also
> rebased it onto current master and ran the full core regression suite with
> assertions enabled; all 243 tests passed.
>
> Cheers,
>
> Matt
>
> On Thu, Aug 27, 2026 at 10:45 AM Nikhil Sontakke <nikkhils(at)gmail(dot)com>
> wrote:
>
>> Hi Matt,
>>
>> Thanks for updating the patch. The simplified tests now cover the default
>> text and bytea coercion paths,
>> and the patch passes the full regression suite. Maybe the code can be
>> tightened further like the example below:
>>
>> @@ -3743,8 +3742,8 @@ makeJsonConstructorExpr(ParseState *pstate,
>> JsonConstructorType type,
>>
>> if (type == JSCTOR_JSON_SERIALIZE)
>> {
>> - Assert(args != NIL);
>> - cte->typeId = exprType(linitial(args));
>> + Assert(list_length(args) == 1);
>> + cte->typeId = exprType((Node *) linitial(args));
>> }
>> else
>>
>> Also, I think, we can expand the tests to consider the implicit typmod
>> conversion case as well:
>>
>> +-- Test implicit typmod coercion with jsonb input
>> +SELECT JSON_SERIALIZE('{ "a" : 1 } '::jsonb RETURNING varchar(2));
>>
>>
>> If no backpatch is intended (which I think is the case here),
>> then any concerns about previously stored expression trees does not apply
>> for this patch.
>>
>> Regards,
>> Nikhil
>>
>> On Wed, Aug 26, 2026 at 5:44 PM Matt Blewitt <mble(at)planetscale(dot)com>
>> wrote:
>>
>>> Hi Nikhil,
>>>
>>> Thanks - I've attached v2 with the revised test cases; there are no
>>> functional changes from v1.
>>>
>>> Cheers,
>>>
>>> Matt
>>>
>>> On Wed, Aug 26, 2026 at 9:07 AM Nikhil Sontakke <nikkhils(at)gmail(dot)com>
>>> wrote:
>>>
>>>> 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.
>>>>>>
>>>>>