Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
pg_createsubscriber: Don't use MAXPGPATH
- 99b726ac4894 19 (unreleased) landed
-
pg_createsubscriber: Remove separate logfile_open() function
- f5528b90b411 19 (unreleased) landed
-
pg_createsubscriber: Use logging.c log file callback
- 847336ba53af 19 (unreleased) landed
-
Add log file support to logging.c
- 41237556f8c5 19 (unreleased) landed
-
pg_createsubscriber: Add -l/--logdir option to redirect output to files.
- 6b5b7eae3ae6 19 (unreleased) landed
-
pg_createsubscriber: Introduce module-specific logging functions.
- d6628a5ea0a5 19 (unreleased) landed
-
Fix another buglet in archive_waldump.c.
- 69c57466a752 19 (unreleased) cited
-
[Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2025-12-09T22:16:40Z
Background: - pg_createsubscriber currently outputs all messages (internal validation messages, standby server start/stop logs, recovery progress output, and output from utilities) directly to the console. As a result, users may find debugging and handling errors difficult. It would be more convenient if messages were separated and stored in different log files. There is already a similar implementation in pg_upgrade. Proposed Solution: - Based on issues mentioned previously, I would like to propose a new argument -l <logdir> which can be specified for pg_createsubscriber. Using it would create the following log files: - logdir/pg_createsubscriber_server.log which captures all logs related to starting and stopping the standby server. - logdir/pg_createsubscriber_resetwal.log which captures the output of pg_resetwal - logdir/pg_createsubscriber_internal.log which captures internal diagnostic output from pg_createsubscriber (validations, checks, etc.) Overall, this proposed solution could make the pg_createsubscriber command output messages more organized. The command would be easier to use as users will only have to read individual log files rather than parse through lots of possibly irrelevant output messages. I have attached the patch for this change. Special thanks to Vignesh C. for his offlist guidance on this project. Regards, Gyan Sreejith -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Smith <smithpb2250@gmail.com> — 2025-12-11T07:29:07Z
On Wed, Dec 10, 2025 at 9:17 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Background: > > pg_createsubscriber currently outputs all messages (internal validation messages, standby server start/stop logs, recovery progress output, and output from utilities) directly to the console. As a result, users may find debugging and handling errors difficult. It would be more convenient if messages were separated and stored in different log files. There is already a similar implementation in pg_upgrade. > > Proposed Solution: > > Based on issues mentioned previously, I would like to propose a new argument -l <logdir> which can be specified for pg_createsubscriber. Using it would create the following log files: > > logdir/pg_createsubscriber_server.log which captures all logs related to starting and stopping the standby server. > > logdir/pg_createsubscriber_resetwal.log which captures the output of pg_resetwal > > logdir/pg_createsubscriber_internal.log which captures internal diagnostic output from pg_createsubscriber (validations, checks, etc.) > > Overall, this proposed solution could make the pg_createsubscriber command output messages more organized. The command would be easier to use as users will only have to read individual log files rather than parse through lots of possibly irrelevant output messages. I have attached the patch for this change. > > Special thanks to Vignesh C. for his offlist guidance on this project. > > > Regards, Gyan Sreejith > Hi Gyan. I haven't yet looked at this patch in any detail, but here are some quick comments: ====== 1. + printf(_(" -l, --logdir=LOGDIR location for the new log directory\n")); The patch is missing SGML docs updates for pg_createsubscriber new option, and any explanation of the split of logfiles. 2. I might be mistaken, but IIUC it seems the splitting of the logfile only works when --logdir is specified. Is that correct? Why should --logdir have any side-effect other than assigning the log destination folder? ====== Kind Regards, Peter Smith. Fujitsu Australia -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2025-12-12T01:33:15Z
Thanks for the feedback, Peter. I am currently working on the SGML docs update, and will promptly get back with an update. For your second point, currently, all output goes directly to the console. I thought it made more sense to break it up into multiple files depending on what was being invoked. Do you have another opinion? Thank you once again, Gyan Sreejith On Thu, Dec 11, 2025 at 2:29 AM Peter Smith <smithpb2250@gmail.com> wrote: > On Wed, Dec 10, 2025 at 9:17 AM Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > Background: > > > > pg_createsubscriber currently outputs all messages (internal validation > messages, standby server start/stop logs, recovery progress output, and > output from utilities) directly to the console. As a result, users may find > debugging and handling errors difficult. It would be more convenient if > messages were separated and stored in different log files. There is already > a similar implementation in pg_upgrade. > > > > Proposed Solution: > > > > Based on issues mentioned previously, I would like to propose a new > argument -l <logdir> which can be specified for pg_createsubscriber. Using > it would create the following log files: > > > > logdir/pg_createsubscriber_server.log which captures all logs related to > starting and stopping the standby server. > > > > logdir/pg_createsubscriber_resetwal.log which captures the output of > pg_resetwal > > > > logdir/pg_createsubscriber_internal.log which captures internal > diagnostic output from pg_createsubscriber (validations, checks, etc.) > > > > Overall, this proposed solution could make the pg_createsubscriber > command output messages more organized. The command would be easier to use > as users will only have to read individual log files rather than parse > through lots of possibly irrelevant output messages. I have attached the > patch for this change. > > > > Special thanks to Vignesh C. for his offlist guidance on this project. > > > > > > Regards, Gyan Sreejith > > > > Hi Gyan. > > I haven't yet looked at this patch in any detail, but here are some > quick comments: > > ====== > > 1. > + printf(_(" -l, --logdir=LOGDIR location for the new log > directory\n")); > > The patch is missing SGML docs updates for pg_createsubscriber new > option, and any explanation of the split of logfiles. > > 2. > I might be mistaken, but IIUC it seems the splitting of the logfile > only works when --logdir is specified. Is that correct? > Why should --logdir have any side-effect other than assigning the log > destination folder? > > ====== > Kind Regards, > Peter Smith. > Fujitsu Australia > -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2025-12-14T15:47:36Z
I have included the patch file after making the changes to the SGML docs. Thanks for your help, Gyan Sreejith On Thu, Dec 11, 2025 at 8:33 PM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > Thanks for the feedback, Peter. > > I am currently working on the SGML docs update, and will promptly get back > with an update. > > For your second point, currently, all output goes directly to the console. > I thought it made more sense to break it up into multiple files depending > on what was being invoked. Do you have another opinion? > > Thank you once again, > Gyan Sreejith > > On Thu, Dec 11, 2025 at 2:29 AM Peter Smith <smithpb2250@gmail.com> wrote: > >> On Wed, Dec 10, 2025 at 9:17 AM Gyan Sreejith <gyan.sreejith@gmail.com> >> wrote: >> > >> > Background: >> > >> > pg_createsubscriber currently outputs all messages (internal validation >> messages, standby server start/stop logs, recovery progress output, and >> output from utilities) directly to the console. As a result, users may find >> debugging and handling errors difficult. It would be more convenient if >> messages were separated and stored in different log files. There is already >> a similar implementation in pg_upgrade. >> > >> > Proposed Solution: >> > >> > Based on issues mentioned previously, I would like to propose a new >> argument -l <logdir> which can be specified for pg_createsubscriber. Using >> it would create the following log files: >> > >> > logdir/pg_createsubscriber_server.log which captures all logs related >> to starting and stopping the standby server. >> > >> > logdir/pg_createsubscriber_resetwal.log which captures the output of >> pg_resetwal >> > >> > logdir/pg_createsubscriber_internal.log which captures internal >> diagnostic output from pg_createsubscriber (validations, checks, etc.) >> > >> > Overall, this proposed solution could make the pg_createsubscriber >> command output messages more organized. The command would be easier to use >> as users will only have to read individual log files rather than parse >> through lots of possibly irrelevant output messages. I have attached the >> patch for this change. >> > >> > Special thanks to Vignesh C. for his offlist guidance on this project. >> > >> > >> > Regards, Gyan Sreejith >> > >> >> Hi Gyan. >> >> I haven't yet looked at this patch in any detail, but here are some >> quick comments: >> >> ====== >> >> 1. >> + printf(_(" -l, --logdir=LOGDIR location for the new log >> directory\n")); >> >> The patch is missing SGML docs updates for pg_createsubscriber new >> option, and any explanation of the split of logfiles. >> >> 2. >> I might be mistaken, but IIUC it seems the splitting of the logfile >> only works when --logdir is specified. Is that correct? >> Why should --logdir have any side-effect other than assigning the log >> destination folder? >> >> ====== >> Kind Regards, >> Peter Smith. >> Fujitsu Australia >> > -
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-12-16T07:01:45Z
Dear Gyan, +1 for the idea. This point has already been discussed since the initial commit [1], but it has left till now. I'm happy if you can take initiative. Of course I can review your patches. Per my understanding, pg_upgrade puts logfiles at the directory, under "${PGDATANEW}/pg_upgrade_output.d/${timestamp}". See Note part in [2]. I feel more straightforward way is to follow that approach: 1. pg_createsubscriber creates a directory pg_createsubscriber_output.d/${timestamp}. ${timestamp} has the same format as ISO 8601 (%Y%m%dT%H%M%S). 2. pg_craetesubscriber saves outputs under the directory. 3. Outputs can be retained when the command failed or --retain is specified. Otherwise, they are removed at the end. Are there benefits to provide -l option? Regarding the patch format, our community prefers patches generated by git format-patch. Can you see the blogpost [3] and try to create patches based on the command? One benefit is we can easily do versioning. [1]: https://www.postgresql.org/message-id/60b45b8a-3047-4a21-ba2a-ddb15daa638f%40eisentraut.org [2]: https://www.postgresql.org/docs/devel/pgupgrade.html [3]: https://peter.eisentraut.org/blog/2023/05/09/how-to-submit-a-patch-by-email-2023-edition Best regards, Hayato Kuroda FUJITSU LIMITED -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Smith <smithpb2250@gmail.com> — 2025-12-16T23:49:28Z
On Fri, Dec 12, 2025 at 12:33 PM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thanks for the feedback, Peter. > > I am currently working on the SGML docs update, and will promptly get back with an update. > > For your second point, currently, all output goes directly to the console. I thought it made more sense to break it up into multiple files depending on what was being invoked. Do you have another opinion? > The point I was trying to make was that if you are going to have a "--logdir" option, then IMO that option should do nothing other than change the destination for the logs. In other words, current behaviour is effectively --logdir=console, so --logdir=some_other_folder should not have some side-effect causing the log to get split into multiple files. If you want to split logs, then I thought Kuroda-San's suggestion [1] sounded better --- (a) mimic pg_upgrade more closely and (b) reconsider if -logdir is needed at all. ====== [1] https://www.postgresql.org/message-id/OSCPR01MB14966FD0961F512B29BD46D6BF5AAA%40OSCPR01MB14966.jpnprd01.prod.outlook.com Kind Regards, Peter Smith. Fujitsu Australia
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2025-12-17T10:07:09Z
On Tue, 16 Dec 2025 at 12:31, Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote: > > Dear Gyan, > > +1 for the idea. This point has already been discussed since the initial commit > [1], but it has left till now. I'm happy if you can take initiative. > Of course I can review your patches. > > Per my understanding, pg_upgrade puts logfiles at the directory, under > "${PGDATANEW}/pg_upgrade_output.d/${timestamp}". See Note part in [2]. > I feel more straightforward way is to follow that approach: > > 1. pg_createsubscriber creates a directory pg_createsubscriber_output.d/${timestamp}. > ${timestamp} has the same format as ISO 8601 (%Y%m%dT%H%M%S). > 2. pg_craetesubscriber saves outputs under the directory. > 3. Outputs can be retained when the command failed or --retain is specified. > Otherwise, they are removed at the end. If I recall correctly, this was implemented that way earlier, but the approach was abandoned around [1]. The primary reason was that when users take a backup of the data directory, they would need to explicitly manage the exclusion of this data, which was considered undesirable. > Are there benefits to provide -l option? By providing this as an option, users can store the log files outside the data directory, eliminating the need for any additional handling during backups. [1] - https://www.postgresql.org/message-id/d546c4bb-92d1-4e2d-898f-48234b12ed25%40app.fastmail.com Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Euler Taveira <euler@eulerto.com> — 2025-12-18T01:28:54Z
On Wed, Dec 17, 2025, at 7:07 AM, vignesh C wrote: > > By providing this as an option, users can store the log files outside > the data directory, eliminating the need for any additional handling > during backups. > Do we really need an option to capture the stdout / stderr output to a file? I doubt it. There is already various ways to capture. psql and pg_upgrade are the only tools that have this option. And honestly, I've never seen these options used in the field. -- Euler Taveira EDB https://www.enterprisedb.com/
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2025-12-18T06:49:02Z
On Thu, Dec 18, 2025 at 6:59 AM Euler Taveira <euler@eulerto.com> wrote: > > On Wed, Dec 17, 2025, at 7:07 AM, vignesh C wrote: > > > > By providing this as an option, users can store the log files outside > > the data directory, eliminating the need for any additional handling > > during backups. > > > > Do we really need an option to capture the stdout / stderr output to a file? I > doubt it. There is already various ways to capture. psql and pg_upgrade are the > only tools that have this option. > pg_ctl also has the -l option. I think any place where long text/errors can be outputted, a log file is preferred because one could later parse it to know the exact details. Also, splitting the log as proposed here or in pg_upgrade helps to navigate the LOG like is the problem in start/stop of the server or a pub-sub setup? Similarly the log can be splitted for pub/sub specific information. There appears to be some useful information like: pg_createsubscriber: warning: two_phase option will not be enabled for replication slots pg_createsubscriber: detail: Subscriptions will be created with the two_phase option disabled. Prepared transactions will be replicated at COMMIT PREPARED. pg_createsubscriber: hint: You can use the command-line option --enable-two-phase to enable two_phase. I think it will be useful to LOG this separately from the main LOG [1] (which can contain server specific info as follows) so that users can consider running pg_createsubscriber with additional options or changing the subscriber configuration once setup is complete. [1]: [startup] LOG: database system was interrupted; last known up at 2025-12-17 14:46:07 IST [startup] LOG: starting backup recovery with redo LSN 0/06000028, checkpoint LSN 0/06000080, on timeline ID 1 [startup] LOG: entering standby mode [startup] LOG: redo starts at 0/06000028 [startup] LOG: completed backup recovery with redo LSN 0/06000028 and end LSN 0/06000120 [startup] LOG: consistent recovery state reached at 0/06000120 -- With Regards, Amit Kapila.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2025-12-23T23:22:11Z
Thank you for the feedback everybody. As I read through this email chain, I found differing opinions on how logging should be implemented. This ambiguity leaves me unsure as to which solution(s) to pursue. As of right now, I have attached the git-format patch like Hayato Kuroda recommended (but it does not have any new changes). I am willing to implement whatever solution when we reach a consensus. Thank you for all of the help, Gyan Sreejith On Thu, Dec 18, 2025 at 1:49 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Dec 18, 2025 at 6:59 AM Euler Taveira <euler@eulerto.com> wrote: > > > > On Wed, Dec 17, 2025, at 7:07 AM, vignesh C wrote: > > > > > > By providing this as an option, users can store the log files outside > > > the data directory, eliminating the need for any additional handling > > > during backups. > > > > > > > Do we really need an option to capture the stdout / stderr output to a > file? I > > doubt it. There is already various ways to capture. psql and pg_upgrade > are the > > only tools that have this option. > > > > pg_ctl also has the -l option. I think any place where long > text/errors can be outputted, a log file is preferred because one > could later parse it to know the exact details. Also, splitting the > log as proposed here or in pg_upgrade helps to navigate the LOG like > is the problem in start/stop of the server or a pub-sub setup? > Similarly the log can be splitted for pub/sub specific information. > There appears to be some useful information like: > > pg_createsubscriber: warning: two_phase option will not be enabled for > replication slots > pg_createsubscriber: detail: Subscriptions will be created with the > two_phase option disabled. Prepared transactions will be replicated at > COMMIT PREPARED. > pg_createsubscriber: hint: You can use the command-line option > --enable-two-phase to enable two_phase. > > I think it will be useful to LOG this separately from the main LOG [1] > (which can contain server specific info as follows) so that users can > consider running pg_createsubscriber with additional options or > changing the subscriber configuration once setup is complete. > > [1]: > [startup] LOG: database system was interrupted; last known up at > 2025-12-17 14:46:07 IST > [startup] LOG: starting backup recovery with redo LSN 0/06000028, > checkpoint LSN 0/06000080, on timeline ID 1 > [startup] LOG: entering standby mode > [startup] LOG: redo starts at 0/06000028 > [startup] LOG: completed backup recovery with redo LSN 0/06000028 and > end LSN 0/06000120 > [startup] LOG: consistent recovery state reached at 0/06000120 > > -- > With Regards, > Amit Kapila. >
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2025-12-29T11:10:46Z
On Wed, 24 Dec 2025 at 04:52, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you for the feedback everybody. As I read through this email chain, I found differing opinions on how logging should be implemented. This ambiguity leaves me unsure as to which solution(s) to pursue. As of right now, I have attached the git-format patch like Hayato Kuroda recommended (but it does not have any new changes). I am willing to implement whatever solution when we reach a consensus. Few comments: 1) The file permissions are 664 for pg_createsubscriber_internal.log, pg_createsubscriber_resetwal.log but 600 for pg_createsubscriber_server.log. The permissions should be the same for all the files. ... if (opt->log_dir != NULL) out_file = psprintf("%s/pg_createsubscriber_resetwal.log", opt->log_dir); else out_file = DEVNULL; cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path, subscriber_dir, out_file); pg_log_debug("pg_resetwal command is: %s", cmd_str); ... ... if (opt->log_dir != NULL) { appendPQExpBuffer(pg_ctl_cmd, " -l %s/pg_createsubscriber_server.log", opt->log_dir); } pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data); rc = system(pg_ctl_cmd->data); ... 2) Can you gracefully handle the case where permissions are not enough in the directory and throw proper error: if (stat(opt.log_dir, &statbuf) != 0) { if (errno == ENOENT) { mkdir(opt.log_dir, S_IRWXU); pg_log_info("log directory created"); } else pg_fatal("could not access directory \"%s\": %m", opt.log_dir); } 3) Currently there is no timestamp included for pg_createsubscriber_internal and pg_createsubscriber_resetwal log file contents. Without that it is difficult to tell when the operations were done. It will be good to include them. 4) The patch does not apply on the head, kindly rebase on top of head. 5) Do you need to open and close the log file each time? ... if (internal_log_file != NULL) { if ((fp = fopen(internal_log_file, "a")) == NULL) pg_fatal("could not write to log file \"%s\": %m", internal_log_file); fprintf(fp, "checking settings on subscriber\n"); fclose(fp); } else pg_log_info("checking settings on subscriber"); ... ... if (internal_log_file != NULL) { if ((fp = fopen(internal_log_file, "a")) == NULL) pg_fatal("could not write to log file \"%s\": %m", internal_log_file); fprintf(fp, "checking settings on publisher\n"); fclose(fp); } else pg_log_info("checking settings on publisher"); ... Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-01-12T08:52:24Z
On Wed, Dec 10, 2025 at 3:47 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > pg_createsubscriber currently outputs all messages (internal validation messages, standby server start/stop logs, recovery progress output, and output from utilities) directly to the console. As a result, users may find debugging and handling errors difficult. It would be more convenient if messages were separated and stored in different log files. There is already a similar implementation in pg_upgrade. > > Proposed Solution: > > Based on issues mentioned previously, I would like to propose a new argument -l <logdir> which can be specified for pg_createsubscriber. Using it would create the following log files: > > logdir/pg_createsubscriber_server.log which captures all logs related to starting and stopping the standby server. > > logdir/pg_createsubscriber_resetwal.log which captures the output of pg_resetwal > > logdir/pg_createsubscriber_internal.log which captures internal diagnostic output from pg_createsubscriber (validations, checks, etc.) > I think we don't need pg_createsubscriber_resetwal.log separately as it just prints "Write-ahead log reset", it could as well be part of pg_createsubscriber_server.log. Also, I noticed that even when -l option is used, the additional LOG in verbose mode still goes to console which is difficult to parse. I fell that should also be part of .log files created by this patch. Additionally, I think it is better to use a folder with time to store as LOG files as used in pg_upgrade. This helps to distinguish log files for different runs (say in case of failures). I also thought to further split pg_createsubscriber_internal.log into publisher and subscriber specific LOGs but not sure if that is worth, so we can leave that for now. -- With Regards, Amit Kapila.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-01-13T04:11:02Z
On Mon, 29 Dec 2025 at 16:40, vignesh C <vignesh21@gmail.com> wrote: > > On Wed, 24 Dec 2025 at 04:52, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > > Thank you for the feedback everybody. As I read through this email chain, I found differing opinions on how logging should be implemented. This ambiguity leaves me unsure as to which solution(s) to pursue. As of right now, I have attached the git-format patch like Hayato Kuroda recommended (but it does not have any new changes). I am willing to implement whatever solution when we reach a consensus. > > Few comments: > 1) The file permissions are 664 for pg_createsubscriber_internal.log, > pg_createsubscriber_resetwal.log but 600 for > pg_createsubscriber_server.log. The permissions should be the same for > all the files. > ... > if (opt->log_dir != NULL) > out_file = psprintf("%s/pg_createsubscriber_resetwal.log", opt->log_dir); > else > out_file = DEVNULL; > > cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path, > subscriber_dir, out_file); > > pg_log_debug("pg_resetwal command is: %s", cmd_str); > ... > > ... > if (opt->log_dir != NULL) > { > appendPQExpBuffer(pg_ctl_cmd, " -l %s/pg_createsubscriber_server.log", > opt->log_dir); > } > > pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data); > rc = system(pg_ctl_cmd->data); > ... For this, you can align the file handling and umask behavior with the logic used in syslogger.c (logfile_open). Doing so will ensure that the resulting log files are created with consistent permissions across all cases. Additionally you can include "The umask is set to 077, so access to the log file is disallowed to other users by default" in the documentation of the pg_createsubscriber log option similar to how it is mentioned in [1]. [1] - https://www.postgresql.org/docs/current/app-pg-ctl.html Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-01-20T00:58:31Z
Thank you for all your input. I have attached the latest version of the patch that includes the changes proposed by Vignesh and Amit. Please let me know if you have any questions or suggestions. Thank you, Gyan Sreejith On Mon, Jan 12, 2026 at 11:11 PM vignesh C <vignesh21@gmail.com> wrote: > On Mon, 29 Dec 2025 at 16:40, vignesh C <vignesh21@gmail.com> wrote: > > > > On Wed, 24 Dec 2025 at 04:52, Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > > > Thank you for the feedback everybody. As I read through this email > chain, I found differing opinions on how logging should be implemented. > This ambiguity leaves me unsure as to which solution(s) to pursue. As of > right now, I have attached the git-format patch like Hayato Kuroda > recommended (but it does not have any new changes). I am willing to > implement whatever solution when we reach a consensus. > > > > Few comments: > > 1) The file permissions are 664 for pg_createsubscriber_internal.log, > > pg_createsubscriber_resetwal.log but 600 for > > pg_createsubscriber_server.log. The permissions should be the same for > > all the files. > > ... > > if (opt->log_dir != NULL) > > out_file = psprintf("%s/pg_createsubscriber_resetwal.log", opt->log_dir); > > else > > out_file = DEVNULL; > > > > cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path, > > subscriber_dir, out_file); > > > > pg_log_debug("pg_resetwal command is: %s", cmd_str); > > ... > > > > ... > > if (opt->log_dir != NULL) > > { > > appendPQExpBuffer(pg_ctl_cmd, " -l %s/pg_createsubscriber_server.log", > > opt->log_dir); > > } > > > > pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data); > > rc = system(pg_ctl_cmd->data); > > ... > > For this, you can align the file handling and umask behavior with the > logic used in syslogger.c (logfile_open). Doing so will ensure that > the resulting log files are created with consistent permissions across > all cases. Additionally you can include "The umask is set to 077, so > access to the log file is disallowed to other users by default" in the > documentation of the pg_createsubscriber log option similar to how it > is mentioned in [1]. > [1] - https://www.postgresql.org/docs/current/app-pg-ctl.html > > Regards, > Vignesh > -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-01-20T12:55:09Z
On Tue, 20 Jan 2026 at 06:28, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you for all your input. I have attached the latest version of the patch that includes the changes proposed by Vignesh and Amit. Please let me know if you have any questions or suggestions. 1) Currently you are creating directories like specifiedlogdir_timestamp for each run, so it generates log directories like: logdir_2026-01-20-18-15-55.267510 logdir_2026-01-20-18-16-49.468882 Instead can you change it to specifiedlogdir/exec_timestamp1. specifiedlogdir/exec_timestamp2, etc + populate_timestamp(timestamp, sizeof(timestamp)); + log_dir = psprintf("%s_%s", optarg, timestamp); + opt.log_dir = pg_strdup(log_dir); + canonicalize_path(opt.log_dir); + + if (stat(opt.log_dir, &statbuf) != 0) + { + if (errno == ENOENT) + { + mkdir(opt.log_dir, S_IRWXU); 2) Your patch is based on a slightly older code, it does not apply on HEAD, Kindly rebase your patch on top of HEAD, Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-01-26T01:38:00Z
Thank you, I have made the changes and attached the patch. Regards, Gyan On Tue, Jan 20, 2026 at 7:55 AM vignesh C <vignesh21@gmail.com> wrote: > On Tue, 20 Jan 2026 at 06:28, Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > Thank you for all your input. I have attached the latest version of the > patch that includes the changes proposed by Vignesh and Amit. Please let me > know if you have any questions or suggestions. > > 1) Currently you are creating directories like > specifiedlogdir_timestamp for each run, so it generates log > directories like: > logdir_2026-01-20-18-15-55.267510 > logdir_2026-01-20-18-16-49.468882 > > Instead can you change it to specifiedlogdir/exec_timestamp1. > specifiedlogdir/exec_timestamp2, etc > > + populate_timestamp(timestamp, > sizeof(timestamp)); > + log_dir = psprintf("%s_%s", optarg, > timestamp); > + opt.log_dir = pg_strdup(log_dir); > + canonicalize_path(opt.log_dir); > + > + if (stat(opt.log_dir, &statbuf) != 0) > + { > + if (errno == ENOENT) > + { > + mkdir(opt.log_dir, > S_IRWXU); > > > 2) Your patch is based on a slightly older code, it does not apply on > HEAD, Kindly rebase your patch on top of HEAD, > > Regards, > Vignesh > -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-01-30T04:49:55Z
On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you, I have made the changes and attached the patch. Currently the files are created like this with the v3 patch: If -l logdir is specified: logdir ├── pg_createsubscriber_internal_2026-01-30-10-00-58.300264.log ├── pg_createsubscriber_server_2026-01-30-10-00-58.300264.log ├── pg_createsubscriber_internal_2026-01-30-10-08-54.270230.log └── pg_createsubscriber_server_2026-01-30-10-08-54.270230.log You might have slightly misunderstood my previous comment, but the expected is like: logdir ├── 20260130T100618.912 │ ├── pg_createsubscriber_internal.log │ └── pg_createsubscriber_server.log └── 20260130T101320.952 ├── pg_createsubscriber_internal.log └── pg_createsubscriber_server.log Let's keep the similar structure as created by pg_upgrade. You can execute pg_upgrade once see how it is created. You can refer to make_outputdirs for the same. Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-01-30T09:34:48Z
On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you, I have made the changes and attached the patch. Few comments: 1) Adding \n at the end will assert as pg_log_generic_v has the following Assert: Assert(fmt[strlen(fmt) - 1] != '\n'); @@ -1106,7 +1220,7 @@ check_subscriber(const struct LogicalRepInfo *dbinfo) int max_reporigins; int max_wprocs; - pg_log_info("checking settings on subscriber"); + INFO("checking settings on subscriber\n"); You can run in verbose mode without -l to see this issue 2) There is a chance that directory creation can fail, it should be checked and error should be thrown: + if (stat(opt.log_dir, &statbuf) != 0) + { + if (errno == ENOENT) + { + mkdir(opt.log_dir, S_IRWXU); + INFO("log directory created"); 3) Can you include an fflush after the fprintf so that there is no log content lost in case of abrupt failure: + va_start(args, format); + vfprintf(internal_log_file_fp, format, args); + fprintf(internal_log_file_fp, "\n"); + va_end(args); 4) Since you are closing the log file early, the logs after this point like the drop publication/drop replication slot in error flow will be lost. They will neither appear in the console nor in the log file: * Clean up objects created by pg_createsubscriber. @@ -212,6 +315,9 @@ cleanup_objects_atexit(void) if (success) return; + if (internal_log_file_fp != NULL) + fclose(internal_log_file_fp); + 5) Since there is only one caller for this function and not needed by anyone else, this code can be moved to the caller: +static void +populate_timestamp(char *timestr, size_t ts_len) +{ + struct timeval tval; + time_t now; + struct tm tmbuf; + + gettimeofday(&tval, NULL); + + /* + * MSVC's implementation of timeval uses a long for tv_sec, however, + * localtime() expects a time_t pointer. Here we'll assign tv_sec to a + * local time_t variable so that we pass localtime() the correct pointer + * type. + */ + now = tval.tv_sec; + strftime(timestr, ts_len, + "%Y-%m-%d-%H-%M-%S", + localtime_r(&now, &tmbuf)); + /* append microseconds */ + snprintf(timestr + strlen(timestr), ts_len - strlen(timestr), + ".%06u", (unsigned int) (tval.tv_usec)); +} Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-02-01T23:05:19Z
Thank you! I have made the suggested changes. In addition, I added a wrapper for pg_fatal to write to the file and then do everything that pg_fatal would do. I have attached the patch. Regards, Gyan On Fri, Jan 30, 2026 at 4:35 AM vignesh C <vignesh21@gmail.com> wrote: > On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > Thank you, I have made the changes and attached the patch. > > Few comments: > 1) Adding \n at the end will assert as pg_log_generic_v has the > following Assert: > Assert(fmt[strlen(fmt) - 1] != '\n'); > > @@ -1106,7 +1220,7 @@ check_subscriber(const struct LogicalRepInfo *dbinfo) > int max_reporigins; > int max_wprocs; > > - pg_log_info("checking settings on subscriber"); > + INFO("checking settings on subscriber\n"); > > You can run in verbose mode without -l to see this issue > > 2) There is a chance that directory creation can fail, it should be > checked and error should be thrown: > + if (stat(opt.log_dir, &statbuf) != 0) > + { > + if (errno == ENOENT) > + { > + mkdir(opt.log_dir, > S_IRWXU); > + INFO("log directory > created"); > > 3) Can you include an fflush after the fprintf so that there is no log > content lost in case of abrupt failure: > + va_start(args, format); > + vfprintf(internal_log_file_fp, format, args); > + fprintf(internal_log_file_fp, "\n"); > + va_end(args); > > 4) Since you are closing the log file early, the logs after this point > like the drop publication/drop replication slot in error flow will be > lost. They will neither appear in the console nor in the log file: > * Clean up objects created by pg_createsubscriber. > @@ -212,6 +315,9 @@ cleanup_objects_atexit(void) > if (success) > return; > > + if (internal_log_file_fp != NULL) > + fclose(internal_log_file_fp); > + > > 5) Since there is only one caller for this function and not needed by > anyone else, this code can be moved to the caller: > +static void > +populate_timestamp(char *timestr, size_t ts_len) > +{ > + struct timeval tval; > + time_t now; > + struct tm tmbuf; > + > + gettimeofday(&tval, NULL); > + > + /* > + * MSVC's implementation of timeval uses a long for tv_sec, > however, > + * localtime() expects a time_t pointer. Here we'll assign tv_sec > to a > + * local time_t variable so that we pass localtime() the correct > pointer > + * type. > + */ > + now = tval.tv_sec; > + strftime(timestr, ts_len, > + "%Y-%m-%d-%H-%M-%S", > + localtime_r(&now, &tmbuf)); > + /* append microseconds */ > + snprintf(timestr + strlen(timestr), ts_len - strlen(timestr), > + ".%06u", (unsigned int) (tval.tv_usec)); > +} > > Regards, > Vignesh > -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-02-04T04:46:22Z
On Mon, 2 Feb 2026 at 04:35, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you! > > I have made the suggested changes. You have missed to address the comments from [1]. Please change it as suggested. [1] - https://www.postgresql.org/message-id/CALDaNm3Kx%3Dqv2XOf9x-bsOQx8R2aRBuJKxeTU0%2BSKR70G9WjZA%40mail.gmail.com Regards, Vignesh
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-02-04T09:43:53Z
On Mon, 2 Feb 2026 at 04:35, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you! > > I have made the suggested changes. In addition, I added a wrapper for pg_fatal to write to the file and then do everything that pg_fatal would do. > I have attached the patch. Few comments: 1) Can we change the macro names: INFO -> pg_log_info INFO_HINT -> pg_log_info_hint DEBUG -> pg_log_debug FATAL -> pg_fatal if required do an #undef and call pg_log_generic in the else similar to how it is done in pg_backup_utils.h +#define INFO(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + else \ + pg_log_info(__VA_ARGS__);\ +} while(0) + +#define INFO_HINT(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + else \ + pg_log_info_hint(__VA_ARGS__);\ +} while(0) + +#define DEBUG(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + else \ + pg_log_debug(__VA_ARGS__);\ +} while(0) + +#define FATAL(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + pg_fatal(__VA_ARGS__); /* call pg_fatal after writing to logs */ \ +} while(0) 2) You can keep the variabled in case 'l' handling to keep the scope accordingly: + char timestamp[128]; + struct timeval tval; + time_t now; 3) Instead of creating a new standby and running with -l option, can you run it with -l for one of the existing tests: +$node_p->backup('backup_3'); + +# Set up node R as a logical replica node +my $node_r = PostgreSQL::Test::Cluster->new('node_r'); +$node_r->init_from_backup($node_p, 'backup_3', has_streaming => 1); +$node_r->append_conf( + 'postgresql.conf', qq[ +primary_conninfo = '$pconnstr dbname=postgres' +hot_standby_feedback = on +]); +$node_r->set_standby_mode(); + +# Test that --logdir works for pg_createsubscriber +command_ok( + [ + 'pg_createsubscriber', + '--verbose', + '--pgdata' => $node_r->data_dir, + '--publisher-server' => $pconnstr, + '--database' => 'postgres', + '--logdir' => $logdir, + ], + 'check for log file creation for pg_createSubscriber'); + +# Check that the log files were created +my @server_log_files = glob "$logdir/pg_createsubscriber_server_*.log"; +is( scalar(@server_log_files), 1, " + pg_createsubscriber_server.log file was created"); +my @internal_log_files = glob "$logdir/pg_createsubscriber_internal_*.log"; +is( scalar(@internal_log_files), 1, " + pg_createsubscriber_internal.log file was created"); Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-02-09T02:08:15Z
I was able to make the first two changes that you suggested. However, I was not able to get the test to run with one of the existing replica servers. I have attached the patch for the first two changes. Thank you! Regards, Gyan On Wed, Feb 4, 2026 at 4:44 AM vignesh C <vignesh21@gmail.com> wrote: > On Mon, 2 Feb 2026 at 04:35, Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > Thank you! > > > > I have made the suggested changes. In addition, I added a wrapper for > pg_fatal to write to the file and then do everything that pg_fatal would do. > > I have attached the patch. > > Few comments: > 1) Can we change the macro names: > INFO -> pg_log_info > INFO_HINT -> pg_log_info_hint > DEBUG -> pg_log_debug > FATAL -> pg_fatal > > if required do an #undef and call pg_log_generic in the else similar > to how it is done in pg_backup_utils.h > > +#define INFO(...) do{\ > + if (internal_log_file_fp != NULL) \ > + internal_log_file_write(__VA_ARGS__); \ > + else \ > + pg_log_info(__VA_ARGS__);\ > +} while(0) > + > +#define INFO_HINT(...) do{\ > + if (internal_log_file_fp != NULL) \ > + internal_log_file_write(__VA_ARGS__); \ > + else \ > + pg_log_info_hint(__VA_ARGS__);\ > +} while(0) > + > +#define DEBUG(...) do{\ > + if (internal_log_file_fp != NULL) \ > + internal_log_file_write(__VA_ARGS__); \ > + else \ > + pg_log_debug(__VA_ARGS__);\ > +} while(0) > + > +#define FATAL(...) do{\ > + if (internal_log_file_fp != NULL) \ > + internal_log_file_write(__VA_ARGS__); \ > + pg_fatal(__VA_ARGS__); /* call pg_fatal after writing to logs */ \ > +} while(0) > > 2) You can keep the variabled in case 'l' handling to keep the scope > accordingly: > + char timestamp[128]; > + struct timeval tval; > + time_t now; > > 3) Instead of creating a new standby and running with -l option, can > you run it with -l for one of the existing tests: > +$node_p->backup('backup_3'); > + > +# Set up node R as a logical replica node > +my $node_r = PostgreSQL::Test::Cluster->new('node_r'); > +$node_r->init_from_backup($node_p, 'backup_3', has_streaming => 1); > +$node_r->append_conf( > + 'postgresql.conf', qq[ > +primary_conninfo = '$pconnstr dbname=postgres' > +hot_standby_feedback = on > +]); > +$node_r->set_standby_mode(); > + > +# Test that --logdir works for pg_createsubscriber > +command_ok( > + [ > + 'pg_createsubscriber', > + '--verbose', > + '--pgdata' => $node_r->data_dir, > + '--publisher-server' => $pconnstr, > + '--database' => 'postgres', > + '--logdir' => $logdir, > + ], > + 'check for log file creation for pg_createSubscriber'); > + > +# Check that the log files were created > +my @server_log_files = glob "$logdir/pg_createsubscriber_server_*.log"; > +is( scalar(@server_log_files), 1, " > + pg_createsubscriber_server.log file was created"); > +my @internal_log_files = glob > "$logdir/pg_createsubscriber_internal_*.log"; > +is( scalar(@internal_log_files), 1, " > + pg_createsubscriber_internal.log file was created"); > > Regards, > Vignesh > -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-02-16T09:14:41Z
On Mon, 9 Feb 2026 at 07:38, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > I was able to make the first two changes that you suggested. However, I was not able to get the test to run with one of the existing replica servers. I have attached the patch for the first two changes. For the other comment, you don't need to add a new test case, you have to choose one of the existing command_ok tests like: command_ok( [ 'pg_createsubscriber', '--verbose', '--dry-run', '--pgdata' => $node_s->data_dir, '--publisher-server' => $node_p->connstr($db1), '--socketdir' => $node_s->host, '--subscriber-port' => $node_s->port, '--replication-slot' => 'replslot1', ], 'run pg_createsubscriber without --databases'); or command_ok( [ 'pg_createsubscriber', '--verbose', '--verbose', '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default, '--pgdata' => $node_s->data_dir, '--publisher-server' => $node_p->connstr($db1), '--socketdir' => $node_s->host, '--subscriber-port' => $node_s->port, '--publication' => 'test_pub3', '--publication' => 'pub2', '--replication-slot' => 'replslot1', '--replication-slot' => 'replslot2', '--database' => $db1, '--database' => $db2, '--enable-two-phase', '--clean' => 'publications', ], 'run pg_createsubscriber on node S'); Add --logdir' => $logdir option to this test and include the verification you are doing. Regards, Vignesh
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-02-24T23:55:08Z
I have made the changes you suggested and have attached the patch below. Thanks for your help, Gyan Sreejith On Mon, Feb 16, 2026 at 4:14 AM vignesh C <vignesh21@gmail.com> wrote: > On Mon, 9 Feb 2026 at 07:38, Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > I was able to make the first two changes that you suggested. However, I > was not able to get the test to run with one of the existing replica > servers. I have attached the patch for the first two changes. > > For the other comment, you don't need to add a new test case, you have > to choose one of the existing command_ok tests like: > command_ok( > [ > 'pg_createsubscriber', > '--verbose', > '--dry-run', > '--pgdata' => $node_s->data_dir, > '--publisher-server' => $node_p->connstr($db1), > '--socketdir' => $node_s->host, > '--subscriber-port' => $node_s->port, > '--replication-slot' => 'replslot1', > ], > 'run pg_createsubscriber without --databases'); > > or > > command_ok( > [ > 'pg_createsubscriber', > '--verbose', '--verbose', > '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default, > '--pgdata' => $node_s->data_dir, > '--publisher-server' => $node_p->connstr($db1), > '--socketdir' => $node_s->host, > '--subscriber-port' => $node_s->port, > '--publication' => 'test_pub3', > '--publication' => 'pub2', > '--replication-slot' => 'replslot1', > '--replication-slot' => 'replslot2', > '--database' => $db1, > '--database' => $db2, > '--enable-two-phase', > '--clean' => 'publications', > ], > 'run pg_createsubscriber on node S'); > > Add --logdir' => $logdir option to this test and include the > verification you are doing. > > Regards, > Vignesh >
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Euler Taveira <euler@eulerto.com> — 2026-03-05T14:48:57Z
On Tue, Feb 24, 2026, at 8:55 PM, Gyan Sreejith wrote: > I have made the changes you suggested and have attached the patch below. > [Avoid top-posting ...] I took another look at this patch. Comments are below: +/* + * Open a new logfile with proper permissions. + * From src/backend/postmaster/syslogger.c + */ +static FILE * +logfile_open(const char *filename, const char *mode) +{ Don't duplicate code. If you are reusing a function, my advice is to move it to src/common. You can always use "ifdef FRONTEND" to use the appropriate log message (elog/ereport vs pg_error, for example). - * XXX this code was extracted from BootStrapXLOG(). + * XXX this code was extracted from BootStrapXpg_log_info(). This is a typo. + case 'l': + { + char timestamp[128]; + struct timeval tval; + time_t now; + struct tm tmbuf; I would expect to have this code in a function. That's the pattern it is using. + strftime(timestamp, sizeof(timestamp), "%Y-%m-%d-%H-%M-%S", localtime_r(&now, &tmbuf)); + /* append microseconds */ + snprintf(timestamp + strlen(timestamp), sizeof(timestamp) - strlen(timestamp), + ".%06u", (unsigned int) (tval.tv_usec)); + log_timestamp = pg_strdup(timestamp); Do we really need microseconds? I would say milliseconds is sufficient. I suggest to remove the dash (-); it is using a different style from existing code (pg_upgrade). + opt.log_dir = pg_strdup(optarg); + canonicalize_path(opt.log_dir); You didn't have a check for long path. It is not a good idea in general. See MAXPGPATH examples. It would be a good idea if the path are restricted to MAXPGPATH length. + if (errno == ENOENT) + { + if (mkdir(opt.log_dir, S_IRWXU) == 0) + pg_log_info("log directory created"); + else if (errno == EACCES) + pg_fatal("permission denied trying to create log directory \"%s\": %m", opt.log_dir); + else + pg_fatal("could not create log directory \"%s\": %m", opt.log_dir); + } + else if (errno == EACCES) + pg_fatal("permission denied trying to access directory \"%s\": %m", opt.log_dir); + else + pg_fatal("could not access directory \"%s\": %m", opt.log_dir); The "permission denied" is redundant here because it will be in %m. Instead, I suggest that you use could not create directory \"%s\": %m The main advantage is that this sentence is already available. It avoids translation effort. +#undef pg_log_info +#define pg_log_info(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + else \ + pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\ +} while(0) I don't like the fact that internal_log_file_fp is not declared before this #define. One of the arguments to have this feature was that pg_createsubscriber mixes the server and tool messages. Couldn't we fix it adding "marks" on the output saying the server log messages starts here and the server log messages ends here? -- Euler Taveira EDB https://www.enterprisedb.com/ -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-06T10:50:51Z
On Thu, Mar 5, 2026 at 8:19 PM Euler Taveira <euler@eulerto.com> wrote: > > One of the arguments to have this feature was that pg_createsubscriber mixes the > server and tool messages. Couldn't we fix it adding "marks" on the output saying > the server log messages starts here and the server log messages ends here? > It will simplify the output to some extent but still it will be mixed at different intervals in the LOG which is not good for grepping required info. Also, I am not sure if we use such markers at other places. -- With Regards, Amit Kapila.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-09T22:55:56Z
On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <euler@eulerto.com> wrote: Don't duplicate code. If you are reusing a function, my advice is to move > it to > src/common. You can always use "ifdef FRONTEND" to use the appropriate log > message (elog/ereport vs pg_error, for example). I have made all the changes except for this one, and I am deferring to Amit Kapila regarding the marks. Regarding duplicating code - I was trying to keep the change contained. Is it okay to make that change in a separate check in since it would also touch src/backend/postmaster/syslogger.c? Thank you! Gyan Sreejith On Fri, Mar 6, 2026 at 5:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Mar 5, 2026 at 8:19 PM Euler Taveira <euler@eulerto.com> wrote: > > > > One of the arguments to have this feature was that pg_createsubscriber > mixes the > > server and tool messages. Couldn't we fix it adding "marks" on the > output saying > > the server log messages starts here and the server log messages ends > here? > > > > It will simplify the output to some extent but still it will be mixed > at different intervals in the LOG which is not good for grepping > required info. Also, I am not sure if we use such markers at other > places. > > -- > With Regards, > Amit Kapila. >
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
vignesh C <vignesh21@gmail.com> — 2026-03-11T10:04:55Z
On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <euler@eulerto.com> wrote: > >> Don't duplicate code. If you are reusing a function, my advice is to move it to >> src/common. You can always use "ifdef FRONTEND" to use the appropriate log >> message (elog/ereport vs pg_error, for example). > > > I have made all the changes except for this one, and I am deferring to Amit Kapila regarding the marks. Few comments: 1) You are not checking log level because of which the contents are logged irrespective of the log level: +#undef pg_log_info +#define pg_log_info(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + else \ + pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\ +} while(0) + +#undef pg_log_info_hint +#define pg_log_info_hint(...) do{\ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(__VA_ARGS__); \ + else \ + pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\ +} while(0) + +#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) 2) Instead of just checking if the file is created or not, let's check for some contents from the file: +# Check that the log files were created +my @server_log_files = glob "$logdir/*/pg_createsubscriber_server.log"; +is( scalar(@server_log_files), 1, " + pg_createsubscriber_server.log file was created"); +my @internal_log_files = glob "$logdir/*/pg_createsubscriber_internal.log"; +is( scalar(@internal_log_files), 1, " + pg_createsubscriber_internal.log file was created"); 3) This change is not required, let's remove this: --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl @@ -13,7 +13,8 @@ program_help_ok('pg_createsubscriber'); program_version_ok('pg_createsubscriber'); program_options_handling_ok('pg_createsubscriber'); -my $datadir = PostgreSQL::Test::Utils::tempdir; +my $datadir = PostgreSQL::Test::Utils::tempdir + "/datadir"; 4) No need of '{' as it is a single line statement if (opt->log_dir != NULL) { appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME); } Regards, Vignesh -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-15T23:23:11Z
On Wed, Mar 11, 2026 at 6:05 AM vignesh C <vignesh21@gmail.com> wrote: > On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <euler@eulerto.com> wrote: > > > >> Don't duplicate code. If you are reusing a function, my advice is to > move it to > >> src/common. You can always use "ifdef FRONTEND" to use the appropriate > log > >> message (elog/ereport vs pg_error, for example). > > > > > > I have made all the changes except for this one, and I am deferring to > Amit Kapila regarding the marks. > > Few comments: > 1) You are not checking log level because of which the contents are > logged irrespective of the log level: > +#undef pg_log_info > +#define pg_log_info(...) do{\ > + if (internal_log_file_fp != NULL) \ > + internal_log_file_write(__VA_ARGS__); \ > + else \ > + pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\ > +} while(0) > + > +#undef pg_log_info_hint > +#define pg_log_info_hint(...) do{\ > + if (internal_log_file_fp != NULL) \ > + internal_log_file_write(__VA_ARGS__); \ > + else \ > + pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\ > +} while(0) > + > +#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 log level is passed to and checked by pg_log_generic_v() which is called by pg_log_generic(). > > 2) Instead of just checking if the file is created or not, let's check > for some contents from the file: > Added checks to ensure that the log files are not empty, thanks! > > 3) This change is not required, let's remove this: > --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl > +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl > @@ -13,7 +13,8 @@ program_help_ok('pg_createsubscriber'); > program_version_ok('pg_createsubscriber'); > program_options_handling_ok('pg_createsubscriber'); > > -my $datadir = PostgreSQL::Test::Utils::tempdir; > +my $datadir = PostgreSQL::Test::Utils::tempdir + "/datadir"; Fixed > > 4) No need of '{' as it is a single line statement > if (opt->log_dir != NULL) > { > appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, > log_timestamp, SERVER_LOG_FILE_NAME); > } > Fixed Thank you! I have attached the changes. Regards, Gyan -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
shveta malik <shveta.malik@gmail.com> — 2026-03-17T06:05:53Z
On Mon, Mar 16, 2026 at 4:53 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > Thank you! I have attached the changes. Thank You for the patch. 1) I observed that when we give both --verbose and --logdir options, it skips verbose and only dumps logs to logdir. Is this intentional? I don't see any other postgres command with both the options provided and thus cannot compare the behaviour. But IMO, if the user has explicitly given both options, we should dump the log on both (stdout and logfile). Thoughts? (Apologies if this has already been discussed and I missed it.) 2) I noticed a minor difference when running the command with both --verbose and --logdir, depending on the order in which they are specified. If --verbose is specified first, I see the following message on the console: pg_createsubscriber: directory ../../pg_createsub/20260317T112204.848 created But if --logdir is specified first, this message does not appear on the console. Is this behavior intentional? thanks Shveta
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-17T10:40:53Z
On Tue, Mar 17, 2026 at 11:36 AM shveta malik <shveta.malik@gmail.com> wrote: > > 1) > I observed that when we give both --verbose and --logdir options, it > skips verbose and only dumps logs to logdir. Is this intentional? > > I don't see any other postgres command with both the options provided > and thus cannot compare the behaviour. But IMO, if the user has > explicitly given both options, we should dump the log on both (stdout > and logfile). > The entire log should be dumped in the specified log_file. I think this is similar to one setting server's log_level to debug1 and then start server with -l option in which case the entire log is dumped to server log file. -- With Regards, Amit Kapila.
-
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-03-17T12:18:41Z
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.gmail.com [2]: https://commitfest.postgresql.org/patch/6592/ Best regards, Hayato Kuroda FUJITSU LIMITED -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-03-17T12:48:27Z
Hi Gyan, Thanks for working on the patch. On Mon, 16 Mar 2026 at 04:53, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > > On Wed, Mar 11, 2026 at 6:05 AM vignesh C <vignesh21@gmail.com> wrote: >> >> On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: >> > >> > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <euler@eulerto.com> wrote: >> > >> >> Don't duplicate code. If you are reusing a function, my advice is to move it to >> >> src/common. You can always use "ifdef FRONTEND" to use the appropriate log >> >> message (elog/ereport vs pg_error, for example). >> > >> > >> > I have made all the changes except for this one, and I am deferring to Amit Kapila regarding the marks. >> >> Few comments: >> 1) You are not checking log level because of which the contents are >> logged irrespective of the log level: >> +#undef pg_log_info >> +#define pg_log_info(...) do{\ >> + if (internal_log_file_fp != NULL) \ >> + internal_log_file_write(__VA_ARGS__); \ >> + else \ >> + pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\ >> +} while(0) >> + >> +#undef pg_log_info_hint >> +#define pg_log_info_hint(...) do{\ >> + if (internal_log_file_fp != NULL) \ >> + internal_log_file_write(__VA_ARGS__); \ >> + else \ >> + pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\ >> +} while(0) >> + >> +#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 log level is passed to and checked by pg_log_generic_v() which is called by pg_log_generic(). But we are not checking the log level when we are writing the logs in the logfiles. Due to this, extra logs can appear. >> >> >> >> 2) Instead of just checking if the file is created or not, let's check >> for some contents from the file: > > Added checks to ensure that the log files are not empty, thanks! I think along with it we should also check the actual contents of each file. >> >> >> 3) This change is not required, let's remove this: >> --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl >> +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl >> @@ -13,7 +13,8 @@ program_help_ok('pg_createsubscriber'); >> program_version_ok('pg_createsubscriber'); >> program_options_handling_ok('pg_createsubscriber'); >> >> -my $datadir = PostgreSQL::Test::Utils::tempdir; >> +my $datadir = PostgreSQL::Test::Utils::tempdir + "/datadir"; > > Fixed >> >> >> >> 4) No need of '{' as it is a single line statement >> if (opt->log_dir != NULL) >> { >> appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, >> log_timestamp, SERVER_LOG_FILE_NAME); >> } > > Fixed > > Thank you! I have attached the changes. I noticed that we do not define pg_log_warning, pg_log_warning_detail and pg_log_warning_hint, due to this the contents when option '--logdir' is specified and when it is not defined can differ. Should we define these as well? Also internal_log_file_write is declared as: +static void + internal_log_file_write(const char *format,...) __attribute__((format(printf, 1, 2))); Should we use 'pg_attribute_printf' instead of __attribute__((format(printf, 1, 2)))? I have added the changes for above in the topup patch. I also did some cosmetic changes. I have also addressed the comment (2) by Shveta in [1]. If you agree with these changes, please feel free to include them in the patch. [1]: https://www.postgresql.org/message-id/CAJpy0uBPvz6S9VE8sLYmoju4BGYh94uks%2BUTocPdD094xqmZ2w%40mail.gmail.com Thanks, Shlok Kyal -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-17T23:24:53Z
Thanks a lot, Shlok, for all the changes. I have included the changes from topup.patch to my changes. I believe it handles all the review comments. Please take a look. Regards, Gyan
-
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
(unknown) — 2026-03-18T06:12:41Z
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? 08. ``` +static void +internal_log_file_write(enum pg_log_level level, const char *format,...) +{ + va_list args; + + if (level < __pg_log_level) + return; + + if (internal_log_file_fp == NULL) + return; + + va_start(args, format); + vfprintf(internal_log_file_fp, format, args); + va_end(args); + + fprintf(internal_log_file_fp, "\n"); + fflush(internal_log_file_fp); +} ``` internal_log_file_fp has already been checked before calling, so it can be Assert(). Also, the translated string should be passed to vfprintf(). My small tests shown that changing from "format" to "_(format)" is enough, but not sure other platforms. Some tests are needed. 09. ``` + if (opt->log_dir != NULL) + out_file = psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME); + else + out_file = DEVNULL; ``` I still think comments should be atop here. Attached patch includes above changes. Best regards, Hayato Kuroda FUJITSU LIMITED -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
shveta malik <shveta.malik@gmail.com> — 2026-03-18T10:03:06Z
On Wed, Mar 18, 2026 at 11:43 AM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> 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 -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-18T13:15:17Z
On Wed, Mar 18, 2026 at 4:55 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thanks a lot, Shlok, for all the changes. I have included the changes from topup.patch to my changes. > +#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__) + +#undef pg_log_warning_detail +#define pg_log_warning_detail(...) \ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \ + pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__) + +#undef pg_log_warning_hint +#define pg_log_warning_hint(...) \ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \ + pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__) + +#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) + +#undef pg_log_info_hint +#define pg_log_info_hint(...) 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_HINT, __VA_ARGS__); \ +} while(0) + +#undef pg_log_debug +#define pg_log_debug(...) do { \ + if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) { \ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(PG_LOG_DEBUG, __VA_ARGS__); \ + else \ + pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \ + } \ +} while(0) + +#undef pg_fatal +#define pg_fatal(...) do { \ + if (internal_log_file_fp != NULL) \ + internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__); \ + pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \ + exit(1); \ +} while(0) This looks odd to me. I think it would be better to encapsulate this in one function (something like we have in pg_log_v) and then based on log level, do required handling. -- With Regards, Amit Kapila. -
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-03-18T13:44:37Z
Dear Amit, > > This looks odd to me. I think it would be better to encapsulate this > in one function (something like we have in pg_log_v) and then based on > log level, do required handling. > Based on the suggestion, I updated patches. Please see attached series. v11-0001 replaces existing pg_log_xxx families to new reporting function pg_createsub_log(). Now only pg_fatal is overwritten, which is same as pg_uprade. v11-0002 is the rebased version of v10. Nothing new, but codes are adjusted based on 0001. v11-0003 contains changes from me [1]. 0002 and 0003 can be combined if it's OK. [1]: https://www.postgresql.org/message-id/OS9PR01MB12149C7DE09F13C3BCBD9357DF54EA%40OS9PR01MB12149.jpnprd01.prod.outlook.com Best regards, Hayato Kuroda FUJITSU LIMITED
-
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
(unknown) — 2026-03-19T11:55:00Z
Hi, While checking again, I found an issue in the new logging function - same va_list is used twice, but it was the undefined behavior. va_copy() is used to fix the issue. Also, below points were found and fixed in 0003. Thanks Peter Smith to work on. ``` + printf(_(" -l, --logdir=LOGDIR location for the new log directory\n")); ``` But there is a possibility that existing directory is specified, right? ``` +my $logdir = PostgreSQL::Test::Utils::tempdir + "/logdir"; ``` Isn't it enough to just use tempdir here? At least, I think using "+" is not appropriate, "." is used to connect strings. ``` + if (opt->log_dir != NULL) + appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME); ``` I do not have 100% confident, but do we have to quote the logfile here? Best regards, Hayato Kuroda FUJITSU LIMITED -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-19T18:29:38Z
Thank you, Kuroda-san, for all the work. I have made a small change - I added a pg_free() to free internal_log_file. Regards, Gyan On Thu, Mar 19, 2026 at 7:55 AM Kuroda, Hayato/黒田 隼人 < kuroda.hayato@fujitsu.com> wrote: > Hi, > > While checking again, I found an issue in the new logging function - same > va_list > is used twice, but it was the undefined behavior. va_copy() is used to fix > the issue. > > Also, below points were found and fixed in 0003. Thanks Peter Smith to > work on. > > ``` > + printf(_(" -l, --logdir=LOGDIR location for the new > log directory\n")); > ``` > > But there is a possibility that existing directory is specified, right? > > ``` > +my $logdir = PostgreSQL::Test::Utils::tempdir + "/logdir"; > ``` > > Isn't it enough to just use tempdir here? At least, I think using "+" is > not > appropriate, "." is used to connect strings. > > ``` > + if (opt->log_dir != NULL) > + appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", > opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME); > ``` > > I do not have 100% confident, but do we have to quote the logfile here? > > Best regards, > Hayato Kuroda > FUJITSU LIMITED > > -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Nisha Moond <nisha.moond412@gmail.com> — 2026-03-20T10:30:03Z
On Thu, Mar 19, 2026 at 11:59 PM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you, Kuroda-san, for all the work. > I have made a small change - I added a pg_free() to free internal_log_file. >> >> Hi Gyan, I reviewed/tested the patches, please find below comments for v13-002 patch - File: pg_createsubscriber.c 1) + + if ((internal_log_file_fp = logfile_open(internal_log_file, "a")) == NULL) + pg_fatal("could not open log file \"%s\": %m", internal_log_file); + IIUC, the pg_fatal() call here seems unreachable as the function logfile_open() itself calls pg_fatal() and exits if it fails to open the file. I think it should just be - internal_log_file_fp = logfile_open(internal_log_file, "a"); Please correct me if I'm missing something. ~~~ 2) + if (opt->log_dir != NULL) + out_file = psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME); + else + out_file = DEVNULL; + + cmd_str = psprintf("\"%s\" -D \"%s\" >> \"%s\"", pg_resetwal_path, + subscriber_dir, out_file); Similar to internal_log_file, why are 'out_file' and 'cmd_str' above not freed? ~~~ 3) Typo - extra blank line after va_end(args); @@ -205,10 +243,11 @@ pg_fatal(const char *pg_restrict fmt,...) va_start(args, fmt); - pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args); + pg_createsub_log_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, args); va_end(args); + exit(1); ~~~ 4) File: t/040_pg_createsubscriber.pl +is( scalar(@server_log_files), 1, " + pg_createsubscriber_server.log file was created"); ... ... +is( scalar(@internal_log_files), 1, " + pg_createsubscriber_internal.log file was created"); Above introduces newlines in result log, test 29 and 32 looks like - [14:46:59.071](0.639s) ok 28 - run pg_createsubscriber --dry-run on node S [14:46:59.071](0.000s) ok 29 - [14:46:59.071](0.000s) # pg_createsubscriber_server.log file was created [14:46:59.071](0.000s) ok 30 - pg_createsubscriber_server.log file not empty [14:46:59.071](0.000s) ok 31 - server reached consistent recovery state [14:46:59.072](0.000s) ok 32 - [14:46:59.072](0.000s) # pg_createsubscriber_internal.log file was created [14:46:59.072](0.000s) ok 33 - pg_createsubscriber_internal.log file not empty These should be single-line results similar to others. -- Thanks, Nisha -
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
(unknown) — 2026-03-20T11:37:55Z
Dear Gyan, Thanks for updating the patch! While considering it further, I started to think the combination with -l and -v is problematic. For now, if -l and -v are set, info/debug-level messages are written only to the file, whereas warning/error messages are written to both. In the case of pg_upgrade, however, all additional messages are printed to both the terminal and the file. They are similar programs, so we should follow the spec. Also, there is another advantage that users can check the progress of the command from INFO/DEBUG messages. Since all comments were OK for you, I merged 0003 into 0002, and now all messages would be printed on both stderr/log file. How do you think? Can you also check other comments like posted on [1] and revise if needed? [1]: https://www.postgresql.org/message-id/CABdArM54_cVUMfE2DGSnJfDBZO1hmaVb82Z4b+1t0fp+xqXaMQ@mail.gmail.com Best regards, Hayato Kuroda FUJITSU LIMITED
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-20T17:00:14Z
Thank you, Kuroda-san and Nisha! I have fixed everything that Nisha suggested. And I agree that all messages should be written to both the log file and the terminal. Thank you, Gyan
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-21T09:57:40Z
On Fri, Mar 20, 2026 at 10:30 PM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > 0001: ===== * +static void pg_createsub_log(enum pg_log_level, enum pg_log_part, + const char *pg_restrict fmt,...) + pg_attribute_printf(3, 4); +pg_noreturn static void pg_fatal(const char *pg_restrict fmt,...) + pg_attribute_printf(1, 2); I see similar functions in other modules are named a bit differently. For example, see report_manifest_error(), report_backup_error(), report_fatal_error(). Then I see many other error reporting functions named similarly in code, some examples are: report_invalid_record(), report_invalid_page(), report_namespace_conflict(), and report_recovery_conflict(). Based on the above information, can we consider renaming the above functions to report_createsub_log() and report_createsub_fatal()? Other than the above point, 0001 LGTM. -- With Regards, Amit Kapila.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-21T23:09:05Z
On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > Based on the above information, can we consider renaming the above functions to report_createsub_log() and report_createsub_fatal()? > > Other than the above point, 0001 LGTM. > I have renamed the functions. Regards, Gyan
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
shveta malik <shveta.malik@gmail.com> — 2026-03-23T03:15:51Z
On Sun, Mar 22, 2026 at 4:39 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> >> Based on the above information, can we consider renaming the above >> >> functions to report_createsub_log() and report_createsub_fatal()? >> >> Other than the above point, 0001 LGTM. > > > I have renamed the functions. > 001 looks good. I like the new function's name. Commit message still refers old function name, it can be corrected during commit. thanks Shveta
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Chao Li <li.evan.chao@gmail.com> — 2026-03-23T06:23:43Z
> On Mar 22, 2026, at 07:09, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > Based on the above information, can we consider renaming the above functions to report_createsub_log() and report_createsub_fatal()? > > Other than the above point, 0001 LGTM. > > I have renamed the functions. > > Regards, > Gyan > <v16-0001-pg_createsubscriber-use-own-reporting-functions.patch><v16-0002-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch> 0001 looks good. Some comments on 0002: 1 ``` + <listitem> + <para> + <literal>pg_createsubscriber_server.log</literal> which captures logs + related to stopping and starting the standby server, + </para> + </listitem> + <listitem> + <para> + <literal>pg_createsubscriber_internal.log</literal> which captures + internal diagnostic output (validations, checks, etc.) + </para> + </listitem> ``` I think we can use tag <filename> for the two file names rather than <literal>. 2 ``` +static void +report_createsub_log_v(enum pg_log_level level, enum pg_log_part part, + const char *pg_restrict fmt, va_list args) +{ + if (internal_log_file_fp != NULL) + { + /* Output to both stderr and the log file */ + va_list arg_cpy; + + va_copy(arg_cpy, args); + pg_log_generic_v(level, part, fmt, arg_cpy); + va_end(arg_cpy); + + internal_log_file_write(level, fmt, args); + } + else + pg_log_generic_v(level, part, fmt, args); +} ``` I think this function can be simplified a little bit as: ``` static void report_createsub_log_v(enum pg_log_level level, enum pg_log_part part, const char *pg_restrict fmt, va_list args) { if (internal_log_file_fp != NULL) { va_list arg_cpy; va_copy(arg_cpy, args); internal_log_file_write(level, fmt, arg_cpy); va_end(arg_cpy); } pg_log_generic_v(level, part, fmt, args); } ``` A few lines shorter, and avoid duplicating of pg_log_generic_v. 3 ``` + if (internal_log_file_fp != NULL) + { + fclose(internal_log_file_fp); ``` We should check return value of fclose(), see 69c57466a7521ee146cfdde766713181d45a2d36. 4 ``` + /* Build timestamp directory path */ + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); + + if (len >= MAXPGPATH) + pg_fatal("directory path for log files, %s/%s, is too long", + logdir, timestamp); ``` In the pg_fatal call, I believe logdir should be log_basedir. 5 ``` + if (internal_log_file_fp != NULL) + fclose(internal_log_file_fp); + return 0; } ``` In the end of main(), we don’t need to close internal_log_file_fp again, because that has been done in cleanup_objects_atexit(). 6 ``` +static void +internal_log_file_write(enum pg_log_level level, const char *pg_restrict fmt, + va_list args) +{ + Assert(internal_log_file_fp); + + /* Do nothing if log level is too low. */ + if (level < __pg_log_level) + return; + + vfprintf(internal_log_file_fp, _(fmt), args); + + fprintf(internal_log_file_fp, "\n"); + fflush(internal_log_file_fp); +} ``` The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is: ``` errno = save_errno; va_copy(ap2, ap); required_len = vsnprintf(NULL, 0, fmt, ap2) + 1; va_end(ap2); ``` Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m. The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional? 7 I think the test script misses a test case for %m. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-23T07:16:36Z
On Mon, Mar 23, 2026 at 8:46 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Sun, Mar 22, 2026 at 4:39 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > > > > On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > >> > >> > >> Based on the above information, can we consider renaming the above > >> > >> functions to report_createsub_log() and report_createsub_fatal()? > >> > >> Other than the above point, 0001 LGTM. > > > > > > I have renamed the functions. > > > > 001 looks good. > Pushed 0001. -- With Regards, Amit Kapila.
-
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
(unknown) — 2026-03-23T09:54:59Z
Dear Gyan, Can you rebase the patch because 0001 is now pushed? Also, below contains my minor comments. 01. Found that pg_log_generic_v() has some prefix but internal_log_file_write() does not. It means output strings are not the same. For example, on terminal: ``` pg_createsubscriber: error: standby server is running pg_createsubscriber: hint: Stop the standby server and try again. ``` But on log file: ``` standby server is running Stop the standby server and try again. ``` It's because pg_log_generic_v() has the format like below. I.e., the program name is printed at the begining, and some prefix also exists in some cases. ${program name}: {error: |warning: |detail: |hint: } content I cannot find such a difference on pg_upgrade: no prefix exists in any cases. So, what should be here? My preference is to basically follow pg_log_generic_v() But remove the program name. How about others? 02. Can we check the permission of generated directory in the test? Best regards, Hayato Kuroda FUJITSU LIMITED -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
shveta malik <shveta.malik@gmail.com> — 2026-03-23T09:55:04Z
On Sun, Mar 22, 2026 at 4:39 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> >> Based on the above information, can we consider renaming the above >> >> functions to report_createsub_log() and report_createsub_fatal()? >> >> Other than the above point, 0001 LGTM. > > > I have renamed the functions. > Thanks. Few comments on 002: 1) We can get rid of below alignment related changes in unrelated test parts. ---- -$node_p->safe_psql($db1, qq( +$node_p->safe_psql( + $db1, qq( -is($result, qq(), - "table is not replicated in database $db1"); +is($result, qq(), "table is not replicated in database $db1"); -is($node_p->safe_psql($db2, "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'pub2'"), - '1', "publication pub2 was created in $db2"); +is( $node_p->safe_psql( + $db2, "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'pub2'"), + '1', + "publication pub2 was created in $db2"); -is($result, qq($db1|{test_pub3} +is( $result, qq($db1|{test_pub3} ---- 2) Can we simplify the logic of report_createsub_log_v to: --- if (internal_log_file_fp != NULL) { va_list arg_cpy; va_copy(arg_cpy, args); internal_log_file_write(level, fmt, arg_cpy); va_end(arg_cpy); } pg_log_generic_v(level, part, fmt, args); --- We need not to invoke pg_log_generic_v in both if and else. 3) + /* Create base directory (ignore if exists) */ + if (mkdir(log_basedir, S_IRWXU) < 0 && errno != EEXIST) + pg_fatal("could not create directory \"%s\": %m", log_basedir); + + /* Create BASE_DIR/$timestamp */ + if (mkdir(logdir, S_IRWXU) < 0) + pg_fatal("could not create directory \"%s\": %m", logdir); -- Instead of S_IRWXU directly, shall we use pg_dir_create_mode (which means S_IRWXU) similar to other modules (pg_upgrade, initdb, pg_dump etc) See pg_upgrade: if (mkdir(log_opts.logdir, pg_dir_create_mode) < 0) pg_fatal("could not create directory \"%s\": %m", log_opts.logdir); 4) + /* Create BASE_DIR/$timestamp */ Above comment refers to BASE_DIR which looks like some variable, but it is not. Can we please change this comment to: /* Create a timestamp-named subdirectory under the base directory */ 5) + /* append milliseconds */ append -->Append thanks Shveta -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-24T01:23:29Z
On Mon, Mar 23, 2026 at 2:24 AM Chao Li <li.evan.chao@gmail.com> wrote: + /* Build timestamp directory path */ + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); + + if (len >= MAXPGPATH) + pg_fatal("directory path for log files, %s/%s, is too long", + logdir, timestamp); ``` > In the pg_fatal call, I believe logdir should be log_basedir. We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir. > The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is: ``` > errno = save_errno; > va_copy(ap2, ap); > required_len = vsnprintf(NULL, 0, fmt, ap2) + 1; > va_end(ap2); ``` > Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m. I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where they are calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically as long as errno is not overwritten - which it will now be. Thank you! >The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional? I could add a switch-case to print it out. Is that important? What do you think? I have fixed the rest of your suggestions. Thank you, Chao Li! On Mon, Mar 23, 2026 at 5:55 AM shveta malik <shveta.malik@gmail.com> wrote: > We can get rid of below alignment related changes in unrelated test parts. They are added when I run pgperltidy. Anyone else trying to change the file after this would see the same thing if we don't change it. Should I move it into another patch? I have fixed the rest of it. Thank you, Shveta Malik! On Mon, Mar 23, 2026 at 5:55 AM Kuroda, Hayato/黒田 隼人 < kuroda.hayato@fujitsu.com> wrote: > 01. > Found that pg_log_generic_v() has some prefix but > internal_log_file_write() does not. > It means output strings are not the same. For example, on terminal: > > ``` > pg_createsubscriber: error: standby server is running > pg_createsubscriber: hint: Stop the standby server and try again. > ``` > > But on log file: > ``` > standby server is running > Stop the standby server and try again. > ``` > > It's because pg_log_generic_v() has the format like below. I.e., the > program name > is printed at the begining, and some prefix also exists in some cases. > > ${program name}: {error: |warning: |detail: |hint: } content > > I cannot find such a difference on pg_upgrade: no prefix exists in any > cases. > So, what should be here? My preference is to basically follow > pg_log_generic_v() > But remove the program name. How about others? > I haven't changed the output of pg_log_generic_v() yet. Shall I add the prefix to the output? I have done the rest of your suggestions. Thank you! Regards, Gyan Sreejith -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Chao Li <li.evan.chao@gmail.com> — 2026-03-24T03:02:12Z
> On Mar 24, 2026, at 09:23, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > On Mon, Mar 23, 2026 at 2:24 AM Chao Li <li.evan.chao@gmail.com> wrote: > + /* Build timestamp directory path */ > + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); > + > + if (len >= MAXPGPATH) > + pg_fatal("directory path for log files, %s/%s, is too long", > + logdir, timestamp); > ``` > > In the pg_fatal call, I believe logdir should be log_basedir. > We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir. Not sure if I stated my comment clearly. The “logdir” is build from (“%s/%s, log_basedir, timestamp), however, in the pg_fatal, you are printing (“%s/%s”, logdir, timestamp), here logdir has included a truncated timestamp as it is too long, and so the fatal message will be “log_basedir/truncated-timestamp/timestamp”. So the pg_fatal should be: ``` report_createsub_fatal("directory path for log files, %s/%s, is too long”, log_basedir, timestamp); ``` > > > The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is: > ``` > > errno = save_errno; > > > va_copy(ap2, ap); > > required_len = vsnprintf(NULL, 0, fmt, ap2) + 1; > > va_end(ap2); > ``` > > Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m. > I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where they are calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically as long as errno is not overwritten - which it will now be. Thank you! I verified with v17, %m works now. > > >The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional? > I could add a switch-case to print it out. Is that important? What do you think? I personally prefer to keep those prefixes, which helps keep the log messages in a consistent style. > > <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch> One comment on v17. ``` + if (internal_log_file_fp != NULL) + { + if (fclose(internal_log_file_fp) != 0) + report_createsub_fatal("could not close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME); + internal_log_file_fp = NULL; + } ``` As the error is about internal_log_file_fp, meaning it may no longer able to write to internal_log_file_fp anymore, we should avoid use report_createsub_fatal to log the error again. Maybe just a pg_fatal(), or just log to stderr. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Chao Li <li.evan.chao@gmail.com> — 2026-03-24T03:31:02Z
> On Mar 24, 2026, at 11:02, Chao Li <li.evan.chao@gmail.com> wrote: > > > >> On Mar 24, 2026, at 09:23, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: >> >> On Mon, Mar 23, 2026 at 2:24 AM Chao Li <li.evan.chao@gmail.com> wrote: >> + /* Build timestamp directory path */ >> + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); >> + >> + if (len >= MAXPGPATH) >> + pg_fatal("directory path for log files, %s/%s, is too long", >> + logdir, timestamp); >> ``` >>> In the pg_fatal call, I believe logdir should be log_basedir. >> We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir. > > Not sure if I stated my comment clearly. The “logdir” is build from (“%s/%s, log_basedir, timestamp), however, in the pg_fatal, you are printing (“%s/%s”, logdir, timestamp), here logdir has included a truncated timestamp as it is too long, and so the fatal message will be “log_basedir/truncated-timestamp/timestamp”. So the pg_fatal should be: > ``` > report_createsub_fatal("directory path for log files, %s/%s, is too long”, > log_basedir, timestamp); > ``` > >> >>> The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is: >> ``` >>> errno = save_errno; >> >>> va_copy(ap2, ap); >>> required_len = vsnprintf(NULL, 0, fmt, ap2) + 1; >>> va_end(ap2); >> ``` >>> Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m. >> I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where they are calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically as long as errno is not overwritten - which it will now be. Thank you! > > I verified with v17, %m works now. > >> >>> The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional? >> I could add a switch-case to print it out. Is that important? What do you think? > > I personally prefer to keep those prefixes, which helps keep the log messages in a consistent style. > >> >> <v17-0001-Add-a-new-argument-l-logdir-to-pg_createsubscrib.patch> > > One comment on v17. > ``` > + if (internal_log_file_fp != NULL) > + { > + if (fclose(internal_log_file_fp) != 0) > + report_createsub_fatal("could not close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME); > + internal_log_file_fp = NULL; > + } > ``` > > As the error is about internal_log_file_fp, meaning it may no longer able to write to internal_log_file_fp anymore, we should avoid use report_createsub_fatal to log the error again. Maybe just a pg_fatal(), or just log to stderr. > I forgot to mention one small suggestion. Since the internal log file path includes a timestamped directory, it is a bit inconvenient to figure out the full path of the file after the command finishes. I think it would be helpful to print the full path of the internal log file at the end, something like: ``` if (internal_log_file_fp != NULL) { if (fclose(internal_log_file_fp) != 0) report_createsub_fatal("could not close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME); internal_log_file_fp = NULL; printf("check the internal log file at \"%s/%s.log\"", logdir, INTERNAL_LOG_FILE_NAME); } ``` Note that I used printf() here, because based on my test, pg_log_info() does not work in this place. The other thing I noticed while trying the above code is that cleanup_objects_atexit() has this: ``` if (success) return; ``` That means on the success path, internal_log_file_fp will not be closed. So this part would need some adjustment as well. Maybe one way is to jump to the log-file cleanup part before returning, for example with a goto, so that internal_log_file_fp is always closed. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-24T06:31:44Z
On Tue, Mar 24, 2026 at 8:32 AM Chao Li <li.evan.chao@gmail.com> wrote: > > > On Mar 24, 2026, at 09:23, Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > > > On Mon, Mar 23, 2026 at 2:24 AM Chao Li <li.evan.chao@gmail.com> wrote: > > + /* Build timestamp directory path */ > > + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); > > + > > + if (len >= MAXPGPATH) > > + pg_fatal("directory path for log files, %s/%s, is too long", > > + logdir, timestamp); > > ``` > > > In the pg_fatal call, I believe logdir should be log_basedir. > > We are writing into logdir, and the if will be true only if it is too long. Hence we should be checking logdir. > > Not sure if I stated my comment clearly. The “logdir” is build from (“%s/%s, log_basedir, timestamp), however, in the pg_fatal, you are printing (“%s/%s”, logdir, timestamp), here logdir has included a truncated timestamp as it is too long, and so the fatal message will be “log_basedir/truncated-timestamp/timestamp”. So the pg_fatal should be: > ``` > report_createsub_fatal("directory path for log files, %s/%s, is too long”, > log_basedir, timestamp); > ``` > > > > > > The biggest problem I see with this patch is here. internal_log_file_write doesn’t handle “%m”. I think we can check the implementation of pg_log_generic_v how %m is handled. The key code snippet is: > > ``` > > > errno = save_errno; > > > > > va_copy(ap2, ap); > > > required_len = vsnprintf(NULL, 0, fmt, ap2) + 1; > > > va_end(ap2); > > ``` > > > Where, vsnprintf points to pg_vsnprintf, and pg_vsnprintf calls dopr to handle %m. > > I have saved and restored errno similar to that. The code snippet you pointed out is, as far as I understand, where they are calculating how much space to allocate (including the \0 at the end). I think it will be handled automatically as long as errno is not overwritten - which it will now be. Thank you! > > I verified with v17, %m works now. > > > > > >The other problem is, with internal_log_file_write, HINT, DETAIL prefix are no longer printed, is that intentional? > > I could add a switch-case to print it out. Is that important? What do you think? > > I personally prefer to keep those prefixes, which helps keep the log messages in a consistent style. > +1 to keep HINT, DETAIL kind of prefixes. I have one additional comment in the latest patch: + mode_t oumask; + + oumask = umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG | S_IRWXO))); + fh = fopen(filename, mode); + umask(oumask); I think we should follow the permissions model for this file same as what pg_ctl does for log_file given with -l option. Basically, start with with 077 permission and then switch to data_directory permissions. This is because to start server we anyway use pg_ctl with -l option which will use a specific way to assign permissions to server log file, so we should use same for internal file, otherwise, we will end up with different file permissions for internal and server logfile. -- With Regards, Amit Kapila. -
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-03-24T09:47:07Z
Dear Chao, > One comment on v17. > ``` > + if (internal_log_file_fp != NULL) > + { > + if (fclose(internal_log_file_fp) != 0) > + report_createsub_fatal("could not > close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME); > + internal_log_file_fp = NULL; > + } > ``` > > As the error is about internal_log_file_fp, meaning it may no longer able to write to > internal_log_file_fp anymore, we should avoid use report_createsub_fatal to log > the error again. Maybe just a pg_fatal(), or just log to stderr. I checked and found [1] in the exit() specification. It means all file descriptors opened by the process would be closed automatically when the process exits. So no need to try fclose() and report the failure here. Similarly, pg_upgrade does not have fclose(log_opts.internal) in case of failures. [1]: First, all functions registered by the atexit function are called, in the reverse order of their registration, except that a function is called after any previously registered functions that had already been called at the time it was registered. If, during the call to any such function, a call to the longjmp function is made that would terminate the call to the registered function, the behavior is undefined. Next, all open streams with unwritten buffered data are flushed, all open streams are closed, and all files created by the tmpfile function are removed. Best regards, Hayato Kuroda FUJITSU LIMITED -
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-03-24T09:50:39Z
Dear Chao, > I forgot to mention one small suggestion. > > Since the internal log file path includes a timestamped directory, it is a bit > inconvenient to figure out the full path of the file after the command finishes. I > think it would be helpful to print the full path of the internal log file at the end, > something like: > ``` > if (internal_log_file_fp != NULL) > { > if (fclose(internal_log_file_fp) != 0) > report_createsub_fatal("could not > close %s/%s.log: %m", logdir, INTERNAL_LOG_FILE_NAME); > internal_log_file_fp = NULL; > printf("check the internal log file at \"%s/%s.log\"", logdir, > INTERNAL_LOG_FILE_NAME); > } > ``` I don't think it's needed, because pg_upgrade does not print such points. > Note that I used printf() here, because based on my test, pg_log_info() does not > work in this place. It might be becasue you used pg_log_info(). pg_createsubscriber outputs only warning or error level messages, info-level cannot be printed. > The other thing I noticed while trying the above code is that > cleanup_objects_atexit() has this: > ``` > if (success) > return; > ``` > > That means on the success path, internal_log_file_fp will not be closed. So this > part would need some adjustment as well. Maybe one way is to jump to the > log-file cleanup part before returning, for example with a goto, so that > internal_log_file_fp is always closed. As the same reason I posted, I think it's not needed. [1]: https://www.postgresql.org/message-id/OS9PR01MB12149F1AD9C79A2644753A18AF548A%40OS9PR01MB12149.jpnprd01.prod.outlook.com Best regards, Hayato Kuroda FUJITSU LIMITED -
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-03-24T10:06:08Z
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
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-24T12:13:43Z
On Tue, Mar 24, 2026 at 3:36 PM Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote: > > I created a top-up patch set which addressed all comments from me and others. > See attached. > + /* Set mask based on the PGDATA permissions */ + if (!GetDataDirectoryCreatePerm(subscriber_dir)) + report_createsub_fatal("could not read permissions of directory \"%s\": %m", + subscriber_dir); We can add a bit more detailed comments as to why we use data_directory permissions. -- With Regards, Amit Kapila. -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Smith <smithpb2250@gmail.com> — 2026-03-24T23:14:08Z
Hi. Here are some review comments for v18-0001. ====== 1. +#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server" +#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal" Those are not the names of logfiles; they are just the first part of the names. Why not also put the ".log" part here, instead of burying it in format strings scattered in the code? SUGGESTION (e.g., more similar to pg_upgrade.h) #define SERVER_LOG_FILE_NAME "pg_createsubscriber_server.log" #define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal.log" ~~~ 2. +static char logdir[MAXPGPATH]; /* Directory log files are put (if specified) */ Missing words in that comment? e.g. Directory where log files are written ... Directory for log files ... It might also be useful to mention that this is more than just the original user-specified --logdir directory; it also includes the timestamp sub-dir. ~~~ 3. /* - * Report a message with a given log level + * Report a message with a given log level to stderr and log file + * (if specified). */ +static void +report_createsub_log_v(enum pg_log_level level, enum pg_log_part part, + const char *pg_restrict fmt, va_list args) This is a function comment, but there is no way to specify the log file by using this function, so maybe reword that "if specified" part for clarity. SUGGESTION: * Report a message with a given log level. * Writes to stderr, and also to the log file (if pg_createsubscriber --logdir option was specified). ~~~ 4. + /* Build timestamp directory path */ + len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp); + + if (len >= MAXPGPATH) + report_createsub_fatal("directory path for log files, %s/%s, is too long", + logdir, timestamp); IIUC, when the snprintf exceeds MAXPGPATH, then 'logdir' is going to contain a truncated string. And, maybe that already includes a partial truncated timestamp part: "myreallylongname/20260" AFAICT, the subsequent fatal message is going to report that incorrectly when it appends the timestamp a second time, like: "... myreallylongname/20260/20260119T204317.204 is too long" Notice that pg_upgrade doesn't try to report the names of files that are too long. It just says things like "directory path for new cluster is too long". ====== Kind Regards, Peter Smith. Fujitsu Australia -
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-25T00:12:59Z
Thank you, Kuroda-san and Chao for the suggestions and the patch. I have merged it with v18-0001, and I have fixed everything that Peter suggested. Regards, Gyan
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-03-25T08:25:05Z
On Wed, Mar 25, 2026 at 5:43 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > > Thank you, Kuroda-san and Chao for the suggestions and the patch. I have merged it with v18-0001, and I have fixed everything that Peter suggested. > Pushed after minor changes. -- With Regards, Amit Kapila.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Gyan Sreejith <gyan.sreejith@gmail.com> — 2026-03-28T16:47:19Z
Thank you everybody for the help. Regards, Gyan On Wed, Mar 25, 2026 at 4:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Mar 25, 2026 at 5:43 AM Gyan Sreejith <gyan.sreejith@gmail.com> > wrote: > > > > Thank you, Kuroda-san and Chao for the suggestions and the patch. I have > merged it with v18-0001, and I have fixed everything that Peter suggested. > > > > Pushed after minor changes. > > -- > With Regards, > Amit Kapila. >
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Eisentraut <peter@eisentraut.org> — 2026-03-31T12:06:22Z
On 25.03.26 09:25, Amit Kapila wrote: > On Wed, Mar 25, 2026 at 5:43 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: >> >> Thank you, Kuroda-san and Chao for the suggestions and the patch. I have merged it with v18-0001, and I have fixed everything that Peter suggested. >> > > Pushed after minor changes. Instead of creating a new logging API in pg_createsubscriber locally, why not extend logging.c to support a log file. This is much simpler, doesn't require caller changes, and could be reused elsewhere. See attached patches. (The 0003 and 0004 are just some additional code simplifications.)
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-04-01T04:56:53Z
On Tue, Mar 31, 2026 at 5:36 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > On 25.03.26 09:25, Amit Kapila wrote: > > On Wed, Mar 25, 2026 at 5:43 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: > >> > >> Thank you, Kuroda-san and Chao for the suggestions and the patch. I have merged it with v18-0001, and I have fixed everything that Peter suggested. > >> > > > > Pushed after minor changes. > > Instead of creating a new logging API in pg_createsubscriber locally, > why not extend logging.c to support a log file. > Few things we may want to consider if we want to use extended logging.c: (a) what if we want information to be written in multiple files (say pub/sub specific info into separate files), something like what pg_upgrade does, (b) what if we want to write some info to only one of terminal or log_file, we can probably extend generic API for this. > This is much simpler, > doesn't require caller changes, and could be reused elsewhere. See > attached patches. (The 0003 and 0004 are just some additional code > simplifications.) > In 0004, -static char logdir[MAXPGPATH]; /* Subdirectory of the user specified logdir +static char *logdir = NULL; /* Subdirectory of the user specified logdir At other places like pg_upgrade, we use MAXPGPATH for a similar case probably to have standard length on all platforms, see make_outputdirs() in pg_upgrade.c. -- With Regards, Amit Kapila.
-
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-04-01T05:05:10Z
Dear Peter E., > Instead of creating a new logging API in pg_createsubscriber locally, > why not extend logging.c to support a log file. I had a concern to extend the common module. Do you have possible idea to use pg_logging_set_logfile() on others? I cannot, tee command on linux seems enough for others. Best regards, Hayato Kuroda FUJITSU LIMITED
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Eisentraut <peter@eisentraut.org> — 2026-04-07T13:36:05Z
On 01.04.26 06:56, Amit Kapila wrote: > On Tue, Mar 31, 2026 at 5:36 PM Peter Eisentraut <peter@eisentraut.org> wrote: >> >> On 25.03.26 09:25, Amit Kapila wrote: >>> On Wed, Mar 25, 2026 at 5:43 AM Gyan Sreejith <gyan.sreejith@gmail.com> wrote: >>>> >>>> Thank you, Kuroda-san and Chao for the suggestions and the patch. I have merged it with v18-0001, and I have fixed everything that Peter suggested. >>>> >>> >>> Pushed after minor changes. >> >> Instead of creating a new logging API in pg_createsubscriber locally, >> why not extend logging.c to support a log file. >> > > Few things we may want to consider if we want to use extended > logging.c: (a) what if we want information to be written in multiple > files (say pub/sub specific info into separate files), something like > what pg_upgrade does, (b) what if we want to write some info to only > one of terminal or log_file, we can probably extend generic API for > this. In the proposed API, you can switch the log file at any time. And there could be a separate log level for file output versus terminal output. >> This is much simpler, >> doesn't require caller changes, and could be reused elsewhere. See >> attached patches. (The 0003 and 0004 are just some additional code >> simplifications.) >> > > In 0004, > -static char logdir[MAXPGPATH]; /* Subdirectory of the user specified logdir > +static char *logdir = NULL; /* Subdirectory of the user specified logdir > > At other places like pg_upgrade, we use MAXPGPATH for a similar case > probably to have standard length on all platforms, see > make_outputdirs() in pg_upgrade.c. I think using MAXPGPATH is kind of deprecated and useless, certainly in new code. But apart from that, note that in pg_createsubscriber.c itself, subscriber_dir is dynamically allocated.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Eisentraut <peter@eisentraut.org> — 2026-04-07T13:37:06Z
On 01.04.26 07:05, Hayato Kuroda (Fujitsu) wrote: > Dear Peter E., > >> Instead of creating a new logging API in pg_createsubscriber locally, >> why not extend logging.c to support a log file. > > I had a concern to extend the common module. Do you have possible idea to use > pg_logging_set_logfile() on others? I cannot, tee command on linux seems enough > for others. pg_upgrade was mentioned.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Amit Kapila <amit.kapila16@gmail.com> — 2026-04-08T05:30:58Z
On Tue, Apr 7, 2026 at 7:07 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > On 01.04.26 07:05, Hayato Kuroda (Fujitsu) wrote: > > Dear Peter E., > > > >> Instead of creating a new logging API in pg_createsubscriber locally, > >> why not extend logging.c to support a log file. > > > > I had a concern to extend the common module. Do you have possible idea to use > > pg_logging_set_logfile() on others? I cannot, tee command on linux seems enough > > for others. > > pg_upgrade was mentioned. > I see that pg_upgrade has some specific requirements related to verbose/status/report kind of messages which doesn't seem to be directly handled by the generic API pg_log_generic(). For example, apart from other things, we need to unset/reset log_file when we want to send messages only to the terminal and quite some specific handling for status or other kinds of messages. If you think pg_log_generic could be extended to handle various upgrade cases then I think it would be a good use case for it and we should go-ahead with what you are proposing. OTOH, maybe we can give it a separate try and use the resulting API for both pg_createsubscriber and pg_upgrade. I am fine either way you would like to go-ahead with this. -- With Regards, Amit Kapila.
-
Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Peter Eisentraut <peter@eisentraut.org> — 2026-04-13T09:24:14Z
On 08.04.26 07:30, Amit Kapila wrote: > On Tue, Apr 7, 2026 at 7:07 PM Peter Eisentraut <peter@eisentraut.org> wrote: >> >> On 01.04.26 07:05, Hayato Kuroda (Fujitsu) wrote: >>> Dear Peter E., >>> >>>> Instead of creating a new logging API in pg_createsubscriber locally, >>>> why not extend logging.c to support a log file. >>> >>> I had a concern to extend the common module. Do you have possible idea to use >>> pg_logging_set_logfile() on others? I cannot, tee command on linux seems enough >>> for others. >> >> pg_upgrade was mentioned. >> > > I see that pg_upgrade has some specific requirements related to > verbose/status/report kind of messages which doesn't seem to be > directly handled by the generic API pg_log_generic(). For example, > apart from other things, we need to unset/reset log_file when we want > to send messages only to the terminal and quite some specific handling > for status or other kinds of messages. If you think pg_log_generic > could be extended to handle various upgrade cases then I think it > would be a good use case for it and we should go-ahead with what you > are proposing. OTOH, maybe we can give it a separate try and use the > resulting API for both pg_createsubscriber and pg_upgrade. I am fine > either way you would like to go-ahead with this. I have committed these patches. I would like to apply the frontend logging API to pg_upgrade as well, but that's a bigger project for not right now.
-
RE: [Proposal] Adding Log File Capability to pg_createsubscriber
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-04-16T00:20:15Z
Dear Peter, > I have committed these patches. Thanks! > I would like to apply the frontend logging API to pg_upgrade as well, > but that's a bigger project for not right now. Right, it may need huge changes. Not sure it can be done for PG19 because we have already been in feature freeze. Best regards, Hayato Kuroda FUJITSU LIMITED