[PATCH] Generalized JSON output functions
Hi, Hackers!
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks which can
be overridden the same way that is already provided for *parsing* JSON.
The motivation behind this to be able to produce specially-crafted JSON in
a logical replication output plugin, such that numeric (and bigint) values
are quoted. This requirement, in turn, arises from the fact that
JavaScript specification, which is quite natural to expect as a consumer
for this JSON data, allows to silently drop significant digits when
converting from string to number object.
I believe this is a well-known problem and I'm aware of a number of tricks
that might be used to avoid it, but none of them seems to be optimal from
my standpoint.
I can also imagine this can be used to convert date/time to string
differently, or adding indentation depending on the depth in object
hierarchy, etc.
What this patch does apart from providing callbacks, is abstracting most of
code for producing the correct JSON structure, which was previously
scattered and repeated in a number of functions with slight differences.
In the current code there are 5 styles for producing JSON object string,
differing in whitespace only:
a) no spaces
select to_json(row(1,2));
to_json
-----------------
{"f1":1,"f2":2}
b) some spaces (hstore_to_json)
select hstore(row(1,2))::json;
hstore
------------------------
{"f1": "1", "f2": "2"}
c) spaces around colon
select json_build_object('f1',1,'f2',2);
json_build_object
----------------------
{"f1" : 1, "f2" : 2}
d) spaces around colon *and* curly braces
select json_object_agg(x,x) from unnest('{1,2}'::int[]) x;
json_object_agg
----------------------
{ "1" : 1, "2" : 2 }
e) line feeds (row_to_json_pretty)
select row_to_json(row(1,2), true) as row;
row
----------
{"f1":1,+
"f2":2}
Personally, I think we should stick to (b), however that would break a lot
of test cases that already depend on (a). I've tried hard to minimize the
amount of changes in expected/json.out, but it is quickly becomes
cumbersome trying to support all of the above formats. So I've altered (c)
and (d) to look like (b), naturally only whitespace was affected.
There's one corner case I don't see a sensible way to support:
select json_agg(x) from generate_series(1,5) x;
json_agg
-----------------
[1, 2, 3, 4, 5]
With the patch applied it puts line feeds between the array elements
instead of spaces.
What also bothers me is that I've hard-coded output function oids for
cstring_out, and textout on assumption that they never change, but would
like to know that for sure.
Feedback is very much welcome!
--
Alex
PS: using a different email address this time, same Alex Shulgin. ;-)
PPS: sample code for mentioned use case with quoting numeric and bigint:
static void out_value_quote_numerics(JsonOutContext *out, Datum val,
JsonTypeCategory tcategory, Oid typoid, Oid
outfuncoid,
bool key_scalar) {
char *outputstr;
if (typoid == INT8OID || typoid == NUMERICOID) {
out->before_value(out);
outputstr = OidOutputFunctionCall(outfuncoid, val);
escape_json(&out->result, outputstr);
pfree(outputstr);
out->after_value(out, key_scalar);
} else {
json_out_value(out, val, tcategory, typoid, outfuncoid, key_scalar);
}
}
...
json_out_init_context(out, JSON_OUT_USE_SPACES);
out->value = out_value_quote_numerics;
Attachments:
json-output-generalized-v1.patchtext/x-patch; charset=US-ASCII; name=json-output-generalized-v1.patchDownload+638-614
On Wed, May 20, 2015 at 8:16 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
Hi, Hackers!
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks which can be
overridden the same way that is already provided for *parsing* JSON.The motivation behind this to be able to produce specially-crafted JSON in a
logical replication output plugin, such that numeric (and bigint) values are
quoted. This requirement, in turn, arises from the fact that JavaScript
specification, which is quite natural to expect as a consumer for this JSON
data, allows to silently drop significant digits when converting from string
to number object.I believe this is a well-known problem and I'm aware of a number of tricks
that might be used to avoid it, but none of them seems to be optimal from my
standpoint.I can also imagine this can be used to convert date/time to string
differently, or adding indentation depending on the depth in object
hierarchy, etc.What this patch does apart from providing callbacks, is abstracting most of
code for producing the correct JSON structure, which was previously
scattered and repeated in a number of functions with slight differences. In
the current code there are 5 styles for producing JSON object string,
differing in whitespace only:a) no spaces
select to_json(row(1,2));
to_json
-----------------
{"f1":1,"f2":2}b) some spaces (hstore_to_json)
select hstore(row(1,2))::json;
hstore
------------------------
{"f1": "1", "f2": "2"}c) spaces around colon
select json_build_object('f1',1,'f2',2);
json_build_object
----------------------
{"f1" : 1, "f2" : 2}d) spaces around colon *and* curly braces
select json_object_agg(x,x) from unnest('{1,2}'::int[]) x;
json_object_agg
----------------------
{ "1" : 1, "2" : 2 }e) line feeds (row_to_json_pretty)
select row_to_json(row(1,2), true) as row;
row
----------
{"f1":1,+
"f2":2}Personally, I think we should stick to (b), however that would break a lot
of test cases that already depend on (a). I've tried hard to minimize the
amount of changes in expected/json.out, but it is quickly becomes cumbersome
trying to support all of the above formats. So I've altered (c) and (d) to
look like (b), naturally only whitespace was affected.
Disagree. IMNSHO, the default should be (a), as it's the most compact
format and therefore the fastest. Whitespace injection should be
reserved for prettification functions.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/20/2015 09:16 AM, Shulgin, Oleksandr wrote:
Hi, Hackers!
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks which
can be overridden the same way that is already provided for *parsing*
JSON.The motivation behind this to be able to produce specially-crafted
JSON in a logical replication output plugin, such that numeric (and
bigint) values are quoted. This requirement, in turn, arises from the
fact that JavaScript specification, which is quite natural to expect
as a consumer for this JSON data, allows to silently drop significant
digits when converting from string to number object.I believe this is a well-known problem and I'm aware of a number of
tricks that might be used to avoid it, but none of them seems to be
optimal from my standpoint.I can also imagine this can be used to convert date/time to string
differently, or adding indentation depending on the depth in object
hierarchy, etc.
I'm not necessarily opposed to this, but it sure seems like a lot of
changes, and moderately invasive ones, to support something that could
be done, at the cost of reparsing, with a simple loadable extension that
I could create in a few hours of programming. The parser API was created
precisely to make this sort of transformation close to trivial. Other
fairly obvious transformations include translating to XML or YAML, and a
less obvious one could be something very specialized, like translating
certain fields. Anyway, for this purpose I could imagine a function like:
json_format (
j json (or text),
indent_spaces smallint default 0,
space_after_colon boolean default false,
space_after_comma boolean default false,
quote_numerics boolean default false)
returns json
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 21, 2015 at 6:43 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
On Thu, May 21, 2015 at 2:23 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:On Wed, May 20, 2015 at 4:06 PM, Merlin Moncure <mmoncure@gmail.com>
wrote:
On Wed, May 20, 2015 at 8:16 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:a) no spaces
select to_json(row(1,2));
to_json
-----------------
{"f1":1,"f2":2}b) some spaces (hstore_to_json)
select hstore(row(1,2))::json;
hstore
------------------------
{"f1": "1", "f2": "2"}c) spaces around colon
select json_build_object('f1',1,'f2',2);
json_build_object
----------------------
{"f1" : 1, "f2" : 2}d) spaces around colon *and* curly braces
select json_object_agg(x,x) from unnest('{1,2}'::int[]) x;
json_object_agg
----------------------
{ "1" : 1, "2" : 2 }e) line feeds (row_to_json_pretty)
select row_to_json(row(1,2), true) as row;
row
----------
{"f1":1,+
"f2":2}Personally, I think we should stick to (b), however that would break a
lot
of test cases that already depend on (a). I've tried hard to minimize
the
amount of changes in expected/json.out, but it is quickly becomes
cumbersome
trying to support all of the above formats. So I've altered (c) and(d)
to
look like (b), naturally only whitespace was affected.Disagree. IMNSHO, the default should be (a), as it's the most compact
format and therefore the fastest. Whitespace injection should be
reserved for prettification functions.I have no strong opinion on choosing (a) over (b), just wanted to make
the
change minimally sensible. If at all, I think we should modify existing
code to make JSON output consistent: that is choose one format an stickto
it.
sure -- did you mean to respond off-list?
No, just using an unusual mail agent :-p
anyways, inserting spacing
into the serialization function formatting (xx_to_json) for me will
raise memory profile of output json by 10%+ in nearly all cases. I
just don't see the benefit of doing that given that the json is still
not very 'pretty'.
I can agree that spaces are only useful for a human being trying to make
sense of the data. My vote is for reasonable default + an option to put
spaces/prettify on demand.
--
Alex
Import Notes
Reply to msg id not found: CAHyXU0zgMvkj0DO2+FjRSuotq2XZdNRs2k785Oupoiq8ETYatA@mail.gmail.com
Andrew Dunstan wrote:
On 05/20/2015 09:16 AM, Shulgin, Oleksandr wrote:
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks which can
be overridden the same way that is already provided for *parsing* JSON.
I'm not necessarily opposed to this, but it sure seems like a lot of
changes, and moderately invasive ones, to support something that could be
done, at the cost of reparsing, with a simple loadable extension that I
could create in a few hours of programming.
But this seems like a pretty reasonable change to make, no? Doesn't the
total amount of code decrease after this patch? JSON stuff is pretty
new so some refactoring and generalization of what we have is to be
expected.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 22, 2015 at 9:43 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Andrew Dunstan wrote:
On 05/20/2015 09:16 AM, Shulgin, Oleksandr wrote:
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks which can
be overridden the same way that is already provided for *parsing* JSON.I'm not necessarily opposed to this, but it sure seems like a lot of
changes, and moderately invasive ones, to support something that could be
done, at the cost of reparsing, with a simple loadable extension that I
could create in a few hours of programming.But this seems like a pretty reasonable change to make, no? Doesn't the
total amount of code decrease after this patch? JSON stuff is pretty
new so some refactoring and generalization of what we have is to be
expected.
Yeah. Also, there have been a few previous gripes about this, for
example, /messages/by-id/CAHbVmPzS+sVR+y-UgxjRq+XW4dqteVL-cOzc69zFFwmxjcKCxg@mail.gmail.com.
As noted, I definitely prefer 'space free' by default for efficiency
reasons, but standardizing the output has definitely got to be a
reasonable goal.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 22, 2015 at 10:51 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
On Fri, May 22, 2015 at 9:43 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Andrew Dunstan wrote:
On 05/20/2015 09:16 AM, Shulgin, Oleksandr wrote:
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks whichcan
be overridden the same way that is already provided for *parsing* JSON.
I'm not necessarily opposed to this, but it sure seems like a lot of
changes, and moderately invasive ones, to support something that couldbe
done, at the cost of reparsing, with a simple loadable extension that I
could create in a few hours of programming.But this seems like a pretty reasonable change to make, no? Doesn't the
total amount of code decrease after this patch? JSON stuff is pretty
new so some refactoring and generalization of what we have is to be
expected.Yeah. Also, there have been a few previous gripes about this, for
example,
/messages/by-id/CAHbVmPzS+sVR+y-UgxjRq+XW4dqteVL-cOzc69zFFwmxjcKCxg@mail.gmail.com
.
As noted, I definitely prefer 'space free' by default for efficiency
reasons, but standardizing the output has definitely got to be a
reasonable goal.
Every JSON implementation I have ever used defaults to the minified version
of JSON (no whitespace) when printed.
On Sat, May 23, 2015 at 3:03 AM, Ryan Pedela <rpedela@datalanche.com> wrote:
On Fri, May 22, 2015 at 10:51 AM, Merlin Moncure <mmoncure@gmail.com>
wrote:On Fri, May 22, 2015 at 9:43 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Andrew Dunstan wrote:
On 05/20/2015 09:16 AM, Shulgin, Oleksandr wrote:
Attached is a patch against master to generalize the JSON-producing
functions in utils/adt/json.c and to provide a set of callbacks whichcan
be overridden the same way that is already provided for *parsing*
JSON.
I'm not necessarily opposed to this, but it sure seems like a lot of
changes, and moderately invasive ones, to support something that couldbe
done, at the cost of reparsing, with a simple loadable extension that I
could create in a few hours of programming.But this seems like a pretty reasonable change to make, no? Doesn't the
total amount of code decrease after this patch? JSON stuff is pretty
new so some refactoring and generalization of what we have is to be
expected.Yeah. Also, there have been a few previous gripes about this, for
example,
/messages/by-id/CAHbVmPzS+sVR+y-UgxjRq+XW4dqteVL-cOzc69zFFwmxjcKCxg@mail.gmail.com
.
As noted, I definitely prefer 'space free' by default for efficiency
reasons, but standardizing the output has definitely got to be a
reasonable goal.Every JSON implementation I have ever used defaults to the minified
version of JSON (no whitespace) when printed.
Hashing of arrays seems to be an important issue: we'd rather make sure to
produce the same output in every code path. That would also mean: no
special logic to add the line feeds in json_agg either.
Is it reasonable to add this patch to CommitFest now?
--
Alex
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/27/2015 02:37 PM, Robert Haas wrote:
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.
I'm not dead set against it either. When I have time I will take a
closer look.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/27/2015 09:51 PM, Andrew Dunstan wrote:
On 05/27/2015 02:37 PM, Robert Haas wrote:
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.I'm not dead set against it either. When I have time I will take a
closer look.
Andrew, will you have the time to review this? Please add yourself as
reviewer in the commitfest app if you do.
My 2 cents is that I agree with your initial reaction: This is a lot of
infrastructure and generalizing things, for little benefit. Let's change
the current code where we generate JSON to be consistent with
whitespace, and call it a day.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/03/2015 06:27 AM, Heikki Linnakangas wrote:
On 05/27/2015 09:51 PM, Andrew Dunstan wrote:
On 05/27/2015 02:37 PM, Robert Haas wrote:
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.I'm not dead set against it either. When I have time I will take a
closer look.Andrew, will you have the time to review this? Please add yourself as
reviewer in the commitfest app if you do.My 2 cents is that I agree with your initial reaction: This is a lot
of infrastructure and generalizing things, for little benefit. Let's
change the current code where we generate JSON to be consistent with
whitespace, and call it a day.- Heikki
I'm somewhat on vacation for the next week or so, so I won't claim it,
but I'll try to make time to look at it. Other people (Merlin?) could
also provide reviews.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2015-07-03 12:27 GMT+02:00 Heikki Linnakangas <hlinnaka@iki.fi>:
On 05/27/2015 09:51 PM, Andrew Dunstan wrote:
On 05/27/2015 02:37 PM, Robert Haas wrote:
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.I'm not dead set against it either. When I have time I will take a
closer look.Andrew, will you have the time to review this? Please add yourself as
reviewer in the commitfest app if you do.My 2 cents is that I agree with your initial reaction: This is a lot of
infrastructure and generalizing things, for little benefit. Let's change
the current code where we generate JSON to be consistent with whitespace,
and call it a day.
I am thinking so it is not bad idea. This code can enforce uniform format,
and it can check if produced value is correct. It can be used in our code,
it can be used by extension's developers.
This patch is not small, but really new lines are not too much.
I'll do review today.
Regards
Pavel
Show quoted text
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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, ....)
? Is it necessary?
3. if it should be used everywhere, then in EXPLAIN statement too.
Regards
Pavel
2015-07-10 6:31 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
Show quoted text
2015-07-03 12:27 GMT+02:00 Heikki Linnakangas <hlinnaka@iki.fi>:
On 05/27/2015 09:51 PM, Andrew Dunstan wrote:
On 05/27/2015 02:37 PM, Robert Haas wrote:
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.I'm not dead set against it either. When I have time I will take a
closer look.Andrew, will you have the time to review this? Please add yourself as
reviewer in the commitfest app if you do.My 2 cents is that I agree with your initial reaction: This is a lot of
infrastructure and generalizing things, for little benefit. Let's change
the current code where we generate JSON to be consistent with whitespace,
and call it a day.I am thinking so it is not bad idea. This code can enforce uniform
format, and it can check if produced value is correct. It can be used in
our code, it can be used by extension's developers.This patch is not small, but really new lines are not too much.
I'll do review today.
Regards
Pavel
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
forgotten attachment
Regards
Pavel
2015-07-10 14:34 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
Show quoted text
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 problemsMy objections:
1. comments - missing comment for some basic API, basic fields like
"key_scalar" and similar
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, ....)
? Is it necessary?
3. if it should be used everywhere, then in EXPLAIN statement too.
Regards
Pavel
2015-07-10 6:31 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
2015-07-03 12:27 GMT+02:00 Heikki Linnakangas <hlinnaka@iki.fi>:
On 05/27/2015 09:51 PM, Andrew Dunstan wrote:
On 05/27/2015 02:37 PM, Robert Haas wrote:
On Tue, May 26, 2015 at 2:50 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:Is it reasonable to add this patch to CommitFest now?
It's always reasonable to add a patch to the CommitFest if you would
like for it to be reviewed and avoid having it get forgotten about.
There seems to be some disagreement about whether we want this, but
don't let that stop you from adding it to the next CommitFest.I'm not dead set against it either. When I have time I will take a
closer look.Andrew, will you have the time to review this? Please add yourself as
reviewer in the commitfest app if you do.My 2 cents is that I agree with your initial reaction: This is a lot of
infrastructure and generalizing things, for little benefit. Let's change
the current code where we generate JSON to be consistent with whitespace,
and call it a day.I am thinking so it is not bad idea. This code can enforce uniform
format, and it can check if produced value is correct. It can be used in
our code, it can be used by extension's developers.This patch is not small, but really new lines are not too much.
I'll do review today.
Regards
Pavel
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
json-output-generalized-v1-2.patchtext/x-patch; charset=US-ASCII; name=json-output-generalized-v1-2.patchDownload+638-614
2015-07-10 14:34 GMT+02:00 Pavel Stehule <pavel.stehule@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 problemsMy 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.
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
2015-07-10 15:57 GMT+02:00 Shulgin, Oleksandr <oleksandr.shulgin@zalando.de>
:
2015-07-10 14:34 GMT+02:00 Pavel Stehule <pavel.stehule@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 problemsMy objections:
1. comments - missing comment for some basic API, basic fields like
"key_scalar" and similarI 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?
Show quoted text
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
On Fri, Jul 10, 2015 at 4:04 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:
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?
Well, one could call it premature pessimization due to dynamic call
overhead.
IMO, the fact that json_out_init_context() sets the value callback to
json_out_value is an implementation detail, the other parts of code should
not rely on. And for the Explain output, there definitely going to be
*some* code between context initialization and output callbacks: these are
done in a number of different functions.
--
Alex
2015-07-10 16:16 GMT+02:00 Shulgin, Oleksandr <oleksandr.shulgin@zalando.de>
:
On Fri, Jul 10, 2015 at 4:04 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote: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?Well, one could call it premature pessimization due to dynamic call
overhead.IMO, the fact that json_out_init_context() sets the value callback to
json_out_value is an implementation detail, the other parts of code should
not rely on. And for the Explain output, there definitely going to be
*some* code between context initialization and output callbacks: these are
done in a number of different functions.
Again - it is necessary? Postgres still use modular code, not OOP code. I
can understand the using of this technique, when I need a possibility to
change behave. But these function are used for printing JSON, not printing
any others.
Pavel
Show quoted text
--
Alex
On Fri, Jul 10, 2015 at 5:16 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:
Well, one could call it premature pessimization due to dynamic call
overhead.IMO, the fact that json_out_init_context() sets the value callback to
json_out_value is an implementation detail, the other parts of code should
not rely on. And for the Explain output, there definitely going to be
*some* code between context initialization and output callbacks: these are
done in a number of different functions.Again - it is necessary? Postgres still use modular code, not OOP code. I
can understand the using of this technique, when I need a possibility to
change behave. But these function are used for printing JSON, not printing
any others.
No, it's not strictly necessary.
For me it's not about procedural- vs. object- style, but rather about being
able to override/extend the behavior consistently. And for that I would
prefer that if I override the value callback in a JSON output context, that
it would be called for every value being printed, not only for some of them.
Thank you for pointing out the case of Explain format, I've totally
overlooked it in my first version. Trying to apply the proposed approach
in the explain printing code led me to reorganize things slightly. I've
added explicit functions for printing keys vs. values, thus no need to
expose that key_scalar param anymore. There are now separate before/after
key and before/after value functions as well, but I believe it makes for a
cleaner code.
The most of the complexity is still in the code that decides whether or not
to put spaces (between the values or for indentation) and newlines at
certain points. Should we decide to unify the style we emit ourselves,
this could be simplified, while still leaving room for great flexibility if
overridden by an extension, for example.
Have a nice weekend.
--
Alex