date:20140919

> "Adam" == Brightwell, Adam  
> writes:

 Adam> At any rate, this appears to be a previously existing issue
 Adam> with WITH CHECK OPTION.  Thoughts?

It's definitely an existing issue; you can reproduce it more simply,
no need to mess with different users.

The issue as far as I can tell is that the withCheckOption exprs are
not being processed anywhere in setrefs.c, so it only works at all by
pure fluke: for most operators, the opfuncid is also filled in by
eval_const_expressions, but for whatever reason SAOPs escape this
treatment. Same goes for other similar cases:

create table colors (name text);
create view vc1 as select * from colors where name is distinct from 'grue' with 
check option;
create view vc2 as select * from colors where name in ('red','green','blue') 
with check option;
create view vc3 as select * from colors where nullif(name,'grue') is null with 
check option;

insert into vc1 values ('red'); -- fails
insert into vc2 values ('red'); -- fails
insert into vc3 values ('red'); -- fails

(Also, commands/policy.c has two instances of "const char" as a
function return type, which is a compiler warning since the "const" is
meaningless.)

-- 
Andrew (irc:RhodiumToad)

-- 
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] Support for N synchronous standby servers

On Fri, Sep 19, 2014 at 12:18 PM, Robert Haas  wrote:
> On Tue, Sep 16, 2014 at 2:19 PM, Michael Paquier
>  wrote:
>> On Tue, Sep 16, 2014 at 5:25 AM, Robert Haas  wrote:
>>> On Mon, Sep 15, 2014 at 3:00 PM, Michael Paquier
>>>  wrote:
 At least a set of hooks has the merit to say: do what you like with
 your synchronous node policy.
>>>
>>> Sure.  I dunno if people will find that terribly user-friendly, so we
>>> might not want that to be the ONLY thing we offer.
>> Well, user-friendly interface is actually the reason why a simple GUC
>> integer was used in the first series of patches present on this thread
>> to set as sync the N-nodes with the lowest priority. I could not come
>> up with something more simple. Hence what about doing the following:
>> - A patch refactoring code for pg_stat_get_wal_senders and
>> SyncRepReleaseWaiters as there is in either case duplicated code in
>> this area to select the synchronous node as the one connected with
>> lowest priority
>
> A strong +1 for this idea.  I have never liked that, and cleaning it
> up seems eminently sensible.

Interestingly, the syncrep code has in some of its code paths the idea
that a synchronous node is unique, while other code paths assume that
there can be multiple synchronous nodes. If that's fine I think that
it would be better to just make the code multiple-sync node aware, by
having a single function call in walsender.c and syncrep.c that
returns an integer array of WAL sender positions (WalSndCtl). as that
seems more extensible long-term. Well for now the array would have a
single element, being the WAL sender with lowest priority > 0. Feel
free to protest about that approach though :)

>> - A patch defining the hooks necessary, I suspect that two of them are
>> necessary as mentioned upthread.
>> - A patch for a contrib module implementing an example of simple
>> policy. It can be a fancy thing with a custom language or even a more
>> simple thing.
>
> I'm less convinced about this part.  There's a big usability gap
> between a GUC and a hook, and I think Heikki's comments upthread were
> meant to suggest that even in GUC-land we can probably satisfy more
> use cases that what this patch does now.  I think that's right.
Hehe. OK then let's see how something with a GUC would go then. There
is no parameter now using a custom language as format base, but I
guess that it is fine to have a text parameter with a validation
callback within. No?
-- 
Michael


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

Re: [HACKERS] pg_shmem_allocations view

On Mon, Aug 18, 2014 at 1:12 PM, Robert Haas  wrote:
> On Mon, Aug 18, 2014 at 1:27 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> I thought you were printing actual pointer addresses.  If you're just
>>> printing offsets relative to wherever the segment happens to be
>>> mapped, I don't care about that.
>>
>> Well, that just means that it's not an *obvious* security risk.
>>
>> I still like the idea of providing something comparable to
>> MemoryContextStats, rather than creating a SQL interface.  The problem
>> with a SQL interface is you can't interrogate it unless (1) you are not
>> already inside a query and (2) the client is interactive and under your
>> control.  Something you can call easily from gdb is likely to be much
>> more useful in practice.
>
> Since the shared memory segment isn't changing at runtime, I don't see
> this as being a big problem.  It could possibly be an issue for
> dynamic shared memory segments, though.
Patch has been reviewed some time ago, extra ideas as well as
potential security risks discussed as well but no new version has been
sent, hence marking it as returned with feedback.
-- 
Michael


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

Re: [HACKERS] Add CREATE support to event triggers

On Tue, Aug 26, 2014 at 11:14 PM, Michael Paquier
 wrote:
> And a last one before lunch, closing the review for all the basic things...
> Patch 13: Could you explain why this is necessary?
> +extern PGDLLIMPORT bool creating_extension;
> It may make sense by looking at the core features (then why isn't it
> with the core features?), but I am trying to review the patches in
> order.
Those patches have been reviewed up to number 14. Some of them could
be applied as-is as they are useful taken independently, but most of
them need a rebase, hence marking it as returned with feedback.
-- 
Michael


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

Re: [HACKERS] Commitfest status

CF3 is actually over for a couple of days, wouldn't it be better to
bounce back patches marked as "waiting on author" and work on the rest
needing review?
-- 
Michael


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

Re: [HACKERS] pg_xlogdump --stats

On 2014-09-19 19:34:08 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > Hi,
> > 
> > On 2014-09-19 15:39:37 +0530, Abhijit Menon-Sen wrote:
> > > I've attached the revised patch, split up differently:
> > > 
> > > 1. Introduce rm_identify, change rm_desc, glue the two together in
> > >xlog.c
> > > 2. Introduce pg_xlogdump --stats[=record].
> > 
> > I've pushed these after some fixing up.
> 
> Hm, did you keep the static thingy you mentioned upthread (append_init
> which is duped unless I read wrong)?

Yes, I did.

>  There's quite some angst due to pg_dump's fmtId() -- I don't think we
> should be introducing more such things ...

I don't think these two are comparable. I don't really see many more
users of this springing up and printing the identity of two different
records in the same printf doesn't seem likely either.

Greetings,

Andres Freund

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


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

Re: [HACKERS] pg_xlogdump --stats

2014-09-19 Thread Alvaro Herrera

Andres Freund wrote:
> Hi,
> 
> On 2014-09-19 15:39:37 +0530, Abhijit Menon-Sen wrote:
> > I've attached the revised patch, split up differently:
> > 
> > 1. Introduce rm_identify, change rm_desc, glue the two together in
> >xlog.c
> > 2. Introduce pg_xlogdump --stats[=record].
> 
> I've pushed these after some fixing up.

Hm, did you keep the static thingy you mentioned upthread (append_init
which is duped unless I read wrong)?  There's quite some angst due to
pg_dump's fmtId() -- I don't think we should be introducing more such
things ...

-- 
Á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] RLS Design

2014-09-19 Thread Brightwell, Adam

Thom,

Also, I seem to get an error message with the following:
>
> # create policy nice_colours ON colours for all to joe using (visible =
> true) with check (name in ('blue','green','yellow'));
> CREATE POLICY
>
> \c - joe
>
> > insert into colours (name, visible) values ('blue',false);
> ERROR:  function with OID 0 does not exist
>
> And if this did work, but I only violated the USING clause, would this
> still say the WITH CHECK clause was the cause?
>

Since RLS is built on top of  the same mechanisms used for Security Barrier
Views, I figured I would check this case against that and, for the heck of
it, regular VIEWs as well.  The result is the same error in both cases
(below and attached).  I also verified that this issue exists for 9.4beta2
and the current REL9_4_STABLE branch.  If this isn't the expected behavior
(I can't imagine that it is), I am certainly willing to dive into it
further and see what I can determine for a solution/recommendation.  At any
rate, this appears to be a previously existing issue with WITH CHECK
OPTION.  Thoughts?

postgres=# DROP TABLE IF EXISTS colors CASCADE;
NOTICE:  table "colors" does not exist, skipping
DROP TABLE
postgres=# DROP ROLE IF EXISTS joe;
DROP ROLE
postgres=# CREATE ROLE joe LOGIN;
CREATE ROLE
postgres=# CREATE TABLE colors (name text, visible bool);
CREATE TABLE
postgres=# CREATE OR REPLACE VIEW v_colors_1 WITH (security_barrier) AS
postgres-# SELECT * FROM colors WHERE (name in ('blue', 'green',
'yellow'))
postgres-# WITH CHECK OPTION;
CREATE VIEW
postgres=# CREATE OR REPLACE VIEW v_colors_2 AS
postgres-# SELECT * FROM colors WHERE (name in ('blue', 'green',
'yellow'))
postgres-# WITH CHECK OPTION;
CREATE VIEW
postgres=# GRANT ALL ON v_colors_1, v_colors_2 TO joe;
GRANT
postgres=# \c - joe
You are now connected to database "postgres" as user "joe".
postgres=> INSERT INTO v_colors_1 (name, visible) VALUES ('blue', false);
ERROR:  function with OID 0 does not exist
postgres=> INSERT INTO v_colors_2 (name, visible) VALUES ('blue', false);
ERROR:  function with OID 0 does not exist

Thanks,
Adam

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


with_check_error.sql
Description: application/sql

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

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

2014-09-19 Thread Peter Geoghegan

On Fri, Sep 19, 2014 at 2:35 PM, Robert Haas  wrote:
> Also, shouldn't you go back and fix up
> those abbreviated keys to point to datum1 again if you abort?

Probably not - it appears to make very little difference to
unoptimized pass-by-reference types whether or not datum1 can be used
(see my simulation of Kevin's worst case, for example [1]). Streaming
through a not inconsiderable proportion of memtuples again is probably
a lot worse. The datum1 optimization (which is not all that old) made
a lot of sense when initially introduced, because it avoided chasing
through a pointer for pass-by-value types. I think that's its sole
justification, though.

BTW, I think that if we ever get around to doing this for numeric, it
won't ever abort. The abbreviation strategy can be adaptive, to
maximize the number of comparisons successfully resolved with
abbreviated keys. This would probably use a streaming algorithm like
HyperLogLog too.

[1] 
http://www.postgresql.org/message-id/cam3swzqhjxiyrsqbs5w3u-vtj_jt2hp8o02big5wyb4m9lp...@mail.gmail.com
-- 
Peter Geoghegan

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

Re: [HACKERS] Turning off HOT/Cleanup sometimes

On 2014-09-19 16:35:19 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On September 19, 2014 10:16:35 PM CEST, Simon Riggs  
> > wrote:
> >> Are you saying this is a problem or a benefit? (and please explain
> >> why).
> 
> > I have no idea what Robert is thinking of, but I'd imagine its horrible for 
> > workloads with catalog bloat. Like ones involving temp tables.
> 
> Yeah.  But it's also the case that we know a good deal more about the
> access patterns for system-driven catalog updates than we do about user
> queries.  ISTM we could probably suppress HOT pruning during catalog
> *scans* and instead try to do it when a system-driven heap_update
> occurs.
> 
> Having said that, this could reasonably be considered outside the scope
> of a patch that's trying to improve the behavior for user queries.
> But if the patch author doesn't want to expand the scope like that,
> ISTM he ought to ensure that the behavior *doesn't* change for system
> accesses, rather than trying to convince us that disabling HOT for
> system updates is a good idea.

I think it'd have to change for anything not done via the
executor. There definitely is user defined code out there doing manual
heap_* stuff. I know because i've written some. And I know I'm not the
only one.

If such paths suddenly stop doing HOT cleanup we'll cause a noticeable
amount of pain.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Turning off HOT/Cleanup sometimes

2014-09-19 Thread Simon Riggs

On 19 September 2014 15:35, Tom Lane  wrote:

> Having said that, this could reasonably be considered outside the scope
> of a patch that's trying to improve the behavior for user queries.
> But if the patch author doesn't want to expand the scope like that,
> ISTM he ought to ensure that the behavior *doesn't* change for system
> accesses, rather than trying to convince us that disabling HOT for
> system updates is a good idea.

As I said, I could make an argument to go either way, so I was unsure.

I'm happy to avoid changing behaviour for catalog scans in this patch.

-- 
 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] Turning off HOT/Cleanup sometimes

On 2014-09-19 17:29:08 -0400, Robert Haas wrote:
> > I generally have serious doubts about disabling it generally for
> > read workloads. I imagine it e.g. will significantly penalize
> > workloads where its likely that a cleanup lock can't be acquired
> > every time...
> 
> I share that doubt.  But I understand why Simon wants to do something,
> too, because the current situation is not great either.

Right, I totally agree. I doubt a simple approach like this will work in
the general case, but I think something needs to be done.

I think limiting the amount of HOT cleanup for readonly queries is a
good idea, but I think it has to be gradual. Say after a single cleaned
up page at least another 500 pages need to have been touched till the
next hot cleanup. That way a single query won't be penalized with
cleaning up everything, but there'll be some progress.

The other thing I think might be quite worthwile would be to abort hot
cleanup when the gain is only minimal. If e.g. only 1 small tuple is
removed from a half full page it's not worth the cost of the wal logging
et al.

Greetings,

Andres Freund

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

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

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

On Thu, Sep 11, 2014 at 8:34 PM, Peter Geoghegan  wrote:
> On Tue, Sep 9, 2014 at 2:25 PM, Robert Haas  wrote:
>>> I like that I don't have to care about every combination, and can
>>> treat abbreviation abortion as the special case with the extra step,
>>> in line with how I think of the optimization conceptually. Does that
>>> make sense?
>>
>> No.  comparetup_heap() is hip-deep in this optimization as it stands,
>> and what I proposed - if done correctly - isn't going to make that
>> significantly worse.  In fact, it really ought to make things better:
>> you should be able to set things up so that ssup->comparator is always
>> the test that should be applied first, regardless of whether we're
>> aborted or not-aborted or not doing this in the first place; and then
>> ssup->tiebreak_comparator, if not NULL, can be applied after that.
>
> I'm not following here. Isn't that at least equally true of what I've
> proposed? Sure, I'm checking "if (!state->abbrevAborted)" first, but
> that's irrelevant to the non-abbreviated case. It has nothing to
> abort. Also, AFAICT we cannot abort and still call ssup->comparator()
> indifferently, since sorttuple.datum1 fields are perhaps abbreviated
> keys in half of all cases (i.e. pre-abort tuples), and uninitialized
> garbage the other half of the time (i.e. post-abort tuples).

You can if you engineer ssup->comparator() to contain the right
pointer at the right time.  Also, shouldn't you go back and fix up
those abbreviated keys to point to datum1 again if you abort?

> Where is the heap_getattr() stuff supposed to happen for the first
> attribute to get an authoritative comparison in the event of aborting
> (if we're calling ssup->comparator() on datum1 indifferently)? We
> decide that we're going to use abbreviated keys within datum1 fields
> up-front. When we abort, we cannot use datum1 fields at all (which
> doesn't appear to matter for text -- the datum1 optimization has
> historically only benefited pass-by-value types).

You always pass datum1 to a function.  The code doesn't need to care
about whether that function is expecting abbreviated or
non-abbreviated datums unless that function returns equality.  Then it
needs to think about calling a backup comparator if there is one.

> Is doing all this worth the small saving in memory? Personally, I
> don't think that it is, but I defer to you.

I don't care about the memory; I care about the code complexity and
the ease of understanding that code.  I am confident that it can be
done better than the patch does it today.

-- 
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] Turning off HOT/Cleanup sometimes

On Fri, Sep 19, 2014 at 4:30 PM, Andres Freund  wrote:
> On September 19, 2014 10:16:35 PM CEST, Simon Riggs  
> wrote:
>>On 19 September 2014 13:04, Robert Haas  wrote:
>>
>>> What I'm thinking about is that the smarts to enable pruning is all
>>in
>>> the executor nodes.  So anything that updates the catalog without
>>> going through the executor will never be subject to pruning.  That
>>> includes nearly all catalog-modifying code throughout the backend.
>>
>>Are you saying this is a problem or a benefit? (and please explain
>>why).
>
> I have no idea what Robert is thinking of, but I'd imagine its horrible for 
> workloads with catalog bloat. Like ones involving temp tables.

Right, that's what I was going for.

> I generally have serious doubts about disabling it generally for read 
> workloads. I imagine it e.g. will significantly penalize workloads where its 
> likely that a cleanup lock can't be acquired every time...

I share that doubt.  But I understand why Simon wants to do something,
too, because the current situation is not great either.

-- 
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] Turning off HOT/Cleanup sometimes

Andres Freund  writes:
> On September 19, 2014 10:16:35 PM CEST, Simon Riggs  
> wrote:
>> Are you saying this is a problem or a benefit? (and please explain
>> why).

> I have no idea what Robert is thinking of, but I'd imagine its horrible for 
> workloads with catalog bloat. Like ones involving temp tables.

Yeah.  But it's also the case that we know a good deal more about the
access patterns for system-driven catalog updates than we do about user
queries.  ISTM we could probably suppress HOT pruning during catalog
*scans* and instead try to do it when a system-driven heap_update
occurs.

Having said that, this could reasonably be considered outside the scope
of a patch that's trying to improve the behavior for user queries.
But if the patch author doesn't want to expand the scope like that,
ISTM he ought to ensure that the behavior *doesn't* change for system
accesses, rather than trying to convince us that disabling HOT for
system updates is a good idea.

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] Turning off HOT/Cleanup sometimes

On September 19, 2014 10:16:35 PM CEST, Simon Riggs  
wrote:
>On 19 September 2014 13:04, Robert Haas  wrote:
>
>> What I'm thinking about is that the smarts to enable pruning is all
>in
>> the executor nodes.  So anything that updates the catalog without
>> going through the executor will never be subject to pruning.  That
>> includes nearly all catalog-modifying code throughout the backend.
>
>Are you saying this is a problem or a benefit? (and please explain
>why).

I have no idea what Robert is thinking of, but I'd imagine its horrible for 
workloads with catalog bloat. Like ones involving temp tables.

I generally have serious doubts about disabling it generally for read 
workloads. I imagine it e.g. will significantly penalize workloads where its 
likely that a cleanup lock can't be acquired every time...

Andres

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

-- 
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] Turning off HOT/Cleanup sometimes

2014-09-19 Thread Simon Riggs

On 19 September 2014 13:04, Robert Haas  wrote:

> What I'm thinking about is that the smarts to enable pruning is all in
> the executor nodes.  So anything that updates the catalog without
> going through the executor will never be subject to pruning.  That
> includes nearly all catalog-modifying code throughout the backend.

Are you saying this is a problem or a benefit? (and please explain why).

-- 
 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] Anonymous code block with parameters

2014-09-19 Thread Marko Tiikkaja


On 2014-09-19 8:20 PM, Merlin Moncure wrote:

actually, this trick *only* works during json serialization -- it
allows control over the column names that row() masks over.  trying to
expand (tup).* still gives the dreaded "ERROR:  record type has not
been registered".  That's because this works:

select (q).* from (select 1 as a, 2 as b) q;

but this doesn't:

select ((select q from (select a,b) q)).* from (select 1 as a, 2 as b) q;


Yeah.  This is a seriously missing feature and a PITA. :-(


.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] Anonymous code block with parameters

2014-09-19 Thread Merlin Moncure

On Fri, Sep 19, 2014 at 9:26 AM, Merlin Moncure  wrote:
> On Thu, Sep 18, 2014 at 5:22 PM, Hannu Krosing  wrote:
>> Though it would be even nicer to have fully in-line type definition
>>
>> SELECT (tup).* FROM
>>   (
>> SELECT CASE WHEN .. THEN ROW(1,2,3)::(a int, b text, c int2)
>> WHEN .. THEN ROW(2,3,4)
>> ELSE ROW (3,4,5) END AS tup
>> FROM ..
>>   ) ss
>
> +1.   Workaround at present (which I mostly use during json serialization) is:
>
> SELECT (tup).* FROM
>   (
> SELECT CASE WHEN .. THEN
>(SELECT q FROM (SELECT 1, 2, 3) q)
> WHEN .. THEN
>(SELECT q FROM (SELECT 2, 3, 4) q)
> ELSE (SELECT q FROM (SELECT 3, 4, 5) q)
> END AS tup
> FROM ..
>   ) ss

actually, this trick *only* works during json serialization -- it
allows control over the column names that row() masks over.  trying to
expand (tup).* still gives the dreaded "ERROR:  record type has not
been registered".  That's because this works:

select (q).* from (select 1 as a, 2 as b) q;

but this doesn't:

select ((select q from (select a,b) q)).* from (select 1 as a, 2 as b) q;

merlin


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

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

2014-09-19 Thread Peter Geoghegan

On Fri, Sep 19, 2014 at 9:59 AM, Robert Haas  wrote:
> OK, good point.  So committed as-is, then, except that I rewrote the
> comments, which I felt were excessively long for the amount of code.

Thanks!

I look forward to hearing your thoughts on the open issues with the
patch as a whole.
-- 
Peter Geoghegan


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

Re: [HACKERS] removing volatile qualifiers from lwlock.c

On 2014-09-19 13:58:17 -0400, Robert Haas wrote:
> On Wed, Sep 17, 2014 at 7:45 AM, Andres Freund  wrote:
> > I just tried this on my normal x86 workstation. I applied your lwlock
> > patch and ontop I removed most volatiles (there's a couple still
> > required) from xlog.c. Works for 100 seconds. Then I reverted the above
> > commits. Breaks within seconds:
> > master:
> > LOG:  request to flush past end of generated WAL; request 2/E5EC3DE0, 
> > currpos 2/E5EC1E60
> > standby:
> > LOG:  record with incorrect prev-link 4/684C3108 at 4/684C3108
> > and similar.
> >
> > So at least for x86 the compiler barriers are obviously required and
> > seemingly working.
> 
> Oh, that's great.  In that case I think I should go ahead and apply
> that patch in the hopes of turning up any systems where the barriers
> aren't working properly yet.

Agreed.

> Although it would be nice to know whether it breaks with *only* the lwlock.c 
> patch.

It didn't, at least not visibly within the 1000s I let pgbench run.

> > I've attached the very quickly written xlog.c de-volatizing patch.
> 
> I don't have time to go through this in detail, but I don't object to
> you applying it if you're confident you've done it carefully enough.

It's definitely not yet carefully enough checked. I wanted to get it
break fast and only made one pass through the file. But I think it
should be easy enough to get it into shape for that.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Turning off HOT/Cleanup sometimes

On Mon, Sep 15, 2014 at 5:13 PM, Simon Riggs  wrote:
> On 15 September 2014 17:09, Robert Haas  wrote:
>> Do we really want to disable HOT for all catalog scans?
>
> The intention of the patch is that catalog scans are treated
> identically to non-catalog scans. The idea here is that HOT cleanup
> only occurs on scans on target relations, so only INSERT, UPDATE and
> DELETE do HOT cleanup.
>
> It's possible that many catalog scans don't follow the normal target
> relation logic, so we might argue we should use HOT every time. OTOH,
> since we now have separate catalog xmins we may find that using HOT on
> catalogs is no longer effective. So I could go either way on how to
> proceed; its an easy change either way.

What I'm thinking about is that the smarts to enable pruning is all in
the executor nodes.  So anything that updates the catalog without
going through the executor will never be subject to pruning.  That
includes nearly all catalog-modifying code throughout the backend.

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

> "Josh" == Josh Berkus  writes:

 Josh> (b) If we're going to discuss ripping out YAML format, please
 Josh> let's do that as a *separate* patch and discussion,

+infinity

 >> Grouping Sets:
 >>   - ["two","four"]
 >>   - ["two"]
 >>   - []
 >> 
 >> Would that be better? (It's not consistent with other YAML outputs
 >> like sort/group keys, but it's equally legal as far as I can tell
 >> and seems more readable.)

 Josh> That works for me.

I prefer that one to any of the others I've come up with, so unless anyone
has a major objection, I'll go with it.

-- 
Andrew (irc:RhodiumToad)


-- 
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] removing volatile qualifiers from lwlock.c

On Wed, Sep 17, 2014 at 7:45 AM, Andres Freund  wrote:
> I just tried this on my normal x86 workstation. I applied your lwlock
> patch and ontop I removed most volatiles (there's a couple still
> required) from xlog.c. Works for 100 seconds. Then I reverted the above
> commits. Breaks within seconds:
> master:
> LOG:  request to flush past end of generated WAL; request 2/E5EC3DE0, currpos 
> 2/E5EC1E60
> standby:
> LOG:  record with incorrect prev-link 4/684C3108 at 4/684C3108
> and similar.
>
> So at least for x86 the compiler barriers are obviously required and
> seemingly working.

Oh, that's great.  In that case I think I should go ahead and apply
that patch in the hopes of turning up any systems where the barriers
aren't working properly yet.  Although it would be nice to know
whether it breaks with *only* the lwlock.c patch.

> I've attached the very quickly written xlog.c de-volatizing patch.

I don't have time to go through this in detail, but I don't object to
you applying it if you're confident you've done it carefully enough.

-- 
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] CreateEventTrigStmt copy fix

On Fri, Sep 19, 2014 at 11:09 AM, Petr Jelinek  wrote:
> Hi hackers,
>
> I was trying to create event trigger inside DO statement inside an extension
> SQL script and noticed that the new event trigger has empty evtevent field.
> After some tinkering with gdb I found out that the memory context switches
> sometimes clear the eventname in CreateEventTrigStmt struct. The reason for
> this is that _copyCreateEventTrigStmt uses COPY_SCALAR_FIELD on eventname
> instead of COPY_STRING_FIELD.
>
> Attached patch fixes this and also the same issue in
> _equalCreateEventTrigStmt.
>
> This should be back-patched to 9.3 where event triggers were introduced.
Nice catch! And no need to care much about outfuncs.c for this Node type.
Regards,
-- 
Michael


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

Re: [HACKERS] Final Patch for GROUPING SETS

2014-09-19 Thread Josh Berkus

On 09/19/2014 08:52 AM, Andres Freund wrote:
>> Until someone decides to dike it out, I think we are obligated to make
>> > it produce something resembling correct output.
> I vote for ripping it out. There really isn't any justification for it
> and it broke more than once.

(a) I personally use it all the time to produce human-readable output,
sometimes also working via markdown.  It's easier to read than the
"standard format" or JSON, especially when combined with grep or other
selective filtering.  Note that this use would not at all preclude
having the YAML output look "wierd" as long as it was readable.

(b) If we're going to discuss ripping out YAML format, please let's do
that as a *separate* patch and discussion, and not as a side effect of
Grouping Sets.  Otherwise this will be one of those things where people
pitch a fit during beta because the people who care about YAML aren't
necessarily reading this thread.

On 09/19/2014 08:52 AM, Andrew Gierth wrote:> Oh, another YAML
alternative would be:
>
> Grouping Sets:
>   - ["two","four"]
>   - ["two"]
>   - []
>
> Would that be better? (It's not consistent with other YAML outputs like
> sort/group keys, but it's equally legal as far as I can tell and seems
> more readable.)

That works for me.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] Support for N synchronous standby servers

On Tue, Sep 16, 2014 at 2:19 PM, Michael Paquier
 wrote:
> On Tue, Sep 16, 2014 at 5:25 AM, Robert Haas  wrote:
>> On Mon, Sep 15, 2014 at 3:00 PM, Michael Paquier
>>  wrote:
>>> At least a set of hooks has the merit to say: do what you like with
>>> your synchronous node policy.
>>
>> Sure.  I dunno if people will find that terribly user-friendly, so we
>> might not want that to be the ONLY thing we offer.
> Well, user-friendly interface is actually the reason why a simple GUC
> integer was used in the first series of patches present on this thread
> to set as sync the N-nodes with the lowest priority. I could not come
> up with something more simple. Hence what about doing the following:
> - A patch refactoring code for pg_stat_get_wal_senders and
> SyncRepReleaseWaiters as there is in either case duplicated code in
> this area to select the synchronous node as the one connected with
> lowest priority

A strong +1 for this idea.  I have never liked that, and cleaning it
up seems eminently sensible.

> - A patch defining the hooks necessary, I suspect that two of them are
> necessary as mentioned upthread.
> - A patch for a contrib module implementing an example of simple
> policy. It can be a fancy thing with a custom language or even a more
> simple thing.

I'm less convinced about this part.  There's a big usability gap
between a GUC and a hook, and I think Heikki's comments upthread were
meant to suggest that even in GUC-land we can probably satisfy more
use cases that what this patch does now.  I think that's right.

-- 
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] Minor improvement in lock.sgml

On Tue, Sep 16, 2014 at 7:20 AM, Etsuro Fujita
 wrote:
> Here is a patch to a bit improve the reference page for the LOCK
> command.  I think it'd be better for the isolation level to be in
> capitals and wrapped in the  tags.

It's done that way elsewhere in the same page, so committed.

Overall, there is some ambiguity about this.  Most places follow your
proposed style if READ COMMITTED,
REPEATABLE READ, and
SERIALIZABLE, but mvcc.sgml, which discusses
isolation levels extensively, just writes them as Read Committed,
Repeatable Read, and Serializable.

I'm not sure whether more consistency overall would be a good thing -
there are some things to recommend the current mix of styles - but
mixing styles within a page doesn't seem smart, so this much at least
seems uncontroversial.

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

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

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

On Tue, Sep 16, 2014 at 4:55 PM, Peter Geoghegan  wrote:
> On Tue, Sep 16, 2014 at 1:45 PM, Robert Haas  wrote:
>> Even though our testing seems to indicate that the memcmp() is
>> basically free, I think it would be good to make the effort to avoid
>> doing memcmp() and then strcoll() and then strncmp().  Seems like it
>> shouldn't be too hard.
>
> Really? The tie-breaker for the benefit of locales like hu_HU uses
> strcmp(), not memcmp(). It operates on the now-terminated copies of
> strings. There is no reason to think that the strings must be the same
> size for that strcmp(). I'd rather only do the new opportunistic
> "memcmp() == 0" thing when len1 == len2. And I wouldn't like to have
> to also figure out that it's safe to use the earlier result, because
> as it happens len1 == len2, or any other such trickery.

OK, good point.  So committed as-is, then, except that I rewrote the
comments, which I felt were excessively long for the amount of code.

-- 
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] RLS Design

2014-09-19 Thread Stephen Frost

Thom,

* Thom Brown (t...@linux.com) wrote:
> On 19 September 2014 17:32, Stephen Frost  wrote:
> > * Thom Brown (t...@linux.com) wrote:
> > > On 14 September 2014 16:38, Stephen Frost  wrote:
> > > # create policy visible_colours on colours for all to joe using (visible
> > =
> > > true);
> > > CREATE POLICY
> > [...]
> > > > insert into colours (name, visible) values ('transparent',false);
> > > ERROR:  new row violates WITH CHECK OPTION for "colours"
> > > DETAIL:  Failing row contains (7, transparent, f).
> > >
> > > > select * from pg_policies ;
> > >policyname| tablename | roles | cmd |   qual   |
> > with_check
> > >
> > -+---+---+-+--+
> > >  visible_colours | colours   | {joe} | ALL | (visible = true) |
> > > (1 row)
> > >
> > > There was no WITH CHECK OPTION.
> >
> > As I hope is clear if you look at the documentation- if the WITH CHECK
> > clause is omitted, then the USING clause is used for both filtering and
> > checking new records, otherwise you'd be able to add records which
> > aren't visible to you.
> 
> I can see that now, although I do find the error message somewhat
> confusing.  Firstly, it looks like "OPTION" is part of the parameter name,
> which it isn't.

Hmm, the notion of 'with check option' is from the SQL standard, which
is why I felt the error message was appropriate as-is..

> Also, I seem to get an error message with the following:
> 
> # create policy nice_colours ON colours for all to joe using (visible =
> true) with check (name in ('blue','green','yellow'));
> CREATE POLICY
> 
> \c - joe
> 
> > insert into colours (name, visible) values ('blue',false);
> ERROR:  function with OID 0 does not exist

Now *that* one is interesting and I'll definitely go take a look at it.
We added quite a few regression tests to try and make sure these things
work.

> And if this did work, but I only violated the USING clause, would this
> still say the WITH CHECK clause was the cause?

WITH CHECK applies for INSERT and UPDATE for the new records going into
the table.  You can't actually violate the USING clause for an INSERT
as USING is for filtering records, not checking that records being added
to the table are valid.

To try and clarify- by explicitly setting both USING and WITH CHECK, you
*are* able to INSERT records which are not visible to you.  We felt that
was an important capability to support.

Thanks for taking a look at it!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] RLS Design

On Fri, Sep 19, 2014 at 12:38 PM, Stephen Frost  wrote:
>> This patch, on the other hand, was massively revised after the start
>> of the CommitFest after many months of inactivity and committed with
>> no thorough review by anyone who was truly independent of the
>> development effort.  It was then committed with no warning over a
>> specific request, from another committer, that more time be allowed
>> for review.
>
> I would not (nor do I feel that I did..) have committed it over a
> specific request to not do so from another committer.

Well, you're wrong.  How could this email possibly have been any more clear?

http://www.postgresql.org/message-id/CA+TgmoYA=uixxmn390sfgfqgvmll-as5bjal0om7yrppvwn...@mail.gmail.com

You can hardly tell me you didn't see that email when you incorporated
the technical content into the next patch version.

> While I wasn't public about it, I actually specifically discussed this
> question with others, a few times even, to try and make sure that I
> wasn't stepping out of line by moving forward.

And yet you completely ignored the only public commentary on the
issue, which was from me.

I *should not have had* to object to this patch going in.  It was
clearly untimely for the August CommitFest, and as a long-time
community member, you ought to know full well that any such patch
should be resubmitted to a later CommitFest.  This patch sat on the
shelf for 4 months because you were too busy to work on it, and was
committed 5 days from the last posted version, which version had zero
review comments.  If you didn't have time to work on it for 4 months,
you can hardly expect everyone else who has an opinion to comment
within 5 days.

But, you know, because I could tell that you were fixated on pushing
this patch through to commit quickly, I took the time to send you a
message on that specific point, even though you should have known full
well.  In fact I took the time to send TWO.  Here's the other one:

http://www.postgresql.org/message-id/ca+tgmobqo0z87eivfdewjcac1dc4ahh5wcvoqoxrsateu1t...@mail.gmail.com

> All-in-all, I feel appropriately chastised and certainly don't wish to
> be surprising fellow committers.  Perhaps we can discuss at the dev
> meeting.

No, I think we should discuss it right now, not nine months from now
when the issue has faded from everyone's mind.

-- 
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] RLS Design

On 2014-09-19 12:38:39 -0400, Stephen Frost wrote:
> I would not (nor do I feel that I did..) have committed it over a
> specific request to not do so from another committer.  I had been hoping
> that there would be another review coming from somewhere, but there is
> always a trade-off between waiting longer to get a review ahead of a
> commit and having it committed and then available more easily for others
> to work with, review, and generally moving forward.

Sure, there is such a tradeoff. But others have to wait months to get
enough review.  The first revision of the patch in the form you
committed was sent 2014-08-19, the first marked *ready for review* (not
my words) is from 2014-08-30. 19 days really isn't very far along the
tradeoff from waiting for a review to uselessly waiting.

Greetings,

Andres Freund

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

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

Re: [HACKERS] RLS Design

2014-09-19 Thread Thom Brown

On 19 September 2014 17:32, Stephen Frost  wrote:

> Thom,
>
> Thanks!
>
> * Thom Brown (t...@linux.com) wrote:
> > On 14 September 2014 16:38, Stephen Frost  wrote:
> > # create policy visible_colours on colours for all to joe using (visible
> =
> > true);
> > CREATE POLICY
> [...]
> > > insert into colours (name, visible) values ('transparent',false);
> > ERROR:  new row violates WITH CHECK OPTION for "colours"
> > DETAIL:  Failing row contains (7, transparent, f).
> >
> > > select * from pg_policies ;
> >policyname| tablename | roles | cmd |   qual   |
> with_check
> >
> -+---+---+-+--+
> >  visible_colours | colours   | {joe} | ALL | (visible = true) |
> > (1 row)
> >
> > There was no WITH CHECK OPTION.
>
> As I hope is clear if you look at the documentation- if the WITH CHECK
> clause is omitted, then the USING clause is used for both filtering and
> checking new records, otherwise you'd be able to add records which
> aren't visible to you.


I can see that now, although I do find the error message somewhat
confusing.  Firstly, it looks like "OPTION" is part of the parameter name,
which it isn't.

Also, I seem to get an error message with the following:

# create policy nice_colours ON colours for all to joe using (visible =
true) with check (name in ('blue','green','yellow'));
CREATE POLICY

\c - joe

> insert into colours (name, visible) values ('blue',false);
ERROR:  function with OID 0 does not exist

And if this did work, but I only violated the USING clause, would this
still say the WITH CHECK clause was the cause?

Thom

Re: [HACKERS] RLS Design

2014-09-19 Thread Stephen Frost

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sun, Sep 14, 2014 at 11:38 AM, Stephen Frost  wrote:
> > Alright, updated patch attached which does just that (thanks to Adam
> > for the updates for this and testing pg_dump- I just reviewed it and
> > added some documentation updates and other minor improvements), and
> > rebased to master.  Also removed the catversion bump, so it should apply
> > cleanly for people, for a while anyway.
> 
> I specifically asked you to hold off on committing this until there
> was adequate opportunity for review, and explained my reasoning.  You
> committed it anyway.

Hum- my apologies, I honestly don't recall you specifically asking for
it to be held off indefinitely.  :(  There was discussion back and
forth, quite a bit of it with you, and I thank you for your help with
that and certainly welcome any additional comments.

> This patch, on the other hand, was massively revised after the start
> of the CommitFest after many months of inactivity and committed with
> no thorough review by anyone who was truly independent of the
> development effort.  It was then committed with no warning over a
> specific request, from another committer, that more time be allowed
> for review.

I would not (nor do I feel that I did..) have committed it over a
specific request to not do so from another committer.  I had been hoping
that there would be another review coming from somewhere, but there is
always a trade-off between waiting longer to get a review ahead of a
commit and having it committed and then available more easily for others
to work with, review, and generally moving forward.

> I'm really disappointed by that.  I feel I'm essentially getting
> punished for trying to follow what I understand to the process, which
> has involved me doing huge amounts of review of other people's patches
> and waiting a very long time to get my own stuff committed, while you
> bull ahead with your own patches.

While I wasn't public about it, I actually specifically discussed this
question with others, a few times even, to try and make sure that I
wasn't stepping out of line by moving forward.

That said, I do see that Andres feels similairly.  It certainly wasn't
my intent to surprise anyone by it but simply to continue to move
forward- in part, to allow me to properly break from it and work on
other things, including reviewing other patches in the commitfest.
I fear I've simply been overly focused on it these past few weeks, for a
variety of reasons that would likely best be discussed at the pub.

All-in-all, I feel appropriately chastised and certainly don't wish to
be surprising fellow committers.  Perhaps we can discuss at the dev
meeting.

Thanks,

Stephen

signature.asc
Description: Digital signature

Re: [HACKERS] RLS Design

2014-09-19 Thread Stephen Frost

Thom,

Thanks!

* Thom Brown (t...@linux.com) wrote:
> On 14 September 2014 16:38, Stephen Frost  wrote:
> # create policy visible_colours on colours for all to joe using (visible =
> true);
> CREATE POLICY
[...]
> > insert into colours (name, visible) values ('transparent',false);
> ERROR:  new row violates WITH CHECK OPTION for "colours"
> DETAIL:  Failing row contains (7, transparent, f).
> 
> > select * from pg_policies ;
>policyname| tablename | roles | cmd |   qual   | with_check
> -+---+---+-+--+
>  visible_colours | colours   | {joe} | ALL | (visible = true) |
> (1 row)
> 
> There was no WITH CHECK OPTION.

As I hope is clear if you look at the documentation- if the WITH CHECK
clause is omitted, then the USING clause is used for both filtering and
checking new records, otherwise you'd be able to add records which
aren't visible to you.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] RLS Design

2014-09-19 Thread Thom Brown

On 14 September 2014 16:38, Stephen Frost  wrote:

> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Thu, Sep 11, 2014 at 3:08 PM, Stephen Frost 
> wrote:
> > > If we want to be able to disable RLS w/o dropping the policies, then I
> > > think we have to completely de-couple the two and users would then have
> > > both add policies AND turn on RLS to have RLS actually be enabled for a
> > > given table.  I'm on the fence about that.
> > >
> > > Thoughts?
> >
> > A strong +1 for doing just that.
>
> Alright, updated patch attached which does just that (thanks to Adam
> for the updates for this and testing pg_dump- I just reviewed it and
> added some documentation updates and other minor improvements), and
> rebased to master.  Also removed the catversion bump, so it should apply
> cleanly for people, for a while anyway.
>

This is testing what has been committed:

# create table colours (id serial, name text, visible boolean);
CREATE TABLE

# insert into colours (name, visible) values
('blue',true),('yellow',true),('ultraviolet',false),('green',true),('infrared',false);
INSERT 0 5

# create policy visible_colours on colours for all to joe using (visible =
true);
CREATE POLICY

# grant all on colours to public;
GRANT

# grant all on sequence colours_id_seq to public;
GRANT

# alter table colours enable row level security ;
ALTER TABLE

\c - joe

> select * from colours;
 id |  name  | visible
++-
  1 | blue   | t
  2 | yellow | t
  4 | green  | t
(3 rows)

> insert into colours (name, visible) values ('purple',true);
INSERT 0 1

> insert into colours (name, visible) values ('transparent',false);
ERROR:  new row violates WITH CHECK OPTION for "colours"
DETAIL:  Failing row contains (7, transparent, f).

> select * from pg_policies ;
   policyname| tablename | roles | cmd |   qual   | with_check
-+---+---+-+--+
 visible_colours | colours   | {joe} | ALL | (visible = true) |
(1 row)


There was no WITH CHECK OPTION.

-- 
Thom

[HACKERS] CreateEventTrigStmt copy fix

2014-09-19 Thread Petr Jelinek


Hi hackers,

I was trying to create event trigger inside DO statement inside an 
extension SQL script and noticed that the new event trigger has empty 
evtevent field.
After some tinkering with gdb I found out that the memory context 
switches sometimes clear the eventname in CreateEventTrigStmt struct. 
The reason for this is that _copyCreateEventTrigStmt uses 
COPY_SCALAR_FIELD on eventname instead of COPY_STRING_FIELD.


Attached patch fixes this and also the same issue in 
_equalCreateEventTrigStmt.


This should be back-patched to 9.3 where event triggers were introduced.

--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index aa053a0..24addfb 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3610,7 +3610,7 @@ _copyCreateEventTrigStmt(const CreateEventTrigStmt *from)
 	CreateEventTrigStmt *newnode = makeNode(CreateEventTrigStmt);
 
 	COPY_STRING_FIELD(trigname);
-	COPY_SCALAR_FIELD(eventname);
+	COPY_STRING_FIELD(eventname);
 	COPY_NODE_FIELD(whenclause);
 	COPY_NODE_FIELD(funcname);
 
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 719923e..9c19f44 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1805,7 +1805,7 @@ static bool
 _equalCreateEventTrigStmt(const CreateEventTrigStmt *a, const CreateEventTrigStmt *b)
 {
 	COMPARE_STRING_FIELD(trigname);
-	COMPARE_SCALAR_FIELD(eventname);
+	COMPARE_STRING_FIELD(eventname);
 	COMPARE_NODE_FIELD(funcname);
 	COMPARE_NODE_FIELD(whenclause);
 

-- 
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] RLS Design

On 2014-09-19 11:53:06 -0400, Robert Haas wrote:
> On Sun, Sep 14, 2014 at 11:38 AM, Stephen Frost  wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> On Thu, Sep 11, 2014 at 3:08 PM, Stephen Frost  wrote:
> >> > If we want to be able to disable RLS w/o dropping the policies, then I
> >> > think we have to completely de-couple the two and users would then have
> >> > both add policies AND turn on RLS to have RLS actually be enabled for a
> >> > given table.  I'm on the fence about that.
> >> >
> >> > Thoughts?
> >>
> >> A strong +1 for doing just that.
> >
> > Alright, updated patch attached which does just that (thanks to Adam
> > for the updates for this and testing pg_dump- I just reviewed it and
> > added some documentation updates and other minor improvements), and
> > rebased to master.  Also removed the catversion bump, so it should apply
> > cleanly for people, for a while anyway.
> 
> I specifically asked you to hold off on committing this until there
> was adequate opportunity for review, and explained my reasoning.  You
> committed it anyway.

I was also rather surprised by the push. I wanted to write something
about it, but:

> This patch, on the other hand, was massively revised after the start
> of the CommitFest after many months of inactivity and committed with
> no thorough review by anyone who was truly independent of the
> development effort.  It was then committed with no warning over a
> specific request, from another committer, that more time be allowed
> for review.

says it better.

I think that's generally the case, but doubly so with sensitive stuff
like this.

> I wonder if I am equally free to commit my own patches without
> properly observing the CommitFest process, because it would be a whole
> lot faster.  My pg_background patches have been pending since before
> the start of the August CommitFest and I accepted that I would have to
> wait an extra two months to commit those because of a *clerical
> error*, namely my failure to actually add them to the CommitFest.

FWIW, I think if a patch has been sent in time and has gotten a decent
amount of review *and* agreement it's fair for a committer to push
forward. That doesn't apply to this thread, but sometimes does for
others.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Final Patch for GROUPING SETS

2014-09-19 Thread Petr Jelinek


On 19/09/14 17:52, Andres Freund wrote:

On 2014-09-19 16:35:52 +0100, Andrew Gierth wrote:

  Marti> But is anyone actually using YAML output format, or was it
  Marti> implemented simply "because we can"?

Until someone decides to dike it out, I think we are obligated to make
it produce something resembling correct output.


I vote for ripping it out. There really isn't any justification for it
and it broke more than once.



Even though I really like YAML I say +1, mainly because any YAML 1.2 
parser should be able to parse JSON output without problem...



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


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

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

2014-09-19 Thread Sawada Masahiko

On Fri, Sep 19, 2014 at 11:05 PM, Rahila Syed  wrote:
>
>>Please find attached patch to compress FPW using pglz compression.
> Please refer the updated patch attached. The earlier patch added few
> duplicate lines of code in guc.c file.
>
> compress_fpw_v1.patch
> 
>
>

I got patching failed to HEAD.
Detail is following.

Hunk #3 FAILED at 142.
1 out of 3 hunks FAILED -- saving rejects to file
src/backend/access/rmgrdesc/xlogdesc.c.rej

Regards,

---
Sawada Masahiko


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

Re: [HACKERS] RLS Design

On Sun, Sep 14, 2014 at 11:38 AM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Thu, Sep 11, 2014 at 3:08 PM, Stephen Frost  wrote:
>> > If we want to be able to disable RLS w/o dropping the policies, then I
>> > think we have to completely de-couple the two and users would then have
>> > both add policies AND turn on RLS to have RLS actually be enabled for a
>> > given table.  I'm on the fence about that.
>> >
>> > Thoughts?
>>
>> A strong +1 for doing just that.
>
> Alright, updated patch attached which does just that (thanks to Adam
> for the updates for this and testing pg_dump- I just reviewed it and
> added some documentation updates and other minor improvements), and
> rebased to master.  Also removed the catversion bump, so it should apply
> cleanly for people, for a while anyway.

I specifically asked you to hold off on committing this until there
was adequate opportunity for review, and explained my reasoning.  You
committed it anyway.

I wonder if I am equally free to commit my own patches without
properly observing the CommitFest process, because it would be a whole
lot faster.  My pg_background patches have been pending since before
the start of the August CommitFest and I accepted that I would have to
wait an extra two months to commit those because of a *clerical
error*, namely my failure to actually add them to the CommitFest.
This patch, on the other hand, was massively revised after the start
of the CommitFest after many months of inactivity and committed with
no thorough review by anyone who was truly independent of the
development effort.  It was then committed with no warning over a
specific request, from another committer, that more time be allowed
for review.

I'm really disappointed by that.  I feel I'm essentially getting
punished for trying to follow what I understand to the process, which
has involved me doing huge amounts of review of other people's patches
and waiting a very long time to get my own stuff committed, while you
bull ahead with your own patches.

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

> "Andrew" == Andrew Gierth  writes:

 Andrew> You're telling me. Also, feeding it to an online yaml-to-json
 Andrew> converter gives the result as [["two","four"],["two"],null]
 Andrew> which is not quite the same as the json version. An
 Andrew> alternative would be:

Oh, another YAML alternative would be:

Grouping Sets:
  - ["two","four"]
  - ["two"]
  - []

Would that be better? (It's not consistent with other YAML outputs like
sort/group keys, but it's equally legal as far as I can tell and seems
more readable.)

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] Final Patch for GROUPING SETS

On 2014-09-19 16:35:52 +0100, Andrew Gierth wrote:
>  Marti> But is anyone actually using YAML output format, or was it
>  Marti> implemented simply "because we can"?
> 
> Until someone decides to dike it out, I think we are obligated to make
> it produce something resembling correct output.

I vote for ripping it out. There really isn't any justification for it
and it broke more than once.

Greg: Did you actually ever end up using the yaml output?

Greetings,

Andres Freund

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


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

Re: [HACKERS] Final Patch for GROUPING SETS

> "Marti" == Marti Raudsepp  writes:

 >> (yaml format)
 >> Grouping Sets:
 >>   - - "two"
 >> - "four"
 >>   - - "two"
 >>   -

 Marti> Now this is weird.

You're telling me. Also, feeding it to an online yaml-to-json
converter gives the result as [["two","four"],["two"],null] which is
not quite the same as the json version. An alternative would be:

  Grouping Sets:
- - "two"
  - "four"
- - "two"
- []

or

  Grouping Sets:
-
  - "two"
  - "four"
-
  - "two"
- []

though I haven't managed to get that second one to work yet.

 Marti> But is anyone actually using YAML output format, or was it
 Marti> implemented simply "because we can"?

Until someone decides to dike it out, I think we are obligated to make
it produce something resembling correct output.

 Marti> The reason I bring this up is that queries are frequently
 Marti> dynamically generated by programs.

Good point.

 >> would you want the original syntax preserved in views

 Marti> Doesn't matter IMO.

I think it's fairly consistent for the parser to do this, since we do
a number of other normalization steps there (removing excess nesting
and so on). This turns out to be quite trivial.

-- 
Andrew (irc:RhodiumToad)

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

[HACKERS] identify_locking_dependencies is broken for schema-only dumps

This can lead to deadlocks during parallel restore.  Test case:

create table bar (a int primary key, b int);
create table baz (z int, a int references bar);
create view foo as select a, b, sum(1) from bar group by a union all
select z, a, 0 from baz;

I dumped this with: pg_dump -Fc -s -f test.dmp
Then: while (dropdb rhaas; createdb; pg_restore -O -d rhaas -j3
test.dmp); do true; done

This quickly fails for me with:

pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 2155; 2606 47822 FK
CONSTRAINT baz_a_fkey rhaas
pg_restore: [archiver (db)] could not execute query: ERROR:  deadlock detected
DETAIL:  Process 81791 waits for AccessExclusiveLock on relation 47862
of database 47861; blocked by process 81789.
Process 81789 waits for AccessShareLock on relation 47865 of database
47861; blocked by process 81791.
HINT:  See server log for query details.
Command was: ALTER TABLE ONLY baz
ADD CONSTRAINT baz_a_fkey FOREIGN KEY (a) REFERENCES bar(a);
WARNING: errors ignored on restore: 2

The attached patch seems to fix it for me.

Comments?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index ded9135..0393153 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -4140,11 +4140,10 @@ identify_locking_dependencies(ArchiveHandle *AH, TocEntry *te)
 		return;
 
 	/*
-	 * We assume the item requires exclusive lock on each TABLE DATA item
-	 * listed among its dependencies.  (This was originally a dependency on
-	 * the TABLE, but fix_dependencies repointed it to the data item. Note
-	 * that all the entry types we are interested in here are POST_DATA, so
-	 * they will all have been changed this way.)
+	 * We assume the item requires exclusive lock on each TABLE or TABLE DATA
+	 * item listed among its dependencies.  (fix_dependencies may have
+	 * repointed TABLE dependencies at TABLE DATA items, but not in a
+	 * schema-only dump.)
 	 */
 	lockids = (DumpId *) pg_malloc(te->nDeps * sizeof(DumpId));
 	nlockids = 0;
@@ -4153,7 +4152,8 @@ identify_locking_dependencies(ArchiveHandle *AH, TocEntry *te)
 		DumpId		depid = te->dependencies[i];
 
 		if (depid <= AH->maxDumpId && AH->tocsByDumpId[depid] != NULL &&
-			strcmp(AH->tocsByDumpId[depid]->desc, "TABLE DATA") == 0)
+			((strcmp(AH->tocsByDumpId[depid]->desc, "TABLE DATA") == 0) ||
+			  strcmp(AH->tocsByDumpId[depid]->desc, "TABLE") == 0))
 			lockids[nlockids++] = depid;
 	}
 

-- 
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] GCC memory barriers are missing "cc" clobbers

On 2014-09-19 10:58:56 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I don't really see any need to backpatch though, do you?

> Well, I'd make it the same in all branches which have that code, which
> is not very far back is it?

It was already introduced in 9.2 - no idea whether that's "far back" or
not ;). Done.

Greetings,

Andres Freund

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


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

Re: [HACKERS] GCC memory barriers are missing "cc" clobbers

Andres Freund  writes:
> On 2014-09-19 09:58:01 -0400, Tom Lane wrote:
>> While it might not be buggy as it stands, I think we should add the "cc"
>> rather than rely on it being implicit.  One reason is that people will
>> look at the x86 cases when developing code for other architectures, and
>> they could easily forget to add "cc" on machines where it does matter.

> Fair point. It's also extremly poorly documented - my answer is from a
> gcc dev, I haven't found an official document stating it. I don't really
> see any need to backpatch though, do you?

Well, I'd make it the same in all branches which have that code, which
is not very far back is it?

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] GCC memory barriers are missing "cc" clobbers

On 2014-09-19 09:58:01 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2014-09-19 12:00:16 +0200, Andres Freund wrote:
> >> But addl sets condition flags. So this really also needs a "cc" clobber?
> >> Or am I missing something?
> 
> > What I missed is that x86 has an implied "cc" clobber for every inline
> > assembly statement. So forget that.
> 
> While it might not be buggy as it stands, I think we should add the "cc"
> rather than rely on it being implicit.  One reason is that people will
> look at the x86 cases when developing code for other architectures, and
> they could easily forget to add "cc" on machines where it does matter.

Fair point. It's also extremly poorly documented - my answer is from a
gcc dev, I haven't found an official document stating it. I don't really
see any need to backpatch though, do you?

Andres Freund

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


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

Re: [HACKERS] pg_xlogdump --stats

Hi,

On 2014-09-19 15:39:37 +0530, Abhijit Menon-Sen wrote:
> I've attached the revised patch, split up differently:
> 
> 1. Introduce rm_identify, change rm_desc, glue the two together in
>xlog.c
> 2. Introduce pg_xlogdump --stats[=record].

I've pushed these after some fixing up.

As we previously discussed, I think we might want to adjust the stats
further. But this is already really useful.

Thanks!

Andres Freund

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


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

Re: [HACKERS] Final Patch for GROUPING SETS

2014-09-19 Thread Marti Raudsepp

On Fri, Sep 19, 2014 at 4:45 AM, Andrew Gierth
 wrote:
>  GroupAggregate  (cost=1122.39..1197.48 rows=9 width=8)
>Group Key: two, four
>Group Key: two
>Group Key: ()

>   "Grouping Sets": [
> ["two", "four"],
> ["two"],
> []

+1 looks good to me.

> (yaml format)
> Grouping Sets:
>   - - "two"
> - "four"
>   - - "two"
>   -

Now this is weird. But is anyone actually using YAML output format, or
was it implemented simply "because we can"?

>  Marti> Do you think it would be reasonable to normalize single-set
>  Marti> grouping sets into a normal GROUP BY?
> It's certainly possible, though it would seem somewhat odd to write
> queries that way.

The reason I bring this up is that queries are frequently dynamically
generated by programs. Coders are unlikely to special-case SQL
generation when there's just a single grouping set. And that's the
power of relational databases: the optimization work is done in the
database pretty much transparently to the coder (when it works, that
is).

> would you want the original syntax preserved in views

Doesn't matter IMO.

>  Marti> I'd expect GROUP BY () to be fully equivalent to having no
>  Marti> GROUP BY clause, but there's a difference in explain
>  Marti> output. The former displays "Grouping Sets: ()" which is odd,
>  Marti> since none of the grouping set keywords were used.
> That's an implementation artifact, in the sense that we preserve the
> fact that GROUP BY () was used by using an empty grouping set. Is it
> a problem, really, that it shows up that way in explain?

No, not really a problem. :)

Regards,
Marti


-- 
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] Re: Compression of full-page-writes

Rahila Syed  writes:
> Please find attached patch to compress FPW using pglz compression.

Patch not actually attached AFAICS (no, a link is not good enough).

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] Anonymous code block with parameters

2014-09-19 Thread Merlin Moncure

On Thu, Sep 18, 2014 at 5:22 PM, Hannu Krosing  wrote:
> Though it would be even nicer to have fully in-line type definition
>
> SELECT (tup).* FROM
>   (
> SELECT CASE WHEN .. THEN ROW(1,2,3)::(a int, b text, c int2)
> WHEN .. THEN ROW(2,3,4)
> ELSE ROW (3,4,5) END AS tup
> FROM ..
>   ) ss

+1.   Workaround at present (which I mostly use during json serialization) is:

SELECT (tup).* FROM
  (
SELECT CASE WHEN .. THEN
   (SELECT q FROM (SELECT 1, 2, 3) q)
WHEN .. THEN
   (SELECT q FROM (SELECT 2, 3, 4) q)
ELSE (SELECT q FROM (SELECT 3, 4, 5) q)
END AS tup
FROM ..
  ) ss

If you're talking in line type definitions (which is kinda off topic)
though, it'd be nice to consider:

* nested type definition:
create type foo_t as
(
  a text,
  b int,
  bars bar_t[] as
  (
 c int,
 d text
  ),
  baz baz_t as
  (
e text,
f text
  )
);

* ...and recursive type references (not being able to recursively
serialize json is a major headache)
create type foo_t as
(
  path text,
  children foo_t[]
);

merlin


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

Re: [HACKERS] jsonb format is pessimal for toast compression

Heikki Linnakangas  writes:
> Tom: You mentioned earlier that your patch fixes some existing bugs. 
> What were they?

What I remember at the moment (sans caffeine) is that the routines for
assembling jsonb values out of field data were lacking some necessary
tests for overflow of the size/offset fields.  If you like I can apply
those fixes separately, but I think they were sufficiently integrated with
other changes in the logic that it wouldn't really help much for patch
reviewability.

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] Re: Compression of full-page-writes

2014-09-19 Thread Rahila Syed


>Please find attached patch to compress FPW using pglz compression.
Please refer the updated patch attached. The earlier patch added few
duplicate lines of code in guc.c file.

compress_fpw_v1.patch
  


Thank you,
Rahila Syed



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Compression-of-full-page-writes-tp5769039p5819659.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] GCC memory barriers are missing "cc" clobbers

Andres Freund  writes:
> On 2014-09-19 12:00:16 +0200, Andres Freund wrote:
>> But addl sets condition flags. So this really also needs a "cc" clobber?
>> Or am I missing something?

> What I missed is that x86 has an implied "cc" clobber for every inline
> assembly statement. So forget that.

While it might not be buggy as it stands, I think we should add the "cc"
rather than rely on it being implicit.  One reason is that people will
look at the x86 cases when developing code for other architectures, and
they could easily forget to add "cc" on machines where it does matter.

regards, tom lane

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

Re: [HACKERS] jsonb format is pessimal for toast compression

2014-09-19 Thread Heikki Linnakangas


On 09/18/2014 09:27 PM, Heikki Linnakangas wrote:

On 09/18/2014 07:53 PM, Josh Berkus wrote:

On 09/16/2014 08:45 PM, Tom Lane wrote:

We're somewhat comparing apples and oranges here, in that I pushed my
approach to something that I think is of committable quality (and which,
not incidentally, fixes some existing bugs that we'd need to fix in any
case); while Heikki's patch was just proof-of-concept.  It would be worth
pushing Heikki's patch to committable quality so that we had a more
complete understanding of just what the complexity difference really is.


Is anyone actually working on this?

If not, I'm voting for the all-lengths patch so that we can get 9.4 out
the door.


I'll try to write a more polished patch tomorrow. We'll then see what it
looks like, and can decide if we want it.


Ok, here are two patches. One is a refined version of my earlier patch, 
and the other implements the separate offsets array approach. They are 
both based on Tom's jsonb-lengths-merged.patch, so they include all the 
whitespace fixes etc. he mentioned.


There is no big difference in terms of code complexity between the 
patches. IMHO the separate offsets array is easier to understand, but it 
makes for more complicated accessor macros to find the beginning of the 
variable-length data.


Unlike Tom's patch, these patches don't cache any offsets when doing a 
binary search. Doesn't seem worth it, when the access time is O(1) anyway.


Both of these patches have a #define JB_OFFSET_STRIDE for the "stride 
size". For the separate offsets array, the offsets array has one element 
for every JB_OFFSET_STRIDE children. For the other patch, every 
JB_OFFSET_STRIDE child stores the end offset, while others store the 
length. A smaller value makes random access faster, at the cost of 
compressibility / on-disk size. I haven't done any measurements to find 
the optimal value, the values in the patches are arbitrary.


I think we should bite the bullet and break compatibility with 9.4beta2 
format, even if we go with "my patch". In a jsonb object, it makes sense 
to store all the keys first, like Tom did, because of cache benefits, 
and the future possibility to do smart EXTERNAL access. Also, even if we 
can make the on-disk format compatible, it's weird that you can get 
different runtime behavior with datums created with a beta version. 
Seems more clear to just require a pg_dump + restore.


Tom: You mentioned earlier that your patch fixes some existing bugs. 
What were they? There were a bunch of whitespace and comment fixes that 
we should apply in any case, but I couldn't see any actual bugs. I think 
we should apply those fixes separately, to make sure we don't forget 
about them, and to make it easier to review these patches.


- Heikki

diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 2fd87fc..456011a 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -196,12 +196,12 @@ jsonb_from_cstring(char *json, int len)
 static size_t
 checkStringLen(size_t len)
 {
-	if (len > JENTRY_POSMASK)
+	if (len > JENTRY_LENMASK)
 		ereport(ERROR,
 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
  errmsg("string too long to represent as jsonb string"),
  errdetail("Due to an implementation restriction, jsonb strings cannot exceed %d bytes.",
-		   JENTRY_POSMASK)));
+		   JENTRY_LENMASK)));
 
 	return len;
 }
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 04f35bf..7f7ed4f 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -26,15 +26,16 @@
  * in MaxAllocSize, and the number of elements (or pairs) must fit in the bits
  * reserved for that in the JsonbContainer.header field.
  *
- * (the total size of an array's elements is also limited by JENTRY_POSMASK,
- * but we're not concerned about that here)
+ * (The total size of an array's or object's elements is also limited by
+ * JENTRY_LENMASK, but we're not concerned about that here.)
  */
 #define JSONB_MAX_ELEMS (Min(MaxAllocSize / sizeof(JsonbValue), JB_CMASK))
 #define JSONB_MAX_PAIRS (Min(MaxAllocSize / sizeof(JsonbPair), JB_CMASK))
 
-static void fillJsonbValue(JEntry *array, int index, char *base_addr,
+static void fillJsonbValue(JEntry *children, int index,
+			   char *base_addr, uint32 offset,
 			   JsonbValue *result);
-static bool	equalsJsonbScalarValue(JsonbValue *a, JsonbValue *b);
+static bool equalsJsonbScalarValue(JsonbValue *a, JsonbValue *b);
 static int	compareJsonbScalarValue(JsonbValue *a, JsonbValue *b);
 static Jsonb *convertToJsonb(JsonbValue *val);
 static void convertJsonbValue(StringInfo buffer, JEntry *header, JsonbValue *val, int level);
@@ -42,7 +43,7 @@ static void convertJsonbArray(StringInfo buffer, JEntry *header, JsonbValue *val
 static void convertJsonbObject(StringInfo buffer, JEntry *header, JsonbValue *val, int level);
 static void convertJsonbScalar(StringInfo buffer, JEntry *header, Json

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

2014-09-19 Thread Rahila Syed

Hello,

>Maybe. Let's get the basic patch done first; then we can argue about that

Please find attached patch to compress FPW using pglz compression.
All backup blocks in WAL record are compressed at once before inserting it
into WAL buffers . Full_page_writes GUC has been modified to accept three
values
1. On
2. Compress
3. Off
FPW are compressed when full_page_writes is set to compress. FPW generated
forcibly during online backup even when full_page_writes is off are also
compressed. When full_page_writes is set on FPW are not compressed.
Benckmark:
Server Specification:
Processors:Intel® Xeon ® Processor E5-2650 (2 GHz, 8C/16T, 20 MB) * 2 nos
RAM: 32GB
Disk : HDD 450GB 10K Hot Plug 2.5-inch SAS HDD * 8 nos
1 x 450 GB SAS HDD, 2.5-inch, 6Gb/s, 10,000 rpm
Checkpoint segments: 1024
Checkpoint timeout: 5 mins
pgbench -c 64 -j 64 -r -T 900 -M prepared
Scale factor: 1000

WAL generated (MB)
Throughput
(tps) Latency(ms)
On 9235.43

979.03 65.36
Compress(pglz) 6518.68
1072.34 59.66
Off501.04
1135.17
56.34

The results show around 30 percent decrease in WAL volume due to
compression of FPW.
compress_fpw_v1.patch

--
View this message in context:
http://postgresql.1045698.n5.nabble.com/Compression-of-full-page-writes-tp5769039p5819645.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] Scaling shared buffer eviction

2014-09-19 Thread Amit Kapila

On Tue, Sep 16, 2014 at 10:21 PM, Robert Haas  wrote:

> On Tue, Sep 16, 2014 at 8:18 AM, Amit Kapila 
> wrote:
>
>> In most cases performance with patch is slightly less as compare
>> to HEAD and the difference is generally less than 1% and in a case
>> or 2 close to 2%. I think the main reason for slight difference is that
>> when the size of shared buffers is almost same as data size, the number
>> of buffers it needs from clock sweep are very less, as an example in first
>> case (when size of shared buffers is 12286MB), it actually needs at most
>> 256 additional buffers (2MB) via clock sweep, where as bgreclaimer
>> will put 2000 (high water mark) additional buffers (0.5% of shared buffers
>> is greater than 2000 ) in free list, so bgreclaimer does some extra work
>> when it is not required and it also leads to condition you mentioned
>> down (freelist will contain buffers that have already been touched since
>> we added them).  Now for case 2 (12166MB), we need buffers more
>> than 2000 additional buffers, but not too many, so it can also have
>> similar effect.
>>
>
> So there are two suboptimal things that can happen and they pull in
> opposite directions.  I think you should instrument the server how often
> each is happening.  #1 is that we can pop a buffer from the freelist and
> find that it's been touched.  That means we wasted the effort of putting it
> on the freelist in the first place.  #2 is that we can want to pop a buffer
> from the freelist and find it empty and thus be forced to run the clock
> sweep ourselves.   If we're having problem #1, we could improve things by
> reducing the water marks.  If we're having problem #2, we could improve
> things by increasing the water marks.  If we're having both problems, then
> I dunno.  But let's get some numbers on the frequency of these specific
> things, rather than just overall tps numbers.
>

Specific numbers of both the configurations for which I have
posted data in previous mail are as follows:

Scale Factor - 800
Shared_Buffers - 12286MB (Total db size is 12288MB)
Client and Thread Count = 64
buffers_touched_freelist - count of buffers that backends found touched
after
popping from freelist.
buffers_backend_clocksweep - count of buffer allocations not satisfied from
freelist

  buffers_alloc 1531023  buffers_backend_clocksweep 0
buffers_touched_freelist 0

Scale Factor - 800
Shared_Buffers - 12166MB (Total db size is 12288MB)
Client and Thread Count = 64

  buffers_alloc 1531010  buffers_backend_clocksweep 0
buffers_touched_freelist 0

In both the above cases, I have taken data multiple times to ensure
correctness.  From the above data, it is evident that in both the above
configurations all the requests are satisfied from the initial freelist.
Basically the amount of shared buffers configured
(12286MB = 1572608 buffers and 12166MB = 1557248 buffers) are
sufficient to contain all the work load for pgbench run.

So now the question is why we are seeing small variation (<1%) in data
in case all the data fits in shared buffers and the reason could be that
we have added few extra instructions (due to increase in StrategyControl
structure size, additional function call, one or two new assignments) in the
Buffer Allocation path (the extra instructions will also be only till all
the data
pages gets associated with buffers, after that the control won't even reach
StrategyGetBuffer()) or it may be due to variation across different runs
with
different binaries.

I have went ahead to take the data in cases shared buffers are tiny bit
(0.1%
and .05%) less than workload (based on buffer allocations done in above
cases).

Performance Data
---

Scale Factor - 800
Shared_Buffers - 11950MB

  Client_Count/Patch_Ver 8 16 32 64 128  HEAD 68424 132540 195496 279511
283280  sbe_v9 68565 132709 194631 284351 289333

Scale Factor - 800
Shared_Buffers - 11955MB

  Client_Count/Patch_Ver 8 16 32 64 128  HEAD 68331 127752 196385 274387
281753  sbe_v9 68922 131314 194452 284292 287221

The above data indicates that performance is better with patch
in almost all cases and especially at high concurrency (64 and
128 client count).

The overall conclusion is that with patch
a. when the data can fit in RAM and not completely in shared buffers,
the performance/scalability is quite good even if shared buffers are just
tiny bit less that all the data.
b. when shared buffers are sufficient to contain all the data, then there is
a slight difference (<1%) in performance.

>
>> d. Lets not do anything as if user does such a configuration, he should
>> be educated to configure shared buffers in a better way and or the
>> performance hit doesn't seem to be justified to do any further
>> work.
>>
>
> At least worth entertaining.
>
> Based on further analysis, I think this is the way to go.

Attached find the patch for new stat (buffers_touched_freelist) just in
case you want to run the patch with it and detailed (individual run)
performance data.

Re: [HACKERS] GCC memory barriers are missing "cc" clobbers

Hi,

On 2014-09-19 12:00:16 +0200, Andres Freund wrote:
> 
> barrier.h defines memory barriers for x86 as:
> 32bit:
> #define pg_memory_barrier()   \
> __asm__ __volatile__ ("lock; addl $0,0(%%esp)" : : : "memory")
> 64bit:
> #define pg_memory_barrier()   \
>   __asm__ __volatile__ ("lock; addl $0,0(%%rsp)" : : : "memory")
> 
> But addl sets condition flags. So this really also needs a "cc" clobber?
> Or am I missing something?

What I missed is that x86 has an implied "cc" clobber for every inline
assembly statement. So forget that.

Greetings,

Andres Freund

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


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

Re: [HACKERS] pg_xlogdump --stats

At 2014-09-19 15:39:37 +0530, a...@2ndquadrant.com wrote:
>
> I hope I didn't miss anything this time.

But of course I did. The attached fixup makes the output of pg_xlogdump
match that of xlog_outdesc for unidentifiable records (UNKNOWN (%x)).
Sorry for the inconvenience.

-- Abhijit
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index 0a176bb..7686a4f 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -515,7 +515,7 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
 
 id = desc->rm_identify(rj << 4);
 if (id == NULL)
-	id = psprintf("0x%x", rj << 4);
+	id = psprintf("UNKNOWN (%x)", rj << 4);
 
 XLogDumpStatsRow(psprintf("%s/%s", desc->rm_name, id),
  count, 100 * (double)count / total_count,

-- 
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_xlogdump --stats

I've attached the revised patch, split up differently:

1. Introduce rm_identify, change rm_desc, glue the two together in
   xlog.c
2. Introduce pg_xlogdump --stats[=record].

The requested changes (16, filter, z:, UNKNOWN) are included. The
grammar nitpicking and rebase cruft is^Ware^Wain't included.

I ran Postgres with WAL_DEBUG for a while, and I ran pg_xlogdump with
--stats (and --stats=rmgr) and --stats=record with and without a few
different variants of "-r heap". Everything looked OK to me.

I hope I didn't miss anything this time.

-- Abhijit
>From da6df2f24bb8ea768d6e7671474b0ad7d0b798d1 Mon Sep 17 00:00:00 2001
From: Abhijit Menon-Sen 
Date: Fri, 19 Sep 2014 15:08:45 +0530
Subject: Introduce an RmgrDescData.rm_identify callback to name records
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

For example, rm_identify turns XLOG_BTREE_VACUUM into "VACUUM" based on
xl_info. rm_desc now omits the prefix that (unsystematically) duplicated
the rm_identity output, and only provides extra detail if available:

LOG:  INSERT @ 0/16C50F0: prev 0/16C5080; xid 690; len 31: Heap/INSERT: rel 1663/16384/16385; tid 0/3

In the above, "Heap" comes from rm_name, "INSERT" comes from
rm_identify, and "rel 1663/…" comes from rm_desc.

The three are glued together by a new xlog_outdesc function in xlog.c.
---
 contrib/pg_xlogdump/rmgrdesc.c|   2 +-
 src/backend/access/rmgrdesc/clogdesc.c|  27 ---
 src/backend/access/rmgrdesc/dbasedesc.c   |  24 +-
 src/backend/access/rmgrdesc/gindesc.c |  54 ++---
 src/backend/access/rmgrdesc/gistdesc.c|  25 +-
 src/backend/access/rmgrdesc/hashdesc.c|   6 ++
 src/backend/access/rmgrdesc/heapdesc.c| 123 +
 src/backend/access/rmgrdesc/mxactdesc.c   |  37 ++---
 src/backend/access/rmgrdesc/nbtdesc.c | 124 +++---
 src/backend/access/rmgrdesc/relmapdesc.c  |  19 -
 src/backend/access/rmgrdesc/seqdesc.c |  21 +++--
 src/backend/access/rmgrdesc/smgrdesc.c|  25 --
 src/backend/access/rmgrdesc/spgdesc.c |  59 +++---
 src/backend/access/rmgrdesc/standbydesc.c |  25 --
 src/backend/access/rmgrdesc/tblspcdesc.c  |  25 --
 src/backend/access/rmgrdesc/xactdesc.c|  48 +---
 src/backend/access/rmgrdesc/xlogdesc.c|  72 -
 src/backend/access/transam/rmgr.c |   4 +-
 src/backend/access/transam/xlog.c |  45 +--
 src/include/access/clog.h |   1 +
 src/include/access/gin.h  |   1 +
 src/include/access/gist_private.h |   1 +
 src/include/access/hash.h |   1 +
 src/include/access/heapam_xlog.h  |   2 +
 src/include/access/multixact.h|   1 +
 src/include/access/nbtree.h   |   1 +
 src/include/access/rmgr.h |   2 +-
 src/include/access/rmgrlist.h |  34 
 src/include/access/spgist.h   |   1 +
 src/include/access/xact.h |   1 +
 src/include/access/xlog.h |   1 +
 src/include/access/xlog_internal.h|  11 +++
 src/include/catalog/storage_xlog.h|   1 +
 src/include/commands/dbcommands.h |   1 +
 src/include/commands/sequence.h   |   1 +
 src/include/commands/tablespace.h |   1 +
 src/include/storage/standby.h |   1 +
 src/include/utils/relmapper.h |   1 +
 38 files changed, 597 insertions(+), 232 deletions(-)

diff --git a/contrib/pg_xlogdump/rmgrdesc.c b/contrib/pg_xlogdump/rmgrdesc.c
index cbcaaa6..1064d34 100644
--- a/contrib/pg_xlogdump/rmgrdesc.c
+++ b/contrib/pg_xlogdump/rmgrdesc.c
@@ -27,7 +27,7 @@
 #include "storage/standby.h"
 #include "utils/relmapper.h"
 
-#define PG_RMGR(symname,name,redo,desc,startup,cleanup) \
+#define PG_RMGR(symname,name,redo,desc,identify,startup,cleanup) \
 	{ name, desc, },
 
 const RmgrDescData RmgrDescTable[RM_MAX_ID + 1] = {
diff --git a/src/backend/access/rmgrdesc/clogdesc.c b/src/backend/access/rmgrdesc/clogdesc.c
index e82baa8..8beb6d0 100644
--- a/src/backend/access/rmgrdesc/clogdesc.c
+++ b/src/backend/access/rmgrdesc/clogdesc.c
@@ -23,20 +23,29 @@ clog_desc(StringInfo buf, XLogRecord *record)
 	char	   *rec = XLogRecGetData(record);
 	uint8		info = record->xl_info & ~XLR_INFO_MASK;
 
-	if (info == CLOG_ZEROPAGE)
+	if (info == CLOG_ZEROPAGE || info == CLOG_TRUNCATE)
 	{
 		int			pageno;
 
 		memcpy(&pageno, rec, sizeof(int));
-		appendStringInfo(buf, "zeropage: %d", pageno);
+		appendStringInfo(buf, "%d", pageno);
 	}
-	else if (info == CLOG_TRUNCATE)
-	{
-		int			pageno;
+}
 
-		memcpy(&pageno, rec, sizeof(int));
-		appendStringInfo(buf, "truncate before: %d", pageno);
+const char *
+clog_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info)
+	{
+		case CLOG_ZEROPAGE:
+			id = "ZEROPAGE";
+			break;
+		case CLOG_TRUNCATE:
+			id = "TRUNCATE";
+			break;
 	}
-	else
-		appendStringInfoString

[HACKERS] GCC memory barriers are missing "cc" clobbers

Hi,

barrier.h defines memory barriers for x86 as:
32bit:
#define pg_memory_barrier()   \
__asm__ __volatile__ ("lock; addl $0,0(%%esp)" : : : "memory")
64bit:
#define pg_memory_barrier() \
__asm__ __volatile__ ("lock; addl $0,0(%%rsp)" : : : "memory")

But addl sets condition flags. So this really also needs a "cc" clobber?
Or am I missing something?

Greetings,

Andres Freund

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


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

Re: [HACKERS] pg_xlogdump --stats

Hi,

On 2014-09-19 14:38:29 +0530, Abhijit Menon-Sen wrote:
> > b) I'm not against it, but I wonder if the best way to add the
> >SizeOfXLogRecord to the record size. It's just as much part of the
> >FPI. And this means that the record length will be > 0 even if all
> >the record data has been removed due to the FPI.
> 
> I'm not sure I understand what you are proposing here.

I'm not really proposing anything. I'm just stating that it's
notationally not clear and might be improved. Doesn't need to be now.

> > What was the reason you moved away from --stats=record/rmgr? I think
> > we possibly will add further ones, so that seems more extensible?
> 
> It was because I wanted --stats to default to "=rmgr", so I tried to
> make the argument optional, but getopt in Windows didn't like that.
> Here's an excerpt from the earlier discussion:
> 
> > 3. Some compilation error in windows
> > .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065: 
> 'optional_argument' : undeclared identifier
> > .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is 
> not a constant
> >
> > optional_argument should be added to getopt_long.h file for windows.
> 
> Hmm. I have no idea what to do about this. I did notice when I wrote the
> code that nothing else used optional_argument, but I didn't realise that
> it wouldn't work on Windows.
> 
> It may be that the best thing to do would be to avoid using
> optional_argument altogether, and have separate --stats and
> --stats-per-record options. Thoughts?

Oh, I've since implemented optional arguments for windows/replacement
getopt_long. It's used by psql since a week or so ago.

> I have no objection to doing it differently if someone tells me how to
> make Windows happy (preferably without making me unhappy).

[x] Done

> > Given that you've removed the UNKNOWNs from the rm_descs, this really
> > should add it here.
> 
> You would prefer to see HEAP/UNKNOWN rather than HEAP/32 for an
> unidentifiable xl_info?

UNKNOWN (32)? It should visually stand out more than just the
number. Somebody borked something if that happens.

Greetings,

Andres Freund


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

Re: [HACKERS] pg_xlogdump --stats

At 2014-09-19 10:44:48 +0200, and...@2ndquadrant.com wrote:
>
> >  # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
> > -# work, fall back to our own snprintf emulation (which we know uses %lld).
> > +# works, fall back to our own snprintf emulation (which we know uses %lld).
> 
> spurious independent change?

It was part of the original patch, but I guess Heikki didn't commit it,
so it was left over in the rebase.

> Also I actually think the original version is correct?

It is not. I suspect it will begin to sound wrong to you if you replace
"none" with "not one" in the sentence. But I don't care enough to argue
about it. It's a very common error.

> > +   Stats   record_stats[RM_NEXT_ID][16];
> > +} XLogDumpStats;
> 
> I dislike the literal 16 here and someplace later. A define for the max
> number of records would make it clearer.

OK, will change.

> Perhaps we should move these kind of checks outside?

OK, will change.

> a) Whoever introduced the notion of rec_len vs tot_len in regards to
>including/excluding SizeOfXLogRecord ...

(It wasn't me, honest!)

> b) I'm not against it, but I wonder if the best way to add the
>SizeOfXLogRecord to the record size. It's just as much part of the
>FPI. And this means that the record length will be > 0 even if all
>the record data has been removed due to the FPI.

I'm not sure I understand what you are proposing here.

> What was the reason you moved away from --stats=record/rmgr? I think
> we possibly will add further ones, so that seems more extensible?

It was because I wanted --stats to default to "=rmgr", so I tried to
make the argument optional, but getopt in Windows didn't like that.
Here's an excerpt from the earlier discussion:

> 3. Some compilation error in windows
> .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065: 
'optional_argument' : undeclared identifier
> .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is 
not a constant
>
> optional_argument should be added to getopt_long.h file for windows.

Hmm. I have no idea what to do about this. I did notice when I wrote the
code that nothing else used optional_argument, but I didn't realise that
it wouldn't work on Windows.

It may be that the best thing to do would be to avoid using
optional_argument altogether, and have separate --stats and
--stats-per-record options. Thoughts?

I have no objection to doing it differently if someone tells me how to
make Windows happy (preferably without making me unhappy).

> It's trivial to separate in this case, but I'd much rather have
> patches like this rm_identity stuff split up in the future.

Sorry. I'd split it up that way in the past, but forgot to do it again
in this round. Will do when I resend with the changes above.

> That means the returned value from heap_identity() is only valid until
> the next call. That at the very least needs to be written down
> explicitly somewhere.

Where? In the comment in xlog_internal.h, perhaps?

> That's already in there afaics:

Will fix (rebase cruft).

> Given that you've removed the UNKNOWNs from the rm_descs, this really
> should add it here.

You would prefer to see HEAP/UNKNOWN rather than HEAP/32 for an
unidentifiable xl_info?

-- Abhijit

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

Re: [HACKERS] pg_xlogdump --stats

At 2014-09-19 13:24:11 +0530, a...@2ndquadrant.com wrote:
>
> Good enough?

Not quite. I've attached a small additional patch that shifts the
responsibility of adding rm_name to the output from xlog_outrec to
xlog_outdesc. Now we get WAL_DEBUG output like this:

LOG:  INSERT @ 0/16C51D0: prev 0/16C5160; xid 692; len 31: Heap/INSERT: rel 
1663/16384/16385; tid 0/5

…which is consistent with pg_xlogdump --stats output too.

-- Abhijit
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1265eca..b9bf206 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1287,7 +1287,7 @@ begin:;
 			for (; rdata != NULL; rdata = rdata->next)
 appendBinaryStringInfo(&recordbuf, rdata->data, rdata->len);
 
-			appendStringInfoString(&buf, " - ");
+			appendStringInfoString(&buf, ": ");
 			xlog_outdesc(&buf, rechdr->xl_rmid, (XLogRecord *) recordbuf.data);
 		}
 		elog(LOG, "%s", buf.data);
@@ -6710,7 +6710,7 @@ StartupXLOG(void)
 			(uint32) (ReadRecPtr >> 32), (uint32) ReadRecPtr,
 			 (uint32) (EndRecPtr >> 32), (uint32) EndRecPtr);
 	xlog_outrec(&buf, record);
-	appendStringInfoString(&buf, " - ");
+	appendStringInfoString(&buf, ": ");
 	xlog_outdesc(&buf, record->xl_rmid, record);
 	elog(LOG, "%s", buf.data);
 	pfree(buf.data);
@@ -9625,8 +9625,6 @@ xlog_outrec(StringInfo buf, XLogRecord *record)
 		if (record->xl_info & XLR_BKP_BLOCK(i))
 			appendStringInfo(buf, "; bkpb%d", i);
 	}
-
-	appendStringInfo(buf, ": %s", RmgrTable[record->xl_rmid].rm_name);
 }
 #endif   /* WAL_DEBUG */
 
@@ -9639,6 +9637,9 @@ xlog_outdesc(StringInfo buf, RmgrId rmid, XLogRecord *record)
 {
 	const char *id;
 
+	appendStringInfoString(buf, RmgrTable[rmid].rm_name);
+	appendStringInfoChar(buf, '/');
+
 	id = RmgrTable[rmid].rm_identify(record->xl_info);
 	if (id == NULL)
 		appendStringInfo(buf, "%X", record->xl_info);

-- 
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_xlogdump --stats

On 2014-09-19 13:24:11 +0530, Abhijit Menon-Sen wrote:
> diff --git a/configure.in b/configure.in
> index 1392277..c3c458c 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -1637,7 +1637,7 @@ fi
>  # If we found "long int" is 64 bits, assume snprintf handles it.  If
>  # we found we need to use "long long int", better check.  We cope with
>  # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
> -# work, fall back to our own snprintf emulation (which we know uses %lld).
> +# works, fall back to our own snprintf emulation (which we know uses %lld).

spurious independent change? Also I actually think the original version
is correct?


> +typedef struct XLogDumpStats
> +{
> + uint64  count;
> + Stats   rmgr_stats[RM_NEXT_ID];
> + Stats   record_stats[RM_NEXT_ID][16];
> +} XLogDumpStats;

I dislike the literal 16 here and someplace later. A define for the max
number of records would make it clearer.

>  /*
> + * Store per-rmgr and per-record statistics for a given record.
> + */
> +static void
> +XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats, XLogRecPtr 
> ReadRecPtr, XLogRecord *record)
> +{
> + RmgrId  rmid;
> + uint8   recid;
> +
> + if (config->filter_by_rmgr != -1 &&
> + config->filter_by_rmgr != record->xl_rmid)
> + return;
> +
> + if (config->filter_by_xid_enabled &&
> + config->filter_by_xid != record->xl_xid)
> + return;

Perhaps we should move these kind of checks outside? So
XLogDumpDisplayRecord and this don't have to repeat them. I sure hope
we'll get some more. I e.g. really, really would like to have a
relfilenode check once Heikki's changes to make that possible are in.

> + stats->count++;
> +
> + /* Update per-rmgr statistics */
> +
> + rmid = record->xl_rmid;
> +
> + stats->rmgr_stats[rmid].count++;
> + stats->rmgr_stats[rmid].rec_len +=
> + record->xl_len + SizeOfXLogRecord;
> + stats->rmgr_stats[rmid].fpi_len +=
> + record->xl_tot_len - (record->xl_len + SizeOfXLogRecord);

a) Whoever introduced the notion of rec_len vs tot_len in regards to
   including/excluding SizeOfXLogRecord ...

b) I'm not against it, but I wonder if the best way to add the
   SizeOfXLogRecord to the record size. It's just as much part of the
   FPI. And this means that the record length will be > 0 even if all
   the record data has been removed due to the FPI.

>  static void
>  usage(void)
>  {
> @@ -401,6 +581,8 @@ usage(void)
>   printf(" (default: 1 or the value used in 
> STARTSEG)\n");
>   printf("  -V, --version  output version information, then 
> exit\n");
>   printf("  -x, --xid=XID  only show records with TransactionId 
> XID\n");
> + printf("  -z, --statsshow per-rmgr statistics\n");
> + printf("  -Z, --record-stats show per-record statistics\n");
>   printf("  -?, --help show this help, then exit\n");
>  }

What was the reason you moved away from --stats=record/rmgr? I think we
possibly will add further ones, so that seems more
extensible?

> diff --git a/contrib/pg_xlogdump/rmgrdesc.c b/contrib/pg_xlogdump/rmgrdesc.c
> index cbcaaa6..dc27fd1 100644
> --- a/contrib/pg_xlogdump/rmgrdesc.c
> +++ b/contrib/pg_xlogdump/rmgrdesc.c

It's trivial to separate in this case, but I'd much rather have patches
like this rm_identity stuff split up in the future.


> diff --git a/src/backend/access/rmgrdesc/heapdesc.c 
> b/src/backend/access/rmgrdesc/heapdesc.c
> index 24b6f92..91a8e72 100644
> --- a/src/backend/access/rmgrdesc/heapdesc.c
> +++ b/src/backend/access/rmgrdesc/heapdesc.c
> @@ -193,3 +193,86 @@ heap2_desc(StringInfo buf, XLogRecord *record)
>   else
>   appendStringInfoString(buf, "UNKNOWN");
>  }
> +
> +static const char *
> +append_init(const char *str)
> +{
> + static char x[32];
> +
> + strcpy(x, str);
> + strcat(x, "+INIT");
> +
> + return x;
> +}
> +
> +const char *
> +heap_identify(uint8 info)
> +{
> + const char *id = NULL;
> +
> + switch (info & XLOG_HEAP_OPMASK)
> + {
> + case XLOG_HEAP_INSERT:
> + id = "INSERT";
> + break;
> + case XLOG_HEAP_DELETE:
> + id = "DELETE";
> + break;
> + case XLOG_HEAP_UPDATE:
> + id = "UPDATE";
> + break;
> + case XLOG_HEAP_HOT_UPDATE:
> + id = "HOT_UPDATE";
> + break;
> + case XLOG_HEAP_LOCK:
> + id = "LOCK";
> + break;
> + case XLOG_HEAP_INPLACE:
> + id = "INPLACE";
> + break;
> + }
> +
> + if (info & XLOG_HEAP_INIT_PAGE)
> + id = append_init(id);
> +
> + return id;
> +}

Hm. I'm a bit doubtful

Re: [HACKERS] pg_xlogdump --stats

Hi.

I've attached two patches here:

0001-Make-pg_xlogdump-record-stats-display-summary-statis.patch is my
earlier patch to pg_xlogdump, rebased to master. It introduces the new
rm_identify callback, but doesn't touch rm_desc. Other than rebasing to
master after the INT64_FORMAT changes, I haven't changed anything.

0002-Clearly-separate-rm_identify-and-rm_desc-outputs.patch then makes
the change you (Heikki) wanted to see: rm_identify returns a name, and
rm_desc can be used to obtain optional additional detail, and xlog.c
just glues the two together in a new xlog_outdesc function. rm_desc
is changed largely only to (a) remove the "prefix: ", and (b) not
append UNKNOWN for unidentified records.

(I've done a little cleaning-up in the second patch, e.g. nbtdesc.c had
a bunch of repeated cases that I've unified, which seems a pretty nice
readability improvement overall.)

Good enough?

-- Abhijit
>From a85a5906733ca1324f7fceb6c4ff5b968a70532e Mon Sep 17 00:00:00 2001
From: Abhijit Menon-Sen 
Date: Wed, 4 Jun 2014 14:22:33 +0530
Subject: Make 'pg_xlogdump --[record-]stats' display summary statistics

---
 configure |   2 +-
 configure.in  |   2 +-
 contrib/pg_xlogdump/pg_xlogdump.c | 207 +-
 contrib/pg_xlogdump/rmgrdesc.c|   4 +-
 contrib/pg_xlogdump/rmgrdesc.h|   1 +
 doc/src/sgml/pg_xlogdump.sgml |  22 
 src/backend/access/rmgrdesc/clogdesc.c|  18 +++
 src/backend/access/rmgrdesc/dbasedesc.c   |  18 +++
 src/backend/access/rmgrdesc/gindesc.c |  42 ++
 src/backend/access/rmgrdesc/gistdesc.c|  21 +++
 src/backend/access/rmgrdesc/hashdesc.c|   6 +
 src/backend/access/rmgrdesc/heapdesc.c|  83 
 src/backend/access/rmgrdesc/mxactdesc.c   |  21 +++
 src/backend/access/rmgrdesc/nbtdesc.c |  54 
 src/backend/access/rmgrdesc/relmapdesc.c  |  15 +++
 src/backend/access/rmgrdesc/seqdesc.c |  15 +++
 src/backend/access/rmgrdesc/smgrdesc.c|  18 +++
 src/backend/access/rmgrdesc/spgdesc.c |  39 ++
 src/backend/access/rmgrdesc/standbydesc.c |  18 +++
 src/backend/access/rmgrdesc/tblspcdesc.c  |  18 +++
 src/backend/access/rmgrdesc/xactdesc.c|  33 +
 src/backend/access/rmgrdesc/xlogdesc.c|  45 +++
 src/backend/access/transam/rmgr.c |   4 +-
 src/include/access/clog.h |   1 +
 src/include/access/gin.h  |   1 +
 src/include/access/gist_private.h |   1 +
 src/include/access/hash.h |   1 +
 src/include/access/heapam_xlog.h  |   2 +
 src/include/access/multixact.h|   1 +
 src/include/access/nbtree.h   |   1 +
 src/include/access/rmgr.h |   2 +-
 src/include/access/rmgrlist.h |  34 ++---
 src/include/access/spgist.h   |   1 +
 src/include/access/xact.h |   1 +
 src/include/access/xlog.h |   1 +
 src/include/access/xlog_internal.h|   1 +
 src/include/c.h   |   3 +
 src/include/catalog/storage_xlog.h|   1 +
 src/include/commands/dbcommands.h |   1 +
 src/include/commands/sequence.h   |   1 +
 src/include/commands/tablespace.h |   1 +
 src/include/storage/standby.h |   1 +
 src/include/utils/relmapper.h |   1 +
 43 files changed, 736 insertions(+), 27 deletions(-)

diff --git a/configure b/configure
index dac8e49..22eb857 100755
--- a/configure
+++ b/configure
@@ -13091,7 +13091,7 @@ fi
 # If we found "long int" is 64 bits, assume snprintf handles it.  If
 # we found we need to use "long long int", better check.  We cope with
 # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
-# work, fall back to our own snprintf emulation (which we know uses %lld).
+# works, fall back to our own snprintf emulation (which we know uses %lld).
 
 if test "$HAVE_LONG_LONG_INT_64" = yes ; then
   if test $pgac_need_repl_snprintf = no; then
diff --git a/configure.in b/configure.in
index 1392277..c3c458c 100644
--- a/configure.in
+++ b/configure.in
@@ -1637,7 +1637,7 @@ fi
 # If we found "long int" is 64 bits, assume snprintf handles it.  If
 # we found we need to use "long long int", better check.  We cope with
 # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
-# work, fall back to our own snprintf emulation (which we know uses %lld).
+# works, fall back to our own snprintf emulation (which we know uses %lld).
 
 if test "$HAVE_LONG_LONG_INT_64" = yes ; then
   if test $pgac_need_repl_snprintf = no; then
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index c555786..11da5a8 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -15,9 +15,10 @@
 #include 
 #include 
 
-#include "access/xlog.h"
 #include "access/xlogreader.h"
 #include "access/transam.h"
+#include "catalog/pg

Re: [HACKERS] [GENERAL] [SQL] pg_multixact issues

2014-09-19 Thread Dev Kumkar

On Fri, Sep 19, 2014 at 1:03 PM, Andres Freund 
wrote:

> Yes: Learning some patience. You'd given the previous answer two hours
> before this one. Nobody is paid to work on this list...


Apologies for the delay, was working/troubleshooting same issue and was
away from my emails. :(

Regards...

Re: [HACKERS] [GENERAL] [SQL] pg_multixact issues