Re: error context for vacuum to include block number

On Thu, Feb 13, 2020 at 02:55:53PM +0900, Masahiko Sawada wrote:
> You need to add a newline to follow the limit line lengths so that the
> code is readable in an 80-column window. Or please run pgindent.

For now I :set tw=80

> 2.
> I think that making initialization process of errcontext argument a
> function is good. But maybe we can merge these two functions into one.

Thanks, this is better, and I used that.

> init_error_context_heap and init_error_context_index actually don't
> only initialize the callback arguments but also push the vacuum
> errcallback, in spite of the function name having 'init'. Also I think
> it might be better to only initialize the callback arguments in this
> function and to set errcallback by caller, rather than to wrap pushing
> errcallback by a function.

However I think it's important not to repeat this 4 times:
        errcallback->callback = vacuum_error_callback;
        errcallback->arg = errcbarg;
        errcallback->previous = error_context_stack;
        error_context_stack = errcallback;

So I kept the first 3 of those in the function and copied only assignment to
the global.  That helps makes the heap scan function clear, which assigns to it
twice.

BTW, for testing, I'm able to consistently hit the "vacuuming block" case like
this:

SET statement_timeout=0; DROP TABLE t; CREATE TABLE t(i int); CREATE INDEX ON t(i); INSERT INTO t SELECT
generate_series(1,99999);UPDATE t SET i=i-1; SET statement_timeout=111;  SET vacuum_cost_delay=3; SET
vacuum_cost_page_dirty=0;SET vacuum_cost_page_hit=11; SET vacuum_cost_limit=33; SET statement_timeout=3333; VACUUM
VERBOSEt;
 

Thanks for re-reviewing.

-- 
Justin