Replacing lfirst() with lfirst_node() appropriately in planner.c

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

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

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

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

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.bapat@enterprisedb.com> wrote:

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

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

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

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

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

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

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

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

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