Re: [Proposal] Allow users to specify multiple tables in VACUUM commands

On Wed, May 17, 2017 at 7:56 AM, Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
> I think this issue already exists, as this comment in get_rel_oids(…) seems to indicate:
>
> /*
> * Since we don't take a lock here, the relation might be gone, or the
> * RangeVar might no longer refer to the OID we look up here. In the
> * former case, VACUUM will do nothing; in the latter case, it will
> * process the OID we looked up here, rather than the new one. Neither
> * is ideal, but there's little practical alternative, since we're
> * going to commit this transaction and begin a new one between now
> * and then.
> */
> relid = RangeVarGetRelid(vacrel, NoLock, false);
>
> With the patch applied, I believe this statement still holds true. So if the relation disappears before we get to vacuum_rel(…), we will simply skip it and move on to the next one. The vacuum_rel(…) code provides a WARNING in many cases (e.g. the user does not have privileges to VACUUM the table), but we seem to silently skip the table when it disappears before the call to vacuum_rel(…).

Yes, that's the bits using try_relation_open() which returns NULL if
the relation is gone. I don't think that we want VACUUM to be noisy
about that when running it on a database.

> If we added a WARNING to the effect of “skipping vacuum of <table_name> — relation no longer exists” for this case, I think what you are suggesting would be satisfied.

We would do no favor by reporting nothing to the user. Without any
information, the user triggering a manual vacuum may believe that the
relation has been vacuumed as it was listed but that would not be the
case.

> However, ANALYZE has a slight caveat. While analyze_rel(…) silently skips the relation if it no longer exists like vacuum_rel(…) does, we do not pre-validate the columns list at all. So, in an ANALYZE statement with multiple tables and columns specified, it’ll only fail once we get to the undefined column. To fix this, we could add a check for the column lists near get_rel_oids(…) and adjust do_analyze_rel(…) to emit a WARNING and skip any columns that vanish in the meantime.
>
> Does this seem like a sane approach?
>
> 1. Emit WARNING when skipping if relation disappears before we get to it.
> 2. Early in vacuum(…), check that the specified columns exist.

And issue an ERROR, right?

> 3. Emit WARNING and skip any specified columns that vanish before processing.

This looks like a sensible approach to me.

+ RelationAndColumns *relinfo = (RelationAndColumns *)
lfirst(relation);
+ int per_table_opts = options | relinfo->options; /*
VACOPT_ANALYZE may be set per-table */
+ ListCell *oid;
I have just noticed this bit in your patch. So you can basically do
something like that:
VACUUM (ANALYZE) foo, (FULL) bar;
Do we really want to go down to this level of control? I would keep
things a bit more restricted as users may be confused by the different
lock levels involved by each operation, and make use of the same
options for all the relations listed. Opinions from others is welcome.
--
Michael