Re: Avoid overwiriting cache entry (src/backend/utils/cache/relcache.c) - Mailing list pgsql-hackers

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.)