On Wed, 7 Sept 2022 at 14:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 7, 2022 at 9:53 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v47 patch has the changes for the same.
> >
>
> V47-0001* looks good to me apart from below minor things. I would like
> to commit this tomorrow unless there are more comments on it.
>
> Few minor suggestions:
> ==================
> 1.
> + list_free_deep(publist);
> + pfree(pubnames->data);
> + pfree(pubnames);
>
> I don't think we need to free this memory as it will automatically be
> released at end of the command (this memory is allocated in portal
> context). I understand there is no harm in freeing it as well but
> better to leave it as there doesn't appear to be any danger of leaking
> memory for a longer time.
Modified
> 2.
> + res = walrcv_exec(wrconn, cmd.data, 1, tableRow);
> + pfree(cmd.data);
>
> Don't you need to free cmd as well here? I think it is better to avoid
> freeing it due to reasons mentioned in the previous point.
cmd need not be freed here as we use initStringInfo for initialization
and initStringInfo allocates memory for data member only.
The attached patch has the changes for the same.
Regards,
Vignesh
