On Tue, Jun 30, 2015 at 11:19 AM, Heikki Linnakangas <hlinn...@iki.fi> wrote: > I agree it would be cleaner to have a separate CHECK_UNIQUE_XXX code for > speculative insertions. You've defined CHECK_UNIQUE_SPECULATIVE as "like > CHECK_UNIQUE_PARTIAL, but you don't have to insert the index tuple if > there's a conflict". I think it'd be better to define it as "like > CHECK_UNIQUE_YES, but return FALSE instead of throwing an error on > conflict". The difference is that the aminsert would not be allowed to > return FALSE when there is no conflict.
Suppose we do it that way. Then what's the difference between CHECK_UNIQUE_SPECULATIVE and CHECK_UNIQUE_PARTIAL? You've just effectively required the CHECK_UNIQUE_YES case to not physically insert a physical tuple before throwing an error, which does not seem essential to the existing definition of CHECK_UNIQUE_YES -- you've redefined CHECK_UNIQUE_YES in a way that nbtree happens to meet at the moment. If we had an amcanunique AM that worked a bit like exclusion constraints, this new obligation for CHECK_UNIQUE_YES might make it impossible for that to work. I'm not necessarily in disagreement. I just didn't do it that way for that reason. > Actually, even without this patch, a dummy implementation of aminsert that > always returns FALSE in CHECK_UNIQUE_PARTIAL mode would be legal per the > docs, but would loop forever. So that would be nice to fix or document away, > even though it's not a problem for B-tree currently. UNIQUE_CHECK_PARTIAL calls to an aminsert function are allowed to report false positives (of not being unique, by returning FALSE as you say), where there may not have been a conflict, but it's not clear (e.g. because the conflicting xact has yet to commit/abort). You still need to insert, though, so that the recheck at end-of-xact works for deferred constraints. At that point, it's really not too different to ordinary unique enforcement, except that the other guy is guaranteed to notice you, too. You can construct a theoretical case where lock starvation occurs with unique constraint enforcement. I think it helps with nbtree here that someone will reliably *not* see a conflict when concurrently inserting, because unique index "value locking" exists (by locking the first leaf page a value could be on with a buffer lock). But even if that wasn't the case, the insert + recheck thing would be safe, just as with exclusion constraints...provided you insert to begin with, that is. >> In passing, I have make ExecInsertIndexTuples() give up when a >> speculative insertion conflict is detected. Again, this is not about >> bloat prevention; it's about making the code easier to understand by >> not doing something that is unnecessary, and in avoiding that also >> avoiding the implication that it is necessary. There are already >> enough complicated interactions that *are* necessary (e.g. "livelock >> avoidance" for exclusion constraints). Let us make our intent clearer. > > Hmm, not sure I like those changes. ExecInsertIndexTuples's behaviour now > depends on whether specConflict is passed, but that seems weird as > specConflict is an output parameter. Your statement is ambiguous and confusing to me. Are you objecting to the idea of returning from ExecInsertIndexTuples() "early" in general, or to one narrow aspect of how that was proposed -- the fact that it occurs due to "specConflict != NULL" rather than "noDupErr"? Obviously if it's just the latter, then that's a small adjustment. But AFAICT your remark about specConflict could one detail of a broader complaint about an idea that you dislike generally. >> The patch also updates the AM interface documentation (the part >> covering unique indexes). It seems quite natural to me to document the >> theory of operation for speculative insertion there. > > > Yeah, although I find the explanation pretty long-winded and difficult to > understand ;-). I don't know what you mean. You say things like this to me fairly regularly, but I'm always left wondering what the take-away should be. Please be more specific. Long-winded generally means that more words than necessary were used. I think that the documentation proposed is actually very information dense, if anything. As always, I aimed to keep it consistent with the surrounding documentation. I understand that the material might be a little hard to follow, but it concerns how someone can externally implement a Postgres index access method that is "amcanunique". That's a pretty esoteric subject -- so far, we've had exactly zero takers (even without the "amcanunique" part). It's damn hard to make these ideas accessible. I feel that I have an obligation to share information about (say) how the speculative insertion mechanism works -- the "theory of operation" seems very useful. Like any one of us, I might not be around to provide input on something like that in the future, so it makes sense to be thorough and unambiguous. There are very few people (3?) who have a good sense of how it works currently, and there are some very subtle aspects to it that I tried to point out. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers