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

On Sat, Sep 9, 2017 at 2:05 AM, Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
> On 9/8/17, 1:27 AM, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com> wrote:
>> Thanks. This looks now correct to me. Except that:
>> + ereport(ERROR,
>> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> + errmsg("column lists cannot have duplicate entries"),
>> + errhint("the column list specified for relation
>> \"%s\" contains duplicates",
>> + relation->relation->relname)));
>> This should use ERRCODE_DUPLICATE_COLUMN.
>
> Absolutely. This is fixed in v3.

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.

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

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.

Sorry for noticing that just now, I am switching the patch back to
waiting on author.

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