Re: 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

On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro(at)ip9(dot)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(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro(at)ip9(dot)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(at)ip9(dot)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(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro(at)ip9(dot)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;

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

On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh(at)agliodbs(dot)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(at)agliodbs(dot)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(at)gmail(dot)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(at)agliodbs(dot)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(at)ip9(dot)org> wrote:
> On 24 October 2013 05:58, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro(at)ip9(dot)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(at)agliodbs(dot)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(at)2ndquadrant(dot)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(at)agliodbs(dot)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(at)2ndquadrant(dot)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(at)agliodbs(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)agliodbs(dot)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.

Thanks
Thomas Munro

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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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 ;)
>
>
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(at)postgresql(dot)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(at)momjian(dot)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(at)agliodbs(dot)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(at)ip9(dot)org> wrote:

> On 25 October 2013 01:17, Josh Berkus <josh(at)agliodbs(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)gmail(dot)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

> Regards
>
> David Rowley
>
>
>
>
>> Thanks
>> Thomas Munro
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)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(at)2ndquadrant(dot)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(at)gmx(dot)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(at)2ndquadrant(dot)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

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(at)2ndquadrant(dot)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