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 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. Attached a new version patch incorporated the aboves. Please review it. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
fix_slot_store_error_callback_v11.patch
Description: Binary data
