Bogus EXPLAIN results with column aliases for mismatched partitions

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Bogus EXPLAIN results with column aliases for mismatched partitions
Date: 2019-12-01 02:40:15
Message-ID: 12424.1575168015@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I started to fool around with the ruleutils.c rewrite discussed in [1],
and ran into an independent bug: if you attach column aliases to a
partitioned table, and some of the partitions don't have an exact match of
column attnums, then EXPLAIN uses the wrong aliases for those partitions.
As an example, after modifying partition_prune.sql like this:

diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index a5900e5..41f0b6f 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -202,8 +202,13 @@ CREATE TABLE part (a INT, b INT) PARTITION BY LIST (a);
CREATE TABLE part_p1 PARTITION OF part FOR VALUES IN (-2,-1,0,1,2);
CREATE TABLE part_p2 PARTITION OF part DEFAULT PARTITION BY RANGE(a);
CREATE TABLE part_p2_p1 PARTITION OF part_p2 DEFAULT;
+CREATE TABLE part_rev (b INT, c INT, a INT);
+ALTER TABLE part ATTACH PARTITION part_rev FOR VALUES IN (3); -- fail
+ALTER TABLE part_rev DROP COLUMN c;
+ALTER TABLE part ATTACH PARTITION part_rev FOR VALUES IN (3); -- now it's ok
INSERT INTO part VALUES (-1,-1), (1,1), (2,NULL), (NULL,-2),(NULL,NULL);
EXPLAIN (COSTS OFF) SELECT tableoid::regclass as part, a, b FROM part WHERE a IS NULL ORDER BY 1, 2, 3;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM part p(x) ORDER BY x;

--
-- some more cases

then the EXPLAIN output produced by HEAD looks like:

QUERY PLAN
-----------------------------------------------
Sort
Output: p.x, p.b
Sort Key: p.x
-> Append
-> Seq Scan on public.part_p1 p
Output: p.x, p.b
-> Seq Scan on public.part_rev p_1
Output: p_1.a, p_1.x
-> Seq Scan on public.part_p2_p1 p_2
Output: p_2.x, p_2.b
(10 rows)

wherein the "x" alias for column "a" has been applied to part_rev.b.
That's wrong and confusing.

The reason this happens is that expand_single_inheritance_child()
just clones the parent RTE's alias node without any thought for
the possibility that the columns don't match one-to-one. It's
an ancient bug that affects traditional inheritance as well as
partitioning.

I experimented with fixing this by making expand_single_inheritance_child
generate a correctly-adjusted child alias node, which seems reasonable
since it takes pains to adjust the rest of the child RTE for the different
column layout. It turns out to be slightly tedious to do that without
causing a lot of regression test diffs: if we add an alias node where
there was none before, that affects ruleutils.c's selection of table
aliases not just column aliases. The "variant-a" patch below mostly
succeeds in avoiding test diffs, but it adds a fair amount of complication
to inherit.c. The "variant-b" patch below uses a simpler way of setting
up the child aliases, which results in a whole lot of test diffs: all
children of a parent named "x" will now show in EXPLAIN with aliases like
"x_1", "x_2", etc. (That happens already if you wrote an explicit table
alias "x", but not if you didn't.) While my initial reaction was that
that was an unacceptable amount of churn, the idea gets more appealing the
more I think about it. It means that tables you did not name in the query
will be shown with aliases that clearly identify their connection to
something you did name. So despite the added churn, I'm kind of attracted
to the variant-b approach. (Note that the discussion in [1] is almost
certainly going to result in some changes to ruleutils.c's alias-selection
behavior anyway, so I don't think staying exactly compatible with v12 is
worth much here.)

On the other hand, if we went with variant-a it might be plausible to
back-patch this fix. But given the fact that this is a mostly cosmetic
problem, and we've not had field complaints, I don't feel a big need
to fix it in the back branches.

Some other loose ends:

* variant-a's diffs in expected/postgres_fdw.out indicate that
postgres_fdw is doing something weird with the table aliases it selects to
print in the "Relations:" output. I think this is an independent bug ---
and it surely *is* a bug, because the aliases printed by HEAD don't agree
with the table aliases used for Vars of those relations. But I haven't
run it to ground yet. (Notice that variant-b fixes those discrepancies in
the opposite direction...)

* To make computing the modified column alias list cheap, I made
make_inh_translation_list() compute a reverse-mapping array showing the
parent column associated with each child column. I'm more than a little
bit tempted to add that array to the AppendRelInfo struct, instead of
passing it back separately and then discarding it. We could use the info
to simplify and speed up the reverse-mapping logic added by commit
553d2ec27, and I bet there will be other use-cases later. But I've not
done that in these patches.

Thoughts, objections, better ideas?

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/001001d4f44b%242a2cca50%247e865ef0%24%40lab.ntt.co.jp

Attachment Content-Type Size
fix-inherited-column-aliases-a-1.patch text/x-diff 18.2 KB
fix-inherited-column-aliases-b-1.patch text/x-diff 365.9 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-12-01 02:43:35 Re: Runtime pruning problem
Previous Message Michael Paquier 2019-12-01 02:34:16 Re: Tid scan improvements