[PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t42594psql -h localhost -U postgresBuilt from patchset v9 (message #9), August 18, 2026 at 04:19 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t42594_9 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t42594_9 && git checkout t42594_9Patchset v9 (message #9) is on t42594_9
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
Attachments:
v1-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patchapplication/octet-stream; name=v1-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patchDownload+9-2
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
Attachments:
v2-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patchDownload+11-1
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
Attachments:
v3-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patchapplication/octet-stream; name=v3-0001-Remove-Extra-palloc-Of-raw_buf-For-Binary-Format-.patchDownload+8-2
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
Show quoted text
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