Обсуждение: Narrow the scope of the variable outputstr in logicalrep_write_tuple

Hi,

I find that the outputstr variable in logicalrep_write_tuple() only use in
`else` branch, I think we can narrow the scope, just like variable outputbytes
in `if` branch (for more readable).

        /*
         * Send in binary if requested and type has suitable send function.
         */
        if (binary && OidIsValid(typclass->typsend))
        {
            bytea      *outputbytes;
            int         len;

            pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
            outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
            len = VARSIZE(outputbytes) - VARHDRSZ;
            pq_sendint(out, len, 4);    /* length */
            pq_sendbytes(out, VARDATA(outputbytes), len);   /* data */
            pfree(outputbytes);
        }
        else
        {
            pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
            outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
            pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
            pfree(outputstr);
        }

Attached is a samll patch to fix it.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Jan 18, 2021 at 1:16 PM japin <japinli@hotmail.com> wrote:
>
>
> Hi,
>
> I find that the outputstr variable in logicalrep_write_tuple() only use in
> `else` branch, I think we can narrow the scope, just like variable outputbytes
> in `if` branch (for more readable).
>
>         /*
>          * Send in binary if requested and type has suitable send function.
>          */
>         if (binary && OidIsValid(typclass->typsend))
>         {
>             bytea      *outputbytes;
>             int         len;
>
>             pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
>             outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
>             len = VARSIZE(outputbytes) - VARHDRSZ;
>             pq_sendint(out, len, 4);    /* length */
>             pq_sendbytes(out, VARDATA(outputbytes), len);   /* data */
>             pfree(outputbytes);
>         }
>         else
>         {
>             pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
>             outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
>             pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
>             pfree(outputstr);
>         }
>
> Attached is a samll patch to fix it.

+1. Binary mode uses block level variable outputbytes, so making
outputstr block level is fine IMO.

Patch basically looks good to me, but it doesn't apply on my system.
Looks like it's not created with git commit. Please create the patch
with git commit command.

git apply /mnt/hgfs/Shared/narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patch
error: corrupt patch at line 10

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, 18 Jan 2021 at 15:59, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Mon, Jan 18, 2021 at 1:16 PM japin <japinli@hotmail.com> wrote:
>>
>>
>> Hi,
>>
>> I find that the outputstr variable in logicalrep_write_tuple() only use in
>> `else` branch, I think we can narrow the scope, just like variable outputbytes
>> in `if` branch (for more readable).
>>
>>         /*
>>          * Send in binary if requested and type has suitable send function.
>>          */
>>         if (binary && OidIsValid(typclass->typsend))
>>         {
>>             bytea      *outputbytes;
>>             int         len;
>>
>>             pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
>>             outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
>>             len = VARSIZE(outputbytes) - VARHDRSZ;
>>             pq_sendint(out, len, 4);    /* length */
>>             pq_sendbytes(out, VARDATA(outputbytes), len);   /* data */
>>             pfree(outputbytes);
>>         }
>>         else
>>         {
>>             pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
>>             outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
>>             pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
>>             pfree(outputstr);
>>         }
>>
>> Attached is a samll patch to fix it.
>
> +1. Binary mode uses block level variable outputbytes, so making
> outputstr block level is fine IMO.
>
> Patch basically looks good to me, but it doesn't apply on my system.
> Looks like it's not created with git commit. Please create the patch
> with git commit command.
>
> git apply /mnt/hgfs/Shared/narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patch
> error: corrupt patch at line 10
>

Thanks for reviewing! Attached v2 as you suggested.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Jan 18, 2021 at 1:39 PM japin <japinli@hotmail.com> wrote:
>
>
> On Mon, 18 Jan 2021 at 15:59, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> > On Mon, Jan 18, 2021 at 1:16 PM japin <japinli@hotmail.com> wrote:
> >>
> >>
> >> Hi,
> >>
> >> I find that the outputstr variable in logicalrep_write_tuple() only use in
> >> `else` branch, I think we can narrow the scope, just like variable outputbytes
> >> in `if` branch (for more readable).
> >>
> >>         /*
> >>          * Send in binary if requested and type has suitable send function.
> >>          */
> >>         if (binary && OidIsValid(typclass->typsend))
> >>         {
> >>             bytea      *outputbytes;
> >>             int         len;
> >>
> >>             pq_sendbyte(out, LOGICALREP_COLUMN_BINARY);
> >>             outputbytes = OidSendFunctionCall(typclass->typsend, values[i]);
> >>             len = VARSIZE(outputbytes) - VARHDRSZ;
> >>             pq_sendint(out, len, 4);    /* length */
> >>             pq_sendbytes(out, VARDATA(outputbytes), len);   /* data */
> >>             pfree(outputbytes);
> >>         }
> >>         else
> >>         {
> >>             pq_sendbyte(out, LOGICALREP_COLUMN_TEXT);
> >>             outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
> >>             pq_sendcountedtext(out, outputstr, strlen(outputstr), false);
> >>             pfree(outputstr);
> >>         }
> >>
> >> Attached is a samll patch to fix it.
> >
> > +1. Binary mode uses block level variable outputbytes, so making
> > outputstr block level is fine IMO.
> >
> > Patch basically looks good to me, but it doesn't apply on my system.
> > Looks like it's not created with git commit. Please create the patch
> > with git commit command.
> >
> > git apply /mnt/hgfs/Shared/narrow-the-scope-of-the-variable-in-logicalrep_write_tuple.patch
> > error: corrupt patch at line 10
> >
>
> Thanks for reviewing! Attached v2 as you suggested.

Thanks. v2 patch LGTM.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

japin <japinli@hotmail.com> writes:
> I find that the outputstr variable in logicalrep_write_tuple() only use in
> `else` branch, I think we can narrow the scope, just like variable outputbytes
> in `if` branch (for more readable).

Agreed, done.

For context, I'm not usually in favor of making one-off stylistic
improvements: the benefit seldom outweighs the risk of creating
merge hazards for future back-patching.  But in this case, the
code involved is mostly new in v14, so improving it now doesn't
cost anything in that way.

            regards, tom lane