patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Started by Joseph Adamsover 15 years ago34 messageshackers
Jump to latest
#1Joseph Adams
joeyadams3.14159@gmail.com

This is a work-in-progress patch of my GSoC project: Add JSON datatype
to PostgreSQL. It provides the following:

* JSON datatype: A TEXT-like datatype for holding JSON-formatted
text. Although the JSON RFC decrees that a JSON text be an "object or
array" (meaning '"hello"' is considered invalid JSON text), this
datatype lets you store any JSON "value" (meaning '"hello"'::JSON is
allowed).
* Validation: Content is validated when a JSON datum is constructed,
but JSON validation can also be done programmatically with the
json_validate() function.
* Conversion to/from JSON for basic types. Conversion functions are
needed because casting will not unwrap JSON-encoded values. For
instance, json('"string"')::text is '"string"', while
from_json('"string"') is 'string'. Also, to_json can convert
PostgreSQL arrays to JSON arrays, providing a nice option for dealing
with arrays client-side. from_json currently can't handle JSON
arrays/objects yet (how they should act is rather unclear to me,
except when array dimensions and element type are consistent).
* Retrieving/setting values in a JSON node (via selectors very
similar to, but not 100% like, JSONPath as described at
http://goessner.net/articles/JsonPath/ ).
* Miscellaneous functions json_condense and json_type.

This is a patch against CVS HEAD. This module compiles, installs, and
passes all 8 tests successfully on my Ubuntu 9.10 system. It is
covered pretty decently with regression tests. It also has SGML
documentation (the generated HTML is attached for convenience).

Although I am aware of many problems in this patch, I'd like to put it
out sooner rather than later so it can get plenty of peer review.
Problems I'm aware of include:
* Probably won't work properly when the encoding (client or server?)
is not UTF-8. When encoding (e.g. with json_condense), it should (but
doesn't) use \uXXXX escapes for characters the target encoding doesn't
support.
* json.c is rather autarkic. It has its own string buffer system
(rather than using StringInfo) and UTF-8 validator (rather than using
pg_verify_mbstr_len(?) ).
* Some functions/structures are named suggestively, as if they belong
to (and would be nice to have in) PostgreSQL's utility libraries.
They are:
- TypeInfo, initTypeInfo, and getTypeInfo: A less cumbersome
wrapper around get_type_io_data.
- FN_EXTRA and FN_EXTRA_SZ: Macros to make working with
fcinfo->flinfo->fn_extra easier.
- enumLabelToOid: Look up the Oid of an enum label; needed to
return an enum that isn't built-in.
- utf8_substring: Extract a range of UTF-8 characters out of a UTF-8 string.
* Capitalization and function arrangement are rather inconsistent.
Braces are K&R-style.
* json_cleanup and company aren't even used.
* The sql/json.sql test case should be broken into more files.

P.S. The patch is gzipped because it expands to 2.6 megabytes.

Joey Adams

Attachments:

json.htmltext/html; charset=US-ASCII; name=json.htmlDownload
json-datatype-wip-01.diff.gzapplication/x-gzip; name=json-datatype-wip-01.diff.gzDownload
#2Robert Haas
robertmhaas@gmail.com
In reply to: Joseph Adams (#1)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Fri, Jul 23, 2010 at 2:18 AM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:

This is a work-in-progress patch of my GSoC project: Add JSON datatype
to PostgreSQL.  It provides the following:

 * JSON datatype: A TEXT-like datatype for holding JSON-formatted
text.  Although the JSON RFC decrees that a JSON text be an "object or
array" (meaning '"hello"' is considered invalid JSON text), this
datatype lets you store any JSON "value" (meaning '"hello"'::JSON is
allowed).
 * Validation: Content is validated when a JSON datum is constructed,
but JSON validation can also be done programmatically with the
json_validate() function.
 * Conversion to/from JSON for basic types.  Conversion functions are
needed because casting will not unwrap JSON-encoded values.  For
instance, json('"string"')::text is '"string"', while
from_json('"string"') is 'string'.  Also, to_json can convert
PostgreSQL arrays to JSON arrays, providing a nice option for dealing
with arrays client-side.  from_json currently can't handle JSON
arrays/objects yet (how they should act is rather unclear to me,
except when array dimensions and element type are consistent).
 * Retrieving/setting values in a JSON node (via selectors very
similar to, but not 100% like, JSONPath as described at
http://goessner.net/articles/JsonPath/ ).
 * Miscellaneous functions json_condense and json_type.

This is a patch against CVS HEAD.  This module compiles, installs, and
passes all 8 tests successfully on my Ubuntu 9.10 system.  It is
covered pretty decently with regression tests.  It also has SGML
documentation (the generated HTML is attached for convenience).

Although I am aware of many problems in this patch, I'd like to put it
out sooner rather than later so it can get plenty of peer review.
Problems I'm aware of include:
 * Probably won't work properly when the encoding (client or server?)
is not UTF-8.  When encoding (e.g. with json_condense), it should (but
doesn't) use \uXXXX escapes for characters the target encoding doesn't
support.
 * json.c is rather autarkic.  It has its own string buffer system
(rather than using StringInfo) and UTF-8 validator (rather than using
pg_verify_mbstr_len(?) ).
 * Some functions/structures are named suggestively, as if they belong
to (and would be nice to have in) PostgreSQL's utility libraries.
They are:
  - TypeInfo, initTypeInfo, and getTypeInfo: A less cumbersome
wrapper around get_type_io_data.
  - FN_EXTRA and FN_EXTRA_SZ: Macros to make working with
fcinfo->flinfo->fn_extra easier.
  - enumLabelToOid: Look up the Oid of an enum label; needed to
return an enum that isn't built-in.
  - utf8_substring: Extract a range of UTF-8 characters out of a UTF-8 string.
 * Capitalization and function arrangement are rather inconsistent.
Braces are K&R-style.
 * json_cleanup and company aren't even used.
 * The sql/json.sql test case should be broken into more files.

P.S. The patch is gzipped because it expands to 2.6 megabytes.

Some technical points about the submission:

- If you want your coded to be included in PostgreSQL, you need to put
the same license and attribution on it that we use elsewhere.
Generally that looks sorta like this:

/*-------------------------------------------------------------------------
*
* tablecmds.c
* Commands for creating and altering table structures and settings
*
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
*
* IDENTIFICATION
* $PostgreSQL$
*
*-------------------------------------------------------------------------
*/

You should have this header on each file, both .c and .h.

- The reason why this patch is 2.6MB is because it has 2.4MB of tests.
I think you should probably pick out the most useful 10% or so, and
drop the rest.

- I was under the impression that we wanted EXPLAIN (FORMAT JSON) to
return type json, but that's obviously not going to be possible if all
of this is contrib. We could (a) move it all into core, (b) move the
type itself and its input and output functions into core and leave the
rest in contrib [or something along those lines], or (c) give up using
it as the return type for EXPLAIN (FORMAT JSON).

- You need to comply with the project coding standards. Thus:

static void
foo()
{
exit(1);
}

Not:

static void foo()
{
exit(1);
}

You should have at least one blank line between every pair of
functions. You should use uncuddled curly braces. Your commenting
style doesn't match the project standard. We prefer explicit NULL
tests over if (!foo). Basically, you should run pgindent on your
code, and also read this:

http://www.postgresql.org/docs/8.4/static/source.html

I don't think this is going to fly either:

/* Declare and initialize a String with the given name. */
#define String(name) String name = NewString()
#define NewString() {{NULL, 0, 0}}

That's just too much magic. We want people to be able to read this
code and easily tell what's going on... spelling a few things out
long hand is OK, good, even.

- You have boatloads of functions in here with no comments whose
functions is entirely non-self-evident. You may or may not need to
rename some of them, but you definitely need to write some comments.

- elog() must be used except for "can't happen" situations. Compare
the way numeric_in() throws an error message versus json_in().

- You have a number of very short convenience functions which don't
seem warranted. For example, build_array_string() is a 2-line
function that is called once. And all the string_append,
string_append_range, string_append_char stuff is duplicating
functionality we already have in appendStringInfoStr,
appendBinaryStringInfo, appendStringInfoChar.

In short, my first impression of this patch is that there's a lot of
good stuff in here, but you need to stop adding features NOW and put A
LOT of work into getting this into a form that the community can
accept. Otherwise, this is likely going to die on the vine, which
would be a shame because a lot of it seems pretty cool.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

#3Robert Haas
robertmhaas@gmail.com
In reply to: Robert Haas (#2)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Fri, Jul 23, 2010 at 2:34 PM, Robert Haas <robertmhaas@gmail.com> wrote:

- elog() must be used except for "can't happen" situations.  Compare
the way numeric_in() throws an error message versus json_in().

Er... that should have said "elog() must NOT be used except for can't
happen situations".

Also, I was just looking at json_delete(). While the existing coding
there is kind of fun, I think this can be written more
straightforwardly by doing something like this (not tested):

while (1)
{
while (is_internal(node) && node->v.children.head)
node = node->v.children.head;
if (node->next)
next = node->next;
else if (node->parent)
next = node->parent;
else
break;
free_node(node);
node = next;
}

That gets rid of all of the gotos and one of the local variables and,
at least IMO, is easier to understand... though it would be even
better still if you also added a comment saying something like "We do
a depth-first, left-to-right traversal of the tree, freeing nodes as
we go. We need not bother clearing any of the pointers, because the
traversal order is such that we're never in danger of revisiting a
node we've already freed."

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

#4Joseph Adams
joeyadams3.14159@gmail.com
In reply to: Robert Haas (#2)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Update: I'm in the middle of cleaning up the JSON code (
http://git.postgresql.org/gitweb?p=json-datatype.git;a=summary if you
want to see the very latest ), so I haven't addressed all of the major
problems with it yet.

On Fri, Jul 23, 2010 at 2:34 PM, Robert Haas <robertmhaas@gmail.com> wrote:

- I was under the impression that we wanted EXPLAIN (FORMAT JSON) to
return type json, but that's obviously not going to be possible if all
of this is contrib.  We could (a) move it all into core, (b) move the
type itself and its input and output functions into core and leave the
rest in contrib [or something along those lines], or (c) give up using
it as the return type for EXPLAIN (FORMAT JSON).

I've been developing it as a contrib module because:
* I'd imagine it's easier than developing it as a built-in datatype
right away (e.g. editing a .sql.in file versus editing pg_type.h ).
* As a module, it has PGXS support, so people can try it out right
away rather than having to recompile PostgreSQL.

I, for one, think it would be great if the JSON datatype were all in
core :-) However, if and how much JSON code should go into core is up
for discussion. Thoughts, anyone?

A particularly useful aspect of the JSON support is the ability to
convert PostgreSQL arrays to JSON arrays (using to_json ), as there
currently isn't any streamlined way to parse arrays in the PostgreSQL
format client-side (that I know of).

Joey Adams

#5Andres Freund
andres@anarazel.de
In reply to: Joseph Adams (#4)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Sat, Jul 24, 2010 at 06:57:18PM -0400, Joseph Adams wrote:

A particularly useful aspect of the JSON support is the ability to
convert PostgreSQL arrays to JSON arrays (using to_json ), as there
currently isn't any streamlined way to parse arrays in the PostgreSQL
format client-side (that I know of).

I really would like to address the latter issue some day. I don't know
how many broken implementations I have seen in my, not that long, time
using pg, but it sure is 10+. I also knowingly have written dumbed
down versions.

Andres

In reply to: Joseph Adams (#4)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

I, for one, think it would be great if the JSON datatype were all in
core :-)  However, if and how much JSON code should go into core >is up for discussion.  Thoughts, anyone?

in my opinion: As soon as possible. Spinning PostgreSQL as the
Ajax-enabled-database has many great uses.

Harald

--
GHUM Harald Massa
persuadere et programmare
Harald Armin Massa
Spielberger Straße 49
70435 Stuttgart
0173/9409607
no fx, no carrier pigeon
-
Using PostgreSQL is mostly about sleeping well at night.

#7Joseph Adams
joeyadams3.14159@gmail.com
In reply to: Joseph Adams (#1)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Updated JSON datatype patch. It cleans up the major problems that
have been discussed, and it's very close to being commit-worthy (I
think). The major issues as I see them are:

* Contains several utility functions that may be useful in general.
They are all in util.c / util.h
* It's still a contrib module
* No json_agg or json_object functions for constructing arrays / objects

The utility functions and their potential to collide with themselves
in the future is the main problem with this patch. Of course, this
problem could be sidestepped simply by namespacifying them (prepending
json_ to all the function names). I would like some thoughts and
opinions about the design and usefulness of the utility code.

An overview, along with my thoughts, of the utility functions:

FN_EXTRA, FN_EXTRA_ALLOC, FN_MCXT macros
* Useful-ometer: ()--------------------o
* Rationale: Using fcinfo->flinfo->fn_extra takes a lot of
boilerplate. These macros help cut down the boilerplate, and the
comment explains what fn_extra is all about.

TypeInfo structure and getTypeInfo function
* Useful-ometer: ()---------------------------o
* Rationale: The get_type_io_data "six-fer" function is very
cumbersome to use, since one has to declare all the output variables.
The getTypeInfo puts the results in a structure. It also performs the
fmgr_info_cxt step, which is a step done after every usage of
get_type_io_data in the PostgreSQL code.

getEnumLabelOids
* Useful-ometer: ()-----------------------------------o
* Rationale: There is currently no streamlined way to return a custom
enum value from a PostgreSQL function written in C. This function
performs a batch lookup of enum OIDs, which can then be cached with
fn_extra. This should be reasonably efficient, and it's quite elegant
to use (see json_op.c for an example).

UTF-8 functions:
utf8_substring
utf8_decode_char
(there's a patch in the works for a utf8_to_unicode function
which does the same thing as this function)
utf8_validate (variant of pg_verify_mbstr(PG_UTF8, str, length, true)
that allows '\0' characters)
server_to_utf8
utf8_to_server
text_to_utf8_cstring
utf8_cstring_to_text
utf8_cstring_to_text_with_len
* Useful-ometer: ()-------o
* Rationale: The JSON code primarily operates in UTF-8 rather than
the server encoding because it needs to deal with Unicode escapes, and
there isn't an efficient way to encode/decode Unicode codepoints
to/from the server encoding. These functions make it easy to perform
encoding conversions needed for the JSON datatype. However, they're
not very useful when operating solely in the server encoding, hence
the low usefulometric reading.

As for the JSON datatype support itself, nobody has come out against
making JSON a core datatype rather than a contrib module, so I will
proceed with making it one. I guess this would involve adding entries
to pg_type.h and pg_proc.h . Where would I put the rest of the code?
I guess json_io.c and json_op.c (the PG_FUNCTION_ARGS functions) would
become json.c in src/backend/utils/adt . Where would json.c and
jsonpath.c (JSON encoding/decoding functions and JSONPath
implementation) go?

Are there any other issues with the JSON code I didn't spot?

Thanks,

Joey Adams

Attachments:

json-datatype-wip-02.diffapplication/octet-stream; name=json-datatype-wip-02.diffDownload+6706-0
#8Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Joseph Adams (#7)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Excerpts from Joseph Adams's message of mar ago 10 04:03:43 -0400 2010:

An overview, along with my thoughts, of the utility functions:

FN_EXTRA, FN_EXTRA_ALLOC, FN_MCXT macros
* Useful-ometer: ()--------------------o

TypeInfo structure and getTypeInfo function
* Useful-ometer: ()---------------------------o

getEnumLabelOids
* Useful-ometer: ()-----------------------------------o

I think this kind of thing could be stripped from the patch and
submitted separately; they would presumably see a quick review and
commit if they are small and useful (particularly if you can show a
decrease of code verbosity by switching other uses in the existing
code).

The advantage is you don't have to keep arguing for their usefulness in
the JSON patch; and if they turn out to be rejected, they won't cause
the JSON patch to be rejected as a whole.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#8)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Joseph Adams's message of mar ago 10 04:03:43 -0400 2010:

An overview, along with my thoughts, of the utility functions:

FN_EXTRA, FN_EXTRA_ALLOC, FN_MCXT macros
* Useful-ometer: ()--------------------o

TypeInfo structure and getTypeInfo function
* Useful-ometer: ()---------------------------o

getEnumLabelOids
* Useful-ometer: ()-----------------------------------o

I think this kind of thing could be stripped from the patch and
submitted separately;

+1. It's easier all around if a patch does just one thing. Code
refactoring and feature addition, in particular, are easier to review
separately.

regards, tom lane

#10Simon Riggs
simon@2ndQuadrant.com
In reply to: Joseph Adams (#4)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Sat, 2010-07-24 at 18:57 -0400, Joseph Adams wrote:

I've been developing it as a contrib module because:
* I'd imagine it's easier than developing it as a built-in datatype
right away (e.g. editing a .sql.in file versus editing pg_type.h ).
* As a module, it has PGXS support, so people can try it out right
away rather than having to recompile PostgreSQL.

I, for one, think it would be great if the JSON datatype were all in
core :-) However, if and how much JSON code should go into core is up
for discussion. Thoughts, anyone?

As a GSoC piece of work, doing it as a contrib module gives an
immediately useful deliverable. Good plan.

Once that is available, we can then get some feedback on it and include
it as an in-core datatype later in the 9.1 cycle.

So lets do both: contrib and in-core.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

#11David Fetter
david@fetter.org
In reply to: Joseph Adams (#4)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Sat, Jul 24, 2010 at 06:57:18PM -0400, Joseph Adams wrote:

Update: I'm in the middle of cleaning up the JSON code (
http://git.postgresql.org/gitweb?p=json-datatype.git;a=summary if you
want to see the very latest ), so I haven't addressed all of the major
problems with it yet.

On Fri, Jul 23, 2010 at 2:34 PM, Robert Haas <robertmhaas@gmail.com> wrote:

- I was under the impression that we wanted EXPLAIN (FORMAT JSON) to
return type json, but that's obviously not going to be possible if all
of this is contrib. �We could (a) move it all into core, (b) move the
type itself and its input and output functions into core and leave the
rest in contrib [or something along those lines], or (c) give up using
it as the return type for EXPLAIN (FORMAT JSON).

I've been developing it as a contrib module because:
* I'd imagine it's easier than developing it as a built-in datatype
right away (e.g. editing a .sql.in file versus editing pg_type.h ).
* As a module, it has PGXS support, so people can try it out right
away rather than having to recompile PostgreSQL.

I, for one, think it would be great if the JSON datatype were all in
core :-) However, if and how much JSON code should go into core is up
for discussion. Thoughts, anyone?

+1 for putting it in core in 9.1 :)

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#12Joshua D. Drake
jd@commandprompt.com
In reply to: David Fetter (#11)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Wed, 2010-08-11 at 15:27 -0700, David Fetter wrote:

I've been developing it as a contrib module because:
* I'd imagine it's easier than developing it as a built-in datatype
right away (e.g. editing a .sql.in file versus editing pg_type.h ).
* As a module, it has PGXS support, so people can try it out right
away rather than having to recompile PostgreSQL.

I, for one, think it would be great if the JSON datatype were all in
core :-) However, if and how much JSON code should go into core is up
for discussion. Thoughts, anyone?

+1 for putting it in core in 9.1 :)

I would be curious to the benefit of putting it in core. I have no
problem with the type but in core?

Joshua D. Drake

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579
Consulting, Training, Support, Custom Development, Engineering
http://twitter.com/cmdpromptinc | http://identi.ca/commandprompt

#13David Fetter
david@fetter.org
In reply to: Joshua D. Drake (#12)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Wed, Aug 11, 2010 at 03:40:36PM -0700, Joshua D. Drake wrote:

On Wed, 2010-08-11 at 15:27 -0700, David Fetter wrote:

I've been developing it as a contrib module because:
* I'd imagine it's easier than developing it as a built-in
datatype right away (e.g. editing a .sql.in file versus editing
pg_type.h ).
* As a module, it has PGXS support, so people can try it out
right away rather than having to recompile PostgreSQL.

I, for one, think it would be great if the JSON datatype were
all in core :-) However, if and how much JSON code should go
into core is up for discussion. Thoughts, anyone?

+1 for putting it in core in 9.1 :)

I would be curious to the benefit of putting it in core. I have no
problem with the type but in core?

If it's not in core, the vast majority of users will not have it
installed, and nothing, in core or otherwise, will be able to count on
it.

As this is really pretty green-field stuff, it's super unlikely to
break extant code. :)

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#14Andrew Dunstan
andrew@dunslane.net
In reply to: David Fetter (#13)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On 08/11/2010 07:33 PM, David Fetter wrote:

I would be curious to the benefit of putting it in core. I have no
problem with the type but in core?

If it's not in core, the vast majority of users will not have it
installed, and nothing, in core or otherwise, will be able to count on
it.

You could say that about almost any feature. PostgreSQL is designed to
be modular, and we can hardly credibly use that as an argument against
ourselves.

A convincing argument would be that there is another feature we want in
core that needs or at least could benefit from it.

cheers

andrew

#15Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#14)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Andrew Dunstan wrote:

On 08/11/2010 07:33 PM, David Fetter wrote:

I would be curious to the benefit of putting it in core. I have no
problem with the type but in core?

If it's not in core, the vast majority of users will not have it
installed, and nothing, in core or otherwise, will be able to count on
it.

You could say that about almost any feature. PostgreSQL is designed to
be modular, and we can hardly credibly use that as an argument against
ourselves.

A convincing argument would be that there is another feature we want in
core that needs or at least could benefit from it.

I would say that JSON is no longer a niche data format, which would
suggest its inclusion in core.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

#16David Fetter
david@fetter.org
In reply to: Andrew Dunstan (#14)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Wed, Aug 11, 2010 at 07:39:37PM -0400, Andrew Dunstan wrote:

On 08/11/2010 07:33 PM, David Fetter wrote:

I would be curious to the benefit of putting it in core. I have no
problem with the type but in core?

If it's not in core, the vast majority of users will not have it
installed, and nothing, in core or otherwise, will be able to count on
it.

You could say that about almost any feature. PostgreSQL is designed
to be modular, and we can hardly credibly use that as an argument
against ourselves.

A convincing argument would be that there is another feature we want
in core that needs or at least could benefit from it.

EXPLAIN (FORMAT JSON) would benefit right away, as its overall code
would be much more likely to be (and stay) correct.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#17Joseph Adams
joeyadams3.14159@gmail.com
In reply to: Joseph Adams (#7)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Updated patch: the JSON code has all been moved into core, so this
patch is now for a built-in data type. However, I factored the
general-purpose utility functions out into a different patch to be
reviewed separately, so this JSON data type patch won't work without
the utility patch at
http://archives.postgresql.org/pgsql-hackers/2010-08/msg00949.php .

I still need to improve the documentation, as it still looks like the
documentation for a module instead of a built-in data type. Also, the
code is not taking advantage of the json_type_t enum being built-in
yet; it still uses getEnumLabelOids (one of the utility functions) to
retrieve the OIDs dynamically.

Joey Adams

Attachments:

json-datatype-wip-03.diffapplication/octet-stream; name=json-datatype-wip-03.diffDownload+6296-4
#18Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Joseph Adams (#17)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

Hi, I start to review JSON patch.

On Fri, Aug 13, 2010 at 7:33 PM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:

Updated patch:  the JSON code has all been moved into core, so this
patch is now for a built-in data type.

I think the patch can be split into two pieces:
1. Basic I/O support for JSON type (in/out/validate)
2. JSONPath support and functions for partial node management

It is better to submit only 1 at first. Of course we should consider
about JSONPath before deciding the internal representation of JSON,
but separated patches can be easily reviewed.

I have several questions about the spec and implementation.
Sorry if you have already discussed about some of them, but I cannot
understand why the current code is the best design from the patch...

* Should we accept a scalar value as a valid JSON?
According to RFC, the root element of JSON text must be an object
or array. But to_json() and from_json() accept scalar values.

* JSON to a scalar value by from_json()
How about to have json_to_array(JSON) instead of from_json()?
JSON value is always an array or object, it's nonsense to convert
it to a scalar value directly; to an array seems to match better.
In addition, an array can be indexed with GIN; index-able JSON
type is very attractive.

* struct JSON seems to be too complex for me.
Can we use List (pg_list.h) instead of linked-list? 'key' and 'key_length'
fields should be held in the parent's List. i.e, JSON_ARRAY has List of
JSON, and JSON_OBJECT has List of {string, JSON} pairs.

We could also discard 'parent' field. It might be needed by JSONPath,
but we can have parent information in variables on C-stack because we
search JSON trees from root to children, no?

I think we don't need 'orig' field because the original input text is
not so important in normal use cases. Instead, we could have formatter
function something like json_pretty(json) RETURNS text.

* On-disk format of JSON values
(There might be some discussions before... What is the conclusion?)
The current code stores the original input text, but we can use
some kinds of pre-parsed format to store JSON, like hstore.
It can be different from BSON.

* Completeness of JSONPath APIs
json_get() can be replaced with json_path(), no?
Also, we can replace existing nodes with json_set(), but we cannot
append new nodes. What do you think modification of JSON value?
If the design is too difficult, it'd be better only to have search
APIs at this moment. Modification APIs will be added in the future.

--
Itagaki Takahiro

#19Robert Haas
robertmhaas@gmail.com
In reply to: Itagaki Takahiro (#18)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

On Wed, Aug 25, 2010 at 1:34 AM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:

* Should we accept a scalar value as a valid JSON?
According to RFC, the root element of JSON text must be an object
or array. But to_json() and from_json() accept scalar values.

This seems a bit like the XML document/content distinction, which I've
never really understood. If [[1], false] is a legal JSON value, then
it seems odd that [1] should be legal but false not.

* JSON to a scalar value by from_json()
How about to have json_to_array(JSON) instead of from_json()?
JSON value is always an array or object, it's nonsense to convert
it to a scalar value directly; to an array seems to match better.
In addition, an array can be indexed with GIN; index-able JSON
type is very attractive.

Yeah, I don't like the excessive use of polymorphism either.

* On-disk format of JSON values
(There might be some discussions before... What is the conclusion?)
The current code stores the original input text, but we can use
some kinds of pre-parsed format to store JSON, like hstore.
It can be different from BSON.

I see no value to choosing a different on-disk format. It might speed
up indexing, but I/O will be slower.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

#20Hitoshi Harada
umi.tanuki@gmail.com
In reply to: Robert Haas (#19)
Re: patch: Add JSON datatype to PostgreSQL (GSoC, WIP)

2010/8/25 Robert Haas <robertmhaas@gmail.com>:

On Wed, Aug 25, 2010 at 1:34 AM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:

* Should we accept a scalar value as a valid JSON?
According to RFC, the root element of JSON text must be an object
or array. But to_json() and from_json() accept scalar values.

This seems a bit like the XML document/content distinction, which I've
never really understood.  If [[1], false] is a legal JSON value, then
it seems odd that [1] should be legal but false not.

I want false to be parsed without error, just for convinience. JSON
specification seems a bit too strict. For example, it doesn't have
date value as its parts, which results in people implement their own
parsing rule for Date(long). And AFAIK the strictness of JSON parsing
is partly because the main platform was browser engines that can
eval() string that causes security issue. Without execution engine, we
can allow more formats than RFC.

* On-disk format of JSON values
(There might be some discussions before... What is the conclusion?)
The current code stores the original input text, but we can use
some kinds of pre-parsed format to store JSON, like hstore.
It can be different from BSON.

I see no value to choosing a different on-disk format.  It might speed
up indexing, but I/O will be slower.

It depends on use cases, but in my mind plain text will do for us. If
we have JavaScript engine in PostgreSQL like pl/v8 and it handles
on-disk format as-is, then we should choose the kind of format, but in
either text or binary format way it is hopeless to have such
compelling environment in the short future.

Regards,

--
Hitoshi Harada

#21Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Hitoshi Harada (#20)
#22Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Itagaki Takahiro (#18)
#23Andrew Dunstan
andrew@dunslane.net
In reply to: Itagaki Takahiro (#22)
#24Magnus Hagander
magnus@hagander.net
In reply to: Andrew Dunstan (#23)
#25Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Joseph Adams (#17)
#26Joseph Adams
joeyadams3.14159@gmail.com
In reply to: Itagaki Takahiro (#25)
#27Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Joseph Adams (#26)
#28Bruce Momjian
bruce@momjian.us
In reply to: Itagaki Takahiro (#27)
#29Robert Haas
robertmhaas@gmail.com
In reply to: Itagaki Takahiro (#27)
#30Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Robert Haas (#29)
#31Robert Haas
robertmhaas@gmail.com
In reply to: Itagaki Takahiro (#30)
#32Joseph Adams
joeyadams3.14159@gmail.com
In reply to: Robert Haas (#31)
#33Robert Haas
robertmhaas@gmail.com
In reply to: Joseph Adams (#32)
#34Craig Ringer
craig@2ndquadrant.com
In reply to: David Fetter (#11)