|From:||Melanie Plageman <melanieplageman(at)gmail(dot)com>|
|To:||David Rowley <dgrowleyml(at)gmail(dot)com>|
|Cc:||Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: heapgettup refactoring|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Thu, Feb 02, 2023 at 07:00:37PM +1300, David Rowley wrote:
> I've attached a version of the next patch in the series. I admit to
> making a couple of adjustments. I couldn't bring myself to remove the
> snapshot local variable in this commit. We can deal with that when we
> come to it in whichever patch that needs to be changed in.
That seems fine to keep the diff easy to understand. Also,
heapgettup_pagemode() didn't have a snapshot local variable either.
> The only other thing I really did was question your use of rs_cindex
> to store the last OffsetNumber. I ended up adding a new field which
> slots into the padding between a bool and BlockNumber field named
> rs_coffset for this purpose. I noticed what Andres wrote  earlier
> in this thread about that, so thought we should move away from looking
> at the last tuple to get this number
>  https://firstname.lastname@example.org
So, what Andres had said was:
> Hm. Finding the next offset via rs_ctup doesn't seem quite right. For one,
> it's not actually that cheap to extract the offset from an ItemPointer because
> of the the way we pack it into ItemPointerData.
Because I was doing this in an earlier version:
> > + HeapTuple tuple = &(scan->rs_ctup);
And then in the later part of the code got tuple->t_self.
I did this because the code in master does this:
lineoff = /* previous offnum */
So I figured it was the same. Based on Andres' feedback, I switched to
saving the offset number in the scan descriptor and figured I could
reuse rs_cindex since it is larger than an OffsetNumber.
Your code also switches to saving the OffsetNumber -- just in a separate
variable of OffsetNumber type. I am fine with this if it your rationale
is that it is not a good idea to store a smaller number in a larger
datatype. However, the benefit I saw in reusing rs_cindex is that we
could someday converge the code for heapgettup() and
heapgettup_pagemode() even more. Even in my final refactor, there is a
lot of duplicate code between the two.
|Next Message||Nathan Bossart||2023-02-02 17:33:48||Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)|
|Previous Message||Jeff Davis||2023-02-02 16:31:46||Re: Move defaults toward ICU in 16?|