Hello, thank you for lookin this.
At Mon, 23 Jan 2017 16:54:36 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in
<21803f50-a823-c444-ee2b-9a153114f454@BlueTreble.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
