Re: INSERT ... ON CONFLICT {UPDATE | IGNORE}

On Sun, Oct 26, 2014 at 4:39 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> I don't care whether you actually generate index-paths or not, and in
>> fact I suspect it makes no sense to do so. But I do care that you do
>> a cost comparison between the available indexes and pick the one that
>> looks cheapest. If somebody's got a bloated index and builds a new
>> one, they will want it to get used even before they drop the old one.
>
> That seems extremely narrow. I am about ready to just do what you say,
> by costing the index based on something like relpages, but for the
> record I see no point. If I see no point, and you're sure that there
> is a point, then there is a danger that I'll miss the point, which
> contributes to my giving push back. I know I said that already, but I
> reiterate it once more for emphasis.

I don't think it's either narrow or difficult: I think you just need
to apply the existing index costing function. CLUSTER does a similar
calculation to figure out whether to seq-scan or sort, even though it
doesn't use index paths per se, or at least I don't think it does.

>> Even if that weren't an issue, though, the fact that you can't
>> re-parse but you can re-plan is a killer point AFAICS. It means you
>> are borked if the statement gets re-executed after the index you
>> picked is dropped.
>
> That simply isn't the case. As specifically noted in comments in the
> patch, relcache invalidation works in such a way as to invalidate the
> query proper, even when only an index has been dropped. For a time
> during development, the semantic dependence was more directly
> represented by actually having extract_query_dependencies() extract
> the arbitrating unique index pg_class Oid as a dependency for the
> benefit of the plan cache, but I ultimately decided against doing
> anything more than noting the potential future problem (the problem
> that arises in a world where relcache invalidation is more selective).

I don't know what you mean by "invalidate the query proper". Here's
my chain of reasoning: If the index selection occurs in parse
analysis, then it's embedded in the parse tree. If this can used a
writable CTE, then the parse tree can get stored someplace. If the
index is then dropped, the parse tree is no good any more. Where's
the flaw in that reasoning?

I haven't looked at the patch to know exactly what will happen in that
case, but it seems to me like it must either not work or there must be
some ugly hack to make it work.

> But, I digress: I'll have inference occur during planning during the
> next revision since you think that's important.

Great.

> I think that it's accurate to say that I asked the community to do
> that once, last year. I was even very explicit about it at the time.
> I'm surprised that you think that's the case now, though. I learned my
> lesson a little later: don't do that, because fellow contributors are
> unwilling to go along with it, even for something as important as
> this.

I'm just telling you how I feel. I can't vouch for it being perfectly
fair or accurate, though I do strive for that.

> As recently as a few months ago, you wanted me to go a *completely*
> different direction with this (i.e. attempt an UPDATE first). I'm
> surprised that you're emphasizing the EXCLUDED()/CONFLICTING() thing
> so much. Should I take it that I more or less have your confidence
> that the way the patch fits together at a high level is sound?

No. Commenting on one aspect of a patch doesn't imply agreement with
other aspects of the patch. Please don't put words into my mouth. I
haven't reviewed this patch in detail; I've only commented on specific
aspects of it as they have arisen in discussion. I may or may not
someday review it in detail, but not before I'm fairly confident that
the known issues raised by other community members have been addressed
as thoroughly as possible.

> In reality, the
> main reason for that is: getting this feature to a point where it
> might plausibly be committed is bloody difficult, as evidenced by the
> fact that it took this long for someone to produce a patch. Please
> don't lose sight of that.

I'm not.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company