From: Mats Kindahl <mats.kindahl@gmail.com>
To: Álvaro Herrera <alvherre@kurilemu.de>
Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
Subject: Re: Refactor StringInfo usage in subscriptioncmds.c
Date: Thu, 6 Nov 2025 13:47:35 +0100
Message-ID: <222afb8b-40da-493a-8a61-1b9cbc72138a@gmail.com> (raw)
In-Reply-To: <202511061224.qwx6pqiyiwku@alvherre.pgsql>
References: <202511061224.qwx6pqiyiwku@alvherre.pgsql>
On 11/6/25 13:40, Álvaro Herrera wrote:
> On 2025-Nov-06, Mats Kindahl wrote:
>
>> Attached is a proposal that instead of building error message and hints
>> dynamically, it will use ereport() directly and as a result does not have to
>> allocate the error message strings and error message hints dynamically and
>> these can be removed.
> LGTM, thanks.
>
> I agree with Amit that there doesn't seem to be a need to free
> pubnames.data. We're already leaking publist, for instance. This is
> okay since we only call these functions during DDL, which in general is
> not sensitive to leaks.
Seems reasonable. Here is an updated version that removes the pfree() calls.
Best wishes,
Mats Kindahl
Attachments:0001-Refactor-StringInfo-usage-in-subscriptioncmds.c.v2.patch (text/x-patch, 5.2K) download
From ebf89a82ba463e019e8dba840ddb01905a32b3e7 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@kindahl.net>
Date: Thu, 6 Nov 2025 11:47:50 +0100
Subject: [PATCH] Refactor StringInfo usage in subscriptioncmds.c
This commit removes some uses of StringInfo that was not necessary and was only
there to avoid having to write two calls of ereport() as well as replacing
dynamic allocation of StringInfo with a StringInfoData usage instead.
---
src/backend/commands/subscriptioncmds.c | 58 +++++++++++--------------
1 file changed, 25 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.cindex 3d29818badd..57298632ad3 100644--- a/src/backend/commands/subscriptioncmds.c+++ b/src/backend/commands/subscriptioncmds.c@@ -2599,33 +2599,30 @@ check_publications_origin_tables(WalReceiverConn *wrconn, List *publications,
*/
if (publist)
{
- StringInfo pubnames = makeStringInfo();- StringInfo err_msg = makeStringInfo();- StringInfo err_hint = makeStringInfo();+ StringInfoData pubnames;
/* Prepare the list of publication(s) for warning message. */
- GetPublicationsStr(publist, pubnames, false);+ initStringInfo(&pubnames);+ GetPublicationsStr(publist, &pubnames, false);
if (check_table_sync)
- {- appendStringInfo(err_msg, _("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin"),- subname);- appendStringInfoString(err_hint, _("Verify that initial data copied from the publisher tables did not come from other origins."));- }+ ereport(WARNING,+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),+ errmsg("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin",+ subname),+ errdetail_plural("The subscription subscribes to a publication (%s) that contains tables that are written to by other subscriptions.",+ "The subscription subscribes to publications (%s) that contain tables that are written to by other subscriptions.",+ list_length(publist), pubnames.data),+ errhint("Verify that initial data copied from the publisher tables did not come from other origins."));
else
- {- appendStringInfo(err_msg, _("subscription \"%s\" enabled retain_dead_tuples but might not reliably detect conflicts for changes from different origins"),- subname);- appendStringInfoString(err_hint, _("Consider using origin = NONE or disabling retain_dead_tuples."));- }-- ereport(WARNING,- errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),- errmsg_internal("%s", err_msg->data),- errdetail_plural("The subscription subscribes to a publication (%s) that contains tables that are written to by other subscriptions.",- "The subscription subscribes to publications (%s) that contain tables that are written to by other subscriptions.",- list_length(publist), pubnames->data),- errhint_internal("%s", err_hint->data));+ ereport(WARNING,+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),+ errmsg("subscription \"%s\" enabled retain_dead_tuples but might not reliably detect conflicts for changes from different origins",+ subname),+ errdetail_plural("The subscription subscribes to a publication (%s) that contains tables that are written to by other subscriptions.",+ "The subscription subscribes to publications (%s) that contain tables that are written to by other subscriptions.",+ list_length(publist), pubnames.data),+ errhint("Consider using origin = NONE or disabling retain_dead_tuples."));
}
ExecDropSingleTupleTableSlot(slot);
@@ -2716,24 +2713,19 @@ check_publications_origin_sequences(WalReceiverConn *wrconn, List *publications,
*/
if (publist)
{
- StringInfo pubnames = makeStringInfo();- StringInfo err_msg = makeStringInfo();- StringInfo err_hint = makeStringInfo();+ StringInfoData pubnames;
/* Prepare the list of publication(s) for warning message. */
- GetPublicationsStr(publist, pubnames, false);-- appendStringInfo(err_msg, _("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin"),- subname);- appendStringInfoString(err_hint, _("Verify that initial data copied from the publisher sequences did not come from other origins."));+ initStringInfo(&pubnames);+ GetPublicationsStr(publist, &pubnames, false);
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg_internal("%s", err_msg->data),+ errmsg("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin", subname),
errdetail_plural("The subscription subscribes to a publication (%s) that contains sequences that are written to by other subscriptions.",
"The subscription subscribes to publications (%s) that contain sequences that are written to by other subscriptions.",
- list_length(publist), pubnames->data),- errhint_internal("%s", err_hint->data));+ list_length(publist), pubnames.data),+ errhint("Verify that initial data copied from the publisher sequences did not come from other origins."));
}
ExecDropSingleTupleTableSlot(slot);
--
2.43.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: mats.kindahl@gmail.com, alvherre@kurilemu.de, dgrowleyml@gmail.com
Subject: Re: Refactor StringInfo usage in subscriptioncmds.c
In-Reply-To: <222afb8b-40da-493a-8a61-1b9cbc72138a@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox