On Thu, Apr 20, 2023 at 2:28 PM shveta malik <shveta.ma...@gmail.com> wrote: > > Few comments for ddl_json.c in the patch dated April17: > ... > > 3) expand_jsonb_array() > arrayelem is allocated as below, but not freed. > initStringInfo(&arrayelem) > > 4) expand_jsonb_array(), > we initialize iterator as below which internally does palloc > it = JsonbIteratorInit(container); > Shall this be freed at the end? I see usage of this function in other files. > At a few places, it is freed while not freed at other places. >
Normally, it is a good idea to free whenever the allocation is done in a long-lived context. However, in some places, we free just for the sake of cleanliness. I think we don't need to bother doing retail free in this case unless it is allocated in some long-lived context. > 5) deparse_ddl_json_to_string(char *json_str, char** owner) > str = palloc(value->val.string.len + 1); > we do palloc here and return allocated memory to caller as 'owner'. Caller > sets this 'owner' using SetConfigOption() which internally allocates new > memory and copies 'owner' to that. So the memory allocated in > deparse_ddl_json_to_string is never freed. Better way should be the caller > passing this allocated memory to deparse_ddl_json_to_string() and freeing it > when done. Thoughts? > I think that will complicate the code. I don't see the need to allocate this in any longer-lived context, so we shouldn't be bothered to retail pfree it. > 6)expand_fmt_recursive(): > value = findJsonbValueFromContainer(container, JB_FOBJECT, &key); > Should this 'value' be freed at the end like we do at all other places in > this file? > Yeah, we can do this for the sake of consistency. Few comments on 0001 patch: ============================= 1. + case 'O': + specifier = SpecOperatorName; + break; ... + case 'R': + specifier = SpecRole; + break; + default: Both of these specifiers don't seem to be used in the patch. So, we can remove them. I see some references to 'O' in the comments, those also need to be modified. 2. + /* For identity column, find the sequence owned by column in order + * to deparse the column definition */ In multi-line comments, the first and last lines should be empty. Refer to multi-line comments at other places. 3. + return object_name.data; + +} An extra empty line before } is not required. -- With Regards, Amit Kapila.
