On Thu, Jul 11, 2024, at 2:00 PM, Alexander Lakhin wrote: > Hello Amit and Hou-San, > > 11.07.2024 13:21, Amit Kapila wrote: > > > > We don't wait for the xmin to catch up corresponding to this insert > > and I don't know if there is a way to do that. So, we should move this > > Insert to after the call to pg_sync_replication_slots(). It won't > > impact the general test of pg_createsubscriber. > > > > Thanks to Hou-San for helping me in the analysis of this BF failure. > > Thank you for investigating that issue! > > May I ask you to look at another failure of the test occurred today [1]? >
Thanks for the report! You are observing the same issue that Amit explained in [1]. The pg_create_logical_replication_slot returns the EndRecPtr (see slot->data.confirmed_flush in DecodingContextFindStartpoint()). EndRecPtr points to the next record and it is a future position for an idle server. That's why the recovery takes some time to finish because it is waiting for an activity to increase the LSN position. Since you modified LOG_SNAPSHOT_INTERVAL_MS to create additional WAL records soon, the EndRecPtr position is reached rapidly and the recovery ends quickly. Hayato proposes a patch [2] to create an additional WAL record that has the same effect from you little hack: increase the LSN position to allow the recovery finishes soon. I don't like the solution although it seems simple to implement. As Amit said if we know the ReadRecPtr, we could use it as consistent LSN. The problem is that it is used by logical decoding but it is not exposed. [reading the code...] When the logical replication slot is created, restart_lsn points to the lastReplayedEndRecPtr (see ReplicationSlotReserveWal()) that is the last record replayed. Since the replication slots aren't in use, we could use the restart_lsn from the last replication slot as a consistent LSN. I'm attaching a patch that implements it.It runs in 6s instead of 26s. [1] https://www.postgresql.org/message-id/CAA4eK1%2Bp%2B7Ag6nqdFRdqowK1EmJ6bG-MtZQ_54dnFBi%3D_OO5RQ%40mail.gmail.com [2] https://www.postgresql.org/message-id/OSBPR01MB25521B15BF950D2523BBE143F5D32%40OSBPR01MB2552.jpnprd01.prod.outlook.com -- Euler Taveira EDB https://www.enterprisedb.com/
From a3be65c9bea08d3c7ffd575342a879d37b28b9a8 Mon Sep 17 00:00:00 2001 From: Euler Taveira <eu...@eulerto.com> Date: Thu, 11 Jul 2024 19:59:56 -0300 Subject: [PATCH] pg_createsubscriber: fix slow recovery If the primary server is idle when you are running pg_createsubscriber, it used to take some time during recovery. The reason is that it was using the LSN returned by pg_create_logical_replication_slot as recovery_target_lsn. This LSN points to "end + 1" record that might not be available at WAL, hence, the recovery routine waits until some activity from auxiliary processes write WAL records and once it reaches the recovery_target_lsn position. Instead, use restart_lsn from the last replication slot. It points to the last WAL record that was replayed. Hence, the recovery finishes soon. create_logical_replication_slot() does not return LSN so change its signature. Discussion: https://www.postgresql.org/message-id/2377319.1719766794%40sss.pgh.pa.us Discussion: https://www.postgresql.org/message-id/68de6498-0449-a113-dd03-e198dded0bac%40gmail.com --- src/bin/pg_basebackup/pg_createsubscriber.c | 56 ++++++++++++++++----- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index 21dd50f8089..5ef3f751a5b 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -86,8 +86,8 @@ static void setup_recovery(const struct LogicalRepInfo *dbinfo, const char *data static void drop_primary_replication_slot(struct LogicalRepInfo *dbinfo, const char *slotname); static void drop_failover_replication_slots(struct LogicalRepInfo *dbinfo); -static char *create_logical_replication_slot(PGconn *conn, - struct LogicalRepInfo *dbinfo); +static bool create_logical_replication_slot(PGconn *conn, + struct LogicalRepInfo *dbinfo); static void drop_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo, const char *slot_name); static void pg_ctl_status(const char *pg_ctl_cmd, int rc); @@ -769,15 +769,47 @@ setup_publisher(struct LogicalRepInfo *dbinfo) create_publication(conn, &dbinfo[i]); /* Create replication slot on publisher */ - if (lsn) - pg_free(lsn); - lsn = create_logical_replication_slot(conn, &dbinfo[i]); - if (lsn != NULL || dry_run) + if (create_logical_replication_slot(conn, &dbinfo[i]) || dry_run) pg_log_info("create replication slot \"%s\" on publisher", dbinfo[i].replslotname); else exit(1); + /* + * Get restart_lsn from the last replication slot. It points to the + * last record replayed. The LSN returned by + * pg_create_logical_replication_slot() should not be used because it + * points to a future position. In case the server is idle while + * running this tool, the recovery will not finish soon because the + * future position was not reached. Hence, obtain the restart_lsn that + * points to a LSN already available at WAL. The recovery parameters + * guarantee that this last replication slot will be applied. + */ + if ((i == num_dbs - 1) && !dry_run) + { + PQExpBuffer str = createPQExpBuffer(); + PGresult *res; + + appendPQExpBuffer(str, + "SELECT restart_lsn FROM pg_catalog.pg_replication_slots " + "WHERE slot_name = '%s'", + dbinfo[i].replslotname); + res = PQexec(conn, str->data); + if (PQresultStatus(res) != PGRES_TUPLES_OK) + { + pg_log_error("could not obtain consistent LSN: %s", + PQresultErrorMessage(res)); + disconnect_database(conn, true); + } + else + { + lsn = pg_strdup(PQgetvalue(res, 0, 0)); + } + + PQclear(res); + destroyPQExpBuffer(str); + } + disconnect_database(conn, false); } @@ -1283,19 +1315,18 @@ drop_failover_replication_slots(struct LogicalRepInfo *dbinfo) } /* - * Create a logical replication slot and returns a LSN. + * Create a logical replication slot. * * CreateReplicationSlot() is not used because it does not provide the one-row * result set that contains the LSN. */ -static char * +static bool create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo) { PQExpBuffer str = createPQExpBuffer(); PGresult *res = NULL; const char *slot_name = dbinfo->replslotname; char *slot_name_esc; - char *lsn = NULL; Assert(conn != NULL); @@ -1305,7 +1336,7 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo) slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name)); appendPQExpBuffer(str, - "SELECT lsn FROM pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false, false, false)", + "SELECT * FROM pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false, false, false)", slot_name_esc); pg_free(slot_name_esc); @@ -1322,10 +1353,9 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo) PQresultErrorMessage(res)); PQclear(res); destroyPQExpBuffer(str); - return NULL; + return false; } - lsn = pg_strdup(PQgetvalue(res, 0, 0)); PQclear(res); } @@ -1334,7 +1364,7 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo) destroyPQExpBuffer(str); - return lsn; + return true; } static void -- 2.30.2
