From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: Simon Riggs <simon@2ndquadrant.com>,
        Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
        Tomas Vondra <tomas.vondra@2ndquadrant.com>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>,
        Dean Rasheed <dean.a.rasheed@gmail.com>
Subject: Re: WITH clause in CREATE STATISTICS
In-reply-to: <20170512221010.nglatgt5azzdxjlj@alvherre.pgsql>
References: <20170512221010.nglatgt5azzdxjlj@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@2ndquadrant.com>
	message dated "Fri, 12 May 2017 19:10:10 -0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <20074.1494633784.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 12 May 2017 20:03:04 -0400
Message-ID: <20075.1494633784@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Hm.  That would behave less than desirably if getObjectClass() could
>> return a value that wasn't a member of the enum, but it's hard to
>> credit that happening.  I guess I'd vote for removing the default:
>> case from all of the places that have "switch (getObjectClass(object))",
>> as forgetting to add an entry seems like a much larger hazard.

> Ignoring the one in alter.c's AlterObjectNamespace_oid, which only
> handles a small subset of the enum values, that gives the following
> warnings:

> /pgsql/source/master/src/backend/catalog/dependency.c: In function 'doDel=
etion':
> /pgsql/source/master/src/backend/catalog/dependency.c:1106:2: warning: en=
umeration value 'OCLASS_ROLE' not handled in switch [-Wswitch]
>   switch (getObjectClass(object))
>   ^
> /pgsql/source/master/src/backend/catalog/dependency.c:1106:2: warning: en=
umeration value 'OCLASS_DATABASE' not handled in switch [-Wswitch]
> /pgsql/source/master/src/backend/catalog/dependency.c:1106:2: warning: en=
umeration value 'OCLASS_TBLSPACE' not handled in switch [-Wswitch]

> /pgsql/source/master/src/backend/catalog/dependency.c:1106:2: warning: en=
umeration value 'OCLASS_SUBSCRIPTION' not handled in switch [-Wswitch]

Hm.  I think it's intentional that shared objects are not handled there,
but I wonder whether the lack of SUBSCRIPTION represents a bug.
Anyway I think it would be fine to add those to the switch as explicit
error cases.


> /pgsql/source/master/src/backend/commands/tablecmds.c: In function 'ATExe=
cAlterColumnType':
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_AM' not handled in switch [-Wswitch]
>    switch (getObjectClass(&foundObject))
>    ^
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_STATISTIC_EXT' not handled in switch [-Wswitch]
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_EVENT_TRIGGER' not handled in switch [-Wswitch]
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_PUBLICATION' not handled in switch [-Wswitch]
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_PUBLICATION_REL' not handled in switch [-Wswitch]
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_SUBSCRIPTION' not handled in switch [-Wswitch]
> /pgsql/source/master/src/backend/commands/tablecmds.c:9112:3: warning: en=
umeration value 'OCLASS_TRANSFORM' not handled in switch [-Wswitch]

All of those ought to go into the category that prints
"unexpected object depending on column", I think; certainly
"unrecognized object class" is not a better report, and the fact that
we've evidently not touched this switch in the last few additions of
object types provides fodder for the idea that the default: is unhelpful.
(Not to mention that not having OCLASS_STATISTIC_EXT here is an outright
bug as of today...)  So losing the default: case here seems good to me.

Alternatively, we could drop the explicit listing of oclasses we're not
expecting to see, and let the default: case be "unexpected object
depending on column".  Treating the default separately is only of value if
we'd expect getObjectDescription to fail, but there's no good reason for
this code to be passing judgment on whether that might happen.

What do we want this to do about statistics objects?  We could either
throw a feature-not-implemented error or try to update the stats
object for the new column type.

			regards, tom lane


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