[Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status update

[Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status update

[Serge Petrenko]

relay_schedule_pending_gc() is executed after relay status update,
which made perfect sense before we've introduced local spaces rework, making
local space operations use a special instance id: 0.
Relay status update is performed only when the remote instance has
reported a bigger vclock, than its previous one. However, we may have an
entire WAL file filled with local space changes, in which case the
changes won't be transmitted to replica, and it will report the same
vclock as before, postponing the scheduled gc until a non-local row is
created on master.

Fix this by reordering relay_schedule_pending_gc() and relay status
update. In case nothing new is added to pending_gc queue and replica
clock is not updated, relay_schedule_pending_gc() will exit on the first
loop iteration, so it doesn't add an overhead.

Also make relay_schedule_pending_gc() use vclock_compare_ignore0() instead
of plain vclock_compare().

Follow-up #4114
---
This code was present in v5 of the patchset regarding local space replication
rework, but I accidentally threw it  away during the review. Sorry.
The letter was "[PATCH v5 2/4] replication: hide 0-th vclock components in
replication responses"

Branch:  https://github.com/tarantool/tarantool/tree/sp/relay-gc-fix



 src/box/relay.cc | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/box/relay.cc b/src/box/relay.cc
index dd6c3a580..2ad02cb8a 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -464,7 +464,7 @@ relay_schedule_pending_gc(struct relay *relay, const struct vclock *vclock)
 		 * the greater signatures is due to changes pulled
 		 * from other members of the cluster.
 		 */
-		if (vclock_compare(&curr->vclock, vclock) > 0)
+		if (vclock_compare_ignore0(&curr->vclock, vclock) > 0)
 			break;
 		stailq_shift(&relay->pending_gc);
 		free(gc_msg);
@@ -638,6 +638,10 @@ relay_subscribe_f(va_list ap)
 			send_vclock = &r->vclock;
 		else
 			send_vclock = &relay->recv_vclock;
+
+		/* Collect xlog files received by the replica. */
+		relay_schedule_pending_gc(relay, send_vclock);
+
 		if (vclock_sum(&relay->status_msg.vclock) ==
 		    vclock_sum(send_vclock))
 			continue;
@@ -648,8 +652,6 @@ relay_subscribe_f(va_list ap)
 		vclock_copy(&relay->status_msg.vclock, send_vclock);
 		relay->status_msg.relay = relay;
 		cpipe_push(&relay->tx_pipe, &relay->status_msg.msg);
-		/* Collect xlog files received by the replica. */
-		relay_schedule_pending_gc(relay, send_vclock);
 	}
 
 	/*
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status update

[Kirill Yukhin]

Hello,

On 16 апр 15:06, Serge Petrenko wrote:
> relay_schedule_pending_gc() is executed after relay status update,
> which made perfect sense before we've introduced local spaces rework, making
> local space operations use a special instance id: 0.
> Relay status update is performed only when the remote instance has
> reported a bigger vclock, than its previous one. However, we may have an
> entire WAL file filled with local space changes, in which case the
> changes won't be transmitted to replica, and it will report the same
> vclock as before, postponing the scheduled gc until a non-local row is
> created on master.
> 
> Fix this by reordering relay_schedule_pending_gc() and relay status
> update. In case nothing new is added to pending_gc queue and replica
> clock is not updated, relay_schedule_pending_gc() will exit on the first
> loop iteration, so it doesn't add an overhead.
> 
> Also make relay_schedule_pending_gc() use vclock_compare_ignore0() instead
> of plain vclock_compare().
> 
> Follow-up #4114
> ---
> This code was present in v5 of the patchset regarding local space replication
> rework, but I accidentally threw it  away during the review. Sorry.
> The letter was "[PATCH v5 2/4] replication: hide 0-th vclock components in
> replication responses"
> 
> Branch:  https://github.com/tarantool/tarantool/tree/sp/relay-gc-fix

LGTM.
I've checked your patch into 2.3 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status update

[Cyrill Gorcunov]

On Thu, Apr 16, 2020 at 03:06:58PM +0300, Serge Petrenko wrote:
> relay_schedule_pending_gc() is executed after relay status update,
> which made perfect sense before we've introduced local spaces rework, making
> local space operations use a special instance id: 0.
> Relay status update is performed only when the remote instance has
> reported a bigger vclock, than its previous one. However, we may have an
> entire WAL file filled with local space changes, in which case the
> changes won't be transmitted to replica, and it will report the same
> vclock as before, postponing the scheduled gc until a non-local row is
> created on master.
> 
> Fix this by reordering relay_schedule_pending_gc() and relay status
> update. In case nothing new is added to pending_gc queue and replica
> clock is not updated, relay_schedule_pending_gc() will exit on the first
> loop iteration, so it doesn't add an overhead.
> 
> Also make relay_schedule_pending_gc() use vclock_compare_ignore0() instead
> of plain vclock_compare().
> 
> Follow-up #4114
> ---
> This code was present in v5 of the patchset regarding local space replication
> rework, but I accidentally threw it  away during the review. Sorry.
> The letter was "[PATCH v5 2/4] replication: hide 0-th vclock components in
> replication responses"
> 
> Branch:  https://github.com/tarantool/tarantool/tree/sp/relay-gc-fix

Maybe it worth to have two commits?

 - use vclock_compare_ignore0 in relay_schedule_pending_gc
 - reorder relay_schedule_pending_gc

Re: [Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status

[Cyrill Gorcunov]

On Thu, Apr 16, 2020 at 03:38:02PM +0300, Cyrill Gorcunov wrote:
...
> > This code was present in v5 of the patchset regarding local space replication
> > rework, but I accidentally threw it  away during the review. Sorry.
> > The letter was "[PATCH v5 2/4] replication: hide 0-th vclock components in
> > replication responses"
> > 
> > Branch:  https://github.com/tarantool/tarantool/tree/sp/relay-gc-fix
> 
> Maybe it worth to have two commits?
> 
>  - use vclock_compare_ignore0 in relay_schedule_pending_gc
>  - reorder relay_schedule_pending_gc

Doesn't matter, already merged :)

Re: [Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status

[Kirill Yukhin]

Hello,

On 16 апр 15:42, Cyrill Gorcunov wrote:
> On Thu, Apr 16, 2020 at 03:38:02PM +0300, Cyrill Gorcunov wrote:
> ...
> > > This code was present in v5 of the patchset regarding local space replication
> > > rework, but I accidentally threw it  away during the review. Sorry.
> > > The letter was "[PATCH v5 2/4] replication: hide 0-th vclock components in
> > > replication responses"
> > > 
> > > Branch:  https://github.com/tarantool/tarantool/tree/sp/relay-gc-fix
> > 
> > Maybe it worth to have two commits?
> > 
> >  - use vclock_compare_ignore0 in relay_schedule_pending_gc
> >  - reorder relay_schedule_pending_gc
> 
> Doesn't matter, already merged :)

I have no idea, why my mail wasn't sent.

Anyway, the patch was committed to master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] relay: move relay_schedule_pending_gc before status

[Cyrill Gorcunov]

On Fri, Apr 17, 2020 at 10:02:27AM +0300, Kirill Yukhin wrote:
> > > 
> > > Maybe it worth to have two commits?
> > > 
> > >  - use vclock_compare_ignore0 in relay_schedule_pending_gc
> > >  - reorder relay_schedule_pending_gc
> > 
> > Doesn't matter, already merged :)
> 
> I have no idea, why my mail wasn't sent.
> 
> Anyway, the patch was committed to master.

No, I've been replying and then found that you've
already merged the patch. IOW you msg been delivered
properly. That's why I said "already merged".