Re: speed up a logical replica setup

On 18.03.24 06:43, Hayato Kuroda (Fujitsu) wrote:
> 02.
>
> "The main difference between the logical replication setup and pg_createsubscriber
> is the initial data copy."
>
> Grammarly suggests:
> "The initial data copy is the main difference between the logical replication
> setup and pg_createsubscriber."

I think that change is worse.

> 09.
>
> "The source server must accept connections from the target server. The source server must not be in recovery."
>
> Grammarly suggests:
> "The source server must accept connections from the target server and not be in recovery."

I think the previous version is better.

> 17.
>
> "specifies promote"
>
> We can do double-quote for the word promote.

The v30 patch has <literal>promote</literal>, which I think is adequate.

> 19.
>
> "is accepting read-write transactions"
>
> Grammarly suggests:
> "accepts read-write transactions"

I like the first one better.

> 20.
>
> New options must be also documented as well. This helps not only users but also
> reviewers.
> (Sometimes we cannot identify that the implementation is intentinal or not.)

Which ones are missing?

> 21.
>
> Also, not sure the specification is good. I preferred to specify them by format
> string. Because it can reduce the number of arguments and I cannot find use cases
> which user want to control the name of objects.
>
> However, your approach has a benefit which users can easily identify the generated
> objects by pg_createsubscriber. How do other think?

I think listing them explicitly is better for the first version. It's
simpler to implement and more flexible.

> 22.
>
> ```
> #define BASE_OUTPUT_DIR "pg_createsubscriber_output.d"
> ```
>
> No one refers the define.

This is gone in v30.

> 23.
>
> ```
> } CreateSubscriberOptions;
> ...
> } LogicalRepInfo;
> ```
>
> Declarations after the "{" are not needed, because we do not do typedef.

Yeah, this is actually wrong, because as it is written now, it defines
global variables.

> 22.
>
> While seeing definitions of functions, I found that some pointers are declared
> as const, but others are not. E.g., "char *lsn" in setup_recovery() won' be
> changed but not the constant. Is it just missing or is there another rule?

Yes, more could be done here. I have attached a patch for this. (This
also requires the just-committed 48018f1d8c.)

> 24.
>
> ```
> /* standby / subscriber data directory */
> static char *subscriber_dir = NULL;
> ```
>
> It is bit strange that only subscriber_dir is a global variable. Caller requires
> the CreateSubscriberOptions as an argument, except cleanup_objects_atexit() and
> main. So, how about makeing `CreateSubscriberOptions opt` to global one?

Fewer global variables seem preferable. Only make global as needed.

> 30.
>
> ```
> if (num_replslots == 0)
> dbinfo[i].replslotname = pg_strdup(genname);
> ```
>
> I think the straightforward way is to use the name of subscription if no name
> is specified. This follows the rule for CREATE SUBSCRIPTION.

right

> 31.
>
> ```
> /* Create replication slot on publisher */
> if (lsn)
> pg_free(lsn);
> ```
>
> I think allocating/freeing memory is not so efficient.
> Can we add a flag to create_logical_replication_slot() for controlling the
> returning value (NULL or duplicated string)? We can use the condition (i == num_dbs-1)
> as flag.

Nothing is even using the return value of
create_logical_replication_slot(). I think this can be removed altogether.

> 34.
>
> ```
> {"config-file", required_argument, NULL, 1},
> {"publication", required_argument, NULL, 2},
> {"replication-slot", required_argument, NULL, 3},
> {"subscription", required_argument, NULL, 4},
> ```
>
> The ordering looks strange for me. According to pg_upgarade and pg_basebackup,
> options which do not have short notation are listed behind.
>
> 35.
>
> ```
> opt.sub_port = palloc(16);
> ```
>
> Per other lines, pg_alloc() should be used.

Even better psprintf().

> 37.
>
> ```
> /* Register a function to clean up objects in case of failure */
> atexit(cleanup_objects_atexit);
> ```
>
> Sorry if we have already discussed. I think the registration can be moved just
> before the boot of the standby. Before that, the callback will be no-op.

But it can also stay where it is. What is the advantage of moving it later?

> 40.
>
> ```
> * XXX this code was extracted from BootStrapXLOG().
> ```
>
> So, can we extract the common part to somewhere? Since system identifier is related
> with the controldata file, I think it can be located in controldata_util.c.

Let's leave it as is for this PG release.