Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jxDev-0004eq-NF for pgsql-hackers@arkaria.postgresql.org; Sun, 19 Jul 2020 18:03:33 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jxDes-0004jN-Be for pgsql-hackers@arkaria.postgresql.org; Sun, 19 Jul 2020 18:03:30 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jxDes-0004jF-3K for pgsql-hackers@lists.postgresql.org; Sun, 19 Jul 2020 18:03:30 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jxDek-0001Fn-8W for pgsql-hackers@lists.postgresql.org; Sun, 19 Jul 2020 18:03:28 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 06JI3KOh204761 for ; Sun, 19 Jul 2020 14:03:20 -0400 From: Tom Lane To: pgsql-hackers@lists.postgresql.org Subject: Fix initdb's unsafe not-null-marking rule MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <204642.1595181632.0@sss.pgh.pa.us> Date: Sun, 19 Jul 2020 14:03:20 -0400 Message-ID: <204760.1595181800@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <204642.1595181632.1@sss.pgh.pa.us> Part of the blame for the pg_subscription.subslotname fiasco can be laid at the feet of initdb's default rule for marking columns NOT NULL; that rule is fairly arbitrary and does not guarantee to make safe choices. I propose that we change it so that it *is* safe, ie it will only mark fields NOT NULL if they'd certainly be safe to access as C struct fields. Keeping the end results the same requires a few more manual applications of BKI_FORCE_NOT_NULL than we had before. But I think that that's fine, because it reduces the amount of poorly-documented magic in this area. I note in particular that bki.sgml was entirely failing to tell the full truth. (Note: this would allow reverting the manual BKI_FORCE_NULL label that I just added to pg_subscription.subslotname, but I feel no great desire to do that.) I propose this only for HEAD, not the back branches. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-default-not-null-markings.patch"; charset="us-ascii" Content-ID: <204642.1595181632.2@sss.pgh.pa.us> Content-Description: fix-default-not-null-markings.patch Content-Transfer-Encoding: quoted-printable diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml index 6776c4a3c1..5b871721d1 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. But note diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/boo= tstrap.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 n= ullness) = /* * 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 =3D=3D OIDVECTOROID || \ - (att)->atttypid =3D=3D INT2VECTOROID) - - if (MARKNOTNULL(attrtypes[attnum])) + if (attrtypes[attnum]->attlen > 0) { int i; = /* check earlier attributes */ for (i =3D 0; i < attnum; i++) { - if (!attrtypes[i]->attnotnull) + if (attrtypes[i]->attlen <=3D 0 || + !attrtypes[i]->attnotnull) break; } if (i =3D=3D 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 =3D 0; - my $priornotnull =3D 1; + my $attnum =3D 0; + my $priorfixedwidth =3D 1; foreach my $attr (@{ $table->{columns} }) { $attnum++; @@ -722,8 +722,12 @@ sub gen_pg_attribute $row{attnum} =3D $attnum; $row{attrelid} =3D $table->{relation_oid}; = - morph_row_for_pgattr(\%row, $schema, $attr, $priornotnull); - $priornotnull &=3D ($row{attnotnull} eq 't'); + morph_row_for_pgattr(\%row, $schema, $attr, $priorfixedwidth); + + # Update $priorfixedwidth --- must match morph_row_for_pgattr + $priorfixedwidth &=3D + ($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 nul= l), +# $priorfixedwidth (all prior columns are fixed-width and not null), # modify the $row hashref for print_bki_insert. This includes setting da= ta # from the corresponding pg_type element and filling in any default value= s. # Any value not handled here must be supplied by caller. sub morph_row_for_pgattr { - my ($row, $pgattr_schema, $attr, $priornotnull) =3D @_; + my ($row, $pgattr_schema, $attr, $priorfixedwidth) =3D @_; my $attname =3D $attr->{name}; my $atttype =3D $attr->{type}; = @@ -801,19 +805,18 @@ sub morph_row_for_pgattr { $row->{attnotnull} =3D '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} =3D - $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 t= his is - * handled. + * of an undefined symbol. See also the BOOTCOL_NULL_AUTO code in bootst= rap.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_MACR= O bool indisreplident; /* is this index the identity for replication? */ = /* variable-length fields start here, but we allow direct access to indk= ey */ - 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/cata= log/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,PartitionedRelationI= d) * 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_tri= gger.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 */ ------- =_aaaaaaaaaa0--