Re: Protect syscache from bloating with negative cache entries

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: Jim(dot)Nasby(at)BlueTreble(dot)com
Cc: tgl(at)sss(dot)pgh(dot)pa(dot)us, robertmhaas(at)gmail(dot)com, craig(at)2ndquadrant(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Protect syscache from bloating with negative cache entries
Date: 2017-01-24 07:58:52
Message-ID: 20170124.165852.78455792.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hello, thank you for lookin this.

At Mon, 23 Jan 2017 16:54:36 -0600, Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> wrote in <21803f50-a823-c444-ee2b-9a153114f454(at)BlueTreble(dot)com>
> On 1/21/17 6:42 PM, Jim Nasby wrote:
> > On 12/26/16 2:31 AM, Kyotaro HORIGUCHI wrote:
> >> The points of discussion are the following, I think.
> >>
> >> 1. The first patch seems working well. It costs the time to scan
> >> the whole of a catcache that have negative entries for other
> >> reloids. However, such negative entries are created by rather
> >> unusual usages. Accesing to undefined columns, and accessing
> >> columns on which no statistics have created. The
> >> whole-catcache scan occurs on ATTNAME, ATTNUM and
> >> STATRELATTINH for every invalidation of a relcache entry.
> >
> > I took a look at this. It looks sane, though I've got a few minor
> > comment tweaks:
> >
> > + * Remove negative cache tuples maching a partial key.
> > s/maching/matching/
> >
> > +/* searching with a paritial key needs scanning the whole cache */
> >
> > s/needs/means/
> >
> > + * a negative cache entry cannot be referenced so we can remove
> >
> > s/referenced/referenced,/
> >
> > I was wondering if there's a way to test the performance impact of
> > deleting negative entries.

Thanks for the pointing out. These are addressed.

> I did a make installcheck run with CATCACHE_STATS to see how often we
> get negative entries in the 3 caches affected by this patch. The
> caches on pg_attribute get almost no negative entries. pg_statistic
> gets a good amount of negative entries, presumably because we start
> off with no entries in there. On a stable system that presumably won't
> be an issue, but if temporary tables are in use and being analyzed I'd
> think there could be a moderate amount of inval traffic on that
> cache. I'll leave it to a committer to decide if they thing that's an
> issue, but you might want to try and quantify how big a hit that is. I
> think it'd also be useful to know how much bloat you were seeing in
> the field.
>
> The patch is currently conflicting against master though, due to some
> caches being added. Can you rebase?

Six new syscaches in 665d1fa was conflicted and 3-way merge
worked correctly. The new syscaches don't seem to be targets of
this patch.

> BTW, if you set a slightly larger
> context size on the patch you might be able to avoid rebases; right
> now the patch doesn't include enough context to uniquely identify the
> chunks against cacheinfo[].

git format-patch -U5 fuses all hunks on cacheinfo[] together. I'm
not sure that such a hunk can avoid rebases. Is this what you
suggested? -U4 added an identifiable forward context line for
some elements so the attached patch is made with four context
lines.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
0001-Cleanup-negative-cache-of-pg_statistic-when-dropping.patch text/x-patch 25.3 KB
0002-Cleanup-negative-cache-of-pg_class-when-dropping-a-s.patch text/x-patch 23.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Torsten Zuehlsdorff 2017-01-24 07:59:59 Re: Checksums by default?
Previous Message Corey Huinker 2017-01-24 07:54:16 Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)