pgsql: Clarify use of temporary tables within partition trees

On 20 June 2018 at 13:53, Michael Paquier <michael@paquier.xyz> wrote:

Clarify use of temporary tables within partition trees

Hi,

Thanks for committing this fix.

I think slightly more should have been done. There's still some dead
code in expand_partitioned_rtentry that I think should be removed.

The code won't cost much performance wise, but it may mislead someone
into thinking they can add some other condition there to skip
partitions.

The attached removes it.

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

On Fri, Jun 29, 2018 at 5:13 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 20 June 2018 at 13:53, Michael Paquier <michael@paquier.xyz> wrote:

Clarify use of temporary tables within partition trees

Thanks for committing this fix.

I think slightly more should have been done. There's still some dead
code in expand_partitioned_rtentry that I think should be removed.

The code won't cost much performance wise, but it may mislead someone
into thinking they can add some other condition there to skip
partitions.

The attached removes it.

I'd rather keep an elog(ERROR) than completely remove the check.

Also, for the record, I think the subject line of Michael's commit
message was pretty unclear about what it was actually doing.

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

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.

Also, for the record, I think the subject line of Michael's commit
message was pretty unclear about what it was actually doing.

How would you formulate it? Perhaps the error message did not emphasize
enough on the fast that it actually blocked a behavior, say "Block mix
of temporary and permanent relations in partition trees" or such?
--
Michael

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

Also, for the record, I think the subject line of Michael's commit
message was pretty unclear about what it was actually doing.

How would you formulate it? Perhaps the error message did not emphasize
enough on the fast that it actually blocked a behavior, say "Block mix
of temporary and permanent relations in partition trees" or such?

For me, reading the subject line of the commit I'd have expected a doc
change, or improved/new code comments.

This is really more "Disallow mixed temp/permanent partitioned hierarchies".

"Clarify" does not really involve a change of behaviour. It's an
explanation of what the behaviour is.

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

On Mon, Jul 2, 2018 at 8:59 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

How would you formulate it? Perhaps the error message did not emphasize
enough on the fast that it actually blocked a behavior, say "Block mix
of temporary and permanent relations in partition trees" or such?

Yes.

For me, reading the subject line of the commit I'd have expected a doc
change, or improved/new code comments.

This is really more "Disallow mixed temp/permanent partitioned hierarchies".

Yes.

"Clarify" does not really involve a change of behaviour. It's an
explanation of what the behaviour is.

And yes.

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

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

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

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

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

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

(re-adding committers list)

On 3 July 2018 at 20:31, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

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

That will cause the AppendRelInfo not to be built for a partitioned
table with no partitions. I'm not personally certain that doing that
comes without consequence. Are you certain of that? If not 100%,
then I think that's more of a task for PG12. I'm trying to propose
removing dead code for PG11 and on.

My current thoughts are that moving this test up a few lines is
unlikely to improve performance in a real-world situation, so it does
not seem worth the risk for a backpatch to me.

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.

I recently saw this too and wondered about it. I then wrote a patch to
just have it create a single RangeTblEntry for each partitioned table.
The tests all passed.

I'd categorise this one the same as I have #1 above, i.e. not
backpatch material. It seems like something useful to look into for
v12 though. I assumed this was done for a reason and that I just
didn't understand what that reason was. I don't recall any comments to
explain the reason why we build two RangeTblEntrys for each
partitioned table.

In light of what Amit has highlighted, I'm still standing by the v3
patch assuming the typo is fixed.

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

On Tue, Jul 03, 2018 at 09:05:41PM +1200, David Rowley wrote:

[...]

I'd categorise this one the same as I have #1 above, i.e. not
backpatch material. It seems like something useful to look into for
v12 though. I assumed this was done for a reason and that I just
didn't understand what that reason was. I don't recall any comments to
explain the reason why we build two RangeTblEntrys for each
partitioned table.

I agree. Please let's keep v11 stable, and discuss further more on
future optimizations like the previous two items for v12, which has
plenty of time to be broken.

In light of what Amit has highlighted, I'm still standing by the v3
patch assuming the typo is fixed.

Yeah. Actually I'd like to add a test as well to test the recursion
call of expand_partitioned_rtentry. If you have an idea, please let me
know or I'll figure out one by myself and add it probably in
create_table.sql.
--
Michael

On 3 July 2018 at 21:15, Michael Paquier <michael@paquier.xyz> wrote:

Yeah. Actually I'd like to add a test as well to test the recursion
call of expand_partitioned_rtentry. If you have an idea, please let me
know or I'll figure out one by myself and add it probably in
create_table.sql.

What specifically do you want to test? There are plenty of partitioned
tests with sub-partitioned tables. Going by [1]https://coverage.postgresql.org/src/backend/optimizer/prep/prepunion.c.gcov.html, there's no shortage
of coverage.

Of course, the dead code I'm proposing we remove is not covered.
There's no way to cover it... it's dead.

[1]: https://coverage.postgresql.org/src/backend/optimizer/prep/prepunion.c.gcov.html

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

On 2018/07/03 18:05, David Rowley wrote:

(re-adding committers list)

On 3 July 2018 at 20:31, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

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

That will cause the AppendRelInfo not to be built for a partitioned
table with no partitions. I'm not personally certain that doing that
comes without consequence. Are you certain of that? If not 100%,
then I think that's more of a task for PG12. I'm trying to propose
removing dead code for PG11 and on.

What to use the AppendRelInfo for if there is no partition?

My current thoughts are that moving this test up a few lines is
unlikely to improve performance in a real-world situation, so it does
not seem worth the risk for a backpatch to me.

It's just that needless structures get created which we could avoid, and
once I realized that, I found out about the 2 below.

If you look at all of what expand_single_inheritance_child does, I'm a bit
surprised you don't think we should try to avoid calling it if we don't
need any of that processing.

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.

I recently saw this too and wondered about it. I then wrote a patch to
just have it create a single RangeTblEntry for each partitioned table.
The tests all passed.

Which is how I was thinking the commit 0a480502b09 had done it, but
apparently not. That's a commit in PG11. None of what I'm suggesting is
for PG 10.

I'd categorise this one the same as I have #1 above, i.e. not
backpatch material. It seems like something useful to look into for
v12 though. I assumed this was done for a reason and that I just
didn't understand what that reason was. I don't recall any comments to
explain the reason why we build two RangeTblEntrys for each
partitioned table.

Maybe because that's what's done for the root parent in a plain
inheritance hierarchy, which is always a plain table. In that case, one
RTE is for its role as the parent (with rte->inh = true) and another is
for its role as a child (with rte->inh = false). The former is processed
as an append rel and the latter as a plain rel that will get assigned scan
paths such as SeqScan, etc.

For partitioned table parent(s), we need only the former because they can
only be processed as append rels. That's why I'm proposing we could
adjust the commit in PG 11 that introduced expand_partitioned_rtentry such
that the duplicate child RTE and other objects are not created.

Thanks,
Amit

On Tue, Jul 03, 2018 at 09:30:20PM +1200, David Rowley wrote:

On 3 July 2018 at 21:15, Michael Paquier <michael@paquier.xyz> wrote:

Yeah. Actually I'd like to add a test as well to test the recursion
call of expand_partitioned_rtentry. If you have an idea, please let me
know or I'll figure out one by myself and add it probably in
create_table.sql.

What specifically do you want to test? There are plenty of partitioned
tests with sub-partitioned tables. Going by [1], there's no shortage
of coverage.

Of course, the dead code I'm proposing we remove is not covered.
There's no way to cover it... it's dead.

Your patch removes this part:
- /*
- * If the partitioned table has no partitions or all the partitions are
- * temporary tables from other backends, treat this as non-inheritance
- * case.
- */
- if (!has_child)
- parentrte->inh = false;

And adds this equivalent part:
+   /*
+    * If the partitioned table has no partitions, treat this as the
+    * non-inheritance case.
+    */
+   if (partdesc->nparts == 0)
+   {
+       parentrte->inh = false;
+       return;
+   }

As far as I can see from the coverage report, the former is not tested,
and corresponds to the case of a partition leaf which is itself
partitioned but has no partitions, and the new portion is equivalent to
the part removed. That ought to be tested, particularly as Amit
mentions that there could be improvements with moving it around in
future versions.
--
Michael