>> On Oct 16, 2025, at 18:17, Tatsuo Ishii <[email protected]> wrote: >> >> Thanks for the report. >> >>> Coverity thinks that this code has still some incorrect bits, and I >>> think that it is right to think so even on today's HEAD at >>> 02c171f63fca. >>> >>> In WinGetFuncArgInPartition()@nodeWindowAgg.c, we have the following >>> loop (keeping only the relevant parts: >>> do >>> { >>> [...] >>> else /* need to check NULL or not */ >>> { >>> /* get tuple and evaluate in partition */ >>> datum = gettuple_eval_partition(winobj, argno, >>> abs_pos, isnull, &myisout); >>> if (myisout) /* out of partition? */ >>> break; >>> if (!*isnull) >>> notnull_offset++; >>> /* record the row status */ >>> put_notnull_info(winobj, abs_pos, *isnull); >>> } >>> } while (notnull_offset < notnull_relpos); >>> >>> /* get tuple and evaluate in partition */ >>> datum = gettuple_eval_partition(winobj, argno, >>> abs_pos, isnull, &myisout); >>> >>> And Coverity is telling that there is no point in setting a datum in >>> this else condition to just override its value when we exit the while >>> loop. To me, it's a sigh that this code's logic could be simplified. >> >> To fix the issue, I think we can change: >> >>> datum = gettuple_eval_partition(winobj, argno, >>> abs_pos, isnull, &myisout); >> >> to: >> >> (void) gettuple_eval_partition(winobj, argno, >> abs_pos, isnull, &myisout); >> >> This explicitely stats that we ignore the return value from >> gettuple_eval_partition. I hope coverity understands this. >> >>> > > I think Coverity is complaining about the redundant call to > gettuple_eval_partition(). > > In the “else” clause, the function is called, then when “if (myisout)” is > satisfied, it will break out the while loop. After that, the function is > immediately called again, so “datum” is overwritten. But I haven’t spent time > thinking about how to fix.
Yes, the function is called again. But I think the cost is cheap in this case. Inside the function window_gettupleslot() is called. It could be costly if it spools tuples. But as tuple is already spooled by the former call of gettuple_eval_partition(), almost no cost is needed. We could avoid the redundant call by putting more code after the former function call to return immediately, or introduce a goto statement or a flag. But I think they will make the code harder to read and do not worth the trouble. Others may think differently though. Best regards, -- Tatsuo Ishii SRA OSS K.K. English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
