Re: Another swing at JSON

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:

* Installable on current and previous versions of PostgreSQL. The
json module supports PostgreSQL 8.4.0 and up.
* Easier to maintain. json.sql.in is easy to work on,
src/include/catalog/pg_proc.h is not.

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
).

JSON datums are stored internally as condensed JSON text. By
"condensed", I mean that whitespace is removed, and escapes are
converted to characters when it's possible and efficient to do so.
Although a binary internal format would be more size-efficient in
theory, condensed JSON text is pretty efficient, too. Internally, the
implementation does not construct any parse trees, which should
provide a major performance advantage.

Almost all of the code has been rewritten since my original patch (
http://archives.postgresql.org/pgsql-hackers/2010-07/msg01215.php ).
Some will be happy to know that the new code doesn't have any gotos
:-)

Joey Adams

On Mon, Mar 28, 2011 at 1:21 PM, Joseph Adams
<joeyadams3(dot)14159(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Mon, Mar 28, 2011 at 1:21 PM, Joseph Adams
> <joeyadams3(dot)14159(at)gmail(dot)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(dot)14159(at)gmail(dot)com> wrote:
> On Mon, Mar 28, 2011 at 1:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Mar 28, 2011 at 1:21 PM, Joseph Adams
>> <joeyadams3(dot)14159(at)gmail(dot)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(at)gmail(dot)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(at)agliodbs(dot)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(dot)14159(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)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(dot)14159(at)gmail(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)fr> wrote:
> Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)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(dot)14159(at)gmail(dot)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(at)2ndquadrant(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)commandprompt(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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 03/30/2011 09:42 AM, David Fetter wrote:
>>
>> 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 :)
>
>

Maybe we could teach pg_config to report appropriate settings for
uninstalled source via an environment setting. Without changing
pg_config it looks like we have a chicken and egg problem.

(If my suggestion is right, this would probably be a good beginner TODO.)

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Maybe we could teach pg_config to report appropriate settings for
> uninstalled source via an environment setting. Without changing pg_config it
> looks like we have a chicken and egg problem.
>
> (If my suggestion is right, this would probably be a good beginner TODO.)

Is it still 9.1 material? I think it should be considered UI
improvements on what we already have (contribs and extensions), and
allowed to be fixed while in beta.

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

On 03/30/2011 11:37 AM, Dimitri Fontaine wrote:
> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>> Maybe we could teach pg_config to report appropriate settings for
>> uninstalled source via an environment setting. Without changing pg_config it
>> looks like we have a chicken and egg problem.
>>
>> (If my suggestion is right, this would probably be a good beginner TODO.)
> Is it still 9.1 material? I think it should be considered UI
> improvements on what we already have (contribs and extensions), and
> allowed to be fixed while in beta.

I think we're pretty much down to only fixing bugs now, for 9.1, and
this isn't a bug, however inconvenient it might be.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I think we're pretty much down to only fixing bugs now, for 9.1, and this
> isn't a bug, however inconvenient it might be.

It's not just inconvenient, it's setting a bad example for people to
work on their own extensions. It's more than unfortunate. I will
prepare a doc patch, but copying from contrib is the usual way to go
creating your own extension, right?

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

On 03/30/2011 12:29 PM, Dimitri Fontaine wrote:
> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>> I think we're pretty much down to only fixing bugs now, for 9.1, and this
>> isn't a bug, however inconvenient it might be.
> It's not just inconvenient, it's setting a bad example for people to
> work on their own extensions. It's more than unfortunate. I will
> prepare a doc patch, but copying from contrib is the usual way to go
> creating your own extension, right?
>
>

None of that makes it a bug.

I don't have any objection to putting some comments in the contrib
Makefiles telling people to use PGXS, but I don't think that at this
stage of the cycle we can start work on something that so far is just an
idea from the top of my head.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I don't have any objection to putting some comments in the contrib Makefiles
> telling people to use PGXS, but I don't think that at this stage of the
> cycle we can start work on something that so far is just an idea from the
> top of my head.

I might be mistaken on how much work is hidden there, ok, you have the
point. I will try to clarify this in the doc patch I intend to submit
soon'ish: we still accept those while getting to beta, right?

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

Excerpts from Dimitri Fontaine's message of mié mar 30 15:05:36 -0300 2011:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > I don't have any objection to putting some comments in the contrib Makefiles
> > telling people to use PGXS, but I don't think that at this stage of the
> > cycle we can start work on something that so far is just an idea from the
> > top of my head.
>
> I might be mistaken on how much work is hidden there, ok, you have the
> point. I will try to clarify this in the doc patch I intend to submit
> soon'ish: we still accept those while getting to beta, right?

Sure.

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

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 03/30/2011 12:29 PM, Dimitri Fontaine wrote:
>> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>>> I think we're pretty much down to only fixing bugs now, for 9.1, and this
>>> isn't a bug, however inconvenient it might be.

>> It's not just inconvenient, it's setting a bad example for people to
>> work on their own extensions. It's more than unfortunate. I will
>> prepare a doc patch, but copying from contrib is the usual way to go
>> creating your own extension, right?

> None of that makes it a bug.

Possibly more to the point, we don't even have a design sketch for a
better solution; and we are *certainly* past the point where blue-sky
stuff ought to be going into 9.1.

The reason it seems (to me) nontrivial to change is this: the PGXS build
method assumes that the correct pg_config can be found in your PATH.
That is pretty much guaranteed to not be the case during a in-tree
build. Even if we modified the PATH to include wherever pg_config is
hiding, the information it puts out about where to look for include and
library files would be wrong.

Another small problem with depending on pg_config during an in-tree
build is that it would completely break cross-compile builds. (Maybe
those are in bad shape already, I'm not sure. But configure for example
is still going out of its way to support them.)

So, I'm interested in trying to improve this, but it looks like a
research project from here.

regards, tom lane

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> 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'm not sure it's a better example. We considered doing that before,
and decided not to on the grounds that using MODULE_PATHNAME avoids
hard-wiring the name of the shared library into the SQL scripts. Also,
IIRC, there are a couple of contrib modules where there's an actual
problem in doing it like that; though I'm unable to recall details as
I'm fighting a head cold at the moment.

My original intention when I started working with the extensions patch
had in fact been to do what you suggest above, but I was convinced not
to. I don't think we should reverse that decision at the last minute.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
>> 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.
>
> My original intention when I started working with the extensions patch
> had in fact been to do what you suggest above, but I was convinced not
> to. I don't think we should reverse that decision at the last minute.

Ok, I'll trust you on that. So the other way to ease compatibility
would be to keep the .sql.in to .sql Makefile rule in pgxs.mk but make
it just to a copy in 9.1. Then the same old script will just continue
working as soon as you edit the module_pathname in the control file.

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> So, I'm interested in trying to improve this, but it looks like a
> research project from here.

True: I don't have a baked solution that we would just need to
apply. The simplest idea I can think of is forcing make install before
to build contribs so that PGXS works “normally”.

We would have to clarify the point somewhere visible in the docs though,
so that we can say to people building extensions to mimic contrib/
except for the in-tree old-style Makefile support. In fact, adding some
comments about that in all the contrib extension Makefiles might be our
best shot at it, right?

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

--On 29. März 2011 21:15:11 -0400 Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)com>
wrote:

> 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.

I started looking into this. A very minor adjusted patch to filelist.sgml was
required to apply the patch cleanly to current -HEAD (attached).

After reading Joseph's comment upthread, I don't see any consensus wether the
existing pre-9.1 support is required or even desired. Maybe i missed it, but do
we really expect an extension (or contrib module) to be backwards compatible to
earlier major releases, when shipped in contrib/ ?

--
Thanks

Bernd

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> After reading Joseph's comment upthread, I don't see any consensus
> wether the existing pre-9.1 support is required or even desired. Maybe
> i missed it, but do we really expect an extension (or contrib module)
> to be backwards compatible to earlier major releases, when shipped in
> contrib/ ?

No, we don't. You won't find any attempt in any contrib module to build
against prior releases. There's not much point, since they're shipped
with a specific release of the core.

regards, tom lane

--On 16. Juni 2011 17:38:07 -0400 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>> After reading Joseph's comment upthread, I don't see any consensus
>> wether the existing pre-9.1 support is required or even desired. Maybe
>> i missed it, but do we really expect an extension (or contrib module)
>> to be backwards compatible to earlier major releases, when shipped in
>> contrib/ ?
>
> No, we don't. You won't find any attempt in any contrib module to build
> against prior releases. There's not much point, since they're shipped
> with a specific release of the core.

Okay, then we should remove this code. It doesn't do any complicated, but it
seems a waste of code in this case (and from a maintenance point of view).

Joseph, are you able to remove the compatibility code for this CF?

--
Thanks

Bernd

On Fri, Jun 17, 2011 at 2:29 AM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
> Joseph, are you able to remove the compatibility code for this CF?

Done. Note that this module builds, tests, and installs successfully
with USE_PGXS=1. However, building without USE_PGXS=1 produces the
following:

CREATE EXTENSION json;
ERROR: incompatible library "/usr/lib/postgresql/json.so": version mismatch
DETAIL: Server is version 9.1, library is version 9.2.

Similar problems occur with a couple other modules I tried (hstore, intarray).

Joey

--On 17. Juni 2011 18:06:58 -0400 Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)com>
wrote:

> Done. Note that this module builds, tests, and installs successfully
> with USE_PGXS=1. However, building without USE_PGXS=1 produces the
> following:
>
> CREATE EXTENSION json;
> ERROR: incompatible library "/usr/lib/postgresql/json.so": version
> mismatch DETAIL: Server is version 9.1, library is version 9.2.
>
> Similar problems occur with a couple other modules I tried (hstore, intarray).

Hmm, works for me. Seems you have messed up your installation in some way
(build against current -HEAD but running against a 9.1?).

I'm going to review in the next couple of days again.

--
Thanks

Bernd