On 11/16/18, Andres Freund <and...@anarazel.de> wrote:
> On 2018-11-15 17:25:21 +0700, John Naylor wrote:
>> I don't see an advantage to having a different range, but maybe it
>> should error out if $maxoid reaches FirstBootstrapObjectId.
>
> Hm. Not sure I really see the point. Note we didn't have that check
> before either, and it'd not catch manual assignment of too high oids. I
> wonder if we should have a check in sanity_check.sql or such, that'd
> then catch both?
My concern is that a developer might assign a high number oid to avoid
conflicts during development, such that the auto-assigned oids can
blow past 10000. If it can be done simply in SQL, I think that'd be
fine, and maybe more robust than checking within the script.
>> This patch breaks reformat_dat_file.pl. I've attached a fix, which
>> also de-lists oid as a special key within the *.dat files. It might be
>> good to put off reformatting until feature freeze, so as not to break
>> others' patches.
>
> Thanks for catching that. I wonder if we could fix that in a way that
> doesn't move oid into the middle of the data - while it's less magic-y
> from a storage level, it's still more important than the rest. Perhaps
> we could just leave oid in metadata and skip all @metadata elements in
> format_hash()?
@metadata is passed to format_hash(), so that wouldn't work without
additional bookkeeping (if I understand you correctly). I think it'd
be simpler to filter out @metadata elements while building @attnames,
which is what we pass to format_hash() for ordinary columns. Let me
know what you think (attached).
>> @@ -193,7 +192,7 @@ sub strip_default_values
>> {
>> my $attname = $column->{name};
>> die "strip_default_values: $catname.$attname undefined\n"
>> - if !defined $row->{$attname};
>> + if !defined $row->{$attname} and $attname ne 'oid';
>>
>> if (defined $column->{default}
>> and ($row->{$attname} eq $column->{default}))
>
> Hm, why is there no column definition for oid?
(This is a sanity check that all keys referring to columns have
values. I'd like to keep it since it has found bugs during
development.) If oid is now a normal column, then catalogs like
pg_amop will have no explicit oid and the check will fail without this
change. I've added a comment -- hopefully it's clearer.
-John Naylor
diff --git a/src/include/catalog/reformat_dat_file.pl b/src/include/catalog/reformat_dat_file.pl
index ca20fb86da..54ad1b72e5 100755
--- a/src/include/catalog/reformat_dat_file.pl
+++ b/src/include/catalog/reformat_dat_file.pl
@@ -26,8 +26,11 @@ use FindBin;
use lib "$FindBin::RealBin/../../backend/catalog/";
use Catalog;
-# Names of the metadata fields of a catalog entry. (line_number is also
-# a metadata field, but we never write it out, so it's not listed here.)
+# Names of the metadata fields of a catalog entry.
+# Note: oid is a normal column from a storage perspective, but it's more
+# important than the rest, so it's listed first among the metadata fields.
+# Note: line_number is also a metadata field, but we never write it out,
+# so it's not listed here.
my @METADATA =
('oid', 'oid_symbol', 'array_type_oid', 'descr', 'autogenerated');
@@ -119,7 +122,12 @@ foreach my $catname (@catnames)
foreach my $column (@$schema)
{
my $attname = $column->{name};
- push @attnames, $attname;
+
+ # We may have ordinary columns at the storage level that we still
+ # want to format as a special value. Exclude these from the column
+ # list so they are not written twice.
+ push @attnames, $attname
+ if !(grep { $_ eq $attname } @METADATA;
}
# Overwrite .dat files in place, since they are under version control.
@@ -192,8 +200,11 @@ sub strip_default_values
foreach my $column (@$schema)
{
my $attname = $column->{name};
+
+ # It's okay if we have no oid value, since it will be assigned
+ # automatically before bootstrap.
die "strip_default_values: $catname.$attname undefined\n"
- if !defined $row->{$attname};
+ if !defined $row->{$attname} and $attname ne 'oid';
if (defined $column->{default}
and ($row->{$attname} eq $column->{default}))
