On Sat, Dec 23, 2017 at 4:08 PM, Dang Minh Huong <kakalo...@gmail.com> wrote: > On 2017/12/21 10:05, Masahiko Sawada wrote: > >> On Wed, Dec 20, 2017 at 5:39 PM, Huong Dangminh >> <huo-dangm...@ys.jp.nec.com> wrote: >>> >>> Hi Sawada-san, >>> >>>> Thank you for quick response. The changes look good to me. But I wonder >>>> if the following changes needs some comments to describe what each >>>> checks >>>> does for. >>>> >>>> - if (errarg->attnum < 0) >>>> + if (errarg->local_attnum <0) >>>> + return; >>>> + >>>> + rel = errarg->rel; >>>> + remote_attnum = rel->attrmap[errarg->local_attnum]; >>>> + >>>> + if (remote_attnum < 0) >>>> return; >>> >>> Thanks, I have added some comments as you mentioned. >>> >> Thank you for updating the patch. >> >> - if (errarg->attnum < 0) >> + /* Check case of slot_store_error_callback() is called before >> + * errarg.local_attnum is set. */ >> + if (errarg->local_attnum <0) >> >> This comment style isn't preferred by PostgreSQL code. Please refer to >> https://www.postgresql.org/docs/current/static/source-format.html >> -- >> $ git diff --check >> src/backend/replication/logical/worker.c:291: trailing whitespace. >> + /* Check case of slot_store_error_callback() is called before > > Thanks, I will be careful in the next time. >> >> There is an extra white space in the patch. >> >> I'm inclined to think SlotErrCallbackArg can have remote_attnum and >> pass it to the callback function. That way, we don't need to case the >> case where local_attnum is not set yet. > > Nice. >> >> Attached a new version patch incorporated the aboves. Please review it. > > Thanks for updating the patch. > It looks fine to me. >
Thank you for confirmation. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
