Re: unsupportable composite type partition keys

On Sun, Dec 22, 2019 at 6:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > 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.
>
> Here's a draft patch for that.

Thanks for writing the patch.  This also came up recently on another thread [1].

>  There are a couple of secondary issues
> I didn't do anything about yet:
>
> * When rebuilding an open relcache entry for a partitioned table, this
> coding now always quasi-leaks the old rd_pdcxt, where before that happened
> only if the partdesc actually changed.  (Even if I'd kept the
> equalPartitionDescs call, it would always fail.)  I complained about the
> quasi-leak behavior before, but this probably pushes it up to the level of
> "must fix".  What I'm inclined to do is to hack
> RelationDecrementReferenceCount so that, when the refcount goes to zero,
> we delete any child contexts of rd_pdcxt.  That's pretty annoying but in
> the big scheme of things it's unlikely to matter.

Hacking RelationDecrementReferenceCount() like that sounds OK.

-        else if (rebuild && newrel->rd_pdcxt != NULL)
+        if (rebuild && newrel->rd_pdcxt != NULL)

Checking rebuild seems unnecessary in this block, although that's true
even without the patch.

+             * To ensure that it's not leaked completely, re-attach it to the
+             * new reldesc, or make it a child of the new reldesc's rd_pdcxt
+             * in the unlikely event that there is one already.  (See hack in
+             * RelationBuildPartitionDesc.)
...
+            if (relation->rd_pdcxt != NULL) /* probably never happens */
+                MemoryContextSetParent(newrel->rd_pdcxt, relation->rd_pdcxt);
+            else
+                relation->rd_pdcxt = newrel->rd_pdcxt;

While I can imagine that RelationBuildPartitionDesc() might encounter
an old partition descriptor making the re-parenting hack necessary
there, I don't see why it's needed here, because a freshly built
relation descriptor would not contain the partition descriptor after
this patch.

> * It'd be better to declare RelationGetPartitionKey and
> RelationGetPartitionDesc in relcache.h and get their callers out of the
> business of including rel.h, where possible.

Although I agree to declare them in relcache.h, that doesn't reduce
the need to include rel.h in their callers much, because anyplace that
uses RelationGetPartitionDesc() is also very likely to use
RelationGetRelid() which is in rel.h.

> * equalPartitionDescs is now dead code, should we remove it?

Don't see any problem with doing so.

> > 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.
>
> I didn't look into that either.  I wouldn't propose back-patching that,
> but it'd be interesting to try to fix it in HEAD.

Agreed.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/CA%2BHiwqFucUh7hYkfZ6x1MVcs_R24eUfNVuRwdE_FwuwK8XpSZg%40mail.gmail.com