Thread: Comment typo in tableam.h

Comment typo in tableam.h

From
Antonin Houska
Date:
Please see the diff attached.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com


Attachment

Re: Comment typo in tableam.h

From
David Rowley
Date:
On Fri, 31 May 2019 at 05:02, Antonin Houska <ah@cybertec.at> wrote:
> Please see the diff attached.

Pushed. Thanks.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services



Re: Comment typo in tableam.h

From
Ashwin Agrawal
Date:
There were few more minor typos I had collected for table am, passing them along here.

Some of the required callback functions are missing Assert checking (minor thing), adding them in separate patch.

Attachment

Re: Comment typo in tableam.h

From
Andres Freund
Date:
Hi,

Thanks for these!

On 2019-06-03 17:24:15 -0700, Ashwin Agrawal wrote:
>      /*
>       * Estimate the size of shared memory needed for a parallel scan of this
> -     * relation. The snapshot does not need to be accounted for.
> +     * relation.
>       */
>      Size        (*parallelscan_estimate) (Relation rel);

That's not a typo?

Greetings,

Andres Freund



Re: Comment typo in tableam.h

From
Ashwin Agrawal
Date:

On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <andres@anarazel.de> wrote:
Hi,

Thanks for these!

On 2019-06-03 17:24:15 -0700, Ashwin Agrawal wrote:
>       /*
>        * Estimate the size of shared memory needed for a parallel scan of this
> -      * relation. The snapshot does not need to be accounted for.
> +      * relation.
>        */
>       Size            (*parallelscan_estimate) (Relation rel);

That's not a typo?

The snapshot is not passed as argument to that function hence seems weird to refer to snapshot in the comment, as anyways callback function can't account for it. Seems stale piece of comment and hence that piece of text should be removed. I should have refereed to changes as general comment fixes instead of explicit typo fixes :-)

Re: Comment typo in tableam.h

From
Andres Freund
Date:
Hi,

On 2019-06-03 18:21:56 -0700, Ashwin Agrawal wrote:
> On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <andres@anarazel.de> wrote:
> 
> > Hi,
> >
> > Thanks for these!
> >
> > On 2019-06-03 17:24:15 -0700, Ashwin Agrawal wrote:
> > >       /*
> > >        * Estimate the size of shared memory needed for a parallel scan
> > of this
> > > -      * relation. The snapshot does not need to be accounted for.
> > > +      * relation.
> > >        */
> > >       Size            (*parallelscan_estimate) (Relation rel);
> >
> > That's not a typo?
> >
> 
> The snapshot is not passed as argument to that function hence seems weird
> to refer to snapshot in the comment, as anyways callback function can't
> account for it.

It's part of the parallel scan struct, and it used to be accounted for
by pre tableam function...

Greetings,

Andres Freund



Re: Comment typo in tableam.h

From
Ashwin Agrawal
Date:

On Mon, Jun 3, 2019 at 6:24 PM Andres Freund <andres@anarazel.de> wrote:
Hi,

On 2019-06-03 18:21:56 -0700, Ashwin Agrawal wrote:
> On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <andres@anarazel.de> wrote:
>
> > Hi,
> >
> > Thanks for these!
> >
> > On 2019-06-03 17:24:15 -0700, Ashwin Agrawal wrote:
> > >       /*
> > >        * Estimate the size of shared memory needed for a parallel scan
> > of this
> > > -      * relation. The snapshot does not need to be accounted for.
> > > +      * relation.
> > >        */
> > >       Size            (*parallelscan_estimate) (Relation rel);
> >
> > That's not a typo?
> >
>
> The snapshot is not passed as argument to that function hence seems weird
> to refer to snapshot in the comment, as anyways callback function can't
> account for it.

It's part of the parallel scan struct, and it used to be accounted for
by pre tableam function...

Reads like the comment written from past context then, and doesn't have much value now. Its confusing than helping, to state not to account for snapshot and not any other field.
table_parallelscan_estimate() has snapshot argument and it accounts for it, but callback doesn't. I am not sure how a callback would explicitly use that comment and avoid accounting for snapshot if its using generic ParallelTableScanDescData. But if you feel is helpful, please feel free to keep that text.

Re: Comment typo in tableam.h

From
Ashwin Agrawal
Date:

On Mon, Jun 3, 2019 at 5:24 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
There were few more minor typos I had collected for table am, passing them along here.

Some of the required callback functions are missing Assert checking (minor thing), adding them in separate patch.

Curious to know if need to register such small typo fixing and assertion adding patchs to commit-fest as well ?

Re: Comment typo in tableam.h

From
Amit Kapila
Date:
On Mon, Jun 24, 2019 at 11:26 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>
> On Mon, Jun 3, 2019 at 5:24 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>>
>> There were few more minor typos I had collected for table am, passing them along here.
>>
>> Some of the required callback functions are missing Assert checking (minor thing), adding them in separate patch.
>
>
> Curious to know if need to register such small typo fixing and assertion adding patchs to commit-fest as well ?
>

Normally, such things are handled out of CF, but to avoid forgetting,
we can register it.  However, this particular item suits more to 'Open
Items'[1].  You can remove the objectionable part of the comment,
other things in your patch look good to me.  If nobody else picks it
up, I will take care of it.

[1] - https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com



Re: Comment typo in tableam.h

From
Ashwin Agrawal
Date:
On Fri, Jun 28, 2019 at 1:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jun 24, 2019 at 11:26 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>
> On Mon, Jun 3, 2019 at 5:24 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>>
>> There were few more minor typos I had collected for table am, passing them along here.
>>
>> Some of the required callback functions are missing Assert checking (minor thing), adding them in separate patch.
>
>
> Curious to know if need to register such small typo fixing and assertion adding patchs to commit-fest as well ?
>

Normally, such things are handled out of CF, but to avoid forgetting,
we can register it.  However, this particular item suits more to 'Open
Items'[1].  You can remove the objectionable part of the comment,
other things in your patch look good to me.  If nobody else picks it
up, I will take care of it.

Thank you, I thought Committer would remove the objectionable part of comment change and commit the patch if seems fine. I don't mind changing, just feel adds extra back and forth cycle.

Please find attached v2 of patch 1 without objectionable comment change. v1 of patch 2 attaching here just for convenience, no modifications made to it.

Attachment

Re: Comment typo in tableam.h

From
Amit Kapila
Date:
On Tue, Jul 2, 2019 at 1:00 AM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> Please find attached v2 of patch 1 without objectionable comment change. v1 of patch 2 attaching here just for
convenience,no modifications made to it.
 
>

0001*
  * See table_index_fetch_tuple's comment about what the difference between
- * these functions is. This function is the correct to use outside of
- * index entry->table tuple lookups.
+ * these functions is. This function is correct to use outside of index
+ * entry->table tuple lookups.

How about if we write the last line of comment as "It is correct to
use this function outside of index entry->table tuple lookups."?  I am
not an expert on this matter, but I find the way I am suggesting
easier to read.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com



Re: Comment typo in tableam.h

From
Ashwin Agrawal
Date:

On Sat, Jul 6, 2019 at 12:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Jul 2, 2019 at 1:00 AM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> Please find attached v2 of patch 1 without objectionable comment change. v1 of patch 2 attaching here just for convenience, no modifications made to it.
>

0001*
  * See table_index_fetch_tuple's comment about what the difference between
- * these functions is. This function is the correct to use outside of
- * index entry->table tuple lookups.
+ * these functions is. This function is correct to use outside of index
+ * entry->table tuple lookups.

How about if we write the last line of comment as "It is correct to
use this function outside of index entry->table tuple lookups."?  I am
not an expert on this matter, but I find the way I am suggesting
easier to read.

I am fine with the way you have suggested.

Re: Comment typo in tableam.h

From
Amit Kapila
Date:
On Mon, Jul 8, 2019 at 10:21 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>
>
> On Sat, Jul 6, 2019 at 12:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Tue, Jul 2, 2019 at 1:00 AM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
>> > Please find attached v2 of patch 1 without objectionable comment change. v1 of patch 2 attaching here just for
convenience,no modifications made to it.
 
>> >
>>
>> 0001*
>>   * See table_index_fetch_tuple's comment about what the difference between
>> - * these functions is. This function is the correct to use outside of
>> - * index entry->table tuple lookups.
>> + * these functions is. This function is correct to use outside of index
>> + * entry->table tuple lookups.
>>
>> How about if we write the last line of comment as "It is correct to
>> use this function outside of index entry->table tuple lookups."?  I am
>> not an expert on this matter, but I find the way I am suggesting
>> easier to read.
>
>
> I am fine with the way you have suggested.
>

Pushed.  I have already pushed your other patch a few days back.  So,
as per my knowledge, we are done here.  Do, let me know if anything
proposed in this thread is pending?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com



Re: Comment typo in tableam.h

From
Brad DeJong
Date:
More typos in tableam.h along with a few grammar changes.
Attachment

Re: Comment typo in tableam.h

From
Andres Freund
Date:
Hi,

On 2019-07-11 20:44:02 -0500, Brad DeJong wrote:
> More typos in tableam.h along with a few grammar changes.

Thanks! Applied.

Greetings,

Andres Freund