On Wed, Oct 28, 2020 at 5:36 PM Hou, Zhijie <houzj.f...@cn.fujitsu.com> wrote: > > Hi > > I found some issue in v9-0002 > > 1. > + > + elog(DEBUG1, "[Worker] Processing - line position:%d, block:%d, unprocessed lines:%d, offset:%d, line size:%d", > + write_pos, lineInfo->first_block, > + pg_atomic_read_u32(&data_blk_ptr->unprocessed_line_parts), > + offset, pg_atomic_read_u32(&lineInfo->line_size)); > + > > write_pos or other variable to be printed here are type of uint32, I think it'better to use '%u' in elog msg. >
Modified it. > 2. > + * line_size will be set. Read the line_size again to be sure if it is > + * completed or partial block. > + */ > + dataSize = pg_atomic_read_u32(&lineInfo->line_size); > + if (dataSize) > > It use dataSize( type int ) to get uint32 which seems a little dangerous. > Is it better to define dataSize uint32 here? > Modified it. > 3. > Since function with 'Cstate' in name has been changed to 'CState' > I think we can change function PopulateCommonCstateInfo as well. > Modified it. > 4. > + if (pcdata->worker_line_buf_count) > > I think some check like the above can be 'if (xxx > 0)', which seems easier to understand. Modified it. Thanks for the comments, these issues are fixed in v10 patch posted at [1] [1] https://www.postgresql.org/message-id/CALDaNm05FnA-ePvYV_t2%2BWE_tXJymbfPwnm%2Bkc9y1iMkR%2BNbUg%40mail.gmail.com Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
