On 1/7/19, 1:12 AM, "Michael Paquier" <mich...@paquier.xyz> wrote: > I have been looking at the patch set, and 0001 can actually happen > only once 0005 is applied because this modifies the query doing on > HEAD a full scan of pg_class which would include at least catalog > tables so it can never be empty. For this reason it seems to me that > 0001 and 0004 should be merged together as common refactoring, making > sure on the way that all relations exist before processing anything. > As 0005 and 0006 change similar portions of the code, we could also > merge these together in a second patch introducing the new options. > Keeping 0001, 0004, 0005 and 0006 separated eases the review, but I > would likely merge things when they make sense together to reduce > diff chunks.
Thanks for reviewing. Sure, I can merge these together so that it's just 2 patches. > 0002 removes some carefully-written query generation, so as it is > never possible to generate something like ANALYZE FULL. HEAD splits > ANALYZE and VACUUM clearly, but 0002 merges them together so as > careless coding in vacuumdb.c makes it easier to generate command > strings which would fail in the backend relying on the option checks > to make sure that for example combining --full and --analyze-only > never happens. Introducing 0002 is mainly here for 0003, so let's > merge them together. Makes sense. I was trying to simplify this code a bit, but I agree with your point about relying on the option checks. > From patch 0004: > + executeCommand(conn, "RESET search_path;", progname, echo); > + res = executeQuery(conn, catalog_query.data, progname, echo); > + termPQExpBuffer(&catalog_query); > + PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL, > + progname, echo)); > We should really avoid that. There are other things like casts which > tend to be forgotten, and if the catalog lookup query gets more > complicated, I feel that this would again be forgotten, reintroducing > the issues that ALWAYS_SECURE_SEARCH_PATH_SQL is here to fix. This was done in order to maintain the current behavior that appendQualifiedRelation() gives us. I found that skipping the search_path handling here forced us to specify the schema in the argument for --table in most cases. At the very least, I could add a comment here to highlight the importance of fully qualifying everything in the catalog query. What do you think? > I have put my hands into what you sent, and worked on putting > 0002/0003 first into shape, finishing with the attached. I have > simplified the query construction a bit, making it IMO easier to read > and to add new options, with comments documenting what's supported > across versions. I have also added a regression test for > --analyze-only --skip-locked. Then I have done some edit of the docs. > What do you think for this portion? Looks good to me, except for one small thing in the documentation: + <para> + Disable all page-skipping behavior during processing based on + the visibility map, similarly to the option + <literal>DISABLE_PAGE_SKIPPING</literal> for <command>VACUUM</command>. + </para> I think the "similarly to the option" part is slightly misleading. It's not just similar, it _is_ using that option in the generated commands. Perhaps we could point to the VACUUM documentation for more information about this one. On 1/7/19, 8:03 PM, "Michael Paquier" <mich...@paquier.xyz> wrote: > On Mon, Jan 07, 2019 at 06:10:21PM +0900, Masahiko Sawada wrote: >> 0002 and 0003 are merged and posted by Michael-san and it looks good >> to me, so I've looked at the 0001, 0004, 0005 and 0006 patches. Here >> is a few review comments. > > I have done another round on 0002/0003 (PQfinish was lacking after > error-ing on incompatible options) and committed the thing. Thanks! >> Even if all tables are filtered by --min-relation-size, min-mxid-age >> or min-xid-age the following message appeared. >> >> $ vacuumdb --verbose --min-relation-size 1000000 postgres >> vacuumdb: vacuuming database "postgres" >> >> Since no tables are processed in this case isn't is better not to >> output this message by moving the message after checking if ntup == >> 0? > > Hmm. My take on this one is that we attempt to vacuum relations on > this database, but this may finish by doing nothing. Printing this > message is still important to let the user know that an attempt was > done so I would keep the message. +1 for keeping the message. >> You use pg_total_relation_size() to check the relation size but it >> might be better to use pg_relation_size() instead. The >> pg_total_relation_size() includes the all index size but I think it's >> common based on my experience that we check only the table size >> (including toast table) to do vacuum it or not. > > Yes, I am also not completely sure if the way things are defined in > the patch are completely what we are looking for. Still, I have seen > as well people complain about the total amount of space a table is > physically taking on disk, including its indexes. So from the user > experience the direction taken by the patch makes sense, as much as > does the argument you do. Good point. I think allowing multiple different relation size options here would be confusing, too (e.g. --min-relation-size versus --min-total-relation-size). IMO pg_total_relation_size() is the way to go here, as we'll most likely need to process the indexes and TOAST tables, too. If/when there is a DISABLE_INDEX_CLEANUP option for VACUUM, we'd then want to use pg_table_size() when --min-relation-size and --disable-index-cleanup are used together in vacuumdb. Thoughts? I've realized that I forgot to add links to the XID/MXID wraparound documentation like you suggested a while back. I'll make sure that gets included in the next patch set, too. Nathan
