Thread: 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@leadboat.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@sss.pgh.pa.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@gmail.com> writes: > On Sat, Apr 27, 2013 at 10:59 AM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@gmail.com> writes:
> On Sat, Apr 27, 2013 at 3:33 PM, Tom Lane <tgl@sss.pgh.pa.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@gmail.com> wrote: > On Sat, Apr 27, 2013 at 10:59 AM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > Noah Misch <noah@leadboat.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sat, Apr 27, 2013 at 3:33 PM, Tom Lane <tgl@sss.pgh.pa.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@gmail.com> writes:
> On Sat, Apr 27, 2013 at 3:51 PM, Tom Lane <tgl@sss.pgh.pa.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@2ndQuadrant.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@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.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@2ndQuadrant.com> writes:
> On 28 April 2013 16:55, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> On 28 April 2013 16:55, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sat, Apr 27, 2013 at 3:51 PM, Tom Lane <tgl@sss.pgh.pa.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@gmail.com> writes:
> On Sun, Apr 28, 2013 at 11:41 AM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@ymail.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@snowman.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@ymail.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@ymail.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@snowman.net> wrote: > * Kevin Grittner (kgrittn@ymail.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@gmail.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@gmail.com> wrote: > Kevin Grittner <kgrittn@ymail.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@ymail.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 thenleads 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 situationsto 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 totruncate 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 theplaces 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 emptyingall matviews unless we only rely on the catalogs which we currently cannot. This will either slow down developmentor make users unhappy. Alternatively we can add yet another fork, but that has its price (quite a bit more openfiles 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 tocatalog 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 thingsmore 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 abouta 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 unloggedrelations the problem exists although its easier to represent. b) Just giving wrong responses is bad [tm]. Outdateddata is something completely different (it has existed in that state in the (near) past) than giving an emptyresponse (might never have been a visible state, and more likely not so in any reasonably near past). Consideran application behind a pooler suddently getting an empty response from a SELECT * FROM unlogged_matview; Itwon'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@ymail.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@2ndquadrant.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@gmail.com> wrote: > Kevin Grittner <kgrittn@ymail.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@2ndquadrant.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@2ndquadrant.com> wrote: > On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote: >> Andres Freund <andres@2ndquadrant.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@ymail.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@2ndquadrant.com> wrote: > > On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote: > >> Andres Freund <andres@2ndquadrant.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@agliodbs.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@2ndquadrant.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@2ndquadrant.com> wrote: > On 2013-04-30 07:33:05 -0700, Kevin Grittner wrote: >> Andres Freund <andres@2ndquadrant.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
Attachment
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@agliodbs.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@ymail.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@sss.pgh.pa.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@mit.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@ymail.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> Kevin Grittner <kgrittn@ymail.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
* Andres Freund (andres@2ndquadrant.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@snowman.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@snowman.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@momjian.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@snowman.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@2ndquadrant.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@2ndquadrant.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.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
Attachment
Tom Lane <tgl@sss.pgh.pa.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@ymail.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@sss.pgh.pa.us> wrote: > Kevin Grittner <kgrittn@ymail.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@momjian.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@ymail.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
Attachment
Kevin Grittner <kgrittn@ymail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.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@ymail.com> writes:
> Kevin Grittner <kgrittn@ymail.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@sss.pgh.pa.us> wrote: > Kevin Grittner <kgrittn@ymail.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@ymail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.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@ymail.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