Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Alvaro Herrera]

Etsuro Fujita wrote:

> Done.  Attached is an updated version of the patch.

Pushed, thanks.

I kinda wonder why this struct member has a name that doesn't match the
naming convention in the rest of the struct, and also why isn't it
documented in the comment above the struct definition.  But that's not
this patch's fault.

-- 
Álvaro Herrerahttp://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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/02/03 3:31, Alvaro Herrera wrote:

Etsuro Fujita wrote:


Done.  Attached is an updated version of the patch.


Pushed, thanks.


Thank you!


I kinda wonder why this struct member has a name that doesn't match the
naming convention in the rest of the struct, and also why isn't it
documented in the comment above the struct definition.  But that's not
this patch's fault.


I think so, too.

Best regards,
Etsuro Fujita




--
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Alvaro Herrera]

Etsuro Fujita wrote:
> On 2016/01/28 12:13, Robert Haas wrote:

> >I don't think this is a good idea.  Most of the time, no system
> >columns will be present, and we'll just be scanning the Bitmapset
> >twice rather than once.  Sure, that doesn't take many extra cycles,
> >but if the point of all this is to micro-optimize this code, that
> >particular change is going in the wrong direction.
> 
> I thought that is a good idea, considering the additional overhead is
> little, because that would be useful for some use-cases.  But I think we
> need more discussions about that, so if there are no objections from Alvaro
> (or anyone), I'll leave that part as-is.

I'm happy to defer.

-- 
Álvaro Herrerahttp://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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/28 18:15, Alvaro Herrera wrote:

Etsuro Fujita wrote:

On 2016/01/28 12:13, Robert Haas wrote:



I don't think this is a good idea.  Most of the time, no system
columns will be present, and we'll just be scanning the Bitmapset
twice rather than once.  Sure, that doesn't take many extra cycles,
but if the point of all this is to micro-optimize this code, that
particular change is going in the wrong direction.



I thought that is a good idea, considering the additional overhead is
little, because that would be useful for some use-cases.  But I think we
need more discussions about that, so if there are no objections from Alvaro
(or anyone), I'll leave that part as-is.



I'm happy to defer.


Done.  Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2099,2108  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
  	Plan	   *outer_plan = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2099,2105 
***
*** 2171,2206  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2168,2215 
  	}
  
  	/*
! 	 * If rel is a base relation, detect whether any system columns are
! 	 * requested from the rel.  (If rel is a join relation, rel->relid will be
! 	 * 0, but there can be no Var with relid 0 in the reltargetlist or the
! 	 * restriction clauses, so we skip this in that case.  Note that any such
! 	 * columns in base relations that were joined are assumed to be contained
! 	 * in fdw_scan_tlist.)  This is a bit of a kluge and might go away someday,
! 	 * so we intentionally leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel->baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo->clause, scan_relid, _used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan->fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Robert Haas]

On Thu, Jan 21, 2016 at 5:55 AM, Etsuro Fujita
 wrote:
> On 2016/01/21 7:04, Alvaro Herrera wrote:
>> Etsuro Fujita wrote:
>>>
>>> On second thought, I noticed that detecting whether we see a system
>>> column
>>> that way needs more cycles in cases where the reltargetlist and the
>>> restriction clauses don't contain any system columns.  ISTM that such
>>> cases
>>> are rather common, so I'm inclined to keep that code as-is.
>
>> Ah, I see now what you did there. I was thinking you'd have the
>> foreach(restrictinfo) loop, then once the loop is complete scan the
>> bitmapset; not scan the bitmap set scan inside the other loop.
>
> Ah, I got to the point.  I think that is a good idea.  The additional cycles
> for the worst case are bounded and negligible.  Please find attached an
> updated patch.

I don't think this is a good idea.  Most of the time, no system
columns will be present, and we'll just be scanning the Bitmapset
twice rather than once.  Sure, that doesn't take many extra cycles,
but if the point of all this is to micro-optimize this code, that
particular change is going in the wrong direction.

-- 
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

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/28 12:13, Robert Haas wrote:

On Thu, Jan 21, 2016 at 5:55 AM, Etsuro Fujita
 wrote:

On 2016/01/21 7:04, Alvaro Herrera wrote:

Etsuro Fujita wrote:


On second thought, I noticed that detecting whether we see a system
column
that way needs more cycles in cases where the reltargetlist and the
restriction clauses don't contain any system columns.  ISTM that such
cases
are rather common, so I'm inclined to keep that code as-is.



Ah, I see now what you did there. I was thinking you'd have the
foreach(restrictinfo) loop, then once the loop is complete scan the
bitmapset; not scan the bitmap set scan inside the other loop.



Ah, I got to the point.  I think that is a good idea.  The additional cycles
for the worst case are bounded and negligible.  Please find attached an
updated patch.



I don't think this is a good idea.  Most of the time, no system
columns will be present, and we'll just be scanning the Bitmapset
twice rather than once.  Sure, that doesn't take many extra cycles,
but if the point of all this is to micro-optimize this code, that
particular change is going in the wrong direction.


I thought that is a good idea, considering the additional overhead is 
little, because that would be useful for some use-cases.  But I think we 
need more discussions about that, so if there are no objections from 
Alvaro (or anyone), I'll leave that part as-is.


Best regards,
Etsuro Fujita




--
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/21 7:04, Alvaro Herrera wrote:

Etsuro Fujita wrote:

On second thought, I noticed that detecting whether we see a system column
that way needs more cycles in cases where the reltargetlist and the
restriction clauses don't contain any system columns.  ISTM that such cases
are rather common, so I'm inclined to keep that code as-is.



Ah, I see now what you did there. I was thinking you'd have the
foreach(restrictinfo) loop, then once the loop is complete scan the
bitmapset; not scan the bitmap set scan inside the other loop.


Ah, I got to the point.  I think that is a good idea.  The additional 
cycles for the worst case are bounded and negligible.  Please find 
attached an updated patch.


Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2099,2108  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
  	Plan	   *outer_plan = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2099,2105 
***
*** 2171,2206  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2168,2228 
  	}
  
  	/*
! 	 * If rel is a base relation, detect whether any system columns are
! 	 * requested from the rel.  (If rel is a join relation, rel->relid will be
! 	 * 0, but there can be no Var with relid 0 in the reltargetlist or the
! 	 * restriction clauses, so we skip this in that case.  Note that any such
! 	 * columns in base relations that were joined are assumed to be contained
! 	 * in fdw_scan_tlist.)  This is a bit of a kluge and might go away someday,
! 	 * so we intentionally leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
  		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan->fsSystemCol = true;
! break;
! 			}
  		}
  
! 		/* Examine all the attributes used by restriction clauses, if needed */
! 		if (!scan_plan->fsSystemCol)
! 		{
! 			foreach(lc, rel->baserestrictinfo)
! 			{
! RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! pull_varattnos((Node *) rinfo->clause, scan_relid, _used);
! 			}
! 
! 			/* Now, are any system columns requested from rel? */
! 			for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 			{
! if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! {
! 	scan_plan->fsSystemCol = true;
! 	break;
! }
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Alvaro Herrera]

Etsuro Fujita wrote:

> On second thought, I noticed that detecting whether we see a system column
> that way needs more cycles in cases where the reltargetlist and the
> restriction clauses don't contain any system columns.  ISTM that such cases
> are rather common, so I'm inclined to keep that code as-is.

Ah, I see now what you did there. I was thinking you'd have the
foreach(restrictinfo) loop, then once the loop is complete scan the
bitmapset; not scan the bitmap set scan inside the other loop.

-- 
Álvaro Herrerahttp://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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/15 19:00, Etsuro Fujita wrote:

On 2016/01/12 18:00, Etsuro Fujita wrote:

On 2016/01/12 2:36, Alvaro Herrera wrote:

I wonder,



--- 2166,2213 
   }

   /*
!  * If rel is a base relation, detect whether any system columns
are
!  * requested from the rel.  (If rel is a join relation,
rel->relid will be
!  * 0, but there can be no Var in the target list with relid 0,
so we skip
!  * this in that case.  Note that any such system columns are
assumed to be
!  * contained in fdw_scan_tlist, so we never need fsSystemCol to
be true in
!  * the joinrel case.)  This is a bit of a kluge and might go
away someday,
!  * so we intentionally leave it out of the API presented to FDWs.
*/
! scan_plan->fsSystemCol = false;
! if (scan_relid > 0)
   {
! Bitmapset  *attrs_used = NULL;
! ListCell   *lc;
! inti;

! /*
!  * First, examine all the attributes needed for joins or
final output.
!  * Note: we must look at reltargetlist, not the attr_needed
data,
!  * because attr_needed isn't computed for inheritance child
rels.
!  */
! pull_varattnos((Node *) rel->reltargetlist, scan_relid,
_used);

! /* Add all the attributes used by restriction clauses. */
! foreach(lc, rel->baserestrictinfo)
   {
! RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
!
! pull_varattnos((Node *) rinfo->clause, scan_relid,
_used);
   }

! /* Now, are any system columns requested from rel? */
! for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! {
! if (bms_is_member(i -
FirstLowInvalidHeapAttributeNumber, attrs_used))
! {
! scan_plan->fsSystemCol = true;
! break;
! }
! }
!
! bms_free(attrs_used);
! }

   return scan_plan;
   }



Would it make sense to call pull_varattnos(reltargetlist), then walk the
bitmapset and break if we see a system column, then call
pull_varattnos() on the rinfo->clause?  That way, if the targetlist
request a system column we don't have to walk the RestrictInfos.



Seems like a good idea.  Will update the patch.



Done.  Attached is an updated version of the patch.


On second thought, I noticed that detecting whether we see a system 
column that way needs more cycles in cases where the reltargetlist and 
the restriction clauses don't contain any system columns.  ISTM that 
such cases are rather common, so I'm inclined to keep that code as-is.


Best regards,
Etsuro Fujita




--
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/12 18:00, Etsuro Fujita wrote:

On 2016/01/12 2:36, Alvaro Herrera wrote:

I wonder,



--- 2166,2213 
   }

   /*
!  * If rel is a base relation, detect whether any system columns
are
!  * requested from the rel.  (If rel is a join relation,
rel->relid will be
!  * 0, but there can be no Var in the target list with relid 0,
so we skip
!  * this in that case.  Note that any such system columns are
assumed to be
!  * contained in fdw_scan_tlist, so we never need fsSystemCol to
be true in
!  * the joinrel case.)  This is a bit of a kluge and might go
away someday,
!  * so we intentionally leave it out of the API presented to FDWs.
*/
! scan_plan->fsSystemCol = false;
! if (scan_relid > 0)
   {
! Bitmapset  *attrs_used = NULL;
! ListCell   *lc;
! inti;

! /*
!  * First, examine all the attributes needed for joins or
final output.
!  * Note: we must look at reltargetlist, not the attr_needed
data,
!  * because attr_needed isn't computed for inheritance child
rels.
!  */
! pull_varattnos((Node *) rel->reltargetlist, scan_relid,
_used);

! /* Add all the attributes used by restriction clauses. */
! foreach(lc, rel->baserestrictinfo)
   {
! RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
!
! pull_varattnos((Node *) rinfo->clause, scan_relid,
_used);
   }

! /* Now, are any system columns requested from rel? */
! for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! {
! if (bms_is_member(i -
FirstLowInvalidHeapAttributeNumber, attrs_used))
! {
! scan_plan->fsSystemCol = true;
! break;
! }
! }
!
! bms_free(attrs_used);
! }

   return scan_plan;
   }



Would it make sense to call pull_varattnos(reltargetlist), then walk the
bitmapset and break if we see a system column, then call
pull_varattnos() on the rinfo->clause?  That way, if the targetlist
request a system column we don't have to walk the RestrictInfos.



Seems like a good idea.  Will update the patch.


Done.  Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2097,2106  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
  	Plan	   *outer_plan = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2097,2103 
***
*** 2169,2204  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2166,2229 
  	}
  
  	/*
! 	 * If rel is a base relation, detect whether any system columns are
! 	 * requested from the rel.  (If rel is a join relation, rel->relid will be
! 	 * 0, but there can be no Var with relid 0 in the reltargetlist or the
! 	 * restriction clauses, so we skip this in that case.  Note that any such
! 	 * columns in base relations that were joined are assumed to be contained
! 	 * in fdw_scan_tlist.)  This is a bit of a kluge and might go away someday,
! 	 * so we intentionally leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Are any system columns 

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/12 2:36, Alvaro Herrera wrote:

I wonder,


--- 2166,2213 
}

/*
!* If rel is a base relation, detect whether any system columns are
!* requested from the rel.  (If rel is a join relation, rel->relid will 
be
!* 0, but there can be no Var in the target list with relid 0, so we 
skip
!* this in that case.  Note that any such system columns are assumed to 
be
!* contained in fdw_scan_tlist, so we never need fsSystemCol to be true 
in
!* the joinrel case.)  This is a bit of a kluge and might go away 
someday,
!* so we intentionally leave it out of the API presented to FDWs.
 */
!   scan_plan->fsSystemCol = false;
!   if (scan_relid > 0)
{
!   Bitmapset  *attrs_used = NULL;
!   ListCell   *lc;
!   int i;

!   /*
!* First, examine all the attributes needed for joins or final 
output.
!* Note: we must look at reltargetlist, not the attr_needed 
data,
!* because attr_needed isn't computed for inheritance child 
rels.
!*/
!   pull_varattnos((Node *) rel->reltargetlist, scan_relid, 
_used);

!   /* Add all the attributes used by restriction clauses. */
!   foreach(lc, rel->baserestrictinfo)
{
!   RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
!
!   pull_varattnos((Node *) rinfo->clause, scan_relid, 
_used);
}

!   /* Now, are any system columns requested from rel? */
!   for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
!   {
!   if (bms_is_member(i - 
FirstLowInvalidHeapAttributeNumber, attrs_used))
!   {
!   scan_plan->fsSystemCol = true;
!   break;
!   }
!   }
!
!   bms_free(attrs_used);
!   }

return scan_plan;
   }


Would it make sense to call pull_varattnos(reltargetlist), then walk the
bitmapset and break if we see a system column, then call
pull_varattnos() on the rinfo->clause?  That way, if the targetlist
request a system column we don't have to walk the RestrictInfos.


Seems like a good idea.  Will update the patch.

Best regards,
Etsuro Fujita




--
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Alvaro Herrera]

I wonder,

> --- 2166,2213 
>   }
>   
>   /*
> !  * If rel is a base relation, detect whether any system columns are
> !  * requested from the rel.  (If rel is a join relation, rel->relid will 
> be
> !  * 0, but there can be no Var in the target list with relid 0, so we 
> skip
> !  * this in that case.  Note that any such system columns are assumed to 
> be
> !  * contained in fdw_scan_tlist, so we never need fsSystemCol to be true 
> in
> !  * the joinrel case.)  This is a bit of a kluge and might go away 
> someday,
> !  * so we intentionally leave it out of the API presented to FDWs.
>*/
> ! scan_plan->fsSystemCol = false;
> ! if (scan_relid > 0)
>   {
> ! Bitmapset  *attrs_used = NULL;
> ! ListCell   *lc;
> ! int i;
>   
> ! /*
> !  * First, examine all the attributes needed for joins or final 
> output.
> !  * Note: we must look at reltargetlist, not the attr_needed 
> data,
> !  * because attr_needed isn't computed for inheritance child 
> rels.
> !  */
> ! pull_varattnos((Node *) rel->reltargetlist, scan_relid, 
> _used);
>   
> ! /* Add all the attributes used by restriction clauses. */
> ! foreach(lc, rel->baserestrictinfo)
>   {
> ! RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
> ! 
> ! pull_varattnos((Node *) rinfo->clause, scan_relid, 
> _used);
>   }
>   
> ! /* Now, are any system columns requested from rel? */
> ! for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
> ! {
> ! if (bms_is_member(i - 
> FirstLowInvalidHeapAttributeNumber, attrs_used))
> ! {
> ! scan_plan->fsSystemCol = true;
> ! break;
> ! }
> ! }
> ! 
> ! bms_free(attrs_used);
> ! }
>   
>   return scan_plan;
>   }

Would it make sense to call pull_varattnos(reltargetlist), then walk the
bitmapset and break if we see a system column, then call
pull_varattnos() on the rinfo->clause?  That way, if the targetlist
request a system column we don't have to walk the RestrictInfos.

-- 
Álvaro Herrerahttp://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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2015/12/23 2:47, Robert Haas wrote:

On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
 wrote:

Moved to next CF because of a lack of reviews.


Thanks, Michael!


I just took a look at this.  I think the basic idea of this patch is
good, but the comments need some work, because they don't really
explain why this should be skipped in the join case.  Maybe something
like this:


Thanks for the review, Robert!


If rel is a base relation, detect whether any system columns were
requested.  (If rel is a join relation, rel->relid will be 0, but
there can be no Var in the target list with relid 0, so we skip this
in that case.) This is a bit of a kluge and might go away someday, so
we intentionally leave it out of the API presented to FDWs.
And the rest as it is currently written.


Agreed.


It might be good, also, to say something about why we never need
fsSystemCol to be true in the joinrel case.


+1 for that.  How about adding something like this:

Note that any such system columns are assumed to be contained in 
fdw_scan_tlist, so we never need fsSystemCol to be true in the joinrel case.


Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2097,2106  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
  	Plan	   *outer_plan = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2097,2103 
***
*** 2169,2204  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2166,2213 
  	}
  
  	/*
! 	 * If rel is a base relation, detect whether any system columns are
! 	 * requested from the rel.  (If rel is a join relation, rel->relid will be
! 	 * 0, but there can be no Var in the target list with relid 0, so we skip
! 	 * this in that case.  Note that any such system columns are assumed to be
! 	 * contained in fdw_scan_tlist, so we never need fsSystemCol to be true in
! 	 * the joinrel case.)  This is a bit of a kluge and might go away someday,
! 	 * so we intentionally leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel->baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo->clause, scan_relid, _used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan->fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Michael Paquier]

On Wed, Sep 9, 2015 at 7:08 PM, Etsuro Fujita
 wrote:
> On 2015/07/10 18:40, Etsuro Fujita wrote:
>> To save cycles, I modified create_foreignscan_plan so that it detects
>> whether any system columns are requested if scanning a base relation.
>> Also, I revised other code there a little bit.
>
> Attached is an updated version of the patch.  The previous version
> contained changes to ExecInitForeignScan, but I've dropped that part, as
> discussed before.

Moved to next CF because of a lack of reviews.
-- 
Michael


-- 
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Robert Haas]

On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
 wrote:
> On Wed, Sep 9, 2015 at 7:08 PM, Etsuro Fujita
>  wrote:
>> On 2015/07/10 18:40, Etsuro Fujita wrote:
>>> To save cycles, I modified create_foreignscan_plan so that it detects
>>> whether any system columns are requested if scanning a base relation.
>>> Also, I revised other code there a little bit.
>>
>> Attached is an updated version of the patch.  The previous version
>> contained changes to ExecInitForeignScan, but I've dropped that part, as
>> discussed before.
>
> Moved to next CF because of a lack of reviews.

I just took a look at this.  I think the basic idea of this patch is
good, but the comments need some work, because they don't really
explain why this should be skipped in the join case.  Maybe something
like this:

If rel is a base relation, detect whether any system columns were
requested.  (If rel is a join relation, rel->relid will be 0, but
there can be no Var in the target list with relid 0, so we skip this
in that case.) This is a bit of a kluge and might go away someday, so
we intentionally leave it out of the API presented to FDWs.

And the rest as it is currently written.

It might be good, also, to say something about why we never need
fsSystemCol to be true in the joinrel case.

-- 
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

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2015/07/10 18:40, Etsuro Fujita wrote:
> To save cycles, I modified create_foreignscan_plan so that it detects
> whether any system columns are requested if scanning a base relation.
> Also, I revised other code there a little bit.

Attached is an updated version of the patch.  The previous version
contained changes to ExecInitForeignScan, but I've dropped that part, as
discussed before.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2058,2066  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2058,2063 
***
*** 2120,2155  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2117,2161 
  	}
  
  	/*
! 	 * If we're scanning a base relation, detect whether any system columns
! 	 * are requested from the rel.  (Irrelevant if scanning a join relation.)
! 	 * This is a bit of a kluge and might go away someday, so we intentionally
! 	 * leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel->baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo->clause, scan_relid, _used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan->fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2015/08/30 13:06, David Rowley wrote:

It's likely not worth changing if there's cases when it'll be slower,
but curiosity got the better of me and I wondered how extreme a case it
would take to actually see a slowdown, and per my benchmark results the
first used column would have to be about attnum 500.



I used the attached to benchmark. has_system_columns is the current
method, has_system_columns2 has my changes. Lines are prefixed by the
position where the first (and only) attnum appears in the bitmap set.



1 has_system_columns complete in 1.196000
1 has_system_columns2 complete in 0.17
2 has_system_columns complete in 1.198000
2 has_system_columns2 complete in 0.167000
4 has_system_columns complete in 1.197000
4 has_system_columns2 complete in 0.17
8 has_system_columns complete in 1.206000
8 has_system_columns2 complete in 0.203000
16 has_system_columns complete in 1.202000
16 has_system_columns2 complete in 0.237000
32 has_system_columns complete in 1.206000
32 has_system_columns2 complete in 0.232000
64 has_system_columns complete in 1.207000
64 has_system_columns2 complete in 0.268000
128 has_system_columns complete in 1.205000
128 has_system_columns2 complete in 0.368000
256 has_system_columns complete in 1.203000
256 has_system_columns2 complete in 0.78
512 has_system_columns complete in 1.202000
512 has_system_columns2 complete in 1.302000
1024 has_system_columns complete in 1.199000
1024 has_system_columns2 complete in 3.539000



So, for what it's worth, could be 6 times faster for an "average" sized
table, but hey, we're talking nanoseconds anyway...


That's interesting.  But ISTM that that needs more discussion, so I'd 
like to leave the method as-is at least for now.


Thanks for the experiment!

Best regards,
Etsuro Fujita


--
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[David Rowley]

On 28 August 2015 at 22:20, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 On 2015/07/22 15:25, Etsuro Fujita wrote:

 On 2015/07/10 21:59, David Rowley wrote:

 I just glanced at this and noticed that the method for determining if
 there's any system columns could be made a bit nicer.


 /* Now, are any system columns requested from rel? */
 scan_plan-fsSystemCol = false;
 for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
 {
 if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
 {
 scan_plan-fsSystemCol = true;
 break;
 }
 }


 I think could just be written as:
 /* Now, are any system columns requested from rel? */
 if (!bms_is_empty(attrs_used) 
 bms_next_member(attrs_used, -1)  -FirstLowInvalidHeapAttributeNumber)
 scan_plan-fsSystemCol = true;
 else
 scan_plan-fsSystemCol = false;


 I know you didn't change this, but just thought I'd mention it while
 there's an opportunity to fix it.


 On second thought, I noticed that there is a case when that fix doesn't
 work well; bms_next_member wouldn't be efficient when only the rear
 user-columns are requested from a foreign table that has a large number of
 user-columns.  So, I'm inclined to leave that as-is.


You might be right here. I'd failed to think about that.

It's likely not worth changing if there's cases when it'll be slower, but
curiosity got the better of me and I wondered how extreme a case it would
take to actually see a slowdown, and per my benchmark results the first
used column would have to be about attnum 500.

I used the attached to benchmark. has_system_columns is the current method,
has_system_columns2 has my changes. Lines are prefixed by the position
where the first (and only) attnum appears in the bitmap set.

1 has_system_columns complete in 1.196000
1 has_system_columns2 complete in 0.17
2 has_system_columns complete in 1.198000
2 has_system_columns2 complete in 0.167000
4 has_system_columns complete in 1.197000
4 has_system_columns2 complete in 0.17
8 has_system_columns complete in 1.206000
8 has_system_columns2 complete in 0.203000
16 has_system_columns complete in 1.202000
16 has_system_columns2 complete in 0.237000
32 has_system_columns complete in 1.206000
32 has_system_columns2 complete in 0.232000
64 has_system_columns complete in 1.207000
64 has_system_columns2 complete in 0.268000
128 has_system_columns complete in 1.205000
128 has_system_columns2 complete in 0.368000
256 has_system_columns complete in 1.203000
256 has_system_columns2 complete in 0.78
512 has_system_columns complete in 1.202000
512 has_system_columns2 complete in 1.302000
1024 has_system_columns complete in 1.199000
1024 has_system_columns2 complete in 3.539000

So, for what it's worth, could be 6 times faster for an average sized
table, but hey, we're talking nanoseconds anyway...

Regards

David Rowley
--
 David Rowley   http://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
#include stdio.h
#include time.h
#include stdlib.h
#include stddef.h

#define FirstLowInvalidHeapAttributeNumber		(-8)
#define FLEXIBLE_ARRAY_MEMBER 1

#define elog(elevel, s)  \
	do { \
		printf(%s\n, s); \
		exit(EXIT_FAILURE); \
	} while(0)


#define BITS_PER_BITMAPWORD 32
#define WORDNUM(x)	((x) / BITS_PER_BITMAPWORD)
#define BITNUM(x)	((x) % BITS_PER_BITMAPWORD)

#define BITMAPSET_SIZE(nwords)	\
	(offsetof(Bitmapset, words) + (nwords) * sizeof(bitmapword))

typedef unsigned int bitmapword;		/* must be an unsigned type */
typedef int signedbitmapword; /* must be the matching signed type */
typedef char bool;

#define true 1
#define false 0

typedef struct Bitmapset
{
	int			nwords;			/* number of words in array */
	bitmapword	words[FLEXIBLE_ARRAY_MEMBER];	/* really [nwords] */
} Bitmapset;

void *
palloc0(size_t size)
{
	void *p = malloc(size);
	if (!p)
		elog(ERROR, out of memory);
	memset(p, 0, size);
	return p;
}
/*
 * bms_is_member - is X a member of A?
 */
bool
bms_is_member(int x, const Bitmapset *a)
{
	int			wordnum,
bitnum;

	/* XXX better to just return false for x0 ? */
	if (x  0)
		elog(ERROR, negative bitmapset member not allowed);
	if (a == NULL)
		return false;
	wordnum = WORDNUM(x);
	bitnum = BITNUM(x);
	if (wordnum = a-nwords)
		return false;
	if ((a-words[wordnum]  ((bitmapword) 1  bitnum)) != 0)
		return true;
	return false;
}

static const unsigned char rightmost_one_pos[256] = {
	0, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	6, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	7, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
	4, 0, 1, 0, 2, 0, 1, 0, 3, 

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2015/07/22 15:25, Etsuro Fujita wrote:

On 2015/07/10 21:59, David Rowley wrote:

On 10 July 2015 at 21:40, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
mailto:fujita.ets...@lab.ntt.co.jp wrote:



To save cycles, I modified create_foreignscan_plan so that it detects
whether any system columns are requested if scanning a base relation.



I just glanced at this and noticed that the method for determining if
there's any system columns could be made a bit nicer.



/* Now, are any system columns requested from rel? */
scan_plan-fsSystemCol = false;
for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
{
if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
scan_plan-fsSystemCol = true;
break;
}
}



I think could just be written as:
/* Now, are any system columns requested from rel? */
if (!bms_is_empty(attrs_used) 
bms_next_member(attrs_used, -1)  -FirstLowInvalidHeapAttributeNumber)
scan_plan-fsSystemCol = true;
else
scan_plan-fsSystemCol = false;



I know you didn't change this, but just thought I'd mention it while
there's an opportunity to fix it.


On second thought, I noticed that there is a case when that fix doesn't 
work well; bms_next_member wouldn't be efficient when only the rear 
user-columns are requested from a foreign table that has a large number 
of user-columns.  So, I'm inclined to leave that as-is.


Anyway, I'll add this to the upcoming CF.

Best regards,
Etsuro Fujita


--
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2015/07/10 21:59, David Rowley wrote:

On 10 July 2015 at 21:40, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
mailto:fujita.ets...@lab.ntt.co.jp wrote:

To save cycles, I modified create_foreignscan_plan so that it detects
whether any system columns are requested if scanning a base relation.
Also, I revised other code there a little bit.

For ExecInitForeignScan, I simplified the code there to determine the
scan tuple type, whith seems to me complex beyound necessity.  Maybe
that might be nitpicking, though.


For the latter, I misunderstood that the latest foreign-join pushdown 
patch allows fdw_scan_tlist to be set to a targetlist even for simple 
foreign table scans.  Sorry for that, but I have a concern about that. 
I'd like to mention it in a new thread later.



I just glanced at this and noticed that the method for determining if
there's any system columns could be made a bit nicer.

/* Now, are any system columns requested from rel? */
scan_plan-fsSystemCol = false;
for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
{
if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
scan_plan-fsSystemCol = true;
break;
}
}

I think could just be written as:
/* Now, are any system columns requested from rel? */
if (!bms_is_empty(attrs_used) 
bms_next_member(attrs_used, -1)  -FirstLowInvalidHeapAttributeNumber)
scan_plan-fsSystemCol = true;
else
scan_plan-fsSystemCol = false;

I know you didn't change this, but just thought I'd mention it while
there's an opportunity to fix it.


Good catch!

Will update the patch (and drop the ExecInitForeignScan part from the 
patch).


Sorry for the delay.

Best regards,
Etsuro Fujita


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

[HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

To save cycles, I modified create_foreignscan_plan so that it detects
whether any system columns are requested if scanning a base relation.
Also, I revised other code there a little bit.

For ExecInitForeignScan, I simplified the code there to determine the
scan tuple type, whith seems to me complex beyound necessity.  Maybe
that might be nitpicking, though.

Best regards,
Etsuro Fujita
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***
*** 159,182  ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
  	}
  
  	/*
! 	 * Determine the scan tuple type.  If the FDW provided a targetlist
! 	 * describing the scan tuples, use that; else use base relation's rowtype.
  	 */
! 	if (node-fdw_scan_tlist != NIL || currentRelation == NULL)
  	{
  		TupleDesc	scan_tupdesc;
  
  		scan_tupdesc = ExecTypeFromTL(node-fdw_scan_tlist, false);
  		ExecAssignScanType(scanstate-ss, scan_tupdesc);
  		/* Node's targetlist will contain Vars with varno = INDEX_VAR */
  		tlistvarno = INDEX_VAR;
  	}
- 	else
- 	{
- 		ExecAssignScanType(scanstate-ss, RelationGetDescr(currentRelation));
- 		/* Node's targetlist will contain Vars with varno = scanrelid */
- 		tlistvarno = scanrelid;
- 	}
  
  	/*
  	 * Initialize result tuple type and projection info.
--- 159,183 
  	}
  
  	/*
! 	 * Determine the scan tuple type.
  	 */
! 	if (scanrelid  0)
! 	{
! 		/* Use base relation's rowtype */
! 		ExecAssignScanType(scanstate-ss, RelationGetDescr(currentRelation));
! 		/* Node's targetlist will contain Vars with varno = scanrelid */
! 		tlistvarno = scanrelid;
! 	}
! 	else
  	{
  		TupleDesc	scan_tupdesc;
  
+ 		/* Use targetlist provided by the FDW */
  		scan_tupdesc = ExecTypeFromTL(node-fdw_scan_tlist, false);
  		ExecAssignScanType(scanstate-ss, scan_tupdesc);
  		/* Node's targetlist will contain Vars with varno = INDEX_VAR */
  		tlistvarno = INDEX_VAR;
  	}
  
  	/*
  	 * Initialize result tuple type and projection info.
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2050,2058  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path-path.parent;
  	Index		scan_relid = rel-relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel-fdwroutine != NULL);
  
--- 2050,2055 
***
*** 2112,2147  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel-baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan-fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan-fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2109,2153 
  	}
  
  	/*
! 	 * If we're scanning a base relation, detect whether any system columns
! 	 * are requested from the rel.  (Irrelevant if scanning a join relation.)
! 	 * This is a bit of a kluge and might go away someday, so we intentionally
! 	 * leave it out of the API presented to FDWs.
  	 */
! 	scan_plan-fsSystemCol = false;
! 	if (scan_relid  0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel-reltargetlist, scan_relid, attrs_used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel-baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo-clause, scan_relid, attrs_used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan-fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

-- 
Sent via pgsql-hackers mailing 

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[David Rowley]

On 10 July 2015 at 21:40, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote:

 To save cycles, I modified create_foreignscan_plan so that it detects
 whether any system columns are requested if scanning a base relation.
 Also, I revised other code there a little bit.

 For ExecInitForeignScan, I simplified the code there to determine the
 scan tuple type, whith seems to me complex beyound necessity.  Maybe
 that might be nitpicking, though.


I just glanced at this and noticed that the method for determining if
there's any system columns could be made a bit nicer.

/* Now, are any system columns requested from rel? */
scan_plan-fsSystemCol = false;
for (i = FirstLowInvalidHeapAttributeNumber + 1; i  0; i++)
{
if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
scan_plan-fsSystemCol = true;
break;
}
}

I think could just be written as:
 /* Now, are any system columns requested from rel? */
if (!bms_is_empty(attrs_used) 
bms_next_member(attrs_used, -1)  -FirstLowInvalidHeapAttributeNumber)
scan_plan-fsSystemCol = true;
else
scan_plan-fsSystemCol = false;

I know you didn't change this, but just thought I'd mention it while
there's an opportunity to fix it.

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services