Hi, I tested the v3 patch for the JSON_SERIALIZE() issue with jsonb input. Before applying the patch, I was able to reproduce the issue, where UTF-8 conversion failed due to invalid byte sequences. After applying the patch, the output was correct, and the UTF-8 conversion succeeded. I also ran the regression tests, and all 245 tests passed. The build completed successfully, and git diff --check reported no issues. Please let me know if any further testing is needed.
Regards, solai On Mon, Oct 26, 2026 at 5:32 PM Nikhil Sontakke <[email protected]> wrote: > > 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.
