On Sun, 2022-10-30 at 09:39 -0400, Tom Lane wrote: > Dilip Kumar <dilipbal...@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.
