From: | Surafel Temesgen <surafel3000(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: New CORRESPONDING clause design |
Date: | 2017-03-25 12:41:07 |
Message-ID: | CALAY4q_2zxsucysDmUSstxgM48n3DEUoN8B5Gbq4pGaKg47Keg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
>
>
> I took a quick look through this and noted that it fails to touch
> ruleutils.c, which means that dumping of views containing CORRESPONDING
> certainly doesn't work.
>
fixed
> Also, the changes in parser/analyze.c seem rather massive and
> correspondingly hard to review. Is it possible to rearrange the
> patch to reduce the size of that diff? If you can avoid moving
> or reindenting existing code, that'd help.
>
Part of transformSetOperationTree that make union data type of
set operation target list became makeUnionDatatype inorder to
easy using it multiple time and avoid very long transformSetOperationTree
function
> The code in that area seems rather confused, too. For instance,
> I'm not sure exactly what orderCorrespondingList() is good for,
> but it certainly doesn't look to be doing anything that either its
> name or its header comment (or the comments at the call sites) would
> suggest. Its input and output tlists are always in the same order.
>
> It give corresponding target list a sequential resnos
Inorder to avoid touching generate_append_tlist I change
the comment and function name as such
I also think there should be some comments about exactly what matching
> semantics we're enforcing. The SQL standard says
>
> a) If CORRESPONDING is specified, then:
> i) Within the columns of T1, equivalent <column name>s shall
> not be specified more than once and within the columns of
> T2, equivalent <column name>s shall not be specified more
> than once.
>
> That seems unreasonably stringent to me; it ought to be sufficient to
> forbid duplicates of the names listed in CORRESPONDING, or the common
> column names if there's no BY list. But whichever restriction you prefer,
> this code seems to be failing to check it --- I certainly don't see any
> new error message about "column name "foo" appears more than once".
>
fixed
I'm not impressed by using A_Const for the members of the CORRESPONDING
> name list. That's not a clever solution, that's a confusing kluge,
> because it's a complete violation of the meaning of A_Const. Elsewhere
> we just use lists of String for name lists, and that seems sufficient
> here. Personally I'd just use the existing columnList production rather
> than rolling your own.
>
fixed
>
>
Attachment | Content-Type | Size |
---|---|---|
corresponding_clause_v7.patch | application/octet-stream | 59.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Arseny Sher | 2017-03-25 12:47:44 | Re: [GSoC] Push-based query executor discussion |
Previous Message | Rushabh Lathia | 2017-03-25 12:09:55 | Re: crashes due to setting max_parallel_workers=0 |