Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove translation marker from libpq-be-fe-helpers.h.

  2. Log remote NOTICE, WARNING, and similar messages using ereport().

  3. Remove translation markers from libpq-be-fe-helpers.h

  1. Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-04-15T04:37:24Z

    Hi,

Currently, when a warning is emitted by the publisher, the
corresponding log message does not include the log prefix. This makes
it harder to correlate such messages with other log entries. For
example, in a simulated error scenario where directory removal fails,
the notice message lacks the standard log prefix, as shown below:
2025-03-18 16:44:36.071 IST [196901] LOG:  logical replication table
synchronization worker for subscription "sub1", table "t1" has
finished
WARNING:  could not remove directory
"pg_replslot/pg_16398_sync_16387_7483106341004194035.tmp"

In this case, the WARNING line does not include the usual timestamp
information, making it harder to trace.

To address this, we can have a custom notice processor for WAL
receiver connections—similar to what's done in the attached patch.
This ensures that notices received during both streaming and logical
replication include the appropriate log prefix. Since this issue is
present in both replication modes, the patch sets the notice processor
for all WAL receiver connections.

Regards,
Vignesh

  2. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-14T16:06:11Z

    
On 2025/04/15 13:37, vignesh C wrote:
> Hi,
> 
> Currently, when a warning is emitted by the publisher, the
> corresponding log message does not include the log prefix. This makes
> it harder to correlate such messages with other log entries. For
> example, in a simulated error scenario where directory removal fails,
> the notice message lacks the standard log prefix, as shown below:
> 2025-03-18 16:44:36.071 IST [196901] LOG:  logical replication table
> synchronization worker for subscription "sub1", table "t1" has
> finished
> WARNING:  could not remove directory
> "pg_replslot/pg_16398_sync_16387_7483106341004194035.tmp"
> 
> In this case, the WARNING line does not include the usual timestamp
> information, making it harder to trace.
> 
> To address this, we can have a custom notice processor for WAL
> receiver connections—similar to what's done in the attached patch.

I like this idea.

If this issue also exists other features like dblink or postgres_fdw
connecting to remote PostgreSQL servers, it might be worth applying
a similar improvement there as well.

+notice_processor(void *arg, const char *message)
+{
+	elog(LOG, "%s", message);

Should ereport() be used here instead?

Also, would it be better to add some context before %s, for example,
something like "received message from the primary:"? Without that,
users might mistakenly think the message originated from the local server.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  3. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-15T17:24:08Z

    On Mon, 14 Jul 2025 at 21:36, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2025/04/15 13:37, vignesh C wrote:
> > Hi,
> >
> > Currently, when a warning is emitted by the publisher, the
> > corresponding log message does not include the log prefix. This makes
> > it harder to correlate such messages with other log entries. For
> > example, in a simulated error scenario where directory removal fails,
> > the notice message lacks the standard log prefix, as shown below:
> > 2025-03-18 16:44:36.071 IST [196901] LOG:  logical replication table
> > synchronization worker for subscription "sub1", table "t1" has
> > finished
> > WARNING:  could not remove directory
> > "pg_replslot/pg_16398_sync_16387_7483106341004194035.tmp"
> >
> > In this case, the WARNING line does not include the usual timestamp
> > information, making it harder to trace.
> >
> > To address this, we can have a custom notice processor for WAL
> > receiver connections—similar to what's done in the attached patch.
>
> I like this idea.
>
> If this issue also exists other features like dblink or postgres_fdw
> connecting to remote PostgreSQL servers, it might be worth applying
> a similar improvement there as well.

Included it for dblink and fdw

> +notice_processor(void *arg, const char *message)
> +{
> +       elog(LOG, "%s", message);
>
> Should ereport() be used here instead?

Yes, that is better. Modified it to ereport

> Also, would it be better to add some context before %s, for example,
> something like "received message from the primary:"? Without that,
> users might mistakenly think the message originated from the local server.

Yes, that makes sense. Updated accordingly.

For dblink and postgres_fdw, create the dblink and postgres_fdw setup
and create the below object to raise a notice:
CREATE OR REPLACE FUNCTION emit_notice(msg text) RETURNS int AS $$
BEGIN
    RAISE NOTICE '%', msg;
    RETURN 1;
END;
$$ LANGUAGE plpgsql;

CREATE VIEW my_notice_view AS SELECT 1 AS value;

CREATE OR REPLACE RULE "_RETURN" AS
    ON SELECT TO my_notice_view
    DO INSTEAD
    SELECT 1 AS value
    FROM (SELECT emit_notice('NOTICE from _RETURN rule') AS dummy) s;

-- Scenario to see the notice raised by remote server using dblink
SELECT * FROM dblink('dbname=remotedb user=remoteuser',  'SELECT *
FROM my_notice_view') AS t(value int);

--  Scenario to see the notice raised by remote server using postgres_fdw
IMPORT FOREIGN SCHEMA public LIMIT TO (my_notice_view) FROM SERVER
foreign_server INTO public;

The attached v2 version patch has the changes for the same.

Regards,
Vignesh

  4. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-16T03:39:00Z

    On Wed, Jul 16, 2025 at 2:24 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 14 Jul 2025 at 21:36, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> >
> >
> > On 2025/04/15 13:37, vignesh C wrote:
> > > Hi,
> > >
> > > Currently, when a warning is emitted by the publisher, the
> > > corresponding log message does not include the log prefix. This makes
> > > it harder to correlate such messages with other log entries. For
> > > example, in a simulated error scenario where directory removal fails,
> > > the notice message lacks the standard log prefix, as shown below:
> > > 2025-03-18 16:44:36.071 IST [196901] LOG:  logical replication table
> > > synchronization worker for subscription "sub1", table "t1" has
> > > finished
> > > WARNING:  could not remove directory
> > > "pg_replslot/pg_16398_sync_16387_7483106341004194035.tmp"
> > >
> > > In this case, the WARNING line does not include the usual timestamp
> > > information, making it harder to trace.
> > >
> > > To address this, we can have a custom notice processor for WAL
> > > receiver connections—similar to what's done in the attached patch.
> >
> > I like this idea.
> >
> > If this issue also exists other features like dblink or postgres_fdw
> > connecting to remote PostgreSQL servers, it might be worth applying
> > a similar improvement there as well.
>
> Included it for dblink and fdw

Thanks! It's better to submit each change for dblink and postgres_fdw
as separate patches, and first focus on the patch that sets the notice
processor for replication connections.


> > +notice_processor(void *arg, const char *message)
> > +{
> > +       elog(LOG, "%s", message);
> >
> > Should ereport() be used here instead?
>
> Yes, that is better. Modified it to ereport
>
> > Also, would it be better to add some context before %s, for example,
> > something like "received message from the primary:"? Without that,
> > users might mistakenly think the message originated from the local server.
>
> Yes, that makes sense. Updated accordingly.
>
> For dblink and postgres_fdw, create the dblink and postgres_fdw setup
> and create the below object to raise a notice:
> CREATE OR REPLACE FUNCTION emit_notice(msg text) RETURNS int AS $$
> BEGIN
>     RAISE NOTICE '%', msg;
>     RETURN 1;
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE VIEW my_notice_view AS SELECT 1 AS value;
>
> CREATE OR REPLACE RULE "_RETURN" AS
>     ON SELECT TO my_notice_view
>     DO INSTEAD
>     SELECT 1 AS value
>     FROM (SELECT emit_notice('NOTICE from _RETURN rule') AS dummy) s;
>
> -- Scenario to see the notice raised by remote server using dblink
> SELECT * FROM dblink('dbname=remotedb user=remoteuser',  'SELECT *
> FROM my_notice_view') AS t(value int);
>
> --  Scenario to see the notice raised by remote server using postgres_fdw
> IMPORT FOREIGN SCHEMA public LIMIT TO (my_notice_view) FROM SERVER
> foreign_server INTO public;
>
> The attached v2 version patch has the changes for the same.

Thanks for updating the patch!

+ /* Trim trailing newline for cleaner logs */
+ size_t len = strlen(message);
+
+ if (len > 0 && message[len - 1] == '\n')
+ ereport(LOG,
+ errmsg("Received message from publisher: %.*s",
+ (int) (len - 1), message));

Do we really need to trim the trailing newline? Are there actual
WARNING/NOTICE/INFO messages that include an unnecessary newline?

"Received" should be lowercase, i.e., "received" since the primary log message
should start with a lowercase letter, according to the Error Message
Style Guide [1].

The phrase "from publisher" could be misleading, since the sender might be
a primary in physical replication, not necessarily a logical publisher.
Maybe something like "received message via replication" would be clearer?
I'd like to hear others' thoughts on a better wording.

Regards,

[1] https://www.postgresql.org/docs/devel/error-style-guide.html

-- 
Fujii Masao

  5. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-16T10:45:49Z

    On Wed, 16 Jul 2025 at 09:09, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> >
> > Included it for dblink and fdw
>
> Thanks! It's better to submit each change for dblink and postgres_fdw
> as separate patches, and first focus on the patch that sets the notice
> processor for replication connections.

Modified

> > > +notice_processor(void *arg, const char *message)
> > > +{
> > > +       elog(LOG, "%s", message);
> > >
> > > Should ereport() be used here instead?
> >
> > Yes, that is better. Modified it to ereport
> >
> > > Also, would it be better to add some context before %s, for example,
> > > something like "received message from the primary:"? Without that,
> > > users might mistakenly think the message originated from the local server.
> >
> > Yes, that makes sense. Updated accordingly.
> >
> > For dblink and postgres_fdw, create the dblink and postgres_fdw setup
> > and create the below object to raise a notice:
> > CREATE OR REPLACE FUNCTION emit_notice(msg text) RETURNS int AS $$
> > BEGIN
> >     RAISE NOTICE '%', msg;
> >     RETURN 1;
> > END;
> > $$ LANGUAGE plpgsql;
> >
> > CREATE VIEW my_notice_view AS SELECT 1 AS value;
> >
> > CREATE OR REPLACE RULE "_RETURN" AS
> >     ON SELECT TO my_notice_view
> >     DO INSTEAD
> >     SELECT 1 AS value
> >     FROM (SELECT emit_notice('NOTICE from _RETURN rule') AS dummy) s;
> >
> > -- Scenario to see the notice raised by remote server using dblink
> > SELECT * FROM dblink('dbname=remotedb user=remoteuser',  'SELECT *
> > FROM my_notice_view') AS t(value int);
> >
> > --  Scenario to see the notice raised by remote server using postgres_fdw
> > IMPORT FOREIGN SCHEMA public LIMIT TO (my_notice_view) FROM SERVER
> > foreign_server INTO public;
> >
> > The attached v2 version patch has the changes for the same.
>
> Thanks for updating the patch!
>
> + /* Trim trailing newline for cleaner logs */
> + size_t len = strlen(message);
> +
> + if (len > 0 && message[len - 1] == '\n')
> + ereport(LOG,
> + errmsg("Received message from publisher: %.*s",
> + (int) (len - 1), message));
>
> Do we really need to trim the trailing newline? Are there actual
> WARNING/NOTICE/INFO messages that include an unnecessary newline?

If we don't trim the trailing newline, an extra blank line will appear
after the message is printed, like this:
2025-07-16 12:44:20.076 IST [534376] LOG:  logical replication table
synchronization worker for subscription "sub1", table "t2" has started
2025-07-16 12:44:20.294 IST [534374] LOG:  received message via
replication: WARNING:  could not remove directory
"pg_replslot/pg_16396_sync_16384_7527574647116269356.tmp"

2025-07-16 12:44:20.294 IST [534374] LOG:  logical replication table
synchronization worker for subscription "sub1", table "t1" has
finished

This occurs because a newline is appended to the message in
pqBuildErrorMessage3, which is called when the message is received in
pqGetErrorNotice3:
...
}
appendPQExpBufferChar(msg, '\n');
if (verbosity != PQERRORS_TERSE)
...

> "Received" should be lowercase, i.e., "received" since the primary log message
> should start with a lowercase letter, according to the Error Message
> Style Guide [1].

Modified

> The phrase "from publisher" could be misleading, since the sender might be
> a primary in physical replication, not necessarily a logical publisher.
> Maybe something like "received message via replication" would be clearer?
> I'd like to hear others' thoughts on a better wording.

Modified the message to "received message via replication." Will
update it further if we get any better wording.

The attached v3 version patch has the changes for the same.

Regards,
Vignesh

  6. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-16T16:00:08Z

    
On 2025/07/16 19:45, vignesh C wrote:
> If we don't trim the trailing newline, an extra blank line will appear
> after the message is printed, like this:
> 2025-07-16 12:44:20.076 IST [534376] LOG:  logical replication table
> synchronization worker for subscription "sub1", table "t2" has started
> 2025-07-16 12:44:20.294 IST [534374] LOG:  received message via
> replication: WARNING:  could not remove directory
> "pg_replslot/pg_16396_sync_16384_7527574647116269356.tmp"
> 
> 2025-07-16 12:44:20.294 IST [534374] LOG:  logical replication table
> synchronization worker for subscription "sub1", table "t1" has
> finished
> 
> This occurs because a newline is appended to the message in
> pqBuildErrorMessage3, which is called when the message is received in
> pqGetErrorNotice3:
> ...
> }
> appendPQExpBufferChar(msg, '\n');
> if (verbosity != PQERRORS_TERSE)
> ...

You're right, the message text passed to the notice processor includes
a trailing newline. I confirmed this is documented in the PQsetNoticeProcessor
docs [1], so I agree it's reasonable to trim the newline for cleaner log output.


Regarding the current implementation:
+       /* Trim trailing newline for cleaner logs */
+       size_t          len = strlen(message);
+
+       if (len > 0 && message[len - 1] == '\n')
+               ereport(LOG,
+                               errmsg("received message via replication: %.*s",
+                                          (int) (len - 1), message));
+       else
+               ereport(LOG,
+                               errmsg("received message via replication: %s", message));

To avoid repeating ereport(), how about refactoring it like this?
I think it's simpler and easier to read:

---------------------------------------
/*
  * Custom notice processor for replication connection.
  */
static void
notice_processor(void *arg, const char *message)
{
	int			len;

	/*
	 * Trim the trailing newline from the message text passed to the notice
	 * processor, as it always includes one, to produce cleaner log output.
	 */
	len = strlen(message);
	if (len > 0 && message[len - 1] == '\n')
		len--;

	ereport(LOG,
			errmsg("received message via replication: %.*s",
				   len, message));
}
---------------------------------------


Also, I suggest adding the prototype for notice_processor() in libpqwalreceiver.c:

---------------------------------------
/* Prototypes for private functions */
static void notice_processor(void *arg, const char *message);
static char *stringlist_to_identifierstr(PGconn *conn, List *strings);
---------------------------------------


Currently, notice_processor() is placed just before libpqrcv_connect(),
but I think it would be better to move it before stringlist_to_identifierstr(),
since the libpqwalreceiver.c seems to follow a structure where private
functions come after the libpqrcv_* functions.


+       PQsetNoticeProcessor(conn->streamConn, notice_processor, NULL);

It might be helpful to add a comment explaining the purpose, such as:

     Set a custom notice processor so that notice, warning, and similar messages
     received via the replication connection are logged in our preferred style,
     instead of just being printed to stderr.

Thought?

>> The phrase "from publisher" could be misleading, since the sender might be
>> a primary in physical replication, not necessarily a logical publisher.
>> Maybe something like "received message via replication" would be clearer?
>> I'd like to hear others' thoughts on a better wording.
> 
> Modified the message to "received message via replication." Will
> update it further if we get any better wording.

+1

Regards,

[1] https://www.postgresql.org/docs/devel/libpq-notice-processing.html

-- 
Fujii Masao
NTT DATA Japan Corporation

  7. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-17T05:32:30Z

    On Wed, 16 Jul 2025 at 21:30, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2025/07/16 19:45, vignesh C wrote:
> > If we don't trim the trailing newline, an extra blank line will appear
> > after the message is printed, like this:
> > 2025-07-16 12:44:20.076 IST [534376] LOG:  logical replication table
> > synchronization worker for subscription "sub1", table "t2" has started
> > 2025-07-16 12:44:20.294 IST [534374] LOG:  received message via
> > replication: WARNING:  could not remove directory
> > "pg_replslot/pg_16396_sync_16384_7527574647116269356.tmp"
> >
> > 2025-07-16 12:44:20.294 IST [534374] LOG:  logical replication table
> > synchronization worker for subscription "sub1", table "t1" has
> > finished
> >
> > This occurs because a newline is appended to the message in
> > pqBuildErrorMessage3, which is called when the message is received in
> > pqGetErrorNotice3:
> > ...
> > }
> > appendPQExpBufferChar(msg, '\n');
> > if (verbosity != PQERRORS_TERSE)
> > ...
>
> You're right, the message text passed to the notice processor includes
> a trailing newline. I confirmed this is documented in the PQsetNoticeProcessor
> docs [1], so I agree it's reasonable to trim the newline for cleaner log output.
>
>
> Regarding the current implementation:
> +       /* Trim trailing newline for cleaner logs */
> +       size_t          len = strlen(message);
> +
> +       if (len > 0 && message[len - 1] == '\n')
> +               ereport(LOG,
> +                               errmsg("received message via replication: %.*s",
> +                                          (int) (len - 1), message));
> +       else
> +               ereport(LOG,
> +                               errmsg("received message via replication: %s", message));
>
> To avoid repeating ereport(), how about refactoring it like this?
> I think it's simpler and easier to read:
>
> ---------------------------------------
> /*
>   * Custom notice processor for replication connection.
>   */
> static void
> notice_processor(void *arg, const char *message)
> {
>         int                     len;
>
>         /*
>          * Trim the trailing newline from the message text passed to the notice
>          * processor, as it always includes one, to produce cleaner log output.
>          */
>         len = strlen(message);
>         if (len > 0 && message[len - 1] == '\n')
>                 len--;
>
>         ereport(LOG,
>                         errmsg("received message via replication: %.*s",
>                                    len, message));
> }
> ---------------------------------------

Looks good, modified

> Also, I suggest adding the prototype for notice_processor() in libpqwalreceiver.c:
>
> ---------------------------------------
> /* Prototypes for private functions */
> static void notice_processor(void *arg, const char *message);
> static char *stringlist_to_identifierstr(PGconn *conn, List *strings);
> ---------------------------------------
>
>
> Currently, notice_processor() is placed just before libpqrcv_connect(),
> but I think it would be better to move it before stringlist_to_identifierstr(),
> since the libpqwalreceiver.c seems to follow a structure where private
> functions come after the libpqrcv_* functions.

Modified

>
> +       PQsetNoticeProcessor(conn->streamConn, notice_processor, NULL);
>
> It might be helpful to add a comment explaining the purpose, such as:
>
>      Set a custom notice processor so that notice, warning, and similar messages
>      received via the replication connection are logged in our preferred style,
>      instead of just being printed to stderr.

Modified

The attached v4 version patch has the changes for the same.

Regards,
Vignesh

  8. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-17T05:47:52Z

    Hi,

Shouldn't we be using a notice receiver rather than a notice processor?

-- 
Álvaro Herrera

  9. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-17T08:05:27Z

    On Thu, 17 Jul 2025 at 11:18, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> Hi,
>
> Shouldn't we be using a notice receiver rather than a notice processor?

I saw the following comment in code regarding PQsetNoticeProcessor
should be deprecated:
/*
 * The default notice message receiver just gets the standard notice text
 * and sends it to the notice processor.  This two-level setup exists
 * mostly for backwards compatibility; perhaps we should deprecate use of
 * PQsetNoticeProcessor?
 */

So I changed it to PQsetNoticeReceiver. The attached v5 version patch
has the changes for the same.

Regards,
Vignesh

  10. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-17T14:34:32Z

    
On 2025/07/17 17:05, vignesh C wrote:
> On Thu, 17 Jul 2025 at 11:18, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>
>> Hi,
>>
>> Shouldn't we be using a notice receiver rather than a notice processor?
> 
> I saw the following comment in code regarding PQsetNoticeProcessor
> should be deprecated:
> /*
>   * The default notice message receiver just gets the standard notice text
>   * and sends it to the notice processor.  This two-level setup exists
>   * mostly for backwards compatibility; perhaps we should deprecate use of
>   * PQsetNoticeProcessor?
>   */
> 
> So I changed it to PQsetNoticeReceiver.

+1

As a side note, I'd like to clarify in the source comments or documentation
that PQsetNoticeProcessor() exists mainly for backward compatibility,
and PQsetNoticeReceiver() should be preferred. But that's a separate topic
from this patch.


> The attached v5 version patch
> has the changes for the same.

Thanks for updating the patches!


+static void notice_receiver(void *arg, const PGresult *result);

For consistency with the typedef for PQnoticeReceiver, it would be better
to name the argument "res" instead of "result".


+	 * Set a custom notice receiver so that NOTICEs, WARNINGs, and similar

The "s" in "NOTICEs" and "WARNINGs" isn't needed.


+	 * Trim the trailing newline from the message text passed to the notice
+	 * receiver, as it always includes one, to produce cleaner log output.


"message text passed to the notice receiver" should be changed to
"message text returned by PQresultErrorMessage()"?

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  11. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-17T15:52:31Z

    
On 2025/07/17 23:34, Fujii Masao wrote:
>> The attached v5 version patch
>> has the changes for the same.
> 
> Thanks for updating the patches!

The current patches add nearly identical notice_receiver functions
in multiple places such as libpqwalreceiver.c and elsewhere. To avoid
duplicating the same logic, could we define a shared notice receiver
function in a common file, like libpq-be-fe-helpers.h, and use it in
all three locations?

--------------------
static inline void
libpqsrv_notice_receiver(void *arg, const PGresult *res)
{
	char	   *message;
	int			len;
	char	   *prefix = (char *) arg;

	/*
	 * Trim the trailing newline from the message text returned from
	 * PQresultErrorMessage(), as it always includes one, to produce
	 * cleaner log output.
	 */
	message = PQresultErrorMessage(res);
	len = strlen(message);
	if (len > 0 && message[len - 1] == '\n')
		len--;

	ereport(LOG,
			errmsg("received message %s: %.*s", prefix, len, message));
}
--------------------

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  12. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-17T16:15:51Z

    On 2025-Jul-18, Fujii Masao wrote:

> The current patches add nearly identical notice_receiver functions
> in multiple places such as libpqwalreceiver.c and elsewhere. To avoid
> duplicating the same logic, could we define a shared notice receiver
> function in a common file, like libpq-be-fe-helpers.h, and use it in
> all three locations?

I like the idea of reducing duplication.  I don't like the "prefix"
as proposed though, because it's not going to work well for translation
(string building) -- I'd rather pass the entire phrase from caller, so
that the translator has one piece to translate which lives in the module
that emits it.  I think I'd do something like

    ereport(LOG,
            errmsg_internal("%s: %.*s",
	                    _(prefix), len, message));

and see to it that each caller uses gettext_noop() around the string
they pass as "arg", for gettext to collect.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  13. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-17T23:16:29Z

    
On 2025/07/18 1:15, Álvaro Herrera wrote:
> On 2025-Jul-18, Fujii Masao wrote:
> 
>> The current patches add nearly identical notice_receiver functions
>> in multiple places such as libpqwalreceiver.c and elsewhere. To avoid
>> duplicating the same logic, could we define a shared notice receiver
>> function in a common file, like libpq-be-fe-helpers.h, and use it in
>> all three locations?
> 
> I like the idea of reducing duplication.  I don't like the "prefix"
> as proposed though, because it's not going to work well for translation
> (string building) -- I'd rather pass the entire phrase from caller, so
> that the translator has one piece to translate which lives in the module
> that emits it.  I think I'd do something like
> 
>      ereport(LOG,
>              errmsg_internal("%s: %.*s",
> 	                    _(prefix), len, message));
> 
> and see to it that each caller uses gettext_noop() around the string
> they pass as "arg", for gettext to collect.

Agreed. Thanks!

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  14. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-18T17:06:15Z

    On Fri, 18 Jul 2025 at 04:46, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2025/07/18 1:15, Álvaro Herrera wrote:
> > On 2025-Jul-18, Fujii Masao wrote:
> >
> >> The current patches add nearly identical notice_receiver functions
> >> in multiple places such as libpqwalreceiver.c and elsewhere. To avoid
> >> duplicating the same logic, could we define a shared notice receiver
> >> function in a common file, like libpq-be-fe-helpers.h, and use it in
> >> all three locations?
> >
> > I like the idea of reducing duplication.  I don't like the "prefix"
> > as proposed though, because it's not going to work well for translation
> > (string building) -- I'd rather pass the entire phrase from caller, so
> > that the translator has one piece to translate which lives in the module
> > that emits it.  I think I'd do something like
> >
> >      ereport(LOG,
> >              errmsg_internal("%s: %.*s",
> >                           _(prefix), len, message));
> >
> > and see to it that each caller uses gettext_noop() around the string
> > they pass as "arg", for gettext to collect.
>
> Agreed. Thanks!

The attached v6 version patch has the changes for these comments.

Regards,
Vignesh

  15. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-18T20:09:33Z

    On 2025-Jul-18, vignesh C wrote:

> diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
> index 304f3c20f83..c1ce6f33436 100644
> --- a/contrib/postgres_fdw/connection.c
> +++ b/contrib/postgres_fdw/connection.c
> @@ -625,6 +625,9 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
>  							server->servername),
>  					 errdetail_internal("%s", pchomp(PQerrorMessage(conn)))));
>  
> +		PQsetNoticeReceiver(conn, libpqsrv_notice_receiver,
> +							"received message via remote connection");
> +

These need to be:

    PQsetNoticeReceiver(conn, libpqsrv_notice_receiver,
                        gettext_noop("received message via remote connection"));

so that the strings are picked up by gettext.


Also, I don't see why there are three patches here instead of just one.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

  16. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-19T06:45:31Z

    On Sat, 19 Jul 2025 at 01:39, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Jul-18, vignesh C wrote:
>
> > diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
> > index 304f3c20f83..c1ce6f33436 100644
> > --- a/contrib/postgres_fdw/connection.c
> > +++ b/contrib/postgres_fdw/connection.c
> > @@ -625,6 +625,9 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
> >                                                       server->servername),
> >                                        errdetail_internal("%s", pchomp(PQerrorMessage(conn)))));
> >
> > +             PQsetNoticeReceiver(conn, libpqsrv_notice_receiver,
> > +                                                     "received message via remote connection");
> > +
>
> These need to be:
>
>     PQsetNoticeReceiver(conn, libpqsrv_notice_receiver,
>                         gettext_noop("received message via remote connection"));
>
> so that the strings are picked up by gettext.

Modified

> Also, I don't see why there are three patches here instead of just one.

Earlier we thought to commit replication changes function firstlly and
then commit dblink and fdw changes, but now that we are using a common
notice receiver function. I feel it can be a single patch. Merged the
patches. The attached v7 version patch has the changes for the same.

Regards,
Vignesh

  17. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-21T01:14:43Z

    
On 2025/07/19 15:45, vignesh C wrote:
>> Also, I don't see why there are three patches here instead of just one.
> 
> Earlier we thought to commit replication changes function firstlly and
> then commit dblink and fdw changes, but now that we are using a common
> notice receiver function. I feel it can be a single patch.

Yes, I initially suggested splitting the patches for that reason,
but I agree it's better to merge them into a single patch now that
we're using a shared custom notice receiver.


> Merged the
> patches. The attached v7 version patch has the changes for the same.

Thanks for updating the patch! It looks good to me, except for one minor point:

  static inline PGresult *libpqsrv_get_result(PGconn *conn, uint32 wait_event_info);
+static inline void libpqsrv_notice_receiver(void *arg, const PGresult *res);

This prototype is only needed if the function is used earlier in libpq-be-fe-helpers.h,
but that's not the case here, so I don't think it's necessary.

Unless there are objections, I'll remove that prototype and commit the patch.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  18. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-21T08:51:14Z

    On 2025-Jul-21, Fujii Masao wrote:

> Thanks for updating the patch! It looks good to me, except for one minor point:
> 
>  static inline PGresult *libpqsrv_get_result(PGconn *conn, uint32 wait_event_info);
> +static inline void libpqsrv_notice_receiver(void *arg, const PGresult *res);
> 
> This prototype is only needed if the function is used earlier in libpq-be-fe-helpers.h,
> but that's not the case here, so I don't think it's necessary.
> 
> Unless there are objections, I'll remove that prototype and commit the patch.

LGTM.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
 algunas personas nos parecen brillantes un minuto antes
 de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

  19. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-22T05:29:51Z

    On Mon, Jul 21, 2025 at 5:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Jul-21, Fujii Masao wrote:
>
> > Thanks for updating the patch! It looks good to me, except for one minor point:
> >
> >  static inline PGresult *libpqsrv_get_result(PGconn *conn, uint32 wait_event_info);
> > +static inline void libpqsrv_notice_receiver(void *arg, const PGresult *res);
> >
> > This prototype is only needed if the function is used earlier in libpq-be-fe-helpers.h,
> > but that's not the case here, so I don't think it's necessary.
> >
> > Unless there are objections, I'll remove that prototype and commit the patch.
>
> LGTM.

I've pushed the patch. Thanks!

Regards,

-- 
Fujii Masao

  20. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-22T06:24:10Z

    
On 2025/07/22 14:29, Fujii Masao wrote:
> On Mon, Jul 21, 2025 at 5:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>
>> On 2025-Jul-21, Fujii Masao wrote:
>>
>>> Thanks for updating the patch! It looks good to me, except for one minor point:
>>>
>>>   static inline PGresult *libpqsrv_get_result(PGconn *conn, uint32 wait_event_info);
>>> +static inline void libpqsrv_notice_receiver(void *arg, const PGresult *res);
>>>
>>> This prototype is only needed if the function is used earlier in libpq-be-fe-helpers.h,
>>> but that's not the case here, so I don't think it's necessary.
>>>
>>> Unless there are objections, I'll remove that prototype and commit the patch.
>>
>> LGTM.
> 
> I've pushed the patch. Thanks!

The buildfarm member indri reported the following error, which seems related to
the recent changes in dblink. I'll investigate this later.

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=indri&dt=2025-07-22%2006%3A11%3A16&stg=make-contrib

/Library/Developer/CommandLineTools/usr/bin/make -C intagg all
make[1]: Nothing to be done for `all'.
/opt/local/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -fexcess-precision=standard -mno-outline-atomics -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-format-truncation -O2  -I. -I. -I../../src/include -I/opt/local/include   -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.5.sdk  -I/opt/local/include/libxml2 -I/opt/local/include -I/opt/local/include -I/opt/local/include  -I/opt/local/include -flto=thin -emit-llvm -c -o fuzzystrmatch.bc fuzzystrmatch.c
Undefined symbols for architecture arm64:
   "_libintl_gettext", referenced from:
       _libpqsrv_notice_receiver in dblink.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [dblink.dylib] Error 1
make: *** [all-dblink-recurse] Error 2
make: *** Waiting for unfinished jobs....

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  21. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-22T07:18:22Z

    On 2025-07-22, Fujii Masao wrote:
 
> The buildfarm member indri reported the following error, which seems related to
> the recent changes in dblink. I'll investigate this later.

Ah yes — contrib doesn't have gettext support and macOS doesn't like that. Maybe removing the gettext_noop calls in contrib is the easiest solution. I think another option is to add -lintl to the link line but that's probably messier, and without advantage since we still won't have translation support.

-- 
Álvaro Herrera

  22. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-22T09:24:03Z

    On 2025-Jul-22, Álvaro Herrera wrote:

> On 2025-07-22, Fujii Masao wrote:
>  
> > The buildfarm member indri reported the following error, which seems related to
> > the recent changes in dblink. I'll investigate this later.
> 
> Ah yes — contrib doesn't have gettext support and macOS doesn't like
> that. Maybe removing the gettext_noop calls in contrib is the easiest
> solution. I think another option is to add -lintl to the link line but
> that's probably messier, and without advantage since we still won't
> have translation support.

I wonder why doesn't contrib/basic_archive fail though.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

  23. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-22T11:37:54Z

    On Tue, Jul 22, 2025 at 6:24 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Jul-22, Álvaro Herrera wrote:
>
> > On 2025-07-22, Fujii Masao wrote:
> >
> > > The buildfarm member indri reported the following error, which seems related to
> > > the recent changes in dblink. I'll investigate this later.
> >
> > Ah yes — contrib doesn't have gettext support and macOS doesn't like
> > that. Maybe removing the gettext_noop calls in contrib is the easiest
> > solution. I think another option is to add -lintl to the link line but
> > that's probably messier, and without advantage since we still won't
> > have translation support.
>
> I wonder why doesn't contrib/basic_archive fail though.

Is the issue caused by the use of _() in libpq-be-fe-helpers.h,
instead of using gettext_noop() in dblink.c? At least in my test
environment, the build error disappeared after applying
the following change:

- errmsg_internal("%s: %.*s", _(prefix), len, message));
+ errmsg_internal("%s: %.*s", prefix, len, message));

Regards,

-- 
Fujii Masao

  24. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-22T11:47:09Z

    On 2025-Jul-22, Fujii Masao wrote:

> Is the issue caused by the use of _() in libpq-be-fe-helpers.h,
> instead of using gettext_noop() in dblink.c? At least in my test
> environment, the build error disappeared after applying
> the following change:
> 
> - errmsg_internal("%s: %.*s", _(prefix), len, message));
> + errmsg_internal("%s: %.*s", prefix, len, message));

Oh yeah, I should have remembered this -- see commit 213c959a294d.  Feel
free to do away with the whole translation thing ... doesn't seem worth
spending more time on it.

We should still remove all gettext_noop() markers in contrib :-)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  25. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-22T12:21:10Z

    On Tue, Jul 22, 2025 at 8:47 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Jul-22, Fujii Masao wrote:
>
> > Is the issue caused by the use of _() in libpq-be-fe-helpers.h,
> > instead of using gettext_noop() in dblink.c? At least in my test
> > environment, the build error disappeared after applying
> > the following change:
> >
> > - errmsg_internal("%s: %.*s", _(prefix), len, message));
> > + errmsg_internal("%s: %.*s", prefix, len, message));
>
> Oh yeah, I should have remembered this -- see commit 213c959a294d.  Feel
> free to do away with the whole translation thing ... doesn't seem worth
> spending more time on it.

Yes! The attached patch removes the translation marker and the
gettext_noop() calls I recently added.

> We should still remove all gettext_noop() markers in contrib :-)

You mean to remove gettext_noop() also from basic_archive.c?

Regards,

-- 
Fujii Masao

  26. Re: Log prefix missing for subscriber log messages received from publisher

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-22T12:49:35Z

    On 2025-Jul-22, Fujii Masao wrote:

> On Tue, Jul 22, 2025 at 8:47 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> > Oh yeah, I should have remembered this -- see commit 213c959a294d.  Feel
> > free to do away with the whole translation thing ... doesn't seem worth
> > spending more time on it.
> 
> Yes! The attached patch removes the translation marker and the
> gettext_noop() calls I recently added.

LGTM.

> > We should still remove all gettext_noop() markers in contrib :-)
> 
> You mean to remove gettext_noop() also from basic_archive.c?

Yes, it's useless and misleading.  Feel free to do it in the same commit ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The ability of users to misuse tools is, of course, legendary" (David Steele)
https://postgr.es/m/11b38a96-6ded-4668-b772-40f992132797@pgmasters.net

  27. Re: Log prefix missing for subscriber log messages received from publisher

    Andrei Lepikhov <lepihov@gmail.com> — 2025-07-22T13:08:37Z

    On 22/7/2025 14:49, Álvaro Herrera wrote:
> On 2025-Jul-22, Fujii Masao wrote:
>>> We should still remove all gettext_noop() markers in contrib :-)
>>
>> You mean to remove gettext_noop() also from basic_archive.c?
> 
> Yes, it's useless and misleading.  Feel free to do it in the same commit ...
> 
Just for the record. This patch obviously resolves my issue [1] too ...

[1] 
https://www.postgresql.org/message-id/39ac724e-58c8-4661-9e88-cb4ee97cb00e@gmail.com

-- 
regards, Andrei Lepikhov

  28. Re: Log prefix missing for subscriber log messages received from publisher

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-22T13:13:31Z

    On Tue, Jul 22, 2025 at 9:49 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Jul-22, Fujii Masao wrote:
>
> > On Tue, Jul 22, 2025 at 8:47 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> > > Oh yeah, I should have remembered this -- see commit 213c959a294d.  Feel
> > > free to do away with the whole translation thing ... doesn't seem worth
> > > spending more time on it.
> >
> > Yes! The attached patch removes the translation marker and the
> > gettext_noop() calls I recently added.
>
> LGTM.

Thanks for the review!


> > > We should still remove all gettext_noop() markers in contrib :-)
> >
> > You mean to remove gettext_noop() also from basic_archive.c?
>
> Yes, it's useless and misleading.  Feel free to do it in the same commit ...

I've included that change in the patch and pushed it. Thanks!

Regards,

-- 
Fujii Masao

  29. Re: Log prefix missing for subscriber log messages received from publisher

    vignesh C <vignesh21@gmail.com> — 2025-07-23T03:46:22Z

    On Tue, 22 Jul 2025 at 18:43, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Jul 22, 2025 at 9:49 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > On 2025-Jul-22, Fujii Masao wrote:
> >
> > > On Tue, Jul 22, 2025 at 8:47 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > > > Oh yeah, I should have remembered this -- see commit 213c959a294d.  Feel
> > > > free to do away with the whole translation thing ... doesn't seem worth
> > > > spending more time on it.
> > >
> > > Yes! The attached patch removes the translation marker and the
> > > gettext_noop() calls I recently added.
> >
> > LGTM.
>
> Thanks for the review!
>
>
> > > > We should still remove all gettext_noop() markers in contrib :-)
> > >
> > > You mean to remove gettext_noop() also from basic_archive.c?
> >
> > Yes, it's useless and misleading.  Feel free to do it in the same commit ...
>
> I've included that change in the patch and pushed it. Thanks!

Thanks Fuji-san for handling and pushing this issue.

Regards,
Vignesh