Message-ID: <04b5212cd549ee3c8efc63f6a92d85d5463bfdf1.camel@gmail.com>
Subject: Re: Segfault on logical replication to partitioned table with
 foreign children
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
To: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, Tom Lane
	 <tgl@sss.pgh.pa.us>, Dilip Kumar <dilipbalaut@gmail.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: Tue, 01 Nov 2022 02:02:42 +0400
In-Reply-To: 
 <OSZPR01MB6310B855B1B90FEE433A98F4FD379@OSZPR01MB6310.jpnprd01.prod.outlook.com>
References: <6b93e3748ba43298694f376ca8797279d7945e29.camel@gmail.com>
	 <CAFiTN-vvnrvHeUibhwhnJNgTyE70DcBTgXPws3iO7HVM9TWANg@mail.gmail.com>
	 <188734.1667137161@sss.pgh.pa.us>
	 <OSZPR01MB6310B855B1B90FEE433A98F4FD379@OSZPR01MB6310.jpnprd01.prod.outlook.com>
Content-Type: multipart/mixed; boundary="=-FdM96811mkSwLF812UVo"
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/04b5212cd549ee3c8efc63f6a92d85d5463bfdf1.camel%40gmail.com>
Precedence: bulk


--=-FdM96811mkSwLF812UVo
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

On Mon, 2022-10-31 at 03:20 +0000, shiy.fnst@fujitsu.com wrote:
> On Sun, Oct 30, 2022 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 
> > 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.
> > 
> > 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.
> > 
> 
> I agree. So maybe we can add this check in
> apply_handle_tuple_routing().
> 
> diff --git a/src/backend/replication/logical/worker.c
> b/src/backend/replication/logical/worker.c
> index 5250ae7f54..e941b68e4b 100644
> --- a/src/backend/replication/logical/worker.c
> +++ b/src/backend/replication/logical/worker.c
> @@ -2176,6 +2176,10 @@ apply_handle_tuple_routing(ApplyExecutionData
> *edata,
>         Assert(partrelinfo != NULL);
>         partrel = partrelinfo->ri_RelationDesc;
> 
> +       /* Check for supported relkind. */
> +       CheckSubscriptionRelkind(partrel->rd_rel->relkind,
> +                                                        relmapentry-
> >remoterel.nspname, relmapentry->remoterel.relname);
> +
>         /*
>          * To perform any of the operations below, the tuple must
> match the
>          * partition's rowtype. Convert if needed or just copy, using
> a dedicated
> 
> 
> Regards,
> Shi yu

I have verified that the current patch handles the attaching of new
partitions to the target partitioned table by throwing an error on
attempt to insert into a foreign table inside the logical replication
worker. I have refactored the code to minimize cache lookups, but I am
yet to write the tests for this. See the attached patch for the
refactored version.


--=-FdM96811mkSwLF812UVo
Content-Disposition: attachment;
	filename*0=v2-0001-check-relkind-of-subscription-target-recursively.patc;
	filename*1=h
Content-Type: text/x-patch;
	name="v2-0001-check-relkind-of-subscription-target-recursively.patch";
	charset="UTF-8"
Content-Transfer-Encoding: base64

RnJvbSA1NjA4NjE4NTMyNGUxMmU5ZGI2YWRkNDBmODRiYzdlNjA4NjdmMWQ2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBJbHlhIEdsYWR5c2hldiA8aWx5YS52LmdsYWR5c2hldkBnbWFp
bC5jb20+CkRhdGU6IFR1ZSwgMSBOb3YgMjAyMiAwMTo0Mjo0NCArMDQwMApTdWJqZWN0OiBbUEFU
Q0ggdjJdIGNoZWNrIHJlbGtpbmQgb2Ygc3Vic2NyaXB0aW9uIHRhcmdldCByZWN1cnNpdmVseQoK
LS0tCiBzcmMvYmFja2VuZC9jb21tYW5kcy9zdWJzY3JpcHRpb25jbWRzLmMgICAgfCAxOCArKysr
LS0tLQogc3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY1JlcGxpY2F0aW9uLmMgICAgIHwgNTEgKysr
KysrKysrKysrKysrKysrLS0tLQogc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9yZWxh
dGlvbi5jIHwgIDYgKy0tCiBzcmMvaW5jbHVkZS9leGVjdXRvci9leGVjdXRvci5oICAgICAgICAg
ICAgfCAgNCArLQogNCBmaWxlcyBjaGFuZ2VkLCA1NyBpbnNlcnRpb25zKCspLCAyMiBkZWxldGlv
bnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9jb21tYW5kcy9zdWJzY3JpcHRpb25jbWRz
LmMgYi9zcmMvYmFja2VuZC9jb21tYW5kcy9zdWJzY3JpcHRpb25jbWRzLmMKaW5kZXggZjBjZWMy
YWQ1ZS4uMmFlODlmOWUwMyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvY29tbWFuZHMvc3Vic2Ny
aXB0aW9uY21kcy5jCisrKyBiL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3N1YnNjcmlwdGlvbmNtZHMu
YwpAQCAtNzAwLDEyICs3MDAsMTMgQEAgQ3JlYXRlU3Vic2NyaXB0aW9uKFBhcnNlU3RhdGUgKnBz
dGF0ZSwgQ3JlYXRlU3Vic2NyaXB0aW9uU3RtdCAqc3RtdCwKIAkJCXsKIAkJCQlSYW5nZVZhciAg
ICpydiA9IChSYW5nZVZhciAqKSBsZmlyc3QobGMpOwogCQkJCU9pZAkJCXJlbGlkOworCQkJCVJl
bGF0aW9uIHJlbDsKIAogCQkJCXJlbGlkID0gUmFuZ2VWYXJHZXRSZWxpZChydiwgQWNjZXNzU2hh
cmVMb2NrLCBmYWxzZSk7Ci0KLQkJCQkvKiBDaGVjayBmb3Igc3VwcG9ydGVkIHJlbGtpbmQuICov
Ci0JCQkJQ2hlY2tTdWJzY3JpcHRpb25SZWxraW5kKGdldF9yZWxfcmVsa2luZChyZWxpZCksCi0J
CQkJCQkJCQkJIHJ2LT5zY2hlbWFuYW1lLCBydi0+cmVsbmFtZSk7CisJCQkJcmVsID0gUmVsYXRp
b25JZEdldFJlbGF0aW9uKHJlbGlkKTsKKwkJCQkvKiBDaGVjayBmb3Igc3VwcG9ydGVkIHJlbGtp
bmQgcmVjdXJzaXZlbHkuICovCisJCQkJQ2hlY2tTdWJzY3JpcHRpb25SZWxhdGlvbihyZWwsIHJ2
LT5zY2hlbWFuYW1lLCBydi0+cmVsbmFtZSk7CisJCQkJUmVsYXRpb25DbG9zZShyZWwpOwogCiAJ
CQkJQWRkU3Vic2NyaXB0aW9uUmVsU3RhdGUoc3ViaWQsIHJlbGlkLCB0YWJsZV9zdGF0ZSwKIAkJ
CQkJCQkJCQlJbnZhbGlkWExvZ1JlY1B0cik7CkBAIC04NjEsMTIgKzg2MiwxMyBAQCBBbHRlclN1
YnNjcmlwdGlvbl9yZWZyZXNoKFN1YnNjcmlwdGlvbiAqc3ViLCBib29sIGNvcHlfZGF0YSwKIAkJ
ewogCQkJUmFuZ2VWYXIgICAqcnYgPSAoUmFuZ2VWYXIgKikgbGZpcnN0KGxjKTsKIAkJCU9pZAkJ
CXJlbGlkOworCQkJUmVsYXRpb24gcmVsOwogCiAJCQlyZWxpZCA9IFJhbmdlVmFyR2V0UmVsaWQo
cnYsIEFjY2Vzc1NoYXJlTG9jaywgZmFsc2UpOwotCi0JCQkvKiBDaGVjayBmb3Igc3VwcG9ydGVk
IHJlbGtpbmQuICovCi0JCQlDaGVja1N1YnNjcmlwdGlvblJlbGtpbmQoZ2V0X3JlbF9yZWxraW5k
KHJlbGlkKSwKLQkJCQkJCQkJCSBydi0+c2NoZW1hbmFtZSwgcnYtPnJlbG5hbWUpOworCQkJcmVs
ID0gUmVsYXRpb25JZEdldFJlbGF0aW9uKHJlbGlkKTsKKwkJCS8qIENoZWNrIGZvciBzdXBwb3J0
ZWQgcmVsa2luZCByZWN1cnNpdmVseS4gKi8KKwkJCUNoZWNrU3Vic2NyaXB0aW9uUmVsYXRpb24o
cmVsLCBydi0+c2NoZW1hbmFtZSwgcnYtPnJlbG5hbWUpOworCQkJUmVsYXRpb25DbG9zZShyZWwp
OwogCiAJCQlwdWJyZWxfbG9jYWxfb2lkc1tvZmYrK10gPSByZWxpZDsKIApkaWZmIC0tZ2l0IGEv
c3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY1JlcGxpY2F0aW9uLmMgYi9zcmMvYmFja2VuZC9leGVj
dXRvci9leGVjUmVwbGljYXRpb24uYwppbmRleCA2MDE0ZjJlMjQ4Li45ZGEwYjZiM2ExIDEwMDY0
NAotLS0gYS9zcmMvYmFja2VuZC9leGVjdXRvci9leGVjUmVwbGljYXRpb24uYworKysgYi9zcmMv
YmFja2VuZC9leGVjdXRvci9leGVjUmVwbGljYXRpb24uYwpAQCAtMTYsOSArMTYsMTEgQEAKIAog
I2luY2x1ZGUgImFjY2Vzcy9nZW5hbS5oIgogI2luY2x1ZGUgImFjY2Vzcy9yZWxzY2FuLmgiCisj
aW5jbHVkZSAiYWNjZXNzL3RhYmxlLmgiCiAjaW5jbHVkZSAiYWNjZXNzL3RhYmxlYW0uaCIKICNp
bmNsdWRlICJhY2Nlc3MvdHJhbnNhbS5oIgogI2luY2x1ZGUgImFjY2Vzcy94YWN0LmgiCisjaW5j
bHVkZSAiY2F0YWxvZy9wZ19pbmhlcml0cy5oIgogI2luY2x1ZGUgImNvbW1hbmRzL3RyaWdnZXIu
aCIKICNpbmNsdWRlICJleGVjdXRvci9leGVjdXRvci5oIgogI2luY2x1ZGUgImV4ZWN1dG9yL25v
ZGVNb2RpZnlUYWJsZS5oIgpAQCAtNjQ1LDIwICs2NDcsNTEgQEAgQ2hlY2tDbWRSZXBsaWNhSWRl
bnRpdHkoUmVsYXRpb24gcmVsLCBDbWRUeXBlIGNtZCkKIAkJCQkgZXJyaGludCgiVG8gZW5hYmxl
IGRlbGV0aW5nIGZyb20gdGhlIHRhYmxlLCBzZXQgUkVQTElDQSBJREVOVElUWSB1c2luZyBBTFRF
UiBUQUJMRS4iKSkpOwogfQogCi0KLS8qCi0gKiBDaGVjayBpZiB3ZSBzdXBwb3J0IHdyaXRpbmcg
aW50byBzcGVjaWZpYyByZWxraW5kLgotICoKLSAqIFRoZSBuc3BuYW1lIGFuZCByZWxuYW1lIGFy
ZSBvbmx5IG5lZWRlZCBmb3IgZXJyb3IgcmVwb3J0aW5nLgotICovCi12b2lkCi1DaGVja1N1YnNj
cmlwdGlvblJlbGtpbmQoY2hhciByZWxraW5kLCBjb25zdCBjaGFyICpuc3BuYW1lLAotCQkJCQkJ
IGNvbnN0IGNoYXIgKnJlbG5hbWUpCitzdGF0aWMgdm9pZAorQ2hlY2tTdWJzY3JpcHRpb25SZWxr
aW5kKFJlbGF0aW9uIHJlbCwgY29uc3QgY2hhciAqbnNwbmFtZSwgY29uc3QgY2hhciAqcmVsbmFt
ZSkKIHsKKwljaGFyIHJlbGtpbmQgPSByZWwtPnJkX3JlbC0+cmVsa2luZDsKKwogCWlmIChyZWxr
aW5kICE9IFJFTEtJTkRfUkVMQVRJT04gJiYgcmVsa2luZCAhPSBSRUxLSU5EX1BBUlRJVElPTkVE
X1RBQkxFKQorCXsKKwkJaWYgKHJlbG5hbWUgPT0gTlVMTCkKKwkJCXJlbG5hbWUgPSBSZWxhdGlv
bkdldFJlbGF0aW9uTmFtZShyZWwpOworCQlpZiAobnNwbmFtZSA9PSBOVUxMKQorCQkJbnNwbmFt
ZSA9IGdldF9uYW1lc3BhY2VfbmFtZShSZWxhdGlvbkdldE5hbWVzcGFjZShyZWwpKTsKIAkJZXJl
cG9ydChFUlJPUiwKIAkJCQkoZXJyY29kZShFUlJDT0RFX1dST05HX09CSkVDVF9UWVBFKSwKIAkJ
CQkgZXJybXNnKCJjYW5ub3QgdXNlIHJlbGF0aW9uIFwiJXMuJXNcIiBhcyBsb2dpY2FsIHJlcGxp
Y2F0aW9uIHRhcmdldCIsCiAJCQkJCQluc3BuYW1lLCByZWxuYW1lKSwKIAkJCQkgZXJyZGV0YWls
X3JlbGtpbmRfbm90X3N1cHBvcnRlZChyZWxraW5kKSkpOworCX0KK30KKworLyoKKyAqIFJlY3Vy
c2l2ZWx5IGNoZWNrIGlmIHdlIHN1cHBvcnQgd3JpdGluZyBpbnRvIHNwZWNpZmljIHJlbGtpbmQu
CisgKgorICogVGhlIG5zcG5hbWUgYW5kIHJlbG5hbWUgYXJlIG9ubHkgbmVlZGVkIGZvciBlcnJv
ciByZXBvcnRpbmcuCisgKi8KK3ZvaWQKK0NoZWNrU3Vic2NyaXB0aW9uUmVsYXRpb24oUmVsYXRp
b24gcmVsLCBjb25zdCBjaGFyICpuc3BuYW1lLAorCQkJCQkJICBjb25zdCBjaGFyICpyZWxuYW1l
KQoreworCUNoZWNrU3Vic2NyaXB0aW9uUmVsa2luZChyZWwsIG5zcG5hbWUsIHJlbG5hbWUpOwor
CisJaWYgKHJlbC0+cmRfcmVsLT5yZWxraW5kID09IFJFTEtJTkRfUEFSVElUSU9ORURfVEFCTEUp
CisJeworCQlMaXN0ICppbmhlcml0b3JzOworCQlMaXN0Q2VsbCAqbGM7CisKKwkJaW5oZXJpdG9y
cyA9IGZpbmRfYWxsX2luaGVyaXRvcnMoUmVsYXRpb25HZXRSZWxpZChyZWwpLAorCQkJCQkJCQkJ
CSBBY2Nlc3NTaGFyZUxvY2ssCisJCQkJCQkJCQkJIE5VTEwpOworCQlmb3JlYWNoIChsYywgaW5o
ZXJpdG9ycykKKwkJeworCQkJT2lkIGNoaWxkX29pZCA9IGxmaXJzdF9vaWQobGMpOworCQkJUmVs
YXRpb24gY2hpbGRfcmVsID0gUmVsYXRpb25JZEdldFJlbGF0aW9uKGNoaWxkX29pZCk7CisKKwkJ
CUNoZWNrU3Vic2NyaXB0aW9uUmVsa2luZChjaGlsZF9yZWwsIE5VTEwsIE5VTEwpOworCQkJUmVs
YXRpb25DbG9zZShjaGlsZF9yZWwpOworCQl9CisJfQogfQpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tl
bmQvcmVwbGljYXRpb24vbG9naWNhbC9yZWxhdGlvbi5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRp
b24vbG9naWNhbC9yZWxhdGlvbi5jCmluZGV4IGU5ODkwNDc2ODEuLmMxZWY3OTkwODQgMTAwNjQ0
Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvcmVsYXRpb24uYworKysgYi9z
cmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3JlbGF0aW9uLmMKQEAgLTM5Miw5ICszOTIs
OSBAQCBsb2dpY2FscmVwX3JlbF9vcGVuKExvZ2ljYWxSZXBSZWxJZCByZW1vdGVpZCwgTE9DS01P
REUgbG9ja21vZGUpCiAJCWVudHJ5LT5sb2NhbHJlbCA9IHRhYmxlX29wZW4ocmVsaWQsIE5vTG9j
ayk7CiAJCWVudHJ5LT5sb2NhbHJlbG9pZCA9IHJlbGlkOwogCi0JCS8qIENoZWNrIGZvciBzdXBw
b3J0ZWQgcmVsa2luZC4gKi8KLQkJQ2hlY2tTdWJzY3JpcHRpb25SZWxraW5kKGVudHJ5LT5sb2Nh
bHJlbC0+cmRfcmVsLT5yZWxraW5kLAotCQkJCQkJCQkgcmVtb3RlcmVsLT5uc3BuYW1lLCByZW1v
dGVyZWwtPnJlbG5hbWUpOworCQkvKiBDaGVjayBmb3Igc3VwcG9ydGVkIHJlbGtpbmQgcmVjdXJz
aXZlbHkuICovCisJCUNoZWNrU3Vic2NyaXB0aW9uUmVsYXRpb24oZW50cnktPmxvY2FscmVsLAor
CQkJCQkJCQkgIHJlbW90ZXJlbC0+bnNwbmFtZSwgcmVtb3RlcmVsLT5yZWxuYW1lKTsKIAogCQkv
KgogCQkgKiBCdWlsZCB0aGUgbWFwcGluZyBvZiBsb2NhbCBhdHRyaWJ1dGUgbnVtYmVycyB0byBy
ZW1vdGUgYXR0cmlidXRlCmRpZmYgLS1naXQgYS9zcmMvaW5jbHVkZS9leGVjdXRvci9leGVjdXRv
ci5oIGIvc3JjL2luY2x1ZGUvZXhlY3V0b3IvZXhlY3V0b3IuaAppbmRleCBlZDk1ZWQxMTc2Li45
M2RlOTI4NjRmIDEwMDY0NAotLS0gYS9zcmMvaW5jbHVkZS9leGVjdXRvci9leGVjdXRvci5oCisr
KyBiL3NyYy9pbmNsdWRlL2V4ZWN1dG9yL2V4ZWN1dG9yLmgKQEAgLTY0Niw4ICs2NDYsOCBAQCBl
eHRlcm4gdm9pZCBFeGVjU2ltcGxlUmVsYXRpb25EZWxldGUoUmVzdWx0UmVsSW5mbyAqcmVzdWx0
UmVsSW5mbywKIAkJCQkJCQkJCSBUdXBsZVRhYmxlU2xvdCAqc2VhcmNoc2xvdCk7CiBleHRlcm4g
dm9pZCBDaGVja0NtZFJlcGxpY2FJZGVudGl0eShSZWxhdGlvbiByZWwsIENtZFR5cGUgY21kKTsK
IAotZXh0ZXJuIHZvaWQgQ2hlY2tTdWJzY3JpcHRpb25SZWxraW5kKGNoYXIgcmVsa2luZCwgY29u
c3QgY2hhciAqbnNwbmFtZSwKLQkJCQkJCQkJCSBjb25zdCBjaGFyICpyZWxuYW1lKTsKK2V4dGVy
biB2b2lkIENoZWNrU3Vic2NyaXB0aW9uUmVsYXRpb24oUmVsYXRpb24gcmVsLCBjb25zdCBjaGFy
ICpuc3BuYW1lLAorCQkJCQkJCQkJICBjb25zdCBjaGFyICpyZWxuYW1lKTsKIAogLyoKICAqIHBy
b3RvdHlwZXMgZnJvbSBmdW5jdGlvbnMgaW4gbm9kZU1vZGlmeVRhYmxlLmMKLS0gCjIuMzAuMgoK


--=-FdM96811mkSwLF812UVo--