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