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(). Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers