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: Wed, 4 Feb 2026 15:13:53 +0530
Message-ID: <CALDaNm17ujrJ2xHyz6r81WAiFUs38RcT8D5ebdxssASpGko0HA@mail.gmail.com> (raw)
In-Reply-To: <CAEqnbaVgFU2Pr=xhhDmA=sK7XPBDBxECovqbSht91ZbHmnteUg@mail.gmail.com>
References: <CAEqnbaUthOQARV1dscGvB_EsqC-YfxiM6rWkVDHc+G+f4oSUHw@mail.gmail.com>
	<CAHut+PvizcpeHA1Twf_hwe=wANQ1LV5zY6_q+39gTFJc7+bCKg@mail.gmail.com>
	<CAEqnbaVNZYbB_YufchM49d=XC1ZGrVV8ikCPmGotWoCZASY3Uw@mail.gmail.com>
	<CAEqnbaV8QMbXZtt25QtGUPAaQZnD-B0HniFSroapMh+QTZgKsQ@mail.gmail.com>
	<OSCPR01MB14966FD0961F512B29BD46D6BF5AAA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
	<CALDaNm0gX2D6fD5yur-R5gagA7+AfLmLZU5Z8+tgjo61b-Y01w@mail.gmail.com>
	<[email protected]>
	<CAA4eK1+2PYU-y2weY_zFeysP8Ux45iBNdaYMZuJtMTqXwCxawQ@mail.gmail.com>
	<CAEqnbaVVp_g1m1nhOBZjtiAz5W-xwPQCmWH4hjYYehA+ktYg9Q@mail.gmail.com>
	<CALDaNm2T__Uha1fn274bP3jKDSwm37ewWYvA+vOo75FTT2t3SA@mail.gmail.com>
	<CALDaNm1fF6yBa2PCv9gO_MVcjKLQFgi9eAL=g5cU=xtbOr=qZw@mail.gmail.com>
	<CAEqnbaUa27R9OcVDHkNuwsZ1uCpy8sTXpH78N1ug0kj60mnchw@mail.gmail.com>
	<CALDaNm1Mj+eNVETmYAYd4ojrbTbQU5iCX20Fso24r2VuXF69AQ@mail.gmail.com>
	<CAEqnbaUWSPGHLL2nuyNHVKU0TB7uGBx9w0NvnisWFyf5TtwRTQ@mail.gmail.com>
	<CALDaNm3Dj7WRdfD+fQ86XzS07vewaFSWx--kB5689_sC7rt1Uw@mail.gmail.com>
	<CAEqnbaVgFU2Pr=xhhDmA=sK7XPBDBxECovqbSht91ZbHmnteUg@mail.gmail.com>

On Mon, 2 Feb 2026 at 04:35, Gyan Sreejith <[email protected]> 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






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: <CALDaNm17ujrJ2xHyz6r81WAiFUs38RcT8D5ebdxssASpGko0HA@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