public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] remove deprecated v8.2 containment operators 7+ messages / 3 participants [nested] [flat]
* [PATCH] remove deprecated v8.2 containment operators @ 2020-04-12 03:57 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Justin Pryzby @ 2020-04-12 03:57 UTC (permalink / raw) See also: ba920e1c9182eac55d5f1327ab0d29b721154277 684ad6a92fcc33adebdab65c4e7d72a68ba05408 3165426e54df02a6199c0a216546e5095e875a0a 2f70fdb0644c32c4154236c2b5c241bec92eac5e 591d282e8d3e0448ec1339c6b066e10953f040a2 --- contrib/cube/Makefile | 2 +- contrib/cube/cube--1.4--1.5.sql | 8 ++++++++ contrib/cube/cube.control | 2 +- contrib/hstore/Makefile | 1 + contrib/hstore/hstore--1.8--1.9.sql | 7 +++++++ contrib/hstore/hstore.control | 2 +- contrib/intarray/Makefile | 2 +- contrib/intarray/intarray--1.4--1.5.sql | 10 ++++++++++ contrib/intarray/intarray.control | 2 +- contrib/seg/Makefile | 2 +- contrib/seg/seg--1.3--1.4.sql | 8 ++++++++ contrib/seg/seg.control | 2 +- doc/src/sgml/cube.sgml | 8 -------- doc/src/sgml/hstore.sgml | 10 ---------- doc/src/sgml/intarray.sgml | 8 -------- doc/src/sgml/seg.sgml | 8 -------- 16 files changed, 41 insertions(+), 41 deletions(-) create mode 100644 contrib/cube/cube--1.4--1.5.sql create mode 100644 contrib/hstore/hstore--1.8--1.9.sql create mode 100644 contrib/intarray/intarray--1.4--1.5.sql create mode 100644 contrib/seg/seg--1.3--1.4.sql diff --git a/contrib/cube/Makefile b/contrib/cube/Makefile index 54f609db17..cf195506c7 100644 --- a/contrib/cube/Makefile +++ b/contrib/cube/Makefile @@ -7,7 +7,7 @@ OBJS = \ cubeparse.o EXTENSION = cube -DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql \ +DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql cube--1.4--1.5.sql \ cube--1.1--1.2.sql cube--1.0--1.1.sql PGFILEDESC = "cube - multidimensional cube data type" diff --git a/contrib/cube/cube--1.4--1.5.sql b/contrib/cube/cube--1.4--1.5.sql new file mode 100644 index 0000000000..54492e5d18 --- /dev/null +++ b/contrib/cube/cube--1.4--1.5.sql @@ -0,0 +1,8 @@ +/* contrib/cube/cube--1.4--1.5.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION cube UPDATE TO '1.5'" to load this file. \quit + +-- Remove @ and ~ +DROP OPERATOR @ (cube, cube); +DROP OPERATOR ~ (cube, cube); diff --git a/contrib/cube/cube.control b/contrib/cube/cube.control index 3e238fc937..50427ec117 100644 --- a/contrib/cube/cube.control +++ b/contrib/cube/cube.control @@ -1,6 +1,6 @@ # cube extension comment = 'data type for multidimensional cubes' -default_version = '1.4' +default_version = '1.5' module_pathname = '$libdir/cube' relocatable = true trusted = true diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile index c4e339b57c..97b228b65f 100644 --- a/contrib/hstore/Makefile +++ b/contrib/hstore/Makefile @@ -12,6 +12,7 @@ OBJS = \ EXTENSION = hstore DATA = hstore--1.4.sql \ + hstore--1.8--1.9.sql \ hstore--1.7--1.8.sql \ hstore--1.6--1.7.sql \ hstore--1.5--1.6.sql \ diff --git a/contrib/hstore/hstore--1.8--1.9.sql b/contrib/hstore/hstore--1.8--1.9.sql new file mode 100644 index 0000000000..7cd3467c55 --- /dev/null +++ b/contrib/hstore/hstore--1.8--1.9.sql @@ -0,0 +1,7 @@ +/* contrib/hstore/hstore--1.8--1.9.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION hstore UPDATE TO '1.9'" to load this file. \quit + +-- Remove @ +DROP OPERATOR @ (hstore, hstore); diff --git a/contrib/hstore/hstore.control b/contrib/hstore/hstore.control index 89e3c746c4..b73c28aa4d 100644 --- a/contrib/hstore/hstore.control +++ b/contrib/hstore/hstore.control @@ -1,6 +1,6 @@ # hstore extension comment = 'data type for storing sets of (key, value) pairs' -default_version = '1.8' +default_version = '1.9' module_pathname = '$libdir/hstore' relocatable = true trusted = true diff --git a/contrib/intarray/Makefile b/contrib/intarray/Makefile index 01faa36b10..3817c1669a 100644 --- a/contrib/intarray/Makefile +++ b/contrib/intarray/Makefile @@ -12,7 +12,7 @@ OBJS = \ _intbig_gist.o EXTENSION = intarray -DATA = intarray--1.3--1.4.sql intarray--1.2--1.3.sql \ +DATA = intarray--1.4--1.5.sql intarray--1.3--1.4.sql intarray--1.2--1.3.sql \ intarray--1.2.sql intarray--1.1--1.2.sql \ intarray--1.0--1.1.sql PGFILEDESC = "intarray - functions and operators for arrays of integers" diff --git a/contrib/intarray/intarray--1.4--1.5.sql b/contrib/intarray/intarray--1.4--1.5.sql new file mode 100644 index 0000000000..d0ac11545c --- /dev/null +++ b/contrib/intarray/intarray--1.4--1.5.sql @@ -0,0 +1,10 @@ +/* contrib/intarray/intarray--1.4--1.5.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION intarray UPDATE TO '1.5'" to load this file. \quit + +-- Remove @ +DROP OPERATOR @ (_int4, _int4); + +-- Remove ~ from GIN - it was removed from gist in 1.4 +DROP OPERATOR ~ (_int4, _int4); diff --git a/contrib/intarray/intarray.control b/contrib/intarray/intarray.control index bbc837c573..c3ff753e2c 100644 --- a/contrib/intarray/intarray.control +++ b/contrib/intarray/intarray.control @@ -1,6 +1,6 @@ # intarray extension comment = 'functions, operators, and index support for 1-D arrays of integers' -default_version = '1.4' +default_version = '1.5' module_pathname = '$libdir/_int' relocatable = true trusted = true diff --git a/contrib/seg/Makefile b/contrib/seg/Makefile index f3578a8634..bb63e83506 100644 --- a/contrib/seg/Makefile +++ b/contrib/seg/Makefile @@ -7,7 +7,7 @@ OBJS = \ segparse.o EXTENSION = seg -DATA = seg--1.1.sql seg--1.1--1.2.sql seg--1.2--1.3.sql \ +DATA = seg--1.1.sql seg--1.1--1.2.sql seg--1.2--1.3.sql seg--1.3--1.4.sql \ seg--1.0--1.1.sql PGFILEDESC = "seg - line segment data type" diff --git a/contrib/seg/seg--1.3--1.4.sql b/contrib/seg/seg--1.3--1.4.sql new file mode 100644 index 0000000000..13babddba4 --- /dev/null +++ b/contrib/seg/seg--1.3--1.4.sql @@ -0,0 +1,8 @@ +/* contrib/seg/seg--1.3--1.4.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION seg UPDATE TO '1.4'" to load this file. \quit + +-- Remove @ and ~ +DROP OPERATOR @ (seg, seg); +DROP OPERATOR ~ (seg, seg); diff --git a/contrib/seg/seg.control b/contrib/seg/seg.control index 9ac3080848..e2c6a4750f 100644 --- a/contrib/seg/seg.control +++ b/contrib/seg/seg.control @@ -1,6 +1,6 @@ # seg extension comment = 'data type for representing line segments or floating-point intervals' -default_version = '1.3' +default_version = '1.4' module_pathname = '$libdir/seg' relocatable = true trusted = true diff --git a/doc/src/sgml/cube.sgml b/doc/src/sgml/cube.sgml index 3dd89bb14d..adf8dbaa91 100644 --- a/doc/src/sgml/cube.sgml +++ b/doc/src/sgml/cube.sgml @@ -222,14 +222,6 @@ </tgroup> </table> - <para> - (Before PostgreSQL 8.2, the containment operators <literal>@></literal> and <literal><@</literal> were - respectively called <literal>@</literal> and <literal>~</literal>. These names are still available, but are - deprecated and will eventually be retired. Notice that the old names - are reversed from the convention formerly followed by the core geometric - data types!) - </para> - <para> In addition to the above operators, the usual comparison operators shown in <xref linkend="functions-comparison-op-table"/> are diff --git a/doc/src/sgml/hstore.sgml b/doc/src/sgml/hstore.sgml index e867fcc5ae..db5779052a 100644 --- a/doc/src/sgml/hstore.sgml +++ b/doc/src/sgml/hstore.sgml @@ -318,16 +318,6 @@ key => NULL </tgroup> </table> - <note> - <para> - Prior to PostgreSQL 8.2, the containment operators <literal>@></literal> - and <literal><@</literal> were called <literal>@</literal> and <literal>~</literal>, - respectively. These names are still available, but are deprecated and will - eventually be removed. Notice that the old names are reversed from the - convention formerly followed by the core geometric data types! - </para> - </note> - <table id="hstore-func-table"> <title><type>hstore</type> Functions</title> <tgroup cols="1"> diff --git a/doc/src/sgml/intarray.sgml b/doc/src/sgml/intarray.sgml index af44c7b214..dfe98279c0 100644 --- a/doc/src/sgml/intarray.sgml +++ b/doc/src/sgml/intarray.sgml @@ -364,14 +364,6 @@ </tgroup> </table> - <para> - (Before PostgreSQL 8.2, the containment operators <literal>@></literal> and - <literal><@</literal> were respectively called <literal>@</literal> and <literal>~</literal>. - These names are still available, but are deprecated and will eventually be - retired. Notice that the old names are reversed from the convention - formerly followed by the core geometric data types!) - </para> - <para> The operators <literal>&&</literal>, <literal>@></literal> and <literal><@</literal> are equivalent to <productname>PostgreSQL</productname>'s built-in diff --git a/doc/src/sgml/seg.sgml b/doc/src/sgml/seg.sgml index e0dfbc76cf..9be69e3609 100644 --- a/doc/src/sgml/seg.sgml +++ b/doc/src/sgml/seg.sgml @@ -352,14 +352,6 @@ test=> select '6.25 .. 6.50'::seg as "pH"; </tgroup> </table> - <para> - (Before PostgreSQL 8.2, the containment operators <literal>@></literal> and <literal><@</literal> were - respectively called <literal>@</literal> and <literal>~</literal>. These names are still available, but are - deprecated and will eventually be retired. Notice that the old names - are reversed from the convention formerly followed by the core geometric - data types!) - </para> - <para> In addition to the above operators, the usual comparison operators shown in <xref linkend="functions-comparison-op-table"/> are -- 2.17.0 --0OWHXb1mYLuhj1Ox-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* Use outerPlanState macro instead of referring to leffttree @ 2022-04-14 06:49 Richard Guo <[email protected]> 2022-07-01 21:32 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Richard Guo @ 2022-04-14 06:49 UTC (permalink / raw) To: pgsql-hackers In the executor code, we mix use outerPlanState macro and referring to leffttree. Commit 40f42d2a tried to keep the code consistent by replacing referring to lefftree with outerPlanState macro, but there are still some outliers. This patch tries to clean them up. Thanks Richard Attachments: [application/octet-stream] v1-0001-Use-outerPlanState-macro.patch (9.4K, ../../CAMbWs4-vYhh1xsa_veah4PUed2Xq=Ed_YH3=Mqt5A3Y=EgfCEg@mail.gmail.com/3-v1-0001-Use-outerPlanState-macro.patch) download | inline diff: From d0d05efbee3f5cfba7decb6a3e16eae456bd6cf3 Mon Sep 17 00:00:00 2001 From: pgsql-guo <[email protected]> Date: Thu, 14 Apr 2022 06:14:23 +0000 Subject: [PATCH v1] Use outerPlanState macro instead of referring to leffttree. --- src/backend/executor/nodeGather.c | 4 ++-- src/backend/executor/nodeGatherMerge.c | 4 ++-- src/backend/executor/nodeHash.c | 6 ++++-- src/backend/executor/nodeHashjoin.c | 6 ++++-- src/backend/executor/nodeLimit.c | 6 ++++-- src/backend/executor/nodeLockRows.c | 6 ++++-- src/backend/executor/nodeMergejoin.c | 11 +++++++---- src/backend/executor/nodeProjectSet.c | 6 ++++-- src/backend/executor/nodeResult.c | 7 ++++--- src/backend/executor/nodeSetOp.c | 8 +++++--- src/backend/executor/nodeUnique.c | 6 ++++-- 11 files changed, 44 insertions(+), 26 deletions(-) diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c index 4f8a17df7d..1283d5b737 100644 --- a/src/backend/executor/nodeGather.c +++ b/src/backend/executor/nodeGather.c @@ -168,13 +168,13 @@ ExecGather(PlanState *pstate) /* Initialize, or re-initialize, shared state needed by workers. */ if (!node->pei) - node->pei = ExecInitParallelPlan(node->ps.lefttree, + node->pei = ExecInitParallelPlan(outerPlanState(node), estate, gather->initParam, gather->num_workers, node->tuples_needed); else - ExecParallelReinitialize(node->ps.lefttree, + ExecParallelReinitialize(outerPlanState(node), node->pei, gather->initParam); diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c index a488cc6d8b..3b1007f352 100644 --- a/src/backend/executor/nodeGatherMerge.c +++ b/src/backend/executor/nodeGatherMerge.c @@ -212,13 +212,13 @@ ExecGatherMerge(PlanState *pstate) /* Initialize, or re-initialize, shared state needed by workers. */ if (!node->pei) - node->pei = ExecInitParallelPlan(node->ps.lefttree, + node->pei = ExecInitParallelPlan(outerPlanState(node), estate, gm->initParam, gm->num_workers, node->tuples_needed); else - ExecParallelReinitialize(node->ps.lefttree, + ExecParallelReinitialize(outerPlanState(node), node->pei, gm->initParam); diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 3510a4247c..123079c16c 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -2212,12 +2212,14 @@ ExecHashTableResetMatchFlags(HashJoinTable hashtable) void ExecReScanHash(HashState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index 88b870655e..520e06c0d4 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -1290,6 +1290,8 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate, void ExecReScanHashJoin(HashJoinState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * In a multi-batch join, we currently have to do rescans the hard way, * primarily because batch temp files may have already been released. But @@ -1368,8 +1370,8 @@ ExecReScanHashJoin(HashJoinState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->js.ps.lefttree->chgParam == NULL) - ExecReScan(node->js.ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } void diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c index 1b91b123fa..75e4302b94 100644 --- a/src/backend/executor/nodeLimit.c +++ b/src/backend/executor/nodeLimit.c @@ -542,6 +542,8 @@ ExecEndLimit(LimitState *node) void ExecReScanLimit(LimitState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * Recompute limit/offset in case parameters changed, and reset the state * machine. We must do this before rescanning our child node, in case @@ -553,6 +555,6 @@ ExecReScanLimit(LimitState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..a74813c7aa 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -394,10 +394,12 @@ ExecEndLockRows(LockRowsState *node) void ExecReScanLockRows(LockRowsState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c index 864e3baf86..fed345eae5 100644 --- a/src/backend/executor/nodeMergejoin.c +++ b/src/backend/executor/nodeMergejoin.c @@ -1658,6 +1658,9 @@ ExecEndMergeJoin(MergeJoinState *node) void ExecReScanMergeJoin(MergeJoinState *node) { + PlanState *outerPlan = outerPlanState(node); + PlanState *innerPlan = innerPlanState(node); + ExecClearTuple(node->mj_MarkedTupleSlot); node->mj_JoinState = EXEC_MJ_INITIALIZE_OUTER; @@ -1670,8 +1673,8 @@ ExecReScanMergeJoin(MergeJoinState *node) * if chgParam of subnodes is not null then plans will be re-scanned by * first ExecProcNode. */ - if (node->js.ps.lefttree->chgParam == NULL) - ExecReScan(node->js.ps.lefttree); - if (node->js.ps.righttree->chgParam == NULL) - ExecReScan(node->js.ps.righttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); + if (innerPlan->chgParam == NULL) + ExecReScan(innerPlan); } diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c index ea40d61b0b..adabfa2882 100644 --- a/src/backend/executor/nodeProjectSet.c +++ b/src/backend/executor/nodeProjectSet.c @@ -339,6 +339,8 @@ ExecEndProjectSet(ProjectSetState *node) void ExecReScanProjectSet(ProjectSetState *node) { + PlanState *outerPlan = outerPlanState(node); + /* Forget any incompletely-evaluated SRFs */ node->pending_srf_tuples = false; @@ -346,6 +348,6 @@ ExecReScanProjectSet(ProjectSetState *node) * If chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeResult.c b/src/backend/executor/nodeResult.c index d0413e05de..78a744ba85 100644 --- a/src/backend/executor/nodeResult.c +++ b/src/backend/executor/nodeResult.c @@ -259,6 +259,8 @@ ExecEndResult(ResultState *node) void ExecReScanResult(ResultState *node) { + PlanState *outerPlan = outerPlanState(node); + node->rs_done = false; node->rs_checkqual = (node->resconstantqual != NULL); @@ -266,7 +268,6 @@ ExecReScanResult(ResultState *node) * If chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree && - node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan && outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c index 4b428cfa39..8af12f96e7 100644 --- a/src/backend/executor/nodeSetOp.c +++ b/src/backend/executor/nodeSetOp.c @@ -597,6 +597,8 @@ ExecEndSetOp(SetOpState *node) void ExecReScanSetOp(SetOpState *node) { + PlanState *outerPlan = outerPlanState(node); + ExecClearTuple(node->ps.ps_ResultTupleSlot); node->setop_done = false; node->numOutput = 0; @@ -617,7 +619,7 @@ ExecReScanSetOp(SetOpState *node) * parameter changes, then we can just rescan the existing hash table; * no need to build it again. */ - if (node->ps.lefttree->chgParam == NULL) + if (outerPlan->chgParam == NULL) { ResetTupleHashIterator(node->hashtable, &node->hashiter); return; @@ -646,6 +648,6 @@ ExecReScanSetOp(SetOpState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeUnique.c b/src/backend/executor/nodeUnique.c index 6c99d13a39..d1399feabb 100644 --- a/src/backend/executor/nodeUnique.c +++ b/src/backend/executor/nodeUnique.c @@ -180,6 +180,8 @@ ExecEndUnique(UniqueState *node) void ExecReScanUnique(UniqueState *node) { + PlanState *outerPlan = outerPlanState(node); + /* must clear result tuple so first input tuple is returned */ ExecClearTuple(node->ps.ps_ResultTupleSlot); @@ -187,6 +189,6 @@ ExecReScanUnique(UniqueState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } -- 2.25.1 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use outerPlanState macro instead of referring to leffttree 2022-04-14 06:49 Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> @ 2022-07-01 21:32 ` Tom Lane <[email protected]> 2022-07-06 09:41 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2022-07-01 21:32 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: [email protected] Richard Guo <[email protected]> writes: > In the executor code, we mix use outerPlanState macro and referring to > leffttree. Commit 40f42d2a tried to keep the code consistent by > replacing referring to lefftree with outerPlanState macro, but there are > still some outliers. This patch tries to clean them up. Seems generally reasonable, but what about righttree? I find a few of those too with "grep". Backing up a little bit, one thing not to like about the outerPlanState and innerPlanState macros is that they lose all semblance of type safety: #define innerPlanState(node) (((PlanState *)(node))->righttree) #define outerPlanState(node) (((PlanState *)(node))->lefttree) You can pass any pointer you want, and the compiler will not complain. I wonder if there's any trick (even a gcc-only one) that could improve on that. In the absence of such a check, people might feel that increasing our reliance on these macros isn't such a hot idea. Now, the typical coding pattern you've used: ExecReScanHash(HashState *node) { + PlanState *outerPlan = outerPlanState(node); is probably reasonably secure against wrong-pointer slip-ups. But I'm less convinced about that for in-line usages in the midst of a function, particularly in the common case that the function has a variable pointing to its Plan node as well as PlanState node. Would it make sense to try to use the local-variable style everywhere? regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use outerPlanState macro instead of referring to leffttree 2022-04-14 06:49 Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-01 21:32 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> @ 2022-07-06 09:41 ` Richard Guo <[email protected]> 2022-07-06 14:48 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Richard Guo @ 2022-07-06 09:41 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Pg Hackers <[email protected]> Thanks for reviewing this patch. On Sat, Jul 2, 2022 at 5:32 AM Tom Lane <[email protected]> wrote: > Richard Guo <[email protected]> writes: > > In the executor code, we mix use outerPlanState macro and referring to > > leffttree. Commit 40f42d2a tried to keep the code consistent by > > replacing referring to lefftree with outerPlanState macro, but there are > > still some outliers. This patch tries to clean them up. > > Seems generally reasonable, but what about righttree? I find a few > of those too with "grep". > Yes. We may do the same trick for righttree. > > Backing up a little bit, one thing not to like about the outerPlanState > and innerPlanState macros is that they lose all semblance of type > safety: > > #define innerPlanState(node) (((PlanState *)(node))->righttree) > #define outerPlanState(node) (((PlanState *)(node))->lefttree) > > You can pass any pointer you want, and the compiler will not complain. > I wonder if there's any trick (even a gcc-only one) that could improve > on that. In the absence of such a check, people might feel that > increasing our reliance on these macros isn't such a hot idea. > Your concern makes sense. I think outerPlan and innerPlan macros share the same issue. Not sure if there is a way to do the type check. > > Now, the typical coding pattern you've used: > > ExecReScanHash(HashState *node) > { > + PlanState *outerPlan = outerPlanState(node); > > is probably reasonably secure against wrong-pointer slip-ups. But > I'm less convinced about that for in-line usages in the midst of > a function, particularly in the common case that the function has > a variable pointing to its Plan node as well as PlanState node. > Would it make sense to try to use the local-variable style everywhere? > Do you mean the pattern like below? outerPlanState(hashstate) = ExecInitNode(outerPlan(node), estate, eflags); It seems that this pattern is mostly used when initializing child nodes with ExecInitNode(), and most calls to ExecInitNode() are using this pattern as a convention. Not sure if it's better to change them to local-variable style. Thanks Richard ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use outerPlanState macro instead of referring to leffttree 2022-04-14 06:49 Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-01 21:32 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 2022-07-06 09:41 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> @ 2022-07-06 14:48 ` Tom Lane <[email protected]> 2022-07-07 06:59 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2022-07-06 14:48 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: Pg Hackers <[email protected]> Richard Guo <[email protected]> writes: > On Sat, Jul 2, 2022 at 5:32 AM Tom Lane <[email protected]> wrote: >> Backing up a little bit, one thing not to like about the outerPlanState >> and innerPlanState macros is that they lose all semblance of type >> safety: > Your concern makes sense. I think outerPlan and innerPlan macros share > the same issue. Not sure if there is a way to do the type check. Yeah, I don't know of one either. It needn't hold up this patch. >> Would it make sense to try to use the local-variable style everywhere? > Do you mean the pattern like below? > outerPlanState(hashstate) = ExecInitNode(outerPlan(node), estate, eflags); > It seems that this pattern is mostly used when initializing child nodes > with ExecInitNode(), and most calls to ExecInitNode() are using this > pattern as a convention. Not sure if it's better to change them to > local-variable style. That's probably fine, especially if it's a commonly used pattern. Typically, if one applies outerPlan() or outerPlanState() to the wrong pointer, the mistake will become obvious upon even minimal testing. My concern here is more about usages in edge cases that perhaps escape testing, for instance in the arguments of an elog() for some nearly-can't-happen case. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use outerPlanState macro instead of referring to leffttree 2022-04-14 06:49 Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-01 21:32 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 2022-07-06 09:41 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-06 14:48 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> @ 2022-07-07 06:59 ` Richard Guo <[email protected]> 2022-07-07 15:25 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Richard Guo @ 2022-07-07 06:59 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Pg Hackers <[email protected]> On Wed, Jul 6, 2022 at 10:48 PM Tom Lane <[email protected]> wrote: > Typically, if one applies outerPlan() or outerPlanState() to the > wrong pointer, the mistake will become obvious upon even minimal > testing. My concern here is more about usages in edge cases that > perhaps escape testing, for instance in the arguments of an > elog() for some nearly-can't-happen case. Yeah, concur with that. For edge case usages maybe we can use the local-variable style to avoid wrong-pointer mistakes. Update the patch to include changes about righttree. But this doesn't include changes for edge case usages. (A rough look through shows to me that the current usages should be able to be covered by tests.) Thanks Richard Attachments: [application/octet-stream] v2-0001-Use-outerPlanState-innerPlanState-macro.patch (10.4K, ../../CAMbWs483RVxexNQxztX0=t_qEhnWbSTN3vXB9RnxoHiuFak_XQ@mail.gmail.com/3-v2-0001-Use-outerPlanState-innerPlanState-macro.patch) download | inline diff: From 3d81d088ef3a4b7f90734f4f08122156a4e78a7b Mon Sep 17 00:00:00 2001 From: pgsql-guo <[email protected]> Date: Thu, 7 Jul 2022 06:34:49 +0000 Subject: [PATCH v2] Use outerPlanState/innerPlanState macro --- src/backend/executor/nodeGather.c | 4 ++-- src/backend/executor/nodeGatherMerge.c | 4 ++-- src/backend/executor/nodeHash.c | 6 ++++-- src/backend/executor/nodeHashjoin.c | 15 +++++++++------ src/backend/executor/nodeLimit.c | 6 ++++-- src/backend/executor/nodeLockRows.c | 6 ++++-- src/backend/executor/nodeMergejoin.c | 11 +++++++---- src/backend/executor/nodeProjectSet.c | 6 ++++-- src/backend/executor/nodeResult.c | 7 ++++--- src/backend/executor/nodeSetOp.c | 8 +++++--- src/backend/executor/nodeUnique.c | 6 ++++-- 11 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c index 4f8a17df7d..1283d5b737 100644 --- a/src/backend/executor/nodeGather.c +++ b/src/backend/executor/nodeGather.c @@ -168,13 +168,13 @@ ExecGather(PlanState *pstate) /* Initialize, or re-initialize, shared state needed by workers. */ if (!node->pei) - node->pei = ExecInitParallelPlan(node->ps.lefttree, + node->pei = ExecInitParallelPlan(outerPlanState(node), estate, gather->initParam, gather->num_workers, node->tuples_needed); else - ExecParallelReinitialize(node->ps.lefttree, + ExecParallelReinitialize(outerPlanState(node), node->pei, gather->initParam); diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c index a488cc6d8b..3b1007f352 100644 --- a/src/backend/executor/nodeGatherMerge.c +++ b/src/backend/executor/nodeGatherMerge.c @@ -212,13 +212,13 @@ ExecGatherMerge(PlanState *pstate) /* Initialize, or re-initialize, shared state needed by workers. */ if (!node->pei) - node->pei = ExecInitParallelPlan(node->ps.lefttree, + node->pei = ExecInitParallelPlan(outerPlanState(node), estate, gm->initParam, gm->num_workers, node->tuples_needed); else - ExecParallelReinitialize(node->ps.lefttree, + ExecParallelReinitialize(outerPlanState(node), node->pei, gm->initParam); diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 3510a4247c..123079c16c 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -2212,12 +2212,14 @@ ExecHashTableResetMatchFlags(HashJoinTable hashtable) void ExecReScanHash(HashState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index 88b870655e..87403e2478 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -1290,6 +1290,9 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate, void ExecReScanHashJoin(HashJoinState *node) { + PlanState *outerPlan = outerPlanState(node); + PlanState *innerPlan = innerPlanState(node); + /* * In a multi-batch join, we currently have to do rescans the hard way, * primarily because batch temp files may have already been released. But @@ -1300,7 +1303,7 @@ ExecReScanHashJoin(HashJoinState *node) if (node->hj_HashTable != NULL) { if (node->hj_HashTable->nbatch == 1 && - node->js.ps.righttree->chgParam == NULL) + innerPlan->chgParam == NULL) { /* * Okay to reuse the hash table; needn't rescan inner, either. @@ -1328,7 +1331,7 @@ ExecReScanHashJoin(HashJoinState *node) else { /* must destroy and rebuild hash table */ - HashState *hashNode = castNode(HashState, innerPlanState(node)); + HashState *hashNode = castNode(HashState, innerPlan); Assert(hashNode->hashtable == node->hj_HashTable); /* accumulate stats from old hash table, if wanted */ @@ -1350,8 +1353,8 @@ ExecReScanHashJoin(HashJoinState *node) * if chgParam of subnode is not null then plan will be re-scanned * by first ExecProcNode. */ - if (node->js.ps.righttree->chgParam == NULL) - ExecReScan(node->js.ps.righttree); + if (innerPlan->chgParam == NULL) + ExecReScan(innerPlan); } } @@ -1368,8 +1371,8 @@ ExecReScanHashJoin(HashJoinState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->js.ps.lefttree->chgParam == NULL) - ExecReScan(node->js.ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } void diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c index 1b91b123fa..75e4302b94 100644 --- a/src/backend/executor/nodeLimit.c +++ b/src/backend/executor/nodeLimit.c @@ -542,6 +542,8 @@ ExecEndLimit(LimitState *node) void ExecReScanLimit(LimitState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * Recompute limit/offset in case parameters changed, and reset the state * machine. We must do this before rescanning our child node, in case @@ -553,6 +555,6 @@ ExecReScanLimit(LimitState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 1a9dab25dd..a74813c7aa 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -394,10 +394,12 @@ ExecEndLockRows(LockRowsState *node) void ExecReScanLockRows(LockRowsState *node) { + PlanState *outerPlan = outerPlanState(node); + /* * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c index 864e3baf86..fed345eae5 100644 --- a/src/backend/executor/nodeMergejoin.c +++ b/src/backend/executor/nodeMergejoin.c @@ -1658,6 +1658,9 @@ ExecEndMergeJoin(MergeJoinState *node) void ExecReScanMergeJoin(MergeJoinState *node) { + PlanState *outerPlan = outerPlanState(node); + PlanState *innerPlan = innerPlanState(node); + ExecClearTuple(node->mj_MarkedTupleSlot); node->mj_JoinState = EXEC_MJ_INITIALIZE_OUTER; @@ -1670,8 +1673,8 @@ ExecReScanMergeJoin(MergeJoinState *node) * if chgParam of subnodes is not null then plans will be re-scanned by * first ExecProcNode. */ - if (node->js.ps.lefttree->chgParam == NULL) - ExecReScan(node->js.ps.lefttree); - if (node->js.ps.righttree->chgParam == NULL) - ExecReScan(node->js.ps.righttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); + if (innerPlan->chgParam == NULL) + ExecReScan(innerPlan); } diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c index ea40d61b0b..adabfa2882 100644 --- a/src/backend/executor/nodeProjectSet.c +++ b/src/backend/executor/nodeProjectSet.c @@ -339,6 +339,8 @@ ExecEndProjectSet(ProjectSetState *node) void ExecReScanProjectSet(ProjectSetState *node) { + PlanState *outerPlan = outerPlanState(node); + /* Forget any incompletely-evaluated SRFs */ node->pending_srf_tuples = false; @@ -346,6 +348,6 @@ ExecReScanProjectSet(ProjectSetState *node) * If chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeResult.c b/src/backend/executor/nodeResult.c index d0413e05de..78a744ba85 100644 --- a/src/backend/executor/nodeResult.c +++ b/src/backend/executor/nodeResult.c @@ -259,6 +259,8 @@ ExecEndResult(ResultState *node) void ExecReScanResult(ResultState *node) { + PlanState *outerPlan = outerPlanState(node); + node->rs_done = false; node->rs_checkqual = (node->resconstantqual != NULL); @@ -266,7 +268,6 @@ ExecReScanResult(ResultState *node) * If chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree && - node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan && outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c index 4b428cfa39..8af12f96e7 100644 --- a/src/backend/executor/nodeSetOp.c +++ b/src/backend/executor/nodeSetOp.c @@ -597,6 +597,8 @@ ExecEndSetOp(SetOpState *node) void ExecReScanSetOp(SetOpState *node) { + PlanState *outerPlan = outerPlanState(node); + ExecClearTuple(node->ps.ps_ResultTupleSlot); node->setop_done = false; node->numOutput = 0; @@ -617,7 +619,7 @@ ExecReScanSetOp(SetOpState *node) * parameter changes, then we can just rescan the existing hash table; * no need to build it again. */ - if (node->ps.lefttree->chgParam == NULL) + if (outerPlan->chgParam == NULL) { ResetTupleHashIterator(node->hashtable, &node->hashiter); return; @@ -646,6 +648,6 @@ ExecReScanSetOp(SetOpState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } diff --git a/src/backend/executor/nodeUnique.c b/src/backend/executor/nodeUnique.c index 6c99d13a39..d1399feabb 100644 --- a/src/backend/executor/nodeUnique.c +++ b/src/backend/executor/nodeUnique.c @@ -180,6 +180,8 @@ ExecEndUnique(UniqueState *node) void ExecReScanUnique(UniqueState *node) { + PlanState *outerPlan = outerPlanState(node); + /* must clear result tuple so first input tuple is returned */ ExecClearTuple(node->ps.ps_ResultTupleSlot); @@ -187,6 +189,6 @@ ExecReScanUnique(UniqueState *node) * if chgParam of subnode is not null then plan will be re-scanned by * first ExecProcNode. */ - if (node->ps.lefttree->chgParam == NULL) - ExecReScan(node->ps.lefttree); + if (outerPlan->chgParam == NULL) + ExecReScan(outerPlan); } -- 2.25.1 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use outerPlanState macro instead of referring to leffttree 2022-04-14 06:49 Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-01 21:32 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 2022-07-06 09:41 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-06 14:48 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 2022-07-07 06:59 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> @ 2022-07-07 15:25 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2022-07-07 15:25 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: Pg Hackers <[email protected]> Richard Guo <[email protected]> writes: > Update the patch to include changes about righttree. But this doesn't > include changes for edge case usages. (A rough look through shows to me > that the current usages should be able to be covered by tests.) I found a couple other places by grepping, and adjusted those too. Pushed. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2022-07-07 15:25 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-04-12 03:57 [PATCH] remove deprecated v8.2 containment operators Justin Pryzby <[email protected]> 2022-04-14 06:49 Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-01 21:32 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 2022-07-06 09:41 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-06 14:48 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]> 2022-07-07 06:59 ` Re: Use outerPlanState macro instead of referring to leffttree Richard Guo <[email protected]> 2022-07-07 15:25 ` Re: Use outerPlanState macro instead of referring to leffttree Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox