Re: Parallel copy
| От | vignesh C |
|---|---|
| Тема | Re: Parallel copy |
| Дата | |
| Msg-id | CALDaNm3V7-Pnm4OeHNR+CVav_7texFS7969OKsRFjc_BaT8rJg@mail.gmail.com обсуждение исходный текст |
| Ответ на | RE: Parallel copy ("Hou, Zhijie" <houzj.fnst@cn.fujitsu.com>) |
| Список | pgsql-hackers |
On Wed, Oct 28, 2020 at 5:36 PM Hou, Zhijie <houzj.fnst@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
>
> 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
В списке pgsql-hackers по дате отправления: