Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <tgl@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