Remove hard-coded schema knowledge about pg_attribute from genbki.pl

Add the ability to label a column's default value in the catalog header,
and implement this for pg_attribute.  A new function in Catalog.pm is
used to fill in a tuple with defaults.  The build process will complain
loudly if a catalog entry is incomplete,

Commit 8137f2c3232 labeled variable length columns for the C preprocessor.
Expose that label to genbki.pl so we can exclude those columns from schema
macros in a general fashion. Also, format schema macro entries according
to their types.

This means slightly less code maintenance, but more importantly it's a
proving ground for mechanisms intended to be used in later commits.

While at it, I (Álvaro) couldn't resist making some changes in
genbki.pl: rename some functions to actually indicate their purpose
instead of actively misleading onlookers; and don't iterate on the whole
of pg_type to find the entry for each catalog row, using a hash instead
of an array.

Author: John Naylor, some changes by Álvaro Herrera
Discussion: https://postgr.es/m/CAJVSVGVJHwD8sfDfZW9TbCHWKf=C1YDRM-rF=2JenRU_y+VcFg@mail.gmail.com

4 files changed, 187 insertions, 136 deletions

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index f18b400bd71..9ced1547f6b 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -37,6 +37,8 @@ sub Catalogs
 	foreach my $input_file (@_)
 	{
 		my %catalog;
+		my $is_varlen     = 0;
+
 		$catalog{columns} = [];
 		$catalog{data}    = [];
 
@@ -164,7 +166,11 @@ sub Catalogs
 			elsif ($declaring_attributes)
 			{
 				next if (/^{|^$/);
-				next if (/^#/);
+				if (/^#/)
+				{
+					$is_varlen = 1 if /^#ifdef\s+CATALOG_VARLEN/;
+					next;
+				}
 				if (/^}/)
 				{
 					undef $declaring_attributes;
@@ -172,8 +178,12 @@ sub Catalogs
 				else
 				{
 					my %column;
-					my ($atttype, $attname, $attopt) = split /\s+/, $_;
-					die "parse error ($input_file)" unless $attname;
+					my @attopts = split /\s+/, $_;
+					my $atttype = shift @attopts;
+					my $attname = shift @attopts;
+					die "parse error ($input_file)"
+					  unless ($attname and $atttype);
+
 					if (exists $RENAME_ATTTYPE{$atttype})
 					{
 						$atttype = $RENAME_ATTTYPE{$atttype};
@@ -181,13 +191,14 @@ sub Catalogs
 					if ($attname =~ /(.*)\[.*\]/)    # array attribute
 					{
 						$attname = $1;
-						$atttype .= '[]';            # variable-length only
+						$atttype .= '[]';
 					}
 
 					$column{type} = $atttype;
 					$column{name} = $attname;
+					$column{is_varlen} = 1 if $is_varlen;
 
-					if (defined $attopt)
+					foreach my $attopt (@attopts)
 					{
 						if ($attopt eq 'BKI_FORCE_NULL')
 						{
@@ -197,11 +208,20 @@ sub Catalogs
 						{
 							$column{forcenotnull} = 1;
 						}
+						elsif ($attopt =~ /BKI_DEFAULT\((\S+)\)/)
+						{
+							$column{default} = $1;
+						}
 						else
 						{
 							die
 "unknown column option $attopt on column $attname";
 						}
+
+						if ($column{forcenull} and $column{forcenotnull})
+						{
+							die "$attname is forced both null and not null";
+						}
 					}
 					push @{ $catalog{columns} }, \%column;
 				}
@@ -235,6 +255,46 @@ sub SplitDataLine
 	return @result;
 }
 
+# Fill in default values of a record using the given schema. It's the
+# caller's responsibility to specify other values beforehand.
+sub AddDefaultValues
+{
+	my ($row, $schema) = @_;
+	my @missing_fields;
+	my $msg;
+
+	foreach my $column (@$schema)
+	{
+		my $attname = $column->{name};
+		my $atttype = $column->{type};
+
+		if (defined $row->{$attname})
+		{
+			;
+		}
+		elsif (defined $column->{default})
+		{
+			$row->{$attname} = $column->{default};
+		}
+		else
+		{
+			# Failed to find a value.
+			push @missing_fields, $attname;
+		}
+	}
+
+	if (@missing_fields)
+	{
+		$msg = "Missing values for: " . join(', ', @missing_fields);
+		$msg .= "\nShowing other values for context:\n";
+		while (my($key, $value) = each %$row)
+		{
+			$msg .= "$key => $value, ";
+		}
+	}
+	return $msg;
+}
+
 # Rename temporary files to final names.
 # Call this function with the final file name and the .tmp extension
 # Note: recommended extension is ".tmp$$", so that parallel make steps
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 1d3bbcc532d..ed90a023035 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -105,7 +105,7 @@ print $bki "# PostgreSQL $major_version\n";
 my %schemapg_entries;
 my @tables_needing_macros;
 my %regprocoids;
-my @types;
+my %types;
 
 # produce output, one catalog at a time
 foreach my $catname (@{ $catalogs->{names} })
@@ -119,7 +119,6 @@ foreach my $catname (@{ $catalogs->{names} })
 	  . $catalog->{without_oids}
 	  . $catalog->{rowtype_oid} . "\n";
 
-	my %bki_attr;
 	my @attnames;
 	my $first = 1;
 
@@ -129,7 +128,6 @@ foreach my $catname (@{ $catalogs->{names} })
 	{
 		my $attname = $column->{name};
 		my $atttype = $column->{type};
-		$bki_attr{$attname} = $column;
 		push @attnames, $attname;
 
 		if (!$first)
@@ -211,7 +209,7 @@ foreach my $catname (@{ $catalogs->{names} })
 			{
 				my %type = %bki_values;
 				$type{oid} = $row->{oid};
-				push @types, \%type;
+				$types{ $type{typname} } = \%type;
 			}
 
 			# Write to postgres.bki
@@ -253,28 +251,24 @@ foreach my $catname (@{ $catalogs->{names} })
 			# Generate entries for user attributes.
 			my $attnum       = 0;
 			my $priornotnull = 1;
-			my @user_attrs   = @{ $table->{columns} };
-			foreach my $attr (@user_attrs)
+			foreach my $attr (@{ $table->{columns} })
 			{
 				$attnum++;
-				my $row = emit_pgattr_row($table_name, $attr, $priornotnull);
-				$row->{attnum}        = $attnum;
-				$row->{attstattarget} = '-1';
-				$priornotnull &= ($row->{attnotnull} eq 't');
+				my %row;
+				$row{attnum}   = $attnum;
+				$row{attrelid} = $table->{relation_oid};
+
+				morph_row_for_pgattr(\%row, $schema, $attr, $priornotnull);
+				$priornotnull &= ($row{attnotnull} eq 't');
 
 				# If it's bootstrapped, put an entry in postgres.bki.
-				if ($table->{bootstrap})
-				{
-					bki_insert($row, @attnames);
-				}
+				print_bki_insert(\%row, @attnames) if $table->{bootstrap};
 
 				# Store schemapg entries for later.
-				$row =
-				  emit_schemapg_row($row,
-					grep { $bki_attr{$_}{type} eq 'bool' } @attnames);
+				morph_row_for_schemapg(\%row, $schema);
 				push @{ $schemapg_entries{$table_name} },
 				  sprintf "{ %s }",
-				    join(', ', grep { defined $_ } @{$row}{@attnames});
+				    join(', ', grep { defined $_ } @row{@attnames});
 			}
 
 			# Generate entries for system attributes.
@@ -293,16 +287,18 @@ foreach my $catname (@{ $catalogs->{names} })
 				foreach my $attr (@SYS_ATTRS)
 				{
 					$attnum--;
-					my $row = emit_pgattr_row($table_name, $attr, 1);
-					$row->{attnum}        = $attnum;
-					$row->{attstattarget} = '0';
+					my %row;
+					$row{attnum}        = $attnum;
+					$row{attrelid}      = $table->{relation_oid};
+					$row{attstattarget} = '0';
 
 					# Omit the oid column if the catalog doesn't have them
 					next
 					  if $table->{without_oids}
-						  && $row->{attname} eq 'oid';
+						  && $attr->{name} eq 'oid';
 
-					bki_insert($row, @attnames);
+					morph_row_for_pgattr(\%row, $schema, $attr, 1);
+					print_bki_insert(\%row, @attnames);
 				}
 			}
 		}
@@ -379,130 +375,122 @@ exit 0;
 #################### Subroutines ########################
 
 
-# Given a system catalog name and a reference to a key-value pair corresponding
-# to the name and type of a column, generate a reference to a hash that
-# represents a pg_attribute entry.  We must also be told whether preceding
-# columns were all not-null.
-sub emit_pgattr_row
+# 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),
+# 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 ($table_name, $attr, $priornotnull) = @_;
+	my ($row, $pgattr_schema, $attr, $priornotnull) = @_;
 	my $attname = $attr->{name};
 	my $atttype = $attr->{type};
-	my %row;
 
-	$row{attrelid} = $catalogs->{$table_name}->{relation_oid};
-	$row{attname}  = $attname;
+	$row->{attname} = $attname;
 
-	# Adjust type name for arrays: foo[] becomes _foo
-	# so we can look it up in pg_type
-	if ($atttype =~ /(.+)\[\]$/)
-	{
-		$atttype = '_' . $1;
-	}
+	# Adjust type name for arrays: foo[] becomes _foo, so we can look it up in
+	# pg_type
+	$atttype = '_' . $1 if $atttype =~ /(.+)\[\]$/;
 
 	# Copy the type data from pg_type, and add some type-dependent items
-	foreach my $type (@types)
-	{
-		if (defined $type->{typname} && $type->{typname} eq $atttype)
-		{
-			$row{atttypid}   = $type->{oid};
-			$row{attlen}     = $type->{typlen};
-			$row{attbyval}   = $type->{typbyval};
-			$row{attstorage} = $type->{typstorage};
-			$row{attalign}   = $type->{typalign};
+	my $type = $types{$atttype};
 
-			# set attndims if it's an array type
-			$row{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
-			$row{attcollation} = $type->{typcollation};
+	$row->{atttypid}   = $type->{oid};
+	$row->{attlen}     = $type->{typlen};
+	$row->{attbyval}   = $type->{typbyval};
+	$row->{attstorage} = $type->{typstorage};
+	$row->{attalign}   = $type->{typalign};
 
-			if (defined $attr->{forcenotnull})
-			{
-				$row{attnotnull} = 't';
-			}
-			elsif (defined $attr->{forcenull})
-			{
-				$row{attnotnull} = 'f';
-			}
-			elsif ($priornotnull)
-			{
+	# set attndims if it's an array type
+	$row->{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
+	$row->{attcollation} = $type->{typcollation};
 
-				# 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.
-				$row{attnotnull} =
-				    $type->{typname} eq 'oidvector'   ? 't'
-				  : $type->{typname} eq 'int2vector'  ? 't'
-				  : $type->{typlen}  eq 'NAMEDATALEN' ? 't'
-				  : $type->{typlen} > 0 ? 't'
-				  :                       'f';
-			}
-			else
-			{
-				$row{attnotnull} = 'f';
-			}
-			last;
-		}
+	if (defined $attr->{forcenotnull})
+	{
+		$row->{attnotnull} = 't';
+	}
+	elsif (defined $attr->{forcenull})
+	{
+		$row->{attnotnull} = 'f';
 	}
+	elsif ($priornotnull)
+	{
 
-	# Add in default values for pg_attribute
-	my %PGATTR_DEFAULTS = (
-		attcacheoff   => '-1',
-		atttypmod     => '-1',
-		atthasdef     => 'f',
-		attidentity   => '',
-		attisdropped  => 'f',
-		attislocal    => 't',
-		attinhcount   => '0',
-		attacl        => '_null_',
-		attoptions    => '_null_',
-		attfdwoptions => '_null_');
-	return { %PGATTR_DEFAULTS, %row };
+		# 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.
+		$row->{attnotnull} =
+		$type->{typname} eq 'oidvector'   ? 't'
+		: $type->{typname} eq 'int2vector'  ? 't'
+		: $type->{typlen}  eq 'NAMEDATALEN' ? 't'
+		: $type->{typlen} > 0 ? 't'
+		:                       'f';
+	}
+	else
+	{
+		$row->{attnotnull} = 'f';
+	}
+
+	my $error = Catalog::AddDefaultValues($row, $pgattr_schema);
+	if ($error)
+	{
+		die "Failed to form full tuple for pg_attribute: ", $error;
+	}
 }
 
 # Write a pg_attribute entry to postgres.bki
-sub bki_insert
+sub print_bki_insert
 {
 	my $row        = shift;
 	my @attnames   = @_;
 	my $oid        = $row->{oid} ? "OID = $row->{oid} " : '';
-	my $bki_values = join ' ', map { $_ eq '' ? '""' : $_ } map $row->{$_},
-	  @attnames;
+	my $bki_values = join ' ', @{$row}{@attnames};
 	printf $bki "insert %s( %s )\n", $oid, $bki_values;
 }
 
+# Given a row reference, modify it so that it becomes a valid entry for
+# a catalog schema declaration in schemapg.h.
+#
 # The field values of a Schema_pg_xxx declaration are similar, but not
 # quite identical, to the corresponding values in postgres.bki.
-sub emit_schemapg_row
+sub morph_row_for_schemapg
 {
-	my $row        = shift;
-	my @bool_attrs = @_;
-
-	# Replace empty string by zero char constant
-	$row->{attidentity} ||= '\0';
-
-	# Supply appropriate quoting for these fields.
-	$row->{attname}     = q|{"| . $row->{attname} . q|"}|;
-	$row->{attstorage}  = q|'| . $row->{attstorage} . q|'|;
-	$row->{attalign}    = q|'| . $row->{attalign} . q|'|;
-	$row->{attidentity} = q|'| . $row->{attidentity} . q|'|;
-
-	# We don't emit initializers for the variable length fields at all.
-	# Only the fixed-size portions of the descriptors are ever used.
-	delete $row->{attacl};
-	delete $row->{attoptions};
-	delete $row->{attfdwoptions};
-
-	# Expand booleans from 'f'/'t' to 'false'/'true'.
-	# Some values might be other macros (eg FLOAT4PASSBYVAL), don't change.
-	foreach my $attr (@bool_attrs)
+	my $row           = shift;
+	my $pgattr_schema = shift;
+
+	foreach my $column (@$pgattr_schema)
 	{
-		$row->{$attr} =
-		    $row->{$attr} eq 't' ? 'true'
-		  : $row->{$attr} eq 'f' ? 'false'
-		  :                        $row->{$attr};
+		my $attname = $column->{name};
+		my $atttype = $column->{type};
+
+		# Some data types have special formatting rules.
+		if ($atttype eq 'name')
+		{
+			# add {" ... "} quoting
+			$row->{$attname} = sprintf(qq'{"%s"}', $row->{$attname});
+		}
+		elsif ($atttype eq 'char')
+		{
+			# Replace empty string by zero char constant; add single quotes
+			$row->{$attname} = '\0' if $row->{$attname} eq q|""|;
+			$row->{$attname} = sprintf("'%s'", $row->{$attname});
+		}
+
+		# Expand booleans from 'f'/'t' to 'false'/'true'.
+		# Some values might be other macros (eg FLOAT4PASSBYVAL),
+		# don't change.
+		elsif ($atttype eq 'bool')
+		{
+			$row->{$attname} = 'true' if $row->{$attname} eq 't';
+			$row->{$attname} = 'false' if $row->{$attname} eq 'f';
+		}
+
+		# We don't emit initializers for the variable length fields at all.
+		# Only the fixed-size portions of the descriptors are ever used.
+		delete $row->{$attname} if $column->{is_varlen};
 	}
-	return $row;
 }
 
 sub usage
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 59b0f8ed5d6..96ac4025de2 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -31,6 +31,9 @@
 #define BKI_FORCE_NULL
 #define BKI_FORCE_NOT_NULL
 
+/* Specifies a default value for a catalog field */
+#define BKI_DEFAULT(value)
+
 /*
  * This is never defined; it's here only for documentation.
  *
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 6104254d7be..8159383834d 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -54,7 +54,7 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
 	 * that no value has been explicitly set for this column, so ANALYZE
 	 * should use the default setting.
 	 */
-	int32		attstattarget;
+	int32		attstattarget BKI_DEFAULT(-1);
 
 	/*
 	 * attlen is a copy of the typlen field from pg_type for this attribute.
@@ -90,7 +90,7 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
 	 * descriptor, we may then update attcacheoff in the copies. This speeds
 	 * up the attribute walking process.
 	 */
-	int32		attcacheoff;
+	int32		attcacheoff BKI_DEFAULT(-1);
 
 	/*
 	 * atttypmod records type-specific data supplied at table creation time
@@ -98,7 +98,7 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
 	 * type-specific input and output functions as the third argument. The
 	 * value will generally be -1 for types that do not need typmod.
 	 */
-	int32		atttypmod;
+	int32		atttypmod BKI_DEFAULT(-1);
 
 	/*
 	 * attbyval is a copy of the typbyval field from pg_type for this
@@ -131,13 +131,13 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
 	bool		attnotnull;
 
 	/* Has DEFAULT value or not */
-	bool		atthasdef;
+	bool		atthasdef BKI_DEFAULT(f);
 
 	/* One of the ATTRIBUTE_IDENTITY_* constants below, or '\0' */
-	char		attidentity;
+	char		attidentity BKI_DEFAULT("");
 
 	/* Is dropped (ie, logically invisible) or not */
-	bool		attisdropped;
+	bool		attisdropped BKI_DEFAULT(f);
 
 	/*
 	 * This flag specifies whether this column has ever had a local
@@ -148,10 +148,10 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
 	 * not dropped by a parent's DROP COLUMN even if this causes the column's
 	 * attinhcount to become zero.
 	 */
-	bool		attislocal;
+	bool		attislocal BKI_DEFAULT(t);
 
 	/* Number of times inherited from direct parent relation(s) */
-	int32		attinhcount;
+	int32		attinhcount BKI_DEFAULT(0);
 
 	/* attribute's collation */
 	Oid			attcollation;
@@ -160,13 +160,13 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
 	/* NOTE: The following fields are not present in tuple descriptors. */
 
 	/* Column-level access permissions */
-	aclitem		attacl[1];
+	aclitem		attacl[1] BKI_DEFAULT(_null_);
 
 	/* Column-level options */
-	text		attoptions[1];
+	text		attoptions[1] BKI_DEFAULT(_null_);
 
 	/* Column-level FDW options */
-	text		attfdwoptions[1];
+	text		attfdwoptions[1] BKI_DEFAULT(_null_);
 #endif
 } FormData_pg_attribute;