2015-07-10 15:57 GMT+02:00 Shulgin, Oleksandr <oleksandr.shul...@zalando.de> :
> 2015-07-10 14:34 GMT+02:00 Pavel Stehule <pavel.steh...@gmail.com>: >> >>> Hi >>> >>> I am sending review of this patch: >>> >>> 1. I reread a previous discussion and almost all are for this patch (me >>> too) >>> >>> 2. I have to fix a typo in hstore_io.c function (update attached), other >>> (patching, regress tests) without problems >>> >>> My objections: >>> >>> 1. comments - missing comment for some basic API, basic fields like >>> "key_scalar" and similar >>> >> > I thought it was pretty obvious from the code, because it's sort of the > only source for docs on the subject right now. Should we add proper > documentation section, this would have been documented for sure. > > >> 2. why you did indirect call via JsonOutContext? >>> >>> What is benefit >>> >>> dst.value(&dst, (Datum) 0, JSONTYPE_NULL, InvalidOid, InvalidOid, false); >>> >>> instead >>> >>> json_out_value(&dst, ....) >>> >> > For consistency. Even though we initialize the output context ourselves, > there might be some code introduced between json_out_init_context() and > dst.value() calls that replaces some of the callbacks, and then there would > be a difference. > with this consistency? I didn't see this style everywhere in Postgres? Isn't it premature optimization? > >> 3. if it should be used everywhere, then in EXPLAIN statement too. >>> >> > Ahh.. good catch. I'll have a look on this now. > > Thanks for the review! > > -- > Alex > >
