Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 12 Jun 2025 08:46:26 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>,
	Dilip Kumar <dilipbalaut@gmail.com>, a.kozhemyakin@postgrespro.ru,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18947: TRAP: failed Assert("len_to_wrt >= 0") in
 pg_stat_statements
Message-ID: <aEoVUjPKJWWWM4QE@paquier.xyz>
References: 
 <CAFiTN-tGux5MZ9Hk2wKv5sVhk7eDq2AHaT79LxnQbVamxoRn9Q@mail.gmail.com>
 <aET38DIyBht33bIP@paquier.xyz>
 <9171ecda-e1f4-4390-ae54-98bc441b98e3@oss.nttdata.com>
 <aEfGl9bNPR5C76W2@paquier.xyz>
 <CAO6_Xqoxsm_Y5dxc_4YmrhRACTBe-Y8Q=PP6Xv-svo6eud1-Mg@mail.gmail.com>
 <aEgToLnBX9W43ne-@paquier.xyz>
 <aEi0N9AmJnrFC6Ug@paquier.xyz>
 <f7d8ac5e-96f2-46ef-ba56-9e211f4ffc18@oss.nttdata.com>
 <aEjH0x7licySiK_L@paquier.xyz>
 <CAO6_XqpxskZZdhgNJ0xQ1cyX3Br=11iSAb_1GLQHO1=OziKNoQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="i1IFZ4qMitKXZXQq"
Content-Disposition: inline
In-Reply-To: 
 <CAO6_XqpxskZZdhgNJ0xQ1cyX3Br=11iSAb_1GLQHO1=OziKNoQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aEoVUjPKJWWWM4QE%40paquier.xyz>
Precedence: bulk


--i1IFZ4qMitKXZXQq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 11, 2025 at 11:04:36AM +0200, Anthonin Bonnefoy wrote:
> Here's a v2 for the attempted fix. This is using the mentioned approach:
> - SelectStmt's location is set to Select's position when the node if built
> - If there are surrounding parentheses, we include them and move the
> start of the statement to the outermost '('
>=20
> The Statement length is only set for:
> - COPY: We use the PreparableStatement's surrounding '()'  to set both
> location and length (setting location should be redundant here though)
> - CTAS: we set the statement's length if there's a tailing 'WITH (NO) DAT=
A'.
> - Other than those, the statement length will fallback to use the top
> RawStmt's remaining length.

@@ -3417,9 +3418,9 @@ CopyStmt: COPY opt_binary qualified_name opt_column_l=
ist=20
[...]
-     updatePreparableStmtEnd($3, @2, @4 - @2);
+     updatePreparableStmtEnd($3, @3, @4 - @3);

For the case of COPY, it is possible to get a much better report by
setting the location and length to not include the first parenthesis,
like that.

> I haven't done it for Views and JSON_ARRAY. Their nested queries
> aren't currently tracked and reported by pgss so there's no way to
> test those.

I have to admit that it is inconsistent to set a location in their
respective inner nodes while we don't use updateSelectStmtEnd() to
adjust the length based on the location assigned by the parser.
There's a risk of having a code path in the future looking at the
location later on based on the parsed state and taking a wrong
decision because the location and the length do not match.  Such cases
could be easier to reach outside of core, for example imagine an
extension looking at a ViewStmt where we set the location but not a
matching length.  We would consider as inner query a string that
begins at the beginning of the location until the end of the string
because the length is not set to match with what the parser finds.

> This could be done but there's no available way to test
> this is correct AFAIK?

Yes, perhaps we should think harder about this part.  There are two
patterns among a few others like UNION, which can also be used in
views.

Anyway, I am going to pull the plug on this one today, because we need
to take a decision with beta2 approaching fast but it's mostly me
being not fully confident that this is right as-is.  If we're wrong in
one path, that's an out-of-bound memory write waiting for us in PGSS
for the query normalization, as this report proves, and that would be
bad.

The first step would be to improve the main regression test suite for
all these grammar "parenthesis" patterns we have never had coverage
for in core.  I am wondering if it is actually possible to reach an
inconsistent state even in older stable branches because we've never
tested all these grammar patterns.  Perhaps I'm wrong, but we would be
more confident with all these extra patterns stressed first by
027_stream_regress.pl which forces queries to have some normalization
applied.
--
Michael

--i1IFZ4qMitKXZXQq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmhKFVIACgkQnvQgOdby
QH0Vvw/+OT8d4BUZF5tS/l35RGwzK2jgKPHKW6crI257/k3WwvKv7l1u4VbM+lXy
Dgx87KUEuw623jgFZLaxW5QHPbvTzoj0WnWHSPjneeWmL7ig1Yp+IOu+i+hPfuwT
AeGKbBIKAJpS1LN6cC9lQ5/u5L8zFUd0sTV4M/dBpSF/iPZDE8elkwMvLruIAONn
AsMf/8dOBleHj0aAdPQse/S2xYEqHFLUqlwjdxX5l3Urksh7bh1QDZwp3ORoQvFU
oWE6LjTIw7YZrNrcaV5L5yYQekPSPqN7WC9UGNksHc8KoM00TwGMy7wSfInB+nMh
ZCFX72gdefcPICa95zlQElqoDLUGe6xa66tUaXeWF7sMVCH21MZ5fJ0KUMGeEDDZ
cWRgVF8Mo0dOJba+488352Fwzx+G+mnmLQaZcdvCLzgonJQtJJfOEFiUHPeSLv9+
207sZwNlYjCVHgPWET7jmeDAvEI8jJ3cjS0z30I9eZp+V+J1/yQ5gLCZhV2eYMBv
I/RfLlm8aueC0vrwMSy+pb24/KWbEs73unoCNrEy4rlHN/q3gwRG7SweKifktBhH
9E54myPptafAvQy11w17tBJvsuP+EkN53579qxJIPqbGjrfPhAhBLTj8zvsRqOgs
qOe0T3lJmVWNF7HI20Q6oeeKzm4e+NHyWQ2hKEEPs/87GZCKm+E=
=STrV
-----END PGP SIGNATURE-----

--i1IFZ4qMitKXZXQq--