public inbox for [email protected]  

help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: Zsolt Parragi <[email protected]>
Cc: [email protected]
Subject: Re: Fix bug of CHECK constraint enforceability recursion
Date: Fri, 5 Jun 2026 08:35:56 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAN4CZFNjnR-7hPBO=v=9h4CdDogUueYoPqpPvR9k3X5Z9-=+TA@mail.gmail.com>
References: <[email protected]>
	<CACJufxGRkCSqqRwd+3MMVFbosKXHv=Jc+KyGK+NM3YAHN7eu+g@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CACJufxGxY=1mxcNz8-bjp6R+ZoCRE3O7DYt2ZqObv-Le+aMOqw@mail.gmail.com>
	<[email protected]>
	<CACJufxFwJH5K1jgL1ryckYX8gxp1sfqz_2ypey1sYtWZ5u45oQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CACJufxHbVuY683j3DHEU4LSswo8DTzLJbTvhv2Uj6gqN8pJs7Q@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CACJufxGgSKFGWja=JPv3-zq3whON5cESLp7Z5=OsYvZOaRGnGw@mail.gmail.com>
	<[email protected]>
	<CAN4CZFOOhDAWdnY1OXB1wfG_=C4g_dw0gC2HCApz_3F7EFpqJA@mail.gmail.com>
	<[email protected]>
	<CAN4CZFNjnR-7hPBO=v=9h4CdDogUueYoPqpPvR9k3X5Z9-=+TA@mail.gmail.com>



> On Jun 5, 2026, at 03:08, Zsolt Parragi <[email protected]> wrote:
> 
> The new version looks correct, I don't see any logic problem with it,
> however, I do have a performance question:
> 
> + /*
> + * A parent listed in changing_conids is being changed by the
> + * same ALTER, but it may not have been updated yet.  For
> + * regular inheritance, recurse upward to check whether an
> + * equivalent enforced parent outside the ALTER will make it
> + * remain enforced.  Partitions cannot have multiple parents,
> + * so they do not need this check.
> + */
> + if (!rel->rd_rel->relispartition &&
> + list_member_oid(changing_conids, parentcon->oid))
> 
> Shouldn't the parent lookup use some form of caching? Otherwise we'll
> end up reevaluating the same parents multiple times. I'm not sure if
> it is needed or not, how much of a performance impact this can have in
> a real-world server.

Actually, after sending v7, I spent several hours trying to cache update history, which could avoid some recurse-up lookups. But I was uncomfortable with the resulting code and gave up on that approach. There were several reasons:

* Re-evaluation only happens in an edge case where all of the following are true:
   - the constraint is altered from ENFORCED to NOT ENFORCED
   - the target is a regular inherited table, partitioned tables do not go through this logic
   - the inheritance graph is complex, as in your test case, only tables like e and f hit the re-evaluation case
   
* It is common for a partitioned table to have thousands of partitions, but I rarely hear of a regular inherited table having thousands of descendants.

* This re-evaluation only reads catalog tables. Compared with ALTER TABLE operations that rewrite table data, the cost should not be too bad.

* More importantly, I don’t think caching update results is the right solution. The root cause is that find_all_inheritors() cannot ensure that a child appears after all of its parents in the returned list. We could add an alternative helper that returns inheritance descendants in topological order. With that, the current recurse-up logic could be avoided, and maybe the changing_oids list could be avoided as well. But it is too late for v19. I have added this to my TODO list and will work on it for v20.

In summary, I think the current re-evaluation has very limited performance impact and is acceptable for v19. We can improve the algorithm with a better solution in v20.

BTW, do you have any comments on the doc changes in 0002 and 0003?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

view thread (32+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Fix bug of CHECK constraint enforceability recursion
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox