Re: Documentation Edits for pg_createsubscriber

Thank you for the quick reply.

On Mon, Mar 10, 2025 at 7:56 PM Hayato Kuroda (Fujitsu) <
kuroda(dot)hayato(at)fujitsu(dot)com> wrote:

> Dear David,
>
> > The main thing I noted is that our synopsis is simply wrong regarding the
> > optionality of the --database option. I went with a fix that included
> only
> > showing the two mandatory options and adding a paragraph to usage
> explaining
> > that --database exists and how to use it. Our existing example also
> covers
> > that common usage.
>
> According to the code, -D and -P is anyway needed, but -d can be omitted
> when -P
> contians dbname option. And you wanted to follow the rule, right?
> IIUC shorter synopsis is always welcomed.
>

I'm having second thoughts here a bit as I absorb more. I'm not really
sure why we added --publication-name etc... but I was giving them more
weight than is probably needed. They don't seem like something a typical
user would care about. I'll probably add back -d but leave mention of all
the long-form option names out still. Or, my other thought, two lines of
commands and explain the single-database and multi-database mechanics that
distinguish them in the description.

> > It took me a bit, and reading implementation code, to understand what
> was meant
> > by "Additional recovery parameters are added to avoid ...". I decided
> that
> > detail seemed unnecessary for the user-facing documentation - supposedly
> they
> > will never see the recovery file if everything works OK (and if they do
> the
> > various empty string settings should be reasonably self-evident). If we
> do
> > want to keep it I'll try to find a better wording.
>
> To confirm; users can run SHOW command after the conversion and they can
> see
> our setting. Do you think it is a user-facing?
>

And they will see empty strings (or blanks or whatever the defaults are)
for everything except LSN, like they always do, and not realize or care
that our code generated file actually contained lines for them. (I haven't
actually experimented here though.) But in normal use they have no reason
to inspect the state data generated by this process so which it is
accessible as a matter of design it is not practically user-facing for a
user of this application. The only state they need to manage is that their
WAL retention for their standby is large enough to execute the needed
point-in-time recovery.

> Here are my comments. (I ran Grammarly to check your sentences)
>
> 01.
> Your patch breaks 80-column-per-line rule. Please check it.
>

Ok, though it seems like a very loosely enforced one in the documentation.

> 02.
> ```
> + The minimal command shown above...
> ```

> "shown" can be removed.
>
>
ok

> 03.
> ```
> + then created using auto-generated names, which can be overriden
> ```

> s/overriden/overridden/.
>
>
ok

04.
> ```
> + using the corresponding long-form command-line options, once each per
> database.
> ```
>
> "," and "each" can be removed.
>
>
I'll ponder this one and explain if I leave it as-is.

> 05.
> ```
> + does not perform an initial data copy. Instead it relies on a
> LSN-based
> ```
>
> Latter sentence can be "Inseted, it relies on an LSN-based..."
>
>
probably, though I have the vague impression that this entire page has too
many unneeded commas right now. But not obviously so.

> 06.
> ```
> + <para>
> + Note, the configuration setting <literal>port</literal> in this
> file is effectively ignored
> + as <application>pg_createsubscriber</application> always specifies
> the target listening port
> + on the <application>pg_ctl</application> command-line.
> + Use the <option>-p</option> option to specify which port the
> target server should listen on.
> </para>
> ```
>
> Can you clarify the <literal>port</literal> in the config file is ignored
> only
> while the pg_createsuscriber commands?
>

I'm not following what you want me to do here? This note only applies
while pg_createsubscriber manages the target service which the first second
basically states directly.

> 07.
> ```
> + <application>pg_createsubscriber</application> is designed have full
> and
> ```
>
> s/is designed have/is designed to have/.
>

yep

David J.