Re: Row pattern recognition

Hi Henson,

>> nocfbot-0006: Disable frame optimization for RPR windows
>> 
>>   Prevents the planner from optimizing the window frame for RPR
>>   windows.  The frame clause in RPR has different semantics (it
>>   bounds the pattern search space), so standard frame optimizations
>>   must be disabled.  Adds EXPLAIN tests to verify.  Please review
>>   this one -- it's a newly discovered issue.
> 
> Oh, this is a very important finding! I will definitely look into the
> patch.

As I promised, I looked into nocfbot-0006 and it is good to me.

While at it, I looked around optimize_window_clauses and encountered
this:
				/*
				 * Perform the same duplicate check that is done in
				 * transformWindowFuncCall.
				 */
				if (equal(wc->partitionClause, existing_wc->partitionClause) &&
					equal(wc->orderClause, existing_wc->orderClause) &&
					wc->frameOptions == existing_wc->frameOptions &&
					equal(wc->startOffset, existing_wc->startOffset) &&
					equal(wc->endOffset, existing_wc->endOffset))

It looks like it missed the check for DEINFE and PATTERN clause but
with your patch, if RPR is defined we do not come here anyway. So, I
looked in transformWindowFuncCall (parse_agg.c) as it mentioned in the
comment above and I found this:

			/*
			 * Also see similar de-duplication code in optimize_window_clauses
			 */
			if (equal(refwin->partitionClause, windef->partitionClause) &&
				equal(refwin->orderClause, windef->orderClause) &&
				refwin->frameOptions == windef->frameOptions &&
				equal(refwin->startOffset, windef->startOffset) &&
				equal(refwin->endOffset, windef->endOffset))

This basically combines a window definition without window name into
an existing window definition if they shar the identical window
definition. For example:

CREATE TEMP TABLE rpr_copy (id INT, val INT);
INSERT INTO rpr_copy VALUES (1, 10), (2, 20), (3, 30), (4, 40);
SELECT id, val, first_value(id) OVER (
       ORDER BY id
       ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
), first_value(id) OVER w1
FROM rpr_copy
WINDOW w1 AS (
       ORDER BY id
       ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
       PATTERN (A+)
       DEFINE A AS val > 10
);

The two window definitions are regarded as identical and produces
wrong result.

 id | val | first_value | first_value 
----+-----+-------------+-------------
  1 |  10 |             |            
  2 |  20 |           2 |           2
  3 |  30 |             |            
  4 |  40 |             |            
(4 rows)

Of course proper result is:

 id | val | first_value | first_value 
----+-----+-------------+-------------
  1 |  10 |           1 |            
  2 |  20 |           2 |           2
  3 |  30 |           3 |            
  4 |  40 |           4 |            
(4 rows)

Explain result:

                                   QUERY PLAN                                   
--------------------------------------------------------------------------------
 WindowAgg  (cost=209.33..215.01 rows=2260 width=17)
   Window: w1 AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING)
   Pattern: a+"
   ->  Sort  (cost=158.51..164.16 rows=2260 width=8)
         Sort Key: id
         ->  Seq Scan on rpr_copy  (cost=0.00..32.60 rows=2260 width=8)
(6 rows)

The two window definitions are unified into w1, which has the RPR
part, which is wrong.  To fix this, we need to check the RPR part
while unifying the window definitions. Attached patch does it (on top
of v44).

Question is, should we fix optimize_window_clause as well? As I
mentioned earlier, with Henson's fix it does not produce any bug at
this moment. However, we may want to fix for code consistency.  Not
fixing it will break the comment.

Thoughts?

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp