Message-ID: <44d2ac16d32f671d2e7df0d4903bbf7cf03c21c7.camel@gmail.com>
Subject: Re: Segfault on logical replication to partitioned table with
 foreign children
From: ilya.v.gladyshev@gmail.com
To: Tom Lane <tgl@sss.pgh.pa.us>, Dilip Kumar <dilipbalaut@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Date: Mon, 31 Oct 2022 17:15:48 +0400
In-Reply-To: <188734.1667137161@sss.pgh.pa.us>
References: <6b93e3748ba43298694f376ca8797279d7945e29.camel@gmail.com>
	 <CAFiTN-vvnrvHeUibhwhnJNgTyE70DcBTgXPws3iO7HVM9TWANg@mail.gmail.com>
	 <188734.1667137161@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/44d2ac16d32f671d2e7df0d4903bbf7cf03c21c7.camel%40gmail.com>
Precedence: bulk


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.