Kevin Grittner wrote:
> I still need to review the timing calls, since I'm not familiar
> with them so it wasn't immediately obvious to me whether they
> were being used correctly. I have no reason to believe that they
> aren't, but feel I should check.
It seems odd to me that assignment of one instr_time to another is
done with INSTR_TIME_SET_ZERO() of the target followed by
INSTR_TIME_ADD() with the target and the source. It seems to me
that simple assignment would be more readable, and I can't see any
down side.
Why shouldn't:
INSTR_TIME_SET_ZERO(elapsed); INSTR_TIME_ADD(elapsed, currenttime); INSTR_TIME_SUBTRACT(elapsed, starttime);
instead be?:
elapsed = currenttime; INSTR_TIME_SUBTRACT(elapsed, starttime);
And why shouldn't:
INSTR_TIME_SET_ZERO(starttime); INSTR_TIME_ADD(starttime, currenttime);
instead be?:
starttime = currenttime;
Resetting starttime this way seems especially odd.
> Also, I want to do another pass looking just for off-by-one
> errors on blkno. Again, I have no reason to believe that there is
> a problem; it just seems like it would be a particularly easy
> type of mistake to make and miss when a key structure has this
> field:
>
> BlockNumber nonempty_pages;
> /* actually, last nonempty page + 1 */
No problems found with that.
> And I want to test more.
The patch worked as advertised in all my tests, but I became
uncomforatable with the games being played with the last autovacuum
timestamp and the count of dead tuples. Sure, that causes
autovacuum to kick back in until it has dealt with the truncation,
but it makes it hard for a human looking at the stat views to see
what's going on, and I'm not sure it wouldn't lead to bad plans due
to stale statistics.
Personally, I would rather see us add a boolean to indicate that
autovacuum was needed (regardless of the math on the other columns)
and use that to control the retries -- leaving the other columns
free to reflect reality.
-Kevin
