Re: [HACKERS] Why does WAL_DEBUG macro need to be defined by default?

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Fri, Oct 7, 2011 at 4:06 PM, Bruce Momjian br...@momjian.us wrote:
 I would just fix it in head.

 That just seems weird.  Either it's cheap enough not to matter (in
 which case there's no reason to revert that change at all) or it's
 expensive enough to matter (in which case presumably we don't want to
 leave it on in 9.1 for the 5 years or so it remains a supported
 release).

It needs to be reverted.  I don't understand why you didn't do that
instantly upon the mistake being pointed out to you.

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] [OT?] Time-zone database down [was: Re: timezone buglet?]

[Tom Lane]

Greg Stark st...@mit.edu writes:
 On Fri, Oct 7, 2011 at 10:10 PM, Merlin Moncure mmonc...@gmail.com wrote:
 Facts are not subject to copyright but compilations can be.

 I know it's popular for engineers to play lawyer and I've been guilty
 of it on many an occasion. But in this case I think you're all *way*
 oversimplifying the situation and I don't think it's within our ken to
 be able to come to any clear conclusion.

Well, I'm not a lawyer and I'm certainly not volunteering to be counsel
for Messrs. Olson et al.  But I can recognize a troll when I see one.
More to the point, this is an attack on a fundamental piece of open
source infrastructure, and I'm quite sure that a lot of large companies
will be stepping up to help ensure that it stays open.

I feel no need for us to do anything, until and unless there's an
adverse court ruling, which I fully expect there will not be.  And
if there is, we won't be the only ones looking for an alternative
solution.

regards, tom lane

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

Re: [HACKERS] [v9.2] Fix Leaky View Problem

[Kohei KaiGai]

2011/10/8 Noah Misch n...@leadboat.com:
 On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote:
 My preference is still also WITH(security_barrier=...) syntax.

 The arguable point was the behavior when a view is replaced without
 explicit WITH clause;
 whether we should consider it was specified a default value, or we
 should consider it means
 the option is preserved.
 If we stand on the viewpoint that object's attribute related to
 security (such as ownership,
 acl, label, ...) should be preserved, the security barrier also shall
 be preserved.
 On the other hand, we can never know what options will be added in the
 future, right now.
 Thus, we may need to sort out options related to security and not at
 DefineVirtualRelation().

 However, do we need to limit type of the options to be preserved to
 security related?
 It is the first case that object with arbitrary options can be replaced.
 It seems to me we have no matter, even if we determine object's
 options are preserved
 unless an explicit new value is provided.

 Currently, you can predict how CREATE OR REPLACE affects a given object
 characteristic with a simple rule: if the CREATE OR REPLACE statement can
 specify a characteristic, we don't preserve its existing value.  Otherwise, we
 do preserve it.  Let's not depart from that rule.

 Applying that rule to the proposed syntax, it shall not preserve the existing
 security_barrier value.  I think that is acceptable.  If it's not acceptable, 
 we
 need a different syntax -- perhaps CREATE SECURITY VIEW.

No. It also preserves the security-barrier flag, when we replace a view without
SECURITY option. The only difference is that we have no way to turn off
security-barrier flag explicitly, right now.

The major reason why I prefer reloptions rather than SECURITY option is
that allows to reuse the existing capability to store a property of relation.
It seems to me both of syntax enables to achieve the rule to preserve flags
when a view is replaced.

 Any other ideas?

 Suppose we permitted pushdown of unsafe predicates when the user can read the
 involved columns anyway, a generalization of the idea from the first paragraph
 of [1].  Would that, along with LEAKPROOF, provide enough strategies for 
 shoring
 up performance to justify removing unsafe views entirely?

The problem was that we do all the access control decision at the
executor stage,
but planner has to make a plan prior to execution. So, it was also reason why we
have tried to add LEAKPROOF flag to functions.

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

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

Re: GiST for range types (was Re: [HACKERS] Range Types - typo + NULL string constructor)

[Jeff Davis]

On Fri, 2011-10-07 at 12:54 +0400, Alexander Korotkov wrote:

 The first thing caught my eye in existing GiST code is idea of
 subtype_float. float8 has limited precision and can't respresent, for
 example, varlena values good enough. Even if we have large int8 value
 we can loose lower bits, but data distribution can be so that these
 bits are valuable. Wouldn't it better to have function like
 subtype_diff_float which returns difference between two values of
 subtype as an float? Using of such function could make penalty more
 sensible to even small difference between values, and accordingly more
 relevant.

The reason I did it that way is for unbounded ranges. With
subtype_diff_float, it's difficult for the GiST code to differentiate
between [10,) and [10,), because infinity minus anything is
infinity. But when inserting the range [100,200), the penalty for the
first one should be zero and the second one should have some positive
penalty, right?

Maybe we can still use subtype_diff_float by calling it on various pairs
of bounds to come up with a reasonable cost?

I'm open to suggestion. I'd just like to make sure that unbounded ranges
are a part of the consideration.

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] pg_upgrade - add config directory setting

[Bruce Momjian]

Bruce Momjian wrote:
 OK, I have modified the postmaster in PG 9.2 to allow output of the data
 directory, and modified pg_ctl to use that, so starting in PG 9.2 pg_ctl
 will work cleanly for config-only directories.
 
 I will now work on pg_upgrade to also use the new flag to find the data
 directory from a config-only install.  However, this is only available
 in PG 9.2, and it will only be in PG 9.3 that you can hope to use this
 feature (if old is PG 9.2 or later).  I am afraid the symlink hack will
 have to be used for several more years, and if you are supporting
 upgrades from pre-9.2, perhaps forever.
 
 I did find that it is possible to use pg_ctl -w start on a config-only
 install using this trick:
 
   su -l postgres \
 -c env PGPORT=\5432\ /usr/lib/postgresql-9.1/bin/pg_ctl start -w \
 -t 60 -s -D /var/lib/postgresql/9.1/data/ \
  -o '-D /etc/postgresql-9.1/ \
 --data-directory=/var/lib/postgresql/9.1/data/ \
 --silent-mode=true'
 
 Unfortunately pg_upgrade doesn't support the -o option which would make
 this possible for pg_upgrade.
 
 One idea would be to add -o/-O options to pg_upgrade 9.2 to allow this
 to work even with old installs, but frankly, this is so confusing I am
 not sure we want to encourage people to do things like this.  Of course,
 the symlink hack is even worse, so maybe there is some merit to this.

OK, the attached patch adds -o/-O options to pg_upgrade to mimick pg_ctl
-o, and documents the 'Gentoo method' for allowing pg_upgrade to handle
pre-9.2 upgrades for config-only installs.  I think this closes the
issue, with no backpatching required for it to work for new PG 9.2. 
Users will have to continue using the symlink method for new PG 9.1.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
new file mode 100644
index 3ab1b5c..9892b97
*** a/contrib/pg_upgrade/option.c
--- b/contrib/pg_upgrade/option.c
*** parseCommandLine(int argc, char *argv[])
*** 39,44 
--- 39,46 
  		{new-datadir, required_argument, NULL, 'D'},
  		{old-bindir, required_argument, NULL, 'b'},
  		{new-bindir, required_argument, NULL, 'B'},
+ 		{old-options, required_argument, NULL, 'o'},
+ 		{new-options, required_argument, NULL, 'O'},
  		{old-port, required_argument, NULL, 'p'},
  		{new-port, required_argument, NULL, 'P'},
  
*** parseCommandLine(int argc, char *argv[])
*** 93,99 
  
  	getcwd(os_info.cwd, MAXPGPATH);
  
! 	while ((option = getopt_long(argc, argv, d:D:b:B:cgG:kl:p:P:u:v,
   long_options, optindex)) != -1)
  	{
  		switch (option)
--- 95,101 
  
  	getcwd(os_info.cwd, MAXPGPATH);
  
! 	while ((option = getopt_long(argc, argv, d:D:b:B:cgG:kl:o:O:p:P:u:v,
   long_options, optindex)) != -1)
  	{
  		switch (option)
*** parseCommandLine(int argc, char *argv[])
*** 141,146 
--- 143,161 
  log_opts.filename = pg_strdup(optarg);
  break;
  
+ 			case 'o':
+ old_cluster.pgopts = pg_strdup(optarg);
+ break;
+ 
+ 			case 'O':
+ new_cluster.pgopts = pg_strdup(optarg);
+ break;
+ 
+ 			/*
+ 			 * Someday, the port number option could be removed and
+ 			 * passed using -o/-O, but that requires postmaster -C
+ 			 * to be supported on all old/new versions.
+ 			 */
  			case 'p':
  if ((old_cluster.port = atoi(optarg)) = 0)
  {
*** Options:\n\
*** 242,247 
--- 257,264 
-G, --debugfile=FILENAME  output debugging activity to file\n\
-k, --linklink instead of copying files to new cluster\n\
-l, --logfile=FILENAMElog session activity to file\n\
+   -o, --old-options=OPTIONS old cluster options to pass to the server\n\
+   -O, --new-options=OPTIONS new cluster options to pass to the server\n\
-p, --old-port=OLDPORTold cluster port number (default %d)\n\
-P, --new-port=NEWPORTnew cluster port number (default %d)\n\
-u, --user=NAME   clusters superuser (default \%s\)\n\
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index 0fb16ed..b7e4ea5
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*** typedef struct
*** 189,194 
--- 189,195 
  	char	   *pgdata;			/* pathname for cluster's $PGDATA directory */
  	char	   *pgconfig;		/* pathname for cluster's config file directory */
  	char	   *bindir;			/* pathname for cluster's executable directory */
+ 	char	   *pgopts;			/* options to pass to the server, like pg_ctl -o */
  	unsigned short port;		/* port number where postmaster is waiting */
  	uint32		major_version;	/* PG_VERSION of cluster */
  	char		major_version_str[64];	/* string PG_VERSION of cluster */
diff --git a/contrib/pg_upgrade/server.c 

Re: [HACKERS] patch : Allow toast tables to be moved to a different tablespace

[Kevin Grittner]

Julien Tachoires  10/07/11 10:17 AM 
 
 Here's a patch to allow TOAST tables to be moved to a different
 tablespace. This item has been picked up from the TODO list.
 Main idea is to consider that a TOAST table can have its own
 tablespace.
 
Thanks for the patch.  Please add this to the open CommitFest at:
 
https://commitfest.postgresql.org/action/commitfest_view/open
 
That will help ensure we don't lose track of it before the next
review cycle.  For more information on the review and commit process,
see this page:
 
http://wiki.postgresql.org/wiki/CommitFest
 
We are currently well in to a CF and still have patches which nobody
has yet volunteered to review.  If you could help with that, it would
be great!
 
https://commitfest.postgresql.org/action/commitfest_view/inprogress
 
-Kevin

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

Re: GiST for range types (was Re: [HACKERS] Range Types - typo + NULL string constructor)

[Alexander Korotkov]

On Sat, Oct 8, 2011 at 1:01 PM, Jeff Davis pg...@j-davis.com wrote:

 On Fri, 2011-10-07 at 12:54 +0400, Alexander Korotkov wrote:

  The first thing caught my eye in existing GiST code is idea of
  subtype_float. float8 has limited precision and can't respresent, for
  example, varlena values good enough. Even if we have large int8 value
  we can loose lower bits, but data distribution can be so that these
  bits are valuable. Wouldn't it better to have function like
  subtype_diff_float which returns difference between two values of
  subtype as an float? Using of such function could make penalty more
  sensible to even small difference between values, and accordingly more
  relevant.

 The reason I did it that way is for unbounded ranges. With
 subtype_diff_float, it's difficult for the GiST code to differentiate
 between [10,) and [10,), because infinity minus anything is
 infinity. But when inserting the range [100,200), the penalty for the
 first one should be zero and the second one should have some positive
 penalty, right?

I meant that penalty can be determined as sum of difference of old and new
bounds of range, i.e. penalty = subtype_diff_float(new_lower, old_lower)
+ subtype_diff_float(old_upper, new_upper).
When we insert [100,200) into [10,+inf), union([100,200), [10,+inf))
= [10,+inf), so penalty =  subtype_diff_float(10,10)
+  subtype_diff_float(+inf, +inf) = 0 + 0 = 0.
When we insert [100,200) into [10,), union([100,200), [10,+inf))
= [100,+inf), so penalty =  subtype_diff_float(100,10)
+  subtype_diff_float(+inf, +inf) = 99900 + 0 = 99900.

But, there are still the problem, when we'are inserting open interval when
there is no such open intervals yet. For example, we're going to insert
[0,+inf), while root page contains [0,10), [10,20), [20,30). Each penalty
will be infinity, while it seems to be better to insert it into [0,10). But,
it seems to me to be general limitation of current GiST interface, when we
have to express penalty in a single float.

--
With best regards,
Alexander Korotkov.

[HACKERS] Intermittent regression test failure from index-only scans patch

[Tom Lane]

I notice that several members of the buildfarm have been consistently
showing the same regression test failure since the index-only scans
patch went in:

*** 
/home/pgbuildfarm/workdir/HEAD/pgsql.27150/src/test/regress/expected/stats.out  
Sat Oct  8 03:20:05 2011
--- 
/home/pgbuildfarm/workdir/HEAD/pgsql.27150/src/test/regress/results/stats.out   
Sat Oct  8 12:30:55 2011
***
*** 94,100 
   WHERE st.relname='tenk2' AND cl.relname='tenk2';
   ?column? | ?column? | ?column? | ?column? 
  --+--+--+--
!  t| t| t| t
  (1 row)
  
  SELECT st.heap_blks_read + st.heap_blks_hit = pr.heap_blks + cl.relpages,
--- 94,100 
   WHERE st.relname='tenk2' AND cl.relname='tenk2';
   ?column? | ?column? | ?column? | ?column? 
  --+--+--+--
!  t| t| t| f
  (1 row)
  
  SELECT st.heap_blks_read + st.heap_blks_hit = pr.heap_blks + cl.relpages,


The diff indicates that the idx_scan count advanced but idx_tup_fetch
did not, which is not so surprising here because tenk2 hasn't been
modified in some time.  If the autovacuum daemon managed to mark it
all-visible before the stats test runs, then an index-only scan will
happen, and bingo, no idx_tup_fetch increment (because indeed no heap
tuple was fetched).

I'm inclined to fix this by changing the test to examine idx_tup_read
not idx_tup_fetch.  Alternatively, we could have the test force
enable_indexonlyscan off.  Thoughts?

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] [OT?] Time-zone database down [was: Re: timezone buglet?]

[Mark Mielke]

On 10/07/2011 11:02 PM, Greg Stark wrote:
All that said I think this is far murkier than you all seem to think. 
Copyright law is one of the most complex areas of the law and this is 
one of the least well defined parts of copyright law. 


Hi Greg:

I don't think we all think this issue is clear. Quoting relevant case 
law and considering what position to hold or what action to take is what 
I would call due diligence. If somebody wants to hire a lawyer that 
might be advisable as well.


I think wait and see whether this is a true violation is a perfectly 
valid legal position to hold and is not pretending in any way that this 
issue is clear...


--
Mark Mielkem...@mielke.cc


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

Re: [HACKERS] libpq, PQdescribePrepared - PQftype, PQfmod, no PQnullable

[Alex Goncharov]

The obvious typos:

,--- I/Alex (Thu, 06 Oct 2011 19:42:13 -0400) *
|   (may use pg_attribute.attnotnull on t1, t2, is I didn't see the 'create's.
(may use pg_attribute.attnotnull on t1, t2, if I didn't see the 'create's.
 
|   Now, for this statement, I can easily identify non-nullable columns.
Now, for this statement, I can easily identify the non-nullable columns:

-- Alex -- goncharov.a...@gmail.com --

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

[HACKERS] SET variable - Permission issues

[Josh]

Hello all,
While working on an application I have been developing for about two 
years now, I have come across a base-limitation in PostgreSQL for three 
separate problems. I was talking with other members of the community 
recently and they agreed that the issue has some merit so I thought I 
would bring it up to the list.


First a bit about my environment to better understand the issue. My 
application allows users to connect directly to the database and issue 
commands as they see fit with their own database credentials, which 
operate under various permission restrictions. While I understand this 
is an unusual way of using the database, this is simply the design of my 
application. With that being said, I believe this issue is also 
important to address for other scenarios as well (like an organization 
that offers shared hosting of a PostgreSQL instance).


The actual issue is that many important variables within the PostgreSQL 
configuration file can be overridden by any (non superuser) account 
using SET.


The first instance of this I ran into was with statement_timeout, which 
I was hoping to use to limit the amount of time a query can run for 
before the query is cancelled. This was crucial to the architecture of 
my application to assure that infinite queries did not bog down the 
system and deny service to other users running their own queries. The 
problem is that regardless of what is set in the configuration, users of 
any privilege level can override the value in the postgresql.conf with 
the SET command.


This was frustrating, but not the end of the world. I solved it by using 
a perl script that monitors the running queries and kills any process 
that runs over the allotted time. This isn't perfect though, since a 
user could issue enough commands to prevent the perl script from being 
able to function correctly.


The second variable that I have recently come to realize exists causes a 
much more serious problem. This would be the work_mem setting. This 
variable allows any user to override the system default (again using 
SET) and issue some commands that may intentionally eat up huge chunks 
of RAM.


This one seems a lot more serious as I have no way of monitoring what 
this value is set to. Also, if the attacker acts fast enough (and uses 
the statement_timeout issue as well), they can completely DoS the server.


Finally, the third variable that has caused my application grief is the 
SEED value used by the RANDOM function that can also be set by any 
user.I realize I should be using a better pseudorandom number generator 
for anything important, but since I did not know that PostgreSQL's 
random is beyond a simple case of low-entropy and is actually completely 
deterministic for all my users (because they can each SET their own SEED 
values without restrictions), I now need to rework quite a bit of code 
to resolve this.


These are the three variables which have been problematic so far in my 
application, but I am sure there are others I simply have not noticed 
(or are harmless in my application, but may cause problems in another 
scenario). As far as a solution to the problem, some discussion was made 
about adding more variables for system maximums (like for 
statement_timeout and work_mem), but this does not cover all cases (like 
SEED) and adds unnecessary limitations to the system. Some back-end 
users may actually need to alter these variables beyond what is set as 
the maximum but cannot do so without altering the configuration file.


The other option that was discussed was to add privileges for variables 
(i.e. adding GRANT and REVOKE privileges for the SET command). This is 
obviously a bit more effort but I feel it would be the best option as it 
will add a lot more flexibility into the security of PostgreSQL.


If anybody has any other solutions for how to temporarily resolve these 
problems I would definitely love to hear it but I think it is a good
idea to discuss this problem and find a solution that everyone is 
comfortable with.


Thanks for taking the time to think this over,
-Josh


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

Re: [HACKERS] Intermittent regression test failure from index-only scans patch

[Cédric Villemain]

2011/10/8 Tom Lane t...@sss.pgh.pa.us:
 I notice that several members of the buildfarm have been consistently
 showing the same regression test failure since the index-only scans
 patch went in:

 *** 
 /home/pgbuildfarm/workdir/HEAD/pgsql.27150/src/test/regress/expected/stats.out
       Sat Oct  8 03:20:05 2011
 --- 
 /home/pgbuildfarm/workdir/HEAD/pgsql.27150/src/test/regress/results/stats.out 
       Sat Oct  8 12:30:55 2011
 ***
 *** 94,100 
   WHERE st.relname='tenk2' AND cl.relname='tenk2';
   ?column? | ?column? | ?column? | ?column?
  --+--+--+--
 !  t        | t        | t        | t
  (1 row)

  SELECT st.heap_blks_read + st.heap_blks_hit = pr.heap_blks + cl.relpages,
 --- 94,100 
   WHERE st.relname='tenk2' AND cl.relname='tenk2';
   ?column? | ?column? | ?column? | ?column?
  --+--+--+--
 !  t        | t        | t        | f
  (1 row)

  SELECT st.heap_blks_read + st.heap_blks_hit = pr.heap_blks + cl.relpages,


 The diff indicates that the idx_scan count advanced but idx_tup_fetch
 did not, which is not so surprising here because tenk2 hasn't been
 modified in some time.  If the autovacuum daemon managed to mark it
 all-visible before the stats test runs, then an index-only scan will
 happen, and bingo, no idx_tup_fetch increment (because indeed no heap
 tuple was fetched).

 I'm inclined to fix this by changing the test to examine idx_tup_read
 not idx_tup_fetch.  Alternatively, we could have the test force
 enable_indexonlyscan off.  Thoughts?

No preferences, but is it interesting to add a vacuum freeze
somewhere and check expected result after index-only scan ? (for both
idx_tup_read and idx_tup_fetch)


                        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




-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

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

Re: [HACKERS] index-only scans

[Tom Lane]

I wrote:
 Robert Haas robertmh...@gmail.com writes:
 Not really.  We have detected a small performance regression when both
 heap and index fit in shared_buffers and an index-only scan is used.
 I have a couple of ideas for improving this.  One is to store a
 virtual tuple into the slot instead of building a regular tuple, but
 what do we do about tuples with OIDs?

[ that's done ]

 I was also toying with the notion of pushing the slot fill-in into the
 AM, so that the AM API is to return a filled TupleSlot not an
 IndexTuple.  This would not save any cycles AFAICT --- at least in
 btree, we still have to make a palloc'd copy of the IndexTuple so that
 we can release lock on the index page.  The point of it would be to
 avoid the assumption that the index's internal storage has exactly the
 format of IndexTuple.  Don't know whether we'd ever have any actual use
 for that flexibility, but it seems like it wouldn't cost much to
 preserve the option.

BTW, I concluded that that would be a bad idea, because it would involve
the index AM in some choices that we're likely to want to change.  In
particular it would foreclose ever doing anything with expression
indexes, without an AM API change.  Better to just define the AM's
responsibility as to hand back a tuple defined according to the index's
columns.

 Another is to avoid locking the
 index buffer multiple times - right now it locks the index buffer to
 get the TID, and then relocks it to extract the index tuple (after
 checking that nothing disturbing has happened meanwhile).  It seems
 likely that with some refactoring we could get this down to a single
 lock/unlock cycle, but I haven't figured out exactly where the TID
 gets copied out.

 Yeah, maybe, but let's get the patch committed before we start looking
 for second-order optimizations.

On reflection I'm starting to think that the above would be a good idea
because there are a couple of bogosities in the basic choices this patch
made.  In particular, I'm thinking about how we could use an index on
f(x) to avoid recalculating f() in something like

select f(x) from tab where f(x)  42;

assuming that f() is expensive but immutable.  The planner side of this
is already a bit daunting, because it's not clear how to recognize that
an index on f(x) is a covering index (the existing code is going to
think that x itself needs to be available).  But the executor side is a
real problem, because it will fail to make use of the f() value fetched
from the index anytime the heap visibility test fails.

I believe that we should rejigger things so that when an index-only scan
is selected, the executor *always* works from the data supplied by the
index.  Even if it has to visit the heap --- it will do that but just to
consult the tuple's visibility data, and then use what it got from the
index anyway.  This means we'd build the plan node's filter quals and
targetlist to reference the index tuple columns not the underlying
table's.  (Which in passing gets rid of the behavior you were
complaining about that EXPLAIN VERBOSE shows a lot of columns that
aren't actually being computed.)

In order to make this work, we have to remove the API wart that says the
index AM is allowed to choose to not return the index tuple.  And that
ties into what you were saying above.  What we ought to do is have
_bt_readpage() save off copies of the whole tuples not only the TIDs
when it is extracting data from the page.  This is no more net copying
work than what happens now (assuming that all the tuples get fetched)
since we won't need the per-tuple memcpy that occurs now in
bt_getindextuple.  The tuples can go into a page-sized workspace buffer
associated with the BTScanPosData structure, and then just be referenced
in-place in that workspace with no second copy step.

I'm inclined to do the last part immediately, since there's a
performance argument for it, and then work on revising the executor
implementation.

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] Intermittent regression test failure from index-only scans patch

[Tom Lane]

=?ISO-8859-1?Q?C=E9dric_Villemain?= cedric.villemain.deb...@gmail.com writes:
 2011/10/8 Tom Lane t...@sss.pgh.pa.us:
 The diff indicates that the idx_scan count advanced but idx_tup_fetch
 did not, which is not so surprising here because tenk2 hasn't been
 modified in some time.  If the autovacuum daemon managed to mark it
 all-visible before the stats test runs, then an index-only scan will
 happen, and bingo, no idx_tup_fetch increment (because indeed no heap
 tuple was fetched).
 
 I'm inclined to fix this by changing the test to examine idx_tup_read
 not idx_tup_fetch.  Alternatively, we could have the test force
 enable_indexonlyscan off.  Thoughts?

 No preferences, but is it interesting to add a vacuum freeze
 somewhere and check expected result after index-only scan ? (for both
 idx_tup_read and idx_tup_fetch)

This test is only trying to make sure that the stats collection
machinery is working.  I don't think that we should try to coerce things
so that it can check something as context-sensitive as whether an
index-only scan happened.  It's too fragile already --- we've seen
non-reproducible failures here many times before.

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] [v9.2] Fix Leaky View Problem

[Noah Misch]

On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote:
 2011/10/8 Noah Misch n...@leadboat.com:
  On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote:
  My preference is still also WITH(security_barrier=...) syntax.
 
  The arguable point was the behavior when a view is replaced without
  explicit WITH clause;
  whether we should consider it was specified a default value, or we
  should consider it means
  the option is preserved.
  If we stand on the viewpoint that object's attribute related to
  security (such as ownership,
  acl, label, ...) should be preserved, the security barrier also shall
  be preserved.
  On the other hand, we can never know what options will be added in the
  future, right now.
  Thus, we may need to sort out options related to security and not at
  DefineVirtualRelation().
 
  However, do we need to limit type of the options to be preserved to
  security related?
  It is the first case that object with arbitrary options can be replaced.
  It seems to me we have no matter, even if we determine object's
  options are preserved
  unless an explicit new value is provided.
 
  Currently, you can predict how CREATE OR REPLACE affects a given object
  characteristic with a simple rule: if the CREATE OR REPLACE statement can
  specify a characteristic, we don't preserve its existing value. ?Otherwise, 
  we
  do preserve it. ?Let's not depart from that rule.
 
  Applying that rule to the proposed syntax, it shall not preserve the 
  existing
  security_barrier value. ?I think that is acceptable. ?If it's not 
  acceptable, we
  need a different syntax -- perhaps CREATE SECURITY VIEW.
 
 No. It also preserves the security-barrier flag, when we replace a view 
 without
 SECURITY option. The only difference is that we have no way to turn off
 security-barrier flag explicitly, right now.
 
 The major reason why I prefer reloptions rather than SECURITY option is
 that allows to reuse the existing capability to store a property of relation.
 It seems to me both of syntax enables to achieve the rule to preserve flags
 when a view is replaced.

Yes, there are no technical barriers to implementing either behavior with either
syntax.  However, CREATE OR REPLACE VIEW ... WITH (...) has a precedent guiding
its behavior: if a CREATE OR REPLACE statement can specify a characteristic, we
don't preserve its existing value.

  Any other ideas?
 
  Suppose we permitted pushdown of unsafe predicates when the user can read 
  the
  involved columns anyway, a generalization of the idea from the first 
  paragraph
  of [1]. ?Would that, along with LEAKPROOF, provide enough strategies for 
  shoring
  up performance to justify removing unsafe views entirely?
 
 The problem was that we do all the access control decision at the
 executor stage,
 but planner has to make a plan prior to execution. So, it was also reason why 
 we
 have tried to add LEAKPROOF flag to functions.

Yes; we'd need to invalidate relevant plans in response to anything that changes
access control decisions.  GRANT and ALTER ... OWNER TO already do that, but
we'd need to cover pg_authid/pg_auth_members changes, SET ROLE, SET SESSION
AUTHORIZATION, and probably a few other things.  That might be a substantial
project in its own right.

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

Re: GiST for range types (was Re: [HACKERS] Range Types - typo + NULL string constructor)

[Jeff Davis]

On Sat, 2011-10-08 at 18:43 +0400, Alexander Korotkov wrote:

 I meant that penalty can be determined as sum of difference of old and
 new bounds of range, i.e. penalty = subtype_diff_float(new_lower,
 old_lower) + subtype_diff_float(old_upper, new_upper). 
 When we insert [100,200) into [10,+inf), union([100,200), [10,+inf))
 = [10,+inf), so penalty =  subtype_diff_float(10,10)
 +  subtype_diff_float(+inf, +inf) = 0 + 0 = 0.
 When we insert [100,200) into [10,), union([100,200), [10,
 +inf)) = [100,+inf), so penalty =  subtype_diff_float(100,10)
 +  subtype_diff_float(+inf, +inf) = 99900 + 0 = 99900.
 
OK, I like that. I will make the change.

 But, there are still the problem, when we'are inserting open interval
 when there is no such open intervals yet. For example, we're going to
 insert [0,+inf), while root page contains [0,10), [10,20), [20,30).
 Each penalty will be infinity, while it seems to be better to insert
 it into [0,10). But, it seems to me to be general limitation of
 current GiST interface, when we have to express penalty in a single
 float.

That seems like an acceptable limitation. I don't think my solution
handles it any better.

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] SET variable - Permission issues

[Tom Lane]

Josh j...@schemaverse.com writes:
 [ unhappy about users being able to freely adjust work_mem etc ]

Really, if you're letting users issue arbitrary SQL queries, there
simply isn't any way to prevent them from beating your server into
the ground.  I don't think that inserting a hack to prevent specific
configuration variables from being adjusted is going to help you
against an uncooperative user.  You'd be better off to rethink the
let them issue SQL queries directly part of your design.

The reason that the specific variables you mention (as well as some
others that bear on such things) are USERSET and not SUSET is precisely
that we are not trying to constrain the amount of resources an
uncooperative user can consume.  If we did try to do that, quite a
lot of design decisions would have to be revisited, and there would
be a number of unpleasant tradeoffs to be made.  GUC privilege levels
are just the tip of the iceberg.

regards, tom lane

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

[HACKERS] REVIEW: Optimizing box_penalty

[Kevin Grittner]

I tried to review the Optimizing box_penalty patch:
 
https://commitfest.postgresql.org/action/patch_view?id=600
 
as posted here:
 
http://archives.postgresql.org/message-id/4e088690.5080...@enterprisedb.com
 
The patch no longer applies to source code, due to other recent GiST
changes.  Parts of it were modifying functions which no longer exist.
I picked out the bits which still seemed relevant, and a patch with
that is attached.  The improvement in REINDEX time is now only about
0.25% on my machine at home, which is small enough that it could
easily be from code shifting around.  (Or such shifts might be hiding
a larger actual savings.)  In any event, this patch doesn't seem to
be justified as a performance patch, based on my benchmarks today.
 
On the other hand, this patch leaves the code a few lines shorter and
eliminates some unnecessary Datum wrapping, PG_FUNCTION_ARGS
parameters on a static function, and allows that function to be
called directly rather than using DirectFunctionCall2().  I find the
resulting code a little cleaner and easier to read.  I would prefer
to see it applied on that basis, personally.
 
Since the author is a committer, and this is a pretty minor code
style patch at this point, I'll mark it Ready for Committer and
leave it to Heikki to decide what to do with it.

-Kevin


penalty-v2.patch
Description: Binary data

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

Re: [HACKERS] libpq, PQdescribePrepared - PQftype, PQfmod, no PQnullable

[Christopher Browne]

I'll point to rather different reasoning...

Libpq is not returning tables, or relations, for that matter, but rather the
results of queries.

It is reasonable to expect to know which attributes of a table are or are
not nullable, and that is commonly available as an attribute of
pg_attribute, however...

General purpose queries are nowhere near so predetermined.  Indeed, whether
a column is nullable may not be at all visible, as the value of a column may
be computed by a function and thereby be quite opaque to static analysis.

That makes me think that guessing which attributes of a query may be null
seems like a pretty futile exercise.  At first blush, we could simplify to
PQnullable() always returning true, but that's not terribly revealing.
However, often, there mayn't be a much better solution that isn't really
tough to implement.

I'd not be keen on people putting much effort into futile exercises ; better
to work on things that are less futile.

Re: [HACKERS] Intermittent regression test failure from index-only scans patch

[Robert Haas]

On Oct 8, 2011, at 11:04 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I'm inclined to fix this by changing the test to examine idx_tup_read
 not idx_tup_fetch.  Alternatively, we could have the test force
 enable_indexonlyscan off.  Thoughts?

No preference.

Should we have another counter for heap fetches avoided?  Seems like that could 
be useful to know.

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

Re: [HACKERS] index-only scans

[Robert Haas]

On Oct 8, 2011, at 11:52 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I'm inclined to do the last part immediately, since there's a
 performance argument for it, and then work on revising the executor
 implementation.

Sounds great.  Thanks for working on this.

...Robert

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

Re: [HACKERS] Range Types - typo + NULL string constructor

[Jeff Janes]

On Sun, Oct 2, 2011 at 12:05 PM, Jeff Davis pg...@j-davis.com wrote:
 On Sun, 2011-10-02 at 11:32 +0200, Florian Pflug wrote:
 Looking at the patch, I noticed that it's possible to specify the default
 boundaries ([], [), (] or ()) per individual float type with the
 DEFAULT_FLAGS clause of CREATE TYPE .. AS RANGE. I wonder if that doesn't
 do more harm then good - it makes it impossible to deduce the meaning of
 e.g. numericrange(1.0, 2.0) without looking up the definition of 
 numericrange.

 I suggest we pick one set of default boundaries, ideally '[)' since that
 is what all the built-in canonization functions produce, and stick with it.

 Done.

 Also, made the range parsing even more like records with more code
 copied verbatim. And fixed some parsing tests along the way.

When I apply this to head, make check fails with:

  create type textrange_en_us as range(subtype=text, collation=en_US);
+ ERROR:  collation en_US for encoding SQL_ASCII does not exist

Is this a problem?  e.g. will it break the build-farm?

make check LANG=en_US does pass

Cheers,

Jeff

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

Re: [HACKERS] Range Types - typo + NULL string constructor

[Jeff Davis]

On Sat, 2011-10-08 at 12:44 -0700, Jeff Janes wrote:
 When I apply this to head, make check fails with:
 
   create type textrange_en_us as range(subtype=text, collation=en_US);
 + ERROR:  collation en_US for encoding SQL_ASCII does not exist
 
 Is this a problem?  e.g. will it break the build-farm?
 
 make check LANG=en_US does pass

Thank you for pointing that out. I think I need to remove those before
commit, but I just wanted them in there now to exercise that part of the
code.

Is there a better way to test collations like that?

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] unite recovery.conf and postgresql.conf

[Jeff Janes]

On Tue, Sep 27, 2011 at 1:33 AM, Fujii Masao masao.fu...@gmail.com wrote:

 Though there is still ongonig discussion, since there is no objection about
 the above two changes, I revised the patch that way. And I fixed the minor
 bug handling the default value of recovery_target_timeline wrongly.
 Attached is the revised version of the patch.

This patch no longer applies as it conflicts with the following commit:

commit d56b3afc0376afe491065d9eca6440b3cc7b1346
Author: Tom Lane t...@sss.pgh.pa.us
Date:   Sun Oct 2 16:50:04 2011 -0400

Restructure error handling in reading of postgresql.conf.

Cheers,

Jeff

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

[HACKERS] Schema grants for creating and dropping objects

[Marc Munro]

It seems that in order to create an object in a given schema, I must
have been granted create privilege on the schema.  But in order to drop
that object I require usage privilege.  

This means that with the right privilege settings I can create objects
that I cannot subsequently drop, or can drop an object that I cannot
recreate.

I assume this is a bug but if it's intended behaviour I'd love to hear
the rationale.

I checked this on 8.3, 8.4, 9.0 and 9.1 all with the same results.

Best regards.
__
Marc


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

Re: [HACKERS] Intermittent regression test failure from index-only scans patch

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Oct 8, 2011, at 11:04 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I'm inclined to fix this by changing the test to examine idx_tup_read
 not idx_tup_fetch.  Alternatively, we could have the test force
 enable_indexonlyscan off.  Thoughts?

 No preference.

I ended up doing it the second way (ie enable_indexonlyscan = off)
because it turns out that pg_stat_user_tables doesn't have the
idx_tup_read column --- we track that count per index, not per table.
I could have complicated the test's stats queries some more, but it
seemed quite not relevant to the goals of the test.

 Should we have another counter for heap fetches avoided?  Seems like that 
 could be useful to know.

Hm.  I'm hesitant to add another per-table (or per index?) statistics
counter because of the resultant bloat in the stats file.  But it
wouldn't be a bad idea for somebody to take two steps back and rethink
what we're counting in this area.  The current counter definitions are
mostly backwards-compatible with pre-8.1 behavior, and it seems like the
goalposts have moved enough that maybe it's time to break compatibility.

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