Re: Incorrect comment in get_partition_dispatch_recurse

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Incorrect comment in get_partition_dispatch_recurse
Date: 2018-05-14 05:29:18
Message-ID: f41b1ff5-732f-c46f-e4d7-3a2868b8bef2@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi David.

On 2018/05/14 13:57, David Rowley wrote:
> I noticed that a comment in get_partition_dispatch_recurse claims that:
>
> "it contains the
> * leaf partition's position in the global list *leaf_part_oids minus 1"
>
> The "minus 1" part is incorrect. It simply just stores the 0-based
> index of the item in the list.

Hmm, while I agree that simply calling it "0-based index" might be better
for readers, what's there now doesn't sound incorrect to me; in the
adjacent code:

if (get_rel_relkind(partrelid) != RELKIND_PARTITIONED_TABLE)
{
*leaf_part_oids = lappend_oid(*leaf_part_oids, partrelid);
pd->indexes[i] = list_length(*leaf_part_oids) - 1;
}

If I call the value of list_length after adding an OID to the list the
OID's position in the list, we're storing into the indexes array exactly
what the existing comment says it is. Now, literally describing the code
in the adjacent comment is not a great documentation style, so I'm open to
revising it like your patch does. :)

> I was going to fix it by removing just
> that part, but instead, I ended up rewriting the whole thing.
>
> Patch attached.

Looks good to me, thanks.

Regards,
Amit

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-05-14 05:49:55 Temporary WAL segments files not cleaned up after an instance crash
Previous Message Chapman Flack 2018-05-14 05:12:47 SPI/backend equivalent of extended-query Describe(statement)?