Re: [HACKERS] Race between SELECT and ALTER TABLE NO INHERIT

At Mon, 28 Aug 2017 18:28:07 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20170828.182807.98097766.horiguchi.kyotaro@lab.ntt.co.jp>
> I'll add this to CF2017-09.

This patch got deadly atack from the commit 30833ba. I changed
the signature of expand_single_inheritance_child in addition to
make_inh_translation_list to notify that the specified child is
no longer a child of the parent.

This passes regular regression test and fixed the the problem.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
*** a/src/backend/catalog/pg_inherits.c
--- b/src/backend/catalog/pg_inherits.c
***************
*** 42,47 **** typedef struct SeenRelsEntry
--- 42,49 ----     ListCell   *numparents_cell;    /* corresponding list cell */ } SeenRelsEntry; 
+ static bool is_descendent_of_internal(Oid parentId, Oid childId,
+                                       HTAB *seen_rels); /*  * find_inheritance_children  *
***************
*** 400,402 **** typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId)
--- 402,472 ----      return result; }
+ 
+ /*
+  * Check if the child is really a descendent of the parent
+  */
+ bool
+ is_descendent_of(Oid parentId, Oid childId)
+ {
+     HTAB       *seen_rels;
+     HASHCTL        ctl;
+     bool        ischild = false;
+ 
+     memset(&ctl, 0, sizeof(ctl));
+     ctl.keysize = sizeof(Oid);
+     ctl.entrysize = sizeof(Oid);
+     ctl.hcxt = CurrentMemoryContext;
+ 
+     seen_rels = hash_create("is_descendent_of temporary table",
+                             32, /* start small and extend */
+                             &ctl,
+                             HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+ 
+     ischild = is_descendent_of_internal(parentId, childId, seen_rels);
+ 
+     hash_destroy(seen_rels);
+ 
+     return ischild;
+ }
+ 
+ static bool
+ is_descendent_of_internal(Oid parentId, Oid childId, HTAB *seen_rels)
+ {
+     Relation    inhrel;
+     SysScanDesc scan;
+     ScanKeyData key[1];
+     bool        ischild = false;
+     HeapTuple    inheritsTuple;
+ 
+     inhrel = heap_open(InheritsRelationId, AccessShareLock);
+     ScanKeyInit(&key[0], Anum_pg_inherits_inhparent,
+                 BTEqualStrategyNumber, F_OIDEQ,    ObjectIdGetDatum(parentId));
+     scan = systable_beginscan(inhrel, InheritsParentIndexId, true,
+                               NULL, 1, key);
+ 
+     while ((inheritsTuple = systable_getnext(scan)) != NULL)
+     {
+         bool found;
+         Oid inhrelid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhrelid;
+ 
+         hash_search(seen_rels, &inhrelid, HASH_ENTER, &found);
+ 
+         /*
+          * Recursively check into children. Although there can't theoretically
+          * be any cycles in the inheritance graph, check the cycles following
+          * find_all_inheritors.
+          */
+         if (inhrelid == childId ||
+             (!found && is_descendent_of_internal(inhrelid, childId, seen_rels)))
+         {
+             ischild = true;
+             break;
+         }
+     }
+ 
+     systable_endscan(scan);
+     heap_close(inhrel, AccessShareLock);
+ 
+     return ischild;
+ }
*** a/src/backend/optimizer/prep/prepunion.c
--- b/src/backend/optimizer/prep/prepunion.c
***************
*** 108,123 **** static void expand_partitioned_rtentry(PlannerInfo *root,                            LOCKMODE
lockmode,                           bool *has_child, List **appinfos,                            List
**partitioned_child_rels);
! static void expand_single_inheritance_child(PlannerInfo *root,                                 RangeTblEntry
*parentrte,                                Index parentRTindex, Relation parentrel,
PlanRowMark*parentrc, Relation childrel,                                 bool *has_child, List **appinfos,
                  List **partitioned_child_rels);
 
! static void make_inh_translation_list(Relation oldrelation,                           Relation newrelation,
!                           Index newvarno,
!                           List **translated_vars); static Bitmapset *translate_col_privs(const Bitmapset
*parent_privs,                    List *translated_vars); static Node *adjust_appendrel_attrs_mutator(Node *node,
 
--- 108,122 ----                            LOCKMODE lockmode,                            bool *has_child, List
**appinfos,                           List **partitioned_child_rels);
 
! static bool expand_single_inheritance_child(PlannerInfo *root,                                 RangeTblEntry
*parentrte,                                Index parentRTindex, Relation parentrel,
PlanRowMark*parentrc, Relation childrel,                                 bool *has_child, List **appinfos,
                  List **partitioned_child_rels);
 
! static List *make_inh_translation_list(Relation oldrelation,                           Relation newrelation,
!                           Index newvarno); static Bitmapset *translate_col_privs(const Bitmapset *parent_privs,
             List *translated_vars); static Node *adjust_appendrel_attrs_mutator(Node *node,
 
***************
*** 1476,1481 **** expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
--- 1475,1482 ----          * in which they appear in the PartitionDesc.  But first, expand the          * parent
itself.         */
 
+ 
+         /* ignore the return value since this doesn't exclude the parent */
expand_single_inheritance_child(root,rte, rti, oldrelation, oldrc,                                         oldrelation,
                                       &has_child, &appinfos,
 
***************
*** 1497,1502 **** expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
--- 1498,1504 ----         {             Oid            childOID = lfirst_oid(l);             Relation    newrelation;
+             bool        expanded = false;              /* Open rel if needed; we already have required locks */
     if (childOID != parentOID)
 
***************
*** 1516,1529 **** expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)                 continue;
           } 
 
!             expand_single_inheritance_child(root, rte, rti, oldrelation, oldrc,
     newrelation,                                             &has_child, &appinfos,
        &partitioned_child_rels); 
 
!             /* Close child relations, but keep locks */             if (childOID != parentOID)
!                 heap_close(newrelation, NoLock);         }     } 
--- 1518,1532 ----                 continue;             } 
!             expanded = expand_single_inheritance_child(root,
!                                             rte, rti, oldrelation, oldrc,
newrelation,                                            &has_child, &appinfos,
  &partitioned_child_rels); 
 
!             /* Close child relations, but keep locks if expanded */             if (childOID != parentOID)
!                 heap_close(newrelation, expanded ? NoLock : lockmode);         }     } 
***************
*** 1581,1586 **** expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,
--- 1584,1590 ----     {         Oid            childOID = partdesc->oids[i];         Relation    childrel;
+         bool        expanded = false;          /* Open rel; we already have required locks */         childrel =
heap_open(childOID,NoLock);
 
***************
*** 1592,1598 **** expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,             continue;
} 
 
!         expand_single_inheritance_child(root, parentrte, parentRTindex,
parentrel,parentrc, childrel,                                         has_child, appinfos,
          partitioned_child_rels);
 
--- 1596,1603 ----             continue;         } 
!         expanded = expand_single_inheritance_child(root,
!                                         parentrte, parentRTindex,                                         parentrel,
parentrc,childrel,                                         has_child, appinfos,
partitioned_child_rels);
***************
*** 1606,1613 **** expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,
            has_child, appinfos,                                           partitioned_child_rels); 
 
!         /* Close child relation, but keep locks */
!         heap_close(childrel, NoLock);     } } 
--- 1611,1618 ----                                           has_child, appinfos,
   partitioned_child_rels); 
 
!         /* Close child relation, but keep locks if expanded */
!         heap_close(childrel, expanded ? NoLock : lockmode);     } } 
***************
*** 1619,1626 **** expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,  * anything at all.
Otherwise,we'll set "has_child" to true, build a  * RangeTblEntry and either a PartitionedChildRelInfo or AppendRelInfo
as * appropriate, plus maybe a PlanRowMark.  */
 
! static void expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte,
  Index parentRTindex, Relation parentrel,                                 PlanRowMark *parentrc, Relation childrel,
 
--- 1624,1634 ----  * anything at all.  Otherwise, we'll set "has_child" to true, build a  * RangeTblEntry and either a
PartitionedChildRelInfoor AppendRelInfo as  * appropriate, plus maybe a PlanRowMark.
 
+  *
+  * Returns false if the child has been found that no longer a child of the
+  * parent.  */
! static bool expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte,
  Index parentRTindex, Relation parentrel,                                 PlanRowMark *parentrc, Relation childrel,
 
***************
*** 1661,1666 **** expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte,
--- 1669,1692 ----      */     if (childrte->relkind != RELKIND_PARTITIONED_TABLE)     {
+         List *translated_vars =
+             make_inh_translation_list(parentrel, childrel, childRTindex);
+ 
+         /*
+          * It may happen that the childOID is no longer a child of the
+          * parent.
+          */
+         if (!translated_vars)
+         {
+             /*
+              * This childrel is no longer a child of the parent.  Return doing
+              * nothing. Halfway built childrte is abandoned but this happens
+              * really rarely.
+              */
+             Assert (childOID != parentOID);
+             return false;
+         }
+          /* Remember if we saw a real child. */         if (childOID != parentOID)             *has_child = true;
***************
*** 1670,1677 **** expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte,
appinfo->child_relid= childRTindex;         appinfo->parent_reltype = parentrel->rd_rel->reltype;
appinfo->child_reltype= childrel->rd_rel->reltype;
 
!         make_inh_translation_list(parentrel, childrel, childRTindex,
!                                   &appinfo->translated_vars);         appinfo->parent_reloid = parentOID;
*appinfos= lappend(*appinfos, appinfo); 
 
--- 1696,1702 ----         appinfo->child_relid = childRTindex;         appinfo->parent_reltype =
parentrel->rd_rel->reltype;        appinfo->child_reltype = childrel->rd_rel->reltype;
 
!         appinfo->translated_vars = translated_vars;         appinfo->parent_reloid = parentOID;         *appinfos =
lappend(*appinfos,appinfo); 
 
***************
*** 1726,1744 **** expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte,          root->rowMarks
=lappend(root->rowMarks, childrc);     } }  /*  * make_inh_translation_list  *      Build the list of translations from
parentVars to child Vars for
 
!  *      an inheritance child.  *  * For paranoia's sake, we match type/collation as well as attribute name.  */
! static void make_inh_translation_list(Relation oldrelation, Relation newrelation,
!                           Index newvarno,
!                           List **translated_vars) {     List       *vars = NIL;     TupleDesc    old_tupdesc =
RelationGetDescr(oldrelation);
--- 1751,1771 ----          root->rowMarks = lappend(root->rowMarks, childrc);     }
+ 
+     return true; }  /*  * make_inh_translation_list  *      Build the list of translations from parent Vars to child
Varsfor
 
!  *      an inheritance child. Returns NULL if the two relations are no longer in
!  *      a inheritance relationship.  *  * For paranoia's sake, we match type/collation as well as attribute name.
*/
! static List * make_inh_translation_list(Relation oldrelation, Relation newrelation,
!                           Index newvarno) {     List       *vars = NIL;     TupleDesc    old_tupdesc =
RelationGetDescr(oldrelation);
***************
*** 1808,1815 **** make_inh_translation_list(Relation oldrelation, Relation newrelation,
--- 1835,1852 ----                     break;             }             if (new_attno >= newnatts)
+             {
+                 /*
+                  * It's possible that newrelation is no longer a child of the
+                  * oldrelation. We just ingore it for the case.
+                  */
+                 if (!is_descendent_of(oldrelation->rd_id, newrelation->rd_id))
+                     return NULL;
+ 
+                 /* If still a child, something's wrong. */                 elog(ERROR, "could not find inherited
attribute\"%s\" of relation \"%s\"",                      attname, RelationGetRelationName(newrelation));
 
+             }         }          /* Found it, check type and collation match */
***************
*** 1828,1834 **** make_inh_translation_list(Relation oldrelation, Relation newrelation,
     0));     } 
 
!     *translated_vars = vars; }  /*
--- 1865,1871 ----                                      0));     } 
!     return vars; }  /*
*** a/src/include/catalog/pg_inherits_fn.h
--- b/src/include/catalog/pg_inherits_fn.h
***************
*** 23,27 **** extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,
--- 23,28 ---- extern bool has_subclass(Oid relationId); extern bool has_superclass(Oid relationId); extern bool
typeInheritsFrom(OidsubclassTypeId, Oid superclassTypeId);
 
+ extern bool is_descendent_of(Oid parentId, Oid childId);  #endif                            /* PG_INHERITS_FN_H */

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers