On Wed, Jan 18, 2017 at 10:55 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Sun, Jan 8, 2017 at 10:55 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> writes: >>> On Tue, Jan 3, 2017 at 5:57 PM, Rahila Syed <rahilasye...@gmail.com> wrote: >>>> Are you suggesting extending the patch to include coercing from unknown to >>>> text for all possible cases where a column of unknown type is being >>>> created? >> >>> I guess, that's what Tom is suggesting. >> >> Yes; I think the point is we should change the semantics we assume for >> "SELECT 'foo'", not only for views but across the board. There are plenty >> of non-view-related cases where it doesn't behave well, for example >> >> regression=# select * from >> (select 'foo' from generate_series(1,3)) ss order by 1; >> ERROR: failed to find conversion function from unknown to text >> >> Furthermore, it seems like a seriously bad idea for "SELECT 'foo'" to mean >> something different in the context of CREATE VIEW than it means elsewhere. > > And this offers the same semantics for any DDL using SELECT's target > list to build the list of column's types, which is consistent. > >> The trick is to not break cases where we've already hacked things to allow >> external influence on the resolved types of SELECT-list items. AFAICT, >> these cases are just INSERT/SELECT and set operations (eg unions): >> >> regression=# create table target (f1 int); >> CREATE TABLE >> regression=# explain verbose insert into target select '42' from >> generate_series(1,3); >> QUERY PLAN >> >> -------------------------------------------------------------------------------- >> --------- >> Insert on public.target (cost=0.00..10.00 rows=1000 width=4) >> -> Function Scan on pg_catalog.generate_series (cost=0.00..10.00 >> rows=1000 >> width=4) >> Output: 42 >> Function Call: generate_series(1, 3) >> (4 rows) >> >> regression=# explain verbose select '42' union all select 43; >> QUERY PLAN >> ------------------------------------------------ >> Append (cost=0.00..0.04 rows=2 width=4) >> -> Result (cost=0.00..0.01 rows=1 width=4) >> Output: 42 >> -> Result (cost=0.00..0.01 rows=1 width=4) >> Output: 43 >> (5 rows) >> >> In both of the above cases, we're getting an integer constant not a >> text or "unknown" constant, because the type information gets imported >> from outside the "SELECT". >> >> I spent some time fooling with this today and came up with the attached >> patch. I think this is basically the direction we should go in, but >> there are various loose ends: >> >> 1. I adjusted a couple of existing regression tests whose results are >> affected, but didn't do anything towards adding new tests showing the >> desirable results of this change (as per the examples in this thread). >> >> 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. >> >> 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". >> >> 4. If "unknown" were marked as a pseudotype not base type in pg_type >> (ie, typtype "p" not "b") then the special case for it in >> CheckAttributeType could go away completely. I'm not really sure >> why it's "b" today --- maybe specifically because of this point that >> it's currently possible to create tables with unknown columns? But >> I've not looked at what all the implications of changing that might >> be, and anyway it could be left for a followon patch. >> >> 5. I noticed that the "resolveUnknown" arguments of transformSortClause >> and other functions in parse_clause.c could go away: there's really no >> reason not to just treat them as "true" always. But that could be >> separate cleanup as well. >> >> Anybody want to hack on those loose ends? > > Ashutosh, Rahila, do you have plans to review things here?
I won't be able to work on creating patches for TODOs listed by Tom. But I can review if someone volunteers to produce the patches. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
