Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 5 Mar 2024 12:44:43 +0900
From: Michael Paquier <michael@paquier.xyz>
To: jian he <jian.universality@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel
 index build
Message-ID: <ZeaVK_J8b29Gxv_P@paquier.xyz>
References: <18314-5f44050ce33a5492@postgresql.org>
 <CAHewXNkPK9F-0fQ8komhJ_fW6Sk=QL4YDvTjVZwqBM2ZEzTd+A@mail.gmail.com>
 <CAHewXNkSYVYktt0KW8RtfNMDYi4Stn7OJtuTqeo+c3He1xoFXQ@mail.gmail.com>
 <CACJufxFFmFKeLBgQfwyGCEoTyEoksXgbJ0uWc=whPu5fk=LXwA@mail.gmail.com>
 <CACJufxEGcGYg62qMzfkkx_0v5QWvM-vMwz9166hDpV8Emi0OZw@mail.gmail.com>
 <CACJufxGG=sjUSQnWOcYz+rhdGaP9OdfrEGa2K1d8krGuUn5B4w@mail.gmail.com>
 <CACJufxHYMSt-irv+unpoAN76WDKzSRxzwOALG3ares52Lvtuug@mail.gmail.com>
 <CAHewXN=vHTbjdUmz8roJFN4MkO6hhGksve06wfpCAykz-F2WTQ@mail.gmail.com>
 <CACJufxFzy4A8Pbk-TtVa1+cjx9Kf09m+7EOEAYwN0ex4mZEjSA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="iwN/rN/xJUAKGiHO"
Content-Disposition: inline
In-Reply-To: 
 <CACJufxFzy4A8Pbk-TtVa1+cjx9Kf09m+7EOEAYwN0ex4mZEjSA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZeaVK_J8b29Gxv_P%40paquier.xyz>
Precedence: bulk


--iwN/rN/xJUAKGiHO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Feb 29, 2024 at 05:17:41PM +0800, jian he wrote:
> if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
> !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index, true)) ||
> !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index, true)))
> {
> parallel_workers = 0;
> goto done;
> }

Interesting bug.

> Overall it looks good.

I disagree with this idea.  Your patch is creating a shortcut in the
relcache code to retrieve data about index predicates and expressions
that make it bypass the flattening that would be done in
eval_const_expressions().  Giving the option to bypass that in the
relcache is a very bad idea, because flattening of the expressions for
the planner is not an option: it must happen.  So, I fear that your
patch could cause bugs if we introduce new code that calls
RelationGetIndexExpressions() with an incorrect option.  Not
documenting the new option is not acceptable either, with only one
comment added at the top of plan_create_index_workers().

On top of that, your approach has two bugs:
RelationGetIndexExpressions and RelationGetIndexPredicate would return
the flattened information if available directly from the relcache, so
even if you pass get_raw_expr=true you may finish with flattened
expressions and/or predicates, facing the same issues.

I think that the correct way to do that would be to get the
information from the syscache when calculating the number of parallel
workers to use for the parallel index builds, with SysCacheGetAttr(),
for example.  That would ensure that the expressions are not
flattened, letting the relcache be.

By the way, this reminds me of 7cce159349cc and its thread for a bug
fix that I did for REINDEX CONCURRENTLY a couple of years ago.  The
mistake is mostly the same:
https://www.postgresql.org/message-id/CA%2Bu7OA5Hp0ra235F3czPom_FyAd-3%2BXwSJmX95r1%2BsRPOJc9VQ%40mail.gmail.com
--
Michael

--iwN/rN/xJUAKGiHO
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmXmlSsACgkQnvQgOdby
QH1bORAAmlzL1QzgCG4m84VJEJwdqXVfNbr2fIzX7ydG7IVcN4oHldvUsLbMKTIR
fKpxgD3UBiMkBXYJtW3yUTXAe0iLgXai/rPWO+uufqAS6DNU2FkjDbMPGDP0+lWt
HWiblvk0Gz770d3CMNPbG1seULIKSWzq3AJur9AX9mSkAKcmNigEf2I8CgfgFjYU
o0smM01uhyfF3HAtGPdPSH45KquZOprGphC7sciB5NuF5dyXtzF1gtiu5Xctftd0
f+io0SDnxO9PSPoYbZTBfi5I5+z1VPzdvJKK6Q9phUvdb9Tmo8QKwJagFXB2jCeI
uq2NzcnKqGqePCt2bJu1FiSTpdKKgEeFoDX4Md6Tt17hNB0bRGuk6AVYjwl17X2J
88q0wmV6X/u/0JGxhUq5E3/CATMvXwWelirFtQnBN25Cps9LkALssBPtJAsZ6x+n
DYzw5G6hVRO7fNviFkElfFW1Lj/T0F+np91JUk+OcdCcpty4KlMa94pxjIsqNZVy
Tj3lu961i4ML2Gb6BUpr3/lbdNuYBWKoVAUvUrHJBA0MK0FHbg06bdabE6untghD
8j5gInuAlaAhCjKy5iNCQ26aJHrHGTxY+7S2lHc2G73trZQF/m+Huy0D7G3eQT7w
9gawq61HBhkZhJpHl1SJTpKUbOscbIHGItN6WnXt2VoULmE2AZk=
=4cBI
-----END PGP SIGNATURE-----

--iwN/rN/xJUAKGiHO--