> > > 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.
This test on its own is valuable to test that we don't merge Var's, so if we keep it we should at least have such a test with verification. > > > > 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. query jumbling should not care about the behavior of the function. If we take a regular call to a volatile function, we will generate the same queryId for every call regardless of the input to the function. Why does the in-list case need to care about the volatility of the function? The way I see it is we need to merge constants that are either simple or potentially wrapped in a cast. We can detect functions that are explicitly called ( and potentially wrapped in a cast), and we ought to skip merging in that situation. > > 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. You're right. Sorry about that. -- Sami
