> On Mon, Feb 17, 2025 at 01:50:00PM GMT, Sami Imseih wrote: > > This test was to catch a crash that was happening in older version of > > the patch, so it doesn't have to verify the actual pgss entry. > > It seems odd to keep this test because of crash behavior experienced > in a previous version of the patch. if the crash reason was understood > and resolved, why keep it?
A preventive measure. As you could notice, the patch has long history, and certain issues could be accidentally reintroduced. > > > 2/ Looking at IsMergeableConst, I am not sure why we care about > > > things like function volatility, implicit cast or funcid > > > > FirstGenbkiObjectId? > > > > Function volatility is important to establish how constant is the > > result, for now we would like to exclude not immutable functions. The > > implicit cast and builtin check are there to limit squashing and exclude > > explicit or user-created functions (the second is probably an overkill, > > but this could be gradually relatex later). Or are you not sure about > > something different? > > My thoughts are when dealing with FuncExpr, if the first arg in the list of > func->args is a Const, shouldn't that be enough to tell us that we have > a mergeable value. If it's not a Const, it may be another FuncExpr, so > that tells us we don't have a mergeable list. Why would this not be enough? It's not a question about whether it's possible to implement this, but about whether it makes sense. In case of plain constants it's straightforward -- they will not change anything meaningfully and hence could be squashed from the query. Now for a function, that might return different values for the same set of constant arguments, it's much less obvious and omitting such expressions might have unexpected consequences. > See the attached 0001-experiement-on-top-of-v27.patch > which applies on top of v27 and produces the results like below. Btw, if you would like to share a code delta, please do not post it as a patch or diff. This hijacks the CI pipeline, because CFbot thinks that's a new version of the original patch.
