public inbox for [email protected]
help / color / mirror / Atom feedFrom: Shlok Kyal <[email protected]>
To: Gyan Sreejith <[email protected]>
Cc: vignesh C <[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: Tue, 17 Mar 2026 18:18:27 +0530
Message-ID: <CANhcyEWaCV7B54oOhuE_BhVCF887h23jKmkHDDWvJRKZ=pBbBQ@mail.gmail.com> (raw)
In-Reply-To: <CAEqnbaWxYp7TbnhKfkjW8c=jRPARsQ1on86bt_pcqvK9vCtLhQ@mail.gmail.com>
References: <CAEqnbaUthOQARV1dscGvB_EsqC-YfxiM6rWkVDHc+G+f4oSUHw@mail.gmail.com>
<CAHut+PvizcpeHA1Twf_hwe=wANQ1LV5zY6_q+39gTFJc7+bCKg@mail.gmail.com>
<CAEqnbaVNZYbB_YufchM49d=XC1ZGrVV8ikCPmGotWoCZASY3Uw@mail.gmail.com>
<CAEqnbaV8QMbXZtt25QtGUPAaQZnD-B0HniFSroapMh+QTZgKsQ@mail.gmail.com>
<OSCPR01MB14966FD0961F512B29BD46D6BF5AAA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CALDaNm0gX2D6fD5yur-R5gagA7+AfLmLZU5Z8+tgjo61b-Y01w@mail.gmail.com>
<[email protected]>
<CAA4eK1+2PYU-y2weY_zFeysP8Ux45iBNdaYMZuJtMTqXwCxawQ@mail.gmail.com>
<CAEqnbaVVp_g1m1nhOBZjtiAz5W-xwPQCmWH4hjYYehA+ktYg9Q@mail.gmail.com>
<CALDaNm2T__Uha1fn274bP3jKDSwm37ewWYvA+vOo75FTT2t3SA@mail.gmail.com>
<CALDaNm1fF6yBa2PCv9gO_MVcjKLQFgi9eAL=g5cU=xtbOr=qZw@mail.gmail.com>
<CAEqnbaUa27R9OcVDHkNuwsZ1uCpy8sTXpH78N1ug0kj60mnchw@mail.gmail.com>
<CALDaNm1Mj+eNVETmYAYd4ojrbTbQU5iCX20Fso24r2VuXF69AQ@mail.gmail.com>
<CAEqnbaUWSPGHLL2nuyNHVKU0TB7uGBx9w0NvnisWFyf5TtwRTQ@mail.gmail.com>
<CALDaNm3Dj7WRdfD+fQ86XzS07vewaFSWx--kB5689_sC7rt1Uw@mail.gmail.com>
<CAEqnbaVgFU2Pr=xhhDmA=sK7XPBDBxECovqbSht91ZbHmnteUg@mail.gmail.com>
<CALDaNm17ujrJ2xHyz6r81WAiFUs38RcT8D5ebdxssASpGko0HA@mail.gmail.com>
<CAEqnbaUbVgGMXBNFbk=sqFJT=_TkAUYGYsFHJKFw5K492M_B=A@mail.gmail.com>
<CALDaNm2SYq_dO7qXHn+BnxmxvmBupH6tofUnx4DKC09h5xfAyQ@mail.gmail.com>
<CAEqnbaWjoSby+_FQOKqTiDwf9AsWVjcGzRn-BQtdivC8xn0ADw@mail.gmail.com>
<[email protected]>
<CAA4eK1+jd6zH-LL8FOrw01chrNetRoZGKXSqV7GNo_--UXYc_w@mail.gmail.com>
<CAEqnbaU50vLy031AbvmfXJ3_qv9iS4pVaMdmsnmpF87r=EqW7Q@mail.gmail.com>
<CALDaNm1P5pZ2tTcaVRvjUAXLAyT7S18GPwp-4bc0xKXKZoXi0w@mail.gmail.com>
<CAEqnbaWxYp7TbnhKfkjW8c=jRPARsQ1on86bt_pcqvK9vCtLhQ@mail.gmail.com>
Hi Gyan,
Thanks for working on the patch.
On Mon, 16 Mar 2026 at 04:53, Gyan Sreejith <[email protected]> wrote:
>
>
>
> On Wed, Mar 11, 2026 at 6:05 AM vignesh C <[email protected]> wrote:
>>
>> On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <[email protected]> wrote:
>> >
>> > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <[email protected]> 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.g...
Thanks,
Shlok Kyal
Attachments:
[application/octet-stream] topup.patch (11.3K, 2-topup.patch)
download | inline diff:
diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml
index 67a683e66c7..2898a5ea111 100644
--- a/doc/src/sgml/ref/pg_createsubscriber.sgml
+++ b/doc/src/sgml/ref/pg_createsubscriber.sgml
@@ -141,16 +141,22 @@ PostgreSQL documentation
<term><option>--logdir=<replaceable class="parameter">directory</replaceable></option></term>
<listitem>
<para>
- Specify the name of the log directory. A new directory is created with this name if it does not exist. A subdirectory with a timestamp indicating the time at which pg_createsubscriber was run will be created. The following two log files will be createdin the subdirectory with a umask of 077 so that access is disallowed to other users by default.
+ Specify the name of the log directory. A new directory is created with
+ this name if it does not exist. A subdirectory with a timestamp
+ indicating the time at which pg_createsubscriber was run will be created.
+ The following two log files will be created in the subdirectory with a
+ umask of 077 so that access is disallowed to other users by default.
<itemizedlist>
<listitem>
<para>
- pg_createsubscriber_server.log which captures logs related to stopping and starting the standby server,
+ pg_createsubscriber_server.log which captures logs related to stopping
+ and starting the standby server,
</para>
</listitem>
<listitem>
<para>
- pg_createsubscriber_internal.log which captures internal diagnostic output (validations, checks, etc.)
+ pg_createsubscriber_internal.log which captures internal diagnostic
+ output (validations, checks, etc.)
</para>
</listitem>
</itemizedlist>
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 3495bdb88ce..4450842d2b5 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -150,8 +150,9 @@ static void drop_existing_subscription(PGconn *conn, const char *subname,
const char *dbname);
static void get_publisher_databases(struct CreateSubscriberOptions *opt,
bool dbnamespecified);
-static void
- internal_log_file_write(const char *format,...) __attribute__((format(printf, 1, 2)));
+static void internal_log_file_write(enum pg_log_level level,
+ const char *format,...)
+ pg_attribute_printf(2, 3);
#define WAIT_INTERVAL 1 /* 1 second */
@@ -185,52 +186,75 @@ static bool recovery_ended = false;
static bool standby_running = false;
static bool recovery_params_set = false;
+#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{\
+#define pg_log_info(...) do { \
if (internal_log_file_fp != NULL) \
- internal_log_file_write(__VA_ARGS__); \
+ internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
else \
- pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__);\
+ pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
} while(0)
#undef pg_log_info_hint
-#define pg_log_info_hint(...) do{\
+#define pg_log_info_hint(...) do { \
if (internal_log_file_fp != NULL) \
- internal_log_file_write(__VA_ARGS__); \
+ internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
else \
- pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\
+ 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)) \
+#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{\
+#define pg_fatal(...) do { \
if (internal_log_file_fp != NULL) \
- internal_log_file_write(__VA_ARGS__); \
+ internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__); \
pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
exit(1); \
} while(0)
static void
-internal_log_file_write(const char *format,...)
+internal_log_file_write(enum pg_log_level level, const char *format,...)
{
- if (internal_log_file_fp != NULL)
- {
- va_list args;
+ va_list args;
- va_start(args, format);
- vfprintf(internal_log_file_fp, format, args);
- fprintf(internal_log_file_fp, "\n");
- fflush(internal_log_file_fp);
- va_end(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);
}
/*
@@ -264,22 +288,27 @@ logfile_open(const char *filename, const char *mode)
}
static void
-make_dir(char *dir)
+make_dir(const char *dir)
{
struct stat statbuf;
- if (stat(dir, &statbuf) != 0)
- if (errno == ENOENT)
- {
- if (mkdir(dir, S_IRWXU) == 0)
- pg_log_info("directory %s created", dir);
- else
- pg_fatal("could not create log directory \"%s\": %m", dir);
- }
+ if (stat(dir, &statbuf) == 0)
+ return;
+
+ if (errno != ENOENT)
+ pg_fatal("could not stat directory \"%s\": %m", dir);
+
+ if (mkdir(dir, S_IRWXU) == 0)
+ {
+ pg_log_info("directory %s created", dir);
+ return;
+ }
+
+ pg_fatal("could not create log directory \"%s\": %m", dir);
}
static void
-make_output_dirs(char *log_dir)
+make_output_dirs(const char *log_dir)
{
char timestamp[128];
struct timeval tval;
@@ -288,18 +317,31 @@ make_output_dirs(char *log_dir)
char timestamp_dir[MAXPGPATH];
int len;
+ /* Generate timestamp */
gettimeofday(&tval, NULL);
now = tval.tv_sec;
- strftime(timestamp, sizeof(timestamp), "%Y%m%dT%H%M%S", localtime_r(&now, &tmbuf));
+
+ strftime(timestamp, sizeof(timestamp), "%Y%m%dT%H%M%S",
+ localtime_r(&now, &tmbuf));
+
/* append milliseconds */
- snprintf(timestamp + strlen(timestamp), sizeof(timestamp) - strlen(timestamp),
- ".%03u", (unsigned int) (tval.tv_usec / 1000));
+ snprintf(timestamp + strlen(timestamp),
+ sizeof(timestamp) - strlen(timestamp), ".%03u",
+ (unsigned int) (tval.tv_usec / 1000));
+
log_timestamp = pg_strdup(timestamp);
+ /* Create base directory (ignore if exists) */
make_dir(log_dir);
+
+ /* 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);
+ pg_fatal("directory path for log files, %s/%s, is too long",
+ log_dir, timestamp);
+
+ /* Create timestamp directory */
make_dir(timestamp_dir);
}
@@ -2361,7 +2403,6 @@ main(int argc, char **argv)
char *consistent_lsn;
char pidfile[MAXPGPATH];
- char *internal_log_file;
pg_logging_init(argv[0]);
pg_logging_set_level(PG_LOG_WARNING);
@@ -2439,10 +2480,6 @@ main(int argc, char **argv)
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);
break;
case 'n':
dry_run = true;
@@ -2512,6 +2549,18 @@ main(int argc, char **argv)
}
}
+ 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);
+ }
+
/* Validate that --all is not used with incompatible options */
if (opt.all_dbs)
{
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 3a82f893e28..4ddfb621a5d 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -371,16 +371,26 @@ command_ok(
my @server_log_files = glob "$logdir/*/pg_createsubscriber_server.log";
is( scalar(@server_log_files), 1, "
pg_createsubscriber_server.log file was created");
-my $server_log_file_size = -s shift(@server_log_files);
+my $server_log_file_size = -s $server_log_files[0];
isnt($server_log_file_size, 0,
"pg_createsubscriber_server.log file not empty");
+my $server_log = slurp_file($server_log_files[0]);
+like(
+ $server_log,
+ qr/consistent recovery state reached/,
+ "server reached consistent recovery state");
my @internal_log_files = glob "$logdir/*/pg_createsubscriber_internal.log";
is( scalar(@internal_log_files), 1, "
pg_createsubscriber_internal.log file was created");
-my $internal_log_file_size = -s shift(@internal_log_files);
+my $internal_log_file_size = -s $internal_log_files[0];
isnt($internal_log_file_size, 0,
"pg_createsubscriber_internal.log file not empty");
+my $internal_log = slurp_file($internal_log_files[0]);
+like(
+ $internal_log,
+ qr/target server reached the consistent state/,
+ "log shows consistent state reached");
# Check if node S is still a standby
$node_s->start;
@@ -461,7 +471,8 @@ is(scalar(() = $stderr =~ /would create subscription/g),
# Create a user-defined publication, and a table that is not a member of that
# publication.
-$node_p->safe_psql($db1, qq(
+$node_p->safe_psql(
+ $db1, qq(
CREATE PUBLICATION test_pub3 FOR TABLE tbl1;
CREATE TABLE not_replicated (a int);
));
@@ -557,8 +568,7 @@ second row
third row),
"logical replication works in database $db1");
$result = $node_s->safe_psql($db1, 'SELECT * FROM not_replicated');
-is($result, qq(),
- "table is not replicated in database $db1");
+is($result, qq(), "table is not replicated in database $db1");
# Check result in database $db2
$result = $node_s->safe_psql($db2, 'SELECT * FROM tbl2');
@@ -572,8 +582,10 @@ my $sysid_s = $node_s->safe_psql('postgres',
isnt($sysid_p, $sysid_s, 'system identifier was changed');
# Verify that 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( $node_p->safe_psql(
+ $db2, "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'pub2'"),
+ '1',
+ "publication pub2 was created in $db2");
# Get subscription and publication names
$result = $node_s->safe_psql(
@@ -598,7 +610,7 @@ $result = $node_s->safe_psql(
)
);
-is($result, qq($db1|{test_pub3}
+is( $result, qq($db1|{test_pub3}
$db2|{pub2}),
"subscriptions use the correct publications");
[application/octet-stream] v9-0001-Add-a-new-argument-l-logdir-to-pg_createsubscribe.patch (12.8K, 3-v9-0001-Add-a-new-argument-l-logdir-to-pg_createsubscribe.patch)
download | inline diff:
From f10eab6b1cef5907be90fa66b879cddb679dfa82 Mon Sep 17 00:00:00 2001
From: Gyan Sreejith <[email protected]>
Date: Sun, 15 Mar 2026 18:40:15 -0400
Subject: [PATCH v9] Add a new argument -l <logdir> to pg_createsubscriber.
Enabling the option to write messages to log files in the specified directory.
A new directory is created if required. A subdirectory is created with timestamp as its name, and it will contain two new logfiles:
1. pg_createsubscriber_server.log - captures messages related to starting and stopping the standby server.
2. pg_createsubscriber_internal.log - captures internal diagnostic output from pg_createsubscriber.
For example, if we specify -l abc as an argument, and if the timestamp on running it is 20260119T204317.204, a directory abc is created if it doesn't exist already, with 20260119T204317.204 as its subdirectory and it will contain the two log files pg_createsubscriber_server.log and pg_createsubscriber_internal.log
---
doc/src/sgml/ref/pg_createsubscriber.sgml | 22 +++
src/bin/pg_basebackup/pg_createsubscriber.c | 165 +++++++++++++++++-
.../t/040_pg_createsubscriber.pl | 17 ++
3 files changed, 201 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml
index cf45ff3573d..67a683e66c7 100644
--- a/doc/src/sgml/ref/pg_createsubscriber.sgml
+++ b/doc/src/sgml/ref/pg_createsubscriber.sgml
@@ -136,6 +136,28 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-l <replaceable class="parameter">directory</replaceable></option></term>
+ <term><option>--logdir=<replaceable class="parameter">directory</replaceable></option></term>
+ <listitem>
+ <para>
+ Specify the name of the log directory. A new directory is created with this name if it does not exist. A subdirectory with a timestamp indicating the time at which pg_createsubscriber was run will be created. The following two log files will be createdin the subdirectory with a umask of 077 so that access is disallowed to other users by default.
+ <itemizedlist>
+ <listitem>
+ <para>
+ pg_createsubscriber_server.log which captures logs related to stopping and starting the standby server,
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ pg_createsubscriber_internal.log which captures internal diagnostic output (validations, checks, etc.)
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-n</option></term>
<term><option>--dry-run</option></term>
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 2bc84505aab..3495bdb88ce 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -49,10 +49,14 @@
#define INCLUDED_CONF_FILE "pg_createsubscriber.conf"
#define INCLUDED_CONF_FILE_DISABLED INCLUDED_CONF_FILE ".disabled"
+#define SERVER_LOG_FILE_NAME "pg_createsubscriber_server"
+#define INTERNAL_LOG_FILE_NAME "pg_createsubscriber_internal"
+
/* Command-line options */
struct CreateSubscriberOptions
{
char *config_file; /* configuration file */
+ char *log_dir; /* log directory name */
char *pub_conninfo_str; /* publisher connection string */
char *socket_dir; /* directory for Unix-domain socket, if any */
char *sub_port; /* subscriber port number */
@@ -146,6 +150,9 @@ static void drop_existing_subscription(PGconn *conn, const char *subname,
const char *dbname);
static void get_publisher_databases(struct CreateSubscriberOptions *opt,
bool dbnamespecified);
+static void
+ internal_log_file_write(const char *format,...) __attribute__((format(printf, 1, 2)));
+
#define WAIT_INTERVAL 1 /* 1 second */
@@ -167,6 +174,10 @@ static pg_prng_state prng_state;
static char *pg_ctl_path = NULL;
static char *pg_resetwal_path = NULL;
+static FILE *internal_log_file_fp = NULL; /* File ptr to log all messages to */
+static char *log_timestamp = NULL; /* Timestamp to be used in all log file
+ * names */
+
/* standby / subscriber data directory */
static char *subscriber_dir = NULL;
@@ -174,6 +185,123 @@ static bool recovery_ended = false;
static bool standby_running = false;
static bool recovery_params_set = false;
+#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)
+
+#undef pg_fatal
+#define pg_fatal(...) do{\
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(__VA_ARGS__); \
+ pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+ exit(1); \
+} while(0)
+
+static void
+internal_log_file_write(const char *format,...)
+{
+ if (internal_log_file_fp != NULL)
+ {
+ va_list args;
+
+ va_start(args, format);
+ vfprintf(internal_log_file_fp, format, args);
+ fprintf(internal_log_file_fp, "\n");
+ fflush(internal_log_file_fp);
+ va_end(args);
+ }
+}
+
+/*
+ * Open a new logfile with proper permissions.
+ * From src/backend/postmaster/syslogger.c
+ */
+static FILE *
+logfile_open(const char *filename, const char *mode)
+{
+ FILE *fh;
+ mode_t oumask;
+
+ oumask = umask((mode_t) ((~(S_IRUSR | S_IWUSR)) & (S_IRWXU | S_IRWXG | S_IRWXO)));
+ fh = fopen(filename, mode);
+ umask(oumask);
+
+ if (fh)
+ {
+ setvbuf(fh, NULL, PG_IOLBF, 0);
+
+#ifdef WIN32
+ /* use CRLF line endings on Windows */
+ _setmode(_fileno(fh), _O_TEXT);
+#endif
+ }
+ else
+ pg_fatal("could not open log file \"%s\": %m",
+ filename);
+
+ return fh;
+}
+
+static void
+make_dir(char *dir)
+{
+ struct stat statbuf;
+
+ if (stat(dir, &statbuf) != 0)
+ if (errno == ENOENT)
+ {
+ if (mkdir(dir, S_IRWXU) == 0)
+ pg_log_info("directory %s created", dir);
+ else
+ pg_fatal("could not create log directory \"%s\": %m", dir);
+ }
+}
+
+static void
+make_output_dirs(char *log_dir)
+{
+ char timestamp[128];
+ struct timeval tval;
+ time_t now;
+ struct tm tmbuf;
+ char timestamp_dir[MAXPGPATH];
+ int len;
+
+ gettimeofday(&tval, NULL);
+ now = tval.tv_sec;
+ strftime(timestamp, sizeof(timestamp), "%Y%m%dT%H%M%S", localtime_r(&now, &tmbuf));
+ /* append milliseconds */
+ snprintf(timestamp + strlen(timestamp), sizeof(timestamp) - strlen(timestamp),
+ ".%03u", (unsigned int) (tval.tv_usec / 1000));
+ log_timestamp = pg_strdup(timestamp);
+
+ make_dir(log_dir);
+ 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);
+ make_dir(timestamp_dir);
+}
/*
* Clean up objects created by pg_createsubscriber.
@@ -269,6 +397,12 @@ cleanup_objects_atexit(void)
if (standby_running)
stop_standby_server(subscriber_dir);
+
+ if (internal_log_file_fp != NULL)
+ {
+ fclose(internal_log_file_fp);
+ internal_log_file_fp = NULL;
+ }
}
static void
@@ -283,6 +417,7 @@ usage(void)
" databases and databases that don't allow connections\n"));
printf(_(" -d, --database=DBNAME database in which to create a subscription\n"));
printf(_(" -D, --pgdata=DATADIR location for the subscriber data directory\n"));
+ printf(_(" -l, --logdir=LOGDIR location for the new log directory\n"));
printf(_(" -n, --dry-run dry run, just show what would be done\n"));
printf(_(" -p, --subscriber-port=PORT subscriber port number (default %s)\n"), DEFAULT_SUB_PORT);
printf(_(" -P, --publisher-server=CONNSTR publisher connection string\n"));
@@ -702,6 +837,7 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
bool crc_ok;
struct timeval tv;
+ char *out_file;
char *cmd_str;
pg_log_info("modifying system identifier of subscriber");
@@ -735,8 +871,14 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
else
pg_log_info("running pg_resetwal on the subscriber");
- cmd_str = psprintf("\"%s\" -D \"%s\" > \"%s\"", pg_resetwal_path,
- subscriber_dir, DEVNULL);
+
+ 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);
pg_log_debug("pg_resetwal command is: %s", cmd_str);
@@ -1650,6 +1792,9 @@ start_standby_server(const struct CreateSubscriberOptions *opt, bool restricted_
if (restrict_logical_worker)
appendPQExpBufferStr(pg_ctl_cmd, " -o \"-c max_logical_replication_workers=0\"");
+ if (opt->log_dir != NULL)
+ appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
+
pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data);
rc = system(pg_ctl_cmd->data);
pg_ctl_status(pg_ctl_cmd->data, rc);
@@ -2181,6 +2326,7 @@ main(int argc, char **argv)
{"all", no_argument, NULL, 'a'},
{"database", required_argument, NULL, 'd'},
{"pgdata", required_argument, NULL, 'D'},
+ {"logdir", required_argument, NULL, 'l'},
{"dry-run", no_argument, NULL, 'n'},
{"subscriber-port", required_argument, NULL, 'p'},
{"publisher-server", required_argument, NULL, 'P'},
@@ -2215,6 +2361,7 @@ main(int argc, char **argv)
char *consistent_lsn;
char pidfile[MAXPGPATH];
+ char *internal_log_file;
pg_logging_init(argv[0]);
pg_logging_set_level(PG_LOG_WARNING);
@@ -2239,6 +2386,7 @@ main(int argc, char **argv)
/* Default settings */
subscriber_dir = NULL;
opt.config_file = NULL;
+ opt.log_dir = NULL;
opt.pub_conninfo_str = NULL;
opt.socket_dir = NULL;
opt.sub_port = DEFAULT_SUB_PORT;
@@ -2267,7 +2415,7 @@ main(int argc, char **argv)
get_restricted_token();
- while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:TU:v",
+ while ((c = getopt_long(argc, argv, "ad:D:l:np:P:s:t:TU:v",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2288,6 +2436,14 @@ main(int argc, char **argv)
subscriber_dir = pg_strdup(optarg);
canonicalize_path(subscriber_dir);
break;
+ 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);
+ break;
case 'n':
dry_run = true;
break;
@@ -2621,5 +2777,8 @@ main(int argc, char **argv)
pg_log_info("Done!");
+ if (internal_log_file_fp != NULL)
+ fclose(internal_log_file_fp);
+
return 0;
}
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0c27fca7bb7..3a82f893e28 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -14,6 +14,7 @@ program_version_ok('pg_createsubscriber');
program_options_handling_ok('pg_createsubscriber');
my $datadir = PostgreSQL::Test::Utils::tempdir;
+my $logdir = PostgreSQL::Test::Utils::tempdir + "/logdir";
# Generate a database with a name made of a range of ASCII characters.
# Extracted from 002_pg_upgrade.pl.
@@ -362,9 +363,25 @@ command_ok(
'--subscription' => 'sub2',
'--database' => $db1,
'--database' => $db2,
+ '--logdir' => $logdir,
],
'run pg_createsubscriber --dry-run on node S');
+# 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 $server_log_file_size = -s shift(@server_log_files);
+isnt($server_log_file_size, 0,
+ "pg_createsubscriber_server.log file not empty");
+
+my @internal_log_files = glob "$logdir/*/pg_createsubscriber_internal.log";
+is( scalar(@internal_log_files), 1, "
+ pg_createsubscriber_internal.log file was created");
+my $internal_log_file_size = -s shift(@internal_log_files);
+isnt($internal_log_file_size, 0,
+ "pg_createsubscriber_internal.log file not empty");
+
# Check if node S is still a standby
$node_s->start;
is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
--
2.34.1
view thread (65+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
In-Reply-To: <CANhcyEWaCV7B54oOhuE_BhVCF887h23jKmkHDDWvJRKZ=pBbBQ@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