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

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

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

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

On Thu, Jul 13, 2017 at 9:15 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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

On Tue, Sep 5, 2017 at 4:02 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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".

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

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> On Tue, Sep 5, 2017 at 4:02 PM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> 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

On Wed, Sep 6, 2017 at 1:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> On Tue, Sep 5, 2017 at 4:02 PM, Jeevan Chalke
>> <jeevan.chalke@enterprisedb.com> 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

Ashutosh Bapat wrote:
> On Wed, Sep 6, 2017 at 1:32 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services