Thread: [HACKERS] Cleanup: avoid direct use of ip_posid/ip_blkid
Hello All,
--
I would like to propose the attached patch which removes all direct references to ip_posid and ip_blkid members of ItemPointerData struct and instead use ItemPointerGetOffsetNumber and ItemPointerGetBlockNumber macros respectively to access these members.
My motivation to do this is to try to free-up some bits from ip_posid field, given that it can only be maximum 13-bits long. But even without that, it seems like a good cleanup to me.
One reason why these macros are not always used is because they typically do assert-validation to ensure ip_posid has a valid value. There are a few places in code, especially in GIN and also when we are dealing with user-supplied TIDs when we might get a TID with invalid ip_posid. I've handled that by defining and using separate macros which skip the validation. This doesn't seem any worse than what we are already doing.
make check-world with --enable-tap-tests passes.
Comments/suggestions?
Thanks,
Pavan
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On 2/22/17 08:38, Pavan Deolasee wrote: > One reason why these macros are not always used is because they > typically do assert-validation to ensure ip_posid has a valid value. > There are a few places in code, especially in GIN and also when we are > dealing with user-supplied TIDs when we might get a TID with invalid > ip_posid. I've handled that by defining and using separate macros which > skip the validation. This doesn't seem any worse than what we are > already doing. I wonder why we allow that. Shouldn't the tid type reject input that has ip_posid == 0? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 2, 2017 at 8:25 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > I wonder why we allow that. Shouldn't the tid type reject input that > has ip_posid == 0? InvalidOffsetNumber (that is, 0) is something that I wouldn't like to bet doesn't make it out to disk at some point. I know that we use 1 as a meaningless placeholder value for internal B-Tree pages. P_HIKEY is where I get 1 from, which might as well be any other value in practice, I think -- we only need an item pointer to point to a block from an internal page. SpecTokenOffsetNumber can certainly make it to disk, and that is invalid in some sense, even if it isn't actually set to InvalidOffsetNumber. So, seems pretty risky to me. -- Peter Geoghegan
On Thu, Mar 2, 2017 at 9:55 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 2/22/17 08:38, Pavan Deolasee wrote:
> One reason why these macros are not always used is because they
> typically do assert-validation to ensure ip_posid has a valid value.
> There are a few places in code, especially in GIN and also when we are
> dealing with user-supplied TIDs when we might get a TID with invalid
> ip_posid. I've handled that by defining and using separate macros which
> skip the validation. This doesn't seem any worse than what we are
> already doing.
I wonder why we allow that. Shouldn't the tid type reject input that
has ip_posid == 0?
Yes, I think it seems sensible to disallow InvalidOffsetNumber (or > MaxOffsetNumber) in user-supplied value. But there are places in GIN and with INSERT ON CONFLICT where we seem to use special values in ip_posid to mean different things. So we might still need some way to accept invalid values there.
Thanks,
Pavan
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services