Thread: 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 andprior 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
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
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
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
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 ittext extversion PG_FORCENOTNULL; /* extension version name */ an alternative where it doesn't do that istext 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
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
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
Attachment
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
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
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
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
--
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
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Attachment
> 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
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