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
Attachments:
depfix.patchtext/x-patch; charset=iso-8859-1; name=depfix.patchDownload
*** 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