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

On 5/16/17, 11:21 PM, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com> wrote:
> 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.

Agreed.

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

Agreed. Unfortunately, I think this is already possible when you specify a table to be vacuumed.

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

Correct. This means that both the relations and columns specified would cause an ERROR if they do not exist and a WARNING if they disappear before we can actually process them.

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

I agree with you here, too. I stopped short of allowing customers to explicitly provide per-table options, so the example you provided wouldn’t work here. This is more applicable for something like the following:

VACUUM (FREEZE, VERBOSE) foo, bar (a);

In this case, the FREEZE and VERBOSE options are used for both tables. However, we have a column list specified for ‘bar’, and the ANALYZE option is implied when we specify a column list. So when we process ‘bar’, we need to apply the ANALYZE option, but we do not need it for ‘foo’. For now, that is all that this per-table options variable is used for.

Nathan