Assert failure in base_yyparse

On Fri, Mar 28, 2025 at 4:40 PM Евгений Горбанев <gorbanyoves@basealt.ru> wrote:

Got an assert failure when fuzzing the raw_parser function.
The query to reproduce:
SELECT xmltable.* FROM xmltest2, LATERAL xmltable('/d/r' PASSING x
COLUMNS a int PATH '' || lower(_path) is_not_null|| 'c');

If I understand correctly, is_not_null is considered as a valid keyword
in xmltable, but it gets the type T_A_Expr.

Prepared a patch to fix it, but there may be a better solution.

Nice catch. Yeah, is_not_null is a valid column option in xmltable.
In you example, the value of the is_not_null option is "|| 'c'", which
is interpreted as an A_Expr.

I wonder if the value's type should be checked earlier, rather than at
the last minute.

Also, I think IsA is a better choice for checking the node type.

Thanks
Richard

On Fri, Mar 28, 2025 at 6:05 PM Richard Guo <guofenglinux@gmail.com> wrote:

Nice catch. Yeah, is_not_null is a valid column option in xmltable.
In you example, the value of the is_not_null option is "|| 'c'", which
is interpreted as an A_Expr.

I wonder if the value's type should be checked earlier, rather than at
the last minute.

Hmm, I wonder if we should allow the use of the 'is_not_null' keyword
in xmltable. According to the doc, it seems that users should declare
NULL or NOT NULL for a column by specifying [NOT NULL | NULL] for the
column.

Thanks
Richard

Also, I think IsA is a better choice for checking the node type.

Agree, IsA is better. Fixed in the patch.

On Fri, Mar 28, 2025 at 6:05 PM Richard Guo <guofenglinux@gmail.com> wrote:
Hmm, I wonder if we should allow the use of the 'is_not_null' keyword
in xmltable. According to the doc, it seems that users should declare
NULL or NOT NULL for a column by specifying [NOT NULL | NULL] for the
column.

Thanks
Richard

If you replace is_not_null with NOT NULL in the query, everything works
correctly.
It seems that is_not_null is an incorrect keyword and it should not be
used, but I don't understand how it gets here.

Best regards,
Evgeniy

On Fri, Mar 28, 2025 at 7:12 PM Евгений Горбанев <gorbanyoves@basealt.ru> wrote:

If you replace is_not_null with NOT NULL in the query, everything works
correctly.
It seems that is_not_null is an incorrect keyword and it should not be
used, but I don't understand how it gets here.

It seems what happens is that internally in gram.y (~line 14274), the
DefElem for the not-null option is assigned the name "is_not_null".
As a result, this allows users to explicitly use "is_not_null" as the
option name. However, the value provided for the is_not_null option
in this way might not be a Boolean as expected, which triggers the
assertion.

I kind of doubt we should allow the use of the "is_not_null" keyword
in the xmltable function.

Hi Álvaro, what do you think about this?

Thanks
Richard

On Mon, Apr 14, 2025 at 5:45 PM Richard Guo <guofenglinux@gmail.com> wrote:

It seems what happens is that internally in gram.y (~line 14274), the
DefElem for the not-null option is assigned the name "is_not_null".
As a result, this allows users to explicitly use "is_not_null" as the
option name. However, the value provided for the is_not_null option
in this way might not be a Boolean as expected, which triggers the
assertion.

I kind of doubt we should allow the use of the "is_not_null" keyword
in the xmltable function.

Hi Álvaro, what do you think about this?

Just realized Álvaro has switched to a new email address — re-cc'ing.

Thanks
Richard

On 2025-Apr-14, Richard Guo wrote:

It seems what happens is that internally in gram.y (~line 14274), the
DefElem for the not-null option is assigned the name "is_not_null".
As a result, this allows users to explicitly use "is_not_null" as the
option name. However, the value provided for the is_not_null option
in this way might not be a Boolean as expected, which triggers the
assertion.

I kind of doubt we should allow the use of the "is_not_null" keyword
in the xmltable function.

Hi Álvaro, what do you think about this?

Hello Richard, sorry that I failed to notice this earlier.

I agree that blocking the index from using the option name that xmltable
parsing uses internally is okay. Maybe we can rename it to something
like "__pg__is_not_null" or something like that, which would reduce the
chances of troubling people; the existing name sounds too much like a
valid name that users could want to use.

Also, maybe rather than just "syntax error" we could say something like
"option name XYZ cannot be used in XMLTABLE".

I wonder if we have any other names used by the parser that can cause
this kind of problem. In a quick look through gram.y I didn't find any
other place that would fabricate a name and also accept arbitrary
user-specified names to use, so this seems to be the only place affected
by this particular bug.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On Fri, May 9, 2025 at 6:50 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

I agree that blocking the index from using the option name that xmltable
parsing uses internally is okay. Maybe we can rename it to something
like "__pg__is_not_null" or something like that, which would reduce the
chances of troubling people; the existing name sounds too much like a
valid name that users could want to use.

Also, maybe rather than just "syntax error" we could say something like
"option name XYZ cannot be used in XMLTABLE".

Agreed on both points.

Evgeniy's patch checks the type of the option argument at the last
moment and raises an error if it's not Boolean. I think it might be
better to check whether the user-supplied name collides with the
internally reserved name earlier in the process. According to the
doc, users should specify column nullability using [NOT NULL | NULL]
in the column definition, rather than explicitly setting an
"is_not_null" option.

Attached is a patch that implements this. It also renames the
internally used option name and updates the error message.

I wonder if we have any other names used by the parser that can cause
this kind of problem. In a quick look through gram.y I didn't find any
other place that would fabricate a name and also accept arbitrary
user-specified names to use, so this seems to be the only place affected
by this particular bug.

Yeah. Thanks for checking.

Thanks
Richard

On 2025-May-14, Richard Guo wrote:

Attached is a patch that implements this. It also renames the
internally used option name and updates the error message.

LGTM.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Thu, May 15, 2025 at 1:20 AM Alvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-May-14, Richard Guo wrote:

Attached is a patch that implements this. It also renames the
internally used option name and updates the error message.

LGTM.

Thanks. I've pushed this patch and backpatched it through v15. I
didn't backpatch it to v14 or v13, as those branches don't use boolVal
to retrieve the value of the "is_not_null" option, so the assertion
failure doesn't occur there.

Thanks
Richard