On 5/16/17, 11:21 PM, "Michael Paquier" <[email protected]> wrote:
> On Wed, May 17, 2017 at 7:56 AM, Bossart, Nathan <[email protected]> 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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
