Re: [HACKERS] autonomous transactions

2016-08-30 Thread Jaime Casanova
On 30 August 2016 at 20:50, Peter Eisentraut
 wrote:
>
> - Patches to PL/pgSQL to implement Oracle-style autonomous transaction
> blocks:
>
> AS $$
> DECLARE
>   PRAGMA AUTONOMOUS_TRANSACTION;
> BEGIN
>   FOR i IN 0..9 LOOP
> START TRANSACTION;
> INSERT INTO test1 VALUES (i);
> IF i % 2 = 0 THEN
> COMMIT;
> ELSE
> ROLLBACK;
> END IF;
>   END LOOP;
>
>   RETURN 42;
> END;
> $$;
>

this is the syntax it will use?
i just compiled this in head and created a function based on this one.
The main difference is that the column in test1 it's a pk so i used
INSERT ON CONFLICT DO NOTHING

and i'm getting this error

postgres=# select foo();
LOG:  namespace item variable itemno 1, name val
CONTEXT:  PL/pgSQL function foo() line 7 at SQL statement
STATEMENT:  select foo();
ERROR:  null value in column "i" violates not-null constraint
DETAIL:  Failing row contains (null).
STATEMENT:  INSERT INTO test1 VALUES (val) ON CONFLICT DO NOTHING
ERROR:  null value in column "i" violates not-null constraint
DETAIL:  Failing row contains (null).
CONTEXT:  PL/pgSQL function foo() line 7 at SQL statement
STATEMENT:  select foo();
ERROR:  null value in column "i" violates not-null constraint
DETAIL:  Failing row contains (null).
CONTEXT:  PL/pgSQL function foo() line 7 at SQL statement

this happens even everytime i use the PRAGMA even if no START
TRANSACTION, COMMIT or ROLLBACK are used

-- 
Jaime Casanova  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] standalone backend PANICs during recovery

2016-08-30 Thread Michael Paquier
On Tue, Aug 30, 2016 at 11:00 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Does the attached suit better then?
>
> Thinking about it some more ... what we actually need to prevent, AFAICS,
> is standby_mode becoming true in a standalone backend.  If you don't set
> that, then the process will do PITR recovery, but I'm not aware that there
> is any problem with running that standalone, and maybe it's useful to
> allow it for debugging purposes.  So I'm thinking more like
>
> /*
>  * Check for recovery control file, and if so set up state for offline
>  * recovery
>  */
> readRecoveryCommandFile();
>
> +   /*
> +* We don't support standby_mode in standalone backends; that
> +* requires other processes such as WAL receivers to be alive.
> +*/
> +   if (StandbyModeRequested && !IsUnderPostmaster)
> +   ereport(FATAL, ...);
> +
> /*
>  * Save archive_cleanup_command in shared memory so that other 
> processes
>  * can see it.
>  */
>
> or we could put the check near the bottom of readRecoveryCommandFile.
> Not sure which placement is clearer.

I have spent some time playing with that and you are right. Only
standby_mode = on is able trigger a failure here, and the first one is
in WaitForWALToBecomeAvailable(). I'd just put the check at the end of
readRecoveryCommandFile(), this will avoid extra thinking should
readRecoveryCommandFile() be moved around. That's unlikely to happen
but it is a cheap insurance.

I have added a test on that using 001_stream_rep.pl, now that's not
actually a good idea to push this test as if it passes the execution
of postgres would just hang and prevent the test to continue to run
and move on. But it makes it easier for anybody looking at this patch
to test the pattern prevented here.
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index acd95aa..b44fed0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5216,6 +5216,14 @@ readRecoveryCommandFile(void)
 	}
 
 	FreeConfigVariables(head);
+
+	/*
+	 * We don't support standby_mode in standalone backends; that
+	 * requires other processes such as the WAL receiver to be alive.
+	 */
+	if (StandbyModeRequested && !IsUnderPostmaster)
+		ereport(FATAL,
+(errmsg("standby mode is not supported for standalone backends")));
 }
 
 /*
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index fd71095..7b47301 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 4;
+use Test::More tests => 5;
 
 # Initialize master node
 my $node_master = get_new_node('master');
@@ -18,6 +18,11 @@ $node_master->backup($backup_name);
 my $node_standby_1 = get_new_node('standby_1');
 $node_standby_1->init_from_backup($node_master, $backup_name,
 	has_streaming => 1);
+
+# Standby mode not supported for standalone backends
+command_fails(['postgres', '--single', '-D', $node_standby_1->data_dir],
+			  'no standby mode support for standalone backends');
+
 $node_standby_1->start;
 
 # Take backup of standby 1 (not mandatory, but useful to check if

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

2016-08-30 Thread Jaime Casanova
On 30 August 2016 at 23:10, Joel Jacobson  wrote:
>
> There should be a way to within the session and/or txn permanently
> block autonomous transactions.
>

This will defeat one of the use cases of autonomous transactions: auditing

>
> Coding conventions, rules and discipline are all good and will help
> against misuse of the feature, but some day someone will make a
> mistake and wrongly use the autonomous transaction and cause unwanted
> unknown side-effect I as a caller function didn't expect or know
> about.
>

well, if the feature is not guilty why do you want to put it in jail?

-- 
Jaime Casanova  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] ICU integration

2016-08-30 Thread Michael Paquier
On Wed, Aug 31, 2016 at 1:12 PM, Peter Eisentraut
 wrote:
> On 8/30/16 11:27 PM, Craig Ringer wrote:
>> Speaking of which, have you had a chance to try it on Windows yet?
>
> nope

+SELECT a, b FROM collate_test2 ORDER BY b;
+ a |  b
+---+-
+ 1 | abc
+ 4 | ABC
+ 3 | bbc
+ 2 | äbc
+(4 rows)
Be careful with non-ASCII characters in the regression tests, remember
for example that where jacana was not happy:
https://www.postgresql.org/message-id/cab7npqrod2mxqy_5+czjvhw0whrrz6p8jv_rsblcrxrtwlh...@mail.gmail.com
-- 
Michael


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


[HACKERS] sequence data type

2016-08-30 Thread Peter Eisentraut
Here is a patch that adds the notion of a data type to a sequence.  So
it might be CREATE SEQUENCE foo AS integer.  The types are restricted to
int{2,4,8} as now.

The main point of this is to make monitoring sequences less complicated.
 Right now, a serial column creates an int4 column but creates the
sequence with a max value for int8.  So in order to correctly answer the
question, is the sequence about to run out, you need to look not only at
the sequence but also any columns it is associated with.  check_postgres
figures this out, but it's complicated and slow, and not easy to do
manually.

If you tell the sequence the data type you have in mind, it
automatically sets appropriate min and max values.  Serial columns also
make use of this, so the sequence type automatically matches the column
type.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 97246197cfe3a69d14af1eb98f894946a2b8122d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 23 Aug 2016 12:00:00 -0400
Subject: [PATCH] Add CREATE SEQUENCE AS  clause

This stores a data type, required to be an integer type, with the
sequence.  The sequences min and max values default to the range
supported by the type, and they cannot be set to values exceeding that
range.  The internal implementation of the sequence is not affected.

Change the serial types to create sequences of the appropriate type.
This makes sure that the min and max values of the sequence for a serial
column match the range of values supported by the table column.  So the
sequence can no longer overflow the table column.

This also makes monitoring for sequence exhaustion/wraparound easier,
which currently requires various contortions to cross-reference the
sequences with the table columns they are used with.
---
 doc/src/sgml/information_schema.sgml  |  4 +-
 doc/src/sgml/ref/create_sequence.sgml | 37 +++
 src/backend/catalog/information_schema.sql|  4 +-
 src/backend/commands/sequence.c   | 92 --
 src/backend/parser/gram.y |  6 +-
 src/backend/parser/parse_utilcmd.c|  2 +-
 src/bin/pg_dump/pg_dump.c | 94 +--
 src/bin/pg_dump/t/002_pg_dump.pl  |  2 +
 src/include/catalog/pg_proc.h |  2 +-
 src/include/commands/sequence.h   |  6 +-
 src/include/pg_config_manual.h|  6 --
 src/test/modules/test_pg_dump/t/001_base.pl   |  1 +
 src/test/regress/expected/sequence.out| 45 +
 src/test/regress/expected/sequence_1.out  | 45 +
 src/test/regress/expected/updatable_views.out |  3 +-
 src/test/regress/sql/sequence.sql | 20 +-
 16 files changed, 269 insertions(+), 100 deletions(-)

diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325..a3a19ce 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4653,9 +4653,7 @@ sequences Columns
   data_type
   character_data
   
-   The data type of the sequence.  In
-   PostgreSQL, this is currently always
-   bigint.
+   The data type of the sequence.
   
  
 
diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index c959146..f31b595 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -21,7 +21,9 @@
 
  
 
-CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name [ INCREMENT [ BY ] increment ]
+CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name
+[ AS data_type ]
+[ INCREMENT [ BY ] increment ]
 [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ]
 [ START [ WITH ] start ] [ CACHE cache ] [ [ NO ] CYCLE ]
 [ OWNED BY { table_name.column_name | NONE } ]
@@ -111,6 +113,21 @@ Parameters

 

+data_type
+
+ 
+  The optional
+  clause AS data_type
+  specifies the data type of the sequence.  Valid types are
+  are smallint, integer,
+  and bigint.  bigint is the
+  default.  The data type determines the default minimum and maximum
+  values of the sequence.
+ 
+
+   
+
+   
 increment
 
  
@@ -132,9 +149,9 @@ Parameters
   class="parameter">minvalue determines
   the minimum value a sequence can generate. If this clause is not
   supplied or NO MINVALUE is specified, then
-  defaults will be used.  The defaults are 1 and
-  -263-1 for ascending and descending sequences,
-  respectively.
+  defaults will be used.  The default for an ascending sequence is 1.  The
+  default for a descending sequence is the minimum value of the data type
+  plus 1.
  
 

@@ -148,9 +165,9 @@ Parameters
   class="parameter">maxvalue determines
   the 

Re: [HACKERS] ICU integration

2016-08-30 Thread Peter Eisentraut
On 8/30/16 11:27 PM, Craig Ringer wrote:
> Speaking of which, have you had a chance to try it on Windows yet?

nope

> How stable are the UCU locales? Most importantly, does ICU offer any
> way to "pin" a locale version, so we can say "we want de_DE as it was
> in ICU 4.6" and get consistent behaviour when the user sets up a
> replica on some other system with ICU 4.8? Even if the German
> government has changed its mind (again) about some details of the
> language and 4.8 knows about the changes but 4.4 doesn't?

I forgot to mention this, but the patch adds a collversion column that
stores the collation version (provided by ICU).  And then when you
upgrade ICU to something incompatible you get

+   if (numversion != collform->collversion)
+   ereport(WARNING,
+   (errmsg("ICU collator version mismatch"),
+errdetail("The database was created using
version 0x%08X, the library provides version 0x%08X.",
+  (uint32) collform->collversion,
(uint32) numversion),
+errhint("Rebuild affected indexes, or build
PostgreSQL with the right version of ICU.")));

So you still need to manage this carefully, but at least you have a
chance to learn about it.

Suggestions for refining this are welcome.

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

2016-08-30 Thread Joel Jacobson
I would love to see autonomous transactions in core.

I just have one major concern, but thankfully it's easily addressed.

There should be a way to within the session and/or txn permanently
block autonomous transactions.

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.

Coding conventions, rules and discipline are all good and will help
against misuse of the feature, but some day someone will make a
mistake and wrongly use the autonomous transaction and cause unwanted
unknown side-effect I as a caller function didn't expect or know
about.

Once you have blocked autonomous transactions in a session or txn,
then any function called must not be able to unblock it (in the
session or txn), otherwise it defeats the purpose.


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


[HACKERS] identity columns

2016-08-30 Thread Peter Eisentraut
Here is another attempt to implement identity columns.  This is a
standard-conforming variant of PostgreSQL's serial columns.  It also
fixes a few usability issues that serial columns have:

- need to set permissions on sequence in addition to table (*)
- CREATE TABLE / LIKE copies default but refers to same sequence
- cannot add/drop serialness with ALTER TABLE
- dropping default does not drop sequence
- slight weirdnesses because serial is some kind of special macro

(*) Not actually implemented yet, because I wanted to make use of the
NEXT VALUE FOR stuff I had previously posted, but I have more work to do
there.

There have been previous attempts at this between 2003 and 2007.  The
latest effort failed because it tried to implement identity columns and
generated columns in one go, but then discovered that they have wildly
different semantics.  But AFAICT, there weren't any fundamental issues
with the identity parts of the patch.

Here some interesting threads of old:

https://www.postgresql.org/message-id/flat/xzp1xw2x5jo.fsf%40dwp.des.no#xzp1xw2x5jo@dwp.des.no
https://www.postgresql.org/message-id/flat/1146360362.839.104.camel%40home#1146360362.839.104.camel@home
https://www.postgresql.org/message-id/23436.1149629297%40sss.pgh.pa.us
https://www.postgresql.org/message-id/flat/448C9B7A.601%40dunaweb.hu#448c9b7a.6010...@dunaweb.hu
https://www.postgresql.org/message-id/flat/45DACD1E.2000207%40dunaweb.hu#45dacd1e.2000...@dunaweb.hu
https://www.postgresql.org/message-id/flat/18812.1178572575%40sss.pgh.pa.us

Some comments on the implementation, and where it differs from previous
patches:

- The new attidentity column stores whether a column is an identity
column and when it is generated (always/by default).  I kept this
independent from atthasdef mainly for he benefit of existing (client)
code querying those catalogs, but I kept confusing myself by this, so
I'm not sure about that choice.  Basically we need to store four
distinct states (nothing, default, identity always, identity by default)
somehow.

- The way attidentity is initialized in some places is a bit hackish.  I
haven't focused on that so much without having a clear resolution to the
previous question.

- One previous patch managed to make GENERATED an unreserved key word.
I have it reserved right now, but perhaps with a bit more trying it can
be unreserved.

- I did not implement the restriction of one identity column per table.
That didn't seem necessary.

- I did not implement an override clause on COPY.  If COPY supplies a
value, it is always used.

- pg_dump is as always a mess.  Some of that is because of the way
pg_upgrade works: It only gives out one OID at a time, so you need to
create the table and sequences in separate entries.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 4e09e06..0879d12 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1095,6 +1095,17 @@ pg_attribute Columns
  
 
  
+  attidentity
+  char
+  
+  
+   If a space character, then not an identity column.  Otherwise,
+   a = generated always, d =
+   generated by default.
+  
+ 
+
+ 
   attisdropped
   bool
   
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325..8ece439 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -1583,13 +1583,20 @@ columns Columns
  
   is_identity
   yes_or_no
-  Applies to a feature not available in PostgreSQL
+  
+   If the column is an identity column, then YES,
+   else NO.
+  
  
 
  
   identity_generation
   character_data
-  Applies to a feature not available in PostgreSQL
+  
+   If the column is an identity column, then ALWAYS
+   or BY DEFAULT, reflecting the definition of the
+   column.
+  
  
 
  
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6f51cbc..969ce45 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -42,6 +42,8 @@
 ALTER [ COLUMN ] column_name SET DEFAULT expression
 ALTER [ COLUMN ] column_name DROP DEFAULT
 ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
+ALTER [ COLUMN ] column_name ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ]
+ALTER [ COLUMN ] column_name DROP IDENTITY
 ALTER [ COLUMN ] column_name SET STATISTICS integer
 ALTER [ COLUMN ] column_name SET ( attribute_option = value [, ... ] )
 ALTER [ COLUMN ] column_name RESET ( attribute_option [, ... ] )
@@ -170,6 +172,15 @@ Description

 

+ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY/DROP IDENTITY
+
+ 
+  These forms change whether a column is an identity 

Re: [HACKERS] ICU integration

2016-08-30 Thread Craig Ringer
On 31 August 2016 at 10:46, Peter Eisentraut
 wrote:
> Here is a patch I've been working on to allow the use of ICU for sorting
> and other locale things.

Great to see you working on this. We've got some icky head-in-the-sand
issues in this area when it comes to OS upgrades, cross-OS-release
replication, etc, and having more control over our locale handling
would be nice. It should also get rid of the Windows-vs-*nix locale
naming wart.

Speaking of which, have you had a chance to try it on Windows yet? If
not, I'm happy to offer some pointers and a working test env.

How stable are the UCU locales? Most importantly, does ICU offer any
way to "pin" a locale version, so we can say "we want de_DE as it was
in ICU 4.6" and get consistent behaviour when the user sets up a
replica on some other system with ICU 4.8? Even if the German
government has changed its mind (again) about some details of the
language and 4.8 knows about the changes but 4.4 doesn't?

Otherwise we'll just have a new version of the same problem when it
comes to replication and upgrades. User upgrades ICU or replicates to
host with newer ICU and the data in indexes no longer correctly
reflects the runtime.

Even if ICU doesn't help solve this problem it's still valuable. I
just think it's something to think about.

We could always bundle a specific ICU version, but I know how well
that'd go down with distributors. They'd just ignore us and unbundle
it then complain about it. Not wholly without reason either; they
don't want to push security updates and bug fixes for bundled
libraries. Also, ICU isn't exactly a pocket-sized library we can stash
in some 3rdpty_libs/ dir .

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


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


[HACKERS] ICU integration

2016-08-30 Thread Peter Eisentraut
Here is a patch I've been working on to allow the use of ICU for sorting
and other locale things.

This is mostly complementary to the existing FreeBSD ICU patch, most
recently discussed in [0].  While that patch removes the POSIX locale
use and replaces it with ICU, my interest was on allowing the use of
both.  I think that is necessary for upgrading, compatibility, and maybe
because someone likes it.

What I have done is extend collation objects with a collprovider column
that tells whether the collation is using POSIX (appropriate name?) or
ICU facilities.  The pg_locale_t type is changed to a struct that
contains the provider-specific locale handles.  Users of locale
information are changed to look into that struct for the appropriate
handle to use.

In initdb, I initialize the default collation set as before from the
`locale -a` output, but also add all available ICU locales with a "%icu"
appended (so "fr_FR%icu").  I suppose one could create a configuration
option perhaps in initdb to change the default so that, say, "fr_FR"
uses ICU and "fr_FR%posix" uses the old stuff.

That all works well enough for named collations and for sorting.  The
thread about the FreeBSD ICU patch discusses some details of how to best
use the ICU APIs to do various aspects of the sorting, so I didn't focus
on that too much.  I took the existing collate.linux.utf8.sql test and
ported it to the ICU setup, and it passes except for the case noted below.

I'm not sure how well it will work to replace all the bits of LIKE and
regular expressions with ICU API calls.  One problem is that ICU likes
to do case folding as a whole string, not by character.  I need to do
more research about that.  Another problem, which was also previously
discussed is that ICU does case folding in a locale-agnostic manner, so
it does not consider things such as the Turkish special cases.  This is
per Unicode standard modulo weasel wording, but it breaks existing tests
at least.

So right now the entries in collcollate and collctype need to be valid
for ICU *and* POSIX for everything to work.

Also note that ICU locales are encoding-independent and don't support a
separate collcollate and collctype, so the existing catalog structure is
not optimal.

Where it gets really interesting is what to do with the database
locales.  They just set the global process locale.  So in order to port
that to ICU we'd need to check every implicit use of the process locale
and tweak it.  We could add a datcollprovider column or something.  But
we also rely on the datctype setting to validate the encoding of the
database.  Maybe we wouldn't need that anymore, but it sounds risky.

We could have a datcollation column that by OID references a collation
defined inside the database.  With a background worker, we can log into
the database as it is being created and make adjustments, including
defining or adjusting collation definitions.  This would open up
interesting new possibilities.

What is a way to go forward here?  What's a minimal useful feature that
is future-proof?  Just allow named collations referencing ICU for now?
Is throwing out POSIX locales even for the process locale reasonable?

Oh, that case folding code in formatting.c needs some refactoring.
There are so many ifdefs there and it's repeated almost identically
three times, it's crazy to work in that.


[0]:
https://www.postgresql.org/message-id/flat/789A2F56-0E42-409D-A840-6AF5110D6085%40pingpong.net


-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/aclocal.m4 b/aclocal.m4
index 6f930b6..5ca902b 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -7,6 +7,7 @@ m4_include([config/docbook.m4])
 m4_include([config/general.m4])
 m4_include([config/libtool.m4])
 m4_include([config/perl.m4])
+m4_include([config/pkg.m4])
 m4_include([config/programs.m4])
 m4_include([config/python.m4])
 m4_include([config/tcl.m4])
diff --git a/config/pkg.m4 b/config/pkg.m4
new file mode 100644
index 000..82bea96
--- /dev/null
+++ b/config/pkg.m4
@@ -0,0 +1,275 @@
+dnl pkg.m4 - Macros to locate and utilise pkg-config.   -*- Autoconf -*-
+dnl serial 11 (pkg-config-0.29.1)
+dnl
+dnl Copyright © 2004 Scott James Remnant .
+dnl Copyright © 2012-2015 Dan Nicholson 
+dnl
+dnl This program is free software; you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 2 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful, but
+dnl WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+dnl General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU General Public License
+dnl along with this program; if not, write to the 

[HACKERS] autonomous transactions

2016-08-30 Thread Peter Eisentraut
I would like to propose the attached patch implementing autonomous
transactions for discussion and review.

This work was mostly inspired by the discussion about pg_background a
while back [0].  It seemed that most people liked the idea of having
something like that, but couldn't perhaps agree on the final interface.
Most if not all of the preliminary patches in that thread were
committed, but the user interface portions were then abandoned in favor
of other work.  (I'm aware that rebased versions of pg_background
existing.  I have one, too.)

The main use case, in a nutshell, is to be able to commit certain things
independently without having it affected by what happens later to the
current transaction, for example for audit logging.

My patch consists of three major pieces.  (I didn't make them three
separate patches because it will be clear where the boundaries are.)

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

- Patches to PL/pgSQL to implement Oracle-style autonomous transaction
blocks:

AS $$
DECLARE
  PRAGMA AUTONOMOUS_TRANSACTION;
BEGIN
  FOR i IN 0..9 LOOP
START TRANSACTION;
INSERT INTO test1 VALUES (i);
IF i % 2 = 0 THEN
COMMIT;
ELSE
ROLLBACK;
END IF;
  END LOOP;

  RETURN 42;
END;
$$;

This is very incomplete and has some open technical issues that I will
discuss below.  But those are all issues of PL/pgSQL, not really issues
of how autonomous sessions work.

Basically, a block that is declared with that pragma uses the autonomous
C API instead of SPI to do its things.

- Patches to PL/Python to implement a context manager for autonomous
sessions (similar to how subtransactions work there):

with plpy.autonomous() as a:
for i in range(0, 10):
a.execute("BEGIN")
a.execute("INSERT INTO test1 (a) VALUES (%d)" % i)
if i % 2 == 0:
a.execute("COMMIT")
else:
a.execute("ROLLBACK")

This works quite well, except perhaps some tuning with memory management
and some caching and some refactoring.

While the PL/pgSQL work is more of a top-level goal, I added the
PL/Python implementation because it is easier to map the C API straight
out to something more accessible, so testing it out is much easier.


The main technical problem I had with PL/pgSQL is how to parse named
parameters.  If you're in PL/Python, say, you do

plan = a.prepare("INSERT INTO test1 (a, b) VALUES ($1, $2)",
 ["int4", "text"])

and that works fine, because it maps straight to the client/server
protocol.  But in PL/pgSQL, you will want something like

DECLARE
  x, y ...
BEGIN
  INSERT INTO test1 (a, b) VALUES (x, y)

When running in-process (SPI), we install parser hooks that allow the
parser to check back into PL/pgSQL about whether x, y are variables and
what they mean.  When we run in an autonomous session, we don't have
that available.  So my idea was to extend the protocol Parse message to
allow sending a symbol table instead of parameter types.  So instead of
saying, there are two parameters and here are their types, I would send
a list of symbols and types, and the server would respond to the Parse
message with some kind of information about which symbols it found.  I
think that would work, but I got lost in the weeds and didn't get very
far.  But you can see some of that in the code.  If anyone has other
ideas, I'd be very interested.


Other than that, I think there are also other bits and pieces that are
worth looking at, and perhaps have some overlap with other efforts, such as:

- Refining the internal APIs for running queries, with more flexibility
than SPI.  There have recently been discussions about that.  I just used
whatever was in tcop/postgres.c directly, like pg_background does, and
that seems mostly fine, but if there are other ideas, they would be
useful for this, too.

- An exception to the "mostly fine" is that the desirable handling of
log_statement, log_duration, log_min_duration_statement for
non-top-level execution is unclear.

- The autonomous session API could also be useful for other things, such
as perhaps implementing a variant of pg_background on top of them, or
doing other asynchronous or background execution schemes.  So input on
that is welcome.

- There is some overlap with the protocol handling for parallel query,
including things like error propagation, notify handling, encoding
handling.  I suspect that other background workers will need similar
facilities, so we could simplify some of that.

- Client encoding in particular was recently discussed for parallel
query.  The problem with the existing solution is that it makes
assign_client_encoding() require hardcoded knowledge of all relevant
background worker types.  So I tried a more general solution, with a hook.

- I added new 

Re: [HACKERS] pageinspect: Hash index support

2016-08-30 Thread Michael Paquier
On Wed, Aug 31, 2016 at 2:06 AM, Alvaro Herrera
 wrote:
> Jesper Pedersen wrote:
>> Attached is a patch that adds support for hash indexes in pageinspect.
>>
>> The functionality (hash_metap, hash_page_stats and hash_page_items) follows
>> the B-tree functions.
>
> I suggest that pageinspect functions are more convenient to use via the
> get_raw_page interface, that is, instead of reading the buffer
> themselves, the buffer is handed over from elsewhere and they receive it
> as bytea.  This enables other use cases such as grabbing a page from one
> server and examining it in another one.

+1.
-- 
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] Logical decoding restart problems

2016-08-30 Thread Craig Ringer
On 25 Aug. 2016 20:03, "Stas Kelvich"  wrote:
>
> > On 20 Aug 2016, at 15:59, Craig Ringer  wrote:
> >
> > I'll wait for a test case or some more detail.
>
> Thanks for clarification about how restart_lsn is working.
>
> Digging slightly deeper into this topic revealed that problem was in two
phase decoding, not it logical decoding itself.

Good to know. The behavior didn't make much sense for logical decoding but
bugs usually don't.

Do you plan to submit a patch for logical decoding of prepared transactions
to 10.0?


Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
> [ malloc-nulls-v5.patch ]

I've committed some form of all of these changes except the one in
adt/pg_locale.c.  I'm not entirely sure whether we need to do anything
about that at all, but if we do, this doesn't cut it:

 thousands_sep = db_encoding_strdup(encoding, extlconv->thousands_sep);
 grouping = strdup(extlconv->grouping);
 
+if (!grouping)
+elog(ERROR, "out of memory");
+
 #ifdef WIN32
 /* use monetary to set the ctype */
 setlocale(LC_CTYPE, locale_monetary);

There are multiple things wrong with that:

1. The db_encoding_strdup calls can also return NULL on OOM, and aren't
being checked likewise.  (And there's another plain strdup further down,
too.)

2. You can't safely throw an elog right there, because you need to restore
the backend's prevailing setlocale state first.

3. Also, if you do it like that, you'll leak whatever strings were already
strdup'd.  (This is a relatively minor problem, but still, if we're
trying to be 100% correct then we're not there yet.)

Also, now that I'm looking at it, I see there's another pre-existing bug:

4. An elog exit is possible, due to either OOM or encoding conversion
failure, inside db_encoding_strdup().  This means we have problems #2 and
#3 even in the existing code.

Now, I believe that the coding intention here was that assigning NULL
to the respective fields of CurrentLocaleConv is okay and better than
failing the operation completely.  One argument against that is that
it's unclear whether everyplace that uses any of those fields is checking
for NULL first; and in any case, silently falling back to nonlocalized
behavior might not be better than reporting OOM.  Still, it's certainly
better than risking problem #2, which could cause all sorts of subsequent
malfunctions.

I think that a complete fix for this might go along the lines of

1. While we are setlocale'd to the nonstandard locales, collect all the
values by strdup'ing into a local variable of type "struct lconv".
(We must strdup for the reason noted in the comment, that localeconv's
output is no longer valid once we do another setlocale.)  Then restore
the standard locale settings.

2. Use db_encoding_strdup to replace any strings that need to be
converted.  (If it throws an elog, we have no damage worse than
leaking the already strdup'd strings.)

3. Check for any nulls in the struct; if so, use free_struct_lconv
to release whatever we did get, and then throw elog("out of memory").

4. Otherwise, copy the struct to CurrentLocaleConv.

If we were really feeling picky, we could probably put in a PG_TRY
block around step 2 to release the strdup'd storage after a conversion
failure.  Not sure if it's worth the trouble.

BTW, I marked the commitfest entry closed, but that may have been
premature --- feel free to reopen it if you submit additional patches
in this thread.

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] Pinning a buffer in TupleTableSlot is unnecessary

2016-08-30 Thread Andres Freund
On 2016-08-30 21:59:44 +0100, Greg Stark wrote:
> On Tue, Aug 30, 2016 at 11:12 AM, Heikki Linnakangas  wrote:
> > While profiling some queries and looking at executor overhead, I realized
> > that we're not making much use of TupleTableSlot's ability to hold a buffer
> > pin. In a SeqScan, the buffer is held pinned by the underlying heap-scan
> > anyway. Same with an IndexScan, and the SampleScan. The only thing that
> > relies on that feature is TidScan, but we could easily teach TidScan to hold
> > the buffer pin directly.
> >
> > So, how about we remove the ability of a TupleTableSlot to hold a buffer
> > pin, per the attached patch? It shaves a few cycles from a ExecStoreTuple()
> > and ExecClearTuple(), which get called a lot. I couldn't measure any actual
> > difference from that, though, but it seems like a good idea from a
> > readability point of view, anyway.
> 
> Out of curiosity why go in this direction and not the other? Why not
> move those other scans to use the TupleTableSlot API to manage the
> pins. Offhand it sounds more readable not less to have the
> TupleTableSlot be a self contained data structure that guarantees the
> lifetime of the pins matches the slots rather than have a dependency
> on the code structure in some far-away scan.

At least for heap scans the pins are page level, and thus longer lived
than the data in a slot. It's important that a scan holds a pin, because
it needs to rely on the page not being hot pruned etc..


-- 
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] Pinning a buffer in TupleTableSlot is unnecessary

2016-08-30 Thread Greg Stark
On Tue, Aug 30, 2016 at 11:12 AM, Heikki Linnakangas  wrote:
> While profiling some queries and looking at executor overhead, I realized
> that we're not making much use of TupleTableSlot's ability to hold a buffer
> pin. In a SeqScan, the buffer is held pinned by the underlying heap-scan
> anyway. Same with an IndexScan, and the SampleScan. The only thing that
> relies on that feature is TidScan, but we could easily teach TidScan to hold
> the buffer pin directly.
>
> So, how about we remove the ability of a TupleTableSlot to hold a buffer
> pin, per the attached patch? It shaves a few cycles from a ExecStoreTuple()
> and ExecClearTuple(), which get called a lot. I couldn't measure any actual
> difference from that, though, but it seems like a good idea from a
> readability point of view, anyway.

Out of curiosity why go in this direction and not the other? Why not
move those other scans to use the TupleTableSlot API to manage the
pins. Offhand it sounds more readable not less to have the
TupleTableSlot be a self contained data structure that guarantees the
lifetime of the pins matches the slots rather than have a dependency
on the code structure in some far-away scan.


-- 
greg


-- 
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] sequences and pg_upgrade

2016-08-30 Thread Andres Freund
On 2016-08-30 08:46:48 -0400, Peter Eisentraut wrote:
> I was toying with a couple of ideas that would involve changing the
> storage of sequences.  (Say, for the sake of discussion, removing the
> problematic/useless sequence_name field.)

I'd be quite interested to know what changes that are...


> I think the other solution mentioned in that thread would also work:
> Have pg_upgrade treat sequences more like system catalogs, whose format
> changes between major releases, and transferred them via the
> dump/restore route.  So instead of copying the disk files, issue a
> setval call, and the sequence should be all set up.

+1.


-- 
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] Pinning a buffer in TupleTableSlot is unnecessary

2016-08-30 Thread Andres Freund
Hi,

On 2016-08-30 13:12:41 +0300, Heikki Linnakangas wrote:
> While profiling some queries and looking at executor overhead, I realized
> that we're not making much use of TupleTableSlot's ability to hold a buffer
> pin.

FWIW, I came to a similar conclusion, while working on passing around
making the executor batched.


> So, how about we remove the ability of a TupleTableSlot to hold a buffer
> pin, per the attached patch? It shaves a few cycles from a ExecStoreTuple()
> and ExecClearTuple(), which get called a lot. I couldn't measure any actual
> difference from that, though, but it seems like a good idea from a
> readability point of view, anyway.

I actually found that rather beneficial from a performance point of
view.


> How much do we need to worry about breaking extensions? I.e. to what extent
> do we consider the TupleTableSlot API to be stable, for extensions to use?
> The FDW API uses TupleTableSlots - this patch had to fix the ExecStoreTuple
> calls in postgres_fdw.

I think we're just going to have to deal with the fallout. Trying to
maintain backward compatibility with the TupleTableSlot API seems to
prevent some rather important improvement in the area.


> We could refrain from changing the signature of ExecStoreTuple(), and throw
> an error if you try to pass a valid buffer to it. But I also have some
> bigger changes to TupleTableSlot in mind.

We should probably coordinate ;)


> I think we could gain some speed
> by making slots "read-only". For example, it would be nice if a SeqScan
> could just replace the tts_tuple pointer in the slot, and not have to worry
> that the upper nodes might have materialized the slot, and freeing the
> copied tuple. Because that happens very rarely in practice. It would be
> better if those few places where we currently materialize an existing slot,
> we would create a copy of the slot, and leave the original slot unmodified.
> I'll start a different thread on that after some more experimentation, but
> if we e.g. get rid of ExecMaterializeSlot() in its current form, would that
> be OK?

Hm.

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] Pinning a buffer in TupleTableSlot is unnecessary

2016-08-30 Thread Heikki Linnakangas

On 08/30/2016 02:38 PM, Tom Lane wrote:

Heikki Linnakangas  writes:

While profiling some queries and looking at executor overhead, I
realized that we're not making much use of TupleTableSlot's ability to
hold a buffer pin. In a SeqScan, the buffer is held pinned by the
underlying heap-scan anyway. Same with an IndexScan, and the SampleScan.


I think this is probably wrong, or at least very dangerous to remove.
The reason for the feature is that the slot may continue to point at
the tuple after the scan has moved on.  You've given no evidence at all
that that scenario doesn't arise (or that we wouldn't need it in future).


For a SeqScan, the buffer is pinned, and the tuple stays valid, because 
we have an open HeapScan on it. It will stay valid until the next 
heap_getnext() call.



At the very least I'd want to see this patch clearing the slot before
advancing the scan, so that it doesn't have actual dangling pointers
laying about.


Fair enough, although we're not 100% careful about that currently 
either. For examples, see gather_getnext, CopyFrom, and others. 
Personally I think that's OK, as long as the window between invalidating 
the underlying buffer (or other source of the tuple), and storing a new 
tuple in it, is small enough. In particular, I think this is OK:


tuple = heap_getnext(); /* this leaves a dangling pointer in slot */
slot = ExecStoreTuple(tuple); /* and this makes it valid again */


So, how about we remove the ability of a TupleTableSlot to hold a buffer
pin, per the attached patch? It shaves a few cycles from a
ExecStoreTuple() and ExecClearTuple(), which get called a lot. I
couldn't measure any actual difference from that, though, but it seems
like a good idea from a readability point of view, anyway.


If you can't measure a clear performance improvement, I'm -1 on the
whole thing.  You've got risk and breakage of third-party code, and
what to show for it?


Well, I think simplicity is a virtue all by itself. But ok, let me try a 
bit harder to benchmark this:


I created a test table with:

create table foo as select repeat('x', 500) || g from generate_series(1, 
100) g;

vacuum freeze;

And then ran:

$ cat count.sql
select count(*) from foo;
$ pgbench -n -f count.sql postgres -t1000

This is pretty much a best case for this patch. A "count(*)" doesn't do 
much else than put the tuple in the slot, and the tuples are wide 
enough, that you switch from one buffer to another every 15 tuples, 
which exercises the buffer pinning code.


I ran that pgbench command three times with and without the patch, and 
picked the best times:


Without patch:
tps = 12.413360 (excluding connections establishing)

With the patch:
tps = 13.183164 (excluding connections establishing)

That's about 6% improvement.

This was with the attached version of this patch. Compared to the 
previous, I added ExecClearTuple() calls to clear the slots before 
advancing the heap/index scan, to avoid the dangling pointers. Doing 
that added an extra function call to this hot codepath, however, so I 
compensated for that by adding an inline version of ExecStoreTuple(), 
for those callers that know that the slot is already empty.


BTW, turning ExecStoreVirtualTuple into a static inline function would 
also be an easy way to shave some cycles. But I refrained from including 
that change into this patch.



I'll start a different thread on that after
some more experimentation, but if we e.g. get rid of
ExecMaterializeSlot() in its current form, would that be OK?


INSERT/UPDATE, for one, relies on that.


Sure. I'm thinking of refactoring that so that it doesn't. Or maybe add 
a new kind of a TupleTableSlot that cannot be materialized, which makes 
ExecStore/ClearTuple for that slot simpler, and use that in SeqScan and 
friends. INSERT/UPDATE could continue to use ExecMaterializeSlot(), but 
it would need to have a slot of its own, instead of materializing the 
slot it got from plan tree. Yes, this is still very hand-wavey...


- Heikki

commit 6f047ac81172c7ea559c9992bfbc1e05a4c3bedd
Author: Heikki Linnakangas 
Date:   Tue Aug 30 20:11:25 2016 +0300

Remove support for holding a buffer pin in a TupleTableSlot.

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index daf0438..299fa62 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1387,7 +1387,6 @@ postgresIterateForeignScan(ForeignScanState *node)
 	 */
 	ExecStoreTuple(fsstate->tuples[fsstate->next_tuple++],
    slot,
-   InvalidBuffer,
    false);
 
 	return slot;
@@ -3152,7 +3151,7 @@ store_returning_result(PgFdwModifyState *fmstate,
 			NULL,
 			fmstate->temp_cxt);
 		/* tuple will be deleted when it is cleared from the slot */
-		ExecStoreTuple(newtup, slot, InvalidBuffer, true);
+		ExecStoreTuple(newtup, slot, true);
 	}
 	PG_CATCH();
 	{
@@ -3258,7 +3257,7 @@ 

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

2016-08-30 Thread Andres Freund
On 2016-08-30 11:22:43 +0300, Maksim Milyutin wrote:
> > Hi,
> > 
> > On 2016-08-29 18:22:56 +0300, maksim wrote:
> > > 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.
> > 
> > Could you expand a bit on what you want this for exactly?
> 
> Max goal - to push my extension to postgres core. But now it's ready only
> for 9.5. Prerequisites of this extension are patches presented here.

I'm asking what you want this for. "An extension" isn't a detailed
description...


> > > 2. Patch that enables to interrupt the query executor
> > > (executor_hooks.patch).
> > > This patch enables to hang up hooks on executor function of each node
> > > (ExecProcNode). I define hooks before any node execution and after
> > > execution.
> > > I use this patch to add possibility of query tracing by emitted rows from
> > > any node. I interrupt query executor after any node delivers one or zero
> > > rows to upper node. And after execution of specific number trace steps I 
> > > can
> > > get the query state of traceable backend which will be somewhat
> > > deterministic. I use this possibility for regression tests of my 
> > > extension.
> > 
> > This will increase executor overhead.
> 
> In simple case we have checks on existence of hooks.

That *is* noticeable.


> > I think we'll need to find a way
> > to hide this behind the existing if (instrument) branches.
> 
> And so can be. It doesn't matter for trace mode. But I think instrument
> branch is intended only for collecting statistics by nodes.

I can't follow here. That's all what analyze is about?

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

2016-08-30 Thread Alvaro Herrera
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).  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.

> insert
> delete
> update
> select (6)
> transaction (10)
> declare
> close (2)
> move
> fetch
> create (37)
> drop (36)
> Alter (56)
> import
> truncate
> comment
> security
> copy
> grant
> revoke
> notify
> listen
> unlisten
> load
> cluster
> vacuum
> analyze
> explain
> refresh
> set (2)
> reset
> show
> discard (4)
> reassign
> lock
> checkpoint
> reindex
> prepare
> execute
> deallocate

-- 
Álvaro Herrerahttp://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] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
> And with an actual patch things are better.

Working through this patch, it suddenly strikes me that we are going
about fixing the callers of simple_prompt the wrong way.  The existing
definition with returning a malloc'd string creates a hazard of malloc
failure, and it *also* creates a hazard of not remembering to free the
result.  Moreover, there are almost no callers that want a max result
longer than ~100 bytes.  Seems like it would be a whole lot easier all
around to make the caller supply the buffer, ie typical call would be
roughly

charbuf[100];

simple_prompt("Password: ", buf, sizeof(buf), false);

Callers that want to deal with a malloc'd buffer (all one of them, looks
like) can do it themselves, for basically only one more line than is
needed now.

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] pageinspect: Hash index support

2016-08-30 Thread Alvaro Herrera
Jesper Pedersen wrote:
> Hi,
> 
> Attached is a patch that adds support for hash indexes in pageinspect.
> 
> The functionality (hash_metap, hash_page_stats and hash_page_items) follows
> the B-tree functions.

I suggest that pageinspect functions are more convenient to use via the
get_raw_page interface, that is, instead of reading the buffer
themselves, the buffer is handed over from elsewhere and they receive it
as bytea.  This enables other use cases such as grabbing a page from one
server and examining it in another one.

-- 
Álvaro Herrerahttp://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] sequences and pg_upgrade

2016-08-30 Thread Bruce Momjian
On Tue, Aug 30, 2016 at 08:46:48AM -0400, Peter Eisentraut wrote:
> I think the other solution mentioned in that thread would also work:
> Have pg_upgrade treat sequences more like system catalogs, whose format
> changes between major releases, and transferred them via the
> dump/restore route.  So instead of copying the disk files, issue a
> setval call, and the sequence should be all set up.
> 
> Am I missing anything?

Looks straight-forward to me.

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

2016-08-30 Thread Tom Lane
Fujii Masao  writes:
> I found that pg_xlogdump code for XLOG_GIN_INSERT record with
> GIN_INSERT_ISLEAF flag has the same issue, i.e.,
> "unknown action 0" error is thrown for that record.
> The latest patch fixes this.

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?
I'm inclined to suspect we should restructure this more like

{
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))
{
+   char   *payload = rec + sizeof(ginxlogInsert);
BlockNumber leftChildBlkno;
BlockNumber rightChildBlkno;

leftChildBlkno = BlockIdGetBlockNumber((BlockId) payload);
payload += sizeof(BlockIdData);
rightChildBlkno = BlockIdGetBlockNumber((BlockId) payload);
payload += sizeof(BlockNumber);
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');
... etc etc ...



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] pageinspect: Hash index support

2016-08-30 Thread Jesper Pedersen

Hi,

Attached is a patch that adds support for hash indexes in pageinspect.

The functionality (hash_metap, hash_page_stats and hash_page_items) 
follows the B-tree functions.


This patch will need an update once Amit's and Mithun's work on 
Concurrent Hash Indexes is committed to account for the new meta-page 
constants.


I'll create a CommitFest entry for this submission.

Feedback is most welcome.

Best regards,
 Jesper
>From 55262d5fa3822afbae94f4989627dd65e91fe098 Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Fri, 5 Aug 2016 10:16:32 -0400
Subject: [PATCH] pageinspect: Hash index support

---
 contrib/pageinspect/Makefile  |  10 +-
 contrib/pageinspect/hashfuncs.c   | 472 ++
 contrib/pageinspect/pageinspect--1.5--1.6.sql |  64 
 contrib/pageinspect/pageinspect--1.5.sql  | 279 ---
 contrib/pageinspect/pageinspect--1.6.sql  | 339 ++
 contrib/pageinspect/pageinspect.control   |   2 +-
 doc/src/sgml/pageinspect.sgml | 100 ++
 7 files changed, 981 insertions(+), 285 deletions(-)
 create mode 100644 contrib/pageinspect/hashfuncs.c
 create mode 100644 contrib/pageinspect/pageinspect--1.5--1.6.sql
 delete mode 100644 contrib/pageinspect/pageinspect--1.5.sql
 create mode 100644 contrib/pageinspect/pageinspect--1.6.sql

diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index a98237e..c73d728 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -2,13 +2,13 @@
 
 MODULE_big	= pageinspect
 OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o \
-		  brinfuncs.o ginfuncs.o $(WIN32RES)
+		  brinfuncs.o ginfuncs.o hashfuncs.o $(WIN32RES)
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.5.sql pageinspect--1.4--1.5.sql \
-	pageinspect--1.3--1.4.sql pageinspect--1.2--1.3.sql \
-	pageinspect--1.1--1.2.sql pageinspect--1.0--1.1.sql \
-	pageinspect--unpackaged--1.0.sql
+DATA = pageinspect--1.6.sql pageinspect--1.5--1.6.sql \
+	pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
+	pageinspect--1.2--1.3.sql pageinspect--1.1--1.2.sql \
+	pageinspect--1.0--1.1.sql pageinspect--unpackaged--1.0.sql
 PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
 
 ifdef USE_PGXS
diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c
new file mode 100644
index 000..598b7b2
--- /dev/null
+++ b/contrib/pageinspect/hashfuncs.c
@@ -0,0 +1,472 @@
+/*
+ * hashfuncs.c
+ *		Functions to investigate the content of HASH indexes
+ *
+ * Copyright (c) 2016, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		contrib/pageinspect/hashfuncs.c
+ */
+
+#include "postgres.h"
+
+#include "access/hash.h"
+#include "access/htup_details.h"
+#include "catalog/namespace.h"
+#include "catalog/pg_am.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "utils/builtins.h"
+#include "utils/rel.h"
+
+
+PG_FUNCTION_INFO_V1(hash_metap);
+PG_FUNCTION_INFO_V1(hash_page_items);
+PG_FUNCTION_INFO_V1(hash_page_stats);
+
+#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
+#define IS_HASH(r) ((r)->rd_rel->relam == HASH_AM_OID)
+
+/* note: BlockNumber is unsigned, hence can't be negative */
+#define CHECK_RELATION_BLOCK_RANGE(rel, blkno) { \
+		if ( RelationGetNumberOfBlocks(rel) <= (BlockNumber) (blkno) ) \
+			 elog(ERROR, "block number out of range"); }
+
+/* 
+ * structure for single hash page statistics
+ * 
+ */
+typedef struct HashPageStat
+{
+	uint32		blkno;
+	uint32		live_items;
+	uint32		dead_items;
+	uint32		page_size;
+	uint32		max_avail;
+	uint32		free_size;
+	uint32		avg_item_size;
+	char		type;
+
+	/* opaque data */
+	BlockNumber hasho_prevblkno;
+	BlockNumber hasho_nextblkno;
+	Bucket		hasho_bucket;
+	uint16		hasho_flag;
+	uint16		hasho_page_id;
+} HashPageStat;
+
+
+/* -
+ * GetHashPageStatistics()
+ *
+ * Collect statistics of single hash page
+ * -
+ */
+static void
+GetHashPageStatistics(BlockNumber blkno, Buffer buffer, HashPageStat *stat)
+{
+	Page		page = BufferGetPage(buffer);
+	PageHeader	phdr = (PageHeader) page;
+	OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
+	HashPageOpaque opaque = (HashPageOpaque) PageGetSpecialPointer(page);
+	int			item_size = 0;
+	int			off;
+
+	stat->blkno = blkno;
+
+	stat->max_avail = BLCKSZ - (BLCKSZ - phdr->pd_special + SizeOfPageHeaderData);
+
+	stat->dead_items = stat->live_items = 0;
+
+	stat->page_size = PageGetPageSize(page);
+
+	/* page type (flags) */
+	if (opaque->hasho_flag & LH_UNUSED_PAGE)
+		stat->type = 'u';
+	else if (opaque->hasho_flag & LH_OVERFLOW_PAGE)
+		stat->type = 'v';
+	else if (opaque->hasho_flag & LH_BUCKET_PAGE)
+		stat->type = 'b';
+	else if (opaque->hasho_flag & LH_BITMAP_PAGE)
+		stat->type = 'i';
+	else
+		stat->type 

[HACKERS] some requests on auditing

2016-08-30 Thread Pavel Stehule
Hi

I am working on pgaudit customization for one my customer.

There are few requests:

1. flat format without complex types, without nesting - CSV is ideal.
2. all important attributes should be separated - is not possible to search
in original queries: table name, database name, role name, rights.
3. if it is possible - own log file
4. one statement can have more rows (flat format is required), but it
should be logged only once success/failed
5. any activity should be logged

The point @4 is hard to implement - static audit should be linked with
result together. There is not any top level hook.

Regards

Pavel


Re: [HACKERS] standalone backend PANICs during recovery

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
> Does the attached suit better then?

Thinking about it some more ... what we actually need to prevent, AFAICS,
is standby_mode becoming true in a standalone backend.  If you don't set
that, then the process will do PITR recovery, but I'm not aware that there
is any problem with running that standalone, and maybe it's useful to
allow it for debugging purposes.  So I'm thinking more like

/*
 * Check for recovery control file, and if so set up state for offline
 * recovery
 */
readRecoveryCommandFile();

+   /*
+* We don't support standby_mode in standalone backends; that
+* requires other processes such as WAL receivers to be alive.
+*/
+   if (StandbyModeRequested && !IsUnderPostmaster)
+   ereport(FATAL, ...);
+
/*
 * Save archive_cleanup_command in shared memory so that other processes
 * can see it.
 */

or we could put the check near the bottom of readRecoveryCommandFile.
Not sure which placement is clearer.

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

2016-08-30 Thread Tom Lane
Aleksander Alekseev  writes:
> I suggest to keep ShmemAlloc as is for backward compatibility and
> introduce a new procedure ShmemAllocSafe.

I think that's about the worst of all possible worlds, as it guarantees
having to touch most call sites.  If there were more than one known caller
that really wanted the return-NULL behavior, exact backwards compatibility
might carry the day; but as things stand I think the odds are that most
call sites need an error check and probably have not got one.  I'd rather
err in favor of "safe by default" than "backwards compatible".

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

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
> On Tue, Aug 30, 2016 at 10:18 PM, Tom Lane  wrote:
>> I think what we ought to do is make ShmemAlloc act like palloc
>> (ie throw error not return NULL), and remove the duplicated error
>> checks.

> The only reason why I did not propose that for ShmemAlloc is because
> of extensions potentially using this routine and having some special
> handling when it returns NULL. And changing it to behave like palloc
> would break such extensions.

The evidence from the callers in core suggests that this change
would be much more likely to fix extensions than break them,
ie it's more likely that they are missing error checks than that
they have something useful to do if the alloc fails.

An extension that actually does need that could do something like

#if CATALOG_VERSION_NO < whatever-v10-is
#define ShmemAllocNoError(x) ShmemAlloc(x)
#endif

...

ptr = ShmemAllocNoError(size);
if (ptr == NULL)  // same as before from here on


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] standalone backend PANICs during recovery

2016-08-30 Thread Michael Paquier
On Tue, Aug 30, 2016 at 10:24 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Tue, Aug 30, 2016 at 9:48 PM, Tom Lane  wrote:
>>> Hm, StartupXLOG seems like a pretty random place to check that, especially
>>> since doing it there requires an extra stat() call.  Why didn't you just
>>> make readRecoveryCommandFile() error out?
>
>> Well, the idea is to do the check before doing anything on PGDATA and
>> leave it intact, particularly the post-crash fsync().
>
> I don't see anything very exciting between the beginning of StartupXLOG
> and readRecoveryCommandFile.  In particular, doing the fsync seems like
> a perfectly harmless and maybe-good thing.  If there were some operation
> with potentially bad side-effects in that range, it would be dangerous
> anyway because of the risk of readRecoveryCommandFile erroring out due
> to invalid contents of recovery.conf.

Does the attached suit better then?
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index acd95aa..5426f75 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4957,6 +4957,16 @@ readRecoveryCommandFile(void)
  errmsg("could not open recovery command file \"%s\": %m",
 		RECOVERY_COMMAND_FILE)));
 	}
+	else if (!IsPostmasterEnvironment)
+	{
+		/*
+		 * Prevent standalone process to start if recovery is wanted. A lot of
+		 * code paths in recovery depend on the assumption that it is not the
+		 * case so recovery would just badly fail.
+		 */
+		ereport(FATAL,
+(errmsg("recovery.conf is not allowed in a standalone process")));
+	}
 
 	/*
 	 * Since we're asking ParseConfigFp() to report errors as FATAL, there's

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

2016-08-30 Thread Aleksander Alekseev
> > I think what we ought to do is make ShmemAlloc act like palloc
> > (ie throw error not return NULL), and remove the duplicated error
> > checks.  For the one caller that that would be bad for, we could
> > invent something like ShmemAllocNoError, or ShmemAllocExtended with
> > a no_error flag, or whatever other new API suits your fancy.  But
> > as-is, it's just inviting more errors-of-omission like the large
> > number that already exist.  I think people are conditioned by palloc
> > to expect such functions to throw error.
> 
> The only reason why I did not propose that for ShmemAlloc is because
> of extensions potentially using this routine and having some special
> handling when it returns NULL. And changing it to behave like palloc
> would break such extensions.

I suggest to keep ShmemAlloc as is for backward compatibility and
introduce a new procedure ShmemAllocSafe. Also we could add a scary
comment (and also a warning log message?) that ShmemAlloc is deprecated
and possibly will be removed in later releases.

-- 
Best regards,
Aleksander Alekseev


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

2016-08-30 Thread Michael Paquier
On Tue, Aug 30, 2016 at 10:18 PM, Tom Lane  wrote:
> I think what we ought to do is make ShmemAlloc act like palloc
> (ie throw error not return NULL), and remove the duplicated error
> checks.  For the one caller that that would be bad for, we could
> invent something like ShmemAllocNoError, or ShmemAllocExtended with
> a no_error flag, or whatever other new API suits your fancy.  But
> as-is, it's just inviting more errors-of-omission like the large
> number that already exist.  I think people are conditioned by palloc
> to expect such functions to throw error.

The only reason why I did not propose that for ShmemAlloc is because
of extensions potentially using this routine and having some special
handling when it returns NULL. And changing it to behave like palloc
would break such extensions.
-- 
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] standalone backend PANICs during recovery

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
> On Tue, Aug 30, 2016 at 9:48 PM, Tom Lane  wrote:
>> Hm, StartupXLOG seems like a pretty random place to check that, especially
>> since doing it there requires an extra stat() call.  Why didn't you just
>> make readRecoveryCommandFile() error out?

> Well, the idea is to do the check before doing anything on PGDATA and
> leave it intact, particularly the post-crash fsync().

I don't see anything very exciting between the beginning of StartupXLOG
and readRecoveryCommandFile.  In particular, doing the fsync seems like
a perfectly harmless and maybe-good thing.  If there were some operation
with potentially bad side-effects in that range, it would be dangerous
anyway because of the risk of readRecoveryCommandFile erroring out due
to invalid contents of recovery.conf.

This might be an argument for re-ordering what we're doing in StartupXLOG,
but that seems like an independent discussion.

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] Aggregate Push Down - Performing aggregation on foreign server

2016-08-30 Thread Pavel Stehule
Hi

2016-08-30 15:02 GMT+02:00 Jeevan Chalke :

> Hi all,
>
> Attached is the patch which adds support to push down aggregation and
> grouping
> to the foreign server for postgres_fdw. Performing aggregation on foreign
> server results into fetching fewer rows from foreign side as compared to
> fetching all the rows and aggregating/grouping locally. Performing
> grouping on
> foreign server may use indexes if available. So pushing down aggregates/
> grouping on foreign server performs better than doing that locally.
> (Attached
> EXPLAIN output for few simple grouping queries, with and without push
> down).
>

is it work without FDW too?. It can be pretty interesting too.

Regards

Pavel


>
> Here are the few details of the implementation
>
> Creating Paths:
>
> Implements the FDW hook GetForeignUpperPaths, which adds foreign scan path
> to
> the output relation when upper relation kind is UPPERREL_GROUP_AGG. This
> path
> represents the aggregation/grouping operations to be performed on the
> foreign
> server. We are able to push down aggregation/grouping if (implemented in
> foreign_grouping_ok()),
> a. Underlying input relation is safe to push down and has no local
> conditions,
> as local conditions need to be applied before aggregation.
> b. All the aggregates, GROUP BY expressions are safe to push down.
> foreign_grouping_ok() functions assesses it.
>
> While checking for shippability, we build the target list which is passed
> to
> the foreign server as fdw_scan_tlist. The target list contains
> a. All the GROUP BY expressions
> b. Shippable entries from the target list of upper relation
> c. Var and Aggref nodes from non-shippable entries from the target list of
> upper relation
> d. Var and Aggref nodes from non-shippable HAVING conditions.
>
> The shippable having conditions are sent to the foreign server as part of
> the
> HAVING clause of the remote SQL.
>
> is_foreign_expr() function, now handles T_Aggref node. Aggregate is safe to
> push down if,
> a. Aggregate is a built-in aggregate
> b. All its arguments are safe to push-down
> c. Other expressions involved like aggorder, aggdistinct, aggfilter etc.
> are
> safe to be pushed down.
>
>
> Costing:
>
> If use_foreign_estimate is true for input relation, like JOIN case, we use
> EXPLAIN output to get the cost of query with aggregation/grouping on the
> foreign server. If not we calculate the costs locally. Similar to core, we
> use
> get_agg_clause_costs() to get costs for aggregation and then using logic
> similar to cost_agg() we calculate startup and total cost. Since we have no
> idea which aggregation strategy will be used at foreign side, we add all
> startup cost (startup cost of input relation, aggregates etc.) into startup
> cost for the grouping path and similarly for total cost.
>
> Deparsing the query:
>
> Target list created while checking for shippability is deparsed using
> deparseExplicitTargetList(). sortgroupref are adjusted according to this
> target list. Most of the logic to deparse an Aggref is inspired from
> get_agg_expr(). For an upper relation, FROM and WHERE clauses come from the
> underlying scan relation and thus for simplicity, FROM clause deparsing
> logic
> is moved from deparseSelectSql() to a new function deparseFromClause(). The
> same function adds WHERE clause to the remote SQL.
>
>
> Area of future work:
>
> 1. Adding path with path-keys to push ORDER BY clause along with
> aggregation/
> grouping.  Should be supported as a separate patch.
>
> 2. Grouping Sets/Rollup/Cube is not supported in current version. I have
> left
> this aside to keep patch smaller. If required I can add that support in the
> next version of the patch.
>
>
> Most of the code in this patch is inspired from the JOIN push down code.
> Ashutosh Bapat provided a high-level design and a skeleton patch to
> start-with
> offlist. Thanks to Tom Lane for his upper-planner pathification work and
> adding
> GetForeignUpperPaths callback function.
>
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 30589500
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>
>
> --
> Sent via pgsql-hackers 

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
>> I've just realized that there is also malloc-compatible ShmemAlloc().
>> Apparently it's return value sometimes is not properly checked too. See
>> attachment.

> The funny part here is that ProcGlobal->allProcs is actually handled,
> but not the two others. Well yes, you are right, we really need to
> fail on FATAL for all of them if ShmemAlloc returns NULL as they
> involve the shmem initialization at postmaster startup.

A quick scan says that most callers are failing to check at all,
several more just have duplicate check-and-immediately-elog code,
and only one has got anything useful to do besides throwing error.

I think what we ought to do is make ShmemAlloc act like palloc
(ie throw error not return NULL), and remove the duplicated error
checks.  For the one caller that that would be bad for, we could
invent something like ShmemAllocNoError, or ShmemAllocExtended with
a no_error flag, or whatever other new API suits your fancy.  But
as-is, it's just inviting more errors-of-omission like the large
number that already exist.  I think people are conditioned by palloc
to expect such functions to throw error.

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] Comment on GatherPath.single_copy

2016-08-30 Thread Tom Lane
Kyotaro HORIGUCHI  writes:
> - boolsingle_copy;/* path must not be executed >1x */
> + boolsingle_copy;/* path must not span on multiple 
> processes */

I agree that the existing comment sucks, but this isn't a lot better
(and it will probably not look nice after pgindent gets done with it).
Possibly it's too complicated to jam a reasonable explanation into the
same-line comment, and what is needed is to expand the sentence about
it in the comment above the struct.

Robert, could you fix the documentation for that field so it's
intelligible?

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] standalone backend PANICs during recovery

2016-08-30 Thread Michael Paquier
On Tue, Aug 30, 2016 at 9:48 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Wed, Aug 24, 2016 at 5:07 PM, Bernd Helmle  wrote:
>>> That said, i'm okay if --single is not intended to bring up a hot standby.
>>> There are many other ways to debug such problems.
>
>> This patch is still on the CF app:
>> https://commitfest.postgresql.org/10/610/
>> Even after thinking about it, my opinion is still the same. Let's
>> prevent a standalone backend to start if it sees recovery.conf.
>> Attached is a patch and the CF entry is switched as ready for
>> committer to move on.
>
> Hm, StartupXLOG seems like a pretty random place to check that, especially
> since doing it there requires an extra stat() call.  Why didn't you just
> make readRecoveryCommandFile() error out?

Well, the idea is to do the check before doing anything on PGDATA and
leave it intact, particularly the post-crash fsync().
-- 
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] sequences and pg_upgrade

2016-08-30 Thread Tom Lane
Peter Eisentraut  writes:
> I was toying with a couple of ideas that would involve changing the
> storage of sequences.  (Say, for the sake of discussion, removing the
> problematic/useless sequence_name field.)  This would cause problems for
> pg_upgrade, because pg_upgrade copies the "heap" storage of sequences
> like it does for normal tables, and we have no facilities for effecting
> any changes during that.

> There was a previous discussion in the early days of pg_migrator, which
> resulted in the current behavior:
> https://www.postgresql.org/message-id/flat/20090713220112.GF7933%40klana.box

> This also alluded to what I think was the last change in the sequence
> storage format (10a3471bed7b57fb986a5be8afdee5f0dda419de) between
> versions 8.3 and 8.4.  How did pg_upgrade handle that?

I think it probably never did handle that.  pg_upgrade doesn't currently
claim to support migrating from 8.3, and the thread you mention shows that
the original attempt at 8.3->8.4 migration crashed-and-burned for numerous
unrelated reasons.  We may not have ever got to the point of noticing that
10a3471be also created a problem.

> I think the other solution mentioned in that thread would also work:
> Have pg_upgrade treat sequences more like system catalogs, whose format
> changes between major releases, and transferred them via the
> dump/restore route.  So instead of copying the disk files, issue a
> setval call, and the sequence should be all set up.

Seems reasonable.

If you're proposing to expose --sequence-data as a user-visible option,
the patch set lacks documentation.  But I wonder whether it shouldn't
simply be a side-effect of --binary-upgrade.  It seems a tad
non-orthogonal for a user switch.

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] standalone backend PANICs during recovery

2016-08-30 Thread Tom Lane
Michael Paquier  writes:
> On Wed, Aug 24, 2016 at 5:07 PM, Bernd Helmle  wrote:
>> That said, i'm okay if --single is not intended to bring up a hot standby.
>> There are many other ways to debug such problems.

> This patch is still on the CF app:
> https://commitfest.postgresql.org/10/610/
> Even after thinking about it, my opinion is still the same. Let's
> prevent a standalone backend to start if it sees recovery.conf.
> Attached is a patch and the CF entry is switched as ready for
> committer to move on.

Hm, StartupXLOG seems like a pretty random place to check that, especially
since doing it there requires an extra stat() call.  Why didn't you just
make readRecoveryCommandFile() error out?

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] sequences and pg_upgrade

2016-08-30 Thread Peter Eisentraut
I was toying with a couple of ideas that would involve changing the
storage of sequences.  (Say, for the sake of discussion, removing the
problematic/useless sequence_name field.)  This would cause problems for
pg_upgrade, because pg_upgrade copies the "heap" storage of sequences
like it does for normal tables, and we have no facilities for effecting
any changes during that.

There was a previous discussion in the early days of pg_migrator, which
resulted in the current behavior:
https://www.postgresql.org/message-id/flat/20090713220112.GF7933%40klana.box

This also alluded to what I think was the last change in the sequence
storage format (10a3471bed7b57fb986a5be8afdee5f0dda419de) between
versions 8.3 and 8.4.  How did pg_upgrade handle that?

I think the other solution mentioned in that thread would also work:
Have pg_upgrade treat sequences more like system catalogs, whose format
changes between major releases, and transferred them via the
dump/restore route.  So instead of copying the disk files, issue a
setval call, and the sequence should be all set up.

Am I missing anything?

Attached is a rough patch set that would implement that.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 0c8f9bb630f48e83dc4dbe36e742db8e20f6b523 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 23 Aug 2016 12:00:00 -0400
Subject: [PATCH 1/3] pg_dump: Separate table data and sequence data object
 types

---
 src/bin/pg_dump/pg_dump.c  | 11 +++
 src/bin/pg_dump/pg_dump.h  |  1 +
 src/bin/pg_dump/pg_dump_sort.c |  7 +++
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a5c2d09..160bc41 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2133,6 +2133,8 @@ makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids)
 
 	if (tbinfo->relkind == RELKIND_MATVIEW)
 		tdinfo->dobj.objType = DO_REFRESH_MATVIEW;
+	else if (tbinfo->relkind == RELKIND_SEQUENCE)
+		tdinfo->dobj.objType = DO_SEQUENCE_DATA;
 	else
 		tdinfo->dobj.objType = DO_TABLE_DATA;
 
@@ -9382,11 +9384,11 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj)
 		case DO_TRANSFORM:
 			dumpTransform(fout, (TransformInfo *) dobj);
 			break;
+		case DO_SEQUENCE_DATA:
+			dumpSequenceData(fout, (TableDataInfo *) dobj);
+			break;
 		case DO_TABLE_DATA:
-			if (((TableDataInfo *) dobj)->tdtable->relkind == RELKIND_SEQUENCE)
-dumpSequenceData(fout, (TableDataInfo *) dobj);
-			else
-dumpTableData(fout, (TableDataInfo *) dobj);
+			dumpTableData(fout, (TableDataInfo *) dobj);
 			break;
 		case DO_DUMMY_TYPE:
 			/* table rowtypes and array types are never dumped separately */
@@ -17482,6 +17484,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs,
 addObjectDependency(preDataBound, dobj->dumpId);
 break;
 			case DO_TABLE_DATA:
+			case DO_SEQUENCE_DATA:
 			case DO_BLOB_DATA:
 /* Data objects: must come between the boundaries */
 addObjectDependency(dobj, preDataBound->dumpId);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 2bfa2d9..6cc78d1 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -63,6 +63,7 @@ typedef enum
 	DO_PROCLANG,
 	DO_CAST,
 	DO_TABLE_DATA,
+	DO_SEQUENCE_DATA,
 	DO_DUMMY_TYPE,
 	DO_TSPARSER,
 	DO_TSDICT,
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index d87f08d..9ca3d2b 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -60,6 +60,7 @@ static const int oldObjectTypePriority[] =
 	2,			/* DO_PROCLANG */
 	2,			/* DO_CAST */
 	11,			/* DO_TABLE_DATA */
+	11,			/* DO_SEQUENCE_DATA */
 	7,			/* DO_DUMMY_TYPE */
 	4,			/* DO_TSPARSER */
 	4,			/* DO_TSDICT */
@@ -111,6 +112,7 @@ static const int newObjectTypePriority[] =
 	2,			/* DO_PROCLANG */
 	10,			/* DO_CAST */
 	23,			/* DO_TABLE_DATA */
+	23,			/* DO_SEQUENCE_DATA */
 	19,			/* DO_DUMMY_TYPE */
 	12,			/* DO_TSPARSER */
 	14,			/* DO_TSDICT */
@@ -1433,6 +1435,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize)
 	 "TABLE DATA %s  (ID %d OID %u)",
 	 obj->name, obj->dumpId, obj->catId.oid);
 			return;
+		case DO_SEQUENCE_DATA:
+			snprintf(buf, bufsize,
+	 "SEQUENCE DATA %s  (ID %d OID %u)",
+	 obj->name, obj->dumpId, obj->catId.oid);
+			return;
 		case DO_DUMMY_TYPE:
 			snprintf(buf, bufsize,
 	 "DUMMY TYPE %s  (ID %d OID %u)",
-- 
2.9.3

From 26325789ef3cb0e898d94f06b395ae4c64e3b2e9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 23 Aug 2016 12:00:00 -0400
Subject: [PATCH 2/3] pg_dump: Add --sequence-data option

---
 src/bin/pg_dump/pg_backup.h  |  2 ++
 src/bin/pg_dump/pg_backup_archiver.c |  6 +-
 src/bin/pg_dump/pg_dump.c| 22 ++
 3 files 

Re: [HACKERS] 9.5.4: Segfault (signal 11) while running ALTER TABLE

2016-08-30 Thread Tom Lane
Devrim =?ISO-8859-1?Q?G=FCnd=FCz?=  writes:
> They wanted to change id column from uuid to int, so created this func first:

> CREATE FUNCTION foofunc_id_uuidtoint(chartoconvert uuid) RETURNS integer
> LANGUAGE sql IMMUTABLE STRICT
> AS $_$
> SELECT newid FROM foo1 WHERE tempuuid = $1 LIMIT 1;
> $_$;

> and ran this:

> ALTER TABLE foo1 ALTER COLUMN id TYPE INTEGER USING
> foofunc_id_uuidtoint(tempuuid);

> This command crashed postmaster.

The above isn't ever likely to work for any large value of "work",
because the function would be confused about what the table rowtype
is.  I thought we had adequate defenses in there to throw an error
if you try to access a table that's in the middle of being altered,
but apparently this case isn't covered.

Why didn't they just do
ALTER TABLE foo1 ALTER COLUMN id TYPE INTEGER USING newid;
?  The intermediate function sure seems like the hard way.

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] old_snapshot_threshold documentation

2016-08-30 Thread Kevin Grittner
On Fri, Aug 26, 2016 at 1:34 PM, Peter Eisentraut
 wrote:
> I doubt the documentation for old_snapshot_threshold is going to be
> understood by many ordinary users.  What is a "snapshot", first of all?
> Why would a snapshot be old?  Why is that a problem?  What can I do to
> avoid it?  What are the circumstances in practice where this issue would
> arise, and what are the trade-offs?  Where can I see existing snapshots
> and how old they are?

I'll put something together to improve that.  I think largely that
will consist of references to other sections of the documentation,
like MVCC, vacuuming, pg_stat_activity, etc.

--
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] Postgres abort found in 9.3.11

2016-08-30 Thread Tom Lane
"K S, Sandhya (Nokia - IN/Bangalore)"  writes:
> During the server restart, we are getting postgres crash with sigabrt. No 
> other operation being performed.
> Attached the backtrace.

What shows up in the postmaster log?

> The occurrence is occasional. The issue is seen once in 30~50 times.

Does it successfully restart if you try again?  If not, what are you
doing to recover?

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] Pinning a buffer in TupleTableSlot is unnecessary

2016-08-30 Thread Tom Lane
Heikki Linnakangas  writes:
> While profiling some queries and looking at executor overhead, I 
> realized that we're not making much use of TupleTableSlot's ability to 
> hold a buffer pin. In a SeqScan, the buffer is held pinned by the 
> underlying heap-scan anyway. Same with an IndexScan, and the SampleScan. 

I think this is probably wrong, or at least very dangerous to remove.
The reason for the feature is that the slot may continue to point at
the tuple after the scan has moved on.  You've given no evidence at all
that that scenario doesn't arise (or that we wouldn't need it in future).

At the very least I'd want to see this patch clearing the slot before
advancing the scan, so that it doesn't have actual dangling pointers
laying about.

> So, how about we remove the ability of a TupleTableSlot to hold a buffer 
> pin, per the attached patch? It shaves a few cycles from a 
> ExecStoreTuple() and ExecClearTuple(), which get called a lot. I 
> couldn't measure any actual difference from that, though, but it seems 
> like a good idea from a readability point of view, anyway.

If you can't measure a clear performance improvement, I'm -1 on the
whole thing.  You've got risk and breakage of third-party code, and
what to show for it?

> I'll start a different thread on that after 
> some more experimentation, but if we e.g. get rid of 
> ExecMaterializeSlot() in its current form, would that be OK?

INSERT/UPDATE, for one, relies on 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


[HACKERS] Postgres abort found in 9.3.11

2016-08-30 Thread K S, Sandhya (Nokia - IN/Bangalore)
Hello,

During the server restart, we are getting postgres crash with sigabrt. No other 
operation being performed.
Attached the backtrace.


The occurrence is occasional. The issue is seen once in 30~50 times.
Recently we had performed postgres upgrade from 9.3.9 to 9.3.11. The issue is 
not seen in 9.3.9.

Postgres server version: 9.3.11
Target architecture: mips-64

We checked the difference between 9.3.9 and 9.3.11 and nothing relevant seems 
to be causing the crash.

Please help in resolving the issue as we are not competent with the postgres 
code.
Also if you see any difference valid be 9.3.9 and 9.3.11 which might be 
pointing to this issue, please let us know.

Thanks in advance!!!
Sandhya



#0  0x005558e909c0 in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x005558e909c0 in raise () from /lib64/libc.so.6
#1  0x005558e952bc in abort () from /lib64/libc.so.6
#2  0x00012039db88 in errfinish ()
#3  0x00012039e868 in elog_finish ()
#4  0x00012009ea08 in btree_redo ()
#5  0x0001200cb178 in StartupXLOG ()
#6  0x000120259838 in StartupProcessMain ()
#7  0x0001200d5b3c in AuxiliaryProcessMain ()
#8  0x000120253314 in ?? ()


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


[HACKERS] Pinning a buffer in TupleTableSlot is unnecessary

2016-08-30 Thread Heikki Linnakangas
While profiling some queries and looking at executor overhead, I 
realized that we're not making much use of TupleTableSlot's ability to 
hold a buffer pin. In a SeqScan, the buffer is held pinned by the 
underlying heap-scan anyway. Same with an IndexScan, and the SampleScan. 
The only thing that relies on that feature is TidScan, but we could 
easily teach TidScan to hold the buffer pin directly.


So, how about we remove the ability of a TupleTableSlot to hold a buffer 
pin, per the attached patch? It shaves a few cycles from a 
ExecStoreTuple() and ExecClearTuple(), which get called a lot. I 
couldn't measure any actual difference from that, though, but it seems 
like a good idea from a readability point of view, anyway.


How much do we need to worry about breaking extensions? I.e. to what 
extent do we consider the TupleTableSlot API to be stable, for 
extensions to use? The FDW API uses TupleTableSlots - this patch had to 
fix the ExecStoreTuple calls in postgres_fdw.


We could refrain from changing the signature of ExecStoreTuple(), and 
throw an error if you try to pass a valid buffer to it. But I also have 
some bigger changes to TupleTableSlot in mind. I think we could gain 
some speed by making slots "read-only". For example, it would be nice if 
a SeqScan could just replace the tts_tuple pointer in the slot, and not 
have to worry that the upper nodes might have materialized the slot, and 
freeing the copied tuple. Because that happens very rarely in practice. 
It would be better if those few places where we currently materialize an 
existing slot, we would create a copy of the slot, and leave the 
original slot unmodified. I'll start a different thread on that after 
some more experimentation, but if we e.g. get rid of 
ExecMaterializeSlot() in its current form, would that be OK?


- Heikki
commit 44ba53760e83921817b4878e48a2e4ad2fd67493
Author: Heikki Linnakangas 
Date:   Tue Aug 30 13:06:59 2016 +0300

Remove support for holding a buffer pin in a TupleTableSlot.

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index daf0438..299fa62 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1387,7 +1387,6 @@ postgresIterateForeignScan(ForeignScanState *node)
 	 */
 	ExecStoreTuple(fsstate->tuples[fsstate->next_tuple++],
    slot,
-   InvalidBuffer,
    false);
 
 	return slot;
@@ -3152,7 +3151,7 @@ store_returning_result(PgFdwModifyState *fmstate,
 			NULL,
 			fmstate->temp_cxt);
 		/* tuple will be deleted when it is cleared from the slot */
-		ExecStoreTuple(newtup, slot, InvalidBuffer, true);
+		ExecStoreTuple(newtup, slot, true);
 	}
 	PG_CATCH();
 	{
@@ -3258,7 +3257,7 @@ get_returning_data(ForeignScanState *node)
 dmstate->retrieved_attrs,
 NULL,
 dmstate->temp_cxt);
-			ExecStoreTuple(newtup, slot, InvalidBuffer, false);
+			ExecStoreTuple(newtup, slot, false);
 		}
 		PG_CATCH();
 		{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b0b43cf..91895fd 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2531,7 +2531,7 @@ IndexBuildHeapRangeScan(Relation heapRelation,
 		MemoryContextReset(econtext->ecxt_per_tuple_memory);
 
 		/* Set up for predicate or expression evaluation */
-		ExecStoreTuple(heapTuple, slot, InvalidBuffer, false);
+		ExecStoreTuple(heapTuple, slot, false);
 
 		/*
 		 * In a partial index, discard tuples that don't satisfy the
@@ -2679,7 +2679,7 @@ IndexCheckExclusion(Relation heapRelation,
 		MemoryContextReset(econtext->ecxt_per_tuple_memory);
 
 		/* Set up for predicate or expression evaluation */
-		ExecStoreTuple(heapTuple, slot, InvalidBuffer, false);
+		ExecStoreTuple(heapTuple, slot, false);
 
 		/*
 		 * In a partial index, ignore tuples that don't satisfy the predicate.
@@ -3100,7 +3100,7 @@ validate_index_heapscan(Relation heapRelation,
 			MemoryContextReset(econtext->ecxt_per_tuple_memory);
 
 			/* Set up for predicate or expression evaluation */
-			ExecStoreTuple(heapTuple, slot, InvalidBuffer, false);
+			ExecStoreTuple(heapTuple, slot, false);
 
 			/*
 			 * In a partial index, discard tuples that don't satisfy the
diff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.c
index b9fe102..faad4b9 100644
--- a/src/backend/catalog/indexing.c
+++ b/src/backend/catalog/indexing.c
@@ -96,7 +96,7 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple)
 
 	/* Need a slot to hold the tuple being examined */
 	slot = MakeSingleTupleTableSlot(RelationGetDescr(heapRelation));
-	ExecStoreTuple(heapTuple, slot, InvalidBuffer, false);
+	ExecStoreTuple(heapTuple, slot, false);
 
 	/*
 	 * for each index, form and insert the index tuple
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index c617abb..6137dda 100644
--- a/src/backend/commands/analyze.c
+++ 

Re: [HACKERS] Patch: Implement failover on libpq connect level.

2016-08-30 Thread Mithun Cy
On Fri, Aug 26, 2016 at 10:10 AM, Mithun Cy 
wrote:
>
> >rpath,'/home/mithun/edbsrc/patch6bin/lib',--enable-new-dtags  -lecpg
> -lpgtypes -lpq -lpgcommon -lpgport -lz -lrt -lcrypt -ldl -lm   -o test1
> >../../../../../src/interfaces/libpq/libpq.so: undefined reference to
> `pg_usleep'
>

As similar to psql I have added -lpq for compilation. So now ecpg tests
compiles and make check-world has passed.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


libpq-failover-ecpg-make-01.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


[HACKERS] 9.5.4: Segfault (signal 11) while running ALTER TABLE

2016-08-30 Thread Devrim Gündüz

Hi,

I received an email offlist about a crash that a non-customer experienced
recently. I asked them to send a few details about the crash, so here it is.
Please note that I don't have access to their systems, but they will be fast
enough to provide data if needed. (They asked me to mask some of the table/func
names)

They have a domain called primaryuuid:

CREATE DOMAIN primaryuuid AS uuid NOT NULL DEFAULT uuid_generate_v4();

and a table called foo1, which used to have this domain as a data type:

CREATE TABLE foo1 (
id public.primaryuuid NOT NULL,
 
    tempuuid uuid
);

They wanted to change id column from uuid to int, so created this func first:

CREATE FUNCTION foofunc_id_uuidtoint(chartoconvert uuid) RETURNS integer
LANGUAGE sql IMMUTABLE STRICT
AS $_$
SELECT newid FROM foo1 WHERE tempuuid = $1 LIMIT 1;
$_$;


and ran this:

ALTER TABLE foo1 ALTER COLUMN id TYPE INTEGER USING
foofunc_id_uuidtoint(tempuuid);

This command crashed postmaster. They connected to related process once again
with gdb, and got this:

===
Program received signal SIGSEGV, Segmentation fault.
slot_deform_tuple (slot=0x1b80440, natts=13) at heaptuple.c:1157
1157values[attnum] = fetchatt(thisatt, tp + off);
Continuing.
===

Below are some lines in postgresql.log:

==
ERROR:  canceling autovacuum task
CONTEXT:  automatic vacuum of table "foo1"
process 10884 acquired AccessExclusiveLock on relation 16961 of database 16401
after 1000.411 ms
STATEMENT:  ALTER TABLE foo1 ALTER COLUMN id TYPE INTEGER USING
foofunc_id_uuidtoint(tempuuid);
LOG:  server process (PID 10884) was terminated by signal 11: Segmentation
fault
==

Please note that this crash is not specific to this table. Various attempts to 
change this data type on various tables also crashed postmaster.

This happened a few times, and after disabling autovacuum and lowering
maintenance_work_mem from 4GB to 2GB, they could complete all ALTER TABLE
commands.

This is PostgreSQL 9.5.4 on CentOS 6.8, fully up2date, and they are using the
community RPMs. 

Any ideas what is causing this crash?

Please let me know if you need more data.

Regards,
-- 
Devrim GÜNDÜZ
EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR


signature.asc
Description: This is a digitally signed message part


Re: [HACKERS] pg_dump with tables created in schemas created by extensions

2016-08-30 Thread Martín Marqués
2016-08-30 2:02 GMT-03:00 Michael Paquier :
> On Tue, Aug 30, 2016 at 5:43 AM, Martín Marqués  
> wrote:
>> This is v4 of the patch, which is actually a cleaner version from the
>> v2 one Michael sent.
>>
>> I stripped off the external index created from the tests as that index
>> shouldn't be dumped (table it belongs to isn't dumped, so neither
>> should the index). I also took off a test which was duplicated.
>>
>> I think this patch is a very good first approach. Future improvements
>> can be made for indexes, but we need to get the extension dependencies
>> right first. That could be done later, on a different patch.
>>
>> Thoughts?
>
> Let's do as you suggest then, and just focus on the schema issue. I
> just had an extra look at the patch and it looks fine to me. So the
> patch is now switched as ready for committer.

That's great. Thanks for all Michael


-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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


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

2016-08-30 Thread Maksim Milyutin

Hi,

On 2016-08-29 18:22:56 +0300, maksim wrote:

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.


Could you expand a bit on what you want this for exactly?


Max goal - to push my extension to postgres core. But now it's ready 
only for 9.5. Prerequisites of this extension are patches presented here.



2. Patch that enables to interrupt the query executor
(executor_hooks.patch).
This patch enables to hang up hooks on executor function of each node
(ExecProcNode). I define hooks before any node execution and after
execution.
I use this patch to add possibility of query tracing by emitted rows from
any node. I interrupt query executor after any node delivers one or zero
rows to upper node. And after execution of specific number trace steps I can
get the query state of traceable backend which will be somewhat
deterministic. I use this possibility for regression tests of my extension.


This will increase executor overhead.


In simple case we have checks on existence of hooks. We may suppose to 
use only not heavy processing on hooks under regular execution of query. 
In my case (query trace), I set up hooks under trace mode and throw off 
otherwise.


> I think we'll need to find a way
> to hide this behind the existing if (instrument) branches.

And so can be. It doesn't matter for trace mode. But I think instrument 
branch is intended only for collecting statistics by nodes.



3. Patch that enables to output runtime explain statistics
(runtime_explain.patch).
This patch extends the regular explain functionality. The problem is in the
point that regular explain call makes result output after query execution
performing InstrEndLoop on nodes where necessary. My patch introduces
specific flag *runtime* that indicates whether we explain running query and
does some insertions in source code dedicated to output the statistics of
running query.


Unless I'm missing something this doesn't really expose a user of this
functionality?


This patch is only for extension. As described in 
https://www.postgresql.org/message-id/CACACo5Sz7G0MFauC082iM=xx_hq7qq5ndr4jpo+h-o5vp6i...@mail.gmail.com 
regular ExplainNode() prints only statistics after query execution and 
asserts under InstEndLoop(). My patch releases this problem and rewrite 
formulas for statistic parameters appropriate to running queries without 
affecting regular EXPLAIN outputs.



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

2016-08-30 Thread Aleksander Alekseev
> >> And with an actual patch things are better.
> >
> > Currently I can't think of any further improvements. I even would dare
> > to say that patch is Ready for Committer.
> 
> Thanks for the fruitful input by the way! You spotted many things.

Thank _you_ for paying attention for such issues in PostgreSQL code!

-- 
Best regards,
Aleksander Alekseev


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

2016-08-30 Thread Michael Paquier
On Tue, Aug 30, 2016 at 5:08 PM, Aleksander Alekseev
 wrote:
>> > The funny part here is that ProcGlobal->allProcs is actually handled,
>> > but not the two others. Well yes, you are right, we really need to
>> > fail on FATAL for all of them if ShmemAlloc returns NULL as they
>> > involve the shmem initialization at postmaster startup.
>>
>> And with an actual patch things are better.
>
> Currently I can't think of any further improvements. I even would dare
> to say that patch is Ready for Committer.

Thanks for the fruitful input by the way! You spotted many things.
-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Aleksander Alekseev
> > The funny part here is that ProcGlobal->allProcs is actually handled,
> > but not the two others. Well yes, you are right, we really need to
> > fail on FATAL for all of them if ShmemAlloc returns NULL as they
> > involve the shmem initialization at postmaster startup.
> 
> And with an actual patch things are better.

Currently I can't think of any further improvements. I even would dare
to say that patch is Ready for Committer.

-- 
Best regards,
Aleksander Alekseev


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

2016-08-30 Thread Fujii Masao
On Tue, Aug 30, 2016 at 3:13 PM, Fujii Masao  wrote:
> On Tue, Aug 30, 2016 at 3:39 AM, Tom Lane  wrote:
>> Fujii Masao  writes:
>>> ISTM that the cause of this issue is that gin_desc() uses XLogRecGetData() 
>>> to
>>> extract ginxlogVacuumDataLeafPage data from XLOG_GIN_VACUUM_DATA_LEAF_PAGE
>>> record. Since it's registered by XLogRegisterBufData() in
>>> ginVacuumPostingTreeLeaf(),
>>> XLogRecGetBlockData() should be used, instead. Patch attached. Thought?
>>
>> I think we probably have more issues than that.  See for example
>> https://www.postgresql.org/message-id/flat/20160826072658.15676.7628%40wrigleys.postgresql.org
>>
>> which clearly shows that the replay logic is seeing something wrong too:
>>
>> 2016-08-26 06:01:50 UTC FATAL:  unexpected GIN leaf action: 0
>> 2016-08-26 06:01:50 UTC CONTEXT:  xlog redo Insert item, node:
>> 1663/16387/33108 blkno: 6622 isdata: T isleaf: T 3 segments: 2 (add 0 items)
>> 0 unknown action 0 ???
>>
>> If it were just a matter of gin_desc() being wrong, we'd not have
>> gotten such a failure.  (Which is not to say that gin_desc() isn't
>> wrong; it may well be.)
>
> Yeah, I got the pg_xlogdump issue while I was trying to reproduce
> the above reported problem. Fixing pg_xlogdump would be helpful for
> the analysis of that problem.

Attached is the updated version of the patch.

I found that pg_xlogdump code for XLOG_GIN_INSERT record with
GIN_INSERT_ISLEAF flag has the same issue, i.e.,
"unknown action 0" error is thrown for that record.
The latest patch fixes this.

Regards,

-- 
Fujii Masao
*** a/src/backend/access/rmgrdesc/gindesc.c
--- b/src/backend/access/rmgrdesc/gindesc.c
***
*** 110,116  gin_desc(StringInfo buf, XLogReaderState *record)
  else if (xlrec->flags & GIN_INSERT_ISLEAF)
  {
  	ginxlogRecompressDataLeaf *insertData =
! 	(ginxlogRecompressDataLeaf *) payload;
  
  	if (XLogRecHasBlockImage(record, 0))
  		appendStringInfoString(buf, " (full page image)");
--- 110,116 
  else if (xlrec->flags & GIN_INSERT_ISLEAF)
  {
  	ginxlogRecompressDataLeaf *insertData =
! 		(ginxlogRecompressDataLeaf *) XLogRecGetBlockData(record, 0, NULL);
  
  	if (XLogRecHasBlockImage(record, 0))
  		appendStringInfoString(buf, " (full page image)");
***
*** 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] [WIP] Patches to enable extraction state of query execution from external session

2016-08-30 Thread Maksim Milyutin

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.

Patch *runtime_explain.patch* releases the problem with error from 
InstrEndLoop(). I catch all places where this unlucky function is called 
and wrap in checks on *runtime* flag. This flag indicates whether 
*ExplainQuery* is called for running query. Also I complement explain 
output, you can see details in README.md in repository.


--
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] RLS related docs

2016-08-30 Thread Dean Rasheed
On 28 August 2016 at 21:23, Joe Conway  wrote:
> Apologies for the delay, but new patch attached. Assuming no more
> comments, will commit this, backpatched to 9.5, in a day or two.
>

Looking at this again, I think there is something fishy about these
dump/restore flags.

If you do pg_dump --enable-row-security, then row_security is turned
on during the dump and only the user-visible portions of the tables
are dumped. But why does such a dump emit "SET row_security = on;" as
part of the dump? There doesn't appear to be any reason for having
row_security turned on during the restore just because it was on
during the dump. The INSERT policies may well be different from the
SELECT policies, and so this may lead to a dump that cannot be
restored. ISTM that row_security should be off inside the dump, and
only enabled during restore if the user explicitly asks for it,
regardless of what setting was used to produce the dump.

Also, isn't it the case that --enable-row-security during pg_restore
is only relevant when performing a data-only restore (like
--disable-triggers). Otherwise, it looks to me as though the restore
will create the tables, restore the data, and then only at the end
restore the table policies and enable row level security on the
tables. So it looks like the flag would have no effect (and a
COPY-format dump would work fine) for a non-data-only dump.

I never really looked at the RLS dump/restore code. Am I missing something?

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] multivariate statistics (v19)

2016-08-30 Thread Michael Paquier
On Wed, Aug 24, 2016 at 2:03 AM, Robert Haas  wrote:
> ISTR that you were going to try to look at this patch set.  It seems
> from the discussion that it's not really ready for serious
> consideration for commit yet, but also that some high-level design
> comments from you at this stage could go a long way toward making sure
> that the final form of the patch is something that will be acceptable.
>
> I'd really like to see us get some kind of capability along these
> lines, but I'm sure it will go a lot better if you or Dean handle it
> than if I try to do it ... not to mention that there are only so many
> hours in the day.

Agreed. What I have been able to look until now was the high-level
structure of the patch, and I think that we should really shave 0002
and simplify it to get a core infrastructure in place, but the core
patch is at another level, and it would be good to get some feedback
regarding the structure of the patch and if it is moving in the good
direction is good or not.
-- 
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] standalone backend PANICs during recovery

2016-08-30 Thread Michael Paquier
On Wed, Aug 24, 2016 at 5:07 PM, Bernd Helmle  wrote:
> That said, i'm okay if --single is not intended to bring up a hot standby.
> There are many other ways to debug such problems.

This patch is still on the CF app:
https://commitfest.postgresql.org/10/610/
Even after thinking about it, my opinion is still the same. Let's
prevent a standalone backend to start if it sees recovery.conf.
Attached is a patch and the CF entry is switched as ready for
committer to move on.
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index acd95aa..3d88727 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5930,6 +5930,18 @@ StartupXLOG(void)
 	struct stat st;
 
 	/*
+	 * Prevent standalone process to start if recovery is wanted. A lot of
+	 * code paths in recovery depend on the assumption that it is not the
+	 * case.
+	 */
+	if (!IsPostmasterEnvironment)
+	{
+		if (stat(RECOVERY_COMMAND_FILE, ) == 0)
+			ereport(FATAL,
+	(errmsg("recovery.conf is not allowed with a standalone process")));
+	}
+
+	/*
 	 * Read control file and check XLOG status looks valid.
 	 *
 	 * Note: in most control paths, *ControlFile is already valid and we need

-- 
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] OpenSSL 1.1 breaks configure and more

2016-08-30 Thread Heikki Linnakangas

On 08/30/2016 03:26 AM, Andreas Karlsson wrote:

On 08/26/2016 11:31 AM, Heikki Linnakangas wrote:

On 07/05/2016 04:46 PM, Andreas Karlsson wrote:

@@ -280,8 +287,9 @@ px_find_digest(const char *name, PX_MD **res)
 digest = px_alloc(sizeof(*digest));
 digest->algo = md;

-EVP_MD_CTX_init(>ctx);
-if (EVP_DigestInit_ex(>ctx, digest->algo, NULL) == 0)
+digest->ctx = EVP_MD_CTX_create();
+EVP_MD_CTX_init(digest->ctx);
+if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0)
 return -1;

 h = px_alloc(sizeof(*h));


Now that we're calling EVP_MD_CTX_create((), which allocates memory, are
we risking memory leaks? It has always been part of the contract that
you have to call px_md_free(), for any context returned by
px_find_digest(), but I wonder just how careful we have been about that.
Before this, you would probably get away with it without leaking, if the
digest implementation didn't allocate any extra memory or other resources.

At least pg_digest and try_unix_std functions call px_find_digest(), and
then do more palloc()s which could elog() if you run out of memory,
leaking th digest struct. Highly unlikely, but I think it would be
fairly straightforward to reorder those calls to eliminate the risk, so
we probably should.


Since px_find_digest() calls palloc() later in the function there is a
slim possibility of memory leaks.


Yep. That palloc() would be easy to move to before the EVP_MD_CTX_new() 
call. And some of the calls to px_find_digest() could likewise be 
rearranged. But there are some more complicated callers. pgp_encrypt(), 
for example, builds a pipeline of multiple "mbuf" filters, and one of 
those filters uses px_find_digest().



How do we generally handle that things
not allocated with palloc() may leak when something calls elog()?


There's the ResourceOwner mechanism, see src/backend/utils/resowner/. 
That would be the proper way to do this. Call 
RegisterResourceReleaseCallback() when the context is allocated, and 
have the callback free it. One pitfall to watch out for is that 
RegisterResourceReleaseCallback() itself calls palloc(), and can error 
out, so you have to do things in such an order that you don't leak in 
that case either.


Want to take a stab at that?

Another approach is put each allocated context in a list or array in a 
global variable, and to register a callback to be called at 
end-of-(sub)transaction, which closes all the contexts. But the resource 
owner mechanism is probably easier.


There's also PG_TRY-CATCH, that you could maybe use in the callers of 
px_find_digest(), to make sure they call px_free_digest() even on error. 
But that also seems difficult to use with the pgp_encrypt() pipeline.



I have attached new versions of the patches which are rebased on master,
with slightly improves error handling in px_find_digest(), and handles
the deprecation of ASN1_STRING_data().


Thanks!

PS. I just remembered that I've wanted to refactor the pgcrypto calls 
for symmetric encryption to use the newer EVP API for some time, and 
even posted a patch for that 
(https://www.postgresql.org/message-id/561274f1.1030...@iki.fi). I 
dropped the ball back then, but I think I'll go ahead and do that now, 
once we get these other OpenSSL changes in.


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

2016-08-30 Thread Michael Paquier
On Tue, Aug 30, 2016 at 2:57 PM, Michael Paquier
 wrote:
> The funny part here is that ProcGlobal->allProcs is actually handled,
> but not the two others. Well yes, you are right, we really need to
> fail on FATAL for all of them if ShmemAlloc returns NULL as they
> involve the shmem initialization at postmaster startup.

And with an actual patch things are better.
-- 
Michael
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index e5eeec2..5dd0046 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -306,6 +306,11 @@ sql_conn(struct options * my_opts)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "%s: out of memory\n", "oid2name");
+exit(1);
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 5eac2b1..e4136f9 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -632,7 +632,7 @@ main(int argc, char **argv)
 }
 break;
 			case 't':			/* Trigger file */
-triggerPath = strdup(optarg);
+triggerPath = pg_strdup(optarg);
 break;
 			case 'w':			/* Max wait time */
 maxwaittime = atoi(optarg);
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 769c805..2b0faf0 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -71,7 +71,14 @@ vacuumlo(const char *database, const struct _param * param)
 
 	/* Note: password can be carried over from a previous call */
 	if (param->pg_prompt == TRI_YES && password == NULL)
+	{
 		password = simple_prompt("Password: ", 100, false);
+		if (!password)
+		{
+			fprintf(stderr, "out of memory\n");
+			return -1;
+		}
+	}
 
 	/*
 	 * Start the connection.  Loop until we have a password if requested by
@@ -115,6 +122,11 @@ vacuumlo(const char *database, const struct _param * param)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "out of memory\n");
+return -1;
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
@@ -514,7 +526,7 @@ main(int argc, char **argv)
 }
 break;
 			case 'U':
-param.pg_user = strdup(optarg);
+param.pg_user = pg_strdup(optarg);
 break;
 			case 'w':
 param.pg_prompt = TRI_NO;
@@ -529,10 +541,10 @@ main(int argc, char **argv)
 	fprintf(stderr, "%s: invalid port number: %s\n", progname, optarg);
 	exit(1);
 }
-param.pg_port = strdup(optarg);
+param.pg_port = pg_strdup(optarg);
 break;
 			case 'h':
-param.pg_host = strdup(optarg);
+param.pg_host = pg_strdup(optarg);
 break;
 		}
 	}
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index bbae584..d1a26ef 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -212,6 +212,10 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
 
 		/* Initialize LWLocks */
 		shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
+		if (!shared->buffer_locks)
+			ereport(FATAL,
+	(errcode(ERRCODE_OUT_OF_MEMORY),
+	 errmsg("out of shared memory")));
 
 		Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
 		strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 8feeae0..5fd6290 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -569,8 +569,15 @@ boot_openrel(char *relname)
 			++i;
 		heap_endscan(scan);
 		app = Typ = ALLOC(struct typmap *, i + 1);
+		if (app == NULL)
+			elog(ERROR, "out of memory");
 		while (i-- > 0)
-			*app++ = ALLOC(struct typmap, 1);
+		{
+			*app = ALLOC(struct typmap, 1);
+			if (*app == NULL)
+elog(ERROR, "out of memory");
+			app++;
+		}
 		*app = NULL;
 		scan = heap_beginscan_catalog(rel, 0, NULL);
 		app = Typ;
@@ -894,8 +901,15 @@ gettype(char *type)
 			++i;
 		heap_endscan(scan);
 		app = Typ = ALLOC(struct typmap *, i + 1);
+		if (app == NULL)
+			elog(ERROR, "out of memory");
 		while (i-- > 0)
-			*app++ = ALLOC(struct typmap, 1);
+		{
+			*app = ALLOC(struct typmap, 1);
+			if (*app == NULL)
+elog(ERROR, "out of memory");
+			app++;
+		}
 		*app = NULL;
 		scan = heap_beginscan_catalog(rel, 0, NULL);
 		app = Typ;
diff --git a/src/backend/port/dynloader/darwin.c b/src/backend/port/dynloader/darwin.c
index ccd92c3..a83c614 100644
--- a/src/backend/port/dynloader/darwin.c
+++ b/src/backend/port/dynloader/darwin.c
@@ -78,6 +78,9 @@ pg_dlsym(void *handle, char *funcname)
 	NSSymbol symbol;
 	char	   *symname = (char *) malloc(strlen(funcname) + 2);
 
+	if (!symname)
+		return NULL;
+
 	sprintf(symname, "_%s", funcname);
 	if (NSIsSymbolNameDefined(symname))
 	{
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-30 Thread Michael Paquier
On Mon, Aug 29, 2016 at 11:26 PM, Tom Lane  wrote:
> Aleksander Alekseev  writes:
>>> if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>>> + {
>>> +free(prodesc);
>
>> I think that prodesc->user_proname and prodesc->internal_proname should
>> also be freed if they are not NULL's.
>
> Hmm, this is kind of putting lipstick on a pig, isn't it?  That code
> is still prone to leakage further down, because it calls stuff like
> SearchSysCache which is entirely capable of throwing elog(ERROR).
>
> If we're going to touch compile_pltcl_function at all, I'd vote for
>
> (1) changing these malloc calls to MemoryContextAlloc(TopMemoryContext,...
> (2) putting the cleanup into a PG_CATCH block, and removing all the
> retail free() calls that are there now.

We've done similar handling lately with for example 8c75ad43 for
plpython, and this has finished by using TopMemoryContext, so that's
the way to do. By the way plperl needs the same cleanup, and by
looking at the archives guess who did exactly that with a set of
patches not long ago:
https://www.postgresql.org/message-id/cab7npqrxvq+q66ufzd9wa5uaftyn4wauadbjxkfrync96kf...@mail.gmail.com
But I did not get much feedback about those patches :)

So for now I have removed this part from the patch of this thread.
-- 
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?

2016-08-30 Thread Fujii Masao
On Tue, Aug 30, 2016 at 3:39 AM, Tom Lane  wrote:
> Fujii Masao  writes:
>> ISTM that the cause of this issue is that gin_desc() uses XLogRecGetData() to
>> extract ginxlogVacuumDataLeafPage data from XLOG_GIN_VACUUM_DATA_LEAF_PAGE
>> record. Since it's registered by XLogRegisterBufData() in
>> ginVacuumPostingTreeLeaf(),
>> XLogRecGetBlockData() should be used, instead. Patch attached. Thought?
>
> I think we probably have more issues than that.  See for example
> https://www.postgresql.org/message-id/flat/20160826072658.15676.7628%40wrigleys.postgresql.org
>
> which clearly shows that the replay logic is seeing something wrong too:
>
> 2016-08-26 06:01:50 UTC FATAL:  unexpected GIN leaf action: 0
> 2016-08-26 06:01:50 UTC CONTEXT:  xlog redo Insert item, node:
> 1663/16387/33108 blkno: 6622 isdata: T isleaf: T 3 segments: 2 (add 0 items)
> 0 unknown action 0 ???
>
> If it were just a matter of gin_desc() being wrong, we'd not have
> gotten such a failure.  (Which is not to say that gin_desc() isn't
> wrong; it may well be.)

Yeah, I got the pg_xlogdump issue while I was trying to reproduce
the above reported problem. Fixing pg_xlogdump would be helpful for
the analysis of that problem.

Regards,

-- 
Fujii Masao


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