On Sun, 2022-10-30 at 09:39 -0400, Tom Lane wrote:
> Dilip Kumar <dilipbalaut@gmail.com> writes:
> > Yes, this looks like a bug and your fix seems correct to me. It
> > would
> > be nice to add a test case for this scenario.
>
> A test case doesn't seem that exciting to me. If we were trying to
> make it actually work, then yeah, but throwing an error isn't that
> useful to test. The code will be exercised by replication to a
> regular partitioned table (I assume we do have tests for that).
>
> What I'm wondering about is whether we can refactor this code
> to avoid so many usually-useless catalog lookups. Pulling the
> namespace name, in particular, is expensive and we generally
> are not going to need the result. In the child-rel case it'd
> be much better to pass the opened relation and let the error-check
> subroutine work from that. Maybe we should just do it like that
> at the start of the recursion, too? Or pass the relid and let
> the subroutine look up the names only in the error case.
Sure, I think passing in the opened relation is a good idea.
> A completely different line of thought is that this doesn't seem
> like a terribly bulletproof fix, since children could get added to
> a partitioned table after we look. Maybe it'd be better to check
> the relkind at the last moment before we do something that depends
> on a child table being a relation.
These checks are run both on subscription DDL commands, which is good
to get some early feedback, and inside logical_rel_open(), right before
something useful is about to get done to the relation, so we should be
good here. I think some tests would actually be nice to verify this,
but I don't really have a strong opinion about it.
I'll refactor the patch and post a bit later.
