v36_amit_1.patch.txt
text/plain
Filename: v36_amit_1.patch.txt
Type: text/plain
Part: 0
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b8f9bf573ea..16310cf474f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -8088,8 +8088,8 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</para>
<para>
If true, the information (e.g., dead tuples, commit timestamps, and
- origins) on the subscriber that is still useful for conflict detection
- is retained.
+ origins) on the subscriber that is useful for conflict detection is\
+ retained.
</para></entry>
</row>
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index be90088bcd0..0e49bf09eca 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -443,18 +443,18 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<listitem>
<para>
Specifies whether the information (e.g., dead tuples, commit
- timestamps, and origins) on the subscriber that is still useful for
- conflict detection is retained. The default is
- <literal>false</literal>. If set to true, an additional replication
- slot named <quote><literal>pg_conflict_detection</literal></quote>
+ timestamps, and origins) required for conflict detection on the
+ subscriber is retained. The default is <literal>false</literal>.
+ If set to true, a replication slot named
+ <quote><literal>pg_conflict_detection</literal></quote>
will be created on the subscriber to prevent the conflict information
from being removed.
</para>
<para>
Note that the information useful for conflict detection is retained
- only after the creation of the additional slot. You can verify the
- existence of this slot by querying <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
+ only after the creation of the slot. You can verify the existence of
+ this slot by querying <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
And even if multiple subscriptions on one node enable this option,
only one replication slot will be created.
</para>
@@ -468,7 +468,7 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
</para>
<para>
- This option cannot be enabled if the publisher is also a physical standby.
+ This option cannot be enabled if the publisher is a physical standby.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9938fbe3e57..396cfcce1f6 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -72,7 +72,7 @@
#define SUBOPT_PASSWORD_REQUIRED 0x00000800
#define SUBOPT_RUN_AS_OWNER 0x00001000
#define SUBOPT_FAILOVER 0x00002000
-#define SUBOPT_RETAIN_CONFLICT_INFO 0x00004000
+#define SUBOPT_RETAIN_CONFLICT_INFO 0x00004000
#define SUBOPT_LSN 0x00008000
#define SUBOPT_ORIGIN 0x00010000
@@ -625,6 +625,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
errmsg("password_required=false is superuser-only"),
errhint("Subscriptions with the password_required option set to false may only be created or modified by the superuser.")));
+ /* Ensure that we can enable retainconflictinfo. */
CheckSubConflictInfoRetention(opts.retainconflictinfo, true,
!opts.enabled);
@@ -1084,10 +1085,15 @@ CheckAlterSubOption(Subscription *sub, const char *option,
* publisher cannot be modified if the slot is currently acquired by the
* existing walsender.
*
- * Do not allow changing the retain_conflict_info option when the
- * subscription is enabled or the apply worker is active, to prevent race
- * conditions arising from the new option value being acknowledged
- * asynchronously by the launcher and apply workers.
+ * Note that two_phase is enabled (aka changed from 'false' to 'true') on
+ * the publisher by the existing walsender, so we could have allowed that
+ * even when the subscription is enabled. But we kept this restriction for
+ * the sake of consistency and simplicity.
+ *
+ * Additionally, do not allow changing the retain_conflict_info option when
+ * the subscription is enabled to prevent race conditions arising from the
+ * new option value being acknowledged asynchronously by the launcher and
+ * apply workers.
*
* Without the restriction, a race condition may arise when a user
* disables and immediately re-enables the retain_conflict_info option. In
@@ -1118,11 +1124,6 @@ CheckAlterSubOption(Subscription *sub, const char *option,
* to avoid the race conditions described above, but we maintain the
* restriction for both enable and disable operations for the sake of
* consistency.
- *
- * Note that two_phase is enabled (aka changed from 'false' to 'true') on
- * the publisher by the existing walsender, so we could have allowed that
- * even when the subscription is enabled. But we kept this restriction for
- * the sake of consistency and simplicity.
*/
if (sub->enabled)
ereport(ERROR,
@@ -1399,7 +1400,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* subscription has been disabled.
*
* Ensure workers have already been exited to avoid the
- * race conditions as described in CheckAlterSubOption().
+ * race conditions described in CheckAlterSubOption().
*/
if (logicalrep_workers_find(subid, true, true))
ereport(ERROR,
@@ -1417,12 +1418,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
Assert(!sub->enabled);
/*
- * Provide a notice if retain_conflict_info is enabled for
- * a disabled subscription, reminding the user to enable
- * the subscription to prevent the accumulation of dead
- * tuples. A warning is not issued since
- * retain_conflict_info can be altered only for disabled
- * subscriptions.
+ * Remind the user that enabling subscription will prevent
+ * the accumulation of dead tuples.
*/
if (opts.retainconflictinfo)
ereport(NOTICE,
@@ -1461,10 +1458,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot enable subscription that does not have a slot name")));
- /*
- * Perform the track_commit_timestamp check only when enabling
- * the subscription.
- */
+ /* Check track_commit_timestamp only when enabling the subscription. */
CheckSubConflictInfoRetention(sub->retainconflictinfo,
opts.enabled, !opts.enabled);
@@ -2341,14 +2335,15 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
}
/*
- * Check if the publisher's status permits enabling retain_conflict_info.
+ * Determine whether the retain_conflict_info can be enable based on the
+ * publisher's status.
*
- * Enabling retain_conflict_info is not allowed if the publisher's version is
- * prior to PG18 or if the publisher is in recovery (operating as a standby
+ * This option is disallowed if the publisher is running a version earlier
+ * than the PG19, or if the publisher is in recovery (i.e., it is a standby
* server).
*
- * Refer to the comments atop maybe_advance_nonremovable_xid() for detailed
- * reasons.
+ * See comments atop maybe_advance_nonremovable_xid() for a detailed
+ * explanation.
*/
static void
check_pub_conflict_info_retention(WalReceiverConn *wrconn, bool retain_conflict_info)
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 799daa247e2..aa763fc5ef2 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -1339,7 +1339,7 @@ compute_min_nonremovable_xid(LogicalRepWorker *worker,
/*
* Create a replication slot to retain information necessary for
* conflict detection such as dead tuples, commit timestamps, and
- * origins if requested by any subscription.
+ * origins.
*
* The slot is created before starting the apply worker to prevent it
* from unnecessarily maintaining its oldest_nonremovable_xid.
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f7434b0f6cd..10e1cb7ea45 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5257,18 +5257,20 @@ InitializeLogRepWorker(void)
}
/*
- * Restart the worker if retain_conflict_info was enabled at startup. The
- * replication slot for conflict detection may not be created yet, or
- * might soon be dropped as the launcher sees retain_conflict_info as
- * disabled. To prevent unnecessary maintenance of oldest_nonremovable_xid
- * when the slot is absent or at risk of being dropped, a restart is
- * initiated.
+ * Restart the worker if retain_conflict_info was enabled during startup.
+ *
+ * At this point, the replication slot used for conflict detection might
+ * not exist yet, or could be dropped soon if the launcher perceives
+ * retain_conflict_info as disabled. To avoid unnecessary tracking of
+ * oldest_nonremovable_xid when the slot is absent or at risk of being
+ * dropped, a restart is initiated.
*
* The oldest_nonremovable_xid should be initialized only when the
* retain_conflict_info is enabled before launching the worker. See
* logicalrep_worker_launch.
*/
- if (am_leader_apply_worker() && MySubscription->retainconflictinfo &&
+ if (am_leader_apply_worker() &&
+ MySubscription->retainconflictinfo &&
!TransactionIdIsValid(MyLogicalRepWorker->oldest_nonremovable_xid))
{
ereport(LOG,