Thread: Comment typo in tableam.h
Please see the diff attached. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Attachment
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
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
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
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?
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
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.
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 ?
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
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.
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
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
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.
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
More typos in tableam.h along with a few grammar changes.
Attachment
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