Comment typo in tableam.h

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.

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?

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

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]https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items. 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.

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.

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.

I am fine with the way you have suggested.

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.

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