Re: Fix inconsistencies for v12

Hi Andres,

I have added you here as some of these are related to commits done by
you.  So need your opinion on the same.  I have mentioned where your
feedback will be helpful.

On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> 6. ExecGetResultSlot - remove (not used since introduction in 1a0586de)
>

Yeah, I also think this is not required.  Andres, this API is not
defined.  Is it intended for some purpose?

> 8. heap_parallelscan_initialize -> remove the sentence (changed in c2fe139c)
>

The same check has been moved to table_block_parallelscan_initialize.
So, I think instead of removing the sentence you need to change the
function name in the comment.

> 10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
> internal inconsistency)
>

  * This is an interface to HeapTupleSatisfiesVacuum that's callable via
- * HeapTupleSatisfiesSnapshot, so it can be used through a Snapshot.
+ * HeapTupleSatisfiesVisibility, so it can be used through a Snapshot.

I think now we don't need to write the second half of the comment ("so
it can be used through a Snapshot").  It makes more sense with
previous style API.

Another related point:
* HeapTupleSatisfiesNonVacuumable
 *
 * True if tuple might be visible to some
transaction; false if it's
 * surely dead to everyone, ie, vacuumable.
 *
 * See SNAPSHOT_TOAST's definition for the intended behaviour.

Here, I think instead of SNAPSHOT_TOAST, we should mention
SNAPSHOT_NON_VACUUMABLE.

Andres, do you have any comments on the proposed changes?

> 14. latestRemovedxids -> latestRemovedXids (an inconsistent case)

* Conjecture: if hitemid is dead then it had xids before the xids
  * marked on LP_NORMAL items. So we just ignore this item and move
  * onto the next, for the purposes of calculating
- * latestRemovedxids.
+ * latestRemovedXids.

I think it should be latestRemovedXid.

> 16. NextSampletuple -> NextSampleTuple (an inconsistent case)

I think this doesn't make much difference, but we can fix it so that
NextSampleTuple's occurrence can be found during grep.

> 20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
> 578b2297)

- * This is exported separately because there are cases where we want to use
- * an index that will not be recognized by RelationGetOidIndex: TOAST tables
- * have indexes that are usable, but have multiple columns and are on
- * ordinary columns rather than a true OID column.  This code will work
- * anyway, so long as the OID is the index's first column.  The caller must
- * pass in the actual heap attnum of the OID column, however.
- *

Instead of removing the entire paragraph, how about changing it like
"This also handles the special cases where TOAST tables have indexes
that are usable, but have multiple columns and are on ordinary columns
rather than a true OID column.  This code will work anyway, so long as
the OID is the index's first column.  The caller must
pass in the actual heap attnum of the OID column, however."

Andres, any suggestions?

> 27. XactTopTransactionId -> XactTopFullTransactionId (an internal
> inconsistency)
>

- * XactTopTransactionId stores the XID of our toplevel transaction, which
+ * XactTopFullTransactionId stores the XID of our toplevel transaction, which
  * will be the same as TopTransactionState.transactionId in an ordinary

I think in the above sentence, now we need to use
TopTransactionState.fullTransactionId.

Note that I agree with your changes for the points where I have not
responded anything.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com