Hello Andrew,
>> A question: would it makes sense to have a symmetrical
>> --include-database=PATTERN option as well?
>
> I don't think so. If you only want a few databases, just use pg_dump. The
> premise of pg_dumpall is that you want all of them and this switch provides
> for exceptions to that.
Ok, sounds reasonable.
>> Somehow the option does not make much sense when under -g/-r/-t... maybe it
>> should complain, like it does when the others are used together?
>
> Added an error check.
Ok.
>> ISTM that it would have been better to issue just one query with an OR
>> list, but that would require to extend "processSQLNamePattern" a little
>> bit. Not sure whether it is worth it.
>
> I don't think it is. This uses the same pattern that is used in pg_dump.c for
> similar switches.
Ok.
> revised patch attached.
Patch applies cleanly, compiles, make check ok, pg_dump tap tests ok, doc
build ok.
Very minor comments:
Missing space after comma:
+ {"exclude-database",required_argument, NULL, 5},
Now that C99 is okay, ISTM that both for loops in expand_dbname_patterns
could benefit from using loop-local variables:
for (SimpleStringListCell *cell = ...
for (int i = ...
About the documentation:
"When using wildcards, be careful to quote the pattern if needed to prevent
the shell from expanding the wildcards."
I'd suggest to consider simplifying the end, maybe "to prevent shell
wildcard expansion".
The feature is not tested per se. Maybe one existing tap test could be
extended with minimal fuss to use it, eg --exclude-database='[a-z]*'
should be close to only keeping the global stuff? I noticed an "exclude
table" test already exists.
--
Fabien.
