From: Justin Pryzby <pryzby@telsasoft.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number (atomic progress update)
Date: Sun, 29 Dec 2019 14:17:47 -0600
Message-ID: <20191229201747.GL12890@telsasoft.com> (raw)
In-Reply-To: <CA+TgmoYSvEP3weQaCPGf6+DXLy2__JbJUYtoXyWP=qHcyGbihA@mail.gmail.com>
References: <20191213030831.GT2082@telsasoft.com>
<20191213132850.GA103520@paquier.xyz>
<20191213224735.GY2082@telsasoft.com>
<20191215130708.GA19063@paquier.xyz>
<20191215162712.GZ2082@telsasoft.com>
<20191216024956.GC2344@paquier.xyz>
<20191224012428.GK30414@telsasoft.com>
<20191224041909.GA323806@paquier.xyz>
<20191226155704.GA12890@telsasoft.com>
<CA+TgmoYSvEP3weQaCPGf6+DXLy2__JbJUYtoXyWP=qHcyGbihA@mail.gmail.com>
On Sat, Dec 28, 2019 at 07:21:31PM -0500, Robert Haas wrote:
> On Thu, Dec 26, 2019 at 10:57 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > I agree that's better.
> > I don't see any reason why the progress params need to be updated atomically.
> > So rebasified against your patch.
>
> I am not sure whether it's important enough to make a stink about, but
> it bothers me a bit that this is being dismissed as unimportant. The
> problem is that, if the updates are not atomic, then somebody might
> see the data after one has been updated and the other has not yet been
> updated. The result is that when the phase is
> PROGRESS_VACUUM_PHASE_VACUUM_INDEX, someone reading the information
> can't tell whether the number of index scans reported is the number
> *previously* performed or the number performed including the one that
> just finished. The race to see the latter state is narrow, so it
> probably wouldn't come up often, but it does seem like it would be
> confusing if it did happen.
What used to be atomic was this:
- hvp_val[0] = PROGRESS_VACUUM_PHASE_VACUUM_HEAP;
- hvp_val[1] = vacrelstats->num_index_scans + 1;
=> switch from PROGRESS_VACUUM_PHASE_VACUUM INDEX to HEAP and increment
index_vacuum_count, which is documented as the "Number of completed index
vacuum cycles."
Now, it 1) increments the number of completed scans; and, 2) then progresses
phase to HEAP, so there's a window where the number of completed scans is
incremented, and it still says VACUUM_INDEX.
Previously, if it said VACUUM_INDEX, one could assume that index_vacuum_count
would increase at least once more, and that's no longer true. If someone sees
VACUUM_INDEX and some NUM_INDEX_VACUUMS, and then later sees VACUUM_HEAP or
other later stage, with same (maybe final) value of NUM_INDEX_VACUUMS, that's
different than previous behavior.
It seems to me that a someone or their tool monitoring pg_stat shouldn't be
confused by this change, since:
1) there's no promise about how high NUM_INDEX_VACUUMS will or won't go; and,
2) index_vacuum_count didn't do anything strange like decreasing, or increased
before the scans were done; and,
3) the vacuum can finish at any time, and the monitoring process presumably
knows that when the PID is gone, it's finished, even if it missed intermediate
updates;
The behavior is different from before, but I think that's ok: the number of
scans is accurate, and the PHASE is accurate, even though it'll change a moment
later.
I see there's similar case here:
| /* report all blocks vacuumed; and that we're cleaning up */
| pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
| pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
| PROGRESS_VACUUM_PHASE_INDEX_CLEANUP);
heap_blks_scanned is documented as "Number of heap blocks SCANNED", and it
increments exactly to heap_blks_total. Would someone be confused if
heap_blks_scanned==heap_blks_total AND phase=='scanning heap' ? I think they'd
just expect PHASE to be updated a moment later. (And if it wasn't, I agree they
should then be legitimately confused or concerned).
Actually, the doc says:
|If heap_blks_scanned is less than heap_blks_total, the system will return to
|scanning the heap after this phase is completed; otherwise, it will begin
|cleaning up indexes AFTER THIS PHASE IS COMPLETED.
I read that to mean that it's okay if heap_blks_scanned==heap_blks_total when
scanning/vacuuming heap.
Justin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, robertmhaas@gmail.com, michael@paquier.xyz, alvherre@2ndquadrant.com, andres@anarazel.de
Subject: Re: error context for vacuum to include block number (atomic progress update)
In-Reply-To: <20191229201747.GL12890@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox