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

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.

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