Re: [HACKERS] NOT NULL markings for BKI columns
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
Re: [HACKERS] NOT NULL markings for BKI columns
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 and...@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
Re: [HACKERS] NOT NULL markings for BKI columns
Andres Freund and...@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
Re: [HACKERS] NOT NULL markings for BKI columns
Andres Freund wrote: On 2015-02-15 12:54:45 -0500, Tom Lane wrote: Andres Freund and...@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 Herrerahttp://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
[HACKERS] 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
Re: [HACKERS] NOT NULL markings for BKI columns
Andres Freund and...@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. */ textextversion 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 textPG_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
Re: [HACKERS] NOT NULL markings for BKI columns
On 2015-02-15 12:11:52 -0500, Tom Lane wrote: Andres Freund and...@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
Re: [HACKERS] NOT NULL markings for BKI columns
Andres Freund and...@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
Re: [HACKERS] NOT NULL markings for BKI columns
Andres Freund and...@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
Re: [HACKERS] NOT NULL markings for BKI columns
On 2015-02-15 12:31:10 -0500, Tom Lane wrote: Andres Freund and...@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. */ textextversion 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 textPG_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
