From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 27 Jun 2018 20:45:09 +0300 From: Konstantin Osipov Subject: Re: [PATCH 4/6] txn: do not require space id for nop requests Message-ID: <20180627174509.GH28358@chai> References: <4dfddcb3838385c88cbfdf51d52ef112f56cf130.1528906027.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4dfddcb3838385c88cbfdf51d52ef112f56cf130.1528906027.git.vdavydov.dev@gmail.com> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [18/06/13 20:40]: > Currently, IPROTO_NOP can only be generated by a before_replace trigger, > when it returns the old tuple thus turning the original operation into a > NOP. In such a case we know the space id and we write it to the request > body. This allows us to dispatch NOP requests via DML route. > > As a part of replica local spaces feature, we will substitute requests > operating on local spaces with NOP in relay in order to promote vclock > on replicas without actual data modification. Since space_id is stored > in request body, sending it to replicas would mean decoding the request > body in relay, which is an overkill. To avoid that, let's separate NOP > and DML paths and remove space_id from NOP requests. LGTM, a few comments below. > @@ -277,6 +277,11 @@ apply_row(struct xstream *stream, struct xrow_header *row) > (void) stream; > struct request request; > xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type)); > + if (request.type == IPROTO_NOP) { > + if (txn_commit_nop(&request) != 0) > + diag_raise(); > + return; > + } txn_commit_nop() -> box_process_nop() or process_nop(). > @@ -50,7 +50,6 @@ request_create_from_tuple(struct request *request, struct space *space, > struct tuple *old_tuple, struct tuple *new_tuple) > { > memset(request, 0, sizeof(*request)); > - request->space_id = space->def->id; > > if (old_tuple == new_tuple) { > /* > @@ -61,6 +60,7 @@ request_create_from_tuple(struct request *request, struct space *space, > return 0; > } > > + request->space_id = space->def->id; Please add a comment /* * Space pointer may be zero in case of NOP, in which case this * line is not reached. */ You changed a lot of code making it more NOP friendly, for example txn_begin_in_engine() now works even if there is already a transaction which has a NOP statement. This seems like a bug fix with no test. Could you please add a test? -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov