Em sex., 30 de jun. de 2023 às 15:48, Karina Litskevich < litskevichkar...@gmail.com> escreveu:
> Hi, > > Good catch. Indeed, eb.rel shouldn't be NULL there and the tests should be > unnecessary. > Thanks for the confirmation. > However, it doesn't follow from the code of these functions. > From what I can see eb.rel can be NULL in both of these functions. There is > the following Assert in the beginning of the ExtendBufferedRelTo() > function: > > Assert((eb.rel != NULL) != (eb.smgr != NULL)); > > And ExtendBufferedRelShared() is only called from ExtendBufferedRelCommon() > which can be called from ExtendBufferedRelTo() or ExtendBufferedRelBy() > that > also has the same Assert(). And none of these functions assigns eb.rel, so > it can be NULL from the very beginning and it stays the same. > > > And there is the following call in xlogutils.c, which is exactly the case > when > eb.rel is NULL: > > buffer = ExtendBufferedRelTo(EB_SMGR(smgr, RELPERSISTENCE_PERMANENT), > forknum, > NULL, > EB_PERFORMING_RECOVERY | > EB_SKIP_EXTENSION_LOCK, > blkno + 1, > mode); > EB_SMGR and EB_REL are macros for making new structs. IMO these are buggy, once make new structs without initializing all fields. Attached a patch to fix this and make more clear when rel or smgr is NULL. > > > > So as for me calling LockRelationForExtension() and > UnlockRelationForExtension() > without testing eb.rel first looks more like a bug here. However, they are > never > actually called with eb.rel=NULL because of the EB_* flags, so there is no > bug > here. I believe we should add Assert checking that when eb.rel is NULL, > flags > are such that we won't use eb.rel. And yes, we can remove unnecessary > checks > where the flags value guaranty us that eb.rel is not NULL. > Not against these Asserts, but It is very confusing and difficult to understand them without some comment. > > And another minor observation. It seems to me that we don't need a "could > have > been closed while waiting for lock" in ExtendBufferedRelShared(), because I > believe the comment above already explains why updating eb.smgr: > > * Note that another backend might have extended the relation by the time > * we get the lock. > Ok, but the first comment still ambiguous, I think that could be: "/* eb.smgr could have been closed while waiting for lock */" best regards, Ranier Vilela
0002-avoid-new-struct-with-undefined-fields-ExtendedBufferedWhat.patch
Description: Binary data
