Comment typo in tableam.h
Please see the diff attached.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
tableam_h_typo.difftext/x-diffDownload
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index bf3a472018..e3619b8e7e 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -1331,7 +1331,7 @@ table_finish_bulk_insert(Relation rel, int options)
*/
/*
- * Create storage for `rel` in `newrode`, with persistence set to
+ * Create storage for `rel` in `newrnode`, with persistence set to
* `persistence`.
*
* This is used both during relation creation and various DDL operations to
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.
Attachments:
v1-0001-Fix-typos-in-few-tableam-comments.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-typos-in-few-tableam-comments.patchDownload
From f32bdf5d0d3af5fd6ee6bf6430905f9c4bf5fefa Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagrawal@pivotal.io>
Date: Fri, 24 May 2019 16:30:38 -0700
Subject: [PATCH v1 1/2] Fix typos in few tableam comments.
---
src/backend/access/table/tableamapi.c | 2 +-
src/include/access/tableam.h | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 32877e7674f..0df5ba35803 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -91,7 +91,7 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->relation_estimate_size != NULL);
- /* optional, but one callback implies presence of hte other */
+ /* optional, but one callback implies presence of the other */
Assert((routine->scan_bitmap_next_block == NULL) ==
(routine->scan_bitmap_next_tuple == NULL));
Assert(routine->scan_sample_next_block != NULL);
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 0b6ac15d316..2d06d52d71f 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -231,7 +231,7 @@ typedef struct TableAmRoutine
/*
* 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);
@@ -434,8 +434,8 @@ typedef struct TableAmRoutine
*
* Note that only the subset of the relcache filled by
* RelationBuildLocalRelation() can be relied upon and that the relation's
- * catalog entries either will either not yet exist (new relation), or
- * will still reference the old relfilenode.
+ * catalog entries will either not yet exist (new relation), or will still
+ * reference the old relfilenode.
*
* As output *freezeXid, *minmulti must be set to the values appropriate
* for pg_class.{relfrozenxid, relminmxid}. For AMs that don't need those
@@ -591,7 +591,7 @@ typedef struct TableAmRoutine
* See table_relation_estimate_size().
*
* While block oriented, it shouldn't be too hard for an AM that doesn't
- * doesn't internally use blocks to convert into a usable representation.
+ * internally use blocks to convert into a usable representation.
*
* This differs from the relation_size callback by returning size
* estimates (both relation size and tuple count) for planning purposes,
@@ -967,7 +967,7 @@ table_index_fetch_end(struct IndexFetchTableData *scan)
*
* *all_dead, if all_dead is not NULL, will be set to true by
* table_index_fetch_tuple() iff it is guaranteed that no backend needs to see
- * that tuple. Index AMs can use that do avoid returning that tid in future
+ * that tuple. Index AMs can use that to avoid returning that tid in future
* searches.
*
* The difference between this function and table_fetch_row_version is that
@@ -1014,8 +1014,8 @@ extern bool table_index_fetch_tuple_check(Relation rel,
* true, false otherwise.
*
* 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.
*/
static inline bool
table_tuple_fetch_row_version(Relation rel,
--
2.19.1
v1-0002-Add-assertions-for-required-table-am-callbacks.patchtext/x-patch; charset=US-ASCII; name=v1-0002-Add-assertions-for-required-table-am-callbacks.patchDownload
From 022569d249918da60d145d7877dc0f8df4ccc6cd Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagrawal@pivotal.io>
Date: Mon, 3 Jun 2019 17:07:05 -0700
Subject: [PATCH v1 2/2] Add assertions for required table am callbacks.
---
src/backend/access/table/tableamapi.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 0df5ba35803..55bd1eea3db 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -50,6 +50,7 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->scan_begin != NULL);
Assert(routine->scan_end != NULL);
Assert(routine->scan_rescan != NULL);
+ Assert(routine->scan_getnextslot != NULL);
Assert(routine->parallelscan_estimate != NULL);
Assert(routine->parallelscan_initialize != NULL);
@@ -61,8 +62,12 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->index_fetch_tuple != NULL);
Assert(routine->tuple_fetch_row_version != NULL);
+ Assert(routine->tuple_tid_valid != NULL);
+ Assert(routine->tuple_get_latest_tid != NULL);
Assert(routine->tuple_satisfies_snapshot != NULL);
+ Assert(routine->compute_xid_horizon_for_tuples != NULL);
+
Assert(routine->tuple_insert != NULL);
/*
@@ -88,6 +93,7 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->index_validate_scan != NULL);
Assert(routine->relation_size != NULL);
+ Assert(routine->relation_needs_toast_table != NULL);
Assert(routine->relation_estimate_size != NULL);
--
2.19.1
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 scanof 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 scanof 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 parallelscan
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.
Attachments:
v2-0001-Fix-typos-in-few-tableam-comments.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-typos-in-few-tableam-comments.patchDownload
From 5c75807a56101a07685ed1f435eabcc43cd22fb7 Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagrawal@pivotal.io>
Date: Fri, 24 May 2019 16:30:38 -0700
Subject: [PATCH v2 1/2] Fix typos in few tableam comments.
---
src/include/access/tableam.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index fd07773b74f..1e45908c78a 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -434,8 +434,8 @@ typedef struct TableAmRoutine
*
* Note that only the subset of the relcache filled by
* RelationBuildLocalRelation() can be relied upon and that the relation's
- * catalog entries either will either not yet exist (new relation), or
- * will still reference the old relfilenode.
+ * catalog entries will either not yet exist (new relation), or will still
+ * reference the old relfilenode.
*
* As output *freezeXid, *minmulti must be set to the values appropriate
* for pg_class.{relfrozenxid, relminmxid}. For AMs that don't need those
@@ -591,7 +591,7 @@ typedef struct TableAmRoutine
* See table_relation_estimate_size().
*
* While block oriented, it shouldn't be too hard for an AM that doesn't
- * doesn't internally use blocks to convert into a usable representation.
+ * internally use blocks to convert into a usable representation.
*
* This differs from the relation_size callback by returning size
* estimates (both relation size and tuple count) for planning purposes,
@@ -967,7 +967,7 @@ table_index_fetch_end(struct IndexFetchTableData *scan)
*
* *all_dead, if all_dead is not NULL, will be set to true by
* table_index_fetch_tuple() iff it is guaranteed that no backend needs to see
- * that tuple. Index AMs can use that do avoid returning that tid in future
+ * that tuple. Index AMs can use that to avoid returning that tid in future
* searches.
*
* The difference between this function and table_fetch_row_version is that
@@ -1014,8 +1014,8 @@ extern bool table_index_fetch_tuple_check(Relation rel,
* true, false otherwise.
*
* 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.
*/
static inline bool
table_tuple_fetch_row_version(Relation rel,
--
2.19.1
v1-0002-Add-assertions-for-required-table-am-callbacks.patchtext/x-patch; charset=US-ASCII; name=v1-0002-Add-assertions-for-required-table-am-callbacks.patchDownload
From 4ed947590f2f61182356a7fa4bc429c679e7619f Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagrawal@pivotal.io>
Date: Mon, 3 Jun 2019 17:07:05 -0700
Subject: [PATCH v2 2/2] Add assertions for required table am callbacks.
---
src/backend/access/table/tableamapi.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index b2f58768107..98b8ea559d8 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -51,6 +51,7 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->scan_begin != NULL);
Assert(routine->scan_end != NULL);
Assert(routine->scan_rescan != NULL);
+ Assert(routine->scan_getnextslot != NULL);
Assert(routine->parallelscan_estimate != NULL);
Assert(routine->parallelscan_initialize != NULL);
@@ -62,8 +63,12 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->index_fetch_tuple != NULL);
Assert(routine->tuple_fetch_row_version != NULL);
+ Assert(routine->tuple_tid_valid != NULL);
+ Assert(routine->tuple_get_latest_tid != NULL);
Assert(routine->tuple_satisfies_snapshot != NULL);
+ Assert(routine->compute_xid_horizon_for_tuples != NULL);
+
Assert(routine->tuple_insert != NULL);
/*
@@ -89,6 +94,7 @@ GetTableAmRoutine(Oid amhandler)
Assert(routine->index_validate_scan != NULL);
Assert(routine->relation_size != NULL);
+ Assert(routine->relation_needs_toast_table != NULL);
Assert(routine->relation_estimate_size != NULL);
--
2.19.1
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.
Attachments:
v1-more-typos-in-tableam.h.patchapplication/octet-stream; name=v1-more-typos-in-tableam.h.patchDownload
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c2b0481e7e..7f81703b78 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -294,7 +294,7 @@ typedef struct TableAmRoutine
*
* *all_dead, if all_dead is not NULL, should be set to true by
* index_fetch_tuple iff it is guaranteed that no backend needs to see
- * that tuple. Index AMs can use that do avoid returning that tid in
+ * that tuple. Index AMs can use that to avoid returning that tid in
* future searches.
*/
bool (*index_fetch_tuple) (struct IndexFetchTableData *scan,
@@ -482,9 +482,9 @@ typedef struct TableAmRoutine
double *tups_recently_dead);
/*
- * React to VACUUM command on the relation. The VACUUM might be user
- * triggered or by autovacuum. The specific actions performed by the AM
- * will depend heavily on the individual AM.
+ * React to VACUUM command on the relation. The VACUUM can be
+ * triggered by a user or by autovacuum. The specific actions
+ * performed by the AM will depend heavily on the individual AM.
*
* On entry a transaction is already established, and the relation is
* locked with a ShareUpdateExclusive lock.
@@ -661,7 +661,7 @@ typedef struct TableAmRoutine
* false if the sample scan is finished, true otherwise. `scan` was
* started via table_beginscan_sampling().
*
- * Typically this will first determine the target block by call the
+ * Typically this will first determine the target block by calling the
* TsmRoutine's NextSampleBlock() callback if not NULL, or alternatively
* perform a sequential scan over all blocks. The determined block is
* then typically read and pinned.
@@ -679,7 +679,7 @@ typedef struct TableAmRoutine
*
* Currently it is required to implement this interface, as there's no
* alternative way (contrary e.g. to bitmap scans) to implement sample
- * scans. If infeasible to implement the AM may raise an error.
+ * scans. If infeasible to implement, the AM may raise an error.
*/
bool (*scan_sample_next_block) (TableScanDesc scan,
struct SampleScanState *scanstate);
@@ -1084,9 +1084,8 @@ table_compute_xid_horizon_for_tuples(Relation rel,
/*
* Insert a tuple from a slot into table AM routine.
*
- * The options bitmask allows to specify options that allow to change the
- * behaviour of the AM. Several options might be ignored by AMs not supporting
- * them.
+ * The options bitmask allows the caller to specify options that may change the
+ * behaviour of the AM. The AM will ignore options that it does not support.
*
* If the TABLE_INSERT_SKIP_WAL option is specified, the new tuple doesn't
* need to be logged to WAL, even for a non-temp relation. It is the AMs
@@ -1094,8 +1093,9 @@ table_compute_xid_horizon_for_tuples(Relation rel,
*
* If the TABLE_INSERT_SKIP_FSM option is specified, AMs are free to not reuse
* free space in the relation. This can save some cycles when we know the
- * relation is new and doesn't contain useful amounts of free space. It's
- * commonly passed directly to RelationGetBufferForTuple, see for more info.
+ * relation is new and doesn't contain useful amounts of free space.
+ * TABLE_INSERT_SKIP_FSM is commonly passed directly to
+ * RelationGetBufferForTuple. See that method for more information.
*
* TABLE_INSERT_FROZEN should only be specified for inserts into
* relfilenodes created during the current subtransaction and when
@@ -1111,7 +1111,6 @@ table_compute_xid_horizon_for_tuples(Relation rel,
* Note that most of these options will be applied when inserting into the
* heap's TOAST table, too, if the tuple requires any out-of-line data.
*
- *
* The BulkInsertState object (if any; bistate can be NULL for default
* behavior) is also just passed through to RelationGetBufferForTuple. If
* `bistate` is provided, table_finish_bulk_insert() needs to be called.
@@ -1383,13 +1382,13 @@ table_relation_copy_data(Relation rel, const RelFileNode *newrnode)
* Additional Input parameters:
* - use_sort - if true, the table contents are sorted appropriate for
* `OldIndex`; if false and OldIndex is not InvalidOid, the data is copied
- * in that index's order; if false and OidIndex is InvalidOid, no sorting is
+ * in that index's order; if false and OldIndex is InvalidOid, no sorting is
* performed
- * - OidIndex - see use_sort
+ * - OldIndex - see use_sort
* - OldestXmin - computed by vacuum_set_xid_limits(), even when
* not needed for the relation's AM
- * - *xid_cutoff - dito
- * - *multi_cutoff - dito
+ * - *xid_cutoff - ditto
+ * - *multi_cutoff - ditto
*
* Output parameters:
* - *xid_cutoff - rel's new relfrozenxid value, may be invalid
@@ -1416,10 +1415,10 @@ table_relation_copy_for_cluster(Relation OldTable, Relation NewTable,
}
/*
- * Perform VACUUM on the relation. The VACUUM can be user-triggered or by
+ * Perform VACUUM on the relation. The VACUUM can be triggered by a user or by
* autovacuum. The specific actions performed by the AM will depend heavily on
* the individual AM.
-
+ *
* On entry a transaction needs to already been established, and the
* table is locked with a ShareUpdateExclusive lock.
*