Thread

PGXS: REGRESS_OPTS=--load-language=plpgsql

David Fetter <david@fetter.org> — 2010-02-18T21:51:08Z

Folks,

While hacking on PL/Parrot, I ran across an issue where when trying to
load PL/pgsql, it's done unconditionally and fails.  How do we fix
pg_regress to be a little more subtle about this?

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

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-18T23:27:30Z

David Fetter <david@fetter.org> writes:
> While hacking on PL/Parrot, I ran across an issue where when trying to
> load PL/pgsql, it's done unconditionally and fails.  How do we fix
> pg_regress to be a little more subtle about this?

Why exactly would we want it to not fail?  Regression tests are not
about papering over problems.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-19T00:36:07Z

On Feb 18, 2010, at 3:27 PM, Tom Lane wrote:

>> While hacking on PL/Parrot, I ran across an issue where when trying to
>> load PL/pgsql, it's done unconditionally and fails.  How do we fix
>> pg_regress to be a little more subtle about this?
> 
> Why exactly would we want it to not fail?  Regression tests are not
> about papering over problems.

pg_regress needs to not install plpgsql into the data database on 9.0 when passed `--load-language=plpgsql`, because plpgsql will of course already be installed.

Unless you want all the third-party modules that depend on plpgsql for tests to somehow detect that they're going to run on 8.5a3 or later and not pass that option. But that'd be kind of a PITA. Much easier if pg_regress knows it doesn't need to install plpgsql.

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David Fetter <david@fetter.org> — 2010-02-19T00:44:46Z

On Thu, Feb 18, 2010 at 06:27:30PM -0500, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > While hacking on PL/Parrot, I ran across an issue where when
> > trying to load PL/pgsql, it's done unconditionally and fails.  How
> > do we fix pg_regress to be a little more subtle about this?
> 
> Why exactly would we want it to not fail?  Regression tests are not
> about papering over problems.

OK, I know it's late, but having PL/pgsql on by default has caused an
unforeseen need: --require-language.

Please find enclosed a patch which implements this.

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

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Euler Taveira <euler@timbira.com> — 2010-02-19T02:26:29Z

David Fetter escreveu:
> OK, I know it's late, but having PL/pgsql on by default has caused an
> unforeseen need: --require-language.
> 
Why? IMHO pg_regress should be used with the same postgres version it was
built with. So any tests that use --load-language=plpgsql should be fixed.
Besides we could  patch pg_regress.c to ignore loading plpgsql language into
the database (instead of adding another parameter -- we already have too many
options).


-- 
  Euler Taveira de Oliveira
  http://www.timbira.com/

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David Fetter <david@fetter.org> — 2010-02-19T02:40:30Z

On Fri, Feb 19, 2010 at 12:26:29AM -0200, Euler Taveira de Oliveira wrote:
> David Fetter escreveu:
> > OK, I know it's late, but having PL/pgsql on by default has caused
> > an unforeseen need: --require-language.
> > 
> Why? IMHO pg_regress should be used with the same postgres version
> it was built with. So any tests that use --load-language=plpgsql
> should be fixed.  Besides we could  patch pg_regress.c to ignore
> loading plpgsql language into the database (instead of adding
> another parameter -- we already have too many options).

Any external projects will fail on this if they're attempting to
support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
suggested that we special-case PL/pgsql for 9.0 and greater, as it's
in template0, where those tests are based.

Cheers,
Another 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

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2010-02-19T03:56:23Z

David Fetter <david@fetter.org> wrote:

> Any external projects will fail on this if they're attempting to
> support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
> suggested that we special-case PL/pgsql for 9.0 and greater, as it's
> in template0, where those tests are based.

+1 for the CREATE LANGUAGE IF NOT EXISTS behavior.

The regression test in the core is targeting only its version,
but some external projects have version-independent tests.
I hope --load-language works as "ensure the language is installed"
rather than "always install the language".

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-19T04:38:41Z

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:
> David Fetter <david@fetter.org> wrote:
>> support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
>> suggested that we special-case PL/pgsql for 9.0 and greater, as it's
>> in template0, where those tests are based.

> +1 for the CREATE LANGUAGE IF NOT EXISTS behavior.

> The regression test in the core is targeting only its version,
> but some external projects have version-independent tests.

I think it's more like "are under the fond illusion that their tests are
version-independent".  Are we going to back out the next incompatible
change we choose to make as soon as somebody notices that it breaks a
third-party test case?  I don't think so.  Let me point out that
choosing to install plpgsql by default has already broken "--single"
restore of practically every pg_dump out there.  Nobody batted an eye
about that.  Why are we suddenly so concerned about its effects on
unnamed test suites?

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-19T07:34:26Z

On Feb 18, 2010, at 8:38 PM, Tom Lane wrote:

>> The regression test in the core is targeting only its version,
>> but some external projects have version-independent tests.
> 
> I think it's more like "are under the fond illusion that their tests are
> version-independent".  Are we going to back out the next incompatible
> change we choose to make as soon as somebody notices that it breaks a
> third-party test case?  I don't think so.  Let me point out that
> choosing to install plpgsql by default has already broken "--single"
> restore of practically every pg_dump out there.  Nobody batted an eye
> about that.  Why are we suddenly so concerned about its effects on
> unnamed test suites?

Because it's a lot easier for `pg_regress --load-language=plpgsql` to mean "ensure the language is installed" than it is for 3rd-party test suites to detect what version they're being installed against.

Really, all that has to happen is pg_regress in 8.5a4+ needs to just ignore `--load-language=plpgsql`. That's it.

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Magnus Hagander <magnus@hagander.net> — 2010-02-19T07:49:39Z

2010/2/19 Tom Lane <tgl@sss.pgh.pa.us>:
> Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:
>> David Fetter <david@fetter.org> wrote:
>>> support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
>>> suggested that we special-case PL/pgsql for 9.0 and greater, as it's
>>> in template0, where those tests are based.
>
>> +1 for the CREATE LANGUAGE IF NOT EXISTS behavior.
>
>> The regression test in the core is targeting only its version,
>> but some external projects have version-independent tests.
>
> I think it's more like "are under the fond illusion that their tests are
> version-independent".  Are we going to back out the next incompatible
> change we choose to make as soon as somebody notices that it breaks a
> third-party test case?  I don't think so.  Let me point out that
> choosing to install plpgsql by default has already broken "--single"
> restore of practically every pg_dump out there.  Nobody batted an eye
> about that.  Why are we suddenly so concerned about its effects on
> unnamed test suites?

Oh yeah, that one is very annoying, can we go fix that one somehow?

I think the use of --single has decreased a lot in favor of parallel
restore,  but it's certainly not all that uncommon... I think the main
reason we haven't heard loads of complains is that it isn't the
default..

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Alvaro Herrera <alvherre@commandprompt.com> — 2010-02-19T13:36:42Z

David E. Wheeler wrote:
> On Feb 18, 2010, at 8:38 PM, Tom Lane wrote:
> 
> >> The regression test in the core is targeting only its version,
> >> but some external projects have version-independent tests.
> > 
> > I think it's more like "are under the fond illusion that their tests are
> > version-independent".  Are we going to back out the next incompatible
> > change we choose to make as soon as somebody notices that it breaks a
> > third-party test case?  I don't think so.  Let me point out that
> > choosing to install plpgsql by default has already broken "--single"
> > restore of practically every pg_dump out there.  Nobody batted an eye
> > about that.  Why are we suddenly so concerned about its effects on
> > unnamed test suites?
> 
> Because it's a lot easier for `pg_regress --load-language=plpgsql` to mean "ensure the language is installed" than it is for 3rd-party test suites to detect what version they're being installed against.

Why doesn't the Makefile running the tests simply avoid adding
--load-language when the version is higher than 9.0?  Shouldn't be a
hard test to write.  We have $(MAJORVERSION) to help with this.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-19T15:43:04Z

On Feb 19, 2010, at 5:36 AM, Alvaro Herrera wrote:

>> Because it's a lot easier for `pg_regress --load-language=plpgsql` to mean "ensure the language is installed" than it is for 3rd-party test suites to detect what version they're being installed against.
> 
> Why doesn't the Makefile running the tests simply avoid adding
> --load-language when the version is higher than 9.0?  Shouldn't be a
> hard test to write.  We have $(MAJORVERSION) to help with this.

Usually PGXS loads after setting all the environment variables, though I suspect that it wouldn't have any side effects to set regress_opts afterward. Also, there is no MAJORVERSION in earlier versions, so module authors would have to work around that.

Basically though, you're asking all third party module authors who depend on plpgsql in their code and/or tests to modify their makefiles and release new versions to work around something that pg_regress could have fixed internally in 1-2 lines of code and be done with it.

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-19T17:07:13Z

On Feb 19, 2010, at 7:43 AM, David E. Wheeler wrote:

> Usually PGXS loads after setting all the environment variables, though I suspect that it wouldn't have any side effects to set regress_opts afterward. Also, there is no MAJORVERSION in earlier versions, so module authors would have to work around that.
> 
> Basically though, you're asking all third party module authors who depend on plpgsql in their code and/or tests to modify their makefiles and release new versions to work around something that pg_regress could have fixed internally in 1-2 lines of code and be done with it.

I'm sure this is bad C and should do a case-insensitive comparison, but this is essentially what I mean:

*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*************** create_database(const char *dbname)
*** 1795,1802 ****
     */
    for (sl = loadlanguage; sl != NULL; sl = sl->next)
    {
!       header(_("installing %s"), sl->str);
!       psql_command(dbname, "CREATE LANGUAGE \"%s\"", sl->str);
    }
  }
  
--- 1795,1804 ----
     */
    for (sl = loadlanguage; sl != NULL; sl = sl->next)
    {
!       if (sl->str != "plpgsql") {
!           header(_("installing %s"), sl->str);
!           psql_command(dbname, "CREATE LANGUAGE \"%s\"", sl->str);
!       }
    }
  }
  
Does that seem unreasonable?

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-19T18:34:46Z

On Thu, Feb 18, 2010 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:
>> David Fetter <david@fetter.org> wrote:
>>> support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
>>> suggested that we special-case PL/pgsql for 9.0 and greater, as it's
>>> in template0, where those tests are based.
>
>> +1 for the CREATE LANGUAGE IF NOT EXISTS behavior.
>
>> The regression test in the core is targeting only its version,
>> but some external projects have version-independent tests.
>
> I think it's more like "are under the fond illusion that their tests are
> version-independent".  Are we going to back out the next incompatible
> change we choose to make as soon as somebody notices that it breaks a
> third-party test case?  I don't think so.  Let me point out that
> choosing to install plpgsql by default has already broken "--single"
> restore of practically every pg_dump out there.  Nobody batted an eye
> about that.  Why are we suddenly so concerned about its effects on
> unnamed test suites?

I am still of the opinion that changing this was a bad idea for
exactly this reason.  We could perhaps ameliorate this problem by
implementing CREATE OR REPLACE for languages and emitting that
instead; then the command in the dump would be a noop.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David Fetter <david@fetter.org> — 2010-02-19T18:40:06Z

On Fri, Feb 19, 2010 at 01:34:46PM -0500, Robert Haas wrote:
> On Thu, Feb 18, 2010 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:
> >> David Fetter <david@fetter.org> wrote:
> >>> support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
> >>> suggested that we special-case PL/pgsql for 9.0 and greater, as it's
> >>> in template0, where those tests are based.
> >
> >> +1 for the CREATE LANGUAGE IF NOT EXISTS behavior.
> >
> >> The regression test in the core is targeting only its version,
> >> but some external projects have version-independent tests.
> >
> > I think it's more like "are under the fond illusion that their
> > tests are version-independent".  Are we going to back out the next
> > incompatible change we choose to make as soon as somebody notices
> > that it breaks a third-party test case?  I don't think so.  Let me
> > point out that choosing to install plpgsql by default has already
> > broken "--single" restore of practically every pg_dump out there.
> >  Nobody batted an eye about that.  Why are we suddenly so
> > concerned about its effects on unnamed test suites?
> 
> I am still of the opinion that changing this was a bad idea for
> exactly this reason.  We could perhaps ameliorate this problem by
> implementing CREATE OR REPLACE for languages and emitting that
> instead; then the command in the dump would be a noop.

CREATE OR REPLACE LANGUAGE is an even bigger tar pit.

For example:
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00386.php

Please find attached a patch which does this check in pg_regress.

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

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-19T18:44:01Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 18, 2010 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... Let me point out that
>> choosing to install plpgsql by default has already broken "--single"
>> restore of practically every pg_dump out there. Nobody batted an eye
>> about that. Why are we suddenly so concerned about its effects on
>> unnamed test suites?

> I am still of the opinion that changing this was a bad idea for
> exactly this reason.  We could perhaps ameliorate this problem by
> implementing CREATE OR REPLACE for languages and emitting that
> instead; then the command in the dump would be a noop.

Not really going to help for existing dumps (nor future dumps made
with pre-9.0 pg_dump versions).

However, the case that is probably going to be the most pressing is
pg_upgrade, which last I heard insists on no errors during the restore
(and I think that's a good thing).  That uses the new version's pg_dump
so a fix involving new syntax would cover it.

Did we have consensus on exactly what CREATE OR REPLACE LANGUAGE would
do?  Particularly in cases where the existing definition doesn't match
pg_pltemplate?

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-19T18:50:41Z

On Fri, Feb 19, 2010 at 1:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Did we have consensus on exactly what CREATE OR REPLACE LANGUAGE would
> do?  Particularly in cases where the existing definition doesn't match
> pg_pltemplate?

I am of the opinion that any CREATE OR REPLACE command that completes
without error should result in exactly the same final state that would
have resulted had the object not existed when the command was issued.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-19T19:09:29Z

David Fetter <david@fetter.org> writes:
> CREATE OR REPLACE LANGUAGE is an even bigger tar pit.
> http://archives.postgresql.org/pgsql-hackers/2009-10/msg00386.php

The reason that patch got rejected was that it was implementing
CREATE IF NOT EXISTS --- under a false name.  The problem with
that is summarized here:
http://archives.postgresql.org/pgsql-patches/2008-03/msg00416.php

It wouldn't be that hard to implement actual CREATE OR REPLACE
if we decide that's the most useful solution here.  The code
would need to be prepared to use heap_update instead of heap_insert,
and to get rid of old dependencies, but there is plenty of precedent
for that.

The sticking point for me is still whether or not it's really a good
idea for pg_dump to be emitting CREATE OR REPLACE LANGUAGE.  It does not
do that for any other object type.  On the other hand, we've already
made languages a special case in pg_dump, since it emits the abbreviated
form of CREATE LANGUAGE in most cases rather than trying to duplicate
the existing object definition.  Maybe there wouldn't be any bad results
in practice.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Dimitri Fontaine <dfontaine@hi-media.com> — 2010-02-19T19:15:09Z

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I am still of the opinion that changing this was a bad idea for
>> exactly this reason.  We could perhaps ameliorate this problem by
>> implementing CREATE OR REPLACE for languages and emitting that
>> instead; then the command in the dump would be a noop.
>
> Not really going to help for existing dumps (nor future dumps made
> with pre-9.0 pg_dump versions).
>
> However, the case that is probably going to be the most pressing is
> pg_upgrade, which last I heard insists on no errors during the restore
> (and I think that's a good thing).  That uses the new version's pg_dump
> so a fix involving new syntax would cover it.

Not sure how helpful I'll be there, but I can't help placing the
extension's proposal again.

If we had extensions here, plpgsql would be a core maintained extension,
made available by CREATE EXTENSION in the database (which initdb would
do in templates), then have the language installed by means of issuing
an INSTALL EXTENSION command.

Now, what would help would be to have that support and have CREATE
LANGUAGE foo; be kept for compatibility only and issue INSTALL EXTENSION
foo; instead.

For those still with me, the choice to have plpgsql available by default
would then boil down to have initdb do the CREATE EXTENSION in the
template database, the database owner would still have to run the
INSTALL EXTENSION. So now --load-language is INSTALL EXTENSION and just
works as intended.

And older dumps are doing CREATE LANGUAGE plpgsql; which is converted to
INSTALL EXTENSION plpgsql;, which just works only because the extension
is made available by default.

So that if there's a CREATE LANGUAGE plpythonu, say, installing this
extension will only succeed when INSTALL EXTENSION plpythonu; has been
done either in the template1 database before creating the target
database, or in the target database itself.

So now we have "smart" success and failure modes falling from the
proposed model.

I'll dare not say "Hope This Helps" as I realize I failed to provide any
code for implementing the extension management proposal. But got back to
acceptable sleeping patterns and should be able to get back on the topic
later this year, unless (please) beaten to it :)

Regards,
-- 
dim

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-19T19:47:32Z

Dimitri Fontaine <dfontaine@hi-media.com> writes:
> Not sure how helpful I'll be there, but I can't help placing the
> extension's proposal again.

> If we had extensions here, plpgsql would be a core maintained extension,
> made available by CREATE EXTENSION in the database (which initdb would
> do in templates), then have the language installed by means of issuing
> an INSTALL EXTENSION command.

Well, that isn't really going to help us in terms of what to do for 9.0.
But the possibility that something like this might happen in future is
one thing that makes me hesitant about extending CREATE LANGUAGE right
now --- the more bells and whistles we put on it, the harder it will be
to have a clean upgrade to an EXTENSION facility.

One thing that strikes me about your proposal is that INSTALL EXTENSION
doesn't sound like a CREATE OR REPLACE operation.  It sounds like a
CREATE IF NOT EXISTS operation, because there simply is not a guarantee
that what gets installed is exactly what the user expected --- in
particular, for pg_dump, it isn't guaranteeing that the new version's
extension is exactly like what was in the old database.  And that's not
a bad thing, in this context; it's more or less the Whole Point.

However it still leaves us with the problem that CINE is underspecified.
In particular, since we have already got the notion that languages have
owners and ACLs, I'm unsure what the desired state is when pg_dump tries
to set the owner and/or ACL for a pre-existing language.  I know what
is likely to happen if we just drop these concepts into the existing
system: restoring a dump will take away ownership from whoever installed
the language (extension) previously.  That doesn't seem very good,
especially if the ownership of any SQL objects contained in the
extension doesn't change.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-19T19:51:12Z

On Fri, Feb 19, 2010 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Fetter <david@fetter.org> writes:
>> CREATE OR REPLACE LANGUAGE is an even bigger tar pit.
>> http://archives.postgresql.org/pgsql-hackers/2009-10/msg00386.php
>
> The reason that patch got rejected was that it was implementing
> CREATE IF NOT EXISTS --- under a false name.  The problem with
> that is summarized here:
> http://archives.postgresql.org/pgsql-patches/2008-03/msg00416.php
>
> It wouldn't be that hard to implement actual CREATE OR REPLACE
> if we decide that's the most useful solution here.  The code
> would need to be prepared to use heap_update instead of heap_insert,
> and to get rid of old dependencies, but there is plenty of precedent
> for that.
>
> The sticking point for me is still whether or not it's really a good
> idea for pg_dump to be emitting CREATE OR REPLACE LANGUAGE.  It does not
> do that for any other object type.  On the other hand, we've already
> made languages a special case in pg_dump, since it emits the abbreviated
> form of CREATE LANGUAGE in most cases rather than trying to duplicate
> the existing object definition.  Maybe there wouldn't be any bad results
> in practice.

We have all sorts of crufty hacks in pg_dump and the backend to cope
with restoration of older dumps.  Compared to some of those, this is
going to be cleaner than newfallen snow.  IMHO, anyway.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-19T20:01:22Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Feb 19, 2010 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The sticking point for me is still whether or not it's really a good
>> idea for pg_dump to be emitting CREATE OR REPLACE LANGUAGE. It does not
>> do that for any other object type. On the other hand, we've already
>> made languages a special case in pg_dump, since it emits the abbreviated
>> form of CREATE LANGUAGE in most cases rather than trying to duplicate
>> the existing object definition. Maybe there wouldn't be any bad results
>> in practice.

> We have all sorts of crufty hacks in pg_dump and the backend to cope
> with restoration of older dumps.  Compared to some of those, this is
> going to be cleaner than newfallen snow.  IMHO, anyway.

What worries me about it is mainly the prospect that restoring a dump
would silently change ownership and/or permissions of a pre-existing
language.  Maybe we can live with that but it's a bit nervous making.

One thing we could do that would help limit the damage is have pg_dump
only insert OR REPLACE when it's emitting a parameterless CREATE
LANGUAGE, ie, it's already depending on there to be a pg_pltemplate
entry.  This would guarantee that we aren't changing any of the core
properties of the pg_language entry (since, because of the way CREATE
LANGUAGE already works, any pre-existing entry must match the
pg_pltemplate entry).  But there's still ownership and ACL to worry
about.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Dimitri Fontaine <dfontaine@hi-media.com> — 2010-02-19T20:14:05Z

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Well, that isn't really going to help us in terms of what to do for 9.0.
> But the possibility that something like this might happen in future is
> one thing that makes me hesitant about extending CREATE LANGUAGE right
> now --- the more bells and whistles we put on it, the harder it will be
> to have a clean upgrade to an EXTENSION facility.

Agreed, but we could still evolve the command with keeping an eye on the
future. As of now I intend to implement what's on this page:

  http://wiki.postgresql.org/wiki/ExtensionPackaging

So maybe a quick glance then some early design approval would make it
possible to change the CREATE LANGUAGE in an EXTENSION compatible way.

> One thing that strikes me about your proposal is that INSTALL EXTENSION
> doesn't sound like a CREATE OR REPLACE operation.  It sounds like a
> CREATE IF NOT EXISTS operation, because there simply is not a guarantee
> that what gets installed is exactly what the user expected --- in
> particular, for pg_dump, it isn't guaranteeing that the new version's
> extension is exactly like what was in the old database.  And that's not
> a bad thing, in this context; it's more or less the Whole Point.

In fact it's not either one or the other, because the CREATE EXTENSION
is providing the meta data, which includes an optional upgrade
function. So if you INSTALL EXTENSION over an existing one, and meantime
you've been installing the new version (file system install, PGAN or
distro packaged or source level install; then the new CREATE EXTENSION
which should be given in the foo.sql for the foo EXTENSION), in this
case it's an upgrade, and what INSTALL EXTENSION is meant to do is run
the upgrade function with as arguments current and new version numbers.

It's when the EXTENSION is not providing this upgrade function that the
behavior is more CREATE OR REPLACE, because it'd then run the
installation script all over again.

In case you provided an upgrade function, we're yet to see how to
provide facilities to the extensions authors in order to easily address
the columns of their data type and the indexes from their operator
classes, etc.

Regards,
-- 
dim

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T17:45:24Z

Dimitri Fontaine wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > Well, that isn't really going to help us in terms of what to do for 9.0.
> > But the possibility that something like this might happen in future is
> > one thing that makes me hesitant about extending CREATE LANGUAGE right
> > now --- the more bells and whistles we put on it, the harder it will be
> > to have a clean upgrade to an EXTENSION facility.
> 
> Agreed, but we could still evolve the command with keeping an eye on the
> future. As of now I intend to implement what's on this page:
> 
>   http://wiki.postgresql.org/wiki/ExtensionPackaging
> 
> So maybe a quick glance then some early design approval would make it
> possible to change the CREATE LANGUAGE in an EXTENSION compatible way.
> 
> > One thing that strikes me about your proposal is that INSTALL EXTENSION
> > doesn't sound like a CREATE OR REPLACE operation.  It sounds like a
> > CREATE IF NOT EXISTS operation, because there simply is not a guarantee
> > that what gets installed is exactly what the user expected --- in
> > particular, for pg_dump, it isn't guaranteeing that the new version's
> > extension is exactly like what was in the old database.  And that's not
> > a bad thing, in this context; it's more or less the Whole Point.
> 
> In fact it's not either one or the other, because the CREATE EXTENSION
> is providing the meta data, which includes an optional upgrade
> function. So if you INSTALL EXTENSION over an existing one, and meantime
> you've been installing the new version (file system install, PGAN or
> distro packaged or source level install; then the new CREATE EXTENSION
> which should be given in the foo.sql for the foo EXTENSION), in this
> case it's an upgrade, and what INSTALL EXTENSION is meant to do is run
> the upgrade function with as arguments current and new version numbers.
> 
> It's when the EXTENSION is not providing this upgrade function that the
> behavior is more CREATE OR REPLACE, because it'd then run the
> installation script all over again.
> 
> In case you provided an upgrade function, we're yet to see how to
> provide facilities to the extensions authors in order to easily address
> the columns of their data type and the indexes from their operator
> classes, etc.

This discussion is sounding very design-ish, which makes me think we
should just leave things unchanged for 9.0 and have external regression
test designers work around this problem in their Makefiles, as Alvaro
suggested.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-20T17:49:55Z

Bruce Momjian <bruce@momjian.us> writes:
> This discussion is sounding very design-ish, which makes me think we
> should just leave things unchanged for 9.0 and have external regression
> test designers work around this problem in their Makefiles, as Alvaro
> suggested.

I would have said that some time ago, except that I think we have a
"must fix" issue here: isn't pg_upgrade broken for any database
containing plpgsql?  A decent solution for that probably will allow
something to fall out for the regression test problem too.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T17:57:55Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > This discussion is sounding very design-ish, which makes me think we
> > should just leave things unchanged for 9.0 and have external regression
> > test designers work around this problem in their Makefiles, as Alvaro
> > suggested.
> 
> I would have said that some time ago, except that I think we have a
> "must fix" issue here: isn't pg_upgrade broken for any database
> containing plpgsql?  A decent solution for that probably will allow
> something to fall out for the regression test problem too.

Uh, well, I added this to pg_dump.c for 9.0:

    else if (g_fout->remoteVersion >= 80300)
    {
        /* pg_language has a lanowner column */
        /* pg_language has a lanowner column */
        appendPQExpBuffer(query, "SELECT tableoid, oid, "
                          "lanname, lanpltrusted, lanplcallfoid, "
                          "lanvalidator,  lanacl, "
                          "(%s lanowner) AS lanowner "
                          "FROM pg_language "
                          "WHERE lanispl%s "
                          "ORDER BY oid",
                          username_subquery,
                          binary_upgrade ? "\nAND lanname != 'plpgsql'" : "");
                          ---------------------------------------------------

meaning it will not dump plpsql when doing a binary upgrade.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-20T18:40:34Z

On Feb 20, 2010, at 9:49 AM, Tom Lane wrote:

>> This discussion is sounding very design-ish, which makes me think we
>> should just leave things unchanged for 9.0 and have external regression
>> test designers work around this problem in their Makefiles, as Alvaro
>> suggested.
> 
> I would have said that some time ago, except that I think we have a
> "must fix" issue here: isn't pg_upgrade broken for any database
> containing plpgsql?  A decent solution for that probably will allow
> something to fall out for the regression test problem too.

There is also the "must fix" issue with pg_regress.

Thanks,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T19:03:08Z

David E. Wheeler wrote:
> On Feb 20, 2010, at 9:49 AM, Tom Lane wrote:
> 
> >> This discussion is sounding very design-ish, which makes me think we
> >> should just leave things unchanged for 9.0 and have external regression
> >> test designers work around this problem in their Makefiles, as Alvaro
> >> suggested.
> > 
> > I would have said that some time ago, except that I think we have a
> > "must fix" issue here: isn't pg_upgrade broken for any database
> > containing plpgsql?  A decent solution for that probably will allow
> > something to fall out for the regression test problem too.
> 
> There is also the "must fix" issue with pg_regress.

Why?  My pg_regress runs just fine.  If you are talking about 3rd party
modules, I suggested conditional Makefile rules.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-20T19:25:56Z

On Feb 20, 2010, at 11:03 AM, Bruce Momjian wrote:

>> There is also the "must fix" issue with pg_regress.
> 
> Why?  My pg_regress runs just fine.  If you are talking about 3rd party
> modules, I suggested conditional Makefile rules.

Because you can either make the simple change to pg_regress that David Fetter sent yesterday and have things continue to work, or you can break a slew of third-party module test suites (and possibly modules, as well) and make a lot of other people do a lot more work, not to mention email -hackers and ask WTF happened because they may well not know.

I think that not changing pg_regress is more work for third-party module maintainers *and* more work for the Pg community when those maintainers come asking what happened and for advice on how to fix it.

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T19:51:28Z

David E. Wheeler wrote:
> On Feb 20, 2010, at 11:03 AM, Bruce Momjian wrote:
> 
> >> There is also the "must fix" issue with pg_regress.
> >
> > Why?  My pg_regress runs just fine.  If you are talking about 3rd party
> > modules, I suggested conditional Makefile rules.
> 
> Because you can either make the simple change to pg_regress that
> David Fetter sent yesterday and have things continue to work,
> or you can break a slew of third-party module test suites (and
> possibly modules, as well) and make a lot of other people do a
> lot more work, not to mention email -hackers and ask WTF happened
> because they may well not know.
> 
> I think that not changing pg_regress is more work for third-party
> module maintainers *and* more work for the Pg community when
> those maintainers come asking what happened and for advice on
> how to fix it.

Well, I was asking why you labeled it "must fix" rather than "should
fix".  I am fine with the pg_regress.c change.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-20T20:45:19Z

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> I would have said that some time ago, except that I think we have a
>> "must fix" issue here: isn't pg_upgrade broken for any database
>> containing plpgsql?  A decent solution for that probably will allow
>> something to fall out for the regression test problem too.

> Uh, well, I added this to pg_dump.c for 9.0:

That's a crock that needs to go away ASAP.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T20:57:58Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> I would have said that some time ago, except that I think we have a
> >> "must fix" issue here: isn't pg_upgrade broken for any database
> >> containing plpgsql?  A decent solution for that probably will allow
> >> something to fall out for the regression test problem too.
> 
> > Uh, well, I added this to pg_dump.c for 9.0:
> 
> That's a crock that needs to go away ASAP.

Well, it works, so you are going to need to explain it.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-20T21:59:56Z

On Sat, Feb 20, 2010 at 2:51 PM, Bruce Momjian <bruce@momjian.us> wrote:
> David E. Wheeler wrote:
>> On Feb 20, 2010, at 11:03 AM, Bruce Momjian wrote:
>>
>> >> There is also the "must fix" issue with pg_regress.
>> >
>> > Why?  My pg_regress runs just fine.  If you are talking about 3rd party
>> > modules, I suggested conditional Makefile rules.
>>
>> Because you can either make the simple change to pg_regress that
>> David Fetter sent yesterday and have things continue to work,
>> or you can break a slew of third-party module test suites (and
>> possibly modules, as well) and make a lot of other people do a
>> lot more work, not to mention email -hackers and ask WTF happened
>> because they may well not know.
>>
>> I think that not changing pg_regress is more work for third-party
>> module maintainers *and* more work for the Pg community when
>> those maintainers come asking what happened and for advice on
>> how to fix it.
>
> Well, I was asking why you labeled it "must fix" rather than "should
> fix".  I am fine with the pg_regress.c change.

Yeah, if it makes life easier for other people, I say we go for it.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-20T22:53:31Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Feb 20, 2010 at 2:51 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> Well, I was asking why you labeled it "must fix" rather than "should
>> fix". I am fine with the pg_regress.c change.

> Yeah, if it makes life easier for other people, I say we go for it.

I don't think that the way to fix this is to have an ugly kluge in
pg_dump and another ugly kluge in pg_regress (and no doubt ugly kluges
elsewhere by the time all the dust settles).

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-20T22:58:11Z

On Sat, Feb 20, 2010 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, Feb 20, 2010 at 2:51 PM, Bruce Momjian <bruce@momjian.us> wrote:
>>> Well, I was asking why you labeled it "must fix" rather than "should
>>> fix".  I am fine with the pg_regress.c change.
>
>> Yeah, if it makes life easier for other people, I say we go for it.
>
> I don't think that the way to fix this is to have an ugly kluge in
> pg_dump and another ugly kluge in pg_regress (and no doubt ugly kluges
> elsewhere by the time all the dust settles).

IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE LANGUAGE
and (2) revert the original patch.  Do you want to do one of those
(which?) or do you have another idea?

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-20T23:03:22Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Feb 20, 2010 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think that the way to fix this is to have an ugly kluge in
>> pg_dump and another ugly kluge in pg_regress (and no doubt ugly kluges
>> elsewhere by the time all the dust settles).

> IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE LANGUAGE
> and (2) revert the original patch.  Do you want to do one of those
> (which?) or do you have another idea?

Well, I'm willing to implement CREATE OR REPLACE LANGUAGE if people
are agreed that that's a reasonable fix.  I'm slightly worried about
the restore-could-change-ownership issue, but I think that's much less
likely to cause problems than embedding special cases for plpgsql in a
pile of places that we'll never find again.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T23:16:47Z

Robert Haas wrote:
> On Sat, Feb 20, 2010 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Sat, Feb 20, 2010 at 2:51 PM, Bruce Momjian <bruce@momjian.us> wrote:
> >>> Well, I was asking why you labeled it "must fix" rather than "should
> >>> fix". ?I am fine with the pg_regress.c change.
> >
> >> Yeah, if it makes life easier for other people, I say we go for it.
> >
> > I don't think that the way to fix this is to have an ugly kluge in
> > pg_dump and another ugly kluge in pg_regress (and no doubt ugly kluges
> > elsewhere by the time all the dust settles).
> 
> IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE LANGUAGE
> and (2) revert the original patch.  Do you want to do one of those
> (which?) or do you have another idea?

For #2, if you mean the pg_dump.c plpgsql hack for pg_migrator, that is
not an option unless you want to break pg_migrator for 9.0.

If you implement #1, why would you have pg_dump issue CREATE OR REPLACE
LANGUAGE?  We don't do the "OR REPLACE" part for any other object I can
think of, so why would pg_dump do it for languages by default?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Andrew Dunstan <andrew@dunslane.net> — 2010-02-20T23:18:04Z

Robert Haas wrote:
>
> IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE LANGUAGE
> and (2) revert the original patch.  Do you want to do one of those
> (which?) or do you have another idea?
>
>
>   

I thought we seemed to be converging on some agreement on CREATE OR 
REPLACE LANGUAGE.

If not, let me add my vote for that.

I also think we need to state explicitly that module authors can not 
expect build files to be version ignorant and always work. Even if we do 
something that handles this particular issue, that is likely to be a 
happy coincidence rather than something that can be expected all the 
time. People need to expect to have to do version-dependent things.

cheers

andrew

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-20T23:18:04Z

Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sat, Feb 20, 2010 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I don't think that the way to fix this is to have an ugly kluge in
> >> pg_dump and another ugly kluge in pg_regress (and no doubt ugly kluges
> >> elsewhere by the time all the dust settles).
> 
> > IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE LANGUAGE
> > and (2) revert the original patch.  Do you want to do one of those
> > (which?) or do you have another idea?
> 
> Well, I'm willing to implement CREATE OR REPLACE LANGUAGE if people
> are agreed that that's a reasonable fix.  I'm slightly worried about
> the restore-could-change-ownership issue, but I think that's much less
> likely to cause problems than embedding special cases for plpgsql in a
> pile of places that we'll never find again.

All binary upgrade code is clearly marked as binary_upgrade (in fact you
complained about my marking them more clearly in tqual.c), so I don't
think we are going to lose it.  I have answered the other questions by
replying to Robert Haas.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David Christensen <david@endpoint.com> — 2010-02-20T23:42:53Z

On Feb 20, 2010, at 5:16 PM, Bruce Momjian wrote:

> Robert Haas wrote:
>> On Sat, Feb 20, 2010 at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> On Sat, Feb 20, 2010 at 2:51 PM, Bruce Momjian <bruce@momjian.us>  
>>>> wrote:
>>>>> Well, I was asking why you labeled it "must fix" rather than  
>>>>> "should
>>>>> fix". ?I am fine with the pg_regress.c change.
>>>
>>>> Yeah, if it makes life easier for other people, I say we go for it.
>>>
>>> I don't think that the way to fix this is to have an ugly kluge in
>>> pg_dump and another ugly kluge in pg_regress (and no doubt ugly  
>>> kluges
>>> elsewhere by the time all the dust settles).
>>
>> IMO, the non-ugly kludges are (1) implement CREATE OR REPLACE  
>> LANGUAGE
>> and (2) revert the original patch.  Do you want to do one of those
>> (which?) or do you have another idea?
>
> For #2, if you mean the pg_dump.c plpgsql hack for pg_migrator, that  
> is
> not an option unless you want to break pg_migrator for 9.0.
>
> If you implement #1, why would you have pg_dump issue CREATE OR  
> REPLACE
> LANGUAGE?  We don't do the "OR REPLACE" part for any other object I  
> can
> think of, so why would pg_dump do it for languages by default?

In what cases would one be able to meaningfully REPLACE a language,  
other than to not break when encountering an already installed  
language?  i.e., in which cases would this not invalidate functions  
already written if you were changing from trusted to untrusted status  
or a different call handler, etc.  If there is not a meaningful case  
for the OR REPLACE, and it is just a syntactic loophole to allow the  
errorless recreation of an existing language and if the parameters for  
the CREATE LANGUAGE call indicate identical final state, why aren't we  
free change change the semantics of CREATE LANGUAGE to just issue a  
NOTIFY instead of an error in that case, and only complain if there  
are differences in the call handler, trusted status, etc?

I am including a preliminary patch to implement this behavior in the  
pg_pltemplate case; since we are already using the defaults from that  
entry and ignoring any explicitly provided ones in the command, this  
seems to be a safe assumption.  Presumably you could do the same in  
the other case, if you verified that the existing pg_language tuple  
had the same relevant fields (i.e., notify without error).

This would have the benefit of allowing CREATE LANGUAGE <langname> for  
those languages with pg_pltemplate entries (specifically plpgsql, but  
any with the same parameters) and would mean that we could use dumps  
from pre 9.0 in 9.0 without breaking, appears to fix --single, the  
pg_regress case, etc.  Thoughts on the approach?

Regards,

David
--
David Christensen
End Point Corporation
david@endpoint.com

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-20T23:52:54Z

On Feb 20, 2010, at 15:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Well, I'm willing to implement CREATE OR REPLACE LANGUAGE if people
> are agreed that that's a reasonable fix.  I'm slightly worried about
> the restore-could-change-ownership issue, but I think that's much less
> likely to cause problems than embedding special cases for plpgsql in a
> pile of places that we'll never find again.

Just throwing this out there: would a syntax such as CREATE OF NOT  
EXISTS, a complement to DROP IF EXISTS, avoid the permissions issue?  
And if so, would that be a syntax we'd want to accept in general?  
Could the be a CREATE IF NOT EXISTS TABLE?

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-20T23:55:47Z

On Feb 20, 2010, at 15:18, Andrew Dunstan <andrew@dunslane.net> wrote:

> I also think we need to state explicitly that module authors can not  
> expect build files to be version ignorant and always work. Even if  
> we do something that handles this particular issue, that is likely  
> to be a happy coincidence rather than something that can be expected  
> all the time. People need to expect to have to do version-dependent  
> things.

Sure, but where it cab easily be avoided it should be. Most Makefiles  
work as-is back to 8.0 and earlier AFAICT.

Best,

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-20T23:57:02Z

"David E. Wheeler" <david@kineticode.com> writes:
> Just throwing this out there: would a syntax such as CREATE OF NOT  
> EXISTS, a complement to DROP IF EXISTS, avoid the permissions issue?  

No, it'd just move it to a different place: now you risk breaking
the restored state rather than pre-existing state.

> And if so, would that be a syntax we'd want to accept in general?  
> Could the be a CREATE IF NOT EXISTS TABLE?

*Please* go read some of the linked older discussions before you propose
that.  I don't want to rehash it yet again.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David E. Wheeler <david@kineticode.com> — 2010-02-21T00:37:42Z

On Feb 20, 2010, at 3:57 PM, Tom Lane wrote:

>> And if so, would that be a syntax we'd want to accept in general?  
>> Could the be a CREATE IF NOT EXISTS TABLE?
> 
> *Please* go read some of the linked older discussions before you propose
> that.  I don't want to rehash it yet again.

:-) Was just a thought.

David

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-21T00:59:35Z

David Christensen <david@endpoint.com> writes:
> On Feb 20, 2010, at 5:16 PM, Bruce Momjian wrote:
>> If you implement #1, why would you have pg_dump issue CREATE OR
>> REPLACE LANGUAGE?  We don't do the "OR REPLACE" part for any other
>> object I can think of, so why would pg_dump do it for languages by
>> default?

> In what cases would one be able to meaningfully REPLACE a language,  
> other than to not break when encountering an already installed  
> language?

I'm getting the distinct impression that Bruce didn't read yesterday's
portion of this thread...

The proposal that I had in mind was to have pg_dump use OR REPLACE
only when emitting a parameterless CREATE LANGUAGE.  This would
guarantee that both the desired new language definition and any
pre-existing one that it could replace would be exactly like the
pg_pltemplate entry for the language.  The only risk is that the
restore would force the language's ownership and permissions to be
what they'd been in the source database, which might possibly not
be desirable.  Then again it might be desirable; it's really hard
to decide that without a specific scenario in mind.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-21T01:11:50Z

Tom Lane wrote:
> David Christensen <david@endpoint.com> writes:
> > On Feb 20, 2010, at 5:16 PM, Bruce Momjian wrote:
> >> If you implement #1, why would you have pg_dump issue CREATE OR
> >> REPLACE LANGUAGE?  We don't do the "OR REPLACE" part for any other
> >> object I can think of, so why would pg_dump do it for languages by
> >> default?
> 
> > In what cases would one be able to meaningfully REPLACE a language,  
> > other than to not break when encountering an already installed  
> > language?
> 
> I'm getting the distinct impression that Bruce didn't read yesterday's
> portion of this thread...
> 
> The proposal that I had in mind was to have pg_dump use OR REPLACE
> only when emitting a parameterless CREATE LANGUAGE.  This would
> guarantee that both the desired new language definition and any
> pre-existing one that it could replace would be exactly like the
> pg_pltemplate entry for the language.  The only risk is that the
> restore would force the language's ownership and permissions to be
> what they'd been in the source database, which might possibly not
> be desirable.  Then again it might be desirable; it's really hard
> to decide that without a specific scenario in mind.

Yea, I did read it, but I was just unclear how adding OR REPLACE wasn't
just moving the hack to another location, and because there was so much
discussion about OR REPLACE, it felt like we were designing the feature,
which is something we don't want to be doing now.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-21T03:38:34Z

On Sat, Feb 20, 2010 at 6:42 PM, David Christensen <david@endpoint.com> wrote:
> In what cases would one be able to meaningfully REPLACE a language, other
> than to not break when encountering an already installed language?  i.e., in
> which cases would this not invalidate functions already written if you were
> changing from trusted to untrusted status or a different call handler, etc.

At the risk of being smart, who cares and why is this our problem?
This question has been asked before, and I can't really figure out
what is behind the asking of it.  It is as if someone imagines that
you would install plperl and then come along later and change the
handlers to, say, the plpython handlers, and then complain that your
functions were all broken.  But that is obviously stupid, and no one
would do it (or if they did, we would laugh at them).

I think the most likely use of CREATE OR REPLACE FUNCTION is to avoid
an error when creating a language that might already exist.  But it
doesn't seem like the only possible use.  Perhaps you've done an
in-place upgrade to version 9.0 and you'd like to add an inline
handler, for example.

>  If there is not a meaningful case for the OR REPLACE, and it is just a
> syntactic loophole to allow the errorless recreation of an existing language
> and if the parameters for the CREATE LANGUAGE call indicate identical final
> state, why aren't we free change change the semantics of CREATE LANGUAGE to
> just issue a NOTIFY instead of an error in that case, and only complain if
> there are differences in the call handler, trusted status, etc?

I guess we could do that, but it's inconsistent with the way we handle
other object types, so it doesn't seem as clean to me.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-21T03:56:10Z

Robert Haas <robertmhaas@gmail.com> writes:
> I think the most likely use of CREATE OR REPLACE [LANGUAGE] is to avoid
> an error when creating a language that might already exist.  But it
> doesn't seem like the only possible use.  Perhaps you've done an
> in-place upgrade to version 9.0 and you'd like to add an inline
> handler, for example.

Given the existing semantics of CREATE LANGUAGE, nothing at all would
happen when replacing a language that has a pg_pltemplate entry, because
that overrides all the command's options.  However, I think CORL has
potential use for developers working on a non-core language
implementation: they could use it to add an inline handler, for example,
without losing the function definitions they already have loaded.

Admittedly that's a pretty thin use-case.  However, I intensely dislike
the line of thought that says "let's take shortcuts because nobody is
going to use this except for one specific use-case".  There is a very
clear set of behaviors that CORL ought to have given the precedents of
our other COR commands.  If we don't make it do things that way then we
are going to surprise users, and we are also going to paint ourselves
into a corner because we won't be able to fix it later without creating
compatibility gotchas.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-21T04:08:58Z

On Feb 20, 2010, at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I think the most likely use of CREATE OR REPLACE [LANGUAGE] is to
>> avoid
>> an error when creating a language that might already exist.  But it
>> doesn't seem like the only possible use.  Perhaps you've done an
>> in-place upgrade to version 9.0 and you'd like to add an inline
>> handler, for example.
>
> Given the existing semantics of CREATE LANGUAGE, nothing at all would
> happen when replacing a language that has a pg_pltemplate entry,
> because
> that overrides all the command's options.  However, I think CORL has
> potential use for developers working on a non-core language
> implementation: they could use it to add an inline handler, for
> example,
> without losing the function definitions they already have loaded.
>
> Admittedly that's a pretty thin use-case.  However, I intensely
> dislike
> the line of thought that says "let's take shortcuts because nobody is
> going to use this except for one specific use-case".  There is a very
> clear set of behaviors that CORL ought to have given the precedents of
> our other COR commands.  If we don't make it do things that way then
> we
> are going to surprise users, and we are also going to paint ourselves
> into a corner because we won't be able to fix it later without
> creating
> compatibility gotchas.

Exactly.  I agree completely.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-21T17:29:10Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Feb 20, 2010, at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There is a very clear set of behaviors that CORL ought to have given
>> the precedents of our other COR commands.  If we don't make it do
>> things that way then we are going to surprise users, and we are also
>> going to paint ourselves into a corner because we won't be able to
>> fix it later without creating compatibility gotchas.

> Exactly.  I agree completely.

Attached is a draft patch (no doc changes) that implements CREATE OR
REPLACE LANGUAGE following the semantics used in CREATE OR REPLACE
FUNCTION, namely that in addition to whatever privileges you need to
do the CREATE, you need to be owner of the existing entry if any;
and the recorded ownership and permissions don't change.  It's not bad
at all --- net addition of 40 lines.  So if we want to go at it this
way, it's certainly feasible.

I've got mixed feelings about the ownership check.  If you get past
the normal CREATE LANGUAGE permission checks, then either you are
superuser, or you are database owner and you are trying to recreate
a language from a pg_pltemplate entry with tmpldbacreate true.
So it would fail only for a database owner who's trying to do
C.O.R.L. on a superuser-installed language.  Which arguably is a case
we ought to allow.  On the other hand, the case where not throwing an
error would really matter is in trying to do pg_restore --single, and
in that case even if we allowed the C.O.R.L. it would still spit up on
the ALTER LANGUAGE OWNER that pg_dump is presumably going to emit right
afterwards (except if using --no-owner, I guess).  So I'm not sure
we'd really be gaining much by omitting the ownership check, and it
would certainly be less consistent with other C.O.R. commands if we
don't apply such a check.

Comments?

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-21T18:09:22Z

On Sun, Feb 21, 2010 at 12:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Feb 20, 2010, at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> There is a very clear set of behaviors that CORL ought to have given
>>> the precedents of our other COR commands.  If we don't make it do
>>> things that way then we are going to surprise users, and we are also
>>> going to paint ourselves into a corner because we won't be able to
>>> fix it later without creating compatibility gotchas.
>
>> Exactly.  I agree completely.
>
> Attached is a draft patch (no doc changes) that implements CREATE OR
> REPLACE LANGUAGE following the semantics used in CREATE OR REPLACE
> FUNCTION, namely that in addition to whatever privileges you need to
> do the CREATE, you need to be owner of the existing entry if any;
> and the recorded ownership and permissions don't change.  It's not bad
> at all --- net addition of 40 lines.  So if we want to go at it this
> way, it's certainly feasible.
>
> I've got mixed feelings about the ownership check.  If you get past
> the normal CREATE LANGUAGE permission checks, then either you are
> superuser, or you are database owner and you are trying to recreate
> a language from a pg_pltemplate entry with tmpldbacreate true.
> So it would fail only for a database owner who's trying to do
> C.O.R.L. on a superuser-installed language.  Which arguably is a case
> we ought to allow.  On the other hand, the case where not throwing an
> error would really matter is in trying to do pg_restore --single, and
> in that case even if we allowed the C.O.R.L. it would still spit up on
> the ALTER LANGUAGE OWNER that pg_dump is presumably going to emit right
> afterwards (except if using --no-owner, I guess).  So I'm not sure
> we'd really be gaining much by omitting the ownership check, and it
> would certainly be less consistent with other C.O.R. commands if we
> don't apply such a check.
>
> Comments?

Well, I'm a big fan of CREATE OR REPLACE anything so I like the patch
regardless of whether it solves the current problem, but having said
that, I'm not clear on whether it does in fact solve the current
problem.  When PL/pgsql is installed by default, is it going to end up
owned by the DB owner, or might it end up owned by the superuser?

If you end up applying this you might take the to fix up the gram.y
comment a little more thoroughly: CREATE [OR REPLACE] [TRUSTED]
[PROCEDURAL] LANGUAGE; DROP [PROCEDURAL] LANGUAGE.

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-21T18:21:23Z

Robert Haas <robertmhaas@gmail.com> writes:
> Well, I'm a big fan of CREATE OR REPLACE anything so I like the patch
> regardless of whether it solves the current problem, but having said
> that, I'm not clear on whether it does in fact solve the current
> problem.  When PL/pgsql is installed by default, is it going to end up
> owned by the DB owner, or might it end up owned by the superuser?

It will be owned by the bootstrap superuser, so the case is exactly
the one that a non-superuser DBA would be faced with.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Robert Haas <robertmhaas@gmail.com> — 2010-02-21T18:40:25Z

On Sun, Feb 21, 2010 at 1:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Well, I'm a big fan of CREATE OR REPLACE anything so I like the patch
>> regardless of whether it solves the current problem, but having said
>> that, I'm not clear on whether it does in fact solve the current
>> problem.  When PL/pgsql is installed by default, is it going to end up
>> owned by the DB owner, or might it end up owned by the superuser?
>
> It will be owned by the bootstrap superuser, so the case is exactly
> the one that a non-superuser DBA would be faced with.

Or even a superuser other than the bootstrap superuser, no?  I dump
out the DB on my 8.4 server and try to reload on 9.0 with --single and
it fails because, even though I'm a superuser, I can't replace a
language owned by someone else?

...Robert

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-21T19:25:28Z

Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Feb 20, 2010, at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> There is a very clear set of behaviors that CORL ought to have given
> >> the precedents of our other COR commands.  If we don't make it do
> >> things that way then we are going to surprise users, and we are also
> >> going to paint ourselves into a corner because we won't be able to
> >> fix it later without creating compatibility gotchas.
> 
> > Exactly.  I agree completely.
> 
> Attached is a draft patch (no doc changes) that implements CREATE OR
> REPLACE LANGUAGE following the semantics used in CREATE OR REPLACE
> FUNCTION, namely that in addition to whatever privileges you need to
> do the CREATE, you need to be owner of the existing entry if any;
> and the recorded ownership and permissions don't change.  It's not bad
> at all --- net addition of 40 lines.  So if we want to go at it this
> way, it's certainly feasible.
> 
> I've got mixed feelings about the ownership check.  If you get past
> the normal CREATE LANGUAGE permission checks, then either you are
> superuser, or you are database owner and you are trying to recreate
> a language from a pg_pltemplate entry with tmpldbacreate true.
> So it would fail only for a database owner who's trying to do
> C.O.R.L. on a superuser-installed language.  Which arguably is a case
> we ought to allow.  On the other hand, the case where not throwing an
> error would really matter is in trying to do pg_restore --single, and
> in that case even if we allowed the C.O.R.L. it would still spit up on
> the ALTER LANGUAGE OWNER that pg_dump is presumably going to emit right
> afterwards (except if using --no-owner, I guess).  So I'm not sure
> we'd really be gaining much by omitting the ownership check, and it
> would certainly be less consistent with other C.O.R. commands if we
> don't apply such a check.

How is pg_migrator affected by this?  It always loads the the dump as
the super-user.  How will the pg_dump use CREATE OR REPLACE LANGUAGE?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-21T22:11:16Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Feb 21, 2010 at 1:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It will be owned by the bootstrap superuser, so the case is exactly
>> the one that a non-superuser DBA would be faced with.

> Or even a superuser other than the bootstrap superuser, no?  I dump
> out the DB on my 8.4 server and try to reload on 9.0 with --single and
> it fails because, even though I'm a superuser, I can't replace a
> language owned by someone else?

No, superusers always pass all permissions checks.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-21T22:16:54Z

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> Attached is a draft patch (no doc changes) that implements CREATE OR
>> REPLACE LANGUAGE

> How is pg_migrator affected by this?  It always loads the the dump as
> the super-user.  How will the pg_dump use CREATE OR REPLACE LANGUAGE?

pg_dump would issue "CREATE OR REPLACE LANGUAGE plpgsql" which would
succeed just fine, since it'd be issued by a superuser.

I think the potential downsides of that are significantly smaller than
having a special case that excludes plpgsql altogether --- for one
example, it would still succeed in a custom installation that had been
changed so that plpgsql wasn't installed by default.

BTW, another problem I just noticed with the current kluge is that it
fails to transfer any nondefault permissions that might have been
attached to plpgsql.

			regards, tom lane

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

Bruce Momjian <bruce@momjian.us> — 2010-02-22T00:23:34Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> Attached is a draft patch (no doc changes) that implements CREATE OR
> >> REPLACE LANGUAGE
> 
> > How is pg_migrator affected by this?  It always loads the the dump as
> > the super-user.  How will the pg_dump use CREATE OR REPLACE LANGUAGE?
> 
> pg_dump would issue "CREATE OR REPLACE LANGUAGE plpgsql" which would
> succeed just fine, since it'd be issued by a superuser.
> 
> I think the potential downsides of that are significantly smaller than
> having a special case that excludes plpgsql altogether --- for one
> example, it would still succeed in a custom installation that had been
> changed so that plpgsql wasn't installed by default.

Are we doing this just for plpgsql in pg_dump?

> BTW, another problem I just noticed with the current kluge is that it
> fails to transfer any nondefault permissions that might have been
> attached to plpgsql.

Well, I assumed the permissions would still come, just not the CREATE
LANGUAGE, but now that I think about it you might be right.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

Re: PGXS: REGRESS_OPTS=--load-language=plpgsql

David Fetter <david@fetter.org> — 2010-02-22T02:06:15Z

On Thu, Feb 18, 2010 at 01:51:08PM -0800, David Fetter wrote:
> Folks,
> 
> While hacking on PL/Parrot, I ran across an issue where when trying
> to load PL/pgsql, it's done unconditionally and fails.  How do we
> fix pg_regress to be a little more subtle about this?

For now, and for the archives, I've come up with this ugly hack:

REGRESS_OPTS = --dbname=$(PL_TESTDB)
NEEDS_PLPGSQL = $(shell psql -Atc "SELECT setting::int < 90000 FROM pg_catalog.pg_settings WHERE name='server_version_num'")
ifeq ($(NEEDS_PLPGSQL), "t")
REGRESS_OPTS += $(if $PG_VERSION < 90000," --load-language=plpgsql", "")
endif

That works all the way back to 8.2, and to be honest, I'm not all that
interested in making something that will work further back than that,
especially for new projects.

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