On 5/11/17, 7:20 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
> Browsing the code....
Thanks for taking a look.
> It seems to me that you don't need those extra square brackets around
> the column list. Same remark for vacuum.sgml.
I’ll remove them. My intent was to ensure that we didn’t accidentally suggest that statements like “VACUUM , foo,
bar;”were accepted, but the extra brackets don’t seem to fix that, and I don’t foresee much confusion anyway.
> In short for all that, it is enough to have "[, ... ]" to document
> that a list is accepted.
That makes sense, I’ll fix it.
> It seems to me that it would have been less invasive to loop through
> vacuum() for each relation. Do you foresee advantages in allowing
> vacuum() to handle multiple? I am not sure if is is worth complicating
> the current logic more considering that you have as well so extra
> logic to carry on option values.
That was the approach I first prototyped. The main disadvantage that I found was that the command wouldn’t fail-fast
ifone of the tables or columns didn’t exist, and I thought that it might be frustrating to encounter such an error in
themiddle of vacuuming several large tables. It’s easy enough to change the logic to emit a warning and simply move on
tothe next table, but that seemed like it could be easily missed among the rest of the vacuum log statements
(especiallywith the verbose option specified). What are your thoughts on this?
In the spirit of simplifying things a bit, I do think it is possible to eliminate one of the new node types, since the
fieldsfor each are almost identical.
> + * used for error messages. In the case that relid is valid, rels
> + * must have exactly one element corresponding to the specified relid.
> s/rels/relations/ as variable name?
Agreed, that seems nicer.
Nathan