Re: New CORRESPONDING clause design - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: New CORRESPONDING clause design
Date
Msg-id CAFj8pRC3PuXmGX77QeUOTyJrbbPxGhvzHWuzd25Ux-7=dPK+1Q@mail.gmail.com
Whole thread Raw
In response to [HACKERS] New CORRESPONDING clause design  (Surafel Temsgen <surafel3000@gmail.com>)
Responses Re: New CORRESPONDING clause design
List pgsql-hackers
Hi

2017-03-25 13:41 GMT+01:00 Surafel Temesgen <surafel3000@gmail.com>:



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  


I am sending updated version:

1. the corresponding columns must be unique, other not - code refactoring (the code is still O(N*M*Z) - but some slow operations (string cmp) reduced) + regress tests
2. regress tests for views
3. some cleaning (white chars)

Regards

Pavel

Attachment

pgsql-hackers by date:

Previous
From: Peter Geoghegan
Date:
Subject: Re: WIP: [[Parallel] Shared] Hash
Next
From: Tomas Vondra
Date:
Subject: Re: crashes due to setting max_parallel_workers=0