Horton

Thread

Commits

  1. Use lfirst_node() and linitial_node() where appropriate in planner.c.

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

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-07-13T05:27:42Z

    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

  2. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-07-13T15:45:01Z

    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

  3. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-07-14T08:55:11Z

    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

  4. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2017-09-05T10:32:57Z

    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

  5. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-09-05T11:57:20Z

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

  6. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-05T20:02:14Z

    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

  7. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-09-06T05:19:21Z

    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

  8. Re: Replacing lfirst() with lfirst_node() appropriately in planner.c

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-09-06T09:07:29Z

    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