Обсуждение: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

Hi Hackers,

There seems to be an extra palloc of 64KB of raw_buf for binary format
files which is not required
as copy logic for binary files don't use raw_buf, instead, attribute_buf
is used in CopyReadBinaryAttribute.

Attached is a patch, which places a check to avoid this unnecessary 64KB palloc.

Request the community to take this patch, if it is useful.

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

On Fri, Jun 26, 2020 at 6:15 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>
>
>
> On Fri, Jun 26, 2020 at 3:16 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> Hi Hackers,
>>
>> There seems to be an extra palloc of 64KB of raw_buf for binary format
>> files which is not required
>> as copy logic for binary files don't use raw_buf, instead, attribute_buf
>> is used in CopyReadBinaryAttribute.
>
>
> +1
>
> I looked at the patch and the changes looked good. Couple of comments;
>
> 1)
>
> +
> + /* For binary files raw_buf is not used,
> + * instead, attribute_buf is used in
> + * CopyReadBinaryAttribute. Hence, don't palloc
> + * raw_buf.
> + */
>
> Not a PG style of commenting.
>
> 2)  In non-binary mode, should assign NULL the raw_buf.
>
> Attaching patch with those changes.
>

+1 for the patch.

One comment:
We could change below code:
+ */
+ if (!cstate->binary)
+ cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
+ else
+ cstate->raw_buf = NULL;
to:
cstate->raw_buf = (cstate->binary) ? NULL : (char *) palloc(RAW_BUF_SIZE + 1);

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Thanks Rushabh and Vignesh for the comments.

>
> One comment:
> We could change below code:
> + */
> + if (!cstate->binary)
> + cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
> + else
> + cstate->raw_buf = NULL;
> to:
> cstate->raw_buf = (cstate->binary) ? NULL : (char *) palloc(RAW_BUF_SIZE + 1);
>

Attached the patch with the above changes.

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

On Sat, Jun 27, 2020 at 9:23 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Thanks Rushabh and Vignesh for the comments.
>
> >
> > One comment:
> > We could change below code:
> > + */
> > + if (!cstate->binary)
> > + cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
> > + else
> > + cstate->raw_buf = NULL;
> > to:
> > cstate->raw_buf = (cstate->binary) ? NULL : (char *) palloc(RAW_BUF_SIZE + 1);
> >
>
> Attached the patch with the above changes.

Changes look fine to me.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Thanks Vignesh and Rushabh for reviewing this.

I've added this patch to commitfest - https://commitfest.postgresql.org/28/.

Request community take this patch further if there are no further issues.

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

On Sat, Jun 27, 2020 at 6:30 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, Jun 27, 2020 at 9:23 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Thanks Rushabh and Vignesh for the comments.
> >
> > >
> > > One comment:
> > > We could change below code:
> > > + */
> > > + if (!cstate->binary)
> > > + cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
> > > + else
> > > + cstate->raw_buf = NULL;
> > > to:
> > > cstate->raw_buf = (cstate->binary) ? NULL : (char *) palloc(RAW_BUF_SIZE + 1);
> > >
> >
> > Attached the patch with the above changes.
>
> Changes look fine to me.
>
> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com

On Tue, Jun 30, 2020 at 2:41 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Thanks Vignesh and Rushabh for reviewing this.
>
> I've added this patch to commitfest - https://commitfest.postgresql.org/28/.

I felt this patch is ready for committer, changing the status to ready
for committer.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

vignesh C <vignesh21@gmail.com> writes:
> On Tue, Jun 30, 2020 at 2:41 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>> I've added this patch to commitfest - https://commitfest.postgresql.org/28/.

> I felt this patch is ready for committer, changing the status to ready
> for committer.

Pushed with some fiddling.  Mainly, if we're going to the trouble of
checking for binary mode here, we might as well skip allocating the
line_buf too.

            regards, tom lane

On Sat, Jul 11, 2020 at 11:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> vignesh C <vignesh21@gmail.com> writes:
> > On Tue, Jun 30, 2020 at 2:41 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >> I've added this patch to commitfest - https://commitfest.postgresql.org/28/.
>
> > I felt this patch is ready for committer, changing the status to ready
> > for committer.
>
> Pushed with some fiddling.  Mainly, if we're going to the trouble of
> checking for binary mode here, we might as well skip allocating the
> line_buf too.
>

Hi Tom,

Isn't it good if we backpatch this to versions 13, 12, 11 and so on?
As we can save good amount of memory with this patch for non-binary
copy.

Attaching the patch which applies on versions 13, 12, 11.

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

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> On Sat, Jul 11, 2020 at 11:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Pushed with some fiddling.  Mainly, if we're going to the trouble of
>> checking for binary mode here, we might as well skip allocating the
>> line_buf too.

> Isn't it good if we backpatch this to versions 13, 12, 11 and so on?

Given the lack of complaints, I wasn't excited about it.  Transient
consumption of 64K is not a huge deal these days.  (And yes, I've
worked on machines where that was the entire address space.  But that
was a very long time ago.)

            regards, tom lane