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

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