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

On 9/9/17, 7:28 AM, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com> wrote:
> In the duplicate patch, it seems to me that you can save one lookup at
> the list of VacuumRelation items by checking for column duplicates
> after checking that all the columns are defined. If you put the
> duplicate check before closing the relation you can also use the
> schema name associated with the Relation.

I did this so that the ERROR prioritization would be enforced across all
relations. For example:

VACUUM ANALYZE table1 (a, a), table2 (does_not_exist);

If I combine the 'for' loops to save a lookup, this example behaves
differently. Instead of an ERROR for the nonexistent column, you would
hit an ERROR for the duplicate column in table1's list. However, I do
not mind changing this.

> + if (i == InvalidAttrNumber)
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_COLUMN),
> + errmsg("column \"%s\" of relation \"%s\" does not exist",
> + col, RelationGetRelationName(rel))));
> This could use the schema name unconditionally as you hold a Relation
> here, using RelationGetNamespace().

Sure, I think this is a good idea. I'll make this change in the next
version of the patch.

> if (!onerel)
> + {
> + /*
> + * If one of the relations specified by the user has disappeared
> + * since we last looked it up, let them know so that they do not
> + * think it was actually analyzed.
> + */
> + if (!IsAutoVacuumWorkerProcess() && relation)
> + ereport(WARNING,
> + (errmsg("skipping \"%s\" --- relation no longer exists",
> + relation->relname)));
> +
> return;
> + }
> Hm. So if the relation with the defined OID is not found, then we'd
> use the RangeVar, but this one may not be set here:
> + /*
> + * It is safe to leave everything except the OID empty here.
> + * Since no tables were specified in the VacuumStmt, we know
> + * we don't have any columns to keep track of. Also, we do
> + * not need the RangeVar, because it is only used for error
> + * messaging when specific relations are chosen.
> + */
> + rel_oid = HeapTupleGetOid(tuple);
> + relinfo = makeVacuumRelation(NULL, NIL, rel_oid);
> + vacrels_tmp = lappend(vacrels_tmp, relinfo);
> So if the relation is analyzed but skipped, we would have no idea that
> it actually got skipped because there are no reports about it. That's
> not really user-friendly. I am wondering if we should not instead have
> analyze_rel also enforce the presence of a RangeVar, and adding an
> assert at the beginning of the function to undertline that, and also
> do the same for vacuum(). It would make things also consistent with
> vacuum() which now implies on HEAD that a RangeVar *must* be
> specified.

I agree that it is nice to see when relations are skipped, but I do not
know if the WARNING messages would provide much value for this
particular use case (i.e. 'VACUUM;'). If a user does not provide a list
of tables to VACUUM, they might not care too much about WARNINGs for
dropped tables.

> Are there opinions about back-patching the patch checking for
> duplicate columns? Stable branches now complain about an unhelpful
> error message.

I wouldn't mind drafting something up for the stable branches.

Nathan