Skip to content

Conversation

@KhaledR57
Copy link
Contributor

  • The Jira issue number for this PR is: MDEV-37072

Description

Add support for the SQL standard IS JSON predicate with the syntax:

<expr> IS [ NOT ] JSON
             [ { VALUE | ARRAY | OBJECT | SCALAR } ]
             [ { WITH  | WITHOUT } UNIQUE [ KEYS ] ]

The predicate allows checking if an expression is valid JSON and optionally constrains the JSON type (VALUE, ARRAY, OBJECT, SCALAR) and whether object keys are unique.

The implementation includes:

  • Basic IS JSON validation
  • Support for NOT operator
  • Type constraints (VALUE, ARRAY, OBJECT, SCALAR)
  • Unique keys constraint (WITH/WITHOUT UNIQUE KEYS)

How can this PR be tested?

$ ./mtr main.func_json

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@KhaledR57 KhaledR57 marked this pull request as draft October 2, 2025 04:43
@grooverdan
Copy link
Member

All test results: https://buildbot.mariadb.org/#/grid?branch=refs%2Fpull%2F4332%2Fhead

main.mdev-35046 appears potentially unrelated to your work.

A cursory look though this shows many good practices. Mark as ready to review (remove draft) when you are ready.

@KhaledR57 KhaledR57 marked this pull request as ready for review October 2, 2025 05:10
@KhaledR57 KhaledR57 force-pushed the MDEV-37072-is-json-predicate branch from 51054ba to f8ca796 Compare October 2, 2025 09:25
@svoj svoj added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Oct 2, 2025
@andrelkin
Copy link
Contributor

andrelkin commented Oct 6, 2025

As to the failing mdev-35046.test it be of great help to restore 51054ba173 headed branch for investigation. Sadly or remarkably the failure with

Warning: Internal memory accounting error of 32960 bytes

reliably happened at building only that branch, prior to its forced reset to f8ca79667. The latter commit witnessed just one failure incident.

Perhaps that could be done dear @KhaledR57 with help of git reflog etc?

@KhaledR57
Copy link
Contributor Author

@andrelkin Thank you for the comment.
I've reverted to commit 51054ba173 and found the cause of the memory accounting error was a memory leak in check_unique_keys function. I was allocating keys with my_malloc() without providing my_free function during my_hash_init().
The fix is to pass my_free to the hash initialization, so the hash table frees keys automatically in my_hash_free().

The reason it doesn't occur in the last commit f8ca79667 is that I had switched to malloc() instead of my_malloc() (I mistakenly thought my_malloc was the issue, not the missing my_free), which hid the error but didn’t actually fix the leak.

Regarding mdev-35046.test, I could not reproduce the specific failure, it passes locally.

@KhaledR57 KhaledR57 force-pushed the MDEV-37072-is-json-predicate branch from f8ca796 to a64dc27 Compare October 7, 2025 04:37
@andrelkin
Copy link
Contributor

andrelkin commented Oct 7, 2025

@KhaledR57, thank you, Khaled!

This explains MDEV-37789, well almost does so because

The reason it doesn't occur in the last commit f8ca796

this commit did fail too on amd64-ubasan-clang-20 with an MSAN error.

This needs explaining from your part.

To

it passes locally.

mdev-35046.test always [ pass ] to me when run alone. MTR sees the leak at the server shutdown, when its worker executes a series of tests within one server launch. And mdev-35046 is the last and only test in a serious that calls shutdown.
Therefore the leak source can be identified by scrutinizing all tests of mdev-35046.test 's mtr worker preceding this test.

@andrelkin
Copy link
Contributor

andrelkin commented Oct 7, 2025

Perhaps you have taken care of it in the latest a64dc2776b which does not fail on the test.
Nevertheless to help in examining the LeakSanitizer issue of f8ca796, you can find more to the following in logs/normal/16/log/mysqld.1.err

2025-10-02  9:43:19 0 [Note] InnoDB: Shutdown completed; log sequence number 9952251; transaction id 5185
2025-10-02  9:43:19 0 [Note] /home/buildbot/bld/sql/mariadbd: Shutdown complete

=================================================================
==223778==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8893 byte(s) in 1000 object(s) allocated from:
    #0 0x555d870a9d14 in malloc (/home/buildbot/bld/sql/mariadbd+0x1fbbd14) (BuildId: 55f809e94e2dc940a0ef2dc660fa775a4c5eaa38)
    #1 0x555d87e20e18 in check_unique_keys(st_json_engine_t*) /home/buildbot/src/sql/item_jsonfunc.cc:6297:30
    #2 0x555d87e20e18 in Item_func_is_json::val_bool() /home/buildbot/src/sql/item_jsonfunc.cc:6376:20
    #3 0x555d871d37e3 in Item_bool_func::val_int() /home/buildbot/src/sql/item_cmpfunc.h:245:12
    #4 0x555d87f41cff in Type_handler::Item_send_long(Item*, Protocol*, st_value*) const /home/buildbot/src/sql/sql_type.cc:7606:22
    #5 0x555d872104c2 in Protocol::send_result_set_row(List<Item>*) /home/buildbot/src/sql/protocol.cc:1359:15
    #6 0x555d874344b5 in select_send::send_data(List<Item>&) /home/buildbot/src/sql/sql_class.cc:3348:17
    #7 0x555d87432daa in select_result_sink::send_data_with_check(List<Item>&, st_select_lex_unit*, unsigned long long) /home/buildbot/src/
sql/sql_class.cc:3246:11
    #8 0x555d877cdeb0 in JOIN::exec_inner() /home/buildbot/src/sql/sql_select.cc:4957:22
    #9 0x555d877cc17b in JOIN::exec() /home/buildbot/src/sql/sql_select.cc:4874:8
    #10 0x555d87752f13 in mysql_select(THD*, TABLE_LIST*, List<Item>&, Item*, unsigned int, st_order*, st_order*, Item*, st_order*, unsigne
d long long, select_result*, st_select_lex_unit*, st_select_lex*) /home/buildbot/src/sql/sql_select.cc:5402:21
    #11 0x555d877515e3 in handle_select(THD*, LEX*, select_result*, unsigned long long) /home/buildbot/src/sql/sql_select.cc:634:10
    #12 0x555d87650eff in execute_sqlcom_select(THD*, TABLE_LIST*) /home/buildbot/src/sql/sql_parse.cc:6167:12
    #13 0x555d87634fb3 in mysql_execute_command(THD*, bool) /home/buildbot/src/sql/sql_parse.cc:3950:12
    #14 0x555d87618733 in mysql_parse(THD*, char*, unsigned int, Parser_state*) /home/buildbot/src/sql/sql_parse.cc:7883:18
    #15 0x555d8760f6b7 in dispatch_command(enum_server_command, THD*, char*, unsigned int, bool) /home/buildbot/src/sql/sql_parse.cc:1878:7
    #16 0x555d8761acf5 in do_command(THD*, bool) /home/buildbot/src/sql/sql_parse.cc:1417:17
    #17 0x555d87c2e1bc in do_handle_one_connection(CONNECT*, bool) /home/buildbot/src/sql/sql_connect.cc:1414:11
    #18 0x555d87c2d9f5 in handle_one_connection /home/buildbot/src/sql/sql_connect.cc:1326:5
    #19 0x555d88eef498 in pfs_spawn_thread /home/buildbot/src/storage/perfschema/pfs.cc:2198:3
    #20 0x555d870a7696 in asan_thread_start(void*) asan_interceptors.cpp.o

If that's resolved or made more clear in any form, feel free to leave a comment on https://jira.mariadb.org/browse/MDEV-37789 that seems to be your PR offspring.

@KhaledR57
Copy link
Contributor Author

@andrelkin

The reason it doesn't occur in the last commit f8ca796

Sorry, I meant that the memory error didn't happen as often in commit f8ca796 compared to 51054ba. This suggests that my changes in f8ca796 partially masked the issue but didn't actually fix the root cause.

The LeakSanitizer trace confirms the leak is in my check_unique_keys function where I'm using malloc() instead of my_malloc().

I've fixed this in the latest commit a64dc27 by:

  1. Using my_malloc() instead of malloc()
  2. Adding my_free to the hash free function

I'll update the JIRA ticket MDEV-37789 with this information.

Also, Is there a way to re-run the failing test (buildbot/amd64-msan-clang-20)? It would be helpful to confirm that everything is working correctly with the fix.

@andrelkin
Copy link
Contributor

The MSAN building environments incl docker one is wonderfully described here.

I tried the docker method successfully.

partially masked the issue but didn't actually fix the root cause.

Perhaps so. However considering that the branches marked by LeakSanitizer failure were all in development/preview and such may still point to this PR.

It'd be helpful for either side if you'll manage to run mtr reproducing precisely its BB failing runs tests distribution (that is composing the same subsets to mtr workers, esp to the mdev-35046's worker) with --repeat=500 or greater value.

@KhaledR57
Copy link
Contributor Author

@andrelkin Thank you again for the guidance. I am currently setting up the environment to investigate the failing tests as you suggested.

I'm hoping you could clarify one point to help my understanding. You mentioned that the LeakSanitizer failures might still point to this PR. I'm a bit confused by this because I believe the latest commit a64dc27 correctly fixes the leak originating from my code, and mdev-35046 is now passing in all recent builds with this fix.

I was also looking at the build history, and it seems the LeakSanitizer failures on this test occurred on branches that were created before my PR. This suggests the underlying issue might be pre-existing, and perhaps my changes just made it easier to spot.

Either way, you're right that a stress test is the best way to be sure. I'm running the test now on the exact same group of tests from the buildbot worker.

I'll let you know the results as soon as the test completes. Thanks again for your help!

@andrelkin
Copy link
Contributor

You mentioned that the LeakSanitizer failures might still point to this PR. I'm a bit confused by this because I believe the latest commit a64dc27 correctly fixes the leak originating from my code

I was concerned with two things:
that just few mtr runs would not be enough and that if there exists an "indicator" test we might capture it now.
And thank you for addressing this 2nd point

I'm running the test now on the exact same group of tests from the buildbot worker.

!

@KhaledR57
Copy link
Contributor Author

The discussion around the failing mdev-35046 test has moved to MDEV-37789.

Add support for the SQL standard IS JSON predicate with the syntax:
expr IS [ NOT ] JSON [ { VALUE | ARRAY | OBJECT | SCALAR } ]
[ { WITH | WITHOUT } UNIQUE [ KEYS ] ]

The predicate allows checking if an expression is valid JSON
and optionally constrains the JSON type (VALUE, ARRAY, OBJECT,
SCALAR) and whether object keys are unique.

The implementation includes:
- Basic IS JSON validation
- Support for NOT operator
- Type constraints (VALUE, ARRAY, OBJECT, SCALAR)
- Unique keys constraint (WITH/WITHOUT UNIQUE KEYS)
@KhaledR57 KhaledR57 force-pushed the MDEV-37072-is-json-predicate branch 2 times, most recently from 9cf0a31 to 0a75d8e Compare October 15, 2025 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

4 participants