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

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