Re: [HACKERS] Allowing NOT IN to use ANTI joins

2014-06-11 Thread David Rowley
On Mon, Jun 9, 2014 at 11:20 PM, Marti Raudsepp ma...@juffo.org wrote:

 On Sun, Jun 8, 2014 at 3:36 PM, David Rowley dgrowle...@gmail.com wrote:
  Currently pull_up_sublinks_qual_recurse only changes the plan for NOT
 EXISTS
  queries and leaves NOT IN alone. The reason for this is because the
 values
  returned by a subquery in the IN clause could have NULLs.

 I believe the reason why this hasn't been done yet, is that the plan
 becomes invalid when another backend modifies the nullability of the
 column. To get it to replan, you'd have to introduce a dependency on
 the NOT NULL constraint, but it's impossible for now because there's
 no pg_constraint entry for NOT NULLs.

 The only way to consistently guarantee nullability is through primary
 key constraints. Fortunately that addresses most of the use cases of
 NOT IN(), in my experience.


I tried to break this by putting a break point
in convert_ANY_sublink_to_join in session 1. Not that it really had to be
in that function, I just wanted it to stop during planning and before the
plan is executed.

-- session 1
select * from n1 where id not in(select id from n1); -- hits breakpoint in
convert_ANY_sublink_to_join

-- session 2
alter table n2 alter column id drop not null;

insert into n2 values(null);

I see that session 2 blocks in the alter table until session 1 completes.

I've not really checked out the code in detail around when the snapshot is
taken and the transaction ID is generated, but as long as the transaction
id is taken before we start planning in session 1 then it should not matter
if another session drops the constraint and inserts a NULL value as we
won't see that NULL value in our transaction... I'd assume that the
transaction has to start before it grabs the table defs that are required
for planning. Or have I got something wrong?



 See the comment in check_functional_grouping:

  * Currently we only check to see if the rel has a primary key that is a
  * subset of the grouping_columns.  We could also use plain unique
 constraints
  * if all their columns are known not null, but there's a problem: we need
  * to be able to represent the not-null-ness as part of the constraints
 added
  * to *constraintDeps.  FIXME whenever not-null constraints get represented
  * in pg_constraint.


I saw that, but I have to say I've not fully got my head around why that's
needed just yet.


 The behavior you want seems somewhat similar to
 check_functional_grouping; maybe you could unify it with your
 targetListIsGuaranteedNotToHaveNulls at some level. (PS: that's one
 ugly function name :)


Agreed :)  Originally I had put the code that does that
in convert_ANY_sublink_to_join, but at the last minute before posting the
patch I decided that it might be useful and reusable so moved it out to
that function. I'll try and think of something better, but I'm open to
ideas.

Regards

David Rowley


Re: [HACKERS] Allowing NOT IN to use ANTI joins

2014-06-11 Thread David Rowley
On Tue, Jun 10, 2014 at 2:19 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Jeff Janes jeff.ja...@gmail.com writes:
  If you are using NOT IN, then once you find a NULL in the outer input (if
  the outer input is the in-list: clearly you can't reverse the two inputs
 in
  this case), you don't even need to finish reading the outer input, nor
  start reading the inner input, because all rows are automatically
 excluded
  by the weird semantics of NOT IN.

 The point I'm trying to make is that the goal of most join types is to
 give an answer without having necessarily read all of either input.
 For instance, if we tried to do this with a mergejoin it wouldn't work
 reliably: it might suppose that it could accept an outer row on the basis
 of no match in a higher-order sort column before it'd reached any nulls
 appearing in lower-order sort columns.

 You might be right that we could hot-wire the hash join case in
 particular, but I'm failing to see the point of expending lots of extra
 effort in order to deliver a useless answer faster.  If there are NULLs
 in the inner input, then NOT IN is simply the wrong query to make, and
 giving an empty output in a relatively short amount of time isn't going
 to help clue the user in on that.  (If the SQL standard would let us do
 so, I'd be arguing for throwing an error if we found a NULL.)


This got me thinking. It is probably a bit useless and wrong to perform a
NOT IN when the subquery in the IN() clause can have NULL values, so I
guess in any realistic useful case, the user will either have a NOT NULL
constraint on the columns, or they'll do a WHERE col IS NOT NULL, so I
should likely also allow a query such as:

SELECT * FROM a WHERE id NOT IN(SELECT nullable_col FROM b WHERE
nullable_col IS NOT NULL);

to also perform an ANTI JOIN. I think it's just a matter of
changing targetListIsGuaranteedNotToHaveNulls so that if it does not find
the NOT NULL constraint, to check the WHERE clause of the query to see if
there's any not null quals.

I'm about to put this to the test, but if it works then I think it should
cover many more cases for using NOT IN(), I guess we're only leaving out
function calls and calculations in the target list.

Regards

David Rowley


Re: [HACKERS] Allowing NOT IN to use ANTI joins

2014-06-11 Thread Marti Raudsepp
On Wed, Jun 11, 2014 at 11:53 AM, David Rowley dgrowle...@gmail.com wrote:
 The only way to consistently guarantee nullability is through primary
 key constraints. Fortunately that addresses most of the use cases of
 NOT IN(), in my experience.

 See the comment in check_functional_grouping:

 I saw that, but I have to say I've not fully got my head around why that's
 needed just yet.

I was wrong, see Tom's reply to my email. It's OK to rely on
attnotnull for optimization decisions. The plan will be invalidated
automatically when the nullability of a referenced column changes.

check_functional_grouping needs special treatment because it decides
whether to accept/reject views; and if it has allowed creating a view,
it needs to guarantee that the dependent constraint isn't dropped for
a longer term.

 as long as the transaction id
 is taken before we start planning in session 1 then it should not matter if
 another session drops the constraint and inserts a NULL value as we won't
 see that NULL value in our transaction... I'd assume that the transaction
 has to start before it grabs the table defs that are required for planning.
 Or have I got something wrong?

1. You're assuming that query plans can only survive for the length of
a transaction. That's not true, prepared query plans can span many
transactions.

2. Also a FOR UPDATE clause can return values from the future, if
another transaction has modified the value and already committed.

But this whole issue is moot anyway, the plan will get invalidated
when the nullability changes.

Regards,
Marti


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


Re: [HACKERS] Allowing NOT IN to use ANTI joins

2014-06-11 Thread Marti Raudsepp
On Sun, Jun 8, 2014 at 3:36 PM, David Rowley dgrowle...@gmail.com wrote:
 Currently pull_up_sublinks_qual_recurse only changes the plan for NOT EXISTS
 queries and leaves NOT IN alone. The reason for this is because the values
 returned by a subquery in the IN clause could have NULLs.

There's a bug in targetListIsGuaranteedNotToHaveNulls, you have to
drill deeper into the query to guarantee the nullability of a result
column. If a table is OUTER JOINed, it can return NULLs even if the
original column specification has NOT NULL.

This test case produces incorrect results with your patch:

create table a (x int not null);
create table b (x int not null, y int not null);
insert into a values(1);
select * from a where x not in (select y from a left join b using (x));

Unpatched version correctly returns 0 rows since y will be NULL.
Your patch returns the value 1 from a.

Regards,
Marti


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


Re: [HACKERS] Allowing NOT IN to use ANTI joins

2014-06-11 Thread David Rowley
On Wed, Jun 11, 2014 at 9:32 PM, Marti Raudsepp ma...@juffo.org wrote:

 On Sun, Jun 8, 2014 at 3:36 PM, David Rowley dgrowle...@gmail.com wrote:
  Currently pull_up_sublinks_qual_recurse only changes the plan for NOT
 EXISTS
  queries and leaves NOT IN alone. The reason for this is because the
 values
  returned by a subquery in the IN clause could have NULLs.

 There's a bug in targetListIsGuaranteedNotToHaveNulls, you have to
 drill deeper into the query to guarantee the nullability of a result
 column. If a table is OUTER JOINed, it can return NULLs even if the
 original column specification has NOT NULL.

 This test case produces incorrect results with your patch:

 create table a (x int not null);
 create table b (x int not null, y int not null);
 insert into a values(1);
 select * from a where x not in (select y from a left join b using (x));

 Unpatched version correctly returns 0 rows since y will be NULL.
 Your patch returns the value 1 from a.


Thanks, I actually was just looking at that. I guess I'll just need to make
sure that nothing in the targetlist comes from an outer join.

Regards

David Rowley


[HACKERS] replication commands and log_statements

2014-06-11 Thread Fujii Masao
Hi,

Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?

Regards,

-- 
Fujii Masao


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


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

2014-06-11 Thread Fujii Masao
On Wed, Jun 11, 2014 at 10:05 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Tue, Jun 10, 2014 at 11:49 PM, Rahila Syed rahilasye...@gmail.com wrote:
 Hello ,


 In order to facilitate changing of compression algorithms  and to be able to
 recover using WAL records compressed with different compression algorithms,
 information about compression algorithm can be stored in WAL record.

 XLOG record header has 2 to 4 padding bytes in order to align the WAL
 record. This space can be used for  a new flag in order to store information
 about the compression algorithm used. Like the xl_info field of XlogRecord
 struct,  8 bits flag  can be constructed with the lower 4 bits of the flag
 used to indicate which backup block is compressed out of 0,1,2,3. Higher
 four bits can be used to indicate state of compression i.e
 off,lz4,snappy,pglz.

 The flag can be extended to incorporate more compression algorithms added in
 future if any.

 What is your opinion on this?
 -1 for any additional bytes in WAL record to control such things,
 having one single compression that we know performs well and relying
 on it makes the life of user and developer easier.

IIUC even when we adopt only one algorithm, additional at least one bit is
necessary to see whether this backup block is compressed or not.

This flag is necessary only for backup block, so there is no need to use
the header of each WAL record. What about just using the backup block
header?

Regards,

-- 
Fujii Masao


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


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

2014-06-11 Thread Pavan Deolasee
On Wed, Jun 11, 2014 at 4:19 PM, Fujii Masao masao.fu...@gmail.com wrote:


 IIUC even when we adopt only one algorithm, additional at least one bit is
 necessary to see whether this backup block is compressed or not.

 This flag is necessary only for backup block, so there is no need to use
 the header of each WAL record. What about just using the backup block
 header?


+1. We can also steal a few bits from ForkNumber field in the backup block
header if required.

Thanks,
Pavan

-- 
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
On 2014-06-11 19:34:25 +0900, Fujii Masao wrote:
 Hi,
 
 Replication commands like IDENTIFY_COMMAND are not logged even when
 log_statements is set to all. Some users who use log_statements to
 audit *all* statements might dislike this current situation. So I'm
 thinking to change log_statements or add something like log_replication
 so that we can log replication commands. Thought?

Yes, I think we should do that. I can't really see it causing too much
volume...

Greetings,

Andres Freund

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


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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Magnus Hagander
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao masao.fu...@gmail.com wrote:

 Hi,

 Replication commands like IDENTIFY_COMMAND are not logged even when
 log_statements is set to all. Some users who use log_statements to
 audit *all* statements might dislike this current situation. So I'm
 thinking to change log_statements or add something like log_replication
 so that we can log replication commands. Thought?


+1. I think adding a separate parameter is the way to go.

The other option would be to turn log_statements into a parameter that you
specify multiple ones - so instead of all today it would be ddl,dml,all
or something like that, and then you'd also add replication as an option.
But that would cause all sorts of backwards compatibility annoyances.. And
do you really want to be able to say things like ddl,all meanin you'd get
ddl and select but not dml?


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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote:
 On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao masao.fu...@gmail.com wrote:
 
  Hi,
 
  Replication commands like IDENTIFY_COMMAND are not logged even when
  log_statements is set to all. Some users who use log_statements to
  audit *all* statements might dislike this current situation. So I'm
  thinking to change log_statements or add something like log_replication
  so that we can log replication commands. Thought?
 
 
 +1. I think adding a separate parameter is the way to go.

Why? I can't really see a case where the log volume by replication
connections is going to be significant in comparison to 'all'?

Greetings,

Andres Freund

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


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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Magnus Hagander
On Wed, Jun 11, 2014 at 2:17 PM, Andres Freund and...@2ndquadrant.com
wrote:

 On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote:
  On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao masao.fu...@gmail.com
 wrote:
 
   Hi,
  
   Replication commands like IDENTIFY_COMMAND are not logged even when
   log_statements is set to all. Some users who use log_statements to
   audit *all* statements might dislike this current situation. So I'm
   thinking to change log_statements or add something like log_replication
   so that we can log replication commands. Thought?
  
 
  +1. I think adding a separate parameter is the way to go.

 Why? I can't really see a case where the log volume by replication
 connections is going to be significant in comparison to 'all'?


Yes, but how would you specify for example i want DDL and all replication
commands (which is quite a reasonable thing to log, I believe). If you
actually require it to be set to all, most people won't have any use at
all for it...

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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote:
 Yes, but how would you specify for example i want DDL and all replication
 commands (which is quite a reasonable thing to log, I believe). If you
 actually require it to be set to all, most people won't have any use at
 all for it...

That's a reasonable feature request - but imo it's a separate
one. It seems pretty noncontroversial and simple to make
log_statement=all log replication commands. What you want - and you're
sure not alone with that - is something considerably more complex...

Greetings,

Andres Freund

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


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


Re: [HACKERS] tests for client programs

2014-06-11 Thread Andres Freund
Hi,

On 2014-06-04 20:40:40 -0400, Peter Eisentraut wrote:
 On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote:
   As an additional issue it currently doesn't seem to work in VPATH
   builds. That's imo a must fix.
  
  A cd $(srcdir)  .. in prove_installcheck and prove_check seems to do
  the trick.
 
 Here is my proposed patch for this.

Works here. The tmpdir thing is probably a separate patch...

Greetings,

Andres Freund

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


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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Magnus Hagander
On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund and...@2ndquadrant.com
wrote:

 On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote:
  Yes, but how would you specify for example i want DDL and all
 replication
  commands (which is quite a reasonable thing to log, I believe). If you
  actually require it to be set to all, most people won't have any use at
  all for it...

 That's a reasonable feature request - but imo it's a separate
 one. It seems pretty noncontroversial and simple to make
 log_statement=all log replication commands. What you want - and you're
 sure not alone with that - is something considerably more complex...


I'm just saying if we're going to do it, let's do it right from the
beginning.

Or are you suggesting this would be something we'd backpatch? Given the
lack of complaints about it, I'd rather suggest we don't backpatch
anything, and instead make the correct feature in the first pace for the
next release.

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


Re: [HACKERS] Inaccuracy in VACUUM's tuple count estimates

2014-06-11 Thread Andres Freund
On 2014-06-09 11:24:22 -0700, Kevin Grittner wrote:
 Andres Freund and...@2ndquadrant.com wrote:
  On 2014-06-09 09:45:12 -0700, Kevin Grittner wrote:
 
  I am not sure, given predicate.c's coding, how
  HEAPTUPLE_DELETE_IN_PROGRESS could cause problems. Could you elaborate,
  since that's the contentious point with Tom? Since 'both in
  progress'
  can only happen if xmin and xmax are the same toplevel xid and you
  resolve subxids to toplevel xids I think it should currently be safe
  either way?
 
 The only way that it could be a problem is if the DELETE is in a
 subtransaction which might get rolled back without rolling back the
 INSERT.

The way I understand the code in that case the subxid in xmax would have
been resolved the toplevel xid.

/*
 * Find top level xid.  Bail out if xid is too early to be a conflict, 
or
 * if it's our own xid.
 */
if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
return;
xid = SubTransGetTopmostTransaction(xid);
if (TransactionIdPrecedes(xid, TransactionXmin))
return;
if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
return;

That should essentially make that case harmless, right? So it seems the
optimization (and pessimization in other cases) of only tracking
toplevel xids seems to save the day here?

  If we ignore the conflict because we assume the INSERT
 will be negated by the DELETE, and that doesn't happen, we would
 get false negatives which would compromise correctness.  If we
 assume that the DELETE might not happen when the DELETE is not in a
 separate subtransaction we might get a false positive, which would
 only be a performance hit.  If we know either is possible and have
 a way to check in predicate.c, it's fine to check it there.

Given the above I don't think this currently can happen. Am I understand
it correctly? If so, it certainly deserves a comment...

Greetings,

Andres Freund

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


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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
On 2014-06-11 14:50:34 +0200, Magnus Hagander wrote:
 On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund and...@2ndquadrant.com
 wrote:
 
  On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote:
   Yes, but how would you specify for example i want DDL and all
  replication
   commands (which is quite a reasonable thing to log, I believe). If you
   actually require it to be set to all, most people won't have any use at
   all for it...
 
  That's a reasonable feature request - but imo it's a separate
  one. It seems pretty noncontroversial and simple to make
  log_statement=all log replication commands. What you want - and you're
  sure not alone with that - is something considerably more complex...
 
 

 I'm just saying if we're going to do it, let's do it right from the
 beginning.

Your wish just seems like a separate feature to me. Including
replication commands in 'all' seems correct independent of the desire
for a more granular control.

 Or are you suggesting this would be something we'd backpatch?

Thought about it for a sec, and then decided it doesn't seem warranted.

Greetings,

Andres Freund

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


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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-11 Thread Fujii Masao
On Tue, Jun 10, 2014 at 5:01 PM,  furu...@pm.nttdata.co.jp wrote:
 No. IIUC walreceiver does flush *less* frequently than what you
 implemented on pg_receivexlog. Your version of pg_receivexlog tries to
 do flush every time when it receives one WAL chunk. OTOH, walreceiver
 does flush only when there is no extra WAL chunk in receive buffer. IOW,
 after writing WAL chunk, if there is another WAL chunk that walreceiver
 can receive immediately, it postpones flush later.

  However, it seems difficult to apply as same way.

 Why? ISTM that's not so difficult.

 I was not able to understand movement of walreceiver well.
 While walreceiver writes data, do PQconsumeInput() by omitting the select().
 Do flush if the PQgetCopyData has been to return the zero continuously.
 Fixed to the same process using the flag.

You introduced the state machine using the flag flush_flg into pg_receivexlog.
That's complicated and would reduce the readability of the source code. I think
that the logic should be simpler like walreceiver's one.

Maybe I found one problematic path as follows:

1. WAL is written and flush_flag is set to 1
2. PQgetCopyData() returns 0 and flush_flg is incremented to 2
3. PQconsumeInput() is executed
4. PQgetCopyData() reads keepalive message
5. After processing keepalive message, PQgetCopyDate() returns 0
6. Since flush_flg is 2, WAL is flushed and flush_flg is reset to 0

But new message can arrive while processing keepalive message. Before
flushing WAL, such new message should be processed.

+Enables synchronous mode. If replication slot is disabled then
+this setting is irrelevant.

Why is that irrelevant in that case?

Even when replication slot is not used, thanks to this feature, pg_receivexlog
can flush WAL more proactively and which may improve the durability of WAL
which pg_receivexlog writes.

+printf(_(  -m, --sync-modesynchronous mode\n));

I think that calling this feature synchronous mode is confusing.

Regards,

-- 
Fujii Masao


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


Re: [HACKERS] Inaccuracy in VACUUM's tuple count estimates

2014-06-11 Thread Kevin Grittner
Andres Freund and...@2ndquadrant.com wrote:
 On 2014-06-09 11:24:22 -0700, Kevin Grittner wrote:

 The only way that it could be a problem is if the DELETE is in a
 subtransaction which might get rolled back without rolling back the
 INSERT.

 The way I understand the code in that case the subxid in xmax would have
 been resolved the toplevel xid.

 /*
 * Find top level xid.  Bail out if xid is too early to be a conflict, or
 * if it's our own xid.
 */
 if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
 return;
 xid = SubTransGetTopmostTransaction(xid);
 if (TransactionIdPrecedes(xid, TransactionXmin))
 return;
 if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
 return;

 That should essentially make that case harmless, right?

Hmm.  Since the switch statement above doesn't limit the
HEAPTUPLE_DELETE_IN_PROGRESS or HEAPTUPLE_INSERT_IN_PROGRESS cases
by visibility, we are safe.  I had been thinking that we had early
returns based on visibility, like the we do for HEAPTUPLE_LIVE and
HEAPTUPLE_RECENTLY_DEAD.  Since we don't do that, there is no
problem with the code either before or after your recent change. 
Apologies that I didn't notice that sooner.

It might be possible that with more guarantees of what those return
codes mean (possibly by adding the new one mentioned by Tom) we
could add that early return in one or both cases, which would
better optimize some corner cases involving subtransactions.  But
now doesn't seem like a good time to worry about that.  Such a
micro-optimization would not be a sane candidate for back-patching,
or for introducing this late in a release cycle.

So, nothing to see here, folks.  Move along.  predicate.c is
neutral in this matter.

Good spot, BTW!

--
Kevin Grittner
EDB: 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] [GENERAL] Question about partial functional indexes and the query planner

2014-06-11 Thread Robert Haas
On Tue, Jun 10, 2014 at 7:19 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Given the lack of previous complaints, I'm not sure this amounts to
 a back-patchable bug, but it does seem like something worth fixing
 going forward.

Agreed, although I'd be willing to see us slip it into 9.4.  It's
doubtful that anyone will get upset if their query plans change
between beta1 and beta2, but the same cannot be said for released
branches.

-- 
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] Allowing NOT IN to use ANTI joins

2014-06-11 Thread Tom Lane
Marti Raudsepp ma...@juffo.org writes:
 On Wed, Jun 11, 2014 at 11:53 AM, David Rowley dgrowle...@gmail.com wrote:
 as long as the transaction id
 is taken before we start planning in session 1 then it should not matter if
 another session drops the constraint and inserts a NULL value as we won't
 see that NULL value in our transaction... I'd assume that the transaction
 has to start before it grabs the table defs that are required for planning.
 Or have I got something wrong?

 1. You're assuming that query plans can only survive for the length of
 a transaction. That's not true, prepared query plans can span many
 transactions.

 2. Also a FOR UPDATE clause can return values from the future, if
 another transaction has modified the value and already committed.

 But this whole issue is moot anyway, the plan will get invalidated
 when the nullability changes.

Right.  The key point for David's concern is that we always hold (at
least) AccessShareLock on every relation used in a query, continuously
from rewrite through to the end of execution.  This will block any attempt
by other transactions to make schema changes in the relation(s).
In the case of re-using a prepared plan, we re-acquire all these locks
and then check to see if we received any invalidation messages that
render the plan invalid; if not, we can proceed to execution with the same
safety guarantees as originally.  (If we did, we replan starting from the
raw parse tree.)

If we didn't have mechanisms like this, we'd have far worse hazards from
ALTER TABLE than whether the planner made an incorrect join optimization.
Consider ALTER COLUMN TYPE for instance.

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] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Robert Haas
On Tue, Jun 10, 2014 at 7:18 PM, Craig Ringer cr...@2ndquadrant.com wrote:
 On 06/11/2014 02:19 AM, Tom Lane wrote:
 Hm ... I'm not following why we'd need a special case for superusers and
 not anyone else?  Seems like any useful RLS scheme is going to require
 more privilege levels than just superuser and not-superuser.

 What it really needs is to invalidate plans when switching between
 RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an RLS
 exempt right or mode sooner rather than later, so I'm against tying
 this explicitly to superuser as such.

 I wouldn't be surprised to see

 SET ROW SECURITY ON|OFF

 down the track, with a right controlling whether you can or not. Or at
 least, a right that directly exempts a user from row security.

I'm really concerned about the security implications of this patch.  I
think we're setting ourselves up for a whole lot of hurt for somewhat
unclear gain.

In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically
*is* row-level security: instead of applying a row-level security
policy to a table, just create a security-barrier view over the table
and grant access to the view.  Forget that the table ever existed.
Done.

With this approach, there's a lot of stuff that we don't have to
reinvent.  We've talked a lot about whether row-level security should
only be concerned with the rows it scans, or whether it should also
restrict the new rows that can be created.  You can get either
behavior by choosing whether or not to use WITH CHECK OPTION.  And
then there's this question of who should be RLS-exempt; that's
basically a question of to whom you grant privileges on the underlying
table.  Note that this can be very fine-grained: for example, you can
allow someone to exempt themselves for selects but not for updates by
granting them SELECT privileges but not UPDATE privileges on the
underlying table.  And potentially-exempt users can choose whether
they want a particular access to actually be exempt by targeting the
view when they don't want to be exempt and the table when they do.
That's mighty useful for debugging, at least IMHO.  And, if you want
to have several row-level security policies for different classes of
users, just create more than one view and grant different privileges
on each.

By contrast, it seems to me that every design so far proposed for
something that is actually called row-level security - as opposed to
commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is*
row-level security, is extremely limited.  Look back at all the things
listed in the previous paragraph; can you do those things easily with
the designs that have been proposed?  As far as I can see, not really.
Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough
equivalent of WITH CHECK OPTION, probably because we've talked a lot
about that specific issue, but it doesn't line up exactly to what WITH
CHECK OPTION actually does.  There's no independently-grantable
RLS-exemption privilege - and even when we talk about that, it's
usually some kind of global bit that applies to all tables and all
operations equally - whereas with the above approach it can be
per-table and per-operation and doesn't require superuser intervention
to flip the bit.  There's no way for users who are RLS exempt to turn
off their exemption for testing purposes, let alone on a per-table
basis.  There's no way to have multiple RLS policies on a single
table.  All of those are things that we get for free in the
view-over-table model, and implementing formal RLS basically requires
us to either invent a new RLS-specific way of doing each of those
things, or suffer along with a subset of the functionality.  Yuck.

But what's really awful about this whole design is that it breaks the
invariant that reading from a table doesn't run anybody else's code.
It's already the case that users need to be awfully careful about
modifying tables, because that might fire triggers that do bad things.
But at least you can SELECT from a table and it will either work, or
it will fail with a permission denied error.  What it will not do is
unexpectedly run some code that you weren't expecting it to run.  You
can't be so blithe about selecting from views, but reading a plain
table is always OK.  Now, as soon as we introduce the concept that
selecting from a table might not really mean read from the table but
read from the table after applying this owner-specified qual, we're
opening up a whole new set of attack surfaces.  Every pg_dump is an
opportunity to hack somebody else's account, or at least audit their
activity.  Protecting the superuser against everybody else is nice,
but I think it's just as important to protect non-superusers against
each other, and I think that's going to be hard -- because in the RLS
world, SELECT * FROM tab is now *fundamentally* ambiguous.  Maybe it's
reading from the table, and maybe it's really clandestinely reading
from a view over the table, and the user has no way 

Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 Your wish just seems like a separate feature to me. Including
 replication commands in 'all' seems correct independent of the desire
 for a more granular control.

No, I think I've got to vote with the other side on that.

The reason we can have log_statement as a scalar progression
none  ddl  mod  all is that there's little visible use-case
for logging DML but not DDL, nor for logging SELECTS but not
INSERT/UPDATE/DELETE.  However, logging replication commands seems
like something people would reasonably want an orthogonal control for.
There's no nice way to squeeze such a behavior into log_statement.

I guess you could say that log_statement treats replication commands
as if they were DDL, but is that really going to satisfy users?

I think we should consider log_statement to control logging of
SQL only, and invent a separate GUC (or, in the future, likely
more than one GUC?) for logging of replication activity.

regards, tom lane


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


Re: [HACKERS] Proposing pg_hibernate

2014-06-11 Thread Robert Haas
On Tue, Jun 10, 2014 at 10:03 PM, Gurjeet Singh gurj...@singh.im wrote:
 And it's probably accepted by now that such a bahviour is not
 catastrophic, merely inconvenient.

I think the whole argument for having pg_hibernator is that getting
the block cache properly initialized is important.  If it's not
important, then we don't need pg_hibernator in the first place.  But
if it is important, then I think not loading unrelated blocks into
shared_buffers is also important.

-- 
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] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Stephen Frost
* Craig Ringer (cr...@2ndquadrant.com) wrote:
 On 06/11/2014 02:19 AM, Tom Lane wrote:
  Hm ... I'm not following why we'd need a special case for superusers and
  not anyone else?  Seems like any useful RLS scheme is going to require
  more privilege levels than just superuser and not-superuser.
 
 What it really needs is to invalidate plans when switching between
 RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an RLS
 exempt right or mode sooner rather than later, so I'm against tying
 this explicitly to superuser as such.

That certainly sounds reasonable to me, but the point is we're just
looking to see if the current role executing the plan should or should
not have RLS applied and, if it's changing, we need to re-plan.  We
don't need to actually track an independent plan for each and every user
executing the plan, which means that the plan cache can be largely left
alone.

 I wouldn't be surprised to see
 
 SET ROW SECURITY ON|OFF
 
 down the track, with a right controlling whether you can or not. Or at
 least, a right that directly exempts a user from row security.

Agreed, but doing a re-planning in that case seems reasonable to me.  I
find it pretty unlikely that there will be a lot of critical path cases
of the same plan flipping back and forth between a role for which RLS is
applied and a role where it shouldn't be.

  Could we put the if superuser then ok test into the RLS condition test
  and thereby not need more than one plan at all?
 
 Only if we put it in another level of security barrier subquery, because
 otherwise the planner might execute the other quals (including possible
 user defined functions) before the superuser test. Which was the whole
 reason for the superuser test in the first place.

Yeah, I'm not a big fan of this and it certainly seems a simpler
approach to just force a re-plan.  We're talking about a query which
has been prepared and then is being executed by different roles, some
of which are RLS enabled and some which are RLS exempt.  That just
strikes me as pretty unlikely to happen and if it does become an issue,
a user could work around it by having two different plans prepared and
making sure that they are called from the appropriate roles to avoid the
replanning.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Stephen Frost
* Craig Ringer (cr...@2ndquadrant.com) wrote:
 On 06/11/2014 07:24 AM, Tom Lane wrote:
  Is the point of that that the table owner might have put trojan-horse
  functions into the RLS qual?  If so, why are we only concerned about
  defending the superuser and not other users?  Seems like the right fix
  would be to insist that functions in the RLS qual run as the table owner.
  Granted, that might be painful to do.  But it still seems like we only
  need to do this for superusers is designing with blinkers on.
 
 I agree, and now that the urgency of trying to deliver this for 9.4 is
 over it's worth seeing if we can just run as table owner.

We'll need to work out how to ensure that things like current_user()
still returns the calling user in that case, otherwise it won't make any
sense.  In general, I agree that having the RLS quals run as the table
owner is a good approach and would love to hear suggestions about how we
can make that happen.

 Failing that, we could take the approach a certain other RDBMS does and
 make the ability to define row security quals a GRANTable right
 initially held only by the superuser.

I don't particularly like this idea- it's akin, to me anyway, to making
the ability to control other permissions on a table (SELECT, INSERT,
etc) something which a user would have to be granted- and it doesn't
really address the issue.

Thanks,

Stephen


signature.asc
Description: Digital signature


[HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

2014-06-11 Thread Fabrízio de Royes Mello
Hi all,

As part of GSoC2014 I'm sending a patch to add the capability of change an
unlogged table to logged [1].

I'll add it to the 9.5CF1.

Regards,


[1]
https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 69a1e14..f822375 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable
 ENABLE REPLICA RULE replaceable class=PARAMETERrewrite_rule_name/replaceable
 ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable
 CLUSTER ON replaceable class=PARAMETERindex_name/replaceable
+SET LOGGED
 SET WITHOUT CLUSTER
 SET WITH OIDS
 SET WITHOUT OIDS
@@ -432,6 +433,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable
/varlistentry
 
varlistentry
+termliteralSET LOGGED/literal/term
+listitem
+ para
+  This form change the table persistence type from unlogged to permanent by
+  rewriting the entire contents of the table and associated indexes into  
+  new disk files.
+ /para
+ para
+  Changing the table persistence type acquires an literalACCESS EXCLUSIVE/literal lock.
+ /para
+/listitem
+   /varlistentry
+
+   varlistentry
 termliteralSET WITHOUT CLUSTER/literal/term
 listitem
  para
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 341262b..f378f6a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -136,7 +136,8 @@ static List *on_commits = NIL;
 #define AT_PASS_ADD_INDEX		6		/* ADD indexes */
 #define AT_PASS_ADD_CONSTR		7		/* ADD constraints, defaults */
 #define AT_PASS_MISC			8		/* other stuff */
-#define AT_NUM_PASSES			9
+#define AT_PASS_SET_LOGGED		9		/* SET LOGGED */
+#define AT_NUM_PASSES			10
 
 typedef struct AlteredTableInfo
 {
@@ -376,6 +377,7 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD
 static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
 	 char *cmd, List **wqueue, LOCKMODE lockmode,
 	 bool rewrite);
+static void ATPostAlterSetLogged(Oid relid);
 static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
 static void TryReuseForeignKey(Oid oldId, Constraint *con);
 static void change_owner_fix_column_acls(Oid relationOid,
@@ -402,6 +404,8 @@ static void ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockm
 static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
 static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
 static void ATExecGenericOptions(Relation rel, List *options);
+static void ATPrepSetLogged(Relation rel);
+static void ATExecSetLogged(Relation rel);
 
 static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
    ForkNumber forkNum, char relpersistence);
@@ -2855,6 +2859,7 @@ AlterTableGetLockLevel(List *cmds)
 			case AT_AddIndexConstraint:
 			case AT_ReplicaIdentity:
 			case AT_SetNotNull:
+			case AT_SetLogged:
 cmd_lockmode = AccessExclusiveLock;
 break;
 
@@ -3246,6 +3251,11 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			/* No command-specific prep needed */
 			pass = AT_PASS_MISC;
 			break;
+		case AT_SetLogged:
+			ATSimplePermissions(rel, ATT_TABLE);	/* SET LOGGED */
+			ATPrepSetLogged(rel);
+			pass = AT_PASS_SET_LOGGED;
+			break;
 		default:/* oops */
 			elog(ERROR, unrecognized alter table type: %d,
  (int) cmd-subtype);
@@ -3308,6 +3318,9 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
 ATPostAlterTypeCleanup(wqueue, tab, lockmode);
 
 			relation_close(rel, NoLock);
+
+			if (pass == AT_PASS_SET_LOGGED)
+ATPostAlterSetLogged(RelationGetRelid(rel));
 		}
 	}
 
@@ -3527,6 +3540,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		case AT_GenericOptions:
 			ATExecGenericOptions(rel, (List *) cmd-def);
 			break;
+		case AT_SetLogged:
+			ATExecSetLogged(rel);
+			break;
 		default:/* oops */
 			elog(ERROR, unrecognized alter table type: %d,
  (int) cmd-subtype);
@@ -10420,6 +10436,175 @@ ATExecGenericOptions(Relation rel, List *options)
 }
 
 /*
+ * ALTER TABLE name SET LOGGED
+ *
+ * Change the table persistence type from unlogged to permanent by
+ * rewriting the entire contents of the table and associated indexes
+ * into new disk files.
+ *
+ * The ATPrepSetLogged function check all precondictions to perform
+ * the operation:
+ * - check if the target table is unlogged
+ * - check if not exists a foreign key to other unlogged table
+ */
+static void

Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 I'm really concerned about the security implications of this patch.  I
 think we're setting ourselves up for a whole lot of hurt for somewhat
 unclear gain.

I'm certainly of a different opinion and, for the most part, I feel that
if there are security concerns then they need to be addressed- and
better by us than by asking users to use some other mechanism to
implement RLS.

 In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically
 *is* row-level security: instead of applying a row-level security
 policy to a table, just create a security-barrier view over the table
 and grant access to the view.  Forget that the table ever existed.
 Done.

This argument could have been made for column-level privileges also, no?
Yet I don't hear any calls for that to be ripped out now that you could
implement it through updatable security-barrier views.  That commit was
the ground-work to allow us to finally get proper RLS and I'm very
disappointed to hear that the mechanical pieces around making RLS easy
for users to use (and getting that check-box taken care of in a wide
variety of fields that we are being exposed to now, see the PGConf.NYC
keynote speakers...) is receiving such push-back.

 With this approach, there's a lot of stuff that we don't have to
 reinvent.  We've talked a lot about whether row-level security should
 only be concerned with the rows it scans, or whether it should also
 restrict the new rows that can be created.  You can get either
 behavior by choosing whether or not to use WITH CHECK OPTION.  And
 then there's this question of who should be RLS-exempt; that's
 basically a question of to whom you grant privileges on the underlying
 table.  Note that this can be very fine-grained: for example, you can
 allow someone to exempt themselves for selects but not for updates by
 granting them SELECT privileges but not UPDATE privileges on the
 underlying table.  And potentially-exempt users can choose whether
 they want a particular access to actually be exempt by targeting the
 view when they don't want to be exempt and the table when they do.

I agree that views, or even security-definer functions, offer a great
deal of flexibility, and that may be necessary in some use-cases, but I
fail to see why that means we should avoid providing the mechanics to
achieve simple and usable RLS akin to what other major RDBMS's have.

 That's mighty useful for debugging, at least IMHO.  And, if you want
 to have several row-level security policies for different classes of
 users, just create more than one view and grant different privileges
 on each.

I'm really not impressed with the idea that RLS should be done with
multiple different views of the same underlying table.

 By contrast, it seems to me that every design so far proposed for
 something that is actually called row-level security - as opposed to
 commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is*
 row-level security, is extremely limited.  Look back at all the things
 listed in the previous paragraph; can you do those things easily with
 the designs that have been proposed?  As far as I can see, not really.

I don't feel that RLS will, or even *should*, have the same level of
flexibility that you can achieve with views and/or security definer
functions.  I expect that, over time, we will add more capabilities to
it, but it's never going to be able to redefine the contents of a column
as a view can, nor will it be able to add columns to a table as views
can.  I don't see those as reasons against having support for RLS.

 Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough
 equivalent of WITH CHECK OPTION, probably because we've talked a lot
 about that specific issue, but it doesn't line up exactly to what WITH
 CHECK OPTION actually does.  There's no independently-grantable
 RLS-exemption privilege - and even when we talk about that, it's
 usually some kind of global bit that applies to all tables and all
 operations equally - whereas with the above approach it can be
 per-table and per-operation and doesn't require superuser intervention
 to flip the bit.  

I'm glad to hear your thoughts on the level of granularity which might
be nice to have with RLS.  What would be great is to spend a bit more
time reviewing what other systems provide in this area and considering
what makes sense for us.  This will also be a feature and an area which
we will be improving for a long time to come, but we do need this
capability and we have to start somewhere.

 There's no way for users who are RLS exempt to turn
 off their exemption for testing purposes, let alone on a per-table
 basis.  

I don't follow this argument entirely- users can't turn off the existing
permissions system for testing either, unless an authorized user with
the correct permissions makes the change to allow it- or the user bumps
themselves up to superuser, or to a role which has broader permissions,
both of 

Re: [HACKERS] Allowing NOT IN to use ANTI joins

2014-06-11 Thread Greg Stark
On Wed, Jun 11, 2014 at 3:26 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 If we didn't have mechanisms like this, we'd have far worse hazards from
 ALTER TABLE than whether the planner made an incorrect join optimization.
 Consider ALTER COLUMN TYPE for instance.

Obviously not general cases of ALTER COLUMN TYPE but dropping a NULL
constraint seems like the kind of change targeted by Simon's reduce
lock strength patch that I'm sure he's still interested in. I think
that patch, while full of dragons to steer around, is something that
will keep coming up again and again in the future. It's a huge
operational risk that even these short exclusive locks can cause a
huge production outage if they happen to get queued up behind a
reporting query.

I don't think it changes anything for this patch -- right now the
world is arranged the way Tom described -- but it's something to keep
in mind when we talk about lock strength reduction and the impact on
existing queries. For example if there's an UPDATE query in repeatable
read mode that has an IN clause like this and was optimized
accordingly then any lock strength reduction patch would have to
beware that an ALTER TABLE that dropped the NULL clause might impact
the update query.

Incidentally, Oracle has a feature for online schema changes that we
might end up having to implement something similar. The good news is
we have the infrastructure to maybe do it. The idea is to start
capturing all the changes to the table using something like our
logical changeset extraction. Then do the equivalent of create
newtable as select ... from oldtable to create the new schema, then
start replaying the accumulated changes to the new table. Eventually
when the change queue drains then get an exclusive lock, drain any new
changes, and swap in the new table with the new schema.

-- 
greg


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


Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 * Robert Haas (robertmh...@gmail.com) wrote:
 I'm really concerned about the security implications of this patch.  I
 think we're setting ourselves up for a whole lot of hurt for somewhat
 unclear gain.

 I'm certainly of a different opinion and, for the most part, I feel that
 if there are security concerns then they need to be addressed- and
 better by us than by asking users to use some other mechanism to
 implement RLS.

TBH, I found Robert's argument pretty persuasive.  The idea that
SELECT * FROM table might invoke arbitrary processing ought to scare
anyone who's concerned about security, because that's going to completely
break any assumptions about pg_dump being safe for instance, as well as
force top-to-bottom rethinking of many other security assumptions.

 ...  That commit was
 the ground-work to allow us to finally get proper RLS and I'm very
 disappointed to hear that the mechanical pieces around making RLS easy
 for users to use (and getting that check-box taken care of in a wide
 variety of fields that we are being exposed to now, see the PGConf.NYC
 keynote speakers...) is receiving such push-back.

If this is being sold as merely ease of use, then it is probably going
to get rejected.  In order to get some extra ease of use for the minority
of users who need RLS, you are going to significantly complicate the lives
of all Postgres users.  That's not a net win in any sane calculation of
ease of use.

Maybe the right thing to think about is how we can make it easier to set
up table + view combinations according to the pattern Robert described.
I wouldn't have a problem with some more-or-less-automated support for
doing that.  (Consider SERIAL as a possible precedent here: it's basically
a table creation macro.)

 You're suggesting that we use views instead, which clearly could run
 someone else's code.  Perhaps the user will notice that they're
 selecting from a view instead of a table, but I've never seen a security
 design around making sure that what is being select'd from is a table
 vs. a view.

pg_dump is a sufficient counterexample to that statement.

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] wrapping in extended mode doesn't work well with default pager

2014-06-11 Thread Peter Eisentraut
On 6/8/14, 11:29 PM, Noah Misch wrote:
 The patch did not restore 9.3 behavior for that one.  Starting with commit
 6513633, the first line of letters is space-padded on the right to the width
 of the second line of letters.  To illustrate, I have attached raw psql output
 from both commit 6513633 and its predecessor.  Also note that
 psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509
 bytes to 511 bytes; 509 is the longstanding width.

I noticed that (or perhaps a related) problem today.  Here is a simple demo:

psql -X -q -t -x -c 'select * from (values (1),(2)) as _ (col)'

9.3:

col | 1
+--
col | 2

9.4:

 col | 1
+--
 col | 2


This breaks check_postgres.  (Why check_postgres doesn't use unaligned
output is beyond me.)



-- 
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] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Robert Haas
On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost sfr...@snowman.net wrote:
 In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically
 *is* row-level security: instead of applying a row-level security
 policy to a table, just create a security-barrier view over the table
 and grant access to the view.  Forget that the table ever existed.
 Done.

 This argument could have been made for column-level privileges also, no?

Not really.  First of all, we didn't have security_barrier views at
that time, let alone security barrier views that are auto-updateable.
That's a really important piece of technology which makes filtering
access via views feasible in ways that really were not feasible in the
past.  Secondly, column-level permissions - like every other
currently-existing type of permissions - are declarative.  They are an
additional opportunity for the system to say no to something it
otherwise would have allowed, but no user-defined code is executed.
Row-level security is not a chance for the system to deny access; it's
a chance for user-defined code to take control and perform arbitrary
operations.  So the scope of what we're contemplating for row-level
security is really far, far more invasive than what we did for
column-level privileges.

 I agree that views, or even security-definer functions, offer a great
 deal of flexibility, and that may be necessary in some use-cases, but I
 fail to see why that means we should avoid providing the mechanics to
 achieve simple and usable RLS akin to what other major RDBMS's have.

Because we don't have a good design.

I'm not categorically opposed to adding more RLS features to
PostgreSQL and never have been; in fact, I was deeply involved in the
original design of security barrier views and committed the original
patch to add that functionality to PostgreSQL, without which none of
what we're talking about here would be possible.  But the
currently-proposed design is very unappealing to me, for the reasons
that I've explained.  The right answer to this feature doesn't
provide anything that we don't already have and will introduce major
new security exposures that haven't been adequate thought is
debatable, but well other people have this so we should too is
definitely not it.  Craig's patch really hasn't grappled with any of
these thorny definition and security issues; it's just about making
the basic functionality work.  That's fine for a POC, but it's not
enough for a feature that the project would be committing to maintain
for the indefinite future.

 That's mighty useful for debugging, at least IMHO.  And, if you want
 to have several row-level security policies for different classes of
 users, just create more than one view and grant different privileges
 on each.

 I'm really not impressed with the idea that RLS should be done with
 multiple different views of the same underlying table.

Are you equally unimpressed with the idea that RLS as proposed can't
support more than one security policy right now *at all*?  Because it
seems to me that either you think multiple RLS policies on a single
table is important (in which case the current patch is inadequate) or
you think it's not important (in which case we need not argue about
whether doing it with multiple views over the same underlying table is
awkward).

 By contrast, it seems to me that every design so far proposed for
 something that is actually called row-level security - as opposed to
 commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is*
 row-level security, is extremely limited.  Look back at all the things
 listed in the previous paragraph; can you do those things easily with
 the designs that have been proposed?  As far as I can see, not really.

 I don't feel that RLS will, or even *should*, have the same level of
 flexibility that you can achieve with views and/or security definer
 functions.  I expect that, over time, we will add more capabilities to
 it, but it's never going to be able to redefine the contents of a column
 as a view can, nor will it be able to add columns to a table as views
 can.  I don't see those as reasons against having support for RLS.

What this patch is doing is basically allowing a table to really be a
view over itself.  If we choose to support that, I think it is
absolutely inevitable that people are going to want all the same
options that they would have if they really made a separate view -
separate permissions, WITH CHECK OPTION, all of it.  I find the
contrary argument - that people will only want X amount and no more -
simply not plausible.  If it's valuable to have some of those
capabilities in an RLS framework, somebody's going to want all of
them.  There's no bright line to divide the things that are valuable
in that context from those that aren't.

 I'm glad to hear your thoughts on the level of granularity which might
 be nice to have with RLS.  What would be great is to spend a bit more
 time reviewing what other systems provide in 

[HACKERS] view reloptions

2014-06-11 Thread Alvaro Herrera
I just noticed by chance that view relations are using StdRdOptions to
allocate their reloptions.  I can't find any reason for this, other than
someone failed to realize that they should instead have a struct defined
of its own, just like (say) GIN indexes do.  Views using StdRdOptions is
quite pointless, given that it's used for stuff like fillfactor and
autovacuum, neither of which apply to views.

9.2 added security_barrier to StdRdOptions, and 9.4 is now adding
check_option_offset, which is a string reloption with some funny rules.

Is it too late to redefine the check_option_offset stuff before 9.4
ships, so that it is in its own struct?  (I'd hope we can redefine it in
a simpler way also, hopefully one that doesn't require strcmp()'ing that
string with local or cascaded every time someone is interested in
knowing the option's value for a particular view.) There are some
problems with this idea though, namely:

1) it's damn late in the release cycle already
2) it would mean that security_barrier would change for external code
that expects StdRdOptions rather than, say, ViewOptions
3) I don't have time to do the legwork before CF1 anyway

If we don't change it now, I'm afraid we'll be stuck with using
StdRdOptions for views for all eternity.

(It's a pity I didn't become aware of this earlier in 9.4 cycle when I
added the multixact freeze reloptions ... I could have promoted a patch
back then.)

Thoughts?

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


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


Re: [HACKERS] wrapping in extended mode doesn't work well with default pager

2014-06-11 Thread Greg Stark
On Wed, Jun 11, 2014 at 7:52 PM, Peter Eisentraut pete...@gmx.net wrote:
 On 6/8/14, 11:29 PM, Noah Misch wrote:
 The patch did not restore 9.3 behavior for that one.  Starting with commit
 6513633, the first line of letters is space-padded on the right to the width
 of the second line of letters.  To illustrate, I have attached raw psql 
 output
 from both commit 6513633 and its predecessor.  Also note that
 psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509
 bytes to 511 bytes; 509 is the longstanding width.

 I noticed that (or perhaps a related) problem today.  Here is a simple demo:

I don't think these two issues are related.

The leading space that you (ie Peter) are complaining about in:

 col | 1
+--
 col | 2

Is there because if the cell wrapped it would get an ellipsis (ie
'...' but it's a single unicode character) in that column to indicate
that it's wrapped. However we don't wrap headers so the only reason to
change it is for the old-ascii linestyle:

stark=***# select * from (values (1),(2)) as _ (col
col);
stark***#
 col | 1
+col ;
-+-
 col | 2
+col ;

Noah's complaint is about the space padding on the *right*. Ie

stark=***# select * from (values ('foo'),('foo bar baz')) as _ (col);
 col | foo
 - This is the end of the line
-+---
This is the end of the line
 col | foo bar baz
 - This is the end of the line

We didn't used to do that in expanded and I kind of agree it would be
nice to avoid. But then there are lots of cases where it would still
be necessary:

stark=***# select * from (values ('foo'),('foo bar
 baz')) as _ (col);
stark'***#
 col | foo
 - This is the end of the line
-+---
This is the end of the line
 col | foo bar
+- This is the end of the line
 |  baz
- This is the end of the line

Obviously we would need to space padd to insert the + there.

I think this whole exercise has mostly just convinced me we should
implement an HTTP interface and reimplement psql as a browser app.

-- 
greg


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


Re: [HACKERS] wrapping in extended mode doesn't work well with default pager

2014-06-11 Thread Greg Stark
And Gmail has thoroughly mangled that email. Let me see if I can
resend it from Emacs more clearly.


-- 
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] view reloptions

2014-06-11 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Is it too late to redefine the check_option_offset stuff before 9.4
 ships, so that it is in its own struct?

We have a forced initdb already for beta2, so I'd say not.

 3) I don't have time to do the legwork before CF1 anyway

... but if nobody does the work, it's moot.

 If we don't change it now, I'm afraid we'll be stuck with using
 StdRdOptions for views for all eternity.

Why would we not be able to change it in 9.5?  It's a catalog change,
sure, but we'll no doubt have others.

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] view reloptions

2014-06-11 Thread Fabrízio de Royes Mello
On Wed, Jun 11, 2014 at 4:46 PM, Alvaro Herrera alvhe...@2ndquadrant.com
wrote:

 I just noticed by chance that view relations are using StdRdOptions to
 allocate their reloptions.  I can't find any reason for this, other than
 someone failed to realize that they should instead have a struct defined
 of its own, just like (say) GIN indexes do.  Views using StdRdOptions is
 quite pointless, given that it's used for stuff like fillfactor and
 autovacuum, neither of which apply to views.

 9.2 added security_barrier to StdRdOptions, and 9.4 is now adding
 check_option_offset, which is a string reloption with some funny rules.

 Is it too late to redefine the check_option_offset stuff before 9.4
 ships, so that it is in its own struct?  (I'd hope we can redefine it in
 a simpler way also, hopefully one that doesn't require strcmp()'ing that
 string with local or cascaded every time someone is interested in
 knowing the option's value for a particular view.)


Are there a big problem with this implementation?

I asked because we already do a strcmmp()'ing in 'buffering' option for
GiST indexes since this commit
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5edb24a8.

Regards,

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


Re: [HACKERS] [GENERAL] Question about partial functional indexes and the query planner

2014-06-11 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Tue, Jun 10, 2014 at 7:19 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Given the lack of previous complaints, I'm not sure this amounts to
 a back-patchable bug, but it does seem like something worth fixing
 going forward.

 Agreed, although I'd be willing to see us slip it into 9.4.  It's
 doubtful that anyone will get upset if their query plans change
 between beta1 and beta2, but the same cannot be said for released
 branches.

After further thought about this I realized that there's another category
of proof possibilities that is pretty simple to add while we're at it.
Namely, once we've found that both subexpressions of the two operator
clauses are equal(), we can use btree opclass relationships to prove that,
say, x  y implies x = y or x  y refutes x  y, independently of
just what x and y are.  (Well, they have to be immutable expressions, but
we'd not get this far if they're not.)  We already had pretty nearly all
of the machinery for that, but again it was only used for proving cases
involving comparisons to constants.

A little bit of refactoring later, I offer the attached draft patch.
I'm thinking this is probably more than we want to slip into 9.4
at this point, but I'll commit it to HEAD soon if there are not
objections.

regards, tom lane

diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c
index 9d61a4d..039221f 100644
*** a/src/backend/optimizer/util/predtest.c
--- b/src/backend/optimizer/util/predtest.c
*** static bool predicate_refuted_by_simple_
*** 95,102 
  static Node *extract_not_arg(Node *clause);
  static Node *extract_strong_not_arg(Node *clause);
  static bool list_member_strip(List *list, Expr *datum);
! static bool btree_predicate_proof(Expr *predicate, Node *clause,
! 	  bool refute_it);
  static Oid	get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it);
  static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashvalue);
  
--- 95,106 
  static Node *extract_not_arg(Node *clause);
  static Node *extract_strong_not_arg(Node *clause);
  static bool list_member_strip(List *list, Expr *datum);
! static bool operator_predicate_proof(Expr *predicate, Node *clause,
! 		 bool refute_it);
! static bool operator_same_subexprs_proof(Oid pred_op, Oid clause_op,
! 			 bool refute_it);
! static bool operator_same_subexprs_lookup(Oid pred_op, Oid clause_op,
! 			  bool refute_it);
  static Oid	get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it);
  static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashvalue);
  
*** arrayexpr_cleanup_fn(PredIterInfo info)
*** 1025,1032 
   * we are within an AND/OR subtree of a WHERE clause.  (Again, foo is
   * already known immutable, so the clause will certainly always fail.)
   *
!  * Finally, we may be able to deduce something using knowledge about btree
!  * operator families; this is encapsulated in btree_predicate_proof().
   *--
   */
  static bool
--- 1029,1037 
   * we are within an AND/OR subtree of a WHERE clause.  (Again, foo is
   * already known immutable, so the clause will certainly always fail.)
   *
!  * Finally, if both clauses are binary operator expressions, we may be able
!  * to prove something using the system's knowledge about operators; those
!  * proof rules are encapsulated in operator_predicate_proof().
   *--
   */
  static bool
*** predicate_implied_by_simple_clause(Expr 
*** 1060,1067 
  		return false;			/* we can't succeed below... */
  	}
  
! 	/* Else try btree operator knowledge */
! 	return btree_predicate_proof(predicate, clause, false);
  }
  
  /*--
--- 1065,1072 
  		return false;			/* we can't succeed below... */
  	}
  
! 	/* Else try operator-related knowledge */
! 	return operator_predicate_proof(predicate, clause, false);
  }
  
  /*--
*** predicate_implied_by_simple_clause(Expr 
*** 1083,1090 
   * these cases is to support using IS NULL/IS NOT NULL as partition-defining
   * constraints.)
   *
!  * Finally, we may be able to deduce something using knowledge about btree
!  * operator families; this is encapsulated in btree_predicate_proof().
   *--
   */
  static bool
--- 1088,1096 
   * these cases is to support using IS NULL/IS NOT NULL as partition-defining
   * constraints.)
   *
!  * Finally, if both clauses are binary operator expressions, we may be able
!  * to prove something using the system's knowledge about operators; those
!  * proof rules are encapsulated in operator_predicate_proof().
   *--
   */
  static bool
*** predicate_refuted_by_simple_clause(Expr 
*** 1148,1155 
  		return false;			/* we can't succeed below... */
  	}
  
! 	/* Else try btree operator knowledge */
! 	return btree_predicate_proof(predicate, clause, true);
  }
  
  
--- 1154,1161 
  		return false;	

Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Stephen Frost
Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Robert Haas (robertmh...@gmail.com) wrote:
  I'm really concerned about the security implications of this patch.  I
  think we're setting ourselves up for a whole lot of hurt for somewhat
  unclear gain.
 
  I'm certainly of a different opinion and, for the most part, I feel that
  if there are security concerns then they need to be addressed- and
  better by us than by asking users to use some other mechanism to
  implement RLS.
 
 TBH, I found Robert's argument pretty persuasive.  The idea that
 SELECT * FROM table might invoke arbitrary processing ought to scare
 anyone who's concerned about security, because that's going to completely
 break any assumptions about pg_dump being safe for instance, as well as
 force top-to-bottom rethinking of many other security assumptions.

SELECT triggers for a wide variety of use-cases are pretty commonly
asked for here and are something I'd like to see us support also.  There
are also quite a few ways in which a select can end up executing code.
Today it requires more than 'select * from table;', but not very
much..  I agree that it'd be good if we had a way to address that but I
continue to view that as an independent issue.

What I haven't heard any comments on, yet found interesting, was the
idea of having the RLS quals run as the owner of the table.  Would that
address these concerns?  I seem to recall wondering why we didn't do
that for views in the first place, though I doubt we could change it
now even if we wanted to (and I'm guessing the spec has something to say
about this, though I haven't gone and looked and don't remember
offhand).  It's certainly rather curious that functions called under a
view are run as the calling user while permissions checks on relations
referred to by the view are as the view owner.

Hopefully that will make the rest of this discussion less relevant, but
I'll respond with my feelings anyway.

  ...  That commit was
  the ground-work to allow us to finally get proper RLS and I'm very
  disappointed to hear that the mechanical pieces around making RLS easy
  for users to use (and getting that check-box taken care of in a wide
  variety of fields that we are being exposed to now, see the PGConf.NYC
  keynote speakers...) is receiving such push-back.
 
 If this is being sold as merely ease of use, then it is probably going
 to get rejected.  In order to get some extra ease of use for the minority
 of users who need RLS, you are going to significantly complicate the lives
 of all Postgres users.  That's not a net win in any sane calculation of
 ease of use.

I don't view this as being at all accurate- how is this complicating the
lives of all Postgres users?  If they are worried about running user
defined code then they *already* have a lot to worry about.

While the users of RLS might be less than 50% and therefore the
minority, I expect it will have quite a bit of up-take in certain
industries and I know that our lack of any RLS is currently preventing
use of Postgres in some rather important cases.

As for it being ease-of-use, again, there are ways in which column level
privileges could have been dealt with using views, rules, security
definer functions, etc, but that doesn't mean we don't want that
feature.  I certainly view RLS (and have for quite some time..) as a much
needed capability, even if it can be done today using a bunch of user
written code that must be security audited.

 Maybe the right thing to think about is how we can make it easier to set
 up table + view combinations according to the pattern Robert described.

While this sounds interesting, I don't see adding columns or redefining
them as being in the perview of RLS.  The current approach of
allowing a boolean expression to be defined is both extremely flexible
while also being simple when the requirement is simple.  Having to
create, manage, update, etc, an independent object would add unnecessary
complexity.

Perhaps having it be a boolean expression is too much flexibility but
the alternatives that I can think of aren't terribly attractive to me
and the boolean expression approach is what folks coming from other
RDBMS's will be familiar with and understand how to build their
applications around.  We may need to provide some additional pieces
around this (perhaps a trigger-like function type which also gets
information about the object being queried, etc) but the point is to
have a straight-forward and simply reasoned about way of limiting what
data is returned.

 I wouldn't have a problem with some more-or-less-automated support for
 doing that.  (Consider SERIAL as a possible precedent here: it's basically
 a table creation macro.)

Perhaps there's a way to make that work, but personally it looks like a
whole bunch more work and I don't see the gain.  How would adding RLS to
an existing table work?  It's worse than the SERIAL case as at least
a default clause can be 

Re: [HACKERS] [patch] build issues on Win32

2014-06-11 Thread Noah Misch
On Wed, Mar 10, 2010 at 10:51:23AM -0500, Tom Lane wrote:
 =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= d...@des.no writes:
  -LDFLAGS=-Wl,--allow-multiple-definition
  +LDFLAGS=${LDFLAGS} -Wl,--allow-multiple-definition
 
 That bit seems sane.

I've committed that part.  Thanks.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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


Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost sfr...@snowman.net wrote:
  This argument could have been made for column-level privileges also, no?
 
 Not really.  First of all, we didn't have security_barrier views at
 that time, let alone security barrier views that are auto-updateable.

We had security definer functions, even set-returning ones, along with
rules and triggers.

 That's a really important piece of technology which makes filtering
 access via views feasible in ways that really were not feasible in the
 past.  Secondly, column-level permissions - like every other
 currently-existing type of permissions - are declarative.  They are an
 additional opportunity for the system to say no to something it
 otherwise would have allowed, but no user-defined code is executed.

We could try to avoid calling user-defined code for RLS, but it'd add a
whole lot of complexity and as far as I can see and your proposed
solution isn't avoiding the user-defined code anyway, so I'm not sure
why this solution should be required to meet that.

 Row-level security is not a chance for the system to deny access; it's
 a chance for user-defined code to take control and perform arbitrary
 operations.  So the scope of what we're contemplating for row-level
 security is really far, far more invasive than what we did for
 column-level privileges.

In this case the user-defined code needs to return a boolean.  We don't
currently do anything to prevent it from having side-effects, no, but
the same is true with views which incorporate functions.  I agree that
it makes a difference when compared to column-level privileges, but my
point was that we have provided easier ways to do things which were
possible using more complicated methods before.  Perhaps the risk with
RLS is higher but these issues look managable to me and the level of
doubt about our ability to provide this feature in a reasonable and
principled way that our users will understand surprises me.

  I agree that views, or even security-definer functions, offer a great
  deal of flexibility, and that may be necessary in some use-cases, but I
  fail to see why that means we should avoid providing the mechanics to
  achieve simple and usable RLS akin to what other major RDBMS's have.
 
 Because we don't have a good design.

We're using a design that's found in multiple other RDBMS's and used
extensively by certain industries which use those RDBMS's today.  I'm
certainly open to improving what is found in other systems for PG but I
have a hard time seeing this approach as a bad design.  Perhaps you're
referring to our implementation, in which case I might agree and things
like running the quals as the table owner is something which should be
considered (I don't know how the other RDBMS's operate in this regard
offhand- it'd be good to find out).

 I'm not categorically opposed to adding more RLS features to
 PostgreSQL and never have been; in fact, I was deeply involved in the
 original design of security barrier views and committed the original
 patch to add that functionality to PostgreSQL, without which none of
 what we're talking about here would be possible.  But the
 currently-proposed design is very unappealing to me, for the reasons
 that I've explained.  The right answer to this feature doesn't
 provide anything that we don't already have and will introduce major
 new security exposures that haven't been adequate thought is
 debatable, but well other people have this so we should too is
 definitely not it.

How about it's in high demand by our user base?  In particular, it's
being asked for by a *highly* technical section of our user base who
uses these capabilities today, with this design, in those other
databases.

 Craig's patch really hasn't grappled with any of
 these thorny definition and security issues; it's just about making
 the basic functionality work.  That's fine for a POC, but it's not
 enough for a feature that the project would be committing to maintain
 for the indefinite future.

Improving the patch is exactly what I'd like to do, but throwing out the
notion that RLS can't be allowed to execute user-defined code is cutting
the legs out of the feature completely- particularly with our system
where users can create all manner of objects in the system with their
own code being run.

  That's mighty useful for debugging, at least IMHO.  And, if you want
  to have several row-level security policies for different classes of
  users, just create more than one view and grant different privileges
  on each.
 
  I'm really not impressed with the idea that RLS should be done with
  multiple different views of the same underlying table.
 
 Are you equally unimpressed with the idea that RLS as proposed can't
 support more than one security policy right now *at all*?  Because it
 seems to me that either you think multiple RLS policies on a single
 table is important (in which case the current patch is 

Re: [HACKERS] wrapping in extended mode doesn't work well with default pager

2014-06-11 Thread Noah Misch
On Wed, Jun 11, 2014 at 08:59:34PM +0100, Greg Stark wrote:
 The leading space that you (ie Peter) are complaining about in:
 
  col | 1
 +--
  col | 2
 
 Is there because if the cell wrapped it would get an ellipsis (ie
 '...' but it's a single unicode character) in that column to indicate
 that it's wrapped. However we don't wrap headers so the only reason to
 change it is for the old-ascii linestyle:
 
 stark=***# select * from (values (1),(2)) as _ (col
 col);
 stark***#
  col | 1
 +col ;
 -+-
  col | 2
 +col ;
 
 Noah's complaint is about the space padding on the *right*. Ie
 
 stark=***# select * from (values ('foo'),('foo bar baz')) as _ (col);
  col | foo
  - This is the end of the line
 -+---
 This is the end of the line
  col | foo bar baz
  - This is the end of the line
 
 We didn't used to do that in expanded and I kind of agree it would be
 nice to avoid.

Based on the commit message and procedural history, I thought commit 6513633
was changing behavior solely for the combination of \pset expanded and
\pset format wrapped.  Peter's and my test cases show that it also changed
behavior for \pset expanded alone.  That's a bug, unless someone sees to
argue that the new \pset expanded behavior is a desirable improvement in
spite of its origin as an accident.  Altering an entrenched psql output format
is a big deal.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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


Re: [HACKERS] wrapping in extended mode doesn't work well with default pager

2014-06-11 Thread Tom Lane
Noah Misch n...@leadboat.com writes:
 Based on the commit message and procedural history, I thought commit 6513633
 was changing behavior solely for the combination of \pset expanded and
 \pset format wrapped.  Peter's and my test cases show that it also changed
 behavior for \pset expanded alone.  That's a bug, unless someone sees to
 argue that the new \pset expanded behavior is a desirable improvement in
 spite of its origin as an accident.  Altering an entrenched psql output format
 is a big deal.

TBH I'm wondering if we shouldn't just revert that patch (and the
subsequent fix attempts).  It was not a major feature and I'm thinking
we have better things to do right now than try to fix the multiple
logic holes it evidently has.  The author's certainly welcome to try
again with a more carefully thought-through patch for 9.5.

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] API change advice: Passing plan invalidation info from the rewriter into the planner?

2014-06-11 Thread Robert Haas
On Wed, Jun 11, 2014 at 8:59 PM, Stephen Frost sfr...@snowman.net wrote:
 Row-level security is not a chance for the system to deny access; it's
 a chance for user-defined code to take control and perform arbitrary
 operations.  So the scope of what we're contemplating for row-level
 security is really far, far more invasive than what we did for
 column-level privileges.

 In this case the user-defined code needs to return a boolean.  We don't
 currently do anything to prevent it from having side-effects, no, but
 the same is true with views which incorporate functions.  I agree that
 it makes a difference when compared to column-level privileges, but my
 point was that we have provided easier ways to do things which were
 possible using more complicated methods before.  Perhaps the risk with
 RLS is higher but these issues look managable to me and the level of
 doubt about our ability to provide this feature in a reasonable and
 principled way that our users will understand surprises me.

I'm glad the issues look manageable to you, but you haven't really
explained how to manage them.  The way to dispel doubt is to come up
with specific technical proposals that address the technical issues
that have been raised.  I accept that you are surprised that someone
might not think we are on the right course here, but it's entirely
appropriate for me to express my doubts about this or any other patch,
much as many people do in regards to many patches that are posted here
- generally for good and valid reasons.

For my part, I'm mildly surprised that anyone thinks it's a good idea
to have SELECT * FROM tab to mean different things depending on who is
typing it.  To me, that seems very confusing; how does an unprivileged
user with no ability to assume some other role validate that the row
security policy they've configured works at all and exposes precisely
the intended set of rows?  Even aside from security exposures, how
does a non-superuser who runs pg_dump know whether they've got a
complete backup or a filtered dump that's missing some rows?  A
filtered dump might not even be restorable if foreign keys are
involved.  I think those are serious issues that deserve serious
thought and consideration, not just a vague assurance that the issues
are probably manageable.

 Because we don't have a good design.

 We're using a design that's found in multiple other RDBMS's and used
 extensively by certain industries which use those RDBMS's today.  I'm
 certainly open to improving what is found in other systems for PG but I
 have a hard time seeing this approach as a bad design.  Perhaps you're
 referring to our implementation, in which case I might agree and things
 like running the quals as the table owner is something which should be
 considered (I don't know how the other RDBMS's operate in this regard
 offhand- it'd be good to find out).

I'm not referring to the proposed implementation particularly; or at
least not that aspect of it.  I don't think trying to run the view
quals as the defining user is likely to be very appealing, because I
think it's going to hurt performance, for example by preventing
function inlining and requiring lots of user-ID switches.  But I'm not
gonna complain if someone wants to mull it over and make a proposal
for how to make it work.  Rather, my concern is that all we've got is
what might be called the core of the feature; the actual guts of it.
There are a lot of ancillary details that seem to me to be not worked
out at all yet, or only half-baked.

 I'm not categorically opposed to adding more RLS features to
 PostgreSQL and never have been; in fact, I was deeply involved in the
 original design of security barrier views and committed the original
 patch to add that functionality to PostgreSQL, without which none of
 what we're talking about here would be possible.  But the
 currently-proposed design is very unappealing to me, for the reasons
 that I've explained.  The right answer to this feature doesn't
 provide anything that we don't already have and will introduce major
 new security exposures that haven't been adequate thought is
 debatable, but well other people have this so we should too is
 definitely not it.

 How about it's in high demand by our user base?  In particular, it's
 being asked for by a *highly* technical section of our user base who
 uses these capabilities today, with this design, in those other
 databases.

Sure, that's a valid reason for considering any feature.  But it's not
an excuse to overlook whatever design problems may exist.

 Are you equally unimpressed with the idea that RLS as proposed can't
 support more than one security policy right now *at all*?  Because it
 seems to me that either you think multiple RLS policies on a single
 table is important (in which case the current patch is inadequate) or
 you think it's not important (in which case we need not argue about
 whether doing it with multiple views over the same underlying table is
 awkward).

 

[HACKERS] Few observations in replication slots related code

2014-06-11 Thread Amit Kapila
Few observations in Replication slots related code:

1. In function StartupReplicationSlots(XLogRecPtr checkPointRedo),
parameter checkPointRedo is not used.


2. Few check are in different order in functions
pg_create_physical_replication_slot() and
pg_create_logical_replication_slot().

if (get_call_result_type(fcinfo, NULL, tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, return type must be a row type);
check_permissions();

CheckLogicalDecodingRequirements();

I don't think it makes any difference, however having checks in similar
order seems better unless there is a reason for not doing so.

3. Currently there is any Assert (Assert(!MyReplicationSlot);) in
pg_create_logical_replication_slot(), is there a need for similar
Assert in pg_create_physical_replication_slot()?

4.
StartupDecodingContext()
{
..
context = AllocSetContextCreate(CurrentMemoryContext,
Changeset Extraction Context,
}

To be consistent, shall we name this context as
logical | logical decoding?

5.
pg_create_logical_replication_slot()
{
..
CreateInitDecodingContext()

..
ReplicationSlotPersist()
}

Function pg_create_logical_replication_slot() is trying to
save slot twice once during CreateInitDecodingContext() and
then in ReplicationSlotPersist(), isn't it better if we can make
it do it just once?

6.
elog(ERROR, cannot handle changeset extraction yet);

Shouldn't it be better to use logical replication instead
of changeset extraction?

7.
+ * XXX: It might make sense to introduce ephemeral slots and always use
+ * the slot mechanism.

Already there are RS_EPHEMERAL slots, might be this
comment needs bit of rephrasing.

8. Is there a chance of inconsistency, if pg_restxlog resets the
xlog and after restart, one of the slots contains xlog position
older than what is resetted by pg_resetxlog?

9.
void
LogicalIncreaseXminForSlot(XLogRecPtr current_lsn, TransactionId xmin)
{
..
/*
 * don't overwrite if we already have a newer xmin. This can happen if we
 * restart decoding in a slot.
 */
if (TransactionIdPrecedesOrEquals(xmin, slot-data.catalog_xmin))
{
}
..
}

Should we just release spinlock in this loop and just return,
rather than keeping no action loop?

10.
* Iff ri_type = REPLICA_IDENTITY_INDEX, indexOid must be the Oid of a
suitable
 * index. Otherwise, it should be InvalidOid.
 */
static void
relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
   bool is_internal)

typo - *Iff*

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


Re: [HACKERS] postgresql.auto.conf read from wrong directory

2014-06-11 Thread Amit Kapila
On Tue, May 27, 2014 at 10:35 AM, Amit Kapila amit.kapil...@gmail.com
wrote:
 On Sun, May 11, 2014 at 11:23 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  I think it's clearly *necessary* to forbid setting data_directory in
  postgresql.auto.conf.  The file is defined to be found in the data
  directory, so any such setting is circular logic by definition;
  no good can come of not rejecting it.
 
  We already have a GUC flag bit about disallowing certain variables
  in the config file (though I don't remember if it's enforced or
  just advisory).  It seems to me that we'd better invent one for
  disallowing in ALTER SYSTEM, as well.

 I introduced a new flag bit (GUC_DISALLOW_IN_AUTO_FILE) to
 disallow parameters by Alter System and disallowed data_directory to
 be set by Alter System.

I have added this patch in next CF to avoid getting it lost.

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


[HACKERS] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Tom Lane
While investigating a different issue, I was astonished to find that
pg_restore in HEAD is incapable of restoring dumps containing large
objects: it fails with messages like

pg_restore: [archiver] could not create large object 10: ERROR:  function 
call message contains 1 arguments but function requires 2

After some investigation, I find that:

1. Commit c50b7c09d added a function lo_create(oid, bytea).

2. libpq's lo_initialize function, in fe-lobj.c, assumes that
the pg_catalog schema will contain *only one* function named
lo_create (and likewise for lo_open and a dozen other names).
With more than one lo_create function, it's luck of the draw
which one's OID ends up in the PGlobjfuncs struct.  If it's
the wrong one, subsequent attempts to use libpq's lo_create()
function fail as above.

While there's certainly a good argument to be made for making
lo_initialize do that query differently, there's no way that we
can fix every copy of libpq that's in the field.  I think we have to
consider that there can be only one lo_create is effectively part of
the protocol spec for the foreseeable future.  (It'd be easy enough
to add a check in the opr_sanity regression test to catch violations
of this rule.)

It's also extremely unfortunate that the regression tests don't
create (or at least don't leave behind) any large objects, as we
might then have possibly caught this bug much earlier.

Meanwhile, we have to either revert the addition of lo_create(oid,
bytea) altogether, or choose a different name for it.  Suggestions?

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] Proposing pg_hibernate

2014-06-11 Thread Gurjeet Singh
On Wed, Jun 11, 2014 at 12:25 AM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Wed, Jun 11, 2014 at 7:59 AM, Gurjeet Singh gurj...@singh.im wrote:
 On Sun, Jun 8, 2014 at 3:24 AM, Amit Kapila amit.kapil...@gmail.com
 wrote:
  Yeap, but if it crashes before writing checkpoint record, it will lead
  to
  recovery which is what we are considering.

 Good point.

 In case of such recovery, the recovery process will read in the blocks
 that were recently modified, and were possibly still in shared-buffers
 when Checkpointer crashed. So after recovery finishes, the
 BlockReaders will be invoked (because save-files were successfully
 written before the crash), and they would request the same blocks to
 be restored. Most likely, those blocks would already be in
 shared-buffers, hence no cause of concern regarding BlockReaders
 evicting buffers populated by recovery.

 Not necessarily because after crash, recovery has to start from
 previous checkpoint, so it might not perform operations on same
 pages as are saved by buffer saver.

Granted, the recovery may not start that way (that is, reading in
blocks that were in shared-buffers when shutdown was initiated), but
it sure would end that way. Towards the end of recovery, the blocks
it'd read back in are highly likely to be the ones that were present
in shared-buffers at the time of shutdown. By the end of recovery,
either (a) blocks read in at the beginning of recovery are evicted by
later operations of recovery, or (b) they are still present in
shared-buffers. So the blocks requested by the BlockReaders are highly
likely to be already in shared-buffers at the end of recovery, because
these are the same blocks that were dirty (and hence recorded in WAL)
just before shutdown time.

I guess what I am trying to say is that the blocks read in by the
BlockReaders will be a superset of those read in by the reocvery
process. At the time of shutdown/saving-buffers, the shared-buffers
may have contained dirty and clean buffers. WAL contains the info of
which blocks were dirtied. Recovery will read back the blocks that
were dirty, to replay the WAL, and since the BlockReaders are started
_after_ recovery finishes, the BlockReaders will effectively read in
only those blocks that are not already read-in by the recovery.

I am not yet convinced, at least in this case, that Postgres
Hibernator would restore blocks that can cause eviction of buffers
restored by recovery.

I don't have intimate knowledge of recovery but I think the above
assessment of recovery's operations holds true. If you still think
this is a concern, can you please provide a bit firm example using
which I can visualize the problem you're talking about.

 Also as the file saved by
 buffer saver can be a file which contains only partial list of
 buffers which were in shared buffer's, it becomes more likely that
 in such cases it can override the buffers populated by recovery.

I beg to differ. As described above, the blocks read-in by the
BlockReader will not evict the recovery-restored blocks. The
save-files being written partially does not change that.

 Now as pg_hibernator doesn't give any preference to usage_count while
 saving buffer's, it can also evict the buffers populated by recovery
 with some lower used pages of previous run.

The case we're discussing (checkpointer/BufferSaver/some-other-process
crash during a smart/fast shutdown) should occur rarely in practice.
Although Postgres Hibernator is not yet proven to do the wrong thing
in this case, I hope you'd agree that BlockReaders evicting buffers
populated by recovery process is not catastrophic at all, merely
inconvenient from performance perspective. Also, the impact is only on
the initial performance immediately after startup, since application
queries will re-prime the shared-buffers with whatever buffers they
need.

 I think Block Readers will remove file only after reading and populating
 buffers from it

Correct.

 and that's the reason I mentioned that it can lead to doing
 both recovery as well as load buffers based on file saved by buffer
 saver.

I am not sure I completely understand the implication here, but I
think the above description of case where
recovery-followed-by-BlockReaders not causing a concern may cover it.

Best regards,
-- 
Gurjeet Singh http://gurjeet.singh.im/

EDB www.EnterpriseDB.com


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


Re: [HACKERS] Proposing pg_hibernate

2014-06-11 Thread Gurjeet Singh
On Wed, Jun 11, 2014 at 10:56 AM, Robert Haas robertmh...@gmail.com wrote:
 On Tue, Jun 10, 2014 at 10:03 PM, Gurjeet Singh gurj...@singh.im wrote:
 And it's probably accepted by now that such a bahviour is not
 catastrophic, merely inconvenient.

 I think the whole argument for having pg_hibernator is that getting
 the block cache properly initialized is important.  If it's not
 important, then we don't need pg_hibernator in the first place.  But
 if it is important, then I think not loading unrelated blocks into
 shared_buffers is also important.

I was constructing a contrived scenario, something that would rarely
happen in reality. I feel that the benefits of this feature greatly
outweigh the minor performance loss caused in such an unlikely scenario.

Best regards,
-- 
Gurjeet Singh http://gurjeet.singh.im/

EDB www.EnterpriseDB.com


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


Re: [HACKERS] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Tatsuo Ishii
 Meanwhile, we have to either revert the addition of lo_create(oid,
 bytea) altogether, or choose a different name for it.  Suggestions?

I wonder if there's any use case where we need to store bytea into
large objects. Don't we already have bytea data type? If the use case
is for large data which does not fit in a tuple, I am afraid that the
query string could become extremely big one.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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


Re: [HACKERS] Something flaky in the relfilenode mapping infrastructure

2014-06-11 Thread Noah Misch
On Tue, Apr 15, 2014 at 03:28:41PM -0400, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On 2014-03-27 08:02:35 -0400, Tom Lane wrote:
  Buildfarm member prairiedog thinks there's something unreliable about
  commit f01d1ae3a104019d6d68aeff85c4816a275130b3:
 
  So I had made a notice to recheck on
  this. 
  http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=prairiedogbr=HEAD
  indicates there haven't been any further failures... So, for now I
  assume this was caused by some problem fixed elsewhere.
 
 Hard to say.  In any case, I agree we can't make any progress unless we
 see it again.

The improved test just tripped:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedogdt=2014-06-12%2000%3A17%3A07

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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


Re: [HACKERS] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Noah Misch
On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote:
 While there's certainly a good argument to be made for making
 lo_initialize do that query differently, there's no way that we
 can fix every copy of libpq that's in the field.  I think we have to
 consider that there can be only one lo_create is effectively part of
 the protocol spec for the foreseeable future.  (It'd be easy enough
 to add a check in the opr_sanity regression test to catch violations
 of this rule.)
 
 It's also extremely unfortunate that the regression tests don't
 create (or at least don't leave behind) any large objects, as we
 might then have possibly caught this bug much earlier.

Agreed.

 Meanwhile, we have to either revert the addition of lo_create(oid,
 bytea) altogether, or choose a different name for it.  Suggestions?

lo_new() or lo_make()?  An earlier draft of the patch that added
lo_create(oid, bytea) had a similar function named make_lo().

Thanks,
nm

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


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


Re: [HACKERS] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Pavel Stehule
2014-06-12 6:22 GMT+02:00 Tatsuo Ishii is...@postgresql.org:

  Meanwhile, we have to either revert the addition of lo_create(oid,
  bytea) altogether, or choose a different name for it.  Suggestions?

 I wonder if there's any use case where we need to store bytea into
 large objects. Don't we already have bytea data type? If the use case
 is for large data which does not fit in a tuple, I am afraid that the
 query string could become extremely big one.


I know a one use case - and I used it in my one application. For one my
customer I wrote a application that ensures a data change between two
independent subjects based on XML. It was relative simple architecture -
applications solved a communication, and stored procedures solved a content
- good success was a using of SQL/XML. After few years the communication
protocol was enhanced about attached tiff scans - serialized in base64 in
result XML doc. I had to quickly fix a this application with minimal
impacts to others applications. And LO API is perfect for transporting
binary data from/to database. But next I needed a functions for conversion
between bytea and LO.

Regards

Pavel



 Best regards,
 --
 Tatsuo Ishii
 SRA OSS, Inc. Japan
 English: http://www.sraoss.co.jp/index_en.php
 Japanese:http://www.sraoss.co.jp


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



Re: [HACKERS] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Tom Lane
Noah Misch n...@leadboat.com writes:
 On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote:
 Meanwhile, we have to either revert the addition of lo_create(oid,
 bytea) altogether, or choose a different name for it.  Suggestions?

 lo_new() or lo_make()?  An earlier draft of the patch that added
 lo_create(oid, bytea) had a similar function named make_lo().

I think we want to stick to the lo_xxx naming convention, whatever
xxx ends up being.

I was idly thinking that we might want to focus on the fact that this
function not only creates a LO but loads some data into it.  lo_make
isn't too bad, but we could also consider lo_load, lo_import, etc.
(lo_import is not one of the names we have to avoid overloading.
OTOH, there's already a 2-argument form of it, so maybe there'd be
issues with resolving calls with unknown-literal arguments.)

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] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Pavel Stehule
2014-06-12 6:54 GMT+02:00 Noah Misch n...@leadboat.com:

 On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote:
  While there's certainly a good argument to be made for making
  lo_initialize do that query differently, there's no way that we
  can fix every copy of libpq that's in the field.  I think we have to
  consider that there can be only one lo_create is effectively part of
  the protocol spec for the foreseeable future.  (It'd be easy enough
  to add a check in the opr_sanity regression test to catch violations
  of this rule.)
 
  It's also extremely unfortunate that the regression tests don't
  create (or at least don't leave behind) any large objects, as we
  might then have possibly caught this bug much earlier.

 Agreed.

  Meanwhile, we have to either revert the addition of lo_create(oid,
  bytea) altogether, or choose a different name for it.  Suggestions?

 lo_new() or lo_make()?  An earlier draft of the patch that added
 lo_create(oid, bytea) had a similar function named make_lo().


+1 for lo_new

Regards

Pavel




 Thanks,
 nm

 --
 Noah Misch
 EnterpriseDB http://www.enterprisedb.com


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



Re: [HACKERS] Something flaky in the relfilenode mapping infrastructure

2014-06-11 Thread Tom Lane
Noah Misch n...@leadboat.com writes:
 The improved test just tripped:
 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedogdt=2014-06-12%2000%3A17%3A07

Ugh.  If the MTBF is circa three months, how will we catch this before
we're dead?

A quick look around the machine's buildfarm directory says there's nothing
left behind that's not included in the buildfarm server report, so I can't
offer any immediate insight.  But I can certainly load it up running some
additional tests if anyone has an idea what to look for.

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] lo_create(oid, bytea) breaks every extant release of libpq

2014-06-11 Thread Pavel Stehule
2014-06-12 7:08 GMT+02:00 Tom Lane t...@sss.pgh.pa.us:

 Noah Misch n...@leadboat.com writes:
  On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote:
  Meanwhile, we have to either revert the addition of lo_create(oid,
  bytea) altogether, or choose a different name for it.  Suggestions?

  lo_new() or lo_make()?  An earlier draft of the patch that added
  lo_create(oid, bytea) had a similar function named make_lo().

 I think we want to stick to the lo_xxx naming convention, whatever
 xxx ends up being.

 I was idly thinking that we might want to focus on the fact that this
 function not only creates a LO but loads some data into it.  lo_make
 isn't too bad, but we could also consider lo_load, lo_import, etc.
 (lo_import is not one of the names we have to avoid overloading.
 OTOH, there's already a 2-argument form of it, so maybe there'd be
 issues with resolving calls with unknown-literal arguments.)


I have not any problem with lo_new, lo_make. lo_import is related to import
from host system. I am not sure about lo_load, but I am not able to specify
arguments why not.

Pavel


 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