Re: [HACKERS] proposal: ANSI SQL 2011 syntax for named parameters

[Pavel Stehule]

Hello

2013/1/2 Robert Haas :
> On Fri, Dec 28, 2012 at 11:22 AM, Pavel Stehule  
> wrote:
>> I am not sure, but maybe is time to introduce ANSI SQL syntax for
>> functions' named parameters
>>
>> It is defined in ANSI SQL 2011
>>
>>  CALL P (B => 1, A => 2)
>>
>> instead PostgreSQL syntax CALL ( B := 1, A := 2)
>
> Keep in mind that, as recently as PostgreSQL 9.1, we shipped hstore
> with a =>(text, text) operator.  That operator was deprecated in 9.0,
> but it wasn't actually removed until PostgreSQL 9.2.  Whenever we do
> this, it's going to break things for anyone who hasn't yet upgraded
> from hstore v1.0 to hstore v1.1.  So I would prefer to wait one more
> release.  That way, anyone who does an upgrade, say, every other major
> release cycle should have a reasonably clean upgrade path.
>
> I realize that the 4+-year journey toward allowing => rather than :=
> probably seems tedious to many people by now, but I think the cautious
> path we've taken is entirely warranted.  As much as I want us to be
> standards-compliant in this area, I also want us to not break any more
> user applications than necessary along the way.
>
> Incidentally, I think there are two changes here which should be
> considered independently.  One, allowing => rather than := for
> specifying named parameters.  And two, adding a statement called CALL
> that can be used to invoke a function.  Maybe those are both good
> ideas and maybe they aren't, but they're independent.

My recent proposal is related only to named parameters.

Statement CALL can wait to full procedure implementation. Still I hope
so we can implement some more precious transaction control and
returning free recordsets. So I don't propose a CALL statement now.

Regards

Pavel

>
> --
> 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] multiple CREATE FUNCTION AS items for PLs

[Hitoshi Harada]

On Fri, Dec 28, 2012 at 12:24 AM, Pavel Stehule  wrote:
> 2012/12/28 Peter Eisentraut :
>> On Mon, 2012-12-17 at 16:34 -0500, Peter Eisentraut wrote:
>>> Yes, this would be a good solution for some applications, but the only
>>> way I can think of to manage the compatibility issue is to invent some
>>> function attribute system like
>>>
>>> CREATE FUNCTION ... OPTIONS (call_convention 'xyz')
>>
>> An alternative that has some amount of precedent in the Python world
>> would be to use comment pragmas, like this:
>>
>> CREATE FUNCTION foo(a,b,c) AS $$
>> # plpython: module
>> import x
>>  from __future__ import nex_cool_feature
>>
>>  def helper_function(x):
>> ...
>>
>>  def __pg_main__(a,b,c):
>>  defined function body here
>>
>> $$;
>>
>> The source code parser would look for this string on, say, the first two
>> lines, and then decide which way to process the source text.
>>
>> This way we could get this done fairly easily without any new
>> infrastructure outside the language handler.
>
> this concept looks like more stronger and cleaner
>
> +1
>
> I thing so same idea is used in PL/v8
>

What part of PL/v8 do you think is same??  It parses the body as other
PLs do and nothing special.   plv8 also wanted this notion of
"function setting" before introducing find_function(), which natively
loads other JS functions that are defined via CREATE FUNCTION.  Of
course it is not optimal, but it turned out it is not terribly slow.
Maybe it depends on language runtime.  So for now, PL/v8 is managing
to do it and happy with the existing mechanism.  It could be improved
somehow, but I don't want it to be complicated than now in order to
improve small stuff.

Thanks,
-- 
Hitoshi Harada


-- 
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] Problematic dependency in plpython Makefile [Windows]

[Andrew Dunstan]

On 01/02/2013 11:34 PM, Andrew Dunstan wrote:


On 01/02/2013 10:13 PM, Noah Misch wrote:
On Windows, src/pl/plpython/Makefile has a rule whose target line 
expands to
something like "python33.def: C:/Windows/system32/python33.dll".  
When doing a

MinGW build with Cygwin's make-3.81, that line elicits an error:

Makefile:69: *** target pattern contains no `%'.  Stop.

Seeing a second colon, make treats the line as a static pattern 
rule.  Perhaps

the MinGW project ships a make patched to avoid this, or perhaps folks
building PostgreSQL override WINDIR.  In any event, that dependency 
is not

useful: we can't build the named file if it's absent, and an error from
pexports is a good as an error from make.  Let's drop the dependency.

Note that this affects --without-python builds during "make clean".





I suspect under Msys the path seen won't contain a colon - it will be 
an Msys virtualized path like /c/Windows/system32/


Apparently not, from testing. Maybe the make is patched after all.

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] Problematic dependency in plpython Makefile [Windows]

[Andrew Dunstan]

On 01/02/2013 10:13 PM, Noah Misch wrote:

On Windows, src/pl/plpython/Makefile has a rule whose target line expands to
something like "python33.def: C:/Windows/system32/python33.dll".  When doing a
MinGW build with Cygwin's make-3.81, that line elicits an error:

Makefile:69: *** target pattern contains no `%'.  Stop.

Seeing a second colon, make treats the line as a static pattern rule.  Perhaps
the MinGW project ships a make patched to avoid this, or perhaps folks
building PostgreSQL override WINDIR.  In any event, that dependency is not
useful: we can't build the named file if it's absent, and an error from
pexports is a good as an error from make.  Let's drop the dependency.

Note that this affects --without-python builds during "make clean".





I suspect under Msys the path seen won't contain a colon - it will be an 
Msys virtualized path like /c/Windows/system32/


Frankly, this is an unsupported toolchain. The supported toolchains 
under Windows are Msys/Mingw and the Microsoft toolsets.


OTOH, I don't object to dropping a dependency that is truly useless.

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] Proposal: Store "timestamptz" of database creation on "pg_database"

[Fabrízio de Royes Mello]

* Robert Haas  wrote:
> I know this has been discussed and rejected before, but I find that
> rejection to be wrong-headed.  I have repeatedly been asked, with
> levels of exasperation ranging from mild to homicidal, why we don't
> have this feature, and I have no good answer.  If it were somehow
> difficult to record this or likely to produce a lot of overhead, that
> would be one thing.  But it isn't.  It's probably a hundred-line
> patch, and AFAICS the overhead would be miniscule.

Hi all,

The attached patch add a new column into 'pg_database' called 'datcreated'
to store the timestamp of database creation.

If this feature is approved I could extend it to add a column into
'pg_class' to store creation timestamp too.

I think we can discuss about psql support to show this new info about
databases...

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello


pg_database_add_datcreated_column_v1.patch
Description: Binary data

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

Re: [HACKERS] Problematic dependency in plpython Makefile [Windows]

[Noah Misch]

On Thu, Jan 03, 2013 at 11:52:58AM +0800, Craig Ringer wrote:
> On 01/03/2013 11:13 AM, Noah Misch wrote:
> > On Windows, src/pl/plpython/Makefile has a rule whose target line expands to
> > something like "python33.def: C:/Windows/system32/python33.dll".  When 
> > doing a
> > MinGW build with Cygwin's make-3.81, that line elicits an error:
> Shouldn't you generally be using msys make from mingw?
> 
> I'm a bit puzzled as to why you'd be using cygwin make.

I don't normally use MSYS at all.  I wouldn't furnish widespread Makefile
changes to avoid doing so, but this is the only change I have needed.


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

Re: [HACKERS] Proposal: Store "timestamptz" of database creation on "pg_database"

[Robert Haas]

On Wed, Jan 2, 2013 at 9:12 PM, Stephen Frost  wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> If I believed that it would be a hundred-line patch, and would *stay*
>> a hundred-line patch, I'd be fine with it.  But it won't.  I will
>> bet a very fine dinner that the feature wouldn't get out the door
>> before there would be demands for pg_dump support.
>
> Fine, how about a function that can be called by pg_dump (and anyone
> else who has the rights and feels the need) to set that value?  That
> avoids all need for any new syntax and still gives us the pg_dump and
> friends support that will apparently be asked for.

TBH, I don't think anyone has any business changing the creation
timestamp.  Ever.  For me, the fact that pg_dump wouldn't preserve
this information would be a feature, not a bug.  I mostly meant to
point out that someone could bypass it if they cared enough, not to
recommend it.  Honestly, I'd probably *rather* store this information
someplace where it couldn't be changed via SQL *at all*.  But I don't
think we have such a place, so I'm happy enough to store it in the
catalogs, with the associated risks of catalog hackery that entails.

>> And arguments
>> about whether ALTER should or should not change the timestamp.
>
> There is no case where ALTER should change the *creation* time, imo.

Duh.

>> And I doubt you counted psql \d support in that hundred lines.
>> So this is just a can of worms that I'd rather not open.
>
> The last psql \d support change that I looked at (thanks Jon) had a
> diffstat (excluding documentation and whitespace changes) of:
>
> sfrost@beorn:/home/sfrost/Downloads> cat qq | diffstat
>  describe.c |5 +
>  1 file changed, 5 insertions(+)
>
> Just saying. ;)

Yeah, I don't think this is really a problem.  I would expect the psql
support for this feature to be not a whole lot more complicated than
that.  Sure, it might be more than 5 lines of raw code if it requires
an additional version of some query for which we're currently using
the same version for both PG93 and PG92, but it's hardly fair to cite
that as an argument for not doing this.  Such changes are almost
entirely boilerplate.

-- 
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] Problematic dependency in plpython Makefile [Windows]

[Craig Ringer]

On 01/03/2013 11:13 AM, Noah Misch wrote:
> On Windows, src/pl/plpython/Makefile has a rule whose target line expands to
> something like "python33.def: C:/Windows/system32/python33.dll".  When doing a
> MinGW build with Cygwin's make-3.81, that line elicits an error:
Shouldn't you generally be using msys make from mingw?

I'm a bit puzzled as to why you'd be using cygwin make.

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



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

[HACKERS] Problematic dependency in plpython Makefile [Windows]

[Noah Misch]

On Windows, src/pl/plpython/Makefile has a rule whose target line expands to
something like "python33.def: C:/Windows/system32/python33.dll".  When doing a
MinGW build with Cygwin's make-3.81, that line elicits an error:

Makefile:69: *** target pattern contains no `%'.  Stop.

Seeing a second colon, make treats the line as a static pattern rule.  Perhaps
the MinGW project ships a make patched to avoid this, or perhaps folks
building PostgreSQL override WINDIR.  In any event, that dependency is not
useful: we can't build the named file if it's absent, and an error from
pexports is a good as an error from make.  Let's drop the dependency.

Note that this affects --without-python builds during "make clean".

Thanks,
nm
*** a/src/pl/plpython/Makefile
--- b/src/pl/plpython/Makefile
***
*** 66,72  OBJS += libpython${pytverstr}.a
  libpython${pytverstr}.a: python${pytverstr}.def
dlltool --dllname python${pytverstr}.dll --def python${pytverstr}.def 
--output-lib  libpython${pytverstr}.a
  WD=$(subst \,/,$(WINDIR))
! python${pytverstr}.def: $(WD)/system32/python${pytverstr}.dll
pexports $(WD)/system32/python${pytverstr}.dll > python${pytverstr}.def
  endif
  
--- 66,72 
  libpython${pytverstr}.a: python${pytverstr}.def
dlltool --dllname python${pytverstr}.dll --def python${pytverstr}.def 
--output-lib  libpython${pytverstr}.a
  WD=$(subst \,/,$(WINDIR))
! python${pytverstr}.def:
pexports $(WD)/system32/python${pytverstr}.dll > python${pytverstr}.def
  endif
  

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

Re: [HACKERS] Proposal: Store "timestamptz" of database creation on "pg_database"

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> If I believed that it would be a hundred-line patch, and would *stay*
> a hundred-line patch, I'd be fine with it.  But it won't.  I will
> bet a very fine dinner that the feature wouldn't get out the door
> before there would be demands for pg_dump support. 

Fine, how about a function that can be called by pg_dump (and anyone
else who has the rights and feels the need) to set that value?  That
avoids all need for any new syntax and still gives us the pg_dump and
friends support that will apparently be asked for.

> And arguments
> about whether ALTER should or should not change the timestamp.

There is no case where ALTER should change the *creation* time, imo.

> And I doubt you counted psql \d support in that hundred lines.
> So this is just a can of worms that I'd rather not open.

The last psql \d support change that I looked at (thanks Jon) had a
diffstat (excluding documentation and whitespace changes) of:

sfrost@beorn:/home/sfrost/Downloads> cat qq | diffstat   
 describe.c |5 +
 1 file changed, 5 insertions(+)

Just saying. ;)

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Proposal: Store "timestamptz" of database creation on "pg_database"

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> Well, IMHO, there is no need for any syntax change at all.  CREATE
> TABLE and CREATE DATABASE should just record the creation time
> somewhere, and that's all.  If you dump-and-reload, the creation time
> changes.  Deal with it, or hack your catalogs if you really care that
> much.

I'd be alright with this also, tbh.  Not preserving such information
across pg_dump's wouldn't really be all *that* much of a loss.

As for hacking at the catalogs, I do find that a rather terrible
recommendation, ever.  I'm currently trying to convince people at $work
that hacking at pg_database to modify datallowconns is really not a
good or ideal solution (and requires a lot more people to have
superuser rights than really should, which is practically no one, imo).
Annoyingly, we don't seem to have a way to ALTER DATABASE to set that
value, although I *think* 'connection limit = 0' might be good enough.

> I find the suggestion of using event triggers for this to miss the
> point almost completely.  At least in my case, the time when you
> really wish you had some timestamps is when you get dropped into a
> customer environment and need to do forensics.  The customer will not
> have installed the convenient package of event triggers at database
> bootstrap time.  Their environment will likely be poorly configured
> and completely undocumented; that's why you're doing forensics, isn't
> it?

Exactly, that's what I was trying to get at upstream.

> I know this has been discussed and rejected before, but I find that
> rejection to be wrong-headed.  I have repeatedly been asked, with
> levels of exasperation ranging from mild to homicidal, why we don't
> have this feature, and I have no good answer.  If it were somehow
> difficult to record this or likely to produce a lot of overhead, that
> would be one thing.  But it isn't.  It's probably a hundred-line
> patch, and AFAICS the overhead would be miniscule.

+1

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Proposal: Store "timestamptz" of database creation on "pg_database"

[Tom Lane]

Robert Haas  writes:
> [ on creation timestamps ]
> I know this has been discussed and rejected before, but I find that
> rejection to be wrong-headed.  I have repeatedly been asked, with
> levels of exasperation ranging from mild to homicidal, why we don't
> have this feature, and I have no good answer.  If it were somehow
> difficult to record this or likely to produce a lot of overhead, that
> would be one thing.  But it isn't.  It's probably a hundred-line
> patch, and AFAICS the overhead would be miniscule.

If I believed that it would be a hundred-line patch, and would *stay*
a hundred-line patch, I'd be fine with it.  But it won't.  I will
bet a very fine dinner that the feature wouldn't get out the door
before there would be demands for pg_dump support.  And arguments
about whether ALTER should or should not change the timestamp.
And I doubt you counted psql \d support in that hundred lines.
So this is just a can of worms that I'd rather not open.

regards, tom lane


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

Re: [HACKERS] Re: Proposal: Store "timestamptz" of database creation on "pg_database"

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sat, Dec 29, 2012 at 10:26 AM, Andres Freund  
> wrote:
> > A shared table for event triggers sounds like it would be the far easier
> > solution (9.4+ that is).
> 
> The problem is that the event trigger table is a just a pointer to a
> function, and there's no procedure OID to store in that shared catalog
> unless you also have a proposal for making pg_proc into a shared
> catalog ... which would also require making pg_language into a shared
> catalog, and maybe a few others.

This was why I was suggesting that there be a single database in which
the events would actually fire and that's where the function itself
would also be stored.  The information to pass to the function would
have to be collected and represented logically from the calling
database, of course, and it wouldn't be possible to make changes in the
database where the modification happened without using something like
dblink, but I could still see there being a lot of good use cases for
such a thing.

All pie-in-the-sky currently though, of course, but that's along the
lines of what I was thinking.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Proposal: Store "timestamptz" of database creation on "pg_database"

[Robert Haas]

On Wed, Dec 26, 2012 at 11:13 PM, Tom Lane  wrote:
> This has been debated, and rejected, before.
>
> To mention just one problem, are we going to add nonstandard,
> non-backwards-compatible syntax to every single kind of CREATE to allow
> pg_dump to preserve the creation dates?  Another interesting question is
> whether we should likewise track the last ALTER time, or perhaps whether
> a sufficiently major ALTER redefinition should update the creation time.

Well, IMHO, there is no need for any syntax change at all.  CREATE
TABLE and CREATE DATABASE should just record the creation time
somewhere, and that's all.  If you dump-and-reload, the creation time
changes.  Deal with it, or hack your catalogs if you really care that
much.

I find the suggestion of using event triggers for this to miss the
point almost completely.  At least in my case, the time when you
really wish you had some timestamps is when you get dropped into a
customer environment and need to do forensics.  The customer will not
have installed the convenient package of event triggers at database
bootstrap time.  Their environment will likely be poorly configured
and completely undocumented; that's why you're doing forensics, isn't
it?

I know this has been discussed and rejected before, but I find that
rejection to be wrong-headed.  I have repeatedly been asked, with
levels of exasperation ranging from mild to homicidal, why we don't
have this feature, and I have no good answer.  If it were somehow
difficult to record this or likely to produce a lot of overhead, that
would be one thing.  But it isn't.  It's probably a hundred-line
patch, and AFAICS the overhead would be miniscule.

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


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

Re: [HACKERS] Re: Proposal: Store "timestamptz" of database creation on "pg_database"

[Robert Haas]

On Sat, Dec 29, 2012 at 10:26 AM, Andres Freund  wrote:
> A shared table for event triggers sounds like it would be the far easier
> solution (9.4+ that is).

The problem is that the event trigger table is a just a pointer to a
function, and there's no procedure OID to store in that shared catalog
unless you also have a proposal for making pg_proc into a shared
catalog ... which would also require making pg_language into a shared
catalog, and maybe a few others.

-- 
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] dynamic SQL - possible performance regression in 9.2

[Tom Lane]

Jeff Janes  writes:
> Using a RULE-based partitioning instead with row by row insertion, the
> plancache changes  slowed it down by 300%, and this patch doesn't change
> that.  But that seems to be down to the insertion getting planned
> repeatedly, because it decides the custom plan is cheaper than the generic
> plan.  Whatever savings the custom plan may have are clearly less than the
> cost of doing the planning repeatedly.

That scenario doesn't sound like it has anything to do with the one being
discussed in this thread.  But what do you mean by "rule-based
partitioning" exactly?  A rule per se wouldn't result in a cached plan
at all, let alone one with parameters, which would be necessary to
trigger any use of the custom-cached-plan code path.

Test cases are way more interesting than hand-wavy complaints.

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] dynamic SQL - possible performance regression in 9.2

[Jeff Janes]

On Friday, December 28, 2012, Heikki Linnakangas wrote:

> On 28.12.2012 23:53, Peter Eisentraut wrote:
>
>> On 12/27/12 1:07 AM, Pavel Stehule wrote:
>>
>>> Hello
>>>
>>> I rechecked performance of dynamic SQL and it is significantly slower
>>> in 9.2 than 9.1
>>>
>>> -- 9.1
>>> postgres=# create or replace function test() returns void as $$ begin
>>> for i in 1..100 loop execute 'select 1'; end loop; end $$ language
>>> plpgsql;
>>>
>>
>> I think this is the same as the case discussed at
>> .
>>
>
> Yeah, probably so.
>
> As it happens, I just spent a lot of time today narrowing down yet another
> report of a regression in 9.2, when running DBT-2:
> http://archives.postgresql.**org/pgsql-performance/2012-11/**msg7.php.
> It looks like that is also caused by the plancache changes. DBT-2
> implements the transactions using C functions, which use SPI_execute() to
> run all the queries.
>
> It looks like the regression is caused by extra copying of the parse tree
> and plan trees. Node-copy-related functions like AllocSetAlloc and _copy*
> are high in the profile, They are also high in the 9.1 profile, but even
> more so in 9.2.
>
> I hacked together a quick&dirty patch to reduce the copying of single-shot
> plans, and was able to buy back much of the regression I was seeing on
> DBT-2. Patch attached.


The plancache change slowed down a dynamic sql partitioning trigger about
26%, and your patch redeems about 1/2 of that cost.

Using a RULE-based partitioning instead with row by row insertion, the
plancache changes  slowed it down by 300%, and this patch doesn't change
that.  But that seems to be down to the insertion getting planned
repeatedly, because it decides the custom plan is cheaper than the generic
plan.  Whatever savings the custom plan may have are clearly less than the
cost of doing the planning repeatedly.

Cheers,

Jeff

PATCH: Split stats file per database WAS: [HACKERS] autovacuum stress-testing our system

[Tomas Vondra]

Hi,

attached is a version of the patch that I believe is ready for the
commitfest. As the patch was discussed over a large number of messages,
I've prepared a brief summary for those who did not follow the thread.


Issue
=

The patch aims to improve the situation in deployments with many tables
in many databases (think for example 1000 tables in 1000 databases).
Currently all the stats for all the objects (dbs, tables and functions)
are written in a single file (pgstat.stat), which may get quite large
and that consequently leads to various issues:

1) I/O load - the file is frequently written / read, which may use a
significant part of the I/O bandwidth. For example we'have to deal with
cases when the pgstat.stat size is >150MB, and it's written (and read)
continuously (once it's written, a new write starts) and utilizes 100%
bandwidth on that device.

2) CPU load - a common solution to the previous issue is moving the file
into RAM, using a tmpfs filesystem. That "fixes" the I/O bottleneck but
causes high CPU load because the system is serializing and deserializing
large amounts of data. We often see ~1 CPU core "lost" due to this (and
causing higher power consumption, but that's Amazon's problem ;-)

3) disk space utilization - the pgstat.stat file is updated in two
steps, i.e. a new version is written to another file (pgstat.tmp) and
then it's renamed to pgstat.stat, which means the device (amount of RAM,
if using tmpfs device) needs to be >2x the actual size of the file.
(Actually more, because there may be descriptors open to multiple
versions of the file.)

This patch does not attempt to fix a "single DB with multiple schemas"
scenario, although it should not have a negative impact on it.


What the patch does
===

1) split into global and per-db files
-

The patch "splits" the huge pgstat.stat file into smaller pieces - one
"global" one (global.stat) with database stats, and one file for each of
the databases (oid.stat) with table.

This makes it possible to write/read much smaller amounts of data, because

a) autovacuum launcher does not need to read the whole file - it needs
just the list of databases (and not the table/func stats)

b) autovacuum workers do request a fresh copy of a single database, so
the stats collector may write just the global.stat + one of the per-db files

and that consequently leads to much lower I/O and CPU load. During our
tests we've seen the I/O to drop from ~150MB/s to less than 4MB/s, and
much lower CPU utilization.


2) a new global/stat directory
--

The pgstat.stat file was originally saved into the "global" directory,
but with so many files that would get rather messy so I've created a new
global/stat directory and all the files are stored there.

This also means we can do a simple "delete files in the dir" when
pgstat_reset_all is called.


3) pgstat_(read|write)_statsfile split
--

These two functions were moved into a global and per-db functions, so
now there's

  pgstat_write_statsfile-- global.stat
  pgstat_write_db_statsfile -- oid.stat

  pgstat_read_statsfile-- global.stat
  pgstat_read_db_statsfile -- oid.stat

There's a logic to read/write only those files that are actually needed.


4) list of (OID, timestamp) inquiries, last db-write


Originally there was a single pair of request/write timestamps for the
whole file, updated whenever a worker requested a fresh file or when the
file was written.

With the split, this had to be replaced by two lists - a timestamp of
the last inquiry (per DB), and a timestamp when each database file was
written for the last time.

The timestamp of the last DB write was added to the PgStat_StatDBEntry
and the list of inquiries is kept in last_statrequests. The fields are
used at several places, so it's probably best to see the code.

Handling the timestamps is a rather complex stuff because of the clock
skews. One of those checks is not needed as the list of inquiries is
freed right after writing all the databases. But I wouldn't be surprised
if there was something I missed, as splitting the file into multiple
pieces made this part more complex.

So please, if you're going to review this patch this is one of the
tricky places.


5) dummy file
-

A special handling is necessary when an inquiry arrives for a database
without a PgStat_StatDBEntry - this happens for example right after
initdb, when there are no stats for template0 and template1, yet the
autovacuum workers do send inqiries for them.

The backend_read_statsfile now uses the timestamp stored in the header
of the per-db file (not in the global one), and the easies way to handle
this for new databases is writing an empty 'dummy file' (just a header
with timestamp). Without this, this would result in 'pgstat wait
timeout' errors.

This is what pgstat_write_db_dummyfile (used 

Re: [HACKERS] json api WIP patch

[Andrew Dunstan]

On 01/02/2013 04:45 PM, Robert Haas wrote:

On Wed, Dec 26, 2012 at 3:33 PM, Andrew Dunstan  wrote:

Here is a patch for the first part of the JSON API that was recently
discussed. It includes the json  parser hook infrastructure and functions
for json_get and friends, plus json_keys.

As is, this exposes the json lexer fully for use by the hook functions. But
I could easily be persuaded that this should be an opaque structure with
some constructor and getter functions - I don't think the hook functions
need or should be able to set anything in the lexer.

Work is proceeding on some of the more advanced functionality discussed.

This seems to contain a large number of spurious whitespace changes.



I'm glad you're looking at it :-)

I did do a run of pgindent on the changed files before I cut the patch, 
which might have made some of those changes.


I notice a couple of other infelicities too, which are undoubtedly my fault.

The original prototype of this was cut against some older code, and I 
then tried to merge it with the current code base, and make sure that 
all the regression tests passed. That might well have resulted in a 
number of things that need review.




And maybe some other spurious changes.  For example, I'm not sure why
this comment is truncated:

   



Another good question.

I'll make another pass over this and try to remove some of what's 
annoying you.


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] proposal: ANSI SQL 2011 syntax for named parameters

[Robert Haas]

On Fri, Dec 28, 2012 at 11:22 AM, Pavel Stehule  wrote:
> I am not sure, but maybe is time to introduce ANSI SQL syntax for
> functions' named parameters
>
> It is defined in ANSI SQL 2011
>
>  CALL P (B => 1, A => 2)
>
> instead PostgreSQL syntax CALL ( B := 1, A := 2)

Keep in mind that, as recently as PostgreSQL 9.1, we shipped hstore
with a =>(text, text) operator.  That operator was deprecated in 9.0,
but it wasn't actually removed until PostgreSQL 9.2.  Whenever we do
this, it's going to break things for anyone who hasn't yet upgraded
from hstore v1.0 to hstore v1.1.  So I would prefer to wait one more
release.  That way, anyone who does an upgrade, say, every other major
release cycle should have a reasonably clean upgrade path.

I realize that the 4+-year journey toward allowing => rather than :=
probably seems tedious to many people by now, but I think the cautious
path we've taken is entirely warranted.  As much as I want us to be
standards-compliant in this area, I also want us to not break any more
user applications than necessary along the way.

Incidentally, I think there are two changes here which should be
considered independently.  One, allowing => rather than := for
specifying named parameters.  And two, adding a statement called CALL
that can be used to invoke a function.  Maybe those are both good
ideas and maybe they aren't, but they're independent.

-- 
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] multiple CREATE FUNCTION AS items for PLs

[Robert Haas]

On Fri, Dec 28, 2012 at 7:09 AM, Hannu Krosing  wrote:
> thus
>
> CREATE FUNCTION foo(a int,b int, c text)
>
> would create module
>
> plpy.modules.foo_int4_int4_text

I don't know much of anything about Python, but keep in mind that
types can be renamed.  It seems like that could break things if the
type name is included in the module name.

-- 
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] Whats the correct way to change trigdata->tg_relation

[Robert Haas]

On Fri, Dec 28, 2012 at 1:06 PM, Charles Gomes  wrote:
> I'm creating a simple trigger that will be called during an insert and change 
> the destination table.
> All values are going to be preserved, just the destination table will be 
> different.
>
> From what I see I can't modify trigdata->tg_relation.
>
> All examples use: return Datum(trigdata->tg_trigtuple); // however 
> tg_relation does not belong there. I'm trying to avoind having to do a 
> SPI_EXEC;
> Should I create a new heap_tuple and call heap_insert() and then Return 
> Datum(NULL); ? Or is there another more straight forward way of doing it?  
> Looks like if I call heap_insert I will have to update the indexes somehow.

I think you need to use SPI_EXEC.  Otherwise something horrible will
probably happen.

-- 
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] json api WIP patch

[Robert Haas]

On Wed, Dec 26, 2012 at 3:33 PM, Andrew Dunstan  wrote:
> Here is a patch for the first part of the JSON API that was recently
> discussed. It includes the json  parser hook infrastructure and functions
> for json_get and friends, plus json_keys.
>
> As is, this exposes the json lexer fully for use by the hook functions. But
> I could easily be persuaded that this should be an opaque structure with
> some constructor and getter functions - I don't think the hook functions
> need or should be able to set anything in the lexer.
>
> Work is proceeding on some of the more advanced functionality discussed.

This seems to contain a large number of spurious whitespace changes.

And maybe some other spurious changes.  For example, I'm not sure why
this comment is truncated:

}
}

!   /*
!* Check for trailing garbage.  As in json_lex(), any alphanumeric stuff
!* here should be considered part of the token for error-reporting
!* purposes.
!*/
for (p = s; JSON_ALPHANUMERIC_CHAR(*p); p++)
error = true;
lex->token_terminator = p;
if (error)
report_invalid_token(lex);
--- 730,739 
}
}

!   /* Check for trailing garbage. */
for (p = s; JSON_ALPHANUMERIC_CHAR(*p); p++)
error = true;
+   lex->prev_token_terminator = lex->token_terminator;
lex->token_terminator = p;
if (error)
report_invalid_token(lex);


-- 
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] Feature Request: pg_replication_master()

[Robert Haas]

On Wed, Dec 26, 2012 at 3:32 PM, Josh Berkus  wrote:
>> There already are two ways to promote a server out of recovery. One is
>> creating the trigger file. The other is "pg_ctl promote". (it uses a
>> trigger file called $PGDATA/promote internally, but that's invisible to
>> the user).
>
> Right, I was thinking of the trigger file to put a server *into*
> replication.  That is, recovery.conf.

Well, we can't put the server back into recovery once it's up and
running.  You have to restart for that, and I can't see that changing
any time soon.  I think we could support something like:

pg_ctl start -m recover

Of course, that would require that primary_conninfo et. al. be
available from somewhere.  If we got rid of all the recovery.conf
parameters and made them GUCs, then that'd be pretty easy: the setting
would always be available from postgresql.conf, but would be ignored
except when starting in recovery mode.

In my view, the biggest problem with recovery.conf is that the
parameters in there are not GUCs, which means that all of the
infrastructure that we've built for managing GUCs does not work with
them.  As an example, when we converted recovery.conf to use the same
lexer that the GUC machinery uses, it allowed recovery.conf values to
be specified unquoted in the same circumstances where that was already
possible for postgresql.conf.  But, you still can't use SHOW or
pg_settings with recovery.conf parameters, and I think pg_ctl reload
doesn't work either.  If we make these parameters into GUCs, then
they'll work the same way everything else works.  Even if (as seems
likely) we end up still needing a trigger file (or a special pg_ctl
mode) to initiate recovery, I think that's probably a win.

Other people's mileage may vary, of course.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Unify some tar functionality across different parts

[Magnus Hagander]

On Wed, Jan 2, 2013 at 5:44 PM, Tom Lane  wrote:
> Magnus Hagander  writes:
>> On Wed, Jan 2, 2013 at 5:36 PM, Tom Lane  wrote:
>>> Why are these very tar-specific functions being declared in such a
>>> globally visible spot as port.h?  That seems like a bad idea on its
>>> face.  IMO stuff in port.h ought to be about as globally applicable
>>> as, say, malloc().
>
>> It's where we put most of the things from src/port in.
>
> Might be time to revisit that, or perhaps better reconsider what we're
> putting in src/port/.  The idea that anything that we want in both
> frontend and backend is a porting issue seems a bit busted in itself.

Yeah, true - but there's certainly other parts that think the same, so
that's a separate issue.


>> I take it you suggest moving it to a special say include/pgtar.h file?
>
> Works for me.

Applied. Should hopefully work once Alvaro gets the buildfarm back to green.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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_basebackup from cascading standby after timeline switch

[Greg Stark]

On Wed, Jan 2, 2013 at 1:55 PM, Heikki Linnakangas
 wrote:
> If you take a backup with "pg_basebackup -X fetch", and the timeline
> switches while the backup is taken, you currently get an error like
> "requested WAL segment 0001000C has already been removed".
> To fix, let's change the server-side support of "-X fetch" to include all
> WAL files between the backup start and end pointers, regardless of
> timelines. I'm thinking of doing this by scanning pg_xlog with readdir(),
> and sending over any files in that range. Another option would be to read
> and parse the timeline history file to figure out the exact filenames
> expected, but the readdir() approach seems simpler.

I'm not clear what you mean by "any files in that range". There could
be other timelines in the archive that aren't relevant to the restore
at all. For example if the database you're requesting a backup from
has previously been restored from an old backup the archive could have
archives from the original timeline as well as the active timeline.

I'm trying to wrap my head around what other combinations are
possible. Is it possible there have been other false starts or
multiple timeline switches during the time the backup was being taken?
At first blush I think not, I think it's only possible for there to be
one timeline switch and it would be when a standby database was being
backed up and is activated while the backup was being taken.


-- 
greg


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

Re: [HACKERS] Review of Row Level Security

[David Fetter]

On Wed, Jan 02, 2013 at 05:31:42PM +, Simon Riggs wrote:
> On 2 January 2013 17:19, David Fetter  wrote:
> 
> > Would COPY be covered separately?  How about TRUNCATE?
> 
> COPY == INSERT

Makes sense.  The reason I mentioned it is that COPY is supposed to be
a very fast bulk loading process, which implies a couple of things:

1.  In the RLS (really should be RLAC, but let's not go there now)
case, COPY makes it pretty simple to probe hugely many things at once
for existence unless there's some kind of COPY pre-processor that
throws away non-matching rows.  Fortunately there's work being done to
that end.

2.  COPY, being intended to be very, very fast, should probably get
some kind of notation, at least in the docs, about how it will slow
down in the RLS case.

> TRUNCATE isn't covered at all since it doesn't look at rows. It has a
> separate privilege that can be granted to those that need it.

OK

> > Also, is there any way to apply this to the catalog, or would that
> > be too large a restructuring, given how catalog access actually
> > works?
> 
> Doubt it.

Somewhat related issue:  Is there a worked example of setting up
PostgreSQL to a "default deny" access policy as far as is possible
today?  This touches a lot of things, among them reading the catalog.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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 of Row Level Security

[Simon Riggs]

On 2 January 2013 17:19, David Fetter  wrote:

> Would COPY be covered separately?  How about TRUNCATE?

COPY == INSERT

TRUNCATE isn't covered at all since it doesn't look at rows. It has a
separate privilege that can be granted to those that need it.

> Also, is there any way to apply this to the catalog, or would that be
> too large a restructuring, given how catalog access actually works?

Doubt it.

-- 
 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] Minor fix in 'clean' action of 'src/backend/Makefile'

[Fabrízio de Royes Mello]

On Wed, Jan 2, 2013 at 2:23 PM, Heikki Linnakangas
wrote:

> On 02.01.2013 18:20, Heikki Linnakangas wrote:
>
>> Hmm, looking closer though, repl_gram.h is not actually needed for
>> anything, though. We could just remove the -d flag from the bison
>> invocation and not build it to begin with. I'll go and do that..
>>
>
> And looking even closer, we don't use the -d flag in git master anymore,
> so repl_gram.h is not being built, and there's nothing to do here. I
> suppose we could change back-branches to also not build it, but I don't
> think I'm going to bother.
>
>
You all right... thanks

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

Re: [HACKERS] Review of Row Level Security

[David Fetter]

On Wed, Jan 02, 2013 at 05:35:13PM +0100, Kohei KaiGai wrote:
> 2012/12/31 Simon Riggs :
> > On 23 December 2012 18:49, Simon Riggs  wrote:
> >
> >> Anyway, hope you can make call on 28th so we can discuss this and
> >> agree a way forwards you're happy with.
> >
> > Stephen, KaiGai and myself met by phone on 28th to discuss.
> >
> > 1. The actual default is not that important to any of us. We could go
> > either way, or have no default at all.
> >
> > 2. What we do want is a declarative way of specifying row security,
> > with options to support all use cases discussed/requested on list. We
> > shouldn't
> > support just one of those use cases and force everybody else to use
> > triggers manually for the other cases.
> >
> > 3. We want to have the possibility of multiple row security
> > expressions, defined for different privilege types (SELECT, UPDATE,
> > INSERT, DELETE). (Note that this means you'd be able to specify that
> > an update could read a row in one security mode by setting SELECT,
> > then update that row to a new security mode by setting a clause on
> > UPDATE - hence we refer to those as privileges not commands/events).
> > The expressions should be separate so they can be  pushed easily into
> > query plans (exactly as in the current patch).
> >
> > Stephen has updated the Wiki with some ideas on how that can be structured
> > https://wiki.postgresql.org/wiki/RLS
> >
> > 4. Supporting multiple expressions may not be possible for 9.3, but if
> > not, we want to agree now what the syntax is to make sure we have a
> > clear route for future development. If we can agree this quickly we
> > increase the chances of KaiGai successfully implementing that.
> >
> The syntax being discussed were below:
> 
>   ALTER TABLE  SET ROW SECURITY FOR  TO ();
>   ALTER TABLE  RESET ROW SECURITY FOR ;
> 
>can be one of: ALL, SELECT, INSERT, UPDATE, DELETE
> 
> The point in development towards v9.3 is, we only support "ALL" but
> we can add other command types in the future.
> IMO, only "parser" should accept command types except for ALL but
> raise an error something like "it is not supported yet" to protect from
> syntax conflicts.

Great!

Would COPY be covered separately?  How about TRUNCATE?

Also, is there any way to apply this to the catalog, or would that be
too large a restructuring, given how catalog access actually works?

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] Big disconnect_and_exit cleanup in pg_basebackup

[Boszormenyi Zoltan]

2013-01-02 17:17 keltezéssel, Boszormenyi Zoltan írta:

Hi,

the previously sent "factor out pg_malloc and friends" patch
may call exit() in case of OOM during allocation and as a consequence,
PQfinish() won't get called, leaving an "unexpected EOF from client"
in the log.

Let's close this annoyance in pg_basebackup. The attached patch does
the following:

- adds a PQfinish() (actually PQfinishSafe()) call that was just posted
  in another thread
- replace all PQfinish() and PQclear() with their *Safe counterpart so
  a normal execution won't result in a crash because of calling PQfinish()
  on a stale pointer


Forget about this point, the attached version sets "conn = NULL;"
explicitly after PQfinish(conn).


- kill the disconnect_and_exit() macro, replace it with plain exit(),
  the atexit callback does the disconnect anyway.

Best regards,
Zoltán Böszörményi






--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

diff -durpN postgresql.3/src/bin/pg_basebackup/pg_basebackup.c postgresql.5/src/bin/pg_basebackup/pg_basebackup.c
--- postgresql.3/src/bin/pg_basebackup/pg_basebackup.c	2013-01-02 13:33:46.127749834 +0100
+++ postgresql.5/src/bin/pg_basebackup/pg_basebackup.c	2013-01-02 17:38:54.617502408 +0100
@@ -277,7 +277,7 @@ StartLogStreamer(char *startpos, uint32
 		fprintf(stderr,
 _("%s: could not parse transaction log location \"%s\"\n"),
 progname, startpos);
-		disconnect_and_exit(1);
+		exit(1);
 	}
 	param->startptr = ((uint64) hi) << 32 | lo;
 	/* Round off to even segment position */
@@ -290,7 +290,7 @@ StartLogStreamer(char *startpos, uint32
 		fprintf(stderr,
 _("%s: could not create pipe for background process: %s\n"),
 progname, strerror(errno));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 #endif
 
@@ -323,7 +323,7 @@ StartLogStreamer(char *startpos, uint32
 	{
 		fprintf(stderr, _("%s: could not create background process: %s\n"),
 progname, strerror(errno));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 
 	/*
@@ -335,7 +335,7 @@ StartLogStreamer(char *startpos, uint32
 	{
 		fprintf(stderr, _("%s: could not create background thread: %s\n"),
 progname, strerror(errno));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 #endif
 }
@@ -360,7 +360,7 @@ verify_dir_is_empty_or_create(char *dirn
 fprintf(stderr,
 		_("%s: could not create directory \"%s\": %s\n"),
 		progname, dirname, strerror(errno));
-disconnect_and_exit(1);
+exit(1);
 			}
 			return;
 		case 1:
@@ -377,7 +377,7 @@ verify_dir_is_empty_or_create(char *dirn
 			fprintf(stderr,
 	_("%s: directory \"%s\" exists but is not empty\n"),
 	progname, dirname);
-			disconnect_and_exit(1);
+			exit(1);
 		case -1:
 
 			/*
@@ -385,7 +385,7 @@ verify_dir_is_empty_or_create(char *dirn
 			 */
 			fprintf(stderr, _("%s: could not access directory \"%s\": %s\n"),
 	progname, dirname, strerror(errno));
-			disconnect_and_exit(1);
+			exit(1);
 	}
 }
 
@@ -493,7 +493,7 @@ writeTarData(
 			fprintf(stderr,
 _("%s: could not write to compressed file \"%s\": %s\n"),
 	progname, current_file, get_gz_error(ztarfile));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 	else
@@ -503,7 +503,7 @@ writeTarData(
 		{
 			fprintf(stderr, _("%s: could not write to file \"%s\": %s\n"),
 	progname, current_file, strerror(errno));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 }
@@ -557,7 +557,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 	fprintf(stderr,
 			_("%s: could not set compression level %d: %s\n"),
 			progname, compresslevel, get_gz_error(ztarfile));
-	disconnect_and_exit(1);
+	exit(1);
 }
 			}
 			else
@@ -577,7 +577,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 	fprintf(stderr,
 			_("%s: could not set compression level %d: %s\n"),
 			progname, compresslevel, get_gz_error(ztarfile));
-	disconnect_and_exit(1);
+	exit(1);
 }
 			}
 			else
@@ -605,7 +605,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 fprintf(stderr,
 		_("%s: could not set compression level %d: %s\n"),
 		progname, compresslevel, get_gz_error(ztarfile));
-disconnect_and_exit(1);
+exit(1);
 			}
 		}
 		else
@@ -626,7 +626,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 			fprintf(stderr,
 	_("%s: could not create compressed file \"%s\": %s\n"),
 	progname, filename, get_gz_error(ztarfile));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 	else
@@ -637,7 +637,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 		{
 			fprintf(stderr, _("%s: could not create file \"%s\": %s\n"),
 	progname, filename, strerror(errno));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 
@@ -649,7 +649,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 	{
 		fprintf(stderr, _("%s: could not get COPY data stream: %s"),
 progname, PQerrorMessage(conn));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 
 	/*

Re: [HACKERS] [COMMITTERS] pgsql: Unify some tar functionality across different parts

[Tom Lane]

Magnus Hagander  writes:
> On Wed, Jan 2, 2013 at 5:36 PM, Tom Lane  wrote:
>> Why are these very tar-specific functions being declared in such a
>> globally visible spot as port.h?  That seems like a bad idea on its
>> face.  IMO stuff in port.h ought to be about as globally applicable
>> as, say, malloc().

> It's where we put most of the things from src/port in.

Might be time to revisit that, or perhaps better reconsider what we're
putting in src/port/.  The idea that anything that we want in both
frontend and backend is a porting issue seems a bit busted in itself.

> I take it you suggest moving it to a special say include/pgtar.h file?

Works for me.

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] [COMMITTERS] pgsql: Unify some tar functionality across different parts

[Magnus Hagander]

On Wed, Jan 2, 2013 at 5:36 PM, Tom Lane  wrote:
> Magnus Hagander  writes:
>>> On Wed, Jan 2, 2013 at 4:14 AM, Andrew Dunstan  wrote:
 This seems to have broken plperl builds on Windows.
>
>> I'm not really sure what the best thing is to do here. We can either
>> use the fact that we know that uid_t and gid_t are "int" on all our
>> platforms, more or less, and change the tar api to use uid_t. Or put
>> the tar functions in their own header and make sure we don't include
>> that one anywhere that we include the perl headers.
>
> Why are these very tar-specific functions being declared in such a
> globally visible spot as port.h?  That seems like a bad idea on its
> face.  IMO stuff in port.h ought to be about as globally applicable
> as, say, malloc().

It's where we put most of the things from src/port in.

I take it you suggest moving it to a special say include/pgtar.h file?


> If there's actually a reason for that sort of namespace abuse, then
> change their parameters to int --- IIRC, the tar format can't support
> UIDs wider than 32 bits anyway.

Andrew suggested #ifndef'ing the declarations the same way that the
typedefs for uid_t and gid_t are done as another optin. I don't really
find either one of them non-kludgy :)


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] allowing multiple PQclear() calls

[Boszormenyi Zoltan]

2013-01-02 17:22 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan  writes:

2013-01-02 16:52 keltezéssel, Heikki Linnakangas írta:

IMHO this doesn't belong into libpq, the interface is fine as it is. It's the 
caller's
responsibility to set the pointer to NULL after PQclear(), same as it's the 
caller's
responsibility to set a pointer to NULL after calling free(), or to set the fd 
variable
to -1 after calling close(fd). There's plenty of precedence for this pattern, 
and it
shouldn't surprise any programmer.

Let me quote Simon Riggs here:

... we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.

Heikki is right and Simon is wrong.  This is not a very helpful idea,
and anybody who wants it is probably doing it already anyway.

There might be some value in the proposed macro if using it reliably
stopped bugs of this class, but it won't; the most obvious reason being
that there is seldom only one copy of a given PGconn pointer in an
application.  If you just blindly s/PQfinish(x)/PQfinishSafe(&x)/
you will most likely be zapping some low-level function's local
parameter copy, and thus accomplishing nothing of use.  You could
possibly make it work fairly consistently if you changed your entire
application to pass around PGconn** instead of PGconn*, but that would
be tedious and somewhat error-prone in itself; and none of the rest of
libpq's API is at all friendly to the idea.

The context where this sort of thing is actually helpful is C++, where
it's possible to introduce enough low-level infrastructure that the
programmer doesn't have to think about what he's doing when using
indirect pointers like that.  You can make a C++ wrapper class that is
both guaranteed safe (unlike this) and notationally transparent.
Of course, that has its own costs, but at least the language provides
some support.  So it'd be reasonable for libpqxx to do something like
this, but it's not very helpful for libpq to do it.  As Heikki says,
there is basically zero precedent for it in libc or other C libraries.
There's a reason for that.

regards, tom lane


OK, then forget about this patch.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



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

Re: [HACKERS] [COMMITTERS] pgsql: Unify some tar functionality across different parts

[Tom Lane]

Magnus Hagander  writes:
>> On Wed, Jan 2, 2013 at 4:14 AM, Andrew Dunstan  wrote:
>>> This seems to have broken plperl builds on Windows.

> I'm not really sure what the best thing is to do here. We can either
> use the fact that we know that uid_t and gid_t are "int" on all our
> platforms, more or less, and change the tar api to use uid_t. Or put
> the tar functions in their own header and make sure we don't include
> that one anywhere that we include the perl headers.

Why are these very tar-specific functions being declared in such a
globally visible spot as port.h?  That seems like a bad idea on its
face.  IMO stuff in port.h ought to be about as globally applicable
as, say, malloc().

If there's actually a reason for that sort of namespace abuse, then
change their parameters to int --- IIRC, the tar format can't support
UIDs wider than 32 bits 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] Review of Row Level Security

[Kohei KaiGai]

2012/12/31 Simon Riggs :
> On 23 December 2012 18:49, Simon Riggs  wrote:
>
>> Anyway, hope you can make call on 28th so we can discuss this and
>> agree a way forwards you're happy with.
>
> Stephen, KaiGai and myself met by phone on 28th to discuss.
>
> 1. The actual default is not that important to any of us. We could go
> either way, or have no default at all.
>
> 2. What we do want is a declarative way of specifying row security,
> with options to support all use cases discussed/requested on list. We
> shouldn't
> support just one of those use cases and force everybody else to use
> triggers manually for the other cases.
>
> 3. We want to have the possibility of multiple row security
> expressions, defined for different privilege types (SELECT, UPDATE,
> INSERT, DELETE). (Note that this means you'd be able to specify that
> an update could read a row in one security mode by setting SELECT,
> then update that row to a new security mode by setting a clause on
> UPDATE - hence we refer to those as privileges not commands/events).
> The expressions should be separate so they can be  pushed easily into
> query plans (exactly as in the current patch).
>
> Stephen has updated the Wiki with some ideas on how that can be structured
> https://wiki.postgresql.org/wiki/RLS
>
> 4. Supporting multiple expressions may not be possible for 9.3, but if
> not, we want to agree now what the syntax is to make sure we have a
> clear route for future development. If we can agree this quickly we
> increase the chances of KaiGai successfully implementing that.
>
The syntax being discussed were below:

  ALTER TABLE  SET ROW SECURITY FOR  TO ();
  ALTER TABLE  RESET ROW SECURITY FOR ;

   can be one of: ALL, SELECT, INSERT, UPDATE, DELETE

The point in development towards v9.3 is, we only support "ALL" but
we can add other command types in the future.
IMO, only "parser" should accept command types except for ALL but
raise an error something like "it is not supported yet" to protect from
syntax conflicts.

Right now, I plan to submit a revised patch with the syntax support
above, and without support for INSERT or NEW of UPDATE checks,
as minimum set of functionality for v9.3.

Please give me some suggestions, if you have different opinion
towards the overall direction, until 15th-Jan.

Thanks,
-- 
KaiGai Kohei 


-- 
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] allowing multiple PQclear() calls

[Dmitriy Igrishin]

2013/1/2 Heikki Linnakangas 

> On 02.01.2013 17:27, Marko Kreen wrote:
>
>> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan
>>  wrote:
>>
>>> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>>>
>>>  On 11 December 2012 12:18, Boszormenyi Zoltan  wrote:

  Such mechanism already exist - you just need to set
>>> your PGresult pointer to NULL after each PQclear().
>>>
>>
>> So why doesn't PQclear() do that?
>>
>
>
> Because then PQclear() would need a ** not a *. Do you want its
> interface changed for 9.3 and break compatibility with previous
> versions?
>

 No, but we should introduce a new public API call that is safer,
 otherwise we get people continually re-inventing new private APIs that
 Do the Right Thing, as the two other respondents have shown.


>>> How about these macros?
>>>
>>
>> * Use do { } while (0) around the macros to get proper statement
>> behaviour.
>> * The if() is not needed, both PQclear and PQfinish do it internally.
>> * Docs
>>
>> Should the names show somehow that they are macros?
>> Or is it enough that it's mentioned in documentation?
>>
>
> IMHO this doesn't belong into libpq, the interface is fine as it is. It's
> the caller's responsibility to set the pointer to NULL after PQclear(),
> same as it's the caller's responsibility to set a pointer to NULL after
> calling free(), or to set the fd variable to -1 after calling close(fd).
> There's plenty of precedence for this pattern, and it shouldn't surprise
> any programmer.

True. +1

-- 
// Dmitriy.

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Alvaro Herrera]

I committed this with minor tweaks to avoid having to scan the
registered workers list on each registration.  Opinions on this error
report are still welcome:

> +   ereport(LOG,
> +   
> (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
> +errmsg("too many background workers"),
> +errdetail("Up to %d background workers can 
> be registered with the current settings.",
> +  MAX_BACKENDS - 
> (MaxConnections + autovacuum_max_workers + 1;

Thanks to everyone for their input --- and happy 2013 hacking to all.

-- 
Á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] Minor fix in 'clean' action of 'src/backend/Makefile'

[Heikki Linnakangas]

On 02.01.2013 18:20, Heikki Linnakangas wrote:

Hmm, looking closer though, repl_gram.h is not actually needed for
anything, though. We could just remove the -d flag from the bison
invocation and not build it to begin with. I'll go and do that..


And looking even closer, we don't use the -d flag in git master anymore, 
so repl_gram.h is not being built, and there's nothing to do here. I 
suppose we could change back-branches to also not build it, but I don't 
think I'm going to bother.


- 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] allowing multiple PQclear() calls

[Tom Lane]

Boszormenyi Zoltan  writes:
> 2013-01-02 16:52 keltezéssel, Heikki Linnakangas írta:
>> IMHO this doesn't belong into libpq, the interface is fine as it is. It's 
>> the caller's 
>> responsibility to set the pointer to NULL after PQclear(), same as it's the 
>> caller's 
>> responsibility to set a pointer to NULL after calling free(), or to set the 
>> fd variable 
>> to -1 after calling close(fd). There's plenty of precedence for this 
>> pattern, and it 
>> shouldn't surprise any programmer.

> Let me quote Simon Riggs here:
>> ... we should introduce a new public API call that is safer,
>> otherwise we get people continually re-inventing new private APIs that
>> Do the Right Thing, as the two other respondents have shown.

Heikki is right and Simon is wrong.  This is not a very helpful idea,
and anybody who wants it is probably doing it already anyway.

There might be some value in the proposed macro if using it reliably
stopped bugs of this class, but it won't; the most obvious reason being
that there is seldom only one copy of a given PGconn pointer in an
application.  If you just blindly s/PQfinish(x)/PQfinishSafe(&x)/
you will most likely be zapping some low-level function's local
parameter copy, and thus accomplishing nothing of use.  You could
possibly make it work fairly consistently if you changed your entire
application to pass around PGconn** instead of PGconn*, but that would
be tedious and somewhat error-prone in itself; and none of the rest of
libpq's API is at all friendly to the idea.

The context where this sort of thing is actually helpful is C++, where
it's possible to introduce enough low-level infrastructure that the
programmer doesn't have to think about what he's doing when using
indirect pointers like that.  You can make a C++ wrapper class that is
both guaranteed safe (unlike this) and notationally transparent.
Of course, that has its own costs, but at least the language provides
some support.  So it'd be reasonable for libpqxx to do something like
this, but it's not very helpful for libpq to do it.  As Heikki says,
there is basically zero precedent for it in libc or other C libraries.
There's a reason for 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] Minor fix in 'clean' action of 'src/backend/Makefile'

[Heikki Linnakangas]

On 02.01.2013 18:05, Fabrízio de Royes Mello wrote:

Hi all,

When we execute 'make clean' the file 'src/backend/replication/repl_gram.h'
  is not removed.


That's on purpose. repl_gram.h is generated by bison, along with 
repl_gram.c. Neither is removed by "make clean", because they're needed 
in a distribution tarball. "make maintainer-clean" does remove them.


Hmm, looking closer though, repl_gram.h is not actually needed for 
anything, though. We could just remove the -d flag from the bison 
invocation and not build it to begin with. I'll go and do that..


- Heikki


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

[HACKERS] Big disconnect_and_exit cleanup in pg_basebackup

[Boszormenyi Zoltan]

Hi,

the previously sent "factor out pg_malloc and friends" patch
may call exit() in case of OOM during allocation and as a consequence,
PQfinish() won't get called, leaving an "unexpected EOF from client"
in the log.

Let's close this annoyance in pg_basebackup. The attached patch does
the following:

- adds a PQfinish() (actually PQfinishSafe()) call that was just posted
  in another thread
- replace all PQfinish() and PQclear() with their *Safe counterpart so
  a normal execution won't result in a crash because of calling PQfinish()
  on a stale pointer
- kill the disconnect_and_exit() macro, replace it with plain exit(),
  the atexit callback does the disconnect anyway.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

diff -durpN postgresql.4/src/bin/pg_basebackup/pg_basebackup.c postgresql.5/src/bin/pg_basebackup/pg_basebackup.c
--- postgresql.4/src/bin/pg_basebackup/pg_basebackup.c	2013-01-02 13:33:46.127749834 +0100
+++ postgresql.5/src/bin/pg_basebackup/pg_basebackup.c	2013-01-02 16:21:32.022484045 +0100
@@ -251,7 +251,7 @@ LogStreamerMain(logstreamer_param *param
 		 */
 		return 1;
 
-	PQfinish(param->bgconn);
+	PQfinishSafe(param->bgconn);
 	return 0;
 }
 
@@ -277,7 +277,7 @@ StartLogStreamer(char *startpos, uint32
 		fprintf(stderr,
 _("%s: could not parse transaction log location \"%s\"\n"),
 progname, startpos);
-		disconnect_and_exit(1);
+		exit(1);
 	}
 	param->startptr = ((uint64) hi) << 32 | lo;
 	/* Round off to even segment position */
@@ -290,7 +290,7 @@ StartLogStreamer(char *startpos, uint32
 		fprintf(stderr,
 _("%s: could not create pipe for background process: %s\n"),
 progname, strerror(errno));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 #endif
 
@@ -323,7 +323,7 @@ StartLogStreamer(char *startpos, uint32
 	{
 		fprintf(stderr, _("%s: could not create background process: %s\n"),
 progname, strerror(errno));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 
 	/*
@@ -335,7 +335,7 @@ StartLogStreamer(char *startpos, uint32
 	{
 		fprintf(stderr, _("%s: could not create background thread: %s\n"),
 progname, strerror(errno));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 #endif
 }
@@ -360,7 +360,7 @@ verify_dir_is_empty_or_create(char *dirn
 fprintf(stderr,
 		_("%s: could not create directory \"%s\": %s\n"),
 		progname, dirname, strerror(errno));
-disconnect_and_exit(1);
+exit(1);
 			}
 			return;
 		case 1:
@@ -377,7 +377,7 @@ verify_dir_is_empty_or_create(char *dirn
 			fprintf(stderr,
 	_("%s: directory \"%s\" exists but is not empty\n"),
 	progname, dirname);
-			disconnect_and_exit(1);
+			exit(1);
 		case -1:
 
 			/*
@@ -385,7 +385,7 @@ verify_dir_is_empty_or_create(char *dirn
 			 */
 			fprintf(stderr, _("%s: could not access directory \"%s\": %s\n"),
 	progname, dirname, strerror(errno));
-			disconnect_and_exit(1);
+			exit(1);
 	}
 }
 
@@ -493,7 +493,7 @@ writeTarData(
 			fprintf(stderr,
 _("%s: could not write to compressed file \"%s\": %s\n"),
 	progname, current_file, get_gz_error(ztarfile));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 	else
@@ -503,7 +503,7 @@ writeTarData(
 		{
 			fprintf(stderr, _("%s: could not write to file \"%s\": %s\n"),
 	progname, current_file, strerror(errno));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 }
@@ -557,7 +557,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 	fprintf(stderr,
 			_("%s: could not set compression level %d: %s\n"),
 			progname, compresslevel, get_gz_error(ztarfile));
-	disconnect_and_exit(1);
+	exit(1);
 }
 			}
 			else
@@ -577,7 +577,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 	fprintf(stderr,
 			_("%s: could not set compression level %d: %s\n"),
 			progname, compresslevel, get_gz_error(ztarfile));
-	disconnect_and_exit(1);
+	exit(1);
 }
 			}
 			else
@@ -605,7 +605,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 fprintf(stderr,
 		_("%s: could not set compression level %d: %s\n"),
 		progname, compresslevel, get_gz_error(ztarfile));
-disconnect_and_exit(1);
+exit(1);
 			}
 		}
 		else
@@ -626,7 +626,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 			fprintf(stderr,
 	_("%s: could not create compressed file \"%s\": %s\n"),
 	progname, filename, get_gz_error(ztarfile));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 	else
@@ -637,7 +637,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 		{
 			fprintf(stderr, _("%s: could not create file \"%s\": %s\n"),
 	progname, filename, strerror(errno));
-			disconnect_and_exit(1);
+			exit(1);
 		}
 	}
 
@@ -649,7 +649,7 @@ ReceiveTarFile(PGconn *conn, PGresult *r
 	{
 		fprintf(stderr, _("%s: could not get COPY data stream: %s"),
 progname, PQerrorMessage(conn));
-		disconnect_and_exit(1);
+		exit(1);
 	}
 
 	/*
@@ -71

[HACKERS] Minor fix in 'clean' action of 'src/backend/Makefile'

[Fabrízio de Royes Mello]

Hi all,

When we execute 'make clean' the file 'src/backend/replication/repl_gram.h'
 is not removed.

The attached patch fix it.

Regards,

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello


fix_clean_src_backend_makefile.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] allowing multiple PQclear() calls

[Boszormenyi Zoltan]

2013-01-02 16:52 keltezéssel, Heikki Linnakangas írta:

On 02.01.2013 17:27, Marko Kreen wrote:

On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan  wrote:

2012-12-11 16:09 keltezéssel, Simon Riggs írta:


On 11 December 2012 12:18, Boszormenyi Zoltan  wrote:


Such mechanism already exist - you just need to set
your PGresult pointer to NULL after each PQclear().


So why doesn't PQclear() do that?



Because then PQclear() would need a ** not a *. Do you want its
interface changed for 9.3 and break compatibility with previous versions?


No, but we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.



How about these macros?


* Use do { } while (0) around the macros to get proper statement behaviour.
* The if() is not needed, both PQclear and PQfinish do it internally.
* Docs

Should the names show somehow that they are macros?
Or is it enough that it's mentioned in documentation?


IMHO this doesn't belong into libpq, the interface is fine as it is. It's the caller's 
responsibility to set the pointer to NULL after PQclear(), same as it's the caller's 
responsibility to set a pointer to NULL after calling free(), or to set the fd variable 
to -1 after calling close(fd). There's plenty of precedence for this pattern, and it 
shouldn't surprise any programmer.


Let me quote Simon Riggs here:

... we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.


Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



--
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] allowing multiple PQclear() calls

[Boszormenyi Zoltan]

2013-01-02 16:27 keltezéssel, Marko Kreen írta:

On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan  wrote:

2012-12-11 16:09 keltezéssel, Simon Riggs írta:


On 11 December 2012 12:18, Boszormenyi Zoltan  wrote:


Such mechanism already exist - you just need to set
your PGresult pointer to NULL after each PQclear().

So why doesn't PQclear() do that?


Because then PQclear() would need a ** not a *. Do you want its
interface changed for 9.3 and break compatibility with previous versions?

No, but we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.


How about these macros?

* Use do { } while (0) around the macros to get proper statement behaviour.
* The if() is not needed, both PQclear and PQfinish do it internally.
* Docs

Should the names show somehow that they are macros?
Or is it enough that it's mentioned in documentation?


Done. The fact that these are macros is mentioned in the docs.

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

diff -durpN postgresql.3/doc/src/sgml/libpq.sgml postgresql.4/doc/src/sgml/libpq.sgml
--- postgresql.3/doc/src/sgml/libpq.sgml	2012-11-30 09:29:49.703286090 +0100
+++ postgresql.4/doc/src/sgml/libpq.sgml	2013-01-02 16:54:43.783565292 +0100
@@ -588,6 +588,27 @@ void PQfinish(PGconn *conn);
  
 
 
+
+ PQfinishSafePQfinishSafe
+ 
+  
+   A macro that calls PQfinish and sets
+   the pointer to NULL afterwards.
+
+#define PQfinishSafe(conn) do { PQfinish(conn); conn = NULL; } while (0)
+
+  
+
+  
+   The PGconn pointer (being NULL) is safe to use
+   after this macro. Every function that deals with a
+   PGconn pointer considers a non-NULL pointer as valid.
+   Using a stale pointer may result in a crash. Setting the pointer
+   to NULL makes calling such functions a no-op instead.
+  
+ 
+
+
 
  PQresetPQreset
  
@@ -2753,6 +2774,29 @@ void PQclear(PGresult *res);

   
  
+
+ 
+  PQclearSafePQclearSafe
+  
+   
+A macro to call PQclear and set
+the pointer to NULL afterwards.
+
+#define PQclearSafe(res) do { PQclear(res); res = NULL; } while (0)
+
+   
+
+   
+Calling PQclear leaves a stale
+PGresult pointer. Calling
+functions that deal with PGresult
+objects afterwards may result in a crash. Setting the
+PGresult pointer to NULL makes
+calling such functions a no-op instead.
+   
+  
+ 
+
 

   
diff -durpN postgresql.3/src/interfaces/libpq/libpq-fe.h postgresql.4/src/interfaces/libpq/libpq-fe.h
--- postgresql.3/src/interfaces/libpq/libpq-fe.h	2013-01-02 09:19:03.929522614 +0100
+++ postgresql.4/src/interfaces/libpq/libpq-fe.h	2013-01-02 16:52:41.236683245 +0100
@@ -256,6 +256,9 @@ extern PGconn *PQsetdbLogin(const char *
 /* close the current connection and free the PGconn data structure */
 extern void PQfinish(PGconn *conn);
 
+/* macro to close the current connection and set the conn pointer to NULL */
+#define PQfinishSafe(conn) do { PQfinish(conn); conn = NULL; } while (0)
+
 /* get info about connection options known to PQconnectdb */
 extern PQconninfoOption *PQconndefaults(void);
 
@@ -472,6 +475,9 @@ extern int	PQsendDescribePortal(PGconn *
 /* Delete a PGresult */
 extern void PQclear(PGresult *res);
 
+/* Macro to delete a PGresult and set the res pointer to NULL */
+#define PQclearSafe(res) do { PQclear(res); res = NULL; } while (0)
+
 /* For freeing other alloc'd results, such as PGnotify structs */
 extern void PQfreemem(void *ptr);
 

-- 
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] allowing multiple PQclear() calls

[Heikki Linnakangas]

On 02.01.2013 17:27, Marko Kreen wrote:

On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan  wrote:

2012-12-11 16:09 keltezéssel, Simon Riggs írta:


On 11 December 2012 12:18, Boszormenyi Zoltan  wrote:


Such mechanism already exist - you just need to set
your PGresult pointer to NULL after each PQclear().


So why doesn't PQclear() do that?



Because then PQclear() would need a ** not a *. Do you want its
interface changed for 9.3 and break compatibility with previous versions?


No, but we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.



How about these macros?


* Use do { } while (0) around the macros to get proper statement behaviour.
* The if() is not needed, both PQclear and PQfinish do it internally.
* Docs

Should the names show somehow that they are macros?
Or is it enough that it's mentioned in documentation?


IMHO this doesn't belong into libpq, the interface is fine as it is. 
It's the caller's responsibility to set the pointer to NULL after 
PQclear(), same as it's the caller's responsibility to set a pointer to 
NULL after calling free(), or to set the fd variable to -1 after calling 
close(fd). There's plenty of precedence for this pattern, and it 
shouldn't surprise any programmer.


- 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] recent ALTER whatever .. SET SCHEMA refactoring

[Kohei KaiGai]

2012/12/20 Robert Haas :
> The recent SET SCHEMA refactoring has changed the error message that
> you get when trying to move a function into the schema that already
> contains it.
>
> For a table, as ever, you get:
>
> rhaas=# create table foo (a int);
> CREATE TABLE
> rhaas=# alter table foo set schema public;
> ERROR:  table foo is already in schema "public"
>
> Functions used to produce the same message, but not any more:
>
> rhaas=# create function foo(a int) returns int as $$select 1$$ language sql;
> CREATE FUNCTION
> rhaas=# alter function foo(int) set schema public;
> ERROR:  function "foo" already exists in schema "public"
>
> The difference is that the first error message is complaining about an
> operation that is a no-op, whereas the second one is complaining about
> a name collision.  It seems a bit off in this case because the name
> collision is between the function and itself, not the function and
> some other object with a conflicting name.  The root of the problem is
> that AlterObjectNamespace_internal generates both error messages and
> does the checks in the correct order, but for functions,
> AlterFunctionNamespace_oid has a second copy of the conflicting-names
> check that is argument-type aware, which happens before the
> same-schema check in AlterObjectNamespace_internal.
>
> IMHO, we ought to fix this by getting rid of the check in
> AlterFunctionNamespace_oid and adding an appropriate special case for
> functions in AlterObjectNamespace_internal that knows how to do the
> check correctly.  It's not a huge deal, but it seems confusing to have
> AlterObjectNamespace_internal know how to do the check correctly in
> some cases but not others.
>
Sorry for my oversight this message.

It seems to me it is a reasonable solution to have a special case for
object types that does not support simple name looking-up with name
itself or combination of name + namespace.
Function and collation are candidates of this special case handling;
here are just two kinds of object.

Another idea is to add a function-pointer as argument of
AlterNamespace_internal for (upcoming) object classes that takes
special handling for detection of name collision.
My personal preference is the later one, rather than hardwired
special case handling.
However, it may be too elaborate to handle just two exceptions.

Thanks,
-- 
KaiGai Kohei 


-- 
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 .. OWNER TO error mislabels schema as other object type

[Kohei KaiGai]

Sorry, I oversight this report.

The reason of this confusing error message is originated by incorrect
aclkind being delivered to aclcheck_error() at AlterObjectOwner_internal().

/* New owner must have CREATE privilege on namespace */
if (OidIsValid(namespaceId))
{
AclResult   aclresult;

aclresult = pg_namespace_aclcheck(namespaceId, new_ownerId,
  ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, aclkind,
   get_namespace_name(namespaceId));
}

The supplied aclkind represents the property of the object being re-owned,
not a namespace that owns the target object. So, right approach is to
give ACL_KIND_NAMESPACE being hardwired in this case, as
AlterObjectNamespace_internal() doing.

The attached patch fixes this trouble.

postgres=# create role clerks;
CREATE ROLE
postgres=# create role bob in role clerks;
CREATE ROLE
postgres=# create schema foo;
CREATE SCHEMA
postgres=# grant usage on schema foo to bob, clerks;
GRANT
postgres=# create aggregate
postgres-# foo.sum(basetype=text,sfunc=textcat,stype=text,initcond='');
CREATE AGGREGATE
postgres=# alter aggregate foo.sum(text) owner to bob;
ALTER AGGREGATE
postgres=# set role bob;
SET
postgres=> alter aggregate foo.sum(text) owner to clerks;
ERROR:  permission denied for schema foo

Thanks,

2012/12/20 Robert Haas :
> This looks busted:
>
> rhaas=# create role clerks;
> CREATE ROLE
> rhaas=# create role bob in role clerks;
> CREATE ROLE
> rhaas=# create schema foo;
> CREATE SCHEMA
> rhaas=# grant usage on schema foo to bob, clerks;
> GRANT
> rhaas=# create aggregate
> foo.sum(basetype=text,sfunc=textcat,stype=text,initcond='');
> CREATE AGGREGATE
> rhaas=# alter aggregate foo.sum(text) owner to bob;
> ALTER AGGREGATE
> rhaas=# set role bob;
> SET
> rhaas=> alter aggregate foo.sum(text) owner to clerks;
> ERROR:  permission denied for function foo
>
> Eh?  There's no function called foo.  There's a schema called foo,
> which seems to be the real problem: clerks needs to have CREATE on foo
> in order for bob to complete the rename.  But somehow the error
> message is confused about what type of object it's dealing with.
>
> [ Credit: The above example is adapted from an EDB-internal regression
> test, the failure of which was what alerted me to this problem. ]
>
> --
> 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



-- 
KaiGai Kohei 


pgsql-fix-incorrect-aclkind-on-alter-owner.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] allowing multiple PQclear() calls

[Marko Kreen]

On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan  wrote:
> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>
>> On 11 December 2012 12:18, Boszormenyi Zoltan  wrote:
>>
> Such mechanism already exist - you just need to set
> your PGresult pointer to NULL after each PQclear().

 So why doesn't PQclear() do that?
>>>
>>>
>>> Because then PQclear() would need a ** not a *. Do you want its
>>> interface changed for 9.3 and break compatibility with previous versions?
>>
>> No, but we should introduce a new public API call that is safer,
>> otherwise we get people continually re-inventing new private APIs that
>> Do the Right Thing, as the two other respondents have shown.
>>
>
> How about these macros?

* Use do { } while (0) around the macros to get proper statement behaviour.
* The if() is not needed, both PQclear and PQfinish do it internally.
* Docs

Should the names show somehow that they are macros?
Or is it enough that it's mentioned in documentation?

-- 
marko


-- 
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] allowing multiple PQclear() calls

[Boszormenyi Zoltan]

2012-12-11 16:09 keltezéssel, Simon Riggs írta:

On 11 December 2012 12:18, Boszormenyi Zoltan  wrote:


Such mechanism already exist - you just need to set
your PGresult pointer to NULL after each PQclear().

So why doesn't PQclear() do that?


Because then PQclear() would need a ** not a *. Do you want its
interface changed for 9.3 and break compatibility with previous versions?

No, but we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.



How about these macros?

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

diff -durpN postgresql.3/src/interfaces/libpq/libpq-fe.h postgresql.4/src/interfaces/libpq/libpq-fe.h
--- postgresql.3/src/interfaces/libpq/libpq-fe.h	2013-01-02 09:19:03.929522614 +0100
+++ postgresql.4/src/interfaces/libpq/libpq-fe.h	2013-01-02 16:10:51.978974575 +0100
@@ -256,6 +256,16 @@ extern PGconn *PQsetdbLogin(const char *
 /* close the current connection and free the PGconn data structure */
 extern void PQfinish(PGconn *conn);
 
+/* macro to close the current connection and set the conn pointer to NULL */
+#define PQfinishSafe(conn)		\
+	{\
+		if (conn)		\
+		{			\
+			PQfinish(conn);	\
+			conn = NULL;	\
+		}			\
+	}
+
 /* get info about connection options known to PQconnectdb */
 extern PQconninfoOption *PQconndefaults(void);
 
@@ -472,6 +482,16 @@ extern int	PQsendDescribePortal(PGconn *
 /* Delete a PGresult */
 extern void PQclear(PGresult *res);
 
+/* Macro to delete a PGresult and set the res pointer to NULL */
+#define PQclearSafe(res)		\
+	{\
+		if (res)		\
+		{			\
+			PQclear(res);	\
+			res = NULL;	\
+		}			\
+	}
+
 /* For freeing other alloc'd results, such as PGnotify structs */
 extern void PQfreemem(void *ptr);
 

-- 
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] default SSL compression (was: libpq compression)

[Claudio Freire]

On Wed, Jan 2, 2013 at 10:03 AM, Magnus Hagander  wrote:
> Finally we deny MD5 - I have no idea why we do that.

Because it's broken, same motivation as in the thread for implementing
ZK authentication.

Also, I seem to have missed something because the thread subject
mentions compression whereas I've seen no mention of it in the
discussion (maybe I just missed it). But, in any case, I would like to
point out this[0]. I'm not sure it's readily applicable to PG's wire
protocol, but just in case I mention it.

[0] http://news.idg.no/cw/art.cfm?id=976ED08F-CB4A-23DA-FFDA0419B8750B72


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

[HACKERS] Adding rewrite rule to QueryRewrite

[Andreas Heinisch]

Hi,

we are trying to integrate GMDA algorithm into postgresql. The operater
decouples grouping from
aggregation and we came up with the following syntax:

SELECT d, u, COUNT(*b.u <= r.u*) AS ccu, COUNT(*b.d <= r.d*) AS ccd
FROM b *GMDJOIN* r
GROUP BY d, u

This select query should be rewritten to:

WITH xi AS (
  SELECT d, u
SUM(COUNT(*)) OVER (PARTITION BY u) AS ccu,
SUM(COUNT(*)) OVER (PARTITION BY d) AS ccd
  FROM r
  GROUP BY d, u
),
x1 AS (
  SELECT b1.u, SUM(ccu) AS ccu
  FROM
(SELECT DISTINCT u FROM xi) b1
LEFT OUTER JOIN
(SELECT DISTINCT u, ccu FROM xi) xi1
ON xi1.u = b1.u
  GROUP BY b1.u
),
x2 AS (
  SELECT b2.d, SUM(ccd) AS ccd
  FROM
(SELECT DISTINCT d FROM xi) b2
LEFT OUTER JOIN
(SELECT DISTINCT d, ccd FROM xi) xi2
ON xi2.d = b2.d
  GROUP BY b2.d
)
SELECT *
FROM x1 NATURAL JOIN x2
ORDER BY d,u

Now to our question:
Can this be integrated into the postresql QueryRewrite function? Should that
be integrated into the rewrite stage of the postgresql kernel or should the
rewrite action be done somewhere else? Is there any tutorial on rewriting
statements?

Thank you very much for your time and help,
Andi



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Adding-rewrite-rule-to-QueryRewrite-tp5738481.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] default SSL compression (was: libpq compression)

[Magnus Hagander]

On Wed, Jan 2, 2013 at 3:15 PM, Noah Misch  wrote:
> On Wed, Jan 02, 2013 at 02:03:20PM +0100, Magnus Hagander wrote:
>> On Wed, Jan 2, 2013 at 1:15 AM, Tom Lane  wrote:
>> > So +1 for changing it to "DEFAULT" from me, too.  There's no reason to
>> > think we know more about this than the OpenSSL authors.
>>
>> The DEFAULT value in OpenSSL 1.0 means "ALL:!aNULL:!eNULL".
>>
>> Researching some more, this might cause a problem actually, which
>> would explain some of the things that are in our default. For example,
>> an ADH algorithm doesn't use certificates - but it uses DH parameters,
>> so it likely won't work anyway. EDH uses certs, but also requires DH
>> parameters.
>>
>> Maybe what we nede is "DEFAULT:!ADH:@STRENGTH" as the default?
>
> I understand aNULL to include ADH.

Hmm. Seems you're right when I run a test on it, I was reading it wrong.


>> The other difference is that our current string denies 40 and 56 bit
>> encryptions (low and export strenghts). Do we stll want to do that?
>
> On the one hand, those seem bad to permit by default in 2013.  On the other
> hand, if so, why hasn't OpenSSL removed them from DEFAULT?  Perhaps it has
> backward-compatibility concerns that wouldn't apply to us by virtue of having
> disabled them for some time.  Sounds reasonable to continue disabling them.

So then the default would be "DEFAULT:!LOW:!EXP:@STRENGTH"

(the @STRENGTH part is the sorting key for preference, which the
default one seems not to have)

The biggest difference being that we start from DEFAULT rather than ALL.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.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] default SSL compression (was: libpq compression)

[Noah Misch]

On Wed, Jan 02, 2013 at 02:03:20PM +0100, Magnus Hagander wrote:
> On Wed, Jan 2, 2013 at 1:15 AM, Tom Lane  wrote:
> > So +1 for changing it to "DEFAULT" from me, too.  There's no reason to
> > think we know more about this than the OpenSSL authors.
> 
> The DEFAULT value in OpenSSL 1.0 means "ALL:!aNULL:!eNULL".
> 
> Researching some more, this might cause a problem actually, which
> would explain some of the things that are in our default. For example,
> an ADH algorithm doesn't use certificates - but it uses DH parameters,
> so it likely won't work anyway. EDH uses certs, but also requires DH
> parameters.
> 
> Maybe what we nede is "DEFAULT:!ADH:@STRENGTH" as the default?

I understand aNULL to include ADH.

> The other difference is that our current string denies 40 and 56 bit
> encryptions (low and export strenghts). Do we stll want to do that?

On the one hand, those seem bad to permit by default in 2013.  On the other
hand, if so, why hasn't OpenSSL removed them from DEFAULT?  Perhaps it has
backward-compatibility concerns that wouldn't apply to us by virtue of having
disabled them for some time.  Sounds reasonable to continue disabling them.


-- 
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_basebackup from cascading standby after timeline switch

[Heikki Linnakangas]

On 27.12.2012 12:06, Heikki Linnakangas wrote:

On 23.12.2012 15:33, Fujii Masao wrote:

On Fri, Dec 21, 2012 at 9:54 PM, Heikki Linnakangas
 wrote:

Yes, this should be backpatched to 9.2. I came up with the attached.


In this patch, if '-X stream' is specified in pg_basebackup, the timeline
history files are not backed up.


Good point.


We should change pg_backup background
process and walsender so that they stream also timeline history files,
for example, by using 'TIMELINE_HISTORY' replication command?
Or basebackup.c should send all timeline history files at the end of
backup
even if '-X stream' is specified?


Perhaps. We should enhance pg_receivexlog to follow timeline switches,
anyway. I was thinking of leaving that as a todo item, but pg_basebackup
-X stream shares the code, so we should implement that now to get that
support into both.

In the problem you reported on the other thread
(http://archives.postgresql.org/message-id/50db5ea9.7010...@vmware.com),
you also need the timeline history files, but that one didn't use "-X"
at all. Even if we teach pg_basebackup to fetch the timeline history
files in "-X stream" mode, that still leaves the problem on that other
thread.

The simplest solution would be to always include all timeline history
files in the backup, even if -X is not used. Currently, however, pg_xlog
is backed up as an empty directory in that case, but that would no
longer be the case if we start including timeline history files there. I
wonder if that would confuse any existing backup scripts people are using.


This thread has spread out a bit, so here's a summary of the remaining 
issues and what I'm going to do about them:


9.2
---

If you take a backup with "pg_basebackup -X fetch", and the timeline 
switches while the backup is taken, you currently get an error like 
"requested WAL segment 0001000C has already been 
removed". To fix, let's change the server-side support of "-X fetch" to 
include all WAL files between the backup start and end pointers, 
regardless of timelines. I'm thinking of doing this by scanning pg_xlog 
with readdir(), and sending over any files in that range. Another option 
would be to read and parse the timeline history file to figure out the 
exact filenames expected, but the readdir() approach seems simpler.


You also need the timeline history files. With "-X fetch", I think we 
should always include them in the pg_xlog directory of the backup, along 
with the WAL files themselves.


"-X stream" has a similar problem. If timeline changes during backup, 
the replication will stop at the timeline switch, and the backup fails. 
There isn't much we can do about that, as you can't follow a timeline 
switch via streaming replication in 9.2. At best, we could try to detect 
the situation and give a better error message.


With plain pg_basebackup, without -X option, you usually need a WAL 
archive to restore. The only exception is when you initialize a 
streaming standby with plain "pg_basebackup", intending to connect it to 
the master right after taking the backup, so that it can stream all the 
required WAL at that point. We have a problem with that scenario, 
because even if there was no timeline switch while the backup was taken 
(if there was, you're screwed the same as with "-X stream"), because of 
the issue mentioned in the first post in this thread: the beginning of 
the first WAL file contains the old timeline ID. Even though that's not 
replayed, because the checkpoint is in the later part of the segment, 
recovery still complains if there is a timeline ID in the beginning of 
the file that we don't recognize as our ancestor. This could be fixed if 
we somehow got the timeline history files in the backup, but I'm worried 
that might break people's backup scripts. At the moment, the pg_xlog 
directory in the backup is empty when -X is not used, we even spell that 
out explicitly in the manual. Including timeline history files would 
change that. That might be an issue if you symlink pg_xlog to a 
different drive, for example. To make things worse, there is no timeline 
history file for timeline 1, so you would not notice when you test your 
backup scripts in simple cases.


In summary, in 9.2 I think we should fix "-X fetch" to tolerate a 
timeline switch, and include all the timeline history files. The 
behavior of other modes would not be changed, and they will fail if a 
timeline changes during or just before backup.


Master
--

In master, we can try harder for the "-X stream" case, because you can 
replicate a timeline switch, and fetch timeline history files via a 
replication connection. Let's teach pg_receivexlog, and "pg_basebackup 
-X stream", to use those facilities, so that even if the timeline 
changes while the backup is taken, all the history files and WAL files 
are correctly included in the backup. I'll start working on a patch to 
do that.


That leaves one case not covered: If you take a backup with plain 

Re: [HACKERS] default SSL compression (was: libpq compression)

[Magnus Hagander]

On Wed, Jan 2, 2013 at 1:15 AM, Tom Lane  wrote:
>
> Noah Misch  writes:
> > On Tue, Jan 01, 2013 at 04:29:35PM +0100, Magnus Hagander wrote:
> >> On Thu, Aug 30, 2012 at 11:41 PM, Bruce Momjian  wrote:
> >>> Do we want to change our ssl_ciphers default to 'DEFAULT'?  Currently it
> >>> is 'ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH'.
>
> >> Did we ever get anywhere with this? Is this a change we want to do for 9.3?
> >> Since nobody seems to have come up with a motivation for not following the
> >> openssl default, we probably should?
>
> > +1 for doing that.  I'm not aware of a PostgreSQL-specific selection 
> > criterion
> > for SSL cipher suites.
>
> I did a bit of digging in the commit logs.  The current default was
> introduced in commit 17386ac45345fe38a10caec9d6e63afa3ce31bb9.  So far
> as I can find, the only discussion leading up to that patch was the
> thread starting at
> http://archives.postgresql.org/pgsql-interfaces/2003-04/msg00075.php
> which only discusses the key renegotiation interval, not anything about
> selecting the allowed ciphers.  What's more, one might be forgiven for
> suspecting that the cipherset string wasn't too carefully researched
> after noticing that it wasn't even spelled correctly in that commit.


Yeah, clearly seems that way.


> So +1 for changing it to "DEFAULT" from me, too.  There's no reason to
> think we know more about this than the OpenSSL authors.
>

The DEFAULT value in OpenSSL 1.0 means "ALL:!aNULL:!eNULL".

Researching some more, this might cause a problem actually, which
would explain some of the things that are in our default. For example,
an ADH algorithm doesn't use certificates - but it uses DH parameters,
so it likely won't work anyway. EDH uses certs, but also requires DH
parameters.

Maybe what we nede is "DEFAULT:!ADH:@STRENGTH" as the default?

The other difference is that our current string denies 40 and 56 bit
encryptions (low and export strenghts). Do we stll want to do that?

Finally we deny MD5 - I have no idea why we do that.


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


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

[HACKERS] [PATCH] Factor out pg_malloc and friends into port code

[Boszormenyi Zoltan]

2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan > wrote:


2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan mailto:z...@cybertec.at>> writes:

2013-01-01 17:18 keltezéssel, Magnus Hagander írta:

That way we can get around the whole need for changing memory 
allocation
across all the
frontends, no? Like the attached.

Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
  fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either 
explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.

Meh.  I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.


Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.


Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's 
good or bad features.


The patch is attached. There is no extra atexit() code in this one.

I did this over my pg_basebackup patch, there are two chunks
that gets rejected if applied without it: one in initdb.c, the other is
in src/port/Makefile. It's because the modified codes are too close
to each other.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

diff -durpN postgresql.2/contrib/oid2name/oid2name.c postgresql.3/contrib/oid2name/oid2name.c
--- postgresql.2/contrib/oid2name/oid2name.c	2012-10-03 10:40:48.241207023 +0200
+++ postgresql.3/contrib/oid2name/oid2name.c	2013-01-02 13:40:08.483260103 +0100
@@ -50,9 +50,6 @@ struct options
 /* function prototypes */
 static void help(const char *progname);
 void		get_opts(int, char **, struct options *);
-void	   *pg_malloc(size_t size);
-void	   *pg_realloc(void *ptr, size_t size);
-char	   *pg_strdup(const char *str);
 void		add_one_elt(char *eltname, eary *eary);
 char	   *get_comma_elts(eary *eary);
 PGconn	   *sql_conn(struct options *);
@@ -201,53 +198,6 @@ help(const char *progname)
 		   progname, progname);
 }
 
-void *
-pg_malloc(size_t size)
-{
-	void	   *ptr;
-
-	/* Avoid unportable behavior of malloc(0) */
-	if (size == 0)
-		size = 1;
-	ptr = malloc(size);
-	if (!ptr)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return ptr;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
-	void	   *result;
-
-	/* Avoid unportable behavior of realloc(NULL, 0) */
-	if (ptr == NULL && size == 0)
-		size = 1;
-	result = realloc(ptr, size);
-	if (!result)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return result;
-}
-
-char *
-pg_strdup(const char *str)
-{
-	char	   *result = strdup(str);
-
-	if (!result)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return result;
-}
-
 /*
  * add_one_elt
  *
diff -durpN postgresql.2/contrib/pgbench/pgbench.c postgresql.3/contrib/pgbench/pgbench.c
--- postgresql.2/contrib/pgbench/pgbench.c	2013-01-02 09:19:03.655519614 +0100
+++ postgresql.3/contrib/pgbench/pgbench.c	2013-01-02 13:40:26.539378189 +0100
@@ -296,58 +296,6 @@ static void setalarm(int seconds);
 static void *threadRun(void *arg);
 
 
-/*
- * routines to check mem allocations and fail noisily.
- */
-static void *
-pg_malloc(size_t size)
-{
-	void	   *result;
-
-	/* Avoid unportable behavior of malloc(0) */
-	if (size == 0)
-		size = 1;
-	result = malloc(size);
-	if (!result)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return result;
-}
-
-static void *
-pg_realloc(void *ptr, size_t size)
-{
-	void	   *result;
-
-	/* Avoid unportable behavior of realloc(NULL, 0) */
-	if (ptr == NULL && size == 0)
-		size = 1;
-	result = realloc(ptr, size);
-	if (!result)
-	{
-		fprintf(stderr, "out of memory\n");
-		exit(1);
-	}
-	return result;
-}
-
-st

Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]

[Boszormenyi Zoltan]

2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta:

2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan > wrote:


2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan mailto:z...@cybertec.at>> writes:

2013-01-01 17:18 keltezéssel, Magnus Hagander írta:

That way we can get around the whole need for changing memory
allocation across all the
frontends, no? Like the attached.

Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
  fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either 
explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.

Meh.  I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.


Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.


Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's 
good or bad features.


I like Magnus' version a lot better than that idea.


OK, I will post the core patch building on his code.


Thanks.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions.  If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane


The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?


We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server encondings, 
and I believe it's all for this reason.


I see, thanks.

That said, do we need to care *in this specific case*? We use it in initdb to parse 
config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, 
correct?


Correct.

Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii 
safe encodings, or rename it to indicate that it's limited in this?


If you send a new patch with the function renamed accordingly, I will modify
my code to use it.


Attached is the quotes-v2 patch, the function is renamed and
the comment is modified plus the pg_basebackup v21 patch
that uses this function.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

diff -durpN postgresql/src/bin/initdb/initdb.c postgresql.1/src/bin/initdb/initdb.c
--- postgresql/src/bin/initdb/initdb.c	2013-01-02 09:19:03.855521804 +0100
+++ postgresql.1/src/bin/initdb/initdb.c	2013-01-02 11:26:10.008494149 +0100
@@ -354,6 +354,18 @@ pg_strdup(const char *s)
 	return result;
 }
 
+static char *
+escape_quotes(const char *src)
+{
+	char *result = escape_single_quotes_ascii(src);
+	if (!result)
+	{
+		fprintf(stderr, _("%s: out of memory\n"), progname);
+		exit(1);
+	}
+	return result;
+}
+
 /*
  * make a copy of the array of lines, with token replaced by replacement
  * the first time it occurs on each line.
@@ -2415,35 +2427,6 @@ check_ok(void)
 	}
 }
 
-/*
- * Escape (by doubling) any single quotes or backslashes in given string
- *
- * Note: this is used to process both postgresql.conf entries and SQL
- * string literals.  Since postgresql.conf strings are defined to treat
- * backslashes as escapes, we have to double backslashes here.	Hence,
- * when using this for a SQL string literal, use E'' syntax.
- *
- * We do not need to worry about encoding considerations because all
- * valid backend encodings are ASCII-safe.
- */
-static char *
-escape_qu

Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]

[Boszormenyi Zoltan]

2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan > wrote:


2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan mailto:z...@cybertec.at>> writes:

2013-01-01 17:18 keltezéssel, Magnus Hagander írta:

That way we can get around the whole need for changing memory 
allocation
across all the
frontends, no? Like the attached.

Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
  fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either 
explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.

Meh.  I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.


Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.


Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's 
good or bad features.


I like Magnus' version a lot better than that idea.


OK, I will post the core patch building on his code.


Thanks.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions.  If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane


The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?


We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server encondings, 
and I believe it's all for this reason.


I see, thanks.

That said, do we need to care *in this specific case*? We use it in initdb to parse 
config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, 
correct?


Correct.

Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe 
encodings, or rename it to indicate that it's limited in this?


If you send a new patch with the function renamed accordingly, I will modify
my code to use it.

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/

Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]

[Magnus Hagander]

On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan  wrote:

> 2013-01-02 01:24 keltezéssel, Tom Lane írta:
>
>  Boszormenyi Zoltan  writes:
>>
>>> 2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
>>>
 That way we can get around the whole need for changing memory
 allocation across all the
 frontends, no? Like the attached.

>>> Sure it's simpler but then the consistent look of the code is lost.
>>> What about the other patch to unify pg_malloc and friends?
>>> Basically all client code boils down to
>>>   fprintf(stderr, ...)
>>> in different disguise in their error reporting, so that patch can
>>> also be simplified but it seems that the atexit() - either explicitly
>>> or hidden behind InitPostgresFrontend() - cannot be avoided.
>>>
>> Meh.  I find it seriously wrongheaded that something as minor as an
>> escape_quotes() function should get to dictate both malloc wrappers
>> and error recovery handling throughout every program that might use it.
>>
>
> Actually, the unification of pg_malloc and friends wasn't dictated
> by this little code, it was just that pg_basebackup doesn't provide
> a pg_malloc implementation (only pg_malloc0) that is used by
> initdb's escape_quotes() function. Then I noticed how wide these
> almost identical functions have spread into client apps already.
>
> I would say this unification patch is completely orthogonal to
> the patch in $SUBJECT. I will post it in a different thread if it's
> wanted at all. The extra atexit() handler is not needed if a simple
> fprintf(stderr, ...) error reporting is enough in all clients.
> As far as I saw, all clients do exactly this but some of them hide
> this behind #define's.


Please do keep that one separate - let's avoid unnecessary feature-creep,
whether it's good or bad features.


I like Magnus' version a lot better than that idea.
>>
>
> OK, I will post the core patch building on his code.


Thanks.


 A bigger issue that I notice with this code is that it's only correct in
>> backend-safe encodings, as the comment mentions.  If we're going to be
>> putting it into frontend programs, how safe is that going to be?
>>
>> regards, tom lane
>>
>
> The question in a different form is: does PostgreSQL support
> non-ASCII-safe encodings at all (or on the client side)? Forgive
> my ignorance and enlighten me: how many such encodings
> exist besides EBCDIC? Is UTF-16 non-ASCII-safe?
>
>
We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server
encondings, and I believe it's all for this reason.

That said, do we need to care *in this specific case*? We use it in initdb
to parse config strings, I believe. And we'd use it to parse a conninfo
string in pg_basebackup, correct? Perhaps all we need to do is to clearly
comment that it doesn't work with non-ascii safe encodings, or rename it to
indicate that it's limited in this?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]

[Boszormenyi Zoltan]

2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan  writes:

2013-01-01 17:18 keltezéssel, Magnus Hagander írta:

That way we can get around the whole need for changing memory allocation across 
all the
frontends, no? Like the attached.

Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
  fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.

Meh.  I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.


Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.


I like Magnus' version a lot better than that idea.


OK, I will post the core patch building on his code.


A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions.  If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane


The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?

Best regards,
Zoltán Böszörményi

--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
 http://www.postgresql.at/



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