public inbox for [email protected]
help / color / mirror / Atom feedFrom: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Euler Taveira' <[email protected]>
To: Amit Kapila <[email protected]>
To: Shlok Kyal <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: FabrÃzio de Royes Mello <[email protected]>
Cc: vignesh C <[email protected]>
Subject: RE: speed up a logical replica setup
Date: Fri, 7 Jun 2024 09:17:59 +0000
Message-ID: <OSBPR01MB255268FA2E655B676B56ECC2F5FB2@OSBPR01MB2552.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CANhcyEW6-dH28gLbFc5XpDTJ6JPizU+t5g-aKUWJBf5W_Zriqw@mail.gmail.com>
<[email protected]>
<CAA4eK1JtnzqYNQ_S+mj_XsPfgk7szxegKiVQq4-8fY1wUcUnpg@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CANhcyEWdTTO71v+GFYUTsOOUvHus1qccg88bRmsU9KykJqjSrQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<CALj2ACV85cn-cTNCTP=bPehYo9d2ddvZtfGhGq==0K+65SvufQ@mail.gmail.com>
<[email protected]>
<TYCPR01MB12077907C052EC329C45AE198F5362@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAA4eK1KPoLUUGzjw9eEQd56LUAf2QQj8BqMcsoSe6vycB5CYUQ@mail.gmail.com>
<[email protected]>
<CAA4eK1JJq_ER6Kq_H=jKHR75QPRd8y9_D=RtYw=aPYKMfqLi9A@mail.gmail.com>
<CAA4eK1KdCb+5sjYu6qCMXXdCX1y_ihr8kFzMozq0=P=auYxgog@mail.gmail.com>
<CANhcyEV6q1Vhd37i1axUeScLi0UAGVxta1LDa0BV0Eh--TcPMg@mail.gmail.com>
<CAA4eK1JaQcAZ=n513-4VDW4ujE9T9f007fczNCpVDbg1smhT4g@mail.gmail.com>
<[email protected]>
<[email protected]>
Dear Euler,
Thanks for making the follow-up patch! I was looking forward to your updates.
I think this patch set is the solution for the found buildfarm error. However,
there are remained claims raised by others. You should reply what you think for
them. At least:
1) There are some misleading messages [1]. I think v3-0005 patch set can solve
the issue.
2) pg_createsubscriber may fail If the primary has subscriptions [2]. IIUC
possible approaches are A)"keep subscriptions disabled at the end",
B)"by default drop the pre-existing subscriptions",
C) "do nothing, just document the risk".
> Before sending this email I realized that I did nothing about physical
> replication slots on the standby. I think we should also remove them too
> unconditionally.
I also considered around here, but it might be difficult to predict the
expectation by users. Can we surely say that it's not intentional one? Regarding
the failover slot, it is OK because that's meaningful only on the standby,
but not sure other slots. I personally think we can keep current spec, but
how do other think?
Below parts are comments for each patches.
0001
Basically LGTM. I was bit confused because the default timeout is not set, but
it seemed to follow the suggestion by Tomas [3].
0002
If you want to improve the commit message, please add that sync_replication_slots
is disabled during the conversion.
0003
Confirmed it followed the discussion [4].
0004
Basically LGTM.
Other minor comments are included by the attached diff file. It contains changes
to follow conventions and pgindent/pgperltidy.
[1]: https://www.postgresql.org/message-id/CAA4eK1J2fAvsJ2HihbWJ_GxETd6sdqSMrZdCVJEutRZRpm1MEQ%40mail.gma...
[2]: https://www.postgresql.org/message-id/CANhcyEWvimA1-f6hSrA%3D9qkfR5SonFb56b36M%2B%2BvT%3DLiFj%3D76g%...
[3]: https://www.postgresql.org/message-id/5d5dd4cd-6359-4109-88e8-c8e13035ae16%40enterprisedb.com
[4]: https://www.postgresql.org/message-id/CAA4eK1LZxYxcbeiOn3Q5hjXVtZKhJWj-fQtndAeTCvZrPev8BA%40mail.gma...
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/
Attachments:
[application/octet-stream] minor_fix_by_kuroda.diff (6.6K, ../OSBPR01MB255268FA2E655B676B56ECC2F5FB2@OSBPR01MB2552.jpnprd01.prod.outlook.com/2-minor_fix_by_kuroda.diff)
download | inline diff:
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index b0233786d4..77ae17073a 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -1152,13 +1152,13 @@ drop_failover_replication_slots(struct LogicalRepInfo *dbinfo)
conn = connect_database(dbinfo[0].subconninfo, false);
if (conn != NULL)
{
- /* Get failover replication slot names. */
+ /* Get failover replication slot names */
res = PQexec(conn,
"SELECT slot_name FROM pg_catalog.pg_replication_slots WHERE failover");
if (PQresultStatus(res) == PGRES_TUPLES_OK)
{
- /* Remove failover replication slots from subscriber. */
+ /* Remove failover replication slots from subscriber */
for (int i = 0; i < PQntuples(res); i++)
drop_replication_slot(conn, &dbinfo[0], PQgetvalue(res, i, 0));
}
@@ -1310,7 +1310,11 @@ start_standby_server(const struct CreateSubscriberOptions *opt, bool restricted_
PQExpBuffer pg_ctl_cmd = createPQExpBuffer();
int rc;
- appendPQExpBuffer(pg_ctl_cmd, "\"%s\" start -D \"%s\" -s -o \"-c sync_replication_slots=off\"",
+ /*
+ * FIXME: appending "sync_replication_slots=off" is not trivial for reader.
+ * Please separate it into the later part with comments.
+ */
+ appendPQExpBuffer(pg_ctl_cmd, "\"%s\" start -D \"%s\" -s",
pg_ctl_path, subscriber_dir);
if (restricted_access)
{
@@ -1333,6 +1337,24 @@ start_standby_server(const struct CreateSubscriberOptions *opt, bool restricted_
if (opt->config_file != NULL)
appendPQExpBuffer(pg_ctl_cmd, " -o \"-c config_file=%s\"",
opt->config_file);
+
+ /*
+ * FIXME: please describe why the sync_replication_slots is set to false.
+ * I.e.,
+ */
+
+ /*
+ * We set up some recovery parameters in the middle part of the
+ * pg_createsubscriber to ensure all changes are caught up. At that time,
+ * primary_conninfo will be set without the dbname attribute. Basically, it
+ * works well, but if the slot sync is enabled, the slot sync worker will
+ * exit with ERROR because the process requires dbname within
+ * primary_conninfo. To avoid the error, we override sync_replication_slots
+ * to off. The synchronization won't happen after the conversion, so it is
+ * acceptable to stop here as well.
+ */
+ appendPQExpBuffer(pg_ctl_cmd, " -o \"-c sync_replication_slots=off\"");
+
pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data);
rc = system(pg_ctl_cmd->data);
pg_ctl_status(pg_ctl_cmd->data, rc);
@@ -2068,6 +2090,7 @@ main(int argc, char **argv)
/* Check if the primary server is ready for logical replication */
pub_failed = check_publisher(dbinfo);
+ /* Exit if either of them is not ready */
if (pub_failed || sub_failed)
exit(1);
@@ -2111,8 +2134,8 @@ main(int argc, char **argv)
setup_subscriber(dbinfo, consistent_lsn);
/*
- * Remove primary_slot_name if it exists on primary. This replication slot
- * has no use after the transformation.
+ * Remove primary_slot_name if it exists on primary. This replication
+ * slot has no use after the transformation.
*/
drop_primary_replication_slot(dbinfo, primary_slot_name);
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0516d4e17e..cc65942dc4 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -266,28 +266,43 @@ $node_p->restart;
# Create failover slot to test its removal
my $fslotname = 'failover_slot';
$node_p->safe_psql('pg1',
- "SELECT pg_create_logical_replication_slot('$fslotname', 'pgoutput', false, false, true)");
+ "SELECT pg_create_logical_replication_slot('$fslotname', 'pgoutput', false, false, true)"
+);
$node_s->start;
$node_s->safe_psql('postgres', "SELECT pg_sync_replication_slots()");
-my $result = $node_s->safe_psql('postgres', "SELECT slot_name FROM pg_replication_slots WHERE slot_name = '$fslotname' AND synced AND NOT temporary");
+my $result = $node_s->safe_psql('postgres',
+ "SELECT slot_name FROM pg_replication_slots WHERE slot_name = '$fslotname' AND synced AND NOT temporary"
+);
is($result, 'failover_slot', 'failover slot is synced');
$node_s->stop;
# dry run mode on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr('pg1'), '--socket-directory',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'pub2', '--subscription',
- 'sub1', '--subscription',
- 'sub2', '--database',
- 'pg1', '--database',
+ 'pg_createsubscriber',
+ '--verbose',
+ '--recovery-timeout',
+ "$PostgreSQL::Test::Utils::timeout_default",
+ '--dry-run',
+ '--pgdata',
+ $node_s->data_dir,
+ '--publisher-server',
+ $node_p->connstr('pg1'),
+ '--socket-directory',
+ $node_s->host,
+ '--subscriber-port',
+ $node_s->port,
+ '--publication',
+ 'pub1',
+ '--publication',
+ 'pub2',
+ '--subscription',
+ 'sub1',
+ '--subscription',
+ 'sub2',
+ '--database',
+ 'pg1',
+ '--database',
'pg2'
],
'run pg_createsubscriber --dry-run on node S');
@@ -314,18 +329,30 @@ command_ok(
# Run pg_createsubscriber on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--verbose', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr('pg1'), '--socket-directory',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'Pub2', '--replication-slot',
- 'replslot1', '--replication-slot',
- 'replslot2', '--database',
- 'pg1', '--database',
+ 'pg_createsubscriber',
+ '--verbose',
+ '--recovery-timeout',
+ "$PostgreSQL::Test::Utils::timeout_default",
+ '--verbose',
+ '--pgdata',
+ $node_s->data_dir,
+ '--publisher-server',
+ $node_p->connstr('pg1'),
+ '--socket-directory',
+ $node_s->host,
+ '--subscriber-port',
+ $node_s->port,
+ '--publication',
+ 'pub1',
+ '--publication',
+ 'Pub2',
+ '--replication-slot',
+ 'replslot1',
+ '--replication-slot',
+ 'replslot2',
+ '--database',
+ 'pg1',
+ '--database',
'pg2'
],
'run pg_createsubscriber on node S');
@@ -358,7 +385,8 @@ $node_s->wait_for_subscription_sync($node_p, $subnames[1]);
# Confirm the failover slot has been removed
$result = $node_s->safe_psql('pg1',
- "SELECT count(*) FROM pg_replication_slots WHERE slot_name = '$fslotname'");
+ "SELECT count(*) FROM pg_replication_slots WHERE slot_name = '$fslotname'"
+);
is($result, qq(0), 'failover slot was removed');
# Check result on database pg1
view thread (107+ 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], [email protected], [email protected], [email protected], [email protected]
Subject: RE: speed up a logical replica setup
In-Reply-To: <OSBPR01MB255268FA2E655B676B56ECC2F5FB2@OSBPR01MB2552.jpnprd01.prod.outlook.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