public inbox for [email protected]  

help / color / mirror / Atom feed

From: vignesh C <[email protected]>
To: Gyan Sreejith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: Fri, 30 Jan 2026 15:04:48 +0530
Message-ID: <CALDaNm3Dj7WRdfD+fQ86XzS07vewaFSWx--kB5689_sC7rt1Uw@mail.gmail.com> (raw)
In-Reply-To: <CAEqnbaUWSPGHLL2nuyNHVKU0TB7uGBx9w0NvnisWFyf5TtwRTQ@mail.gmail.com>
References: <CAEqnbaUthOQARV1dscGvB_EsqC-YfxiM6rWkVDHc+G+f4oSUHw@mail.gmail.com>
	<CAHut+PvizcpeHA1Twf_hwe=wANQ1LV5zY6_q+39gTFJc7+bCKg@mail.gmail.com>
	<CAEqnbaVNZYbB_YufchM49d=XC1ZGrVV8ikCPmGotWoCZASY3Uw@mail.gmail.com>
	<CAEqnbaV8QMbXZtt25QtGUPAaQZnD-B0HniFSroapMh+QTZgKsQ@mail.gmail.com>
	<OSCPR01MB14966FD0961F512B29BD46D6BF5AAA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
	<CALDaNm0gX2D6fD5yur-R5gagA7+AfLmLZU5Z8+tgjo61b-Y01w@mail.gmail.com>
	<[email protected]>
	<CAA4eK1+2PYU-y2weY_zFeysP8Ux45iBNdaYMZuJtMTqXwCxawQ@mail.gmail.com>
	<CAEqnbaVVp_g1m1nhOBZjtiAz5W-xwPQCmWH4hjYYehA+ktYg9Q@mail.gmail.com>
	<CALDaNm2T__Uha1fn274bP3jKDSwm37ewWYvA+vOo75FTT2t3SA@mail.gmail.com>
	<CALDaNm1fF6yBa2PCv9gO_MVcjKLQFgi9eAL=g5cU=xtbOr=qZw@mail.gmail.com>
	<CAEqnbaUa27R9OcVDHkNuwsZ1uCpy8sTXpH78N1ug0kj60mnchw@mail.gmail.com>
	<CALDaNm1Mj+eNVETmYAYd4ojrbTbQU5iCX20Fso24r2VuXF69AQ@mail.gmail.com>
	<CAEqnbaUWSPGHLL2nuyNHVKU0TB7uGBx9w0NvnisWFyf5TtwRTQ@mail.gmail.com>

On Mon, 26 Jan 2026 at 07:08, Gyan Sreejith <[email protected]> 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

view thread (47+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
  In-Reply-To: <CALDaNm3Dj7WRdfD+fQ86XzS07vewaFSWx--kB5689_sC7rt1Uw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox