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