Excessive number of replication slots for 12->14 logical replication

At Thu, 14 Jul 2022 13:51:55 +0200, hubert depesz lubaczewski <depesz@depesz.com> wrote in

Hi,
We're using logical replication to replicate database from pg 12.9 to
14.4.

Given the number of objects we made decision to use 10 separate
publications, each containing subset of tables.

So, we have:

(10 publications with an average of 15000 talbes)

On both sides we have max_sync_workers_per_subscription set to 2.

So, I would assume that during initial subscription, we can go up to 30
connections. To make it safe we set limit to 50, but apparently it's not
enough.

I'm getting *LOTS* of errors about number of replication slots:

Yeah, the limitation by max_sync_workers_per_subscription is performed
on subscriber, but replication slot drops happen not on the
subscriber, but at the termination of corresponding walsender process
on publisher. So, there's a lag between the finish of subscription
worker and the corresponding slot's drop. Thus, a new sync worker can
be created while the walsenders corresponding to some already finished
sync workers is still going to finish.

What is happening, and why Pg is trying to get more than 30

concurrent

slots in this case?

Thus, not just about logical replication, some spare slots are
necessary to keep things running smoothly. I guess that it is
widely(?) recommended to provide at least one or two spare slots to
allow replication reconnection in case of silent disconnections, but I
haven't found something like that in the documentation.. In your case,
it seems to me larger number of spare slots makes the server run more
smoothly, *I* don't think 10 spare slots affect anything badly.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jul 15, 2022 at 01:55:32PM +0900, Kyotaro Horiguchi wrote:

Thus, not just about logical replication, some spare slots are
necessary to keep things running smoothly. I guess that it is
widely(?) recommended to provide at least one or two spare slots to
allow replication reconnection in case of silent disconnections, but I
haven't found something like that in the documentation.. In your case,
it seems to me larger number of spare slots makes the server run more
smoothly, *I* don't think 10 spare slots affect anything badly.

OK. The explanation makes sense, but leads to problem - we have 50
max replication slots, which gives me 20 spare slots.

And I'm not sure how much should I increase it so that it will still
work.

Plus - there is the problem that increasing max_replication_slots
requires restart :(

Anyway, will deal with it, now that I understand the issue. Thanks
a lot.

Best regards,

depesz

Hi,

On 2022-07-15 13:55:32 +0900, Kyotaro Horiguchi wrote:

Yeah, the limitation by max_sync_workers_per_subscription is performed
on subscriber, but replication slot drops happen not on the
subscriber, but at the termination of corresponding walsender process
on publisher. So, there's a lag between the finish of subscription
worker and the corresponding slot's drop. Thus, a new sync worker can
be created while the walsenders corresponding to some already finished
sync workers is still going to finish.

Why are we relying on the slots being dropped at the end of connection? That
doesn't seem like a good idea to me. Can't we just do that explicitly? We
still need the on-connection-close cleanup to deal with network failures etc,
but that doesn't mean we can do something else in the happy path.

Greetings,

Andres Freund

On Thu, Jul 14, 2022 at 5:22 PM hubert depesz lubaczewski
<depesz@depesz.com> wrote:

Hi,
We're using logical replication to replicate database from pg 12.9 to
14.4.

Given the number of objects we made decision to use 10 separate
publications, each containing subset of tables.

So, we have:

...

On both sides we have max_sync_workers_per_subscription set to 2.

So, I would assume that during initial subscription, we can go up to 30
connections. To make it safe we set limit to 50, but apparently it's not
enough.

I'm getting *LOTS* of errors about number of replication slots:

2022-07-14 11:40:30.612 UTC,"upgrayedd","dbname",11829,"10.1.191.34:37394",62d000ae.2e35,5,"idle in transaction",2022-07-14 11:40:30 UTC,38/350893,0,ERROR,53400,"all replication slots are in use",,"Free one or increase max_replication_slots.",,,,"CREATE_REPLICATION_SLOT ""pg_1337070_sync_189406_7119125406647933415"" LOGICAL pgoutput USE_SNAPSHOT",,,"pg_1337070_sync_189406_7119125406647933415"

We're getting them (in the beginning of sync) to the tune of 2-4 thousand per minute.!

...

What is happening, and why Pg is trying to get more than 30 concurrent
slots in this case?

As per my understanding, each subscription will use three slots (one
for apply worker and one for each of the sync workers) in your case in
the sync phase. So, in your case, if you have 10 subscriptions then
ideally it should be no more than 30. Can you please check and share
the subscriber logs to see if there are any errors in the initial sync
phase? Also, please confirm the number of subscriptions you create on
the subscriber?

--
With Regards,
Amit Kapila.

On Sat, Jul 16, 2022 at 3:44 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-07-15 13:55:32 +0900, Kyotaro Horiguchi wrote:

Yeah, the limitation by max_sync_workers_per_subscription is performed
on subscriber, but replication slot drops happen not on the
subscriber, but at the termination of corresponding walsender process
on publisher.

We do drop the replication slots on subscribers since commit
ce0fdbfe97 once the initial sync is complete.

So, there's a lag between the finish of subscription
worker and the corresponding slot's drop. Thus, a new sync worker can
be created while the walsenders corresponding to some already finished
sync workers is still going to finish.

Why are we relying on the slots being dropped at the end of connection? That
doesn't seem like a good idea to me. Can't we just do that explicitly?

We do that explicitly once the initial sync in finished.

--
With Regards,
Amit Kapila.

On Mon, Jul 18, 2022 at 09:07:35AM +0530, Amit Kapila wrote:

As per my understanding, each subscription will use three slots (one
for apply worker and one for each of the sync workers) in your case in
the sync phase. So, in your case, if you have 10 subscriptions then
ideally it should be no more than 30. Can you please check and share
the subscriber logs to see if there are any errors in the initial sync
phase? Also, please confirm the number of subscriptions you create on
the subscriber?

Hi,
I'm 100% sure we create 10, with the table distribution as shown in
original post.

Will reset the whole thing from scratch, and check for other errors, but
it will take "a while".

Best regards,

depesz

On Mon, Jul 18, 2022 at 09:07:35AM +0530, Amit Kapila wrote:

As per my understanding, each subscription will use three slots (one
for apply worker and one for each of the sync workers) in your case in
the sync phase. So, in your case, if you have 10 subscriptions then
ideally it should be no more than 30. Can you please check and share
the subscriber logs to see if there are any errors in the initial sync
phase? Also, please confirm the number of subscriptions you create on
the subscriber?

Ok. In next test I did:

@09:21:16 AM UTC, on source
All the "create publication" for focal14_1 to focal14_10 sets.

@09:21:53 (immediately after), on recipient:
create subscription focal14_1 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_1;
create subscription focal14_2 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_2;
create subscription focal14_3 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_3;
create subscription focal14_4 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_4;
create subscription focal14_5 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_5;
create subscription focal14_6 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_6;
create subscription focal14_7 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_7;
create subscription focal14_8 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_8;
create subscription focal14_9 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_9;
create subscription focal14_10 connection 'host=10.10.10.10 port=5432 user=upgrayedd dbname=dbname' publication focal14_10;

This script has ened at 09:21:56

First error:
#v+
2022-07-18 09:22:07.046 UTC,,,4145917,,62d5263f.3f42fd,2,,2022-07-18 09:22:07 UTC,28/21641,1219146,ERROR,53400,"could not find free replication state slot for replication origin with OID 51",,"Increase max_replication_slots and try again.",,,,,,,"","logical replication worker",,0
#v-

Nothing else errored out before, no warning, no fatals.

from the first ERROR I was getting them in the range of 40-70 per minute.

At the same time I was logging data from `select now(), * from pg_replication_slots`, every 2 seconds.

Then, i grouped it into 4 groups:
slots with name focal* with active = true
slots with name focal* with active = false
slots with name *sync* with active = true
slots with name *sync* with active = false

How it looked in time:
09:21:54: 5 total; focal: 1 active, 2 inactive; sync: 1 active, 1 inactive
09:21:56: 11 total; focal: 1 active, 9 inactive; sync: 1 active, 0 inactive
09:21:58: 11 total; focal: 1 active, 8 inactive; sync: 1 active, 1 inactive
09:22:00: 23 total; focal: 10 active, 0 inactive; sync: 0 active, 13 inactive
09:22:03: 28 total; focal: 10 active, 0 inactive; sync: 9 active, 9 inactive
09:22:05: 21 total; focal: 10 active, 0 inactive; sync: 7 active, 4 inactive
09:22:07: 20 total; focal: 10 active, 0 inactive; sync: 7 active, 3 inactive
09:22:09: 27 total; focal: 10 active, 0 inactive; sync: 9 active, 8 inactive
09:22:11: 30 total; focal: 10 active, 0 inactive; sync: 10 active, 10 inactive
09:22:13: 49 total; focal: 10 active, 0 inactive; sync: 5 active, 34 inactive
09:22:15: 50 total; focal: 10 active, 0 inactive; sync: 0 active, 40 inactive
09:22:17: 49 total; focal: 10 active, 0 inactive; sync: 0 active, 39 inactive
09:22:19: 44 total; focal: 10 active, 0 inactive; sync: 1 active, 33 inactive
09:22:21: 43 total; focal: 10 active, 0 inactive; sync: 2 active, 31 inactive
09:22:23: 44 total; focal: 10 active, 0 inactive; sync: 1 active, 33 inactive
09:22:25: 43 total; focal: 10 active, 0 inactive; sync: 1 active, 32 inactive

So, it looks that there are up to 10 focal slots, all active, and then there are sync slots with weirdly high counts for inactive ones.

At most, I had 11 active sync slots.

Looks like some kind of timing issue, which would be inline with what
Kyotaro Horiguchi wrote initially.

The thing is that after some time, the data *gets* replicated, and it
seems to be full on publication side, but the errors are unnerving :)

depesz

On Mon, Jul 18, 2022 at 3:13 PM hubert depesz lubaczewski
<depesz@depesz.com> wrote:

On Mon, Jul 18, 2022 at 09:07:35AM +0530, Amit Kapila wrote:

First error:
#v+
2022-07-18 09:22:07.046 UTC,,,4145917,,62d5263f.3f42fd,2,,2022-07-18 09:22:07 UTC,28/21641,1219146,ERROR,53400,"could not find free replication state slot for replication origin with OID 51",,"Increase max_replication_slots and try again.",,,,,,,"","logical replication worker",,0
#v-

Nothing else errored out before, no warning, no fatals.

from the first ERROR I was getting them in the range of 40-70 per minute.

At the same time I was logging data from `select now(), * from pg_replication_slots`, every 2 seconds.

...

So, it looks that there are up to 10 focal slots, all active, and then there are sync slots with weirdly high counts for inactive ones.

At most, I had 11 active sync slots.

Looks like some kind of timing issue, which would be inline with what
Kyotaro Horiguchi wrote initially.

I think this is a timing issue similar to what Horiguchi-San has
pointed out but due to replication origins. We drop the replication
origin after the sync worker that has used it is finished. This is
done by the apply worker because we don't allow to drop the origin
till the process owning the origin is alive. I am not sure of
repercussions but maybe we can allow dropping the origin by the
process that owns it.

I think this will also be addressed once we start resuing
workers/slots/origin to copy multiple tables in the initial sync phase
as is being discussed in the thread [1]/messages/by-id/CAGPVpCTq=rUDd4JUdaRc1XUWf4BrH2gdSNf3rtOMUGj9rPpfzQ@mail.gmail.com.

[1]: /messages/by-id/CAGPVpCTq=rUDd4JUdaRc1XUWf4BrH2gdSNf3rtOMUGj9rPpfzQ@mail.gmail.com

--
With Regards,
Amit Kapila.

On Sun, Jul 24, 2022 at 6:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 18, 2022 at 3:13 PM hubert depesz lubaczewski
<depesz@depesz.com> wrote:

On Mon, Jul 18, 2022 at 09:07:35AM +0530, Amit Kapila wrote:

First error:
#v+
2022-07-18 09:22:07.046 UTC,,,4145917,,62d5263f.3f42fd,2,,2022-07-18 09:22:07 UTC,28/21641,1219146,ERROR,53400,"could not find free replication state slot for replication origin with OID 51",,"Increase max_replication_slots and try again.",,,,,,,"","logical replication worker",,0
#v-

Nothing else errored out before, no warning, no fatals.

from the first ERROR I was getting them in the range of 40-70 per minute.

At the same time I was logging data from `select now(), * from pg_replication_slots`, every 2 seconds.

...

So, it looks that there are up to 10 focal slots, all active, and then there are sync slots with weirdly high counts for inactive ones.

At most, I had 11 active sync slots.

Looks like some kind of timing issue, which would be inline with what
Kyotaro Horiguchi wrote initially.

I think this is a timing issue similar to what Horiguchi-San has
pointed out but due to replication origins. We drop the replication
origin after the sync worker that has used it is finished. This is
done by the apply worker because we don't allow to drop the origin
till the process owning the origin is alive. I am not sure of
repercussions but maybe we can allow dropping the origin by the
process that owns it.

I have written a patch which will do the dropping of replication
origins in the sync worker itself.
I had to reset the origin session (which also resets the owned by
flag) prior to the dropping of the slots.

regards,
Ajin Cherian
Fujitsu Australia

On Sunday, July 24, 2022 4:17 PM Ajin Cherian <itsajin@gmail.com> wrote:

On Sun, Jul 24, 2022 at 6:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 18, 2022 at 3:13 PM hubert depesz lubaczewski
<depesz@depesz.com> wrote:

On Mon, Jul 18, 2022 at 09:07:35AM +0530, Amit Kapila wrote:

First error:
#v+
2022-07-18 09:22:07.046 UTC,,,4145917,,62d5263f.3f42fd,2,,2022-07-18
09:22:07 UTC,28/21641,1219146,ERROR,53400,"could not find free
replication state slot for replication origin with OID
51",,"Increase max_replication_slots and try
again.",,,,,,,"","logical replication worker",,0
#v-

Nothing else errored out before, no warning, no fatals.

from the first ERROR I was getting them in the range of 40-70 per minute.

At the same time I was logging data from `select now(), * from

pg_replication_slots`, every 2 seconds.

...

So, it looks that there are up to 10 focal slots, all active, and then there are

sync slots with weirdly high counts for inactive ones.

At most, I had 11 active sync slots.

Looks like some kind of timing issue, which would be inline with
what Kyotaro Horiguchi wrote initially.

I think this is a timing issue similar to what Horiguchi-San has
pointed out but due to replication origins. We drop the replication
origin after the sync worker that has used it is finished. This is
done by the apply worker because we don't allow to drop the origin
till the process owning the origin is alive. I am not sure of
repercussions but maybe we can allow dropping the origin by the
process that owns it.

I have written a patch which will do the dropping of replication origins in the
sync worker itself.
I had to reset the origin session (which also resets the owned by
flag) prior to the dropping of the slots.

Thanks for the patch.

I tried the patch and confirmed that we won't get the ERROR "could not find
free replication state slot for replication origin with OID" again after
applying the patch.

I tested the patch by letting the apply worker wait for a bit more time after
setting the state to SUBREL_STATE_CATCHUP. In this case(before the patch) the
table sync worker will exit before the apply worker drop the replorigin, and
the apply worker will try to start another worker which would cause the
ERROR(before the patch).

Few comments:

1)
- * 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.
- */

I think it would be better if we can move these comments to the new place where
we drop the replorigin.

2)

- replorigin_drop_by_name(originname, true, false);

/*
* Update the state to READY only after the origin cleanup.

Do we need to slightly modify the comment here as the origin drop code has been
moved to other places. Maybe "It's safe to update the state to READY as the
origin should have been dropped by table sync worker".

Best regards,
Hou zj

On Mon, Jul 25, 2022 at 7:48 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On Sunday, July 24, 2022 4:17 PM Ajin Cherian <itsajin@gmail.com> wrote:
I tried the patch and confirmed that we won't get the ERROR "could not find
free replication state slot for replication origin with OID" again after
applying the patch.

I tested the patch by letting the apply worker wait for a bit more time after
setting the state to SUBREL_STATE_CATCHUP. In this case(before the patch) the
table sync worker will exit before the apply worker drop the replorigin, and
the apply worker will try to start another worker which would cause the
ERROR(before the patch).

Thanks for testing the patch.

Few comments:

1)
- * 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.
- */

I think it would be better if we can move these comments to the new place where
we drop the replorigin.

Fixed this.

2)

- replorigin_drop_by_name(originname, true, false);

/*
* Update the state to READY only after the origin cleanup.

Do we need to slightly modify the comment here as the origin drop code has been
moved to other places. Maybe "It's safe to update the state to READY as the
origin should have been dropped by table sync worker".

Fixed this.
Added an updated patch.

regards,
Ajin Cherian
Fujitsu Australia

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.

======

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

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.

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.

~~~

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.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

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

Here are some comments for v3-0001:

(all cosmetic / comments)

======

0. <apply>

There were multiple whitespace warnings reported by git apply.

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:36:
indent with spaces.
/*
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:37:
indent with spaces.
* Cleanup the tablesync slot and the origin tracking if exists.
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:38:
indent with spaces.
*
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:39:
indent with spaces.
* This has to be done after updating the state because otherwise if
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:40:
indent with spaces.
* there is an error while doing the database operations we won't be
warning: squelched 3 whitespace errors
warning: 8 lines add whitespace errors.

======

1. Commit message

I think the Replication Origin tracking is maybe not strictly speaking
a "slot", even though it does use the same GUC as true slots. With
that in mind, perhaps the following text is more accurate.

SUGGESTION
The replication origin tracking uses the same GUC
(max_replication_slots) as the tablesync slots for limiting resources.

In the current (HEAD) code the replication origin tracking of the
completed tablesync worker is dropped by the apply worker, but this
means there can be a small lag between when the tablesync worker
exited, and when its origin tracking is actually dropped.

Meanwhile, new tablesync workers can be launched and will immediately
try to acquire new slots and origin tracking. If this happens before
the worker's origin tracking gets dropped then the available number of
slots (max_replication_slots) can be exceeded, leading to the error as
reported.

To avoid this lag, the dropping of replicating origin tracking is
moved to the tablesync worker where it exits.

======

2. src/backend/replication/logical/tablesync.c - process_syncing_tables_for_sync

@@ -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.
+         *

Consider reversing that comment's first sentence so that it describes
what it will do in the same order as the code logic.

SUGGESTION
Cleanup the origin tracking and tablesync slot.

~~~

3.

+ /*
+ * 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.
  */

"slot to be dropped" -> "origin to be dropped" (maybe?)

~~~

4. src/backend/replication/logical/tablesync.c -
process_syncing_tables_for_apply

@@ -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);

/*

This change results in a double blank line remaining instead of just a
single blank line.

------
Kind Regards,
Peter Smith
Fujitsu Australia

On Fri, Jul 29, 2022 at 7:37 PM Peter Smith <smithpb2250@gmail.com> wrote:

Here are some comments for v3-0001:

(all cosmetic / comments)

======

0. <apply>

There were multiple whitespace warnings reported by git apply.

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:36:
indent with spaces.
/*
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:37:
indent with spaces.
* Cleanup the tablesync slot and the origin tracking if exists.
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:38:
indent with spaces.
*
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:39:
indent with spaces.
* This has to be done after updating the state because otherwise if
../patches_misc/v3-0001-fix-excessive-replicating-origin-slots-issue.patch:40:
indent with spaces.
* there is an error while doing the database operations we won't be
warning: squelched 3 whitespace errors
warning: 8 lines add whitespace errors.

======

Fixed.

1. Commit message

I think the Replication Origin tracking is maybe not strictly speaking
a "slot", even though it does use the same GUC as true slots. With
that in mind, perhaps the following text is more accurate.

SUGGESTION
The replication origin tracking uses the same GUC
(max_replication_slots) as the tablesync slots for limiting resources.

In the current (HEAD) code the replication origin tracking of the
completed tablesync worker is dropped by the apply worker, but this
means there can be a small lag between when the tablesync worker
exited, and when its origin tracking is actually dropped.

Meanwhile, new tablesync workers can be launched and will immediately
try to acquire new slots and origin tracking. If this happens before
the worker's origin tracking gets dropped then the available number of
slots (max_replication_slots) can be exceeded, leading to the error as
reported.

To avoid this lag, the dropping of replicating origin tracking is
moved to the tablesync worker where it exits.

======

Updated as suggested.

2. src/backend/replication/logical/tablesync.c - process_syncing_tables_for_sync

@@ -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.
+         *

Consider reversing that comment's first sentence so that it describes
what it will do in the same order as the code logic.

SUGGESTION
Cleanup the origin tracking and tablesync slot.

~~~

Fixed.

3.

+ /*
+ * 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.
*/

"slot to be dropped" -> "origin to be dropped" (maybe?)

~~~

Fixed

4. src/backend/replication/logical/tablesync.c -
process_syncing_tables_for_apply

@@ -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);

/*

This change results in a double blank line remaining instead of just a
single blank line.

Fixed.

regards,
Ajin Cherian
Fujitsu Australia

Here is my review comment for v4-0001.

(Just a nitpick about comments)

======

1. src/backend/replication/logical/tablesync.c - process_syncing_tables_for_sync

There are really just 2 main things that are being done for the cleanup here:
- Drop the origin tracking
- Drop the tablesync slot

So, IMO the code will have more clarity by having one bigger comment
for each of those drops, instead of commenting every step separately.

e.g.

BEFORE
/*
* Cleanup the tablesync origin tracking if exists.
*/
ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
originname,
sizeof(originname));

/*
* Reset the origin session before dropping.
*
* This is required to reset the ownership of the slot
* and allow the origin 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,
sizeof(syncslotname));

/*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);

SUGGESTION

/*
* Cleanup the tablesync origin tracking.
*
* Resetting the origin session removes the ownership of the slot.
* This is needed to allow the origin to be dropped.
*
* Also, 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,
MyLogicalRepWorker->relid,
originname,
sizeof(originname));
replorigin_session_reset();
replorigin_session_origin = InvalidRepOriginId;
replorigin_session_origin_lsn = InvalidXLogRecPtr;
replorigin_session_origin_timestamp = 0;

replorigin_drop_by_name(originname, true, false);

/*
* Cleanup the tablesync slot.
*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
syncslotname,
sizeof(syncslotname));
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Aug 1, 2022 at 11:50 AM Peter Smith <smithpb2250@gmail.com> wrote:

Here is my review comment for v4-0001.

(Just a nitpick about comments)

======

1. src/backend/replication/logical/tablesync.c - process_syncing_tables_for_sync

There are really just 2 main things that are being done for the cleanup here:
- Drop the origin tracking
- Drop the tablesync slot

So, IMO the code will have more clarity by having one bigger comment
for each of those drops, instead of commenting every step separately.

e.g.

BEFORE
/*
* Cleanup the tablesync origin tracking if exists.
*/
ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
originname,
sizeof(originname));

/*
* Reset the origin session before dropping.
*
* This is required to reset the ownership of the slot
* and allow the origin 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,
sizeof(syncslotname));

/*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);

SUGGESTION

/*
* Cleanup the tablesync origin tracking.
*
* Resetting the origin session removes the ownership of the slot.
* This is needed to allow the origin to be dropped.
*
* Also, 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,
MyLogicalRepWorker->relid,
originname,
sizeof(originname));
replorigin_session_reset();
replorigin_session_origin = InvalidRepOriginId;
replorigin_session_origin_lsn = InvalidXLogRecPtr;
replorigin_session_origin_timestamp = 0;

replorigin_drop_by_name(originname, true, false);

/*
* Cleanup the tablesync slot.
*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
syncslotname,
sizeof(syncslotname));
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);

Updated.

regards,
Ajin Cherian
Fujitsu Australia

The patch v5-0001 looks good to me.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Aug 1, 2022 at 3:46 PM Ajin Cherian <itsajin@gmail.com> wrote:

On Mon, Aug 1, 2022 at 11:50 AM Peter Smith <smithpb2250@gmail.com> wrote:

Here is my review comment for v4-0001.

(Just a nitpick about comments)

======

1. src/backend/replication/logical/tablesync.c - process_syncing_tables_for_sync

There are really just 2 main things that are being done for the cleanup here:
- Drop the origin tracking
- Drop the tablesync slot

So, IMO the code will have more clarity by having one bigger comment
for each of those drops, instead of commenting every step separately.

e.g.

BEFORE
/*
* Cleanup the tablesync origin tracking if exists.
*/
ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
originname,
sizeof(originname));

/*
* Reset the origin session before dropping.
*
* This is required to reset the ownership of the slot
* and allow the origin 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,
sizeof(syncslotname));

/*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);

SUGGESTION

/*
* Cleanup the tablesync origin tracking.
*
* Resetting the origin session removes the ownership of the slot.
* This is needed to allow the origin to be dropped.
*
* Also, 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,
MyLogicalRepWorker->relid,
originname,
sizeof(originname));
replorigin_session_reset();
replorigin_session_origin = InvalidRepOriginId;
replorigin_session_origin_lsn = InvalidXLogRecPtr;
replorigin_session_origin_timestamp = 0;

replorigin_drop_by_name(originname, true, false);

/*
* Cleanup the tablesync slot.
*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
syncslotname,
sizeof(syncslotname));
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);

Updated.

Thank you for working on this. I have a comment and a question:

                  * 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.
+                 * able to rollback dropped slot or origin tracking.

I think we can actually roll back dropping the replication origin. So
the above comment is true for only replication slots.

---
+       replorigin_session_reset();
+       replorigin_session_origin = InvalidRepOriginId;
+       replorigin_session_origin_lsn = InvalidXLogRecPtr;
+       replorigin_session_origin_timestamp = 0;
+
+       replorigin_drop_by_name(originname, true, false);

With this change, the committing the ongoing transaction will be done
without replication origin. Is this okay? it's probably okay, but
since tablesync worker commits other changes with replication origin
I'm concerned a bit there might be a corner case.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/