Re: Row pattern recognition

Thanks for the review!

> I found one more bug, the following testcase crashes the server:
> 
> CREATE TEMP TABLE t (id int, val text);
> INSERT INTO t VALUES
>   (1,'A'),(2,'B'),
>   (3,'A'),(4,'B'),
>   (5,'A'),(6,'B'),
>   (7,'A'),(8,'B'),
>   (9,'X');
> 
> SELECT id, val, count(*) OVER w AS match_count
> FROM t
> WINDOW w AS (
>   ORDER BY id
>   ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
>   INITIAL
>   PATTERN (A B (A B){1,2} A B)
>   DEFINE
>     A AS val = 'A',
>     B AS val = 'B'
> );

I just confirmed the crash.

> Where the code does
> 
> + child->min += 1;
> 
> It should also increment max if it's not infinity.
> 
> if (child->max != RPR_QUANTITY_INF)
>     child->max += 1;

Henson, What do you think? 

> + expr = te->expr;
> ...
> + attno_map((Node *) expr);
> 
> It it okay to mutate an object in the plan cache in the executor?
> Wouldn't it be better to first copy it?

Good point. You are right, the plan cache should be read only.
However, Henson is working on a different approach and the code will
not be used any more...

>> Adds stock trading scenario tests using realistic synthetic data
>> (stock.data with 1632 rows). Tests V-shape recovery, W-shape,
>> consecutive rises, and other common pattern matching use cases.
> 
> The data file is not included in the patchset

I forggot to include the data file. Please apply attached patch on top
of v45.

> + if (wc->frameOptions & FRAMEOPTION_GROUPS)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("FRAME option GROUP is not permitted when row pattern
> recognition is used"),
> + errhint("Use: ROWS instead"),
> + parser_errposition(pstate,
> + windef->frameLocation >= 0 ?
> + windef->frameLocation : windef->location)));
> 
> Here and at a few other places, shouldn't this use a different error
> code, like ERRCODE_WINDOWING_ERROR?

Agreed.

> Also a typo, the error message should say GROUPS.
> 
> + elog(ERROR, "PREV/NEXT must have 1 argument but function %d has %d args",
> + func->funcid, nargs);
> 
> Isn't this a user triggerable error, which should have a proper error code?

Right. Maybe ERRCODE_TOO_MANY_ARGUMENTS?

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