From: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Rafia Sabih <rafia(dot)sabih(at)enterprisedb(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Partition-wise join for join between (declaratively) partitioned tables |
Date: | 2017-10-03 12:57:05 |
Message-ID: | CAFjFpRcpbMmsKv_eCn6SNoiVrNi=y4RXfE8b3UH=ZZOEDL_w2g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Oct 3, 2017 at 7:48 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Sep 21, 2017 at 8:21 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> Here's set of rebased patches. The patch with extra tests is not for
>> committing. All other patches, except the last one, will need to be
>> committed together. The last patch may be committed along with other
>> patches or as a separate patch.
>
> In set_append_rel_size, is it necessary to set attr_needed =
> bms_copy(rel->attr_needed[index]) rather than just pointing to the
> existing value? If so, perhaps the comments should explain the
> reasons. I would have thought that the values wouldn't change after
> this point, in which case it might not be necessary to copy them.
Right. The only places where attr_needed is changed is in
remove_rel_from_query() (useless join removal) and
add_vars_to_targetlist(). Both of those happen before
set_append_rel_size(). Since parent and child join should project same
attributes, having them share the Relids set makes more sense. So,
changed accordingly and explained the same in comments.
Also, changed list_nth() in the following code block to use list_nth_node().
>
> Regarding nomenclature and my previous griping about wisdom, I was
> wondering about just calling this a "partition join" like you have in
> the regression test. So the GUC would be enable_partition_join, you'd
> have generate_partition_join_paths(), etc. Basically just delete
> "wise" throughout.
Partition-wise join is standard term used in literature and in
documentation of other popular DBMSes, so partition_wise makes more
sense. But I am fine with partition_join as well. Do you want it
partition_join or partitionjoin like enable_mergejoin/enable_hashjoin
etc.?
>
> The elog(DEBUG3) in try_partition_wise_join() doesn't follow message
> style guidelines and I think should just be removed. It was useful
> for development, I'm sure, but it's time for it to go.
Done.
>
> + elog(ERROR, "unrecognized path node type %d", (int) nodeTag(path));
>
> I think we should use the same formulation as elsewhere, namely
> "unrecognized node type: %d". And likewise probably "unexpected join
> type: %d".
Changed "unrecognized path node type" to "unrecognized node type".
"unrecognized join type: %d" seems to be used everywhere except
postgres_fdw. So, used that. Also added a cast to int similar to other
places.
>
> partition_join_extras.sql has a bunch of whitespace damage, although
> it doesn't really matter since, as you say, that's not for commit.
>
Right. I will remove that patch from the patch-set when those tests
are no more needed i.e. once we are done with code changes to the
patches.
Attached the updated patch-set.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Attachment | Content-Type | Size |
---|---|---|
pg_dp_join_patches_v35.tar.gz | application/x-gzip | 123.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tatsuo Ishii | 2017-10-03 12:57:15 | Re: list of credits for release notes |
Previous Message | Euler Taveira | 2017-10-03 12:47:32 | Re: Postgresql gives error that role goes not exists while it exists |