Re: Assignment of valid collation for SET operations on queries with UNKNOWN types.

On Mon, Jan 23, 2017 at 7:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 2. I didn't do anything about docs, either, though maybe no change
>> is needed. One user-visible change from this is that queries should
>> never return any "unknown"-type columns to clients anymore. But I
>> think that is not documented now, so maybe there's nothing to change.
>
> The only thing I could find in the SGML docs that directly addresses
> unknown-type columns was a remark in the CREATE VIEW man page, which
> I've updated. I also added a section to typeconv.sgml to document
> the behavior.

This looks good.

>> 3. We need to look at whether pg_upgrade is affected. I think we
>> might be able to let it just ignore the issue for views, since they'd
>> get properly updated during the dump and reload anyway. But if someone
>> had an actual table (or matview) with an "unknown" column, that would
>> be a pg_upgrade hazard, because "unknown" doesn't store like "text".
>
> I tested and found that simple views with unknown columns seem to update
> sanely if we just let pg_upgrade dump and restore them. So I suggest we
> allow that to happen. There might be cases where dependent views behave
> unexpectedly after such a conversion, but I think that would be rare
> enough that it's not worth forcing users to fix these views by hand before
> updating. However, tables with unknown columns would fail the upgrade
> (since we'd reject the CREATE TABLE command) while matviews would be
> accepted but then the DDL wouldn't match the physical data storage.
> So I added code to pg_upgrade to check for those cases and refuse to
> proceed. This is almost a straight copy-and-paste of the existing
> pg_upgrade code for checking for "line" columns.
>
> I think this is committable now; the other loose ends can be dealt
> with in follow-on patches. Does anyone want to review it?

I have spent a couple of hours looking at it, and it looks in pretty
good shape. The concept of doing the checks in the parser is far
cleaner than what was proposed upthread to tweak the column lists, and
more generic.

One thing though: even with this patch, it is still possible to define
a domain with unknown as underlying type and have a table grab it:
create domain name as unknown;
create table foo_name (a name);
I think that this case should be restricted as well, and pg_upgrade
had better complain with a lookup at typbasetype in pg_type.
--
Michael