(WIP) VACUUM REWRITE - CLUSTER by ctid

Itagaki Takahiro wrote:

I'm working on alternative version of VACUUM FULL, which is
like CLUSTER but sort tuples in ctid order without index.
The original discussion is here:
[HACKERS] Feedback on getting rid of VACUUM FULL
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01047.php

WIP patch attached. I have some questions over the development:

1. Syntax: I choose "CLUSTER tbl WITHOUT INDEX" for the syntax,
but it is debatable. What syntax is the best?
VACUUM REWRITE? CLUSTER ORDER BY ctid? or replace VACUUM FULL?

I got the impression that replacing VACUUM FULL is the most popular
opinion. I like VACUUM REWRITE myself, except that it would require
making REWRITE a reserved keyword. I don't like tacking this onto
CLUSTER, particularly not with "ORDER BY ctid". ctids are an
implementation detail most users don't care about, and ORDER BY sounds
like it's sorting something, but it's not.

2. Superclass of HeapScanDesc and IndexScanDesc:
We don't have an abstraction layer of HeapScanDesc and IndexScanDesc,
but the layer is useful for this purpose. Is it reasonable?
It is partially implemented as genam_beginscan() family in the patch.

I don't think it's really necessary. You might as well put a few "if
(indexOid)" clauses directly into copy_heap_data.

3. Should we allow "ctid" as a default clustered index?
We could assume "ctid" as a virtual index. The syntax for it
might be "ALTER TABLE tbl CLUSTER ON COLUMN ctid" or so.

Isn't that the same as having no clustering index? We already have
"ALTER TABLE tbl SET WITHOUT CLUSTER".

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

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:

I got the impression that replacing VACUUM FULL is the most popular
opinion. I like VACUUM REWRITE myself, except that it would require
making REWRITE a reserved keyword.

My next proposal for the syntex is "VACUUM (options) table_name".
Since "options" are quoted by '(' and ')', we can add new options
without adding them into reserved keywords.

The traditional vacuum syntax:
VACUUM FULL FREEZE VERBOSE ANALYZE table_name (columns);
will be:
VACUUM (FULL, FREEZE, VERBOSE, ANALYZE) table_name (columns);

I think the syntax is consistent with existing syntex of "EXPLAIN (...)".
We can choose any keyword for the new "rewrite" version.
For example:
* VACUUM ( REWRITE )
* VACUUM ( FULL [ INPLACE | REPLACE ] )

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Here is a patch to support "rewrite" version of VACUUM FULL.
It was called "VACUUM REWRITE" in the past disucussin,
but I choose the following syntax for now:

VACUUM ( FULL [ INPLACE | REPLACE ] ) [ table_name ]

The reason is to contrast the new REPLACE behavior with the old INPLACE
behavior. I cannot find any good terms of opposite meaning of REWRITE.

The new version works like as CLUSTER USING ctid or rewriting in
ALTER TABLE. It must be faster than them if we have many dead tuples
and are not interested in physical order of tuples.

We still need traditional VACUUM FULL behavior for system catalog because
we cannot change relfilenode for them. Also, VACUUM FULL REPLACE is not
always better than traditional VACUUM FULL; the new version requires
additional disk space and might be slower if we have a few dead tuples.

"VACUUM FULL" and "VACUUM (FULL)" are synonyms for "VACUUM (FULL REPLACE)",
but if the target table is s system catalog, instead "FULL INPLACE" is
used automatically.

If this approach is reasonable, I'll go for other supplemental parts.
(documentations, vacuumdb, etc.)

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Tue, 2009-10-27 at 13:55 +0900, Itagaki Takahiro wrote:

Here is a patch to support "rewrite" version of VACUUM FULL.
It was called "VACUUM REWRITE" in the past disucussin,
but I choose the following syntax for now:

VACUUM ( FULL [ INPLACE | REPLACE ] ) [ table_name ]

The reason is to contrast the new REPLACE behavior with the old INPLACE
behavior. I cannot find any good terms of opposite meaning of REWRITE.

The new version works like as CLUSTER USING ctid or rewriting in
ALTER TABLE. It must be faster than them if we have many dead tuples
and are not interested in physical order of tuples.

We still need traditional VACUUM FULL behavior for system catalog because
we cannot change relfilenode for them. Also, VACUUM FULL REPLACE is not
always better than traditional VACUUM FULL; the new version requires
additional disk space and might be slower if we have a few dead tuples.

"VACUUM FULL" and "VACUUM (FULL)" are synonyms for "VACUUM (FULL REPLACE)",
but if the target table is s system catalog, instead "FULL INPLACE" is
used automatically.

If this approach is reasonable, I'll go for other supplemental parts.
(documentations, vacuumdb, etc.)

Rough approach looks fine to me.

The internal logic is fairly hard to read. I'd suggest you have option
flags VACUUM_FULL and VACUUM_REPLACE, rather than VACUUM_INPLACE and
VACUUM_REPLACE. So VACUUM_REPLACE can only be set iff VACUUM_FULL. That
will reduce much of the logic.

--
Simon Riggs www.2ndQuadrant.com

Itagaki Takahiro wrote:

Here is a patch to support "rewrite" version of VACUUM FULL.
It was called "VACUUM REWRITE" in the past disucussin,
but I choose the following syntax for now:

VACUUM ( FULL [ INPLACE | REPLACE ] ) [ table_name ]

The reason is to contrast the new REPLACE behavior with the old INPLACE
behavior. I cannot find any good terms of opposite meaning of REWRITE.

I thought the idea is to rip out the current implementation altogether.
If that's the case, then it doesn't make sense to use a different
syntax. Just rip the innards of VACUUM FULL and replace them with your
new implementation.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Itagaki Takahiro wrote:

We still need traditional VACUUM FULL behavior for system catalog because
we cannot change relfilenode for them. Also, VACUUM FULL REPLACE is not
always better than traditional VACUUM FULL; the new version requires
additional disk space and might be slower if we have a few dead tuples.

Tom was saying that we could fix the problem that relfilenode could not
be changed by having a flat file filenode map. It would only be needed
for nailed system catalogs (the rest of the tables grab their
relfilenode from pg_class as usual) so it wouldn't have the problems
that the previous flatfiles had (which was that they could grow too
much). I don't recall if this got implemented (I don't think it did).

As for it being slower with few dead tuples, I don't think this is a
problem -- just use lazy vacuum in that case.

I also remember we agreed on something about the need for extra disk
space, but I can't remember what it was.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.

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

Alvaro Herrera <alvherre@commandprompt.com> wrote:

BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.

It's a good idea. I separated the part into the attached patch.
It replaces VacuumStmt's "vacuum", "full", "analyze", and "verbose"
fields into one "options" flag field.

The only user-visible change is to support "VACUUM (options)" syntax:
VACUUM ( FULL, FREEZE, VERBOSE, ANALYZE ) table (columns)
We don't bother with the order of options in this form :)

There is a debatable issue that we can use "VACUUM (VERBOSE) table (col)"
in the abobe syntax. Columns are specified but no ANALYZE option there.
An ANALYZE option is added automatically in the current implementation,
but we should have thrown an syntax error in such case.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Fri, 2009-11-13 at 10:47 +0900, Itagaki Takahiro wrote:

Alvaro Herrera <alvherre@commandprompt.com> wrote:

BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.

It's a good idea. I separated the part into the attached patch.
It replaces VacuumStmt's "vacuum", "full", "analyze", and "verbose"
fields into one "options" flag field.

The only user-visible change is to support "VACUUM (options)" syntax:
VACUUM ( FULL, FREEZE, VERBOSE, ANALYZE ) table (columns)
We don't bother with the order of options in this form :)

There is a debatable issue that we can use "VACUUM (VERBOSE) table (col)"
in the abobe syntax. Columns are specified but no ANALYZE option there.
An ANALYZE option is added automatically in the current implementation,
but we should have thrown an syntax error in such case.

I have just begun review by reading some of the previous threads.

I'd like to try to summarize the goals we have for VACUUM to put these
patches in perspective:

1. Implement a rewrite version of VACUUM FULL, which is closer to
CLUSTER.

2. Use the new options syntax, to make the order of vacuum options
irrelevant.

3. Implement several flatfile maps to allow the rewrite version of
VACUUM FULL to work on catalogs:
http://archives.postgresql.org/message-id/19750.1252094460@sss.pgh.pa.us

4. Implement some kind of concurrent tuple mover that can slowly update
tuples and lazily VACUUM in such a way that they migrate to lower heap
pages (assuming no long-running transactions). This would be done with
normal updates (new xmin) and would not remove the old tuple until it
was really dead; otherwise there are concurrency problems. Such a
utility would be useful in cases where a very small fraction of tuples
need to be moved, or you don't have enough space for a rewrite.

5. Remove VACUUM FULL (in it's current form) completely.

Some other ideas also came out of the thread, like:

* Provide a way to truncate the dead space from the end of a relation in
a blocking manner. Right now, lazy vacuum won't shrink the file unless
it can acquire an exclusive lock without waiting, and there's no way to
actually make it wait. This can be a separate command, not necessarily a
part of VACUUM.

* Josh Berkus suggested allowing the user to specify a different
tablespace in which to rebuild the tablespace.

I'll begin looking at the patches themselves now, which implement #1 and
#2.

If we can implement enough of these features (say, #3 also) to remove
the current form of VACUUM FULL, then we can just call the new feature
VACUUM FULL, and save ourselves from syntactical headaches.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

I'd like to try to summarize the goals we have for VACUUM to put these
patches in perspective:

Good summary, but ...

Some other ideas also came out of the thread, like:

* Provide a way to truncate the dead space from the end of a relation in
a blocking manner. Right now, lazy vacuum won't shrink the file unless
it can acquire an exclusive lock without waiting, and there's no way to
actually make it wait. This can be a separate command, not necessarily a
part of VACUUM.

What I remembered was actually closer to the opposite: people are
concerned about lazy vac holding the exclusive lock too long once it
does acquire it. In a manual vacuum this leads to unexpected blockage
of concurrent queries, and in an autovac this leads to a forced cancel
preventing autovac from completing the operation (which means no
space removal at all, and no stats update either). The code is designed
on the assumption that it won't spend very long holding the ex lock,
and so I think a reasonable TODO item is to ensure that by having it
limit how many blocks it will scan during the shrinkage attempt.

FWIW, once we provide the extensible option syntax, it doesn't seem
like it'd be hard to have an option to make it wait for the lock,
so I'd recommend that approach over anything with a separate command.

regards, tom lane

On Fri, 2009-11-13 at 10:47 +0900, Itagaki Takahiro wrote:

Alvaro Herrera <alvherre@commandprompt.com> wrote:

BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.

It's a good idea. I separated the part into the attached patch.
It replaces VacuumStmt's "vacuum", "full", "analyze", and "verbose"
fields into one "options" flag field.

I added a separate commitfest item for this patch to track it separately
from the rewrite version of VACUUM:

https://commitfest.postgresql.org/action/patch_view?id=222

The only user-visible change is to support "VACUUM (options)" syntax:
VACUUM ( FULL, FREEZE, VERBOSE, ANALYZE ) table (columns)
We don't bother with the order of options in this form :)

I looked at this patch. You left INPLACE in the patch, which looks like
an oversight when you were separating the two. Please remove that from
this part.

There is a debatable issue that we can use "VACUUM (VERBOSE) table (col)"
in the abobe syntax. Columns are specified but no ANALYZE option there.
An ANALYZE option is added automatically in the current implementation,
but we should have thrown an syntax error in such case.

Sounds fine, but worth a mention in the documentation. Just add to the
"columns" part of the VACUUM page something like: "If specified, implies
ANALYZE".

Other than these two minor issues, I don't see any problems with the
patch. Please post an updated version to the new commitfest entry.

Regards,
Jeff Davis

On Tue, 2009-10-27 at 13:55 +0900, Itagaki Takahiro wrote:

Here is a patch to support "rewrite" version of VACUUM FULL.

Can you please provide a patch that applies cleanly on top of the vacuum
options patch and on top of CVS HEAD (there are a couple minor conflicts
with this version). That would make review easier.

Initial comments:

1. Do we want to introduce syntax for INPLACE at all, if we are
eventually going to remove the current mechanism? If not, then we should
probably use REWRITE, because that's a better word than "REPLACE", I
think.

My opinion is that if we really still need the current in-place
mechanism, then VACUUM (FULL) should use the current in-place mechanism;
and VACUUM (FULL REWRITE) should use your new rewrite mechanism. That
gives us a nice migration path toward always using the rewrite
mechanism.

2. Why do all of the following exist: VACOPT_FULL, VACOPT_REPLACE, and
VACOPT_INPLACE? Shouldn't VACOPT_FULL be equivalent to one of the other
two? This is essentially what Simon was getting at, I think.

3. Some options are being set in vacuum() itself. It looks like the
options should already be set in gram.y, so should that be an Assert
instead? I think it's cleaner to set all of the options properly early
on, rather than waiting until vacuum() to interpret the combinations.

I haven't looked at the implementation in detail yet, but at a glance,
it seems to be a good approach.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

3. Some options are being set in vacuum() itself. It looks like the
options should already be set in gram.y, so should that be an Assert
instead? I think it's cleaner to set all of the options properly early
on, rather than waiting until vacuum() to interpret the combinations.

As a rule of thumb, I'd recommend keeping as much complexity as possible
*out* of gram.y. It's best if that code just reports the facts, ie what
options the user entered. Deriving conclusions from that (like what
default behaviors should be used) is best done later. One example of
why is that if you want the defaults to depend on GUC settings then
that logic must *not* happen in gram.y, since the parse tree might
outlive the current GUC settings.

I haven't read the patch but it sounds like vacuum() is the right
place for this type of activity.

regards, tom lane

On Sat, 2009-11-14 at 19:25 -0500, Tom Lane wrote:

As a rule of thumb, I'd recommend keeping as much complexity as possible
*out* of gram.y. It's best if that code just reports the facts, ie what
options the user entered. Deriving conclusions from that (like what
default behaviors should be used) is best done later. One example of
why is that if you want the defaults to depend on GUC settings then
that logic must *not* happen in gram.y, since the parse tree might
outlive the current GUC settings.

I was referring to (in vacuum()):

+       if (vacstmt->options & (VACOPT_INPLACE | VACOPT_REPLACE |
VACOPT_FREEZE))
+               vacstmt->options |= VACOPT_VACUUM;
+       if (vacstmt->va_cols)
+               vacstmt->options |= VACOPT_ANALYZE;

I think what you say applies to VACOPT_ANALYZE, which is implied when
columns are supplied by the user but ANALYZE is not specified
explicitly. In that case it should be set in vacuum() but not in gram.y
(unless specified by the user).

However, for VACOPT_VACUUM, I think that's still in the territory of
gram.y.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> wrote:

You left INPLACE in the patch

Oops, removed.

Sounds fine, but worth a mention in the documentation. Just add to the
"columns" part of the VACUUM page something like: "If specified, implies
ANALYZE".

Added.

Other than these two minor issues, I don't see any problems with the
patch. Please post an updated version to the new commitfest entry.

All of the vacuum options are adjusted in gram.y in the current patch.
We only check the options with assertions in vacuum():

/* sanity checks */
Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE));
Assert(!(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)) ||
(vacstmt->options & VACOPT_VACUUM));
Assert(vacstmt->va_cols == NIL || (vacstmt->options & VACOPT_ANALYZE));

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Mon, 2009-11-16 at 13:37 +0900, Itagaki Takahiro wrote:

Jeff Davis <pgsql@j-davis.com> wrote:

You left INPLACE in the patch

Oops, removed.

Sounds fine, but worth a mention in the documentation. Just add to the
"columns" part of the VACUUM page something like: "If specified, implies
ANALYZE".

Added.

Great, I am marking this part "ready for committer".

I may be slow to review the remainder of the VACUUM FULL rewrite patch
due to the conference in Tokyo, but I will review it soon afterward.

Regards,
Jeff Davis

Here is an updated patch of rewriting vacuum based on vacuum options patch.
Documentations and vacuumdb modification (-i, --inplace) are added.

Jeff Davis <pgsql@j-davis.com> wrote:

1. Do we want to introduce syntax for INPLACE at all, if we are
eventually going to remove the current mechanism?
My opinion is that if we really still need the current in-place
mechanism, then VACUUM (FULL) should use the current in-place mechanism;
and VACUUM (FULL REWRITE) should use your new rewrite mechanism.

AFAIK, "VACUUM FULL" should behave as "REWRITE" in the past discussion.
Since we don't want users to use in-place FULL vacuum, so we will change
the default behavior of VACUUM FULL. There are some choices:

<REWRITE version> <in-place version>
1. VACUUM (FULL REPLACE) vs. VACUUM (FULL INPLACE)
2. VACUUM (FULL) vs. VACUUM (FULL INPLACE)
3. VACUUM (REWRITE) vs. VACUUM (FULL)
4. VACUUM (FULL REWRITE) vs. VACUUM (FULL)
5. Don't use SQL and use a GUC instead. (bool inplace_vacuum_full ?)

I choose a hybrid syntax of 1 + 2 in the patch,
but I'm not particular about it. What is the best?

2. Why do all of the following exist: VACOPT_FULL, VACOPT_REPLACE, and
VACOPT_INPLACE? Shouldn't VACOPT_FULL be equivalent to one of the other
two? This is essentially what Simon was getting at, I think.

* FULL [REPLACE] := VACOPT_FULL
* FULL INPLACE := VACOPT_FULL + VACOPT_INPLACE

3. Some options are being set in vacuum() itself. It looks like the
options should already be set in gram.y, so should that be an Assert
instead? I think it's cleaner to set all of the options properly early
on, rather than waiting until vacuum() to interpret the combinations.

I moved all of the logic into gram.y. vacuum() has only assert tests.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Jeff Davis <pgsql@j-davis.com> writes:

On Mon, 2009-11-16 at 13:37 +0900, Itagaki Takahiro wrote:

[ new options syntax for VACUUM ]

Great, I am marking this part "ready for committer".

Applied with very minor editorialization.

regards, tom lane

Review comments:

* I attached some documentation suggestions.
* The patch should be merged with CVS HEAD. The changes required are
minor; but notice that there is a minor style difference in the assert
in vacuum().
* vacuumdb should reject -i without -f
* The replace or inplace option is a "magical" default, because "VACUUM
FULL" defaults to "replace" for user tables and "inplace" for system
tables. I tried to make that more clear in my documentation suggestions.
* "VACUUM (FULL REPLACE) pg_class" should be rejected, not silently
turned into "VACUUM (FULL INPLACE) pg_class".
* There are some windows line endings in the patch, which should be
removed.
* In vacuumdb, the code for handling INPLACE is a little ugly. Perhaps
it should be changed to always use your new options syntax? That might
be more code, but it would be a little more readable.

Regards,
Jeff Davis