Re: Row pattern recognition

Re: Row pattern recognition
To: jian he, Tatsuo Ishii (+ pgsql-hackers)

Hi jian, Tatsuo,

Thanks for the careful review -- replies inline. One question up front,
for Tatsuo: I'd like your take on the LOWPRICE/UP/DOWN casing below
(details at that point). The rest are replies to jian's points.

> The below in advanced.sgml actually seems better suited for
> explain.sgml (which currently has no changes).
> At the very least, explain.sgml should mention it.

Agreed; I'll move the EXPLAIN marker description over to explain.sgml.

> LOWPRICE, UP, DOWN should be lower case, since they are not keywords?

They're user identifiers, and the standard and Oracle docs show them in
upper case, so I lean toward leaving the example as written -- though
EXPLAIN and deparse do lower-case them today. Tatsuo, I'd defer the
casing to you.

> scanRPRPatternRecursive
> It would be better to replace all ``non-trivial quantifier`` to
> ``non-trivial quantifier (not {1,1}).``

Agreed, that's clearer; will do.

> I want to rename allocateRPRPattern to makeRPRPattern, what do you think?

Good idea -- makeRPRPattern fits the makeNode/makeXxx convention.

> RPRPatternNode.reluctant_location is not necessary.

Agreed. The field is only written and propagated, never read back, so
I'll drop it and keep the reluctance as a plain bool.

> Is nav_volatile_func_checker really necessary? ... The convention is
> not to check expression volatility during parse analysis stage.

Good point -- I'll move the volatility rejection out of parse analysis
and into the planner, in line with that convention.

> query_jumble_ignore is not needed because it's only manipulated in
> gram.y ...

Right; I'll drop the query_jumble_ignore attribute.

> v47-0001 ... Change errmsg to errmsg("invalid token \"%s\" after range
> quantifier", $5) will make the error message clearer.

Agreed, that reads better; I'll adjust the message accordingly.

> v47-0002 ... We should just drop these comments from the regress tests.

Agreed -- the internal function names don't belong in the tests; I'll
remove them.

> v47-0003 ... mutate them in place using list_delete_nth_cell ...
> for overflow handling ... pg_mul_s32_overflow, pg_add_s32_overflow.

The overflow helpers are a clear win. The in-place list mutation is
behavior-equivalent, but I'd rather not reshape that logic right now
without a concrete need, so I'll hold off on it for the moment. The new
tests are good and I'll fold them in.

More broadly, I see the patch as being in its final verification stage
at this point, so I'd prefer to limit logic changes to those backed by a
clear review or test finding.

> v47-0004 ... Some misc refactoring ...

Mostly agree -- the rpSkipTo/initial comments read more accurately as
flag assignments. I'd keep the "PATTERN AST" wording (it's an AST, not a
flag) and the deparse note, but the rest is fine.

Best,
Henson