Re: unsupportable composite type partition keys

Amit Langote <amitlangote09@gmail.com> writes:
> On Wed, Dec 18, 2019 at 10:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My point is basically that CheckAttributeType already covers that
>> issue, as well as a lot of others.  So why isn't the partitioning
>> code using it?

> My reason to not use it was that the error message that are produced
> are not quite helpful in this case;

I can't get terribly excited about that; but in any case, if we think
the errors aren't nice enough, the answer is to improve them, not
re-implement the function badly.

After further thought, it seems to me that we are dealing with two
nearly independent issues:

1. We must not accept partition bounds values that are of underdetermined
types, else (a) we are likely to get failures like "record type has not
been registered" while loading them back from disk, and (b) polymorphic
btree support functions are likely to complain that they can't identify
the type they're supposed to work on.  This is exactly the same issue that
expression indexes face, so we should be applying the same checks, that
is CheckAttributeType().  I do not believe that checking for RECORD is
adequate to close this hole.  At the very least, RECORD[] is equally
dangerous, and in general I think any pseudotype would be risky.

2. If the partitioning expression contains a reference to the partitioned
table's rowtype, we get infinite recursion while trying to load the
relcache entry.  The patch proposes to prevent that by checking whether
the expression's final result type is that type, but that's not nearly
adequate because a reference anywhere inside the expression is just as
bad.  In general, considering possibly-inlined SQL functions, I'm doubtful
that any precheck is going to be able to prevent this scenario.

Now as far as point 1 goes, I think it's not really that awful to use
CheckAttributeType() with a dummy attribute name.  The attached
incomplete patch uses "partition key" which causes it to emit errors
like

regression=# create table fool (a int, b int) partition by list ((row(a, b)));
ERROR:  column "partition key" has pseudo-type record

I don't think that that's unacceptable.  But if we wanted to improve it,
we could imagine adding another flag, say CHKATYPE_IS_PARTITION_KEY,
that doesn't affect CheckAttributeType's semantics, just the wording of
the error messages it throws.

As far as point 2 goes, I think this is another outgrowth of the
fundamental design error that we load a partitioned rel's partitioning
info immediately when the relcache entry is created, rather than later
on-demand.  If we weren't doing that then it wouldn't be problematic
to inspect the rel's rowtype while constructing the partitioning info.
I've bitched about this before, if memory serves, but couldn't light
a fire under anyone about fixing it.  Now I think we have no choice.
It was never a great idea that minimal construction of a relcache
entry could result in running arbitrary user-defined code.

Note that the end result of this would be to allow, not prohibit,
cases like your example.  I wonder whether we couldn't also lift
the restriction against whole-row Vars in partition expressions.
Doesn't seem like there is much difference between such a Var and
a row(...)::table_rowtype expression.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e8e004e..29efc9c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15102,6 +15102,16 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
             attcollation = exprCollation(expr);

             /*
+             * The expression must be of a storable type (e.g., not RECORD).
+             * The test is the same as for whether a table column is of a safe
+             * type (which is why we needn't check for the non-expression
+             * case).
+             */
+            CheckAttributeType("partition key",
+                               atttype, attcollation,
+                               NIL, 0);
+
+            /*
              * Strip any top-level COLLATE clause.  This ensures that we treat
              * "x COLLATE y" and "(x COLLATE y)" alike.
              */