Warnings around booleans
Hi,
Forcing our bool to be stdbool.h shows up a bunch of errors and
warnings:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal->check[i]) actually is a
ternary.
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
=> rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
bool bypassrls = -1;
it's a boolean.
else if (authform->rolbypassrls || bypassrls >= 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to change bypassrls attribute")));
}
...
if (bypassrls >= 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls > 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
=> convert to an int
4) _tableInfo->relreplident is a bool, should be a char
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 2015-08-12 10:43:51 +0200, Andres Freund wrote:
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:bool bypassrls = -1;
it's a boolean.
else if (authform->rolbypassrls || bypassrls >= 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to change bypassrls attribute")));
}
...
if (bypassrls >= 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls > 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}if it's a boolean, that's always true.
It's not entirely clear to me why this was written that
way. Stephen?
Which incidentally leads to really scary results:
postgres[25069][1]=# SELECT oid, rolname, rolbypassrls FROM pg_authid WHERE rolname = 'plain';
┌───────┬─────────┬──────────────┐
│ oid │ rolname │ rolbypassrls │
├───────┼─────────┼──────────────┤
│ 76892 │ plain │ f │
└───────┴─────────┴──────────────┘
(1 row)
postgres[25069][1]=# ALTER ROLE plain NOLOGIN;
ALTER ROLE
postgres[25069][1]=# SELECT oid, rolname, rolbypassrls FROM pg_authid WHERE rolname = 'plain';
┌───────┬─────────┬──────────────┐
│ oid │ rolname │ rolbypassrls │
├───────┼─────────┼──────────────┤
│ 76892 │ plain │ t │
└───────┴─────────┴──────────────┘
(1 row)
The -1 isn't a valid value for a bool, which is now unsigned, and just
wraps around to true.
There are no tests catching this, which is a scary in it's own right.
So on linux/x86 this normally is only an issue when using a definition
for bool other than c.h's (which we explicitly try to cater for!). But
on other platforms the whole logic above is terminally broken: Consider
what happens when char is unsigned. That's e.g. the case on nearly all,
if not all, arm compilers.
This is reproducible today in an unmodified postgres on x86 if you add
-funsigned-char. Or, on any arm and possibly some other platforms.
Whoa, allowing the compiler to warn us is a good thing. This would have
been a fucktastically embarassing security issue.
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
Andres,
* Andres Freund (andres@anarazel.de) wrote:
Forcing our bool to be stdbool.h shows up a bunch of errors and
warnings:
Wow.
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal->check[i]) actually is a
ternary.
Is there a potential corruption issue from that..?
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...=> rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
I assume you mean AlterRole() above?
bool bypassrls = -1;
it's a boolean.
else if (authform->rolbypassrls || bypassrls >= 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to change bypassrls attribute")));
}
...
if (bypassrls >= 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls > 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
=> convert to an int
Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?
As for what's happening, it appears that we'll happily continue to go
through all of the databases even on an error condition, right? I seem
to recall seeing that happen and being a bit surprised at it, but wasn't
in a position at the time to debug it.
4) _tableInfo->relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?
Thanks!
Stephen
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal->check[i]) actually is a
ternary.Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...=> rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.
There's alread a _enum in the struct, so that doesn't bother my much.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:I assume you mean AlterRole() above?
Oops.
bool bypassrls = -1;
it's a boolean.
else if (authform->rolbypassrls || bypassrls >= 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to change bypassrls attribute")));
}
...
if (bypassrls >= 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls > 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().
I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
=> convert to an int
Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?
Yea, you're right.
4) _tableInfo->relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?
With our current boolean definition this doesn't fail at all. All the
values fall into both char and unsigned char range.
WRT tests: It'd probably be a good idea to not drop the tables at the
end of replica_identity.sql.
--
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@anarazel.de) wrote:
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal->check[i]) actually is a
ternary.Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the
impact of this?
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...=> rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.There's alread a _enum in the struct, so that doesn't bother my much.
Fair enough.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
[...]
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
Agreed. My role attributes patch had actually changed how that all was
done, but once the discussion moved on to default roles instead of a
bunch of additional role attributes, it felt like it would be
unnecessary code churn to change that function. Perhaps we should do so
anyway though.
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
We certainly do have tests around BYPASSRLS, but not one which changes
an independent role attribute and then re-tests. I'm fine with adding
that test (or other tests, in general), but that won't help when another
role attribute is added and someone makes a similar mistake, which I
believe the test I'm thinking of will. Even if I'm unable to make that
work though, someone grep'ing through our tests for places to add their
new role attribute would likely catch a test that's checking all of the
other ones while they might not consider what's done for just one
attribute to be a generally applicable case.
4) _tableInfo->relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?With our current boolean definition this doesn't fail at all. All the
values fall into both char and unsigned char range.WRT tests: It'd probably be a good idea to not drop the tables at the
end of replica_identity.sql.
Agreed.
Thanks!
Stephen
Hi Michael,
I'm currently investigating some of our code cleanliness issues around
booleans. Turns out that ecpg fails if C99's _Bool is used as bool
instead of typedef char bool.
Playing around a bit lead to to find that this is caused by a wrong type
declaration in two places. 'isarray' is declared as bool instead of enum
ARRAY_TYPE in two places. This appears to be an oversight, perhaps
caused by the boolean sounding name.
Does this look right to you? If so, will you apply it?
- Andres
Attachments:
0001-ecpg-Use-ARRAY_TYPE-instead-of-a-bool-to-store-the-a.patchtext/x-patch; charset=us-asciiDownload
>From 8335ad3b9b96964f0f4eac7c16fb4016c6370acb Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 12 Aug 2015 21:44:49 +0200
Subject: [PATCH] ecpg: Use ARRAY_TYPE instead of a bool to store the array
type.
Using a bool isn't just somewhat ugly, but actually fails badly if bool
is mapped to C99's _Bool. This appears to be caused by an oversight
instead of a conscious decision.
---
src/interfaces/ecpg/ecpglib/execute.c | 2 +-
src/interfaces/ecpg/ecpglib/extern.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 9a56a5c..9e40f41 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -146,7 +146,7 @@ next_insert(char *text, int pos, bool questionmarks)
}
static bool
-ecpg_type_infocache_push(struct ECPGtype_information_cache ** cache, int oid, bool isarray, int lineno)
+ecpg_type_infocache_push(struct ECPGtype_information_cache ** cache, int oid, enum ARRAY_TYPE isarray, int lineno)
{
struct ECPGtype_information_cache *new_entry
= (struct ECPGtype_information_cache *) ecpg_alloc(sizeof(struct ECPGtype_information_cache), lineno);
diff --git a/src/interfaces/ecpg/ecpglib/extern.h b/src/interfaces/ecpg/ecpglib/extern.h
index 1fa21fd..263e001 100644
--- a/src/interfaces/ecpg/ecpglib/extern.h
+++ b/src/interfaces/ecpg/ecpglib/extern.h
@@ -44,7 +44,7 @@ struct ECPGtype_information_cache
{
struct ECPGtype_information_cache *next;
int oid;
- bool isarray;
+ enum ARRAY_TYPE isarray;
};
/* structure to store one statement */
--
2.3.0.149.gf3f4077.dirty
* Andres Freund (andres@anarazel.de) wrote:
I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
Agreed, but not going to change it at this point.
Would love feedback on the attached. I included the variable renames
discussed previously with Noah as they're quite minor changes.
Had no trouble cherry-picking this back to 9.5.
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
I'll look at doing this also in the rowsecurity regression suite, but I
really like having this coverage of CREATE/ALTER ROLE too, plus testing
the role dump/restore paths in pg_dumpall which I don't think were being
covered at all previously...
Thanks!
Stephen
Attachments:
bypassrls-fix.patchtext/x-diff; charset=us-asciiDownload
From ab44660e9b8fc5b10f84ce6ba99a8340047ac8a5 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Wed, 12 Aug 2015 15:50:54 -0400
Subject: [PATCH] In AlterRole, make bypassrls an int
When reworking bypassrls in AlterRole to operate the same way the other
attribute handling is done, I missed that the variable was incorrectly a
bool rather than an int. This meant that on platforms with an unsigned
char, we could end up with incorrect behavior during ALTER ROLE.
Pointed out by Andres thanks to tests he did changing our bool to be the
one from stdbool.h which showed this and a number of other issues.
Add regression tests to test CREATE/ALTER role for the various role
attributes. Arrange to leave roles behind for testing pg_dumpall, but
none which have the LOGIN attribute.
In passing, to avoid confusion, rename CreatePolicyStmt's 'cmd' to
'cmd_name', parse_policy_command's 'cmd' to 'polcmd', and
AlterPolicy's 'cmd_datum' to 'polcmd_datum', per discussion with Noah
and as a follow-up to his correction of copynodes/equalnodes handling of
the CreatePolicyStmt 'cmd' field.
Back-patch to 9.5 where the AlterRole bug exists and to avoid the code
diverging due to minor changes while in alpha.
---
src/backend/commands/policy.c | 22 +--
src/backend/commands/user.c | 2 +-
src/backend/nodes/copyfuncs.c | 2 +-
src/backend/nodes/equalfuncs.c | 2 +-
src/backend/parser/gram.y | 2 +-
src/include/nodes/parsenodes.h | 2 +-
src/test/regress/expected/roleattributes.out | 236 +++++++++++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/roleattributes.sql | 85 ++++++++++
10 files changed, 339 insertions(+), 17 deletions(-)
create mode 100644 src/test/regress/expected/roleattributes.out
create mode 100644 src/test/regress/sql/roleattributes.sql
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index bcf4a8f..45326a3 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -108,25 +108,25 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid,
static char
parse_policy_command(const char *cmd_name)
{
- char cmd;
+ char polcmd;
if (!cmd_name)
elog(ERROR, "unrecognized policy command");
if (strcmp(cmd_name, "all") == 0)
- cmd = '*';
+ polcmd = '*';
else if (strcmp(cmd_name, "select") == 0)
- cmd = ACL_SELECT_CHR;
+ polcmd = ACL_SELECT_CHR;
else if (strcmp(cmd_name, "insert") == 0)
- cmd = ACL_INSERT_CHR;
+ polcmd = ACL_INSERT_CHR;
else if (strcmp(cmd_name, "update") == 0)
- cmd = ACL_UPDATE_CHR;
+ polcmd = ACL_UPDATE_CHR;
else if (strcmp(cmd_name, "delete") == 0)
- cmd = ACL_DELETE_CHR;
+ polcmd = ACL_DELETE_CHR;
else
elog(ERROR, "unrecognized policy command");
- return cmd;
+ return polcmd;
}
/*
@@ -480,7 +480,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
int i;
/* Parse command */
- polcmd = parse_policy_command(stmt->cmd);
+ polcmd = parse_policy_command(stmt->cmd_name);
/*
* If the command is SELECT or DELETE then WITH CHECK should be NULL.
@@ -674,7 +674,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
bool replaces[Natts_pg_policy];
ObjectAddress target;
ObjectAddress myself;
- Datum cmd_datum;
+ Datum polcmd_datum;
char polcmd;
bool polcmd_isnull;
int i;
@@ -775,11 +775,11 @@ AlterPolicy(AlterPolicyStmt *stmt)
RelationGetRelationName(target_table))));
/* Get policy command */
- cmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
+ polcmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
RelationGetDescr(pg_policy_rel),
&polcmd_isnull);
Assert(!polcmd_isnull);
- polcmd = DatumGetChar(cmd_datum);
+ polcmd = DatumGetChar(polcmd_datum);
/*
* If the command is SELECT or DELETE then WITH CHECK should be NULL.
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index afbf276..295e0b0 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -493,7 +493,7 @@ AlterRole(AlterRoleStmt *stmt)
char *validUntil = NULL; /* time the login is valid until */
Datum validUntil_datum; /* same, as timestamptz Datum */
bool validUntil_null;
- bool bypassrls = -1;
+ int bypassrls = -1;
DefElem *dpassword = NULL;
DefElem *dissuper = NULL;
DefElem *dinherit = NULL;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 1c8425d..bd2e80e 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4083,7 +4083,7 @@ _copyCreatePolicyStmt(const CreatePolicyStmt *from)
COPY_STRING_FIELD(policy_name);
COPY_NODE_FIELD(table);
- COPY_STRING_FIELD(cmd);
+ COPY_STRING_FIELD(cmd_name);
COPY_NODE_FIELD(roles);
COPY_NODE_FIELD(qual);
COPY_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 1d6c43c..19412fe 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2074,7 +2074,7 @@ _equalCreatePolicyStmt(const CreatePolicyStmt *a, const CreatePolicyStmt *b)
{
COMPARE_STRING_FIELD(policy_name);
COMPARE_NODE_FIELD(table);
- COMPARE_STRING_FIELD(cmd);
+ COMPARE_STRING_FIELD(cmd_name);
COMPARE_NODE_FIELD(roles);
COMPARE_NODE_FIELD(qual);
COMPARE_NODE_FIELD(with_check);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 426a09d..1efc6d6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4613,7 +4613,7 @@ CreatePolicyStmt:
CreatePolicyStmt *n = makeNode(CreatePolicyStmt);
n->policy_name = $3;
n->table = $5;
- n->cmd = $6;
+ n->cmd_name = $6;
n->roles = $7;
n->qual = $8;
n->with_check = $9;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 151c93a..f0dcd2f 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2039,7 +2039,7 @@ typedef struct CreatePolicyStmt
NodeTag type;
char *policy_name; /* Policy's name */
RangeVar *table; /* the table name the policy applies to */
- char *cmd; /* the command name the policy applies to */
+ char *cmd_name; /* the command name the policy applies to */
List *roles; /* the roles associated with the policy */
Node *qual; /* the policy's condition */
Node *with_check; /* the policy's WITH CHECK condition. */
diff --git a/src/test/regress/expected/roleattributes.out b/src/test/regress/expected/roleattributes.out
new file mode 100644
index 0000000..b7fded9
--- /dev/null
+++ b/src/test/regress/expected/roleattributes.out
@@ -0,0 +1,236 @@
+-- default for superuser is false
+CREATE ROLE test_def_superuser;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_superuser';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_superuser | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_superuser WITH SUPERUSER;
+SELECT * FROM pg_authid WHERE rolname = 'test_superuser';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_superuser | t | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_superuser WITH NOSUPERUSER;
+SELECT * FROM pg_authid WHERE rolname = 'test_superuser';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_superuser | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_superuser WITH SUPERUSER;
+SELECT * FROM pg_authid WHERE rolname = 'test_superuser';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_superuser | t | t | f | f | f | f | f | -1 | |
+(1 row)
+
+-- default for inherit is true
+CREATE ROLE test_def_inherit;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_inherit';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_inherit | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_inherit WITH NOINHERIT;
+SELECT * FROM pg_authid WHERE rolname = 'test_inherit';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_inherit | f | f | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_inherit WITH INHERIT;
+SELECT * FROM pg_authid WHERE rolname = 'test_inherit';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_inherit | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_inherit WITH NOINHERIT;
+SELECT * FROM pg_authid WHERE rolname = 'test_inherit';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_inherit | f | f | f | f | f | f | f | -1 | |
+(1 row)
+
+-- default for create role is false
+CREATE ROLE test_def_createrole;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_createrole';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+---------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_createrole | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_createrole WITH CREATEROLE;
+SELECT * FROM pg_authid WHERE rolname = 'test_createrole';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+-----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_createrole | f | t | t | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_createrole WITH NOCREATEROLE;
+SELECT * FROM pg_authid WHERE rolname = 'test_createrole';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+-----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_createrole | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_createrole WITH CREATEROLE;
+SELECT * FROM pg_authid WHERE rolname = 'test_createrole';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+-----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_createrole | f | t | t | f | f | f | f | -1 | |
+(1 row)
+
+-- default for create database is false
+CREATE ROLE test_def_createdb;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_createdb';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+-------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_createdb | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_createdb WITH CREATEDB;
+SELECT * FROM pg_authid WHERE rolname = 'test_createdb';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+---------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_createdb | f | t | f | t | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_createdb WITH NOCREATEDB;
+SELECT * FROM pg_authid WHERE rolname = 'test_createdb';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+---------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_createdb | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_createdb WITH CREATEDB;
+SELECT * FROM pg_authid WHERE rolname = 'test_createdb';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+---------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_createdb | f | t | f | t | f | f | f | -1 | |
+(1 row)
+
+-- default for can login is false for role
+CREATE ROLE test_def_role_canlogin;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_role_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+------------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_role_canlogin | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_role_canlogin WITH LOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_role_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_role_canlogin | f | t | f | f | t | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_role_canlogin WITH NOLOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_role_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_role_canlogin | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_role_canlogin WITH LOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_role_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_role_canlogin | f | t | f | f | t | f | f | -1 | |
+(1 row)
+
+-- default for can login is true for user
+CREATE USER test_def_user_canlogin;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_user_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+------------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_user_canlogin | f | t | f | f | t | f | f | -1 | |
+(1 row)
+
+CREATE USER test_user_canlogin WITH NOLOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_user_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_user_canlogin | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER USER test_user_canlogin WITH LOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_user_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_user_canlogin | f | t | f | f | t | f | f | -1 | |
+(1 row)
+
+ALTER USER test_user_canlogin WITH NOLOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_user_canlogin';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_user_canlogin | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+-- default for replication is false
+CREATE ROLE test_def_replication;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_replication';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_replication | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_replication WITH REPLICATION;
+SELECT * FROM pg_authid WHERE rolname = 'test_replication';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_replication | f | t | f | f | f | t | f | -1 | |
+(1 row)
+
+ALTER ROLE test_replication WITH NOREPLICATION;
+SELECT * FROM pg_authid WHERE rolname = 'test_replication';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_replication | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_replication WITH REPLICATION;
+SELECT * FROM pg_authid WHERE rolname = 'test_replication';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_replication | f | t | f | f | f | t | f | -1 | |
+(1 row)
+
+-- default for bypassrls is false
+CREATE ROLE test_def_bypassrls;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_bypassrls';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+--------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_def_bypassrls | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+CREATE ROLE test_bypassrls WITH BYPASSRLS;
+SELECT * FROM pg_authid WHERE rolname = 'test_bypassrls';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_bypassrls | f | t | f | f | f | f | t | -1 | |
+(1 row)
+
+ALTER ROLE test_bypassrls WITH NOBYPASSRLS;
+SELECT * FROM pg_authid WHERE rolname = 'test_bypassrls';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_bypassrls | f | t | f | f | f | f | f | -1 | |
+(1 row)
+
+ALTER ROLE test_bypassrls WITH BYPASSRLS;
+SELECT * FROM pg_authid WHERE rolname = 'test_bypassrls';
+ rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | rolpassword | rolvaliduntil
+----------------+----------+------------+---------------+-------------+-------------+----------------+--------------+--------------+-------------+---------------
+ test_bypassrls | f | t | f | f | f | f | t | -1 | |
+(1 row)
+
+-- remove the one role with LOGIN rights
+DROP ROLE test_role_canlogin;
+-- other roles not removed to test pg_dumpall role dump through
+-- pg_upgrade
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 4df15de..6fc5d1e 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -60,7 +60,7 @@ test: create_index create_view
# ----------
# Another group of parallel tests
# ----------
-test: create_aggregate create_function_3 create_cast constraints triggers inherit create_table_like typed_table vacuum drop_if_exists updatable_views rolenames
+test: create_aggregate create_function_3 create_cast constraints triggers inherit create_table_like typed_table vacuum drop_if_exists updatable_views rolenames roleattributes
# ----------
# sanity_check does a vacuum, affecting the sort order of SELECT *
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 15d74d4..2ae51cf 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -73,6 +73,7 @@ test: vacuum
test: drop_if_exists
test: updatable_views
test: rolenames
+test: roleattributes
test: sanity_check
test: errors
test: select
diff --git a/src/test/regress/sql/roleattributes.sql b/src/test/regress/sql/roleattributes.sql
new file mode 100644
index 0000000..9f9dd9c
--- /dev/null
+++ b/src/test/regress/sql/roleattributes.sql
@@ -0,0 +1,85 @@
+-- default for superuser is false
+CREATE ROLE test_def_superuser;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_superuser';
+CREATE ROLE test_superuser WITH SUPERUSER;
+SELECT * FROM pg_authid WHERE rolname = 'test_superuser';
+ALTER ROLE test_superuser WITH NOSUPERUSER;
+SELECT * FROM pg_authid WHERE rolname = 'test_superuser';
+ALTER ROLE test_superuser WITH SUPERUSER;
+SELECT * FROM pg_authid WHERE rolname = 'test_superuser';
+
+-- default for inherit is true
+CREATE ROLE test_def_inherit;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_inherit';
+CREATE ROLE test_inherit WITH NOINHERIT;
+SELECT * FROM pg_authid WHERE rolname = 'test_inherit';
+ALTER ROLE test_inherit WITH INHERIT;
+SELECT * FROM pg_authid WHERE rolname = 'test_inherit';
+ALTER ROLE test_inherit WITH NOINHERIT;
+SELECT * FROM pg_authid WHERE rolname = 'test_inherit';
+
+-- default for create role is false
+CREATE ROLE test_def_createrole;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_createrole';
+CREATE ROLE test_createrole WITH CREATEROLE;
+SELECT * FROM pg_authid WHERE rolname = 'test_createrole';
+ALTER ROLE test_createrole WITH NOCREATEROLE;
+SELECT * FROM pg_authid WHERE rolname = 'test_createrole';
+ALTER ROLE test_createrole WITH CREATEROLE;
+SELECT * FROM pg_authid WHERE rolname = 'test_createrole';
+
+-- default for create database is false
+CREATE ROLE test_def_createdb;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_createdb';
+CREATE ROLE test_createdb WITH CREATEDB;
+SELECT * FROM pg_authid WHERE rolname = 'test_createdb';
+ALTER ROLE test_createdb WITH NOCREATEDB;
+SELECT * FROM pg_authid WHERE rolname = 'test_createdb';
+ALTER ROLE test_createdb WITH CREATEDB;
+SELECT * FROM pg_authid WHERE rolname = 'test_createdb';
+
+-- default for can login is false for role
+CREATE ROLE test_def_role_canlogin;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_role_canlogin';
+CREATE ROLE test_role_canlogin WITH LOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_role_canlogin';
+ALTER ROLE test_role_canlogin WITH NOLOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_role_canlogin';
+ALTER ROLE test_role_canlogin WITH LOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_role_canlogin';
+
+-- default for can login is true for user
+CREATE USER test_def_user_canlogin;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_user_canlogin';
+CREATE USER test_user_canlogin WITH NOLOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_user_canlogin';
+ALTER USER test_user_canlogin WITH LOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_user_canlogin';
+ALTER USER test_user_canlogin WITH NOLOGIN;
+SELECT * FROM pg_authid WHERE rolname = 'test_user_canlogin';
+
+-- default for replication is false
+CREATE ROLE test_def_replication;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_replication';
+CREATE ROLE test_replication WITH REPLICATION;
+SELECT * FROM pg_authid WHERE rolname = 'test_replication';
+ALTER ROLE test_replication WITH NOREPLICATION;
+SELECT * FROM pg_authid WHERE rolname = 'test_replication';
+ALTER ROLE test_replication WITH REPLICATION;
+SELECT * FROM pg_authid WHERE rolname = 'test_replication';
+
+-- default for bypassrls is false
+CREATE ROLE test_def_bypassrls;
+SELECT * FROM pg_authid WHERE rolname = 'test_def_bypassrls';
+CREATE ROLE test_bypassrls WITH BYPASSRLS;
+SELECT * FROM pg_authid WHERE rolname = 'test_bypassrls';
+ALTER ROLE test_bypassrls WITH NOBYPASSRLS;
+SELECT * FROM pg_authid WHERE rolname = 'test_bypassrls';
+ALTER ROLE test_bypassrls WITH BYPASSRLS;
+SELECT * FROM pg_authid WHERE rolname = 'test_bypassrls';
+
+-- remove the one role with LOGIN rights
+DROP ROLE test_role_canlogin;
+
+-- other roles not removed to test pg_dumpall role dump through
+-- pg_upgrade
--
1.9.1
Playing around a bit lead to to find that this is caused by a wrong type
declaration in two places. 'isarray' is declared as bool instead of enum
ARRAY_TYPE in two places. This appears to be an oversight, perhaps
caused by the boolean sounding name.
I vaguely remember that it used to be bool back in the day.
Does this look right to you? If so, will you apply it?
Yes and yes. Thanks for spotting, fixed in master and the back branches.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at gmail dot com
VfL Borussia! For�a Bar�a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/12/2015 03:46 PM, Stephen Frost wrote:
* Andres Freund (andres@anarazel.de) wrote:
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal->check[i]) actually is a
ternary.Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the
impact of this?
It's harmless. gin_tsquery_consistent() places a boolean array as the
'check' array, and therefore checkcondition_gin will also only return
TRUEs and FALSEs, never MAYBEs. A comment to explain why that's OK would
probably be in order though.
- Heikki
--
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-08-13 13:27:33 +0200, Michael Meskes wrote:
Playing around a bit lead to to find that this is caused by a wrong type
declaration in two places. 'isarray' is declared as bool instead of enum
ARRAY_TYPE in two places. This appears to be an oversight, perhaps
caused by the boolean sounding name.I vaguely remember that it used to be bool back in the day.
I dug a bit back, but got tired of it around 2003 ;)
Does this look right to you? If so, will you apply it?
Yes and yes. Thanks for spotting, fixed in master and the back branches.
Thanks!
--
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-08-13 14:28:58 +0300, Heikki Linnakangas wrote:
On 08/12/2015 03:46 PM, Stephen Frost wrote:
* Andres Freund (andres@anarazel.de) wrote:
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against
MAYBE).
That bit looks sane to you? That appears to be an actual misdeclaration
to me.
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal->check[i]) actually is a
ternary.Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the
impact of this?It's harmless. gin_tsquery_consistent() places a boolean array as the
'check' array, and therefore checkcondition_gin will also only return TRUEs
and FALSEs, never MAYBEs. A comment to explain why that's OK would probably
be in order though.
Ok. As I get warnings here it seems best to add a cast when passing the
function (i.e. (bool (*) (void *, QueryOperand *)) checkcondition_gin))
and adding a comment to checkcondition_gin() explaining that it better
only return booleans?
For reference, those are the warnings.
/home/andres/src/postgresql/src/backend/access/gin/ginlogic.c: In function ‘shimTriConsistentFn’:
/home/andres/src/postgresql/src/backend/access/gin/ginlogic.c:171:24: warning: comparison of constant ‘2’ with boolean expression is always false [-Wbool-compare]
if (key->entryRes[i] == GIN_MAYBE)
^
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c: In function ‘gin_tsquery_consistent’:
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c:287:13: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
gcv.check = check;
^
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c:294:8: warning: passing argument 4 of ‘TS_execute’ from incompatible pointer type [-Wincompatible-pointer-types]
checkcondition_gin);
^
In file included from /home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c:20:0:
/home/andres/src/postgresql/src/include/tsearch/ts_utils.h:107:13: note: expected ‘_Bool (*)(void *, QueryOperand *) {aka _Bool (*)(void *, struct <anonymous> *)}’ but argument is of type ‘GinTernaryValue (*)(void *, QueryOperand *) {aka char (*)(void *, struct <anonymous> *)}’
extern bool TS_execute(QueryItem *curitem, void *checkval, bool calcnot,
^
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 08/13/2015 02:41 PM, Andres Freund wrote:
On 2015-08-13 14:28:58 +0300, Heikki Linnakangas wrote:
On 08/12/2015 03:46 PM, Stephen Frost wrote:
* Andres Freund (andres@anarazel.de) wrote:
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
be a GinTernaryValue (it's actually is compared against
MAYBE).That bit looks sane to you? That appears to be an actual misdeclaration
to me.
Yeah, changing entryRes to GinTernaryValue seems good to me.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: 4424_1439466152_55CC82A7_4424_54_1_20150813114154.GB8988@awork2.anarazel.de
Hi,
On 2015-08-12 16:46:01 -0400, Stephen Frost wrote:
From ab44660e9b8fc5b10f84ce6ba99a8340047ac8a5 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Wed, 12 Aug 2015 15:50:54 -0400
Subject: [PATCH] In AlterRole, make bypassrls an intWhen reworking bypassrls in AlterRole to operate the same way the other
attribute handling is done, I missed that the variable was incorrectly a
bool rather than an int. This meant that on platforms with an unsigned
char, we could end up with incorrect behavior during ALTER ROLE.Pointed out by Andres thanks to tests he did changing our bool to be the
one from stdbool.h which showed this and a number of other issues.Add regression tests to test CREATE/ALTER role for the various role
attributes. Arrange to leave roles behind for testing pg_dumpall, but
none which have the LOGIN attribute.In passing, to avoid confusion, rename CreatePolicyStmt's 'cmd' to
'cmd_name', parse_policy_command's 'cmd' to 'polcmd', and
AlterPolicy's 'cmd_datum' to 'polcmd_datum', per discussion with Noah
and as a follow-up to his correction of copynodes/equalnodes handling of
the CreatePolicyStmt 'cmd' field.
I'd rather see those split into separate commits. Doing polishing and
active bugs in one commit imo isn't a good idea if the polishing goes
beyond a line or two.
Otherwise this looks ok to me.
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 Mon, Aug 17, 2015 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
I'd rather see those split into separate commits. Doing polishing and
active bugs in one commit imo isn't a good idea if the polishing goes
beyond a line or two.
+1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
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@anarazel.de) wrote:
I'd rather see those split into separate commits. Doing polishing and
active bugs in one commit imo isn't a good idea if the polishing goes
beyond a line or two.Otherwise this looks ok to me.
Done that way.
Thanks!
Stephen
If I'm not mistaken, the roles introduced in this test are never
dropped, which will cause the test to fail on consequent runs.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Friday, August 21, 2015, Piotr Stefaniak <postgres@piotr-stefaniak.me>
wrote:
If I'm not mistaken, the roles introduced in this test are never dropped,
which will cause the test to fail on consequent runs.
Ah, I was thinking there was a reason to not leave them around but couldn't
think of it and liked the idea of testing the pg_dumpall / pg_upgrade code
paths associated.
Perhaps the way to address this is to add a bit of code to drop them if
they exist? That would be reasonably straight-forward to do, I believe.
Will take a look at that.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
On Friday, August 21, 2015, Piotr Stefaniak <postgres@piotr-stefaniak.me>
wrote:If I'm not mistaken, the roles introduced in this test are never dropped,
which will cause the test to fail on consequent runs.
Ah, I was thinking there was a reason to not leave them around but couldn't
think of it and liked the idea of testing the pg_dumpall / pg_upgrade code
paths associated.
Perhaps the way to address this is to add a bit of code to drop them if
they exist? That would be reasonably straight-forward to do, I believe.
It is not really acceptable to leave roles hanging around after "make
installcheck"; that would be a security hazard for the installation.
Please drop them.
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 Friday, August 21, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Stephen Frost <sfrost@snowman.net <javascript:;>> writes:
On Friday, August 21, 2015, Piotr Stefaniak <postgres@piotr-stefaniak.me
<javascript:;>>
wrote:
If I'm not mistaken, the roles introduced in this test are never
dropped,
which will cause the test to fail on consequent runs.
Ah, I was thinking there was a reason to not leave them around but
couldn't
think of it and liked the idea of testing the pg_dumpall / pg_upgrade
code
paths associated.
Perhaps the way to address this is to add a bit of code to drop them if
they exist? That would be reasonably straight-forward to do, I believe.It is not really acceptable to leave roles hanging around after "make
installcheck"; that would be a security hazard for the installation.
Please drop them.
The only ones which were left were intentionally all NOLOGIN to address
that concern, which I had considered. Is there another issue with them
beyond potential login that I'm missing?
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
On Friday, August 21, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It is not really acceptable to leave roles hanging around after "make
installcheck"; that would be a security hazard for the installation.
Please drop them.
The only ones which were left were intentionally all NOLOGIN to address
that concern, which I had considered. Is there another issue with them
beyond potential login that I'm missing?
NOLOGIN addresses the most obvious abuse potential, but it hardly seems
like the only risk. And we have never yet intended the main regression
tests to serve as a testbed for "pg_dumpall -g". A bugfix commit is
not the place to start changing that policy.
(If you want to have some testing in this area, perhaps adding roles
during the pg_upgrade test would be a safer place to do 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
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Stephen Frost <sfrost@snowman.net> writes:
On Friday, August 21, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It is not really acceptable to leave roles hanging around after "make
installcheck"; that would be a security hazard for the installation.
Please drop them.The only ones which were left were intentionally all NOLOGIN to address
that concern, which I had considered. Is there another issue with them
beyond potential login that I'm missing?NOLOGIN addresses the most obvious abuse potential, but it hardly seems
like the only risk. And we have never yet intended the main regression
tests to serve as a testbed for "pg_dumpall -g". A bugfix commit is
not the place to start changing that policy.
I've updated the test to drop the roles at the end.
(If you want to have some testing in this area, perhaps adding roles
during the pg_upgrade test would be a safer place to do it.)
I'll look into this. The lack of pg_dumpall testing is pretty
concerning, considering how important it is to pg_upgrade.
Thanks!
Stephen