Robert Haas <robertmh...@gmail.com> writes: > On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <t...@sss.pgh.pa.us> wrote: >> I have a very strong feeling that this patch was not fully baked.
> I think you're right, but I don't understand the comment in the > preceding paragraph. How does this problem prevent tupgone from > getting set? My point is that I suspect that tupgone *shouldn't* get set. It's not (going to be) gone. > It looks to me like nleft_dead_tuples should be ripped out. That was pretty much what I was thinking too. It makes more sense just to treat this case identically to dead-but-not-yet-removable. I have substantial doubts about nleft_dead_itemids being worth anything, as well. > We should do something like: > if (params->index_cleanup == VACOPT_TERNARY_DISABLED) > { > nkeep += tups_vacuumed; > tups_vacuumed = 0; > } No. I'm thinking there should be exactly one test of index_cleanup in this logic, and what it would be is along the lines of if (HeapTupleIsHotUpdated(&tuple) || HeapTupleIsHeapOnly(&tuple) || + params->index_cleanup == VACOPT_TERNARY_DISABLED) nkeep += 1; else In general, this thing has a strong whiff of "large patch with a small patch struggling to get out". regards, tom lane