> On Tue, Feb 11, 2025 at 07:51:43PM GMT, Álvaro Herrera wrote:
> On 2025-Feb-11, Dmitry Dolgov wrote:
>
> > > On Tue, Feb 11, 2025 at 10:49:59AM GMT, Sami Imseih wrote:
> > > I have only looked at 0001, but I am wondering why
> > > query_id_const_merge is a pg_stat_statements GUC
> > > rather than a core GUC?
> >
> > It was moved from being a core GUC into a pg_stat_statements GUC on the request
> > from the reviewers. Community tries to prevent adding more and more core GUCs
> > into PostgreSQL.
>
> I understand, but I tend to disagree with the argument because it's okay
> to simplify things that can be simplified, but if simplifying makes them
> more complex, then it goes against the original desire for simplicity
> anyway. My impression is that it would be better to put it back.
>
> (Otherwise, how do you document the behavior that pg_stat_activity
> suddenly emits different query_ids than before because you installed
> pg_stat_statement and enabled this feature? It just doesn't make much
> sense.)
I don't have strong opinion on that and open to move it back, so that we
can see if anyone will object.
> > > I suppose this is telling us that detecting the case with consts wrapped
> > > in casts is not really optional. (Dmitry said this was supported at
> > > early stages of the patch, and now I'm really curious about that
> > > implementation because what IsMergeableConstList sees is a FuncExpr that
> > > invokes the cast function for float8 to int4.)
> >
> > Yes, those cases in question are usually FuncExpr. The original patch
> > implementation used to handle that via simplifying the node with
> > eval_const_expressions to figure out if the value we work with is a constant.
> > This approach was marked as too risky by reviewers, as this could reach a lot
> > of unexpected functionality in the mutator part.
>
> Hmm, what about doing something much simpler, such as testing whether
> there's just a CoerceViaIO/RelabelType around a Const or a one-parameter
> function call of an immutable boostrap-OID function that has a Const as
> argument, and trivial cases like that? Something very very simple
> that's going to catch the majority of cases without anything as complex
> as a node walker.
>
> Maybe something like statext_is_compatible_clause_internal() can be an
> inspiration (and even that is far more complex than we need here.)
I'm somewhat hesitant to cover only some cases, but let me try, maybe
it's indeed going to be good enough.
