Thread: pg_dump with tables created in schemas created by extensions
Hi, About a month or two ago I reported a pg_dump bug regarding tables (and other objects) created inside a schema from an extension. Objects created by the extensions are not dumped, as they will be created once again with the CREATE EXTENSION call, but and other objects which might live inside an object created by the extension should be dumped so they get created inside the same schema. The problem showed up when dumping a DB with PgQ installed as an extension. Check here: https://www.postgresql.org/message-id/d86dd685-1870-cfa0-e5e4-def1f918bec9%402ndquadrant.com and here: https://www.postgresql.org/message-id/409fe594-f4cc-89f5-c0d2-0a921987a864%402ndquadrant.com Some discussion came up on the bugs list on how to fix the issue, and the fact the new tests were needed. I'm attaching a patch to provide such test, which if applied now, returns failure on a number of runs, all expected due to the bug we have at hand. 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? Regards, -- Martín Marqués http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Sat, Aug 13, 2016 at 6:58 AM, Martín Marqués <martin@2ndquadrant.com> 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
Hi Michael, 2016-08-23 5:02 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>: > On Sat, Aug 13, 2016 at 6:58 AM, Martín Marqués <martin@2ndquadrant.com> 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hi, 2016-08-23 16:46 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>: > > 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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
On Wed, Aug 24, 2016 at 5:34 AM, Martín Marqués <martin@2ndquadrant.com> wrote: > Hi, > > 2016-08-23 16:46 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>: >> >> 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
On Wed, Aug 24, 2016 at 9:07 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Aug 24, 2016 at 5:34 AM, Martín Marqués <martin@2ndquadrant.com> wrote: >> Hi, >> >> 2016-08-23 16:46 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>: >>> >>> 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
Attachment
Michael, * Michael Paquier (michael.paquier@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
2016-08-24 11:15 GMT-03:00 Stephen Frost <sfrost@snowman.net>: > Michael, > > * Michael Paquier (michael.paquier@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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
2016-08-24 17:01 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>:
> 2016-08-24 11:15 GMT-03:00 Stephen Frost <sfrost@snowman.net>:
>> Michael,
>>
>> * Michael Paquier (michael.paquier@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és http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Aug 24, 2016 at 11:15 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Michael Paquier (michael.paquier@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
2016-08-24 21:34 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>: > > 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Aug 25, 2016 at 10:25 AM, Martín Marqués <martin@2ndquadrant.com> wrote: > 2016-08-24 21:34 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>: >> >> 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
Hi,
2016-08-25 8:10 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
> On Thu, Aug 25, 2016 at 10:25 AM, Martín Marqués <martin@2ndquadrant.com> wrote:
>> 2016-08-24 21:34 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
>>>
>>> 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és http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hi, 2016-08-26 10:53 GMT-03:00 Martín Marqués <martin@2ndquadrant.com>: > > 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Martín Marqués <martin@2ndquadrant.com> 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
On 08/27/2016 12:37 AM, Tom Lane wrote: > Martín Marqués <martin@2ndquadrant.com> 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
Tomas Vondra <tomas.vondra@2ndquadrant.com> 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.
On 08/26/2016 04:15 PM, Tomas Vondra wrote: > On 08/27/2016 12:37 AM, Tom Lane wrote: >> Martín Marqués <martin@2ndquadrant.com> 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.
2016-08-26 19:37 GMT-03:00 Tom Lane <tgl@sss.pgh.pa.us>: > Martín Marqués <martin@2ndquadrant.com> 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sat, Aug 27, 2016 at 8:15 AM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > On 08/27/2016 12:37 AM, Tom Lane wrote: >> Martín Marqués <martin@2ndquadrant.com> 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
2016-08-29 4:51 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>: > >> 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués <martin@2ndquadrant.com> 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
2016-08-30 2:02 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>: > On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués <martin@2ndquadrant.com> 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és http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués <martin@2ndquadrant.com> 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