Re: [HACKERS] [Proposal] Allow users to specify multiple tables inVACUUM commands - Mailing list pgsql-hackers

On 9/9/17, 7:28 AM, "Michael Paquier" <michael.paquier@gmail.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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers