Assertion on create index concurrently

Started by Andrei Lepikhovalmost 5 years ago5 messagesbugs
Jump to latest
#1Andrei Lepikhov
lepihov@gmail.com

I found one annoying assertion in the CREATE INDEX CONCURRENTLY
operation. It will happen if you add into WHERE part of CREATE INDEX
CONCURRENTLY some function that will make transactional update (see an
example in the attached patch).

It looks like a hack, but it lead to assertion and restart of the
instance, that isn't good.
I suggest to perform error, not assertion in this case (see the patch).

This problem affects PG 9.6 - 13. The master branch was fixed 9 months
ago by commit 83158f7, that made this operation transactional, but still.

--
regards,
Andrey Lepikhov
Postgres Professional

Attachments:

0001-Use-more-safe-procedure-to-check-a-non-transactional.patchtext/plain; charset=UTF-8; name=0001-Use-more-safe-procedure-to-check-a-non-transactional.patch; x-mac-creator=0; x-mac-type=0Download+36-3
#2Michael Paquier
michael@paquier.xyz
In reply to: Andrei Lepikhov (#1)
Re: Assertion on create index concurrently

On Sat, Jun 26, 2021 at 09:48:45PM +0300, Andrey Lepikhov wrote:

I found one annoying assertion in the CREATE INDEX CONCURRENTLY operation.
It will happen if you add into WHERE part of CREATE INDEX CONCURRENTLY some
function that will make transactional update (see an example in the attached
patch).

It looks like a hack, but it lead to assertion and restart of the instance,
that isn't good.
I suggest to perform error, not assertion in this case (see the patch).

This problem affects PG 9.6 - 13. The master branch was fixed 9 months ago
by commit 83158f7, that made this operation transactional, but still.

This looks like a good argument for back-patching 83158f7 all the way
down, as 813fb03 exists since 9.4. Any thoughts from others?
--
Michael

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Michael Paquier (#2)
Re: Assertion on create index concurrently

Michael Paquier <michael@paquier.xyz> writes:

On Sat, Jun 26, 2021 at 09:48:45PM +0300, Andrey Lepikhov wrote:

This problem affects PG 9.6 - 13. The master branch was fixed 9 months ago
by commit 83158f7, that made this operation transactional, but still.

This looks like a good argument for back-patching 83158f7 all the way
down, as 813fb03 exists since 9.4. Any thoughts from others?

+1 ... 83158f7 has survived long enough to have reasonable confidence
in it.

regards, tom lane

#4Andrei Lepikhov
lepihov@gmail.com
In reply to: Tom Lane (#3)
Re: Assertion on create index concurrently

On 27/6/21 18:37, Tom Lane wrote:

Michael Paquier <michael@paquier.xyz> writes:

On Sat, Jun 26, 2021 at 09:48:45PM +0300, Andrey Lepikhov wrote:

This problem affects PG 9.6 - 13. The master branch was fixed 9 months ago
by commit 83158f7, that made this operation transactional, but still.

This looks like a good argument for back-patching 83158f7 all the way
down, as 813fb03 exists since 9.4. Any thoughts from others?

+1 ... 83158f7 has survived long enough to have reasonable confidence
in it.

Technically, 83158f7 is trivial patch. I think, some efforts is required
only for PG9.6. I prepared a patch for this version.

--
regards,
Andrey Lepikhov
Postgres Professional

Attachments:

0001-Back-patch-of-83158f7-for-PG9_6.patchtext/plain; charset=UTF-8; name=0001-Back-patch-of-83158f7-for-PG9_6.patch; x-mac-creator=0; x-mac-type=0Download+8-16
#5Michael Paquier
michael@paquier.xyz
In reply to: Andrei Lepikhov (#4)
Re: Assertion on create index concurrently

On Sun, Jun 27, 2021 at 10:33:44PM +0300, Andrey Lepikhov wrote:

Technically, 83158f7 is trivial patch. I think, some efforts is required
only for PG9.6. I prepared a patch for this version.

Thanks. This version had two problems:
- The comment on top of index_set_state_flags() was not updated, still
mentioning CatalogTupleUpdate().
- CatalogUpdateIndexes() does the job for the index updates, and using
it is more consistent with the other areas of index.c in
REL9_6_STABLE.

Just to keep a cleaner history, I have split your test case into a
separate commit, and applied that and the backpatch of 83158f7.
--
Michael