Обсуждение: Refactor StringInfo usage in subscriptioncmds.c

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.gmail.com

[2]: 

https://www.postgresql.org/message-id/flat/CAApHDvrZnM28wa2VY58cvtY0y9XbMhKJH4m%3Dga3c1wfsx%3DMF4Q%40mail.gmail.com#ba34970e59f9fd847f1ab52777d57edc

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

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.

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

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

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

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