Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-27 Thread Tom Lane
I wrote: I'm toying with the idea of adding a lock manager call defined as give me a list of XIDs that currently hold locks conflicting with lockmode X on object Y --- but not including XIDs merely waiting for such a lock. Then we could get the list of XIDs currently blocking ExclusiveLock,

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-26 Thread Gregory Stark
Tom Lane [EMAIL PROTECTED] writes: Barring objections, I'm off to program this. A few concerns a) The use of ShareUpdateExclusiveLock is supposed to lock out concurrent vacuums. I just tried it and vacuum seemed to be unaffected. I'm going to retry it with a clean cvs checkout to be sure

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-26 Thread David Fetter
On Fri, Aug 25, 2006 at 08:02:21PM -0500, Jim C. Nasby wrote: On Fri, Aug 25, 2006 at 06:57:58PM +0100, Gregory Stark wrote: I'll use this opportunity to plug that feature again. I think most people should use autocommit off with on_error_rollack on for most of their daily use. Being able

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-26 Thread Tom Lane
Gregory Stark [EMAIL PROTECTED] writes: A few concerns a) The use of ShareUpdateExclusiveLock is supposed to lock out concurrent vacuums. I just tried it and vacuum seemed to be unaffected. Can't reproduce such a problem here. Do we still need to block concurrent vacuums if we're

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-26 Thread Tom Lane
I wrote: Gregory Stark [EMAIL PROTECTED] writes: b) You introduced a LockRelationIdForSession() call (I even didn't realize we had this capability when I wrote the patch). Does this introduce the possibility of a deadlock though? Indeed, I had noticed this while testing your point (a). I

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread stark
Tom Lane [EMAIL PROTECTED] writes: Greg Stark [EMAIL PROTECTED] writes: Hmmm. Or is that true. The problem may be somewhat easier since at least you can be sure every tuple in the heap is in the index. So if you see a DELETE_IN_PROGRESS either it *was* a constraint violation prior to the

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Gregory Stark [EMAIL PROTECTED] writes: Because of the way the AM API works changing how the initial heap scan works is a bit of a pain. It would require either having some global state or passing the concurrent flag through the AM methods or alternatively having a whole new AM method. Yeah,

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Gregory Stark
Do we want something like this? I just made this error myself so unless I'm special (pauses for jokes) I imagine others would be prone to it as well. I would normally be pretty leery of code like this but it seems unlikely anyone would actually want an index named concurrently and the

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Andrew Dunstan
Gregory Stark wrote: Do we want something like this? I just made this error myself so unless I'm special (pauses for jokes) I imagine others would be prone to it as well. I would normally be pretty leery of code like this but it seems unlikely anyone would actually want an index named

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] writes: I see we have: CREATE index_opt_unique INDEX CONCURRENTLY index_name ... which explains how this error occurs. Maybe to you, but I'm still caffeine-deprived and don't exactly see what it was that Greg mistyped. AFAICS he'd have to type CONCURRENTLY

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Andrew Dunstan
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: I see we have: CREATE index_opt_unique INDEX CONCURRENTLY index_name ... which explains how this error occurs. Maybe to you, but I'm still caffeine-deprived and don't exactly see what it was that Greg mistyped. AFAICS he'd

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Stefan Kaltenbrunner
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: I see we have: CREATE index_opt_unique INDEX CONCURRENTLY index_name ... which explains how this error occurs. Maybe to you, but I'm still caffeine-deprived and don't exactly see what it was that Greg mistyped. AFAICS he'd have to

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Bruce Momjian
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: I see we have: CREATE index_opt_unique INDEX CONCURRENTLY index_name ... which explains how this error occurs. Maybe to you, but I'm still caffeine-deprived and don't exactly see what it was that Greg mistyped. AFAICS he'd have

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes: The original thinking was to use CONCURRENT, and CREATE CONCURRENT INDEX sounded like a different type of index, not a different way to build the index. I don't think CONCURRENTLY has that problem, so CREATE CONCURRENTLY INDEX sounds good. To read in

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Gregory Stark
Bruce Momjian [EMAIL PROTECTED] writes: The original thinking was to use CONCURRENT, and CREATE CONCURRENT INDEX sounded like a different type of index, not a different way to build the index. I don't think CONCURRENTLY has that problem, so CREATE CONCURRENTLY INDEX sounds good. To read in

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Bruce Momjian
Gregory Stark wrote: Bruce Momjian [EMAIL PROTECTED] writes: The original thinking was to use CONCURRENT, and CREATE CONCURRENT INDEX sounded like a different type of index, not a different way to build the index. I don't think CONCURRENTLY has that problem, so CREATE CONCURRENTLY

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes: What bothers me about what we have now is that we have optional keywords before and after INDEX, rather than only between CREATE and INDEX. Yeah, putting them both into that space seems consistent to me, and it will fix the problem of making an omitted

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Alvaro Herrera
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: What bothers me about what we have now is that we have optional keywords before and after INDEX, rather than only between CREATE and INDEX. Yeah, putting them both into that space seems consistent to me, and it will fix the problem

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Alvaro Herrera [EMAIL PROTECTED] writes: The problem is that what the qualifier is doing is modifying the operation itself, not the properties of the index to be created, like UNIQUE, which modifies the index. Right, which was the same point Bruce made earlier. And really you can't respect

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Andrew Dunstan
Gregory Stark wrote: Bruce Momjian [EMAIL PROTECTED] writes: The original thinking was to use CONCURRENT, and CREATE CONCURRENT INDEX sounded like a different type of index, not a different way to build the index. I don't think CONCURRENTLY has that problem, so CREATE CONCURRENTLY INDEX

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Zeugswetter Andreas DCP SD
What bothers me about what we have now is that we have optional keywords before and after INDEX, rather than only between CREATE and INDEX. Yeah, putting them both into that space seems consistent to me, and it will fix the problem of making an omitted index name look like a valid

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Alvaro Herrera
Zeugswetter Andreas DCP SD wrote: What bothers me about what we have now is that we have optional keywords before and after INDEX, rather than only between CREATE and INDEX. Yeah, putting them both into that space seems consistent to me, and it will fix the problem of making an

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Zeugswetter Andreas DCP SD [EMAIL PROTECTED] writes: precedent syntax (Oracle, Informix) uses the keyword ONLINE at the end: CREATE INDEX blabla_x0 ON blabla (a,b) ONLINE; We rejected that one already ... regards, tom lane ---(end of

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Zeugswetter Andreas DCP SD
precedent syntax (Oracle, Informix) uses the keyword ONLINE at the end: CREATE INDEX blabla_x0 ON blabla (a,b) ONLINE; That was what the patch originally used, but it was changed because it made difficult for psql to auto-complete that. That is imho not enough of a reason to divert.

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Gregory Stark
Alvaro Herrera [EMAIL PROTECTED] writes: That was what the patch originally used, but it was changed because it made difficult for psql to auto-complete that. Psql has to be able to parse it not for auto-completion but because it needs to know that it's not a transactional command. The regular

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Tom Lane
Zeugswetter Andreas DCP SD [EMAIL PROTECTED] writes: That was what the patch originally used, but it was changed because it made difficult for psql to auto-complete that. That is imho not enough of a reason to divert. My recollection is that the principal argument against ONLINE was that it

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Jim C. Nasby
On Fri, Aug 25, 2006 at 06:57:58PM +0100, Gregory Stark wrote: I'll use this opportunity to plug that feature again. I think most people should use autocommit off with on_error_rollack on for most of their daily use. Being able to double check the results of my ad-hoc updates before committing

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-25 Thread Jim C. Nasby
On Fri, Aug 25, 2006 at 11:25:43AM -0400, Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: The problem is that what the qualifier is doing is modifying the operation itself, not the properties of the index to be created, like UNIQUE, which modifies the index. Right, which was the

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-24 Thread Gregory Stark
Tom Lane [EMAIL PROTECTED] writes: I wrote: The problem case is that we take a tuple and try to insert it into the index. Meanwhile someone else updates the tuple, and they're faster than us so they get the new version into the index first. Now our aminsert sees a conflicting index entry,

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-24 Thread Tom Lane
Gregory Stark [EMAIL PROTECTED] writes: I was also considering going ahead and implementing Hannu's ALTER INDEX SET UNIQUE too. Don't waste your time, when we don't have an algorithm that would make it work. It's too late for 8.2 anyway... regards, tom lane

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-24 Thread Gregory Stark
Tom Lane [EMAIL PROTECTED] writes: Gregory Stark [EMAIL PROTECTED] writes: I was also considering going ahead and implementing Hannu's ALTER INDEX SET UNIQUE too. Don't waste your time, when we don't have an algorithm that would make it work. It's too late for 8.2 anyway... Oh, I think

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-24 Thread Tom Lane
Gregory Stark [EMAIL PROTECTED] writes: Tom Lane [EMAIL PROTECTED] writes: Unless someone's got a brilliant idea, my recommendation at this point is that we restrict the patch to building only non-unique indexes. I assume you'll add the check? Yeah, I'll take care of it. At the moment it

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-24 Thread Tom Lane
I wrote: I'm trying to work out whether we can fix this by taking an MVCC snapshot before we scan the index, and then inserting all tuples we find in the heap that are live according to the snap but are not present in the indexscan data. There are still race conditions in this but I think

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Hannu Krosing
Ühel kenal päeval, T, 2006-08-22 kell 16:48, kirjutas Tom Lane: Joshua D. Drake [EMAIL PROTECTED] writes: It's fairly clear that we could support concurrent builds of nonunique indexes, but is that enough of a use-case to justify it? I believe there would be. Most PostgreSQL users I run

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Hannu Krosing
Ühel kenal päeval, K, 2006-08-23 kell 11:05, kirjutas Hannu Krosing: Ühel kenal päeval, T, 2006-08-22 kell 16:48, kirjutas Tom Lane: Joshua D. Drake [EMAIL PROTECTED] writes: It's fairly clear that we could support concurrent builds of nonunique indexes, but is that enough of a use-case

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Zeugswetter Andreas DCP SD
Is it not possible to brute force this adding an AM method to insert without the uniqueness check? Hm. Actually there already is a feature of aminsert to allow suppressing the unique check, but I'm not sure whether using it for RECENTLY_DEAD tuples helps. Seems like we have to

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Greg Stark
Tom Lane [EMAIL PROTECTED] writes: What I think we can do about this is to include DELETE_IN_PROGRESS tuples in the set of candidate tuples to insert in the second pass. During the merge step that verifies whether the tuple is already in the index, if we find that it's not, then we must wait

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Greg Stark
Tom Lane [EMAIL PROTECTED] writes: In the past, the only way we could see HEAPTUPLE_INSERT_IN_PROGRESS or HEAPTUPLE_DELETE_IN_PROGRESS was for tuples created/deleted by our own transaction, and so the actions taken by IndexBuildHeapScan are to include in the index in both cases, but exclude

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Greg Stark
[Sorry for the duplicate -- I accidentally sent the previous before I was finished editing it] Tom Lane [EMAIL PROTECTED] writes: I think we can solve this by having IndexBuildHeapScan not index DELETE_IN_PROGRESS tuples if it's doing a concurrent build. The problem of old transactions

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Greg Stark
Hannu Krosing [EMAIL PROTECTED] writes: Ühel kenal päeval, K, 2006-08-23 kell 11:05, kirjutas Hannu Krosing: Maybe we could find a way to build a non-unique index first and then convert it to a unique one later, in yet another pass ? Or even add ALTER INDEX myindex ADD/DROP UNIQUE;

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
Greg Stark [EMAIL PROTECTED] writes: But then wouldn't we have deadlock risks? If we come across these records in a different order from someone else (possibly even the deleter) who also wants to lock them? Or would it be safe to lock and release them one by one so we only every hold one lock

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
Greg Stark [EMAIL PROTECTED] writes: It seems like it would be simpler to leave the core in charge the whole time. It would call an AM method to initialize state, then call an AM method for each tuple that should be indexed, and lastly call a finalize method. [ shrug... ] I'm uninterested in

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
Greg Stark [EMAIL PROTECTED] writes: Hmmm. Or is that true. The problem may be somewhat easier since at least you can be sure every tuple in the heap is in the index. So if you see a DELETE_IN_PROGRESS either it *was* a constraint violation prior to the delete and failing is reasonable or it's

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Hannu Krosing
Ühel kenal päeval, K, 2006-08-23 kell 09:01, kirjutas Tom Lane: Greg Stark [EMAIL PROTECTED] writes: Hmmm. Or is that true. The problem may be somewhat easier since at least you can be sure every tuple in the heap is in the index. So if you see a DELETE_IN_PROGRESS either it *was* a

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
stark [EMAIL PROTECTED] writes: Tom Lane [EMAIL PROTECTED] writes: [ thinks for a bit... ] At least, it seems hopeless if we use SnapshotNow. Does it help if we use a real snapshot? I'm thinking pass 1 inserts exactly those tuples that are good according to a snap taken at its beginning,

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread stark
Tom Lane [EMAIL PROTECTED] writes: Or do you mean we use SatisfiesVacuum to determine what to insert but SatisfiesSnapshot to determine whether to check uniqueness? Right. The problems seem to all stem from the risk of trying to unique-check more than one version of a tuple, and using a

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
stark [EMAIL PROTECTED] writes: What happens if someone inserts a record that we miss, but it gets deleted by the same phase 2 starts. So it's not visible to phase 2 but conflicts with some other record we find. I suppose that's ok since the delete would have to have comitted for that to

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Gregory Stark
Tom Lane [EMAIL PROTECTED] writes: I think that's OK, but the whole idea of using an MVCC snap in phase 2 doesn't work on closer inspection. The problem is still the same one that you need to take (at least) share lock on each tuple you insert into the index. Telling aminsert to check

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
Gregory Stark [EMAIL PROTECTED] writes: Tom Lane [EMAIL PROTECTED] writes: I think that's OK, but the whole idea of using an MVCC snap in phase 2 doesn't work on closer inspection. The problem is still the same one that you need to take (at least) share lock on each tuple you insert into the

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-23 Thread Tom Lane
I wrote: The problem case is that we take a tuple and try to insert it into the index. Meanwhile someone else updates the tuple, and they're faster than us so they get the new version into the index first. Now our aminsert sees a conflicting index entry, and as soon as it commits good

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Greg Stark
Tom Lane [EMAIL PROTECTED] writes: I think we can solve this by having IndexBuildHeapScan not index DELETE_IN_PROGRESS tuples if it's doing a concurrent build. Sure It's entirely possible for a tuple that is RECENTLY_DEAD or DELETE_IN_PROGRESS to have no entry in the index, if it was

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Tom Lane
Greg Stark [EMAIL PROTECTED] writes: Is it not possible to brute force this adding an AM method to insert without the uniqueness check? Hm. Actually there already is a feature of aminsert to allow suppressing the unique check, but I'm not sure whether using it for RECENTLY_DEAD tuples helps.

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Joshua D. Drake
Wow, that seems pretty unsatisfactory, all the waiting and locking sounds awful. Yeah, I'm very unhappy. The whole idea may be going down in flames :-( It's fairly clear that we could support concurrent builds of nonunique indexes, but is that enough of a use-case to justify it? I believe

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Tom Lane
Joshua D. Drake [EMAIL PROTECTED] writes: It's fairly clear that we could support concurrent builds of nonunique indexes, but is that enough of a use-case to justify it? I believe there would be. Most PostgreSQL users I run into, develop in production, which means being able to add an index

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Joshua D. Drake
Tom Lane wrote: Joshua D. Drake [EMAIL PROTECTED] writes: It's fairly clear that we could support concurrent builds of nonunique indexes, but is that enough of a use-case to justify it? I believe there would be. Most PostgreSQL users I run into, develop in production, which means being able

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Greg Stark
Tom Lane [EMAIL PROTECTED] writes: Greg Stark [EMAIL PROTECTED] writes: Is it not possible to brute force this adding an AM method to insert without the uniqueness check? Hm. Actually there already is a feature of aminsert to allow suppressing the unique check, but I'm not sure whether

Re: [HACKERS] Tricky bugs in concurrent index build

2006-08-22 Thread Tom Lane
Greg Stark [EMAIL PROTECTED] writes: What would happen if we just insert DELETE_IN_PROGRESS tuples normally? Would the only risk be that the index build would fail with a spurious unique constraint violation? I suppose it would be pretty common though given how updates work. Yeah, that's the