From: Tom Lane <tgl@sss.pgh.pa.us>
To: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
cc: Mats Kindahl <mats.kindahl@gmail.com>,
 David Rowley <dgrowleyml@gmail.com>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Use stack-allocated StringInfoData
In-reply-to: <202511061027.6kun3grytx2d@alvherre.pgsql>
References: <202511061027.6kun3grytx2d@alvherre.pgsql>
Comments: In-reply-to =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
	message dated "Thu, 06 Nov 2025 11:31:38 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1032331.1762441863.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 06 Nov 2025 10:11:03 -0500
Message-ID: <1032332.1762441863@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1032332.1762441863%40sss.pgh.pa.us>
Precedence: bulk

=3D?utf-8?Q?=3DC3=3D81lvaro?=3D Herrera <alvherre@kurilemu.de> writes:
> Yeah, I came across this a few weeks ago as well.  I was thinking maybe
> we can expand the ereport() macro instead of duplicating things.  It
> looks a bit ugly and I don't think we do it anywhere else ...  I mean
> something like

> errstart(WARNING, NULL);
> if (cond)
> 	errmsg( ... );
> else
> 	errmsg( ... );
> // and so on
> errfinish(__FILE__, __LINE__, __func__);

> Is this too ugly to live?  (I swear I had this on a branch already, but
> can't find it right this instant.)

I really dislike exposing errstart/errfinish to calling code like
that.  I agree that check_publications_origin_tables is not pretty
as-is, but it's better than breaking open the ereport abstraction.

In the particular case at hand, it seems best to just write
two separate ereport calls for the check_table_sync and
not-check_table_sync cases, as David mentioned.  Duplicating
the errdetail_plural call is slightly annoying but it's net less
code and less surprise than what's there now.

In general, in most other places where we have conditional message
text, it's done via ?: operators within the ereport macro,
for example this decades-old bit in sysv_shmem.c:

        ereport(FATAL,
                (errmsg("could not create shared memory segment: %m"),
                 errdetail("Failed system call was shmget(key=3D%lu, size=3D=
%zu, 0%o).",
                           (unsigned long) memKey, size,
                           IPC_CREAT | IPC_EXCL | IPCProtection),
                 (shmget_errno =3D=3D EINVAL) ?
                 errhint("This error usually means that PostgreSQL's reque=
st for a shared memory "
                         "segment exceeded your kernel's SHMMAX parameter,=
 or possibly that "
                         "it is less than "
                         "your kernel's SHMMIN parameter.\n"
                         "The PostgreSQL documentation contains more infor=
mation about shared "
                         "memory configuration.") : 0,
                 (shmget_errno =3D=3D ENOMEM) ?
                 errhint("This error usually means that PostgreSQL's reque=
st for a shared "
                         "memory segment exceeded your kernel's SHMALL par=
ameter.  You might need "
                         "to reconfigure the kernel with larger SHMALL.\n"
                         "The PostgreSQL documentation contains more infor=
mation about shared "
                         "memory configuration.") : 0,
                 (shmget_errno =3D=3D ENOSPC) ?
                 errhint("This error does *not* mean that you have run out=
 of disk space.  "
                         "It occurs either if all available shared memory =
IDs have been taken, "
                         "in which case you need to raise the SHMMNI param=
eter in your kernel, "
                         "or because the system's overall limit for shared=
 memory has been "
                         "reached.\n"
                         "The PostgreSQL documentation contains more infor=
mation about shared "
                         "memory configuration.") : 0));

We could do something similar here, but I'm not convinced
it's better than just writing two ereport's.

			regards, tom lane