On Sat, Nov 8, 2014 at 12:45 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> Here are more thoughts among those lines looking at the current state of
>> the patch 4 that introduces the infrastructure of the whole feature. This
>> would make possible in-memory manipulation of jsonb containers without
>> relying on a 3rd-part set of APIs like what this patch is doing with
>> ObjTree to deparse the DDL parse trees.
>
> Thanks for the thoughts. I have to say that I have no intention of
> reworking the jsonb code. If somebody else wants to do the legwork and
> add that API as you suggest, I'm happy to remove all the ObjTree stuff
> from this patch. I don't expect this to happen too soon, though, so I
> would instead consider committing this patch based on ObjTree. Later,
> when somebody comes around to reworking jsonb, we can rip ObjTree out.
The thing freaking me out in this case is when would that really
happen? Maybe years from now, and perhaps at that point we would
regret to not have put in place the infrastructure that we knew we
could have done.
>> This infrastructure would allow in-memory manipulation of jsonb containers.
>> Containers that can then be easily be manipulated to be changed back to
>> strings and for value lookups using key strings.
>
> Honestly, I had hoped that the jsonb code would have already included
> this kind of functionality. I wasn't too happy when I discovered that I
> needed to keep the ObjTree crap. But I don't want to do that work
> myself.
I can't blame you for that.
In any case, if this code goes in as-is (I am against it at this point
but I am just giving my opinion as a reviewer here), I think that at
least the following things could be done with a minimal effort:
- Provide the set of constructor functions for JsonbValue
- Move the jsonb APIs (find_*_in_jsonbcontainer) doing value lookups
using keys from ddl_json.c to jsonb_util.c, rename them to something
more consistent and complete the set for the other object types.
- Add a TODO item in the wiki, and a TODO item in where ObjTree is defined.
Thanks,
--
Michael