Re: pg_dumpall --exclude-database option

On 08/03/2018 05:08 PM, Fabien COELHO wrote:
>
>> Among other use cases, this is useful where a database name is 
>> visible but the database is not dumpable by the user. Examples of 
>> this occur in some managed Postgres services.
>
> This looks like a reasonable feature.


Thanks for the review.

>
>> I will add this to the September CF.
>
> My 0.02€:
>
> Patch applies cleanly, compiles, and works for me.
>
> 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.

>
> 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.

>
> 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.

>
> Function "database_excluded": I'd suggest to consider reusing the
> "simple_string_list_member" function instead of reimplementing it in a 
> special case.


done.

>
> XML doc: "--exclude-database=dbname", ISTM that 
> "--exclude-database=pattern" would be closer to what it is? "Multiple 
> database can be matched by writing multiple switches". Sure, but it 
> can also be done with a pattern. The documentation seems to assume 
> that the argument is one database name, and then changes this 
> afterwards. I'd suggest to start by saying that a pattern like psql is 
> expected, and then proceed to simply tell that the option can be 
> repeated, instead of implying that it is a dbname and then telling 
> that it is a pattern.

docco revised.

>
> The simple list is not freed. Ok, it seems to be part of the design of 
> the data structure.

I don't see much point in freeing it.

revised patch attached.

cheers

andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services