Re: Re: [COMMITTERS] pgsql: Remove old-style VACUUM FULL (which was known for a little while

Log Message:
-----------
Remove old-style VACUUM FULL (which was known for a little while as
VACUUM FULL INPLACE), along with a boatload of subsidiary code and complexity.
Per discussion, the use case for this method of vacuuming is no longer large
enough to justify maintaining it; not to mention that we don't wish to invest
the work that would be needed to make it play nicely with Hot Standby.

Aside from the code directly related to old-style VACUUM FULL, this commit
removes support for certain WAL record types that could only be generated
within VACUUM FULL, redirect-pointer removal in heap_page_prune, and
nontransactional generation of cache invalidation sinval messages (the last
being the sticking point for Hot Standby).

We still have to retain all code that copes with finding HEAP_MOVED_OFF and
HEAP_MOVED_IN flag bits on existing tuples. This can't be removed as long
as we want to support in-place update from pre-9.0 databases.

Modified Files:
--------------
pgsql/doc/src/sgml:
indexam.sgml (r2.32 -> r2.33)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/indexam.sgml?r1=2.32&r2=2.33)
maintenance.sgml (r1.98 -> r1.99)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/maintenance.sgml?r1=1.98&r2=1.99)
pgsql/doc/src/sgml/ref:
vacuum.sgml (r1.57 -> r1.58)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/vacuum.sgml?r1=1.57&r2=1.58)
vacuumdb.sgml (r1.49 -> r1.50)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/vacuumdb.sgml?r1=1.49&r2=1.50)
pgsql/src/backend/access/gin:
README (r1.6 -> r1.7)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/gin/README?r1=1.6&r2=1.7)
ginvacuum.c (r1.32 -> r1.33)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/gin/ginvacuum.c?r1=1.32&r2=1.33)
pgsql/src/backend/access/gist:
gistvacuum.c (r1.46 -> r1.47)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/gist/gistvacuum.c?r1=1.46&r2=1.47)
pgsql/src/backend/access/heap:
README.HOT (r1.4 -> r1.5)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/heap/README.HOT?r1=1.4&r2=1.5)
heapam.c (r1.285 -> r1.286)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/heap/heapam.c?r1=1.285&r2=1.286)
pruneheap.c (r1.20 -> r1.21)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/heap/pruneheap.c?r1=1.20&r2=1.21)
pgsql/src/backend/access/nbtree:
README (r1.21 -> r1.22)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/nbtree/README?r1=1.21&r2=1.22)
nbtpage.c (r1.117 -> r1.118)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/nbtree/nbtpage.c?r1=1.117&r2=1.118)
nbtree.c (r1.174 -> r1.175)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/nbtree/nbtree.c?r1=1.174&r2=1.175)
nbtxlog.c (r1.59 -> r1.60)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/nbtree/nbtxlog.c?r1=1.59&r2=1.60)
pgsql/src/backend/access/transam:
xact.c (r1.283 -> r1.284)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xact.c?r1=1.283&r2=1.284)
xlog.c (r1.367 -> r1.368)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.367&r2=1.368)
pgsql/src/backend/catalog:
index.c (r1.334 -> r1.335)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/index.c?r1=1.334&r2=1.335)
pgsql/src/backend/commands:
analyze.c (r1.149 -> r1.150)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/analyze.c?r1=1.149&r2=1.150)
copy.c (r1.323 -> r1.324)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/copy.c?r1=1.323&r2=1.324)
vacuum.c (r1.404 -> r1.405)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/vacuum.c?r1=1.404&r2=1.405)
vacuumlazy.c (r1.128 -> r1.129)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/vacuumlazy.c?r1=1.128&r2=1.129)
pgsql/src/backend/executor:
execUtils.c (r1.169 -> r1.170)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/execUtils.c?r1=1.169&r2=1.170)
nodeModifyTable.c (r1.5 -> r1.6)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/nodeModifyTable.c?r1=1.5&r2=1.6)
pgsql/src/backend/parser:
gram.y (r2.706 -> r2.707)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/gram.y?r1=2.706&r2=2.707)
pgsql/src/backend/storage/lmgr:
proc.c (r1.214 -> r1.215)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/lmgr/proc.c?r1=1.214&r2=1.215)
pgsql/src/backend/utils/cache:
inval.c (r1.94 -> r1.95)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/cache/inval.c?r1=1.94&r2=1.95)
pgsql/src/backend/utils/time:
tqual.c (r1.115 -> r1.116)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/time/tqual.c?r1=1.115&r2=1.116)
pgsql/src/bin/scripts:
vacuumdb.c (r1.33 -> r1.34)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/bin/scripts/vacuumdb.c?r1=1.33&r2=1.34)
pgsql/src/include/access:
genam.h (r1.82 -> r1.83)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/genam.h?r1=1.82&r2=1.83)
heapam.h (r1.146 -> r1.147)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/heapam.h?r1=1.146&r2=1.147)
htup.h (r1.110 -> r1.111)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/htup.h?r1=1.110&r2=1.111)
nbtree.h (r1.127 -> r1.128)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/nbtree.h?r1=1.127&r2=1.128)
xact.h (r1.100 -> r1.101)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/xact.h?r1=1.100&r2=1.101)
xlog.h (r1.101 -> r1.102)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/xlog.h?r1=1.101&r2=1.102)
pgsql/src/include/commands:
vacuum.h (r1.87 -> r1.88)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/commands/vacuum.h?r1=1.87&r2=1.88)
pgsql/src/include/executor:
executor.h (r1.166 -> r1.167)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/executor/executor.h?r1=1.166&r2=1.167)
pgsql/src/include/nodes:
parsenodes.h (r1.427 -> r1.428)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/parsenodes.h?r1=1.427&r2=1.428)
pgsql/src/include/parser:
kwlist.h (r1.10 -> r1.11)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/parser/kwlist.h?r1=1.10&r2=1.11)
pgsql/src/include/utils:
inval.h (r1.48 -> r1.49)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/inval.h?r1=1.48&r2=1.49)
pgsql/src/test/regress/expected:
vacuum.out (r1.4 -> r1.5)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/vacuum.out?r1=1.4&r2=1.5)
pgsql/src/test/regress/sql:
vacuum.sql (r1.3 -> r1.4)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/sql/vacuum.sql?r1=1.3&r2=1.4)

On Mon, 2010-02-08 at 04:33 +0000, Tom Lane wrote:

> We still have to retain all code that copes with finding
> HEAP_MOVED_OFF and HEAP_MOVED_IN flag bits on existing tuples. This
> can't be removed as long as we want to support in-place update from
> pre-9.0 databases.

This doesn't seem to be a great reason. Letting weird states exist is
not a feature, its a risk. Let me explain.

This would only happen if a VACUUM FULL had been run on the pre-9.0
database and it had failed part way through. Re-VACUUMing would remove
those settings.

ISTM that that the upgrade process should cover this, not force the
server to cope with rare and legacy situations. If we do not do this,
then we might argue it should *never* be removed because this same rare
situation can persist into 9.1 etc..

There were data loss situations possible in early 8.4 and these
persisted into later releases *because* the minor release upgrade
process did not contain a scan to detect and remove the earlier
problems. If we allow tuples to be in strange legacy states we greatly
increase the difficulty of diagnosing and fixing problems. People will
say "moved in/off can be ignored now" and mistakes will happen.

We should remove the moved in/off flag bits and make it a part of the
upgrade process to ensure the absence of those states.

--
Simon Riggs www.2ndQuadrant.com

On Thursday 11 February 2010 11:10:32 Simon Riggs wrote:
> On Mon, 2010-02-08 at 04:33 +0000, Tom Lane wrote:
> > We still have to retain all code that copes with finding
> > HEAP_MOVED_OFF and HEAP_MOVED_IN flag bits on existing tuples. This
> > can't be removed as long as we want to support in-place update from
> > pre-9.0 databases.
>
> This doesn't seem to be a great reason. Letting weird states exist is
> not a feature, its a risk. Let me explain.
>
> This would only happen if a VACUUM FULL had been run on the pre-9.0
> database and it had failed part way through. Re-VACUUMing would remove
> those settings.
>
> ISTM that that the upgrade process should cover this, not force the
> server to cope with rare and legacy situations. If we do not do this,
> then we might argue it should *never* be removed because this same rare
> situation can persist into 9.1 etc..
>
> There were data loss situations possible in early 8.4 and these
> persisted into later releases *because* the minor release upgrade
> process did not contain a scan to detect and remove the earlier
> problems. If we allow tuples to be in strange legacy states we greatly
> increase the difficulty of diagnosing and fixing problems. People will
> say "moved in/off can be ignored now" and mistakes will happen.
>
> We should remove the moved in/off flag bits and make it a part of the
> upgrade process to ensure the absence of those states.
Essentially requiring a successfull VACUUM FULL or CLUSTER on all tables is
imho in the same ballpark as requiring a dump+restore timewise on bigger
databases.

Andres

Andres Freund wrote:
> On Thursday 11 February 2010 11:10:32 Simon Riggs wrote:
>> On Mon, 2010-02-08 at 04:33 +0000, Tom Lane wrote:
>>> We still have to retain all code that copes with finding
>>> HEAP_MOVED_OFF and HEAP_MOVED_IN flag bits on existing tuples. This
>>> can't be removed as long as we want to support in-place update from
>>> pre-9.0 databases.
>> This doesn't seem to be a great reason. Letting weird states exist is
>> not a feature, its a risk. Let me explain.
>>
>> This would only happen if a VACUUM FULL had been run on the pre-9.0
>> database and it had failed part way through. Re-VACUUMing would remove
>> those settings.
>>
>> ISTM that that the upgrade process should cover this, not force the
>> server to cope with rare and legacy situations. If we do not do this,
>> then we might argue it should *never* be removed because this same rare
>> situation can persist into 9.1 etc..
>>
>> There were data loss situations possible in early 8.4 and these
>> persisted into later releases *because* the minor release upgrade
>> process did not contain a scan to detect and remove the earlier
>> problems. If we allow tuples to be in strange legacy states we greatly
>> increase the difficulty of diagnosing and fixing problems. People will
>> say "moved in/off can be ignored now" and mistakes will happen.
>>
>> We should remove the moved in/off flag bits and make it a part of the
>> upgrade process to ensure the absence of those states.
> Essentially requiring a successfull VACUUM FULL or CLUSTER on all tables is
> imho in the same ballpark as requiring a dump+restore timewise on bigger
> databases.

A plain VACUUM would be enough.

But FWIW, +1 from me for keeping the support for HEAP_IN/OUT in 9.0.
It's not a lot of code, and that way we don't need to invent some
safeguards in pg_migrator to check that there's no HEAP_IN/OUT flags
just yet.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Thu, 2010-02-11 at 14:53 +0200, Heikki Linnakangas wrote:
> Andres Freund wrote:
> > On Thursday 11 February 2010 11:10:32 Simon Riggs wrote:
> >> We should remove the moved in/off flag bits and make it a part of the
> >> upgrade process to ensure the absence of those states.
> > Essentially requiring a successfull VACUUM FULL or CLUSTER on all tables is
> > imho in the same ballpark as requiring a dump+restore timewise on bigger
> > databases.
>
> A plain VACUUM would be enough.

Yes

> But FWIW, +1 from me for keeping the support for HEAP_IN/OUT in 9.0.
> It's not a lot of code, and that way we don't need to invent some
> safeguards in pg_migrator to check that there's no HEAP_IN/OUT flags
> just yet.

The amount of code has nothing to do with keeping it or removing it.

Requiring the backend to support something just because an external
utility wants to optimise the performance of upgrades in a way that may
introduce later bugs seems rather questionable to me.

You still have to perform a backup of the database prior to upgrade and
that also must scan the whole database, so the overall time to upgrade
will still vary according to database size. So I don't see any overall
benefit, just risk, and I cited a similar situation where that risk has
already materialized into damage for a user in at least one case.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> This would only happen if a VACUUM FULL had been run on the pre-9.0
> database and it had failed part way through.

If that were true, you might have an argument, but it isn't. VACUUM
FULL was never very careful about getting rid of all MOVED_xxx bits.
See the comments for update_hint_bits().

> We should remove the moved in/off flag bits and make it a part of the
> upgrade process to ensure the absence of those states.

That's not happening. The whole point of upgrade in place is to not do
anything as expensive as a full-database scan.

regards, tom lane

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> You still have to perform a backup of the database prior to upgrade and
> that also must scan the whole database, so the overall time to upgrade
> will still vary according to database size. So I don't see any overall
> benefit, just risk, and I cited a similar situation where that risk has
> already materialized into damage for a user in at least one case.

You cited no such case; you merely hypothesized that it could happen.

As for the alleged risks involved, keeping the tqual support for MOVED
bits cannot create any data-loss risks that haven't existed right along
in every previous release. But depending on MOVED bits to be reliably
gone after a pg_upgrade would introduce a very obvious data loss risk
that wasn't there before, namely that pg_upgrade misses one.

regards, tom lane

On Thu, 2010-02-11 at 11:27 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > You still have to perform a backup of the database prior to upgrade and
> > that also must scan the whole database, so the overall time to upgrade
> > will still vary according to database size. So I don't see any overall
> > benefit, just risk, and I cited a similar situation where that risk has
> > already materialized into damage for a user in at least one case.
>
> You cited no such case; you merely hypothesized that it could happen.

Apologies for not providing more details. There was a serious problem in
an 8.4.1 database just before Christmas. Mostly off-list but a few
community members knew of it. The db had been upgraded from 8.4.0, where
some data loss issues existed and the corruption persisted even in a
release where it could never have been created.

> As for the alleged risks involved, keeping the tqual support for MOVED
> bits cannot create any data-loss risks that haven't existed right along
> in every previous release. But depending on MOVED bits to be reliably
> gone after a pg_upgrade would introduce a very obvious data loss risk
> that wasn't there before, namely that pg_upgrade misses one.

Avoiding a scan before running pg_upgrade is just a performance
optimisation. I don't think we should be optimising an upgrade in this
way, especially since sane people do database backups before upgrade
anyway. The optimisation is misplaced. The fact that we are actively
planning to have code in the server that only gets executed if
pg_upgrade screws up scares the hell out of me. If someone else
suggested it you'd give them both barrels. We should be ensuring
pg_upgrade works, not giving it leeway to miss a few things but work
quickly. I think pg_upgrade should be investing time in a utility which
pre-scans the database to check it is safely upgradeable, not have the
server support an external utility that has unsafe usage procedures.

--
Simon Riggs www.2ndQuadrant.com

On Thu, Feb 11, 2010 at 1:33 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Avoiding a scan before running pg_upgrade is just a performance
> optimisation.

But using pg_upgrade AT ALL is also a performance optimization; in
fact AFAICS it's the only reason to use pg_upgrade. So if you take
that away there's no reason to use it at all.

...Robert

On Thu, 2010-02-11 at 13:42 -0500, Robert Haas wrote:
> On Thu, Feb 11, 2010 at 1:33 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > Avoiding a scan before running pg_upgrade is just a performance
> > optimisation.
>
> But using pg_upgrade AT ALL is also a performance optimization; in
> fact AFAICS it's the only reason to use pg_upgrade. So if you take
> that away there's no reason to use it at all.

I understand that the final process to switch from one release to
another needs to be quick. Before that we can have any number of
preparatory steps. One of those is backup, if you're sane. Another one
should be a preparatory step that can be performed while the database is
still on-line that checks that everything is in a good state for
upgrade. No corruptions, no weird flags, everything good.

If that last step is part of all upgrade procedures, including both
minor and major we will all be happier and healthier. And the server can
depend on that check and doesn't need to check itself for those
weirdnesses from an earlier era.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> Avoiding a scan before running pg_upgrade is just a performance
> optimisation. I don't think we should be optimising an upgrade in this
> way, especially since sane people do database backups before upgrade
> anyway. The optimisation is misplaced. The fact that we are actively
> planning to have code in the server that only gets executed if
> pg_upgrade screws up scares the hell out of me. If someone else
> suggested it you'd give them both barrels.

If we were putting in new, never tested, code of that description I'd be
scared of it too. Code that's been there since the previous century,
however, is not even remotely the same type of case. Arguably, there is
bigger risk in removing it from tqual.c than not doing so --- it is not
impossible to screw up the removal ...

regards, tom lane

On Thu, Feb 11, 2010 at 2:03 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, 2010-02-11 at 13:42 -0500, Robert Haas wrote:
>> On Thu, Feb 11, 2010 at 1:33 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > Avoiding a scan before running pg_upgrade is just a performance
>> > optimisation.
>>
>> But using pg_upgrade AT ALL is also a performance optimization; in
>> fact AFAICS it's the only reason to use pg_upgrade.  So if you take
>> that away there's no reason to use it at all.
>
> I understand that the final process to switch from one release to
> another needs to be quick. Before that we can have any number of
> preparatory steps. One of those is backup, if you're sane. Another one
> should be a preparatory step that can be performed while the database is
> still on-line that checks that everything is in a good state for
> upgrade. No corruptions, no weird flags, everything good.
>
> If that last step is part of all upgrade procedures, including both
> minor and major we will all be happier and healthier. And the server can
> depend on that check and doesn't need to check itself for those
> weirdnesses from an earlier era.

That's a good point. I think we're going to keep running across
situations where we'd like to have a way of verifying that a
particular invariant holds for every page of a given relation. With
the infrastructure that we have now, we're going to be stuck with the
MOVED_xxx bits essentially forever. When we got to release 9.5, we
still won't be able to drop this code, because there could be someone
who used pg_upgrade to go from 8.3 or 8.4 to 9.0 and then to 9.1 and
then to 9.2 and then to 9.3 and then to 9.4 and now wants to go to
9.5.

I'm not quite sure how to do this in practice. One idea would be to
record the catversion that created the relation in pg_class, and make
pg_upgrade preserve the catversion, but make CLUSTER or similar bump
it on successful completion. But I'm not sure if that covers all the
cases we care about, or if requiring CLUSTER is too intrusive.

I think it's probably too late to work on this for 9.0, but it would
be nice to get it done for 9.1 so that we can make a long-term plan to
phase things like this out without relying on making statements like
"if before you pg_upgrade'd your database X times it was originally
from version X or earlier, and if you ever vacuum full'd it and any of
those tuples are still around, you might have a problem - but we can't
tell you whether that's the case or not."

...Robert

Robert Haas escribió:

> I'm not quite sure how to do this in practice. One idea would be to
> record the catversion that created the relation in pg_class, and make
> pg_upgrade preserve the catversion, but make CLUSTER or similar bump
> it on successful completion. But I'm not sure if that covers all the
> cases we care about, or if requiring CLUSTER is too intrusive.

Wouldn't a table-wide vacuum remove those hint bits too? If so, just
letting the database live for a bit would get rid of them.

... it seems it doesn't, but I wonder why can't we just patch
heap_freeze_tuple to unset HEAP_MOVED if they are seen set and the
VACUUM FULL transaction is no longer running. In fact, it seems silly
not to.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Feb 11, 2010 at 2:46 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Robert Haas escribió:
>
>> I'm not quite sure how to do this in practice.  One idea would be to
>> record the catversion that created the relation in pg_class, and make
>> pg_upgrade preserve the catversion, but make CLUSTER or similar bump
>> it on successful completion.  But I'm not sure if that covers all the
>> cases we care about, or if requiring CLUSTER is too intrusive.
>
> Wouldn't a table-wide vacuum remove those hint bits too?  If so, just
> letting the database live for a bit would get rid of them.
>
> ... it seems it doesn't, but I wonder why can't we just patch
> heap_freeze_tuple to unset HEAP_MOVED if they are seen set and the
> VACUUM FULL transaction is no longer running.  In fact, it seems silly
> not to.

Well the issue is that it's not enough to get rid of them; we need a
way for pg_migrator to be certain that they're all gone. And there
will be other things in the future that we may want to handle this
way: page format conversions, for example, say to add checksums. You
don't want to let people do the migration and then have the new
cluster choke after it's already in production.

Now, the issue is that for some types of modifications, VACUUM might
be sufficient; others might require CLUSTER; still others might (I
suppose) require some other treatment still - like, say, regular
VACUUM but with some option to force every page to be scanned. So we
might find that for an upgrade from 9.3 to 9.4 you just need a regular
VACUUM; unless the relation originally came from 9.2 or earlier, in
which case you need a VACUUM that doesn't skip any pages; but if the
relation originally came from 8.4 or earlier, then you actually need
CLUSTER. Or whatever the case may be. Recording some bookkeeping
information in pg_class so that pg_migrator can tell what's going on
at a glance seems like the right approach, but I'm fuzzy on the
details.

...Robert

Robert Haas escribió:
> On Thu, Feb 11, 2010 at 2:46 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > Robert Haas escribió:
> >
> >> I'm not quite sure how to do this in practice.  One idea would be to
> >> record the catversion that created the relation in pg_class, and make
> >> pg_upgrade preserve the catversion, but make CLUSTER or similar bump
> >> it on successful completion.  But I'm not sure if that covers all the
> >> cases we care about, or if requiring CLUSTER is too intrusive.
> >
> > Wouldn't a table-wide vacuum remove those hint bits too?  If so, just
> > letting the database live for a bit would get rid of them.
> >
> > ... it seems it doesn't, but I wonder why can't we just patch
> > heap_freeze_tuple to unset HEAP_MOVED if they are seen set and the
> > VACUUM FULL transaction is no longer running.  In fact, it seems silly
> > not to.
>
> Well the issue is that it's not enough to get rid of them; we need a
> way for pg_migrator to be certain that they're all gone.

Oh, I was just pointing out that if we were to add a catversion column
to the table, it could be fixed by a simple complete vacuum -- no need
for something heavy like CLUSTER. So to upgrade from 8.4 to 9.1 you
could first upgrade to 9.0, then run VACUUM on all your tables, then
upgrade to 9.1.

> Now, the issue is that for some types of modifications, VACUUM might
> be sufficient; others might require CLUSTER; still others might (I
> suppose) require some other treatment still - like, say, regular
> VACUUM but with some option to force every page to be scanned. So we
> might find that for an upgrade from 9.3 to 9.4 you just need a regular
> VACUUM; unless the relation originally came from 9.2 or earlier, in
> which case you need a VACUUM that doesn't skip any pages; but if the
> relation originally came from 8.4 or earlier, then you actually need
> CLUSTER. Or whatever the case may be. Recording some bookkeeping
> information in pg_class so that pg_migrator can tell what's going on
> at a glance seems like the right approach, but I'm fuzzy on the
> details.

Maybe a bitmap of stuff that was applied to the table, where bit 1 means
vacuum, bit 2 means space reservation, bit 3 means CRC added, and so on.
"relflags", so to speak ... ?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> I understand that the final process to switch from one release to
> another needs to be quick. Before that we can have any number of
> preparatory steps. One of those is backup, if you're sane. Another one
> should be a preparatory step that can be performed while the database is
> still on-line that checks that everything is in a good state for
> upgrade. No corruptions, no weird flags, everything good.

No, that's just fantasy. Unless you lock down the database to read only
(which subverts the point, namely minimal operational downtime), the
prescan doesn't work because it can't be sure somebody didn't break
something after it examined it. In the case at hand, there's no way to
prevent somebody from running a VACUUM FULL just before you trigger
the changeover.

It would probably be useful to have a utility that runs *in 9.0* and
gets rid of MOVED bits, so that we could drop support for them in 9.1.
But it's not happening for 9.0.

regards, tom lane

Robert Haas wrote:
> Recording some bookkeeping information in pg_class so that pg_migrator can tell what's going on
> at a glance seems like the right approach, but I'm fuzzy on the details.
>

November of 2008 was a pretty good month for me, so I enjoyed this
flashback to it. That's when the path for how to handle space
reservation wandered to this same place and then died there: how to
know for sure what information to put into the catalog for the upgrade
utility, before the upgrade utility exists. Message from Bruce at
http://archives.postgresql.org/message-id/200901300457.n0U4v1707979@momjian.us
and my follow-up summarized/linked to the highlights of the earlier
discussion on that one.

This time around, the way the upgrade is being staged allows a possible
path through this dependency chain, as noted by Tom:

> It would probably be useful to have a utility that runs *in 9.0* and
> gets rid of MOVED bits, so that we could drop support for them in 9.1.
> But it's not happening for 9.0.

As long as this one gets deprecated nicely here--so the server still
knows how to deal with the ugly parts, but will not introduce any more
of them--this should make for a good test case to gain experience with
handling this whole class of problem. If the above exercise finishes
with a clear "had we just recorded <x> in the catalog before 9.0 came
out we could have done this more easily", I think it would be much more
likely that a future "we should record <y> in the catalog to improve the
odds of adding this feature in a way that can upgrade to it in-place"
decision might get made correctly in advance of the upgrade utility
actually existing. Right now, just like the 8.4 case, it seems quite
possible no one will develop a plan in time they can prove will work
well enough to justify adding speculative catalog support for it. Much
easier to figure that out in retrospect though, after the matching
utility that uses the data exists.

If you think through the implications of that far enough, eventually you
start to realize that you really can't even add a feature that requires
an in-place upgrade hack to fix without first having the code that
performs said hack done. Otherwise you're never completely sure that
you put the right catalog pieces and related support code into the
version you want to upgrade from. This is why it's not unheard of for
commercial database products to require a working in-place upgrade code
*before* the feature change gets committed.

In this case, we get a lucky break in that it's easy to leave support
for old path in there and punt the problem for now. I hope that we all
learn something useful about this class of issue during this opportunity
to get away with that with little downside.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On Sat, Feb 13, 2010 at 3:34 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> Recording some bookkeeping information in pg_class so that pg_migrator can
>> tell what's going on
>> at a glance seems like the right approach, but I'm fuzzy on the details.
>
> November of 2008 was a pretty good month for me, so I enjoyed this flashback
> to it.  That's when the path for how to handle space reservation wandered to
> this same place and then died there:  how to know for sure what information
> to put into the catalog for the upgrade utility, before the upgrade utility
> exists.  Message from Bruce at
> http://archives.postgresql.org/message-id/200901300457.n0U4v1707979@momjian.us
> and my follow-up summarized/linked to the highlights of the earlier
> discussion on that one.

Sure. I think there's an a critical difference between the two
discussions: the framework I'm proposing is general and applicable to
almost any upgrade situation that changes the ODF in any way, and
provides a general way of eventually desupporting ODFs we no longer
want. The previous discussion was about a space reservation system
which couldn't be made to work for a variety of reasons, including
uncertainty about what the future needs might be, and lack of any sort
of bookkeeping system (such as the one I'm proposing here) for
tracking the current state of the system.

> If you think through the implications of that far enough, eventually you
> start to realize that you really can't even add a feature that requires an
> in-place upgrade hack to fix without first having the code that performs
> said hack done.  Otherwise you're never completely sure that you put the
> right catalog pieces and related support code into the version you want to
> upgrade from.  This is why it's not unheard of for commercial database
> products to require a working in-place upgrade code *before* the feature
> change gets committed.

Agreed.

> In this case, we get a lucky break in that it's easy to leave support for
> old path in there and punt the problem for now.  I hope that we all learn
> something useful about this class of issue during this opportunity to get
> away with that with little downside.

Yep.

...Robert

Greg Smith wrote:
> If you think through the implications of that far enough, eventually you
> start to realize that you really can't even add a feature that requires
> an in-place upgrade hack to fix without first having the code that
> performs said hack done. Otherwise you're never completely sure that
> you put the right catalog pieces and related support code into the
> version you want to upgrade from. This is why it's not unheard of for
> commercial database products to require a working in-place upgrade code
> *before* the feature change gets committed.
>
> In this case, we get a lucky break in that it's easy to leave support
> for old path in there and punt the problem for now. I hope that we all
> learn something useful about this class of issue during this opportunity
> to get away with that with little downside.

Yea, the crux of the matter is that we are getting away easy with 9.0 in
only having to keep around some MOVE_* code in tqual.c. This is just
the start of the pain we will have to bear for inplace upgrades. :-(

The MOVE_* bits go away after a while by vacuum and there is an easy
solution for 9.1 --- vacuum everything in 9.0. Where things really get
hard is when we have to support two page formats or two data formats in
the same database. You might think we will never get there, but there
have been such changes in the past, and I suspect that we will have them
in the future, maybe not in 9.1, but perhaps 9.3.

Ultimately we are going to have to decide how to resolve the burden of
code used just for binary upgrades, and as Tom pointed out, it is very
hard to remove the old data format in the old database because new
sessions could be creating it while it is being removed. It seems that
only the next major version can clean out the old format, meaning you
have to keep support for the old format around for a full major release,
add code to remove it in that major release too, then remove all of the
code in the _next_ major release. This is frankly a complexity we have
never had to deal with before, and we don't even have the infrastructure
to track that all of the old format is gone.

So, in summary, MOVE_* problems look minor compared to the complexities
ahead.

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

+ If your life is a hard drive, Christ can be your backup. +

On Tue, Feb 16, 2010 at 2:04 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> The MOVE_* bits go away after a while by vacuum and there is an easy
> solution for 9.1 --- vacuum everything in 9.0.  Where things really get
> hard is when we have to support two page formats or two data formats in
> the same database.  You might think we will never get there, but there
> have been such changes in the past, and I suspect that we will have them
> in the future, maybe not in 9.1, but perhaps 9.3.

I think a O(size of database) step in the upgrade process is
acceptable iff it can be performed while the database is operational.

In this case that would mean having some code in 8.4.3 to prevent
VACUUM FULL from being used once a flag indicating that a migration is
under way. Then you would have to vacuum every table which would set a
flag indicating that no MOVED_* bits were set. Then pg_migrator would
check that that flag was set on every table before allowing you to
migrate.

This might actually be a reasonable thing to put in 9.0. We already
have the code to prevent you from running VACUUM FULL -- namely that
it doesn't exist any longer. And I think we can tell whether there are
any MOVED_* bits set by looking at the vacuum freeze age of the table.
The only thing we're missing is the youngest xid seen in 8.4 before
the 9.0 migration.

--
greg

Greg Stark wrote:
> On Tue, Feb 16, 2010 at 2:04 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > The MOVE_* bits go away after a while by vacuum and there is an easy
> > solution for 9.1 --- vacuum everything in 9.0. ?Where things really get
> > hard is when we have to support two page formats or two data formats in
> > the same database. ?You might think we will never get there, but there
> > have been such changes in the past, and I suspect that we will have them
> > in the future, maybe not in 9.1, but perhaps 9.3.
>
> I think a O(size of database) step in the upgrade process is
> acceptable iff it can be performed while the database is operational.
>
> In this case that would mean having some code in 8.4.3 to prevent
> VACUUM FULL from being used once a flag indicating that a migration is
> under way. Then you would have to vacuum every table which would set a
> flag indicating that no MOVED_* bits were set. Then pg_migrator would
> check that that flag was set on every table before allowing you to
> migrate.
>
> This might actually be a reasonable thing to put in 9.0. We already
> have the code to prevent you from running VACUUM FULL -- namely that
> it doesn't exist any longer. And I think we can tell whether there are
> any MOVED_* bits set by looking at the vacuum freeze age of the table.
> The only thing we're missing is the youngest xid seen in 8.4 before
> the 9.0 migration.

That might work for this case, but I think long-term we will need to do
such changes in the _next_ major release, and add some mechanism that
pg_migrator could test to know that the old format is gone. I don't
think backpatching to minor releases is really sustainable.

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

+ If your life is a hard drive, Christ can be your backup. +