Horton

Re: Parallel copy

vignesh C <vignesh21@gmail.com>

View thread
From: vignesh C <vignesh21@gmail.com>
To: "Hou, Zhijie" <houzj.fnst@cn.fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Rafia Sabih <rafia.pghackers@gmail.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>, Ants Aasma <ants@cybertec.at>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, Alastair Turner <minion@decodable.me>, Thomas Munro <thomas.munro@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Heikki Linnakangas <hlinnaka@iki.fi>
Date: 2020-11-18T10:01:10Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow WaitLatch() to be used without a latch.

  2. Add %P to log_line_prefix for parallel group leader

  3. Include replication origins in SQL functions for commit timestamp

  4. Avoid useless buffer allocations during binary COPY FROM.

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