Horton

Thread

Commits

  1. Use key and partdesc from PartitionDispatch where possible.

  1. PartitionDispatch's partdesc field

    Robert Haas <robertmhaas@gmail.com> — 2018-07-25T20:23:29Z

    I noticed today that in the PartitionDispatch structure, the partdesc
field is set but not used.  So we could remove it.  See attached
pd-partdesc-remove.patch.  If we want to go this route, I suggest
doing a slightly more thorough removal and getting rid of the key
field as well.  See attached pd-partdesc-and-key-remove.patch.

Another alternative, which I think might make more sense, is to make
use pd->key and pd->partdesc in preference to pd->reldesc->rd_partkey
and pd->reldesc->rd_partdesc.  It seems to me that the idea of the
PartitionDispatch structure is that it gathers together all of the
information that we need for tuple routing, so it might make sense for
the tuple routing code ought to get the information from there rather
than referring back to the RelationDesc.  See attached
pd-partdesc-use.patch.

Basically, the decision here is whether we want to avoid invoking
RelationGetPartitionKey and RelationGetPartitionDesc over and over
again, either for reasons of notational cleanliness (macro names are
long) or safety (absolutely no chance that the answer will change) or
efficiency (maybe those macros will someday do more than just a
pointer dereference?).  If so, we should cache the data in the
PartitionDispatch object and then use it.  If not, there seems to be
no reason to copy those pointers from the Relation to the
PartitionDispatch in the first place, since the latter has a pointer
to the former anyway.

Opinions?

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

  2. Re: PartitionDispatch's partdesc field

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-07-26T02:42:31Z

    On 2018/07/26 5:23, Robert Haas wrote:
> I noticed today that in the PartitionDispatch structure, the partdesc
> field is set but not used.  So we could remove it.  See attached
> pd-partdesc-remove.patch.  If we want to go this route, I suggest
> doing a slightly more thorough removal and getting rid of the key
> field as well.  See attached pd-partdesc-and-key-remove.patch.
> 
> Another alternative, which I think might make more sense, is to make
> use pd->key and pd->partdesc in preference to pd->reldesc->rd_partkey
> and pd->reldesc->rd_partdesc.  It seems to me that the idea of the
> PartitionDispatch structure is that it gathers together all of the
> information that we need for tuple routing, so it might make sense for
> the tuple routing code ought to get the information from there rather
> than referring back to the RelationDesc.  See attached
> pd-partdesc-use.patch.

+1 to pd-partdesc-use.patch.

> Basically, the decision here is whether we want to avoid invoking
> RelationGetPartitionKey and RelationGetPartitionDesc over and over
> again, either for reasons of notational cleanliness (macro names are
> long) or safety (absolutely no chance that the answer will change) or
> efficiency (maybe those macros will someday do more than just a
> pointer dereference?).  If so, we should cache the data in the
> PartitionDispatch object and then use it.

I agree.

Thanks,
Amit

  3. Re: PartitionDispatch's partdesc field

    Robert Haas <robertmhaas@gmail.com> — 2018-07-27T13:44:54Z

    On Wed, Jul 25, 2018 at 10:42 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Another alternative, which I think might make more sense, is to make
>> use pd->key and pd->partdesc in preference to pd->reldesc->rd_partkey
>> and pd->reldesc->rd_partdesc.  It seems to me that the idea of the
>> PartitionDispatch structure is that it gathers together all of the
>> information that we need for tuple routing, so it might make sense for
>> the tuple routing code ought to get the information from there rather
>> than referring back to the RelationDesc.  See attached
>> pd-partdesc-use.patch.
>
> +1 to pd-partdesc-use.patch.

OK, that makes 2 votes for that alternative and 0 for everything else
combined, so I've committed that version.

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