Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Robert Haas]

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.

[Tom Lane]

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.

[Robert Haas]

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.

[Tom Lane]

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.

[Jeevan Ladhe]

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.

[Ashutosh Bapat]

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.

[Ashutosh Bapat]

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 

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Tom Lane]

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.

[Robert Haas]

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.

[amul sul]

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.

[Ashutosh Bapat]

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.

[Jeevan Ladhe]

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.

[Ashutosh Bapat]

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.

[Amit Langote]

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.

[Ashutosh Bapat]

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.

[Amit Langote]

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
--- 

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Jeevan Ladhe]

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.

[Jeevan Ladhe]

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.

[amul sul]

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

[Michael Paquier]

On Wed, Mar 4, 2015 at 2:03 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut pete...@gmx.net 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

[Peter Eisentraut]

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

[Michael Paquier]

On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut pete...@gmx.net 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

[Michael Paquier]

On Tue, Mar 3, 2015 at 4:57 AM, Stephen Frost sfr...@snowman.net wrote:
 * Michael Paquier (michael.paqu...@gmail.com) wrote:
 On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost sfr...@snowman.net 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

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
 On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost sfr...@snowman.net 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

[Stephen Frost]

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 sfr...@snowman.net
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 commandALTER EXTENSION ... DROP TABLE/.
 /para
+
+para
+ 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.
+/para
/sect2
 
sect2
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 Paquier]

On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost sfr...@snowman.net 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 Paquier]

On Sun, Mar 1, 2015 at 1:09 AM, Stephen Frost sfr...@snowman.net 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 michael@otacoo.com
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 extensions, check their FK dependencies and 

Re: [HACKERS] Bug in pg_dump

[Stephen Frost]

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

[Stephen Frost]

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

[Stephen Frost]

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

[Stephen Frost]

Michael, all,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
 On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold gilles.dar...@dalibo.com 
 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

[Stephen Frost]

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 Paquier]

On Sat, Feb 28, 2015 at 12:29 AM, Gilles Darold
gilles.dar...@dalibo.com wrote:
 Le 26/02/2015 12:41, Michael Paquier a écrit :
 On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold gilles.dar...@dalibo.com 
 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

[David Fetter]

On Thu, Feb 26, 2015 at 08:41:46PM +0900, Michael Paquier wrote:
 On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold gilles.dar...@dalibo.com 
 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 da...@fetter.org 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

[Gilles Darold]

Le 26/02/2015 12:41, Michael Paquier a écrit :
 On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold gilles.dar...@dalibo.com 
 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

[Michael Paquier]

On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold gilles.dar...@dalibo.com 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 mich...@otacoo.com
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 mich...@otacoo.com
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 

Re: [HACKERS] Bug in pg_dump

[Gilles Darold]

Le 24/02/2015 05:40, Michael Paquier a écrit :


 On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold
 gilles.dar...@dalibo.com 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

[Gilles Darold]

Le 17/02/2015 14:44, Michael Paquier a écrit :
 On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold gilles.dar...@dalibo.com 
 wrote:
 Le 19/01/2015 14:41, Robert Haas a écrit :
 On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold gilles.dar...@dalibo.com 
 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

[Michael Paquier]

On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold 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
From 7fb84d68df023d913c448f2498987ca4f0a70595 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
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 mich...@otacoo.com
Date: Tue, 17 Feb 2015 22:29:28 +0900
Subject: [PATCH 2/3] Make 

Re: [HACKERS] Bug in pg_dump

[Michael Paquier]

On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold gilles.dar...@dalibo.com wrote:
 Le 19/01/2015 14:41, Robert Haas a écrit :
 On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold gilles.dar...@dalibo.com 
 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 mich...@otacoo.com
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 

Re: [HACKERS] Bug in pg_dump

[Gilles Darold]

Le 19/01/2015 14:41, Robert Haas a écrit :
 On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold gilles.dar...@dalibo.com 
 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 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-dobj.name);

Re: [HACKERS] Bug in pg_dump

[Robert Haas]

On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold gilles.dar...@dalibo.com 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

[Gilles Darold]

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

[Jim Nasby]

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

[Gilles Darold]

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 @@ 

[HACKERS] Bug in pg_dump

[Joel Jacobson]

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 t...@sss.pgh.pa.us:
 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

[Marko Tiikkaja]

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

Re: [HACKERS] Bug in pg_dump

[Christian Ullrich]

* 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

[Alvaro Herrera]

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 alvhe...@commandprompt.com
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

[Tom Lane]

Alvaro Herrera alvhe...@commandprompt.com 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 -c with casts

[Christopher Kings-Lynne]

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

[Christopher Kings-Lynne]

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

[Christopher Kings-Lynne]

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]

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

[HACKERS] bug in pg_dump ALTER DATABASE

[Christopher Kings-Lynne]

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

[HACKERS] Bug in pg_dump 7.4

[Darko Prenosil]

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]

Re: [HACKERS] Bug in pg_dump 7.4

[Rod Taylor]

 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])

Re: [HACKERS] Bug in pg_dump 7.4

[Tom Lane]

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

[Bruno Wolff III]

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

[Darko Prenosil]

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

[HACKERS] bug in pg_dump with GRANT/REVOKE

[Robert Forsman]

  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