Another swing at JSON

On Mon, Mar 28, 2011 at 1:21 PM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:

Attached is a patch that adds a 'json' contrib module.  Although we
may want a built-in JSON data type in the near future, making it a
module (for the time being) has a couple advantages:

Is this something you'd hope to get committed at some point, or do you
plan to maintain it as an independent project?

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

On Mon, Mar 28, 2011 at 1:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Mar 28, 2011 at 1:21 PM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:

Attached is a patch that adds a 'json' contrib module.  Although we
may want a built-in JSON data type in the near future, making it a
module (for the time being) has a couple advantages:

Is this something you'd hope to get committed at some point, or do you
plan to maintain it as an independent project?

I'm hoping to get it committed at some point, perhaps as a module
soon, and a built-in later. Plenty of people are still waiting for
JSON data type support, for example:

http://stackoverflow.com/questions/4995945/optimize-escape-json-in-postgresql-9-0

On Mon, Mar 28, 2011 at 2:03 PM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:

On Mon, Mar 28, 2011 at 1:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Mar 28, 2011 at 1:21 PM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:

Attached is a patch that adds a 'json' contrib module.  Although we
may want a built-in JSON data type in the near future, making it a
module (for the time being) has a couple advantages:

Is this something you'd hope to get committed at some point, or do you
plan to maintain it as an independent project?

I'm hoping to get it committed at some point, perhaps as a module
soon, and a built-in later.  Plenty of people are still waiting for
JSON data type support, for example:

   http://stackoverflow.com/questions/4995945/optimize-escape-json-in-postgresql-9-0

Well, one thing you'll need to do is rework it for the new 9.1
extensions interface. Once you're reasonably happy with it, I think
it'd be good to add this to the next CommitFest:

https://commitfest.postgresql.org/action/commitfest_view/open

I'd like to review it more, but it's more than I can tackle at the moment.

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

On 3/28/11 10:21 AM, Joseph Adams wrote:

Currently, there are no functions for converting to/from PostgreSQL
values or getting/setting sub-values (e.g. JSONPath). However, I did
adapt the json_stringify function written by Itagaki Takahiro in his
patch ( http://archives.postgresql.org/pgsql-hackers/2010-09/msg01200.php
).

Would it be possible for you to add a TODO list for JSON support to the
wiki? We have some potential GSOC students who are interested in
working on JSON support.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Mar 28, 2011 at 2:17 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Well, one thing you'll need to do is rework it for the new 9.1
extensions interface.

Done. The new extension interface isn't exactly compatible with the
old, so I dropped support for PostgreSQL 8.4 from the module. I
suppose I could maintain a back-ported json module separately.

On Mon, Mar 28, 2011 at 7:44 PM, Josh Berkus <josh@agliodbs.com> wrote:

On 3/28/11 10:21 AM, Joseph Adams wrote:

Currently, there are no functions for converting to/from PostgreSQL
values or getting/setting sub-values (e.g. JSONPath). However, I did
adapt the json_stringify function written by Itagaki Takahiro in his
patch ( http://archives.postgresql.org/pgsql-hackers/2010-09/msg01200.php
).

Would it be possible for you to add a TODO list for JSON support to the
wiki? We have some potential GSOC students who are interested in
working on JSON support.

What exactly should go on the TODO list? Adding more features to this
JSON data type implementation (and eventually merging some/all of it
into core)? Or doing a comparative analysis (benchmarking, etc.) of
the ~seven JSON data type implementations floating around? Since I'm
not sure what was decided regarding an efficient binary internal
representation, I don't know what we should write on the TODO list.

In my humble (and biased) opinion, we should review and commit my JSON
data type code as a starting point. Then, a GSoC student could work
on features (e.g. value conversion, JSONPath), integration (e.g.
moving to core, EXPLAIN FORMAT JSON, PL/Js, etc.), and improvements
(e.g. a binary internal representation).

Thoughts?

Joey Adams

Joseph Adams <joeyadams3.14159@gmail.com> writes:

Done. The new extension interface isn't exactly compatible with the
old, so I dropped support for PostgreSQL 8.4 from the module. I
suppose I could maintain a back-ported json module separately.

In fact it is, but there's some history hiding the fact. I'm overdue to
another doc patch on the matter, but it's quite simple.

You don't need to use MODULE_PATHNAME in recent enough versions of
PostgreSQL, meaning any version that's not currently EOL'ed. Just use
$libdir and the backend code will be happy with it. That means you
don't need the .sql.in file either.

You don't need to use the control file property module_pathname either
in most cases, that's only useful if you are building more than one
extension from the same Makefile.

So just use $libdir/json in your json.sql file and be done with it.
Your extension is now compatible with both pre-9.1 and 9.1.

I'm not sure how to spell that in the docs though, any help here would
be welcome. Also, do we want to adapt contrib to be better examples, or
do we want contrib to remain full of its history?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Tue, Mar 29, 2011 at 10:26 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Joseph Adams <joeyadams3.14159@gmail.com> writes:

Done.  The new extension interface isn't exactly compatible with the
old, so I dropped support for PostgreSQL 8.4 from the module.  I
suppose I could maintain a back-ported json module separately.

In fact it is, but there's some history hiding the fact.  I'm overdue to
another doc patch on the matter, but it's quite simple.

Cool, thanks! Attached is an updated patch for the module. Backward
compatibility has been brought back, and the module has been tested on
PostgreSQL versions 8.4.0 and 9.1devel.

However, there are a couple minor caveats:

* The last test, relocatable, fails (and should fail) on pre-9.1
because it's a test related to the new extension interface.
* init.sql is rather hacky in how it caters to both the old extension
system and the new one:

\set ECHO none
SET client_min_messages = fatal;

CREATE EXTENSION json;
\i json--0.1.sql

SET client_min_messages = warning;
...
RESET client_min_messages;
\set ECHO all

It would be nice if I could make a Makefile conditional that skips the
relocatable test and loads init-pre9.1.sql if the new extension
interface isn't available. Is there a Makefile variable or something
I can use to do this?

Also, should uninstall_json.sql be named something else, like
json--uninstall--0.1.sql ?

Thanks,

Joey Adams

Joseph Adams <joeyadams3.14159@gmail.com> writes:

It would be nice if I could make a Makefile conditional that skips the
relocatable test and loads init-pre9.1.sql if the new extension
interface isn't available. Is there a Makefile variable or something
I can use to do this?

You can use VERSION and MAJORVERSION variables, those are defined in
Makefile.global and available as soon as you did include the PGXS
Makefile.

Also, should uninstall_json.sql be named something else, like
json--uninstall--0.1.sql ?

You don't need no uninstall script no more, try DROP EXTENSION json; and
DROP EXTENSION json CASCADE;

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Tue, Mar 29, 2011 at 2:42 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Also, should uninstall_json.sql be named something else, like
json--uninstall--0.1.sql ?

You don't need no uninstall script no more, try DROP EXTENSION json; and
DROP EXTENSION json CASCADE;

It's there for pre-9.1, where DROP EXTENSION is not available.

On Tue, Mar 29, 2011 at 2:42 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Joseph Adams <joeyadams3.14159@gmail.com> writes:

It would be nice if I could make a Makefile conditional that skips the
relocatable test and loads init-pre9.1.sql if the new extension
interface isn't available.  Is there a Makefile variable or something
I can use to do this?

You can use VERSION and MAJORVERSION variables, those are defined in
Makefile.global and available as soon as you did include the PGXS
Makefile.

The problem is, I'd have to include the PGXS Makefile before defining
a parameter used by the PGXS Makefile. I could include the PGXS
Makefile twice, once at the top, and again at the bottom, but that
produces a bunch of ugly warnings like:

../../src/Makefile.global:418: warning: overriding commands for
target `submake-libpq'

Not only that, but MAJORVERSION is formatted as a decimal (like 9.1 or
8.4). Although I could use some hacky string manipulation, or compare
MAJORVERSION against all prior supported versions, either approach
would be needlessly error-prone. I'm thinking the pg_config utility
should either make PG_VERSION_NUM (e.g. 90100) available, or it should
define something indicating the presence of the new extension system.

Joey Adams

Joseph Adams <joeyadams3.14159@gmail.com> writes:

would be needlessly error-prone. I'm thinking the pg_config utility
should either make PG_VERSION_NUM (e.g. 90100) available, or it should
define something indicating the presence of the new extension system.

Here's the ugly trick from ip4r, that's used by more extension:

PREFIX_PGVER = $(shell echo $(VERSION) | awk -F. '{ print $$1*100+$$2 }')

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Tue, Mar 29, 2011 at 02:56:52PM -0400, Joseph Adams wrote:

On Tue, Mar 29, 2011 at 2:42 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Also, should uninstall_json.sql be named something else, like
json--uninstall--0.1.sql ?

You don't need no uninstall script no more, try DROP EXTENSION json; and
DROP EXTENSION json CASCADE;

It's there for pre-9.1, where DROP EXTENSION is not available.

Anything going into the PostgreSQL code base will be for 9.2, so
anything else would be a separate (if somewhat related) project. I
suspect the code will be a good deal cleaner if you do just the 9.2+
version and see who wants it back-patched, if anyone does :)

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

On Tue, Mar 29, 2011 at 4:02 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Here's the ugly trick from ip4r, that's used by more extension:

PREFIX_PGVER = $(shell echo $(VERSION) | awk -F. '{ print $$1*100+$$2 }')

Thanks. I applied a minor variation of this trick to the JSON module,
so now it builds/installs/tests cleanly on both REL8_4_0 and HEAD
(though it won't work if you copy contrib/json into a pre-9.1
PostgreSQL source directory and type `make` without USE_PGXS=1).

I also went ahead and renamed uninstall_json.sql to
json--uninstall--0.1.sql (again, it's for pre-9.1 users) and removed
unnecessary trailing spaces.

Anything going into the PostgreSQL code base will be for 9.2, so
anything else would be a separate (if somewhat related) project. I
suspect the code will be a good deal cleaner if you do just the 9.2+
version and see who wants it back-patched, if anyone does :)

It's a trivial matter to remove backward compatibility from
contrib/json, if anybody wants me to do it. I can just remove
compat.[ch], */init-pre9.1.* , remove the PREFIX_PGVER trick from the
Makefile, remove a few lines in the source code, and maintain the
backported json module elsewhere. It's just a matter of whether or
not explicit backward-compatibility is desirable in modules shipped
with releases.

Joey Adams

Excerpts from Joseph Adams's message of mar mar 29 22:15:11 -0300 2011:

On Tue, Mar 29, 2011 at 4:02 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Here's the ugly trick from ip4r, that's used by more extension:

PREFIX_PGVER = $(shell echo $(VERSION) | awk -F. '{ print $$1*100+$$2 }')

Thanks. I applied a minor variation of this trick to the JSON module,
so now it builds/installs/tests cleanly on both REL8_4_0 and HEAD
(though it won't work if you copy contrib/json into a pre-9.1
PostgreSQL source directory and type `make` without USE_PGXS=1).

Why are you worrying with the non-PGXS build chain anyway? Just assume
that the module is going to be built with PGXS and things should just
work.

We've gone over this a dozen times in the past.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:

Why are you worrying with the non-PGXS build chain anyway? Just assume
that the module is going to be built with PGXS and things should just
work.

We've gone over this a dozen times in the past.

+1

I'm not sure why we still support the pre-PGXS build recipe in the
contrib Makefiles, and didn't want to change that as part as the
extension patch series, but I think we should reconsider.

This and removing module_pathname in the control files to just use
$libdir/contrib in the .sql files. That would set a better example to
people who want to make their own extensions, as the general case is
that those will not get into contrib.

I think we should lower the differences between contrib and external
extensions, so that contrib is only about who maintains the code and
distribute the extension.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Excerpts from Dimitri Fontaine's message of mié mar 30 05:27:07 -0300 2011:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Why are you worrying with the non-PGXS build chain anyway? Just assume
that the module is going to be built with PGXS and things should just
work.

We've gone over this a dozen times in the past.

+1

I'm not sure why we still support the pre-PGXS build recipe in the
contrib Makefiles, and didn't want to change that as part as the
extension patch series, but I think we should reconsider.

This is in the archives somewhere. I think it's something to do with
the fact that "make check" doesn't work under PGXS; you have to use
"make installcheck". Or maybe it was something else. I don't really
remember the details. I also pushed for this, a long time ago.

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

On Wed, Mar 30, 2011 at 10:27:07AM +0200, Dimitri Fontaine wrote:

I think we should lower the differences between contrib and external
extensions, so that contrib is only about who maintains the code and
distribute the extension.

+10 :)

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

Excerpts from Alvaro Herrera's message of mié mar 30 10:27:39 -0300 2011:

Excerpts from Dimitri Fontaine's message of mié mar 30 05:27:07 -0300 2011:

I'm not sure why we still support the pre-PGXS build recipe in the
contrib Makefiles, and didn't want to change that as part as the
extension patch series, but I think we should reconsider.

This is in the archives somewhere. I think it's something to do with
the fact that "make check" doesn't work under PGXS; you have to use
"make installcheck". Or maybe it was something else. I don't really
remember the details. I also pushed for this, a long time ago.

In http://archives.postgresql.org/pgsql-hackers/2009-07/msg00245.php
Tom writes:

The main reason contrib still has the alternate method is that PGXS
doesn't really work until after you've installed the core build.

Maybe we could have a look and try to fix that problem.

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

On Wed, Mar 30, 2011 at 10:32:55AM -0300, Alvaro Herrera wrote:

Excerpts from Alvaro Herrera's message of mi� mar 30 10:27:39 -0300 2011:

Excerpts from Dimitri Fontaine's message of mi� mar 30 05:27:07 -0300 2011:

I'm not sure why we still support the pre-PGXS build recipe in the
contrib Makefiles, and didn't want to change that as part as the
extension patch series, but I think we should reconsider.

This is in the archives somewhere. I think it's something to do with
the fact that "make check" doesn't work under PGXS; you have to use
"make installcheck". Or maybe it was something else. I don't really
remember the details. I also pushed for this, a long time ago.

In http://archives.postgresql.org/pgsql-hackers/2009-07/msg00245.php
Tom writes:

The main reason contrib still has the alternate method is that PGXS
doesn't really work until after you've installed the core build.

Maybe we could have a look and try to fix that problem.

+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