[9.1] 2 bugs with extensions

I converted Skytools modules to extensions and found 2 problems:

1) Dumpable sequences are not supported - if sequence is tagged
with pg_catalog.pg_extension_config_dump(), the pg_dump tries
to run COPY on it.

2) If there is schema with functions, but nothing else,
then when later converting it to extension by adding
functions and schema to extension, later drop
of that extension fails to remove schema.
Proper CREATE EXT + DROP EXT works fine.

To reproduce, checkout 'master' branch and go directly
to module directory:

$ git clone --recursive git://github.com/markokr/skytools.git
$ cd skytools

1) $ cd sql/pgq && make install installcheck
..
pg_dump regression > test.dump
pg_dump: SQL command failed
pg_dump: Error message from server: ERROR: cannot copy from sequence
"batch_id_seq"
pg_dump: The command was: COPY pgq.batch_id_seq TO stdout;

2) $ cd sql/pgq_coop && make install installcheck
..
create extension pgq_coop from 'unpackaged';
drop extension pgq_coop;
create extension pgq_coop;
ERROR: schema "pgq_coop" already exists

--
marko

Hi,

After much distractions I've at least been able to do something about
that bug report.

Marko Kreen <markokr(at)gmail(dot)com> writes:
> 1) Dumpable sequences are not supported - if sequence is tagged
> with pg_catalog.pg_extension_config_dump(), the pg_dump tries
> to run COPY on it.

I can only reproduce that in 9.1. I first tried in HEAD where pg_dump
will just entirely skip the sequence, which is not the right answer
either, but at least does not spit out that message.

> pg_dump: Error message from server: ERROR: cannot copy from sequence
> "batch_id_seq"
> pg_dump: The command was: COPY pgq.batch_id_seq TO stdout;

Please find attached a patch that fixes it in 9.1, in all classic
pg_dump, --data-only and --schema-only.

git diff --stat
src/bin/pg_dump/pg_dump.c | 68 +++++++++++++++++++++++++++++++++++---------
1 files changed, 54 insertions(+), 14 deletions(-)

Once that's ok for 9.1, I'll get to work on a fix for master, oh and
look at what the situation is in 9.2, which I guess is the same as in
master actually.

> 2) If there is schema with functions, but nothing else,
> create extension pgq_coop from 'unpackaged';
> drop extension pgq_coop;
> create extension pgq_coop;
> ERROR: schema "pgq_coop" already exists

From reading the scripts, it's not clear to me, but it appears that the
schema existed before the create from unpackaged then got added to the
extension by way of

ALTER EXTENSION pgq_coop ADD SCHEMA pgq_coop;

Is that true? Can we work out a minimal example to reproduce the bug?
(The Makefile in skytools/sql/pgq_coop fails on my OS)

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

On Wed, Sep 26, 2012 at 5:36 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> After much distractions I've at least been able to do something about
> that bug report.

Thanks.

>> 2) If there is schema with functions, but nothing else,
>> create extension pgq_coop from 'unpackaged';
>> drop extension pgq_coop;
>> create extension pgq_coop;
>> ERROR: schema "pgq_coop" already exists
>
> From reading the scripts, it's not clear to me, but it appears that the
> schema existed before the create from unpackaged then got added to the
> extension by way of
>
> ALTER EXTENSION pgq_coop ADD SCHEMA pgq_coop;
>
> Is that true?

Yes.

> Can we work out a minimal example to reproduce the bug?

Yes, the above text or sql/pgq_coop/sql/pgq_coop_init_ext.sql

I guess you could replace pgq_coop with any extension just
consisting of just functions.

> (The Makefile in skytools/sql/pgq_coop fails on my OS)

How does it fail? Are you using gnu make? What version?

--
marko

Marko Kreen <markokr(at)gmail(dot)com> writes:
>> Can we work out a minimal example to reproduce the bug?
>
> Yes, the above text or sql/pgq_coop/sql/pgq_coop_init_ext.sql
>
> I guess you could replace pgq_coop with any extension just
> consisting of just functions.

I did just that, with a single function, and couldn't reproduce the
problem either in 9.1 nor in master, with relocatable = true then with
relocatable = false and schema = 'pg_catalog' as in your repository.

The extension has those files contents:

Makefile
EXTENSION = extschema
DATA = extschema--unpackaged--1.0.sql extschema.sql extschema--1.0.sql

PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)

extschema.control
# extschema extension
comment = 'debug extension schema handling'
default_version = '1.0'
relocatable = false
schema = 'pg_catalog'

extschema.sql
create schema ext;
create or replace function ext.extschema() returns int language sql
as $$ select 1; $$;

extschema--unpackaged--1.0.sql
alter extension extschema add schema ext;
alter extension extschema add function ext.extschema();

extschema--1.0.sql
create schema ext;
create or replace function ext.extschema() returns int language sql
as $$ select 1; $$;

So I guess that needs some more work to reproduce the bug.

>> (The Makefile in skytools/sql/pgq_coop fails on my OS)
>
> How does it fail? Are you using gnu make? What version?

I guess sed is the problem here, it's a BSD variant:

dim ~/skytools/sql/pgq_coop make pgq_coop--unpackaged--3.1.sql
sed: 1: "/^\\/{s/\\i //;p}\n": extra characters at the end of p command
sed: 1: "/^\\/{s/\\i //;p}\n": extra characters at the end of p command
sed: 1: "/^\\/{s/\\i //;p}\n": extra characters at the end of p command
cat pgq_coop.upgrade.sql > pgq_coop--unpackaged--3.1.sql

sed --version
sed: illegal option -- -
usage: sed script [-Ealn] [-i extension] [file ...]
sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...]

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

On Wed, Sep 26, 2012 at 7:15 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
>>> Can we work out a minimal example to reproduce the bug?
>>
>> Yes, the above text or sql/pgq_coop/sql/pgq_coop_init_ext.sql
>>
>> I guess you could replace pgq_coop with any extension just
>> consisting of just functions.
>
> I did just that, with a single function, and couldn't reproduce the
> problem either in 9.1 nor in master, with relocatable = true then with
> relocatable = false and schema = 'pg_catalog' as in your repository.

Indeed, after another makefile reorg, I could not reproduce it
on skytools master either, some digging showed that due
to a makefile bug ($< instead $^) the ADD SCHEMA
was missing from .sql file. So no bug in postgres.

>>> (The Makefile in skytools/sql/pgq_coop fails on my OS)
>>
>> How does it fail? Are you using gnu make? What version?
>
> I guess sed is the problem here, it's a BSD variant:

Could you test if Skytools git now works for you?

I replaced sed usage with awk there, perhaps that will be
more portable.

--
marko

Marko Kreen <markokr(at)gmail(dot)com> writes:
> Indeed, after another makefile reorg, I could not reproduce it
> on skytools master either, some digging showed that due
> to a makefile bug ($< instead $^) the ADD SCHEMA
> was missing from .sql file. So no bug in postgres.

That would explain, indeed :)

> Could you test if Skytools git now works for you?

It does:

dim ~/dev/skytools/sql/pgq_coop make pgq_coop--unpackaged--3.1.1.sql
../../scripts/catsql.py structure/upgrade.sql > pgq_coop.upgrade.sql
../../scripts/catsql.py pgq_coop.upgrade.sql structure/ext_unpackaged.sql structure/ext_postproc.sql > pgq_coop--unpackaged--3.1.1.sql
dim ~/dev/skytools/sql/pgq_coop make pgq_coop--3.1.1.sql
../../scripts/catsql.py structure/install.sql > pgq_coop.sql
../../scripts/catsql.py pgq_coop.sql structure/ext_postproc.sql >
pgq_coop--3.1.1.sql

> I replaced sed usage with awk there, perhaps that will be
> more portable.

I seem to recall needing to explicitly use `gawk` for some scripts,
depending on the features you want to have. Some systems default awk are
`mawk` or even some really old versions and don't have much features…

That said, it seems to work here, now. Thanks!

Regards,
--
Dimitri Fontaine 06 63 07 10 78
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Please find attached a patch that fixes it in 9.1, in all classic
> pg_dump, --data-only and --schema-only.

Same for 9.2, attached. master needs yet another patch because of the
recent headers reorg, it seems, that's for another day though.

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

Excerpts from Dimitri Fontaine's message of vie sep 28 17:36:41 -0300 2012:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> > Please find attached a patch that fixes it in 9.1, in all classic
> > pg_dump, --data-only and --schema-only.
>
> Same for 9.2, attached. master needs yet another patch because of the
> recent headers reorg, it seems, that's for another day though.

No, just remove the RELKIND_UNCATALOGUED case in that switch.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> Same for 9.2, attached. master needs yet another patch because of the
>> recent headers reorg, it seems, that's for another day though.
>
> No, just remove the RELKIND_UNCATALOGUED case in that switch.

Oh. As in the attached? :)

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

Excerpts from Dimitri Fontaine's message of lun oct 01 04:44:28 -0300 2012:
> Hi,
>
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> >> Same for 9.2, attached. master needs yet another patch because of the
> >> recent headers reorg, it seems, that's for another day though.
> >
> > No, just remove the RELKIND_UNCATALOGUED case in that switch.
>
> Oh. As in the attached? :)

That seems to work, yes.

While testing this out I noticed that this silently does nothing:

SELECT pg_catalog.pg_extension_config_dump('my_config', NULL);

i.e. the table is not marked as configuration but no error or warning
appears, either. This seems rather surprising.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Dimitri Fontaine's message of mié sep 26 11:36:38 -0300 2012:

> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > 1) Dumpable sequences are not supported - if sequence is tagged
> > with pg_catalog.pg_extension_config_dump(), the pg_dump tries
> > to run COPY on it.
>
> I can only reproduce that in 9.1. I first tried in HEAD where pg_dump
> will just entirely skip the sequence, which is not the right answer
> either, but at least does not spit out that message.
>
> > pg_dump: Error message from server: ERROR: cannot copy from sequence
> > "batch_id_seq"
> > pg_dump: The command was: COPY pgq.batch_id_seq TO stdout;
>
> Please find attached a patch that fixes it in 9.1, in all classic
> pg_dump, --data-only and --schema-only.
>
> git diff --stat
> src/bin/pg_dump/pg_dump.c | 68 +++++++++++++++++++++++++++++++++++---------
> 1 files changed, 54 insertions(+), 14 deletions(-)
>
> Once that's ok for 9.1, I'll get to work on a fix for master, oh and
> look at what the situation is in 9.2, which I guess is the same as in
> master actually.

I had a look at the patches submitted by Dimitri and in my tests they do
exactly what's intended, i.e. produce a data dump of the extension's
config sequences when necessary. However, after a couple of hours
trying to understand what the patches are *doing* I failed to figure it
out completely, and I'm afraid that there might be secondary side
effects that I'm failing to notice. So I'm punting this to Tom, who
seems to be the author of this code.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> No, just remove the RELKIND_UNCATALOGUED case in that switch.

> Oh. As in the attached?:)

I don't think you tested this patch in 9.2 or HEAD, because it bleats
like mad. I installed an extension containing

create sequence extseq;
select pg_catalog.pg_extension_config_dump('extseq', '');

into the regression database, and then did:

$ pg_dump -Fc regression >r.dump
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order
pg_dump: [archiver] WARNING: archive items not in correct section order

The reason is that it calls dumpSequence() to emit the SEQUENCE SET
archive item during table-data dumping, but the archive item gets marked
SECTION_PRE_DATA. As of 9.2 we have to be rigid about keeping those
section markings correct and in-sequence. This is not really right in
9.1 either (wouldn't be surprised if it breaks parallel restore).

The fact that SEQUENCE SET is considered pre-data has bitten us several
times already, eg
http://archives.postgresql.org/pgsql-bugs/2012-05/msg00084.php

I think it may be time to bite the bullet and change that (including
breaking dumpSequence() into two separate functions). I'm a little bit
worried about the compatibility implications of back-patching such a
change, though. Is it likely that anybody out there is depending on the
fact that, eg, pg_dump --section=pre-data currently includes SEQUENCE SET
items? Personally I think it's more likely that that'd be seen as a
bug, but ...

regards, tom lane

I wrote:
> The fact that SEQUENCE SET is considered pre-data has bitten us several
> times already, eg
> http://archives.postgresql.org/pgsql-bugs/2012-05/msg00084.php

> I think it may be time to bite the bullet and change that (including
> breaking dumpSequence() into two separate functions). I'm a little bit
> worried about the compatibility implications of back-patching such a
> change, though. Is it likely that anybody out there is depending on the
> fact that, eg, pg_dump --section=pre-data currently includes SEQUENCE SET
> items? Personally I think it's more likely that that'd be seen as a
> bug, but ...

Specifically, I'm thinking this, which looks rather bulky but most of
the diff is from reindenting the guts of dumpSequence().

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> I think it may be time to bite the bullet and change that (including
>> breaking dumpSequence() into two separate functions). I'm a little bit
>> worried about the compatibility implications of back-patching such a
>> change, though. Is it likely that anybody out there is depending on the
>> fact that, eg, pg_dump --section=pre-data currently includes SEQUENCE SET
>> items? Personally I think it's more likely that that'd be seen as a
>> bug, but ...

FWIW, +1

> Specifically, I'm thinking this, which looks rather bulky but most of
> the diff is from reindenting the guts of dumpSequence().

I see that you commited that patch, thanks a lot Tom!

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