Thread: Re: pgsql: Clarify use of temporary tables within partition trees
On Tue, Jul 03, 2018 at 12:59:33PM +1200, David Rowley wrote: > On 3 July 2018 at 10:16, Michael Paquier <michael@paquier.xyz> wrote: >> On Mon, Jul 02, 2018 at 02:07:37PM -0400, Robert Haas wrote: >>> I'd rather keep an elog(ERROR) than completely remove the check. >> >> +1. > > Attached Okay, the patch looks logically correct to me, I just tweaked the comments as per the attached. I would also back-patch that down to v11 to keep the code consistent with HEAD.. What do you think? -- Michael
Attachment
On 3 July 2018 at 16:55, Michael Paquier <michael@paquier.xyz> wrote: > Okay, the patch looks logically correct to me, I just tweaked the > comments as per the attached. I would also back-patch that down to v11 > to keep the code consistent with HEAD.. What do you think? Thanks for fixing it up. It looks fine apart from "Temporation" should be "Temporary". I think it should be backpatched to v11 and v10. Your original commit went there too. I don't see any reason to do any different here than what you did with the original commit. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Jul 03, 2018 at 06:00:46PM +1200, David Rowley wrote: > Thanks for fixing it up. It looks fine apart from "Temporation" should > be "Temporary". Of course, thanks. > I think it should be backpatched to v11 and v10. Your original commit > went there too. I don't see any reason to do any different here than > what you did with the original commit. expand_partitioned_rtentry is new as of v11. Or you mean to tweak expand_inherited_rtentry() perhaps? I am not sure that it is worth it as the code has already diverged between 10 and 11. -- Michael
Attachment
On 3 July 2018 at 18:11, Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Jul 03, 2018 at 06:00:46PM +1200, David Rowley wrote: >> I think it should be backpatched to v11 and v10. Your original commit >> went there too. I don't see any reason to do any different here than >> what you did with the original commit. > > expand_partitioned_rtentry is new as of v11. Or you mean to tweak > expand_inherited_rtentry() perhaps? I am not sure that it is worth it > as the code has already diverged between 10 and 11. Oh right. I'd forgotten that changed in v11. I think the v10 code is fine as is then. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2018/07/03 15:16, David Rowley wrote:
> On 3 July 2018 at 18:11, Michael Paquier <michael@paquier.xyz> wrote:
>> On Tue, Jul 03, 2018 at 06:00:46PM +1200, David Rowley wrote:
>>> I think it should be backpatched to v11 and v10. Your original commit
>>> went there too. I don't see any reason to do any different here than
>>> what you did with the original commit.
>>
>> expand_partitioned_rtentry is new as of v11. Or you mean to tweak
>> expand_inherited_rtentry() perhaps? I am not sure that it is worth it
>> as the code has already diverged between 10 and 11.
>
> Oh right. I'd forgotten that changed in v11. I think the v10 code is
> fine as is then.
Sorry for jumping in late here. I have a comment on the patch.
+ /* if there are no partitions then treat this as non-inheritance case. */
+ if (partdesc->nparts == 0)
+ {
+ parentrte->inh = false;
+ return;
+ }
+
Why is this not near the beginning of expand_partitioned_rtentry()?
Also, ISTM, this code would be unreachable because
expand_inherited_rtentry would not call here if the above if statement is
true, no?
I see the following two blocks in expand_inherited_rtentry before one gets
to the call to expand_partitioned_rtentry:
if (!has_subclass(parentOID))
{
/* Clear flag before returning */
rte->inh = false;
return;
}
and
if (list_length(inhOIDs) < 2)
{
/* Clear flag before returning */
rte->inh = false;
return;
}
Thanks,
Amit
On Tue, Jul 03, 2018 at 03:29:36PM +0900, Amit Langote wrote: > Why is this not near the beginning of expand_partitioned_rtentry()? > > Also, ISTM, this code would be unreachable because > expand_inherited_rtentry would not call here if the above if statement is > true, no? FWIW, I understood that the intention here is to be careful, particularly if expand_partitioned_rtentry begins to get called from a different code path in the future, which is something that would likely happen. We could replace that by an assertion or even an elog(), and change again this code in the future, now what's proposed here makes quite some sense to me as well. -- Michael
Attachment
Just realized something...
On 2018/07/03 15:29, Amit Langote wrote:
> Sorry for jumping in late here. I have a comment on the patch.
>
> + /* if there are no partitions then treat this as non-inheritance case. */
> + if (partdesc->nparts == 0)
> + {
> + parentrte->inh = false;
> + return;
> + }
> +
>
> Why is this not near the beginning of expand_partitioned_rtentry()?
This one still stands I think.
> Also, ISTM, this code would be unreachable because
> expand_inherited_rtentry would not call here if the above if statement is
> true, no?
I forgot that expand_partitioned_rtentry() will recursively call itself if
a partition is itself a partitioned table, in which case the above code helps.
Thanks,
Amit
On Tue, Jul 03, 2018 at 03:49:44PM +0900, Amit Langote wrote: > I forgot that expand_partitioned_rtentry() will recursively call itself if > a partition is itself a partitioned table, in which case the above > code helps. Actually look at the coverage reports: https://coverage.postgresql.org/src/backend/optimizer/prep/prepunion.c.gcov.html 1742 : /* 1743 : * If the partitioned table has no partitions or all the partitions are 1744 : * temporary tables from other backends, treat this as non-inheritance 1745 : * case. 1746 : */ 1747 4920 : if (!has_child) 1748 0 : parentrte->inh = false; 1749 4920 : } expand_partitioned_rtentry() never disables this flag on recursive calls with a multi-level tree. Could it be possible to get a test which closes the gap? -- Michael
Attachment
On 2018/07/03 17:31, Amit Langote wrote: > On 2018/07/03 16:05, Michael Paquier wrote: >> On Tue, Jul 03, 2018 at 03:49:44PM +0900, Amit Langote wrote: >>> I forgot that expand_partitioned_rtentry() will recursively call itself if >>> a partition is itself a partitioned table, in which case the above >>> code helps. >> >> Actually look at the coverage reports: >> https://coverage.postgresql.org/src/backend/optimizer/prep/prepunion.c.gcov.html >> 1742 : /* >> 1743 : * If the partitioned table has no partitions or all the partitions are >> 1744 : * temporary tables from other backends, treat this as non-inheritance >> 1745 : * case. >> 1746 : */ >> 1747 4920 : if (!has_child) >> 1748 0 : parentrte->inh = false; >> 1749 4920 : } >> >> expand_partitioned_rtentry() never disables this flag on recursive calls >> with a multi-level tree. Could it be possible to get a test which >> closes the gap? > > I guess that it will be hard as you mentioned before on the thread that > led to this commit. We can't write regression tests which require using > temporary partitions from other sessions. > > Anyway, I just wanted to say that I was wrong when I said that the block > added by the patch is unreachable. It *is* reachable for multi-level > partitioning. For example, it will execute in the following case: > > create table p (a int, b int) partition by list (a); > create table pd partition of p default partition by list (b); > select * from p; > > expand_partitioned_rtentry will get called twice and the newly added code > would result in early return from the function in the 2nd invocation which > is for 'pd'. > > But, > > 1. I still insist that it's better for the newly added code to be near the > top of the function body than in the middle, which brings me to... > > 2. While we're at fixing the code around here, I think we should think > about trying to get rid of the *non-dead* code that produces a structure > that isn't used anywhere, which I was under the impression, 0a480502b09 > [1] already did (cc'ing Ashutosh). To clarify, we still unnecessarily > create a "duplicate" RTE for partitioned tables in a partition tree > (non-leaf tables) in its role as a child. So, for the above query, there > end up being created 4 entries in the query's range table (2 for 'p' and 2 > for 'pd'). That makes sense for plain inheritance, because even the root > parent table in a plain inheritance tree is a regular table containing > data. That's not true for partition inheritance trees, where non-leaf > tables contain no data, so we don't create a plan to scan them (see > d3cc37f1d80 [2]), which in turn means we don't need to create the > redundant "duplicate" child RTEs for them either. > > See attached my delta patch to address both 1 and 2. > > Thanks, > Amit > > [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0a480502b09 > > [2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d3cc37f1d80 For some reason, ML address got removed from the list of address when sending the above message. Thanks, Amit