From: Tom Lane <tgl@sss.pgh.pa.us>
To: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Cc: shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: Segfault on logical replication to partitioned table with foreign children
Date: Tue, 01 Nov 2022 15:10:14 -0400
Message-ID: <1361869.1667329814@sss.pgh.pa.us> (raw)
In-Reply-To: <04b5212cd549ee3c8efc63f6a92d85d5463bfdf1.camel@gmail.com>
References: <6b93e3748ba43298694f376ca8797279d7945e29.camel@gmail.com>
<CAFiTN-vvnrvHeUibhwhnJNgTyE70DcBTgXPws3iO7HVM9TWANg@mail.gmail.com>
<188734.1667137161@sss.pgh.pa.us>
<OSZPR01MB6310B855B1B90FEE433A98F4FD379@OSZPR01MB6310.jpnprd01.prod.outlook.com>
<04b5212cd549ee3c8efc63f6a92d85d5463bfdf1.camel@gmail.com>
Ilya Gladyshev <ilya.v.gladyshev@gmail.com> writes:
> [ v2-0001-check-relkind-of-subscription-target-recursively.patch ]
Hmm. I like Shi yu's way better (formal patch attached). Checking
at CREATE/ALTER SUBSCRIPTION is much more complicated, and it's really
insufficient, because what if someone adds a new partition after
setting up the subscription?
I get the argument about it being a useful check for simple mistakes,
but I don't entirely buy that argument, because I think there are
potential use-cases that it'd disallow needlessly. Imagine a
partitioned table that receives replication updates, but only into
the "current" partition; older partitions are basically static.
Now suppose you'd like to offload some of that old seldom-used data
to another server, and make those partitions into foreign tables
so you can still access it at need. Such a setup will work perfectly
fine today, but this patch would break it.
So I think what we want is to check when we identify the partition.
Maybe Shi yu missed a place or two to check, but I verified that the
attached stops the crash.
There'd still be value in refactoring to avoid premature lookup
of the namespace name, but that's just micro-optimization.
regards, tom lane
Attachments:v3-0001-check-target-partition-relkind.patch (text/x-diff, 910B) downloaddiff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.cindex 5250ae7f54..17b9c42ecf 100644--- a/src/backend/replication/logical/worker.c+++ b/src/backend/replication/logical/worker.c@@ -2176,6 +2176,15 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
Assert(partrelinfo != NULL);
partrel = partrelinfo->ri_RelationDesc;
+ /*+ * Check for supported relkind. We need this since partitions might be of+ * unsupported relkinds; and the set of partitions can change, so checking+ * at CREATE/ALTER SUBSCRIPTION would be insufficient.+ */+ CheckSubscriptionRelkind(partrel->rd_rel->relkind,+ get_namespace_name(RelationGetNamespace(partrel)),+ RelationGetRelationName(partrel));+
/*
* To perform any of the operations below, the tuple must match the
* partition's rowtype. Convert if needed or just copy, using a dedicated
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: tgl@sss.pgh.pa.us, ilya.v.gladyshev@gmail.com, shiy.fnst@fujitsu.com, dilipbalaut@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Segfault on logical replication to partitioned table with foreign children
In-Reply-To: <1361869.1667329814@sss.pgh.pa.us>
* 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