Re: identify_locking_dependencies is broken for schema-only dumps

This can lead to deadlocks during parallel restore. Test case:

create table bar (a int primary key, b int);
create table baz (z int, a int references bar);
create view foo as select a, b, sum(1) from bar group by a union all
select z, a, 0 from baz;

I dumped this with: pg_dump -Fc -s -f test.dmp
Then: while (dropdb rhaas; createdb; pg_restore -O -d rhaas -j3
test.dmp); do true; done

This quickly fails for me with:

pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 2155; 2606 47822 FK
CONSTRAINT baz_a_fkey rhaas
pg_restore: [archiver (db)] could not execute query: ERROR: deadlock detected
DETAIL: Process 81791 waits for AccessExclusiveLock on relation 47862
of database 47861; blocked by process 81789.
Process 81789 waits for AccessShareLock on relation 47865 of database
47861; blocked by process 81791.
HINT: See server log for query details.
Command was: ALTER TABLE ONLY baz
ADD CONSTRAINT baz_a_fkey FOREIGN KEY (a) REFERENCES bar(a);
WARNING: errors ignored on restore: 2

The attached patch seems to fix it for me.

Comments?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Sep 19, 2014 at 11:23 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> This can lead to deadlocks during parallel restore. Test case:
>
> create table bar (a int primary key, b int);
> create table baz (z int, a int references bar);
> create view foo as select a, b, sum(1) from bar group by a union all
> select z, a, 0 from baz;
>
> I dumped this with: pg_dump -Fc -s -f test.dmp
> Then: while (dropdb rhaas; createdb; pg_restore -O -d rhaas -j3
> test.dmp); do true; done
>
> This quickly fails for me with:
>
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 2155; 2606 47822 FK
> CONSTRAINT baz_a_fkey rhaas
> pg_restore: [archiver (db)] could not execute query: ERROR: deadlock detected
> DETAIL: Process 81791 waits for AccessExclusiveLock on relation 47862
> of database 47861; blocked by process 81789.
> Process 81789 waits for AccessShareLock on relation 47865 of database
> 47861; blocked by process 81791.
> HINT: See server log for query details.
> Command was: ALTER TABLE ONLY baz
> ADD CONSTRAINT baz_a_fkey FOREIGN KEY (a) REFERENCES bar(a);
> WARNING: errors ignored on restore: 2
>
> The attached patch seems to fix it for me.
>
> Comments?

If there are no comments on this soon-ish, I'm going to push and
back-patched the patch I attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> If there are no comments on this soon-ish, I'm going to push and
> back-patched the patch I attached.

Sorry for not paying attention sooner. After studying it for awhile,
I think the change is probably all right but your proposed comment is
entirely inadequate. There are extremely specific reasons why this
works, and you removed an existing comment about that and replaced it
with nothing but a wishy-washy "maybe".

regards, tom lane

On Wed, Sep 24, 2014 at 12:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> If there are no comments on this soon-ish, I'm going to push and
>> back-patched the patch I attached.
>
> Sorry for not paying attention sooner. After studying it for awhile,
> I think the change is probably all right but your proposed comment is
> entirely inadequate. There are extremely specific reasons why this
> works, and you removed an existing comment about that and replaced it
> with nothing but a wishy-washy "maybe".

Well, I could write something like this:

* We assume the item requires exclusive lock on each TABLE or TABLE DATA
* item listed among its dependencies. (This was originally a dependency on
* the TABLE, but fix_dependencies may have repointed it to the data item.
* In a schema-only dump, however, this will not have been done.)

If you don't like that version, can you suggest something you would like better?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Sep 24, 2014 at 12:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Sorry for not paying attention sooner. After studying it for awhile,
>> I think the change is probably all right but your proposed comment is
>> entirely inadequate.

> If you don't like that version, can you suggest something you would like better?

Perhaps like this:

* We assume the entry requires exclusive lock on each TABLE or TABLE DATA
* item listed among its dependencies. Originally all of these would have
* been TABLE items, but repoint_table_dependencies would have repointed
* them to the TABLE DATA items if those are present (which they might not
* be, eg in a schema-only dump). Note that all of the entries we are
* processing here are POST_DATA; otherwise there might be a significant
* difference between a dependency on a table and a dependency on its
* data, so that closer analysis would be needed here.

regards, tom lane

On Wed, Sep 24, 2014 at 4:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Sep 24, 2014 at 12:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Sorry for not paying attention sooner. After studying it for awhile,
>>> I think the change is probably all right but your proposed comment is
>>> entirely inadequate.
>
>> If you don't like that version, can you suggest something you would like better?
>
> Perhaps like this:
>
> * We assume the entry requires exclusive lock on each TABLE or TABLE DATA
> * item listed among its dependencies. Originally all of these would have
> * been TABLE items, but repoint_table_dependencies would have repointed
> * them to the TABLE DATA items if those are present (which they might not
> * be, eg in a schema-only dump). Note that all of the entries we are
> * processing here are POST_DATA; otherwise there might be a significant
> * difference between a dependency on a table and a dependency on its
> * data, so that closer analysis would be needed here.

Works for me. I'll push with that text unless you'd like to take care of it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company