public inbox for [email protected]
help / color / mirror / Atom feedFrom: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Gyan Sreejith' <[email protected]>
To: vignesh C <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: Tue, 17 Mar 2026 12:18:41 +0000
Message-ID: <OS9PR01MB12149483AFCE393ACB7CD5D7AF541A@OS9PR01MB12149.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAEqnbaWxYp7TbnhKfkjW8c=jRPARsQ1on86bt_pcqvK9vCtLhQ@mail.gmail.com>
References: <CAEqnbaUthOQARV1dscGvB_EsqC-YfxiM6rWkVDHc+G+f4oSUHw@mail.gmail.com>
<CAHut+PvizcpeHA1Twf_hwe=wANQ1LV5zY6_q+39gTFJc7+bCKg@mail.gmail.com>
<CAEqnbaVNZYbB_YufchM49d=XC1ZGrVV8ikCPmGotWoCZASY3Uw@mail.gmail.com>
<CAEqnbaV8QMbXZtt25QtGUPAaQZnD-B0HniFSroapMh+QTZgKsQ@mail.gmail.com>
<OSCPR01MB14966FD0961F512B29BD46D6BF5AAA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CALDaNm0gX2D6fD5yur-R5gagA7+AfLmLZU5Z8+tgjo61b-Y01w@mail.gmail.com>
<[email protected]>
<CAA4eK1+2PYU-y2weY_zFeysP8Ux45iBNdaYMZuJtMTqXwCxawQ@mail.gmail.com>
<CAEqnbaVVp_g1m1nhOBZjtiAz5W-xwPQCmWH4hjYYehA+ktYg9Q@mail.gmail.com>
<CALDaNm2T__Uha1fn274bP3jKDSwm37ewWYvA+vOo75FTT2t3SA@mail.gmail.com>
<CALDaNm1fF6yBa2PCv9gO_MVcjKLQFgi9eAL=g5cU=xtbOr=qZw@mail.gmail.com>
<CAEqnbaUa27R9OcVDHkNuwsZ1uCpy8sTXpH78N1ug0kj60mnchw@mail.gmail.com>
<CALDaNm1Mj+eNVETmYAYd4ojrbTbQU5iCX20Fso24r2VuXF69AQ@mail.gmail.com>
<CAEqnbaUWSPGHLL2nuyNHVKU0TB7uGBx9w0NvnisWFyf5TtwRTQ@mail.gmail.com>
<CALDaNm3Dj7WRdfD+fQ86XzS07vewaFSWx--kB5689_sC7rt1Uw@mail.gmail.com>
<CAEqnbaVgFU2Pr=xhhDmA=sK7XPBDBxECovqbSht91ZbHmnteUg@mail.gmail.com>
<CALDaNm17ujrJ2xHyz6r81WAiFUs38RcT8D5ebdxssASpGko0HA@mail.gmail.com>
<CAEqnbaUbVgGMXBNFbk=sqFJT=_TkAUYGYsFHJKFw5K492M_B=A@mail.gmail.com>
<CALDaNm2SYq_dO7qXHn+BnxmxvmBupH6tofUnx4DKC09h5xfAyQ@mail.gmail.com>
<CAEqnbaWjoSby+_FQOKqTiDwf9AsWVjcGzRn-BQtdivC8xn0ADw@mail.gmail.com>
<[email protected]>
<CAA4eK1+jd6zH-LL8FOrw01chrNetRoZGKXSqV7GNo_--UXYc_w@mail.gmail.com>
<CAEqnbaU50vLy031AbvmfXJ3_qv9iS4pVaMdmsnmpF87r=EqW7Q@mail.gmail.com>
<CALDaNm1P5pZ2tTcaVRvjUAXLAyT7S18GPwp-4bc0xKXKZoXi0w@mail.gmail.com>
<CAEqnbaWxYp7TbnhKfkjW8c=jRPARsQ1on86bt_pcqvK9vCtLhQ@mail.gmail.com>
Dear Gayn,
Thanks for updating the patch. I resumed reviewing.
01.
```
+static void
+ internal_log_file_write(const char *format,...) __attribute__((format(printf, 1, 2)));
```
pg_attribute_printf seems to be used in postgres.
02.
```
+static void
+make_dir(char *dir)
```
Since there are only two callers, I think no need to introduce the funciton.
E.g, make_outpudirs in pg_upgrade does mkdir() four times.
03.
```
#undef pg_log_debug
#define pg_log_debug(...) do{\
if (internal_log_file_fp != NULL) \
internal_log_file_write(__VA_ARGS__); \
else \
if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \
} while(0)
```
The patch ignores setting of log level if -l is specified. By default only
warnings/errors/fatals should be output, but even debug messages are output with
-l option. Checking logic is in pg_log_generic()->pg_log_generic_v() but we
cannot reach if internal_log_file_fp is set.
Also, are there any examples to undefine these macros? It's bit surprising for
me; I prefer some inline functions instead.
04.
```
+ case 'l':
+ opt.log_dir = pg_strdup(optarg);
+ canonicalize_path(opt.log_dir);
+ make_output_dirs(opt.log_dir);
+ internal_log_file = psprintf("%s/%s/%s.log", opt.log_dir, log_timestamp, INTERNAL_LOG_FILE_NAME);
+ if ((internal_log_file_fp = logfile_open(internal_log_file, "a")) == NULL)
+ pg_fatal("could not open log file \"%s\": %m", internal_log_file);
```
I feel creating the log file is too early, otherwise the issue Shveta raised the
at #2 could happen [1].
I feel it's OK putting after the if (opt.pub_conninfo_str == NULL). Because
there is the first place where call pg_log_info, after doing very simple validation.
05.
Let me confirm one point. IIUC, freopen() can be used to replace the stderr to
the file, and this may able to remove all #undef macros. The downside of this
approach is that even ERROR/FATAL messages would be on the file and nothing
appears on the terminal. See DebugFileOpen() the example.
Do you think that we may not use freopen() as-is? Do others have variants?
06.
I added this thread and the cfbot cannot accept your patch [2]. Please fix.
[1]: https://www.postgresql.org/message-id/CAJpy0uBPvz6S9VE8sLYmoju4BGYh94uks%2BUTocPdD094xqmZ2w%40mail.g...
[2]: https://commitfest.postgresql.org/patch/6592/
Best regards,
Hayato Kuroda
FUJITSU LIMITED
view thread (65+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: [Proposal] Adding Log File Capability to pg_createsubscriber
In-Reply-To: <OS9PR01MB12149483AFCE393ACB7CD5D7AF541A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox