Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Tom Lane]

Michael Paquier  writes:
> On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués  
> wrote:
>> This is v4 of the patch, which is actually a cleaner version from the
>> v2 one Michael sent.

> Let's do as you suggest then, and just focus on the schema issue. I
> just had an extra look at the patch and it looks fine to me. So the
> patch is now switched as ready for committer.

Pushed with cosmetic adjustments (mainly, I thought the comment needed
to be much more explicit).

I'm not sure whether we want to try to fix this in older branches.
It would be a significantly larger patch, and maybe more of a behavior
change than we want to make in stable branches.  So I'm inclined to call
it done at this point.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-30 2:02 GMT-03:00 Michael Paquier :
> On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués  
> wrote:
>> This is v4 of the patch, which is actually a cleaner version from the
>> v2 one Michael sent.
>>
>> I stripped off the external index created from the tests as that index
>> shouldn't be dumped (table it belongs to isn't dumped, so neither
>> should the index). I also took off a test which was duplicated.
>>
>> I think this patch is a very good first approach. Future improvements
>> can be made for indexes, but we need to get the extension dependencies
>> right first. That could be done later, on a different patch.
>>
>> Thoughts?
>
> Let's do as you suggest then, and just focus on the schema issue. I
> just had an extra look at the patch and it looks fine to me. So the
> patch is now switched as ready for committer.

That's great. Thanks for all Michael


-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués  wrote:
> This is v4 of the patch, which is actually a cleaner version from the
> v2 one Michael sent.
>
> I stripped off the external index created from the tests as that index
> shouldn't be dumped (table it belongs to isn't dumped, so neither
> should the index). I also took off a test which was duplicated.
>
> I think this patch is a very good first approach. Future improvements
> can be made for indexes, but we need to get the extension dependencies
> right first. That could be done later, on a different patch.
>
> Thoughts?

Let's do as you suggest then, and just focus on the schema issue. I
just had an extra look at the patch and it looks fine to me. So the
patch is now switched as ready for committer.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

Hi,

This is v4 of the patch, which is actually a cleaner version from the
v2 one Michael sent.

I stripped off the external index created from the tests as that index
shouldn't be dumped (table it belongs to isn't dumped, so neither
should the index). I also took off a test which was duplicated.

I think this patch is a very good first approach. Future improvements
can be made for indexes, but we need to get the extension dependencies
right first. That could be done later, on a different patch.

Thoughts?

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


pgdump-extension-v4.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-29 4:51 GMT-03:00 Michael Paquier :
>
>> I see the current behavior is documented, and I do understand why global
>> objects can't be part of the extension, but for indexes it seems to violate
>> POLA a bit.
>>
>> Is there a reason why we don't want the extension/index dependencies?
>
> I think that we could do a better effort for indexes at least, in the
> same way as we do for sequences as both are referenced in pg_class. I
> don't know the effort to get that done for < 9.6, but if we can do it
> at least for 9.6 and 10, which is where pg_dump is a bit smarter in
> the way it deals with dependencies, we should do it.

ATM I don't have a strong opinion one way or the other regarding the
dependency of indexes and extensions. I believe we have to put more
thought into it, and at the end we might just leave it as it is.

What I do believe is that this requires a separate thread, and if
agreed, a separate patch from this issue.

I'm going to prepare another patch where I'm going to strip the tests
for external indexes which are failing now. They actually fail
correctly as the table they depend on will not be dumped, so it's the
developer/DB designer who has to take care of these things.

If in the near or not so near future we provide a patch to deal with
these missing dependencies, we can easily patch pg_dump so it deals
with this correctly.

Regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Sat, Aug 27, 2016 at 8:15 AM, Tomas Vondra
 wrote:
> On 08/27/2016 12:37 AM, Tom Lane wrote:
>> =?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?=  writes:
>>> Looking at this issue today, I found that we are not setting a
>>> dependency for an index created inside an extension.
>>
>> Surely the index has a dependency on a table, which depends on the
>> extension?
>>
>> If you mean that you want an extension to create an index on a table
>> that doesn't belong to it, but it's assuming pre-exists, I think
>> that's just stupid and we need not support it.
>
> I don't see why that would be stupid (and I'm not sure it's up to us to just
> decide it's stupid).

Like Tomas, I am not really getting this opposition..

> I see the current behavior is documented, and I do understand why global
> objects can't be part of the extension, but for indexes it seems to violate
> POLA a bit.
>
> Is there a reason why we don't want the extension/index dependencies?

I think that we could do a better effort for indexes at least, in the
same way as we do for sequences as both are referenced in pg_class. I
don't know the effort to get that done for < 9.6, but if we can do it
at least for 9.6 and 10, which is where pg_dump is a bit smarter in
the way it deals with dependencies, we should do it.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-26 19:37 GMT-03:00 Tom Lane :
> =?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?=  writes:
>> Looking at this issue today, I found that we are not setting a
>> dependency for an index created inside an extension.
>
> Surely the index has a dependency on a table, which depends on the
> extension?
>
> If you mean that you want an extension to create an index on a table that
> doesn't belong to it, but it's assuming pre-exists, I think that's just
> stupid and we need not support it.

Well, there's still the second pattern I mentioned before (which
actually came up while trying to fix this patch).

Extension creates a table and an index over one of the columns:

CREATE TABLE regress_pg_dump_schema.test_table (
col1 int,
col2 int
);

CREATE INDEX test_extension_index ON regress_pg_dump_schema.test_table (col2);


Later, some application (or a user, doesn't matter really) creates a
second index over col1:

CREATE INDEX test_index ON regress_pg_dump_schema.test_table (col1);

What we are doing (or at least it's what I understand from the code)
is checking if the table depends on an extension, and so we don't dump
it.

We should be able to use the same procedure (and reuse the code we
already have) to decide if an index should be dumped or not. But we
are missing the dependency, and so it's not possible to know that
regress_pg_dump_schema.test_extension_index depends on the extension
and regress_pg_dump_schema.test_index doesn't.

Or is this something we shouldn't support (in that case we should document it).

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Joshua D. Drake]

On 08/26/2016 04:15 PM, Tomas Vondra wrote:

On 08/27/2016 12:37 AM, Tom Lane wrote:

=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?=  writes:

Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.


Surely the index has a dependency on a table, which depends on the
extension?

If you mean that you want an extension to create an index on a table
that doesn't belong to it, but it's assuming pre-exists, I think
that's just stupid and we need not support it.



I don't see why that would be stupid (and I'm not sure it's up to us to
just decide it's stupid).


+1, there are a lot of workflow patterns that make sense and don't make 
sense depending on your perspective, consider safe mode with MySQL. I 
personally think it is stupid too but it also obviously has a huge 
useful user base.




Imagine you use extensions to break the application into modules. You
have a "basic" extension, with the table, and a "search" extension
implementing fulltext search on top of that table. Isn't it natural to
keep the gin indexes in the search extension?

So the basic--1.0.sql will do something like

  CREATE TABLE x ( ...)

and search--1.0.sql would do

  CREATE INDEX y ON x USING gin (z);
  CREATE FUNCTION search_objects(..) ...

because the index and function doing the search are somewhat tightly
coupled. I admit this is just an example and I don't know how many
people use extensions this way, but I don't see why this would be
inherently stupid pattern?


It isn't and in fact shows that as we extend Pg, the more we allow 
people flexibility in developing WITHIN the database, the more we will 
be able to influence them to do so. That is a good thing.


Or maybe we should just tell everyone, use an ORM it will solve all your 
problems. (/sarcasm)



Sincerely,

JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Tom Lane]

Tomas Vondra  writes:
> On 08/27/2016 12:37 AM, Tom Lane wrote:
>> If you mean that you want an extension to create an index on a table
>> that doesn't belong to it, but it's assuming pre-exists, I think
>> that's just stupid and we need not support it.

> I don't see why that would be stupid (and I'm not sure it's up to us to 
> just decide it's stupid).

Well, think about it.

1. Let's say user A owns the pre-existing table and user B owns the
extension.  Who owns the index?

2. Generally speaking, if an object is part of an extension then you
can't drop the object without dropping the whole extension.  This means
that either user A can't drop his own table, or he can but that causes
dropping the whole extension (which he does not own), not to mention
whatever else depends on it (which he owns even less).

3. Can user A do something that would mutate the index?  (For instance,
ALTER COLUMN TYPE on one of the columns it's on.)  Now is it still a
member of the extension?  If user A can't, can user B?  What if either of
them mutated the column to a datatype that belongs to some extension that
has a dependency on the original one?  Now you've got a circular
dependency loop, what fun --- especially at dump/reload time, where
pg_dump has no hope of breaking the circularity.

4. If we're going to support indexes as independent members of extensions,
why not foreign-key constraints, or check constraints?  Maybe check
constraints on domain types that don't belong to the extension?  Heck,
why not allow an extension to do ALTER TABLE ADD COLUMN?  (I think
BTW that several of these cases would allow creation of circular
extension dependencies even without any ALTER COLUMN TYPE shenanigans.)

None of this makes any sense, because these things are not stand-alone
objects in any sense of the word.  They are attributes of tables (or
domain types, for that case) and there are good reasons why we don't
consider such attributes to have ownership independent of the object
they are part of.  I do not think it is sensible for an extension to "own"
objects that don't also have, or could potentially have [1], independent
ownership in the sense of an owning user.

> Imagine you use extensions to break the application into modules.

I do not think that extensions as we currently understand them are a
suitable basis for slicing an application in the way you suggest.
I'm fine with an extension owning a whole table, but the rest of this
is just crazy.  And I sure as hell do not want to put it in as part
of a bug fix for existing releases.

regards, tom lane

[1] weasel wording for cases like casts, which we don't consider
to have owners, though I'm not sure why not.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Tomas Vondra]

On 08/27/2016 12:37 AM, Tom Lane wrote:

=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?=  writes:

Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension.


Surely the index has a dependency on a table, which depends on the
extension?

If you mean that you want an extension to create an index on a table
that doesn't belong to it, but it's assuming pre-exists, I think
that's just stupid and we need not support it.



I don't see why that would be stupid (and I'm not sure it's up to us to 
just decide it's stupid).


Imagine you use extensions to break the application into modules. You 
have a "basic" extension, with the table, and a "search" extension 
implementing fulltext search on top of that table. Isn't it natural to 
keep the gin indexes in the search extension?


So the basic--1.0.sql will do something like

  CREATE TABLE x ( ...)

and search--1.0.sql would do

  CREATE INDEX y ON x USING gin (z);
  CREATE FUNCTION search_objects(..) ...

because the index and function doing the search are somewhat tightly 
coupled. I admit this is just an example and I don't know how many 
people use extensions this way, but I don't see why this would be 
inherently stupid pattern?


I see the current behavior is documented, and I do understand why global 
objects can't be part of the extension, but for indexes it seems to 
violate POLA a bit.


Is there a reason why we don't want the extension/index dependencies?

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Tom Lane]

=?UTF-8?B?TWFydMOtbiBNYXJxdcOpcw==?=  writes:
> Looking at this issue today, I found that we are not setting a
> dependency for an index created inside an extension.

Surely the index has a dependency on a table, which depends on the
extension?

If you mean that you want an extension to create an index on a table that
doesn't belong to it, but it's assuming pre-exists, I think that's just
stupid and we need not support it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

Hi,

2016-08-26 10:53 GMT-03:00 Martín Marqués :
>
> There's still one issue, which I'll add a test for as well, which is
> that if the index was created by the extension, it will be dumped
> anyway. I'll have a look at that as well.

Looking at this issue today, I found that we are not setting a
dependency for an index created inside an extension. I don't know if
it's deliberate or an overlook.

The thing is that we can't check pg_depend for dependency of an index
and the extension that creates it.

I was talking with other developers, and we kind of agree this is a
bug, for 2 reasons we thought of:

*) If the extension only creates an index over an existing table, a
drop extension will not drop that index

*) We need to have the dependency for this patch as well, else we'll
end up with an inconsistent dump, or at least one that could restore
with a != 0 return error code.

I'll open a separate bug report for this.

Regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

Hi,

2016-08-25 8:10 GMT-03:00 Michael Paquier :
> On Thu, Aug 25, 2016 at 10:25 AM, Martín Marqués  
> wrote:
>> 2016-08-24 21:34 GMT-03:00 Michael Paquier :
>>>
>>> Yes, you are right. If I look at the diffs this morning I am seeing
>>> the ACLs being dumped for this aggregate. So we could just fix the
>>> test and be done with it. I did not imagine the index issue though,
>>> and this is broken for some time, so that's not exclusive to 9.6 :)
>>
>> Do you see any easier way than what I mentioned earlier (adding a
>> selectDumpableIndex() function) to fix the index dumping issue?
>
> Yes, we are going to need something across those lines. And my guess
> is that this is going to be rather close to getOwnedSeqs() in terms of
> logic.

I was able to get this fixed without any further new functions (just
using the dump/dump_contains and applying the same fix on
selectDumpableTable).

Main problem relied here in getIndexes()

@@ -6158,7 +6167,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[],
int numTables)
continue;

/* Ignore indexes of tables whose definitions are not
to be dumped */
-   if (!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
+   if (!(tbinfo->dobj.dump_contains & DUMP_COMPONENT_DEFINITION))
continue;

if (g_verbose)

But we have to set dump_contains with correct values.

There's still one issue, which I'll add a test for as well, which is
that if the index was created by the extension, it will be dumped
anyway. I'll ave a look at that as well.

One other thing I found was that one of the CREATE INDEX tests had
incorrectly set like and unlike for pre_data and post_data. (indexes
are dumped in section post_data)

That's been fixes as well.

I've cleaned up the patch a bit, so this is v3 with all checks passing.

I'll add that new test regarding dumping an index created by the
extension (which will fail) and look for ways to fix it.

Regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


pgdump-extension-v3.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Thu, Aug 25, 2016 at 10:25 AM, Martín Marqués  wrote:
> 2016-08-24 21:34 GMT-03:00 Michael Paquier :
>>
>> Yes, you are right. If I look at the diffs this morning I am seeing
>> the ACLs being dumped for this aggregate. So we could just fix the
>> test and be done with it. I did not imagine the index issue though,
>> and this is broken for some time, so that's not exclusive to 9.6 :)
>
> Do you see any easier way than what I mentioned earlier (adding a
> selectDumpableIndex() function) to fix the index dumping issue?

Yes, we are going to need something across those lines. And my guess
is that this is going to be rather close to getOwnedSeqs() in terms of
logic.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-24 21:34 GMT-03:00 Michael Paquier :
>
> Yes, you are right. If I look at the diffs this morning I am seeing
> the ACLs being dumped for this aggregate. So we could just fix the
> test and be done with it. I did not imagine the index issue though,
> and this is broken for some time, so that's not exclusive to 9.6 :)

Hi Michael,

Do you see any easier way than what I mentioned earlier (adding a
selectDumpableIndex() function) to fix the index dumping issue?

Regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Wed, Aug 24, 2016 at 11:15 PM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> The patch attached includes all those tests and they are failing. We
>> are going to need a patch able to pass all that, and even for master
>> this is going to need more thoughts, and let's focus on HEAD/9.6
>> first.
>
> Are you sure you have the tests correct..?   At least for testagg(), it
> looks like you're testing for:
>
> GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
>
> but what's in the dump is (equivilantly):
>
> GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;
>
> I've not looked into all the failures, but at least this one seems like
> an issue in the test, not an issue in pg_dump.

Yes, you are right. If I look at the diffs this morning I am seeing
the ACLs being dumped for this aggregate. So we could just fix the
test and be done with it. I did not imagine the index issue though,
and this is broken for some time, so that's not exclusive to 9.6 :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-24 17:01 GMT-03:00 Martín Marqués :
> 2016-08-24 11:15 GMT-03:00 Stephen Frost :
>> Michael,
>>
>> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>>> The patch attached includes all those tests and they are failing. We
>>> are going to need a patch able to pass all that, and even for master
>>> this is going to need more thoughts, and let's focus on HEAD/9.6
>>> first.
>>
>> Are you sure you have the tests correct..?   At least for testagg(), it
>> looks like you're testing for:
>>
>> GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
>>
>> but what's in the dump is (equivilantly):
>>
>> GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;
>
> Yes, that was the problem there.
>
>> I've not looked into all the failures, but at least this one seems like
>> an issue in the test, not an issue in pg_dump.
>
> I see the other 12 failures regarding the CREATE INDEX that Michael
> reported but can't quite find where it's originated. (or actually
> where the problem is)

OK, I see where the problem is.

Indexes don't have a selectDumpableIndex() function to see if we dump
it or not. We just don't gather indexes from tables for which we are
dumping their definition:

/* Ignore indexes of tables whose definitions are not
to be dumped */
if (!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;

This means we have to perform the same change we did on
selectDumpableNamespace for selectDumpableTable, and also assign the
correct value to dump_contains, which is not set there.

The problem will come when we have to decide on which indexes were
created by the extension (primary key indexes, other indexes created
by the extension) and which were created afterwards over a table which
depends on the extension (the test_table from the extension).

Right now, I'm in an intermediate state, where I got getIndexes() to
query for the indexes of these tables, but dumpIndexes is not dumping
the indexes that were queried.

I wonder if we should have a selectDumpableIndexes to set the
appropriate dobj.dump for the Indexes.

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

2016-08-24 11:15 GMT-03:00 Stephen Frost :
> Michael,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> The patch attached includes all those tests and they are failing. We
>> are going to need a patch able to pass all that, and even for master
>> this is going to need more thoughts, and let's focus on HEAD/9.6
>> first.
>
> Are you sure you have the tests correct..?   At least for testagg(), it
> looks like you're testing for:
>
> GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;
>
> but what's in the dump is (equivilantly):
>
> GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;

Yes, that was the problem there.

> I've not looked into all the failures, but at least this one seems like
> an issue in the test, not an issue in pg_dump.

I see the other 12 failures regarding the CREATE INDEX that Michael
reported but can't quite find where it's originated. (or actually
where the problem is)

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> The patch attached includes all those tests and they are failing. We
> are going to need a patch able to pass all that, and even for master
> this is going to need more thoughts, and let's focus on HEAD/9.6
> first.

Are you sure you have the tests correct..?   At least for testagg(), it
looks like you're testing for:

GRANT ALL ON FUNCTION test_agg(int2) TO regress_dump_test_role;

but what's in the dump is (equivilantly):

GRANT ALL ON FUNCTION test_agg(smallint) TO regress_dump_test_role;

I've not looked into all the failures, but at least this one seems like
an issue in the test, not an issue in pg_dump.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Wed, Aug 24, 2016 at 9:07 AM, Michael Paquier
 wrote:
> On Wed, Aug 24, 2016 at 5:34 AM, Martín Marqués  
> wrote:
>> Hi,
>>
>> 2016-08-23 16:46 GMT-03:00 Martín Marqués :
>>>
>>> I will add tests for sequence and functions as you mention and test again.
>>>
>>> Then I'll check if other tests should be added as well.
>>
>> I found quite some other objects we should be checking as well, but
>> this will add some duplication to the tests, as I'd just copy (with
>> minor changes) what's in src/bin/pg_dump/t/002_pg_dump.pl
>>
>> I can't think of a way to avoid this duplication, not that it really
>> hurts. We would have to make sure that any new objects added to one
>> test, if needed, are added to the other (that's a bit cumbersome).
>>
>> Other things to check:
>>
>> CREATE AGGREGATE
>> CREATE FUNCTION

Agreed on those two.

>> CREATE DOMAIN
>> CREATE TYPE

Those two overlap, so adding just a type would be fine.

>> CREATE MATERIALIZED VIEW

This overlaps with normal relations.

>> CREATE POLICY

Policies are not schema-qualified.

>> Maybe even CREATE INDEX over a table created in the schema.

Yes, we can do something fancy things here... But see below as pg_dump
is broken even in this case...

>> Also, ACLs have to be tested against objects in the schema.
>>
>> I hope I didn't miss anything there.

So I have reviewed the patch for master, did some wordsmithing and
added more tests. Particularly, I have added tests for a couple of
objects created in the extension, as well as objects that are not part
of the extension but make use of the schema of the extension. Even
with that, I am still seeing two problems:
- ACLs assigned to an aggregate in an extension are not getting dumped
in a binary upgrade, and I think that they should be. The aggregate
definition gets correctly handled though.
- if an index is defined on a table part of an extension it will not
be dumped. We can fix the problem of this thread and the one I just
found separately though.
The patch attached includes all those tests and they are failing. We
are going to need a patch able to pass all that, and even for master
this is going to need more thoughts, and let's focus on HEAD/9.6
first.
-- 
Michael


pgdump-extension-v2.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Wed, Aug 24, 2016 at 5:34 AM, Martín Marqués  wrote:
> Hi,
>
> 2016-08-23 16:46 GMT-03:00 Martín Marqués :
>>
>> I will add tests for sequence and functions as you mention and test again.
>>
>> Then I'll check if other tests should be added as well.
>
> I found quite some other objects we should be checking as well, but
> this will add some duplication to the tests, as I'd just copy (with
> minor changes) what's in src/bin/pg_dump/t/002_pg_dump.pl
>
> I can't think of a way to avoid this duplication, not that it really
> hurts. We would have to make sure that any new objects added to one
> test, if needed, are added to the other (that's a bit cumbersome).
>
> Other things to check:
>
> CREATE AGGREGATE
> CREATE DOMAIN
> CREATE FUNCTION
> CREATE TYPE
> CREATE MATERIALIZED VIEW
> CREATE POLICY
>
> Maybe even CREATE INDEX over a table created in the schema.
>
> Also, ACLs have to be tested against objects in the schema.
>
> I hope I didn't miss anything there.

I'll take a look at that soon and review your patch as well as your
tests. For now I have added an entry in the CF app to not lose track
of it:
https://commitfest.postgresql.org/10/736/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Jim Nasby]

On 8/23/16 3:34 PM, Martín Marqués wrote:

I found quite some other objects we should be checking as well, but
this will add some duplication to the tests, as I'd just copy (with
minor changes) what's in src/bin/pg_dump/t/002_pg_dump.pl

I can't think of a way to avoid this duplication, not that it really
hurts. We would have to make sure that any new objects added to one
test, if needed, are added to the other (that's a bit cumbersome).


At one point I had some code that understood what object names (ie: 
AGGREGATE, TABLE, etc) went with what catalog tables, what ones lived 
inside a schema (as opposed to globally), and which ones were 
shared/global (cross-database). I think I needed this for some automatic 
handling of comments, but it's been a while. Maybe something like that 
would help reduce the duplication...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

Hi,

2016-08-23 16:46 GMT-03:00 Martín Marqués :
>
> I will add tests for sequence and functions as you mention and test again.
>
> Then I'll check if other tests should be added as well.

I found quite some other objects we should be checking as well, but
this will add some duplication to the tests, as I'd just copy (with
minor changes) what's in src/bin/pg_dump/t/002_pg_dump.pl

I can't think of a way to avoid this duplication, not that it really
hurts. We would have to make sure that any new objects added to one
test, if needed, are added to the other (that's a bit cumbersome).

Other things to check:

CREATE AGGREGATE
CREATE DOMAIN
CREATE FUNCTION
CREATE TYPE
CREATE MATERIALIZED VIEW
CREATE POLICY

Maybe even CREATE INDEX over a table created in the schema.

Also, ACLs have to be tested against objects in the schema.

I hope I didn't miss anything there.

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Martín Marqués]

Hi Michael,

2016-08-23 5:02 GMT-03:00 Michael Paquier :
> On Sat, Aug 13, 2016 at 6:58 AM, Martín Marqués  
> wrote:
>> I believe the fix will be simple after the back and forth mails with
>> Michael, Stephen and Tom. I will work on that later, but preferred to
>> have the tests the show the problem which will also make testing the fix
>> easier.
>>
>> Thoughts?
>
> It seems to me that we are going to need a bit more coverage for more
> object types depending on the code paths that are being changed. For
> example, sequences or functions should be checked for as well, and not
> only tables. By the way, do you need help to build a patch or should I
> jump in?

I wanted to test what I had in mind with one object, and then see if
any replication is needed for other objects.

I was struggling the last days as what I was reading in my patched
pg_dump.c had to work as expected, and dump the tables not created by
the test_pg_dump extension but inside the schema
regress_pg_dump_schema.

Today I decided to go over the test I wrote, and found a bug there,
reason why I couldn't get a successful make check.

Here go 2 patches. One is a fix for the test I sent earlier. The other
is the proposed idea Tom had using the dump_contains that Stephan
committed on 9.6.

So far I've checked that it fixes the dumpable for Tables, but I think
it should work for all other objects as well, as all this patch does
is leave execution of checkExtensionMembership at the end of
selectDumpableNamespace, leaving the dump_contains untouched.

Checks pass ok.

I will add tests for sequence and functions as you mention and test again.

Then I'll check if other tests should be added as well.

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 6cbba9e87d1733ccb03d3d705e135e607be63cba Mon Sep 17 00:00:00 2001
From: Martin 
Date: Tue, 23 Aug 2016 16:17:38 -0300
Subject: [PATCH 1/2] New tests for pg_dump which make sure that tables from a
 schema depending on an extension will get dumped when they don't depend on
 the extension.

The only objects we shouldn't dump are the ones created by the extension.

We will provide a patch that fixes this bug.
---
 src/test/modules/test_pg_dump/t/001_base.pl| 25 ++
 .../modules/test_pg_dump/test_pg_dump--1.0.sql |  9 
 2 files changed, 34 insertions(+)

diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index fb4f573..7bb92e7 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -283,6 +283,31 @@ my %tests = (
 			schema_only=> 1,
 			section_pre_data   => 1,
 			section_post_data  => 1, }, },
+	'CREATE TABLE regress_test_schema_table' => {
+		create_order => 3,
+		create_sql   => 'CREATE TABLE regress_pg_dump_schema.test_schema_table (
+		   col1 serial primary key,
+		   CHECK (col1 <= 1000)
+	   );',
+		regexp => qr/^
+			\QCREATE TABLE test_schema_table (\E
+			\n\s+\Qcol1 integer NOT NULL,\E
+			\n\s+\QCONSTRAINT test_schema_table_col1_check CHECK \E
+			\Q((col1 <= 1000))\E
+			\n\);/xm,
+		like => {
+			binary_upgrade => 1,
+			clean  => 1,
+			clean_if_exists=> 1,
+			createdb   => 1,
+			defaults   => 1,
+			no_privs   => 1,
+			no_owner   => 1,
+			schema_only=> 1,
+			section_pre_data   => 1, },
+		unlike => {
+			pg_dumpall_globals => 1,
+			section_post_data  => 1, }, },
 	'CREATE ACCESS METHOD regress_test_am' => {
 		regexp => qr/^
 			\QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E
diff --git a/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql b/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
index c2fe90d..3f88e6c 100644
--- a/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
+++ b/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
@@ -10,6 +10,15 @@ CREATE TABLE regress_pg_dump_table (
 
 CREATE SEQUENCE regress_pg_dump_seq;
 
+-- We want to test that schemas and objects created in the schema by the
+-- extension are not dumped, yet other objects created afterwards will be
+-- dumped.
+CREATE SCHEMA regress_pg_dump_schema
+   CREATE TABLE regress_pg_dump_schema_table (
+  col1 serial,
+  col2 int
+	);
+
 GRANT USAGE ON regress_pg_dump_seq TO regress_dump_test_role;
 
 GRANT SELECT ON regress_pg_dump_table TO regress_dump_test_role;
-- 
2.5.5

From 0dfe2224fae86ab2b6f8da4fc25b96fb13ca0f6c Mon Sep 17 00:00:00 2001
From: Martin 
Date: Tue, 23 Aug 2016 16:23:44 -0300
Subject: [PATCH 2/2] This patch fixes a bug reported against pg_dump, and
 makes pg_dump dump the objects contained in schemas depending on an
 extension.

Schema will not be dumped, as it will be created by the extension, but
any other tables created outside the extension's SQL 

Re: [HACKERS] pg_dump with tables created in schemas created by extensions

[Michael Paquier]

On Sat, Aug 13, 2016 at 6:58 AM, Martín Marqués  wrote:
> I believe the fix will be simple after the back and forth mails with
> Michael, Stephen and Tom. I will work on that later, but preferred to
> have the tests the show the problem which will also make testing the fix
> easier.
>
> Thoughts?

It seems to me that we are going to need a bit more coverage for more
object types depending on the code paths that are being changed. For
example, sequences or functions should be checked for as well, and not
only tables. By the way, do you need help to build a patch or should I
jump in?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers