Re: Narrow the scope of the variable outputstr in logicalrep_write_tuple

On Mon, Jan 18, 2021 at 1:39 PM japin <japinli(at)hotmail(dot)com> wrote:
>
>
> On Mon, 18 Jan 2021 at 15:59, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > On Mon, Jan 18, 2021 at 1:16 PM japin <japinli(at)hotmail(dot)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