Re: Failure assertion in GROUPS mode of window function in current HEAD
On Fri, Jul 13, 2018 at 12:17 AM, Tom Lane wrote: > Masahiko Sawada writes: >> I think we also can update the doc about that GROUPS offset mode >> requires ORDER BY clause. Thoughts? Attached patch updates it. > > Ooops, I forgot to check the docs. This isn't quite the right fix > though --- the problem is that there's a sentence at the end of that > para that now says exactly the wrong thing. I fixed that. > Yeah, that's not the right fix. Thank you for fixing! Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: Failure assertion in GROUPS mode of window function in current HEAD
Masahiko Sawada writes: > I think we also can update the doc about that GROUPS offset mode > requires ORDER BY clause. Thoughts? Attached patch updates it. Ooops, I forgot to check the docs. This isn't quite the right fix though --- the problem is that there's a sentence at the end of that para that now says exactly the wrong thing. I fixed that. regards, tom lane
Re: Failure assertion in GROUPS mode of window function in current HEAD
On Wed, Jul 11, 2018 at 4:21 AM, Tom Lane wrote: > Masahiko Sawada writes: >> BTW, I found an another but related issue. We can got an assertion >> failure also by the following query. > >> =# select sum(c) over (partition by c order by c groups between 1 >> preceding and current row) from test; > > Oh, good point, that's certainly legal per spec, so we'd need to do > something about it. > > The proximate cause of the problem, I think, is that the planner throws > away the "order by c" as being redundant because it already sorted by c > for the partitioning requirement. So we end up with ordNumCols = 0 > even though the query had ORDER BY. Yeah, I think so too. > > This breaks not only GROUPS cases, but also RANGE OFFSET cases, because > the executor expects to have an ordering column. I thought for a bit > about fixing that by forcing the planner to emit the ordering column for > RANGE OFFSET even if it's redundant. But it's hard to make the existing > logic in get_column_info_for_window do that --- it can't tell which > partitioning column the ordering column was redundant with, and even if it > could, that column might've been of a different data type. So eventually > I just threw away that redundant-key-elimination logic altogether. > I believe that we never thought it was really useful as an optimization, > but way back when window functions were put in, we didn't have (or at > least didn't think about) a way to identify the partitioning/ordering > columns without reference to the input pathkeys. > Agreed. > With this patch, WindowAggPath.winpathkeys is not used for anything > anymore. I'm inclined to get rid of it, though I didn't do so here. > (If we keep it, we at least need to adjust the comment in relation.h > that claims createplan.c needs it.) +1 for removing. > > The other issue here is that nodeWindowAgg's setup logic is not quite > consistent with update_frameheadpos and update_frametailpos about when > to create tuplestore read pointers and slots. We might've prevented > all the inconsistent cases with the parser and planner fixes, but it > seems best to make them really consistent anyway, so I changed that. > > Draft patch attached. > Thank you for committing! I think we also can update the doc about that GROUPS offset mode requires ORDER BY clause. Thoughts? Attached patch updates it. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center update_window_syntax_doc.patch Description: Binary data
Re: Failure assertion in GROUPS mode of window function in current HEAD
Masahiko Sawada writes:
> BTW, I found an another but related issue. We can got an assertion
> failure also by the following query.
> =# select sum(c) over (partition by c order by c groups between 1
> preceding and current row) from test;
Oh, good point, that's certainly legal per spec, so we'd need to do
something about it.
The proximate cause of the problem, I think, is that the planner throws
away the "order by c" as being redundant because it already sorted by c
for the partitioning requirement. So we end up with ordNumCols = 0
even though the query had ORDER BY.
This breaks not only GROUPS cases, but also RANGE OFFSET cases, because
the executor expects to have an ordering column. I thought for a bit
about fixing that by forcing the planner to emit the ordering column for
RANGE OFFSET even if it's redundant. But it's hard to make the existing
logic in get_column_info_for_window do that --- it can't tell which
partitioning column the ordering column was redundant with, and even if it
could, that column might've been of a different data type. So eventually
I just threw away that redundant-key-elimination logic altogether.
I believe that we never thought it was really useful as an optimization,
but way back when window functions were put in, we didn't have (or at
least didn't think about) a way to identify the partitioning/ordering
columns without reference to the input pathkeys.
With this patch, WindowAggPath.winpathkeys is not used for anything
anymore. I'm inclined to get rid of it, though I didn't do so here.
(If we keep it, we at least need to adjust the comment in relation.h
that claims createplan.c needs it.)
The other issue here is that nodeWindowAgg's setup logic is not quite
consistent with update_frameheadpos and update_frametailpos about when
to create tuplestore read pointers and slots. We might've prevented
all the inconsistent cases with the parser and planner fixes, but it
seems best to make them really consistent anyway, so I changed that.
Draft patch attached.
regards, tom lane
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 968d5d3..0db193b 100644
*** a/src/backend/executor/nodeWindowAgg.c
--- b/src/backend/executor/nodeWindowAgg.c
*** begin_partition(WindowAggState *winstate
*** 1079,1084
--- 1079,1085
{
WindowAgg *node = (WindowAgg *) winstate->ss.ps.plan;
PlanState *outerPlan = outerPlanState(winstate);
+ int frameOptions = winstate->frameOptions;
int numfuncs = winstate->numfuncs;
int i;
*** begin_partition(WindowAggState *winstate
*** 1143,1150
* If the frame head is potentially movable, or we have an EXCLUSION
* clause, we might need to restart aggregation ...
*/
! if (!(winstate->frameOptions & FRAMEOPTION_START_UNBOUNDED_PRECEDING) ||
! (winstate->frameOptions & FRAMEOPTION_EXCLUSION))
{
/* ... so create a mark pointer to track the frame head */
agg_winobj->markptr = tuplestore_alloc_read_pointer(winstate->buffer, 0);
--- 1144,1151
* If the frame head is potentially movable, or we have an EXCLUSION
* clause, we might need to restart aggregation ...
*/
! if (!(frameOptions & FRAMEOPTION_START_UNBOUNDED_PRECEDING) ||
! (frameOptions & FRAMEOPTION_EXCLUSION))
{
/* ... so create a mark pointer to track the frame head */
agg_winobj->markptr = tuplestore_alloc_read_pointer(winstate->buffer, 0);
*** begin_partition(WindowAggState *winstate
*** 1182,1202
/*
* If we are in RANGE or GROUPS mode, then determining frame boundaries
! * requires physical access to the frame endpoint rows, except in
* degenerate cases. We create read pointers to point to those rows, to
* simplify access and ensure that the tuplestore doesn't discard the
! * endpoint rows prematurely. (Must match logic in update_frameheadpos
! * and update_frametailpos.)
*/
winstate->framehead_ptr = winstate->frametail_ptr = -1; /* if not used */
! if ((winstate->frameOptions & (FRAMEOPTION_RANGE | FRAMEOPTION_GROUPS)) &&
! node->ordNumCols != 0)
{
! if (!(winstate->frameOptions & FRAMEOPTION_START_UNBOUNDED_PRECEDING))
winstate->framehead_ptr =
tuplestore_alloc_read_pointer(winstate->buffer, 0);
! if (!(winstate->frameOptions & FRAMEOPTION_END_UNBOUNDED_FOLLOWING))
winstate->frametail_ptr =
tuplestore_alloc_read_pointer(winstate->buffer, 0);
}
--- 1183,1206
/*
* If we are in RANGE or GROUPS mode, then determining frame boundaries
! * requires physical access to the frame endpoint rows, except in certain
* degenerate cases. We create read pointers to point to those rows, to
* simplify access and ensure that the tuplestore doesn't discard the
! * endpoint rows prematurely. (Must create pointers in exactly the same
! * cases that update_frameheadpos and update_frametailpos need them.)
Re: Failure assertion in GROUPS mode of window function in current HEAD
On Tue, Jul 10, 2018 at 7:24 AM, Tom Lane wrote:
> Masahiko Sawada writes:
>> I got an assertion failure when I use GROUPS mode and specifies offset
>> without ORDER BY clause. The reproduction steps and the backtrace I
>> got are following.
>
>> =# create table test as select 1 as c;
>> =# select sum(c) over (partition by c groups between 1 preceding and
>> current row) from test;
>> TRAP: FailedAssertion("!(ptr >= 0 && ptr < state->readptrcount)",
>> File: "tuplestore.c", Line: 478)
>
> Hm. Shouldn't we reject this case? I don't see how GROUPS mode makes
> any sense with no ORDER BY. Maybe you could define it as working with
> "groups" of one row each, but the standard seems to think not:
>
>c) If GROUPS is specified, then:
>
> i) Either WDEF shall contain a , or WDEF shall
> specify an that identifies a window structure
> descriptor that includes a window ordering clause.
>
> The fact that you got an assertion failure is not very nice, maybe we
> need some more code defenses there; but probably the whole thing
> should be rejected at parse time.
>
Agreed. So should we reject only GROUPS offset mode without ORDER BY?
Although since I don't have the standard I'm not sure the ideal
behavior exactly there is the code that considers GROUPS non-offset
mode without ORDER BY.
BTW, I found an another but related issue. We can got an assertion
failure also by the following query.
=# select sum(c) over (partition by c order by c groups between 1
preceding and current row) from test;
The cause of this assertion failure is similar to the first problem I
reported; in the above case, since node->ordNumCols will be set to 0
we don't allocate both the frame head and tail pointer. The proposed
patch seems to fix the both problems but If we fix the first problem
by rejecting it as not-supported-feature error we need to fix the
second problem by other way.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Re: Failure assertion in GROUPS mode of window function in current HEAD
Masahiko Sawada writes:
> I got an assertion failure when I use GROUPS mode and specifies offset
> without ORDER BY clause. The reproduction steps and the backtrace I
> got are following.
> =# create table test as select 1 as c;
> =# select sum(c) over (partition by c groups between 1 preceding and
> current row) from test;
> TRAP: FailedAssertion("!(ptr >= 0 && ptr < state->readptrcount)",
> File: "tuplestore.c", Line: 478)
Hm. Shouldn't we reject this case? I don't see how GROUPS mode makes
any sense with no ORDER BY. Maybe you could define it as working with
"groups" of one row each, but the standard seems to think not:
c) If GROUPS is specified, then:
i) Either WDEF shall contain a , or WDEF shall
specify an that identifies a window structure
descriptor that includes a window ordering clause.
The fact that you got an assertion failure is not very nice, maybe we
need some more code defenses there; but probably the whole thing
should be rejected at parse time.
regards, tom lane
Re: Failure assertion in GROUPS mode of window function in current HEAD
On Wed, Jul 4, 2018 at 11:24 PM, Masahiko Sawada wrote:
> Hi,
>
> I got an assertion failure when I use GROUPS mode and specifies offset
> without ORDER BY clause. The reproduction steps and the backtrace I
> got are following.
>
> =# create table test as select 1 as c;
> =# select sum(c) over (partition by c groups between 1 preceding and
> current row) from test;
> TRAP: FailedAssertion("!(ptr >= 0 && ptr < state->readptrcount)",
> File: "tuplestore.c", Line: 478)
>
> #0 0x003b3ce32495 in raise () from /lib64/libc.so.6
> #1 0x003b3ce33c75 in abort () from /lib64/libc.so.6
> #2 0x00a2ef5a in ExceptionalCondition (conditionName=0xc99f38
> "!(ptr >= 0 && ptr < state->readptrcount)", errorType=0xc99f22
> "FailedAssertion",
> fileName=0xc99ec0 "tuplestore.c", lineNumber=478) at assert.c:54
> #3 0x00a7fa7d in tuplestore_select_read_pointer
> (state=0x139e4a0, ptr=-1) at tuplestore.c:478
> #4 0x007216cd in update_frameheadpos (winstate=0x137fc30) at
> nodeWindowAgg.c:1655
> #5 0x0071fb7f in eval_windowaggregates (winstate=0x137fc30)
> at nodeWindowAgg.c:735
> #6 0x00722ac8 in ExecWindowAgg (pstate=0x137fc30) at
> nodeWindowAgg.c:2196
> #7 0x006e5776 in ExecProcNodeFirst (node=0x137fc30) at
> execProcnode.c:445
> #8 0x006da945 in ExecProcNode (node=0x137fc30) at
> ../../../src/include/executor/executor.h:237
> #9 0x006dd2fc in ExecutePlan (estate=0x137fa18,
> planstate=0x137fc30, use_parallel_mode=false, operation=CMD_SELECT,
> sendTuples=true,
> numberTuples=0, direction=ForwardScanDirection, dest=0x138b098,
> execute_once=true) at execMain.c:1726
> #10 0x006daf2c in standard_ExecutorRun (queryDesc=0x13785b8,
> direction=ForwardScanDirection, count=0, execute_once=true) at
> execMain.c:363
> #11 0x006dad48 in ExecutorRun (queryDesc=0x13785b8,
> direction=ForwardScanDirection, count=0, execute_once=true) at
> execMain.c:306
> #12 0x008c7626 in PortalRunSelect (portal=0x12f3bd8,
> forward=true, count=0, dest=0x138b098) at pquery.c:932
> #13 0x008c72b4 in PortalRun (portal=0x12f3bd8,
> count=9223372036854775807, isTopLevel=true, run_once=true,
> dest=0x138b098, altdest=0x138b098,
> completionTag=0x7fffaece38b0 "") at pquery.c:773
> #14 0x008c1288 in exec_simple_query (
> query_string=0x128e938 "select sum(c) over (partition by c groups
> between 1 preceding and current row) from test;") at postgres.c:1122
> #15 0x008c555e in PostgresMain (argc=1, argv=0x12b8258,
> dbname=0x12b80d8 "postgres", username=0x12b80b0 "masahiko") at
> postgres.c:4153
> #16 0x00822c3c in BackendRun (port=0x12b0020) at postmaster.c:4361
> #17 0x008223aa in BackendStartup (port=0x12b0020) at postmaster.c:4033
> #18 0x0081e84b in ServerLoop () at postmaster.c:1706
> #19 0x0081e17d in PostmasterMain (argc=5, argv=0x1289330) at
> postmaster.c:1379
> #20 0x007452d0 in main (argc=5, argv=0x1289330) at main.c:228
>
> The cause of this assertion failure is that we attempted to select a
> read pointer (framehead_ptr) that is not allocated. We allocate read
> pointers for both frame head and tail when begin a new partition in
> begin_partition(). The current code doesn't allocate them as follows
> if ORDER BY clause is omitted, but this behavior doesn't match to both
> update_framheadpos() and update_framtailpos() which always use each
> read pointer in GROUPS offset mode.
>
> if ((winstate->frameOptions & (FRAMEOPTION_RANGE | FRAMEOPTION_GROUPS)) &&
>node->ordNumCols != 0)
> {
> if (!(winstate->frameOptions & FRAMEOPTION_START_UNBOUNDED_PRECEDING))
> winstate->framehead_ptr =
> tuplestore_alloc_read_pointer(winstate->buffer, 0);
> if (!(winstate->frameOptions & FRAMEOPTION_END_UNBOUNDED_FOLLOWING))
> winstate->frametail_ptr =
> tuplestore_alloc_read_pointer(winstate->buffer, 0);
> }
>
> Since this issue relates to only GROUPS mode it happen in PostgreSQL
> 11 or later. Attached patch fixes this issue and add regression tests
> for testing GROUPS mode without ORDER BY clause.
Since this problem still happens with current HEAD I've added this
item to Open Item.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
