Thread: Typo about subxip in comments
Hi, hackers
Recently, when I read the XidInMVCCSnapshot(), and find there are some
typos in the comments.
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 207c4b27fd..9e8b6756fe 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2409,7 +2409,7 @@ GetSnapshotData(Snapshot snapshot)
* We could try to store xids into xip[] first and then into subxip[]
* if there are too many xids. That only works if the snapshot doesn't
* overflow because we do not search subxip[] in that case. A simpler
- * way is to just store all xids in the subxact array because this is
+ * way is to just store all xids in the subxip array because this is
* by far the bigger array. We just leave the xip array empty.
*
* Either way we need to change the way XidInMVCCSnapshot() works
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index f1f2ddac17..2524b1c585 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -2345,7 +2345,7 @@ XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot)
else
{
/*
- * In recovery we store all xids in the subxact array because it is by
+ * In recovery we store all xids in the subxip array because it is by
* far the bigger array, and we mostly don't know which xids are
* top-level and which are subxacts. The xip array is empty.
*
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Fri, Nov 11, 2022 at 8:56 AM Japin Li <japinli@hotmail.com> wrote:
>
> Recently, when I read the XidInMVCCSnapshot(), and find there are some
> typos in the comments.
>
> diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
> index 207c4b27fd..9e8b6756fe 100644
> --- a/src/backend/storage/ipc/procarray.c
> +++ b/src/backend/storage/ipc/procarray.c
> @@ -2409,7 +2409,7 @@ GetSnapshotData(Snapshot snapshot)
> * We could try to store xids into xip[] first and then into subxip[]
> * if there are too many xids. That only works if the snapshot doesn't
> * overflow because we do not search subxip[] in that case. A simpler
> - * way is to just store all xids in the subxact array because this is
> + * way is to just store all xids in the subxip array because this is
> * by far the bigger array. We just leave the xip array empty.
> *
> * Either way we need to change the way XidInMVCCSnapshot() works
> diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
> index f1f2ddac17..2524b1c585 100644
> --- a/src/backend/utils/time/snapmgr.c
> +++ b/src/backend/utils/time/snapmgr.c
> @@ -2345,7 +2345,7 @@ XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot)
> else
> {
> /*
> - * In recovery we store all xids in the subxact array because it is by
> + * In recovery we store all xids in the subxip array because it is by
> * far the bigger array, and we mostly don't know which xids are
> * top-level and which are subxacts. The xip array is empty.
> *
>
LGTM.
--
With Regards,
Amit Kapila.
On Fri, Nov 11, 2022 at 10:39:06AM +0530, Amit Kapila wrote:
> On Fri, Nov 11, 2022 at 8:56 AM Japin Li <japinli@hotmail.com> wrote:
> >
> > Recently, when I read the XidInMVCCSnapshot(), and find there are some
> > typos in the comments.
> >
> > diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
> > index 207c4b27fd..9e8b6756fe 100644
> > --- a/src/backend/storage/ipc/procarray.c
> > +++ b/src/backend/storage/ipc/procarray.c
> > @@ -2409,7 +2409,7 @@ GetSnapshotData(Snapshot snapshot)
> > * We could try to store xids into xip[] first and then into subxip[]
> > * if there are too many xids. That only works if the snapshot doesn't
> > * overflow because we do not search subxip[] in that case. A simpler
> > - * way is to just store all xids in the subxact array because this is
> > + * way is to just store all xids in the subxip array because this is
> > * by far the bigger array. We just leave the xip array empty.
> > *
> > * Either way we need to change the way XidInMVCCSnapshot() works
> > diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
> > index f1f2ddac17..2524b1c585 100644
> > --- a/src/backend/utils/time/snapmgr.c
> > +++ b/src/backend/utils/time/snapmgr.c
> > @@ -2345,7 +2345,7 @@ XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot)
> > else
> > {
> > /*
> > - * In recovery we store all xids in the subxact array because it is by
> > + * In recovery we store all xids in the subxip array because it is by
> > * far the bigger array, and we mostly don't know which xids are
> > * top-level and which are subxacts. The xip array is empty.
> > *
> >
>
> LGTM.
+1
On Fri, Nov 11, 2022 at 11:26 AM Japin Li <japinli@hotmail.com> wrote:
Recently, when I read the XidInMVCCSnapshot(), and find there are some
typos in the comments.
Hmm, it seems to me 'the subxact array' is just another saying to refer
to snapshot->subxip. I'm not sure about this being typo. But I have no
objection to this change, as it is more consistent with the 'xip array'
saying followed.
Thanks
Richard
to snapshot->subxip. I'm not sure about this being typo. But I have no
objection to this change, as it is more consistent with the 'xip array'
saying followed.
Thanks
Richard
On Fri, Nov 11, 2022 at 12:16 PM Richard Guo <guofenglinux@gmail.com> wrote: > > On Fri, Nov 11, 2022 at 11:26 AM Japin Li <japinli@hotmail.com> wrote: >> >> Recently, when I read the XidInMVCCSnapshot(), and find there are some >> typos in the comments. > > > Hmm, it seems to me 'the subxact array' is just another saying to refer > to snapshot->subxip. I'm not sure about this being typo. But I have no > objection to this change, as it is more consistent with the 'xip array' > saying followed. > Agreed, it is more about being consistent with xip array. -- With Regards, Amit Kapila.
On Fri, 11 Nov 2022 at 15:23, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Nov 11, 2022 at 12:16 PM Richard Guo <guofenglinux@gmail.com> wrote:
>>
>> On Fri, Nov 11, 2022 at 11:26 AM Japin Li <japinli@hotmail.com> wrote:
>>>
>>> Recently, when I read the XidInMVCCSnapshot(), and find there are some
>>> typos in the comments.
>>
>>
>> Hmm, it seems to me 'the subxact array' is just another saying to refer
>> to snapshot->subxip. I'm not sure about this being typo. But I have no
>> objection to this change, as it is more consistent with the 'xip array'
>> saying followed.
>>
>
> Agreed, it is more about being consistent with xip array.
Thanks for reviewings.
Maybe a wrong plural in XidInMvccSnapshot().
* Make a quick range check to eliminate most XIDs without looking at the
* xip arrays.
I think we should use "xip array" instead of "xip arrays".
Furthermore, if the snapshot is taken during recovery, the xip array is
empty, and we should check subxip array. How about changing "xip arrays"
to "xip or subxip array"?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Fri, Nov 11, 2022 at 2:45 PM Japin Li <japinli@hotmail.com> wrote: > > On Fri, 11 Nov 2022 at 15:23, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Nov 11, 2022 at 12:16 PM Richard Guo <guofenglinux@gmail.com> wrote: > >> > >> On Fri, Nov 11, 2022 at 11:26 AM Japin Li <japinli@hotmail.com> wrote: > >>> > >>> Recently, when I read the XidInMVCCSnapshot(), and find there are some > >>> typos in the comments. > >> > >> > >> Hmm, it seems to me 'the subxact array' is just another saying to refer > >> to snapshot->subxip. I'm not sure about this being typo. But I have no > >> objection to this change, as it is more consistent with the 'xip array' > >> saying followed. > >> > > > > Agreed, it is more about being consistent with xip array. > > Thanks for reviewings. > > Maybe a wrong plural in XidInMvccSnapshot(). > > * Make a quick range check to eliminate most XIDs without looking at the > * xip arrays. > > I think we should use "xip array" instead of "xip arrays". > I think here the comment is referring to both xip and subxip array, so it looks okay to me. > Furthermore, if the snapshot is taken during recovery, the xip array is > empty, and we should check subxip array. How about changing "xip arrays" > to "xip or subxip array"? > I don't know if that is an improvement. I think we should stick to your initial proposed change. -- With Regards, Amit Kapila.
On Sat, 12 Nov 2022 at 12:12, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Fri, Nov 11, 2022 at 2:45 PM Japin Li <japinli@hotmail.com> wrote: >> Maybe a wrong plural in XidInMvccSnapshot(). >> >> * Make a quick range check to eliminate most XIDs without looking at the >> * xip arrays. >> >> I think we should use "xip array" instead of "xip arrays". >> > > I think here the comment is referring to both xip and subxip array, so > it looks okay to me. > Yeah, it means xip in normal case, and subxip in recovery case. >> Furthermore, if the snapshot is taken during recovery, the xip array is >> empty, and we should check subxip array. How about changing "xip arrays" >> to "xip or subxip array"? >> > > I don't know if that is an improvement. I think we should stick to > your initial proposed change. Agreed. Let's focus on the initial proposed change. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
On Sun, Nov 13, 2022 at 8:32 PM Japin Li <japinli@hotmail.com> wrote: > > On Sat, 12 Nov 2022 at 12:12, Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > I don't know if that is an improvement. I think we should stick to > > your initial proposed change. > > Agreed. Let's focus on the initial proposed change. > Pushed. -- With Regards, Amit Kapila.