Missing dependency tracking for TableFunc nodes
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.
This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type NNNNN"
error because we allow dropping a type the XMLTABLE construct
references.
This isn't hard to fix, as per the attached patch, but it makes
me nervous. I wonder what other dependencies we might be missing.
Would it be a good idea to move find_expr_references_walker to
nodeFuncs.c, in hopes of making it more visible to people adding
new node types? We could decouple it from the specific use-case
of recordDependencyOnExpr by having it call a callback function
for each identified OID; although maybe there's no point in that,
since I'm not sure there are any other use-cases.
Another thought is that maybe the code could be automatically
generated, as Andres has been threatening to do with respect
to the other stuff in backend/nodes/.
In practice, this bug is probably not a huge problem, because a
view that involves a column of type X will likely have some other
dependencies on X. I had to tweak the example view a bit to get
it to not have any other dependencies on "seg". So I'm not feeling
that this is a stop-ship problem for today's releases --- I'll plan
on installing the fix after the releases are tagged.
regards, tom lane
Attachments:
fix-tablefunc-dependencies.patchtext/x-diff; charset=us-ascii; name=fix-tablefunc-dependencies.patchDownload
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 0358278..d07bb44 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2256,6 +2256,28 @@ find_expr_references_walker(Node *node,
context->addrs);
}
}
+ else if (IsA(node, TableFunc))
+ {
+ TableFunc *tf = (TableFunc *) node;
+ ListCell *ct;
+
+ /*
+ * Add refs for the datatypes and collations used in the TableFunc.
+ */
+ foreach(ct, tf->coltypes)
+ {
+ add_object_address(OCLASS_TYPE, lfirst_oid(ct), 0,
+ context->addrs);
+ }
+ foreach(ct, tf->colcollations)
+ {
+ Oid collid = lfirst_oid(ct);
+
+ if (OidIsValid(collid) && collid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, collid, 0,
+ context->addrs);
+ }
+ }
else if (IsA(node, TableSampleClause))
{
TableSampleClause *tsc = (TableSampleClause *) node;
On 11/11/19 1:41 PM, Tom Lane wrote:
Would it be a good idea to move find_expr_references_walker to
nodeFuncs.c, in hopes of making it more visible to people adding
new node types?
I'm not sure that would be enough. The logic of that function is not
immediately obvious, and where to add a node to it might not occur to
people. If the repeated use of
else if (IsA(node, XXX))
were replaced with
switch (nodeTag(node)) {
case XXX:
then the compiler, ala -Wswitch, would alert folks when they forget to
handle a new node type.
--
Mark Dilger
On 11/11/19 2:33 PM, Mark Dilger wrote:
On 11/11/19 1:41 PM, Tom Lane wrote:
Would it be a good idea to move find_expr_references_walker to
nodeFuncs.c, in hopes of making it more visible to people adding
new node types?I'm not sure that would be enough.� The logic of that function is not
immediately obvious, and where to add a node to it might not occur to
people.� If the repeated use of��� else if (IsA(node, XXX))
were replaced with
��� switch (nodeTag(node)) {
������� case XXX:then the compiler, ala -Wswitch, would alert folks when they forget to
handle a new node type.
I played with this a bit, making the change I proposed, and got lots of
warnings from the compiler. I don't know how many of these would need
to be suppressed by adding a no-op for them at the end of the switch vs.
how many need to be handled, but the attached patch implements the idea.
I admit adding all these extra cases to the end is verbose....
The change as written is much too verbose to be acceptable, but given
how many places in the code could really use this sort of treatment, I
wonder if there is a way to reorganize the NodeTag enum into multiple
enums, one for each logical subtype (such as executor nodes, plan nodes,
etc) and then have switches over enums of the given subtype, with the
compiler helping detect tags of same subtype that are unhandled in the
switch.
I have added enough nodes over the years, and spent enough time tracking
down all the parts of the code that need updating for a new node, to say
that this would be very helpful if we could make it work. I have not
done the research yet on how many places would be made less elegant by
such a change, though. I think I'll go look into that a bit....
--
Mark Dilger
Attachments:
nodeTag.WIP.1text/plain; charset=UTF-8; name=nodeTag.WIP.1Download
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 03582781f6..f1085b5d4d 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1740,529 +1740,955 @@ find_expr_references_walker(Node *node,
{
if (node == NULL)
return false;
- if (IsA(node, Var))
+ switch (nodeTag(node))
{
- Var *var = (Var *) node;
- List *rtable;
- RangeTblEntry *rte;
-
- /* Find matching rtable entry, or complain if not found */
- if (var->varlevelsup >= list_length(context->rtables))
- elog(ERROR, "invalid varlevelsup %d", var->varlevelsup);
- rtable = (List *) list_nth(context->rtables, var->varlevelsup);
- if (var->varno <= 0 || var->varno > list_length(rtable))
- elog(ERROR, "invalid varno %d", var->varno);
- rte = rt_fetch(var->varno, rtable);
+ case T_Var:
+ {
+ Var *var = (Var *) node;
+ List *rtable;
+ RangeTblEntry *rte;
- /*
- * A whole-row Var references no specific columns, so adds no new
- * dependency. (We assume that there is a whole-table dependency
- * arising from each underlying rangetable entry. While we could
- * record such a dependency when finding a whole-row Var that
- * references a relation directly, it's quite unclear how to extend
- * that to whole-row Vars for JOINs, so it seems better to leave the
- * responsibility with the range table. Note that this poses some
- * risks for identifying dependencies of stand-alone expressions:
- * whole-table references may need to be created separately.)
- */
- if (var->varattno == InvalidAttrNumber)
+ /* Find matching rtable entry, or complain if not found */
+ if (var->varlevelsup >= list_length(context->rtables))
+ elog(ERROR, "invalid varlevelsup %d", var->varlevelsup);
+ rtable = (List *) list_nth(context->rtables, var->varlevelsup);
+ if (var->varno <= 0 || var->varno > list_length(rtable))
+ elog(ERROR, "invalid varno %d", var->varno);
+ rte = rt_fetch(var->varno, rtable);
+
+ /*
+ * A whole-row Var references no specific columns, so adds no new
+ * dependency. (We assume that there is a whole-table dependency
+ * arising from each underlying rangetable entry. While we could
+ * record such a dependency when finding a whole-row Var that
+ * references a relation directly, it's quite unclear how to extend
+ * that to whole-row Vars for JOINs, so it seems better to leave the
+ * responsibility with the range table. Note that this poses some
+ * risks for identifying dependencies of stand-alone expressions:
+ * whole-table references may need to be created separately.)
+ */
+ if (var->varattno == InvalidAttrNumber)
+ return false;
+ if (rte->rtekind == RTE_RELATION)
+ {
+ /* If it's a plain relation, reference this column */
+ add_object_address(OCLASS_CLASS, rte->relid, var->varattno,
+ context->addrs);
+ }
+ else if (rte->rtekind == RTE_JOIN)
+ {
+ /* Scan join output column to add references to join inputs */
+ List *save_rtables;
+
+ /* We must make the context appropriate for join's level */
+ save_rtables = context->rtables;
+ context->rtables = list_copy_tail(context->rtables,
+ var->varlevelsup);
+ if (var->varattno <= 0 ||
+ var->varattno > list_length(rte->joinaliasvars))
+ elog(ERROR, "invalid varattno %d", var->varattno);
+ find_expr_references_walker((Node *) list_nth(rte->joinaliasvars,
+ var->varattno - 1),
+ context);
+ list_free(context->rtables);
+ context->rtables = save_rtables;
+ }
return false;
- if (rte->rtekind == RTE_RELATION)
- {
- /* If it's a plain relation, reference this column */
- add_object_address(OCLASS_CLASS, rte->relid, var->varattno,
- context->addrs);
}
- else if (rte->rtekind == RTE_JOIN)
+ break;
+ case T_Const:
{
- /* Scan join output column to add references to join inputs */
- List *save_rtables;
-
- /* We must make the context appropriate for join's level */
- save_rtables = context->rtables;
- context->rtables = list_copy_tail(context->rtables,
- var->varlevelsup);
- if (var->varattno <= 0 ||
- var->varattno > list_length(rte->joinaliasvars))
- elog(ERROR, "invalid varattno %d", var->varattno);
- find_expr_references_walker((Node *) list_nth(rte->joinaliasvars,
- var->varattno - 1),
- context);
- list_free(context->rtables);
- context->rtables = save_rtables;
- }
- return false;
- }
- else if (IsA(node, Const))
- {
- Const *con = (Const *) node;
- Oid objoid;
+ Const *con = (Const *) node;
+ Oid objoid;
- /* A constant must depend on the constant's datatype */
- add_object_address(OCLASS_TYPE, con->consttype, 0,
- context->addrs);
-
- /*
- * We must also depend on the constant's collation: it could be
- * different from the datatype's, if a CollateExpr was const-folded to
- * a simple constant. However we can save work in the most common
- * case where the collation is "default", since we know that's pinned.
- */
- if (OidIsValid(con->constcollid) &&
- con->constcollid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, con->constcollid, 0,
+ /* A constant must depend on the constant's datatype */
+ add_object_address(OCLASS_TYPE, con->consttype, 0,
context->addrs);
- /*
- * If it's a regclass or similar literal referring to an existing
- * object, add a reference to that object. (Currently, only the
- * regclass and regconfig cases have any likely use, but we may as
- * well handle all the OID-alias datatypes consistently.)
- */
- if (!con->constisnull)
- {
- switch (con->consttype)
- {
- case REGPROCOID:
- case REGPROCEDUREOID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(PROCOID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_PROC, objoid, 0,
- context->addrs);
- break;
- case REGOPEROID:
- case REGOPERATOROID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(OPEROID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_OPERATOR, objoid, 0,
- context->addrs);
- break;
- case REGCLASSOID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(RELOID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_CLASS, objoid, 0,
- context->addrs);
- break;
- case REGTYPEOID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(TYPEOID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_TYPE, objoid, 0,
- context->addrs);
- break;
- case REGCONFIGOID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(TSCONFIGOID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_TSCONFIG, objoid, 0,
- context->addrs);
- break;
- case REGDICTIONARYOID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(TSDICTOID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_TSDICT, objoid, 0,
- context->addrs);
- break;
-
- case REGNAMESPACEOID:
- objoid = DatumGetObjectId(con->constvalue);
- if (SearchSysCacheExists1(NAMESPACEOID,
- ObjectIdGetDatum(objoid)))
- add_object_address(OCLASS_SCHEMA, objoid, 0,
- context->addrs);
- break;
+ /*
+ * We must also depend on the constant's collation: it could be
+ * different from the datatype's, if a CollateExpr was const-folded to
+ * a simple constant. However we can save work in the most common
+ * case where the collation is "default", since we know that's pinned.
+ */
+ if (OidIsValid(con->constcollid) &&
+ con->constcollid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, con->constcollid, 0,
+ context->addrs);
- /*
- * Dependencies for regrole should be shared among all
- * databases, so explicitly inhibit to have dependencies.
- */
- case REGROLEOID:
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("constant of the type %s cannot be used here",
- "regrole")));
- break;
+ /*
+ * If it's a regclass or similar literal referring to an existing
+ * object, add a reference to that object. (Currently, only the
+ * regclass and regconfig cases have any likely use, but we may as
+ * well handle all the OID-alias datatypes consistently.)
+ */
+ if (!con->constisnull)
+ {
+ switch (con->consttype)
+ {
+ case REGPROCOID:
+ case REGPROCEDUREOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(PROCOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_PROC, objoid, 0,
+ context->addrs);
+ break;
+ case REGOPEROID:
+ case REGOPERATOROID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(OPEROID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_OPERATOR, objoid, 0,
+ context->addrs);
+ break;
+ case REGCLASSOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(RELOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_CLASS, objoid, 0,
+ context->addrs);
+ break;
+ case REGTYPEOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(TYPEOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_TYPE, objoid, 0,
+ context->addrs);
+ break;
+ case REGCONFIGOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(TSCONFIGOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_TSCONFIG, objoid, 0,
+ context->addrs);
+ break;
+ case REGDICTIONARYOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(TSDICTOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_TSDICT, objoid, 0,
+ context->addrs);
+ break;
+
+ case REGNAMESPACEOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(NAMESPACEOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_SCHEMA, objoid, 0,
+ context->addrs);
+ break;
+
+ /*
+ * Dependencies for regrole should be shared among all
+ * databases, so explicitly inhibit to have dependencies.
+ */
+ case REGROLEOID:
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("constant of the type %s cannot be used here",
+ "regrole")));
+ break;
+ }
}
+ return false;
}
- return false;
- }
- else if (IsA(node, Param))
- {
- Param *param = (Param *) node;
-
- /* A parameter must depend on the parameter's datatype */
- add_object_address(OCLASS_TYPE, param->paramtype, 0,
- context->addrs);
- /* and its collation, just as for Consts */
- if (OidIsValid(param->paramcollid) &&
- param->paramcollid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, param->paramcollid, 0,
- context->addrs);
- }
- else if (IsA(node, FuncExpr))
- {
- FuncExpr *funcexpr = (FuncExpr *) node;
-
- add_object_address(OCLASS_PROC, funcexpr->funcid, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, OpExpr))
- {
- OpExpr *opexpr = (OpExpr *) node;
-
- add_object_address(OCLASS_OPERATOR, opexpr->opno, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, DistinctExpr))
- {
- DistinctExpr *distinctexpr = (DistinctExpr *) node;
-
- add_object_address(OCLASS_OPERATOR, distinctexpr->opno, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, NullIfExpr))
- {
- NullIfExpr *nullifexpr = (NullIfExpr *) node;
+ break;
+ case T_Param:
+ {
+ Param *param = (Param *) node;
- add_object_address(OCLASS_OPERATOR, nullifexpr->opno, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, ScalarArrayOpExpr))
- {
- ScalarArrayOpExpr *opexpr = (ScalarArrayOpExpr *) node;
+ /* A parameter must depend on the parameter's datatype */
+ add_object_address(OCLASS_TYPE, param->paramtype, 0,
+ context->addrs);
+ /* and its collation, just as for Consts */
+ if (OidIsValid(param->paramcollid) &&
+ param->paramcollid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, param->paramcollid, 0,
+ context->addrs);
+ }
+ break;
+ case T_FuncExpr:
+ {
+ FuncExpr *funcexpr = (FuncExpr *) node;
- add_object_address(OCLASS_OPERATOR, opexpr->opno, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, Aggref))
- {
- Aggref *aggref = (Aggref *) node;
+ add_object_address(OCLASS_PROC, funcexpr->funcid, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_OpExpr:
+ {
+ OpExpr *opexpr = (OpExpr *) node;
- add_object_address(OCLASS_PROC, aggref->aggfnoid, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, WindowFunc))
- {
- WindowFunc *wfunc = (WindowFunc *) node;
+ add_object_address(OCLASS_OPERATOR, opexpr->opno, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_DistinctExpr:
+ {
+ DistinctExpr *distinctexpr = (DistinctExpr *) node;
- add_object_address(OCLASS_PROC, wfunc->winfnoid, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, SubPlan))
- {
- /* Extra work needed here if we ever need this case */
- elog(ERROR, "already-planned subqueries not supported");
- }
- else if (IsA(node, FieldSelect))
- {
- FieldSelect *fselect = (FieldSelect *) node;
- Oid argtype = getBaseType(exprType((Node *) fselect->arg));
- Oid reltype = get_typ_typrelid(argtype);
+ add_object_address(OCLASS_OPERATOR, distinctexpr->opno, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_NullIfExpr:
+ {
+ NullIfExpr *nullifexpr = (NullIfExpr *) node;
- /*
- * We need a dependency on the specific column named in FieldSelect,
- * assuming we can identify the pg_class OID for it. (Probably we
- * always can at the moment, but in future it might be possible for
- * argtype to be RECORDOID.) If we can make a column dependency then
- * we shouldn't need a dependency on the column's type; but if we
- * can't, make a dependency on the type, as it might not appear
- * anywhere else in the expression.
- */
- if (OidIsValid(reltype))
- add_object_address(OCLASS_CLASS, reltype, fselect->fieldnum,
+ add_object_address(OCLASS_OPERATOR, nullifexpr->opno, 0,
context->addrs);
- else
- add_object_address(OCLASS_TYPE, fselect->resulttype, 0,
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_ScalarArrayOpExpr:
+ {
+ ScalarArrayOpExpr *opexpr = (ScalarArrayOpExpr *) node;
+
+ add_object_address(OCLASS_OPERATOR, opexpr->opno, 0,
context->addrs);
- /* the collation might not be referenced anywhere else, either */
- if (OidIsValid(fselect->resultcollid) &&
- fselect->resultcollid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, fselect->resultcollid, 0,
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_Aggref:
+ {
+ Aggref *aggref = (Aggref *) node;
+
+ add_object_address(OCLASS_PROC, aggref->aggfnoid, 0,
context->addrs);
- }
- else if (IsA(node, FieldStore))
- {
- FieldStore *fstore = (FieldStore *) node;
- Oid reltype = get_typ_typrelid(fstore->resulttype);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_WindowFunc:
+ {
+ WindowFunc *wfunc = (WindowFunc *) node;
- /* similar considerations to FieldSelect, but multiple column(s) */
- if (OidIsValid(reltype))
+ add_object_address(OCLASS_PROC, wfunc->winfnoid, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_SubPlan:
{
- ListCell *l;
+ /* Extra work needed here if we ever need this case */
+ elog(ERROR, "already-planned subqueries not supported");
+ }
+ break;
+ case T_FieldSelect:
+ {
+ FieldSelect *fselect = (FieldSelect *) node;
+ Oid argtype = getBaseType(exprType((Node *) fselect->arg));
+ Oid reltype = get_typ_typrelid(argtype);
- foreach(l, fstore->fieldnums)
- add_object_address(OCLASS_CLASS, reltype, lfirst_int(l),
+ /*
+ * We need a dependency on the specific column named in FieldSelect,
+ * assuming we can identify the pg_class OID for it. (Probably we
+ * always can at the moment, but in future it might be possible for
+ * argtype to be RECORDOID.) If we can make a column dependency then
+ * we shouldn't need a dependency on the column's type; but if we
+ * can't, make a dependency on the type, as it might not appear
+ * anywhere else in the expression.
+ */
+ if (OidIsValid(reltype))
+ add_object_address(OCLASS_CLASS, reltype, fselect->fieldnum,
+ context->addrs);
+ else
+ add_object_address(OCLASS_TYPE, fselect->resulttype, 0,
+ context->addrs);
+ /* the collation might not be referenced anywhere else, either */
+ if (OidIsValid(fselect->resultcollid) &&
+ fselect->resultcollid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, fselect->resultcollid, 0,
context->addrs);
}
- else
- add_object_address(OCLASS_TYPE, fstore->resulttype, 0,
- context->addrs);
- }
- else if (IsA(node, RelabelType))
- {
- RelabelType *relab = (RelabelType *) node;
-
- /* since there is no function dependency, need to depend on type */
- add_object_address(OCLASS_TYPE, relab->resulttype, 0,
- context->addrs);
- /* the collation might not be referenced anywhere else, either */
- if (OidIsValid(relab->resultcollid) &&
- relab->resultcollid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, relab->resultcollid, 0,
- context->addrs);
- }
- else if (IsA(node, CoerceViaIO))
- {
- CoerceViaIO *iocoerce = (CoerceViaIO *) node;
-
- /* since there is no exposed function, need to depend on type */
- add_object_address(OCLASS_TYPE, iocoerce->resulttype, 0,
- context->addrs);
- /* the collation might not be referenced anywhere else, either */
- if (OidIsValid(iocoerce->resultcollid) &&
- iocoerce->resultcollid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, iocoerce->resultcollid, 0,
- context->addrs);
- }
- else if (IsA(node, ArrayCoerceExpr))
- {
- ArrayCoerceExpr *acoerce = (ArrayCoerceExpr *) node;
-
- /* as above, depend on type */
- add_object_address(OCLASS_TYPE, acoerce->resulttype, 0,
- context->addrs);
- /* the collation might not be referenced anywhere else, either */
- if (OidIsValid(acoerce->resultcollid) &&
- acoerce->resultcollid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, acoerce->resultcollid, 0,
- context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, ConvertRowtypeExpr))
- {
- ConvertRowtypeExpr *cvt = (ConvertRowtypeExpr *) node;
-
- /* since there is no function dependency, need to depend on type */
- add_object_address(OCLASS_TYPE, cvt->resulttype, 0,
- context->addrs);
- }
- else if (IsA(node, CollateExpr))
- {
- CollateExpr *coll = (CollateExpr *) node;
-
- add_object_address(OCLASS_COLLATION, coll->collOid, 0,
- context->addrs);
- }
- else if (IsA(node, RowExpr))
- {
- RowExpr *rowexpr = (RowExpr *) node;
+ break;
+ case T_FieldStore:
+ {
+ FieldStore *fstore = (FieldStore *) node;
+ Oid reltype = get_typ_typrelid(fstore->resulttype);
- add_object_address(OCLASS_TYPE, rowexpr->row_typeid, 0,
- context->addrs);
- }
- else if (IsA(node, RowCompareExpr))
- {
- RowCompareExpr *rcexpr = (RowCompareExpr *) node;
- ListCell *l;
+ /* similar considerations to FieldSelect, but multiple column(s) */
+ if (OidIsValid(reltype))
+ {
+ ListCell *l;
- foreach(l, rcexpr->opnos)
+ foreach(l, fstore->fieldnums)
+ add_object_address(OCLASS_CLASS, reltype, lfirst_int(l),
+ context->addrs);
+ }
+ else
+ add_object_address(OCLASS_TYPE, fstore->resulttype, 0,
+ context->addrs);
+ }
+ break;
+ case T_RelabelType:
{
- add_object_address(OCLASS_OPERATOR, lfirst_oid(l), 0,
+ RelabelType *relab = (RelabelType *) node;
+
+ /* since there is no function dependency, need to depend on type */
+ add_object_address(OCLASS_TYPE, relab->resulttype, 0,
context->addrs);
+ /* the collation might not be referenced anywhere else, either */
+ if (OidIsValid(relab->resultcollid) &&
+ relab->resultcollid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, relab->resultcollid, 0,
+ context->addrs);
}
- foreach(l, rcexpr->opfamilies)
+ break;
+ case T_CoerceViaIO:
{
- add_object_address(OCLASS_OPFAMILY, lfirst_oid(l), 0,
+ CoerceViaIO *iocoerce = (CoerceViaIO *) node;
+
+ /* since there is no exposed function, need to depend on type */
+ add_object_address(OCLASS_TYPE, iocoerce->resulttype, 0,
context->addrs);
+ /* the collation might not be referenced anywhere else, either */
+ if (OidIsValid(iocoerce->resultcollid) &&
+ iocoerce->resultcollid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, iocoerce->resultcollid, 0,
+ context->addrs);
}
- /* fall through to examine arguments */
- }
- else if (IsA(node, CoerceToDomain))
- {
- CoerceToDomain *cd = (CoerceToDomain *) node;
-
- add_object_address(OCLASS_TYPE, cd->resulttype, 0,
- context->addrs);
- }
- else if (IsA(node, NextValueExpr))
- {
- NextValueExpr *nve = (NextValueExpr *) node;
+ break;
+ case T_ArrayCoerceExpr:
+ {
+ ArrayCoerceExpr *acoerce = (ArrayCoerceExpr *) node;
- add_object_address(OCLASS_CLASS, nve->seqid, 0,
- context->addrs);
- }
- else if (IsA(node, OnConflictExpr))
- {
- OnConflictExpr *onconflict = (OnConflictExpr *) node;
+ /* as above, depend on type */
+ add_object_address(OCLASS_TYPE, acoerce->resulttype, 0,
+ context->addrs);
+ /* the collation might not be referenced anywhere else, either */
+ if (OidIsValid(acoerce->resultcollid) &&
+ acoerce->resultcollid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, acoerce->resultcollid, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_ConvertRowtypeExpr:
+ {
+ ConvertRowtypeExpr *cvt = (ConvertRowtypeExpr *) node;
- if (OidIsValid(onconflict->constraint))
- add_object_address(OCLASS_CONSTRAINT, onconflict->constraint, 0,
+ /* since there is no function dependency, need to depend on type */
+ add_object_address(OCLASS_TYPE, cvt->resulttype, 0,
context->addrs);
- /* fall through to examine arguments */
- }
- else if (IsA(node, SortGroupClause))
- {
- SortGroupClause *sgc = (SortGroupClause *) node;
+ }
+ break;
+ case T_CollateExpr:
+ {
+ CollateExpr *coll = (CollateExpr *) node;
- add_object_address(OCLASS_OPERATOR, sgc->eqop, 0,
- context->addrs);
- if (OidIsValid(sgc->sortop))
- add_object_address(OCLASS_OPERATOR, sgc->sortop, 0,
+ add_object_address(OCLASS_COLLATION, coll->collOid, 0,
context->addrs);
- return false;
- }
- else if (IsA(node, WindowClause))
- {
- WindowClause *wc = (WindowClause *) node;
+ }
+ break;
+ case T_RowExpr:
+ {
+ RowExpr *rowexpr = (RowExpr *) node;
- if (OidIsValid(wc->startInRangeFunc))
- add_object_address(OCLASS_PROC, wc->startInRangeFunc, 0,
+ add_object_address(OCLASS_TYPE, rowexpr->row_typeid, 0,
context->addrs);
- if (OidIsValid(wc->endInRangeFunc))
- add_object_address(OCLASS_PROC, wc->endInRangeFunc, 0,
+ }
+ break;
+ case T_RowCompareExpr:
+ {
+ RowCompareExpr *rcexpr = (RowCompareExpr *) node;
+ ListCell *l;
+
+ foreach(l, rcexpr->opnos)
+ {
+ add_object_address(OCLASS_OPERATOR, lfirst_oid(l), 0,
+ context->addrs);
+ }
+ foreach(l, rcexpr->opfamilies)
+ {
+ add_object_address(OCLASS_OPFAMILY, lfirst_oid(l), 0,
+ context->addrs);
+ }
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_CoerceToDomain:
+ {
+ CoerceToDomain *cd = (CoerceToDomain *) node;
+
+ add_object_address(OCLASS_TYPE, cd->resulttype, 0,
context->addrs);
- if (OidIsValid(wc->inRangeColl) &&
- wc->inRangeColl != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, wc->inRangeColl, 0,
+ }
+ break;
+ case T_NextValueExpr:
+ {
+ NextValueExpr *nve = (NextValueExpr *) node;
+
+ add_object_address(OCLASS_CLASS, nve->seqid, 0,
context->addrs);
- /* fall through to examine substructure */
- }
- else if (IsA(node, Query))
- {
- /* Recurse into RTE subquery or not-yet-planned sublink subquery */
- Query *query = (Query *) node;
- ListCell *lc;
- bool result;
+ }
+ break;
+ case T_OnConflictExpr:
+ {
+ OnConflictExpr *onconflict = (OnConflictExpr *) node;
- /*
- * Add whole-relation refs for each plain relation mentioned in the
- * subquery's rtable.
- *
- * Note: query_tree_walker takes care of recursing into RTE_FUNCTION
- * RTEs, subqueries, etc, so no need to do that here. But keep it
- * from looking at join alias lists.
- *
- * Note: we don't need to worry about collations mentioned in
- * RTE_VALUES or RTE_CTE RTEs, because those must just duplicate
- * collations referenced in other parts of the Query. We do have to
- * worry about collations mentioned in RTE_FUNCTION, but we take care
- * of those when we recurse to the RangeTblFunction node(s).
- */
- foreach(lc, query->rtable)
+ if (OidIsValid(onconflict->constraint))
+ add_object_address(OCLASS_CONSTRAINT, onconflict->constraint, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+ case T_SortGroupClause:
{
- RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+ SortGroupClause *sgc = (SortGroupClause *) node;
- switch (rte->rtekind)
- {
- case RTE_RELATION:
- add_object_address(OCLASS_CLASS, rte->relid, 0,
- context->addrs);
- break;
- default:
- break;
- }
+ add_object_address(OCLASS_OPERATOR, sgc->eqop, 0,
+ context->addrs);
+ if (OidIsValid(sgc->sortop))
+ add_object_address(OCLASS_OPERATOR, sgc->sortop, 0,
+ context->addrs);
+ return false;
}
+ break;
+ case T_WindowClause:
+ {
+ WindowClause *wc = (WindowClause *) node;
- /*
- * If the query is an INSERT or UPDATE, we should create a dependency
- * on each target column, to prevent the specific target column from
- * being dropped. Although we will visit the TargetEntry nodes again
- * during query_tree_walker, we won't have enough context to do this
- * conveniently, so do it here.
- */
- if (query->commandType == CMD_INSERT ||
- query->commandType == CMD_UPDATE)
+ if (OidIsValid(wc->startInRangeFunc))
+ add_object_address(OCLASS_PROC, wc->startInRangeFunc, 0,
+ context->addrs);
+ if (OidIsValid(wc->endInRangeFunc))
+ add_object_address(OCLASS_PROC, wc->endInRangeFunc, 0,
+ context->addrs);
+ if (OidIsValid(wc->inRangeColl) &&
+ wc->inRangeColl != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, wc->inRangeColl, 0,
+ context->addrs);
+ /* fall through to examine substructure */
+ }
+ break;
+ case T_Query:
{
- RangeTblEntry *rte;
+ /* Recurse into RTE subquery or not-yet-planned sublink subquery */
+ Query *query = (Query *) node;
+ ListCell *lc;
+ bool result;
- if (query->resultRelation <= 0 ||
- query->resultRelation > list_length(query->rtable))
- elog(ERROR, "invalid resultRelation %d",
- query->resultRelation);
- rte = rt_fetch(query->resultRelation, query->rtable);
- if (rte->rtekind == RTE_RELATION)
+ /*
+ * Add whole-relation refs for each plain relation mentioned in the
+ * subquery's rtable.
+ *
+ * Note: query_tree_walker takes care of recursing into RTE_FUNCTION
+ * RTEs, subqueries, etc, so no need to do that here. But keep it
+ * from looking at join alias lists.
+ *
+ * Note: we don't need to worry about collations mentioned in
+ * RTE_VALUES or RTE_CTE RTEs, because those must just duplicate
+ * collations referenced in other parts of the Query. We do have to
+ * worry about collations mentioned in RTE_FUNCTION, but we take care
+ * of those when we recurse to the RangeTblFunction node(s).
+ */
+ foreach(lc, query->rtable)
{
- foreach(lc, query->targetList)
- {
- TargetEntry *tle = (TargetEntry *) lfirst(lc);
+ RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
- if (tle->resjunk)
- continue; /* ignore junk tlist items */
- add_object_address(OCLASS_CLASS, rte->relid, tle->resno,
- context->addrs);
+ switch (rte->rtekind)
+ {
+ case RTE_RELATION:
+ add_object_address(OCLASS_CLASS, rte->relid, 0,
+ context->addrs);
+ break;
+ default:
+ break;
}
}
- }
- /*
- * Add dependencies on constraints listed in query's constraintDeps
- */
- foreach(lc, query->constraintDeps)
- {
- add_object_address(OCLASS_CONSTRAINT, lfirst_oid(lc), 0,
- context->addrs);
- }
+ /*
+ * If the query is an INSERT or UPDATE, we should create a dependency
+ * on each target column, to prevent the specific target column from
+ * being dropped. Although we will visit the TargetEntry nodes again
+ * during query_tree_walker, we won't have enough context to do this
+ * conveniently, so do it here.
+ */
+ if (query->commandType == CMD_INSERT ||
+ query->commandType == CMD_UPDATE)
+ {
+ RangeTblEntry *rte;
+
+ if (query->resultRelation <= 0 ||
+ query->resultRelation > list_length(query->rtable))
+ elog(ERROR, "invalid resultRelation %d",
+ query->resultRelation);
+ rte = rt_fetch(query->resultRelation, query->rtable);
+ if (rte->rtekind == RTE_RELATION)
+ {
+ foreach(lc, query->targetList)
+ {
+ TargetEntry *tle = (TargetEntry *) lfirst(lc);
- /* Examine substructure of query */
- context->rtables = lcons(query->rtable, context->rtables);
- result = query_tree_walker(query,
- find_expr_references_walker,
- (void *) context,
- QTW_IGNORE_JOINALIASES |
- QTW_EXAMINE_SORTGROUP);
- context->rtables = list_delete_first(context->rtables);
- return result;
- }
- else if (IsA(node, SetOperationStmt))
- {
- SetOperationStmt *setop = (SetOperationStmt *) node;
+ if (tle->resjunk)
+ continue; /* ignore junk tlist items */
+ add_object_address(OCLASS_CLASS, rte->relid, tle->resno,
+ context->addrs);
+ }
+ }
+ }
- /* we need to look at the groupClauses for operator references */
- find_expr_references_walker((Node *) setop->groupClauses, context);
- /* fall through to examine child nodes */
- }
- else if (IsA(node, RangeTblFunction))
- {
- RangeTblFunction *rtfunc = (RangeTblFunction *) node;
- ListCell *ct;
+ /*
+ * Add dependencies on constraints listed in query's constraintDeps
+ */
+ foreach(lc, query->constraintDeps)
+ {
+ add_object_address(OCLASS_CONSTRAINT, lfirst_oid(lc), 0,
+ context->addrs);
+ }
- /*
- * Add refs for any datatypes and collations used in a column
- * definition list for a RECORD function. (For other cases, it should
- * be enough to depend on the function itself.)
- */
- foreach(ct, rtfunc->funccoltypes)
+ /* Examine substructure of query */
+ context->rtables = lcons(query->rtable, context->rtables);
+ result = query_tree_walker(query,
+ find_expr_references_walker,
+ (void *) context,
+ QTW_IGNORE_JOINALIASES |
+ QTW_EXAMINE_SORTGROUP);
+ context->rtables = list_delete_first(context->rtables);
+ return result;
+ }
+ break;
+ case T_SetOperationStmt:
{
- add_object_address(OCLASS_TYPE, lfirst_oid(ct), 0,
- context->addrs);
+ SetOperationStmt *setop = (SetOperationStmt *) node;
+
+ /* we need to look at the groupClauses for operator references */
+ find_expr_references_walker((Node *) setop->groupClauses, context);
+ /* fall through to examine child nodes */
}
- foreach(ct, rtfunc->funccolcollations)
+ break;
+ case T_RangeTblFunction:
{
- Oid collid = lfirst_oid(ct);
+ RangeTblFunction *rtfunc = (RangeTblFunction *) node;
+ ListCell *ct;
- if (OidIsValid(collid) && collid != DEFAULT_COLLATION_OID)
- add_object_address(OCLASS_COLLATION, collid, 0,
+ /*
+ * Add refs for any datatypes and collations used in a column
+ * definition list for a RECORD function. (For other cases, it should
+ * be enough to depend on the function itself.)
+ */
+ foreach(ct, rtfunc->funccoltypes)
+ {
+ add_object_address(OCLASS_TYPE, lfirst_oid(ct), 0,
context->addrs);
+ }
+ foreach(ct, rtfunc->funccolcollations)
+ {
+ Oid collid = lfirst_oid(ct);
+
+ if (OidIsValid(collid) && collid != DEFAULT_COLLATION_OID)
+ add_object_address(OCLASS_COLLATION, collid, 0,
+ context->addrs);
+ }
}
- }
- else if (IsA(node, TableSampleClause))
- {
- TableSampleClause *tsc = (TableSampleClause *) node;
+ break;
+ case T_TableSampleClause:
+ {
+ TableSampleClause *tsc = (TableSampleClause *) node;
+
+ add_object_address(OCLASS_PROC, tsc->tsmhandler, 0,
+ context->addrs);
+ /* fall through to examine arguments */
+ }
+ break;
+
+ case T_Invalid:
+ case T_IndexInfo:
+ case T_ExprContext:
+ case T_ProjectionInfo:
+ case T_JunkFilter:
+ case T_OnConflictSetState:
+ case T_ResultRelInfo:
+ case T_EState:
+ case T_TupleTableSlot:
+ case T_Plan:
+ case T_Result:
+ case T_ProjectSet:
+ case T_ModifyTable:
+ case T_Append:
+ case T_MergeAppend:
+ case T_RecursiveUnion:
+ case T_BitmapAnd:
+ case T_BitmapOr:
+ case T_Scan:
+ case T_SeqScan:
+ case T_SampleScan:
+ case T_IndexScan:
+ case T_IndexOnlyScan:
+ case T_BitmapIndexScan:
+ case T_BitmapHeapScan:
+ case T_TidScan:
+ case T_SubqueryScan:
+ case T_FunctionScan:
+ case T_ValuesScan:
+ case T_TableFuncScan:
+ case T_CteScan:
+ case T_NamedTuplestoreScan:
+ case T_WorkTableScan:
+ case T_ForeignScan:
+ case T_CustomScan:
+ case T_Join:
+ case T_NestLoop:
+ case T_MergeJoin:
+ case T_HashJoin:
+ case T_Material:
+ case T_Sort:
+ case T_Group:
+ case T_Agg:
+ case T_WindowAgg:
+ case T_Unique:
+ case T_Gather:
+ case T_GatherMerge:
+ case T_Hash:
+ case T_SetOp:
+ case T_LockRows:
+ case T_Limit:
+ case T_NestLoopParam:
+ case T_PlanRowMark:
+ case T_PartitionPruneInfo:
+ case T_PartitionedRelPruneInfo:
+ case T_PartitionPruneStepOp:
+ case T_PartitionPruneStepCombine:
+ case T_PlanInvalItem:
+ case T_PlanState:
+ case T_ResultState:
+ case T_ProjectSetState:
+ case T_ModifyTableState:
+ case T_AppendState:
+ case T_MergeAppendState:
+ case T_RecursiveUnionState:
+ case T_BitmapAndState:
+ case T_BitmapOrState:
+ case T_ScanState:
+ case T_SeqScanState:
+ case T_SampleScanState:
+ case T_IndexScanState:
+ case T_IndexOnlyScanState:
+ case T_BitmapIndexScanState:
+ case T_BitmapHeapScanState:
+ case T_TidScanState:
+ case T_SubqueryScanState:
+ case T_FunctionScanState:
+ case T_TableFuncScanState:
+ case T_ValuesScanState:
+ case T_CteScanState:
+ case T_NamedTuplestoreScanState:
+ case T_WorkTableScanState:
+ case T_ForeignScanState:
+ case T_CustomScanState:
+ case T_JoinState:
+ case T_NestLoopState:
+ case T_MergeJoinState:
+ case T_HashJoinState:
+ case T_MaterialState:
+ case T_SortState:
+ case T_GroupState:
+ case T_AggState:
+ case T_WindowAggState:
+ case T_UniqueState:
+ case T_GatherState:
+ case T_GatherMergeState:
+ case T_HashState:
+ case T_SetOpState:
+ case T_LockRowsState:
+ case T_LimitState:
+ case T_Alias:
+ case T_RangeVar:
+ case T_TableFunc:
+ case T_Expr:
+ case T_GroupingFunc:
+ case T_SubscriptingRef:
+ case T_NamedArgExpr:
+ case T_BoolExpr:
+ case T_SubLink:
+ case T_AlternativeSubPlan:
+ case T_CaseExpr:
+ case T_CaseWhen:
+ case T_CaseTestExpr:
+ case T_ArrayExpr:
+ case T_CoalesceExpr:
+ case T_MinMaxExpr:
+ case T_SQLValueFunction:
+ case T_XmlExpr:
+ case T_NullTest:
+ case T_BooleanTest:
+ case T_CoerceToDomainValue:
+ case T_SetToDefault:
+ case T_CurrentOfExpr:
+ case T_InferenceElem:
+ case T_TargetEntry:
+ case T_RangeTblRef:
+ case T_JoinExpr:
+ case T_FromExpr:
+ case T_IntoClause:
+ case T_ExprState:
+ case T_AggrefExprState:
+ case T_WindowFuncExprState:
+ case T_SetExprState:
+ case T_SubPlanState:
+ case T_AlternativeSubPlanState:
+ case T_DomainConstraintState:
+ case T_PlannerInfo:
+ case T_PlannerGlobal:
+ case T_RelOptInfo:
+ case T_IndexOptInfo:
+ case T_ForeignKeyOptInfo:
+ case T_ParamPathInfo:
+ case T_Path:
+ case T_IndexPath:
+ case T_BitmapHeapPath:
+ case T_BitmapAndPath:
+ case T_BitmapOrPath:
+ case T_TidPath:
+ case T_SubqueryScanPath:
+ case T_ForeignPath:
+ case T_CustomPath:
+ case T_NestPath:
+ case T_MergePath:
+ case T_HashPath:
+ case T_AppendPath:
+ case T_MergeAppendPath:
+ case T_GroupResultPath:
+ case T_MaterialPath:
+ case T_UniquePath:
+ case T_GatherPath:
+ case T_GatherMergePath:
+ case T_ProjectionPath:
+ case T_ProjectSetPath:
+ case T_SortPath:
+ case T_GroupPath:
+ case T_UpperUniquePath:
+ case T_AggPath:
+ case T_GroupingSetsPath:
+ case T_MinMaxAggPath:
+ case T_WindowAggPath:
+ case T_SetOpPath:
+ case T_RecursiveUnionPath:
+ case T_LockRowsPath:
+ case T_ModifyTablePath:
+ case T_LimitPath:
+ case T_EquivalenceClass:
+ case T_EquivalenceMember:
+ case T_PathKey:
+ case T_PathTarget:
+ case T_RestrictInfo:
+ case T_IndexClause:
+ case T_PlaceHolderVar:
+ case T_SpecialJoinInfo:
+ case T_AppendRelInfo:
+ case T_PlaceHolderInfo:
+ case T_MinMaxAggInfo:
+ case T_PlannerParamItem:
+ case T_RollupData:
+ case T_GroupingSetData:
+ case T_StatisticExtInfo:
+ case T_MemoryContext:
+ case T_AllocSetContext:
+ case T_SlabContext:
+ case T_GenerationContext:
+ case T_Value:
+ case T_Integer:
+ case T_Float:
+ case T_String:
+ case T_BitString:
+ case T_Null:
+ case T_List:
+ case T_IntList:
+ case T_OidList:
+ case T_ExtensibleNode:
+ case T_RawStmt:
+ case T_PlannedStmt:
+ case T_InsertStmt:
+ case T_DeleteStmt:
+ case T_UpdateStmt:
+ case T_SelectStmt:
+ case T_AlterTableStmt:
+ case T_AlterTableCmd:
+ case T_AlterDomainStmt:
+ case T_GrantStmt:
+ case T_GrantRoleStmt:
+ case T_AlterDefaultPrivilegesStmt:
+ case T_ClosePortalStmt:
+ case T_ClusterStmt:
+ case T_CopyStmt:
+ case T_CreateStmt:
+ case T_DefineStmt:
+ case T_DropStmt:
+ case T_TruncateStmt:
+ case T_CommentStmt:
+ case T_FetchStmt:
+ case T_IndexStmt:
+ case T_CreateFunctionStmt:
+ case T_AlterFunctionStmt:
+ case T_DoStmt:
+ case T_RenameStmt:
+ case T_RuleStmt:
+ case T_NotifyStmt:
+ case T_ListenStmt:
+ case T_UnlistenStmt:
+ case T_TransactionStmt:
+ case T_ViewStmt:
+ case T_LoadStmt:
+ case T_CreateDomainStmt:
+ case T_CreatedbStmt:
+ case T_DropdbStmt:
+ case T_VacuumStmt:
+ case T_ExplainStmt:
+ case T_CreateTableAsStmt:
+ case T_CreateSeqStmt:
+ case T_AlterSeqStmt:
+ case T_VariableSetStmt:
+ case T_VariableShowStmt:
+ case T_DiscardStmt:
+ case T_CreateTrigStmt:
+ case T_CreatePLangStmt:
+ case T_CreateRoleStmt:
+ case T_AlterRoleStmt:
+ case T_DropRoleStmt:
+ case T_LockStmt:
+ case T_ConstraintsSetStmt:
+ case T_ReindexStmt:
+ case T_CheckPointStmt:
+ case T_CreateSchemaStmt:
+ case T_AlterDatabaseStmt:
+ case T_AlterDatabaseSetStmt:
+ case T_AlterRoleSetStmt:
+ case T_CreateConversionStmt:
+ case T_CreateCastStmt:
+ case T_CreateOpClassStmt:
+ case T_CreateOpFamilyStmt:
+ case T_AlterOpFamilyStmt:
+ case T_PrepareStmt:
+ case T_ExecuteStmt:
+ case T_DeallocateStmt:
+ case T_DeclareCursorStmt:
+ case T_CreateTableSpaceStmt:
+ case T_DropTableSpaceStmt:
+ case T_AlterObjectDependsStmt:
+ case T_AlterObjectSchemaStmt:
+ case T_AlterOwnerStmt:
+ case T_AlterOperatorStmt:
+ case T_DropOwnedStmt:
+ case T_ReassignOwnedStmt:
+ case T_CompositeTypeStmt:
+ case T_CreateEnumStmt:
+ case T_CreateRangeStmt:
+ case T_AlterEnumStmt:
+ case T_AlterTSDictionaryStmt:
+ case T_AlterTSConfigurationStmt:
+ case T_CreateFdwStmt:
+ case T_AlterFdwStmt:
+ case T_CreateForeignServerStmt:
+ case T_AlterForeignServerStmt:
+ case T_CreateUserMappingStmt:
+ case T_AlterUserMappingStmt:
+ case T_DropUserMappingStmt:
+ case T_AlterTableSpaceOptionsStmt:
+ case T_AlterTableMoveAllStmt:
+ case T_SecLabelStmt:
+ case T_CreateForeignTableStmt:
+ case T_ImportForeignSchemaStmt:
+ case T_CreateExtensionStmt:
+ case T_AlterExtensionStmt:
+ case T_AlterExtensionContentsStmt:
+ case T_CreateEventTrigStmt:
+ case T_AlterEventTrigStmt:
+ case T_RefreshMatViewStmt:
+ case T_ReplicaIdentityStmt:
+ case T_AlterSystemStmt:
+ case T_CreatePolicyStmt:
+ case T_AlterPolicyStmt:
+ case T_CreateTransformStmt:
+ case T_CreateAmStmt:
+ case T_CreatePublicationStmt:
+ case T_AlterPublicationStmt:
+ case T_CreateSubscriptionStmt:
+ case T_AlterSubscriptionStmt:
+ case T_DropSubscriptionStmt:
+ case T_CreateStatsStmt:
+ case T_AlterCollationStmt:
+ case T_CallStmt:
+ case T_AlterStatsStmt:
+ case T_A_Expr:
+ case T_ColumnRef:
+ case T_ParamRef:
+ case T_A_Const:
+ case T_FuncCall:
+ case T_A_Star:
+ case T_A_Indices:
+ case T_A_Indirection:
+ case T_A_ArrayExpr:
+ case T_ResTarget:
+ case T_MultiAssignRef:
+ case T_TypeCast:
+ case T_CollateClause:
+ case T_SortBy:
+ case T_WindowDef:
+ case T_RangeSubselect:
+ case T_RangeFunction:
+ case T_RangeTableSample:
+ case T_RangeTableFunc:
+ case T_RangeTableFuncCol:
+ case T_TypeName:
+ case T_ColumnDef:
+ case T_IndexElem:
+ case T_Constraint:
+ case T_DefElem:
+ case T_RangeTblEntry:
+ case T_WithCheckOption:
+ case T_GroupingSet:
+ case T_ObjectWithArgs:
+ case T_AccessPriv:
+ case T_CreateOpClassItem:
+ case T_TableLikeClause:
+ case T_FunctionParameter:
+ case T_LockingClause:
+ case T_RowMarkClause:
+ case T_XmlSerialize:
+ case T_WithClause:
+ case T_InferClause:
+ case T_OnConflictClause:
+ case T_CommonTableExpr:
+ case T_RoleSpec:
+ case T_TriggerTransition:
+ case T_PartitionElem:
+ case T_PartitionSpec:
+ case T_PartitionBoundSpec:
+ case T_PartitionRangeDatum:
+ case T_PartitionCmd:
+ case T_VacuumRelation:
+ case T_IdentifySystemCmd:
+ case T_BaseBackupCmd:
+ case T_CreateReplicationSlotCmd:
+ case T_DropReplicationSlotCmd:
+ case T_StartReplicationCmd:
+ case T_TimeLineHistoryCmd:
+ case T_SQLCmd:
+ case T_TriggerData:
+ case T_EventTriggerData:
+ case T_ReturnSetInfo:
+ case T_WindowObjectData:
+ case T_TIDBitmap:
+ case T_InlineCodeBlock:
+ case T_FdwRoutine:
+ case T_IndexAmRoutine:
+ case T_TableAmRoutine:
+ case T_TsmRoutine:
+ case T_ForeignKeyCacheInfo:
+ case T_CallContext:
+ case T_SupportRequestSimplify:
+ case T_SupportRequestSelectivity:
+ case T_SupportRequestCost:
+ case T_SupportRequestRows:
+ case T_SupportRequestIndexCondition:
+ /* no-op */
+ break;
- add_object_address(OCLASS_PROC, tsc->tsmhandler, 0,
- context->addrs);
- /* fall through to examine arguments */
+ /* Intentionally no default */
}
return expression_tree_walker(node, find_expr_references_walker,
Mark Dilger <hornschnorter@gmail.com> writes:
I played with this a bit, making the change I proposed, and got lots of
warnings from the compiler. I don't know how many of these would need
to be suppressed by adding a no-op for them at the end of the switch vs.
how many need to be handled, but the attached patch implements the idea.
I admit adding all these extra cases to the end is verbose....
Yeah, that's why it's not done that way ...
The change as written is much too verbose to be acceptable, but given
how many places in the code could really use this sort of treatment, I
wonder if there is a way to reorganize the NodeTag enum into multiple
enums, one for each logical subtype (such as executor nodes, plan nodes,
etc) and then have switches over enums of the given subtype, with the
compiler helping detect tags of same subtype that are unhandled in the
switch.
The problem here is that the set of nodes of interest can vary depending
on what you're doing. As a case in point, find_expr_references has to
cover both expression nodes and some things that aren't expression nodes
but can represent dependencies of a plan tree.
I think that the long-term answer, if Andres gets somewhere with his
project to autogenerate code like this, is that we'd rely on annotating
the struct declarations to tell us what to do. In the case at hand,
I could imagine annotations that say "this field contains a function OID"
or "this list contains collation OIDs" and then the find_expr_references
logic could be derived from that. Now, that's not perfect either, because
it's always possible to forget to annotate something. But it'd be a lot
easier, because there'd be tons of nearby examples of doing it right.
regards, tom lane
Hi,
On 2019-11-12 10:19:30 -0500, Tom Lane wrote:
I think that the long-term answer, if Andres gets somewhere with his
project to autogenerate code like this, is that we'd rely on annotating
the struct declarations to tell us what to do. In the case at hand,
I could imagine annotations that say "this field contains a function OID"
or "this list contains collation OIDs" and then the find_expr_references
logic could be derived from that. Now, that's not perfect either, because
it's always possible to forget to annotate something. But it'd be a lot
easier, because there'd be tons of nearby examples of doing it right.
Yea, I think that'd be going in the right direction.
I've a few annotations for other purposes in my local version of the
patch (e.g. to ignore fields for comparison), and adding further ones
for purposes like this ought to be easy.
I want to attach some annotations to types, rather than fields. I
e.g. introduced a Location typedef, annotated as being ignored for
equality purposes, instead of annotating each 'int location'. Wonder if
we should also do something like that for your hypothetical "function
OID" etc. above - seems like it also might give the human reader of code
a hint.
On 2019-11-11 16:41:41 -0500, Tom Lane wrote:
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.
Would it be a good idea to move find_expr_references_walker to
nodeFuncs.c, in hopes of making it more visible to people adding
new node types?
Can't hurt, at least. Reducing the number of files that need to be
fairly mechanically be touched when adding a node type / node type
field strikes me as a good idea.
Wonder if there's any way to write an assertion check that verifies we
have the necessary dependencies. But the only idea I have - basically
record all the syscache lookups while parse analysing an expression, and
then check that all the necessary dependencies exist - seems too
complicated to be worthwhile.
We could decouple it from the specific use-case
of recordDependencyOnExpr by having it call a callback function
for each identified OID; although maybe there's no point in that,
since I'm not sure there are any other use-cases.
I could see it being useful for a few other purposes, e.g. it seems
*marginally* possible we could share *some* code with
extract_query_dependencies(). But I think I'd only go there if we
actually convert something else to it.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2019-11-12 10:19:30 -0500, Tom Lane wrote:
I could imagine annotations that say "this field contains a function OID"
or "this list contains collation OIDs" and then the find_expr_references
logic could be derived from that.
I want to attach some annotations to types, rather than fields. I
e.g. introduced a Location typedef, annotated as being ignored for
equality purposes, instead of annotating each 'int location'. Wonder if
we should also do something like that for your hypothetical "function
OID" etc. above - seems like it also might give the human reader of code
a hint.
Hm. We could certainly do "typedef FunctionOid Oid;",
"typedef CollationOidList List;" etc, but I think it'd get pretty
tedious pretty quickly --- just for this one purpose, you'd need
a couple of typedefs for every system catalog that contains
query-referenceable OIDs. Maybe that's better than comment-style
annotations, but I'm not convinced.
Wonder if there's any way to write an assertion check that verifies we
have the necessary dependencies. But the only idea I have - basically
record all the syscache lookups while parse analysing an expression, and
then check that all the necessary dependencies exist - seems too
complicated to be worthwhile.
Yeah, it's problematic. One issue there that I'm not sure how to
resolve with autogenerated code, much less automated checking, is that
quite a few cases in find_expr_references know that we don't need to
record a dependency on an OID stored in the node because there's an
indirect dependency on something else. For example, in FuncExpr we
needn't log funcresulttype because the funcid is enough dependency,
and we needn't log either funccollid or inputcollid because those are
derived from the input expressions or the function result type.
(And giving up those optimizations would be pretty costly, 4x more
dependency checks in this example alone.)
For sure I don't want both "CollationOid" and "RedundantCollationOid"
typedefs, so it seems like annotation is the solution for this, but
I see no reasonable way to automatically verify such annotations.
Still, just writing down the annotations would be a way to expose
such assumptions for manual checking, which we don't really have now.
regards, tom lane
On 2019-11-12 15:32:14 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2019-11-12 10:19:30 -0500, Tom Lane wrote:
I could imagine annotations that say "this field contains a function OID"
or "this list contains collation OIDs" and then the find_expr_references
logic could be derived from that.I want to attach some annotations to types, rather than fields. I
e.g. introduced a Location typedef, annotated as being ignored for
equality purposes, instead of annotating each 'int location'. Wonder if
we should also do something like that for your hypothetical "function
OID" etc. above - seems like it also might give the human reader of code
a hint.Hm. We could certainly do "typedef FunctionOid Oid;",
"typedef CollationOidList List;" etc, but I think it'd get pretty
tedious pretty quickly --- just for this one purpose, you'd need
a couple of typedefs for every system catalog that contains
query-referenceable OIDs. Maybe that's better than comment-style
annotations, but I'm not convinced.
I'm not saying that we should exclusively do so, just that it's
worthwhile for a few frequent cases.
One issue there that I'm not sure how to resolve with autogenerated
code, much less automated checking, is that quite a few cases in
find_expr_references know that we don't need to record a dependency on
an OID stored in the node because there's an indirect dependency on
something else. For example, in FuncExpr we needn't log
funcresulttype because the funcid is enough dependency, and we needn't
log either funccollid or inputcollid because those are derived from
the input expressions or the function result type. (And giving up
those optimizations would be pretty costly, 4x more dependency checks
in this example alone.)
Yea, that one is hard. I suspect the best way to address that is to have
explicit code for a few cases that are worth optimizing (like
e.g. FuncExpr), and for the rest use the generic logic using. I'd so
far just written the specialized cases into the "generated metadata"
using code - but we also could have an annotation that instructs to
instead call some function, but I doubt that's worthwhile.
For sure I don't want both "CollationOid" and "RedundantCollationOid"
typedefs
Indeed.
so it seems like annotation is the solution for this
I'm not even sure annotations are going to be the easiest way to
implement some of the more complicated edge cases. Might be easier to
just open-code those, and fall back to generic logic for the rest. We'll
have to see, I think.
Greetings,
Andres Freund
On 11/11/19 1:41 PM, Tom Lane wrote:
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type NNNNN"
error because we allow dropping a type the XMLTABLE construct
references.This isn't hard to fix, as per the attached patch, but it makes
me nervous. I wonder what other dependencies we might be missing.
I can consistently generate errors like the following in master:
ERROR: cache lookup failed for statistics object 31041
This happens in a stress test in which multiple processes are making
changes to the schema. So far, all the sessions that report this cache
lookup error do so when performing one of ANALYZE, VACUUM ANALYZE,
UPDATE, DELETE or EXPLAIN ANALYZE on a table that has MCV statistics.
All processes running simultaneously are running the same set of
functions, which create and delete tables, indexes, and statistics
objects, insert, update, and delete rows in those tables, etc.
The fact that the statistics are of the MCV type might not be relevant;
I'm creating those on tables as part of testing Tomas Vondra's MCV
statistics patch, so all the tables have statistics of that kind on them.
I can try to distill my test case a bit, but first I'd like to know if
you are interested. Currently, the patch is over 2.2MB, gzip'd. I'll
only bother distilling it if you don't already know about these cache
lookup failures.
--
Mark Dilger
On Wed, Nov 13, 2019 at 03:00:03PM -0800, Mark Dilger wrote:
On 11/11/19 1:41 PM, Tom Lane wrote:
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type NNNNN"
error because we allow dropping a type the XMLTABLE construct
references.This isn't hard to fix, as per the attached patch, but it makes
me nervous. I wonder what other dependencies we might be missing.I can consistently generate errors like the following in master:
ERROR: cache lookup failed for statistics object 31041
This happens in a stress test in which multiple processes are making
changes to the schema. So far, all the sessions that report this
cache lookup error do so when performing one of ANALYZE, VACUUM
ANALYZE, UPDATE, DELETE or EXPLAIN ANALYZE on a table that has MCV
statistics. All processes running simultaneously are running the same
set of functions, which create and delete tables, indexes, and
statistics objects, insert, update, and delete rows in those tables,
etc.The fact that the statistics are of the MCV type might not be
relevant; I'm creating those on tables as part of testing Tomas
Vondra's MCV statistics patch, so all the tables have statistics of
that kind on them.
Hmmm, I don't know the details of the test, but this seems a bit like
we're trying to use the stats during estimation but it got dropped
meanwhile. If that's the case, it probably affects all stats types, not
just MCV lists - there should no significant difference between
different statistics types, I think.
I've managed to reproduce this with a stress-test, and I do get these
failures with both dependencies and mcv stats, although in slightly
different places.
And I think I see the issue - when dropping the statistics, we do
RemoveObjects which however does not acquire any lock on the table. So
we get the list of stats (without the serialized data), but before we
get to load the contents, someone drops it. If that's the root cause,
it's there since pg 10.
I'm not sure what's the right solution. An straightforward option would
be to lock the relation, but will that work after adding support for
stats on joins? An alternative would be to just ignore those failures,
but that kinda breaks the estimation (we should have picked a different
stats in this case).
I can try to distill my test case a bit, but first I'd like to know if
you are interested. Currently, the patch is over 2.2MB, gzip'd. I'll
only bother distilling it if you don't already know about these cache
lookup failures.
Not sure. But I do wonder if we see the same issue.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Mark Dilger <hornschnorter@gmail.com> writes:
On 11/11/19 1:41 PM, Tom Lane wrote:
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.
I can consistently generate errors like the following in master:
ERROR: cache lookup failed for statistics object 31041
This is surely a completely different issue --- there are not,
one hopes, any extended-stats OIDs embedded in views or other
query trees.
I concur with Tomas' suspicion that this must be a race condition
during DROP STATISTICS. If we're going to allow people to do that
separately from dropping the table(s), there has to be some kind of
locking around it, and it sounds like there's not :-(
regards, tom lane
On 11/13/19 4:46 PM, Tomas Vondra wrote:
On Wed, Nov 13, 2019 at 03:00:03PM -0800, Mark Dilger wrote:
On 11/11/19 1:41 PM, Tom Lane wrote:
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.This can be demonstrated to be a problem by the attached script,
which will end up with a "cache lookup failed for type NNNNN"
error because we allow dropping a type the XMLTABLE construct
references.This isn't hard to fix, as per the attached patch, but it makes
me nervous. I wonder what other dependencies we might be missing.I can consistently generate errors like the following in master:
ERROR: cache lookup failed for statistics object 31041
This happens in a stress test in which multiple processes are making
changes to the schema. So far, all the sessions that report this
cache lookup error do so when performing one of ANALYZE, VACUUM
ANALYZE, UPDATE, DELETE or EXPLAIN ANALYZE on a table that has MCV
statistics. All processes running simultaneously are running the same
set of functions, which create and delete tables, indexes, and
statistics objects, insert, update, and delete rows in those tables, etc.The fact that the statistics are of the MCV type might not be
relevant; I'm creating those on tables as part of testing Tomas
Vondra's MCV statistics patch, so all the tables have statistics of
that kind on them.Hmmm, I don't know the details of the test, but this seems a bit like
we're trying to use the stats during estimation but it got dropped
meanwhile. If that's the case, it probably affects all stats types, not
just MCV lists - there should no significant difference between
different statistics types, I think.I've managed to reproduce this with a stress-test, and I do get these
failures with both dependencies and mcv stats, although in slightly
different places.And I think I see the issue - when dropping the statistics, we do
RemoveObjects which however does not acquire any lock on the table. So
we get the list of stats (without the serialized data), but before we
get to load the contents, someone drops it. If that's the root cause,
it's there since pg 10.I'm not sure what's the right solution. An straightforward option would
be to lock the relation, but will that work after adding support for
stats on joins? An alternative would be to just ignore those failures,
but that kinda breaks the estimation (we should have picked a different
stats in this case).I can try to distill my test case a bit, but first I'd like to know if
you are interested. Currently, the patch is over 2.2MB, gzip'd. I'll
only bother distilling it if you don't already know about these cache
lookup failures.Not sure. But I do wonder if we see the same issue.
I don't know. If you want to reproduce what I'm seeing....
I added a parallel_schedule target:
diff --git a/src/test/regress/parallel_schedule
b/src/test/regress/parallel_schedule
index fc0f14122b..5ace7c7a8a 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -85,6 +85,8 @@ test: create_table_like alter_generic alter_operator
misc async dbsize misc_func
# collate.*.utf8 tests cannot be run in parallel with each other
test: rules psql psql_crosstab amutils stats_ext collate.linux.utf8+test: mcv_huge_stress_a mcv_huge_stress_b mcv_huge_stress_c
mcv_huge_stress_d mcv_huge_stress_e mcv_huge_stress_f mcv_huge_stress_g
+
# run by itself so it can run parallel workers
test: select_parallel
test: write_parallelAnd used the attached script to generate the contents of the seven
parallel tests. If you want to duplicate this, you'll have to manually
run gen.pl and direct its output to those src/test/regress/sql/ files.
The src/test/regress/expected/ files are just empty, as I don't care
about whether the test results match. I'm just checking what kinds of
errors I get and whether any of them are concerning.
After my most recent run of the stress tests, I grep'd for cache
failures and got 23 of them, all coming from get_relation_statistics(),
statext_store() and statext_mcv_load(). Two different adjacent spots in
get_relation_statistics() were involved:
htup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statOid));
if (!HeapTupleIsValid(htup))
elog(ERROR, "cache lookup failed for statistics object %u",
statOid);
staForm = (Form_pg_statistic_ext) GETSTRUCT(htup);
dtup = SearchSysCache1(STATEXTDATASTXOID,
ObjectIdGetDatum(statOid));
if (!HeapTupleIsValid(dtup))
elog(ERROR, "cache lookup failed for statistics object %u",
statOid);
Most were from the first SearchSysCache1 call, but one of them was from
the second.
--
Mark Dilger
Attachments:
On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:
Mark Dilger <hornschnorter@gmail.com> writes:
On 11/11/19 1:41 PM, Tom Lane wrote:
I happened to notice that find_expr_references_walker has not
been taught anything about TableFunc nodes, which means it will
miss the type and collation OIDs embedded in such a node.I can consistently generate errors like the following in master:
ERROR: cache lookup failed for statistics object 31041This is surely a completely different issue --- there are not,
one hopes, any extended-stats OIDs embedded in views or other
query trees.I concur with Tomas' suspicion that this must be a race condition
during DROP STATISTICS. If we're going to allow people to do that
separately from dropping the table(s), there has to be some kind of
locking around it, and it sounds like there's not :-(
I think the right thing to do is simply acquire AE lock on the relation
in RemoveStatisticsById, per the attached patch. It's possible we'll
need to do something more complicated once join stats are added, but
for now this should be enough (and backpatchable).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
stats-locking.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 0ab43deb71..1126e7a585 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -30,6 +30,7 @@
#include "commands/defrem.h"
#include "miscadmin.h"
#include "statistics/statistics.h"
+#include "storage/lmgr.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/inval.h"
@@ -533,6 +534,21 @@ RemoveStatisticsById(Oid statsOid)
Form_pg_statistic_ext statext;
Oid relid;
+ /* Lookup and lock relation the statistics is defined on. */
+ tup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statsOid));
+
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for statistics object %u", statsOid);
+
+ statext = (Form_pg_statistic_ext) GETSTRUCT(tup);
+ relid = statext->stxrelid;
+
+ /* lock the relation in access exclusive mode */
+ LockRelationOid(relid, AccessExclusiveLock);
+
+ ReleaseSysCache(tup);
+
+
/*
* First delete the pg_statistic_ext_data tuple holding the actual
* statistical data.
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:
I concur with Tomas' suspicion that this must be a race condition
during DROP STATISTICS. If we're going to allow people to do that
separately from dropping the table(s), there has to be some kind of
locking around it, and it sounds like there's not :-(
I think the right thing to do is simply acquire AE lock on the relation
in RemoveStatisticsById, per the attached patch. It's possible we'll
need to do something more complicated once join stats are added, but
for now this should be enough (and backpatchable).
Hm. No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock. Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.
regards, tom lane
On Thu, Nov 14, 2019 at 04:36:54PM -0500, Tom Lane wrote:
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:
I concur with Tomas' suspicion that this must be a race condition
during DROP STATISTICS. If we're going to allow people to do that
separately from dropping the table(s), there has to be some kind of
locking around it, and it sounds like there's not :-(I think the right thing to do is simply acquire AE lock on the relation
in RemoveStatisticsById, per the attached patch. It's possible we'll
need to do something more complicated once join stats are added, but
for now this should be enough (and backpatchable).Hm. No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock. Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.
Isn't that solved by RemoveObjects() doing this?
/* Get an ObjectAddress for the object. */
address = get_object_address(stmt->removeType,
object,
&relation,
AccessExclusiveLock,
stmt->missing_ok);
I've actually done some debugging before sending the patch, and I think
this prevent the issue you describe.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Nov-14, Tomas Vondra wrote:
Isn't that solved by RemoveObjects() doing this?
/* Get an ObjectAddress for the object. */
address = get_object_address(stmt->removeType,
object,
&relation,
AccessExclusiveLock,
stmt->missing_ok);I've actually done some debugging before sending the patch, and I think
this prevent the issue you describe.
Hmm .. shouldn't get_statistics_object_oid get a lock on the table that
owns the stats object too? I think it should be setting *relp to it.
That way, the lock you're proposing to add would be obtained there.
That means it'd be similar to what we do for OBJECT_TRIGGER etc,
get_object_address_relobject().
I admit this'd crash and burn if we had stats on multiple relations,
because there'd be no way to return the multiple relations that would
end up locked.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
On Thu, Nov 14, 2019 at 04:36:54PM -0500, Tom Lane wrote:
Hm. No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock. Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.
Isn't that solved by RemoveObjects() doing this?
/* Get an ObjectAddress for the object. */
address = get_object_address(stmt->removeType,
object,
&relation,
AccessExclusiveLock,
stmt->missing_ok);
Ah, I see, we already have AEL on the stats object itself. So that
eliminates my concern about a race between two RemoveStatisticsById
calls, but what we have instead is fear of deadlock. A DROP STATISTICS
command will acquire AEL on the stats object but then AEL on the table,
the opposite of what will happen during DROP TABLE, so concurrent
executions of those will deadlock. That might be better than the
failures Mark is seeing now, but not by much.
A correct fix I think is that the planner ought to acquire AccessShareLock
on a stats object it's trying to use (and then recheck whether the object
is still there). That seems rather expensive, but there may be no other
way.
regards, tom lane
On Thu, Nov 14, 2019 at 07:27:29PM -0300, Alvaro Herrera wrote:
On 2019-Nov-14, Tomas Vondra wrote:
Isn't that solved by RemoveObjects() doing this?
/* Get an ObjectAddress for the object. */
address = get_object_address(stmt->removeType,
object,
&relation,
AccessExclusiveLock,
stmt->missing_ok);I've actually done some debugging before sending the patch, and I think
this prevent the issue you describe.Hmm .. shouldn't get_statistics_object_oid get a lock on the table that
owns the stats object too? I think it should be setting *relp to it.
That way, the lock you're proposing to add would be obtained there.
That means it'd be similar to what we do for OBJECT_TRIGGER etc,
get_object_address_relobject().
Hmmm, maybe. We'd have to fake the list of names, because that function
expects the relation name to be included in the list of names, and we
don't have that for extended stats. But it might work, I guess.
I admit this'd crash and burn if we had stats on multiple relations,
because there'd be no way to return the multiple relations that would
end up locked.
I think that's less important now. If we ever get that feature, we'll
need to make that work somehow.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Nov 14, 2019 at 05:35:06PM -0500, Tom Lane wrote:
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
On Thu, Nov 14, 2019 at 04:36:54PM -0500, Tom Lane wrote:
Hm. No, it's not enough, unless you add more logic to deal with the
possibility that the stats object is gone by the time you have the
table lock. Consider e.g. two concurrent DROP STATISTICS commands,
or a statistics drop that's cascading from something like a drop
of a relevant function and so has no earlier table lock.Isn't that solved by RemoveObjects() doing this?
/* Get an ObjectAddress for the object. */
address = get_object_address(stmt->removeType,
object,
&relation,
AccessExclusiveLock,
stmt->missing_ok);Ah, I see, we already have AEL on the stats object itself. So that
eliminates my concern about a race between two RemoveStatisticsById
calls, but what we have instead is fear of deadlock. A DROP STATISTICS
command will acquire AEL on the stats object but then AEL on the table,
the opposite of what will happen during DROP TABLE, so concurrent
executions of those will deadlock. That might be better than the
failures Mark is seeing now, but not by much.
Hmmm, yeah :-(
A correct fix I think is that the planner ought to acquire AccessShareLock
on a stats object it's trying to use (and then recheck whether the object
is still there). That seems rather expensive, but there may be no other
way.
Yes, so something like for indexes, although we don't need the recheck
in that case. I think the attached patch does that (but it's 1AM here).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
stats-locking-v2.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 5e889d1861..a1e325615c 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -46,6 +46,7 @@
#include "rewrite/rewriteManip.h"
#include "statistics/statistics.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/partcache.h"
@@ -1305,9 +1306,13 @@ get_relation_statistics(RelOptInfo *rel, Relation relation)
Bitmapset *keys = NULL;
int i;
+ LockDatabaseObject(StatisticExtRelationId, statOid, 0, AccessShareLock);
+
+ /* we may get invalid tuple, if the statistic just got dropped */
htup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statOid));
if (!HeapTupleIsValid(htup))
- elog(ERROR, "cache lookup failed for statistics object %u", statOid);
+ continue;
+
staForm = (Form_pg_statistic_ext) GETSTRUCT(htup);
dtup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statOid));