[HACKERS] Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
Hi all,
Here is a continuation of the thread which covered $subject for MinGW
and Cygwin:
http://www.postgresql.org/message-id/51f19059.7050...@pgexperts.com
But this time it is for MSVC.
Just a bit of background... Since commit cb4a3b04 we are installing
shared libraries in bin/ and lib/ for Cygwin and MinGW on Windows, but
we are still missing MSVC, so as of now build method is inconsistent.
Attached is a patch aimed at fixing this inconsistency. What it does
is simple: when kicking Install.pm to install the deliverables of each
project, we check if the project Makefile contains SO_MAJOR_VERSION.
If yes, libraries of this project are installed in bin/ and lib/. The
path to the Makefile is found by scanning ResourceCompile in the
vcproj file, this method having the advantage to make the patch
independent of build process.
This also removes a wart present in Install.pm installing copying
manually libpq.dll:
- lcopy($target . '/lib/libpq.dll', $target . '/bin/libpq.dll');
I am adding an entry in the upcoming CF for this patch, and let's use
this thread for this discussion.
Regards,
--
Michael
From 13ca64cb5cdae419d6ee1bbf7c7a04f6bd3d2388 Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Tue, 23 Dec 2014 21:58:50 -0800
Subject: [PATCH 2/2] Install shared libraries in bin/ and lib/ with MSVC
This is the MSVC part of the previous commit, to ensure that install is
completely consistent with the multiple methods supported.
---
src/tools/msvc/Install.pm | 43 +++
1 file changed, 35 insertions(+), 8 deletions(-)
diff --git a/src/tools/msvc/Install.pm b/src/tools/msvc/Install.pm
index eba9aa0..616cd9d 100644
--- a/src/tools/msvc/Install.pm
+++ b/src/tools/msvc/Install.pm
@@ -91,7 +91,6 @@ sub Install
}
CopySolutionOutput($conf, $target);
- lcopy($target . '/lib/libpq.dll', $target . '/bin/libpq.dll');
my $sample_files = [];
my @top_dir = (src);
@top_dir = (src\\bin, src\\interfaces) if ($insttype eq client);
@@ -236,8 +235,9 @@ sub CopySolutionOutput
while ($sln =~ $rem)
{
my $pf = $1;
- my $dir;
+ my @dirs;
my $ext;
+ my $is_sharedlib = 0;
$sln =~ s/$rem//;
@@ -247,17 +247,37 @@ sub CopySolutionOutput
my $proj = read_file($pf.$vcproj)
|| croak Could not open $pf.$vcproj\n;
+
+ # Check this project uses a shared library by looking if
+ # SO_MAJOR_VERSION is defined in its Makefile, whose path
+ # can be found using the resource file of this project.
+ if ($proj =~ qr{ResourceCompile\s*Include=([^]+)})
+ {
+ my $projpath = dirname($1);
+ my $mf = read_file($projpath . '/Makefile')
+|| croak Could not open $projpath/Makefile\n;
+
+ if ($mf =~ /^SO_MAJOR_VERSION\s*=\s*(.*)$/mg)
+ {
+$is_sharedlib = 1;
+ }
+ }
+
if ($vcproj eq 'vcproj' $proj =~ qr{ConfigurationType=([^]+)})
{
if ($1 == 1)
{
-$dir = bin;
+@dirs = qw(bin);
$ext = exe;
}
elsif ($1 == 2)
{
-$dir = lib;
+@dirs = qw(lib);
$ext = dll;
+if ($is_sharedlib)
+{
+ push(@dirs, 'bin');
+}
}
else
{
@@ -271,13 +291,17 @@ sub CopySolutionOutput
{
if ($1 eq 'Application')
{
-$dir = bin;
+@dirs = qw(bin);
$ext = exe;
}
elsif ($1 eq 'DynamicLibrary')
{
-$dir = lib;
+@dirs = qw(lib);
$ext = dll;
+if ($is_sharedlib)
+{
+ push(@dirs, 'bin');
+}
}
else# 'StaticLibrary'
{
@@ -290,8 +314,11 @@ sub CopySolutionOutput
{
croak Could not parse $pf.$vcproj\n;
}
- lcopy($conf\\$pf\\$pf.$ext, $target\\$dir\\$pf.$ext)
- || croak Could not copy $pf.$ext\n;
+ foreach my $dir (@dirs)
+ {
+ lcopy($conf\\$pf\\$pf.$ext, $target\\$dir\\$pf.$ext)
+|| croak Could not copy $pf.$ext\n;
+ }
lcopy($conf\\$pf\\$pf.pdb, $target\\symbols\\$pf.pdb)
|| croak Could not copy $pf.pdb\n;
print .;
--
2.2.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
On Tue, Jan 20, 2015 at 11:29 AM, Peter Geoghegan p...@heroku.com wrote: On Mon, Jan 19, 2015 at 5:59 PM, Peter Geoghegan p...@heroku.com wrote: On Mon, Jan 19, 2015 at 5:33 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: You did notice that bowerbird isn't building, right? http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbirddt=2015-01-19%2023%3A54%3A46 Yeah. Looks like strxfrm_l() isn't available on the animal, for whatever reason. I think that the attached patch should at least fix that much. Maybe the problem on the other animal is also explained by the lack of this, since there could also be a MinGW-ish strxfrm_l(), I suppose. On MinGW-32, not that I know of: $ find . -name *.h | xgrep strxfrm_l ./lib/gcc/mingw32/4.8.1/include/c++/mingw32/bits/c++config.h:/* Define if strxfr m_l is available in string.h. */ ./mingw32/lib/gcc/mingw32/4.8.1/include/c++/mingw32/bits/c++config.h:/* Define i f strxfrm_l is available in string.h. */ strxfrm is defined in string.h though. With your patch applied, the failure with MSVC disappeared, but there is still a warning showing up: (ClCompile target) - src\backend\lib\hyperloglog.c(73): warning C4334: '' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
Hi, On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote: The new site has been deployed and should now be usable. I think this unfortunately lost the RSS feature? I found that quite useful to see who changed what recently (it's forwared to an imap mailbox for me...). What I'm also missing from the old app is that previously 'reviews' could explicitly be linked from the app. Now there's a list of emails in the thread, nice!, but in bigger threads that really doesn't help to find the actual review. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
On Tue, Jan 20, 2015 at 6:54 AM, Magnus Hagander mag...@hagander.net wrote: On Mon, Jan 19, 2015 at 10:10 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 19, 2015 at 4:05 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander mag...@hagander.net wrote: The new site has been deployed and should now be usable. There are, for some reason, three copies of Clarify need for memory barriers in bgworkers in the in-progress CF. I don't know why that happened, or how to fix it. There are also two copies of ctidscan as an example of custom-scan. Yeah, Michael pointed out he was unable to fix that one. I think I've fixed the permissions errors required for that, so I'll wait for him to try again to confirm that I managed to fix it. Thanks for fixing the permission problem. I have removed the duplicated entries, something actually caused by me when adding the existing patches to the new app. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
On Tue, Jan 20, 2015 at 10:09 AM, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote: The new site has been deployed and should now be usable. I think this unfortunately lost the RSS feature? I found that quite useful to see who changed what recently (it's forwared to an imap mailbox for me...). Yep, added here: https://commitfest.postgresql.org/3/109/ I linked it with 20140922230255.gd2...@awork2.anarazel.de. I also recall that you and Robert were marked as reviewers, right? -- 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] Reducing buildfarm disk usage: remove temp installs when done
On 1/19/15 1:07 PM, Andres Freund wrote: On 2015-01-18 17:48:11 -0500, Tom Lane wrote: One of the biggest causes of buildfarm run failures is out of disk space. That's not just because people are running buildfarm critters on small slow machines; it's because make check-world is an enormous space hog. Some numbers from current HEAD: clean source tree: 120MB built source tree: 400MB tree after make check-world:3GB (This is excluding ~250MB for one's git repo.) The reason for all the bloat is the temporary install trees that we create, which tend to eat up about 100MB apiece, and there are dozens of them (eg, one per testable contrib module). Those don't get removed until the end of the test run, so the usage is cumulative. The attached proposed patch removes each temp install tree as soon as we're done with it, in the normal case where no error was detected. This brings the peak space usage down from ~3GB to ~750MB. I was wondering before if we couldn't always do the the temp installation into $top_builddir/tmp_install or something like it. With an additional small ugly hacking ontop we could even avoid reinstalling for every target in check-world. FWIW, if anyone's going to do some serious tinkering in here; it'd be really nice to create a separate utility for managing temporary installs. That would make it trivial for PGXN modules to use something other than pg_regress for their test framework. -- 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] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code
On Tue, Jan 20, 2015 at 1:56 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-19 17:16:11 +0900, Michael Paquier wrote: On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund and...@2ndquadrant.com wrote: Not this patch's fault, but I'm getting a bit tired seeing the above open coded. How about adding a function that does the sleeping based on a timestamptz and a ms interval? You mean in plugins, right? I don't recall seeing similar patterns in other code paths of backend. But I think that we can use something like that in timestamp.c then because we need to leverage that between two timestamps, the last failure and now(): TimestampSleepDifference(start_time, stop_time, internal_ms); Perhaps you have something else in mind? Attached is an updated patch. Actually I came with better than last patch by using a boolean flag as return value of TimestampSleepDifference and use TimestampDifferenceExceeds directly inside it. Subject: [PATCH] Add wal_availability_check_interval to control WAL fetching on failure I think that name isn't a very good. And its isn't very accurate either. How about wal_retrieve_retry_interval? Not very nice, but I think it's still better than the above. Fine for me. + varlistentry id=wal-availability-check-interval xreflabel=wal_availability_check_interval + termvarnamewal_availability_check_interval/varname (typeinteger/type) + indexterm +primaryvarnamewal_availability_check_interval/ recovery parameter/primary + /indexterm + /term + listitem + para +This parameter specifies the amount of time to wait when +WAL is not available for a node in recovery. Default value is +literal5s/. + /para + para +A node in recovery will wait for this amount of time if +varnamerestore_command/ returns nonzero exit status code when +fetching new WAL segment files from archive or when a WAL receiver +is not able to fetch a WAL record when using streaming replication. + /para + /listitem + /varlistentry + /variablelist Walreceiver doesn't wait that amount, but rather how long the connection is intact. And restore_command may or may not retry. I changed this paragraph as follows: +This parameter specifies the amount of time to wait when a failure +occurred when reading WAL from a source (be it via streaming +replication, local filenamepg_xlog/ or WAL archive) for a node +in standby mode, or when WAL is expected from a source. Default +value is literal5s/. Pondering more about this patch, I am getting the feeling that it is not that much a win to explain in details in the doc each failure depending on the source from which WAL is taken. But it is essential to mention that the wait is done only for a node using standby mode. Btw, I noticed two extra things that I think should be done for consistency: - We should use as well wal_retrieve_retry_interval when waiting for WAL to arrive after checking for the failures: /* -* Wait for more WAL to arrive. Time out after 5 seconds, -* like when polling the archive, to react to a trigger -* file promptly. +* Wait for more WAL to arrive. Time out after +* wal_retrieve_retry_interval ms, like when polling the archive +* to react to a trigger file promptly. */ WaitLatch(XLogCtl-recoveryWakeupLatch, WL_LATCH_SET | WL_TIMEOUT, - 5000L); + wal_retrieve_retry_interval * 1L); - Updated patch attached. -- Michael From e07949030a676b033f4a563cfaac4687bcc37dca Mon Sep 17 00:00:00 2001 From: Michael Paquier michael@otacoo.com Date: Mon, 19 Jan 2015 16:08:48 +0900 Subject: [PATCH] Add wal_retrieve_retry_interval to control WAL retrieval at recovery This parameter allows to control the amount of time process will wait for WAL from a source when a failure occurred for a standby node, or for the amount of time to wait when WAL is expected from a source. Default value is 5s. --- doc/src/sgml/recovery-config.sgml | 17 + src/backend/access/transam/recovery.conf.sample | 9 + src/backend/access/transam/xlog.c | 50 + src/backend/utils/adt/timestamp.c | 24 src/include/utils/timestamp.h | 2 + 5 files changed, 87 insertions(+), 15 deletions(-) diff --git
[HACKERS] New CF app: changing email sender
Hello, the new app's clean looking gets my favor and the integrated operation will do good for me:) By the way, I got the following notice when I tried to Add comment on the new app. Note!: This form will generate an email to the public mailinglist pgsql-hackers, with sender set to horiguchi.kyot...@oss.ntt.co.jp! Hmm. The mail address indeed *was* mine but is now obsolete, so that the email might bounce. But I haven't find how to change it within the app itself, and the PostgreSQL community account page. https://www.postgresql.org/account/ Could you tell me how do I change the sender address? regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Mon, Jan 19, 2015 at 12:05:01PM -0500, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Jan 19, 2015 at 11:30 AM, Andres Freund and...@2ndquadrant.com wrote: Sure, but the log isn't invisible. As mentioned one paragraph above, I don't think it's likely to ever be noticed in the client code in the cases where it happens in production. Yes, that is my feeling as well. Meh. I still don't like it, but I guess I'm outvoted. Unless there are further votes, what we have at this point is just: - elog(WARNING, pgstat wait timeout); + ereport(LOG, (errmsg(using stale statistics instead of current ones because stats collector is not responding))); with no conditionality for pgstat_vacuum_stat vs. other callers. That is satisfactory to me, too. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
Peter Geoghegan wrote: It appears that the buildfarm animal brolga isn't happy about this patch. I'm not sure why, since I thought we already figured out bugs or other inconsistencies in various strxfrm() implementations. You did notice that bowerbird isn't building, right? http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbirddt=2015-01-19%2023%3A54%3A46 -- Á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] WITH CHECK and Column-Level Privileges
All, * Stephen Frost (sfr...@snowman.net) wrote: It seems that the reason for this is to be consistent with BuildIndexValueDescription, which seems to do the same thing to simplify the stuff going on at check_exclusion_constraint() -- by returning an empty string, that code doesn't need to check which of the returned values is empty, only whether both are. That seems an odd choice to me; it seems better to me to be specific, i.e. include each of the %s depending on whether the returned string is null or not. You would have three possible different errdetails, but that seems a good thing anyway. Right, ExecBuildSlotValueDescription() was made to be consistent with BuildIndexValueDescription. The reason for BuildIndexValueDescription returning an empty string is different from why you hypothosize though- it's exported and I was a bit worried that existing external callers might not be prepared for it to return a NULL instead of a string of some kind. If this was a green field instead of a back-patch fix, I'd certainly return NULL instead. If others feel that's not a good reason to avoid returning NULL, I can certainly change it around. Does anyone else want to weigh in on this? It's no guarantee, but checking codesearch.debian.net, the only packages which include BuildIndexValueDescription are PG core and derivatives. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Merging postgresql.conf and postgresql.auto.conf
On Tue, Jan 20, 2015 at 9:09 AM, Amit Kapila amit.kapil...@gmail.com wrote: Right, but we can't completely eliminate such a possibility (as an example we have some default settings like max_connections, shared_buffers, etc). I agree with you that users should use only way Sorry for incomplete sentence, I mean to say only one way of changing the settings, however for certain rare cases (default settings or some other) we can provide a way for user to know which of the settings are duplicate. I think if we can provide such an information via some view with ease then it's worth to have it. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Merging postgresql.conf and postgresql.auto.conf
On Mon, Jan 19, 2015 at 9:35 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Jan 17, 2015 at 12:19 AM, Amit Kapila amit.kapil...@gmail.com wrote: So are you telling that whenever we read, save the settings to some catalog (probably a new one)? Which process are you imagining would do this? Certainly not the postmaster. I think whichever process reads postgresql.conf/postgresql.auto.conf have to do this (unless we restrict that this will be done at some other time) and postmaster is one of them. It seems to me that it is not good idea unless we do it at other time like populating entries for a view. Independently of that, it sounds like solving the problem from the wrong end. I think the idea of ALTER SYSTEM .. SET ought to be that you should EITHER edit postgresql.conf for all your configuration changes, OR you should use ALTER SYSTEM .. SET for all of your changes. If you mix-and-match, well, that's certainly allowed and it will work and everything, but you - as a human being - might get confused. Right, but we can't completely eliminate such a possibility (as an example we have some default settings like max_connections, shared_buffers, etc). I agree with you that users should use only way of changing the settings, however for certain rare cases (default settings or some other) we can provide a way for user to know which of the settings are duplicate. I think if we can provide such an information via some view with ease then it's worth to have it. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
On Mon, Jan 19, 2015 at 5:43 PM, Peter Geoghegan p...@heroku.com wrote: It appears that the buildfarm animal brolga isn't happy about this patch. I'm not sure why, since I thought we already figured out bugs or other inconsistencies in various strxfrm() implementations. Well, the first thing that comes to mind is that strxfrm() is returning strings that, when sorted, do not give the same order we would have obtained via strcoll(). It's true that there are existing callers of strxfrm(), but it looks like that is mostly used for statistics-gathering, so it's possible that differences vs. strcoll() would not have shown up before now. Is there any legitimate way that strxfrm() and strcoll() can return inconsistent answers - e.g. they are somehow allowed to derive their notion of the relevant locale differently - or is this just a case of Cygwin being busted? -- 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] B-Tree support function number 3 (strxfrm() optimization)
On Mon, Jan 19, 2015 at 7:47 PM, Michael Paquier michael.paqu...@gmail.com wrote: On MinGW-32, not that I know of: $ find . -name *.h | xgrep strxfrm_l ./lib/gcc/mingw32/4.8.1/include/c++/mingw32/bits/c++config.h:/* Define if strxfr m_l is available in string.h. */ ./mingw32/lib/gcc/mingw32/4.8.1/include/c++/mingw32/bits/c++config.h:/* Define i f strxfrm_l is available in string.h. */ strxfrm is defined in string.h though. I'm not quite following. Doesn't that imply that strxfrm_l() at least *could* be available? I guess it doesn't matter, though, because the animal with the successful build that fails the locale regression test (brolga) does not have locale_t support. Therefore, there is no new strxfrm_l() caller. My next guess is that the real problem is an assumption I've made. That is, my assumption that strxfrm() always behaves equivalently to strcpy() when the C locale happens to be in use may not be portable (due to external factors). I guess we're inconsistent about making sure that LC_COLLATE is set correctly in WIN32 and/or EXEC_BACKEND builds, or something along those lines. The implementation in the past got away with strcoll()/strxfrm() not having the C locale set, since strcoll() was never called when the C locale was in use -- we just called strcmp() instead. Assuming that's correct, it might be easier just to entirely disable the optimization on Windows, even with the C locale. It may not be worth it to even bother just for C locale support of abbreviated keys. I'm curious about what will happen there when the _strxfrm_l() fix patch is applied. With your patch applied, the failure with MSVC disappeared, but there is still a warning showing up: (ClCompile target) - src\backend\lib\hyperloglog.c(73): warning C4334: '' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended? That seems harmless. I suggest an explicit cast to Size here. -- 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: Partitioning: issues/ideas (Was: Re: [HACKERS] On partitioning)
On 17-01-2015 AM 02:34, Robert Haas wrote: On Wed, Jan 14, 2015 at 9:07 PM, Amit Langote langote_amit...@lab.ntt.co.jp wrote: * It has been repeatedly pointed out that we may want to decouple partitioning from inheritance because implementing partitioning as an extension of inheritance mechanism means that we have to keep all the existing semantics which might limit what we want to do with the special case of it which is partitioning; in other words, we would find ourselves in difficult position where we have to inject a special case code into a very generalized mechanism that is inheritance. Specifically, do we regard a partitions as pg_inherits children of its partitioning parent? I don't think this is totally an all-or-nothing decision. I think everyone is agreed that we need to not break things that work today -- e.g. Merge Append. What that implies for pg_inherits isn't altogether clear. One point is that an implementation may end up establishing the parent-partition hierarchy somewhere other than (or in addition to) pg_inherits. One intention would be to avoid tying partitioning scheme to certain inheritance features that use pg_inherits. For example, consider call sites of find_all_inheritors(). One notable example is Append/MergeAppend which would be of interest to partitioning. We would want to reuse that part of the infrastructure but we could might as well write an equivalent, say find_all_partitions() which scans something other than pg_inherits to get all partitions. Now, we may not want to do that and instead add special case code to prevent partitioning from fiddling with unnecessary inheritance features in the code paths of inheritance. This seems like an important decision to make. * Syntax: do we want to make it similar to one of the many other databases out there? Or we could invent our own? Well, what I think we don't want is something that is *almost* like some other database but not quite. I lean toward inventing our own since I'm not aware of something that we'd want to copy exactly. I wonder if we could add a clause like DISTRIBUTED BY to complement PARTITION ON that represents a hash distributed/partitioned table (that could be a syntax to support sharded tables maybe; we would definitely want to move ahead in that direction I guess) Maybe eventually, but let's not complicate things by worrying too much about that now. Agree that we may not want to mix the two too much at this point. * Catalog: We would like to have a catalog structure suitable to implement capabilities like multi-column partitioning, sub-partitioning (with arbitrary number of levels in the hierarchy). I had suggested that we create two new catalogs viz. pg_partitioned_rel, pg_partition_def to store metadata about a partition key of a partitioned relation and partition bound info of a partition, respectively. Also, see the point about on-disk representation of partition bounds I'm not convinced that there is any benefit in spreading this information across two tables neither of which exist today. If the representation of the partitioning scheme is going to be a node tree, then there's no point in taking what would otherwise have been a List and storing each element of it in a separate tuple. The overarching point here is that the system catalog structure should be whatever is most convenient for the system internals; I'm not sure we understand what that is yet. Agree that some concrete idea of internal representation should help guide the catalog structure. If we are going to cache the partitioning info in relcache (which we most definitely will), then we should try to make sure to consider the scenario of having a lot of partitioned tables with a lot of individual partitions. It looks like it would be similar to a scenarios where there are a lot of inheritance hierarchies. But, availability of partitioning feature would definitely cause these numbers to grow larger. Perhaps this is an important point driving this discussion. I guess this remains tied to the decision we would like make regarding inheritance (pg_inherits, etc.) * It is desirable to treat partitions as pg_class relations with perhaps a new relkind(s). We may want to choose an implementation where only the lowest level relations in a partitioning hierarchy have storage; those at the upper layers are mere placeholder relations though of course with associated constraints determined by partitioning criteria (with appropriate metadata entered into the additional catalogs). I think the storage-less parents need a new relkind precisely to denote that they have no storage; I am not convinced that there's any reason to change the relkind for the leaf nodes. But that's been proposed, so evidently someone thinks there's a reason to do it. Again, this remains partly tied to decisions we make regarding catalog structure. I am not sure but wouldn't we ever need to tell from a
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
On Mon, Jan 19, 2015 at 5:59 PM, Peter Geoghegan p...@heroku.com wrote: On Mon, Jan 19, 2015 at 5:33 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: You did notice that bowerbird isn't building, right? http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbirddt=2015-01-19%2023%3A54%3A46 Yeah. Looks like strxfrm_l() isn't available on the animal, for whatever reason. I think that the attached patch should at least fix that much. Maybe the problem on the other animal is also explained by the lack of this, since there could also be a MinGW-ish strxfrm_l(), I suppose. -- Peter Geoghegan diff --git a/src/include/port/win32.h b/src/include/port/win32.h index 550c3ec..4cb51ec 100644 --- a/src/include/port/win32.h +++ b/src/include/port/win32.h @@ -341,6 +341,7 @@ typedef int pid_t; #define isspace_l _isspace_l #define iswspace_l _iswspace_l #define strcoll_l _strcoll_l +#define strxfrm_l _strxfrm_l #define wcscoll_l _wcscoll_l #define wcstombs_l _wcstombs_l #define mbstowcs_l _mbstowcs_l -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inaccuracy in VACUUM's tuple count estimates
Was slightly optimistic that this comment in the release notes might mean that my bug with bloat on hot tables might have been fixed in 9.4 /Make VACUUM properly report dead but not-yet-removable rows to the statistics collector (Hari Babu) Previously these were reported as live rows./ Unfortunately that is not the case, and we still have the problem in 9.4 -- View this message in context: http://postgresql.nabble.com/Inaccuracy-in-VACUUM-s-tuple-count-estimates-tp5806367p5834687.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fillfactor for GIN indexes
On Tue, Jan 20, 2015 at 12:06 AM, Cédric Villemain ced...@2ndquadrant.com wrote: Michael, I first didn't understood how GiN can benefits from the patch...thus my question. There were no noise for me, and I learn some more about GiN. So I thank you for your work! Kicking back the ball, I haven't done as much work as I should have. Alexander, I think that we should continue on this patch in the next CF. Are you fine if an entry is added with you as author. I'll drop myself as author (not that I cannot find the time, just that I am sort of useless). -- 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] Error check always bypassed in tablefunc.c
On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway m...@joeconway.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 01/19/2015 08:16 AM, Alvaro Herrera wrote: Haven't looked at this patch, but I wonder if it would be better to replace the innards of connectby with a rewrite of the query to use standard WITH queries. Maybe we can remove a couple hundred lines from tablefunc.c? Seems like a good idea -- connectby is really obsolete for quite a while now other than as an SRF example. I guess we only keep it around for backwards compatibility? For master, yes we could brush up things a bit. Now do we really do the same for back-branches? I would think that the answer there is something close to the patch I sent. -- 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] PATCH: decreasing memory needlessly consumed by array_agg
On Wed, Jan 14, 2015 at 1:52 PM, Tomas Vondra tomas.von...@2ndquadrant.com wrote: Attached is v8 patch, with a few comments added: 1) before initArrayResult() - explaining when it's better to use a single memory context, and when it's more efficient to use a separate memory context for each array build state 2) before makeArrayResult() - explaining that it won't free memory when allocated in a single memory context (and that a pfree() has to be used if necessary) 3) before makeMdArrayResult() - explaining that it's illegal to use release=true unless using a subcontext I understand there is history to this API, and we need to be compatible, but the end result is awkward. I'm wondering whether it would be better to just have a new set of functions like accumArrayResultElem, etc., and only allow astate=NULL in the original accumArrayResult(). That cure might be worse than the disease though. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
On Mon, Jan 19, 2015 at 5:33 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: You did notice that bowerbird isn't building, right? http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbirddt=2015-01-19%2023%3A54%3A46 Yeah. Looks like strxfrm_l() isn't available on the animal, for whatever reason. -- 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] Async execution of postgres_fdw.
I think its telling that varying the fetch size doubled the performance, even on localhost. If you were to repeat this test across a network, the performance difference would be far more drastic. I understand the desire to keep the fetch size small by default, but I think your results demonstrate how important the value is. At the very least, it is worth reconsidering this arbitrary value. However, I think the real solution is to make this configurable. It probably should be a new option on the foreign server or table, but an argument could be made for it to be global across the server just like work_mem. Obviously, this shouldn't block your current patch but its worth revisiting. - Matt Kelly
Re: [HACKERS] INSERT ... ON CONFLICT UPDATE and RLS
On 10 January 2015 at 15:12, Stephen Frost sfr...@snowman.net wrote:
* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
Currently we're applying RLS CHECKs after the INSERT or UPDATE, like
WITH CHECK OPTIONs on views. The SQL spec says that WITH CHECK OPTIONs
on views have to be applied after the INSERT/UPDATE on the base
relation, but we're free to do something different for RLS CHECKs if
that makes more sense. If we want RLS to be more like column-level
privilege checking, then it does make sense to do the checks sooner,
so perhaps we should be checking the RLS policies before the
INSERT/UPDATE, like CHECK constraints.
Were you thinking about working up a patch for such a change? If not,
I'll see about finding time to do it, unless someone else wants to
volunteer. :)
Attached is a patch to make RLS checks run before attempting to
insert/update any data rather than afterwards.
In the end I decided not to create a new structure for RLS checks
because most of the code that handles them treats them the same as
WCOs. Instead, I just added a new 'kind' enum field to the existing
structure and renamed/reworded things a bit.
The patch also changes the error message for a RLS check violation, to
make the cause of the error clearer. One thing I'm not sure about is
what sqlstate code to use for this error, but I don't think that using
WITH_CHECK_OPTION_VIOLATION is appropriate, because that seems to be
specifically intended for views.
Regards,
Dean
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
new file mode 100644
index 5b70cc9..6762b63
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*** ExecConstraints(ResultRelInfo *resultRel
*** 1638,1646
/*
* ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs
*/
void
! ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
ExprContext *econtext;
--- 1638,1647
/*
* ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs
+ * of the specified kind.
*/
void
! ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
ExprContext *econtext;
*** ExecWithCheckOptions(ResultRelInfo *resu
*** 1663,1668
--- 1664,1672
WithCheckOption *wco = (WithCheckOption *) lfirst(l1);
ExprState *wcoExpr = (ExprState *) lfirst(l2);
+ if (wco-kind != kind)
+ continue;
+
/*
* WITH CHECK OPTION checks are intended to ensure that the new tuple
* is visible (in the case of a view) or that it passes the
*** ExecWithCheckOptions(ResultRelInfo *resu
*** 1673,1686
* case (the opposite of what we do above for CHECK constraints).
*/
if (!ExecQual((List *) wcoExpr, econtext, false))
! ereport(ERROR,
! (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
! errmsg(new row violates WITH CHECK OPTION for \%s\,
! wco-viewname),
! errdetail(Failing row contains %s.,
! ExecBuildSlotValueDescription(slot,
! RelationGetDescr(resultRelInfo-ri_RelationDesc),
! 64;
}
}
--- 1677,1711
* case (the opposite of what we do above for CHECK constraints).
*/
if (!ExecQual((List *) wcoExpr, econtext, false))
! {
! switch (wco-kind)
! {
! case WCO_VIEW_CHECK:
! ereport(ERROR,
! (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
! errmsg(new row violates WITH CHECK OPTION for \%s\,
! wco-relname),
! errdetail(Failing row contains %s.,
! ExecBuildSlotValueDescription(slot,
! RelationGetDescr(resultRelInfo-ri_RelationDesc),
! 64;
! break;
! case WCO_RLS_INSERT_CHECK:
! case WCO_RLS_UPDATE_CHECK:
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg(new row violates row level security policy for \%s\,
! wco-relname),
! errdetail(Failing row contains %s.,
! ExecBuildSlotValueDescription(slot,
! RelationGetDescr(resultRelInfo-ri_RelationDesc),
! 64;
! break;
! default:
! elog(ERROR, unrecognized WCO kind: %u, wco-kind);
! break;
! }
! }
}
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
new file mode 100644
index f96fb24..be879a4
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
*** ExecInsert(TupleTableSlot *slot,
*** 253,258
--- 253,265
tuple-t_tableOid = RelationGetRelid(resultRelationDesc);
/*
+ * Check any RLS INSERT WITH CHECK policies
+ */
+ if (resultRelInfo-ri_WithCheckOptions != NIL)
+ ExecWithCheckOptions(WCO_RLS_INSERT_CHECK,
+ resultRelInfo, slot, estate);
+
+ /*
* Check the constraints of the tuple
*/
if
Re: [HACKERS] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code
On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund and...@2ndquadrant.com wrote:
Not this patch's fault, but I'm getting a bit tired seeing the above open
coded. How about adding a function that does the sleeping based on a
timestamptz and a ms interval?
You mean in plugins, right? I don't recall seeing similar patterns in
other code paths of backend. But I think that we can use something
like that in timestamp.c then because we need to leverage that between
two timestamps, the last failure and now():
TimestampSleepDifference(start_time, stop_time, internal_ms);
Perhaps you have something else in mind?
Attached is an updated patch.
Actually I came with better than last patch by using a boolean flag as
return value of TimestampSleepDifference and use
TimestampDifferenceExceeds directly inside it.
Regards,
--
Michael
From d77429197838c0ad9d378aba08137d4ca0b384fd Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 19 Jan 2015 16:08:48 +0900
Subject: [PATCH] Add wal_availability_check_interval to control WAL fetching
on failure
Default value is 5s.
---
doc/src/sgml/recovery-config.sgml | 21 +
src/backend/access/transam/recovery.conf.sample | 10 +++
src/backend/access/transam/xlog.c | 39 ++---
src/backend/utils/adt/timestamp.c | 24 +++
src/include/utils/timestamp.h | 2 ++
5 files changed, 86 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml
index 0c64ff2..7fd51ce 100644
--- a/doc/src/sgml/recovery-config.sgml
+++ b/doc/src/sgml/recovery-config.sgml
@@ -145,6 +145,27 @@ restore_command = 'copy C:\\server\\archivedir\\%f %p' # Windows
/listitem
/varlistentry
+ varlistentry id=wal-availability-check-interval xreflabel=wal_availability_check_interval
+ termvarnamewal_availability_check_interval/varname (typeinteger/type)
+ indexterm
+primaryvarnamewal_availability_check_interval/ recovery parameter/primary
+ /indexterm
+ /term
+ listitem
+ para
+This parameter specifies the amount of time to wait when
+WAL is not available for a node in recovery. Default value is
+literal5s/.
+ /para
+ para
+A node in recovery will wait for this amount of time if
+varnamerestore_command/ returns nonzero exit status code when
+fetching new WAL segment files from archive or when a WAL receiver
+is not able to fetch a WAL record when using streaming replication.
+ /para
+ /listitem
+ /varlistentry
+
/variablelist
/sect1
diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample
index b777400..70d3946 100644
--- a/src/backend/access/transam/recovery.conf.sample
+++ b/src/backend/access/transam/recovery.conf.sample
@@ -58,6 +58,16 @@
#
#recovery_end_command = ''
#
+#
+# wal_availability_check_interval
+#
+# specifies an optional interval to check for availability of WAL when
+# recovering a node. This interval of time represents the frequency of
+# retries if a previous command of restore_command returned nonzero exit
+# status code or if a walreceiver did not stream completely a WAL record.
+#
+#wal_availability_check_interval = '5s'
+#
#---
# RECOVERY TARGET PARAMETERS
#---
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 629a457..39b4e1c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -235,6 +235,7 @@ static TimestampTz recoveryTargetTime;
static char *recoveryTargetName;
static int recovery_min_apply_delay = 0;
static TimestampTz recoveryDelayUntilTime;
+static int wal_availability_check_interval = 5000;
/* options taken from recovery.conf for XLOG streaming */
static bool StandbyModeRequested = false;
@@ -4881,6 +4882,26 @@ readRecoveryCommandFile(void)
(errmsg_internal(trigger_file = '%s',
TriggerFile)));
}
+ else if (strcmp(item-name, wal_availability_check_interval) == 0)
+ {
+ const char *hintmsg;
+
+ if (!parse_int(item-value, wal_availability_check_interval, GUC_UNIT_MS,
+ hintmsg))
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(parameter \%s\ requires a temporal value,
+wal_availability_check_interval),
+ hintmsg ? errhint(%s, _(hintmsg)) : 0));
+ ereport(DEBUG2,
+ (errmsg_internal(wal_availability_check_interval = '%s', item-value)));
+
+ if (wal_availability_check_interval = 0)
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(\%s\ must have a value strictly positive,
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/01/16 1:24, Alvaro Herrera wrote:
Etsuro Fujita wrote:
*** 817,826 InitPlan(QueryDesc *queryDesc, int eflags)
--- 818,833
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
+ relkind = rt_fetch(rc-rti,
rangeTable)-relkind;
+ if (relkind == RELKIND_FOREIGN_TABLE)
+ relid = getrelid(rc-rti, rangeTable);
+ else
+ relid = InvalidOid;
relation = NULL;
break;
--- 2326,2342
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! /* if relid is valid, rel is a foreign table; set
system columns */
! if (OidIsValid(erm-relid))
! {
! tuple.t_self = td-t_ctid;
! tuple.t_tableOid = erm-relid;
! }
! else
! {
! ItemPointerSetInvalid((tuple.t_self));
! tuple.t_tableOid = InvalidOid;
! }
tuple.t_data = td;
I find this arrangement confusing and unnecessary -- surely if you have
access to the ExecRowMark here, you could just obtain the relid with
RelationGetRelid instead of saving the OID beforehand?
I don't think so because we don't have the Relation (ie, erm-relation =
NULL) here since InitPlan don't open/lock relations having markType =
ROW_MARK_COPY as shown above, which include foreign tables selected for
update/share. But I noticed we should open/lock such foreign tables as
well in InitPlan because I think ExecOpenScanRelation is assuming that,
IIUC.
And if you have
the Relation, you could just consult the relkind at that point instead
of relying on the relid being set or not as a flag to indicate whether
the rel is foreign.
Followed your revision. Attached is an updated version of the patch.
Thanks for the comment!
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 2947,2953 make_tuple_from_result_row(PGresult *res,
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple-t_self = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
--- 2947,2953
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple-t_self = tuple-t_data-t_ctid = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 795,800 InitPlan(QueryDesc *queryDesc, int eflags)
--- 795,801
{
PlanRowMark *rc = (PlanRowMark *) lfirst(l);
Oid relid;
+ char relkind;
Relation relation;
ExecRowMark *erm;
***
*** 817,823 InitPlan(QueryDesc *queryDesc, int eflags)
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
! relation = NULL;
break;
default:
elog(ERROR, unrecognized markType: %d, rc-markType);
--- 818,831
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
! relkind = rt_fetch(rc-rti, rangeTable)-relkind;
! if (relkind == RELKIND_FOREIGN_TABLE)
! {
! relid = getrelid(rc-rti, rangeTable);
! relation = heap_open(relid, AccessShareLock);
! }
! else
! relation = NULL;
break;
default:
elog(ERROR, unrecognized markType: %d, rc-markType);
***
*** 1115,1125 CheckValidRowMarkRel(Relation rel, RowMarkType markType)
RelationGetRelationName(rel;
break;
case RELKIND_FOREIGN_TABLE:
! /* Should not get here; planner should have used ROW_MARK_COPY */
! ereport(ERROR,
! (errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg(cannot lock rows in foreign table \%s\,
! RelationGetRelationName(rel;
break;
default:
ereport(ERROR,
--- 1123,1134
RelationGetRelationName(rel;
break;
case RELKIND_FOREIGN_TABLE:
! /* Allow opening/locking a foreign table only if ROW_MARK_COPY */
! if (markType != ROW_MARK_COPY)
! ereport(ERROR,
! (errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg(cannot lock rows in foreign table \%s\,
! RelationGetRelationName(rel;
break;
default:
ereport(ERROR,
***
*** 1792,1798 ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist)
aerm-rowmark = erm;
/* Look up the resjunk columns associated with this rowmark */
! if (erm-relation)
{
Assert(erm-markType != ROW_MARK_COPY);
--- 1801,1808
[HACKERS] Another comment typo in src/backend/executor/execMain.c
Hi, I ran into another typo in execMain.c. Patch attached. Best regards, Etsuro Fujita diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index fcc42fa..bc910f0 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2182,7 +2182,7 @@ EvalPlanQualInit(EPQState *epqstate, EState *estate, /* * EvalPlanQualSetPlan -- set or change subplan of an EPQState. * - * We need this so that ModifyTuple can deal with multiple subplans. + * We need this so that ModifyTable can deal with multiple subplans. */ void EvalPlanQualSetPlan(EPQState *epqstate, Plan *subplan, List *auxrowmarks) -- Sent 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: disallow operator = and use it for named parameters
2015-01-19 4:54 GMT+01:00 Robert Haas robertmh...@gmail.com: On Sat, Jan 17, 2015 at 8:27 PM, Pavel Stehule pavel.steh...@gmail.com wrote: two years a operator = is marked as deprecated (from PostgreSQL 9.2). Isn't time to use it for named parameters now (for PostgreSQL 9.5) ? I'm cool with that. It's possible that there are installations out there that still have = operators installed, but every still-supported release warns you not to do that, and the hstore change exists in three released versions. Anyway, no amount of waiting will eliminate the hazard completely. I am sending a implementation where syntax based on = symbol is second (but preferred) variant of := syntax .. syntax := will be supported still. Here is a patch I think you should just remove the WARNING, not change it to an error. If somebody wants to quote the operator name to be able to continue using it, I think that's OK. It looks so quoting doesn't help here + CREATE OPERATOR = ( +leftarg = int8,-- right unary +procedure = numeric_fac + ); + ERROR: syntax error at or near ( + LINE 1: CREATE OPERATOR = ( + ^ Regards Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] New CF app deployment
On Tue, Jan 20, 2015 at 4:31 PM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-20 11:05:43 +0900, Michael Paquier wrote: On Tue, Jan 20, 2015 at 10:09 AM, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote: The new site has been deployed and should now be usable. I think this unfortunately lost the RSS feature? I found that quite useful to see who changed what recently (it's forwared to an imap mailbox for me...). Yep, added here: https://commitfest.postgresql.org/3/109/ I linked it with 20140922230255.gd2...@awork2.anarazel.de. I also recall that you and Robert were marked as reviewers, right? I think you misunderstood me ;). I was talking about the old CF application providing a RSS feed of all changes to all entries. I.e. https://commitfest-old.postgresql.org/action/commitfest_activity.rss Oh, I didn't know this one. That's indeed useful. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
On 2015-01-20 11:05:43 +0900, Michael Paquier wrote: On Tue, Jan 20, 2015 at 10:09 AM, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote: The new site has been deployed and should now be usable. I think this unfortunately lost the RSS feature? I found that quite useful to see who changed what recently (it's forwared to an imap mailbox for me...). Yep, added here: https://commitfest.postgresql.org/3/109/ I linked it with 20140922230255.gd2...@awork2.anarazel.de. I also recall that you and Robert were marked as reviewers, right? I think you misunderstood me ;). I was talking about the old CF application providing a RSS feed of all changes to all entries. I.e. https://commitfest-old.postgresql.org/action/commitfest_activity.rss Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Partitioning: issues/ideas (Was: Re: [HACKERS] On partitioning)
On 20-01-2015 AM 10:48, Amit Langote wrote: On 17-01-2015 AM 02:34, Robert Haas wrote: On Wed, Jan 14, 2015 at 9:07 PM, Amit Langote langote_amit...@lab.ntt.co.jp wrote: * It is desirable to treat partitions as pg_class relations with perhaps a new relkind(s). We may want to choose an implementation where only the lowest level relations in a partitioning hierarchy have storage; those at the upper layers are mere placeholder relations though of course with associated constraints determined by partitioning criteria (with appropriate metadata entered into the additional catalogs). I think the storage-less parents need a new relkind precisely to denote that they have no storage; I am not convinced that there's any reason to change the relkind for the leaf nodes. But that's been proposed, so evidently someone thinks there's a reason to do it. Again, this remains partly tied to decisions we make regarding catalog structure. I am not sure but wouldn't we ever need to tell from a pg_class entry that a leaf relation has partition bounds associated with it? One reason I can see that we may not need it is that we would rather use relispartitioned of a non-leaf relation to trigger finding all its partitions and their associated bounds; we don't need to know (or reserve a field for) that a relation has partition bounds associated with it. The bounds can be stored in pg_partition indexed by relid. Maybe relkind is not the right field for this anyway. With that said, would we be comfortable with putting partition key into pg_class (maybe as a pg_node_tree also encapsulating opclass) so that if relispartitioned, also look for relpartkey? This paints a picture that our leaf relations would be plain old relations. They are almost similar in all respects (how they are planned, modified, maintained, ...). They just have an additional property that the values they can contain are restricted by, say, pg_partition.values; but it doesn't concern how they are planned. Planning related changes are confined to upper layers of the hierarchy instead. Kinda like saying instead of doing relation_excluded_by_constraints(childrel), we'd instead say prune_useless_partitions(partitionedrel) possibly at some other site than its counterpart. Guess that illustrates the point. I am not sure again if we want to limit access to individual partitions unless via some special syntax, then what that means for the above. We have been discussing that. Such access limiting could (only) be facilitated by a new relkind. On the other hand, the non-leaf relations are slightly new kind of relations in that they do not have storage (they could have a tablespace which would be the default tablespace for its underlying partitions). Obviously they do not have indexes pointing at them. Because they are further partitioned, they are differently planned - most probably Append with partition-pruning (almost like Append with constraint-exclusion but supposedly quicker because of the explicit access to partition definitions and perhaps execution-time). INSERT/COPY on these involve routing tuple to the appropriate leaf relation. Not surprisingly, this is almost similar to the picture that Alvaro had presented modulo some differences. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
On Tue, Jan 20, 2015 at 8:47 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway m...@joeconway.com wrote:
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 01/19/2015 08:16 AM, Alvaro Herrera wrote:
Haven't looked at this patch, but I wonder if it would be better
to replace the innards of connectby with a rewrite of the query to
use standard WITH queries. Maybe we can remove a couple hundred
lines from tablefunc.c?
Seems like a good idea -- connectby is really obsolete for quite a
while now other than as an SRF example. I guess we only keep it around
for backwards compatibility?
For master, yes we could brush up things a bit. Now do we really do
the same for back-branches? I would think that the answer there is
something close to the patch I sent.
So, using a WITH RECURSIVE, here is a query equivalent to what connectby does:
=# SELECT * FROM connectby_text;
keyid | parent_keyid | pos
---+--+-
row2 | row1 | 0
row3 | row1 | 0
row4 | row2 | 1
row5 | row2 | 0
row6 | row4 | 0
row7 | row3 | 0
row8 | row6 | 0
row9 | row5 | 0
row1 | null | 0
(9 rows)
=# SELECT * FROM
connectby('connectby_text', 'keyid', 'parent_keyid', 'row1', 3, '~') AS
t(keyid text, parent_keyid text, level int, branch text);
keyid | parent_keyid | level | branch
---+--+---+-
row1 | null | 0 | row1
row2 | row1 | 1 | row1~row2
row4 | row2 | 2 | row1~row2~row4
row6 | row4 | 3 | row1~row2~row4~row6
row5 | row2 | 2 | row1~row2~row5
row9 | row5 | 3 | row1~row2~row5~row9
row3 | row1 | 1 | row1~row3
row7 | row3 | 2 | row1~row3~row7
(8 rows)
=# WITH RECURSIVE connectby_tree AS
(
SELECT keyid, 0::int AS level, parent_keyid, keyid as
ct_full_list -- root portion
FROM connectby_text
WHERE keyid = 'row1' -- start point
UNION ALL
SELECT ctext.keyid,
(ctree.level + 1)::int AS level,
ctext.parent_keyid,
CAST(ctree.ct_full_list || '~' || ctext.keyid AS text) AS ct_full_list
FROM connectby_text AS ctext
INNER JOIN connectby_tree AS ctree
ON (ctext.parent_keyid = ctree.keyid) -- connect by
WHERE ctree.level = 2 -- limit of level
)
SELECT keyid, parent_keyid, level, ct_full_list
FROM connectby_tree ORDER BY ct_full_list;
keyid | parent_keyid | level |ct_full_list
---+--+---+-
row1 | null | 0 | row1
row2 | row1 | 1 | row1~row2
row4 | row2 | 2 | row1~row2~row4
row6 | row4 | 3 | row1~row2~row4~row6
row5 | row2 | 2 | row1~row2~row5
row9 | row5 | 3 | row1~row2~row5~row9
row3 | row1 | 1 | row1~row3
row7 | row3 | 2 | row1~row3~row7
(8 rows)
Using that we got a couple of options:
- Parametrize this query in some set of plpgsql functions and dump
tablefunc to 1.1
- Integrate directly this query in the existing C code and use SPI,
without dumping tablefunc.
Thoughts?
--
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] pgaudit - an auditing extension for PostgreSQL
At 2015-01-19 08:26:59 -0500, sfr...@snowman.net wrote: I'm confused by this statement.. Let me see if I've understood your clarification: Suppose you have pgaudit.roles set to 'r0, r1', and that you have granted r0 to u0. Now, if you're connected to the database as r0 or r1, then everything you do is logged. But if you're connected as u0, then only those things are logged that can be derived (in a manner discussed later) from the permissions explicitly granted to r0 (but not u0)? So when I'm trying to decide what to audit, I have to: (a) check if the current user is mentioned in .roles; if so, audit. (b) check if the current user is a descendant of one of the roles mentioned in .roles; if not, no audit. (c) check for permissions granted to the root role and see if that tells us to audit. Is that right? If so, it would work fine. I don't look forward to trying to explain it to people, but yes, it would work (for anything you could grant permissions for). You can't say that you want r1 to have X actions logged, with r2 having Y actions logged. Right. But how do you do that for non-GRANT-able actions in your scheme? For example, what if I want to see all the tables created and dropped by a particular user? Have you considered splitting pgaudit.c into multiple files, perhaps along the pre/post deparse lines? If such a split were done properly, then could the backwards-compatible version be more acceptable for inclusion in contrib in 9.5? If so, I'll look into it. One thought might be to provide the intersection between LOGSTMT and ObjectTypes. Hm. OK. The key part above is any. We're using the grant system as a proxy for saying I want to audit column X. That's different from I want to audit commands which would be allowed if I *only* had access to column X. If I want to audit access to column X, then: select A, B, X from mytable; Should be audited, even if the audit role doesn't have access to columns A or B. Yes, that's what the code already does (modulo handling of the audit role's oid, which I tweaked to match the behaviour described earlier in this mail). I did the following: create table x(a int,b int,c int); insert into x(a,b,c) values (1,2,3); create user y; grant select on x to y; create role x; grant select (a) on table x to x; grant x to y; Then, as user y, I did «select a,b,c from x» and «select b,c from x». Only the former was logged: LOG: AUDIT,2015-01-20 11:19:02.156441+05:30,postgres,y,y,READ,SELECT,TABLE,public.x,select a,b,c from x; Yeah- but are we always going to have to deal with a partial event trigger set? As a practical matter, yes. I don't know if there are current plans to extend event trigger support to the commands and object types that are yet unsupported. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WITH CHECK and Column-Level Privileges
On Mon, Jan 19, 2015 at 11:05:22AM -0500, Stephen Frost wrote: One remaining question is about single-column key violations. Should we special-case those and allow them to be shown or no? I can't see a reason not to currently but I wonder if we might have cause to act differently in the future (not that I can think of a reason we'd ever need to). Keep them hidden. Attempting to delete a PK row should not reveal otherwise-inaccessible values involved in any constraint violation. It's tempting to make an exception for single-column UNIQUE constraints, but some of the ensuing data disclosures are questionable. What if the violation arose from a column default expression or from index corruption? On Mon, Jan 19, 2015 at 11:46:35AM -0500, Stephen Frost wrote: Right, ExecBuildSlotValueDescription() was made to be consistent with BuildIndexValueDescription. The reason for BuildIndexValueDescription returning an empty string is different from why you hypothosize though- it's exported and I was a bit worried that existing external callers might not be prepared for it to return a NULL instead of a string of some kind. If this was a green field instead of a back-patch fix, I'd certainly return NULL instead. If others feel that's not a good reason to avoid returning NULL, I can certainly change it around. I won't lose sleep if it does return for that reason, but I'm relatively unconcerned about extension API compatibility here. That function is called from datatype-independent index uniqueness checks. This mailing list has discussed the difficulties of implementing an index access method in an extension, and no such extension has come forward. Your latest patch has whitespace errors; visit git diff --check. -- Sent 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: decreasing memory needlessly consumed by array_agg
On Sun, Dec 28, 2014 at 11:53 PM, Jeff Davis pg...@j-davis.com wrote: On Tue, 2014-04-01 at 13:08 -0400, Tom Lane wrote: I think a patch that stood a chance of getting committed would need to detect whether the aggregate was being called in simple or grouped contexts, and apply different behaviors in the two cases. The simple context doesn't seem like a big problem even if we change things as Tomas suggests: IMNSHO these are the issues we really should fix - by lowering the initial element count (64-4) and using a single memory context. In the simple context, there's only one context regardless, so the only cost I see is from reducing the initial allocation from 64 to some lower number. But if we're doubling each time, it won't take long to get there; and because it's the simple context, we only need to do it once. Tom (tgl), Is my reasoning above acceptable? The current patch, which I am evaluating for commit, does away with per-group memory contexts (it uses a common context for all groups), and reduces the initial array allocation from 64 to 8 (but preserves doubling behavior). Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
On Mon, Jan 19, 2015 at 10:10 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 19, 2015 at 4:05 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander mag...@hagander.net wrote: The new site has been deployed and should now be usable. There are, for some reason, three copies of Clarify need for memory barriers in bgworkers in the in-progress CF. I don't know why that happened, or how to fix it. There are also two copies of ctidscan as an example of custom-scan. Yeah, Michael pointed out he was unable to fix that one. I think I've fixed the permissions errors required for that, so I'll wait for him to try again to confirm that I managed to fix it. Also, I can't figure out what I'm supposed to do with the Author and Reviewer checkboxes. Checking and unchecking them doesn't seem to do anything. That's part of the mass email to reviewers/authors that's available only to cf managers (which you are flagged as in the system). There's a button to actually send the mail at the bottom (pick selected as receipient). -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] A minor typo in brin.c
On Thu, Jan 15, 2015 at 8:23 PM, Amit Langote langote_amit...@lab.ntt.co.jp wrote: [ patch to fix a typo ] 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] Bug in pg_dump
On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold gilles.dar...@dalibo.com wrote: I attach a patch that solves the issue in pg_dump, let me know if it might be included in Commit Fest or if the three other solutions are a better choice. I think a fix in pg_dump is appropriate, so I'd encourage you to add this to the next CommitFest. -- 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, Thanks for the feedback. I'm slightly mystified as to how including the word online helps here. It's unlikely that there will be an offline_backup permission, because if the system is off-line, SQL-level permissions are irrelevant. I'm certainly open to recommendations on this one. Initially, BACKUP was proposed, but based on the discussion, it is unacceptable. As mentioned, the documentation for the affected functions refer to starting/stopping an 'on-line backup', hence the current proposal. I feel like it is obviously more in line with the documentation and removes the ambiguity in what 'type' of backup it allows, as that seemed to be one of the major concerns of just using BACKUP. However, I could certainly understand if there was a confusion on the terminology of 'online' vs 'offline' if those are not regularly used terms or concepts. At any rate, I'll certainly continue to give this one thought, but I wouldn't mind any recommendations/suggestions anyone was willing to throw my way. * LOG - allows role to rotate log files - remains broad enough to consider future log related operations Maybe LOGFILE? Only because some confusion with the LOG message level seems possible; or confusion about whether this is a permission that lets you log things. That's a good point. I'll change this one up. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Additional role attributes superuser review
* Robert Haas (robertmh...@gmail.com) wrote: On Thu, Jan 15, 2015 at 6:03 PM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: * ONLINE_BACKUP - allows role to perform backup operations - originally proposed as BACKUP - due to concern for the use of that term in relation to other potential backup related permissions this form is in line with the documentation as it describes the affected backup operations as being 'online backups'. - applies only to the originally proposed backup functions. I'm slightly mystified as to how including the word online helps here. It's unlikely that there will be an offline_backup permission, because if the system is off-line, SQL-level permissions are irrelevant. ONLINE does match up with what we call the pg_start/stop_backup based backups in the documentation, at least. Also, it's intended to contrast against pg_dump-based backups, not offline backups (which we don't discuss at all in the docs that I can see). Looking over the docs again a bit though, what about BACKUP_CONTROL, following the title of 9.26.3? Suggestions certainly welcome. * LOG - allows role to rotate log files - remains broad enough to consider future log related operations Maybe LOGFILE? Only because some confusion with the LOG message level seems possible; or confusion about whether this is a permission that lets you log things. LOGFILE works for me. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Mon, Jan 19, 2015 at 7:09 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-18 21:33:25 -0500, Robert Haas wrote: On Sun, Jan 18, 2015 at 4:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: After looking at the code, the minimum-change alternative would be more or less as attached: first, get rid of the long-obsolete proposition that autovacuum workers need fresher-than-usual stats; second, allow pgstat_vacuum_stat to accept stats that are moderately stale (the number given below allows them to be up to 50 seconds old); and third, suppress wait-timeout warnings when the call is from pgstat_vacuum_stat. The third point is what we need to avoid unnecessary buildfarm failures. The second point addresses the idea that we don't need to stress the stats collector too much for this. I think this is too much of a good thing. I don't see any reason why autovacuum's statistics need to be fresher than normal, but I also don't see any reason why they need to be less fresh. I think suppressing the warning is a good idea, but why only suppress it for autovacuum? How about just knocking the level down to, say, DEBUG1? +1 for just using LOG - which by default does not end up on client machines. In contrast to WARNING. Yeah, that doesn't seem like a bad idea, either. The message seems much more likely to be of interest to the DBA than the user; the DBA can use the message to diagnose an overloaded I/O subsystem (which I think is the usual cause of this problem) whereas the only point of likely interest to the user is that their query ran slowly (which they know without the message). -- 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] Fillfactor for GIN indexes
On Sat, Jan 17, 2015 at 4:49 AM, Alexander Korotkov aekorot...@gmail.com wrote: I already wrote quite detailed explanation of subject. Let mel try to explain in shortly. GIN is two level nested btree. Thus, GIN would have absolutely same benefits from fillfactor as btree. Lack of tests showing it is, for sure, fault. However, GIN posting trees are ordered by ItemPointer and this makes some specific. If you have freshly created table and do inserts/updates they would use the end of heap. Thus, inserts would go to the end of GIN posting tree and fillfactor wouldn't affect anything. Fillfactor would give benefits on HOT or heap space re-usage. Ah, OK. Those tests clarify things considerably; I see the point now. -- 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: disallow operator = and use it for named parameters
Pavel Stehule wrote: It looks so quoting doesn't help here + CREATE OPERATOR = ( +leftarg = int8,-- right unary +procedure = numeric_fac + ); + ERROR: syntax error at or near ( + LINE 1: CREATE OPERATOR = ( + ^ Does it work to use OPERATOR(=) syntax? I don't think identifier quoting works for operators. -- Á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] Parallel Seq Scan
On Mon, Jan 19, 2015 at 2:24 AM, Amit Kapila amit.kapil...@gmail.com wrote: Okay, I think I got the idea what you want to achieve via prefetching. So assuming prefetch_distance = 100 and prefetch_increment = 50 (prefetch_distance /2), it seems to me that as soon as there are less than 100 blocks in prefetch quota, it will fetch next 50 blocks which means the system will be always approximately 50 blocks ahead, that will ensure that in this algorithm it will always perform sequential scan, however eventually this is turning to be a system where one worker is reading from disk and then other workers are reading from OS buffers to shared buffers and then getting the tuple. In this approach only one downside I can see and that is there could be times during execution where some/all workers will have to wait on the worker doing prefetching, however I think we should try this approach and see how it works. Right. We probably want to make prefetch_distance a GUC. After all, we currently rely on the operating system for prefetching, and the operating system has a setting for this, at least on Linux (blockdev --getra). It's possible, however, that we don't need this at all, because the OS might be smart enough to figure it out for us. It's probably worth testing, though. Another thing is that I think prefetching is not supported on all platforms (Windows) and for such systems as per above algorithm we need to rely on block-by-block method. Well, I think we should try to set up a test to see if this is hurting us. First, do a sequential-scan of a related too big at least twice as large as RAM. Then, do a parallel sequential scan of the same relation with 2 workers. Repeat these in alternation several times. If the operating system is accomplishing meaningful readahead, and the parallel sequential scan is breaking it, then since the test is I/O-bound I would expect to see the parallel scan actually being slower than the normal way. Or perhaps there is some other test that would be better (ideas welcome) but the point is we may need something like this, but we should try to figure out whether we need it before spending too much time on it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
Abhijit, * Abhijit Menon-Sen (a...@2ndquadrant.com) wrote: At 2015-01-18 22:28:37 -0500, sfr...@snowman.net wrote: 2. Set pgaudit.roles = 'r1, r2, …', which logs everything done by r1, r2, and any of their descendants. If these roles were the 'audit' roles as considered under #1 then couldn't administrators control what pgaudit.roles provide today using role memberships granted to the roles listed? Yes. But then there would be no convenient way to say just log everything this particular role does. I'm confused by this statement.. Having the role listed in pgaudit.roles would still mean that everything that role does is logged. Adding other roles to be audited would simply be 'GRANT audit TO role1;'. Is there a specific action which you don't think would be audited through this? 3. Set pgaudit.log = 'read, write, …', which logs any events in any of the listed classes. Approach #1 addresses this too, doesn't it? Approach #1 applies only to things you can grant permissions for. What if you want to audit other commands? You can grant roles to other roles, which is how the role-level auditing would be handled. Hopefully that clarifies the idea here. As currently implemented, role-level auditing is required to have DDL be logged, but you may very well want to log all DDL and no DML, for example. Well, as formerly implemented, you could do this by adding r1 to .roles and then setting .log appropriately. But you know that already and, if I'm not mistaken, have said you don't like it. Right, because it's not flexible. You can't say that you want r1 to have X actions logged, with r2 having Y actions logged. I'm all for making it possible to configure fine-grained auditing, but I don't think that should come at the expense of being able to whack things with a simpler, heavier^Wmore inclusive hammer if you want to. I agree that it's valuable to support simple use-cases with simple configurations. My feeling is that we should strip down what goes into contrib to be 9.5 based. I do not think I can express a constructive opinion about this. I will go along with whatever people agree is best. One of the issues that I see is just how much of the code is under the #ifdef's. If this is going into contrib, we really shouldn't have references and large code blocks that are #ifdef'd out implementations based on out-of-core patches. Further, the pieces which are under the #ifdef's for DEPARSE would likely end up having to be under #if PG_VERSION_NUM, as the deparse tree goes into 9.5. Really, pgaudit pre-deparse and post-deparse are quite different and having them all in one pgaudit.c ends up being pretty grotty. Have you considered splitting pgaudit.c into multiple files, perhaps along the pre/post deparse lines? I'm also wondering if pieces of that are now out-of-date with where core is. Yes, they are. I'll clean that up. Ok, good. I don't particularly like how pgaudit will need to be kept in sync with what's in ProcessUtility (normal and slow). Sure, it's a pain. I'd really like to see this additional information regarding what kind a command is codified in core. Ideally, we'd have a single place which tracks the kind of command and possibly other information which can then be addressed all at once when new commands are added. What would this look like, and is it a realistic goal for 9.5? One thought might be to provide the intersection between LOGSTMT and ObjectTypes. We can learn what commands are DDL and what are modification commands from GetCommandLogLevel. The other distinctions are mostly about different object types and we might be able to use ObjectPropertyType and ObjectTypeMap for that, perhaps adding another 'kind' to ObjectProperty for the object categorization. There are still further distinctions and I agree that it's very useful to be able to identify which commands are privilege-related (T_GrantStmt, T_GrantRoleStmt, T_CreatePolicyStmt, etc) vs. things like Vacuum. Also, we could allow more granularity by using the actual classes which we already have for objects. Explain? That thought was based on looking at ObjectTypeMap- we might be able to provide a way for administrators to configure which objects they want to be audited by naming them using the names provided by ObjectTypeMap. /* * This module collects AuditEvents from various sources (event * triggers, and executor/utility hooks) and passes them to the * log_audit_event() function. * * An AuditEvent represents an operation that potentially affects a * single object. If an underlying command affects multiple objects, * multiple AuditEvents must be created to represent it. */ The above doesn't appear to be accurate (perhaps it once was?) as log_audit_event() only takes a single AuditEvent structure at a time and it's not a list. I'm not sure that needs to change, just pointing out
Re: [HACKERS] proposal: disallow operator = and use it for named parameters
On Mon, Jan 19, 2015 at 2:59 AM, Pavel Stehule pavel.steh...@gmail.com wrote: I think you should just remove the WARNING, not change it to an error. If somebody wants to quote the operator name to be able to continue using it, I think that's OK. It looks so quoting doesn't help here + CREATE OPERATOR = ( +leftarg = int8,-- right unary +procedure = numeric_fac + ); + ERROR: syntax error at or near ( + LINE 1: CREATE OPERATOR = ( + ^ Well then the error check is just dead code. Either way, you don't need it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes superuser review
On Thu, Jan 15, 2015 at 6:03 PM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: * ONLINE_BACKUP - allows role to perform backup operations - originally proposed as BACKUP - due to concern for the use of that term in relation to other potential backup related permissions this form is in line with the documentation as it describes the affected backup operations as being 'online backups'. - applies only to the originally proposed backup functions. I'm slightly mystified as to how including the word online helps here. It's unlikely that there will be an offline_backup permission, because if the system is off-line, SQL-level permissions are irrelevant. * LOG - allows role to rotate log files - remains broad enough to consider future log related operations Maybe LOGFILE? Only because some confusion with the LOG message level seems possible; or confusion about whether this is a permission that lets you log things. -- 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] Error check always bypassed in tablefunc.c
Haven't looked at this patch, but I wonder if it would be better to replace the innards of connectby with a rewrite of the query to use standard WITH queries. Maybe we can remove a couple hundred lines from tablefunc.c? -- Á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] Error check always bypassed in tablefunc.c
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 01/19/2015 08:16 AM, Alvaro Herrera wrote: Haven't looked at this patch, but I wonder if it would be better to replace the innards of connectby with a rewrite of the query to use standard WITH queries. Maybe we can remove a couple hundred lines from tablefunc.c? Seems like a good idea -- connectby is really obsolete for quite a while now other than as an SRF example. I guess we only keep it around for backwards compatibility? Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJUvQ9RAAoJEDfy90M199hlRBwP/0KvbDh8x7PpRtqpjSpH7riL 5MMF12XBOJ1UaUcEDKnEFiFj/DBQs/CRva+GB1ahwo3dqNmD733+w9RsSpdEHM7e 7s8K9zUTrlQYnqMs2GXgoi/DK7qzBgPTkAF+9gp7WPbjCqs/G9f7wlCyxM+2Sg0+ UUEGvI0rSvPObsyKjHMOQMTaMaOiQWvvcQ1aKiTBNl2lg5vb8yNUBbqq5/tlx2Cd OMlJi+PFRkCo7aKjT6HRojYVJCbK+QzXZp1UvXOAWzt1ecR695XR3HDAP/d8IInc gAZJsZbYMw8VuWQa8W6brZd2c+3sU21sMSV50dgBpO5nGBnqryKlLs9bP91+BNu6 doUB2sVDaimcYoN8pML/4lrxhQrr2tm9SBmRJMAJEhKUHnjPB3AGwwB2InDKdusK voIFKS12yduqAI7ZQ8ZcpmxCoOesV6a1himdIH/WikPan1ITkCD+toGtmniEuUNv QwDFPCueswlRJEBEq3pbh07ZN7FBeNYxZMVIcdDmomj2BffoDDwovK9mqm2SgpJq WNCT8388lak0eNyZkt8O/6n514Ensn6KAWD/FunMQPVBwgFn8J6fDChZ9z6aw85X 9RgIb3OyjK7tICpTZ4GXkY5xUma3I3LMogzsnkqFc3FaVVCVJ9eCKZ8l2TEil2Uz 5X498pFhQWaut8ptlS9c =OQdT -END PGP SIGNATURE- -- Sent 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: disallow operator = and use it for named parameters
2015-01-19 14:30 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.com: Pavel Stehule wrote: It looks so quoting doesn't help here + CREATE OPERATOR = ( +leftarg = int8,-- right unary +procedure = numeric_fac + ); + ERROR: syntax error at or near ( + LINE 1: CREATE OPERATOR = ( + ^ Does it work to use OPERATOR(=) syntax? I don't think identifier quoting works for operators. it doesn't work too -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] Fillfactor for GIN indexes
On Mon, Jan 19, 2015 at 5:46 PM, Cédric Villemain ced...@2ndquadrant.com wrote: Le lundi 19 janvier 2015 08:24:08 Robert Haas a écrit : On Sat, Jan 17, 2015 at 4:49 AM, Alexander Korotkov aekorot...@gmail.com wrote: I already wrote quite detailed explanation of subject. Let mel try to explain in shortly. GIN is two level nested btree. Thus, GIN would have absolutely same benefits from fillfactor as btree. Lack of tests showing it is, for sure, fault. However, GIN posting trees are ordered by ItemPointer and this makes some specific. If you have freshly created table and do inserts/updates they would use the end of heap. Thus, inserts would go to the end of GIN posting tree and fillfactor wouldn't affect anything. Fillfactor would give benefits on HOT or heap space re-usage. Ah, OK. Those tests clarify things considerably; I see the point now. So I do. Alexander said: 1) In order to have fully correct support of fillfactor in GIN we need to rewrite GIN build algorithm. 2) Without rewriting GIN build algorithm, not much can be done with entry tree. However, you can implement some heuristics. The patch is 2), for the posting tree only ? Yes, it's just for posting tree. -- With best regards, Alexander Korotkov.
Re: [HACKERS] Fillfactor for GIN indexes
Le lundi 19 janvier 2015 08:24:08 Robert Haas a écrit : On Sat, Jan 17, 2015 at 4:49 AM, Alexander Korotkov aekorot...@gmail.com wrote: I already wrote quite detailed explanation of subject. Let mel try to explain in shortly. GIN is two level nested btree. Thus, GIN would have absolutely same benefits from fillfactor as btree. Lack of tests showing it is, for sure, fault. However, GIN posting trees are ordered by ItemPointer and this makes some specific. If you have freshly created table and do inserts/updates they would use the end of heap. Thus, inserts would go to the end of GIN posting tree and fillfactor wouldn't affect anything. Fillfactor would give benefits on HOT or heap space re-usage. Ah, OK. Those tests clarify things considerably; I see the point now. So I do. Alexander said: 1) In order to have fully correct support of fillfactor in GIN we need to rewrite GIN build algorithm. 2) Without rewriting GIN build algorithm, not much can be done with entry tree. However, you can implement some heuristics. The patch is 2), for the posting tree only ? Thanks! -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] [PATCH] explain sortorder
Hi Tom, we are very happy seeing this committed. Thank you for committing and Mike for the review!! Your changes make sense to us, except one question: We think, you wanted to switch to DESC behavior (print out NULLS FIRST) in cases, where „USING“ uses an operator which is considered to be a DESC operator. Great, we missed that. But get_equality_op_for_ordering_op is called in cases, where reverse is false, but the part if (reverse) *reverse = (strategy == BTGreaterStrategyNumber); never changes this to true? VlG Marius Arne --- Marius Timmer Arne Scheffer Zentrum für Informationsverarbeitung Westfälische Wilhelms-Universität Münster Einsteinstraße 60 mtimm...@uni-muenster.de Am 17.01.2015 um 00:45 schrieb Tom Lane t...@sss.pgh.pa.us: Timmer, Marius marius.tim...@uni-muenster.de writes: attached is version 8, fixing remaining issues, adding docs and tests as requested/agreed. I've committed this with some rather substantial alterations, notably: * Having get_sortorder_by_keyno dig into the plan state node by itself seemed like a bad idea; it's certainly at variance with the existing division of knowledge in this code, and I think it might outright do the wrong thing if there's a Sort node underneath an Agg or Group node (since in those cases the child Sort node, not the parent node, would get passed in). I fixed it so that show_sort_keys and siblings are responsible for extracting and passing in the correct data arrays. * There were some minor bugs in the rules for when to print NULLS FIRST/LAST too. I think the way I've got it now is a precise inverse of what addTargetToSortList() will do. * The proposed new regression test cases were not portable (en_US isn't guaranteed to exist), and I thought adding a new regression script file for just one test was a bit excessive. The DESC and USING behaviors were already covered by existing test cases so I just added something to exercise COLLATE and NULLS FIRST in collate.sql. * I took out the change in perform.sgml. The change as proposed was seriously confusing because it injected off-topic discussion into an example that was meant to be just about the additional information printed by EXPLAIN ANALYZE. I'm not really sure that this feature needs any specific documentation (other than its eventual mention in the release notes); but if it does, we should probably add a brand new example someplace before the EXPLAIN ANALYZE subsection. * Assorted cleanups such as removal of irrelevant whitespace changes. That sort of thing just makes a reviewer's job harder, so it's best to avoid it if you can. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Timmer, Marius marius.tim...@uni-muenster.de writes: We think, you wanted to switch to DESC behavior (print out NULLS FIRST) in cases, where USING uses an operator which is considered to be a DESC operator. Right, because that's how addTargetToSortList() would parse it. But get_equality_op_for_ordering_op is called in cases, where reverse is false, but the part if (reverse) *reverse = (strategy == BTGreaterStrategyNumber); never changes this to true? Sorry, not following? It's true that what I added to explain.c doesn't worry too much about the possibility of get_ordering_op_properties() failing --- that really shouldn't happen for something that was previously accepted as a sorting operator. But if it does, reverse will just be left as false, so the behavior will anyway be unsurprising I think. We could alternatively make it throw a cache lookup failed error but I'm not sure how that makes anyone's life better. 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 buildfarm disk usage: remove temp installs when done
On 01/19/2015 12:28 AM, Tom Lane wrote: An alternative would be to remove the pgsql directory at the end of the run and thus do a complete fresh checkout each run. As you say it would cost some time but save some space. At least it would be doable as an option, not sure I'd want to make it non-optional. What I was thinking is that a complete-fresh-checkout approach would remove the need for the copy_source step that happens now, thus buying back at least most of the I/O cost. But that's only considering the working tree. The real issue here seems to be about having duplicative git repos ... seems like we ought to be able to avoid that. It won't save a copy in the case of a vpath build, because there's no copying done then. But I'm wondering if we should look at using the tricks git-new-workdir uses, setting up symlinks instead of a full clone. Then we'd have one clone with a bunch of different work dirs. That plus a but of explicitly done garbage collection and possibly a periodic re-clone might do the trick. 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] Additional role attributes superuser review
* Simon Riggs (si...@2ndquadrant.com) wrote: On 3 November 2014 at 17:08, Stephen Frost sfr...@snowman.net wrote: role attributes don't act like GRANTs anyway (there's no ADMIN option and they aren't inheirited). I'm happy with us *not* doing this using GRANTs, as long as we spend some love on the docs to show there is a very clear distinction between the two. The distinction already exists. I agree that the documentation should be improved to clarify how GRANT'd privileges are different from role attributes (which is what our existing superuser, createrole, etc options are). Users get confused between privs, role attributes and SETs that apply to roles. Agreed. Introducing the new word capability needs to also have some clarity. Is that the same thing as role attribute, or is that a 4th kind of thang? At present, it's exactly the same as 'role attribute' and, for my part at least, I was thinking it would remain the same. I believe the idea was to migrate the terminology from 'role attribute' to 'capability' as the latter better represents both the existing options and the new ones. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Mon, Jan 19, 2015 at 10:35 AM, Tom Lane t...@sss.pgh.pa.us wrote: If that were true I'd agree with you, but it's false on its face. A user who is actually examining the statistics might well want to know if they're significantly out of date. A very relevant example is that I've always wondered how come, when we see buildfarm failures in the stats regression test, they always appear in the form of output differences that indicate that the session did not see the expected stats update --- but there's never a timeout warning printed, which indicates that whatever the cause is, it ain't that. Sure, but nobody who is not a developer is going to care about that. A typical user who sees pgstat wait timeout, or doesn't, isn't going to be able to make anything at all out of that. I'd be fine with changing the warning to LOG level rather than suppressing it entirely for the specific case of pgstat_vacuum_stat; but I do want to distinguish that case, wherein it's fair to conclude that obsolete stats aren't too worrisome, from other cases where no such conclusion is justified. But I can live with this compromise. -- 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
Robert Haas robertmh...@gmail.com writes: Sure, but nobody who is not a developer is going to care about that. A typical user who sees pgstat wait timeout, or doesn't, isn't going to be able to make anything at all out of that. Possibly we need to improve the wording of that error message then. When it was written, we really assumed that it was a can't-happen case and so didn't spend much effort on it. Perhaps it should become a translatable ereport phrased like WARNING: using stale statistics instead of current ones because stats collector is not responding. (I'm also wondering if it'd make sense to expose the stats timestamp as a callable function, so that the case could be dealt with programmatically as well. But that's future-feature territory.) I'd be fine with changing the warning to LOG level rather than suppressing it entirely for the specific case of pgstat_vacuum_stat; but I do want to distinguish that case, wherein it's fair to conclude that obsolete stats aren't too worrisome, from other cases where no such conclusion is justified. But I can live with this compromise. Is that OK with everybody? Going once, going twice ... 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 buildfarm disk usage: remove temp installs when done
Andrew Dunstan and...@dunslane.net writes: But I'm wondering if we should look at using the tricks git-new-workdir uses, setting up symlinks instead of a full clone. Then we'd have one clone with a bunch of different work dirs. That plus a but of explicitly done garbage collection and possibly a periodic re-clone might do the trick. Yeah, I was wondering whether it'd be okay to depend on git-new-workdir. That would fix the problem pretty nicely. But in the installations I've seen, that's not in PATH but squirreled away in some hard-to-guess library directory ... 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
On 3 November 2014 at 17:08, Stephen Frost sfr...@snowman.net wrote: role attributes don't act like GRANTs anyway (there's no ADMIN option and they aren't inheirited). I'm happy with us *not* doing this using GRANTs, as long as we spend some love on the docs to show there is a very clear distinction between the two. Users get confused between privs, role attributes and SETs that apply to roles. Introducing the new word capability needs to also have some clarity. Is that the same thing as role attribute, or is that a 4th kind of thang? Make things make sense and they're easy to agree. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] Reducing buildfarm disk usage: remove temp installs when done
On 01/19/2015 09:53 AM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: But I'm wondering if we should look at using the tricks git-new-workdir uses, setting up symlinks instead of a full clone. Then we'd have one clone with a bunch of different work dirs. That plus a but of explicitly done garbage collection and possibly a periodic re-clone might do the trick. Yeah, I was wondering whether it'd be okay to depend on git-new-workdir. That would fix the problem pretty nicely. But in the installations I've seen, that's not in PATH but squirreled away in some hard-to-guess library directory ... Yeah. Luckily, there are really only half a dozen or so lines of script that do the actual work - the rest is sanity checks. I think we can replicate that without requiring the script. I'll have a stab later in the week. 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] WITH CHECK and Column-Level Privileges
Noah,
* Noah Misch (n...@leadboat.com) wrote:
On Mon, Jan 12, 2015 at 05:16:40PM -0500, Stephen Frost wrote:
Alright, here's an updated patch which doesn't return any detail if no
values are visible or if only a partial key is visible.
I browsed this patch. There's been no mention of foreign key constraints, but
ri_ReportViolation() deserves similar hardening. If a user has only DELETE
privilege on a PK table, FK violation messages should not leak the PK values.
Modifications to the foreign side are less concerning, since the user will
often know the attempted value; still, I would lock down both sides.
Done.
Please add a comment explaining the safety of each row-emitting message you
haven't changed. For example, the one in refresh_by_match_merge() is safe
because getting there requires MV ownership.
Done.
[...]
Instead of duplicating an entire ereport() to change whether you include an
errdetail, use condition ? errdetail(...) : 0.
Done.
I've also updated the commit message to note the assigned CVE.
One remaining question is about single-column key violations. Should we
special-case those and allow them to be shown or no? I can't see a
reason not to currently but I wonder if we might have cause to act
differently in the future (not that I can think of a reason we'd ever
need to).
Certainly happy to change the specific messages around, if folks would
prefer something different from what I've chosen. I've kept errdetail's
for the cases where I feel it's still useful clarification.
Thanks!
Stephen
From 3ea19711f06718fa3e43fa8e587a54bcd6acf8fa Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Mon, 12 Jan 2015 17:04:11 -0500
Subject: [PATCH] Fix column-privilege leak in error-message paths
While building error messages to return to the user,
BuildIndexValueDescription, ExecBuildSlotValueDescription and
ri_ReportViolation would happily include the entire key or entire row in
the result returned to the user, even if the user didn't have access to
view all of the columns being included.
Instead, include only those columns which the user is providing or which
the user has select rights on. If the user does not have any rights
to view the table or any of the columns involved then no detail is
provided. Note that, for key cases, the user must have access to all of
the columns for the key to be shown; a partial key will not be returned.
Back-patch all the way, as column-level privileges are now in all
supported versions.
This has been assigned CVE-2014-8161, but since the issue and the patch
have already been publicized on pgsql-hackers, there's no point in trying
to hide this commit.
---
src/backend/access/index/genam.c | 59 ++-
src/backend/access/nbtree/nbtinsert.c| 13 ++-
src/backend/commands/copy.c | 6 +-
src/backend/commands/matview.c | 7 ++
src/backend/executor/execMain.c | 170 ---
src/backend/executor/execUtils.c | 12 ++-
src/backend/utils/adt/ri_triggers.c | 78 ++
src/backend/utils/sort/tuplesort.c | 28 +++--
src/test/regress/expected/privileges.out | 31 ++
src/test/regress/sql/privileges.sql | 24 +
10 files changed, 351 insertions(+), 77 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 850008b..e34a280 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -25,10 +25,12 @@
#include lib/stringinfo.h
#include miscadmin.h
#include storage/bufmgr.h
+#include utils/acl.h
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/rel.h
#include utils/snapmgr.h
+#include utils/syscache.h
#include utils/tqual.h
@@ -154,6 +156,9 @@ IndexScanEnd(IndexScanDesc scan)
* form (key_name, ...)=(key_value, ...). This is currently used
* for building unique-constraint and exclusion-constraint error messages.
*
+ * Note that if the user does not have permissions to view the columns
+ * involved, an empty string will be returned instead.
+ *
* The passed-in values/nulls arrays are the raw input to the index AM,
* e.g. results of FormIndexDatum --- this is not necessarily what is stored
* in the index, but it's what the user perceives to be stored.
@@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
Datum *values, bool *isnull)
{
StringInfoData buf;
+ Form_pg_index idxrec;
+ HeapTuple ht_idx;
int natts = indexRelation-rd_rel-relnatts;
int i;
+ int keyno;
+ Oid indexrelid = RelationGetRelid(indexRelation);
+ Oid indrelid;
+ AclResult aclresult;
+
+ /*
+ * Check permissions- if the users does not have access to view the
+ * data in the key columns, we return instead, which callers can
+ * test for and use or not accordingly.
+ *
+ * First we need to check table-level SELECT access and then, if
+ * there is no access there, check
Re: [HACKERS] proposal: disallow operator = and use it for named parameters
2015-01-19 14:27 GMT+01:00 Robert Haas robertmh...@gmail.com:
On Mon, Jan 19, 2015 at 2:59 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:
I think you should just remove the WARNING, not change it to an error.
If somebody wants to quote the operator name to be able to continue
using it, I think that's OK.
It looks so quoting doesn't help here
+ CREATE OPERATOR = (
+leftarg = int8,-- right unary
+procedure = numeric_fac
+ );
+ ERROR: syntax error at or near (
+ LINE 1: CREATE OPERATOR = (
+ ^
Well then the error check is just dead code. Either way, you don't need
it.
yes, I removed it
Regards
Pavel
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 5e7b000..c33190e
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT SUBSTRING('XY1234Z', 'Y*?([0-9]{1
*** 6785,6791
Create interval from years, months, weeks, days, hours, minutes and
seconds fields
/entry
! entryliteralmake_interval(days := 10)/literal/entry
entryliteral10 days/literal/entry
/row
--- 6785,6791
Create interval from years, months, weeks, days, hours, minutes and
seconds fields
/entry
! entryliteralmake_interval(days = 10)/literal/entry
entryliteral10 days/literal/entry
/row
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
new file mode 100644
index 4b81b08..d30db6a
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*** SELECT concat_lower_or_upper('Hello', 'W
*** 2599,2605
literal:=/literal to separate it from the argument expression.
For example:
screen
! SELECT concat_lower_or_upper(a := 'Hello', b := 'World');
concat_lower_or_upper
---
hello world
--- 2599,2605
literal:=/literal to separate it from the argument expression.
For example:
screen
! SELECT concat_lower_or_upper(a = 'Hello', b = 'World');
concat_lower_or_upper
---
hello world
*** SELECT concat_lower_or_upper(a := 'Hello
*** 2610,2622
using named notation is that the arguments may be specified in any
order, for example:
screen
! SELECT concat_lower_or_upper(a := 'Hello', b := 'World', uppercase := true);
concat_lower_or_upper
---
HELLO WORLD
(1 row)
! SELECT concat_lower_or_upper(a := 'Hello', uppercase := true, b := 'World');
concat_lower_or_upper
---
HELLO WORLD
--- 2610,2633
using named notation is that the arguments may be specified in any
order, for example:
screen
! SELECT concat_lower_or_upper(a = 'Hello', b = 'World', uppercase = true);
concat_lower_or_upper
---
HELLO WORLD
(1 row)
! SELECT concat_lower_or_upper(a = 'Hello', uppercase = true, b = 'World');
! concat_lower_or_upper
! ---
! HELLO WORLD
! (1 row)
! /screen
! /para
!
! para
! Older syntax based on := symbol is still supported:
! screen
! SELECT concat_lower_or_upper(a := 'Hello', uppercase := true, b := 'World');
concat_lower_or_upper
---
HELLO WORLD
*** SELECT concat_lower_or_upper(a := 'Hello
*** 2638,2644
already mentioned, named arguments cannot precede positional arguments.
For example:
screen
! SELECT concat_lower_or_upper('Hello', 'World', uppercase := true);
concat_lower_or_upper
---
HELLO WORLD
--- 2649,2655
already mentioned, named arguments cannot precede positional arguments.
For example:
screen
! SELECT concat_lower_or_upper('Hello', 'World', uppercase = true);
concat_lower_or_upper
---
HELLO WORLD
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
new file mode 100644
index f40504c..264e5ff
*** a/doc/src/sgml/xfunc.sgml
--- b/doc/src/sgml/xfunc.sgml
*** SELECT mleast(VARIADIC ARRAY[]::numeric[
*** 776,789
literalVARIADIC/. For example, this will work:
screen
! SELECT mleast(VARIADIC arr := ARRAY[10, -1, 5, 4.4]);
/screen
but not these:
screen
! SELECT mleast(arr := 10);
! SELECT mleast(arr := ARRAY[10, -1, 5, 4.4]);
/screen
/para
/sect2
--- 776,789
literalVARIADIC/. For example, this will work:
screen
! SELECT mleast(VARIADIC arr = ARRAY[10, -1, 5, 4.4]);
/screen
but not these:
screen
! SELECT mleast(arr = 10);
! SELECT mleast(arr = ARRAY[10, -1, 5, 4.4]);
/screen
/para
/sect2
diff --git a/src/backend/commands/operatorcmds.c b/src/backend/commands/operatorcmds.c
new file mode 100644
index 2996019..e4327c2
*** a/src/backend/commands/operatorcmds.c
Re: [HACKERS] Fillfactor for GIN indexes
Le samedi 17 janvier 2015 08:23:44 Michael Paquier a écrit : On Sat, Jan 17, 2015 at 2:40 AM, Robert Haas robertmh...@gmail.com wrote: There's only value in adding a fillfactor parameter to GIN indexes if it improves performance. There are no benchmarks showing it does. So, why are we still talking about this? Indeed. There is no such benchmark as of now, and I am not sure I'll get the time to work more on that soon, so let's reject the patch for now. And sorry for the useless noise. Michael, I first didn't understood how GiN can benefits from the patch...thus my question. There were no noise for me, and I learn some more about GiN. So I thank you for your work! -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
Robert Haas robertmh...@gmail.com writes: On Mon, Jan 19, 2015 at 7:09 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-18 21:33:25 -0500, Robert Haas wrote: I think this is too much of a good thing. I don't see any reason why autovacuum's statistics need to be fresher than normal, but I also don't see any reason why they need to be less fresh. I think suppressing the warning is a good idea, but why only suppress it for autovacuum? How about just knocking the level down to, say, DEBUG1? +1 for just using LOG - which by default does not end up on client machines. In contrast to WARNING. Yeah, that doesn't seem like a bad idea, either. The message seems much more likely to be of interest to the DBA than the user; the DBA can use the message to diagnose an overloaded I/O subsystem (which I think is the usual cause of this problem) whereas the only point of likely interest to the user is that their query ran slowly (which they know without the message). If that were true I'd agree with you, but it's false on its face. A user who is actually examining the statistics might well want to know if they're significantly out of date. A very relevant example is that I've always wondered how come, when we see buildfarm failures in the stats regression test, they always appear in the form of output differences that indicate that the session did not see the expected stats update --- but there's never a timeout warning printed, which indicates that whatever the cause is, it ain't that. I'd be fine with changing the warning to LOG level rather than suppressing it entirely for the specific case of pgstat_vacuum_stat; but I do want to distinguish that case, wherein it's fair to conclude that obsolete stats aren't too worrisome, from other cases where no such conclusion is justified. 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On 2015-01-18 21:33:25 -0500, Robert Haas wrote: On Sun, Jan 18, 2015 at 4:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: After looking at the code, the minimum-change alternative would be more or less as attached: first, get rid of the long-obsolete proposition that autovacuum workers need fresher-than-usual stats; second, allow pgstat_vacuum_stat to accept stats that are moderately stale (the number given below allows them to be up to 50 seconds old); and third, suppress wait-timeout warnings when the call is from pgstat_vacuum_stat. The third point is what we need to avoid unnecessary buildfarm failures. The second point addresses the idea that we don't need to stress the stats collector too much for this. I think this is too much of a good thing. I don't see any reason why autovacuum's statistics need to be fresher than normal, but I also don't see any reason why they need to be less fresh. I think suppressing the warning is a good idea, but why only suppress it for autovacuum? How about just knocking the level down to, say, DEBUG1? +1 for just using LOG - which by default does not end up on client machines. In contrast to WARNING. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On 2015-01-19 11:28:41 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2015-01-19 11:16:09 -0500, Tom Lane wrote: Possibly we need to improve the wording of that error message then. When it was written, we really assumed that it was a can't-happen case and so didn't spend much effort on it. Perhaps it should become a translatable ereport phrased like WARNING: using stale statistics instead of current ones because stats collector is not responding. Yes, that seems like a good message improvement. It's not like making it LOG makes the message invisible... Uh, yes it does. So far as the user is concerned anyway. Sure, but the log isn't invisible. As mentioned one paragraph above, I don't think it's likely to ever be noticed in the client code in the cases where it happens in production. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] explain sortorder
Tom, Thanks for the comments on what you ended up changing. It helps point out the kind of things I should be looking for. I'll try to let less slip through in the future. Mike __ *Mike Blackwell | Technical Analyst, Distribution Services/Rollout Management | RR Donnelley* 1750 Wallace Ave | St Charles, IL 60174-3401 Office: 630.313.7818 mike.blackw...@rrd.com http://www.rrdonnelley.com http://www.rrdonnelley.com/ * mike.blackw...@rrd.com* On Mon, Jan 19, 2015 at 10:09 AM, Tom Lane t...@sss.pgh.pa.us wrote: Timmer, Marius marius.tim...@uni-muenster.de writes: We think, you wanted to switch to DESC behavior (print out NULLS FIRST) in cases, where „USING“ uses an operator which is considered to be a DESC operator. Right, because that's how addTargetToSortList() would parse it. But get_equality_op_for_ordering_op is called in cases, where reverse is false, but the part if (reverse) *reverse = (strategy == BTGreaterStrategyNumber); never changes this to true? Sorry, not following? It's true that what I added to explain.c doesn't worry too much about the possibility of get_ordering_op_properties() failing --- that really shouldn't happen for something that was previously accepted as a sorting operator. But if it does, reverse will just be left as false, so the behavior will anyway be unsurprising I think. We could alternatively make it throw a cache lookup failed error but I'm not sure how that makes anyone's life better. 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] Another comment typo in src/backend/executor/execMain.c
On Mon, Jan 19, 2015 at 4:00 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: I ran into another typo in execMain.c. Patch attached. Thanks, 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On 2015-01-19 11:16:09 -0500, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: I'd be fine with changing the warning to LOG level rather than suppressing it entirely for the specific case of pgstat_vacuum_stat; but I do want to distinguish that case, wherein it's fair to conclude that obsolete stats aren't too worrisome, from other cases where no such conclusion is justified. But I can live with this compromise. Is that OK with everybody? Going once, going twice ... I can live with the compromise as well, but I'd rather change it to always be of LOG priority. LOG is more likely to end up in the log and that's where it's actually likely to be noticed. In most of the cases WARNINGs going to the client won't be noticed as this error is much more likely on servers with a high load caused by programs than during interactive use. Sure, but nobody who is not a developer is going to care about that. A typical user who sees pgstat wait timeout, or doesn't, isn't going to be able to make anything at all out of that. Possibly we need to improve the wording of that error message then. When it was written, we really assumed that it was a can't-happen case and so didn't spend much effort on it. Perhaps it should become a translatable ereport phrased like WARNING: using stale statistics instead of current ones because stats collector is not responding. Yes, that seems like a good message improvement. It's not like making it LOG makes the message invisible... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WITH CHECK and Column-Level Privileges
I'm confused. Why does ExecBuildSlotValueDescription() return an empty string instead of NULL? There doesn't seem to be any value left in that idea, and returning NULL would make the callsites slightly simpler as well. (Also, I think the comment on top of it should be updated to reflect the permissions-related issues.) It seems that the reason for this is to be consistent with BuildIndexValueDescription, which seems to do the same thing to simplify the stuff going on at check_exclusion_constraint() -- by returning an empty string, that code doesn't need to check which of the returned values is empty, only whether both are. That seems an odd choice to me; it seems better to me to be specific, i.e. include each of the %s depending on whether the returned string is null or not. You would have three possible different errdetails, but that seems a good thing anyway. (Also, ISTM the if (!strlen(foo)) idiom should be avoided because it is confusing; better test explicitely for zero or nonzero. Anyway if you change the functions to return NULL, you can test for NULL-ness easily and there's no possible confusion anymore.) -- Á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] CATUPDATE confusion?
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: Given this discussion, I have attached a patch that removes CATUPDATE for review/discussion. Thanks! I've added this patch (up-thread) to the next CommitFest (2015-02). -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] WITH CHECK and Column-Level Privileges
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: I'm confused. Why does ExecBuildSlotValueDescription() return an empty string instead of NULL? There doesn't seem to be any value left in that idea, and returning NULL would make the callsites slightly simpler as well. (Also, I think the comment on top of it should be updated to reflect the permissions-related issues.) The comment on top of ExecBuildSlotValueDescription() does include this: * Note that, like BuildIndexValueDescription, if the user does not have * permission to view any of the columns involved, an empty string is returned. Is that insufficient? It seems that the reason for this is to be consistent with BuildIndexValueDescription, which seems to do the same thing to simplify the stuff going on at check_exclusion_constraint() -- by returning an empty string, that code doesn't need to check which of the returned values is empty, only whether both are. That seems an odd choice to me; it seems better to me to be specific, i.e. include each of the %s depending on whether the returned string is null or not. You would have three possible different errdetails, but that seems a good thing anyway. Right, ExecBuildSlotValueDescription() was made to be consistent with BuildIndexValueDescription. The reason for BuildIndexValueDescription returning an empty string is different from why you hypothosize though- it's exported and I was a bit worried that existing external callers might not be prepared for it to return a NULL instead of a string of some kind. If this was a green field instead of a back-patch fix, I'd certainly return NULL instead. If others feel that's not a good reason to avoid returning NULL, I can certainly change it around. (Also, ISTM the if (!strlen(foo)) idiom should be avoided because it is confusing; better test explicitely for zero or nonzero. Anyway if you change the functions to return NULL, you can test for NULL-ness easily and there's no possible confusion anymore.) Yeah, I thought I had eliminated all of those on my own review, but looks like I missed one. Updated patch attached. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WITH CHECK and Column-Level Privileges
* Stephen Frost (sfr...@snowman.net) wrote:
Updated patch attached.
Ugh. Hit send too quickly. Attached.
Thanks!
Stephen
From f74dcef56bd3507c6bb26b0468655fd8e408fc80 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Mon, 12 Jan 2015 17:04:11 -0500
Subject: [PATCH] Fix column-privilege leak in error-message paths
While building error messages to return to the user,
BuildIndexValueDescription, ExecBuildSlotValueDescription and
ri_ReportViolation would happily include the entire key or entire row in
the result returned to the user, even if the user didn't have access to
view all of the columns being included.
Instead, include only those columns which the user is providing or which
the user has select rights on. If the user does not have any rights
to view the table or any of the columns involved then no detail is
provided. Note that, for key cases, the user must have access to all of
the columns for the key to be shown; a partial key will not be returned.
Back-patch all the way, as column-level privileges are now in all
supported versions.
This has been assigned CVE-2014-8161, but since the issue and the patch
have already been publicized on pgsql-hackers, there's no point in trying
to hide this commit.
---
src/backend/access/index/genam.c | 59 ++-
src/backend/access/nbtree/nbtinsert.c| 13 ++-
src/backend/commands/copy.c | 6 +-
src/backend/commands/matview.c | 7 ++
src/backend/executor/execMain.c | 170 ---
src/backend/executor/execUtils.c | 12 ++-
src/backend/utils/adt/ri_triggers.c | 78 ++
src/backend/utils/sort/tuplesort.c | 10 +-
src/test/regress/expected/privileges.out | 31 ++
src/test/regress/sql/privileges.sql | 24 +
10 files changed, 339 insertions(+), 71 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 850008b..e34a280 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -25,10 +25,12 @@
#include lib/stringinfo.h
#include miscadmin.h
#include storage/bufmgr.h
+#include utils/acl.h
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/rel.h
#include utils/snapmgr.h
+#include utils/syscache.h
#include utils/tqual.h
@@ -154,6 +156,9 @@ IndexScanEnd(IndexScanDesc scan)
* form (key_name, ...)=(key_value, ...). This is currently used
* for building unique-constraint and exclusion-constraint error messages.
*
+ * Note that if the user does not have permissions to view the columns
+ * involved, an empty string will be returned instead.
+ *
* The passed-in values/nulls arrays are the raw input to the index AM,
* e.g. results of FormIndexDatum --- this is not necessarily what is stored
* in the index, but it's what the user perceives to be stored.
@@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
Datum *values, bool *isnull)
{
StringInfoData buf;
+ Form_pg_index idxrec;
+ HeapTuple ht_idx;
int natts = indexRelation-rd_rel-relnatts;
int i;
+ int keyno;
+ Oid indexrelid = RelationGetRelid(indexRelation);
+ Oid indrelid;
+ AclResult aclresult;
+
+ /*
+ * Check permissions- if the users does not have access to view the
+ * data in the key columns, we return instead, which callers can
+ * test for and use or not accordingly.
+ *
+ * First we need to check table-level SELECT access and then, if
+ * there is no access there, check column-level permissions.
+ */
+
+ /*
+ * Fetch the pg_index tuple by the Oid of the index
+ */
+ ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexrelid));
+ if (!HeapTupleIsValid(ht_idx))
+ elog(ERROR, cache lookup failed for index %u, indexrelid);
+ idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
+
+ indrelid = idxrec-indrelid;
+ Assert(indexrelid == idxrec-indexrelid);
+
+ /* Table-level SELECT is enough, if the user has it */
+ aclresult = pg_class_aclcheck(indrelid, GetUserId(), ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ {
+ /*
+ * No table-level access, so step through the columns in the
+ * index and make sure the user has SELECT rights on all of them.
+ */
+ for (keyno = 0; keyno idxrec-indnatts; keyno++)
+ {
+ AttrNumber attnum = idxrec-indkey.values[keyno];
+
+ aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
+ ACL_SELECT);
+
+ if (aclresult != ACLCHECK_OK)
+ {
+/* No access, so clean up and just return */
+ReleaseSysCache(ht_idx);
+return ;
+ }
+ }
+ }
+ ReleaseSysCache(ht_idx);
initStringInfo(buf);
appendStringInfo(buf, (%s)=(,
- pg_get_indexdef_columns(RelationGetRelid(indexRelation),
- true));
+ pg_get_indexdef_columns(indexrelid, true));
for (i = 0; i natts; i++)
{
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 59d7006..2413c68 100644
---
Re: [HACKERS] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code
On 2015-01-19 17:16:11 +0900, Michael Paquier wrote:
On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund and...@2ndquadrant.com
wrote:
Not this patch's fault, but I'm getting a bit tired seeing the above open
coded. How about adding a function that does the sleeping based on a
timestamptz and a ms interval?
You mean in plugins, right? I don't recall seeing similar patterns in
other code paths of backend. But I think that we can use something
like that in timestamp.c then because we need to leverage that between
two timestamps, the last failure and now():
TimestampSleepDifference(start_time, stop_time, internal_ms);
Perhaps you have something else in mind?
Attached is an updated patch.
Actually I came with better than last patch by using a boolean flag as
return value of TimestampSleepDifference and use
TimestampDifferenceExceeds directly inside it.
Subject: [PATCH] Add wal_availability_check_interval to control WAL fetching
on failure
I think that name isn't a very good. And its isn't very accurate
either.
How about wal_retrieve_retry_interval? Not very nice, but I think it's
still better than the above.
+ varlistentry id=wal-availability-check-interval
xreflabel=wal_availability_check_interval
+ termvarnamewal_availability_check_interval/varname
(typeinteger/type)
+ indexterm
+primaryvarnamewal_availability_check_interval/ recovery
parameter/primary
+ /indexterm
+ /term
+ listitem
+ para
+This parameter specifies the amount of time to wait when
+WAL is not available for a node in recovery. Default value is
+literal5s/.
+ /para
+ para
+A node in recovery will wait for this amount of time if
+varnamerestore_command/ returns nonzero exit status code when
+fetching new WAL segment files from archive or when a WAL receiver
+is not able to fetch a WAL record when using streaming replication.
+ /para
+ /listitem
+ /varlistentry
+
/variablelist
Walreceiver doesn't wait that amount, but rather how long the connection
is intact. And restore_command may or may not retry.
/*---
* Standby mode is implemented by a state machine:
@@ -10490,15 +10511,13 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool
randAccess,
* machine, so we've exhausted all the
options for
* obtaining the requested WAL. We're
going to loop back
* and retry from the archive, but if
it hasn't been long
- * since last attempt, sleep 5 seconds
to avoid
- * busy-waiting.
+ * since last attempt, sleep the amount
of time specified
+ * by wal_availability_check_interval
to avoid busy-waiting.
*/
- now = (pg_time_t) time(NULL);
- if ((now - last_fail_time) 5)
- {
- pg_usleep(100L * (5 - (now
- last_fail_time)));
- now = (pg_time_t) time(NULL);
- }
+ now = GetCurrentTimestamp();
+ if
(TimestampSleepDifference(last_fail_time, now,
+
wal_availability_check_interval))
+ now = GetCurrentTimestamp();
Not bad, that's much easier to read imo.
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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
Robert Haas robertmh...@gmail.com writes: On Mon, Jan 19, 2015 at 11:30 AM, Andres Freund and...@2ndquadrant.com wrote: Sure, but the log isn't invisible. As mentioned one paragraph above, I don't think it's likely to ever be noticed in the client code in the cases where it happens in production. Yes, that is my feeling as well. Meh. I still don't like it, but I guess I'm outvoted. Unless there are further votes, what we have at this point is just: - elog(WARNING, pgstat wait timeout); + ereport(LOG, (errmsg(using stale statistics instead of current ones because stats collector is not responding))); with no conditionality for pgstat_vacuum_stat vs. other callers. 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Mon, Jan 19, 2015 at 11:16 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Sure, but nobody who is not a developer is going to care about that. A typical user who sees pgstat wait timeout, or doesn't, isn't going to be able to make anything at all out of that. Possibly we need to improve the wording of that error message then. When it was written, we really assumed that it was a can't-happen case and so didn't spend much effort on it. Perhaps it should become a translatable ereport phrased like WARNING: using stale statistics instead of current ones because stats collector is not responding. I'm still not completely convinced it deserves to be a WARNING, but I definitely think turning it into a translatable error message is the right call. Calling this a can't happen case is clearly ridiculous at this point. -- 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Mon, Jan 19, 2015 at 11:30 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-19 11:28:41 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2015-01-19 11:16:09 -0500, Tom Lane wrote: Possibly we need to improve the wording of that error message then. When it was written, we really assumed that it was a can't-happen case and so didn't spend much effort on it. Perhaps it should become a translatable ereport phrased like WARNING: using stale statistics instead of current ones because stats collector is not responding. Yes, that seems like a good message improvement. It's not like making it LOG makes the message invisible... Uh, yes it does. So far as the user is concerned anyway. Sure, but the log isn't invisible. As mentioned one paragraph above, I don't think it's likely to ever be noticed in the client code in the cases where it happens in production. Yes, that is my feeling as well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
Andres Freund and...@2ndquadrant.com writes: On 2015-01-19 11:16:09 -0500, Tom Lane wrote: Possibly we need to improve the wording of that error message then. When it was written, we really assumed that it was a can't-happen case and so didn't spend much effort on it. Perhaps it should become a translatable ereport phrased like WARNING: using stale statistics instead of current ones because stats collector is not responding. Yes, that seems like a good message improvement. It's not like making it LOG makes the message invisible... Uh, yes it does. So far as the user is concerned anyway. The entire point of this discussion is to prevent the message from showing up in buildfarm output, remember? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Reducing lock strength of trigger and foreign key DDL
On Fri, Jan 16, 2015 at 10:59 AM, Andres Freund and...@2ndquadrant.com wrote: Just consider: S1: CREATE TABLE flubber(id serial primary key, data text); S1: CREATE FUNCTION blarg() RETURNS TRIGGER LANGUAGE plpgsql AS $$BEGIN RETURN NEW; END;$$; S1: CREATE TRIGGER flubber_blarg BEFORE INSERT ON flubber FOR EACH ROW WHEN (NEW.data IS NOT NULL) EXECUTE PROCEDURE blarg(); S2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; S2: SELECT 'dosomethingelse'; S1: ALTER TABLE flubber RENAME TO wasflubber; S1: ALTER TABLE wasflubber RENAME COLUMN data TO wasdata; S1: ALTER TRIGGER flubber_blarg ON wasflubber RENAME TO wasflubber_blarg; S1: ALTER FUNCTION blarg() RENAME TO wasblarg; S2: SELECT pg_get_triggerdef(oid) FROM pg_trigger; This will give you: The old trigger name. The new table name. The new column name. The new function name. Ouch. That's clearly no good. I'm struggling to understand whether this is a problem with our previous analysis, or a problem with this patch: http://www.postgresql.org/message-id/20141028003356.ga387...@tornado.leadboat.com pg_get_triggerdef_worker() relies on generate_function_name(), which uses the system caches, and on get_rule_expr(), for deparsing the WHEN clause. If we allowed only ADDING triggers with a lesser lock and never modifying or dropping them with a lesser lock, then changing the initial scan of pg_trigger at the top of pg_get_triggerdef_worker() to use the transaction snapshot might be OK; if we can see the trigger with the transaction snapshot at all, we know it can't have subsequently changed. But allowing alterations of any kind isn't going to work, so I think our previous analysis on that point was incorrect. I *think* we could fix that if generate_function_name() and get_rule_expr() had an option to use the active snapshot instead of a fresh snapshot. The former doesn't look too hard to arrange, but the latter is a tougher nut to crack. -- 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] New CF app deployment
On Mon, Jan 19, 2015 at 10:01 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander mag...@hagander.net wrote: The new site has been deployed and should now be usable. The old site is still available in readonly mode at https://commitfest-old.postgresql.org/. These links, which were originally requested by Tom and which I have bookmarked, used, and publicized extensively, no longer work: https://commitfest.postgresql.org/action/commitfest_view/inprogress https://commitfest.postgresql.org/action/commitfest_view/open Any chance you could make those work, or at the very least provide some equivalent? Hmm. I missed that request. Will fix. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] New CF app deployment
On Mon, Jan 19, 2015 at 4:05 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander mag...@hagander.net wrote: The new site has been deployed and should now be usable. There are, for some reason, three copies of Clarify need for memory barriers in bgworkers in the in-progress CF. I don't know why that happened, or how to fix it. There are also two copies of ctidscan as an example of custom-scan. Also, I can't figure out what I'm supposed to do with the Author and Reviewer checkboxes. Checking and unchecking them doesn't seem to do anything. -- 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] documentation update for doc/src/sgml/func.sgml
On Wed, Dec 31, 2014 at 9:44 AM, Fabien COELHO coe...@cri.ensmp.fr wrote: Here is a slight update so that type names are treated homogeneously between both added paragraphs. ITSM that this patch should be committed without further ado. I had a look at this patch. This patch adds some text below a table of functions. Immediately above that table, there is this existing language: The functions working with typedouble precision/type data are mostly implemented on top of the host system's C library; accuracy and behavior in boundary cases can therefore vary depending on the host system. This seems to me to substantially duplicate the information added by the patch. -- 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] New CF app deployment
On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander mag...@hagander.net wrote: The new site has been deployed and should now be usable. The old site is still available in readonly mode at https://commitfest-old.postgresql.org/. These links, which were originally requested by Tom and which I have bookmarked, used, and publicized extensively, no longer work: https://commitfest.postgresql.org/action/commitfest_view/inprogress https://commitfest.postgresql.org/action/commitfest_view/open Any chance you could make those work, or at the very least provide some equivalent? -- 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] Re: [COMMITTERS] pgsql: Another attempt at fixing Windows Norwegian locale.
On 01/16/2015 07:05 PM, Heikki Linnakangas wrote: On 01/16/2015 04:17 PM, Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@iki.fi writes: Backpatch to 9.2 like the previous attempt. We haven't made a release that includes the previous fix yet, so we don't need to worry about changing the locale of existing clusters from norwegian-bokmal to Norwegian_Norway. (Doing any mapping like this at all requires changing the locale of existing databases; the release notes need to include instructions for that). What instructions do you have in mind to give? I wrote preliminary instructions here: http://www.postgresql.org/message-id/attachment/35205/fix-bokmal-pg_database.txt. Another method is to use pg_upgrade. I'll need to format that for the release notes, and add more explanation of what the issue is and who it applies to. Or perhaps it would be better to put those instructions on a wiki page, so that we can easily add to it later if necessary? Ok, I have created a wiki page for these instructions: http://wiki.postgresql.org/wiki/Changes_To_Norwegian_Locale They can be moved to the release notes, or we can just add a note there with a link to the wiki page. I think the latter would be better. Suggested reference in the release notes: Migration to Version X If you are a Windows user, using the Norwegian (Bokmål) locale, manual action is needed after the upgrade, to replace the Norwegian (Bokmål)_Norway locale names stored in system catalogs with its pure-ASCII alias, Norwegian_Norway. More information is available at http://wiki.postgresql.org/wiki/Changes_To_Norwegian_Locale - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
On Tue, Dec 2, 2014 at 8:28 PM, Peter Geoghegan p...@heroku.com wrote: On Tue, Dec 2, 2014 at 2:16 PM, Peter Geoghegan p...@heroku.com wrote: On Tue, Dec 2, 2014 at 2:07 PM, Robert Haas robertmh...@gmail.com wrote: Well, maybe you should make the updates we've agreed on and I can take another look at it. Agreed. Attached, revised patchset makes these updates. I continue to use the sortsupport struct to convey that a given attribute of a given sort is abbreviation-applicable (although the field is now a bool, not an enum). All right, it seems Tom is with you on that point, so after some study, I've committed this with very minor modifications. Sorry for the long delay. I have not committed the 0002 patch, though, because I haven't studied that enough yet to know whether I think it's a good idea. Perhaps that could get its own CommitFest entry and thread, though, to separate it from this exceedingly long discussion and make it clear exactly what we're hoping to gain by that patch specifically. -- 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] B-Tree support function number 3 (strxfrm() optimization)
On Mon, Jan 19, 2015 at 3:33 PM, Robert Haas robertmh...@gmail.com wrote: All right, it seems Tom is with you on that point, so after some study, I've committed this with very minor modifications. Sorry for the long delay. I have not committed the 0002 patch, though, because I haven't studied that enough yet to know whether I think it's a good idea. Perhaps that could get its own CommitFest entry and thread, though, to separate it from this exceedingly long discussion and make it clear exactly what we're hoping to gain by that patch specifically. By the way, for those following along at home, here's an example of how this patch can help: rhaas=# create table stuff as select random()::text as a, 'filler filler filler'::text as b, g as c from generate_series(1, 100) g; SELECT 100 rhaas=# create index on stuff (a); CREATE INDEX On the PPC64 machine I normally use for performance testing, it takes about 6.3 seconds to build the index with the commit just before this one. With this commit, it drops to 1.9 seconds. That's more than a 3x speedup! Now, if I change the query that creates the table to this. rhaas=# create table stuff as select '' || random()::text as a, 'filler filler filler'::text as b, g as c from generate_series(1, 100) g; ...then it takes 10.8 seconds with or without this patch. In general, any case where the first few characters of every string are exactly identical (or only quite rarely different) will not benefit, but many practical cases will benefit significantly. Also, Peter's gone to a fair amount of work to make sure that even when the patch does not help, it doesn't hurt, either. So that's pretty 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
[HACKERS] PGCon 2015 - last day
Today is your last day to submit your PGCon 2015 proposal. -- Dan Langille http://langille.org/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)
* Robert Haas (robertmh...@gmail.com) wrote: On the PPC64 machine I normally use for performance testing, it takes about 6.3 seconds to build the index with the commit just before this one. With this commit, it drops to 1.9 seconds. That's more than a 3x speedup! Now, if I change the query that creates the table to this. rhaas=# create table stuff as select '' || random()::text as a, 'filler filler filler'::text as b, g as c from generate_series(1, 100) g; ...then it takes 10.8 seconds with or without this patch. In general, any case where the first few characters of every string are exactly identical (or only quite rarely different) will not benefit, but many practical cases will benefit significantly. Also, Peter's gone to a fair amount of work to make sure that even when the patch does not help, it doesn't hurt, either. So that's pretty cool. Wow, nice! Good work Peter! Thanks, Stephen signature.asc Description: Digital signature
