Обсуждение: Use LOCKMODE in parse_relation.c/.h
There are a couple of comments in parse_relation.c > Note: properly, lockmode should be declared LOCKMODE not int, but that > would require importing storage/lock.h into parse_relation.h. Since > LOCKMODE is typedef'd as int anyway, that seems like overkill. but actually LOCKMODE has been in storage/lockdefs.h for a while, which is intentionally a more narrow header. So we can include that one in parse_relation.h and just use LOCKMODE normally. An alternative would be to add a duplicate typedef into parse_relation.h, but that doesn't seem necessary here.
Вложения
On 2/18/26 10:50 AM, Peter Eisentraut wrote: > There are a couple of comments in parse_relation.c > > > Note: properly, lockmode should be declared LOCKMODE not int, but that > > would require importing storage/lock.h into parse_relation.h. Since > > LOCKMODE is typedef'd as int anyway, that seems like overkill. > > but actually LOCKMODE has been in storage/lockdefs.h for a while, > which is intentionally a more narrow header. So we can include that > one in parse_relation.h and just use LOCKMODE normally. > > An alternative would be to add a duplicate typedef into > parse_relation.h, but that doesn't seem necessary here. Looks like a nice change and I did not find any more cases where we should fix this. But when I was looking I found a case where BufferLockMode could be used in the gin code (ginStepRight() and the brin (brinGetTupleForHeapBlock) code but I am not sure there are worth fixing. Andreas
On Wed, Feb 18, 2026 at 3:20 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > There are a couple of comments in parse_relation.c > > > Note: properly, lockmode should be declared LOCKMODE not int, but that > > would require importing storage/lock.h into parse_relation.h. Since > > LOCKMODE is typedef'd as int anyway, that seems like overkill. > > but actually LOCKMODE has been in storage/lockdefs.h for a while, > which is intentionally a more narrow header. So we can include that > one in parse_relation.h and just use LOCKMODE normally. lockdefs.h is much younger (4eda0a64705763854225a29703b606692af50e77) than the comment (b153c0920960a6059b67969469166fb29c0105d7) mentioned above. The commit changed some #include "lock.h" to use lockdefs.h. I guess it didn't notice that parse_relation.h can use it because it didn't include lock.h and didn't define LOCKMODE. The change looks good to me. > > An alternative would be to add a duplicate typedef into > parse_relation.h, but that doesn't seem necessary here. Not needed. -- Best Wishes, Ashutosh Bapat
On 19.02.26 10:03, Ashutosh Bapat wrote: > On Wed, Feb 18, 2026 at 3:20 PM Peter Eisentraut <peter@eisentraut.org> wrote: >> >> There are a couple of comments in parse_relation.c >> >> > Note: properly, lockmode should be declared LOCKMODE not int, but that >> > would require importing storage/lock.h into parse_relation.h. Since >> > LOCKMODE is typedef'd as int anyway, that seems like overkill. >> >> but actually LOCKMODE has been in storage/lockdefs.h for a while, >> which is intentionally a more narrow header. So we can include that >> one in parse_relation.h and just use LOCKMODE normally. > > lockdefs.h is much younger (4eda0a64705763854225a29703b606692af50e77) > than the comment (b153c0920960a6059b67969469166fb29c0105d7) mentioned > above. The commit changed some #include "lock.h" to use lockdefs.h. I > guess it didn't notice that parse_relation.h can use it because it > didn't include lock.h and didn't define LOCKMODE. The change looks > good to me. committed
On 18.02.26 18:19, Andreas Karlsson wrote: > On 2/18/26 10:50 AM, Peter Eisentraut wrote: >> There are a couple of comments in parse_relation.c >> >> > Note: properly, lockmode should be declared LOCKMODE not int, but that >> > would require importing storage/lock.h into parse_relation.h. Since >> > LOCKMODE is typedef'd as int anyway, that seems like overkill. >> >> but actually LOCKMODE has been in storage/lockdefs.h for a while, >> which is intentionally a more narrow header. So we can include that >> one in parse_relation.h and just use LOCKMODE normally. >> >> An alternative would be to add a duplicate typedef into >> parse_relation.h, but that doesn't seem necessary here. > > Looks like a nice change and I did not find any more cases where we > should fix this. > > But when I was looking I found a case where BufferLockMode could be used > in the gin code (ginStepRight() and the brin (brinGetTupleForHeapBlock) > code but I am not sure there are worth fixing. I think these could be worth improving, if only to make the function signatures more clear (there are different kinds of enums for different kinds of lock modes).