Re: FETCH FIRST clause WITH TIES option

On Thu, Nov 28, 2019 at 12:36 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Thanks.
>
> (I would suggest renaming the new state LIMIT_WINDOWEND_TIES, because it
> seems to convey what it does a little better.)
>
> I think you should add a /* fall-though */ comment after changing state.
> Like this (this flow seems clearer; also DRY):
>
> if (!node->noCount &&
> node->position - node->offset >= node->count)
> {
> if (node->limitOption == LIMIT_OPTION_COUNT)
> {
> node->lstate = LIMIT_WINDOWEND;
> return NULL;
> }
> else
> {
> node->lstate = LIMIT_WINDOWEND_TIES;
> /* fall-through */
> }
> }
> else
> ...
>
>
changed

> I've been playing with this a little more, and I think you've overlooked
> a few places in ExecLimit that need to be changed. In the regression
> database, I tried this:
>
> 55432 13devel 17282=# begin;
> BEGIN
> 55432 13devel 17282=# declare c1 cursor for select * from int8_tbl order
> by q1 fetch first 2 rows with ties;
> DECLARE CURSOR
> 55432 13devel 17282=# fetch all in c1;
> q1 │ q2
> ─────┼──────────────────
> 123 │ 456
> 123 │ 4567890123456789
> (2 filas)
>
> 55432 13devel 17282=# fetch all in c1;
> q1 │ q2
> ────┼────
> (0 filas)
>
> 55432 13devel 17282=# fetch forward all in c1;
> q1 │ q2
> ────┼────
> (0 filas)
>
> So far so good .. things look normal. But here's where things get
> crazy:
>
> 55432 13devel 17282=# fetch backward all in c1;
> q1 │ q2
> ──────────────────┼──────────────────
> 4567890123456789 │ 123
> 123 │ 4567890123456789
> (2 filas)
>
> (huh???)
>
> 55432 13devel 17282=# fetch backward all in c1;
> q1 │ q2
> ────┼────
> (0 filas)
>
> Okay -- ignoring the fact that we got a row that should not be in the
> result, we've exhausted the cursor going back. Let's scan it again from
> the start:
>
> 55432 13devel 17282=# fetch forward all in c1;
> q1 │ q2
> ──────────────────┼───────────────────
> 123 │ 4567890123456789
> 4567890123456789 │ 123
> 4567890123456789 │ 4567890123456789
> 4567890123456789 │ -4567890123456789
> (4 filas)
>
> This is just crazy.
>
> I think you need to stare a that thing a little harder.
>
>
This is because the new state didn't know about backward scan
and there was off by one error it scan one position past to detect
ties. The attached patch fix both
regards
Surafel