Thread: CLUSTER FREEZE
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add that, for consistency with VACUUM. Is it useful?
Thanks
Thomas Munro
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add that, for consistency with VACUUM. Is it useful?
Thanks
Thomas Munro
Attachment
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
> Hi
> I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add
> that, for consistency with VACUUM. Is it useful?
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?
Anyway code side, I think you need to set both feeze_min_age as well
as freeze_table_age, see VACUUM command in gram.y
CLUSTER opt_freeze opt_verbose qualified_name cluster_index_specification
{ ClusterStmt *n = makeNode(ClusterStmt);
- n->relation = $3;
- n->indexname = $4;
- n->verbose = $2;
+ n->relation = $4;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->indexname = $5;
+ n->verbose = $3;
..
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On 24 October 2013 05:58, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote: >> Hi >> I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add >> that, for consistency with VACUUM. Is it useful? > > I wonder why anyone would like to freeze during CLUSTER command when > they already have separate way (VACUUM FREEZE) to achieve it, do you > know or can think of any case where user wants to do it along with > Cluster command? Well I can't speak for Thomas, but if you're doing such a heavy-duty operation on a table that involves an exclusive lock, you may as well freeze it. And in the case of CLUSTER, I imagine that in most instances you'd be confident the table isn't going to undergo much change (or at least not quickly). CLUSTER FREEZE would mean not having to run a separate VACUUM FREEZE after, and on a 10GB table, that's a big deal as it means not having to rescan the whole table again to freeze every row. Note that REFRESH MATERIALIZED VIEW freezes rows too as it's already writing all the data from scratch again, and has an exclusive lock. (this isn't the case with the CONCURRENTLY option obviously) -- Thom
Hi, On 2013-10-24 00:28:44 +0100, Thomas Munro wrote: > I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to > add that, for consistency with VACUUM. Is it useful? I think you'd need to prevent that form from working on system catalogs - xmin has a meaning there sometimes (e.g. indcheckxmin) at least. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Oct 24, 2013 at 10:28:43AM +0530, Amit Kapila wrote:
> On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
> > Hi
> > I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add
> > that, for consistency with VACUUM. Is it useful?
>
> I wonder why anyone would like to freeze during CLUSTER command when
> they already have separate way (VACUUM FREEZE) to achieve it, do you
> know or can think of any case where user wants to do it along with
> Cluster command?
>
> Anyway code side, I think you need to set both feeze_min_age as well
> as freeze_table_age, see VACUUM command in gram.y
>
> CLUSTER opt_freeze opt_verbose qualified_name cluster_index_specification
>
> {
> ClusterStmt *n = makeNode(ClusterStmt);
> - n->relation = $3;
> - n->indexname = $4;
> - n->verbose = $2;
> + n->relation = $4;
> + n->freeze_min_age = $2 ? 0 : -1;
> + n->indexname = $5;
> + n->verbose = $3;
> ..
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
Hi Amit,
If the FREEZE is part of the CLUSTER, you would only read/write the table
once. With a follow-up VACUUM FREEZE, you would re-read/write a second time.
I, for one, would appreciate being able to perform both in the same run. (+1)
Regards,
Ken
On 10/23/2013 09:58 PM, Amit Kapila wrote: > I wonder why anyone would like to freeze during CLUSTER command when > they already have separate way (VACUUM FREEZE) to achieve it, do you > know or can think of any case where user wants to do it along with > Cluster command? "If I'm rewriting the table anyway, let's freeze it". Otherwise, you have to write the same pages twice, if both CLUSTER and FREEZE are required. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 24 October 2013 05:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
Ok, I attach a new version that is more like VACUUM in gram.y. (Although I'm not sure why it isn't a single boolean flag).
Thanks,
Thomas Munro
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:I wonder why anyone would like to freeze during CLUSTER command when
> Hi
> I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add
> that, for consistency with VACUUM. Is it useful?
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?
As others have said, the goal is to freeze and cluster in a single step. You can already do that if you know how things work under the covers with:
SET vacuum_freeze_min_age = 0;
CLUSTER my_table;
This patch lets you say CLUSTER FREEZE instead. It mirrors VACUUM, which can freeze tuples based on the GUC and tuple age or the FREEZE keyword.
SET vacuum_freeze_min_age = 0;
CLUSTER my_table;
This patch lets you say CLUSTER FREEZE instead. It mirrors VACUUM, which can freeze tuples based on the GUC and tuple age or the FREEZE keyword.
Anyway code side, I think you need to set both feeze_min_age as well
as freeze_table_age, see VACUUM command in gram.y
Ok, I attach a new version that is more like VACUUM in gram.y. (Although I'm not sure why it isn't a single boolean flag).
Thanks,
Thomas Munro
Attachment
On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 10/23/2013 09:58 PM, Amit Kapila wrote: >> I wonder why anyone would like to freeze during CLUSTER command when >> they already have separate way (VACUUM FREEZE) to achieve it, do you >> know or can think of any case where user wants to do it along with >> Cluster command? > > "If I'm rewriting the table anyway, let's freeze it". > > Otherwise, you have to write the same pages twice, if both CLUSTER and > FREEZE are required. I wonder if we should go so far as to make this the default behavior, instead of just making it an option. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 10/24/2013 04:55 PM, Robert Haas wrote: > On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote: >> On 10/23/2013 09:58 PM, Amit Kapila wrote: >>> I wonder why anyone would like to freeze during CLUSTER command when >>> they already have separate way (VACUUM FREEZE) to achieve it, do you >>> know or can think of any case where user wants to do it along with >>> Cluster command? >> >> "If I'm rewriting the table anyway, let's freeze it". >> >> Otherwise, you have to write the same pages twice, if both CLUSTER and >> FREEZE are required. > > I wonder if we should go so far as to make this the default behavior, > instead of just making it an option. +1 from me. Can you think of a reason you *wouldn't* want to freeze? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Robert Haas <robertmhaas@gmail.com> writes:
> I wonder if we should go so far as to make this the default behavior,
> instead of just making it an option.
In that case you'd have to invent a NOFREEZE keyword, no? Ick.
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.
regards, tom lane
On Thu, Oct 24, 2013 at 10:39 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 10/23/2013 09:58 PM, Amit Kapila wrote: >> I wonder why anyone would like to freeze during CLUSTER command when >> they already have separate way (VACUUM FREEZE) to achieve it, do you >> know or can think of any case where user wants to do it along with >> Cluster command? > > "If I'm rewriting the table anyway, let's freeze it". So do you think that other places where we are rewriting the table with exclusive lock, we should have such mechanism or option and even if it is not there, it is kind of Todo item and tomorrow someone can write a patch to improve that situation. > Otherwise, you have to write the same pages twice, if both CLUSTER and > FREEZE are required. Yes, this is completely right and I understand this point, but the question I had in my mind before writing my last mail was that whether any or all places having this concept deserves to have an option like FREEZE? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Fri, Oct 25, 2013 at 4:29 AM, Thomas Munro <munro@ip9.org> wrote: > On 24 October 2013 05:58, Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote: >> > Hi >> > I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to >> > add >> > that, for consistency with VACUUM. Is it useful? >> >> I wonder why anyone would like to freeze during CLUSTER command when >> they already have separate way (VACUUM FREEZE) to achieve it, do you >> know or can think of any case where user wants to do it along with >> Cluster command? > > > As others have said, the goal is to freeze and cluster in a single step. > You can already do that if you know how things work under the covers with: > > SET vacuum_freeze_min_age = 0; > CLUSTER my_table; True, but in that case why don't we just mention above in the documentation of CLUSTER command, so that if user wants to freeze along with Cluster, he can use above way to Cluster. Some of the reason's, I could think of adding FREEZE as an option are: a. it's more explicit and easy to use for user. b. if by chance underlying mechanism changes (which is less likely) then above way of doing Cluster might not result into freeze. > This patch lets you say CLUSTER FREEZE instead. It mirrors VACUUM, which > can freeze tuples based on the GUC and tuple age or the FREEZE keyword. > >> >> Anyway code side, I think you need to set both feeze_min_age as well >> as freeze_table_age, see VACUUM command in gram.y > > > Ok, I attach a new version that is more like VACUUM in gram.y. (Although > I'm not sure why it isn't a single boolean flag). The reason of separate flags is that both are used to decide different things, freeze_min_age - this is used to decide the cutoff_xid, based on which FrozenTransactionId will be placed on tuple. freeze_table_age - used do decide, whether to scan all pages of a relation in Vacuum and in Cluster command it is ignored as it needs to scan all pages anyway. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On 2013-10-24 17:17:22 -0700, Josh Berkus wrote: > On 10/24/2013 04:55 PM, Robert Haas wrote: > > On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote: > >> On 10/23/2013 09:58 PM, Amit Kapila wrote: > >>> I wonder why anyone would like to freeze during CLUSTER command when > >>> they already have separate way (VACUUM FREEZE) to achieve it, do you > >>> know or can think of any case where user wants to do it along with > >>> Cluster command? > >> > >> "If I'm rewriting the table anyway, let's freeze it". > >> > >> Otherwise, you have to write the same pages twice, if both CLUSTER and > >> FREEZE are required. > > > > I wonder if we should go so far as to make this the default behavior, > > instead of just making it an option. > > +1 from me. Can you think of a reason you *wouldn't* want to freeze? It makes content from the future appear when you start using the relation in a query/session with an older snapshot. Currently CLUSTER is safe against that. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Oct 25, 2013 at 2:12 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-24 17:17:22 -0700, Josh Berkus wrote: >> On 10/24/2013 04:55 PM, Robert Haas wrote: >> > On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote: >> >> On 10/23/2013 09:58 PM, Amit Kapila wrote: >> >>> I wonder why anyone would like to freeze during CLUSTER command when >> >>> they already have separate way (VACUUM FREEZE) to achieve it, do you >> >>> know or can think of any case where user wants to do it along with >> >>> Cluster command? >> >> >> >> "If I'm rewriting the table anyway, let's freeze it". >> >> >> >> Otherwise, you have to write the same pages twice, if both CLUSTER and >> >> FREEZE are required. >> > >> > I wonder if we should go so far as to make this the default behavior, >> > instead of just making it an option. >> >> +1 from me. Can you think of a reason you *wouldn't* want to freeze? > > It makes content from the future appear when you start using the > relation in a query/session with an older snapshot. Currently CLUSTER is > safe against that. Eh, what? We wouldn't freeze XIDs that don't precede RecentGlobalXmin. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-10-25 09:13:20 -0400, Robert Haas wrote: > On Fri, Oct 25, 2013 at 2:12 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-10-24 17:17:22 -0700, Josh Berkus wrote: > >> On 10/24/2013 04:55 PM, Robert Haas wrote: > >> > On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote: > >> >> On 10/23/2013 09:58 PM, Amit Kapila wrote: > >> >>> I wonder why anyone would like to freeze during CLUSTER command when > >> >>> they already have separate way (VACUUM FREEZE) to achieve it, do you > >> >>> know or can think of any case where user wants to do it along with > >> >>> Cluster command? > >> >> > >> >> "If I'm rewriting the table anyway, let's freeze it". > >> >> > >> >> Otherwise, you have to write the same pages twice, if both CLUSTER and > >> >> FREEZE are required. > >> > > >> > I wonder if we should go so far as to make this the default behavior, > >> > instead of just making it an option. > >> > >> +1 from me. Can you think of a reason you *wouldn't* want to freeze? > > > > It makes content from the future appear when you start using the > > relation in a query/session with an older snapshot. Currently CLUSTER is > > safe against that. > > Eh, what? We wouldn't freeze XIDs that don't precede RecentGlobalXmin. Ah sorry, I thought that'd be the plan, similar to COPY FREEZE. Maybe because I've wished for it in the past ;) In that case I agree it should be the default. There really isn't any reason to not to immediately freeze tuples that can be frozen according to the xmin horizon. We don't immediately do it during normal vacuums because it would possibly cause superflous io - but that's not the case here. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Oct 24, 2013 at 10:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I wonder if we should go so far as to make this the default behavior, >> instead of just making it an option. > > In that case you'd have to invent a NOFREEZE keyword, no? Ick. Only if we think anyone would ever NOT want to freeze. > In any case, it's very far from obvious to me that CLUSTER ought > to throw away information by default, which is what you're proposing. I find it odd to referring to this as throwing away information. I know that you have a general concern about throwing away XIDs that are still needed for forensic purposes, but that is clearly the ONLY purpose that those XIDs serve, and the I/O advantages of freezing by default could be massive for many of our users. What's going to happen in practice is that experienced users will simply recommend CLUSTER FREEZE rather than plain CLUSTER, and you won't have the forensic information *anyway*. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 10/24/2013 07:19 PM, Tom Lane wrote: > In any case, it's very far from obvious to me that CLUSTER ought > to throw away information by default, which is what you're proposing. The problem here is that you're thinking of the 1/10 of 1% of our users who have a serious PostgreSQL failure and post something on the lists for help, for which XID forensic information is useful. As opposed to the 99.9% of our users for whom deferred freezing is a performance burden. While I realize that the 0.1% of users are more likely to have contact with you, personally, it's still bad policy for the project. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 25 October 2013 01:17, Josh Berkus <josh@agliodbs.com> wrote:
Ok, I attach an alternative patch that makes CLUSTER *always* freeze, without any option (but doesn't affect VACUUM FULL in the same way). I will post both alternatives to the commitfest app since there seems to be some disagreement about whether tuple freezing should be an optional.
Thanks
Thomas Munro
On 10/24/2013 04:55 PM, Robert Haas wrote:+1 from me. Can you think of a reason you *wouldn't* want to freeze?
> I wonder if we should go so far as to make this the default behavior,
> instead of just making it an option.
Ok, I attach an alternative patch that makes CLUSTER *always* freeze, without any option (but doesn't affect VACUUM FULL in the same way). I will post both alternatives to the commitfest app since there seems to be some disagreement about whether tuple freezing should be an optional.
Thanks
Thomas Munro
Attachment
On 10/26/2013 01:20 AM, Josh Berkus wrote: > On 10/24/2013 07:19 PM, Tom Lane wrote: >> In any case, it's very far from obvious to me that CLUSTER ought >> to throw away information by default, which is what you're proposing. > > The problem here is that you're thinking of the 1/10 of 1% of our users > who have a serious PostgreSQL failure and post something on the lists > for help, for which XID forensic information is useful. As opposed to > the 99.9% of our users for whom deferred freezing is a performance > burden. While I realize that the 0.1% of users are more likely to have > contact with you, personally, it's still bad policy for the project. Strong +1 from me. Doing the performant, sensible, low-admin thing by default is really important if you don't want a database that requires a two year training course and a professional DBA to use. Some DB vendors make that part of their business model, but personally at least that's certainly not the direction I'd like to see Pg go in. Autovacuum wrap-around prevention already gets rid of this info, it's not like it's kept forever anyway. Anything that makes the mechanics of bloat and vacuum less visible is a big win as far as I'm concerned. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-10-25 09:26:29 -0400, Robert Haas wrote: > > In any case, it's very far from obvious to me that CLUSTER ought > > to throw away information by default, which is what you're proposing. > > I find it odd to referring to this as throwing away information. I > know that you have a general concern about throwing away XIDs that are > still needed for forensic purposes, but that is clearly the ONLY > purpose that those XIDs serve, and the I/O advantages of freezing by > default could be massive for many of our users. What's going to > happen in practice is that experienced users will simply recommend > CLUSTER FREEZE rather than plain CLUSTER, and you won't have the > forensic information *anyway*. I think we should just apply your "preserve forensic information when freezing" patch. Then we're good to go without big arguments ;) Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Oct 29, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-25 09:26:29 -0400, Robert Haas wrote: >> > In any case, it's very far from obvious to me that CLUSTER ought >> > to throw away information by default, which is what you're proposing. >> >> I find it odd to referring to this as throwing away information. I >> know that you have a general concern about throwing away XIDs that are >> still needed for forensic purposes, but that is clearly the ONLY >> purpose that those XIDs serve, and the I/O advantages of freezing by >> default could be massive for many of our users. What's going to >> happen in practice is that experienced users will simply recommend >> CLUSTER FREEZE rather than plain CLUSTER, and you won't have the >> forensic information *anyway*. > > I think we should just apply your "preserve forensic information when > freezing" patch. Then we're good to go without big arguments ;) Well, I'm happy with that, too. But you wanted it significantly reworked and I haven't had time to do that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-10-29 11:29:24 -0400, Robert Haas wrote: > On Tue, Oct 29, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-10-25 09:26:29 -0400, Robert Haas wrote: > >> > In any case, it's very far from obvious to me that CLUSTER ought > >> > to throw away information by default, which is what you're proposing. > >> > >> I find it odd to referring to this as throwing away information. I > >> know that you have a general concern about throwing away XIDs that are > >> still needed for forensic purposes, but that is clearly the ONLY > >> purpose that those XIDs serve, and the I/O advantages of freezing by > >> default could be massive for many of our users. What's going to > >> happen in practice is that experienced users will simply recommend > >> CLUSTER FREEZE rather than plain CLUSTER, and you won't have the > >> forensic information *anyway*. > > > > I think we should just apply your "preserve forensic information when > > freezing" patch. Then we're good to go without big arguments ;) > > Well, I'm happy with that, too. But you wanted it significantly > reworked and I haven't had time to do that. I did? I only seem to remember suggesting to introduce HeapTupleHeaderGetRawXmin() and some bugfix around rewriteheap.c? I think the RawXmin() thing is a judgement call... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Oct 29, 2013 at 11:37 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-29 11:29:24 -0400, Robert Haas wrote: >> On Tue, Oct 29, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> > On 2013-10-25 09:26:29 -0400, Robert Haas wrote: >> >> > In any case, it's very far from obvious to me that CLUSTER ought >> >> > to throw away information by default, which is what you're proposing. >> >> >> >> I find it odd to referring to this as throwing away information. I >> >> know that you have a general concern about throwing away XIDs that are >> >> still needed for forensic purposes, but that is clearly the ONLY >> >> purpose that those XIDs serve, and the I/O advantages of freezing by >> >> default could be massive for many of our users. What's going to >> >> happen in practice is that experienced users will simply recommend >> >> CLUSTER FREEZE rather than plain CLUSTER, and you won't have the >> >> forensic information *anyway*. >> > >> > I think we should just apply your "preserve forensic information when >> > freezing" patch. Then we're good to go without big arguments ;) >> >> Well, I'm happy with that, too. But you wanted it significantly >> reworked and I haven't had time to do that. > > I did? I only seem to remember suggesting to introduce > HeapTupleHeaderGetRawXmin() and some bugfix around rewriteheap.c? I > think the RawXmin() thing is a judgement call... Well every place that currently gets the xmin will have to be changed to get the raw-xmin instead, with the exception of hunks like this: - targetxmin = HeapTupleHeaderGetXmin(tuple->t_data); + if (HeapTupleHeaderXminFrozen(tuple->t_data)) + targetxmin = FrozenTransactionId; + else + targetxmin = HeapTupleHeaderGetXmin(tuple->t_data); ...which will instead need to be reverted. The rename is mostly mechanical, but going through and looking for places where the difference between Xmin() and RawXmin() means that other hunks can be reverted is less so. I suppose it wouldn't take more than a few hours; I've just been up to my ears in parallelism stuff. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Oct 30, 2013 at 3:32 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-25 09:26:29 -0400, Robert Haas wrote:I think we should just apply your "preserve forensic information when
> > In any case, it's very far from obvious to me that CLUSTER ought
> > to throw away information by default, which is what you're proposing.
>
> I find it odd to referring to this as throwing away information. I
> know that you have a general concern about throwing away XIDs that are
> still needed for forensic purposes, but that is clearly the ONLY
> purpose that those XIDs serve, and the I/O advantages of freezing by
> default could be massive for many of our users. What's going to
> happen in practice is that experienced users will simply recommend
> CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
> forensic information *anyway*.
freezing" patch. Then we're good to go without big arguments ;)
Ok, here's a recap on the thread as I see it now.
1. Thomas wrote the patch with the idea that FREEZE could be an option for cluster to freeze the tuples during the cluster operation, which would save a vacuum freeze somewhere down the line. Sounds like a good idea.
2. Robert introduced the idea that this perhaps could be the default option for cluster.
3. Tom highlighted that making freeze the default would wipe out xmin values so they wouldn't be available to any forensics which might to take place in the event of a disaster.
4. Andres mentioned that we might want to wait for a patch which Robert has been working on which, currently I don't know much about but it sounds like it freezes without setting xmin to frozenXid?
5. Robert stated that he's not had much time to work on this patch due to having other commitments.
In the meantime Thomas sent in a patch which gets rid of the FREEZE option from cluster and makes it the default.
So now I'm wondering what the next move should be for this patch?
a. Are we waiting on Robert's patch to be commited before we can apply Thomas's cluster with freeze as default?
b. Are we waiting on me reviewing one or both of the patches? Which one?
I think probably it sounds safer not to make freeze the default, but then if we release 9.4 with CLUSTER FREEZE then in 9.5/10 decide it should be the default then we then have a redundant syntax to support for ever and ever.
Decision time?
Regards
David Rowley
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 18, 2013 at 09:22:58PM +1300, David Rowley wrote: > So now I'm wondering what the next move should be for this patch? > > a. Are we waiting on Robert's patch to be committed before we can apply Thomas's > cluster with freeze as default? > b. Are we waiting on me reviewing one or both of the patches? Which one? > > I think probably it sounds safer not to make freeze the default, but then if we > release 9.4 with CLUSTER FREEZE then in 9.5/10 decide it should be the default > then we then have a redundant syntax to support for ever and ever. > > Decision time? Yes, we probably should make a decision, unless Robert's idea can be implemented. We have to balance the ease of debugging MVCC failures with the interface we give to the user community. From my perspective, I don't see how we can justify the addition of a FREEZE option to CLUSTER. If we explain that you would always use FREEZE unless you want to preserve MVCC information for later debugging, users will reply with "Huh, why would I want that?" MVCC debugging is just too rare an event for them to understand its usefulness. If we do add FREEZE, all we would be doing is delaying the time when all CLUSTER operations will use FREEZE, and hence debugging will be just as difficult. My point is that will full knowledge, everyone would use FREEZE unless they expect MVCC bugs, which is going to be an almost zero population. Many MVCC failures are reproducible, so if we provide a C define that disables the freezing so MVCC information can be preserved, that might be good enough. Developers could enable the macro, and the macro could be used in other places where MVCC information is lost. This will make some MVCC harder, and will perhaps allow some MVCC bugs to exist longer. I believe there are other cases where rows could be frozen but we have avoided it for MVCC debugging reasons. Another idea would be the addition of a GUC that can disable optimistic freezing. This could be enabled by sites that suspect MVCC bugs. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 2013-11-18 11:39:44 -0500, Bruce Momjian wrote: > On Mon, Nov 18, 2013 at 09:22:58PM +1300, David Rowley wrote: > > So now I'm wondering what the next move should be for this patch? > > > > a. Are we waiting on Robert's patch to be committed before we can apply Thomas's > > cluster with freeze as default? > > b. Are we waiting on me reviewing one or both of the patches? Which one? > > > > I think probably it sounds safer not to make freeze the default, but then if we > > release 9.4 with CLUSTER FREEZE then in 9.5/10 decide it should be the default > > then we then have a redundant syntax to support for ever and ever. > > > > Decision time? > > Yes, we probably should make a decision, unless Robert's idea can be > implemented. We have to balance the ease of debugging MVCC failures > with the interface we give to the user community. Imo that patch really doesn't need too much further work. > From my perspective, I don't see how we can justify the addition of a > FREEZE option to CLUSTER. If we explain that you would always use > FREEZE unless you want to preserve MVCC information for later debugging, > users will reply with "Huh, why would I want that?" I tend to agree that we should work towards not needing that option. > Many MVCC failures are reproducible, so if we provide a C define that > disables the freezing so MVCC information can be preserved, that might > be good enough. Developers could enable the macro, and the macro could > be used in other places where MVCC information is lost. > This will make some MVCC harder, and will perhaps allow some MVCC bugs > to exist longer. > I believe there are other cases where rows could be frozen but we have > avoided it for MVCC debugging reasons. Another idea would be the > addition of a GUC that can disable optimistic freezing. This could be > enabled by sites that suspect MVCC bugs. I think that'd be an enormous failure. You don't usually need to look at those because there's a bug in postgres' mvcc logic but somewhere else (application, other postgres code). And looking at the mvcc information helps you to narrow it down, so you have a chance of actually finding a reproducible bug. Besides, in many of the !rewrite cases it's far from clear in which cases it's a benefit to freeze earlier. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 11/18/2013 08:39 AM, Bruce Momjian wrote: > If we do add FREEZE, all we would be doing is delaying the time when all > CLUSTER operations will use FREEZE, and hence debugging will be just as > difficult. My point is that will full knowledge, everyone would use > FREEZE unless they expect MVCC bugs, which is going to be an almost zero > population. +1 None of our users would willingly choose worse performance over the 0.1% possibility of needing to analyze a transaction failure. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Josh Berkus <josh@agliodbs.com> wrote: > On 11/18/2013 08:39 AM, Bruce Momjian wrote: >> If we do add FREEZE, all we would be doing is delaying the time >> when all CLUSTER operations will use FREEZE, and hence debugging >> will be just as difficult. My point is that will full >> knowledge, everyone would use FREEZE unless they expect MVCC >> bugs, which is going to be an almost zero population. > > +1 +1 > None of our users would willingly choose worse performance over > the 0.1% possibility of needing to analyze a transaction failure. I assume that's intended to represent the lifetime probability that a typical user would ever benefit, not per year or per transaction. Even as a lifetime number, it seems high unless we're specifically talking about hackers. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Oct 26, 2013 at 11:19 AM, Thomas Munro <munro@ip9.org> wrote:
On 25 October 2013 01:17, Josh Berkus <josh@agliodbs.com> wrote:On 10/24/2013 04:55 PM, Robert Haas wrote:> I wonder if we should go so far as to make this the default behavior,
> instead of just making it an option.+1 from me. Can you think of a reason you *wouldn't* want to freeze?
Ok, I attach an alternative patch that makes CLUSTER *always* freeze, without any option (but doesn't affect VACUUM FULL in the same way). I will post both alternatives to the commitfest app since there seems to be some disagreement about whether tuple freezing should be an optional.
It seems that most people to voice their opinion are leaning towards this being the more desired behaviour rather than adding the FREEZE as an option, so I reviewed this patch tonight.
I followed the code around and checked that we do still need the freeze age parameters in cluster_rel and we do because vacuum full uses the cluster_rel code and it will only perform a freeze if a user does VACUUM FULL FREEZE.
I have mixed feelings about updating the comment before the call to vacuum_set_xid_limits. It looks like the previous comment probably had not been looked at since vacuum full started using cluster_rel, so perhaps removing that was good, but on the other hand maybe it should be mentioning vacuum full and vacuum full freeze? Though I'm probably leaning more towards what you've changed it to as previously the comment was being a bit too clever and assuming things about the calling code which turned out bad as it seemed out of date and lacked knowledge of vacuum full using it.
I think that the patch should include some sort of notes in the documents to say that cluster performs freezing of tuples. I've attached a patch which adds something there, but I'm not 100% sure it is the right thing. Perhaps it should just read:
Cluster also performs aggressive "freezing" of tuples similar to VACUUM FULL FREEZE.
Although it's not exactly the same as you can perform a vacuum full freeze on a relation which does not have the clustered index set.
I'll delay a bit to see if anyone else has any comments about what the docs should read, but I think it is pretty much ready for a commiter's eyes.
Regards
David Rowley
Thanks
Thomas Munro
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> Yes, we probably should make a decision, unless Robert's idea can be >> implemented. We have to balance the ease of debugging MVCC failures >> with the interface we give to the user community. > > Imo that patch really doesn't need too much further work. Would you care to submit a version you're happy with? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-11-19 12:23:30 -0500, Robert Haas wrote: > On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote: > >> Yes, we probably should make a decision, unless Robert's idea can be > >> implemented. We have to balance the ease of debugging MVCC failures > >> with the interface we give to the user community. > > > > Imo that patch really doesn't need too much further work. > > Would you care to submit a version you're happy with? I'll give it a spin sometime this week. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Nov 19, 2013 at 11:35 PM, David Rowley <dgrowleyml@gmail.com> wrote:
I think that the patch should include some sort of notes in the documents to say that cluster performs freezing of tuples. I've attached a patch which adds something there, but I'm not 100% sure it is the right thing. Perhaps it should just read:Cluster also performs aggressive "freezing" of tuples similar to VACUUM FULL FREEZE.Although it's not exactly the same as you can perform a vacuum full freeze on a relation which does not have the clustered index set.I'll delay a bit to see if anyone else has any comments about what the docs should read, but I think it is pretty much ready for a commiter's eyes.
Hi Thomas,
I'm just going to mark the patch as waiting for author for now until you get the chance to add some changes to the documents.
Everything else looks ok.
Regards
David Rowley
RegardsDavid RowleyThanks
Thomas Munro
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, 2013-11-19 at 18:24 +0100, Andres Freund wrote: > On 2013-11-19 12:23:30 -0500, Robert Haas wrote: > > On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > >> Yes, we probably should make a decision, unless Robert's idea can be > > >> implemented. We have to balance the ease of debugging MVCC failures > > >> with the interface we give to the user community. > > > > > > Imo that patch really doesn't need too much further work. > > > > Would you care to submit a version you're happy with? > > I'll give it a spin sometime this week. I'm setting the CLUSTER FREEZE patch as returned with feedback, until this other patch has been considered.
On Sun, Dec 8, 2013 at 10:51 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On Tue, 2013-11-19 at 18:24 +0100, Andres Freund wrote: >> On 2013-11-19 12:23:30 -0500, Robert Haas wrote: >> > On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> > >> Yes, we probably should make a decision, unless Robert's idea can be >> > >> implemented. We have to balance the ease of debugging MVCC failures >> > >> with the interface we give to the user community. >> > > >> > > Imo that patch really doesn't need too much further work. >> > >> > Would you care to submit a version you're happy with? >> >> I'll give it a spin sometime this week. > > I'm setting the CLUSTER FREEZE patch as returned with feedback, until > this other patch has been considered. The other patch in question is now committed. Here's a patch for this, which does somewhat more extensive surgery than previously proposed (actually, I wrote it from scratch, before looking at the prior submission). It's basically the same idea, though. Note that both versions of the patch affect not only CLUSTER, but also VACUUM FULL. I suspect we ought to extend this to rewriting variants of ALTER TABLE as well, but a little thought is needed there. ATRewriteTables() appears to just call heap_insert() for each updated row, which if I'm not mistaken is an MVCC violation - offhand, it seems like a transaction with an older MVCC snapshot could access the table for this first time after the rewriter commits, and fail to see rows which would have appeared to be there before the rewrite. (I haven't actually tested this, so watch me be wrong.) If we're OK with an MVCC violation here, we could just pass HEAP_INSERT_FROZEN and have a slightly different flavor of violation; if not, this needs some kind of more extensive surgery quite apart from what we do about freezing. Review of the attached appreciated... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On 2013-12-22 20:45:02 -0500, Robert Haas wrote: > I suspect we ought to extend this to rewriting variants of ALTER TABLE > as well, but a little thought is needed there. ATRewriteTables() > appears to just call heap_insert() for each updated row, which if I'm > not mistaken is an MVCC violation - offhand, it seems like a > transaction with an older MVCC snapshot could access the table for > this first time after the rewriter commits, and fail to see rows which > would have appeared to be there before the rewrite. (I haven't > actually tested this, so watch me be wrong.) If we're OK with an MVCC > violation here, we could just pass HEAP_INSERT_FROZEN and have a > slightly different flavor of violation; if not, this needs some kind > of more extensive surgery quite apart from what we do about freezing. Yes, rewriting ALTER TABLEs certainly aren't MVCC safe. I thought that was documented, but apparently not. I am not sure it can be fixed easily using the tricks CLUSTER plays, there might be nasty edge-cases because of the changes in the column definition. Certainly not a trivial project. I think we should leave ALTER TABLE as a completely separate project and just improve CLUSTER for now. The practical impact of rewriting ALTER TABLEs not freezing is far smaller, because they are very seldomly performed in bigger databases. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Dec 23, 2013 at 6:53 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-12-22 20:45:02 -0500, Robert Haas wrote: >> I suspect we ought to extend this to rewriting variants of ALTER TABLE >> as well, but a little thought is needed there. ATRewriteTables() >> appears to just call heap_insert() for each updated row, which if I'm >> not mistaken is an MVCC violation - offhand, it seems like a >> transaction with an older MVCC snapshot could access the table for >> this first time after the rewriter commits, and fail to see rows which >> would have appeared to be there before the rewrite. (I haven't >> actually tested this, so watch me be wrong.) If we're OK with an MVCC >> violation here, we could just pass HEAP_INSERT_FROZEN and have a >> slightly different flavor of violation; if not, this needs some kind >> of more extensive surgery quite apart from what we do about freezing. > > Yes, rewriting ALTER TABLEs certainly aren't MVCC safe. I thought that > was documented, but apparently not. > I am not sure it can be fixed easily using the tricks CLUSTER plays, > there might be nasty edge-cases because of the changes in the column > definition. Certainly not a trivial project. > > I think we should leave ALTER TABLE as a completely separate project and > just improve CLUSTER for now. The practical impact of rewriting ALTER > TABLEs not freezing is far smaller, because they are very seldomly > performed in bigger databases. OK, I have committed my patch to make CLUSTER and VACUUM FULL freeze aggressively, and have left ALTER TABLE alone for now. It would be nice to get to the point where a database-wide VACUUM FULL would serve to bump datfrozenxid, so as to avoid having to give this sort of advice: http://www.postgresql.org/message-id/CA+Tgmobth=AQkwMWtCsQLAEnV59GT4g3oqPQS45CB+fvG9MxSA@mail.gmail.com However, it doesn't, quite: a bare VACUUM FULL now bumps relfrozenxid for every table in the database *except pg_class*. It does call vac_update_datfrozenxid() afterwards, but that only helps if pg_class is not among the things holding back datfrozenxid. I haven't dug into this much yet, but I think it might be worth fixing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company