Re: BUG #18947: TRAP: failed Assert("len_to_wrt >= 0") in pg_stat_statements

On Tue, Jun 10, 2025 at 7:46 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> Touché. That means messing with any parent Node that includes or
> touches select_with_parens, select_no_parens (JSON_ARRAY has one) or
> SelectStmt (few of these, like in CTAS). That's not cool in the long
> term because we would expect any new node that englobes a SelectStmt
> to be able to set up their inner length and location as they should,
> as far as I get it. I was wondering first if we could have done
> something with the top-level statement depending on the nesting level
> of PGSS, but that's not going to fly high, for example with cases like
> this one (any case that has an inner SELECT):
> copy ((select 1) union ((select 1)) fetch first 1 row only) to stdout

One wrong assumption I made was that parenthesis aren't part of the
select statement, which is definitely wrong with queries like '(SELECT
1) limit 1;'. As we don't have end location tracking, we don't have
the possibility to get the length of the select statement.

I've joined a possible (and very rough, this would definitely require
more tests) fix:
- SelectStmt's location is now the outermost '(' position
- Don't track the length anymore in 'select_no_parens'
- Nodes that are embedding a SelectStatement would need to update its
length if necessary. For COPY, this is straightforward as we can use
the parenthesis location surrounding the statement. With CTAS,
SelectStmt's length should be updated if there's an existing
opt_with_data.

> Anyway, this is the second bug that we have in this area, and this one
> is worse. Now that we are in the middle of beta, this is gathering
> the signs that we should revert the change from the tree for the
> moment and potentially revisit the whole in v19 with these edge cases
> handled. So attached is a patch doing a revert of:
> 499edb09741b, nested statement tracking.
> 06450c7b8c70, follow-up fix for 499edb09741b.

That's fair. I can get a better patch with the approach mentioned, but
I don't know if that would be enough to cover all edge cases. Though
reverting this is also going to break pgAudit tests if I remember
correctly.

Regards,
Anthonin