Re: making relfilenodes 56 bits

On Mon, Sep 26, 2022 at 9:56 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
>
> > Can we move the existing definitions from
> > c.h file to some common file (common for client and server)?
>
> Yeah, I think that would be a good idea. Here's a quick patch that
> moves them to common/relpath.h, which seems like a possibly-reasonable
> choice, though perhaps you or someone else will have a better idea.

Looks fine to me.

> > Based on the discussion [1], it seems we can not use
> > INT64_FORMAT/UINT64_FORMAT while using ereport.  But all other places
> > I am using INT64_FORMAT/UINT64_FORMAT.  Does this make sense?
> >
> > [1] https://www.postgresql.org/message-id/20220730113922.qd7qmenwcmzyacje%40alvherre.pgsql
>
> Oh, hmm. So you're saying if the string is not translated then use
> (U)INT64_FORMAT but if it is translated then cast?

Right

I guess that makes
> sense. It feels a bit strange to have the style dependent on the
> context like that, but maybe it's fine. I'll reread with that idea in
> mind.

Ok

> > If we're going to bank on that, we could adapt this more
> > > heavily, e.g. RelidByRelfilenumber() could lose the reltablespace
> > > parameter.
> >
> > Yeah we might, although we need a bool to identify whether it is
> > shared relation or not.
>
> Why?

Because if entry is not in cache then we need to look into the
relmapper and for that we need to know whether it is a shared relation
or not.  And I don't think we can identify that just by looking at
relfilenumber.


Another open comment which I missed in last reply

>      /*
>       * We set up the lockRelId in case anything tries to lock the dummy
> -     * relation.  Note that this is fairly bogus since relNumber may be
> -     * different from the relation's OID.  It shouldn't really matter though.
> -     * In recovery, we are running by ourselves and can't have any lock
> -     * conflicts.  While syncing, we already hold AccessExclusiveLock.
> +     * relation.  Note we are setting relId to just FirstNormalObjectId which
> +     * is completely bogus.  It shouldn't really matter though. In recovery,
> +     * we are running by ourselves and can't have any lock conflicts.  While
> +     * syncing, we already hold AccessExclusiveLock.
>       */
>      rel->rd_lockInfo.lockRelId.dbId = rlocator.dbOid;
> -    rel->rd_lockInfo.lockRelId.relId = rlocator.relNumber;
> +    rel->rd_lockInfo.lockRelId.relId = FirstNormalObjectId;
>
> Boy, this makes me uncomfortable. The existing logic is pretty bogus,
> and we're replacing it with some other bogus thing. Do we know whether
> anything actually does try to use this for locking?

Looking at the code it seems it is not used for locking.  I also test
by setting some special value for relid in
CreateFakeRelcacheEntry() and validating that id is never used for
locking in SET_LOCKTAG_RELATION.  And ran check-world so I could not
see we are ever trying to create lock tag using fake relcache entry.

> One notable difference between the existing logic and your change is
> that, with the existing logic, we use a bogus value that will differ
> from one relation to the next, whereas with this change, it will
> always be the same value. Perhaps el->rd_lockInfo.lockRelId.relId =
> (Oid) rlocator.relNumber would be a more natural adaptation?

I agree, so changed it this way.

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