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. >>>>> >>>>
v3-0001-Fix-JSON_SERIALIZE-coercion-placeholder-type-for-.patch
Description: Binary data
