pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRefactor StringInfo usage in subscriptioncmds.c
7+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2025-11-06 448b6a4173 Tidyup WARNING ereports in subscriptioncmds.c
* Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 11:38 Mats Kindahl <mats.kindahl@gmail.com>
0 siblings, 2 replies; 7+ messages in thread
From: Mats Kindahl @ 2025-11-06 11:38 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: David Rowley <dgrowleyml@gmail.com>
Hi all,
As discussed in [1] the functions check_publications_origin_tables() and
check_publications_origin_sequences() are building error messages using
dynamically allocated StringInfo instances only to avoid duplicating a
call of ereport().
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.
This also means that a previous use of gettext() (in the form of the "_"
macro) is not needed any more and we can use errmsg() and errhint()
rather than errmsg_internal() and errhint_internal() with ereport().
It also replaces the usage a dynamically allocated StringInfo of
pubnames containing the publication names with a stack allocated
StringInfoData, along the same line as in [2].
[1]:
https://www.postgresql.org/message-id/CAApHDvrZnM28wa2VY58cvtY0y9XbMhKJH4m%3Dga3c1wfsx%3DMF4Q%40mail...
[2]:
https://www.postgresql.org/message-id/flat/CAApHDvrZnM28wa2VY58cvtY0y9XbMhKJH4m%3Dga3c1wfsx%3DMF4Q%4...
Attachments:
0001-Refactor-StringInfo-usage-in-subscriptioncmds.c.v1.patch (text/x-patch, 5.3K) download
From 2436de272d39e664fa0c0010cf875cb34fcecd1e 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 | 61 ++++++++++++-------------
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 3d29818badd..bd457dc6e62 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2599,33 +2599,32 @@ 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."));
+
+ pfree(pubnames.data);
}
ExecDropSingleTupleTableSlot(slot);
@@ -2716,24 +2715,20 @@ 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."));
+ pfree(pubnames.data);
}
ExecDropSingleTupleTableSlot(slot);
--
2.43.0
* Re: Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 11:54 David Rowley <dgrowleyml@gmail.com>
parent: Mats Kindahl <mats.kindahl@gmail.com>
1 sibling, 1 reply; 7+ messages in thread
From: David Rowley @ 2025-11-06 11:54 UTC (permalink / raw)
To: Mats Kindahl <mats.kindahl@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; vignesh C <vignesh21@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, 7 Nov 2025 at 00:38, Mats Kindahl <mats.kindahl@gmail.com> wrote:
> As discussed in [1] the functions check_publications_origin_tables() and
> check_publications_origin_sequences() are building error messages using
> dynamically allocated StringInfo instances only to avoid duplicating a
> call of ereport().
Looks better and more traditional to me, plus git diff --stat reports:
1 file changed, 28 insertions(+), 33 deletions(-)
It has my vote. I've included Vignesh and Amit to see if they're also
ok with it.
David
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 12:09 Amit Kapila <amit.kapila16@gmail.com>
parent: David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Amit Kapila @ 2025-11-06 12:09 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Mats Kindahl <mats.kindahl@gmail.com>; vignesh C <vignesh21@gmail.com>; pgsql-hackers@lists.postgresql.org
On Thu, Nov 6, 2025 at 5:24 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Fri, 7 Nov 2025 at 00:38, Mats Kindahl <mats.kindahl@gmail.com> wrote:
> > As discussed in [1] the functions check_publications_origin_tables() and
> > check_publications_origin_sequences() are building error messages using
> > dynamically allocated StringInfo instances only to avoid duplicating a
> > call of ereport().
>
> Looks better and more traditional to me, plus git diff --stat reports:
>
> 1 file changed, 28 insertions(+), 33 deletions(-)
>
> It has my vote. I've included Vignesh and Amit to see if they're also
> ok with it.
>
LGTM as well. We can avoid doing pfree(pubnames.data) as it is not
allocated in any long term context but I am fine if we want to do it
as there are places where we do such retail pfree as well.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 12:34 Mats Kindahl <mats.kindahl@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Mats Kindahl @ 2025-11-06 12:34 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; David Rowley <dgrowleyml@gmail.com>; +Cc: vignesh C <vignesh21@gmail.com>; pgsql-hackers@lists.postgresql.org
On 11/6/25 13:09, Amit Kapila wrote:
> On Thu, Nov 6, 2025 at 5:24 PM David Rowley <dgrowleyml@gmail.com> wrote:
>> On Fri, 7 Nov 2025 at 00:38, Mats Kindahl <mats.kindahl@gmail.com> wrote:
>>> As discussed in [1] the functions check_publications_origin_tables() and
>>> check_publications_origin_sequences() are building error messages using
>>> dynamically allocated StringInfo instances only to avoid duplicating a
>>> call of ereport().
>> Looks better and more traditional to me, plus git diff --stat reports:
>>
>> 1 file changed, 28 insertions(+), 33 deletions(-)
>>
>> It has my vote. I've included Vignesh and Amit to see if they're also
>> ok with it.
>>
> LGTM as well. We can avoid doing pfree(pubnames.data) as it is not
> allocated in any long term context but I am fine if we want to do it
> as there are places where we do such retail pfree as well.
I have no strong opinion either way, but since it is not needed and it
is not a frequent call it seemed prudent to free it after.
Best wishes,
Mats Kindahl
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 12:40 Álvaro Herrera <alvherre@kurilemu.de>
parent: Mats Kindahl <mats.kindahl@gmail.com>
1 sibling, 1 reply; 7+ messages in thread
From: Álvaro Herrera @ 2025-11-06 12:40 UTC (permalink / raw)
To: Mats Kindahl <mats.kindahl@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
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.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Small aircraft do not crash frequently ... usually only once!"
(ponder, http://thedailywtf.com/)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 12:47 Mats Kindahl <mats.kindahl@gmail.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 7+ messages in thread
From: Mats Kindahl @ 2025-11-06 12:47 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
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.c
index 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
* Re: Refactor StringInfo usage in subscriptioncmds.c
@ 2025-11-06 20:50 David Rowley <dgrowleyml@gmail.com>
parent: Mats Kindahl <mats.kindahl@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: David Rowley @ 2025-11-06 20:50 UTC (permalink / raw)
To: Mats Kindahl <mats.kindahl@gmail.com>; +Cc: Álvaro Herrera <alvherre@kurilemu.de>; pgsql-hackers@lists.postgresql.org
On Fri, 7 Nov 2025 at 01:47, Mats Kindahl <mats.kindahl@gmail.com> wrote:
> > 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.
Sounds like all are in favour. Pushed.
David
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-11-06 20:50 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-11-06 11:38 Refactor StringInfo usage in subscriptioncmds.c Mats Kindahl <mats.kindahl@gmail.com>
2025-11-06 11:54 ` David Rowley <dgrowleyml@gmail.com>
2025-11-06 12:09 ` Amit Kapila <amit.kapila16@gmail.com>
2025-11-06 12:34 ` Mats Kindahl <mats.kindahl@gmail.com>
2025-11-06 12:40 ` Álvaro Herrera <alvherre@kurilemu.de>
2025-11-06 12:47 ` Mats Kindahl <mats.kindahl@gmail.com>
2025-11-06 20:50 ` David Rowley <dgrowleyml@gmail.com>
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