Re: concurrent index builds unneeded lock?

We just ran into a case where we were performing two concurrent index
builds on two different tables in two different schemas in the same
database (no relational constraints between them).

One of the index builds locked on the other.

The first index build started...
The second index build started...
The first one locked on the second one....
The second one finished...
The first one was allows to continue and finish.

quux=# select * from pg_locks where pid IN (25264, 20108);
locktype | database | relation | page | tuple | virtualxid |
transactionid | classid | objid | objsubid | virtualtransaction |
pid | mode | granted
------------+----------+----------+------+-------+------------
+---------------+---------+-------+----------+--------------------
+-------+--------------------------+---------
relation | 16385 | 25852 | | |
| | | | | 9/3041 |
20108 | RowExclusiveLock | t
relation | 16385 | 25861 | | |
| | | | | 1/15735 |
25264 | RowExclusiveLock | t
relation | 16385 | 16421 | | |
| | | | | 9/3041 |
20108 | ShareUpdateExclusiveLock | t
virtualxid | | | | | 9/3041
| | | | | 9/3041 |
20108 | ExclusiveLock | t
virtualxid | | | | | 1/15735
| | | | | 1/15735 |
25264 | ExclusiveLock | t
virtualxid | | | | | 9/3041
| | | | | 1/15735 |
25264 | ShareLock | f
relation | 16385 | 16528 | | |
| | | | | 1/15735 |
25264 | ShareUpdateExclusiveLock | t
(7 rows)

Reading the comments in the concurrent index build code, it seems that
in prep for phase 3 of the index build it looks for any open txns that
could feasibly see deleted tuples prior to the snap.

I would think it would be txns that would be reading that table, but
I'm thinking it is a bit to aggressive. Am I reading the code wrong
there? I'm thinking it should be more selective about vxids it
chooses to block on. I'd expect it to block on vxids touching the
same table only.

Thoughts?

--
Theo Schlossnagle
http://omniti.com/is/theo-schlossnagle
p: +1.443.325.1357 x201 f: +1.410.872.4911

Theo Schlossnagle <jesus(at)omniti(dot)com> writes:
> I would think it would be txns that would be reading that table, but
> I'm thinking it is a bit to aggressive. Am I reading the code wrong
> there? I'm thinking it should be more selective about vxids it
> chooses to block on. I'd expect it to block on vxids touching the
> same table only.

There is no way to know whether a currently active vxid will try to look
at the other table later. We can not just ignore this case...

regards, tom lane

On Jul 11, 2009, at 12:12 AM, Tom Lane wrote:

> Theo Schlossnagle <jesus(at)omniti(dot)com> writes:
>> I would think it would be txns that would be reading that table, but
>> I'm thinking it is a bit to aggressive. Am I reading the code wrong
>> there? I'm thinking it should be more selective about vxids it
>> chooses to block on. I'd expect it to block on vxids touching the
>> same table only.
>
> There is no way to know whether a currently active vxid will try to
> look
> at the other table later. We can not just ignore this case...
>
> regards, tom lane

Can't you know "that" if the other active query in question is a
concurrent index build?

Concurrent index builds by their current implementation cannot exist
within another transaction, so you know everythere there is to know
about that transaction by looking at it (no risk of prior or future
work).

While very much unlike a vacuum (a special exclusion in concurrent
index builds), they still seem to constitute a "special case" for
exclusion.

Happy to be wrong here, I really haven't completely digested the code.

--
Theo Schlossnagle
http://omniti.com/is/theo-schlossnagle
p: +1.443.325.1357 x201 f: +1.410.872.4911

On Sat, Jul 11, 2009 at 6:17 AM, Theo Schlossnagle<jesus(at)omniti(dot)com> wrote:
>
>
> On Jul 11, 2009, at 12:12 AM, Tom Lane wrote:
>
>> Theo Schlossnagle <jesus(at)omniti(dot)com> writes:
>>>
>>> I would think it would be txns that would be reading that table, but
>>> I'm thinking it is a bit to aggressive.  Am I reading the code wrong
>>> there?  I'm thinking it should be more selective about vxids it
>>> chooses to block on.  I'd expect it to block on vxids touching the
>>> same table only.
>>
>> There is no way to know whether a currently active vxid will try to look
>> at the other table later.  We can not just ignore this case...
>>
>>                        regards, tom lane
>
>
> Can't you know "that" if the other active query in question is a concurrent
> index build?

I think so.

Hm. Actually maybe not. What if the index is an expression index and
the expression includes a function which does an SQL operation? I'm
not sure how realistic that is since to be a danger that SQL operation
would have to be an insert, update, or delete which is not just
bending the rules.

The real problem is that we only added the ability to distinguish
vacuums relatively recently and adding more special cases of
transactions which can be ignored for one purpose or another seems
like a lot of corner cases to worry about.

I wonder whether an earlier more general proposal could have some
leverage here though: some way to indicate that the transaction has
taken all the locks it plans to take already. This was originally
proposed as a way for vacuum to know it can ignore the snapshots of a
transaction since it isn't going to access the table being vacuumed.

In this case the concurrent index build could mark itself as having
taken all the locks it plans to take and other concurrent index builds
could ignore it. They could also ignore any transactions which have
that flag set through any other mechanisms we might add such as a
manual SQL command.

--
greg
http://mit.edu/~gsstark/resume.pdf

On Jul 11, 2009, at 6:50 AM, Greg Stark wrote:

> On Sat, Jul 11, 2009 at 6:17 AM, Theo Schlossnagle<jesus(at)omniti(dot)com>
> wrote:
>>
>>
>> On Jul 11, 2009, at 12:12 AM, Tom Lane wrote:
>>
>>> Theo Schlossnagle <jesus(at)omniti(dot)com> writes:
>>>>
>>>> I would think it would be txns that would be reading that table,
>>>> but
>>>> I'm thinking it is a bit to aggressive. Am I reading the code
>>>> wrong
>>>> there? I'm thinking it should be more selective about vxids it
>>>> chooses to block on. I'd expect it to block on vxids touching the
>>>> same table only.
>>>
>>> There is no way to know whether a currently active vxid will try
>>> to look
>>> at the other table later. We can not just ignore this case...
>>>
>>> regards, tom lane
>>
>>
>> Can't you know "that" if the other active query in question is a
>> concurrent
>> index build?
>
> I think so.
>
> Hm. Actually maybe not. What if the index is an expression index and
> the expression includes a function which does an SQL operation? I'm
> not sure how realistic that is since to be a danger that SQL operation
> would have to be an insert, update, or delete which is not just
> bending the rules.
>
> The real problem is that we only added the ability to distinguish
> vacuums relatively recently and adding more special cases of
> transactions which can be ignored for one purpose or another seems
> like a lot of corner cases to worry about.
>
> I wonder whether an earlier more general proposal could have some
> leverage here though: some way to indicate that the transaction has
> taken all the locks it plans to take already. This was originally
> proposed as a way for vacuum to know it can ignore the snapshots of a
> transaction since it isn't going to access the table being vacuumed.
>
> In this case the concurrent index build could mark itself as having
> taken all the locks it plans to take and other concurrent index builds
> could ignore it. They could also ignore any transactions which have
> that flag set through any other mechanisms we might add such as a
> manual SQL command.

While I see that feature being extremely hard for users to leverage
via SQL, it seems like a very simplistic and useful internal control.

When acquire locks, we simply check if we've have the future-locking
bit flipped and abort the transaction if it is. It seems really
safe. I'd imagine the various places we try to use that we'll be
reminded of the incidental locks we'll be needing to grab. But, after
looking at the concurrent index code, it seems that it would solve my
issue at least. Also much more elegant that adding more exceptions.

--
Theo Schlossnagle
http://omniti.com/is/theo-schlossnagle
p: +1.443.325.1357 x201 f: +1.410.872.4911

Greg Stark <gsstark(at)mit(dot)edu> writes:
> I wonder whether an earlier more general proposal could have some
> leverage here though: some way to indicate that the transaction has
> taken all the locks it plans to take already. This was originally
> proposed as a way for vacuum to know it can ignore the snapshots of a
> transaction since it isn't going to access the table being vacuumed.

Again, this doesn't work for any interesting cases. You can't for
example assume that a user-defined datatype won't choose to look into
tables that hadn't been accessed as of the start of the index build.
(This isn't a hypothetical example --- I believe PostGIS does some
such things already, because it keeps spatial reference definitions
in a central catalog table.)

regards, tom lane

On 7/11/09 3:50 AM, Greg Stark wrote:
> Hm. Actually maybe not. What if the index is an expression index and
> the expression includes a function which does an SQL operation? I'm
> not sure how realistic that is since to be a danger that SQL operation
> would have to be an insert, update, or delete which is not just
> bending the rules.

It's not realistic at all. People are only supposed to use IMMUTABLE
functions for experession indexes; if they declare a volatile function
as immutable, then it's their own lookout if they corrupt their data.

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

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 7/11/09 3:50 AM, Greg Stark wrote:
>> Hm. Actually maybe not. What if the index is an expression index and
>> the expression includes a function which does an SQL operation? I'm
>> not sure how realistic that is since to be a danger that SQL operation
>> would have to be an insert, update, or delete which is not just
>> bending the rules.

> It's not realistic at all. People are only supposed to use IMMUTABLE
> functions for experession indexes; if they declare a volatile function
> as immutable, then it's their own lookout if they corrupt their data.

The requirement wasn't just on not changing SQL data though. To make
use of this you'd also have to forbid indexed functions from *reading*
other tables. Which is something we discourage because of the risk that
the results aren't really immutable, but we don't forbid it; and there
are obvious use-cases.

regards, tom lane

On Sun, Jul 12, 2009 at 4:17 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The requirement wasn't just on not changing SQL data though.  To make
> use of this you'd also have to forbid indexed functions from *reading*
> other tables.  Which is something we discourage because of the risk that
> the results aren't really immutable, but we don't forbid it; and there
> are obvious use-cases.

Well it's my fault but the discussion kind of mutated in the middle.
For the original use case I think only changing SQL data would
actually be a threat. The concurrent index build only has to wait out
any transactions which might update the table without updating the
index. Which, even if there are volatile functions in the index
expression, index where clause, or index operators, they aren't really
likely to do.

The other thing is that the worst case if they do is you end up with a
corrupted index which is missing entries or has duplicate entries.
That's the same risk you always have if you have volatile functions
mismarked and used in your index definition.

For the mutated discussion where I was trying to find a mechanism that
would be more generally useful that's not the case. Vacuum needs to
know whether you ever plan to *read* from a table in the future. But
that's not what concurrent index builds need to know.

So I think we're back to looking at a special case for concurrent
index builds to not wait on other concurrent index builds.
--
greg
http://mit.edu/~gsstark/resume.pdf

Greg Stark <gsstark(at)mit(dot)edu> writes:
> So I think we're back to looking at a special case for concurrent
> index builds to not wait on other concurrent index builds.

I'm kind of wondering how big the use case for that really is.
AFAICT the point of a concurrent build is to (re)build an index
without incurring too much performance penalty for foreground
query processing. So how often are you really going to want
to fire off several of them in parallel? If you can afford to
saturate your machine with indexing work, you could use plain
index builds.

regards, tom lane

Actually ... why do we have to have that third wait step at all?
Doesn't the indcheckxmin mechanism render it unnecessary, or couldn't
we adjust the comparison xmin to make it unnecessary?

regards, tom lane

On Sun, Jul 12, 2009 at 4:42 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I'm kind of wondering how big the use case for that really is.
> AFAICT the point of a concurrent build is to (re)build an index
> without incurring too much performance penalty for foreground
> query processing.  So how often are you really going to want
> to fire off several of them in parallel?  If you can afford to
> saturate your machine with indexing work, you could use plain
> index builds.

I don't really see those as comparable cases. Firing off multiple
concurrent index builds only requires lots of available I/O
throughput; using plain index builds requires a maintenance window
where any updates to the table is shut down.

Being able to run multiple concurrent index builds just means being
able to roll out a schema change more quickly. It doesn't let you do
anything that was impossible before.

Another thing that's annoyed me about our current support for
concurrent index builds is that you can't run multiple concurrent
builds on the same table. Since they all take the strangely named
ShareUpdateExclusiveLock you can only run one at a time. Fixing that
would require introducing a new, uh, ShareUpdateSharedLock(?) which
conflicts with the vacuum lock but not itself. It didn't seem worth
introducing a new lock type at the time but with syncscanning and the
evidence people are actually doing this I'm starting to wonder.

--
greg
http://mit.edu/~gsstark/resume.pdf