Re: [HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS
On Thu, Mar 2, 2017 at 10:27 AM, David Steele <da...@pgmasters.net> wrote:
> It looks like this patch is still waiting on an update for tab
> completion in psql.
Hi All,
Sorry about the long delay... It was so simple to add it to tab-complete.c
that is a shame I didn't do it before, very sorry about that.
Attached the new version of the patch that is basically the same as
previously with the addition to tab completion for psql and rebased with
master.
Hope it is enough. Thank you all.
--
Matheus de Oliveira
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 04064d3..e3363f8 100644
*** a/doc/src/sgml/ref/alter_default_privileges.sgml
--- b/doc/src/sgml/ref/alter_default_privileges.sgml
***
*** 46,51 GRANT { USAGE | ALL [ PRIVILEGES ] }
--- 46,55
ON TYPES
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
REVOKE [ GRANT OPTION FOR ]
{ { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
[, ...] | ALL [ PRIVILEGES ] }
***
*** 71,76 REVOKE [ GRANT OPTION FOR ]
--- 75,86
ON TYPES
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
+
+ REVOKE [ GRANT OPTION FOR ]
+ { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
***
*** 81,88 REVOKE [ GRANT OPTION FOR ]
ALTER DEFAULT PRIVILEGES allows you to set the privileges
that will be applied to objects created in the future. (It does not
affect privileges assigned to already-existing objects.) Currently,
!only the privileges for tables (including views and foreign tables),
!sequences, functions, and types (including domains) can be altered.
--- 91,99
ALTER DEFAULT PRIVILEGES allows you to set the privileges
that will be applied to objects created in the future. (It does not
affect privileges assigned to already-existing objects.) Currently,
!only the privileges for schemas, tables (including views and foreign
!tables), sequences, functions, and types (including domains) can be
!altered.
***
*** 125,130 REVOKE [ GRANT OPTION FOR ]
--- 136,143
are altered for objects later created in that schema.
If IN SCHEMA is omitted, the global default privileges
are altered.
+ IN SCHEMA is not allowed when using ON SCHEMAS
+ as schemas can't be nested.
diff --git a/src/backend/catalog/aclchk.c b/src/backeindex d01930f..2d535c2 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 959,964 ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
--- 959,968
all_privileges = ACL_ALL_RIGHTS_TYPE;
errormsg = gettext_noop("invalid privilege type %s for type");
break;
+ case ACL_OBJECT_NAMESPACE:
+ all_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ errormsg = gettext_noop("invalid privilege type %s for schema");
+ break;
default:
elog(ERROR, "unrecognized GrantStmt.objtype: %d",
(int) action->objtype);
***
*** 1146,1151 SetDefaultACL(InternalDefaultACL *iacls)
--- 1150,1165
this_privileges = ACL_ALL_RIGHTS_TYPE;
break;
+ case ACL_OBJECT_NAMESPACE:
+ if (OidIsValid(iacls->nspid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS")));
+ objtype = DEFACLOBJ_NAMESPACE;
+ if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+ this_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ break;
+
default:
elog(ERROR, "unrecognized objtype: %d",
(int) iacls->objtype);
***
*** 1369,1374 RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
--- 1383,1391
case DEFACLOBJ_TYPE:
iacls.objtype = ACL_OBJECT_TYPE;
break;
+ case DEFACLOBJ_NAMESPACE:
+ iacls.objtype = ACL_OBJECT_NAMESPACE;
+ break;
default:
/* Shouldn't get here */
elog(ERROR, "unexpected default ACL type: %d",
***
*** 5259,5264 get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid)
--- 5276,5285
defaclobjtype = DEFACLOBJ_TYPE;
break;
+ case ACL_OBJECT_NAMESPACE:
+ defaclobjtype = DEFACLOBJ_NAMESPACE;
+ break;
+
default:
return NULL;
}
diff --git a/src/backend/catalog/obindex 2948d64..1eb7930 100644
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
***
*** 184
Re: [HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS
On Mon, Jan 9, 2017 at 10:58 AM, Ashutosh Sharma <ashu.coe...@gmail.com>
wrote:
> > Also, should I add translations for that error message in other
> languages (I
> > can do that without help of tools for pt_BR) or is that a latter process
> in
> > the releasing?
> >
>
> I think you should add it but i am not sure when it is done.
>
>
I'll ask one of the guys who work with pt_BR translations (I know him in
person).
> > Other than that, I added a few regression tests (similar to others used
> for
> > ADP), and patched the documentation (my English is not that good, so I'm
> > open to suggestions). Anything else I forgot?
>
> You have forgot to change the description section of "ADP". In the
> description section you need to mention that privileges for schemas
> too can be altered along with other database objects.
Oh... Indeed an oversight, thanks for pointing that out.
> Other than that,
> I feel the patch looks good and has no bug.
Attached a rebased version and with the docs update pointed by Ashutosh
Sharma.
Best regards,
--
Matheus de Oliveira
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 04064d3..e3363f8 100644
*** a/doc/src/sgml/ref/alter_default_privileges.sgml
--- b/doc/src/sgml/ref/alter_default_privileges.sgml
***
*** 46,51 GRANT { USAGE | ALL [ PRIVILEGES ] }
--- 46,55
ON TYPES
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
REVOKE [ GRANT OPTION FOR ]
{ { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
[, ...] | ALL [ PRIVILEGES ] }
***
*** 71,76 REVOKE [ GRANT OPTION FOR ]
--- 75,86
ON TYPES
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
+
+ REVOKE [ GRANT OPTION FOR ]
+ { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
***
*** 81,88 REVOKE [ GRANT OPTION FOR ]
ALTER DEFAULT PRIVILEGES allows you to set the privileges
that will be applied to objects created in the future. (It does not
affect privileges assigned to already-existing objects.) Currently,
!only the privileges for tables (including views and foreign tables),
!sequences, functions, and types (including domains) can be altered.
--- 91,99
ALTER DEFAULT PRIVILEGES allows you to set the privileges
that will be applied to objects created in the future. (It does not
affect privileges assigned to already-existing objects.) Currently,
!only the privileges for schemas, tables (including views and foreign
!tables), sequences, functions, and types (including domains) can be
!altered.
***
*** 125,130 REVOKE [ GRANT OPTION FOR ]
--- 136,143
are altered for objects later created in that schema.
If IN SCHEMA is omitted, the global default privileges
are altered.
+ IN SCHEMA is not allowed when using ON SCHEMAS
+ as schemas can't be nested.
diff --git a/src/backend/catalog/aclchk.c b/src/backeindex 7803d0d..9d64ab6 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 950,955 ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
--- 950,959
all_privileges = ACL_ALL_RIGHTS_TYPE;
errormsg = gettext_noop("invalid privilege type %s for type");
break;
+ case ACL_OBJECT_NAMESPACE:
+ all_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ errormsg = gettext_noop("invalid privilege type %s for schema");
+ break;
default:
elog(ERROR, "unrecognized GrantStmt.objtype: %d",
(int) action->objtype);
***
*** 1137,1142 SetDefaultACL(InternalDefaultACL *iacls)
--- 1141,1156
this_privileges = ACL_ALL_RIGHTS_TYPE;
break;
+ case ACL_OBJECT_NAMESPACE:
+ if (OidIsValid(iacls->nspid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS")));
+ objtype = DEFACLOBJ_NAMESPACE;
+ if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+ this_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ break;
+
default:
elog(ERROR, "unrecognized objtype: %d",
(int) iacls->objtype);
***
*** 1363,1368 RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
--- 1377,1385
case DEFACLOBJ_TYPE:
iacls.objtype = ACL_OBJECT_TYPE;
break;
+ case DEFACL
Re: [HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS
Just sending the same patch but rebase with current master (it was broken
for gram.y after new commits).
Best regards,
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 04064d3..7745792 100644
*** a/doc/src/sgml/ref/alter_default_privileges.sgml
--- b/doc/src/sgml/ref/alter_default_privileges.sgml
***
*** 46,51 GRANT { USAGE | ALL [ PRIVILEGES ] }
--- 46,55
ON TYPES
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
REVOKE [ GRANT OPTION FOR ]
{ { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
[, ...] | ALL [ PRIVILEGES ] }
***
*** 71,76 REVOKE [ GRANT OPTION FOR ]
--- 75,86
ON TYPES
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
+
+ REVOKE [ GRANT OPTION FOR ]
+ { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
***
*** 125,130 REVOKE [ GRANT OPTION FOR ]
--- 135,142
are altered for objects later created in that schema.
If IN SCHEMA is omitted, the global default privileges
are altered.
+ IN SCHEMA is not allowed when using ON SCHEMAS
+ as schemas can't be nested.
diff --git a/src/backend/catalog/aclchk.c b/src/backeindex c0df671..e1256f1 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 948,953 ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
--- 948,957
all_privileges = ACL_ALL_RIGHTS_TYPE;
errormsg = gettext_noop("invalid privilege type %s for type");
break;
+ case ACL_OBJECT_NAMESPACE:
+ all_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ errormsg = gettext_noop("invalid privilege type %s for schema");
+ break;
default:
elog(ERROR, "unrecognized GrantStmt.objtype: %d",
(int) action->objtype);
***
*** 1135,1140 SetDefaultACL(InternalDefaultACL *iacls)
--- 1139,1154
this_privileges = ACL_ALL_RIGHTS_TYPE;
break;
+ case ACL_OBJECT_NAMESPACE:
+ if (OidIsValid(iacls->nspid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS")));
+ objtype = DEFACLOBJ_NAMESPACE;
+ if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+ this_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ break;
+
default:
elog(ERROR, "unrecognized objtype: %d",
(int) iacls->objtype);
***
*** 1361,1366 RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
--- 1375,1383
case DEFACLOBJ_TYPE:
iacls.objtype = ACL_OBJECT_TYPE;
break;
+ case DEFACLOBJ_NAMESPACE:
+ iacls.objtype = ACL_OBJECT_NAMESPACE;
+ break;
default:
/* Shouldn't get here */
elog(ERROR, "unexpected default ACL type: %d",
***
*** 5189,5194 get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid)
--- 5206,5215
defaclobjtype = DEFACLOBJ_TYPE;
break;
+ case ACL_OBJECT_NAMESPACE:
+ defaclobjtype = DEFACLOBJ_NAMESPACE;
+ break;
+
default:
return NULL;
}
diff --git a/src/backend/catalog/obindex d531d17..23d6684 100644
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
***
*** 1788,1798 get_object_address_defacl(List *objname, List *objargs, bool missing_ok)
case DEFACLOBJ_TYPE:
objtype_str = "types";
break;
default:
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("unrecognized default ACL object type %c", objtype),
! errhint("Valid object types are \"r\", \"S\", \"f\", and \"T\".")));
}
/*
--- 1788,1801
case DEFACLOBJ_TYPE:
objtype_str = "types";
break;
+ case DEFACLOBJ_NAMESPACE:
+ objtype_str = "schemas";
+ break;
default:
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("unrecognized default ACL object type %c", objtype),
! errhint("Valid object types are \"r\", \"S\", \"f\", \"T\" and \"s\".")));
}
/*
***
*** 3093,3098 getObjectDescription(const ObjectAddress *object)
--- 3096,3106
_("default privileges on new types belonging to role %s"),
GetUserNameFromId(defacl->defaclrole, false));
break;
+ case DEFACLOBJ_NAMESPACE:
+ appendStringInfo(,
+ _("default privileges on new schemas belonging to role %s"),
+ GetUserNameFromId(defacl->defaclrole, false));
+ break;
default:
/* shouldn't get here */
appendStringInfo(,
[HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS
Hi all,
I noticed that we have no option to set default privileges for newly
created schemas, other than calling GRANT explicitly. At work I use ALTER
DEFAULT PRIVILEGE (ADP) command extensively, as the developers are
permitted to manage DDL on the databases, and all work fine except for when
a new schema is created. So,I'd like to propose this very simple patch
(attached) that adds the capability of using SCHEMAS, adding the following
syntax to ADP:
ALTER DEFAULT PRIVILEGES
[ FOR { ROLE | USER } target_role [, ...] ]
abbreviated_grant_or_revoke
where abbreviated_grant_or_revoke is one of:
GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
ON SCHEMAS
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
REVOKE [ GRANT OPTION FOR ]
{ USAGE | CREATE | ALL [ PRIVILEGES ] }
ON SCHEMAS
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
The patch itself is really straight forward (I'm new to sending patches, so
I've chosen a simple one), and there is only one thing that concerns me (as
in, if I did it right/good). The difference in syntax for SCHEMAS and the
other objects is that IN SCHEMA option makes no sense here (as we don't
have nested schemas), and to solve that I simple added the error "cannot
use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS".
Does that look good to you?
Also, should I add translations for that error message in other languages
(I can do that without help of tools for pt_BR) or is that a latter process
in the releasing?
Other than that, I added a few regression tests (similar to others used for
ADP), and patched the documentation (my English is not that good, so I'm
open to suggestions). Anything else I forgot?
While at this, I'd like to ask if you are interested in have all the other
types we have in GRANT/REVOKE for ADP (I myself see few use for that at
work, but the symmetry on those commands seems like a good idea). If you
agree, I can take some time to do the others (looks very simple to do). I
just wonder if that should be done as one patch for each, or just a single
patch for all of them (perhaps send the sequence of patches in order, as
certainly one will conflict with the other if done apart).
Best regards,
--
Matheus de Oliveira
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 04064d3..7745792 100644
*** a/doc/src/sgml/ref/alter_default_privileges.sgml
--- b/doc/src/sgml/ref/alter_default_privileges.sgml
***
*** 46,51 GRANT { USAGE | ALL [ PRIVILEGES ] }
--- 46,55
ON TYPES
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
REVOKE [ GRANT OPTION FOR ]
{ { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
[, ...] | ALL [ PRIVILEGES ] }
***
*** 71,76 REVOKE [ GRANT OPTION FOR ]
--- 75,86
ON TYPES
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
+
+ REVOKE [ GRANT OPTION FOR ]
+ { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
***
*** 125,130 REVOKE [ GRANT OPTION FOR ]
--- 135,142
are altered for objects later created in that schema.
If IN SCHEMA is omitted, the global default privileges
are altered.
+ IN SCHEMA is not allowed when using ON SCHEMAS
+ as schemas can't be nested.
diff --git a/src/backend/catalog/aclchk.c b/src/backeindex c0df671..e1256f1 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 948,953 ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
--- 948,957
all_privileges = ACL_ALL_RIGHTS_TYPE;
errormsg = gettext_noop("invalid privilege type %s for type");
break;
+ case ACL_OBJECT_NAMESPACE:
+ all_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ errormsg = gettext_noop("invalid privilege type %s for schema");
+ break;
default:
elog(ERROR, "unrecognized GrantStmt.objtype: %d",
(int) action->objtype);
***
*** 1135,1140 SetDefaultACL(InternalDefaultACL *iacls)
--- 1139,1154
this_privileges = ACL_ALL_RIGHTS_TYPE;
break;
+ case ACL_OBJECT_NAMESPACE:
+ if (OidIsValid(iacls->nspid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS")));
+ objtype = DEFACLOBJ_NAMESPACE;
+ if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+ this_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ break;
+
Re: [HACKERS] How about a proper TEMPORARY TABLESPACE?
On Mon, Jun 30, 2014 at 5:14 AM, Craig Ringer cr...@2ndquadrant.com wrote: Right now PostgreSQL appears to rely on the absence of the tablespace directory as a flag to tell it don't start up, something's badly wrong here. Well, in fact the behavior is just to give a LOG message: LOG: could not open tablespace directory pg_tblspc/100607/PG_9.3_201306121: No such file or directory But it starts fine. No? I ask because I'm relying on that. My patch does not on startup, so in the absence of the tablespace directory, the above LOG is shown even for temporary-tablespace, and it tries to create the directory when any process tries to use it. As we don't have catalog access on startup, it will require me to use some kind of _init fork for tablespace if we want it to be re-created during startup. Should we bother about this? If the user creates the tablespace directory, it figures they at least know what they're doing and carries on starting up with the empty tablespace. It's treated as an admin statement - I know it's gone, it's not coming back, just carry on as best you can. Well, I think for a tablespace like that it is okay to create it on-demand (note that I have done this only for temp, not ordinary ones). If Pg were to auto-create the directory, then if (say) a mount of a tablespace dir failed at boot, Pg would still happily start up and might create files in the tablespace, despite there being inaccessible tables/indexes/etc on the not-mounted volume that's *supposed* to be the tablespace storage. That'd create plenty of mess. So no, I don't think Pg should auto-create the tablespace directory if it's missing for non-temporary tablespaces. Ok. I agree. Let's create only for temporary ones (as done by the patch now). Not unless the current (less than friendly) behaviour around lost tablespaces is replaced with something like an `ignore_missing_tablespaces` or `drop_missing_tablespaces` GUC - akin to our existing `zero_missing_pages` recovery GUC. You meant `zero_damaged_pages`, I think. I don't think that is really necessary in this case. Anyway, I was thinking about some way to this tablespace to also allow creation of non-temporary indexes, and after a crash we could mark such indexes as `NOT VALID` (perhaps `zero_damaged_pages` could be of help here?!). That should be part of another patch thought, and would require more thoughts. But I think that would be a great improvement for some environments, like those on AWS with their ephemeral SSDs. Regards, -- Matheus de Oliveira Analista de Banco de Dados Dextra Sistemas - MPS.Br nível F! www.dextra.com.br/postgres
Re: [HACKERS] How about a proper TEMPORARY TABLESPACE?
Hi Hackers,
I have worked on that patch a little more. So now I have functional patch
(although still WIP) attached. The feature works as following:
- Added a boolean parameter only_temp_files to pg_tablespace.spcoptions;
- This parameter can be set to true only during CREATE TABLESPACE, not on
ALTER TABLESPACE (I have thought of ways of implementing the latter, and
I'd like to discuss it more latter);
- On the creation of relations, it is checked if it is a
temporary-tablespace, and an error occurs when it is and the relation is
not temporary (temp table or index on a temp table);
- When a temporary file (either relation file or sort/agg file) is created
inside a temporary-tablespace, the entire directories structure is created
on-demand (e.g. if pg_tblspc/oid/TABLESPACE_VERSION_DIRECTORY is
missing, it is created on demand) it is done on
OpenTemporaryFileInTablespace, at fd.c (I wonder if shouldn't we do that
for any tablespace) and on TablespaceCreateDbspace, at tablespace.c.
I still haven't change documentation, as I think I need some insights about
the changes. I have some more thoughts about the syntax and I still think
that TEMP LOCATION syntax is better suited for this patch. First because
of the nature of the changes I made, it seems more suitable to a column on
pg_tablespace rather than an option. Second because no ALTER is available
(so far) and I think it is odd to have an option that can't be changed.
Third, I think TEMP keyword is more clear and users can be more used to
it.
Thoughts?
I'm going to add the CF app entry next. Could I get some review now or
after discussion about how things are going (remember I'm a newbie on this,
so I'm a little lost)?
Regards,
--
Matheus de Oliveira
Analista de Banco de Dados
Dextra Sistemas - MPS.Br nível F!
www.dextra.com.br/postgres
*** a/src/backend/access/common/reloptions.c
--- b/src/backend/access/common/reloptions.c
***
*** 71,76 static relopt_bool boolRelOpts[] =
--- 71,84
},
{
{
+ only_temp_files,
+ Allow only temporary files to be created on this tablespace,
+ RELOPT_KIND_TABLESPACE
+ },
+ false
+ },
+ {
+ {
fastupdate,
Enables \fast update\ feature for this GIN index,
RELOPT_KIND_GIN
***
*** 1337,1343 tablespace_reloptions(Datum reloptions, bool validate)
int numoptions;
static const relopt_parse_elt tab[] = {
{random_page_cost, RELOPT_TYPE_REAL, offsetof(TableSpaceOpts, random_page_cost)},
! {seq_page_cost, RELOPT_TYPE_REAL, offsetof(TableSpaceOpts, seq_page_cost)}
};
options = parseRelOptions(reloptions, validate, RELOPT_KIND_TABLESPACE,
--- 1345,1352
int numoptions;
static const relopt_parse_elt tab[] = {
{random_page_cost, RELOPT_TYPE_REAL, offsetof(TableSpaceOpts, random_page_cost)},
! {seq_page_cost, RELOPT_TYPE_REAL, offsetof(TableSpaceOpts, seq_page_cost)},
! {only_temp_files, RELOPT_TYPE_BOOL, offsetof(TableSpaceOpts, only_temp_files)}
};
options = parseRelOptions(reloptions, validate, RELOPT_KIND_TABLESPACE,
*** a/src/backend/commands/dbcommands.c
--- b/src/backend/commands/dbcommands.c
***
*** 394,399 createdb(const CreatedbStmt *stmt)
--- 394,405
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(pg_global cannot be used as default tablespace)));
+ /* can't create a database on temporary tablespace */
+ if (is_tablespace_temp_only(dst_deftablespace))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(this tablespace only allows temporary files)));
+
/*
* If we are trying to change the default tablespace of the template,
* we require that the template not have any files in the new default
***
*** 1083,1088 movedb(const char *dbname, const char *tblspcname)
--- 1089,1100
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(pg_global cannot be used as default tablespace)));
+ /* can't create a database on temporary tablespace */
+ if (is_tablespace_temp_only(dst_tblspcoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(this tablespace only allows temporary files)));
+
/*
* No-op if same tablespace
*/
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***
*** 432,437 DefineIndex(Oid relationId,
--- 432,446
get_tablespace_name(tablespaceId));
}
+ /* Can't save relations on temporary tablespace */
+ if (rel-rd_rel-relpersistence != RELPERSISTENCE_TEMP
+ is_tablespace_temp_only(OidIsValid(tablespaceId) ? tablespaceId : MyDatabaseTableSpace))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(this tablespace only allows temporary files)));
+ }
+
/*
* Force shared indexes into the pg_global tablespace. This is a bit of a
* hack but seems simpler than marking them in the BKI commands. On the
*** a/src/backend
Re: [HACKERS] How about a proper TEMPORARY TABLESPACE?
On Sun, Jun 22, 2014 at 2:35 AM, Craig Ringer cr...@2ndquadrant.com wrote: A way to put UNLOGGED objects in such a space and have them recovered if they vanish would also be valuable, IMO. Not necessarily in the same patch, I'd just rather keep it in mind so any chosen design doesn't preclude adding that later. The idea is nice, but I think you should think more about it. Were would we put the init files in this case? It surely can't be in the tablespace. Best regards, -- Matheus de Oliveira Analista de Banco de Dados Dextra Sistemas - MPS.Br nível F! www.dextra.com.br/postgres
Re: [HACKERS] How about a proper TEMPORARY TABLESPACE?
I was going to answer each message, but Fabrízio summarized everything so far really nicely. Thanks Fabrízio. On Wed, Jun 18, 2014 at 5:25 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: Then, to summarize Matheus must do: * use an option instead of change the syntax and catalog to indicate that a tablespace is used to store temp objects Yes. I myself wasn't sure TEMPORARY syntax would be good, but I would just like to hear more about. Storing as an options seems nice to me. * throw an exception if we try ALTER the option only_temp_relations I think we should postpone the ALTER option, perhaps as another patch. Wasn't that what was decided? I'll investigate the options better before going this route. Let me work on CREATE first. * add regression tests * add documentation And, of course, register to the next open commitfest [1] to get detailed feedback and review. Noted. It will be on the next commitfest. Just knowing some people do want this make me more comfortable on working better. Best regards, -- Matheus de Oliveira Analista de Banco de Dados Dextra Sistemas - MPS.Br nível F! www.dextra.com.br/postgres
[HACKERS] How about a proper TEMPORARY TABLESPACE?
Hi Hackers,
I was facing a situation were we wanted to set temp_tablespaces to a
tablespace on a ephemeral disk (yes, it is AWS ephemeral disk), and I know
many users have faced the same situation. Although it seems safe to create
a tablespace on ephemeral disks if you use it to store only temporary files
(either created by queries or temp tables), PostgreSQL does not have such
information, and can't safely prevent a careless user of creating a
non-temporary relation on this tablespace.
Also, in case of a lost of this tablespace, its PG_TEMP_FILES_DIR should be
created by hand after recovering. That is fine actually, but many users may
not even noticed that.
So, what you guys think about letting PG know somehow that a tablespace is
temporary?
I have took some small time to make a PoC just to see if that is doable.
And so I did a new syntax like:
CREATE TABLESPACE spcname [TEMP | TEMPORARY] LOCATION ...
So, if TEMP or TEMPORARY is present, I mark a new column at pg_tablespace
as true. On every table creation or moving to a new tablespace, I just
check this, and fails if the tablespace is temporary but the
relpersistence says the table is not.
The other part is, every time some query or relation is asked to be stored
on this tablespace, I create (on-demand) the PG_TEMP_FILES_DIR inside of it
(also if it is temporary).
The attached patch (and also on my Github, [1]), shows the PoC. For now,
I'm not worried about the code quality, there are yet a lot of work to be
done there (like ALTER TABLESPACE, better testing, use relcache, etc...),
and it is my first hacking on PG (so I'm a newbie). But I'd like to hear
from you guys if such feature is desirable and if I could starting working
on that for real. Also some thoughts about better way of implementing it.
[1] https://github.com/matheusoliveira/postgres/compare/my_temptablespace
Waiting for thoughts on that.
Best regards,
--
Matheus de Oliveira
Analista de Banco de Dados
Dextra Sistemas - MPS.Br nível F!
www.dextra.com.br/postgres
*** a/src/backend/commands/dbcommands.c
--- b/src/backend/commands/dbcommands.c
***
*** 388,399 createdb(const CreatedbStmt *stmt)
--- 388,406
aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
tablespacename);
+
/* pg_global must never be the default tablespace */
if (dst_deftablespace == GLOBALTABLESPACE_OID)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(pg_global cannot be used as default tablespace)));
+ /* can't create a database on temporary tablespace */
+ if (is_tablespace_storage_temporary(dst_deftablespace))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(cannot create a database on a tablespace in temporary storage)));
+
/*
* If we are trying to change the default tablespace of the template,
* we require that the template not have any files in the new default
***
*** 1083,1088 movedb(const char *dbname, const char *tblspcname)
--- 1090,1101
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(pg_global cannot be used as default tablespace)));
+ /* can't create a database on temporary tablespace */
+ if (is_tablespace_storage_temporary(dst_tblspcoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(cannot create a database on a tablespace in temporary storage)));
+
/*
* No-op if same tablespace
*/
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***
*** 432,437 DefineIndex(Oid relationId,
--- 432,446
get_tablespace_name(tablespaceId));
}
+ /* Can't save relations on temporary tablespace */
+ if (rel-rd_rel-relpersistence != RELPERSISTENCE_TEMP
+ is_tablespace_storage_temporary(OidIsValid(tablespaceId) ? tablespaceId : MyDatabaseTableSpace))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(cannot save relation on a tablespace in temporary storage)));
+ }
+
/*
* Force shared indexes into the pg_global tablespace. This is a bit of a
* hack but seems simpler than marking them in the BKI commands. On the
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 523,528 DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
--- 523,537
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(only shared relations can be placed in pg_global tablespace)));
+ /* Can't save relations on temporary tablespace */
+ if (stmt-relation-relpersistence != RELPERSISTENCE_TEMP
+ is_tablespace_storage_temporary(OidIsValid(tablespaceId) ? tablespaceId : MyDatabaseTableSpace))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(cannot save relation on a tablespace in temporary storage)));
+ }
+
/* Identify user ID that will own the table */
if (!OidIsValid(ownerId
Re: [HACKERS] Bug in visibility map WAL-logging
On Tue, Jan 7, 2014 at 10:42 PM, Matheus de Oliveira matioli.math...@gmail.com wrote: How did you set up the standby? Did you initialize it from an offline backup of the master's data directory, perhaps? The log shows that the startup took the the crash recovery first, then start archive recovery path, because there was no backup label file. In that mode, the standby assumes that the system is consistent after replaying all the WAL in pg_xlog, which is correct if you initialize from an offline backup or atomic filesystem snapshot, for example. But WAL contains references to invalid pages could also be a symptom of an inconsistent base backup, cause by incorrect backup procedure. In particular, I have to ask because I've seen it before: you didn't delete backup_label from the backup, did you? Well, I cannot answer this right now, but makes all sense and is possible. I've just confirmed. That was indeed the case, the script was removing the backup_label. I've just removed this line and synced it again, it is running nice (for past 1 hour at least). Thank you guys for all your help, and sorry for all the confusion I caused. Best regards, -- Matheus de Oliveira Analista de Banco de Dados Dextra Sistemas - MPS.Br nível F! www.dextra.com.br/postgres
Re: [HACKERS] Bug in visibility map WAL-logging
Hi folks, On Fri, Dec 13, 2013 at 9:47 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: lazy_vacuum_page() does this: 1. START_CRIT_SECTION() 2. Remove dead tuples from the page, marking the itemid's unused. 3. MarkBufferDirty 4. if all remaining tuples on the page are visible to everyone, set the all-visible flag on the page, and call visibilitymap_set() to set the VM bit. 5 visibilitymap_set() writes a WAL record about setting the bit in the visibility map. 6. write the WAL record of removing the dead tuples. 7. END_CRIT_SECTION(). See the problem? Setting the VM bit is WAL-logged first, before the removal of the tuples. If you stop recovery between the two WAL records, the page's VM bit in the VM map will be set, but the dead tuples are still on the page. This bug was introduced in Feb 2013, by commit fdf9e21196a6f58c6021c967dc5776a16190f295, so it's present in 9.3 and master. The fix seems quite straightforward, we just have to change the order of those two records. I'll go commit that. With a lot of help from Andres on IRC (thanks a lot man), I think (he thinks actually, =P ) I may ran into the issue this bug can cause. I'm sending this e-mail to (1) confirm if my issue is really caused by that bug and if that is the case, (2) let you guys know the problems it can cause. Scenario: Master: 9.3.1 (I know, trying to go to 9.3.2) Slave: 9.3.2 The slave was running with hot_standby=off (because of other bug Andres is working on), but it stopped working with the following log lines: 2014-01-07 12:38:04 BRST [22668]: [7-1] user=,db= WARNING: page 1076 of relation base/883916/151040222 is uninitialized 2014-01-07 12:38:04 BRST [22668]: [8-1] user=,db= CONTEXT: xlog redo visible: rel 1663/883916/151040222; blk 1076 2014-01-07 12:38:04 BRST [22668]: [9-1] user=,db= PANIC: WAL contains references to invalid pages 2014-01-07 12:38:04 BRST [22668]: [10-1] user=,db= CONTEXT: xlog redo visible: rel 1663/883916/151040222; blk 1076 2014-01-07 12:38:04 BRST [22665]: [3-1] user=,db= LOG: startup process (PID 22668) was terminated by signal 6: Aborted (Complete log at https://pgsql.privatepaste.com/343f3190fe). I used pg_xlogdump to (try to) find the block the error relates to: $ pg_xlogdump pg_xlog/000102BC002B 000102BC007F | grep -n -C5 '883916/151040222; blk 1076' 2088220 ... Heap2 ... 24/ 192, ... lsn: 2BC/46AB8B20 ... desc: clean: rel 1663/883916/151040222; blk 1073 remxid 107409880 2088221 ... Heap2 ... 20/52, ... lsn: 2BC/46AB8BE0 ... desc: visible: rel 1663/883916/151040222; blk 1074 2088222 ... Heap2 ... 24/ 128, ... lsn: 2BC/46AB8C18 ... desc: clean: rel 1663/883916/151040222; blk 1074 remxid 107409880 2088223 ... Heap2 ... 20/52, ... lsn: 2BC/46AB8C98 ... desc: visible: rel 1663/883916/151040222; blk 1075 2088224 ... Heap2 ... 32/64, ... lsn: 2BC/46AB8CD0 ... desc: clean: rel 1663/883916/151040222; blk 1075 remxid 107409880 == two lines that matched bellow == 2088225 ... Heap2 ... 20/52, ... lsn: 2BC/46AB8D10 ... desc: visible: rel 1663/883916/151040222; blk 1076 2088226 ... Heap2 ... 24/ 164, ... lsn: 2BC/46AB8D48 ... desc: clean: rel 1663/883916/151040222; blk 1076 remxid 107409880 2088227 ... Heap2 ... 20/52, ... lsn: 2BC/46AB8DF0 ... desc: visible: rel 1663/883916/151040222; blk 1077 2088228 ... Heap2 ... 24/ 128, ... lsn: 2BC/46AB8E28 ... desc: clean: rel 1663/883916/151040222; blk 1077 remxid 107409880 2088229 ... Heap2 ... 20/52, ... lsn: 2BC/46AB8EA8 ... desc: visible: rel 1663/883916/151040222; blk 1078 2088230 ... Heap2 ... 24/ 5748, ... lsn: 2BC/46AB8EE0 ... desc: clean: rel 1663/883916/151040222; blk 1078 remxid 107409880 2088231 ... Heap2 ... 20/52, ... lsn: 2BC/46ABA570 ... desc: visible: rel 1663/883916/151040222; blk 1079 I cropped some columns to make it easy to read (the entire result is attached), if you guys need more information, I can send the xlogdump of all the WALs (or any other information). Also attached the controldata, if needed. Thanks a lot. Best regards, -- Matheus de Oliveira Analista de Banco de Dados Dextra Sistemas - MPS.Br nível F! www.dextra.com.br/postgres 2088220-rmgr: Heap2 len (rec/tot): 24/ 192, tx: 0, lsn: 2BC/46AB8B20, prev 2BC/46AB8AE8, bkp: 1000, desc: clean: rel 1663/883916/151040222; blk 1073 remxid 107409880 2088221-rmgr: Heap2 len (rec/tot): 20/52, tx: 0, lsn: 2BC/46AB8BE0, prev 2BC/46AB8B20, bkp: , desc: visible: rel 1663/883916/151040222; blk 1074 2088222-rmgr: Heap2 len (rec/tot): 24/ 128, tx: 0, lsn: 2BC/46AB8C18, prev 2BC/46AB8BE0, bkp: 1000, desc: clean: rel 1663/883916/151040222; blk 1074 remxid 107409880 2088223-rmgr: Heap2 len (rec/tot): 20/52, tx: 0, lsn: 2BC/46AB8C98, prev 2BC/46AB8C18, bkp: , desc: visible: rel 1663/883916/151040222; blk 1075 2088224-rmgr: Heap2 len (rec/tot): 32/64, tx: 0, lsn: 2BC
Re: [HACKERS] Bug in visibility map WAL-logging
On Tue, Jan 7, 2014 at 5:36 PM, Heikki Linnakangas hlinnakan...@vmware.comwrote: On 01/07/2014 07:15 PM, Matheus de Oliveira wrote: Hi folks, On Fri, Dec 13, 2013 at 9:47 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: lazy_vacuum_page() does this: 1. START_CRIT_SECTION() 2. Remove dead tuples from the page, marking the itemid's unused. 3. MarkBufferDirty 4. if all remaining tuples on the page are visible to everyone, set the all-visible flag on the page, and call visibilitymap_set() to set the VM bit. 5 visibilitymap_set() writes a WAL record about setting the bit in the visibility map. 6. write the WAL record of removing the dead tuples. 7. END_CRIT_SECTION(). See the problem? Setting the VM bit is WAL-logged first, before the removal of the tuples. If you stop recovery between the two WAL records, the page's VM bit in the VM map will be set, but the dead tuples are still on the page. This bug was introduced in Feb 2013, by commit fdf9e21196a6f58c6021c967dc5776a16190f295, so it's present in 9.3 and master. The fix seems quite straightforward, we just have to change the order of those two records. I'll go commit that. With a lot of help from Andres on IRC (thanks a lot man), I think (he thinks actually, =P ) I may ran into the issue this bug can cause. I'm sending this e-mail to (1) confirm if my issue is really caused by that bug and if that is the case, (2) let you guys know the problems it can cause. Scenario: Master: 9.3.1 (I know, trying to go to 9.3.2) Slave: 9.3.2 The slave was running with hot_standby=off (because of other bug Andres is working on), but it stopped working with the following log lines: 2014-01-07 12:38:04 BRST [22668]: [7-1] user=,db= WARNING: page 1076 of relation base/883916/151040222 is uninitialized 2014-01-07 12:38:04 BRST [22668]: [8-1] user=,db= CONTEXT: xlog redo visible: rel 1663/883916/151040222; blk 1076 2014-01-07 12:38:04 BRST [22668]: [9-1] user=,db= PANIC: WAL contains references to invalid pages 2014-01-07 12:38:04 BRST [22668]: [10-1] user=,db= CONTEXT: xlog redo visible: rel 1663/883916/151040222; blk 1076 2014-01-07 12:38:04 BRST [22665]: [3-1] user=,db= LOG: startup process (PID 22668) was terminated by signal 6: Aborted (Complete log at https://pgsql.privatepaste.com/343f3190fe). I used pg_xlogdump to (try to) find the block the error relates to: $ pg_xlogdump pg_xlog/000102BC002B 000102BC007F | grep -n -C5 '883916/151040222; blk 1076' 2088220 ... Heap2 ... 24/ 192, ... lsn: 2BC/46AB8B20 ... desc: clean: rel 1663/883916/151040222; blk 1073 remxid 107409880 2088221 ... Heap2 ... 20/52, ... lsn: 2BC/46AB8BE0 ... desc: visible: rel 1663/883916/151040222; blk 1074 2088222 ... Heap2 ... 24/ 128, ... lsn: 2BC/46AB8C18 ... desc: clean: rel 1663/883916/151040222; blk 1074 remxid 107409880 ... Hmm. The xlogdump indeed shows that the order of 'clean' and 'visible' is incorrect, but I don't immediately see how that could cause the PANIC. Well... I also didn't understand if it could cause the PANIC. If I got right, it could only cause a visibility map bit with wrong value (e.g. first change the bit but fails to mark the tuple, in case of a failure in between - which seems that not happened). Is that right? Why is the page uninitialized in the standby? If VACUUM is removing some dead tuples from it, it certainly should exist and be correctly initialized. That I don't know for sure... How did you set up the standby? Did you initialize it from an offline backup of the master's data directory, perhaps? The log shows that the startup took the the crash recovery first, then start archive recovery path, because there was no backup label file. In that mode, the standby assumes that the system is consistent after replaying all the WAL in pg_xlog, which is correct if you initialize from an offline backup or atomic filesystem snapshot, for example. But WAL contains references to invalid pages could also be a symptom of an inconsistent base backup, cause by incorrect backup procedure. In particular, I have to ask because I've seen it before: you didn't delete backup_label from the backup, did you? Well, I cannot answer this right now, but makes all sense and is possible. I used a script created by someone else that does pg_start_backup + rsync + pg_stop_backup, but in fact I didn't look at this script to see if it is doing something nasty, as removing backup_label. I'll be able to check it tomorrow and so I'll come back to give a definitive answer. @andres, if it is really removing backup_label it could also cause that other issue we saw on Monday, right? (yes I did run the same script). BTW, is there any situation that backup_label should be removed? I see no reason for doing this, and also have not yet saw someone doing it, so I didn't even thought that could be it. Thank you guys for your attention. Best regards
