Re: [HACKERS] Regression tests failing if not launched on db "regression"

[Michael Paquier]

On Fri, Jan 31, 2014 at 6:09 AM, Robert Haas  wrote:
> On Thu, Jan 30, 2014 at 3:32 AM, Christian Kruse
>  wrote:
>>> For the documentation patch, I propose the attached to avoid future
>>> confusions. Comments? It might make sense to back-patch as well.
>>
>> Compiles, didn't find any typos and I think it is comprehensible.
>
> I took a look at this with a view to committing it but on examination
> I'm not sure this is the best way to proceed.  The proposed text
> documents that the tests should be run in a database called
> regression, but the larger documentation chapter of which this section
> is a part never explains how to run them anywhere else, so it feels a
> bit like telling a ten-year-old not to burn out the clutch.
>
> The bit about not changing enable_* probably belongs, if anywhere, in
> section 30.2, on test evaluation, rather than here.
And what about the attached? I have moved all the content to 30.2, and
added two paragraphs: one about the planner flags, the other about the
database used.
Regards,
-- 
Michael
*** a/doc/src/sgml/regress.sgml
--- b/doc/src/sgml/regress.sgml
***
*** 471,476  diff results/random.out expected/random.out
--- 471,510 
   not worry unless the random test fails repeatedly.
  
 
+ 
+
+ Planner Configuration parameters
+ 
+ 
+  Parameters able to change the planning of queries like enable/disable
+  flags described in  for
+  EXPLAIN should use default values.
+ 
+
+ 
+
+ Database Name
+ 
+ 
+  Regression tests should be run on a database named regression
+  to prevent failures of tests using directly or indirectly the current
+  database name in output results. The tests that would fail in this case
+  include updatable_views, foreign_data,
+  xml_map and sequence.
+ 
+ 
+ 
+  regression is the default database name for tests of core,
+  and contrib modules use contrib_regression as default.
+ 
+ 
+ 
+  Running regression tests with pg_regress
+  causes the existed database to be dropped before running the tests,
+  so be sure that there is not already a database with the same name
+  existing on server before running it.
+ 
+

  
  

-- 
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] Prefix compression of B-Tree keys

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 9:26 PM, Claudio Freire  wrote:
> Maybe not TOAST compression, but prefix compression.

I've thought about it as well. It's totally feasible, and likely
worthwhile, but a little more tricky.

> I've been wondering lately, whether a format change in the B-Tree
> could be worth the effort: store values with prefix compression. It
> would certainly help indexes of many kinds (text-valued, multi-column,
> etc...).

You can play tricks with the AM version, generally stored in the meta
page, as in the recent commit
36a35c550ac114caa423bcbe339d3515db0cd957, without breaking pg_upgrade.
Although a lot of these techniques may not actually require that kind
of additional effort.

> Now, I haven't checked if it's already done. Sorry if it is. I did
> mock around btree code a lot and don't remember any of this, but I do
> remember stuff that could be used to achieve it (specifically, all the
> index-only-scan machinery, which allows for implicit info).

I think it would make sense to do it on leaf pages, where there is
frequently a lot of redundancy. The reason that I myself wouldn't do
it first is that offhand I think that it'd be harder to come up with a
very generic infrastructure to make it work across diverse types, and
it isn't that useful where there isn't much redundancy in prefixes.
The B-Tree code doesn't really know anything about the type indexed,
and that's a useful property. But it's still definitely a useful goal.


-- 
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] [bug fix] postgres.exe fails to start on Windows Server 2012 due to ASLR

[Craig Ringer]

On 01/30/2014 08:14 PM, MauMau wrote:
>> Does this issue also occur on 9.3.2, or in 9.4 HEAD, when tested on
>> Win2k12?
> 
> I'm sure it should.  The release note doesn't have any reference to this
> issue.  Another user who reported this issue in pgsql-general
> experienced this with 9.2.4.

> In addition, Another customer reported to me that psql failed to connect
> to PostgreSQL 9.1.9 running on Windows Server 2012 R2, about once out of
> 7 times.  I believe this needs immediate fix.

I'm reasonably persuaded that there's a need for this, though IFEO (see
below) can be used at or after install-time as a workaround.

It looks like your patch sets the msbuild equivalent of the
/DYNAMICBASE:NO flag
(http://msdn.microsoft.com/en-us/library/bb384887.aspx). Is this known
to work (or be silently ignored) back to Windows SDK 7.1? (I'll test it
today and see).

I don't think we need to worry about Force ASLR
(http://support.microsoft.com/kb/2639308) as it seems it only applies
when an application loads modules, unless an admin goes playing in the
registry.


Users facing this problem can work around it without code changes by
setting IFEO in the registry to disable ASLR for postgres.exe. See:

http://msdn.microsoft.com/en-us/library/windows/desktop/ff190925(v=vs.85).aspx

http://support.microsoft.com/kb/2639308

http://msdn.microsoft.com/en-us/library/bb430720.aspx

It may be reasonable for EDB to consider releasing updated installers
that set IFEO flags to disable ASLR on postgres.exe .

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


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

Re: [HACKERS] Backup throttling

[Fujii Masao]

On Tue, Jan 21, 2014 at 1:10 AM, Antonin Houska
 wrote:
> On 01/15/2014 10:52 PM, Alvaro Herrera wrote:
>> I gave this patch a look.  There was a bug that the final bounds check
>> for int32 range was not done when there was no suffix, so in effect you
>> could pass numbers larger than UINT_MAX and pg_basebackup would not
>> complain until the number reached the server via BASE_BACKUP.  Maybe
>> that's fine, given that numbers above 1G will cause a failure on the
>> server side anyway, but it looked like a bug to me.  I tweaked the parse
>> routine slightly; other than fix the bug, I made it accept fractional
>> numbers, so you can say 0.5M for instance.
>
> Thanks.
>
>> Perhaps we should make sure pg_basebackup rejects numbers larger than 1G
>> as well.
>
> Is there a good place to define the constant, so that both backend and
> client can use it? I'd say 'include/common' but no existing file seems
> to be appropriate. I'm not sure if it's worth to add a new file there.

If there is no good place to define them, it's okay to define them
also in client side
for now.

+BASE_BACKUP [LABEL
'label'] [PROGRESS]
[FAST] [WAL]
[NOWAIT] [MAX_RATE]

It's better to add something like 'rate' just after
MAX_RATE.

+ 
+  MAX_RATE does not affect WAL streaming.
+ 

I don't think that this paragraph is required here because BASE_BACKUP is
basically independent from WAL streaming.

Why did you choose "bytes per second" as a valid rate which we can specify?
Since the minimum rate is 32kB, isn't it better to use "KB per second" for that?
If we do that, we can easily increase the maximum rate from 1GB to very large
number in the future if required.

+wait_result = WaitLatch(&MyWalSnd->latch, WL_LATCH_SET | WL_TIMEOUT
+| WL_POSTMASTER_DEATH, (long) (sleep / 1000));

If WL_POSTMASTER_DEATH is triggered, we should exit immediately like
other process does? This is not a problem of this patch. This problem exists
also in current master. But ISTM it's better to solve that together. Thought?

Regards,

-- 
Fujii Masao


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

[HACKERS] Prefix compression of B-Tree keys

[Claudio Freire]

On Thu, Jan 30, 2014 at 9:34 PM, Tom Lane  wrote:
> Peter Geoghegan  writes:
>> On more occasions than I care to recall, someone has suggested that it
>> would be valuable to do something with strxfrm() blobs in order to
>> have cheaper locale-aware text comparisons. One obvious place to do so
>> would be in indexes, but in the past that has been dismissed on the
>> following grounds:
>
>> * Index-only scans need fully formed datums to work, and strxfrm() is
>> a one-way function (or so I believe). There is no reason to think that
>> the original string can be reassembled from the blob, so clearly that
>> won't fly.
>
>> * There is a high cost to be paid in storage overhead. Even for
>> collations like "en_US.UTF-8", that can mean that the blob is as much
>> as 3-4 times larger than the original text string. Who is to say that
>> we'll come out ahead even with the savings of just doing a strcmp()
>> rather than a strcoll()?
>
> Quite aside from the index bloat risk, this effect means a 3-4x reduction
> in the maximum string length that can be indexed before getting the
> dreaded "Values larger than 1/3 of a buffer page cannot be indexed" error.
> Worse, a value insertion might well succeed, with the failure happening
> only (much?) later when that entry is chosen as a page split boundary.
>
> It's possible that TOAST compression of the strings would save you, but
> I'm not convinced of that; it certainly doesn't seem like we could
> guarantee no post-insertion failures that way.

Maybe not TOAST compression, but prefix compression.

I've been wondering lately, whether a format change in the B-Tree
could be worth the effort: store values with prefix compression. It
would certainly help indexes of many kinds (text-valued, multi-column,
etc...).

Now, I haven't checked if it's already done. Sorry if it is. I did
mock around btree code a lot and don't remember any of this, but I do
remember stuff that could be used to achieve it (specifically, all the
index-only-scan machinery, which allows for implicit info).

Granted, this isn't strxfrm, but it may provide some of the benefits
(faster comparisons because less bytes are compared?).

On Fri, Jan 31, 2014 at 1:51 AM, Peter Geoghegan  wrote:
> We're already suffering from the fact
> that strcoll() mandates a NULL-terminated string, since we have to
> copy text datums to a buffer to deal with the impedance mismatch.
> Furthermore, multi-column comparisons probably have a lot of overhead
> at present from all the indirection alone.

Which makes the above even more likely to help.


-- 
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] Making strxfrm() blobs in indexes work

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 5:05 PM, Peter Geoghegan  wrote:
> On Thu, Jan 30, 2014 at 5:04 PM, Tom Lane  wrote:
>>> That's not hard to prevent. If that should happen, we don't go with
>>> the strxfrm() datum. We have a spare IndexTuple bit we could use to
>>> mark when the optimization was applied.
>>
>> You'd need a bit per column, no?
>
> I don't think so. It would be no big deal if it was all or nothing.

I've done some more digging. It turns out that the 1977 paper "An
Encoding Method for Multifield Sorting and Indexing" describes a
technique that involves concatenating multiple column values and
comparing them using a simple strcmp(). Apparently this is something
that was in system R back in the 1970s. Here is a description of that
paper:

"Sequences of character strings with an order relation imposed between
sequences are considered. An encoding scheme is described which
produces a single, order-preserving string from a sequence of strings.
The original sequence can be recovered from the encoded string, and
one sequence of strings precedes another if and only if the encoding
of the first precedes the encoding of the second. The strings may be
variable length, without a maximum length restriction, and no symbols
need be reserved for control purposes. Hence any symbol may occur in
any string. The scheme is useful for multifield sorting, multifield
indexing, and other applications where ordering on more than one field
is important."

I think we should implement the scheme in this paper, for inner B-Tree
pages. The paper describes how lexicographic sort order must be
preserved, so it's pretty much identical to what I've described,
except it doesn't say anything about inner pages presumably because
there was no Unicode back then, and I don't think that B+Trees/B-link
trees had fully caught on yet. We're already suffering from the fact
that strcoll() mandates a NULL-terminated string, since we have to
copy text datums to a buffer to deal with the impedance mismatch.
Furthermore, multi-column comparisons probably have a lot of overhead
at present from all the indirection alone.

-- 
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] pgindent behavior we could do without

[Tom Lane]

Bruce Momjian  writes:
> OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
> blank lines above #elif/#else/#endif, and therefore removed the special
> case code from pgindent.

> You will need to download version 1.3 of pg_bsd_indent for this to work,
> and pgindent will complain if it doesn't find the right pg_bsd_indent
> version.

Cool.

> Do we need to go an clean up any places where pgindent has suppressed
> blank lines above #elif/#else/#endif in the past?

Not sure it's worth making a massive change for this, but I appreciate the
fact that pgindent won't mess up such code in the future.

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] Review: tests for client programs

[Craig Ringer]

On 01/31/2014 02:50 AM, Pavel Stehule wrote:
> 
> 5. I didn't test it on windows

I guess that's my cue. I'll be home later today, and will take a look at
it on my Windows test setup.

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


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

Re: [HACKERS] updated emacs configuration

[Bruce Momjian]

On Thu, Jan 30, 2014 at 03:36:48PM -0500, Bruce Momjian wrote:
> On Thu, Jan 30, 2014 at 03:32:27PM -0500, Robert Haas wrote:
> > Or this:
> > 
> > - mp_int_copy(a, b); /* ok: 0 <= r < b */
> > - mp_int_copy(&q, a); /* ok: q <= a   */
> > + mp_int_copy(a, b); /* ok:  0 <= r < b */
> > + mp_int_copy(&q, a); /* ok:  q <= a */
> > 
> > I do agree with you some of the changes-after-periods look like
> > improvements; what I meant to say is that there is an awful lot of
> > churn in this patch that is unrelated to that particular point.
> 
> Let me work on a entab patch that does replacements in comments only
> after periods and post the results.  I should be done in an hour.

OK, eight hours later, I have the results for only removing tabs after
periods in comments:

http://momjian.us/expire/entab_comment.v2.cdiff
http://momjian.us/expire/entab_comment.v2.pdiff
http://momjian.us/expire/entab_comment.v2.udiff

The diff line counts are:

64356 entab_comment.v2.cdiff
17327 entab_comment.v2.pdiff
35453 entab_comment.v2.udiff

It represents 3903 lines changed.

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

  + Everyone has their own god. +


-- 
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] Issue with PGC_BACKEND parameters

[Amit Kapila]

On Fri, Jan 31, 2014 at 2:01 AM, Andrew Dunstan  wrote:
>
> On 01/30/2014 03:12 PM, Tom Lane wrote:
>>
>> Andrew Dunstan  writes:
>>>
>>> On 12/22/2013 11:30 PM, Amit Kapila wrote:
>>> - * backend start.
>>> + * backend start. However for windows, we need to
>>> process
>>> + * config file during backend start for non-default
>>> parameters,
>>> + * so we need to allow change of PGC_BACKEND during
>>> backend
>>> + * startup.
>>> */
>>> -if (IsUnderPostmaster)
>>> +if (IsUnderPostmaster && !IsInitProcessingMode())
>>>return -1;
>>>}
>>> I think this change looks OK.
>>
>> The comment is pretty awful, since this is neither Windows-specific nor
>> a read of the config file.  Perhaps more like "However, in EXEC_BACKEND
>> builds we load nondefault settings from the CONFIG_EXEC_PARAMS file during
>> backend start.  In that situation we should accept PGC_SIGHUP
>> settings, so as to have the same value as if we'd forked from the
>> postmaster."

Changed as per suggestion.

>> Also, I think that the extra test should only be made #ifdef EXEC_BACKEND,
>> so as to minimize the risk of breaking things.

Agreed and changed the patch as per suggestion.

>>Not that this isn't pretty
>> darn fragile anyway; I think testing IsInitProcessingMode here is a very
>> random way to detect this case.  I wonder if it'd be better to pass down
>> an explicit flag indicating that we're doing read_nondefault_variables().

My first idea was to add a parameter, but set_config_option is getting called
from multiple places and this case doesn't seem to be generic enough to
add a parameter to commonly used function, so I found another way of doing
it. I agree that adding a new parameter would be a better fix, but just seeing
the places from where it get called, I thought of doing it other way, however
if you feel strongly about it, I can change the patch to pass a new parameter
to set_config_option().

>> If we don't do that, maybe an Assert(IsInitProcessingMode()) in
>> read_nondefault_variables() would be a good thing.

Added Assert in read_nondefault_variables().

>
>
>
> OK, I've added your comment to the commitfest item and marked it as "Waiting
> on Author".

Thanks for Review.

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


set_guc_backend_params_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] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jan 30, 2014 at 01:52:55PM -0500, Bruce Momjian wrote:
> On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> > >> I assert that we should simply remove both of these bits of code, as
> > >> just about every committer on the project is smarter about when to use
> > >> vertical whitespace than this program is.
> > 
> > > OK, I have developed the attached patch that shows the code change of
> > > removing the test for #else/#elif/#endif.  You will see that the new
> > > output has odd blank lines for cases like:
> > 
> > >   #ifndef WIN32
> > >   static intcopy_file(const char *fromfile, const char *tofile, bool 
> > > force);
> > > -->
> > >   #else
> > >   static intwin32_pghardlink(const char *src, const char *dst);
> > > -->
> > >   #endif
> > 
> > Hm.  So actually, that code is trying to undo excess vertical whitespace
> > that something earlier in pgindent added?  Where is that happening?
> 
> I am afraid it is _inside_ BSD indent, and if ever looked at that code,
> you would not want to go in there.  :-O

OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
blank lines above #elif/#else/#endif, and therefore removed the special
case code from pgindent.

You will need to download version 1.3 of pg_bsd_indent for this to work,
and pgindent will complain if it doesn't find the right pg_bsd_indent
version.

Do we need to go an clean up any places where pgindent has suppressed
blank lines above #elif/#else/#endif in the past?

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

  + Everyone has their own god. +


-- 
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] Regarding google summer of code

[Amit Langote]

Hi Anirudh,

On Fri, Jan 31, 2014 at 12:23 PM, Anirudh  wrote:
> Hello everyone,
>
> My name is Anirudh Subramanian and I am a graduate student in Computer
> Science. I would like to participate in Google Summer of Code and would like
> to contribute to postgresql. I am not familiar with the postgresql codebase
> yet but will surely get started in the near future. Right now as part of my
> coursework I am building  Is there any list of project ideas that have been
> decided for Google Summer of Code 2014 that I can look at.
>
> Cheers,
>

Check out this page:

http://www.postgresql.org/developer/summerofcode/

--
Amit


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

[HACKERS] Regarding google summer of code

[Anirudh]

Hello everyone,

My name is Anirudh Subramanian and I am a graduate student in Computer
Science. I would like to participate in Google Summer of Code and would
like to contribute to postgresql. I am not familiar with the postgresql
codebase yet but will surely get started in the near future. Right now as
part of my coursework I am building  Is there any list of project ideas
that have been decided for Google Summer of Code 2014 that I can look at.

Cheers,

Anirudh Subramanian

Re: [HACKERS] [Review] inherit support for foreign tables

[Etsuro Fujita]

(2014/01/28 22:01), Etsuro Fujita wrote:

(2014/01/27 21:49), Shigeru Hanada wrote:

Is it too big change that making ANALYZE command to handle foreign
tables too even if no table name was specified?  IIRC, performance
issue was the reason to exclude foreign tables from auto-analyze
processing.  ANALYZEing large database contains local huge data also
takes long time.  One idea to avoid unexpected long processing is to
add option to foreign tables to mark it as "not-auto-analyzable".


Maybe I didn't express my idea clearly.  Sorry for that.

I don't think that we now allow the ANALYZE command to handle foreign
tables when no table name is specified with the command.  I think that
we allow the ANALYZE command to handle an inheritance tree that includes
foreign tables when the name of the parent table is specified, without
ignoring such foreign tables in the caluculation.  ISTM it would be
possible to do so if we introduce a new parameter, say, vac_mode, which
indicates wether vacuum() is called with a specific table or not.

I'll try to modify the ANALYZE command to do so on top of you patch.


Done on top of your patch, foreign_inherit.patch, not the latest 
version, foreign_inherit-v2.patch.  As I proposed, an inheritance tree 
that includes foreign tables is now ANALYZEd, without ignoring such 
foreign tables in the inherited-stats computation, if the name of the 
parent table is specified with the ANALYZE command.  (That has been done 
by a small modification of analyze.c, thanks to [1].)  The ANALYZE 
command with no tablename or the autovacuum worker skips the 
inherited-stats computation itself for inheritance trees that includes 
foreign tables, which is different from the original patch.  To 
distinguish the ANALYZE-with-a-tablename command from the others (ie, 
the ANALYZE-with-no-tablename command or the autovacuum worker), I've 
introduced a new parameter, vacmode, in vacuum(), and thus called 
analyze_rel() with that parameter.  Attached is the modified patch. 
Could you review the patch?


Thanks,

[1] 
http://www.postgresql.org/message-id/e1sgfoo-0006zf...@gemulon.postgresql.org


Best regards,
Etsuro Fujita
*** a/doc/src/sgml/ddl.sgml
--- b/doc/src/sgml/ddl.sgml
***
*** 258,263  CREATE TABLE products (
--- 258,274 
 even if the value came from the default value definition.

  
+   
+
+ Note that constraints can be defined on foreign tables too, but such
+ constraints are not enforced on insert or update.  Those constraints are
+ "assertive", and work only to tell planner that some kind of optimization
+ such as constraint exclusion can be considerd.  This seems useless, but
+ allows us to use foriegn table as child table (see
+ ) to off-load to multiple servers.
+
+   
+ 

 Check Constraints
  
***
*** 2021,2028  CREATE TABLE capitals (

  

!In PostgreSQL, a table can inherit from
!zero or more other tables, and a query can reference either all
 rows of a table or all rows of a table plus all of its descendant tables.
 The latter behavior is the default.
 For example, the following query finds the names of all cities,
--- 2032,2039 

  

!In PostgreSQL, a table or foreign table can
!inherit from zero or more other tables, and a query can reference either 
all
 rows of a table or all rows of a table plus all of its descendant tables.
 The latter behavior is the default.
 For example, the following query finds the names of all cities,
*** a/doc/src/sgml/ref/alter_foreign_table.sgml
--- b/doc/src/sgml/ref/alter_foreign_table.sgml
***
*** 42,47  ALTER FOREIGN TABLE [ IF EXISTS ] namecolumn_name 
SET ( attribute_option = 
value [, ... ] )
  ALTER [ COLUMN ] column_name 
RESET ( attribute_option [, ... ] )
  ALTER [ COLUMN ] column_name 
OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ])
+ INHERIT parent_table
+ NO INHERIT parent_table
  OWNER TO new_owner
  OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ])
  
***
*** 178,183  ALTER FOREIGN TABLE [ IF EXISTS ] name
  
 
+ INHERIT parent_table
+ 
+  
+   This form adds the target foreign table as a new child of the specified
+   parent table.
+  
+ 
+
+ 
+
+ NO INHERIT parent_table
+ 
+  
+   This form removes the target foreign table from the list of children of
+   the specified parent table.
+  
+ 
+
+ 
+
  OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ] )
  
   
***
*** 306,311  ALTER FOREIGN TABLE [ IF EXISTS ] name

   
+ 
+  
+   parent_name
+   
+
+ A parent table to associate or de-associate with this foreign table.
+
+   
+  
  
   
  
*** a/doc/src/sgml/ref/create_foreign_table.sgml
--- b/doc/src/sgml/ref/create_foreign_table.sgml
***
*** 22,2

Re: [HACKERS] Shave a few instructions from child-process startup sequence

[Peter Eisentraut]

On 1/20/14, 8:08 PM, Alvaro Herrera wrote:
> Peter Eisentraut escribió:
>> src/backend/postmaster/postmaster.c:2255: indent with spaces.
>> +else
>> src/backend/postmaster/postmaster.c:2267: indent with spaces.
>> +break;
> 
> I just checked the Jenkins page for this patch:
> http://pgci.eisentraut.org/jenkins/job/postgresql_commitfest_world/243/
> just to make sure I can figure out what it means.  You reported it as
> "build unstable" in the commitfest entry:
> https://commitfest.postgresql.org/action/patch_view?id=1277
> However, looking at Jenkins, I couldn't figure out what the problem is.

In this case, it was the whitespace violation.  (Yeah, I'm constantly
debating with myself whether it's worth reporting that, but at the
moment I'm still on the side of the fence that wants to make people
submit clean patches.)

In general, it's sometimes a bit hard to find out what caused the build
to fail.  Jenkins can detect and report that for standard tools (e.g.,
compiler warnings, JUnit test results), but not for our custom test
tools.  Another issue is that the build is running with make -k, so the
issue could be somewhere in the middle of the build log.  I'm exploring
new plugins to improve that, as it's a significant problem.



-- 
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] Prohibit row-security + inheritance in 9.4?

[Craig Ringer]

On 01/31/2014 09:01 AM, Stephen Frost wrote:
> I don't see where this follows at all- clearly, you already get a subset
> of rows from the child than if you queried the parent because there are
> other children.

Er, what? I don't see what you're saying here.

Currently, when you query the parent, you see rows from other children
(superset). You don't ever _not_ see rows from the child that you would
see when querying the child directly.

> If you are first playing with inheritance in PG then it
> might seem odd for that to be the case. Ditto for what you describe
> where the child returns more rows than the parent, but these things need
> to simply be documented as "this is how it works" for those cases where
> both are reasonable possibilities and we need to pick one. 

I'm increasingly inclined to agree. Everything else is too messy, and
creates inflexible limitations for users.

> Personally, I don't see the suggestion that we filter rows accessed via
> the child based on quals of the parent as making any sense.

Neither do I; that (point 4, original post) was pretty much a way to
make the other approaches look better by comparison ;-)

> I feel the
> same about applying child quals when querying through the parent as we
> don't apply GRANT permissions that way. Using the parent and using the
> child are two different paths by which to access the data and which you
> are using is what drives what you will see.

That's a reasonable way to explain it, and consistent with the
privileges model already used for inheritance.

> Is there a case which can't be implemented if the two are independent as
> I am describing?  There are cases which can NOT be implemented if we
> force the two paths to be handled identically but I feel the approach
> where we keep them independently managed is flexible to allow the other
> cases if people want them. 

The only case prevented is one where access to the child via the parent
shows rows that the parent's row-security qual would hide, because the
child's qual doesn't.

Personally I think that's ugly anyway; I don't want to support that, and
have only been looking at it because it'd solve the consistency issues.

Since the user can achieve this with:

SELECT ...
FROM ONLY parent
UNION ALL
SELECT ...
FROM ONLY child1

I think it's fine to just apply the parent qual to all children.

> There's another bit of fun too: If you have a policy on a child, and
> query the child via the parent, should the child policy be applied?
> 
> 
> No!  We do not do that for GRANT and I do not see doing it for row
> security either. 

If we're approaching this as "different entry point, different policy",
that makes sense, and I'm increasingly pesuaded by that view of things.

> Treating row-security checks as permission checks, that'd make this
> consistent. The difference is that you get a nice error telling you
> what's going on currently, not a potentially WTF-y different resultset.
> 
> 
> I understand where you're coming from but this strikes me as a
> documentation/definition issue and not really a cause for concern or
> against POLA. These are complex and important topics that anyone who
> cares about security needs to understand. 

I'm happy with that. It's clear that there isn't going to be any way to
do this that doesn't result in _some_ kind of surprising behaviour, so
it's just a matter of being clear about where the astonishment lies.

So what we're talking about here (conveniently, exactly what's currently
impemented) is to:

Apply the policy of the relation actually named in the query before
inheritance expansion. If the relation has children expanded during
planning, allow the parent policy to be copied to those children. The
children are _not_ checked for their own row-security policies when the
appendrel is created, and any child policies are _not_ applied.

That's consistent with how security barrier views (and views in general)
work, and it means that the policy on a relation is applied consistently
to all rows, including rows from child relations. As we discussed, it's
also consistent with relation-level GRANTs for access to relations. The
trade-off is that it creates inconsistent views of the contents of the
data depending on whether you query via a parent, or query a child
directly, and it means that child policies are ignored when querying via
a parent relation.

Since that's what I've already written, I'm happy with that ;-)

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


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

Re: [HACKERS] Making strxfrm() blobs in indexes work

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 3:49 PM, Peter Geoghegan  wrote:
> So ISTM that we could come up with an infrastructure, possibly just
> for insertion scanKeys (limiting the code footprint of all of this) in
> order to inner-page-process datums at this juncture, and store a blob
> instead, for later savings on walking the tree. I believe that this
> technique has applicability beyond strxfrm(), and a type-naive
> infrastructure could be developed to support it, with a degree of
> complexity not too far in excess of, say, the SortSupport
> infrastructure. The realization that we don't really need to recover
> the original information directly from the inner pages gives us scope
> to push things in several useful directions, I suspect.

Techniques around B-Trees that are useful for common workloads may
have problems around something that rhymes with "combatants", since it
stands to reason that many were developed within industry, and some of
the major vendors are known to be very combative. This whole area
seems safe, though. After all, Singleton wrote in a 1969 paper
"ALGORITHM 347: AN EFFICIENT ALGORITHM FOR SORTING WITH MINIMAL
STORAGE" that ACM members can download:

"This FORTRAN subroutine was tested on a CDC 6400 computer. For random
uniform numbers, sorting times divided by n log^2 n were nearly
constant at 20.2 X 10^-6 for 100 < n < 10,000, with a time of 0.202
seconds for 1000 items. This subroutine was also hand-compiled for the
same computer to produce a more efficient machine code. In this
version the constant of proportionality was 5.2 X 10^-6, with a time
of 0.052 seconds for 1000 items. In both cases, integer comparisons
were used to order normalized floating-point numbers."

This looks very much like prior art for our purposes. I'm pretty sure
that the problem was back then they didn't have dedicated
floating-point units, so they had to do what a contemporary embedded
systems engineer would call floating point emulation, at great
expense. Better to avoid a more expensive comparisons when you can.

-- 
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] GiST support for inet datatypes

[Andreas Karlsson]

On 01/23/2014 11:22 PM, Emre Hasegeli wrote:
> inet-gist
> -
> I realized that create extension btree_gist fails with the patch.


ERROR:  could not make operator class "gist_inet_ops" be default for type inet
DETAIL:  Operator class "inet_ops" already is the default.

I think making the new operator class default makes more sense. Name conflict
can be a bigger problem. Should I rename the new operator class?


I agree that the new operator class should be the default one, it is 
more useful and also not buggy. No idea about the naming.



The only thing is that I think your index would do poorly on the case where
all values share a prefix before the netmask but have different values after
the netmask, since gist_union and gist_penalty does not care about the bits
after the netmask. Am I correct? Have you thought anything about this?


Yes, I have tried to construct the index with ip_maxbits() instead of ip_bits()
but I could not make it work with the cidr type. It can be done by adding
operator as a parameter for union, penalty and picksplit functions on the
GiST framework. I am not sure it worths the trouble. It would only help
the basic comparison operators and it would slow down other operators because
network length comparison before network address would not be possible for
the intermediate nodes. I mentioned this behavior of the operator class on
the paragraph added to the documentation.


I think this is fine since it does not seem like a very useful case to 
support to me. Would be worth doing only if it is easy to do.


A separate concern: we still have not come to a decision about operators 
for inet here. I do not like the fact that the operators differ between 
ranges and inet. But I feel this might be out of scope for this patch.


Does any third party want to chime in with an opinion?

Current inet/cidr
<>contains
>>=   contains or equals

Range types
@>   contains range
<@   range is contained by
&&  overlap (have points in common)
<>strictly right of


inet-selfuncs
-

Here I have to honestly admit I am not sure if I can tell if your
selectivity function is correct. Your explanation sounds convincing and the
general idea of looking at the histogram is correct. I am just have no idea
if the details are right.


I tried to improve it in this version. I hope it is more readable now. I used
the word inclusion instead of overlap, made some changes on the algorithm
to make it more suitable to the other operators.

Using the histogram for inclusion which is originally for basic comparison,
is definitely not correct. It is an empirical approach. I have tried several
versions of it, tested them with different data sets and thought it is better
than nothing.


Thanks for the updates. The code in this version is cleaner and easier 
to follow.


I am not convinced of your approach to calculating the selectivity from 
the histogram. The thing I have the problem with is the clever trickery 
involved with how you handle different operator types. I prefer the 
clearer code of the range types with how calc_hist_selectivity_scalar is 
used. Is there any reason for why that approach would not work here or 
result in worse code?


I have attached a patch where I improved the language of your comment 
describing the workings of the selectivity estimation. Could you please 
check it so I did not change the meaning of any of it?


I see from the tests that you still are missing selectivity functions 
for operators, what is your plan for this?


--
Andreas Karlsson
diff --git a/src/backend/utils/adt/network_selfuncs.c b/src/backend/utils/adt/network_selfuncs.c
new file mode 100644
index 87a7390..3b99afe
*** a/src/backend/utils/adt/network_selfuncs.c
--- b/src/backend/utils/adt/network_selfuncs.c
*** inet_opr_order(Oid operator)
*** 172,181 
   *
   * Calculates histogram selectivity for the subnet inclusion operators of
   * the inet type. In the normal case, the return value is between 0 and 1.
!  * It should be corrected with MVC selectivity and null fraction. If
   * the constant is less than the first element or greater than the last
   * element of the histogram the return value will be 0. If the histogram
!  * does not available, the return value will be -1.
   *
   * The histogram is originally for the basic comparison operators. Only
   * the common bits of the network part and the lenght of the network part
--- 172,181 
   *
   * Calculates histogram selectivity for the subnet inclusion operators of
   * the inet type. In the normal case, the return value is between 0 and 1.
!  * It should be corrected with the MVC selectivity and null fraction. If
   * the constant is less than the first element or greater than the last
   * element of the histogram the return value will be 0. If the histogram
!  * is not available, the return value will be -1.

Re: [HACKERS] [PATCH] pg_sleep(interval)

[Vik Fearing]

On 01/30/2014 09:48 PM, Robert Haas wrote:
> On Thu, Oct 17, 2013 at 9:11 AM, Vik Fearing  wrote:
>> On 10/17/2013 02:42 PM, Robert Haas wrote:
>>> On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing  wrote:
 On 10/17/2013 10:03 AM, Fabien COELHO wrote:
> My guess is that it won't be committed if there is a single "but it
> might break one code or surprise one user somewhere in the universe",
> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
> liner is really akin to "rejected".
 I have attached here an entirely new patch (new documentation and
 everything) that should please everyone.  It no longer overloads
 pg_sleep(double precision) but instead add two new functions:

  * pg_sleep_for(interval)
  * pg_sleep_until(timestamp with time zone)

 Because it's no longer overloading the original pg_sleep, Robert's
 ambiguity objection is no more.

 Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');

 If people like this, I'll reject the current patch and add this one to
 the next commitfest.
>>> I find that naming relatively elegant.  However, you've got to
>>> schema-qualify every function and operator used in the definitions, or
>>> you're creating a search-path security vulnerability.
>>>
>> Good catch.  Updated patch attached.
> Committed.

Thanks!

-- 
Vik



-- 
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] jsonb and nested hstore

[Merlin Moncure]

On Thu, Jan 30, 2014 at 4:52 PM, Andrew Dunstan  wrote:
>
> On 01/30/2014 07:21 PM, Merlin Moncure wrote:
>> postgres=# select hstore(row(1, array[row(1, array[row(1,
>> array[1,2])::z])::y])::x);
>>  hstore
>>
>> --
>>   "a"=>1,
>> "b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\"\\\"{1,2}\\\"\\\")\\\"\\\"}\\\")\"}"
>>
>> here, the output escaping has leaked into the internal array
>> structures.  istm we should have a json expressing the internal
>> structure.
>
> What has this to do with json at all? It's clearly a failure in the hstore()
> function.

yeah -- meant to say 'hstore' there.  Also I'm not sure that it's
'wrong'; it's just doing what it always did.  That brings up another
point: are there any interesting cases of compatibility breakage?  I'm
inclined not to care about this particular case  though...

>> array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
>> ERROR:  malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1,
>> 2}"
>>
>> yikes. The situation as I read it is that (notwithstanding my comments
>> upthread) there is no clean way to slide rowtypes to/from hstore and
>> jsonb while preserving structure.  IMO, the above query should work
>> and the populate function record above should return the internally
>> structured row object, not the text escaped version.
>
>
>
> And this is a failure in populate_record().
>
> I think we possibly need to say that handling of nested composites and
> arrays is an area that needs further work. OTOH, the refusal of
> json_populate_record() and json_populate_recordset() to handle these in 9.3
> has not generated a flood of complaints, so I don't think it's a tragedy,
> just a limitation, which should be documented if it's not already. (And of
> course hstore hasn't handled nested anything before now.)
>
> Meanwhile, maybe Teodor can fix the two hstore bugs shown here.

While not a "flood", there certainly have been complaints.  See
http://postgresql.1045698.n5.nabble.com/Best-way-to-populate-nested-composite-type-from-JSON-td5770566.html
http://osdir.com/ml/postgresql-pgsql-general/2014-01/msg00205.html

But, if we had to drop this in the interests of time I'd rather see
the behavior cauterized off so that it errored out 'not supported' (as
json_populate does) that attempt to implement the wrong behavior.

merlin


-- 
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] Making strxfrm() blobs in indexes work

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 5:04 PM, Tom Lane  wrote:
>> That's not hard to prevent. If that should happen, we don't go with
>> the strxfrm() datum. We have a spare IndexTuple bit we could use to
>> mark when the optimization was applied.
>
> You'd need a bit per column, no?

I don't think so. It would be no big deal if it was all or nothing.


-- 
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] Making strxfrm() blobs in indexes work

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 4:45 PM, Peter Geoghegan  wrote:
> So we consider the
> appropriateness of a regular strcoll() or a strxfrm() in all contexts
> (in a generic and extensible manner, but that's essentially what we
> do). I'm not too discouraged by this restriction, because in practice
> it won't come up very often.

I meant: We consider the appropriateness of a strxfrm() + strcmp()
against the pre-strfxfrm()'d scanKey datum, when the optimization is
not in force, as against just a plain strcmp() when it is.


-- 
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] Making strxfrm() blobs in indexes work

[Tom Lane]

Peter Geoghegan  writes:
> On Thu, Jan 30, 2014 at 4:34 PM, Tom Lane  wrote:
>> Quite aside from the index bloat risk, this effect means a 3-4x reduction
>> in the maximum string length that can be indexed before getting the
>> dreaded "Values larger than 1/3 of a buffer page cannot be indexed" error.
>> Worse, a value insertion might well succeed, with the failure happening
>> only (much?) later when that entry is chosen as a page split boundary.

> That's not hard to prevent. If that should happen, we don't go with
> the strxfrm() datum. We have a spare IndexTuple bit we could use to
> mark when the optimization was applied.

You'd need a bit per column, no?

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] Prohibit row-security + inheritance in 9.4?

[Stephen Frost]

On Thursday, January 30, 2014, Craig Ringer  wrote:
>
> > This strikes me as a bit odd- isn't this against how we handle the
> > GRANT system when it comes to inheiritance?  That is to say- if
> > you have access to query the parent, then you'll get rows back from
> > the child and I believe everyone feels that makes perfect sense.
>
> Thanks for taking the time to look at this.
>
> I agree that it's odd. The trouble is that there's a conflict between
> two "makes perfect sense"s here.
>
> I expect to get all rows, including inherited rows, filtered by a
> parent's predicate back when you query the parent. Fair enough.
>
> I also expect that when I query a child table, I'll see the same rows
> I see when I query it via the parent table. Especially in the common
> case of an empty parent table.


I don't see where this follows at all- clearly, you already get a subset of
rows from the child than if you queried the parent because there are other
children. If you are first playing with inheritance in PG then it might
seem odd for that to be the case. Ditto for what you describe where the
child returns more rows than the parent, but these things need to simply be
documented as "this is how it works" for those cases where both are
reasonable possibilities and we need to pick one.

Personally, I don't see the suggestion that we filter rows accessed via the
child based on quals of the parent as making any sense. I feel the same
about applying child quals when querying through the parent as we don't
apply GRANT permissions that way. Using the parent and using the child are
two different paths by which to access the data and which you are using is
what drives what you will see.


> One of these has to give, we can't have both.


I agree that we can't do both.


> I'm speaking with an outside party who has an inheritance-based data
> model they want to apply row-security to. Hopefully that'll shed some
> light on practical implications.


Is there a case which can't be implemented if the two are independent as I
am describing?  There are cases which can NOT be implemented if we force
the two paths to be handled identically but I feel the approach where we
keep them independently managed is flexible to allow the other cases if
people want them.


> There's another bit of fun too: If you have a policy on a child, and
> query the child via the parent, should the child policy be applied?


No!  We do not do that for GRANT and I do not see doing it for row security
either.


> The immediate thought is "obviously" - but then, often the child and
> parent policies are going to be the same, in which case it's a
> duplicate filter step. Here security trumps efficiency; I think we
> just apply both, and leave proving they're identical and skipping the
> child's as an optimization problem for later.


No, if you apply both then you reduce the ability of the user to set it up
to meet their needs. Allowing these paths to be managed independent allows
more flexibility. If it adds a bit of bookkeeping for users who wish to
allow access to both the child and the parent directly then tools can be
written to manage that.


> > ... Just how our existing GRANT system works.
>
> True; it's possible to be able to query the parent, but not its
> children, at present.


It was actually changed not all that long ago to be this way because having
a query against the parent *sometimes* fail when hitting a certain child
table was not sensible.


> Treating row-security checks as permission checks, that'd make this
> consistent. The difference is that you get a nice error telling you
> what's going on currently, not a potentially WTF-y different resultset.


I understand where you're coming from but this strikes me as a
documentation/definition issue and not really a cause for concern or
against POLA. These are complex and important topics that anyone who cares
about security needs to understand.


> > If you want to constrain the children in the same way as the
> > parent, then the user can add to the row-security on the children
> > to match the parent.
>
> Yes, with the caveat mentioned above that this will cause multiple
> nested row-security policies when querying the parent; each child's
> policy will get applied, *and* the parent's policy will get applied.
> Inefficient and ugly if they're the same.


No. You misunderstand. When it comes to querying the parent only the
parents would apply. Yes, this may mean the parent has to have more than it
would otherwise but there would not need to be any redundant evaluation-
perhaps redundant definition, but that's not the same.


> > If the user wants to have one view for the entire inheiritance
> > tree then they need to only implement the row-security on the
> > parent and not grant any access for users on the children (or, if
> > they're paranoid, add row-security to the children which are
> > essentially deny-all).
>
> That works if the children are used for partitioning/inheritance,
> where 

Re: [HACKERS] inherit support for foreign tables

[Robert Haas]

On Thu, Jan 30, 2014 at 5:05 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane  wrote:
>>> I think this is totally misguided.  Who's to say that some weird FDW
>>> might not pay attention to attstorage?  I could imagine a file-based
>>> FDW using that to decide whether to compress columns, for instance.
>>> Admittedly, the chances of that aren't large, but it's pretty hard
>>> to argue that going out of our way to prevent it is a useful activity.
>
>> I think that's a pretty tenuous position.  There are already
>> FDW-specific options sufficient to let a particular FDW store whatever
>> kinds of options it likes; letting the user set options that were only
>> ever intended to be applied to tables just because we can seems sort
>> of dubious.  I'm tempted by the idea of continuing to disallow SET
>> STORAGE on an unvarnished foreign table, but allowing it on an
>> inheritance hierarchy that contains at least one real table, with the
>> semantics that we quietly ignore the foreign tables and apply the
>> operation to the plain tables.
>
> [ shrug... ] By far the easiest implementation of that is just to apply
> the catalog change to all of them.  According to your assumptions, it'll
> be a no-op on the foreign tables anyway.

Well, there's some point to that, too, I suppose.  What do others think?

-- 
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] jsonb and nested hstore

[Andrew Dunstan]

On 01/30/2014 07:21 PM, Merlin Moncure wrote:


Something seems off:

postgres=# create type z as (a int, b int[]);
CREATE TYPE
postgres=# create type y as (a int, b z[]);
CREATE TYPE
postgres=# create type x as (a int, b y[]);
CREATE TYPE

-- test a complicated construction
postgres=# select row(1, array[row(1, array[row(1, array[1,2])::z])::y])::x;
  row
-
  
(1,"{""(1,\\""{\\""\\""(1,\\""\\""{1,2}\\""\\"")\\""\\""}\\"")""}")

postgres=# select hstore(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x);
 hstore
--
  "a"=>1, 
"b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\"\\\"{1,2}\\\"\\\")\\\"\\\"}\\\")\"}"

here, the output escaping has leaked into the internal array
structures.  istm we should have a json expressing the internal
structure.


What has this to do with json at all? It's clearly a failure in the 
hstore() function.




   It does (weirdly) map back however:

postgres=# select populate_record(null::x, hstore(row(1, array[row(1,
array[row(1, array[1,2])::z])::y])::x));
populate_record
-
  
(1,"{""(1,\\""{\\""\\""(1,\\""\\""{1,2}\\""\\"")\\""\\""}\\"")""}")


OTOH, if I go via json route:

postgres=# select row_to_json(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x);
   row_to_json
---
  {"a":1,"b":[{"a":1,"b":[{"a":1,"b":[1,2]}]}]}


so far, so good.  let's push to hstore:
postgres=# select row_to_json(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x)::jsonb::hstore;
   row_to_json
---
  "a"=>1, "b"=>[{"a"=>1, "b"=>[{"a"=>1, "b"=>[1, 2]}]}]

this ISTM is the 'right' behavior.  but what if we bring it back to
record object?

postgres=# select populate_record(null::x, row_to_json(row(1,
array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
ERROR:  malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1, 2}"

yikes. The situation as I read it is that (notwithstanding my comments
upthread) there is no clean way to slide rowtypes to/from hstore and
jsonb while preserving structure.  IMO, the above query should work
and the populate function record above should return the internally
structured row object, not the text escaped version.



And this is a failure in populate_record().

I think we possibly need to say that handling of nested composites and 
arrays is an area that needs further work. OTOH, the refusal of 
json_populate_record() and json_populate_recordset() to handle these in 
9.3 has not generated a flood of complaints, so I don't think it's a 
tragedy, just a limitation, which should be documented if it's not 
already. (And of course hstore hasn't handled nested anything before now.)


Meanwhile, maybe Teodor can fix the two hstore bugs shown here.

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] Making strxfrm() blobs in indexes work

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 4:34 PM, Tom Lane  wrote:
> Quite aside from the index bloat risk, this effect means a 3-4x reduction
> in the maximum string length that can be indexed before getting the
> dreaded "Values larger than 1/3 of a buffer page cannot be indexed" error.
> Worse, a value insertion might well succeed, with the failure happening
> only (much?) later when that entry is chosen as a page split boundary.

That's not hard to prevent. If that should happen, we don't go with
the strxfrm() datum. We have a spare IndexTuple bit we could use to
mark when the optimization was applied. So we consider the
appropriateness of a regular strcoll() or a strxfrm() in all contexts
(in a generic and extensible manner, but that's essentially what we
do). I'm not too discouraged by this restriction, because in practice
it won't come up very often.

>> I'm sure anyone that has read this far knows where I'm going with
>> this: why can't we just have strxfrm() blobs in the inner pages,
>> implying large savings for a big majority of text comparisons that
>> service index scans, without bloating the indexes too badly, and
>> without breaking anything? We only use inner pages to find leaf pages.
>> They're redundant copies of the data within the index.
>
> It's a cute idea though, and perhaps worth pursuing as long as you've
> got the pitfalls in mind.

I'll think about pursuing it. I might prefer to declare it as fair
game for anyone else that wants to do it.


-- 
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] Prohibit row-security + inheritance in 9.4?

[Craig Ringer]

On 01/31/2014 01:18 AM, Robert Haas wrote:
> On Thu, Jan 30, 2014 at 2:39 AM, Craig Ringer  wrote:
>> Earlier discussions seemed to settle on each relation having its own
>> row-security quals applied independently. So quals on a parent rel did
>> not cascade down to child rels.
> 
> Do you have a link to that prior discussion?

Not to hand; I'm basing that on discussion with KaiGai, and on the
implementation of his RLS patch. The patch goes far out of its way to
ensure that policies on a parent relation aren't applied to children,
only to rows taken directly from the parent.

I read some discussion on the topic when I was first reviewing all the
old threads for this, but didn't see anything that seemed to
conclusively decide on that approach.

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


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

Re: [HACKERS] Prohibit row-security + inheritance in 9.4?

[Craig Ringer]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/30/2014 10:41 PM, Stephen Frost wrote:
>> Earlier discussions seemed to settle on each relation having its 
>> own row-security quals applied independently. So quals on a 
>> parent rel did not cascade down to child rels.
> 
> This strikes me as a bit odd- isn't this against how we handle the 
> GRANT system when it comes to inheiritance?  That is to say- if
> you have access to query the parent, then you'll get rows back from
> the child and I believe everyone feels that makes perfect sense.

Thanks for taking the time to look at this.

I agree that it's odd. The trouble is that there's a conflict between
two "makes perfect sense"s here.

I expect to get all rows, including inherited rows, filtered by a
parent's predicate back when you query the parent. Fair enough.

I also expect that when I query a child table, I'll see the same rows
I see when I query it via the parent table. Especially in the common
case of an empty parent table.

One of these has to give, we can't have both.

I'm speaking with an outside party who has an inheritance-based data
model they want to apply row-security to. Hopefully that'll shed some
light on practical implications.

There's another bit of fun too: If you have a policy on a child, and
query the child via the parent, should the child policy be applied?
The immediate thought is "obviously" - but then, often the child and
parent policies are going to be the same, in which case it's a
duplicate filter step. Here security trumps efficiency; I think we
just apply both, and leave proving they're identical and skipping the
child's as an optimization problem for later.

>> It's what you'd expect to happen when querying a parent rel with
>>  row-security, too. Parent quals are applied to children. But
>> that then gives you an inconsistent view of a rel's contents
>> based on whether you query it via a parent or directly.
> 
> ... Just how our existing GRANT system works.

True; it's possible to be able to query the parent, but not its
children, at present.

Treating row-security checks as permission checks, that'd make this
consistent. The difference is that you get a nice error telling you
what's going on currently, not a potentially WTF-y different resultset.

> If you want to constrain the children in the same way as the 
> parent, then the user can add to the row-security on the children 
> to match the parent.

Yes, with the caveat mentioned above that this will cause multiple
nested row-security policies when querying the parent; each child's
policy will get applied, *and* the parent's policy will get applied.
Inefficient and ugly if they're the same.

> If the user wants to have one view for the entire inheiritance
> tree then they need to only implement the row-security on the
> parent and not grant any access for users on the children (or, if
> they're paranoid, add row-security to the children which are
> essentially deny-all).

That works if the children are used for partitioning/inheritance,
where no direct access (or at least no read access) is required. It
doesn't work so well when they're actually being used in a "real"
inheritance model, where they'll have their own additional attributes
that can only be accessed via the child.

In that case the only option is to apply a policy to each child too.
If we apply the parent policy when querying via the parent, we get
multiple nested layers of policy, but it still works.

> If we force everything to behave the same between querying the 
> parent and querying the children then we cut off various scenarios 
> of partitioning where users are allowed to query specific children 
> but not the parent or query the parent to get things they're not 
> able to see directly from the children.

That's a fair concern. Note that the second one won't work if we apply
child policies when querying via the parent though; there'd be no way
to see rows via the parent that you can't see via the child.

>> 4. attempt to pull quals from parents when querying a child rel 
>> directly.
> 
> That strikes me as borderline insane. ;)

I'm glad to hear it, because it'd be inefficient and horrifying to
implement.

- -- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJS6u+5AAoJELBXNkqjr+S2QkEH/iXg4qww3KfcKLW/dDHWUL/g
33T/168ZGgeAvgFgTQdrgZmM8bUDsNnCD/GdH4PBmNWMlxwTeHmdANBEsKgDCL7r
Fu4HuF0JFEQMqHPtZSKUIXxW1KYEnWjISd+4YDQqor3aH03OV3z4vFEgAi73truR
kcqOe/xyeuPQDPe/9UTtiyIT2g/sQaeXhNqQx+queKYwjgYTIZgkUs0y46lH4pAK
nvWcWCscPIJ4bFpMr3joJQiFwegRaVIcAac89uZHL5iuMKPzp5lfEfWHUmTreZLu
1gPjRxWTcOhNZoaVpVCBA+Gsqw0255IxWKKD8I2RYPp0bK88t42cCB3aHejAjzo=
=cB2U
-END PGP SIGNATURE-


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

Re: [HACKERS] Making strxfrm() blobs in indexes work

[Tom Lane]

Peter Geoghegan  writes:
> On more occasions than I care to recall, someone has suggested that it
> would be valuable to do something with strxfrm() blobs in order to
> have cheaper locale-aware text comparisons. One obvious place to do so
> would be in indexes, but in the past that has been dismissed on the
> following grounds:

> * Index-only scans need fully formed datums to work, and strxfrm() is
> a one-way function (or so I believe). There is no reason to think that
> the original string can be reassembled from the blob, so clearly that
> won't fly.

> * There is a high cost to be paid in storage overhead. Even for
> collations like "en_US.UTF-8", that can mean that the blob is as much
> as 3-4 times larger than the original text string. Who is to say that
> we'll come out ahead even with the savings of just doing a strcmp()
> rather than a strcoll()?

Quite aside from the index bloat risk, this effect means a 3-4x reduction
in the maximum string length that can be indexed before getting the
dreaded "Values larger than 1/3 of a buffer page cannot be indexed" error.
Worse, a value insertion might well succeed, with the failure happening
only (much?) later when that entry is chosen as a page split boundary.

It's possible that TOAST compression of the strings would save you, but
I'm not convinced of that; it certainly doesn't seem like we could
guarantee no post-insertion failures that way.

Also, detoasting of strings that hadn't been long enough to need toasting
before could easily eat any savings.

> I'm sure anyone that has read this far knows where I'm going with
> this: why can't we just have strxfrm() blobs in the inner pages,
> implying large savings for a big majority of text comparisons that
> service index scans, without bloating the indexes too badly, and
> without breaking anything? We only use inner pages to find leaf pages.
> They're redundant copies of the data within the index.

It's a cute idea though, and perhaps worth pursuing as long as you've
got the pitfalls in mind.

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] jsonb and nested hstore

[Merlin Moncure]

On Thu, Jan 30, 2014 at 1:07 PM, Andrew Dunstan  wrote:
>
> On 01/29/2014 04:56 PM, Andrew Dunstan wrote:
>>
>>
>> On 01/29/2014 01:03 PM, Andrew Dunstan wrote:
>>>
>>>
>>> On 01/27/2014 10:43 PM, Andrew Dunstan wrote:


 On 01/26/2014 05:42 PM, Andrew Dunstan wrote:
>
>
> Here is the latest set of patches for nested hstore and jsonb.
>
> Because it's so large I've broken this into two patches and compressed
> them. The jsonb patch should work standalone. The nested hstore patch
> depends on it.
>
> All the jsonb functions now use the jsonb API - there is no more
> turning jsonb into text and reparsing it.
>
> At this stage I'm going to be starting cleanup on the jsonb code
> (indentation, error messages, comments etc.) as well get getting up some
> jsonb docs.
>
>
>


 Here is an update of the jsonb part of this. Charges:

  * there is now documentation for jsonb
  * most uses of elog() in json_funcs.c are replaced by ereport().
  * indentation fixes and other tidying.

 No changes in functionality.

>>>
>>>
>>> Further update of jsonb portion.
>>>
>>> Only change in functionality is the addition of casts between jsonb and
>>> json.
>>>
>>> The other changes are the merge with the new json functions code, and
>>> rearrangement of the docs changes to make them less ugly. Essentially I
>>> moved the indexterm tags right out of the table as is done in some other
>>> parts pf the docs. That makes the entry tags much clearer to read.
>>>
>>>
>>>
>>
>>
>> Updated to apply cleanly after recent commits.
>>
>>
>
> Updated  patches for both pieces. Included is some tidying done by Teodor,
> and fixes for remaining whitespace issues. This now passes "git diff --check
> master" cleanly for me.

Something seems off:

postgres=# create type z as (a int, b int[]);
CREATE TYPE
postgres=# create type y as (a int, b z[]);
CREATE TYPE
postgres=# create type x as (a int, b y[]);
CREATE TYPE

-- test a complicated construction
postgres=# select row(1, array[row(1, array[row(1, array[1,2])::z])::y])::x;
 row
-
 
(1,"{""(1,\\""{\\""\\""(1,\\""\\""{1,2}\\""\\"")\\""\\""}\\"")""}")

postgres=# select hstore(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x);
hstore
--
 "a"=>1, 
"b"=>"{\"(1,\\\"{\\\"\\\"(1,\\\"\\\"{1,2}\\\"\\\")\\\"\\\"}\\\")\"}"

here, the output escaping has leaked into the internal array
structures.  istm we should have a json expressing the internal
structure.  It does (weirdly) map back however:

postgres=# select populate_record(null::x, hstore(row(1, array[row(1,
array[row(1, array[1,2])::z])::y])::x));
   populate_record
-
 
(1,"{""(1,\\""{\\""\\""(1,\\""\\""{1,2}\\""\\"")\\""\\""}\\"")""}")


OTOH, if I go via json route:

postgres=# select row_to_json(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x);
  row_to_json
---
 {"a":1,"b":[{"a":1,"b":[{"a":1,"b":[1,2]}]}]}


so far, so good.  let's push to hstore:
postgres=# select row_to_json(row(1, array[row(1, array[row(1,
array[1,2])::z])::y])::x)::jsonb::hstore;
  row_to_json
---
 "a"=>1, "b"=>[{"a"=>1, "b"=>[{"a"=>1, "b"=>[1, 2]}]}]

this ISTM is the 'right' behavior.  but what if we bring it back to
record object?

postgres=# select populate_record(null::x, row_to_json(row(1,
array[row(1, array[row(1, array[1,2])::z])::y])::x)::jsonb::hstore);
ERROR:  malformed array literal: "{{"a"=>1, "b"=>{{"a"=>1, "b"=>{1, 2}"

yikes. The situation as I read it is that (notwithstanding my comments
upthread) there is no clean way to slide rowtypes to/from hstore and
jsonb while preserving structure.  IMO, the above query should work
and the populate function record above should return the internally
structured row object, not the text escaped version.

merlin


-- 
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: make the locale comparison more tolerating

[Tom Lane]

Pavel Raiskup  writes:
> [ 0001-pg_upgrade-make-the-locale-comparison-more-toleratin.patch ]

Committed with minor adjustments (mostly, wordsmithing the comments).

Although this was categorized as a bug fix, I'm not sure it's worth
taking the risk of back-patching.  We've not seen very many reports
of problems of this type.  Of course, you're free to carry it as a
patch in Red Hat's packages if you differ ...

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] Making strxfrm() blobs in indexes work

[Peter Geoghegan]

On more occasions than I care to recall, someone has suggested that it
would be valuable to do something with strxfrm() blobs in order to
have cheaper locale-aware text comparisons. One obvious place to do so
would be in indexes, but in the past that has been dismissed on the
following grounds:

* Index-only scans need fully formed datums to work, and strxfrm() is
a one-way function (or so I believe). There is no reason to think that
the original string can be reassembled from the blob, so clearly that
won't fly.

* There is a high cost to be paid in storage overhead. Even for
collations like "en_US.UTF-8", that can mean that the blob is as much
as 3-4 times larger than the original text string. Who is to say that
we'll come out ahead even with the savings of just doing a strcmp()
rather than a strcoll()?

So, even though using strxfrm() blobs can be much faster, it seems
hard to find a place to use them. A compelling improvement in
performance for index scans is so close, and yet so far. But having
mulled it over during my recent work on various aspects of the B-Tree
code, I believe that there is a way that we can cheat and come out
ahead with strxfrm() blobs in indexes.

I have a sample dataset that I like to work off of. It's the database
of the Mouse Genome Database, available from
http://www.informatics.jax.org/. This is real data, from a real
Postgres database, and so I recommend working off one of their weekly
dumps if you're looking for a real dataset that is big enough to
exercise most interesting things, and yet not too big. It seems that
there is an entire ecosystem of tools for medical researchers around
the dataset, and they have the right attitude about opening it all up,
which is pretty cool.

Anyway, I quickly threw together the following query, which shows the
break-down of leaf pages, inner pages and root pages among the largest
indexes, and the proportion of each per index:

[local] pg@mouse=# with tots as (
SELECT count(*) c, type, relname from
(select relname, relpages, generate_series(1, relpages - 1) i
from pg_class c join pg_namespace n on c.relnamespace = n.oid where
relkind = 'i' and nspname = 'mgd') r,
lateral (select * from bt_page_stats('mgd.' || relname, i)) u
group by relname, type)
select tots.relname, relpages -1 as non_meta_pages, c, c/sum(c)
over(partition by tots.relname) as prop_of_index, type from tots join
pg_class c on c.relname = tots.relname order by 2 desc, 1, type;

relname | non_meta_pages |   c
   |   prop_of_index| type
++++--
 acc_accession_0| 106181 |
520 | 0.00489729801000178940 | i
 acc_accession_0| 106181 |
105660 | 0.99509328410920974562 | l
 acc_accession_0| 106181 |
 1 | 0.09417880788464979610 | r
 acc_accession_idx_accid| 106181 |
520 | 0.00489729801000178940 | i
 acc_accession_idx_accid| 106181 |
105660 | 0.99509328410920974562 | l
 acc_accession_idx_accid| 106181 |
 1 | 0.09417880788464979610 | r
 mgi_notechunk_idx_note | 101127 |
4817 | 0.04763317412758214918 | i
 mgi_notechunk_idx_note | 101127 |
96309 | 0.95235693731644367973 | l
 mgi_notechunk_idx_note | 101127 |
 1 | 0.09888555974171091795 | r
 acc_accession_1|  80507 |
302 | 0.00375122660141354168 | i
 acc_accession_1|  80507 |
80204 | 0.99623635211844932739 | l
 acc_accession_1|  80507 |
 1 | 0.12421280137130932714 | r
 acc_accession_idx_prefixpart   |  80507 |
302 | 0.00375122660141354168 | i
 acc_accession_idx_prefixpart   |  80507 |
80204 | 0.99623635211844932739 | l
 acc_accession_idx_prefixpart   |  80507 |
 1 | 0.12421280137130932714 | r

***SNIP***

To the surprise of no one, typically ~99.5% of B-Tree index pages are
leaf pages. I do see one index on a column of very large text strings
(a "notes" column) that is only about 95% leaf pages, which is omitted
here. But the general picture is that over 99% of non-meta pages are
leaf pages. Again, this is hardly surprising: That's more or less why
B-Tree indexes work so well when partially cached.

I'm sure anyone that has read this far knows where I'm going with
this: why can't we just have strxfrm() blobs in the inner pages,
implying large savings for a big majority of text comparisons that
service index scans, without bloating the indexes too badly, and
without breaking anything? We only use inner pages to find leaf

Re: [HACKERS] Fwd: Request for error explaination || Adding a new integer in indextupleData Structure

[Jim Nasby]

On 1/28/14, 3:59 PM, Rohit Goyal wrote:


The data from IndexTupleData is written to disk, and then read back in 
again.  Did you initdb a new database cluster after you made your change?  If 
you did the initdb with the original code, and then tried to point your new 
code at the old disk files, that is very unlikely to work, as format is now 
different.

Cheers,

Jeff


Hi Jeff and Tom,

Thanks you so much. I was making the mistake you mentioned in the last mail. :)


The real issue here is that you need to bump the catalog version number (sorry, 
but I don't know where that is in code).
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
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] jsonb and nested hstore - small docpatch

[Erik Rijkers]

On Thu, January 30, 2014 23:15, Erik Rijkers wrote:
> On Thu, January 30, 2014 20:07, Andrew Dunstan wrote:
>>
>> Updated  patches for both pieces. Included is some tidying done by
>>
>> [ nested-hstore-9.patch.gz ]
>
> Here is a small doc-patch to   Table F-6. hstore Operators
>
> It corrects its booleans in the 'Result' column ( t and f  instead of  true 
> and false ).

I mean, here it is...

--- doc/src/sgml/hstore.sgml.orig	2014-01-30 22:39:52.970474354 +0100
+++ doc/src/sgml/hstore.sgml	2014-01-30 22:57:27.630698633 +0100
@@ -286,7 +286,7 @@
   boolean
   get boolean value for key (NULL if not boolean or not present)
   'a => 42.0, b => true'::hstore ?> 'b'
-  true
+  t
  
 
  
@@ -294,7 +294,7 @@
   boolean
   get boolean value for array index (NULL if not boolean or not present)
   '[false,null,44]'::hstore ?> 0
-  false
+  f
  
 
  
@@ -318,7 +318,7 @@
   boolean
   get boolean value for key path (NULL if not boolean or not present)
   'foo => {bar => true}'::hstore #?> '{foo,bar}'
-  true
+  t
  
 
  
@@ -366,7 +366,7 @@
   boolean
   does hstore contain key?
   'a=>1'::hstore ? 'a'
-  true
+  t
  
 
  
@@ -374,7 +374,7 @@
   boolean
   does hstore contain array index?
   '[a,b,c]'::hstore ? 2
-  true
+  t
  
 
  
@@ -382,7 +382,7 @@
   boolean
   does hstore contain key path?
   '[1, 2, {foo=>hi}]'::hstore #? '{2,foo}'
-  true
+  t
  
 
  
@@ -390,7 +390,7 @@
   boolean
   does hstore contain all specified keys?
   'a=>1,b=>2'::hstore ?& ARRAY['a','b']
-  true
+  t
  
 
  
@@ -398,7 +398,7 @@
   boolean
   does hstore contain any of the specified keys?
   'a=>1,b=>2'::hstore ?| ARRAY['b','c']
-  true
+  t
  
 
  
@@ -406,7 +406,7 @@
   boolean
   does left operand contain right?
   'a=>b, b=>1, c=>NULL'::hstore @> 'b=>1'
-  true
+  t
  
 
  
@@ -414,7 +414,7 @@
   boolean
   is left operand contained in right?
   'a=>c'::hstore <@ 'a=>b, b=>1, c=>NULL'
-  false
+  f
  
 
  
-- 
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] jsonb and nested hstore - small docpatch

[Erik Rijkers]

On Thu, January 30, 2014 20:07, Andrew Dunstan wrote:
>
> Updated  patches for both pieces. Included is some tidying done by
>
> [ nested-hstore-9.patch.gz ]

Here is a small doc-patch to   Table F-6. hstore Operators

It corrects its booleans in the 'Result' column ( t and f  instead of  true and 
false ).

Thanks,

Erik Rijkers



-- 
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] inherit support for foreign tables

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane  wrote:
>> I think this is totally misguided.  Who's to say that some weird FDW
>> might not pay attention to attstorage?  I could imagine a file-based
>> FDW using that to decide whether to compress columns, for instance.
>> Admittedly, the chances of that aren't large, but it's pretty hard
>> to argue that going out of our way to prevent it is a useful activity.

> I think that's a pretty tenuous position.  There are already
> FDW-specific options sufficient to let a particular FDW store whatever
> kinds of options it likes; letting the user set options that were only
> ever intended to be applied to tables just because we can seems sort
> of dubious.  I'm tempted by the idea of continuing to disallow SET
> STORAGE on an unvarnished foreign table, but allowing it on an
> inheritance hierarchy that contains at least one real table, with the
> semantics that we quietly ignore the foreign tables and apply the
> operation to the plain tables.

[ shrug... ] By far the easiest implementation of that is just to apply
the catalog change to all of them.  According to your assumptions, it'll
be a no-op on the foreign tables anyway.

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] inherit support for foreign tables

[Robert Haas]

On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane  wrote:
> Shigeru Hanada  writes:
>> At the moment we don't use attstorage for foreign tables, so allowing
>> SET STORAGE against foreign tables never introduce visible change
>> except \d+ output of foreign tables.  But IMO such operation should
>> not allowed because users would be confused.  So I changed
>> ATExecSetStorage() to skip on foreign tables.
>
> I think this is totally misguided.  Who's to say that some weird FDW
> might not pay attention to attstorage?  I could imagine a file-based
> FDW using that to decide whether to compress columns, for instance.
> Admittedly, the chances of that aren't large, but it's pretty hard
> to argue that going out of our way to prevent it is a useful activity.

I think that's a pretty tenuous position.  There are already
FDW-specific options sufficient to let a particular FDW store whatever
kinds of options it likes; letting the user set options that were only
ever intended to be applied to tables just because we can seems sort
of dubious.  I'm tempted by the idea of continuing to disallow SET
STORAGE on an unvarnished foreign table, but allowing it on an
inheritance hierarchy that contains at least one real table, with the
semantics that we quietly ignore the foreign tables and apply the
operation to the plain tables.

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


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

Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication

[Simon Riggs]

On 30 January 2014 17:27, Robert Haas  wrote:
> On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs  wrote:
>> On 14 January 2014 08:38, Christian Kruse  wrote:
>>> Hi,
>>> On 13/01/14 20:06, Heikki Linnakangas wrote:
 On 12/17/2013 04:58 PM, Christian Kruse wrote:
 >attached you will find a patch for showing the current transaction id
 >(xid) and the xmin of a backend in pg_stat_activty and the xmin in
 >pg_stat_replication.

 Docs.
>>>
>>> Thanks, update with updated docs is attached.
>>
>> Looks simple enough and useful for working out which people are
>> holding up CONCURRENT activities.
>>
>> I've not been involved with this patch, so any objections to me doing
>> final review and commit?
>
> Nope, but I think this patch is broken.  It looks to me like it's
> conflating the process offset in the BackendStatus array with its
> backendId, which does not seem like a good idea even if it happens to
> work at present.  And the way BackendIdGetProc() is used looks unsafe,
> too: the contents might no longer be valid by the time we read them.
> I suspect we should have a new accessor function that takes a backend
> ID and copies the xid and xmin to pointers provided by the client
> while holding the lock.  I also note that the docs seem to need some
> copy-editing:
>
> + The current xmin
> value.

Thanks, saved me the trouble of a detailed review... good catches.

-- 
 Simon Riggs   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] Add min and max execute statement time in pg_stat_statement

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 12:32 PM, Tom Lane  wrote:
>> In reality, actual applications
>> could hardly be further from the perfectly uniform distribution of
>> distinct queries presented here.
>
> Yeah, I made the same point in different words.  I think any realistic
> comparison of this code to what we had before needs to measure a workload
> with a more plausible query frequency distribution.

Even though that distribution just doesn't square with anybody's
reality, you can still increase the pg_stat_statements.max setting to
10k and the problem goes away at little cost (a lower setting is
better, but a setting high enough to cache everything is best). But
you're not going to have terribly much use for pg_stat_statements
anywayif you really do experience churn at that rate with 5,000
possible entries, the module is ipso facto useless, and should be
disabled.


-- 
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] Regression tests failing if not launched on db "regression"

[Robert Haas]

On Thu, Jan 30, 2014 at 3:32 AM, Christian Kruse
 wrote:
>> For the documentation patch, I propose the attached to avoid future
>> confusions. Comments? It might make sense to back-patch as well.
>
> Compiles, didn't find any typos and I think it is comprehensible.

I took a look at this with a view to committing it but on examination
I'm not sure this is the best way to proceed.  The proposed text
documents that the tests should be run in a database called
regression, but the larger documentation chapter of which this section
is a part never explains how to run them anywhere else, so it feels a
bit like telling a ten-year-old not to burn out the clutch.

The bit about not changing enable_* probably belongs, if anywhere, in
section 30.2, on test evaluation, rather than here.

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


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

Re: [HACKERS] [PATCH] pg_sleep(interval)

[Robert Haas]

On Thu, Oct 17, 2013 at 9:11 AM, Vik Fearing  wrote:
> On 10/17/2013 02:42 PM, Robert Haas wrote:
>> On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing  wrote:
>>> On 10/17/2013 10:03 AM, Fabien COELHO wrote:
 My guess is that it won't be committed if there is a single "but it
 might break one code or surprise one user somewhere in the universe",
 but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
 liner is really akin to "rejected".
>>> I have attached here an entirely new patch (new documentation and
>>> everything) that should please everyone.  It no longer overloads
>>> pg_sleep(double precision) but instead add two new functions:
>>>
>>>  * pg_sleep_for(interval)
>>>  * pg_sleep_until(timestamp with time zone)
>>>
>>> Because it's no longer overloading the original pg_sleep, Robert's
>>> ambiguity objection is no more.
>>>
>>> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
>>>
>>> If people like this, I'll reject the current patch and add this one to
>>> the next commitfest.
>> I find that naming relatively elegant.  However, you've got to
>> schema-qualify every function and operator used in the definitions, or
>> you're creating a search-path security vulnerability.
>>
>
> Good catch.  Updated patch attached.

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] updated emacs configuration

[Bruce Momjian]

On Thu, Jan 30, 2014 at 03:32:27PM -0500, Robert Haas wrote:
> Or this:
> 
> - mp_int_copy(a, b); /* ok: 0 <= r < b */
> - mp_int_copy(&q, a); /* ok: q <= a   */
> + mp_int_copy(a, b); /* ok:  0 <= r < b */
> + mp_int_copy(&q, a); /* ok:  q <= a */
> 
> I do agree with you some of the changes-after-periods look like
> improvements; what I meant to say is that there is an awful lot of
> churn in this patch that is unrelated to that particular point.

Let me work on a entab patch that does replacements in comments only
after periods and post the results.  I should be done in an hour.

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

  + Everyone has their own god. +


-- 
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] updated emacs configuration

[Tom Lane]

Alvaro Herrera  writes:
> Robert Haas escribió:
>> I don't have any comment on that specific point, but I took a quick
>> look through one of these diffs and it looks like a lot of churn for
>> no improvement.  So I'm not sure what we want to do here, but I don't
>> think it's this.

> I, on the contrary, think that the cases that are preceded by a period
> are an improvement, and the rest are not (the opposite, actually, so
> better not change those).  Maybe there are specific cases in which a
> period-tab sequence should be kept, but that seems rarer.

I concur with Alvaro; the only cases I want to see changed are period
followed by a tab that would be exactly two spaces.

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] Add min and max execute statement time in pg_stat_statement

[Tom Lane]

Peter Geoghegan  writes:
> On Thu, Jan 30, 2014 at 9:57 AM, Tom Lane  wrote:
>> BTW ... it occurs to me to wonder if it'd be feasible to keep the
>> query-texts file mmap'd in each backend, thereby reducing the overhead
>> to write a new text to about the cost of a memcpy, and eliminating the
>> read cost in pg_stat_statements() altogether.  It's most likely not worth
>> the trouble; but if a more-realistic benchmark test shows that we actually
>> have a performance issue there, that might be a way out without giving up
>> the functional advantages of Peter's patch.

> There could be a worst case for that scheme too, plus we'd have to
> figure out how to make in work with windows, which in the case of
> mmap() is not a sunk cost AFAIK. I'm skeptical of the benefit of
> pursuing that.

Well, it'd be something like #ifdef HAVE_MMAP then use mmap, else
use what's there today.  But I agree that it's not something to pursue
unless we see a more credible demonstration of a problem.  Presumably
any such code would have to be prepared to remap if the file grows
larger than it initially allowed for; and that would be complicated
and perhaps have unwanted failure modes.

> In reality, actual applications
> could hardly be further from the perfectly uniform distribution of
> distinct queries presented here.

Yeah, I made the same point in different words.  I think any realistic
comparison of this code to what we had before needs to measure a workload
with a more plausible query frequency distribution.

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] updated emacs configuration

[Robert Haas]

On Thu, Jan 30, 2014 at 3:04 PM, Alvaro Herrera
 wrote:
> Robert Haas escribió:
>> On Wed, Jan 29, 2014 at 11:21 PM, Bruce Momjian  wrote:
>> > I compute 6627 lines as modified.  What I did not do is handle _only_
>> > cases with periods before the tabs.  Should I try that?
>>
>> I don't have any comment on that specific point, but I took a quick
>> look through one of these diffs and it looks like a lot of churn for
>> no improvement.  So I'm not sure what we want to do here, but I don't
>> think it's this.
>
> I, on the contrary, think that the cases that are preceded by a period
> are an improvement, and the rest are not (the opposite, actually, so
> better not change those).  Maybe there are specific cases in which a
> period-tab sequence should be kept, but that seems rarer.
>
> I didn't check the entire diff obviously, so I can't comment on how good
> about detecting comments the new entab code is.

Well, the thing I really didn't like was this sort of thing:

- /* Oid subtype = PG_GETARG_OID(3); */
+ /* Oid  subtype = PG_GETARG_OID(3); */

Or this:

- * Original author: Simon Riggs  si...@2ndquadrant.com
- * Current maintainer: Simon Riggs
+ * Original author: Simon Riggs  si...@2ndquadrant.com
+ * Current maintainer:  Simon Riggs

Or this:

- * dirpath  - the directory name supplied on the command line
- * configpath  - optional configuration directory
- * envVarName  - the name of an environment variable to get if dirpath is NULL
+ * dirpath   - the directory name supplied on the command line
+ * configpath- optional configuration directory
+ * envVarName- the name of an environment variable to get if
dirpath is NULL

Or this:

- mp_int_copy(a, b); /* ok: 0 <= r < b */
- mp_int_copy(&q, a); /* ok: q <= a   */
+ mp_int_copy(a, b); /* ok:  0 <= r < b */
+ mp_int_copy(&q, a); /* ok:  q <= a */

I do agree with you some of the changes-after-periods look like
improvements; what I meant to say is that there is an awful lot of
churn in this patch that is unrelated to that particular 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] Issue with PGC_BACKEND parameters

[Andrew Dunstan]

On 01/30/2014 03:12 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 12/22/2013 11:30 PM, Amit Kapila wrote:
- * backend start.
+ * backend start. However for windows, we need to process
+ * config file during backend start for non-default
parameters,
+ * so we need to allow change of PGC_BACKEND during backend
+ * startup.
*/
-if (IsUnderPostmaster)
+if (IsUnderPostmaster && !IsInitProcessingMode())
   return -1;
   }
I think this change looks OK.

The comment is pretty awful, since this is neither Windows-specific nor
a read of the config file.  Perhaps more like "However, in EXEC_BACKEND
builds we load nondefault settings from the CONFIG_EXEC_PARAMS file during
backend start.  In that situation we should accept PGC_SIGHUP
settings, so as to have the same value as if we'd forked from the
postmaster."

Also, I think that the extra test should only be made #ifdef EXEC_BACKEND,
so as to minimize the risk of breaking things.  Not that this isn't pretty
darn fragile anyway; I think testing IsInitProcessingMode here is a very
random way to detect this case.  I wonder if it'd be better to pass down
an explicit flag indicating that we're doing read_nondefault_variables().
If we don't do that, maybe an Assert(IsInitProcessingMode()) in
read_nondefault_variables() would be a good thing.





OK, I've added your comment to the commitfest item and marked it as 
"Waiting on Author".


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] Issue with PGC_BACKEND parameters

[Tom Lane]

Andrew Dunstan  writes:
> On 12/22/2013 11:30 PM, Amit Kapila wrote:
> - * backend start.
> + * backend start. However for windows, we need to process
> + * config file during backend start for non-default 
> parameters,
> + * so we need to allow change of PGC_BACKEND during backend
> + * startup.
>*/
> -if (IsUnderPostmaster)
> +if (IsUnderPostmaster && !IsInitProcessingMode())
>   return -1;
>   }

> I think this change looks OK.

The comment is pretty awful, since this is neither Windows-specific nor
a read of the config file.  Perhaps more like "However, in EXEC_BACKEND
builds we load nondefault settings from the CONFIG_EXEC_PARAMS file during
backend start.  In that situation we should accept PGC_SIGHUP
settings, so as to have the same value as if we'd forked from the
postmaster."

Also, I think that the extra test should only be made #ifdef EXEC_BACKEND,
so as to minimize the risk of breaking things.  Not that this isn't pretty
darn fragile anyway; I think testing IsInitProcessingMode here is a very
random way to detect this case.  I wonder if it'd be better to pass down
an explicit flag indicating that we're doing read_nondefault_variables().
If we don't do that, maybe an Assert(IsInitProcessingMode()) in
read_nondefault_variables() would be a good thing.

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] updated emacs configuration

[Alvaro Herrera]

Robert Haas escribió:
> On Wed, Jan 29, 2014 at 11:21 PM, Bruce Momjian  wrote:

> > I compute 6627 lines as modified.  What I did not do is handle _only_
> > cases with periods before the tabs.  Should I try that?
> 
> I don't have any comment on that specific point, but I took a quick
> look through one of these diffs and it looks like a lot of churn for
> no improvement.  So I'm not sure what we want to do here, but I don't
> think it's this.

I, on the contrary, think that the cases that are preceded by a period
are an improvement, and the rest are not (the opposite, actually, so
better not change those).  Maybe there are specific cases in which a
period-tab sequence should be kept, but that seems rarer.

I didn't check the entire diff obviously, so I can't comment on how good
about detecting comments the new entab code is.

-- 
Álvaro Herrerahttp://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] updated emacs configuration

[Robert Haas]

On Wed, Jan 29, 2014 at 11:21 PM, Bruce Momjian  wrote:
> On Wed, Jan 29, 2014 at 07:31:29PM -0500, Tom Lane wrote:
>> Bruce Momjian  writes:
>> > I have cleaned up entab.c so I am ready to add a new option that removes
>> > tabs from only comments.  Would you like me to create that and provide a
>> > diff at a URL?  It would have to be run against all back branches.
>>
>> If you think you can actually tell the difference reliably in entab,
>> sure, give it a go.
>
> OK, I have modified entab.c in a private patch to only process text
> inside comments, and not process leading whitespace, patch attached.  I
> basically ran 'entab -o -t4 -d' on the C files.
>
> The result are here, in context, plain, and unified format:
>
> http://momjian.us/expire/entab_comment.cdiff
> http://momjian.us/expire/entab_comment.pdiff
> http://momjian.us/expire/entab_comment.udiff
>
> and their line counts:
>
> 89741 entab_comment.cdiff
> 26351 entab_comment.pdiff
> 50503 entab_comment.udiff
>
> I compute 6627 lines as modified.  What I did not do is handle _only_
> cases with periods before the tabs.  Should I try that?

I don't have any comment on that specific point, but I took a quick
look through one of these diffs and it looks like a lot of churn for
no improvement.  So I'm not sure what we want to do here, but I don't
think it's this.

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


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

Re: [HACKERS] Issue with PGC_BACKEND parameters

[Andrew Dunstan]

On 12/22/2013 11:30 PM, Amit Kapila wrote:

I had observed one problem with PGC_BACKEND parameters while testing patch
for ALTER SYSTEM command.
Problem statement: If I change PGC_BACKEND parameters directly in
postgresql.conf and then do pg_reload_conf() and reconnect, it will
   still show the old value.
Detailed steps
1. Start server with default settings
2. Connect Client
3. show log_connections; -- it will show as off, this is correct.
4. Change log_connections in postgresql.conf to on
5. issue command select pg_reload_conf() in client (which is started in step-2)
6. Connect a new client
7. show log_connections; -- it will show as off, this is "in-correct".
The problem is in step-7, it should show as on.
This problem occur only in Windows.
The reason for this problem is that in WINDOWS, when a new session is
started it will load the changed parameters in new backend by
global/config_exec_params file. The flow is in
SubPostmasterMain()->read_nondefault_variables()->set_config_option().
In below code in function set_config_option(), it will not allow to change
PGC_BACKEND variable and even in comments it has mentioned that only
postmaster will be allowed to change and the same will propagate to
subsequently started backends, but this is not TRUE for Windows.
switch (record->context)
{
..
..
case PGC_BACKEND:
   if (context == PGC_SIGHUP)
   {
   /*
* If a PGC_BACKEND parameter is changed in
the config file,
* we want to accept the new value in the
postmaster (whence
* it will propagate to subsequently-started
backends), but
* ignore it in existing backends.
This is a tad klugy, but
* necessary because we don't re-read the
config file during
* backend start.
*/
   if (IsUnderPostmaster)
   return -1;
   }
}



I think to fix the issue we need to pass the information whether PGC_BACKEND
parameter is allowed to change in set_config_option() function.
One way is to pass a new parameter.

Yesterday, I again thought about this issue and found that we can handle it by
checking IsInitProcessingMode() which will be True only during backend startup
which is what we need here.

Please find the attached patch to fix this issue.

I think that this issue should be fixed in PostgreSQL, because
currently PGC_BACKEND
parameters doesn't work on Windows.





--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5605,9 +5605,12 @@ set_config_option(const char *name, const char 
*value,
  * it will propagate to subsequently-started 
backends), but
  * ignore it in existing backends.This is a tad 
klugy, but
  * necessary because we don't re-read the config file 
during

- * backend start.
+ * backend start. However for windows, we need to process
+ * config file during backend start for non-default 
parameters,

+ * so we need to allow change of PGC_BACKEND during backend
+ * startup.
  */
-if (IsUnderPostmaster)
+if (IsUnderPostmaster && !IsInitProcessingMode())
 return -1;
 }
 else if (context != PGC_POSTMASTER && context != 
PGC_BACKEND &&





I think this change looks OK. Does anyone else have any comments before 
I test and apply it? I presume it's a bugfix that should be backpatched.


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] Add min and max execute statement time in pg_stat_statement

[Peter Geoghegan]

On Thu, Jan 30, 2014 at 9:57 AM, Tom Lane  wrote:
> BTW ... it occurs to me to wonder if it'd be feasible to keep the
> query-texts file mmap'd in each backend, thereby reducing the overhead
> to write a new text to about the cost of a memcpy, and eliminating the
> read cost in pg_stat_statements() altogether.  It's most likely not worth
> the trouble; but if a more-realistic benchmark test shows that we actually
> have a performance issue there, that might be a way out without giving up
> the functional advantages of Peter's patch.

There could be a worst case for that scheme too, plus we'd have to
figure out how to make in work with windows, which in the case of
mmap() is not a sunk cost AFAIK. I'm skeptical of the benefit of
pursuing that.

I'm totally unimpressed with the benchmark as things stand. It relies
on keeping 64 clients in perfect lockstep as each executes 10,000
queries that are each unique snowflakes.  Though even though they're
unique snowflakes, and even though there are 10,000 of them, everyone
executes the same one at exactly the same time relative to each other,
in exactly the same order as quickly as possible. Even still, the
headline "reference score" of -35% is completely misleading, because
it isn't comparing like with like in terms of has table size. This
benchmark incidentally recommends that we reduce the default hash
table size to improve performance when the hash table is under
pressure, which is ludicrous. It's completely backwards. You could
also use the benchmark to demonstrate that the overhead of calling
pg_stat_statements() is ridiculously high, since like creating a new
query text, that only requires a shared lock too.

This is an implausibly bad worst case for larger hash table sizes in
pg_stat_statements generally. 5,000 entries is enough for the large
majority of applications. But for those that hit that limit, in
practice they're still going to find the vast majority of queries
already in the table as they're executed. If they don't, they can
double or triple their "max" setting, because the shared memory
overhead is so low. No one has any additional overhead once their
query is in the hash table already. In reality, actual applications
could hardly be further from the perfectly uniform distribution of
distinct queries presented 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: [HACKERS] Re: Patch to add support of "IF NOT EXISTS" to others "CREATE" statements

[Pavel Stehule]

2014-01-19 Tom Lane :

> Stephen Frost  writes:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> I kind of don't see the point of having IF NOT EXISTS for things that
> >> have OR REPLACE, and am generally in favor of implementing OR REPLACE
> >> rather than IF NOT EXISTS where possible.  The point is usually to get
> >> the object to a known state, and OR REPLACE will generally accomplish
> >> that better than IF NOT EXISTS.  However, if the object has complex
> >> structure (like a table that contains data) then "replacing" it is a
> >> bad plan, so IF NOT EXISTS is really the best you can do - and it's
> >> still useful, even if it does require more care.
>
> > This patch is in the most recent commitfest and marked as Ready for
> > Committer, so I started reviewing it and came across the above.
>
> > I find myself mostly agreeing with the above comments from Robert, but
> > it doesn't seem like we've really done a comprehensive review of the
> > various commands to make a 'command' decision on each as to if it should
> > have IF NOT EXISTS or OR REPLACE options.
>
> There's been pretty extensive theorizing about this in the past (try
> searching the pghackers archives for "CINE" and "COR"), and I think the
> rough consensus was that it's hard to do COR sensibly for objects
> containing persistent state (ie tables) or with separately-declarable
> substructure (again, mostly tables, though composite types have some of
> the same issues).  However, if COR does make sense then CINE is an
> inferior alternative, because of the issue about not knowing the resulting
> state of the object for sure.
>
> Given this list I would absolutely reject CINE for aggregates (why in the
> world would we make them act differently from functions?), and likewise
> for casts, collations, operators, and types.  I don't see any reason not
> to prefer COR for these object kinds.  There is room for argument about
> the text search stuff, though, because of the fact that some of the text
> search object types have separately declarable substructure.
>
> > The one difficulty that I do see with the 'OR REPLACE' option is when we
> > can't simply replace an existing object due to dependencies on the
> > existing definition of that object.  Still, if that's the case, wouldn't
> > you want an error?
>
> The main knock on COR is that there's no way for the system to completely
> protect itself from the possibility that you replaced the object
> definition with something that behaves incompatibly.  For instance, if we
> had COR for collations and you redefined a collation, that might (or might
> not) break indexes whose ordering depends on that collation.  However,
> we already bought into that type of risk when we invented COR for
> functions, and by and large there have been few complaints about it.
> The ability to substitute an improved version of a function seems to be
> worth the risks of substituting a broken version.
>
> regards, tom lane
>
>
I agree with Tom proposal - CINE - where object holds data, COR everywhere
else.

But it means, so all functionality from this patch have to be rewritten :(

Regards

Pavel


>
> --
> 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] Triggers on foreign tables

[Noah Misch]

On Thu, Jan 23, 2014 at 03:17:35PM +0100, Ronan Dunklau wrote:
> > > - for after triggers, the whole queuing mechanism is bypassed for foreign
> > > tables. This is IMO acceptable, since foreign tables cannot have
> > > constraints or constraints triggers, and thus have not need for
> > > deferrable execution. This design avoids the need for storing and
> > > retrieving/identifying remote tuples until the query or transaction end.
> > 
> > Whether an AFTER ROW trigger is deferred determines whether it runs at the
> > end of the firing query or at the end of the firing query's transaction. 
> > In all cases, every BEFORE ROW trigger of a given query fires before any
> > AFTER ROW trigger of the same query.  SQL requires that.  This proposal
> > would give foreign table AFTER ROW triggers a novel firing time; let's not
> > do that.
> > 
> > I think the options going forward are either (a) design a way to queue
> > foreign table AFTER ROW triggers such that we can get the old and/or new
> > rows at the end of the query or (b) not support AFTER ROW triggers on
> > foreign tables for the time being.
> >
> 
> I did not know this was mandated by the standard.
> 
> The attached patch tries to solve this problem by allocating a tuplestore
> in the global afterTriggers structure. This tuplestore is used for the whole 
> transaction, and uses only work_mem per transaction.
> 
> Both old and new tuples are stored in this tuplestore. Some additional 
> bookkeeping is done on the afterTriggers global structure, to keep track of 
> the number of inserted tuples, and the current read pointer position. The 
> tuples are identified by their order of insertion during the transaction.
> I think this could benefit from some support in the tuplestore API, by 
> allowing arbitrary seek without the need to store more ReadPointers.
> 
> I initially tried to keep track of them by allocating read pointers on the 
> tuple store, but it turned out to be so expensive that I had to find another 
> way (24bytes per stored tuple, which are not reclaimable until the end of the 
> transaction).
> 
> What do you think about this approach ? Is there something I missed which 
> would make it not sustainable ?

Seems basically reasonable.  I foresee multiple advantages from having one
tuplestore per query level as opposed to one for the entire transaction.  You
would remove the performance trap of backing up the tuplestore by rescanning.
It permits reclaiming memory and disk space in AfterTriggerEndQuery() rather
than at end of transaction.  You could remove ate_ptr1 and ate_ptr2 from
AfterTriggerEventDataFDW and just store the flags word: depending on
AFTER_TRIGGER_2CTIDS, grab either the next one or the next two tuples from the
tuplestore.  Using work_mem per AfterTriggerBeginQuery() instead of per
transaction is no problem.  What do you think of that design change?

If you do pursue that change, make sure the code still does the right thing
when it drops queued entries during subxact abort.

> I do not have access to the standard specification, any advice regarding 
> specs compliance would be welcomed.

http://wiki.postgresql.org/wiki/Developer_FAQ#Where_can_I_get_a_copy_of_the_SQL_standards.3F

> > > There is still one small issue with the attached patch: modifications to
> > > the tuple performed by the foreign data wrapper (via the returned
> > > TupleTableSlot in ExecForeignUpdate and ExecForeignInsert hooks) are not
> > > visible to the AFTER trigger. This could be fixed by merging the planslot
> > > containing the resjunk columns with the returned slot before calling the
> > > trigger, but I'm not really sure how to safely perform that. Any advice ?
> > 
> > Currently, FDWs are permitted to skip returning columns not actually
> > referenced by any RETURNING clause.  I would change that part of the API
> > contract to require returning all columns when an AFTER ROW trigger is
> > involved.  You can't get around doing that by merging old column values,
> > because, among other reasons, an INSERT does not have those values at all.
> 
> I'm not sure this should be part of the API contract: it would make 
> implementing a FDW more complicated than it is now. The attached patch hooks 
> on rewriteTargetListIU to add the missing targets to the returning clause, 
> when needed.

You're effectively faking the presence of a RETURNING list so today's
conforming FDWs will already do the right thing?  Clever.

> > Please don't change unrelated whitespace.
> 
> > Please use pgindent to fix the formatting of your new code.  It's fine to
> > introduce occasional whitespace errors, but they're unusually-plentiful
> > here.
> 
> I think its done now. One problem I have with running pgindent is that I 
> accidentally add chunks that were modified only by pgindent.

Yep; that is a pain.

Note that pgindent can't fix many unrelated whitespace changes.  For example,
if you add or remove a blank line, pgindent won't interfere.  (We would not
want it to inte

Re: [HACKERS] jsonb and nested hstore

[Andrew Dunstan]

On 01/30/2014 01:50 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 01/30/2014 01:03 PM, Hannu Krosing wrote:

On 01/30/2014 06:45 PM, Andrew Dunstan wrote:

We'd have to move that code from hstore to jsonb_support.c and then
make hstore refer to it.

Or just copy it and leave hstore alone - the code duplication is not
terribly huge here and hstore might still want to develop independently.

We have gone to great deal of trouble to make jsonb and nested hstore
more or less incarnations of the same thing. The new hstore relies
heavily on the new jsonb. So what you're suggesting is the opposite of
what's been developed these last months.

If so, why would you be resistant to pushing more code out of hstore
and into jsonb?




I'm not. Above I suggested exactly that. I was simply opposed to Hannu's 
suggestion that instead  of making hstore refer to the adopted code we 
maintain two copies of code that does essentially the same thing.


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] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> >> I assert that we should simply remove both of these bits of code, as
> >> just about every committer on the project is smarter about when to use
> >> vertical whitespace than this program is.
> 
> > OK, I have developed the attached patch that shows the code change of
> > removing the test for #else/#elif/#endif.  You will see that the new
> > output has odd blank lines for cases like:
> 
> > #ifndef WIN32
> > static intcopy_file(const char *fromfile, const char *tofile, bool 
> > force);
> > -->
> > #else
> > static intwin32_pghardlink(const char *src, const char *dst);
> > -->
> > #endif
> 
> Hm.  So actually, that code is trying to undo excess vertical whitespace
> that something earlier in pgindent added?  Where is that happening?

I am afraid it is _inside_ BSD indent, and if ever looked at that code,
you would not want to go in there.  :-O

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

  + Everyone has their own god. +


-- 
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: tests for client programs

[Pavel Stehule]

Hello

This patch creates a infrastructure for client side testing environment

1. Surely we want this patch, this infrastructure - lot of important
functionality is not covered by tests - pg_dump, pg_basebackup, ... Without
infrastructure is terrible work to create some tests.

2. This patch does exactly what was proposed

3. New functionality has zero impact on our server side or client side
software (performance, quality, readability, ..)

4. I was able to use this patch cleanly and it works

5. I didn't test it on windows

6. I found only few issues:

a) Configure doesn't check a required IRC::Run module

b) Prepared tests fails when PostgreSQL server was up - should be checked
and should to raise a valuable error message

c) I am not sure if infrastructure is enough - for test pg_dump I miss a
comparation of produced file with some other expected file. This objection
is not blocker - someone can enhance it (when will write tests of pg_dump
for example).

Regards

Pavel Stehule


2014-01-29 Pavel Stehule :

> Hello
>
> I am looking on this patch.
>
> It is great idea, and I am sure, so we want this patch - it was requested
> and proposed more time.
>
> Some tips:
>
> a) possibility to test only selected tests
> b) possibility to verify generated file against expected file
> c) detection some warnings (expected/unexpected)
>
> p.s. some tests fails when other Postgres is up. It should be checked on
> start.and raise warning or stop testing.
>
> Regards
>
> Pavel
>
>
> ok 4 - pg_ctl initdb
> waiting for server to startLOG:  could not bind IPv6 socket: Address
> already in use
> HINT:  Is another postmaster already running on port 5432? If not, wait a
> few seconds and retry.
> LOG:  could not bind IPv4 socket: Address already in use
> HINT:  Is another postmaster already running on port 5432? If not, wait a
> few seconds and retry.
> WARNING:  could not create listen socket for "localhost"
> FATAL:  could not create any TCP/IP sockets
>  stopped waiting
> pg_ctl: could not start server
> Examine the log output.
> not ok 5 - pg_ctl start -w
>
> #   Failed test 'pg_ctl start -w'
> #   at
> /home/pavel/src/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> line 89.
> waiting for server to startLOG:  could not bind IPv6 socket: Address
> already in use
> HINT:  Is another postmaster already running on port 5432? If not, wait a
> few seconds and retry.
> LOG:  could not bind IPv4 socket: Address already in use
> HINT:  Is another postmaster already running on port 5432? If not, wait a
> few seconds and retry.
> WARNING:  could not create listen socket for "localhost"
> FATAL:  could not create any TCP/IP sockets
>  stopped waiting
> pg_ctl: could not start server
> Examine the log output.
> not ok 6 - second pg_ctl start succeeds
>
> #   Failed test 'second pg_ctl start succeeds'
> #   at
> /home/pavel/src/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> line 89.
> pg_ctl: PID file
> "/home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data/postmaster.pid"
> does not exist
> Is server running?
> not ok 7 - pg_ctl stop -w
>
> #   Failed test 'pg_ctl stop -w'
> #   at
> /home/pavel/src/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> line 89.
> ok 8 - second pg_ctl stop fails
> pg_ctl: PID file
> "/home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data/postmaster.pid"
> does not exist
> Is server running?
> starting server anyway
> pg_ctl: could not read file
> "/home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data/postmaster.opts"
> not ok 9 - pg_ctl restart with server not running
>
> #   Failed test 'pg_ctl restart with server not running'
> #   at
> /home/pavel/src/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> line 89.
> pg_ctl: PID file
> "/home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data/postmaster.pid"
> does not exist
> Is server running?
> starting server anyway
> pg_ctl: could not read file
> "/home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data/postmaster.opts"
> not ok 10 - pg_ctl restart with server running
>
> #   Failed test 'pg_ctl restart with server running'
> #   at
> /home/pavel/src/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> line 89.
> pg_ctl: PID file
> "/home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data/postmaster.pid"
> does not exist
> Is server running?
> Bailout called.  Further testing stopped:  system pg_ctl stop -D
> /home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data -m fast failed: 256
> Bail out!  system pg_ctl stop -D
> /home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data -m fast failed: 256
> FAILED--Further testing stopped: system pg_ctl stop -D
> /home/pavel/src/postgresql/src/bin/pg_ctl/testf2KW/data -m fast failed: 256
> make[1]: *** [check] Error 255
> make[1]: Leaving directory `/home/pavel/src/postgresql/src/bin/pg_ctl'
> make: *** [check-pg_ctl-recurse] Error 2
> make: Leaving directory `/home/pavel/src/postgresql/src/bin'
>
>

Re: [HACKERS] jsonb and nested hstore

[Tom Lane]

Andrew Dunstan  writes:
> On 01/30/2014 01:03 PM, Hannu Krosing wrote:
>> On 01/30/2014 06:45 PM, Andrew Dunstan wrote:
>>> We'd have to move that code from hstore to jsonb_support.c and then
>>> make hstore refer to it.

>> Or just copy it and leave hstore alone - the code duplication is not
>> terribly huge here and hstore might still want to develop independently.

> We have gone to great deal of trouble to make jsonb and nested hstore 
> more or less incarnations of the same thing. The new hstore relies 
> heavily on the new jsonb. So what you're suggesting is the opposite of 
> what's been developed these last months.

If so, why would you be resistant to pushing more code out of hstore
and into jsonb?

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] CREATE EVENT TRIGGER syntax

[Bruce Momjian]

On Fri, Aug  9, 2013 at 09:12:03AM -0400, Robert Haas wrote:
> On Mon, Aug 5, 2013 at 4:53 PM, Dimitri Fontaine  
> wrote:
> > Bruce Momjian  writes:
> >> So do we want to keep that "AND" in the 9.3beta and 9.4 documentation?
> >
> > The grammar as in gram.y still allows the AND form, and I think we're
> > used to maintain documentation that matches the code here. So I think it
> > makes sense to remove both capabilities as we failed to deliver any
> > other filter.
> >
> > But if we wanted to clean that, what about having the grammar check for
> > the only one item we support rather than waiting until into
> > CreateEventTrigger() to ereport a syntax error?
> 
> I have found that it's generally better to recognize such errors in
> the post-parse phase rather than during parsing.  When you start
> adding more options, that tends to quickly become the only workable
> option anyway.

OK, so I am assuming there is no additional work to do this area.  Thanks.

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

  + Everyone has their own god. +


-- 
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] pgindent behavior we could do without

[Tom Lane]

Bruce Momjian  writes:
> On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
>> I assert that we should simply remove both of these bits of code, as
>> just about every committer on the project is smarter about when to use
>> vertical whitespace than this program is.

> OK, I have developed the attached patch that shows the code change of
> removing the test for #else/#elif/#endif.  You will see that the new
> output has odd blank lines for cases like:

>   #ifndef WIN32
>   static intcopy_file(const char *fromfile, const char *tofile, bool 
> force);
> -->
>   #else
>   static intwin32_pghardlink(const char *src, const char *dst);
> -->
>   #endif

Hm.  So actually, that code is trying to undo excess vertical whitespace
that something earlier in pgindent added?  Where is that happening?

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] pgindent behavior we could do without

[Bruce Momjian]

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> It's always annoyed me that pgindent insists on adjusting vertical
> whitespace around #else and related commands.  This has, for example,
> rendered src/include/storage/barrier.h nigh illegible: you get things
> like
> 
> /*
>  * lwsync orders loads with respect to each other, and similarly with stores.
>  * But a load can be performed before a subsequent store, so sync must be used
>  * for a full memory barrier.
>  */
> #define pg_memory_barrier() __asm__ __volatile__ ("sync" : : : "memory")
> #define pg_read_barrier()   __asm__ __volatile__ ("lwsync" : : : "memory")
> #define pg_write_barrier()  __asm__ __volatile__ ("lwsync" : : : "memory")
> #elif defined(__alpha) || defined(__alpha__)/* Alpha */
> 
> which makes it look like this block of code has something to do with
> Alpha.
> 
> By chance, I noticed today that this misbehavior comes from a discretely
> identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:
> 
>   # Remove blank line(s) before #else, #elif, and #endif
>   $source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;
> 
> This seems pretty broken to me: why exactly is whitespace there such a
> bad idea?  Not only that, but the next action is concerned with undoing
> some of the damage this rule causes:
> 
>   # Add blank line before #endif if it is the last line in the file
>   $source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;
> 
> I assert that we should simply remove both of these bits of code, as
> just about every committer on the project is smarter about when to use
> vertical whitespace than this program is.

OK, I have developed the attached patch that shows the code change of
removing the test for #else/#elif/#endif.  You will see that the new
output has odd blank lines for cases like:

#ifndef WIN32
static intcopy_file(const char *fromfile, const char *tofile, bool 
force);
-->
#else
static intwin32_pghardlink(const char *src, const char *dst);
-->
#endif

I can't think of a way to detect tight blocks like the above from cases
where there are sizable blocks, like:

#ifndef WIN32

static intcopy_file(const char *fromfile, const char *tofile, bool 
force);
...
...
...

#else

static intwin32_pghardlink(const char *src, const char *dst);
...
...
...

#endif

Ideas?

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

  + Everyone has their own god. +


-- 
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] jsonb and nested hstore

[Andrew Dunstan]

On 01/30/2014 01:03 PM, Hannu Krosing wrote:

On 01/30/2014 06:45 PM, Andrew Dunstan wrote:

On 01/30/2014 12:34 PM, Merlin Moncure wrote:

On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan 
wrote:

Now, if we're agreed on that, I then also wonder if the 'as_text'
argument needs to exist at all for the populate functions except for
backwards compatibility on the json side (not jsonb).  For
non-complex
structures it does best effort casting anyways so the flag is moot.


Well, I could certainly look at making the populate_record{set} and
to_record{set} logic handle types that are arrays or composites
inside the
record. It might not be terribly hard to do - not sure.




A quick analysis suggests that this is fixable with fairly minimal
disturbance in the jsonb case.

As row_to_json() works with arbitrarily complex nested types (for
example row having a field
of type array of another (table)type containing arrays of third type) it
would be really nice if
you can get the result back into that row without too much hassle.

and it should be ok to treat json as "source type" and require it to be
translated to jsonb
for more complex operations



Might be possible.


In the json case it would probably involve
reparsing the inner json. That's probably doable, because the
routines are
all reentrant, but not likely to be terribly efficient. It will also
be a
deal more work.

Right.  Also the text json functions are already in the wild anyways
-- that's not in the scope of this patch so if they need to be fixed
that could be done later.

ISTM then the right course of action is to point jsonb 'populate'
variants at hstore implementation, not the text json one and remove
the 'as text' argument.  Being able to ditch that argument is the main
reason why I think this should be handled now (not forcing hstore
dependency to handle complex json is gravy).


We can't reference any hstore code in jsonb. There is no guarantee
that hstore will even be loaded.

We'd have to move that code from hstore to jsonb_support.c and then
make hstore refer to it.

Or just copy it and leave hstore alone - the code duplication is not
terribly huge
here and hstore might still want to develop independently.




We have gone to great deal of trouble to make jsonb and nested hstore 
more or less incarnations of the same thing. The new hstore relies 
heavily on the new jsonb. So what you're suggesting is the opposite of 
what's been developed these last months.


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] Patch: compiling the docs under Gentoo

[Alvaro Herrera]

Christian Kruse wrote:

> +Since Gentoo often supports different versions of a package to be
> +installed you have to tell the PostgreSQL build environment where the
> +Docbook DTD is located:
> +
> +cd /path/to/postgresql/sources/doc
> +make DOCBOOKSTYLE=/usr/share/sgml/docbook/sgml-dtd-4.2
> +

AFAICS this should be handled in config/docbook.m4 by adding
sgml/docbook/sgml-dtd-4.2 to the list already there.  Maybe a wildcard
could be used to avoid the version dependency, not sure.

-- 
Álvaro Herrerahttp://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] extension_control_path

[Sergey Muraviov]

Hi.

Now it looks fine for me.

2014-01-28 Dimitri Fontaine :

> Hi,
>
> Sergey Muraviov  writes:
> > Now patch applies cleanly and works. :-)
>
> Cool ;-)
>
> > But I have some notes:
> >
> > 1. There is an odd underscore character in functions
> > find_in_extension_control_path and list_extension_control_paths:
> > \"extension_control__path\""
>
> Fixed in the new version of the patch, attached.
>
> > 2. If we have several versions of one extension in different directories
> > (which are listed in extension_control_path parameter) then we
> > get strange output from pg_available_extensions and
> > pg_available_extension_versions views (Information about extension, whose
> > path is at the beginning of the list, is duplicated). And only one
> version
> > of the extension can be created.
>
> Fixed.
>
> > 3. It would be fine to see an extension control path
> > in pg_available_extensions and pg_available_extension_versions views (in
> > separate column or within of extension name).
>
> I think the on-disk location is an implementation detail and decided in
> the attached version not to change those system view definitions.
>
> > 4. Perhaps the CREATE EXTENSION command should be improved to allow
> > creation of the required version of the extension.
> > So we can use different versions of extensions in different databases.
>
> Fixed in the attached.
>
> I also fixed ALTER EXTENSION UPDATE to search for udpate scripts in the
> same directory where the main control file is found, but I suspect this
> part requires more thinking.
>
> When we ALTER EXTENSION UPDATE we might now have several places where we
> find extname.control files, with possibly differents default_version
> properties.
>
> In the attached, we select the directory containing the control file
> where default_version matches the already installed extension version.
> That matches with a model where the new version of the extension changes
> the default_version in an auxiliary file.
>
> We might want to instead match on the default_version in the control
> file to match with the new version we are asked to upgrade to.
>
> Regards,
> --
> Dimitri Fontaine
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
>
>


-- 
Best regards,
Sergey Muraviov

Re: [HACKERS] jsonb and nested hstore

[Hannu Krosing]

On 01/30/2014 06:45 PM, Andrew Dunstan wrote:
>
> On 01/30/2014 12:34 PM, Merlin Moncure wrote:
>> On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan 
>> wrote:
> Now, if we're agreed on that, I then also wonder if the 'as_text'
> argument needs to exist at all for the populate functions except for
> backwards compatibility on the json side (not jsonb).  For
> non-complex
> structures it does best effort casting anyways so the flag is moot.
>
 Well, I could certainly look at making the populate_record{set} and
 to_record{set} logic handle types that are arrays or composites
 inside the
 record. It might not be terribly hard to do - not sure.
>>> A quick analysis suggests that this is fixable with fairly minimal
>>> disturbance in the jsonb case. 
As row_to_json() works with arbitrarily complex nested types (for
example row having a field
of type array of another (table)type containing arrays of third type) it
would be really nice if
you can get the result back into that row without too much hassle.

and it should be ok to treat json as "source type" and require it to be
translated to jsonb
for more complex operations
>>> In the json case it would probably involve
>>> reparsing the inner json. That's probably doable, because the
>>> routines are
>>> all reentrant, but not likely to be terribly efficient. It will also
>>> be a
>>> deal more work.
>> Right.  Also the text json functions are already in the wild anyways
>> -- that's not in the scope of this patch so if they need to be fixed
>> that could be done later.
>>
>> ISTM then the right course of action is to point jsonb 'populate'
>> variants at hstore implementation, not the text json one and remove
>> the 'as text' argument.  Being able to ditch that argument is the main
>> reason why I think this should be handled now (not forcing hstore
>> dependency to handle complex json is gravy).
>
>
> We can't reference any hstore code in jsonb. There is no guarantee
> that hstore will even be loaded.
>
> We'd have to move that code from hstore to jsonb_support.c and then
> make hstore refer to it.
Or just copy it and leave hstore alone - the code duplication is not
terribly huge
here and hstore might still want to develop independently.

Cheers

-- 
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ



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

Re: [HACKERS] Add min and max execute statement time in pg_stat_statement

[Tom Lane]

BTW ... it occurs to me to wonder if it'd be feasible to keep the
query-texts file mmap'd in each backend, thereby reducing the overhead
to write a new text to about the cost of a memcpy, and eliminating the
read cost in pg_stat_statements() altogether.  It's most likely not worth
the trouble; but if a more-realistic benchmark test shows that we actually
have a performance issue there, that might be a way out without giving up
the functional advantages of Peter's patch.

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] jsonb and nested hstore

[Andrew Dunstan]

On 01/30/2014 12:34 PM, Merlin Moncure wrote:

On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan  wrote:

Now, if we're agreed on that, I then also wonder if the 'as_text'
argument needs to exist at all for the populate functions except for
backwards compatibility on the json side (not jsonb).  For non-complex
structures it does best effort casting anyways so the flag is moot.


Well, I could certainly look at making the populate_record{set} and
to_record{set} logic handle types that are arrays or composites inside the
record. It might not be terribly hard to do - not sure.

A quick analysis suggests that this is fixable with fairly minimal
disturbance in the jsonb case. In the json case it would probably involve
reparsing the inner json. That's probably doable, because the routines are
all reentrant, but not likely to be terribly efficient. It will also be a
deal more work.

Right.  Also the text json functions are already in the wild anyways
-- that's not in the scope of this patch so if they need to be fixed
that could be done later.

ISTM then the right course of action is to point jsonb 'populate'
variants at hstore implementation, not the text json one and remove
the 'as text' argument.  Being able to ditch that argument is the main
reason why I think this should be handled now (not forcing hstore
dependency to handle complex json is gravy).



We can't reference any hstore code in jsonb. There is no guarantee that 
hstore will even be loaded.


We'd have to move that code from hstore to jsonb_support.c and then make 
hstore refer to it.


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] Add min and max execute statement time in pg_stat_statement

[Tom Lane]

Robert Haas  writes:
> One could test it with each pgbench thread starting at a random point
> in the same sequence and wrapping at the end.

Well, the real point is that 1 distinct statements all occurring with
exactly the same frequency isn't a realistic scenario: any hashtable size
less than 1 necessarily sucks, any size >= 1 is perfect.

I'd think that what you want to test is a long-tailed frequency
distribution (probably a 1/N type of law) where a small number of
statements account for most of the hits and there are progressively fewer
uses of less common statements.  What would then be interesting is how
does the performance change as the hashtable size is varied to cover more
or less of that distribution.

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] jsonb and nested hstore

[Merlin Moncure]

On Thu, Jan 30, 2014 at 9:50 AM, Andrew Dunstan  wrote:
>>> Now, if we're agreed on that, I then also wonder if the 'as_text'
>>> argument needs to exist at all for the populate functions except for
>>> backwards compatibility on the json side (not jsonb).  For non-complex
>>> structures it does best effort casting anyways so the flag is moot.
>>>
>>
>> Well, I could certainly look at making the populate_record{set} and
>> to_record{set} logic handle types that are arrays or composites inside the
>> record. It might not be terribly hard to do - not sure.
>
> A quick analysis suggests that this is fixable with fairly minimal
> disturbance in the jsonb case. In the json case it would probably involve
> reparsing the inner json. That's probably doable, because the routines are
> all reentrant, but not likely to be terribly efficient. It will also be a
> deal more work.

Right.  Also the text json functions are already in the wild anyways
-- that's not in the scope of this patch so if they need to be fixed
that could be done later.

ISTM then the right course of action is to point jsonb 'populate'
variants at hstore implementation, not the text json one and remove
the 'as text' argument.  Being able to ditch that argument is the main
reason why I think this should be handled now (not forcing hstore
dependency to handle complex json is gravy).

People handling json as text would then invoke a ::jsonb cast trading
off performance for flexibility which is perfectly fine.  If you
agree, perhaps we can HINT the error in certain places that return
"ERROR:  cannot call json_populate_record on a nested object" that the
jsonb variant can be used as a workaround.

merlin


-- 
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: show xid and xmin in pg_stat_activity and pg_stat_replication

[Robert Haas]

On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs  wrote:
> On 14 January 2014 08:38, Christian Kruse  wrote:
>> Hi,
>> On 13/01/14 20:06, Heikki Linnakangas wrote:
>>> On 12/17/2013 04:58 PM, Christian Kruse wrote:
>>> >attached you will find a patch for showing the current transaction id
>>> >(xid) and the xmin of a backend in pg_stat_activty and the xmin in
>>> >pg_stat_replication.
>>>
>>> Docs.
>>
>> Thanks, update with updated docs is attached.
>
> Looks simple enough and useful for working out which people are
> holding up CONCURRENT activities.
>
> I've not been involved with this patch, so any objections to me doing
> final review and commit?

Nope, but I think this patch is broken.  It looks to me like it's
conflating the process offset in the BackendStatus array with its
backendId, which does not seem like a good idea even if it happens to
work at present.  And the way BackendIdGetProc() is used looks unsafe,
too: the contents might no longer be valid by the time we read them.
I suspect we should have a new accessor function that takes a backend
ID and copies the xid and xmin to pointers provided by the client
while holding the lock.  I also note that the docs seem to need some
copy-editing:

+ The current xmin
value.

-- 
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] Add min and max execute statement time in pg_stat_statement

[Robert Haas]

On Thu, Jan 30, 2014 at 12:23 PM, Tom Lane  wrote:
> I wrote:
>> If I understand this test scenario properly, there are no duplicate
>> queries, so that each iteration creates a new hashtable entry (possibly
>> evicting an old one).  And it's a single-threaded test, so that there
>> can be no benefit from reduced locking.
>
> After looking more closely, it's *not* single-threaded, but each pgbench
> thread is running through the same sequence of 1 randomly generated
> SQL statements.  So everything depends on how nearly those clients stay
> in step.  I bet they'd stay pretty nearly in step, though --- any process
> lagging behind would find all the hashtable entries already created, and
> thus be able to catch up relative to the ones in the lead which are being
> slowed by having to write out their query texts.  So it seems fairly
> likely that this scenario is greatly stressing the case where multiple
> processes redundantly create the same hashtable entry.  In any case,
> while the same table entry does get touched once-per-client over a short
> interval, there's no long-term reuse of table entries.  So I still say
> this isn't at all representative of real-world use of pg_stat_statements.

One could test it with each pgbench thread starting at a random point
in the same sequence and wrapping at the end.

-- 
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] Add min and max execute statement time in pg_stat_statement

[Tom Lane]

I wrote:
> If I understand this test scenario properly, there are no duplicate
> queries, so that each iteration creates a new hashtable entry (possibly
> evicting an old one).  And it's a single-threaded test, so that there
> can be no benefit from reduced locking.

After looking more closely, it's *not* single-threaded, but each pgbench
thread is running through the same sequence of 1 randomly generated
SQL statements.  So everything depends on how nearly those clients stay
in step.  I bet they'd stay pretty nearly in step, though --- any process
lagging behind would find all the hashtable entries already created, and
thus be able to catch up relative to the ones in the lead which are being
slowed by having to write out their query texts.  So it seems fairly
likely that this scenario is greatly stressing the case where multiple
processes redundantly create the same hashtable entry.  In any case,
while the same table entry does get touched once-per-client over a short
interval, there's no long-term reuse of table entries.  So I still say
this isn't at all representative of real-world use of pg_stat_statements.

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] Prohibit row-security + inheritance in 9.4?

[Robert Haas]

On Thu, Jan 30, 2014 at 2:39 AM, Craig Ringer  wrote:
> Earlier discussions seemed to settle on each relation having its own
> row-security quals applied independently. So quals on a parent rel did
> not cascade down to child rels.

Do you have a link to that prior discussion?  I don't remember
reaching consensus specifically on that point.  And at any rate, if
the implementation complexity mitigates strongly in the other
direction, that may be a sign that we should consider revising our
opinion on what the right thing to do is.

The problem with just accepting that feature A doesn't work with
feature B and releasing anyway is that, in many cases, nobody ever
gets around to fixing it.  We have some of those warts already, and
I'm not keen to add more.

-- 
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] Add min and max execute statement time in pg_stat_statement

[Tom Lane]

Peter Geoghegan  writes:
> On Wed, Jan 29, 2014 at 8:48 PM, KONDO Mitsumasa
>  wrote:
>> method   |  try1  |  try2  |  try3  | degrade performance ratio
>> -
>> method 1 | 6.546  | 6.558  | 6.638  | 0% (reference score)
>> method 2 | 6.527  | 6.556  | 6.574  | 1%
>> peter 1  | 5.204  | 5.203  | 5.216  |20%
>> peter 2  | 4.241  | 4.236  | 4.262  |35%
>> 
>> method 5 | 5.931  | 5.848  | 5.872  |11%
>> method 6 | 5.794  | 5.792  | 5.776  |12%

> So, if we compare the old pg_stat_statements and old default with the
> new pg_stat_statements and the new default (why not? The latter still
> uses less shared memory than the former), by the standard of this
> benchmark there is a regression of about 20%. But you also note that
> there is an 11% "regression" in the old pg_stat_statements against
> *itself* when used with a higher pg_stat_statements.max setting of
> 5,000. This is completely contrary to everyone's prior understanding
> that increasing the size of the hash table had a very *positive*
> effect on performance by relieving cache pressure and thereby causing
> less exclusive locking for an entry_dealloc().

> Do you suppose that this very surprising result might just be because
> this isn't a very representative benchmark? Nothing ever has the
> benefit of *not* having to exclusive lock.

If I understand this test scenario properly, there are no duplicate
queries, so that each iteration creates a new hashtable entry (possibly
evicting an old one).  And it's a single-threaded test, so that there
can be no benefit from reduced locking.

I don't find the results particularly surprising given that.  We knew
going in that the external-texts patch would slow down creation of a new
hashtable entry: there's no way that writing a query text to a file isn't
slower than memcpy'ing it into shared memory.  The performance argument
for doing it anyway is that by greatly reducing the size of hashtable
entries, it becomes more feasible to size the hashtable large enough so
that it's seldom necessary to evict hashtable entries.  It's always going
to be possible to make the patch look bad by testing cases where no
savings in hashtable evictions is possible; which is exactly what this
test case does.  But I don't think that's relevant to real-world usage.
pg_stat_statements isn't going to be useful unless there's a great deal of
repeated statement execution, so what we should be worrying about is the
case where a table entry exists not the case where it doesn't.  At the
very least, test cases where there are *no* repeats are not representative
of cases people would be using pg_stat_statements with.

As for the measured slowdown with larger hashtable size (but still smaller
than the number of distinct queries in the test), my money is on the
larger table not fitting in L3 cache or something like that.

regards, tom lane


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

Re: [HACKERS] GIN improvements part2: fast scan

[Heikki Linnakangas]

On 01/30/2014 01:53 AM, Tomas Vondra wrote:

(3) A file with explain plans for 4 queries suffering ~2x slowdown,
 and explain plans with 9.4 master and Heikki's patches is available
 here:

   http://www.fuzzy.cz/tmp/gin/queries.txt

 All the queries have 6 common words, and the explain plans look
 just fine to me - exactly like the plans for other queries.

 Two things now caught my eye. First some of these queries actually
 have words repeated - either exactly like "database & database" or
 in negated form like "!anything & anything". Second, while
 generating the queries, I use "dumb" frequency, where only exact
 matches count. I.e. "write != written" etc. But the actual number
 of hits may be much higher - for example "write" matches exactly
 just 5% documents, but using @@ it matches more than 20%.

 I don't know if that's the actual cause though.


I tried these queries with the data set you posted here: 
http://www.postgresql.org/message-id/52e4141e.60...@fuzzy.cz. The reason 
for the slowdown is the extra consistent calls it causes. That's 
expected - the patch certainly does call consistent in situations where 
it didn't before, and if the "pre-consistent" checks are not able to 
eliminate many tuples, you lose.


So, what can we do to mitigate that? Some ideas:

1. Implement the catalog changes from Alexander's patch. That ought to 
remove the overhead, as you only need to call the consistent function 
once, not "both ways". OTOH, currently we only call the consistent 
function if there is only one unknown column. If with the catalog 
changes, we always call the consistent function even if there are more 
unknown columns, we might end up calling it even more often.


2. Cache the result of the consistent function.

3. Make the consistent function cheaper. (How? Magic?)

4. Use some kind of a heuristic, and stop doing the pre-consistent 
checks if they're not effective. Not sure what the heuristic would look 
like.


The caching we could easily do. It's very simple and very effective, as 
long as the number of number of entries is limited. The amount of space 
required to cache all combinations grows exponentially, so it's only 
feasible for up to 10 or so entries.


- 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] inherit support for foreign tables

[Tom Lane]

Shigeru Hanada  writes:
> At the moment we don't use attstorage for foreign tables, so allowing
> SET STORAGE against foreign tables never introduce visible change
> except \d+ output of foreign tables.  But IMO such operation should
> not allowed because users would be confused.  So I changed
> ATExecSetStorage() to skip on foreign tables.

I think this is totally misguided.  Who's to say that some weird FDW
might not pay attention to attstorage?  I could imagine a file-based
FDW using that to decide whether to compress columns, for instance.
Admittedly, the chances of that aren't large, but it's pretty hard
to argue that going out of our way to prevent it is a useful activity.

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] Add min and max execute statement time in pg_stat_statement

[Tom Lane]

KONDO Mitsumasa  writes:
> (2014/01/30 8:29), Tom Lane wrote:
>> If we felt that min/max were of similar value to stddev then this
>> would be mere nitpicking.  But since people seem to agree they're
>> worth less, I'm thinking the cost/benefit ratio isn't there.

> Why do you surplus worried about cost in my patch? Were three double variables
> assumed a lot of memory, or having lot of calculating cost? My test result
> showed LWlock bottele-neck is urban legend. If you have more fair test
> pattern, please tell me, I'll do it if the community will do fair judge.

[ shrug... ]  Standard pgbench tests are useless for measuring microscopic
issues like this one; whatever incremental cost is there will be swamped
by client communication and parse/plan overhead.  You may have proven that
the overhead isn't 10% of round-trip query time, but we knew that without
testing.

If I were trying to measure the costs of these changes, I'd use short
statements executed inside loops in a plpgsql function.  And I'd
definitely do it on a machine with quite a few cores (and a running
session on each one), so that spinlock contention might conceivably
happen often enough to be measurable.

If that still says that the runtime cost is down in the noise, we
could then proceed to debate whether min/max are worth a 10% increase
in the size of the shared pgss hash table.  Because by my count,
that's about what two more doubles would be, now that we removed the
query texts from the hash table --- a table entry is currently 21
doublewords (at least on 64-bit machines) and we're debating increasing
it to 22 or 24.

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] jsonb and nested hstore

[Andrew Dunstan]

ok, great.  This is really fabulous.  So far most everything feels
natural and good.

I see something odd in terms of the jsonb use case coverage. One of
the major headaches with json deserialization presently is that
there's no easy way to easily move a complex (record- or array-
containing) json structure into a row object.  For example,

create table bar(a int, b int[]);
postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
[1,2]}'::jsonb, false);
ERROR:  cannot populate with a nested object unless use_json_as_text 
is true


If find the use_json_as_text argument here to be pretty useless
(unlike in the json_build to_record variants where it least provides
some hope for an escape hatch) for handling this since it will just
continue to fail:

postgres=# select jsonb_populate_record(null::bar, '{"a": 1, "b":
[1,2]}'::jsonb, true);
ERROR:  missing "]" in array dimensions

OTOH, the nested hstore handles this no questions asked:

postgres=# select * from populate_record(null::bar, '"a"=>1,
"b"=>{1,2}'::hstore);
  a |   b
---+---
  1 | {1,2}

So, if you need to convert a complex json to a row type, the only
effective way to do that is like this:
postgres=# select* from  populate_record(null::bar, '{"a": 1, "b":
[1,2]}'::json::hstore);
  a |   b
---+---
  1 | {1,2}

Not a big deal really. But it makes me wonder (now that we have the
internal capability of properly mapping to a record) why *both* the
json/jsonb populate record variants shouldn't point to what the nested
hstore behavior is when the 'as_text' flag is false.  That would
demolish the error and remove the dependency on hstore in order to do
effective rowtype mapping.  In an ideal world the json_build
'to_record' variants would behave similarly I think although there's
no existing hstore analog so I'm assuming it's a non-trival amount of
work.

Now, if we're agreed on that, I then also wonder if the 'as_text'
argument needs to exist at all for the populate functions except for
backwards compatibility on the json side (not jsonb).  For non-complex
structures it does best effort casting anyways so the flag is moot.



Well, I could certainly look at making the populate_record{set} and 
to_record{set} logic handle types that are arrays or composites inside 
the record. It might not be terribly hard to do - not sure.






A quick analysis suggests that this is fixable with fairly minimal 
disturbance in the jsonb case. In the json case it would probably 
involve reparsing the inner json. That's probably doable, because the 
routines are all reentrant, but not likely to be terribly efficient. It 
will also be a deal more work.


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] Suspicion of a compiler bug in clang: using ternary operator in ereport()

[Christian Kruse]

Hi,

On 30/01/14 10:01, Tom Lane wrote:
> While I haven't actually read the gettext docs, I'm pretty sure that
> gettext() is defined to preserve errno.  It's supposed to be something
> that you can drop into existing printf's without thinking, and if
> it mangled errno that would certainly not be the case.

Thanks for your explanation. I verified reading the man page and it
explicitly says:

ERRORS
   errno is not modified.


Best regards,

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



pgpHCNemua8zx.pgp
Description: PGP signature

Re: [HACKERS] Suspicion of a compiler bug in clang: using ternary operator in ereport()

[Alvaro Herrera]

Tom Lane wrote:
> Christian Kruse  writes:
> > Have a look at the psprintf() call: we first have a _("failed to look
> > up effective user id %ld: %s") as an argument, then we have a (long)
> > user_id and after that we have a ternary expression using errno. Isn't
> > it possible that the first _() changes errno?
> 
> While I haven't actually read the gettext docs, I'm pretty sure that
> gettext() is defined to preserve errno.  It's supposed to be something
> that you can drop into existing printf's without thinking, and if
> it mangled errno that would certainly not be the case.

It specifically says:

ERRORS
   errno is not modified.


-- 
Álvaro Herrerahttp://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] Suspicion of a compiler bug in clang: using ternary operator in ereport()

[Tom Lane]

Christian Kruse  writes:
> Have a look at the psprintf() call: we first have a _("failed to look
> up effective user id %ld: %s") as an argument, then we have a (long)
> user_id and after that we have a ternary expression using errno. Isn't
> it possible that the first _() changes errno?

While I haven't actually read the gettext docs, I'm pretty sure that
gettext() is defined to preserve errno.  It's supposed to be something
that you can drop into existing printf's without thinking, and if
it mangled errno that would certainly not be the case.

If this isn't true, we've got probably hundreds of places that would
need fixing, most of them of the form printf(_(...), strerror(errno)).

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] INTERVAL overflow detection is terribly broken

[Bruce Momjian]

On Mon, Jan 27, 2014 at 10:48:16PM -0500, Bruce Momjian wrote:
> On Mon, Jan 27, 2014 at 07:19:21PM -0500, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > Oh, one odd thing about this patch.  I found I needed to use INT64_MAX,
> > > but I don't see it used anywhere else in our codebase.  Is this OK?  Is
> > > there a better way?
> > 
> > Most of the overflow tests in int.c and int8.c are coded to avoid relying
> > on the MIN or MAX constants; which seemed like better style at the time.
> 
> Yes, I looked at those but they seemed like overkill for interval.  For
> a case where there was an int64 multiplied by a double, then cast back
> to an int64, I checked the double against INT64_MAX before casting to an
> int64.
> 
> > I'm not sure whether relying on INT64_MAX to exist is portable.
> 
> The only use I found was in pgbench:
> 
>   #ifndef INT64_MAX
>   #define INT64_MAX   INT64CONST(0x7FFF)
>   #endif
> 
> so I have just added that to my patch, and INT64_MIN:
> 
>   #ifndef INT64_MIN
>   #define INT64_MIN   (-INT64CONST(0x7FFF) - 1)
>   #endif
> 
> This is only used for HAVE_INT64_TIMESTAMP.

Adjusted patch applied for PG 9.4.  Thanks for the report.

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

  + Everyone has their own god. +


-- 
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] Prohibit row-security + inheritance in 9.4?

[Stephen Frost]

Craig, all,

* Craig Ringer (cr...@2ndquadrant.com) wrote:
> I'm considering just punting on inheritance for 9.4, adding checks to
> prohibit inheritance from being added to a rel with row security and
> prohibiting any rel with inheritance from being given a row-security policy.

I'm alright with punting on it for 9.4, certainly we need to get the
row-security work done soon if we're going to have any chance of having
this in 9.4 (and I have to admit, I suspect we're pushing the envelope
here already wrt getting row-security into 9.4).

> Earlier discussions seemed to settle on each relation having its own
> row-security quals applied independently. So quals on a parent rel did
> not cascade down to child rels.

This strikes me as a bit odd- isn't this against how we handle the GRANT
system when it comes to inheiritance?  That is to say- if you have
access to query the parent, then you'll get rows back from the child and
I believe everyone feels that makes perfect sense.

> That gives you a consistent view of the data in a child rel when
> querying via the parent vs directly, which is good. It's surprising when
> you query via the parent and see rows the parent's row-security
> qualifier doesn't permit, but that surprising behaviour is consistent
> with other surprising things in inheritance, like a primary key on the
> parent not constraining rows inserted into children.

While it agrees with the PK issue, that's a data consistency concern
rather than a security concern, and to contrast this with our existing
security model- if you don't have access to the parent then you can't
query against it; doesn't matter if you have access to the child or not.

> The trouble is that this isn't going to work when applying row-security
> rules using the security barriers support. Security quals get expanded
> before inheritance expansion so that they're copied to all child rels.
> Just what you'd expect when querying a relation that's a parent of an
> inheritance tree via a view.

Right.

> It's what you'd expect to happen when querying a parent rel with
> row-security, too. Parent quals are applied to children. But that then
> gives you an inconsistent view of a rel's contents based on whether you
> query it via a parent or directly.

... Just how our existing GRANT system works.

If you want to constrain the children in the same way as the parent,
then the user can add to the row-security on the children to match the
parent.  If the user wants to have one view for the entire inheiritance
tree then they need to only implement the row-security on the parent and
not grant any access for users on the children (or, if they're paranoid,
add row-security to the children which are essentially deny-all).

If we force everything to behave the same between querying the parent
and querying the children then we cut off various scenarios of
partitioning where users are allowed to query specific children but not
the parent or query the parent to get things they're not able to see
directly from the children.  This matches the "each rel stands alone"
comment below, from my perspective, but seems to result in a different
behavior from what you describe here.  Perhaps that's because "each rel
standing alone", when it comes to inheiritance, looks at the parent as
a simple combination of all the rows from the parent plus the children
and then filtering them based on the row-security of the parent.

> I embarked upon that because of the concern that was expressed here
> about the way KaiGai's row-security patch fiddles directly with
> remapping attributes during planning; rebuilding row-security on top of
> updatable security barrier views was seen as a cleaner approach.

I agree with this approach of adding row-security over top of updatable
security barrier views.

> 1. Prohibit (in CREATE TABLE ... INHERITS, ALTER TABLE ... INHERITS, and
> ALTER TABLE ... SET ROW SECURITY) any parent or child rel from having
> row-security policy, i.e. punt it until 9.5;
> 
> 2. Do another round of security qual expansion that fetches quals from
> pg_rowsecurity *after* inheritance expansion, giving us the
> each-relation-stands-alone behaviour;

Again, this doesn't match what I think of as
"each-relation-stands-alone".

> 3. Accept the inconsistent view of child rel contents in exchange for
> the otherwise sane behaviour of applying a parent's quals to children;
> document that if you don't want this, don't grant users direct access to
> the child tables;

Or write identical row-security rules on the children (or deny-all
row-security to force the user to use the parent).

> 4. attempt to pull quals from parents when querying a child rel directly.

That strikes me as borderline insane. ;)

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] WIP patch (v2) for updatable security barrier views

[Simon Riggs]

On 30 January 2014 11:55, Dean Rasheed  wrote:

> Hmm, looks like this is a pre-existing bug.
>
> The first thing I tried was to reproduce it using SQL, so I used a
> RULE to turn a DELETE into a SELECT FOR UPDATE. This is pretty dumb,
> but it shows the problem:
>
> CREATE TABLE foo (a int);
> CREATE RULE foo_del_rule AS ON DELETE TO foo
>   DO INSTEAD SELECT * FROM foo FOR UPDATE;
> DELETE FROM foo;
>
> ERROR:  no relation entry for relid 1
>
> So I think this should be fixed independently of the updatable s.b. view code.

Looks to me there isn't much use case for turning DML into a SELECT -
where would we expect the output to go to if the caller wasn't
prepared to handle the result rows?

IMHO we should simply prohibit such cases rather than attempt to fix
the fact they don't work. We know for certain nobody relies on this
behaviour because its broken already.

-- 
 Simon Riggs   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] Add CREATE support to event triggers

[Dimitri Fontaine]

Hi,

Alvaro Herrera  writes:
> So here's a patch implementing the ideas expressed in this thread.
> There are two new SQL functions:

I spent some time reviewing the patch and tried to focus on a higher
level review, as I saw Andres already began with the lower level stuff.

The main things to keep in mind here are:

  - this patch enables running Event Triggers anytime a new object is
created, in a way that the user code is run once the object already
made it through the catalogs;

  - the Event Trigger code has access to the full details about every
created object, so it's not tied to a command but really the fact
that an object just was created in the catalogs;

   (it's important with serial and primary key sub-commands)

  - facilities are provided so that it's possible to easily build an SQL
command that if executed would create the exact same object again;

  - the facilities around passing the created object details and
building a SQL command are made in such a way that it's trivially
possible to "hack" away the captured objects properties before
producing again a new SQL command.

After careful study and thinking, it appears to me that the proposed
patch addresses the whole range of features we expect here, and is both
flexible enough for the users and easy enough to maintain.

The event being fired once the objects are available in the catalogs
makes it possible for the code providing the details in the JSON format
to complete the parsetree with necessary information.

Current state of the patch is not ready for commit yet, independant of
code details some more high-level work needs to be done:

  - preliminary commit

It might be a good idea to separate away some pre-requisites of this
patch and commit them separately: the OID publishing parts and
allowing an Event Trigger to get fired after CREATE but without the
extra detailed JSON formated information might be good commits
already, and later add the much needed details about what did
happen.

  - document the JSON format

I vote for going with the proposed format, because it actually
allows to implement both the audit and replication features we want,
with the capability of hacking schema, data types, SQL
representation etc; and because I couldn't think of anything better
than what's proposed here ;-)

  - other usual documentation

I don't suppose I have to expand on what I mean here…

  - fill-in other commands

Not all commands are supported in the submitted patch. I think once
we have a clear documentation on the general JSON formating and how
to use it as a user, we need to include support for all CREATE
commands that we have.

I see nothing against extending when this work has to bo done until
after the CF, as long as it's fully done before beta. After all it's
only about filling in minutia at this point.

  - review the JSON producing code

It might be possible to use more of the internal supports for JSON
now that the format is freezing.

  - regression tests

The patch will need some. The simpler solution is to add a new
regression test entry and exercise all the CREATE commands in there,
in a specific schema, activating an event trigger that outputs the
JSON detailed information each time (the snitch() example).

Best would be to have some "pretty" indented output of the JSON to
help with reviewing diffs, and I have to wonder about JSON object
inner-ordering if we're going to do that.

No other ideas on this topic from me.

> The JSON parsing is done in event_trigger.c.  This code should probably
> live elsewhere, but I again hesitate to put it in json.c or jsonfuncs.c,
> at least until some discussion about its general applicability takes
> place.

I see that as useful enough if it can be made to work without the
special "fmt" fields somehow, with a nice default formatting ability.

In particular, being able to build some intermediate object with
json_agg then call the formating/expanding function on top of that might
be quite useful.

That said, I don't think we have enough time to attack this problem now,
I think it would be wiser to address your immediate problem separately
in your patch and clean it later (next release) with sharing code and
infrastructure and offering a more generally useful tool. At least we
will have some feedback about the Event Trigger specific context then.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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: option --if-exists for pg_dump

[Pavel Stehule]

2014-01-30 Jeevan Chalke :

> OK.
>
> Assigned it to committer.
>
> Thanks for the hard work.
>

Thank you for review

Pavel


>
>
> On Thu, Jan 30, 2014 at 6:16 PM, Pavel Stehule wrote:
>
>> Hello
>>
>> All is ok
>>
>> Thank you
>>
>> Pavel
>>
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

Re: [HACKERS] patch: option --if-exists for pg_dump

[Jeevan Chalke]

OK.

Assigned it to committer.

Thanks for the hard work.


On Thu, Jan 30, 2014 at 6:16 PM, Pavel Stehule wrote:

> Hello
>
> All is ok
>
> Thank you
>
> Pavel
>

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] patch: option --if-exists for pg_dump

[Pavel Stehule]

Hello

All is ok

Thank you

Pavel


2014-01-30 Jeevan Chalke :

> Hi Pavel,
>
> Now patch looks good to me and I think it is in good shape to pass it on to
> the committer as well.
>
> However, I have
> - Tweaked few comments
> - Removed white-space errors
> - Fixed typos
> - Fixed indentation
>
> Attached patch with my changes. However entire design and code logic is
> untouched.
>
> Please have a quick look and pass it on to committor if you have no issues
> OR
> ask me to assign it to the committor, NO issues either way.
>
> Feel free to reject my changes if you didn't like them.
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

Re: [HACKERS] [bug fix] "pg_ctl stop" times out when it should respond quickly

[MauMau]

From: "Ronan Dunklau" 
There is no regression tests covering this bugfix, althought I don't know 
if

it would be practical to implement them.


Thanks for reviewing the patch.  I'm glad to know that it seems OK. 
Unfortunately, the current regression test system cannot handle the testing 
of pg_ctl.


Regards
MauMau




--
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 fix] postgres.exe fails to start on Windows Server 2012 due to ASLR

[MauMau]

I'm sorry for the late reply.  I was unable to access email.

From: "Craig Ringer" 

On 01/24/2014 06:42 PM, MauMau wrote:

The customer is using 64-bit PostgreSQL 9.1.x


Which "x"?


9.1.6.

Does this issue also occur on 9.3.2, or in 9.4 HEAD, when tested on 
Win2k12?


I'm sure it should.  The release note doesn't have any reference to this 
issue.  Another user who reported this issue in pgsql-general experienced 
this with 9.2.4.


In addition, Another customer reported to me that psql failed to connect to 
PostgreSQL 9.1.9 running on Windows Server 2012 R2, about once out of 7 
times.  I believe this needs immediate fix.


Regards
MauMau



--
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: option --if-exists for pg_dump

[Jeevan Chalke]

Hi Pavel,

Now patch looks good to me and I think it is in good shape to pass it on to
the committer as well.

However, I have
- Tweaked few comments
- Removed white-space errors
- Fixed typos
- Fixed indentation

Attached patch with my changes. However entire design and code logic is
untouched.

Please have a quick look and pass it on to committor if you have no issues
OR
ask me to assign it to the committor, NO issues either way.

Feel free to reject my changes if you didn't like them.

Thanks
-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 8d45f24..c39767b 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -650,6 +650,16 @@ PostgreSQL documentation
  
 
  
+  --if-exists
+  
+   
+It uses conditional commands (with IF EXISTS
+clause) for cleaning database schema.
+   
+  
+ 
+
+ 
   --disable-dollar-quoting
   

diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 5c6a101..ba6583d 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -301,6 +301,16 @@ PostgreSQL documentation
  
 
  
+  --if-exists
+  
+   
+It uses conditional commands (with IF EXISTS
+clause) for cleaning database schema.
+   
+  
+ 
+
+ 
   --inserts
   

diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 717da42..55326d5 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -490,6 +490,16 @@
  
 
  
+  --if-exists
+  
+   
+It uses conditional commands (with IF EXISTS
+clause) for cleaning database schema.
+   
+  
+ 
+
+ 
   --no-data-for-failed-tables
   

diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 6927968..83f7216 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -113,6 +113,7 @@ typedef struct _restoreOptions
 	char	   *superuser;		/* Username to use as superuser */
 	char	   *use_role;		/* Issue SET ROLE to this */
 	int			dropSchema;
+	int			if_exists;
 	const char *filename;
 	int			dataOnly;
 	int			schemaOnly;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7fc0288..c08a0d3 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -413,8 +413,84 @@ RestoreArchive(Archive *AHX)
 /* Select owner and schema as necessary */
 _becomeOwner(AH, te);
 _selectOutputSchema(AH, te->namespace);
-/* Drop it */
-ahprintf(AH, "%s", te->dropStmt);
+
+if (*te->dropStmt != '\0')
+{
+	/* Inject IF EXISTS clause to DROP part when required. */
+	if (ropt->if_exists)
+	{
+		char buffer[40];
+		char *mark;
+		char *dropStmt = te->dropStmt;
+		PQExpBuffer ftStmt = createPQExpBuffer();
+
+		/*
+		 * Need to inject IF EXISTS clause after ALTER TABLE
+		 * part in ALTER TABLE .. DROP statement
+		 */
+		if (strncmp(dropStmt, "ALTER TABLE", 11) == 0)
+		{
+			/*
+			 * Prepare fault tolerant statement, but ensure
+			 * unique IF EXISTS option.
+			 */
+			if (strncmp(dropStmt + 11, " IF EXISTS", 10) != 0)
+			{
+appendPQExpBuffer(ftStmt,
+  "ALTER TABLE IF EXISTS");
+dropStmt = dropStmt + 11;
+			}
+		}
+
+		/*
+		 * A content of te->desc is usually substring of a DROP
+		 * statement with one related exception - CONSTRAINTs.
+		 *
+		 * Independent to previous sentence, ALTER TABLE ..
+		 * ALTER COLUMN .. DROP DEFAULT statement does not
+		 * support IF EXISTS clause and therefore it should not
+		 * be injected.
+		 */
+		if (strcmp(te->desc, "DEFAULT") != 0)
+		{
+			if (strcmp(te->desc, "CONSTRAINT") == 0 ||
+strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
+strcmp(te->desc, "FK CONSTRAINT") == 0)
+strcpy(buffer, "DROP CONSTRAINT");
+			else
+snprintf(buffer, sizeof(buffer), "DROP %s",
+	 te->desc);
+
+			mark = strstr(dropStmt, buffer);
+		}
+		else
+			mark = NULL;
+
+		if (mark != NULL)
+		{
+			size_t l = strlen(buffer);
+
+			/* Inject IF EXISTS clause if not alredy present */
+			if (strncmp(mark + l, " IF EXISTS", 10) != 0)
+			{
+*mark = '\0';
+
+appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
+  dropStmt, buffer, mark + l);
+			}
+			else
+appendPQExpBuffer(ftStmt, "%s", dropStmt);
+		}
+		else
+			appendPQExpBuffer(ftStmt, "%s", dropStmt);
+
+		ahprintf(AH, "%s", ftStmt->data);
+
+		destroyPQExpBuffer(ftStmt);
+	}
+	else
+		ahpr

Re: [HACKERS] [PATCH] Support for pg_stat_archiver view

[Fujii Masao]

On Wed, Jan 29, 2014 at 12:35 PM, Michael Paquier
 wrote:
> On Wed, Jan 29, 2014 at 10:55 AM, Fujii Masao  wrote:
>> On Wed, Jan 29, 2014 at 3:07 AM, Alvaro Herrera
>>  wrote:
>>>
>>> Anybody knows about this patch?
>>> http://www.postgresql.org/message-id/508dfec9.4020...@uptime.jp
>>
>> Though I'm not sure whether Nagayasu is still working on that patch,
>> it's worth thinking to introduce that together with pg_stat_archiver.
> This patch uses globalStats to implement the new stat fields for
> walwriter, I think that we should use a different structure for
> clarity and to be in-line with what is done now with the archiver
> part.

Is there clear reason why we should define separate structure for each
global stats view?
If we introduce something like pg_stat_walwriter later, it seems
better to use only one
structure, i.e., globalStats, for all global stats views. Which would
simplify the code and
can reduce the number of calls of fwrite()/fread().

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] WIP patch (v2) for updatable security barrier views

[Dean Rasheed]

On 30 January 2014 05:36, Craig Ringer  wrote:
> On 01/29/2014 08:29 PM, Dean Rasheed wrote:
>> On 29 January 2014 11:34, Craig Ringer  wrote:
>>> On 01/23/2014 06:06 PM, Dean Rasheed wrote:
 On 21 January 2014 09:18, Dean Rasheed  wrote:
> Yes, please review the patch from 09-Jan
> (http://www.postgresql.org/message-id/CAEZATCUiKxOg=voovja2s6g-sixzzxg18totggp8zobq6qn...@mail.gmail.com).
>

 After further testing I found a bug --- it involves having a security
 barrier view on top of a base relation that has a rule that rewrites
 the query to have a different result relation, and possibly also a
 different command type, so that the securityQuals are no longer on the
 result relation, which is a code path not previously tested and the
 rowmark handling was wrong. That's probably a pretty obscure case in
 the context of security barrier views, but that code path would be
 used much more commonly if RLS were built on top of this. Fortunately
 the fix is trivial --- updated patch attached.
>>>
>>> This is the most recent patch I see, and the one I've been working on
>>> top of.
>>>
>>> Are there any known tests that this patch fails?
>>>
>>
>> None that I've been able to come up with.
>
> I've found an issue. I'm not sure if it can be triggered from SQL, but
> it affects in-code users who add their own securityQuals.
>
> expand_security_quals fails to update any rowMarks that may point to a
> relation being expanded. If the relation isn't the target relation, this
> causes a rowmark to refer to a RTE with no relid, and thus failure in
> the executor.
>
> Relative patch against updatable s.b. views attached (for easier
> reading), along with a new revision of updatable s.b. views that
> incorporates the patch.
>
> This isn't triggered by FOR SHARE / FOR UPDATE rowmark on a security
> barrier view because the flattening to securityQuals and re-expansion in
> the optimizer is only done for _target_ security barrier views. For
> target views, different rowmark handling applies, so they don't trigger
> it either.
>
> This is triggered by adding securityQuals to a non-target relation
> during row-security processing, though.
>

Hmm, looks like this is a pre-existing bug.

The first thing I tried was to reproduce it using SQL, so I used a
RULE to turn a DELETE into a SELECT FOR UPDATE. This is pretty dumb,
but it shows the problem:

CREATE TABLE foo (a int);
CREATE RULE foo_del_rule AS ON DELETE TO foo
  DO INSTEAD SELECT * FROM foo FOR UPDATE;
DELETE FROM foo;

ERROR:  no relation entry for relid 1

So I think this should be fixed independently of the updatable s.b. view code.

Regards,
Dean


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

Re: [HACKERS] ALTER TABLESPACE ... MOVE ALL TO ...

[Fujii Masao]

On Tue, Jan 21, 2014 at 1:33 AM, Simon Riggs  wrote:
> On 20 January 2014 17:00, Stephen Frost  wrote:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> What if you're a superuser and you want to move everybody's objects
>>> (perhaps in preparation for dropping the tablespace)?  I think there's
>>> value in both the ALL and OWNED forms.
>>
>> A superuser is considered to 'own' all objects and so 'ALL' and 'OWNED'
>> above would be the same when issued by a superuser, in the current
>> implementation.
>>
>> Looking at DROP OWNED and REASSIGN OWNED, they operate at the more
>> specific level of "OWNED" == "relowner" rather than if the role is
>> considered an 'owner' of the object through role membership, as you are
>> implying above.
>>
>> As such, I'll rework this to be more in-line with the existing OWNED BY
>> semantics of REASSIGN OWNED BY and DROP OWNED BY, which means we'd have:
>>
>> ALTER TABLESPACE name MOVE [ ALL | OWNED [ BY reluser ] ]
>> [ TABLES | INDEXES | MATERIALIZED VIEWS ] TO name opt_nowait
>>
>> eg:
>>
>> ALTER TABLESPACE tblspc1 MOVE ALL TO tblspc2;
>> ALTER TABLESPACE tblspc1 MOVE OWNED TO tblspc2;
>> ALTER TABLESPACE tblspc1 MOVE OWNED BY myrole TO tblspc2;
>> ALTER TABLESPACE tblspc1 MOVE TABLES OWNED BY myrole TO tblspc2;
>> ALTER TABLESPACE tblspc1 MOVE ALL OWNED BY myrole TO tblspc2;
>
> Sounds great, thanks.

We should add the tab-completion for ALTER TABLESPACE MOVE?
Attached does that.

Regards,

-- 
Fujii Masao
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***
*** 1622,1633  psql_completion(char *text, int start, int end)
  		COMPLETE_WITH_CONST("IDENTITY");
  	}
  
! 	/* ALTER TABLESPACE  with RENAME TO, OWNER TO, SET, RESET */
  	else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
  			 pg_strcasecmp(prev2_wd, "TABLESPACE") == 0)
  	{
  		static const char *const list_ALTERTSPC[] =
! 		{"RENAME TO", "OWNER TO", "SET", "RESET", NULL};
  
  		COMPLETE_WITH_LIST(list_ALTERTSPC);
  	}
--- 1622,1633 
  		COMPLETE_WITH_CONST("IDENTITY");
  	}
  
! 	/* ALTER TABLESPACE  with RENAME TO, OWNER TO, SET, RESET, MOVE */
  	else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
  			 pg_strcasecmp(prev2_wd, "TABLESPACE") == 0)
  	{
  		static const char *const list_ALTERTSPC[] =
! 		{"RENAME TO", "OWNER TO", "SET", "RESET", "MOVE", NULL};
  
  		COMPLETE_WITH_LIST(list_ALTERTSPC);
  	}
***
*** 1649,1654  psql_completion(char *text, int start, int end)
--- 1649,1675 
  
  		COMPLETE_WITH_LIST(list_TABLESPACEOPTIONS);
  	}
+ 	/* ALTER TABLESPACE  MOVE ALL|TABLES|INDEXES|MATERIALIZED VIEWS */
+ 	else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
+ 			 pg_strcasecmp(prev3_wd, "TABLESPACE") == 0 &&
+ 			 pg_strcasecmp(prev_wd, "MOVE") == 0)
+ 	{
+ 		static const char *const list_TABLESPACEMOVETARGETS[] =
+ 		{"ALL", "TABLES", "INDEXES", "MATERIALIZED VIEWS", NULL};
+ 
+ 		COMPLETE_WITH_LIST(list_TABLESPACEMOVETARGETS);
+ 	}
+ 	else if ((pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
+ 			  pg_strcasecmp(prev2_wd, "MOVE") == 0) ||
+ 			 (pg_strcasecmp(prev5_wd, "TABLESPACE") == 0 &&
+ 			  pg_strcasecmp(prev3_wd, "MOVE") == 0 &&
+ 			  pg_strcasecmp(prev2_wd, "MATERIALIZED") == 0))
+ 	{
+ 		static const char *const list_TABLESPACEMOVEOPTIONS[] =
+ 		{"OWNED BY", "TO", NULL};
+ 
+ 		COMPLETE_WITH_LIST(list_TABLESPACEMOVEOPTIONS);
+ 	}
  
  	/* ALTER TEXT SEARCH */
  	else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
***
*** 2559,2566  psql_completion(char *text, int start, int end)
  	 * but we may as well tab-complete both: perhaps some users prefer one
  	 * variant or the other.
  	 */
! 	else if (pg_strcasecmp(prev3_wd, "FETCH") == 0 ||
! 			 pg_strcasecmp(prev3_wd, "MOVE") == 0)
  	{
  		static const char *const list_FROMIN[] =
  		{"FROM", "IN", NULL};
--- 2580,2588 
  	 * but we may as well tab-complete both: perhaps some users prefer one
  	 * variant or the other.
  	 */
! 	else if ((pg_strcasecmp(prev3_wd, "FETCH") == 0 ||
! 			  pg_strcasecmp(prev3_wd, "MOVE") == 0) &&
! 			 pg_strcasecmp(prev_wd, "TO") != 0)
  	{
  		static const char *const list_FROMIN[] =
  		{"FROM", "IN", NULL};

-- 
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] Performance Improvement by reducing WAL for Update Operation

[Amit Kapila]

On Thu, Jan 30, 2014 at 12:23 PM, Amit Kapila  wrote:
> On Wed, Jan 29, 2014 at 8:13 PM, Heikki Linnakangas
>  wrote:
>
> Few observations in patch (back-to-pglz-like-delta-encoding-1):
>
> 1.
> +#define pgrb_hash_unroll(_p, hindex) \
> + hindex = hindex ^ ((_p)[0] << 8)
>
> shouldn't it shift by 6 rather than by 8.
>
> 2.
> + if (bp - bstart >= result_max)
> + return false;
>
> I think for nothing or lesser match case it will traverse whole tuple.
> Can we optimize such that if there is no match till 75%, we can bail out.
> Ideally, I think if we don't find any match in first 50 to 60%, we should
> come out.
>
> 3. pg_rbcompress.h is missing.
>
> I am still working on patch back-to-pglz-like-delta-encoding-1 to see if
> it works well for all cases, but thought of sharing what I have done till
> now to you.
>
> After basic verification of  back-to-pglz-like-delta-encoding-1, I will
> take the data with both the patches and report the same.

Apart from above, the only other thing which I found problematic is
below code in find match

+ while (*ip == *hp && thislen < maxlen)
+ thislen++;

It should be
while (*ip++ == *hp++ && thislen < maxlen)

Please confirm.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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] pg_upgrade: make the locale comparison more tolerating

[Rushabh Lathia]

Hi Pavel,

I looked at your latest cleanup patch. Yes its looks more cleaner in term
equivalent_locale & equivalent_encoding separate functions.

I did run the test again on latest patch and all looks good.

Marking it as ready for committer.

Regards,



On Fri, Jan 24, 2014 at 9:49 PM, Pavel Raiskup  wrote:

> Rushabh, really sorry I have to re-create the patch and thanks a
> lot for looking at it!
>
> Looking at the patch once again, I see that there were at least two
> problems.  Firstly, I used the equivalent_locale function also on the
> encoding values.  Even if that should not cause bugs (as it should result
> in strncasecmp anyway), it was not pretty..
>
> The second problem was assuming that the locale specifier "A" is not
> longer then locale specifier B.  Comparisons like 'en_US.utf8' with
> 'en_US_.utf8' would result in success.  Bug resulting from this mistake is
> not real probably but it is not nice anyway..
>
> Rather cleaning the patch once more, attached,
> Pavel
>



-- 
Rushabh Lathia

Re: [HACKERS] patch: option --if-exists for pg_dump

[Pavel Stehule]

Hello

patch with updated comment

regards

Pavel



2014-01-30 Jeevan Chalke :

> Hi Pavel,
>
>  it should be fixed by
>>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b152c6cd0de1827ba58756e24e18110cf902182acommit
>>>
>>
> Ok. Good.
> Sorry I didn't update my sources. Done now. Thanks
>
>
>>
>>>

 Also, I didn't quite understand these lines of comments:

 /*
  * Descriptor string (te-desc) should not be
 same as object
  * specifier for DROP STATEMENT. The DROP
 DEFAULT has not
  * IF EXISTS clause - has not sense.
  */

 Will you please rephrase ?

>>>
>>> I can try it - .
>>>
>>> A content of te->desc is usually substring of DROP STATEMENT with one
>>> related exception - CONSTRAINT.
>>> Independent to previous sentence - ALTER TABLE ALTER COLUMN DROP DEFAULT
>>> doesn't support IF EXISTS - and therefore it should not be injected.
>>>
>>
>> is it ok?
>>
>
> Fine with me.
>
> Thanks
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>
commit e72e3ae9003d2c8eea0e687122a2f31d21674b81
Author: Pavel Stehule 
Date:   Thu Jan 30 11:28:40 2014 +0100

fix comment

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
new file mode 100644
index 8d45f24..c39767b
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
*** PostgreSQL documentation
*** 650,655 
--- 650,665 
   
  
   
+   --if-exists
+   
+
+ It uses conditional commands (with IF EXISTS
+ clause) for cleaning database schema.
+
+   
+  
+ 
+  
--disable-dollar-quoting

 
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
new file mode 100644
index 5c6a101..ba6583d
*** a/doc/src/sgml/ref/pg_dumpall.sgml
--- b/doc/src/sgml/ref/pg_dumpall.sgml
*** PostgreSQL documentation
*** 301,306 
--- 301,316 
   
  
   
+   --if-exists
+   
+
+ It uses conditional commands (with IF EXISTS
+ clause) for cleaning database schema.
+
+   
+  
+ 
+  
--inserts

 
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
new file mode 100644
index 717da42..55326d5
*** a/doc/src/sgml/ref/pg_restore.sgml
--- b/doc/src/sgml/ref/pg_restore.sgml
***
*** 490,495 
--- 490,505 
   
  
   
+   --if-exists
+   
+
+ It uses conditional commands (with IF EXISTS
+ clause) for cleaning database schema.
+
+   
+  
+ 
+  
--no-data-for-failed-tables

 
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
new file mode 100644
index 6927968..83f7216
*** a/src/bin/pg_dump/pg_backup.h
--- b/src/bin/pg_dump/pg_backup.h
*** typedef struct _restoreOptions
*** 113,118 
--- 113,119 
  	char	   *superuser;		/* Username to use as superuser */
  	char	   *use_role;		/* Issue SET ROLE to this */
  	int			dropSchema;
+ 	int			if_exists;
  	const char *filename;
  	int			dataOnly;
  	int			schemaOnly;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
new file mode 100644
index 7fc0288..a7061d3
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*** RestoreArchive(Archive *AHX)
*** 413,420 
  /* Select owner and schema as necessary */
  _becomeOwner(AH, te);
  _selectOutputSchema(AH, te->namespace);
! /* Drop it */
! ahprintf(AH, "%s", te->dropStmt);
  			}
  		}
  
--- 413,496 
  /* Select owner and schema as necessary */
  _becomeOwner(AH, te);
  _selectOutputSchema(AH, te->namespace);
! 
! if (*te->dropStmt != '\0')
! {
! 	/* Inject IF EXISTS clause to DROP part when it is required. */
! 	if (ropt->if_exists)
! 	{
! 		char buffer[40];
! 		char *mark;
! 		char *dropStmt = te->dropStmt;
! 		PQExpBuffer ftStmt = createPQExpBuffer();
! 
! 		/* ALTER TABLE should be enahnced to ALTER TABLE IF EXISTS */
! 		if (strncmp(dropStmt, "ALTER TABLE", 11) == 0)
! 		{
! 			/*
! 			 * Prepare fault tolerant statement, but
! 			 * ensure unique IF EXISTS option.
! 			 */
! 			if (strncmp(dropStmt + 11, " IF EXISTS", 10) != 0)
! 			{
! appendPQExpBuffer(ftStmt, "ALTER TABLE IF EXISTS");
! dropStmt = dropStmt + 11;
! 			}
! 		}
! 
! 		/*
! 		 * A content of te->desc is usually substring of DROP STATEMENT
! 		 * with one related exception - CONSTRAINTs.
! 		 *
! 		 * Independent to previous sentence - ALTER TABLE ALTER COLUMN
! 		 * DROP DEFAULT doesn't support IF 

Re: [HACKERS] Add min and max execute statement time in pg_stat_statement

[Peter Geoghegan]

On Wed, Jan 29, 2014 at 8:48 PM, KONDO Mitsumasa
 wrote:
> I'd like to know the truth and the fact in your patch, rather than your
> argument:-)

I see.


> [part of sample sqls file, they are executed in pgbench]
> SELECT
> 1-1/9+5*8*6+5+9-6-3-4/9%7-2%7/5-9/7+2+9/7-1%5%9/3-4/4-9/1+5+5/6/5%2+1*2*3-8/8+5-3-8-4/8+5%2*2-2-5%6+4
> SELECT
> 1%9%7-8/5%6-1%2*2-7+9*6-2*6-9+1-2*9+6+7*8-4-2*1%3/7-1%4%9+4+7/5+4/2-3-5%8/3/7*6-1%8*6*1/7/2%5*6/2-3-9
> SELECT
> 1%3*2/8%6%5-3%1+1/6*6*5/9-2*5+6/6*5-1/2-6%4%8/7%2*7%5%9%5/9%1%1-3-9/2*1+1*6%8/2*4/1+6*7*1+5%6+9*1-9*6

> [methods]
> method 1: with old pgss, pg_stat_statements.max=1000(default)
> method 2: with old pgss with my patch, pg_stat_statements.max=1000(default)
> peter 1 : with new pgss(Peter's patch), pg_stat_statements.max=5000(default)
> peter 2 : with new pgss(Peter's patch), pg_stat_statements.max=1000
>
> [for reference]
> method 5:with old pgss, pg_stat_statements.max=5000
> method 6:with old pgss with my patch, pg_stat_statements.max=5000
>
> [results]
>  method   |  try1  |  try2  |  try3  | degrade performance ratio
> -
>  method 1 | 6.546  | 6.558  | 6.638  | 0% (reference score)
>  method 2 | 6.527  | 6.556  | 6.574  | 1%
>  peter 1  | 5.204  | 5.203  | 5.216  |20%
>  peter 2  | 4.241  | 4.236  | 4.262  |35%
>
>  method 5 | 5.931  | 5.848  | 5.872  |11%
>  method 6 | 5.794  | 5.792  | 5.776  |12%

So, if we compare the old pg_stat_statements and old default with the
new pg_stat_statements and the new default (why not? The latter still
uses less shared memory than the former), by the standard of this
benchmark there is a regression of about 20%. But you also note that
there is an 11% "regression" in the old pg_stat_statements against
*itself* when used with a higher pg_stat_statements.max setting of
5,000. This is completely contrary to everyone's prior understanding
that increasing the size of the hash table had a very *positive*
effect on performance by relieving cache pressure and thereby causing
less exclusive locking for an entry_dealloc().

Do you suppose that this very surprising result might just be because
this isn't a very representative benchmark? Nothing ever has the
benefit of *not* having to exclusive lock.

-- 
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] Add min and max execute statement time in pg_stat_statement

[KONDO Mitsumasa]

(2014/01/30 8:29), Tom Lane wrote:
> Andrew Dunstan  writes:
>> I could live with just stddev. Not sure others would be so happy.
> 
> FWIW, I'd vote for just stddev, on the basis that min/max appear to add
> more to the counter update time than stddev does; you've got
> this:
> 
> + e->counters.total_sqtime += total_time * total_time;
> 
> versus this:
>
> + if (e->counters.min_time > total_time || e->counters.min_time 
> == EXEC_TIME_INIT)
> + e->counters.min_time = total_time;
> + if (e->counters.max_time < total_time)
> + e->counters.max_time = total_time;
> 
> I think on most modern machines, a float multiply-and-add is pretty
> darn cheap; a branch that might or might not be taken, OTOH, is a
> performance bottleneck.
> 
> Similarly, the shared memory footprint hit is more: two new doubles
> for min/max versus one for total_sqtime (assuming we're happy with
> the naive stddev calculation).
> 
> If we felt that min/max were of similar value to stddev then this
> would be mere nitpicking.  But since people seem to agree they're
> worth less, I'm thinking the cost/benefit ratio isn't there.
Why do you surplus worried about cost in my patch? Were three double variables
assumed a lot of memory, or having lot of calculating cost? My test result
showed LWlock bottele-neck is urban legend. If you have more fair test
pattern, please tell me, I'll do it if the community will do fair judge.

Regards,
--
Mitsumasa KONDO
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] [PATCH] Filter error log statements by sqlstate

[Oskari Saarenmaa]

30.01.2014 11:37, Jeevan Chalke kirjoitti:

Hi Oskari,

Are you planning to work on what Tom has suggested ? It make sense to me
as well.

What are your views on that ?


Tom's suggestions make sense to me, but unfortunately I haven't had time 
to work on this feature recently so I don't think it'll make it to 9.4 
unless I can complete it during FOSDEM.


I updated https://github.com/saaros/postgres/tree/log-by-sqlstate some 
time ago based on Tom's first set of comments but the tree is still 
missing changes suggested in the last email.


/ Oskari



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