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
