Re: [HACKERS] Sample configuration files

[Robert Haas]

On Mon, Aug 29, 2016 at 7:04 AM, Vik Fearing  wrote:
> We have sample configuration files for postgresql.conf and
> recovery.conf, but we do not have them for contrib modules.  This patch
> attempts to add them.
>
> Although the patch puts the sample configuration files in the tree, it
> doesn't try to install them.  That's partly because I think it would
> need an extension version bump and that doesn't seem worth it.

I don't think that would need an extension version bump; we only need
that if we're changing which SQL objects get created.  So my opinion
of this effort is:

1. If we're going to add these files, there's no reason not to install
them; in fact, not installing them makes the whole thing rather
pointless, as most people will only see the stuff that gets installed,
not uninstalled files in the source tree.

2. But I'm not sure that this will actually be useful to people.  It
seems like it might just be one more thing for patch authors to
maintain.  I think that if somebody wants to set a parameter defined
for a contrib module, it's easy enough to just add an entry into
postgresql.conf, or use ALTER SYSTEM .. SET.  Going and finding the
sample file (which only sets the value to the default) and then
putting that into your postgresql.conf seems like an extra step.

Of course, others may feel differently and that is fine...

-- 
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] Declarative partitioning - another take

[Ashutosh Bapat]

On Fri, Sep 2, 2016 at 12:23 PM, Amit Langote  wrote:

> On 2016/09/02 15:22, Ashutosh Bapat wrote:
> >>
> >>
> >>> 2. A combination of constraints on the partitions should be applicable
> to
> >>> the parent. We aren't doing that.
> >>
> >> How about on seeing that a RELOPT_OTHER_MEMBER_REL is partitioned parent
> >> table, we can have get_relation_constraints() include a constant false
> >> clause in the list of constraints returned for
> >> relation_excluded_by_constraints() to process so that it is not
> included
> >> in the append result by way of constraint exclusion.  One more option is
> >> to mark such rels dummy in set_rel_size().
> >>
> >>
> > I am not complaining about having parent relation there. For the people
> who
> > are used to seeing the parent relation in the list of append relations,
> it
> > may be awkward. But +1 if we can do that. If we can't do that, we should
> at
> > least mark with an OR of all constraints on the partitions, so that
> > constraint exclusion can exclude it if there are conditions incompatible
> > with constraints. This is what would happen in inheritance case as well,
> if
> > there are constraints on the parent. In the above example, the parent
> table
> > would have constraints CHECK ((a >= 0 AND a < 250) OR (a >= 250 and a <
> > 500) OR (a >= 500 or a < 600)). It will probably get excluded, if
> > constraint exclusion is smart enough to understand ORing.
>
> I guess constraint exclusion would be (is) smart enough to handle that
> correctly but why make it unnecessarily spend a *significant* amount of
> time on doing the proof (when we *know* we can just skip it).  Imagine how
> long the OR list could get.  By the way, my suggestion of just returning a
> constant false clause also does not work - neither in case of a query with
> restrictions (predicate has to be an OpExpr to go ahead with the proof
> which constant false clause is not) nor in case of a query without
> restrictions (proof is not performed at all).  So, that one is useless.
>

Huh!


>
> Getting rid of the parent table in the append list by other means may be a
> way to go.  We know that the table is empty and safe to just drop.
>
>
Ok. Does a constraint (1 = 2) or something like that which is obviously
false, help?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2016/09/02 15:22, Ashutosh Bapat wrote:
>>
>>
>>> 2. A combination of constraints on the partitions should be applicable to
>>> the parent. We aren't doing that.
>>
>> How about on seeing that a RELOPT_OTHER_MEMBER_REL is partitioned parent
>> table, we can have get_relation_constraints() include a constant false
>> clause in the list of constraints returned for
>> relation_excluded_by_constraints() to process so that it is not included
>> in the append result by way of constraint exclusion.  One more option is
>> to mark such rels dummy in set_rel_size().
>>
>>
> I am not complaining about having parent relation there. For the people who
> are used to seeing the parent relation in the list of append relations, it
> may be awkward. But +1 if we can do that. If we can't do that, we should at
> least mark with an OR of all constraints on the partitions, so that
> constraint exclusion can exclude it if there are conditions incompatible
> with constraints. This is what would happen in inheritance case as well, if
> there are constraints on the parent. In the above example, the parent table
> would have constraints CHECK ((a >= 0 AND a < 250) OR (a >= 250 and a <
> 500) OR (a >= 500 or a < 600)). It will probably get excluded, if
> constraint exclusion is smart enough to understand ORing.

I guess constraint exclusion would be (is) smart enough to handle that
correctly but why make it unnecessarily spend a *significant* amount of
time on doing the proof (when we *know* we can just skip it).  Imagine how
long the OR list could get.  By the way, my suggestion of just returning a
constant false clause also does not work - neither in case of a query with
restrictions (predicate has to be an OpExpr to go ahead with the proof
which constant false clause is not) nor in case of a query without
restrictions (proof is not performed at all).  So, that one is useless.

Getting rid of the parent table in the append list by other means may be a
way to go.  We know that the table is empty and safe to just drop.

Thanks,
Amit




-- 
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] pg_basebackup, pg_receivexlog and data durability (was: silent data loss with ext4 / all current versions)

[Michael Paquier]

On Fri, Sep 2, 2016 at 2:20 AM, Peter Eisentraut
 wrote:
> On 5/13/16 2:39 AM, Michael Paquier wrote:
>> So, attached are two patches that apply on HEAD to address the problem
>> of pg_basebackup that does not sync the data it writes. As
>> pg_basebackup cannot use directly initdb -S because, as a client-side
>> utility, it may be installed while initdb is not (see Fedora and
>> RHEL), I have refactored the code so as the routines in initdb.c doing
>> the fsync of PGDATA and other fsync stuff are in src/fe_utils/, and
>> this is 0001.
>
> Why fe_utils?  initdb is not a front-end program.

Thinking about that, you are right. Let's move it to src/common,
frontend-only though.

>> Patch 0002 is a set of fixes for pg_basebackup:
>> - In plain mode, fsync_pgdata is used so as all the tablespaces are
>> fsync'd at once. This takes care as well of the case where pg_xlog is
>> a symlink.
>> - In tar mode (no stdout), each tar file is synced individually, and
>> the base directory is synced once at the end.
>> In both cases, failures are not considered fatal.
>
> Maybe there should be --nosync options like initdb has?

What do others think about that? I could implement that on top of 0002
with some extra options. But to be honest that looks to be just some
extra sugar for what is basically a bug fix... And I am feeling that
providing such a switch to users would be a way for one to shoot
himself badly, particularly for pg_receivexlog where a crash can cause
segments to go missing.
-- 
Michael


0001-Relocation-fsync-routines-of-initdb-into-src-common.patch
Description: application/download


0002-Issue-fsync-more-carefully-in-pg_receivexlog-and-pg_.patch
Description: application/download

-- 
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 SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Wed, Aug 31, 2016 at 3:19 AM, Alvaro Herrera 
wrote:
> Haribabu Kommi wrote:
>
>> Apart from the above, here are the following list of command tags that
>> are generated in the code, I took only the first word of the command tag
>> just to see how many categories present. The number indicates the
>> subset of operations or number of types it is used. Like create table,
>> create function and etc.
>
> Sounds about right.  I suppose all those cases that you aggregated here
> would expand to full tags in the actual code.  I furthermore suppose
> that some of these could be ignored, such as the transaction ones and
> things like load, lock, move, fetch, discard, deallocate (maybe lump
> them all together under "other", or some other rough categorization, as
> Tom suggests).

Following is the pg_stat_sql view with the SQL categories that I considered
that are important. Rest of the them will be shown under others category.

postgres=# \d pg_stat_sql
 View "pg_catalog.pg_stat_sql"
 Column  |   Type   | Modifiers
-+--+---
 inserts | bigint   |
 deletes | bigint   |
 updates | bigint   |
 selects | bigint   |
 declare_cursors | bigint   |
 closes  | bigint   |
 creates | bigint   |
 drops   | bigint   |
 alters  | bigint   |
 imports | bigint   |
 truncates   | bigint   |
 copies  | bigint   |
 grants  | bigint   |
 revokes | bigint   |
 clusters| bigint   |
 vacuums | bigint   |
 analyzes| bigint   |
 refreshs| bigint   |
 locks   | bigint   |
 checkpoints | bigint   |
 reindexes   | bigint   |
 deallocates | bigint   |
 others  | bigint   |
 stats_reset | timestamp with time zone |


If any additions/deletions, I can accommodate them.

The stats data gets updated in exec_simple_query and exec_execute_message
functions and the collected stats will be sent to stats collector similar
like function usage stats in pgstat_report_stat function.

These SQL statistics data is stored in the stats file similar like global
statistics. The STAT file format is updated to accommodate the new stats.

A new GUC "track_sql" is added to track the SQL statement
statistics, by default this is off. Only superuser can change this
parameter.

Attached a patch for the same.

>  Also, for many of these commands it's probably relevant
> whether they are acting on a temporary object or not; we should either
> count these separately, or not count the temp ones at all.

Currently the SQL stats are not checking any object level that is a temp
one or not? The temp objects are specific to a backend only. But what
I feel, any way that is an SQL query that was executed on a temp object,
so we need to count that operation.

I feel this SQL stats should not worry about the object type. May be I am
wrong.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_1.patch
Description: Binary data

-- 
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] Exclude schema during pg_restore

[Michael Banck]

Am Donnerstag, den 01.09.2016, 21:39 -0400 schrieb Peter Eisentraut:
> On 8/31/16 4:10 AM, Michael Banck wrote:
> > attached is a small patch that adds an -N option to pg_restore, in order
> > to exclude a schema, in addition to -n for the restriction to a schema.
> 
> I think this is a good idea, and the approach looks sound.  However,
> something doesn't work right.  If I take an empty database and dump it,
> it will dump the plpgsql extension.  If I run pg_dump in plain-text mode
> with -N, then the plpgsql extension is also dumped (since it is not in
> the excluded schema).  But if I use the new pg_restore -N option, the
> plpgsql extension is not dumped.  Maybe this is because it doesn't have
> a schema, but I haven't checked.

Thanks for the testing and feedback, I hadn't thought of issues with
extensions when I tested myself.  I will take a look.

> pg_dump does not apply --strict-names to -N, but your patch for
> pg_restore does that.  I think that should be made the same as pg_dump.

Aye.


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Declarative partitioning - another take

[Ashutosh Bapat]

>
>
> > 2. A combination of constraints on the partitions should be applicable to
> > the parent. We aren't doing that.
>
> How about on seeing that a RELOPT_OTHER_MEMBER_REL is partitioned parent
> table, we can have get_relation_constraints() include a constant false
> clause in the list of constraints returned for
> relation_excluded_by_constraints() to process so that it is not included
> in the append result by way of constraint exclusion.  One more option is
> to mark such rels dummy in set_rel_size().
>
>
I am not complaining about having parent relation there. For the people who
are used to seeing the parent relation in the list of append relations, it
may be awkward. But +1 if we can do that. If we can't do that, we should at
least mark with an OR of all constraints on the partitions, so that
constraint exclusion can exclude it if there are conditions incompatible
with constraints. This is what would happen in inheritance case as well, if
there are constraints on the parent. In the above example, the parent table
would have constraints CHECK ((a >= 0 AND a < 250) OR (a >= 250 and a <
500) OR (a >= 500 or a < 600)). It will probably get excluded, if
constraint exclusion is smart enough to understand ORing.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Surprising behaviour of \set AUTOCOMMIT ON

[Rushabh Lathia]

I don't think patch is doing correct things:

1)

postgres=# \set AUTOCOMMIT off
postgres=# create table foo (a int );
CREATE TABLE
postgres=# \set AUTOCOMMIT on

Above test not throwing psql error, as you used strcmp(newval,"ON"). You
should use pg_strcasecmp.

2)

postgres=# \set AUTOCOMMIT OFF
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set VERBOSITY ON
\set: Cannot set VERBOSITY to ON inside a transaction, either COMMIT or
ROLLBACK and retry

Above error coming because in below code block change, you don't have check
for
command (you should check opt0 for AUTOCOMMIT command)

-if (!SetVariable(pset.vars, opt0, newval))
+if (transaction_status == PQTRANS_INTRANS && !pset.autocommit
&&
+!strcmp(newval,"ON"))
+{
+psql_error("\\%s: Cannot set %s to %s inside a
transaction, either COMMIT or ROLLBACK and retry\n",
+cmd, opt0, newval);
+success = false;
+}

3)

postgres=# BEGIN;
BEGIN
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set AUTOCOMMIT ON

Don't you think, in above case also we should throw a psql error?

4)

postgres=# \set AUTOCOMMIT off
postgres=# create table foo ( a int );
CREATE TABLE
postgres=# \set XYZ ON
\set: Cannot set XYZ to ON inside a transaction, either COMMIT or ROLLBACK
and retry

May be you would like to move the new code block inside SetVariable(). So
that
don't need to worry about the validity of the variable names.

Basically if I understand correctly, if we are within transaction and
someone
tries the set the AUTOCOMMIT ON, it should throw a psql error. Correct me
if I am missing anything?

On Thu, Sep 1, 2016 at 4:23 PM, Rahila Syed  wrote:

> Ok. Please find attached a patch which introduces psql error when
> autocommit is turned on inside a transaction. It also adds relevant
> documentation in psql-ref.sgml. Following is the output.
>
> bash-4.2$ psql -d postgres
> psql (10devel)
> Type "help" for help.
>
> postgres=# \set AUTOCOMMIT OFF
> postgres=# create table test(i int);
> CREATE TABLE
> postgres=# \set AUTOCOMMIT ON
> \set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or
> ROLLBACK and retry
> postgres=#
>
>
> Thank you,
> Rahila Syed
>
> On Wed, Aug 17, 2016 at 6:31 PM, Robert Haas 
> wrote:
>
>> On Tue, Aug 16, 2016 at 5:25 PM, Rahila Syed 
>> wrote:
>> >>I think I like the option of having psql issue an error.  On the
>> >>server side, the transaction would still be open, but the user would
>> >>receive a psql error message and the autocommit setting would not be
>> >>changed.  So the user could type COMMIT or ROLLBACK manually and then
>> >>retry changing the value of the setting.
>> >
>> > Throwing psql error comes out to be most accepted outcome on this
>> thread. I
>> > agree it is safer than guessing user intention.
>> >
>> > Although according to the default behaviour of psql, error will abort
>> the
>> > current transaction and roll back all the previous commands.
>>
>> A server error would do that, but a psql errror won'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
>
>


-- 
Rushabh Lathia

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2016/09/02 14:38, Ashutosh Bapat wrote:
> Here's something I observed with your set of patches posted in June. I have
> not checked the latest set of patches. So, if it's something fixed, please
> ignore the mail and sorry for me being lazy.
> 
> prt1 is partitioned table and it shows following information with \d+
> 
> regression=# \d+ prt1
> Partitioned table "public.prt1"
>  Column |   Type| Modifiers | Storage  | Stats target |
> Description
> +---+---+--+--+-
>  a  | integer   |   | plain|  |
>  b  | integer   |   | plain|  |
>  c  | character varying |   | extended |  |
> Partition Key: PARTITION BY RANGE (a)
> Indexes:
> "iprt1_a" btree (a)
> 
> Shouldn't we show all the partitions of this table and may be their ranges
> of lists?

Something I thought about as well.  I will implement that.

> I found the partitions from EXPLAIN plan
> 
> regression=# explain verbose select * from prt1;
>   QUERY PLAN
> ---
>  Append  (cost=0.00..6.00 rows=301 width=13)
>->  Seq Scan on public.prt1  (cost=0.00..0.00 rows=1 width=40)
>  Output: prt1.a, prt1.b, prt1.c
>->  Seq Scan on public.prt1_p1  (cost=0.00..2.25 rows=125 width=13)
>  Output: prt1_p1.a, prt1_p1.b, prt1_p1.c
>->  Seq Scan on public.prt1_p3  (cost=0.00..1.50 rows=50 width=13)
>  Output: prt1_p3.a, prt1_p3.b, prt1_p3.c
>->  Seq Scan on public.prt1_p2  (cost=0.00..2.25 rows=125 width=13)
>  Output: prt1_p2.a, prt1_p2.b, prt1_p2.c
> (9 rows)
> 
> Then did \d+ on each of those to find their ranges

[ ... ]

> 
> As you will observe that the table prt1 can not have any row with a < 0 and
> a > 600. But when I execute
> 
> regression=# explain verbose select * from prt1 where a > 100;
> QUERY PLAN
> --
>  Append  (cost=0.00..0.00 rows=1 width=40)
>->  Seq Scan on public.prt1  (cost=0.00..0.00 rows=1 width=40)
>  Output: prt1.a, prt1.b, prt1.c
>  Filter: (prt1.a > 100)
> (4 rows)
> 
> it correctly excluded all the partitions, but did not exclude the parent
> relation. I guess, we have enough information to exclude it. Probably, we
> should add a check constraint on the parent which is OR of the check
> constraints on all the partitions. So there are two problems here
> 
> 1. \d+ doesn't show partitions - this is probably reported earlier, I don't
> remember.

You just did, :)

As I said I will implement that on lines of how inheritance children are
listed (with additional information ie, range or list).

> 2. A combination of constraints on the partitions should be applicable to
> the parent. We aren't doing that.

How about on seeing that a RELOPT_OTHER_MEMBER_REL is partitioned parent
table, we can have get_relation_constraints() include a constant false
clause in the list of constraints returned for
relation_excluded_by_constraints() to process so that it is not included
in the append result by way of constraint exclusion.  One more option is
to mark such rels dummy in set_rel_size().

Thanks,
Amit




-- 
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] Forbid use of LF and CR characters in database and role names

[Michael Paquier]

On Fri, Sep 2, 2016 at 2:44 AM, Peter Eisentraut
 wrote:
> On 8/11/16 9:12 PM, Michael Paquier wrote:
>> Note that pg_dump[all] and pg_upgrade already have safeguards against
>> those things per the same routines putting quotes for execution as
>> commands into psql and shell. So attached is a patch to implement this
>> restriction in the backend,
>
> How about some documentation?  I think the CREATE ROLE and CREATE
> DATABASE man pages might be suitable places.

Sure. What do you think about that?
+  
+Database names cannot include LF or CR characters
+as those could be at the origin of security breaches, particularly on
+Windows where the command shell is unusable with arguments containing
+such characters.
+   
-- 
Michael
diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index cf33746..d73dc3a 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -260,6 +260,13 @@ CREATE DATABASE name
connection slot remains for the database, it is possible that
both will fail.  Also, the limit is not enforced against superusers.
   
+
+  
+Database names cannot include LF or CR characters
+as those could be at the origin of security breaches, particularly on
+Windows where the command shell is unusable with arguments containing
+such characters.
+   
  
 
  
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8..27259f2 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -404,6 +404,13 @@ CREATE ROLE name [ [ WITH ] \password that can be used to safely change the
password later.
   
+
+  
+   Role names cannot include LF or CR characters
+   as those could be at the origin of security breaches, particularly on
+   Windows where the command shell is unusable with arguments containing
+   such characters.
+  
  
 
  
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index c1c0223..5746958 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -77,6 +77,7 @@ typedef struct
 } movedb_failure_params;
 
 /* non-export function prototypes */
+static void check_db_name(const char *dbname);
 static void createdb_failure_callback(int code, Datum arg);
 static void movedb(const char *dbname, const char *tblspcname);
 static void movedb_failure_callback(int code, Datum arg);
@@ -456,6 +457,9 @@ createdb(const CreatedbStmt *stmt)
 		/* Note there is no additional permission check in this path */
 	}
 
+	/* do sanity checks on database name */
+	check_db_name(dbname);
+
 	/*
 	 * Check for db name conflict.  This is just to give a more friendly error
 	 * message than "unique index violation".  There's a race condition but
@@ -745,6 +749,22 @@ check_encoding_locale_matches(int encoding, const char *collate, const char *cty
    pg_encoding_to_char(collate_encoding;
 }
 
+/*
+ * Perform sanity checks on the database name.
+ */
+static void
+check_db_name(const char *dbname)
+{
+	if (strchr(dbname, '\n') != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name cannot use LF character")));
+	if (strchr(dbname, '\r') != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name cannot use CR character")));
+}
+
 /* Error cleanup callback for createdb */
 static void
 createdb_failure_callback(int code, Datum arg)
@@ -949,6 +969,9 @@ RenameDatabase(const char *oldname, const char *newname)
 	int			npreparedxacts;
 	ObjectAddress address;
 
+	/* check format of new name */
+	check_db_name(newname);
+
 	/*
 	 * Look up the target database's OID, and get exclusive lock on it. We
 	 * need this for the same reasons as DROP DATABASE.
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index b6ea950..8954e16 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -57,6 +57,21 @@ static void DelRoleMems(const char *rolename, Oid roleid,
 			bool admin_opt);
 
 
+/* Do sanity checks on role name */
+static void
+check_role_name(const char *rolname)
+{
+	if (strchr(rolname, '\n') != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name cannot use LF character")));
+	if (strchr(rolname, '\r') != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name cannot use CR character")));
+}
+
+
 /* Check if current user has createrole privileges */
 static bool
 have_createrole_privilege(void)
@@ -111,6 +126,9 @@ CreateRole(CreateRoleStmt *stmt)
 	DefElem*dvalidUntil = NULL;
 	DefElem*dbypassRLS = NULL;
 
+	/* sanity check for role name */
+	check_role_name(stmt->role);
+
 	/* The defaults can vary depending on the original statement type */
 	switch (stmt->stmt_type)
 	{
@@ -1137,6 +1155,9 @@ RenameRole(const char *oldname, const char *newname)
 	ObjectAddress 

Re: [HACKERS] [Patch] RBTree iteration interface improvement

[Heikki Linnakangas]

On 08/26/2016 04:07 PM, Aleksander Alekseev wrote:

Another unrelated change in this patch is the addition of
rb_rightmost(). It's not used for anything, so I'm not sure what the
point is. Then again, there don't seem to be any callers of
rb_leftmost() either.


It's just something I needed in tests to reach a good percent of code
coverage. Implementation of rb_rightmost is trivial so we probably can do
without it.


Looking closer, we don't currently use any of the iterators besides the 
left-right iterator either. Nor rb_delete().



I think we should something like in the attached patch. It seems to pass
your test suite, but I haven't done any other testing on this. Does it
look OK to you?


Looks good to me.


Ok, committed.

- Heikki



--
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] Declarative partitioning - another take

[Ashutosh Bapat]

Here's something I observed with your set of patches posted in June. I have
not checked the latest set of patches. So, if it's something fixed, please
ignore the mail and sorry for me being lazy.

prt1 is partitioned table and it shows following information with \d+

regression=# \d+ prt1
Partitioned table "public.prt1"
 Column |   Type| Modifiers | Storage  | Stats target |
Description
+---+---+--+--+-
 a  | integer   |   | plain|  |
 b  | integer   |   | plain|  |
 c  | character varying |   | extended |  |
Partition Key: PARTITION BY RANGE (a)
Indexes:
"iprt1_a" btree (a)

Shouldn't we show all the partitions of this table and may be their ranges
of lists?

I found the partitions from EXPLAIN plan

regression=# explain verbose select * from prt1;
  QUERY PLAN
---
 Append  (cost=0.00..6.00 rows=301 width=13)
   ->  Seq Scan on public.prt1  (cost=0.00..0.00 rows=1 width=40)
 Output: prt1.a, prt1.b, prt1.c
   ->  Seq Scan on public.prt1_p1  (cost=0.00..2.25 rows=125 width=13)
 Output: prt1_p1.a, prt1_p1.b, prt1_p1.c
   ->  Seq Scan on public.prt1_p3  (cost=0.00..1.50 rows=50 width=13)
 Output: prt1_p3.a, prt1_p3.b, prt1_p3.c
   ->  Seq Scan on public.prt1_p2  (cost=0.00..2.25 rows=125 width=13)
 Output: prt1_p2.a, prt1_p2.b, prt1_p2.c
(9 rows)

Then did \d+ on each of those to find their ranges

regression=# \d+ prt1_p1
 Table "public.prt1_p1"
 Column |   Type| Modifiers | Storage  | Stats target |
Description
+---+---+--+--+-
 a  | integer   |   | plain|  |
 b  | integer   |   | plain|  |
 c  | character varying |   | extended |  |
Partition Of: prt1 FOR VALUES START (0) END (250)
Indexes:
"iprt1_p1_a" btree (a)

regression=# \d+ prt1_p2
 Table "public.prt1_p2"
 Column |   Type| Modifiers | Storage  | Stats target |
Description
+---+---+--+--+-
 a  | integer   |   | plain|  |
 b  | integer   |   | plain|  |
 c  | character varying |   | extended |  |
Partition Of: prt1 FOR VALUES START (250) END (500)
Indexes:
"iprt1_p2_a" btree (a)

regression=# \d+ prt1_p3
 Table "public.prt1_p3"
 Column |   Type| Modifiers | Storage  | Stats target |
Description
+---+---+--+--+-
 a  | integer   |   | plain|  |
 b  | integer   |   | plain|  |
 c  | character varying |   | extended |  |
Partition Of: prt1 FOR VALUES START (500) END (600)
Indexes:
"iprt1_p3_a" btree (a)

As you will observe that the table prt1 can not have any row with a < 0 and
a > 600. But when I execute

regression=# explain verbose select * from prt1 where a > 100;
QUERY PLAN
--
 Append  (cost=0.00..0.00 rows=1 width=40)
   ->  Seq Scan on public.prt1  (cost=0.00..0.00 rows=1 width=40)
 Output: prt1.a, prt1.b, prt1.c
 Filter: (prt1.a > 100)
(4 rows)

it correctly excluded all the partitions, but did not exclude the parent
relation. I guess, we have enough information to exclude it. Probably, we
should add a check constraint on the parent which is OR of the check
constraints on all the partitions. So there are two problems here

1. \d+ doesn't show partitions - this is probably reported earlier, I don't
remember.
2. A combination of constraints on the partitions should be applicable to
the parent. We aren't doing that.


On Wed, Aug 10, 2016 at 4:39 PM, Amit Langote  wrote:

> Hi,
>
> Attached is the latest set of patches to implement declarative
> partitioning.  There is already a commitfest entry for the same:
> https://commitfest.postgresql.org/10/611/
>
> The old discussion is here:
> http://www.postgresql.org/message-id/55d3093c.5010...@lab.ntt.co.jp/
>
> Attached patches are described below:
>
> 0001-Catalog-and-DDL-for-partition-key.patch
> 0002-psql-and-pg_dump-support-for-partitioned-tables.patch
>
> These patches create the infrastructure and DDL for partitioned
> tables.
>
> In addition to a catalog for storing the partition key information, this
> adds a new relkind to pg_class.h. PARTITION BY clause is added to CREATE
> TABLE. Tables so created are RELKIND_PARTITIONED_REL relations which are
> to be spec

Re: [HACKERS] Hash Indexes

[Amit Kapila]

On Thu, Sep 1, 2016 at 11:33 PM, Jesper Pedersen
 wrote:
> On 08/05/2016 07:36 AM, Amit Kapila wrote:

>
> Needs a rebase.
>

Done.

>
> +   if (blkno == P_NEW)
> +   elog(ERROR, "hash AM does not use P_NEW");
>
> Left over ?
>

No.  We need this check similar to all other _hash_*buf API's, as we
never expect caller of those API's to pass P_NEW.  The new buckets
(blocks) are created during split and it uses different mechanism to
allocate blocks in bulk.

I have fixed all other issues you have raised.  Updated patch is
attached with this mail.

>
> Ran some tests on a CHAR() based column which showed good results. Will have
> to compare with a run with the WAL patch applied.
>

Okay, Thanks for testing.  I think WAL patch is still not ready for
performance testing, I am fixing few issues in that patch, but you can
do the design or code level review of that patch at this stage.  I
think it is fine even if you share the performance numbers with this
and or Mithun's patch [1].


[1] - https://commitfest.postgresql.org/10/715/

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


concurrent_hash_index_v5.patch
Description: Binary data

-- 
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] less expensive pg_buffercache on big shmem

[Peter Geoghegan]

On Thu, Sep 1, 2016 at 8:19 PM, Andres Freund  wrote:
> On 2016-09-02 08:31:42 +0530, Robert Haas wrote:
>> I wonder whether we ought to just switch from the consistent method to
>> the semiconsistent method and call it good.
>
> +1. I think, before long, we're going to have to switch away from having
> locks & partitions in the first place. So I don't see a problem relaxing
> this. It's not like that consistency really buys you anything...  I'd
> even consider not using any locks.

Right. ISTM that the consistency guarantee was added on the off chance
that it mattered, without actually being justified. I would like to be
able to run pg_buffercache in production from time to time.


-- 
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] less expensive pg_buffercache on big shmem

[Andres Freund]

On 2016-09-02 08:31:42 +0530, Robert Haas wrote:
> I wonder whether we ought to just switch from the consistent method to
> the semiconsistent method and call it good.

+1. I think, before long, we're going to have to switch away from having
locks & partitions in the first place. So I don't see a problem relaxing
this. It's not like that consistency really buys you anything...  I'd
even consider not using any locks.

Andres


-- 
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] less expensive pg_buffercache on big shmem

[Robert Haas]

On Wed, Aug 31, 2016 at 8:27 PM, Ivan Kartyshov
 wrote:
> Recently I have finished my work on a patch for pg_buffercache contrib, I
> think it's time to share my results.

Thanks for sharing your results.

> V1.2 implementation contains flexible loop which can collect shared memory
> statistic using three different methods:
> 1) with holding LWLock only on one partition of shared memory
> (semiconsistent method)
> 2) without LWLocks (nonconsistent method),
> 3) or in vanilia way (consistent method)
>
> The aforementioned allow us to launch pg_buffercache in the three different
> ways.

I am a little skeptical about the idea of offering the user three
different choices about how to do this.  That seems like it is
exposing what ought to be an internal implementation detail and I'm
not sure users will really know how to make an intelligent choice
between the three options.  But of course others may have a different
view on that.

I wonder whether we ought to just switch from the consistent method to
the semiconsistent method and call it good.  I agree with you that
taking every buffer partition lock simultaneously seems like too much
locking.  And in the future if we replace the buffer mapping hash with
something that is lock-free or close to it, then we wouldn't even have
buffer partition locks any more, and probably no way at all to get an
entirely consistent snapshot.

What do you think of this?

-- 
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] pgbench - allow to store select results into variables

[Amit Langote]

Hi Fabien,

On 2016/07/16 1:33, Fabien COELHO wrote:
> Here is a v2 with more or less this approach, although \into does not end
> the query, but applies to the current or last sql command. A query is
> still terminated with a ";".

This patch needs to be rebased because of commit 64710452 (on 2016-08-19).

Thanks,
Amit




-- 
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] Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

[Robert Haas]

On Mon, Aug 1, 2016 at 5:44 PM, Etsuro Fujita
 wrote:
> I noticed that the following note about direct modification via
> GetForeignUpperPaths in fdwhandler.sgml is a bit confusing.  We have another
> approach using PlanDirectModify, so that should be reflected in the note as
> well.  Please find attached a patch.
>
>  PlanForeignModify and the other callbacks described in
>   are designed around the
> assumption
>  that the foreign relation will be scanned in the usual way and then
>  individual row updates will be driven by a local
> ModifyTable
>  plan node.  This approach is necessary for the general case where an
>  update requires reading local tables as well as foreign tables.
>  However, if the operation could be executed entirely by the foreign
>  server, the FDW could generate a path representing that and insert it
>  into the UPPERREL_FINAL upper relation, where it would
>  compete against the ModifyTable approach.

I suppose this is factually correct, but I don't think it's very
illuminating.  I think that if we're going to document both
approaches, there should be some discussion of the pros and cons of
PlanDirectModify vs. PlanForeignModify.   Of course either should be
better than an iterative ModifyTable, but how should the FDW author
decide between the two of them?

-- 
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] [PATCH] COPY vs \copy HINT

[Craig Ringer]

On 2 September 2016 at 04:28, Tom Lane  wrote:
> Craig Ringer  writes:
>> On 12 August 2016 at 16:34, Christoph Berg  wrote:
>>> Also, I vaguely get what you wanted to say with "a driver ...
>>> wrapper", but it's pretty nonsensical if one doesn't know about the
>>> protocol details. I don't have a better suggestion now, but I think it
>>> needs rephrasing.
>
>> I don't like it either, but didn't come up with anything better. The
>> problem is that every driver calls it something different.
>
> A few thoughts on this patch:

Thanks for the review. I'll leave the first change pending your
comments, the others are made, though I'm not completely sure we
should restrict it to ENOENT.

> 1. I don't really think the HINT is appropriate for the not-absolute-path
> case.

Why? If the user runs

# COPY sometable FROM 'localfile.csv' WITH (FORMAT CSV);
ERROR:  could not open file "localfile.csv" for reading: No such file
or directory

they're going to be just as confused by this error as by:

# COPY batch_demo FROM '/tmp/localfile.csv' WITH (FORMAT CSV);
ERROR:  could not open file "/tmp/localfile.csv" for reading: No such
file or directory

so I don't really see the rationale for this change.

> 2. I don't think it's appropriate for all possible cases of AllocateFile
> failure either, eg surely not for EACCES or similar situations where we
> did find a file.  Maybe print it only for ENOENT?  (See for example
> report_newlocale_failure() for technique.)

I thought about that but figured it didn't really matter too much,
when thinking about examples like

# COPY batch_demo FROM '/root/secret.csv' WITH (FORMAT CSV);
ERROR:  could not open file "/root/secret.csv" for reading: Permission denied

or whatever, where the user doesn't understand why they can't read the
file given that their local client has permission to do so.

I don't feel strongly about this and think that the error on ENOENT is
by far the most important, so I'll adjust it per your recommendation.

> 3. As for the wording, maybe you could do it like this:
>
> HINT:  COPY copies to[from] a file on the PostgreSQL server, not on the
> client.  You may want a client-side facility such as psql's \copy.
>
> That avoids trying to invent a name for other implementations.

I like that wording a lot more, thanks. Adopted.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From aa4f1fc810c8617d78dec75adad9951faf929909 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 12 Aug 2016 15:42:12 +0800
Subject: [PATCH] Emit a HINT when COPY can't find a file

Users often get confused between COPY and \copy and try to use client-side
paths with COPY. The server of course cannot find the file.

Emit a HINT in the most common cases to help users out.

Craig Ringer, review by Tom Lane and Christoph Berg
---
 src/backend/commands/copy.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f45b330..fe44bd9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1753,6 +1753,7 @@ BeginCopyTo(Relation rel,
 		{
 			mode_t		oumask; /* Pre-existing umask value */
 			struct stat st;
+			int			save_errno;
 
 			/*
 			 * Prevent write to relative path ... too easy to shoot oneself in
@@ -1761,16 +1762,23 @@ BeginCopyTo(Relation rel,
 			if (!is_absolute_path(filename))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_NAME),
-	  errmsg("relative path not allowed for COPY to file")));
+		 errmsg("relative path not allowed for COPY to file"),
+		 errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+ "You may want a client-side facility such as psql's \\copy.")));
 
 			oumask = umask(S_IWGRP | S_IWOTH);
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+			save_errno = errno;
 			umask(oumask);
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for writing: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno == ENOENT ?
+		  errhint("COPY copies to a file on the PostgreSQL server, not on the client. "
+  "You may want a client-side facility such as psql's \\copy.") : 0)));
 
 			if (fstat(fileno(cstate->copy_file), &st))
 ereport(ERROR,
@@ -2790,13 +2798,21 @@ BeginCopyFrom(Relation rel,
 		else
 		{
 			struct stat st;
+			int			save_errno;
 
 			cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+
+			/* in case something in ereport changes errno: */
+			save_errno = errno;
+
 			if (cstate->copy_file == NULL)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not open file \"%s\" for reading: %m",
-cstate->filename)));
+cstate->filename),
+		 (save_errno == ENOENT ?
+		  errhint("COPY copies from a file on the PostgreSQL ser

Re: [HACKERS] Exclude schema during pg_restore

[Peter Eisentraut]

On 8/31/16 4:10 AM, Michael Banck wrote:
> attached is a small patch that adds an -N option to pg_restore, in order
> to exclude a schema, in addition to -n for the restriction to a schema.

I think this is a good idea, and the approach looks sound.  However,
something doesn't work right.  If I take an empty database and dump it,
it will dump the plpgsql extension.  If I run pg_dump in plain-text mode
with -N, then the plpgsql extension is also dumped (since it is not in
the excluded schema).  But if I use the new pg_restore -N option, the
plpgsql extension is not dumped.  Maybe this is because it doesn't have
a schema, but I haven't checked.

pg_dump does not apply --strict-names to -N, but your patch for
pg_restore does that.  I think that should be made the same as pg_dump.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] restoration after crash slowness, any way to improve?

[Jeff Janes]

On Wed, Aug 31, 2016 at 6:26 PM, Joshua D. Drake 
wrote:

> -hackers,
>
> So this is more of a spit balling thread than anything. As I understand
> it, if we have a long running transaction or a large number of wal logs and
> we crash, we then have to restore those logs on restart to the last known
> good transaction. No problem.
>

It only has to replay from the start of the last successful checkpoint.  It
doesn't matter whether there was a long-running transaction or not.  If a
long transaction spans many checkpoints, replay still only has to go back
to the start of the last successful checkpoint.  Maybe you just had
checkpoint_segments or max_wal_size st way too high, assuming
checkpoint_timeout to always kick in instead and be the limiting factor.
But then your long-running transaction  invalidated that assumption?


> I recently ran a very long transaction. I was building up a large number
> of rows into a two column table to test index performance. I ended up
> having to kill the connection and thus the transaction after I realized I
> had an extra zero in my generate_series(). (Side note: Amazing the
> difference a single zero can make ;)). When coming back up, I watched the
> machine and I was averaging anywhere from 60MBs to 97MBs on writes.


Was it IO limited?

Killing a session/connection/transaction should not take down the entire
server, so there should be no recovery taking place in the first place.
Are you sure you are seeing recovery, and not just the vacuuming of the
aborted tuples?



> However, since I know this machine can get well over 400MBs when using
> multiple connections I can't help but wonder if there is anything we can do
> to make restoration more efficient without sacrificing the purpose of what
> it is doing?
>
> Can we have multiple readers pull transaction logs into shared_buffers (on
> recovery only), sort the good transactions and then push them back to the
> walwriter or bgwriter to the pages?
>

I don't see how that could work.  Whether a page is consistent or not is
orthogonal to whether the transactions on that page have committed or
aborted.

There are two possibilities that I've considered though for long-running
PITR, which could also apply to crash recovery, and which I think have been
discussed here before.  One is to have a leading recovery process which
would identify pages which will be recovered from a FPI, and send word back
to the lagging process not to bother applying incremental WAL to those
pages.  The other would be for a leading process to asynchronously read
into memory (either FS cache or shared_buffers) pages which it sees the
lagging process will need to write to.

In the first case, you would want the leading process to be leading by a
lot, so that it has the broadest scope to detect FPI.  Basically you would
want it to read all the way to the end of the replay, provided it had
enough memory to store the list of FPI pages.  For the second one, you
would not want it to run so far ahead that it the pages it read in would
get pushed out again before the lagging process got to them.  Controlling
how far ahead that would be seems like it would be hard.

Cheers,

Jeff

Re: [HACKERS] [PATCH] OpenSSL 1.1.0 support

[Tom Lane]

Kurt Roeckx  writes:
> Attached is a patch to make it build with OpenSSL 1.1.0.

Hi Kurt,

There's already been some work on this topic:
https://www.postgresql.org/message-id/flat/20160627151604.gd1...@msg.df7cb.de

Maybe you should join forces with Andreas to get it finished.

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] Missing checks when malloc returns NULL...

[Michael Paquier]

On Thu, Sep 1, 2016 at 11:16 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane  wrote:
>>> Don't see the point really ... it's just more API churn without any
>>> very compelling reason.
>
>> OK, then no objection to your approach. At least I tried.
>
> OK, pushed my version then.  I think we have now dealt with everything
> mentioned in this thread except for the issues in pg_locale.c.  Are
> you planning to tackle that?

Yes, not for this CF though. So I have switched the CF entry as committed.
-- 
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] CommitFest 2016-09

[Fabrízio de Royes Mello]

On Thu, Sep 1, 2016 at 5:11 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
> Please check the "needs reviewer" list
> (https://commitfest.postgresql.org/9/?reviewer=-2) for patches to
> review.  The committers need our help to work.
>

Just to fix the correct link bellow sent in my previous e-mail

https://commitfest.postgresql.org/10/?reviewer=-2

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

[HACKERS] [PATCH] OpenSSL 1.1.0 support

[Kurt Roeckx]

Hi,

Attached is a patch to make it build with OpenSSL 1.1.0.

There is probably a minor problem on windows where the name of the
dlls got changed.  Someone probably should look into that.


Kurt

>From efd7aa3499b2b4eedd4c4d4164b75175f3c10d2f Mon Sep 17 00:00:00 2001
From: Kurt Roeckx 
Date: Thu, 1 Sep 2016 23:24:07 +0200
Subject: [PATCH] Add support for OpenSSL 1.1.0

---
 configure| 68 +++-
 configure.in |  4 +-
 src/backend/libpq/be-secure-openssl.c| 49 ++-
 src/interfaces/libpq/fe-secure-openssl.c | 54 -
 4 files changed, 127 insertions(+), 48 deletions(-)

diff --git a/configure b/configure
index 45c8eef..930da6e 100755
--- a/configure
+++ b/configure
@@ -774,6 +774,7 @@ infodir
 docdir
 oldincludedir
 includedir
+runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -896,6 +897,7 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -1148,6 +1150,15 @@ do
   | -silent | --silent | --silen | --sile | --sil)
 silent=yes ;;
 
+  -runstatedir | --runstatedir | --runstatedi | --runstated \
+  | --runstate | --runstat | --runsta | --runst | --runs \
+  | --run | --ru | --r)
+ac_prev=runstatedir ;;
+  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+runstatedir=$ac_optarg ;;
+
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
 ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1285,7 +1296,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir
+		libdir localedir mandir runstatedir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1438,6 +1449,7 @@ Fine tuning of the installation directories:
   --sysconfdir=DIRread-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIRmodifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR modifiable single-machine data [PREFIX/var]
+  --runstatedir=DIR   modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIRobject code libraries [EPREFIX/lib]
   --includedir=DIRC header files [PREFIX/include]
   --oldincludedir=DIR C header files for non-gcc [/usr/include]
@@ -9538,9 +9550,9 @@ else
   as_fn_error $? "library 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_library_init in -lssl" >&5
-$as_echo_n "checking for SSL_library_init in -lssl... " >&6; }
-if ${ac_cv_lib_ssl_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_CTX_new in -lssl" >&5
+$as_echo_n "checking for SSL_CTX_new in -lssl... " >&6; }
+if ${ac_cv_lib_ssl_SSL_CTX_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_check_lib_save_LIBS=$LIBS
@@ -9554,27 +9566,27 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_CTX_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_CTX_new ();
   ;
   return 0;
 }
 _ACEOF
 if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_lib_ssl_SSL_library_init=yes
+  ac_cv_lib_ssl_SSL_CTX_new=yes
 else
-  ac_cv_lib_ssl_SSL_library_init=no
+  ac_cv_lib_ssl_SSL_CTX_new=no
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext conftest.$ac_ext
 LIBS=$ac_check_lib_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_library_init" >&5
-$as_echo "$ac_cv_lib_ssl_SSL_library_init" >&6; }
-if test "x$ac_cv_lib_ssl_SSL_library_init" = xyes; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_CTX_new" >&5
+$as_echo "$ac_cv_lib_ssl_SSL_CTX_new" >&6; }
+if test "x$ac_cv_lib_ssl_SSL_CTX_new" = xyes; then :
   cat >>confdefs.h <<_ACEOF
 #define HAVE_LIBSSL 1
 _ACEOF
@@ -9644,9 +9656,9 @@ else
   as_fn_error $? "library 'eay32' or 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_library_init" >&5
-$as_echo_n "checking for library containing SSL_library_init... " >&6; }
-if ${ac_cv_search_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_CTX_new" >&5
+$as_echo_n "checking for library containing SSL_CTX_new... " >&6; }
+if ${ac_cv_search_SSL_CTX_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_func_search_save_LIBS=$LIBS
@@ -9659,11 +9671,11 @@ cat confdefs.h -

Re: [HACKERS] Improve BEGIN tab completion

[Kevin Grittner]

On Wed, May 18, 2016 at 8:59 AM, Andreas Karlsson  wrote:

> I noticed that the tab completion was not aware of that TRANSACTION/WORK is
> optional in BEGIN, and that we do not complete [NOT] DEFERRABLE.
>
> While fixing it I also improved the completion support for SET SESSION
> slightly.

Pushed, with an adjustment to handle SET TRANSACTION SNAPSHOT, too.

Thanks!

-- 
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] incomplete removal of not referenced CTEs

[Tomas Vondra]

On 09/01/2016 09:58 PM, Andres Freund wrote:
> On 2016-09-01 15:46:45 -0400, Tom Lane wrote:
>> Tomas Vondra  writes:
>>> While investigating a CTE-related query, I've noticed that we don't
>>> really remove all unreachable CTEs.
>>
>> We expend a grand total of three lines of code on making that happen.
>> I'm pretty much -1 on adding a great deal more code or complexity
>> to make it happen recursively;
> 
> Agreed. And the consequences are pretty much harmless.
> 

I'm not sure I agree with that. The trouble is that we have customers,
and they don't always write "reasonably well written queries",
particularly when those queries are a bit more complex. And I have to
debug issues with those queries from time to time.

I'd actually be much happier if we haven't removed any CTEs at all,
instead of removing just some of them.

This incomplete CTE removal actually confused the hell out of me today,
which is why I started this thread. I only realized some of the CTEs are
useless after the EXPLAIN ANALYZE completed (as this was a performance
issue, it took a very long time).

> 
>> the case simply doesn't arise in reasonably well written queries.
> 
> Well, it might, when the CTE reference can be removed due to some other
> part of the query (e.g. plan time evaluation of immutable function).
> 

We also never remove CTEs with INSERT/UPDATE/DELETE commands, which
makes it a bit more complicated (but those are easy to spot).

FWIW, I'm not planning to hack on this, but I was thinking that this
might be a nice topic for a first patch for new PostgreSQL hackers
(something like EasyHacks from LibreOffice).

regards


-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] LOCK TABLE .. DEFERRABLE

[Simon Riggs]

On 5 April 2016 at 18:34, Rod Taylor  wrote:
>
>
> On Tue, Apr 5, 2016 at 1:10 PM, Simon Riggs  wrote:
>>>
>>> If a lock is successfully obtained on one table, but not on all tables,
>>> it releases that lock and will retry to get them as a group in the future.
>>> Since inheritance acts as a group of tables (top + recursive cascade to
>>> children), this implementation is necessary even if only a single table is
>>> specified in the command.
>>
>>
>> I'd prefer to see this as a lock wait mode where it sits in the normal
>> lock queue BUT other lock requestors are allowed to queue jump past it. That
>> should be just a few lines changed in the lock conflict checker and some
>> sleight of hand in the lock queue code.
>>
>> That way we avoid the busy-wait loop and multiple DEFERRABLE lock waiters
>> queue up normally.
>
>
> Yeah, that would be better. I can see how to handle a single structure in
> that way but I'm not at all certain how to handle multiple tables and
> inheritance is multiple tables even with a single command.

Agreed; neither can I.

> X1 inherits from X
>
> There is a long-running task on X1.
>
> Someone requests LOCK TABLE X IN ACCESS EXCLUSIVE MODE WAIT PATIENTLY.
> Internally this also grabs X1.
>
> The lock on X might be granted immediately and now blocks all other access
> to that table.
>
> There would need be a Locking Group kind of thing so various LockTags are
> treated as a single entity to grant them simultaneously. That seems pretty
> invasive; at least I don't see anything like that today.

Multiple locktags would likely be behind different LWLocks anyway, so
I don't see a way to make that work.

So the only way to handle multiple locks is to do this roughly the way
Rod suggests.

The only thing I would add at this stage is that if one lock is
unavailable, unlocking all previous locks is unnecessary. We only need
to unlock if there are lock waiters for the locks we already hold.

The use cases I am thinking of require only one table at a time, so
I'm still inclined towards the non-looping approach.

Thoughts?

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] COPY vs \copy HINT

[Tom Lane]

Craig Ringer  writes:
> On 12 August 2016 at 16:34, Christoph Berg  wrote:
>> Also, I vaguely get what you wanted to say with "a driver ...
>> wrapper", but it's pretty nonsensical if one doesn't know about the
>> protocol details. I don't have a better suggestion now, but I think it
>> needs rephrasing.

> I don't like it either, but didn't come up with anything better. The
> problem is that every driver calls it something different.

A few thoughts on this patch:

1. I don't really think the HINT is appropriate for the not-absolute-path
case.

2. I don't think it's appropriate for all possible cases of AllocateFile
failure either, eg surely not for EACCES or similar situations where we
did find a file.  Maybe print it only for ENOENT?  (See for example
report_newlocale_failure() for technique.)

3. As for the wording, maybe you could do it like this:

HINT:  COPY copies to[from] a file on the PostgreSQL server, not on the
client.  You may want a client-side facility such as psql's \copy.

That avoids trying to invent a name for other implementations.

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] System load consideration before spawning parallel workers

[Tom Lane]

Gavin Flower  writes:
> On 02/09/16 04:44, Peter Eisentraut wrote:
>> You can try this out by building PostgreSQL this way.  Please save your
>> work first, because you might have to hard-reboot your system.

> Hmm...  I've built several versions of pg this way, without any obvious 
> problems!

I'm a little skeptical of that too.  However, I'd note that with a "make"
you're not likely to care, or possibly even notice, if the thing does
something like go completely to sleep for a little while, or if some
sub-jobs proceed well while others do not.  The fact that "-l 8" works
okay for make doesn't necessarily translate to more-interactive use cases.

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

[HACKERS] CommitFest 2016-09

[Fabrízio de Royes Mello]

Hi all,

The 2016-09 commitfest is officially in progress, and I'm the manager.

The current status summary is

Needs review: 119
Needs *reviewer*: 63

Please check the "needs reviewer" list
(https://commitfest.postgresql.org/10/?reviewer=-2
) for patches to
review.  The committers need our help to work.

If this is you:

Waiting on Author: 15

Then please post an updated patch as soon as possible so it can be
reviewed.  Some of these patches have not seen any activity from the
author in a long time.

The good news is we are already 29% done with the CF:

Committed: 57
Rejected: 6
Returned with Feedback: 4

TOTAL: 219

I'll post a status update on this thread at least once a week and more
often as needed.

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] System load consideration before spawning parallel workers

[Gavin Flower]

On 02/09/16 05:01, Peter Eisentraut wrote:

On 8/16/16 3:39 AM, Haribabu Kommi wrote:

[...]


All of this seems very platform specific, too.  You have
Windows-specific code, but the rest seems very Linux-specific.  The
dstat tool I had never heard of before.  There is stuff with cgroups,
which I don't know how portable they are across different Linux
installations.  Something about Solaris was mentioned.  What about the
rest?  How can we maintain this in the long term?  How do we know that
these facilities actually work correctly and not cause mysterious problems?

[...]
I think that we should not hobble pg in Linux, because of limitations of 
other O/S's like those from Microsoft!


On the safe side, if a feature has insufficient evidence of working in a 
particular O/S, then it should not be default enabled for that O/S.


If a feature is useful in Linux, but not elsewhere: then pg should still 
run in the other O/S's but the documentation should reflect that.



Cheers,.
Gavin


--
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] Speedup twophase transactions

[Simon Riggs]

On 13 April 2016 at 15:31, Stas Kelvich  wrote:

> Fixed patch attached. There already was infrastructure to skip currently
> held locks during replay of standby_redo() and I’ve extended that with check 
> for
> prepared xids.

Please confirm that everything still works on current HEAD for the new
CF, so review can start.

Thanks

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] incomplete removal of not referenced CTEs

[Andres Freund]

On 2016-09-01 15:46:45 -0400, Tom Lane wrote:
> Tomas Vondra  writes:
> > While investigating a CTE-related query, I've noticed that we don't
> > really remove all unreachable CTEs.
> 
> We expend a grand total of three lines of code on making that happen.
> I'm pretty much -1 on adding a great deal more code or complexity
> to make it happen recursively;

Agreed. And the consequences are pretty much harmless.


> the case simply doesn't arise in reasonably well written queries.

Well, it might, when the CTE reference can be removed due to some other
part of the query (e.g. plan time evaluation of immutable function).


Andres


-- 
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] System load consideration before spawning parallel workers

[Gavin Flower]

On 02/09/16 04:44, Peter Eisentraut wrote:

On 8/1/16 2:17 AM, Gavin Flower wrote:

Possibly look how make does it with the '-l' flag?

'-l 8' don't start more process when load is 8 or greater, works on
Linux at least...

The problem with that approach is that it takes about a minute for the
load averages figures to be updated, by which time you have already
thrashed your system.

You can try this out by building PostgreSQL this way.  Please save your
work first, because you might have to hard-reboot your system.

Hmm...  I've built several versions of pg this way, without any obvious 
problems!


Looking at top, suggests that the load averages never go much above 8, 
and are usually less.


This is the bash script I use:


#!/bin/bash
# postgresql-build.sh


VERSION='9.5.0'

TAR_FILE="postgresql-$VERSION.tar.bz2"
echo 'TAR_FILE['$TAR_FILE']'
tar xvf $TAR_FILE

PORT='--with-pgport=5433'   std is 5432

BASE_DIR="postgresql-$VERSION"
echo 'BASE_DIR['$BASE_DIR']'
cd $BASE_DIR

PREFIX="--prefix=/usr/local/lib/postgres-$VERSION"
echo 'PREFIX['$PREFIX']'

LANGUAGES='--with-python'
echo 'LANGUAGES['$LANGUAGES']'

SECURITY='--with-openssl --with-pam --with-ldap'
echo 'PREFIX['$PREFIX']'

XML='--with-libxml --with-libxslt'
echo 'SECURITY['$SECURITY']'

TZDATA='--with-system-tzdata=/usr/share/zoneinfo'
echo 'TZDATA['$TZDATA']'

##DEBUG='--enable-debug'
##echo 'DEBUG['$DEBUG']'


./configure $PREFIX $LANGUAGES $SECURITY $XML $TZDATA $DEBUG

time make -j7 -l8 && time make -j7 -l8 check


Cheers,
Gavin



--
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] incomplete removal of not referenced CTEs

[Tom Lane]

Tomas Vondra  writes:
> While investigating a CTE-related query, I've noticed that we don't
> really remove all unreachable CTEs.

We expend a grand total of three lines of code on making that happen.
I'm pretty much -1 on adding a great deal more code or complexity
to make it happen recursively; the case simply doesn't arise in
reasonably well written queries.

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] incomplete removal of not referenced CTEs

[Andres Freund]

On 2016-09-01 21:36:13 +0200, Tomas Vondra wrote:
> Of course, it's harmless as none of those CTEs gets actually executed,
> but is this intentional, or do we want/need to fix it? I don't see
> anything about this in the docs, but it seems a bit awkward and
> confusing to remove only some of the CTEs - I think we should either
> remove all or none of them.
> 
> I don't think that should be particularly difficult - ISTM we need to
> make SS_process_ctes a bit smarter, essentially by adding a loop to
> remove the CTEs recursively (and decrease the refcount).

I don't really see a lot of benefit in expanding energy on
this. Skipping the CTE in the easy case saves som eplan cycles. Making more
effort to remove CTEs recursively probably doesn't...

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] \timing interval

[Tom Lane]

Peter van Hardenberg  writes:
> Some kind of units on the parenthetical format would be helpful.

I was really hoping to not re-open that can of worms :-(

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

[HACKERS] incomplete removal of not referenced CTEs

[Tomas Vondra]

Hi,

While investigating a CTE-related query, I've noticed that we don't
really remove all unreachable CTEs. For example, for this query

with a as (select 1),
 b as (select * from a),
 c as (select * from b)
select 2;

where none of the CTEs if (directly or indirectly) referenced from the
query, we get a plan like this:

   QUERY PLAN
-
 Result  (cost=0.03..0.04 rows=1 width=4)
   CTE a
 ->  Result  (cost=0.00..0.01 rows=1 width=4)
   CTE b
 ->  CTE Scan on a  (cost=0.00..0.02 rows=1 width=4)
(5 rows)

So we only remove the top-level CTE, but we fail to remove the other
CTEs because we don't tweak the refcount in SS_process_ctes().

Of course, it's harmless as none of those CTEs gets actually executed,
but is this intentional, or do we want/need to fix it? I don't see
anything about this in the docs, but it seems a bit awkward and
confusing to remove only some of the CTEs - I think we should either
remove all or none of them.

I don't think that should be particularly difficult - ISTM we need to
make SS_process_ctes a bit smarter, essentially by adding a loop to
remove the CTEs recursively (and decrease the refcount).

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] \timing interval

[Peter van Hardenberg]

On Thu, Sep 1, 2016 at 12:14 PM, Tom Lane  wrote:

> Corey Huinker  writes:
> > On Thu, Sep 1, 2016 at 3:01 PM, Tom Lane  wrote:
> >> Well, that code's on the backend side so we're not going to just call it
> >> in any case.  And I think we don't want to be quite so verbose as to go
> up
> >> to hh:mm:ss.fff as soon as we get past 1 second.  However, comparing
> that
> >> output to what I had suggests that maybe it's better to keep a leading
> >> zero in two-digit fields, that is render times like "00:01.234",
> >> "01:23.456", or "01:23:45.678" rather than suppressing the initial zero
> as
> >> I had in my examples.  It's an extra character but I think it reinforces
> >> the meaning.
>
> > +1
> > The larger jump in widths from no MM:SS to HH:MM:SS is a good visual cue.
> > Jumping from MM:SS to H:MM:SS to HH:MM:SS would be more subtle and
> possibly
> > confusing.
>
> Attached is an updated patch that does it like that.  Sample output
> (generated by forcing specific arguments to PrintTiming):
>
> Time: 0.100 ms
> Time: 1.200 ms
> Time: 1001.200 ms (00:01.001)
> Time: 12001.200 ms (00:12.001)
> Time: 60001.200 ms (01:00.001)
> Time: 720001.200 ms (12:00.001)
> Time: 3660001.200 ms (01:01:00.001)
> Time: 43920001.200 ms (12:12:00.001)
> Time: 176460001.200 ms (2 01:01:00.001)
> Time: 216720001.200 ms (2 12:12:00.001)
> Time: 8816460001.200 ms (102 01:01:00.001)
> Time: 8856720001.200 ms (102 12:12:00.001)
>
> Barring objections I'll commit this soon.
>
> regards, tom lane
>


Some kind of units on the parenthetical format would be helpful. Glancing
at several of these values it takes me a couple of seconds to decide what
I'm reading.

-- 
Peter van Hardenberg
San Francisco, California
"Everything was beautiful, and nothing hurt."—Kurt Vonnegut

Re: [HACKERS] \timing interval

[Tom Lane]

Corey Huinker  writes:
> On Thu, Sep 1, 2016 at 3:01 PM, Tom Lane  wrote:
>> Well, that code's on the backend side so we're not going to just call it
>> in any case.  And I think we don't want to be quite so verbose as to go up
>> to hh:mm:ss.fff as soon as we get past 1 second.  However, comparing that
>> output to what I had suggests that maybe it's better to keep a leading
>> zero in two-digit fields, that is render times like "00:01.234",
>> "01:23.456", or "01:23:45.678" rather than suppressing the initial zero as
>> I had in my examples.  It's an extra character but I think it reinforces
>> the meaning.

> +1
> The larger jump in widths from no MM:SS to HH:MM:SS is a good visual cue.
> Jumping from MM:SS to H:MM:SS to HH:MM:SS would be more subtle and possibly
> confusing.

Attached is an updated patch that does it like that.  Sample output
(generated by forcing specific arguments to PrintTiming):

Time: 0.100 ms
Time: 1.200 ms
Time: 1001.200 ms (00:01.001)
Time: 12001.200 ms (00:12.001)
Time: 60001.200 ms (01:00.001)
Time: 720001.200 ms (12:00.001)
Time: 3660001.200 ms (01:01:00.001)
Time: 43920001.200 ms (12:12:00.001)
Time: 176460001.200 ms (2 01:01:00.001)
Time: 216720001.200 ms (2 12:12:00.001)
Time: 8816460001.200 ms (102 01:01:00.001)
Time: 8856720001.200 ms (102 12:12:00.001)

Barring objections I'll commit this soon.

regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a66ce7..88e2f66 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=> \setenv LESS -imx
*** 2789,2796 
 \timing [ on | off ]
  
  
!  Without parameter, toggles a display of how long each SQL statement
!  takes, in milliseconds.  With parameter, sets same.
  
 

--- 2789,2798 
 \timing [ on | off ]
  
  
!  With a parameter, turns displaying of how long each SQL statement
!  takes on or off.  Without a parameter, toggles the display between
!  on and off.  The display is in milliseconds; intervals longer than
!  1 second are also shown in days/hours/minutes/seconds format.
  
 

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 7399950..a17f1de 100644
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 10,15 
--- 10,16 
  
  #include 
  #include 
+ #include 
  #include 
  #ifndef WIN32
  #include /* for write() */
*** ClearOrSaveResult(PGresult *result)
*** 530,535 
--- 531,586 
  	}
  }
  
+ /*
+  * Print microtiming output.  Always print raw milliseconds; if the interval
+  * is >= 1 second, also break it down into days/hours/minutes/seconds.
+  */
+ static void
+ PrintTiming(double elapsed_msec)
+ {
+ 	double		seconds;
+ 	double		minutes;
+ 	double		hours;
+ 	double		days;
+ 
+ 	if (elapsed_msec < 1000.0)
+ 	{
+ 		/* This is the traditional (pre-v10) output format */
+ 		printf(_("Time: %.3f ms\n"), elapsed_msec);
+ 		return;
+ 	}
+ 
+ 	/*
+ 	 * Note: we could print just seconds, in a format like %06.3f, when the
+ 	 * total is less than 1min.  But that's hard to interpret unless we tack
+ 	 * on "s" or otherwise annotate it.  Forcing the display to include
+ 	 * minutes seems like a better solution.
+ 	 */
+ 	seconds = elapsed_msec / 1000.0;
+ 	minutes = floor(seconds / 60.0);
+ 	seconds -= 60.0 * minutes;
+ 	if (minutes < 60.0)
+ 	{
+ 		printf(_("Time: %.3f ms (%02d:%06.3f)\n"),
+ 			   elapsed_msec, (int) minutes, seconds);
+ 		return;
+ 	}
+ 
+ 	hours = floor(minutes / 60.0);
+ 	minutes -= 60.0 * hours;
+ 	if (hours < 24.0)
+ 	{
+ 		printf(_("Time: %.3f ms (%02d:%02d:%06.3f)\n"),
+ 			   elapsed_msec, (int) hours, (int) minutes, seconds);
+ 		return;
+ 	}
+ 
+ 	days = floor(hours / 24.0);
+ 	hours -= 24.0 * days;
+ 	printf(_("Time: %.3f ms (%.0f %02d:%02d:%06.3f)\n"),
+ 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
+ }
+ 
  
  /*
   * PSQLexec
*** PSQLexecWatch(const char *query, const p
*** 679,685 
  
  	/* Possible microtiming output */
  	if (pset.timing)
! 		printf(_("Time: %.3f ms\n"), elapsed_msec);
  
  	return 1;
  }
--- 730,736 
  
  	/* Possible microtiming output */
  	if (pset.timing)
! 		PrintTiming(elapsed_msec);
  
  	return 1;
  }
*** SendQuery(const char *query)
*** 1332,1338 
  
  	/* Possible microtiming output */
  	if (pset.timing)
! 		printf(_("Time: %.3f ms\n"), elapsed_msec);
  
  	/* check for events that may occur during query execution */
  
--- 1383,1389 
  
  	/* Possible microtiming output */
  	if (pset.timing)
! 		PrintTiming(elapsed_msec);
  
  	/* check for events that may occur during query execution */
  

-- 
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] \timing interval

[Corey Huinker]

On Thu, Sep 1, 2016 at 3:01 PM, Tom Lane  wrote:

> Corey Huinker  writes:
> > On Thu, Sep 1, 2016 at 2:43 PM, Tom Lane  wrote:
> >> Note that times from 1 second to 1 hour all get the nn:nn.nnn
> >> treatment.  I experimented with these variants for sub-minute times:
> >> ...
> >> but it seems like the first variant is not terribly intelligible and
> >> the second variant is inconsistent with what happens for longer times.
>
> > Well, if we're looking to be consistent, here's what interval does now:
> > ...
> > Should we just plug into whatever code that uses?
>
> Well, that code's on the backend side so we're not going to just call it
> in any case.  And I think we don't want to be quite so verbose as to go up
> to hh:mm:ss.fff as soon as we get past 1 second.  However, comparing that
> output to what I had suggests that maybe it's better to keep a leading
> zero in two-digit fields, that is render times like "00:01.234",
> "01:23.456", or "01:23:45.678" rather than suppressing the initial zero as
> I had in my examples.  It's an extra character but I think it reinforces
> the meaning.
>
> regards, tom lane
>

+1
The larger jump in widths from no MM:SS to HH:MM:SS is a good visual cue.
Jumping from MM:SS to H:MM:SS to HH:MM:SS would be more subtle and possibly
confusing.

Re: [HACKERS] \timing interval

[Tom Lane]

Corey Huinker  writes:
> On Thu, Sep 1, 2016 at 2:43 PM, Tom Lane  wrote:
>> Note that times from 1 second to 1 hour all get the nn:nn.nnn
>> treatment.  I experimented with these variants for sub-minute times:
>> ...
>> but it seems like the first variant is not terribly intelligible and
>> the second variant is inconsistent with what happens for longer times.

> Well, if we're looking to be consistent, here's what interval does now:
> ...
> Should we just plug into whatever code that uses?

Well, that code's on the backend side so we're not going to just call it
in any case.  And I think we don't want to be quite so verbose as to go up
to hh:mm:ss.fff as soon as we get past 1 second.  However, comparing that
output to what I had suggests that maybe it's better to keep a leading
zero in two-digit fields, that is render times like "00:01.234",
"01:23.456", or "01:23:45.678" rather than suppressing the initial zero as
I had in my examples.  It's an extra character but I think it reinforces
the meaning.

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] \timing interval

[Corey Huinker]

On Thu, Sep 1, 2016 at 2:43 PM, Tom Lane  wrote:

> I wrote:
> > Sorry, that probably added no clarity at all, I was confusing
> > seconds with milliseconds in the example values :-(
>
> After a bit of further fooling with sample values, I propose this
> progression:
>
> Time: 0.100 ms
> Time: 1.200 ms
> Time: 1001.200 ms (0:01.001)
> Time: 12001.200 ms (0:12.001)
> Time: 60001.200 ms (1:00.001)
> Time: 720001.200 ms (12:00.001)
> Time: 3660001.200 ms (1:01:00.001)
> Time: 43920001.200 ms (12:12:00.001)
> Time: 176460001.200 ms (2 01:01:00.001)
> Time: 216720001.200 ms (2 12:12:00.001)
> Time: 1000.000 ms (115740740740 17:46:40.000)
>
> Note that times from 1 second to 1 hour all get the nn:nn.nnn
> treatment.  I experimented with these variants for sub-minute times:
>
> Time: 1001.200 ms (1.001)
> Time: 12001.200 ms (12.001)
> Time: 1001.200 ms (1.001 s)
> Time: 12001.200 ms (12.001 s)
>
> but it seems like the first variant is not terribly intelligible and
> the second variant is inconsistent with what happens for longer times.
> Adding a zero minutes field is a subtler way of cueing the reader that
> it's mm:ss.
>
> regards, tom lane
>

Well, if we're looking to be consistent, here's what interval does now:

# select '1 second 1 millisecond'::interval, '1 minute 2
milliseconds'::interval, '1 hour 30 milliseconds'::interval, '1 day 1 hour
999 milliseconds'::interval, '1 week 1 day 1 hour'::interval;
   interval   |   interval   |  interval   |  interval  |
 interval
--+--+-++-
 00:00:01.001 | 00:01:00.002 | 01:00:00.03 | 1 day 01:00:00.999 | 8 days
01:00:00
(1 row)


Should we just plug into whatever code that uses? It's slightly different:

# select interval '1000.001 milliseconds'::interval;
ERROR:  interval field value out of range: "1000.001
milliseconds"
LINE 1: select interval '1000.001 milliseconds'::int...
^
# select interval '216720001.200 milliseconds';
   interval
---
 60:12:00.0012
(1 row)

# select interval '176460001.200 ms';
   interval
---
 49:01:00.0012
(1 row)

Re: [HACKERS] \timing interval

[Tom Lane]

Corey Huinker  writes:
> I'm going to hold off a bit to see if anybody else chimes in, and if not
> I'm going to deliver a patch.

I've already been updating yours, no need for another.

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] \timing interval

[Tom Lane]

I wrote:
> Sorry, that probably added no clarity at all, I was confusing
> seconds with milliseconds in the example values :-(

After a bit of further fooling with sample values, I propose this
progression:

Time: 0.100 ms
Time: 1.200 ms
Time: 1001.200 ms (0:01.001)
Time: 12001.200 ms (0:12.001)
Time: 60001.200 ms (1:00.001)
Time: 720001.200 ms (12:00.001)
Time: 3660001.200 ms (1:01:00.001)
Time: 43920001.200 ms (12:12:00.001)
Time: 176460001.200 ms (2 01:01:00.001)
Time: 216720001.200 ms (2 12:12:00.001)
Time: 1000.000 ms (115740740740 17:46:40.000)

Note that times from 1 second to 1 hour all get the nn:nn.nnn
treatment.  I experimented with these variants for sub-minute times:

Time: 1001.200 ms (1.001)
Time: 12001.200 ms (12.001)
Time: 1001.200 ms (1.001 s)
Time: 12001.200 ms (12.001 s)

but it seems like the first variant is not terribly intelligible and
the second variant is inconsistent with what happens for longer times.
Adding a zero minutes field is a subtler way of cueing the reader that
it's mm:ss.

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] autonomous transactions

[Andres Freund]

On 2016-08-31 06:10:31 +0200, Joel Jacobson wrote:
> This is important if you as a caller function want to be sure none of
> the work made by anything called down the stack gets committed.
> That is, if you as a caller decide to rollback, e.g. by raising an
> exception, and you want to be sure *everything* gets rollbacked,
> including all work by functions you've called.

> If the caller can't control this, then the author of the caller
> function would need to inspect the source code of all function being
> called, to be sure there are no code using autonomous transactions.

I'm not convinced this makes much sense. All FDWs, dblink etc. already
allow you do stuff outside of a transaction.

Andres


-- 
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] \timing interval

[Corey Huinker]

On Thu, Sep 1, 2016 at 2:17 PM, Tom Lane  wrote:

> I wrote:
> > So for clarity's sake: first suitable format among these:
>
> > Time: 59.999 ms
> > Time: 121.999 ms (2:01.999)
> > Time: 10921.999 ms (3:02:01.999)
> > Time: 356521.999 ms (4 3:02:01.999)
>
> Sorry, that probably added no clarity at all, I was confusing
> seconds with milliseconds in the example values :-(
>
> regards, tom lane
>

I didn't scan your examples for correctness, but the parentheticals matched
my own idea for what "left-trimmed" meant.

I'm going to hold off a bit to see if anybody else chimes in, and if not
I'm going to deliver a patch.

Re: [HACKERS] autonomous transactions

[Joel Jacobson]

On Wed, Aug 31, 2016 at 6:22 PM, Simon Riggs  wrote:
> On 31 August 2016 at 14:09, Joel Jacobson  wrote:
>> My idea on how to deal with this would be to mark the function to be
>> "AUTONOMOUS" similar to how a function is marked to be "PARALLEL
>> SAFE",
>> and to throw an error if a caller that has blocked autonomous
>> transactions tries to call a function that is marked to be autonomous.
>>
>> That way none of the code that needs to be audited would ever get executed.
>
> Not sure I see why you would want to turn off execution for only some 
> functions.
>
> What happens if your function calls some other function with
> side-effects?

I'm not sure I understand your questions. All volatile functions modifying data
have side-effects. What I meant was if they are allowed to commit it
even if the caller doesn't want to.

However, I'll try to clarify the two scenarios I envision:

1. If a function is declared AUTONOMOUS and it gets called,
then that means nothing in the txn has blocked autonomous yet
and the function and any other function will be able to do autonomous txns
from that here on, so if some function would try to block autonomous that
would throw an error.

2. If a function has blocked autonomous and something later on
tries to call a function declared AUTONOMOUS then that would throw an error.

Basically, we start with a NULL state where autonomous is neither blocked
or explicitly allowed. Whatever happens first decides if autonomous transactions
will explicitly be blocked or allowed during the txn.

So we can go from NULL -> AUTONOMOUS ALLOWED
or NULL -> AUTONOMOUS BLOCKED,
but that's the only two state transitions possible.

Once set, it cannot be changed.

If nothing in an application cares about autonomous transactions,
they don't have to do anything special, they don't need to modify any
of their code.

But if it for some reason is important to block autonomous transactions
because the application is written in a way where it is expected
a RAISE EXCEPTION always rollbacks everything,
then the author of such an application (e.g. me) can just block
autonomous transactions
and continue to live happily ever after without having to dream nightmares about
developers misusing the feature, and only use it when appropriate.


-- 
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] autonomous transactions

[Joel Jacobson]

On Thu, Sep 1, 2016 at 12:12 AM, Vik Fearing  wrote:
> Part of what people want this for is to audit what people *try* to do.
> We can already audit what they've actually done.
>
> With your solution, we still wouldn't know when an unauthorized attempt
> to do something happened.

The unauthorized attempt to execute the function will still be logged
to the PostgreSQL log file
since it would throw an error, just like trying to connect with e.g.
an invalid username would be logged to the log files.
I think that's enough for that use-case, since it's arguably not an
application layer problem,
since no part of the code was ever executed.

But if someone tries to execute a function where one of the input params
is a password and the function raises an exception if the password
is incorrect and wants to log the unauthorized attempt, then that
would be a good example of when you could use and would need to use
autonomous transactions to log the invalid password attempt.


-- 
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] \timing interval

[Tom Lane]

I wrote:
> So for clarity's sake: first suitable format among these:

> Time: 59.999 ms
> Time: 121.999 ms (2:01.999)
> Time: 10921.999 ms (3:02:01.999)
> Time: 356521.999 ms (4 3:02:01.999)

Sorry, that probably added no clarity at all, I was confusing
seconds with milliseconds in the example values :-(

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] WAL consistency check facility

[Peter Geoghegan]

On Thu, Sep 1, 2016 at 9:23 AM, Robert Haas  wrote:
> Indeed, it had occurred to me that we might not even want to compile
> this code into the server unless WAL_DEBUG is defined; after all, how
> does it help a regular user to detect that the server has a bug?  Bug
> or no bug, that's the code they've got.  But on further reflection, it
> seems like it could be useful: if we suspect a bug in the redo code
> but we can't reproduce it here, we could ask the customer to turn this
> option on to see whether it produces logging indicating the nature of
> the problem.  However, because of the likely expensive of enabling the
> feature, it seems like it would be quite desirable to limit the
> expense of generating many extra FPWs to the affected rmgr.  For
> example, if a user has a table with a btree index and a gin index, and
> we suspect a bug in GIN, it would be nice for the user to be able to
> enable the feature *only for GIN* rather than paying the cost of
> enabling it for btree and heap as well.[2]

Yes, that would be rather a large advantage.

I think that there really is no hard distinction between users and
hackers. Some people will want to run this in production, and it would
be a lot better if performance was at least not atrocious. If amcheck
couldn't do the majority of its verification with only an
AccessShareLock, then users probably just couldn't use it. Heroku
wouldn't have been able to use it on all production databases. It
wouldn't have mattered that the verification was no less effective,
since the bugs it found would simply never have been observed in
practice.

-- 
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] Hash Indexes

[Jesper Pedersen]

On 08/05/2016 07:36 AM, Amit Kapila wrote:

On Thu, Aug 4, 2016 at 8:02 PM, Mithun Cy  wrote:

I did some basic testing of same. In that I found one issue with cursor.



Thanks for the testing.  The reason for failure was that the patch
didn't take into account the fact that for scrolling cursors, scan can
reacquire the lock and pin on bucket buffer multiple times.   I have
fixed it such that we release the pin on bucket buffers after we scan
the last overflow page in bucket. Attached patch fixes the issue for
me, let me know if you still see the issue.



Needs a rebase.

hashinsert.c

+* reuse the space.  There is no such apparent benefit from finsihing 
the

-> finishing

hashpage.c

+ * retrun the buffer, else return InvalidBuffer.

-> return

+   if (blkno == P_NEW)
+   elog(ERROR, "hash AM does not use P_NEW");

Left over ?

+ * for unlocking it.

-> for unlocking them.

hashsearch.c

+* bucket, but not pin, then acuire the lock on new bucket and again

-> acquire

hashutil.c

+ * half.  It is mainly required to finsh the incomplete splits where we are

-> finish

Ran some tests on a CHAR() based column which showed good results. Will 
have to compare with a run with the WAL patch applied.


make check-world passes.

Best regards,
 Jesper



--
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] GiST penalty functions [PoC]

[Andrew Borodin]

Here is new version of the patch.
Now function pack_float is commented better.
All inline keywords are removed. I haven't found any performance loss for that.
Remove of static's showed 1%-7% performance loss in SELECT computation
(3 test runs), so I left statics where they are.

Actually, to avoid this kind of hacks, probably, it would be better to
fork GiST to GiSTv2 and implement many API changes there:
1. Bulk load API
2. Richier choose subtree API
3. Allow for key test not just NO\MAYBE answers, but SURE answer to
skip key test for underlying tree
And some other improvements require breaking chanes for extensions.
GiST idea is almost 25, modern spatial indices vary a lot from things
that were there during 90th.

Best regards, Andrey Borodin, Octonica & Ural Federal University.
diff --git a/contrib/cube/cube.c b/contrib/cube/cube.c
index 3feddef..9ce3cd6 100644
--- a/contrib/cube/cube.c
+++ b/contrib/cube/cube.c
@@ -91,14 +91,16 @@ PG_FUNCTION_INFO_V1(cube_enlarge);
 /*
 ** For internal use only
 */
-int32  cube_cmp_v0(NDBOX *a, NDBOX *b);
-bool   cube_contains_v0(NDBOX *a, NDBOX *b);
-bool   cube_overlap_v0(NDBOX *a, NDBOX *b);
-NDBOX *cube_union_v0(NDBOX *a, NDBOX *b);
-void   rt_cube_size(NDBOX *a, double *sz);
-NDBOX *g_cube_binary_union(NDBOX *r1, NDBOX *r2, int *sizep);
-bool   g_cube_leaf_consistent(NDBOX *key, NDBOX *query, StrategyNumber 
strategy);
-bool   g_cube_internal_consistent(NDBOX *key, NDBOX *query, 
StrategyNumber strategy);
+static int32   cube_cmp_v0(NDBOX *a, NDBOX *b);
+static boolcube_contains_v0(NDBOX *a, NDBOX *b);
+static boolcube_overlap_v0(NDBOX *a, NDBOX *b);
+static NDBOX  *cube_union_v0(NDBOX *a, NDBOX *b);
+static float   pack_float(float actualValue, int realm);
+static voidrt_cube_size(NDBOX *a, double *sz);
+static voidrt_cube_edge(NDBOX *a, double *sz);
+static NDBOX  *g_cube_binary_union(NDBOX *r1, NDBOX *r2, int *sizep);
+static boolg_cube_leaf_consistent(NDBOX *key, NDBOX *query, 
StrategyNumber strategy);
+static boolg_cube_internal_consistent(NDBOX *key, NDBOX *query, 
StrategyNumber strategy);
 
 /*
 ** Auxiliary funxtions
@@ -419,7 +421,27 @@ g_cube_decompress(PG_FUNCTION_ARGS)
}
PG_RETURN_POINTER(entry);
 }
-
+/*
+ * Function to pack bit flags inside float type
+ * Resulted value represent can be from four different "realms"
+ * Every value from realm 3 is greater than any value from realms 2,1 and 0.
+ * Every value from realm 2 is less than every value from realm 3 and greater
+ * than any value from realm 1 and 0, and so on. Values from the same realm
+ * loose two bits of precision. This technique is possible due to floating
+ * point numbers specification according to IEEE 754: exponent bits are highest
+ * (excluding sign bits, but here penalty is always positive). If float a is
+ * greater than float b, integer A with same bit representation as a is greater
+ * than integer B with same bits as b.
+ */
+static float
+pack_float(float actualValue, int realm)
+{
+   /* two bits for realm, others for value */
+   /* we have 4 realms */
+   int realmAjustment = *((int*)&actualValue)/4;
+   int realCode = realm * (INT32_MAX/4) + realmAjustment;
+   return *((float*)&realCode);
+}
 
 /*
 ** The GiST Penalty method for boxes
@@ -428,6 +450,11 @@ g_cube_decompress(PG_FUNCTION_ARGS)
 Datum
 g_cube_penalty(PG_FUNCTION_ARGS)
 {
+   /* REALM 0: No extension is required, volume is zero, return edge   
*/
+   /* REALM 1: No extension is required, return nonzero volume 
*/
+   /* REALM 2: Volume extension is zero, return nonzero edge extension 
*/
+   /* REALM 3: Volume extension is nonzero, return it  
*/
+
GISTENTRY  *origentry = (GISTENTRY *) PG_GETARG_POINTER(0);
GISTENTRY  *newentry = (GISTENTRY *) PG_GETARG_POINTER(1);
float  *result = (float *) PG_GETARG_POINTER(2);
@@ -441,9 +468,33 @@ g_cube_penalty(PG_FUNCTION_ARGS)
rt_cube_size(DatumGetNDBOX(origentry->key), &tmp2);
*result = (float) (tmp1 - tmp2);
 
-   /*
-* fprintf(stderr, "penalty\n"); fprintf(stderr, "\t%g\n", *result);
-*/
+   if( *result == 0 )
+   {
+   double tmp3 = tmp1; /* remember entry volume */
+   rt_cube_edge(ud, &tmp1);
+   rt_cube_edge(DatumGetNDBOX(origentry->key), &tmp2);
+   *result = (float) (tmp1 - tmp2);
+   if( *result == 0 )
+   {
+   if( tmp3 != 0 )
+   {
+   *result = pack_float(tmp3, 1); /* REALM 1 */
+   }
+   else
+   {
+   *result = pack_float(tmp1, 0); /* REALM 0 */
+  

Re: [HACKERS] \timing interval

[Tom Lane]

[ This patch is marked Ready For Committer, and discussion seems to have
died off, so let's get on with committing something ... ]

Corey Huinker  writes:
> Generally speaking, people disliked the third mode for \timing, and were
> generally fine with AndrewG's idea of printing the timing in both raw
> milliseconds and a more human-digestible format, which means that we can:

Yeah, there seemed to be general agreement on just appending a more human
readable format to the existing printout.

> 3. ignore locales and fall back to a left-trimmed DDD HH:MM:SS.mmm format
>  + Easy to revert to that code
>  + My original format and one PeterE advocated
>  - others disliked

I think this is the approach to go with as a starting point, since it
largely avoids both localization and units-naming concerns.  If someone
feels the desire to build a customizable output format, that can be dealt
with as a separate patch on top of this one ... but I really question that
it'd ever be worth the trouble.

So for clarity's sake: first suitable format among these:

Time: 59.999 ms
Time: 121.999 ms (2:01.999)
Time: 10921.999 ms (3:02:01.999)
Time: 356521.999 ms (4 3:02:01.999)

In an NLS-enabled build, the translator would be able to fool with the
punctuation, though I dunno whether any translators would need to.

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] Forbid use of LF and CR characters in database and role names

[Peter Eisentraut]

On 8/11/16 9:12 PM, Michael Paquier wrote:
> Note that pg_dump[all] and pg_upgrade already have safeguards against
> those things per the same routines putting quotes for execution as
> commands into psql and shell. So attached is a patch to implement this
> restriction in the backend,

How about some documentation?  I think the CREATE ROLE and CREATE
DATABASE man pages might be suitable places.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_basebackup, pg_receivexlog and data durability (was: silent data loss with ext4 / all current versions)

[Peter Eisentraut]

On 5/13/16 2:39 AM, Michael Paquier wrote:
> So, attached are two patches that apply on HEAD to address the problem
> of pg_basebackup that does not sync the data it writes. As
> pg_basebackup cannot use directly initdb -S because, as a client-side
> utility, it may be installed while initdb is not (see Fedora and
> RHEL), I have refactored the code so as the routines in initdb.c doing
> the fsync of PGDATA and other fsync stuff are in src/fe_utils/, and
> this is 0001.

Why fe_utils?  initdb is not a front-end program.

> Patch 0002 is a set of fixes for pg_basebackup:
> - In plain mode, fsync_pgdata is used so as all the tablespaces are
> fsync'd at once. This takes care as well of the case where pg_xlog is
> a symlink.
> - In tar mode (no stdout), each tar file is synced individually, and
> the base directory is synced once at the end.
> In both cases, failures are not considered fatal.

Maybe there should be --nosync options like initdb has?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WAL consistency check facility

[Simon Riggs]

On 1 September 2016 at 17:23, Robert Haas  wrote:

> The primary audience of this feature is PostgreSQL developers

I have spoken to users who are waiting for this feature to run in
production, which is why I suggested it.

Some people care more about correctness than they do about loss of performance.

Obviously, this would be expensive and those with a super high
performance requirement may not be able to take advantage of this. I'm
sure many people will turn it off once if they hit a performance
issue, but running it in production for the first few months will give
people a very safe feeling.

I think the primary use for an rmgr filter might well be PostgreSQL developers.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] System load consideration before spawning parallel workers

[Tom Lane]

Peter Eisentraut  writes:
> As I just wrote in another message in this thread, I don't trust system
> load metrics very much as a gatekeeper.  They are reasonable for
> long-term charting to discover trends, but there are numerous potential
> problems for using them for this kind of resource control thing.

As a note in support of that, sendmail has a "feature" to suppress service
if system load gets above X, which I have never found to do anything
except result in self-DOSing.  The load spike might not have anything to
do with the service that is trying to un-spike things.  Even if it does,
Peter is correct to note that the response delay is much too long to form
part of a useful feedback loop.  It could be all right for scheduling
activities whose length is comparable to the load average measurement
interval, but not for short-term decisions.

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

[HACKERS] What is the posix_memalign() equivalent for the PostgreSQL?

[Anderson Carniel]

Dear all,

I am developing an extension for the PostgreSQL that write/read some
external files from the PostgreSQL. In order to write/read, I am using the
O_DIRECT flag and using the posix_memalign to allocate memory. I would like
to know if the postgresql internal library provides an equivalent function
for the posix_memalign since I am getting unexpected errors. All my
allocations are in the TopMemoryContext since I am working with several
buffers that must be alive while the PostgreSQL Server is activated.

Thanks in advance,
Anderson

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Peter Eisentraut]

On 8/23/16 5:22 PM, Stephen Frost wrote:
> Now that we track initial privileges on extension objects and changes to
> those permissions, we can drop the superuser() checks from the various
> functions which are part of the pgstattuple extension.
> 
> Since a pg_upgrade will preserve the version of the extension which
> existed prior to the upgrade, we can't simply modify the existing
> functions but instead need to create new functions which remove the
> checks and update the SQL-level functions to use the new functions

I think this is a good change to pursue, and we'll likely want to do
more similar changes in contrib.  But I'm worried that what is logically
a 10-line change will end up a 20 KiB patch every time.

Have we explored other options for addressing the upgrade problems?
Maybe the function could check that non-default privileges have been
granted?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PostgreSQL 10 kick-off

[Fabrízio de Royes Mello]

On Thu, Sep 1, 2016 at 1:02 PM, Magnus Hagander  wrote:
>
> On Sep 1, 2016 17:44, "Fabrízio de Royes Mello" 
wrote:
> >
> >
> >
> > On Thu, Sep 1, 2016 at 8:35 AM, Tom Lane  wrote:
> > >
> > > Magnus Hagander  writes:
> > > >> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
> > > >>  wrote:
> > > >>> If there are no complains about my lack of experience in this
field I
> > > >>> would like do become the next CFM (am I the first brazilian??)
> > >
> > > > Did we make a decision on this? Cf is starting now, are we
appointing
> > > > Fabrizio as the cf manager?
> > >
> > > I didn't hear any other volunteers, so he's it.
> > >
> >
> > Well... I was waiting for an approval from core team... then now I am
the CF manager...
>
> Yup,  clearly you have it now :-)
>

And here we go!!!


> >
> > I'm getting some help from Alvaro to get the admin grants in CommitFest
App to start this party.
>
> Sounds good and don't hesitate to ask any of us if you are wondering
about something or need some help.
>

Alvaro sent a message by IM that I'm granted as admin in commitfest app but
something seams wrong... The administration "link/button" doesn't appear to
me yet... What's the correct way to get this rights?

Thanks in advance!

--
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] System load consideration before spawning parallel workers

[Peter Eisentraut]

On 8/16/16 3:39 AM, Haribabu Kommi wrote:
> Yes, we need to consider many parameters as a system load, not just only
> the CPU. Here I attached a POC patch that implements the CPU load
> calculation and decide the number of workers based on the available CPU
> load. The load calculation code is not an optimized one, there are many ways
> that can used to calculate the system load. This is just for an example.

I see a number of discussion points here:

We don't yet have enough field experience with the parallel query
facilities to know what kind of use patterns there are and what systems
for load management we need.  So I think building a highly specific
system like this seems premature.  We have settings to limit process
numbers, which seems OK as a start, and those knobs have worked
reasonably well in other areas (e.g., max connections, autovacuum).  We
might well want to enhance this area, but we'll need more experience and
information.

If we think that checking the CPU load is a useful way to manage process
resources, why not apply this to more kinds of processes?  I could
imagine that limiting connections by load could be useful.  Parallel
workers is only one specific niche of this problem.

As I just wrote in another message in this thread, I don't trust system
load metrics very much as a gatekeeper.  They are reasonable for
long-term charting to discover trends, but there are numerous potential
problems for using them for this kind of resource control thing.

All of this seems very platform specific, too.  You have
Windows-specific code, but the rest seems very Linux-specific.  The
dstat tool I had never heard of before.  There is stuff with cgroups,
which I don't know how portable they are across different Linux
installations.  Something about Solaris was mentioned.  What about the
rest?  How can we maintain this in the long term?  How do we know that
these facilities actually work correctly and not cause mysterious problems?

There is a bunch of math in there that is not documented much.  I can't
tell without reverse engineering the code what any of this is supposed
to do.

My suggestion is that we focus on refining the process control numbers
that we already have.  We had extensive discussions about that during
9.6 beta.  We have related patches in the commit fest right now.  Many
ideas have been posted.  System admins are generally able to count their
CPUs and match that to the number of sessions and jobs they need to run.
 Everything beyond that could be great but seems premature before we
have the basics figured out.

Maybe a couple of hooks could be useful to allow people to experiment
with this.  But the hooks should be more general, as described above.
But I think a few GUC settings that can be adjusted at run time could be
sufficient as well.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] new gcc warning

[Pavel Stehule]

2016-09-01 18:40 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > 2016-09-01 14:31 GMT+02:00 Tom Lane :
> >> That should have gone away in commit a2fd62dd5.  What version are
> >> you looking at?
>
> > I am checking 9.5 branch and I cannot to find this commit there
>
> Hmm ... it wasn't back-patched, evidently.  We don't have a clear
> project policy on whether to back-patch changes that are only meant
> to silence useless warnings, but it seems reasonable to push this
> one, since gcc 6 is getting more widespread.
>

Thank you

Pavel


>
> regards, tom lane
>

Re: [HACKERS] System load consideration before spawning parallel workers

[Peter Eisentraut]

On 8/1/16 2:17 AM, Gavin Flower wrote:
> Possibly look how make does it with the '-l' flag?
> 
> '-l 8' don't start more process when load is 8 or greater, works on 
> Linux at least...

The problem with that approach is that it takes about a minute for the
load averages figures to be updated, by which time you have already
thrashed your system.

You can try this out by building PostgreSQL this way.  Please save your
work first, because you might have to hard-reboot your system.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] new gcc warning

[Tom Lane]

Pavel Stehule  writes:
> 2016-09-01 14:31 GMT+02:00 Tom Lane :
>> That should have gone away in commit a2fd62dd5.  What version are
>> you looking at?

> I am checking 9.5 branch and I cannot to find this commit there

Hmm ... it wasn't back-patched, evidently.  We don't have a clear
project policy on whether to back-patch changes that are only meant
to silence useless warnings, but it seems reasonable to push this
one, since gcc 6 is getting more widespread.

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] new gcc warning

[Pavel Stehule]

2016-09-01 14:31 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > I see a new warning in upstream
>
> > r/include/libxml2   -c -o path.o path.c
> > path.c: In function ‘has_drive_prefix’:
> > path.c:89:26: warning: self-comparison always evaluates to false
> > [-Wtautological-compare]
> >   return skip_drive(path) != path;
> >   ^~
>
> That should have gone away in commit a2fd62dd5.  What version are
> you looking at?
>
>
I am checking 9.5 branch and I cannot to find this commit there

Regards

Pavel


> regards, tom lane
>

Re: [HACKERS] WAL consistency check facility

[Robert Haas]

On Thu, Sep 1, 2016 at 4:12 PM, Simon Riggs  wrote:
> So in my current understanding, a hinted change has by definition no
> WAL record, so we just ship a FPW.

Hmm.  An FPW would have to be contained in a WAL record, so it can't
be right to say that we ship an FPW for lack of a WAL record.  I think
what we ship is nothing at all when wal_log_hints is disabled, and
when wal_log_hints is enabled we log an FDW once per checkpoint.

> A non-hint change has a WAL record
> and it is my (possibly naive) hope that all changes to a page are
> reflected in the WAL record/replay,

I hope for this, too.

> so we can just make a simple
> comparison without caring what is the rmgr of the WAL record.

Sure, that is 100% possible, and likely a good idea as far as the
behavior on the standby is concerned.  What's not so clear is whether
a simple on/off switch is a wise plan on the master.

The purpose of this code, as I understand it, is to check for
discrepancies between "do" and "redo"; that is, to verify that the
changes made to the buffer at the time the WAL record is generated
produce the same result as replay of that WAL record on the standby.
To accomplish this purpose, a post-image of the affected buffers is
included in each and every WAL record.  On replay, that post-image can
be compared with the result of replay.  If they differ, PostgreSQL has
a bug.  I would not expect many users to run this in production,
because it will presumably be wicked expensive.  If I recall
correctly, doing full page writes once per buffer per checkpoint, the
space taken up by FPWs is >75% of WAL volume.  Doing it for every
record will be exponentially more expensive.  The primary audience of
this feature is PostgreSQL developers, who might want to use it to try
to verify that, for example, Amit's patch to add write-ahead logging
for hash indexes does not have bugs.[1]

Indeed, it had occurred to me that we might not even want to compile
this code into the server unless WAL_DEBUG is defined; after all, how
does it help a regular user to detect that the server has a bug?  Bug
or no bug, that's the code they've got.  But on further reflection, it
seems like it could be useful: if we suspect a bug in the redo code
but we can't reproduce it here, we could ask the customer to turn this
option on to see whether it produces logging indicating the nature of
the problem.  However, because of the likely expensive of enabling the
feature, it seems like it would be quite desirable to limit the
expense of generating many extra FPWs to the affected rmgr.  For
example, if a user has a table with a btree index and a gin index, and
we suspect a bug in GIN, it would be nice for the user to be able to
enable the feature *only for GIN* rather than paying the cost of
enabling it for btree and heap as well.[2]

Similarly, when we imagine a developer using this feature to test for
bugs, it may at times be useful to enable it across-the-board to look
for bugs in any aspect of the write-ahead logging system. However, at
other times, when the goal is to find bugs in a particular AM, it
might be useful to enable it only for the corresponding rmgr.  It is
altogether likely that this feature will slow the system down quite a
lot.  If enabling this feature for hash indexes also means enabling it
for the heap, the incremental performance hit might be sufficient to
mask concurrency-related bugs in the hash index code that would
otherwise have been found.  So, I think having at least some basic
filtering is probably a pretty smart idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[1] It probably has bugs.
[2] One could of course add filtering considerably more complex than
per-rmgr - e.g. enabling it for only one particular relfilenode on a
busy production system might be rather desirable.  But I'm not sure we
really want to go there.  It adds a fair amount of complexity to a
feature that many people are obviously hoping will be quite simple to
use.


-- 
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] Patch: Write Amplification Reduction Method (WARM)

[Bruce Momjian]

On Thu, Sep  1, 2016 at 02:37:40PM +0530, Pavan Deolasee wrote:
> I like the simplified approach, as long as it doesn't block further
> improvements.
>
> 
> 
> Yes, the proposed approach is simple yet does not stop us from improving 
> things
> further. Moreover it has shown good performance characteristics and I believe
> it's a good first step.

Agreed.  This is BIG.  Do you think it can be done for PG 10?

> Thanks. What's also interesting and something that headline numbers don't show
> is that WARM TPS is as much as 3 times of master TPS when the percentage of
> WARM updates is very high. Notice the spike in TPS in the comparison graph.
> 
> Results with non-default heap fill factor are even better. In both cases, the
> improvement in TPS stays constant over long periods. 

Yes, I expect the benefits of this to show up in better long-term
performance.

> > During first heap scan of VACUUM, we look for tuples with 
> HEAP_WARM_TUPLE
> set.
> > If all live tuples in the chain are either marked with Blue flag or Red
> flag
> > (but no mix of Red and Blue), then the chain is a candidate for HOT
> conversion.
> 
> Uh, if the chain is all blue, then there is are WARM entries so it
> already a HOT chain, so there is nothing to do, right?
> 
> 
> For aborted WARM updates, the heap chain may be all blue, but there may still
> be a red index pointer which must be cleared before we allow further WARM
> updates to the chain.

Ah, understood now.  Thanks.

> Why not just remember the tid of chains converted from WARM to HOT, then
> use "amrecheck" on an index entry matching that tid to see if the index
> matches one of the entries in the chain. 
> 
> 
> That will require random access to heap during index vacuum phase, something I
> would like to avoid. But we can have that as a fall back solution for handling
> aborted vacuums. 

Yes, that is true.  So the challenge is figuring how which of the index
entries pointing to the same tid is valid, and coloring helps with that?

> (It will match all of them or
> none of them, because they are all red.)  I don't see a point in
> coloring the index entries as reds as later you would have to convert to
> blue in the WARM-to-HOT conversion, and a vacuum crash could lead to
> inconsistencies. 
> 
> 
> Yes, that's a concern since the conversion of red to blue will also need to 
> WAL
> logged to ensure that a crash doesn't leave us in inconsistent state. I still
> think that this will be an overall improvement as compared to allowing one 
> WARM
> update per chain.

OK.  I will think some more on this to see if I can come with another
approach.

>  
> 
> Consider that you can just call "amrecheck" on the few
> chains that have converted from WARM to HOT.  I believe this is more
> crash-safe too.  However, if you have converted WARM to HOT in the heap,
> but crash durin the index entry removal, you could potentially have
> duplicates in the index later, which is bad.
>
> As you probably already noted, we clear heap flags only after all indexes are
> cleared of duplicate entries and hence a crash in between should not cause any
> correctness issue. As long as heap tuples are marked as warm, amrecheck will
> ensure that only valid tuples are returned to the caller.

OK, got it.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
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] PostgreSQL 10 kick-off

[Magnus Hagander]

On Sep 1, 2016 17:44, "Fabrízio de Royes Mello" 
wrote:
>
>
>
> On Thu, Sep 1, 2016 at 8:35 AM, Tom Lane  wrote:
> >
> > Magnus Hagander  writes:
> > >> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
> > >>  wrote:
> > >>> If there are no complains about my lack of experience in this field
I
> > >>> would like do become the next CFM (am I the first brazilian??)
> >
> > > Did we make a decision on this? Cf is starting now, are we appointing
> > > Fabrizio as the cf manager?
> >
> > I didn't hear any other volunteers, so he's it.
> >
>
> Well... I was waiting for an approval from core team... then now I am the
CF manager...

Yup,  clearly you have it now :-)

>
> I'm getting some help from Alvaro to get the admin grants in CommitFest
App to start this party.

Sounds good and don't hesitate to ask any of us if you are wondering about
something or need some help.

>
> In a few hours I'm send the emails and officially start the commitfest.

Excellent!

/Magnus

Re: [HACKERS] [WIP] Patches to enable extraction state of query execution from external session

[Tom Lane]

Maksim Milyutin  writes:
>> On Tue, Aug 30, 2016 at 9:34 AM, Maksim Milyutin
>> mailto:m.milyu...@postgrespro.ru>> wrote:
>> Yes, but the problem is that nothing gives you the guarantee that at the
>> moment you decide to handle the interrupt, the QueryDesc structures
>> you're looking at are in a good shape for Explain* functions to run on
>> them.  Even if that appears to be the case in your testing now, there's
>> no way to tell if that will be the case at any future point in time.

> CHECK_FOR_INTERRUPTS are located in places where query state (QueryDesc 
> structure) is more or less consistent.

Really?  Even if that's 100% true today, which I wouldn't bet very much
on, it seems like a really dangerous property to insist on system-wide.
The only restriction we have ever placed on CHECK_FOR_INTERRUPTS is that
it occur at places where it'd be safe to throw elog(ERROR), and in general
we don't assume that the active query is still in a usable state after
an error.  What you propose would amount to a new restriction that nothing
can ever call any nontrivial subroutine while the active query tree is
less than fully valid (because the subroutine might contain a
CHECK_FOR_INTERRUPTS somewhere).  That sounds impractical and
unenforceable.

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] PostgreSQL 10 kick-off

[Fabrízio de Royes Mello]

On Thu, Sep 1, 2016 at 8:35 AM, Tom Lane  wrote:
>
> Magnus Hagander  writes:
> >> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
> >>  wrote:
> >>> If there are no complains about my lack of experience in this field I
> >>> would like do become the next CFM (am I the first brazilian??)
>
> > Did we make a decision on this? Cf is starting now, are we appointing
> > Fabrizio as the cf manager?
>
> I didn't hear any other volunteers, so he's it.
>

Well... I was waiting for an approval from core team... then now I am the
CF manager...

I'm getting some help from Alvaro to get the admin grants in CommitFest App
to start this party.

In a few hours I'm send the emails and officially start the commitfest.

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] [WIP] Patches to enable extraction state of query execution from external session

[Maksim Milyutin]

On Tue, Aug 30, 2016 at 9:34 AM, Maksim Milyutin
mailto:m.milyu...@postgrespro.ru>> wrote:

On Mon, Aug 29, 2016 at 5:22 PM, maksim
mailto:m.milyu...@postgrespro.ru>
>> wrote:

Hi, hackers!

Now I complete extension that provides facility to see the
current
state of query execution working on external session in form of
EXPLAIN ANALYZE output. This extension works on 9.5 version,
for 9.6
and later it doesn't support detailed statistics for
parallel nodes yet.

I want to present patches to the latest version of
PostgreSQL core
to enable this extension.

Hello,

Did you publish the extension itself yet?


Hello, extension for version 9.5 is available in repository
https://github.com/postgrespro/pg_query_state/tree/master
.

Last year (actually, exactly one year ago) I was trying to do
something
very similar, and it quickly turned out that signals are not the
best
way to make this sort of inspection.  You can find the discussion
here:

https://www.postgresql.org/message-id/CACACo5Sz7G0MFauC082iM=xx_hq7qq5ndr4jpo+h-o5vp6i...@mail.gmail.com




Thanks for link!

My patch *custom_signal.patch* resolves the problem of «heavy»
signal handlers. In essence, I follow the course offered in
*procsignal.c* file. They define *ProcSignalReason* values on which
the SUGUSR1 is multiplexed. Signal recent causes setting flags for
*ProcessInterrupt* actuating, i.e. procsignal_sigusr1_handler() only
sets specific flags. When CHECK_FOR_INTERRUPTS appears later on
query execution *ProcessInterrupt* is called. Then triggered user
defined signal handler is executed.
As a result, we have a deferred signal handling.


Yes, but the problem is that nothing gives you the guarantee that at the
moment you decide to handle the interrupt, the QueryDesc structures
you're looking at are in a good shape for Explain* functions to run on
them.  Even if that appears to be the case in your testing now, there's
no way to tell if that will be the case at any future point in time.


CHECK_FOR_INTERRUPTS are located in places where query state (QueryDesc 
structure) is more or less consistent. In these macro calls I pass 
QueryDesc to Explain* functions. I exactly know that elementary 
statistics updating functions (e.g. InstrStartNode, InstrStopNode, etc) 
don't contain CHECK_FOR_INTERRUPTS within itself therefore statistics at 
least on node level is consistent when backend will be ready to transfer 
its state.
The problem may be in interpretation of collected statistics in Explain* 
functions. In my practice there was the case when wrong number of 
inserted rows is shown under INSERT ON CONFLICT request. That request 
consisted of two parts: SELECT from table and INSERT with check on 
predicate. And I was interrupted between these parts. Formula for 
inserted rows was the number of extracting rows from SELECT minus 
rejected rows from INSERT. And I got imaginary inserted row. I removed 
the printing number of inserted rows under explain of running query 
because I don't know whether INSERT node has processed that last row. 
But the remaining number of rejected rows was deterministic and I showed it.



Another problem is use if shm_mq facility, because it manipulates the
state of process latch.  This is not supposed to happen to a backend
happily performing its tasks, at random due to external factors, and
this is what the proposed approach introduces


In Postgres source code the most WaitLatch() call on process latch is 
surrounded by loop and forms the pattern like this:


  for (;;)
  {
 if (desired_state_has_occured)
   break;

 WaitLatch(MyLatch);
 CHECK_FOR_INTERRUPTS();
 ResetLatch(MyLatch)
  }

The motivation of this decision is pretty clear illustrated by the 
extract from comment in Postgres core:


usage of "the generic process latch has to be robust against unrelated 
wakeups: Always check that the desired state has occurred, and wait 
again if not"[1].


I mean that random setting of process latch at the time of query 
executing don't affect on another usage of that latch later in code.



1. src/backend/storage/lmgr/proc.c:1724 for ProcWaitForSignal function

--
Maksim Milyutin
Postgres Professional: http://www.postgrespro.com
Russian Postgres 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] Missing checks when malloc returns NULL...

[Tom Lane]

Michael Paquier  writes:
> On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane  wrote:
>> Don't see the point really ... it's just more API churn without any
>> very compelling reason.

> OK, then no objection to your approach. At least I tried.

OK, pushed my version then.  I think we have now dealt with everything
mentioned in this thread except for the issues in pg_locale.c.  Are
you planning to tackle that?

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] PATCH: Exclude additional directories in pg_basebackup

[Peter Eisentraut]

On 8/15/16 3:39 PM, David Steele wrote:
> That patch got me thinking about what else could be excluded and after
> some investigation I found the following: pg_notify, pg_serial,
> pg_snapshots, pg_subtrans.  These directories are all cleaned, zeroed,
> or rebuilt on server start.
> 
> The attached patch adds these directories to the pg_basebackup
> exclusions and takes an array-based approach to excluding directories
> and files during backup.

We do support other backup methods besides using pg_basebackup.  So I
think we need to document the required or recommended handling of each
of these directories.  And then pg_basebackup should become a consumer
of that documentation.

The current documentation on this is at
,
which covers pg_xlog and pg_replslot.  I think that documentation should
be expanded, maybe with a simple list that is easy to copy into an
exclude file, following by more detail on each directory.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Postgres abort found in 9.3.11

[Tom Lane]

"K S, Sandhya (Nokia - IN/Bangalore)"  writes:
> Our setup is a hot-standby architecture. This crash is occurring only on 
> stand-by node. Postgres continues to run without any issues on active node.
> Postmaster is waiting for a start and is throwing this message.

> Aug 22 11:44:21.462555 info node-0 postgres[8222]: [1-2] HINT:  Is another 
> postmaster already running on port 5433? If not, wait a few seconds and 
> retry.  
> Aug 22 11:44:52.065760 crit node-1 postgres[8629]: [18-1] err-3:  
> btree_xlog_delete_get_latestRemovedXid: cannot operate with inconsistent 
> dataAug 22 11:44:52.065971 crit CFPU-1 postgres[8629]: [18-2] CONTEXT:  xlog 
> redo delete: index 1663/16386/17378; iblk 1, heap 1663/16386/16518;

Hmm, that HINT seems to be the tail end of a message indicating that the
postmaster is refusing to start because of an existing postmaster.  Why
is that appearing?  If you've got some script that's overeagerly launching
and killing postmasters, maybe that's the ultimate cause of problems.

The only method I've heard of for getting that get_latestRemovedXid
error is to try to launch a standalone backend (postgres --single)
in a standby server directory.  We don't support that, cf
https://www.postgresql.org/message-id/flat/00F0B2CEF6D0CEF8A90119D4%40eje.credativ.lan

BTW, I'm curious about the "err-3:" part.  That would not be expected
in any standard build of Postgres ... is this something custom modified?

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] autonomous transactions

[Constantin S. Pan]

On Wed, 31 Aug 2016 14:46:30 +0100
Greg Stark  wrote:

> On Wed, Aug 31, 2016 at 2:50 AM, Peter Eisentraut
>  wrote:
> > - A API interface to open a "connection" to a background worker, run
> > queries, get results: AutonomousSessionStart(),
> > AutonomousSessionEnd(), AutonomousSessionExecute(), etc.  The
> > communication happens using the client/server protocol.
> 
> I'm surprised by the background worker. I had envisioned autonomous
> transactions being implemented by allowing a process to reserve a
> second entry in PGPROC with the same pid. Or perhaps save its existing
> information in a separate pgproc slot (or stack of slots) and
> restoring it after the autonomous transaction commits.
> 
> Using a background worker mean that the autonomous transaction can't
> access any state from the process memory. Parameters in plpgsql are a
> symptom of this but I suspect there will be others. What happens if a
> statement timeout occurs during an autonomous transaction? What
> happens if you use a pl language in the autonomous transaction and if
> it tries to use non-transactional information such as prepared
> statements?

I am trying to implement autonomous transactions that way. I
have already implemented suspending and restoring the parent
transaction state, at least some of it. The next thing on
the plan is the procarray/snapshot stuff. I think we should
reuse the same PGPROC for the autonomous transaction, and
allocate a stack of PGXACTs for the case of nested
autonomous transactions.

Solving the more general problem, running multiple
concurrent transactions with a single backend, may also be
interesting for some users. Autonomous transactions would
then be just a use case for that feature.

Regards,
Constantin Pan
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Choosing parallel_degree

[Simon Riggs]

On 12 April 2016 at 14:11, Simon Riggs  wrote:
> On 12 April 2016 at 13:53, Robert Haas  wrote:
>>
>> On Mon, Apr 11, 2016 at 5:45 PM, Simon Riggs 
>> wrote:
>> > On 8 April 2016 at 17:49, Robert Haas  wrote:
>> >> With the patch, you can - if you wish - substitute
>> >> some other number for the one the planner comes up with.
>> >
>> > I saw you're using AccessExclusiveLock, the reason being it affects
>> > SELECTs.
>> >
>> > That is supposed to apply when things might change the answer from a
>> > SELECT,
>> > whereas this affects only the default for a plan.
>> >
>> > Can I change this to a lower setting? I would have done this before
>> > applying
>> > the patch, but you beat me to it.
>>
>> I don't have a problem with reducing the lock level there, if we're
>> convinced that it's safe.
>
>
> I'll run up a patch, with appropriate comments.

Attached

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


reduce_lock_levels_incl_comments.patch
Description: Binary data

-- 
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] Missing checks when malloc returns NULL...

[Michael Paquier]

On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Also, we could take one extra step forward then, and just introduce
>> ShmemAllocExtended that holds two flags as per the attached:
>> - SHMEM_ALLOC_ZERO that zeros all the fields
>> - SHMEM_ALLOC_NO_OOM that does not fail
>
> Don't see the point really ... it's just more API churn without any
> very compelling reason.

OK, then no objection to your approach. At least I tried.
-- 
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] GIN logging GIN_SEGMENT_UNMODIFIED actions?

[Tom Lane]

Fujii Masao  writes:
> I applied your suggested changes into the patch. Patch attached.

That looks pretty sane to me (but I just eyeballed it, didn't test).

One further minor improvement would be to rearrange the
XLOG_GIN_VACUUM_DATA_LEAF_PAGE case so that we don't bother calling
XLogRecGetBlockData() if there's a full-page image.

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] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Simon Riggs]

On 20 August 2016 at 15:04, Petr Jelinek  wrote:
> On 20/08/16 16:01, Craig Ringer wrote:
>>
>> On 5 June 2016 at 09:54, David G. Johnston > > wrote:
>>
>> On Thursday, March 17, 2016, Craig Ringer > > wrote:
>>
>> The first patch was incorrectly created on top of failover slots
>> not HEAD. Attached patch applies on HEAD.
>>
>>
>> Lots of logical decoding work ongoing but this one shows as active
>> in the September cf and the comments by Craig indicate potential
>> relevance to 9.6.  Is this still live as-is or has it been subsumed
>> by other threads?
>>
>>
>>
>> Yes. Both those patches are still pending and should be considered for
>> commit in the next CF. (Of course, I think they should just be
>> committed, but I would, wouldn't I?)
>>
>>
>
> I think the doc one should definitely go in and possibly be back-patched all
> the way to 9.4. I didn't look at the other one.

I agree the doc patch should go in, though I suggest reword it
slightly, like attached patch.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Doc-correction-each-change-once.v2.patch
Description: Binary data

-- 
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] GIN logging GIN_SEGMENT_UNMODIFIED actions?

[Fujii Masao]

On Wed, Aug 31, 2016 at 8:32 PM, Tom Lane  wrote:
> Fujii Masao  writes:
>> On Wed, Aug 31, 2016 at 12:10 AM, Tom Lane  wrote:
>>> Hmm, comparing gin_desc() to ginRedoInsert() makes me think there are more
>>> problems there than that one.  Aren't the references to "payload" wrong
>>> in all three branches of that "if" construct, not just the middle one?
>
>> If we do this, the extra information like ginxlogInsertEntry->isDelete will
>> never be reported when the record has FPW.
>
> I'm happy to have it print whatever is there, but are you sure that the
> information is even there?  I thought that this chunk of the WAL record
> would get optimized away if we write an FPW image instead.

I was thinking that optimization logic was changed for logical decoding.
This understanding is right for heap, but not right for GIN, i.e., you're right,
such data chunk for GIN would be removed from WAL record if FPW is taken.
I applied your suggested changes into the patch. Patch attached.

Regards,

-- 
Fujii Masao
*** a/src/backend/access/rmgrdesc/gindesc.c
--- b/src/backend/access/rmgrdesc/gindesc.c
***
*** 87,99  gin_desc(StringInfo buf, XLogReaderState *record)
  		case XLOG_GIN_INSERT:
  			{
  ginxlogInsert *xlrec = (ginxlogInsert *) rec;
- char	   *payload = rec + sizeof(ginxlogInsert);
  
  appendStringInfo(buf, "isdata: %c isleaf: %c",
  			  (xlrec->flags & GIN_INSERT_ISDATA) ? 'T' : 'F',
  			 (xlrec->flags & GIN_INSERT_ISLEAF) ? 'T' : 'F');
  if (!(xlrec->flags & GIN_INSERT_ISLEAF))
  {
  	BlockNumber leftChildBlkno;
  	BlockNumber rightChildBlkno;
  
--- 87,99 
  		case XLOG_GIN_INSERT:
  			{
  ginxlogInsert *xlrec = (ginxlogInsert *) rec;
  
  appendStringInfo(buf, "isdata: %c isleaf: %c",
  			  (xlrec->flags & GIN_INSERT_ISDATA) ? 'T' : 'F',
  			 (xlrec->flags & GIN_INSERT_ISLEAF) ? 'T' : 'F');
  if (!(xlrec->flags & GIN_INSERT_ISLEAF))
  {
+ 	char	   *payload = rec + sizeof(ginxlogInsert);
  	BlockNumber leftChildBlkno;
  	BlockNumber rightChildBlkno;
  
***
*** 104,130  gin_desc(StringInfo buf, XLogReaderState *record)
  	appendStringInfo(buf, " children: %u/%u",
  	 leftChildBlkno, rightChildBlkno);
  }
! if (!(xlrec->flags & GIN_INSERT_ISDATA))
! 	appendStringInfo(buf, " isdelete: %c",
! 	(((ginxlogInsertEntry *) payload)->isDelete) ? 'T' : 'F');
! else if (xlrec->flags & GIN_INSERT_ISLEAF)
! {
! 	ginxlogRecompressDataLeaf *insertData =
! 	(ginxlogRecompressDataLeaf *) payload;
! 
! 	if (XLogRecHasBlockImage(record, 0))
! 		appendStringInfoString(buf, " (full page image)");
! 	else
! 		desc_recompress_leaf(buf, insertData);
! }
  else
  {
! 	ginxlogInsertDataInternal *insertData = (ginxlogInsertDataInternal *) payload;
  
! 	appendStringInfo(buf, " pitem: %u-%u/%u",
! 			 PostingItemGetBlockNumber(&insertData->newitem),
! 		 ItemPointerGetBlockNumber(&insertData->newitem.key),
! 	   ItemPointerGetOffsetNumber(&insertData->newitem.key));
  }
  			}
  			break;
--- 104,130 
  	appendStringInfo(buf, " children: %u/%u",
  	 leftChildBlkno, rightChildBlkno);
  }
! if (XLogRecHasBlockImage(record, 0))
! 	appendStringInfoString(buf, " (full page image)");
  else
  {
! 	char	   *payload = XLogRecGetBlockData(record, 0, NULL);
  
! 	if (!(xlrec->flags & GIN_INSERT_ISDATA))
! 		appendStringInfo(buf, " isdelete: %c",
! 		 (((ginxlogInsertEntry *) payload)->isDelete) ? 'T' : 'F');
! 	else if (xlrec->flags & GIN_INSERT_ISLEAF)
! 		desc_recompress_leaf(buf, (ginxlogRecompressDataLeaf *) payload);
! 	else
! 	{
! 		ginxlogInsertDataInternal *insertData =
! 			(ginxlogInsertDataInternal *) payload;
! 
! 		appendStringInfo(buf, " pitem: %u-%u/%u",
! 		 PostingItemGetBlockNumber(&insertData->newitem),
! 		 ItemPointerGetBlockNumber(&insertData->newitem.key),
! 		 ItemPointerGetOffsetNumber(&insertData->newitem.key));
! 	}
  }
  			}
  			break;
***
*** 144,150  gin_desc(StringInfo buf, XLogReaderState *record)
  			break;
  		case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
  			{
! ginxlogVacuumDataLeafPage *xlrec = (ginxlogVacuumDataLeafPage *) rec;
  
  if (XLogRecHasBlockImage(record, 0))
  	appendStringInfoString(buf, " (full page image)");
--- 144,151 
  			break;
  		case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
  			{
! ginxlogVacuumDataLeafPage *xlrec =
! 	(ginxlogVacuumDataLeafPage *) XLogRecGetBlockData(record, 0, NULL);
  
  if (XLogRecHasBlockImage(record, 0))
  	appendStringInfoString(buf, " (full page image)");

-- 
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] Missing checks when malloc returns NULL...

[Tom Lane]

Michael Paquier  writes:
> Also, we could take one extra step forward then, and just introduce
> ShmemAllocExtended that holds two flags as per the attached:
> - SHMEM_ALLOC_ZERO that zeros all the fields
> - SHMEM_ALLOC_NO_OOM that does not fail

Don't see the point really ... it's just more API churn without any
very compelling reason.

(It doesn't look like you correctly implemented the case of both
flags being set, either.)

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] GiST penalty functions [PoC]

[Andrew Borodin]

Thank you for your coments, Tom.

> Modern compilers are generally able to make their own decisions about it, and 
> trying to put your thumb on the scales this heavily is not likely to improve 
> the code.
Well, I have tested that combination of "static inline" affets
performance of index build on a scale of 5%. Though I didn't tested
with "static" only.
AFAIK compiler cannot prove that array of function input and output do
not intersect, so it emits lots of writes to output address inside
loop body.

>That pack_float function is absolutely not acceptable
Well, possibly, there are ways to achive penalty optimization without
such hacks, but it failed for pg_shpere and in PostGIS code. They
reverted plain arithmetic optimization without bit hacks, becuase it
didn't worked. This one works.
There is other way: advance GiST API. But I'm not sure it is possible
to do so without breaking compatibility with many existing extensions.

Best regards, Andrey Borodin, Octonica & Ural Federal University.


-- 
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] GiST penalty functions [PoC]

[Tom Lane]

Andrew Borodin  writes:
> Does every supported Postgres platform conforms to IEEE 754 floating
> point specification?

Possibly, but it is against project policy to allow code to assume so.
That pack_float function is absolutely not acceptable IMV, and would
still be if it were adequately documented, which it's far away from
being.

On general grounds, I would forget all the "inline"s.  Modern compilers
are generally able to make their own decisions about it, and trying to put
your thumb on the scales this heavily is not likely to improve the code.

Haven't really read the patch, just responding to a couple points you
mentioned in the intro.

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] pg_basebackup stream xlog to tar

[Michael Paquier]

On Thu, Sep 1, 2016 at 5:13 PM, Magnus Hagander  wrote:
> We don't seem to check for similar issues as the one just found in the
> existing tests though, do we? As in, we don't actually verify that the xlog
> files being streamed are 16Mb? (Or for that matter that the tarfile emitted
> by -Ft is actually a tarfile?) Or am I missing some magic somewhere? :)

No. There is no checks on the WAL file size (you should use the output
of pg_controldata to see how large the segments should be). For the
tar file, the complication is in its untar... Perl provides some ways
to untar things, though the oldest version that we support in the TAP
tests does not offer that :(
-- 
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] new gcc warning

[Tom Lane]

Pavel Stehule  writes:
> I see a new warning in upstream

> r/include/libxml2   -c -o path.o path.c
> path.c: In function ‘has_drive_prefix’:
> path.c:89:26: warning: self-comparison always evaluates to false
> [-Wtautological-compare]
>   return skip_drive(path) != path;
>   ^~

That should have gone away in commit a2fd62dd5.  What version are
you looking at?

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] Patches to enable extraction state of query execution from external session

[Oleksandr Shulgin]

On Tue, Aug 30, 2016 at 9:34 AM, Maksim Milyutin 
wrote:

> On Mon, Aug 29, 2016 at 5:22 PM, maksim > > wrote:
>>
>> Hi, hackers!
>>
>> Now I complete extension that provides facility to see the current
>> state of query execution working on external session in form of
>> EXPLAIN ANALYZE output. This extension works on 9.5 version, for 9.6
>> and later it doesn't support detailed statistics for parallel nodes
>> yet.
>>
>> I want to present patches to the latest version of PostgreSQL core
>> to enable this extension.
>>
>> Hello,
>>
>> Did you publish the extension itself yet?
>>
>>
> Hello, extension for version 9.5 is available in repository
> https://github.com/postgrespro/pg_query_state/tree/master.
>
> Last year (actually, exactly one year ago) I was trying to do something
>> very similar, and it quickly turned out that signals are not the best
>> way to make this sort of inspection.  You can find the discussion
>> here: https://www.postgresql.org/message-id/CACACo5Sz7G0MFauC082iM
>> =xx_hq7qq5ndr4jpo+h-o5vp6i...@mail.gmail.com
>>
>
> Thanks for link!
>
> My patch *custom_signal.patch* resolves the problem of «heavy» signal
> handlers. In essence, I follow the course offered in *procsignal.c* file.
> They define *ProcSignalReason* values on which the SUGUSR1 is multiplexed.
> Signal recent causes setting flags for *ProcessInterrupt* actuating, i.e.
> procsignal_sigusr1_handler() only sets specific flags. When
> CHECK_FOR_INTERRUPTS appears later on query execution *ProcessInterrupt* is
> called. Then triggered user defined signal handler is executed.
> As a result, we have a deferred signal handling.
>

Yes, but the problem is that nothing gives you the guarantee that at the
moment you decide to handle the interrupt, the QueryDesc structures you're
looking at are in a good shape for Explain* functions to run on them.  Even
if that appears to be the case in your testing now, there's no way to tell
if that will be the case at any future point in time.

Another problem is use if shm_mq facility, because it manipulates the state
of process latch.  This is not supposed to happen to a backend happily
performing its tasks, at random due to external factors, and this is what
the proposed approach introduces.

--
Alex

Re: [HACKERS] PostgreSQL 10 kick-off

[Tom Lane]

Magnus Hagander  writes:
>> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
>>  wrote:
>>> If there are no complains about my lack of experience in this field I
>>> would like do become the next CFM (am I the first brazilian??)

> Did we make a decision on this? Cf is starting now, are we appointing
> Fabrizio as the cf manager?

I didn't hear any other volunteers, so he's it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Surprising behaviour of \set AUTOCOMMIT ON

[Rahila Syed]

Ok. Please find attached a patch which introduces psql error when
autocommit is turned on inside a transaction. It also adds relevant
documentation in psql-ref.sgml. Following is the output.

bash-4.2$ psql -d postgres
psql (10devel)
Type "help" for help.

postgres=# \set AUTOCOMMIT OFF
postgres=# create table test(i int);
CREATE TABLE
postgres=# \set AUTOCOMMIT ON
\set: Cannot set AUTOCOMMIT to ON inside a transaction, either COMMIT or
ROLLBACK and retry
postgres=#


Thank you,
Rahila Syed

On Wed, Aug 17, 2016 at 6:31 PM, Robert Haas  wrote:

> On Tue, Aug 16, 2016 at 5:25 PM, Rahila Syed 
> wrote:
> >>I think I like the option of having psql issue an error.  On the
> >>server side, the transaction would still be open, but the user would
> >>receive a psql error message and the autocommit setting would not be
> >>changed.  So the user could type COMMIT or ROLLBACK manually and then
> >>retry changing the value of the setting.
> >
> > Throwing psql error comes out to be most accepted outcome on this
> thread. I
> > agree it is safer than guessing user intention.
> >
> > Although according to the default behaviour of psql, error will abort the
> > current transaction and roll back all the previous commands.
>
> A server error would do that, but a psql errror won't.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


psql_error_on_autocommit.patch
Description: application/download

-- 
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] WAL consistency check facility

[Simon Riggs]

On 1 September 2016 at 11:16, Robert Haas  wrote:
> On Wed, Aug 31, 2016 at 7:02 PM, Simon Riggs  wrote:
>> I'd prefer a solution that was not dependent upon RmgrID at all.
>>
>> If there are various special cases that we need to cater for, ISTM
>> they would be flaws in the existing WAL implementation rather than
>> anything we would want to perpetuate. I hope we'll spend time fixing
>> them rather than add loads of weird code to work around the
>> imperfections.
>>
>> Underdocumented special case code is going to be unbelievably
>> difficult to get right in the long term.
>
> It seems to me that you may be conflating the issue of which changes
> should be masked out as hints (which is, indeed, special case code,
> whether underdocumented or not) with the issue of which rmgrs the user
> may want to verify (which is just a case of matching the rmgr ID in
> the WAL record against a list provided by the user, and is not special
> case code at all).

Yep, it seems entirely likely that I am misunderstanding what is
happening here. I'd like to see an analysis/discussion before we write
code. As you might expect, I'm excited by this feature and the
discoveries it appears likely to bring.

We've got wal_log_hints and that causes lots of extra traffic. I'm
happy with assuming that is switched on in this case also. (Perhaps we
might have a wal_log_level with various levels of logging.)

So in my current understanding, a hinted change has by definition no
WAL record, so we just ship a FPW. A non-hint change has a WAL record
and it is my (possibly naive) hope that all changes to a page are
reflected in the WAL record/replay, so we can just make a simple
comparison without caring what is the rmgr of the WAL record.

If we can start by discussing which special cases we know about that
require extra code, that will help. We can then decide whether to fix
the WAL record/replay or fix the comparison logic, possibly on a case
by case basis. My current preference is to generate lots of small
fixes to existing WAL code and then have a very, very simple patch for
this actual feature, but am willing to discuss alternate approaches.

IMV this would be a feature certain users would want turned on all the
time for everything. So I'm not bothered much about making this
feature settable by rmgr. I might question why this particular feature
would be settable by rmgr, when features like wal_log_hints and
wal_compression are not, but such discussion is a minor point in
comparison to discussing the main feature.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WAL consistency check facility

[Robert Haas]

On Wed, Aug 31, 2016 at 7:02 PM, Simon Riggs  wrote:
> I'd prefer a solution that was not dependent upon RmgrID at all.
>
> If there are various special cases that we need to cater for, ISTM
> they would be flaws in the existing WAL implementation rather than
> anything we would want to perpetuate. I hope we'll spend time fixing
> them rather than add loads of weird code to work around the
> imperfections.
>
> Underdocumented special case code is going to be unbelievably
> difficult to get right in the long term.

It seems to me that you may be conflating the issue of which changes
should be masked out as hints (which is, indeed, special case code,
whether underdocumented or not) with the issue of which rmgrs the user
may want to verify (which is just a case of matching the rmgr ID in
the WAL record against a list provided by the user, and is not special
case code at all).

-- 
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] PostgreSQL 10 kick-off

[Magnus Hagander]

On Aug 4, 2016 2:25 AM, "Michael Paquier"  wrote:
>
> On Thu, Aug 4, 2016 at 12:09 AM, Fabrízio de Royes Mello
>  wrote:
> > If there are no complains about my lack of experience in this field I
would
> > like do become the next CFM (am I the first brazilian??)
>
> That would be great. I am not sending more patches for the 1st CF and
> as long as there are no issues for 9.6 to deal with, so I am switching
> to patch review mode pretty soon, so I'll help out with things.

Did we make a decision on this? Cf is starting now, are we appointing
Fabrizio as the cf manager?

/Magnus

Re: [HACKERS] CREATE POLICY bug ?

[Dean Rasheed]

[Please reply to the list, not just to me, so that others can benefit
from and contribute to the discussion]

On 31 August 2016 at 11:52, Andrea Adami  wrote:
> Thnaks Dean, i did further investigations:
> i set the owner of the view to: "mana...@scuola247.it" with:
> ALTER TABLE public.policy_view OWNER TO "mana...@scuola247.it";
> and i thinking to see from the select:
> select * from policy_view
> the rows: 1,2,3
> then
> set role 'mana...@scuola247.it';
> select * from policy_view;
> return rows 1,2,3 as expected but:
> set role 'teac...@scuola247.it';
> select * from policy_view;
> returns rows 4,5 and
> set role 'postgres'
> select * from policy_view
> return nothing ...
> what you thinking about ?
>
> Andrea

That's correct. With the table owned by postgres and the view owned by
"mana...@scuola247.it", access to the table via the view is subject to
the policies that apply to "mana...@scuola247.it". So regardless of
who the current user is, when selecting from the view, the policy
"standard" will be applied, and that will limit the visible rows to
those for which usr = current_user.

Regards,
Dean


-- 
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 for changes to recovery.conf API

[Simon Riggs]

On 31 August 2016 at 20:01, Abhijit Menon-Sen  wrote:

> Unfortunately, some parts conflict with the patch that Simon just posted
> (e.g., his patch removes trigger_file altogether, whereas mine converts
> it into a GUC along the lines of the original patch). Rather than trying
> to untangle that right now, I'm posting what I have as-is, and I'll post
> an updated version tomorrow.

Thanks.

archive_cleanup_command no longer needs to be in shmem. Checkpointer
will have its own copy of the value.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Proposal for changes to recovery.conf API

[Simon Riggs]

On 1 September 2016 at 06:34, Michael Paquier  wrote:
> On Thu, Sep 1, 2016 at 1:15 AM, Simon Riggs  wrote:
>> This is a summary of proposed changes to the recovery.conf API for
>> v10. These are based in part on earlier discussions, and represent a
>> minimal modification to current usage.
>>
>> Proposed changes (with reference to patches from Abhijit Menon-Sen and 
>> myself)
>>
>> * pg_ctl start -M (normal|recover|continue)
...
> That looks like a sound design.

Thanks

>> * Recovery parameters would now be part of the main postgresql.conf
>> infrastructure
>> Any parameters set in $DATADIR/recovery.conf will be read after the
>> main parameter file, similar to the way that postgresql.conf.auto is
>> read.
>> (Abhijit)
>> * pg_basebackup -R will continue to generate a parameter file called
>> recovery.conf as it does now, but will also create a file named
>> recovery.trigger.
>> (This part is WIP; patch doesn't include implementation for tar format yet)
>
> Or we could just throw away this dependency with recovery.conf,
> simply. I see no need to keep it if recovery is triggered depending on
> recovery.trigger, nor recommend its use. We could instead add
> include_if_exists 'recovery.conf' at the bottom of postgresql.conf and
> let the infrastructure do the rest to simplify the patch.

It works exactly the same as ALTER SYSTEM and adds only one small hunk of code.

>> * Parameters
>> All of the parameters formerly set in recovery.conf can be set in
>> postgresql.conf using RELOAD
>> These parameters will have no defaults in postgresql.conf.sample
>> Setting them has no effect during normal running, or once recovery ends.
>>  https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
>>  https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
>>  https://www.postgresql.org/docs/devel/static/standby-settings.html
>> (Abhijit)
>
> Hm. I think that what would make sense here is a new GUC category,
> meaning that once recovery is launched the new parameters are not
> taken into account once again. Even updating primary_conninfo would
> need a restart of the WAL receiver, so we could just make them
> GUC_POSTMASTER and be done with it.

We definitely want most of them set at RELOAD, especially recovery targets.

Almost all of them will have no effect when normal mode starts, so I
don't see any other special handling needed.

>> Related cleanup
>> * Promotion signal file is now called "promote.trigger" rather than
>> just "promote"
>
> If that's not strictly necessary this renaming is not mandatory.

I think it makes sense to keep both files with same suffix, for clarity.

>> * Remove user configurable "trigger_file" mechanism - use
>> "promote.trigger" for all cases
>
> Ugh. I am -1 on that. There are likely backup tools and users that
> rely on this option, particularly to be able to trigger promotion
> using a file that is on a different partition than PGDATA.

OK, I wasn't thinking of that. Perhaps we should have a trigger
directory parameter?

>> * Remove Fallback promote mechanism, so all promotion is now "fast" in xlog.c
>
> No problem with that. Now others have surely other opinions. That
> could be addressed as a separate patch.

I'll post separate patch.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add support for restrictive RLS policies

[Thom Brown]

On 1 September 2016 at 10:02, Robert Haas  wrote:
> On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost  wrote:
>> As outlined in the commit message, this adds support for restrictive RLS
>> policies.  We've had this in the backend since 9.5, but they were only
>> available via hooks and therefore extensions.  This adds support for
>> them to be configured through regular DDL commands.  These policies are,
>> essentially "AND"d instead of "OR"d.
>>
>> Includes updates to the catalog, grammer, psql, pg_dump, and regression
>> tests.  Documentation will be added soon, but until then, would be great
>> to get feedback on the grammer, catalog and code changes.
>
> I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> for one thing.  I think putting the word RESTRICT, or maybe AS
> RESTRICT, somewhere later in the command would be better.
>
> I also think that it is very strange to have the grammar keyword be
> "restrict" but the internal flag be called "permissive".  It would be
> better to have the sense of those flags match.
>
> (This is not intended as a full review, just a quick comment.)

I had proposed this sort of functionality a couple years back:
https://www.depesz.com/2014/10/02/waiting-for-9-5-row-level-security-policies-rls/#comment-187800

And I suggested CREATE RESTRICTIVE POLICY, but looking back at that,
perhaps you're right, and it would be better to add it later in the
command.

Thom


-- 
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] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Thu, Sep 1, 2016 at 1:33 AM, Bruce Momjian  wrote:

> On Wed, Aug 31, 2016 at 10:15:33PM +0530, Pavan Deolasee wrote:
> > Instead, what I would like to propose and the patch currently implements
> is to
> > restrict WARM update to once per chain. So the first non-HOT update to a
> tuple
> > or a HOT chain can be a WARM update. The chain can further be HOT
> updated any
> > number of times. But it can no further be WARM updated. This might look
> too
> > restrictive, but it can still bring down the number of regular updates by
> > almost 50%. Further, if we devise a strategy to convert a WARM chain
> back to
> > HOT chain, it can again be WARM updated. (This part is currently not
> > implemented). A good side effect of this simple strategy is that we know
> there
> > can maximum two index entries pointing to any given WARM chain.
>
> I like the simplified approach, as long as it doesn't block further
> improvements.
>
>
Yes, the proposed approach is simple yet does not stop us from improving
things further. Moreover it has shown good performance characteristics and
I believe it's a good first step.


>
> > Master:
> > tps = 1138.072117 (including connections establishing)
> >
> > WARM:
> > tps = 2016.812924 (including connections establishing)
>
> These are very impressive results.
>
>
Thanks. What's also interesting and something that headline numbers don't
show is that WARM TPS is as much as 3 times of master TPS when the
percentage of WARM updates is very high. Notice the spike in TPS in the
comparison graph.

Results with non-default heap fill factor are even better. In both cases,
the improvement in TPS stays constant over long periods.


>
> >
> > During first heap scan of VACUUM, we look for tuples with
> HEAP_WARM_TUPLE set.
> > If all live tuples in the chain are either marked with Blue flag or Red
> flag
> > (but no mix of Red and Blue), then the chain is a candidate for HOT
> conversion.
>
> Uh, if the chain is all blue, then there is are WARM entries so it
> already a HOT chain, so there is nothing to do, right?
>

For aborted WARM updates, the heap chain may be all blue, but there may
still be a red index pointer which must be cleared before we allow further
WARM updates to the chain.


>
> > We remember the root line pointer and Red-Blue flag of the WARM chain in
> a
> > separate array.
> >
> > If we have a Red WARM chain, then our goal is to remove Blue pointers
> and vice
> > versa. But there is a catch. For Index2 above, there is only Blue pointer
> > and that must not be removed. IOW we should remove Blue pointer iff a Red
> > pointer exists. Since index vacuum may visit Red and Blue pointers in any
> > order, I think we will need another index pass to remove dead
> > index pointers. So in the first index pass we check which WARM
> candidates have
> > 2 index pointers. In the second pass, we remove the dead pointer and
> reset Red
> > flag is the surviving index pointer is Red.
>
> Why not just remember the tid of chains converted from WARM to HOT, then
> use "amrecheck" on an index entry matching that tid to see if the index
> matches one of the entries in the chain.


That will require random access to heap during index vacuum phase,
something I would like to avoid. But we can have that as a fall back
solution for handling aborted vacuums.



> (It will match all of them or
> none of them, because they are all red.)  I don't see a point in
> coloring the index entries as reds as later you would have to convert to
> blue in the WARM-to-HOT conversion, and a vacuum crash could lead to
> inconsistencies.


Yes, that's a concern since the conversion of red to blue will also need to
WAL logged to ensure that a crash doesn't leave us in inconsistent state. I
still think that this will be an overall improvement as compared to
allowing one WARM update per chain.


> Consider that you can just call "amrecheck" on the few
> chains that have converted from WARM to HOT.  I believe this is more
> crash-safe too.  However, if you have converted WARM to HOT in the heap,
> but crash durin the index entry removal, you could potentially have
> duplicates in the index later, which is bad.
>
>
As you probably already noted, we clear heap flags only after all indexes
are cleared of duplicate entries and hence a crash in between should not
cause any correctness issue. As long as heap tuples are marked as warm,
amrecheck will ensure that only valid tuples are returned to the caller.

Thanks,
Pavan

-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg_basebackup wish list

[Simon Riggs]

On 19 August 2016 at 08:46, Michael Paquier  wrote:
> On Fri, Aug 19, 2016 at 2:04 PM, Masahiko Sawada  
> wrote:
>> I agree with adding this as an option and removing directory by default.
>> And it looks good to me except for missing new line in usage output.
>>
>> printf(_("  -l, --label=LABEL  set backup label\n"));
>> +   printf(_("  -n, --noclean  do not clean up after errors"));
>> printf(_("  -P, --progress show progress information\n"));
>>
>> Registered this patch to CF1.
>
> +1 for the idea. Looking at the patch it is taking a sane approach.

Apart from this one liner change we look good to go.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add support for restrictive RLS policies

[Robert Haas]

On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost  wrote:
> As outlined in the commit message, this adds support for restrictive RLS
> policies.  We've had this in the backend since 9.5, but they were only
> available via hooks and therefore extensions.  This adds support for
> them to be configured through regular DDL commands.  These policies are,
> essentially "AND"d instead of "OR"d.
>
> Includes updates to the catalog, grammer, psql, pg_dump, and regression
> tests.  Documentation will be added soon, but until then, would be great
> to get feedback on the grammer, catalog and code changes.

I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
for one thing.  I think putting the word RESTRICT, or maybe AS
RESTRICT, somewhere later in the command would be better.

I also think that it is very strange to have the grammar keyword be
"restrict" but the internal flag be called "permissive".  It would be
better to have the sense of those flags match.

(This is not intended as a full review, just a quick comment.)

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