Re: [HACKERS] [PATCH] Refactoring of LWLock tranches

[Robert Haas]

On Thu, Dec 31, 2015 at 3:36 AM, Amit Kapila  wrote:
> Going further on this work, I have written a patch for separating the
> tranches for extensions.  The basic idea is to expose two new API's,
> first to request a new tranche and second to assign a lock from that
> tranche.
> RequestAddinLWLockTranche(const char *tranche_name, int num_lwlocks)
> will be used by extensions to request a new tranche with specified number
> of locks, this will be used instead of current API RequestAddinLWLocks().

I like this idea.

> LWLock *LWLockAssignFromTranche(const char *tranche_name) will
> assign a lock for the specified tranche.  This also ensures that no
> more than requested number of LWLocks can be assigned from a
> specified tranche.

However, this seems like an awkward API, because if there are many
LWLocks you're going to need to look up the tranche name many times,
and then you're going to need to make an array of pointers to them
somehow, introducing a thoroughly unnecessary layer of indirection.
Instead, I suggest just having a function LWLockPadded
*GetLWLockAddinTranche(const char *tranche_name) that returns a
pointer to the base of the array.

> Also I have retained NUM_USER_DEFINED_LWLOCKS in
> MainLWLockArray so that if any extensions want to assign a LWLock
> after startup, it can be used from this pool.  The tranche for such locks
> will be reported as main.

I don't like this.  I think we should get rid of
NUM_USER_DEFINED_LWLOCKS altogether.  It's just an accident waiting to
happen, and I don't think there are enough people using LWLocks from
extensions that we'll annoy very many people by breaking backward
compatibility here.  If we are going to care about backward
compatibility, then I think the old-style lwlocks should go in their
own tranche, not main.  But personally, I think it'd be better to just
force a hard break and make people switch to using the new APIs.

-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Michael Paquier]

On Mon, Jan 4, 2016 at 11:16 AM, Tom Lane  wrote:
> Jim Nasby  writes:
>> regrole and regnamespace don't run their output through quote_ident().
>> That's contrary to all the other reg* operators.
>
>> Worse, they also don't *allow* quoted input. Not only is that different
>> from reg*, it's the *opposite*:
>
> I'm inclined to think that you're right, and that this is something that
> ought to be changed.  It's not quite too late ...

Well, I can send a patch with some tests if need be... Tom, I guess
that you are already on it?
-- 
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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 6:09 PM, Peter Geoghegan  wrote:
>> I think we should get rid of it altogether (since I also agree with the
>> upthread comment that it's in the wrong place) and instead put an example
>> into section 5.7 saying directly that sub-selects, or in general
>> references to rows other than the one under test, are risky in RLS
>> policies.  We could also show the FOR UPDATE workaround there.
>
> I agree that there should be a worked out example.

I should remind you that SELECT FOR UPDATE requires conventional
UPDATE privilege (at least on columns appearing in the SELECT list).
So, among SELECT commands, SELECT FOR UPDATE is special in that it
requires UPDATE privilege. This is not true of equivalent RLS
policies, though.

So, as part of documenting the SELECT FOR UPDATE workaround, you're
going to have to advise admins that they need to have (lesser
privileged) user accounts with conventional UPDATE privilege on a
(USING qual referenced) table that they're most certainly not supposed
to be able to UPDATE at all (since they can totally undermine RLS with
such an UPDATE). RLS can make it impossible to actually proceed with
such an UPDATE, which makes the SELECT FOR UPDATE workaround possible,
but it's all pretty messy.

-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

I wrote:
> There's no real advantage to that anyway, compared with just doing
> stringToQualifiedNameList and then complaining if its list_length isn't 1.

Or just use SplitIdentifierString directly ...

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] Improved tab completion for FDW DDL

[David Fetter]

On Wed, Dec 30, 2015 at 01:21:06PM +0100, Andreas Karlsson wrote:
> Hi,
> 
> Here is a patch which adds the below missing tab completions for the FDW DDL
> commands. I noticed these were missing while playing around with FDWs a
> while ago.
> 
> "ALTER SERVER " with "RENAME TO"
> "CREATE SERVER  FOREIGN DATA WRAPPER" with fdw name
> "CREATE|ALTER USER MAPPING FOR  SERVER " with "OPTIONS ("
> 
> Another completion which is currently missing but I am not sure if we should
> add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING FOR", but
> since it might interfere with completing to username for "ALTER|DROP USER" I
> am not sure we want it. What do you think?

Is there a way to require some reasonable chunk--say, one that's
disambiguated from the name any known ROLE with LOGIN--of MAPPING
before completing with MAPPING FOR?  I confess to not knowing how the
new system works in enough detail to know that off the top of my head.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

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


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

[HACKERS] row_security GUC does not behave as documented

[Tom Lane]

The fine manual says that when row_security is set to off, "queries fail
which would otherwise apply at least one policy".  However, a look at
check_enable_rls() says that that is a true statement only when the user
is not table owner.  If the user *is* table owner, turning off
row_security seems to amount to just silently disabling RLS, even for
tables with FORCE ROW LEVEL SECURITY.

I am not sure if this is a documentation bug or a code bug, but it
sure looks to be one or the other.

Meanwhile, there's a statement about row_security in ddl.sgml that is so
vague as to be nearly meaningless, but it doesn't seem to quite match
either of those interpretations.  I'm in the midst of copy-editing that
section and will make it match what the code actually does at the moment,
but we'll have to change it again if this is deemed a code bug.

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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 6:00 PM, Stephen Frost  wrote:
>> I'm not sure what you mean. The CREATE POLICY changes in commit
>> 43cd468cf01007f3 specifically call out the issue illustrated in my
>> example test case. There are some other changes made in that commit,
>> but they don't seem to be attempting to address this specific problem.
>> They also seem fine.
>
>
> I believe Tom's complaint was that the overall page is about CREATE POLICY,
> technically, and that the text in attempting to address the concern might be
> taken under the context of being a CREATE POLICY issue rather than a general
> RLS issue with row locking.

Really? But the problem happens as a consequence of having a
subqueries within CREATE POLICY's USING quals (as well as a
misunderstanding made by the admin about just what is possible).

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

[HACKERS] Description tweak for vacuumdb

[Ian Barwick]

Hi

Like the docs say, vacuumdb is a "wrapper around the SQL command VACUUM"
which I used to use in dim-and-distant pre-autovacuum days came for cronjobs,
but until messing around with pg_upgrade recently I hadn't really had much
use for it. Anyway, the docs also say "There is no effective difference
between vacuuming and analyzing databases via this utility and via other
methods for accessing the server", which IMHO seems a bit out-of-date as
it now does two things which you can't do directly via e.g. psql:

- generate statistics in stages (9.4)
- parallel vacuum (9.5)

Attached patches (for 9.4 and 9.5/HEAD) update the description to make
clear that it now does a bit more than just execute a single command.


Regards

Ian Barwick


-- 
 Ian Barwick   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
new file mode 100644
index 3ecd999..35e3d6f
*** a/doc/src/sgml/ref/vacuumdb.sgml
--- b/doc/src/sgml/ref/vacuumdb.sgml
*** PostgreSQL documentation
*** 65,71 
 command .
 There is no effective difference between vacuuming and analyzing
 databases via this utility and via other methods for accessing the
!server.

  
   
--- 65,73 
 command .
 There is no effective difference between vacuuming and analyzing
 databases via this utility and via other methods for accessing the
!server. However it does provide additional functionality for generating
!statistics in stages, which is useful when optimizing a newly populated
!database.

  
   
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
new file mode 100644
index 92b8984..d640887
*** a/doc/src/sgml/ref/vacuumdb.sgml
--- b/doc/src/sgml/ref/vacuumdb.sgml
*** PostgreSQL documentation
*** 65,71 
 command .
 There is no effective difference between vacuuming and analyzing
 databases via this utility and via other methods for accessing the
!server.

  
   
--- 65,73 
 command .
 There is no effective difference between vacuuming and analyzing
 databases via this utility and via other methods for accessing the
!server. However it does provide additional functionality for generating
!statistics in stages, which is useful when optimizing a newly populated
!database, and for executing vacuum or analyze commands in parallel.

  
   

-- 
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] Broken lock management in policy.c.

[Tom Lane]

Peter Geoghegan  writes:
> On Sun, Jan 3, 2016 at 5:18 PM, Tom Lane  wrote:
>> In any case, the text in create_policy.sgml seems to be a misleading
>> description of the problem, as it's talking about DDL modifications
>> which are *not* what's happening here.

> I'm not sure what you mean. The CREATE POLICY changes in commit
> 43cd468cf01007f3 specifically call out the issue illustrated in my
> example test case. There are some other changes made in that commit,
> but they don't seem to be attempting to address this specific problem.
> They also seem fine.

I'm going to state it as an incontrovertible fact that that paragraph
is so vague as to be useless, because I sure misunderstood it, and in
fact just copy-edited it to talk about changes to RLS policies.  I now
see that it did say "relations referenced by RLS policies", but that
distinction is going to escape just about everybody who does not already
know what effects are being obliquely referred to.

I think we should get rid of it altogether (since I also agree with the
upthread comment that it's in the wrong place) and instead put an example
into section 5.7 saying directly that sub-selects, or in general
references to rows other than the one under test, are risky in RLS
policies.  We could also show the FOR UPDATE workaround there.

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] Broken lock management in policy.c.

[Stephen Frost]

Peter,

On Sunday, January 3, 2016, Peter Geoghegan  wrote:

> On Sun, Jan 3, 2016 at 6:09 PM, Peter Geoghegan  > wrote:
> >> I think we should get rid of it altogether (since I also agree with the
> >> upthread comment that it's in the wrong place) and instead put an
> example
> >> into section 5.7 saying directly that sub-selects, or in general
> >> references to rows other than the one under test, are risky in RLS
> >> policies.  We could also show the FOR UPDATE workaround there.
> >
> > I agree that there should be a worked out example.
>
> I should remind you that SELECT FOR UPDATE requires conventional
> UPDATE privilege (at least on columns appearing in the SELECT list).
> So, among SELECT commands, SELECT FOR UPDATE is special in that it
> requires UPDATE privilege. This is not true of equivalent RLS
> policies, though.
>
> So, as part of documenting the SELECT FOR UPDATE workaround, you're
> going to have to advise admins that they need to have (lesser
> privileged) user accounts with conventional UPDATE privilege on a
> (USING qual referenced) table that they're most certainly not supposed
> to be able to UPDATE at all (since they can totally undermine RLS with
> such an UPDATE). RLS can make it impossible to actually proceed with
> such an UPDATE, which makes the SELECT FOR UPDATE workaround possible,
> but it's all pretty messy.
>

A security defined function could be used to address that, of course. That
could have performance implications, naturally.

Thanks,

Stephen

Re: [HACKERS] Broken lock management in policy.c.

[Stephen Frost]

Tom, Peter,

On Sunday, January 3, 2016, Peter Geoghegan  wrote:

> On Sun, Jan 3, 2016 at 4:32 PM, Tom Lane >
> wrote:
> > CREATE POLICY takes AccessExclusiveLock on the table a policy is being
> > added to -- good -- and then releases it when done -- bad.  Correct
> > behavior is to hold the lock till commit, because otherwise there is
> > no guarantee that other backends will see the updated catalog rows in
> > time, or indeed at all.
> >
> > The same goes for ALTER POLICY, and possibly DROP POLICY (I've not
> > found the implementation of that ...)
>
> Right.
>
> > If we fix this, I believe we could also remove the weasel wording that
> was
> > added to create_policy.sgml in commit 43cd468cf01007f3 about how the
> > system might transiently fail to enforce row security correctly.
>
> IIUC, then what you say here isn't true, because that issue was about
> a transient failure without the involvement of *any* DDL from start to
> finish. CREATE POLICY accepts subqueries referencing other relations
> in its USING quals. This seems to be idiomatic usage of CREATE POLICY,
> in fact.
>
> See my original isolation tester test case, where only the setup involves
> DDL:
>
>
> http://www.postgresql.org/message-id/attachment/38467/0001-RLS-isolation-test.patch
>

Further, as mentioned in the discussion of that issue- it also can apply to
updatable security barrier views. It's not specific to RLS.

Thanks,

Stephen

Re: [HACKERS] row_security GUC does not behave as documented

[Alvaro Herrera]

Tom Lane wrote:
> Stephen Frost  writes:
> > On Sunday, January 3, 2016, Tom Lane  wrote:
> >> The fine manual says that when row_security is set to off, "queries fail
> >> which would otherwise apply at least one policy".  However, a look at
> >> check_enable_rls() says that that is a true statement only when the user
> >> is not table owner.  If the user *is* table owner, turning off
> >> row_security seems to amount to just silently disabling RLS, even for
> >> tables with FORCE ROW LEVEL SECURITY.
> >> 
> >> I am not sure if this is a documentation bug or a code bug, but it
> >> sure looks to be one or the other.
> 
> > The original reason for changing how row_security works was to avoid a
> > change in behavior based on a GUC changing. As such, I'm thinking that has
> > to be a code bug, as otherwise it would be a behavior change due to a GUC
> > being changed in the FORCE RLS case for table owners.
> 
> Well, I tried changing the code to act the way I gather it should, and
> it breaks a whole bunch of regression test cases.  See attached.

I think this means we need to postpone 9.5.0 for a week.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] row_security GUC does not behave as documented

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > On Sunday, January 3, 2016, Tom Lane  wrote:
> >> The fine manual says that when row_security is set to off, "queries fail
> >> which would otherwise apply at least one policy".  However, a look at
> >> check_enable_rls() says that that is a true statement only when the user
> >> is not table owner.  If the user *is* table owner, turning off
> >> row_security seems to amount to just silently disabling RLS, even for
> >> tables with FORCE ROW LEVEL SECURITY.
> >> 
> >> I am not sure if this is a documentation bug or a code bug, but it
> >> sure looks to be one or the other.
> 
> > The original reason for changing how row_security works was to avoid a
> > change in behavior based on a GUC changing. As such, I'm thinking that has
> > to be a code bug, as otherwise it would be a behavior change due to a GUC
> > being changed in the FORCE RLS case for table owners.
> 
> Well, I tried changing the code to act the way I gather it should, and
> it breaks a whole bunch of regression test cases.  See attached.

Right, I wrote the code that way originally thinking that it didn't make
sense to throw a permission denied error when it's the owner, but I
hadn't been thinking about, at that time, how we don't want the GUC to
result in a behavior change.

As we don't want to end up with the same behavior-change-due-to-GUC that
we had with the original row_security implementation, we should change
the code as your patch does and update the regression tests accordingly.
Perhaps the error code thrown could be tailored a bit when it's the
owner, to indicate that FORCE RLS has been set on the table, but I'm not
sure it's really a big deal either way.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

I didn't see this discussed on the thread...

regrole and regnamespace don't run their output through quote_ident(). 
That's contrary to all the other reg* operators.


Worse, they also don't *allow* quoted input. Not only is that different 
from reg*, it's the *opposite*:


select 'with spaces'::regclass;
ERROR:  invalid name syntax
LINE 1: select 'with spaces'::regclass;
select '"with spaces"'::regclass;
   regclass
---
 "with spaces"
(1 row)

I think this needs to be fixed before 9.5 releases. :(
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Jim Nasby  writes:
> regrole and regnamespace don't run their output through quote_ident(). 
> That's contrary to all the other reg* operators.
> Worse, they also don't *allow* quoted input. Not only is that different 
> from reg*, it's the *opposite*:

BTW, there's a concrete reason why this is broken, which is that although
regrole and regnamespace didn't bother with copying quoting/dequoting from
the other types, they *did* copy the special case logic about allowing
and emitting numeric OIDs.  This means that an output like "1234" from
regroleout is formally inconsistent: there is no way to tell if it's an
numeric OID or a role name that happens to be all digits.  (With proper
quoting logic, you could tell because an all-digits role name would have
gotten quoted.)  Conversely, if you create an all-digits role name, there
is no way to get regrolein to interpret it as such, whereas a dequoting
rule would have disambiguated.

I'm inclined to leave to_regrole and to_regnamespace alone, though, since
they have no numeric-OID path, and they will provide an "out" for anyone
who wants to handle nonquoted names.  (Though at least in HEAD we ought to
fix them to take type text as input.  Using cstring for ordinary functions
is just sloppy.)

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] row_security GUC does not behave as documented

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Well, I tried changing the code to act the way I gather it should, and
>> it breaks a whole bunch of regression test cases.  See attached.

> I think this means we need to postpone 9.5.0 for a week.

I think the regression cases are not that big a deal to fix, but
I'd rather that the original authors did it, as I might miss what
they intended to test.

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] pg_upgrade in 9.5 broken for adminpack

[Andreas Seltenreich]

Bruce Momjian writes:

> On Thu, Apr 16, 2015 at 11:29:07PM -0700, Jeff Janes wrote:
>> Of course after sending that it became obvious.  The C function is not 
>> getting
>> called because the SQL function is marked as being strict, yet is called with
>> NULL arguments.
>> 
>> Trivial patch attached to unset strict flag in pg_proc.h.
>> 
>> But  CATALOG_VERSION_NO probably needs another bump as well.
>
> Patch applied and catversion bumped.  Thanks.

Shouldn't there be some validation of arguments now that the function is
no longer marked strict?  Currently, unprivileged users can crash the
server calling binary_upgrade_create_empty_extension with null
arguments.  Found using sqlsmith.

regards,
Andreas


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

Re: [HACKERS] pg_upgrade in 9.5 broken for adminpack

[Tom Lane]

Andreas Seltenreich  writes:
> Shouldn't there be some validation of arguments now that the function is
> no longer marked strict?  Currently, unprivileged users can crash the
> server calling binary_upgrade_create_empty_extension with null
> arguments.  Found using sqlsmith.

Good catch, thanks!  Will fix.

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] Broken lock management in policy.c.

[Stephen Frost]

Tom,

On Sunday, January 3, 2016, Tom Lane  wrote:

> CREATE POLICY takes AccessExclusiveLock on the table a policy is being
> added to -- good -- and then releases it when done -- bad.  Correct
> behavior is to hold the lock till commit, because otherwise there is
> no guarantee that other backends will see the updated catalog rows in
> time, or indeed at all.


Agreed.


> The same goes for ALTER POLICY, and possibly DROP POLICY (I've not
> found the implementation of that ...)


DROP POLICY is handled through the generalized drop command and likely
doesn't have a problem due to that.


> If we fix this, I believe we could also remove the weasel wording that was
> added to create_policy.sgml in commit 43cd468cf01007f3 about how the
> system might transiently fail to enforce row security correctly.
>

The issue addressed there is with row locking, not table level locks. The
concern is that a user could acquire a lock on a row to which their access
to is then removed due to changes in rows which are used by the policy on
the table- not any changes to the policy definition itself. The row that is
locked might then be updated by the user who removed access to the row and
the results of that update seen by the original user via RETURNING.

Peter provided a test case which illustrated the concern.

http://www.postgresql.org/message-id/flat/20150803220725.gb3...@tamriel.snowman.net

Apologies if the above isn't entirely accurate, on my phone at the moment.

Thanks!

Stephen

Re: [HACKERS] Broken lock management in policy.c.

[Stephen Frost]

Tom,

On Sunday, January 3, 2016, Tom Lane  wrote:

> Peter Geoghegan > writes:
> > On Sun, Jan 3, 2016 at 4:32 PM, Tom Lane  > wrote:
> >> If we fix this, I believe we could also remove the weasel wording that
> was
> >> added to create_policy.sgml in commit 43cd468cf01007f3 about how the
> >> system might transiently fail to enforce row security correctly.
>
> > IIUC, then what you say here isn't true, because that issue was about
> > a transient failure without the involvement of *any* DDL from start to
> > finish. CREATE POLICY accepts subqueries referencing other relations
> > in its USING quals. This seems to be idiomatic usage of CREATE POLICY,
> > in fact.
>
> > See my original isolation tester test case, where only the setup
> involves DDL:
>
> >
> http://www.postgresql.org/message-id/attachment/38467/0001-RLS-isolation-test.patch
>
> Hmm.  I agree that this test case's behavior does not depend on CREATE
> POLICY's lock mismanagement.  I think what is going on here is that the
> RLS quals are being checked with an older snapshot than what controls
> the output of the UPDATE RETURNING.  Even the EPQ recheck that's done
> after getting update lock on the "information" row doesn't fix it,
> because we *don't* insist on taking an update lock on the "users" table,
> so we don't see the new value of that row.
>
> If that diagnosis is correct, you could fix it by changing the RLS
> policies' sub-selects to use SELECT FOR UPDATE, though the loss of
> concurrency might well be unacceptable.
>
> In any case, the text in create_policy.sgml seems to be a misleading
> description of the problem, as it's talking about DDL modifications
> which are *not* what's happening here.


There was some debate about the right place for that discussion as there
didn't seem to be any particularly ideal location. I had been intending to
have a locking section in the RLS section rework.

Thanks,

Stephen

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Michael Paquier  writes:
> On Mon, Jan 4, 2016 at 11:16 AM, Tom Lane  wrote:
>> I'm inclined to think that you're right, and that this is something that
>> ought to be changed.  It's not quite too late ...

> Well, I can send a patch with some tests if need be... Tom, I guess
> that you are already on it?

If you're already working on it, feel free to continue.

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] count_nulls(VARIADIC "any")

[Pavel Stehule]

2016-01-03 21:37 GMT+01:00 Pavel Stehule :

> Hi
>
>
> 2015-08-12 19:18 GMT+02:00 Marko Tiikkaja :
>
>> Hi,
>>
>> I'd like to suggest $SUBJECT for inclusion in Postgres 9.6.  I'm sure
>> everyone would've found it useful at some point in their lives, and the
>> fact that it can't be properly implemented in any language other than C I
>> think speaks for the fact that we as a project should provide it.
>>
>> A quick and dirty proof of concept (patch attached):
>>
>> =# select count_nulls(null::int, null::text, 17, 'bar');
>>  count_nulls
>> -
>>2
>> (1 row)
>>
>> Its natural habitat would be CHECK constraints, e.g:
>>
>>   CHECK (count_nulls(a,b,c) IN (0, 3))
>>
>> Will finish this up for the next CF, unless someone wants to tell me how
>> stupid this idea is before that.
>>
>>
>> .m
>>
>
> I am sending updated version - support num_nulls and num_notnulls
>

and patch


>
> Regards
>
> Pavel
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 8ef9fce..fd7890e
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 182,188 

  

!Comparison Operators
  
 
  comparison
--- 182,188 

  

!Comparison Functions and Operators
  
 
  comparison
***
*** 191,200 
  
 
  The usual comparison operators are available, shown in .
 
  
!
  Comparison Operators
  
   
--- 191,200 
  
 
  The usual comparison operators are available, shown in .
 
  
!
  Comparison Operators
  
   
***
*** 437,442 
--- 437,479 
 
  -->
  
+   
+ Comparison Functions
+ 
+  
+   
+Function
+Description
+Example
+Example Result
+   
+  
+  
+   
+
+  
+   num_notnulls
+  
+  num_notnulls(VARIADIC "any")
+
+Returns the number of not NULL input arguments
+num_nulls(1, NULL, 2)
+2
+   
+   
+
+  
+   num_nulls
+  
+  num_nulls(VARIADIC "any")
+
+Returns the number of NULL input arguments
+num_nulls(1, NULL, 2)
+1
+   
+  
+ 
+

  

*** table2-mapping
*** 10307,10313 


 The standard comparison operators shown in   are available for
 jsonb, but not for json. They follow the
 ordering rules for B-tree operations outlined at .
--- 10344,10350 


 The standard comparison operators shown in   are available for
 jsonb, but not for json. They follow the
 ordering rules for B-tree operations outlined at .
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
new file mode 100644
index 6a306f3..873d8f6
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 43,48 
--- 43,155 
  
  #define atooid(x)  ((Oid) strtoul((x), NULL, 10))
  
+ /*
+  * Collect info about NULL arguments. Returns true when result values
+  * are valid.
+  */
+ static bool
+ count_nulls(FunctionCallInfo fcinfo,
+ int32 *nargs, int32 *nulls)
+ {
+ 	int32 count = 0;
+ 	int i;
+ 
+ 	if (get_fn_expr_variadic(fcinfo->flinfo))
+ 	{
+ 		ArrayType  *arr;
+ 		int ndims, nitems, *dims;
+ 		bits8 *bitmap;
+ 		int bitmask;
+ 
+ 		/* num_nulls(VARIADIC NULL) is defined as NULL */
+ 		if (PG_ARGISNULL(0))
+ 			return false;
+ 
+ 		/*
+ 		 * Non-null argument had better be an array.  We assume that any call
+ 		 * context that could let get_fn_expr_variadic return true will have
+ 		 * checked that a VARIADIC-labeled parameter actually is an array.  So
+ 		 * it should be okay to just Assert that it's an array rather than
+ 		 * doing a full-fledged error check.
+ 		 */
+ 		Assert(OidIsValid(get_base_element_type(get_fn_expr_argtype(fcinfo->flinfo, 0;
+ 
+ 		/* OK, safe to fetch the array value */
+ 		arr = PG_GETARG_ARRAYTYPE_P(0);
+ 
+ 		ndims = ARR_NDIM(arr);
+ 		dims = ARR_DIMS(arr);
+ 		nitems = ArrayGetNItems(ndims, dims);
+ 
+ 		bitmap = ARR_NULLBITMAP(arr);
+ 		if (bitmap)
+ 		{
+ 			bitmask = 1;
+ 
+ 			for (i = 0; i < nitems; i++)
+ 			{
+ if ((*bitmap & bitmask) == 0)
+ 	count++;
+ 
+ bitmask <<= 1;
+ if (bitmask == 0x100)
+ {
+ 	bitmap++;
+ 	bitmask = 1;
+ }
+ 			}
+ 		}
+ 
+ 		*nargs = nitems;
+ 		*nulls = count;
+ 	}
+ 	else
+ 	{
+ 		for (i = 0; i < PG_NARGS(); i++)
+ 		{
+ 			if (PG_ARGISNULL(i))
+ count++;
+ 		}
+ 
+ 		*nargs = PG_NARGS();
+ 		*nulls = count;
+ 	}
+ 
+ 	return true;
+ }
+ 
+ /*
+  * num_nulls()
+  *  Count the number of NULL input arguments
+  */
+ Datum
+ pg_num_nulls(PG_FUNCTION_ARGS)
+ {
+ 	int32 nargs,
+ 		nulls;
+ 
+ 	if (!count_nulls(fcinfo, , ))
+ 		PG_RETURN_NULL();
+ 
+ 	PG_RETURN_INT32(nulls);
+ }
+ 
+ /*
+  * num_notnulls()
+  *  Count the number of not NULL input arguments
+  */
+ 

Re: [HACKERS] count_nulls(VARIADIC "any")

[Pavel Stehule]

Hi

2015-08-12 19:18 GMT+02:00 Marko Tiikkaja :

> Hi,
>
> I'd like to suggest $SUBJECT for inclusion in Postgres 9.6.  I'm sure
> everyone would've found it useful at some point in their lives, and the
> fact that it can't be properly implemented in any language other than C I
> think speaks for the fact that we as a project should provide it.
>
> A quick and dirty proof of concept (patch attached):
>
> =# select count_nulls(null::int, null::text, 17, 'bar');
>  count_nulls
> -
>2
> (1 row)
>
> Its natural habitat would be CHECK constraints, e.g:
>
>   CHECK (count_nulls(a,b,c) IN (0, 3))
>
> Will finish this up for the next CF, unless someone wants to tell me how
> stupid this idea is before that.
>
>
> .m
>

I am sending updated version - support num_nulls and num_notnulls

Regards

Pavel

[HACKERS] Very confusing installcheck behavior with PGXS

[Jim Nasby]

The rule that gets executed if you do `make installcheck` with something 
using PGXS is


pgxs.mk:$(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS)

where $(pg_regress_installcheck) is set in Makefile.global.in to


pg_regress_installcheck = $(top_builddir)/src/test/regress/pg_regress 
--inputdir=$(srcdir) --bindir='$(bindir)' $(pg_regress_locale_flags) 
$(EXTRA_REGRESS_OPTS)


The problem here is that in a PGXS make, srcdir is set to '.'[1], and 
--inputdir is specified a second time in REGRESS_OPTS. Normally that 
works OK (for some reason ignoring what's in ./sql), but if you happen 
to have a file in your test/sql directory that matches a file in ./sql, 
pg_regress runs the first file and not the second.


Presumably that's exactly what you'd want in most of the tree, but it's 
definitely not what you want in an extension.


Is the best way to fix this to add a pg_regress_installcheck_pgxs 
variable in Makefile.global.in and modify pgxs.mk accordingly?


[1]:

decibel@decina:[16:18]~/git/trunklet (master=)$make 
print-pg_regress_installcheck print-REGRESS_OPTS print-REGRESS
pg_regress_installcheck = 
/Users/decibel/pgsql/9.4/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress
 --inputdir=./ --psqldir=/Users/decibel/pgsql/9.4/i/bin
REGRESS_OPTS = --inputdir=test --load-language=plpgsql 
--dbname=contrib_regression
REGRESS = all build

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] row_security GUC does not behave as documented

[Stephen Frost]

Tom,

On Sunday, January 3, 2016, Tom Lane  wrote:

> The fine manual says that when row_security is set to off, "queries fail
> which would otherwise apply at least one policy".  However, a look at
> check_enable_rls() says that that is a true statement only when the user
> is not table owner.  If the user *is* table owner, turning off
> row_security seems to amount to just silently disabling RLS, even for
> tables with FORCE ROW LEVEL SECURITY.
>
> I am not sure if this is a documentation bug or a code bug, but it
> sure looks to be one or the other.


The original reason for changing how row_security works was to avoid a
change in behavior based on a GUC changing. As such, I'm thinking that has
to be a code bug, as otherwise it would be a behavior change due to a GUC
being changed in the FORCE RLS case for table owners.

Thanks,

Stephen

Re: [HACKERS] Broken lock management in policy.c.

[Tom Lane]

Peter Geoghegan  writes:
> On Sun, Jan 3, 2016 at 4:32 PM, Tom Lane  wrote:
>> If we fix this, I believe we could also remove the weasel wording that was
>> added to create_policy.sgml in commit 43cd468cf01007f3 about how the
>> system might transiently fail to enforce row security correctly.

> IIUC, then what you say here isn't true, because that issue was about
> a transient failure without the involvement of *any* DDL from start to
> finish. CREATE POLICY accepts subqueries referencing other relations
> in its USING quals. This seems to be idiomatic usage of CREATE POLICY,
> in fact.

> See my original isolation tester test case, where only the setup involves DDL:

> http://www.postgresql.org/message-id/attachment/38467/0001-RLS-isolation-test.patch

Hmm.  I agree that this test case's behavior does not depend on CREATE
POLICY's lock mismanagement.  I think what is going on here is that the
RLS quals are being checked with an older snapshot than what controls
the output of the UPDATE RETURNING.  Even the EPQ recheck that's done
after getting update lock on the "information" row doesn't fix it,
because we *don't* insist on taking an update lock on the "users" table,
so we don't see the new value of that row.

If that diagnosis is correct, you could fix it by changing the RLS
policies' sub-selects to use SELECT FOR UPDATE, though the loss of
concurrency might well be unacceptable.

In any case, the text in create_policy.sgml seems to be a misleading
description of the problem, as it's talking about DDL modifications
which are *not* what's happening here.

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] Broken lock management in policy.c.

[Tom Lane]

Stephen Frost  writes:
> On Sunday, January 3, 2016, Tom Lane  wrote:
>> CREATE POLICY takes AccessExclusiveLock on the table a policy is being
>> added to -- good -- and then releases it when done -- bad.  Correct
>> behavior is to hold the lock till commit, because otherwise there is
>> no guarantee that other backends will see the updated catalog rows in
>> time, or indeed at all.

> Agreed.

On closer inspection, I'd misidentified the functions containing the
bad code --- it was really RemovePolicyById and RemoveRoleFromObjectPolicy
that were wrong.  Fix pushed.

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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 5:18 PM, Tom Lane  wrote:
> Hmm.  I agree that this test case's behavior does not depend on CREATE
> POLICY's lock mismanagement.  I think what is going on here is that the
> RLS quals are being checked with an older snapshot than what controls
> the output of the UPDATE RETURNING.  Even the EPQ recheck that's done
> after getting update lock on the "information" row doesn't fix it,
> because we *don't* insist on taking an update lock on the "users" table,
> so we don't see the new value of that row.

That's clearly what's going on in the example scenario: the EPQ
recheck walks the update chain, and uses a combination of a dirty
snapshot (for the target's scan tuple), and the MVCC snapshot (for
everything else). The scenario involves an attacker exploiting the
inconsistency, where the admin did not anticipate such an
inconsistency.

This seemed reasonably plausible to me, not least because READ
COMMITTED conflict handling is a mystery to the vast majority of
users.

> If that diagnosis is correct, you could fix it by changing the RLS
> policies' sub-selects to use SELECT FOR UPDATE, though the loss of
> concurrency might well be unacceptable.

That was one of the things that we talked about doing. Of course, that
would work because we walk the UPDATE chain to do EPQ recheck for
SELECT FOR UPDATE, just as with an UPDATE or a DELETE.

> In any case, the text in create_policy.sgml seems to be a misleading
> description of the problem, as it's talking about DDL modifications
> which are *not* what's happening here.

I'm not sure what you mean. The CREATE POLICY changes in commit
43cd468cf01007f3 specifically call out the issue illustrated in my
example test case. There are some other changes made in that commit,
but they don't seem to be attempting to address this specific problem.
They also seem fine.

-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Jim Nasby  writes:
> regrole and regnamespace don't run their output through quote_ident(). 
> That's contrary to all the other reg* operators.

> Worse, they also don't *allow* quoted input. Not only is that different 
> from reg*, it's the *opposite*:

I'm inclined to think that you're right, and that this is something that
ought to be changed.  It's not quite too late ...

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] count_nulls(VARIADIC "any")

[Jim Nasby]

On 1/3/16 2:37 PM, Pavel Stehule wrote:

+   /* num_nulls(VARIADIC NULL) is defined as NULL */
+   if (PG_ARGISNULL(0))
+   return false;


Could you add to the comment explaining why that's the desired behavior?


+   /*
+* Non-null argument had better be an array.  We assume that 
any call
+* context that could let get_fn_expr_variadic return true will 
have
+* checked that a VARIADIC-labeled parameter actually is an 
array.  So
+* it should be okay to just Assert that it's an array rather 
than
+* doing a full-fledged error check.
+*/
+   
Assert(OidIsValid(get_base_element_type(get_fn_expr_argtype(fcinfo->flinfo, 
0;


Erm... is that really the way to verify that what you have is an array? 
ISTM there should be a macro for that somewhere...



+   /* OK, safe to fetch the array value */
+   arr = PG_GETARG_ARRAYTYPE_P(0);
+
+   ndims = ARR_NDIM(arr);
+   dims = ARR_DIMS(arr);
+   nitems = ArrayGetNItems(ndims, dims);
+
+   bitmap = ARR_NULLBITMAP(arr);
+   if (bitmap)
+   {
+   bitmask = 1;
+
+   for (i = 0; i < nitems; i++)
+   {
+   if ((*bitmap & bitmask) == 0)
+   count++;
+
+   bitmask <<= 1;
+   if (bitmask == 0x100)
+   {
+   bitmap++;
+   bitmask = 1;


For brevity and example sake it'd probably be better to just use the 
normal iterator, unless there's a serious speed difference?


In the unit test, I'd personally prefer just building a table with the 
test cases and the expected NULL/NOT NULL results, at least for all the 
calls that would fit that paradigm. That should significantly reduce the 
size of the test. Not a huge deal though...


Also, I don't think anything is testing multiples of whatever value... 
how 'bout change the generate_series CASE statement to >40 instead of <>40?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] Broken lock management in policy.c.

[Stephen Frost]

Peter,

On Sunday, January 3, 2016, Peter Geoghegan  wrote:

> On Sun, Jan 3, 2016 at 5:18 PM, Tom Lane >
> wrote:
> > In any case, the text in create_policy.sgml seems to be a misleading
> > description of the problem, as it's talking about DDL modifications
> > which are *not* what's happening here.
>
> I'm not sure what you mean. The CREATE POLICY changes in commit
> 43cd468cf01007f3 specifically call out the issue illustrated in my
> example test case. There are some other changes made in that commit,
> but they don't seem to be attempting to address this specific problem.
> They also seem fine.
>

I believe Tom's complaint was that the overall page is about CREATE POLICY,
technically, and that the text in attempting to address the concern might
be taken under the context of being a CREATE POLICY issue rather than a
general RLS issue with row locking.

Thanks!

Stephen

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Jim Nasby  writes:
> On 1/3/16 9:23 PM, Tom Lane wrote:
>> I'm inclined to leave to_regrole and to_regnamespace alone, though, since
>> they have no numeric-OID path, and they will provide an "out" for anyone
>> who wants to handle nonquoted names.  (Though at least in HEAD we ought to
>> fix them to take type text as input.  Using cstring for ordinary functions
>> is just sloppy.)

> None of the other to_reg* casts do that though, so this would be 
> inconsistent.

Good point.  Also, anybody who really does want it like that can still
fall back to the traditional sub-SELECT-from-the-catalog approach.

> Another potential problem for regnamespace is that it doesn't allow an 
> entry for the catalog. I'm not sure what the spec says about that, but 
> every other function allows dbname.schema.blah (dbname == catalog).

Meh, these types conform to no spec, so we can make them do what we
like.  I see about zero reason to allow a catalog name, it would mostly
just confuse people.

> I started working on a fix, but it's currently blowing up in bootstrap 
> and I haven't been able to figure out why yet:
> running bootstrap script ... FATAL:  improper qualified name (too many 
> dotted names): oid_ops

Recommendation: don't muck with DeconstructQualifiedName.  That is called
in too many places and we are *not* touching any such API twenty-four
hours before release wrap.

There's no real advantage to that anyway, compared with just doing
stringToQualifiedNameList and then complaining if its list_length isn't 1.

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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 7:01 PM, Peter Geoghegan  wrote:
> I would also advise only referencing a single relation within the
> SELECT FOR UPDATE.

To state what may be obvious: We should recommend that SELECT FOR
SHARE appear in the CREATE POLICY USING qual as part of this
workaround (not SELECT FOR UPDATE), because there is no need for
anything stronger than that. We only need to prevent the admin
updating a referenced-in-using-qual tuple in a way that allows a
malicious user to exploit an inconsistency in tuple visibility during
EPQ rechec. (Using SELECT FOR KEY SHARE would not reliably workaround
the underlying issue, though.)

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

[HACKERS] Proposal for JSONB functions for internal representation casting insted text-casting

[Peter Krauss]

The usefulness of  ->>  operator is indisputable, but even with boolean or
numeric values, with good binary internal representation, it returns JSONB
value as text data type.

The simple *(myJSONB->>'myField')::expectedType* is not enough because:

1) there are no internal optimization,  need two-step casting, first
bynary-to-text, them text-to-expectedType.

2) if expectedType is not the expected (in the associated jsonb_typeof),
generates an error... The ideal "return NULL" convention is not easy to
implement with usual casting.

More details and some examples at
   http://stackoverflow.com/q/34579758/287948

- - - -
CONTEXT OF USEFULNESS

As section "8.14. JSON Types" in the pg9.4 guide,
"Representing data as JSON can be considerably more flexible (...) is quite
possible for both approaches to co-exist and complement each other (...)
However, even for applications where maximal flexibility is desired, it is
still recommended that JSON documents have a somewhat fixed structure".

The proposed casting functions of JSONB is a part of "predictable but fluid
structure" demands in JSON representation, and makes it easier to write
queries that mix usual data types with JSONB.

- - - -
Formal requeriment  for
a C implementation below

CREATE FUNCTION jbval_to_numeric(JSONB, varchar) RETURNS numeric AS $f$
  SELECT CASE
WHEN jsonb_typeof($1->$2)='number' THEN ($1->>$2)::numeric
ELSE NULL::numeric
  END;$f$ LANGUAGE sql IMMUTABLE;
CREATE FUNCTION jbval_to_float(JSONB, varchar) RETURNS float AS $f$
  SELECT CASE
WHEN jsonb_typeof($1->$2)='number' THEN ($1->>$2)::float
ELSE NULL::float
  END;$f$ LANGUAGE sql IMMUTABLE;
CREATE FUNCTION jbval_to_int(JSONB, varchar, boolean DEFAULT true)
RETURNS int AS $f$
  SELECT CASE
WHEN jsonb_typeof($1->$2)='number' THEN
   CASE WHEN $3 THEN ($1->>$2)::int ELSE ($1->>$2)::float::int END
ELSE NULL::int
  END;$f$ LANGUAGE sql IMMUTABLE;
CREATE FUNCTION jbval_to_boolean(JSONB, varchar) RETURNS boolean AS $f$
  SELECT CASE
WHEN jsonb_typeof($1->$2)='boolean' THEN ($1->>$2)::boolean
ELSE NULL::boolean
  END;$f$ LANGUAGE sql IMMUTABLE;

[HACKERS] PGCon 2016 call for papers

[Dan Langille]

In case you've overlooked it, you have about two weeks to submit your proposal.

PGCon 2016 will be on 17-21 May 2016 at University of Ottawa.

* 17-18 (Tue-Wed) tutorials
* 19 & 20 (Thu-Fri) talks - the main part of the conference
* 17 & 21 (Wed & Sat) The Developer Unconference & the User Unconference (both 
very popular)

PLEASE NOTE: PGCon 2016 is in May.

See http://www.pgcon.org/2016/

We are now accepting proposals for the main part of the conference (19-20 May).
Proposals can be quite simple. We do not require academic-style papers.

If you are doing something interesting with PostgreSQL, please submit
a proposal.  You might be one of the backend hackers or work on a
PostgreSQL related project and want to share your know-how with
others. You might be developing an interesting system using PostgreSQL
as the foundation. Perhaps you migrated from another database to
PostgreSQL and would like to share details.  These, and other stories
are welcome. Both users and developers are encouraged to share their
experiences.

Here are a some ideas to jump start your proposal process:

- novel ways in which PostgreSQL is used
- migration of production systems from another database
- data warehousing
- tuning PostgreSQL for different work loads
- replication and clustering
- hacking the PostgreSQL code
- PostgreSQL derivatives and forks
- applications built around PostgreSQL
- benchmarking and performance engineering
- case studies
- location-aware and mapping software with PostGIS
- The latest PostgreSQL features and features in development
- research and teaching with PostgreSQL
- things the PostgreSQL project could do better
- integrating PostgreSQL with 3rd-party software

Both users and developers are encouraged to share their experiences.

The schedule is:

1 Dec 2015 Proposal acceptance begins
19 Jan 2016 Proposal acceptance ends
19 Feb 2016 Confirmation of accepted proposals

NOTE: the call for lightning talks will go out very close to the conference.
Do not submit lightning talks proposals until then.

See also 

Instructions for submitting a proposal to PGCon 2016 are available
from: 



signature.asc
Description: Message signed with OpenPGP using GPGMail

[HACKERS] Broken lock management in policy.c.

[Tom Lane]

CREATE POLICY takes AccessExclusiveLock on the table a policy is being
added to -- good -- and then releases it when done -- bad.  Correct
behavior is to hold the lock till commit, because otherwise there is
no guarantee that other backends will see the updated catalog rows in
time, or indeed at all.

The same goes for ALTER POLICY, and possibly DROP POLICY (I've not
found the implementation of that ...)

If we fix this, I believe we could also remove the weasel wording that was
added to create_policy.sgml in commit 43cd468cf01007f3 about how the
system might transiently fail to enforce row security correctly.

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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 6:09 PM, Tom Lane  wrote:
>> Really? But the problem happens as a consequence of having a
>> subqueries within CREATE POLICY's USING quals
>
> If that's what we're talking about, let's say it in precisely that many
> words.  With an example.  The current text is 100% useless.

I agree that the text was unclear, and that that should be fixed,
because it's too complicated to expect anyone to understand this
without an example (indeed, that's why I used isolationtester to
explain the issue). My confusion was only about whether it was
understood that Stephen had fulfilled my request to document this
behavior.

-- 
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] row_security GUC does not behave as documented

[Tom Lane]

Stephen Frost  writes:
> On Sunday, January 3, 2016, Tom Lane  wrote:
>> The fine manual says that when row_security is set to off, "queries fail
>> which would otherwise apply at least one policy".  However, a look at
>> check_enable_rls() says that that is a true statement only when the user
>> is not table owner.  If the user *is* table owner, turning off
>> row_security seems to amount to just silently disabling RLS, even for
>> tables with FORCE ROW LEVEL SECURITY.
>> 
>> I am not sure if this is a documentation bug or a code bug, but it
>> sure looks to be one or the other.

> The original reason for changing how row_security works was to avoid a
> change in behavior based on a GUC changing. As such, I'm thinking that has
> to be a code bug, as otherwise it would be a behavior change due to a GUC
> being changed in the FORCE RLS case for table owners.

Well, I tried changing the code to act the way I gather it should, and
it breaks a whole bunch of regression test cases.  See attached.

regards, tom lane

diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index b6c1d75..4e7b4d1 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
*** check_enable_rls(Oid relid, Oid checkAsU
*** 59,67 
  	Oid			user_id = checkAsUser ? checkAsUser : GetUserId();
  
  	/* Nothing to do for built-in relations */
! 	if (relid < FirstNormalObjectId)
  		return RLS_NONE;
  
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
  	if (!HeapTupleIsValid(tuple))
  		return RLS_NONE;
--- 59,68 
  	Oid			user_id = checkAsUser ? checkAsUser : GetUserId();
  
  	/* Nothing to do for built-in relations */
! 	if (relid < (Oid) FirstNormalObjectId)
  		return RLS_NONE;
  
+ 	/* Fetch relation's relrowsecurity and relforcerowsecurity flags */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
  	if (!HeapTupleIsValid(tuple))
  		return RLS_NONE;
*** check_enable_rls(Oid relid, Oid checkAsU
*** 88,96 
  		return RLS_NONE_ENV;
  
  	/*
! 	 * Table owners generally bypass RLS, except if row_security=true and the
! 	 * table has been set (by an owner) to FORCE ROW SECURITY, and this is not
! 	 * a referential integrity check.
  	 *
  	 * Return RLS_NONE_ENV to indicate that this decision depends on the
  	 * environment (in this case, the user_id).
--- 89,97 
  		return RLS_NONE_ENV;
  
  	/*
! 	 * Table owners generally bypass RLS, except if the table has been set (by
! 	 * an owner) to FORCE ROW SECURITY, and this is not a referential
! 	 * integrity check.
  	 *
  	 * Return RLS_NONE_ENV to indicate that this decision depends on the
  	 * environment (in this case, the user_id).
*** check_enable_rls(Oid relid, Oid checkAsU
*** 98,128 
  	if (pg_class_ownercheck(relid, user_id))
  	{
  		/*
! 		 * If row_security=true and FORCE ROW LEVEL SECURITY has been set on
! 		 * the relation then we return RLS_ENABLED to indicate that RLS should
! 		 * still be applied.  If we are in a SECURITY_NOFORCE_RLS context or if
! 		 * row_security=false then we return RLS_NONE_ENV.
  		 *
! 		 * The SECURITY_NOFORCE_RLS indicates that we should not apply RLS even
! 		 * if the table has FORCE RLS set- IF the current user is the owner.
! 		 * This is specifically to ensure that referential integrity checks are
! 		 * able to still run correctly.
  		 *
  		 * This is intentionally only done after we have checked that the user
  		 * is the table owner, which should always be the case for referential
  		 * integrity checks.
  		 */
! 		if (row_security && relforcerowsecurity && !InNoForceRLSOperation())
! 			return RLS_ENABLED;
! 		else
  			return RLS_NONE_ENV;
  	}
  
! 	/* row_security GUC says to bypass RLS, but user lacks permission */
  	if (!row_security && !noError)
  		ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!  errmsg("insufficient privilege to bypass row-level security")));
  
  	/* RLS should be fully enabled for this relation. */
  	return RLS_ENABLED;
--- 99,130 
  	if (pg_class_ownercheck(relid, user_id))
  	{
  		/*
! 		 * If FORCE ROW LEVEL SECURITY has been set on the relation then we
! 		 * should return RLS_ENABLED to indicate that RLS should be applied.
! 		 * If not, or if we are in an InNoForceRLSOperation context, we return
! 		 * RLS_NONE_ENV.
  		 *
! 		 * InNoForceRLSOperation indicates that we should not apply RLS even
! 		 * if the table has FORCE RLS set - IF the current user is the owner.
! 		 * This is specifically to ensure that referential integrity checks
! 		 * are able to still run correctly.
  		 *
  		 * This is intentionally only done after we have checked that the user
  		 * is the table owner, which should always be the case for referential
  		 * integrity checks.
  		 */
! 		if (!relforcerowsecurity || InNoForceRLSOperation())
  			return RLS_NONE_ENV;
  	}
  
! 	/*
! 	 * We should apply RLS.  

Re: [HACKERS] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 6:41 PM, Stephen Frost  wrote:
> A security defined function could be used to address that, of course. That
> could have performance implications, naturally.

True.

I would also advise only referencing a single relation within the
SELECT FOR UPDATE.

-- 
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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 4:32 PM, Tom Lane  wrote:
> CREATE POLICY takes AccessExclusiveLock on the table a policy is being
> added to -- good -- and then releases it when done -- bad.  Correct
> behavior is to hold the lock till commit, because otherwise there is
> no guarantee that other backends will see the updated catalog rows in
> time, or indeed at all.
>
> The same goes for ALTER POLICY, and possibly DROP POLICY (I've not
> found the implementation of that ...)

Right.

> If we fix this, I believe we could also remove the weasel wording that was
> added to create_policy.sgml in commit 43cd468cf01007f3 about how the
> system might transiently fail to enforce row security correctly.

IIUC, then what you say here isn't true, because that issue was about
a transient failure without the involvement of *any* DDL from start to
finish. CREATE POLICY accepts subqueries referencing other relations
in its USING quals. This seems to be idiomatic usage of CREATE POLICY,
in fact.

See my original isolation tester test case, where only the setup involves DDL:

http://www.postgresql.org/message-id/attachment/38467/0001-RLS-isolation-test.patch

-- 
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] Broken lock management in policy.c.

[Peter Geoghegan]

On Sun, Jan 3, 2016 at 6:04 PM, Tom Lane  wrote:
> I'm going to state it as an incontrovertible fact that that paragraph
> is so vague as to be useless, because I sure misunderstood it, and in
> fact just copy-edited it to talk about changes to RLS policies.  I now
> see that it did say "relations referenced by RLS policies", but that
> distinction is going to escape just about everybody who does not already
> know what effects are being obliquely referred to.

That's fair.

> I think we should get rid of it altogether (since I also agree with the
> upthread comment that it's in the wrong place) and instead put an example
> into section 5.7 saying directly that sub-selects, or in general
> references to rows other than the one under test, are risky in RLS
> policies.  We could also show the FOR UPDATE workaround there.

I agree that there should be a worked out example. After all, EPQ is a
behavior that is peculiar to Postgres, and the problem hinges on EPQ
being how READ COMMITTED conflicts are handled. An equivalent RLS
feature in any other database system (including other MVCC systems)
would naturally not have this problem, for reasons peculiar to the
other systems.

-- 
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] Broken lock management in policy.c.

[Tom Lane]

Peter Geoghegan  writes:
> On Sun, Jan 3, 2016 at 6:00 PM, Stephen Frost  wrote:
>> I believe Tom's complaint was that the overall page is about CREATE POLICY,
>> technically, and that the text in attempting to address the concern might be
>> taken under the context of being a CREATE POLICY issue rather than a general
>> RLS issue with row locking.

> Really? But the problem happens as a consequence of having a
> subqueries within CREATE POLICY's USING quals

If that's what we're talking about, let's say it in precisely that many
words.  With an example.  The current text is 100% useless.

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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 9:23 PM, Tom Lane wrote:

Jim Nasby  writes:

regrole and regnamespace don't run their output through quote_ident().
That's contrary to all the other reg* operators.
Worse, they also don't *allow* quoted input. Not only is that different
from reg*, it's the *opposite*:


BTW, there's a concrete reason why this is broken, which is that although
regrole and regnamespace didn't bother with copying quoting/dequoting from
the other types, they *did* copy the special case logic about allowing
and emitting numeric OIDs.  This means that an output like "1234" from
regroleout is formally inconsistent: there is no way to tell if it's an
numeric OID or a role name that happens to be all digits.  (With proper
quoting logic, you could tell because an all-digits role name would have
gotten quoted.)  Conversely, if you create an all-digits role name, there
is no way to get regrolein to interpret it as such, whereas a dequoting
rule would have disambiguated.

I'm inclined to leave to_regrole and to_regnamespace alone, though, since
they have no numeric-OID path, and they will provide an "out" for anyone
who wants to handle nonquoted names.  (Though at least in HEAD we ought to
fix them to take type text as input.  Using cstring for ordinary functions
is just sloppy.)


None of the other to_reg* casts do that though, so this would be 
inconsistent.


Another potential problem for regnamespace is that it doesn't allow an 
entry for the catalog. I'm not sure what the spec says about that, but 
every other function allows dbname.schema.blah (dbname == catalog).


I started working on a fix, but it's currently blowing up in bootstrap 
and I haven't been able to figure out why yet:


running bootstrap script ... FATAL:  improper qualified name (too many 
dotted names): oid_ops

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 8b105fe..a555a1f 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -2613,6 +2613,9 @@ TSConfigIsVisible(Oid cfgid)
  * extract the schema name and object name.
  *
  * *nspname_p is set to NULL if there is no explicit schema name.
+ * 
+ * If *objname_p is not set then treat names as a schema name, possibly with a
+ * catalog name.
  */
 void
 DeconstructQualifiedName(List *names,
@@ -2623,19 +2626,21 @@ DeconstructQualifiedName(List *names,
char   *schemaname = NULL;
char   *objname = NULL;
 
-   switch (list_length(names))
+   switch (list_length(names) + objname_p ? 0 : 1)
{
case 1:
objname = strVal(linitial(names));
break;
case 2:
schemaname = strVal(linitial(names));
-   objname = strVal(lsecond(names));
+   if (objname_p)
+   objname = strVal(lsecond(names));
break;
case 3:
catalogname = strVal(linitial(names));
schemaname = strVal(lsecond(names));
-   objname = strVal(lthird(names));
+   if (objname_p)
+   objname = strVal(lthird(names));
 
/*
 * We check the catalog name and then ignore it.
@@ -2655,7 +2660,8 @@ DeconstructQualifiedName(List *names,
}
 
*nspname_p = schemaname;
-   *objname_p = objname;
+   if (objname_p)
+   *objname_p = objname;
 }
 
 /*
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 59e5dc8..8c862cf 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -1569,6 +1569,7 @@ Datum
 regrolein(PG_FUNCTION_ARGS)
 {
char   *role_name_or_oid = PG_GETARG_CSTRING(0);
+   List   *names;
Oid result;
 
/* '-' ? */
@@ -1586,7 +1587,15 @@ regrolein(PG_FUNCTION_ARGS)
}
 
/* Normal case: see if the name matches any pg_authid entry. */
-   result = get_role_oid(role_name_or_oid, false);
+   names = stringToQualifiedNameList(role_name_or_oid);
+
+   if (list_length(names) > 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_role_oid(strVal(linitial(names)), false);
 
PG_RETURN_OID(result);
 }
@@ -1668,7 +1677,9 @@ Datum
 regnamespacein(PG_FUNCTION_ARGS)
 {
char   *nsp_name_or_oid = PG_GETARG_CSTRING(0);
+   char   *nsp_name;
Oid result = 

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Jim Nasby  writes:
> What I went with. Now to figure out why this is happening...

>SELECT regnamespace('pg_catalog');
> ! ERROR:  schema "Y" does not exist
> ! LINE 1: SELECT regnamespace('pg_catalog');

Confusion between a C string and a string Value node, mayhap?

regards, tom lane


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

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Dec 29, 2015 at 11:55 PM, Stephen Frost  wrote:
> > I could go either way on that, really.  I don't find namespace to be
> > confusing when used in that way, but I'll change it since others do.
> 
> It seems to me that the way patch does it is fine..

Alright, fair enough.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Michael Paquier]

On Mon, Jan 4, 2016 at 1:20 PM, Jim Nasby  wrote:
> On 1/3/16 9:43 PM, Tom Lane wrote:
>>
>> Jim Nasby  writes:
>>>
>>> On 1/3/16 9:23 PM, Tom Lane wrote:
>>> Another potential problem for regnamespace is that it doesn't allow an
>>> entry for the catalog. I'm not sure what the spec says about that, but
>>> every other function allows dbname.schema.blah (dbname == catalog).
>>
>>
>> Meh, these types conform to no spec, so we can make them do what we
>> like.  I see about zero reason to allow a catalog name, it would mostly
>> just confuse people.
>
>
> Ok. Not like CREATE SCHEMA allows that anyway.
>
>>> I started working on a fix, but it's currently blowing up in bootstrap
>>> and I haven't been able to figure out why yet:
>>> running bootstrap script ... FATAL:  improper qualified name (too many
>>> dotted names): oid_ops
>>
>>
>> Recommendation: don't muck with DeconstructQualifiedName.  That is called
>> in too many places and we are *not* touching any such API twenty-four
>> hours before release wrap.
>
>
> Yeah, looks like that's what was blowing up.
>
>> There's no real advantage to that anyway, compared with just doing
>> stringToQualifiedNameList and then complaining if its list_length isn't 1.
>
>
> What I went with.

Thanks, this is more or less what I... just did..

+result = get_namespace_oid(nsp_name, false);
This is incorrect, you should use strVal(linitial(names)) instead.

+if (list_length(names) > 1)
+ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("improper qualified name (too many dotted names): %s",
+   NameListToString(names;
I would just mark that as "Invalid syntax".

A couple of tests in regproc.sql would be a good addition as well.
-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 10:20 PM, Jim Nasby wrote:

What I went with. Now to figure out why this is happening...


Nevermind, see my stupidity now. Should have full patch soon.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 11:05 PM, Michael Paquier wrote:

I'll take a look, but Michael, if you have time to review, an extra set
>of eyeballs wouldn't hurt.  There is no margin for error right now.

I'm just on it:)  Will update in a couple of minutes, I noticed some
stuff in Jim's patch.


BTW, in case you miss it... I was inconsistent with the list 
length_names checks... one is


if (list_length(names) > 1)

the other is

if (list_length(names) != 1)

(stringToQualifiedNameList() can't actually return a 0 length list and 
IIRC there was another place doing a > check.)

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] pgsql: Further tweaking of print_aligned_vertical().

[Michael Paquier]

On Fri, Jan 1, 2016 at 6:41 AM, Bruce Momjian  wrote:
> On Wed, Dec  2, 2015 at 01:16:09AM +, Greg Stark wrote:
>>
>> On 1 Dec 2015 19:48, "Tom Lane"  wrote:
>> >
>> > In passing, avoid possible calculation of log10(0).  Probably that's
>> > harmless, given the lack of field complaints, but it seems risky:
>> > conversion of NaN to an integer isn't well defined.
>>
>> Am I going to have to fire up the emulator again?
>
> Greg's lightning talk in Vienna about how he got the emulator working
> was priceless.  I know he posted the VAX results, but how he got them
> was amazing.

+1. The work of a pure hacker.
-- 
Michael


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

Re: [HACKERS] [PATCH] Refactoring of LWLock tranches

[Amit Kapila]

On Mon, Jan 4, 2016 at 4:49 AM, Robert Haas  wrote:
>
> On Thu, Dec 31, 2015 at 3:36 AM, Amit Kapila 
wrote:
> > LWLock *LWLockAssignFromTranche(const char *tranche_name) will
> > assign a lock for the specified tranche.  This also ensures that no
> > more than requested number of LWLocks can be assigned from a
> > specified tranche.
>
> However, this seems like an awkward API, because if there are many
> LWLocks you're going to need to look up the tranche name many times,
> and then you're going to need to make an array of pointers to them
> somehow, introducing a thoroughly unnecessary layer of indirection.
> Instead, I suggest just having a function LWLockPadded
> *GetLWLockAddinTranche(const char *tranche_name) that returns a
> pointer to the base of the array.
>

If we do that way, then user of API needs to maintain the interlock
guarantee that the requested number of locks is same as allocations
done from that tranche and also if it is not allocating all the
LWLocks in one function, it needs to save the base address of the
array and then allocate from it by maintaining some counter.
I agree that looking up for tranche names multiple times is not good,
if there are many number of lwlocks, but that is done at startup
time and not at operation-level.  Also if we look currently at
the extensions in contrib, then just one of them allocactes one
LWLock in this fashion, now there could be extnsions apart from
extensions in contrib, but not sure if they require many number of
LWLocks, so I feel it is better to give an API which is more
convinient for user to use.

> > Also I have retained NUM_USER_DEFINED_LWLOCKS in
> > MainLWLockArray so that if any extensions want to assign a LWLock
> > after startup, it can be used from this pool.  The tranche for such
locks
> > will be reported as main.
>
> I don't like this.  I think we should get rid of
> NUM_USER_DEFINED_LWLOCKS altogether.  It's just an accident waiting to
> happen, and I don't think there are enough people using LWLocks from
> extensions that we'll annoy very many people by breaking backward
> compatibility here.  If we are going to care about backward
> compatibility, then I think the old-style lwlocks should go in their
> own tranche, not main.  But personally, I think it'd be better to just
> force a hard break and make people switch to using the new APIs.
>

One point to think before removing it altogether, is that the new API's
provide a way to allocate LWLocks at the startup-time and if any user
wants to allocate a new LWLock after start-up, it will not be possible
with the proposed API's.  Do you think for such cases, we should ask
user to use it the way we have exposed or do you have something else
in mind?



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

Re: [HACKERS] pgsql: Further tweaking of print_aligned_vertical().

[Tom Lane]

Michael Paquier  writes:
> On Fri, Jan 1, 2016 at 6:41 AM, Bruce Momjian  wrote:
>> Greg's lightning talk in Vienna about how he got the emulator working
>> was priceless.  I know he posted the VAX results, but how he got them
>> was amazing.

> +1. The work of a pure hacker.

Is this available online anywhere?

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] Keyword classifications

[Michael Paquier]

On Sat, Jan 2, 2016 at 4:06 AM, Tom Lane  wrote:
> I wrote:
>> Now, one line of thought here is that flatten_reloptions() is out of its
>> mind to not be worrying about quoting the reloption values.  And perhaps
>> it is, but I think if we go that direction, we may be fighting similar
>> fires for awhile to come.  psql's describe.c, for example, doesn't worry
>> about quoting anything when printing reloptions, and there's likely
>> similar code in third-party clients.  Also, a solution like this would
>> do nothing for existing dump files.
>
>> The other line of thought is that we're already making an effort to allow
>> any keyword to appear as the value of a def_arg, and maybe we should try
>> to make that work 100% instead of only 90%.
>
> After further thought I believe that the right thing to do is pursue both
> these lines of attack.  Adding quoting in flatten_reloptions() seems like
> a safely back-patchable fix for the original complaint, and it's really
> necessary anyway for reloption values that don't look like either an
> identifier or a number.  The grammar allows any arbitrary string constant
> to be the original form of a reloption, and we have no good reason to
> assume that extension modules will constrain their custom reloptions to
> be one or the other.  (I'm thinking we'd better be prepared to
> double-quote the option names, too, just in case.)
>
> The grammar fixes seem like a good thing to do in the long run, too,
> but there's little need to risk back-patching it since accepting
> col_name_keywords without quoting would be mostly a convenience issue.

A different angle of attack is to flatten the argument quotes directly
in reloptions.c:
http://www.postgresql.org/message-id/CAB7nPqTpdGLqLTxuGhBC2GabGNiFRAtLjFbxu=agy1rx_dg...@mail.gmail.com
But you did not like that :p
-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 10:46 PM, Jim Nasby wrote:

Added. I'm gonna call this good for now. Note this is just against HEAD
since I don't have 9.5 setup yet. Presumably the patch should still
apply...


BTW, in case it's helpful... 
https://github.com/decibel/postgres/tree/regquote

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 11:24 PM, Michael Paquier wrote:

Sorry, didn't realize you were on it.

No worries. I know it's already late where you are.


And late where 9.5 is... ;)


I would use != 1 instead here, even if the function is strict.


Yeah, I effectively pulled the pattern from DeconstructQualifiedName, 
but != 1 is better.



You forgot to update the regression test output. And actually on

Doh.


second thoughts the tests you added do not actually bring much value
because what is tested is just the behavior of
stringToQualifiedNameList, and the other reg* are not tested that as


Seemed good to test what the original bug was, but sure.


well. However I think that we had better test the failures of
regnamespace and regrole when more than 1 element is parsed as this is
a new particular code path. Attached is an updated patch which passes
check-world in my environment.


Patch looks good to me. FWIW, RhodiumToad and macdice looked at my patch 
as well and didn't see any problems you didn't mention.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] pgsql: Further tweaking of print_aligned_vertical().

[Michael Paquier]

On Mon, Jan 4, 2016 at 3:22 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Fri, Jan 1, 2016 at 6:41 AM, Bruce Momjian  wrote:
>>> Greg's lightning talk in Vienna about how he got the emulator working
>>> was priceless.  I know he posted the VAX results, but how he got them
>>> was amazing.
>
>> +1. The work of a pure hacker.
>
> Is this available online anywhere?

Arg, actually it is not:
https://wiki.postgresql.org/wiki/PostgreSQL_Conference_Europe_Talks_2015
Greg could you add it there?
-- 
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] Additional role attributes && superuser review

[Michael Paquier]

On Thu, Dec 31, 2015 at 4:26 PM, Noah Misch  wrote:
> On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote:
>> * Noah Misch (n...@leadboat.com) wrote:
>> I disagree that we would.  Having a single
>> set of default roles which provide a sensible breakdown of permissions
>> is a better approach than asking every administrator and application
>> developer who is building tools on top of PG to try and work through
>> what makes sense themselves, even if that means we have a default role
>> with a small, or even only an individual, capability.
>
> The proposed pg_replication role introduces abstraction that could, as you
> hope, spare a DBA from studying sets of functions to grant together.  The
> pg_rotate_logfile role, however, does not shield the DBA from complexity.
> Being narrowly tied to a specific function, it's just a suboptimal spelling of
> GRANT.  The gap in GRANT has distorted the design for these predefined roles.
> I do not anticipate a sound design discussion about specific predefined roles
> so long as the state of GRANT clouds the matter.

As the patch stands, the system roles that are just close to synonyms
of GRANT/REVOKE are the following:
- pg_file_settings, which works just on the system view
pg_file_settings and the function pg_show_all_file_settings().
- pg_rotate_logfile as mentioned already.
- pg_signal_backend, which is just checked once in pg_signal_backend
Based on those concerns, it looks clear that they should be shaved off
from the patch.

>> > To summarize, I think the right next step is to resume designing pg_dump
>> > support for system object ACLs.  I looked over your other two patches and 
>> > will
>> > unshelve those reviews when their time comes.
>>
>> To be clear, I don't believe the two patches are particularly involved
>> with each other and don't feel that one needs to wait for the other.
>
> Patch 2/3 could stand without patch 3/3, but not vice-versa.  It's patch 2/3
> that makes pg_dumpall skip ^pg_ roles, and that must be in place no later than
> the first patch that adds a predefined ^pg_ role.

I am a bit confused by this statement, and I disagree with Stephen's
point as well. It seems to me that 2/3 exists *because* 3/3 is here.
Why would we want to restrict the role names that can be used if we
are not going to introduce some system roles that are created at
bootstrap?
-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Jim Nasby  writes:
> On 1/3/16 9:23 PM, Tom Lane wrote:
>> (Though at least in HEAD we ought to
>> fix them to take type text as input.  Using cstring for ordinary functions
>> is just sloppy.)

> BTW, *all* the reg*in() functions do that...

Yeah, I noticed that.  They're all broken.  There are no other built-in
functions that take cstring except for datatype input functions and
encoding conversion functions, which are required to by their respective
meta-APIs, and which are not meant to be invoked directly from SQL.
These functions had no business being the first to think that cstring is
a full fledged type; which it is not.  Notably, that means this doesn't
work:

regression=# select to_regrole('foo'::text);
ERROR:  function to_regrole(text) does not exist

and most other cases where you'd computed an input value rather than
merely typed in a literal would fail likewise.

It's not a release stopper, but I plan to fix it in HEAD whenever I have
an idle hour.

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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 10:26 PM, Michael Paquier wrote:

Thanks, this is more or less what I... just did..


Sorry, didn't realize you were on it.


+result = get_namespace_oid(nsp_name, false);
This is incorrect, you should use strVal(linitial(names)) instead.


Yup. Dur.


+if (list_length(names) > 1)
+ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("improper qualified name (too many dotted names): %s",
+   NameListToString(names;
I would just mark that as "Invalid syntax".


Just noticed this... I just copied the same syntax used elsewhere... 
whoever commits feel free to editorialize...



A couple of tests in regproc.sql would be a good addition as well.


Added. I'm gonna call this good for now. Note this is just against HEAD 
since I don't have 9.5 setup yet. Presumably the patch should still apply...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 59e5dc8..529d692 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -1569,6 +1569,7 @@ Datum
 regrolein(PG_FUNCTION_ARGS)
 {
char   *role_name_or_oid = PG_GETARG_CSTRING(0);
+   List   *names;
Oid result;
 
/* '-' ? */
@@ -1586,7 +1587,15 @@ regrolein(PG_FUNCTION_ARGS)
}
 
/* Normal case: see if the name matches any pg_authid entry. */
-   result = get_role_oid(role_name_or_oid, false);
+   names = stringToQualifiedNameList(role_name_or_oid);
+
+   if (list_length(names) > 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_role_oid(strVal(linitial(names)), false);
 
PG_RETURN_OID(result);
 }
@@ -1600,9 +1609,18 @@ Datum
 to_regrole(PG_FUNCTION_ARGS)
 {
char   *role_name = PG_GETARG_CSTRING(0);
+   List   *names;
Oid result;
 
-   result = get_role_oid(role_name, true);
+   names = stringToQualifiedNameList(role_name);
+
+   if (list_length(names) > 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_role_oid(strVal(linitial(names)), true);
 
if (OidIsValid(result))
PG_RETURN_OID(result);
@@ -1668,6 +1686,7 @@ Datum
 regnamespacein(PG_FUNCTION_ARGS)
 {
char   *nsp_name_or_oid = PG_GETARG_CSTRING(0);
+   List   *names;
Oid result = InvalidOid;
 
/* '-' ? */
@@ -1685,7 +1704,15 @@ regnamespacein(PG_FUNCTION_ARGS)
}
 
/* Normal case: see if the name matches any pg_namespace entry. */
-   result = get_namespace_oid(nsp_name_or_oid, false);
+   names = stringToQualifiedNameList(nsp_name_or_oid);
+
+   if (list_length(names) != 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_namespace_oid(strVal(linitial(names)), false);
 
PG_RETURN_OID(result);
 }
@@ -1699,9 +1726,18 @@ Datum
 to_regnamespace(PG_FUNCTION_ARGS)
 {
char   *nsp_name = PG_GETARG_CSTRING(0);
+   List   *names;
Oid result;
 
-   result = get_namespace_oid(nsp_name, true);
+   names = stringToQualifiedNameList(nsp_name);
+
+   if (list_length(names) != 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_namespace_oid(strVal(linitial(names)), true);
 
if (OidIsValid(result))
PG_RETURN_OID(result);
diff --git a/src/test/regress/sql/regproc.sql b/src/test/regress/sql/regproc.sql
index 8edaf15..cb427dc 100644
--- a/src/test/regress/sql/regproc.sql
+++ b/src/test/regress/sql/regproc.sql
@@ -14,7 +14,9 @@ SELECT regprocedure('abs(numeric)');
 SELECT regclass('pg_class');
 SELECT regtype('int4');
 SELECT regrole('regtestrole');
+SELECT regrole('"regtestrole"');
 SELECT regnamespace('pg_catalog');
+SELECT regnamespace('"pg_catalog"');
 
 SELECT to_regoper('||/');
 SELECT to_regoperator('+(int4,int4)');
@@ -23,7 +25,9 @@ SELECT to_regprocedure('abs(numeric)');
 SELECT to_regclass('pg_class');
 

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Michael Paquier  writes:
> Attached is an updated patch which passes
> check-world in my environment.

Pushed with trivial cosmetic changes to the code, and slightly more
extensive work on the regression test cases.

It strikes me that there might be an argument for returning NULL
rather than raising an error for to_regrole('foo.bar') and
to_regnamespace('foo.bar').  On the other hand, input-syntax issues
result in errors for the other to_reg* functions, so it might
be fine as-is.  In any case, relaxing the error rather than adding
one is much easier to justify as a future change, so I left it
as Jim/Michael had it for the moment.

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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Michael Paquier]

On Mon, Jan 4, 2016 at 3:09 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Attached is an updated patch which passes
>> check-world in my environment.
>
> Pushed with trivial cosmetic changes to the code, and slightly more
> extensive work on the regression test cases.
>
> It strikes me that there might be an argument for returning NULL
> rather than raising an error for to_regrole('foo.bar') and
> to_regnamespace('foo.bar').  On the other hand, input-syntax issues
> result in errors for the other to_reg* functions, so it might
> be fine as-is.  In any case, relaxing the error rather than adding
> one is much easier to justify as a future change, so I left it
> as Jim/Michael had it for the moment.

Thanks!
-- 
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] Improved tab completion for FDW DDL

[Michael Paquier]

On Wed, Dec 30, 2015 at 9:21 PM, Andreas Karlsson  wrote:
> Hi,
>
> Here is a patch which adds the below missing tab completions for the FDW DDL
> commands. I noticed these were missing while playing around with FDWs a
> while ago.
>
> "ALTER SERVER " with "RENAME TO"
> "CREATE SERVER  FOREIGN DATA WRAPPER" with fdw name
> "CREATE|ALTER USER MAPPING FOR  SERVER " with "OPTIONS ("
>
> Another completion which is currently missing but I am not sure if we should
> add or not is completing "ALTER|CREATE|DROP USER" with "MAPPING FOR", but
> since it might interfere with completing to username for "ALTER|DROP USER" I
> am not sure we want it. What do you think?

You may want to use Matches() instead of TailMatches() for performance reasons.
-- 
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] Remove Windows crash dump support?

[Michael Paquier]

On Fri, Dec 25, 2015 at 8:57 PM, Craig Ringer  wrote:
> On 25 December 2015 at 19:45, Michael Paquier 
> wrote:
>>
>> On Thu, Dec 24, 2015 at 5:57 PM, Dave Page  wrote:
>> > On Thu, Dec 24, 2015 at 2:14 AM, Craig Ringer 
>> > wrote:
>> >> On 22 December 2015 at 23:48, Alex Ignatov 
>> >> wrote:
>> >>
>> >>>
>> >>> I think that you can debug crash dump since windbg exists.
>> >>
>> >>
>> >> Nobody in their right mind uses windbg though. Visual Studio is really
>> >> where
>> >> it's at and the Express versions make it much more practical.
>>
>> Well, FWIW, I have been working lately on a bug hidden in a custom
>> background worker on Windows that crashed in some weird way with a
>> 0x00C5, and while I have a reproducible test case I have not yet
>> found the time to look at it yet but I would think that this is
>> generating a core dump, and that I will need to use windbg for that.
>> Hence count me on the -1 team.
>
>
> Huh?
>
> You can use VS Express to debug a core dump.  Per links upthread you can get
> the platform to generate core dumps for you. No windbg required.
>
> If you want to torture yourself using windbg go ahead, but it really isn't
> necessary, you can use a sane debugger.

Well, coming back to my story with the background worker I have been
debugging. Creating PGDATA/crashdumps has allowed me to easily get a
dump, and then I had a look at it using my Win7 workstation because VS
was not available in the server where the crash happened, which was a
2k12 server btw. So I think that it is still useful for debugging code
paths running custom code, even if we consider Postgres as rock-solid
on Windows.

In short, -1.
-- 
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] row_security GUC does not behave as documented

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > As we don't want to end up with the same behavior-change-due-to-GUC that
> > we had with the original row_security implementation, we should change
> > the code as your patch does and update the regression tests accordingly.
> 
> I think probably the tests need some adjustment rather than just stuffing
> in the new results; but I'm unsure what's most appropriate.

Right, the comments, at least, need to be updated to be correct.

> > Perhaps the error code thrown could be tailored a bit when it's the
> > owner, to indicate that FORCE RLS has been set on the table, but I'm not
> > sure it's really a big deal either way.
> 
> Yeah, the error message seemed less than apropos to me too; but on the
> other hand there's an argument that FORCE RLS means "treat me just like
> everybody else".

Agreed.

> One idea would be to use the same primary error message either way,
> but add a DETAIL or HINT mentioning FORCE RLS if it's the table owner.

Having a detail or hint which indicates that seems like a great approach
to me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 9:43 PM, Tom Lane wrote:

Jim Nasby  writes:

On 1/3/16 9:23 PM, Tom Lane wrote:
Another potential problem for regnamespace is that it doesn't allow an
entry for the catalog. I'm not sure what the spec says about that, but
every other function allows dbname.schema.blah (dbname == catalog).


Meh, these types conform to no spec, so we can make them do what we
like.  I see about zero reason to allow a catalog name, it would mostly
just confuse people.


Ok. Not like CREATE SCHEMA allows that anyway.


I started working on a fix, but it's currently blowing up in bootstrap
and I haven't been able to figure out why yet:
running bootstrap script ... FATAL:  improper qualified name (too many
dotted names): oid_ops


Recommendation: don't muck with DeconstructQualifiedName.  That is called
in too many places and we are *not* touching any such API twenty-four
hours before release wrap.


Yeah, looks like that's what was blowing up.


There's no real advantage to that anyway, compared with just doing
stringToQualifiedNameList and then complaining if its list_length isn't 1.


What I went with. Now to figure out why this is happening...

  SELECT regnamespace('pg_catalog');
! ERROR:  schema "Y" does not exist
! LINE 1: SELECT regnamespace('pg_catalog');
!
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 59e5dc8..339bfe7 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -1569,6 +1569,7 @@ Datum
 regrolein(PG_FUNCTION_ARGS)
 {
char   *role_name_or_oid = PG_GETARG_CSTRING(0);
+   List   *names;
Oid result;
 
/* '-' ? */
@@ -1586,7 +1587,15 @@ regrolein(PG_FUNCTION_ARGS)
}
 
/* Normal case: see if the name matches any pg_authid entry. */
-   result = get_role_oid(role_name_or_oid, false);
+   names = stringToQualifiedNameList(role_name_or_oid);
+
+   if (list_length(names) > 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_role_oid(strVal(linitial(names)), false);
 
PG_RETURN_OID(result);
 }
@@ -1668,7 +1677,9 @@ Datum
 regnamespacein(PG_FUNCTION_ARGS)
 {
char   *nsp_name_or_oid = PG_GETARG_CSTRING(0);
+   char   *nsp_name;
Oid result = InvalidOid;
+   List   *names;
 
/* '-' ? */
if (strcmp(nsp_name_or_oid, "-") == 0)
@@ -1685,7 +1696,15 @@ regnamespacein(PG_FUNCTION_ARGS)
}
 
/* Normal case: see if the name matches any pg_namespace entry. */
-   result = get_namespace_oid(nsp_name_or_oid, false);
+   names = stringToQualifiedNameList(nsp_name_or_oid);
+
+   if (list_length(names) != 1)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   errmsg("improper qualified name (too many dotted 
names): %s",
+  NameListToString(names;
+
+   result = get_namespace_oid(nsp_name, false);
 
PG_RETURN_OID(result);
 }

-- 
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] count_nulls(VARIADIC "any")

[Pavel Stehule]

Hi

2016-01-03 22:49 GMT+01:00 Jim Nasby :

> On 1/3/16 2:37 PM, Pavel Stehule wrote:
>
>> +   /* num_nulls(VARIADIC NULL) is defined as NULL */
>> +   if (PG_ARGISNULL(0))
>> +   return false;
>>
>
> Could you add to the comment explaining why that's the desired behavior?
>

This case should be different than num_nulls(VARIADIC ARRAY[..]) - this
situation is really equivalent of missing data and NULL is correct answer.
It should not be too clean in num_nulls, but when it is cleaner for
num_notnulls. And more, it is consistent with other variadic functions in
Postgres: see concat_internal and text_format.


>
> +   /*
>> +* Non-null argument had better be an array.  We assume
>> that any call
>> +* context that could let get_fn_expr_variadic return
>> true will have
>> +* checked that a VARIADIC-labeled parameter actually is
>> an array.  So
>> +* it should be okay to just Assert that it's an array
>> rather than
>> +* doing a full-fledged error check.
>> +*/
>> +
>>  Assert(OidIsValid(get_base_element_type(get_fn_expr_argtype(fcinfo->flinfo,
>> 0;
>>
>
> Erm... is that really the way to verify that what you have is an array?
> ISTM there should be a macro for that somewhere...
>

really, it is. It is used more time. Although I am not against some macro,
I don't think so it is necessary. The macro should not be too shorter than
this text.


>
> +   /* OK, safe to fetch the array value */
>> +   arr = PG_GETARG_ARRAYTYPE_P(0);
>> +
>> +   ndims = ARR_NDIM(arr);
>> +   dims = ARR_DIMS(arr);
>> +   nitems = ArrayGetNItems(ndims, dims);
>> +
>> +   bitmap = ARR_NULLBITMAP(arr);
>> +   if (bitmap)
>> +   {
>> +   bitmask = 1;
>> +
>> +   for (i = 0; i < nitems; i++)
>> +   {
>> +   if ((*bitmap & bitmask) == 0)
>> +   count++;
>> +
>> +   bitmask <<= 1;
>> +   if (bitmask == 0x100)
>> +   {
>> +   bitmap++;
>> +   bitmask = 1;
>>
>
> For brevity and example sake it'd probably be better to just use the
> normal iterator, unless there's a serious speed difference?
>

The iterator does some memory allocations and some access to type cache.
Almost all work of iterator is useless for this case. This code is
developed by Marko, but I agree with this design. Using the iterator is big
gun for this case. I didn't any performance checks, but it should be
measurable  for any varlena arrays.

Regards

Pavel


> In the unit test, I'd personally prefer just building a table with the
> test cases and the expected NULL/NOT NULL results, at least for all the
> calls that would fit that paradigm. That should significantly reduce the
> size of the test. Not a huge deal though...
>
> Also, I don't think anything is testing multiples of whatever value... how
> 'bout change the generate_series CASE statement to >40 instead of <>40?
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

Re: [HACKERS] 9.5 BLOCKER: regrole and regnamespace and quotes

[Michael Paquier]

On Mon, Jan 4, 2016 at 2:01 PM, Tom Lane  wrote:
> Jim Nasby  writes:
>> On 1/3/16 10:26 PM, Michael Paquier wrote:
>>> Thanks, this is more or less what I... just did..
>
>> Sorry, didn't realize you were on it.
>
>>> A couple of tests in regproc.sql would be a good addition as well.
>
>> Added. I'm gonna call this good for now. Note this is just against HEAD
>> since I don't have 9.5 setup yet. Presumably the patch should still apply...
>
> I'll take a look, but Michael, if you have time to review, an extra set
> of eyeballs wouldn't hurt.  There is no margin for error right now.

I'm just on it :) Will update in a couple of minutes, I noticed some
stuff in Jim's patch.
-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Michael Paquier]

On Mon, Jan 4, 2016 at 1:46 PM, Jim Nasby  wrote:
> On 1/3/16 10:26 PM, Michael Paquier wrote:
>>
>> Thanks, this is more or less what I... just did..
>
>
> Sorry, didn't realize you were on it.

No worries. I know it's already late where you are.

>> A couple of tests in regproc.sql would be a good addition as well.
>
> Added. I'm gonna call this good for now. Note this is just against HEAD
> since I don't have 9.5 setup yet. Presumably the patch should still apply...

+if (list_length(names) > 1)
+ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("improper qualified name (too many dotted names): %s",
+   NameListToString(names;
I would use != 1 instead here, even if the function is strict.

You forgot to update the regression test output. And actually on
second thoughts the tests you added do not actually bring much value
because what is tested is just the behavior of
stringToQualifiedNameList, and the other reg* are not tested that as
well. However I think that we had better test the failures of
regnamespace and regrole when more than 1 element is parsed as this is
a new particular code path. Attached is an updated patch which passes
check-world in my environment.
-- 
Michael


20160104_regproc_quotes.patch
Description: binary/octet-stream

-- 
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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Jim Nasby]

On 1/3/16 9:23 PM, Tom Lane wrote:

(Though at least in HEAD we ought to
fix them to take type text as input.  Using cstring for ordinary functions
is just sloppy.)


BTW, *all* the reg*in() functions do that...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] count_nulls(VARIADIC "any")

[Jim Nasby]

On 1/3/16 10:23 PM, Pavel Stehule wrote:

Hi

2016-01-03 22:49 GMT+01:00 Jim Nasby >:

On 1/3/16 2:37 PM, Pavel Stehule wrote:

+   /* num_nulls(VARIADIC NULL) is defined as NULL */
+   if (PG_ARGISNULL(0))
+   return false;


Could you add to the comment explaining why that's the desired behavior?


This case should be different than num_nulls(VARIADIC ARRAY[..]) - this
situation is really equivalent of missing data and NULL is correct
answer. It should not be too clean in num_nulls, but when it is cleaner
for num_notnulls. And more, it is consistent with other variadic
functions in Postgres: see concat_internal and text_format.


Makes sense, now that you explain it. Which is why I'm thinking it'd be 
good to add that explanation to the comment... ;)



  
Assert(OidIsValid(get_base_element_type(get_fn_expr_argtype(fcinfo->flinfo, 
0;


Erm... is that really the way to verify that what you have is an
array? ISTM there should be a macro for that somewhere...


really, it is. It is used more time. Although I am not against some
macro, I don't think so it is necessary. The macro should not be too
shorter than this text.


Well, if there's other stuff doing that... would be nice to refactor 
that though.



For brevity and example sake it'd probably be better to just use the
normal iterator, unless there's a serious speed difference?


The iterator does some memory allocations and some access to type cache.
Almost all work of iterator is useless for this case. This code is
developed by Marko, but I agree with this design. Using the iterator is
big gun for this case. I didn't any performance checks, but it should be
measurable  for any varlena arrays.


Makes sense then.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


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

Re: [HACKERS] row_security GUC does not behave as documented

[Tom Lane]

Stephen Frost  writes:
> As we don't want to end up with the same behavior-change-due-to-GUC that
> we had with the original row_security implementation, we should change
> the code as your patch does and update the regression tests accordingly.

I think probably the tests need some adjustment rather than just stuffing
in the new results; but I'm unsure what's most appropriate.

> Perhaps the error code thrown could be tailored a bit when it's the
> owner, to indicate that FORCE RLS has been set on the table, but I'm not
> sure it's really a big deal either way.

Yeah, the error message seemed less than apropos to me too; but on the
other hand there's an argument that FORCE RLS means "treat me just like
everybody else".

One idea would be to use the same primary error message either way,
but add a DETAIL or HINT mentioning FORCE RLS if it's the table owner.

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] 9.5 BLOCKER: regrole and regnamespace and quotes

[Tom Lane]

Jim Nasby  writes:
> On 1/3/16 10:26 PM, Michael Paquier wrote:
>> Thanks, this is more or less what I... just did..

> Sorry, didn't realize you were on it.

>> A couple of tests in regproc.sql would be a good addition as well.

> Added. I'm gonna call this good for now. Note this is just against HEAD 
> since I don't have 9.5 setup yet. Presumably the patch should still apply...

I'll take a look, but Michael, if you have time to review, an extra set
of eyeballs wouldn't hurt.  There is no margin for error right now.

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

[Michael Paquier]

On Sun, Jan 3, 2016 at 10:26 PM, Masahiko Sawada  wrote:
> On Fri, Dec 25, 2015 at 7:21 AM, Thomas Munro
>  wrote:
>> On Fri, Dec 25, 2015 at 8:50 AM, Masahiko Sawada  
>> wrote:
>>> On Wed, Dec 23, 2015 at 8:45 AM, Thomas Munro
>>>  wrote:
 On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
  wrote:
> If you got rid of SyncRepGetSyncStandbysOnePriority as suggested
> above, then this function could be renamed to SyncRepGetSyncStandbys.
> I think it would be a tiny bit nicer if it also took a Size n argument
> along with the output buffer pointer.
>>>
>>> Sorry, I could not get your point. SyncRepGetSyncStandbysPriority()
>>> function uses synchronous_standby_num which is global variable.
>>> But you mean that the number of synchronous standbys is given as
>>> function argument?
>>
>> Yeah, I was thinking of it as the output buffer size which I would be
>> inclined to make more explicit (I am still coming to terms with the
>> use of global variables in Postgres) but it doesn't matter, please
>> disregard that suggestion.
>>
> As for the body of that function (which I won't paste here), it
> contains an algorithm to find the top K elements in an array of N
> elements.  It does that with a linear search through the top K seen so
> far for each value in the input array, so its worst case is O(KN)
> comparisons.  Some of the sorting gurus on this list might have
> something to say about that but my take is that it seems fine for the
> tiny values of K and N that we're dealing with here, and it's nice
> that it doesn't need any space other than the output buffer, unlike
> some other top-K algorithms which would win for larger inputs.
>>>
>>> Yeah, it's improvement point.
>>> But I'm assumed that the number of synchronous replication is not
>>> large, so I use this algorithm as first version.
>>> And I think that its worst case is O(K(N-K)). Am I missing something?
>>
>> You're right, I was dropping that detail, in the tradition of the
>> hand-wavy school of big-O notation.  (I suppose you could skip the
>> inner loop when the priority is lower than the current lowest
>> priority, giving a O(N) best case when the walsenders are perfectly
>> ordered by coincidence.  Probably a bad idea or just not worth
>> worrying about.)
>
> Thank you for reviewing the patch.
> Yeah, I added the logic that skip the inner loop.
>
>>
>>> Attached latest version patch.
>>
>> +/*
>> + * Obtain currently synced LSN location: write and flush, using priority
>> - * In 9.1 we support only a single synchronous standby, chosen from a
>> - * priority list of synchronous_standby_names. Before it can become the
>> + * In 9.6 we support multiple synchronous standby, chosen from a priority
>>
>> s/standby/standbys/
>>
>> + * list of synchronous_standby_names. Before it can become the
>>
>> s/Before it can become the/Before any standby can become a/
>>
>>   * synchronous standby it must have caught up with the primary; that may
>>   * take some time. Once caught up, the current highest priority standby
>>
>> s/standby/standbys/
>>
>>   * will release waiters from the queue.
>>
>> +bool
>> +SyncRepGetSyncLsnsPriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
>> +{
>> + int sync_standbys[synchronous_standby_num];
>>
>> I think this should be sync_standbys[SYNC_REP_MAX_SYNC_STANDBY_NUM].
>> (Variable sized arrays are a feature of C99 and PostgreSQL is written
>> in C89.)
>>
>> +/*
>> + * Populate a caller-supplied array which much have enough space for
>> + * synchronous_standby_num. Returns position of standbys currently
>> + * considered as synchronous, and its length.
>> + */
>> +int
>> +SyncRepGetSyncStandbys(int *sync_standbys)
>>
>> s/much/must/ (my bad, in previous email).
>>
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("The number of synchronous standbys must be smaller than the
>> number of listed : %d",
>> + synchronous_standby_num)));
>>
>> How about "the number of synchronous standbys exceeds the length of
>> the standby list: %d"?  Error messages usually start with lower case,
>> ':' is not usually preceded by a space.
>>
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("The number of synchronous standbys must be between 1 and %d : %d",
>>
>> s/The/the/, s/ : /: /
>
> Fixed you mentioned.
>
> Attached latest v5 patch.
> Please review it.

Something that I find rather scary with this patch: could it be
possible to get actual regression tests now that there is more
machinery with PostgresNode.pm? As syncrep code paths get more and
more complex, so are debugging and maintenance.
-- 
Michael


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

[HACKERS] commitfest html - wrong closing tag

[Erik Rijkers]

Hi,

I noticed (trying to parse out the "Latest patch" urls) that
in the html of the commitfest pages, for instance in:

   https://commitfest.postgresql.org/8/

all rows of the html-table (I think it is the "Committer"-column) 
contain an erroneous ''. It should be ''.


See example below, the '' should obviously be ''.

Browsers don't seem to mind (and I can easily "parse around" it) but 
maybe it can be fixed?



Thanks,

Erik Rijkers





(html from https://commitfest.postgresql.org/8/ )
--8<--
  
   Improve handling of OOM errors in libpq making 
process hangling for COPY and bind (take 2)

   Ready for Committer
   Michael Paquier (michael-kun)
   Amit Kapila (amitkapila), Heikki Linnakangas (heikki)
   
   2015-12-2013:14
   2015-12-1506:54

  
--8<--


--
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 html - wrong closing tag

[Magnus Hagander]

On Sun, Jan 3, 2016 at 9:26 AM, Erik Rijkers  wrote:

> Hi,
>
> I noticed (trying to parse out the "Latest patch" urls) that
> in the html of the commitfest pages, for instance in:
>
>https://commitfest.postgresql.org/8/
>
> all rows of the html-table (I think it is the "Committer"-column) contain
> an erroneous ''. It should be ''.
>
> See example below, the '' should obviously be ''.
>
> Browsers don't seem to mind (and I can easily "parse around" it) but maybe
> it can be fixed?
>

That is indeed obviously wrong. Fixed and deployed. (
http://git.postgresql.org/gitweb/?p=pgcommitfest2.git;a=commitdiff;h=3d583f3fb0184b4858395a385438e4cb22080612
)

Is there a particular thing you're trying to parse the data out for? As in
is there some data that we should already be providing in a structured
format instead of requiring parsing it out?


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

[HACKERS] \x auto and EXPLAIN

[Andreas Karlsson]

Hi,

psql's "\x auto" is a nice feature, but it is made much less useful in 
my opinion due to the expanded output format making query plans 
unreadable (and query plans often end up using expanded display due to 
their width). I think we should never use the expanded format for 
EXPLAIN output in the "\x auto" mode, since even when the wrapped format 
is used the query plans are very hard to read.


I see two ways to fix this.

1) Never use expanded display for the case where there is only one 
column. There seems to me like there is little value in using expanded 
display for when you only have one column, but I may be missing some use 
case here.


2) Explicitly detect (for example based on the headers) that the result 
is a query plan and if so disable expanded display.


I have attached a trivial patch for each solution.

Andreas
diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index 8958903..29061d2 100644
--- a/src/bin/psql/print.c
+++ b/src/bin/psql/print.c
@@ -818,7 +818,7 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
 	 * If in expanded auto mode, we have now calculated the expected width, so
 	 * we can now escape to vertical mode if necessary.
 	 */
-	if (cont->opt->expanded == 2 && output_columns > 0 &&
+	if (cont->opt->expanded == 2 && output_columns > 0 && cont->ncolumns > 1 &&
 		(output_columns < total_header_width || output_columns < width_total))
 	{
 		print_aligned_vertical(cont, fout, is_pager);
diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index 8958903..319ec65 100644
--- a/src/bin/psql/print.c
+++ b/src/bin/psql/print.c
@@ -614,6 +614,7 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
 	printTextLineWrap *wrap;	/* Wrap status for each column */
 	int			output_columns = 0;		/* Width of interactive console */
 	bool		is_local_pager = false;
+	bool		is_explain;
 
 	if (cancel_pressed)
 		return;
@@ -814,11 +815,14 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
 		}
 	}
 
+	is_explain = cont->ncolumns == 1 &&
+	  strcmp(cont->headers[0], "QUERY PLAN") == 0;
+
 	/*
 	 * If in expanded auto mode, we have now calculated the expected width, so
 	 * we can now escape to vertical mode if necessary.
 	 */
-	if (cont->opt->expanded == 2 && output_columns > 0 &&
+	if (cont->opt->expanded == 2 && output_columns > 0 && !is_explain &&
 		(output_columns < total_header_width || output_columns < width_total))
 	{
 		print_aligned_vertical(cont, fout, is_pager);

-- 
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 html - wrong closing tag

[Erik Rijkers]

On 2016-01-03 10:06, Magnus Hagander wrote:

On Sun, Jan 3, 2016 at 9:26 AM, Erik Rijkers  wrote:

an erroneous ''. It should be ''.


Is there a particular thing you're trying to parse the data out for? As 
in

is there some data that we should already be providing in a structured
format instead of requiring parsing it out?



I'm just trying to set up a way to compile and test all outstanding 
patches.


It might perhaps be handy to have that patches table's columns somewhere 
(in tab-separated, perhaps?).


Of course most of the work is downstream of that download, anyway. 
Compile & check (also rather simple) but especially to have some 
loading/testing (both general, and specific to the patch's goal).


thanks,

Erik Rijkers





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

Re: [HACKERS] Some 9.5beta2 backend processes not terminating properly?

[Andres Freund]

On 2016-01-03 10:03:41 +0530, Amit Kapila wrote:
> On Sun, Jan 3, 2016 at 3:01 AM, Andres Freund  wrote:
> > Indeed it does use shutdown(). If I read the npgsql code that'll even be
> > done in the exception handling path. So fixing the 0 byte case might
> > already do the trick.
> >
> 
> I think this true for a TCP socket, but this code-path is used for UDP
> (SOCK_DGRAM) sockets as well and there is a comment below in
> that function which seems to be indicating why originally 0 byte case
> has not been handled at the place suggested by you (now it seems to
> be much less relevant).

I'm not sure what the origin of that comment is, it's been there all the
way since a4c40f14. But it doesn't really have much real effect: If
WSARecv in the retry loop returns 0 bytes, we'll not retry again as r !=
SOCKET_ERROR but actually return 0.

Note that the whole retry loop in pgwin32_recv(), which kinda mitigates
the problem explained above, isn't entered anymore as the FE/BE socket
is now always operated in non-blocking mode. I.e.
if (pgwin32_noblock)
{
/*
 * No data received, and we are in "emulated non-blocking 
mode", so
 * return indicating that we'd block if we were to continue.
 */
errno = EWOULDBLOCK;
return -1;
}
will always be taken. Which exactly explains the problem, together with
the edge-triggered behaviour of
WaitForMultipleObjects()/WSAEventSelect().

I really think we have a host of buggy code around the event handling -
but most of it has been used for a long while. So I think fixing the 0
byte case for 9.5 is good enough.

Regards,

Andres


-- 
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] Avoiding pin scan during btree vacuum

[Simon Riggs]

On 21 December 2015 at 21:28, Alvaro Herrera 
wrote:

> Simon Riggs wrote:
> > During VACUUM of btrees, we need to pin all pages, even those where
> tuples
> > are not removed, which I am calling here the "pin scan". This is
> especially
> > a problem during redo, where removing one tuple from a 100GB btree can
> take
> > a minute or more. That causes replication lags. Bad Thing.
>
> Agreed on there being a problem here.
>
> As a reminder, this "pin scan" is implemented in the
> HotStandbyActiveInReplay() block in btree_xlog_vacuum; this routine is
> in charge of replaying an action recorded by _bt_delitems_vacuum.  That
> replay works by acquiring cleanup lock on all btree blocks from
> lastBlockVacuumed to "this one" (i.e. the one in which elements are
> being removed).  In essence, this means pinning *all* the blocks in the
> index, which is bad.
> The new code sets lastBlockVacuumed to Invalid; a new test in the replay
> code makes that value not pin anything.  This is supposed to optimize
> the case in question.
>

Nice summary, thanks.


> One thing that this patch should update is the comment above struct
> xl_btree_vacuum, which currently state that EnsureBlockUnpinned() is one
> of the options to apply to each block, but that routine doesn't exist.
>

Updated


> One possible naive optimization is to avoid locking pages that aren't
> leaf pages, but that would still require fetching the block from disk,
> so it wouldn't actually optimize anything other than the cleanup lock
> acquisition.  (And probably not too useful, since leaf pages are said to
> be 95% - 99% of index pages anyway.)
>

Agreed, not worth it.


> Also of interest is the comment above the call to _bt_delitems_vacuum in
> btvacuumpage(); that needs an update too.
>

Updated


> It appears to me that your patch changes the call in btvacuumscan() but
> not the one in btvacuumpage().  I assume you should be changing both.
>

Yes, that was correct. Updated.


> I think the new comment that talks about Toast Index should explain
> *why* we can skip the pinning in all cases except that one, instead of
> just saying we can do it.
>

Greatly expanded comments.

Thanks for the review.

New patch attached.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


avoid_standby_pin_scan.v2.patch
Description: Binary data

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

Re: [HACKERS] Avoiding pin scan during btree vacuum

[Simon Riggs]

On 21 December 2015 at 21:36, Tom Lane  wrote:

> Alvaro Herrera  writes:
> > I think the new comment that talks about Toast Index should explain
> > *why* we can skip the pinning in all cases except that one, instead of
> > just saying we can do it.
>
> I've not looked at that code in a long while, but my recollection is
> that it's designed that way to protect non-MVCC catalog scans, which
> are gone now ... except for SnapshotToast.


Yes, that's the principle in use here. Which means we can optimize away the
scan on a Hot Standby in all cases except for Toast Index vacuums.


> Maybe the right way to
> approach this is to adjust HeapTupleSatisfiesToast (or maybe just
> convince ourselves that no one could be dereferencing a stale toast
> pointer in the first place?) and then redesign btree vacuuming without
> the requirement to support non-MVCC scans, period.
>

We could, but its likely to be a much more complex patch.

I'm happy with this being a simple patch now, not least because I would
like to backpatch this to 9.4 where catalog scans became MVCC.

A backpatch is warranted because it is a severe performance issue with
replication and we can fix that before 9.5 hits the streets.

I'll be doing some more testing and checking, so not in a rush.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] dynloader.h missing in prebuilt package for Windows?

[Robert Haas]

On Dec 31, 2015, at 1:04 PM, Bruce Momjian  wrote:
>> On Thu, Dec 31, 2015 at 12:50:13AM -0500, Bruce Momjian wrote:
>>> On Wed, Dec 30, 2015 at 11:57:45PM -0500, Tom Lane wrote:
>>> Bruce Momjian  writes:
 Oops.  Once this patch is applied, it is only going to take effect when
 someone _installs_ Postgres.  Even an initdb will not add the file. 
 This means that upgrading to the next minor release will _not_ fix this.
>>> 
>>> Uh, what?  Surely an upgrade to a new package *would* fix it, because
>>> that is a reinstall at the filesystem level.  This patch has nothing
>>> to do with system catalog contents, which is what initdb would be
>>> concerned with.
>> 
>> Uh, do we install hew lib files and stuff in a minor upgrade?  I guess
>> we do, yeah.
> 
> Let's hold this for 9.5.1 and all minor releases will get it at the same
> time.

I vote for going ahead with this at once. It seems low risk, and having 9.5.1 
install different files from 9.5.0 seems like an unnecessary annoyance.

...Robert

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

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Fri, Dec 25, 2015 at 7:21 AM, Thomas Munro
 wrote:
> On Fri, Dec 25, 2015 at 8:50 AM, Masahiko Sawada  
> wrote:
>> On Wed, Dec 23, 2015 at 8:45 AM, Thomas Munro
>>  wrote:
>>> On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
>>>  wrote:
 If you got rid of SyncRepGetSyncStandbysOnePriority as suggested
 above, then this function could be renamed to SyncRepGetSyncStandbys.
 I think it would be a tiny bit nicer if it also took a Size n argument
 along with the output buffer pointer.
>>
>> Sorry, I could not get your point. SyncRepGetSyncStandbysPriority()
>> function uses synchronous_standby_num which is global variable.
>> But you mean that the number of synchronous standbys is given as
>> function argument?
>
> Yeah, I was thinking of it as the output buffer size which I would be
> inclined to make more explicit (I am still coming to terms with the
> use of global variables in Postgres) but it doesn't matter, please
> disregard that suggestion.
>
 As for the body of that function (which I won't paste here), it
 contains an algorithm to find the top K elements in an array of N
 elements.  It does that with a linear search through the top K seen so
 far for each value in the input array, so its worst case is O(KN)
 comparisons.  Some of the sorting gurus on this list might have
 something to say about that but my take is that it seems fine for the
 tiny values of K and N that we're dealing with here, and it's nice
 that it doesn't need any space other than the output buffer, unlike
 some other top-K algorithms which would win for larger inputs.
>>
>> Yeah, it's improvement point.
>> But I'm assumed that the number of synchronous replication is not
>> large, so I use this algorithm as first version.
>> And I think that its worst case is O(K(N-K)). Am I missing something?
>
> You're right, I was dropping that detail, in the tradition of the
> hand-wavy school of big-O notation.  (I suppose you could skip the
> inner loop when the priority is lower than the current lowest
> priority, giving a O(N) best case when the walsenders are perfectly
> ordered by coincidence.  Probably a bad idea or just not worth
> worrying about.)

Thank you for reviewing the patch.
Yeah, I added the logic that skip the inner loop.

>
>> Attached latest version patch.
>
> +/*
> + * Obtain currently synced LSN location: write and flush, using priority
> - * In 9.1 we support only a single synchronous standby, chosen from a
> - * priority list of synchronous_standby_names. Before it can become the
> + * In 9.6 we support multiple synchronous standby, chosen from a priority
>
> s/standby/standbys/
>
> + * list of synchronous_standby_names. Before it can become the
>
> s/Before it can become the/Before any standby can become a/
>
>   * synchronous standby it must have caught up with the primary; that may
>   * take some time. Once caught up, the current highest priority standby
>
> s/standby/standbys/
>
>   * will release waiters from the queue.
>
> +bool
> +SyncRepGetSyncLsnsPriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
> +{
> + int sync_standbys[synchronous_standby_num];
>
> I think this should be sync_standbys[SYNC_REP_MAX_SYNC_STANDBY_NUM].
> (Variable sized arrays are a feature of C99 and PostgreSQL is written
> in C89.)
>
> +/*
> + * Populate a caller-supplied array which much have enough space for
> + * synchronous_standby_num. Returns position of standbys currently
> + * considered as synchronous, and its length.
> + */
> +int
> +SyncRepGetSyncStandbys(int *sync_standbys)
>
> s/much/must/ (my bad, in previous email).
>
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("The number of synchronous standbys must be smaller than the
> number of listed : %d",
> + synchronous_standby_num)));
>
> How about "the number of synchronous standbys exceeds the length of
> the standby list: %d"?  Error messages usually start with lower case,
> ':' is not usually preceded by a space.
>
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("The number of synchronous standbys must be between 1 and %d : %d",
>
> s/The/the/, s/ : /: /

Fixed you mentioned.

Attached latest v5 patch.
Please review it.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 7f85b88..0c78919 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -29,10 +29,10 @@
  * single ordered queue of waiting backends, so that we can avoid
  * searching the through all waiters each time we receive a reply.
  *
- * In 9.1 we support only a single synchronous standby, chosen from a
- * priority list of synchronous_standby_names. Before it can become the
- * synchronous standby it must have caught up with the primary; that may
- * take some time. Once caught up, the 

Re: [HACKERS] rows estimate in explain analyze for the BRIN index

[Emre Hasegeli]

> But is it? Is it impossible for the BRIN bitmap index scan to return 0 rows
> (say, if the value being matched is outside the min/max boundary for every
> block range?) Granted, if we document that it always returns 0 and should be
> ignored, then confusing the actual 0 with the 0 as a representation of
> “unknown” would be less a problem.

How about -1 ?  It is an impossible value for sure.  Maybe we should
change BitmapAnd and BitmapOr nodes, too.  It is better to make it
obvious that it is not the correct value.  I don't think many people
would notice the note on the documentation.

On the other hand, returning -1 broke parser of explain.depesz.com [1].

[1] http://explain.depesz.com/s/tAkd


-- 
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] Some 9.5beta2 backend processes not terminating properly?

[Tom Lane]

Andres Freund  writes:
> On January 3, 2016 6:23:20 PM GMT+01:00, Tom Lane  wrote:
>> Agreed.  Let's do it and ship this puppy.

> Unless somebody beats me to it, I'll push in the European morning.

Um.  For something that at least potentially has portability issues
(we think not, but we could be wrong), it's pretty scary to push only
a couple of hours before the wrap deadline.  I'd like to get it done
now so we can get a full day's buildfarm testing.  If you like I can
push something ...

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] Some 9.5beta2 backend processes not terminating properly?

[Andres Freund]

On January 3, 2016 7:04:29 PM GMT+01:00, Tom Lane  wrote:
>Andres Freund  writes:
>> On January 3, 2016 6:23:20 PM GMT+01:00, Tom Lane 
>wrote:
>>> Agreed.  Let's do it and ship this puppy.
>
>> Unless somebody beats me to it, I'll push in the European morning.
>
>Um.  For something that at least potentially has portability issues
>(we think not, but we could be wrong), it's pretty scary to push only
>a couple of hours before the wrap deadline.  I'd like to get it done
>now so we can get a full day's buildfarm testing.  If you like I can
>push something ...

I agree. But I'm at an event and only have my phone with me. So I can't get to 
it right now. I might be able to push in 5ish hours. Otherwise it's in 15h. 
Sorry.

Fell free to push earlier, if you can make the 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

[HACKERS] Re: pglogical_output - a general purpose logical decoding output plugin

[Tomasz Rybak]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:tested, failed

Applies cleanly on current master (b416c0bb622ce5d33fdbec3bbce00451132f10ec).

Builds without any problems on current Debian unstable (am64 arch, GCC 5.3.1-4, 
glibc 2.21-6)
There are 2 errors in tests, but they also occur on trunk build.
parallel group (20 tests):  json_encoding combocid portals_p2 advisory_lock 
tsdicts xmlmap equivclass guc functional_deps dependency json select_views 
cluster tsearch window jsonb indirect_toast bitmapops foreign_key foreign_data
 select_views ... FAILED
 portals_p2   ... ok
parallel group (2 tests):  create_view create_index
 create_index ... FAILED
 create_view  ... ok

README.md:
+Only one database is replicated, rather than the whole PostgreSQL install. A
[...]
+Unlike block-level ("physical") streaming replication, the change stream from
+the `pglogical` output plugin is compatible across different PostgreSQL
+versions and can even be consumed by non-PostgreSQL clients.
+
+Because logical decoding is used, only the changed rows are sent on the wire.
+There's no index change data, no vacuum activity, etc transmitted.
+
+The use of a replication slot means that the change stream is reliable and
+crash-safe. If

Isn't it feature of logical replication, not this particular plugin?
I'm not sure whether all that text needs to be repeated here.
OTOH this is good summary - so maybe just add links to base
documentation about replication, logical replication, slots, etc.

+# Usage
+
+The overall flow of client/server interaction is:
This part (flow) belongs to DESIGN.md, not to usage.

+* [Client issues `IDENTIFY_SYSTEM`
Is the [ needed here?

protocol.txt
Contains wrapped lines, but also very long lines. While long
lines make sense for tables, they should not occur in paragraphs, e.g. in
+== Arguments client supplies to output plugin
and following ones. It looks like parts of this file were formatted, and parts 
not.

In summary, documentation is mostly OK, and it describe plugin quite nicely.
The only thing I haven't fully understood is COPY-related - so it might be
good to extend a bit. And how COPY relates to JSON output format?

pglogical_output_plhooks.c
+#if PG_VERSION_NUM >= 90500
+   username = GetUserNameFromId(GetUserId(), false);
+#else
+   username = GetUserNameFromId(GetUserId());
+#endif

Is it needed? I mean - will tris module compiled for 9.4 (or earlier)
versions, or will it be 9.5 (or even 9.6)+ only?

pglogical_output.c
+   /*
+* Will we forward changesets? We have to if we're on 9.4;
+* otherwise honour the client's request.
+*/
+   if (PG_VERSION_NUM/100 == 904)
+   {
+   /*
+* 9.4 unconditionally forwards changesets due to lack 
of
+* replication origins, and it can't ever send origin 
info
+* for the same reason.
+*/

Similar case. In mail from 2015-11-12 (path v2) you removed v9.4 compatibility,
so I'm not sure whether checking for 9.4 or 9.5 makes any sense now.

This review focuses mostly on documentation, but I went through both
documentation and code. I understood most of the code (and it makes
sense after some cosideration :-) ), but I'm not proficient in PostgreSQL
to be fully sure that there are no hidden bugs.
At the same time - I haven't seen problems and suspicious fragments of code,
so after fixing mentioned problems it should go to the commiter.

Best regards.


The new status of this patch is: Waiting on Author


-- 
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] Some 9.5beta2 backend processes not terminating properly?

[Tom Lane]

Andres Freund  writes:
> On January 3, 2016 7:04:29 PM GMT+01:00, Tom Lane  wrote:
>> Um.  For something that at least potentially has portability issues
>> (we think not, but we could be wrong), it's pretty scary to push only
>> a couple of hours before the wrap deadline.  I'd like to get it done
>> now so we can get a full day's buildfarm testing.  If you like I can
>> push something ...

> I agree. But I'm at an event and only have my phone with me. So I can't get 
> to it right now. I might be able to push in 5ish hours. Otherwise it's in 
> 15h. Sorry.

> Fell free to push earlier, if you can make the time.

Done, we'll soon see what the buildfarm thinks.

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] dynloader.h missing in prebuilt package for Windows?

[Tom Lane]

Robert Haas  writes:
> On Dec 31, 2015, at 1:04 PM, Bruce Momjian  wrote:
>> Let's hold this for 9.5.1 and all minor releases will get it at the same
>> time.

> I vote for going ahead with this at once. It seems low risk, and having 9.5.1 
> install different files from 9.5.0 seems like an unnecessary annoyance.

If we're willing to allow 9.4.6 to install different files than 9.4.5
does, I don't see why it's a problem for 9.5.1.  But having said that,
I agree that this seems pretty low-risk, and so IMO we might as well
ship it sooner not later.

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] \x auto and EXPLAIN

[Tom Lane]

Andreas Karlsson  writes:
> psql's "\x auto" is a nice feature, but it is made much less useful in 
> my opinion due to the expanded output format making query plans 
> unreadable (and query plans often end up using expanded display due to 
> their width). I think we should never use the expanded format for 
> EXPLAIN output in the "\x auto" mode, since even when the wrapped format 
> is used the query plans are very hard to read.

> I see two ways to fix this.

> 1) Never use expanded display for the case where there is only one 
> column. There seems to me like there is little value in using expanded 
> display for when you only have one column, but I may be missing some use 
> case here.

> 2) Explicitly detect (for example based on the headers) that the result 
> is a query plan and if so disable expanded display.

The second of these seems pretty bletcherous --- for one thing, it might
fall foul of localization attempts.  However, I could see the argument
for not using expanded mode for any single-column output.

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] Some 9.5beta2 backend processes not terminating properly?

[Andres Freund]

On January 3, 2016 6:23:20 PM GMT+01:00, Tom Lane  wrote:

>> I really think we have a host of buggy code around the event handling
>-
>> but most of it has been used for a long while. So I think fixing the
>0
>> byte case for 9.5 is good enough.
>
>Agreed.  Let's do it and ship this puppy.

Unless somebody beats me to it, I'll push in the European morning.

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] Some 9.5beta2 backend processes not terminating properly?

[Tom Lane]

Andres Freund  writes:
> On 2016-01-03 10:03:41 +0530, Amit Kapila wrote:
>> I think this true for a TCP socket, but this code-path is used for UDP
>> (SOCK_DGRAM) sockets as well and there is a comment below in
>> that function which seems to be indicating why originally 0 byte case
>> has not been handled at the place suggested by you (now it seems to
>> be much less relevant).

> I'm not sure what the origin of that comment is, it's been there all the
> way since a4c40f14. But it doesn't really have much real effect: If
> WSARecv in the retry loop returns 0 bytes, we'll not retry again as r !=
> SOCKET_ERROR but actually return 0.

That comment is a bit scary because it suggests that there might be some
cross-Windows-versions differences in the behavior of WSARecv.  However,
I poked around on msdn.microsoft.com until I found a version of the
WSARecv man page that claimed to apply to Windows 2000, and it says the
same thing as the newer versions: b==0 implies graceful connection closure
(if stream protocol) or zero-size message (if message-oriented protocol)
and in neither case is it appropriate for this code to block.

So I think the exclusion of zero is an outright bug and always has been.

Actually, we could remove the test on b altogether and then simplify the
next line; I see no indication in Microsoft's docs that b<0 is a possible
case.  That would make the code here more nearly match what is in the
retry loop --- and we now realize that it's only because we used to fall
through to the retry loop that the EOF case behaved sanely.

> I really think we have a host of buggy code around the event handling -
> but most of it has been used for a long while. So I think fixing the 0
> byte case for 9.5 is good enough.

Agreed.  Let's do it and ship this puppy.

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