Re: pg_restore dependencies

We still have a little work to do on dependencies in parallel
pg_restore. The current test compares the candidate's locking
dependencies with those of the running jobs, and allows the candidate is
there isn't a match. That's not a broad enough test. The candidate will
block if there's a currently running CREATE INDEX command on the table,
for example, even though that doesn't require an exclusive lock. That's
not catastrophic, in that the restore doesn't fail, but it's fairly bad
because it reduces the achievable parallelism. Josh Berkus observed this
during testing on a very large restore.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> We still have a little work to do on dependencies in parallel
> pg_restore. The current test compares the candidate's locking
> dependencies with those of the running jobs, and allows the candidate is
> there isn't a match. That's not a broad enough test. The candidate will
> block if there's a currently running CREATE INDEX command on the table,
> for example, even though that doesn't require an exclusive lock. That's
> not catastrophic, in that the restore doesn't fail, but it's fairly bad
> because it reduces the achievable parallelism. Josh Berkus observed this
> during testing on a very large restore.

Well, we certainly want to be able to run CREATE INDEXes in parallel,
so this would appear to require hard-wiring some conception of shared
versus exclusive lock into pg_restore. I think it might be a bit late
to consider that for 8.4.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> We still have a little work to do on dependencies in parallel
>> pg_restore. The current test compares the candidate's locking
>> dependencies with those of the running jobs, and allows the candidate is
>> there isn't a match. That's not a broad enough test. The candidate will
>> block if there's a currently running CREATE INDEX command on the table,
>> for example, even though that doesn't require an exclusive lock. That's
>> not catastrophic, in that the restore doesn't fail, but it's fairly bad
>> because it reduces the achievable parallelism. Josh Berkus observed this
>> during testing on a very large restore.
>>
>
> Well, we certainly want to be able to run CREATE INDEXes in parallel,
> so this would appear to require hard-wiring some conception of shared
> versus exclusive lock into pg_restore. I think it might be a bit late
> to consider that for 8.4.
>

I'm pretty sure I had the logic for this correct stuff originally, so
I'm going to go back and check that.

With luck it won't take long. It shouldn't hold up beta - it's just a
bug we need to fix, and with any luck I'll actually have it fixed in the
next few days.

cheers

andrew

Tom, Andrew,

>> Well, we certainly want to be able to run CREATE INDEXes in parallel,
>> so this would appear to require hard-wiring some conception of shared
>> versus exclusive lock into pg_restore. I think it might be a bit late
>> to consider that for 8.4.
>
>
> I'm pretty sure I had the logic for this correct stuff originally, so
> I'm going to go back and check that.

FWIW, I've tested 3 moderately complex databases with this, and the
locking issue happens on every one. As a result, getting more than 3
cores of scalability on any fairly complex DB isn't possible without
fixing this.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Josh Berkus wrote:
> Tom, Andrew,
>
>>> Well, we certainly want to be able to run CREATE INDEXes in parallel,
>>> so this would appear to require hard-wiring some conception of shared
>>> versus exclusive lock into pg_restore. I think it might be a bit late
>>> to consider that for 8.4.
>>
>>
>> I'm pretty sure I had the logic for this correct stuff originally, so
>> I'm going to go back and check that.
>
> FWIW, I've tested 3 moderately complex databases with this, and the
> locking issue happens on every one. As a result, getting more than 3
> cores of scalability on any fairly complex DB isn't possible without
> fixing this.

Yeah. I think the correct logic is roughly this: When considering if a
candidate item has a locking conflict with a running item, then if
*either* of them has a locking dependency that coincides with *any*
dependency of the other item, then the candidate is rejected. The
principle is that we don't give any item a chance to block on a lock.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Yeah. I think the correct logic is roughly this: When considering if a
> candidate item has a locking conflict with a running item, then if
> *either* of them has a locking dependency that coincides with *any*
> dependency of the other item, then the candidate is rejected. The
> principle is that we don't give any item a chance to block on a lock.

Doesn't that eliminate any chance of running two CREATE INDEXes
concurrently on the same table?

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Yeah. I think the correct logic is roughly this: When considering if a
>> candidate item has a locking conflict with a running item, then if
>> *either* of them has a locking dependency that coincides with *any*
>> dependency of the other item, then the candidate is rejected. The
>> principle is that we don't give any item a chance to block on a lock.
>>
>
> Doesn't that eliminate any chance of running two CREATE INDEXes
> concurrently on the same table?
>
>
>

No, since neither of them will have any locking dependencies, which are
only for items that take an exclusive lock on the table(s), such as FK
constraints.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> Doesn't that eliminate any chance of running two CREATE INDEXes
>> concurrently on the same table?

> No, since neither of them will have any locking dependencies, which are
> only for items that take an exclusive lock on the table(s), such as FK
> constraints.

In that case a CREATE INDEX would also fail to be seen as conflicting
with an ALTER ADD FOREIGN KEY, which I thought was the nub of Josh's
complaint.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Tom Lane wrote:
>>
>>> Doesn't that eliminate any chance of running two CREATE INDEXes
>>> concurrently on the same table?
>>>
>
>
>> No, since neither of them will have any locking dependencies, which are
>> only for items that take an exclusive lock on the table(s), such as FK
>> constraints.
>>
>
> In that case a CREATE INDEX would also fail to be seen as conflicting
> with an ALTER ADD FOREIGN KEY, which I thought was the nub of Josh's
> complaint.
>
>
>

No it won't.

What you're missing is that we need to compare the lockdeps of each item
(i.e. both the candidate item and the running item) with all the deps
(not just the lockdeps) of the other item. If neither item has any
lockdeps there will be no conflict. This will allow concurrent index
creation, since neither item will have any lockdeps. But it will prevent
us selecting a create index that conflicts with a running FK creation or
vice versa.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> What you're missing is that we need to compare the lockdeps of each item
> (i.e. both the candidate item and the running item) with all the deps
> (not just the lockdeps) of the other item. If neither item has any
> lockdeps there will be no conflict. This will allow concurrent index
> creation, since neither item will have any lockdeps. But it will prevent
> us selecting a create index that conflicts with a running FK creation or
> vice versa.

Oh, I see, you're using the deps as a proxy for the shared locks the
operation will acquire. Yeah, that might work. Seems like it's nearly
a one-liner fix, too.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> What you're missing is that we need to compare the lockdeps of each item
>> (i.e. both the candidate item and the running item) with all the deps
>> (not just the lockdeps) of the other item. If neither item has any
>> lockdeps there will be no conflict. This will allow concurrent index
>> creation, since neither item will have any lockdeps. But it will prevent
>> us selecting a create index that conflicts with a running FK creation or
>> vice versa.
>>
>
> Oh, I see, you're using the deps as a proxy for the shared locks the
> operation will acquire. Yeah, that might work. Seems like it's nearly
> a one-liner fix, too.
>
>
>

Well, what I have in mind is a bit bigger, but not large. See attached
patch.

cheers

andrew

Adnrew,

> Well, what I have in mind is a bit bigger, but not large. See attached
> patch.

I'll test it this weekend.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> ... Seems like it's nearly a one-liner fix, too.

> Well, what I have in mind is a bit bigger, but not large. See attached
> patch.

Hmm, you do need two instances of the loop, don't you? Might be
better to refactor along the lines of

if (has_lock_conflicts(te, running_te) ||
has_lock_conflicts(running_te, te))
// has a conflict

...

// true if te1 requires exclusive lock on any dependency of te2
static bool
has_lock_conflicts(te1, te2)
{
for (j = 0; j < te1->nLockDeps; j++)
{
for (k = 0; k < te2->nDeps; k++)
{
if (te1->lockDeps[j] == te2->dependencies[k])
return true;
}
}
return false;
}

regards, tom lane

Andrew,

Do we have a final version of this patch yet? I have to do an upgrade
test run today, so it would be a good time to test it.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

Josh Berkus wrote:
> Andrew,
>
> Do we have a final version of this patch yet? I have to do an upgrade
> test run today, so it would be a good time to test it.
>

I'm working on an updated patch right now. But it is only cosmetically
different from the one I posted before. Functionally it's identical.

cheers

andrew

Andrew, Tom,

I just did a test run using Andrew's patch with a database with over 400
objects. I didn't see any locks waiting during the entire run. So the
patch logic appears to work.

Note that it also shows up that some CONSTRAINT declarations really
shouldn't require an exclusive lock. I'd estimate that if we could step
a lot of constraints down to sharelock (yes, I know, there's some issues
with that), it would shorten a parallel restore by a large chunk (like
25% in the 4-core case I'm testing).

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com