Обсуждение: Thinko/typo in ExecSimpleRelationInsert
Hi, There seems to be a thinko/typo in ExecSimpleRelationInsert(). A tuple can never store a slot, but a comment in that function says so. Tried to fix it in the patch attached. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Вложения
Looks like we need similar adjustment in ExecSimpleRelationUpdate() as well. Updated the patch. On Tue, Jun 26, 2018 at 3:12 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > Hi, > There seems to be a thinko/typo in ExecSimpleRelationInsert(). A tuple > can never store a slot, but a comment in that function says so. Tried > to fix it in the patch attached. > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Вложения
On Tue, Jun 26, 2018 at 4:33 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > Looks like we need similar adjustment in ExecSimpleRelationUpdate() as > well. Updated the patch. > - /* Store the slot into tuple that we can write. */ + /* Materialize slot into a tuple that we can inspect. */ tuple = ExecMaterializeSlot(slot); I think it is better to keep the last word of the sentence as "write" instead of "inspect" as was in the original sentence. It makes more sense as we are materializing the tuple to write it. Similarly, in the other change in the patch can use "write". -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Jun 26, 2018 at 6:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Tue, Jun 26, 2018 at 4:33 PM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> Looks like we need similar adjustment in ExecSimpleRelationUpdate() as >> well. Updated the patch. >> > > - /* Store the slot into tuple that we can write. */ > + /* Materialize slot into a tuple that we can inspect. */ > tuple = ExecMaterializeSlot(slot); > > I think it is better to keep the last word of the sentence as "write" > instead of "inspect" as was in the original sentence. A copy-pasto while correcting a typo :) > It makes more > sense as we are materializing the tuple to write it. Similarly, in > the other change in the patch can use "write". Are you suggesting that we should use "write" in the modified comment instead of "inspect" in original comment. Ok, I have now corrected grammar as well. Here's updated patch. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Вложения
On Tue, Jun 26, 2018 at 7:02 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Tue, Jun 26, 2018 at 6:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Tue, Jun 26, 2018 at 4:33 PM, Ashutosh Bapat >> <ashutosh.bapat@enterprisedb.com> wrote: >>> Looks like we need similar adjustment in ExecSimpleRelationUpdate() as >>> well. Updated the patch. >>> >> >> - /* Store the slot into tuple that we can write. */ >> + /* Materialize slot into a tuple that we can inspect. */ >> tuple = ExecMaterializeSlot(slot); >> >> I think it is better to keep the last word of the sentence as "write" >> instead of "inspect" as was in the original sentence. > > A copy-pasto while correcting a typo :) > >> It makes more >> sense as we are materializing the tuple to write it. Similarly, in >> the other change in the patch can use "write". > > Are you suggesting that we should use "write" in the modified comment > instead of "inspect" in original comment. > Yes. Another variant used in few other similar places is: /* * get the heap tuple out of the tuple table slot, making sure we have a * writable copy */ > Ok, I have now corrected grammar as well. Here's updated patch. > Your new comment is also correct. I like the comment already used in code as that makes the code consistent, what do you think? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Jun 26, 2018 at 8:43 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Tue, Jun 26, 2018 at 7:02 PM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> On Tue, Jun 26, 2018 at 6:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> On Tue, Jun 26, 2018 at 4:33 PM, Ashutosh Bapat >>> <ashutosh.bapat@enterprisedb.com> wrote: >>>> Looks like we need similar adjustment in ExecSimpleRelationUpdate() as >>>> well. Updated the patch. >>>> >>> >>> - /* Store the slot into tuple that we can write. */ >>> + /* Materialize slot into a tuple that we can inspect. */ >>> tuple = ExecMaterializeSlot(slot); >>> >>> I think it is better to keep the last word of the sentence as "write" >>> instead of "inspect" as was in the original sentence. >> >> A copy-pasto while correcting a typo :) >> >>> It makes more >>> sense as we are materializing the tuple to write it. Similarly, in >>> the other change in the patch can use "write". >> >> Are you suggesting that we should use "write" in the modified comment >> instead of "inspect" in original comment. >> > > Yes. > > Another variant used in few other similar places is: > > /* > * get the heap tuple out of the tuple table slot, making sure we have a > * writable copy > */ > > >> Ok, I have now corrected grammar as well. Here's updated patch. >> > > Your new comment is also correct. I like the comment already used in > code as that makes the code consistent, what do you think? > I don't understand what do you mean by consistent. Do you mean to say that current usage " Store the slot into tuple ... " is correct? I don't think " so that we can write" is right usage either. I think, there should be a preposition after "write" something like "write to", to indicate that we will write to tuple. I find "scrible upon" to be better than "write to". But I am not wedded to any of those. However, I do want to change "store the slot into tuple" usage, which is wrong as explained earlier as well as in the commit message. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Wed, Jun 27, 2018 at 10:09 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Tue, Jun 26, 2018 at 8:43 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Tue, Jun 26, 2018 at 7:02 PM, Ashutosh Bapat >> <ashutosh.bapat@enterprisedb.com> wrote: >>> On Tue, Jun 26, 2018 at 6:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>>> On Tue, Jun 26, 2018 at 4:33 PM, Ashutosh Bapat >>>> <ashutosh.bapat@enterprisedb.com> wrote: >>>>> Looks like we need similar adjustment in ExecSimpleRelationUpdate() as >>>>> well. Updated the patch. >>>>> >>>> >>>> - /* Store the slot into tuple that we can write. */ >>>> + /* Materialize slot into a tuple that we can inspect. */ >>>> tuple = ExecMaterializeSlot(slot); >>>> >>>> I think it is better to keep the last word of the sentence as "write" >>>> instead of "inspect" as was in the original sentence. >>> >>> A copy-pasto while correcting a typo :) >>> >>>> It makes more >>>> sense as we are materializing the tuple to write it. Similarly, in >>>> the other change in the patch can use "write". >>> >>> Are you suggesting that we should use "write" in the modified comment >>> instead of "inspect" in original comment. >>> >> >> Yes. >> >> Another variant used in few other similar places is: >> >> /* >> * get the heap tuple out of the tuple table slot, making sure we have a >> * writable copy >> */ >> >> >>> Ok, I have now corrected grammar as well. Here's updated patch. >>> >> >> Your new comment is also correct. I like the comment already used in >> code as that makes the code consistent, what do you think? >> > > I don't understand what do you mean by consistent. Do you mean to say > that current usage " Store the slot into tuple ... " is correct? > Oh no, I was talking about replacing it with below comment which is used at other places in the code. /* * get the heap tuple out of the tuple table slot, making sure we have a * writable copy */ -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 27, 2018 at 11:24 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> I don't understand what do you mean by consistent. Do you mean to say >> that current usage " Store the slot into tuple ... " is correct? >> > > Oh no, I was talking about replacing it with below comment which is > used at other places in the code. > /* > * get the heap tuple out of the tuple table slot, making sure we have a > * writable copy > */ Probably yes. But I would just correct the usage and grammar. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Wed, Jun 27, 2018 at 11:30 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Wed, Jun 27, 2018 at 11:24 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> >>> I don't understand what do you mean by consistent. Do you mean to say >>> that current usage " Store the slot into tuple ... " is correct? >>> >> >> Oh no, I was talking about replacing it with below comment which is >> used at other places in the code. >> /* >> * get the heap tuple out of the tuple table slot, making sure we have a >> * writable copy >> */ > > Probably yes. But I would just correct the usage and grammar. > Okay, pushed! -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com