From fc032bed2fb809ee69eaf2fece349f65274c876f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Jul 2020 13:03:48 -0400 Subject: [PATCH] Be more careful about marking catalog columns NOT NULL by default. The bug fixed in commit 72eab84a5 would not have occurred if initdb had a less surprising rule about which columns should be marked NOT NULL by default. Let's make that rule be strictly that the column must be fixed-width and its predecessors must be fixed-width and NOT NULL, removing the hacky and unsafe exceptions for oidvector and int2vector. Since we do still want all existing oidvector and int2vector columns to be marked NOT NULL, we have to put BKI_FORCE_NOT_NULL labels on them. But making this less magic and more documented seems like a good idea, even if it's a shade more verbose. I didn't bump catversion since the initial catalog contents are not actually changed by this patch. Note however that the contents of postgres.bki do change, and feeding an old copy of that to a new backend will produce wrong results. Discussion: https://postgr.es/m/204760.1595181800@sss.pgh.pa.us --- doc/src/sgml/bki.sgml | 3 +- src/backend/bootstrap/bootstrap.c | 17 ++++------- src/backend/catalog/genbki.pl | 33 ++++++++++++---------- src/include/catalog/genbki.h | 4 +-- src/include/catalog/pg_index.h | 10 ++++--- src/include/catalog/pg_partitioned_table.h | 14 +++++---- src/include/catalog/pg_proc.h | 2 +- src/include/catalog/pg_statistic_ext.h | 2 +- src/include/catalog/pg_trigger.h | 3 +- 9 files changed, 46 insertions(+), 42 deletions(-) diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml index 4e7568f5ce..272a10a98c 100644 --- a/doc/src/sgml/bki.sgml +++ b/doc/src/sgml/bki.sgml @@ -119,7 +119,8 @@ require all columns that should be non-nullable to be marked so in pg_attribute. The bootstrap code will automatically mark catalog columns as NOT NULL - if they are fixed-width and are not preceded by any nullable column. + if they are fixed-width and are not preceded by any nullable or + variable-width column. Where this rule is inadequate, you can force correct marking by using BKI_FORCE_NOT_NULL and BKI_FORCE_NULL annotations as needed. diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 5480a024e0..45b7efbe46 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -770,25 +770,18 @@ DefineAttr(char *name, char *type, int attnum, int nullness) /* * Mark as "not null" if type is fixed-width and prior columns are - * too. This corresponds to case where column can be accessed - * directly via C struct declaration. - * - * oidvector and int2vector are also treated as not-nullable, even - * though they are no longer fixed-width. + * likewise fixed-width and not-null. This corresponds to case where + * column can be accessed directly via C struct declaration. */ -#define MARKNOTNULL(att) \ - ((att)->attlen > 0 || \ - (att)->atttypid == OIDVECTOROID || \ - (att)->atttypid == INT2VECTOROID) - - if (MARKNOTNULL(attrtypes[attnum])) + if (attrtypes[attnum]->attlen > 0) { int i; /* check earlier attributes */ for (i = 0; i < attnum; i++) { - if (!attrtypes[i]->attnotnull) + if (attrtypes[i]->attlen <= 0 || + !attrtypes[i]->attnotnull) break; } if (i == attnum) diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl index b07537fbba..dc5f442397 100644 --- a/src/backend/catalog/genbki.pl +++ b/src/backend/catalog/genbki.pl @@ -713,8 +713,8 @@ sub gen_pg_attribute push @tables_needing_macros, $table_name; # Generate entries for user attributes. - my $attnum = 0; - my $priornotnull = 1; + my $attnum = 0; + my $priorfixedwidth = 1; foreach my $attr (@{ $table->{columns} }) { $attnum++; @@ -722,8 +722,12 @@ sub gen_pg_attribute $row{attnum} = $attnum; $row{attrelid} = $table->{relation_oid}; - morph_row_for_pgattr(\%row, $schema, $attr, $priornotnull); - $priornotnull &= ($row{attnotnull} eq 't'); + morph_row_for_pgattr(\%row, $schema, $attr, $priorfixedwidth); + + # Update $priorfixedwidth --- must match morph_row_for_pgattr + $priorfixedwidth &= + ($row{attnotnull} eq 't' + && ($row{attlen} eq 'NAMEDATALEN' || $row{attlen} > 0)); # If it's bootstrapped, put an entry in postgres.bki. print_bki_insert(\%row, $schema) if $table->{bootstrap}; @@ -765,13 +769,13 @@ sub gen_pg_attribute # Given $pgattr_schema (the pg_attribute schema for a catalog sufficient for # AddDefaultValues), $attr (the description of a catalog row), and -# $priornotnull (whether all prior attributes in this catalog are not null), +# $priorfixedwidth (all prior columns are fixed-width and not null), # modify the $row hashref for print_bki_insert. This includes setting data # from the corresponding pg_type element and filling in any default values. # Any value not handled here must be supplied by caller. sub morph_row_for_pgattr { - my ($row, $pgattr_schema, $attr, $priornotnull) = @_; + my ($row, $pgattr_schema, $attr, $priorfixedwidth) = @_; my $attname = $attr->{name}; my $atttype = $attr->{type}; @@ -801,19 +805,18 @@ sub morph_row_for_pgattr { $row->{attnotnull} = 'f'; } - elsif ($priornotnull) + elsif ($priorfixedwidth) { # attnotnull will automatically be set if the type is - # fixed-width and prior columns are all NOT NULL --- - # compare DefineAttr in bootstrap.c. oidvector and - # int2vector are also treated as not-nullable. + # fixed-width and prior columns are likewise fixed-width + # and NOT NULL --- compare DefineAttr in bootstrap.c. + # At this point the width of type name is still symbolic, + # so we need a special test. $row->{attnotnull} = - $type->{typname} eq 'oidvector' ? 't' - : $type->{typname} eq 'int2vector' ? 't' - : $type->{typlen} eq 'NAMEDATALEN' ? 't' - : $type->{typlen} > 0 ? 't' - : 'f'; + $row->{attlen} eq 'NAMEDATALEN' ? 't' + : $row->{attlen} > 0 ? 't' + : 'f'; } else { diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h index 4a6c8636da..8cac7ec878 100644 --- a/src/include/catalog/genbki.h +++ b/src/include/catalog/genbki.h @@ -46,8 +46,8 @@ /* * Variable-length catalog fields (except possibly the first not nullable one) * should not be visible in C structures, so they are made invisible by #ifdefs - * of an undefined symbol. See also MARKNOTNULL in bootstrap.c for how this is - * handled. + * of an undefined symbol. See also the BOOTCOL_NULL_AUTO code in bootstrap.c + * for how this is handled. */ #undef CATALOG_VARLEN diff --git a/src/include/catalog/pg_index.h b/src/include/catalog/pg_index.h index d3d7ea77fb..4a642f336f 100644 --- a/src/include/catalog/pg_index.h +++ b/src/include/catalog/pg_index.h @@ -44,12 +44,14 @@ CATALOG(pg_index,2610,IndexRelationId) BKI_SCHEMA_MACRO bool indisreplident; /* is this index the identity for replication? */ /* variable-length fields start here, but we allow direct access to indkey */ - int2vector indkey; /* column numbers of indexed cols, or 0 */ + int2vector indkey BKI_FORCE_NOT_NULL; /* column numbers of indexed cols, + * or 0 */ #ifdef CATALOG_VARLEN - oidvector indcollation; /* collation identifiers */ - oidvector indclass; /* opclass identifiers */ - int2vector indoption; /* per-column flags (AM-specific meanings) */ + oidvector indcollation BKI_FORCE_NOT_NULL; /* collation identifiers */ + oidvector indclass BKI_FORCE_NOT_NULL; /* opclass identifiers */ + int2vector indoption BKI_FORCE_NOT_NULL; /* per-column flags + * (AM-specific meanings) */ pg_node_tree indexprs; /* expression trees for index attributes that * are not simple column references; one for * each zero entry in indkey[] */ diff --git a/src/include/catalog/pg_partitioned_table.h b/src/include/catalog/pg_partitioned_table.h index a73cd0d3a4..7ee0419373 100644 --- a/src/include/catalog/pg_partitioned_table.h +++ b/src/include/catalog/pg_partitioned_table.h @@ -41,13 +41,17 @@ CATALOG(pg_partitioned_table,3350,PartitionedRelationId) * field of a heap tuple can be reliably accessed using its C struct * offset, as previous fields are all non-nullable fixed-length fields. */ - int2vector partattrs; /* each member of the array is the attribute - * number of a partition key column, or 0 if - * the column is actually an expression */ + int2vector partattrs BKI_FORCE_NOT_NULL; /* each member of the array is + * the attribute number of a + * partition key column, or 0 + * if the column is actually + * an expression */ #ifdef CATALOG_VARLEN - oidvector partclass; /* operator class to compare keys */ - oidvector partcollation; /* user-specified collation for keys */ + oidvector partclass BKI_FORCE_NOT_NULL; /* operator class to compare + * keys */ + oidvector partcollation BKI_FORCE_NOT_NULL; /* user-specified + * collation for keys */ pg_node_tree partexprs; /* list of expressions in the partition key; * one item for each zero entry in partattrs[] */ #endif diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index 65e8c9f054..b50fa25dbd 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -92,7 +92,7 @@ CATALOG(pg_proc,1255,ProcedureRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81,Proce */ /* parameter types (excludes OUT params) */ - oidvector proargtypes BKI_LOOKUP(pg_type); + oidvector proargtypes BKI_LOOKUP(pg_type) BKI_FORCE_NOT_NULL; #ifdef CATALOG_VARLEN diff --git a/src/include/catalog/pg_statistic_ext.h b/src/include/catalog/pg_statistic_ext.h index a8cb16997a..8747903fc7 100644 --- a/src/include/catalog/pg_statistic_ext.h +++ b/src/include/catalog/pg_statistic_ext.h @@ -47,7 +47,7 @@ CATALOG(pg_statistic_ext,3381,StatisticExtRelationId) * variable-length fields start here, but we allow direct access to * stxkeys */ - int2vector stxkeys; /* array of column keys */ + int2vector stxkeys BKI_FORCE_NOT_NULL; /* array of column keys */ #ifdef CATALOG_VARLEN char stxkind[1] BKI_FORCE_NOT_NULL; /* statistics kinds requested diff --git a/src/include/catalog/pg_trigger.h b/src/include/catalog/pg_trigger.h index 9612b9bdd6..fa5761b784 100644 --- a/src/include/catalog/pg_trigger.h +++ b/src/include/catalog/pg_trigger.h @@ -54,7 +54,8 @@ CATALOG(pg_trigger,2620,TriggerRelationId) * Variable-length fields start here, but we allow direct access to * tgattr. Note: tgattr and tgargs must not be null. */ - int2vector tgattr; /* column numbers, if trigger is on columns */ + int2vector tgattr BKI_FORCE_NOT_NULL; /* column numbers, if trigger is + * on columns */ #ifdef CATALOG_VARLEN bytea tgargs BKI_FORCE_NOT_NULL; /* first\000second\000tgnargs\000 */