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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
>> 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 <
>>> [email protected]> 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.
>>>>
>>>
