Thread: PGXS: REGRESS_OPTS=--load-language=plpgsql
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
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
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
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
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/
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
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
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
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
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/
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
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
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
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
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
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
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
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
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
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
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
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
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
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. +
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
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. +
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
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. +
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
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. +
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
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. +
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
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
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
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
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. +
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
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. +
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
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
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
"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
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
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
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. +
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
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
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
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
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) */
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
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
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
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. +
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
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
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. +
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