PGXS: REGRESS_OPTS=--load-language=plpgsql

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 plpgsql will of course already be installed.

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

Best,

David

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

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 is for 3rd-party test suites to detect what version they're being installed against.

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

Best,

David

2010/2/19 Tom Lane <tgl@sss.pgh.pa.us>:

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:

David Fetter <david@fetter.org> wrote:

support both pre-9.0 and post-9.0 PostgreSQLs.  David Wheeler has
suggested that we special-case PL/pgsql for 9.0 and greater, as it's
in template0, where those tests are based.

+1 for the CREATE LANGUAGE IF NOT EXISTS behavior.

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

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

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

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

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

David E. Wheeler wrote:

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

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

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

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

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

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

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

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

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

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

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

Best,

David

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

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

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

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

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

--- 1795,1804 ----
     */
    for (sl = loadlanguage; sl != NULL; sl = sl->next)
    {
!       if (sl->str != "plpgsql") {
!           header(_("installing %s"), sl->str);
!           psql_command(dbname, "CREATE LANGUAGE \"%s\"", sl->str);
!       }
    }
  }

Does that seem unreasonable?

Best,

David

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

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