public inbox for [email protected]  

help / color / mirror / Atom feed

From: shveta malik <[email protected]>
To: Gyan Sreejith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Kuroda, Hayato/黒田 隼人 <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Date: Mon, 23 Mar 2026 15:25:04 +0530
Message-ID: <CAJpy0uCrGenynPf_0fdhh+M_wACc3ktzD+GhjZ4fYk5k1VUeKg@mail.gmail.com> (raw)
In-Reply-To: <CAEqnbaVF0yXQk=VVzr-8V23E=iUpUqyah6hxx2fy0ZVJh+CaGA@mail.gmail.com>
References: <OSOPR01MB1215385EAC6A76650444E40B0F54FA@OSOPR01MB12153.jpnprd01.prod.outlook.com>
	<CAEqnbaU=EgqhxEx0ig4TdY8pdt0Vn+vmCJBuoORRHOzovW9dWA@mail.gmail.com>
	<TYRPR01MB12156F431E4B83ECFF14034BDF54CA@TYRPR01MB12156.jpnprd01.prod.outlook.com>
	<CAEqnbaXb-TKUm7P-=_zrgQ=shRXkkZscPOHEL9OS6Cb2V8YT8Q@mail.gmail.com>
	<CAA4eK1JiAzaY7UYnya7uDhX-kAL1PrsOAxBtys7c35t4q4H3_A@mail.gmail.com>
	<CAEqnbaVF0yXQk=VVzr-8V23E=iUpUqyah6hxx2fy0ZVJh+CaGA@mail.gmail.com>

On Sun, Mar 22, 2026 at 4:39 AM Gyan Sreejith <[email protected]> wrote:
>
>
> On Sat, Mar 21, 2026 at 5:57 AM Amit Kapila <[email protected]> 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

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], [email protected]
  Subject: Re: [Proposal] Adding Log File Capability to pg_createsubscriber
  In-Reply-To: <CAJpy0uCrGenynPf_0fdhh+M_wACc3ktzD+GhjZ4fYk5k1VUeKg@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