What is an item pointer, anyway?
itemid.h introduces the struct ItemIdData as follows:
/*
* An item pointer (also called line pointer) on a buffer page
Meanwhile, itemptr.h introduces the struct ItemPointerData as follows:
/*
* ItemPointer:
*
* This is a pointer to an item within a disk page of a known file
* (for example, a cross-link from an index to its parent table).
It doesn't seem reasonable to assume that you should know the
difference based on context. The two concepts are closely related. An
ItemPointerData points to a block, as well as the, uh, item pointer
within that block.
This ambiguity is avoidable, and should be avoided. ISTM that the
least confusing way of removing the ambiguity would be to no longer
refer to ItemIds as item pointers, without changing anything else.
--
Peter Geoghegan
On Fri, Apr 26, 2019 at 2:19 PM Peter Geoghegan <pg@bowt.ie> wrote:
itemid.h introduces the struct ItemIdData as follows:
/*
* An item pointer (also called line pointer) on a buffer pageMeanwhile, itemptr.h introduces the struct ItemPointerData as follows:
/*
* ItemPointer:
*
* This is a pointer to an item within a disk page of a known file
* (for example, a cross-link from an index to its parent table).It doesn't seem reasonable to assume that you should know the
difference based on context. The two concepts are closely related. An
ItemPointerData points to a block, as well as the, uh, item pointer
within that block.This ambiguity is avoidable, and should be avoided.
Agree.
ISTM that the
least confusing way of removing the ambiguity would be to no longer
refer to ItemIds as item pointers, without changing anything else.
How about we rename ItemPointerData to TupleIdentifier or ItemIdentifier
instead and leave ItemPointer or Item confined to AM term, where item can
be tuple, datum or anything else ?
On Fri, Apr 26, 2019 at 4:23 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
How about we rename ItemPointerData to TupleIdentifier or ItemIdentifier instead and leave ItemPointer or Item confined to AM term, where item can be tuple, datum or anything else ?
I'm not a fan of that idea, because the reality is that an
ItemPointerData is quite explicitly supposed to be a physiological
identifier (TID) used by heapam, or a similar heap-like access method
such as zheap. This is baked into a number of things.
The limitation that pluggable storage engines have to work in terms of
item pointers is certainly a problem, especially for things like the
Zedstore column store project you're working on. However, I suspect
that that problem is best solved by accommodating other types of
identifiers that don't work like TIDs.
I understand why you've adopted ItemPointerData as a fully-logical
identifier in your prototype, but it's not a great long term solution.
--
Peter Geoghegan
Ashwin Agrawal <aagrawal@pivotal.io> writes:
On Fri, Apr 26, 2019 at 2:19 PM Peter Geoghegan <pg@bowt.ie> wrote:
ISTM that the
least confusing way of removing the ambiguity would be to no longer
refer to ItemIds as item pointers, without changing anything else.
How many places would we be changing to clean that up?
How about we rename ItemPointerData to TupleIdentifier or ItemIdentifier
instead and leave ItemPointer or Item confined to AM term, where item can
be tuple, datum or anything else ?
There's half a thousand references to ItemPointer[Data] in our
sources, and probably tons more in external modules. I'm *not*
in favor of renaming it.
ItemId[Data] is somewhat less widely referenced, but I'm still not
much in favor of renaming that type. I think fixing comments to
uniformly call it an item ID would be more reasonable. (We should
leave the "line pointer" terminology in place, too; if memory serves,
an awful lot of variables of the type are named "lp" or variants.
Renaming all of those is to nobody's benefit.)
regards, tom lane
On Fri, Apr 26, 2019 at 4:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
ItemId[Data] is somewhat less widely referenced, but I'm still not
much in favor of renaming that type. I think fixing comments to
uniformly call it an item ID would be more reasonable. (We should
leave the "line pointer" terminology in place, too; if memory serves,
an awful lot of variables of the type are named "lp" or variants.
Renaming all of those is to nobody's benefit.)
I was proposing that we not rename any struct at all, and continue to
call ItemId[Data]s "line pointers" only. This would involve removing
the comment in itemid.h that confusingly refers to line pointers as
"item pointers" (plus any other comments that fail to make a clear
distinction).
I think that the total number of comments that would be affected by
this approach is quite low.
--
Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
I was proposing that we not rename any struct at all, and continue to
call ItemId[Data]s "line pointers" only.
Yeah, I'd be fine with that, although the disconnect between the type
name and the comment terminology might confuse some people.
regards, tom lane
On Fri, Apr 26, 2019 at 5:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah, I'd be fine with that, although the disconnect between the type
name and the comment terminology might confuse some people.
Maybe, but the fact that the ItemIdData struct consists of bit fields
that are all named "lp_*" offers a hint. Plus you have the LP_*
constants that get stored in ItemIdData.lp_flags.
I wouldn't call the struct ItemIdData if I was in a green field
situation, but it doesn't seem too bad under the present
circumstances. I'd rather not change the struct's name, because that
would probably cause problems without any real benefit. OTOH, calling
two closely related but distinct things by the same name is atrocious.
--
Peter Geoghegan
On Fri, Apr 26, 2019 at 5:13 PM Peter Geoghegan <pg@bowt.ie> wrote:
On Fri, Apr 26, 2019 at 5:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah, I'd be fine with that, although the disconnect between the type
name and the comment terminology might confuse some people.Maybe, but the fact that the ItemIdData struct consists of bit fields
that are all named "lp_*" offers a hint. Plus you have the LP_*
constants that get stored in ItemIdData.lp_flags.
Attached draft patch adjusts code comments and error messages where a
line pointer is referred to as an item pointer. It turns out that this
practice isn't all that prevalent. Here are some specific concerns
that I had to think about when writing the patch, though:
* I ended up removing a big indexam.c "old comments" comment paragraph
from the Berkeley days, because it used the term item pointer in what
seemed like the wrong way, but also because AFAICT it's totally
obsolete.
* Someone should confirm that I have preserved the original intent of
the changes within README.HOT, and the heapam changes that relate to
pruning. It's possible that I changed "item pointer" to "line pointer"
in one or two places where I should have changed "item pointer" to
"tuple" instead.
* I changed a few long standing "can't happen" error messages that
concern corruption, most of which also relate to pruning. Maybe that's
a cost that needs to be considered.
* I changed a lazy_scan_heap() log message of long-standing. Another
downside that needs to be considered.
* I expanded a little on the advantages of using line pointers within
bufpage.h. That seemed in scope to me, because it drives home the
distinction between item pointers and line pointers.
--
Peter Geoghegan
Attachments:
v1-0001-Standardize-line-pointers-item-pointer-terminolog.patchapplication/octet-stream; name=v1-0001-Standardize-line-pointers-item-pointer-terminolog.patchDownload
From 5fd2a234fc63270bf7aeabc207b04c796ef0563a Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Thu, 2 May 2019 17:47:56 -0700
Subject: [PATCH v1] Standardize line pointers/item pointer terminology.
Consistently refer to ItemIdData structs as "line pointers", and
consistently refer to ItemPointerData structs as "item pointers".
---
contrib/amcheck/verify_nbtree.c | 4 ++--
doc/src/sgml/storage.sgml | 2 +-
src/backend/access/heap/README.HOT | 10 ++++-----
src/backend/access/heap/heapam.c | 4 ++--
src/backend/access/heap/heapam_handler.c | 2 +-
src/backend/access/heap/pruneheap.c | 10 ++++-----
src/backend/access/heap/vacuumlazy.c | 6 +++---
src/backend/access/index/indexam.c | 26 ------------------------
src/backend/access/nbtree/nbtinsert.c | 2 +-
src/backend/access/spgist/spgvacuum.c | 2 +-
src/backend/storage/page/bufpage.c | 26 ++++++++++++------------
src/include/access/htup_details.h | 2 +-
src/include/access/itup.h | 2 +-
src/include/storage/bufpage.h | 16 +++++++++------
src/include/storage/itemid.h | 13 ++++++------
15 files changed, 53 insertions(+), 74 deletions(-)
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 0424e0eb27..2576d4f9e9 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2164,7 +2164,7 @@ invariant_l_offset(BtreeCheckState *state, BTScanInsert key,
* Does the invariant hold that the key is less than or equal to a given upper
* bound offset item?
*
- * Caller should have verified that upperbound's item pointer is consistent
+ * Caller should have verified that upperbound's line pointer is consistent
* using PageGetItemIdCareful() call.
*
* If this function returns false, convention is that caller throws error due
@@ -2187,7 +2187,7 @@ invariant_leq_offset(BtreeCheckState *state, BTScanInsert key,
* Does the invariant hold that the key is strictly greater than a given lower
* bound offset item?
*
- * Caller should have verified that lowerbound's item pointer is consistent
+ * Caller should have verified that lowerbound's line pointer is consistent
* using PageGetItemIdCareful() call.
*
* If this function returns false, convention is that caller throws error due
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index e0915b6fa0..be61898fe9 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -766,7 +766,7 @@ free space pointers.</entry>
<row>
<entry>Free space</entry>
-<entry>The unallocated space. New item pointers are allocated from the start
+<entry>The unallocated space. New line pointers are allocated from the start
of this area, new items from the end.</entry>
</row>
diff --git a/src/backend/access/heap/README.HOT b/src/backend/access/heap/README.HOT
index 4cf3c3a0d4..68c6709aa8 100644
--- a/src/backend/access/heap/README.HOT
+++ b/src/backend/access/heap/README.HOT
@@ -149,8 +149,8 @@ the descendant heap-only tuple. It is conceivable that someone prunes
the heap-only tuple before that, and even conceivable that the line pointer
is re-used for another purpose. Therefore, when following a HOT chain,
it is always necessary to be prepared for the possibility that the
-linked-to item pointer is unused, dead, or redirected; and if it is a
-normal item pointer, we still have to check that XMIN of the tuple matches
+linked-to line pointer is unused, dead, or redirected; and if it is a
+normal line pointer, we still have to check that XMIN of the tuple matches
the XMAX of the tuple we left. Otherwise we should assume that we have
come to the end of the HOT chain. Note that this sort of XMIN/XMAX
matching is required when following ordinary update chains anyway.
@@ -171,14 +171,14 @@ bit: there can be at most one visible tuple in the chain, so we can stop
when we find it. This rule does not work for non-MVCC snapshots, though.)
Sequential scans do not need to pay attention to the HOT links because
-they scan every item pointer on the page anyway. The same goes for a
+they scan every line pointer on the page anyway. The same goes for a
bitmap heap scan with a lossy bitmap.
Pruning
-------
-HOT pruning means updating item pointers so that HOT chains are
+HOT pruning means updating line pointers so that HOT chains are
reduced in length, by collapsing out line pointers for intermediate dead
tuples. Although this makes those line pointers available for re-use,
it does not immediately make the space occupied by their tuples available.
@@ -271,7 +271,7 @@ physical tuple by eliminating an intermediate heap-only tuple or
replacing a physical root tuple by a redirect pointer, a decrement in
the table's number of dead tuples is reported to pgstats, which may
postpone autovacuuming. Note that we do not count replacing a root tuple
-by a DEAD item pointer as decrementing n_dead_tuples; we still want
+by a DEAD line pointer as decrementing n_dead_tuples; we still want
autovacuum to run to clean up the index entries and DEAD item.
This area probably needs further work ...
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 6f26ddac5f..d97cb4c642 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -7163,7 +7163,7 @@ log_heap_clean(Relation reln, Buffer buffer,
* arrays need not be stored too. Note that even if all three arrays are
* empty, we want to expose the buffer as a candidate for whole-page
* storage, since this record type implies a defragmentation operation
- * even if no item pointers changed state.
+ * even if no line pointers changed state.
*/
if (nredirected > 0)
XLogRegisterBufData(0, (char *) redirected,
@@ -7724,7 +7724,7 @@ heap_xlog_clean(XLogReaderState *record)
nunused = (end - nowunused);
Assert(nunused >= 0);
- /* Update all item pointers per the record, and repair fragmentation */
+ /* Update all line pointers per the record, and repair fragmentation */
heap_page_prune_execute(buffer,
redirected, nredirected,
nowdead, ndead,
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 4d179881f2..bc47856ad5 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2162,7 +2162,7 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
else
{
/*
- * Bitmap is lossy, so we must examine each item pointer on the page.
+ * Bitmap is lossy, so we must examine each line pointer on the page.
* But we can ignore HOT chains, since we'll check each tuple anyway.
*/
Page dp = (Page) BufferGetPage(buffer);
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index a3e51922d8..e59063dd11 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -324,7 +324,7 @@ heap_page_prune(Relation relation, Buffer buffer, TransactionId OldestXmin,
/*
- * Prune specified item pointer or a HOT chain originating at that item.
+ * Prune specified line pointer or a HOT chain originating at that item.
*
* If the item is an index-referenced tuple (i.e. not a heap-only tuple),
* the HOT chain is pruned by removing all DEAD tuples at the start of the HOT
@@ -454,7 +454,7 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum,
}
/*
- * Likewise, a dead item pointer can't be part of the chain. (We
+ * Likewise, a dead line pointer can't be part of the chain. (We
* already eliminated the case of dead root tuple outside this
* function.)
*/
@@ -630,7 +630,7 @@ heap_prune_record_prunable(PruneState *prstate, TransactionId xid)
prstate->new_prune_xid = xid;
}
-/* Record item pointer to be redirected */
+/* Record line pointer to be redirected */
static void
heap_prune_record_redirect(PruneState *prstate,
OffsetNumber offnum, OffsetNumber rdoffnum)
@@ -645,7 +645,7 @@ heap_prune_record_redirect(PruneState *prstate,
prstate->marked[rdoffnum] = true;
}
-/* Record item pointer to be marked dead */
+/* Record line pointer to be marked dead */
static void
heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum)
{
@@ -656,7 +656,7 @@ heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum)
prstate->marked[offnum] = true;
}
-/* Record item pointer to be marked unused */
+/* Record line pointer to be marked unused */
static void
heap_prune_record_unused(PruneState *prstate, OffsetNumber offnum)
{
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2aa729817c..a89fe4f851 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -507,7 +507,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
live_tuples, /* live tuples (reltuples estimate) */
tups_vacuumed, /* tuples cleaned up by vacuum */
nkeep, /* dead-but-not-removable tuples */
- nunused; /* unused item pointers */
+ nunused; /* unused line pointers */
IndexBulkDeleteResult **indstats;
int i;
PGRUsage ru0;
@@ -1015,7 +1015,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
ItemPointerSet(&(tuple.t_self), blkno, offnum);
/*
- * DEAD item pointers are to be vacuumed normally; but we don't
+ * DEAD line pointers are to be vacuumed normally; but we don't
* count them in tups_vacuumed, else we'd be double-counting (at
* least in the common case where heap_page_prune() just freed up
* a non-HOT tuple).
@@ -1481,7 +1481,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
appendStringInfo(&buf,
_("%.0f dead row versions cannot be removed yet, oldest xmin: %u\n"),
nkeep, OldestXmin);
- appendStringInfo(&buf, _("There were %.0f unused item pointers.\n"),
+ appendStringInfo(&buf, _("There were %.0f unused line pointers.\n"),
nunused);
appendStringInfo(&buf, ngettext("Skipped %u page due to buffer pins, ",
"Skipped %u pages due to buffer pins, ",
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index ae1c87ebad..0fc9139bad 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -38,32 +38,6 @@
* This file contains the index_ routines which used
* to be a scattered collection of stuff in access/genam.
*
- *
- * old comments
- * Scans are implemented as follows:
- *
- * `0' represents an invalid item pointer.
- * `-' represents an unknown item pointer.
- * `X' represents a known item pointers.
- * `+' represents known or invalid item pointers.
- * `*' represents any item pointers.
- *
- * State is represented by a triple of these symbols in the order of
- * previous, current, next. Note that the case of reverse scans works
- * identically.
- *
- * State Result
- * (1) + + - + 0 0 (if the next item pointer is invalid)
- * (2) + X - (otherwise)
- * (3) * 0 0 * 0 0 (no change)
- * (4) + X 0 X 0 0 (shift)
- * (5) * + X + X - (shift, add unknown)
- *
- * All other states cannot occur.
- *
- * Note: It would be possible to cache the status of the previous and
- * next item pointer using the flags.
- *
*-------------------------------------------------------------------------
*/
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index c6c952dd49..717d6ab6b9 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -1650,7 +1650,7 @@ _bt_split(Relation rel, BTScanInsert itup_key, Buffer buf, Buffer cbuf,
* Direct access to page is not good but faster - we should implement
* some new func in page API. Note we only store the tuples
* themselves, knowing that they were inserted in item-number order
- * and so the item pointers can be reconstructed. See comments for
+ * and so the line pointers can be reconstructed. See comments for
* _bt_restore_page().
*/
XLogRegisterBufData(1,
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index b9311ce595..fc85c6f940 100644
--- a/src/backend/access/spgist/spgvacuum.c
+++ b/src/backend/access/spgist/spgvacuum.c
@@ -337,7 +337,7 @@ vacuumLeafPage(spgBulkDeleteState *bds, Relation index, Buffer buffer,
InvalidBlockNumber, InvalidOffsetNumber);
/*
- * We implement the move step by swapping the item pointers of the source
+ * We implement the move step by swapping the line pointers of the source
* and target tuples, then replacing the newly-source tuples with
* placeholders. This is perhaps unduly friendly with the page data
* representation, but it's fast and doesn't risk page overflow when a
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 14bc61b8ad..0e6df515d5 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -65,7 +65,7 @@ PageInit(Page page, Size pageSize, Size specialSize)
* Check that the page header and checksum (if any) appear valid.
*
* This is called when a page has just been read in from disk. The idea is
- * to cheaply detect trashed pages before we go nuts following bogus item
+ * to cheaply detect trashed pages before we go nuts following bogus line
* pointers, testing invalid transaction identifiers, etc.
*
* It turns out to be necessary to allow zeroed pages here too. Even though
@@ -170,12 +170,12 @@ PageIsVerified(Page page, BlockNumber blkno)
* reason. A WARNING is issued indicating the reason for the refusal.
*
* offsetNumber must be either InvalidOffsetNumber to specify finding a
- * free item pointer, or a value between FirstOffsetNumber and one past
- * the last existing item, to specify using that particular item pointer.
+ * free line pointer, or a value between FirstOffsetNumber and one past
+ * the last existing item, to specify using that particular line pointer.
*
* If offsetNumber is valid and flag PAI_OVERWRITE is set, we just store
* the item at the specified offsetNumber, which must be either a
- * currently-unused item pointer, or one past the last existing item.
+ * currently-unused line pointer, or one past the last existing item.
*
* If offsetNumber is valid and flag PAI_OVERWRITE is not set, insert
* the item at the specified offsetNumber, moving existing items later
@@ -314,7 +314,7 @@ PageAddItemExtended(Page page,
memmove(itemId + 1, itemId,
(limit - offsetNumber) * sizeof(ItemIdData));
- /* set the item pointer */
+ /* set the line pointer */
ItemIdSetNormal(itemId, upper, size);
/*
@@ -529,7 +529,7 @@ PageRepairFragmentation(Page page)
itemidptr->itemoff >= (int) pd_special))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("corrupted item pointer: %u",
+ errmsg("corrupted line pointer: %u",
itemidptr->itemoff)));
itemidptr->alignedlen = MAXALIGN(ItemIdGetLength(lp));
totallen += itemidptr->alignedlen;
@@ -763,7 +763,7 @@ PageIndexTupleDelete(Page page, OffsetNumber offnum)
offset != MAXALIGN(offset))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("corrupted item pointer: offset = %u, size = %u",
+ errmsg("corrupted line pointer: offset = %u, size = %u",
offset, (unsigned int) size)));
/* Amount of space to actually be deleted */
@@ -881,7 +881,7 @@ PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems)
pd_lower, pd_upper, pd_special)));
/*
- * Scan the item pointer array and build a list of just the ones we are
+ * Scan the line pointer array and build a list of just the ones we are
* going to keep. Notice we do not modify the page yet, since we are
* still validity-checking.
*/
@@ -901,7 +901,7 @@ PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems)
offset != MAXALIGN(offset))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("corrupted item pointer: offset = %u, length = %u",
+ errmsg("corrupted line pointer: offset = %u, length = %u",
offset, (unsigned int) size)));
if (nextitm < nitems && offnum == itemnos[nextitm])
@@ -989,14 +989,14 @@ PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum)
offset != MAXALIGN(offset))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("corrupted item pointer: offset = %u, size = %u",
+ errmsg("corrupted line pointer: offset = %u, size = %u",
offset, (unsigned int) size)));
/* Amount of space to actually be deleted */
size = MAXALIGN(size);
/*
- * Either set the item pointer to "unused", or zap it if it's the last
+ * Either set the line pointer to "unused", or zap it if it's the last
* one. (Note: it's possible that the next-to-last one(s) are already
* unused, but we do not trouble to try to compact them out if so.)
*/
@@ -1054,7 +1054,7 @@ PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum)
* other tuples' data up or down as needed to keep the page compacted.
* This is better than deleting and reinserting the tuple, because it
* avoids any data shifting when the tuple size doesn't change; and
- * even when it does, we avoid moving the item pointers around.
+ * even when it does, we avoid moving the line pointers around.
* Conceivably this could also be of use to an index AM that cares about
* the physical order of tuples as well as their ItemId order.
*
@@ -1099,7 +1099,7 @@ PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
offset != MAXALIGN(offset))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("corrupted item pointer: offset = %u, size = %u",
+ errmsg("corrupted line pointer: offset = %u, size = %u",
offset, (unsigned int) oldsize)));
/*
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 6d51f9062b..8c9cc37482 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -564,7 +564,7 @@ do { \
* MaxHeapTuplesPerPage is an upper bound on the number of tuples that can
* fit on one heap page. (Note that indexes could have more, because they
* use a smaller tuple header.) We arrive at the divisor because each tuple
- * must be maxaligned, and it must have an associated item pointer.
+ * must be maxaligned, and it must have an associated line pointer.
*
* Note: with HOT, there could theoretically be more line pointers (not actual
* tuples) than this on a heap page. However we constrain the number of line
diff --git a/src/include/access/itup.h b/src/include/access/itup.h
index 2f90cc0142..8c3a57a8eb 100644
--- a/src/include/access/itup.h
+++ b/src/include/access/itup.h
@@ -131,7 +131,7 @@ typedef IndexAttributeBitMapData * IndexAttributeBitMap;
* fit on one index page. An index tuple must have either data or a null
* bitmap, so we can safely assume it's at least 1 byte bigger than a bare
* IndexTupleData struct. We arrive at the divisor because each tuple
- * must be maxaligned, and it must have an associated item pointer.
+ * must be maxaligned, and it must have an associated line pointer.
*
* To be index-type-independent, this does not account for any special space
* on the page, and is thus conservative.
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 85cd0abb97..3ba665f4ce 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -53,14 +53,18 @@
*
* NOTES:
*
- * linp1..N form an ItemId array. ItemPointers point into this array
- * rather than pointing directly to a tuple. Note that OffsetNumbers
+ * linp1..N form an ItemId (line pointer) array. ItemPointers point
+ * to a physical block number and a logical offset (line pointer
+ * number) within that block/page. Note that OffsetNumbers
* conventionally start at 1, not 0.
*
- * tuple1..N are added "backwards" on the page. because a tuple's
- * ItemPointer points to its ItemId entry rather than its actual
+ * tuple1..N are added "backwards" on the page. Since an ItemPointer
+ * offset is used to access an ItemId entry rather than an actual
* byte-offset position, tuples can be physically shuffled on a page
- * whenever the need arises.
+ * whenever the need arises. This indirection keeps crash recovery
+ * relatively simple, because the low-level details of page space
+ * management can be controlled by standard buffer page code during
+ * logging, and during recovery.
*
* AM-generic per-page information is kept in PageHeaderData.
*
@@ -233,7 +237,7 @@ typedef PageHeaderData *PageHeader;
/*
* PageGetContents
- * To be used in case the page does not contain item pointers.
+ * To be used in cases where the page does not contain line pointers.
*
* Note: prior to 8.3 this was not guaranteed to yield a MAXALIGN'd result.
* Now it is. Beware of old code that might think the offset to the contents
diff --git a/src/include/storage/itemid.h b/src/include/storage/itemid.h
index 30a5193132..197334a403 100644
--- a/src/include/storage/itemid.h
+++ b/src/include/storage/itemid.h
@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* itemid.h
- * Standard POSTGRES buffer page item identifier definitions.
+ * Standard POSTGRES buffer page item identifier/line pointer definitions.
*
*
* Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
@@ -15,16 +15,17 @@
#define ITEMID_H
/*
- * An item pointer (also called line pointer) on a buffer page
+ * A line pointer on a buffer page. See buffer page definitions and comments
+ * for an explanation of how line pointers are used.
*
- * In some cases an item pointer is "in use" but does not have any associated
- * storage on the page. By convention, lp_len == 0 in every item pointer
+ * In some cases a line pointer is "in use" but does not have any associated
+ * storage on the page. By convention, lp_len == 0 in every line pointer
* that does not have storage, independently of its lp_flags state.
*/
typedef struct ItemIdData
{
unsigned lp_off:15, /* offset to tuple (from start of page) */
- lp_flags:2, /* state of item pointer, see below */
+ lp_flags:2, /* state of line pointer, see below */
lp_len:15; /* byte length of tuple */
} ItemIdData;
@@ -72,7 +73,7 @@ typedef uint16 ItemLength;
/*
* ItemIdGetRedirect
- * In a REDIRECT pointer, lp_off holds the link to the next item pointer
+ * In a REDIRECT pointer, lp_off holds the link to the next line pointer
*/
#define ItemIdGetRedirect(itemId) \
((itemId)->lp_off)
--
2.17.1
On Sun, May 5, 2019 at 1:14 PM Peter Geoghegan <pg@bowt.ie> wrote:
Attached draft patch adjusts code comments and error messages where a
line pointer is referred to as an item pointer. It turns out that this
practice isn't all that prevalent. Here are some specific concerns
that I had to think about when writing the patch, though:
Ping? Any objections to pushing ahead with this?
--
Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
On Sun, May 5, 2019 at 1:14 PM Peter Geoghegan <pg@bowt.ie> wrote:
Attached draft patch adjusts code comments and error messages where a
line pointer is referred to as an item pointer. It turns out that this
practice isn't all that prevalent. Here are some specific concerns
that I had to think about when writing the patch, though:
Ping? Any objections to pushing ahead with this?
Patch looks fine to me. One minor quibble: in pruneheap.c you have
/*
- * Prune specified item pointer or a HOT chain originating at that item.
+ * Prune specified line pointer or a HOT chain originating at that item.
*
* If the item is an index-referenced tuple (i.e. not a heap-only tuple),Should "that item" also be re-worded, for consistency?
regards, tom lane
On Mon, May 13, 2019 at 12:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
/* - * Prune specified item pointer or a HOT chain originating at that item. + * Prune specified line pointer or a HOT chain originating at that item. * * If the item is an index-referenced tuple (i.e. not a heap-only tuple),Should "that item" also be re-worded, for consistency?
Yes, it should be -- I'll fix it.
I'm going to backpatch the storage.sgml change on its own, while
pushing everything else in a separate HEAD-only commit.
Thanks
--
Peter Geoghegan