RE: speed up a logical replica setup - Mailing list pgsql-hackers

Dear Euler,

Sorry for disturbing your work and thanks for updates.
I will review your patch again.

>
* 0001 patch
It is almost the same as v3-0001, which was posted by Euler.
An unnecessary change for Mkvcbuild.pm (this file was removed) was ignored.

v5 removes the MSVC support.
>

Confirmed that the patch could be applied.

>
* 0002 patch
This contains small fixes to keep complier quiet.

I applied it. Although, I used a different approach for format specifier.
>

Good, all warnings were removed. However, the patch failed to pass tests on FreeBSD twice.
 I'm quite not sure the ERROR, but is it related with us?

>
* 0003 patch
This addresses comments posted to -hackers. For now, this does not contain a doc.
Will add if everyone agrees these idea.

I didn't review all items but ...

1.
An option --port was added to control the port number for physical standby.
Users can specify a port number via the option, or an environment variable PGSUBPORT.
If not specified, a fixed value (50111) would be used.

My first reaction as a new user would be: why do I need to specify a port if my
--subscriber-conninfo already contains a port? Ugh. I'm wondering if we can do
it behind the scenes. Try a range of ports.
>

My initial motivation of the setting was to avoid establishing connections
during the pg_subscriber. While considering more, I started to think that
--subscriber-conninfo may not be needed. pg_upgrade does not requires the
string: it requries username, and optionally port number (which would be used
during the upgrade) instead. The advantage of this approach is that we do not
have to parse the connection string.
How do you think?

>
2.
A FATAL error would be raised if --subscriber-conninfo specifies non-local server.

Extra protection is always good. However, let's make sure this code path is
really useful. I'll think a bit about it.
>

OK, I can wait your consideration. Note that if we follow the pg_ugprade style,
we may able to reuse check_pghost_envvar().

>
3.
Options -o/-O were added to specify options for publications/subscriptions.

Flexibility is cool. However, I think the cost benefit of it is not good. You
have to parse the options to catch preliminary errors. Things like publish only
delete and subscription options that conflicts with the embedded ones are
additional sources of failure.
>

As I already replied, let's stop doing it once. We can resume based on the requirement.

>
4.
Made standby to save their output to log file.

It was already done in v5. I did in a different way.
>

Good. I felt that yours were better. BTW, can we record outputs by pg_subscriber to a file as well?
pg_upgrade did similar thing. Thought?

>
5.
Unnecessary Assert in drop_replication_slot() was removed.

Instead, I fixed the code and keep the assert.
>

Cool.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED