Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rSTqR-00E1kj-Hh for pgsql-hackers@arkaria.postgresql.org; Wed, 24 Jan 2024 03:22:32 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rSTqO-001Me3-8N for pgsql-hackers@arkaria.postgresql.org; Wed, 24 Jan 2024 03:22:28 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rSTqN-001Mc1-Nd for pgsql-hackers@lists.postgresql.org; Wed, 24 Jan 2024 03:22:27 +0000 Received: from mail-lj1-x22c.google.com ([2a00:1450:4864:20::22c]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1rSTqK-0033vf-QI for pgsql-hackers@postgresql.org; Wed, 24 Jan 2024 03:22:26 +0000 Received: by mail-lj1-x22c.google.com with SMTP id 38308e7fff4ca-2cf1c275e95so6779661fa.1 for ; Tue, 23 Jan 2024 19:22:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1706066542; x=1706671342; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=cs2SWmGvjdX8K79dEH2wlsNAxH7qYN26Zg4WUcAn0FY=; b=fgJBC6uqMP8Czt4g0hDZs45dMbbWFnUHlAliOz4zaCmFW4ggBTaRCgefoJQMFmaj5V SVbS1FQm/uQ3uXCAyQUgR9ypvextGKX6EYZRx35k0skq1DFYb6cuzWN3I9aWklCGq4X/ 9PcBvd4gSo32xL0pW/go5ao7/9tQr+df4aTHW2Qj6nNCSMAqVIwvLpfJVfR+7TfG/EjD O3zqTZmry/SW3R3N7ZViKd9SXvZB1fMlw/zPlk0dGLXNP7eNO1k6cNuhmgJx+0s6xGA5 00w59A/mJCtvFvT4n78OdivLCrog8r8BHsAGJqTqTfY8VZvhfwOYnu4geirWPtmROsh5 pGgg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706066542; x=1706671342; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=cs2SWmGvjdX8K79dEH2wlsNAxH7qYN26Zg4WUcAn0FY=; b=S/8xFtAmVzJdnPt1IcPsTf2/7EsVAb39EpPLKIF3/EMfN3KszmykiuF7lVv3T6wVqK b4GoxCrCTS25Qn4q2EukG0W4xkF1VyS03yO3DS6KVrnvxmBPhBsG5pdCpBvAflwSBbg/ ZlHOCPyNB/MEX568Tdk05XksJJq+/P0cyZHO7Ih86AbGIxTthzajM03nJOQXoJYUjcNK lqSnrJ5Il69XAhtR0dIl2nwH23mkkIvrdgmKejTlCezTn8sXA2RPytFr2zBA2E/RWeNo fVblSua61lZpct2q7yUX8we30YmiixP9Q0+AtOLKUILSbIdk2ZHmx2lYc5JwcGn9rlTl dXkw== X-Gm-Message-State: AOJu0YzhGOU9/cOI/2LZmmNmgseedCtKIagfWiLIxVbHSWcYdx8jHCyL 8pzuveg/BwRyjL8qBUjUWohgjwoCpBeRlrN6cSM/q/RCNddfvhxZ3ksMG/yip4XDP2Wgcz5rZgl YFE4FtYJCGjRZzMWIe2yDoALTo50= X-Google-Smtp-Source: AGHT+IE2uPB7ehKSvEoQpnbxPxLhGBlB1AdSgP4bKtwsF/jcmTb4FMPqXI0cHOhHSX/JYWRoJ2FOOMbyBGv/4/8Ji5I= X-Received: by 2002:a2e:9089:0:b0:2cd:f093:7939 with SMTP id l9-20020a2e9089000000b002cdf0937939mr395853ljg.32.1706066541774; Tue, 23 Jan 2024 19:22:21 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Wed, 24 Jan 2024 14:21:53 +1100 Message-ID: Subject: Re: Synchronizing slots from primary to standby To: shveta malik Cc: Ajin Cherian , Amit Kapila , Bertrand Drouvot , Masahiko Sawada , "Zhijie Hou (Fujitsu)" , Dilip Kumar , Nisha Moond , "Hayato Kuroda (Fujitsu)" , Bharath Rupireddy , Peter Eisentraut , Bruce Momjian , Ashutosh Sharma , Andres Freund , pgsql-hackers , Alvaro Herrera Content-Type: text/plain; charset="UTF-8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk Here are some comments for patch v66-0001. ====== doc/src/sgml/catalogs.sgml 1. + + If true, the associated replication slots (i.e. the main slot and the + table sync slots) in the upstream database are enabled to be + synchronized to the physical standbys + /physical standbys/physical standby/ I wondered if it is better just to say singular "standby" instead of "standbys" in places like this; e.g. plural might imply cascading for some readers. There are a number of examples like this, so I've repeated the same comment multiple times below. If you disagree, please just ignore all of them. ====== doc/src/sgml/func.sgml 2. that the decoding of prepared transactions is enabled for this - slot. A call to this function has the same effect as the replication - protocol command CREATE_REPLICATION_SLOT ... LOGICAL. + slot. The optional fifth parameter, + failover, when set to true, + specifies that this slot is enabled to be synced to the + physical standbys so that logical replication can be resumed + after failover. A call to this function has the same effect as + the replication protocol command + CREATE_REPLICATION_SLOT ... LOGICAL. (same as above) /physical standbys/physical standby/ Also, I don't see anything else on this page using plural "standbys". ====== doc/src/sgml/protocol.sgml 3. CREATE_REPLICATION_SLOT + + FAILOVER [ boolean ] + + + If true, the slot is enabled to be synced to the physical + standbys so that logical replication can be resumed after failover. + The default is false. + + + (same as above) /physical standbys/physical standby/ ~~~ 4. ALTER_REPLICATION_SLOT + + + FAILOVER [ boolean ] + + + If true, the slot is enabled to be synced to the physical + standbys so that logical replication can be resumed after failover. + + + + (same as above) /physical standbys/physical standby/ ====== doc/src/sgml/ref/create_subscription.sgml 5. + + failover (boolean) + + + Specifies whether the replication slots associated with the subscription + are enabled to be synced to the physical standbys so that logical + replication can be resumed from the new primary after failover. + The default is false. + + + (same as above) /physical standbys/physical standby/ ====== doc/src/sgml/system-views.sgml 6. + + + + failover bool + + + True if this is a logical slot enabled to be synced to the physical + standbys so that logical replication can be resumed from the new primary + after failover. Always false for physical slots. + + (same as above) /physical standbys/physical standby/ ====== src/backend/commands/subscriptioncmds.c 7. + if (IsSet(opts.specified_opts, SUBOPT_FAILOVER)) + { + if (!sub->slotname) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot set failover for a subscription that does not have a slot name"))); + + /* + * Do not allow changing the failover state if the + * subscription is enabled. This is because the failover + * state of the slot on the publisher cannot be modified if + * the slot is currently acquired by the apply worker. + */ + if (sub->enabled) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot set %s for enabled subscription", + "failover"))); + + values[Anum_pg_subscription_subfailover - 1] = + BoolGetDatum(opts.failover); + replaces[Anum_pg_subscription_subfailover - 1] = true; + } The first message is not consistent with the second. The "failover" option maybe should be extracted so it won't be translated. SUGGESTION errmsg("cannot set %s for a subscription that does not have a slot name", "failover") ~~~ 8. AlterSubscription + if (!wrconn) + ereport(ERROR, + (errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("could not connect to the publisher: %s", err))); + Need to keep an eye on the patch proposed by Nisha [1] for messages similar to this one, so in case that gets pushed this code should be changed appropriately. ====== src/backend/replication/slot.c 9. * during getting changes, if the two_phase option is enabled it can skip * prepare because by that time start decoding point has been moved. So the * user will only get commit prepared. + * failover: If enabled, allows the slot to be synced to physical standbys so + * that logical replication can be resumed after failover. */ (same as earlier) /physical standbys/physical standby/ ~~~ 10. + /* + * Do not allow users to alter slots to enable failover on the standby + * as we do not support sync to the cascading standby. + */ + if (RecoveryInProgress() && failover) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot alter replication slot to have failover" + " enabled on the standby")); I felt the errmsg could be expressed with less ambiguity: SUGGESTION: cannot enable failover for a replication slot on the standby ====== src/backend/replication/slotfuncs.c 11. create_physical_replication_slot /* acquire replication slot, this will check for conflicting names */ ReplicationSlotCreate(name, false, - temporary ? RS_TEMPORARY : RS_PERSISTENT, false); + temporary ? RS_TEMPORARY : RS_PERSISTENT, false, + false); Having an inline comment might be helpful here instead of passing "false,false" SUGGESTION ReplicationSlotCreate(name, false, temporary ? RS_TEMPORARY : RS_PERSISTENT, false, false /* failover */); ~~~ 12. create_logical_replication_slot + /* + * Do not allow users to create the slots with failover enabled on the + * standby as we do not support sync to the cascading standby. + */ + if (RecoveryInProgress() && failover) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot create replication slot with failover" + " enabled on the standby")); (similar to previous comment) SUGGESTION: cannot enable failover for a replication slot created on the standby ~~~ 13. copy_replication_slot * hence pass find_startpoint false. confirmed_flush will be set * below, by copying from the source slot. + * + * To avoid potential issues with the slotsync worker when the + * restart_lsn of a replication slot goes backwards, we set the + * failover option to false here. This situation occurs when a slot on + * the primary server is dropped and immediately replaced with a new + * slot of the same name, created by copying from another existing + * slot. However, the slotsync worker will only observe the restart_lsn + * of the same slot going backwards. */ create_logical_replication_slot(NameStr(*dst_name), plugin, temporary, false, + false, src_restart_lsn, false); (similar to an earlier comment) Having an inline comment might be helpful here. e.g. false /* failover */, ====== src/backend/replication/walreceiver.c 14. - walrcv_create_slot(wrconn, slotname, true, false, 0, NULL); + walrcv_create_slot(wrconn, slotname, true, false, false, 0, NULL); (similar to an earlier comment) Having an inline comment might be helpful here: SUGGESTION walrcv_create_slot(wrconn, slotname, true, false, false /* failover */, 0, NULL); ====== src/backend/replication/walsender.c 15. CreateReplicationSlot ReplicationSlotCreate(cmd->slotname, false, cmd->temporary ? RS_TEMPORARY : RS_PERSISTENT, - false); + false, false); (similar to an earlier comment) Having an inline comment might be helpful here. e.g. false /* failover */, ~~~ 16. CreateReplicationSlot + /* + * Do not allow users to create the slots with failover enabled on the + * standby as we do not support sync to the cascading standby. + */ + if (RecoveryInProgress() && failover) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot create replication slot with failover" + " enabled on the standby")); + /* * Initially create persistent slot as ephemeral - that allows us to * nicely handle errors during initialization because it'll get @@ -1243,7 +1265,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) */ ReplicationSlotCreate(cmd->slotname, true, cmd->temporary ? RS_TEMPORARY : RS_EPHEMERAL, - two_phase); + two_phase, failover); This errmsg seems to be repeated in a few places, so I wondered if this code can be refactored to call direct to create_logical_replication_slot() so the errmsg can be just once in a common place. OTOH, if it cannot be refactored, then needs to be using same errmsg as suggested by earlier review comments (see above). ====== src/include/catalog/pg_subscription.h 17. + bool subfailover; /* True if the associated replication slots + * (i.e. the main slot and the table sync + * slots) in the upstream database are enabled + * to be synchronized to the physical + * standbys. */ (same as earlier) /physical standbys/physical standby/ ~~~ 18. + bool failover; /* True if the associated replication slots + * (i.e. the main slot and the table sync + * slots) in the upstream database are enabled + * to be synchronized to the physical + * standbys. */ (same as earlier) /physical standbys/physical standby/ ====== src/include/replication/slot.h 19. + + /* + * Is this a failover slot (sync candidate for physical standbys)? Only + * relevant for logical slots on the primary server. + */ + bool failover; (same as earlier) /physical standbys/physical standby/ ====== [1] Nisha errmsg - https://www.postgresql.org/message-id/CABdArM5-VR4Akt_AHap_0Ofne0cTcsdnN6FcNe%2BMU8eXsa_ERQ%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia