Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On Tue, Sep 21, 2021 at 2:07 AM Tom Lane wrote: > Alexander Korotkov writes: > > On Mon, Sep 20, 2021 at 10:48 PM Andres Freund wrote: > >> I assume you're planning to backpatch this? > > > Yes. > > Probably good to wait 24 hours until 14rc1 has been tagged. OK, NP! -- Regards, Alexander Korotkov
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
Alexander Korotkov writes: > On Mon, Sep 20, 2021 at 10:48 PM Andres Freund wrote: >> I assume you're planning to backpatch this? > Yes. Probably good to wait 24 hours until 14rc1 has been tagged. regards, tom lane
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On Mon, Sep 20, 2021 at 10:48 PM Andres Freund wrote: > On 2021-09-19 18:45:45 +0300, Alexander Korotkov wrote: > > Any objections to pushing this? > > lgtm Thanks! > I assume you're planning to backpatch this? Yes. -- Regards, Alexander Korotkov
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On 2021-09-19 18:45:45 +0300, Alexander Korotkov wrote: > Any objections to pushing this? lgtm I assume you're planning to backpatch this?
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On Sat, Sep 18, 2021 at 11:35 PM Alvaro Herrera wrote:
> On 2021-Sep-18, Alexander Korotkov wrote:
>
> > I see now. I think I'm rather favoring splitting visibilitymap.h.
>
> Agreed, this looks sane to me. However, I think the
> VM_ALL_{VISIBLE,FROZEN} macros should remain in visibilitymap.h, since
> they depend on the visibilitymap_get_status function (and pg_upgrade
> doesn't use them).
>
> There's a typo "maros" for "macros" in the new header file. (Also, why
> does the copyright line say "portions" if no portion under another
> copyright? I think we don't say "portions" when there is only one
> copyright statement line.)
Thank you for the feedback. All changes are accepted.
Any objections to pushing this?
--
Regards,
Alexander Korotkov
0001-Split-macros-from-visibilitymap.h-into-a-separate--3.patch
Description: Binary data
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On 2021-Sep-18, Alexander Korotkov wrote:
> I see now. I think I'm rather favoring splitting visibilitymap.h.
Agreed, this looks sane to me. However, I think the
VM_ALL_{VISIBLE,FROZEN} macros should remain in visibilitymap.h, since
they depend on the visibilitymap_get_status function (and pg_upgrade
doesn't use them).
There's a typo "maros" for "macros" in the new header file. (Also, why
does the copyright line say "portions" if no portion under another
copyright? I think we don't say "portions" when there is only one
copyright statement line.)
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
Hi, On Sat, Sep 18, 2021 at 3:06 AM Andres Freund wrote: > On 2021-09-18 02:51:09 +0300, Alexander Korotkov wrote: > > On Tue, Sep 14, 2021 at 6:53 AM Tom Lane wrote: > > > Without having looked at the details, I think using a forward-declare > > > to avoid including relcache.h in visibilitymap.h might be a reasonably > > > non-painful fix. > > > > I like that idea, but I didn't find an appropriate existing header for > > forward-declaration of Relation. relation.h isn't suitable, because > > it includes primnodes.h. A separate header for just > > forward-definition of Relation seems too much. > > I was just thinking of doing something like the attached. I see now. I think I'm rather favoring splitting visibilitymap.h. > > > TOH, in the long run it might be worth the effort > > > to split visibilitymap.h to separate useful file-contents knowledge > > > from backend function declarations. > > > > I've drafted a patch splitting visibilitymap_maros.h from > > visibilitymap.h. What do you think? > > I'd name it visibilitymapdefs.h or such, mostly because that's what other > headers are named like... Good point. The revised patch is attached. -- Regards, Alexander Korotkov From 798431948da5374ad947fc5f00ef3c87b1bb03ba Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Sat, 18 Sep 2021 02:45:41 +0300 Subject: [PATCH] Split macros from visibilitymap.h into a separate header That allows to include just visibilitymapdefs.h from file.c, and in turn remove include of postgres.h from relcache.h. --- src/bin/pg_upgrade/file.c | 2 +- src/include/access/visibilitymap.h | 16 + src/include/access/visibilitymapdefs.h | 31 ++ src/include/utils/relcache.h | 1 - 4 files changed, 33 insertions(+), 17 deletions(-) create mode 100644 src/include/access/visibilitymapdefs.h diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c index 9b0cc16e452..1b34ee09fa6 100644 --- a/src/bin/pg_upgrade/file.c +++ b/src/bin/pg_upgrade/file.c @@ -19,7 +19,7 @@ #include #endif -#include "access/visibilitymap.h" +#include "access/visibilitymapdefs.h" #include "common/file_perm.h" #include "pg_upgrade.h" #include "storage/bufpage.h" diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h index 57362c36876..3f546667337 100644 --- a/src/include/access/visibilitymap.h +++ b/src/include/access/visibilitymap.h @@ -14,26 +14,12 @@ #ifndef VISIBILITYMAP_H #define VISIBILITYMAP_H +#include "access/visibilitymapdefs.h" #include "access/xlogdefs.h" #include "storage/block.h" #include "storage/buf.h" #include "utils/relcache.h" -/* Number of bits for one heap page */ -#define BITS_PER_HEAPBLOCK 2 - -/* Flags for bit map */ -#define VISIBILITYMAP_ALL_VISIBLE 0x01 -#define VISIBILITYMAP_ALL_FROZEN 0x02 -#define VISIBILITYMAP_VALID_BITS 0x03 /* OR of all valid visibilitymap - * flags bits */ - -/* Macros for visibilitymap test */ -#define VM_ALL_VISIBLE(r, b, v) \ - ((visibilitymap_get_status((r), (b), (v)) & VISIBILITYMAP_ALL_VISIBLE) != 0) -#define VM_ALL_FROZEN(r, b, v) \ - ((visibilitymap_get_status((r), (b), (v)) & VISIBILITYMAP_ALL_FROZEN) != 0) - extern bool visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags); extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk, diff --git a/src/include/access/visibilitymapdefs.h b/src/include/access/visibilitymapdefs.h new file mode 100644 index 000..01d0611bb6b --- /dev/null +++ b/src/include/access/visibilitymapdefs.h @@ -0,0 +1,31 @@ +/*- + * + * visibilitymapdefs.h + * maros for accessing contents of visibility map pages + * + * + * Portions Copyright (c) 2021, PostgreSQL Global Development Group + * + * src/include/access/visibilitymapdefs.h + * + *- + */ +#ifndef VISIBILITYMAPDEFS_H +#define VISIBILITYMAPDEFS_H + +/* Number of bits for one heap page */ +#define BITS_PER_HEAPBLOCK 2 + +/* Flags for bit map */ +#define VISIBILITYMAP_ALL_VISIBLE 0x01 +#define VISIBILITYMAP_ALL_FROZEN 0x02 +#define VISIBILITYMAP_VALID_BITS 0x03 /* OR of all valid visibilitymap + * flags bits */ + +/* Macros for visibilitymap test */ +#define VM_ALL_VISIBLE(r, b, v) \ + ((visibilitymap_get_status((r), (b), (v)) & VISIBILITYMAP_ALL_VISIBLE) != 0) +#define VM_ALL_FROZEN(r, b, v) \ + ((visibilitymap_get_status((r), (b), (v)) & VISIBILITYMAP_ALL_FROZEN) != 0) + +#endif /* VISIBILITYMAPDEFS_H */ diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index f772855ac69..d2c17575f65 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -14,7 +14,6 @@ #ifndef RELCACHE_H #define RELCACHE_H -#include "postgres.h" #include "access/tupdesc.h" #include "nodes/bitmapset.h" -- 2.24.3 (Apple Git-128)
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
Hi, On 2021-09-18 02:51:09 +0300, Alexander Korotkov wrote: > On Tue, Sep 14, 2021 at 6:53 AM Tom Lane wrote: > > Without having looked at the details, I think using a forward-declare > > to avoid including relcache.h in visibilitymap.h might be a reasonably > > non-painful fix. > > I like that idea, but I didn't find an appropriate existing header for > forward-declaration of Relation. relation.h isn't suitable, because > it includes primnodes.h. A separate header for just > forward-definition of Relation seems too much. I was just thinking of doing something like the attached. > > TOH, in the long run it might be worth the effort > > to split visibilitymap.h to separate useful file-contents knowledge > > from backend function declarations. > > I've drafted a patch splitting visibilitymap_maros.h from > visibilitymap.h. What do you think? I'd name it visibilitymapdefs.h or such, mostly because that's what other headers are named like... Greetings, Andres Freund diff --git i/src/include/access/visibilitymap.h w/src/include/access/visibilitymap.h index 57362c36876..d7f7f30b1fb 100644 --- i/src/include/access/visibilitymap.h +++ w/src/include/access/visibilitymap.h @@ -17,7 +17,6 @@ #include "access/xlogdefs.h" #include "storage/block.h" #include "storage/buf.h" -#include "utils/relcache.h" /* Number of bits for one heap page */ #define BITS_PER_HEAPBLOCK 2 @@ -27,6 +26,7 @@ #define VISIBILITYMAP_ALL_FROZEN 0x02 #define VISIBILITYMAP_VALID_BITS 0x03 /* OR of all valid visibilitymap * flags bits */ +struct RelationData; /* Macros for visibilitymap test */ #define VM_ALL_VISIBLE(r, b, v) \ @@ -34,17 +34,17 @@ #define VM_ALL_FROZEN(r, b, v) \ ((visibilitymap_get_status((r), (b), (v)) & VISIBILITYMAP_ALL_FROZEN) != 0) -extern bool visibilitymap_clear(Relation rel, BlockNumber heapBlk, +extern bool visibilitymap_clear(struct RelationData *rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags); -extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk, +extern void visibilitymap_pin(struct RelationData *rel, BlockNumber heapBlk, Buffer *vmbuf); extern bool visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf); -extern void visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, +extern void visibilitymap_set(struct RelationData *rel, BlockNumber heapBlk, Buffer heapBuf, XLogRecPtr recptr, Buffer vmBuf, TransactionId cutoff_xid, uint8 flags); -extern uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *vmbuf); -extern void visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen); -extern BlockNumber visibilitymap_prepare_truncate(Relation rel, +extern uint8 visibilitymap_get_status(struct RelationData *rel, BlockNumber heapBlk, Buffer *vmbuf); +extern void visibilitymap_count(struct RelationData *rel, BlockNumber *all_visible, BlockNumber *all_frozen); +extern BlockNumber visibilitymap_prepare_truncate(struct RelationData *rel, BlockNumber nheapblocks); #endif /* VISIBILITYMAP_H */ diff --git i/src/include/utils/relcache.h w/src/include/utils/relcache.h index f772855ac69..d2c17575f65 100644 --- i/src/include/utils/relcache.h +++ w/src/include/utils/relcache.h @@ -14,7 +14,6 @@ #ifndef RELCACHE_H #define RELCACHE_H -#include "postgres.h" #include "access/tupdesc.h" #include "nodes/bitmapset.h"
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On Tue, Sep 14, 2021 at 6:53 AM Tom Lane wrote: > Andres Freund writes: > > On 2021-09-13 22:40:19 -0400, Tom Lane wrote: > >> As for the fix ... what in the world is pg_upgrade doing including > >> relcache.h? It seems like there's a more fundamental problem here: > >> either relcache.h is declaring something that needs to be elsewhere, > >> or pg_upgrade is doing something it should not. > > > We could split visibilitymap.h into two, or we could forward-declare > > Relation > > and not include relcache... > > Without having looked at the details, I think using a forward-declare > to avoid including relcache.h in visibilitymap.h might be a reasonably > non-painful fix. I like that idea, but I didn't find an appropriate existing header for forward-declaration of Relation. relation.h isn't suitable, because it includes primnodes.h. A separate header for just forward-definition of Relation seems too much. > TOH, in the long run it might be worth the effort > to split visibilitymap.h to separate useful file-contents knowledge > from backend function declarations. I've drafted a patch splitting visibilitymap_maros.h from visibilitymap.h. What do you think? -- Regards, Alexander Korotkov 0001-Split-macros-from-visibilitymap.h-into-a-separate-h-.patch Description: Binary data
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
On Tue, Sep 14, 2021 at 5:40 AM Tom Lane wrote: > Andres Freund writes: > > I noticed that postgres.h is included from relcache.h (starting in [1]) and > > wanted to fix that - it violates our usual policy against including > > postgres.h > > from within headers. > > Ugh, yeah, that's entirely against policy. I see. This is my oversight, sorry for that. -- Regards, Alexander Korotkov
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
Andres Freund writes: > On 2021-09-13 22:40:19 -0400, Tom Lane wrote: >> As for the fix ... what in the world is pg_upgrade doing including >> relcache.h? It seems like there's a more fundamental problem here: >> either relcache.h is declaring something that needs to be elsewhere, >> or pg_upgrade is doing something it should not. > We could split visibilitymap.h into two, or we could forward-declare Relation > and not include relcache... Without having looked at the details, I think using a forward-declare to avoid including relcache.h in visibilitymap.h might be a reasonably non-painful fix. OTOH, in the long run it might be worth the effort to split visibilitymap.h to separate useful file-contents knowledge from backend function declarations. >> No. If anything, I'd want to throw an error for "redundant" includes >> of these files, because it's a pretty good red flag about >> poorly-thought-out header modularization. > I think we might be thinking of the same. What I meant with "avoid" was to > raise a warning or error. Ah, we are on the same page then. I misunderstood what you wrote. > If we were to do that, it's probably worth doing the > build system ugliness to do this only when building postgres code, rather than > extensions... As long as we do this in HEAD only, I'm not sure why extensions need an exception. Perhaps it will result in somebody pointing out additional poorly-thought-out header contents, but I don't think that's bad. regards, tom lane
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
Hi, On 2021-09-13 22:40:19 -0400, Tom Lane wrote: > Andres Freund writes: > > I noticed that postgres.h is included from relcache.h (starting in [1]) and > > wanted to fix that - it violates our usual policy against including > > postgres.h > > from within headers. > > Ugh, yeah, that's entirely against policy. > > As for the fix ... what in the world is pg_upgrade doing including > relcache.h? It seems like there's a more fundamental problem here: > either relcache.h is declaring something that needs to be elsewhere, > or pg_upgrade is doing something it should not. It's not directly including relcache. pg_upgrade/file.c needs a few symbols from visibilitymap.h, for the upgrade of the visibilitymap from old to new format. And visibilitymap needs relcache.h because several of its routines take Relation params. We could split visibilitymap.h into two, or we could forward-declare Relation and not include relcache... > > I was also wondering if we should put something in c.h and postgres.h to > > avoid > > redundant includes? > > No. If anything, I'd want to throw an error for "redundant" includes > of these files, because it's a pretty good red flag about > poorly-thought-out header modularization. I think we might be thinking of the same. What I meant with "avoid" was to raise a warning or error. If we were to do that, it's probably worth doing the build system ugliness to do this only when building postgres code, rather than extensions... Greetings, Andres Freund
Re: postgres.h included from relcache.h - but removing it breaks pg_upgrade
Andres Freund writes: > I noticed that postgres.h is included from relcache.h (starting in [1]) and > wanted to fix that - it violates our usual policy against including postgres.h > from within headers. Ugh, yeah, that's entirely against policy. As for the fix ... what in the world is pg_upgrade doing including relcache.h? It seems like there's a more fundamental problem here: either relcache.h is declaring something that needs to be elsewhere, or pg_upgrade is doing something it should not. > I was also wondering if we should put something in c.h and postgres.h to avoid > redundant includes? No. If anything, I'd want to throw an error for "redundant" includes of these files, because it's a pretty good red flag about poorly-thought-out header modularization. regards, tom lane
postgres.h included from relcache.h - but removing it breaks pg_upgrade
Hi, I noticed that postgres.h is included from relcache.h (starting in [1]) and wanted to fix that - it violates our usual policy against including postgres.h from within headers. But then I noticed that that causes pg_upgrade/file.c to fail to compile: In file included from /home/andres/src/postgresql/src/include/access/visibilitymap.h:20, from /home/andres/src/postgresql/src/bin/pg_upgrade/file.c:22: /home/andres/src/postgresql/src/include/utils/relcache.h:53:8: error: unknown type name ‘Datum’ 53 | extern Datum *RelationGetIndexRawAttOptions(Relation relation); Which is presumably why the postgres.h include was added in [1]. The only reason this didn't fail before is because there wasn't any other reference to Datum (or any of the other postgres.h types) in relcache.h before this commit. I guess the best solution is to add include the "full" postgres.h explicitly from file.c like several other places, like e.g. src/bin/pg_controldata/pg_controldata.c do: /* * We have to use postgres.h not postgres_fe.h here, because there's so much * backend-only stuff in the XLOG include files we need. But we need a * frontend-ish environment otherwise. Hence this ugly hack. */ #define FRONTEND 1 I was also wondering if we should put something in c.h and postgres.h to avoid redundant includes? Currently there's a few .c files that "use" the header guards, all scanners that we include into the parsers. Greetings, Andres Freund [1] commit 911e70207703799605f5a0e8aad9f06cff067c63 Author: Alexander Korotkov Date: 2020-03-30 19:17:11 +0300 Implement operator class parameters
