Re: Remaining beta blockers

The schedule says we're going to wrap 9.3beta1 on Monday, but it doesn't
feel to me like we are anywhere near ready to ship a credible beta.
Of the items on the 9.3 open-items page,
https://wiki.postgresql.org/wiki/PostgreSQL_9.3_Open_Items
there are at least three that seem like absolute drop-dead stop-ship issues:

1. The matviews mess. Changing that will force initdb, more than
likely, so we need it resolved before beta1.

2. The checksum algorithm business. Again, we don't get to tinker with
that anymore once we're in beta.

3. The ProcessUtility restructuring problem. Surely we're not going to
ship a beta with persistent buildfarm failures, which even show up
sometimes on non-CLOBBER_CACHE_ALWAYS animals, eg today at
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=nightjar&dt=2013-04-27%2009%3A27%3A00

Can we get these resolved by Monday, or must we postpone beta?

As far as #1 goes, I think we have little choice at this point but to
remove the unlogged-matviews feature for 9.3. Various alternatives were
kicked around in the "matview scannability rehash" thread but they were
only marginally less klugy, and nobody's stepped up with a patch anyway.
I will undertake to remove unlogged matviews and replace isscannable-
as-a-file-size-property with isscannable-as-a-reloption (unless anyone
feels it would be better as a separate pg_class column?).

I haven't been paying too close attention to the checksum threads
so I'm not sure where we are on #2.

As for #3, there's a draft patch, who's going to take responsibility
for that?

Anything else that's "must fix"?

regards, tom lane

On Sat, Apr 27, 2013 at 10:59:32AM -0400, Tom Lane wrote:
> The schedule says we're going to wrap 9.3beta1 on Monday, but it doesn't
> feel to me like we are anywhere near ready to ship a credible beta.
> Of the items on the 9.3 open-items page,
> https://wiki.postgresql.org/wiki/PostgreSQL_9.3_Open_Items
> there are at least three that seem like absolute drop-dead stop-ship issues:

> 1. The matviews mess. Changing that will force initdb, more than
> likely, so we need it resolved before beta1.

In similar discussions last year, we concluded that forcing initdb after beta
is fine so long as pg_upgrade can handle the change. Any of the proposals for
changing materialized view scannability are easy for pg_upgrade to handle.

> As far as #1 goes, I think we have little choice at this point but to
> remove the unlogged-matviews feature for 9.3. Various alternatives were
> kicked around in the "matview scannability rehash" thread but they were
> only marginally less klugy, and nobody's stepped up with a patch anyway.
> I will undertake to remove unlogged matviews and replace isscannable-
> as-a-file-size-property with isscannable-as-a-reloption (unless anyone
> feels it would be better as a separate pg_class column?).

This perspective is all wrong. I hate to be blunt, but that thread ended with
your technical objections to the committed implementation breaking apart and
sinking. There was no consensus to change it on policy/UI grounds, either.

> 2. The checksum algorithm business. Again, we don't get to tinker with
> that anymore once we're in beta.

Since pg_upgrade isn't in a position to migrate beta clusters to a new
checksum algorithm, I share the desire to settle this sooner rather than
later. However, if finalizing it before beta singularly entails slipping beta
by more than a week or two, I think we should cut the beta without doing so.
Then mention in its release notes that "initdb --data-checksums" beta clusters
may require dump/reload to upgrade to a release or later beta.

> Anything else that's "must fix"?

Not to my knowledge.

nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Sat, Apr 27, 2013 at 10:59:32AM -0400, Tom Lane wrote:
>> As far as #1 goes, I think we have little choice at this point but to
>> remove the unlogged-matviews feature for 9.3.

> This perspective is all wrong. I hate to be blunt, but that thread ended with
> your technical objections to the committed implementation breaking apart and
> sinking. There was no consensus to change it on policy/UI grounds, either.

[ shrug... ] You and Kevin essentially repeated your claims that the
current implementation is OK; nobody else weighed in. I see absolutely
no reason to change my opinion on this. Keeping relisscannable state in
the form of is-the-file-of-nonzero-size is something we WILL regret, and
Pollyanna-ish refusal to believe that is not an adequate reason for
painting ourselves into a corner, especially not for a second-order
feature like unlogged matviews.

The way forward to unlogged matviews that behave the way Kevin wants
is to improve crash recovery so that we can update catalog state after
completing recovery, which is something there are other reasons to want
anyway. But it's far too late to do that in this cycle. In the
meantime I remain convinced that we're better off dropping the feature
until we have an implementation that's not going to bite us in the rear
in the future.

I also note that there are acknowledged-even-by-you bugs in the current
implementation, which no patch has emerged for, so *something's* got to
be done. I'm done waiting for something to happen, and am going to go
fix it in the way I think best.

>> 2. The checksum algorithm business. Again, we don't get to tinker with
>> that anymore once we're in beta.

> Since pg_upgrade isn't in a position to migrate beta clusters to a new
> checksum algorithm, I share the desire to settle this sooner rather than
> later. However, if finalizing it before beta singularly entails slipping beta
> by more than a week or two, I think we should cut the beta without doing so.
> Then mention in its release notes that "initdb --data-checksums" beta clusters
> may require dump/reload to upgrade to a release or later beta.

Meh. If we want to reserve the right to do that, we'd better do
something about putting a checksum algorithm ID in pg_control after all.
Otherwise we'll be faced with not detecting the algorithm change, or
else changing pg_control format post-beta1, which would break beta
clusters for everybody not just those who'd experimented with checksums.

regards, tom lane

On Sat, Apr 27, 2013 at 10:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The schedule says we're going to wrap 9.3beta1 on Monday, but it doesn't
> feel to me like we are anywhere near ready to ship a credible beta.
> Of the items on the 9.3 open-items page,
> https://wiki.postgresql.org/wiki/PostgreSQL_9.3_Open_Items
> there are at least three that seem like absolute drop-dead stop-ship issues:

I completely agree. I think it's considerably premature to wrap a
beta at this point. We haven't resolved the issue of what to do about
accidental restores into pg_catalog either; nobody replied to my last
email on that thread.

> 1. The matviews mess. Changing that will force initdb, more than
> likely, so we need it resolved before beta1.

I would like to rip out the whole notion of whether a materialized
view is scannable and am happy to do that on Monday if you're willing
to sit still for it. I think that's better than failing to support
unlogged relations, and I'm confident that the decision to put the
scannability flag in pg_class rather than the backing file is dead
wrong. At the same time, I *also* agree that using the file size as a
flag is untenable.

> 2. The checksum algorithm business. Again, we don't get to tinker with
> that anymore once we're in beta.

I think it's pretty darn clear that we should change the algorithm,
and I think we've got a patch to do that. So we should be able to
resolve this relatively quickly. But +1 for adding a checksum
algorithm ID to pg_control anyway.

> 3. The ProcessUtility restructuring problem. Surely we're not going to
> ship a beta with persistent buildfarm failures, which even show up
> sometimes on non-CLOBBER_CACHE_ALWAYS animals, eg today at
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=nightjar&dt=2013-04-27%2009%3A27%3A00
>
> As for #3, there's a draft patch, who's going to take responsibility
> for that?

I have been assuming that Alvaro was responsible for fixing this since
he (AIUI) was the one who committed what broke it. If that's not the
case, I should probably jump in, since I committed some earlier event
trigger patches. Or Dimitri, as the original author of said patches.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Apr 27, 2013 at 10:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Of the items on the 9.3 open-items page,
>> https://wiki.postgresql.org/wiki/PostgreSQL_9.3_Open_Items
>> there are at least three that seem like absolute drop-dead stop-ship issues:

> I completely agree. I think it's considerably premature to wrap a
> beta at this point. We haven't resolved the issue of what to do about
> accidental restores into pg_catalog either; nobody replied to my last
> email on that thread.

As far as that item goes, I agree it's must-fix, but I'm not sure it's
must-fix-before-beta.

>> 1. The matviews mess. Changing that will force initdb, more than
>> likely, so we need it resolved before beta1.

> I would like to rip out the whole notion of whether a materialized
> view is scannable and am happy to do that on Monday if you're willing
> to sit still for it.

That would actually be my druthers too; while I see that we're going to
want such a concept eventually, I'm not convinced that the current
feature is a reasonable (and upward-compatible) subset of what we'll
want later. However, when I proposed doing that earlier, Kevin
complained pretty strenuously. I'm willing to yield on the point,
as long as the implementation doesn't make use of storage-file size
to represent scannability.

> I think that's better than failing to support
> unlogged relations, and I'm confident that the decision to put the
> scannability flag in pg_class rather than the backing file is dead
> wrong. At the same time, I *also* agree that using the file size as a
> flag is untenable.

Um, wait, it's *not* in pg_class now, and what I was about to do was
go put it there. Is there a typo in the above para, or are you saying
you don't like either approach? If the latter, what concept have you
got for an eventual implementation?

regards, tom lane

On Sat, Apr 27, 2013 at 3:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 1. The matviews mess. Changing that will force initdb, more than
>>> likely, so we need it resolved before beta1.
>
>> I would like to rip out the whole notion of whether a materialized
>> view is scannable and am happy to do that on Monday if you're willing
>> to sit still for it.
>
> That would actually be my druthers too; while I see that we're going to
> want such a concept eventually, I'm not convinced that the current
> feature is a reasonable (and upward-compatible) subset of what we'll
> want later. However, when I proposed doing that earlier, Kevin
> complained pretty strenuously. I'm willing to yield on the point,
> as long as the implementation doesn't make use of storage-file size
> to represent scannability.
>
>> I think that's better than failing to support
>> unlogged relations, and I'm confident that the decision to put the
>> scannability flag in pg_class rather than the backing file is dead
>> wrong. At the same time, I *also* agree that using the file size as a
>> flag is untenable.
>
> Um, wait, it's *not* in pg_class now, and what I was about to do was
> go put it there. Is there a typo in the above para, or are you saying
> you don't like either approach? If the latter, what concept have you
> got for an eventual implementation?

If we're going to have it at all, I'd like to make it a flag in the
page header on page 0, or maybe have a dedicated metapage that stores
that detail, and perhaps other things.

But really, I'd rather not have it at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Apr 27, 2013 at 3:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Um, wait, it's *not* in pg_class now, and what I was about to do was
>> go put it there. Is there a typo in the above para, or are you saying
>> you don't like either approach? If the latter, what concept have you
>> got for an eventual implementation?

> If we're going to have it at all, I'd like to make it a flag in the
> page header on page 0, or maybe have a dedicated metapage that stores
> that detail, and perhaps other things.

I cannot say that I find that idea attractive; the biggest problem with
it being that updating such a state flag will be nontransactional,
unless we go to a lot of effort to support rollbacks. ISTM that the
scannability property is a perfectly normal relation property and as
such *ought* to be in the pg_class row, or at worst some other catalog
entry. Why do you think differently?

regards, tom lane

On 27 April 2013 20:23, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Apr 27, 2013 at 10:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>> 2. The checksum algorithm business. Again, we don't get to tinker with
>> that anymore once we're in beta.
>
> I think it's pretty darn clear that we should change the algorithm,
> and I think we've got a patch to do that. So we should be able to
> resolve this relatively quickly.

I'll be working on this today.

> But +1 for adding a checksum
> algorithm ID to pg_control anyway.

Yes, seems best.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 27 April 2013 19:06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
>> On Sat, Apr 27, 2013 at 10:59:32AM -0400, Tom Lane wrote:
>>> As far as #1 goes, I think we have little choice at this point but to
>>> remove the unlogged-matviews feature for 9.3.
>
>> This perspective is all wrong. I hate to be blunt, but that thread ended with
>> your technical objections to the committed implementation breaking apart and
>> sinking. There was no consensus to change it on policy/UI grounds, either.
>
> [ shrug... ] You and Kevin essentially repeated your claims that the
> current implementation is OK; nobody else weighed in.

On other patches, one committer objecting to something is seen as
enough of a blocker to require change. That should work in every
direction.

In any case, we shouldn't all need to line up and vote on stuff, its
so timewasting. Of course, we need to sometimes, but only when the
case is put clearly enough that it can be done sensibly, otherwise we
just end up voting ad hominem.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Apr 27, 2013 at 3:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Apr 27, 2013 at 3:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Um, wait, it's *not* in pg_class now, and what I was about to do was
>>> go put it there. Is there a typo in the above para, or are you saying
>>> you don't like either approach? If the latter, what concept have you
>>> got for an eventual implementation?
>
>> If we're going to have it at all, I'd like to make it a flag in the
>> page header on page 0, or maybe have a dedicated metapage that stores
>> that detail, and perhaps other things.
>
> I cannot say that I find that idea attractive; the biggest problem with
> it being that updating such a state flag will be nontransactional,
> unless we go to a lot of effort to support rollbacks. ISTM that the
> scannability property is a perfectly normal relation property and as
> such *ought* to be in the pg_class row, or at worst some other catalog
> entry. Why do you think differently?

Mostly because of the issue with unlogged tables, I suppose. If
you've got a reasonable idea how to do catalog updates on restart,
though, I could probably be convinced to yield to that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Apr 27, 2013 at 3:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I cannot say that I find that idea attractive; the biggest problem with
>> it being that updating such a state flag will be nontransactional,
>> unless we go to a lot of effort to support rollbacks. ISTM that the
>> scannability property is a perfectly normal relation property and as
>> such *ought* to be in the pg_class row, or at worst some other catalog
>> entry. Why do you think differently?

> Mostly because of the issue with unlogged tables, I suppose. If
> you've got a reasonable idea how to do catalog updates on restart,
> though, I could probably be convinced to yield to that.

Well, it's fairly clear *how* to do it: add some more processing that
occurs after we've completed crash replay. We already have some of
that, eg completion of partial splits in btrees, so it's not that much
of a stretch; it's just a lot of code that's not been written yet.

regards, tom lane

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On other patches, one committer objecting to something is seen as
> enough of a blocker to require change. That should work in every
> direction.

The bottom line here is that we have substantial disagreement on how
unlogged matviews should be implemented, and there's no longer enough
time for coming to a resolution that will satisfy everybody. I think
that means we have to pull the feature from 9.3. If it had not yet
been committed it would certainly not be getting in now over multiple
objections.

Given Robert's concerns, it may be that the same should be said for
scannability tracking. I think it's definitely the case that if we
don't have unlogged matviews then the need for system-level tracking
of scannability is greatly decreased. Kevin's already said that he
plans to work on a much more flexible notion of scannability for 9.4,
and I remain concerned that something we do in haste now might not
prove to be a good upward-compatible basis for that redesign.

regards, tom lane

On 28 April 2013 16:55, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On other patches, one committer objecting to something is seen as
>> enough of a blocker to require change. That should work in every
>> direction.
>
> The bottom line here is that we have substantial disagreement on how
> unlogged matviews should be implemented, and there's no longer enough
> time for coming to a resolution that will satisfy everybody. I think
> that means we have to pull the feature from 9.3. If it had not yet
> been committed it would certainly not be getting in now over multiple
> objections.

I've not said much good about Mat Views, that is true, but that was
aimed at not running with it as a headline feature without
qualification. I don't take that as far as thinking the feature should
be pulled completely; there is some good worth having in most things.
Is this issue worth pulling the whole feature on?

> Given Robert's concerns, it may be that the same should be said for
> scannability tracking. I think it's definitely the case that if we
> don't have unlogged matviews then the need for system-level tracking
> of scannability is greatly decreased. Kevin's already said that he
> plans to work on a much more flexible notion of scannability for 9.4,
> and I remain concerned that something we do in haste now might not
> prove to be a good upward-compatible basis for that redesign.

Given that unlogged tables are somewhat volatile, unlogged matviews
wouldn't be missed much AFAICS. We can add that thought as a later
optimisation.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 28 April 2013 16:55, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The bottom line here is that we have substantial disagreement on how
>> unlogged matviews should be implemented, and there's no longer enough
>> time for coming to a resolution that will satisfy everybody. I think
>> that means we have to pull the feature from 9.3. If it had not yet
>> been committed it would certainly not be getting in now over multiple
>> objections.

> I've not said much good about Mat Views, that is true, but that was
> aimed at not running with it as a headline feature without
> qualification. I don't take that as far as thinking the feature should
> be pulled completely; there is some good worth having in most things.
> Is this issue worth pulling the whole feature on?

I think you misread that. I'm only proposing that we remove *unlogged*
matviews, and perhaps scannability tracking for matviews.

regards, tom lane

On 28 April 2013 21:06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On 28 April 2013 16:55, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> The bottom line here is that we have substantial disagreement on how
>>> unlogged matviews should be implemented, and there's no longer enough
>>> time for coming to a resolution that will satisfy everybody. I think
>>> that means we have to pull the feature from 9.3. If it had not yet
>>> been committed it would certainly not be getting in now over multiple
>>> objections.
>
>> I've not said much good about Mat Views, that is true, but that was
>> aimed at not running with it as a headline feature without
>> qualification. I don't take that as far as thinking the feature should
>> be pulled completely; there is some good worth having in most things.
>> Is this issue worth pulling the whole feature on?
>
> I think you misread that. I'm only proposing that we remove *unlogged*
> matviews, and perhaps scannability tracking for matviews.

Happily so.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Apr 28, 2013 at 11:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Apr 27, 2013 at 3:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I cannot say that I find that idea attractive; the biggest problem with
>>> it being that updating such a state flag will be nontransactional,
>>> unless we go to a lot of effort to support rollbacks. ISTM that the
>>> scannability property is a perfectly normal relation property and as
>>> such *ought* to be in the pg_class row, or at worst some other catalog
>>> entry. Why do you think differently?
>
>> Mostly because of the issue with unlogged tables, I suppose. If
>> you've got a reasonable idea how to do catalog updates on restart,
>> though, I could probably be convinced to yield to that.
>
> Well, it's fairly clear *how* to do it: add some more processing that
> occurs after we've completed crash replay. We already have some of
> that, eg completion of partial splits in btrees, so it's not that much
> of a stretch; it's just a lot of code that's not been written yet.

As far as I can see, that would require starting a separate backend
process for every database, and keeping track of which of those
completed their post-recovery work, and disallowing connections to any
given database until the post-recovery work for that database had been
completed. That seems to add quite a few failure modes that we don't
have today, which is why I haven't been much interested in going that
route.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Apr 28, 2013 at 11:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Well, it's fairly clear *how* to do it: add some more processing that
>> occurs after we've completed crash replay. We already have some of
>> that, eg completion of partial splits in btrees, so it's not that much
>> of a stretch; it's just a lot of code that's not been written yet.

> As far as I can see, that would require starting a separate backend
> process for every database, and keeping track of which of those
> completed their post-recovery work, and disallowing connections to any
> given database until the post-recovery work for that database had been
> completed. That seems to add quite a few failure modes that we don't
> have today, which is why I haven't been much interested in going that
> route.

Well, I didn't say it would be easy or quick ;-). But you're presuming
quite a number of design elements that don't seem essential to me.

What I'd be inclined to think about as prerequisite work is fixing
things so that a process could reattach to a new database, after
flushing all its caches. That's a feature that's been requested plenty,
and could have applications in autovacuum or other places besides this,
and would certainly get lots of testing outside of crash recovery.

Having done that, we could imagine that the startup process itself
cycles through all the databases and does the fixup work, rather than
complicating the postmaster logic as suggested above. Potentially this
could replace the filesystem-scan-driven fixup logic that exists in it
now, if it turns out to be faster to search for unlogged tables via a
catalog scan rather than directory scans.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> [ shrug... ]  You and Kevin essentially repeated your claims that
> the current implementation is OK; nobody else weighed in.

Many people weighed in on the need to differentiate between an
empty matview and one which has not been populated.  Many have also
weighed in on the benefits of unlogged matviews.

> I see absolutely no reason to change my opinion on this.  Keeping
> relisscannable state in the form of is-the-file-of-nonzero-size
> is something we WILL regret

... or change in a subsequent major release.  I see no reason why
using this technique now make it harder to do something else later.
Could you elaborate on the technical challenges you see to doing
so?

> Pollyanna-ish refusal to believe that is not an adequate reason
> for painting ourselves into a corner, especially not for a
> second-order feature like unlogged matviews.

I would guess that about half the use-cases for materialized views
will stay with tables in spite of the added hassle, if they have to
degrade performance by adding logging where they currently have
none.

> The way forward to unlogged matviews that behave the way Kevin
> wants is to improve crash recovery so that we can update catalog
> state after completing recovery, which is something there are
> other reasons to want anyway.

That would certainly be for the best.

> But it's far too late to do that in this cycle.

Yes it is.

> In the meantime I remain convinced that we're better off dropping
> the feature until we have an implementation that's not going to
> bite us in the rear in the future.

I haven't caught up with you on how it will do that.

> I also note that there are acknowledged-even-by-you bugs in the
> current implementation, which no patch has emerged for, so
> *something's* got to be done.  I'm done waiting for something to
> happen, and am going to go fix it in the way I think best.

Are you referring to the case where if you refresh a matview with
over 8GB and it winds up empty, that vacuum can make it look
invalid until the next REFRESH?  This is one of many ways that
could be fixed.

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 8a1ffcf..b950b16 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -230,7 +230,13 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
      *
      * Don't even think about it unless we have a shot at releasing a goodly
      * number of pages.  Otherwise, the time taken isn't worth it.
+     *
+     * Leave a populated materialized view with at least one page.
      */
+    if (onerel->rd_rel->relkind == RELKIND_MATVIEW &&
+        vacrelstats->nonempty_pages == 0)
+        vacrelstats->nonempty_pages = 1;
+
     possibly_freeable = vacrelstats->rel_pages - vacrelstats->nonempty_pages;
     if (possibly_freeable > 0 &&
         (possibly_freeable >= REL_TRUNCATE_MINIMUM ||

The hysteria and FUD about using the currently-supported technique
for unlogged tables to implement unlogged matviews are very
discouraging.  And the notion that we would release a matview
feature which allowed false results (in the form of treating a
never-populated matview as a legal empty matview) is truly scary to
me.  If we can't tell the difference between those two things, I
don't think we should be releasing the feature.

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

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Many people weighed in on the need to differentiate between an
> empty matview and one which has not been populated.  Many have also
> weighed in on the benefits of unlogged matviews.

Sure, I think I did that- we should be able to distinguish between those
two cases and unlogged matviews are great.

> > I see absolutely no reason to change my opinion on this.  Keeping
> > relisscannable state in the form of is-the-file-of-nonzero-size
> > is something we WILL regret
>
> ... or change in a subsequent major release.  I see no reason why
> using this technique now make it harder to do something else later.
> Could you elaborate on the technical challenges you see to doing
> so?

I've not followed this all that closely, to be honest, but I tend to
side with Tom on this, making PG depend on the file size for an
attribute of a relation is a bad idea. For one thing, what happens when
an admin figures out that they can 'hack' the system to do what they
want by truncating that file? Or we end up wanting to have that file be
non-zero and considered 'empty' later, but we don't want pg_upgrade
running around touching all of the existing files out there?

> I would guess that about half the use-cases for materialized views
> will stay with tables in spite of the added hassle, if they have to
> degrade performance by adding logging where they currently have
> none.

Life's tough. There's quite a few things that I wish had made it into
9.3 which didn't. One might also point out that a lot of individuals
may be, understandably, cautious about this new feature and not use it
in the first major rev it's released in anyway (I'm talking about
matviews entirely here..). Basically, I don't believe we should be
acting like we've promised unlogged-matviews will be in 9.3 just because
it's been committed.

> > The way forward to unlogged matviews that behave the way Kevin
> > wants is to improve crash recovery so that we can update catalog
> > state after completing recovery, which is something there are
> > other reasons to want anyway.
>
> That would certainly be for the best.

Then let's forgo having this specific feature in 9.3 and implement it
correctly for 9.4.

> The hysteria and FUD about using the currently-supported technique
> for unlogged tables to implement unlogged matviews are very
> discouraging. 

Perhaps I'm all wet here, but it's not obvious to me that what's done
for unlogged tables really equates to what's being done here.

> And the notion that we would release a matview
> feature which allowed false results (in the form of treating a
> never-populated matview as a legal empty matview) is truly scary to
> me.  If we can't tell the difference between those two things, I
> don't think we should be releasing the feature.

I agree that it's important to distinguish the two. I'm not sure that
I've heard anyone explicitly say otherwise..

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> what happens when an admin figures out that they can 'hack' the
> system to do what they want by truncating that file?

If they modified the heap files that way while the server was
running, the results would be somewhat unpredictable.  If they did
it while the server was stopped, starting the server and attempting
to access the matview would generate:

ERROR:  materialized view "matview_name" has not been populated
HINT:  Use the REFRESH MATERIALIZED VIEW command.

> Or we end up wanting to have that file be non-zero and considered
> 'empty' later, but we don't want pg_upgrade running around
> touching all of the existing files out there?

I didn't follow this one; could you restate it, please?

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

>> what happens when an admin figures out that they can 'hack' the
>> system to do what they want by truncating that file?

That can't possibly be a serious objection. DBAs who mess with DB files
are on their own, and should expect unpredictable behavior.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Apr 29, 2013 at 3:34 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> The hysteria and FUD about using the currently-supported technique
> for unlogged tables to implement unlogged matviews are very
> discouraging. And the notion that we would release a matview
> feature which allowed false results (in the form of treating a
> never-populated matview as a legal empty matview) is truly scary to
> me.

I think that labeling other people's concerns as hysteria and FUD is
not something which will advance the debate. I might be
misunderstanding the nature of Tom's concern, but I believe the
concern is not so much that whatever bugs exist now can't be fixed,
but that the use of this mechanism might leave us vulnerable to a
steady stream of future bugs or make it hard to do things which, in
the future, we may want to do.

To take one not-particularly-hypothetical example of that, I was
recently discussing with someone, probably Noah, the idea of adding a
command to pre-extend a relation to a certain number of blocks (and
prevent vacuum from truncating those extra blocks away again). If
you're counting on the number of blocks in the relation to mean
something other than the number of blocks in the relation, there's a
potential conflict there. Now maybe the fact that you've defined 0
to mean non-scannable and 1+ to mean scannable means there's no real
problem with that particular idea... but suppose you'd defined it the
other way around. I don't think it's purely unreasonable to think
that this could conflict with future developments in other areas.

The basic problem here is that the way unlogged tables work is kind of
a crock. I didn't fully realize that at the time I implemented it,
but it's boxed us in a little in terms of trying to extend that
feature; for example, we still don't have a way to convert logged
relations to unlogged relations, or visca versa, and we're not going
to grow a way without some kind of reworking of that mechanism. This
is another aspect of that problem. To solve these problems cleanly,
we need either a relation metapage, or some more state in pg_class.
Short of that, this isn't a scalable solution. Even if you think it's
workable to coax one more bit of state out of the file length, what
will you do when you need a second bit (or say another 32 bits)?

> If we can't tell the difference between those two things, I
> don't think we should be releasing the feature.

So, speaking of hysteria...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> If they modified the heap files that way while the server was
> running, the results would be somewhat unpredictable.  If they did
> it while the server was stopped, starting the server and attempting
> to access the matview would generate:

Right, the point being that they could (ab)use it as a flag to trigger
something to happen. I'd also be worried about failure cases where
files appear to be zero-length.

> > Or we end up wanting to have that file be non-zero and considered
> > 'empty' later, but we don't want pg_upgrade running around
> > touching all of the existing files out there?
>
> I didn't follow this one; could you restate it, please?

Down the road we decide that we shouldn't have any zero-length files
(perhaps due to checksums..?), yet we have to special case around these
mat views and figure out a way to deal with them during pg_upgrade.

Thanks,

Stephen

On Mon, Apr 29, 2013 at 9:44 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
>> If they modified the heap files that way while the server was
>> running, the results would be somewhat unpredictable. If they did
>> it while the server was stopped, starting the server and attempting
>> to access the matview would generate:
>
> Right, the point being that they could (ab)use it as a flag to trigger
> something to happen. I'd also be worried about failure cases where
> files appear to be zero-length.

If you assume that people are going to modify files while the backend
is running, nothing we do anywhere is safe.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> If you assume that people are going to modify files while the backend
> is running, nothing we do anywhere is safe.

I agree that it's a bad idea and that people who do such things deserve
what they get, but that doesn't mean it won't happen when people realize
they can do it.

And, really, it's all fun and games until someone writes a tool to do
it...

Thanks,

Stephen

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

>> The hysteria and FUD about using the currently-supported technique
>> for unlogged tables to implement unlogged matviews are very
>> discouraging.  And the notion that we would release a matview
>> feature which allowed false results (in the form of treating a
>> never-populated matview as a legal empty matview) is truly scary to
>> me.
>
> I think that labeling other people's concerns as hysteria and FUD is
> not something which will advance the debate.

The point of that language is that "charged" language which has
nothing to do with reality is already being thrown around; by all
means let's get back to constructive discussion.  If there is some
particular problem someone sees, I don't think it has been
expressed yet, which makes it impossible to address, unless you
count the assertion that *if* we had chosen a zero-length heap to
represent a table with valid data we *would* have a problem?

Let's look at the "corner" this supposedly paints us into.  If a
later major release creates a better mechanism, current pg_dump and
load will already use it, based on the way matviews are created
empty and REFRESHed by pg_dump.  Worst case, we need to modify the
behavior of pg_dump running with the switch used by pg_upgrade to
use a new ALTER MATERIALIZED VIEW SET (populated); (or whatever
syntax is chosen) -- a command we would probably want at that point
anyway.  I'm not seeing cause for panic here.

What is a real problem or risk with using this mechanism until we
engineer something better?  What problems with converting to a
later major release does anyone see?

>> If we can't tell the difference between those two things, I
>> don't think we should be releasing the feature.
>
> So, speaking of hysteria...

Yeah, I know you don't get it, but as a DBA I would never have
allowed a feature which could quietly generate false results to be
used -- which is exactly what you have without a way to
differentiate these cases.  If it comes down to a choice between a
performance tweak like unlogged matviews and an issue of
correctness of results, I will pick correctness.  Now, as I've
already said, this tweak is significant (I expect it will make
matviews useful in roughly twice as many cases), but it is just a
performance tweak.

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

Kevin,

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> If there is some
> particular problem someone sees, I don't think it has been
> expressed yet, which makes it impossible to address, unless you
> count the assertion that *if* we had chosen a zero-length heap to
> represent a table with valid data we *would* have a problem?

The problem which people see is that this approach isn't a good idea and
will lead down a road to ugly hacks to make things work correctly. It's
not a question about code correctness- it's a question about the design.
Those are two very different things but both need to be considered.

> I'm not seeing cause for panic here.

Speaking to overcharged words, I don't see any 'panic' here but rather a
strong disagreement between individuals over this design. Actually, I'm
not even sure there is disagreement about there being a better design
for this- it sounds like everyone agrees that there is. The question
boils down to "do we ship it with an inferior design, or hold off and do
it right the first time".

> What is a real problem or risk with using this mechanism until we
> engineer something better?  What problems with converting to a
> later major release does anyone see?

Various examples have been articulated and you've worked through
specific high-level designs for how to deal with those, which is great,
but design isn't about just those things which you can forsee and
predict, it's about ensuring flexibility is there for those things that
you don't predict.

> >> If we can't tell the difference between those two things, I
> >> don't think we should be releasing the feature.
> >
> > So, speaking of hysteria...
>
> Yeah, I know you don't get it, but as a DBA I would never have
> allowed a feature which could quietly generate false results to be
> used -- which is exactly what you have without a way to
> differentiate these cases. 

It also happens to be what you can end up having with unlogged tables
already.. We ran into exactly that issue and decided that we'd be
better off not using them (for now anyway) even for things which really
should be just transient data sets.

> If it comes down to a choice between a
> performance tweak like unlogged matviews and an issue of
> correctness of results, I will pick correctness.  Now, as I've
> already said, this tweak is significant (I expect it will make
> matviews useful in roughly twice as many cases), but it is just a
> performance tweak.

And, I think anyway, I agree w/ you (Kevin) here- we should really have
a way to tell the difference between no-data-in-query-result and
never-populated. I'd like more options than just a big ERROR result
happening when it's never been populated, but that's a different
discussion.

Thanks,

Stephen

Could we please stop the ad-hominem stuff from all sides? We want to
solve the issue not to make it bigger.

On 2013-04-30 04:29:26 -0700, Kevin Grittner wrote:
> Let's look at the "corner" this supposedly paints us into.  If a
> later major release creates a better mechanism, current pg_dump and
> load will already use it, based on the way matviews are created
> empty and REFRESHed by pg_dump.  Worst case, we need to modify the
> behavior of pg_dump running with the switch used by pg_upgrade to
> use a new ALTER MATERIALIZED VIEW SET (populated); (or whatever
> syntax is chosen) -- a command we would probably want at that point
> anyway.  I'm not seeing cause for panic here.

I don't think panic is appropriate either, but I think there are some
valid concerns around this.

1) vacuum can truncate the table to an empty relation already if there is
no data returned by the view's query which then leads to the wrong
scannability state.

S1: CREATE MATERIALIZED VIEW matview_empty AS SELECT false WHERE random() < -1;
S2: S2: SELECT * FROM matview_empty; -- works
S1: VACUUM matview_empty;
S2: SELECT * FROM matview_empty; -- still works
S3: SELECT * FROM matview_empty; -- errors out

So we need to make vacuum skip cleaning out the last page. Once we
get incrementally updated matviews there are more situations to get
into this than just a query not returning anything.
I remember this being discussed somewhere already, but couldn't find
it quickly enough.

Imo this is quite an indicator that the idea of using the filesize is
just plain wrong. Adding logic to vacuum not to truncate data because
its a flag for matview scannability is quite the layer violation and
a sign for a bad design decision. Such a hack has already been added
to copy_heap_data(), while not as bad, shows that it is hard to find
all the places where we need to add it.

2) Since we don't have a metapage to represent scannability in 9.3 we
cannot easily use one in 9.4+ without pg_upgrade emptying all
matviews unless we only rely on the catalogs which we currently
cannot. This will either slow down development or make users
unhappy. Alternatively we can add yet another fork, but that has its
price (quite a bit more open files during normal operation, slower
CREATE DATABASE).

This is actually an argument for not releasing matviews without such
an external state. Going from disk-based state to catalog is easy,
the other way round: not so much.

3) Using the filesize as a flag will make other stuff like preallocating
on-disk data in bigger chunks and related things more complicated.

4) doing the check for scannability in the executor imo is a rather bad
idea. Note that we e.g. don't see an error about a matview which
won't be scanned because of constraint exclusion or one-time
filters.

S1: CREATE MATERIALIZED VIEW matview_unit_false AS SELECT false WHERE false WITH NO DATA;
S1: SELECT * FROM matview_unit_false;

You can get there in far more reasonable cases than WHERE false.

5) I have to agree with Kevin that the scannability is an important thing
to track though.

a) we cannot remove support for WITH NO DATA because of pg_dump order
and unlogged relations. So even without unlogged relations the
problem exists although its easier to represent.
b) Just giving wrong responses is bad [tm]. Outdated data is something
completely different (it has existed in that state in the (near)
past) than giving an empty response (might never have been a visible
state, and more likely not so in any reasonably near
past). Consider an application behind a pooler suddently getting
an empty response from a SELECT * FROM unlogged_matview; It won't
notice anything without a unscannable state since it probably
won't even notice the database restart.

Not sure what the consequence of this is. The most reasonable solution
seems to be to introduce a metapage somewhere *now* which sucks, but ...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Apr 30, 2013 at 7:29 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Let's look at the "corner" this supposedly paints us into. If a
> later major release creates a better mechanism, current pg_dump and
> load will already use it, based on the way matviews are created
> empty and REFRESHed by pg_dump. Worst case, we need to modify the
> behavior of pg_dump running with the switch used by pg_upgrade to
> use a new ALTER MATERIALIZED VIEW SET (populated); (or whatever
> syntax is chosen) -- a command we would probably want at that point
> anyway. I'm not seeing cause for panic here.
>
> What is a real problem or risk with using this mechanism until we
> engineer something better? What problems with converting to a
> later major release does anyone see?

Well, it's a pg_upgrade hazard, if nothing else, isn't it?

> Yeah, I know you don't get it, but as a DBA I would never have
> allowed a feature which could quietly generate false results to be
> used -- which is exactly what you have without a way to
> differentiate these cases. If it comes down to a choice between a
> performance tweak like unlogged matviews and an issue of
> correctness of results, I will pick correctness. Now, as I've
> already said, this tweak is significant (I expect it will make
> matviews useful in roughly twice as many cases), but it is just a
> performance tweak.

Sure, I wouldn't allow that either. My point is that I feel that
could be engineered around in user space. If you have a materialized
view which could legitimately be empty (there are many for which that
won't be an issue), then you can either arrange the view definition so
that it isn't (e.g. by including a dummy record that clients can look
for), or you can include a sentinel unlogged table that will contain a
row if and only if materialized views have been refreshed since the
last crash. In the examples I can think of,
indefinitely-stale-but-valid-at-some-point wouldn't be very good
either, so I would anticipate needing to do some engineering around
relative freshness anyway - e.g. keeping a side table that lists the
last-refreshed-time for each matview. Or, maybe I'd wouldn't care
about tracking elapsed time per se, but instead want to track
freshness relative to updates - e.g. set things up so that anyone who
performs an action that would invalidate a row in the materialized
view would also update a row someplace that would allow me to identify
the row as stale. In either case, handling the case where the view is
altogether invalid doesn't seem to add a whole lot of additional
complexity.

Now, I can imagine cases where it does. For example, suppose you have
a cron job (which you trust to work) to refresh your materialized
views every night. Well, that means that you'll never be more than 24
hours stale - but if any of those materialized views are unlogged,
that also means that you could have no data at all for up to 24 hours
following a crash. Not great, because now you need some logic to
handle just that one case that wouldn't be necessary if the DB did it
for you. But I just think it's a judgement call how serious one
thinks that scenario is, vs. any other scenario where a boolean isn't
adequate either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-04-30 04:29:26 -0700, Kevin Grittner wrote:

>> Let's look at the "corner" this supposedly paints us into.  If a
>> later major release creates a better mechanism, current pg_dump and
>> load will already use it, based on the way matviews are created
>> empty and REFRESHed by pg_dump.  Worst case, we need to modify the
>> behavior of pg_dump running with the switch used by pg_upgrade to
>> use a new ALTER MATERIALIZED VIEW SET (populated); (or whatever
>> syntax is chosen) -- a command we would probably want at that point
>> anyway.  I'm not seeing cause for panic here.
>
> I don't think panic is appropriate either, but I think there are some
> valid concerns around this.
>
> 1) vacuum can truncate the table to an empty relation already if there is
>   no data returned by the view's query which then leads to the wrong
>   scannability state.
>
>   S1: CREATE MATERIALIZED VIEW matview_empty AS SELECT false WHERE random() < -1;
>   S2: S2: SELECT * FROM matview_empty; -- works
>   S1: VACUUM matview_empty;
>   S2: SELECT * FROM matview_empty; -- still works
>   S3: SELECT * FROM matview_empty; -- errors out
>
>   So we need to make vacuum skip cleaning out the last page. Once we
>   get incrementally updated matviews there are more situations to get
>   into this than just a query not returning anything.
>   I remember this being discussed somewhere already, but couldn't find
>   it quickly enough.

Yeah, I posted a short patch earlier on this thread that fixes
that.  Nobody has commented on it, and so far I have not committed
anything related to this without posting details and giving ample
opportunity for anyone to comment.  If nobody objects, I can push
that, and this issue is gone.

>   Imo this is quite an indicator that the idea of using the filesize is
>   just plain wrong. Adding logic to vacuum not to truncate data because
>   its a flag for matview scannability is quite the layer violation and
>   a sign for a bad design decision. Such a hack has already been added
>   to copy_heap_data(), while not as bad, shows that it is hard to find
>   all the places where we need to add it.

I agree, but if you review the thread I started with a flag in
pg_class, I tried using the "special" area of the first page of the
heap, and finally wound up with the current technique based on
several people reviewing the patch and offering opinions and
reaching an apparent consensus that this was the least evil way to
do it given current infrastructure for unlogged tables.  This
really is a result of trying to preserver *correct* behavior while
catering to those emphasizing how important the performance of
unlogged matviews is.

> 2) Since we don't have a metapage to represent scannability in 9.3 we
>   cannot easily use one in 9.4+ without pg_upgrade emptying all
>   matviews unless we only rely on the catalogs which we currently
>   cannot.

I am not following this argument at all.  Doesn't pg_upgrade use
pg_dump to create the tables and matviews WITH NO DATA and take
later action for ones which are populated in the source?  It would
not be hard at all to move to a new release which used a different
technique for tracking populated tables by changing what pg_dump
does for populated tables with the switch pg_upgrade uses.

>   This will either slow down development or make users
>   unhappy. Alternatively we can add yet another fork, but that has its
>   price (quite a bit more open files during normal operation, slower
>   CREATE DATABASE).
>
>   This is actually an argument for not releasing matviews without such
>   an external state. Going from disk-based state to catalog is easy,
>   the other way round: not so much.

I am not seeing this at all.  Given my comment above about how this
works for pg_upgrade and pg_dump, can you explain how this is a
problem?

> 3) Using the filesize as a flag will make other stuff like preallocating
>   on-disk data in bigger chunks and related things more complicated.

Why?  You don't need to preallocate for a non-populated matview,
since its heap will be replaced on REFRESH.  You would not *want*
to preallocate a lot of space for something guaranteed to remain at
zero length until deleted.

> 4) doing the check for scannability in the executor imo is a rather bad
>   idea. Note that we e.g. don't see an error about a matview which
>   won't be scanned because of constraint exclusion or one-time
>   filters.
>
>   S1: CREATE MATERIALIZED VIEW matview_unit_false AS SELECT false WHERE false
> WITH NO DATA;
>   S1: SELECT * FROM matview_unit_false;
>
> You can get there in far more reasonable cases than WHERE false.

I am a little concerned about that, but the case you show doesn't demonstrate a problem:

test=# CREATE MATERIALIZED VIEW matview_unit_false AS SELECT false WHERE false WITH NO DATA;
SELECT 0
test=# SELECT * FROM matview_unit_false;
ERROR:  materialized view "matview_unit_false" has not been populated
HINT:  Use the REFRESH MATERIALIZED VIEW command.

The view was defined WITH NO DATA and is behaving correctly for
that case.  When populated (with zero rows) it behaves correctly:

test=# REFRESH materialized view matview_unit_false ;
REFRESH MATERIALIZED VIEW
test=# SELECT * FROM matview_unit_false;
 bool
------
(0 rows)

I would be interested to see whether anyone can come up with an
actual mis-behavior related to this either before or after the
recent refactoring.

> 5) I have to agree with Kevin that the scannability is an important thing
>   to track though.
>
>   a) we cannot remove support for WITH NO DATA because of pg_dump order
>       and unlogged relations. So even without unlogged relations the
>       problem exists although its easier to represent.
>   b) Just giving wrong responses is bad [tm]. Outdated data is something
>       completely different (it has existed in that state in the (near)
>       past) than giving an empty response (might never have been a visible
>       state, and more likely not so in any reasonably near
>       past). Consider an application behind a pooler suddently getting
>       an empty response from a SELECT * FROM unlogged_matview; It won't
>       notice anything without a unscannable state since it probably
>       won't even notice the database restart.
>
> Not sure what the consequence of this is. The most reasonable solution
> seems to be to introduce a metapage somewhere *now* which sucks, but ...

That seems far riskier to me than using the current
lame-but-functional approach now and improving it in a subsequent
release.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

>> Let's look at the "corner" this supposedly paints us into.  If a
>> later major release creates a better mechanism, current pg_dump and
>> load will already use it, based on the way matviews are created
>> empty and REFRESHed by pg_dump.  Worst case, we need to modify the
>> behavior of pg_dump running with the switch used by pg_upgrade to
>> use a new ALTER MATERIALIZED VIEW SET (populated); (or whatever
>> syntax is chosen) -- a command we would probably want at that point
>> anyway.  I'm not seeing cause for panic here.
>>
>> What is a real problem or risk with using this mechanism until we
>> engineer something better?  What problems with converting to a
>> later major release does anyone see?
>
> Well, it's a pg_upgrade hazard, if nothing else, isn't it?

I don't think so.  What do you see as a problem?

> Sure, I wouldn't allow that either.  My point is that I feel that
> could be engineered around in user space.  If you have a materialized
> view which could legitimately be empty (there are many for which that
> won't be an issue), then you can either arrange the view definition so
> that it isn't (e.g. by including a dummy record that clients can look
> for), or you can include a sentinel unlogged table that will contain a
> row if and only if materialized views have been refreshed since the
> last crash.  In the examples I can think of,
> indefinitely-stale-but-valid-at-some-point wouldn't be very good
> either, so I would anticipate needing to do some engineering around
> relative freshness anyway - e.g. keeping a side table that lists the
> last-refreshed-time for each matview.  Or, maybe I'd wouldn't care
> about tracking elapsed time per se, but instead want to track
> freshness relative to updates - e.g. set things up so that anyone who
> performs an action that would invalidate a row in the materialized
> view would also update a row someplace that would allow me to identify
> the row as stale.  In either case, handling the case where the view is
> altogether invalid doesn't seem to add a whole lot of additional
> complexity.
>
> Now, I can imagine cases where it does.  For example, suppose you have
> a cron job (which you trust to work) to refresh your materialized
> views every night.  Well, that means that you'll never be more than 24
> hours stale - but if any of those materialized views are unlogged,
> that also means that you could have no data at all for up to 24 hours
> following a crash.  Not great, because now you need some logic to
> handle just that one case that wouldn't be necessary if the DB did it
> for you.  But I just think it's a judgement call how serious one
> thinks that scenario is, vs. any other scenario where a boolean isn't
> adequate either.

"Staleness" is a completely different issue, in my view, from
quietly returning results that are not, and never were, accurate.
Sure we need to implement more refined "scannability" tests than
whether valid data from *some* point in time is present.  But that
should always be *part* of the scannability testing, and without it
I don't feel we have a feature of a quality suitable for delivery.

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

On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > 1) vacuum can truncate the table to an empty relation already if there is
> >   no data returned by the view's query which then leads to the wrong
> >   scannability state.
> >
> >   S1: CREATE MATERIALIZED VIEW matview_empty AS SELECT false WHERE random() < -1;
> >   S2: S2: SELECT * FROM matview_empty; -- works
> >   S1: VACUUM matview_empty;
> >   S2: SELECT * FROM matview_empty; -- still works
> >   S3: SELECT * FROM matview_empty; -- errors out
> >
> >   So we need to make vacuum skip cleaning out the last page. Once we
> >   get incrementally updated matviews there are more situations to get
> >   into this than just a query not returning anything.
> >   I remember this being discussed somewhere already, but couldn't find
> >   it quickly enough.
>
> Yeah, I posted a short patch earlier on this thread that fixes
> that.  Nobody has commented on it, and so far I have not committed
> anything related to this without posting details and giving ample
> opportunity for anyone to comment.  If nobody objects, I can push
> that, and this issue is gone.

Well, this bug is gone, but the multiple layer violations aren't.

> >   Imo this is quite an indicator that the idea of using the filesize is
> >   just plain wrong. Adding logic to vacuum not to truncate data because
> >   its a flag for matview scannability is quite the layer violation and
> >   a sign for a bad design decision. Such a hack has already been added
> >   to copy_heap_data(), while not as bad, shows that it is hard to find
> >   all the places where we need to add it.
>
> I agree, but if you review the thread I started with a flag in
> pg_class, I tried using the "special" area of the first page of the
> heap, and finally wound up with the current technique based on
> several people reviewing the patch and offering opinions and
> reaching an apparent consensus that this was the least evil way to
> do it given current infrastructure for unlogged tables.  This
> really is a result of trying to preserver *correct* behavior while
> catering to those emphasizing how important the performance of
> unlogged matviews is.

Imo it now uses the worst of both worlds...

> > 2) Since we don't have a metapage to represent scannability in 9.3 we
> >   cannot easily use one in 9.4+ without pg_upgrade emptying all
> >   matviews unless we only rely on the catalogs which we currently
> >   cannot.

> I am not following this argument at all.  Doesn't pg_upgrade use
> pg_dump to create the tables and matviews WITH NO DATA and take
> later action for ones which are populated in the source?  It would
> not be hard at all to move to a new release which used a different
> technique for tracking populated tables by changing what pg_dump
> does for populated tables with the switch pg_upgrade uses.

What I am thinking about is a 100GB materialized view which has been
filled in 9.3 and should now be pg_upgraded into 9.4. If we don't have a
metapage yet and we want to add one we would need to rewrite the whole
100GB which seems like a rather bad idea. Or how are you proposing to
add the metapage? You cannot add it to the end or somesuch.

> I am not seeing this at all.  Given my comment above about how this
> works for pg_upgrade and pg_dump, can you explain how this is a
> problem?

Well, that only works if there is a cheap way to add the new notation to
existing matview heaps which I don't see.

> > 3) Using the filesize as a flag will make other stuff like preallocating
> >   on-disk data in bigger chunks and related things more complicated.
>
> Why?  You don't need to preallocate for a non-populated matview,
> since its heap will be replaced on REFRESH.  You would not *want*
> to preallocate a lot of space for something guaranteed to remain at
> zero length until deleted.

But we would likely also want to optimize reducing the filesize in the
same chunks because you otherwise would end up with even worse
fragmentation. And I am not talking about an unscannable relation but
about one which is currently empty (e.g. SELECT ... WHERE false).

> > 4) doing the check for scannability in the executor imo is a rather bad
> >   idea. Note that we e.g. don't see an error about a matview which
> >   won't be scanned because of constraint exclusion or one-time
> >   filters.
> >
> >   S1: CREATE MATERIALIZED VIEW matview_unit_false AS SELECT false WHERE false
> > WITH NO DATA;
> >   S1: SELECT * FROM matview_unit_false;
> >
> > You can get there in far more reasonable cases than WHERE false.
>
> I am a little concerned about that, but the case you show doesn't demonstrate a problem:

> The view was defined WITH NO DATA and is behaving correctly for
> that case.  When populated (with zero rows) it behaves correctly:

Ah. Tom already fixed the problem in
5194024d72f33fb209e10f9ab0ada7cc67df45b7. I was working in a branch
based on 3ccae48f44d993351e1f881761bd6c556ebd6638 and it failed there.

> I would be interested to see whether anyone can come up with an
> actual mis-behavior related to this either before or after the
> recent refactoring.

Seems ok after Tom's refactoring.

> > Not sure what the consequence of this is. The most reasonable solution
> > seems to be to introduce a metapage somewhere *now* which sucks, but ...
>
> That seems far riskier to me than using the current
> lame-but-functional approach now and improving it in a subsequent
> release.

Why? We have bitten by the lack of such metapages several times now and
I don't think we have bitten by their presence in relation types that
have them by now.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

>>> 2) Since we don't have a metapage to represent scannability in 9.3
>>>    we cannot easily use one in 9.4+ without pg_upgrade emptying all
>>>    matviews unless we only rely on the catalogs which we currently
>>>    cannot.
>
>> I am not following this argument at all.  Doesn't pg_upgrade use
>> pg_dump to create the tables and matviews WITH NO DATA and take
>> later action for ones which are populated in the source?  It would
>> not be hard at all to move to a new release which used a different
>> technique for tracking populated tables by changing what pg_dump
>> does for populated tables with the switch pg_upgrade uses.
>
> What I am thinking about is a 100GB materialized view which has been
> filled in 9.3 and should now be pg_upgraded into 9.4. If we don't have a
> metapage yet and we want to add one we would need to rewrite the whole
> 100GB which seems like a rather bad idea. Or how are you proposing to
> add the metapage? You cannot add it to the end or somesuch.

Oh, you are suggesting prepending a metapage to existing matviews
(and tables?)?  So to check whether a view has been populated you
not only look at the directory but open the file and read a page?
Now I follow why you think this would be an issue.  I'm not sure I
think that is the best solution, though.  In what way would it be
better than adding info to pg_class or some other system table?
Why would this be important for unlogged matviews but not unlogged
tables?

>> I am not seeing this at all.  Given my comment above about how this
>> works for pg_upgrade and pg_dump, can you explain how this is a
>> problem?
>
> Well, that only works if there is a cheap way to add the new notation to
> existing matview heaps which I don't see.

We could perhaps reserve some space in the special area of the
first page, if we can agree on a generous enough amount of space.

>>> 3) Using the filesize as a flag will make other stuff like
>>>    preallocating on-disk data in bigger chunks and related
>>>    things more complicated.
>>
>> Why?  You don't need to preallocate for a non-populated matview,
>> since its heap will be replaced on REFRESH.  You would not *want*
>> to preallocate a lot of space for something guaranteed to remain at
>> zero length until deleted.
>
> But we would likely also want to optimize reducing the filesize in the
> same chunks because you otherwise would end up with even worse
> fragmentation. And I am not talking about an unscannable relation but
> about one which is currently empty (e.g. SELECT ... WHERE false).

Yes, if we get to both incremental updates *and* preallocations
before we develop a better technique for this, a special case would
be needed for the case where a matview was incrementally updated to
zero rows.

>>> Not sure what the consequence of this is. The most reasonable solution
>>> seems to be to introduce a metapage somewhere *now* which sucks, but
> ...
>>
>> That seems far riskier to me than using the current
>> lame-but-functional approach now and improving it in a subsequent
>> release.
>
> Why? We have bitten by the lack of such metapages several times now and
> I don't think we have bitten by their presence in relation types that
> have them by now.

Like I said, months ago I had a version which used the special area
for the first page of a matview heap, but was convinced to change
that.  I could probably be convinced to change back.  :-)  I don't
know whether you see a problem with using the special like that for
the metadata you envision.

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

On Tue, Apr 30, 2013 at 10:40 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>>> What is a real problem or risk with using this mechanism until we
>>> engineer something better? What problems with converting to a
>>> later major release does anyone see?
>>
>> Well, it's a pg_upgrade hazard, if nothing else, isn't it?
>
> I don't think so. What do you see as a problem?

pg_upgrade only handles changes in catalog state, not on-disk
representation. If the on-disk representation of an non-scannable
view might change in a future release, it's a pg_upgrade hazard.

>> Sure, I wouldn't allow that either. My point is that I feel that
>> could be engineered around in user space. If you have a materialized
>> view which could legitimately be empty (there are many for which that
>> won't be an issue), then you can either arrange the view definition so
>> that it isn't (e.g. by including a dummy record that clients can look
>> for), or you can include a sentinel unlogged table that will contain a
>> row if and only if materialized views have been refreshed since the
>> last crash. In the examples I can think of,
>> indefinitely-stale-but-valid-at-some-point wouldn't be very good
>> either, so I would anticipate needing to do some engineering around
>> relative freshness anyway - e.g. keeping a side table that lists the
>> last-refreshed-time for each matview. Or, maybe I'd wouldn't care
>> about tracking elapsed time per se, but instead want to track
>> freshness relative to updates - e.g. set things up so that anyone who
>> performs an action that would invalidate a row in the materialized
>> view would also update a row someplace that would allow me to identify
>> the row as stale. In either case, handling the case where the view is
>> altogether invalid doesn't seem to add a whole lot of additional
>> complexity.
>>
>> Now, I can imagine cases where it does. For example, suppose you have
>> a cron job (which you trust to work) to refresh your materialized
>> views every night. Well, that means that you'll never be more than 24
>> hours stale - but if any of those materialized views are unlogged,
>> that also means that you could have no data at all for up to 24 hours
>> following a crash. Not great, because now you need some logic to
>> handle just that one case that wouldn't be necessary if the DB did it
>> for you. But I just think it's a judgement call how serious one
>> thinks that scenario is, vs. any other scenario where a boolean isn't
>> adequate either.
>
> "Staleness" is a completely different issue, in my view, from
> quietly returning results that are not, and never were, accurate.
> Sure we need to implement more refined "scannability" tests than
> whether valid data from *some* point in time is present. But that
> should always be *part* of the scannability testing, and without it
> I don't feel we have a feature of a quality suitable for delivery.

I don't really see these as being all that separate. The user is
going to want to know whether the data is usable or not. The answer
to that question depends on the user's business rules, and those could
be anything. The current system implements the following business
rule: "The data can be used if the matview has ever been populated."
But there are lots of other possibilities:

- The data can be used if the matview is fully up-to-date.
- The data can be used if the matview is not out of date by more than
a certain amount of time.
- The data can be used if the matview is out of date with respect to
one of its base tables, but not if it is out of date with respect to
another of its base tables. For example, maybe you're OK with using
an order-summary view if new orders have arrived (but not if the view
is more than a day out of date); but not if any customers have been
renamed.

Not only can the business rules vary, but they can vary between
queries. Query A might need an exact answer, hence can only use the
matview when its up to date. Query B against the very same matview
might only need data that's up to date within some time threshold, and
query C might well want to just use whatever data exists. I'm not at
all clear that it's even feasible to solve all of these problems
inside the database; my suspicion is that at least some of these
things are going to end up being things that have to be handled at
least partially in user-space, while others will be handled in the DB
through mechanisms not yet designed.

I am even willing to go so far as to say that I am unconvinced that
everyone will want a just-truncated materialized view to be
automatically set to non-scannable. Suppose you have an unlogged view
that summarizes a real-time data stream - all SMTP traps received in
the last minute summarized by the node from which they were received.
When the server reboots unexpectedly, the raw data is lost along with
the summary. But for the scannability flag, we'd read the summary as
empty, which would be right. Adding the scannability flag forces the
user to add application logic to treat the error as equivalent to an
empty view.

The real concern I have about using an empty page to differentiate
whether or not the view is scannable is that I think it's a modularity
violation. But on the value of the underlying feature, my concern is
that there are an essentially infinite number of possible business
rules here that someone might want to implement, and I don't think
it's the job of a materialized view, either now or in the future, to
lock the user into any particular set of policy decisions.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-04-30 08:35:32 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote:
> >> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> >>> 2) Since we don't have a metapage to represent scannability in 9.3
> >>>    we cannot easily use one in 9.4+ without pg_upgrade emptying all
> >>>    matviews unless we only rely on the catalogs which we currently
> >>>    cannot.
> >
> >> I am not following this argument at all.  Doesn't pg_upgrade use
> >> pg_dump to create the tables and matviews WITH NO DATA and take
> >> later action for ones which are populated in the source?  It would
> >> not be hard at all to move to a new release which used a different
> >> technique for tracking populated tables by changing what pg_dump
> >> does for populated tables with the switch pg_upgrade uses.
> >
> > What I am thinking about is a 100GB materialized view which has been
> > filled in 9.3 and should now be pg_upgraded into 9.4. If we don't have a
> > metapage yet and we want to add one we would need to rewrite the whole
> > 100GB which seems like a rather bad idea. Or how are you proposing to
> > add the metapage? You cannot add it to the end or somesuch.
>
> Oh, you are suggesting prepending a metapage to existing matviews
> (and tables?)?  So to check whether a view has been populated you
> not only look at the directory but open the file and read a page?
> Now I follow why you think this would be an issue.  I'm not sure I
> think that is the best solution, though.  In what way would it be
> better than adding info to pg_class or some other system table?

You can write/read it without having a database opened. Like in the
startup process.

Then you can abstract away the knowledge about that into
PageIsMetapage(relation, blockno) or such which then can be used by
vacuum, heapam et al in an extensible fashion.

This is far from a fully working solution though - you still have issues
with BEGIN;REFRESH ...; ROLLBACK; if you do it naively. Afair that was
what made Tom protest against this.

> Why would this be important for unlogged matviews but not unlogged
> tables?

Unlogged tables basically have some very raw version of this - the _init
fork. But yes, a more generic version would have been nice.

> >> I am not seeing this at all.  Given my comment above about how this
> >> works for pg_upgrade and pg_dump, can you explain how this is a
> >> problem?
> >
> > Well, that only works if there is a cheap way to add the new notation to
> > existing matview heaps which I don't see.
>
> We could perhaps reserve some space in the special area of the
> first page, if we can agree on a generous enough amount of space.

If we do it we should just use a whole page, no point in being to
cautious there imo.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert,

> - The data can be used if the matview is fully up-to-date.
> - The data can be used if the matview is not out of date by more than
> a certain amount of time.
> - The data can be used if the matview is out of date with respect to
> one of its base tables, but not if it is out of date with respect to
> another of its base tables. For example, maybe you're OK with using
> an order-summary view if new orders have arrived (but not if the view
> is more than a day out of date); but not if any customers have been
> renamed.

We discussed this around the beginning of CF4. For some reason (which I
didn't quite understand at the time), the consensus was that creating a
"matview last updated" timestamp was not implementable for 9.3 and would
need to wait for 9.4.

Again, all of this points to additional columns, or at least relopts, in
pg_class. Why is that off the table?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> We discussed this around the beginning of CF4.  For some reason
> (which I didn't quite understand at the time), the consensus was
> that creating a "matview last updated" timestamp was not
> implementable for 9.3 and would need to wait for 9.4.

The reason was that the start of CF4 was deemed too late in the
development cycle to be trying to design what that should look
like.  No sooner had you suggested that one column than someone
suggested two others which might also be useful, and it seemed to
me that it would be hard to get it right before we had a better
sense of what incremental maintenance based on the view declaration
would look like.  So, as I recall it, the consensus developed
around the simplest possible test, which from the user perspective
was hidden behind a function, should be used for this release.

> Again, all of this points to additional columns, or at least
> relopts, in pg_class.  Why is that off the table?

That was deemed to be incompatible with unlogged matviews, which
some didn't want to give up in this initial release.

Basically, what this patch aims at is more or less what some other
databases had in their initial releases of materialized views 10 to
20 years ago.  Other products have built on those foundations with
each major release.  I was hoping we could do the same.  We are not
going to reach parity on this with any other major database product
in one release, or probably even two or three.

I didn't dig back through all the threads to confirm these
recollections, so take them with a grain of salt.  The one thing I
am sure of is that they are a rehash of discussions which go back
to before the start of CF3.  Only with more tone and urgency.

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

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> Ah. Tom already fixed the problem in
> 5194024d72f33fb209e10f9ab0ada7cc67df45b7. I was working in a
> branch based on 3ccae48f44d993351e1f881761bd6c556ebd6638 and it
> failed there.

Since previous regression tests missed this bug, I've added the
test you posted, to make sure it doesn't come back unnoticed.

Thanks for the test case!

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

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

>>> 1) vacuum can truncate the table to an empty relation already
>>>    if there is no data returned by the view's query which then
>>>    leads to the wrong scannability state.

>>>   So we need to make vacuum skip cleaning out the last page.
>>>   Once we get incrementally updated matviews there are more
>>>   situations to get into this than just a query not returning
>>>   anything.

>> Yeah, I posted a short patch earlier on this thread that fixes
>> that.  Nobody has commented on it, and so far I have not
>> committed anything related to this without posting details and
>> giving ample opportunity for anyone to comment.  If nobody
>> objects, I can push that, and this issue is gone.
>
> Well, this bug is gone, but the multiple layer violations aren't.

Attached is a patch with regression test if we don't obviate the
need for it by tracking the populated status in a different way or
allowing unpopulated matviews to be used in queries.  I'll hold off
on pushing it until we decide.

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

Kevin,

> The reason was that the start of CF4 was deemed too late in the
> development cycle to be trying to design what that should look
> like. No sooner had you suggested that one column than someone
> suggested two others which might also be useful, and it seemed to

Yeah, I'm just pointing out that we *already had* this discussion, so
there isn't any point in having it again.

> That was deemed to be incompatible with unlogged matviews, which
> some didn't want to give up in this initial release.

Huh? Unlogged tables don't go in pg_class?

> Basically, what this patch aims at is more or less what some other
> databases had in their initial releases of materialized views 10 to
> 20 years ago. Other products have built on those foundations with
> each major release. I was hoping we could do the same. We are not
> going to reach parity on this with any other major database product
> in one release, or probably even two or three.

Yep.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> wrote:

>> That was deemed to be incompatible with unlogged matviews, which
>> some didn't want to give up in this initial release.
>
> Huh?  Unlogged tables don't go in pg_class?

Sorry if I wasn't clear.  I try to do incremental development --
get one part working and then go on to the next.  I had stored the
"populated" state in pg_class, although under what, in retrospect,
was a bad name -- I had a bool called relisvalid.  It should have
been relispopulated or a bit in a flag byte.  Anyway, as I
proceeded to implement the unlogged feature I discovered that the
heap is truncated at recovery time based on the init fork, before
the server is at the point where we can access the system tables.
So then I tried to keep the pg_class state but populate it when an
unlogged matview was opened, based on using the "special" space in
the first page of the init fork, updating pg_class if that was
found to indicate a truncated heap.  I was roundly criticized for
"keeping this state in two places" -- both the heap and pg_class,
and urged to keep it only in the heap, because only keeping it in
pg_class would not allow the proper recovery for an unlogged
matview.  So I responded to that feedback with the current
implementation.

Clearly we would need to change how we do recovery of unlogged
relations to both allow unlogged matviews and keep the populated
state in pg_class.  I don't think we can really make the "two
places" technique work, where the recovery state of the unlogged
matview is used to trigger a pg_class change.  For one thing, it
was really messy -- far more so than current code.  For another
thing, the matview would show as good until it was first opened,
which was nasty.

Kinda verbose, but I hope that makes it more clear.

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

On Tue, Apr 30, 2013 at 10:19 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Clearly we would need to change how we do recovery of unlogged
> relations to both allow unlogged matviews and keep the populated
> state in pg_class. I don't think we can really make the "two
> places" technique work, where the recovery state of the unlogged
> matview is used to trigger a pg_class change. For one thing, it
> was really messy -- far more so than current code. For another
> thing, the matview would show as good until it was first opened,
> which was nasty.

Can I ask what is the design goal of unlogged relations? Are they just
an optimization so you can load lots of data without generating piles
of wal log? Or are they intended to generate zero wal traffic so they
can be populated on a standby for example, or outside a transaction
entirely?

Because if it's the former then I don't see any problem generating wal
traffic to update the pg_class entry. If they can satisfy the latter
that gets a bit trickier.

--
greg

On 27 April 2013 15:59, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> 2. The checksum algorithm business. Again, we don't get to tinker with
> that anymore once we're in beta.

Checksum changes to output value and control file are now complete and
we are ready to go to beta with it.

Robert has an outstanding comment on vismap handling, but thats not to
anything that will cause a re-initdb. There are no further tinkerings
planned, only bug fixes if and when they occur.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Apr 30, 2013 at 9:23 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> Can I ask what is the design goal of unlogged relations? Are they just
> an optimization so you can load lots of data without generating piles
> of wal log? Or are they intended to generate zero wal traffic so they
> can be populated on a standby for example, or outside a transaction
> entirely?

Unlogged relations don't generate any WAL traffic on inserts, updates,
deletes, or tuple locks. So they're not just for bulk-loading;
indeed, for bulk-loading, you can often avoid WAL without an unlogged
relation, if you can arrange to create or truncate the table in the
same transaction that does the load. They cannot be populated on a
standby or outside a transaction because they still require XIDs. If
someone can solve the problem of allowing multiple XID spaces, though,
they might be usable on a standby, which would be pretty cool.

> Because if it's the former then I don't see any problem generating wal
> traffic to update the pg_class entry. If they can satisfy the latter
> that gets a bit trickier.

The problem is that you need to update the pg_class entry in response
to the crash. The startup process has no ability to go hunt down the
pg_class record in the relevant database and update it - it only
understands pages, not tuples.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 30, 2013 at 12:02:59PM -0400, Robert Haas wrote:
> On Tue, Apr 30, 2013 at 10:40 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> >>> What is a real problem or risk with using this mechanism until we
> >>> engineer something better? What problems with converting to a
> >>> later major release does anyone see?
> >>
> >> Well, it's a pg_upgrade hazard, if nothing else, isn't it?
> >
> > I don't think so. What do you see as a problem?
>
> pg_upgrade only handles changes in catalog state, not on-disk
> representation. If the on-disk representation of an non-scannable
> view might change in a future release, it's a pg_upgrade hazard.

Yes, pg_upgrade is never going to write to data pages as that would be
slow and prevent the ability to roll back to the previous cluster on
error.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>>>>> What is a real problem or risk with using this mechanism
>>>>> until we engineer something better?  What problems with
>>>>> converting to a later major release does anyone see?
>>>>
>>>> Well, it's a pg_upgrade hazard, if nothing else, isn't it?
>>>
>>> I don't think so.  What do you see as a problem?
>>
>> pg_upgrade only handles changes in catalog state, not on-disk
>> representation.  If the on-disk representation of an
>> non-scannable view might change in a future release, it's a
>> pg_upgrade hazard.
>
> Yes, pg_upgrade is never going to write to data pages as that
> would be slow and prevent the ability to roll back to the
> previous cluster on error.

The only person who has suggested anything which would require that
is Andres, who suggests adding a metadata page to the front of the
heap to store information on whether the matview is populated.  I
think it is the direct opposite of what Tom is suggesting, and has
too many issues to be considered at this time.

Nobody has proposed how the technique currently used creates a
pg_upgrade hazard now or in some future release where we provide a
way for recovery to put the information into the catalog.  I have
gone into more detail on this earlier on this thread.

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

On Thu, May 2, 2013 at 02:20:15PM -0700, Kevin Grittner wrote:
> > Yes, pg_upgrade is never going to write to data pages as that
> > would be slow and prevent the ability to roll back to the
> > previous cluster on error.
>
> The only person who has suggested anything which would require that
> is Andres, who suggests adding a metadata page to the front of the
> heap to store information on whether the matview is populated.  I
> think it is the direct opposite of what Tom is suggesting, and has
> too many issues to be considered at this time.
>
> Nobody has proposed how the technique currently used creates a
> pg_upgrade hazard now or in some future release where we provide a
> way for recovery to put the information into the catalog.  I have
> gone into more detail on this earlier on this thread.

I was more thinking of the idea of having some status on the first page
that might need to change in a future release.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, May 3, 2013 at 1:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I was more thinking of the idea of having some status on the first page
> that might need to change in a future release.

Incidentally, another option might be to have a <relfilenode>.meta
fork that has information like this. It doesn't fundamentally change
anything but it does mean that code that doesn't need to know about it
doesn't need to know to skip the first page. It also means we could
maybe expand it more easily. There have been previous wishlist items
to have some meta information so external tools can more easily parse
the data without needing access to the full catalog for example.

--
greg

On 2013-05-03 16:11:13 +0100, Greg Stark wrote:
> On Fri, May 3, 2013 at 1:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > I was more thinking of the idea of having some status on the first page
> > that might need to change in a future release.
>
> Incidentally, another option might be to have a <relfilenode>.meta
> fork that has information like this. It doesn't fundamentally change
> anything but it does mean that code that doesn't need to know about it
> doesn't need to know to skip the first page. It also means we could
> maybe expand it more easily. There have been previous wishlist items
> to have some meta information so external tools can more easily parse
> the data without needing access to the full catalog for example.

The problem with an extra metadata fork is that it essentially would
double the files in a cluster and it would also noticeably increase the
amount of open files we need.
There have been quite some complaints about CREATE DATABASE speed, I
am not sure we want to make it even slower :(

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, May 3, 2013 at 05:24:54PM +0200, Andres Freund wrote:
> On 2013-05-03 16:11:13 +0100, Greg Stark wrote:
> > On Fri, May 3, 2013 at 1:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > > I was more thinking of the idea of having some status on the first page
> > > that might need to change in a future release.
> >
> > Incidentally, another option might be to have a <relfilenode>.meta
> > fork that has information like this. It doesn't fundamentally change
> > anything but it does mean that code that doesn't need to know about it
> > doesn't need to know to skip the first page. It also means we could
> > maybe expand it more easily. There have been previous wishlist items
> > to have some meta information so external tools can more easily parse
> > the data without needing access to the full catalog for example.
>
> The problem with an extra metadata fork is that it essentially would
> double the files in a cluster and it would also noticeably increase the
> amount of open files we need.
> There have been quite some complaints about CREATE DATABASE speed, I
> am not sure we want to make it even slower :(

Agreed. We start to get into file system performance issues at that
point. I have often wondered if we need to create hash the files into
subdirectories for databases with many tables. Has anyone profiled
this?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> The problem with an extra metadata fork is that it essentially would
> double the files in a cluster and it would also noticeably increase the
> amount of open files we need.

Why would we need it for every relation? We have other forks (fsm, vm),
but they exist when needed.

I'm more concerned about moving information which really should be in
the system catalogs out into magic files on disk..

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I'm more concerned about moving information which really should be in
> the system catalogs out into magic files on disk..

Right. The whole thing is just a kluge, which I'm convinced we'll
regret sooner or later --- probably sooner. I would much rather drop
unlogged matviews for now and put scannability status into pg_class
where it belongs.

regards, tom lane

Bruce Momjian escribió:
> On Fri, May 3, 2013 at 05:24:54PM +0200, Andres Freund wrote:

> > The problem with an extra metadata fork is that it essentially would
> > double the files in a cluster and it would also noticeably increase the
> > amount of open files we need.
> > There have been quite some complaints about CREATE DATABASE speed, I
> > am not sure we want to make it even slower :(
>
> Agreed. We start to get into file system performance issues at that
> point. I have often wondered if we need to create hash the files into
> subdirectories for databases with many tables. Has anyone profiled
> this?

Modern filesystems use trees to store file nowadays, not linear arrays,
and so that old scenario (long time to find one specific directory entry
within a directory containing lots of files) is no longer that serious a
problem.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-05-03 12:15:27 -0400, Alvaro Herrera wrote:
> Bruce Momjian escribió:
> > On Fri, May 3, 2013 at 05:24:54PM +0200, Andres Freund wrote:
>
> > > The problem with an extra metadata fork is that it essentially would
> > > double the files in a cluster and it would also noticeably increase the
> > > amount of open files we need.
> > > There have been quite some complaints about CREATE DATABASE speed, I
> > > am not sure we want to make it even slower :(
> >
> > Agreed. We start to get into file system performance issues at that
> > point. I have often wondered if we need to create hash the files into
> > subdirectories for databases with many tables. Has anyone profiled
> > this?
>
> Modern filesystems use trees to store file nowadays, not linear arrays,
> and so that old scenario (long time to find one specific directory entry
> within a directory containing lots of files) is no longer that serious a
> problem.

Absolutely. Also, we normally shouldn't open/close files constantly
since we cache open files so open(2) performance shouldn't be *that*
critical.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, May 3, 2013 at 12:10:14PM -0400, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I'm more concerned about moving information which really should be in
> > the system catalogs out into magic files on disk..
>
> Right. The whole thing is just a kluge, which I'm convinced we'll
> regret sooner or later --- probably sooner. I would much rather drop
> unlogged matviews for now and put scannability status into pg_class
> where it belongs.

Right. I think the big question is whether we can add crash recovery
system catalog writes in the time before beta, and whether we would need
that even if we remove unlogged table support.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 2013-05-03 12:10:14 -0400, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I'm more concerned about moving information which really should be in
> > the system catalogs out into magic files on disk..
>
> Right. The whole thing is just a kluge, which I'm convinced we'll
> regret sooner or later --- probably sooner.

I tentatively agree as well. The only argument for introducing some
additional location for such information is that it would be the start
of an infrastructure for information we would need for incrementally
adding checksums, page upgrades and such.

> I would much rather drop
> unlogged matviews for now and put scannability status into pg_class
> where it belongs.

I have no problem with that. And getting to the point were we can switch
databases in a backend in at least a restricted environment is a good
thing.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-05-03 12:10:14 -0400, Tom Lane wrote:
>> Right. The whole thing is just a kluge, which I'm convinced we'll
>> regret sooner or later --- probably sooner.

> I tentatively agree as well. The only argument for introducing some
> additional location for such information is that it would be the start
> of an infrastructure for information we would need for incrementally
> adding checksums, page upgrades and such.

It's possible that a metadata fork would be a good design for such
stuff, but I'd want to see a pretty completely worked-out design before
committing to the idea. In any case we're way too late in the 9.3 cycle
to be considering something like that right now.

regards, tom lane

On Fri, May 3, 2013 at 12:45:36PM -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-05-03 12:10:14 -0400, Tom Lane wrote:
> >> Right. The whole thing is just a kluge, which I'm convinced we'll
> >> regret sooner or later --- probably sooner.
>
> > I tentatively agree as well. The only argument for introducing some
> > additional location for such information is that it would be the start
> > of an infrastructure for information we would need for incrementally
> > adding checksums, page upgrades and such.
>
> It's possible that a metadata fork would be a good design for such
> stuff, but I'd want to see a pretty completely worked-out design before
> committing to the idea. In any case we're way too late in the 9.3 cycle
> to be considering something like that right now.

Yes, I think the big question is how much information do we want per
relation that we don't need in the system tables.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, May 3, 2013 at 5:49 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Yes, I think the big question is how much information do we want per
> relation that we don't need in the system tables.

It's not that we don't need it in the system tables. It's that there's
some state that we *can't* have in the system tables because we need
it to be accessible without access to the catalog or we need to be
able to change it on a standby.

But note that this all sounds very similar to the global temp table
discussion a while ago. I think we're gong to need some infrastructure
for table state that isn't transactional and it will make sense to
solve that with something general that we can then depend on for lots
of things. If I had to guess it would look more like a cached copy of
the pg_class row or the whole relcache entry rather than an entirely
independent structure.

--
greg

On Sat, May 4, 2013 at 03:04:33AM +0100, Greg Stark wrote:
> On Fri, May 3, 2013 at 5:49 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Yes, I think the big question is how much information do we want per
> > relation that we don't need in the system tables.
>
> It's not that we don't need it in the system tables. It's that there's
> some state that we *can't* have in the system tables because we need
> it to be accessible without access to the catalog or we need to be
> able to change it on a standby.
>
> But note that this all sounds very similar to the global temp table
> discussion a while ago. I think we're gong to need some infrastructure
> for table state that isn't transactional and it will make sense to
> solve that with something general that we can then depend on for lots
> of things. If I had to guess it would look more like a cached copy of
> the pg_class row or the whole relcache entry rather than an entirely
> independent structure.

Well, the big question is whether this state _eventually_ will need to
be accessible from SQL, so we might as well add code to allow crash
recovery to write to system tables.

Things like how fresh the materialized view is certainly should be
accessible via SQL and transactional.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, May 3, 2013 at 10:19:42PM -0400, Bruce Momjian wrote:
> On Sat, May 4, 2013 at 03:04:33AM +0100, Greg Stark wrote:
> > On Fri, May 3, 2013 at 5:49 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > > Yes, I think the big question is how much information do we want per
> > > relation that we don't need in the system tables.
> >
> > It's not that we don't need it in the system tables. It's that there's
> > some state that we *can't* have in the system tables because we need
> > it to be accessible without access to the catalog or we need to be
> > able to change it on a standby.
> >
> > But note that this all sounds very similar to the global temp table
> > discussion a while ago. I think we're gong to need some infrastructure
> > for table state that isn't transactional and it will make sense to
> > solve that with something general that we can then depend on for lots
> > of things. If I had to guess it would look more like a cached copy of
> > the pg_class row or the whole relcache entry rather than an entirely
> > independent structure.
>
> Well, the big question is whether this state _eventually_ will need to
> be accessible from SQL, so we might as well add code to allow crash
> recovery to write to system tables.
>
> Things like how fresh the materialized view is certainly should be
> accessible via SQL and transactional.

OK, how are we for bata packaging on Monday? I don't see how we can do
that until we decide on how to handle unlogged materialized views.

Can someone summarize what we have considered for a meta-page as the
first page in the past? Have they always been things we couldn't
recording in the catalogs, or things we didn't want in the catalogs,
e.g. visibility/hint bits?

If we would eventually want the materialized information in the system
catalogs rather than on the first heap page, or recorded as the size of
the help file, I suggest we just remove anything that depends on it and
move to beta.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> OK, how are we for bata packaging on Monday? I don't see how we can do
> that until we decide on how to handle unlogged materialized views.

In the interests of moving the discussion along, attached are draft
patches to show what I think we should do in 9.3. The first patch
disables the unlogged-matviews feature at the user level (and perhaps
could be reverted someday), while the second one moves
scannability-state storage into a pg_class column.

I had previously proposed that we keep scannability state in reloptions,
but that turns out not to be terribly easy because the relcache doesn't
store the reloptions part of the pg_class row; so this patch does it
with a new boolean column instead.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> In the interests of moving the discussion along, attached are
> draft patches to show what I think we should do in 9.3.  The
> first patch disables the unlogged-matviews feature at the user
> level (and perhaps could be reverted someday), while the second
> one moves scannability-state storage into a pg_class column.

I'm going to submit a modified version of the second patch today.
My biggest problems with it as it stands are the name chosen for
the new pg_class column, and the hard-coded assumption that this
relation-level flag is a good long-term indicator of whether all
connections find a matview to be scannable.  Scannability should
be abstracted to allow easy addition of other factors as we add
them.  Whether or not the "populated" state is in the catalog, it
is a serious mistake to conflate that with scannability.

Scannability will always be influenced by whether the matview has
been populated, but it is short-sighted not to draw a distinction
now, so that work people do in their applications is does not have
to be redone as we make scannability tests better.  Not to mention
the confusion factor of treating them as this patch does and then
trying to properly draw the distinction later.  IMV this patch, as
it stands, does much more to paint us into a corner regarding
future expansion than what came before.

As one example, I think it is highly desirable, long term, to allow
different sessions to set GUCs to different tolerances for old
data, and thus have different perspectives on whether a matview is
scannable; and this patch forces that to be the same for every
session.  The code I committed allows expansion in the direction of
different session perspectives on scannability, and the suggested
patch closes that door.

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

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> I'm going to submit a modified version of the second patch today.
> My biggest problems with it as it stands are the name chosen for
> the new pg_class column, and the hard-coded assumption that this
> relation-level flag is a good long-term indicator of whether all
> connections find a matview to be scannable. Scannability should
> be abstracted to allow easy addition of other factors as we add
> them. Whether or not the "populated" state is in the catalog, it
> is a serious mistake to conflate that with scannability.

> Scannability will always be influenced by whether the matview has
> been populated, but it is short-sighted not to draw a distinction
> now, so that work people do in their applications is does not have
> to be redone as we make scannability tests better. Not to mention
> the confusion factor of treating them as this patch does and then
> trying to properly draw the distinction later. IMV this patch, as
> it stands, does much more to paint us into a corner regarding
> future expansion than what came before.

> As one example, I think it is highly desirable, long term, to allow
> different sessions to set GUCs to different tolerances for old
> data, and thus have different perspectives on whether a matview is
> scannable; and this patch forces that to be the same for every
> session. The code I committed allows expansion in the direction of
> different session perspectives on scannability, and the suggested
> patch closes that door.

[ raised eyebrow... ] There is certainly nothing about file-size-based
state that would particularly support per-session scannability
determination. If you want to call the pg_class column relispopulated
rather than relisscannable, I have no particular objection to that ---
but otherwise it sounds like you're moving the goalposts at the last
minute.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> I'm going to submit a modified version of the second patch today.
>> My biggest problems with it as it stands are the name chosen for
>> the new pg_class column, and the hard-coded assumption that this
>> relation-level flag is a good long-term indicator of whether all
>> connections find a matview to be scannable.  Scannability should
>> be abstracted to allow easy addition of other factors as we add
>> them.  Whether or not the "populated" state is in the catalog, it
>> is a serious mistake to conflate that with scannability.
>>
>> Scannability will always be influenced by whether the matview has
>> been populated, but it is short-sighted not to draw a distinction
>> now, so that work people do in their applications is does not have
>> to be redone as we make scannability tests better.  Not to mention
>> the confusion factor of treating them as this patch does and then
>> trying to properly draw the distinction later.  IMV this patch, as
>> it stands, does much more to paint us into a corner regarding
>> future expansion than what came before.
>>
>> As one example, I think it is highly desirable, long term, to allow
>> different sessions to set GUCs to different tolerances for old
>> data, and thus have different perspectives on whether a matview is
>> scannable; and this patch forces that to be the same for every
>> session.  The code I committed allows expansion in the direction of
>> different session perspectives on scannability, and the suggested
>> patch closes that door.
>
> [ raised eyebrow... ]  There is certainly nothing about file-size-based
> state that would particularly support per-session scannability
> determination.

I didn't mean to suggest that there was; I was talking about
enshrining the notion that the relation was either scannable by all
or by none into pg_class.

> 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?

In the absence of a specific use case, maybe it is OK to leave
pg_relation_is_scannable() off for this release, but I worry we'll
encourage fuzzy thinking on the point that could be hard to undo
later.  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.  They might "get it"
more easily if the function is introduced at the same time we
introduce other criteria for determining scannability (primarily
based around how current the results are) and other methods for
dealing with stale data (like the ability to force a
concurrency-friendly form of REFRESH on an attempt to query stale
data).

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?  I haven't written that yet, but I figured that
when we went to an in-catalog representation of populated state, we
would want that.

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?

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

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> Things like how fresh the materialized view is certainly should
> be accessible via SQL and transactional.

Keep in mind that something like "whether the materialized view is
fresh enough to be scanned by this connection" may eventually
depend on session GUCs and the passage of time in the absence of
any database modifications.  That's why I believe that checking for
scannability should be done through a function, not a check of a
system table.

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

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

> That column name and the wording of some comments are the main
> things

Patch for that attached.  I left the part where you got rid of the
SQL function to allow users to test whether a matview is currently
scannable, and I did not add an AMV option to change the populated
flag, since those haven't had any real discussion yet.

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

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

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>> That column name and the wording of some comments are the main
>> things

> Patch for that attached. I left the part where you got rid of the
> SQL function to allow users to test whether a matview is currently
> scannable, and I did not add an AMV option to change the populated
> flag, since those haven't had any real discussion yet.

Per my other mail, I think adding an AMV option at this time is
inadvisable. I could go either way on removing or keeping the
is_scannable function --- anybody else have an opinion on that point?

Which of us is going to commit this? We're running low on time ...

regards, tom lane

On 05/06/2013 08:17 AM, Tom Lane wrote:

> Per my other mail, I think adding an AMV option at this time is
> inadvisable. I could go either way on removing or keeping the
> is_scannable function --- anybody else have an opinion on that point?
>
> Which of us is going to commit this? We're running low on time ...

As a my two cents, I have been watching this thread and the concern on
timeline is bothering me. I fully understand our want to get into Beta
and I know we don't want to slip schedule too much but quality is
important. It is what makes our project what it is more than any other
value we hold.

I also know we already slipped the beta once but we are not a
corporation, we do not have shareholders and nobody can fire us. If we
need to push it again for quality, shouldn't we?

Sincerely,

JD

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:

>> 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.

Since the patch we have floating around drops it, let's leave it
that way, in the interest of saving time getting to beta.  If it
was still there, I'd probably vote to leave it for the same reason.
It's pretty close to a toss-up at this point in terms of
cost/benefit, and that seems like the tie-breaker.

>> 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;

No kidding.  The same could be said for the rest of this.  It was
all talked to death months ago before I posted a patch which was
proposed for commit.  All this eleventh hour drama bothers me.
I've always maintained we should add an ALTER capabilities for such
things once they are in the catalog.  A few days ago they weren't.
Now they are.

> moreover, how would you inject any data without doing something
> like what pg_upgrade is doing?

I wouldn't.  I'm talking about taking that code out of pg_upgrade
and putting it in the server under an ALTER command.  If the point
of moving the info to the catalog was to avoid hacks, it would be
nice not to add a hack like that in the process.

> I see no point in an ALTER command until there's some other
> SQL-level infrastructure for incremental matview updates.

It's only important to avoid having client code directly update
system tables, which I generally view as a worthwhile goal.

> 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.

In that case, I guess we might as well follow suit and do it the
way you have it for 9.3.

I didn't see anything I thought needed changing in your first patch
(to disable unlogged matviews), and my suggested changes to your
second patch (to move tracking of populated status to pg_class) are
just names, aliases, and comments.  I suggest you review my
proposed tweak to your patch and apply both with any final
polishing you feel are appropriate.

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

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> It seems a bit late to be adding such a thing;

> No kidding. The same could be said for the rest of this. It was
> all talked to death months ago before I posted a patch which was
> proposed for commit. All this eleventh hour drama bothers me.

Well, we've been going back and forth about it for weeks. Without a
looming deadline, we'd probably still just be arguing inconclusively ...

> I didn't see anything I thought needed changing in your first patch
> (to disable unlogged matviews), and my suggested changes to your
> second patch (to move tracking of populated status to pg_class) are
> just names, aliases, and comments. I suggest you review my
> proposed tweak to your patch and apply both with any final
> polishing you feel are appropriate.

OK, will do.

regards, tom lane

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Since the patch we have floating around drops it, let's leave it
> that way, in the interest of saving time getting to beta.  If it
> was still there, I'd probably vote to leave it for the same reason.

I'll vote for dropping it also, though for a slightly different reason-
people can't build things on something that isn't there. Given that
we're still discussing it, that strikes me as the best idea. What goes
into 9.4 could be quite different and it's a lot easier if we don't have
to deal with supporting what may end up being the 'old' approach.

Thanks,

Stephen