Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Thu, May 11, 2017 at 9:33 AM, Tom Lane wrote: > Uh ... what in that is creating the already-extant parent? /me looks embarrassed. Never mind. I didn't read what you wrote carefully enough. >> I think one answer to the original complaint might be to add a new >> flag to pg_dump, something like --recursive-selection, maybe -r for >> short, which makes --table, --exclude-table, and --exclude-table-data >> cascade to inheritance descendents. > > Yeah, you could do it like that. Another way to do it would be to > create variants of all the selection switches, along the lines of > "--table-all=foo" meaning "foo plus its children". Then you could > have some switches recursing and others not within the same command. > But maybe that's more flexibility than needed ... and I'm having a > hard time coming up with nice switch names, anyway. I don't think that's as good. It's a lot more typing than what I proposed and I don't think anyone is really going to want the flexibility. > Anyway, I'm still of the opinion that it's fine to leave this as a > future feature. If we've gotten away without it this long for > inherited tables, it's unlikely to be critical for partitioned tables. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Robert Haas writes: > On Thu, May 11, 2017 at 9:02 AM, Tom Lane wrote: >> You could argue that it would be better for pg_dump to emit something >> like >> >> CREATE TABLE c (...); >> ALTER TABLE c INHERIT p; >> >> so that if p isn't around, at least c gets created. And I think it >> *would* be better, probably. But if that isn't a new feature then >> I don't know what is. And partitioning really ought to track the >> behavior of inheritance here. > Hmm, I think that'd actually be worse, because it would break the use > case where you want to restore the old contents of one particular > inheritance child. So you drop that child (but not the parent or any > other children) and then do a selective restore of that one child. > Right now that works fine, but it'll fail with an error if we try to > create the already-extant parent. Uh ... what in that is creating the already-extant parent? What I'm envisioning is simply that the TOC entry for the child table contains those two statements rather than "CREATE TABLE c (...) INHERITS (p)". The equivalent thing for the partitioned case is to use a separate ATTACH PARTITION command rather than naming the parent immediately in the child's CREATE TABLE. This is independent of the question of how --table and friends ought to behave. > I think one answer to the original complaint might be to add a new > flag to pg_dump, something like --recursive-selection, maybe -r for > short, which makes --table, --exclude-table, and --exclude-table-data > cascade to inheritance descendents. Yeah, you could do it like that. Another way to do it would be to create variants of all the selection switches, along the lines of "--table-all=foo" meaning "foo plus its children". Then you could have some switches recursing and others not within the same command. But maybe that's more flexibility than needed ... and I'm having a hard time coming up with nice switch names, anyway. Anyway, I'm still of the opinion that it's fine to leave this as a future feature. If we've gotten away without it this long for inherited tables, it's unlikely to be critical for partitioned tables. 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Thu, May 11, 2017 at 9:02 AM, Tom Lane wrote: > Jeevan Ladhe writes: >> On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat < >>> We add PARTITION OF clause for a table which is partition, so if the >>> parent is not present while restoring, the restore is going to fail. > >> +1 >> But, similarly for inheritance if we dump a child table, it's dump is >> broken as >> of today. When we dump a child table we append "inherits(parenttab)" clause >> at >> the end of the DDL. Later when we try to restore this table on a database >> not >> having the parenttab, the restore fails. >> So, I consider this as a bug. > > It sounds exactly what I'd expect. In particular, given that pg_dump has > worked that way for inherited tables from the beginning, it's hard to see > any must-fix bugs here. I agree. > You could argue that it would be better for pg_dump to emit something > like > > CREATE TABLE c (...); > ALTER TABLE c INHERIT p; > > so that if p isn't around, at least c gets created. And I think it > *would* be better, probably. But if that isn't a new feature then > I don't know what is. And partitioning really ought to track the > behavior of inheritance here. Hmm, I think that'd actually be worse, because it would break the use case where you want to restore the old contents of one particular inheritance child. So you drop that child (but not the parent or any other children) and then do a selective restore of that one child. Right now that works fine, but it'll fail with an error if we try to create the already-extant parent. I think one answer to the original complaint might be to add a new flag to pg_dump, something like --recursive-selection, maybe -r for short, which makes --table, --exclude-table, and --exclude-table-data cascade to inheritance descendents. Then if you want to dump your partition table's definition without picking up the partitions, you can say pg_dump -t splat, and if you want the children as well you can say pg_dump -r -t splat. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Jeevan Ladhe writes: > On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat < >> We add PARTITION OF clause for a table which is partition, so if the >> parent is not present while restoring, the restore is going to fail. > +1 > But, similarly for inheritance if we dump a child table, it's dump is > broken as > of today. When we dump a child table we append "inherits(parenttab)" clause > at > the end of the DDL. Later when we try to restore this table on a database > not > having the parenttab, the restore fails. > So, I consider this as a bug. It sounds exactly what I'd expect. In particular, given that pg_dump has worked that way for inherited tables from the beginning, it's hard to see any must-fix bugs here. You could argue that it would be better for pg_dump to emit something like CREATE TABLE c (...); ALTER TABLE c INHERIT p; so that if p isn't around, at least c gets created. And I think it *would* be better, probably. But if that isn't a new feature then I don't know what is. And partitioning really ought to track the behavior of inheritance here. 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > > We add PARTITION OF clause for a table which is partition, so if the > parent is not present while restoring, the restore is going to fail. +1 But, similarly for inheritance if we dump a child table, it's dump is broken as of today. When we dump a child table we append "inherits(parenttab)" clause at the end of the DDL. Later when we try to restore this table on a database not having the parenttab, the restore fails. So, I consider this as a bug. Consider following example: postgres=# create table tab1(a int, b int); CREATE TABLE postgres=# create table tab2(c int, d char) inherits(tab1); CREATE TABLE postgres=# \! pg_dump postgres -t tab2 > a.sql postgres=# create database bkdb; CREATE DATABASE postgres=# \! psql bkdb < a.sql SET SET SET SET SET SET SET SET SET SET SET ERROR: relation "tab1" does not exist ERROR: relation "tab2" does not exist ERROR: relation "tab2" does not exist invalid command \. postgres=# Regards, Jeevan Ladhe
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Thu, May 11, 2017 at 3:05 AM, Tom Lane wrote: > > We should make sure that pg_dump behaves sanely when dumping just > some elements of a partition tree, of course. And for that matter, > pg_restore ought to be able to successfully restore just some elements > out of a an archive containing more. > We add PARTITION OF clause for a table which is partition, so if the parent is not present while restoring, the restore is going to fail. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Thu, May 11, 2017 at 2:06 AM, Robert Haas wrote: > On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe > wrote: >>> Current pg_dump --exclude-table option excludes partitioned relation >>> and dumps all its child relations and vice versa for --table option, which >>> I think is incorrect. >>> >>> In this case we might need to explore all partitions and exclude or >>> include >>> from dump according to given pg_dump option, attaching WIP patch proposing >>> same fix. Thoughts/Comments? >> >> +1. >> >> Also, I can see similar issue exists with inheritance. > > That's a pretty insightful comment which makes me think that this > isn't properly categorized as a bug. Table partitioning is based on > inheritance and is intended to behave like inheritance except when > we've decided to make it behave otherwise. We made no such decision > in this case, so it behaves like inheritance in this case. So, this > is only a bug if the inheritance behavior is also a bug. We have chosen inheritance as mechanism to implement partitioning, but users will have different expectations from them. Partitioned table is a "table" "containing" partitions. So, if we want to dump a table which is partitioned, we better dump its partitions (which happen to be tables by themselves) as well. I don't think we can say that inheritance parent contains its children, esp. thinking in the context of multiple inheritance. IOW users would expect us to dump partitioned table with its partitions and not just the shell. In case of DROP TABLE we do drop all the partitions without specifying CASCADE. To drop an inheritance parent we need CASCADE to drop its children. So, we have already started diverging from inheritance. > > And I think there's a pretty good argument that it isn't. Are we > saying we think that it was always intended that dumping an > inheritance parent would always dump its inheritance children as well, > and the code accidentally failed to achieve that? Are we saying we'd > back-patch a behavior change in this area to correct the wrong > behavior we've had since time immemorial? I can't speak for anyone > else, but I think the first is probably false and I would vote against > the second. I think the inheritance behaviour we have got, whether intentional or unintentional, is acceptable since we do not consider an inheritance parent alongwith its children a unit, esp. when multiple inheritance exists. > > That's not to say that this isn't a possibly-useful behavior change. > I can easily imagine that users would find it convenient to be able to > dump a whole partitioning hierarchy by just selecting the parent table > rather than having to list all of the partitions. But that's also > true of inheritance. I agree that its useful behaviour for inheritance but I am not sure that it's a "feature" for partitioned tables. > Is partitioning different enough from > inheritance that the two should have different behaviors, perhaps > because the partitioning parent can't contain data but the inheritance > parent could? Yes, most users would see them as different things, esp. those who come from other DBMS backgrounds. > Or should we change the behavior for inheritance as > well, on the theory that the proposed new behavior is just plain > better than the current behavior and everyone will want it? Not necessarily, as is the inheritance behaviour looks sane to me. > Or should > we do nothing at all, so as to avoid breaking things for the user who > says they want to dump JUST the parent and really means it? Even if > the parent couldn't contain any rows, it's still got a schema that can > be dumped. The option of changing partitioning in one patch and then > having a separate patch later that makes a similar change for > inheritance seems like the worst option, since then users might get > any of three behaviors depending on which release they have. If we > want to change this, let's change it all at once. But first we need > to get clarity on exactly what we're changing and for what reason. > > There is a question of timing. If we accept that this is not a bug > but a proposed behavior change, then it's not clear that it really > qualifies as an open item for v10. I understand that there's an urge > to keep tinkering and making things better, but it's far from clear to > me that anything bad will happen if we just postpone changing anything > until v11, especially if we decide to change both partitioning and > inheritance. I am somewhat inclined to classify this proposed open > item as a non-bug (i.e. a feature) but I'm also curious to hear what > others think. To me this looks like a bug, something to be fixed in v10 only for partitioned tables. But we don't need to entangle that with inheritance. Partitioned tables get dumped (or not dumped) as a whole and inheritance parents as single units. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers maili
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Robert Haas writes: > On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe > wrote: >> Also, I can see similar issue exists with inheritance. > That's a pretty insightful comment which makes me think that this > isn't properly categorized as a bug. Table partitioning is based on > inheritance and is intended to behave like inheritance except when > we've decided to make it behave otherwise. We made no such decision > in this case, so it behaves like inheritance in this case. So, this > is only a bug if the inheritance behavior is also a bug. > And I think there's a pretty good argument that it isn't. I agree. There is room for a feature that would make --table or --exclude-table on an inheritance parent apply to its children, but that's a missing feature not a bug. Also, if we did make that happen automatically, for either inheritance or partitioning, there would inevitably be people who need to turn it off. ISTM that the point of partitioning is mainly to be able to split up maintenance work for a table that's too large to handle as an indivisible unit, and it's hard to see why that wouldn't apply to dump/restore as much as it does, say, vacuum. So I think this can be classified as a feature to add later. We should make sure that pg_dump behaves sanely when dumping just some elements of a partition tree, of course. And for that matter, pg_restore ought to be able to successfully restore just some elements out of a an archive containing more. 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe wrote: >> Current pg_dump --exclude-table option excludes partitioned relation >> and dumps all its child relations and vice versa for --table option, which >> I think is incorrect. >> >> In this case we might need to explore all partitions and exclude or >> include >> from dump according to given pg_dump option, attaching WIP patch proposing >> same fix. Thoughts/Comments? > > +1. > > Also, I can see similar issue exists with inheritance. That's a pretty insightful comment which makes me think that this isn't properly categorized as a bug. Table partitioning is based on inheritance and is intended to behave like inheritance except when we've decided to make it behave otherwise. We made no such decision in this case, so it behaves like inheritance in this case. So, this is only a bug if the inheritance behavior is also a bug. And I think there's a pretty good argument that it isn't. Are we saying we think that it was always intended that dumping an inheritance parent would always dump its inheritance children as well, and the code accidentally failed to achieve that? Are we saying we'd back-patch a behavior change in this area to correct the wrong behavior we've had since time immemorial? I can't speak for anyone else, but I think the first is probably false and I would vote against the second. That's not to say that this isn't a possibly-useful behavior change. I can easily imagine that users would find it convenient to be able to dump a whole partitioning hierarchy by just selecting the parent table rather than having to list all of the partitions. But that's also true of inheritance. Is partitioning different enough from inheritance that the two should have different behaviors, perhaps because the partitioning parent can't contain data but the inheritance parent could? Or should we change the behavior for inheritance as well, on the theory that the proposed new behavior is just plain better than the current behavior and everyone will want it? Or should we do nothing at all, so as to avoid breaking things for the user who says they want to dump JUST the parent and really means it? Even if the parent couldn't contain any rows, it's still got a schema that can be dumped. The option of changing partitioning in one patch and then having a separate patch later that makes a similar change for inheritance seems like the worst option, since then users might get any of three behaviors depending on which release they have. If we want to change this, let's change it all at once. But first we need to get clarity on exactly what we're changing and for what reason. There is a question of timing. If we accept that this is not a bug but a proposed behavior change, then it's not clear that it really qualifies as an open item for v10. I understand that there's an urge to keep tinkering and making things better, but it's far from clear to me that anything bad will happen if we just postpone changing anything until v11, especially if we decide to change both partitioning and inheritance. I am somewhat inclined to classify this proposed open item as a non-bug (i.e. a feature) but I'm also curious to hear what others think. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Tue, May 9, 2017 at 3:51 PM, Ashutosh Bapat wrote: > On Tue, May 9, 2017 at 2:59 PM, Amit Langote > wrote: >> Hi Amul, >> >> On 2017/05/09 16:13, amul sul wrote: >>> Hi, >>> >>> Current pg_dump --exclude-table option excludes partitioned relation >>> and dumps all its child relations and vice versa for --table option, which >>> I think is incorrect. >> >> I agree that `pg_dump -t partitioned_table` should dump all of its >> partitions too and that `pg_dump -T partitioned_table` should exclude >> partitions. Your patch achieves that. Some comments: >> >> I think we should update doc/src/sgml/ref/pg_dump.sgml to mention this >> behavior. >> >> +static void >> +find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids) >> >> Is expand_partitioned_table() a slightly better name? >> >> >> + appendPQExpBuffer(query, "SELECT relkind " >> + "FROM pg_catalog.pg_class " >> + "WHERE oid = %u", partid); >> + >> + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); >> + >> + if (PQntuples(res) == 0) >> + exit_horribly(NULL, "no matching partition tables >> were "); >> + >> + relkind = PQgetvalue(res, 0, 0); >> + >> + if (relkind[0] == RELKIND_PARTITIONED_TABLE) >> + find_partition_by_relid(fout, partid, oids); >> >> Instead of recursing like this requiring to send one query for every >> partition, why not issue just one query such that all the partitions in >> the inheritance tree are returned. For example, as follows: >> >> +appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS (" >> + " SELECT inhrelid" >> + " FROM pg_inherits" >> + " WHERE inhparent = %u" >> + " UNION ALL" >> + " SELECT inhrelid" >> + " FROM pg_inherits, partitions" >> + " WHERE inhparent = partitions.partoid )" >> + " SELECT partoid FROM partitions", >> + parentId); >> >> I included your patch with the above modifications in the attached 0001 >> patch, which also contains the documentation updates. Please take a look. > > I think this patch too has the same problem I described in my reply to > Amul; it fires a query to server for every partitioned table it > encounters, that's not very efficient. > Agree with Ashutosh, If such information is already available then we need to leverage of it. >> >> When testing the patch, I found that if --table specifies the parent and >> --exclude specifies one of its partitions, the latter won't be forcefully >> be included due to the partitioned table expansion, which seems like an >> acceptable behavior. > > unless the partition is default partition or a hash partition. > >> On the other hand, if --table specifies a partition >> and --exclude specifies the partition's parent, the latter makes partition >> to be excluded as well as part of the expansion, which seems fine too. >> > > I am not sure if it's worth considering partitions and partitioned > table as different entities as far as dump is concerned. We should > probably dump the whole partitioned table or none of it. There's merit > in dumping just a single partition to transfer that data to some other > server, but I am not sure how much the feature would be used. > but won't be useless. > In order to avoid any such potential anomalies, we should copy dump > flag for a partition from that of the parent as I have described in my > reply to Amul. > >> One more thing I am wondering about is whether `pg_dump -t partition` >> should be dumped as a partition definition (that is, as CREATE TABLE >> PARTITION OF) which is what happens currently or as a regular table (just >> CREATE TABLE)? I'm thinking the latter would be better, but would like to >> confirm before writing any code for that. > > If we go about dumping a partition without its parent table, we should > dump CREATE TABLE with proper list of columns and constraints without > PARTITION OF clause. But I am not sure whether we should go that > route. IMO, I think it's worth to dump CREATE TABLE without PARTITION OF clause. Regards, Amul -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Tue, May 9, 2017 at 2:59 PM, Amit Langote wrote: > Hi Amul, > > On 2017/05/09 16:13, amul sul wrote: >> Hi, >> >> Current pg_dump --exclude-table option excludes partitioned relation >> and dumps all its child relations and vice versa for --table option, which >> I think is incorrect. > > I agree that `pg_dump -t partitioned_table` should dump all of its > partitions too and that `pg_dump -T partitioned_table` should exclude > partitions. Your patch achieves that. Some comments: > > I think we should update doc/src/sgml/ref/pg_dump.sgml to mention this > behavior. > > +static void > +find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids) > > Is expand_partitioned_table() a slightly better name? > > > + appendPQExpBuffer(query, "SELECT relkind " > + "FROM pg_catalog.pg_class " > + "WHERE oid = %u", partid); > + > + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); > + > + if (PQntuples(res) == 0) > + exit_horribly(NULL, "no matching partition tables > were "); > + > + relkind = PQgetvalue(res, 0, 0); > + > + if (relkind[0] == RELKIND_PARTITIONED_TABLE) > + find_partition_by_relid(fout, partid, oids); > > Instead of recursing like this requiring to send one query for every > partition, why not issue just one query such that all the partitions in > the inheritance tree are returned. For example, as follows: > > +appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS (" > + " SELECT inhrelid" > + " FROM pg_inherits" > + " WHERE inhparent = %u" > + " UNION ALL" > + " SELECT inhrelid" > + " FROM pg_inherits, partitions" > + " WHERE inhparent = partitions.partoid )" > + " SELECT partoid FROM partitions", > + parentId); > > I included your patch with the above modifications in the attached 0001 > patch, which also contains the documentation updates. Please take a look. I think this patch too has the same problem I described in my reply to Amul; it fires a query to server for every partitioned table it encounters, that's not very efficient. > > When testing the patch, I found that if --table specifies the parent and > --exclude specifies one of its partitions, the latter won't be forcefully > be included due to the partitioned table expansion, which seems like an > acceptable behavior. unless the partition is default partition or a hash partition. > On the other hand, if --table specifies a partition > and --exclude specifies the partition's parent, the latter makes partition > to be excluded as well as part of the expansion, which seems fine too. > I am not sure if it's worth considering partitions and partitioned table as different entities as far as dump is concerned. We should probably dump the whole partitioned table or none of it. There's merit in dumping just a single partition to transfer that data to some other server, but I am not sure how much the feature would be used. In order to avoid any such potential anomalies, we should copy dump flag for a partition from that of the parent as I have described in my reply to Amul. > One more thing I am wondering about is whether `pg_dump -t partition` > should be dumped as a partition definition (that is, as CREATE TABLE > PARTITION OF) which is what happens currently or as a regular table (just > CREATE TABLE)? I'm thinking the latter would be better, but would like to > confirm before writing any code for that. If we go about dumping a partition without its parent table, we should dump CREATE TABLE with proper list of columns and constraints without PARTITION OF clause. But I am not sure whether we should go that route. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi Amit, Ashutosh, On Tue, May 9, 2017 at 3:29 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Tue, May 9, 2017 at 3:13 PM, Amit Langote > wrote: > > On 2017/05/09 17:21, Jeevan Ladhe wrote: > >> On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > >>> Current pg_dump --exclude-table option excludes partitioned relation > >>> and dumps all its child relations and vice versa for --table option, > which > >>> I think is incorrect. > >>> > >>> In this case we might need to explore all partitions and exclude or > include > >>> from dump according to given pg_dump option, attaching WIP patch > proposing > >>> same fix. Thoughts/Comments? > >> > >> Also, I can see similar issue exists with inheritance. > >> In attached patch, I have extended Amul's original patch to address the > >> inheritance dumping issue. > > > > Perhaps, it will be better not to touch the regular inheritance tables in > > this patch. > > Yeah, I think it's fine if parent gets dumped without one or more of > its children, that's user's choice when it used a certain pattern. > Problematic case might be when we dump a child without its parent and > have INHERITS clause there. pg_restore would throw an error. But in > case that problem exists it's very old and should be fixed separately. I agree that this should be taken as a separate fix, rather than taking it with partition. Regards, Jeevan Ladhe
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Tue, May 9, 2017 at 3:13 PM, Amit Langote wrote: > On 2017/05/09 17:21, Jeevan Ladhe wrote: >> On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: >>> Current pg_dump --exclude-table option excludes partitioned relation >>> and dumps all its child relations and vice versa for --table option, which >>> I think is incorrect. >>> >>> In this case we might need to explore all partitions and exclude or include >>> from dump according to given pg_dump option, attaching WIP patch proposing >>> same fix. Thoughts/Comments? >> >> Also, I can see similar issue exists with inheritance. >> In attached patch, I have extended Amul's original patch to address the >> inheritance dumping issue. > > Perhaps, it will be better not to touch the regular inheritance tables in > this patch. Yeah, I think it's fine if parent gets dumped without one or more of its children, that's user's choice when it used a certain pattern. Problematic case might be when we dump a child without its parent and have INHERITS clause there. pg_restore would throw an error. But in case that problem exists it's very old and should be fixed separately. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On 2017/05/09 17:21, Jeevan Ladhe wrote: > On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: >> Current pg_dump --exclude-table option excludes partitioned relation >> and dumps all its child relations and vice versa for --table option, which >> I think is incorrect. >> >> In this case we might need to explore all partitions and exclude or include >> from dump according to given pg_dump option, attaching WIP patch proposing >> same fix. Thoughts/Comments? > > Also, I can see similar issue exists with inheritance. > In attached patch, I have extended Amul's original patch to address the > inheritance dumping issue. Perhaps, it will be better not to touch the regular inheritance tables in this patch. Thanks, Amit -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > Hi, > > Current pg_dump --exclude-table option excludes partitioned relation > and dumps all its child relations and vice versa for --table option, which > I think is incorrect. > > In this case we might need to explore all partitions and exclude or include > from dump according to given pg_dump option, attaching WIP patch proposing > same fix. Thoughts/Comments? > +1 for fixing this. Since we didn't catch this issue earlier it looks like we don't have testcases testing this scenario. May be we should add those in the patch. The way this patch has been written, there is a possibility that a partition would be included multiple times in the list of tables, if names of the partition and the parent table both match the pattern. That's not a correctness issue, but it would slow down simple_oid_list_member() a bit because of longer OID list. I am not sure what would be the use of dumping a partitioned table without its partitions or vice-versa. I think, instead, look partitioned table and its partitions as a single unit as far as dump is concerned. So, either we dump partitioned table along with all its partitions or we don't dump any of those. In that case, we should modify the query in expand_table_name_patterns(), to not include partitions in the result. Rest of the code in the patch would take care of including/excluding the partitions when the parent table is included/excluded. This patch looks up pg_inherits for every partitioned tables that it encounters, which is costly. Instead, a fix with better performance is to set a table dumpable based on the corresponding setting of its parent. The parent is available in TableInfo structure, but the comment there says that it's set only for dumpable objects. The comment is correct since flagInhTables() skips the tables with dump = false. May be we should modify this function not to skip the partitions, find their parent tables and set the dump flat based on the parents. This means that the immediate parent's dump flag should be set correctly before any of its children are encountered by the flagInhTables() for the case of multi-level partitioning to work. I don't think we can guarantee that. May be we can modify flagInhTables() to set the parent tables of parent if that's not done already and then set the dump flag from top parent to bottom. If we do that, we will have to add code in tflagInhTables() to skip the entries whose parents have been set already since those might have been set because of an earlier grand child. Even better if we could dump the partitions along with partitioned table instead of creating separate TableInfo entries for those. But I think that's a lot of change. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi Amul, On 2017/05/09 16:13, amul sul wrote: > Hi, > > Current pg_dump --exclude-table option excludes partitioned relation > and dumps all its child relations and vice versa for --table option, which > I think is incorrect. I agree that `pg_dump -t partitioned_table` should dump all of its partitions too and that `pg_dump -T partitioned_table` should exclude partitions. Your patch achieves that. Some comments: I think we should update doc/src/sgml/ref/pg_dump.sgml to mention this behavior. +static void +find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids) Is expand_partitioned_table() a slightly better name? + appendPQExpBuffer(query, "SELECT relkind " + "FROM pg_catalog.pg_class " + "WHERE oid = %u", partid); + + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + + if (PQntuples(res) == 0) + exit_horribly(NULL, "no matching partition tables were "); + + relkind = PQgetvalue(res, 0, 0); + + if (relkind[0] == RELKIND_PARTITIONED_TABLE) + find_partition_by_relid(fout, partid, oids); Instead of recursing like this requiring to send one query for every partition, why not issue just one query such that all the partitions in the inheritance tree are returned. For example, as follows: +appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS (" + " SELECT inhrelid" + " FROM pg_inherits" + " WHERE inhparent = %u" + " UNION ALL" + " SELECT inhrelid" + " FROM pg_inherits, partitions" + " WHERE inhparent = partitions.partoid )" + " SELECT partoid FROM partitions", + parentId); I included your patch with the above modifications in the attached 0001 patch, which also contains the documentation updates. Please take a look. When testing the patch, I found that if --table specifies the parent and --exclude specifies one of its partitions, the latter won't be forcefully be included due to the partitioned table expansion, which seems like an acceptable behavior. On the other hand, if --table specifies a partition and --exclude specifies the partition's parent, the latter makes partition to be excluded as well as part of the expansion, which seems fine too. One more thing I am wondering about is whether `pg_dump -t partition` should be dumped as a partition definition (that is, as CREATE TABLE PARTITION OF) which is what happens currently or as a regular table (just CREATE TABLE)? I'm thinking the latter would be better, but would like to confirm before writing any code for that. I will add this as an open item. Thanks for working on it. Thanks, Amit >From 3b3103d6a23aab78592d7547e11f7e6414cfe0ec Mon Sep 17 00:00:00 2001 From: amit Date: Tue, 9 May 2017 16:39:14 +0900 Subject: [PATCH] Expand partitioned table specified using pg_dump's -t or -T options Report and patch by Amul Sul. --- doc/src/sgml/ref/pg_dump.sgml | 12 + src/bin/pg_dump/pg_dump.c | 57 +-- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 6cf7e570ef..c90a3298ca 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -520,10 +520,11 @@ PostgreSQL documentation table. For this purpose, table includes views, materialized views, sequences, and foreign tables. Multiple tables -can be selected by writing multiple -t switches. Also, the -table parameter is -interpreted as a pattern according to the same rules used by -psql's \d commands (see -t switches. If the +specified table is a partitioned table, its partitions are dumped +too. Also, the table +parameter is interpreted as a pattern according to the same rules used +by psql's \d commands (see ), so multiple tables can also be selected by writing wildcard characters in the pattern. When using wildcards, be careful to quote the pattern @@ -572,7 +573,8 @@ PostgreSQL documentation class="parameter">table pattern. The pattern is interpreted according to the same rules as for -t. -T can be given more than once to exclude tables -matching any of several patterns. +matching any of several patterns. If the specified table is a +partitioned table, its partitions are not dumped either. diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index af84c25093..00146786e3 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
In my last email by mistake I attached Amul's patch itself. Please find attached patch extending the fix to inheritance relations. Regards, Jeevan Ladhe On Tue, May 9, 2017 at 1:51 PM, Jeevan Ladhe wrote: > Hi, > > On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > >> Hi, >> >> Current pg_dump --exclude-table option excludes partitioned relation >> and dumps all its child relations and vice versa for --table option, which >> I think is incorrect. >> >> In this case we might need to explore all partitions and exclude or >> include >> from dump according to given pg_dump option, attaching WIP patch proposing >> same fix. Thoughts/Comments? >> >> Regards, >> Amul >> > > +1. > > Also, I can see similar issue exists with inheritance. > In attached patch, I have extended Amul's original patch to address the > inheritance dumping issue. > > Regards, > Jeevan Ladhe > > pg_dump_fix_for_partition_and_inheritance_wip.patch Description: Binary data -- 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi, On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > Hi, > > Current pg_dump --exclude-table option excludes partitioned relation > and dumps all its child relations and vice versa for --table option, which > I think is incorrect. > > In this case we might need to explore all partitions and exclude or include > from dump according to given pg_dump option, attaching WIP patch proposing > same fix. Thoughts/Comments? > > Regards, > Amul > +1. Also, I can see similar issue exists with inheritance. In attached patch, I have extended Amul's original patch to address the inheritance dumping issue. Regards, Jeevan Ladhe pg_dump_fix_for_partition_and_inheritance.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi, Current pg_dump --exclude-table option excludes partitioned relation and dumps all its child relations and vice versa for --table option, which I think is incorrect. In this case we might need to explore all partitions and exclude or include from dump according to given pg_dump option, attaching WIP patch proposing same fix. Thoughts/Comments? Regards, Amul pg_dump_fix_for_partitioned_rel_wip.patch Description: Binary data -- 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] Bug in pg_dump
On Wed, Mar 4, 2015 at 2:03 PM, Michael Paquier wrote: > On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut wrote: >> - set up basic scaffolding for TAP tests in src/bin/pg_dump > > Agreed. > >> - write a Perl function that can create an extension on the fly, given >> name, C code, SQL code > > I am perplex about that. Where would the SQL code or C code be stored? > In the pl script itself? I cannot really see the advantage to generate > automatically the skeletton of an extension based on some C or SQL > code in comparison to store the extension statically as-is. Adding > those extensions in src/test/modules is out of scope to not bloat it, > so we could for example add such test extensions in t/extensions/ or > similar, and have prove_check scan this folder, then install those > extensions in the temporary installation. > >> - add to the proposed t/001_dump_test.pl code to write the extension >> - add that test to the pg_dump test suite >> Eventually, the dump-and-restore routine could also be refactored, but >> as long as we only have one test case, that can wait. > > Agreed on all those points. Please note that I have created a new thread especially for this purpose here: http://www.postgresql.org/message-id/CAB7nPqRx=zmbfjyjrwhguhhqk__8m+wd+p95cenjtmhxxr7...@mail.gmail.com Perhaps we should move there this discussion as it is rather independent of the problem that has been reported. Regards, -- 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] Bug in pg_dump
On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut wrote: > - set up basic scaffolding for TAP tests in src/bin/pg_dump Agreed. > - write a Perl function that can create an extension on the fly, given > name, C code, SQL code I am perplex about that. Where would the SQL code or C code be stored? In the pl script itself? I cannot really see the advantage to generate automatically the skeletton of an extension based on some C or SQL code in comparison to store the extension statically as-is. Adding those extensions in src/test/modules is out of scope to not bloat it, so we could for example add such test extensions in t/extensions/ or similar, and have prove_check scan this folder, then install those extensions in the temporary installation. > - add to the proposed t/001_dump_test.pl code to write the extension > - add that test to the pg_dump test suite > Eventually, the dump-and-restore routine could also be refactored, but > as long as we only have one test case, that can wait. Agreed on all those points. -- 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] Bug in pg_dump
On 3/1/15 2:17 PM, Stephen Frost wrote: > Peter, if you have a minute, could you take a look at this thread and > discussion of having TAP tests under src/test/modules which need to > install an extension? I think it's something we certainly want to > support but I'm not sure it's a good idea to just run install in every > directory that has a prove_check. I don't think the way the tests are set up is scalable. Over time, we will surely want to test more and different extension dumping scenarios. We don't want to have to create a new fully built-out extension for each of those test cases, and we're going to run out of useful names for the extensions, too. Moreover, I would like to have tests of pg_dump in src/bin/pg_dump/, not in remote areas of the code. Here's what I would do: - set up basic scaffolding for TAP tests in src/bin/pg_dump - write a Perl function that can create an extension on the fly, given name, C code, SQL code - add to the proposed t/001_dump_test.pl code to write the extension - add that test to the pg_dump test suite Eventually, the dump-and-restore routine could also be refactored, but as long as we only have one test case, that can wait. -- 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] Bug in pg_dump
On Tue, Mar 3, 2015 at 4:57 AM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost wrote: >> > Please see the latest version of this, attached. I've removed the left >> > join, re-used the 'query' buffer (instead of destroying and recreating >> > it), and added a bit of documentation, along with a note in the commit >> > message for the release notes. >> >> Thanks, those modifications look good to me. > > Ok, I've pushed the pg_dump fix for all the branches it applies to. Thanks. >> > Would be great if you could review it and let me know. As mentioned in >> > my other email, I'm happy to include the TAP test in master once we've >> > worked out the correct way to handle installing the extension for >> > testing in the Makefile system. >> >> Sure. As that's unrelated to this thread, perhaps we could discuss >> this point on another thread with refreshed patches? I'd like to hear >> some input from Peter on the matter as well. > > That's fine with me- feel free to start a new thread with new patches. Will do so. -- 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] Bug in pg_dump
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost wrote: > > Please see the latest version of this, attached. I've removed the left > > join, re-used the 'query' buffer (instead of destroying and recreating > > it), and added a bit of documentation, along with a note in the commit > > message for the release notes. > > Thanks, those modifications look good to me. Ok, I've pushed the pg_dump fix for all the branches it applies to. Thanks for the help! > > Would be great if you could review it and let me know. As mentioned in > > my other email, I'm happy to include the TAP test in master once we've > > worked out the correct way to handle installing the extension for > > testing in the Makefile system. > > Sure. As that's unrelated to this thread, perhaps we could discuss > this point on another thread with refreshed patches? I'd like to hear > some input from Peter on the matter as well. That's fine with me- feel free to start a new thread with new patches. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug in pg_dump
On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost wrote: > Please see the latest version of this, attached. I've removed the left > join, re-used the 'query' buffer (instead of destroying and recreating > it), and added a bit of documentation, along with a note in the commit > message for the release notes. Thanks, those modifications look good to me. > Would be great if you could review it and let me know. As mentioned in > my other email, I'm happy to include the TAP test in master once we've > worked out the correct way to handle installing the extension for > testing in the Makefile system. Sure. As that's unrelated to this thread, perhaps we could discuss this point on another thread with refreshed patches? I'd like to hear some input from Peter on the matter as well. -- 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] Bug in pg_dump
Michael, * Stephen Frost (sfr...@snowman.net) wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: > > The thing is that we must absolutely wait for *all* the TableInfoData > > of all the extensions to be created because we need to mark the > > dependencies between them, and even my last patch, or even with what > > you are proposing we may miss tracking of FK dependencies between > > tables in different extensions. This has little chance to happen in > > practice, but I think that we should definitely get things right. > > Hence something like this query able to query all the FK dependencies > > with tables in extensions is more useful, and it has no IN clause: > > Ah, yes, extensions can depend on each other and so this could > definitely happen. The current issue is around postgis, which by itself > provides three different extensions. > > > + appendPQExpBuffer(query, > > + "SELECT conrelid, confrelid " > > + "FROM pg_constraint " > > + "LEFT JOIN pg_depend ON (objid = confrelid) " > > + "WHERE contype = 'f' " > > + "AND refclassid = 'pg_extension'::regclass " > > + "AND classid = 'pg_class'::regclass;"); > > I'm trying to figure out why this is a left join..? Please see the latest version of this, attached. I've removed the left join, re-used the 'query' buffer (instead of destroying and recreating it), and added a bit of documentation, along with a note in the commit message for the release notes. Would be great if you could review it and let me know. As mentioned in my other email, I'm happy to include the TAP test in master once we've worked out the correct way to handle installing the extension for testing in the Makefile system. Thanks! Stephen From ffc459240a97bc4e7a4e4bc81be7a019d4f6782e Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Sun, 1 Mar 2015 15:51:04 -0500 Subject: [PATCH] Fix pg_dump handling of extension config tables Since 9.1, we've provided extensions with a way to denote "configuration" tables- tables created by an extension which the user may modify. By marking these as "configuration" tables, the extension is asking for the data in these tables to be pg_dump'd (tables which are not marked in this way are assumed to be entirely handled during CREATE EXTENSION and are not included at all in a pg_dump). Unfortunately, pg_dump neglected to consider foreign key relationships between extension configuration tables and therefore could end up trying to reload the data in an order which would cause FK violations. This patch teaches pg_dump about these dependencies, so that the data dumped out is done so in the best order possible. Note that there's no way to handle circular dependencies, but those have yet to be seen in the wild. The release notes for this should include a caution to users that existing pg_dump-based backups may be invalid due to this issue. The data is all there, but restoring from it will require extracting the data for the configuration tables and then loading them in the correct order by hand. Discussed initially back in bug #6738, more recently brought up by Gilles Darold, who provided an initial patch which was further reworked by Michael Paquier. Further modifications and documentation updates by me. Back-patch to 9.1 where we added the concept of extension configuration tables. --- doc/src/sgml/extend.sgml | 8 +++ src/bin/pg_dump/pg_dump.c | 54 +-- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index be10252..0a79f56 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -721,6 +721,14 @@ SELECT pg_catalog.pg_extension_config_dump('my_config', 'WHERE NOT standard_entr a table as no longer a configuration table is to dissociate it from the extension with ALTER EXTENSION ... DROP TABLE. + + + Note that foreign key relationships between these tables will dictate the + order in which the tables are dumped out by pg_dump. Specifically, it will + attempt to dump the referenced-by table before the referencing table. As + the foreign key relationships are set up at CREATE EXTENSION time (prior to + data being loaded into the tables) circular dependencies are not supported. + diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 2b53c72..4e404b4 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15236,7 +15236,8 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo) } /* - * getExtensionMembership --- obtain extension membership data + * getExtensionMembership --- obtain extension membership data and check FK + * dependencies among relations interacting with the ones in extensions. */ void getExtensionMembership(Archive *fout,
Re: [HACKERS] Bug in pg_dump
Michael, * Stephen Frost (sfr...@snowman.net) wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: > > >> Subject: [PATCH 2/3] Make prove_check install contents of current > > >> directory as well > > > > > > This is really an independent thing, no? I don't see any particular > > > problem with it, for my part. > > > > Yes, that's an independent thing, but my guess is that it would be > > good to have a real test case this time to be sure that this does not > > break again, at least on master where test/modules is an ideal place. > > I've been looking at this, but noticed the following in > src/test/Makefile: > > # We want to recurse to all subdirs for all standard targets, except that > # installcheck and install should not recurse into the subdirectory "modules". Hrmpf, that hadn't fixed it as I thought, I had another issue which was making it appear to work. The other modules work because they use pg_regress and pass it an '--extra-install' option, so perhaps adding this makes sense after all, though I'm a bit nervous that we're doing double-duty with this approach as some things clearly do get installed by the first 'install'. Peter, if you have a minute, could you take a look at this thread and discussion of having TAP tests under src/test/modules which need to install an extension? I think it's something we certainly want to support but I'm not sure it's a good idea to just run install in every directory that has a prove_check. I'm going to move forward with the actual bug fix. We can certainly add the test in later, once we've got this all sorted. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug in pg_dump
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > >> Subject: [PATCH 2/3] Make prove_check install contents of current > >> directory as well > > > > This is really an independent thing, no? I don't see any particular > > problem with it, for my part. > > Yes, that's an independent thing, but my guess is that it would be > good to have a real test case this time to be sure that this does not > break again, at least on master where test/modules is an ideal place. I've been looking at this, but noticed the following in src/test/Makefile: # We want to recurse to all subdirs for all standard targets, except that # installcheck and install should not recurse into the subdirectory "modules". Also, adding a few more items to the Makefile makes the regression tests pass: + MODULE_big = dump_test + REGRESS = dump_test So I'm thinking this isn't really necessary? I've not really looked into it further but my hunch is the difference is a pgxs build vs. a non-pgxs build (which I think might be what the regression suite runs..). One or both of the above might be necessary to make non-pgxs builds work. I've made a few other modifications to the test (basically, I wrapped all the commands run in command_ok() since it wasn't actually failing when I was expecting it to and plan to include it in the commit to master. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug in pg_dump
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > The thing is that we must absolutely wait for *all* the TableInfoData > of all the extensions to be created because we need to mark the > dependencies between them, and even my last patch, or even with what > you are proposing we may miss tracking of FK dependencies between > tables in different extensions. This has little chance to happen in > practice, but I think that we should definitely get things right. > Hence something like this query able to query all the FK dependencies > with tables in extensions is more useful, and it has no IN clause: Ah, yes, extensions can depend on each other and so this could definitely happen. The current issue is around postgis, which by itself provides three different extensions. > + appendPQExpBuffer(query, > + "SELECT conrelid, confrelid " > + "FROM pg_constraint " > + "LEFT JOIN pg_depend ON (objid = confrelid) " > + "WHERE contype = 'f' " > + "AND refclassid = 'pg_extension'::regclass " > + "AND classid = 'pg_class'::regclass;"); I'm trying to figure out why this is a left join..? > >> Subject: [PATCH 2/3] Make prove_check install contents of current > >> directory as well > > > > This is really an independent thing, no? I don't see any particular > > problem with it, for my part. > > Yes, that's an independent thing, but my guess is that it would be > good to have a real test case this time to be sure that this does not > break again, at least on master where test/modules is an ideal place. No objection to it, just pointing out that it's independent. > Attached are updated patches, the fix of pg_dump can be easily > cherry-picked down to 9.2. For 9.1 things are changed a bit because of > the way SQL queries are run, still patches are attached for all the > versions needed. I tested as well the fix down to this version 9.1 > using the test case dump_test. Will review. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug in pg_dump
On Sun, Mar 1, 2015 at 1:09 AM, Stephen Frost wrote: > Michael, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> + /* >> + * Query all the foreign key dependencies for all the >> extension >> + * tables found previously. Only tables whose data >> need to be >> + * have to be tracked. >> + */ >> + appendPQExpBuffer(query2, >> + "SELECT conrelid, confrelid " >> + "FROM pg_catalog.pg_constraint " >> + "WHERE contype = 'f' AND conrelid IN >> ("); >> + >> + for (j = 0; j < nconfigitems; j++) >> + { > > [...] > > Instead of building up a (potentially) big list like this, couldn't we > simply join against pg_extension and check if conrelid = ANY (extconfig) > instead, based on the extension we're currently processing? > > eg: > > appendPQExpBuffer(query2, > "SELECT conrelid, confrelid " > "FROM pg_catalog.pg_constraint, > pg_extension " > "WHERE contype = 'f' AND extname = '%s' AND > conrelid = ANY (extconfig)", > fmtId(curext->dobj.name)); > > This seemed to work just fine for me, at least, and reduces the size of > the patch a bit further since we don't need the loop that builds up the > ids. Actually, I did a mistake in my last patch, see this comment: + * Now that all the TableInfoData objects have been created for + * all the extensions, check their FK dependencies and register + * them to ensure correct data ordering. The thing is that we must absolutely wait for *all* the TableInfoData of all the extensions to be created because we need to mark the dependencies between them, and even my last patch, or even with what you are proposing we may miss tracking of FK dependencies between tables in different extensions. This has little chance to happen in practice, but I think that we should definitely get things right. Hence something like this query able to query all the FK dependencies with tables in extensions is more useful, and it has no IN clause: + appendPQExpBuffer(query, + "SELECT conrelid, confrelid " + "FROM pg_constraint " + "LEFT JOIN pg_depend ON (objid = confrelid) " + "WHERE contype = 'f' " + "AND refclassid = 'pg_extension'::regclass " + "AND classid = 'pg_class'::regclass;"); >> Subject: [PATCH 2/3] Make prove_check install contents of current directory >> as well > > This is really an independent thing, no? I don't see any particular > problem with it, for my part. Yes, that's an independent thing, but my guess is that it would be good to have a real test case this time to be sure that this does not break again, at least on master where test/modules is an ideal place. Attached are updated patches, the fix of pg_dump can be easily cherry-picked down to 9.2. For 9.1 things are changed a bit because of the way SQL queries are run, still patches are attached for all the versions needed. I tested as well the fix down to this version 9.1 using the test case dump_test. Thanks, -- Michael From 9ef14f2f67cbec9df2a1d30978efdeda059fa12f Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 27 Feb 2015 12:23:42 + Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with constraints Additional checks on FK constraints potentially linking between them extension objects are done and data dump ordering is ensured. --- src/bin/pg_dump/pg_dump.c | 58 +-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 2b53c72..e210f88 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15236,7 +15236,8 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo) } /* - * getExtensionMembership --- obtain extension membership data + * getExtensionMembership --- obtain extension membership data and check FK + * dependencies among relations interacting with the ones in extensions. */ void getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[], @@ -15249,7 +15250,9 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[] int i_classid, i_objid, i_refclassid, -i_refobjid; +i_refobjid, +i_conrelid, +i_confrelid; DumpableObject *dobj, *refdobj; @@ -15431,6 +15434,57 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[] } destroyPQExpBuffer(query); + + /* + * Now that all the TableInfoData objects have been created for all + * the e
Re: [HACKERS] Bug in pg_dump
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > + /* > + * Query all the foreign key dependencies for all the > extension > + * tables found previously. Only tables whose data need > to be > + * have to be tracked. > + */ > + appendPQExpBuffer(query2, > + "SELECT conrelid, confrelid " > + "FROM pg_catalog.pg_constraint " > + "WHERE contype = 'f' AND conrelid IN > ("); > + > + for (j = 0; j < nconfigitems; j++) > + { [...] Instead of building up a (potentially) big list like this, couldn't we simply join against pg_extension and check if conrelid = ANY (extconfig) instead, based on the extension we're currently processing? eg: appendPQExpBuffer(query2, "SELECT conrelid, confrelid " "FROM pg_catalog.pg_constraint, pg_extension " "WHERE contype = 'f' AND extname = '%s' AND conrelid = ANY (extconfig)", fmtId(curext->dobj.name)); This seemed to work just fine for me, at least, and reduces the size of the patch a bit further since we don't need the loop that builds up the ids. > Subject: [PATCH 2/3] Make prove_check install contents of current directory > as well This is really an independent thing, no? I don't see any particular problem with it, for my part. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug in pg_dump
Michael, all, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold > wrote: > > This is a far better patch and the test to export/import of the > > postgis_topology extension works great for me. > > > > Thanks for the work. > > Attached is a patch that uses an even better approach by querying only > once all the FK dependencies of tables in extensions whose data is > registered as dumpable by getExtensionMembership(). Could you check > that it works correctly? My test case passes but an extra check would > be a good nice. The patch footprint is pretty low so we may be able to > backport this patch easily. I've started looking at this and it looks pretty simple and definitely something to backpatch (and mention in the release notes that existing pg_dump exports might be broken..). One thing that might be missing is what Jim brought up though- that this won't be able to deal with circular dependencies. I'm not sure that we need to care, but I *do* think we should document that in the extension documentation as unsupported. Perhaps in the future we can improve on this situation by setting up to drop and recreate the constraints, though another thought I had was to require extensions with circular dependencies to use deferrable constraints and then make sure we set constraints to deferred. That isn't something we'd want to backpatch though, so my plan is to push forward with this. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Bug in pg_dump
On Sat, Feb 28, 2015 at 12:29 AM, Gilles Darold wrote: > Le 26/02/2015 12:41, Michael Paquier a écrit : >> On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold >> wrote: >>> This is a far better patch and the test to export/import of the >>> postgis_topology extension works great for me. >>> >>> Thanks for the work. >> Attached is a patch that uses an even better approach by querying only >> once all the FK dependencies of tables in extensions whose data is >> registered as dumpable by getExtensionMembership(). Could you check >> that it works correctly? My test case passes but an extra check would >> be a good nice. The patch footprint is pretty low so we may be able to >> backport this patch easily. > > Works great too. I'm agree that this patch should be backported but I > think we need to warn admins in the release note that their > import/restore scripts may be broken. Of course it makes sense to mention that in the release notes, this behavior of pg_dump being broken since the creation of extensions. Since it is working on your side as well, let's put it in the hands of a committer then and I am marking it as such on the CF app. The test case I sent on this thread can be used to reproduce the problem easily with TAP tests... -- 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] Bug in pg_dump
On Thu, Feb 26, 2015 at 08:41:46PM +0900, Michael Paquier wrote: > On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold > wrote: > > This is a far better patch and the test to export/import of the > > postgis_topology extension works great for me. > > > > Thanks for the work. > > Attached is a patch that uses an even better approach by querying > only once all the FK dependencies of tables in extensions whose data > is registered as dumpable by getExtensionMembership(). Could you > check that it works correctly? My test case passes but an extra > check would be a good nice. The patch footprint is pretty low so we > may be able to backport this patch easily. +1 for backporting. It's a real bug, and real people get hit by it if they're using PostGIS, one of our most popular add-ons. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Bug in pg_dump
Le 26/02/2015 12:41, Michael Paquier a écrit : > On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold > wrote: >> This is a far better patch and the test to export/import of the >> postgis_topology extension works great for me. >> >> Thanks for the work. > Attached is a patch that uses an even better approach by querying only > once all the FK dependencies of tables in extensions whose data is > registered as dumpable by getExtensionMembership(). Could you check > that it works correctly? My test case passes but an extra check would > be a good nice. The patch footprint is pretty low so we may be able to > backport this patch easily. Works great too. I'm agree that this patch should be backported but I think we need to warn admins in the release note that their import/restore scripts may be broken. One of the common workaround about this issue was to not take care of the error during data import and then reload data from the tables with FK errors at the end of the import. If the admins are not warned, this can conduct to duplicate entries or return an error. -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org -- 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] Bug in pg_dump
On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold wrote: > This is a far better patch and the test to export/import of the > postgis_topology extension works great for me. > > Thanks for the work. Attached is a patch that uses an even better approach by querying only once all the FK dependencies of tables in extensions whose data is registered as dumpable by getExtensionMembership(). Could you check that it works correctly? My test case passes but an extra check would be a good nice. The patch footprint is pretty low so we may be able to backport this patch easily. -- Michael From 72e369ee725301003cf065b0bf9875724455dac1 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 17 Feb 2015 07:39:23 + Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with constraints Additional checks on FK constraints potentially linking between them extension objects are done and data dump ordering is ensured. Note that this does not take into account foreign keys of tables that are not part of an extension linking to an extension table. --- src/bin/pg_dump/pg_dump.c | 80 +-- 1 file changed, 78 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 2b53c72..bbcd600 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15236,7 +15236,8 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo) } /* - * getExtensionMembership --- obtain extension membership data + * getExtensionMembership --- obtain extension membership data and check FK + * dependencies among extension tables. */ void getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[], @@ -15368,7 +15369,9 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[] parsePGArray(extcondition, &extconditionarray, &nconditionitems) && nconfigitems == nconditionitems) { - int j; + int j, i_conrelid, i_confrelid; + PQExpBuffer query2; + bool first_elt = true; for (j = 0; j < nconfigitems; j++) { @@ -15423,6 +15426,79 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[] } } } + + /* + * Now that all the TableInfoData objects have been created for + * all the extensions, check their FK dependencies and register + * them to ensure correct data ordering. Note that this is not + * a problem for user tables not included in an extension + * referencing with a FK tables in extensions as their constraint + * is declared after dumping their data. In --data-only mode the + * table ordering is ensured as well thanks to + * getTableDataFKConstraints(). + */ + query2 = createPQExpBuffer(); + + /* + * Query all the foreign key dependencies for all the extension + * tables found previously. Only tables whose data need to be + * have to be tracked. + */ + appendPQExpBuffer(query2, + "SELECT conrelid, confrelid " + "FROM pg_catalog.pg_constraint " + "WHERE contype = 'f' AND conrelid IN ("); + + for (j = 0; j < nconfigitems; j++) + { +TableInfo *configtbl; +Oid configtbloid = atooid(extconfigarray[j]); + +configtbl = findTableByOid(configtbloid); +if (configtbl == NULL || configtbl->dataObj == NULL) + continue; + +if (first_elt) +{ + appendPQExpBuffer(query2, "%u", configtbloid); + first_elt = false; +} +else + appendPQExpBuffer(query2, ", %u", configtbloid); + } + appendPQExpBuffer(query2, ");"); + + res = ExecuteSqlQuery(fout, query2->data, PGRES_TUPLES_OK); + ntups = PQntuples(res); + + i_conrelid = PQfnumber(res, "conrelid"); + i_confrelid = PQfnumber(res, "confrelid"); + + /* Now get the dependencies and register them */ + for (j = 0; j < ntups; j++) + { +Oid conrelid, confrelid; +TableInfo *reftable, *contable; + +conrelid = atooid(PQgetvalue(res, j, i_conrelid)); +confrelid = atooid(PQgetvalue(res, j, i_confrelid)); +contable = findTableByOid(conrelid); +reftable = findTableByOid(confrelid); + +if (reftable == NULL || + reftable->dataObj == NULL || + contable == NULL || + contable->dataObj == NULL) + continue; + +/* + * Make referencing TABLE_DATA object depend on the + * referenced table's TABLE_DATA object. + */ +addObjectDependency(&contable->dataObj->dobj, + reftable->dataObj->dobj.dumpId); + } + resetPQExpBuffer(query2); } if (extconfigarray) free(extconfigarray); -- 2.3.0 From 1d8fe1c39ec5049c39810f94153d347971738616 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 17 Feb 2015 22:29:28 +0900 Subject: [PATCH 2/3] Make prove_check install contents of current directory as well This is useful for example for TAP tests in extensions. --- src/Makefile.global.in | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 7
Re: [HACKERS] Bug in pg_dump
Le 24/02/2015 05:40, Michael Paquier a écrit : > > > On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold > mailto:gilles.dar...@dalibo.com>> wrote: > > Looks great to me, I have tested with the postgis_topology extension > everything works fine. > > > Actually, after looking more in depth at the internals of pg_dump I > think that both patches are wrong (did that yesterday night for > another patch). First this patch marks a table in an extension as > always dumpable: > + /* Mark member table as dumpable */ > + configtbl->dobj.dump = true; > And then many checks on ext_member are added in many code paths to > ensure that objects in extensions have their definition never dumped. > But actually this assumption is not true all the time per this code in > getExtensionMemberShip: > if (!dopt->binary_upgrade) > dobj->dump = false; > else > dobj->dump = refdobj->dump; > So this patch would break binary upgrade where some extension objects > should be dumped (one reason why I haven't noticed that before is that > pg_upgrade tests do not include extensions). > > Hence, one idea coming to my mind to fix the problem would be to add > some extra processing directly in getExtensionMembership() after > building the objects DO_TABLE_DATA with makeTableDataInfo() by > checking the FK dependencies and add a dependency link with > addObjectDependency. The good part with that is that even user tables > that reference extension tables with a FK can be restored correctly > because their constraint is added *after* loading the data. I noticed > as well that with this patch the --data-only mode was dumping tables > in the correct order. > > Speaking of which, patches implementing this idea are attached. The > module test case has been improved as well with a check using a table > not in an extension linked with a FK to a table in an extension. > -- > Michael This is a far better patch and the test to export/import of the postgis_topology extension works great for me. Thanks for the work. -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org
Re: [HACKERS] Bug in pg_dump
On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold wrote: > Looks great to me, I have tested with the postgis_topology extension > everything works fine. > Actually, after looking more in depth at the internals of pg_dump I think that both patches are wrong (did that yesterday night for another patch). First this patch marks a table in an extension as always dumpable: + /* Mark member table as dumpable */ + configtbl->dobj.dump = true; And then many checks on ext_member are added in many code paths to ensure that objects in extensions have their definition never dumped. But actually this assumption is not true all the time per this code in getExtensionMemberShip: if (!dopt->binary_upgrade) dobj->dump = false; else dobj->dump = refdobj->dump; So this patch would break binary upgrade where some extension objects should be dumped (one reason why I haven't noticed that before is that pg_upgrade tests do not include extensions). Hence, one idea coming to my mind to fix the problem would be to add some extra processing directly in getExtensionMembership() after building the objects DO_TABLE_DATA with makeTableDataInfo() by checking the FK dependencies and add a dependency link with addObjectDependency. The good part with that is that even user tables that reference extension tables with a FK can be restored correctly because their constraint is added *after* loading the data. I noticed as well that with this patch the --data-only mode was dumping tables in the correct order. Speaking of which, patches implementing this idea are attached. The module test case has been improved as well with a check using a table not in an extension linked with a FK to a table in an extension. -- Michael From 7fb84d68df023d913c448f2498987ca4f0a70595 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 17 Feb 2015 07:39:23 + Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with constraints Additional checks on FK constraints potentially linking between them extension objects are done and data dump ordering is ensured. Note that this does not take into account foreign keys of tables that are not part of an extension linking to an extension table. --- src/bin/pg_dump/pg_dump.c | 56 ++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 2b53c72..5b1b240 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15236,7 +15236,8 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo) } /* - * getExtensionMembership --- obtain extension membership data + * getExtensionMembership --- obtain extension membership data and check FK + * dependencies among extension tables. */ void getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[], @@ -15423,6 +15424,59 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[] } } } + + /* + * Now that all the TableInfoData objects have been created for + * all the extensions, check their FK dependencies and register + * them to ensure correct data ordering. Note that this is not + * a problem for user tables not included in an extension + * referencing with a FK tables in extensions as their constraint + * is declared after dumping their data. In --data-only mode the + * table ordering is ensured as well thanks to + * getTableDataFKConstraints(). + */ + for (j = 0; j < nconfigitems; j++) + { +int i_confrelid, k; +PQExpBuffer query2 = createPQExpBuffer(); +TableInfo *configtbl; +Oid configtbloid = atooid(extconfigarray[j]); + +configtbl = findTableByOid(configtbloid); +if (configtbl == NULL || configtbl->dataObj == NULL) + continue; + +appendPQExpBuffer(query2, + "SELECT confrelid " + "FROM pg_catalog.pg_constraint " + "WHERE conrelid = '%u' " + "AND contype = 'f'", + configtbloid); + +res = ExecuteSqlQuery(fout, query2->data, PGRES_TUPLES_OK); +ntups = PQntuples(res); +i_confrelid = PQfnumber(res, "confrelid"); + +for (k = 0; k < ntups; k++) +{ + Oid confrelid; + TableInfo *reftable; + + confrelid = atooid(PQgetvalue(res, k, i_confrelid)); + reftable = findTableByOid(confrelid); + + if (reftable == NULL || reftable->dataObj == NULL) + continue; + + /* + * Make referencing TABLE_DATA object depend on the + * referenced table's TABLE_DATA object. + */ + addObjectDependency(&configtbl->dataObj->dobj, + reftable->dataObj->dobj.dumpId); +} +resetPQExpBuffer(query2); + } } if (extconfigarray) free(extconfigarray); -- 2.3.0 From ad5cd360243b44e735195c5e94df3c21e8f18e07 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 17 Feb 2015 22:29:28 +0900 Subject: [PATCH 2/3] Make prove_check install contents of current
Re: [HACKERS] Bug in pg_dump
Le 17/02/2015 14:44, Michael Paquier a écrit : > On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold > wrote: >> Le 19/01/2015 14:41, Robert Haas a écrit : >>> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold >>> wrote: I attach a patch that solves the issue in pg_dump, let me know if it might be included in Commit Fest or if the three other solutions are a better choice. >>> I think a fix in pg_dump is appropriate, so I'd encourage you to add >>> this to the next CommitFest. >>> >> Ok, thanks Robert. The patch have been added to next CommitFest under >> the Bug Fixes section. >> >> I've also made some review of the patch and more test. I've rewritten >> some comments and added a test when TableInfo is NULL to avoid potential >> pg_dump crash. >> >> New patch attached: pg_dump.c-extension-FK-v2.patch > So, I looked at your patch and I have some comments... > > The approach taken by the patch looks correct to me as we cannot > create FK constraints after loading the data in the case of an > extension, something that can be done with a data-only dump. > > Now, I think that the routine hasExtensionMember may impact > performance unnecessarily in the case of databases with many tables, > say thousands or more. And we can easily avoid this performance > problem by checking the content of each object dumped by doing the > legwork directly in getTableData. Also, most of the NULL pointer > checks are not needed as most of those code paths would crash if > tbinfo is NULL, and actually only keeping the one in dumpConstraint is > fine. Yes that's exactly what we discuss at Moscow, thanks for removing the hasExtensionMember() routine. About NULL pointer I'm was not sure that it could not crash on some other parts so I decided to check it everywhere. If that's ok to just check in dumpConstraint() that's fine. > On top of those things, I have written a small extension able to > reproduce the problem that I have included as a test module in > src/test/modules. The dump/restore check is done using the TAP tests, > and I have hacked prove_check to install as well the contents of the > current folder to be able to use the TAP routines with an extension > easily. IMO, as we have no coverage of pg_dump with extensions I think > that it would be useful to ensure that this does not break again in > the future. Great ! I did not had time to do that, thank you so much. > All the hacking I have done during the review results in the set of > patch attached: > - 0001 is your patch, Gilles, with some comment fixes as well as the > performance issue with hasExtensionMember fix > - 0002 is the modification of prove_check that makes TAP tests usable > with extensions > - 0003 is the test module covering the tests needed for pg_dump, at > least for the problem of this thread. > > Gilles, how does that look to you? Looks great to me, I have tested with the postgis_topology extension everything works fine. > (Btw, be sure to generate your patches directly with git next time. :) ) Sorry, some old reminiscence :-) Thanks for the review. Best regards, -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org -- 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] Bug in pg_dump
On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold wrote: > Le 19/01/2015 14:41, Robert Haas a écrit : >> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold >> wrote: >>> I attach a patch that solves the issue in pg_dump, let me know if it might >>> be included in Commit Fest or if the three other solutions are a better >>> choice. >> I think a fix in pg_dump is appropriate, so I'd encourage you to add >> this to the next CommitFest. >> > Ok, thanks Robert. The patch have been added to next CommitFest under > the Bug Fixes section. > > I've also made some review of the patch and more test. I've rewritten > some comments and added a test when TableInfo is NULL to avoid potential > pg_dump crash. > > New patch attached: pg_dump.c-extension-FK-v2.patch So, I looked at your patch and I have some comments... The approach taken by the patch looks correct to me as we cannot create FK constraints after loading the data in the case of an extension, something that can be done with a data-only dump. Now, I think that the routine hasExtensionMember may impact performance unnecessarily in the case of databases with many tables, say thousands or more. And we can easily avoid this performance problem by checking the content of each object dumped by doing the legwork directly in getTableData. Also, most of the NULL pointer checks are not needed as most of those code paths would crash if tbinfo is NULL, and actually only keeping the one in dumpConstraint is fine. On top of those things, I have written a small extension able to reproduce the problem that I have included as a test module in src/test/modules. The dump/restore check is done using the TAP tests, and I have hacked prove_check to install as well the contents of the current folder to be able to use the TAP routines with an extension easily. IMO, as we have no coverage of pg_dump with extensions I think that it would be useful to ensure that this does not break again in the future. All the hacking I have done during the review results in the set of patch attached: - 0001 is your patch, Gilles, with some comment fixes as well as the performance issue with hasExtensionMember fix - 0002 is the modification of prove_check that makes TAP tests usable with extensions - 0003 is the test module covering the tests needed for pg_dump, at least for the problem of this thread. Gilles, how does that look to you? (Btw, be sure to generate your patches directly with git next time. :) ) Regards, -- Michael From 8005cffd08c57b77564bb0294d8ebd4600bc1dcf Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 17 Feb 2015 07:39:23 + Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with constraints The same mechanism as data-only dumps ensuring that data is loaded respecting foreign key constains is used if it at least one dumped object is found as being part of an extension. This commit reinforces as well a couple of code paths to not dump objects that are directly part of an extension. Patch by Gilles Darold. --- src/bin/pg_dump/pg_dump.c | 99 +++ 1 file changed, 83 insertions(+), 16 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 7e92b74..c95ae3d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -208,7 +208,8 @@ static void addBoundaryDependencies(DumpableObject **dobjs, int numObjs, DumpableObject *boundaryObjs); static void getDomainConstraints(Archive *fout, TypeInfo *tyinfo); -static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids); +static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, + bool oids, bool *has_ext_member); static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids); static void buildMatViewRefreshDependencies(Archive *fout); static void getTableDataFKConstraints(void); @@ -730,9 +731,12 @@ main(int argc, char **argv) if (!dopt.schemaOnly) { - getTableData(&dopt, tblinfo, numTables, dopt.oids); + bool has_ext_member = false; + + getTableData(&dopt, tblinfo, numTables, dopt.oids, &has_ext_member); buildMatViewRefreshDependencies(fout); - if (dopt.dataOnly) + + if (dopt.dataOnly || has_ext_member) getTableDataFKConstraints(); } @@ -1866,7 +1870,8 @@ refreshMatViewData(Archive *fout, TableDataInfo *tdinfo) * set up dumpable objects representing the contents of tables */ static void -getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids) +getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, + bool oids, bool *has_ext_member) { int i; @@ -1874,6 +1879,8 @@ getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids) { if (tblinfo[i].dobj.dump) makeTableDataInfo(dopt, &(tblinfo[i]), oids); + if (!(*has_ext_member) && tblinfo[i].dobj.ext_member) + *has_ext_member = true; } } @@ -2052,13 +2059,15 @@ buildMatViewRefreshDependenci
Re: [HACKERS] Bug in pg_dump
Le 19/01/2015 14:41, Robert Haas a écrit : > On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold > wrote: >> I attach a patch that solves the issue in pg_dump, let me know if it might >> be included in Commit Fest or if the three other solutions are a better >> choice. > I think a fix in pg_dump is appropriate, so I'd encourage you to add > this to the next CommitFest. > Ok, thanks Robert. The patch have been added to next CommitFest under the Bug Fixes section. I've also made some review of the patch and more test. I've rewritten some comments and added a test when TableInfo is NULL to avoid potential pg_dump crash. New patch attached: pg_dump.c-extension-FK-v2.patch Regards -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org --- ../postgresql/src/bin/pg_dump/pg_dump.c 2015-01-19 19:03:45.897706879 +0100 +++ src/bin/pg_dump/pg_dump.c 2015-01-20 11:22:01.144794489 +0100 @@ -209,6 +209,7 @@ static void getDomainConstraints(Archive *fout, TypeInfo *tyinfo); static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids); +static bool hasExtensionMember(TableInfo *tblinfo, int numTables); static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids); static void buildMatViewRefreshDependencies(Archive *fout); static void getTableDataFKConstraints(void); @@ -730,9 +731,20 @@ if (!dopt.schemaOnly) { + bool has_ext_member; + getTableData(&dopt, tblinfo, numTables, dopt.oids); + + /* Search if there's dumpable table's members in an extension */ + has_ext_member = hasExtensionMember(tblinfo, numTables); + buildMatViewRefreshDependencies(fout); - if (dopt.dataOnly) + /* + * Get FK constraints even with schema+data if extension's + * members have FK because in this case tables need to be + * dump-ordered too. + */ + if (dopt.dataOnly || has_ext_member) getTableDataFKConstraints(); } @@ -1852,6 +1864,26 @@ } /* + * hasExtensionMember - + * return true when on of the dumpable object + * is an extension members + */ +static bool +hasExtensionMember(TableInfo *tblinfo, int numTables) +{ + int i; + + for (i = 0; i < numTables; i++) + { + if (tblinfo[i].dobj.ext_member) + return true; + } + + return false; +} + + +/* * Make a dumpable object for the data of this specific table * * Note: we make a TableDataInfo if and only if we are going to dump the @@ -2026,10 +2058,12 @@ * getTableDataFKConstraints - * add dump-order dependencies reflecting foreign key constraints * - * This code is executed only in a data-only dump --- in schema+data dumps - * we handle foreign key issues by not creating the FK constraints until - * after the data is loaded. In a data-only dump, however, we want to - * order the table data objects in such a way that a table's referenced + * This code is executed only in a data-only dump or when there is extension's + * member -- in schema+data dumps we handle foreign key issues by not creating + * the FK constraints until after the data is loaded. In a data-only dump or + * when there is an extension member to dump (schema dumps do not concern + * extension's objects, they are created during the CREATE EXTENSION), we want + * to order the table data objects in such a way that a table's referenced * tables are restored first. (In the presence of circular references or * self-references this may be impossible; we'll detect and complain about * that during the dependency sorting step.) @@ -2930,9 +2964,14 @@ PQExpBuffer delqry; const char *cmd; + /* Policy is SCHEMA only */ if (dopt->dataOnly) return; + /* CREATE EXTENSION should take care of that */ + if ((tbinfo != NULL) && tbinfo->dobj.ext_member) + return; + /* * If polname is NULL, then this record is just indicating that ROW * LEVEL SECURITY is enabled for the table. Dump as ALTER TABLE @@ -7884,6 +7923,10 @@ if (dopt->dataOnly) return; + /* CREATE EXTENSION should take care of that */ + if ((tbinfo != NULL) && tbinfo->dobj.ext_member) + return; + /* Search for comments associated with relation, using table */ ncomments = findComments(fout, tbinfo->dobj.catId.tableoid, @@ -13138,6 +13181,10 @@ if (dopt->dataOnly) return; + /* CREATE EXTENSION should take care of that */ + if ((tbinfo != NULL) && tbinfo->dobj.ext_member) + return; + /* Search for comments associated with relation, using table */ nlabels = findSecLabels(fout, tbinfo->dobj.catId.tableoid, @@ -13345,7 +13392,7 @@ static void dumpTable(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo) { - if (tbinfo->dobj.dump && !dopt->dataOnly) + if (tbinfo->dobj.dump && !dopt->dataOnly && !tbinfo->dobj.ext_member) { char *namecopy; @@ -13483,6 +13530,10 @@ int j, k; + /* CREATE EXTENSION should take care of that */ + if ((tbinfo != NULL) && tbinfo->dobj.ext_member) + return; + /* Make sure we are in proper schema */ selectSourceSchema(fout, tbinfo->dobj.namespace
Re: [HACKERS] Bug in pg_dump
On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold wrote: > I attach a patch that solves the issue in pg_dump, let me know if it might > be included in Commit Fest or if the three other solutions are a better > choice. I think a fix in pg_dump is appropriate, so I'd encourage you to add this to the next CommitFest. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Bug in pg_dump
On 16/01/2015 01:06, Jim Nasby wrote: > On 1/15/15 5:26 AM, Gilles Darold wrote: >> Hello, >> >> There's a long pending issue with pg_dump and extensions that have >> table members with foreign keys. This was previously reported in this >> thread >> http://www.postgresql.org/message-id/ca+tgmoyvzkadmgh_8el7uvm472geru0b4pnnfjqye6ss1k9...@mail.gmail.com >> and discuss by Robert. All PostgreSQL users that use the PostGis >> extension postgis_topology are facing the issue because the two >> members tables (topology and layer) are linked by foreign keys. >> >> If you dump a database with this extension and try to import it you >> will experience this error: >> >> pg_restore: [archiver (db)] Error while PROCESSING TOC: >> pg_restore: [archiver (db)] Error from TOC entry 3345; 0 >> 157059176 TABLE DATA layer gilles >> pg_restore: [archiver (db)] COPY failed for table "layer": ERROR: >> insert or update on table "layer" violates foreign key constraint >> "layer_topology_id_fkey" >> DETAIL: Key (topology_id)=(1) is not present in table "topology". >> WARNING: errors ignored on restore: 1 >> >> >> The problem is that, whatever export type you choose (plain/custom >> and full-export/data-only) the data of tables "topology" and "layer" >> are always exported in alphabetic order. I think this is a bug >> because outside extension, in data-only export, pg_dump is able to >> find foreign keys dependency and dump table's data in the right order >> but not with extension's members. Default is alphabetic order but >> that should not be the case with extension's members because >> constraints are recreated during the CREATE EXTENSION order. I hope I >> am clear enough. >> >> Here we have three solutions: >> >> 1/ Inform developers of extensions to take care to alphabetical >> order when they have member tables using foreign keys. >> 2/ Inform DBAs that they have to restore the failing table >> independently. The use case above can be resumed using the following >> command: >> >> pg_restore -h localhost -n topology -t layer -Fc -d >> testdb_empty testdump.dump >> >> 3/ Inform DBAs that they have to restore the schema first then >> the data only using --disable-triggers > > I don't like 1-3, and I doubt anyone else does... > >> 4/ Patch pg_dump to solve this issue. > > 5. Disable FK's during load. > This is really a bigger item than just extensions. It would have the > nice benefit of doing a wholesale FK validation instead of firing > per-row triggers, but it would leave the database in a weird state if > a restore failed... I think this is an other problem. Here we just need to apply to extension's members tables the same work than to normal tables. I guess this is what this patch try to solve. > >> I attach a patch that solves the issue in pg_dump, let me know if it >> might be included in Commit Fest or if the three other solutions are >> a better choice. I also join a sample extension (test_fk_in_ext) to >> be able to reproduce the issue and test the patch. Note that it might >> exists a simpler solution than the one I used in this patch, if this >> is the case please point me on the right way, I will be pleased to >> rewrite and send an other patch. > > The only problem I see with this approach is circular FK's: > > decibel@decina.local=# create table a(a_id serial primary key, b_id int); > CREATE TABLE > decibel@decina.local=# create table b(b_id serial primary key, a_id > int references a); > CREATE TABLE > decibel@decina.local=# alter table a add foreign key(b_id) references b; > ALTER TABLE > decibel@decina.local=# > > That's esoteric enough that I think it's OK not to directly support > them, but pg_dump shouldn't puke on them (and really should throw a > warning). Though it looks like it doesn't handle that in the data-only > case anyway... The patch is taking care or circular references and you will be warn if pg_dump found it in the extension members. That was not the case before. If you try do dump a database with the postgis extension you will be warn about FK defined on the edge_data table. -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org -- 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] Bug in pg_dump
On 1/15/15 5:26 AM, Gilles Darold wrote: Hello, There's a long pending issue with pg_dump and extensions that have table members with foreign keys. This was previously reported in this thread http://www.postgresql.org/message-id/ca+tgmoyvzkadmgh_8el7uvm472geru0b4pnnfjqye6ss1k9...@mail.gmail.com and discuss by Robert. All PostgreSQL users that use the PostGis extension postgis_topology are facing the issue because the two members tables (topology and layer) are linked by foreign keys. If you dump a database with this extension and try to import it you will experience this error: pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 3345; 0 157059176 TABLE DATA layer gilles pg_restore: [archiver (db)] COPY failed for table "layer": ERROR: insert or update on table "layer" violates foreign key constraint "layer_topology_id_fkey" DETAIL: Key (topology_id)=(1) is not present in table "topology". WARNING: errors ignored on restore: 1 The problem is that, whatever export type you choose (plain/custom and full-export/data-only) the data of tables "topology" and "layer" are always exported in alphabetic order. I think this is a bug because outside extension, in data-only export, pg_dump is able to find foreign keys dependency and dump table's data in the right order but not with extension's members. Default is alphabetic order but that should not be the case with extension's members because constraints are recreated during the CREATE EXTENSION order. I hope I am clear enough. Here we have three solutions: 1/ Inform developers of extensions to take care to alphabetical order when they have member tables using foreign keys. 2/ Inform DBAs that they have to restore the failing table independently. The use case above can be resumed using the following command: pg_restore -h localhost -n topology -t layer -Fc -d testdb_empty testdump.dump 3/ Inform DBAs that they have to restore the schema first then the data only using --disable-triggers I don't like 1-3, and I doubt anyone else does... 4/ Patch pg_dump to solve this issue. 5. Disable FK's during load. This is really a bigger item than just extensions. It would have the nice benefit of doing a wholesale FK validation instead of firing per-row triggers, but it would leave the database in a weird state if a restore failed... I attach a patch that solves the issue in pg_dump, let me know if it might be included in Commit Fest or if the three other solutions are a better choice. I also join a sample extension (test_fk_in_ext) to be able to reproduce the issue and test the patch. Note that it might exists a simpler solution than the one I used in this patch, if this is the case please point me on the right way, I will be pleased to rewrite and send an other patch. The only problem I see with this approach is circular FK's: decibel@decina.local=# create table a(a_id serial primary key, b_id int); CREATE TABLE decibel@decina.local=# create table b(b_id serial primary key, a_id int references a); CREATE TABLE decibel@decina.local=# alter table a add foreign key(b_id) references b; ALTER TABLE decibel@decina.local=# That's esoteric enough that I think it's OK not to directly support them, but pg_dump shouldn't puke on them (and really should throw a warning). Though it looks like it doesn't handle that in the data-only case anyway... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug in pg_dump
Hello, There's a long pending issue with pg_dump and extensions that have table members with foreign keys. This was previously reported in this thread http://www.postgresql.org/message-id/ca+tgmoyvzkadmgh_8el7uvm472geru0b4pnnfjqye6ss1k9...@mail.gmail.com and discuss by Robert. All PostgreSQL users that use the PostGis extension postgis_topology are facing the issue because the two members tables (topology and layer) are linked by foreign keys. If you dump a database with this extension and try to import it you will experience this error: pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 3345; 0 157059176 TABLE DATA layer gilles pg_restore: [archiver (db)] COPY failed for table "layer": ERROR: insert or update on table "layer" violates foreign key constraint "layer_topology_id_fkey" DETAIL: Key (topology_id)=(1) is not present in table "topology". WARNING: errors ignored on restore: 1 The problem is that, whatever export type you choose (plain/custom and full-export/data-only) the data of tables "topology" and "layer" are always exported in alphabetic order. I think this is a bug because outside extension, in data-only export, pg_dump is able to find foreign keys dependency and dump table's data in the right order but not with extension's members. Default is alphabetic order but that should not be the case with extension's members because constraints are recreated during the CREATE EXTENSION order. I hope I am clear enough. Here we have three solutions: 1/ Inform developers of extensions to take care to alphabetical order when they have member tables using foreign keys. 2/ Inform DBAs that they have to restore the failing table independently. The use case above can be resumed using the following command: pg_restore -h localhost -n topology -t layer -Fc -d testdb_empty testdump.dump 3/ Inform DBAs that they have to restore the schema first then the data only using --disable-triggers 4/ Patch pg_dump to solve this issue. I attach a patch that solves the issue in pg_dump, let me know if it might be included in Commit Fest or if the three other solutions are a better choice. I also join a sample extension (test_fk_in_ext) to be able to reproduce the issue and test the patch. Note that it might exists a simpler solution than the one I used in this patch, if this is the case please point me on the right way, I will be pleased to rewrite and send an other patch. In the test extension attached, there is a file called test_fk_in_ext/SYNOPSIS.txt that describe all actions to reproduce the issue and test the patch. Here is the SQL part of the test extension: CREATE TABLE IF NOT EXISTS b_test_fk_in_ext1 ( id int PRIMARY KEY ); CREATE TABLE IF NOT EXISTS a_test_fk_in_ext1 ( id int REFERENCES b_test_fk_in_ext1(id) ); SELECT pg_catalog.pg_extension_config_dump('b_test_fk_in_ext1', ''); SELECT pg_catalog.pg_extension_config_dump('a_test_fk_in_ext1', ''); Best regards, -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index dc062e6..49889ce 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -209,6 +209,7 @@ static void addBoundaryDependencies(DumpableObject **dobjs, int numObjs, static void getDomainConstraints(Archive *fout, TypeInfo *tyinfo); static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids); +static bool hasExtensionMember(TableInfo *tblinfo, int numTables); static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids); static void buildMatViewRefreshDependencies(Archive *fout); static void getTableDataFKConstraints(void); @@ -730,9 +731,17 @@ main(int argc, char **argv) if (!dopt.schemaOnly) { + bool has_ext_member; + getTableData(&dopt, tblinfo, numTables, dopt.oids); + /* Search if there is dumpable tables member of and extension */ + has_ext_member = hasExtensionMember(tblinfo, numTables); buildMatViewRefreshDependencies(fout); - if (dopt.dataOnly) + /* + * Always get FK constraints even with schema+data, extension's + * members can have FK so tables need to be dump-ordered. + */ + if (dopt.dataOnly || has_ext_member) getTableDataFKConstraints(); } @@ -1852,6 +1861,25 @@ getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids) } /* + * hasExtensionMember - + * set up dumpable objects representing the contents of tables + */ +static bool +hasExtensionMember(TableInfo *tblinfo, int numTables) +{ + int i; + + for (i = 0; i < numTables; i++) + { + if (tblinfo[i].dobj.ext_member) + return true; + } + + return false; +} + + +/* * Make a dumpable object for the data of this specific table * * Note: we make a TableDataInfo if and only if we are going to dump the @@ -2024,12 +2052,14 @@ buildMatViewRe
Re: [HACKERS] Bug in pg_dump
Alvaro Herrera writes: > Excerpts from Joel Jacobson's message of jue ene 13 06:31:06 -0300 2011: >> The example from Tom Lane below results in a database which is not >> possible to correctly dump using pg_dump. > I wouldn't care too much about that particular case -- you can't query > any of the views either. Yeah, the particular case is useless, but IIRC it's possible to construct non-useless cases where there's a circular dependency involving a view and something else (probably a function, but I'm too lazy to try to make an example right now). pg_dump's hack to break the circularity by separating the view from its rule can save the day in such cases. 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] Bug in pg_dump
Excerpts from Joel Jacobson's message of jue ene 13 06:31:06 -0300 2011: > The example from Tom Lane below results in a database which is not > possible to correctly dump using pg_dump. I wouldn't care too much about that particular case -- you can't query any of the views either. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Bug in pg_dump
* Joel Jacobson wrote: The example from Tom Lane below results in a database which is not possible to correctly dump using pg_dump. The view v1 strangely becomes a table in the dump output?! This is no bug, it's a feature (tm). pg_dump is clever enough to detect the circular dependency and break it open by creating v1 in two steps. A view in PostgreSQL is simply an empty table with an ON SELECT DO INSTEAD rule named "_RETURN" on it. pg_dump first creates the empty table, then view v2 depending on that table, and finally the _RETURN rule turning v1 into a view and reintroducing the circular dependency. -- Christian -- 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] Bug in pg_dump
On 2011-01-13 11:31 AM +0200, Joel Jacobson wrote: The example from Tom Lane below results in a database which is not possible to correctly dump using pg_dump. The view v1 strangely becomes a table in the dump output?! CREATE RULE "_RETURN" AS ON SELECT TO v1 DO INSTEAD SELECT v2.f1, v2.f2 FROM v2; This statement turns the table into a view. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug in pg_dump
The example from Tom Lane below results in a database which is not possible to correctly dump using pg_dump. The view v1 strangely becomes a table in the dump output?! It's probably a quite useless database to dump in the first place, but that is no excuse to generate an invalid dump, it would be better to throw an exception and complain about "your database is retarded, refusing to dump" or something like that. regression=# \d List of relations Schema | Name | Type | Owner +--+---+-- public | tt | table | postgres public | v1 | view | postgres public | v2 | view | postgres (3 rows) ubuntu@ubuntu:/crypt/postgresql-9.1alpha3/src/bin/pg_dump$ ./pg_dump regression | grep -v -E '^--' | grep -E '^.+$' | grep -v SET CREATE OR REPLACE PROCEDURAL LANGUAGE plpgsql; ALTER PROCEDURAL LANGUAGE plpgsql OWNER TO ubuntu; CREATE TABLE tt ( f1 integer, f2 integer ); ALTER TABLE public.tt OWNER TO postgres; CREATE TABLE v1 ( f1 integer, f2 integer ); ALTER TABLE public.v1 OWNER TO postgres; CREATE VIEW v2 AS SELECT v1.f1, v1.f2 FROM v1; ALTER TABLE public.v2 OWNER TO postgres; COPY tt (f1, f2) FROM stdin; \. CREATE RULE "_RETURN" AS ON SELECT TO v1 DO INSTEAD SELECT v2.f1, v2.f2 FROM v2; REVOKE ALL ON SCHEMA public FROM PUBLIC; REVOKE ALL ON SCHEMA public FROM ubuntu; GRANT ALL ON SCHEMA public TO ubuntu; GRANT ALL ON SCHEMA public TO PUBLIC; 2011/1/12 Tom Lane : > regression=# create table tt(f1 int, f2 int); > CREATE TABLE > regression=# create view v1 as select * from tt; > CREATE VIEW > regression=# create view v2 as select * from v1; > CREATE VIEW > regression=# create or replace view v1 as select * from v2; > CREATE VIEW > regression=# drop view v1; > ERROR: cannot drop view v1 because other objects depend on it > DETAIL: view v2 depends on view v1 > HINT: Use DROP ... CASCADE to drop the dependent objects too. > regression=# drop view v2; > ERROR: cannot drop view v2 because other objects depend on it > DETAIL: view v1 depends on view v2 > HINT: Use DROP ... CASCADE to drop the dependent objects too. > > This isn't particularly *useful*, maybe, but it's hardly "impossible". > And if we analyzed function dependencies in any detail, circular > dependencies among functions would be possible (and useful). > > regards, tom lane -- Best regards, Joel Jacobson Glue Finance -- 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] Bug in pg_dump -c with casts
Actually, scratch that - I'm wrong... It appeared separately from the other DROP commands... Chris Christopher Kings-Lynne wrote: Hi, Playing around with this MySQL compatibility library, I noticed that pg_dump -c does not emit DROP commands for casts. Seems like a bug...? Chris ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
[HACKERS] Bug in pg_dump -c with casts
Hi, Playing around with this MySQL compatibility library, I noticed that pg_dump -c does not emit DROP commands for casts. Seems like a bug...? Chris ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] bug in pg_dump ALTER DATABASE
So what are we going to do about this problem? The pg_settings view does not have enough information to determine it generically. (It only says 'string', not 'list'...) I propose that we modify pg_dumpall to hard-code the set of list-type GUC variables for each backend version. The current (CVS) list of such GUCs is: * DateStyle * preload_libraries * search_path * log_destination * custom_variable_classes (probably doesn't need to be worried about) Shall I go ahead and do this? Oh, and we'll need to fix the pg_settings view for the future, because otherwise it will make life difficult for GUI writies (like me)... Chris ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [HACKERS] bug in pg_dump ALTER DATABASE
As part of my testing, I noticed this bug. My database has a search_path set in the database vars. It dumps lik ethis: DROP DATABASE usa; CREATE DATABASE usa WITH TEMPLATE = template0 OWNER = usadmin ENCODING = 'LATIN1'; ALTER DATABASE usa SET search_path TO 'public, contrib'; Notice the single quotes around the TO bit? That's completely broken. Those '' must not be there. Is a fix for this required for only search_path, or is it a more general problem? So what are we going to do about this problem? The pg_settings view does not have enough information to determine it generically. (It only says 'string', not 'list'...) I propose that we modify pg_dumpall to hard-code the set of list-type GUC variables for each backend version. The current (CVS) list of such GUCs is: * DateStyle * preload_libraries * search_path * log_destination * custom_variable_classes (probably doesn't need to be worried about) Shall I go ahead and do this? Chris ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] bug in pg_dump ALTER DATABASE
Christopher Kings-Lynne <[EMAIL PROTECTED]> writes: > Is a fix for this required for only search_path, or is it a more general > problem? I think this has to be driven off the GUC_LIST_INPUT and/or GUC_LIST_QUOTE flag bits (too late at night to remember just what those two flags do, but one or both determines this). One small problem is that pg_dump can't see the GUC flag bits AFAIK ... regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
[HACKERS] bug in pg_dump ALTER DATABASE
As part of my testing, I noticed this bug. My database has a search_path set in the database vars. It dumps lik ethis: DROP DATABASE usa; CREATE DATABASE usa WITH TEMPLATE = template0 OWNER = usadmin ENCODING = 'LATIN1'; ALTER DATABASE usa SET search_path TO 'public, contrib'; Notice the single quotes around the TO bit? That's completely broken. Those '' must not be there. Is a fix for this required for only search_path, or is it a more general problem? Chris ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] Bug in pg_dump 7.4
Sorry, this was 7.4 beta 3 ( I was upgrading one test database from 7.4 beta 3, pg_restore was version 7.4.2). You are right, fn_chk_doc_id is bool type. However I'll try to dump upgraded database with a new version of pg_dump and let You know. Sorry again :-( Regards ! - Original Message - From: "Bruno Wolff III" <[EMAIL PROTECTED]> To: "Rod Taylor" <[EMAIL PROTECTED]> Cc: "Darko Prenosil" <[EMAIL PROTECTED]>; "PostgreSQL Development" <[EMAIL PROTECTED]> Sent: Thursday, May 06, 2004 6:41 PM Subject: Re: [HACKERS] Bug in pg_dump 7.4 > On Thu, May 06, 2004 at 10:17:31 -0400, > Rod Taylor <[EMAIL PROTECTED]> wrote: > > > CREATE DOMAIN doc_ident AS bigint NOT NULL DEFAULT > > > nextval('doc.seq_doc_id'::text) > > > CONSTRAINT cnst_chk_doc_id CHECK ( fn_chk_doc_id(VALUE) ) ; > > > > > > I did not notice any similar error report on the list, so I believe that this > > > is not fixed yet ? > > > > It comes out right for me in 7.4.2. > > What type is fn_chk_doc_id? There was a bug like this for boolean variables > in the 7.4 beta. Maybe there is a similar bug for boolean functions? > Just to be sure, this is happening in a released version of 7.4, not a beta > version, correct? > > ---(end of broadcast)--- > TIP 8: explain analyze is your friend > ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] Bug in pg_dump 7.4
On Thu, May 06, 2004 at 10:17:31 -0400, Rod Taylor <[EMAIL PROTECTED]> wrote: > > CREATE DOMAIN doc_ident AS bigint NOT NULL DEFAULT > > nextval('doc.seq_doc_id'::text) > > CONSTRAINT cnst_chk_doc_id CHECK ( fn_chk_doc_id(VALUE) ) ; > > > > I did not notice any similar error report on the list, so I believe that this > > is not fixed yet ? > > It comes out right for me in 7.4.2. What type is fn_chk_doc_id? There was a bug like this for boolean variables in the 7.4 beta. Maybe there is a similar bug for boolean functions? Just to be sure, this is happening in a released version of 7.4, not a beta version, correct? ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] Bug in pg_dump 7.4
Rod Taylor <[EMAIL PROTECTED]> writes: >> CREATE DOMAIN doc_ident AS bigint NOT NULL DEFAULT >> nextval('doc.seq_doc_id'::text) >> CONSTRAINT cnst_chk_doc_id CHECK ( fn_chk_doc_id(VALUE) ) ; > It comes out right for me in 7.4.2. AFAICT the relevant fix was well before 7.4 release: 2003-10-04 14:22 tgl * src/: backend/utils/adt/ruleutils.c, backend/utils/cache/lsyscache.c, include/utils/lsyscache.h: Fix pg_get_constraintdef() to ensure CHECK constraints are always shown with required outer parentheses. Breakage seems to be leftover from domain-constraint patches. This could be smarter about suppressing extra parens, but at this stage of the release cycle I want certainty not cuteness. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] Bug in pg_dump 7.4
> CREATE DOMAIN doc_ident AS bigint NOT NULL DEFAULT > nextval('doc.seq_doc_id'::text) > CONSTRAINT cnst_chk_doc_id CHECK ( fn_chk_doc_id(VALUE) ) ; > > I did not notice any similar error report on the list, so I believe that this > is not fixed yet ? It comes out right for me in 7.4.2. ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
[HACKERS] Bug in pg_dump 7.4
Part of dump file: CREATE DOMAIN doc_ident AS bigint NOT NULL DEFAULT nextval('doc.seq_doc_id'::text) CONSTRAINT cnst_chk_doc_id CHECK fn_chk_doc_id(VALUE); It should look like this: CREATE DOMAIN doc_ident AS bigint NOT NULL DEFAULT nextval('doc.seq_doc_id'::text) CONSTRAINT cnst_chk_doc_id CHECK ( fn_chk_doc_id(VALUE) ) ; I did not notice any similar error report on the list, so I believe that this is not fixed yet ? Regards ! ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
[HACKERS] bug in pg_dump with GRANT/REVOKE
I'm running postgres 6.5.3 and 7.0.3 and pg_dump gives me the following output: DROP TABLE "genrenametable"; CREATE TABLE "genrenametable" ( "genreid" int4, "name" character varying(128), "parentgenre" int4, "enabled" bool DEFAULT 'f' NOT NULL ); REVOKE ALL on "genrenametable" from PUBLIC; GRANT SELECT on "genrenametable" to "hammor"; GRANT SELECT on "genrenametable" to "johnbr"; COPY "genrenametable" FROM stdin; 411580s Alt Hits4096t 4138New Wave Hits 4096t 411790s Alt Hits4096t ... As you can guess, this will not successfully restore the table. Perhaps the REVOKE/GRANT statements can be moved to after the COPY. The fancy solution would be to make sure the table owner has permissions to do the COPY, and then revoke the permissions afterward if necessary. ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster