Обсуждение: Add n_tup_newpage_upd to pg_stat table views

Hi,

On 2023-01-27 18:23:39 -0500, Corey Huinker wrote:
> This patch adds the n_tup_newpage_upd to all the table stat views.
>
> Just as we currently track HOT updates, it should be beneficial to track
> updates where the new tuple cannot fit on the existing page and must go to
> a different one.

I like that idea.


I wonder if it's quite detailed enough - we can be forced to do out-of-page
updates because we actually are out of space, or because we reach the max
number of line pointers we allow in a page. HOT pruning can't remove dead line
pointers, so that doesn't necessarily help.

Which e.g. means that:

> Hopefully this can give users some insight as to whether their current
> fillfactor settings need to be adjusted.

Isn't that easy, because you can have a page with just a visible single tuple
on, but still be unable to do a same-page update. The fix instead is to VACUUM
(more aggressively).


OTOH, just seeing that there's high percentage "out-of-page updates" provides
more information than we have right now.  And the alternative would be to add
yet another counter.


Similarly, it's a bit sad that we can't distinguish between the number of
potential-HOT out-of-page updates and the other out-of-page updates. But
that'd mean even more counters.


I guess we could try to add tracepoints to allow to distinguish between those
cases instead? Not a lot of people use those though.



> @@ -372,8 +372,11 @@ pgstat_count_heap_update(Relation rel, bool hot)
>          pgstat_info->trans->tuples_updated++;
>
>          /* t_tuples_hot_updated is nontransactional, so just advance it */
> -        if (hot)
> +        if (hut == PGSTAT_HEAPUPDATE_HOT)
>              pgstat_info->t_counts.t_tuples_hot_updated++;
> +        else if (hut == PGSTAT_HEAPUPDATE_NEW_PAGE)
> +            pgstat_info->t_counts.t_tuples_newpage_updated++;
> +
>      }
>  }
>

I think this might cause some trouble for existing monitoring setups after an
upgrade. Suddenly the number of updates will appear way lower than
before... And if we end up eventually distinguishing between different reasons
for out-of-page updates, or hot/non-hot out-of-page that'll happen again.

I wish we'd included HOT updates in the total number of updates, and just kept
HOT updates a separate counter that'd always be less than updates in total.


From that angle: Perhaps it'd be better to have counter for how many times a
page is found to be full during an update?


> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -3155,7 +3155,8 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
>                  pagefree;
>      bool        have_tuple_lock = false;
>      bool        iscombo;
> -    bool        use_hot_update = false;
> +    PgStat_HeapUpdateType update_type = PGSTAT_HEAPUPDATE_NON_HOT;
> +
>      bool        key_intact;
>      bool        all_visible_cleared = false;
>      bool        all_visible_cleared_new = false;
> @@ -3838,10 +3839,11 @@ l2:
>           * changed.
>           */
>          if (!bms_overlap(modified_attrs, hot_attrs))
> -            use_hot_update = true;
> +            update_type = PGSTAT_HEAPUPDATE_HOT;
>      }
>      else
>      {
> +        update_type = PGSTAT_HEAPUPDATE_NEW_PAGE;
>          /* Set a hint that the old page could use prune/defrag */
>          PageSetFull(page);
>      }
> @@ -3875,7 +3877,7 @@ l2:
>       */
>      PageSetPrunable(page, xid);
>
> -    if (use_hot_update)
> +    if (update_type == PGSTAT_HEAPUPDATE_HOT)

It's a bit weird that heap_update() uses a pgstat type to decide what to
do. But not sure there's a much better alternative.

Greetings,

Andres Freund

On Fri, Jan 27, 2023 at 3:55 PM Andres Freund <andres@anarazel.de> wrote:
> I wonder if it's quite detailed enough - we can be forced to do out-of-page
> updates because we actually are out of space, or because we reach the max
> number of line pointers we allow in a page. HOT pruning can't remove dead line
> pointers, so that doesn't necessarily help.

It would be hard to apply that kind of information, I suspect. Maybe
it's still worth having, though.

> Similarly, it's a bit sad that we can't distinguish between the number of
> potential-HOT out-of-page updates and the other out-of-page updates. But
> that'd mean even more counters.

ISTM that it would make more sense to do that at the index level
instead. It wouldn't be all that hard to teach ExecInsertIndexTuples()
to remember whether each index received the indexUnchanged hint used
by bottom-up deletion, which is approximately the same thing, but
works at the index level.

This is obviously more useful, because you have index-granularity
information that can guide users in how to index to maximize the
number of HOT updates. And, even if changing things around didn't lead
to the hoped-for improvement in the rate of HOT updates, it would at
least still allow the indexes on the table to use bottom-up deletion
more often, on average.

Admittedly this has some problems. The index_unchanged_by_update()
logic probably isn't as sophisticated as it ought to be because it's
driven by the statement-level extraUpdatedCols bitmap set, and not a
per-tuple test, like the HOT safety test in heap_update() is.
But...that should probably be fixed anyway.

> I think this might cause some trouble for existing monitoring setups after an
> upgrade. Suddenly the number of updates will appear way lower than
> before... And if we end up eventually distinguishing between different reasons
> for out-of-page updates, or hot/non-hot out-of-page that'll happen again.

Uh...no it won't? The new counter is totally independent of the existing
HOT counter, and the transactional all-updates counter. It's just that
there is an enum that encodes which of the two non-transactional "sub
counters" to use (either for HOT updates or new-page-migration
updates).

> I wish we'd included HOT updates in the total number of updates, and just kept
> HOT updates a separate counter that'd always be less than updates in total.

Uh...we did in fact do it that way to begin with?

> From that angle: Perhaps it'd be better to have counter for how many times a
> page is found to be full during an update?

Didn't Corey propose a patch to add just that? Do you mean something
more specific, like a tracker for when an UPDATE leaves a page full,
without needing to go to a new page itself?

If so, then that does require defining what that really means, because
it isn't trivial. Do you assume that all updates have a successor
version that is equal in size to that of the UPDATE that gets counted
by this hypothetical other counter of yours?

--
Peter Geoghegan

Hi,

On 2023-01-27 17:59:32 -0800, Peter Geoghegan wrote:
> > I think this might cause some trouble for existing monitoring setups after an
> > upgrade. Suddenly the number of updates will appear way lower than
> > before... And if we end up eventually distinguishing between different reasons
> > for out-of-page updates, or hot/non-hot out-of-page that'll happen again.
> 
> Uh...no it won't? The new counter is totally independent of the existing
> HOT counter, and the transactional all-updates counter. It's just that
> there is an enum that encodes which of the two non-transactional "sub
> counters" to use (either for HOT updates or new-page-migration
> updates).
>
> > I wish we'd included HOT updates in the total number of updates, and just kept
> > HOT updates a separate counter that'd always be less than updates in total.
> 
> Uh...we did in fact do it that way to begin with?

Sorry, I misread the diff, and then misremembered some old issue.


> > From that angle: Perhaps it'd be better to have counter for how many times a
> > page is found to be full during an update?
> 
> Didn't Corey propose a patch to add just that? Do you mean something
> more specific, like a tracker for when an UPDATE leaves a page full,
> without needing to go to a new page itself?

Nope, I just had a brainfart.


> > Similarly, it's a bit sad that we can't distinguish between the number of
> > potential-HOT out-of-page updates and the other out-of-page updates. But
> > that'd mean even more counters.
> 
> ISTM that it would make more sense to do that at the index level
> instead. It wouldn't be all that hard to teach ExecInsertIndexTuples()
> to remember whether each index received the indexUnchanged hint used
> by bottom-up deletion, which is approximately the same thing, but
> works at the index level.

I don't think that'd make it particularly easy to figure out how often
out-of-space causes non-HOT updates to go out of page, and how often it causes
potential HOT updates to go out of page.  If you just have a single index,
it's not too hard, but after that seems decidedly nontrival.

But I might just be missing what you're suggesting.


Greetings,

Andres Freund

On Fri, Jan 27, 2023 at 6:44 PM Andres Freund <andres@anarazel.de> wrote:
> I don't think that'd make it particularly easy to figure out how often
> out-of-space causes non-HOT updates to go out of page, and how often it causes
> potential HOT updates to go out of page.  If you just have a single index,
> it's not too hard, but after that seems decidedly nontrival.
>
> But I might just be missing what you're suggesting.

It would be useless for that, of course. But it would be a good proxy
for what specific indexes force non-hot updates due to HOT safety
issues. This would work independently of the issue of what's going on
in the heap. That matters too, of course, but in practice the main
problem is likely the specific combination of indexes and updates.
(Maybe it would just be an issue with heap fill factor, at times, but
even then you'd still want to rule out basic HOT safety issues first.)

If you see one particular index that gets a far larger number of
non-hot updates that are reported as "logical changes to the indexed
columns", then dropping that index has the potential to make the HOT
update situation far better.

--
Peter Geoghegan

Hi,

On 2023-01-30 13:40:15 -0500, Corey Huinker wrote:
> I must be missing something, I only see the check for running out of space,
> not the check for exhausting line pointers. I agree dividing them would be
> interesting.

See PageGetHeapFreeSpace(), particularly the header comment and the
MaxHeapTuplesPerPage check.


> > Similarly, it's a bit sad that we can't distinguish between the number of
> > potential-HOT out-of-page updates and the other out-of-page updates. But
> > that'd mean even more counters.
>
> I wondered that too, but the combinations of "would have been HOT but not
> no space" and "key update suggested not-HOT but it was id=id so today's
> your lucky HOT" combinations started to get away from me.

Not sure I follow the second part. Are you just worried about explaining when
a HOT update is possible?


> I wondered if there was interest in knowing if the tuple had to get
> TOASTed, and further wondered if we would be interested in the number of
> updates that had to wait on a lock. Again, more counters.

Those seem a lot less actionable / related to the topic at hand to me.

Greetings,

Andres Freund

On Fri, Jan 27, 2023 at 3:23 PM Corey Huinker <corey.huinker@gmail.com> wrote:
> This patch adds the n_tup_newpage_upd to all the table stat views.

I think that this is pretty close to being committable already.

I'll move on that early next week, barring any objections.

--
Peter Geoghegan

On Fri, Mar 17, 2023 at 3:22 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I think that this is pretty close to being committable already.

Attached revision has some small tweaks by me. Going to commit this
revised version tomorrow morning.

Changes:

* No more dedicated struct to carry around the type of an update.

We just use two boolean arguments to the pgstats function instead. The
struct didn't seem to be adding much, and it was distracting to track
the information this way within heap_update().

* Small adjustments to the documentation.

Nearby related items were tweaked slightly to make everything fit
together a bit better. For example, the description of n_tup_hot_upd
is revised to make it obvious that n_tup_hot_upd counts row updates
that can never get counted under the new n_tup_newpage_upd counter.

--
Peter Geoghegan

On Wed, Mar 22, 2023 at 05:14:08PM -0700, Peter Geoghegan wrote:
> * Small adjustments to the documentation.
>
> Nearby related items were tweaked slightly to make everything fit
> together a bit better. For example, the description of n_tup_hot_upd
> is revised to make it obvious that n_tup_hot_upd counts row updates
> that can never get counted under the new n_tup_newpage_upd counter.

@@ -168,6 +168,7 @@ typedef struct PgStat_TableCounts
        PgStat_Counter t_tuples_updated;
        PgStat_Counter t_tuples_deleted;
        PgStat_Counter t_tuples_hot_updated;
+       PgStat_Counter t_tuples_newpage_updated;
        bool            t_truncdropped;

I have in the works something that's going to rename these fields to
not have the "t_" prefix anymore, to ease some global refactoring in
pgstatfuncs.c so as we have less repetitive code with the functions
that grab these counters.  I don't think that's something you need to
name without the prefix here, just a FYI that this is going to be
immediately renamed ;)
--
Michael

On Wed, Mar 22, 2023 at 10:38 PM Corey Huinker <corey.huinker@gmail.com> wrote:
> That's probably a good move, especially if we start tallying updates that use TOAST.

Okay, pushed.

Thanks
--
Peter Geoghegan