Horton

RE: Parallel copy

Hou, Zhijie <houzj.fnst@cn.fujitsu.com>

View thread
From: "Hou, Zhijie" <houzj.fnst@cn.fujitsu.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>
Date: 2020-11-19T11:16:42Z
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.

Hi Vignesh,

I took a look at the v10 patch set. Here are some comments:

1. 
+/*
+ * CheckExprParallelSafety
+ *
+ * Determine if where cluase and default expressions are parallel safe & do not
+ * have volatile expressions, return true if condition satisfies else return
+ * false.
+ */

'cluase' seems a typo.


2.
+			/*
+			 * Make sure that no worker has consumed this element, if this
+			 * line is spread across multiple data blocks, worker would have
+			 * started processing, no need to change the state to
+			 * LINE_LEADER_POPULATING in this case.
+			 */
+			(void) pg_atomic_compare_exchange_u32(&lineInfo->line_state,
+												  &current_line_state,
+												  LINE_LEADER_POPULATED);
About the commect

+			 * started processing, no need to change the state to
+			 * LINE_LEADER_POPULATING in this case.

Does it means no need to change the state to LINE_LEADER_POPULATED ' here?


3.
+ * 3) only one worker should choose one line for processing, this is handled by
+ *    using pg_atomic_compare_exchange_u32, worker will change the state to
+ *    LINE_WORKER_PROCESSING only if line_state is LINE_LEADER_POPULATED.

In the latest patch, it will set the state to LINE_WORKER_PROCESSING if line_state is LINE_LEADER_POPULATED or LINE_LEADER_POPULATING.
So The comment here seems wrong.


4.
A suggestion for CacheLineInfo.

It use appendBinaryStringXXX to store the line in memory.
appendBinaryStringXXX will double the str memory when there is no enough spaces.

How about call enlargeStringInfo in advance, if we already know the whole line size?
It can avoid some memory waste and may impove a little performance.


Best regards,
houzj