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

PGXS: REGRESS_OPTS=--load-language=plpgsql

From
David Fetter
Date:
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

From
Tom Lane
Date:
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

From
"David E. Wheeler"
Date:
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
plpgsqlwill 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
runon 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
needto install plpgsql. 

Best,

David



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

From
David Fetter
Date:
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

Attachment

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

From
Euler Taveira de Oliveira
Date:
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

From
David Fetter
Date:
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

From
Takahiro Itagaki
Date:
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

From
Tom Lane
Date:
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

From
"David E. Wheeler"
Date:
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
isfor 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

From
Magnus Hagander
Date:
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/


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

From
Alvaro Herrera
Date:
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
isfor 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

From
"David E. Wheeler"
Date:
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
itis 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
toset regress_opts afterward. Also, there is no MAJORVERSION in earlier versions, so module authors would have to work
aroundthat. 

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

Best,

David



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

From
David E. Wheeler
Date:
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
effectsto set regress_opts afterward. Also, there is no MAJORVERSION in earlier versions, so module authors would have
towork around that. 
>
> Basically though, you're asking all third party module authors who depend on plpgsql in their code and/or tests to
modifytheir makefiles and release new versions to work around something that pg_regress could have fixed internally in
1-2lines 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

From
Robert Haas
Date:
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

From
David Fetter
Date:
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

Attachment

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

From
Tom Lane
Date:
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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

From
Dimitri Fontaine
Date:
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

From
Tom Lane
Date:
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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

From
Dimitri Fontaine
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
Tom Lane
Date:
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

From
Bruce Momjian
Date:
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
alanowner column */       appendPQExpBuffer(query, "SELECT tableoid, oid, "                         "lanname,
lanpltrusted,lanplcallfoid, "                         "lanvalidator,  lanacl, "                         "(%s lanowner)
ASlanowner "                         "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.comPG 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

From
"David E. Wheeler"
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
"David E. Wheeler"
Date:
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
towork, or you can break a slew of third-party module test suites (and possibly modules, as well) and make a lot of
otherpeople 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
communitywhen those maintainers come asking what happened and for advice on how to fix it. 

Best,

David



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

From
Bruce Momjian
Date:
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.comPG 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

From
Tom Lane
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
Andrew Dunstan
Date:

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

From
Bruce Momjian
Date:
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.comPG 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

From
David Christensen
Date:
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




Attachment

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

From
"David E. Wheeler"
Date:
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

From
"David E. Wheeler"
Date:
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

From
Tom Lane
Date:
"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

From
"David E. Wheeler"
Date:
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

From
Tom Lane
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

From
Robert Haas
Date:
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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

Index: src/backend/commands/proclang.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/proclang.c,v
retrieving revision 1.89
diff -c -r1.89 proclang.c
*** src/backend/commands/proclang.c    14 Feb 2010 18:42:14 -0000    1.89
--- src/backend/commands/proclang.c    21 Feb 2010 17:08:15 -0000
***************
*** 17,23 ****
  #include "access/heapam.h"
  #include "catalog/dependency.h"
  #include "catalog/indexing.h"
- #include "catalog/pg_authid.h"
  #include "catalog/pg_language.h"
  #include "catalog/pg_namespace.h"
  #include "catalog/pg_pltemplate.h"
--- 17,22 ----
***************
*** 49,55 ****
      char       *tmpllibrary;    /* path of shared library */
  } PLTemplate;

! static void create_proc_lang(const char *languageName,
                   Oid languageOwner, Oid handlerOid, Oid inlineOid,
                   Oid valOid, bool trusted);
  static PLTemplate *find_language_template(const char *languageName);
--- 48,54 ----
      char       *tmpllibrary;    /* path of shared library */
  } PLTemplate;

! static void create_proc_lang(const char *languageName, bool replace,
                   Oid languageOwner, Oid handlerOid, Oid inlineOid,
                   Oid valOid, bool trusted);
  static PLTemplate *find_language_template(const char *languageName);
***************
*** 73,88 ****
      Oid            funcargtypes[1];

      /*
!      * Translate the language name and check that this language doesn't
!      * already exist
       */
      languageName = case_translate_language_name(stmt->plname);

-     if (SearchSysCacheExists1(LANGNAME, PointerGetDatum(languageName)))
-         ereport(ERROR,
-                 (errcode(ERRCODE_DUPLICATE_OBJECT),
-                  errmsg("language \"%s\" already exists", languageName)));
-
      /*
       * If we have template information for the language, ignore the supplied
       * parameters (if any) and use the template information.
--- 72,81 ----
      Oid            funcargtypes[1];

      /*
!      * Translate the language name to lower case
       */
      languageName = case_translate_language_name(stmt->plname);

      /*
       * If we have template information for the language, ignore the supplied
       * parameters (if any) and use the template information.
***************
*** 232,238 ****
              valOid = InvalidOid;

          /* ok, create it */
!         create_proc_lang(languageName, GetUserId(), handlerOid, inlineOid,
                           valOid, pltemplate->tmpltrusted);
      }
      else
--- 225,232 ----
              valOid = InvalidOid;

          /* ok, create it */
!         create_proc_lang(languageName, stmt->replace, GetUserId(),
!                          handlerOid, inlineOid,
                           valOid, pltemplate->tmpltrusted);
      }
      else
***************
*** 306,312 ****
              valOid = InvalidOid;

          /* ok, create it */
!         create_proc_lang(languageName, GetUserId(), handlerOid, inlineOid,
                           valOid, stmt->pltrusted);
      }
  }
--- 300,307 ----
              valOid = InvalidOid;

          /* ok, create it */
!         create_proc_lang(languageName, stmt->replace, GetUserId(),
!                          handlerOid, inlineOid,
                           valOid, stmt->pltrusted);
      }
  }
***************
*** 315,321 ****
   * Guts of language creation.
   */
  static void
! create_proc_lang(const char *languageName,
                   Oid languageOwner, Oid handlerOid, Oid inlineOid,
                   Oid valOid, bool trusted)
  {
--- 310,316 ----
   * Guts of language creation.
   */
  static void
! create_proc_lang(const char *languageName, bool replace,
                   Oid languageOwner, Oid handlerOid, Oid inlineOid,
                   Oid valOid, bool trusted)
  {
***************
*** 323,341 ****
      TupleDesc    tupDesc;
      Datum        values[Natts_pg_language];
      bool        nulls[Natts_pg_language];
      NameData    langname;
      HeapTuple    tup;
      ObjectAddress myself,
                  referenced;

-     /*
-      * Insert the new language into pg_language
-      */
      rel = heap_open(LanguageRelationId, RowExclusiveLock);
!     tupDesc = rel->rd_att;

      memset(values, 0, sizeof(values));
      memset(nulls, false, sizeof(nulls));

      namestrcpy(&langname, languageName);
      values[Anum_pg_language_lanname - 1] = NameGetDatum(&langname);
--- 318,338 ----
      TupleDesc    tupDesc;
      Datum        values[Natts_pg_language];
      bool        nulls[Natts_pg_language];
+     bool        replaces[Natts_pg_language];
      NameData    langname;
+     HeapTuple    oldtup;
      HeapTuple    tup;
+     bool        is_update;
      ObjectAddress myself,
                  referenced;

      rel = heap_open(LanguageRelationId, RowExclusiveLock);
!     tupDesc = RelationGetDescr(rel);

+     /* Prepare data to be inserted */
      memset(values, 0, sizeof(values));
      memset(nulls, false, sizeof(nulls));
+     memset(replaces, true, sizeof(replaces));

      namestrcpy(&langname, languageName);
      values[Anum_pg_language_lanname - 1] = NameGetDatum(&langname);
***************
*** 347,370 ****
      values[Anum_pg_language_lanvalidator - 1] = ObjectIdGetDatum(valOid);
      nulls[Anum_pg_language_lanacl - 1] = true;

!     tup = heap_form_tuple(tupDesc, values, nulls);

!     simple_heap_insert(rel, tup);

      CatalogUpdateIndexes(rel, tup);

      /*
!      * Create dependencies for language
       */
      myself.classId = LanguageRelationId;
      myself.objectId = HeapTupleGetOid(tup);
      myself.objectSubId = 0;

      /* dependency on owner of language */
!     referenced.classId = AuthIdRelationId;
!     referenced.objectId = languageOwner;
!     referenced.objectSubId = 0;
!     recordSharedDependencyOn(&myself, &referenced, SHARED_DEPENDENCY_OWNER);

      /* dependency on the PL handler function */
      referenced.classId = ProcedureRelationId;
--- 344,405 ----
      values[Anum_pg_language_lanvalidator - 1] = ObjectIdGetDatum(valOid);
      nulls[Anum_pg_language_lanacl - 1] = true;

!     /* Check for pre-existing definition */
!     oldtup = SearchSysCache1(LANGNAME, PointerGetDatum(languageName));

!     if (HeapTupleIsValid(oldtup))
!     {
!         /* There is one; okay to replace it? */
!         if (!replace)
!             ereport(ERROR,
!                     (errcode(ERRCODE_DUPLICATE_OBJECT),
!                      errmsg("language \"%s\" already exists", languageName)));
!         if (!pg_language_ownercheck(HeapTupleGetOid(oldtup), languageOwner))
!             aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_LANGUAGE,
!                            languageName);

+         /*
+          * Do not change existing ownership or permissions.  Note
+          * dependency-update code below has to agree with this decision.
+          */
+         replaces[Anum_pg_language_lanowner - 1] = false;
+         replaces[Anum_pg_language_lanacl - 1] = false;
+
+         /* Okay, do it... */
+         tup = heap_modify_tuple(oldtup, tupDesc, values, nulls, replaces);
+         simple_heap_update(rel, &tup->t_self, tup);
+
+         ReleaseSysCache(oldtup);
+         is_update = true;
+     }
+     else
+     {
+         /* Creating a new language */
+         tup = heap_form_tuple(tupDesc, values, nulls);
+         simple_heap_insert(rel, tup);
+         is_update = false;
+     }
+
+     /* Need to update indexes for either the insert or update case */
      CatalogUpdateIndexes(rel, tup);

      /*
!      * Create dependencies for the new language.  If we are updating an
!      * existing language, first delete any existing pg_depend entries.
!      * (However, since we are not changing ownership or permissions, the
!      * shared dependencies do *not* need to change, and we leave them alone.)
       */
      myself.classId = LanguageRelationId;
      myself.objectId = HeapTupleGetOid(tup);
      myself.objectSubId = 0;

+     if (is_update)
+         deleteDependencyRecordsFor(myself.classId, myself.objectId);
+
      /* dependency on owner of language */
!     if (!is_update)
!         recordDependencyOnOwner(myself.classId, myself.objectId,
!                                 languageOwner);

      /* dependency on the PL handler function */
      referenced.classId = ProcedureRelationId;
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.462
diff -c -r1.462 copyfuncs.c
*** src/backend/nodes/copyfuncs.c    16 Feb 2010 22:34:43 -0000    1.462
--- src/backend/nodes/copyfuncs.c    21 Feb 2010 17:08:15 -0000
***************
*** 3237,3242 ****
--- 3237,3243 ----
  {
      CreatePLangStmt *newnode = makeNode(CreatePLangStmt);

+     COPY_SCALAR_FIELD(replace);
      COPY_STRING_FIELD(plname);
      COPY_NODE_FIELD(plhandler);
      COPY_NODE_FIELD(plinline);
Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.383
diff -c -r1.383 equalfuncs.c
*** src/backend/nodes/equalfuncs.c    16 Feb 2010 22:34:43 -0000    1.383
--- src/backend/nodes/equalfuncs.c    21 Feb 2010 17:08:15 -0000
***************
*** 1710,1715 ****
--- 1710,1716 ----
  static bool
  _equalCreatePLangStmt(CreatePLangStmt *a, CreatePLangStmt *b)
  {
+     COMPARE_SCALAR_FIELD(replace);
      COMPARE_STRING_FIELD(plname);
      COMPARE_NODE_FIELD(plhandler);
      COMPARE_NODE_FIELD(plinline);
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.710
diff -c -r2.710 gram.y
*** src/backend/parser/gram.y    17 Feb 2010 04:19:39 -0000    2.710
--- src/backend/parser/gram.y    21 Feb 2010 17:08:16 -0000
***************
*** 2882,2897 ****
  /*****************************************************************************
   *
   *        QUERIES :
!  *                CREATE PROCEDURAL LANGUAGE ...
   *                DROP PROCEDURAL LANGUAGE ...
   *
   *****************************************************************************/

  CreatePLangStmt:
!             CREATE opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
              {
                  CreatePLangStmt *n = makeNode(CreatePLangStmt);
!                 n->plname = $5;
                  /* parameters are all to be supplied by system */
                  n->plhandler = NIL;
                  n->plinline = NIL;
--- 2882,2898 ----
  /*****************************************************************************
   *
   *        QUERIES :
!  *                CREATE [OR REPLACE] PROCEDURAL LANGUAGE ...
   *                DROP PROCEDURAL LANGUAGE ...
   *
   *****************************************************************************/

  CreatePLangStmt:
!             CREATE opt_or_replace opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
              {
                  CreatePLangStmt *n = makeNode(CreatePLangStmt);
!                 n->replace = $2;
!                 n->plname = $6;
                  /* parameters are all to be supplied by system */
                  n->plhandler = NIL;
                  n->plinline = NIL;
***************
*** 2899,2913 ****
                  n->pltrusted = false;
                  $$ = (Node *)n;
              }
!             | CREATE opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
                HANDLER handler_name opt_inline_handler opt_validator
              {
                  CreatePLangStmt *n = makeNode(CreatePLangStmt);
!                 n->plname = $5;
!                 n->plhandler = $7;
!                 n->plinline = $8;
!                 n->plvalidator = $9;
!                 n->pltrusted = $2;
                  $$ = (Node *)n;
              }
          ;
--- 2900,2915 ----
                  n->pltrusted = false;
                  $$ = (Node *)n;
              }
!             | CREATE opt_or_replace opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
                HANDLER handler_name opt_inline_handler opt_validator
              {
                  CreatePLangStmt *n = makeNode(CreatePLangStmt);
!                 n->replace = $2;
!                 n->plname = $6;
!                 n->plhandler = $8;
!                 n->plinline = $9;
!                 n->plvalidator = $10;
!                 n->pltrusted = $3;
                  $$ = (Node *)n;
              }
          ;
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.430
diff -c -r1.430 parsenodes.h
*** src/include/nodes/parsenodes.h    16 Feb 2010 22:34:57 -0000    1.430
--- src/include/nodes/parsenodes.h    21 Feb 2010 17:08:16 -0000
***************
*** 1623,1628 ****
--- 1623,1629 ----
  typedef struct CreatePLangStmt
  {
      NodeTag        type;
+     bool        replace;        /* T => replace if already exists */
      char       *plname;            /* PL name */
      List       *plhandler;        /* PL call handler function (qual. name) */
      List       *plinline;        /* optional inline function (qual. name) */

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

From
Robert Haas
Date:
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

From
Tom Lane
Date:
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

From
Robert Haas
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
Tom Lane
Date:
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

From
Tom Lane
Date:
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

From
Bruce Momjian
Date:
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.comPG 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

From
David Fetter
Date:
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