New VACUUM FULL
in [PgSql]
|
From: Simon Riggs on 4 Dec 2009 04:20 On Tue, 2009-12-01 at 01:43 -0800, Jeff Davis wrote: > Marking as ready. You're saying its ready, yet there are 3 additional suggestions patches attached here. Please can you resolve these and re-submit a single final patch from author and reviewer? I will review and eventually commit this, if appropriate. It is either 1st or 2nd in my queue, unless someone else grabs it first. Review comments * What happens if you issue VACUUM FULL; which we would expect to use the new method of vacuum on all tables in the database. Won't that just fail with an error when it comes to catalog tables? Sounds to me like we should avoid the error and just silently do an INPLACE on catalog tables. * Such a pivotal change to Postgres needs more test coverage than a single line in regression tests. It might have been OK before, but I think we need a few more combinations here, at least in this release: with, without indexes, empty table, clustered, non-clustered etc and of course a full database VACUUM so that we have the catalog table case covered, plus an explicit catalog table vacuum. -- Simon Riggs 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 4 Dec 2009 12:56 On Fri, 2009-12-04 at 09:20 +0000, Simon Riggs wrote: > On Tue, 2009-12-01 at 01:43 -0800, Jeff Davis wrote: > > Marking as ready. > > You're saying its ready, yet there are 3 additional suggestions patches > attached here. Please can you resolve these and re-submit a single final > patch from author and reviewer? My apologies. At the time, I thought a couple days might matter, and the changes are in areas that committers tend to editorialize anyway: docs and a style issue. The only substantial patch was to vacuumdb.c. Complete patch attached including my edits. > * What happens if you issue VACUUM FULL; which we would expect to use > the new method of vacuum on all tables in the database. Won't that just > fail with an error when it comes to catalog tables? Sounds to me like we > should avoid the error and just silently do an INPLACE on catalog > tables. That's how it works. > * Such a pivotal change to Postgres needs more test coverage than a > single line in regression tests. It might have been OK before, but I > think we need a few more combinations here, at least in this release: > with, without indexes, empty table, clustered, non-clustered etc and of > course a full database VACUUM so that we have the catalog table case > covered, plus an explicit catalog table vacuum. It was my impression that the regression tests aren't meant to be exhaustive, but merely exercise a good portion of the code to help detect simple breakage. Also, pg_regress isn't good for detecting a lot of the problems that vacuum might have (how do you even know whether the vacuum happened in-place or not?). We could put a VACUUM FULL; and a VACUUM (FULL INPLACE); somewhere, which will cover a lot of the cases you're talking about. However, that may be a performance consideration especially for people who develop on laptops. In general, though, I think the right place for this is a longer test suite that is meant to be run less frequently. Regards, Jeff Davis
From: Simon Riggs on 4 Dec 2009 13:36 On Fri, 2009-12-04 at 09:56 -0800, Jeff Davis wrote: > We could put a VACUUM FULL; and a VACUUM (FULL INPLACE); somewhere, > which will cover a lot of the cases you're talking about. However, that > may be a performance consideration especially for people who develop on > laptops. Let's check it works before worrying about performance. We can take tests out as well as add them once it becomes obvious its working. -- Simon Riggs 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 4 Dec 2009 18:07 On Fri, 2009-12-04 at 18:36 +0000, Simon Riggs wrote: > Let's check it works before worrying about performance. We can take > tests out as well as add them once it becomes obvious its working. Itagaki-san, perhaps you should add a variety of tests, and then Simon can remove extra tests after he's convinced that it works. I tested a variety of situations during my review, and everything worked as I expected. 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: Michael Glaesemann on 4 Dec 2009 19:49
On Dec 4, 2009, at 18:07 , Jeff Davis wrote: > On Fri, 2009-12-04 at 18:36 +0000, Simon Riggs wrote: >> Let's check it works before worrying about performance. We can take >> tests out as well as add them once it becomes obvious its working. > > Itagaki-san, perhaps you should add a variety of tests, and then Simon > can remove extra tests after he's convinced that it works. > > I tested a variety of situations during my review, and everything > worked > as I expected. Would there be a way for you to package the scenarios you tested into a suite? Michael Glaesemann grzm seespotcode net -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |