NOT NULL markings for BKI columns
Hi,
8b38a538c0aa5a432dacd90f10805dc667a3d1a0 changed things so that all
columns are checked for NOT NULL ness. Which is fine in general, but it
currently does make it impossible to have a varlena column (except
OID/INT2 vector...) as a key column in syscache. Even if the column is
factually NOT NUL.
The rule for determining attribute's NOT NULL setting in bootstrap is:
/*
* 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.
*/
#define MARKNOTNULL(att) \
((att)->attlen > 0 || \
(att)->atttypid == OIDVECTOROID || \
(att)->atttypid == INT2VECTOROID)
if (MARKNOTNULL(attrtypes[attnum]))
{
int i;
for (i = 0; i < attnum; i++)
{
if (!MARKNOTNULL(attrtypes[i]))
break;
}
if (i == attnum)
attrtypes[attnum]->attnotnull = true;
}
(the rule is also encoded in genbki.pl)
Now, you can argue that it's a folly to use the syscache code to access
something via a text column (which is what I want to do). I don't agree,
but even if you're of that position, it seems worthwhile to mark further
catalog columns as NOT NULL in the catalog if that's what the code
expects?
E.g. pg_(sh)?seclabel.provider should certainly be NOT
NULL. pg_extension.extversion as well. There's a couple more I think.
So, how about providing bootstrap infrastructure for marking columns as
NOT NULL?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
So, how about providing bootstrap infrastructure for marking columns as
NOT NULL?
We've not desperately needed it up to now, but if you can think of a clean
representation, go for it. I'd want to preserve the property that all
columns accessible via C structs are automatically NOT NULL though; too
much risk of breakage otherwise.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-02-15 12:11:52 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
So, how about providing bootstrap infrastructure for marking columns as
NOT NULL?We've not desperately needed it up to now, but if you can think of a clean
representation, go for it. I'd want to preserve the property that all
columns accessible via C structs are automatically NOT NULL though; too
much risk of breakage otherwise.
Agreed. I think that the noise of changing all existing attributes to
have the marker would far outweigh the cleanliness of being
consistent. I was thinking of adding BKI_FORCENOTNULL which would be
specified on the attributes you want it. The FORCE in there representing
that the default choice is overwritten.
On a first glance that doesn't look too hard.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
I was thinking of adding BKI_FORCENOTNULL which would be
specified on the attributes you want it. The FORCE in there representing
that the default choice is overwritten.
Where are you thinking of sticking that exactly, and will pgindent
do something sane with it?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-02-15 12:31:10 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
I was thinking of adding BKI_FORCENOTNULL which would be
specified on the attributes you want it. The FORCE in there representing
that the default choice is overwritten.Where are you thinking of sticking that exactly, and will pgindent
do something sane with it?
Hm, I was thinking about
/* extversion should never be null, but the others can be. */
text extversion PG_FORCENOTNULL; /* extension version name */
but pgindent then removes some of the space between text and extversion,
making it
text extversion PG_FORCENOTNULL; /* extension version name */
an alternative where it doesn't do that is
text PG_FORCENOTNULL(extversion); /* extension version name */
Not sure what's the best way here.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2015-02-15 12:31:10 -0500, Tom Lane wrote:
Where are you thinking of sticking that exactly, and will pgindent
do something sane with it?
Hm, I was thinking about
/* extversion should never be null, but the others can be. */
text extversion PG_FORCENOTNULL; /* extension version name */
but pgindent then removes some of the space between text and extversion,
making it
text extversion PG_FORCENOTNULL; /* extension version name */
an alternative where it doesn't do that is
text PG_FORCENOTNULL(extversion); /* extension version name */
Not sure what's the best way here.
The former is clearly a lot more sane semantically, so I'd go with
that even if the whitespace is a bit less nice.
I notice that pgindent does a similar not-very-nice thing with
PG_USED_FOR_ASSERTS_ONLY. I wonder if we could hack it to handle
those two identifiers specially?
BTW, the precedent of PG_USED_FOR_ASSERTS_ONLY would suggest calling
this one PG_FORCE_NOT_NULL, or at least using underscores for word
breaks in whatever we end up calling it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-02-15 12:54:45 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Hm, I was thinking about
/* extversion should never be null, but the others can be. */
text extversion PG_FORCENOTNULL; /* extension version name */
but pgindent then removes some of the space between text and extversion,
making it
text extversion PG_FORCENOTNULL; /* extension version name */
an alternative where it doesn't do that is
text PG_FORCENOTNULL(extversion); /* extension version name */Not sure what's the best way here.
The former is clearly a lot more sane semantically, so I'd go with
that even if the whitespace is a bit less nice.
Yea.
I notice that pgindent does a similar not-very-nice thing with
PG_USED_FOR_ASSERTS_ONLY. I wonder if we could hack it to handle
those two identifiers specially?
I looked for about a minute and it didn't seem trivial to
do. Unfortunately that's pretty much all I'm willing to do.
BTW, the precedent of PG_USED_FOR_ASSERTS_ONLY would suggest calling
this one PG_FORCE_NOT_NULL, or at least using underscores for word
breaks in whatever we end up calling it.
I've named it BKI_FORCE_(NOT_)?NULL.
So, I've implemented this - turned out to be a bit more work than I'd
expected... I had to whack around the representation Catalog.pm returns
for columns, but I think the new representation for columns is better
anyway. Doesn't look too bad.
The second patch attached adds BKI_FORCE_NOT_NULL marker to the system
columns that, based on a single scan through the relevant headers, are
missing NOT NULL.
I've also added BKI_FORCE_NULL as you mentioned it as being possibly
useful, was easy enough. I haven't identified a user so far though, so
we could just remove it if we want.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0001-Allow-forcing-nullness-of-columns-during-bootstrap.patchtext/x-patch; charset=us-asciiDownload
>From 9893065e8e5d311a00ec815c02646008056abc3b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 20 Feb 2015 22:07:05 +0100
Subject: [PATCH 1/2] Allow forcing nullness of columns during bootstrap.
Bootstrap determines whether a column is null based on simple builtin
rules. Those work surprisingly well, but nonetheless a few existing
columns aren't set correctly. Additionally there's future patches where
forcing the nullness would be helpful.
---
doc/src/sgml/bki.sgml | 9 +++--
src/backend/bootstrap/bootparse.y | 13 ++++++--
src/backend/bootstrap/bootscanner.l | 3 ++
src/backend/bootstrap/bootstrap.c | 56 ++++++++++++++++++++------------
src/backend/catalog/Catalog.pm | 24 ++++++++++++--
src/backend/catalog/genbki.pl | 65 +++++++++++++++++++++++++++----------
src/backend/utils/Gen_fmgrtab.pl | 2 +-
src/include/bootstrap/bootstrap.h | 6 +++-
src/include/catalog/genbki.h | 2 ++
9 files changed, 131 insertions(+), 49 deletions(-)
diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml
index aaf500a..af6d8d1 100644
--- a/doc/src/sgml/bki.sgml
+++ b/doc/src/sgml/bki.sgml
@@ -75,9 +75,12 @@
<optional><literal>without_oids</></optional>
<optional><literal>rowtype_oid</> <replaceable>oid</></optional>
(<replaceable class="parameter">name1</replaceable> =
- <replaceable class="parameter">type1</replaceable> <optional>,
- <replaceable class="parameter">name2</replaceable> = <replaceable
- class="parameter">type2</replaceable>, ...</optional>)
+ <replaceable class="parameter">type1</replaceable>
+ <optional>FORCE NOT NULL | FORCE NULL </optional> <optional>,
+ <replaceable class="parameter">name2</replaceable> =
+ <replaceable class="parameter">type2</replaceable>
+ <optional>FORCE NOT NULL | FORCE NULL </optional>,
+ ...</optional>)
</term>
<listitem>
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index 56fa1aa..9edd1a0 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -107,7 +107,7 @@ static int num_columns_read = 0;
%type <list> boot_index_params
%type <ielem> boot_index_param
%type <str> boot_const boot_ident
-%type <ival> optbootstrap optsharedrelation optwithoutoids
+%type <ival> optbootstrap optsharedrelation optwithoutoids boot_column_nullness
%type <oidval> oidspec optoideq optrowtypeoid
%token <str> CONST_P ID
@@ -115,6 +115,7 @@ static int num_columns_read = 0;
%token XDECLARE INDEX ON USING XBUILD INDICES UNIQUE XTOAST
%token COMMA EQUALS LPAREN RPAREN
%token OBJ_ID XBOOTSTRAP XSHARED_RELATION XWITHOUT_OIDS XROWTYPE_OID NULLVAL
+%token XFORCE XNOT XNULL
%start TopLevel
@@ -427,14 +428,20 @@ boot_column_list:
;
boot_column_def:
- boot_ident EQUALS boot_ident
+ boot_ident EQUALS boot_ident boot_column_nullness
{
if (++numattr > MAXATTR)
elog(FATAL, "too many columns");
- DefineAttr($1, $3, numattr-1);
+ DefineAttr($1, $3, numattr-1, $4);
}
;
+boot_column_nullness:
+ XFORCE XNOT XNULL { $$ = BOOTCOL_NULL_FORCE_NOT_NULL; }
+ | XFORCE XNULL { $$ = BOOTCOL_NULL_FORCE_NULL; }
+ | { $$ = BOOTCOL_NULL_AUTO; }
+ ;
+
oidspec:
boot_ident { $$ = atooid($1); }
;
diff --git a/src/backend/bootstrap/bootscanner.l b/src/backend/bootstrap/bootscanner.l
index fa4e2ff..72714f4 100644
--- a/src/backend/bootstrap/bootscanner.l
+++ b/src/backend/bootstrap/bootscanner.l
@@ -109,6 +109,9 @@ insert { return(INSERT_TUPLE); }
"on" { return(ON); }
"using" { return(USING); }
"toast" { return(XTOAST); }
+"FORCE" { return(XFORCE); }
+"NOT" { return(XNOT); }
+"NULL" { return(XNULL); }
{arrayid} {
yylval.str = MapArrayTypeName(yytext);
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index bc66eac..ad49964 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -642,7 +642,7 @@ closerel(char *name)
* ----------------
*/
void
-DefineAttr(char *name, char *type, int attnum)
+DefineAttr(char *name, char *type, int attnum, int nullness)
{
Oid typeoid;
@@ -697,30 +697,44 @@ DefineAttr(char *name, char *type, int attnum)
attrtypes[attnum]->atttypmod = -1;
attrtypes[attnum]->attislocal = true;
- /*
- * 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.
- */
-#define MARKNOTNULL(att) \
- ((att)->attlen > 0 || \
- (att)->atttypid == OIDVECTOROID || \
- (att)->atttypid == INT2VECTOROID)
-
- if (MARKNOTNULL(attrtypes[attnum]))
+ if (nullness == BOOTCOL_NULL_FORCE_NOT_NULL)
+ {
+ attrtypes[attnum]->attnotnull = true;
+ }
+ else if (nullness == BOOTCOL_NULL_FORCE_NULL)
{
- int i;
+ attrtypes[attnum]->attnotnull = false;
+ }
+ else
+ {
+ Assert(nullness == BOOTCOL_NULL_AUTO);
- for (i = 0; i < attnum; i++)
+ /*
+ * 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.
+ */
+#define MARKNOTNULL(att) \
+ ((att)->attlen > 0 || \
+ (att)->atttypid == OIDVECTOROID || \
+ (att)->atttypid == INT2VECTOROID)
+
+ if (MARKNOTNULL(attrtypes[attnum]))
{
- if (!MARKNOTNULL(attrtypes[i]))
- break;
+ int i;
+
+ /* check earlier attributes */
+ for (i = 0; i < attnum; i++)
+ {
+ if (!attrtypes[i]->attnotnull)
+ break;
+ }
+ if (i == attnum)
+ attrtypes[attnum]->attnotnull = true;
}
- if (i == attnum)
- attrtypes[attnum]->attnotnull = true;
}
}
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index c773eca..c7b1c17 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -161,7 +161,8 @@ sub Catalogs
}
else
{
- my ($atttype, $attname) = split /\s+/, $_;
+ my %row;
+ my ($atttype, $attname, $attopt) = split /\s+/, $_;
die "parse error ($input_file)" unless $attname;
if (exists $RENAME_ATTTYPE{$atttype})
{
@@ -172,7 +173,26 @@ sub Catalogs
$attname = $1;
$atttype .= '[]'; # variable-length only
}
- push @{ $catalog{columns} }, { $attname => $atttype };
+
+ $row{'type'} = $atttype;
+ $row{'name'} = $attname;
+
+ if (defined $attopt)
+ {
+ if ($attopt eq 'PG_FORCE_NULL')
+ {
+ $row{'forcenull'} = 1;
+ }
+ elsif ($attopt eq 'BKI_FORCE_NOT_NULL')
+ {
+ $row{'forcenotnull'} = 1;
+ }
+ else
+ {
+ die "unknown column option $attopt on column $attname"
+ }
+ }
+ push @{ $catalog{columns} }, \%row;
}
}
}
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index e1c7fe5..a5c78ee 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -118,17 +118,36 @@ foreach my $catname (@{ $catalogs->{names} })
my %bki_attr;
my @attnames;
+ my $first = 1;
+
+ print BKI " (\n";
foreach my $column (@{ $catalog->{columns} })
{
- my ($attname, $atttype) = %$column;
- $bki_attr{$attname} = $atttype;
+ my $attname = $column->{name};
+ my $atttype = $column->{type};
+ $bki_attr{$attname} = $column;
push @attnames, $attname;
+
+ if (!$first)
+ {
+ print BKI " ,\n";
+ }
+ $first = 0;
+
+ print BKI " $attname = $atttype";
+
+ if (defined $column->{forcenotnull})
+ {
+ print BKI " FORCE NOT NULL";
+ }
+ elsif (defined $column->{forcenull})
+ {
+ print BKI " FORCE NULL";
+ }
}
- print BKI " (\n";
- print BKI join " ,\n", map(" $_ = $bki_attr{$_}", @attnames);
print BKI "\n )\n";
- # open it, unless bootstrap case (create bootstrap does this automatically)
+ # open it, unless bootstrap case (create bootstrap does this automatically)
if ($catalog->{bootstrap} eq '')
{
print BKI "open $catname\n";
@@ -210,7 +229,7 @@ foreach my $catname (@{ $catalogs->{names} })
# Store schemapg entries for later.
$row =
emit_schemapg_row($row,
- grep { $bki_attr{$_} eq 'bool' } @attnames);
+ grep { $bki_attr{$_}{type} eq 'bool' } @attnames);
push @{ $schemapg_entries{$table_name} }, '{ '
. join(
', ', grep { defined $_ }
@@ -223,13 +242,13 @@ foreach my $catname (@{ $catalogs->{names} })
{
$attnum = 0;
my @SYS_ATTRS = (
- { ctid => 'tid' },
- { oid => 'oid' },
- { xmin => 'xid' },
- { cmin => 'cid' },
- { xmax => 'xid' },
- { cmax => 'cid' },
- { tableoid => 'oid' });
+ { name => 'ctid', type => 'tid' },
+ { name => 'oid', type => 'oid' },
+ { name => 'xmin', type => 'xid' },
+ { name => 'cmin', type=> 'cid' },
+ { name => 'xmax', type=> 'xid' },
+ { name => 'cmax', type => 'cid' },
+ { name => 'tableoid', type => 'oid' });
foreach my $attr (@SYS_ATTRS)
{
$attnum--;
@@ -326,7 +345,8 @@ exit 0;
sub emit_pgattr_row
{
my ($table_name, $attr, $priornotnull) = @_;
- my ($attname, $atttype) = %$attr;
+ my $attname = $attr->{name};
+ my $atttype = $attr->{type};
my %row;
$row{attrelid} = $catalogs->{$table_name}->{relation_oid};
@@ -354,11 +374,20 @@ sub emit_pgattr_row
$row{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
$row{attcollation} = $type->{typcollation};
- # attnotnull must be set true if the type is fixed-width and
- # prior columns are too --- compare DefineAttr in bootstrap.c.
- # oidvector and int2vector are also treated as not-nullable.
- if ($priornotnull)
+ if (defined $attr->{forcenotnull})
+ {
+ $row{attnotnull} = 't';
+ }
+ elsif (defined $attr->{forcenull})
+ {
+ $row{attnotnull} = 'f';
+ }
+ elsif ($priornotnull)
{
+ # 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'
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 8b71864..f5cc265 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -52,7 +52,7 @@ my @fmgr = ();
my @attnames;
foreach my $column (@{ $catalogs->{pg_proc}->{columns} })
{
- push @attnames, keys %$column;
+ push @attnames, $column->{name};
}
my $data = $catalogs->{pg_proc}->{data};
diff --git a/src/include/bootstrap/bootstrap.h b/src/include/bootstrap/bootstrap.h
index be4430a..f9cbc13 100644
--- a/src/include/bootstrap/bootstrap.h
+++ b/src/include/bootstrap/bootstrap.h
@@ -23,6 +23,10 @@
*/
#define MAXATTR 40
+#define BOOTCOL_NULL_AUTO 1
+#define BOOTCOL_NULL_FORCE_NULL 2
+#define BOOTCOL_NULL_FORCE_NOT_NULL 3
+
extern Relation boot_reldesc;
extern Form_pg_attribute attrtypes[MAXATTR];
extern int numattr;
@@ -35,7 +39,7 @@ extern void err_out(void);
extern void closerel(char *name);
extern void boot_openrel(char *name);
-extern void DefineAttr(char *name, char *type, int attnum);
+extern void DefineAttr(char *name, char *type, int attnum, int nullness);
extern void InsertOneTuple(Oid objectid);
extern void InsertOneValue(char *value, int i);
extern void InsertOneNull(int i);
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 5d6039d..cebf51d 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -28,6 +28,8 @@
#define BKI_WITHOUT_OIDS
#define BKI_ROWTYPE_OID(oid)
#define BKI_SCHEMA_MACRO
+#define BKI_FORCE_NULL
+#define BKI_FORCE_NOT_NULL
/*
* This is never defined; it's here only for documentation.
--
2.3.0.149.gf3f4077.dirty
0002-Force-some-system-catalog-table-columns-to-be-marked.patchtext/x-patch; charset=us-asciiDownload
>From 16070f6e6f158f8d929ecc7d003c5ac3c61c5faa Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 20 Feb 2015 22:11:00 +0100
Subject: [PATCH 2/2] Force some system catalog table columns to be marked NOT
NULL.
In a manual pass over the catalog declaration I found a number of
columns which the boostrap automatism didn't mark NOT NULL even though
they actually were. Add BKI_FORCE_NOT_NULL markings to them.
It's usually not critical if a system table column is falsely determined
to be nullable as the code should always catch relevant cases. But it's
good to have a extra layer in place.
---
src/include/catalog/pg_description.h | 2 +-
src/include/catalog/pg_extension.h | 4 ++--
src/include/catalog/pg_largeobject.h | 2 +-
src/include/catalog/pg_pltemplate.h | 4 ++--
src/include/catalog/pg_proc.h | 2 +-
src/include/catalog/pg_seclabel.h | 4 ++--
src/include/catalog/pg_shdescription.h | 2 +-
src/include/catalog/pg_shseclabel.h | 4 ++--
src/include/catalog/pg_trigger.h | 2 +-
9 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/include/catalog/pg_description.h b/src/include/catalog/pg_description.h
index 5a936e8..692455f 100644
--- a/src/include/catalog/pg_description.h
+++ b/src/include/catalog/pg_description.h
@@ -52,7 +52,7 @@ CATALOG(pg_description,2609) BKI_WITHOUT_OIDS
int32 objsubid; /* column number, or 0 if not used */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
- text description; /* description of object */
+ text description BKI_FORCE_NOT_NULL; /* description of object */
#endif
} FormData_pg_description;
diff --git a/src/include/catalog/pg_extension.h b/src/include/catalog/pg_extension.h
index f45d6cb..99ab35b 100644
--- a/src/include/catalog/pg_extension.h
+++ b/src/include/catalog/pg_extension.h
@@ -36,8 +36,8 @@ CATALOG(pg_extension,3079)
bool extrelocatable; /* if true, allow ALTER EXTENSION SET SCHEMA */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
- /* extversion should never be null, but the others can be. */
- text extversion; /* extension version name */
+ /* extversion may never be null, but the others can be. */
+ text extversion BKI_FORCE_NOT_NULL; /* extension version name */
Oid extconfig[1]; /* dumpable configuration tables */
text extcondition[1]; /* WHERE clauses for config tables */
#endif
diff --git a/src/include/catalog/pg_largeobject.h b/src/include/catalog/pg_largeobject.h
index 6a8d0cc..b7810ef 100644
--- a/src/include/catalog/pg_largeobject.h
+++ b/src/include/catalog/pg_largeobject.h
@@ -34,7 +34,7 @@ CATALOG(pg_largeobject,2613) BKI_WITHOUT_OIDS
int32 pageno; /* Page number (starting from 0) */
/* data has variable length, but we allow direct access; see inv_api.c */
- bytea data; /* Data for page (may be zero-length) */
+ bytea data BKI_FORCE_NOT_NULL; /* Data for page (may be zero-length) */
} FormData_pg_largeobject;
/* ----------------
diff --git a/src/include/catalog/pg_pltemplate.h b/src/include/catalog/pg_pltemplate.h
index c5e6554..569d724 100644
--- a/src/include/catalog/pg_pltemplate.h
+++ b/src/include/catalog/pg_pltemplate.h
@@ -35,10 +35,10 @@ CATALOG(pg_pltemplate,1136) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
bool tmpldbacreate; /* PL is installable by db owner? */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
- text tmplhandler; /* name of call handler function */
+ text tmplhandler BKI_FORCE_NOT_NULL; /* name of call handler function */
text tmplinline; /* name of anonymous-block handler, or NULL */
text tmplvalidator; /* name of validator function, or NULL */
- text tmpllibrary; /* path of shared library */
+ text tmpllibrary BKI_FORCE_NOT_NULL; /* path of shared library */
aclitem tmplacl[1]; /* access privileges for template */
#endif
} FormData_pg_pltemplate;
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6fae8a0..4268b99 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -66,7 +66,7 @@ CATALOG(pg_proc,1255) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81) BKI_SCHEMA_MACRO
text proargnames[1]; /* parameter names (NULL if no names) */
pg_node_tree proargdefaults;/* list of expression trees for argument
* defaults (NULL if none) */
- text prosrc; /* procedure source text */
+ text prosrc BKI_FORCE_NOT_NULL; /* procedure source text */
text probin; /* secondary procedure info (can be NULL) */
text proconfig[1]; /* procedure-local GUC settings */
aclitem proacl[1]; /* access permissions */
diff --git a/src/include/catalog/pg_seclabel.h b/src/include/catalog/pg_seclabel.h
index d54e699..c9f5b0c 100644
--- a/src/include/catalog/pg_seclabel.h
+++ b/src/include/catalog/pg_seclabel.h
@@ -27,8 +27,8 @@ CATALOG(pg_seclabel,3596) BKI_WITHOUT_OIDS
int32 objsubid; /* column number, or 0 if not used */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
- text provider; /* name of label provider */
- text label; /* security label of the object */
+ text provider BKI_FORCE_NOT_NULL; /* name of label provider */
+ text label BKI_FORCE_NOT_NULL; /* security label of the object */
#endif
} FormData_pg_seclabel;
diff --git a/src/include/catalog/pg_shdescription.h b/src/include/catalog/pg_shdescription.h
index 723f984..c524099 100644
--- a/src/include/catalog/pg_shdescription.h
+++ b/src/include/catalog/pg_shdescription.h
@@ -44,7 +44,7 @@ CATALOG(pg_shdescription,2396) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
Oid classoid; /* OID of table containing object */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
- text description; /* description of object */
+ text description BKI_FORCE_NOT_NULL; /* description of object */
#endif
} FormData_pg_shdescription;
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index f0b9952..3977b42 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -26,8 +26,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
Oid classoid; /* OID of table containing the shared object */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
- text provider; /* name of label provider */
- text label; /* security label of the object */
+ text provider BKI_FORCE_NOT_NULL; /* name of label provider */
+ text label BKI_FORCE_NOT_NULL; /* security label of the object */
#endif
} FormData_pg_shseclabel;
diff --git a/src/include/catalog/pg_trigger.h b/src/include/catalog/pg_trigger.h
index 40c8c0f..bff8fcf 100644
--- a/src/include/catalog/pg_trigger.h
+++ b/src/include/catalog/pg_trigger.h
@@ -57,7 +57,7 @@ CATALOG(pg_trigger,2620)
int2vector tgattr; /* column numbers, if trigger is on columns */
#ifdef CATALOG_VARLEN
- bytea tgargs; /* first\000second\000tgnargs\000 */
+ bytea tgargs BKI_FORCE_NOT_NULL; /* first\000second\000tgnargs\000 */
pg_node_tree tgqual; /* WHEN expression, or NULL if none */
#endif
} FormData_pg_trigger;
--
2.3.0.149.gf3f4077.dirty
Andres Freund wrote:
On 2015-02-15 12:54:45 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
BTW, the precedent of PG_USED_FOR_ASSERTS_ONLY would suggest calling
this one PG_FORCE_NOT_NULL, or at least using underscores for word
breaks in whatever we end up calling it.I've named it BKI_FORCE_(NOT_)?NULL.
So, I've implemented this - turned out to be a bit more work than I'd
expected... I had to whack around the representation Catalog.pm returns
for columns, but I think the new representation for columns is better
anyway. Doesn't look too bad.
Just gave it a quick read, I think it's good. +1 for your
implementation.
I've also added BKI_FORCE_NULL as you mentioned it as being possibly
useful, was easy enough. I haven't identified a user so far though, so
we could just remove it if we want.
I think we should just save this part of the patch until some use turns up.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-02-20 19:06:50 -0300, Alvaro Herrera wrote:
Andres Freund wrote:
On 2015-02-15 12:54:45 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
BTW, the precedent of PG_USED_FOR_ASSERTS_ONLY would suggest calling
this one PG_FORCE_NOT_NULL, or at least using underscores for word
breaks in whatever we end up calling it.I've named it BKI_FORCE_(NOT_)?NULL.
So, I've implemented this - turned out to be a bit more work than I'd
expected... I had to whack around the representation Catalog.pm returns
for columns, but I think the new representation for columns is better
anyway. Doesn't look too bad.Just gave it a quick read, I think it's good. +1 for your
implementation.
Unless somebody protests I'm going to push this soon.
I've also added BKI_FORCE_NULL as you mentioned it as being possibly
useful, was easy enough. I haven't identified a user so far though, so
we could just remove it if we want.I think we should just save this part of the patch until some use turns up.
I pondered this for a while and I don't agree. If the flag had been
available a couple column that now use 0 instead of NULLs and such would
have been NULLable. And since it's very few lines I'm inclined to keep
it, it's really cheap enough.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2015-02-20 19:06:50 -0300, Alvaro Herrera wrote:
I think we should just save this part of the patch until some use turns up.
I pondered this for a while and I don't agree. If the flag had been
available a couple column that now use 0 instead of NULLs and such would
have been NULLable. And since it's very few lines I'm inclined to keep
it, it's really cheap enough.
I agree, we'll probably find a use for it someday.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Andres,
Following commit (related to this discussion),
added a bug when we use BKI_FORCE_NULL.
commit eb68379c38202180bc8e33fb9987284e314b7fc8
Author: Andres Freund <andres@anarazel.de>
Date: Sat Feb 21 22:25:49 2015 +0100
Allow forcing nullness of columns during bootstrap.
Bootstrap determines whether a column is null based on simple builtin
rules. Those work surprisingly well, but nonetheless a few existing
columns aren't set correctly. Additionally there is at least one patch
sent to hackers where forcing the nullness of a column would be helpful.
The boostrap format has gained FORCE [NOT] NULL for this, which will be
emitted by genbki.pl when BKI_FORCE_(NOT_)?NULL is specified for a
column in a catalog header.
This patch doesn't change the marking of any existing columns.
Discussion: 20150215170014.GE15326@awork2.anarazel.de
Specifically, this code chunk:
+ if (defined $attopt)
+ {
+ if ($attopt eq 'PG_FORCE_NULL')
+ {
+ $row{'forcenull'} = 1;
+ }
+ elsif ($attopt eq 'BKI_FORCE_NOT_NULL')
+ {
+ $row{'forcenotnull'} = 1;
+ }
+ else
+ {
+ die "unknown column option $attopt on column
$attname"
+ }
+ }In case of BKI_FORCE_NULL, it is ending up in else part and throwing an
error.
We should have BKI_FORCE_NULL instead of PG_FORCE_NULL in the comparison.
Attached patch which does that.
Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Attachments:
fix_make_error_with_BKI_FORCE_NULL.patchapplication/x-download; name=fix_make_error_with_BKI_FORCE_NULL.patchDownload
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index c7b1c17..ac1dafb 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -179,7 +179,7 @@ sub Catalogs
if (defined $attopt)
{
- if ($attopt eq 'PG_FORCE_NULL')
+ if ($attopt eq 'BKI_FORCE_NULL')
{
$row{'forcenull'} = 1;
}
Specifically, this code chunk:
+ if (defined $attopt) + { + if ($attopt eq 'PG_FORCE_NULL') + { + $row{'forcenull'} = 1; + } + elsif ($attopt eq 'BKI_FORCE_NOT_NULL') + { + $row{'forcenotnull'} = 1; + } + else + { + die "unknown column option $attopt on column $attname" + } + }
We should have BKI_FORCE_NULL instead of PG_FORCE_NULL in the comparison.
Ick. Thanks. Fixed.
Just out of interest and if you can answer: What are you using it for? I
guess it's AS?
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Apr 9, 2015, at 5:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Specifically, this code chunk:
+ if (defined $attopt) + { + if ($attopt eq 'PG_FORCE_NULL') + { + $row{'forcenull'} = 1; + } + elsif ($attopt eq 'BKI_FORCE_NOT_NULL') + { + $row{'forcenotnull'} = 1; + } + else + { + die "unknown column option $attopt on column $attname" + } + }We should have BKI_FORCE_NULL instead of PG_FORCE_NULL in the comparison.
Ick. Thanks. Fixed.
Just out of interest and if you can answer: What are you using it for? I
guess it's AS?
Yep.
...Robert
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers