Re: misleading error message in ProcessUtilitySlow T_CreateStatsStmt

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> I propose to use this report:

> ERROR:  cannot create statistics on specified relation
> DETAIL:  CREATE STATISTICS only supports tables, materialized views, foreign tables, and partitioned tables.

WFM, although I think you could shorten it to "tables, materialized
views, and foreign tables".  We generally expect that partitioned
tables are included when saying "tables", no?  I'm not dead set on
that either way, though.

> While looking at this whole thing, I noticed that this code is somewhat
> bogus in a different way: we're resolving the relation name twice, both
> here in ProcessUtilitySlow() (to pass to transformStatsStmt), and later
> again inside CreateStatistics().  It's really strange that we decided
> not to pass the relation Oids as a list argument to CreateStatistics.  I
> suggest to change that as the first attached patch.

I think this is a good idea, but I'm not sure that the locking
considerations are straight in this version.  Once we translate a
relation name to OID, we'd better hold lock on the rel continuously to
the end of usage of that OID.  It looks like you do, but then couldn't
the

+        rel = relation_open(relid, ShareUpdateExclusiveLock);

in CreateStatistics use NoLock to signify that we expect to have
the lock already?

Alternatively, maybe the right fix is to move the parse-analysis
work into CreateStatistics altogether.  I think there is not a
very good argument for ProcessUtilitySlow doing all that stuff.
It's supposed to mainly just be a dispatching switch(), IMO.

            regards, tom lane