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

Em sex., 29 de ago. de 2025 às 07:58, Álvaro Herrera <alvherre(at)kurilemu(dot)de>
escreveu:

> 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:
>
I think that the main point of the patch is to avoid losing a cache 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?

No.

> Can you find any crashers?
>
No.

>
>
> 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);
>
With this change and the patch applied the behavior is:

postgres=# create table bar (a int check (a > 0), b int constraint tssk
check (null));
CREATE TABLE
postgres=# update pg_constraint set conbin = null where conname = 'tssk';
UPDATE 1
postgres=# insert into bar values (-1);
WARNING: null conbin for relation "bar"
LINE 1: insert into bar values (-1);
^
WARNING: 1 pg_constraint record(s) missing for relation "bar"
LINE 1: insert into bar values (-1);
^
ERROR: 1 pg_constraint record(s) missing for relation "bar"
postgres=# alter table bar add check (b > 0);
WARNING: unexpected pg_constraint record found for relation "bar"
ALTER TABLE
postgres=# drop table bar;
WARNING: unexpected pg_constraint record found for relation "bar"
WARNING: unexpected pg_constraint record found for relation "bar"
WARNING: relation "bar" has relchecks = 0
DROP TABLE

>
>
> (There's another decrement for coninhcount in line 1155 of the same
> file. Maybe that deserves the same treatment, not sure.)
>
I'm not have an opinion about this.

best regards,
Ranier Vilela