Re: [HACKERS] Additional role attributes superuser review

2014-10-16 Thread Jim Nasby

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.
--
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] Add CREATE support to event triggers

2014-10-16 Thread Michael Paquier
On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 Here's a new version of this series.  The main change is that I've
 changed deparse_utility.c to generate JSON, and the code that was in
 commands/event_trigger.c to decode that JSON, so that it uses the new
 Jsonb API instead.  In addition, I've moved the new code that was in
 commands/event_trigger.c to utils/adt/ddl_json.c.  (The only entry point
 of the new file is the SQL-callable pg_event_trigger_expand_command()
 function, and its purpose is to expand a JSON object emitted by the
 deparse_utility.c code back into a plain text SQL command.)
 I have also cleaned up the code per comments from Michael Paquier and
 Andres Freund:

 * the GRANT support for event triggers now correctly ignores global
 objects.

 * COMMENT ON .. IS NULL no longer causes a crash
Why would this not fail? Patch 3 in this set is identical to the last
one. I tested that and it worked well...

 * renameatt() and ExecRenameStmt are consistent in returning the
 objSubId as an int (not int32).  This is what is used as objectSubId
 in ObjectAddress, which is what we're using this value for.

In patch 1, ExecRenameStmt returns objsubid for an attribute name when
rename is done on an attribute. Now could you clarify why we skip this
list of objects even if their sub-object ID is available with
address.objectSubId?
case OBJECT_AGGREGATE:
case OBJECT_COLLATION:
case OBJECT_CONVERSION:
case OBJECT_EVENT_TRIGGER:
case OBJECT_FDW:
case OBJECT_FOREIGN_SERVER:
case OBJECT_FUNCTION:
case OBJECT_OPCLASS:
case OBJECT_OPFAMILY:
case OBJECT_LANGUAGE:
case OBJECT_TSCONFIGURATION:
case OBJECT_TSDICTIONARY:
case OBJECT_TSPARSER:
case OBJECT_TSTEMPLATE:

Patch 2 fails on make check for the tests privileges and foreign_data
by showing up double amount warnings for some object types:
*** /Users/ioltas/git/postgres/src/test/regress/expected/privileges.out
Fri Oct 10 14:34:10 2014
--- /Users/ioltas/git/postgres/src/test/regress/results/privileges.out
 Thu Oct 16 15:47:42 2014
***
*** 118,123 
--- 118,124 
  ERROR:  permission denied for relation atest2
  GRANT ALL ON atest1 TO PUBLIC; -- fail
  WARNING:  no privileges were granted for atest1
+ WARNING:  no privileges were granted for atest1
  -- checks in subquery, both ok
  SELECT * FROM atest1 WHERE ( b IN ( SELECT col1 FROM atest2 ) );
   a | b
EventTriggerSupportsGrantObjectType is fine to remove
ACL_OBJECT_DATABASE and ACL_OBJECT_TABLESPACE from the list of
supported objects. That's as well in line with the current firing
matrix. I think that it would be appropriate to add a comment on top
of this function.

Patch 3 looks good.
Regards,
-- 
Michael


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

2014-10-16 Thread Andres Freund
On 2014-10-14 17:53:10 -0400, Robert Haas wrote:
 On Tue, Oct 14, 2014 at 4:42 PM, Andres Freund and...@2ndquadrant.com 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.  It probably doesn't need the second mfence,
 before clearing the the hazard pointer, because x86 allows loads to be
 reordered before stores but not stores before loads.  We could
 certainly try removing the second fence on x86 for benchmarking
 purposes, and we could also check whether mfence outperforms lock;
 addl in that scenario.

Hm. So I thought about this for a while this morning after I wasn't able
to unprogram my hotel room's alarm clock that a previous occupant set
way to early. Given that premise, take the following with a grain of
salt.

The reason for neading an mfence is that a read/write barrier doesn't
guarantee that the store is visible to other processes - just in which
order they become visible. Right? And that's essentially because the
write might sit in the process's store buffer.

So, how about *not* using a full barrier on the reader side (i.e. the
side that does the hazard pointer writes). But instead do something like
a atomic_fetch_add(hazard_ptr, 0) (i.e. lock; xaddl) on the side that
needs to scan the hazard pointers. That, combined with the write memory
barrier, should guarantee that the store buffers are emptied. Which is
pretty nice, because it moves the overhead to the rather infrequent
scanning of the hazard pointers - which needs to do lots of other atomic
ops anyway.

Sounds sane?

That's something that should best be simulated in an exhaustive x86
simulator, but it sounds sane - and it'd be quite awesome imo :)

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] inherit support for foreign tables

2014-10-16 Thread Etsuro Fujita

(2014/08/28 18:00), Etsuro Fujita wrote:

(2014/08/22 11:51), Noah Misch wrote:

Today's ANALYZE VERBOSE messaging for former inheritance parents
(tables with
relhassubclass = true but no pg_inherits.inhparent links) is
deceptive, and I
welcome a fix to omit the spurious message.  As defects go, this is quite
minor.  There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.



A
credible alternative is to emit a second message indicating that we
skipped
the inheritance tree statistics after all, and why we skipped them.



I'd like to address this by emitting the second message as shown below:



A separate patch (analyze.patch) handles the former case in a similar way.


I'll add to the upcoming CF, the analyze.patch as an independent item, 
which emits a second message indicating that we skipped the inheritance 
tree statistics and why we skipped them.


Thanks,

Best regards,
Etsuro Fujita
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
***
*** 1478,1483  acquire_inherited_sample_rows(Relation onerel, int elevel,
--- 1478,1487 
  		/* CCI because we already updated the pg_class row in this command */
  		CommandCounterIncrement();
  		SetRelationHasSubclass(RelationGetRelid(onerel), false);
+ 		ereport(elevel,
+ (errmsg(skipping analyze of \%s.%s\ inheritance tree --- this inheritance tree contains no child tables,
+ 		get_namespace_name(RelationGetNamespace(onerel)),
+ 		RelationGetRelationName(onerel;
  		return 0;
  	}
  

-- 
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: plpgsql - Assert statement

2014-10-16 Thread Pavel Stehule
2014-10-15 11:57 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 2014-09-30 10:04 GMT+07:00 Jim Nasby j...@nasby.net:

 On 9/17/14, 7:40 PM, Jan Wieck wrote:

 Exactly. Doing something like

  ASSERT (select count(*) from foo
  where fk not in (select pk from bar)) = 0;

 is a perfectly fine, arbitrary boolean expression. It will probably work
 well in a development environment too. And I am very sure that it will not
 scale well once that code gets deployed. And I know how DBAs react to the
 guaranteed following performance problem. They will disable ALL assert ...
 or was there some sort of assert class system proposed that I missed?


 Actually, compared with for example Java or C, in production systems,
 usually all asserts are disabled for performance (in java removed by JIT,
 in C we define NDEBUG).


This argument should not be too valid for plpgsql - possible bottlenecks
are in SQL execution (or should be)




  We're also putting too much weight on the term assert here. C-style
 asserts are generally not nearly as useful in a database as general
 sanity-checking or error handling, especially if you're trying to use the
 database to enforce data sanity.


 +1.
 without any query capability, assert will become much less useful. If we
 cannot query in assert, we will code:

 -- perform some query
 ASSERT WHEN some_check_on_query_result;

 .. and disabling the query in production system will become another
 trouble.

 My wish-list for asserts is:

 - Works at a SQL level
 - Unique/clear way to identify asserts (so you're not guessing where the
 assert came from)

 +1


 - Allows for over-riding individual asserts (so if you need to do
 something you're not supposed to do you still have the protection of all
 other asserts)
 - Less verbose than IF THEN RAISE END IF

 +1

 --
 Ali Akbar



Re: [HACKERS] pg_receivexlog --status-interval add fsync feedback

2014-10-16 Thread Simon Riggs
On 10 October 2014 09:28, Fujii Masao masao.fu...@gmail.com wrote:

 In synchronous mode, pg_receivexlog should have similar logic as 
 walreceiver does.

 OK. I understand that removing --fsync-interval has no problem.

 +1 for adding something like --synchronous option. To me,
 it sounds walreceiver-compatible mode rather than synchronous.

 Better to add a new notify message type. And pg_recevexlog should be 
 prepared to receive it at any time. The status might change on the fly, if 
 the server's configuration is reloaded.

 OK. I'll consider it.

 I don't think that's good idea because it prevents us from using
 pg_receivexlog as async walreceiver (i.e., received WAL data is
 fsynced and feedback is sent back to the server soon,
 but transaction commit in the server doesn't wait for the feedback).

Sync rep works by setting parameters on the master. Standby servers
send replies by default, though you can turn replies off.

pg_receivexlog should work the same, but can't do this because it
doesn't know the fsync position unless it fsyncs.

So its not appropriate to have an option called --synchronous in the
same way that there is no parameter called synchronous on the
standby, for good reason.

A new parameter to send feedback should be called --feedback
A second parameter to decide whether to fsync should be called --fsync

if (feedback  fsync)
   send fsynced LSN
else if (feedback)
   send received LSN
; /* else send no feedback */

-- 
 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] Improve automatic analyze messages for inheritance trees

2014-10-16 Thread Simon Riggs
On 16 October 2014 06:49, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote:

 How about this?

 automatic analyze of table \%s.%s.%s\ as inheritance tree

 Thank you for the comment.

Would it be useful to keep track of how many tables just got analyzed?

i.e. analyze of foo (including N inheritance children)

-- 
 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] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

2014-10-16 Thread Simon Riggs
On 16 October 2014 06:05, Amit Kapila amit.kapil...@gmail.com wrote:
 On Thu, Oct 16, 2014 at 8:08 AM, Simon Riggs si...@2ndquadrant.com wrote:


 I've been trying to review this thread with the thought what does
 this give me?. I am keen to encourage contributions and also keen to
 extend our feature set, but I do not wish to complicate our code base.
 Dilip's developments do seem to be good quality; what I question is
 whether we want this feature.

 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?

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.

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?

-- 
 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] New Event Trigger: table_rewrite

2014-10-16 Thread Dimitri Fontaine
Dimitri Fontaine dimi...@2ndquadrant.fr writes:
 Please find attached to this email a patch to implement a new Event
 Trigger, fired on the the table_rewrite event. As attached, it's meant
 as a discussion enabler and only supports ALTER TABLE (and maybe not in
 all forms of it). It will need to grow support for VACUUM FULL and
 CLUSTER and more before getting commited.

And here's already a new version of it, including support for ALTER
TABLE, VACUUM and CLUSTER commands, and documentation.

Still is a small patch:

 doc/src/sgml/event-trigger.sgml | 106 
 src/backend/commands/cluster.c  |  14 ++-
 src/backend/commands/event_trigger.c| 106 +++-
 src/backend/commands/tablecmds.c|  53 --
 src/backend/commands/vacuum.c   |   3 +-
 src/backend/utils/cache/evtcache.c  |   2 +
 src/include/commands/cluster.h  |   4 +-
 src/include/commands/event_trigger.h|   1 +
 src/include/utils/evtcache.h|   3 +-
 src/test/regress/expected/event_trigger.out |  23 +
 src/test/regress/sql/event_trigger.sql  |  24 +
 11 files changed, 322 insertions(+), 17 deletions(-)

-- 
Dimitri Fontaine06 63 07 10 78
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index 6f71a27..08ae838 100644
--- a/doc/src/sgml/event-trigger.sgml
+++ b/doc/src/sgml/event-trigger.sgml
@@ -65,6 +65,12 @@
/para
 
para
+The literaltable_rewrite/ event occurs just before a table is going to
+get rewritten by the commands literalALTER TABLE/literal,
+literalCLUSTER/literal or literalVACUUM/literal.
+   /para
+
+   para
  Event triggers (like other functions) cannot be executed in an aborted
  transaction.  Thus, if a DDL command fails with an error, any associated
  literalddl_command_end/ triggers will not be executed.  Conversely,
@@ -120,6 +126,7 @@
 entryliteralddl_command_start/literal/entry
 entryliteralddl_command_end/literal/entry
 entryliteralsql_drop/literal/entry
+entryliteraltable_rewrite/literal/entry
/row
   /thead
   tbody
@@ -128,510 +135,609 @@
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER COLLATION/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER CONVERSION/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER DOMAIN/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER EXTENSION/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER FOREIGN DATA WRAPPER/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER FOREIGN TABLE/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER FUNCTION/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER LANGUAGE/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=centerliteral-/literal/entry
/row
row
 entry align=leftliteralALTER OPERATOR/literal/entry
 entry align=centerliteralX/literal/entry
 entry 

Re: [HACKERS] Incorrect initialization of sentPtr in walsender.c

2014-10-16 Thread Simon Riggs
On 12 September 2014 13:16, Michael Paquier michael.paqu...@gmail.com wrote:
 On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
 hlinnakan...@vmware.com 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


Re: [HACKERS] Deferring some AtStart* allocations?

2014-10-16 Thread Simon Riggs
On 9 October 2014 20:01, Robert Haas robertmh...@gmail.com 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] Additional role attributes superuser review

2014-10-16 Thread Simon Riggs
On 15 October 2014 06:22, Stephen Frost sfr...@snowman.net 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] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY

2014-10-16 Thread Andrew Gierth
 Bruce == Bruce Momjian br...@momjian.us 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

2014-10-16 Thread Petr Jelinek

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] CREATE POLICY and RETURNING

2014-10-16 Thread Stephen Frost
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] proposal: plpgsql - Assert statement

2014-10-16 Thread Pavel Stehule
Hi

2014-10-14 22:57 GMT+02:00 Petr Jelinek p...@2ndquadrant.com:

 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 optional replaceablelabel/
*** 3369,3379 
  raise errors.
  
  synopsis
! RAISE optional replaceable class=parameterlevel/replaceable /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
! RAISE optional replaceable class=parameterlevel/replaceable /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
! RAISE optional replaceable class=parameterlevel/replaceable /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional;
! RAISE optional replaceable class=parameterlevel/replaceable /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional;
! RAISE ;
  /synopsis
  
  The replaceable class=parameterlevel/replaceable option specifies
--- 3369,3379 
  raise errors.
  
  synopsis
! RAISE optional replaceable class=parameterlevel/replaceable /optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, ... /optional/optional optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional optional WHEN replaceable class=parameterboolean-expression/replaceable /optional;
! RAISE optional replaceable class=parameterlevel/replaceable /optional replaceable class=parametercondition_name/ optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional optional WHEN replaceable class=parameterboolean-expression/replaceable /optional;
! RAISE optional replaceable class=parameterlevel/replaceable /optional SQLSTATE 'replaceable class=parametersqlstate/' optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional /optional optional WHEN replaceable class=parameterboolean-expression/replaceable /optional;
! RAISE optional replaceable class=parameterlevel/replaceable /optional USING replaceable class=parameteroption/replaceable = replaceable class=parameterexpression/replaceable optional, ... /optional optional WHEN replaceable class=parameterboolean-expression/replaceable /optional;
! RAISE optional WHEN replaceable class=parameterboolean-expression/replaceable /optional;
  /synopsis
  
  The replaceable class=parameterlevel/replaceable option specifies
*** RAISE unique_violation USING MESSAGE = '
*** 3548,3553 
--- 3548,3561 
  /para
 /note
  
+para
+  If literalWHEN/literal is specified, the message or error is raised
+  only if replaceableboolean-expression/ is true.
+ 

Re: [HACKERS] CREATE POLICY and RETURNING

2014-10-16 Thread Stephen Frost
* 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] Additional role attributes superuser review

2014-10-16 Thread Stephen Frost
* 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] jsonb generator functions

2014-10-16 Thread Pavel Stehule
2014-10-15 23:49 GMT+02:00 Andrew Dunstan and...@dunslane.net:


 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

2014-10-16 Thread Stephen Frost
* Simon Riggs (si...@2ndquadrant.com) wrote:
 On 15 October 2014 06:22, Stephen Frost sfr...@snowman.net 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] Incorrect initialization of sentPtr in walsender.c

2014-10-16 Thread Michael Paquier
On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs si...@2ndquadrant.com 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Ryan Johnson

On 15/10/2014 11:41 AM, Robert Haas wrote:

On Wed, Oct 15, 2014 at 2:03 AM, Andres Freund and...@2ndquadrant.com wrote:

On 2014-10-14 17:53:10 -0400, Robert Haas wrote:

On Tue, Oct 14, 2014 at 4:42 PM, Andres Freund and...@2ndquadrant.com 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
   

[HACKERS] Moving of INT64_FORMAT to c.h

2014-10-16 Thread Jan Wieck

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] Moving of INT64_FORMAT to c.h

2014-10-16 Thread Andres Freund
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


Re: [HACKERS] Additional role attributes superuser review

2014-10-16 Thread Stephen Frost
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] Additional role attributes superuser review

2014-10-16 Thread Magnus Hagander
On Oct 16, 2014 1:59 PM, Stephen Frost sfr...@snowman.net wrote:

 * Simon Riggs (si...@2ndquadrant.com) wrote:
  On 15 October 2014 06:22, Stephen Frost sfr...@snowman.net 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

2014-10-16 Thread Petr Jelinek

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

2014-10-16 Thread Simon Riggs
On 16 October 2014 12:59, Stephen Frost sfr...@snowman.net 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

2014-10-16 Thread Stephen Frost
* Magnus Hagander (mag...@hagander.net) wrote:
 On Oct 16, 2014 1:59 PM, Stephen Frost sfr...@snowman.net 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson
ryan.john...@cs.utoronto.ca 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

2014-10-16 Thread Stephen Frost
* 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

2014-10-16 Thread Andres Freund
On 2014-10-16 09:19:16 -0400, Robert Haas wrote:
 On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson
 ryan.john...@cs.utoronto.ca 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

2014-10-16 Thread Stephen Frost
* Simon Riggs (si...@2ndquadrant.com) wrote:
 On 16 October 2014 12:59, Stephen Frost sfr...@snowman.net 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] Buffer Requests Trace

2014-10-16 Thread Lucas Lersch
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 jeff.ja...@gmail.com wrote:

 On Wed, Oct 15, 2014 at 6:22 AM, Lucas Lersch lucasler...@gmail.com
 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] Materialized views don't show up in information_schema

2014-10-16 Thread Stephen Frost
* 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] UPSERT wiki page, and SQL MERGE syntax

2014-10-16 Thread Robert Haas
On Mon, Oct 13, 2014 at 2:02 PM, Peter Geoghegan p...@heroku.com 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] Review of GetUserId() Usage

2014-10-16 Thread Stephen Frost
* 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


[HACKERS] Question about RI checks

2014-10-16 Thread Nick Barnes
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] Hide 'Execution time' in EXPLAIN (COSTS OFF)

2014-10-16 Thread Robert Haas
On Tue, Oct 14, 2014 at 3:03 AM, David Rowley dgrowle...@gmail.com wrote:
 Andres Freund and...@2ndquadrant.com 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


Re: [HACKERS] Performance regression: 9.2+ vs. ScalarArrayOpExpr vs. ORDER BY

2014-10-16 Thread Tom Lane
Andrew Gierth and...@tao11.riddles.org.uk writes:
 Bruce == Bruce Momjian br...@momjian.us 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] Additional role attributes superuser review

2014-10-16 Thread Tom Lane
Stephen Frost sfr...@snowman.net 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 9:30 AM, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-10-16 09:19:16 -0400, Robert Haas wrote:
 On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson
 ryan.john...@cs.utoronto.ca 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

2014-10-16 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net 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] Hide 'Execution time' in EXPLAIN (COSTS OFF)

2014-10-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Tue, Oct 14, 2014 at 3:03 AM, David Rowley dgrowle...@gmail.com 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] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

2014-10-16 Thread Amit Kapila
On Thu, Oct 16, 2014 at 1:56 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On 16 October 2014 06:05, Amit Kapila amit.kapil...@gmail.com wrote:
  On Thu, Oct 16, 2014 at 8:08 AM, Simon Riggs si...@2ndquadrant.com
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)

2014-10-16 Thread Andres Freund
On 2014-10-16 10:06:59 -0400, Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Tue, Oct 14, 2014 at 3:03 AM, David Rowley dgrowle...@gmail.com 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] Hide 'Execution time' in EXPLAIN (COSTS OFF)

2014-10-16 Thread Tom Lane
Andres Freund and...@2ndquadrant.com 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)

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 10:06 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Tue, Oct 14, 2014 at 3:03 AM, David Rowley dgrowle...@gmail.com 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] Directory/File Access Permissions for COPY and Generic File Access Functions

2014-10-16 Thread Robert Haas
On Wed, Oct 15, 2014 at 11:34 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Brightwell, Adam adam.brightw...@crunchydatasolutions.com 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] Better support of exported snapshots with pg_dump

2014-10-16 Thread Robert Haas
On Wed, Oct 15, 2014 at 1:06 AM, Andres Freund and...@2ndquadrant.com 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] group locking: incomplete patch, just for discussion

2014-10-16 Thread Robert Haas
On Wed, Oct 15, 2014 at 2:55 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On 15 October 2014 17:03, Robert Haas robertmh...@gmail.com 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] Additional role attributes superuser review

2014-10-16 Thread Alvaro Herrera
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] Review of GetUserId() Usage

2014-10-16 Thread Stephen Frost
Robert,

On Thursday, October 16, 2014, Robert Haas robertmh...@gmail.com wrote:

 On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost sfr...@snowman.net
 javascript:; 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Ryan Johnson

On 16/10/2014 7:19 AM, Robert Haas wrote:

On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson
ryan.john...@cs.utoronto.ca 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] Additional role attributes superuser review

2014-10-16 Thread Stephen Frost
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] Directory/File Access Permissions for COPY and Generic File Access Functions

2014-10-16 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Wed, Oct 15, 2014 at 11:34 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Brightwell, Adam adam.brightw...@crunchydatasolutions.com 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] Hide 'Execution time' in EXPLAIN (COSTS OFF)

2014-10-16 Thread Tom Lane
Ronan Dunklau ronan.dunk...@dalibo.com 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Ryan Johnson

On 16/10/2014 7:59 AM, Robert Haas wrote:

On Thu, Oct 16, 2014 at 9:30 AM, Andres Freund and...@2ndquadrant.com wrote:

On 2014-10-16 09:19:16 -0400, Robert Haas wrote:

On Thu, Oct 16, 2014 at 8:03 AM, Ryan Johnson
ryan.john...@cs.utoronto.ca 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Ants Aasma
On Oct 15, 2014 7:32 PM, Ants Aasma a...@cybertec.at 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] UPSERT wiki page, and SQL MERGE syntax

2014-10-16 Thread Peter Geoghegan
On Thu, Oct 16, 2014 at 6:48 AM, Robert Haas robertmh...@gmail.com 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


[HACKERS] 2014-10 CommitFest

2014-10-16 Thread Kevin Grittner
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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 12:26 PM, Ryan Johnson
ryan.john...@cs.utoronto.ca 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


Re: [HACKERS] UPSERT wiki page, and SQL MERGE syntax

2014-10-16 Thread Peter Geoghegan
On Thu, Oct 16, 2014 at 11:01 AM, Peter Geoghegan p...@heroku.com 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] UPSERT wiki page, and SQL MERGE syntax

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 2:01 PM, Peter Geoghegan p...@heroku.com 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] Additional role attributes superuser review

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 11:24 AM, 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.

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] Review of GetUserId() Usage

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 11:28 AM, Stephen Frost sfr...@snowman.net wrote:
 On Thursday, October 16, 2014, Robert Haas robertmh...@gmail.com wrote:

 On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost sfr...@snowman.net 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] CREATE POLICY and RETURNING

2014-10-16 Thread Robert Haas
 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] Additional role attributes superuser review

2014-10-16 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Oct 16, 2014 at 11:24 AM, Alvaro Herrera alvhe...@2ndquadrant.com 
 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 keyword 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] UPSERT wiki page, and SQL MERGE syntax

2014-10-16 Thread Peter Geoghegan
On Thu, Oct 16, 2014 at 11:39 AM, Robert Haas robertmh...@gmail.com wrote:
 On Thu, Oct 16, 2014 at 2:01 PM, Peter Geoghegan p...@heroku.com 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

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 2:59 PM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Oct 16, 2014 at 11:24 AM, Alvaro Herrera alvhe...@2ndquadrant.com 
 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] Review of GetUserId() Usage

2014-10-16 Thread Stephen Frost
* 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

2014-10-16 Thread Stephen Frost
* 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] Log notice that checkpoint is to be written on shutdown

2014-10-16 Thread Michael Banck
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

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 3:09 PM, Stephen Frost sfr...@snowman.net 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] Proposal : REINDEX SCHEMA

2014-10-16 Thread Fabrízio de Royes Mello
On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:

 On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas robertmh...@gmail.com
wrote:
  On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost sfr...@snowman.net
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

2014-10-16 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Oct 16, 2014 at 3:09 PM, Stephen Frost sfr...@snowman.net 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

2014-10-16 Thread Simon Riggs
On 16 October 2014 20:04, Robert Haas robertmh...@gmail.com 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] Review of GetUserId() Usage

2014-10-16 Thread Brightwell, Adam

 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

2014-10-16 Thread Jim Nasby

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


[HACKERS] Superuser connect during smart shutdown

2014-10-16 Thread Jim Nasby

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] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

2014-10-16 Thread Simon Riggs
On 16 October 2014 15:09, Amit Kapila amit.kapil...@gmail.com 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


Re: [HACKERS] strip nulls functions for json and jsonb

2014-10-16 Thread Pavel Stehule
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 and...@dunslane.net:

 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Robert Haas
On Wed, Oct 15, 2014 at 2:03 AM, Andres Freund and...@2ndquadrant.com 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] Additional role attributes superuser review

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Oct 16, 2014 at 3:09 PM, Stephen Frost sfr...@snowman.net 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] Additional role attributes superuser review

2014-10-16 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost sfr...@snowman.net 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

2014-10-16 Thread Simon Riggs
On 16 October 2014 20:37, Stephen Frost sfr...@snowman.net 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

2014-10-16 Thread Alvaro Herrera
Robert Haas wrote:
 On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost sfr...@snowman.net 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

2014-10-16 Thread Stephen Frost
* Simon Riggs (si...@2ndquadrant.com) wrote:
 On 16 October 2014 20:37, Stephen Frost sfr...@snowman.net 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 something TO role;

As something 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

2014-10-16 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Robert Haas wrote:
  On Thu, Oct 16, 2014 at 3:34 PM, Stephen Frost sfr...@snowman.net 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] pgaudit - an auditing extension for PostgreSQL

2014-10-16 Thread MauMau

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] pgaudit - an auditing extension for PostgreSQL

2014-10-16 Thread Simon Riggs
Thanks for the review.

On 16 October 2014 23:23, MauMau maumau...@gmail.com 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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Andres Freund
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] WIP: dynahash replacement for buffer table

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 6:53 PM, Andres Freund and...@2ndquadrant.com 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] Superuser connect during smart shutdown

2014-10-16 Thread Craig Ringer
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


[HACKERS] [PATCH] HINT: pg_hba.conf changed since last config reload

2014-10-16 Thread Craig Ringer
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 cr...@2ndquadrant.com
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;
+		const char * const hint
+			= edata-hint_log != NULL ? 

Re: [HACKERS] [Segmentation fault] pg_dump binary-upgrade fail for type without element

2014-10-16 Thread Amit Kapila
On Thu, Oct 16, 2014 at 11:16 AM, Rushabh Lathia rushabh.lat...@gmail.com
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


Re: [HACKERS] detect custom-format dumps in psql and emit a useful error

2014-10-16 Thread Craig Ringer
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 cr...@2ndquadrant.com
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 cr...@2ndquadrant.com
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] Superuser connect during smart shutdown

2014-10-16 Thread Tom Lane
Jim Nasby jim.na...@bluetreble.com 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] Better support of exported snapshots with pg_dump

2014-10-16 Thread Michael Paquier
On Fri, Oct 17, 2014 at 12:19 AM, Robert Haas robertmh...@gmail.com wrote:

 On Wed, Oct 15, 2014 at 1:06 AM, Andres Freund and...@2ndquadrant.com
 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 mich...@otacoo.com
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
  /varlistentry
 
  varlistentry
+   termoption--snapshot=replaceable class=parametersnapshotname/replaceable/option/term
+   listitem
+ para
+  Use given synchronous snapshot when taking a dump of the database (see
+  xref linkend=functions-snapshot-synchronization-table for more
+  details).
+ /para
+ para
+  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 xref linkend=logicaldecoding).
+ /para
+ para
+  In the case of a parallel dump, the snapshot name defined by this
+  option is used in priority.
+ /para
+   /listitem
+ /varlistentry
+
+ varlistentry
   termoption--serializable-deferrable/option/term
   listitem
para
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 

Re: [HACKERS] Superuser connect during smart shutdown

2014-10-16 Thread Stephen Frost
* 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


  1   2   >