On Tue, Sep 12, 2017 at 1:42 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > On 2017/09/12 16:55, Ashutosh Bapat wrote: >> On Tue, Sep 12, 2017 at 1:16 PM, Amit Langote wrote: >>> So I looked at this a bit closely and came to the conclusion that we may >>> not need to keep partitioned table RT indexes in the >>> (Merge)Append.partitioned_rels after all, as far as execution-time locking >>> is concerned. >>> >>> Consider two cases: >>> >>> 1. Plan is created and executed in the same transaction >>> >>> In this case, locks taken on the partitioned tables by the planner will >>> suffice. >>> >>> 2. Plan is executed in a different transaction from the one in which it >>> was created (a cached plan) >>> >>> In this case, AcquireExecutorLocks will lock all the relations in >>> PlannedStmt.rtable, which must include all partitioned tables of all >>> partition trees involved in the query. Of those, it will lock the tables >>> whose RT indexes appear in PlannedStmt.nonleafResultRelations with >>> RowExclusiveLock mode. PlannedStmt.nonleafResultRelations is a global >>> list of all partitioned table RT indexes obtained by concatenating >>> partitioned_rels lists of all ModifyTable nodes involved in the query >>> (set_plan_refs does that). We need to distinguish nonleafResultRelations, >>> because we need to take the stronger lock on a given table before any >>> weaker one if it happens to appear in the query as a non-result relation >>> too, to avoid lock strength upgrade deadlock hazard. >>> >>> Moreover, because all the tables from plannedstmt->rtable, including the >>> partitioned tables, will be added to PlannedStmt.relationsOids, any >>> invalidation events affecting the partitioned tables (for example, >>> add/remove a partition) will cause the plan involving partitioned tables >>> to be recreated. >>> >>> In none of this do we rely on the partitioned table RT indexes appearing >>> in the (Merge)Append node itself. Maybe, we should just remove >>> partitioned_rels from (Merge)AppendPath and (Merge)Append node in a >>> separate patch and move on. >>> >>> Thoughts? >> >> Yes, I did the same analysis (to which I refer in my earlier reply to >> you). I too think we should just remove partitioned_rels from Append >> paths. But then the question is those are then transferred to >> ModifyTable node in create_modifytable_plan() and use it for something >> else. What should we do about that code? I don't think we are really >> using that list from ModifyTable node as well, so may be we could >> remove it from there as well. What do you think? Does that mean >> partitioned_rels isn't used at all in the code? > > No, we cannot simply get rid of partitioned_rels altogether. We'll need > to keep it in the ModifyTable node, because we *do* need the > nonleafResultRelations list in PlannedStmt to distinguish partitioned > table result relations, which set_plan_refs builds by concatenating > partitioned_rels lists of various ModifyTable nodes of the query. The > PlannedStmt.nonleafResultRelations list actually has some use (which > parallels PlannedStmt.resultRelations), but partitioned_rels list in the > individual (Merge)Append, as it turns out, doesn't. > > So, we can remove partitioned_rels from (Merge)AppendPath and > (Merge)Append nodes and remove ExecLockNonLeafAppendTables().
Don't we need partitioned_rels from Append paths to be transferred to ModifyTable node or we have a different way of calculating nonleafResultRelations? -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
