Re: Add 64-bit XIDs into PostgreSQL 15 - Mailing list pgsql-hackers

On Sun, Sep 4, 2022 at 9:53 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jul 18, 2022 at 2:54 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> >>
> >> > I can agree with you that sending rebased patches too often can be a little annoying. On the other hand,
otherwise,it's just red in Cfbot. I suppose it's much easier and more comfortable to review the patches that at least
applycleanly and pass all tests. So if Cfbot is red for a long time I feel we need to send a rebased patchset anyway. 
> >> >
> >> > I'll try to not doing this too often but frankly, I don't see a better alternative at the moment.
> >>
> >> Considering the overall activity on the mailing list personally I
> >> don't see a problem here. Several extra emails don't bother me at all,
> >> but I would like to see a green cfbot report for an open item in the
> >> CF application. Otherwise someone will complain that the patch doesn't
> >> apply anymore and the result will be the same as for sending an
> >> updated patch, except that we will receive at least two emails instead
> >> of one.
> >
> > Hi, Alexander!
> > Agree with you. I also consider green cfbot entry important. So PFA rebased v43.
>
> Since we have converted TransactionId to 64-bit, so do we still need
> the concept of FullTransactionId?  I mean it is really confusing to
> have 3 forms of transaction ids.  i.e. Transaction Id,
> FullTransactionId and ShortTransactionId.

I have done some more reviews to understand the idea.  I think this
patch needs far more comments to make it completely readable.

1.
 typedef struct HeapTupleData
 {
+ TransactionId t_xmin; /* base value for normal transaction ids */
+ TransactionId t_xmax; /* base value for mutlixact */

I think the field name and comments are not in sync, field says xmin
and xmax whereas the comment says base value for
transaction id and multi-xact.

2.
extern bool heap_page_prepare_for_xid(Relation relation, Buffer buffer,
  TransactionId xid, bool multi);

I noticed that this function is returning bool but all the callers are
ignoring the return type.

3.
+static int
+heap_page_try_prepare_for_xid(Relation relation, Buffer buffer, Page page,
+   TransactionId xid, bool multi, bool is_toast)
+{
+ TransactionId base;
+ ShortTransactionId min = InvalidTransactionId,

add function header comments.

4.

+ if (!multi)
+ {
+ Assert(!is_toast || !(htup->t_infomask & HEAP_XMAX_IS_MULTI));
+
+ if (TransactionIdIsNormal(htup->t_choice.t_heap.t_xmin) &&
+ !HeapTupleHeaderXminFrozen(htup))
+ {
+ xid_min_max(min, max, htup->t_choice.t_heap.t_xmin, &found);
+ }
+
+ if (htup->t_infomask & HEAP_XMAX_INVALID)
+ continue;
+
+ if ((htup->t_infomask & HEAP_XMAX_IS_MULTI) &&
+ (!(htup->t_infomask & HEAP_XMAX_LOCK_ONLY)))
+ {
+ TransactionId update_xid;
+ ShortTransactionId xid;
+
+ Assert(!is_toast);
+ update_xid = MultiXactIdGetUpdateXid(HeapTupleHeaderGetRawXmax(page, htup),
+ htup->t_infomask);
+ xid = NormalTransactionIdToShort(HeapPageGetSpecial(page)->pd_xid_base,
+ update_xid);
+
+ xid_min_max(min, max, xid, &found);
+ }
+ }

Why no handling for multi?  And this function has absolutely no
comments to understand the reason for this.

5.
+ if (IsToastRelation(relation))
+ {
+ PageInit(page, BufferGetPageSize(buffer), sizeof(ToastPageSpecialData));
+ ToastPageGetSpecial(page)->pd_xid_base = RecentXmin -
FirstNormalTransactionId;
+ }
+ else
+ {
+ PageInit(page, BufferGetPageSize(buffer), sizeof(HeapPageSpecialData));
+ HeapPageGetSpecial(page)->pd_xid_base = RecentXmin - FirstNormalTransactionId;
+ }

Why pd_xid_base can not be just RecentXmin?  Please explain in the
comments above.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com