Horton

Thread

Commits

  1. tableam: comment improvements.

  2. Fix few typos and minor word smithing in tableam comments.

  3. Fix few typos and minor wordsmithing in tableam comments.

  4. Add missing assertions for required table am callbacks.

  5. Fix incorrect parameter name in comment

  1. Comment typo in tableam.h

    Antonin Houska <ah@cybertec.at> — 2019-05-31T09:02:37Z

    Please see the diff attached.

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

  2. Re: Comment typo in tableam.h

    David Rowley <david.rowley@2ndquadrant.com> — 2019-05-31T17:34:17Z

    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

  3. Re: Comment typo in tableam.h

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-06-04T00:24:15Z

    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.

  4. Re: Comment typo in tableam.h

    Andres Freund <andres@anarazel.de> — 2019-06-04T00:26:39Z

    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

  5. Re: Comment typo in tableam.h

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-06-04T01:21:56Z

    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 :-)

  6. Re: Comment typo in tableam.h

    Andres Freund <andres@anarazel.de> — 2019-06-04T01:24:15Z

    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

  7. Re: Comment typo in tableam.h

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-06-04T01:41:35Z

    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.

  8. Re: Comment typo in tableam.h

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-06-24T17:55:43Z

    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 ?

  9. Re: Comment typo in tableam.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-06-28T20:47:37Z

    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

  10. Re: Comment typo in tableam.h

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-07-01T19:30:04Z

    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.

  11. Re: Comment typo in tableam.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-07-06T07:05:06Z

    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

  12. Re: Comment typo in tableam.h

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-07-08T16:51:34Z

    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.

  13. Re: Comment typo in tableam.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-07-10T11:50:26Z

    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

  14. Re: Comment typo in tableam.h

    Brad DeJong <bpd0018@gmail.com> — 2019-07-12T01:44:02Z

    More typos in tableam.h along with a few grammar changes.

  15. Re: Comment typo in tableam.h

    Andres Freund <andres@anarazel.de> — 2019-07-18T02:52:10Z

    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