From: Ajin Cherian <itsajin@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Hubert Lubaczewski <depesz@depesz.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: Excessive number of replication slots for 12->14 logical replication
Date: Fri, 29 Jul 2022 17:05:46 +1000
Message-ID: <CAFPTHDa+M1SLWdHiMC55LDTikP+DcsuFC8z9TjFkhxpp09pgHQ@mail.gmail.com> (raw)
In-Reply-To: <CAHut+Puq73cDWPiGFTw=v6tHkL7pDNKJcB4VScPA25qPQOVn8A@mail.gmail.com>
References: <20220714115155.GA5439@depesz.com>
<CAA4eK1LAk=9fi7V=XKb7d40hOyKTt5a3MTOaEQpUr-RHgpixxg@mail.gmail.com>
<20220718094330.GA24063@depesz.com>
<CAA4eK1+RL43Qty=Rb+RJhw0+0sm-f2T=7ST=9u0R+vmnDDVZaA@mail.gmail.com>
<CAFPTHDbppsYxW3Ttg0fpYrJ8F8Zx1HVFr3Sn56XLK54C2GUYCg@mail.gmail.com>
<OS0PR01MB5716ED883D44E2214F3563B094959@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<CAFPTHDZBs98Vn9OBf=x1tAo_3bTY9wnvRrRX1KMYfMqu+Pngng@mail.gmail.com>
<CAHut+Puq73cDWPiGFTw=v6tHkL7pDNKJcB4VScPA25qPQOVn8A@mail.gmail.com>
On Thu, Jul 28, 2022 at 11:56 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for the v2-0001 patch:
>
> ======
>
> 1. Commit message
>
> The commit message should give some reason why relocating the origin
> slot drop code is expected to fix the reported problem.
>
> ======
>
Updated.
> 2. src/backend/replication/logical/tablesync.c
>
> + /* reset the origin session before dropping */
> + replorigin_session_reset();
> +
> + replorigin_session_origin = InvalidRepOriginId;
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
>
> 2a.
> Uppercase comment
Fixed.
>
> 2b.
> IIUC you are not doing the reset because you particularly want to do a
> reset, but really because this is the only (?) way to dis-associate
> the current process from owning the slot. Otherwise, the slot would be
> considered still "busy" and the subsequent replorigin_drop_by_name
> would be rejected. I thought the current comment should be expanded to
> explain all this background.
Updated.
>
> 2c.
> Perhaps it is non-standard to do so, but I wondered if you can just
> call pg_replication_origin_session_reset instead of
> replorigin_session_reset here so that there would only be 1 LOC
> instead of 4.
That is a psql function, invoking that indirectly is not really a
standard practice.
>
> ~~~
>
> 3. src/backend/replication/logical/tablesync.c
>
> + /*
> * Cleanup the tablesync slot.
> *
> * This has to be done after updating the state because otherwise if
>
> Doesn't the same note ("This has to be done after...") also apply to
> the code dropping the origin slot? Maybe this note needs to be either
> duplicated or just put a common note about this above both of those
> slot drops.
>
Updated.
regards,
Ajin Cherian
Fujitsu Australia
Attachments:v3-0001-fix-excessive-replicating-origin-slots-issue.patch (application/octet-stream, 4.5K) download
From c85249360d8f1a45d55b3eab3e995b0436a0b9b2 Mon Sep 17 00:00:00 2001
From: Ajin Cherian <ajinc@fast.au.fujitsu.com>
Date: Fri, 29 Jul 2022 02:31:51 -0400
Subject: [PATCH v3] fix excessive replicating origin slots issue.
In the current (HEAD) code the origin slots of the completed tablesync worker
are dropped by the apply worker, but this means there can be a small lag
between when the tablesync worker exits, and when its origin slot is actually dropped.
Meanwhile new tablesync workers are launched and try and acquire slots before the
previously finished tablesync worker slots could be dropped. If this happens the
the available number of slots might be exceeded leading to the error reported.
To avoid this lag, the dropping of replicating origin slots is moved to the
tablesync worker when it exits.
---
src/backend/replication/logical/tablesync.c | 61 +++++++++++++++++------------
1 file changed, 36 insertions(+), 25 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.cindex 6a01ffd..40de753 100644--- a/src/backend/replication/logical/tablesync.c+++ b/src/backend/replication/logical/tablesync.c@@ -291,6 +291,7 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn)
{
TimeLineID tli;
char syncslotname[NAMEDATALEN] = {0};
+ char originname[NAMEDATALEN] = {0};
MyLogicalRepWorker->relstate = SUBREL_STATE_SYNCDONE;
MyLogicalRepWorker->relstate_lsn = current_lsn;
@@ -315,13 +316,43 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn)
*/
walrcv_endstreaming(LogRepWorkerWalRcvConn, &tli);
+ /*+ * Cleanup the tablesync slot and the origin tracking if exists.+ *+ * This has to be done after updating the state because otherwise if+ * there is an error while doing the database operations we won't be+ * able to rollback dropped slot or origin tracking.+ */+
/*
- * Cleanup the tablesync slot.+ * Cleanup the tablesync origin tracking if exists.+ */+ ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,+ MyLogicalRepWorker->relid,+ originname,+ sizeof(originname));++ /* + * Reset the origin session before dropping.
*
- * This has to be done after updating the state because otherwise if- * there is an error while doing the database operations we won't be- * able to rollback dropped slot.+ * This is required to reset the ownership of the slot+ * and allow the slot to be dropped.
*/
+ replorigin_session_reset();++ replorigin_session_origin = InvalidRepOriginId;+ replorigin_session_origin_lsn = InvalidXLogRecPtr;+ replorigin_session_origin_timestamp = 0;++ /*+ * There is a chance that the user is concurrently performing+ * refresh for the subscription where we remove the table+ * state and its origin and by this time the origin might be+ * already removed. So passing missing_ok = true.+ */+ replorigin_drop_by_name(originname, true, false);++ /* Cleanup the tablesync slot. */
ReplicationSlotNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
syncslotname,
@@ -441,8 +472,6 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
*/
if (current_lsn >= rstate->lsn)
{
- char originname[NAMEDATALEN];-
rstate->state = SUBREL_STATE_READY;
rstate->lsn = current_lsn;
if (!started_tx)
@@ -451,27 +480,9 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
started_tx = true;
}
- /*- * Remove the tablesync origin tracking if exists.- *- * The normal case origin drop is done here instead of in the- * process_syncing_tables_for_sync function because we don't- * allow to drop the origin till the process owning the origin- * is alive.- *- * There is a chance that the user is concurrently performing- * refresh for the subscription where we remove the table- * state and its origin and by this time the origin might be- * already removed. So passing missing_ok = true.- */- ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,- rstate->relid,- originname,- sizeof(originname));- replorigin_drop_by_name(originname, true, false);
/*
- * Update the state to READY only after the origin cleanup.+ * Update the state to READY.
*/
UpdateSubscriptionRelState(MyLogicalRepWorker->subid,
rstate->relid, rstate->state,
--
1.8.3.1
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: pgsql-bugs@postgresql.org
Cc: itsajin@gmail.com, smithpb2250@gmail.com, houzj.fnst@fujitsu.com, amit.kapila16@gmail.com, depesz@depesz.com, pgsql-bugs@lists.postgresql.org
Subject: Re: Excessive number of replication slots for 12->14 logical replication
In-Reply-To: <CAFPTHDa+M1SLWdHiMC55LDTikP+DcsuFC8z9TjFkhxpp09pgHQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox