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