Обсуждение: 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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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
*** pg_backup_archiver.c 2009-04-10 00:09:57.000000000 -0400
--- pg_backup_archiver-fix.c 2009-04-10 19:22:07.000000000 -0400
***************
*** 3423,3433 ****
if (slots[i].args == NULL)
continue;
running_te = slots[i].args->te;
for (j = 0; j < te->nLockDeps && !conflicts; j++)
{
! for (k = 0; k < running_te->nLockDeps; k++)
{
! if (te->lockDeps[j] == running_te->lockDeps[k])
{
conflicts = true;
break;
--- 3423,3450 ----
if (slots[i].args == NULL)
continue;
running_te = slots[i].args->te;
+ /* does the candidate item require an exclusive lock that
+ * would block on or conflict with the running item?
+ */
for (j = 0; j < te->nLockDeps && !conflicts; j++)
{
! for (k = 0; k < running_te->nDeps; k++)
{
! if (te->lockDeps[j] == running_te->dependencies[k])
! {
! conflicts = true;
! break;
! }
! }
! }
! /* or does the running item hold an exclusive lock that
! * would block or conflict with the candidate item?
! */
! for (j = 0; j < running_te->nLockDeps && !conflicts; j++)
! {
! for (k = 0; k < te->nDeps; k++)
! {
! if (running_te->lockDeps[j] == te->dependencies[k])
{
conflicts = true;
break;
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@dunslane.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