Thread: pg_dump partitions can lead to inconsistent state after restore

Per my comment at https://postgr.es/m/20190422225129.GA6126@alvherre.pgsql
I think that pg_dump can possibly cause bogus partition definitions,
when the users explicitly decide to join tables as partitions that have
different column ordering than the parent table.  Any COPY or INSERT
command without an explicit column list that tries to put tuples in the
table will fail after the restore.

Tom Lane said:

> I haven't looked at the partitioning code, but I am quite sure that that's
> always happened for old-style inheritance children, and I imagine pg_dump
> is just duplicating that old behavior.

Actually, the new code is unrelated to the old one; for legacy
inheritance, the children are always created exactly as they were
created at definition time.  If you use ALTER TABLE ... INHERITS
(attach a table as a children after creation) then obviously the child
table cannot be modified to match its new parent; and pg_dump reproduces
the exact column ordering that the table originally had.  If you use
"CREATE TABLE ... INHERITS (parent)" then the child columns are reordered
*at that point* (creation time); the dump will, again, reproduce the
exact same definition.


I think failing to reproduce the exact same definition is a pg_dump bug
that should be fixed and backpatched to pg10.  It's just sheer luck that
nobody has complained of being bitten by it.

-- 
Álvaro Herrera                                http://www.twitter.com/alvherre

On Wed, 24 Apr 2019 at 06:50, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Per my comment at https://postgr.es/m/20190422225129.GA6126@alvherre.pgsql
> I think that pg_dump can possibly cause bogus partition definitions,
> when the users explicitly decide to join tables as partitions that have
> different column ordering than the parent table.  Any COPY or INSERT
> command without an explicit column list that tries to put tuples in the
> table will fail after the restore.

Yeah, pg_dump itself is broken here, never mind dreaming up some other
user command.

We do use a column list when doing COPY, but with --inserts (not
--column-inserts) we don't include a column list.

All it takes is:

postgres=# create table listp (a int, b text) partition by list(a);
CREATE TABLE
postgres=# create table listp1 (b text, a int);
CREATE TABLE
postgres=# alter table listp attach partition listp1 for values in(1);
ALTER TABLE
postgres=# insert into listp values(1,'One');
INSERT 0 1
postgres=# \q

$ createdb test1
$ pg_dump --inserts postgres | psql test1
...
ERROR:  invalid input syntax for type integer: "One"
LINE 1: INSERT INTO public.listp1 VALUES ('One', 1);

That settles the debate on the other thread...

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2019/04/24 10:19, David Rowley wrote:
> On Wed, 24 Apr 2019 at 06:50, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Per my comment at https://postgr.es/m/20190422225129.GA6126@alvherre.pgsql
>> I think that pg_dump can possibly cause bogus partition definitions,
>> when the users explicitly decide to join tables as partitions that have
>> different column ordering than the parent table.  Any COPY or INSERT
>> command without an explicit column list that tries to put tuples in the
>> table will fail after the restore.
> 
> Yeah, pg_dump itself is broken here, never mind dreaming up some other
> user command.
> 
> We do use a column list when doing COPY, but with --inserts (not
> --column-inserts) we don't include a column list.
> 
> All it takes is:
> 
> postgres=# create table listp (a int, b text) partition by list(a);
> CREATE TABLE
> postgres=# create table listp1 (b text, a int);
> CREATE TABLE
> postgres=# alter table listp attach partition listp1 for values in(1);
> ALTER TABLE
> postgres=# insert into listp values(1,'One');
> INSERT 0 1
> postgres=# \q
> 
> $ createdb test1
> $ pg_dump --inserts postgres | psql test1
> ...
> ERROR:  invalid input syntax for type integer: "One"
> LINE 1: INSERT INTO public.listp1 VALUES ('One', 1);
> 
> That settles the debate on the other thread...

+1 to fixing this, although +0.5 to back-patching.

The reason no one has complained so far of being bitten by this may be
that, as each of one us has said at least once on the other thread, users
are not very likely to create partitions with different column orders to
begin with.  Maybe, that isn't a reason to leave it as is though.

Thanks,
Amit

On Wed, 24 Apr 2019 at 14:53, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2019/04/24 10:19, David Rowley wrote:
> > ERROR:  invalid input syntax for type integer: "One"
> > LINE 1: INSERT INTO public.listp1 VALUES ('One', 1);
> >
> > That settles the debate on the other thread...
>
> +1 to fixing this, although +0.5 to back-patching.
>
> The reason no one has complained so far of being bitten by this may be
> that, as each of one us has said at least once on the other thread, users
> are not very likely to create partitions with different column orders to
> begin with.  Maybe, that isn't a reason to leave it as is though.

Well, you could probably class most of the bugs that make their way
through feature freeze, alpha and beta as unlikely.  I don't think
that gives us an excuse to leave them as bugs.  If someone reported it
we'd most likely go and fix it then anyway, so I really don't see the
point in waiting until then.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

So, while testing this I noticed that pg_restore fails with deadlocks if
you do a parallel restore if the --load-via-partition-root switch was
given to pg_dump.  Is that a known bug?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Apr 25, 2019 at 4:01 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> So, while testing this I noticed that pg_restore fails with deadlocks if
> you do a parallel restore if the --load-via-partition-root switch was
> given to pg_dump.  Is that a known bug?

Was investigating --load-via-partition-root with a coworker and came
across the following note in the documentation:

https://www.postgresql.org/docs/11/app-pgdump.html

"It is best not to use parallelism when restoring from an archive made
with this option, because pg_restore will not know exactly which
partition(s) a given archive data item will load data into. This could
result in inefficiency due to lock conflicts between parallel jobs, or
perhaps even reload failures due to foreign key constraints being set
up before all the relevant data is loaded."

Apparently, this note was added as a result of the following discussion:

https://www.postgresql.org/message-id/flat/13624.1535486019%40sss.pgh.pa.us

So, while the documentation doesn't explicitly list deadlocks as
possible risk, Tom hinted in the first email that it's possible.

I set out to reproduce one and was able to, although I'm not sure if
it's the same deadlock as seen by Alvaro.  Steps I used to reproduce:

# in the source database

create table foo (a int primary key);
insert into foo select generate_series(1, 1000000);
create table ht (a int) partition by hash (a);
select 'create table ht' || i || ' partition of ht for values with
(modulus 100, remainder ' || i -1 || ');' from generate_series(1, 100)
i;
\gexec
insert into ht select generate_series(1, 1000000);
alter table ht add foreign key (a) references foo (a);

# in shell
pg_dump --load-via-partition-root -Fd -f /tmp/dump
createdb targetdb
pg_restore -d targetdb -j 2 /tmp/dump

The last step reports deadlocks; in the server log:

ERROR:  deadlock detected
DETAIL:  Process 14213 waits for RowExclusiveLock on relation 17447 of
database 17443; blocked by process 14212.
        Process 14212 waits for ShareRowExclusiveLock on relation
17507 of database 17443; blocked by process 14213.
        Process 14213: COPY public.ht (a) FROM stdin;

        Process 14212: ALTER TABLE public.ht
            ADD CONSTRAINT ht_a_fkey FOREIGN KEY (a) REFERENCES public.foo(a);

Here, the process adding the foreign key has got the lock on the
parent and trying to lock a partition to add the foreign key to it.
The process doing COPY (via root) has apperently locked the partition
and waiting for the lock on the parent to do actual copying.  Looking
into why the latter had got a lock on the partition at all if it
hasn't started the copying yet, I noticed that it was locked when
TRUNCATE was executed on it earlier in the same transaction as part of
some WAL-related optimization, which is something that only happens in
the parallel restore mode.  I was under the impression that the TABLE
DATA archive item (its TocEntry) would have no trace of the partition
if it was dumped with --load-via-partition-root, but that's not the
case.  --load-via-partition-root only dictates that the command that
will be dumped for the item will use the root parent as COPY target,
even though the TocEntry itself is owned by the partition.

Maybe, a way to prevent the deadlock would be for the process that
will do copy-into-given-partition-via-root-parent to do a `LOCK TABLE
root_parent` before `TRUNCATE the_partition`, but we'll need to get
hold of the root parent from the TocEntry somehow.  Turns out it's
only present in the TocEntry.copyStmt, from where it will have to
parsed out.  Maybe that's the only thing we could do without breaking
the archive format though.

Thoughts?

By the way, I couldn't think of ways to reproduce any of the hazards
mentioned in the documentations of using parallel mode to restore an
archive written with pg_dump --load-via-root-parent, but maybe I just
haven't tried hard enough.

Thanks,
Amit