Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 091e22b

Browse filesBrowse files
tglsfdctglsfdc
committed
Clean up treatment of missing default and CHECK-constraint records.
Andrew Gierth reported that it's possible to crash the backend if no pg_attrdef record is found to match an attribute that has atthasdef set. AttrDefaultFetch warns about this situation, but then leaves behind a relation tupdesc that has null "adbin" pointer(s), which most places don't guard against. We considered promoting the warning to an error, but throwing errors during relcache load is pretty drastic: it effectively locks one out of using the relation at all. What seems better is to leave the load-time behavior as a warning, but then throw an error in any code path that wants to use a default and can't find it. This confines the error to a subset of INSERT/UPDATE operations on the table, and in particular will at least allow a pg_dump to succeed. Also, we should fix AttrDefaultFetch to not leave any null pointers in the tupdesc, because that just creates an untested bug hazard. While at it, apply the same philosophy of "warn at load, throw error only upon use of the known-missing info" to CHECK constraints. CheckConstraintFetch is very nearly the same logic as AttrDefaultFetch, but for reasons lost in the mists of time, it was throwing ERROR for the same cases that AttrDefaultFetch treats as WARNING. Make the two functions more nearly alike. In passing, get rid of potentially-O(N^2) loops in equalTupleDesc by making AttrDefaultFetch sort the entries after fetching them, so that equalTupleDesc can assume that entries in two equal tupdescs must be in matching order. (CheckConstraintFetch already was sorting CHECK constraints, but equalTupleDesc hadn't been told about it.) There's some argument for back-patching this, but with such a small number of field reports, I'm content to fix it in HEAD. Discussion: https://postgr.es/m/87pmzaq4gx.fsf@news-spur.riddles.org.uk
1 parent 9de9294 commit 091e22b
Copy full SHA for 091e22b

File tree

6 files changed

+173
-159
lines changed
Filter options

6 files changed

+173
-159
lines changed

‎src/backend/access/common/tupdesc.c

Copy file name to clipboardExpand all lines: src/backend/access/common/tupdesc.c
+22-46Lines changed: 22 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -174,10 +174,7 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
174174
cpy->defval = (AttrDefault *) palloc(cpy->num_defval * sizeof(AttrDefault));
175175
memcpy(cpy->defval, constr->defval, cpy->num_defval * sizeof(AttrDefault));
176176
for (i = cpy->num_defval - 1; i >= 0; i--)
177-
{
178-
if (constr->defval[i].adbin)
179-
cpy->defval[i].adbin = pstrdup(constr->defval[i].adbin);
180-
}
177+
cpy->defval[i].adbin = pstrdup(constr->defval[i].adbin);
181178
}
182179

183180
if (constr->missing)
@@ -203,10 +200,8 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
203200
memcpy(cpy->check, constr->check, cpy->num_check * sizeof(ConstrCheck));
204201
for (i = cpy->num_check - 1; i >= 0; i--)
205202
{
206-
if (constr->check[i].ccname)
207-
cpy->check[i].ccname = pstrdup(constr->check[i].ccname);
208-
if (constr->check[i].ccbin)
209-
cpy->check[i].ccbin = pstrdup(constr->check[i].ccbin);
203+
cpy->check[i].ccname = pstrdup(constr->check[i].ccname);
204+
cpy->check[i].ccbin = pstrdup(constr->check[i].ccbin);
210205
cpy->check[i].ccvalid = constr->check[i].ccvalid;
211206
cpy->check[i].ccnoinherit = constr->check[i].ccnoinherit;
212207
}
@@ -328,10 +323,7 @@ FreeTupleDesc(TupleDesc tupdesc)
328323
AttrDefault *attrdef = tupdesc->constr->defval;
329324

330325
for (i = tupdesc->constr->num_defval - 1; i >= 0; i--)
331-
{
332-
if (attrdef[i].adbin)
333-
pfree(attrdef[i].adbin);
334-
}
326+
pfree(attrdef[i].adbin);
335327
pfree(attrdef);
336328
}
337329
if (tupdesc->constr->missing)
@@ -352,10 +344,8 @@ FreeTupleDesc(TupleDesc tupdesc)
352344

353345
for (i = tupdesc->constr->num_check - 1; i >= 0; i--)
354346
{
355-
if (check[i].ccname)
356-
pfree(check[i].ccname);
357-
if (check[i].ccbin)
358-
pfree(check[i].ccbin);
347+
pfree(check[i].ccname);
348+
pfree(check[i].ccbin);
359349
}
360350
pfree(check);
361351
}
@@ -412,7 +402,6 @@ bool
412402
equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
413403
{
414404
int i,
415-
j,
416405
n;
417406

418407
if (tupdesc1->natts != tupdesc2->natts)
@@ -488,22 +477,13 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
488477
n = constr1->num_defval;
489478
if (n != (int) constr2->num_defval)
490479
return false;
480+
/* We assume here that both AttrDefault arrays are in adnum order */
491481
for (i = 0; i < n; i++)
492482
{
493483
AttrDefault *defval1 = constr1->defval + i;
494-
AttrDefault *defval2 = constr2->defval;
495-
496-
/*
497-
* We can't assume that the items are always read from the system
498-
* catalogs in the same order; so use the adnum field to identify
499-
* the matching item to compare.
500-
*/
501-
for (j = 0; j < n; defval2++, j++)
502-
{
503-
if (defval1->adnum == defval2->adnum)
504-
break;
505-
}
506-
if (j >= n)
484+
AttrDefault *defval2 = constr2->defval + i;
485+
486+
if (defval1->adnum != defval2->adnum)
507487
return false;
508488
if (strcmp(defval1->adbin, defval2->adbin) != 0)
509489
return false;
@@ -534,25 +514,21 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
534514
n = constr1->num_check;
535515
if (n != (int) constr2->num_check)
536516
return false;
517+
518+
/*
519+
* Similarly, we rely here on the ConstrCheck entries being sorted by
520+
* name. If there are duplicate names, the outcome of the comparison
521+
* is uncertain, but that should not happen.
522+
*/
537523
for (i = 0; i < n; i++)
538524
{
539525
ConstrCheck *check1 = constr1->check + i;
540-
ConstrCheck *check2 = constr2->check;
541-
542-
/*
543-
* Similarly, don't assume that the checks are always read in the
544-
* same order; match them up by name and contents. (The name
545-
* *should* be unique, but...)
546-
*/
547-
for (j = 0; j < n; check2++, j++)
548-
{
549-
if (strcmp(check1->ccname, check2->ccname) == 0 &&
550-
strcmp(check1->ccbin, check2->ccbin) == 0 &&
551-
check1->ccvalid == check2->ccvalid &&
552-
check1->ccnoinherit == check2->ccnoinherit)
553-
break;
554-
}
555-
if (j >= n)
526+
ConstrCheck *check2 = constr2->check + i;
527+
528+
if (!(strcmp(check1->ccname, check2->ccname) == 0 &&
529+
strcmp(check1->ccbin, check2->ccbin) == 0 &&
530+
check1->ccvalid == check2->ccvalid &&
531+
check1->ccnoinherit == check2->ccnoinherit))
556532
return false;
557533
}
558534
}

‎src/backend/commands/tablecmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/tablecmds.c
+12-9Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2501,21 +2501,24 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
25012501
if (attribute->atthasdef)
25022502
{
25032503
Node *this_default = NULL;
2504-
AttrDefault *attrdef;
2505-
int i;
25062504

25072505
/* Find default in constraint structure */
2508-
Assert(constr != NULL);
2509-
attrdef = constr->defval;
2510-
for (i = 0; i < constr->num_defval; i++)
2506+
if (constr != NULL)
25112507
{
2512-
if (attrdef[i].adnum == parent_attno)
2508+
AttrDefault *attrdef = constr->defval;
2509+
2510+
for (int i = 0; i < constr->num_defval; i++)
25132511
{
2514-
this_default = stringToNode(attrdef[i].adbin);
2515-
break;
2512+
if (attrdef[i].adnum == parent_attno)
2513+
{
2514+
this_default = stringToNode(attrdef[i].adbin);
2515+
break;
2516+
}
25162517
}
25172518
}
2518-
Assert(this_default != NULL);
2519+
if (this_default == NULL)
2520+
elog(ERROR, "default expression not found for attribute %d of relation \"%s\"",
2521+
parent_attno, RelationGetRelationName(relation));
25192522

25202523
/*
25212524
* If it's a GENERATED default, it might contain Vars that

‎src/backend/executor/execMain.c

Copy file name to clipboardExpand all lines: src/backend/executor/execMain.c
+10-1Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1609,6 +1609,15 @@ ExecRelCheck(ResultRelInfo *resultRelInfo,
16091609
MemoryContext oldContext;
16101610
int i;
16111611

1612+
/*
1613+
* CheckConstraintFetch let this pass with only a warning, but now we
1614+
* should fail rather than possibly failing to enforce an important
1615+
* constraint.
1616+
*/
1617+
if (ncheck != rel->rd_rel->relchecks)
1618+
elog(ERROR, "%d pg_constraint record(s) missing for relation \"%s\"",
1619+
rel->rd_rel->relchecks - ncheck, RelationGetRelationName(rel));
1620+
16121621
/*
16131622
* If first time through for this result relation, build expression
16141623
* nodetrees for rel's constraint expressions. Keep them in the per-query
@@ -1862,7 +1871,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
18621871
}
18631872
}
18641873

1865-
if (constr->num_check > 0)
1874+
if (rel->rd_rel->relchecks > 0)
18661875
{
18671876
const char *failed;
18681877

‎src/backend/parser/parse_utilcmd.c

Copy file name to clipboardExpand all lines: src/backend/parser/parse_utilcmd.c
+4-3Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,8 +1239,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
12391239
(CREATE_TABLE_LIKE_DEFAULTS | CREATE_TABLE_LIKE_GENERATED)) &&
12401240
constr != NULL)
12411241
{
1242-
AttrDefault *attrdef = constr->defval;
1243-
12441242
for (parent_attno = 1; parent_attno <= tupleDesc->natts;
12451243
parent_attno++)
12461244
{
@@ -1264,6 +1262,7 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
12641262
(table_like_clause->options & CREATE_TABLE_LIKE_DEFAULTS)))
12651263
{
12661264
Node *this_default = NULL;
1265+
AttrDefault *attrdef = constr->defval;
12671266
AlterTableCmd *atsubcmd;
12681267
bool found_whole_row;
12691268

@@ -1276,7 +1275,9 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
12761275
break;
12771276
}
12781277
}
1279-
Assert(this_default != NULL);
1278+
if (this_default == NULL)
1279+
elog(ERROR, "default expression not found for attribute %d of relation \"%s\"",
1280+
parent_attno, RelationGetRelationName(relation));
12801281

12811282
atsubcmd = makeNode(AlterTableCmd);
12821283
atsubcmd->subtype = AT_CookedColumnDefault;

‎src/backend/rewrite/rewriteHandler.c

Copy file name to clipboardExpand all lines: src/backend/rewrite/rewriteHandler.c
+16-13Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,25 +1228,28 @@ build_column_default(Relation rel, int attrno)
12281228
}
12291229

12301230
/*
1231-
* Scan to see if relation has a default for this column.
1231+
* If relation has a default for this column, fetch that expression.
12321232
*/
1233-
if (att_tup->atthasdef && rd_att->constr &&
1234-
rd_att->constr->num_defval > 0)
1233+
if (att_tup->atthasdef)
12351234
{
1236-
AttrDefault *defval = rd_att->constr->defval;
1237-
int ndef = rd_att->constr->num_defval;
1238-
1239-
while (--ndef >= 0)
1235+
if (rd_att->constr && rd_att->constr->num_defval > 0)
12401236
{
1241-
if (attrno == defval[ndef].adnum)
1237+
AttrDefault *defval = rd_att->constr->defval;
1238+
int ndef = rd_att->constr->num_defval;
1239+
1240+
while (--ndef >= 0)
12421241
{
1243-
/*
1244-
* Found it, convert string representation to node tree.
1245-
*/
1246-
expr = stringToNode(defval[ndef].adbin);
1247-
break;
1242+
if (attrno == defval[ndef].adnum)
1243+
{
1244+
/* Found it, convert string representation to node tree. */
1245+
expr = stringToNode(defval[ndef].adbin);
1246+
break;
1247+
}
12481248
}
12491249
}
1250+
if (expr == NULL)
1251+
elog(ERROR, "default expression not found for attribute %d of relation \"%s\"",
1252+
attrno, RelationGetRelationName(rel));
12501253
}
12511254

12521255
/*

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.