Re: pg_createsubscriber --dry-run logging concerns

On Sat, Nov 1, 2025 at 5:02 AM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> On 2025-Oct-09, Peter Smith wrote:
>
> > Please see the v3 patches.
>
> Okay, I have pushed 0002 to 18 and master. I wanted to backpatch to 17
> but there are too many conflicts there.
>

Thanks for pushing!

> I'm not opposed to 0001 (to master only), but I think the lines of
> dashes are a little excessively noisy. Are there other opinions on
> that? Note that vacuumlo also has it, with no dashes.

It was intentionally "noisy" with dashes to make it impossible to
accidentally overlook dry-run mode in the logs. But if that's contrary
to the way Postgres does things I am fine if you want to remove the
dashes.

>
> I'll mark the commitfest item for version 19.
>

> While looking this over, I noticed that we define USEC_PER_SEC, but why?
> We already have USECS_PER_SEC, so let's use that. And WaitPMResult
> seems an overgrown boolean, so how about we remove it? Patch for these
> things attached. Thoughts?
>

I took a quick look at your follow-up patches and below are some comments:

//////////
Patch 0001 - USECS_PER_SEC
//////////

+1 to use USECS_PER_SEC from timestamps.h

1.
-#define USEC_PER_SEC 1000000
-
-#define WAITS_PER_SEC 10 /* should divide USEC_PER_SEC evenly */
+#define WAITS_PER_SEC 10 /* should divide USECS_PER_SEC evenly */

static bool do_wait = true;
static int wait_seconds = DEFAULT_WAIT;
@@ -594,6 +593,7 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
{
int i;

+ static_assert(USECS_PER_SEC % WAITS_PER_SEC == 0);

Maybe that static assert might be better placed adjacent to the
#define to enforce that comment about "divide evenly".

SUGGESTION
#define WAITS_PER_SEC 10

/* WAITS_PER_SEC should divide USECS_PER_SEC evenly */
StatiAssertDecl(USECS_PER_SEC % WAITS_PER_SEC == 0);

//////////
Patch 0002 -- boolean
//////////

LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia