Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ppQ7x-0006sI-PI for pgsql-hackers@arkaria.postgresql.org; Thu, 20 Apr 2023 08:58:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ppQ7w-0003u3-Mo for pgsql-hackers@arkaria.postgresql.org; Thu, 20 Apr 2023 08:58:52 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ppQ7w-0003tu-C7 for pgsql-hackers@lists.postgresql.org; Thu, 20 Apr 2023 08:58:52 +0000 Received: from mail-wm1-x329.google.com ([2a00:1450:4864:20::329]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1ppQ7u-00406L-9c for pgsql-hackers@lists.postgresql.org; Thu, 20 Apr 2023 08:58:51 +0000 Received: by mail-wm1-x329.google.com with SMTP id 5b1f17b1804b1-3f09b9ac51dso13870315e9.0 for ; Thu, 20 Apr 2023 01:58:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1681981128; x=1684573128; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=2urAvyPV6fxJ3I31vVhVWv00bxaEQEeEJCFuZkp4Eg0=; b=DMdvyr9kqvJwy9MZBMaY9peBmo/NAWxBeKuShrWvTsKjeF0i/UrcseZTESdwtLVH8b Xy+vStIreSWuLZaFX99nTxHHsl+xInm0t872+JKzm85KXJYKugzc/AyMgwR/Y/h19A1b JZLYAlYOYuHokkVGML9LTHJmyteDlbWicPBlSQNx1Myc869wsUTe00iKH+g5mHUde33c jOHRfIEWOwCkp2/TYy9Kbjm0MG/EE2a+vDtDVgN8goiu1GjTuOOyDP7Cbn59Z9hJuD00 /TRajGTsKDFi2pIgWsW1tfYCUViEmZDXWV0kuPWNX/siGDPO5p3LYe3sf7+ycLJ4Epy6 ix8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681981128; x=1684573128; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=2urAvyPV6fxJ3I31vVhVWv00bxaEQEeEJCFuZkp4Eg0=; b=bNKfN2D31/k9ESp79w9QwnL+dx16I+HHtUrkSLaPjIwwYWVWK6RTA9e5gDxiIEyxVd aNdx18h4BNkc5z6Wp7RH9n0DhVLj2ZyltPHF1wafIi0n/jyBAUu9lfqOBkfWcLkWOGjF Bb0XI4fYwzMo5nA6wwHF4S/QXhDybkqX0sHqGPi5JTWnvZzbVZjGBzPe9hXRNWeZybBZ aR6VZMhr+BwA6L0ncJ7loJzbEKh/9uCvi7zxIj9aekqlzeAR16QHlWmhs3E9Reb6BC6X fiP6ynvUeODMVb4uv0316m6EBn5bG1YKjC2xoBc7H4lsntHR4kFEEHmqKobUdQUvRHMJ GURw== X-Gm-Message-State: AAQBX9dmMfoVvYDX5C9e88gFbaYJGcBYYe0RO98urla043GkC5L+ZAl7 TeAG/bYpxb9TR2CCpGCg5Mk1X0DOwj/3qXXBMzA= X-Google-Smtp-Source: AKy350aemtHU1dToS8udm/9g4sYYv9S9WjX+p8AhVqSSAD5N9DJmB1ftFqhaF9ZbEXQo8ssMZRe/fJAHST1Ng6ZynBg= X-Received: by 2002:adf:e689:0:b0:2f9:805f:eecb with SMTP id r9-20020adfe689000000b002f9805feecbmr788353wrm.3.1681981128420; Thu, 20 Apr 2023 01:58:48 -0700 (PDT) MIME-Version: 1.0 References: <3032112.1679865718@sss.pgh.pa.us> In-Reply-To: From: shveta malik Date: Thu, 20 Apr 2023 14:28:36 +0530 Message-ID: Subject: Re: Support logical replication of DDLs To: "Zhijie Hou (Fujitsu)" Cc: Amit Kapila , vignesh C , Ajin Cherian , "Wei Wang (Fujitsu)" , Runqi Tian , Peter Smith , Tom Lane , li jie , Dilip Kumar , Alvaro Herrera , Masahiko Sawada , Japin Li , rajesh singarapu , PostgreSQL Hackers , Zheng Li , shveta malik Content-Type: multipart/alternative; boundary="000000000000adb02605f9c0bf29" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000adb02605f9c0bf29 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Apr 20, 2023 at 9:11=E2=80=AFAM shveta malik wrote: > On Mon, Apr 17, 2023 at 5:32=E2=80=AFPM Zhijie Hou (Fujitsu) > wrote: > > > > Attach the new version patch set which include the following changes: > > > > Few comments for ddl_deparse.c in patch dated April17: > > Few comments for ddl_json.c in the patch dated April17: 1) expand_jsonval_string() I think we need to assert if jsonval is neither jbvString nor jbvBinary. 2) expand_jsonval_strlit() same here, assert if not jbvString (like we do in expand_jsonval_number and expand_jsonval_identifier etc) 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 =3D 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. 5) deparse_ddl_json_to_string(char *json_str, char** owner) str =3D 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? 6)expand_fmt_recursive(): value =3D findJsonbValueFromContainer(container, JB_FOBJECT, &key); Should this 'value' be freed at the end like we do at all other places in this file? thanks Shveta --000000000000adb02605f9c0bf29 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Apr 20, 2023 at 9:11=E2=80=AF= AM shveta malik <shveta.malik@= gmail.com> wrote:
On Mon, Apr 17, 2023 at 5:32=E2=80=AFPM Zhijie Hou (Fujitsu)
<houzj.fnst@= fujitsu.com> wrote:
>
> Attach the new version patch set which include the following c= hanges:
>

Few comments for ddl_deparse.c in patch dated April17:

=

=C2=A0Few comments for ddl_json.c in the patch dated Ap= ril17:

1) expand_jsonval_string()
I think we need to assert= if jsonval is neither jbvString nor jbvBinary.

2) expand_jsonval_st= rlit()
same here, assert if not jbvString (like we do in expand_jsonval_= number=C2=A0and expand_jsonval_identifier etc)

3) expand_jsonb_array= ()
arrayelem is allocated as below, but not freed.
initStringInfo(&am= p;arrayelem)

4) expand_jsonb_array(),
we initialize iterator as = below which internally does palloc
it =3D JsonbIteratorInit(container);<= br>Shall this be freed at the end? I see usage of this function in other fi= les.=C2=A0At a few places, it is freed while not freed at other places.
=
5) deparse_ddl_json_to_string(char *json_str, char** owner)
str =3D = palloc(value->val.string.len + 1);
we do=C2=A0 palloc here and return= allocated memory to caller as 'owner'.=C2=A0Caller sets this '= owner' using SetConfigOption() which internally allocates new memory an= d copies 'owner' to that. So the memory allocated in deparse_ddl_js= on_to_string is never freed.=C2=A0Better way should be the caller passing t= his allocated memory to deparse_ddl_json_to_string()=C2=A0and freeing it wh= en done. Thoughts?

6)expand_fmt_recursive():
value =3D findJsonbV= alueFromContainer(container, JB_FOBJECT, &key);
Should this = 9;value' be freed at the end like we do at all other places in this fil= e?=C2=A0


thanks
Shveta

--000000000000adb02605f9c0bf29--