[HACKERS] Minor typo in doc: replication-origins.sgml

[Amit Langote]

Hi,

Attached does:

s/pg_replication_origin_xact-setup/pg_replication_origin_xact_setup/g

or, (s/-/_/g)

Thanks,
Amit
diff --git a/doc/src/sgml/replication-origins.sgml b/doc/src/sgml/replication-origins.sgml
index c531022..0cd08ee 100644
--- a/doc/src/sgml/replication-origins.sgml
+++ b/doc/src/sgml/replication-origins.sgml
@@ -60,7 +60,7 @@
   function. Additionally the LSN and commit
   timestamp of every source transaction can be configured on a per
   transaction basis using
-  pg_replication_origin_xact-setup().
+  pg_replication_origin_xact_setup().
   If that's done replication progress will be persist in a crash safe
   manner. Replay progress for all replication origins can be seen in the
   

-- 
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] CTE optimization fence on the todo list?

[Chris Rogers]

Has there been any movement on this in the last couple years?

I could really use the ability to optimize across CTE boundaries, and it
seems like a lot of other people could too.

Re: [HACKERS] Final Patch for GROUPING SETS

[Andrew Gierth]

> "Andres" == Andres Freund  writes:

 Andres> This is not a real review.  I'm just scanning through the
 Andres> patch, without reading the thread, to understand if I see
 Andres> something "worthy" of controversy. While scanning I might have
 Andres> a couple observations or questions.

 >> + *   A list of grouping sets which is structurally equivalent to a ROLLUP
 >> + *   clause (e.g. (a,b,c), (a,b), (a)) can be processed in a single pass 
 >> over
 >> + *   ordered data.  We do this by keeping a separate set of transition 
 >> values
 >> + *   for each grouping set being concurrently processed; for each input 
 >> tuple
 >> + *   we update them all, and on group boundaries we reset some initial 
 >> subset
 >> + *   of the states (the list of grouping sets is ordered from most 
 >> specific to
 >> + *   least specific).  One AGG_SORTED node thus handles any number of 
 >> grouping
 >> + *   sets as long as they share a sort order.

 Andres> Found "initial subset" not very clear, even if I probably
 Andres> guessed the right meaning.

How about:

 *  [...], and on group boundaries we reset those states
 *  (starting at the front of the list) whose grouping values have
 *  changed (the list of grouping sets is ordered from most specific to
 *  least specific).  One AGG_SORTED node thus handles any number [...]

 >> + *   To handle multiple grouping sets that _don't_ share a sort order, we 
 >> use
 >> + *   a different strategy.  An AGG_CHAINED node receives rows in sorted 
 >> order
 >> + *   and returns them unchanged, but computes transition values for its own
 >> + *   list of grouping sets.  At group boundaries, rather than returning the
 >> + *   aggregated row (which is incompatible with the input rows), it writes 
 >> it
 >> + *   to a side-channel in the form of a tuplestore.  Thus, a number of
 >> + *   AGG_CHAINED nodes are associated with a single AGG_SORTED node (the
 >> + *   "chain head"), which creates the side channel and, when it has 
 >> returned
 >> + *   all of its own data, returns the tuples from the tuplestore to its own
 >> + *   caller.

 Andres> This paragraph deserves to be expanded imo.

OK, but what in particular needs clarifying?

 >> + *   In order to avoid excess memory consumption from a chain of 
 >> alternating
 >> + *   Sort and AGG_CHAINED nodes, we reset each child Sort node 
 >> preemptively,
 >> + *   allowing us to cap the memory usage for all the sorts in the chain at
 >> + *   twice the usage for a single node.

 Andres> What does reseting 'preemtively' mean?

Plan nodes are normally not reset (in the sense of calling ExecReScan)
just because they finished, but rather it's done before a subsequent new
scan is done.  Doing the rescan call after all the sorted output has
been read means we discard the data from each sort node as soon as it is
transferred to the next one.

There is a more specific comment in agg_retrieve_chained where this
actually happens.

 >> + *   From the perspective of aggregate transition and final functions, the
 >> + *   only issue regarding grouping sets is this: a single call site 
 >> (flinfo)
 >> + *   of an aggregate function may be used for updating several different
 >> + *   transition values in turn. So the function must not cache in the 
 >> flinfo
 >> + *   anything which logically belongs as part of the transition value (most
 >> + *   importantly, the memory context in which the transition value exists).
 >> + *   The support API functions (AggCheckCallContext, AggRegisterCallback) 
 >> are
 >> + *   sensitive to the grouping set for which the aggregate function is
 >> + *   currently being called.

 Andres> Hm. I've seen a bunch of aggreates do this.

Such as? This was discussed about a year ago in the context of WITHIN
GROUP:

http://www.postgresql.org/message-id/87r424i24w@news-spur.riddles.org.uk

 >> + *   TODO: AGG_HASHED doesn't support multiple grouping sets yet.

 Andres> Are you intending to resolve this before an eventual commit?

Original plan was to tackle AGG_HASHED after a working implementation
was committed; we figured that we'd have two commitfests to get the
basics right, and then have a chance to get AGG_HASHED done for the
third one. Also, there was talk of other people working on hashagg
memory usage issues, and we didn't want to conflict with that.

Naturally the extended delays rather put paid to that plan. Going ahead
and writing code for AGG_HASHED anyway wasn't really an option, since
with the overall structural questions unresolved there was too much
chance of it being wasted effort.

 Andres> Possibly after the 'structural' issues are resolved? Or do you
 Andres> think this can safely be put of for another release?

I think the feature is useful even without AGG_HASHED, even though that
means it can sometimes be beaten on performance by using UNION ALL of
many separate GROUP BYs; but I'd defer to the opinions of others on that
point.

 Andres> Maybe it's just me, but I get twitchy if I se

Re: [HACKERS] Reducing tuple overhead

[Amit Kapila]

On Tue, Apr 28, 2015 at 2:31 AM, Jim Nasby  wrote:
>
> On 4/25/15 12:12 AM, Amit Kapila wrote:
>>
>>  > ... which isn't possible. You can not go from a heap tuple to an
>> index tuple.
>>
>> We will have the access to index value during delete, so why do you
>> think that we need linkage between heap and index tuple to perform
>> Delete operation?  I think we need to think more to design Delete
>> .. by CTID, but that should be doable.
>
>
> The problem with just having the value is that if *anything* changes
between how you evaluated the value when you created the index tuple and
when you evaluate it a second time you'll corrupt your index.
>

I think I am missing something here, but when this second
evaluation is needed.  Basically what I understand from index
insertion is that it evaluates the value to be inserted in index
before calling nbtree module and then nbtree just inserts the
value/tuple passed to it.


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

Re: [HACKERS] Auditing extension for PostgreSQL (Take 2)

[Sawada Masahiko]

On Wed, Apr 29, 2015 at 12:17 AM, David Steele  wrote:
> On 4/28/15 2:14 AM, Sawada Masahiko wrote:
>> On Fri, Apr 24, 2015 at 3:23 AM, David Steele  wrote:
>>> I've also added some checking to make sure that if anything looks funny
>>> on the stack an error will be generated.
>>>
>>> Thanks for the feedback!
>>>
>>
>> Thank you for updating the patch!
>> I ran the postgres regression test on database which is enabled
>> pg_audit, it works fine.
>> Looks good to me.
>>
>> If someone don't have review comment or bug report, I will mark this
>> as "Ready for Committer".
>
> Thank you!  I appreciate all your work reviewing this patch and of
> course everyone else who commented on, reviewed or tested the patch
> along the way.
>

I have changed the status this to "Ready for Committer".

Regards,

---
Sawada Masahiko


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

Re: [HACKERS] Proposal: knowing detail of config files via SQL

[Sawada Masahiko]

On Thu, Apr 30, 2015 at 6:31 AM, David Steele  wrote:
> On 4/29/15 5:16 PM, Robert Haas wrote:
>> On Fri, Apr 24, 2015 at 2:40 PM, David Steele  wrote:
>>>   
>>>The view pg_file_settings provides access to
>>> run-time parameters
>>>that are defined in configuration files via SQL.  In contrast to
>>>pg_settings a row is provided for each
>>> occurrence
>>>of the parameter in a configuration file.  This is helpful for
>>> discovering why one value
>>>may have been used in preference to another when the parameters were
>>> loaded.
>>>   
>>
>> This seems to imply that this gives information about only a subset of
>> configuration files; specifically, those auto-generated based on SQL
>> commands - i.e. postgresql.conf.auto.  But I think it's supposed to
>> give information about all configuration files, regardless of how
>> generated.  Am I wrong?  If not, I'd suggest "run-time parameters that
>> are defined in configuration files via SQL" -> "run-time parameters
>> stored in configuration files".
>
> The view does give information about all configuration files regardless
> of how they were created.
>
> That's what I intended the text to say but I think your phrasing is clearer.
>

Thank you for reviewing.
I agree with this.
Attached patch is updated version v10.

Regards,

---
Sawada Masahiko
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 4b79958..adb8628 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7560,6 +7560,11 @@
  
 
  
+  pg_file_settings
+  parameter settings of file
+ 
+
+ 
   pg_shadow
   database users
  
@@ -9173,6 +9178,74 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
 
  
 
+ 
+  pg_file_settings
+
+  
+   pg_file_settings
+  
+
+  
+   The view pg_file_settings provides access to
+   run-time parameters stored in configuration files.
+   In contrast to pg_settings a row is provided for
+   each occurrence of the parameter in a configuration file. This is helpful
+   for discovering why one value may have been used in preference to another
+   when the parameters were loaded.
+  
+
+  
+   pg_file_settings Columns
+
+  
+   
+
+ Name
+ Type
+ Description
+
+   
+   
+
+ sourcefile
+ text
+ A path of configration file
+
+
+ sourceline
+ integer
+ 
+  Line number within the configuration file the current value was
+  set at (null for values set from sources other than configuration files,
+  or when examined by a non-superuser)
+ 
+
+
+ seqno
+ integer
+ Order in which the setting was loaded from the configuration
+
+
+ name
+ text
+ 
+  Configuration file the current value was set in (null for values
+  set from sources other than configuration files, or when examined by a
+  non-superuser); helpful when using include directives in
+  configuration files
+ 
+
+
+ setting
+ text
+ Run-time configuration parameter name
+
+   
+  
+ 
+
+
+
  
   pg_shadow
 
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2ad01f4..18921c4 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -411,6 +411,12 @@ CREATE RULE pg_settings_n AS
 
 GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;
 
+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM PUBLIC;
+REVOKE EXECUTE ON FUNCTION pg_show_all_file_settings() FROM PUBLIC;
+
 CREATE VIEW pg_timezone_abbrevs AS
 SELECT * FROM pg_timezone_abbrevs();
 
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index c5e0fac..873d950 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -119,6 +119,8 @@ ProcessConfigFile(GucContext context)
 	ConfigVariable *item,
 			   *head,
 			   *tail;
+	ConfigFileVariable *guc_array;
+	size_t			guc_array_size;
 	int			i;
 
 	/*
@@ -217,6 +219,9 @@ ProcessConfigFile(GucContext context)
 		gconf->status &= ~GUC_IS_IN_FILE;
 	}
 
+	/* Reset number of guc_file_variables */
+	num_guc_file_variables = 0;
+
 	/*
 	 * Check if all the supplied option names are valid, as an additional
 	 * quasi-syntactic check on the validity of the config file.  It is
@@ -255,6 +260,7 @@ ProcessConfigFile(GucContext context)
 			error = true;
 			ConfFileWithError = item->filename;
 		}
+		num_guc_file_variables++;
 	}
 
 	/*
@@ -342,6 +348,47 @@ ProcessConfigFile(GucContext context)
 	}
 
 	/*
+	 * Calculate size of guc_array and allocate it. From the secound time to allcate memory,
+	 * we should free old allocated memory.
+	 */
+	guc_array_size = num_guc_file_variables * sizeof(struct ConfigFileVariable);
+	if (!guc_file_variables)
+	{
+		/* For the first call */
+		guc_file_variables = (ConfigFileVariable *) guc_malloc(FATAL, guc_array_size);
+	}
+	

Re: [HACKERS] Make more portable TAP tests of initdb

[Noah Misch]

On Wed, Apr 15, 2015 at 02:59:55PM +0900, Michael Paquier wrote:
> On Wed, Apr 15, 2015 at 2:38 PM, Noah Misch wrote:
> > Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8.  Therefore, 
> > Perl
> > installations lacking this File::Path feature will receive vendor support 
> > for
> > years to come.  Replacing the use of keep_root with rmtree+mkdir will add 
> > 2-10
> > lines of code, right?  Let's do that and not raise the system requirements.
> 
> Good point. Let's use mkdir in combination then. Attached is an updated patch.

> --- a/src/bin/initdb/t/001_initdb.pl
> +++ b/src/bin/initdb/t/001_initdb.pl
> @@ -1,6 +1,7 @@
>  use strict;
>  use warnings;
>  use TestLib;
> +use File::Path qw(rmtree);
>  use Test::More tests => 19;
>  
>  my $tempdir = TestLib::tempdir;
> @@ -18,27 +19,30 @@ command_fails([ 'initdb', '-S', "$tempdir/data3" ],
>  mkdir "$tempdir/data4" or BAIL_OUT($!);
>  command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory');
>  
> -system_or_bail "rm -rf '$tempdir'/*";
> -
> +rmtree($tempdir);
> +mkdir $tempdir;

It's usually wrong to remove and recreate the very directory made by
File::Temp.  Doing so permits a race condition: an attacker can replace the
directory between the rmdir() and the mkdir().  However, TestLib::tempdir
returns a subdirectory of the build directory, and the build directory is
presumed secure.  That's good enough.

> -system_or_bail "rm -rf '$tempdir'/*";
> +rmtree($tempdir);
>  command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ],
>   'select default dictionary');

You omitted the mkdir() on that last one.  It works, since initdb does the
equivalent of "mkdir -p", but it looks like an oversight.


As I pondered this, I felt it would do better to solve a different problem.
The "rm -rf" invocations presumably crept in to reduce peak disk usage.
Considering the relatively-high duration of a successful initdb run, I doubt
we get good value from so many positive tests.  I squashed those positive
tests into one, as attached.  (I changed the order of negative tests but kept
them all.)  This removed the need for intra-script cleaning, and it
accelerated script runtime from 22s to 9s.  Does this seem good to you?

Thanks,
nm
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index d12be84..ed13bdc 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -1,44 +1,32 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 19;
+use Test::More tests => 14;
 
 my $tempdir = TestLib::tempdir;
+my $xlogdir = "$tempdir/pgxlog";
+my $datadir = "$tempdir/data";
 
 program_help_ok('initdb');
 program_version_ok('initdb');
 program_options_handling_ok('initdb');
 
-command_ok([ 'initdb', "$tempdir/data" ], 'basic initdb');
-command_fails([ 'initdb', "$tempdir/data" ], 'existing data directory');
-command_ok([ 'initdb', '-N', "$tempdir/data2" ], 'nosync');
-command_ok([ 'initdb', '-S', "$tempdir/data2" ], 'sync only');
-command_fails([ 'initdb', '-S', "$tempdir/data3" ],
+command_fails([ 'initdb', '-S', "$tempdir/nonexistent" ],
'sync missing data directory');
-mkdir "$tempdir/data4" or BAIL_OUT($!);
-command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory');
 
-system_or_bail "rm -rf '$tempdir'/*";
-
-command_ok([ 'initdb', '-X', "$tempdir/pgxlog", "$tempdir/data" ],
-   'separate xlog directory');
-
-system_or_bail "rm -rf '$tempdir'/*";
+mkdir $xlogdir;
+mkdir "$xlogdir/lost+found";
+command_fails(
+   [ 'initdb', '-X', $xlogdir, $datadir ],
+   'existing nonempty xlog directory');
+rmdir "$xlogdir/lost+found";
 command_fails(
-   [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ],
+   [ 'initdb', $datadir, '-X', 'pgxlog' ],
'relative xlog directory not allowed');
 
-system_or_bail "rm -rf '$tempdir'/*";
-mkdir "$tempdir/pgxlog";
-command_ok([ 'initdb', '-X', "$tempdir/pgxlog", "$tempdir/data" ],
-   'existing empty xlog directory');
-
-system_or_bail "rm -rf '$tempdir'/*";
-mkdir "$tempdir/pgxlog";
-mkdir "$tempdir/pgxlog/lost+found";
-command_fails([ 'initdb', '-X', "$tempdir/pgxlog", "$tempdir/data" ],
-   'existing nonempty xlog directory');
+mkdir $datadir;
+command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
+   'successful creation');
 
-system_or_bail "rm -rf '$tempdir'/*";
-command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ],
-   'select default dictionary');
+command_ok([ 'initdb', '-S', $datadir ], 'sync only');
+command_fails([ 'initdb', $datadir ], 'existing data directory');

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

Re: [HACKERS] pg_upgrade: quote directory names in delete_old_cluster script

[Bruce Momjian]

On Mon, Feb 16, 2015 at 05:03:45PM -0500, Bruce Momjian wrote:
> > All of our makefiles use single quotes, so effectively the only
> > character we don't support in directory paths is the single quote itself.
> 
> This seems to say that Windows batch files don't have any special
> meaning for single quotes:
> 
>   
> http://stackoverflow.com/questions/24173825/what-does-single-quote-do-in-windows-batch-files
>   
> http://stackoverflow.com/questions/10737283/single-quotes-and-double-quotes-how-to-have-the-same-behaviour-in-unix-and-wind
> 
> Again, is it worth doing something platform-specific?  I can certainly
> define a platform-specific macro for " or ' as and use that.  Good idea?

I have developed the attached patch to use platform-specific quoting of
path names.

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

  + Everyone has their own god. +
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 6db223a..be66b24
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*** create_script_for_old_cluster_deletion(c
*** 532,538 
  #endif
  
  	/* delete old cluster's default tablespace */
! 	fprintf(script, RMDIR_CMD " \"%s\"\n", fix_path_separator(old_cluster.pgdata));
  
  	/* delete old cluster's alternate tablespaces */
  	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
--- 532,539 
  #endif
  
  	/* delete old cluster's default tablespace */
! 	fprintf(script, RMDIR_CMD " %c%s%c\n", PATH_QUOTE,
! 			fix_path_separator(old_cluster.pgdata), PATH_QUOTE);
  
  	/* delete old cluster's alternate tablespaces */
  	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
*** create_script_for_old_cluster_deletion(c
*** 554,562 
  		PATH_SEPARATOR);
  
  			for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
! fprintf(script, RMDIR_CMD " \"%s%c%d\"\n",
  		fix_path_separator(os_info.old_tablespaces[tblnum]),
! 		PATH_SEPARATOR, old_cluster.dbarr.dbs[dbnum].db_oid);
  		}
  		else
  		{
--- 555,564 
  		PATH_SEPARATOR);
  
  			for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
! fprintf(script, RMDIR_CMD " %c%s%c%d%c\n", PATH_QUOTE,
  		fix_path_separator(os_info.old_tablespaces[tblnum]),
! 		PATH_SEPARATOR, old_cluster.dbarr.dbs[dbnum].db_oid,
! 		PATH_QUOTE);
  		}
  		else
  		{
*** create_script_for_old_cluster_deletion(c
*** 566,574 
  			 * Simply delete the tablespace directory, which might be ".old"
  			 * or a version-specific subdirectory.
  			 */
! 			fprintf(script, RMDIR_CMD " \"%s%s\"\n",
  	fix_path_separator(os_info.old_tablespaces[tblnum]),
! 	fix_path_separator(suffix_path));
  			pfree(suffix_path);
  		}
  	}
--- 568,576 
  			 * Simply delete the tablespace directory, which might be ".old"
  			 * or a version-specific subdirectory.
  			 */
! 			fprintf(script, RMDIR_CMD " %c%s%s%c\n", PATH_QUOTE,
  	fix_path_separator(os_info.old_tablespaces[tblnum]),
! 	fix_path_separator(suffix_path), PATH_QUOTE);
  			pfree(suffix_path);
  		}
  	}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
new file mode 100644
index 4683c6f..bb035e1
*** a/src/bin/pg_upgrade/pg_upgrade.h
--- b/src/bin/pg_upgrade/pg_upgrade.h
*** extern char *output_files[];
*** 74,79 
--- 74,80 
  #define pg_mv_file			rename
  #define pg_link_file		link
  #define PATH_SEPARATOR		'/'
+ #define PATH_QUOTE	'\''
  #define RM_CMD"rm -f"
  #define RMDIR_CMD			"rm -rf"
  #define SCRIPT_PREFIX		"./"
*** extern char *output_files[];
*** 85,90 
--- 86,92 
  #define pg_mv_file			pgrename
  #define pg_link_file		win32_pghardlink
  #define PATH_SEPARATOR		'\\'
+ #define PATH_QUOTE	'"'
  #define RM_CMD"DEL /q"
  #define RMDIR_CMD			"RMDIR /s/q"
  #define SCRIPT_PREFIX		""

-- 
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] alternative compression algorithms?

[Tomas Vondra]

On 04/30/15 02:42, Robert Haas wrote:

On Wed, Apr 29, 2015 at 6:55 PM, Tomas Vondra
 wrote:

I'm not convinced not compressing the data is a good idea - it suspect it
would only move the time to TOAST, increase memory pressure (in general and
in shared buffers). But I think that using a more efficient compression
algorithm would help a lot.

For example, when profiling the multivariate stats patch (with multiple
quite large histograms), the pglz_decompress is #1 in the profile, occupying
more than 30% of the time. After replacing it with the lz4, the data are bit
larger, but it drops to ~0.25% in the profile and planning the drops
proportionally.


That seems to imply a >100x improvement in decompression speed.  Really???


Sorry, that was a bit misleading over-statement. The profiles (same 
dataset, same workload) look like this:



pglz_decompress
---
  44.51%  postgres  [.] pglz_decompress
  13.60%  postgres  [.] update_match_bitmap_histogram
   8.40%  postgres  [.] float8_cmp_internal
   7.43%  postgres  [.] float8lt
   6.49%  postgres  [.] deserialize_mv_histogram
   6.23%  postgres  [.] FunctionCall2Coll
   4.06%  postgres  [.] DatumGetFloat8
   3.48%  libc-2.18.so  [.] __isnan
   1.26%  postgres  [.] clauselist_mv_selectivity
   1.09%  libc-2.18.so  [.] __memcpy_sse2_unaligned

lz4
---
  18.05%  postgres  [.] update_match_bitmap_histogram
  11.67%  postgres  [.] float8_cmp_internal
  10.53%  postgres  [.] float8lt
   8.67%  postgres  [.] FunctionCall2Coll
   8.52%  postgres  [.] deserialize_mv_histogram
   5.52%  postgres  [.] DatumGetFloat8
   4.90%  libc-2.18.so  [.] __isnan
   3.92%  liblz4.so.1.6.0   [.] 0x2603
   2.08%  liblz4.so.1.6.0   [.] 0x2847
   1.81%  postgres  [.] clauselist_mv_selectivity
   1.47%  libc-2.18.so  [.] __memcpy_sse2_unaligned
   1.33%  liblz4.so.1.6.0   [.] 0x260f
   1.16%  liblz4.so.1.6.0   [.] 0x25e3
   (and then a long tail of other lz4 calls)

The difference used to more significant, but I've done a lot of 
improvements in the update_match_bitmap method (so the lz4 methods are 
more significant).


The whole script (doing a lot of estimates) takes 1:50 with pglz and 
only 1:25 with lz4. That's ~25-30% improvement.


The results are slightly unreliable because collected in a Xen VM, and 
the overhead is non-negligible (but the same in both cases). I wouldn't 
be surprised if the difference was more significant without the VM.


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


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

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Wed, Apr 29, 2015 at 8:20 PM, Alvaro Herrera
 wrote:
>> Finally, you've got the idea of making pg_ a reserved prefix for
>> roles, adding some predefined roles, and giving them some predefined
>> privileges.  That should be yet another patch.
>
> On this part I have a bit of a problem -- the prefix is not really
> reserved, is it.  I mean, evidently it's still possible to create roles
> with the pg_ prefix ... otherwise, how come the new lines to
> system_views.sql that create the "predefined" roles work in the first
> place?  I think if we're going to reserve role names, we should reserve
> them for real: CREATE ROLE should flat out reject creation of such
> roles, and the default ones should be created during bootstrap.
>
> IMO anyway.

This is exactly what I mean about needing separate discussion for
separate parts of the patch.  There's so much different stuff in there
right now that objections like this won't necessarily come out until
it's far too late to change things around.

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


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

Re: [HACKERS] alternative compression algorithms?

[Robert Haas]

On Wed, Apr 29, 2015 at 6:55 PM, Tomas Vondra
 wrote:
> I'm not convinced not compressing the data is a good idea - it suspect it
> would only move the time to TOAST, increase memory pressure (in general and
> in shared buffers). But I think that using a more efficient compression
> algorithm would help a lot.
>
> For example, when profiling the multivariate stats patch (with multiple
> quite large histograms), the pglz_decompress is #1 in the profile, occupying
> more than 30% of the time. After replacing it with the lz4, the data are bit
> larger, but it drops to ~0.25% in the profile and planning the drops
> proportionally.

That seems to imply a >100x improvement in decompression speed.  Really???

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


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

Re: [HACKERS] Additional role attributes && superuser review

[Gavin Flower]

On 30/04/15 12:20, Alvaro Herrera wrote:

Robert Haas wrote:


I think that if you commit this the way you have it today, everybody
will go, oh, look, Stephen committed something, but it looks
complicated, I won't pay attention.

Yeah, that sucks.


Finally, you've got the idea of making pg_ a reserved prefix for
roles, adding some predefined roles, and giving them some predefined
privileges.  That should be yet another patch.

On this part I have a bit of a problem -- the prefix is not really
reserved, is it.  I mean, evidently it's still possible to create roles
with the pg_ prefix ... otherwise, how come the new lines to
system_views.sql that create the "predefined" roles work in the first
place?  I think if we're going to reserve role names, we should reserve
them for real: CREATE ROLE should flat out reject creation of such
roles, and the default ones should be created during bootstrap.

IMO anyway.

What if I had a company with several subsidiaries using the same 
database, and want to prefix roles and other things with the 
subsidiary's initials? (I am not saying this would be a good 
architecture!!!)


For example if one subsidiary was called 'Perfect Gentleman', so I would 
want roles prefixed by 'pg_' and would be annoyed if I couldn't!



Cheers,
Gavin


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

Re: [HACKERS] Additional role attributes && superuser review

[Alvaro Herrera]

Robert Haas wrote:

> I think that if you commit this the way you have it today, everybody
> will go, oh, look, Stephen committed something, but it looks
> complicated, I won't pay attention.

Yeah, that sucks.

> Finally, you've got the idea of making pg_ a reserved prefix for
> roles, adding some predefined roles, and giving them some predefined
> privileges.  That should be yet another patch.

On this part I have a bit of a problem -- the prefix is not really
reserved, is it.  I mean, evidently it's still possible to create roles
with the pg_ prefix ... otherwise, how come the new lines to
system_views.sql that create the "predefined" roles work in the first
place?  I think if we're going to reserve role names, we should reserve
them for real: CREATE ROLE should flat out reject creation of such
roles, and the default ones should be created during bootstrap.

IMO anyway.

-- 
Á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] Incompatible trig error handling

[Tom Lane]

John Gorman  writes:
> Two of the trigonometry functions have differing error condition behavior
> between Linux and OSX. The Linux behavior follows the standard set by the
> other trig functions.

We have never considered it part of Postgres' charter to try to hide
platform-specific variations in floating-point behavior.  If we did,
we'd spend all our time doing that rather than more productive stuff.

In particular, it appears to me that both of these behaviors are allowed
per the POSIX standard, which makes it very questionable why we should
insist that one is correct and the other is not.

In addition, the proposed patch turns *all* cases that return NaN into
errors, which is wrong at least for the case where the input is NaN.

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] Final Patch for GROUPING SETS

[Andres Freund]

Hi,

This is not a real review.  I'm just scanning through the patch, without
reading the thread, to understand if I see something "worthy" of
controversy. While scanning I might have a couple observations or
questions.


On 2015-03-13 15:46:15 +, Andrew Gierth wrote:

> + * A list of grouping sets which is structurally equivalent to a ROLLUP
> + * clause (e.g. (a,b,c), (a,b), (a)) can be processed in a single pass 
> over
> + * ordered data.  We do this by keeping a separate set of transition 
> values
> + * for each grouping set being concurrently processed; for each input 
> tuple
> + * we update them all, and on group boundaries we reset some initial 
> subset
> + * of the states (the list of grouping sets is ordered from most 
> specific to
> + * least specific).  One AGG_SORTED node thus handles any number of 
> grouping
> + * sets as long as they share a sort order.

Found "initial subset" not very clear, even if I probably guessed the
right meaning.

> + * To handle multiple grouping sets that _don't_ share a sort order, we 
> use
> + * a different strategy.  An AGG_CHAINED node receives rows in sorted 
> order
> + * and returns them unchanged, but computes transition values for its own
> + * list of grouping sets.  At group boundaries, rather than returning the
> + * aggregated row (which is incompatible with the input rows), it writes 
> it
> + * to a side-channel in the form of a tuplestore.  Thus, a number of
> + * AGG_CHAINED nodes are associated with a single AGG_SORTED node (the
> + * "chain head"), which creates the side channel and, when it has 
> returned
> + * all of its own data, returns the tuples from the tuplestore to its own
> + * caller.

This paragraph deserves to be expanded imo.

> + * In order to avoid excess memory consumption from a chain of 
> alternating
> + * Sort and AGG_CHAINED nodes, we reset each child Sort node 
> preemptively,
> + * allowing us to cap the memory usage for all the sorts in the chain at
> + * twice the usage for a single node.

What does reseting 'preemtively' mean?

> + * From the perspective of aggregate transition and final functions, the
> + * only issue regarding grouping sets is this: a single call site 
> (flinfo)
> + * of an aggregate function may be used for updating several different
> + * transition values in turn. So the function must not cache in the 
> flinfo
> + * anything which logically belongs as part of the transition value (most
> + * importantly, the memory context in which the transition value exists).
> + * The support API functions (AggCheckCallContext, AggRegisterCallback) 
> are
> + * sensitive to the grouping set for which the aggregate function is
> + * currently being called.

Hm. I've seen a bunch of aggreates do this.

> + * TODO: AGG_HASHED doesn't support multiple grouping sets yet.

Are you intending to resolve this before an eventual commit? Possibly
after the 'structural' issues are resolved? Or do you think this can
safely be put of for another release?

> @@ -534,11 +603,13 @@ static void
>  advance_aggregates(AggState *aggstate, AggStatePerGroup pergroup)
>  {
>   int aggno;
> + int setno = 0;
> + int numGroupingSets = Max(aggstate->numsets, 1);
> + int numAggs = aggstate->numaggs;
>
> - for (aggno = 0; aggno < aggstate->numaggs; aggno++)
> + for (aggno = 0; aggno < numAggs; aggno++)
>   {
>   AggStatePerAgg peraggstate = &aggstate->peragg[aggno];
> - AggStatePerGroup pergroupstate = &pergroup[aggno];
>   ExprState  *filter = peraggstate->aggrefstate->aggfilter;
>   int numTransInputs = 
> peraggstate->numTransInputs;
>   int i;
> @@ -582,13 +653,16 @@ advance_aggregates(AggState *aggstate, AggStatePerGroup 
> pergroup)
>   continue;
>   }
>
> - /* OK, put the tuple into the tuplesort object */
> - if (peraggstate->numInputs == 1)
> - tuplesort_putdatum(peraggstate->sortstate,
> -
> slot->tts_values[0],
> -
> slot->tts_isnull[0]);
> - else
> - tuplesort_puttupleslot(peraggstate->sortstate, 
> slot);
> + for (setno = 0; setno < numGroupingSets; setno++)
> + {
> + /* OK, put the tuple into the tuplesort object 
> */
> + if (peraggstate->numInputs == 1)
> + 
> tuplesort_putdatum(peraggstate->sortstates[setno],
> +
> slot->tt

Re: [HACKERS] alternative compression algorithms?

[Petr Jelinek]

On 30/04/15 00:44, Tom Lane wrote:

Robert Haas  writes:

On Mon, Apr 20, 2015 at 9:03 AM, Tomas Vondra
 wrote:

Sure, it's not an ultimate solution, but it might help a bit. I do have
other ideas how to optimize this, but in the planner every milisecond
counts. Looking at 'perf top' and seeing pglz_decompress() in top 3.



I suggested years ago that we should not compress data in
pg_statistic.  Tom shot that down, but I don't understand why.  It
seems to me that when we know data is extremely frequently accessed,
storing it uncompressed makes sense.


I've not been following this thread, but I do not think your argument here
holds any water.  pg_statistic entries are generally fetched via the
syscaches, and we fixed things years ago so that toasted tuple entries
are detoasted before insertion in syscache.  So I don't believe that
preventing on-disk compression would make for any significant improvement,
at least not after the first reference within a session.


AFAICS the syscache tuples are detoasted but not decompressed before 
insertion to syscache (catcache calls toast_flatten_tuple which doesn't 
do decompression) so the pglz_decompress is still called every time.


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


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Peter Geoghegan]

On Wed, Apr 29, 2015 at 4:09 PM, Simon Riggs  wrote:
> I dislike the way that ignoring objections for a period leads them to be
> potentially discarded. I'd prefer to think that as a community we are able
> to listen to people even when they aren't continually present to reinforce
> the original objection(s).

What objection was ignored? Because, I looked just now, and the only
thing I could find of substance that you said on the MySQL syntax [1]
seemed pretty lukewarm. You mostly argued for MERGE.

> Not supporting MySQL syntax will seem like a bizarre choice to people
> watching this from a distance. I accept that the patch implements useful
> behaviour that MySQL does not implement and we thus provide enhanced syntax,
> but the default should be match on PK using the MySQL syntax.

Does it?

We can't use the MERGE syntax, because this isn't MERGE. Everything
else UPSERT-like some new and distinct custom syntax, for various
reasons.

>> Note that the syntax is quite similar to the SQLite
>> syntax of the same feature, that has ON CONFLICT IGNORE (it also has
>> ON CONFLICT REPLACE, but not ON CONFLICT UPDATE).
>
>
> Why are we not also supporting ON CONFLICT REPLACE and IGNORE then?

The point I was trying to make was that CONFLICT also appears as a
more general term than "duplicate key" or whatever.

> If we are using syntax from other products then it should be identical
> syntax, or the argument to use it doesn't stand.

I was making a narrow point about the keyword "CONFLICT". Nothing more.

> We must think about what SQL Standard people are likely to say and do. If we
> act as independently, our thought may be ignored. If we act in support of
> other previous implementations we may draw support to adopt that as the
> standard. Whatever the standard says we will eventually support, so we
> should be acting with an eye to that future.

UPSERT seems like exactly the kind of thing that the SQL standard does
not concern itself with. For example, I have a "unique index inference
specification". The SQL standard does not have anything to say about
indexes. I would be extremely surprised if the SQL standard adopted
MySQL's UPSERT thing. They omit the "SET" on the UPDATE, probably to
fix some parser ambiguity issue. While there are some similarities to
what I have here, it's a bit shoddy.

I have requirements coming out of my ears for this patch, Simon. I
think it's odd that you're taking umbrage because I supposedly ignored
something you said 6 months ago.

[1] 
http://www.postgresql.org/message-id/CA+U5nMK-efLg00FhCWk=asbet_77iss87egdsptq0ukzqdr...@mail.gmail.com
-- 
Peter Geoghegan


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Simon Riggs]

On 25 April 2015 at 14:05, Peter Geoghegan  wrote:


> > a) Why is is 'CONFLICT"? We're talking about a uniquness violation. What
> >if we, at some later point, also want to handle other kind of
> >violations? Shouldn't it be ON UNIQUE CONFLICT/ERROR/VIOLATION ...
>
> I think that naming unique violations alone would be wrong (not to
> mention ludicrously verbose). Heikki and I both feel that the CONFLICT
> keyword captures the fact that this could be a dup violation, or an
> exclusion violation. The syntax has been like this for some time, and
> hasn't been a point of contention for a long time, so I thought this
> was settled.


I dislike the way that ignoring objections for a period leads them to be
potentially discarded. I'd prefer to think that as a community we are able
to listen to people even when they aren't continually present to reinforce
the original objection(s).

Not supporting MySQL syntax will seem like a bizarre choice to people
watching this from a distance. I accept that the patch implements useful
behaviour that MySQL does not implement and we thus provide enhanced
syntax, but the default should be match on PK using the MySQL syntax.


> Note that the syntax is quite similar to the SQLite
> syntax of the same feature, that has ON CONFLICT IGNORE (it also has
> ON CONFLICT REPLACE, but not ON CONFLICT UPDATE).


Why are we not also supporting ON CONFLICT REPLACE and IGNORE then?

If we are using syntax from other products then it should be identical
syntax, or the argument to use it doesn't stand.

We must think about what SQL Standard people are likely to say and do. If
we act as independently, our thought may be ignored. If we act in support
of other previous implementations we may draw support to adopt that as the
standard. Whatever the standard says we will eventually support, so we
should be acting with an eye to that future.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

[HACKERS] Incompatible trig error handling

[John Gorman]

Two of the trigonometry functions have differing error condition behavior
between Linux and OSX. The Linux behavior follows the standard set by the
other trig functions.

On Linux:

SELECT asin(2);
> ERROR:  input is out of range
>


SELECT acos(2);
> ERROR:  input is out of range


On OSX:

SELECT asin(2);
>  asin
> --
>   NaN
> (1 row)
>


SELECT asin(2);
>  asin
> --
>   NaN
> (1 row)


The attached patch brings OSX into line with the expected behaviour and the
additional regression tests verify this.

Is this worth fixing and if so what is the next step?

Best, John


trig-v1.patch
Description: Binary data

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

Re: [HACKERS] mogrify and indent features for jsonb

[Petr Jelinek]

On 27/04/15 18:46, Petr Jelinek wrote:

On 18/04/15 20:35, Dmitry Dolgov wrote:

Sorry for late reply. Here is a slightly improved version of the patch
with the new `h_atoi` function, I hope this implementation will be more
appropriate.



It's better, but a) I don't like the name of the function b) I don't see
why we need the function at all given that it's called from only one
place and is effectively couple lines of code.

In general I wonder if it wouldn't be better to split the replacePath
into 3 functions, one replacePath, one replacePathObject,
replacePathArray and call those Object/Array ones from the replacePath
and inlining the h_atoi code into the Array one. If this was done then
it would make also sense to change the main if/else in replacePath into
a switch.

Another thing I noticed now when reading the code again - you should not
be using braces when you only have one command in the code-block.



Hi,

I worked this over a bit (I hope Dmitry won't mind) and I am now more or 
less happy with the patch. Here is list of changes I made:

- rebased to todays master (Oid conflicts, transforms patch conflicts)
- changed the main if/else if/else if/else to switch in replacePath
- split the replacePath into 3 functions (main one plus 2 helpers for 
Object and Array)

- removed the h_atoi and use the strtol directly
- renamed jsonb_indent to jsonb_pretty because we use "pretty" for 
similar use-case everywhere else

- fixed whitespace/brackets where needed
- added/reworded some comments and couple of lines in docs

I think it's ready for Andrew now.

--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From b2da4958fc75e7c7009340e6b2e4ccb155632078 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Wed, 29 Apr 2015 22:54:01 +0200
Subject: [PATCH] jsonbxcore5

---
 doc/src/sgml/func.sgml|  62 +++
 src/backend/utils/adt/jsonb.c |  81 +++-
 src/backend/utils/adt/jsonfuncs.c | 709 ++
 src/include/catalog/pg_operator.h |   8 +
 src/include/catalog/pg_proc.h |   9 +-
 src/include/utils/jsonb.h |  19 +-
 src/test/regress/expected/jsonb.out   | 424 +++-
 src/test/regress/expected/jsonb_1.out | 424 +++-
 src/test/regress/sql/jsonb.sql|  85 +++-
 9 files changed, 1805 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index dcade93..f0d8d96 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10257,6 +10257,32 @@ table2-mapping
 Do all of these key/element strings exist?
 '["a", "b"]'::jsonb ?& array['a', 'b']

+   
+||
+jsonb
+Concatentate two jsonb values into a new jsonb value
+'["a", "b"]'::jsonb || '["c", "d"]'::jsonb
+   
+   
+-
+text
+Delete the field with a specified key, or element with this
+value
+'{"a": "b"}'::jsonb - 'a' 
+   
+   
+-
+integer
+Delete the field or element with specified index (Negative
+integers count from the end)
+'["a", "b"]'::jsonb - 1 
+   
+   
+-
+text[]
+Delete the field or element with specified path
+'["a", {"b":1}]'::jsonb - '{1,b}'::text[] 
+   
   
  

@@ -10767,6 +10793,42 @@ table2-mapping
json_strip_nulls('[{"f1":1,"f2":null},2,null,3]')
[{"f1":1},2,null,3]

+  
+   jsonb_replace(target jsonb, path text[], replacement jsonb)
+ 
+   jsonb
+   
+ Returns target
+ with the section designated by  path
+ replaced by replacement.
+   
+   jsonb_replace('[{"f1":1,"f2":null},2,null,3]', '{0,f1}','[2,3,4]')
+   [{"f1":[2,3,4],"f2":null},2,null,3]
+
+   
+  
+   jsonb_pretty(from_json jsonb)
+ 
+   text
+   
+ Returns from_json
+ as indented json text.
+   
+   jsonb_pretty('[{"f1":1,"f2":null},2,null,3]')
+   
+
+ [
+ {
+ "f1": 1,
+ "f2": null
+ },
+ 2,
+ null,
+ 3
+ ]
+
+
+   
  
 

diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 7e2c359..bccc669 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -85,6 +85,8 @@ static void datum_to_jsonb(Datum val, bool is_null, JsonbInState *result,
 static void add_jsonb(Datum val, bool is_null, JsonbInState *result,
 		  Oid val_type, bool key_scalar);
 static JsonbParseState * clone_parse_state(JsonbParseState * state);
+static char *JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool indent);
+static void add_indent(StringInfo out, bool indent, int level);
 
 /*
  * jsonb type input function
@@ -422,12 +424,39 @@ jsonb_in_scalar(void *pstate, char *token, JsonTokenTyp

Re: [HACKERS] alternative compression algorithms?

[Tomas Vondra]

Hi,

On 04/29/15 23:54, Robert Haas wrote:

On Mon, Apr 20, 2015 at 9:03 AM, Tomas Vondra
 wrote:

Sure, it's not an ultimate solution, but it might help a bit. I do have
other ideas how to optimize this, but in the planner every milisecond
counts. Looking at 'perf top' and seeing pglz_decompress() in top 3.


I suggested years ago that we should not compress data in
pg_statistic.  Tom shot that down, but I don't understand why.  It
seems to me that when we know data is extremely frequently accessed,
storing it uncompressed makes sense.


I'm not convinced not compressing the data is a good idea - it suspect 
it would only move the time to TOAST, increase memory pressure (in 
general and in shared buffers). But I think that using a more efficient 
compression algorithm would help a lot.


For example, when profiling the multivariate stats patch (with multiple 
quite large histograms), the pglz_decompress is #1 in the profile, 
occupying more than 30% of the time. After replacing it with the lz4, 
the data are bit larger, but it drops to ~0.25% in the profile and 
planning the drops proportionally.


It's not a silver bullet, but it would help a lot in those cases.


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


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

Re: [HACKERS] alternative compression algorithms?

[Tom Lane]

Robert Haas  writes:
> On Mon, Apr 20, 2015 at 9:03 AM, Tomas Vondra
>  wrote:
>> Sure, it's not an ultimate solution, but it might help a bit. I do have
>> other ideas how to optimize this, but in the planner every milisecond
>> counts. Looking at 'perf top' and seeing pglz_decompress() in top 3.

> I suggested years ago that we should not compress data in
> pg_statistic.  Tom shot that down, but I don't understand why.  It
> seems to me that when we know data is extremely frequently accessed,
> storing it uncompressed makes sense.

I've not been following this thread, but I do not think your argument here
holds any water.  pg_statistic entries are generally fetched via the
syscaches, and we fixed things years ago so that toasted tuple entries
are detoasted before insertion in syscache.  So I don't believe that
preventing on-disk compression would make for any significant improvement,
at least not after the first reference within a session.

Also, it's a very long way from "some pg_statistic entries are frequently
accessed" to "all pg_statistic entries are frequently accessed".

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] Reducing tuple overhead

[Jim Nasby]

On 4/29/15 12:18 PM, Robert Haas wrote:

On Mon, Apr 27, 2015 at 5:01 PM, Jim Nasby  wrote:

The problem with just having the value is that if *anything* changes between
how you evaluated the value when you created the index tuple and when you
evaluate it a second time you'll corrupt your index. This is actually an
incredibly easy problem to have; witness how we allowed indexing
timestamptz::date until very recently. That was clearly broken, but because
we never attempted to re-run the index expression to do vacuuming at least
we never corrupted the index itself.


True.  But I guess what I don't understand is: how big a deal is this,
really?  The "uncorrupted" index can still return wrong answers to
queries.  The fact that you won't end up with index entries pointing
to completely unrelated tuples is nice, but if index scans are missing
tuples that they should see, aren't you still pretty hosed?


Maybe, maybe not. You could argue it's better to miss some rows than 
have completely unrelated ones.


My recollection is there's other scenarios where this causes problems, 
but that's from several years ago and I wasn't able to find anything on 
a quick search of archives. I've wondered the same in the past and Tom 
had reasons it was bad, but perhaps they're overstated.

--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] Minor improvement to config.sgml

[Robert Haas]

On Thu, Apr 16, 2015 at 3:30 AM, Etsuro Fujita
 wrote:
> Attached is a small patch to mark up on with  in
> doc/src/sgml/config.sgml.

Committed.

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


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Andres Freund]

On 2015-04-29 15:31:59 -0400, Robert Haas wrote:
> On Wed, Apr 29, 2015 at 3:13 PM, Stephen Frost  wrote:
> >> I still think that constraints should never be named in the syntax.
> >
> > I guess I don't see a particular problem with that..?  Perhaps I'm
> > missing something, but if there's multiple ways for something to
> > conflict, it might be nice to be able to differentiate between them?
> > Then again, I'm not sure if that's what the intent here is.
> 
> So, with unique indexes, people can create an index concurrently, then
> drop the old index concurrently, and nothing breaks.  I don't think we
> have a similar capacity for constraints at the moment, but we should.
> When somebody does that dance, the object names change, but all of the
> DML keeps working.  That's a property I'd like to preserve.

On the other hand it's way more convenient to specify a single
constraint name than several columns and a predicate. I'm pretty sure
there's situations where I a) rather live with a smaller chance of error
during a replacement of the constraint b) if we get concurrently
replaceable constraints the naming should be doable too.

I don't see your argument strong enough to argue against allowing this
*as an alternative*.

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] [PATCH] libpq: Allow specifying multiple host names to try to connect to

[Robert Haas]

On Sun, Apr 19, 2015 at 11:18 AM, Mikko Tiihonen
 wrote:
> I would like allow specifying multiple host names for libpq to try to
> connecting to. This is currently only supported if the host name resolves to
> multiple addresses. Having the support for it without complex dns setup
> would be much easier.
>
> Example:
>
> psql -h dbslave,dbmaster -p 5432 dbname
>
> psql 'postgresql://dbslave,dbmaster:5432/dbname'
>
>
> Here the idea is that without any added complexity of pgbouncer or similar
> tool I can get any libpq client to try connecting to multiple nodes until
> one answers. I have added the similar functionality to the jdbc driver few
> years ago.

I'm not sure if this exact idea is what we want to do, but I like the
concept, and I think a lot of users would find it handy.

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


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

Re: [HACKERS] patch for xidin

[Robert Haas]

On Fri, Apr 17, 2015 at 10:27 AM, Tom Lane  wrote:
>> The patch will correct it. I have justly copy some code of 'OID'. Whether we 
>> need to extract the common code?
>
> This seems like an awful lot of code to solve a problem that will never
> occur in practice.

It does seem like an awful lot of code.  We should be able to come up
with something shorter.  But the bug report is legitimate.  It's not
too much to ask that data types sanity check their inputs.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature

[Michael Paquier]

On Thu, Apr 30, 2015 at 3:30 AM, Andrew Dunstan  wrote:
>
> On 04/28/2015 04:10 PM, Andrew Dunstan wrote:
>>
>>
>> On 04/28/2015 12:03 PM, Andrew Dunstan wrote:
>>>
>>>
>>> [switching to -hackers]
>>>
>>> On 04/28/2015 11:51 AM, Andrew Dunstan wrote:


 On 04/28/2015 09:04 AM, Michael Paquier wrote:
>
> On Tue, Apr 28, 2015 at 10:01 AM, Michael Paquier
>  wrote:
>>
>> On Tue, Apr 28, 2015 at 9:55 AM, Andrew Dunstan wrote:
>>>
>>> w.r.t MSVC builds, it looks like we need entries in
>>> $contrib_extraincludes
>>> in src/tools/msvc/Mkvcbuild.pm at the very least.
>>
>> If our goal is to put back to green the Windows nodes as quick as
>> possible, we could bypass their build this way , and we'll
>> additionally need to update the install script and
>> vcregress.pl/contribcheck to bypass those modules accordingly. Now I
>> don't think that we should make the things produced inconsistent.
>
> OK, attached are two patches to put back the buildfarm nodes using MSVC
> to green
> - 0001 adds support for build and installation of the new transform
> modules: hstore_plperl, hstore_plpython and  ltree_plpython. Note that
> this patch is enough to put back the buildfarm to a green state for
> MSVC, but it disables the regression tests for those modules,
> something addressed in the next patch...
> - 0002 adds support for regression tests for the new modules. The
> thing is that we need to check the major version version of Python
> available in configuration and choose what are the extensions to
> preload before the tests run. It is a little bit hacky... But it has
> the merit to work, and I am not sure we could have a cleaner solution
> by looking at the Makefiles of the transform modules that use
> currently conditions based on $(python_majorversion).
> Regards,



 I have pushed the first of these with some tweaks.

 I'm looking at the second.


>>>
>>>
>>> Regarding this second patch - apart from the buggy python version test
>>> which I just fixed in the first patch, I see this:
>>>
>>>+   if ($pyver eq "2")
>>>+   {
>>>+   push @opts, "--load-extension=plpythonu";
>>>+   push @opts, '--load-extension=' . $module . 'u';
>>>+   }
>>>
>>>
>>> But what do we do for Python3?
>>>
>>> Do we actually have a Windows machine building with Python3?
>>
>>
>>
>> The answer seems to be "probably not". When I tried enabling this with
>> bowerbird I got a ton of errors like these:
>>
>>plpy_cursorobject.obj : error LNK2001: unresolved external symbol
>>PyObject_SelfIter [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
>>plpy_cursorobject.obj : error LNK2019: unresolved external symbol
>>__imp_PyType_Ready referenced in function PLy_cursor_init_type
>>[C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
>>
>>
>> Something else to fix I guess.
>>
>>
>
> OK, I fixed this as Michael suggested by installing the 64 bit python3 (it's
> a pity that python.org didn't offer that to me as a download on their front
> page - that's a bit ugly).
>
> However, when it came to running these tests that was a miserable failure.
> And indeed, we don't run any regression tests for plpython3 on MSVC.

I arrived to the same conclusion. Perhaps we shall write a TODO in the
code or on the wiki to not forget about that..

> So I committed this with just python2 tests enabled. All the buildfarm MSVC
> hosts seem to be using python2 anyway.

OK, thanks.

> Meanwhile, we have some work to do on the mingw/gcc side.
>
> These changes help make some progress - they let compilation succeed for
> hstore_plperl but I still get linkage failures. I suspect we need some
> things marked for export that we haven't to be marked needed before.
>
> [...]

I'll try to have a look at that a bit if possible...
Regards,
-- 
Michael


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

Re: [HACKERS] alternative compression algorithms?

[Robert Haas]

On Mon, Apr 20, 2015 at 9:03 AM, Tomas Vondra
 wrote:
> Sure, it's not an ultimate solution, but it might help a bit. I do have
> other ideas how to optimize this, but in the planner every milisecond
> counts. Looking at 'perf top' and seeing pglz_decompress() in top 3.

I suggested years ago that we should not compress data in
pg_statistic.  Tom shot that down, but I don't understand why.  It
seems to me that when we know data is extremely frequently accessed,
storing it uncompressed makes sense.

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


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

Re: [HACKERS] Proposal: knowing detail of config files via SQL

[David Steele]

On 4/29/15 5:16 PM, Robert Haas wrote:
> On Fri, Apr 24, 2015 at 2:40 PM, David Steele  wrote:
>>   
>>The view pg_file_settings provides access to
>> run-time parameters
>>that are defined in configuration files via SQL.  In contrast to
>>pg_settings a row is provided for each
>> occurrence
>>of the parameter in a configuration file.  This is helpful for
>> discovering why one value
>>may have been used in preference to another when the parameters were
>> loaded.
>>   
> 
> This seems to imply that this gives information about only a subset of
> configuration files; specifically, those auto-generated based on SQL
> commands - i.e. postgresql.conf.auto.  But I think it's supposed to
> give information about all configuration files, regardless of how
> generated.  Am I wrong?  If not, I'd suggest "run-time parameters that
> are defined in configuration files via SQL" -> "run-time parameters
> stored in configuration files".

The view does give information about all configuration files regardless
of how they were created.

That's what I intended the text to say but I think your phrasing is clearer.

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Proposal: knowing detail of config files via SQL

[Robert Haas]

On Fri, Apr 24, 2015 at 2:40 PM, David Steele  wrote:
>   
>The view pg_file_settings provides access to
> run-time parameters
>that are defined in configuration files via SQL.  In contrast to
>pg_settings a row is provided for each
> occurrence
>of the parameter in a configuration file.  This is helpful for
> discovering why one value
>may have been used in preference to another when the parameters were
> loaded.
>   

This seems to imply that this gives information about only a subset of
configuration files; specifically, those auto-generated based on SQL
commands - i.e. postgresql.conf.auto.  But I think it's supposed to
give information about all configuration files, regardless of how
generated.  Am I wrong?  If not, I'd suggest "run-time parameters that
are defined in configuration files via SQL" -> "run-time parameters
stored in configuration files".

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


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

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Wed, Apr 29, 2015 at 10:47 AM, Stephen Frost  wrote:
> Here is the latest revision of this patch.

I think this patch is too big and does too many things.  It should be
broken up into small patches which can be discussed and validated
independently.  The fact that your commit message is incredibly long
is a sign that there's too much going on here, and that message
doesn't even cover all of it.

It seems to me that the core change here is really to pg_dump.  You're
adding the ability for pg_dump to dump and restore privileges on
objects in pg_dump.  That capability is a complete - and useful -
feature in its own right, with no other changes.

Then the next thing you've got here is a series of patches that change
the rights to execute various utility functions.  Some of those, like
the changes to pg_stop_backup(), are pretty much a slam-dunk, because
they don't really change user-visible behavior; they just give the DBA
more options.  Others, like the changes to replication permissions,
are likely to be more controversial.  You should split the stuff
that's a slam-dunk apart from the stuff that makes policy decisions,
and plan to commit the former changes first, and the latter changes
only if and when there is very clear agreement on the specific
policies to be changed.

Finally, you've got the idea of making pg_ a reserved prefix for
roles, adding some predefined roles, and giving them some predefined
privileges.  That should be yet another patch.

I think that if you commit this the way you have it today, everybody
will go, oh, look, Stephen committed something, but it looks
complicated, I won't pay attention.  And then, six months from now
when we're in beta, or maybe after final, people will start looking at
this and realizing that there are parts of it they hate, but it will
be hard to fix at that point.  Breaking it out will hopefully allow
more discussion on the individual features of in here, of which there
are probably at least four.  It will also make it easier to revert
part of it rather than all of it if that turns out to be needed.

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


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

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Robert Haas]

On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai  wrote:
> The attached patch v13 is revised one according to the suggestion
> by Robert.

Thanks.

The last hunk in foreign.c is a useless whitespace change.

+   /* actually, not shift members */

Change to: "shift of 0 is the same as copying"

But actually, do we really need all of this?  I think you could reduce
the size of this function to three lines of code if you just did this:

x = -1;
while ((x = bms_next_member(inputset, x)) >= 0)
outputset = bms_add_member(inputset, x + shift);

It might be very slightly slower, but I think it would be worth it to
reduce the amount of code needed.

+* 5. Consider paths added by FDW, in case when both of outer and
+* inner relations are managed by the same driver.

Change to: "If both inner and outer relations are managed by the same
FDW, give it a chance to push down joins."

+* 6. At the last, consider paths added by extension, in addition to the
+* built-in paths.

Change to: "Finally, give extensions a chance to manipulate the path list."

+* Fetch relation-id, if this foreign-scan node actuall scans on
+* a particular real relation. Elsewhere, InvalidOid shall be
+* informed to the FDW driver.

Change to: "If we're scanning a base relation, look up the OID.  (We
can skip this if scanning a join relation.)"

+* Sanity check. Pseudo scan tuple-descriptor shall be constructed
+* based on the fdw_ps_tlist, excluding resjunk=true, so we need to
+* ensure all valid TLEs have to locate prior to junk ones.

Is the goal here to make attribute numbers match up?  If so, between
where and where?  If not, please explain further.

+   if (splan->scan.scanrelid == 0)
+   {
...
+   }
splan->scan.scanrelid += rtoffset;

Does this need an "else"?  It seems surprising that you would offset
scanrelid even if it's starting out as zero.

(Note that there are two instances of this pattern.)

+ * 'found' : indicates whether RelOptInfo is actually constructed.
+ * true, if it was already built and on the cache.

Leftover hunk.  Revert this.

+typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,

Whitespace is wrong, still.

+ * An optional fdw_ps_tlist is used to map a reference to an attribute of
+ * underlying relation(s) on a pair of INDEX_VAR and alternative varattno.

on -> onto

+ * It looks like a scan on pseudo relation that is usually result of
+ * relations join on remote data source, and FDW driver is responsible to
+ * set expected target list for this.

Change to: "When fdw_ps_tlist is used, this represents a remote join,
and the FDW driver is responsible for setting this field to an
appropriate value."

If FDW returns records as foreign-
+ * table definition, just put NIL here.

I think this is just referring to the non-join case; if so, just drop
it.  Otherwise, I'm confused and need a further explanation.

+ *  Note that since Plan trees can be copied, custom scan providers *must*

Extra space before "Note"

+   Bitmapset  *custom_relids;  /* set of relid (index of range-tables)
+*
represented by this node */

Maybe "RTIs this node generates"?

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


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

Re: [HACKERS] Replication identifiers, take 4

[Petr Jelinek]

On 29/04/15 22:12, David Rowley wrote:

On 25 April 2015 at 00:32, Andres Freund mailto:and...@anarazel.de>> wrote:


Attached is a patch that does this, and some more, renaming. That was
more work than I'd imagined.  I've also made the internal naming in
origin.c more consistent/simpler and did a bunch of other cleanup.


Hi,

It looks like bowerbird is not too happy with this, neither is my build
environment.

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2015-04-29%2019%3A31%3A06

The attached seems to fix it.
I put the include in the origin.h rather than in the origin.c as the
DoNotReplicateId macro uses UINT16_MAX and is also used in xact.c.



I think correct fix is using PG_UINT16_MAX.


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


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

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Robert Haas]

On Mon, Apr 27, 2015 at 5:05 AM, Shigeru HANADA
 wrote:
> Currently INNER JOINs with unsafe join conditions are not pushed down, so 
> such test is not in the suit.  As you say, in theory, INNER JOINs can be 
> pushed down even they have push-down-unsafe join conditions, because such 
> conditions can be evaluated no local side against rows retrieved without 
> those conditions.

I suspect it's worth trying to do the pushdown if there is at least
one safe joinclause.  If there are none, fetching a Cartesian product
figures to be a loser.

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


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

Re: [HACKERS] Replication identifiers, take 4

[David Rowley]

On 25 April 2015 at 00:32, Andres Freund  wrote:

>
> Attached is a patch that does this, and some more, renaming. That was
> more work than I'd imagined.  I've also made the internal naming in
> origin.c more consistent/simpler and did a bunch of other cleanup.
>
>
Hi,

It looks like bowerbird is not too happy with this, neither is my build
environment.

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2015-04-29%2019%3A31%3A06

The attached seems to fix it.
I put the include in the origin.h rather than in the origin.c as the
DoNotReplicateId macro uses UINT16_MAX and is also used in xact.c.

Regards

David Rowley


UINT16_MAX_fix.patch
Description: Binary data

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

Re: [HACKERS] mogrify and indent features for jsonb

[Jan de Visser]

On April 29, 2015 03:09:51 PM Andrew Dunstan wrote:
> On 04/29/2015 01:19 PM, Robert Haas wrote:
> > On Mon, Apr 27, 2015 at 6:41 PM, Andrew Dunstan  
wrote:
> >> There's one exception I, at least, have to this rule, namely when there's
> >> a
> >> corresponding compound if or else. I personally find this unaesthetic to
> >> put>> 
> >> it mildly:
> >> if (condition)
> >> 
> >> statement;
> >> 
> >> else
> >> {
> >> 
> >> block of statements;
> >> 
> >> }
> > 
> > Hmm, I don't dislike that style.  If somebody submitted a patch with
> > braces around the lone statement, I would remove them before
> > committing.
> > 
> > 
> 
> It's a matter of taste, but I find things a lot easier to understand
> when they are symmetrical. Thus I like all the branches of an "if" to be
> either in a block or not, and I like braces to line up either
> horizontally or vertically. Perhaps this reflects my history, where I
> wrote huge amounts of Ada and other non-C-like languages, well before I
> ever wrote lots of C or C-ish languages.
> 
> 
> Another case where I think putting a single statement in a block makes
> sense is where the condition of the "if" spreads across more than one
> line. This works particularly well with our BSD style brace placement.

I'm sure that many, many bits have been spilled over this, reaching way back 
into the stone age of computing, sometimes almost reaching emacs-vs-vi levels 
of intensity.

My position is the better-safe-than-sorry corner, which says to always add 
braces, even if there's only one statement. Because one day somebody will be 
in a rush, and will add a second statement without adding the braces, and 
things will explode horribly.

But that's just me.

jan



-- 
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] mogrify and indent features for jsonb

[Kevin Grittner]

Andrew Dunstan  wrote:

> It's a matter of taste, but I find things a lot easier to
> understand when they are symmetrical. Thus I like all the
> branches of an "if" to be either in a block or not, and I like
> braces to line up either horizontally or vertically. Perhaps this
> reflects my history, where I wrote huge amounts of Ada and other
> non-C-like languages, well before I ever wrote lots of C or C-ish
> languages.
>
> Another case where I think putting a single statement in a block
> makes sense is where the condition of the "if" spreads across
> more than one line. This works particularly well with our BSD
> style brace placement.

My personal preferences are the same on all of that, especially
that the closing paren, brace, or bracket should be either in the
same line or the same column as its mate.  If we were going to open
a green-field discussion about what style to *choose* I would be
arguing for all of the above (plus a few other things which are not
current PostgreSQL style).

That said, I feel very strongly that it is important that everyone
use the *same* style.  It is far more important to me that we stick
to a single style than that the style match my personal
preferences.  The project style seems to me to be that a single
statement is not put into braces unless needed for correctness or
to prevent warnings about ambiguity from the compilers.

By the way, my preference for the above are not strong enough to
want to open up the style choices to re-evaluation.  PLEASE, no!

--
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] Replication identifiers, take 4

[Robert Haas]

On Tue, Apr 28, 2015 at 10:00 PM, Peter Eisentraut  wrote:
> On 4/24/15 4:29 PM, Andres Freund wrote:
>>> Shouldn't this be backed up by pg_dump(all?)?
>>
>> Given it deals with LSNs and is, quite fundamentally due to concurrency, non 
>> transactional, I doubt it's worth it. The other side's slots also aren't 
>> going to be backed up as pg dump obviously can't know about then. So the 
>> represented data won't make much sense.
>
> I agree it might not be the best match.  But we should consider that we
> used to say, a backup by pg_dumpall plus configuration files is a
> complete backup.  Now we have replication slots and possibly replication
> identifiers and maybe similar features in the future that are not
> covered by this backup method.

That's true.  But if you did backup the replication slots with
pg_dump, and then you restored them as part of restoring the dump,
your replication setup would be just as broken as if you had never
backed up those replication slots at all.

I think the problem here is that replication slots are part of
*cluster* configuration, not individual node configuration.  If you
back up a set of nodes that make up a cluster, and then restore them,
you might hope that you will end up with working slots established
between the same pairs of machines that had working slots between them
before.  But I don't see a way to make that happen when you look at it
from the point of view of backing up and restoring just one node.  As
we get better clustering facilities into core, we may develop more
instances of this problem; the best way of solving it is not clear to
me.

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


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

Re: [HACKERS] why does enum_endpoint call GetTransactionSnapshot()?

[Robert Haas]

On Tue, Apr 28, 2015 at 9:28 PM, Bruce Momjian  wrote:
> On Sat, Feb 14, 2015 at 05:29:43PM -0500, Robert Haas wrote:
>> On Sat, Feb 14, 2015 at 5:12 PM, Tom Lane  wrote:
>> > Robert Haas  writes:
>> >> I think this is probably a holdover from before the death of
>> >> SnapshotNow, and that we should just pass NULL to
>> >> systable_beginscan_ordered() here, the same as we do for other catalog
>> >> accesses.  Barring objections, I'll go make that change.
>> >
>> > Seems reasonable to me, but why are you only looking at that one and
>> > not the identical code in enum_range_internal()?
>>
>> I noticed that one after hitting send.  I think we should change them both.
>
> Any news on this?

Done.

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


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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

On Tue, Apr 28, 2015 at 09:15:49AM -0400, Bruce Momjian wrote:
> > It seems to me that waiting for 9.6 for what's arguably a bug fix is too
> > much.  It's not like this is a new feature.  Why don't we just make sure
> > it is as correct as possible and get it done for 9.5?  It's not even in
> > beta yet, nor feature freeze.
> 
> Well, I applied what I thought would work, but did not handle three
> cases:
> 
> *  checking of hasoids by index specifications
> *  queries with multiple LIKE'ed tables
> *  matching inheritance behavior
> 
> I am unclear if I should be addressing such complex issues at this point
> in the development cycle.  I can certainly apply this patch, but I need
> someone else to tell me it is good and should be applied.  I am also
> thinking such review time would be better spent on patches submitted
> long before mine.

I have added regression tests to the patch, attached.  I have included
Tom's test that doesn't directly use LIKE.

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

  + Everyone has their own god. +
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 0a55db4..a76f957
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
***
*** 56,61 
--- 56,62 
  #include "rewrite/rewriteManip.h"
  #include "utils/acl.h"
  #include "utils/builtins.h"
+ #include "utils/guc.h"
  #include "utils/lsyscache.h"
  #include "utils/rel.h"
  #include "utils/syscache.h"
*** transformCreateStmt(CreateStmt *stmt, co
*** 150,155 
--- 151,157 
  	Oid			namespaceid;
  	Oid			existing_relid;
  	ParseCallbackState pcbstate;
+ 	bool		like_found = false;
  
  	/*
  	 * We must not scribble on the passed-in CreateStmt, so copy it.  (This is
*** transformCreateStmt(CreateStmt *stmt, co
*** 242,248 
  
  	/*
  	 * Run through each primary element in the table creation clause. Separate
! 	 * column defs from constraints, and do preliminary analysis.
  	 */
  	foreach(elements, stmt->tableElts)
  	{
--- 244,253 
  
  	/*
  	 * Run through each primary element in the table creation clause. Separate
! 	 * column defs from constraints, and do preliminary analysis.  We have to
! 	 * process column-defining clauses first because it can control the
! 	 * presence of columns which are referenced by columns referenced by
! 	 * constraints.
  	 */
  	foreach(elements, stmt->tableElts)
  	{
*** transformCreateStmt(CreateStmt *stmt, co
*** 254,267 
  transformColumnDefinition(&cxt, (ColumnDef *) element);
  break;
  
- 			case T_Constraint:
- transformTableConstraint(&cxt, (Constraint *) element);
- break;
- 
  			case T_TableLikeClause:
  transformTableLikeClause(&cxt, (TableLikeClause *) element);
  break;
  
  			default:
  elog(ERROR, "unrecognized node type: %d",
  	 (int) nodeTag(element));
--- 259,277 
  transformColumnDefinition(&cxt, (ColumnDef *) element);
  break;
  
  			case T_TableLikeClause:
+ if (!like_found)
+ {
+ 	cxt.hasoids = false;
+ 	like_found = true;
+ }
  transformTableLikeClause(&cxt, (TableLikeClause *) element);
  break;
  
+ 			case T_Constraint:
+ /* process later */
+ break;
+ 
  			default:
  elog(ERROR, "unrecognized node type: %d",
  	 (int) nodeTag(element));
*** transformCreateStmt(CreateStmt *stmt, co
*** 269,274 
--- 279,305 
  		}
  	}
  
+ 	if (like_found)
+ 	{
+ 		/*
+ 		 * To match INHERITS, the existance of any LIKE table with OIDs
+ 		 * causes the new table to have oids.  For the same reason,
+ 		 * WITH/WITHOUT OIDs is also ignored with LIKE.  We prepend
+ 		 * because the first oid option list entry is honored.  Our
+ 		 * prepended WITHOUT OIDS clause will be overridden if an
+ 		 * inherited table has oids.
+ 		 */
+ 		stmt->options = lcons(makeDefElem("oids",
+ 			  (Node *)makeInteger(cxt.hasoids)), stmt->options);
+ 	}
+ 
+ 	foreach(elements, stmt->tableElts)
+ 	{
+ 		Node	   *element = lfirst(elements);
+ 
+ 		if (nodeTag(element) == T_Constraint)
+ 			transformTableConstraint(&cxt, (Constraint *) element);
+ 	}
  	/*
  	 * transformIndexConstraints wants cxt.alist to contain only index
  	 * statements, so transfer anything we already have into save_alist.
*** transformTableLikeClause(CreateStmtConte
*** 860,865 
--- 891,899 
  		}
  	}
  
+ 	/* We use oids if at least one LIKE'ed table has oids. */
+ 	cxt->hasoids = cxt->hasoids || relation->rd_rel->relhasoids;
+ 
  	/*
  	 * Copy CHECK constraints if requested, being careful to adjust attribute
  	 * numbers so they match the child.
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
new file mode 100644
index 8ba7bbc..41ceb87
*** a/src/test/regress/expected/create_table.out
--- b/src/test/regress/expected/create_

Re: [HACKERS] Help needed for PL/Ruby

[Szymon Guz]

Hi Devrim,
I will take a look at this.

regards,
Szymon

On 29 April 2015 at 18:24, Devrim Gündüz  wrote:

>
> Hi,
>
> Anyone? :)
>
> Regards, Devrim
>
> On Wed, 2015-03-18 at 15:19 +0200, Devrim Gündüz wrote:
> > Hi,
> >
> > Background info first: PL/Ruby was originally maintained by Guy Decoux,
> > who passed away in 2008: https://www.ruby-forum.com/topic/166658 . After
> > his death, Akinori MUSHA forked the project and maintained it until
> > 2010: https://github.com/knu/postgresql-plruby . Last release was on Jan
> > 2010, and recent distros started throwing build errors.
> >
> > I was having similar build issues while trying to RPMify PL/Ruby, and
> > finally stepped up the plate and tried to fix those build issues by
> > forking the project:
> >
> > https://github.com/devrimgunduz/postgresql-plruby/
> >
> > I mainly applied patches from Fedora, and also did some basic cleanup.
> > However, I don't know Ruby and have limited C skills, so I need some
> > help on these:
> >
> > * Complete the extension support: I committed initial infrastructure for
> > it, but I don't think it is ready yet.
> >
> > * Fix the FIXME:
> >
> https://github.com/devrimgunduz/postgresql-plruby/blob/master/src/plpl.c#L844
> >
> > * Documentation review and update: The recent example is against
> > PostgreSQL 8.0. A recent Ruby example would be good, and also we need
> > updates for creating the language.
> >
> > Any contributions are welcome. I recently released 0.5.5 that at least
> > is ready for testing.
> >
> > I want to remind that I am not a Ruby guy, so this is really a community
> > stuff for me.
> >
> > Thanks by now.
> >
> > Regards,
>
>
> --
> Devrim GÜNDÜZ
> Principal Systems Engineer @ EnterpriseDB: http://www.enterprisedb.com
> PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
> Twitter: @DevrimGunduz , @DevrimGunduzTR
>
>


-- 


  Szymon

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Wed, Apr 29, 2015 at 3:21 PM, Jeff Janes  wrote:
> On Wed, Apr 29, 2015 at 11:27 AM, Robert Haas  wrote:
>> On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes  wrote:
>> > This commit is causing a compiler warning for me in non-cassert builds:
>> >
>> > funcapi.c: In function 'get_func_trftypes':
>> > funcapi.c:890: warning: unused variable 'procStruct'
>> >
>> > Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.
>>
>> I took a stab at fixing this via a slightly different method.  Let me
>> know whether that worked.
>
> It worked, thanks.

Cool.

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


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Robert Haas]

On Wed, Apr 29, 2015 at 3:13 PM, Stephen Frost  wrote:
> My general feeling is "keep it short" but I'm not particular beyond
> that.  I do like the idea that we'll be able to support more options in
> the future.

Yeah.  To me "ON CONFLICT" sounds like "if there's a war, then...".
So I like ON DUPLICATE, which I think is clear as crystal.  But if we
settle on something else I won't cry myself to sleep.

>> > * Don't change the names of the pseudo-alias EXCLUDED.* (or the alias
>> > TARGET.*). Those seem fine to me as well.
>>
>> There seem to be a few votes for NEW and OLD.  That's what I proposed
>> originally, and (surprise, surprise) I still like that better too.
>
> I was promoting NEW/OLD, until I realized that we'd end up having a
> problem in trigger functions because NEW/OLD are already defined there,
> unless you have a suggestion for how to improve on that?

I don't.  That's a good point.

>> > * Finally, add "ON CONFLICT ON CONSTRAINT my_constraint" support, so
>> > you can name (exactly one) constraint by name. Particularly useful for
>> > unique constraints. I really don't want to make this accept multiple
>> > constraints, even though we may infer multiple constraints, because
>> > messy, and is probably too complex to every be put to good use
>> > (bearing in mind that exclusion constraints, that really need this,
>> > will still only be supported by the IGNORE/DO NOTHING variant).
>>
>> I still think that constraints should never be named in the syntax.
>
> I guess I don't see a particular problem with that..?  Perhaps I'm
> missing something, but if there's multiple ways for something to
> conflict, it might be nice to be able to differentiate between them?
> Then again, I'm not sure if that's what the intent here is.

So, with unique indexes, people can create an index concurrently, then
drop the old index concurrently, and nothing breaks.  I don't think we
have a similar capacity for constraints at the moment, but we should.
When somebody does that dance, the object names change, but all of the
DML keeps working.  That's a property I'd like to preserve.

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


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

Re: [HACKERS] [PATCH] Add transforms feature

[Jeff Janes]

On Wed, Apr 29, 2015 at 11:27 AM, Robert Haas  wrote:

> On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes  wrote:
> > This commit is causing a compiler warning for me in non-cassert builds:
> >
> > funcapi.c: In function 'get_func_trftypes':
> > funcapi.c:890: warning: unused variable 'procStruct'
> >
> > Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.
>
> I took a stab at fixing this via a slightly different method.  Let me
> know whether that worked.
>

It worked, thanks.

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Apr 27, 2015 at 7:21 PM, Peter Geoghegan  wrote:
> > On Mon, Apr 27, 2015 at 10:20 AM, Bruce Momjian  wrote:
> >> Agreed, and I like the DO [ UPDATE | NOTHING ] too.
> >
> > Here is what I think I need to do:
> >
> > * Don't change the ON CONFLICT spelling.
> 
> What I proposed originally was ON DUPLICATE.  Not ON CONFLICT.  And I
> still like that better.  ON UNIQUE CONFLICT, which Andres mentioned,
> gets us there too, but it's

My general feeling is "keep it short" but I'm not particular beyond
that.  I do like the idea that we'll be able to support more options in
the future.

> > * Don't change the names of the pseudo-alias EXCLUDED.* (or the alias
> > TARGET.*). Those seem fine to me as well.
> 
> There seem to be a few votes for NEW and OLD.  That's what I proposed
> originally, and (surprise, surprise) I still like that better too.

I was promoting NEW/OLD, until I realized that we'd end up having a
problem in trigger functions because NEW/OLD are already defined there,
unless you have a suggestion for how to improve on that?

> > * Finally, add "ON CONFLICT ON CONSTRAINT my_constraint" support, so
> > you can name (exactly one) constraint by name. Particularly useful for
> > unique constraints. I really don't want to make this accept multiple
> > constraints, even though we may infer multiple constraints, because
> > messy, and is probably too complex to every be put to good use
> > (bearing in mind that exclusion constraints, that really need this,
> > will still only be supported by the IGNORE/DO NOTHING variant).
> 
> I still think that constraints should never be named in the syntax.

I guess I don't see a particular problem with that..?  Perhaps I'm
missing something, but if there's multiple ways for something to
conflict, it might be nice to be able to differentiate between them?
Then again, I'm not sure if that's what the intent here is.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] mogrify and indent features for jsonb

[Andrew Dunstan]

On 04/29/2015 01:19 PM, Robert Haas wrote:

On Mon, Apr 27, 2015 at 6:41 PM, Andrew Dunstan  wrote:

There's one exception I, at least, have to this rule, namely when there's a
corresponding compound if or else. I personally find this unaesthetic to put
it mildly:

if (condition)
statement;
else
{
block of statements;
}

Hmm, I don't dislike that style.  If somebody submitted a patch with
braces around the lone statement, I would remove them before
committing.





It's a matter of taste, but I find things a lot easier to understand 
when they are symmetrical. Thus I like all the branches of an "if" to be 
either in a block or not, and I like braces to line up either 
horizontally or vertically. Perhaps this reflects my history, where I 
wrote huge amounts of Ada and other non-C-like languages, well before I 
ever wrote lots of C or C-ish languages.



Another case where I think putting a single statement in a block makes 
sense is where the condition of the "if" spreads across more than one 
line. This works particularly well with our BSD style brace placement.


cheers

andrew


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Robert Haas]

On Mon, Apr 27, 2015 at 7:21 PM, Peter Geoghegan  wrote:
> On Mon, Apr 27, 2015 at 10:20 AM, Bruce Momjian  wrote:
>> Agreed, and I like the DO [ UPDATE | NOTHING ] too.
>
> Here is what I think I need to do:
>
> * Don't change the ON CONFLICT spelling.

What I proposed originally was ON DUPLICATE.  Not ON CONFLICT.  And I
still like that better.  ON UNIQUE CONFLICT, which Andres mentioned,
gets us there too, but it's

> * Don't change the names of the pseudo-alias EXCLUDED.* (or the alias
> TARGET.*). Those seem fine to me as well.

There seem to be a few votes for NEW and OLD.  That's what I proposed
originally, and (surprise, surprise) I still like that better too.

> * Change the syntax to put the WHERE clause used to infer partial
> indexes outside parens.

+1.

> * Change the syntax to make this work, by adding the fully reserved
> keyword DO. Assuming you have a partial index (WHERE is_active) on the
> column "key", you're left with:
>
> INSERT  ON CONFLICT (key) where is_active DO UPDATE set ... WHERE ... ;
>
> or:
>
> INSERT  ON CONFLICT (key) where is_active DO NOTHING;
>
> The DO keyword makes this work where it cannot otherwise, because it's
> a RESERVED_KEYWORD.

Seems fine.

> * Finally, add "ON CONFLICT ON CONSTRAINT my_constraint" support, so
> you can name (exactly one) constraint by name. Particularly useful for
> unique constraints. I really don't want to make this accept multiple
> constraints, even though we may infer multiple constraints, because
> messy, and is probably too complex to every be put to good use
> (bearing in mind that exclusion constraints, that really need this,
> will still only be supported by the IGNORE/DO NOTHING variant).

I still think that constraints should never be named in the syntax.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature

[Andrew Dunstan]

On 04/28/2015 04:10 PM, Andrew Dunstan wrote:


On 04/28/2015 12:03 PM, Andrew Dunstan wrote:


[switching to -hackers]

On 04/28/2015 11:51 AM, Andrew Dunstan wrote:


On 04/28/2015 09:04 AM, Michael Paquier wrote:

On Tue, Apr 28, 2015 at 10:01 AM, Michael Paquier
 wrote:

On Tue, Apr 28, 2015 at 9:55 AM, Andrew Dunstan wrote:
w.r.t MSVC builds, it looks like we need entries in 
$contrib_extraincludes

in src/tools/msvc/Mkvcbuild.pm at the very least.

If our goal is to put back to green the Windows nodes as quick as
possible, we could bypass their build this way , and we'll
additionally need to update the install script and
vcregress.pl/contribcheck to bypass those modules accordingly. Now I
don't think that we should make the things produced inconsistent.
OK, attached are two patches to put back the buildfarm nodes using 
MSVC to green

- 0001 adds support for build and installation of the new transform
modules: hstore_plperl, hstore_plpython and  ltree_plpython. Note that
this patch is enough to put back the buildfarm to a green state for
MSVC, but it disables the regression tests for those modules,
something addressed in the next patch...
- 0002 adds support for regression tests for the new modules. The
thing is that we need to check the major version version of Python
available in configuration and choose what are the extensions to
preload before the tests run. It is a little bit hacky... But it has
the merit to work, and I am not sure we could have a cleaner solution
by looking at the Makefiles of the transform modules that use
currently conditions based on $(python_majorversion).
Regards,



I have pushed the first of these with some tweaks.

I'm looking at the second.





Regarding this second patch - apart from the buggy python version 
test which I just fixed in the first patch, I see this:


   +   if ($pyver eq "2")
   +   {
   +   push @opts, "--load-extension=plpythonu";
   +   push @opts, '--load-extension=' . $module . 'u';
   +   }


But what do we do for Python3?

Do we actually have a Windows machine building with Python3?



The answer seems to be "probably not". When I tried enabling this with 
bowerbird I got a ton of errors like these:


   plpy_cursorobject.obj : error LNK2001: unresolved external symbol
   PyObject_SelfIter [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
   plpy_cursorobject.obj : error LNK2019: unresolved external symbol
   __imp_PyType_Ready referenced in function PLy_cursor_init_type
   [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]


Something else to fix I guess.




OK, I fixed this as Michael suggested by installing the 64 bit python3 
(it's a pity that python.org didn't offer that to me as a download on 
their front page - that's a bit ugly).


However, when it came to running these tests that was a miserable 
failure. And indeed, we don't run any regression tests for plpython3 on 
MSVC.


So I committed this with just python2 tests enabled. All the buildfarm 
MSVC hosts seem to be using python2 anyway.


Meanwhile, we have some work to do on the mingw/gcc side.

These changes help make some progress - they let compilation succeed for 
hstore_plperl but I still get linkage failures. I suspect we need some 
things marked for export that we haven't to be marked needed before.


diff --git a/contrib/hstore_plperl/Makefile b/contrib/hstore_plperl/Makefile
index ddf6036..81fe1af 100644
--- a/contrib/hstore_plperl/Makefile
+++ b/contrib/hstore_plperl/Makefile
@@ -3,7 +3,7 @@
 MODULE_big = hstore_plperl
 OBJS = hstore_plperl.o

-PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(perl_archlibexp)/CORE 
-I$(top_srcdir)/contrib/hstore

+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl -I$(top_srcdir)/contrib/hstore

 EXTENSION = hstore_plperl hstore_plperlu
 DATA = hstore_plperl--1.0.sql hstore_plperlu--1.0.sql
@@ -21,3 +21,6 @@ top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif
+
+override CPPFLAGS := $(CPPFLAGS) -DPLPERL_HAVE_UID_GID 
-I$(perl_archlibexp)/CORE

+override CFLAGS += -Wno-comment



The linkage failures look like:


x86_64-w64-mingw32-gcc -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute 
-Wformat-security -fno-strict-aliasing -fwrapv 
-fexcess-precision=standard -g -O2  -Wno-comment -I../../src/pl/plperl 
-I../../contrib/hstore -I. -I. -I../../src/include 
-I./src/include/port/win32 -DEXEC_BACKEND 
-I/c/prog/3p64/include/libxml2  -I/c/prog/3p64/include 
-I/c/prog/3p64/openssl/include "-I../../src/include/port/win32" 
-DPLPERL_HAVE_UID_GID -IC:/Perl64/lib/CORE  -c -o hstore_plperl.o 
hstore_plperl.c
x86_64-w64-mingw32-gcc -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute 
-Wformat-security -fno-strict-aliasing -fwrapv 
-fexcess-precision=standard -g -O2  -Wno-comment  -shared -static-libgcc 
-o hstore_plperl.dll  hstore_plperl.o

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes  wrote:
> This commit is causing a compiler warning for me in non-cassert builds:
>
> funcapi.c: In function 'get_func_trftypes':
> funcapi.c:890: warning: unused variable 'procStruct'
>
> Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.

I took a stab at fixing this via a slightly different method.  Let me
know whether that worked.

Thanks,

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


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

Re: [HACKERS] cache invalidation for PL/pgsql functions

[Robert Haas]

On Tue, Apr 28, 2015 at 3:59 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> rhaas=# create table foo (a int);
>> CREATE TABLE
>> rhaas=# create or replace function test (x foo) returns int as $$begin
>> return x.b; end$$ language plpgsql;
>> CREATE FUNCTION
>> rhaas=# alter table foo add column b int;
>> ALTER TABLE
>> rhaas=# select test(null::foo);
>> ERROR:  record "x" has no field "b"
>> LINE 1: SELECT x.b
>>^
>> QUERY:  SELECT x.b
>> CONTEXT:  PL/pgSQL function test(foo) line 1 at RETURN
>
> I believe that this was one of the cases I had in mind when I previously
> proposed that we stop using PLPGSQL_DTYPE_ROW entirely for composite-type
> variables, and make them use PLPGSQL_DTYPE_REC (that is, the same code
> paths used for RECORD).
>
> As I recall, that proposal was shot down with no investigation whatsoever,
> on the grounds that it might possibly make some cases slower.

I don't know whether that would help or not.  I was thinking about
whether PLs should be using CacheRegisterSyscacheCallback() to notice
when types that they care about change.

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


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

Re: [HACKERS] mogrify and indent features for jsonb

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Robert Haas wrote:
> > On Mon, Apr 27, 2015 at 6:41 PM, Andrew Dunstan  wrote:
> > > There's one exception I, at least, have to this rule, namely when there's 
> > > a
> > > corresponding compound if or else. I personally find this unaesthetic to 
> > > put
> > > it mildly:
> > >
> > >if (condition)
> > >statement;
> > >else
> > >{
> > >block of statements;
> > >}
> > 
> > Hmm, I don't dislike that style.  If somebody submitted a patch with
> > braces around the lone statement, I would remove them before
> > committing.
> 
> Same here.

Agreed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] mogrify and indent features for jsonb

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Apr 27, 2015 at 6:41 PM, Andrew Dunstan  wrote:
> > There's one exception I, at least, have to this rule, namely when there's a
> > corresponding compound if or else. I personally find this unaesthetic to put
> > it mildly:
> >
> >if (condition)
> >statement;
> >else
> >{
> >block of statements;
> >}
> 
> Hmm, I don't dislike that style.  If somebody submitted a patch with
> braces around the lone statement, I would remove them before
> committing.

Same here.

-- 
Á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] [RFC] sepgsql: prohibit users to relabel objects

[Adam Brightwell]

>
> Really?  Why?  I would think it's the policy's job to restrict relabel
> operations.
>

I agree.  This seems like an unnecessary change.

-Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Re: [HACKERS] [RFC] sepgsql: prohibit users to relabel objects

[Robert Haas]

On Wed, Apr 29, 2015 at 9:15 AM, Denis Kirjanov  wrote:
> Enforce access control on security labels defined by admin
> and prohibit users to relabel the objects

Really?  Why?  I would think it's the policy's job to restrict relabel
operations.

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


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

[HACKERS] [RFC] sepgsql: prohibit users to relabel objects

[Denis Kirjanov]

Enforce access control on security labels defined by admin
and prohibit users to relabel the objects

Signed-off-by: Denis Kirjanov 
---
 contrib/sepgsql/label.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index ef7661c..470b90e 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -504,6 +504,11 @@ sepgsql_object_relabel(const ObjectAddress *object, const 
char *seclabel)
(errcode(ERRCODE_INVALID_NAME),
   errmsg("SELinux: invalid security label: \"%s\"", 
seclabel)));
 
+   if (!superuser())
+   ereport(ERROR,
+   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("SELinux: must be superuser to relabel objects")));
+
/*
 * Do actual permission checks for each object classes
 */
-- 
1.7.10.4



-- 
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] Feedback on getting rid of VACUUM FULL

[Jeff Janes]

On Tue, Apr 28, 2015 at 11:32 AM, Robert Haas  wrote:

> On Fri, Apr 24, 2015 at 3:04 PM, Alvaro Herrera
>  wrote:
>
> > I think what we need here is something that does heap_update to tuples
> > at the end of the table, moving them to earlier pages; then wait for old
> > snapshots to die (the infrastructure for which we have now, thanks to
> > CREATE INDEX CONCURRENTLY); then truncate the empty pages.  Of course,
> > there are lots of details to resolve.  It doesn't really matter that
> > this runs for long: a process doing this for hours might be better than
> > AccessExclusiveLock on the table for a much shorter period.
>
> Why do you need to do anything other than update the tuples and let
> autovacuum clean up the mess?
>

It could take a long time before autovacuum kicked in and did so.  I think
a lot of time when people need this, the lack of space in the file system
is blocking some other action they want to do, so they want a definitive
answer as to when the deed is done rather than manually polling the file
system with "df".  You could invoke vacuum manually rather than waiting for
autovacuum, but it would kind of suck to do that only to find out you
didn't wait long enough for all the snapshots to go away and so no space
was actually released--and I don't think we have good ways of finding out
how long is long enough.  Ways of squeezing tables in the background would
be nice, but so would a way of doing it in the foreground and getting a
message when it is complete.

Cheers,

Jeff

Re: [HACKERS] mogrify and indent features for jsonb

[Robert Haas]

On Mon, Apr 27, 2015 at 6:41 PM, Andrew Dunstan  wrote:
> There's one exception I, at least, have to this rule, namely when there's a
> corresponding compound if or else. I personally find this unaesthetic to put
> it mildly:
>
>if (condition)
>statement;
>else
>{
>block of statements;
>}

Hmm, I don't dislike that style.  If somebody submitted a patch with
braces around the lone statement, I would remove them before
committing.



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


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

Re: [HACKERS] Reducing tuple overhead

[Robert Haas]

On Mon, Apr 27, 2015 at 5:01 PM, Jim Nasby  wrote:
> The problem with just having the value is that if *anything* changes between
> how you evaluated the value when you created the index tuple and when you
> evaluate it a second time you'll corrupt your index. This is actually an
> incredibly easy problem to have; witness how we allowed indexing
> timestamptz::date until very recently. That was clearly broken, but because
> we never attempted to re-run the index expression to do vacuuming at least
> we never corrupted the index itself.

True.  But I guess what I don't understand is: how big a deal is this,
really?  The "uncorrupted" index can still return wrong answers to
queries.  The fact that you won't end up with index entries pointing
to completely unrelated tuples is nice, but if index scans are missing
tuples that they should see, aren't you still pretty hosed?

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


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

Re: [HACKERS] forward vs backward slashes in msvc build code

[Robert Haas]

On Mon, Apr 27, 2015 at 10:59 AM, Andrew Dunstan  wrote:
> Yeah. I've pushed this with tiny fixes to avoid Leaning Toothpick Syndrome
> (man perlop for definition).

I had to Google that.  Then I had to think about it.  Then I laughed.

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


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

Re: [HACKERS] Missing importing option of postgres_fdw

[Robert Haas]

On Mon, Apr 27, 2015 at 7:47 AM, Michael Paquier
 wrote:
> Authorizing ALTER FOREIGN TABLE as query string that a FDW can use
> with IMPORT FOREIGN SCHEMA is a different feature than what is
> proposed in this patch, aka an option for postgres_fdw and meritates a
> discussion on its own because it impacts all the FDWs and not only
> postgres_fdw. Now, related to this patch, we could live without
> authorizing ALTER FOREIGN TABLE because CREATE FOREIGN TABLE does
> authorize the definition of CHECK constraints.

I agree.  I don't think there's a huge problem with allowing IMPORT
FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements, but it
doesn't really seem to be necessary.  I don't see why we can't just
declare the CHECK constraints in the CREATE FOREIGN TABLE statement
instead of adding more DDL.

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


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

Re: [HACKERS] Can pg_dump make use of CURRENT/SESSION_USER

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:

> I have this idea:
> 
> 1) Add an ObjectAddress field to CommentStmt struct an set it in gram.y
> 
> 2) In the CommentObject check if CommentStmt->address is
> InvalidObjectAddress then call get_object_address else use it

For DDL deparsing purposes, it seems important that the affected object
address can be reproduced somehow.  I think pg_get_object_address()
should be considered, as well as the object_address test.

If we do as you propose, we would have to deparse
COMMENT ON CURRENT DATABASE IS 'foo'
as
  COMMENT ON DATABASE whatever_the_name_is IS 'foo'

which is not a fatal objection but doesn't seem all that great.

-- 
Á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] Feedback on getting rid of VACUUM FULL

[Robert Haas]

On Tue, Apr 28, 2015 at 2:44 PM, Alvaro Herrera
 wrote:
>> > I think what we need here is something that does heap_update to tuples
>> > at the end of the table, moving them to earlier pages; then wait for old
>> > snapshots to die (the infrastructure for which we have now, thanks to
>> > CREATE INDEX CONCURRENTLY); then truncate the empty pages.  Of course,
>> > there are lots of details to resolve.  It doesn't really matter that
>> > this runs for long: a process doing this for hours might be better than
>> > AccessExclusiveLock on the table for a much shorter period.
>>
>> Why do you need to do anything other than update the tuples and let
>> autovacuum clean up the mess?
>
> Sure, that's one option.  I think autovac's current approach is too
> heavyweight: it always has to scan the whole relation and all the
> indexes.  It might be more convenient to do something more
> fine-grained; for instance, maybe instead of scanning the whole
> relation, start from the end of the relation walking backwards and stop
> once the first page containing a live or recently-dead tuple is found.
> Perhaps, while scanning the indexes you know that all CTIDs with pages
> higher than some threshold value are gone; you can remove them without
> scanning the heap at all perhaps.

I agree that scanning all of the indexes is awfully heavy-weight, but
I don't see how we're going to get around that.  The problem with
index vac is not that it's expensive to decide which CTIDs need to get
killed, but that we have to search for them in every page of the
index.  Unfortunately, I have no idea how to get around that.  The
only alternative approach is to regenerate the index tuples we expect
to find based on the heap tuples we're killing and search the index
for them one at a time.  Tom's been opposed to that in the past, but
maybe it's worth reconsidering.

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


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

Re: [HACKERS] Missing psql tab-completion for ALTER FOREIGN TABLE

[Robert Haas]

On Mon, Apr 27, 2015 at 2:50 AM, Etsuro Fujita
 wrote:
> Here is a patch to add missing tab-completion for ALTER FOREIGN TABLE.
> I'll add this to the next CF.

Committed, thanks.

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


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

Re: [HACKERS] Can pg_dump make use of CURRENT/SESSION_USER

[Fabrízio de Royes Mello]

On Wed, Apr 29, 2015 at 1:14 PM, Alvaro Herrera 
wrote:
>
> Fabrízio de Royes Mello wrote:
> >
> > Looking at the patch again I realize the code is very ugly, so I'll
rework
> > the patch.
>
> Yes, I think get_object_address should figure out what to do with the
> representation of CURRENT DATABASE directly, rather than having the
> COMMENT code morph from that into a list containing the name of the
> current database.  Please see about changing SECURITY LABEL at the same
> time, if only to make sure that the representation is sane.
>

I have this idea:

1) Add an ObjectAddress field to CommentStmt struct an set it in gram.y

2) In the CommentObject check if CommentStmt->address is
InvalidObjectAddress then call get_object_address else use it

Thoughts?

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] basebackups during ALTER DATABASE ... SET TABLESPACE ... not safe?

[Andres Freund]

On 2015-04-28 10:26:54 +0530, Abhijit Menon-Sen wrote:
> Hi Andres. Do you intend to commit these patches? (I just verified that
> they apply without trouble to HEAD.)

I intend to come back to them, yes. I'm not fully sure whether we should
really apply them to the back branches.

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] FIX : teach expression walker about RestrictInfo

[Tomas Vondra]

On 04/29/15 18:26, Tom Lane wrote:

Tomas Vondra  writes:

...

OK, let me explain the context a bit more. When working on the
multivariate statistics patch, I need to choose which stats to use for
estimating the clauses. I do that in clauselist_selectivity(), although
there might be other places where to do that.


Hm, well, clauselist_selectivity and clause_selectivity already contain
extensive special-casing for RestrictInfos, so I'm not sure that I see why
you're having difficulty working within that structure for this change.


So let's I'm in the clause_selectivity(), and have AND or OR clause to 
estimate. I need to get varnos/varattnos for the clause(s). What should 
I do? I can't call pull_varnos() or pull_varattnos() because there might 
be nested RestrictInfos, as demonstrated by the query I posted.



But there are basic reasons why expression_tree_walker should not try
to deal with RestrictInfos; the most obvious one being that it's not
clear whether it should descend into both the basic and OR-clause
subtrees. Also, if a node has expression_tree_walker support then it
should logically have expression_tree_mutator support as well, but
that's got multiple issues. RestrictInfos are not supposed to be
copied once created; and the mutator couldn't detect whether their
derived fields are still valid.


OK, I do understand that. So what about pull_varnos_walker and 
pull_varattnos_walker - what about teaching them about RestrictInfos?




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


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

Re: [HACKERS] FIX : teach expression walker about RestrictInfo

[Tom Lane]

Tomas Vondra  writes:
> On 04/29/15 05:55, Tom Lane wrote:
>> pull_varnos is not, and should not be, applied to a RestrictInfo; for one
>> thing, it'd be redundant with work that was already done when creating the
>> RestrictInfo (cf make_restrictinfo_internal).  You've not shown the
>> context of your problem very fully, but in general most code in the planner
>> should be well aware of whether it is working with a RestrictInfo (or list
>> of same) or a bare expression.  I don't believe that it's a very good idea
>> to obscure that distinction.

> OK, let me explain the context a bit more. When working on the 
> multivariate statistics patch, I need to choose which stats to use for 
> estimating the clauses. I do that in clauselist_selectivity(), although 
> there might be other places where to do that.

Hm, well, clauselist_selectivity and clause_selectivity already contain
extensive special-casing for RestrictInfos, so I'm not sure that I see why
you're having difficulty working within that structure for this change.

But there are basic reasons why expression_tree_walker should not try to
deal with RestrictInfos; the most obvious one being that it's not clear
whether it should descend into both the basic and OR-clause subtrees.
Also, if a node has expression_tree_walker support then it should
logically have expression_tree_mutator support as well, but that's
got multiple issues.  RestrictInfos are not supposed to be copied
once created; and the mutator couldn't detect whether their derived
fields are still valid.

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] Help needed for PL/Ruby

[Devrim Gündüz]

Hi,

Anyone? :)

Regards, Devrim

On Wed, 2015-03-18 at 15:19 +0200, Devrim Gündüz wrote:
> Hi,
> 
> Background info first: PL/Ruby was originally maintained by Guy Decoux,
> who passed away in 2008: https://www.ruby-forum.com/topic/166658 . After
> his death, Akinori MUSHA forked the project and maintained it until
> 2010: https://github.com/knu/postgresql-plruby . Last release was on Jan
> 2010, and recent distros started throwing build errors.
> 
> I was having similar build issues while trying to RPMify PL/Ruby, and
> finally stepped up the plate and tried to fix those build issues by
> forking the project:
> 
> https://github.com/devrimgunduz/postgresql-plruby/
> 
> I mainly applied patches from Fedora, and also did some basic cleanup.
> However, I don't know Ruby and have limited C skills, so I need some
> help on these:
> 
> * Complete the extension support: I committed initial infrastructure for
> it, but I don't think it is ready yet.
> 
> * Fix the FIXME:
> https://github.com/devrimgunduz/postgresql-plruby/blob/master/src/plpl.c#L844
> 
> * Documentation review and update: The recent example is against
> PostgreSQL 8.0. A recent Ruby example would be good, and also we need
> updates for creating the language.
> 
> Any contributions are welcome. I recently released 0.5.5 that at least
> is ready for testing.
> 
> I want to remind that I am not a Ruby guy, so this is really a community
> stuff for me.
> 
> Thanks by now.
> 
> Regards,


-- 
Devrim GÜNDÜZ
Principal Systems Engineer @ 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] Proposal: knowing detail of config files via SQL

[David Steele]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Looks good - ready for committer.

The new status of this patch is: Ready for Committer


-- 
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] Can pg_dump make use of CURRENT/SESSION_USER

[Alvaro Herrera]

Fabrízio de Royes Mello wrote:
> 
> Looking at the patch again I realize the code is very ugly, so I'll rework
> the patch.

Yes, I think get_object_address should figure out what to do with the
representation of CURRENT DATABASE directly, rather than having the
COMMENT code morph from that into a list containing the name of the
current database.  Please see about changing SECURITY LABEL at the same
time, if only to make sure that the representation is sane.

-- 
Á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] FIX : teach expression walker about RestrictInfo

[Tomas Vondra]

Hi,

On 04/29/15 05:55, Tom Lane wrote:

Tomas Vondra  writes:

On 04/28/15 21:50, Tom Lane wrote:

RestrictInfo is not a general expression node and support for it has
been deliberately omitted from expression_tree_walker().  So I think
what you are proposing is a bad idea and probably a band-aid for some
other bad idea.



That's not what I said, though. I said that calling pull_varnos() causes
the issue - apparently that does not happen in master, but I ran into
that when hacking on a patch.


pull_varnos is not, and should not be, applied to a RestrictInfo; for one
thing, it'd be redundant with work that was already done when creating the
RestrictInfo (cf make_restrictinfo_internal).  You've not shown the
context of your problem very fully, but in general most code in the planner
should be well aware of whether it is working with a RestrictInfo (or list
of same) or a bare expression.  I don't believe that it's a very good idea
to obscure that distinction.


OK, let me explain the context a bit more. When working on the 
multivariate statistics patch, I need to choose which stats to use for 
estimating the clauses. I do that in clauselist_selectivity(), although 
there might be other places where to do that.


Selecting the stats that best match the clauses is based on how well 
they cover the vars in the clauses (and some other criteria, that is 
mostly irrelevant here). So at some point I do call functions like 
pull_varnos() and pull_varattnos() to get this information.


I do handle RestrictInfo nodes explicitly - for those nodes I can do get 
the info from the node itself. But this does not work for the condition 
I posted, because that contains nested RestrictInfo nodes. So I do call 
pull_varnos() on a BoolExpr node, but in reality the node tree 
representing the clauses


WHERE (a >= 10 AND a <= 20) OR (b >= 20 AND b <= 40)

looks like this:

BoolExpr [OR_EXPR]
BoolExpr [AND_EXPR]
RestrictInfo
OpExpr [Var >= Const]
RestrictInfo
OpExpr [Var <= Const]
BoolExpr [AND_EXPR]
RestrictInfo
OpExpr [Var >= Const]
RestrictInfo
OpExpr [Var <= Const]

so the pull_varnos() fails because it runs into RestrictInfo node while 
walking the tree.


So I see three possibilities:

1) pull_varnos/pull_varattnos (and such functions, using the
   expression walker internally) are not supposed to be called unless
   you are sure there are no RestrictInfo nodes in the tree, but this
   seems really awkward

2) expression walker is supposed to know about RestrictInfo nodes (as I
   did in my patch), but you say this is not the case

3) pull_varnos_walker / pull_varattnos_walker are supposed to handle
   RestrictInfo nodes explicitly (either by using the cached information
   or by using RestrictInfo->clause in the next step)


regards

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


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

Re: [HACKERS] Selectivity estimation for intarray

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> For the specific cases you mention, perhaps it would be all right if we
>> taught plancache.c to blow away *all* cached plans upon seeing any change
>> in pg_operator; but that seems like a brute-force solution.

> Agreed that it is- but is that really a problem...?

Perhaps it isn't; we certainly have assumptions that pg_amop, for
instance, changes seldom enough that it's not worth tracking individual
changes.  The same might be true of pg_operator.  I'm not sure though.

The core point I'm trying to make is that making pg_operator entries
mutable is something that's going to require very careful review.

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] Selectivity estimation for intarray

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Alexander Korotkov  writes:
> > My proposal is to let ALTER OPERATOR change restrict and join selectivity
> > functions of the operator. Also it would be useful to be able to change
> > commutator and negator of operator: extension could add commutators and
> > negators in further versions. Any thoughts?
> 
> I'm pretty dubious about this, because we lack any mechanism for undoing
> parser/planner decisions based on operator properties.  And there's quite
> a lot of stuff that is based on the assumption that operator properties
> will never change.
> 
> An example of the pitfalls here is that we can never allow ALTER OPERATOR
> RENAME, because for example if you rename '<' to '~<~' that will change
> its precedence, and we have no way to fix the parse trees embedded in
> stored views to reflect that.
> 
> For the specific cases you mention, perhaps it would be all right if we
> taught plancache.c to blow away *all* cached plans upon seeing any change
> in pg_operator; but that seems like a brute-force solution.

Agreed that it is- but is that really a problem...?  I've not run into
many (any?) systems where pg_operator is getting changed often...  The
worst part would be adding new operators/extensions, but perhaps we
could exclude that specific case from triggering the cache invalidation?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Selectivity estimation for intarray

[Tom Lane]

Oleg Bartunov  writes:
> Any chance to have this patch in 9.5 ? Many intarray users will be happy.

Considering how desperately behind we are on reviewing/committing patches
that were submitted by the deadline, I don't think it would be appropriate
or fair to add on major new patches that came in months late.  Please add
this to the first 9.6 commitfest, instead.

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] Selectivity estimation for intarray

[Tom Lane]

Alexander Korotkov  writes:
> My proposal is to let ALTER OPERATOR change restrict and join selectivity
> functions of the operator. Also it would be useful to be able to change
> commutator and negator of operator: extension could add commutators and
> negators in further versions. Any thoughts?

I'm pretty dubious about this, because we lack any mechanism for undoing
parser/planner decisions based on operator properties.  And there's quite
a lot of stuff that is based on the assumption that operator properties
will never change.

An example of the pitfalls here is that we can never allow ALTER OPERATOR
RENAME, because for example if you rename '<' to '~<~' that will change
its precedence, and we have no way to fix the parse trees embedded in
stored views to reflect that.

For the specific cases you mention, perhaps it would be all right if we
taught plancache.c to blow away *all* cached plans upon seeing any change
in pg_operator; but that seems like a brute-force solution.

regards, tom lane


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

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Robert, all,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Stephen Frost (sfr...@snowman.net) wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> > > The tricky part of this seems to me to be the pg_dump changes.  The
> > > new catalog flag seems a little sketchy to me; wouldn't it be better
> > > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
> > > DUMP_NONE?
> > 
> > I agree that the pg_dump changes are a very big part of this change.
> > I'll look at using an enum and see if that would work.
> 
> I've implemented this approach and there are things which I like about
> it and things which I don't.  I'd love to hear your thoughts.  As
> mentioned previously, this patch does not break the pg_stat_activity or
> pg_stat_replication view APIs.  Similairly, the functions which directly
> feed those views return the same results they did previously, there are
> just additional functions now which provide everything, and view on top
> of those, for users who are GRANT'd access to them.

Here is the latest revision of this patch.

The big change here is the addition of default roles.  This approach has
been brought up a few times and Magnus recently mentioned it to me
again.  Having the default roles greatly reduced the impact of this
change on the test_deparse regression test, which was quite nice.

Updates are included for pg_upgrade and pg_dumpall to handle roles which
start with "pg_" specially as we are now claiming those as "System
defined" roles (similar to how we claim schemas starting with pg_ are
system defined, etc).  These new default roles are in line with the
previously discussed role attributes, but have the advantage that they
act just like normal roles and work inside of the normal permissions
system.  They are:

pg_backup - Start and stop backups, switch xlogs, create restore points.
pg_monitor - View privileged system info (user activity, replica lag)
pg_replay - Control XLOG replay on replica (pause, resume)
pg_replication - Create, destroy, work with replication slots

pg_admin - All of the above, plus rotate logfiles and signal backends

Feedback on all of this would be great.  One interesting idea is that,
with these defined default roles, we could rip out the majority of the
changes to pg_dump and declare that users have to use only the roles we
provide to manage access to those functions (or risk any changes they
make to the ACLs of system objects disappearing across upgrades or
pg_dump/restore's, which is what happens today anyway).  I'm a bit on
the fence about it myself; it'd certainly reduce the risk of this change
but it would also limit users to only being able to operate at the
pre-defined levels we've set, but then again, the same was going to be
true with the role attributes-based approach and I don't recall any
complaints during that discussion.

Thoughts?  Feedback on this would be most welcome; it's been a long time
incubating and I'd really like to get this capability in and close it
out of the current commitfest.  I'm certainly of the opinion that it
will be a welcome step forward for quite a few of our users as the
discussion about how to create non-superuser roles for certain
operations (a monitor role, in particular, but also backup and replay)
has come up quite a bit, both on the lists and directly from clients.

Thanks!

Stephen
From 488df9c567fdd0a56afa084a8f22f8b8a2412bd7 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 19 Mar 2015 14:49:26 -0400
Subject: [PATCH] Use GRANT for access to privileged functions

Historically, we have hard-coded the permissions for privileged
functions in C code (eg: if (superuser()) then allow X, else don't).
Unfortunately, that's pretty limiting and means that functions which are
useful for roles that should not be superusers (eg: monitoring, backup
management) require that the user calling them be a superuser.  This
leads to far more uses of superuser roles than is ideal.

Thankfully, we have a very handy and complex privilege system for
managing who has access to what already built into PG.  This is the
GRANT system which has existed since very near the beginning of PG.

This provides a set of system functions which are not able to be
executed by all users by default and allows administrators to grant
access to those functions for the users (eg: monitoring or other roles)
where they feel it is appropriate.

Further, create a set of pre-defined roles (which start with "pg_")
for administrators to use to grant bulk access with.  This greatly
simplifies the granting of "monitor", "backup", and similar privileges.
pg_upgrade and pg_dumpall are updated to treat roles starting with
"pg_" in much the same way as the default role is handled, and
pg_upgrade has been taught to complain if it finds any roles starting
with "pg_" in a 9.4 or older system.  Having pre-defined roles also
allows 3rd-party utilities (eg: check_postgres.pl) to depend on these
roles and the access they provide in t

Re: [HACKERS] pg_rewind test race condition..?

[Stephen Frost]

Heikki,

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> The problem seems to be that when the standby is promoted, it's a
> so-called "fast promotion", where it writes an end-of-recovery
> record and starts accepting queries before creating a real
> checkpoint. pg_rewind looks at the TLI in the latest checkpoint, as
> it's in the control file, but that isn't updated until the
> checkpoint completes. I don't see it on my laptop normally, but I
> can reproduce it if I insert a "sleep(5)" in StartupXLog, just
> before it requests the checkpoint:

Ah, interesting.

> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -7173,7 +7173,10 @@ StartupXLOG(void)
>* than is appropriate now that we're not in standby mode anymore.
>*/
>   if (fast_promoted)
> + {
> + sleep(5);
>   RequestCheckpoint(CHECKPOINT_FORCE);
> + }
>  }
> 
> The simplest fix would be to force a checkpoint in the regression
> test, before running pg_rewind. It's a bit of a cop out, since you'd
> still get the same issue when you tried to do the same thing in the
> real world. It should be rare in practice - you'd not normally run
> pg_rewind immediately after promoting the standby - but a better
> error message at least would be nice..

Forcing a checkpoint in the regression tests and then providing a better
error message sounds reasonable to me.  I agree that it's very unlikely
to happen in the real world, even when you're bouncing between systems
for upgrades, etc, you're unlikely to do it fast enough for this issue
to exhibit itself, and a better error message would help any users who
manage to run into this (perhaps during their own testing).

Another thought would be to provide an option to pg_rewind to have it do
an explicit checkpoint before it reads the control file..  I'm not
against having it simply always do it as I don't see pg_rewind being a
commonly run thing, but I know some environments have very heavy
checkpoints and that might not be ideal.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Selectivity estimation for intarray

[Oleg Bartunov]

Any chance to have this patch in 9.5 ? Many intarray users will be happy.

On Wed, Apr 29, 2015 at 1:48 PM, Alexander Korotkov 
wrote:

> Hackers,
>
> currently built-in &&, @>, <@ array operators have selectivity estimations
> while same operator in intarray contrib haven't them. Problem is that
> operators in intarray contrib still use contsel and contjoinsel functions
> for selectivity estimation even when built-in operators receive their
> specific selectivity estimations.
>
> Attached patch adds selectivity estimation functions to  &&, @>, <@
> operators in intarray contrib. In order to have less code duplication they
> are just wrappers over built-in selectivity estimation functions.
>
> However, I faced a problem of migration scripts. Currently, ALTER OPERATOR
> can only change owner and schema of operator not operator parameters
> themselves. Change pg_operator directly is also not an option. At first, it
> would be kludge. At second, in order to correctly find corresponding
> operator in pg_operator migration script need to know schema where
> extension is installed. But it can't refer @extschema@ because extension
> is relocatable.
>
> My proposal is to let ALTER OPERATOR change restrict and join selectivity
> functions of the operator. Also it would be useful to be able to change
> commutator and negator of operator: extension could add commutators and
> negators in further versions. Any thoughts?
>
> --
> With best regards,
> Alexander Korotkov.
>
>
> --
> 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] shared_libperl, shared_libpython

[Michael Paquier]

On Wed, Apr 29, 2015 at 12:48 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 4/28/15 12:05 AM, Tom Lane wrote:
>>> Yeah.  Even more specifically, olinguito does have --with-python in its
>>> configure flags, but then the plpython Makefile skips the build because
>>> libpython isn't available as a shared library.  But the contrib test is
>>> (I assume, haven't looked) conditional only on the configure flag.
>>>
>>> I'm not real sure now why we felt that was a good approach.  The general
>>> project policy is that if you ask for a feature in the configure flags,
>>> we'll build it or die trying; how come this specific Python issue gets
>>> special treatment contrary to that policy?
>
>> The reason for the current setup is actually that when plperl and later
>> plpython was added, we still had Perl and Python client modules in our
>> tree (Pg.pm and pygresql), and configure --with-perl and --with-python
>> were meant to activate their build primarily.  Also, in those days,
>> having a shared libperl or libpython was rare.  But we didn't want to
>> fail the frontend interface builds because of that.  So we arrived at
>> the current workaround.
>
> Ah.  I'm glad you remember, because I didn't.

Interesting, those are pieces of history:
commit: f10a9033bf308f9dde0aa77caad6503e233489d1
author: Peter Eisentraut 
date: Mon, 1 Sep 2003 23:01:49 +
Clean up after pygresql removal: adjust/remove documentation and remove
unneeded configure work.

commit: 9a0b4d7f847469544798133391e221481548e1b9
author: Marc G. Fournier 
date: Fri, 30 Aug 2002 13:06:22 +

perl5 interface moved to gborg

>> My preference would be to rip all that out and let the compiler or
>> linker decide when it doesn't want to link something.
>
> Works for me, assuming that we get an understandable failure message and
> not, say, a plperl.so that mysteriously doesn't work.

+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

[HACKERS] Selectivity estimation for intarray

[Alexander Korotkov]

Hackers,

currently built-in &&, @>, <@ array operators have selectivity estimations
while same operator in intarray contrib haven't them. Problem is that
operators in intarray contrib still use contsel and contjoinsel functions
for selectivity estimation even when built-in operators receive their
specific selectivity estimations.

Attached patch adds selectivity estimation functions to  &&, @>, <@
operators in intarray contrib. In order to have less code duplication they
are just wrappers over built-in selectivity estimation functions.

However, I faced a problem of migration scripts. Currently, ALTER OPERATOR
can only change owner and schema of operator not operator parameters
themselves. Change pg_operator directly is also not an option. At first, it
would be kludge. At second, in order to correctly find corresponding
operator in pg_operator migration script need to know schema where
extension is installed. But it can't refer @extschema@ because extension is
relocatable.

My proposal is to let ALTER OPERATOR change restrict and join selectivity
functions of the operator. Also it would be useful to be able to change
commutator and negator of operator: extension could add commutators and
negators in further versions. Any thoughts?

--
With best regards,
Alexander Korotkov.


intarray-sel-1.patch
Description: Binary data

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

Re: [HACKERS] Proposal: knowing detail of config files via SQL

[Sawada Masahiko]

On Wed, Apr 29, 2015 at 12:20 AM, David Steele  wrote:
> On 4/27/15 10:37 PM, Sawada Masahiko wrote:
>>
>> Thank you for your reviewing.
>> Attached v8 patch is latest version.
>
> I posted a review through the CF app but it only went to the list
> instead of recipients of the latest message.  install-checkworld is
> failing but the fix is pretty trivial.
>
> See:
> http://www.postgresql.org/message-id/20150428145626.2632.75287.p...@coridan.postgresql.org
>

Thank you for reviewing!
I have fixed regarding regression test.
The latest patch is attached.

Regards,

---
Sawada Masahiko


000_pg_file_settings_view_v9.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] Faster setup_param_list() in plpgsql

[Pavel Stehule]

Hi all

I am looking on this patch. I can confirm 10-15% speedup - and the idea
behind this patch looks well.

This patch
http://www.postgresql.org/message-id/4146.1425872...@sss.pgh.pa.us contains
two parts

a) relative large refactoring

b) support for resettable fields in param_list instead total reset

I believe it should be in two separate patches. Refactoring is trivial and
there is no any possible objection.

Regards

Pavel