Re: Remaining beta blockers

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If you want to call the pg_class column relispopulated rather
>> than relisscannable, I have no particular objection to that

> That column name and the wording of some comments are the main
> things, although I'm also wondering whether it is bad form to force
> users to test the pg_class.relispopulated column if they want to
> test whether they can currently scan a matview, by removing the
> pg_relation_is_scannable() function. As I mentioned earlier when
> you asked why these two distinct properties weren't both exposed, I
> mentioned that I hadn't thought that the "populated" property was
> likely to be useful at the SQL level, but then questioned that,
> saying that I wasn't sure I picked the right property to pay
> attention to in pg_dump - and if pg_dump needed the "populated"
> property it had to be exposed. I've come around to thinking that
> it is more proper to use populated, but I have the same question
> you asked earlier -- If it will be important or users to understand
> that these are distinct properties, why are we just exposing one of
> them?

That's fair. So what say we call the pg_class column relispopulated
or something like that, and reinstate pg_relation_is_scannable()
as a function, for any client-side code that wants to test that
property as distinct from is-populated?

> The flip side of that is that it might be confusing to try
> to explain why users should care which test they use before they
> are capable of returning different results.

That's a good point too, though; if they are returning the same thing
right now, it's not very clear that users will pick the right test to
make anyway. Especially not if pg_relation_is_scannable() is a couple
orders of magnitude more expensive, which it will be, cf my original
complaint about pg_dump slowdown.

> Also, rather than do the direct update to pg_class in pg_dump, how
> would you feel about an ALTER MATERIALIZED VIEW option to set the
> populated state?

It seems a bit late to be adding such a thing; moreover, how would
you inject any data without doing something like what pg_upgrade is
doing? I see no point in an ALTER command until there's some other
SQL-level infrastructure for incremental matview updates.

In the context of pg_dump's binary upgrade option, I had thought of
adding a new pg_upgrade_support function, but I saw that we already use
direct pg_class updates for other nearby binary-upgrade hacking; so it
didn't seem unreasonable to do it that way here.

> I'm just reviewing the changes I made, and figured it might be good
> to show a diff between my form of the patch and yours, but I'm
> getting a lot spurious differences based on how we generate our
> context diff files (or maybe the versions of some software
> involved). You you share how you generate your patch file?

I use git diff with the context-style-diff external helper that's
described in our wiki. It could well be a version-discrepancy
problem... this machine has got git version 1.7.9.6 and diffutils
2.8.1, and I think the latter is pretty old.

regards, tom lane