Re: [HACKERS] Reduce pinning in btree indexes

[Kyotaro HORIGUCHI]

Hello,

> It looks like the remaining performance regression was indeed a
> result of code alignment.  I found two "paranoia" assignments I had
> accidentally failed to put back with the rest of the mark/restore
> optimization; after that trivial change the patched version is
> ever-so-slightly faster than master on all tests.

The performance which your test shows looks great. The gain might
comes from removing of buffer locking on _bt_next. I also would
like to see this to come along with 9.5 but it is the matter for
committers.

Apart from it, I looked into the patch itself more
closely. Please reconcile as you like among contradicting
comments below:)


In nbtutils.c, _bt_killitems:

- This is not in charge of this patch, but the comment for
  _bt_killitems looks to have a trivial typo.

  >  * _bt_killitems - set LP_DEAD state for items an indexscan caller has
  >  * told us were killed
  >  *
  >  * scan->so contains information about the current page and killed tuples
  >  * thereon (generally, this should only be called if so->numKilled > 0).

  I suppose "scan->so" should be "scan->opaque" and
  "so->numKilled" be "opaque->numKilled".


- The following comment looks somewhat wrong.

  > /* Since the else condition needs page, get it here, too. */

  "the else condition needs page" means "the page of the buffer
  is needed later"? But, I suppose the comment itself is not
  necessary.

- If (BTScanPosIsPinned(so->currPos)).

  As I mention below for nbtsearch.c, the page aquired in the
  if-then block may be vacuumed so the LSN check done in the
  if-else block is need also in the if-then block. It will be
  accomplished only by changing the position of the end of the
  if-else block.

- _bt_killitems is called without pin when rescanning from
  before, so I think the previous code has already considered the
  unpinned case ("if (ItemPointerEquals(&ituple..." does
  that). Vacuum rearranges line pointers after deleting LP_DEAD
  items so the previous check seems enough for the purpose. The
  previous code is more effeicient becuase the mathing items will
  be processed even after vacuum.


In nbtsearch.c

- _bt_first(): It now releases the share lock on the page before
  the items to be killed is processed. This allows other backends
  to insert items into the page simultaneously. It seems to be
  dangerous alone, but _bt_killitems already considers the
  case. Anyway I think It'll be better to add a comment
  mentioning unlocking is not dangerous.


... Sorry, time's up for now.

regards,


> Average of 3 `make check-world` runs:
> master: 336.288 seconds
> patch:  332.237 seconds
> 
> Average of 6 `make check` runs:
> master: 25.409 seconds
> patch:  25.270 seconds
> 
> The following were all run 12 times, the worst 2 dropped, the rest
> averaged:
> 
> Kyotaro-san's 1m mark "worst case" benchmark:
> master: 962.581 ms, 6.765 stdev
> patch:  947.518 ms, 3.584 stdev
> 
> Kyotaro-san's 500k mark, 500k restore "worst case" benchmark:
> master: 1366.639 ms, 5.314 stdev
> patch:  1363.844 ms, 5.896 stdev
> 
> pgbench -i -s 16 pgbench / pgbench -c 16 -j 4 -T 500 pgbench
> master: 265.011 tps, 4.952 stdev
> patch:  267.164 tps, 9.094 stdev
> 
> How do people feel about the idea of me pushing this for 9.5 (after
> I clean up all the affected comments and README files)?  I know
> this first appeared in the last CF, but the footprint is fairly
> small and the only user-visible behavior change is that a btree
> index scan of a WAL-logged table using an MVCC snapshot no longer
> blocks a vacuum indefinitely.  (If there are objections I will move
> this to the first CF for the next release.)
> 
>  src/backend/access/nbtree/nbtree.c|  50 +---
>  src/backend/access/nbtree/nbtsearch.c | 141 
> +-
>  src/backend/access/nbtree/nbtutils.c  |  58 ++
>  src/include/access/nbtree.h   |  36 -
>  4 files changed, 201 insertions(+), 84 deletions(-)

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Join push-down support for foreign tables

[Etsuro Fujita]

On 2015/03/03 21:34, Shigeru Hanada wrote:

I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
v6 patch.


Thanks for the work, Hanada-san and KaiGai-san!

Maybe I'm missing something, but did we agree to take this approach, ie, 
"join push-down" on top of custom join?  There is a comment ahout that 
[1].  I just thought it'd be better to achieve a consensus before 
implementing the feature further.



but still the patch
has an issue about joins underlying UPDATE or DELETE.  Now I'm working
on fixing this issue.


Is that something like "UPDATE foo ... FROM bar ..." where both foo and 
bar are remote?  If so, I think it'd be better to push such an update 
down to the remote, as discussed in [2], and I'd like to work on that 
together!


Sorry for having been late for the party.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/23343.1418658...@sss.pgh.pa.us
[2] http://www.postgresql.org/message-id/31942.1410534...@sss.pgh.pa.us


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

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2015/03/04 16:58, Albe Laurenz wrote:

Etsuro Fujita wrote:

While updating the patch, I noticed that in the previous patch, there is
a bug in pushing down parameterized UPDATE/DELETE queries; generic plans
for such queries fail with a can't-happen error.  I fixed the bug and
tried to add the regression tests that execute the generic plans, but I
couldn't because I can't figure out how to force generic plans.  Is
there any way to do that?


I don't know about a way to force it, but if you run the statement six
times, it will probably switch to a generic plan.


Yeah, I was just thinking running the statement six times in the 
regression tests ...


Thanks!

Best regards,
Etsuro Fujita


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

Re: [HACKERS] add modulo (%) operator to pgbench

[Fabien COELHO]

On Mon, Mar 2, 2015 at 5:41 PM, Fabien COELHO  wrote:

but I'd like to have a more robust discussion about what we want the error
reporting to look like rather than just sliding it into this patch.


As an input, I suggest that the error reporting feature should provide a
clue about where the issue is, including a line number and possibly the
offending line. Not sure what else is needed.


I agree.  But I think it might be better to try to put everything
related to a single error on one line, in a consistent format, e.g.:

bad.sql:3: syntax error in set command at column 25


Hmmm... sure that's better. I'll look into that.

--
Fabien.


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

Re: [HACKERS] Join push-down support for foreign tables

[Kouhei Kaigai]

> On 2015/03/03 21:34, Shigeru Hanada wrote:
> > I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
> > v6 patch.
> 
> Thanks for the work, Hanada-san and KaiGai-san!
> 
> Maybe I'm missing something, but did we agree to take this approach, ie,
> "join push-down" on top of custom join?  There is a comment ahout that
> [1].  I just thought it'd be better to achieve a consensus before
> implementing the feature further.
> 
It is not correct. The join push-down feature is not implemented
on top of the custom-join feature, however, both of them are 99%
similar on both of the concept and implementation.
So, we're working to enhance foreign/custom-join interface together,
according to Robert's suggestion [3], using postgres_fdw extension
as a minimum worthwhile example for both of foreign/custom-scan.

[3] http://bit.ly/1w1PoDU

> > but still the patch
> > has an issue about joins underlying UPDATE or DELETE.  Now I'm working
> > on fixing this issue.
> 
> Is that something like "UPDATE foo ... FROM bar ..." where both foo and
> bar are remote?  If so, I think it'd be better to push such an update
> down to the remote, as discussed in [2], and I'd like to work on that
> together!
>
Hanada-san, could you give us test query to reproduce the problem
above? I and Fujita-san can help to investigate the problem from
different standpoints for each.

> Sorry for having been late for the party.
> 
We are still in the party.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei 


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

Re: [HACKERS] Join push-down support for foreign tables

[Etsuro Fujita]

On 2015/03/04 17:31, Kouhei Kaigai wrote:

On 2015/03/03 21:34, Shigeru Hanada wrote:

I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
v6 patch.



Maybe I'm missing something, but did we agree to take this approach, ie,
"join push-down" on top of custom join?  There is a comment ahout that
[1].  I just thought it'd be better to achieve a consensus before
implementing the feature further.



It is not correct. The join push-down feature is not implemented
on top of the custom-join feature, however, both of them are 99%
similar on both of the concept and implementation.
So, we're working to enhance foreign/custom-join interface together,
according to Robert's suggestion [3], using postgres_fdw extension
as a minimum worthwhile example for both of foreign/custom-scan.


OK, thanks for the explanation!


but still the patch
has an issue about joins underlying UPDATE or DELETE.  Now I'm working
on fixing this issue.



Is that something like "UPDATE foo ... FROM bar ..." where both foo and
bar are remote?  If so, I think it'd be better to push such an update
down to the remote, as discussed in [2], and I'd like to work on that
together!



Hanada-san, could you give us test query to reproduce the problem
above? I and Fujita-san can help to investigate the problem from
different standpoints for each.


Yeah, will do.

Best regards,
Etsuro Fujita


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

Re: [HACKERS] Bootstrap DATA is a pita

[Andres Freund]

On 2015-03-03 21:49:21 -0500, Robert Haas wrote:
> On Sat, Feb 21, 2015 at 11:34 AM, Tom Lane  wrote:
> > Andres Freund  writes:
> >> On 2015-02-20 22:19:54 -0500, Peter Eisentraut wrote:
> >>> On 2/20/15 8:46 PM, Josh Berkus wrote:
>  Or what about just doing CSV?
> >
> >>> I don't think that would actually address the problems.  It would just
> >>> be the same format as now with different delimiters.
> >
> >> Yea, we need hierarchies and named keys.
> >
> > Yeah.  One thought though is that I don't think we need the "data" layer
> > in your proposal; that is, I'd flatten the representation to something
> > more like
> >
> >  {
> >  oid => 2249,
> >  oiddefine => 'CSTRINGOID',
> >  typname => 'cstring',
> >  typlen => -2,
> >  typbyval => 1,
> >  ...
> >  }
> 
> Even this promises to vastly increase the number of lines in the file,
> and make it harder to compare entries by grepping out some common
> substring.  I agree that the current format is a pain in the tail, but
> pg_proc.h is >5k lines already.  I don't want it to be 100k lines
> instead.

Do you have a better suggestion? Sure it'll be a long file, but it still
seems vastly superiour to what we have now.

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] Join push-down support for foreign tables

[Shigeru Hanada]

2015-03-04 17:00 GMT+09:00 Etsuro Fujita :
> On 2015/03/03 21:34, Shigeru Hanada wrote:
>>
>> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
>> v6 patch.
>
>
> Thanks for the work, Hanada-san and KaiGai-san!
>
> Maybe I'm missing something, but did we agree to take this approach, ie,
> "join push-down" on top of custom join?  There is a comment ahout that [1].
> I just thought it'd be better to achieve a consensus before implementing the
> feature further.

As Kaigai-san says, foreign join push-down is beside custom scan, and
they are on the custom/foreign join api patch.

>
>> but still the patch
>> has an issue about joins underlying UPDATE or DELETE.  Now I'm working
>> on fixing this issue.
>
>
> Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar
> are remote?  If so, I think it'd be better to push such an update down to
> the remote, as discussed in [2], and I'd like to work on that together!

A part of it, perhaps.  But at the moment I see many issues to solve
around pushing down complex UPDATE/DELETE.  So I once tightened the
restriction, that joins between foreign tables are pushed down only if
they are part of SELECT statement.  Please see next v4 patch I'll post
soon.

>
> Sorry for having been late for the party.
>
> Best regards,
> Etsuro Fujita
>
> [1] http://www.postgresql.org/message-id/23343.1418658...@sss.pgh.pa.us
> [2] http://www.postgresql.org/message-id/31942.1410534...@sss.pgh.pa.us



-- 
Shigeru HANADA


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

Re: [HACKERS] Performance improvement for joins where outer side is unique

[David Rowley]

On 27 February 2015 at 06:48, Tomas Vondra 
wrote:

> On 26.2.2015 18:34, Tom Lane wrote:
> > Tomas Vondra  writes:
> >> FWIW this apparently happens because the code only expect that
> >> EquivalenceMembers only contain Var, but in this particular case that's
> >> not the case - it contains RelabelType (because oprcode is regproc, and
> >> needs to be relabeled to oid).
> >
> > If it thinks an EquivalenceMember must be a Var, it's outright
> > broken; I'm pretty sure any nonvolatile expression is possible.
>
> I came to the same conclusion, because even with the RelabelType fix
> it's trivial to crash it with a query like this:
>
> SELECT 1 FROM pg_proc p JOIN pg_operator o
>   ON oprcode = (p.oid::int4 + 1);
>
>
Thanks for looking at this Tomas. Sorry it's taken this long for me to
respond, but I wanted to do so with a working patch.

I've made a few changes in the attached version:

1. Fixed Assert failure when eclass contained non-Var types, as reported by
you.
2. Added support for expression indexes.

The expression indexes should really be supported as with the previous
patch they worked ok with LEFT JOINs, but not INNER JOINs, that
inconsistency is pretty much a bug in my book, so I've fixed it.

The one weird quirk with the patch is that, if we had some tables like:

create table r1 (id int primary key, value int not null);
create table r2 (id int primary key);

And a query:
explain verbose select * from r1 inner join r2 on r1.id=r2.id where r2.id
=r1.value;

The join is not properly detected as a unique join. This is down to the
eclass containing 3 members, when the code finds the 2nd ec member for r1
it returns false as it already found another one. I'm not quite sure what
the fix is for this just yet as it's not quite clear to me how the code
would work if there were 2 vars from each relation in the same eclass... If
these Vars were of different types then which operators would we use for
them? I'm not sure if having eclassjoin_is_unique_join() append every
possible combination to index_exprs is the answer. I'm also not quite sure
if the complexity is worth the extra code either.

Updated patch attached.

Thank you for looking it and reporting that bug.

Regards

David Rowley


unijoin_2015-03-04_ac455bd.patch
Description: Binary data

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

Re: [HACKERS] Performance improvement for joins where outer side is unique

[David Rowley]

On 26 February 2015 at 13:15, Tomas Vondra 
wrote:

>
> BTW, I find this coding (first cast, then check) rather strange:
>
> Var *var = (Var *) ecm->em_expr;
>
> if (!IsA(var, Var))
> continue; /* Ignore Consts */
>
> It's probably harmless, but I find it confusing and I can't remember
> seeing it elsewhere in the code (for example clausesel.c and such) use
> this style:
>
> ... clause is (Node*) ...
>
> if (IsA(clause, Var))
> {
> Var *var = (Var*)clause;
> ...
> }
>
> or
>
> Var * var = NULL;
>
> if (! IsA(clause, Var))
> // error / continue
>
> var = (Var*)clause;
>
>
Yeah, it does look a bit weird, but if you search the code for "IsA(var,
Var)" you'll see it's nothing new.

Regards

David Rowley

Re: [HACKERS] Combining Aggregates

[David Rowley]

On 25 February 2015 at 08:15, Peter Eisentraut  wrote:

> On 2/20/15 3:32 PM, Tomas Vondra wrote:
>
> > Also, there are aggregate functions like array_agg() or string_agg()
> > that make this impossible, just like for many custom aggregates (like
> > hyperloglog for example). Again, I might not understand the idea
> > correctly ...
>
> How would a combining function work for something like array_agg()?  I
> don't think it would, at least if you want to preserve the ordering
> option for the user.
>
>
They just wouldn't work in that case. We'd simply not have a combine
function for that aggregate.

The yet to be written code, (say parallel hash aggregate), the planner
would have to ensure that each aggregate function being used had a combine
function, if any aggregate in the current query level didn't have one then
it would not parallelise the query.

Regards

David Rowley

Re: [HACKERS] Join push-down support for foreign tables

[Etsuro Fujita]

On 2015/03/04 17:57, Shigeru Hanada wrote:

2015-03-04 17:00 GMT+09:00 Etsuro Fujita :

On 2015/03/03 21:34, Shigeru Hanada wrote:

I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
v6 patch.



but still the patch
has an issue about joins underlying UPDATE or DELETE.  Now I'm working
on fixing this issue.



Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar
are remote?  If so, I think it'd be better to push such an update down to
the remote, as discussed in [2], and I'd like to work on that together!



A part of it, perhaps.  But at the moment I see many issues to solve
around pushing down complex UPDATE/DELETE.  So I once tightened the
restriction, that joins between foreign tables are pushed down only if
they are part of SELECT statement.  Please see next v4 patch I'll post
soon.


OK, thanks for the reply!

Best regards,
Etsuro Fujita


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

Re: [HACKERS] Join push-down support for foreign tables

[Shigeru Hanada]

Here is v4 patch of Join push-down support for foreign tables.  This
patch requires Custom/Foreign join patch v7 posted by Kaigai-san.

In this version I added check about query type which gives up pushing
down joins when the join is a part of an underlying query of
UPDATE/DELETE.



As of now postgres_fdw builds a proper remote query but it can't bring
ctid value up to postgresExecForeignUpdate()...

I'm still working on supporting such query, but I'm not sure that
supporting UPDATE/DELETE is required in the first version.  I attached
a patch foreign_join_update.patch to sure WIP for supporting
update/delete as top of foreign joins.

How to reproduce the error, please execute query below after running
attached init_fdw.sql for building test environment.  Note that the
script drops "user1", and creates database "fdw" and "pgbench".

fdw=# explain (verbose) update pgbench_branches b set filler = 'foo'
from pgbench_tellers t where t.bid = b.bid and t.tid < 10;

 QUERY
PLAN


---
 Update on public.pgbench_branches b  (cost=100.00..100.67 rows=67 width=390)
   Remote SQL: UPDATE public.pgbench_branches SET filler = $2 WHERE ctid = $1
   ->  Foreign Scan  (cost=100.00..100.67 rows=67 width=390)
 Output: b.bid, b.bbalance, 'foo

'::character(88), b.ctid, *
 Remote SQL: SELECT r.a_0, r.a_1, r.a_2, l FROM (SELECT tid,
bid, tbalance, filler FROM public.pgbench_tellers WHERE ((tid < 10)))
l (a_0, a_1) INNER JOIN (SELECT b
id, bbalance, NULL, ctid FROM public.pgbench_branches FOR UPDATE) r
(a_0, a_1, a_2, a_3)  ON ((r.a_0 = l.a_1))
(5 rows)
fdw=# explain (analyze, verbose) update pgbench_branches b set filler
= 'foo' from pgbench_tellers t where t.bid = b.bid and t.tid < 10;
ERROR:  ctid is NULL


2015-03-03 21:34 GMT+09:00 Shigeru Hanada :
> I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join
> v6 patch.  I posted some comments to v6 patch in this post:
>
> http://www.postgresql.org/message-id/CAEZqfEcNvjqq-P=jxnw1pb4t9wvpcporcn7g6cc46jgub7d...@mail.gmail.com
>
> Before applying my v3 patch, please apply Kaigai-san's v6 patch and my
> mod_cjv6.patch.
> Sorry for complex patch combination.  Those patches will be arranged
> soon by Kaigai-san and me.
>
> I fixed the issues pointed out by Thom and Kohei, but still the patch
> has an issue about joins underlying UPDATE or DELETE.  Now I'm working
> on fixing this issue.  Besides this issue, existing regression test
> passed.
>
> 2015-03-03 19:48 GMT+09:00 Kouhei Kaigai :
>>> > * Bug reported by Thom Brown
>>> > -
>>> > # EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER 
>>> > JOIN
>>> countries ON people.country_id = countries.id LIMIT 3) x;
>>> > ERROR:  could not open relation with OID 0
>>> >
>>> > Sorry, it was a problem caused by my portion. The patched setrefs.c
>>> > checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
>>> > node is associated with a certain base relation. If *_ps_tlist is
>>> > valid, it also expects scanrelid == 0.
>>> > However, things we should check is incorrect. We may have a case
>>> > with empty *_ps_tlist if remote join expects no columns.
>>> > So, I adjusted the condition to check scanrelid instead.
>>>
>>> Is this issue fixed by v5 custom/foreign join patch?
>>>
>> Yes, please rebase it.
>>
>> --
>> NEC OSS Promotion Center / PG-Strom Project
>> KaiGai Kohei 
>>
>
>
>
> --
> Shigeru HANADA



-- 
Shigeru HANADA


foreign_join_v4.patch
Description: Binary data


init_fdw.sql
Description: Binary data


foreign_join_update.patch
Description: Binary data

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

Re: [HACKERS] Combining Aggregates

[David Rowley]

On 18 February 2015 at 21:13, Kouhei Kaigai  wrote:
>
> This patch itself looks good as an infrastructure towards
> the big picture, however, we still don't reach the consensus
> how combined functions are used instead of usual translation
> functions.


Thank you for taking the time to look at the patch.

>
> Aggregate function usually consumes one or more values extracted
> from a tuple, then it accumulates its internal state according
> to the argument. Exiting transition function performs to update
> its internal state with assumption of a function call per records.
> On the other hand, new combined function allows to update its
> internal state with partial aggregated values which is processed
> by preprocessor node.
> An aggregate function is represented with Aggref node in plan tree,
> however, we have no certain way to determine which function shall
> be called to update internal state of aggregate.
>
>
This is true, there's nothing done in the planner to set any sort of state
in the aggregation nodes to tell them weather to call the final function or
not.  It's quite hard to know how far to go with this patch. It's really
only intended to provide the necessary infrastructure for things like
parallel query and various other possible usages of aggregate combine
functions. I don't think it's really appropriate for this patch to go
adding such a property to any nodes as there would still be nothing in the
planner to actually set those properties...  The only thing I can think of
to get around this is implement the most simple use for combine aggregate
functions, the problem with that is, that the most simple case is not at
all simple.



> For example, avg(float) has an internal state with float[3] type
> for number of rows, sum of X and X^2. If combined function can
> update its internal state with partially aggregated values, its
> argument should be float[3]. It is obviously incompatible to
> float8_accum(float) that is transition function of avg(float).
> I think, we need a new flag on Aggref node to inform executor
> which function shall be called to update internal state of
> aggregate. Executor cannot decide it without this hint.
>
> Also, do you have idea to push down aggregate function across
> joins? Even though it is a bit old research, I could find
> a systematic approach to push down aggregate across join.
> https://cs.uwaterloo.ca/research/tr/1993/46/file.pdf
>
>
I've not read the paper yet, but I do have a very incomplete WIP patch to
do this. I've just not had much time to work on it.


> I think, it is great if core functionality support this query
> rewriting feature based on cost estimation, without external
> modules.
>

Regards

David Rowley

Re: [HACKERS] Combining Aggregates

[David Rowley]

On 21 February 2015 at 07:16, Tomas Vondra 
wrote:

>
> 2) serialize/deserialize functions
> --
>
> This thread mentions "parallel queries" as a use case, but that means
> passing data between processes, and that requires being able to
> serialize and deserialize the aggregate state somehow. For actual data
> types that's not overly difficult I guess (we can use in/out functions),
> but what about aggretates using 'internal' state? I.e. aggregates
> passing pointers that we can't serialize?
>
>
This is a good question. I really don't know the answer to it as I've not
looked at the Robert's API for passing data between backends yet.

Maybe Robert or Amit can answer this?

Regards

David Rowley

Re: [HACKERS] How about to have relnamespace and relrole?

[Jeevan Chalke]

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

> 1.
> +#include "utils/acl.h"
>
> Can you please add it at appropriate place as #include list is an ordered
> list

  >  regrolein calls reg_role_oid in acl.c, which is declared in acl.h.

Well. I was suggesting that putting  #include "utils/acl.h" line after
#include "parser/parse_type.h" and before #include "utils/builtins.h"
so that they will be in order.
I understand that it is needed for reg_role_oid() call.

Review comments on *_v4 patches:

1.
+ The OID alias types don't sctrictly comply the transaction isolation

Typo. sctrictly => strictly


2.
+ joining the system tables correnspond to the OID types.
Typo. correnspond => correspond

Apart from these typos, I see no issues.
However, you can have a look over Jim's suggestion on doc wordings.

If you feel comfortable, I have no issues if you mark this as
"Ready for Committer" once you fix the typos and doc wordings.


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

Re: [HACKERS] Join push-down support for foreign tables

[Ashutosh Bapat]

Hi Hanada-san,
I am looking at the patch. Here are my comments

In create_foreignscan_path() we have lines like -
1587 pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
1588
required_outer);
Now, that the same function is being used for creating foreign scan paths
for joins, we should be calling get_joinrel_parampathinfo() on a join rel
and get_baserel_parampathinfo() on base rel.

The patch seems to handle all the restriction clauses in the same way.
There are two kinds of restriction clauses - a. join quals (specified using
ON clause; optimizer might move them to the other class if that doesn't
affect correctness) and b. quals on join relation (specified in the WHERE
clause, optimizer might move them to the other class if that doesn't affect
correctness). The quals in "a" are applied while the join is being computed
whereas those in "b" are applied after the join is computed. For example,
postgres=# select * from lt;
 val | val2
-+--
   1 |2
   1 |3
(2 rows)

postgres=# select * from lt2;
 val | val2
-+--
   1 |2
(1 row)

postgres=# select * from lt left join lt2 on (lt.val2 = lt2.val2);
 val | val2 | val | val2
-+--+-+--
   1 |2 |   1 |2
   1 |3 | |
(2 rows)

postgres=# select * from lt left join lt2 on (true) where (lt.val2 =
lt2.val2);
 val | val2 | val | val2
-+--+-+--
   1 |2 |   1 |2
(1 row)

The difference between these two kinds is evident in case of outer joins,
for inner join optimizer puts all of them in class "b". The remote query
sent to the foreign server has all those in ON clause. Consider foreign
tables ft1 and ft2 pointing to local tables on the same server.
postgres=# \d ft1
 Foreign table "public.ft1"
 Column |  Type   | Modifiers | FDW Options
+-+---+-
 val| integer |   |
 val2   | integer |   |
Server: loopback
FDW Options: (table_name 'lt')

postgres=# \d ft2
 Foreign table "public.ft2"
 Column |  Type   | Modifiers | FDW Options
+-+---+-
 val| integer |   |
 val2   | integer |   |
Server: loopback
FDW Options: (table_name 'lt2')

postgres=# explain verbose select * from ft1 left join ft2 on (ft1.val2 =
ft2.val2) where ft1.val + ft2.val > ft1.val2 or ft2.val is null;

QUERY PLAN


---

 Foreign Scan  (cost=100.00..125.60 rows=2560 width=16)
   Output: val, val2, val, val2
   Remote SQL: SELECT r.a_0, r.a_1, l.a_0, l.a_1 FROM (SELECT val, val2
FROM public.lt2) l (a_0, a_1) RIGHT JOIN (SELECT val, val2 FROM public.lt)
r (a
_0, a_1)  ON r.a_0 + l.a_0) > r.a_1) OR (l.a_0 IS NULL))) AND ((r.a_1 =
l.a_1))
(3 rows)

The result is then wrong
postgres=# select * from ft1 left join ft2 on (ft1.val2 = ft2.val2) where
ft1.val + ft2.val > ft1.val2 or ft2.val is null;
 val | val2 | val | val2
-+--+-+--
   1 |2 | |
   1 |3 | |
(2 rows)

which should match the result obtained by substituting local tables for
foreign ones
postgres=# select * from lt left join lt2 on (lt.val2 = lt2.val2) where
lt.val + lt2.val > lt.val2 or lt2.val is null;
 val | val2 | val | val2
-+--+-+--
   1 |3 | |
(1 row)

Once we start distinguishing the two kinds of quals, there is some
optimization possible. For pushing down a join it's essential that all the
quals in "a" are safe to be pushed down. But a join can be pushed down,
even if quals in "a" are not safe to be pushed down. But more clauses one
pushed down to foreign server, lesser are the rows fetched from the foreign
server. In postgresGetForeignJoinPath, instead of checking all the
restriction clauses to be safe to be pushed down, we need to check only
those which are join quals (class "a").

Following EXPLAIN output seems to be confusing
ft1 and ft2 both are pointing to same lt on a foreign server.
postgres=# explain verbose select ft1.val + ft1.val2 from ft1, ft2 where
ft1.val + ft1.val2 = ft2.val;

QUERY PLAN

---
--
 Foreign Scan  (cost=100.00..132.00 rows=2560 width=8)
   Output: (val + val2)
   Remote SQL: SELECT r.a_0, r.a_1 FROM (SELECT val, NULL FROM public.lt) l
(a_0, a_1) INNER JOIN (SELECT val, val2 FROM public.lt) r (a_0, a_1)  ON ((
(r.a_0 + r.a_1) = l.a_0))

Output just specified val + val2, it doesn't tell, where those val and val2
come from, neither it's evident from the rest of the context.

On Mon, Mar 2, 2015 at 6:18 PM, Shigeru Hanada 
wrote:

> Attached is the revised/rebased version of the $SUBJECT.
>
> This patch is based on Kaigai-san's custom/foreign join p

Re: [HACKERS] pg_upgrade and rsync

[Vladimir Borodin]

> 3 марта 2015 г., в 18:01, Bruce Momjian  написал(а):
> 
> On Tue, Mar  3, 2015 at 04:55:56PM +0300, Vladimir Borodin wrote:
>>OK, hmmm.  Thanks for testing.  It feels like you didn't have your new
>>master set up for streaming replication when you ran pg_upgrade.  Is
>>that correct?  Should I specify that specifically in the instructions?
>> 
>> 
>> After running pg_upgrade I do put in new PGDATA on master old pg_hba.conf and
>> postgresql.conf with wal_level = hot_standby. The full content of
>> postgresql.conf could be seen here - http://pastie.org/9995902. Then I do 
>> rsync
>> to replica, put recovery.conf and try to start both - first master, then
>> replica. If I turn off hot_standby in replica configuration, it starts. What 
>> am
>> I doing wrong?
> 
> After running initdb to create the new master, but before running
> pg_upgrade, modify the new master's postgresql.conf and change wal_level
> = hot_standby.  (Don't modify pg_hba.conf at this stage.)
> 

That does not help. The reason is that pg_upgrade sets 'Current wal_level 
setting: minimal' in control-file, and it does not depend on what is set in 
postgresql.conf before running pg_upgrade. Details could be seen here - 
http://pastie.org/9998671 .

The workaround for this is to start  and cleanly shut down postgres on master 
after running pg_upgrade but before running rsync. After that there would be a 
good control-file for streaming replication and rsync to replica can be done. 
But it could not be done with --size-only key, because control-file is of fixed 
size and rsync would skip it. Or may be everything should be copied with 
--size-only and control-file should be copied without this option.

> I didn't think that was necessary, but this might be some 9.3-specific
> problem, but let's get it working first.
> 
> -- 
>  Bruce Momjian  http://momjian.us
>  EnterpriseDB http://enterprisedb.com
> 
>  + Everyone has their own god. +


--
Да пребудет с вами сила…
https://simply.name/ru

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

[Syed, Rahila]

Hello,

>Are there any other flag bits that we should or are planning to add into WAL 
>header newly, except the above two? If yes and they are required by even a 
>block which doesn't have an image, I will change my mind and agree to add 
>something like chunk ID to a block header. 
>But I guess the answer of the question is No. Since the flag bits now we are 
>thinking to add are required only by a block having an image, adding them into 
>a block header (instead of block image header) seems a waste of bytes in WAL. 
>So I concur with Michael.
I agree.
As per my understanding, this change of xlog format was to provide for future 
enhancement which would need flags relevant to entire block.
But as mentioned, currently the flags being added are related to block image 
only. Hence for this patch it makes sense to add a field to 
XLogRecordImageHeader rather than block header. 
This will also save bytes per WAL record. 

Thank you,
Rahila Syed

 


__
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.

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

Re: [HACKERS] NULL-pointer check and incorrect comment for pstate in addRangeTableEntry

[Greg Stark]

This sounded familiar... I pointed out the same thing a while back and Tom
had some feedback on what to do about it:

http://www.postgresql.org/message-id/23294.1384142...@sss.pgh.pa.us
On 4 Mar 2015 00:37, "Michael Paquier"  wrote:

> On Wed, Mar 4, 2015 at 6:43 AM, Robert Haas wrote:
> > That seems to make sense to me.  Committed.
>
> Thanks.
> --
> Michael
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)

[Michael Paquier]

On Wed, Mar 4, 2015 at 4:43 PM, Michael Paquier wrote:
> On Tue, Mar 3, 2015 at 8:36 PM, Asif Naeem wrote:
>> Thank you Michael. I have looked the patch.
>
> Thanks for the review!
>
>> Overall logic looks good to me,
>> I have checked it with MSVC{2013,2008}. It works for MSVC 2013 but fail for
>> MSVC 2008, I think the condition "if ($proj =~
>> qr{ResourceCompile\s*Include="([^"]+)"})" is not going to work for MSVC2008
>> and MSVC2005 i.e.
>> [...]
>
> Thanks for the details, my patch is definitely missing vcproj entries.
> Note that I don't have yet an environment with MSVC 2008 or similar, I
> just got 2010 on my box for now. So you will have to wait until I have
> a fresh environment to get an updated patch...

OK, so I have been able to figure out a regex expression scanning for
win32ver.rc in RelativePath for a vcproj file. For vcxproj files, I am
still using ResourceCompile. Attached is the promised patch.

>> Although this patch is going to make MSVC
>> build consistent with Cygwin and MinGW build, following files seems
>> redundant now, is there any use for them other than backward compatibility ?
>> i.e.
>>> inst\lib\libpq.dll
>>> inst\lib\libpgtypes.dll
>>> inst\lib\libecpg_compat.dll
>>> inst\lib\libecpg.dll
>
> Indeed, I haven't noticed them. But we can simply remove them as they
> are installed in bin/ as well with this patch, it seems better for
> consistency with MinGW and Cygwin in any case.

Those entries are removed as well in the patch.

Regards,
-- 
Michael
From 257685e441ad940012600202e720de447ae5d6dc Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 23 Dec 2014 21:58:50 -0800
Subject: [PATCH] Install shared libraries in bin/ and lib/ on MSVC

This is the MSVC part of the previous commit, to ensure that install is
completely consistent with the multiple methods supported.
---
 src/tools/msvc/Install.pm | 52 +++
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/src/tools/msvc/Install.pm b/src/tools/msvc/Install.pm
index eba9aa0..237a0e6 100644
--- a/src/tools/msvc/Install.pm
+++ b/src/tools/msvc/Install.pm
@@ -91,7 +91,6 @@ sub Install
 	}
 
 	CopySolutionOutput($conf, $target);
-	lcopy($target . '/lib/libpq.dll', $target . '/bin/libpq.dll');
 	my $sample_files = [];
 	my @top_dir  = ("src");
 	@top_dir = ("src\\bin", "src\\interfaces") if ($insttype eq "client");
@@ -108,12 +107,8 @@ sub Install
 		$target . '/lib/',
 		"$conf\\",
 		"postgres\\postgres.lib",
-		"libpq\\libpq.lib",
-		"libecpg\\libecpg.lib",
 		"libpgcommon\\libpgcommon.lib",
-		"libpgport\\libpgport.lib",
-		"libpgtypes\\libpgtypes.lib",
-		"libecpg_compat\\libecpg_compat.lib");
+		"libpgport\\libpgport.lib");
 	CopyContribFiles($config, $target);
 	CopyIncludeFiles($target);
 
@@ -236,8 +231,9 @@ sub CopySolutionOutput
 	while ($sln =~ $rem)
 	{
 		my $pf = $1;
-		my $dir;
+		my @dirs;
 		my $ext;
+		my $is_sharedlib = 0;
 
 		$sln =~ s/$rem//;
 
@@ -247,17 +243,40 @@ sub CopySolutionOutput
 
 		my $proj = read_file("$pf.$vcproj")
 		  || croak "Could not open $pf.$vcproj\n";
+
+		# Check if this project uses a shared library by looking if
+		# SO_MAJOR_VERSION is defined in its Makefile, whose path
+		# can be found using the resource file of this project.
+		if (($vcproj eq 'vcxproj' &&
+			 $proj =~ qr{ResourceCompile\s*Include="([^"]+)"}) ||
+			($vcproj eq 'vcproj' &&
+			 $proj =~ qr{File\s*RelativePath="([^\"]+)\.rc"}))
+		{
+			my $projpath = dirname($1);
+			my $mf = read_file($projpath . '/Makefile')
+|| croak "Could not open $projpath/Makefile\n";
+
+			if ($mf =~ /^SO_MAJOR_VERSION\s*=\s*(.*)$/mg)
+			{
+$is_sharedlib = 1;
+			}
+		}
+
 		if ($vcproj eq 'vcproj' && $proj =~ qr{ConfigurationType="([^"]+)"})
 		{
 			if ($1 == 1)
 			{
-$dir = "bin";
+@dirs = qw(bin);
 $ext = "exe";
 			}
 			elsif ($1 == 2)
 			{
-$dir = "lib";
+@dirs = qw(lib);
 $ext = "dll";
+if ($is_sharedlib)
+{
+	push(@dirs, 'bin');
+}
 			}
 			else
 			{
@@ -271,13 +290,17 @@ sub CopySolutionOutput
 		{
 			if ($1 eq 'Application')
 			{
-$dir = "bin";
+@dirs = qw(bin);
 $ext = "exe";
 			}
 			elsif ($1 eq 'DynamicLibrary')
 			{
-$dir = "lib";
+@dirs = qw(lib);
 $ext = "dll";
+if ($is_sharedlib)
+{
+	push(@dirs, 'bin');
+}
 			}
 			else# 'StaticLibrary'
 			{
@@ -290,8 +313,11 @@ sub CopySolutionOutput
 		{
 			croak "Could not parse $pf.$vcproj\n";
 		}
-		lcopy("$conf\\$pf\\$pf.$ext", "$target\\$dir\\$pf.$ext")
-		  || croak "Could not copy $pf.$ext\n";
+		foreach my $dir (@dirs)
+		{
+			lcopy("$conf\\$pf\\$pf.$ext", "$target\\$dir\\$pf.$ext")
+|| croak "Could not copy $pf.$ext\n";
+		}
 		lcopy("$conf\\$pf\\$pf.pdb", "$target\\symbols\\$pf.pdb")
 		  || croak "Could not copy $pf.pdb\n";
 		print ".";
-- 
1.9.2.msysgit.0


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

Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission

[Greg Stark]

On Wed, Mar 4, 2015 at 1:35 AM, Haribabu Kommi  wrote:
> I feel there is no problem of current pg_hba reloads, because the
> check_for_interrupts
> doesn't reload the conf files. It will be done in the "postgresMain"
> function once the
> query finishes. Am I missing something?

Ah, no I guess that's right. I even noticed that earlier but forgot.

-- 
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] NULL-pointer check and incorrect comment for pstate in addRangeTableEntry

[Michael Paquier]

On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark  wrote:
> This sounded familiar... I pointed out the same thing a while back and Tom
> had some feedback on what to do about it:
>
> http://www.postgresql.org/message-id/23294.1384142...@sss.pgh.pa.us

Translated into code I guess that this gives the attached patch.
-- 
Michael
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 6f2a749..efa4be1 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -345,6 +345,7 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse)
 	List	   *new_rt;
 	RangeTblEntry *rt_entry1,
 			   *rt_entry2;
+	ParseState *pstate;
 
 	/*
 	 * Make a copy of the given parsetree.  It's not so much that we don't
@@ -356,6 +357,9 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse)
 	 */
 	viewParse = (Query *) copyObject(viewParse);
 
+	/* Create a dummy ParseState for addRangeTableEntryForRelation */
+	pstate = make_parsestate(NULL);
+
 	/* need to open the rel for addRangeTableEntryForRelation */
 	viewRel = relation_open(viewOid, AccessShareLock);
 
@@ -363,10 +367,10 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse)
 	 * Create the 2 new range table entries and form the new range table...
 	 * OLD first, then NEW
 	 */
-	rt_entry1 = addRangeTableEntryForRelation(NULL, viewRel,
+	rt_entry1 = addRangeTableEntryForRelation(pstate, viewRel,
 			  makeAlias("old", NIL),
 			  false, false);
-	rt_entry2 = addRangeTableEntryForRelation(NULL, viewRel,
+	rt_entry2 = addRangeTableEntryForRelation(pstate, viewRel,
 			  makeAlias("new", NIL),
 			  false, false);
 	/* Must override addRangeTableEntry's default access-check flags */
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 5a1d539..acfd0bc 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -1233,6 +1233,7 @@ convert_ANY_sublink_to_join(PlannerInfo *root, SubLink *sublink,
 	RangeTblRef *rtr;
 	List	   *subquery_vars;
 	Node	   *quals;
+	ParseState *pstate;
 
 	Assert(sublink->subLinkType == ANY_SUBLINK);
 
@@ -1264,6 +1265,9 @@ convert_ANY_sublink_to_join(PlannerInfo *root, SubLink *sublink,
 	if (contain_volatile_functions(sublink->testexpr))
 		return NULL;
 
+	/* Create a dummy ParseState for addRangeTableEntryForSubquery */
+	pstate = make_parsestate(NULL);
+
 	/*
 	 * Okay, pull up the sub-select into upper range table.
 	 *
@@ -1272,7 +1276,7 @@ convert_ANY_sublink_to_join(PlannerInfo *root, SubLink *sublink,
 	 * below). Therefore this is a lot easier than what pull_up_subqueries has
 	 * to go through.
 	 */
-	rte = addRangeTableEntryForSubquery(NULL,
+	rte = addRangeTableEntryForSubquery(pstate,
 		subselect,
 		makeAlias("ANY_subquery", NIL),
 		false,
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index f416fc2..24fe998 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1078,6 +1078,8 @@ addRangeTableEntryForRelation(ParseState *pstate,
 	RangeTblEntry *rte = makeNode(RangeTblEntry);
 	char	   *refname = alias ? alias->aliasname : RelationGetRelationName(rel);
 
+	Assert(pstate != NULL);
+
 	rte->rtekind = RTE_RELATION;
 	rte->alias = alias;
 	rte->relid = RelationGetRelid(rel);
@@ -1109,8 +,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
 	 * Add completed RTE to pstate's range table list, but not to join list
 	 * nor namespace --- caller must do that if appropriate.
 	 */
-	if (pstate != NULL)
-		pstate->p_rtable = lappend(pstate->p_rtable, rte);
+	pstate->p_rtable = lappend(pstate->p_rtable, rte);
 
 	return rte;
 }
@@ -1135,6 +1136,8 @@ addRangeTableEntryForSubquery(ParseState *pstate,
 	int			varattno;
 	ListCell   *tlistitem;
 
+	Assert(pstate != NULL);
+
 	rte->rtekind = RTE_SUBQUERY;
 	rte->relid = InvalidOid;
 	rte->subquery = subquery;
@@ -1187,8 +1190,7 @@ addRangeTableEntryForSubquery(ParseState *pstate,
 	 * Add completed RTE to pstate's range table list, but not to join list
 	 * nor namespace --- caller must do that if appropriate.
 	 */
-	if (pstate != NULL)
-		pstate->p_rtable = lappend(pstate->p_rtable, rte);
+	pstate->p_rtable = lappend(pstate->p_rtable, rte);
 
 	return rte;
 }
@@ -1224,6 +1226,8 @@ addRangeTableEntryForFunction(ParseState *pstate,
 	int			natts,
 totalatts;
 
+	Assert(pstate != NULL);
+
 	rte->rtekind = RTE_FUNCTION;
 	rte->relid = InvalidOid;
 	rte->subquery = NULL;
@@ -1441,8 +1445,7 @@ addRangeTableEntryForFunction(ParseState *pstate,
 	 * Add completed RTE to pstate's range table list, but not to join list
 	 * nor namespace --- caller must do that if appropriate.
 	 */
-	if (pstate != NULL)
-		pstate->p_rtable = lappend(pstate->p_rtable, rte);
+	pstate->p_rtable = lappend(pstate->p_rtable, rte);
 
 	return rte;
 }
@@ -1466,6 +1469,8 @@ addRangeTableEntryForValues(ParseState *pstate,
 	int			numaliases;
 	int			nu

Re: [HACKERS] POLA violation with \c service=

[David Fetter]

On Tue, Mar 03, 2015 at 09:52:55PM -0500, Robert Haas wrote:
> On Mon, Mar 2, 2015 at 5:05 PM, David Fetter  wrote:
> > So just to clarify, are you against back-patching the behavior
> > change, or the addition to src/common?
> 
> Mostly the latter.

So you're saying the former isn't a problem?  To recap, the behavior I
dug up was that sending a conninfo string or a URI to \c resulted in
\c's silently mangling same in a way that could only work accidentally.

Anyhow, patches have been posted for both approaches to all relevant
branches.

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

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


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

Re: [HACKERS] Bootstrap DATA is a pita

[Robert Haas]

>> Even this promises to vastly increase the number of lines in the file,
>> and make it harder to compare entries by grepping out some common
>> substring.  I agree that the current format is a pain in the tail, but
>> pg_proc.h is >5k lines already.  I don't want it to be 100k lines
>> instead.
>
> Do you have a better suggestion? Sure it'll be a long file, but it still
> seems vastly superiour to what we have now.

Not really.  What had occurred to me is to try to improve the format
of the DATA lines (e.g. by allowing names to be used instead of OIDs)
but that wouldn't allow defaulted fields to be omitted, which is
certainly a big win.  I wonder whether some home-grown single-line
format might be better than using a pre-existing format, but I'm not
too sure it would.

-- 
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] Bootstrap DATA is a pita

[Andres Freund]

On 2015-03-04 08:47:44 -0500, Robert Haas wrote:
> >> Even this promises to vastly increase the number of lines in the file,
> >> and make it harder to compare entries by grepping out some common
> >> substring.  I agree that the current format is a pain in the tail, but
> >> pg_proc.h is >5k lines already.  I don't want it to be 100k lines
> >> instead.
> >
> > Do you have a better suggestion? Sure it'll be a long file, but it still
> > seems vastly superiour to what we have now.
> 
> Not really.  What had occurred to me is to try to improve the format
> of the DATA lines (e.g. by allowing names to be used instead of OIDs)

That's a separate patch so far, so if we decide to only want thta we can
do it.

> but that wouldn't allow defaulted fields to be omitted, which is
> certainly a big win.  I wonder whether some home-grown single-line
> format might be better than using a pre-existing format, but I'm not
> too sure it would.

I can't see readability of anything being good unless the column names
are there - we just have too many columns in some of the tables. I think
having more lines is a acceptable price to pay. We can easily start to
split the files at some point if we want, that'd just be a couple lines
of code.

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] POLA violation with \c service=

[Robert Haas]

On Wed, Mar 4, 2015 at 8:33 AM, David Fetter  wrote:
> On Tue, Mar 03, 2015 at 09:52:55PM -0500, Robert Haas wrote:
>> On Mon, Mar 2, 2015 at 5:05 PM, David Fetter  wrote:
>> > So just to clarify, are you against back-patching the behavior
>> > change, or the addition to src/common?
>>
>> Mostly the latter.
>
> So you're saying the former isn't a problem?  To recap, the behavior I
> dug up was that sending a conninfo string or a URI to \c resulted in
> \c's silently mangling same in a way that could only work accidentally.

Well, the thing is, I'm not sure that's actually documented to work
anywhere.  psql says this:

 \c[onnect] [DBNAME|- USER|- HOST|- PORT|-]

The psql documentation looks like this:

\c or \connect [ dbname [ username ] [ host ] [ port ] ]

Neither says anything about being able to use a conninfo string or a
URI.  So I am not sure why we shouldn't regard this as a new feature
--- which, by the way, should be documented.  Arguably the right thing
to do is back-patch a change that prevents the dbname from being
interpreted as anything other than a literal database name, and then
in master make it work as you suggest.  Now, if those two patches are
substantially equal in size and risk, then you could argue that's just
silly, and that we should just make this work all the way back.  I'm
willing to accept that argument if it is in fact true.  But I'm not
very excited about doing what amounts to a refactoring exercise in the
back-branches.  Shuffling code around from one file to another seems
like something that we really ought to only be doing in master unless
there's a really compelling reason to do otherwise, and making
something work that is not documented to work does not compel me.

-- 
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] Bootstrap DATA is a pita

[Peter Eisentraut]

On 3/3/15 9:49 PM, Robert Haas wrote:
>> Yeah.  One thought though is that I don't think we need the "data" layer
>> in your proposal; that is, I'd flatten the representation to something
>> more like
>>
>>  {
>>  oid => 2249,
>>  oiddefine => 'CSTRINGOID',
>>  typname => 'cstring',
>>  typlen => -2,
>>  typbyval => 1,
>>  ...
>>  }
> 
> Even this promises to vastly increase the number of lines in the file,

I think lines are cheap.  Columns are much harder to deal with.

> and make it harder to compare entries by grepping out some common
> substring.

Could you give an example of the sort of thing you wish to do?



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

Re: [HACKERS] NULL-pointer check and incorrect comment for pstate in addRangeTableEntry

[Greg Stark]

On Wed, Mar 4, 2015 at 12:38 PM, Michael Paquier
 wrote:
> Translated into code I guess that this gives the attached patch.

Probably want to update a comment somewhere but yes, those are the
same three call sites I had found back then. I don't see any patch
lying around so I don't think I got any further than searching for
call sites.

-- 
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] Bootstrap DATA is a pita

[Tom Lane]

Peter Eisentraut  writes:
> On 3/3/15 9:49 PM, Robert Haas wrote:
>> Even this promises to vastly increase the number of lines in the file,

> I think lines are cheap.  Columns are much harder to deal with.

Yeah.  pg_proc.h is already impossible to work with in a standard
80-column window.  I don't want to find that the lines mostly wrap even
when I expand my editor window to full screen width, but that is certainly
what will happen if we adopt column labelling *and* insist that entries
remain all on one line.  (As a data point, the maximum usable Emacs window
width on my Mac laptop seems to be about 230 characters.)

It's possible that gaining the ability to depend on per-column defaults
would reduce the typical number of fields so much that pg_proc.h entries
would still fit on a line of 100-some characters ... but I'd want to see
proof before assuming that.  And pg_proc isn't even our widest catalog.
Some of the ones that are wider, like pg_am, don't seem like there would
be any scope whatsoever for saving space with per-column defaults.

So while I can see the attraction of trying to fit things on one line,
I doubt it's gonna work very well.  I'd rather go over to a
one-value-per-line format and live with lots of lines.

>> and make it harder to compare entries by grepping out some common
>> substring.

> Could you give an example of the sort of thing you wish to do?

On that angle, I'm dubious that a format that allows omission of fields is
going to be easy for editing scripts to modify, no matter what the layout
convention is.  I've found it relatively easy to write sed or even Emacs
macros to add new column values to old-school pg_proc.h ... but in this
brave new world, I'm going to be really hoping that the column default
works for 99.9% of pg_proc entries when we add a new pg_proc column,
because slipping a value into a desired position is gonna be hard for
a script when you don't know whether the adjacent existing fields are
present or not.

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] Bootstrap DATA is a pita

[Robert Haas]

On Wed, Mar 4, 2015 at 9:06 AM, Peter Eisentraut  wrote:
>> and make it harder to compare entries by grepping out some common
>> substring.
>
> Could you give an example of the sort of thing you wish to do?

e.g. grep for a function name and check that all the matches have the
same volatility.

-- 
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] Bootstrap DATA is a pita

[Robert Haas]

On Wed, Mar 4, 2015 at 9:42 AM, Tom Lane  wrote:
>>> and make it harder to compare entries by grepping out some common
>>> substring.
>
>> Could you give an example of the sort of thing you wish to do?
>
> On that angle, I'm dubious that a format that allows omission of fields is
> going to be easy for editing scripts to modify, no matter what the layout
> convention is.  I've found it relatively easy to write sed or even Emacs
> macros to add new column values to old-school pg_proc.h ... but in this
> brave new world, I'm going to be really hoping that the column default
> works for 99.9% of pg_proc entries when we add a new pg_proc column,
> because slipping a value into a desired position is gonna be hard for
> a script when you don't know whether the adjacent existing fields are
> present or not.

I wonder if we should have a tool in our repository to help people
edit the file.  So instead of going in there yourself and changing
things by hand, or writing your own script, you can do:

updatepgproc.pl --oid 5678 provolatile=v

or

updatepgpproc.pl --name='.*xact.*' prowhatever=someval

Regardless of what format we end up with, that seems like it would
make things easier.

-- 
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] improve pgbench syntax error messages

[Robert Haas]

On Tue, Mar 3, 2015 at 3:48 AM, Fabien COELHO  wrote:
> Report the origin of syntax errors from pgbench.
>
> Currently only the column number (for expressions) and command are
> essentially reported:
>
>   sh> ./pgbench -f bad.sql
>   syntax error at column 14
>   set: parse error
>
> The patch helps locate the origin of errors with the file name, line number
> and the actual text triggering the issue (either the line or an extract for
> expressions):
>
>   sh> ./pgbench -f bad.sql
>   syntax error at column 14
>   error while processing "bad.sql" line 3: (1021 * :id) %
>   set: parse error
>
> Whether using a macro is the right tool is debatable. The contents could be
> expanded, but that would mean replicating the same message over and over
> again, so it seems cleaner to me this way. An function seems overkill.

As I mentioned on the other thread, I'd really like to get this into a
better format, where each error message is on one line.  Looking at
that, you can't tell whether you've got one mistake, two mistakes, or
three mistakes.

-- 
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] Bootstrap DATA is a pita

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 4, 2015 at 9:06 AM, Peter Eisentraut  wrote:
>> Could you give an example of the sort of thing you wish to do?

> e.g. grep for a function name and check that all the matches have the
> same volatility.

Well, grep is not going to work too well anymore, but extracting a
specific field from an entry is going to be beyond the competence of
simple grep/sed tools anyway if we allow column default substitutions.

I think a fairer question is "can you do that in a one-liner Perl script",
which seems like it might be achievable given an appropriate choice of
data markup language.

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] Bootstrap DATA is a pita

[Andrew Dunstan]

On 03/04/2015 09:42 AM, Tom Lane wrote:

Peter Eisentraut  writes:

On 3/3/15 9:49 PM, Robert Haas wrote:

Even this promises to vastly increase the number of lines in the file,

I think lines are cheap.  Columns are much harder to deal with.

Yeah.  pg_proc.h is already impossible to work with in a standard
80-column window.  I don't want to find that the lines mostly wrap even
when I expand my editor window to full screen width, but that is certainly
what will happen if we adopt column labelling *and* insist that entries
remain all on one line.  (As a data point, the maximum usable Emacs window
width on my Mac laptop seems to be about 230 characters.)

It's possible that gaining the ability to depend on per-column defaults
would reduce the typical number of fields so much that pg_proc.h entries
would still fit on a line of 100-some characters ... but I'd want to see
proof before assuming that.  And pg_proc isn't even our widest catalog.
Some of the ones that are wider, like pg_am, don't seem like there would
be any scope whatsoever for saving space with per-column defaults.

So while I can see the attraction of trying to fit things on one line,
I doubt it's gonna work very well.  I'd rather go over to a
one-value-per-line format and live with lots of lines.




Is it necessarily an all or nothing deal?

Taking a previous example, we could have something like:

{
 oid => 2249,  oiddefine => 'CSTRINGOID',  typname => 'cstring',
 typlen => -2, typbyval => 1,
 ...
}

which would allow us to fit within a reasonable edit window (for my 
normal window and font that's around 180 characters) and still reduce 
the number of lines.


I'm not wedded to it, but it's a thought.

cheers

andrew


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

Re: [HACKERS] Bootstrap DATA is a pita

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Mar 4, 2015 at 9:42 AM, Tom Lane  wrote:
> >>> and make it harder to compare entries by grepping out some common
> >>> substring.
> >
> >> Could you give an example of the sort of thing you wish to do?
> >
> > On that angle, I'm dubious that a format that allows omission of fields is
> > going to be easy for editing scripts to modify, no matter what the layout
> > convention is.  I've found it relatively easy to write sed or even Emacs
> > macros to add new column values to old-school pg_proc.h ... but in this
> > brave new world, I'm going to be really hoping that the column default
> > works for 99.9% of pg_proc entries when we add a new pg_proc column,
> > because slipping a value into a desired position is gonna be hard for
> > a script when you don't know whether the adjacent existing fields are
> > present or not.
> 
> I wonder if we should have a tool in our repository to help people
> edit the file.  So instead of going in there yourself and changing
> things by hand, or writing your own script, you can do:
> 
> updatepgproc.pl --oid 5678 provolatile=v
> 
> or
> 
> updatepgpproc.pl --name='.*xact.*' prowhatever=someval
> 
> Regardless of what format we end up with, that seems like it would
> make things easier.

Alright, I'll bite on this- we have this really neat tool for editing
data in bulk, or individual values, or pulling out data to look at based
on particular values or even functions...  It's called PostgreSQL.

What if we had an easy way to export an existing table into whatever
format we decide to use for initdb to use?  For that matter, what if
that file was simple to import into PG?

What about having a way to load all the catalog tables from their git
repo files into a "pg_dev" schema?  Maybe even include a make target or
initdb option which does that?  (the point here being to provide a way
to modify the tables and compare the results to the existing tables
without breaking the instance one is using for this)

I have to admit that I've never tried to do that with the existing
format, but seems like an interesting idea to consider.  I further
wonder if it'd be possible to generate the table structures too..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Bootstrap DATA is a pita

[Andrew Dunstan]

On 03/04/2015 09:51 AM, Robert Haas wrote:

On Wed, Mar 4, 2015 at 9:06 AM, Peter Eisentraut  wrote:

and make it harder to compare entries by grepping out some common
substring.

Could you give an example of the sort of thing you wish to do?

e.g. grep for a function name and check that all the matches have the
same volatility.



I think grep will be the wrong tool for this format, but if we're 
settling on a perl format, a few perl one-liners should be able to work 
pretty well. It might be worth shipping a small perl module that 
provided some functions, or a script doing common tasks (or both).


cheers

andrew


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

Re: [HACKERS] Question about lazy_space_alloc() / linux over-commit

[Robert Haas]

On Wed, Feb 25, 2015 at 5:06 PM, Jim Nasby  wrote:
> Could the large allocation[2] for the dead tuple array in lazy_space_alloc
> cause problems with linux OOM? [1] and some other things I've read indicate
> that a large mmap will count towards total system memory, including
> producing a failure if overcommit is disabled.

I believe that this is possible.

> Would it be worth avoiding the full size allocation when we can?

Maybe.  I'm not aware of any evidence that this is an actual problem
as opposed to a theoretical one.  vacrelstats->dead_tuples is limited
to a 1GB allocation, which is not a trivial amount of memory, but it's
not huge, either.  But we could consider changing the representation
from a single flat array to a list of chunks, with each chunk capped
at say 64MB.  That would not only reduce the amount of memory that we
needlessly allocate, but would allow autovacuum to make use of more
than 1GB of maintenance_work_mem, which it looks like it currently
can't.  I'm not sure that's a huge problem right now either, because
it's probably rare to vacuum at able with more than 1GB / 6 =
178,956,970 dead tuples in it, but it would certainly suck if you did
and if the current 1GB limit forced you to do multiple vacuum passes.

-- 
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] collations in shared catalogs?

[Robert Haas]

On Wed, Feb 25, 2015 at 7:54 PM, David Steele  wrote:
> +1 on 128/256 character names.
>
>> /me runs and hides.
>
> /stands brazenly in the open and volunteers to try it if I don't get
> clobbered within seconds.

I think the question is whether making lots of rows in system catalogs
better is going to have undesirable effects on (a) the size of our
initial on-disk format (i.e. how big an empty database is), (b) the
amount of memory consumed by the syscache and relcaches on workloads
that touch lots of tables/functions/whatever, or (c) CPU consumption
mostly as a result of more cache line accesses for the same operation.
If you can prove those effects are minimal, that'd be a good place to
start.

-- 
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] Bootstrap DATA is a pita

[Andres Freund]

On 2015-03-04 09:55:01 -0500, Robert Haas wrote:
> On Wed, Mar 4, 2015 at 9:42 AM, Tom Lane  wrote:
> I wonder if we should have a tool in our repository to help people
> edit the file.  So instead of going in there yourself and changing
> things by hand, or writing your own script, you can do:
> 
> updatepgproc.pl --oid 5678 provolatile=v
> 
> or
> 
> updatepgpproc.pl --name='.*xact.*' prowhatever=someval
> 
> Regardless of what format we end up with, that seems like it would
> make things easier.

The stuff I've started to work on basically allows to load the stuff
that Catalog.pm provides (in an extended format), edit it in memory, and
then serialize it again. So such a thing could relatively easily be
added if somebody wants to do so.  I sure hope though that the need for
it will become drastically lower with the new format.

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] Bootstrap DATA is a pita

[Robert Haas]

On Wed, Mar 4, 2015 at 10:04 AM, Andrew Dunstan  wrote:
> Is it necessarily an all or nothing deal?
>
> Taking a previous example, we could have something like:
>
> {
>  oid => 2249,  oiddefine => 'CSTRINGOID',  typname => 'cstring',
>  typlen => -2, typbyval => 1,
>  ...
> }
>
> which would allow us to fit within a reasonable edit window (for my normal
> window and font that's around 180 characters) and still reduce the number of
> lines.
>
> I'm not wedded to it, but it's a thought.

Another advantage of this is that it would probably make git less
likely to fumble a rebase.  If there are lots of places in the file
where we have the same 10 lines in a row with occasional variations,
rebasing a patch could easily pick the the wrong place to reapply the
hunk.  I would personally consider a substantial increase in the rate
of such occurrences as being a cure far, far worse than the disease.
If you keep the entry for each function on just a couple of lines the
chances of this happening are greatly reduced, because you're much
likely to get a false match to surrounding context.

-- 
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] Weirdly pesimistic estimates in optimizer

[Robert Haas]

On Mon, Mar 2, 2015 at 2:19 PM, David Kubečka  wrote:
> The question is why optimizer, or rather the cost estimator, produced so
> much different estimates upon very small change in input. Moreover it seems
> that the main culprit of bad estimates isn't actually directly related to
> outer table, but rather to inner table. Just compare estimates for the two
> index scans:
>
> With 'random_fk_dupl':
>  ->  Index Scan using facts_fk_idx on facts  (cost=0.42..5.75
> rows=100 width=15) (actual time=0.009..0.117 rows=98 loops=100)
> With 'random_fk_uniq':
>  ->  Index Scan using facts_fk_idx on facts  (cost=0.42..214.26
> rows=100 width=15) (actual time=0.007..0.109 rows=98 loops=100)

Whoa.  That's pretty dramatic.  Am I correctly understanding that the
two tables contain *exactly* the same data?  Could the issue be that
the optimizer thinks that one of the tables is thought to have a
higher logical-to-physical correlation than the other (see
pg_stats.correlation)?

-- 
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] Bootstrap DATA is a pita

[Tom Lane]

Robert Haas  writes:
> Another advantage of this is that it would probably make git less
> likely to fumble a rebase.  If there are lots of places in the file
> where we have the same 10 lines in a row with occasional variations,
> rebasing a patch could easily pick the the wrong place to reapply the
> hunk.

That is a really, really good point.

I had been thinking it was a disadvantage of Andrew's proposal that
line breaks would tend to fall in inconsistent places from one entry
to another ... but from this perspective, maybe that's not such a
bad thing.

regards, tom lane


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

Re: [HACKERS] collations in shared catalogs?

[David Steele]

Hi Robert,

On 3/4/15 10:14 AM, Robert Haas wrote:
> On Wed, Feb 25, 2015 at 7:54 PM, David Steele  wrote:
>> +1 on 128/256 character names.
>>
>>> /me runs and hides.
>>
>> /stands brazenly in the open and volunteers to try it if I don't get
>> clobbered within seconds.
> 
> I think the question is whether making lots of rows in system catalogs
> better is going to have undesirable effects on (a) the size of our
> initial on-disk format (i.e. how big an empty database is), (b) the
> amount of memory consumed by the syscache and relcaches on workloads
> that touch lots of tables/functions/whatever, or (c) CPU consumption
> mostly as a result of more cache line accesses for the same operation.
> If you can prove those effects are minimal, that'd be a good place to
> start.

Thanks, that's encouraging.  I've already compiled with NAMEDATALEN=256
and verified that the only failing tests are the ones making sure that
identifier lengths are truncated or fail appropriately when they are >
63.  I'm sure lots of people have done that before and gotten the same
result.

I'm currently investigating the issues that you've identified above
since they constitute the real problem with increasing NAMEDATALEN.
Once I have some answers I'll send a proposal to hackers.

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c

[Robert Haas]

On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier
 wrote:
> Hm, why not. That would remove all inconsistencies between the parser
> and the autovacuum code path. Perhaps something like the attached
> makes sense then?

I really don't see this patch, or any of the previous ones, as solving
any actual problem.  There's no bug here, and no reason to suspect
that future code changes would be particularly like to introduce one.
Assertions are a great way to help developers catch coding mistakes,
but it's a real stretch to think that a developer is going to add a
new syntax for ANALYZE that involves setting options proper to VACUUM
and not notice it.

This thread started out because Michael read an assertion in the code
and misunderstood what that assertion was trying to guard against.
I'm not sure there's any code change needed here at all, but if there
is, I suggest we confine it to adding a one-line comment above that
assertion clarifying its purpose, like /* check that parser didn't
produce ANALYZE FULL or ANALYZE FREEZE */.

-- 
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] MD5 authentication needs help

[Magnus Hagander]

On Wed, Mar 4, 2015 at 4:52 PM, Stephen Frost  wrote:

>
> A lot of discussion has been going on with SCRAM and SASL, which is all
> great, but that means we end up with a dependency on SASL or we have to
> reimplement SCRAM (which I've been thinking might not be a bad idea-
> it's actually not that hard), but another suggestion was made which may
>


I'd really rather not add a dependency on SASL if we can avoid it. I
haven't read up on SCRAM, but if it's reasonable enough to reimplement - or
if there is a BSD licensed implementation that we can import into our own
sourcetree without adding a dependency on SASL, that sounds like a good way
to proceed.



> be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
> RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We already
> have OpenSSL and therefore this wouldn't create any new dependencies and
> might be slightly simpler to implement.
>


OpenSSL is not a *requirement* today, it's an optional dependency.  Given
it's license we really can't make it a mandatory requirement I think. So if
we go down that route, we still leave md5 in there as the one that works
everywhere.

Also AFAICT TLS-SRP actually requires the connection to be over TLS - so
are you suggesting that TLS becomes mandatory?

It sounds like something that could be interesting to have, but not as a
solution to the "md5 problem", imo.

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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

Bruce, all,

I've been discussing this with a few folks outside of the PG community
(Debian and Openwall people specifically) and a few interesting ideas
have come out of that which might be useful to discuss.

The first is a "don't break anything" approach which would move the
needle between "network data sensitivity" and "on-disk data sensitivity"
a bit back in the direction of making the network data more sensitive.

this approach looks like this: pre-determine and store the values (on a
per-user basis, so a new field in pg_authid or some hack on the existing
field) which will be sent to the client in the AuthenticationMD5Password
message.  Further, calculate a random salt to be used when storing data
in pg_authid.  Then, for however many variations we feel are necessary,
calculate and store, for each AuthenticationMD5Password value:

md5_challenge, hash(salt || response)

We wouldn't store 4 billion of these, of course, which means that the
challenge / response system becomes less effective on a per-user basis.
We could, however, store X number of these and provide a lock-out
mechanism (something users have asked after for a long time..) which
would make it likely that the account would be locked before the
attacker was able to gain access.  Further, an attacker with access to
the backend still wouldn't see the user's cleartext password, nor would
we store the cleartext password or a token in pg_authid which could be
directly used for authentication, and we don't break the wireline
protocol or existing installations (since we could detect that the
pg_authid entry has the old-style and simply 'upgrade' it).

That's probably the extent of what we could do to improve the current
'md5' approach without breaking the wireline protocol or existing stored
data.

A lot of discussion has been going on with SCRAM and SASL, which is all
great, but that means we end up with a dependency on SASL or we have to
reimplement SCRAM (which I've been thinking might not be a bad idea-
it's actually not that hard), but another suggestion was made which may
be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We already
have OpenSSL and therefore this wouldn't create any new dependencies and
might be slightly simpler to implement.

Thoughts?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Abbreviated keys for Numeric

[Robert Haas]

On Mon, Feb 23, 2015 at 5:59 AM, Andrew Gierth
 wrote:
>> "Tomas" == Tomas Vondra  writes:
>  Tomas> Interesting, but I think Gavin was asking about how much
>  Tomas> variation was there for each tested case (e.g. query executed on
>  Tomas> the same code / dataset). And in those cases the padding /
>  Tomas> alignment won't change, and thus the effects you describe won't
>  Tomas> influence the results, no?
>
> My point is exactly the fact that since the result is not affected, this
> variation between runs of the same code is of no real relevance to the
> question of whether a given change in performance can properly be
> attributed to a patch.
>
> Put it this way: suppose I have a test that when run repeatedly with no
> code changes takes 6.10s (s=0.025s), and I apply a patch that changes
> that to 6.26s (s=0.025s). Did the patch have an impact on performance?
>
> Now suppose that instead of applying the patch I insert random amounts
> of padding in an unused function and find that my same test now takes a
> mean of 6.20s (s=0.058s) when I take the best timing for each padding
> size and calculate stats across sizes. Now it looks obvious that the
> actual code of the patch probably wasn't responsible for any change...
>
> The numbers used here aren't theoretical; they are obtained by testing a
> single query - "select * from d_flt order by v offset 1000" where
> d_flt contains 5 million float8 values - over 990 times with 33
> different random padding sizes (uniform in 0-32767). Here's a scatter
> plot, with 3 runs of each padding size so you can see the repeatability:
> http://tinyurl.com/op9qg8a

Neat chart.

-- 
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] Strange assertion using VACOPT_FREEZE in vacuum.c

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier
>  wrote:
> > Hm, why not. That would remove all inconsistencies between the parser
> > and the autovacuum code path. Perhaps something like the attached
> > makes sense then?
> 
> I really don't see this patch, or any of the previous ones, as solving
> any actual problem.  There's no bug here, and no reason to suspect
> that future code changes would be particularly like to introduce one.
> Assertions are a great way to help developers catch coding mistakes,
> but it's a real stretch to think that a developer is going to add a
> new syntax for ANALYZE that involves setting options proper to VACUUM
> and not notice it.

Yeah, I haven't been terribly excited about it for the same reasons.
Had Michael's latest patch meant that we didn't need to pass VacuumStmt
down into the other functions then I might have been a bit more behind
it, but as is we seem to be simply duplicating everything except the
actual Node entry in the struct, which kind of missed the point.

> This thread started out because Michael read an assertion in the code
> and misunderstood what that assertion was trying to guard against.
> I'm not sure there's any code change needed here at all, but if there
> is, I suggest we confine it to adding a one-line comment above that
> assertion clarifying its purpose, like /* check that parser didn't
> produce ANALYZE FULL or ANALYZE FREEZE */.

I'd be fine with that.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Andres Freund]

Hi,

On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> I've been discussing this with a few folks outside of the PG community
> (Debian and Openwall people specifically) and a few interesting ideas
> have come out of that which might be useful to discuss.
> 
> The first is a "don't break anything" approach which would move the
> needle between "network data sensitivity" and "on-disk data sensitivity"
> a bit back in the direction of making the network data more sensitive.

I think that's a really bad tradeoff for pg. There's pretty good reasons
not to encrypt database connections. I don't think you really can
compare routinely encrypted stuff like imap and submission with
pg. Neither is it as harmful to end up with leaked hashes for database
users as it is for a email provider's authentication database.

> A lot of discussion has been going on with SCRAM and SASL, which is all
> great, but that means we end up with a dependency on SASL or we have to
> reimplement SCRAM (which I've been thinking might not be a bad idea-
> it's actually not that hard), but another suggestion was made which may
> be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
> RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We already
> have OpenSSL and therefore this wouldn't create any new dependencies and
> might be slightly simpler to implement.

We don't have a hard dependency openssl, so I can't really see that
being a fully viable alternative to md5 TBH.

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] MD5 authentication needs help

[Stephen Frost]

Magnus,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Wed, Mar 4, 2015 at 4:52 PM, Stephen Frost  wrote:
> > A lot of discussion has been going on with SCRAM and SASL, which is all
> > great, but that means we end up with a dependency on SASL or we have to
> > reimplement SCRAM (which I've been thinking might not be a bad idea-
> > it's actually not that hard), but another suggestion was made which may
> 
> I'd really rather not add a dependency on SASL if we can avoid it. I
> haven't read up on SCRAM, but if it's reasonable enough to reimplement - or
> if there is a BSD licensed implementation that we can import into our own
> sourcetree without adding a dependency on SASL, that sounds like a good way
> to proceed.

I actually like the idea of supporting SASL generally, but I agree that
we don't really want to force it as a dependency.  I've started looking
around for BSD-licensed SCRAM implementations and will update with any I
find that are worthwhile to review.

> > be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
> > RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We already
> > have OpenSSL and therefore this wouldn't create any new dependencies and
> > might be slightly simpler to implement.
> 
> OpenSSL is not a *requirement* today, it's an optional dependency.  Given
> it's license we really can't make it a mandatory requirement I think. So if
> we go down that route, we still leave md5 in there as the one that works
> everywhere.
> 
> Also AFAICT TLS-SRP actually requires the connection to be over TLS - so
> are you suggesting that TLS becomes mandatory?
> 
> It sounds like something that could be interesting to have, but not as a
> solution to the "md5 problem", imo.

No, I'm not suggesting that OpenSSL or TLS become mandatory but was
thinking it might be good alternative as a middle-ground between full
client-and-server side certificates and straight password-based auth
(which is clearly why it was invented in the first place) and so, yes,
md5 would still have to be kept around, but we'd at least be able to
deprecate it and tell people "Use TLS-SRP if you really want to use
passwords and care about network security".

SCRAM doesn't actually fix the issue with network connection hijacking
or eavesdropping, except to the extent that it protects the password
itself, and so we might want to recommend, for people who are worried
about network-based attacks, using TLS-SRP.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Magnus Hagander]

On Wed, Mar 4, 2015 at 5:03 PM, Stephen Frost  wrote:

> Magnus,
>
> * Magnus Hagander (mag...@hagander.net) wrote:
> > On Wed, Mar 4, 2015 at 4:52 PM, Stephen Frost 
> wrote:
> > > A lot of discussion has been going on with SCRAM and SASL, which is all
> > > great, but that means we end up with a dependency on SASL or we have to
> > > reimplement SCRAM (which I've been thinking might not be a bad idea-
> > > it's actually not that hard), but another suggestion was made which may
> >
> > I'd really rather not add a dependency on SASL if we can avoid it. I
> > haven't read up on SCRAM, but if it's reasonable enough to reimplement -
> or
> > if there is a BSD licensed implementation that we can import into our own
> > sourcetree without adding a dependency on SASL, that sounds like a good
> way
> > to proceed.
>
> I actually like the idea of supporting SASL generally, but I agree that
> we don't really want to force it as a dependency.  I've started looking
> around for BSD-licensed SCRAM implementations and will update with any I
> find that are worthwhile to review.
>
> > > be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
> > > RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We
> already
> > > have OpenSSL and therefore this wouldn't create any new dependencies
> and
> > > might be slightly simpler to implement.
> >
> > OpenSSL is not a *requirement* today, it's an optional dependency.  Given
> > it's license we really can't make it a mandatory requirement I think. So
> if
> > we go down that route, we still leave md5 in there as the one that works
> > everywhere.
> >
> > Also AFAICT TLS-SRP actually requires the connection to be over TLS - so
> > are you suggesting that TLS becomes mandatory?
> >
> > It sounds like something that could be interesting to have, but not as a
> > solution to the "md5 problem", imo.
>
> No, I'm not suggesting that OpenSSL or TLS become mandatory but was
> thinking it might be good alternative as a middle-ground between full
> client-and-server side certificates and straight password-based auth
> (which is clearly why it was invented in the first place) and so, yes,
> md5 would still have to be kept around, but we'd at least be able to
> deprecate it and tell people "Use TLS-SRP if you really want to use
> passwords and care about network security".
>
> SCRAM doesn't actually fix the issue with network connection hijacking
> or eavesdropping, except to the extent that it protects the password
> itself, and so we might want to recommend, for people who are worried
> about network-based attacks, using TLS-SRP.
>

Assuming we do implement SCRAM, what does TLS-SRP give us that we wouldn't
get by just using SCRAM over a TLS connection?

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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Andres Freund (and...@2ndquadrant.com) wrote:
> Hi,
> 
> On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> > I've been discussing this with a few folks outside of the PG community
> > (Debian and Openwall people specifically) and a few interesting ideas
> > have come out of that which might be useful to discuss.
> > 
> > The first is a "don't break anything" approach which would move the
> > needle between "network data sensitivity" and "on-disk data sensitivity"
> > a bit back in the direction of making the network data more sensitive.
> 
> I think that's a really bad tradeoff for pg. There's pretty good reasons
> not to encrypt database connections. I don't think you really can
> compare routinely encrypted stuff like imap and submission with
> pg. Neither is it as harmful to end up with leaked hashes for database
> users as it is for a email provider's authentication database.

I'm confused..  The paragraph you reply to here discusses an approach
which doesn't include encrypting the database connection.

> > A lot of discussion has been going on with SCRAM and SASL, which is all
> > great, but that means we end up with a dependency on SASL or we have to
> > reimplement SCRAM (which I've been thinking might not be a bad idea-
> > it's actually not that hard), but another suggestion was made which may
> > be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
> > RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We already
> > have OpenSSL and therefore this wouldn't create any new dependencies and
> > might be slightly simpler to implement.
> 
> We don't have a hard dependency openssl, so I can't really see that
> being a fully viable alternative to md5 TBH.

Right, agreed, that wasn't intended to be a complete replacement for md5
but rather an additional auth mechanism we could get nearly "for free"
which would provide password-based authentication with network-level
encryption for users who are worried about network-based attacks (and
therefore want to or are already using TLS, as Debian is configured to
do by default...).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Combining Aggregates

[Robert Haas]

On Wed, Mar 4, 2015 at 4:41 AM, David Rowley  wrote:
>> This thread mentions "parallel queries" as a use case, but that means
>> passing data between processes, and that requires being able to
>> serialize and deserialize the aggregate state somehow. For actual data
>> types that's not overly difficult I guess (we can use in/out functions),
>> but what about aggretates using 'internal' state? I.e. aggregates
>> passing pointers that we can't serialize?
>
> This is a good question. I really don't know the answer to it as I've not
> looked at the Robert's API for passing data between backends yet.
>
> Maybe Robert or Amit can answer this?

I think parallel aggregation will probably require both the
infrastructure discussed here, namely the ability to combine two
transition states into a single transition state, and also the ability
to serialize and de-serialize transition states, which has previously
been discussed in the context of letting hash aggregates spill to
disk.  My current thinking is that the parallel plan will look
something like this:

HashAggregateFinish
-> Funnel
-> HashAggregatePartial
  -> PartialHeapScan

So the workers will all pull from a partial heap scan and each worker
will aggregate its own portion of the data.  Then it'll need to pass
the results of that step back to the master, which will aggregate the
partial results and produce the final results.  I'm guessing that if
we're grouping on, say, column a, the HashAggregatePartial nodes will
kick out 2-column tuples of the form (, ).

Of course this is all pie in the sky right now, but I think it's a
pretty good bet that partial aggregation is a useful thing to be able
to do.  Postgres-XC put a bunch of work into this, too, so there's
lots of people saying "hey, we want that".

-- 
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] MD5 authentication needs help

[Andres Freund]

On 2015-03-04 11:06:33 -0500, Stephen Frost wrote:
> * Andres Freund (and...@2ndquadrant.com) wrote:
> > On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> > > The first is a "don't break anything" approach which would move the
> > > needle between "network data sensitivity" and "on-disk data sensitivity"
> > > a bit back in the direction of making the network data more sensitive.
> > 
> > I think that's a really bad tradeoff for pg. There's pretty good reasons
> > not to encrypt database connections. I don't think you really can
> > compare routinely encrypted stuff like imap and submission with
> > pg. Neither is it as harmful to end up with leaked hashes for database
> > users as it is for a email provider's authentication database.
> 
> I'm confused..  The paragraph you reply to here discusses an approach
> which doesn't include encrypting the database connection.

An increase in "network data sensitivity" also increases the need for
encryption.

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] xpath changes in the recent back branches

[Robert Haas]

On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja  wrote:
> Commit 79af9a1d2668c9edc8171f03c39e7fed571eeb98 changed xpath handling with
> regard to namespaces, and it seems to be fixing an actual issue. However, it
> was also backpatched to all branches despite it breaking for example code
> like this:
>
> do $$
> declare
> _x xml;
> begin
> _x := (xpath('/x:Foo/x:Bar', xml ' xmlns="teh:urn">12',
> array[['x','teh:urn']]))[1];
> raise notice '%', xpath('/Bar/Baz/text()', _x);
> raise notice '%', xpath('/Bar/Bat/text()', _x);
> end
> $$;
>
> The problem is that there's no way to write the code like this in such a way
> that it would work on both versions.  If I add the namespace, it's broken on
> 9.1.14.  Without it it's broken on 9.1.15.

That certainly sucks.

> I'm not sure how changing behavior like this in a minor release was
> considered acceptable.

I'm guessing that the fact that it changes behavior in cases like this
wasn't recognized, but I suppose Peter will have to be the one to
comment on that.

-- 
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] xpath changes in the recent back branches

[Tom Lane]

Robert Haas  writes:
> On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja  wrote:
>> I'm not sure how changing behavior like this in a minor release was
>> considered acceptable.

> I'm guessing that the fact that it changes behavior in cases like this
> wasn't recognized, but I suppose Peter will have to be the one to
> comment on that.

It was considered to be a bug fix; more, given the few complaints about
the clearly-broken old behavior, we thought it was a fix that would affect
few people, and them positively.  Sorry you're not happy about it, but
these things are always judgment calls.

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] MD5 authentication needs help

[Stephen Frost]

* Andres Freund (and...@2ndquadrant.com) wrote:
> On 2015-03-04 11:06:33 -0500, Stephen Frost wrote:
> > * Andres Freund (and...@2ndquadrant.com) wrote:
> > > On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> > > > The first is a "don't break anything" approach which would move the
> > > > needle between "network data sensitivity" and "on-disk data sensitivity"
> > > > a bit back in the direction of making the network data more sensitive.
> > > 
> > > I think that's a really bad tradeoff for pg. There's pretty good reasons
> > > not to encrypt database connections. I don't think you really can
> > > compare routinely encrypted stuff like imap and submission with
> > > pg. Neither is it as harmful to end up with leaked hashes for database
> > > users as it is for a email provider's authentication database.
> > 
> > I'm confused..  The paragraph you reply to here discusses an approach
> > which doesn't include encrypting the database connection.
> 
> An increase in "network data sensitivity" also increases the need for
> encryption.

Ok, I see what you're getting at there, though our existing md5
implementation with no lock-out mechanism or ability to deal with
hijacking isn't exactly making us all that safe when it comes to network
based attacks.  The best part about md5 is that we don't send the user's
password over the wire in the clear, the actual challenge/response piece
is not considered terribly secure today, nor is the salt+password we use
for pg_authid for that matter. :/

SCRAM won't fix network connection hijacking but it does address replay
attacks better than our current challenge/response system (at least the
example in the RFC uses a 16-byte base64-encoded salt)) and the on-disk
storage risk (multiple iterations are supported), and multiple hashing
algorithms can be supported including ones much better than what we
support today (eg: SHA256) which applies to both network and on-disk
vectors.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Comparing primary/HS standby in tests

[Jeff Janes]

On Tue, Mar 3, 2015 at 7:49 AM, Andres Freund 
wrote:

> Hi,
>
> I've regularly wished we had automated tests that setup HS and then
> compare primary/standby at the end to verify replay worked
> correctly.
>
> Heikki's page comparison tools deals with some of that verification, but
> it's really quite expensive and doesn't care about runtime only
> differences. I.e. it doesn't test HS at all.
>
> I every now and then run installcheck against a primary, verify that
> replay works without errors, and then compare pg_dumpall from both
> clusters. Unfortunately that currently requires hand inspection of
> dumps, there are differences like:
> -SELECT pg_catalog.setval('default_seq', 1, true);
> +SELECT pg_catalog.setval('default_seq', 33, true);
>
> The reason these differences is that the primary increases the
> sequence's last_value by 1, but temporarily sets it to +SEQ_LOG_VALS
> before XLogInsert(). So the two differ.
>
> Does anybody have a good idea how to get rid of that difference? One way
> to do that would be to log the value the standby is sure to have - but
> that's not entirely trivial.
>
> I'd very much like to add a automated test like this to the tree, but I
> don't see wa way to do that sanely without a comparison tool...
>

Couldn't we just arbitrarily exclude sequence internal states from the
comparison?

That wouldn't work where the standby has been promoted and then used in a
way that draws on the sequence (with the same workload being put through
the now-promoted standby and the original-master), though, but I don't
think that that was what you were asking about.

How many similar issues have you seen?

In the case where you have a promoted replica and put the same through
workflow through both it and the master, I've seen "pg_dump -s" dump
objects in different orders, for no apparent reason.  That is kind of
annoying, but I never traced it back to the cause (nor have I excluded
PEBCAK as the real cause).

Cheers,

Jeff

Re: [HACKERS] improve pgbench syntax error messages

[Fabien COELHO]

As I mentioned on the other thread, I'd really like to get this into a
better format, where each error message is on one line.  Looking at
that, you can't tell whether you've got one mistake, two mistakes, or
three mistakes.


Indeed. Here is a v2.

  sh> ./pgbench -f bad.sql
  bad.sql:3: syntax error at column 23 in command "set"
  \set aid (1021 * :id) %
^ error found here

  sh> ./pgbench -f bad2.sql
  bad2.sql:1: unexpected argument (x) at column 25 in command "setrandom"
 \setrandom id 1 1000 x
  ^ error found here

  sh> ./pgbench -f bad3.sql
  bad3.sql:1: too many arguments (gaussian) at column 35 in command "setrandom"
  \setrandom foo 1 10 gaussian 10.3 1
^ error found here

 sh> ./pgbench -f bad4.sql
  bad4.sql:1: missing threshold argument (exponential) in command "setrandom"
  \setrandom foo 1 10 exponential


I think that transforming unexpected garbage in errors would be a good 
move, even if this breaks backwards compatibility (currently a warning is 
printed about ignored extra stuff).


--
Fabien.diff --git a/contrib/pgbench/exprscan.l b/contrib/pgbench/exprscan.l
index 4c9229c..dda2635 100644
--- a/contrib/pgbench/exprscan.l
+++ b/contrib/pgbench/exprscan.l
@@ -57,24 +57,36 @@ space			[ \t\r\f]
 }
 %%
 
+static char *expr_source = NULL;
+static int expr_lineno = 0;
+static char *expr_full_line = NULL;
+static char *expr_command = NULL;
+static int expr_col = 0;
+
 void
 yyerror(const char *message)
 {
-	/* yyline is always 1 as pgbench calls the parser for each line...
-	 * so the interesting location information is the column number */
-	fprintf(stderr, "%s at column %d\n", message, yycol);
-	/* go on to raise the error from pgbench with more information */
-	/* exit(1); */
+	syntax_error(expr_source, expr_lineno, expr_full_line, expr_command,
+ message, NULL, expr_col + yycol);
 }
 
 /*
  * Called before any actual parsing is done
  */
 void
-expr_scanner_init(const char *str)
+expr_scanner_init(const char *str, const char *source,
+  const int lineno, const char *line,
+  const char *cmd, const int ecol)
 {
 	Size	slen = strlen(str);
 
+	/* save context informations for error messages */
+	expr_source = (char *) source;
+	expr_lineno = (int) lineno;
+	expr_full_line = (char *) line;
+	expr_command = (char *) cmd;
+	expr_col = (int) ecol;
+
 	/*
 	 * Might be left over after error
 	 */
@@ -102,4 +114,9 @@ expr_scanner_finish(void)
 {
 	yy_delete_buffer(scanbufhandle);
 	pg_free(scanbuf);
+	expr_source = NULL;
+	expr_lineno = 0;
+	expr_full_line = NULL;
+	expr_command = NULL;
+	expr_col = 0;
 }
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 706fdf5..3e50bae 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -291,6 +291,7 @@ typedef struct
 	int			type;			/* command type (SQL_COMMAND or META_COMMAND) */
 	int			argc;			/* number of command words */
 	char	   *argv[MAX_ARGS]; /* command word list */
+	int			cols[MAX_ARGS]; /* corresponding column starting from 1 */
 	PgBenchExpr *expr;			/* parsed expression */
 } Command;
 
@@ -2189,6 +2190,28 @@ parseQuery(Command *cmd, const char *raw_sql)
 	return true;
 }
 
+void syntax_error(const char *source, const int lineno,
+  const char *line, const char *command,
+  const char *msg, const char *more, const int column)
+{
+	fprintf(stderr, "%s:%d: %s", source, lineno, msg);
+	if (more)
+		fprintf(stderr, " (%s)", more);
+	if (column != -1)
+		fprintf(stderr, " at column %d", column);
+	fprintf(stderr, " in command \"%s\"\n", command);
+	if (line) {
+		fprintf(stderr, "%s\n", line);
+		if (column != -1) {
+			int i = 0;
+			for (i = 0; i < column - 1; i++)
+fprintf(stderr, " ");
+			fprintf(stderr, "^ error found here\n");
+		}
+	}
+	exit(1);
+}
+
 /* Parse a command; return a Command struct, or NULL if it's a comment */
 static Command *
 process_commands(char *buf, const char *source, const int lineno)
@@ -2200,6 +2223,11 @@ process_commands(char *buf, const char *source, const int lineno)
 	char	   *p,
 			   *tok;
 
+/* error message generation helper */
+#define SYNTAX_ERROR(msg, more, col)	\
+	syntax_error(source, lineno, my_commands->line,		\
+ my_commands->argv[0], msg, more, col)
+
 	/* Make the string buf end at the next newline */
 	if ((p = strchr(buf, '\n')) != NULL)
 		*p = '\0';
@@ -2228,11 +2256,13 @@ process_commands(char *buf, const char *source, const int lineno)
 		j = 0;
 		tok = strtok(++p, delim);
 
+		/* stop "set" argument parsing the variable name */
 		if (tok != NULL && pg_strcasecmp(tok, "set") == 0)
 			max_args = 2;
 
 		while (tok != NULL)
 		{
+			my_commands->cols[j] = tok - buf + 1;
 			my_commands->argv[j++] = pg_strdup(tok);
 			my_commands->argc++;
 			if (max_args >= 0 && my_commands->argc >= max_args)
@@ -2250,9 +2280,9 @@ process_commands(char *buf, const char *source, const int lineno)
 
 			if (my_commands->argc < 4)
 			{
-f

Re: [HACKERS] xpath changes in the recent back branches

[Mike Rylander]

On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja  wrote:

> Hi,
>
> Commit 79af9a1d2668c9edc8171f03c39e7fed571eeb98 changed xpath handling
> with regard to namespaces, and it seems to be fixing an actual issue.
> However, it was also backpatched to all branches despite it breaking for
> example code like this:
>
> do $$
> declare
> _x xml;
> begin
> _x := (xpath('/x:Foo/x:Bar', xml ' xmlns="teh:urn">12',
> array[['x','teh:urn']]))[1];
> raise notice '%', xpath('/Bar/Baz/text()', _x);
> raise notice '%', xpath('/Bar/Bat/text()', _x);
> end
> $$;
>
> The problem is that there's no way to write the code like this in such a
> way that it would work on both versions.  If I add the namespace, it's
> broken on 9.1.14.  Without it it's broken on 9.1.15.
>
> I'm now thinking of adding a workaround which strips namespaces, but that
> doesn't seem to be easy to do, even with PL/Perl.  Is there a better
> workaround here that I'm not seeing?
>
>
FWIW, I've been working around the bug fixed in that commit for ages by
spelling my xpath like this:

  xpath('/*[local-name()="Bar"]/*[local-name()="Baz"]/text()', data)

I've modularized my XML handling functions so the source of 'data' is
immaterial -- maybe it's a full document, maybe it's a fragment from a
previous xpath() call -- and the referenced commit is going to make correct
XPATH much more sane, readable, and maintainable.  I, for one, welcome it
wholeheartedly.

HTH,

--Mike

Re: [HACKERS] compress method for spgist - 2

[Paul Ramsey]

On Wed, Feb 25, 2015 at 6:13 AM, Heikki Linnakangas
 wrote:
> In the original post on this, you mentioned that the PostGIS guys planned to
> use this to store polygons, as bounding boxes
> (http://www.postgresql.org/message-id/5447b3ff.2080...@sigaev.ru). Any idea
> how would that work?

Poorly, by hanging boxes that straddled dividing lines off the parent
node in a big linear list. The hope would be that the case was
sufficiently rare compared to the overall volume of data, to not be an
issue. Oddly enough this big hammer has worked in other
implementations at least passable well
(https://github.com/mapserver/mapserver/blob/branch-7-0/maptree.c#L261)

P.


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

Re: [HACKERS] File based Incremental backup v8

[Marco Nenciarini]

Hi Fujii,

Il 03/03/15 11:48, Fujii Masao ha scritto:
> On Tue, Mar 3, 2015 at 12:36 AM, Marco Nenciarini
>  wrote:
>> Il 02/03/15 14:21, Fujii Masao ha scritto:
>>> On Thu, Feb 12, 2015 at 10:50 PM, Marco Nenciarini
>>>  wrote:
 Hi,

 I've attached an updated version of the patch.
>>>
>>> basebackup.c:1565: warning: format '%lld' expects type 'long long
>>> int', but argument 8 has type '__off_t'
>>> basebackup.c:1565: warning: format '%lld' expects type 'long long
>>> int', but argument 8 has type '__off_t'
>>> pg_basebackup.c:865: warning: ISO C90 forbids mixed declarations and code
>>>
>>
>> I'll add the an explicit cast at that two lines.
>>
>>> When I applied three patches and compiled the code, I got the above 
>>> warnings.
>>>
>>> How can we get the full backup that we can use for the archive recovery, 
>>> from
>>> the first full backup and subsequent incremental backups? What commands 
>>> should
>>> we use for that, for example? It's better to document that.
>>>
>>
>> I've sent a python PoC that supports the plain format only (not the tar one).
>> I'm currently rewriting it in C (with also the tar support) and I'll send a 
>> new patch containing it ASAP.
> 
> Yeah, if special tool is required for that purpose, the patch should include 
> it.
> 

I'm working on it. The interface will be exactly the same of the PoC script 
I've attached to 

54c7cdad.6060...@2ndquadrant.it

>>> What does "1" of the heading line in backup_profile mean?
>>>
>>
>> Nothing. It's a version number. If you think it's misleading I will remove 
>> it.
> 
> A version number of file format of backup profile? If it's required for
> the validation of backup profile file as a safe-guard, it should be included
> in the profile file. For example, it might be useful to check whether
> pg_basebackup executable is compatible with the "source" backup that
> you specify. But more info might be needed for such validation.
> 

The current implementation bail out with an error if the header line is 
different from what it expect.
It also reports and error if the 2nd line is not the start WAL location. That's 
all that pg_basebackup needs to start a new incremental backup. All the other 
information are useful to reconstruct a full backup in case of an incremental 
backup, or maybe to check the completeness of an archived full backup.
Initially the profile was present only in incremental backups, but after some 
discussion on list we agreed to always write it.

>>> Sorry if this has been already discussed so far. Why is a backup profile 
>>> file
>>> necessary? Maybe it's necessary in the future, but currently seems not.
>>
>> It's necessary because it's the only way to detect deleted files.
> 
> Maybe I'm missing something. Seems we can detect that even without a profile.
> For example, please imagine the case where the file has been deleted since
> the last full backup and then the incremental backup is taken. In this case,
> that deleted file exists only in the full backup. We can detect the deletion 
> of
> the file by checking both full and incremental backups.
> 

When you take an incremental backup, only changed files are sent. Without the 
backup_profile in the incremental backup, you cannot detect a deleted file, 
because it's indistinguishable from a file that is not changed.

>>> We've really gotten the consensus about the current design, especially that
>>> every files basically need to be read to check whether they have been 
>>> modified
>>> since last backup even when *no* modification happens since last backup?
>>
>> The real problem here is that there is currently no way to detect that a 
>> file is not changed since the last backup. We agreed to not use file system 
>> timestamps as they are not reliable for that purpose.
> 
> TBH I prefer timestamp-based approach in the first version of incremental 
> backup
> even if's less reliable than LSN-based one. I think that some users who are
> using timestamp-based rsync (i.e., default mode) for the backup would be
> satisfied with timestamp-based one.

The original design was to compare size+timestamp+checksums (only if everything 
else matches and the file has been modified after the start of the backup), but 
the feedback from the list was that we cannot trust the filesystem mtime and we 
must use LSN instead.

> 
>> Using LSN have a significant advantage over using checksum, as we can start 
>> the full copy as soon as we found a block whith a LSN greater than the 
>> threshold.
>> There are two cases: 1) the file is changed, so we can assume that we detect 
>> it after reading 50% of the file, then we send it taking advantage of file 
>> system cache; 2) the file is not changed, so we read it without sending 
>> anything.
>> It will end up producing an I/O comparable to a normal backup.
> 
> Yeah, it might make the situation better than today. But I'm afraid that
> many users might get disappointed about that behavior of an incremental
> backup after the rel

Re: [HACKERS] Comparing primary/HS standby in tests

[Andres Freund]

On 2015-03-04 08:41:23 -0800, Jeff Janes wrote:
> Couldn't we just arbitrarily exclude sequence internal states from the
> comparison?

Not sure what you mean? You mean just not dump them? I guess we could by
editing the contents of a custom format dump? A bit annoying to have a
script doing that...

> How many similar issues have you seen?

That's usually the only difference.

> In the case where you have a promoted replica and put the same through
> workflow through both it and the master, I've seen "pg_dump -s" dump
> objects in different orders, for no apparent reason.  That is kind of
> annoying, but I never traced it back to the cause (nor have I excluded
> PEBCAK as the real cause).

I'm not surprised. Independent runs - which you seem to be describing -
are quite dependent on on-disk order, and effects of concurrency. Oids
get assigned in different orders and such.

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_upgrade and rsync

[Bruce Momjian]

On Wed, Mar  4, 2015 at 01:53:47PM +0300, Vladimir Borodin wrote:
> After running initdb to create the new master, but before running
> pg_upgrade, modify the new master's postgresql.conf and change wal_level
> = hot_standby.  (Don't modify pg_hba.conf at this stage.)
> 
> 
> 
> That does not help. The reason is that pg_upgrade sets 'Current wal_level
> setting: minimal' in control-file, and it does not depend on what is set in
> postgresql.conf before running pg_upgrade. Details could be seen here - 
> http://
> pastie.org/9998671.

Well, what is happening is that the pg_resetxlog commands we run inside
pg_upgrade set the pg_controldata file's wal_level to minimal, but as
you saw, starting the server sets the pg_controldata properly. 
pg_resetxlog is not changing the WAL files at all, just the control
file.

> The workaround for this is to start  and cleanly shut down postgres on master
> after running pg_upgrade but before running rsync. After that there would be a
> good control-file for streaming replication and rsync to replica can be done.

You are correct that a pg_controldata file is copied over that has
wal_level=minimal, but that should not be a problem.

> But it could not be done with --size-only key, because control-file is of 
> fixed
> size and rsync would skip it. Or may be everything should be copied with
> --size-only and control-file should be copied without this option.

Well, what happens is that there is no _new_ standby pg_controldata
file, so it is copied fully from the new master.  Are you running initdb
to create the new standby --- you shouldn't be doing that as the rsync
will do that for you.  Also, are you cleanly shutting down all the
servers, or using pg_ctl -m immediate?

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

  + Everyone has their own god. +


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

Re: [HACKERS] xpath changes in the recent back branches

[Marko Tiikkaja]

On 3/4/15 5:26 PM, Tom Lane wrote:

Robert Haas  writes:

On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja  wrote:

I'm not sure how changing behavior like this in a minor release was
considered acceptable.



I'm guessing that the fact that it changes behavior in cases like this
wasn't recognized, but I suppose Peter will have to be the one to
comment on that.


It was considered to be a bug fix; more, given the few complaints about
the clearly-broken old behavior, we thought it was a fix that would affect
few people, and them positively.


Yeah, but these things usually go the other way.  "This has been broken 
for 10 years but nobody noticed, so we're not going to fix this" is the 
answer I'm more used to seeing.  And frankly, even though I remember 
getting the wrong end of that hose a few times, I prefer that to 
breaking apps in minor releases.



.m


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

Re: [HACKERS] xpath changes in the recent back branches

[Marko Tiikkaja]

On 3/4/15 6:19 PM, I wrote:

On 3/4/15 5:26 PM, Tom Lane wrote:

It was considered to be a bug fix; more, given the few complaints about
the clearly-broken old behavior, we thought it was a fix that would affect
few people, and them positively.


Yeah, but these things usually go the other way.  "This has been broken
for 10 years but nobody noticed, so we're not going to fix this"


Sorry, that should have said "we're not going to fix this *in the back 
branches*".



.m


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

Re: [HACKERS] MD5 authentication needs help

[Bruce Momjian]

On Wed, Mar  4, 2015 at 10:52:30AM -0500, Stephen Frost wrote:
> The first is a "don't break anything" approach which would move the
> needle between "network data sensitivity" and "on-disk data sensitivity"
> a bit back in the direction of making the network data more sensitive.
> 
> this approach looks like this: pre-determine and store the values (on a
> per-user basis, so a new field in pg_authid or some hack on the existing
> field) which will be sent to the client in the AuthenticationMD5Password
> message.  Further, calculate a random salt to be used when storing data
> in pg_authid.  Then, for however many variations we feel are necessary,
> calculate and store, for each AuthenticationMD5Password value:
> 
> md5_challenge, hash(salt || response)
> 
> We wouldn't store 4 billion of these, of course, which means that the
> challenge / response system becomes less effective on a per-user basis.
> We could, however, store X number of these and provide a lock-out
> mechanism (something users have asked after for a long time..) which
> would make it likely that the account would be locked before the
> attacker was able to gain access.  Further, an attacker with access to
> the backend still wouldn't see the user's cleartext password, nor would
> we store the cleartext password or a token in pg_authid which could be
> directly used for authentication, and we don't break the wireline
> protocol or existing installations (since we could detect that the
> pg_authid entry has the old-style and simply 'upgrade' it).

What does storing multiple hash(password || stoarage_salt) values do for
us that session_salt doesn't already do?

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

  + Everyone has their own god. +


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

Re: [HACKERS] MD5 authentication needs help

[Bruce Momjian]

On Wed, Mar  4, 2015 at 11:36:23AM -0500, Stephen Frost wrote:
> * Andres Freund (and...@2ndquadrant.com) wrote:
> > On 2015-03-04 11:06:33 -0500, Stephen Frost wrote:
> > > * Andres Freund (and...@2ndquadrant.com) wrote:
> > > > On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> > > > > The first is a "don't break anything" approach which would move the
> > > > > needle between "network data sensitivity" and "on-disk data 
> > > > > sensitivity"
> > > > > a bit back in the direction of making the network data more sensitive.
> > > > 
> > > > I think that's a really bad tradeoff for pg. There's pretty good reasons
> > > > not to encrypt database connections. I don't think you really can
> > > > compare routinely encrypted stuff like imap and submission with
> > > > pg. Neither is it as harmful to end up with leaked hashes for database
> > > > users as it is for a email provider's authentication database.
> > > 
> > > I'm confused..  The paragraph you reply to here discusses an approach
> > > which doesn't include encrypting the database connection.
> > 
> > An increase in "network data sensitivity" also increases the need for
> > encryption.
> 
> Ok, I see what you're getting at there, though our existing md5
> implementation with no lock-out mechanism or ability to deal with
> hijacking isn't exactly making us all that safe when it comes to network
> based attacks.  The best part about md5 is that we don't send the user's
> password over the wire in the clear, the actual challenge/response piece
 - here is where I was lost
> is not considered terribly secure today, nor is the salt+password we use
> for pg_authid for that matter. :/

Can you please rephrase the last sentence as it doesn't make sense to
me?

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

  + Everyone has their own god. +


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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Mar  4, 2015 at 10:52:30AM -0500, Stephen Frost wrote:
> > The first is a "don't break anything" approach which would move the
> > needle between "network data sensitivity" and "on-disk data sensitivity"
> > a bit back in the direction of making the network data more sensitive.
> > 
> > this approach looks like this: pre-determine and store the values (on a
> > per-user basis, so a new field in pg_authid or some hack on the existing
> > field) which will be sent to the client in the AuthenticationMD5Password
> > message.  Further, calculate a random salt to be used when storing data
> > in pg_authid.  Then, for however many variations we feel are necessary,
> > calculate and store, for each AuthenticationMD5Password value:
> > 
> > md5_challenge, hash(salt || response)
> > 
> > We wouldn't store 4 billion of these, of course, which means that the
> > challenge / response system becomes less effective on a per-user basis.
> > We could, however, store X number of these and provide a lock-out
> > mechanism (something users have asked after for a long time..) which
> > would make it likely that the account would be locked before the
> > attacker was able to gain access.  Further, an attacker with access to
> > the backend still wouldn't see the user's cleartext password, nor would
> > we store the cleartext password or a token in pg_authid which could be
> > directly used for authentication, and we don't break the wireline
> > protocol or existing installations (since we could detect that the
> > pg_authid entry has the old-style and simply 'upgrade' it).
> 
> What does storing multiple hash(password || stoarage_salt) values do for
> us that session_salt doesn't already do?

By storing a hash of the result of the challenge/response, we wouldn't
be susceptible to attacks where the user has gained access to the
contents of pg_authid because the values there would not be (directly)
useful for authentication.  Today, an attacker can take what's in
pg_authid and directly use it to authenticate (which is what the
interwebs are complaining about).

We wouldn't want to do that for just a single value though because then
there wouldn't be any value to the challenge/response system (which is
intended to prevent replay attacks where the attacker has sniffed a
value from the network and then uses that value to authenticate
themselves).

The only way we can keep the session salt random without breaking the
wireline protocol is to keep the raw data necessary for authentication
in pg_authid (as we do now) since we'd need that information to recreate
the results of the random session salt+user-hash for comparison.

This is essentially a middle ground which maintains the existing
wireline protocol while changing what is in pg_authid to be something
that an attacker can't trivially use to authenticate.  It is not a
proper solution as that requires changing the wireline protocol (or,
really, extending it with another auth method that's better).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Mar  4, 2015 at 11:36:23AM -0500, Stephen Frost wrote:
> > * Andres Freund (and...@2ndquadrant.com) wrote:
> > > On 2015-03-04 11:06:33 -0500, Stephen Frost wrote:
> > > > * Andres Freund (and...@2ndquadrant.com) wrote:
> > > > > On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> > > > > > The first is a "don't break anything" approach which would move the
> > > > > > needle between "network data sensitivity" and "on-disk data 
> > > > > > sensitivity"
> > > > > > a bit back in the direction of making the network data more 
> > > > > > sensitive.
> > > > > 
> > > > > I think that's a really bad tradeoff for pg. There's pretty good 
> > > > > reasons
> > > > > not to encrypt database connections. I don't think you really can
> > > > > compare routinely encrypted stuff like imap and submission with
> > > > > pg. Neither is it as harmful to end up with leaked hashes for database
> > > > > users as it is for a email provider's authentication database.
> > > > 
> > > > I'm confused..  The paragraph you reply to here discusses an approach
> > > > which doesn't include encrypting the database connection.
> > > 
> > > An increase in "network data sensitivity" also increases the need for
> > > encryption.
> > 
> > Ok, I see what you're getting at there, though our existing md5
> > implementation with no lock-out mechanism or ability to deal with
> > hijacking isn't exactly making us all that safe when it comes to network
> > based attacks.  The best part about md5 is that we don't send the user's
> > password over the wire in the clear, the actual challenge/response piece
>  - here is where I was lost
> > is not considered terribly secure today, nor is the salt+password we use
> > for pg_authid for that matter. :/
> 
> Can you please rephrase the last sentence as it doesn't make sense to
> me?

The best part of the existing authentication method we call "md5" is
that the user's password is never sent over the network in the clear.

The challenge/response implementation we have only provides for 4 bytes
of hash (or around four billion possible permutations) which is not very
secure today (as compared to the 16-character base64 salt used in SCRAM,
which is 16^64 or 2^96 instead of 2^32).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Wed, Mar  4, 2015 at 11:36:23AM -0500, Stephen Frost wrote:
> > > * Andres Freund (and...@2ndquadrant.com) wrote:
> > > > On 2015-03-04 11:06:33 -0500, Stephen Frost wrote:
> > > > > * Andres Freund (and...@2ndquadrant.com) wrote:
> > > > > > On 2015-03-04 10:52:30 -0500, Stephen Frost wrote:
> > > > > > > The first is a "don't break anything" approach which would move 
> > > > > > > the
> > > > > > > needle between "network data sensitivity" and "on-disk data 
> > > > > > > sensitivity"
> > > > > > > a bit back in the direction of making the network data more 
> > > > > > > sensitive.
> > > > > > 
> > > > > > I think that's a really bad tradeoff for pg. There's pretty good 
> > > > > > reasons
> > > > > > not to encrypt database connections. I don't think you really can
> > > > > > compare routinely encrypted stuff like imap and submission with
> > > > > > pg. Neither is it as harmful to end up with leaked hashes for 
> > > > > > database
> > > > > > users as it is for a email provider's authentication database.
> > > > > 
> > > > > I'm confused..  The paragraph you reply to here discusses an approach
> > > > > which doesn't include encrypting the database connection.
> > > > 
> > > > An increase in "network data sensitivity" also increases the need for
> > > > encryption.
> > > 
> > > Ok, I see what you're getting at there, though our existing md5
> > > implementation with no lock-out mechanism or ability to deal with
> > > hijacking isn't exactly making us all that safe when it comes to network
> > > based attacks.  The best part about md5 is that we don't send the user's
> > > password over the wire in the clear, the actual challenge/response piece
> >  - here is where I was lost
> > > is not considered terribly secure today, nor is the salt+password we use
> > > for pg_authid for that matter. :/
> > 
> > Can you please rephrase the last sentence as it doesn't make sense to
> > me?
> 
> The best part of the existing authentication method we call "md5" is
> that the user's password is never sent over the network in the clear.
> 
> The challenge/response implementation we have only provides for 4 bytes
> of hash (or around four billion possible permutations) which is not very
> secure today (as compared to the 16-character base64 salt used in SCRAM,
> which is 16^64 or 2^96 instead of 2^32).

Err, 64^16 or 2^96, that is.  16^64 is not the same and would be way
larger. :)

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Bruce Momjian]

On Wed, Mar  4, 2015 at 12:43:54PM -0500, Stephen Frost wrote:
> > What does storing multiple hash(password || stoarage_salt) values do for
> > us that session_salt doesn't already do?
> 
> By storing a hash of the result of the challenge/response, we wouldn't
> be susceptible to attacks where the user has gained access to the
> contents of pg_authid because the values there would not be (directly)
> useful for authentication.  Today, an attacker can take what's in
> pg_authid and directly use it to authenticate (which is what the
> interwebs are complaining about).
> 
> We wouldn't want to do that for just a single value though because then
> there wouldn't be any value to the challenge/response system (which is
> intended to prevent replay attacks where the attacker has sniffed a
> value from the network and then uses that value to authenticate
> themselves).

So you are storing the password + storage-salt + session-saltX, where X
is greater than the maximum number of login attempts?  How do you know
the attacker will not be given a salt that was already seen before?

> The only way we can keep the session salt random without breaking the
> wireline protocol is to keep the raw data necessary for authentication
> in pg_authid (as we do now) since we'd need that information to recreate
> the results of the random session salt+user-hash for comparison.
> 
> This is essentially a middle ground which maintains the existing
> wireline protocol while changing what is in pg_authid to be something
> that an attacker can't trivially use to authenticate.  It is not a

I don't understand how this works.

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

  + Everyone has their own god. +


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

Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB

[Greg Stark]

On Tue, Mar 3, 2015 at 7:57 PM, Tom Lane  wrote:
> Now, if we were to change the server so that it *refused* settings that
> didn't have a unit, that argument would become moot.  But I'm not going
> to defend the castle against the villagers who will show up if you do
> that.

That might be something we could get away with eventually if we put in
a warning like this for a few years:

WARNING: Using default units of seconds for GUC tcp_keepalives

Though we'll have to deal with the "0 means system default" and "-1
means really really disabled" stuff.

-- 
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] Normalizing units for postgresql.conf WAS: Patch: raise default for max_wal_segments to 1GB

[Robert Haas]

On Tue, Mar 3, 2015 at 1:42 PM, Josh Berkus  wrote:
>>> Sure.  Although, do we take (s) for tcp_keepalives_idle?  Or only an INT?
>>
>> It's a "time unit", so you can say "10s" or "1ms". If you don't
>> specify a unit, it implies seconds.
>
> So if we're going to make this consistent, let's make it consistent.
>
> 1. All GUCs which accept time/size units will have them on the default
> setting.

+1.

> 2. Time/size comments will be removed, *except* from GUCs which do not
> accept (ms/s/min) or (kB/MB/GB).

+1.

> Argument Against: will create unnecessary diff changes between 9.4's
> pg.conf and 9.5's pg.conf.

I don't care about that.  I don't like to change postgresql.conf in
minor releases unless we have important reasons for doing so, but
changing it in major releases seems fine.

I do like Greg Stark's suggestion of also warning about unitless settings.

-- 
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] MD5 authentication needs help

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Mar  4, 2015 at 12:43:54PM -0500, Stephen Frost wrote:
> > > What does storing multiple hash(password || stoarage_salt) values do for
> > > us that session_salt doesn't already do?
> > 
> > By storing a hash of the result of the challenge/response, we wouldn't
> > be susceptible to attacks where the user has gained access to the
> > contents of pg_authid because the values there would not be (directly)
> > useful for authentication.  Today, an attacker can take what's in
> > pg_authid and directly use it to authenticate (which is what the
> > interwebs are complaining about).
> > 
> > We wouldn't want to do that for just a single value though because then
> > there wouldn't be any value to the challenge/response system (which is
> > intended to prevent replay attacks where the attacker has sniffed a
> > value from the network and then uses that value to authenticate
> > themselves).
> 
> So you are storing the password + storage-salt + session-saltX, where X
> is greater than the maximum number of login attempts?  How do you know
> the attacker will not be given a salt that was already seen before?

The password isn't stored and I don't know what you're talking about
regarding the number of login attempts.  Further, we don't know today if
an attacker has seen a particular challege/response sequence or not (nor
can we...) and we can certainly repeat it.

> > The only way we can keep the session salt random without breaking the
> > wireline protocol is to keep the raw data necessary for authentication
> > in pg_authid (as we do now) since we'd need that information to recreate
> > the results of the random session salt+user-hash for comparison.
> > 
> > This is essentially a middle ground which maintains the existing
> > wireline protocol while changing what is in pg_authid to be something
> > that an attacker can't trivially use to authenticate.  It is not a
> 
> I don't understand how this works.

Ok, let me try to explain it another way.

The current system looks like this:

client has username and password
server has hash(username + password)

client:

  send auth request w/ username and database

server:

  send random salt to client

client:

  send hash(hash(username + password) + salt) to server

server:

  calculate hash(hash(username + password) + salt)
  compare to what client sent

What the interwebs are complaining about is that the 
"hash(username + password)" piece that's stored in pg_authid is
sufficient to authenticate.

Here's what was proposed as an alternative which would prevent that
without breaking the existing wireline protocol:

client has username and password
server has user_salt,
   N *
 {salt, hash(hash(hash(username + password) + salt), user_salt)}

client:

  send auth request w/ username and database

server:

  picks random salt from the salts available for this user
  sends salt to the user

client:

  send hash(hash(username + password) + salt) to server

server:

  server calculates, using the data from the client,
hash(FROM_CLIENT + user_salt)
  compares to hash stored for salt chosen

This makes what is stored in pg_authid no longer directly useful for
authentication (similar to how Unix passwd-based authentication works)
because if the client sent any of those values, we'd add the user_salt
and hash it again and it wouldn't match what's stored.

This further makes what is sent over the network not directly
susceptible to a replay attack because the server has multiple values
available to pick for the salt to use and sends one at random to the
client, exactly how our current challenge/response replay-prevention
system works.  The downside is that the number of possible values for
the server to send to prevent replay attacke is reduced from 2^32 to N.

To mitigate the replay risk we would, ideally, support a lock-out
mechanism.  This won't help if the attacker has extended network access
though as we would almost certainly eventually go through all N
permutations for this user.

However, use of TLS would prevent the network-based attack vector.

Using TLS + the 'password' authentication mechanism would also achieve
the goal of making what is in pg_authid not directly useful for
authentication, but that would mean that the postmaster would see the
user's password (post-decryption), leading to a risk that the password
could be reused for access to other systems by a rogue server
administrator.  Now, that's what SSH and *most* systems have had for
ages when it comes to password-based authentication and therefore it's
well understood in the industry and session-level encryption is
generally considered to avoid the network-based vector associated with
that approach.

For PG users, however, we encourage using md5 instead of password as we
consider that "better", but users familiar with SSH and other
unix-password based authentication mechanisms might, understandably,
think that MD5+TLS prevents both attack 

Re: [HACKERS] MD5 authentication needs help

[Stefan Kaltenbrunner]

On 03/04/2015 04:52 PM, Stephen Frost wrote:
> Bruce, all,
> 
> I've been discussing this with a few folks outside of the PG community
> (Debian and Openwall people specifically) and a few interesting ideas
> have come out of that which might be useful to discuss.
> 
> The first is a "don't break anything" approach which would move the
> needle between "network data sensitivity" and "on-disk data sensitivity"
> a bit back in the direction of making the network data more sensitive.
> 
> this approach looks like this: pre-determine and store the values (on a
> per-user basis, so a new field in pg_authid or some hack on the existing
> field) which will be sent to the client in the AuthenticationMD5Password
> message.  Further, calculate a random salt to be used when storing data
> in pg_authid.  Then, for however many variations we feel are necessary,
> calculate and store, for each AuthenticationMD5Password value:
> 
> md5_challenge, hash(salt || response)
> 
> We wouldn't store 4 billion of these, of course, which means that the
> challenge / response system becomes less effective on a per-user basis.
> We could, however, store X number of these and provide a lock-out
> mechanism (something users have asked after for a long time..) which
> would make it likely that the account would be locked before the
> attacker was able to gain access.  Further, an attacker with access to
> the backend still wouldn't see the user's cleartext password, nor would
> we store the cleartext password or a token in pg_authid which could be
> directly used for authentication, and we don't break the wireline
> protocol or existing installations (since we could detect that the
> pg_authid entry has the old-style and simply 'upgrade' it).
> 
> That's probably the extent of what we could do to improve the current
> 'md5' approach without breaking the wireline protocol or existing stored
> data.
> 
> A lot of discussion has been going on with SCRAM and SASL, which is all
> great, but that means we end up with a dependency on SASL or we have to
> reimplement SCRAM (which I've been thinking might not be a bad idea-
> it's actually not that hard), but another suggestion was made which may
> be worthwhile to consider- OpenSSL and GnuTLS both support TLS-SRP, the
> RFC for which is here: http://www.ietf.org/rfc/rfc5054.txt.  We already
> have OpenSSL and therefore this wouldn't create any new dependencies and
> might be slightly simpler to implement.

not sure we should depend on TLS-SRP - the libressl people removed the
support for SRP pretty early in the development process:
https://github.com/libressl/libressl/commit/f089354ca79035afce9ec649f54c18711a950ecd



Stefan


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

Re: [HACKERS] MD5 authentication needs help

[Heikki Linnakangas]

On 03/04/2015 06:11 PM, Stephen Frost wrote:

* Magnus Hagander (mag...@hagander.net) wrote:

On Wed, Mar 4, 2015 at 5:03 PM, Stephen Frost  wrote:

No, I'm not suggesting that OpenSSL or TLS become mandatory but was
thinking it might be good alternative as a middle-ground between full
client-and-server side certificates and straight password-based auth
(which is clearly why it was invented in the first place) and so, yes,
md5 would still have to be kept around, but we'd at least be able to
deprecate it and tell people "Use TLS-SRP if you really want to useou
passwords and care about network security".

SCRAM doesn't actually fix the issue with network connection hijacking
or eavesdropping, except to the extent that it protects the password
itself, and so we might want to recommend, for people who are worried
about network-based attacks, using TLS-SRP.


Assuming we do implement SCRAM, what does TLS-SRP give us that we wouldn't
get by just using SCRAM over a TLS connection?


Good question and I'll have to dig more into that.  SCRAM does appear to
support channel binding with TLS and therefore there might not be much
to be gained from having both.


The big difference between SRP and SCRAM is that if you eavesdrop the 
SCRAM handshake, you can use that information to launch a brute-force or 
dictionary attack. With SRP, you cannot do that. That makes it 
relatively safe to use weak passwords with SRP, which is not the case 
with SCRAM (nor MD5)


Let me list the possible attacks that we're trying to protect against:

A) Eve eavesdrops on the authentication exchange. Can she use the 
information gathered directly to authenticate to the server?


B) Can Eve use the information to launch a dictionary or brute force the 
password?


C) Can a malicious server impersonate the real server? (i.e. does the 
protocol not authenticate the server to the client)


D) If Eve obtains a copy pg_authid (e.g from a backup tape), can she use 
that information to authenticate directly? (Brute forcing the passwords 
is always possible in this case)


A)  B)  C)  D)
passwordYes Yes Yes No [1]
MD5 No  Yes Yes Yes
SCRAM   No  Yes No  No
SRP No  No  No  No

[1] assuming that pg_authid stored MD5 hashes, not plaintext passwords, 
which should be the case these days.


Note that this table does not consider how difficult a brute-force 
attack is in each case; MD5 is a lot cheaper to calculate than SCRAM or 
SRP hashes. And there are more things to consider like implementation 
effort, strength of the underlying hash and other algorithms etc.


Also, attacks A), B) and C) can be thwarted by using SSL, with the 
client configured to check the server certificate (sslmode=verify-full). 
So actually, password authentication with SSL is not a bad option at 
all; it's actually better than MD5 because it doesn't allow attack D).


- Heikki



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

Re: [HACKERS] failures with tuplesort and ordered set aggregates (due to 5cefbf5a6c44)

[Peter Geoghegan]

On Wed, Mar 4, 2015 at 8:26 AM, Robert Haas  wrote:
> I think we should commit my patch, and if a future patch needs
> sortKeys set in more places, it can make that change itself.  There's
> no reason why it's needed with the code as it is today, and no reason
> to let bits of future changes leak into a bug-fix patch.

It's not as if I feel strongly about it. It's trivial to change the
code to make the comment true, rather than change the comment to make
the code true, and we need to do so anyway. I defer to you.


-- 
Peter Geoghegan


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

Re: [HACKERS] Reduce pinning in btree indexes

[Kevin Grittner]

Kyotaro HORIGUCHI  wrote:

> The performance which your test shows looks great. The gain might
> comes from removing of buffer locking on _bt_next.

Yeah, I had been hoping that removing some buffer pins and locks
from the common path of scanning forward from one page to the next
might have some direct performance benefit; it's possible that this
will show up more dramatically at high concurrency levels.  The
main goal, of course, was to improve performance more indirectly,
by not having autovacuum worker processes blocked for extended
periods in some situations where that can now happen.

> In nbtutils.c, _bt_killitems:
>
> - This is not in charge of this patch, but the comment for
> _bt_killitems looks to have a trivial typo.

As you say, that problem was already there, but no reason not to
fix it in the patch as long as we're here.  Reworded.

> - The following comment looks somewhat wrong.
>
> > /* Since the else condition needs page, get it here, too. */
>
> "the else condition needs page" means "the page of the buffer
> is needed later"? But, I suppose the comment itself is not
> necessary.

I guess that comment isn't really worth the space it takes up; what
it says is pretty obvious from the code.  Removed.

> - If (BTScanPosIsPinned(so->currPos)).
>
> As I mention below for nbtsearch.c, the page aquired in the
> if-then block may be vacuumed so the LSN check done in the
> if-else block is need also in the if-then block. It will be
> accomplished only by changing the position of the end of the
> if-else block.

I'm not sure I agree on this.  If the page is pinned it should have
been pinned continuously since we initially read it, so the line
pointers we have could not have been removed by any vacuum process.
The tuples may have been pruned away in the heap, but that doesn't
matter.  Index entries may have been added and the index page may
have been split, but that was true before this patch and
_bt_killitems will deal with those things the same as it always
has.  I don't see any benefit to doing the LSN compare in this
case; if we've paid the costs of holding the pin through to this
point, we might as well flag any dead entries we can.

> - _bt_killitems is called without pin when rescanning from
> before, so I think the previous code has already considered the
> unpinned case ("if (ItemPointerEquals(&ituple..." does
> that). Vacuum rearranges line pointers after deleting LP_DEAD
> items so the previous check seems enough for the purpose. The
> previous code is more effeicient becuase the mathing items will
> be processed even after vacuum.

I'm not following you on this one; could you rephrase it?

> In nbtsearch.c
>
> - _bt_first(): It now releases the share lock on the page before
> the items to be killed is processed. This allows other backends
> to insert items into the page simultaneously. It seems to be
> dangerous alone, but _bt_killitems already considers the
> case. Anyway I think It'll be better to add a comment
> mentioning unlocking is not dangerous.

Well, _bt_killitems does acquire a shared (read) lock to flag index
tuples as LP_DEAD, and it has always been OK for the index page to
accept inserts and allow page splits before calling _bt_killitems.
I don't really see anything new with the patch in this regard.  I
do agree, though, that a lot of comments and at least two README
files refer to the pins blocking vacuum, and these must be found
and changed.

It doesn't seem worth posting to the list for the small changes
since the last version; I'll wait until I update the comments and
README files.  If you want review or test the latest, you can peek
at: https://github.com/kgrittn/postgres/tree/btnopin

--
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] MD5 authentication needs help

[Stephen Frost]

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> The big difference between SRP and SCRAM is that if you eavesdrop
> the SCRAM handshake, you can use that information to launch a
> brute-force or dictionary attack. With SRP, you cannot do that. That
> makes it relatively safe to use weak passwords with SRP, which is
> not the case with SCRAM (nor MD5)

Thanks for the info!

Looking around a bit, one issue with SRP (as pointed out by Simon
Josefsson, the author of the SCRAM implementation for GNU SASL) is that
the username is included in the verifier (similar to our implementation
today with MD5) meaning that the stored data on the server is no longer
valid if the username is changed.  Obviously, our users are used to
that, but it's still something to be considered.

One question though- isn't the iteration option to SCRAM intended to
address the dictionary/brute force risk?  SRP uses an exponentiation
instead of iterations but it's unclear to me if one is really strictly
better or worse than the other (nor have I found any discussion of that
comparison) for this vector.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Bruce Momjian]

On Wed, Mar  4, 2015 at 01:27:32PM -0500, Stephen Frost wrote:
> This further makes what is sent over the network not directly
> susceptible to a replay attack because the server has multiple values
> available to pick for the salt to use and sends one at random to the
> client, exactly how our current challenge/response replay-prevention
> system works.  The downside is that the number of possible values for
> the server to send to prevent replay attacke is reduced from 2^32 to N.

OK, I understand now --- by not using a random session salt, you can
store a post-hash of what you receive from the client, preventing the
pg_authid from being resent by a client.  Nice trick, though going from
2^32 to N randomness doesn't seem like a win.

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

  + Everyone has their own god. +


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

Re: [HACKERS] MD5 authentication needs help

[Heikki Linnakangas]

On 03/04/2015 08:59 PM, Stephen Frost wrote:

* Heikki Linnakangas (hlinn...@iki.fi) wrote:

The big difference between SRP and SCRAM is that if you eavesdrop
the SCRAM handshake, you can use that information to launch a
brute-force or dictionary attack. With SRP, you cannot do that. That
makes it relatively safe to use weak passwords with SRP, which is
not the case with SCRAM (nor MD5)


Thanks for the info!

Looking around a bit, one issue with SRP (as pointed out by Simon
Josefsson, the author of the SCRAM implementation for GNU SASL) is that
the username is included in the verifier (similar to our implementation
today with MD5) meaning that the stored data on the server is no longer
valid if the username is changed.  Obviously, our users are used to
that, but it's still something to be considered.


Yes, good point, that's yet another thing that needs to be considered.


One question though- isn't the iteration option to SCRAM intended to
address the dictionary/brute force risk?  SRP uses an exponentiation
instead of iterations but it's unclear to me if one is really strictly
better or worse than the other (nor have I found any discussion of that
comparison) for this vector.


Not sure what you mean. Yes, the iterations option in SCRAM is designed 
to make brute forcing more expensive. For both a captured authentication 
handshake, or if you steal a backup tape.


I'm not sure how expensive a brute force attack on SRP would be, using a 
stolen backup tape. There doesn't seem to be an iterations count similar 
to SCRAM. But note that SRP's resistance to brute-forcing the 
authentication handshake is of a different kind. It's not just 
expensive, but outright impossible. (Don't ask me how that works; I'm 
not well-versed in the maths involved.) That's a big advantage because 
it means that it's OK to use a fairly weak password like 'foobar123' 
that would be trivially cracked with a dictionary attack. (You can still 
connect to the server and try different passwords, but the server can 
log that and throttle how many guesses / minute it let's you do)


- Heikki



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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> I'm not sure how expensive a brute force attack on SRP would be,
> using a stolen backup tape. There doesn't seem to be an iterations
> count similar to SCRAM. But note that SRP's resistance to
> brute-forcing the authentication handshake is of a different kind.
> It's not just expensive, but outright impossible. (Don't ask me how
> that works; I'm not well-versed in the maths involved.) That's a big
> advantage because it means that it's OK to use a fairly weak
> password like 'foobar123' that would be trivially cracked with a
> dictionary attack.

If it's actually impossible then that's certainly interesting..  I don't
get how that's possible, but ok.

> (You can still connect to the server and try
> different passwords, but the server can log that and throttle how
> many guesses / minute it let's you do)

Wouldn't that be nice...  Wish we did it. :(

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Mar  4, 2015 at 01:27:32PM -0500, Stephen Frost wrote:
> > This further makes what is sent over the network not directly
> > susceptible to a replay attack because the server has multiple values
> > available to pick for the salt to use and sends one at random to the
> > client, exactly how our current challenge/response replay-prevention
> > system works.  The downside is that the number of possible values for
> > the server to send to prevent replay attacke is reduced from 2^32 to N.
> 
> OK, I understand now --- by not using a random session salt, you can
> store a post-hash of what you receive from the client, preventing the
> pg_authid from being resent by a client.  Nice trick, though going from
> 2^32 to N randomness doesn't seem like a win.

You're only looking at it from the network attack vector angle where
clearly that's a reduction in strength.  That is not the only angle and
in many environments the network attack vector is already addressed with
TLS.

From the perspective of what everyone is currently complaining about on
the web, which is the pg_authid compromise vector, it'd be a huge
improvement over the current situation and we wouldn't be breaking any
existing clients, nor does it require having the postmaster see the
user's cleartext password during authentication (which is a common
argument against recommending the 'password' authentication method).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Bootstrap DATA is a pita

[Alvaro Herrera]

Andrew Dunstan wrote:
> 
> On 03/04/2015 09:51 AM, Robert Haas wrote:
> >On Wed, Mar 4, 2015 at 9:06 AM, Peter Eisentraut  wrote:
> >>>and make it harder to compare entries by grepping out some common
> >>>substring.
> >>Could you give an example of the sort of thing you wish to do?
> >e.g. grep for a function name and check that all the matches have the
> >same volatility.
> 
> I think grep will be the wrong tool for this format, but if we're settling
> on a perl format, a few perl one-liners should be able to work pretty well.
> It might be worth shipping a small perl module that provided some functions,
> or a script doing common tasks (or both).

I was going to say the same thing.  We need to make sure that the output
format of those oneliners is consistent, though -- it wouldn't be nice
if adding one column with nondefault value to a dozen of entries changes
the formatting of other entries.  For example, perhaps declare that the
order of entries is alphabetical or it matches something declared at the
start of the file.

>From that POV, I don't like the idea of having multiple columns for a
sigle entry in a single line; adding more columns means that eventually
we're going to split lines that have become too long in a different
place, which would reformat the whole file; not very nice.  But maybe
this doesn't matter if we decree that changing the column split is a
manual chore rather than automatic, because then it can be done in a
separate mechanical commit after the extra column is added.

BTW one solution to the merge problem is to have unique separators for
each entry.  For instance, instead of

}   -- this is the end of the previous entry
,
{ 
oid = 2233,
proname = array_append,

we could have
} # array_prepend 2232
,
} # array_append 2233
oid = 2233,
proname = array_append,

where the funcname-oid comment is there to avoid busted merges.  The
automatic editing tools make sure that those markers are always present.

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


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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Mar  4, 2015 at 02:21:51PM -0500, Stephen Frost wrote:
> > * Bruce Momjian (br...@momjian.us) wrote:
> > > On Wed, Mar  4, 2015 at 01:27:32PM -0500, Stephen Frost wrote:
> > > > This further makes what is sent over the network not directly
> > > > susceptible to a replay attack because the server has multiple values
> > > > available to pick for the salt to use and sends one at random to the
> > > > client, exactly how our current challenge/response replay-prevention
> > > > system works.  The downside is that the number of possible values for
> > > > the server to send to prevent replay attacke is reduced from 2^32 to N.
> > > 
> > > OK, I understand now --- by not using a random session salt, you can
> > > store a post-hash of what you receive from the client, preventing the
> > > pg_authid from being resent by a client.  Nice trick, though going from
> > > 2^32 to N randomness doesn't seem like a win.
> > 
> > You're only looking at it from the network attack vector angle where
> > clearly that's a reduction in strength.  That is not the only angle and
> > in many environments the network attack vector is already addressed with
> > TLS.
> 
> Well, passwords are already addressed by certificate authentication, so
> what's your point?  I think we decided we wanted a way to do passwords
> without requiring network encryption.

It's completely unclear to me what you mean by "passwords are already
addressed by certificate authentication."  Password-based and
certificate-based authentication are two independent mechanisms
available today, and both can be used with TLS.  Certainly the more
common implementation that I've come across is password-based
authentication through the md5 mechanism with TLS.  There are few places
which actually use client-side certificate-based authentication but tons
which use TLS.

> > From the perspective of what everyone is currently complaining about on
> > the web, which is the pg_authid compromise vector, it'd be a huge
> > improvement over the current situation and we wouldn't be breaking any
> > existing clients, nor does it require having the postmaster see the
> > user's cleartext password during authentication (which is a common
> > argument against recommending the 'password' authentication method).
> 
> We are not designing only for what people are complaining about today.

I was simply trying to explain what the 'newly discovered' vector
being discussed today is.  I complained about our implementation here 10
years ago and it has come up before this recent wave of complaints since
then, though perhaps with a bit less publicity, but I suspect that's
just because PG is getting popular.

And, no, I'm not suggesting that we design differently because we're now
popular, but I do think we need to address this issue because it's very
clearly an obvious deficiency.  I trust you feel the same as you started
this thread.

I brought up this approach because it avoids breaking the wireline
protocol or forcing everyone to switch to a new authentication method.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Bruce Momjian]

On Wed, Mar  4, 2015 at 02:21:51PM -0500, Stephen Frost wrote:
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Wed, Mar  4, 2015 at 01:27:32PM -0500, Stephen Frost wrote:
> > > This further makes what is sent over the network not directly
> > > susceptible to a replay attack because the server has multiple values
> > > available to pick for the salt to use and sends one at random to the
> > > client, exactly how our current challenge/response replay-prevention
> > > system works.  The downside is that the number of possible values for
> > > the server to send to prevent replay attacke is reduced from 2^32 to N.
> > 
> > OK, I understand now --- by not using a random session salt, you can
> > store a post-hash of what you receive from the client, preventing the
> > pg_authid from being resent by a client.  Nice trick, though going from
> > 2^32 to N randomness doesn't seem like a win.
> 
> You're only looking at it from the network attack vector angle where
> clearly that's a reduction in strength.  That is not the only angle and
> in many environments the network attack vector is already addressed with
> TLS.

Well, passwords are already addressed by certificate authentication, so
what's your point?  I think we decided we wanted a way to do passwords
without requiring network encryption.

> From the perspective of what everyone is currently complaining about on
> the web, which is the pg_authid compromise vector, it'd be a huge
> improvement over the current situation and we wouldn't be breaking any
> existing clients, nor does it require having the postmaster see the
> user's cleartext password during authentication (which is a common
> argument against recommending the 'password' authentication method).

We are not designing only for what people are complaining about today.

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

  + Everyone has their own god. +


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

Re: [HACKERS] Bootstrap DATA is a pita

[Peter Eisentraut]

On 3/4/15 9:51 AM, Robert Haas wrote:
> On Wed, Mar 4, 2015 at 9:06 AM, Peter Eisentraut  wrote:
>>> and make it harder to compare entries by grepping out some common
>>> substring.
>>
>> Could you give an example of the sort of thing you wish to do?
> 
> e.g. grep for a function name and check that all the matches have the
> same volatility.

You could still do that with grep -A or something like that.  I think
that it would be easier than now.



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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Magnus Hagander (mag...@hagander.net) wrote:
> On Wed, Mar 4, 2015 at 5:03 PM, Stephen Frost  wrote:
> > No, I'm not suggesting that OpenSSL or TLS become mandatory but was
> > thinking it might be good alternative as a middle-ground between full
> > client-and-server side certificates and straight password-based auth
> > (which is clearly why it was invented in the first place) and so, yes,
> > md5 would still have to be kept around, but we'd at least be able to
> > deprecate it and tell people "Use TLS-SRP if you really want to use
> > passwords and care about network security".
> >
> > SCRAM doesn't actually fix the issue with network connection hijacking
> > or eavesdropping, except to the extent that it protects the password
> > itself, and so we might want to recommend, for people who are worried
> > about network-based attacks, using TLS-SRP.
> 
> Assuming we do implement SCRAM, what does TLS-SRP give us that we wouldn't
> get by just using SCRAM over a TLS connection?

Good question and I'll have to dig more into that.  SCRAM does appear to
support channel binding with TLS and therefore there might not be much
to be gained from having both.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Bruce Momjian]

On Wed, Mar  4, 2015 at 02:46:54PM -0500, Stephen Frost wrote:
> > Well, passwords are already addressed by certificate authentication, so
> > what's your point?  I think we decided we wanted a way to do passwords
> > without requiring network encryption.
> 
> It's completely unclear to me what you mean by "passwords are already
> addressed by certificate authentication."  Password-based and
> certificate-based authentication are two independent mechanisms
> available today, and both can be used with TLS.  Certainly the more
> common implementation that I've come across is password-based
> authentication through the md5 mechanism with TLS.  There are few places
> which actually use client-side certificate-based authentication but tons
> which use TLS.

My point is we can't design a new authentication method to replace MD5
if it doesn't work well without TLS.

> > > From the perspective of what everyone is currently complaining about on
> > > the web, which is the pg_authid compromise vector, it'd be a huge
> > > improvement over the current situation and we wouldn't be breaking any
> > > existing clients, nor does it require having the postmaster see the
> > > user's cleartext password during authentication (which is a common
> > > argument against recommending the 'password' authentication method).
> > 
> > We are not designing only for what people are complaining about today.
> 
> I was simply trying to explain what the 'newly discovered' vector
> being discussed today is.  I complained about our implementation here 10
> years ago and it has come up before this recent wave of complaints since
> then, though perhaps with a bit less publicity, but I suspect that's
> just because PG is getting popular.
> 
> And, no, I'm not suggesting that we design differently because we're now
> popular, but I do think we need to address this issue because it's very
> clearly an obvious deficiency.  I trust you feel the same as you started
> this thread.
> 
> I brought up this approach because it avoids breaking the wireline
> protocol or forcing everyone to switch to a new authentication method.

Let me update my list of possible improvements:

1)  MD5 makes users feel uneasy (though our usage is mostly safe)

2)  The per-session salt sent to the client is only 32-bits, meaning
that it is possible to reply an observed MD5 hash in ~16k connection
attempts.

3)  Using the user name for the MD5 storage salt allows the MD5 stored
hash to be used on a different cluster if the user used the same
password.

4)  Using the user name for the MD5 storage salt allows the MD5 stored
hash to be used on the _same_ cluster.

5)  Using the user name for the MD5 storage salt causes the renaming of
a user to break the stored password.

I see your idea of hashing what the client sends to the server is to
currect #4?  And #2 becomes more of a problem?  And my idea of using a
random storage salt and longer session salt fix numbers 2 and 3, but not
4?

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

  + Everyone has their own god. +


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

Re: [HACKERS] MD5 authentication needs help

[Tom Lane]

Bruce Momjian  writes:
> Let me update my list of possible improvements:

> 1)  MD5 makes users feel uneasy (though our usage is mostly safe)

> 2)  The per-session salt sent to the client is only 32-bits, meaning
> that it is possible to reply an observed MD5 hash in ~16k connection
> attempts.

> 3)  Using the user name for the MD5 storage salt allows the MD5 stored
> hash to be used on a different cluster if the user used the same
> password.

> 4)  Using the user name for the MD5 storage salt allows the MD5 stored
> hash to be used on the _same_ cluster.

> 5)  Using the user name for the MD5 storage salt causes the renaming of
> a user to break the stored password.

What happened to "possession of the contents of pg_authid is sufficient
to log in"?  I thought fixing that was one of the objectives here.

regards, tom lane


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

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Mar  4, 2015 at 02:46:54PM -0500, Stephen Frost wrote:
> > > Well, passwords are already addressed by certificate authentication, so
> > > what's your point?  I think we decided we wanted a way to do passwords
> > > without requiring network encryption.
> > 
> > It's completely unclear to me what you mean by "passwords are already
> > addressed by certificate authentication."  Password-based and
> > certificate-based authentication are two independent mechanisms
> > available today, and both can be used with TLS.  Certainly the more
> > common implementation that I've come across is password-based
> > authentication through the md5 mechanism with TLS.  There are few places
> > which actually use client-side certificate-based authentication but tons
> > which use TLS.
> 
> My point is we can't design a new authentication method to replace MD5
> if it doesn't work well without TLS.

This isn't a discussion about designing a new authentication method to
replace MD5.  I'm not interested in doing that- we should be using one
which has already been designed by experts in that field (just like we'd
prefer they use PG instead of writing their own relational databases).

This is about trying to make the current situation around MD5 better
without breaking existing clients.  This proposal, in my view and at
least that of some others, does that.  Does it make the md5
authentication method secure?  No, but nothing is going to do that
except deprecating it entirely.

Further, to the extent that we can make *common use-cases* of md5-based
authentication better, that's a good thing.

> > I was simply trying to explain what the 'newly discovered' vector
> > being discussed today is.  I complained about our implementation here 10
> > years ago and it has come up before this recent wave of complaints since
> > then, though perhaps with a bit less publicity, but I suspect that's
> > just because PG is getting popular.
> > 
> > And, no, I'm not suggesting that we design differently because we're now
> > popular, but I do think we need to address this issue because it's very
> > clearly an obvious deficiency.  I trust you feel the same as you started
> > this thread.
> > 
> > I brought up this approach because it avoids breaking the wireline
> > protocol or forcing everyone to switch to a new authentication method.
> 
> Let me update my list of possible improvements:
> 
> 1)  MD5 makes users feel uneasy (though our usage is mostly safe)

Saying it's "mostly safe" is questionable, at best.

> 2)  The per-session salt sent to the client is only 32-bits, meaning
> that it is possible to reply an observed MD5 hash in ~16k connection
> attempts.

Yes, and we have no (PG-based) mechanism to prevent those connection
attempts, which is a pretty horrible situation to be in.

> 3)  Using the user name for the MD5 storage salt allows the MD5 stored
> hash to be used on a different cluster if the user used the same
> password.
> 
> 4)  Using the user name for the MD5 storage salt allows the MD5 stored
> hash to be used on the _same_ cluster.
> 
> 5)  Using the user name for the MD5 storage salt causes the renaming of
> a user to break the stored password.
> 
> I see your idea of hashing what the client sends to the server is to
> currect #4?  And #2 becomes more of a problem?  And my idea of using a
> random storage salt and longer session salt fix numbers 2 and 3, but not
> 4?

The proposal I've been discussing for improving md5 while not breaking
the existing wireline protocol would address 3 and 4 in your list while
making #2 require fewer connection attempts by an attacker (but then
again, with either approach, it's not like it takes a long time on
today's systems to reach the requisite number of attempts to get a
successful login).

I've not seen a detailed explanation of what your proposal is and so I'm
not sure that I can really comment on it since I don't really know what
it is.  The comment about "using a random storage salt" doesn't make any
sense since we can't have both a session salt *and* a storage salt, and
the comment about a "longer session salt" implies a break in the
wireline protocol, which goes against the premise of this discussion.

If we're going to break the wireline protocol then we need to go to
SCRAM or some existing, well-defined authentication system and not
something home-grown.

I'd suggest reviewing Heikki's list of attack vectors and the matrix he
built.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Bruce Momjian  writes:
> > Let me update my list of possible improvements:
> 
> > 1)  MD5 makes users feel uneasy (though our usage is mostly safe)
> 
> > 2)  The per-session salt sent to the client is only 32-bits, meaning
> > that it is possible to reply an observed MD5 hash in ~16k connection
> > attempts.
> 
> > 3)  Using the user name for the MD5 storage salt allows the MD5 stored
> > hash to be used on a different cluster if the user used the same
> > password.
> 
> > 4)  Using the user name for the MD5 storage salt allows the MD5 stored
> > hash to be used on the _same_ cluster.
> 
> > 5)  Using the user name for the MD5 storage salt causes the renaming of
> > a user to break the stored password.
> 
> What happened to "possession of the contents of pg_authid is sufficient
> to log in"?  I thought fixing that was one of the objectives here.

Yes, it certainly was.  I think Bruce was thinking that we could simply
hash what goes on to disk with an additional salt that's stored, but
that wouldn't actually work without requiring a change to the wireline
protocol, which is the basis of this entire line of discussion, in my
view.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MD5 authentication needs help

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> What happened to "possession of the contents of pg_authid is sufficient
>> to log in"?  I thought fixing that was one of the objectives here.

> Yes, it certainly was.  I think Bruce was thinking that we could simply
> hash what goes on to disk with an additional salt that's stored, but
> that wouldn't actually work without requiring a change to the wireline
> protocol, which is the basis of this entire line of discussion, in my
> view.

Hm, well, "don't change the wireline protocol" could be another wanna-have
... but if we want to stop using MD5, that's not really a realistic goal
is it?

regards, tom lane


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

Re: [HACKERS] xpath changes in the recent back branches

[Peter Eisentraut]

On 3/4/15 12:20 PM, Marko Tiikkaja wrote:
> On 3/4/15 6:19 PM, I wrote:
>> On 3/4/15 5:26 PM, Tom Lane wrote:
>>> It was considered to be a bug fix; more, given the few complaints about
>>> the clearly-broken old behavior, we thought it was a fix that would
>>> affect
>>> few people, and them positively.
>>
>> Yeah, but these things usually go the other way.  "This has been broken
>> for 10 years but nobody noticed, so we're not going to fix this"
> 
> Sorry, that should have said "we're not going to fix this *in the back
> branches*".

I tend to agree.  I was not in favor of backpatching, but other people
were in favor of it, and no one spoke up against it.

That said, if I understand your test case correctly, this would
basically be an argument against any semantic corrections in stable
releases, since user code could expect to continue to work with the
previous incorrect value.

You can always test the server version number, and you'll have to for
the next major release anyway.



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

Re: [HACKERS] improve pgbench syntax error messages

[Fabien COELHO]

Indeed. Here is a v2.


Here is a v3, which (1) activates better error messages from bison
and (2) improves the error reporting from the scanner as well.


 sh> ./pgbench -f bad.sql
 bad.sql:3: syntax error at column 23 in command "set"
 \set aid (1021 * :id) %
   ^ error found here


the improved message is:
  bad.sql:3: syntax error, unexpected $end at column 23 in command "set"
  \set aid (1021 * :id) %
^ error found here

Also scanner errors are better:

  sh> ./pgbench -f bad5.sql
  bad5.sql:1: unexpected character (a) at column 12 in command "set"
  \set foo 12abc
 ^ error found here

--
Fabien.diff --git a/contrib/pgbench/exprparse.y b/contrib/pgbench/exprparse.y
index 243c6b9..0405a50 100644
--- a/contrib/pgbench/exprparse.y
+++ b/contrib/pgbench/exprparse.y
@@ -26,6 +26,10 @@ static PgBenchExpr *make_op(char operator, PgBenchExpr *lexpr,
 %expect 0
 %name-prefix="expr_yy"
 
+/* better error reporting with bison */
+%define parse.lac full
+%define parse.error verbose
+
 %union
 {
 	int64		ival;
diff --git a/contrib/pgbench/exprscan.l b/contrib/pgbench/exprscan.l
index 4c9229c..9de7dab 100644
--- a/contrib/pgbench/exprscan.l
+++ b/contrib/pgbench/exprscan.l
@@ -18,6 +18,13 @@ static YY_BUFFER_STATE scanbufhandle;
 static char *scanbuf;
 static int	scanbuflen;
 
+/* context information for error reporting */
+static char *expr_source = NULL;
+static int expr_lineno = 0;
+static char *expr_full_line = NULL;
+static char *expr_command = NULL;
+static int expr_col = 0;
+
 /* flex 2.5.4 doesn't bother with a decl for this */
 int expr_yylex(void);
 
@@ -52,7 +59,9 @@ space			[ \t\r\f]
 
 .{
 	yycol += yyleng;
-	fprintf(stderr, "unexpected character '%s'\n", yytext);
+	syntax_error(expr_source, expr_lineno, expr_full_line, expr_command,
+ "unexpected character", yytext, expr_col + yycol);
+	/* dead code, exit is called from syntax_error */
 	return CHAR_ERROR;
 }
 %%
@@ -60,21 +69,27 @@ space			[ \t\r\f]
 void
 yyerror(const char *message)
 {
-	/* yyline is always 1 as pgbench calls the parser for each line...
-	 * so the interesting location information is the column number */
-	fprintf(stderr, "%s at column %d\n", message, yycol);
-	/* go on to raise the error from pgbench with more information */
-	/* exit(1); */
+	syntax_error(expr_source, expr_lineno, expr_full_line, expr_command,
+ message, NULL, expr_col + yycol);
 }
 
 /*
  * Called before any actual parsing is done
  */
 void
-expr_scanner_init(const char *str)
+expr_scanner_init(const char *str, const char *source,
+  const int lineno, const char *line,
+  const char *cmd, const int ecol)
 {
 	Size	slen = strlen(str);
 
+	/* save context informations for error messages */
+	expr_source = (char *) source;
+	expr_lineno = (int) lineno;
+	expr_full_line = (char *) line;
+	expr_command = (char *) cmd;
+	expr_col = (int) ecol;
+
 	/*
 	 * Might be left over after error
 	 */
@@ -102,4 +117,9 @@ expr_scanner_finish(void)
 {
 	yy_delete_buffer(scanbufhandle);
 	pg_free(scanbuf);
+	expr_source = NULL;
+	expr_lineno = 0;
+	expr_full_line = NULL;
+	expr_command = NULL;
+	expr_col = 0;
 }
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 706fdf5..3e50bae 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -291,6 +291,7 @@ typedef struct
 	int			type;			/* command type (SQL_COMMAND or META_COMMAND) */
 	int			argc;			/* number of command words */
 	char	   *argv[MAX_ARGS]; /* command word list */
+	int			cols[MAX_ARGS]; /* corresponding column starting from 1 */
 	PgBenchExpr *expr;			/* parsed expression */
 } Command;
 
@@ -2189,6 +2190,28 @@ parseQuery(Command *cmd, const char *raw_sql)
 	return true;
 }
 
+void syntax_error(const char *source, const int lineno,
+  const char *line, const char *command,
+  const char *msg, const char *more, const int column)
+{
+	fprintf(stderr, "%s:%d: %s", source, lineno, msg);
+	if (more)
+		fprintf(stderr, " (%s)", more);
+	if (column != -1)
+		fprintf(stderr, " at column %d", column);
+	fprintf(stderr, " in command \"%s\"\n", command);
+	if (line) {
+		fprintf(stderr, "%s\n", line);
+		if (column != -1) {
+			int i = 0;
+			for (i = 0; i < column - 1; i++)
+fprintf(stderr, " ");
+			fprintf(stderr, "^ error found here\n");
+		}
+	}
+	exit(1);
+}
+
 /* Parse a command; return a Command struct, or NULL if it's a comment */
 static Command *
 process_commands(char *buf, const char *source, const int lineno)
@@ -2200,6 +2223,11 @@ process_commands(char *buf, const char *source, const int lineno)
 	char	   *p,
 			   *tok;
 
+/* error message generation helper */
+#define SYNTAX_ERROR(msg, more, col)	\
+	syntax_error(source, lineno, my_commands->line,		\
+ my_commands->argv[0], msg, more, col)
+
 	/* Make the string buf end at the next newline */
 	if ((p = strchr(buf, '\n')) != NULL)
 		*p = '\0';
@@ -2228,11 +2256,13 @@ process_commands(char *buf, co

Re: [HACKERS] MD5 authentication needs help

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> What happened to "possession of the contents of pg_authid is sufficient
> >> to log in"?  I thought fixing that was one of the objectives here.
> 
> > Yes, it certainly was.  I think Bruce was thinking that we could simply
> > hash what goes on to disk with an additional salt that's stored, but
> > that wouldn't actually work without requiring a change to the wireline
> > protocol, which is the basis of this entire line of discussion, in my
> > view.
> 
> Hm, well, "don't change the wireline protocol" could be another wanna-have
> ... but if we want to stop using MD5, that's not really a realistic goal
> is it?

I'm trying to address both sides of the issue- improve the current
situation without breaking existing clients AND provide a new auth
method and encourage everyone to move to it as soon as they're able.  We
can't simply deprecate md5 as that would break pg_upgrade for any users
who are currently using it.

This half of the discussion has been all about improving md5 without
breaking the wireline protocol or existing users.

The other half of the discussion is about implementing a new
password-based authentication based on one of the vetted authentication
protocols already in use today (SCRAM or SRP, for example).  Using those
new authentication protocols would include a move off of the MD5 hashing
function, of course.  This would also mean breaking the on-disk hash,
but that's necessary anyway because what we do there today isn't secure
either and no amount of futzing is going to change that.

I've got nearly zero interest in trying to go half-way on this by
designing something that we think is secure which has had no external
review or anyone else who uses it.  Further, going that route makes me
very nervous that we'd decide on certain compromises in order to make
things easier for users without actually realising the problems with
such an approach (eg: "well, if we use hack X we wouldn't have to
change what is stored on disk and therefore we wouldn't break
pg_upgrade..").  I'd *much* rather have a clean break and migration
path for users.  If we had a password rotation capability then this kind
of a transistion would be a lot easier on our users and I'd definitely
recommend that we add that with this new authentication mechanism, to
address this kind of issue in the future (not to mention that's often
asked for..).  Password complexity would be another thing we should
really add and is also often requested.

Frankly, in the end, I don't see us being able to produce something in
time for this release unless someone can be dedicated to the effort over
the next couple of months, and therefore I'd prefer to improve the
current issues with md5 without breaking the wireline protocol than
simply do nothing (again).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Bootstrap DATA is a pita

[Robert Haas]

On Wed, Mar 4, 2015 at 2:27 PM, Alvaro Herrera  wrote:
> Andrew Dunstan wrote:
>> On 03/04/2015 09:51 AM, Robert Haas wrote:
>> >On Wed, Mar 4, 2015 at 9:06 AM, Peter Eisentraut  wrote:
>> >>>and make it harder to compare entries by grepping out some common
>> >>>substring.
>> >>Could you give an example of the sort of thing you wish to do?
>> >e.g. grep for a function name and check that all the matches have the
>> >same volatility.
>>
>> I think grep will be the wrong tool for this format, but if we're settling
>> on a perl format, a few perl one-liners should be able to work pretty well.
>> It might be worth shipping a small perl module that provided some functions,
>> or a script doing common tasks (or both).
>
> I was going to say the same thing.  We need to make sure that the output
> format of those oneliners is consistent, though -- it wouldn't be nice
> if adding one column with nondefault value to a dozen of entries changes
> the formatting of other entries.  For example, perhaps declare that the
> order of entries is alphabetical or it matches something declared at the
> start of the file.
>
> From that POV, I don't like the idea of having multiple columns for a
> sigle entry in a single line; adding more columns means that eventually
> we're going to split lines that have become too long in a different
> place, which would reformat the whole file; not very nice.  But maybe
> this doesn't matter if we decree that changing the column split is a
> manual chore rather than automatic, because then it can be done in a
> separate mechanical commit after the extra column is added.
>
> BTW one solution to the merge problem is to have unique separators for
> each entry.  For instance, instead of
>
> }   -- this is the end of the previous entry
> ,
> {
> oid = 2233,
> proname = array_append,
>
> we could have
> } # array_prepend 2232
> ,
> } # array_append 2233
> oid = 2233,
> proname = array_append,
>
> where the funcname-oid comment is there to avoid busted merges.  The
> automatic editing tools make sure that those markers are always present.

Speaking from entirely too much experience, that's not nearly enough.
git only needs 3 lines of context to apply a hunk with no qualms at
all, and it'll shade that to just 1 or 2 with little fanfare.  If your
pg_proc entries are each 20 lines long, this sort of thing will
provide little protection.

-- 
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] Providing catalog view to pg_hba.conf file - Patch submission

[Peter Eisentraut]

On 3/3/15 7:17 PM, Jim Nasby wrote:
> I think we're screwed in that regard anyway, because of the special
> constructs. You'd need different logic to handle things like +role and
> sameuser. We might even end up painted in a corner where we can't change
> it in the future because it'll break everyone's scripts.

Yeah, I'm getting worried about this.  I think most people agree that
getting a peek at pg_hba.conf from within the server is useful, but
everyone seems to have quite different uses for it.  Greg wants to join
against other catalog tables, Jim wants to reassemble a valid and
accurate pg_hba.conf, Josh wants to write an editing tool.  Personally,
I'd like to see something as close to the actual file as possible.

If there were an obviously correct way to map the various special
constructs to the available SQL data types, then fine.  But if there
isn't, then we shouldn't give a false overinterpretation.  So I'd render
everything that's disputed as a plain text field.  (Not even an array of
text.)


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