Thank you for all your reviews! >>> PATTERN should be changed to SCHEMA because -n and -N options don't support >>> pattern matching for schema names. The attached patch 0001 fixes this. >> >> True, there is no pattern matching performed. I wonder if it's worth lifting >> the pattern matching from pg_dump into common code such that tools like this >> can use it? > > I agree that this should be changed to SCHEMA. It might be tough to add > pattern matching with the current catalog query, and I don't know whether > there is demand for such a feature, but I wouldn't discourage someone from > trying.
I think that supporting pattern matching is quite nice. But it will be not only tough but also a breaking change, I wonder. So I guess this change should be commited either way. >>> Yeah, I think we can fix the JOIN as you suggest. I quickly put a patch >>> together to demonstrate. > > Looks good from a quick skim. I do agree with this updates. Thank you! >> We should probably add some tests... > > Agreed. The attached patch includes new tests for this bug. Also, I fixed the current test for -N option seems to be incorrect. >>> It seems to work fine. However, if we're aiming for consistent >>> spacing, the "IS NULL" (two spaces in between) might be an concern. >> >> I don't think that's a problem. I would rather have readable C code and two >> spaces in the generated SQL than contorting the C code to produce less >> whitespace in a query few will read in its generated form. > > I think we could pretty easily avoid the extra space and keep the C code > relatively readable. These sorts of things bug me, too (see 2af3336). Though I don't think it affects readability, I'm neutral about this. >> >> Third, for the description of the -N option, I wonder if "vacuum all tables except >> >> in the specified schema(s)" might be clearer. The current one says nothing about >> >> tables not in the specified schema. >> > >> > Maybe, but the point of vacuumdb is to analyze a database so I'm not sure who >> > would expect anything else than vacuuming everything but the excluded schema >> > when specifying -N. What else could "vacuumdb -N foo" be interpreted to do >> > that can be confusing? >> >> I agree with Daniel on this one. > > +1. That make sense. I retract my suggestion.
v1_vacuumdb_add_tests.patch
Description: Binary data
