Message-ID: <5bafee07d0e1b3ce5359d1656c90b4836d7cb6a1.camel@gmail.com>
Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>, Alexander Pyhalov
	 <a.pyhalov@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org, Masahiko Sawada
 <sawada.mshk@gmail.com>,  Michael Paquier <michael@paquier.xyz>,
 =?UTF-8?Q?=E6=9D=8E=E6=9D=B0=28=E6=85=8E=E8=BF=BD=29?=
 <adger.lj@alibaba-inc.com>,
 =?UTF-8?Q?=E6=9B=BE=E6=96=87=E6=97=8C=28=E4=B9=89=E4=BB=8E=29?=
 <wenjing.zwj@alibaba-inc.com>, Alvaro Herrera <alvherre@2ndquadrant.com>
Date: Sat, 03 Dec 2022 19:13:30 +0400
In-Reply-To: <20221121030011.GU11463@telsasoft.com>
References: <20201031063117.GF3080@telsasoft.com>
	 <20201129202221.GA9477@telsasoft.com>
	 <CAD21AoCHYE5=PU7kJGGUHZuYVrHTHqmpUxN+8-rLGYpdgPwJ9g@mail.gmail.com>
	 <20210128143013.GC7450@telsasoft.com>
	 <be8ff634-72c6-3fc7-9175-5a6b974af4a9@postgrespro.ru>
	 <20210226182019.GU20769@telsasoft.com>
	 <04657227cc37b6353cf0c72bedac70cc@postgrespro.ru>
	 <20220628183309.GD28130@telsasoft.com>
	 <bec008ae632d54b3d81af2ca86419732@postgrespro.ru>
	 <20221121030011.GU11463@telsasoft.com>
Content-Type: multipart/mixed; boundary="=-jPDZ0owIUYK2uuLznVbI"
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/5bafee07d0e1b3ce5359d1656c90b4836d7cb6a1.camel%40gmail.com>
Precedence: bulk


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

Hi,

Thank you Justin and Alexander for working on this, I have reviewed and
tested the latest patch, it works well, the problems mentioned
previously are all fixed. I like the idea of sharing code of reindex
and index, but I have noticed some peculiarities as a user. 

The reporting is somewhat confusing as it switches to reporting for
reindex concurrently while building child indexes, this should be fixed
with the simple patch I have attached. Another thing that I have
noticed is that REINDEX, which is used under the hood, creates new
indexes with suffix _ccnew, and if the index building fails, the
indexes that could not be build will have the name with _ccnew suffix.
This can actually be seen in your test:

ERROR:  could not create unique index "idxpart2_a_idx2_ccnew"

I find it quite confusing and I don't think that this the expected
behavior (if it is, I think it should be documented, like it is for
REINDEX). As an example of problems that it might entail, DROP INDEX
will not drop all the invalid indexes in the inheritance tree, because
it will leave _ccnew indexes in place, which is ok for reindex
concurrently, but that's not how C-I-C works now. I think that fixing
this problem requires some heavy code rewrite and I'm not quite sure
how to go about it, if you have any ideas, I will be happy to try them
out.


Thanks,
Ilya

--=-jPDZ0owIUYK2uuLznVbI
Content-Disposition: attachment;
 filename="0001-turn-off-reindex-reporting-for-create.patch"
Content-Type: text/x-patch;
 name="0001-turn-off-reindex-reporting-for-create.patch";
	charset="UTF-8"
Content-Transfer-Encoding: base64

RnJvbSA4ZWI5ZmQ3Y2U3ZDM0YzVjMzIzYzQ3YjYwYTdmODgzZjM2MGVmMDkwIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBJbHlhIEdsYWR5c2hldiA8aWx5YS52LmdsYWR5c2hldkBnbWFp
bC5jb20+CkRhdGU6IFNhdCwgMyBEZWMgMjAyMiAxODoyMDowMyArMDQwMApTdWJqZWN0OiBbUEFU
Q0hdIHR1cm4gb2ZmIHJlaW5kZXggcmVwb3J0aW5nIGZvciBjcmVhdGUKCi0tLQogc3JjL2JhY2tl
bmQvY29tbWFuZHMvaW5kZXhjbWRzLmMgfCAzNiArKysrKysrKysrKysrKysrKysrLS0tLS0tLS0t
LS0tLQogMSBmaWxlIGNoYW5nZWQsIDIxIGluc2VydGlvbnMoKyksIDE1IGRlbGV0aW9ucygtKQoK
ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL2luZGV4Y21kcy5jIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvaW5kZXhjbWRzLmMKaW5kZXggYTI3NzU5MzFlMi4uYjNjNzEzMDM3ZiAxMDA2
NDQKLS0tIGEvc3JjL2JhY2tlbmQvY29tbWFuZHMvaW5kZXhjbWRzLmMKKysrIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvaW5kZXhjbWRzLmMKQEAgLTM4MDQsMTQgKzM4MDQsMTYgQEAgUmVpbmRleFJl
bGF0aW9uQ29uY3VycmVudGx5KE9pZCByZWxhdGlvbk9pZCwgUmVpbmRleFBhcmFtcyAqcGFyYW1z
KQogCiAJCS8qIERvbid0IG92ZXJ3cml0ZSBDUkVBVEUgSU5ERVggY29tbWFuZCAqLwogCQlpZiAo
IShwYXJhbXMtPm9wdGlvbnMgJiBSRUlOREVYT1BUX1JFUE9SVF9DUkVBVEVfUEFSVCkpCisJCXsK
IAkJCXBnc3RhdF9wcm9ncmVzc19zdGFydF9jb21tYW5kKFBST0dSRVNTX0NPTU1BTkRfQ1JFQVRF
X0lOREVYLAogCQkJCQkJCQkJICBpZHgtPnRhYmxlSWQpOwogCi0JCXByb2dyZXNzX3ZhbHNbMF0g
PSBQUk9HUkVTU19DUkVBVEVJRFhfQ09NTUFORF9SRUlOREVYX0NPTkNVUlJFTlRMWTsKLQkJcHJv
Z3Jlc3NfdmFsc1sxXSA9IDA7CS8qIGluaXRpYWxpemluZyAqLwotCQlwcm9ncmVzc192YWxzWzJd
ID0gaWR4LT5pbmRleElkOwotCQlwcm9ncmVzc192YWxzWzNdID0gaWR4LT5hbUlkOwotCQlwZ3N0
YXRfcHJvZ3Jlc3NfdXBkYXRlX211bHRpX3BhcmFtKDQsIHByb2dyZXNzX2luZGV4LCBwcm9ncmVz
c192YWxzKTsKKwkJCXByb2dyZXNzX3ZhbHNbMF0gPSBQUk9HUkVTU19DUkVBVEVJRFhfQ09NTUFO
RF9SRUlOREVYX0NPTkNVUlJFTlRMWTsKKwkJCXByb2dyZXNzX3ZhbHNbMV0gPSAwOwkvKiBpbml0
aWFsaXppbmcgKi8KKwkJCXByb2dyZXNzX3ZhbHNbMl0gPSBpZHgtPmluZGV4SWQ7CisJCQlwcm9n
cmVzc192YWxzWzNdID0gaWR4LT5hbUlkOworCQkJcGdzdGF0X3Byb2dyZXNzX3VwZGF0ZV9tdWx0
aV9wYXJhbSg0LCBwcm9ncmVzc19pbmRleCwgcHJvZ3Jlc3NfdmFscyk7CisJCX0KIAogCQkvKiBD
aG9vc2UgYSB0ZW1wb3JhcnkgcmVsYXRpb24gbmFtZSBmb3IgdGhlIG5ldyBpbmRleCAqLwogCQlj
b25jdXJyZW50TmFtZSA9IENob29zZVJlbGF0aW9uTmFtZShnZXRfcmVsX25hbWUoaWR4LT5pbmRl
eElkKSwKQEAgLTM5NjcsMTMgKzM5NjksMTUgQEAgUmVpbmRleFJlbGF0aW9uQ29uY3VycmVudGx5
KE9pZCByZWxhdGlvbk9pZCwgUmVpbmRleFBhcmFtcyAqcGFyYW1zKQogCQkgKiB0YWJsZSBpbnZv
bHZlZC4gIERvbid0IG92ZXJ3cml0ZSBDUkVBVEUgSU5ERVggY29tbWFuZC4KIAkJICovCiAJCWlm
ICghKHBhcmFtcy0+b3B0aW9ucyAmIFJFSU5ERVhPUFRfUkVQT1JUX0NSRUFURV9QQVJUKSkKKwkJ
ewogCQkJcGdzdGF0X3Byb2dyZXNzX3N0YXJ0X2NvbW1hbmQoUFJPR1JFU1NfQ09NTUFORF9DUkVB
VEVfSU5ERVgsIG5ld2lkeC0+dGFibGVJZCk7CiAKLQkJcHJvZ3Jlc3NfdmFsc1swXSA9IFBST0dS
RVNTX0NSRUFURUlEWF9DT01NQU5EX1JFSU5ERVhfQ09OQ1VSUkVOVExZOwotCQlwcm9ncmVzc192
YWxzWzFdID0gUFJPR1JFU1NfQ1JFQVRFSURYX1BIQVNFX0JVSUxEOwotCQlwcm9ncmVzc192YWxz
WzJdID0gbmV3aWR4LT5pbmRleElkOwotCQlwcm9ncmVzc192YWxzWzNdID0gbmV3aWR4LT5hbUlk
OwotCQlwZ3N0YXRfcHJvZ3Jlc3NfdXBkYXRlX211bHRpX3BhcmFtKDQsIHByb2dyZXNzX2luZGV4
LCBwcm9ncmVzc192YWxzKTsKKwkJCXByb2dyZXNzX3ZhbHNbMF0gPSBQUk9HUkVTU19DUkVBVEVJ
RFhfQ09NTUFORF9SRUlOREVYX0NPTkNVUlJFTlRMWTsKKwkJCXByb2dyZXNzX3ZhbHNbMV0gPSBQ
Uk9HUkVTU19DUkVBVEVJRFhfUEhBU0VfQlVJTEQ7CisJCQlwcm9ncmVzc192YWxzWzJdID0gbmV3
aWR4LT5pbmRleElkOworCQkJcHJvZ3Jlc3NfdmFsc1szXSA9IG5ld2lkeC0+YW1JZDsKKwkJCXBn
c3RhdF9wcm9ncmVzc191cGRhdGVfbXVsdGlfcGFyYW0oNCwgcHJvZ3Jlc3NfaW5kZXgsIHByb2dy
ZXNzX3ZhbHMpOworCQl9CiAKIAkJLyogUGVyZm9ybSBjb25jdXJyZW50IGJ1aWxkIG9mIG5ldyBp
bmRleCAqLwogCQlpbmRleF9jb25jdXJyZW50bHlfYnVpbGQobmV3aWR4LT50YWJsZUlkLCBuZXdp
ZHgtPmluZGV4SWQpOwpAQCAtNDAzMywxNCArNDAzNywxNiBAQCBSZWluZGV4UmVsYXRpb25Db25j
dXJyZW50bHkoT2lkIHJlbGF0aW9uT2lkLCBSZWluZGV4UGFyYW1zICpwYXJhbXMpCiAJCSAqIHRh
YmxlIGludm9sdmVkLiBEb24ndCBvdmVyd3JpdGUgQ1JFQVRFIElOREVYIGNvbW1hbmQuCiAJCSAq
LwogCQlpZiAoIShwYXJhbXMtPm9wdGlvbnMgJiBSRUlOREVYT1BUX1JFUE9SVF9DUkVBVEVfUEFS
VCkpCisJCXsKIAkJCXBnc3RhdF9wcm9ncmVzc19zdGFydF9jb21tYW5kKFBST0dSRVNTX0NPTU1B
TkRfQ1JFQVRFX0lOREVYLAogCQkJCQkJCQkJICBuZXdpZHgtPnRhYmxlSWQpOwogCi0JCXByb2dy
ZXNzX3ZhbHNbMF0gPSBQUk9HUkVTU19DUkVBVEVJRFhfQ09NTUFORF9SRUlOREVYX0NPTkNVUlJF
TlRMWTsKLQkJcHJvZ3Jlc3NfdmFsc1sxXSA9IFBST0dSRVNTX0NSRUFURUlEWF9QSEFTRV9WQUxJ
REFURV9JRFhTQ0FOOwotCQlwcm9ncmVzc192YWxzWzJdID0gbmV3aWR4LT5pbmRleElkOwotCQlw
cm9ncmVzc192YWxzWzNdID0gbmV3aWR4LT5hbUlkOwotCQlwZ3N0YXRfcHJvZ3Jlc3NfdXBkYXRl
X211bHRpX3BhcmFtKDQsIHByb2dyZXNzX2luZGV4LCBwcm9ncmVzc192YWxzKTsKKwkJCXByb2dy
ZXNzX3ZhbHNbMF0gPSBQUk9HUkVTU19DUkVBVEVJRFhfQ09NTUFORF9SRUlOREVYX0NPTkNVUlJF
TlRMWTsKKwkJCXByb2dyZXNzX3ZhbHNbMV0gPSBQUk9HUkVTU19DUkVBVEVJRFhfUEhBU0VfVkFM
SURBVEVfSURYU0NBTjsKKwkJCXByb2dyZXNzX3ZhbHNbMl0gPSBuZXdpZHgtPmluZGV4SWQ7CisJ
CQlwcm9ncmVzc192YWxzWzNdID0gbmV3aWR4LT5hbUlkOworCQkJcGdzdGF0X3Byb2dyZXNzX3Vw
ZGF0ZV9tdWx0aV9wYXJhbSg0LCBwcm9ncmVzc19pbmRleCwgcHJvZ3Jlc3NfdmFscyk7CisJCX0K
IAogCQl2YWxpZGF0ZV9pbmRleChuZXdpZHgtPnRhYmxlSWQsIG5ld2lkeC0+aW5kZXhJZCwgc25h
cHNob3QpOwogCi0tIAoyLjMwLjIKCg==


--=-jPDZ0owIUYK2uuLznVbI--