Re: matview scannability rehash (was Re: Drastic performance loss in assert-enabled build in HEAD)

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

> I think a large part of what is at issue here stems from a bad
> name for the new bool field I added to the RelationData structure
> -- instead of rd_isscannable it should probably be called
> something like rd_ispopulated.  The current name led to some
> fuzzy thinking on my part when it was referenced, and both the
> name and a couple ill-considered uses of it probably contributed
> to concerns about how it was generated and used.
>
> I will post a draft patch today to see whether concerns abate.
> Basically, the name change should help make clear that this is
> not intended to be the only way to determine whether a matview is
> scannable.  Second, there is at least on (and probably more)
> direct tests of this field which should use a function for a
> scannability test.  For 9.3, that will just wrap a test of this
> bool, but it makes clear what the longer-term intent is, and help
> ensure that things don't get missed when patches are written in
> later releases.  Third, some comments need to be corrected and
> added.
>
> Hopefully it can help get us all onto the same page.  If not, it
> should at least better focus the discussion.

Attached is a firt cut at drawing a bright line between the notion
of whether a matview has been *populated* and whether it is
*scannable*.  In 9.3 one is true if and only if the other is, but
I plan on doing work such that this will no longer be true in the
next release, and not making a clear distinction now has been
confusing everyone, including me.

This patch is light on functional changes, and heavier on name and
comment changes.  It does fix the lack of locking for the
user-visible pg_relation_is_scannable() function, and it does
correct the performance regression in pg_dump, but those should be
the only user-visible changes.  Internally I also tried to correct
a modularity violation complained of by Tom.

Vacuum still needs to be taught not to truncate away the first page
of a matview, but that seemed like it was better left for a
separate patch, and if we decide not to use the current technique
for identifying a non-populated matview, it owuld be one more thing
to undo.

It passed `make check-world` and `make installcheck-world`, and a
dump and load of the regression database works.  I got sidetracked
with some support issues, so I didn't have time to dig around for
other weak comments, but I think I'm close on all other changes.

Comments?

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company