public inbox for [email protected]
help / color / mirror / Atom feedFrom: shveta malik <[email protected]>
To: Kuroda, Hayato/黒田 隼人 <[email protected]>
Cc: Gyan Sreejith <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: Wed, 18 Mar 2026 15:33:06 +0530
Message-ID: <CAJpy0uCfBWLiCgcnLEbwgjcVKpV4xYmxYCfs-nqOFX1mCFfyUA@mail.gmail.com> (raw)
In-Reply-To: <OS9PR01MB12149C7DE09F13C3BCBD9357DF54EA@OS9PR01MB12149.jpnprd01.prod.outlook.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>
<CANhcyEWaCV7B54oOhuE_BhVCF887h23jKmkHDDWvJRKZ=pBbBQ@mail.gmail.com>
<CAEqnbaVRQwPNYqXVJu3YmPiS0d-kG00H81fEBR8ZULP0GJrp7Q@mail.gmail.com>
<OS9PR01MB12149C7DE09F13C3BCBD9357DF54EA@OS9PR01MB12149.jpnprd01.prod.outlook.com>
On Wed, Mar 18, 2026 at 11:43 AM Kuroda, Hayato/黒田 隼人
<[email protected]> wrote:
>
> Dear Gyan,
>
> Thanks for updating. Not sure you have addressed my comments as well,
> but I reviewed again.
>
> 01.
> ```
> +#undef pg_log_warning
> +#define pg_log_warning(...) \
> + if (internal_log_file_fp != NULL) \
> + internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
> + pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__)
> ```
>
> IIUC it's more common that to have do {...} while if the marco function has
> several lines.
>
> 02.
> ```
> +#undef pg_log_info
> +#define pg_log_info(...) do { \
> + if (internal_log_file_fp != NULL) \
> + internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
> + else \
> + pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
> +} while(0)
> ```
>
> Missing update?
>
> 03.
>
> I think pg_log_error faimily should be also output on the file, but it misses.
>
> 04.
> ```
> +static void
> +make_dir(const char *dir)
> ```
>
> I think this is not needed, we can follow the approach done on the pg_upgrade.
> The message "directory %s created" may be removed, but I think it's ok.
>
> 05.
> ```
> + /* Build timestamp directory path */
> + len = snprintf(timestamp_dir, MAXPGPATH, "%s/%s", log_dir, timestamp);
> +
> + if (len >= MAXPGPATH)
> + pg_fatal("directory path for log files, %s/%s, is too long",
> + log_dir, timestamp);
> ```
>
> These checks should be done before creating the base directory.
>
> 06.
> ```
> +static char *log_timestamp = NULL; /* Timestamp to be used in all log file
> + * names */
> ```
>
> I feel it's more efficient to directly have the directory where log exists.
>
> 07.
> ```
> + if (opt.log_dir != NULL)
> + {
> + char *internal_log_file;
> +
> + 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 still think it can be put bit later; after validating simple messages.
> How about others?
+1, otherwise it will simply create a directory and error out after
that if the command is wrong. We can avoid creating a directory.
Patch has a compilation issue, logfile_open() needs a second argument in main().
If we fix that, it gives a segmentation fault in make_output_dirs() as
logdir is not allocated, earlier it was an array, now a NULL pointer.
Kuroda-san is going to post a new patch soon.
thanks
Shveta
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], [email protected], [email protected]
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
In-Reply-To: <CAJpy0uCfBWLiCgcnLEbwgjcVKpV4xYmxYCfs-nqOFX1mCFfyUA@mail.gmail.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