From: ilya.v.gladyshev@gmail.com
To: Tom Lane <tgl@sss.pgh.pa.us>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Segfault on logical replication to partitioned table with foreign children
Date: Mon, 31 Oct 2022 17:15:48 +0400
Message-ID: <44d2ac16d32f671d2e7df0d4903bbf7cf03c21c7.camel@gmail.com> (raw)
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>
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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilya.v.gladyshev@gmail.com, tgl@sss.pgh.pa.us, dilipbalaut@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Segfault on logical replication to partitioned table with foreign children
In-Reply-To: <44d2ac16d32f671d2e7df0d4903bbf7cf03c21c7.camel@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox