RE: [Proposal] Adding Log File Capability to pg_createsubscriber

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'Gyan Sreejith' <gyan(dot)sreejith(at)gmail(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Euler Taveira <euler(at)eulerto(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com>
Subject: RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: 2026-03-24 10:06:08
Message-ID: OS9PR01MB1214926DB01BF19C09368188EF548A@OS9PR01MB12149.jpnprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Gyan,

Thanks for updating the patch!

> I haven't changed the output of pg_log_generic_v() yet. Shall I add the prefix to the output?

Since pg_log_generic_v() is the common function used even by others, we should
not fix it. Let's fix internal_log_file_write() instead.

Further comments:

01.
```
+ setvbuf(fh, NULL, PG_IOLBF, 0);
```

I think setvbuf is not needed.

IIUC, setvbuf() controls when messages like printf() are flushed from to the
stdout/stderr/files and so on. In logfile_open(), we are setting that output
messages should be flushed once per new lines. Syslogger process does not call
fflush() frequently thus it seems to be controlled by the library call.
However, pg_createsubscriber always does the fflush() every time, no need to
specify when the buffered strings are flushed on the file.

Also note that pg_upgrade does not call the library function.

02.
```
+ /* use CRLF line endings on Windows */
+ _setmode(_fileno(fh), _O_TEXT);
```

_setmode() is called in win32 system. According to [2] and code comment, it's
used to open the file as TEXT mode for unifying the new-line character to CR-LF.

However, postgres has already been done in fopen(). The function is overwritten
to pgwin32_fopen()->pgwin32_open(), and there we already specify to open with
the _O_TEXT mode. I think no need to do in the pg_createsubscriber.c.

Also note that pg_upgrade does not call the library function.

03.
If we fix above and comments from Amit [1], logfile_open() is not completely
different function from the syslogger.c. We can remove the comment atop the
function.

04.
As I told in [2], no need to do fclose() the file in the cleanup function.

I created a top-up patch set which addressed all comments from me and others.
See attached.

[1]: https://www.postgresql.org/message-id/CAA4eK1KHZraUvb5U5QdMEqiCAWhGPAnStun88ZR0qFunRZu9uQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/OS9PR01MB12149F1AD9C79A2644753A18AF548A%40OS9PR01MB12149.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Attachment Content-Type Size
v18-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch application/octet-stream 16.5 KB
v18-0002-Address-comments-from-Chao-Amit-and-Kuroda.patch application/octet-stream 4.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Yugo Nagata 2026-03-24 10:25:37 Re: Add a berief general comment on BTScanInsertData's nextkey and backward
Previous Message Andy Fan 2026-03-24 10:04:43 raise ERROR between EndPrepare and PostPrepare_Locks causes ROLLBACK 2pc PAINC