clearing opfuncid vs. parallel query

Started by Robert Haasover 10 years ago31 messages

robertmhaas@gmail.com

over 10 years ago

1 attachment(s)

readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.

As previously discussed, parallel query will use outfuncs/readfuncs to
copy the Plan to be executed by a parallel worker to that worker.
That plan may contain expressions, and the round-trip through
outfuncs/readfuncs will lose their opfuncids. In this case, that's
pretty annoying, if not outright wrong. It's annoying because it
means that, after the worker reads the plan tree, it's got to iterate
over the whole thing and repeat the lookups of all the opfuncid
fields. This turns out not to be straightforward to code, because we
don't have a generic plan tree walker, and even if we did, you still
need knowledge of which plan nodes have expressions inside which
fields, and we don't have a generic facility for that either: it's
there inside e.g. set_plan_refs, but not in a form other code can use.
Moreover, if we ever did have an ALTER OPERATOR command that could
change the pg_proc mapping, this would go from annoying to outright
broken, because it would be real bad if the worker and the leader came
to different conclusions about what opfuncid to use. Maybe we could
add heavyweight locking to prevent that, but that would be expensive
and we surely don't have it today.

So I think we should abandon the approach Amit took, namely trying to
reset all of the opfuncids. Instead, I think we should provide a
method of not throwing them out in the first place. The attached
patch does by adding an "int flags" field to the relevant read
routines. stringToNode() becomes a macro which passes
STRINGTONODE_INVALIDATE_OPFUNCID to stringToNodeExtended(), which is
one of the functions that now takes an additional "int flags"
argument. If a caller doesn't wish to ignore opfuncid, they can call
stringToNodeExtended directly. This way, existing stringToNode()
callers see no behavior change, but the parallel query code can easily
get the behavior that it wants.

Thoughts? Better ideas? Objections?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

stringtonode-extended-v1.patchapplication/x-patch; name=stringtonode-extended-v1.patchDownload

diff --git a/src/backend/nodes/read.c b/src/backend/nodes/read.c
index 0dabfa7..87ece914 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -35,7 +35,7 @@ static char *pg_strtok_ptr = NULL;
  *	  returns a Node with a given legal ASCII representation
  */
 void *
-stringToNode(char *str)
+stringToNodeExtended(char *str, int flags)
 {
 	char	   *save_strtok;
 	void	   *retval;
@@ -50,7 +50,7 @@ stringToNode(char *str)
 
 	pg_strtok_ptr = str;		/* point pg_strtok at the string to read */
 
-	retval = nodeRead(NULL, 0); /* do the reading */
+	retval = nodeRead(NULL, 0, flags); /* do the reading */
 
 	pg_strtok_ptr = save_strtok;
 
@@ -275,7 +275,7 @@ nodeTokenType(char *token, int length)
  * this should only be invoked from within a stringToNode operation).
  */
 void *
-nodeRead(char *token, int tok_len)
+nodeRead(char *token, int tok_len, int flags)
 {
 	Node	   *result;
 	NodeTag		type;
@@ -293,7 +293,7 @@ nodeRead(char *token, int tok_len)
 	switch ((int) type)
 	{
 		case LEFT_BRACE:
-			result = parseNodeString();
+			result = parseNodeString(flags);
 			token = pg_strtok(&tok_len);
 			if (token == NULL || token[0] != '}')
 				elog(ERROR, "did not find '}' at end of input node");
@@ -359,7 +359,7 @@ nodeRead(char *token, int tok_len)
 						/* We have already scanned next token... */
 						if (token[0] == ')')
 							break;
-						l = lappend(l, nodeRead(token, tok_len));
+						l = lappend(l, nodeRead(token, tok_len, flags));
 						token = pg_strtok(&tok_len);
 						if (token == NULL)
 							elog(ERROR, "unterminated List structure");
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index ef88209..bf101ae 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -121,7 +121,7 @@
 #define READ_NODE_FIELD(fldname) \
 	token = pg_strtok(&length);		/* skip :fldname */ \
 	(void) token;				/* in case not used elsewhere */ \
-	local_node->fldname = nodeRead(NULL, 0)
+	local_node->fldname = nodeRead(NULL, 0, flags)
 
 /* Read a bitmapset field */
 #define READ_BITMAPSET_FIELD(fldname) \
@@ -222,7 +222,7 @@ _readBitmapset(void)
  * _readQuery
  */
 static Query *
-_readQuery(void)
+_readQuery(int flags)
 {
 	READ_LOCALS(Query);
 
@@ -266,7 +266,7 @@ _readQuery(void)
  * _readNotifyStmt
  */
 static NotifyStmt *
-_readNotifyStmt(void)
+_readNotifyStmt(int flags)
 {
 	READ_LOCALS(NotifyStmt);
 
@@ -280,7 +280,7 @@ _readNotifyStmt(void)
  * _readDeclareCursorStmt
  */
 static DeclareCursorStmt *
-_readDeclareCursorStmt(void)
+_readDeclareCursorStmt(int flags)
 {
 	READ_LOCALS(DeclareCursorStmt);
 
@@ -295,7 +295,7 @@ _readDeclareCursorStmt(void)
  * _readWithCheckOption
  */
 static WithCheckOption *
-_readWithCheckOption(void)
+_readWithCheckOption(int flags)
 {
 	READ_LOCALS(WithCheckOption);
 
@@ -312,7 +312,7 @@ _readWithCheckOption(void)
  * _readSortGroupClause
  */
 static SortGroupClause *
-_readSortGroupClause(void)
+_readSortGroupClause(int flags)
 {
 	READ_LOCALS(SortGroupClause);
 
@@ -329,7 +329,7 @@ _readSortGroupClause(void)
  * _readGroupingSet
  */
 static GroupingSet *
-_readGroupingSet(void)
+_readGroupingSet(int flags)
 {
 	READ_LOCALS(GroupingSet);
 
@@ -344,7 +344,7 @@ _readGroupingSet(void)
  * _readWindowClause
  */
 static WindowClause *
-_readWindowClause(void)
+_readWindowClause(int flags)
 {
 	READ_LOCALS(WindowClause);
 
@@ -365,7 +365,7 @@ _readWindowClause(void)
  * _readRowMarkClause
  */
 static RowMarkClause *
-_readRowMarkClause(void)
+_readRowMarkClause(int flags)
 {
 	READ_LOCALS(RowMarkClause);
 
@@ -381,7 +381,7 @@ _readRowMarkClause(void)
  * _readCommonTableExpr
  */
 static CommonTableExpr *
-_readCommonTableExpr(void)
+_readCommonTableExpr(int flags)
 {
 	READ_LOCALS(CommonTableExpr);
 
@@ -403,7 +403,7 @@ _readCommonTableExpr(void)
  * _readSetOperationStmt
  */
 static SetOperationStmt *
-_readSetOperationStmt(void)
+_readSetOperationStmt(int flags)
 {
 	READ_LOCALS(SetOperationStmt);
 
@@ -425,7 +425,7 @@ _readSetOperationStmt(void)
  */
 
 static Alias *
-_readAlias(void)
+_readAlias(int flags)
 {
 	READ_LOCALS(Alias);
 
@@ -436,7 +436,7 @@ _readAlias(void)
 }
 
 static RangeVar *
-_readRangeVar(void)
+_readRangeVar(int flags)
 {
 	READ_LOCALS(RangeVar);
 
@@ -454,7 +454,7 @@ _readRangeVar(void)
 }
 
 static IntoClause *
-_readIntoClause(void)
+_readIntoClause(int flags)
 {
 	READ_LOCALS(IntoClause);
 
@@ -473,7 +473,7 @@ _readIntoClause(void)
  * _readVar
  */
 static Var *
-_readVar(void)
+_readVar(int flags)
 {
 	READ_LOCALS(Var);
 
@@ -494,7 +494,7 @@ _readVar(void)
  * _readConst
  */
 static Const *
-_readConst(void)
+_readConst(int flags)
 {
 	READ_LOCALS(Const);
 
@@ -519,7 +519,7 @@ _readConst(void)
  * _readParam
  */
 static Param *
-_readParam(void)
+_readParam(int flags)
 {
 	READ_LOCALS(Param);
 
@@ -537,7 +537,7 @@ _readParam(void)
  * _readAggref
  */
 static Aggref *
-_readAggref(void)
+_readAggref(int flags)
 {
 	READ_LOCALS(Aggref);
 
@@ -563,7 +563,7 @@ _readAggref(void)
  * _readGroupingFunc
  */
 static GroupingFunc *
-_readGroupingFunc(void)
+_readGroupingFunc(int flags)
 {
 	READ_LOCALS(GroupingFunc);
 
@@ -580,7 +580,7 @@ _readGroupingFunc(void)
  * _readWindowFunc
  */
 static WindowFunc *
-_readWindowFunc(void)
+_readWindowFunc(int flags)
 {
 	READ_LOCALS(WindowFunc);
 
@@ -602,7 +602,7 @@ _readWindowFunc(void)
  * _readArrayRef
  */
 static ArrayRef *
-_readArrayRef(void)
+_readArrayRef(int flags)
 {
 	READ_LOCALS(ArrayRef);
 
@@ -622,7 +622,7 @@ _readArrayRef(void)
  * _readFuncExpr
  */
 static FuncExpr *
-_readFuncExpr(void)
+_readFuncExpr(int flags)
 {
 	READ_LOCALS(FuncExpr);
 
@@ -643,7 +643,7 @@ _readFuncExpr(void)
  * _readNamedArgExpr
  */
 static NamedArgExpr *
-_readNamedArgExpr(void)
+_readNamedArgExpr(int flags)
 {
 	READ_LOCALS(NamedArgExpr);
 
@@ -659,22 +659,16 @@ _readNamedArgExpr(void)
  * _readOpExpr
  */
 static OpExpr *
-_readOpExpr(void)
+_readOpExpr(int flags)
 {
 	READ_LOCALS(OpExpr);
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
 
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
+	/* Most callers want to zero opfuncid; see comments in nodes.h */
+	if (flags & STRINGTONODE_INVALIDATE_OPFUNCID)
+		local_node->opfuncid = InvalidOid;
 
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
@@ -690,22 +684,16 @@ _readOpExpr(void)
  * _readDistinctExpr
  */
 static DistinctExpr *
-_readDistinctExpr(void)
+_readDistinctExpr(int flags)
 {
 	READ_LOCALS(DistinctExpr);
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
 
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
+	/* Most callers want to zero opfuncid; see comments in nodes.h */
+	if (flags & STRINGTONODE_INVALIDATE_OPFUNCID)
+		local_node->opfuncid = InvalidOid;
 
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
@@ -721,22 +709,16 @@ _readDistinctExpr(void)
  * _readNullIfExpr
  */
 static NullIfExpr *
-_readNullIfExpr(void)
+_readNullIfExpr(int flags)
 {
 	READ_LOCALS(NullIfExpr);
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
 
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
+	/* Most callers want to zero opfuncid; see comments in nodes.h */
+	if (flags & STRINGTONODE_INVALIDATE_OPFUNCID)
+		local_node->opfuncid = InvalidOid;
 
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
@@ -752,22 +734,16 @@ _readNullIfExpr(void)
  * _readScalarArrayOpExpr
  */
 static ScalarArrayOpExpr *
-_readScalarArrayOpExpr(void)
+_readScalarArrayOpExpr(int flags)
 {
 	READ_LOCALS(ScalarArrayOpExpr);
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
 
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
+	/* Most callers want to zero opfuncid; see comments in nodes.h */
+	if (flags & STRINGTONODE_INVALIDATE_OPFUNCID)
+		local_node->opfuncid = InvalidOid;
 
 	READ_BOOL_FIELD(useOr);
 	READ_OID_FIELD(inputcollid);
@@ -781,7 +757,7 @@ _readScalarArrayOpExpr(void)
  * _readBoolExpr
  */
 static BoolExpr *
-_readBoolExpr(void)
+_readBoolExpr(int flags)
 {
 	READ_LOCALS(BoolExpr);
 
@@ -807,7 +783,7 @@ _readBoolExpr(void)
  * _readSubLink
  */
 static SubLink *
-_readSubLink(void)
+_readSubLink(int flags)
 {
 	READ_LOCALS(SubLink);
 
@@ -829,7 +805,7 @@ _readSubLink(void)
  * _readFieldSelect
  */
 static FieldSelect *
-_readFieldSelect(void)
+_readFieldSelect(int flags)
 {
 	READ_LOCALS(FieldSelect);
 
@@ -846,7 +822,7 @@ _readFieldSelect(void)
  * _readFieldStore
  */
 static FieldStore *
-_readFieldStore(void)
+_readFieldStore(int flags)
 {
 	READ_LOCALS(FieldStore);
 
@@ -862,7 +838,7 @@ _readFieldStore(void)
  * _readRelabelType
  */
 static RelabelType *
-_readRelabelType(void)
+_readRelabelType(int flags)
 {
 	READ_LOCALS(RelabelType);
 
@@ -880,7 +856,7 @@ _readRelabelType(void)
  * _readCoerceViaIO
  */
 static CoerceViaIO *
-_readCoerceViaIO(void)
+_readCoerceViaIO(int flags)
 {
 	READ_LOCALS(CoerceViaIO);
 
@@ -897,7 +873,7 @@ _readCoerceViaIO(void)
  * _readArrayCoerceExpr
  */
 static ArrayCoerceExpr *
-_readArrayCoerceExpr(void)
+_readArrayCoerceExpr(int flags)
 {
 	READ_LOCALS(ArrayCoerceExpr);
 
@@ -917,7 +893,7 @@ _readArrayCoerceExpr(void)
  * _readConvertRowtypeExpr
  */
 static ConvertRowtypeExpr *
-_readConvertRowtypeExpr(void)
+_readConvertRowtypeExpr(int flags)
 {
 	READ_LOCALS(ConvertRowtypeExpr);
 
@@ -933,7 +909,7 @@ _readConvertRowtypeExpr(void)
  * _readCollateExpr
  */
 static CollateExpr *
-_readCollateExpr(void)
+_readCollateExpr(int flags)
 {
 	READ_LOCALS(CollateExpr);
 
@@ -948,7 +924,7 @@ _readCollateExpr(void)
  * _readCaseExpr
  */
 static CaseExpr *
-_readCaseExpr(void)
+_readCaseExpr(int flags)
 {
 	READ_LOCALS(CaseExpr);
 
@@ -966,7 +942,7 @@ _readCaseExpr(void)
  * _readCaseWhen
  */
 static CaseWhen *
-_readCaseWhen(void)
+_readCaseWhen(int flags)
 {
 	READ_LOCALS(CaseWhen);
 
@@ -981,7 +957,7 @@ _readCaseWhen(void)
  * _readCaseTestExpr
  */
 static CaseTestExpr *
-_readCaseTestExpr(void)
+_readCaseTestExpr(int flags)
 {
 	READ_LOCALS(CaseTestExpr);
 
@@ -996,7 +972,7 @@ _readCaseTestExpr(void)
  * _readArrayExpr
  */
 static ArrayExpr *
-_readArrayExpr(void)
+_readArrayExpr(int flags)
 {
 	READ_LOCALS(ArrayExpr);
 
@@ -1014,7 +990,7 @@ _readArrayExpr(void)
  * _readRowExpr
  */
 static RowExpr *
-_readRowExpr(void)
+_readRowExpr(int flags)
 {
 	READ_LOCALS(RowExpr);
 
@@ -1031,7 +1007,7 @@ _readRowExpr(void)
  * _readRowCompareExpr
  */
 static RowCompareExpr *
-_readRowCompareExpr(void)
+_readRowCompareExpr(int flags)
 {
 	READ_LOCALS(RowCompareExpr);
 
@@ -1049,7 +1025,7 @@ _readRowCompareExpr(void)
  * _readCoalesceExpr
  */
 static CoalesceExpr *
-_readCoalesceExpr(void)
+_readCoalesceExpr(int flags)
 {
 	READ_LOCALS(CoalesceExpr);
 
@@ -1065,7 +1041,7 @@ _readCoalesceExpr(void)
  * _readMinMaxExpr
  */
 static MinMaxExpr *
-_readMinMaxExpr(void)
+_readMinMaxExpr(int flags)
 {
 	READ_LOCALS(MinMaxExpr);
 
@@ -1083,7 +1059,7 @@ _readMinMaxExpr(void)
  * _readXmlExpr
  */
 static XmlExpr *
-_readXmlExpr(void)
+_readXmlExpr(int flags)
 {
 	READ_LOCALS(XmlExpr);
 
@@ -1104,7 +1080,7 @@ _readXmlExpr(void)
  * _readNullTest
  */
 static NullTest *
-_readNullTest(void)
+_readNullTest(int flags)
 {
 	READ_LOCALS(NullTest);
 
@@ -1120,7 +1096,7 @@ _readNullTest(void)
  * _readBooleanTest
  */
 static BooleanTest *
-_readBooleanTest(void)
+_readBooleanTest(int flags)
 {
 	READ_LOCALS(BooleanTest);
 
@@ -1135,7 +1111,7 @@ _readBooleanTest(void)
  * _readCoerceToDomain
  */
 static CoerceToDomain *
-_readCoerceToDomain(void)
+_readCoerceToDomain(int flags)
 {
 	READ_LOCALS(CoerceToDomain);
 
@@ -1153,7 +1129,7 @@ _readCoerceToDomain(void)
  * _readCoerceToDomainValue
  */
 static CoerceToDomainValue *
-_readCoerceToDomainValue(void)
+_readCoerceToDomainValue(int flags)
 {
 	READ_LOCALS(CoerceToDomainValue);
 
@@ -1169,7 +1145,7 @@ _readCoerceToDomainValue(void)
  * _readSetToDefault
  */
 static SetToDefault *
-_readSetToDefault(void)
+_readSetToDefault(int flags)
 {
 	READ_LOCALS(SetToDefault);
 
@@ -1185,7 +1161,7 @@ _readSetToDefault(void)
  * _readCurrentOfExpr
  */
 static CurrentOfExpr *
-_readCurrentOfExpr(void)
+_readCurrentOfExpr(int flags)
 {
 	READ_LOCALS(CurrentOfExpr);
 
@@ -1200,7 +1176,7 @@ _readCurrentOfExpr(void)
  * _readInferenceElem
  */
 static InferenceElem *
-_readInferenceElem(void)
+_readInferenceElem(int flags)
 {
 	READ_LOCALS(InferenceElem);
 
@@ -1215,7 +1191,7 @@ _readInferenceElem(void)
  * _readTargetEntry
  */
 static TargetEntry *
-_readTargetEntry(void)
+_readTargetEntry(int flags)
 {
 	READ_LOCALS(TargetEntry);
 
@@ -1234,7 +1210,7 @@ _readTargetEntry(void)
  * _readRangeTblRef
  */
 static RangeTblRef *
-_readRangeTblRef(void)
+_readRangeTblRef(int flags)
 {
 	READ_LOCALS(RangeTblRef);
 
@@ -1247,7 +1223,7 @@ _readRangeTblRef(void)
  * _readJoinExpr
  */
 static JoinExpr *
-_readJoinExpr(void)
+_readJoinExpr(int flags)
 {
 	READ_LOCALS(JoinExpr);
 
@@ -1267,7 +1243,7 @@ _readJoinExpr(void)
  * _readFromExpr
  */
 static FromExpr *
-_readFromExpr(void)
+_readFromExpr(int flags)
 {
 	READ_LOCALS(FromExpr);
 
@@ -1281,7 +1257,7 @@ _readFromExpr(void)
  * _readOnConflictExpr
  */
 static OnConflictExpr *
-_readOnConflictExpr(void)
+_readOnConflictExpr(int flags)
 {
 	READ_LOCALS(OnConflictExpr);
 
@@ -1305,7 +1281,7 @@ _readOnConflictExpr(void)
  * _readRangeTblEntry
  */
 static RangeTblEntry *
-_readRangeTblEntry(void)
+_readRangeTblEntry(int flags)
 {
 	READ_LOCALS(RangeTblEntry);
 
@@ -1368,7 +1344,7 @@ _readRangeTblEntry(void)
  * _readRangeTblFunction
  */
 static RangeTblFunction *
-_readRangeTblFunction(void)
+_readRangeTblFunction(int flags)
 {
 	READ_LOCALS(RangeTblFunction);
 
@@ -1387,7 +1363,7 @@ _readRangeTblFunction(void)
  * _readTableSampleClause
  */
 static TableSampleClause *
-_readTableSampleClause(void)
+_readTableSampleClause(int flags)
 {
 	READ_LOCALS(TableSampleClause);
 
@@ -1402,7 +1378,7 @@ _readTableSampleClause(void)
  * _readDefElem
  */
 static DefElem *
-_readDefElem(void)
+_readDefElem(int flags)
 {
 	READ_LOCALS(DefElem);
 
@@ -1418,7 +1394,7 @@ _readDefElem(void)
  * _readPlannedStmt
  */
 static PlannedStmt *
-_readPlannedStmt(void)
+_readPlannedStmt(int flags)
 {
 	READ_LOCALS(PlannedStmt);
 
@@ -1449,7 +1425,7 @@ _readPlannedStmt(void)
  *	Assign the basic stuff of all nodes that inherit from Plan
  */
 static void
-ReadCommonPlan(Plan *local_node)
+ReadCommonPlan(Plan *local_node, int flags)
 {
 	READ_TEMP_LOCALS();
 
@@ -1470,11 +1446,11 @@ ReadCommonPlan(Plan *local_node)
  * _readPlan
  */
 static Plan *
-_readPlan(void)
+_readPlan(int flags)
 {
 	READ_LOCALS_NO_FIELDS(Plan);
 
-	ReadCommonPlan(local_node);
+	ReadCommonPlan(local_node, flags);
 
 	READ_DONE();
 }
@@ -1483,11 +1459,11 @@ _readPlan(void)
  * _readResult
  */
 static Result *
-_readResult(void)
+_readResult(int flags)
 {
 	READ_LOCALS(Result);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(resconstantqual);
 
@@ -1498,11 +1474,11 @@ _readResult(void)
  * _readModifyTable
  */
 static ModifyTable *
-_readModifyTable(void)
+_readModifyTable(int flags)
 {
 	READ_LOCALS(ModifyTable);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_ENUM_FIELD(operation, CmdType);
 	READ_BOOL_FIELD(canSetTag);
@@ -1529,11 +1505,11 @@ _readModifyTable(void)
  * _readAppend
  */
 static Append *
-_readAppend(void)
+_readAppend(int flags)
 {
 	READ_LOCALS(Append);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(appendplans);
 
@@ -1544,11 +1520,11 @@ _readAppend(void)
  * _readMergeAppend
  */
 static MergeAppend *
-_readMergeAppend(void)
+_readMergeAppend(int flags)
 {
 	READ_LOCALS(MergeAppend);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(mergeplans);
 	READ_INT_FIELD(numCols);
@@ -1564,11 +1540,11 @@ _readMergeAppend(void)
  * _readRecursiveUnion
  */
 static RecursiveUnion *
-_readRecursiveUnion(void)
+_readRecursiveUnion(int flags)
 {
 	READ_LOCALS(RecursiveUnion);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_INT_FIELD(wtParam);
 	READ_INT_FIELD(numCols);
@@ -1583,11 +1559,11 @@ _readRecursiveUnion(void)
  * _readBitmapAnd
  */
 static BitmapAnd *
-_readBitmapAnd(void)
+_readBitmapAnd(int flags)
 {
 	READ_LOCALS(BitmapAnd);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(bitmapplans);
 
@@ -1598,11 +1574,11 @@ _readBitmapAnd(void)
  * _readBitmapOr
  */
 static BitmapOr *
-_readBitmapOr(void)
+_readBitmapOr(int flags)
 {
 	READ_LOCALS(BitmapOr);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(bitmapplans);
 
@@ -1614,11 +1590,11 @@ _readBitmapOr(void)
  *	Assign the basic stuff of all nodes that inherit from Scan
  */
 static void
-ReadCommonScan(Scan *local_node)
+ReadCommonScan(Scan *local_node, int flags)
 {
 	READ_TEMP_LOCALS();
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_UINT_FIELD(scanrelid);
 }
@@ -1627,11 +1603,11 @@ ReadCommonScan(Scan *local_node)
  * _readScan
  */
 static Scan *
-_readScan(void)
+_readScan(int flags)
 {
 	READ_LOCALS_NO_FIELDS(Scan);
 
-	ReadCommonScan(local_node);
+	ReadCommonScan(local_node, flags);
 
 	READ_DONE();
 }
@@ -1640,11 +1616,11 @@ _readScan(void)
  * _readSeqScan
  */
 static SeqScan *
-_readSeqScan(void)
+_readSeqScan(int flags)
 {
 	READ_LOCALS_NO_FIELDS(SeqScan);
 
-	ReadCommonScan(local_node);
+	ReadCommonScan(local_node, flags);
 
 	READ_DONE();
 }
@@ -1653,11 +1629,11 @@ _readSeqScan(void)
  * _readSampleScan
  */
 static SampleScan *
-_readSampleScan(void)
+_readSampleScan(int flags)
 {
 	READ_LOCALS(SampleScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_NODE_FIELD(tablesample);
 
@@ -1668,11 +1644,11 @@ _readSampleScan(void)
  * _readIndexScan
  */
 static IndexScan *
-_readIndexScan(void)
+_readIndexScan(int flags)
 {
 	READ_LOCALS(IndexScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_OID_FIELD(indexid);
 	READ_NODE_FIELD(indexqual);
@@ -1689,11 +1665,11 @@ _readIndexScan(void)
  * _readIndexOnlyScan
  */
 static IndexOnlyScan *
-_readIndexOnlyScan(void)
+_readIndexOnlyScan(int flags)
 {
 	READ_LOCALS(IndexOnlyScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_OID_FIELD(indexid);
 	READ_NODE_FIELD(indexqual);
@@ -1708,11 +1684,11 @@ _readIndexOnlyScan(void)
  * _readBitmapIndexScan
  */
 static BitmapIndexScan *
-_readBitmapIndexScan(void)
+_readBitmapIndexScan(int flags)
 {
 	READ_LOCALS(BitmapIndexScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_OID_FIELD(indexid);
 	READ_NODE_FIELD(indexqual);
@@ -1725,11 +1701,11 @@ _readBitmapIndexScan(void)
  * _readBitmapHeapScan
  */
 static BitmapHeapScan *
-_readBitmapHeapScan(void)
+_readBitmapHeapScan(int flags)
 {
 	READ_LOCALS(BitmapHeapScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_NODE_FIELD(bitmapqualorig);
 
@@ -1740,11 +1716,11 @@ _readBitmapHeapScan(void)
  * _readTidScan
  */
 static TidScan *
-_readTidScan(void)
+_readTidScan(int flags)
 {
 	READ_LOCALS(TidScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_NODE_FIELD(tidquals);
 
@@ -1755,11 +1731,11 @@ _readTidScan(void)
  * _readSubqueryScan
  */
 static SubqueryScan *
-_readSubqueryScan(void)
+_readSubqueryScan(int flags)
 {
 	READ_LOCALS(SubqueryScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_NODE_FIELD(subplan);
 
@@ -1770,11 +1746,11 @@ _readSubqueryScan(void)
  * _readFunctionScan
  */
 static FunctionScan *
-_readFunctionScan(void)
+_readFunctionScan(int flags)
 {
 	READ_LOCALS(FunctionScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_NODE_FIELD(functions);
 	READ_BOOL_FIELD(funcordinality);
@@ -1786,11 +1762,11 @@ _readFunctionScan(void)
  * _readValuesScan
  */
 static ValuesScan *
-_readValuesScan(void)
+_readValuesScan(int flags)
 {
 	READ_LOCALS(ValuesScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_NODE_FIELD(values_lists);
 
@@ -1801,11 +1777,11 @@ _readValuesScan(void)
  * _readCteScan
  */
 static CteScan *
-_readCteScan(void)
+_readCteScan(int flags)
 {
 	READ_LOCALS(CteScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_INT_FIELD(ctePlanId);
 	READ_INT_FIELD(cteParam);
@@ -1817,11 +1793,11 @@ _readCteScan(void)
  * _readWorkTableScan
  */
 static WorkTableScan *
-_readWorkTableScan(void)
+_readWorkTableScan(int flags)
 {
 	READ_LOCALS(WorkTableScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_INT_FIELD(wtParam);
 
@@ -1832,11 +1808,11 @@ _readWorkTableScan(void)
  * _readForeignScan
  */
 static ForeignScan *
-_readForeignScan(void)
+_readForeignScan(int flags)
 {
 	READ_LOCALS(ForeignScan);
 
-	ReadCommonScan(&local_node->scan);
+	ReadCommonScan(&local_node->scan, flags);
 
 	READ_OID_FIELD(fs_server);
 	READ_NODE_FIELD(fdw_exprs);
@@ -1853,11 +1829,11 @@ _readForeignScan(void)
  *	Assign the basic stuff of all nodes that inherit from Join
  */
 static void
-ReadCommonJoin(Join *local_node)
+ReadCommonJoin(Join *local_node, int flags)
 {
 	READ_TEMP_LOCALS();
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_ENUM_FIELD(jointype, JoinType);
 	READ_NODE_FIELD(joinqual);
@@ -1867,11 +1843,11 @@ ReadCommonJoin(Join *local_node)
  * _readJoin
  */
 static Join *
-_readJoin(void)
+_readJoin(int flags)
 {
 	READ_LOCALS_NO_FIELDS(Join);
 
-	ReadCommonJoin(local_node);
+	ReadCommonJoin(local_node, flags);
 
 	READ_DONE();
 }
@@ -1880,11 +1856,11 @@ _readJoin(void)
  * _readNestLoop
  */
 static NestLoop *
-_readNestLoop(void)
+_readNestLoop(int flags)
 {
 	READ_LOCALS(NestLoop);
 
-	ReadCommonJoin(&local_node->join);
+	ReadCommonJoin(&local_node->join, flags);
 
 	READ_NODE_FIELD(nestParams);
 
@@ -1895,13 +1871,13 @@ _readNestLoop(void)
  * _readMergeJoin
  */
 static MergeJoin *
-_readMergeJoin(void)
+_readMergeJoin(int flags)
 {
 	int			numCols;
 
 	READ_LOCALS(MergeJoin);
 
-	ReadCommonJoin(&local_node->join);
+	ReadCommonJoin(&local_node->join, flags);
 
 	READ_NODE_FIELD(mergeclauses);
 
@@ -1919,11 +1895,11 @@ _readMergeJoin(void)
  * _readHashJoin
  */
 static HashJoin *
-_readHashJoin(void)
+_readHashJoin(int flags)
 {
 	READ_LOCALS(HashJoin);
 
-	ReadCommonJoin(&local_node->join);
+	ReadCommonJoin(&local_node->join, flags);
 
 	READ_NODE_FIELD(hashclauses);
 
@@ -1934,11 +1910,11 @@ _readHashJoin(void)
  * _readMaterial
  */
 static Material *
-_readMaterial(void)
+_readMaterial(int flags)
 {
 	READ_LOCALS_NO_FIELDS(Material);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_DONE();
 }
@@ -1947,11 +1923,11 @@ _readMaterial(void)
  * _readSort
  */
 static Sort *
-_readSort(void)
+_readSort(int flags)
 {
 	READ_LOCALS(Sort);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_INT_FIELD(numCols);
 	READ_ATTRNUMBER_ARRAY(sortColIdx, local_node->numCols);
@@ -1966,11 +1942,11 @@ _readSort(void)
  * _readGroup
  */
 static Group *
-_readGroup(void)
+_readGroup(int flags)
 {
 	READ_LOCALS(Group);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_INT_FIELD(numCols);
 	READ_ATTRNUMBER_ARRAY(grpColIdx, local_node->numCols);
@@ -1983,11 +1959,11 @@ _readGroup(void)
  * _readAgg
  */
 static Agg *
-_readAgg(void)
+_readAgg(int flags)
 {
 	READ_LOCALS(Agg);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_ENUM_FIELD(aggstrategy, AggStrategy);
 	READ_INT_FIELD(numCols);
@@ -2004,11 +1980,11 @@ _readAgg(void)
  * _readWindowAgg
  */
 static WindowAgg *
-_readWindowAgg(void)
+_readWindowAgg(int flags)
 {
 	READ_LOCALS(WindowAgg);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_UINT_FIELD(winref);
 	READ_INT_FIELD(partNumCols);
@@ -2028,11 +2004,11 @@ _readWindowAgg(void)
  * _readUnique
  */
 static Unique *
-_readUnique(void)
+_readUnique(int flags)
 {
 	READ_LOCALS(Unique);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_INT_FIELD(numCols);
 	READ_ATTRNUMBER_ARRAY(uniqColIdx, local_node->numCols);
@@ -2045,11 +2021,11 @@ _readUnique(void)
  * _readHash
  */
 static Hash *
-_readHash(void)
+_readHash(int flags)
 {
 	READ_LOCALS(Hash);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_OID_FIELD(skewTable);
 	READ_INT_FIELD(skewColumn);
@@ -2064,11 +2040,11 @@ _readHash(void)
  * _readSetOp
  */
 static SetOp *
-_readSetOp(void)
+_readSetOp(int flags)
 {
 	READ_LOCALS(SetOp);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_ENUM_FIELD(cmd, SetOpCmd);
 	READ_ENUM_FIELD(strategy, SetOpStrategy);
@@ -2086,11 +2062,11 @@ _readSetOp(void)
  * _readLockRows
  */
 static LockRows *
-_readLockRows(void)
+_readLockRows(int flags)
 {
 	READ_LOCALS(LockRows);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(rowMarks);
 	READ_INT_FIELD(epqParam);
@@ -2102,11 +2078,11 @@ _readLockRows(void)
  * _readLimit
  */
 static Limit *
-_readLimit(void)
+_readLimit(int flags)
 {
 	READ_LOCALS(Limit);
 
-	ReadCommonPlan(&local_node->plan);
+	ReadCommonPlan(&local_node->plan, flags);
 
 	READ_NODE_FIELD(limitOffset);
 	READ_NODE_FIELD(limitCount);
@@ -2118,7 +2094,7 @@ _readLimit(void)
  * _readNestLoopParam
  */
 static NestLoopParam *
-_readNestLoopParam(void)
+_readNestLoopParam(int flags)
 {
 	READ_LOCALS(NestLoopParam);
 
@@ -2132,7 +2108,7 @@ _readNestLoopParam(void)
  * _readPlanRowMark
  */
 static PlanRowMark *
-_readPlanRowMark(void)
+_readPlanRowMark(int flags)
 {
 	READ_LOCALS(PlanRowMark);
 
@@ -2152,7 +2128,7 @@ _readPlanRowMark(void)
  * _readPlanInvalItem
  */
 static PlanInvalItem *
-_readPlanInvalItem(void)
+_readPlanInvalItem(int flags)
 {
 	READ_LOCALS(PlanInvalItem);
 
@@ -2166,7 +2142,7 @@ _readPlanInvalItem(void)
  * _readSubPlan
  */
 static SubPlan *
-_readSubPlan(void)
+_readSubPlan(int flags)
 {
 	READ_LOCALS(SubPlan);
 
@@ -2193,7 +2169,7 @@ _readSubPlan(void)
  * _readAlternativeSubPlan
  */
 static AlternativeSubPlan *
-_readAlternativeSubPlan(void)
+_readAlternativeSubPlan(int flags)
 {
 	READ_LOCALS(AlternativeSubPlan);
 
@@ -2211,7 +2187,7 @@ _readAlternativeSubPlan(void)
  * The string to be read must already have been loaded into pg_strtok().
  */
 Node *
-parseNodeString(void)
+parseNodeString(int flags)
 {
 	void	   *return_value;
 
@@ -2223,209 +2199,209 @@ parseNodeString(void)
 	(length == namelen && memcmp(token, tokname, namelen) == 0)
 
 	if (MATCH("QUERY", 5))
-		return_value = _readQuery();
+		return_value = _readQuery(flags);
 	else if (MATCH("WITHCHECKOPTION", 15))
-		return_value = _readWithCheckOption();
+		return_value = _readWithCheckOption(flags);
 	else if (MATCH("SORTGROUPCLAUSE", 15))
-		return_value = _readSortGroupClause();
+		return_value = _readSortGroupClause(flags);
 	else if (MATCH("GROUPINGSET", 11))
-		return_value = _readGroupingSet();
+		return_value = _readGroupingSet(flags);
 	else if (MATCH("WINDOWCLAUSE", 12))
-		return_value = _readWindowClause();
+		return_value = _readWindowClause(flags);
 	else if (MATCH("ROWMARKCLAUSE", 13))
-		return_value = _readRowMarkClause();
+		return_value = _readRowMarkClause(flags);
 	else if (MATCH("COMMONTABLEEXPR", 15))
-		return_value = _readCommonTableExpr();
+		return_value = _readCommonTableExpr(flags);
 	else if (MATCH("SETOPERATIONSTMT", 16))
-		return_value = _readSetOperationStmt();
+		return_value = _readSetOperationStmt(flags);
 	else if (MATCH("ALIAS", 5))
-		return_value = _readAlias();
+		return_value = _readAlias(flags);
 	else if (MATCH("RANGEVAR", 8))
-		return_value = _readRangeVar();
+		return_value = _readRangeVar(flags);
 	else if (MATCH("INTOCLAUSE", 10))
-		return_value = _readIntoClause();
+		return_value = _readIntoClause(flags);
 	else if (MATCH("VAR", 3))
-		return_value = _readVar();
+		return_value = _readVar(flags);
 	else if (MATCH("CONST", 5))
-		return_value = _readConst();
+		return_value = _readConst(flags);
 	else if (MATCH("PARAM", 5))
-		return_value = _readParam();
+		return_value = _readParam(flags);
 	else if (MATCH("AGGREF", 6))
-		return_value = _readAggref();
+		return_value = _readAggref(flags);
 	else if (MATCH("GROUPINGFUNC", 12))
-		return_value = _readGroupingFunc();
+		return_value = _readGroupingFunc(flags);
 	else if (MATCH("WINDOWFUNC", 10))
-		return_value = _readWindowFunc();
+		return_value = _readWindowFunc(flags);
 	else if (MATCH("ARRAYREF", 8))
-		return_value = _readArrayRef();
+		return_value = _readArrayRef(flags);
 	else if (MATCH("FUNCEXPR", 8))
-		return_value = _readFuncExpr();
+		return_value = _readFuncExpr(flags);
 	else if (MATCH("NAMEDARGEXPR", 12))
-		return_value = _readNamedArgExpr();
+		return_value = _readNamedArgExpr(flags);
 	else if (MATCH("OPEXPR", 6))
-		return_value = _readOpExpr();
+		return_value = _readOpExpr(flags);
 	else if (MATCH("DISTINCTEXPR", 12))
-		return_value = _readDistinctExpr();
+		return_value = _readDistinctExpr(flags);
 	else if (MATCH("NULLIFEXPR", 10))
-		return_value = _readNullIfExpr();
+		return_value = _readNullIfExpr(flags);
 	else if (MATCH("SCALARARRAYOPEXPR", 17))
-		return_value = _readScalarArrayOpExpr();
+		return_value = _readScalarArrayOpExpr(flags);
 	else if (MATCH("BOOLEXPR", 8))
-		return_value = _readBoolExpr();
+		return_value = _readBoolExpr(flags);
 	else if (MATCH("SUBLINK", 7))
-		return_value = _readSubLink();
+		return_value = _readSubLink(flags);
 	else if (MATCH("FIELDSELECT", 11))
-		return_value = _readFieldSelect();
+		return_value = _readFieldSelect(flags);
 	else if (MATCH("FIELDSTORE", 10))
-		return_value = _readFieldStore();
+		return_value = _readFieldStore(flags);
 	else if (MATCH("RELABELTYPE", 11))
-		return_value = _readRelabelType();
+		return_value = _readRelabelType(flags);
 	else if (MATCH("COERCEVIAIO", 11))
-		return_value = _readCoerceViaIO();
+		return_value = _readCoerceViaIO(flags);
 	else if (MATCH("ARRAYCOERCEEXPR", 15))
-		return_value = _readArrayCoerceExpr();
+		return_value = _readArrayCoerceExpr(flags);
 	else if (MATCH("CONVERTROWTYPEEXPR", 18))
-		return_value = _readConvertRowtypeExpr();
+		return_value = _readConvertRowtypeExpr(flags);
 	else if (MATCH("COLLATE", 7))
-		return_value = _readCollateExpr();
+		return_value = _readCollateExpr(flags);
 	else if (MATCH("CASE", 4))
-		return_value = _readCaseExpr();
+		return_value = _readCaseExpr(flags);
 	else if (MATCH("WHEN", 4))
-		return_value = _readCaseWhen();
+		return_value = _readCaseWhen(flags);
 	else if (MATCH("CASETESTEXPR", 12))
-		return_value = _readCaseTestExpr();
+		return_value = _readCaseTestExpr(flags);
 	else if (MATCH("ARRAY", 5))
-		return_value = _readArrayExpr();
+		return_value = _readArrayExpr(flags);
 	else if (MATCH("ROW", 3))
-		return_value = _readRowExpr();
+		return_value = _readRowExpr(flags);
 	else if (MATCH("ROWCOMPARE", 10))
-		return_value = _readRowCompareExpr();
+		return_value = _readRowCompareExpr(flags);
 	else if (MATCH("COALESCE", 8))
-		return_value = _readCoalesceExpr();
+		return_value = _readCoalesceExpr(flags);
 	else if (MATCH("MINMAX", 6))
-		return_value = _readMinMaxExpr();
+		return_value = _readMinMaxExpr(flags);
 	else if (MATCH("XMLEXPR", 7))
-		return_value = _readXmlExpr();
+		return_value = _readXmlExpr(flags);
 	else if (MATCH("NULLTEST", 8))
-		return_value = _readNullTest();
+		return_value = _readNullTest(flags);
 	else if (MATCH("BOOLEANTEST", 11))
-		return_value = _readBooleanTest();
+		return_value = _readBooleanTest(flags);
 	else if (MATCH("COERCETODOMAIN", 14))
-		return_value = _readCoerceToDomain();
+		return_value = _readCoerceToDomain(flags);
 	else if (MATCH("COERCETODOMAINVALUE", 19))
-		return_value = _readCoerceToDomainValue();
+		return_value = _readCoerceToDomainValue(flags);
 	else if (MATCH("SETTODEFAULT", 12))
-		return_value = _readSetToDefault();
+		return_value = _readSetToDefault(flags);
 	else if (MATCH("CURRENTOFEXPR", 13))
-		return_value = _readCurrentOfExpr();
+		return_value = _readCurrentOfExpr(flags);
 	else if (MATCH("INFERENCEELEM", 13))
-		return_value = _readInferenceElem();
+		return_value = _readInferenceElem(flags);
 	else if (MATCH("TARGETENTRY", 11))
-		return_value = _readTargetEntry();
+		return_value = _readTargetEntry(flags);
 	else if (MATCH("RANGETBLREF", 11))
-		return_value = _readRangeTblRef();
+		return_value = _readRangeTblRef(flags);
 	else if (MATCH("JOINEXPR", 8))
-		return_value = _readJoinExpr();
+		return_value = _readJoinExpr(flags);
 	else if (MATCH("FROMEXPR", 8))
-		return_value = _readFromExpr();
+		return_value = _readFromExpr(flags);
 	else if (MATCH("ONCONFLICTEXPR", 14))
-		return_value = _readOnConflictExpr();
+		return_value = _readOnConflictExpr(flags);
 	else if (MATCH("RTE", 3))
-		return_value = _readRangeTblEntry();
+		return_value = _readRangeTblEntry(flags);
 	else if (MATCH("RANGETBLFUNCTION", 16))
-		return_value = _readRangeTblFunction();
+		return_value = _readRangeTblFunction(flags);
 	else if (MATCH("TABLESAMPLECLAUSE", 17))
-		return_value = _readTableSampleClause();
+		return_value = _readTableSampleClause(flags);
 	else if (MATCH("NOTIFY", 6))
-		return_value = _readNotifyStmt();
+		return_value = _readNotifyStmt(flags);
 	else if (MATCH("DEFELEM", 7))
-		return_value = _readDefElem();
+		return_value = _readDefElem(flags);
 	else if (MATCH("DECLARECURSOR", 13))
-		return_value = _readDeclareCursorStmt();
+		return_value = _readDeclareCursorStmt(flags);
 	else if (MATCH("PLANNEDSTMT", 11))
-		return_value = _readPlannedStmt();
+		return_value = _readPlannedStmt(flags);
 	else if (MATCH("PLAN", 4))
-		return_value = _readPlan();
+		return_value = _readPlan(flags);
 	else if (MATCH("RESULT", 6))
-		return_value = _readResult();
+		return_value = _readResult(flags);
 	else if (MATCH("MODIFYTABLE", 11))
-		return_value = _readModifyTable();
+		return_value = _readModifyTable(flags);
 	else if (MATCH("APPEND", 6))
-		return_value = _readAppend();
+		return_value = _readAppend(flags);
 	else if (MATCH("MERGEAPPEND", 11))
-		return_value = _readMergeAppend();
+		return_value = _readMergeAppend(flags);
 	else if (MATCH("RECURSIVEUNION", 14))
-		return_value = _readRecursiveUnion();
+		return_value = _readRecursiveUnion(flags);
 	else if (MATCH("BITMAPAND", 9))
-		return_value = _readBitmapAnd();
+		return_value = _readBitmapAnd(flags);
 	else if (MATCH("BITMAPOR", 8))
-		return_value = _readBitmapOr();
+		return_value = _readBitmapOr(flags);
 	else if (MATCH("SCAN", 4))
-		return_value = _readScan();
+		return_value = _readScan(flags);
 	else if (MATCH("SEQSCAN", 7))
-		return_value = _readSeqScan();
+		return_value = _readSeqScan(flags);
 	else if (MATCH("SAMPLESCAN", 10))
-		return_value = _readSampleScan();
+		return_value = _readSampleScan(flags);
 	else if (MATCH("INDEXSCAN", 9))
-		return_value = _readIndexScan();
+		return_value = _readIndexScan(flags);
 	else if (MATCH("INDEXONLYSCAN", 13))
-		return_value = _readIndexOnlyScan();
+		return_value = _readIndexOnlyScan(flags);
 	else if (MATCH("BITMAPINDEXSCAN", 15))
-		return_value = _readBitmapIndexScan();
+		return_value = _readBitmapIndexScan(flags);
 	else if (MATCH("BITMAPHEAPSCAN", 14))
-		return_value = _readBitmapHeapScan();
+		return_value = _readBitmapHeapScan(flags);
 	else if (MATCH("TIDSCAN", 7))
-		return_value = _readTidScan();
+		return_value = _readTidScan(flags);
 	else if (MATCH("SUBQUERYSCAN", 12))
-		return_value = _readSubqueryScan();
+		return_value = _readSubqueryScan(flags);
 	else if (MATCH("FUNCTIONSCAN", 12))
-		return_value = _readFunctionScan();
+		return_value = _readFunctionScan(flags);
 	else if (MATCH("VALUESSCAN", 10))
-		return_value = _readValuesScan();
+		return_value = _readValuesScan(flags);
 	else if (MATCH("CTESCAN", 7))
-		return_value = _readCteScan();
+		return_value = _readCteScan(flags);
 	else if (MATCH("WORKTABLESCAN", 13))
-		return_value = _readWorkTableScan();
+		return_value = _readWorkTableScan(flags);
 	else if (MATCH("FOREIGNSCAN", 11))
-		return_value = _readForeignScan();
+		return_value = _readForeignScan(flags);
 	else if (MATCH("JOIN", 4))
-		return_value = _readJoin();
+		return_value = _readJoin(flags);
 	else if (MATCH("NESTLOOP", 8))
-		return_value = _readNestLoop();
+		return_value = _readNestLoop(flags);
 	else if (MATCH("MERGEJOIN", 9))
-		return_value = _readMergeJoin();
+		return_value = _readMergeJoin(flags);
 	else if (MATCH("HASHJOIN", 8))
-		return_value = _readHashJoin();
+		return_value = _readHashJoin(flags);
 	else if (MATCH("MATERIAL", 8))
-		return_value = _readMaterial();
+		return_value = _readMaterial(flags);
 	else if (MATCH("SORT", 4))
-		return_value = _readSort();
+		return_value = _readSort(flags);
 	else if (MATCH("GROUP", 5))
-		return_value = _readGroup();
+		return_value = _readGroup(flags);
 	else if (MATCH("AGG", 3))
-		return_value = _readAgg();
+		return_value = _readAgg(flags);
 	else if (MATCH("WINDOWAGG", 9))
-		return_value = _readWindowAgg();
+		return_value = _readWindowAgg(flags);
 	else if (MATCH("UNIQUE", 6))
-		return_value = _readUnique();
+		return_value = _readUnique(flags);
 	else if (MATCH("HASH", 4))
-		return_value = _readHash();
+		return_value = _readHash(flags);
 	else if (MATCH("SETOP", 5))
-		return_value = _readSetOp();
+		return_value = _readSetOp(flags);
 	else if (MATCH("LOCKROWS", 8))
-		return_value = _readLockRows();
+		return_value = _readLockRows(flags);
 	else if (MATCH("LIMIT", 5))
-		return_value = _readLimit();
+		return_value = _readLimit(flags);
 	else if (MATCH("NESTLOOPPARAM", 13))
-		return_value = _readNestLoopParam();
+		return_value = _readNestLoopParam(flags);
 	else if (MATCH("PLANROWMARK", 11))
-		return_value = _readPlanRowMark();
+		return_value = _readPlanRowMark(flags);
 	else if (MATCH("PLANINVALITEM", 13))
-		return_value = _readPlanInvalItem();
+		return_value = _readPlanInvalItem(flags);
 	else if (MATCH("SUBPLAN", 7))
-		return_value = _readSubPlan();
+		return_value = _readSubPlan(flags);
 	else if (MATCH("ALTERNATIVESUBPLAN", 18))
-		return_value = _readAlternativeSubPlan();
+		return_value = _readAlternativeSubPlan(flags);
 	else
 	{
 		elog(ERROR, "badly formatted node string \"%.32s\"...", token);
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 274480e..0e9e28b 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -527,7 +527,23 @@ extern char *nodeToString(const void *obj);
 /*
  * nodes/{readfuncs.c,read.c}
  */
-extern void *stringToNode(char *str);
+extern void *stringToNodeExtended(char *str, int flags);
+
+#define STRINGTONODE_INVALIDATE_OPFUNCID			0x0001
+
+/*
+ * Most callers want to zero out opfuncid fields in parsed node trees,
+ * because most of the time a node tree we've just parsed is taken from
+ * a system catalog.  We want to always read it as zero to force it to be
+ * re-looked-up in the pg_operator entry.  This ensures that stored rules
+ * don't have hidden dependencies on operators' functions.  (We don't
+ * currently support an ALTER OPERATOR command, but might someday.)
+ *
+ * Callers who don't want this behavior can use stringToNodeExtended
+ * directly.
+ */
+#define stringToNode(x) \
+	stringToNodeExtended((x), STRINGTONODE_INVALIDATE_OPFUNCID)
 
 /*
  * nodes/copyfuncs.c
diff --git a/src/include/nodes/readfuncs.h b/src/include/nodes/readfuncs.h
index 703307e..f2fc61e 100644
--- a/src/include/nodes/readfuncs.h
+++ b/src/include/nodes/readfuncs.h
@@ -21,11 +21,11 @@
  */
 extern char *pg_strtok(int *length);
 extern char *debackslash(char *token, int length);
-extern void *nodeRead(char *token, int tok_len);
+extern void *nodeRead(char *token, int tok_len, int flags);
 
 /*
  * prototypes for functions in readfuncs.c
  */
-extern Node *parseNodeString(void);
+extern Node *parseNodeString(int flags);
 
 #endif   /* READFUNCS_H */

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Robert Haas (#1)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.

Right, but considering that nobody has even thought about implementing
such a command in the past twenty years, maybe we should just change
the behavior of those read routines? I've wondered for some time why
we don't just insist on the parser filling those nodes fully to begin
with, and get rid of the notion that assorted random places should
be expected to fix them up later. This is one of the behaviors that
would have to change to support such a simplification.

... The attached
patch does by adding an "int flags" field to the relevant read
routines.

Ick. TBH, this is just taking a bad design and putting another one
on top.

regards, tom lane

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

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Tom Lane (#2)

Re: clearing opfuncid vs. parallel query

On Wed, Sep 23, 2015 at 4:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.

Right, but considering that nobody has even thought about implementing
such a command in the past twenty years, maybe we should just change
the behavior of those read routines?

Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.

But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too. It would even save a
few cycles. Would you also want to rip out the stuff that fixes up
opfuncid as dead code? I assume yes, but sometimes I assume things
that are false.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Robert Haas (#3)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too. It would even save a
few cycles. Would you also want to rip out the stuff that fixes up
opfuncid as dead code? I assume yes, but sometimes I assume things
that are false.

Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later. I'm not sure right now that everyplace that initially
creates OpExpr etc. nodes is on board with having to supply opfuncid.
I do know that the main path through the parser provides 'em. (So another
reason I don't like the current approach is that I doubt all code that
should theoretically be doing set_opfuncid() is actually doing so; it
would be too easy to miss the need for it in simple testing.)

regards, tom lane

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

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Tom Lane (#4)

1 attachment(s)

Re: clearing opfuncid vs. parallel query

On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too. It would even save a
few cycles. Would you also want to rip out the stuff that fixes up
opfuncid as dead code? I assume yes, but sometimes I assume things
that are false.

Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.

So, you're thinking of something as simple as the attached?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

dont-clear-opfuncid.patchapplication/x-patch; name=dont-clear-opfuncid.patchDownload

diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index ef88209..08519ed 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -665,17 +665,6 @@ _readOpExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
 	READ_OID_FIELD(opcollid);
@@ -696,17 +685,6 @@ _readDistinctExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
 	READ_OID_FIELD(opcollid);
@@ -727,17 +705,6 @@ _readNullIfExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
 	READ_OID_FIELD(opcollid);
@@ -758,17 +725,6 @@ _readScalarArrayOpExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_BOOL_FIELD(useOr);
 	READ_OID_FIELD(inputcollid);
 	READ_NODE_FIELD(args);

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Robert Haas (#5)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.

So, you're thinking of something as simple as the attached?

Right. I'll make a mental to-do to see about getting rid of set_opfuncid
later.

regards, tom lane

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

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Tom Lane (#6)

Re: clearing opfuncid vs. parallel query

On Wed, Sep 23, 2015 at 7:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.

So, you're thinking of something as simple as the attached?

Right. I'll make a mental to-do to see about getting rid of set_opfuncid
later.

Cool, thanks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Andres Freund

andres@anarazel.de

over 10 years ago

In reply to: Robert Haas (#3)

Re: clearing opfuncid vs. parallel query

On 2015-09-23 17:29:50 -0400, Robert Haas wrote:

Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.

Wouldn't we use plan invalidation to deal with that anyway?

Andres

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

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Andres Freund (#8)

Re: clearing opfuncid vs. parallel query

Andres Freund <andres@anarazel.de> writes:

On 2015-09-23 17:29:50 -0400, Robert Haas wrote:

Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.

Wouldn't we use plan invalidation to deal with that anyway?

Plan invalidation wouldn't help, because the obsolete data exists
on-disk in stored rules. You'd have to run through the pg_rewrite
entries and update them.

To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class. In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.

regards, tom lane

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

#10

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Tom Lane (#9)

Re: clearing opfuncid vs. parallel query

On Thu, Sep 24, 2015 at 11:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2015-09-23 17:29:50 -0400, Robert Haas wrote:

Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.

Wouldn't we use plan invalidation to deal with that anyway?

Plan invalidation wouldn't help, because the obsolete data exists
on-disk in stored rules. You'd have to run through the pg_rewrite
entries and update them.

To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class. In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.

Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it. Or we could engineer something else.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#11

Alvaro Herrera

alvherre@2ndquadrant.com

over 10 years ago

In reply to: Tom Lane (#9)

Re: clearing opfuncid vs. parallel query

Tom Lane wrote:

To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class. In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.

I think allowing an operator's implementation function to change would
be rather problematic, would it not? There's no way to know whether the
semantic changes to stored rules would make sense, not least because the
person running ALTER OPERATOR wouldn't know (== has no easy way to find
out) what is being changed in the first place.

To me, it looks like we should just not allow ALTER OPERATOR SET FUNCTION
to be implemented at all.

It's not like changing an operator's implementation is an oft-requested
feature anyway.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

#12

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Alvaro Herrera (#11)

Re: clearing opfuncid vs. parallel query

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

I think allowing an operator's implementation function to change would
be rather problematic, would it not? There's no way to know whether the
semantic changes to stored rules would make sense, not least because the
person running ALTER OPERATOR wouldn't know (== has no easy way to find
out) what is being changed in the first place.

To me, it looks like we should just not allow ALTER OPERATOR SET FUNCTION
to be implemented at all.

It's not like changing an operator's implementation is an oft-requested
feature anyway.

Well, the point is that usually anything you want in this line can be
accomplished by executing CREATE OR REPLACE FUNCTION on the operator's
function. It's up to you that that doesn't create any interesting
semantic incompatibilities. That would still be true for an ALTER
OPERATOR SET FUNCTION command: if you break it, you get to keep both
pieces. But the availability of that alternative really cuts down
on the plausible use-cases for ALTER OPERATOR SET FUNCTION.

regards, tom lane

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

#13

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Robert Haas (#10)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it. Or we could engineer something else.

For the record: that's true for the patch you just committed. But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.

regards, tom lane

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

#14

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Tom Lane (#13)

Re: clearing opfuncid vs. parallel query

On Thu, Sep 24, 2015 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it. Or we could engineer something else.

For the record: that's true for the patch you just committed. But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.

True. I think it's pretty wacky that we store the opfuncid in the
tree at all. If somebody were to propose adding a dependent value of
that sort to a node type that didn't already have it, I suspect either
you or I would do our best to shoot that down. The only possible
argument for having that in there at all is that the performance gains
from so doing are so large that we have no choice but to sacrifice a
principle to expediency.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#15

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Robert Haas (#14)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Sep 24, 2015 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

For the record: that's true for the patch you just committed. But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.

True. I think it's pretty wacky that we store the opfuncid in the
tree at all. If somebody were to propose adding a dependent value of
that sort to a node type that didn't already have it, I suspect either
you or I would do our best to shoot that down. The only possible
argument for having that in there at all is that the performance gains
from so doing are so large that we have no choice but to sacrifice a
principle to expediency.

Hm. It might be interesting to try removing those node fields altogether
and see what it costs us. Setting the field at parse time is zero-cost,
at least in the primary code path through make_op(), because we have our
hands on the pg_operator row at that time anyway. (I have a vague
recollection that that was once not true, but it certainly is true now
and has been for a long time.) Not having it would cost us one syscache
lookup per operator at execution start, and maybe more than that if there
are additional places that would need the function OID, which there
probably are --- volatility checks in the planner come to mind. But
I'm not sure how significant that would be. There are certainly plenty
of syscache lookups being done at plan startup anyway.

But this is mostly moot unless someone is actively planning to try to
implement ALTER OPERATOR SET FUNCTION.

regards, tom lane

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

#16

YUriy Zhuravlev

u.zhuravlev@postgrespro.ru

about 10 years ago

In reply to: Robert Haas (#1)

Re: clearing opfuncid vs. parallel query

Hello.
Currently using nodeToString and stringToNode you can not pass a full plan. In
this regard, what is the plan to fix it? Or in the under task parallel query
does not have such a problem?

This turns out not to be straightforward to code, because we
don't have a generic plan tree walker,

I have an inner development. I am using python analyzing header files and
generates a universal walker (parser, paths ,executer and etc trees), as well
as the serializer and deserializer to jsonb. Maybe I should publish this code?

Thanks.

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

#17

David Fetter

david@fetter.org

about 10 years ago

In reply to: YUriy Zhuravlev (#16)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:

Hello.
Currently using nodeToString and stringToNode you can not pass a
full plan. In this regard, what is the plan to fix it? Or in the
under task parallel query does not have such a problem?

This turns out not to be straightforward to code, because we don't
have a generic plan tree walker,

I have an inner development. I am using python analyzing header
files and generates a universal walker (parser, paths ,executer and
etc trees), as well as the serializer and deserializer to jsonb.
Maybe I should publish this code?

Please do.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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

#18

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: YUriy Zhuravlev (#16)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:

Hello.
Currently using nodeToString and stringToNode you can not pass a full plan. In
this regard, what is the plan to fix it? Or in the under task parallel query
does not have such a problem?

It's already fixed. See commits
a0d9f6e434bb56f7e5441b7988f3982feead33b3 and
9f1255ac859364a86264a67729dbd1a36dd63ff2.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#19

YUriy Zhuravlev

u.zhuravlev@postgrespro.ru

about 10 years ago

In reply to: Robert Haas (#18)

Re: clearing opfuncid vs. parallel query

On Thursday 22 October 2015 12:53:49 you wrote:

On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev

<u.zhuravlev@postgrespro.ru> wrote:

Hello.
Currently using nodeToString and stringToNode you can not pass a full
plan. In this regard, what is the plan to fix it? Or in the under task
parallel query does not have such a problem?

It's already fixed. See commits
a0d9f6e434bb56f7e5441b7988f3982feead33b3 and
9f1255ac859364a86264a67729dbd1a36dd63ff2.

Ahh. Thanks.

And then another question:
What do you think if the generated equalfuncs.c, copyfuncs.c, readfuncs.c,
outfuncs.c from XML or JSON?
In order not to change the code in four places when changing nodes.

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

#20

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: YUriy Zhuravlev (#19)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 1:19 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:

And then another question:
What do you think if the generated equalfuncs.c, copyfuncs.c, readfuncs.c,
outfuncs.c from XML or JSON?
In order not to change the code in four places when changing nodes.

It would be more useful, if we're going to autogenerate code, to do it
from the actual struct definitions.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#21

YUriy Zhuravlev

u.zhuravlev@postgrespro.ru

about 10 years ago

In reply to: Robert Haas (#20)

Re: clearing opfuncid vs. parallel query

On Thursday 22 October 2015 13:25:52 you wrote:

It would be more useful, if we're going to autogenerate code,

Are we going to use autogenerate code?

to do it from the actual struct definitions.

I can gen xml/json from actual struct. I offered XML/JSON as the analysis of C
code much more difficult. But my current generator just use the structure from
the header files (by pycparser).

Thanks.
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

#22

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: YUriy Zhuravlev (#21)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:

On Thursday 22 October 2015 13:25:52 you wrote:

It would be more useful, if we're going to autogenerate code,

Are we going to use autogenerate code?

I thought that's what you were proposing. Process the struct
definitions and emit .c files.

to do it from the actual struct definitions.

I can gen xml/json from actual struct. I offered XML/JSON as the analysis of C
code much more difficult. But my current generator just use the structure from
the header files (by pycparser).

Anything that is part of the build process will have to be done in C or Perl.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#23

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: Robert Haas (#22)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:

I can gen xml/json from actual struct. I offered XML/JSON as the analysis of C
code much more difficult. But my current generator just use the structure from
the header files (by pycparser).

Anything that is part of the build process will have to be done in C or Perl.

Yeah. The bar for introducing new build tool requirements is very high;
*way* higher than the likely benefit from not having to hand-maintain
outfuncs.c et al. If you wanna do this in Perl, fine, but we're not going
to introduce a requirement to have Python to build just because somebody
wants to write a tool in the latter not the former.

Having said that, there is more knowledge embedded in the nodes/*.c files
than there is in the nodes/*.h headers; an example being that there are
certain fields that we deliberately don't dump and restore. (This is
still true despite Robert's recent changes to take opfuncid out of that
class.) I'm not sure that you could get to a point where you were
generating this stuff from anything that wasn't in essence an arcane
representation of the .c files. It might be slightly harder to make
errors of omission that way, but I'm suspicious that that would come at
the cost of a net loss of readability.

regards, tom lane

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

#24

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Tom Lane (#23)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:

I can gen xml/json from actual struct. I offered XML/JSON as the analysis of C
code much more difficult. But my current generator just use the structure from
the header files (by pycparser).

Anything that is part of the build process will have to be done in C or Perl.

Yeah. The bar for introducing new build tool requirements is very high;
*way* higher than the likely benefit from not having to hand-maintain
outfuncs.c et al. If you wanna do this in Perl, fine, but we're not going
to introduce a requirement to have Python to build just because somebody
wants to write a tool in the latter not the former.

Having said that, there is more knowledge embedded in the nodes/*.c files
than there is in the nodes/*.h headers; an example being that there are
certain fields that we deliberately don't dump and restore. (This is
still true despite Robert's recent changes to take opfuncid out of that
class.)

Are those things, ah, documented somewhere?

I'm not sure that you could get to a point where you were
generating this stuff from anything that wasn't in essence an arcane
representation of the .c files. It might be slightly harder to make
errors of omission that way, but I'm suspicious that that would come at
the cost of a net loss of readability.

That is possible, but the current situation isn't good either.
Despite everybody's best efforts, we mess this up more than is really
desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
in a whole bunch of preceding commits, and I wonder if anybody else
would have found those if Noah hadn't. It's just too easy to miss
these things today. If generating the .c files isn't practical,
another alternative worth exploring would be a tool to cross-check
them against the .h files.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#25

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: Robert Haas (#24)

Re: clearing opfuncid vs. parallel query

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm not sure that you could get to a point where you were
generating this stuff from anything that wasn't in essence an arcane
representation of the .c files. It might be slightly harder to make
errors of omission that way, but I'm suspicious that that would come at
the cost of a net loss of readability.

That is possible, but the current situation isn't good either.
Despite everybody's best efforts, we mess this up more than is really
desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
in a whole bunch of preceding commits, and I wonder if anybody else
would have found those if Noah hadn't. It's just too easy to miss
these things today. If generating the .c files isn't practical,
another alternative worth exploring would be a tool to cross-check
them against the .h files.

Yeah, I could get on board with that. It doesn't seem terribly hard:
just make sure that all fields mentioned in the struct declaration are
mentioned in each relevant routine. (Cases where we intentionally skip
a field could be handled by adding a no-op macro, eg "COPY_IGNORE(foo);")

It would be nice if we could also check that the macro type is sane for
the field datatype (eg, not use COPY_SCALAR_FIELD() for a pointer field).
But that would probably increase the difficulty very substantially for
just a small gain in error detection.

regards, tom lane

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

#26

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Tom Lane (#25)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm not sure that you could get to a point where you were
generating this stuff from anything that wasn't in essence an arcane
representation of the .c files. It might be slightly harder to make
errors of omission that way, but I'm suspicious that that would come at
the cost of a net loss of readability.

That is possible, but the current situation isn't good either.
Despite everybody's best efforts, we mess this up more than is really
desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
in a whole bunch of preceding commits, and I wonder if anybody else
would have found those if Noah hadn't. It's just too easy to miss
these things today. If generating the .c files isn't practical,
another alternative worth exploring would be a tool to cross-check
them against the .h files.

Yeah, I could get on board with that. It doesn't seem terribly hard:
just make sure that all fields mentioned in the struct declaration are
mentioned in each relevant routine. (Cases where we intentionally skip
a field could be handled by adding a no-op macro, eg "COPY_IGNORE(foo);")

It would be nice if we could also check that the macro type is sane for
the field datatype (eg, not use COPY_SCALAR_FIELD() for a pointer field).
But that would probably increase the difficulty very substantially for
just a small gain in error detection.

I actually think that's quite an easy mistake to make, so I'd be happy
if we could catch it. But anything would be better than nothing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

#27

YUriy Zhuravlev

u.zhuravlev@postgrespro.ru

about 10 years ago

In reply to: Robert Haas (#22)

Re: clearing opfuncid vs. parallel query

I thought that's what you were proposing. Process the struct
definitions and emit .c files.

We have 2 ways. The first is always to generate the * .c files from the * .h
files. Another way is to generate once from * .h file a XML/JSON and after
generate from it to * .c files (parsing xml/json easy).

Anything that is part of the build process will have to be done in C or
Perl.

I know about the relationship between Postgres and C / Perl. Yet this is not
the language which would be worth to do something associated with code
generation. Python is better in many ways than the Perl. I'm not trying to
convince someone. I just see the situation, and I do not like.

What do you think about the format of the serialization? Now it is very
primitive. For example, there are selected data in order, rather than by key.
In its development, I used jsonb, it also helped to simplify the saved of
query plans in the Postgres table.

Thanks.

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

#28

YUriy Zhuravlev

u.zhuravlev@postgrespro.ru

about 10 years ago

In reply to: David Fetter (#17)

Re: clearing opfuncid vs. parallel query

On Thursday 22 October 2015 09:26:46 David Fetter wrote:

On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:

Hello.
Currently using nodeToString and stringToNode you can not pass a
full plan. In this regard, what is the plan to fix it? Or in the
under task parallel query does not have such a problem?

This turns out not to be straightforward to code, because we don't
have a generic plan tree walker,

I have an inner development. I am using python analyzing header
files and generates a universal walker (parser, paths ,executer and
etc trees), as well as the serializer and deserializer to jsonb.
Maybe I should publish this code?

Please do.

Tom Lane and Robert Haas are very unhappy with a python. Is there any reason?

Thanks!

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

#29

Alexander Korotkov

a.korotkov@postgrespro.ru

about 10 years ago

In reply to: YUriy Zhuravlev (#28)

Re: clearing opfuncid vs. parallel query

On Fri, Oct 23, 2015 at 12:31 PM, YUriy Zhuravlev <
u.zhuravlev@postgrespro.ru> wrote:

On Thursday 22 October 2015 09:26:46 David Fetter wrote:

On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:

Hello.
Currently using nodeToString and stringToNode you can not pass a
full plan. In this regard, what is the plan to fix it? Or in the
under task parallel query does not have such a problem?

This turns out not to be straightforward to code, because we don't
have a generic plan tree walker,

I have an inner development. I am using python analyzing header
files and generates a universal walker (parser, paths ,executer and
etc trees), as well as the serializer and deserializer to jsonb.
Maybe I should publish this code?

Please do.

Tom Lane and Robert Haas are very unhappy with a python. Is there any
reason?

Requirement of python with pycparser as build dependency is a
serious cataclysm. For instance, how many buildfarms will survive it?
This is why Tom and Robert are looking for ways to evade it.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

#30

YUriy Zhuravlev

u.zhuravlev@postgrespro.ru

about 10 years ago

In reply to: Alexander Korotkov (#29)

Re: clearing opfuncid vs. parallel query

On Friday 23 October 2015 12:41:50 you wrote:

Requirement of python with pycparser as build dependency is a
serious cataclysm. For instance, how many buildfarms will survive it?
This is why Tom and Robert are looking for ways to evade it.

I agree. But it is also a fact that Perl less suited for such development.
Also at the moment Python is more common:
http://www.tiobe.com/index.php/content/paperinfo/tpci/index.html

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

#31

Noah Misch

noah@leadboat.com

about 10 years ago

In reply to: Robert Haas (#26)

Re: clearing opfuncid vs. parallel query

On Thu, Oct 22, 2015 at 05:14:29PM -0400, Robert Haas wrote:

On Thu, Oct 22, 2015 at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Despite everybody's best efforts, we mess this up more than is really
desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
in a whole bunch of preceding commits, and I wonder if anybody else
would have found those if Noah hadn't. It's just too easy to miss
these things today. If generating the .c files isn't practical,
another alternative worth exploring would be a tool to cross-check
them against the .h files.

FWIW, such a tool found the bugs I fixed in 53bbc68, b5eccae, b8fe12a. My
script generates {copy,equal,out,read}funcs.c functions from the headers and
diffs each function against its hand-maintained counterpart. I read through
the diff for anything suspicious. (Most functions with deliberate nonstandard
behavior carry a comment about it.)

Yeah, I could get on board with that. It doesn't seem terribly hard:
just make sure that all fields mentioned in the struct declaration are
mentioned in each relevant routine. (Cases where we intentionally skip
a field could be handled by adding a no-op macro, eg "COPY_IGNORE(foo);")

It would be nice if we could also check that the macro type is sane for
the field datatype (eg, not use COPY_SCALAR_FIELD() for a pointer field).
But that would probably increase the difficulty very substantially for
just a small gain in error detection.

I actually think that's quite an easy mistake to make, so I'd be happy
if we could catch it. But anything would be better than nothing.

So far, type mismatch defects have been no less common than neglecting a field
entirely.

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