Re: 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

Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

Andres Freund wrote:
> On 2015-02-15 12:54:45 -0500, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)anarazel(dot)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(dot)GE15326(at)awork2(dot)anarazel(dot)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

> 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(at)2ndquadrant(dot)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