Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
Bharath Rupireddy writes: > On Sat, Jul 11, 2020 at 11:53 PM Tom Lane 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
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
On Sat, Jul 11, 2020 at 11:53 PM Tom Lane wrote: > > vignesh C writes: > > On Tue, Jun 30, 2020 at 2:41 PM Bharath Rupireddy > > 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 v1-0001-Avoid-useless-buffer-allocations-during-binary-CO.patch Description: Binary data
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
vignesh C writes: > On Tue, Jun 30, 2020 at 2:41 PM Bharath Rupireddy > 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
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
On Tue, Jun 30, 2020 at 2:41 PM Bharath Rupireddy 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
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
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 wrote: > > On Sat, Jun 27, 2020 at 9:23 AM Bharath Rupireddy > 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
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
On Sat, Jun 27, 2020 at 9:23 AM Bharath Rupireddy 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
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
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 Description: Binary data
Re: [PATCH] Remove Extra palloc Of raw_buf For Binary Format In COPY FROM
On Fri, Jun 26, 2020 at 6:15 PM Rushabh Lathia wrote: > > > > On Fri, Jun 26, 2020 at 3:16 PM Bharath Rupireddy > 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
Re: [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.rupireddyforpostg...@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 diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 6d53dc4..97170d3 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -3368,7 +3368,17 @@ BeginCopyFrom(ParseState *pstate, initStringInfo(&cstate->attribute_buf); initStringInfo(&cstate->line_buf); cstate->line_buf_converted = false; - cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1); + + /* + * For binary files raw_buf is not used and instead attribute_buf + * is used in CopyReadBinaryAttribute. Hence, don't palloc raw_buf + * for binary files. + */ + if (!cstate->binary) + cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1); + else + cstate->raw_buf = NULL; + cstate->raw_buf_index = cstate->raw_buf_len = 0; /* Assign range table, we'll need it in CopyFrom. */
