Re: [HACKERS] CREATE POLICY and RETURNING
On 10/17/2014 02:49 AM, Robert Haas wrote: > I think you could probably make the DELETE policy control what can get > deleted, but then have the SELECT policy further filter what gets > returned. That seems like the worst of both worlds to me. Suddenly DELETE ... RETURNING might delete more rows than it reports a resultset for. As well as being potentially dangerous for people using it in wCTEs, etc, to me that's the most astonishing possible outcome of all. I'd be much happier with even: ERROR: RETURNING not permitted with SELECT row-security policy than this. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes
Hello, Please find the updated patch attached. >1) I don't think it's a good idea to put the full page write compression into struct XLogRecord. 1. The compressed blocks is of varlena type. Hence, VARATT_IS_COMPRESSED can be used to detect if the datum is compressed. But, it can give false positive when blocks are not compressed because uncompressed blocks in WAL record are not of type varlena. If I understand correctly, VARATT_IS_COMPRESSED looks for particular bit pattern in the datum which when found it returns true irrespective of type of datum. 2. BkpBlock header of the first block in a WAL record can be copied as it is followed by compressed data including block corresponding to first header and remaining headers and blocks. This header can then be used to store flag indicating if the blocks are compressed or not. This seems to be a feasible option but will increase few bytes equivalent to sizeof(BkpBlock) in record when compared to the method of compressing all blocks and headers. Also , the full page write compression currently stored in WAL record occupies 1 byte of padding hence does not increase the overall size. But at the same timecompression attribute is related to backup up blocks hence it makes more sense to have it in BkpBlock header. Although, the attached patch does not include this yet as it will be better to get consensus first. Thoughts? >2) You've essentially removed a lot of checks about the validity of bkp blocks in xlogreader. I don't think that's acceptable Check to see if size of compressed blocks agrees with the total size stored on WAL record header is added in the patch attached. This serves as a check to validate length of record. >3) You have both FullPageWritesStr() and full_page_writes_str(). This has not changed for now reason being full_page_writes_str() is true/false version of FullPageWritesStr macro. It is implemented for backward compatibility with pg_xlogdump. >4)I don't like FullPageWritesIsNeeded(). For one it, at least to me, >7) Unless I miss something CompressBackupBlock should be plural, right? ATM it compresses all the blocks? >8) I don't tests like "if (fpw <= FULL_PAGE_WRITES_COMPRESS)". That relies on the, less than intuitive, ordering of FULL_PAGE_WRITES_COMPRESS (=1) before FULL_PAGE_WRITES_ON (=2). >9) I think you've broken the case where we first think 1 block needs to be backed up, and another doesn't. If we then detect, after the START_CRIT_SECTION(), that we need to "goto begin;" orig_len will still have it's old content. I have corrected these in the patch attached. >5) CompressBackupBlockPagesAlloc is declared static but not defined as such. Have made it global now in order to be able to access it from PostgresMain. >6) You call CompressBackupBlockPagesAlloc() from two places. Neither is IIRC within a critical section. So you imo should remove the outOfMem handling and revert to palloc() instead of using malloc directly. This has not been changed in the current patch reason being outOfMem handling is done in order to proceed without compression of FPW in case sufficient memory is not available for compression. >One > thing worthy of note is that I don't think you currently can > "legally" check fullPageWrites == FULL_PAGE_WRITES_ON when calling it > only during startup as fullPageWrites can be changed at runtime In the attached patch, this check is also added in PostgresMain on SIGHUP after processing postgresql.conf file. Thank you, Rahila Syed On Mon, Sep 29, 2014 at 6:06 PM, Andres Freund wrote: > Hi, > > On 2014-09-22 10:39:32 +, Syed, Rahila wrote: > > >Please find attached the patch to compress FPW. > > I've given this a quick look and noticed some things: > 1) I don't think it's a good idea to put the full page write compression >into struct XLogRecord. > > 2) You've essentially removed a lot of checks about the validity of bkp >blocks in xlogreader. I don't think that's acceptable. > > 3) You have both FullPageWritesStr() and full_page_writes_str(). > > 4) I don't like FullPageWritesIsNeeded(). For one it, at least to me, >sounds grammatically wrong. More importantly when reading it I'm >thinking of it being about the LSN check. How about instead directly >checking whatever != FULL_PAGE_WRITES_OFF? > > 5) CompressBackupBlockPagesAlloc is declared static but not defined as >such. > > 6) You call CompressBackupBlockPagesAlloc() from two places. Neither is >IIRC within a critical section. So you imo should remove the outOfMem >handling and revert to palloc() instead of using malloc directly. One >thing worthy of note is that I don't think you currently can >"legally" check fullPageWrites == FULL_PAGE_WRITES_ON when calling it >only during startup as fullPageWrites can be changed at runtime. > > 7) Unless I miss something CompressBackupBlock should be plural, right? >ATM it compresses all the blocks? > > 8) I don't tests like "
Re: [HACKERS] Trailing comma support in SELECT statements
> > > > >> We might as well allow a final trailing (or initial leading) comma on a >> values list at the same time: >> >> VALUES >> (...), >> (...), >> (...), >> > > > do you know, so this feature is a proprietary and it is not based on > ANSI/SQL? Any user, that use this feature and will to port to other > database will hate it. > > Regards > > Pavel > > > I've got no complaint if "at the same time" means that neither behavior is ever implemented... David J.
Re: [HACKERS] Superuser connect during smart shutdown
Tom Lane-2 wrote > Jim Nasby < > Jim.Nasby@ > > writes: >> Something else mentioned was that once you start a smart shutdown you >> have no good way (other than limited ps output) to see what the shutdown >> is waiting on. I'd like to have some way to get back into the database >> to see what's going on. Perhaps we could allow superusers to connect >> while waiting for shutdown. > > I think this idea is going to founder on the fact that the postmaster > has no way to tell whether an incoming connection is for a superuser. > You don't find that out until you've connected to a database and run > a transaction (so you can read pg_authid). And by that point, you've > already had a catastrophic impact on any attempt to shut things down. This quote from the documentation seems suspect in light of your comment... "While backup mode is active, new connections will still be allowed, but only to superusers (this exception allows a superuser to connect to terminate online backup mode)." http://www.postgresql.org/docs/9.3/interactive/server-shutdown.html David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/Superuser-connect-during-smart-shutdown-tp5823332p5823367.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Trailing comma support in SELECT statements
2014-10-17 6:34 GMT+02:00 David G Johnston : > Jim Nasby-5 wrote > > On 10/3/14, 4:02 PM, David G Johnston wrote: > >> Should we also allow: > >> > >> SELECT > >> , col1 > >> , col2 > >> , col3 > >> FROM ... > >> > >> ? > > I would say yes, if we're going to do this. I don't see it being any > worse > > than trailing commas. > > > > If we are going to do this, we need to do it EVERYWHERE. > > > > FWIW, the way I normally "work around" this problem is: > > > > SELECT > > blah > > , foo > > , bar > > , baz > > > > In my experience, it's quite uncommon to mess with the first item in the > > list, which mostly eliminates the issue. A missing leading comma is also > > MUCH easier to spot than a missing trailing comma. > do you know, so this feature is a proprietary and it is not based on ANSI/SQL? Any user, that use this feature and will to port to other database will hate it. Regards Pavel > > > > > > -- > > Sent via pgsql-hackers mailing list ( > > > pgsql-hackers@ > > > ) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-hackers > > > Jim Nasby-5 wrote > > On 10/3/14, 4:02 PM, David G Johnston wrote: > >> Should we also allow: > >> > >> SELECT > >> , col1 > >> , col2 > >> , col3 > >> FROM ... > >> > >> ? > > I would say yes, if we're going to do this. I don't see it being any > worse > > than trailing commas. > > > > If we are going to do this, we need to do it EVERYWHERE. > > > > FWIW, the way I normally "work around" this problem is: > > > > SELECT > > blah > > , foo > > , bar > > , baz > > > > In my experience, it's quite uncommon to mess with the first item in the > > list, which mostly eliminates the issue. A missing leading comma is also > > MUCH easier to spot than a missing trailing comma. > > We might as well allow a final trailing (or initial leading) comma on a > values list at the same time: > > VALUES > (...), > (...), > (...), > ; > > > David J. > > > > > -- > View this message in context: > http://postgresql.1045698.n5.nabble.com/Trailing-comma-support-in-SELECT-statements-tp5821613p5823365.html > Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Trailing comma support in SELECT statements
Jim Nasby-5 wrote > On 10/3/14, 4:02 PM, David G Johnston wrote: >> Should we also allow: >> >> SELECT >> , col1 >> , col2 >> , col3 >> FROM ... >> >> ? > I would say yes, if we're going to do this. I don't see it being any worse > than trailing commas. > > If we are going to do this, we need to do it EVERYWHERE. > > FWIW, the way I normally "work around" this problem is: > > SELECT > blah > , foo > , bar > , baz > > In my experience, it's quite uncommon to mess with the first item in the > list, which mostly eliminates the issue. A missing leading comma is also > MUCH easier to spot than a missing trailing comma. > > > -- > Sent via pgsql-hackers mailing list ( > pgsql-hackers@ > ) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers Jim Nasby-5 wrote > On 10/3/14, 4:02 PM, David G Johnston wrote: >> Should we also allow: >> >> SELECT >> , col1 >> , col2 >> , col3 >> FROM ... >> >> ? > I would say yes, if we're going to do this. I don't see it being any worse > than trailing commas. > > If we are going to do this, we need to do it EVERYWHERE. > > FWIW, the way I normally "work around" this problem is: > > SELECT > blah > , foo > , bar > , baz > > In my experience, it's quite uncommon to mess with the first item in the > list, which mostly eliminates the issue. A missing leading comma is also > MUCH easier to spot than a missing trailing comma. We might as well allow a final trailing (or initial leading) comma on a values list at the same time: VALUES (...), (...), (...), ; David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/Trailing-comma-support-in-SELECT-statements-tp5821613p5823365.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Superuser connect during smart shutdown
* Tom Lane (t...@sss.pgh.pa.us) wrote: > But TBH I suspect 95% of the problems here would vanish if smart > shutdown weren't the default ... +1000 ... Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Better support of exported snapshots with pg_dump
On Fri, Oct 17, 2014 at 12:19 AM, Robert Haas wrote:
> On Wed, Oct 15, 2014 at 1:06 AM, Andres Freund
> wrote:
> > I personally think we should just disregard the race here and add a
> > snapshot parameter. The race is already there and not exactly
> > small. Let's not kid ourselves that hiding it solves anything.
> I, too, favor that plan.
>
Two votes in favor of that from two committers sounds like a deal. Here is
an refreshed version of the patch introducing --snapshot from here, after
fixing a couple of things and adding documentation:
http://www.postgresql.org/message-id/ca+u5nmk9+ttcff_-4mfdxwhnastauhuq7u7uedd57vay28a...@mail.gmail.com
When the snapshot specified by user is not a valid one, here is the error
returned by pg_dump:
$ pg_dump --snapshot 'ppo'
pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier:
"ppo"
pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT 'ppo'
I thinks that's fine, and it makes the code lighter to rely on the existing
error machinery.
Regards,
--
Michael
From 6f0d5370d152fc7979632a0abc1dee1ac58f8b30 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 17 Oct 2014 13:21:32 +0900
Subject: [PATCH] Add option --snapshot to pg_dump
This can be used to define a snapshot previously defined by a session
in parallel that has either used pg_export_snapshot or obtained one when
creating a logical slot. When this option is used with parallel pg_dump,
the snapshot defined by this option is used and no new snapshot is taken.
---
doc/src/sgml/ref/pg_dump.sgml | 20 +
src/bin/pg_dump/pg_dump.c | 52 +--
2 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index c92c6ee..f3ab71a 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -848,6 +848,26 @@ PostgreSQL documentation
+ --snapshot=snapshotname
+
+
+ Use given synchronous snapshot when taking a dump of the database (see
+ for more
+ details).
+
+
+ This option is useful when needing a dump consistent with a session
+ in parallel that exported this snapshot, when for example creating
+ a logical replication slot (see ).
+
+
+ In the case of a parallel dump, the snapshot name defined by this
+ option is used in priority.
+
+
+
+
+
--serializable-deferrable
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c56a4cb..2d3c7dd 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -126,7 +126,8 @@ static const CatalogId nilCatalogId = {0, 0};
static void help(const char *progname);
static void setup_connection(Archive *AH, DumpOptions *dopt,
- const char *dumpencoding, char *use_role);
+const char *dumpencoding, const char *dumpsnapshot,
+char *use_role);
static ArchiveFormat parseArchiveFormat(const char *format, ArchiveMode *mode);
static void expand_schema_name_patterns(Archive *fout,
SimpleStringList *patterns,
@@ -269,6 +270,7 @@ main(int argc, char **argv)
RestoreOptions *ropt;
Archive*fout; /* the script file */
const char *dumpencoding = NULL;
+ const char *dumpsnapshot = NULL;
char *use_role = NULL;
int numWorkers = 1;
trivalue prompt_password = TRI_DEFAULT;
@@ -329,6 +331,7 @@ main(int argc, char **argv)
{"role", required_argument, NULL, 3},
{"section", required_argument, NULL, 5},
{"serializable-deferrable", no_argument, &dopt->serializable_deferrable, 1},
+ {"snapshot", required_argument, NULL, 6},
{"use-set-session-authorization", no_argument, &dopt->use_setsessauth, 1},
{"no-security-labels", no_argument, &dopt->no_security_labels, 1},
{"no-synchronized-snapshots", no_argument, &dopt->no_synchronized_snapshots, 1},
@@ -506,6 +509,10 @@ main(int argc, char **argv)
set_dump_section(optarg, &dopt->dumpSections);
break;
+ case 6:/* snapshot */
+dumpsnapshot = pg_strdup(optarg);
+break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit_nicely(1);
@@ -614,7 +621,7 @@ main(int argc, char **argv)
* death.
*/
ConnectDatabase(fout, dopt->dbname, dopt->pghost, dopt->pgport, dopt->username, prompt_password);
- setup_connection(fout, dopt, dumpencoding, use_role);
+ setup_connection(fout, dopt, dumpencoding, dumpsnapshot, use_role);
/*
* Disable security label support if server version < v9.1.x (prevents
@@ -658,6 +665,11 @@ main(int argc, char **argv)
"Run with --no-synchronized-snapshots instead if you do not need\n"
"synchronized snapshots.\n");
+ /* check the version when a snapshot is explicitely specified by user */
+ if (dumpsnapshot && fout->remoteVersion < 90200)
+ exit_horribly(NULL,
+ "Exported snapshots are not s
Re: [HACKERS] Superuser connect during smart shutdown
Jim Nasby writes: > Something else mentioned was that once you start a smart shutdown you > have no good way (other than limited ps output) to see what the shutdown > is waiting on. I'd like to have some way to get back into the database > to see what's going on. Perhaps we could allow superusers to connect > while waiting for shutdown. I think this idea is going to founder on the fact that the postmaster has no way to tell whether an incoming connection is for a superuser. You don't find that out until you've connected to a database and run a transaction (so you can read pg_authid). And by that point, you've already had a catastrophic impact on any attempt to shut things down. > It would also be good to be able to abort a smart shutdown if you > determine it was a bad idea. That sounds possibly more feasible. But TBH I suspect 95% of the problems here would vanish if smart shutdown weren't the default ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] detect custom-format dumps in psql and emit a useful error
On 09/18/2014 05:58 PM, Andres Freund wrote:
> I don't think we need to make any discinction between psql -f mydb.dump,
> psql < mydb.dump, and whatever | psql. Just check, when noninteractively
> reading the first line in mainloop.c:MainLoop(), whether it starts with
> the magic header. That'd also trigger the warning on \i pg_restore_file,
> but that's hardly a problem.
Done, patch attached.
If psql sees that the first line begins with PGDMP it'll emit:
The input is a PostgreSQL custom-format dump. Use the pg_restore
command-line client to restore this dump to a database.
then discard the rest of the current input source.
>> pg_restore already knows to tell you to use psql if it sees an SQL file
>> as input. Having something similar for pg_dump would be really useful.
>
> Agreed.
>
> We could additionally write out a hint whenever a directory is fed to
> psql -f that psql can't be used to read directory type dumps.
Unlike the confusion between pg_restore and psql for custom file format
dumps I haven't seen people getting this one muddled. Perhaps directory
format dumps are just a bit more niche, or perhaps it's just more
obvious that:
psql:sometump:0: could not read from input file: Is a directory
... means psql is the wrong tool.
Still, separate patch attached. psql will now emit:
psql:blah:0: Input path is a directory. Use pg_restore to restore
directory-format database dumps.
I'm less sure that this is a worthwhile improvement than the check for
PGDMP and custom format dumps though.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From e3a6633ec2782264f3a87fbe3be079f94d89b911 Mon Sep 17 00:00:00 2001
From: Craig Ringer
Date: Fri, 17 Oct 2014 12:07:37 +0800
Subject: [PATCH 2/2] If the input path to psql is a directory, mention
pg_restore in the error
This should help users who try to use psql to restore a directory-format
dump from pg_dump.
---
src/bin/psql/input.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 6416ab9..e332318 100644
--- a/src/bin/psql/input.c
+++ b/src/bin/psql/input.c
@@ -191,8 +191,15 @@ gets_fromFile(FILE *source)
{
if (ferror(source))
{
-psql_error("could not read from input file: %s\n",
- strerror(errno));
+/*
+ * Could the user be trying to restore a directory
+ * format dump?
+ */
+if (errno == EISDIR)
+ psql_error("Input path is a directory. Use pg_restore to restore directory-format database dumps.\n");
+else
+ psql_error("could not read from input file: %s\n",
+ strerror(errno));
return NULL;
}
break;
--
1.9.3
>From 5330eea78029f9cb689fd3c53722cb02217f47df Mon Sep 17 00:00:00 2001
From: Craig Ringer
Date: Fri, 17 Oct 2014 11:54:29 +0800
Subject: [PATCH 1/2] Emit an error when psql is used to load a custom-format
dump file
Users tend to get confused between psql and pg_restore, and will
use psql to try to restore a dump from pg_dump -Fc, or use pg_restore
to try to restore an SQL format dump.
pg_restore complains if it sees an SQL format dump, but psql doesn't
complain if it sees a custom-format dump.
Fix that by emitting an error if you try to run a custom format dump:
The input is a PostgreSQL custom-format dump. Use the pg_restore
command-line client to restore this dump to a database.
---
src/bin/psql/mainloop.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index 98211dc..1e057a6 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -175,6 +175,17 @@ MainLoop(FILE *source)
if (pset.lineno == 1 && pset.encoding == PG_UTF8 && strncmp(line, "\xef\xbb\xbf", 3) == 0)
memmove(line, line + 3, strlen(line + 3) + 1);
+ /* Detect attempts to run custom-format dumps as SQL scripts */
+ if (pset.lineno == 1 && !pset.cur_cmd_interactive && strncmp(line, "PGDMP", 5) == 0)
+ {
+ free(line);
+ puts(_("The input is a PostgreSQL custom-format dump. Use the pg_restore \n"
+ "command-line client to restore this dump to a database.\n"));
+ fflush(stdout);
+ successResult = EXIT_FAILURE;
+ break;
+ }
+
/* nothing left on line? then ignore */
if (line[0] == '\0' && !psql_scan_in_quote(scan_state))
{
--
1.9.3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Segmentation fault] pg_dump binary-upgrade fail for type without element
On Thu, Oct 16, 2014 at 11:16 AM, Rushabh Lathia wrote: > > PFA patch patch for the master branch. I think you should upload this patch to CF to avoid getting it lost. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
[HACKERS] [PATCH] HINT: pg_hba.conf changed since last config reload
On 08/10/2014 07:48 PM, Craig Ringer wrote:
> Hi all
>
> I just had an idea I wanted to run by you all before turning it into a
> patch.
>
> People seem to get confused when they get auth errors because they
> changed pg_hba.conf but didn't reload.
>
> Should we emit a HINT alongside the main auth error in that case?
>
> Given the amount of confusion that I see around pg_hba.conf from new
> users, I figure anything that makes it less confusing might be a good
> thing if there aren't other consequences.
>
> Interested in a patch?
Given the generally positive reception to this, here's a patch.
The first patch adds an errhint_log , akin to the current errdetail_log,
so we can send a different HINT to the server log than we do to the client.
(Even if DETAIL was appropriate for this info, which it isn't, I can't
use errdetail_log because it's already used for other information in
some of the same error sites.)
The second patch adds a test during errors to report if pg_hba.conf is
stale, or if pg_ident.conf is stale.
Typical output, client:
psql: FATAL: Peer authentication failed for user "fred"
HINT: See the server error log for additional information.
Typical output, server:
LOG: provided user name (fred) and authenticated user name (craig) do
not match
FATAL: Peer authentication failed for user "fred"
DETAIL: Connection matched pg_hba.conf line 84: "local all
all peer"
HINT: pg_hba.conf has been changed since last server configuration
reload. Reload the server configuration to apply the changes.
I've added this to the next CF.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 8b2bf6ae615d716ca9857017fd862386c6bdcd1f Mon Sep 17 00:00:00 2001
From: Craig Ringer
Date: Fri, 17 Oct 2014 11:18:18 +0800
Subject: [PATCH 1/2] Add an errhint_log, akin to errdetail_log
This allows a different HINT to be sent to the server error log
and to the client, which will be useful where there's security
sensitive information that's more appropriate for a HINT than
a DETAIL message.
---
src/backend/utils/error/elog.c | 54 --
src/include/utils/elog.h | 7 ++
2 files changed, 48 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 32a9663..59be8a6 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1015,6 +1015,26 @@ errhint(const char *fmt,...)
return 0; /* return value does not matter */
}
+/*
+ * errhint_log --- add a hint_log error message text to the current error
+ */
+int
+errhint_log(const char *fmt,...)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+ MemoryContext oldcontext;
+
+ recursion_depth++;
+ CHECK_STACK_DEPTH();
+ oldcontext = MemoryContextSwitchTo(edata->assoc_context);
+
+ EVALUATE_MESSAGE(edata->domain, hint_log, false, true);
+
+ MemoryContextSwitchTo(oldcontext);
+ recursion_depth--;
+ return 0; /* return value does not matter */
+}
+
/*
* errcontext_msg --- add a context error message text to the current error
@@ -1498,6 +1518,8 @@ CopyErrorData(void)
newedata->detail_log = pstrdup(newedata->detail_log);
if (newedata->hint)
newedata->hint = pstrdup(newedata->hint);
+ if (newedata->hint_log)
+ newedata->hint_log = pstrdup(newedata->hint_log);
if (newedata->context)
newedata->context = pstrdup(newedata->context);
if (newedata->schema_name)
@@ -1536,6 +1558,8 @@ FreeErrorData(ErrorData *edata)
pfree(edata->detail_log);
if (edata->hint)
pfree(edata->hint);
+ if (edata->hint_log)
+ pfree(edata->hint_log);
if (edata->context)
pfree(edata->context);
if (edata->schema_name)
@@ -1618,6 +1642,8 @@ ReThrowError(ErrorData *edata)
newedata->detail_log = pstrdup(newedata->detail_log);
if (newedata->hint)
newedata->hint = pstrdup(newedata->hint);
+ if (newedata->hint_log)
+ newedata->hint_log = pstrdup(newedata->hint_log);
if (newedata->context)
newedata->context = pstrdup(newedata->context);
if (newedata->schema_name)
@@ -2659,8 +2685,11 @@ write_csvlog(ErrorData *edata)
appendCSVLiteral(&buf, edata->detail);
appendStringInfoChar(&buf, ',');
- /* errhint */
- appendCSVLiteral(&buf, edata->hint);
+ /* errhint or errhint_log */
+ if (edata->hint_log)
+ appendCSVLiteral(&buf, edata->hint_log);
+ else
+ appendCSVLiteral(&buf, edata->hint);
appendStringInfoChar(&buf, ',');
/* internal query */
@@ -2777,25 +2806,22 @@ send_message_to_server_log(ErrorData *edata)
if (Log_error_verbosity >= PGERROR_DEFAULT)
{
- if (edata->detail_log)
- {
- log_line_prefix(&buf, edata);
- appendStringInfoString(&buf, _("DETAIL: "));
- append_with_tabs(&buf, edata->detail_log);
- appendStringInfoChar(&buf, '\n');
- }
- else if (edata->detail)
+ const char * const detail
+ = edata->detail_log != NULL ? edata->detail_log : edata->detail;
+ cons
Re: [HACKERS] Superuser connect during smart shutdown
On 10/17/2014 03:59 AM, Jim Nasby wrote: > Over in the "Log notice that checkpoint is to be written on shutdown" > thread... > > On 10/16/14, 2:31 PM, Michael Banck wrote: >> There were some comments that this might not actually be the case and/or >> that the postmaster was simply waiting for clients to disconnect due to >> smart shutdown being invoked. > > Something else mentioned was that once you start a smart shutdown you > have no good way (other than limited ps output) to see what the shutdown > is waiting on. I'd like to have some way to get back into the database > to see what's going on. Perhaps we could allow superusers to connect > while waiting for shutdown. A big warning that we're in shutdown would > be nice, and maybe it would make sense to further restrict this to only > local connections. You'd also want to flag this connection so it's ignored by the smart shutdown check, allowing the server to shut down even if it's active. That'd be a pretty useful thing to have anyway, so monitoring tools, long-running reports that can be restarted ,etc can mark their connections as ignored for the purpose of smart shutdown. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: dynahash replacement for buffer table
On Thu, Oct 16, 2014 at 6:53 PM, Andres Freund wrote: > When using shared_buffers = 96GB there's a performance benefit, but not > huge: > master (f630b0dd5ea2de52972d456f5978a012436115e): 153621.8 > master + LW_SHARED + lockless StrategyGetBuffer(): 477118.4 > master + LW_SHARED + lockless StrategyGetBuffer() + chash: 496788.6 > master + LW_SHARED + lockless StrategyGetBuffer() + chash-nomb: 499562.7 > > But with shared_buffers = 16GB: > master (f630b0dd5ea2de52972d456f5978a012436115e): 177302.9 > master + LW_SHARED + lockless StrategyGetBuffer(): 206172.4 > master + LW_SHARED + lockless StrategyGetBuffer() + chash: 413344.1 > master + LW_SHARED + lockless StrategyGetBuffer() + chash-nomb: 426405.8 Very interesting. This doesn't show that chash is the right solution, but it definitely shows that doing nothing is the wrong solution. It shows that, even with the recent bump to 128 lock manager partitions, and LW_SHARED on top of that, workloads that actually update the buffer mapping table still produce a lot of contention there. This hasn't been obvious to me from profiling, but the numbers above make it pretty clear. It also seems to suggest that trying to get rid of the memory barriers isn't a very useful optimization project. We might get a couple of percent out of it, but it's pretty small potatoes, so unless it can be done more easily than I suspect, it's probably not worth bothering with. An approach I think might have more promise is to have bufmgr.c call the CHash stuff directly instead of going through buf_table.c. Right now, for example, BufferAlloc() creates and initializes a BufferTag and passes a pointer to that buffer tag to BufTableLookup, which copies it into a BufferLookupEnt. But it would be just as easy for BufferAlloc() to put the BufferLookupEnt on its own stack, and then you wouldn't need to copy the data an extra time. Now a 20-byte copy isn't a lot, but it's completely unnecessary and looks easy to get rid of. > I had to play with setting max_connections+1 sometimes to get halfway > comparable results for master - unaligned data otherwise causes wierd > results otherwise. Without doing that the performance gap between master > 96/16G was even bigger. We really need to fix that... > > This is pretty awesome. Thanks. I wasn't quite sure how to test this or where the workloads that it would benefit would be found, so I appreciate you putting time into it. And I'm really glad to hear that it delivers good results. I think it would be useful to plumb the chash statistics into the stats collector or at least a debugging dump of some kind for testing. They include a number of useful contention measures, and I'd be interested to see what those look like on this workload. (If we're really desperate for every last ounce of performance, we could also disable those statistics in production builds. That's probably worth testing at least once to see if it matters much, but I kind of hope it doesn't.) -- 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
Re: [HACKERS] WIP: dynahash replacement for buffer table
Hi, On 2014-10-14 09:30:58 -0400, Robert Haas wrote: > A few years ago I started working on a concurrent hash table for > PostgreSQL. The hash table part of it worked, but I never did > anything with it, really. Amit mentioned to me earlier this week that > he was seeing contention inside the dynahash machinery, which inspired > me to go back and update the patch. I took the basic infrastructure > from before and used it to replace the buffer table. Patch is > attached. So. I ran a quick tests on a larger x86 machine. 4xE5-4620, 256GB RAM. The hotel wifi is too flaky to get detailed results, but some tasty bits. The test I used is readonly pgbench on a scale 5000 DB - about 73GB. I'm benchmarking chash ontop my LW_SHARED and StrategyGetBuffer() optimizations because otherwise the bottleneck is completely elsewhere. When using shared_buffers = 96GB there's a performance benefit, but not huge: master (f630b0dd5ea2de52972d456f5978a012436115e): 153621.8 master + LW_SHARED + lockless StrategyGetBuffer(): 477118.4 master + LW_SHARED + lockless StrategyGetBuffer() + chash: 496788.6 master + LW_SHARED + lockless StrategyGetBuffer() + chash-nomb: 499562.7 But with shared_buffers = 16GB: master (f630b0dd5ea2de52972d456f5978a012436115e): 177302.9 master + LW_SHARED + lockless StrategyGetBuffer(): 206172.4 master + LW_SHARED + lockless StrategyGetBuffer() + chash: 413344.1 master + LW_SHARED + lockless StrategyGetBuffer() + chash-nomb: 426405.8 All the numbers here -P5 output when it looks like it has stabilized - it takes a couple minutes to get to that point so pgbench runs would have to be really long to not be skewed by the startup. I don't think my manual selection of measurements matters much at this scale of differences ;) I had to play with setting max_connections+1 sometimes to get halfway comparable results for master - unaligned data otherwise causes wierd results otherwise. Without doing that the performance gap between master 96/16G was even bigger. We really need to fix that... This is pretty awesome. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
Thanks for the review. On 16 October 2014 23:23, MauMau wrote: > (3) > SELECT against a view generated two audit log lines, one for the view > itself, and the other for the underlying table. Is this intended? I'm not > saying that's wrong but just asking. Intended > (4) > I'm afraid audit-logging DML statements on temporary tables will annoy > users, because temporary tables are not interesting. Agreed > (5) > This is related to (4). As somebody mentioned, I think the ability to > select target objects of audit logging is definitely necessary. Without > that, huge amount of audit logs would be generated for uninteresting > objects. That would also impact performance. Discussed and agreed already > (6) > What's the performance impact of audit logging? I bet many users will ask > "about what percentage would the throughtput decrease by?" I'd like to know > the concrete example, say, pgbench and DBT-2. Don't know, but its not hugely relevant. If you need it, you need it. > (8) > The code looks good. However, I'm worried about the maintenance. How can > developers notice that pgaudit.c needs modification when they add a new SQL > statement? What keyword can they use to grep the source code to find > pgaudit.c? Suggestions welcome. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
Hello, I had a quick look through the code and did some testing. Let me give you some comments. I will proceed with checking if pgaudit can meet PCI DSS requirements. By the way, I'd like to use pgaudit with 9.2. Is it possible with a slight modification of the code? If it is, what features of pgaudit would be unavailable? Could you support 9.2? (1) The build failed with PostgreSQL 9.5, although I know the README mentions that pgaudit supports 9.3 and 9.4. The cause is T_AlterTableSpaceMoveStmt macro is not present in 9.5. I could build and use pgaudit by removing two lines referring to that macro. I tested pgaudit only with 9.5. (2) I could confirm that DECLARE is audit-logged as SELECT and FETCH/CLOSE are not. This is just as expected. Nice. (3) SELECT against a view generated two audit log lines, one for the view itself, and the other for the underlying table. Is this intended? I'm not saying that's wrong but just asking. (4) I'm afraid audit-logging DML statements on temporary tables will annoy users, because temporary tables are not interesting. In addition, in applications which use the same temporary table in multiple types of transactions as follows, audit log entries for the DDL statement are also annoying. BEGIN; CREATE TEMPORARY TABLE mytemp ... ON COMMIT DROP; DML; COMMIT; The workaround is "CREATE TEMPORARY TABLE mytemp IF NOT EXIST ... ON COMMIT DELETE ROWS". However, users probably don't (or can't) modify applications just for audit logging. (5) This is related to (4). As somebody mentioned, I think the ability to select target objects of audit logging is definitely necessary. Without that, huge amount of audit logs would be generated for uninteresting objects. That would also impact performance. (6) What's the performance impact of audit logging? I bet many users will ask "about what percentage would the throughtput decrease by?" I'd like to know the concrete example, say, pgbench and DBT-2. (7) In README, COPY FROM/TO should be added to read and write respectively. (8) The code looks good. However, I'm worried about the maintenance. How can developers notice that pgaudit.c needs modification when they add a new SQL statement? What keyword can they use to grep the source code to find pgaudit.c? Regards MauMau -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Robert Haas wrote: > > On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost wrote: > > > > My feeling is basically this- either we make a clean break to the new > > > syntax and catalog representation, or we just use the same approach the > > > existing attriubtes use. Long term, I think your proposed syntax and an > > > int64 representation is better but it'll mean a lot of client code that > > > has to change. I don't really like the idea of changing the syntax but > > > not the representation, nor am I thrilled with the idea of supporting > > > both syntaxes, and changing the syntax without changing the > > > representation just doesn't make sense to me as I think we'd end up > > > wanting to change it later, making clients have to update their code > > > twice. > > > > I don't see any reason why it has to be both or neither. > > I was thinking we would change the catalogs and implement the new syntax > for new and old settings, but also keep the old syntax working as a > backward compatibility measure. I don't see what's so terrible about > continuing to support the old syntax; we do that in COPY and EXPLAIN, > for example. It just complicates things and I'm not sure there's much benefit to it. Clients are going to need to be updated to support the new attributes anyway, and if the new attributes can only be used through the new syntax, well, I don't know why they'd want to deal with the old syntax too. That said, I don't feel very strongly about that position, so if you and Robert (and others on the thread) agree that's the right approach then I'll see about getting it done. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
* Simon Riggs (si...@2ndquadrant.com) wrote: > On 16 October 2014 20:37, Stephen Frost wrote: > > >> How about > >> > >> GRANT EXECUTE [PRIVILEGES] ON CAPABILITY foo TO bar; > >> > >> That is similar to granting execution privs on a function. And I think > >> gets round the keyword issue? > > > > No, it doesn't.. EXECUTE isn't reserved at all. > > Yet GRANT EXECUTE is already valid syntax, so that should work. Yeah, sorry, the issue with the above is that the "ON CAPABILITY" would mean CAPABILITY needs to be reserved as otherwise we don't know if it's a function or something else. The keyword issue is with GRANT TO ; As could be a role. Not sure offhand if GRANT EXECUTE PRIVILEGES ON CAPABILITY foo TO bar; would work.. In general, I'm not anxious to get involved in the SQL-specified GRANT syntax though unless there's really good reason to. Also, these aren't like normally granted privileges which can have an ADMIN option and which are inheirited through role membership.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Robert Haas wrote: > On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost wrote: > > My feeling is basically this- either we make a clean break to the new > > syntax and catalog representation, or we just use the same approach the > > existing attriubtes use. Long term, I think your proposed syntax and an > > int64 representation is better but it'll mean a lot of client code that > > has to change. I don't really like the idea of changing the syntax but > > not the representation, nor am I thrilled with the idea of supporting > > both syntaxes, and changing the syntax without changing the > > representation just doesn't make sense to me as I think we'd end up > > wanting to change it later, making clients have to update their code > > twice. > > I don't see any reason why it has to be both or neither. I was thinking we would change the catalogs and implement the new syntax for new and old settings, but also keep the old syntax working as a backward compatibility measure. I don't see what's so terrible about continuing to support the old syntax; we do that in COPY and EXPLAIN, for example. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On 16 October 2014 20:37, Stephen Frost wrote: >> How about >> >> GRANT EXECUTE [PRIVILEGES] ON CAPABILITY foo TO bar; >> >> That is similar to granting execution privs on a function. And I think >> gets round the keyword issue? > > No, it doesn't.. EXECUTE isn't reserved at all. Yet GRANT EXECUTE is already valid syntax, so that should work. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost wrote: > > My feeling is basically this- either we make a clean break to the new > > syntax and catalog representation, or we just use the same approach the > > existing attriubtes use. Long term, I think your proposed syntax and an > > int64 representation is better but it'll mean a lot of client code that > > has to change. I don't really like the idea of changing the syntax but > > not the representation, nor am I thrilled with the idea of supporting > > both syntaxes, and changing the syntax without changing the > > representation just doesn't make sense to me as I think we'd end up > > wanting to change it later, making clients have to update their code > > twice. > > I don't see any reason why it has to be both or neither. My reasoning on that is that either breaks existing clients and so they'll have to update and if we're going to force that then we might as well go whole-hog and get it over with once. If my assumption is incorrect and we don't think changes to the catalog representation will break existing clients then I withdraw the argument that they should be done together. For the syntax piece of it, my feeling is that all these role attributes should be handled the same way. There's three ways to address that- support them using the existing syntax, change to a new syntax and only support that, or have two different syntaxes. I don't like the idea that these new attributes will be supported with one syntax but the old attributes would be supported with a different syntax. If we think it's too much to change in one release, I could see changing the catalog representation and then providing the new syntax while supporting the old syntax as deprecated, but we do have a pretty bad history of such changes and any maintained client should be getting updated for the new role attributes anyway.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Thu, Oct 16, 2014 at 3:09 PM, Stephen Frost wrote: >> > * Robert Haas (robertmh...@gmail.com) wrote: >> >> Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER >> >> ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making >> >> CAPABILITY a keyword, but it could be unreserved. >> > >> > That works for me- would we change the existing role attributes to be >> > configurable this way and change everything over to using an int64 in >> > the catalog? Unless I'm having trouble counting, I think that would >> > actually result in the pg_authid catalog not changing in size at all >> > while giving us the ability to add these capabilities and something like >> > 50 others if we had cause to. >> >> I definitely think we should support the new syntax for the existing >> attributes. > > Ok. > >> I could go either way on whether to change the catalog >> storage for the existing attributes. Some people might prefer to >> avoid the backward compatibility break, and I can see that argument. > > There's really two issues when it comes to backwards compatibility here- > the catalog representation and the syntax. > > My feeling is basically this- either we make a clean break to the new > syntax and catalog representation, or we just use the same approach the > existing attriubtes use. Long term, I think your proposed syntax and an > int64 representation is better but it'll mean a lot of client code that > has to change. I don't really like the idea of changing the syntax but > not the representation, nor am I thrilled with the idea of supporting > both syntaxes, and changing the syntax without changing the > representation just doesn't make sense to me as I think we'd end up > wanting to change it later, making clients have to update their code > twice. I don't see any reason why it has to be both or neither. -- 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
Re: [HACKERS] WIP: dynahash replacement for buffer table
On Wed, Oct 15, 2014 at 2:03 AM, Andres Freund wrote: > Yes, I can see that now. I do wonder if that's not going to prove quite > expensive... I think I can see some ways around needing that. But I > think that needs some benchmarking first - no need to build a even more > complex solution if not needed. I did a bit of testing on my MacBook Pro last night. Unfortunately, I don't have access to a large x86 machine the moment.[1] I tried four configurations: (1) master (2) master with chash patch (3) master with chash patch, pg_memory_barrier() changed from lock addl to mfence (4) same as (3), plus barrier at the end of CHashSearch changed to a compiler barrier (which should be safe on x86) I tested pgbench -S, scale factor 100, shared_buffers 400MB. 3 trials per configuration; runs were interleaved. Percentages indicate the average difference between that line and master. At 1 client: (1) 11689.388986 11426.653176 11297.775005 (2) 11618.306822 11331.805396 11273.272945 -0.55% (3) 11813.664402 11300.082928 11231.265030 -0.20% (4) 11428.097384 11266.979165 11294.422376 -1.23% At 16 clients: (1) 56716.465893 56992.590587 56755.298362 (2) 57126.787712 56848.527712 57351.559138 +0.51% (3) 56779.624757 57036.610925 56878.036445 +0.13% (4) 56818.522369 56885.544509 56977.810413 +0.13% I think this tends to confirm that the patch is a small loss (for unknown reasons) at 1 client, but that it picks up speed with more contention, even on a machine with only 4 cores. But if there's an important difference between the different fencing techniques, it doesn't show up in this test. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company [1] Yes, this is a hint. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strip nulls functions for json and jsonb
Hello I checked this patch. 1. There is a consensus we want this feature. 2. This patch implement just this mentioned feature. There is no objection against design. 3. It is possible to apply this patch and compile without warnings. 4. I tested null stripping on large json, jsonb values without problems. 5. regress tests are enough 6. code is well formatted Objections & questions: 1. missing documentation 2. I miss more comments related to this functions. This code is relative simple, but some more explanation can be welcome. 3. why these functions are marked as "stable"? Regards Pavel 2014-10-04 1:23 GMT+02:00 Andrew Dunstan : > As discussed recently, here is an undocumented patch for json_strip_nulls > and jsonb_strip_nulls. > > cheers > > andrew > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
Re: [HACKERS] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]
On 16 October 2014 15:09, Amit Kapila wrote: >> Just send a message to autovacuum to request an immediate action. Let >> it manage the children and the tasks. >> >>SELECT pg_autovacuum_immediate(nworkers = N, list_of_tables); >> >> Request would allocate an additional N workers and immediately begin >> vacuuming the stated tables. > > I think doing anything on the server side can have higher complexity like: > a. Does this function return immediately after sending request to > autovacuum, if yes then the behaviour of this new functionality > will be different as compare to vacuumdb which user might not > expect? > b. We need to have some way to handle errors that can occur in > autovacuum (may be need to find a way to pass back to user), > vacuumdb or Vacuum can report error to user. > c. How does nworkers input relates to autovacuum_max_workers > which is needed at start for shared memory initialization and in calc > of maxbackends. > d. How to handle database wide vacuum which is possible via vacuumdb > e. What is the best UI (a command like above or via config parameters)? c) seems like the only issue that needs any thought. I don't think its going to be that hard. I don't see any problems with the other points. You can make a function wait, if you wish. > I think we can find a way for above and may be if any other similar things > needs to be taken care, but still I think it is better that we have this > feature > via vacuumdb rather than adding complexity in server code. Also the > current algorithm used in patch is discussed and agreed upon in this > thread and if now we want to go via some other method (auto vacuum), > it might take much more time to build consensus on all the points. Well, I read Alvaro's point from earlier in the thread and agreed with it. All we really need is an instruction to autovacuum to say "be aggressive". Just because somebody added something to the TODO list doesn't make it a good idea. I apologise to Dilip for saying this, it is not anything against him, just the idea. Perhaps we just accept the patch and change AV in the future. >> vacuumdb can still issue the request, but the guts of this are done by >> the server, not a heavily modified client. >> >> If we are going to heavily modify a client then it needs to be able to >> run more than just one thing. Parallel psql would be nice. pg_batch? > > Could you be more specific in this point, I am not able to see how > vacuumdb utility has anything to do with parallel psql? That's my point. All this code in vacuumdb just for this one isolated use case? Twice the maintenance burden. A more generic utility to run commands in parallel would be useful. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Superuser connect during smart shutdown
Over in the "Log notice that checkpoint is to be written on shutdown" thread... On 10/16/14, 2:31 PM, Michael Banck wrote: > There were some comments that this might not actually be the case and/or > that the postmaster was simply waiting for clients to disconnect due to > smart shutdown being invoked. Something else mentioned was that once you start a smart shutdown you have no good way (other than limited ps output) to see what the shutdown is waiting on. I'd like to have some way to get back into the database to see what's going on. Perhaps we could allow superusers to connect while waiting for shutdown. A big warning that we're in shutdown would be nice, and maybe it would make sense to further restrict this to only local connections. It would also be good to be able to abort a smart shutdown if you determine it was a bad idea. Perhaps that's an acceptable way to solve both problems: if your smart shutdown is hung, cancel it and connect to see what's going on. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On 10/16/14, 10:47 AM, Stephen Frost wrote: As others have commented, I too think this should support pg_dump. > > > >I'm uttly mystified as to what that*means*. Everyone asking for it is > >great but until someone can define what "support pg_dump" means, there's > >not much progress I can make towards it.. > >To me, what this repeated discussion on this particular BACKUP point >says, is that the ability to run pg_start/stop_backend and the xlog >related functions should be a different privilege, i.e. something other >than BACKUP; because later we will want the ability to grant someone the >ability to run pg_dump on the whole database without being superuser, >and we will want to use the name BACKUP for that. So I'm inclined to >propose something more specific for this like WAL_CONTROL or >XLOG_OPERATOR, say. Ok. Not sure that I see 'XLOG_OPERATOR' as making sense for 'pg_start_backup' though, and I see the need to support pg_dump'ing the whole database as a non-superuser being more like a 'READONLY' kind of role. We've actually already looked at the notion of a 'READONLY' or 'READALL' role attribute and, well, it's quite simple to implement.. We're talking about a few lines of code to implicitly allow 'USAGE' on all schemas, plus a minor change in ExecCheckRTEPerms to always (or perhaps *only*) include SELECT rights. As there's so much interest in that capability, perhaps we should add it.. One of the big question is- do we take steps to try and prevent a role with this attribute from being able to modify the database in any way? Or would this role attribute only ever increase your rights? Let me address the pg_dump case first, because it's simpler. I want a way to allow certain roles to successfully run pg_dump without being superuser and without having to manually try and maintain a magic read-all role. Note that pg_dump might (today or in the future) need more than just read-all so it's probably wise to treat the two features (pg_dump vs a plain read-all) as separate. For PITR, I see 3 different needs: 1) The ability for someone to start a backup, and if needed cancel that backup 2) The ability to manage running backups/archiving 3) The ability to actually setup/modify PITR infrastructure #2 is probably a weak case that may not be needed; I include it because someone mentioned stopping a backup that someone else started. I think #3 should just require superuser. #1 is what you'd want a more junior person to handle. "Bob needs a snapshot of cluster A". This role needs to be able to run everything you need to get that backup started, monitor it, and cancel if needed. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
> > I'll break them into three pieces- superuser() cleanup, GetUserId() -> > has_privs_of_role(), and the additional-role-attributes patch will just > depend on the others. > The superuser() cleanup has already been submitted to the current commitfest. https://commitfest.postgresql.org/action/patch_view?id=1587 -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Additional role attributes && superuser review
* Simon Riggs (si...@2ndquadrant.com) wrote: > On 16 October 2014 20:04, Robert Haas wrote: > >>> GRANT CAPABILITY whatever TO somebody; > >> > >> So, we went back to just role attributes to avoid the keyword issue.. > >> The above would require making 'CAPABILITY' a reserved word, and there > >> really isn't a 'good' already-reserved word we can use there that I > >> found. > > > > Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER > > ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making > > CAPABILITY a keyword, but it could be unreserved. > > I thought you had it right first time. It is mighty annoying that some > privileges are GRANTed and others ALTER ROLEd. Yeah- but there's a material difference in the two, as I tried to outline previously.. > How about > > GRANT EXECUTE [PRIVILEGES] ON CAPABILITY foo TO bar; > > That is similar to granting execution privs on a function. And I think > gets round the keyword issue? No, it doesn't.. EXECUTE isn't reserved at all. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 16, 2014 at 3:09 PM, Stephen Frost wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > >> Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER > >> ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making > >> CAPABILITY a keyword, but it could be unreserved. > > > > That works for me- would we change the existing role attributes to be > > configurable this way and change everything over to using an int64 in > > the catalog? Unless I'm having trouble counting, I think that would > > actually result in the pg_authid catalog not changing in size at all > > while giving us the ability to add these capabilities and something like > > 50 others if we had cause to. > > I definitely think we should support the new syntax for the existing > attributes. Ok. > I could go either way on whether to change the catalog > storage for the existing attributes. Some people might prefer to > avoid the backward compatibility break, and I can see that argument. There's really two issues when it comes to backwards compatibility here- the catalog representation and the syntax. My feeling is basically this- either we make a clean break to the new syntax and catalog representation, or we just use the same approach the existing attriubtes use. Long term, I think your proposed syntax and an int64 representation is better but it'll mean a lot of client code that has to change. I don't really like the idea of changing the syntax but not the representation, nor am I thrilled with the idea of supporting both syntaxes, and changing the syntax without changing the representation just doesn't make sense to me as I think we'd end up wanting to change it later, making clients have to update their code twice. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On 16 October 2014 20:04, Robert Haas wrote: >>> I'd suggest calling these capabilities, and allow: >>> >>> GRANT CAPABILITY whatever TO somebody; >> >> So, we went back to just role attributes to avoid the keyword issue.. >> The above would require making 'CAPABILITY' a reserved word, and there >> really isn't a 'good' already-reserved word we can use there that I >> found. > > Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER > ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making > CAPABILITY a keyword, but it could be unreserved. I thought you had it right first time. It is mighty annoying that some privileges are GRANTed and others ALTER ROLEd. And we did agree earlier to call these capabilities. How about GRANT EXECUTE [PRIVILEGES] ON CAPABILITY foo TO bar; That is similar to granting execution privs on a function. And I think gets round the keyword issue? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX SCHEMA
On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko wrote: > > On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas wrote: > > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost wrote: > >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > >>> Sawada Masahiko wrote: > >>> > Attached WIP patch adds new syntax REINEX SCHEMA which does reindexing > >>> > all table of specified schema. > >>> > There are syntax dose reindexing specified index, per table and per database, > >>> > but we can not do reindexing per schema for now. > >>> > >>> It seems doubtful that there really is much use for this feature, but if > >>> there is, I think a better syntax precedent is the new ALTER TABLE ALL > >>> IN TABLESPACE thingy, rather than your proposed REINDEX SCHEMA. > >>> Something like REINDEX TABLE ALL IN SCHEMA perhaps. > >> > >> Yeah, I tend to agree that we should be looking at the 'ALL IN > >> TABLESPACE' and 'ALL IN SCHEMA' type of commands to keep things > >> consistent. This might be an alternative for the vacuum / analyze / > >> reindex database commands also.. > > > > Urgh. I don't have a problem with that syntax in general, but it > > clashes pretty awfully with what we're already doing for REINDEX > > otherwise. > > > > Attached patches are latest version patch. Ok. > I changed syntax to REINDEX ALL IN SCHEMA, but I felt a sense of > discomfort a little > as Robert mentioned. > I understood, but the real problem will in a near future when the features will be pushed... :-) They are separated features, but maybe we can join this features to a one future commit... it's just an idea. > Anyway, you can apply these patches in numerical order, > can use REINDEX ALL IN SCHEMA feature and "-S/--schema" option in reindexdb. > > 000_reindex_all_in_schema_v2.patch : It contains REINDEX ALL IN SCHEMA feature 1) Compile without warnings 2) IMHO you can add more test cases to better code coverage: * reindex a schema that doesn't exists * try to run "reindex all in schema" inside a transaction block 3) Isn't enough just? bool do_database = (kind == OBJECT_DATABASE); ... instead of... + bool do_database = (kind == OBJECT_DATABASE) ? true : false; 4) IMHO you can add other Assert to check valid relkinds, like: Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA); 5) I think is more legible: /* Get OID of object for result */ if (do_database) objectOid = MyDatabaseId else objectOid = get_namespace_oid(objectName, false); ... insead of ... + /* Get OID of object for result */ + objectOid = (do_database) ? MyDatabaseId : get_namespace_oid(objectName, false); > 001_Add_schema_option_to_reindexdb_v1.patch : It contains reindexdb > "-S/--schema" supporting > The code itself is good for me, but IMHO you can add test cases to src/bin/scripts/t/090_reindexdb.pl Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Oct 16, 2014 at 3:09 PM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER >> ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making >> CAPABILITY a keyword, but it could be unreserved. > > That works for me- would we change the existing role attributes to be > configurable this way and change everything over to using an int64 in > the catalog? Unless I'm having trouble counting, I think that would > actually result in the pg_authid catalog not changing in size at all > while giving us the ability to add these capabilities and something like > 50 others if we had cause to. I definitely think we should support the new syntax for the existing attributes. I could go either way on whether to change the catalog storage for the existing attributes. Some people might prefer to avoid the backward compatibility break, and I can see that argument. -- 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
Re: [HACKERS] Log notice that checkpoint is to be written on shutdown
Hi,
Am Donnerstag, den 02.10.2014, 15:21 +0200 schrieb Michael Banck:
> we have seen repeatedly that users can be confused about why PostgreSQL
> is not shutting down even though they requested it. Usually, this is
> because `log_checkpoints' is not enabled and the final checkpoint is
> being written, delaying shutdown. As no message besides "shutting down"
> is written to the server log in this case, we even had users believing
> the server was hanging and pondering killing it manually.
There were some comments that this might not actually be the case and/or
that the postmaster was simply waiting for clients to disconnect due to
smart shutdown being invoked.
About the former, it might be possible to hook into the checkpoint code
and try to estimate how much I/O is to be written, and decide on some
threshold above which a warning is issued, but this looks rather fragile
so I won't try to tackle it now.
About the latter, this is a valid concern, and I decided to add a
WARNING in this case (if normal clients are connected), thus moving into
the "additional logging on shutdown" territory.
The other point was changing the default shutdown method and/or the
default for the log_checkpoints parameter. The former has not been
discussed much and the latter was contentious - I won't touch those
either. And even if the defaults are changed, old installations might
still carry the old defaults or DBAs might change them for whatever
reasons, so ISTM additional logging is rather orthogonal to that.
Finally, I am not convinced changing the pg_ctl logging is worthwhile.
I have updated the initial patch with the following: (i) the message got
changed to mention that this is a shutdown checkpoint, (ii) the end of
the shutdown checkpoint is logged as well and (iii) if normal clients
are connected during smart shutdown, a warning is logged.
I'll attach it to the next commitfest and see whether anybody likes it.
Michael
--
Michael Banck
Projektleiter / Berater
Tel.: +49 (2161) 4643-171
Fax: +49 (2161) 4643-100
Email: michael.ba...@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5a4dbb9..e6f03fe 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8085,10 +8085,14 @@ CreateCheckPoint(int flags)
/*
* If enabled, log checkpoint start. We postpone this until now so as not
- * to log anything if we decided to skip the checkpoint.
+ * to log anything if we decided to skip the checkpoint. If we are during
+ * shutdown and checkpoints are not being logged, add a log message that a
+ * checkpoint is to be written and shutdown is potentially delayed.
*/
if (log_checkpoints)
LogCheckpointStart(flags, false);
+ else if (shutdown)
+ ereport(LOG, (errmsg("waiting for shutdown checkpoint ...")));
TRACE_POSTGRESQL_CHECKPOINT_START(flags);
@@ -8311,6 +8315,12 @@ CreateCheckPoint(int flags)
/* Real work is done, but log and update stats before releasing lock. */
LogCheckpointEnd(false);
+ /* On shutdown, log a note about the completed shutdown checkpoint even
+ * if log_checkpoints is off.
+ */
+ if (!log_checkpoints && shutdown)
+ ereport(LOG, (errmsg("shutdown checkpoint completed")));
+
TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
NBuffers,
CheckpointStats.ckpt_segs_added,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ce920ab..99c8911 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2405,6 +2405,11 @@ pmdie(SIGNAL_ARGS)
if (WalWriterPID != 0)
signal_child(WalWriterPID, SIGTERM);
+/* check whether normal children are connected and log a warning if so */
+if (CountChildren(BACKEND_TYPE_NORMAL) != 0)
+ ereport(WARNING,
+ (errmsg("waiting for clients to disconnect ... ")));
+
/*
* If we're in recovery, we can't kill the startup process
* right away, because at present doing so does not release
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER > ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making > CAPABILITY a keyword, but it could be unreserved. That works for me- would we change the existing role attributes to be configurable this way and change everything over to using an int64 in the catalog? Unless I'm having trouble counting, I think that would actually result in the pg_authid catalog not changing in size at all while giving us the ability to add these capabilities and something like 50 others if we had cause to. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
* Robert Haas (robertmh...@gmail.com) wrote: > I'm not sure what your point is. Whether keeping changes separate is > easy or hard, and whether things overlap with multiple other things or > just one, we need to make the effort to do it. What I was getting at is that the role attributes patch would need to depend on these changes.. If the two are completely independent then one would fail to apply cleanly when/if the other is committed, that's all. I'll break them into three pieces- superuser() cleanup, GetUserId() -> has_privs_of_role(), and the additional-role-attributes patch will just depend on the others. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Oct 16, 2014 at 2:59 PM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Thu, Oct 16, 2014 at 11:24 AM, Alvaro Herrera >> wrote: >> > To me, what this repeated discussion on this particular BACKUP point >> > says, is that the ability to run pg_start/stop_backend and the xlog >> > related functions should be a different privilege, i.e. something other >> > than BACKUP; because later we will want the ability to grant someone the >> > ability to run pg_dump on the whole database without being superuser, >> > and we will want to use the name BACKUP for that. So I'm inclined to >> > propose something more specific for this like WAL_CONTROL or >> > XLOG_OPERATOR, say. >> >> I'm a little nervous that we're going to end up with a whole bunch of >> things with names like X_control, Y_operator, and Z_admin, which I >> think is particularly bad if we end up with a mix of styles and also >> bad (though less so) if we end up just tacking the word "operator" >> onto the end of everything. > > Yeah, that's certainly a good point. > >> I'd suggest calling these capabilities, and allow: >> >> GRANT CAPABILITY whatever TO somebody; > > So, we went back to just role attributes to avoid the keyword issue.. > The above would require making 'CAPABILITY' a reserved word, and there > really isn't a 'good' already-reserved word we can use there that I > found. Ah, good point. Using ALTER ROLE is better. Maybe we should do ALTER ROLE .. [ ADD | DROP ] CAPABILITY x. That would still require making CAPABILITY a keyword, but it could be unreserved. -- 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
Re: [HACKERS] UPSERT wiki page, and SQL MERGE syntax
On Thu, Oct 16, 2014 at 11:39 AM, Robert Haas wrote: > On Thu, Oct 16, 2014 at 2:01 PM, Peter Geoghegan wrote: >> I want to retain CONFLICTING(), although I'm thinking about changing >> the spelling to EXCLUDED(). While CONFLICTING() is more or less a new >> and unprecedented style of expression, and in general that's something >> to be skeptical of, I think it's appropriate because what we want here >> isn't quite like any existing expression. Using an alias-like syntax >> is misleading, since it implies that are no effects carried from the >> firing of before row insert triggers. It's also trickier to implement >> alias-like referencing. > > I think that the general consensus was against that style. I don't > like it, and IIRC a few other people commented as well. I think that's accurate, but I'd ask those that didn't like the style to reconsider. Also, I am willing to use any type of special expression, and any keyword or keywords. It doesn't have to be function-like at all. But I believe that an alias-like syntax presents certain difficulties. There is the fact that this isn't a join, and so misleads users, which I've explained. But if I have to add a range table entry for the alias to make parse analysis of the UPDATE work in the more or less conventional way (which is something I like a lot about the current design), then that creates considerable headaches. I have to discriminate against those in the optimizer, when time comes to disallow params in the auxiliary plan. I have to think about each case then, and I think that could add a lot more complexity than you'd think. I'm not really sure. Basically, it's difficult to make this work for technical reasons precisely because what I have here isn't join-like. Can I easily disallow "OLD.*" in a RETURNING clause (recall that we only project inserted tuples, as always)? Even if you think that's okay, I'd rather give an error message indicating that that makes no sense, which is what happens right now. Besides all that, there may also be an advantage to having something similar to MySQL. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Oct 16, 2014 at 11:24 AM, Alvaro Herrera > wrote: > > To me, what this repeated discussion on this particular BACKUP point > > says, is that the ability to run pg_start/stop_backend and the xlog > > related functions should be a different privilege, i.e. something other > > than BACKUP; because later we will want the ability to grant someone the > > ability to run pg_dump on the whole database without being superuser, > > and we will want to use the name BACKUP for that. So I'm inclined to > > propose something more specific for this like WAL_CONTROL or > > XLOG_OPERATOR, say. > > I'm a little nervous that we're going to end up with a whole bunch of > things with names like X_control, Y_operator, and Z_admin, which I > think is particularly bad if we end up with a mix of styles and also > bad (though less so) if we end up just tacking the word "operator" > onto the end of everything. Yeah, that's certainly a good point. > I'd suggest calling these capabilities, and allow: > > GRANT CAPABILITY whatever TO somebody; So, we went back to just role attributes to avoid the keyword issue.. The above would require making 'CAPABILITY' a reserved word, and there really isn't a 'good' already-reserved word we can use there that I found. Also, role attributes aren't inheirited nor is there an 'ADMIN' option for them as there is for GRANT- both of which I feel are correct for these capabilities. Or, to say it another way, I don't think these should have an 'ADMIN' option and I don't think they need to be inheirited through role membership the way granted privileges are. We could still use 'GRANT whatever TO somebody;' without the admin opton and without inheiritance, but I think it'd just be confusing for users who are familiar with how GRANT works already. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] CREATE POLICY and RETURNING
>> That's an argument in favour of only applying a read-filtering policy >> where a RETURNING clause is present, but that introduces the "surprise! >> the effects of your DELETE changed based on an unrelated clause!" issue. > > No- if we were going to do this, I wouldn't want to change the existing > structure but rather provide either: > > a) a way to simply disable RETURNING if the policy is in effect and the >policy creator doesn't wish to allow it > b) allow the user to define another clause which would be applied to the >rows in the RETURNING set I think you could probably make the DELETE policy control what can get deleted, but then have the SELECT policy further filter what gets returned. -- 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
Re: [HACKERS] Review of GetUserId() Usage
On Thu, Oct 16, 2014 at 11:28 AM, Stephen Frost wrote: > On Thursday, October 16, 2014, Robert Haas wrote: >> >> On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost wrote: >> > As a side-note, this change is included in the 'role attributes' patch. >> >> It's really important that we keep separate changes in separate >> patches that are committed in separate commits. Otherwise, it gets >> really confusing. > > I can do that, but it overlaps with the MONITORING role attribute changes > also.. I'm not sure what your point is. Whether keeping changes separate is easy or hard, and whether things overlap with multiple other things or just one, we need to make the effort to do it. -- 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
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Oct 16, 2014 at 11:24 AM, Alvaro Herrera wrote: > Stephen Frost wrote: >> * Petr Jelinek (p...@2ndquadrant.com) wrote: >> > On 15/10/14 07:22, Stephen Frost wrote: >> > > First though, the new privileges, about which the bikeshedding can >> > > begin, short-and-sweet format: >> > > >> > > BACKUP: >> > > pg_start_backup() >> > > pg_stop_backup() >> > > pg_switch_xlog() >> > > pg_create_restore_point() >> > >> > As others have commented, I too think this should support pg_dump. >> >> I'm uttly mystified as to what that *means*. Everyone asking for it is >> great but until someone can define what "support pg_dump" means, there's >> not much progress I can make towards it.. > > To me, what this repeated discussion on this particular BACKUP point > says, is that the ability to run pg_start/stop_backend and the xlog > related functions should be a different privilege, i.e. something other > than BACKUP; because later we will want the ability to grant someone the > ability to run pg_dump on the whole database without being superuser, > and we will want to use the name BACKUP for that. So I'm inclined to > propose something more specific for this like WAL_CONTROL or > XLOG_OPERATOR, say. I'm a little nervous that we're going to end up with a whole bunch of things with names like X_control, Y_operator, and Z_admin, which I think is particularly bad if we end up with a mix of styles and also bad (though less so) if we end up just tacking the word "operator" onto the end of everything. I'd suggest calling these capabilities, and allow: GRANT CAPABILITY whatever TO somebody; ...but keep extraneous words like "control" or "operator" out of the capabilities names themselves. So just wal, xlog, logfile, etc. rather than wal_operator, xlog_operator, logfile_operator and so on. -- 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
Re: [HACKERS] UPSERT wiki page, and SQL MERGE syntax
On Thu, Oct 16, 2014 at 2:01 PM, Peter Geoghegan wrote: > I want to retain CONFLICTING(), although I'm thinking about changing > the spelling to EXCLUDED(). While CONFLICTING() is more or less a new > and unprecedented style of expression, and in general that's something > to be skeptical of, I think it's appropriate because what we want here > isn't quite like any existing expression. Using an alias-like syntax > is misleading, since it implies that are no effects carried from the > firing of before row insert triggers. It's also trickier to implement > alias-like referencing. I think that the general consensus was against that style. I don't like it, and IIRC a few other people commented as well. -- 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
Re: [HACKERS] UPSERT wiki page, and SQL MERGE syntax
On Thu, Oct 16, 2014 at 11:01 AM, Peter Geoghegan wrote: > It is? In any case, I'm working on a revision with this syntax: By the way, in my next revision, barring any objections, the ON CONFLICT (column/expression) syntax is mandatory in the case of ON CONFLICT UPDATE, and optional in the case of ON CONFLICT IGNORE. If we end up supporting exclusion constraints, it's pretty clear that they're only useful with ON CONFLICT IGNORE anyway, and so I guess we don't have to worry about naming those. I guess there would be some advantage to naming an exclusion constraint directly even for the IGNORE case (which is another reason I considered naming an index directly), but it doesn't seem that important. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: dynahash replacement for buffer table
On Thu, Oct 16, 2014 at 12:26 PM, Ryan Johnson wrote: > The only metric where RCU loses to > hazard pointers is if you have really tight timing constraints on resource > reclamation. I think we do have that problem. It's certainly completely unacceptable for a query to prevent buffer reclaim on any significant number of buffers even until the end of the query, let alone the end of the transaction. But, hey, if somebody wants to try writing a patch different than the one that I wrote and see whether it works better than mine, I'm totally cool with that. This is something I came up with, and we're here to evaluate whether it works better than any other option that we have now or that someone wants to develop. I'm not saying this is the best solution; it's just something I've got that seems to basically work. -- 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
[HACKERS] 2014-10 CommitFest
I just tripped over that CommitFest mace that went missing after Heikki put it down from the last CF. Unless someone else wants to pick it up, I'll manage this one. Just to get the ball rolling, I note that 19 patches were moved to this CF from the last one. These are in the following states: Committed pg_dump refactor to remove global variables Ready for Committer Using Levenshtein distance to HINT a candidate column name Custom Plan API pgcrypto: support PGP signatures pg_basebackup vs. Windows and tablespaces (Extend base backup to include symlink file used to restore symlinks) run xmllint during build Waiting on Author Function returning the timestamp of last transaction Flush buffers belonging to unlogged tables Needs Review - with reviewer(s) Selectivity estimation for inet operators Fix local_preload_libraries to work as expected. KNN-GiST with recheck Abbreviated keys (Was: Sort support for text with strxfrm() poor man's keys) Scaling shared buffer eviction Foreign table inheritance Grouping Sets Allow parallel cores to be used by vacuumdb storage parameter specifying the maximum size of GIN pending list Stating the significance of Lehman & Yao in the nbtree README Needs Review - without reviewer Fix xpath() to return namespace definitions (fixes the issue with nested or repeated xpath()) So it would be particularly nice if someone could sign up for that last one. If nobody else jumps in just itching to manage this CF, I'll follow up tomorrow with more details. Meanwhile, please sign up to review a patch! Since there was no previous warning, I'll allow a grace day for the cut-off on submissions for this CF; if it isn't registered in the web application 24 hours after this email, I will move it to the next CF. So look for those patches that "fell through the cracks" without getting registered, and post the ones you've got. -- Kevin Grittner EDB: 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
Re: [HACKERS] UPSERT wiki page, and SQL MERGE syntax
On Thu, Oct 16, 2014 at 6:48 AM, Robert Haas wrote: > If that seems too complicated, leave it out for v1: just insist that > there must be at least one unique non-partial index on the relevant > set of columns. That's what I'll do. > There seems to be some confusion here. This part was about this syntax: > > INSERT INTO overwrite_with_abandon (key, value) > VALUES (42, 'meaning of life') > ON DUPLICATE (key) UPDATE; > > That's a different issue from naming indexes. It is? In any case, I'm working on a revision with this syntax: postgres=# insert into upsert values (1, 'Foo') on conflict (key) update set val = conflicting(val); INSERT 0 1 postgres=# insert into upsert values (1, 'Foo') on conflict (val) update set val = conflicting(val); ERROR: 42P10: could not infer which unique index to use from expressions/columns provided for ON CONFLICT LINE 1: insert into upsert values (1, 'Foo') on conflict (val) updat... ^ HINT: Partial unique indexes are not supported LOCATION: transformConflictClause, parse_clause.c:2365 Expression indexes work fine with this syntax. I want to retain CONFLICTING(), although I'm thinking about changing the spelling to EXCLUDED(). While CONFLICTING() is more or less a new and unprecedented style of expression, and in general that's something to be skeptical of, I think it's appropriate because what we want here isn't quite like any existing expression. Using an alias-like syntax is misleading, since it implies that are no effects carried from the firing of before row insert triggers. It's also trickier to implement alias-like referencing. This is not a join, and I think suggesting that it is by using an alias-like syntax to refer to excluded rows proposed for insertion from the UPDATE is a mistake. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: dynahash replacement for buffer table
On Oct 15, 2014 7:32 PM, "Ants Aasma" wrote: > I'm imagining a bucketized cuckoo hash with 5 item buckets (5-way > associativity). This allows us to fit the bucket onto 2 regular sized > cache lines and have 8 bytes left over. Buckets would be protected by > seqlocks stored in the extra space. On the read side we would only > need 2 read barriers (basically free on x86), and we are guaranteed to > have an answer by fetching two pairs of cache lines. We can trade > memory bandwidth for latency by issuing prefetches (once we add > primitives for this). Alternatively we can trade insert speed for > lookup speed by using asymmetrically sized tables. I was thinking about this. It came to me that we might not even need BufferTag's to be present in the hash table. In the hash table itself we store just a combination of 4 byte buffer tag hash-buffer id. This way we can store 8 pairs in one cacheline. Lookup must account for the fact that due to hash collisions we may find multiple matches one of which may be the buffer we are looking for. We can make condition exceedingly unlikely by taking advantage of the fact that we have two hashes anyway, in each table we use the lookup hash of the other table for tagging. (32bit hash collision within 8 items). By having a reasonably high load factor (75% should work) and 8 bytes per key we even have a pretty good chance of fitting the hotter parts of the buffer lookup table in CPU caches. We use a separate array for the locks (spinlocks should be ok here) for fine grained locking, this may be 1:1 with the buckets or we can map many buckets to a single lock. Otherwise the scheme should work mostly the same. Using this scheme we can get by on the lookup side using 64 bit atomic reads with no barriers, we only need atomic ops for pinning the found buffer. I haven't had the chance to check out how second-chance hashing works and if this scheme would be applicable to it. Regards, Ants Aasma -- Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: dynahash replacement for buffer table
On 16/10/2014 7:59 AM, Robert Haas wrote: On Thu, Oct 16, 2014 at 9:30 AM, Andres Freund wrote: On 2014-10-16 09:19:16 -0400, Robert Haas wrote: On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson wrote: Why not use an RCU mechanism [1] and ditch the hazard pointers? Seems like an ideal fit... In brief, RCU has the following requirements: Read-heavy access pattern Writers must be able to make dead objects unreachable to new readers (easily done for most data structures) Writers must be able to mark dead objects in such a way that existing readers know to ignore their contents but can still traverse the data structure properly (usually straightforward) Readers must occasionally inform the system that they are not currently using any RCU-protected pointers (to allow resource reclamation) Have a look at http://lwn.net/Articles/573424/ and specifically the "URCU overview" section. Basically, that last requirement - that readers inform the system tat they are not currently using any RCU-protected pointers - turns out to require either memory barriers or signals. Well, there's the "quiescent-state-based RCU" - that's actually something that could reasonably be used inside postgres. Put something around socket reads (syscalls are synchronization points) and non-nested lwlock acquires. That'd mean it's nearly no additional overhead. Sure, so, you reuse your existing barriers instead of adding new ones. Making it work sounds like a lot of work for an uncertain benefit though. Perhaps RCU is too much work and/or too little benefit to justify replacing the current latch-based code. I personally am not convinced, but have no hard data to offer other than to point at the rapid (even accelerating) uptake of RCU throughout the Linux kernel (cf. Fig. 1 of http://www2.rdrop.com/users/paulmck/techreports/RCUUsage.2013.02.24a.pdf). However--- If we are convinced the latch-based code needs to go and the question is whether to replace it with RCU or hazard pointers, then RCU wins hands-down IMO. It's comparable work to implement, easier to reason about (RCU read protocol is significantly simpler), and a clearer performance benefit (hazard pointers require more barriers, more atomic ops, more validating, and more pointer chasing than RCU). The only metric where RCU loses to hazard pointers is if you have really tight timing constraints on resource reclamation. Hazard pointers give a tight bound on the number of dead objects that cannot be reclaimed at any given moment, while RCU does not. I've not heard that this is a major concern, though, and in practice it doesn't seem to be a problem unless you forget to annotate an important quiescent point (like a blocking syscall). Ryan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
Ronan Dunklau writes: > From my point of view as a FDW implementor, the feature I need is to have > EXPLAIN (COSTS ON) with stable output for foreign scan nodes. Well, as long as the FDW's costing is exactly predictable, you can have that ... > In the Multicorn FDW (Python API on top of the C-API), we introduced this > commit to make the tests pass on 9.4: > https://github.com/Kozea/Multicorn/commit/76decb360b822b57bf322892ed6c504ba44a8b28 > Clearly, we've lost the ability to test that the costs as set from the Python > API are indeed used. We did fix that yesterday. The remaining argument is about whether it's practical to get platform-independent output out of EXPLAIN ANALYZE. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Directory/File Access Permissions for COPY and Generic File Access Functions
* Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Oct 15, 2014 at 11:34 PM, Tom Lane wrote: > > "Brightwell, Adam" writes: > >> The attached patch for review implements a directory permission system that > >> allows for providing a directory read/write capability to directories for > >> COPY TO/FROM and Generic File Access Functions to non-superusers. > > > > TBH, this sounds like it's adding a lot of mechanism and *significant* > > risk of unforeseen security issues in order to solve a problem that we > > do not need to solve. The field demand for such a feature is just about > > indistinguishable from zero. > > I am also not convinced that we need this. If we need to allow > non-superusers COPY permission at all, can we just exclude certain > "unsafe" directories (like the data directory, and tablespaces) and > let them access anything else? Wow.. I'd say 'no' to this, certainly. Granularity is required here. I want to give a non-superuser the ability to slurp data off a specific NFS mount, not read /etc/passwd.. > Or can we have a whitelist of > directories stored as a PGC_SUSER GUC? This seems awfully heavyweight > for what it is. Hrm, perhaps this would work though.. Allow me to outline a few use-cases which I see for this though and perhaps that'll help us make progress. This started out as a request for a non-superuser to be able to review the log files without needing access to the server. Now, things can certainly be set up on the server to import *all* logs and then grant access to a non-superuser, but generally it's "I need to review the log from X to Y" and not *all* logs need to be stored or kept in PG. In years past, I've wanted to be able to grant this ability out for users to do loads without having to transfer the data through the user's laptop or get them to log onto the Linux box from their Windows desktop and pull the data in via psql (it's a bigger deal than some might think..), and then there's the general ETL case where, without this, you end up running something like Pentaho and having to pass all the data through Java to get it into the database. Building on that is the concept of *background* loads, with pg_background. That's a killer capability, in my view. "Hey, PG, go load all the files in this directory into this table, but don't make me have to stick around and make sure my laptop is still connected for the next 3 hours." Next, the file_fdw could leverage this catalog to do its own checks and allow non-superusers to use it, which would be fantastic and gets back to the 'log file' use-case above. And then there is the next-level item: CREATE TABLESPACE, which we already see folks like RDS and others having to hack the codebase to add as a non-superuser capability. It'd need to be an independently grantable capability, of course. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Petr Jelinek (p...@2ndquadrant.com) wrote: > > > On 15/10/14 07:22, Stephen Frost wrote: > > > > First though, the new privileges, about which the bikeshedding can > > > > begin, short-and-sweet format: > > > > > > > > BACKUP: > > > > pg_start_backup() > > > > pg_stop_backup() > > > > pg_switch_xlog() > > > > pg_create_restore_point() > > > > > > As others have commented, I too think this should support pg_dump. > > > > I'm uttly mystified as to what that *means*. Everyone asking for it is > > great but until someone can define what "support pg_dump" means, there's > > not much progress I can make towards it.. > > To me, what this repeated discussion on this particular BACKUP point > says, is that the ability to run pg_start/stop_backend and the xlog > related functions should be a different privilege, i.e. something other > than BACKUP; because later we will want the ability to grant someone the > ability to run pg_dump on the whole database without being superuser, > and we will want to use the name BACKUP for that. So I'm inclined to > propose something more specific for this like WAL_CONTROL or > XLOG_OPERATOR, say. Ok. Not sure that I see 'XLOG_OPERATOR' as making sense for 'pg_start_backup' though, and I see the need to support pg_dump'ing the whole database as a non-superuser being more like a 'READONLY' kind of role. We've actually already looked at the notion of a 'READONLY' or 'READALL' role attribute and, well, it's quite simple to implement.. We're talking about a few lines of code to implicitly allow 'USAGE' on all schemas, plus a minor change in ExecCheckRTEPerms to always (or perhaps *only*) include SELECT rights. As there's so much interest in that capability, perhaps we should add it.. One of the big question is- do we take steps to try and prevent a role with this attribute from being able to modify the database in any way? Or would this role attribute only ever increase your rights? > > pg_dump doesn't require superuser rights today. If you're looking for a > > role which allows a user to arbitrairly read all data, fine, but that's > > a different consideration and would be a different role attribute, imv. > > If you'd like the role attribute renamed to avoid confusion, I'm all for > > that, just suggest something. > > There. Fair enough, just don't like the specific suggestions. :) In the docs, we talk about pg_start_backup being for 'on-line' backups, perhaps we use ONLINE_BACKUP ? Or PITR_BACKUP ? > (Along the same lines, perhaps the log rotate thingy that's being > mentioned elsewhere could be LOG_OPERATOR instead of just OPERATOR, to > avoid privilege "upgrades" as later releases include more capabilities > to the hypothetical OPERATOR capability.) I'd be fine calling it LOG_OPERATOR instead, sure. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: dynahash replacement for buffer table
On 16/10/2014 7:19 AM, Robert Haas wrote: On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson wrote: Why not use an RCU mechanism [1] and ditch the hazard pointers? Seems like an ideal fit... In brief, RCU has the following requirements: Read-heavy access pattern Writers must be able to make dead objects unreachable to new readers (easily done for most data structures) Writers must be able to mark dead objects in such a way that existing readers know to ignore their contents but can still traverse the data structure properly (usually straightforward) Readers must occasionally inform the system that they are not currently using any RCU-protected pointers (to allow resource reclamation) Have a look at http://lwn.net/Articles/573424/ and specifically the "URCU overview" section. Basically, that last requirement - that readers inform the system tat they are not currently using any RCU-protected pointers - turns out to require either memory barriers or signals. All of the many techniques that have been developed in this area are merely minor variations on a very old theme: set some kind of flag variable in shared memory to let people know that you are reading a shared data structure, and clear it when you are done. Then, other people can figure out when it's safe to recycle memory that was previously part of that data structure. Sure, but RCU has the key benefit of decoupling its machinery (esp. that flag update) from the actual critical section(s) it protects. In a DBMS setting, for example, once per transaction or SQL statement would do just fine. The notification can be much better than a simple flag---you want to know whether the thread has ever quiesced since the last reclaim cycle began, not whether it is currently quiesced (which it usually isn't). In the implementation I use, a busy thread (e.g. not about to go idle) can "chain" its RCU "transactions." In the common case, a chained quiesce call comes when the RCU epoch is not trying to change, and the "flag update" degenerates to a simple load. Further, the only time it's critical to have that memory barrier is if the quiescing thread is about to go idle. Otherwise, missing a flag just imposes a small delay on resource reclamation (and that's assuming the flag in question even belonged to a straggler process). How you implement epoch management, especially the handling of stragglers, is the deciding factor in whether RCU works well. The early URCU techniques were pretty terrible, and maybe general-purpose URCU is doomed to stay that way, but in a DBMS core it can be done very cleanly and efficiently because we can easily add the quiescent points at appropriate locations in the code. In Linux's RCU, the flag variable is "whether the process is currently scheduled on a CPU", which is obviously not workable from user-space. Lacking that, you need an explicit flag variable, which means you need memory barriers, since the protected operation is a load and the flag variable is updated via a store. You can try to avoid some of the overhead by updating the flag variable less often (say, when a signal arrives) or you can make it more fine-grained (in my case, we only prevent reclaim of a fraction of the data structure at a time, rather than all of it) or various other variants, but none of this is unfortunately so simple as "apply technique X and your problem just goes away". Magic wand, no (does nothing for update contention, for example, and requires some care to apply). But from a practical perspective RCU, properly implemented, does make an awful lot of problems an awful lot simpler to tackle. Especially for the readers. Ryan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
Robert, On Thursday, October 16, 2014, Robert Haas wrote: > On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost > wrote: > > As a side-note, this change is included in the 'role attributes' patch. > > It's really important that we keep separate changes in separate > patches that are committed in separate commits. Otherwise, it gets > really confusing. > I can do that, but it overlaps with the MONITORING role attribute changes also.. Thanks, Stephen
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost wrote: > * Petr Jelinek (p...@2ndquadrant.com) wrote: > > On 15/10/14 07:22, Stephen Frost wrote: > > > First though, the new privileges, about which the bikeshedding can > > > begin, short-and-sweet format: > > > > > > BACKUP: > > > pg_start_backup() > > > pg_stop_backup() > > > pg_switch_xlog() > > > pg_create_restore_point() > > > > As others have commented, I too think this should support pg_dump. > > I'm uttly mystified as to what that *means*. Everyone asking for it is > great but until someone can define what "support pg_dump" means, there's > not much progress I can make towards it.. To me, what this repeated discussion on this particular BACKUP point says, is that the ability to run pg_start/stop_backend and the xlog related functions should be a different privilege, i.e. something other than BACKUP; because later we will want the ability to grant someone the ability to run pg_dump on the whole database without being superuser, and we will want to use the name BACKUP for that. So I'm inclined to propose something more specific for this like WAL_CONTROL or XLOG_OPERATOR, say. > pg_dump doesn't require superuser rights today. If you're looking for a > role which allows a user to arbitrairly read all data, fine, but that's > a different consideration and would be a different role attribute, imv. > If you'd like the role attribute renamed to avoid confusion, I'm all for > that, just suggest something. There. (Along the same lines, perhaps the log rotate thingy that's being mentioned elsewhere could be LOG_OPERATOR instead of just OPERATOR, to avoid privilege "upgrades" as later releases include more capabilities to the hypothetical OPERATOR capability.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] group locking: incomplete patch, just for discussion
On Wed, Oct 15, 2014 at 2:55 PM, Simon Riggs wrote: > On 15 October 2014 17:03, Robert Haas wrote: >> Well, I'm fervently in agreement with you on one point: the first >> version of all this needs to be as simple as possible, or the time to >> get to the first version will be longer than we can afford to wait. I >> think what we're discussing here is which things are important enough >> that it makes sense to have them in the first version, and which >> things can wait. > > I'm guessing we might differ slightly on what constitutes as simple as > possible. Yes, I believe there have been occasions in the past when that has happened, so definitely possible. :-) > Something usable, with severe restrictions, is actually better than we > have now. I understand the journey this work represents, so don't be > embarrassed by submitting things with heuristics and good-enoughs in > it. Our mentor, Mr.Lane, achieved much by spreading work over many > releases, leaving others to join in the task. > > Might I gently enquire what the "something usable" we are going to see > in this release? I'm not up on current plans. I don't know how far I'm going to get for this release yet. I think pg_background is a pretty good milestone, and useful in its own right. I would like to get something that's truly parallel working sooner rather than later, but this group locking issue is one of 2 or 3 significant hurdles that I need to climb over first. -- 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
Re: [HACKERS] Better support of exported snapshots with pg_dump
On Wed, Oct 15, 2014 at 1:06 AM, Andres Freund wrote: > I personally think we should just disregard the race here and add a > snapshot parameter. The race is already there and not exactly > small. Let's not kid ourselves that hiding it solves anything. I, too, favor that plan. -- 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
Re: [HACKERS] Directory/File Access Permissions for COPY and Generic File Access Functions
On Wed, Oct 15, 2014 at 11:34 PM, Tom Lane wrote: > "Brightwell, Adam" writes: >> The attached patch for review implements a directory permission system that >> allows for providing a directory read/write capability to directories for >> COPY TO/FROM and Generic File Access Functions to non-superusers. > > TBH, this sounds like it's adding a lot of mechanism and *significant* > risk of unforeseen security issues in order to solve a problem that we > do not need to solve. The field demand for such a feature is just about > indistinguishable from zero. I am also not convinced that we need this. If we need to allow non-superusers COPY permission at all, can we just exclude certain "unsafe" directories (like the data directory, and tablespaces) and let them access anything else? Or can we have a whitelist of directories stored as a PGC_SUSER GUC? This seems awfully heavyweight for what it is. -- 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
Re: [HACKERS] Review of GetUserId() Usage
On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost wrote: > As a side-note, this change is included in the 'role attributes' patch. It's really important that we keep separate changes in separate patches that are committed in separate commits. Otherwise, it gets really confusing. -- 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
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
Le jeudi 16 octobre 2014 10:43:25 Robert Haas a écrit : > On Thu, Oct 16, 2014 at 10:06 AM, Tom Lane wrote: > > Robert Haas writes: > >> On Tue, Oct 14, 2014 at 3:03 AM, David Rowley wrote: > >>> Hmm, was my case above not compelling enough? > >> > >> Apparently not to Tom, but it made sense to me. > > > > No, it wasn't. I'm not convinced either that that patch will get in at > > all, or that it has to have regression tests of that particular form, > > or that such a switch would be sufficient to make such tests platform > > independent. > > People clearly want to be able to run EXPLAIN (ANALYZE) and get stable > output. If the proposed change isn't enough to make that happen, we > need to do more, not give up. Regardless of what happens to inner > join removal. From my point of view as a FDW implementor, the feature I need is to have EXPLAIN (COSTS ON) with stable output for foreign scan nodes. In the Multicorn FDW (Python API on top of the C-API), we introduced this commit to make the tests pass on 9.4: https://github.com/Kozea/Multicorn/commit/76decb360b822b57bf322892ed6c504ba44a8b28 Clearly, we've lost the ability to test that the costs as set from the Python API are indeed used. But I agree that it would be better to have more flexibility in the regression framework itself. If this use case is too marginal to warrant such a change, I'll keep the tests as they are now. -- Ronan Dunklau http://dalibo.com - http://dalibo.org signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
On Thu, Oct 16, 2014 at 10:06 AM, Tom Lane wrote: > Robert Haas writes: >> On Tue, Oct 14, 2014 at 3:03 AM, David Rowley wrote: >>> Hmm, was my case above not compelling enough? > >> Apparently not to Tom, but it made sense to me. > > No, it wasn't. I'm not convinced either that that patch will get in at > all, or that it has to have regression tests of that particular form, > or that such a switch would be sufficient to make such tests platform > independent. People clearly want to be able to run EXPLAIN (ANALYZE) and get stable output. If the proposed change isn't enough to make that happen, we need to do more, not give up. Regardless of what happens to inner join removal. -- 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
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
Andres Freund writes: > On 2014-10-16 10:06:59 -0400, Tom Lane wrote: >> No, it wasn't. I'm not convinced either that that patch will get in at >> all, or that it has to have regression tests of that particular form, >> or that such a switch would be sufficient to make such tests platform >> independent. > Why should the EXPLAIN ANALYZE output without timing information be less > consistent for sensibly selected cases than EXPLAIN itself? To take just one example, the performance numbers that get printed for a sort, such as memory consumption, are undoubtedly platform-dependent. Maybe your idea of "sensibly selected cases" excludes sorting, but I don't find such an argument terribly convincing. I think if we go down this road, we are going to end up with an EXPLAIN that has one hundred parameters turning on and off tiny pieces of the output, none of which are of any great use for anything except the regression tests. I don't want to go there. It would be a lot better to expend the effort on a better regression testing infrastructure that wouldn't *need* bitwise-identical output across platforms. (mysql is ahead of us in that department: they have some hacks for selective matching of the output.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
On 2014-10-16 10:06:59 -0400, Tom Lane wrote: > Robert Haas writes: > > On Tue, Oct 14, 2014 at 3:03 AM, David Rowley wrote: > >> Hmm, was my case above not compelling enough? > > > Apparently not to Tom, but it made sense to me. > > No, it wasn't. I'm not convinced either that that patch will get in at > all, or that it has to have regression tests of that particular form, > or that such a switch would be sufficient to make such tests platform > independent. It's not like we don't already have features where that capability actually would be useful. E.g. testing that certain nodes aren't reached during execution and instead '(never executed)' and things like that. Why should the EXPLAIN ANALYZE output without timing information be less consistent for sensibly selected cases than EXPLAIN itself? I'd actually say stats are harder to get right than the actual number of rows. There also have been bugs where this capability would have been useful. And don't say that regression testing wouldn't have helped there - the case I'm thinking of was broken several times over the years. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]
On Thu, Oct 16, 2014 at 1:56 PM, Simon Riggs wrote: > On 16 October 2014 06:05, Amit Kapila wrote: > > On Thu, Oct 16, 2014 at 8:08 AM, Simon Riggs wrote: > >> > >> This patch seems to allow me to run multiple VACUUMs at once. But I > >> can already do this, with autovacuum. > >> > >> Is there anything this patch can do that cannot be already done with > >> autovacuum? > > > > The difference lies in the fact that vacuumdb (or VACUUM) gives > > the option to user to control the vacuum activity for cases when > > autovacuum doesn't suffice the need, one of the example is to perform > > vacuum via vacuumdb after pg_upgrade or some other maintenance > > activity (as mentioned by Jeff upthread). So I think in all such cases > > having parallel option can give benefit in terms of performance which > > is already shown by Dilip upthread by running some tests (with and > > without patch). > > Why do we need 2 ways to do the same thing? Isn't the multiple ways to do the same already exists like via vacuumdb | Vaccum and autovaccum? > Why not ask autovacuum to do this for you? > > Just send a message to autovacuum to request an immediate action. Let > it manage the children and the tasks. > >SELECT pg_autovacuum_immediate(nworkers = N, list_of_tables); > > Request would allocate an additional N workers and immediately begin > vacuuming the stated tables. I think doing anything on the server side can have higher complexity like: a. Does this function return immediately after sending request to autovacuum, if yes then the behaviour of this new functionality will be different as compare to vacuumdb which user might not expect? b. We need to have some way to handle errors that can occur in autovacuum (may be need to find a way to pass back to user), vacuumdb or Vacuum can report error to user. c. How does nworkers input relates to autovacuum_max_workers which is needed at start for shared memory initialization and in calc of maxbackends. d. How to handle database wide vacuum which is possible via vacuumdb e. What is the best UI (a command like above or via config parameters)? I think we can find a way for above and may be if any other similar things needs to be taken care, but still I think it is better that we have this feature via vacuumdb rather than adding complexity in server code. Also the current algorithm used in patch is discussed and agreed upon in this thread and if now we want to go via some other method (auto vacuum), it might take much more time to build consensus on all the points. > vacuumdb can still issue the request, but the guts of this are done by > the server, not a heavily modified client. > > If we are going to heavily modify a client then it needs to be able to > run more than just one thing. Parallel psql would be nice. pg_batch? Could you be more specific in this point, I am not able to see how vacuumdb utility has anything to do with parallel psql? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
Robert Haas writes: > On Tue, Oct 14, 2014 at 3:03 AM, David Rowley wrote: >> Hmm, was my case above not compelling enough? > Apparently not to Tom, but it made sense to me. No, it wasn't. I'm not convinced either that that patch will get in at all, or that it has to have regression tests of that particular form, or that such a switch would be sufficient to make such tests platform independent. > I think we should > find a way to do something about this - maybe making TIMING OFF also > suppress that info is the simplest approach. We intentionally did *not* make TIMING OFF do that to begin with, and I think changing that behavior now has even less chance of escaping push-back than the "planning time" change did. If we're convinced we must do something, I think exposing the SUMMARY ON/OFF flag (possibly after bikeshedding the name) that I implemented internally yesterday would be the thing to do. But as I said, I find the use-case for this pretty darn weak. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Petr Jelinek (p...@2ndquadrant.com) wrote: > >> Yeah it will, mainly because extensions can load modules and can > >> have untrusted functions, we might want to limit which extensions > >> are possible to create without being superuser. > > > The extension has to be available on the filesystem before it can be > > created, of course. I'm not against providing some kind of whitelist or > > similar which a superuser could control.. That's similar to how PLs > > work wrt pltemplate, no? > > The existing behavior is "you can create an extension if you can execute > all the commands contained in its script". I'm not sure that messing > with that rule is a good idea; in any case it seems well out of scope > for this patch. Right, that's the normal rule. I still like the idea of letting non-superusers create "safe" extensions, but I completely agree- beyond the scope of this patch (as I noted in my initial post). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: dynahash replacement for buffer table
On Thu, Oct 16, 2014 at 9:30 AM, Andres Freund wrote: > On 2014-10-16 09:19:16 -0400, Robert Haas wrote: >> On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson >> wrote: >> > Why not use an RCU mechanism [1] and ditch the hazard pointers? Seems like >> > an ideal fit... >> > >> > In brief, RCU has the following requirements: >> > >> > Read-heavy access pattern >> > Writers must be able to make dead objects unreachable to new readers >> > (easily >> > done for most data structures) >> > Writers must be able to mark dead objects in such a way that existing >> > readers know to ignore their contents but can still traverse the data >> > structure properly (usually straightforward) >> > Readers must occasionally inform the system that they are not currently >> > using any RCU-protected pointers (to allow resource reclamation) >> >> Have a look at http://lwn.net/Articles/573424/ and specifically the >> "URCU overview" section. Basically, that last requirement - that >> readers inform the system tat they are not currently using any >> RCU-protected pointers - turns out to require either memory barriers >> or signals. > > Well, there's the "quiescent-state-based RCU" - that's actually > something that could reasonably be used inside postgres. Put something > around socket reads (syscalls are synchronization points) and non-nested > lwlock acquires. That'd mean it's nearly no additional overhead. Sure, so, you reuse your existing barriers instead of adding new ones. Making it work sounds like a lot of work for an uncertain benefit though. -- 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
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost writes: > * Petr Jelinek (p...@2ndquadrant.com) wrote: >> Yeah it will, mainly because extensions can load modules and can >> have untrusted functions, we might want to limit which extensions >> are possible to create without being superuser. > The extension has to be available on the filesystem before it can be > created, of course. I'm not against providing some kind of whitelist or > similar which a superuser could control.. That's similar to how PLs > work wrt pltemplate, no? The existing behavior is "you can create an extension if you can execute all the commands contained in its script". I'm not sure that messing with that rule is a good idea; in any case it seems well out of scope for this patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
Andrew Gierth writes: > "Bruce" == Bruce Momjian writes: > Bruce> Uh, did this ever get addressed? > It did not. It dropped off the radar screen (I think I'd assumed the patch would appear in the next commitfest, which it didn't unless I missed something). I'll make a note to look at it once I've finished with the timezone abbreviation mess. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hide 'Execution time' in EXPLAIN (COSTS OFF)
On Tue, Oct 14, 2014 at 3:03 AM, David Rowley wrote: >> Andres Freund writes: >> > Well. Unless I miss something it doesn't resolve the problem that >> > started this thread. Namely that it's currently impossible to write >> > regression using EXPLAIN (ANALYZE, TIMING OFF. COSTS OFF). Which is >> > worthwhile because it allows to tests some behaviour that's only visible >> > in actually executed plans (like seing that a subtree wasn't executed). >> >> TBH, I don't particularly care about that. A new flag for turning >> "summary timing" off would answer the complaint with not too much >> non-orthogonality ... but I really don't find this use case compelling >> enough to justify adding a feature to EXPLAIN. >> > Hmm, was my case above not compelling enough? Apparently not to Tom, but it made sense to me. I think we should find a way to do something about this - maybe making TIMING OFF also suppress that info is the simplest approach. -- 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
[HACKERS] Question about RI checks
One of the queries in ri_triggers.c has be a little baffled. For (relatively) obvious reasons, a FK insert triggers a SELECT 1 FROM pk_rel ... FOR KEY SHARE. For not-so-obvious reasons, a PK delete triggers a SELECT 1 FROM fk_rel ... FOR KEY SHARE. I can't see what the lock on fk_rel achieves. Both operations are already contending for the lock on the PK row, which seems like enough to cover every eventuality. And even if the lock serves a purpose, KEY SHARE is an odd choice, since the referencing field is, in general, not a "key" in this sense. Can anyone provide an explanation / counterexample?
Re: [HACKERS] Review of GetUserId() Usage
* Peter Eisentraut (pete...@gmx.net) wrote: > On 9/24/14 4:58 PM, Stephen Frost wrote: > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > >> I think the case for pgstat_get_backend_current_activity() and > >> pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make > >> and seems acceptable to me; but I would leave pg_signal_backend out of > >> that discussion, because it has a potentially harmful side effect. By > >> requiring SET ROLE you add an extra layer of protection against > >> mistakes. (Hopefully, pg_signal_backend() is not a routine thing for > >> well-run systems, which means human intervention, and therefore the room > >> for error isn't insignificant.) > > > > While I certainly understand where you're coming from, I don't really > > buy into it. Yes, cancelling a query (the only thing normal users can > > do anyway- they can't terminate backends) could mean the loss of any > > in-progress work, but it's not like 'rm' and I don't see that it needs > > to require extra hoops for individuals to go through. > > It would be weird if it were inconsistent: some things require role > membership, some things require SET ROLE. Try explaining that. Agreed. As a side-note, this change is included in the 'role attributes' patch. Might be worth splitting out if there is interest in back-patching this, but as it's a behavior change, my thinking was that it wouldn't make sense to back-patch. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] UPSERT wiki page, and SQL MERGE syntax
On Mon, Oct 13, 2014 at 2:02 PM, Peter Geoghegan wrote: >> If the user issues INSERT .. ON DUPLICATE (a) UPDATE, we'll fire >> before-insert triggers and then inspect the tuple to be inserted. If >> b is neither 1 nor 2, then we'll fail with an error saying that we >> can't support ON DUPLICATE without a relevant index to enforce >> uniqueness. (This can presumably re-use the same error message that >> would have popped out if the user done ON DUPLICATE (b), which is >> altogether un-indexed.) But if b is 1 or 2, then we'll search the >> matching index for a conflicting tuple; finding none, we'll insert; >> finding one, we'll do the update as per the user's instructions. > > Before row insert triggers might invalidate that conclusion at the > last possible moment. So you'd have to recheck, which is just messy. I can't imagine that you'd decide which index to use and then change your mind when you turn out to be wrong. I think rather you'd compute a list of possibly-applicable indexes based on the ON DUPLICATE column list, and then, after firing before-insert triggers, check whether there's one that will definitely work. If there's a non-partial unique index on the relevant columns, then you can put any single such index into the list of possibly-usable indexes and leave the rest out; otherwise, you include all the candidates and pick between them at runtime. If that seems too complicated, leave it out for v1: just insist that there must be at least one unique non-partial index on the relevant set of columns. >> I'm considering your points in this area as well as I can, but as far >> as I can tell you haven't actually set what's bad about it, just that >> it might be hazardous in some way that you don't appear to have >> specified, and that MySQL doesn't allow it. I am untroubled by the >> latter point; it is not our goal to confine our implementation to a >> subset of MySQL. > > I did - several times. I linked to the discussion [1]. I said "There > is a trade-off here. I am willing to go another way in that trade-off, > but let's have a realistic conversation about it". And Kevin > eventually said of not supporting partial unique indexes: "That seems > like the only sensible course, to me". At which point I agreed to do > it that way [2]. So you've already won this argument. All it took was > looking at my reasons for doing things that way from my perspective. > If there has been digging of heals, you should consider that it might > be for a reason, even if on balance you still disagree with my > conclusion (which was clearly not really a firm conclusion in this > instance anyway). That's all I mean here. There seems to be some confusion here. This part was about this syntax: INSERT INTO overwrite_with_abandon (key, value) VALUES (42, 'meaning of life') ON DUPLICATE (key) UPDATE; That's a different issue from naming indexes. -- 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
Re: [HACKERS] Materialized views don't show up in information_schema
* Peter Eisentraut (pete...@gmx.net) wrote: > On 10/10/14 8:44 PM, Stephen Frost wrote: > > As a comparison, what about unlogged tables? They're not normal tables > > and they aren't defined by the SQL standard either. > > They are normal tables when considered within the scope of the SQL > standard. The only difference to normal tables is their crash recovery > behavior, which is outside the scope of the SQL standard. Alright, coming back to this, I have to ask- how are matviews different from views from the SQL standard's perspective? I tried looking through the standard to figure it out (and I admit that I probably missed something), but the only thing appears to be a statement in the standard that (paraphrased) "functions are run with the view is queried" and that strikes me as a relatively minor point.. I'm also curious how other databases address this.. Do none of them put matviews into information_schema? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Buffer Requests Trace
Answering your first question: running tpcc for 1 minute, in a database with 64 warehouses (6~7GB), with a buffer pool of 128MB (around 1.8% of database size) and a hit ratio of ~91%, I get a throughput of 45~50 transactions per second. I did some experiments and I got the following information about my tpcc database and benchmark. The database is created with 64 warehouses. Table|Index | Data Size | Index Size | Total +--+---++- stock | stock_pkey | 2209 MB | 263 MB | 2472 MB order_line | order_line_pkey | 2041 MB | 678 MB | 2719 MB customer | idx_customer_name| 1216 MB | 146 MB | 1420 MB customer | customer_pkey| 1216 MB | 58 MB | 1420 MB history| | 164 MB| | 164 MB oorder | oorder_pkey | 134 MB| 68 MB | 362 MB oorder | idx_order| 134 MB| 80 MB | 362 MB oorder | oorder_o_w_id_o_d_id_o_c_id_o_id_key | 134 MB| 80 MB | 362 MB new_order | new_order_pkey | 27 MB | 17 MB | 45 MB item | item_pkey| 10168 kB | 2208 kB | 12 MB district | district_pkey| 776 kB| 72 kB | 880 kB warehouse | warehouse_pkey | 384 kB| 16 kB | 432 kB By executing the tpcc benchmark for 1 minute I get about 2.9 million buffer requests. The distribution of these requests in the relations and indexes are (in descending order): customer1383399 stock_pkey 442600 stock321370 order_line 255314 order_line_pkey 156132 oorder58665 oorder_pkey 57895 customer_pkey 44471 new_order_pkey39552 idx_customer_name 28286 new_order 25861 item_pkey 11702 item 11606 district 11389 district_pkey 7575 warehouse 5276 idx_order 4072 oorder_o_w_id_o_d_id_o_c_id_o_id_key 2410 warehouse_pkey 1998 history1958 All this information seems normal to me. However, from the 2.9 million buffer requests over ~800k pages, only ~150k distinct pages are being requested. This behavior could be explained by the benchmark accessing only a small set of the 64 warehouses instead of having a normal distributed access over the 64 warehouses. In other words, I think that the execution time of the benchmark is irrelevant, assuming that the transactions follow a normal distribution regarding accesses to warehouses. On Wed, Oct 15, 2014 at 7:41 PM, Jeff Janes wrote: > On Wed, Oct 15, 2014 at 6:22 AM, Lucas Lersch > wrote: > >> So is it a possible normal behavior that running tpcc for 10min only >> access 50% of the database? Furthermore, is there a guideline of parameters >> for tpcc (# of warehouses, execution time, operations weight)? >> >> > I'm not familiar with your benchmarking tool. With the one I am most > familiar with, pgbench, if you run it against a database which is too big > to fit in memory, it can take a very long time to touch each page once, > because the constant random disk reads makes it run very slowly. Maybe > that is something to consider here--how many transactions were actually > executed during your 10 min run? > > Also, the tool might build tables that are only used under certain run > options. Perhaps you just aren't choosing the options which invoke usage > of those tables. Since you have the trace data, it should be pretty easy > to count how many distinct blocks are accessed from each relation, and > compare that to the size of the relations to see which relations are unused > or lightly used. > > Cheers, > > Jeff > -- Lucas Lersch
Re: [HACKERS] Additional role attributes && superuser review
* Simon Riggs (si...@2ndquadrant.com) wrote: > On 16 October 2014 12:59, Stephen Frost wrote: > >> > LOGROTATE: > >> > pg_rotate_logfile() > >> > >> Do we need one just for that? > > > > It didn't seem to "belong" to any others and it's currently limited to > > superuser but useful for non-superusers, so I would argue 'yes'. Now, > > another option (actually, for many of these...) would be to trust in our > > authorization system (GRANT EXECUTE) and remove the explicit superuser > > check inside the functions, revoke EXECUTE from public, and tell users > > to GRANT EXECUTE to the roles which should be allowed. > > Seems like OPERATOR would be a general description that could be > useful elsewhere. Ok.. but what else? Are there specific functions which aren't covered by these role attributes which should be and could fall into this category? I'm not against the idea of an 'operator' role, but I don't like the idea that it means 'logrotate, until we figure out some other thing which makes sense to put into this category'. For one thing, this approach would mean that future version which add more rights to the 'operator' role attribute would mean that upgrades are granting rights which didn't previously exist.. > > There was a suggestion raised (by Robert, I believe) that we store these > > additional role attributes as a bitmask instead of individual columns. > > I'm fine with either way, but it'd be a backwards-incompatible change > > for anyone who looks at pg_authid. This would be across a major version > > change, of course, so we are certainly within rights to do so, but I'm > > also not sure how much we need to worry about a few bytes per pg_authid > > row; we still have other issues if we want to try and support millions > > of roles, starting with the inability to partition catalogs.. > > I assumed that was an internal change for fast access. We could make it that way by turning pg_authid into a view and using a new catalog table for roles instead (similar to what we did with pg_user ages ago when we added roles), but that feels like overkill to me. > An array of role attributes would be extensible and more databasey. Hrm. Agreed, and it would save a bit of space for the common case where the user hasn't got any of these attributes, though it wouldn't be as efficient as a bitmap. For my part, I'm not really wedded to any particular catalog representation. Having reviewed the various superuser checks, I think there's a few more role attributes which could/should be added beyond the ones listed, but I don't think we'll ever get to 64 of them, so a single int64 would work if we want the most compact solution. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: dynahash replacement for buffer table
On 2014-10-16 09:19:16 -0400, Robert Haas wrote: > On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson > wrote: > > Why not use an RCU mechanism [1] and ditch the hazard pointers? Seems like > > an ideal fit... > > > > In brief, RCU has the following requirements: > > > > Read-heavy access pattern > > Writers must be able to make dead objects unreachable to new readers (easily > > done for most data structures) > > Writers must be able to mark dead objects in such a way that existing > > readers know to ignore their contents but can still traverse the data > > structure properly (usually straightforward) > > Readers must occasionally inform the system that they are not currently > > using any RCU-protected pointers (to allow resource reclamation) > > Have a look at http://lwn.net/Articles/573424/ and specifically the > "URCU overview" section. Basically, that last requirement - that > readers inform the system tat they are not currently using any > RCU-protected pointers - turns out to require either memory barriers > or signals. Well, there's the "quiescent-state-based RCU" - that's actually something that could reasonably be used inside postgres. Put something around socket reads (syscalls are synchronization points) and non-nested lwlock acquires. That'd mean it's nearly no additional overhead. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Petr Jelinek (p...@2ndquadrant.com) wrote: > The explanation you wrote to Jim makes sense, plus given the comment > from Magnus about REPLICATION flag I guess I am happy enough with it > (I might have liked to have REPLICATION flag to somehow be part of > BACKUP flag but that's very minor thing). k. :) > >The extension has to be available on the filesystem before it can be > >created, of course. I'm not against providing some kind of whitelist or > >similar which a superuser could control.. That's similar to how PLs > >work wrt pltemplate, no? > > > > Makes sense, there is actually extension called pgextwlist which does this. Yeah. Not sure if that should only exist as an extension, but that's really a conversation for a different thread. > >Not sure what you're getting at..? Is there a level of 'granularity' > >for this which would make it safe for non-superusers to create > >untrusted-language functions? I would think that'd be handled mainly > >through extensions, but certainly curious what others think. > > I've been in situation where I wanted to give access to *some* > untrusted languages to non-superuser (as not every untrusted > language can do same kind of damage) and had to solve it via > scripting outside of pg. Really curious what untrusted language you're referring to which isn't as risky as others..? Any kind of filesystem or network access, or the ability to run external commands, is dangerous to give non-superusers. Perhaps more to the point- what would the more granular solution look like..? Though, this is still getting a bit off-topic for this thread. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WIP: dynahash replacement for buffer table
On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson wrote: > Why not use an RCU mechanism [1] and ditch the hazard pointers? Seems like > an ideal fit... > > In brief, RCU has the following requirements: > > Read-heavy access pattern > Writers must be able to make dead objects unreachable to new readers (easily > done for most data structures) > Writers must be able to mark dead objects in such a way that existing > readers know to ignore their contents but can still traverse the data > structure properly (usually straightforward) > Readers must occasionally inform the system that they are not currently > using any RCU-protected pointers (to allow resource reclamation) Have a look at http://lwn.net/Articles/573424/ and specifically the "URCU overview" section. Basically, that last requirement - that readers inform the system tat they are not currently using any RCU-protected pointers - turns out to require either memory barriers or signals. All of the many techniques that have been developed in this area are merely minor variations on a very old theme: set some kind of flag variable in shared memory to let people know that you are reading a shared data structure, and clear it when you are done. Then, other people can figure out when it's safe to recycle memory that was previously part of that data structure. In Linux's RCU, the flag variable is "whether the process is currently scheduled on a CPU", which is obviously not workable from user-space. Lacking that, you need an explicit flag variable, which means you need memory barriers, since the protected operation is a load and the flag variable is updated via a store. You can try to avoid some of the overhead by updating the flag variable less often (say, when a signal arrives) or you can make it more fine-grained (in my case, we only prevent reclaim of a fraction of the data structure at a time, rather than all of it) or various other variants, but none of this is unfortunately so simple as "apply technique X and your problem just goes away". -- 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
Re: [HACKERS] Additional role attributes && superuser review
* Magnus Hagander (mag...@hagander.net) wrote: > On Oct 16, 2014 1:59 PM, "Stephen Frost" wrote: > > Once I understand what "include pg_dump" and "include pg_basebackup" > > mean, I'd be happy to work on adding those. > > Include pg_basebackup would mean the replication protocol methods for base > backup and streaming. Which is already covered by the REPLICATION flag. Well, right. I had the impression there was some distinction that I just wasn't getting. > But in think it's somewhat useful to separate these. Being able to execute > pg_stop_backup allows you to break somebody else's backup currently > running, which pg_basebackup is safe against. So being able to call those > functions is clearly a higher privilege than being able to use > pg_basebackup. Agreed. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On 16 October 2014 12:59, Stephen Frost wrote: >> > LOGROTATE: >> > pg_rotate_logfile() >> >> Do we need one just for that? > > It didn't seem to "belong" to any others and it's currently limited to > superuser but useful for non-superusers, so I would argue 'yes'. Now, > another option (actually, for many of these...) would be to trust in our > authorization system (GRANT EXECUTE) and remove the explicit superuser > check inside the functions, revoke EXECUTE from public, and tell users > to GRANT EXECUTE to the roles which should be allowed. Seems like OPERATOR would be a general description that could be useful elsewhere. > There was a suggestion raised (by Robert, I believe) that we store these > additional role attributes as a bitmask instead of individual columns. > I'm fine with either way, but it'd be a backwards-incompatible change > for anyone who looks at pg_authid. This would be across a major version > change, of course, so we are certainly within rights to do so, but I'm > also not sure how much we need to worry about a few bytes per pg_authid > row; we still have other issues if we want to try and support millions > of roles, starting with the inability to partition catalogs.. I assumed that was an internal change for fast access. An array of role attributes would be extensible and more databasey. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On 16/10/14 13:44, Stephen Frost wrote: * Petr Jelinek (p...@2ndquadrant.com) wrote: On 15/10/14 07:22, Stephen Frost wrote: First though, the new privileges, about which the bikeshedding can begin, short-and-sweet format: BACKUP: pg_start_backup() pg_stop_backup() pg_switch_xlog() pg_create_restore_point() As others have commented, I too think this should support pg_dump. I'm uttly mystified as to what that *means*. Everyone asking for it is great but until someone can define what "support pg_dump" means, there's not much progress I can make towards it.. The explanation you wrote to Jim makes sense, plus given the comment from Magnus about REPLICATION flag I guess I am happy enough with it (I might have liked to have REPLICATION flag to somehow be part of BACKUP flag but that's very minor thing). CREATE EXTENSION This could be a role attribute as the others above, but I didn't want to try and include it in this patch as it has a lot of hairy parts, I expect. Yeah it will, mainly because extensions can load modules and can have untrusted functions, we might want to limit which extensions are possible to create without being superuser. The extension has to be available on the filesystem before it can be created, of course. I'm not against providing some kind of whitelist or similar which a superuser could control.. That's similar to how PLs work wrt pltemplate, no? Makes sense, there is actually extension called pgextwlist which does this. commands/functioncmds.c create untrusted-language functions I often needed more granularity there (plproxy). Not sure what you're getting at..? Is there a level of 'granularity' for this which would make it safe for non-superusers to create untrusted-language functions? I would think that'd be handled mainly through extensions, but certainly curious what others think. I've been in situation where I wanted to give access to *some* untrusted languages to non-superuser (as not every untrusted language can do same kind of damage) and had to solve it via scripting outside of pg. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On Oct 16, 2014 1:59 PM, "Stephen Frost" wrote: > > * Simon Riggs (si...@2ndquadrant.com) wrote: > > On 15 October 2014 06:22, Stephen Frost wrote: > > > BACKUP: > > > pg_start_backup() > > > pg_stop_backup() > > > pg_switch_xlog() > > > pg_create_restore_point() > > > > Yes, but its more complex. As Jim says, you need to include pg_dump, > > plus you need to include the streaming utilities, e.g. pg_basebackup. > > I'd rather have more, simpler, role attributes than one 'catch-all'. > > Once I understand what "include pg_dump" and "include pg_basebackup" > mean, I'd be happy to work on adding those. > Include pg_basebackup would mean the replication protocol methods for base backup and streaming. Which is already covered by the REPLICATION flag. But in think it's somewhat useful to separate these. Being able to execute pg_stop_backup allows you to break somebody else's backup currently running, which pg_basebackup is safe against. So being able to call those functions is clearly a higher privilege than being able to use pg_basebackup. /Magnus
Re: [HACKERS] Additional role attributes && superuser review
Jim, * Jim Nasby (jim.na...@bluetreble.com) wrote: > On 10/15/14, 12:22 AM, Stephen Frost wrote: > > BACKUP: > > pg_start_backup() > > pg_stop_backup() > > pg_switch_xlog() > > pg_create_restore_point() > > It seems odd to me that this (presumably) supports PITR but not pg_dump*. I > realize that most folks probably don't use pg_dump for actual backups, but I > think it is very common to use it to produce schema-only (maybe with data > from a few tables as well) dumps for developers. I've certainly wished I > could offer that ability without going full-blown super-user. Can you clarify what you mean by "supports PITR but not pg_dump"? The role attribute specifically allows a user to execute those functions. Further, yes, this capability could be given to a role which is used by, say, barman, to backup the database, or by other backup solutions which do filesystem backups, but what do you mean by "does not support pg_dump"? What I think you're getting at here is a role attribute which can read all data, which could certainly be done (as, say, a "READONLY" attribute), but I view that a bit differently, as it could be used for auditing and other purposes besides just non-superuser pg_dump support. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Moving of INT64_FORMAT to c.h
On 2014-10-16 08:04:17 -0400, Jan Wieck wrote: > Hi, > > PostgreSQL has for ages defined INT64_FORMAT and UINT64_FORMAT in > pg_config.h. This commit > > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ce486056ecd28050 > > moved those definitions to c.h, which breaks compilation of all released > Slony-I versions against current master. Can those be moved back to where > they used to be? Well, you could add additional configure stuff to also emit what you want. > Slony uses the definitions in external tools, like slon and slonik, to > format sequence numbers in log output. Then it should include c.h/postgres_fe.h? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Moving of INT64_FORMAT to c.h
Hi, PostgreSQL has for ages defined INT64_FORMAT and UINT64_FORMAT in pg_config.h. This commit http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ce486056ecd28050 moved those definitions to c.h, which breaks compilation of all released Slony-I versions against current master. Can those be moved back to where they used to be? Slony uses the definitions in external tools, like slon and slonik, to format sequence numbers in log output. Regards, Jan -- Jan Wieck Senior Software Engineer http://slony.info -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: dynahash replacement for buffer table
On 15/10/2014 11:41 AM, Robert Haas wrote: On Wed, Oct 15, 2014 at 2:03 AM, Andres Freund wrote: On 2014-10-14 17:53:10 -0400, Robert Haas wrote: On Tue, Oct 14, 2014 at 4:42 PM, Andres Freund wrote: The code in CHashSearch shows the problem there: you need to STORE the hazard pointer before you begin to do the LOAD operations to scan the bucket, and you must finish all of those LOADs before you STORE the NULL hazard pointer. A read or write barrier won't provide store/load or load/store ordering. I'm not sure I understand the problem here - but a read + write barrier should suffice? To avoid falling back to two full barriers, we'd need a separate define pg_read_write_barrier(), but that's not a problem. IIUC that should allow us to avoid emitting any actual code on x86. Well, thinking about x86 specifically, it definitely needs at least one mfence, after setting the hazard pointer and before beginning to read the bucket chain. Yes, I can see that now. I do wonder if that's not going to prove quite expensive... I think I can see some ways around needing that. But I think that needs some benchmarking first - no need to build a even more complex solution if not needed. The solution I'm thinking of is to essentially move away from hazard pointers and store something like a generation counter per backend. Which is updated less often, and in combination with other operations. When a backend need to clean up and sees that there's a straggler with a old generation it sends that backend a signal to ensure it sets the latest generation. It's possible that might work ... but on the timescale we're talking about here, that's asking the garbage collecting process to wait for practically geologic time. Back when I first wrote this code, I spent a fair amount of time looking at existing work in the area of lock-free hash tables. Essentially all of the work I was able to find on this topic assumes a threaded model - or more precisely, it assumes that shared memory allocation is cheap and easy and you'll have no problem getting as much of it as you need whenever you need it. This assumption often isn't even spelled out explicitly: it's just assumed that that's the programming environment you're working in. Finding highly parallel algorithms that don't rely on memory allocation as a primitive is hard. Hazard pointers are one of the few techniques I found that seems like it can work in our architecture. I'm quite reluctant to leave aside techniques that have been proven to work well in favor of inventing something entirely novel to PostgreSQL. That having been said, there is some literature on generation numbers, and I think something like that could be made to work. It does have some significant disadvantages, though. One is that a single process which fails to update its generation number prevents all reclamation, system-wide.In my algorithm, a process whose execution is suspended while a hazard pointer is set prevents recycling of only one of many garbage lists. A process searching for a reusable element can mostly likely find some other garbage list to reclaim instead. Also, a generation number implies that we update the value periodically, rather than before and after each critical section. Anything that forces garbage collection to be postponed longer than absolutely necessary seems to me likely to be a loser. It might be a little faster as long as we have free elements to allocate, but as soon as we're out and have to wait longer than we otherwise would for garbage collection, and all system activity halts as a result, even for a few milliseconds, it's going to be a whole lot slower. Or at least, I think. That having been said, I don't know what to do about the fact that the fence is too expensive. I don't know that we've really established that that's the true root of the problem rather than some other pedestrian optimization failure. But the existing code is using an atomic operation to acquire a spinlock, then releasing it, walking the bucket chain, and then using another atomic operation to acquire a spinlock and then releasing it again. Surely a pure fence shouldn't cost more than a spinlock cycle? Even with arguably one extra cache line touch, that seems like it ought to be a win. But my intuitions in this area are shaky. Why not use an RCU mechanism [1] and ditch the hazard pointers? Seems like an ideal fit... In brief, RCU has the following requirements: * Read-heavy access pattern * Writers must be able to make dead objects unreachable to new readers (easily done for most data structures) * Writers must be able to mark dead objects in such a way that existing readers know to ignore their contents but can still traverse the data structure properly (usually straightforward) * Readers must occasionally inform the system that they are not currently using any RCU-protected pointers (to allow resource reclamation) [1] http://www.rdrop.com/~paulmck/RCU/ I
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs wrote: > I find it confusing that the "Lowest" pointer value is also "Invalid". > Valid != Invalid > In complement to that, note that I mentioned Invalid should be UINT_MAX for clarity. -- Michael
Re: [HACKERS] Additional role attributes && superuser review
* Simon Riggs (si...@2ndquadrant.com) wrote: > On 15 October 2014 06:22, Stephen Frost wrote: > > BACKUP: > > pg_start_backup() > > pg_stop_backup() > > pg_switch_xlog() > > pg_create_restore_point() > > Yes, but its more complex. As Jim says, you need to include pg_dump, > plus you need to include the streaming utilities, e.g. pg_basebackup. I'd rather have more, simpler, role attributes than one 'catch-all'. Once I understand what "include pg_dump" and "include pg_basebackup" mean, I'd be happy to work on adding those. > > LOGROTATE: > > pg_rotate_logfile() > > Do we need one just for that? It didn't seem to "belong" to any others and it's currently limited to superuser but useful for non-superusers, so I would argue 'yes'. Now, another option (actually, for many of these...) would be to trust in our authorization system (GRANT EXECUTE) and remove the explicit superuser check inside the functions, revoke EXECUTE from public, and tell users to GRANT EXECUTE to the roles which should be allowed. > > MONITOR: > > View detailed information regarding other processes. > > pg_stat_get_wal_senders() > > pg_stat_get_activity() > > +1 > > > Connect using 'reserved' slots > > This is another one which we might add as another role attribute. > > RESERVED? Seems reasonable, but do we need another GUC for how many connections are reserved for 'RESERVED' roles? Or are we happy to allow those with the RESERVED role attribute to contend for the same slots as superusers? For my 2c- I'm happy to continue to have just one 'pool' of reserved connections and just allow roles with RESERVED to connect using those slots also. > Perhaps we need a few others also - BASHFUL, HAPPY, GRUMPY etc Hah. :) There was a suggestion raised (by Robert, I believe) that we store these additional role attributes as a bitmask instead of individual columns. I'm fine with either way, but it'd be a backwards-incompatible change for anyone who looks at pg_authid. This would be across a major version change, of course, so we are certainly within rights to do so, but I'm also not sure how much we need to worry about a few bytes per pg_authid row; we still have other issues if we want to try and support millions of roles, starting with the inability to partition catalogs.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] jsonb generator functions
2014-10-15 23:49 GMT+02:00 Andrew Dunstan : > > On 10/15/2014 05:47 PM, Alvaro Herrera wrote: > >> Andrew Dunstan wrote: >> >> If we really want to change the name of json_object_two_arg, it >>> would probably be best to change it NOW in 9.4 before it gets out >>> into a production release at all. >>> >> Doesn't it require initdb? If so, I think it's too late now. >> >> yes, it is too heavy argument. ok Pavel > > Yeah, you're right, it would. > > OK, forget that. > > cheers > > andrew >
Re: [HACKERS] Additional role attributes && superuser review
* Petr Jelinek (p...@2ndquadrant.com) wrote: > On 15/10/14 07:22, Stephen Frost wrote: > > First though, the new privileges, about which the bikeshedding can > > begin, short-and-sweet format: > > > > BACKUP: > > pg_start_backup() > > pg_stop_backup() > > pg_switch_xlog() > > pg_create_restore_point() > > As others have commented, I too think this should support pg_dump. I'm uttly mystified as to what that *means*. Everyone asking for it is great but until someone can define what "support pg_dump" means, there's not much progress I can make towards it.. pg_dump doesn't require superuser rights today. If you're looking for a role which allows a user to arbitrairly read all data, fine, but that's a different consideration and would be a different role attribute, imv. If you'd like the role attribute renamed to avoid confusion, I'm all for that, just suggest something. > > For posterity's sake, here's my review and comments on the various > > existing superuser checks in the backend (those not addressed above): > > > > CREATE EXTENSION > > This could be a role attribute as the others above, but I didn't > > want to try and include it in this patch as it has a lot of hairy > > parts, I expect. > > Yeah it will, mainly because extensions can load modules and can > have untrusted functions, we might want to limit which extensions > are possible to create without being superuser. The extension has to be available on the filesystem before it can be created, of course. I'm not against providing some kind of whitelist or similar which a superuser could control.. That's similar to how PLs work wrt pltemplate, no? > > tcop/utility.c > > LOAD (load shared library) > > This already somewhat handles non-superuser access. You can do LOAD > as normal user as long as the library is in $libdir/plugins > directory so it probably does not need separate role attribute > (might be somehow useful in combination with CREATE EXTENSION > though). Ah, fair enough. Note that I wasn't suggesting this be changed, just noting it in my overall review. > > commands/functioncmds.c > > create untrusted-language functions > > I often needed more granularity there (plproxy). Not sure what you're getting at..? Is there a level of 'granularity' for this which would make it safe for non-superusers to create untrusted-language functions? I would think that'd be handled mainly through extensions, but certainly curious what others think. > > commands/functioncmds.c > > execute DO blocks with untrusted languages > > I am not sure if this is significantly different from > untrusted-language functions. Nope, just another case where we're doing a superuser() check. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] CREATE POLICY and RETURNING
* Craig Ringer (cr...@2ndquadrant.com) wrote: > On 10/16/2014 01:44 PM, Craig Ringer wrote: > > So the read-filtering policy should apply to all statements. Not just > > SELECT. > > Oh, IIRC one wrinkle in the prior discussion about this was that doing > this will prevent the implementation of policies that permit users to > update/delete rows they cannot otherwise see. Yeah. > That's an argument in favour of only applying a read-filtering policy > where a RETURNING clause is present, but that introduces the "surprise! > the effects of your DELETE changed based on an unrelated clause!" issue. No- if we were going to do this, I wouldn't want to change the existing structure but rather provide either: a) a way to simply disable RETURNING if the policy is in effect and the policy creator doesn't wish to allow it b) allow the user to define another clause which would be applied to the rows in the RETURNING set > Keep in mind, when considering RETURNING, that users don't always add > this clause directly. PgJDBC will tack a RETURNING clause on the end of > a statement if the user requests generated keys, for example. They will > be very surprised if the behaviour of their DML changes based on whether > or not they asked to get generated keys. Right- that consideration was one of the reasons to not mess with RETURNING, in my view. > To my mind having behaviour change based on RETURNING is actively wrong, > wheras policies that permit rows to be updated/deleted but not selected > are a nice-to-have at most. > > I'd really like to see some more coverage of the details of how these > policies apply to inheritance, both the read- and write- sides of DML > with RETURNING clauses, etc. I assume you mean with regard to documentation..? I'll work on improving that. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] proposal: plpgsql - Assert statement
Hi
2014-10-14 22:57 GMT+02:00 Petr Jelinek :
> On 09/09/14 17:37, Pavel Stehule wrote:
>
>> Ada is language with strong character, and PLpgSQL is little bit strange
>> fork - so it isn't easy to find some form, how to solve all requirements.
>>
>> My requests:
>>
>> * be consistent with current PLpgSQL syntax and logic
>> * allow some future extensibility
>> * allow a static analyses without hard expression processing
>>
>> But I am thinking so there are some points where can be some agreement -
>> although it is not ASSERT implementation.
>>
>> enhancing RAISE WHEN - please, see attached patch -
>>
>> I prefer RAISE WHEN again RAISE WHERE due consistency with EXIT and
>> CONTINUE [ WHEN ];
>>
>>
> Short review of the patch. Note that this has nothing to do with actual
> assertions, at the moment it's just enhancement of RAISE statement to make
> it optionally conditional. As I was one of the people who voted for it I do
> think we want this and I like the syntax.
>
> Code applies cleanly, seems formatted according to project standards -
> there is unnecessary whitespace added in variable declaration part of
> exec_stmt_raise which should be removed.
>
fixed
>
> Passes make check, I would prefer to have little more complex expression
> than just "true" in the new regression test added for this feature.
>
fixed
>
> Did some manual testing, seems to work as advertised.
>
> There are no docs at the moment which is the only show-stopper that I can
> see so far.
>
fixed
Regards
Pavel
>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
>
> PostgreSQL Development, 24x7 Support, Training & Services
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index f195495..eae72f6
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** END LOOP label
! RAISE level 'format' , expression , ... USING option = expression , ... ;
! RAISE level condition_name USING option = expression , ... ;
! RAISE level SQLSTATE 'sqlstate' USING option = expression , ... ;
! RAISE level USING option = expression , ... ;
! RAISE ;
The level option specifies
--- 3369,3379
raise errors.
! RAISE level 'format' , expression , ... USING option = expression , ...WHEN boolean-expression ;
! RAISE level condition_name USING option = expression , ...WHEN boolean-expression ;
! RAISE level SQLSTATE 'sqlstate' USING option = expression , ...WHEN boolean-expression ;
! RAISE level USING option = expression , ... WHEN boolean-expression ;
! RAISE WHEN boolean-expression ;
The level option specifies
*** RAISE unique_violation USING MESSAGE = '
*** 3548,3553
--- 3548,3561
+
+ If WHEN is specified, the message or error is raised
+ only if boolean-expression is true.
+
+ RAISE 'Unexpected NULL in varible: name' WHEN name IS NULL;
+
+
+
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
new file mode 100644
index 11cb47b..1c44f85
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2892,2897
--- 2892,2908
char *err_schema = NULL;
ListCell *lc;
+ if (stmt->cond != NULL)
+ {
+ bool value;
+ bool isnull;
+
+ value = exec_eval_boolean(estate, stmt->cond, &isnull);
+ exec_eval_cleanup(estate);
+ if (isnull || value == false)
+ return PLPGSQL_RC_OK;
+ }
+
/* RAISE with no parameters: re-throw current exception */
if (stmt->condname == NULL && stmt->message == NULL &&
stmt->options == NIL)
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
new file mode 100644
index 893f3a4..1f0b861
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*** static void current_token_is_not_varia
*** 63,68
--- 63,69
static PLpgSQL_expr *read_sql_construct(int until,
int until2,
int until3,
+ int until4,
const char *expected,
const char *sqlstart,
bool isexpression,
*** static void check_labels(const char *
*** 105,111
int end_location);
static PLpgSQL_expr *read_cursor_args(PLpgSQL_var *cursor,
int until, const char *expected);
! static List *read_raise_options(void);
static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%}
--- 106,112
int end_location);
static PLpgSQL_expr *read_cursor_args(PLpgSQL_var *cursor,
int until, const char *expected);
! static List *read_raise_options(int *endtok);
static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%}
*** for_control : for_variable K_IN
*** 1422,1427
--- 1423,1429
expr1 = read_sql_construct(DOT_DOT,
K_LOOP,
0,
+
Re: [HACKERS] CREATE POLICY and RETURNING
Fujii, * Fujii Masao (masao.fu...@gmail.com) wrote: > While I was checking the behavior of RLS, I found that the policy for SELECT > doesn't seem to be applied to RETURNING. Is this intentional? Please see > the following example. Yes, it was intentional. That said, I'm not against changing it. > CREATE ROLE foo LOGIN NOSUPERUSER; > CREATE TABLE hoge AS SELECT col FROM generate_series(1,10) col; > ALTER TABLE hoge ENABLE ROW LEVEL SECURITY; > GRANT SELECT, DELETE ON hoge TO foo; > CREATE POLICY hoge_select_policy ON hoge FOR SELECT TO foo USING (col < 4); > CREATE POLICY hoge_delete_policy ON hoge FOR DELETE TO foo USING (col < 8); > \c - foo > DELETE FROM hoge WHERE col = 6 RETURNING *; > > The policy "hoge_select_policy" should disallow the user "foo" to see the row > with "col = 6". But the last DELETE RETURNING returns that row. The DELETE USING policy allows DELETE to see the record and therefore it's available for RETURNING. > One minor suggestion is: what about changing the message as follows? > There are two more similar messages in policy.c, and they use the word > "row-policy" instead of "policy". For the consistency, I think that > the following also should use the word "row-policy". I was looking at these while going over the larger "try to be more consistent" concerns, but that was leading me towards 'policy' instead of 'row-policy', as the commands are 'CREATE POLICY', etc. Not against going the other way, but it seems more consistent to do 'policy' everywhere.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On 15/10/14 07:22, Stephen Frost wrote: First though, the new privileges, about which the bikeshedding can begin, short-and-sweet format: BACKUP: pg_start_backup() pg_stop_backup() pg_switch_xlog() pg_create_restore_point() As others have commented, I too think this should support pg_dump. For posterity's sake, here's my review and comments on the various existing superuser checks in the backend (those not addressed above): CREATE EXTENSION This could be a role attribute as the others above, but I didn't want to try and include it in this patch as it has a lot of hairy parts, I expect. Yeah it will, mainly because extensions can load modules and can have untrusted functions, we might want to limit which extensions are possible to create without being superuser. tcop/utility.c LOAD (load shared library) This already somewhat handles non-superuser access. You can do LOAD as normal user as long as the library is in $libdir/plugins directory so it probably does not need separate role attribute (might be somehow useful in combination with CREATE EXTENSION though). commands/functioncmds.c create untrusted-language functions I often needed more granularity there (plproxy). commands/functioncmds.c execute DO blocks with untrusted languages I am not sure if this is significantly different from untrusted-language functions. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY
> "Bruce" == Bruce Momjian writes: Bruce> Uh, did this ever get addressed? It did not. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On 15 October 2014 06:22, Stephen Frost wrote: > BACKUP: > pg_start_backup() > pg_stop_backup() > pg_switch_xlog() > pg_create_restore_point() Yes, but its more complex. As Jim says, you need to include pg_dump, plus you need to include the streaming utilities, e.g. pg_basebackup. > LOGROTATE: > pg_rotate_logfile() Do we need one just for that? > MONITOR: > View detailed information regarding other processes. > pg_stat_get_wal_senders() > pg_stat_get_activity() +1 > Connect using 'reserved' slots > This is another one which we might add as another role attribute. RESERVED? Perhaps we need a few others also - BASHFUL, HAPPY, GRUMPY etc -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 9 October 2014 20:01, Robert Haas wrote: > OK, here's an attempt at a real patch for that. I haven't perf-tested this. Patch looks good to me. Surprised we didn't do that before. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c
On 12 September 2014 13:16, Michael Paquier wrote: > On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas > wrote: >> I haven't looked at those places closely, but it seems possible that at >> least some of those variables are supposed to be initialized to a value >> smaller than any valid WAL position, rather than just Invalid. In other >> words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would >> still want those variables to be initialized to zero. As I said, I didn't >> check the code, but before we change those that ought to be checked. > > Ah, OK. I just had a look at that, and receivedUpto and lastComplaint > in xlog.c need to use the lowest pointer value possible as they do a > couple of comparisons with other positions. This is as well the case > of sentPtr in walsender.c. However, that's not the case of writePtr > and flushPtr in walreceiver.c as those positions are just used for > direct comparison with LogstreamResult, so we could use > InvalidXLogRecPtr in this case. I don't see this patch gives us anything. All it will do is prevent easy backpatching of related fixes. -1 for changing the code in this kind of way I find it confusing that the "Lowest" pointer value is also "Invalid". Valid != Invalid -1 for this patch -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
