Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
CheckNNConstraintFetch: Fill all of ConstrCheck in a single pass
- 94f95d91b025 19 (unreleased) landed
-
Don't error out when dropping constraint if relchecks is already zero
- d5845aa8adb2 19 (unreleased) landed
-
Avoid overwiriting cache entry (src/backend/utils/cache/relcache.c)
Ranier Vilela <ranier.vf@gmail.com> — 2025-08-26T14:17:33Z
Hi. 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. Fix by moving the array fill. patch attached. best regards, Ranier Vilela
-
Re: Avoid overwiriting cache entry (src/backend/utils/cache/relcache.c)
Álvaro Herrera <alvherre@kurilemu.de> — 2025-08-29T10:58:52Z
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/ -
Re: Avoid overwiriting cache entry (src/backend/utils/cache/relcache.c)
Ranier Vilela <ranier.vf@gmail.com> — 2025-08-29T16:48:40Z
Em sex., 29 de ago. de 2025 às 07:58, Álvaro Herrera <alvherre@kurilemu.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 -
Re: Avoid overwiriting cache entry (src/backend/utils/cache/relcache.c)
Ranier Vilela <ranier.vf@gmail.com> — 2025-10-29T11:54:30Z
Thanks Álvaro, for the commit. Closed the CF entry. best regards, Ranier Vilela