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