On Tue, Jun 4, 2019 at 7:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
>
> > 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?
>
>
> > 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?
>
Leaving the changes related to the above two points, I have combined
all the changes and fixed the things as per my review in the attached
patch. Alexander, see if you can verify once the combined patch. I
am planning to commit the attached by tomorrow and then we can deal
with the remaining two. However, in the meantime, if Andres shared
his views on the above two points, then we can include the changes
corresponding to them as well.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com