|From:||Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>|
|Cc:||Robert Haas <robertmhaas(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: expanding inheritance in partition bound order|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 2017/09/11 16:16, Amit Khandekar wrote:
> Thanks Amit for the patch. I am still reviewing it, but meanwhile
> below are a few comments so far ...
Thanks for the review.
> + next_parted_idx += (list_length(*pds) - next_parted_idx - 1);
> I think this can be replaced just by :
> + next_parted_idx = list_length(*pds) - 1;
> Or, how about removing this variable next_parted_idx altogether ?
> Instead, we can just do this :
> pd->indexes[i] = -(1 + list_length(*pds));
That seems like the simplest possible way to do it.
> + next_leaf_idx += (list_length(*leaf_part_oids) - next_leaf_idx);
> Didn't understand why next_leaf_idx needs to be updated in case when
> the current partrelid is partitioned. I think it should be incremented
> only for leaf partitions, no ? Or for that matter, again, how about
> removing the variable 'next_leaf_idx' and doing this :
> *leaf_part_oids = lappend_oid(*leaf_part_oids, partrelid);
> pd->indexes[i] = list_length(*leaf_part_oids) - 1;
Attached updated patch does it that way for both partitioned table indexes
and leaf partition indexes. Thanks for pointing it out.
> * For every partitioned table in the tree, starting with the root
> * partitioned table, add its relcache entry to parted_rels, while also
> * queuing its partitions (in the order in which they appear in the
> * partition descriptor) to be looked at later in the same loop. This is
> * a bit tricky but works because the foreach() macro doesn't fetch the
> * next list element until the bottom of the loop.
> I think the above comment needs to be modified with something
> explaining the relevant changed code. For e.g. there is no
> parted_rels, and the "tricky" part was there earlier because of the
> list being iterated and at the same time being appended.
I think I forgot to update this comment.
> I couldn't see the existing comments like "Indexes corresponding to
> the internal partitions are multiplied by" anywhere in the patch. I
> think those comments are still valid, and important.
Again, I failed to keep this comment. Anyway, I reworded the comments a
bit to describe what the code is doing more clearly. Hope you find it so too.
|Next Message||Andrey Borodin||2017-09-11 10:25:15||Re: Allow GiST opcalsses without compress\decompres functions|
|Previous Message||Christoph Berg||2017-09-11 09:53:38||Re: mysql_fdw + PG10: unrecognized node type: 217|