agroal / pgagroal

High-performance connection pool for PostgreSQL
https://agroal.github.io/pgagroal/
BSD 3-Clause "New" or "Revised" License
684 stars 61 forks source link

[#465] Replace cJSON dependancy with the native in-tree json API #477

Closed MohanadKh03 closed 3 weeks ago

MohanadKh03 commented 1 month ago

Replaced cJSON completely with the native json parser. Tested manually using the following commands from the docs user guide section 6.4 JSON Output Format

pgagroal-cli conf ls --format json

pgagroal-cli conf set max_connections 1000 --format json

pgagroal-cli status --format json

pgagroal-cli ping --format json

Though I noticed that when using text format it gave a status of 1 and exits for some reason ? After debugging I think the problem is that some exit status is not what it should be I think ? I am kind of stuck in it cuz now it's only working with json formats not text somehow

jesperpedersen commented 1 month ago

Start by putting the definitions into MANAGEMENT_ARGUMENT_XYZ defines.

Also, look at the return values from the functions so f.ex. .._json_create should result into a goto error: if it fails.

But, you are close on this !

MohanadKh03 commented 1 month ago

Ok so appearantly the issue was the way printing using text was coupled with cJSON which would have wasted a lot of time and effort to debug on it so instead I used the native API's way --> pgagroal_json_print_and_free_json_object and renamed it cuz it will have both json and text and possibly any other format in the future ?

As for the definitions , not sure if I got this right but I tried moving the functions to the management.h file though they are static so do you mean to remove them being static or move some other functions ?

Added another commit , I know I should squash but for the sake of readability since this has a lot of changes I let it like this till all is good then ill squash WDYT ?

Update: CI failing because of missing memory leaks management . fixed that but it still is failing not sure why ? even though it's working when using make locally using gcc though the CI uses clang i think

jesperpedersen commented 1 month ago

See CI - always init to NULL then assign after

jesperpedersen commented 1 month ago

@MohanadKh03 See https://github.com/agroal/pgagroal/issues/466 which is next, and how it is done in pgmoneta.

You can just follow cli.c -> management.c -> main.c -> ?

jesperpedersen commented 1 month ago

@MohanadKh03 You can do both at the same time if that is easier... and you want to

MohanadKh03 commented 1 month ago

It's all good I will fix the memory leaks issue and continue with this one before getting to the 466 @jesperpedersen Quick question though why did the leak appear only on the linux build ? Also there seems to be a lot of stuff that are not freed at the end of lots of functions even before this PR's changes so does this have to do with sth else not memory leaks ? CI says AddressSanitizer: 6 byte(s) leaked in 1 allocation(s). which is understandable there is indeed sth that is not freed up but this has been around for a while so why did it say there were 6 bytes leaked now ?

 if (pgagroal_executable_version_string(&version_buf, version))
   {
      goto error;
   }

This is the end of the function which has been like this before my changes so im kinda confused why did it detect a leak only now not before ?

 free(buffer);
   return json;
error:
   free(buffer);
   return NULL;
jesperpedersen commented 1 month ago

Maybe it depends on the path of the code in question, and likely the Mac builds are using much older software.

I use clang 18.x w/ Debug to catch most of them - valgrind is too slow

jesperpedersen commented 1 month ago

... and GitHub have seemed to update their platform yesterday so more reports are coming in now - I'm guessing the ubuntu-latest image is different now