Re: clang-tidy complaints
Hi, On Sat, Apr 12, 2025 at 01:16:39PM -0400, Tom Lane wrote: > Peter Geoghegan writes: > > Looks like I'm done with this process, at least until this time next year. > > OK. Thanks for taking care of it. +1. Peter, do you think you could add "misc-header-include-cycle" in your annual check (see [1])? I'll also put a note on my side to do it at a regular basis. [1]: https://www.postgresql.org/message-id/2238517.1745644856%40sss.pgh.pa.us Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: pgsql: Move SQL-callable code related to multixacts into its own file
Hi, On Tue, Aug 19, 2025 at 10:28:29AM -0400, Andres Freund wrote: > Hi, > > On 2025-08-19 14:01:52 +, Bertrand Drouvot wrote: > > On Tue, Aug 19, 2025 at 12:31:14PM +0200, Álvaro Herrera wrote: > > > After these two changes, a lot of > > > files can stop including dbcommands.h. This seems a nice cleanup to me, > > > and passes headerscheck. > > > > meson does report: > > > > ../src/test/modules/worker_spi/worker_spi.c: In function > > ‘worker_spi_launch’: > > ../src/test/modules/worker_spi/worker_spi.c:455:25: error: implicit > > declaration of function ‘get_database_oid’ [-Wimplicit-function-declaration] > > 455 | dboid = get_database_oid(worker_spi_database, > > false); > > > > It looks like pg_database.h include is missing in worker_spi.c. > > > > That said, autoconf does not report the issue, and that's because > > test/modules > > is missing in src/Makefile. Is there any reasons for that? If not, the > > attached > > fix it. > > That can't be the reason - it's reached from src/test/Makefile Right, but is src/test/Makefile reached? I think that src/test/Makefile has to be reached from src/Makefile. In src/Makefile we already have in SUBDIRS: test/regress \ test/isolation \ test/perl and I think we should add test/modules (like proposed). Does that make sense? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: pgsql: Move SQL-callable code related to multixacts into its own file
Hi, On Tue, Aug 19, 2025 at 12:31:14PM +0200, Álvaro Herrera wrote: > On 2025-Aug-18, Tom Lane wrote: > > > Michael Paquier writes: > > > On Mon, Aug 18, 2025 at 09:47:14AM -0400, Tom Lane wrote: > > >> Couldn't this have removed some #include-s from multixact.c? > > > > > Right. funcapi.h and fmgrprotos.h are direct dependencies, but > > > looking closer it is also possible to remove four more of them. > > > > Sounds good! > > Hmm, don't you find strange that dbcommands.h is still included there? > I wondered about that and found out that we have get_database_name() > declared there. It sort-of makes sense, because it was originally done > by scanning pg_database, but since commit cb98e6fb8fd4 introduced a > syscache for it, we can have this routine in lsyscache.c/h instead, > where it feels more at home. That makes sense to me. > It also seems more sensible to declare > get_database_oid() in pg_database.h. Yes. > After these two changes, a lot of > files can stop including dbcommands.h. This seems a nice cleanup to me, > and passes headerscheck. meson does report: ../src/test/modules/worker_spi/worker_spi.c: In function ‘worker_spi_launch’: ../src/test/modules/worker_spi/worker_spi.c:455:25: error: implicit declaration of function ‘get_database_oid’ [-Wimplicit-function-declaration] 455 | dboid = get_database_oid(worker_spi_database, false); It looks like pg_database.h include is missing in worker_spi.c. That said, autoconf does not report the issue, and that's because test/modules is missing in src/Makefile. Is there any reasons for that? If not, the attached fix it. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com >From 8241c09d6837cd9123ba3177cc261804ea656fb9 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Tue, 19 Aug 2025 13:58:34 + Subject: [PATCH v1] Add test/modules to src/Makefile meson does compile test/modules so let's be consistent in autoconf. --- src/Makefile | 1 + 1 file changed, 1 insertion(+) 100.0% src/ diff --git a/src/Makefile b/src/Makefile index 2f31a2f20a7..fd601c47039 100644 --- a/src/Makefile +++ b/src/Makefile @@ -27,6 +27,7 @@ SUBDIRS = \ bin \ pl \ makefiles \ + test/modules \ test/regress \ test/isolation \ test/perl -- 2.34.1
Re: pgsql: Move SQL-callable code related to multixacts into its own file
Hi, On Tue, Aug 19, 2025 at 05:17:52PM +0200, Álvaro Herrera wrote: > > Yeah, I'm not on board with making changes to the makefiles, because > AFAIR the current arrangement is purposefully what is is. It's coming from 22dfd116a127 and looking at [1] it looks like that the idea at that time was to follow the same build logic as contrib (which, I think, made sense). [1]: https://www.postgresql.org/message-id/20141127192420.GU1639%40alvin.alvh.no-ip.org Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: pgsql: Move SQL-callable code related to multixacts into its own file
Hi, On Tue, Aug 19, 2025 at 11:27:25AM -0400, Tom Lane wrote: > =?utf-8?Q?=C3=81lvaro?= Herrera writes: > > Yeah, I'm not on board with making changes to the makefiles, because > > AFAIR the current arrangement is purposefully what is is. I did > > discover that my build script does "make install ; make -C contrib > > install", which means src/test/modules is not built. But that's a local > > fix for me. > > I think "make world-bin" We have: $(call recurse,world-bin,src config contrib,all) means src/Makefile is reached but will not reach src/test/modules/Makefile because test/modules is not part of src/Makefile's SUBDIRS. > or "make install-world" are the accepted targets for building everything. We have: $(call recurse,install-world,doc src config contrib,install) so, same as above (src/test/modules/Makefile will not be reached). >From what I can see, only: check-world: $(call recurse,check-world,src/test src/pl src/interfaces contrib src/bin src/tools/pg_bsd_indent,check) checkprep: $(call recurse,checkprep, src/test src/pl src/interfaces contrib src/bin) installcheck-world: $(call recurse,installcheck-world,src/test src/pl src/interfaces contrib src/bin,installcheck) build src/test/modules. I just found surprising that a "default" make (no target specified) does not build src/test/modules while a "default" meson/ninja does. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
