Hello Greg-san,
Initially, some miner comments:
(1)
- * (Note that we do allow CREATE TABLE AS, SELECT INTO, and CREATE
- * MATERIALIZED VIEW to use parallel plans, but as of now, only the
leader
- * backend writes into a completely new table. In the future, we can
- * extend it to allow workers to write into the table. However, to
allow
- * parallel updates and deletes, we have to solve other problems,
- * especially around combo CIDs.)
+ * (Note that we do allow CREATE TABLE AS, INSERT INTO...SELECT, SELECT
+ * INTO, and CREATE MATERIALIZED VIEW to use parallel plans. However, as
+ * of now, only the leader backend writes into a completely new table.
In
This can read "In INSERT INTO...SELECT case, like other existing cases, only
the leader backend writes into a completely new table." The reality is that
workers as well as the leader can write into an empty or non-empty table in
parallel, isn't it?
(2)
/*
* RELATION_IS_LOCAL
- * If a rel is either temp or newly created in the current
transaction,
- * it can be assumed to be accessible only to the current backend.
- * This is typically used to decide that we can skip acquiring
locks.
+ * If a rel is temp, it can be assumed to be accessible only to the
+ * current backend. This is typically used to decide that we can
+ * skip acquiring locks.
*
* Beware of multiple eval of argument
*/
#define RELATION_IS_LOCAL(relation) \
- ((relation)->rd_islocaltemp || \
- (relation)->rd_createSubid != InvalidSubTransactionId)
+ ((relation)->rd_islocaltemp)
How is this correct? At least, this change would cause a transaction that
creates a new relation acquire an unnecessary lock. I'm not sure if that
overhead is worth worrying about (perhaps not, I guess). But can we still
check >rd_createSubid in non-parallel mode? If we adopt the above change, the
comments at call sites need modification - "new or temp relation" becomes "temp
relations".
(3)
@@ -173,9 +175,11 @@ ExecSerializePlan(Plan *plan, EState *estate)
...
- pstmt->commandType = CMD_SELECT;
+ Assert(estate->es_plannedstmt->commandType == CMD_SELECT ||
+
IsModifySupportedInParallelMode(estate->es_plannedstmt->commandType));
+ pstmt->commandType = IsA(plan, ModifyTable) ? castNode(ModifyTable,
plan)->operation : CMD_SELECT;
The last line can just be as follows, according to the Assert():
+ pstmt->commandType = estate->es_plannedstmt->commandType);
(4)
@@ -1527,7 +1528,9 @@ ExecutePlan(EState *estate,
estate->es_use_parallel_mode = use_parallel_mode;
if (use_parallel_mode)
{
- PrepareParallelMode(estate->es_plannedstmt->commandType);
+ bool isParallelModifyLeader = IsA(planstate,
GatherState) && IsA(outerPlanState(planstate), ModifyTableState);
+
+ PrepareParallelMode(estate->es_plannedstmt->commandType,
isParallelModifyLeader);
EnterParallelMode();
}
@@ -1021,12 +1039,25 @@ IsInParallelMode(void)
* Prepare for entering parallel mode, based on command-type.
*/
void
-PrepareParallelMode(CmdType commandType)
+PrepareParallelMode(CmdType commandType, bool isParallelModifyLeader)
{
if (IsModifySupportedInParallelMode(commandType))
{
Assert(!IsInParallelMode());
+ if (isParallelModifyLeader)
+ {
+ /*
+ * Set currentCommandIdUsed to true, to ensure that the
current
+ * CommandId (which will be used by the parallel
workers) won't
+ * change during this parallel operation, as starting
new
+ * commands in parallel-mode is not currently supported.
+ * See related comments in GetCurrentCommandId and
+ * CommandCounterIncrement.
+ */
+ (void) GetCurrentCommandId(true);
+ }
I think we can eliminate the second argument of PrepareParallelMode() and the
new code in ExecutePlan(). PrepareParallelMode() can use !IsParallelWorker()
in the if condition, because the caller is either a would-be parallel leader or
a parallel worker.
BTW, why do we want to add PrepareParallelMode() separately from
EnterParallelMode()? Someone who will read other call sites of
EnterParallelMode() (index build, VACUUM) may be worried that
PrepareParallelMode() call is missing there. Can we just add an argument to
EnterParallelMode()? Other call sites can use CMD_UNKNOWN or CMD_UTILITY, if
we want to use CMD_XX.
Regards
Takayuki Tsunakawa
