Re: Avoid overwiriting cache entry (src/backend/utils/cache/relcache.c)

On 2025-Aug-26, Ranier Vilela wrote:

> In function *CheckNNConstraintFetch* the array index is
> incremented only when "null combin" is not found.
>
> IMO the last cache entry can be overwriting if the next tuple is null,
> because, the fill array fields is not syncronized with array index
> increment.

Hmm, yeah this is wrong. However, If you do hit the case where conbin
is null the behavior is unpleasant but I don't see any crashers, and the
case would amount to catalog corruption. Overall I agree that it seems
better to not modify the entry at all unless we increment the counter,
which your patch achieves.

Not very excited about this though. For instance, I don't think we
reach the point of _using_ the corrupted entry:

=# create table bar (a int check (a > 0), b int constraint tssk check (null));
CREATE TABLE

=# update pg_constraint set conbin = null where conname = 'tssk';
UPDATE 1

=# insert into bar values (-1);
WARNING: null conbin for relation "bar"
LÍNEA 1: insert into bar values (-1);
^
WARNING: 1 pg_constraint record(s) missing for relation "bar"
LÍNEA 1: insert into bar values (-1);
^
ERROR: 1 pg_constraint record(s) missing for relation "bar"

The WARNINGs come from CheckNNConstraintFetch itself, but the error
comes from ExecRelCheck. Did you find a way to make the code behave
worse than this? Can you find any crashers?

Now consider this:

=# alter table bar add check (b > 0);
WARNING: unexpected pg_constraint record found for relation "bar"
ALTER TABLE

For some reason, here we set relchecks to 0, which makes no sense.
Which leads to

=# drop table bar;
WARNING: unexpected pg_constraint record found for relation "bar"
WARNING: unexpected pg_constraint record found for relation "bar"
ERROR: relation "bar" has relchecks = 0

This suggests that we should relax that error and make it a warning,
perhaps like

diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 6002fd0002f..9944e4bd2d1 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -937,10 +937,12 @@ RemoveConstraintById(Oid conId)
con->conrelid);
classForm = (Form_pg_class) GETSTRUCT(relTup);

- if (classForm->relchecks == 0) /* should not happen */
- elog(ERROR, "relation \"%s\" has relchecks = 0",
- RelationGetRelationName(rel));
- classForm->relchecks--;
+ if (classForm->relchecks > 0)
+ classForm->relchecks--;
+ else
+ /* should not happen */
+ elog(WARNING, "relation \"%s\" has relchecks = %d",
+ RelationGetRelationName(rel), classForm->relchecks);

CatalogTupleUpdate(pgrel, &relTup->t_self, relTup);

(There's another decrement for coninhcount in line 1155 of the same
file. Maybe that deserves the same treatment, not sure.)

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/