[PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

Bharath Rupireddy
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

v1-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

Rushabh Lathia


On Fri, Jun 26, 2020 at 3:16 PM Bharath Rupireddy <[hidden email]> 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.



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


Thanks,
Rushabh Lathia

v2-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

vignesh C
On Fri, Jun 26, 2020 at 6:15 PM Rushabh Lathia <[hidden email]> wrote:

>
>
>
> On Fri, Jun 26, 2020 at 3:16 PM Bharath Rupireddy <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

Bharath Rupireddy
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

v3-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

vignesh C
On Sat, Jun 27, 2020 at 9:23 AM Bharath Rupireddy
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM

Bharath Rupireddy
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 <[hidden email]> wrote:

>
> On Sat, Jun 27, 2020 at 9:23 AM Bharath Rupireddy
> <[hidden email]> 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