Re: executor relation handling - Mailing list pgsql-hackers

On 4 September 2018 at 20:53, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Updated patches attached; 0001-0003 are same as v1.

I've looked at these. Here's my review so far:

0001:

1. The following does not seem to be true any longer:

+ /*
+ * If you change the conditions under which rel locks are acquired
+ * here, be sure to adjust ExecOpenScanRelation to match.
+ */

per:

@@ -652,28 +654,10 @@ ExecOpenScanRelation(EState *estate, Index
scanrelid, int eflags)
 {
  Relation rel;
  Oid reloid;
- LOCKMODE lockmode;

- /*
- * Determine the lock type we need.  First, scan to see if target relation
- * is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
- * relation.  In either of those cases, we got the lock already.
- */
- lockmode = AccessShareLock;
- if (ExecRelationIsTargetRelation(estate, scanrelid))
- lockmode = NoLock;
- else
- {
- /* Keep this check in sync with InitPlan! */
- ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
-
- if (erm != NULL && erm->relation != NULL)
- lockmode = NoLock;
- }
-

2. Should addRangeTableEntryForRelation() initialize lockmode, or
maybe take it as a parameter?

3. Don't think there's a need to capitalise true and false in:

+ * Return TRUE if we acquired a new lock, FALSE if already held.

4. The comment probably should read "lock level to obtain, or 0 if no
lock is required" in:

+ int lockmode; /* lock taken on the relation or 0 */

The field should likely also be LOCKMODE, not int.

5. AcquireExecutorLocks() does not need the local variable named rt_index

0002:

6. I don't think "rootRelation" is a good name for this field. I think
"root" is being confused with "target". Nothing is it say the target
is the same as the root.

+ Index rootRelation; /* RT index of root partitioned table */

Perhaps "partitionedTarget" is a better name?

7. Should use "else" instead of "else if" in:

+ /* Top-level Plan must be LockRows or ModifyTable */
+ Assert(IsA(stmt->planTree, LockRows) ||
+    IsA(stmt->planTree, ModifyTable));
+ if (IsA(stmt->planTree, LockRows))
+ rowMarks = ((LockRows *) stmt->planTree)->rowMarks;
+ else if (IsA(stmt->planTree, ModifyTable))
+ rowMarks = ((ModifyTable *) stmt->planTree)->rowMarks;

or you'll likely get a compiler warning on non-Assert enabled builds.

0003:

8. The following code seems repeated enough to warrant a static function:

+ rowMarks = NIL;
+ foreach(lc, root->rowMarks)
+ {
+ PlanRowMark *rc = lfirst(lc);
+
+ if (root->simple_rel_array[rc->rti] != NULL &&
+ IS_DUMMY_REL(root->simple_rel_array[rc->rti]))
+ continue;
+
+ rowMarks = lappend(rowMarks, rc);
+ }

Also, why not reverse the condition and do the lappend inside the if?
Save two lines.

9. The following code appears in copy.c, which is pretty much the same
as the code in execMain.c:

estate->es_range_table_size = list_length(cstate->range_table);
estate->es_range_table_array = (RangeTblEntry **)
palloc(sizeof(RangeTblEntry *) *
   estate->es_range_table_size);
/* Populate the range table array */
i = 0;
foreach(lc, cstate->range_table)
estate->es_range_table_array[i++] = lfirst_node(RangeTblEntry, lc);

Would it not be better to invent a function with the signature:

void
setup_range_table_array(EState *estate, List *rangeTable)

and use it in both locations?

10. In create_estate_for_relation() I don't think you should remove
the line that sets es_range_table.

@@ -199,7 +199,8 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
  rte->rtekind = RTE_RELATION;
  rte->relid = RelationGetRelid(rel->localrel);
  rte->relkind = rel->localrel->rd_rel->relkind;
- estate->es_range_table = list_make1(rte);

If you're keeping es_range_table then I think it needs to always be
set properly to help prevent future bugs in that area.

11. ExecRangeTableRelation should look more like:

ExecRangeTableRelation(EState *estate, Index rti)
{
  Relation rel = estate->es_relations[rti - 1];

  if (rel != NULL)
    RelationIncrementReferenceCount(rel);
  else
  {
    RangeTblEntry *rte = exec_rt_fetch(rti, estate->es_range_table_array);

   /*
    * No need to lock the relation lock, because upstream code
    * must hold the lock already.
    */
    rel = estate->es_relations[rti - 1] = heap_open(rte->relid, NoLock);
  }

  return rel;
}

12. I think this should read: /* Fill in RTEs. es_relations will be
populated later. */

+ /* Fill the RTEs, Relations array will be filled later. */

13. I also notice that you're still cleaning up Relations with
heap_close or relation_close. Did you consider not doing that and just
having a function that's run at the end of execution which closes all
non-NULL es_relations? This way you'd not need to perform
RelationIncrementReferenceCount inside ExecRangeTableRelation.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services