+ return;
/*
* We're feeding a WAL, thus responding to FINAL JOIN or SUBSCRIBE
* request. If this is FINAL JOIN (i.e. relay->replica is NULL),
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 968c3a202..10edbf6a8 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1194,17 +1194,23 @@ int
xrow_encode_subscribe(struct xrow_header *row,
const struct tt_uuid *replicaset_uuid,
const struct tt_uuid *instance_uuid,
- const struct vclock *vclock, bool anon)
+ const struct vclock *vclock, bool anon,
+ unsigned int id_filter)
3. Nit - it would be better to have it as uint32_t explicitly.
Becuase max id count is 32. Unsigned int does not have size
guarantees, formally speaking. It is at least 16 bits, no more
info.
Done, and I’ll send a followup to vclock.h later, as Kostja suggests.
{
memset(row, 0, sizeof(*row));
size_t size = XROW_BODY_LEN_MAX + mp_sizeof_vclock(vclock);
+ unsigned int filter_size = __builtin_popcount(id_filter);
+ if (filter_size) {
+ size += mp_sizeof_array(filter_size) + filter_size *
+ mp_sizeof_uint(VCLOCK_MAX);
4. You didn't add mp_sizeof_uint(IPROTO_ID_FILTER) here, but you
didn't update XROW_BODY_LEN_MAX either. Does it fit in the current
value? Also seems like it can't be called XROW_BODY_LEN_MAX anymore.
Because clearly it is not enough to fit the filter array, if you
needed to patch the allocation here.
Let’s then just double XROW_BODY_LEN_MAX up.
You could also update XROW_BODY_LEN_MAX so as it includes the biggest
possible filter. Max filter size is just 34 bytes. This is the
simplest option, I think.
+ }
char *buf = (char *) region_alloc(&fiber()->gc, size);
if (buf == NULL) {
diag_set(OutOfMemory, size, "region_alloc", "buf");
return -1;
}
char *data = buf;
- data = mp_encode_map(data, 5);
+ data = mp_encode_map(data, filter_size ? 6 : 5);
data = mp_encode_uint(data, IPROTO_CLUSTER_UUID);
data = xrow_encode_uuid(data, replicaset_uuid);
data = mp_encode_uint(data, IPROTO_INSTANCE_UUID);
@@ -1215,6 +1221,17 @@ xrow_encode_subscribe(struct xrow_header *row,
data = mp_encode_uint(data, tarantool_version_id());
data = mp_encode_uint(data, IPROTO_REPLICA_ANON);
data = mp_encode_bool(data, anon);
+ if (filter_size) {
5. I wouldn't make it optional in encoding, since this is sent
really rare, but could simplify the code a bit. However, up to
you.
I don’t like the idea to pass zero length arrays in case there is no filter,
Lets just not encode this part. The filter is initially empty on decoding side
anyway.
Also, won't it break replication from an old version? You
will send subscribe with this new key, and the old instance
should ignore it. Does it? I don't remember.
Yes, the old instance ignores all unknown keys.
If the old instance would ignore it, it means that the bug
still can explode when replicating from an old version, right?
I don't know how to fix that, but if it is true, we need to
document that, at least.
Yes, the bug’s still here when replicating from an old instance.
To trigger it, you have to set up master-master replication between
the old and the new instances and fit some additional conditions.
I don’t think it’s usual to set up master-master when upgrading, as
Kostja has pointed out.
+ data = mp_encode_uint(data, IPROTO_ID_FILTER);
+ data = mp_encode_array(data, filter_size);
+ struct bit_iterator it;
+ bit_iterator_init(&it, &id_filter, sizeof(id_filter),
+ true);
+ for (size_t id = bit_iterator_next(&it); id < VCLOCK_MAX;
+ id = bit_iterator_next(&it)) {
+ data = mp_encode_uint(data, id);
+ }
+ }
assert(data <= buf + size);
row->body[0].iov_base = buf;
row->body[0].iov_len = (data - buf);
@@ -1301,6 +1321,21 @@ xrow_decode_subscribe(struct xrow_header *row, struct tt_uuid *replicaset_uuid,
}
*anon = mp_decode_bool(&d);
break;
+ case IPROTO_ID_FILTER:
+ if (id_filter == NULL)
+ goto skip;
+ if (mp_typeof(*d) != MP_ARRAY) {
+decode_err: xrow_on_decode_err(data, end, ER_INVALID_MSGPACK,
+ "invalid id_filter");
6. Lets say it in caps: ID_FILTER, just like with other decode
error messages. Also I would name this label 'id_filter_decode_err',
becuase other keys can't jump here.
Okay.
+ return -1;
+ }
+ uint32_t len = mp_decode_array(&d);
+ for(uint32_t i = 0; i < len; ++i) {
+ if (mp_typeof(*d) != MP_UINT)
+ goto decode_err;
+ *id_filter |= 1 << mp_decode_uint(&d);
7. If someone would send a big ID (a program, pretending to be a Tarantool
instance), it would cause unsigned bit shift overflow, which is undefined
behaviour. Lets check that it is not bigger than 31.
However this won't really help much. This code will crash even if I will
just send a truncated packet. From what I see.
I’m not sure I understand what you’re speaking about. This piece of code is
similar to the one we have in mp_decode_vclock. The situation didn’t get worse,
at least.
Up to you whether you want to fix the bit shift.
Fixed.