New VACUUM FULL
in [PgSql]
|
From: Jeff Davis on 27 Nov 2009 18:12 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
From: Itagaki Takahiro on 30 Nov 2009 00:38 Thanks for review! Jeff Davis <pgsql(a)j-davis.com> wrote: > * "VACUUM (FULL REPLACE) pg_class" should be rejected, not silently > turned into "VACUUM (FULL INPLACE) pg_class". Hmmm, it requires to remember whether REPLACE is specified or not for the non-INPLACE vacuum, but I don't want to add VACOPT_REPLACE only for the purpose. I just removed "FULL REPLACE" syntax; Only "FULL" and "FULL INPLACE" are available. VACUUM FULL without INPLACE behaves as cluster-like rewrites for non-system tables. FULL INPLACE is a traditional vacuum full. System catalogs are always vacuumed with INPLACE version. - VACUUM FULL / VACUUM (FULL) => rewritten version - VACUUM (FULL INPLACE) => traditional version > * 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. It might make the code cleaner, but I want vacuumdb in 8.5 to work on older versions of servers unless we use the new feature. Older servers can only accept older syntax, so I avoided using the new syntax if not needed. (The new patch still uses two versions of syntax.) > * 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. > * There are some windows line endings in the patch, which should be > removed. Done. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
From: Greg Smith on 30 Nov 2009 15:10 Itagaki Takahiro wrote: > Done. (vacuum-full_20091130.patch) > Is this ready for a committer now? Not sure whether Jeff intends to re-review here or not, given that the suggestions and their fixes were pretty straightforward. It looks pretty solid at this point to me. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Jeff Davis on 30 Nov 2009 16:22 On Mon, 2009-11-30 at 15:10 -0500, Greg Smith wrote: > Itagaki Takahiro wrote: > > Done. (vacuum-full_20091130.patch) > > > Is this ready for a committer now? Not sure whether Jeff intends to > re-review here or not, given that the suggestions and their fixes were > pretty straightforward. It looks pretty solid at this point to me. The code is in good shape. I was going to take another stab at the documentation (which needs some rewording after the changes), and maybe look at vacuumdb again, as well. Nothing major, so expect it to be "ready for committer" tonight. Of course, a committer can take a look at it sooner, if they have time. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Jeff Davis on 1 Dec 2009 04:43
On Mon, 2009-11-30 at 14:38 +0900, Itagaki Takahiro wrote: > > * "VACUUM (FULL REPLACE) pg_class" should be rejected, not silently > > turned into "VACUUM (FULL INPLACE) pg_class". > > Hmmm, it requires to remember whether REPLACE is specified or not > for the non-INPLACE vacuum, but I don't want to add VACOPT_REPLACE > only for the purpose. > > I just removed "FULL REPLACE" syntax; Only "FULL" and "FULL INPLACE" are > available. VACUUM FULL without INPLACE behaves as cluster-like rewrites > for non-system tables. FULL INPLACE is a traditional vacuum full. > System catalogs are always vacuumed with INPLACE version. > - VACUUM FULL / VACUUM (FULL) => rewritten version > - VACUUM (FULL INPLACE) => traditional version Ok, looks good. It's cleaner now, too. > It might make the code cleaner, but I want vacuumdb in 8.5 to work on older > versions of servers unless we use the new feature. Older servers can only > accept older syntax, so I avoided using the new syntax if not needed. > (The new patch still uses two versions of syntax.) Good point. I attached a suggestion of how it might look if you detected the server version explicitly. You don't have to use it, but it's what I had in mind. Also, I think the current version fails if -i is passed and it's connecting to an old server, so explicit server version detection may be required. > > * 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(). Very minor style issue: it looks like Tom specifically changed the order of the expression in the Assert() from your first vacuum options patch. I attached a diff to show you what I mean -- the complex boolean expressions are easier to read if the styles match. > > * 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. > > * There are some windows line endings in the patch, which should be > > removed. Great, thank you for the patch! Marking as ready. Regards, Jeff Davis |