Incorrect comment in get_partition_dispatch_recurse

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

On 14 May 2018 at 17:29, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

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

Thanks for looking.

I wouldn't have complained if list_nth() accepted a 1-based index, but
it does not. So, indexes[] does not store the "position in the global
list *leaf_part_oids minus 1", it just stores the position in the
list.

I imagine it's only written this way due to the way you're obtaining
the index using list_length(*leaf_part_oids) - 1, but the fact you had
to subtract 1 there does not make it "position minus 1". That's just
how you get the position of the list item in a List.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2018/05/14 20:29, David Rowley wrote:

On 14 May 2018 at 17:29, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

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

Thanks for looking.

I wouldn't have complained if list_nth() accepted a 1-based index, but
it does not. So, indexes[] does not store the "position in the global
list *leaf_part_oids minus 1", it just stores the position in the
list.

I imagine it's only written this way due to the way you're obtaining
the index using list_length(*leaf_part_oids) - 1, but the fact you had
to subtract 1 there does not make it "position minus 1". That's just
how you get the position of the list item in a List.

Hmm, yes. I guess I had not bothered back then to confirm that the
pg_list.h Lists are in fact 0-based. That said, I wasn't thinking of the
physical implementation of lists when writing the comment, but probably of
a logical sequence, IOW, I had it all mixed up. Anyway, it's good that
we're getting rid of that misguided terminology and thank you for that.

Regards,
Amit

On Mon, May 14, 2018 at 12:57 AM, David Rowley
<david.rowley@2ndquadrant.com> 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. I was going to fix it by removing just
that part, but instead, I ended up rewriting the whole thing.

I think that's clearer. Committed with a few tweaks that are
hopefully improvements.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 17 May 2018 at 02:51, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, May 14, 2018 at 12:57 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

The "minus 1" part is incorrect. It simply just stores the 0-based
index of the item in the list. I was going to fix it by removing just
that part, but instead, I ended up rewriting the whole thing.

I think that's clearer. Committed with a few tweaks that are
hopefully improvements.

Thanks for committing. Although, I disagree with your tweak:

+ * 1-based index into the *pds list.

I think that's making the same mistake as the last comment did. You
think it's 1-based because the index is being set with list_length
rather than list_length - 1, but it can do that simply because the
item has not been added to the list yet.

Nothing converts this index back to 0-based;

RelationGetPartitionDispatchInfo builds the array from the list with:

i = 0;
foreach(lc, pdlist)
{
pd[i++] = lfirst(lc);
}

ExecFindPartition uses the pd array with:

parent = pd[-parent->indexes[cur_index]];

So if it was 1-based then we'd be off by one here.

Maybe we can clear up that confusion with

+ /*
+  * No need to subtract 1 to get the 0-based index as the item for this
+  * partitioned table has not been added to the list yet.
+  */
pd->indexes[i] = -list_length(*pds);

and just switch 1-based to 0-based in the new comment.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, May 16, 2018 at 2:28 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

Thanks for committing. Although, I disagree with your tweak:

+ * 1-based index into the *pds list.

I think that's making the same mistake as the last comment did. You
think it's 1-based because the index is being set with list_length
rather than list_length - 1, but it can do that simply because the
item has not been added to the list yet.

Uh, maybe I've got that wrong. We can say 0-based instead if that's
right. I just didn't want to say that in one case it was 0-based and
in the other case make no mention.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018/05/17 3:28, David Rowley wrote:

On 17 May 2018 at 02:51, Robert Haas <robertmhaas@gmail.com> wrote:

I think that's clearer. Committed with a few tweaks that are
hopefully improvements.

Thanks for committing. Although, I disagree with your tweak:

+ * 1-based index into the *pds list.

I think that's making the same mistake as the last comment did. You
think it's 1-based because the index is being set with list_length
rather than list_length - 1, but it can do that simply because the
item has not been added to the list yet.

Nothing converts this index back to 0-based;

RelationGetPartitionDispatchInfo builds the array from the list with:

i = 0;
foreach(lc, pdlist)
{
pd[i++] = lfirst(lc);
}

ExecFindPartition uses the pd array with:

parent = pd[-parent->indexes[cur_index]];

So if it was 1-based then we'd be off by one here.

That's right. Even those negative values in the pd->indexes are still
0-based, with the 0th entry being for the root table.

Maybe we can clear up that confusion with

+ /*
+  * No need to subtract 1 to get the 0-based index as the item for this
+  * partitioned table has not been added to the list yet.
+  */
pd->indexes[i] = -list_length(*pds);

and just switch 1-based to 0-based in the new comment.

Or maybe, change the comment to say that even the negative indexes are
0-based like you pointed out, *but* instead of updating the comment like
you suggest above, change the other index value assignment statement to
not subtract 1 from the list_length by switching order with the
accompanying lappend; like this:

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

Attached a patch.

Thanks,
Amit

On 17 May 2018 at 13:17, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Or maybe, change the comment to say that even the negative indexes are
0-based like you pointed out, *but* instead of updating the comment like
you suggest above, change the other index value assignment statement to
not subtract 1 from the list_length by switching order with the
accompanying lappend; like this:

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

That makes sense. It's probably less confusing that way.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, May 16, 2018 at 3:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, May 16, 2018 at 2:28 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

Thanks for committing. Although, I disagree with your tweak:

+ * 1-based index into the *pds list.

I think that's making the same mistake as the last comment did. You
think it's 1-based because the index is being set with list_length
rather than list_length - 1, but it can do that simply because the
item has not been added to the list yet.

Uh, maybe I've got that wrong. We can say 0-based instead if that's
right. I just didn't want to say that in one case it was 0-based and
in the other case make no mention.

Hang on, I can't be wrong (famous last words). If the negative
indexes were 0-based, that would mean that the first element of the
list was referenced by -0, which obviously can't be true, because 0 =
-0. In other words, we can't be using 0-based indexing for both the
positive and the negative values, because then 0 itself would be
ambiguous. It's got to be that -1 is the first element of the *pds
list, which means -- AFAICS, anyway -- that the way I phrased it is
correct.

Unless the indexing system actually can't reference the first element
of *pds, and -1 means the second element. But then I think we need a
more verbose explanation here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

Hang on, I can't be wrong (famous last words). If the negative
indexes were 0-based, that would mean that the first element of the
list was referenced by -0, which obviously can't be true, because 0 =
-0. In other words, we can't be using 0-based indexing for both the
positive and the negative values, because then 0 itself would be
ambiguous. It's got to be that -1 is the first element of the *pds
list, which means -- AFAICS, anyway -- that the way I phrased it is
correct.

Unless the indexing system actually can't reference the first element
of *pds, and -1 means the second element. But then I think we need a
more verbose explanation here.

Maybe what you need is a redesign. This convention seems impossibly
confusing and hence error-prone. What about using a separate bool to
indicate which list the index refers to?

regards, tom lane

On Thu, May 17, 2018 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Unless the indexing system actually can't reference the first element
of *pds, and -1 means the second element. But then I think we need a
more verbose explanation here.

First element in *pds list (and the array subsequently created from
it) contains the root table's entry. So, a -1 does mean the 2nd entry
in that list/array. A 0 in the indexes array always refers to a leaf
partition and hence an index into the array for leaf partitions.

Thanks,
Amit

On Thu, May 17, 2018 at 10:36 AM, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, May 17, 2018 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Unless the indexing system actually can't reference the first element
of *pds, and -1 means the second element. But then I think we need a
more verbose explanation here.

First element in *pds list (and the array subsequently created from
it) contains the root table's entry. So, a -1 does mean the 2nd entry
in that list/array. A 0 in the indexes array always refers to a leaf
partition and hence an index into the array for leaf partitions.

All right, so let's just say that explicitly. Maybe something like
the attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-May-17, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Hang on, I can't be wrong (famous last words). If the negative
indexes were 0-based, that would mean that the first element of the
list was referenced by -0, which obviously can't be true, because 0 =
-0. In other words, we can't be using 0-based indexing for both the
positive and the negative values, because then 0 itself would be
ambiguous. It's got to be that -1 is the first element of the *pds
list, which means -- AFAICS, anyway -- that the way I phrased it is
correct.

Maybe what you need is a redesign. This convention seems impossibly
confusing and hence error-prone. What about using a separate bool to
indicate which list the index refers to?

That was my impression I first came across this, FWIW, and I confess I
didn't try hard enough to understand it fully.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 18 May 2018 at 06:21, Robert Haas <robertmhaas@gmail.com> wrote:

All right, so let's just say that explicitly. Maybe something like
the attached.

That looks fine to me.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 18 May 2018 at 02:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe what you need is a redesign. This convention seems impossibly
confusing and hence error-prone. What about using a separate bool to
indicate which list the index refers to?

While I agree that the coding is a bit unusual, I think it's also good
that we can get away without allocating yet another array nparts in
size. ExecSetupPartitionTupleRouting is already a huge bottleneck with
single-row INSERT into a partitioned table with a large number of
partitions. Allocating yet another array nparts in size will just slow
it down further.

I have patches locally that I'll be submitting during the v12 cycle to
improve on this. Among other things, the patches go to lengths to not
allocate these arrays when we don't have to.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2018/05/18 6:14, David Rowley wrote:

On 18 May 2018 at 02:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe what you need is a redesign. This convention seems impossibly
confusing and hence error-prone. What about using a separate bool to
indicate which list the index refers to?

While I agree that the coding is a bit unusual, I think it's also good
that we can get away without allocating yet another array nparts in
size. ExecSetupPartitionTupleRouting is already a huge bottleneck with
single-row INSERT into a partitioned table with a large number of
partitions. Allocating yet another array nparts in size will just slow
it down further.

I recall having considered the idea of adding an array of bools, but went
with the negative-indexes-for-partitioned-tables idea anyway, which I
remember was suggested by Robert back then [1]/messages/by-id/CA+TgmobF2r=f-crrE-k7WM8iFpBKLz3dtBtEc=KmkudYViYcyQ@mail.gmail.com. I admit it's a bit
confusing, but it's nice not have one more array allocation in that path
as you say.

I have patches locally that I'll be submitting during the v12 cycle to
improve on this. Among other things, the patches go to lengths to not
allocate these arrays when we don't have to.

That would be nice.

Thanks,
Amit

[1]: /messages/by-id/CA+TgmobF2r=f-crrE-k7WM8iFpBKLz3dtBtEc=KmkudYViYcyQ@mail.gmail.com
/messages/by-id/CA+TgmobF2r=f-crrE-k7WM8iFpBKLz3dtBtEc=KmkudYViYcyQ@mail.gmail.com

On 2018/05/18 5:56, David Rowley wrote:

On 18 May 2018 at 06:21, Robert Haas <robertmhaas@gmail.com> wrote:

All right, so let's just say that explicitly. Maybe something like
the attached.

That looks fine to me.

Me too, except:

+ * *pds list is the root partition, so 0 always means the first leaf. When

I prefer "root partitioned table" over "root partition", and git grep
suggests that that's actually what we use elsewhere in the source code.

Thanks,
Amit

On Thu, May 17, 2018 at 9:56 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2018/05/18 5:56, David Rowley wrote:

On 18 May 2018 at 06:21, Robert Haas <robertmhaas@gmail.com> wrote:

All right, so let's just say that explicitly. Maybe something like
the attached.

That looks fine to me.

Me too, except:

+ * *pds list is the root partition, so 0 always means the first leaf. When

I prefer "root partitioned table" over "root partition", and git grep
suggests that that's actually what we use elsewhere in the source code.

I don't know that it's necessary to be that rigid about this, so I
chose to commit without changing it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, May 17, 2018 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe what you need is a redesign. This convention seems impossibly
confusing and hence error-prone. What about using a separate bool to
indicate which list the index refers to?

I really don't think it's a big deal. The comments may need some
improvement (which is what we're doing here), but it's not as if there
are no other places in the PostgreSQL code where positive and negative
values indicate different kinds of things, user and system columns
being the most obvious such distinction. What we're doing here can't
possibly be more error-prone than adding and subtracting
FirstLowInvalidHeapAttributeNumber all over the place.

I'm biased, of course. I invented this particular convention. If
somebody else feels like redesigning it for a future release, and the
result is better than what we have now, cool. But I do not think that
it would be a clear improvement to have a boolean indicating whether
or not the index is an index into subpartitions or leaf nodes and have
the index value always be positive. In the current convention, if you
fail to handle negative values, you will probably crash. With that
convention, if you forget to check the boolean and just assume you
have a leaf partition, it's quite likely that it will look like it
works, but do the wrong thing. And as David points out, it also uses
less memory (which also makes it faster).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company