Обсуждение: pgsql: Move SQL-callable code related to multixacts into its own file
Move SQL-callable code related to multixacts into its own file A patch is under discussion to add more SQL capabilities related to multixacts, and this move avoids bloating the file more than necessary. This affects pg_get_multixact_members(). A side effect of this move is the requirement to add mxstatus_to_string() to multixact.h. Extracted from a larger patch by the same author, tweaked by me. Author: Naga Appani <nagnrik@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Discussion: https://postgr.es/m/CA+QeY+AAsYK6WvBW4qYzHz4bahHycDAY_q5ECmHkEV_eB9ckzg@mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/df9133fa63843627fb3579c89d892dc3d9ea2b95 Modified Files -------------- src/backend/access/transam/multixact.c | 66 +------------------------- src/backend/utils/adt/Makefile | 1 + src/backend/utils/adt/meson.build | 1 + src/backend/utils/adt/multixactfuncs.c | 87 ++++++++++++++++++++++++++++++++++ src/include/access/multixact.h | 1 + 5 files changed, 91 insertions(+), 65 deletions(-)
Michael Paquier <michael@paquier.xyz> writes:
> Move SQL-callable code related to multixacts into its own file
Couldn't this have removed some #include-s from multixact.c?
regards, tom lane
On Mon, Aug 18, 2025 at 09:47:14AM -0400, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: > > Move SQL-callable code related to multixacts into its own file > > 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. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> 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!
regards, tom lane
On 2025-Aug-18, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> 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. It also seems more sensible to declare get_database_oid() in pg_database.h. After these two changes, a lot of files can stop including dbcommands.h. This seems a nice cleanup to me, and passes headerscheck. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Вложения
Hi, On Tue, Aug 19, 2025 at 12:31:14PM +0200, Álvaro Herrera wrote: > On 2025-Aug-18, Tom Lane wrote: > > > Michael Paquier <michael@paquier.xyz> 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
Вложения
Hi, On 2025-08-19 14:01:52 +0000, 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 > SUBDIRS = perl postmaster regress isolation modules authentication recovery subscription Greetings, Andres Freund
Hi,
On Tue, Aug 19, 2025 at 10:28:29AM -0400, Andres Freund wrote:
> Hi,
>
> On 2025-08-19 14:01:52 +0000, 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
Hi, On 2025-08-19 15:08:08 +0000, Bertrand Drouvot wrote: > Hi, > > On Tue, Aug 19, 2025 at 10:28:29AM -0400, Andres Freund wrote: > > Hi, > > > > On 2025-08-19 14:01:52 +0000, 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? Yes. See GNUmakefile.in $(call recurse,check-world,src/test src/pl src/interfaces contrib src/bin src/tools/pg_bsd_indent,check) $(call recurse,checkprep, src/test src/pl src/interfaces contrib src/bin) $(call recurse,installcheck-world,src/test src/pl src/interfaces contrib src/bin,installcheck) $(call recurse,install-tests,src/test/regress,install-tests) Greetings, Andres Freund
On 2025-Aug-19, Andres Freund wrote: > On 2025-08-19 14:01:52 +0000, Bertrand Drouvot wrote: > > 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 > > > SUBDIRS = perl postmaster regress isolation modules authentication recovery subscription 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. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ <inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell <crab> inflex: you know that "amalgam" means "mixture with mercury", more or less, right? <crab> i.e., "deadly poison"
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> 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" or "make install-world" are the accepted
targets for building everything.
regards, tom lane
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
Hi, On Tue, Aug 19, 2025 at 11:27:25AM -0400, Tom Lane wrote: > =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> 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
On Tue, Aug 19, 2025 at 12:31:14PM +0200, Alvaro Herrera wrote: > 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. It also seems more sensible to declare > get_database_oid() in pg_database.h. After these two changes, a lot of > files can stop including dbcommands.h. This seems a nice cleanup to me, > and passes headerscheck. > > 33 files changed, 39 insertions(+), 56 deletions(-) Indeed, that's a nice located cleanup. -- Michael
Вложения
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:
> On Tue, Aug 19, 2025 at 11:27:25AM -0400, Tom Lane wrote:
>> I think "make world-bin"
>> or "make install-world" are the accepted targets for building everything.
> From what I can see, only:
> check-world:
> checkprep:
> installcheck-world:
> build src/test/modules.
I agree that this does not meet the principle of least surprise.
world-bin ought to build every bit of C code we have.
> I just found surprising that a "default" make (no target specified) does not
> build src/test/modules while a "default" meson/ninja does.
In itself that observation doesn't bother me: I don't think the
conventions in the Meson world are identical to those for makefiles.
We've long expected that contrib is not included in the default
make target, and it makes sense to me for src/test/modules to work
like contrib for this purpose.
regards, tom lane