> On Mon, Feb 17, 2025 at 09:51:32AM GMT, Sami Imseih wrote:
> > This should do it. The last patch for today,
>
> I looked at v27 today and have a few comments.
>
> 1/ It looks like the CTE test is missing a check for results.
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.
> 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?
> 3/ Here, this looks wrong as we could end up traversing an elements list
> twice. Once inside IsMergeableConstList and if that call returns false, we end
> up traversing through the elements list again in _jumbleNode.
IsMergeableConstList and _jumbleNode serve different purposes, so it's
probably unwise to try to replace one with another. E.g.
IsMergeableConstList will stop at the first non-constant expression, so
it's not a full traversal.
