Horton

Thread

Commits

  1. Avoid including commands/dbcommands.h in so many places

  2. Remove unneeded header declarations in multixact.c

  3. Move SQL-callable code related to multixacts into its own file

  4. Create a syscache for pg_database-indexed-by-oid, and make use of it

  1. pgsql: Move SQL-callable code related to multixacts into its own file

    Michael Paquier <michael@paquier.xyz> — 2025-08-18T06:14:05Z

    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(-)

  2. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-18T13:47:14Z

    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

  3. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Michael Paquier <michael@paquier.xyz> — 2025-08-18T23:24:04Z

    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

  4. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-18T23:38:09Z

    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

  5. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-08-19T10:31:14Z

    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/

  6. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-19T14:01:52Z

    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

  7. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Andres Freund <andres@anarazel.de> — 2025-08-19T14:28:29Z

    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

  8. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-19T15:08:08Z

    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

  9. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Andres Freund <andres@anarazel.de> — 2025-08-19T15:12:03Z

    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

  10. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-08-19T15:17:52Z

    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"

  11. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-19T15:27:25Z

    =?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

  12. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-20T05:57:20Z

    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

  13. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-20T05:58:57Z

    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

  14. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Michael Paquier <michael@paquier.xyz> — 2025-08-20T08:11:05Z

    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

  15. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-20T12:17:26Z

    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

  16. Re: pgsql: Move SQL-callable code related to multixacts into its own file

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-08-28T13:23:07Z

    On 2025-Aug-19, Bertrand Drouvot wrote:
> That makes sense to me.

On 2025-Aug-20, Michael Paquier wrote:
> Indeed, that's a nice located cleanup.


Thanks for looking!  I have pushed it now.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I'm always right, but sometimes I'm more right than other times."
                                                  (Linus Torvalds)
https://lore.kernel.org/git/Pine.LNX.4.58.0504150753440.7211@ppc970.osdl.org/