date:20131129

Andrew Dunstan  writes:
> On 11/29/2013 06:43 PM, Tom Lane wrote:
>> I've committed this patch.  I added a make_native_path() call to fix the
>> slashes-versus-backslashes issue noted by Christian Ullrich, since that
>> was an easy one-line addition.

> I don't mind changing this, but IMNSHO it's not a bug. The program 
> that's reported to fail with the old use of mixed separators is the one 
> with the bug. But changing it costs us little.

Yeah, no doubt, but we're certainly swimming against the tide by not
following the platform convention.

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] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"

2013-11-29 Thread Andrew Dunstan



On 11/29/2013 06:43 PM, Tom Lane wrote:

Rajeev rastogi  writes:

OK. Then I am moving it to "ready for committer".

I've committed this patch.  I added a make_native_path() call to fix the
slashes-versus-backslashes issue noted by Christian Ullrich, since that
was an easy one-line addition.



I don't mind changing this, but IMNSHO it's not a bug. The program 
that's reported to fail with the old use of mixed separators is the one 
with the bug. But changing it costs us little.



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] fe-secure.c and SSL/TLS

On Fri, Nov 29, 2013 at 06:01:01PM -0500, Jeffrey Walton wrote:
> I know of no other ways to check the result of OpenSSL's chain
> validation. The open question (for me) is where are
> SSL_get_verify_result/X509_V_OK checked? Neither show up in the
> Postgres sources.

According to SSL_set_verify manpage, you are perhaps talking about
SSL_VERIFY_NONE case?  Which has suggestion that you should call
SSL_get_verify_result if you want to know if cert was valid.

But if SSL_VERIFY_PEER is used, this is not needed.

> > 3) libpq starts using TLSv1_2_method() by default.
> > 4) libpq will give switch to users to request TLSv1.2.
> This might have negative effects on non-TLSv1.2 clients. For example,
> an Android 2.3 device can only do TLSv1.0 (IIRC). I think there's a
> similar limitation on a lot of Windows XP clients (depending on the IE
> version and SChannel version). And OpenSSL-based clients prior to
> 1.0.0h (released 14 Mar 2012) will have trouble (if I am reading the
> change log correctly).

Note we are talking about client-side settings here.  So the negative
effect would be that clients with TLSv1.2+ libpq cannot connect to
old servers.

> I believe the "standard" way of achieving TLS1.0 and above is to use
> the SSLv23_client_method() and then remove the SSL protocols with
> SSL_OP_NO_SSLv2 and SSL_OP_NO_SSLv3. I have to use handwaiving around
> "standard" because I don't believe its documented anywhere (one of the
> devs told me its the standard way to do it.).

Indeed - Python ssl module seems to achieve TLSv1.1 and it uses
SSLv23_method().  But still no TLSv1.2.

I'll play with it a bit to see whether it can have any negative effects.

-- 
marko

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

Re: [HACKERS] PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"

Rajeev rastogi  writes:
> OK. Then I am moving it to "ready for committer".

I've committed this patch.  I added a make_native_path() call to fix the
slashes-versus-backslashes issue noted by Christian Ullrich, since that
was an easy one-line addition.  I didn't do anything about the
relative-path-for-the-data-directory issue.  That would take a bit more
code and I'm not certain that we've fully analyzed the implications of
changing it.  In any case it seems like a completely separate issue from
getting the executable pathname right.

Thanks for all your work on this!  This code's been busted for a long
while ...

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] fe-secure.c and SSL/TLS

2013-11-29 Thread Jeffrey Walton

Hi Marko,

Forgive me for cherry picking two of these...

> I think Postgres uses SSL_VERIFY_PEER + SSL_set_verify() callback instead.
> At least for me, the psql -d "dbname=foo sslmode=verify-ca" fails
> when cert does not match.
I can't comment on the use of psql. My apologies for my ignorance.
However, here's what I see in fe-secure.c around line 695:

static int
verify_cb(int ok, X509_STORE_CTX *ctx)
{
return ok;
}

If `ok` is 0, then validation fails. To learn of the failure, a
program must call SSL_get_verify_result to fetch the error code. Error
codes are listed at https://www.openssl.org/docs/apps/verify.html.

If `ok` is always 1, then validation succeeds. To learn of the
success, a program must call SSL_get_verify_result and ensure
X509_V_OK is returned.

I know of no other ways to check the result of OpenSSL's chain
validation. The open question (for me) is where are
SSL_get_verify_result/X509_V_OK checked? Neither show up in the
Postgres sources.

> 1) OpenSSL guys change default back to TLSv1.0+.
> 2) OpenSSL guys give switch to make TLSv1_method() mean TLSv1.0+.
Well, I don't think that's going to happen, but I could be wrong :)

For what its worth, I agree with you. I want a TLSv1.0+ option and
even had this discussion with Tim Hudson offline.

> 3) libpq starts using TLSv1_2_method() by default.
> 4) libpq will give switch to users to request TLSv1.2.
This might have negative effects on non-TLSv1.2 clients. For example,
an Android 2.3 device can only do TLSv1.0 (IIRC). I think there's a
similar limitation on a lot of Windows XP clients (depending on the IE
version and SChannel version). And OpenSSL-based clients prior to
1.0.0h (released 14 Mar 2012) will have trouble (if I am reading the
change log correctly).

I believe the "standard" way of achieving TLS1.0 and above is to use
the SSLv23_client_method() and then remove the SSL protocols with
SSL_OP_NO_SSLv2 and SSL_OP_NO_SSLv3. I have to use handwaiving around
"standard" because I don't believe its documented anywhere (one of the
devs told me its the standard way to do it.).

Jeff

On Fri, Nov 29, 2013 at 3:19 PM, Marko Kreen  wrote:
> Reply to mails in pgsql-bugs:
>
>   
> http://www.postgresql.org/message-id/CAH8yC8mc_2J2UY0Q42WQdWFyaoqT3onG+83Fr=vn46j5+ml...@mail.gmail.com
>
> and
>
>   
> http://www.postgresql.org/message-id/CAH8yC8nZVUyCQznkQd8=ELMM4k_=uxjrjt8yf9v22cy2x_d...@mail.gmail.com
>
>
> * Default ciphersuite
>
>> I would argue nothing should be left to chance, and the project should
>> take control of everything. But I don't really have a dog in the fight ;)
>
> Indeed, on my own servers I've stopped bothering with pattern-based
> ciphersuite strings, now I am listing ciphers explicitly.
>
> But the discussion here is about default suite for admins who don't
> know or care about TLS.  Also, it would be good if it does not
> need to be tuned yearly to stay good.
>
>
> * SSL_get_verify_result
>
> I think Postgres uses SSL_VERIFY_PEER + SSL_set_verify() callback instead.
> At least for me, the psql -d "dbname=foo sslmode=verify-ca" fails
> when cert does not match.
>
>
> * SNI (Server Name Indication extension).
>
> It might be proper to configure it for connections, but it's unlikely
> to be useful as the many-domains-few-ips-one-port problem really does
> not apply to databases.  And from my experience, even the non-SNI
> hostname check is underused (or even - unusable) in many database
> setups.
>
>
> * TLSv1.2
>
> That's the remaining problem with Postgres SSL - new SHA2/AESGCM
> ciphersuites will not be used even when both client and server
> support them.  Also CBC-mode fixes in TLSv1.1 will be missed.
>
> It's a client-side OpenSSL problem and caused indeed
> by following line in fe-secure.c:
>
> SSL_context = SSL_CTX_new(TLSv1_method());
>
> It's an ugly problem, because TLSv1_method() actually *should* mean
> "TLSv1.0 and higher", but due to problems with various broken
> SSL implementations, it's disabled.
>
> I see various ways it can improve:
>
> 1) OpenSSL guys change default back to TLSv1.0+.
> 2) OpenSSL guys give switch to make TLSv1_method() mean TLSv1.0+.
> 3) libpq starts using TLSv1_2_method() by default.
> 4) libpq will give switch to users to request TLSv1.2.
>
> I see 1) and 3) as unlikely to happen.  As it's not an urgent problem,
> we could watch if 2) happens and go with 4) otherwise.
>
>
> I tried your suggested OP_ALL patch and it does not work.  And it's
> even harmful to use as it disables few security workarounds.
> It's mainly for servers for compat with legacy browsers.
>
> I also tried to clear OP_NO_TLSv1_x to see if there is some default
> OPs in TLSv1_method() that we could change, but that also did not work.
> My conclusion is that currently there is no switch to make TLSv1.0+
> work.  (OpenSSL v1.0.1 / 1.1.0-git).
>


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

Re: [HACKERS] MultiXact bugs

2013-11-29 Thread Kevin Grittner

Andres Freund  wrote:

> Looking at predicate.c I think I see a bigger problem though: Isn't its
> usage of HeapTupleSatisfiesVacuum() quite dangerous? It passes
> TransactionXmin to HeapTupleSatisfiesVacuum(). But since that's just the
> transaction's own cutoff, not the global cutoff that will cause wrong
> hint bits to be set. Or am I missing something?

I don't see where that parameter has anything to do with setting
hint bits; it only seems to affect the return code for the caller.

--
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] [RFC] overflow checks optimized away

Greg Stark  writes:
> Also, one of the places GCC warns about optimizing away an overflow
> check (with -fno-wrapv) is inside the localtime.c file from the tz
> library. I fixed it in my patch but in fact I checked and it's already
> fixed upstream so I'm wondering whether you expect to merge in an
> updated tz library? Is there anything surprising about the process or
> do you just copy in the files? Would you be happy for someone else to
> do it?

We've made a number of changes in our copies, unfortunately.  What you
have to do is look at the upstream diffs since we last synchronized
with them (which according to src/timezone/README was tzcode 2010c)
and merge those diffs as appropriate.  It should be reasonably
mechanical, but don't forget to update the README.

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] MultiXact bugs

On 2013-11-29 22:27:16 +0100, Andres Freund wrote:
> Looking at predicate.c I think I see a bigger problem though: Isn't its
> usage of HeapTupleSatisfiesVacuum() quite dangerous? It passes
> TransactionXmin to HeapTupleSatisfiesVacuum(). But since that's just the
> transaction's own cutoff, not the global cutoff that will cause wrong
> hint bits to be set. Or am I missing something?
> HTSV's comment says:
>  *
>  * OldestXmin is a cutoff XID (obtained from GetOldestXmin()).Tuples
>  * deleted by XIDs >= OldestXmin are deemed "recently dead"; they might
>  * still be visible to some open transaction, so we can't remove them,
>  * even if we see that the deleting transaction has committed.
>  */

Strike that, sorry for the noise. I was thinking there was some
conditional hint bit setting based on OldestXmin in there, but I was
misremembering.

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] MultiXact truncation, startup et al.

On 2013-11-29 13:26:00 -0800, Kevin Grittner wrote:
> Alvaro Herrera  wrote:
> 
> > New versions of all these patches, plus one more patch which
> > removes the behavior that HeapTupleGetUpdateXid checks for
> > aborted updates. (Turns out this was necessary to get freezing
> > right.)  My previous patch to avoid InvalidXid as page prune
> > point is reverted in there, too (no longer necessary.)
> 
> Does this mean that when HeapTupleSatisfiesVacuum() returns
> HEAPTUPLE_DELETE_IN_PROGRESS it is no longer possible for
> HeapTupleHeaderGetUpdateXid() to return InvalidTransactionId?

Correct.

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] MultiXact bugs

On 2013-11-29 13:14:06 -0800, Kevin Grittner wrote:
> Andres Freund  wrote:
> > On 2013-11-27 15:42:11 -0800, Kevin Grittner wrote:
> 
> >> What back-patching will be needed for a fix?  It sounds like
> >> 9.3?
> >
> > Yep.
> 
> In going over this, I found pre-existing bugs when a tuple was both
> inserted and deleted by concurrent transactions, but fixing that is
> too invasive to consider for Monday's minor release lockdown.  The
> attached seems very safe to me, and protects against some new
> hazards related to the subtransaction changes (mostly just for an
> assert-enabled build, but still worth fixing).  It includes a lot
> of work on the comments, to guide the subsequent fixes or other
> work in that area.

> If nobody objects, I will push it to master and 9.3 tomorrow.

Alvaro is about to commit a patch that will remove the behaviour of
GetUpdateXid() to check for IdDidAbort() because it makes other fixes
really complicated and it's a really suprising behaviour. But most of
your patch shouldn't be affected by that.
Check
http://archives.postgresql.org/message-id/20131129193008.GP5513%40eldon.alvh.no-ip.org
for the current state of the series.

Looking at predicate.c I think I see a bigger problem though: Isn't its
usage of HeapTupleSatisfiesVacuum() quite dangerous? It passes
TransactionXmin to HeapTupleSatisfiesVacuum(). But since that's just the
transaction's own cutoff, not the global cutoff that will cause wrong
hint bits to be set. Or am I missing something?
HTSV's comment says:
 *
 * OldestXmin is a cutoff XID (obtained from GetOldestXmin()).  Tuples
 * deleted by XIDs >= OldestXmin are deemed "recently dead"; they might
 * still be visible to some open transaction, so we can't remove them,
 * even if we see that the deleting transaction has committed.
 */

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] MultiXact truncation, startup et al.

2013-11-29 Thread Kevin Grittner

Alvaro Herrera  wrote:

> New versions of all these patches, plus one more patch which
> removes the behavior that HeapTupleGetUpdateXid checks for
> aborted updates. (Turns out this was necessary to get freezing
> right.)  My previous patch to avoid InvalidXid as page prune
> point is reverted in there, too (no longer necessary.)

Does this mean that when HeapTupleSatisfiesVacuum() returns
HEAPTUPLE_DELETE_IN_PROGRESS it is no longer possible for
HeapTupleHeaderGetUpdateXid() to return InvalidTransactionId?

--
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] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

On Fri, Nov 29, 2013 at 01:21:27PM -0800, Jeff Davis wrote:
> On Fri, 2013-11-29 at 16:18 -0500, Bruce Momjian wrote:
> > On Fri, Jul  5, 2013 at 02:36:10PM -0700, Jeff Davis wrote:
> > > On Fri, 2013-06-28 at 13:26 -0400, Robert Haas wrote:
> > > > I haven't really reviewed the windowing-related code in depth; I
> > > > thought Jeff might jump back in for that part of it.  Jeff, is that
> > > > something you're planning to do?
> > > 
> > > Yes, getting back into this patch now after a bit of delay.
> > 
> > Jeff, any status on this?
> 
> The last message was a review from Alvaro that hasn't been addressed
> yet.
> 
> Right now I am looking at the extension templates patch. But this patch
> is fairly close, so if Nicholas doesn't get to looking at it, I'll see
> what I can do.

Thank you.  I see it is looking very active on the commit-fest:  :-)

https://commitfest.postgresql.org/action/patch_view?id=1096

Thanks for all the work.

-- 
  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] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

2013-11-29 Thread Jeff Davis

On Fri, 2013-11-29 at 16:18 -0500, Bruce Momjian wrote:
> On Fri, Jul  5, 2013 at 02:36:10PM -0700, Jeff Davis wrote:
> > On Fri, 2013-06-28 at 13:26 -0400, Robert Haas wrote:
> > > I haven't really reviewed the windowing-related code in depth; I
> > > thought Jeff might jump back in for that part of it.  Jeff, is that
> > > something you're planning to do?
> > 
> > Yes, getting back into this patch now after a bit of delay.
> 
> Jeff, any status on this?

The last message was a review from Alvaro that hasn't been addressed
yet.

Right now I am looking at the extension templates patch. But this patch
is fairly close, so if Nicholas doesn't get to looking at it, I'll see
what I can do.

Regards,
Jeff Davis




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

Re: [HACKERS] Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

On Fri, Jul  5, 2013 at 02:36:10PM -0700, Jeff Davis wrote:
> On Fri, 2013-06-28 at 13:26 -0400, Robert Haas wrote:
> > I haven't really reviewed the windowing-related code in depth; I
> > thought Jeff might jump back in for that part of it.  Jeff, is that
> > something you're planning to do?
> 
> Yes, getting back into this patch now after a bit of delay.

Jeff, any status on this?

-- 
  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] pg_upgrade segfaults when given an invalid PGSERVICE value

On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote:
> So to summarise:
> 
> Plan A: The first patch I attached for pg_upgrade + documentation
> changes, and changing the other places that call PQconndefaults() to
> accept failures on either out of memory, or an invalid PGSERVICE
> 
> Plan B: Create a new function PQconndefaults2(char * errorBuffer) or
> something similar that returned error information to the caller.
> 
> Plan C: PQconndefaults() just ignores an invalid service but
> connection attempts fail because other callers of
> conninfo_add_defaults still pay attention to connection failures.
> This is the second patch I sent.
> 
> Plan D:  Service lookup failures are always ignored by
> conninfo_add_defaults. If you attempt to connect with a bad
> PGSERVICE set it will behave as if no PGSERVICE value was set.   I
> don't think anyone explicitly proposed this yet.
> 
> Plan 'D' is the only option that I'm opposed to, it will effect a
> lot more applications then ones that call PQconndefaults() and I
> feel it will confuse users.
> 
> I'm not convinced plan B is worth the effort of having to maintain
> two versions of PQconndefaults() for a while to fix a corner case.

OK, I am back to looking at this issue from March.  I went with option
C, patch attached, which seems to be the most popular.  It is similar to
Steve's patch, but structured differently and includes a doc patch.

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

  + Everyone has their own god. +
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index b75f553..7d2aa35
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** check_pghost_envvar(void)
*** 325,330 
--- 325,333 
  
  	start = PQconndefaults();
  
+ 	if (!start)
+ 		pg_fatal("out of memory\n");
+ 		
  	for (option = start; option->keyword != NULL; option++)
  	{
  		if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index 955f248..503a63a
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** typedef struct
*** 483,489 
 with an entry having a null keyword pointer.  The
 null pointer is returned if memory could not be allocated. Note that
 the current default values (val fields)
!will depend on environment variables and other context.  Callers
 must treat the connection options data as read-only.

  
--- 483,490 
 with an entry having a null keyword pointer.  The
 null pointer is returned if memory could not be allocated. Note that
 the current default values (val fields)
!will depend on environment variables and other context.  A
!missing or invalid service file will be silently ignored.  Callers
 must treat the connection options data as read-only.

  
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 975f795..9797140
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*** pg_fe_sendauth(AuthRequest areq, PGconn
*** 982,988 
   * if there is an error, return NULL with an error message in errorMessage
   */
  char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
  {
  	const char *name = NULL;
  	char	   *authn;
--- 982,988 
   * if there is an error, return NULL with an error message in errorMessage
   */
  char *
! pg_fe_getauthname(void)
  {
  	const char *name = NULL;
  	char	   *authn;
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
new file mode 100644
index bfddc68..25440b0
*** a/src/interfaces/libpq/fe-auth.h
--- b/src/interfaces/libpq/fe-auth.h
***
*** 19,24 
  
  
  extern int	pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
  
  #endif   /* FE_AUTH_H */
--- 19,24 
  
  
  extern int	pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(void);
  
  #endif   /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 8dd1a59..7ab4a9a
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** PQconndefaults(void)
*** 875,881 
  	connOptions = conninfo_init(&errorBuf);
  	if (connOptions != NULL)
  	{
! 		if (!conninfo_add_defaults(connOptions, &errorBuf))
  		{
  			PQconninfoFree(connOptions);
  			connOptions = NULL;
--- 875,882 
  	connOptions = conninfo_init(&errorBuf);
  	if (connOptions != NULL)
  	{
! 		/* pass NULL errorBuf to ignore errors */
! 		if (!conninfo_add_defaults(connOptions, NULL))
  		{
  			PQconninfoFree(connOptions);
  			connOptions = NULL;
*** conninfo_array_parse(const char *const *
*** 4412,4420 
   *
   * Default

Re: [HACKERS] MultiXact bugs

2013-11-29 Thread Kevin Grittner

Andres Freund  wrote:
> On 2013-11-27 15:42:11 -0800, Kevin Grittner wrote:

>> What back-patching will be needed for a fix?  It sounds like
>> 9.3?
>
> Yep.

In going over this, I found pre-existing bugs when a tuple was both
inserted and deleted by concurrent transactions, but fixing that is
too invasive to consider for Monday's minor release lockdown.  The
attached seems very safe to me, and protects against some new
hazards related to the subtransaction changes (mostly just for an
assert-enabled build, but still worth fixing).  It includes a lot
of work on the comments, to guide the subsequent fixes or other
work in that area.

If nobody objects, I will push it to master and 9.3 tomorrow.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
***
*** 3846,3855  XidIsConcurrent(TransactionId xid)
  
  /*
   * CheckForSerializableConflictOut
!  *		We are reading a tuple which has been modified.  If it is visible to
!  *		us but has been deleted, that indicates a rw-conflict out.	If it's
!  *		not visible and was created by a concurrent (overlapping)
!  *		serializable transaction, that is also a rw-conflict out,
   *
   * We will determine the top level xid of the writing transaction with which
   * we may be in conflict, and check for overlap with our own transaction.
--- 3846,3857 
  
  /*
   * CheckForSerializableConflictOut
!  *		We are reading a tuple which may have been modified since this
!  *		transaction started.  If it is visible to us but has been deleted,
!  *		that indicates a rw-conflict out.  If it's not visible and was created
!  *		by a concurrent (overlapping) serializable transaction, that is also a
!  *		rw-conflict out.  Visibility must be determined by the caller using
!  *		HeapTupleSatisfiesVisibility() before calling this function.
   *
   * We will determine the top level xid of the writing transaction with which
   * we may be in conflict, and check for overlap with our own transaction.
***
*** 3896,3918  CheckForSerializableConflictOut(bool visible, Relation relation,
--- 3898,3974 
  	switch (htsvResult)
  	{
  		case HEAPTUPLE_LIVE:
+ 
+ 			/*
+ 			 * The transaction which created the tuple has committed, and the
+ 			 * tuple has not been deleted.	If that insert committed in time
+ 			 * for us to see it, there is no conflict; otherwise we conflict
+ 			 * with the inserting transaction.
+ 			 */
  			if (visible)
  return;
  			xid = HeapTupleHeaderGetXmin(tuple->t_data);
  			break;
+ 
  		case HEAPTUPLE_RECENTLY_DEAD:
+ 
+ 			/*
+ 			 * The tuple has been deleted, but is still visible to some
+ 			 * transactions (possibly including our own).  If the delete
+ 			 * committed in time to make the tuple not-visible to us, we do
+ 			 * not conflict with that; otherwise we conflict with the deleting
+ 			 * transaction.
+ 			 *
+ 			 * TODO: Handle the case that the deleted tuple was both created
+ 			 * and deleted after our transaction started.
+ 			 */
  			if (!visible)
  return;
  			xid = HeapTupleHeaderGetUpdateXid(tuple->t_data);
  			break;
+ 
  		case HEAPTUPLE_DELETE_IN_PROGRESS:
+ 
+ 			/*
+ 			 * The tuple has been deleted, but that delete has not yet been
+ 			 * committed.  In fact, the delete may have occurred in a
+ 			 * subtransaction which has subsequently been rolled back, in
+ 			 * which case the deleting transaction ID will be invalid, and the
+ 			 * delete should be ignored.  If the delete committed in time to
+ 			 * make the tuple not-visible to us, we do not conflict with that;
+ 			 * otherwise we conflict with the deleting transaction.
+ 			 *
+ 			 * TODO: Handle the case that the deleted tuple was both created
+ 			 * and deleted after our transaction started.
+ 			 */
+ 			if (!visible)
+ return;
  			xid = HeapTupleHeaderGetUpdateXid(tuple->t_data);
+ 			if (!TransactionIdIsValid(xid))
+ return;
  			break;
+ 
  		case HEAPTUPLE_INSERT_IN_PROGRESS:
+ 
+ 			/*
+ 			 * The tuple has been inserted, but that insert has not yet been
+ 			 * committed.  Since the insert is not yet committed, it cannot be
+ 			 * visible to us unless we inserted it; otherwise we conflict with
+ 			 * the inserting transaction.
+ 			 */
+ 			if (visible)
+ return;
  			xid = HeapTupleHeaderGetXmin(tuple->t_data);
  			break;
+ 
  		case HEAPTUPLE_DEAD:
+ 
+ 			/*
+ 			 * The tuple was gone before our transaction started; there is no
+ 			 * conflict.
+ 			 */
  			return;
+ 
  		default:
  
  			/*
***
*** 3931,3942  CheckForSerializableConflictOut(bool visible, Relation relation,
  			xid = InvalidTransactionId;
  	}
  	Assert(TransactionIdIsValid(xid));
- 	Assert(TransactionIdFollowsOrEquals(xid, TransactionXmin));
  
  	/*
  	 * Find top level xid.	Bail out if xid is too early to be a conflict, or
  	 * if it's our own xid.
  	 */
  	if (TransactionIdEquals(xid, GetTopTrans

Re: [HACKERS] MultiXact truncation, startup et al.

On 2013-11-29 16:30:08 -0300, Alvaro Herrera wrote:
> As a second bug, heap_freeze_tuple() didn't properly handle multixacts
> that need to be frozen according to cutoff_multi, but whose updater xid
> is still alive.  Instead of preserving the update Xid, it just set Xmax
> invalid, which leads to both old and new tuple versions becoming
> visible.  This is pretty rare in practice, but a real threat
> nonetheless.  Existing corrupted rows, unfortunately, cannot be repaired
> in an automated fashion.

I think this bug is worth mentioning here explicitly. As released, if
you have a table where some rows are updated using an xmax as multi, and
you freeze it you're pretty likely to experience corruption where you
see both the old and the new version of a tuple as live. I haven't seen
this one in the wild but just noticed it while looking at the other
freezing bug, but it's really quite easy to reproduce. As demonstrated
in the attached isolationtester spec which doubles the row count via an
UPDATE in 9.3/HEAD.

> +  * Note the update Xid cannot possibly be older 
> than
> +  * cutoff_xid; if it were, we wouldn't be here: 
> if committed,
> +  * the tuple would have been pruned, and if 
> aborted, the Xmax
> +  * would have been marked Invalid by 
> HeapTupleSatisfiesVacuum.
> +  * (Not in-progress either, because then 
> cutoff_xid would be
> +  * newer.)

s/newer/older/?

> @@ -5644,24 +5729,54 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, 
> TransactionId cutoff_xid,
>   TransactionIdPrecedes(xid, cutoff_xid))
>   return true;

Maybe add a comment referring to heap_freeze_tuple() for justification
of individual parts and that it needs to be kept in sync?


> + nmembers = GetMultiXactIdMembers(xid, &members, true);
> + for (i = 0; i < nmembers; i++)
> + {
> + TransactionId member = members[i].xid;
> +
> + Assert(TransactionIdIsNormal(member));
> +
> + /* we don't care about lockers */
> + if (!ISUPDATE_from_mxstatus(members[i].status))
> + continue;
> +
> + if (TransactionIdPrecedes(member, cutoff_xid))
> + {
> + pfree(members);
> + return true;
> + }
> + }

This now can use GetUpdateXid() as well.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
setup
{
  DROP TABLE IF EXISTS vacme;
  CREATE TABLE vacme AS SELECT g.i FROM generate_series(1, 300) g(i);
  ALTER TABLE vacme ADD COLUMN id serial primary key;
  ALTER TABLE vacme SET (AUTOVACUUM_ENABLED = false);
}

teardown
{
/*DROP TABLE vacme;*/
}

session "s1"
step "s1b" {BEGIN; SELECT txid_current(); SELECT 1; }

session "s2"
step "s2b" {BEGIN; SELECT txid_current(); SELECT 1; }
step "s2share" {SELECT * FROM vacme FOR KEY SHARE;}
step "s2c" {COMMIT;}

session "s3"
step "s3b" {BEGIN; SELECT txid_current(); SELECT 1; }
step "s3update" {UPDATE vacme SET i = -1; }
step "s3c" {COMMIT;}

session "s4"
step "s4v" {VACUUM FREEZE vacme;}
step "s4s" {SELECT count(*) FROM vacme; SELECT 1;}

permutation "s1b" "s2b" "s3b" "s2share" "s3update" "s2c" "s3c" "s4v" "s4s"

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

Re: [HACKERS] [RFC] overflow checks optimized away


On 11/29/2013 10:06 PM, Greg Stark wrote:

On Fri, Nov 29, 2013 at 7:39 PM, Greg Stark  wrote:


Just as an update I did get gcc to do the wrong thing on purpose. The
only overflow check that the regression tests find missing is the one
for int8abs() ie:



Also, one of the places GCC warns about optimizing away an overflow
check (with -fno-wrapv) is inside the localtime.c file from the tz
library. I fixed it in my patch but in fact I checked and it's already
fixed upstream so I'm wondering whether you expect to merge in an
updated tz library? Is there anything surprising about the process or
do you just copy in the files? Would you be happy for someone else to
do it?


IIRC some files can be copied directly, but not all. Might be easiest to 
generate a diff between the last version in PostgreSQL and the latest 
upstream version, and apply that.


- Heikki


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

Re: [HACKERS] Time-Delayed Standbys

2013-11-29 Thread Fabrízio de Royes Mello

On Fri, Nov 29, 2013 at 5:49 AM, KONDO Mitsumasa <
kondo.mitsum...@lab.ntt.co.jp> wrote:
>
> Hi Royes,
>
> I'm sorry for my late review...
>

No problem...


> I feel potential of your patch in PG replication function, and it might
be something useful for all people. I check your patch and have some
comment for improvement. I haven't executed detail of unexpected sutuation
yet. But I think that under following problem2 is important functionality
problem. So I ask you to solve the problem in first.
>
> * Regress test
> No problem.
>
> * Problem1
> Your patch does not code recovery.conf.sample about recovery_time_delay.
> Please add it.
>

Fixed.


> * Problem2
> When I set time-delayed standby and start standby server, I cannot access
stanby server by psql. It is because PG is in first starting recovery which
cannot access by psql. I think that time-delayed standby is only delayed
recovery position, it must not affect other functionality.
>
> I didn't test recoevery in master server with recovery_time_delay. If you
have detail test result of these cases, please send me.
>

Well, I could not reproduce the problem that you described.

I run the following test:

1) Clusters
- build master
- build slave and attach to the master using SR and config
recovery_time_delay to 1min.

2) Stop de Slave

3) Run some transactions on the master using pgbench to generate a lot of
archives

4) Start the slave and connect to it using psql and in another session I
can see all archive recovery log


> My first easy review of your patch is that all.
>

Thanks.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml
index c0c543e..641c9c6 100644
--- a/doc/src/sgml/recovery-config.sgml
+++ b/doc/src/sgml/recovery-config.sgml
@@ -135,6 +135,27 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
   
  
 
+ 
+  recovery_time_delay (integer)
+  
+recovery_time_delay recovery parameter
+  
+  
+   
+Specifies the amount of time (in milliseconds, if no unit is specified)
+which recovery of transaction commits should lag the master.  This
+parameter allows creation of a time-delayed standby.  For example, if
+you set this parameter to 5min, the standby will
+replay each transaction commit only when the system time on the standby
+is at least five minutes past the commit time reported by the master.
+   
+   
+Note that if the master and standby system clocks are not synchronized,
+this might lead to unexpected results.
+   
+  
+ 
+
 
 
   
diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample
index 5acfa57..97cc7af 100644
--- a/src/backend/access/transam/recovery.conf.sample
+++ b/src/backend/access/transam/recovery.conf.sample
@@ -123,6 +123,17 @@
 #
 #trigger_file = ''
 #
+# recovery_time_delay
+#
+# By default, a standby server keeps restoring XLOG records from the
+# primary as soon as possible. If you want to delay the replay of
+# commited transactions from the master, specify a recovery time delay.
+# For example, if you set this parameter to 5min, the standby will replay
+# each transaction commit only whe the system time on the standby is least
+# five minutes past the commit time reported by the master.
+#
+#recovery_time_delay = 0
+#
 #---
 # HOT STANDBY PARAMETERS
 #---
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index de19d22..714b1bd 100755
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -218,6 +218,8 @@ static bool recoveryPauseAtTarget = true;
 static TransactionId recoveryTargetXid;
 static TimestampTz recoveryTargetTime;
 static char *recoveryTargetName;
+static int recovery_time_delay = 0;
+static TimestampTz recoveryDelayUntilTime;
 
 /* options taken from recovery.conf for XLOG streaming */
 static bool StandbyModeRequested = false;
@@ -730,6 +732,7 @@ static void readRecoveryCommandFile(void);
 static void exitArchiveRecovery(TimeLineID endTLI, XLogSegNo endLogSegNo);
 static bool recoveryStopsHere(XLogRecord *record, bool *includeThis);
 static void recoveryPausesHere(void);
+static void recoveryDelay(void);
 static void SetLatestXTime(TimestampTz xtime);
 static void SetCurrentChunkStartTime(TimestampTz xtime);
 static void CheckRequiredParameterValues(void);
@@ -5474,6 +5477,19 @@ readRecoveryCommandFile(void)
 	(errmsg_internal("trigger_file = '%s'",
 	 TriggerFile)));
 		}
+		else if (strcmp(item

Re: [HACKERS] fe-secure.c and SSL/TLS

Reply to mails in pgsql-bugs:

http://www.postgresql.org/message-id/CAH8yC8mc_2J2UY0Q42WQdWFyaoqT3onG+83Fr=vn46j5+ml...@mail.gmail.com

and

http://www.postgresql.org/message-id/CAH8yC8nZVUyCQznkQd8=ELMM4k_=uxjrjt8yf9v22cy2x_d...@mail.gmail.com

* Default ciphersuite

> I would argue nothing should be left to chance, and the project should
> take control of everything. But I don't really have a dog in the fight ;)

Indeed, on my own servers I've stopped bothering with pattern-based
ciphersuite strings, now I am listing ciphers explicitly.

But the discussion here is about default suite for admins who don't
know or care about TLS. Also, it would be good if it does not
need to be tuned yearly to stay good.

* SSL_get_verify_result

I think Postgres uses SSL_VERIFY_PEER + SSL_set_verify() callback instead.
At least for me, the psql -d "dbname=foo sslmode=verify-ca" fails
when cert does not match.

* SNI (Server Name Indication extension).

It might be proper to configure it for connections, but it's unlikely
to be useful as the many-domains-few-ips-one-port problem really does
not apply to databases. And from my experience, even the non-SNI
hostname check is underused (or even - unusable) in many database
setups.

* TLSv1.2

That's the remaining problem with Postgres SSL - new SHA2/AESGCM
ciphersuites will not be used even when both client and server
support them. Also CBC-mode fixes in TLSv1.1 will be missed.

It's a client-side OpenSSL problem and caused indeed
by following line in fe-secure.c:

SSL_context = SSL_CTX_new(TLSv1_method());

It's an ugly problem, because TLSv1_method() actually *should* mean
"TLSv1.0 and higher", but due to problems with various broken
SSL implementations, it's disabled.

I see various ways it can improve:

1) OpenSSL guys change default back to TLSv1.0+.
2) OpenSSL guys give switch to make TLSv1_method() mean TLSv1.0+.
3) libpq starts using TLSv1_2_method() by default.
4) libpq will give switch to users to request TLSv1.2.

I see 1) and 3) as unlikely to happen. As it's not an urgent problem,
we could watch if 2) happens and go with 4) otherwise.

I tried your suggested OP_ALL patch and it does not work. And it's
even harmful to use as it disables few security workarounds.
It's mainly for servers for compat with legacy browsers.

I also tried to clear OP_NO_TLSv1_x to see if there is some default
OPs in TLSv1_method() that we could change, but that also did not work.
My conclusion is that currently there is no switch to make TLSv1.0+
work. (OpenSSL v1.0.1 / 1.1.0-git).

--
marko

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

Re: [HACKERS] lock on object is already held

Daniel Wood  writes:
> ... Presuming your fix is putting PG_SETMASK(&UnBlockSig)
> immediately before each of the 6 calls to ereport(ERROR,...) I've been
> running the stress test with both this fix and the lock already held fix.

I'm now planning to put it in error cleanup instead, but that's good
enough for proving that the problem is what I thought it was.

> I get plenty of lock timeout errors as expected.  However, once in a great
> while I get:  sqlcode = -400, sqlstate = 57014, sqlerrmc = canceling
> statement due to user request
> My stress test certainly doesn't do a user cancel.  Should this be expected?

I think I see what must be happening there: the lock timeout interrupt is
happening at some point after the lock has been granted, but before
ProcSleep reaches its disable_timeouts call.  QueryCancelPending gets set,
and will be honored next time something does CHECK_FOR_INTERRUPTS.
But because ProcSleep told disable_timeouts to clear the LOCK_TIMEOUT
indicator bit, ProcessInterrupts thinks the cancel must've been a plain
user SIGINT, and reports it that way.

What we should probably do about this is change ProcSleep to not clear the 
LOCK_TIMEOUT indicator bit, same as we already do in LockErrorCleanup,
which is the less-race-condition-y path out of a lock timeout.

(It would be cooler if the timeout handler had a way to realize that the
lock is already granted, and not issue a query cancel in the first place.
But having a signal handler poking at shared memory state is a little too
scary for my taste.)

It strikes me that this also means that places where we throw away pending
cancels by clearing QueryCancelPending, such as the sigsetjmp stanza in
postgres.c, had better reset the LOCK_TIMEOUT indicator bit.  Otherwise,
a thrown-away lock timeout cancel might cause a later SIGINT cancel to be
misreported.

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] [RFC] overflow checks optimized away

2013-11-29 Thread Greg Stark

On Fri, Nov 29, 2013 at 7:39 PM, Greg Stark  wrote:
>
> Just as an update I did get gcc to do the wrong thing on purpose. The
> only overflow check that the regression tests find missing is the one
> for int8abs() ie:

Also, one of the places GCC warns about optimizing away an overflow
check (with -fno-wrapv) is inside the localtime.c file from the tz
library. I fixed it in my patch but in fact I checked and it's already
fixed upstream so I'm wondering whether you expect to merge in an
updated tz library? Is there anything surprising about the process or
do you just copy in the files? Would you be happy for someone else to
do it?

-- 
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] MultiXact truncation, startup et al.

On 2013-11-29 16:30:08 -0300, Alvaro Herrera wrote:
> New versions of all these patches, plus one more patch which removes the
> behavior that HeapTupleGetUpdateXid checks for aborted updates.

> From 0dce0b75da2732ca93f4c451b9bae6d4416794c3 Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera 
> Date: Fri, 29 Nov 2013 16:08:06 -0300
> Subject: [PATCH 4/5] Don't TransactionIdDidAbort in HeapTupleGetUpdateXid
> 
> It is dangerous to do so, because some code expects to be able to see what's
> the true Xmax even if it is aborted (particularly while traversing HOT
> chains).  So don't do it, and instead rely on the callers to verify for
> abortedness, if necessary.
> 
> Several race conditions and bugs fixed in the process.

The current version of that additional patch causes two failures in
isolationtester's delete-abort-savept.spec/out. But afaics the old
behaviour was a bug: An updater seems to have waited for an aborted
subtransaction.

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] [RFC] overflow checks optimized away

2013-11-29 Thread Greg Stark

On Fri, Nov 29, 2013 at 5:21 PM, Tom Lane  wrote:
>
>> c) I want to add regression tests that will ensure that the overflow
>> checks are all working. So far I haven't been able to catch any being
>> optimized away even with -fno-wrapv and -fstrict-overflow.
>
> This does not leave me with a warm feeling about whether this is a useful
> exercise.  Maybe you need to try a different compiler?

Just as an update I did get gcc to do the wrong thing on purpose. The
only overflow check that the regression tests find missing is the one
for int8abs() ie:

*** /home/stark/src/postgresql/postgresql/src/test/regress/expected/int8.out
   Wed Jul 17 19:23:02 2013
--- /home/stark/src/postgresql/postgresql/src/test/regress/results/int8.out
   Fri Nov 29 14:22:31 2013
***
*** 674,680 
  select '9223372036854775800'::int8 % '0'::int8;
  ERROR:  division by zero
  select abs('-9223372036854775808'::int8);
! ERROR:  bigint out of range
  select '9223372036854775800'::int8 + '100'::int4;
  ERROR:  bigint out of range
  select '-9223372036854775800'::int8 - '100'::int4;
--- 674,684 
  select '9223372036854775800'::int8 % '0'::int8;
  ERROR:  division by zero
  select abs('-9223372036854775808'::int8);
!  abs
! --
!  -9223372036854775808
! (1 row)
!
  select '9223372036854775800'::int8 + '100'::int4;
  ERROR:  bigint out of range
  select '-9223372036854775800'::int8 - '100'::int4;

==



-- 
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] GIN improvements part 1: additional information


On 11/29/2013 11:41 AM, Heikki Linnakangas wrote:

On 11/28/2013 09:19 AM, Alexander Korotkov wrote:

On Wed, Nov 27, 2013 at 1:14 AM, Heikki Linnakangas

wrote:



On 11/26/13 15:34, Alexander Korotkov wrote:


What's your plans about GIN now? I tried to rebase packed posting lists
with head. But I found that you've changed interface of placeToPage
function. That's conflicts with packed posting lists, because
dataPlaceToPageLeaf needs not only offset number to describe place to
insert item pointer. Do you like to commit rework of handling GIN
incomplete splits before?


Yeah, I'm planning to get back to this patch after committing the
incomplete splits patch. I think the refactoring of the WAL-logging
that I
did in that patch will simplify this patch, too. I'll take a look at
Michael's latest comments on the incomplete splits patch tomorrow, so I
should get back to this on Thursday or Friday.


Should I try to rebase this patch now or you plan to do it yourself? Some
changes like "void *insertdata" argument make me think you have some
particular plan to rebase this patch, but I didn't get it exactly.


Here's rebased version. I'll continue reviewing it now..


Another update. Fixes a bunch of bugs. Mostly introduced by me, but a 
couple were present in your v16:


* When allocating the entry->list buffer in a scan, it must be large 
enough for the max number of items that can fit on a compressed page, 
whether the current page is compressed or not. That's because the same 
buffer is reused on subsequent pages, which might be compressed.


* When splitting a leaf page during index creation, missed the trick 
that's present in current master, to choose the split point so that left 
page is packed as full as possible. I put that back, it makes 
newly-built indexes somewhat smaller. (I wonder if we should leave some 
free space for future updates. But that's a separate patch, let's keep 
the current behavior in this patch)


I'll continue reviewing next week..

- Heikki


gin-packed-postinglists-18.patch.gz
Description: GNU Zip compressed 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] Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

On Fri, Nov 29, 2013 at 01:05:20PM -0500, Bruce Momjian wrote:
> On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> > On Thu, Nov 28, 2013 at 11:04 PM, Alvaro Herrera
> >  wrote:
> > > David Johnston wrote:
> > >
> > >> In all of these cases we are assuming that the user understands that
> > >> emitting a warning means that something is being logged to disk and thus 
> > >> is
> > >> causing a resource drain.
> > >>
> > >> I like explicitly saying that issuing these commands is pointless/"has no
> > >> effect"; being indirect and saying that the only thing they do is emit a
> > >> warning omits any explicit explicit explanation of why.  And while I 
> > >> agree
> > >> that logging the warning is an effect; but it is not the primary/direct
> > >> effect that the user cares about.
> > >
> > > Honestly I still prefer what I proposed initially, which AFAICS has all
> > > the properties you deem desirable in the wording:
> > >
> > > "issuing ROLLBACK outside a transaction emits a warning and otherwise has 
> > > no effect".
> > 
> > Yeah, I still like "otherwise has no effect" or "has no other effect"
> > best.  But I can live with Bruce's latest proposal, too.
> 
> OK, great, I have gone with Alvaro's wording;  patch attached.

Duh, missing patch.  Attached now.

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

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/abort.sgml b/doc/src/sgml/ref/abort.sgml
new file mode 100644
index f3a2fa8..e9138d5
*** a/doc/src/sgml/ref/abort.sgml
--- b/doc/src/sgml/ref/abort.sgml
*** ABORT [ WORK | TRANSACTION ]
*** 63,69 

  

!Issuing ABORT outside of a transaction block has no effect.

   
  
--- 63,70 

  

!Issuing ABORT outside of a transaction block
!emits a warning and otherwise has no effect.

   
  
diff --git a/doc/src/sgml/ref/rollback.sgml b/doc/src/sgml/ref/rollback.sgml
new file mode 100644
index 4f79621..9a1529f
*** a/doc/src/sgml/ref/rollback.sgml
--- b/doc/src/sgml/ref/rollback.sgml
*** ROLLBACK [ WORK | TRANSACTION ]
*** 60,66 
  

 Issuing ROLLBACK outside of a transaction
!block has no effect.

   
  
--- 60,66 
  

 Issuing ROLLBACK outside of a transaction
!block emits a warning and otherwise has no effect.

   
  
diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml
new file mode 100644
index 5a84f69..aaad61e
*** a/doc/src/sgml/ref/set.sgml
--- b/doc/src/sgml/ref/set.sgml
*** SET [ SESSION | LOCAL ] TIME ZONE { 
Specifies that the command takes effect for only the current
transaction.  After COMMIT or ROLLBACK,
!   the session-level setting takes effect again.  This has no effect
!   outside of a transaction block.
   
  
 
--- 110,118 
   
Specifies that the command takes effect for only the current
transaction.  After COMMIT or ROLLBACK,
!   the session-level setting takes effect again.  Issuing this
!   outside of a transaction block emits a warning and otherwise has
!   no effect.
   
  
 
diff --git a/doc/src/sgml/ref/set_constraints.sgml b/doc/src/sgml/ref/set_constraints.sgml
new file mode 100644
index a33190c..60cabed
*** a/doc/src/sgml/ref/set_constraints.sgml
--- b/doc/src/sgml/ref/set_constraints.sgml
*** SET CONSTRAINTS { ALL | 
 This command only alters the behavior of constraints within the
!current transaction.  This has no effect outside of a transaction block.

   
  
--- 99,106 
  

 This command only alters the behavior of constraints within the
!current transaction.  Issuing this outside of a transaction block
!emits a warning and otherwise has no effect.

   
  
diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
new file mode 100644
index e90ff4a..029b75a
*** a/doc/src/sgml/ref/set_transaction.sgml
--- b/doc/src/sgml/ref/set_transaction.sgml
*** SET SESSION CHARACTERISTICS AS TRANSACTI
*** 185,191 

 If SET TRANSACTION is executed without a prior
 START TRANSACTION or BEGIN,
!it will have no effect.

  

--- 185,191 

 If SET TRANSACTION is executed without a prior
 START TRANSACTION or BEGIN,
!it emits a warning and otherwise has no effect.

  


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

Re: [HACKERS] Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> On Thu, Nov 28, 2013 at 11:04 PM, Alvaro Herrera
>  wrote:
> > David Johnston wrote:
> >
> >> In all of these cases we are assuming that the user understands that
> >> emitting a warning means that something is being logged to disk and thus is
> >> causing a resource drain.
> >>
> >> I like explicitly saying that issuing these commands is pointless/"has no
> >> effect"; being indirect and saying that the only thing they do is emit a
> >> warning omits any explicit explicit explanation of why.  And while I agree
> >> that logging the warning is an effect; but it is not the primary/direct
> >> effect that the user cares about.
> >
> > Honestly I still prefer what I proposed initially, which AFAICS has all
> > the properties you deem desirable in the wording:
> >
> > "issuing ROLLBACK outside a transaction emits a warning and otherwise has 
> > no effect".
> 
> Yeah, I still like "otherwise has no effect" or "has no other effect"
> best.  But I can live with Bruce's latest proposal, too.

OK, great, I have gone with Alvaro's wording;  patch attached.

-- 
  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] Todo item: Support amgettuple() in GIN

2013-11-29 Thread Andreas Karlsson


On 11/29/2013 06:13 PM, Tom Lane wrote:

Note that Robert's proposed solution is no solution, because it just
puts you right back in the bind of needing guaranteed non-lossy
storage of a TID set that might be too big to fit in memory.


The solution should work if we could guarantee that a TIDBitmap based on 
the fast update pending list always will fit in the memory. That does 
not sound like a good assumption to me.


--
Andreas Karlsson


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

Re: [HACKERS] Todo item: Support amgettuple() in GIN

On 11/29/2013 07:13 PM, Tom Lane wrote:

Andreas Karlsson writes:

I decided to look into how much work implementing the todo item about
supporting amgettuple in GIN would be, since exclusion constraints on
GIN would be neat. Robert Haas suggested a solution[1], but to fix it we
also need to look into why the commit message mentions that it did not
work anyway with the partial matches.
...
This TIDBitmap becomes lossy if it too many TIDs are added to it, and
this case is what broke amgettuple for partial matches.

Right, see
https://urldefense.proofpoint.com/v1/url?u=http://www.postgresql.org/message-id/49AC300F.1050903%40enterprisedb.com&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=xGch4oNJbpD%2BKPJECmgw4SLBhytSZLBX7UnkZhtNcpw%3D%0A&m=OqhHlGFG81LH1EqJLzTW8HuXdXslGEL%2FPu1f27HxV%2Bs%3D%0A&s=9f3fad064e2845bd2b99c85f684d237fbe96e542081e4b2dc49b1fe51f91f144

Note that fixing the potential lossiness in scanning is not the only
roadblock to re-enabling amgettuple. Fast updates also pose problems:
https://urldefense.proofpoint.com/v1/url?u=http://www.postgresql.org/message-id/4974B002.3040202%40sigaev.ru&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=xGch4oNJbpD%2BKPJECmgw4SLBhytSZLBX7UnkZhtNcpw%3D%0A&m=OqhHlGFG81LH1EqJLzTW8HuXdXslGEL%2FPu1f27HxV%2Bs%3D%0A&s=0e08a781fcc17a3d68ce247344a3499a23a9f545b937f254439dadfaf7b9b8ab

Half of that is basically the same lossiness problem, but the other
half is that we're relying on the bitmap to suppress duplicate reports
of the same TID. It's fairly hard to see how you'd avoid that without
creating other problems.

Note that Robert's proposed solution is no solution, because it just
puts you right back in the bind of needing guaranteed non-lossy
storage of a TID set that might be too big to fit in memory.

You can always call amgetbitmap, and return the tuples from the bitmap
one by one. For a lossy result, re-check all tuples on the page. IOW, do
a bitmap index + heap scan. You could do that within indexam.c, and
present the familiar index_getnext() interface for callers. Or you could
modify the exclusion constraint code to do that if amgettuple is not
available

- 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] Re: Suggestion: Issue warning when calling SET TRANSACTION outside transaction block

On Fri, Nov 29, 2013 at 12:27:49AM -0500, Robert Haas wrote:
> I wish we'd just left this whole thing well enough alone.  It wasn't
> broken, and didn't need fixing.

Well, this started with a complaint that one SET command outside of a
transaction had no effect, and expanded to other SET commands and the
ABORT/notice inconsistency.

I realize the doc discussion is probably excessive, but we do regularly
get credit for our docs:

https://www.sparkfun.com/news/1316
The PostgreSQL manual is a thing of quiet beauty.

I hope "quiet beauty" matches our discussion goal here.  :-)

-- 
  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] [RFC] overflow checks optimized away

Greg Stark  writes:
> b) I'm concerned these checks depend on INT_MIN/MAX and SHRT_MIN/MAX
> which may not be exactly the right length. I'm kind of confused why
> c.h assumes long is 32 bits and short is 16 bits though so I don't
> think I'm making it any worse.

I think it's something we figured we could make configure deal with
if it ever proved necessary; which it hasn't yet.  (In practice, unless
an implementor is going to omit support for these integer widths entirely,
he doesn't have too much choice but to assign them the names "short"
and "int" --- C doesn't provide all that many names he can use.)

> It may be better for us to just define
> our own limits since we know exactly how large we expect these data
> types to be.

Yeah, using INT16_MAX etc would likely be more transparent, if the code
is declaring the variables as int16.

> c) I want to add regression tests that will ensure that the overflow
> checks are all working. So far I haven't been able to catch any being
> optimized away even with -fno-wrapv and -fstrict-overflow.

This does not leave me with a warm feeling about whether this is a useful
exercise.  Maybe you need to try a different compiler?

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] Todo item: Support amgettuple() in GIN

Andreas Karlsson  writes:
> I decided to look into how much work implementing the todo item about 
> supporting amgettuple in GIN would be, since exclusion constraints on 
> GIN would be neat. Robert Haas suggested a solution[1], but to fix it we 
> also need to look into why the commit message mentions that it did not 
> work anyway with the partial matches.
> ...
> This TIDBitmap becomes lossy if it too many TIDs are added to it, and 
> this case is what broke amgettuple for partial matches.

Right, see
http://www.postgresql.org/message-id/49ac300f.1050...@enterprisedb.com

Note that fixing the potential lossiness in scanning is not the only
roadblock to re-enabling amgettuple.  Fast updates also pose problems:
http://www.postgresql.org/message-id/4974b002.3040...@sigaev.ru

Half of that is basically the same lossiness problem, but the other
half is that we're relying on the bitmap to suppress duplicate reports
of the same TID.  It's fairly hard to see how you'd avoid that without
creating other problems.

Note that Robert's proposed solution is no solution, because it just
puts you right back in the bind of needing guaranteed non-lossy
storage of a TID set that might be too big to fit in memory.

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] [PATCH 1/2] SSL: GUC option to prefer server cipher order

On Fri, Nov 29, 2013 at 05:51:28PM +0200, Heikki Linnakangas wrote:
> On 11/29/2013 05:43 PM, Marko Kreen wrote:
> >On Fri, Nov 29, 2013 at 09:25:02AM -0500, Peter Eisentraut wrote:
> >>On Thu, 2013-11-14 at 11:45 +0100, Magnus Hagander wrote:
> >>>I think the default behaviour should be the one we recommend (which
> >>>would be to have the server one be preferred). But I do agree with the
> >>>requirement to have a GUC to be able to  remove it
> >>
> >>Is there a reason why you would want to turn it off?
> >
> >GUC is there so old behaviour can be restored.
> >
> >Why would anyone want that, I don't know.  In context of PostgreSQL,
> >I see no reason to prefer old behaviour.
> 
> Imagine that the server is public, and anyone can connect. The
> server offers SSL protection not to protect the data in the server,
> since that's public anyway, but to protect the communication of the
> client. In that situation, it should be the client's choice what
> encryption to use (if any). This is analogous to using https on a
> public website.
> 
> I concur that that's pretty far-fetched. Just changing the behavior,
> with no GUC, is fine by me.

But client can control that behaviour - it just needs to specify
suites it wants and drop the rest.

So only question is that does any client have better (non-tuned?)
defaults than we can set from server.

Considering the whole HTTPS world has answered 'no' to that question
and nowadays server-controlled behaviour is preferred, I think it's
safe to change the behaviour in Postgres too.

-- 
marko



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

Re: [HACKERS] SSL: better default ciphersuite

On Fri, Nov 29, 2013 at 09:18:49AM -0500, Peter Eisentraut wrote:
> On Fri, 2013-11-15 at 01:11 +0200, Marko Kreen wrote:
> > Attached patch changes the default ciphersuite to
> > 
> > HIGH:!aNULL
> > 
> > instead of old
> > 
> > DEFAULT:!LOW:!EXP:!MD5:@STRENGTH
> > 
> > where DEFAULT is a shortcut for "ALL:!aNULL:!eNULL".
> 
> > Main goal is to leave low-level ciphersuite details to OpenSSL guys
> > and give clear impression to Postgres admins what it is about.
> 
> If we want to leave the details of the ciphers to OpenSSL, I think we
> shouldn't be second-guessing their judgement of what a reasonable
> default is.

Well, we should - the DEFAULT is clearly a client-side default
for compatibility only.  No server should ever run with it.

OTOH, the whole point of "HIGH:!aNULL" is to leave low-level
ciphersuite details to OpenSSL guys - as a Postgres admin
is not clear to me that

   DEFAULT:!LOW:!EXP:!MD5:@STRENGTH

is actually good suite.  It contains "DEFAULT" plus several
fixes which show that DEFAULT is not good enough.  Why all that?

Admin would need to do lot research to see what it is about.
Another aspect is that the "HIGH:!aNULL" is more futureproof
as default, current default has needed fixes (!LOW:!EXP:!MD5)
and would need more fixes in the future (RC4).

Also note that OpenSSL has only one ordered cipher list - ALL.
All other tokens simply walk that list while keeping the order.
So it's not like not using DEFAULT would somehow lose important
details about order.

> I checked Apache mod_ssl and Postfix, and even though they are
> configuring this slightly differently, I think their defaults end up
> being about the same as what PostgreSQL currently has.
> 
> https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslciphersuite
> http://www.postfix.org/postconf.5.html#smtpd_tls_mandatory_ciphers

My proposal is inspired by nginx default:

  http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_ciphers

> > HIGH:
> >   Contains only secure and well-researched algorithms.
> > 
> > !aNULL
> >   Needed to disable suites that do not authenticate server.
> >   DEFAULT includes !aNULL by default.
> 
> Wouldn't HIGH exclude aNULL also?  (If not, what about eNULL?)

HIGH/MEDIUM/LOW/ALL are only about cipher strength so they don't
exclude aNULL.  HIGH does exclude eNULL because eNULL ciphers
are not strong enough...

> > !MD5
> >   This affects only one suite: DES-CBC3-MD5, which is available only
> >   for SSL2 connections.  So it would only pollute the default value.
> 
> I think this is only there for political correctness.

But it's noise so should be removed.

> > @STRENGTH
> >   The OpenSSL cipher list is already sorted by humans,
> >   it's unlikely that mechanical sort would improve things.
> >   Also the existence of this value in old list is rather
> >   dubious, as server cipher order was never respected anyway.
> 
> Aren't you proposing to change that?

No, ALL and subsets of it (HIGH) are already ordered by @STRENGTH.
@STRENGTH as token is only useful if admin does complex filtering by
other parameters then wants to reorder it again by cipher strength.

Eg. an other default I've considered is:

  EECDH+HIGH:EDH+HIGH:@STRENGTH

which enforces ephemeral key use.  @STRENGTH is actually useful there,
as without it ECDHE-AES128 would be preferred to DHE-AES256.

But it exposes unnecessary complexity to database admins who
are not particularly familiar with TLS and OpenSSL internals.
So I think the HIGH:!aNULL is better default as it's simpler.
And ephemeral keys are preferred anyway.

-- 
marko

PS. @STRENGTH and OpenSSL default order in general has problem
that it orders 3DES (168-bit key) before AES128, but 3DES
strength is around 112-bit only.  So crypto-wise, the
"perfect" default, while keeping compatibility, would be:

  EECDH+HIGH:EDH+HIGH:@STRENGTH:+3DES

but it's getting messier and messier...

PS2.  And more fragile too - admin could change +3DES to -3DES
and that would be fine, but plain '3DES' would enable aNULL suite.
So keeping '!aNULL' visible might not be bad idea.

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

Re: [HACKERS] [GENERAL] pg_upgrade ?deficiency

On Fri, Nov 29, 2013 at 12:46:01AM +0100, Karsten Hilbert wrote:
> On Thu, Nov 28, 2013 at 10:39:18AM -0500, Bruce Momjian wrote:
> 
> > Well, then we are actually using two different reasons for patching
> > pg_dumpall and not pg_dump.  Your reason is based on the probability of
> > failure, while Tom/Kevin's reason is that default_transaction_read_only
> > might be used to block changes to a specific database, and they want a
> > pg_dump restore prevented, but a pg_dumpall restore to succeed.
> 
> I can't really argue one way or another because *I* am
> not likely to be able to offer an actual patch. At any
> rate all I am interested in is that pg_upgrade does not
> fail to upgrade in surprising ways.

Once the patch is applied, I will be patching pg_upgrade by appending to
PGOPTIONS, but that will only be for 9.4.  The patch will be too risky
and there are not enough problem reports to override that and warrant
backpatching.

> Regarding restoring a pg_dump IMO the line would need to
> be drawn along the -c/--clean option because using such seems
> to clearly say that, yes, the user *wants* data to be deleted.
> 
> If -C/--create is used it shouldn't matter ...
> 
> However, I'm not saying that this is how it is to
> be done. I am well aware that drawing such subtle
> distinctions is walking quite a fine line.

So you are saying that default_transaction_read_only should be turned
off by pg_dump if --clean is used?  Interesting.

The bottom line is that we can't handle every case and if we tried the
code would be very complex and error-prone.  I am not sure where to draw
the line but it has to be drawn somewhere.

-- 
  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] MultiXact truncation, startup et al.

On 2013-11-29 12:49:32 -0300, Alvaro Herrera wrote:
> + * - xidFullScanLimit (also known as the table freeze age) represents the
> + *   minimum Xid age past which a vacuum will be turned into a full-table 
> one,
> + *   to freeze tuples across the whole table.  Vacuuming a table younger than
> + *   this can use a partial scan.

Imo "age" isn't really appropriate, since it's a concrete xid that's the
cutoff. It's determined by vacuum_freeze_table_age, sure, but at that
point it's an absolute value.

> + * - mxactFullScanLimit (as xidFullScanLimit) represents the minimum 
> MultiXact
> + *   age past which a vacuum will be turned into a full-table one, as with
> + *   xidFullScanLimit.

Not an age again.


> + scan_all |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
> + 
> mxactFullScanLimit);

Hah. That's cute ;).
> @@ -1906,6 +1931,7 @@ CheckPointMultiXact(void)
>   SimpleLruFlush(MultiXactOffsetCtl, true);
>   SimpleLruFlush(MultiXactMemberCtl, true);
>  
> +
>   TRACE_POSTGRESQL_MULTIXACT_CHECKPOINT_DONE(true);
>  }

My fault, but superflous newline added.

> @@ -8619,6 +8623,22 @@ CreateRestartPoint(int flags)
>   }
>   LWLockRelease(ControlFileLock);
>  
> +

Also an inconsistent newline, again by me :(

> - multi = HeapTupleHeaderGetRawXmax(tuple);
> - if (MultiXactIdPrecedes(multi, cutoff_multi))
> - return true;
> + nmembers = GetMultiXactIdMembers(xid, &members, true);
> + for (i = 0; i < nmembers; i++)
> + {
> + TransactionId member = members[i].xid;
> + Assert(TransactionIdIsNormal(member));
> +
> + /* we don't care about lockers */
> + if (ISUPDATE_from_mxstatus(members[i].status))
> + continue;

Isn't there a ! missing?

> diff --git a/src/backend/access/transam/multixact.c 
> b/src/backend/access/transam/multixact.c
> index c389bf3..518c22d 100644
> --- a/src/backend/access/transam/multixact.c
> +++ b/src/backend/access/transam/multixact.c
> @@ -445,6 +445,10 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, 
> MultiXactStatus status)
>  
>   for (i = 0, j = 0; i < nmembers; i++)
>   {
> + /*
> +  * FIXME: is it possible that we copy over too old updater xids
> +  * here?
> +  */
>   if (TransactionIdIsInProgress(members[i].xid) ||
>   ((members[i].status > MultiXactStatusForUpdate) &&
>TransactionIdDidCommit(members[i].xid)))

That's not really a new thing though, so I am fine with leaving that as
is for 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] [PATCH 1/2] SSL: GUC option to prefer server cipher order


On 11/29/2013 05:43 PM, Marko Kreen wrote:

On Fri, Nov 29, 2013 at 09:25:02AM -0500, Peter Eisentraut wrote:

On Thu, 2013-11-14 at 11:45 +0100, Magnus Hagander wrote:

I think the default behaviour should be the one we recommend (which
would be to have the server one be preferred). But I do agree with the
requirement to have a GUC to be able to  remove it


Is there a reason why you would want to turn it off?


GUC is there so old behaviour can be restored.

Why would anyone want that, I don't know.  In context of PostgreSQL,
I see no reason to prefer old behaviour.


Imagine that the server is public, and anyone can connect. The server 
offers SSL protection not to protect the data in the server, since 
that's public anyway, but to protect the communication of the client. In 
that situation, it should be the client's choice what encryption to use 
(if any). This is analogous to using https on a public website.


I concur that that's pretty far-fetched. Just changing the behavior, 
with no GUC, is fine by me.


- 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] [PATCH 1/2] SSL: GUC option to prefer server cipher order

On Fri, Nov 29, 2013 at 09:25:02AM -0500, Peter Eisentraut wrote:
> On Thu, 2013-11-14 at 11:45 +0100, Magnus Hagander wrote:
> > I think the default behaviour should be the one we recommend (which
> > would be to have the server one be preferred). But I do agree with the
> > requirement to have a GUC to be able to  remove it
> 
> Is there a reason why you would want to turn it off?

GUC is there so old behaviour can be restored.

Why would anyone want that, I don't know.  In context of PostgreSQL,
I see no reason to prefer old behaviour.

-- 
marko



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

Re: [HACKERS] Status of FDW pushdowns

2013-11-29 Thread Atri Sharma



Sent from my iPad

> On 28-Nov-2013, at 16:13, Dimitri Fontaine  wrote:
> 
> Tom Lane  writes:
>> I'm not real sure what this'd buy us that wouldn't be done as well or
>> better by creating a view on the remote side.  (IOW, there's nothing
>> that says that the remote object backing a foreign table can't be a
>> view.)
> 
> Agreed, for those remote sides that know what a view is.

I agree.

I agree with the overall model here, but I am not sure how it would work out 
for non SQL supporting remote sides.

Regards,
Atri

-- 
Sent 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 1/2] SSL: GUC option to prefer server cipher order

2013-11-29 Thread Peter Eisentraut

On Thu, 2013-11-14 at 11:45 +0100, Magnus Hagander wrote:
> I think the default behaviour should be the one we recommend (which
> would be to have the server one be preferred). But I do agree with the
> requirement to have a GUC to be able to  remove it

Is there a reason why you would want to turn it off?




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

Re: [HACKERS] SSL: better default ciphersuite

2013-11-29 Thread Peter Eisentraut

On Fri, 2013-11-15 at 01:11 +0200, Marko Kreen wrote:
> Attached patch changes the default ciphersuite to
> 
> HIGH:!aNULL
> 
> instead of old
> 
> DEFAULT:!LOW:!EXP:!MD5:@STRENGTH
> 
> where DEFAULT is a shortcut for "ALL:!aNULL:!eNULL".

> Main goal is to leave low-level ciphersuite details to OpenSSL guys
> and give clear impression to Postgres admins what it is about.

If we want to leave the details of the ciphers to OpenSSL, I think we
shouldn't be second-guessing their judgement of what a reasonable
default is.

I checked Apache mod_ssl and Postfix, and even though they are
configuring this slightly differently, I think their defaults end up
being about the same as what PostgreSQL currently has.

https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslciphersuite
http://www.postfix.org/postconf.5.html#smtpd_tls_mandatory_ciphers

> HIGH:
>   Contains only secure and well-researched algorithms.
> 
> !aNULL
>   Needed to disable suites that do not authenticate server.
>   DEFAULT includes !aNULL by default.

Wouldn't HIGH exclude aNULL also?  (If not, what about eNULL?)

> !MD5
>   This affects only one suite: DES-CBC3-MD5, which is available only
>   for SSL2 connections.  So it would only pollute the default value.

I think this is only there for political correctness.

> @STRENGTH
>   The OpenSSL cipher list is already sorted by humans,
>   it's unlikely that mechanical sort would improve things.
>   Also the existence of this value in old list is rather
>   dubious, as server cipher order was never respected anyway.

Aren't you proposing to change that?




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

Re: [HACKERS] Todo item: Support amgettuple() in GIN

2013-11-29 Thread Antonin Houska

On 11/29/2013 01:57 PM, Andreas Karlsson wrote:
> On 11/29/2013 09:54 AM, Antonin Houska wrote:
>> On 11/29/2013 01:13 AM, Andreas Karlsson wrote:
>>
>>> When doing partial matching the code need to be able to return the union
>>> of all TIDs in all the matching posting trees in TID order (to be able
>>> to do AND and OR operations with multiple search keys later). It does
>>> this by traversing them posting tree after posting tree and collecting
>>> them all in a TIDBitmap which is later iterated over.
>>
>> I think it's not a plain union. My understanding is that - to evaluate a
>> single key (typically array) - you first need to get all the TID streams
>> for that key (i.e. one posting list/tree per element of the key array)
>> and then iterate all these streams in parallel and 'merge' them  using
>> consistent() function. That's how I understand ginget.c:keyGetItem().
> 
> For partial matches the merging is done in two steps: first a simple 
> union of all the streams per key and then second merging those union 
> streams using the consistent() function.

Yes, short after I sent my previous mail I realized that your "union"
probably referred to the things that collectMatchBitmap() does.

// Tony



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

Re: [HACKERS] COPY table FROM STDIN doesn't show count tag

2013-11-29 Thread Rajeev rastogi

On 26 November 2013, Amit Khandelkar wrote:
>Can you please submit the \COPY patch as a separate patch ? Since these are 
>two different issues, I would like to have these two fixed and committed 
>separately. You can always test the \COPY issue using \COPY TO followed by 
>INSERT.

Please find the attached two separate patches:


1.  slashcopyissuev1.patch :- This patch fixes the \COPY issue.

2.  initialcopyissuev1_ontopofslashcopy.patch : Fix for "COPY table FROM 
STDIN/STDOUT doesn't show count tag".

Thanks and Regards,
Kumar Rajeev Rastogi




slashcopyissuev1.patch
Description: slashcopyissuev1.patch


initialcopyissuev1_ontopofslashcopy.patch
Description: initialcopyissuev1_ontopofslashcopy.patch

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

Re: [HACKERS] Heavily modified big table bloat even in auto vacuum is running

2013-11-29 Thread Haribabu kommi

On 29 November 2013 12:00 Amit Kapila wrote: 
> On Tue, Nov 26, 2013 at 7:26 PM, Haribabu kommi
>  wrote:
> > On 25 November 2013 10:43 Amit Kapila wrote:
> >> On Fri, Nov 22, 2013 at 12:12 PM, Haribabu kommi
> >>  wrote:
> >> > On 19 November 2013 10:33 Amit Kapila wrote:
> >> >> If I understood correctly, then your patch's main intention is to
> >> >> correct the estimate of dead tuples, so that it can lead to
> Vacuum
> >> >> cleaning the table/index which otherwise is not happening as per
> >> >> configuration value (autovacuum_vacuum_threshold) in some of the
> >> >> cases, also it is not reducing the complete bloat (Unpatched -
> >> 1532MB
> >> >> ~Patched   - 1474MB), as the main reason of bloat is extra space
> in
> >> >> index which can be reclaimed by reindex operation.
> >> >>
> >> >> So if above is correct then this patch has 3 advantages:
> >> >> a. Extra Vacuum on table/index due to better estimation of dead
> >> tuples.
> >> >> b. Space reclaim due to this extra vacuum c. may be some
> >> >> performance advantage as it will avoid the delay in cleaning dead
> >> >> tuples
> >> >>
> >> >> I think better way to test the patch is to see how much benefit
> is
> >> >> there due to above (a and b points) advantages. Different values
> >> >> of autovacuum_vacuum_threshold can be used to test.
> >> >
> >> >
> >> > The performance effect of the patch is not much visible as I think
> >> the
> >> > analyze on the table estimates the number of dead tuples of the
> >> > table
> >> with some estimation.
> >>
> >>Yes, that seems to be the reason why you are not seeing any
> >> performance benefit, but still I think this is useful optimization
> to
> >> do, as
> >>analyze updates both the livetuples and dead tuples and similarly
> >> vacuum should also update both the counts. Do you see any reason
> >>why Vacuum should only update live tuples and not deadtuples?
> >
> > As vacuum touches all the pages where the dead tuples are present.
> > This is not the Same with analyzer. Because of this reason, the
> analyzer estimates the dead tuples also.
> > With the proposed patch the vacuum also estimates the dead tuples.
> 
> Few questions about your latest patch:
> a. Is there any reason why you are doing estimation of dead tuples only
> for Autovacuum and not for Vacuum.

No, changed.
 
> /* clear and get the new stats for calculating proper dead tuples */
> pgstat_clear_snapshot(); tabentry =
> pgstat_fetch_stat_tabentry(RelationGetRelid(onerel));
> b. In the above code, to get latest data you are first clearing
> snapshot and then calling pgstat function. It will inturn perform I/O
> (read of stats file) and send/receive message from stats collector
> to ensure it can read latest data. I think it will add overhead
> to Vacuum, especially if 'nkeep' calculated in function
> lazy_scan_heap() can serve the purpose. In my simple test[1], I
> observed
> that value of keep can serve the purpose.
> 
> Can you please once try the test on 'nkeep' approach patch.

Using the nkeep and snapshot approach, I ran the test for 40 mins with a
high analyze_threshold and results are below.

Auto vacuum count   Bloat size
Master   11  220MB
Patched_nkeep  14  215MB
Patched_snapshot   18  198MB

Both the approaches are showing good improvement in the test.
Updated patches, test script and configuration is attached in the mail.

Regards,
Hari babu.



vacuum_fix_v6_nkeep.patch
Description: vacuum_fix_v6_nkeep.patch


test_script.tar.gz
Description: test_script.tar.gz


vacuum_fix_v6_snapshot.patch
Description: vacuum_fix_v6_snapshot.patch

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

[HACKERS] commit fest 2013-11 week 2 report

2013-11-29 Thread Peter Eisentraut

Last week's status:

Fri Nov 22

Status Summary. Needs Review: 47, Waiting on Author: 28, Ready for
Committer: 10, Committed: 18, Returned with Feedback: 3, Rejected: 3.
Total: 109.

Current status:

Fri Nov 29

Status Summary. Needs Review: 29, Waiting on Author: 33, Ready for
Committer: 13, Committed: 24, Returned with Feedback: 5, Rejected: 5.
Total: 109.

Last week, we asked reviewers to send in their first review.  Next week,
we'll be kicking off any reviewers who are not responding at all, if
any.

Next week, we'll also start being more eager to set patches as "Returned
with feedback" if it is unlikely that they will get done within the next
two weeks.

If you are an author or a reviewer and it looks as though you are not
active, but you really are, please be heard so that we don't unlist you.



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

Re: [HACKERS] Todo item: Support amgettuple() in GIN

2013-11-29 Thread Andreas Karlsson


On 11/29/2013 09:54 AM, Antonin Houska wrote:

On 11/29/2013 01:13 AM, Andreas Karlsson wrote:


When doing partial matching the code need to be able to return the union
of all TIDs in all the matching posting trees in TID order (to be able
to do AND and OR operations with multiple search keys later). It does
this by traversing them posting tree after posting tree and collecting
them all in a TIDBitmap which is later iterated over.


I think it's not a plain union. My understanding is that - to evaluate a
single key (typically array) - you first need to get all the TID streams
for that key (i.e. one posting list/tree per element of the key array)
and then iterate all these streams in parallel and 'merge' them using
consistent() function. That's how I understand ginget.c:keyGetItem().


For partial matches the merging is done in two steps: first a simple 
union of all the streams per key and then second merging those union 
streams using the consistent() function.


It is the first step that can be lossy.


So the problem of partial match is (IMO) that there can be too many TID
streams to merge - much more than the number of elements of the key array.


Agreed.

--
Andreas Karlsson


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

Re: [HACKERS] COPY table FROM STDIN doesn't show count tag

2013-11-29 Thread Amit Khandekar

On 27 November 2013 09:59, Rajeev rastogi  wrote:

>  On 26 November 2013, Amit Khandelkar wrote:
>
>
> On 26 November 2013 18:59, Amit Khandekar  > wrote:
>
>>
>>
>>
>> On 25 November 2013 15:25, Rajeev rastogi 
>>  wrote:
>>
>>> OK. I have revised the patch as per the discussion.
>>>
>> Could you please submit only the \COPY fix first ? The attached patch
>> also contains the fix for the original COPY status fix.
>>
> Can you please submit the \COPY patch as a separate patch ? Since these
are two different issues, I would like to have these two fixed and
committed separately. You can always test the \COPY issue using \COPY TO
followed by INSERT.

Re: [HACKERS] docbook-xsl version for release builds

2013-11-29 Thread Magnus Hagander

On Fri, Nov 29, 2013 at 4:06 AM, Peter Eisentraut  wrote:

> On Mon, 2013-11-25 at 16:32 +0100, Magnus Hagander wrote:
> > Thanks for the reminder - done (installed the DEB from sid, version
> > 1.78.1).
>
> > This will somehow show up in the snapshot builds, correct? So we
> > (you? :P) can verify after the next snapshots that it's correct?
> >
> After in-depth inspection, please roll back to the previous 1.76.1
> package.  While the whitespace fixes I was looking for indeed appeared,
> I found two more severe formatting bugs that should be fixed before we
> move ahead.
>
> https://sourceforge.net/p/docbook/bugs/1321/
> https://sourceforge.net/p/docbook/bugs/1322/


Done.


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

Re: [HACKERS] Todo item: Support amgettuple() in GIN

2013-11-29 Thread Antonin Houska

On 11/29/2013 01:13 AM, Andreas Karlsson wrote:

> When doing partial matching the code need to be able to return the union 
> of all TIDs in all the matching posting trees in TID order (to be able 
> to do AND and OR operations with multiple search keys later). It does 
> this by traversing them posting tree after posting tree and collecting 
> them all in a TIDBitmap which is later iterated over.

I think it's not a plain union. My understanding is that - to evaluate a
single key (typically array) - you first need to get all the TID streams
for that key (i.e. one posting list/tree per element of the key array)
and then iterate all these streams in parallel and 'merge' them using
consistent() function. That's how I understand ginget.c:keyGetItem().

So the problem of partial match is (IMO) that there can be too many TID
streams to merge - much more than the number of elements of the key array.

// Antonin Houska (Tony)

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

Re: [HACKERS] MultiXact truncation, startup et al.