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

On Mon, Sep 11, 2017 at 2:05 PM, Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
>> + 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().
>
> This is added in v16 of the main patch.
>
>> 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've made these changes in v16 of the main patch.

+ if (include_parts)
+ {
+ List *partition_oids = find_all_inheritors(relid, NoLock, NULL);
+ ListCell *part_lc;
+ foreach(part_lc, partition_oids)
+ {
+ VacuumRelation *tmp = copyObject(relinfo);
+ Oid part_oid = lfirst_oid(part_lc);
+ tmp->oid = part_oid;
+ vacrels_tmp = lappend(vacrels_tmp, tmp);
+ }
+ }
I thought that you would have changed that as well, but that's not
completely complete... In my opinion, HEAD is wrong in using the same
RangeVar for error reporting for a parent table and its partitions, so
that's not completely the fault of your patch. But I think that as
this patch makes vacuum routines smarter, you should create a new
RangeVar using makeRangeVar as you hold the OID of the child partition
in this code path. This would allow error reports to actually use the
data of the partition saved here instead of the parent data.

The rest looks fine to me.
--
Michael