Re: [Proposal] Adding Log File Capability to pg_createsubscriber

On Sun, Mar 22, 2026 at 4:39 AM Gyan Sreejith <gyan(dot)sreejith(at)gmail(dot)com> wrote:
>
>
> On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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