Обсуждение: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

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

On 2015/07/10 21:59, David Rowley wrote:
> On 10 July 2015 at 21:40, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp
> <mailto:fujita.etsuro@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

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.etsuro@lab.ntt.co.jp
>> <mailto:fujita.etsuro@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

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.170000
> 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.170000
> 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.780000
> 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

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

On Wed, Sep 9, 2015 at 7:08 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 7:08 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> 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

On 2015/12/23 2:47, Robert Haas wrote:
> On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
> <michael.paquier@gmail.com> 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

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, &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;
>   }

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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, &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;
>>    }
>
> 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

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;
>>> !         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;
>>>    }

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

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;
>>>> !         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;
>>>>    }

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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Jan 21, 2016 at 5:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

On 2016/01/28 12:13, Robert Haas wrote:
> On Thu, Jan 21, 2016 at 5:55 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> 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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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