Extensions not dumped when --schema is used

On Wed, 2020-05-20 at 10:06 +0200, Guillaume Lelarge wrote:

I've discovered something today that I didn't really expect.
When a user dumps a database with the --schema flag of pg_dump,
extensions in this schema aren't dumped. As far as I can tell,
the documentation isn't clear about this ("Dump only schemas
matching pattern; this selects both the schema itself, and all
its contained objects."), though the source code definitely is
("We dump all user-added extensions by default, or none of them
if include_everything is false (i.e., a --schema or --table
switch was given).", in pg_dump.c).

I was wondering the reason behind this choice. If anyone knows,
I'd be happy to hear about it.

I see two things:
* it's been overlooked, and we should dump all the extensions
available in a schema if this schema has been selected through
the --schema flag.
* it's kind of like the large objects handling, and I'd pretty
interested in adding a --extensions (as the same way there is a
--blobs flag).

I am not sure if there is a good reason for the current behavior,
but I'd favor the second solution. I think as extensions as belonging
to the database rather than the schema; the schema is just where the
objects are housed.

Yours,
Laurenz Albe

On 20 May 2020, at 10:06, Guillaume Lelarge <guillaume@lelarge.info> wrote:

I was wondering the reason behind this choice. If anyone knows, I'd be happy to hear about it.

Extensions were dumped unconditionally in the beginning, but it was changed to
match how procedural language definitions were dumped.

* it's been overlooked, and we should dump all the extensions available in a schema if this schema has been selected through the --schema flag.

For reference, --schema-only will include all the extensions, but not
--schema=foo and not "--schema-only --schema=foo".

Extensions don't belong to a schema per se, the namespace oid in pg_extension
marks where most of the objects are contained but not necessarily all of them.
Given that, it makes sense to not include extensions for --schema. However,
that can be considered sort of an implementation detail which may not be
entirely obvious to users (especially since you yourself is a power-user).

* it's kind of like the large objects handling, and I'd pretty interested in adding a --extensions (as the same way there is a --blobs flag).

An object in a schema might have attributes that depend on an extension in
order to restore, so it makes sense to provide a way to include them for a
--schema dump. The question is what --extensions should do: only dump any
extensions that objects in the schema depend on; require a pattern and only
dump matching extensions; dump all extensions (probably not) or something else?

cheers ./daniel

Le mer. 20 mai 2020 à 11:26, Daniel Gustafsson <daniel@yesql.se> a écrit :

On 20 May 2020, at 10:06, Guillaume Lelarge <guillaume@lelarge.info>

wrote:

I was wondering the reason behind this choice. If anyone knows, I'd be

happy to hear about it.

Extensions were dumped unconditionally in the beginning, but it was
changed to
match how procedural language definitions were dumped.

That makes sense. Thank you.

* it's been overlooked, and we should dump all the extensions available
in a schema if this schema has been selected through the --schema flag.

For reference, --schema-only will include all the extensions, but not
--schema=foo and not "--schema-only --schema=foo".

Yes.

Extensions don't belong to a schema per se, the namespace oid in

pg_extension
marks where most of the objects are contained but not necessarily all of
them.
Given that, it makes sense to not include extensions for --schema.
However,
that can be considered sort of an implementation detail which may not be
entirely obvious to users (especially since you yourself is a power-user).

I agree.

* it's kind of like the large objects handling, and I'd pretty interested
in adding a --extensions (as the same way there is a --blobs flag).

An object in a schema might have attributes that depend on an extension in
order to restore, so it makes sense to provide a way to include them for a
--schema dump.

That's actually how I figured this out. A customer can't restore his dump
because of a missing extension (pg_trgm to be precise).

The question is what --extensions should do: only dump any

extensions that objects in the schema depend on; require a pattern and only
dump matching extensions; dump all extensions (probably not) or something
else?

Actually, "dump all extensions" (#3) would make sense to me, and has my
vote. Otherwise, and though it would imply much more work, "only dump any
extension that objects in the schema depend on" (#1) comes second in my
opinion. Using the pattern means something more manual for users, and I
really think it would be a bad idea. People dump databases, schemas, and
tables. Theu usually don't know which extensions those objects depend on.
But, anyway, I would work on any of these solutions, depending on what most
people think is best.

Thanks.

--
Guillaume.

On 20 May 2020, at 11:38, Guillaume Lelarge <guillaume@lelarge.info> wrote:

Actually, "dump all extensions" (#3) would make sense to me, and has my vote.

Wouldn't that open for another set of problems when running with --schema=bar
and getting errors on restoring for relocatable extensions like these:

CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA foo;

Only dumping extensions depended on has the same problem of course.

People dump databases, schemas, and tables. Theu usually don't know which extensions those objects depend on.

That I totally agree with, question is how we best can help users here.

cheers ./daniel

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le mer. 20 mai 2020 à 11:26, Daniel Gustafsson <daniel@yesql.se> a écrit :

The question is what --extensions should do: only dump any
extensions that objects in the schema depend on; require a pattern and only
dump matching extensions; dump all extensions (probably not) or something
else?

Actually, "dump all extensions" (#3) would make sense to me, and has my
vote.

I think that makes no sense at all. By definition, a dump that's been
restricted with --schema, --table, or any similar switch is incomplete
and may not restore on its own. Typical examples include foreign key
references to tables in other schemas, views using functions in other
schemas, etc etc. I see no reason for extension dependencies to be
treated differently from those cases.

In any use of selective dump, it's the user's job to select a set of
objects that she wants dumped (or restored). Trying to second-guess that
is mostly going to make the feature less usable for power-user cases.

As a counterexample, what if you want the dump to be restorable on a
system that doesn't have all of the extensions available on the source?
You carefully pick out the tables that you need, which don't require the
unavailable extensions ... and then pg_dump decides you don't know what
you're doing and includes all the problematic extensions anyway.

I could get behind an "--extensions=PATTERN" switch to allow selective
addition of extensions to a selective dump, but I don't want to see us
overruling the user's choices about what to dump.

regards, tom lane

Le mer. 20 mai 2020 à 16:39, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le mer. 20 mai 2020 à 11:26, Daniel Gustafsson <daniel@yesql.se> a

écrit :

The question is what --extensions should do: only dump any
extensions that objects in the schema depend on; require a pattern and

only

dump matching extensions; dump all extensions (probably not) or

something

else?

Actually, "dump all extensions" (#3) would make sense to me, and has my
vote.

I think that makes no sense at all. By definition, a dump that's been
restricted with --schema, --table, or any similar switch is incomplete
and may not restore on its own. Typical examples include foreign key
references to tables in other schemas, views using functions in other
schemas, etc etc. I see no reason for extension dependencies to be
treated differently from those cases.

Agreed.

In any use of selective dump, it's the user's job to select a set of

objects that she wants dumped (or restored). Trying to second-guess that
is mostly going to make the feature less usable for power-user cases.

Agreed, though right now he has no way to do this for extensions.

As a counterexample, what if you want the dump to be restorable on a

system that doesn't have all of the extensions available on the source?
You carefully pick out the tables that you need, which don't require the
unavailable extensions ... and then pg_dump decides you don't know what
you're doing and includes all the problematic extensions anyway.

That's true.

I could get behind an "--extensions=PATTERN" switch to allow selective

addition of extensions to a selective dump, but I don't want to see us
overruling the user's choices about what to dump.

With all your comments, I can only agree to your views. I'll try to work on
this anytime soon.

--
Guillaume.

Hi,

Le sam. 23 mai 2020 à 14:53, Guillaume Lelarge <guillaume@lelarge.info> a
écrit :

Le mer. 20 mai 2020 à 16:39, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

Guillaume Lelarge <guillaume@lelarge.info> writes:

Le mer. 20 mai 2020 à 11:26, Daniel Gustafsson <daniel@yesql.se> a

écrit :

The question is what --extensions should do: only dump any
extensions that objects in the schema depend on; require a pattern and

only

dump matching extensions; dump all extensions (probably not) or

something

else?

Actually, "dump all extensions" (#3) would make sense to me, and has my
vote.

I think that makes no sense at all. By definition, a dump that's been
restricted with --schema, --table, or any similar switch is incomplete
and may not restore on its own. Typical examples include foreign key
references to tables in other schemas, views using functions in other
schemas, etc etc. I see no reason for extension dependencies to be
treated differently from those cases.

Agreed.

In any use of selective dump, it's the user's job to select a set of

objects that she wants dumped (or restored). Trying to second-guess that
is mostly going to make the feature less usable for power-user cases.

Agreed, though right now he has no way to do this for extensions.

As a counterexample, what if you want the dump to be restorable on a

system that doesn't have all of the extensions available on the source?
You carefully pick out the tables that you need, which don't require the
unavailable extensions ... and then pg_dump decides you don't know what
you're doing and includes all the problematic extensions anyway.

That's true.

I could get behind an "--extensions=PATTERN" switch to allow selective

addition of extensions to a selective dump, but I don't want to see us
overruling the user's choices about what to dump.

With all your comments, I can only agree to your views. I'll try to work
on this anytime soon.

"Anytime soon" was a long long time ago, and I eventually completely forgot
this, sorry. As nobody worked on it yet, I took a shot at it. See attached
patch.

I don't know if I should add this right away in the commit fest app. If
yes, I guess it should go on the next commit fest (2021-03), right?

--
Guillaume.

On Mon, Jan 25, 2021 at 9:34 PM Guillaume Lelarge
<guillaume@lelarge.info> wrote:

"Anytime soon" was a long long time ago, and I eventually completely forgot this, sorry. As nobody worked on it yet, I took a shot at it. See attached patch.

Great!

I didn't reviewed it thoroughly yet, but after a quick look it sounds
sensible. I'd prefer to see some tests added, and it looks like a
test for plpgsql could be added quite easily.

I don't know if I should add this right away in the commit fest app. If yes, I guess it should go on the next commit fest (2021-03), right?

Correct, and please add it on the commit fest!

Le mar. 26 janv. 2021 à 05:10, Julien Rouhaud <rjuju123@gmail.com> a écrit :

On Mon, Jan 25, 2021 at 9:34 PM Guillaume Lelarge
<guillaume@lelarge.info> wrote:

"Anytime soon" was a long long time ago, and I eventually completely

forgot this, sorry. As nobody worked on it yet, I took a shot at it. See
attached patch.

Great!

I didn't reviewed it thoroughly yet, but after a quick look it sounds
sensible. I'd prefer to see some tests added, and it looks like a
test for plpgsql could be added quite easily.

I tried that all afternoon yesterday, but failed to do so. My had still
hurts, but I'll try again though it may take some time.

I don't know if I should add this right away in the commit fest app. If
yes, I guess it should go on the next commit fest (2021-03), right?

Correct, and please add it on the commit fest!

Done, see https://commitfest.postgresql.org/32/2956/.

--
Guillaume.

Le mar. 26 janv. 2021 à 13:41, Guillaume Lelarge <guillaume@lelarge.info> a
écrit :

Le mar. 26 janv. 2021 à 05:10, Julien Rouhaud <rjuju123@gmail.com> a
écrit :

On Mon, Jan 25, 2021 at 9:34 PM Guillaume Lelarge
<guillaume@lelarge.info> wrote:

"Anytime soon" was a long long time ago, and I eventually completely

forgot this, sorry. As nobody worked on it yet, I took a shot at it. See
attached patch.

Great!

I didn't reviewed it thoroughly yet, but after a quick look it sounds
sensible. I'd prefer to see some tests added, and it looks like a
test for plpgsql could be added quite easily.

I tried that all afternoon yesterday, but failed to do so. My had still
hurts, but I'll try again though it may take some time.

s/My had/My head/ ..

I don't know if I should add this right away in the commit fest app. If

yes, I guess it should go on the next commit fest (2021-03), right?

Correct, and please add it on the commit fest!

Done, see https://commitfest.postgresql.org/32/2956/.

--
Guillaume.

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

The patch applies cleanly and looks fine to me. However consider this scenario.

- CREATE SCHEMA foo;
- CREATE EXTENSION file_fdw WITH SCHEMA foo;
- pg_dump --file=/tmp/test.sql --exclude-schema=foo postgres

This will still include the extension 'file_fdw' in the backup script. Shouldn't it be excluded as well?

The new status of this patch is: Waiting on Author

Hi,

Thanks for the review.

Le mer. 3 févr. 2021 à 18:33, Asif Rehman <asifr.rehman@gmail.com> a écrit :

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

The patch applies cleanly and looks fine to me. However consider this
scenario.

- CREATE SCHEMA foo;
- CREATE EXTENSION file_fdw WITH SCHEMA foo;
- pg_dump --file=/tmp/test.sql --exclude-schema=foo postgres

This will still include the extension 'file_fdw' in the backup script.
Shouldn't it be excluded as well?

The new status of this patch is: Waiting on Author

This behaviour is already there without my patch, and I think it's a valid
behaviour. An extension doesn't belong to a schema. Its objects do, but the
extension doesn't.

--
Guillaume.

Le mar. 26 janv. 2021 à 13:42, Guillaume Lelarge <guillaume@lelarge.info> a
écrit :

Le mar. 26 janv. 2021 à 13:41, Guillaume Lelarge <guillaume@lelarge.info>
a écrit :

Le mar. 26 janv. 2021 à 05:10, Julien Rouhaud <rjuju123@gmail.com> a
écrit :

On Mon, Jan 25, 2021 at 9:34 PM Guillaume Lelarge
<guillaume@lelarge.info> wrote:

"Anytime soon" was a long long time ago, and I eventually completely

forgot this, sorry. As nobody worked on it yet, I took a shot at it. See
attached patch.

Great!

I didn't reviewed it thoroughly yet, but after a quick look it sounds
sensible. I'd prefer to see some tests added, and it looks like a
test for plpgsql could be added quite easily.

I tried that all afternoon yesterday, but failed to do so. My had still
hurts, but I'll try again though it may take some time.

s/My had/My head/ ..

I finally managed to get a working TAP test for my patch. I have no idea if
it's good, and if it's enough. Anyway, new version of the patch attached.

--
Guillaume.

On Thu, Feb 18, 2021 at 11:13:06AM +0100, Guillaume Lelarge wrote:

I finally managed to get a working TAP test for my patch. I have no idea if
it's good, and if it's enough. Anyway, new version of the patch attached.

As presented in this patch, specifying both --extension and
--table/--schema means that pg_dump will dump both tables and
extensions matching the pattern passed down. But shouldn't extensions
not be dumped if --table or --schema is used? Combining --schema with
--table implies that the schema part is ignored, for instance.

The comment at the top of selectDumpableExtension() is incorrect,
missing the new case added by --extension.

The use of strict_names looks right to me, but you have missed a
refresh of the documentation of --strict-names that applies to
--extension with this patch.

+       dump_cmd => [
+           'pg_dump', "--file=$tempdir/test_schema_plus_extensions.sql",
+           '--schema=dump_test', '--extension=plpgsql', '--no-sync',
What's the goal of this test.  plpgsql is a system extension so it
will never be dumped.  It seems to me that any tests related to this 
patch should be added to src/test/modules/test_pg_dump/, which
includes a custom extension in the test, that could be dumped.

+        dumped.  Multiple schemas can be selected by writing multiple
+        <option>-e</option> switches.  The <replaceable
s/schemas/extensions/.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

As presented in this patch, specifying both --extension and
--table/--schema means that pg_dump will dump both tables and
extensions matching the pattern passed down. But shouldn't extensions
not be dumped if --table or --schema is used? Combining --schema with
--table implies that the schema part is ignored, for instance.

I haven't read the patch, but the behavior I would expect is:

1. If --extension=pattern is given, then extensions matching the
pattern are included in the dump, regardless of other switches.
(Conversely, use of --extension doesn't affect choices about what
other objects are dumped.)

2. Without --extension, the behavior is backward compatible,
ie, dump extensions in an include_everything dump but not
otherwise.

Maybe we could have a separate discussion as to which switches turn
off include_everything, but that seems independent of this patch.

regards, tom lane

Le sam. 20 févr. 2021 à 13:25, Michael Paquier <michael@paquier.xyz> a
écrit :

On Thu, Feb 18, 2021 at 11:13:06AM +0100, Guillaume Lelarge wrote:

I finally managed to get a working TAP test for my patch. I have no idea

if

it's good, and if it's enough. Anyway, new version of the patch attached.

As presented in this patch, specifying both --extension and
--table/--schema means that pg_dump will dump both tables and
extensions matching the pattern passed down. But shouldn't extensions
not be dumped if --table or --schema is used? Combining --schema with
--table implies that the schema part is ignored, for instance.

Actually, that's the whole point of the patch. Imagine someone who wants to
backup a table with a citext column. With the --table option, this user
will only have the table, without the extension that would allow to restore
the dump. He will have to remember to create the extension before. The new
option allows him to add the CREATE EXTENSION he needs into his dump.

The comment at the top of selectDumpableExtension() is incorrect,

missing the new case added by --extension.

The use of strict_names looks right to me, but you have missed a
refresh of the documentation of --strict-names that applies to
--extension with this patch.

+       dump_cmd => [
+           'pg_dump', "--file=$tempdir/test_schema_plus_extensions.sql",
+           '--schema=dump_test', '--extension=plpgsql', '--no-sync',
What's the goal of this test.  plpgsql is a system extension so it
will never be dumped.  It seems to me that any tests related to this
patch should be added to src/test/modules/test_pg_dump/, which
includes a custom extension in the test, that could be dumped.

+        dumped.  Multiple schemas can be selected by writing multiple
+        <option>-e</option> switches.  The <replaceable
s/schemas/extensions/.

You're right on all these points. I'm on vacation this week, but I'll build
a v3 with these issues fixed when I'll get back from vacation.

Thanks.

--
Guillaume.

Le sam. 20 févr. 2021 à 17:31, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

Michael Paquier <michael@paquier.xyz> writes:

As presented in this patch, specifying both --extension and
--table/--schema means that pg_dump will dump both tables and
extensions matching the pattern passed down. But shouldn't extensions
not be dumped if --table or --schema is used? Combining --schema with
--table implies that the schema part is ignored, for instance.

I haven't read the patch, but the behavior I would expect is:

1. If --extension=pattern is given, then extensions matching the
pattern are included in the dump, regardless of other switches.
(Conversely, use of --extension doesn't affect choices about what
other objects are dumped.)

2. Without --extension, the behavior is backward compatible,
ie, dump extensions in an include_everything dump but not
otherwise.

Yes, that's what it's supposed to do.

Maybe we could have a separate discussion as to which switches turn

off include_everything, but that seems independent of this patch.

+1

--
Guillaume.

On Sat, Feb 20, 2021 at 10:39:24PM +0100, Guillaume Lelarge wrote:

Le sam. 20 févr. 2021 à 17:31, Tom Lane <tgl@sss.pgh.pa.us> a écrit :

I haven't read the patch, but the behavior I would expect is:

1. If --extension=pattern is given, then extensions matching the
pattern are included in the dump, regardless of other switches.
(Conversely, use of --extension doesn't affect choices about what
other objects are dumped.)

2. Without --extension, the behavior is backward compatible,
ie, dump extensions in an include_everything dump but not
otherwise.

Yes, that's what it's supposed to do.

Okay, that sounds fine to me. Thanks for confirming.
--
Michael

On Thu, Feb 18, 2021 at 11:13:06AM +0100, Guillaume Lelarge wrote:

Le mar. 26 janv. 2021 � 13:42, Guillaume Lelarge <guillaume@lelarge.info> a
�crit :

Le mar. 26 janv. 2021 � 13:41, Guillaume Lelarge <guillaume@lelarge.info>
a �crit :

Le mar. 26 janv. 2021 � 05:10, Julien Rouhaud <rjuju123@gmail.com> a
�crit :

On Mon, Jan 25, 2021 at 9:34 PM Guillaume Lelarge
<guillaume@lelarge.info> wrote:

"Anytime soon" was a long long time ago, and I eventually completely

forgot this, sorry. As nobody worked on it yet, I took a shot at it. See
attached patch.

Great!

I didn't reviewed it thoroughly yet, but after a quick look it sounds
sensible. I'd prefer to see some tests added, and it looks like a
test for plpgsql could be added quite easily.

I tried that all afternoon yesterday, but failed to do so. My had still
hurts, but I'll try again though it may take some time.

s/My had/My head/ ..

I finally managed to get a working TAP test for my patch. I have no idea if
it's good, and if it's enough. Anyway, new version of the patch attached.

--
Guillaume.

Thanks for doing this work!

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index bcbb7a25fb..95d45fabfb 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -215,6 +215,38 @@ PostgreSQL documentation
</listitem>
</varlistentry>

+     <varlistentry>
+      <term><option>-e <replaceable class="parameter">pattern</replaceable></option></term>
+      <term><option>--extension=<replaceable class="parameter">pattern</replaceable></option></term>
+      <listitem>
+       <para>
+        Dump only extensions matching <replaceable
+        class="parameter">pattern</replaceable>.  When this option is not
+        specified, all non-system extensions in the target database will be
+        dumped.  Multiple schemas can be selected by writing multiple

^^^^^^^^^^^^^^^^
I think this should read "Multiple extensions".

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate