Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Alvaro Herrera]

Ashutosh Bapat wrote:
> On Wed, Sep 6, 2017 at 1:32 AM, Tom Lane  wrote:

> > BTW, I think we *could* use "lfirst_node(List, ...)" in cases where
> > we know the list is supposed to be a list of objects rather than ints
> > or Oids.  I didn't do anything about that observation, though.
> 
> IMO, it won't be apparent as to why some code uses lfirst_node(List,
> ...) and some code uses (List *) lfirst().

Yeah -- based on that argument, I too think we should leave those alone.

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Ashutosh Bapat]

On Wed, Sep 6, 2017 at 1:32 AM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> On Tue, Sep 5, 2017 at 4:02 PM, Jeevan Chalke
>>  wrote:
>>> Attached patch for replacing linitial() with linital_node() appropriately in
>>> planner.c
>
>> Ok. The patch looks good to me. It compiles and make check passes.
>> Here are both the patches rebased on the latest sources.
>
> LGTM, pushed.  I also changed a couple of places that left off any cast
> of lfirst, eg:
>
> -   List   *gset = lfirst(lc);
> +   List   *gset = (List *) lfirst(lc);
>
> While this isn't really harmful, it's not per prevailing style.

+1. Thanks.

>
> BTW, I think we *could* use "lfirst_node(List, ...)" in cases where
> we know the list is supposed to be a list of objects rather than ints
> or Oids.  I didn't do anything about that observation, though.
>

IMO, it won't be apparent as to why some code uses lfirst_node(List,
...) and some code uses (List *) lfirst().

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Tom Lane]

Ashutosh Bapat  writes:
> On Tue, Sep 5, 2017 at 4:02 PM, Jeevan Chalke
>  wrote:
>> Attached patch for replacing linitial() with linital_node() appropriately in
>> planner.c

> Ok. The patch looks good to me. It compiles and make check passes.
> Here are both the patches rebased on the latest sources.

LGTM, pushed.  I also changed a couple of places that left off any cast
of lfirst, eg:

-   List   *gset = lfirst(lc);
+   List   *gset = (List *) lfirst(lc);

While this isn't really harmful, it's not per prevailing style.

BTW, I think we *could* use "lfirst_node(List, ...)" in cases where
we know the list is supposed to be a list of objects rather than ints
or Oids.  I didn't do anything about that observation, though.

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

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Ashutosh Bapat]

On Tue, Sep 5, 2017 at 4:02 PM, Jeevan Chalke
 wrote:
> Hi,
>
> lfirst_node() changes are missing for List node type and I was thinking
> about adding those. But it looks like we cannot do so as List can be a
> T_List, T_IntList, or T_OidList. So I am OK with that.

Thanks for investigating the case of T_List.

>
> Apart from this, changes look good to me. Everything works fine.
>
> As we are now doing it for lfirst(), can we also do this for linitial()?
> I did not find any usage for lsecond(), lthird(), lfourth() and llast()
> though.
>
> Attached patch for replacing linitial() with linital_node() appropriately in
> planner.c
>
Ok. The patch looks good to me. It compiles and make check passes.
Here are both the patches rebased on the latest sources.

I am marking this commitfest entry as "ready for committer".
From 25ae53d7f72a54a03ec90206c7e5579a562a121c Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Tue, 5 Sep 2017 16:50:58 +0530
Subject: [PATCH 1/2] Use lfirst_node() instead of lfirst() wherever suitable
 in planner.c

Ashutosh Bapat, reviewed by Jeevan Chalke
---
 src/backend/optimizer/plan/planner.c |   94 +-
 1 file changed, 47 insertions(+), 47 deletions(-)

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 9662302..a1dd157 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -411,7 +411,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
 		forboth(lp, glob->subplans, lr, glob->subroots)
 		{
 			Plan	   *subplan = (Plan *) lfirst(lp);
-			PlannerInfo *subroot = (PlannerInfo *) lfirst(lr);
+			PlannerInfo *subroot = lfirst_node(PlannerInfo, lr);
 
 			SS_finalize_plan(subroot, subplan);
 		}
@@ -430,7 +430,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
 	forboth(lp, glob->subplans, lr, glob->subroots)
 	{
 		Plan	   *subplan = (Plan *) lfirst(lp);
-		PlannerInfo *subroot = (PlannerInfo *) lfirst(lr);
+		PlannerInfo *subroot = lfirst_node(PlannerInfo, lr);
 
 		lfirst(lp) = set_plan_references(subroot, subplan);
 	}
@@ -586,7 +586,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 	hasOuterJoins = false;
 	foreach(l, parse->rtable)
 	{
-		RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+		RangeTblEntry *rte = lfirst_node(RangeTblEntry, l);
 
 		if (rte->rtekind == RTE_JOIN)
 		{
@@ -643,7 +643,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 	newWithCheckOptions = NIL;
 	foreach(l, parse->withCheckOptions)
 	{
-		WithCheckOption *wco = (WithCheckOption *) lfirst(l);
+		WithCheckOption *wco = lfirst_node(WithCheckOption, l);
 
 		wco->qual = preprocess_expression(root, wco->qual,
 		  EXPRKIND_QUAL);
@@ -663,7 +663,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 
 	foreach(l, parse->windowClause)
 	{
-		WindowClause *wc = (WindowClause *) lfirst(l);
+		WindowClause *wc = lfirst_node(WindowClause, l);
 
 		/* partitionClause/orderClause are sort/group expressions */
 		wc->startOffset = preprocess_expression(root, wc->startOffset,
@@ -705,7 +705,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 	/* Also need to preprocess expressions within RTEs */
 	foreach(l, parse->rtable)
 	{
-		RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+		RangeTblEntry *rte = lfirst_node(RangeTblEntry, l);
 		int			kind;
 		ListCell   *lcsq;
 
@@ -1080,7 +1080,7 @@ inheritance_planner(PlannerInfo *root)
 	rti = 1;
 	foreach(lc, parse->rtable)
 	{
-		RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+		RangeTblEntry *rte = lfirst_node(RangeTblEntry, lc);
 
 		if (rte->rtekind == RTE_SUBQUERY)
 			subqueryRTindexes = bms_add_member(subqueryRTindexes, rti);
@@ -1102,7 +1102,7 @@ inheritance_planner(PlannerInfo *root)
 	{
 		foreach(lc, root->append_rel_list)
 		{
-			AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+			AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
 
 			if (bms_is_member(appinfo->parent_relid, subqueryRTindexes) ||
 bms_is_member(appinfo->child_relid, subqueryRTindexes) ||
@@ -1130,7 +1130,7 @@ inheritance_planner(PlannerInfo *root)
 	 */
 	foreach(lc, root->append_rel_list)
 	{
-		AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+		AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
 		PlannerInfo *subroot;
 		RangeTblEntry *child_rte;
 		RelOptInfo *sub_final_rel;
@@ -1192,7 +1192,7 @@ inheritance_planner(PlannerInfo *root)
 			subroot->append_rel_list = NIL;
 			foreach(lc2, root->append_rel_list)
 			{
-AppendRelInfo *appinfo2 = (AppendRelInfo *) lfirst(lc2);
+AppendRelInfo *appinfo2 = lfirst_node(AppendRelInfo, lc2);
 
 if (bms_is_member(appinfo2->child_relid, modifiableARIindexes))
 	appinfo2 = copyObject(appinfo2);
@@ -1227,7 +1227,7 @@ inheritance_planner(PlannerInfo *root)
 			rti = 1;
 			foreach(lr, parse->rtable)
 			{
-RangeTblEntry *rte = 

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Jeevan Chalke]

Hi,

lfirst_node() changes are missing for List node type and I was thinking
about adding those. But it looks like we cannot do so as List can be a
T_List, T_IntList, or T_OidList. So I am OK with that.

Apart from this, changes look good to me. Everything works fine.

As we are now doing it for lfirst(), can we also do this for linitial()?
I did not find any usage for lsecond(), lthird(), lfourth() and llast()
though.

Attached patch for replacing linitial() with linital_node() appropriately in
planner.c

Thanks

On Fri, Jul 14, 2017 at 2:25 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Thu, Jul 13, 2017 at 9:15 PM, Alvaro Herrera
>  wrote:
> > Ashutosh Bapat wrote:
> >
> >> Happened to stumble across some instances of lfirst() which could use
> >> lfirst_node() in planner.c. Here's patch which replaces calls to
> >> lfirst() extracting node pointers by lfirst_node() in planner.c.
> >
> > Sounds good.
> >
> >> Are we carrying out such replacements in master or this will be
> >> considered for v11?
> >
> > This is for pg11 definitely; please add to the open commitfest.
>
> Thanks. Added. https://commitfest.postgresql.org/14/1195/
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Jeevan Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index a1dd157..9115655 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1556,8 +1556,8 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 			/*--
 			  translator: %s is a SQL row locking clause such as FOR UPDATE */
 	 errmsg("%s is not allowed with UNION/INTERSECT/EXCEPT",
-			LCS_asString(((RowMarkClause *)
-		  linitial(parse->rowMarks))->strength;
+			LCS_asString(linitial_node(RowMarkClause,
+	   parse->rowMarks)->strength;
 
 		/*
 		 * Calculate pathkeys that represent result ordering requirements
@@ -1687,7 +1687,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		qp_extra.tlist = tlist;
 		qp_extra.activeWindows = activeWindows;
 		qp_extra.groupClause = (gset_data
-? (gset_data->rollups ? ((RollupData *) linitial(gset_data->rollups))->groupClause : NIL)
+? (gset_data->rollups ? linitial_node(RollupData, gset_data->rollups)->groupClause : NIL)
 : parse->groupClause);
 
 		/*
@@ -1757,25 +1757,25 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 			split_pathtarget_at_srfs(root, final_target, sort_input_target,
 	 _targets,
 	 _targets_contain_srfs);
-			final_target = (PathTarget *) linitial(final_targets);
+			final_target = linitial_node(PathTarget, final_targets);
 			Assert(!linitial_int(final_targets_contain_srfs));
 			/* likewise for sort_input_target vs. grouping_target */
 			split_pathtarget_at_srfs(root, sort_input_target, grouping_target,
 	 _input_targets,
 	 _input_targets_contain_srfs);
-			sort_input_target = (PathTarget *) linitial(sort_input_targets);
+			sort_input_target = linitial_node(PathTarget, sort_input_targets);
 			Assert(!linitial_int(sort_input_targets_contain_srfs));
 			/* likewise for grouping_target vs. scanjoin_target */
 			split_pathtarget_at_srfs(root, grouping_target, scanjoin_target,
 	 _targets,
 	 _targets_contain_srfs);
-			grouping_target = (PathTarget *) linitial(grouping_targets);
+			grouping_target = linitial_node(PathTarget, grouping_targets);
 			Assert(!linitial_int(grouping_targets_contain_srfs));
 			/* scanjoin_target will not have any SRFs precomputed for it */
 			split_pathtarget_at_srfs(root, scanjoin_target, NULL,
 	 _targets,
 	 _targets_contain_srfs);
-			scanjoin_target = (PathTarget *) linitial(scanjoin_targets);
+			scanjoin_target = linitial_node(PathTarget, scanjoin_targets);
 			Assert(!linitial_int(scanjoin_targets_contain_srfs));
 		}
 		else
@@ -2194,7 +2194,7 @@ preprocess_grouping_sets(PlannerInfo *root)
 		/*
 		 * Get the initial (and therefore largest) grouping set.
 		 */
-		gs = linitial(current_sets);
+		gs = linitial_node(GroupingSetData, current_sets);
 
 		/*
 		 * Order the groupClause appropriately.  If the first grouping set is
@@ -2344,8 +2344,8 @@ preprocess_rowmarks(PlannerInfo *root)
 		 * CTIDs invalid.  This is also checked at parse time, but that's
 		 * insufficient because of rule substitution, query pullup, etc.
 		 */
-		CheckSelectLocking(parse, ((RowMarkClause *)
-   linitial(parse->rowMarks))->strength);
+		CheckSelectLocking(parse, linitial_node(RowMarkClause,
+parse->rowMarks)->strength);
 	}
 	else
 	{
@@ 

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Ashutosh Bapat]

On Thu, Jul 13, 2017 at 9:15 PM, Alvaro Herrera
 wrote:
> Ashutosh Bapat wrote:
>
>> Happened to stumble across some instances of lfirst() which could use
>> lfirst_node() in planner.c. Here's patch which replaces calls to
>> lfirst() extracting node pointers by lfirst_node() in planner.c.
>
> Sounds good.
>
>> Are we carrying out such replacements in master or this will be
>> considered for v11?
>
> This is for pg11 definitely; please add to the open commitfest.

Thanks. Added. https://commitfest.postgresql.org/14/1195/
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Alvaro Herrera]

Ashutosh Bapat wrote:

> Happened to stumble across some instances of lfirst() which could use
> lfirst_node() in planner.c. Here's patch which replaces calls to
> lfirst() extracting node pointers by lfirst_node() in planner.c.

Sounds good.

> Are we carrying out such replacements in master or this will be
> considered for v11?

This is for pg11 definitely; please add to the open commitfest.

-- 
Álvaro Herrerahttps://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

[HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c

[Ashutosh Bapat]

Hi,

Happened to stumble across some instances of lfirst() which could use
lfirst_node() in planner.c. Here's patch which replaces calls to
lfirst() extracting node pointers by lfirst_node() in planner.c. I
have covered all the occurences of lfirst() except
1. those extract generic pointers like Path, Plan etc.
2. those extract any node as Aggref, Var and then check using IsA()
3. those extracting some pointers other than nodes.

"make check" runs without any failure.

Are we carrying out such replacements in master or this will be
considered for v11?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


pg_lfirst_node_planner_c.patch
Description: Binary data

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