Date: Sun, 22 Mar 2020 09:38:29 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Why does [auto-]vacuum delay not report a wait event?
Message-ID: <20200322143829.GD2563@telsasoft.com>
References: <20200319224449.pbjrivflbf4x76tj@alap3.anarazel.de>
 <20200321040750.GD13662@telsasoft.com>
 <20200322002457.dvepqpj4legcmuxn@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200322002457.dvepqpj4legcmuxn@alap3.anarazel.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Sat, Mar 21, 2020 at 05:24:57PM -0700, Andres Freund wrote:
> > Also, I noticed that SLEEP_ON_ASSERT comment (31338352b) wants to use pg_usleep
> > "which seems too short.".  Surely it should use pg_sleep, added at 782eefc58 a
> > few years later ?
> 
> I don't see problem with using sleep here?

There's no problem with pg_sleep (with no "u") - it just didn't exist when
SLEEP_ON_ASSERT was added (and I guess it's potentially unsafe to do much of
anything, like loop around pg_usleep(1e6)).  I'm suggesting it *should* use
pg_sleep, rather than explaining why pg_usleep (with a "u") doesn't work.

> > Also, there was a suggestion recently that this should have a separate
> > vacuum_progress phase:
> > |vacuumlazy.c:#define VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL	50 /* ms */
> > |vacuumlazy.c:pg_usleep(VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL * 1000L);
> > 
> > I was planning to look at that eventually ; should it have a wait event instead
> > or in addition ?
> 
> A separate phase? How would that look like? We don't want to replace the
> knowledge that currently e.g. the heap scan is in progress?

I don't think that's an issue, since the heap scan is done at that point ?
heap_vacuum_rel() (the publicly callable routine) calls lazy_scan_heap (which
does everything) and then (optionally) lazy_truncate_heap() and then
immediately afterwards does:

        pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
			PROGRESS_VACUUM_PHASE_FINAL_CLEANUP);
        ...
        pgstat_progress_end_command();

> > VACUUM VERBOSE wouldn't normally be run with cost_delay > 0, so that field will
> > typically be zero, so I made it conditional
> 
> I personally dislike conditional output like that, because it makes
> parsing the output harder.

I dislike it too, mostly because there's a comment explaining why it's done
like that, without any desirable use of the functionality.  If it's not useful
for a case where the field is typically zero, it should go away until its
utility is instantiated.

-- 
Justin