Comment typo in tableam.h

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Comment typo in tableam.h

Antonin Houska-2
Please see the diff attached.

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


tableam_h_typo.diff (512 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

David Rowley-3
On Fri, 31 May 2019 at 05:02, Antonin Houska <[hidden email]> wrote:
> Please see the diff attached.

Pushed. Thanks.

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


Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Ashwin Agrawal
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.


v1-0001-Fix-typos-in-few-tableam-comments.patch (4K) Download Attachment
v1-0002-Add-assertions-for-required-table-am-callbacks.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Ashwin Agrawal

On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <[hidden email]> 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 :-)

Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Andres Freund
Hi,

On 2019-06-03 18:21:56 -0700, Ashwin Agrawal wrote:

> On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Ashwin Agrawal

On Mon, Jun 3, 2019 at 6:24 PM Andres Freund <[hidden email]> wrote:
Hi,

On 2019-06-03 18:21:56 -0700, Ashwin Agrawal wrote:
> On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Ashwin Agrawal
In reply to this post by Ashwin Agrawal

On Mon, Jun 3, 2019 at 5:24 PM Ashwin Agrawal <[hidden email]> 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 ?

Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

akapila
On Mon, Jun 24, 2019 at 11:26 PM Ashwin Agrawal <[hidden email]> wrote:

>
> On Mon, Jun 3, 2019 at 5:24 PM Ashwin Agrawal <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Ashwin Agrawal
On Fri, Jun 28, 2019 at 1:47 PM Amit Kapila <[hidden email]> wrote:
On Mon, Jun 24, 2019 at 11:26 PM Ashwin Agrawal <[hidden email]> wrote:
>
> On Mon, Jun 3, 2019 at 5:24 PM Ashwin Agrawal <[hidden email]> 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.


v2-0001-Fix-typos-in-few-tableam-comments.patch (3K) Download Attachment
v1-0002-Add-assertions-for-required-table-am-callbacks.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

akapila
On Tue, Jul 2, 2019 at 1:00 AM Ashwin Agrawal <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Ashwin Agrawal

On Sat, Jul 6, 2019 at 12:05 AM Amit Kapila <[hidden email]> wrote:
On Tue, Jul 2, 2019 at 1:00 AM Ashwin Agrawal <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

akapila
On Mon, Jul 8, 2019 at 10:21 PM Ashwin Agrawal <[hidden email]> wrote:

>
>
> On Sat, Jul 6, 2019 at 12:05 AM Amit Kapila <[hidden email]> wrote:
>>
>> On Tue, Jul 2, 2019 at 1:00 AM Ashwin Agrawal <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Brad DeJong-2
More typos in tableam.h along with a few grammar changes.

v1-more-typos-in-tableam.h.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Comment typo in tableam.h

Andres Freund
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