Re: [HACKERS] pg_basebackup wish list

[Masahiko Sawada]

On Fri, Aug 19, 2016 at 7:06 AM, Peter Eisentraut
 wrote:
> On 7/12/16 9:55 PM, Masahiko Sawada wrote:
>> And what I think is pg_baseback never remove the directory specified
>> by -D option even if execution is failed. initdb command behaves so.
>> I think it's helpful for backup operation.
>
> This has been bothering me as well.
>
> How about the attached patch as a start?
>

Thank you for the patch!

I agree with adding this as an option and removing directory by default.
And it looks good to me except for missing new line in usage output.

printf(_("  -l, --label=LABEL  set backup label\n"));
+   printf(_("  -n, --noclean  do not clean up after errors"));
printf(_("  -P, --progress show progress information\n"));

Registered this patch to CF1.

Regards,

--
Masahiko Sawada


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

Re: [HACKERS] Bug in to_timestamp().

[amul sul]

On Friday, August 19, 2016 12:42 AM, Robert Haas  wrote:
>On Wed, Aug 17, 2016 at 10:35 AM, amul sul  wrote:
>
>
>> Hmm. I haven't really looked into the code, but with applying both patches 
>> it looks precisely imitate Oracle's behaviour. Thanks.
>
>
>This is good to hear, but for us to consider applying something like
>this, somebody would need to look into the code pretty deeply.

Sure Robert, Im planning to start initial review until next week at the 
earliest. Thanks


Regards,
Amul Sul


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

Re: [HACKERS] errno clobbering in reorderbuffer

[Andres Freund]

On August 18, 2016 7:21:03 PM PDT, Alvaro Herrera  
wrote:
>Andres Freund wrote:
>> On 2016-08-18 19:06:02 -0300, Alvaro Herrera wrote:
>> > While researching a customer issue with BDR I noticed that one
>ereport()
>> > call happens after clobbering errno, leading to the wrong strerror
>being
>> > reported.  This patch fixes it by saving before calling
>> > CloseTransientFile and restoring afterwards.
>> > 
>> > I also threw in a missing errcode I noticed while looking for
>similar
>> > problems in the same file.
>> > 
>> > This is to backpatch to 9.4.
>> 
>> Makes sense - you're doing that or shall I?
>
>I am, if you don't mind.

Not at all, thanks.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


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

Re: [HACKERS] errno clobbering in reorderbuffer

[Alvaro Herrera]

Andres Freund wrote:
> On 2016-08-18 19:06:02 -0300, Alvaro Herrera wrote:
> > While researching a customer issue with BDR I noticed that one ereport()
> > call happens after clobbering errno, leading to the wrong strerror being
> > reported.  This patch fixes it by saving before calling
> > CloseTransientFile and restoring afterwards.
> > 
> > I also threw in a missing errcode I noticed while looking for similar
> > problems in the same file.
> > 
> > This is to backpatch to 9.4.
> 
> Makes sense - you're doing that or shall I?

I am, if you don't mind.

-- 
Á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] Missing checks when malloc returns NULL...

[Michael Paquier]

On Thu, Aug 18, 2016 at 6:12 PM, Heikki Linnakangas  wrote:
> On 06/22/2016 04:41 AM, Michael Paquier wrote:
>> make s
>> OK, there is not much that we can do here then. What about the rest?
>> Those seem like legit concerns to me.
>
>
> There's also a realloc() and an strdup() call in refint.c. But looking at
> refint.c, I wonder why it's using malloc()/free() in the first place, rather
> than e.g. TopMemoryContext. The string construction code with sprintf() and
> strlen() looks quite antiqued, too, StringInfo would make it more readable.
>
> Does refint.c actually have any value anymore? What if we just removed it
> altogether? It's good to have some C triggers in contrib, to serve as
> examples, and to have some regression test coverage for all that. But ISTM
> that the 'autoinc' module covers the trigger API just as well.

I'd be in favor for nuking it instead of keeping this code as it
overlaps with autoinc, I did not notice that. Even if that's an
example, it is actually showing some bad code patters, which kills its
own purpose.

> The code in ps_status() runs before the elog machinery has been initialized,
> so you get a rather unhelpful error:
>
>> error occurred at ps_status.c:167 before error message processing is
>> available
>
> I guess that's still better than outright crashing, though. There are also a
> few strdup() calls there that can run out of memory..

Right. Another possibility is to directly call write_stderr to be sure
that the user gets the right message, then do exit(1). Thinking more
about it, as save_ps_display_args is called really at the beginning
this is perhaps what makes the most sense, so I changed the patch this
way.

> Not many of the callers of simple_prompt() check for a NULL result, so in
> all probability, returning NULL from there will just crash in the caller.
> There's one existing "return NULL" in there already, so this isn't a new
> problem, but can we find a better way?

I got inspired by the return value in the case of WIN32. Letting
sprompt.c in charge of printing an error does not seem like a good
idea to me, and there are already callers of simple_prompt() that
check for NULL and report an error appropriately, like pg_backup_db.c.
So I think that we had better adapt all the existing calls of
simple_prompt checking for NULL and make things more consistent by
having the callers report errors. Hence I updated the patch with those
changes.

Another possibility would be to keep a static buffer that has a fixed
size, basically 4096 as this is the maximum expected by psql, but
that's a waste of bytes in all other cases: one caller uses 4096, two
128 and the rest use 100 or less.

By the way, while looking at that, I also noticed that in exec_command
of psql's command.c we don't check for gets_fromFile that could return
NULL.

> There are more malloc(), realloc() and strdup() calls in isolationtester and
> pg_regress, that we ought to change too while we're at it.

Right. I added some handling for those callers as well with the pg_ equivalents.
-- 
Michael
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index e5eeec2..5dd0046 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -306,6 +306,11 @@ sql_conn(struct options * my_opts)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "%s: out of memory\n", "oid2name");
+exit(1);
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 769c805..30b9ed2 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -71,7 +71,14 @@ vacuumlo(const char *database, const struct _param * param)
 
 	/* Note: password can be carried over from a previous call */
 	if (param->pg_prompt == TRI_YES && password == NULL)
+	{
 		password = simple_prompt("Password: ", 100, false);
+		if (!password)
+		{
+			fprintf(stderr, "out of memory\n");
+			return -1;
+		}
+	}
 
 	/*
 	 * Start the connection.  Loop until we have a password if requested by
@@ -115,6 +122,11 @@ vacuumlo(const char *database, const struct _param * param)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "out of memory\n");
+return -1;
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
diff --git a/src/backend/port/dynloader/darwin.c b/src/backend/port/dynloader/darwin.c
index ccd92c3..a83c614 100644
--- a/src/backend/port/dynloader/darwin.c
+++ b/src/backend/port/dynloader/darwin.c
@@ -78,6 +78,9 @@ pg_dlsym(void *handle, char *funcname)
 	NSSymbol symbol;
 	char	   *symname = (char *) malloc(strlen(funcname) + 2);
 
+	if (!symname)
+		return NULL;
+
 	sprintf(symname, "_%s", funcname);
 	if (NSIsSymbolNameDefined(symname))
 	{
diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c
index 892a810..09ef2df 100644
--- 

Re: [HACKERS] [PATCH] bigint txids vs 'xid' type, new txid_recent(bigint) => xid

[Craig Ringer]

On 19 August 2016 at 02:35, Jim Nasby  wrote:

> On 8/18/16 5:46 AM, Amit Kapila wrote:
>
>> I think there is a value in exposing such a variant which takes bigint
>> and internally converts it to xid.  I am not sure the semantics for
>>
>
> I think that's a bad idea because you have the exact same problems we have
> now: bigint is signed, epoch is not.


Eh. A distant future problem IMO. txid_current will already start returning
negative values if the epoch crosses INT32_MAX.

the other proposal txid_recent() is equivalent to what we have for
>> txid_current().  One thing which is different is that txid_current()
>> allocates a new transaction if there is currently none.  If you
>>
>
> Right, and it would be nice to be able to tell if an XID has been assigned
> to your transaction or not; something you currently can't do.


It's trivial to expose GetTopTransactionIdIfAny() . Basically copy and
paste txid_current() to txid_current_ifassigned() and replace the
GetTopTransactionId() call with GetTopTransactionIdIfAny() .

Or add a bool argument to txid_current() to not assign one. But I'd rather
a new function in this case, and it's so short that the duplication is no
concern.

plainly want to convert it to 32 bit xid, then may be txid_convert or
>> something like that is more suitable.
>>
>
> I think we need to either add real types for handling XID/epoch/TXID or
> finally create uint types. It's *way* too easy to screw things up the way
> they are today.


Hm. Large scope increase there. Especially introducing unsigned types.
There's a reason that hasn't been done already - it's not just copying a
whole pile of code, it's also defining all the signed/unsigned interactions
and conversions carefully. People mix signed and unsigned types incorrectly
in C all the time, and often don't notice the problems. It also only gains
you an extra bit. Unsigned types would be nice when interacting with
outside systems that use them and Pg innards, but that's about all they're
good for IMO. For everything else you should be using numeric if you're
worried about fitting in a bigint.

I'm not against adding a 'bigxid' or 'epoch_xid' or something, internally a
uint64. It wouldn't need all the opclasses, casts, function overloads, etc
that uint8 would. It's likely to break code that expects txid_current() to
return a bigint, but since it looks like most of that code is already
silently broken I'm not too upset by that.

Separately to all that, though, we should still have a way to get the
32-bit xid from an xid with epoch that doesn't require the user to know its
internal structure and bitshift it, especially since they can't check the
epoch. Maybe call it txid_convert_ifrecent(bigint). IMO the "recent" part
is important because of the returns-null-if-xid-is-old behaviour. It's not
a straight conversion.

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

Re: [HACKERS] Most efficient way for libPQ .. PGresult serialization

[Craig Ringer]

On 19 August 2016 at 03:08, Joshua Bay  wrote:

> Thanks,
> But I don't think my question was clear enough.
>
> I already managed the connection pooling, and what I need is to serialize
> the result.
>
> If PGresult was a contiguous block, I could have just create buffer and
> call memcpy for serialization, but structure of result seems much more
> complicated.
>
> So, I was asking if there is an easy way to achieve serialization
>

It's wire format is a serialization. That's kind of the point.

I don't understand what you're trying to do here, so it's hard to give a
better answer.

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

Re: [HACKERS] Fix comment in ATExecValidateConstraint

[Amit Langote]

On 2016/08/19 5:35, Robert Haas wrote:
> On Thu, Aug 18, 2016 at 5:15 AM, Amit Langote
>  wrote:
>> On 2016/07/25 17:18, Amit Langote wrote:
>>> The comment seems to have been copied from ATExecAddColumn, which says:
>>>
>>>  /*
>>>   * If we are told not to recurse, there had better not be any
>>> - * child tables; else the addition would put them out of step.
>>>
>>> For ATExecValidateConstraint, it should say something like:
>>>
>>> + * child tables; else validating the constraint would put them
>>> + * out of step.
>>>
>>> Attached patch fixes it.
>>
>> I noticed that the ALTER TABLE documentation doesn't mention that VALIDATE
>> CONSTRAINT will fail if ONLY is specified and there are descendant tables.
>>  It only mentions that a constraint cannot be renamed unless also renamed
>> in the descendant tables.
>>
>> Attached patch fixes that.
> 
> I did some wordsmithing on the two patches you posted to this thread.
> I suggest the attached version.  What do you think?

Reads much less ambiguous, so +1.

Except in the doc patch:

s/change the type of a column constraint/change the type of a column/g

I fixed that part in the attached.

Thanks,
Amit
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6f51cbc..e48ccf2 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1028,11 +1028,15 @@ ALTER TABLE ALL IN TABLESPACE name
 

 If a table has any descendant tables, it is not permitted to add,
-rename, or change the type of a column, or rename an inherited constraint
-in the parent table without doing
-the same to the descendants.  That is, ALTER TABLE ONLY
-will be rejected.  This ensures that the descendants always have
-columns matching the parent.
+rename, or change the type of a column in the parent table without doing
+same to the descendants.  This ensures that the descendants always have
+columns matching the parent.  Similarly, a constraint cannot be renamed
+in the parent without also renaming it in all descendents, so that
+constraints also match between the parent and its descendents.
+Also, because selecting from the parent also selects from its descendents,
+a constraint on the parent cannot be marked valid unless it is also marked
+valid for those descendents.  In all of these cases, ALTER TABLE
+ONLY will be rejected.  

 

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..d312762 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6908,7 +6908,8 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
 
 /*
  * If we are told not to recurse, there had better not be any
- * child tables; else the addition would put them out of step.
+ * child tables, because we can't mark the constraint on the
+ * parent valid unless it is valid for all child tables.
  */
 if (!recurse)
 	ereport(ERROR,

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

Re: [HACKERS] amcheck (B-Tree integrity checking tool)

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 5:35 PM, Michael Paquier
 wrote:
> This would be packaged from source in my case, but that's no big deal
> :) At least I can see that it is added in the next CF, and that's
> marked as ready for committer for a couple of months now...

If you consider how the code is written, you'll see that it's very
unlikely to destabilize production systems.

Every page is copied into local memory once before being operated on,
and only one buffer lock/pin is held at once (for long enough to do
that copying). If there were bugs in amcheck, it's much more likely
that they'd be "false positive" bugs. In any case, I haven't seen any
issue with the tool itself yet, having now run the tool on thousands
of servers.

I think I'll have a lot more information in about a week, when I've
had time to work through more data.

-- 
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] WIP: About CMake v2

[Michael Paquier]

On Fri, Aug 19, 2016 at 4:25 AM, Tom Lane  wrote:
> BTW, I just noticed that cmake doesn't seem to be supplied as part of
> Apple's dev tools, at least not up to current (El Capitan) releases.
> That's going to be a rather large minus to be taken into account
> whenever we make the go/no-go decision on this.

Indeed. Now in order to test the patch on macos, one can depend on
brew's cmake. That's what I am doing.

I recall that a couple of releases before El Capitan (10.11), Lion and
Mountain Lion had a server edition that bundled Postgres natively.
Does this still exist for El Capitan? If yes, moving to cmake may
actually give an argument to those folks to begin to touch cmake and
integrate it natively... But as Apple is a fortress of secrecy that's
hard to tell.
-- 
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] _mdfd_getseg can be expensive

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 5:42 PM, Andres Freund  wrote:
> How large was the index & table in question? I mean this really only
> comes into effect at 100+ segments.

Not that big, but I see no reason to take the chance, I suppose.

-- 
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] _mdfd_getseg can be expensive

[Andres Freund]

On 2016-08-18 17:35:47 -0700, Peter Geoghegan wrote:
> On Thu, Aug 18, 2016 at 5:28 PM, Andres Freund  wrote:
> >> I can review this next week.
> >
> > Thanks
> 
> Given the time frame that you have in mind, I won't revisit the
> question the parallel CLUSTER CPU bottleneck issue until this is
> committed. The patch might change things enough that that would be a
> waste of time.

How large was the index & table in question? I mean this really only
comes into effect at 100+ segments.


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

Re: [HACKERS] _mdfd_getseg can be expensive

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 5:28 PM, Andres Freund  wrote:
>> I can review this next week.
>
> Thanks

Given the time frame that you have in mind, I won't revisit the
question the parallel CLUSTER CPU bottleneck issue until this is
committed. The patch might change things enough that that would be a
waste of time.

-- 
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] amcheck (B-Tree integrity checking tool)

[Michael Paquier]

On Fri, Aug 19, 2016 at 9:14 AM, Peter Geoghegan  wrote:
> On Thu, Aug 18, 2016 at 5:06 PM, Michael Paquier
>  wrote:
>> Cool. I have been honestly wondering about deploying this tool as well
>> to allow some of the QE tests to perform live checks of btree indexes
>> as we use a bunch of them.
>
> I'd certainly welcome that. There are Debian packages available from
> the Github version of amcheck, which is otherwise practically
> identical to the most recent version of the patch posted here:
>
> https://github.com/petergeoghegan/amcheck

This would be packaged from source in my case, but that's no big deal
:) At least I can see that it is added in the next CF, and that's
marked as ready for committer for a couple of months now...
-- 
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] _mdfd_getseg can be expensive

[Andres Freund]

On 2016-08-18 17:27:59 -0700, Peter Geoghegan wrote:
> On Thu, Aug 18, 2016 at 5:26 PM, Andres Freund  wrote:
> > Rebased version attached. A review would be welcome. Plan to push this
> > forward otherwise in the not too far away future.
> 
> I can review this next week.

Thanks


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

Re: [HACKERS] _mdfd_getseg can be expensive

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 5:26 PM, Andres Freund  wrote:
> Rebased version attached. A review would be welcome. Plan to push this
> forward otherwise in the not too far away future.

I can review this next week.


-- 
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] _mdfd_getseg can be expensive

[Andres Freund]

On 2016-06-30 18:14:15 -0700, Peter Geoghegan wrote:
> On Tue, Dec 15, 2015 at 10:04 AM, Andres Freund  wrote:
> > Took a while. But here we go. The attached version is a significantly
> > revised version of my earlier patch. Notably I've pretty much entirely
> > revised the code in _mdfd_getseg() to be more similar to the state in
> > master. Also some comment policing.
> 
> Are you planning to pick this up again, Andres?

Rebased version attached. A review would be welcome. Plan to push this
forward otherwise in the not too far away future.

Andres
>From 62b0d36864b23b91961bb800c1b0814f20d00a8e Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 18 Aug 2016 16:04:33 -0700
Subject: [PATCH 1/2] Improve scalability of md.c for large relations.

Previously several routines in md.c were O(#segments) - a problem these
days, where it's not uncommon to have tables in the multi TB range.

Replace the linked list of segments hanging of SMgrRelationData, with an
array of opened segments. That allows O(1) access to individual
segments, if they've previously been opened.
---
 src/backend/storage/smgr/md.c   | 408 ++--
 src/backend/storage/smgr/smgr.c |   4 +-
 src/include/storage/smgr.h  |   8 +-
 3 files changed, 234 insertions(+), 186 deletions(-)

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index f329d15..0992a7e 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -92,27 +92,23 @@
  *	out to an unlinked old copy of a segment file that will eventually
  *	disappear.
  *
- *	The file descriptor pointer (md_fd field) stored in the SMgrRelation
- *	cache is, therefore, just the head of a list of MdfdVec objects, one
- *	per segment.  But note the md_fd pointer can be NULL, indicating
- *	relation not open.
+ *	File descriptors are stored in md_seg_fds arrays inside SMgrRelation. The
+ *	length of these arrays is stored in md_num_open_segs.  Note that
+ *	md_num_open_segs having a specific value does not necessarily mean the
+ *	relation doesn't have additional segments; we may just not have opened the
+ *	next segment yet.  (We could not have "all segments are in the array" as
+ *	an invariant anyway, since another backend could extend the relation while
+ *	we aren't looking.)  We do not have entries for inactive segments,
+ *	however; as soon as we find a partial segment, we assume that any
+ *	subsequent segments are inactive.
  *
- *	Also note that mdfd_chain == NULL does not necessarily mean the relation
- *	doesn't have another segment after this one; we may just not have
- *	opened the next segment yet.  (We could not have "all segments are
- *	in the chain" as an invariant anyway, since another backend could
- *	extend the relation when we weren't looking.)  We do not make chain
- *	entries for inactive segments, however; as soon as we find a partial
- *	segment, we assume that any subsequent segments are inactive.
- *
- *	All MdfdVec objects are palloc'd in the MdCxt memory context.
+ *	The entire MdfdVec array is palloc'd in the MdCxt memory context.
  */
 
 typedef struct _MdfdVec
 {
 	File		mdfd_vfd;		/* fd number in fd.c's pool */
 	BlockNumber mdfd_segno;		/* segment number, from 0 */
-	struct _MdfdVec *mdfd_chain;	/* next segment, or NULL */
 } MdfdVec;
 
 static MemoryContext MdCxt;		/* context for all MdfdVec objects */
@@ -189,7 +185,9 @@ static MdfdVec *mdopen(SMgrRelation reln, ForkNumber forknum, int behavior);
 static void register_dirty_segment(SMgrRelation reln, ForkNumber forknum,
 	   MdfdVec *seg);
 static void register_unlink(RelFileNodeBackend rnode);
-static MdfdVec *_fdvec_alloc(void);
+static void _fdvec_resize(SMgrRelation reln,
+			  ForkNumber forknum,
+			  int nseg);
 static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum,
 			  BlockNumber segno);
 static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forkno,
@@ -298,13 +296,14 @@ mdexists(SMgrRelation reln, ForkNumber forkNum)
 void
 mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
 {
+	MdfdVec*mdfd;
 	char	   *path;
 	File		fd;
 
-	if (isRedo && reln->md_fd[forkNum] != NULL)
+	if (isRedo && reln->md_num_open_segs[forkNum] > 0)
 		return;	/* created and opened already... */
 
-	Assert(reln->md_fd[forkNum] == NULL);
+	Assert(reln->md_num_open_segs[forkNum] == 0);
 
 	path = relpath(reln->smgr_rnode, forkNum);
 
@@ -334,11 +333,10 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
 
 	pfree(path);
 
-	reln->md_fd[forkNum] = _fdvec_alloc();
-
-	reln->md_fd[forkNum]->mdfd_vfd = fd;
-	reln->md_fd[forkNum]->mdfd_segno = 0;
-	reln->md_fd[forkNum]->mdfd_chain = NULL;
+	_fdvec_resize(reln, forkNum, 1);
+	mdfd = >md_seg_fds[forkNum][0];
+	mdfd->mdfd_vfd = fd;
+	mdfd->mdfd_segno = 0;
 }
 
 /*
@@ -583,8 +581,8 @@ mdopen(SMgrRelation reln, ForkNumber forknum, int behavior)
 	File		fd;
 
 	/* No work if already open */
-	if (reln->md_fd[forknum])

Re: [HACKERS] amcheck (B-Tree integrity checking tool)

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 5:06 PM, Michael Paquier
 wrote:
> Cool. I have been honestly wondering about deploying this tool as well
> to allow some of the QE tests to perform live checks of btree indexes
> as we use a bunch of them.

I'd certainly welcome that. There are Debian packages available from
the Github version of amcheck, which is otherwise practically
identical to the most recent version of the patch posted here:

https://github.com/petergeoghegan/amcheck

(This version also targets Postgres 9.4+).

> By the way, I have not looked at the patch,
> but this supports just btree, right? Wouldn't btree_check be a better
> name, or do you think that the interface you provide is generic enough
> that it could be extended in the future for gin, gist, etc.?

Yes, the idea of calling it amcheck was support for other AMs could be
added later.

Personally, I would like to make amcheck verifying the heap through a
B-Tree index as a next step. There is also a good case for the tool
directly verifying heap relations, without involving any index, but
that can come later.

-- 
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] amcheck (B-Tree integrity checking tool)

[Michael Paquier]

On Fri, Aug 19, 2016 at 2:40 AM, Peter Geoghegan  wrote:
> Heroku began a selective roll-out of amcheck yesterday. amcheck
> already found a bug in the PostGiS Geography B-Tree opclass:
> [...]
> I'll go report this to the PostGiS people.

Cool. I have been honestly wondering about deploying this tool as well
to allow some of the QE tests to perform live checks of btree indexes
as we use a bunch of them. By the way, I have not looked at the patch,
but this supports just btree, right? Wouldn't btree_check be a better
name, or do you think that the interface you provide is generic enough
that it could be extended in the future for gin, gist, etc.?
-- 
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] anyelement -> anyrange

[Corey Huinker]

I'd be happy to roll your code into the extension, and make it marked more
stable.

On Thu, Aug 18, 2016 at 2:49 PM, Jim Nasby  wrote:

> On 8/18/16 1:06 PM, Corey Huinker wrote:
>
>> You might also find some gleanable gems in:
>> https://github.com/moat/range_type_functions/blob/master/doc
>> /range_type_functions.md
>>
>
> Well crap, I searched for range stuff on PGXN before creating
> http://pgxn.org/dist/range_tools/ and the only thing that came up was
> your range_partitioning stuff, which AFAICT is unrelated.
> http://pgxn.org/dist/range_type_functions/ still doesn't show up in
> search, maybe because it's marked unstable?
>
> Rather frustrating that I've spent time creating an extension that
> duplicates your work. :(
>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
>

Re: [HACKERS] errno clobbering in reorderbuffer

[Andres Freund]

Hi,

On 2016-08-18 19:06:02 -0300, Alvaro Herrera wrote:
>   if (write(fd, rb->outbuf, ondisk->size) != ondisk->size)
>   {
> + int save_errno = errno;
> +
>   CloseTransientFile(fd);
> + errno = save_errno;
>   ereport(ERROR,
>   (errcode_for_file_access(),
>errmsg("could not write to data file for XID 
> %u: %m",

Independent of this specific case I kinda wish we had a better way to
deal with exactly this pattern. I even wonder whether having a close
variant not clobbering errno might be worthwhile.


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

Re: [HACKERS] errno clobbering in reorderbuffer

[Tom Lane]

Alvaro Herrera  writes:
> While researching a customer issue with BDR I noticed that one ereport()
> call happens after clobbering errno, leading to the wrong strerror being
> reported.  This patch fixes it by saving before calling
> CloseTransientFile and restoring afterwards.

> I also threw in a missing errcode I noticed while looking for similar
> problems in the same file.

Looks sane to me.

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] errno clobbering in reorderbuffer

[Andres Freund]

On 2016-08-18 19:06:02 -0300, Alvaro Herrera wrote:
> While researching a customer issue with BDR I noticed that one ereport()
> call happens after clobbering errno, leading to the wrong strerror being
> reported.  This patch fixes it by saving before calling
> CloseTransientFile and restoring afterwards.
> 
> I also threw in a missing errcode I noticed while looking for similar
> problems in the same file.
> 
> This is to backpatch to 9.4.

Makes sense - you're doing that or shall I?


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

Re: [HACKERS] drop src/backend/port/darwin/system.c ?

[Peter Eisentraut]

On 8/17/16 12:29 PM, Tom Lane wrote:
> Also, the early releases of OS X were rough enough that it's pretty hard
> to believe anyone is still using them anywhere (certainly the buildfarm
> isn't).  So the odds of anyone caring if we remove this file seem
> negligible.  Let's nuke it.

done

-- 
Peter Eisentraut  http://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] pg_basebackup wish list

[Peter Eisentraut]

On 7/12/16 9:55 PM, Masahiko Sawada wrote:
> And what I think is pg_baseback never remove the directory specified
> by -D option even if execution is failed. initdb command behaves so.
> I think it's helpful for backup operation.

This has been bothering me as well.

How about the attached patch as a start?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 910310b7eab88af8972906307a55e02e64618da7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 18 Aug 2016 12:00:00 -0400
Subject: [PATCH] pg_basebackup: Clean created directories on failure

Like initdb, clean up created data and xlog directories, unless the new
-n/--noclean option is specified.

Tablespace directories are not cleaned up, but a message is written
about that.
---
 doc/src/sgml/ref/pg_basebackup.sgml  | 18 +
 src/bin/pg_basebackup/pg_basebackup.c| 98 ++--
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 10 ++-
 3 files changed, 119 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 03615da..9f1eae1 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -399,6 +399,24 @@ Options
  
 
  
+  -n
+  --noclean
+  
+   
+By default, when pg_basebackup aborts with an
+error, it removes any directories it might have created before
+discovering that it cannot finish the job (for example, data directory
+and transaction log directory). This option inhibits tidying-up and is
+thus useful for debugging.
+   
+
+   
+Note that tablespace directories are not cleaned up either way.
+   
+  
+ 
+
+ 
   -P
   --progress
   
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index ed41db8..d13a9a3 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -57,6 +57,7 @@ static TablespaceList tablespace_dirs = {NULL, NULL};
 static char *xlog_dir = "";
 static char format = 'p';		/* p(lain)/t(ar) */
 static char *label = "pg_basebackup base backup";
+static bool noclean = false;
 static bool showprogress = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
@@ -68,6 +69,13 @@ static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static pg_time_t last_progress_report = 0;
 static int32 maxrate = 0;		/* no limit by default */
 
+static bool success = false;
+static bool made_new_pgdata = false;
+static bool found_existing_pgdata = false;
+static bool made_new_xlogdir = false;
+static bool found_existing_xlogdir = false;
+static bool made_tablespace_dirs = false;
+static bool found_tablespace_dirs = false;
 
 /* Progress counters */
 static uint64 totalsize;
@@ -81,6 +89,7 @@ static int	bgpipe[2] = {-1, -1};
 
 /* Handle to child process */
 static pid_t bgchild = -1;
+static bool in_log_streamer = false;
 
 /* End position for xlog streaming, empty string if unknown yet */
 static XLogRecPtr xlogendptr;
@@ -97,7 +106,7 @@ static PQExpBuffer recoveryconfcontents = NULL;
 /* Function headers */
 static void usage(void);
 static void disconnect_and_exit(int code);
-static void verify_dir_is_empty_or_create(char *dirname);
+static void verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found);
 static void progress_report(int tablespacenum, const char *filename, bool force);
 
 static void ReceiveTarFile(PGconn *conn, PGresult *res, int rownum);
@@ -114,6 +123,69 @@ static void tablespace_list_append(const char *arg);
 
 
 static void
+cleanup_directories_atexit(void)
+{
+	if (success || in_log_streamer)
+		return;
+
+	if (!noclean)
+	{
+		if (made_new_pgdata)
+		{
+			fprintf(stderr, _("%s: removing data directory \"%s\"\n"),
+	progname, basedir);
+			if (!rmtree(basedir, true))
+fprintf(stderr, _("%s: failed to remove data directory\n"),
+		progname);
+		}
+		else if (found_existing_pgdata)
+		{
+			fprintf(stderr,
+	_("%s: removing contents of data directory \"%s\"\n"),
+	progname, basedir);
+			if (!rmtree(basedir, false))
+fprintf(stderr, _("%s: failed to remove contents of data directory\n"),
+		progname);
+		}
+
+		if (made_new_xlogdir)
+		{
+			fprintf(stderr, _("%s: removing transaction log directory \"%s\"\n"),
+	progname, xlog_dir);
+			if (!rmtree(xlog_dir, true))
+fprintf(stderr, _("%s: failed to remove transaction log directory\n"),
+		progname);
+		}
+		else if (found_existing_xlogdir)
+		{
+			fprintf(stderr,
+	_("%s: removing contents of transaction log directory \"%s\"\n"),
+	progname, xlog_dir);
+			if (!rmtree(xlog_dir, false))
+fprintf(stderr, _("%s: failed to remove contents of transaction log directory\n"),
+		progname);
+		}
+	}
+	else
+	{
+		if (made_new_pgdata || found_existing_pgdata)
+			

[HACKERS] errno clobbering in reorderbuffer

[Alvaro Herrera]

While researching a customer issue with BDR I noticed that one ereport()
call happens after clobbering errno, leading to the wrong strerror being
reported.  This patch fixes it by saving before calling
CloseTransientFile and restoring afterwards.

I also threw in a missing errcode I noticed while looking for similar
problems in the same file.

This is to backpatch to 9.4.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 7ff6f9b..a6d44d5 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2135,7 +2135,10 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 
 	if (write(fd, rb->outbuf, ondisk->size) != ondisk->size)
 	{
+		int save_errno = errno;
+
 		CloseTransientFile(fd);
+		errno = save_errno;
 		ereport(ERROR,
 (errcode_for_file_access(),
  errmsg("could not write to data file for XID %u: %m",
@@ -2864,7 +2867,8 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
 	fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0);
 	if (fd < 0)
 		ereport(ERROR,
-(errmsg("could not open file \"%s\": %m", path)));
+(errcode_for_file_access(),
+ errmsg("could not open file \"%s\": %m", path)));
 
 	while (true)
 	{

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

Re: [HACKERS] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Claudio Freire]

On Thu, Aug 18, 2016 at 6:38 PM, Peter Geoghegan  wrote:
> On Thu, Aug 18, 2016 at 2:26 PM, Claudio Freire  
> wrote:
>> I see that. I could try to measure average depth to measure the impact
>> this had on fan-in.
>>
>> While it should cut it in half for narrow indexes, half of very high
>> is still high. Wide indexes, which are are the ones that would suffer
>> from poor fan-in, would feel this far less, since the overhead is
>> constant.
>
> You'll also have to consider the impact on the choice of split point,
> FWIW. There is currently leeway about what page an index tuple can
> land on, if and when it happens to be equal to the high-key. I'm not
> sure how important that is, but it's a consideration.

When I read the code, it seemed generic enough, using ItemIdGetLength
(which works on non-leaf index tuples correctly, reporting the right
size), so it should still work.

But the choice of split point may make a difference for future
insertions, so I'll look into that.


>> Even if it does have an impact, I don't see an alternative, without
>> also implementing suffix truncation. Perhaps I could try to avoid
>> adding the leaf tid header if it isn't necessary, though I would have
>> to use up the last available flag bit in t_info for that.
>
> To be clear, I'm particularly worried about painting ourselves into a
> corner, suffix-truncation-wise. It's a very common optimization, that
> we should really have.

Well, page version numbers could be used to switch between
semantically similar page formats later on. So if another format is
necessary (say, prefix compression, suffix truncation, etc), it can be
changed on-the-fly on existing indexes by checking page version
numbers and doing the proper switch.

So we won't be painting ourselves into a corner, but picking the wrong
format may indeed be a headache.

I can go the way of the flag in t_info if that's preferrable. Both
would work. It's just that it's the last flag available, and that
would be painting ourselves into a corner regarding flags. To avoid
that, the flag itself could be "has extra data" (instead of has leaf
tid), and add a whole set of flags in the extra data. That could also
help for preffix compression and suffix truncation.

>>> ISTM that the way to address this problem is with a duplicate list
>>> and/or prefix compression in leaf pages.
>>
>> Prefix compression is another one I will be looking into eventually,
>> but last time I tried it was far more invasive so I abandoned until I
>> could find a less invasive way to do it.
>
> Unfortunately, sometimes it is necessary to be very ambitious in order
> to solve a problem. The understandable and usually well-reasoned
> approach of making progress as incremental as possible occasionally
> works against contributors. It's worth considering if this is such a
> case.

I'd agree if this regressed performance considerably without the other
improvements, so I guess this will depend on the fan-in measurements.


-- 
Sent 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] add option to pg_dumpall to exclude tables from the dump

[Tom Lane]

Jim Nasby  writes:
> On 8/18/16 2:40 PM, Tom Lane wrote:
>> This seems pretty dubious to me, in particular that the identical -T
>> option will be passed willy-nilly into the pg_dump runs for every
>> database.  That seems more likely to be a foot-gun than something useful.

> I agree, but I think mandating a database name (which I suppose could be 
> *) with the specifiers would solve that issue.

Hmm, something like "-T dbname1:pattern1 -T dbname2:pattern2" ?

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] Curing plpgsql's memory leaks for statement-lifespan values

[Jim Nasby]

On 7/25/16 1:50 PM, Tom Lane wrote:

There's a glibc-dependent hack in aset.c that reports any
plpgsql-driven palloc or pfree against a context named "SPI Proc", as
well as changes in pl_comp.c so that transient junk created during initial
parsing of a plpgsql function body doesn't end up in the SPI Proc context.
(I did that just to cut the amount of noise I had to chase down.  I suppose
in large functions it might be worth adopting such a change so that that
junk can be released immediately after parsing; but I suspect for most
functions it'd just be an extra context without much gain.)


Some folks do create very large plpgsql functions, so if there's a handy 
way to estimate the size of the function (pg_proc.prosrc's varlena size 
perhaps) then it might be worth doing that for quite large functions.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 2:26 PM, Claudio Freire  wrote:
> I see that. I could try to measure average depth to measure the impact
> this had on fan-in.
>
> While it should cut it in half for narrow indexes, half of very high
> is still high. Wide indexes, which are are the ones that would suffer
> from poor fan-in, would feel this far less, since the overhead is
> constant.

You'll also have to consider the impact on the choice of split point,
FWIW. There is currently leeway about what page an index tuple can
land on, if and when it happens to be equal to the high-key. I'm not
sure how important that is, but it's a consideration.

> Even if it does have an impact, I don't see an alternative, without
> also implementing suffix truncation. Perhaps I could try to avoid
> adding the leaf tid header if it isn't necessary, though I would have
> to use up the last available flag bit in t_info for that.

To be clear, I'm particularly worried about painting ourselves into a
corner, suffix-truncation-wise. It's a very common optimization, that
we should really have.

>> ISTM that the way to address this problem is with a duplicate list
>> and/or prefix compression in leaf pages.
>
> Prefix compression is another one I will be looking into eventually,
> but last time I tried it was far more invasive so I abandoned until I
> could find a less invasive way to do it.

Unfortunately, sometimes it is necessary to be very ambitious in order
to solve a problem. The understandable and usually well-reasoned
approach of making progress as incremental as possible occasionally
works against contributors. It's worth considering if this is such a
case.

-- 
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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Claudio Freire]

On Thu, Aug 18, 2016 at 6:27 PM, Tom Lane  wrote:
> Claudio Freire  writes:
>> On Thu, Aug 18, 2016 at 6:04 PM, Kevin Grittner  wrote:
>>> Speaking of performance side effects, does this avoid O(N^2)
>>> performance on index tuple insertion with duplicate values, for all
>>> insertion orderings?  For example, does it descend directly to the
>>> right leaf page for the insert rather than starting at the front of
>>> the block of duplicate values and scanning to the right for a
>>> block with space, with a random chance to split a full block on
>>> each page it moves through?
>
>> Yes, but only on non-unique indexes.
>
> How's that work if the existing entries aren't in TID order (which they
> will not be, in a pre-existing index)?  Or are you assuming you can blow
> off on-disk compatibility of indexes?
>
> regards, tom lane

This patch does blow off on-disk compatibility, but the plan is to
re-add it later on.

A flag in the meta page would indicate whether it's a sorted index or
not, and the only way to get a sorted index would be with a reindex.
The code would have to support both for a while until whatever point
we'd figure we can drop support for old format indexes.

Since this is a sort order change I see no alternative, either the
whole index is sorted by TID or not.


-- 
Sent 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] add option to pg_dumpall to exclude tables from the dump

[Jim Nasby]

On 8/18/16 2:40 PM, Tom Lane wrote:

This seems pretty dubious to me, in particular that the identical -T
option will be passed willy-nilly into the pg_dump runs for every
database.  That seems more likely to be a foot-gun than something useful.


I agree, but I think mandating a database name (which I suppose could be 
*) with the specifiers would solve that issue.



Also, if we believe that this has a safe use-case, why only -T, and
not pg_dump's other object selectivity options?


+1.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Tom Lane]

Claudio Freire  writes:
> On Thu, Aug 18, 2016 at 6:04 PM, Kevin Grittner  wrote:
>> Speaking of performance side effects, does this avoid O(N^2)
>> performance on index tuple insertion with duplicate values, for all
>> insertion orderings?  For example, does it descend directly to the
>> right leaf page for the insert rather than starting at the front of
>> the block of duplicate values and scanning to the right for a
>> block with space, with a random chance to split a full block on
>> each page it moves through?

> Yes, but only on non-unique indexes.

How's that work if the existing entries aren't in TID order (which they
will not be, in a pre-existing index)?  Or are you assuming you can blow
off on-disk compatibility of indexes?

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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Claudio Freire]

On Thu, Aug 18, 2016 at 6:15 PM, Peter Geoghegan  wrote:
> On Thu, Aug 18, 2016 at 1:41 PM, Claudio Freire  
> wrote:
>> In fact, that's why non-leaf index tuples need a different format,
>> because while leaf index tuples contain the heap pointer already,
>> non-leaf ones contain only the downlink, not the pointer into the
>> heap. To be able to do comparisons and pick the right downlink, the
>> original heap pointer in the leaf index tuple is copied into the
>> downlink index tuple when splitting pages into an additional
>> IndexTupleData header that is prepended only to non-leaf index tuples.
>
> I think that this is a bad idea. We need to implement suffix
> truncation of internal page index tuples at some point, to make them
> contain less information from the original leaf page index tuple.
> That's an important optimization, because it increases fan-in. This
> seems like a move in the opposite direction.

I see that. I could try to measure average depth to measure the impact
this had on fan-in.

While it should cut it in half for narrow indexes, half of very high
is still high. Wide indexes, which are are the ones that would suffer
from poor fan-in, would feel this far less, since the overhead is
constant.

Even if it does have an impact, I don't see an alternative, without
also implementing suffix truncation. Perhaps I could try to avoid
adding the leaf tid header if it isn't necessary, though I would have
to use up the last available flag bit in t_info for that.

> ISTM that the way to address this problem is with a duplicate list
> and/or prefix compression in leaf pages.

Prefix compression is another one I will be looking into eventually,
but last time I tried it was far more invasive so I abandoned until I
could find a less invasive way to do it.


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

Re: [HACKERS] distinct estimate of a hard-coded VALUES list

[Tom Lane]

Jeff Janes  writes:
> So even though it knows that 6952 values have been shoved in the bottom, it
> thinks only 200 are going to come out of the aggregation.  This seems like
> a really lousy estimate.  In more complex queries than the example one
> given it leads to poor planning choices.

> Is the size of the input list not available to the planner at the point
> where it estimates the distinct size of the input list?  I'm assuming that
> if it is available to EXPLAIN than it is available to the planner.  Does it
> know how large the input list is, but just throw up its hands and use 200
> as the distinct size anyway?

It does know it, what it doesn't know is how many duplicates there are.
If we do what I think you're suggesting, which is assume the entries are
all distinct, I'm afraid we'll just move the estimation problems somewhere
else.

I recall some talk of actually running an ANALYZE-like process on the
elements of a VALUES list, but it seemed like overkill at the time and
still does.

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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Alvaro Herrera]

Claudio Freire wrote:

> Unique indexes still need to scan all duplicates to check visibility
> and will become O(N^2) there.

That scenario doesn't matter, because on unique indexes there aren't
many duplicate values anyway -- only one can be a live tuple.

-- 
Á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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Peter Geoghegan]

On Thu, Aug 18, 2016 at 1:41 PM, Claudio Freire  wrote:
> In fact, that's why non-leaf index tuples need a different format,
> because while leaf index tuples contain the heap pointer already,
> non-leaf ones contain only the downlink, not the pointer into the
> heap. To be able to do comparisons and pick the right downlink, the
> original heap pointer in the leaf index tuple is copied into the
> downlink index tuple when splitting pages into an additional
> IndexTupleData header that is prepended only to non-leaf index tuples.

I think that this is a bad idea. We need to implement suffix
truncation of internal page index tuples at some point, to make them
contain less information from the original leaf page index tuple.
That's an important optimization, because it increases fan-in. This
seems like a move in the opposite direction.

ISTM that the way to address this problem is with a duplicate list
and/or prefix compression in leaf pages.

-- 
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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Claudio Freire]

On Thu, Aug 18, 2016 at 6:04 PM, Kevin Grittner  wrote:
> On Thu, Aug 18, 2016 at 3:41 PM, Claudio Freire  
> wrote:
>
>> It also makes index scans of the form
>>
>> SELECT * FROM t WHERE some_col = some_const;
>>
>> Scan the heap in sequential order, even if some_col has low
>> cardinality (ie: the query returns lots of tuples), which is a nice
>> performance side effect.
>
> Speaking of performance side effects, does this avoid O(N^2)
> performance on index tuple insertion with duplicate values, for all
> insertion orderings?  For example, does it descend directly to the
> right leaf page for the insert rather than starting at the front of
> the block of duplicate values and scanning to the right for a
> block with space, with a random chance to split a full block on
> each page it moves through?

Yes, but only on non-unique indexes.

Unique indexes still need to scan all duplicates to check visibility
and will become O(N^2) there.

The code with the random chance to split is still there, but it should
never have a chance to run because the comparison against the heap
tuple pointer would stop it very quickly (before walking a single
offset I believe).

I thought about cleaning that up, but to make review easier I thought
I'd leave it there for now. Removing it would add a lot of diff noise.


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

Re: [HACKERS] Improving planner's checks for parallel-unsafety

[Tom Lane]

Robert Haas  writes:
> I have reviewed this and it looks good to me.  My only comment is that
> this comment is slightly confusing:

> !  * Returns the first of PROPARALLEL_UNSAFE, PROPARALLEL_RESTRICTED, or
> !  * PROPARALLEL_SAFE that can be found in the given parsetree.  We use this

> "First" might be read to mean "the first one we happen to run across"
> rather than "the earliest in list ordering".

Thanks for the review.  I'll reconsider how to phrase that --- have you
any suggestions?

regards, tom lane


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

Re: [HACKERS] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Kevin Grittner]

On Thu, Aug 18, 2016 at 3:41 PM, Claudio Freire  wrote:

> It also makes index scans of the form
>
> SELECT * FROM t WHERE some_col = some_const;
>
> Scan the heap in sequential order, even if some_col has low
> cardinality (ie: the query returns lots of tuples), which is a nice
> performance side effect.

Speaking of performance side effects, does this avoid O(N^2)
performance on index tuple insertion with duplicate values, for all
insertion orderings?  For example, does it descend directly to the
right leaf page for the insert rather than starting at the front of
the block of duplicate values and scanning to the right for a
block with space, with a random chance to split a full block on
each page it moves through?

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

[HACKERS] distinct estimate of a hard-coded VALUES list

[Jeff Janes]

I have a query which contains a where clause like:

 aid =ANY(VALUES (1),(45),(87), <6948 more>, (447))

for example:

perl -le 'print "explain (analyze) select sum(abalance) from
pgbench_accounts where aid=ANY(VALUES "; print join ",", map "($_)", sort
{$a<=>$b} map int(rand(500)), 1..6952; print ");"'  | psql


And it gives me an explain section like:

->  HashAggregate  (cost=104.28..106.28 rows=200 width=32) (actual
time=15.171..30.859 rows=6931 loops=1)
Group Key: "*VALUES*".column1
 ->  Values Scan on "*VALUES*"  (cost=0.00..86.90 rows=6952
width=32) (actual time=0.007..6.926 rows=6952 loops=1)

So even though it knows that 6952 values have been shoved in the bottom, it
thinks only 200 are going to come out of the aggregation.  This seems like
a really lousy estimate.  In more complex queries than the example one
given it leads to poor planning choices.

Is the size of the input list not available to the planner at the point
where it estimates the distinct size of the input list?  I'm assuming that
if it is available to EXPLAIN than it is available to the planner.  Does it
know how large the input list is, but just throw up its hands and use 200
as the distinct size anyway?

If I throw at the system a massively degenerate list, and it makes bad
choices by assuming the list is distinct, I have the recourse of
uniquifying the list myself before passing it in.  If I pass in a mostly
distinct list, and it makes bad choices by assuming only 200 are distinct,
I don't have much recourse aside from creating, populating, analyzing, and
then using temporary tables. Which is annoying, and causes other problems
like catalog bloat.

Is this something in the planner that could be fixed in a future version,
or is a temp table the only real solution here?

Cheers,

Jeff

Re: [HACKERS] Improving planner's checks for parallel-unsafety

[Robert Haas]

On Thu, Aug 18, 2016 at 12:39 PM, Tom Lane  wrote:
> Attached is a patch I'd fooled around with back in July but not submitted.
> The idea is that, if our initial scan of the query tree found only
> parallel-safe functions, there is no need to rescan subsets of the tree
> looking for parallel-restricted functions.  We can mechanize that by
> saving the "maximum unsafety" level in PlannerGlobal and looking aside
> at that value before conducting a check of a subset of the tree.
>
> This is not a huge win, but it's measurable.  I see about 3% overall TPS
> improvement in pgbench on repeated execution of this test query:
>
> select
>  abs(unique1) + abs(unique1),
>  abs(unique2) + abs(unique2),
>  abs(two) + abs(two),
>  abs(four) + abs(four),
>  abs(ten) + abs(ten),
>  abs(twenty) + abs(twenty),
>  abs(hundred) + abs(hundred),
>  abs(thousand) + abs(thousand),
>  abs(twothousand) + abs(twothousand),
>  abs(fivethous) + abs(fivethous),
>  abs(tenthous) + abs(tenthous),
>  abs(odd) + abs(odd),
>  abs(even) + abs(even)
> from tenk1 limit 1;
>
> This test case is admittedly a bit contrived, in that the number of
> function calls that have to be checked is high relative to both the
> planning cost and execution cost of the query.  Still, the fact that
> the difference is above the noise floor even in an end-to-end test
> says that the current method of checking functions twice is pretty
> inefficient.
>
> I'll put this in the commitfest queue.

I have reviewed this and it looks good to me.  My only comment is that
this comment is slightly confusing:

!  * Returns the first of PROPARALLEL_UNSAFE, PROPARALLEL_RESTRICTED, or
!  * PROPARALLEL_SAFE that can be found in the given parsetree.  We use this

"First" might be read to mean "the first one we happen to run across"
rather than "the earliest in list ordering".

-- 
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] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane  wrote:

> Ryan Murphy  writes:
> > On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas 
> wrote:
>  +{ /* pg_ctl command w path, properly quoted */
>  +PQExpBuffer pg_ctl_path = createPQExpBuffer();
>  +printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
>
> >> I think it's worth reducing the scope of variables when that's as
> >> simple as putting them inside a block that you have to create anyway,
> >> but I'm skeptical about the idea that one would create a block just to
> >> reduce the scope of the variables.  I don't think that's our usual
> >> practice, and I would expect the compiler to detect where the variable
> >> is referenced first and last anyway.
>
> > I enjoy adding the blocks for explicit variable scoping and for quick
> > navigation in vim (the % key navigates between matching {}'s).  But I
> want
> > to fit in with the style conventions of the project.
>
> Another point here is that code like this will look quite a bit different
> after pgindent gets done with it --- that comment will not stay where
> you put it, for example.  Some of our worst formatting messes come from
> code wherein somebody adhered to their own favorite layout style without
> any thought for how it would play with pgindent.
>
> regards, tom lane
>

Ahh, I didn't know about pgindent, but now I see it in /src/tools.  I can
run that on my code before submitting.

I found these

links 
about the style convention and will make sure my patch fits the conventions
before submitting it.

Re: [HACKERS] Making pg_hba.conf case-insensitive

[Bruce Momjian]

On Thu, Aug 18, 2016 at 03:01:48PM -0400, Peter Eisentraut wrote:
> On 8/18/16 1:59 PM, Bruce Momjian wrote:
> > o  compares words in columns that can only support keywords as
> > case-insensitive, double-quoted or not
> > 
> > o  compares words in columns that can contain user/db names or keywords
> > as case-sensitive if double-quoted, case-insensitive if not
> 
> I can maybe see the case of the second one, but the first one doesn't
> make sense to me.
> 
> We've in the past had discussions like this about whether command line
> arguments of tools should be case insensitive like SQL, and we had
> resolved that since the shell is not SQL, it shouldn't work like that.
> pg_hba.conf is also not SQL, and neither, for that matter, is
> pg_ident.conf and postgresql.conf.

OK, I am happy to remove the TODO item and see if we get new complaints.

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

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


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

Re: [HACKERS] WIP: About CMake v2

[Peter Eisentraut]

On 8/18/16 4:23 PM, Christian Convey wrote:
> What standard would you suggest for those platforms which don't have
> an obvious default version of CMake?

In the olden days, when many platforms we supported didn't come with GNU
make or other GNU tools or even a compiler, we collected a lot of
practical information about where to get these things.  You can see most
of that at
.

That's at least the spirit of things.  I wouldn't worry about this so
much right now.  Get it working first.

-- 
Peter Eisentraut  http://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] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Tom Lane]

Ryan Murphy  writes:
> On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas  wrote:
 +{ /* pg_ctl command w path, properly quoted */
 +PQExpBuffer pg_ctl_path = createPQExpBuffer();
 +printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",

>> I think it's worth reducing the scope of variables when that's as
>> simple as putting them inside a block that you have to create anyway,
>> but I'm skeptical about the idea that one would create a block just to
>> reduce the scope of the variables.  I don't think that's our usual
>> practice, and I would expect the compiler to detect where the variable
>> is referenced first and last anyway.

> I enjoy adding the blocks for explicit variable scoping and for quick
> navigation in vim (the % key navigates between matching {}'s).  But I want
> to fit in with the style conventions of the project.

Another point here is that code like this will look quite a bit different
after pgindent gets done with it --- that comment will not stay where
you put it, for example.  Some of our worst formatting messes come from
code wherein somebody adhered to their own favorite layout style without
any thought for how it would play with pgindent.

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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Claudio Freire]

On Thu, Aug 18, 2016 at 5:05 PM, Robert Haas  wrote:
> On Wed, Aug 17, 2016 at 10:54 PM, Claudio Freire  
> wrote:
>> The attached patch tries to maintain the initial status of B-Tree
>> indexes, which are created with equal-key runs in physical order,
>> during the whole life of the B-Tree, and make key-tid pairs
>> efficiently searchable in the process.
>
> I have two pretty important questions that doesn't seem to be
> addressed in your email.

I believe I addressed that in the README, but if I wasn't clear, let
me know and I'll expand there too.

Summarizing here:

> 1. How does it do that?

It adds the block number and the offset number of the index tuple's
t_tid (that is, the pointer into the heap) as a final (implicit) sort
key, as is done already in tuplesort. It just keeps doing that while
comparing keys in the B-Tree when searching the leaf page for
insertion, so the insertion point now points to the exact point where
the insertion has to happen to maintain that condition, and not just
to the first valid position within the same-key run.

In fact, that's why non-leaf index tuples need a different format,
because while leaf index tuples contain the heap pointer already,
non-leaf ones contain only the downlink, not the pointer into the
heap. To be able to do comparisons and pick the right downlink, the
original heap pointer in the leaf index tuple is copied into the
downlink index tuple when splitting pages into an additional
IndexTupleData header that is prepended only to non-leaf index tuples.

This representation is chosen to minimize code changes, such that
(itup+1) is usable as before, and also because it's reasonably
compact. But it *is* necessary to check whether it's a leaf or
non-leaf tuple to choose whether to use (itup+1) or just itup, which
is why the patch requires so many changes in so many places.

> 2. What's the benefit?

The idea came up in the discussion about WARM updates.

Basically, in various situations it is convenient to be able to find
the index tuples that point to a specific heap tuple. Without this,
doing so isn't efficient - the only way to find such index tuples is
to find the leftmost index tuple that has the same key, and walk the
index right links until the right tid is found. For non-unique
indexes, but also for unique indexes with heavy bloat, this could
involve a large amount of I/O, so it isn't efficient in several
contexts. Think vacuum and WARM updates mostly (like HOT updates, but
where only some indexes don't need updating, and some do).

Having the ability to find a heap tuple by key-ctid pair is thus
beneficial because it allows efficient implementations for those
operations. It may benefit other parts of the code, but those are the
ones that come to mind.

It also makes index scans of the form

SELECT * FROM t WHERE some_col = some_const;

Scan the heap in sequential order, even if some_col has low
cardinality (ie: the query returns lots of tuples), which is a nice
performance side 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] Fix comment in ATExecValidateConstraint

[Robert Haas]

On Thu, Aug 18, 2016 at 5:15 AM, Amit Langote
 wrote:
> On 2016/07/25 17:18, Amit Langote wrote:
>> The comment seems to have been copied from ATExecAddColumn, which says:
>>
>>  /*
>>   * If we are told not to recurse, there had better not be any
>> - * child tables; else the addition would put them out of step.
>>
>> For ATExecValidateConstraint, it should say something like:
>>
>> + * child tables; else validating the constraint would put them
>> + * out of step.
>>
>> Attached patch fixes it.
>
> I noticed that the ALTER TABLE documentation doesn't mention that VALIDATE
> CONSTRAINT will fail if ONLY is specified and there are descendant tables.
>  It only mentions that a constraint cannot be renamed unless also renamed
> in the descendant tables.
>
> Attached patch fixes that.

I did some wordsmithing on the two patches you posted to this thread.
I suggest the attached version.  What do you think?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6f51cbc..a070041 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1028,11 +1028,15 @@ ALTER TABLE ALL IN TABLESPACE name
 

 If a table has any descendant tables, it is not permitted to add,
-rename, or change the type of a column, or rename an inherited constraint
-in the parent table without doing
-the same to the descendants.  That is, ALTER TABLE ONLY
-will be rejected.  This ensures that the descendants always have
-columns matching the parent.
+rename, or change the type of a column constraint in the parent table
+without doing same to the descendants.  This ensures that the descendants
+always have columns matching the parent.  Similarly, a constraint cannot be
+renamed in the parent without also renaming it in all descendents, so that
+constraints also match between the parent and its descendents.
+Also, because selecting from the parent also selects from its descendents,
+a constraint on the parent cannot be marked valid unless it is also marked
+valid for those descendents.  In all of these cases, ALTER TABLE
+ONLY will be rejected.  

 

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..d312762 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6908,7 +6908,8 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
 
 /*
  * If we are told not to recurse, there had better not be any
- * child tables; else the addition would put them out of step.
+ * child tables, because we can't mark the constraint on the
+ * parent valid unless it is valid for all child tables.
  */
 if (!recurse)
 	ereport(ERROR,

-- 
Sent 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: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas  wrote:

> On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund  wrote:
> > On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
> >> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund 
> wrote:
> >> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
> >> >
> >> >>+{ /* pg_ctl command w path, properly quoted */
> >> >>+PQExpBuffer pg_ctl_path = createPQExpBuffer();
> >> >>+printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
> >> >>+bin_dir,
> >> >>+(strlen(bin_dir) > 0) ? DIR_SEP : ""
> >> >>+);
> >> >>+appendShellString(start_db_cmd, pg_ctl_path->data);
> >> >>+destroyPQExpBuffer(pg_ctl_path);
> >> >>+}
> >> >>
> >> >>This is not really project-style to have an independent block. Usually
> >> >>those are controlled by for, while or if.
> >> >
> >> > Besides the comment positioning I'd not say that that is against the
> usual style, there's a number of such blocks already.  Don't think it's
> necessarily needed here though...
> >>
> >> Really?  I'd remove such blocks before committing anything, or ask for
> >> them to be removed, unless there were some special reason for having
> >> them.
> >
> > Well, reducing the scope of variables *can* be such a reason, no? As I
> > said, I don't see any reason here, but in general, it's imo a reasonable
> > tool on one's belt.
>
> I think it's worth reducing the scope of variables when that's as
> simple as putting them inside a block that you have to create anyway,
> but I'm skeptical about the idea that one would create a block just to
> reduce the scope of the variables.  I don't think that's our usual
> practice, and I would expect the compiler to detect where the variable
> is referenced first and last anyway.
>
>
I'm can change my patch to take out that block.

I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s).  But I want
to fit in with the style conventions of the project.

Before I change and resubmit my patch, are there any other changes, style
or otherwise, that you all would recommend?


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

Re: [HACKERS] WIP: About CMake v2

[Tom Lane]

Christian Convey  writes:
>> well I personally think the level to meet would be that all the systems
>> on the buildfarm that can build -HEAD at the time the patch is proposed
>> for a commit should be able to build using the new system with whatever
>> cmake version is available in those by default (if it is at all).

> I see.  In other projects I've worked on, the configuration of a build
> farm has been driven by some list of platforms that were considered
> important to support.

> Is that the case here as well?  I.e., is the build-farm population
> just a convenient proxy for some other source of information regarding
> what platforms are important?

To a large extent, the buildfarm population is interesting because those
machines represent platforms for which someone is willing to put their
money where their mouth is, ie go to the effort of maintaining a buildfarm
member.  For those cases, we're likely to hear complaints if we break it.

There are also buildfarm members that are really only there as coal mine
canaries, that is, we don't want to break compatibility by accident ---
but if there were a good reason for it, we might be willing to throw those
platforms overboard.  My HPPA/HPUX dinosaur is certainly in that category,
for example.

I do not think we have a project policy about where the dividing line
is, though.

BTW, Stefan's formulation supposes that on platforms where the vendor
didn't supply any version of cmake, we can tell people to install whatever
version we want.  But that's assuming something not in evidence, namely
that cmake works on those platforms at all, in any version.  We'll have
a lot of legwork to do to find out what platforms we are risking losing.

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] synchronous_commit = remote_flush

[Robert Haas]

On Thu, Aug 18, 2016 at 12:22 AM, Thomas Munro
 wrote:
> To do something about the confusion I keep seeing about what exactly
> "on" means, I've often wished we had "remote_flush".  But it's not
> obvious how the backwards compatibility could work, ie how to keep the
> people happy who use "local" vs "on" to control syncrep, and also the
> people who use "off" vs "on" to control asynchronous commit on
> single-node systems.  Is there any sensible way to do that, or is it
> not broken and I should pipe down, or is it just far too entrenched
> and never going to change?

I don't see why we can't add "remote_flush" as a synonym for "on".  Do
you have something else in mind?

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


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

Re: [HACKERS] WIP: About CMake v2

[Christian Convey]

Hi Stefan,

>> I ask because I'm curious if/how someone in Yury's situation could
>> predict which minimum version of CMake must be supported in order for
>> his patch to be accepted.  (And if he accepts my offer to pitch in,
>> I'll actually need that particular detail.)
>
> well I personally think the level to meet would be that all the systems
> on the buildfarm that can build -HEAD at the time the patch is proposed
> for a commit should be able to build using the new system with whatever
> cmake version is available in those by default (if it is at all).

What standard would you suggest for those platforms which don't have
an obvious default version of CMake?

I assume this includes all versions of Windows and of OS X, but
perhaps I'm misinformed.

Thanks,
Christian


-- 
Sent 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: initdb: "'" for QUOTE_PATH (non-windows)

[Robert Haas]

On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund  wrote:
> On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
>> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund  wrote:
>> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier 
>> >  wrote:
>> >
>> >>+{ /* pg_ctl command w path, properly quoted */
>> >>+PQExpBuffer pg_ctl_path = createPQExpBuffer();
>> >>+printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
>> >>+bin_dir,
>> >>+(strlen(bin_dir) > 0) ? DIR_SEP : ""
>> >>+);
>> >>+appendShellString(start_db_cmd, pg_ctl_path->data);
>> >>+destroyPQExpBuffer(pg_ctl_path);
>> >>+}
>> >>
>> >>This is not really project-style to have an independent block. Usually
>> >>those are controlled by for, while or if.
>> >
>> > Besides the comment positioning I'd not say that that is against the usual 
>> > style, there's a number of such blocks already.  Don't think it's 
>> > necessarily needed here though...
>>
>> Really?  I'd remove such blocks before committing anything, or ask for
>> them to be removed, unless there were some special reason for having
>> them.
>
> Well, reducing the scope of variables *can* be such a reason, no? As I
> said, I don't see any reason here, but in general, it's imo a reasonable
> tool on one's belt.

I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables.  I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.

-- 
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] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Andres Freund]

On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund  wrote:
> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier 
> >  wrote:
> >
> >>+{ /* pg_ctl command w path, properly quoted */
> >>+PQExpBuffer pg_ctl_path = createPQExpBuffer();
> >>+printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
> >>+bin_dir,
> >>+(strlen(bin_dir) > 0) ? DIR_SEP : ""
> >>+);
> >>+appendShellString(start_db_cmd, pg_ctl_path->data);
> >>+destroyPQExpBuffer(pg_ctl_path);
> >>+}
> >>
> >>This is not really project-style to have an independent block. Usually
> >>those are controlled by for, while or if.
> >
> > Besides the comment positioning I'd not say that that is against the usual 
> > style, there's a number of such blocks already.  Don't think it's 
> > necessarily needed here though...
> 
> Really?  I'd remove such blocks before committing anything, or ask for
> them to be removed, unless there were some special reason for having
> them.

Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.


-- 
Sent 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: initdb: "'" for QUOTE_PATH (non-windows)

[Robert Haas]

On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund  wrote:
> On August 17, 2016 8:15:56 PM PDT, Michael Paquier 
>  wrote:
>
>>+{ /* pg_ctl command w path, properly quoted */
>>+PQExpBuffer pg_ctl_path = createPQExpBuffer();
>>+printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
>>+bin_dir,
>>+(strlen(bin_dir) > 0) ? DIR_SEP : ""
>>+);
>>+appendShellString(start_db_cmd, pg_ctl_path->data);
>>+destroyPQExpBuffer(pg_ctl_path);
>>+}
>>
>>This is not really project-style to have an independent block. Usually
>>those are controlled by for, while or if.
>
> Besides the comment positioning I'd not say that that is against the usual 
> style, there's a number of such blocks already.  Don't think it's necessarily 
> needed here though...

Really?  I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.

-- 
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] WIP: About CMake v2

[Andres Freund]

On 2016-08-18 15:55:20 -0400, Christian Convey wrote:
> * Allow the CMake-based build system to assume a fairly modern version
> of CMake.  (Maybe 2.8.12, or 3.0.)
> 
> * For systems where the minimum CMake version isn't readily available,
> have an alternative build system which is just a simplistic Bash
> script that naively performs a full build every time it's invoked.
> The idea being that PG contributors are mostly the people who want
> efficient rebuilds, and most/all of them could easily install that
> minimal CMake version.

The benefit cmake brings to the table, from my pov, is that it allows to
get rid of somewhat a parallel buildsystem (msvc / windows, which
sources most of its information from the makefiles). If we continue to
have two, especially if they're entirely separate, I see little benefit
in this whole endeavor.


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

Re: [HACKERS] WIP: About CMake v2

[Alvaro Herrera]

Christian Convey wrote:

> I see.  In other projects I've worked on, the configuration of a build
> farm has been driven by some list of platforms that were considered
> important to support.

Build farm members are systems that somebody has seen as interesting
enough that they deserve enough effort to set up a buildfarm member for.
So the buildfarm roster *is* that list.

-- 
Á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] [WIP] [B-Tree] Keep indexes sorted by heap physical location

[Robert Haas]

On Wed, Aug 17, 2016 at 10:54 PM, Claudio Freire  wrote:
> The attached patch tries to maintain the initial status of B-Tree
> indexes, which are created with equal-key runs in physical order,
> during the whole life of the B-Tree, and make key-tid pairs
> efficiently searchable in the process.

I have two pretty important questions that doesn't seem to be
addressed in your email.

1. How does it do that?

2. What's the benefit?

-- 
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] WIP: About CMake v2

[Christian Convey]

>
> I don't think we're interested in maintaining more build systems than we
> already are.  If cmake can replace the current MSVC tooling and
> autoconf, all in one, my impression is that we're in.  If we're
> replacing two tools we've hammered pretty well over the years with two
> tools that we haven't, I doubt we're interested.

Good to know, thanks.

- Christian


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

Re: [HACKERS] WIP: About CMake v2

[Alvaro Herrera]

Christian Convey wrote:

> * Allow the CMake-based build system to assume a fairly modern version
> of CMake.  (Maybe 2.8.12, or 3.0.)
> 
> * For systems where the minimum CMake version isn't readily available,
> have an alternative build system which is just a simplistic Bash
> script that naively performs a full build every time it's invoked.
> The idea being that PG contributors are mostly the people who want
> efficient rebuilds, and most/all of them could easily install that
> minimal CMake version.
> 
> *IF* it proved possible to write a clear, maintainable Bash script for
> that, perhaps that would eliminate most concerns about CMake not being
> well-supported on uncommon platforms / platform versions.

I think this "simplistic Bash script" is called "configure" and is
generated by autoconf from our configure.in.  I don't think the word
"simplistic" describes it very well (nor does "maintainable").

I don't think we're interested in maintaining more build systems than we
already are.  If cmake can replace the current MSVC tooling and
autoconf, all in one, my impression is that we're in.  If we're
replacing two tools we've hammered pretty well over the years with two
tools that we haven't, I doubt we're interested.

-- 
Á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] WIP: About CMake v2

[Christian Convey]

Hi Tom,

>> I ask because I'm curious if/how someone in Yury's situation could
>> predict which minimum version of CMake must be supported in order for
>> his patch to be accepted.  (And if he accepts my offer to pitch in,
>> I'll actually need that particular detail.)
>
> well I personally think the level to meet would be that all the systems
> on the buildfarm that can build -HEAD at the time the patch is proposed
> for a commit should be able to build using the new system with whatever
> cmake version is available in those by default (if it is at all).

I see.  In other projects I've worked on, the configuration of a build
farm has been driven by some list of platforms that were considered
important to support.

Is that the case here as well?  I.e., is the build-farm population
just a convenient proxy for some other source of information regarding
what platforms are important?

Apologies if my questions are so basic that I can find the answers
elsewhere.  I'll happily follow any RTFM links.

Thanks again,
Christian


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

Re: [HACKERS] WIP: About CMake v2

[Stefan Kaltenbrunner]

On 08/18/2016 09:52 PM, Alvaro Herrera wrote:
> Stefan Kaltenbrunner wrote:
>> On 08/18/2016 09:30 PM, Christian Convey wrote:
> 
>>> Yury: Would it make sense to add a call to "cmake_minimum_required" in
>>> one or more of your CMakeLists.txt files?
>>
>> it would make sense nevertheless but I dont think that 2.8.11 is old
>> enough - looking at the release information and the feature compatibily
>> matrix it would seems we should more aim at something like 2.8.0 or 2.8.3...
> 
> Last year I checked versions installable in Debian:
> https://www.postgresql.org/message-id/20150829213631.GI2912@alvherre.pgsql
> From that I would say that the maximum minimum is 2.8.2.  Not sure if
> there's any platform where 2.8.0 (released in 2009) or older would be
> necessary.

well we have for example a NetBSD 5.1 boxe (coypu) on the buildfarm that
have a software stack that is basically 2008/2009ish...
So 2.8.0-2.8.3 seems like a realistic target to me still



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] WIP: About CMake v2

[Christian Convey]

Hi Stefan,

>> Yury: Would it make sense to add a call to "cmake_minimum_required" in
>> one or more of your CMakeLists.txt files?
>
> it would make sense nevertheless but I dont think that 2.8.11 is old
> enough - looking at the release information and the feature compatibily
> matrix it would seems we should more aim at something like 2.8.0 or 2.8.3...

I'm new to PG development, so I don't know what ideas the community is
open to.  But I wonder if there's any merit to the following
approach...

* Allow the CMake-based build system to assume a fairly modern version
of CMake.  (Maybe 2.8.12, or 3.0.)

* For systems where the minimum CMake version isn't readily available,
have an alternative build system which is just a simplistic Bash
script that naively performs a full build every time it's invoked.
The idea being that PG contributors are mostly the people who want
efficient rebuilds, and most/all of them could easily install that
minimal CMake version.

*IF* it proved possible to write a clear, maintainable Bash script for
that, perhaps that would eliminate most concerns about CMake not being
well-supported on uncommon platforms / platform versions.

- Christian


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

Re: [HACKERS] WIP: About CMake v2

[Stefan Kaltenbrunner]

On 08/18/2016 09:42 PM, Christian Convey wrote:
> Hi Tom,
> 
> Thanks for that information.
> 
> Is there some document I can read that explains which platform
> versions (e.g., OpenBSD 5.3) are considered strongly supported?

well not sure we have very clear document on that - I would say that the
buildfarm is the most authoritative answer to that. So I think skimming
the buildfarm for the oldest and strangest platforms would be a good start.

> 
> I ask because I'm curious if/how someone in Yury's situation could
> predict which minimum version of CMake must be supported in order for
> his patch to be accepted.  (And if he accepts my offer to pitch in,
> I'll actually need that particular detail.)

well I personally think the level to meet would be that all the systems
on the buildfarm that can build -HEAD at the time the patch is proposed
for a commit should be able to build using the new system with whatever
cmake version is available in those by default (if it is at all).


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] WIP: About CMake v2

[Alvaro Herrera]

Stefan Kaltenbrunner wrote:
> On 08/18/2016 09:30 PM, Christian Convey wrote:

> > Yury: Would it make sense to add a call to "cmake_minimum_required" in
> > one or more of your CMakeLists.txt files?
> 
> it would make sense nevertheless but I dont think that 2.8.11 is old
> enough - looking at the release information and the feature compatibily
> matrix it would seems we should more aim at something like 2.8.0 or 2.8.3...

Last year I checked versions installable in Debian:
https://www.postgresql.org/message-id/20150829213631.GI2912@alvherre.pgsql
>From that I would say that the maximum minimum is 2.8.2.  Not sure if
there's any platform where 2.8.0 (released in 2009) or older would be
necessary.

For the record, IIRC the oldest Perl we support is something like 5.8.3
(released in 2004, bugfix release after 5.8.0 released in 2002)

-- 
Á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] anyelement -> anyrange

[David E. Wheeler]

On Aug 18, 2016, at 11:49 AM, Jim Nasby  wrote:

> Well crap, I searched for range stuff on PGXN before creating 
> http://pgxn.org/dist/range_tools/ and the only thing that came up was your 
> range_partitioning stuff, which AFAICT is unrelated. 
> http://pgxn.org/dist/range_type_functions/still doesn't show up in search, 
> maybe because it's marked unstable?

Yep. https://github.com/pgxn/pgxn-api/issues/2

David



smime.p7s
Description: S/MIME cryptographic signature

Re: [HACKERS] WIP: About CMake v2

[Christian Convey]

Hi Tom,

Thanks for that information.

Is there some document I can read that explains which platform
versions (e.g., OpenBSD 5.3) are considered strongly supported?

I ask because I'm curious if/how someone in Yury's situation could
predict which minimum version of CMake must be supported in order for
his patch to be accepted.  (And if he accepts my offer to pitch in,
I'll actually need that particular detail.)

Kind regards,
Christian


> As an additional comment, I don't see us accepting a build system
> that doesn't run on pretty old cmakes.  We have never had a policy
> of "latest and greatest is required" for *any* build support tool,
> and cmake isn't likely to be given an exception.
>
> BTW, I just noticed that cmake doesn't seem to be supplied as part of
> Apple's dev tools, at least not up to current (El Capitan) releases.
> That's going to be a rather large minus to be taken into account
> whenever we make the go/no-go decision on this.
>
> 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] WIP: About CMake v2

[Stefan Kaltenbrunner]

On 08/18/2016 09:30 PM, Christian Convey wrote:
> Hi Karl,
> 
> I'll need to let Yury answer your original question regarding the best
> way to report CMake-related bugs.
> 
> Regarding the errors you're getting...  I just looked at CMake's
> online documentation regarding your "target_compile_definitions"
> error.
> 
> From what I can tell, the "target_compile_definition" property was
> introduced in CMake 2.8.11.  It sounds like your version of CMake is
> just a little too old.

Well - "too old" is a relative term - cmake 2.8.10 was released in only
october 2012 and cmake 2.8.11 in may 2013 so it is not even 4 years old,
the oldest currently supported (though for not much longer) postgresql
release 9.1 was released in september 2011 and 9.2 was also released
before october 2012.
So while Cmake compat might only make it for v10, I dont think that we
can depend on bleeding edge version like that for our buildtools...


> 
> Regarding how one can know the required CMake version: My modus
> operandi for CMake projects in general is (1) read the project's
> how-to-build docs, and if that's not heplful, (2) hope that the
> project's CMake files contain a "cmake_minimum_required" call to give
> me a clear error message.  I didn't find any such indication in Yuri's
> files, although perhaps I missed it.
> 
> 
> Yury: Would it make sense to add a call to "cmake_minimum_required" in
> one or more of your CMakeLists.txt files?

it would make sense nevertheless but I dont think that 2.8.11 is old
enough - looking at the release information and the feature compatibily
matrix it would seems we should more aim at something like 2.8.0 or 2.8.3...


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] [PATCH] add option to pg_dumpall to exclude tables from the dump

[Tom Lane]

Juergen Hannappel  writes:
> A new option -T --exlude-table for pg_dumpall. This option is then
> passed through to the pg_dump which really does the work.
> This feature can be used to exclude large tables that are known not
> to change from a database backup dump so that only the changing parts
> of the database are dumped.

This seems pretty dubious to me, in particular that the identical -T
option will be passed willy-nilly into the pg_dump runs for every
database.  That seems more likely to be a foot-gun than something useful.

Also, if we believe that this has a safe use-case, why only -T, and
not pg_dump's other object selectivity options?

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

[HACKERS] Re: [COMMITTERS] pgsql: Fix deletion of speculatively inserted TOAST on conflict

[Alvaro Herrera]

Andres Freund wrote:

> This commit also adds a isolationtester spec test, exercising the
> relevant code path. Unfortunately 9.5 cannot handle two waiting
> sessions, and thus cannot execute this test.

This test seems to fail randomly, depending on which session is awakened
first.  Annoying ...

-- 
Á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] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Peter Eisentraut]

On 3/22/16 9:27 AM, Aleksander Alekseev wrote:
> diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
> index 28f9fb5..45aa802 100644
> --- a/src/backend/libpq/hba.c
> +++ b/src/backend/libpq/hba.c
> @@ -1008,14 +1008,9 @@ parse_hba_line(List *line, int line_num, char 
> *raw_line)
>   *cidr_slash = '\0';
>  
>   /* Get the IP address either way */
> + memset(, 0, sizeof(hints));
>   hints.ai_flags = AI_NUMERICHOST;
>   hints.ai_family = AF_UNSPEC;
> - hints.ai_socktype = 0;
> - hints.ai_protocol = 0;
> - hints.ai_addrlen = 0;
> - hints.ai_canonname = NULL;
> - hints.ai_addr = NULL;
> - hints.ai_next = NULL;
>  
>   ret = pg_getaddrinfo_all(str, NULL, , 
> _result);
>   if (ret == 0 && gai_result)

In addition to what Heikki wrote, I think the above is not necessary.

-- 
Peter Eisentraut  http://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] WIP: About CMake v2

[Christian Convey]

Hi Karl,

I'll need to let Yury answer your original question regarding the best
way to report CMake-related bugs.

Regarding the errors you're getting...  I just looked at CMake's
online documentation regarding your "target_compile_definitions"
error.

>From what I can tell, the "target_compile_definition" property was
introduced in CMake 2.8.11.  It sounds like your version of CMake is
just a little too old.

Regarding how one can know the required CMake version: My modus
operandi for CMake projects in general is (1) read the project's
how-to-build docs, and if that's not heplful, (2) hope that the
project's CMake files contain a "cmake_minimum_required" call to give
me a clear error message.  I didn't find any such indication in Yuri's
files, although perhaps I missed it.


Yury: Would it make sense to add a call to "cmake_minimum_required" in
one or more of your CMakeLists.txt files?

Kind regards,
Christian

On Thu, Aug 18, 2016 at 3:08 PM, Stefan Kaltenbrunner
 wrote:
> On 08/18/2016 08:57 PM, Christian Convey wrote:
>> Hi Stefan,
>>
>> I think I've seen similar errors when a project's CMake files assumed
>> a newer version of CMake than the one being run.
>>
>> Which version of CMake gave you those errors?  (Sorry if you provided
>> that detail and I'm just missing it.)
>
>
> % cmake --version
> cmake version 2.8.10.2
>
> a quick look in the docs does not seem to reveal any kind of "minimum"
> cmake version required - and the above is what is packaged as part of
> openbsd 5.3 (which is outdated and unsupported upstream but it is
> currently perfectly fine building all postgresql versions including
> -HEAD and serving as a buildfarm member for years)
>
>
>
> 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] WIP: About CMake v2

[Tom Lane]

Stefan Kaltenbrunner  writes:
> On 08/18/2016 08:57 PM, Christian Convey wrote:
>> Which version of CMake gave you those errors?  (Sorry if you provided
>> that detail and I'm just missing it.)

> % cmake --version
> cmake version 2.8.10.2

> a quick look in the docs does not seem to reveal any kind of "minimum"
> cmake version required - and the above is what is packaged as part of
> openbsd 5.3 (which is outdated and unsupported upstream but it is
> currently perfectly fine building all postgresql versions including
> -HEAD and serving as a buildfarm member for years)

As an additional comment, I don't see us accepting a build system
that doesn't run on pretty old cmakes.  We have never had a policy
of "latest and greatest is required" for *any* build support tool,
and cmake isn't likely to be given an exception.

BTW, I just noticed that cmake doesn't seem to be supplied as part of
Apple's dev tools, at least not up to current (El Capitan) releases.
That's going to be a rather large minus to be taken into account
whenever we make the go/no-go decision on this.

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] Use pread and pwrite instead of lseek + write and read

[Tom Lane]

Robert Haas  writes:
> Well, I think you're pointing out some things that need to be figured
> out, but I hardly think that's a good enough reason to pour cold water
> on the whole approach.

If somebody feels like doing the legwork to find out if those performance
hazards are real (which I freely concede they may not be), fine.  I'm just
saying this isn't a slam-dunk commit-it-and-move-on win.

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] Most efficient way for libPQ .. PGresult serialization

[Joshua Bay]

Thanks,
But I don't think my question was clear enough.

I already managed the connection pooling, and what I need is to serialize
the result.

If PGresult was a contiguous block, I could have just create buffer and
call memcpy for serialization, but structure of result seems much more
complicated.

So, I was asking if there is an easy way to achieve serialization

Thanks!



On Thu, Aug 18, 2016 at 1:33 AM, Tatsuo Ishii  wrote:

> > On 18 August 2016 at 10:05, Joshua Bay  wrote:
> >
> >> Hi,
> >>
> >> I was trying to implement a middleware that lies between client and
> >> postgres.
> >>
> >> So, this middleware is supposed to run query with libpq, do its job on
> >> them, and then serialize the result of query, and send it to the client
> !
> >> (client deserializes to PGresult)
> >>
> >> I could simply iterate over rows and columns but than that would be
> slow.
> >> I also found that query results consist of 3 parts (PGresult, tuples,
> data
> >> blocks).
> >>
> >> Could I please get some pointers ? :)
> >>
> >>
> > Take a look at the code for PgBouncer and PgPool-II. Both implement
> > PostgreSQL protocol proxies you could use as starting points.
>
> This one is based on Pgpool-II.
>
> https://github.com/treasure-data/prestogres
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>

Re: [HACKERS] WIP: About CMake v2

[Stefan Kaltenbrunner]

On 08/18/2016 08:57 PM, Christian Convey wrote:
> Hi Stefan,
> 
> I think I've seen similar errors when a project's CMake files assumed
> a newer version of CMake than the one being run.
> 
> Which version of CMake gave you those errors?  (Sorry if you provided
> that detail and I'm just missing it.)


% cmake --version
cmake version 2.8.10.2

a quick look in the docs does not seem to reveal any kind of "minimum"
cmake version required - and the above is what is packaged as part of
openbsd 5.3 (which is outdated and unsupported upstream but it is
currently perfectly fine building all postgresql versions including
-HEAD and serving as a buildfarm member for years)



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] Making pg_hba.conf case-insensitive

[Peter Eisentraut]

On 8/18/16 1:59 PM, Bruce Momjian wrote:
> o  compares words in columns that can only support keywords as
> case-insensitive, double-quoted or not
> 
> o  compares words in columns that can contain user/db names or keywords
> as case-sensitive if double-quoted, case-insensitive if not

I can maybe see the case of the second one, but the first one doesn't
make sense to me.

We've in the past had discussions like this about whether command line
arguments of tools should be case insensitive like SQL, and we had
resolved that since the shell is not SQL, it shouldn't work like that.
pg_hba.conf is also not SQL, and neither, for that matter, is
pg_ident.conf and postgresql.conf.

-- 
Peter Eisentraut  http://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] Making pg_hba.conf case-insensitive

[Tom Lane]

Bruce Momjian  writes:
> On Thu, Aug 18, 2016 at 02:06:39PM -0400, Tom Lane wrote:
>> Is there really enough demand for this to justify subtle breakage
>> of existing pg_hba.conf entries?  It'd probably have been fine if
>> we did it like that originally, but I think it's a bit late now.

> Well, in 2009 the discussion was whether to backpatch or not, which
> seems a long way from saying we can't change it in a major release:

That was seven years ago, which means there's now seven more years
of precedent for the way it works today; and still we've only ever
heard the one complaint.  I think documenting the gotcha more clearly
might be the right answer at this point.

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] WIP: About CMake v2

[Christian Convey]

Hi Stefan,

I think I've seen similar errors when a project's CMake files assumed
a newer version of CMake than the one being run.

Which version of CMake gave you those errors?  (Sorry if you provided
that detail and I'm just missing it.)

Kind regards,
Christian

On Thu, Aug 18, 2016 at 2:45 PM, Stefan Kaltenbrunner
 wrote:
> On 06/29/2016 06:23 PM, Yury Zhuravlev wrote:
>> Hello Hackers.
>>
>> I decided to talk about the current state of the project:
>> 1. Merge with 9.6 master. 2. plpython2, plpython3, plperl, pltcl, plsql
>> all work correctly (all tests pass).
>> 3. Works done for all contrib modules. 4. You can use gettext, .po->.mo
>> will have converted by CMake.  5. All test pass under some Linux,
>> FreeBSD, Solaris10 (on Sparc), Windows MSVC 2015. MacOSX I think not big
>> trouble too.  6. Prototype for PGXS (with MSVC support) done.
>> I think is very big progress but I came across one problem.
>> I not have access to many OS and platforms. For each platform need tests
>> and small fixes. I can't develop and give guarantee without it.
>>
>> I think this is very important work which makes it easier further
>> support Postgres but I can not do everything himself. It's physically
>> impossible.
>>
>> I think without community support I can't do significantly more.
>>
>> Current version you can get here:
>> https://github.com/stalkerg/postgres_cmake
>
> hmm how do you actually want reports on how it works?
>
> I just played with it on spoonbill (OpenBSD 5.3/sparc64) and got this:
>
> CMake Error at CMakeLists.txt:1250 (file):
>   file does not recognize sub-command GENERATE
>
>
> CMake Error at src/port/CMakeLists.txt:156 (target_compile_definitions):
>   Unknown CMake command "target_compile_definitions".
>
>
> -- Configuring incomplete, errors occurred!
>
>
> there is also a ton of stuff like:
>
>
> Make Error: Internal CMake error, TryCompile generation of cmake failed
> -- Looking for opterr - not found
> -- Looking for optreset
> CMake Error at CMakeLists.txt:10 (ADD_EXECUTABLE):
>   Target "cmTryCompileExec3458204847" links to item " m" which has
> leading or
>   trailing whitespace.  This is now an error according to policy CMP0004.
>
>
> CMake Error: Internal CMake error, TryCompile generation of cmake failed
> -- Looking for optreset - not found
> -- Looking for fseeko
> CMake Error at CMakeLists.txt:10 (ADD_EXECUTABLE):
>   Target "cmTryCompileExec2628321539" links to item " m" which has
> leading or
>   trailing whitespace.  This is now an error according to policy CMP0004.
>
>
> CMake Error: Internal CMake error, TryCompile generation of cmake failed
>
>
> which I have no idea whether they are an actual problem or just
> "configure" noise
>
>
>
>
>
> Stefan
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


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

Re: [HACKERS] anyelement -> anyrange

[Jim Nasby]

On 8/18/16 1:06 PM, Corey Huinker wrote:

You might also find some gleanable gems in:
https://github.com/moat/range_type_functions/blob/master/doc/range_type_functions.md


Well crap, I searched for range stuff on PGXN before creating 
http://pgxn.org/dist/range_tools/ and the only thing that came up was 
your range_partitioning stuff, which AFAICT is unrelated. 
http://pgxn.org/dist/range_type_functions/ still doesn't show up in 
search, maybe because it's marked unstable?


Rather frustrating that I've spent time creating an extension that 
duplicates your work. :(

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
Sent 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 -c to rsync commands on SR tutorial wiki page

[Jim Nasby]

On 8/18/16 1:31 PM, Stephen Frost wrote:

To have proper incremental backups done requires a lot more than just
throwing "-c" into the rsyncs.  For my 2c, I'm at the point where I'd
prefer we discourage people from using rsync, cp, or generally try to
set up their own hand-rolled backup system with PG.  Those examples
simply encourage poor setups that risk losing data and introducing
corruption.


I agree (and the only reason I was looking at that page is because I 
client was following the directions on it!)


So, what would this mean in terms of the wiki? Should we nuke that page 
and point at something in the docs? Something else?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] Making pg_hba.conf case-insensitive

[Bruce Momjian]

On Thu, Aug 18, 2016 at 02:06:39PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > I was looking at this TODO item from 2009:
> > https://www.postgresql.org/message-id/4AA7B197.70002%40usit.uio.no
> > I have implemented this in the attached patch.  It does two things:
> 
> Is there really enough demand for this to justify subtle breakage
> of existing pg_hba.conf entries?  It'd probably have been fine if
> we did it like that originally, but I think it's a bit late now.

Well, in 2009 the discussion was whether to backpatch or not, which
seems a long way from saying we can't change it in a major release:

https://www.postgresql.org/message-id/1336.1252509807%40sss.pgh.pa.us
https://www.postgresql.org/message-id/5060.1252523065%40sss.pgh.pa.us

It is certainly something we can discuss.

> Also, why strcasecmp and not pg_strcasecmp?  The former is going
> to induce misbehavior in e.g. Turkish locales.

OK, changed, and attached.

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

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index ca262d9..66a54ef
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** hostnossl  databasePostgreSQL database.
+Non-keyword values are compared as lower-case, unless double-quoted.
 Multiple database names can be supplied by separating them with
 commas.  A separate file containing database names can be specified by
 preceding the file name with @.
*** hostnossl  database@.
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
new file mode 100644
index 1b4bbce..809cb9a
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
*** typedef struct check_network_data
*** 61,68 
  } check_network_data;
  
  
! #define token_is_keyword(t, k)	(!t->quoted && strcmp(t->string, k) == 0)
! #define token_matches(t, k)  (strcmp(t->string, k) == 0)
  
  /*
   * A single string token lexed from the HBA config file, together with whether
--- 61,69 
  } check_network_data;
  
  
! #define token_is_keyword(t, k)	(!t->quoted && pg_strcasecmp(t->string, k) == 0)
! /* case-insensitive comparison for unquoted strings */
! #define token_matches(t, k)  (t->quoted ? (strcmp(t->string, k) == 0) : (pg_strcasecmp(t->string, k) == 0))
  
  /*
   * A single string token lexed from the HBA config file, together with whether
*** parse_hba_line(List *line, int line_num,
*** 851,857 
  		return NULL;
  	}
  	token = linitial(tokens);
! 	if (strcmp(token->string, "local") == 0)
  	{
  #ifdef HAVE_UNIX_SOCKETS
  		parsedline->conntype = ctLocal;
--- 852,858 
  		return NULL;
  	}
  	token = linitial(tokens);
! 	if (pg_strcasecmp(token->string, "local") == 0)
  	{
  #ifdef HAVE_UNIX_SOCKETS
  		parsedline->conntype = ctLocal;
*** parse_hba_line(List *line, int line_num,
*** 864,872 
  		return NULL;
  #endif
  	}
! 	else if (strcmp(token->string, "host") == 0 ||
! 			 strcmp(token->string, "hostssl") == 0 ||
! 			 strcmp(token->string, "hostnossl") == 0)
  	{
  
  		if (token->string[4] == 's')	/* "hostssl" */
--- 865,873 
  		return NULL;
  #endif
  	}
! 	else if (pg_strcasecmp(token->string, "host") == 0 ||
! 			 pg_strcasecmp(token->string, "hostssl") == 0 ||
! 			 pg_strcasecmp(token->string, "hostnossl") == 0)
  	{
  
  		if (token->string[4] == 's')	/* "hostssl" */
*** parse_hba_line(List *line, int line_num,
*** 1149,1177 
  	token = linitial(tokens);
  
  	unsupauth = NULL;
! 	if (strcmp(token->string, "trust") == 0)
  		parsedline->auth_method = uaTrust;
! 	else if (strcmp(token->string, "ident") == 0)
  		parsedline->auth_method = uaIdent;
! 	else if (strcmp(token->string, "peer") == 0)
  		parsedline->auth_method = uaPeer;
! 	else if (strcmp(token->string, "password") == 0)
  		parsedline->auth_method = uaPassword;
! 	else if (strcmp(token->string, "gss") == 0)
  #ifdef ENABLE_GSS
  		parsedline->auth_method = uaGSS;
  #else
  		unsupauth = "gss";
  #endif
! 	else if (strcmp(token->string, "sspi") == 0)
  #ifdef ENABLE_SSPI
  		parsedline->auth_method = uaSSPI;
  #else
  		unsupauth = "sspi";
  #endif
! 	else if (strcmp(token->string, "reject") == 0)
  		parsedline->auth_method = uaReject;
! 	else if (strcmp(token->string, "md5") == 0)
  	{
  		if (Db_user_namespace)
  		{
--- 1150,1178 
  	token = linitial(tokens);
  
  	unsupauth = NULL;
! 	if (pg_strcasecmp(token->string, "trust") == 0)
  		parsedline->auth_method = uaTrust;
! 	else if (pg_strcasecmp(token->string, "ident") == 0)
  		parsedline->auth_method = uaIdent;
! 	else if (pg_strcasecmp(token->string, "peer") == 0)
  		parsedline->auth_method = uaPeer;
! 	else if (pg_strcasecmp(token->string, 

Re: [HACKERS] WIP: About CMake v2

[Stefan Kaltenbrunner]

On 06/29/2016 06:23 PM, Yury Zhuravlev wrote:
> Hello Hackers.
> 
> I decided to talk about the current state of the project:
> 1. Merge with 9.6 master. 2. plpython2, plpython3, plperl, pltcl, plsql
> all work correctly (all tests pass).
> 3. Works done for all contrib modules. 4. You can use gettext, .po->.mo
> will have converted by CMake.  5. All test pass under some Linux,
> FreeBSD, Solaris10 (on Sparc), Windows MSVC 2015. MacOSX I think not big
> trouble too.  6. Prototype for PGXS (with MSVC support) done.
> I think is very big progress but I came across one problem.
> I not have access to many OS and platforms. For each platform need tests
> and small fixes. I can't develop and give guarantee without it.
> 
> I think this is very important work which makes it easier further
> support Postgres but I can not do everything himself. It's physically
> impossible.
> 
> I think without community support I can't do significantly more.
> 
> Current version you can get here:
> https://github.com/stalkerg/postgres_cmake

hmm how do you actually want reports on how it works?

I just played with it on spoonbill (OpenBSD 5.3/sparc64) and got this:

CMake Error at CMakeLists.txt:1250 (file):
  file does not recognize sub-command GENERATE


CMake Error at src/port/CMakeLists.txt:156 (target_compile_definitions):
  Unknown CMake command "target_compile_definitions".


-- Configuring incomplete, errors occurred!


there is also a ton of stuff like:


Make Error: Internal CMake error, TryCompile generation of cmake failed
-- Looking for opterr - not found
-- Looking for optreset
CMake Error at CMakeLists.txt:10 (ADD_EXECUTABLE):
  Target "cmTryCompileExec3458204847" links to item " m" which has
leading or
  trailing whitespace.  This is now an error according to policy CMP0004.


CMake Error: Internal CMake error, TryCompile generation of cmake failed
-- Looking for optreset - not found
-- Looking for fseeko
CMake Error at CMakeLists.txt:10 (ADD_EXECUTABLE):
  Target "cmTryCompileExec2628321539" links to item " m" which has
leading or
  trailing whitespace.  This is now an error according to policy CMP0004.


CMake Error: Internal CMake error, TryCompile generation of cmake failed


which I have no idea whether they are an actual problem or just
"configure" noise





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] [Patch] New psql prompt substitution %r (m = master, r = replica)

[Jim Nasby]

On 8/18/16 10:05 AM, Alvaro Herrera wrote:

Bash has PROMPT_COMMAND, which is executed prior to issuing each prompt.
What about introducing such a feature into psql?  Then the \gset command
you had in your first post could be used to set the variable correctly
just before each prompt.


As someone that has the backend PID in his prompt... +1. Or 
alternatively, a command that is just run when the connection changes 
instead of every command...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] Bug in to_timestamp().

[Robert Haas]

On Wed, Aug 17, 2016 at 10:35 AM, amul sul  wrote:
> Hmm. I haven't really looked into the code, but with applying both patches it 
> looks precisely imitate Oracle's behaviour. Thanks.

This is good to hear, but for us to consider applying something like
this, somebody would need to look into the code pretty deeply.

-- 
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] Use pread and pwrite instead of lseek + write and read

[Robert Haas]

On Wed, Aug 17, 2016 at 3:11 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I don't understand why you think this would create non-trivial
>> portability issues.
>
> The patch as submitted breaks entirely on platforms without pread/pwrite.
> Yes, we can add a configure test and some shim functions to fix that,
> but the argument that it makes the code shorter will get a lot weaker
> once we do.
>
> I agree that adding such functions is pretty trivial, but there are
> reasons to think there are other hazards that are less trivial:
>
> First, a self-contained shim function will necessarily do an lseek every
> time, which means performance will get *worse* not better on non-pread
> platforms.  And yes, the existing logic to avoid lseeks fires often enough
> to be worthwhile, particularly in seqscans.
>
> Second, I wonder whether this will break any kernel's readahead detection.
> I wouldn't be too surprised if successive reads (not preads) without
> intervening lseeks are needed to trigger readahead on at least some
> platforms.  So there's a potential, both on platforms with pread and those
> without, for this to completely destroy seqscan performance, with
> penalties very far exceeding what we might save by avoiding some kernel
> calls.
>
> I'd be more excited about this if the claimed improvement were more than
> 1.5%, but you know as well as I do that that's barely above the noise
> floor for most performance measurements.  I'm left wondering why bother,
> and why take any risk of de-optimizing on some platforms.

Well, I think you're pointing out some things that need to be figured
out, but I hardly think that's a good enough reason to pour cold water
on the whole approach.  The number of lseeks we issue on many
workloads is absolutely appalling, and I don't think there's any
reason at all to assume that a 1.5% gain is as good as it gets.  Even
if it is, a 1% speedup on a benchmark where the noise is 5-10% is just
as much of a speedup as a 1% speedup on a benchmark on a benchmark
where the noise is 0.1%.  Faster is faster, and 1% improvements are
not so numerous that we can afford to ignore them when they pop up.

-- 
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] [PATCH] bigint txids vs 'xid' type, new txid_recent(bigint) => xid

[Jim Nasby]

On 8/18/16 5:46 AM, Amit Kapila wrote:

I think there is a value in exposing such a variant which takes bigint
and internally converts it to xid.  I am not sure the semantics for


I think that's a bad idea because you have the exact same problems we 
have now: bigint is signed, epoch is not.



the other proposal txid_recent() is equivalent to what we have for
txid_current().  One thing which is different is that txid_current()
allocates a new transaction if there is currently none.  If you


Right, and it would be nice to be able to tell if an XID has been 
assigned to your transaction or not; something you currently can't do.



plainly want to convert it to 32 bit xid, then may be txid_convert or
something like that is more suitable.


I think we need to either add real types for handling XID/epoch/TXID or 
finally create uint types. It's *way* too easy to screw things up the 
way they are today.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
Sent 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 -c to rsync commands on SR tutorial wiki page

[Stephen Frost]

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> On 8/17/16 9:46 PM, Stephen Frost wrote:
> >* Jim Nasby (jim.na...@bluetreble.com) wrote:
> >>> https://wiki.postgresql.org/wiki/Binary_Replication_Tutorial does
> >>> not specify -c for any of the rsync commands. That's maybe safe for
> >>> WAL, but I don't think it's safe for any of the other uses, right?
> >>> I'd like someone to confirm before I just change the page... my
> >>> intention is to just stick -c in all the commands.
> >-c is only relevant when you are doing an incremental copy, but on a
> >quick look, all those rsync commands appear to be doing full copies?
> >
> >You would want -c if you were taking a backup and then doing an update
> >of it using rsync. or something along those lines, as you can't really
> >trust rsync's time/size based comparison as it only has a 1 second level
> >granularity.
> 
> I don't think it's any great leap for someone to think they can use
> those commands incrementally. It's certainly one of the first things
> you think of when using rsync. AFAIK there's no downside at all to
> using -c when it is a brand new copy, so I'm thinking we should just
> put it in there, especially considering what the potential downside
> is.

To have proper incremental backups done requires a lot more than just
throwing "-c" into the rsyncs.  For my 2c, I'm at the point where I'd
prefer we discourage people from using rsync, cp, or generally try to
set up their own hand-rolled backup system with PG.  Those examples
simply encourage poor setups that risk losing data and introducing
corruption.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] synchronous_commit = remote_flush

[Jim Nasby]

On 8/17/16 11:22 PM, Thomas Munro wrote:

Hi hackers,

To do something about the confusion I keep seeing about what exactly
"on" means, I've often wished we had "remote_flush".  But it's not
obvious how the backwards compatibility could work, ie how to keep the
people happy who use "local" vs "on" to control syncrep, and also the
people who use "off" vs "on" to control asynchronous commit on
single-node systems.  Is there any sensible way to do that, or is it
not broken and I should pipe down, or is it just far too entrenched
and never going to change?


I'm wondering if we've hit the point where trying to put all of this in 
a single GUC is a bad idea... changing that probably means a config 
compatibility break, but I don't think that's necessarily a bad thing at 
this point...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
Sent 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 -c to rsync commands on SR tutorial wiki page

[Jim Nasby]

On 8/17/16 9:46 PM, Stephen Frost wrote:

* Jim Nasby (jim.na...@bluetreble.com) wrote:

> https://wiki.postgresql.org/wiki/Binary_Replication_Tutorial does
> not specify -c for any of the rsync commands. That's maybe safe for
> WAL, but I don't think it's safe for any of the other uses, right?
> I'd like someone to confirm before I just change the page... my
> intention is to just stick -c in all the commands.

-c is only relevant when you are doing an incremental copy, but on a
quick look, all those rsync commands appear to be doing full copies?

You would want -c if you were taking a backup and then doing an update
of it using rsync. or something along those lines, as you can't really
trust rsync's time/size based comparison as it only has a 1 second level
granularity.


I don't think it's any great leap for someone to think they can use 
those commands incrementally. It's certainly one of the first things you 
think of when using rsync. AFAIK there's no downside at all to using -c 
when it is a brand new copy, so I'm thinking we should just put it in 
there, especially considering what the potential downside is.



--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] anyelement -> anyrange

[Corey Huinker]

On Tue, Aug 16, 2016 at 9:29 PM, Tom Lane  wrote:

> Jim Nasby  writes:
> > I can't think of any reason you'd want two different range types on a
> > single element type.
>
> We would not have built it that way if there were not clear use-cases.
> An easy example is you might want both a continuous timestamp range
> and one that is quantized to hour boundaries.  Primarily what the
> range type brings in besides the element type is a canonicalization
> function; and we can't guess which one you want.
>
> 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
>


Jim,

I wrote a routine that fishes in the dictionary for a suitable range type:
https://github.com/moat/range_partitioning/blob/master/sql/range_partitioning.sql#L459-L485

Obviously, it has the problems when the number of suitable ranges <> 1 as
mentioned by Tom.

You might also find some gleanable gems in:
https://github.com/moat/range_type_functions/blob/master/doc/range_type_functions.md

Re: [HACKERS] Making pg_hba.conf case-insensitive

[Tom Lane]

Bruce Momjian  writes:
> I was looking at this TODO item from 2009:
>   https://www.postgresql.org/message-id/4AA7B197.70002%40usit.uio.no
> I have implemented this in the attached patch.  It does two things:

Is there really enough demand for this to justify subtle breakage
of existing pg_hba.conf entries?  It'd probably have been fine if
we did it like that originally, but I think it's a bit late now.

Also, why strcasecmp and not pg_strcasecmp?  The former is going
to induce misbehavior in e.g. Turkish locales.

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

[HACKERS] Making pg_hba.conf case-insensitive

[Bruce Momjian]

I was looking at this TODO item from 2009:

https://www.postgresql.org/message-id/4AA7B197.70002%40usit.uio.no

I have implemented this in the attached patch.  It does two things:

o  compares words in columns that can only support keywords as
case-insensitive, double-quoted or not

o  compares words in columns that can contain user/db names or keywords
as case-sensitive if double-quoted, case-insensitive if not

Here is the 'local' line we install during initdb, and a newly
identically-behaving line:

# TYPE  DATABASEUSERADDRESS METHOD
local   all all trust
"LOCAL" ALL ALL TRUST

This 9.6 line:

local   Testall trust

would have to be represented in PG 10 as:

local   "Test"  all trust

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

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index ca262d9..66a54ef
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** hostnossl  databasePostgreSQL database.
+Non-keyword values are compared as lower-case, unless double-quoted.
 Multiple database names can be supplied by separating them with
 commas.  A separate file containing database names can be specified by
 preceding the file name with @.
*** hostnossl  database@.
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
new file mode 100644
index 1b4bbce..fc8d5a0
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
*** typedef struct check_network_data
*** 61,68 
  } check_network_data;
  
  
! #define token_is_keyword(t, k)	(!t->quoted && strcmp(t->string, k) == 0)
! #define token_matches(t, k)  (strcmp(t->string, k) == 0)
  
  /*
   * A single string token lexed from the HBA config file, together with whether
--- 61,69 
  } check_network_data;
  
  
! #define token_is_keyword(t, k)	(!t->quoted && strcasecmp(t->string, k) == 0)
! /* case-insensitive comparison for unquoted strings */
! #define token_matches(t, k)  (t->quoted ? (strcmp(t->string, k) == 0) : (strcasecmp(t->string, k) == 0))
  
  /*
   * A single string token lexed from the HBA config file, together with whether
*** parse_hba_line(List *line, int line_num,
*** 851,857 
  		return NULL;
  	}
  	token = linitial(tokens);
! 	if (strcmp(token->string, "local") == 0)
  	{
  #ifdef HAVE_UNIX_SOCKETS
  		parsedline->conntype = ctLocal;
--- 852,858 
  		return NULL;
  	}
  	token = linitial(tokens);
! 	if (strcasecmp(token->string, "local") == 0)
  	{
  #ifdef HAVE_UNIX_SOCKETS
  		parsedline->conntype = ctLocal;
*** parse_hba_line(List *line, int line_num,
*** 864,872 
  		return NULL;
  #endif
  	}
! 	else if (strcmp(token->string, "host") == 0 ||
! 			 strcmp(token->string, "hostssl") == 0 ||
! 			 strcmp(token->string, "hostnossl") == 0)
  	{
  
  		if (token->string[4] == 's')	/* "hostssl" */
--- 865,873 
  		return NULL;
  #endif
  	}
! 	else if (strcasecmp(token->string, "host") == 0 ||
! 			 strcasecmp(token->string, "hostssl") == 0 ||
! 			 strcasecmp(token->string, "hostnossl") == 0)
  	{
  
  		if (token->string[4] == 's')	/* "hostssl" */
*** parse_hba_line(List *line, int line_num,
*** 1149,1177 
  	token = linitial(tokens);
  
  	unsupauth = NULL;
! 	if (strcmp(token->string, "trust") == 0)
  		parsedline->auth_method = uaTrust;
! 	else if (strcmp(token->string, "ident") == 0)
  		parsedline->auth_method = uaIdent;
! 	else if (strcmp(token->string, "peer") == 0)
  		parsedline->auth_method = uaPeer;
! 	else if (strcmp(token->string, "password") == 0)
  		parsedline->auth_method = uaPassword;
! 	else if (strcmp(token->string, "gss") == 0)
  #ifdef ENABLE_GSS
  		parsedline->auth_method = uaGSS;
  #else
  		unsupauth = "gss";
  #endif
! 	else if (strcmp(token->string, "sspi") == 0)
  #ifdef ENABLE_SSPI
  		parsedline->auth_method = uaSSPI;
  #else
  		unsupauth = "sspi";
  #endif
! 	else if (strcmp(token->string, "reject") == 0)
  		parsedline->auth_method = uaReject;
! 	else if (strcmp(token->string, "md5") == 0)
  	{
  		if (Db_user_namespace)
  		{
--- 1150,1178 
  	token = linitial(tokens);
  
  	unsupauth = NULL;
! 	if (strcasecmp(token->string, "trust") == 0)
  		parsedline->auth_method = uaTrust;
! 	else if (strcasecmp(token->string, "ident") == 0)
  		parsedline->auth_method = uaIdent;
! 	else if (strcasecmp(token->string, "peer") == 0)
  		parsedline->auth_method = uaPeer;
! 	else if (strcasecmp(token->string, "password") == 0)
  		parsedline->auth_method = 

[HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Heikki Linnakangas]

On 03/22/2016 03:27 PM, Aleksander Alekseev wrote:

diff --git a/src/backend/commands/tablespace.c 
b/src/backend/commands/tablespace.c
index 1ff5728..a10c078 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -669,6 +669,11 @@ destroy_tablespace_directories(Oid tablespaceoid, bool 
redo)
char   *subfile;
struct stat st;

+   /*
+* Prevents MemorySanitizer's "use-of-uninitialized-value" warning
+*/
+   memset(, 0, sizeof(st));
+
linkloc_with_version_dir = psprintf("pg_tblspc/%u/%s", tablespaceoid,

TABLESPACE_VERSION_DIRECTORY);


Why does MemorySanitizer complain about that? Calling stat(2) is 
supposed to fill in all the fields we look at, right?



diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 924bebb..498e7bd 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -330,6 +330,7 @@ AddCatcacheInvalidationMessage(InvalidationListHeader *hdr,
SharedInvalidationMessage msg;

Assert(id < CHAR_MAX);
+   memset(, 0, sizeof(msg));
msg.cc.id = (int8) id;
msg.cc.dbId = dbId;
msg.cc.hashValue = hashValue;


Right after this, we have:


/*
 * Define padding bytes in SharedInvalidationMessage structs to be
 * defined. Otherwise the sinvaladt.c ringbuffer, which is accessed by
 * multiple processes, will cause spurious valgrind warnings about
 * undefined memory being used. That's because valgrind remembers the
 * undefined bytes from the last local process's store, not realizing 
that
 * another process has written since, filling the previously 
uninitialized
 * bytes
 */
VALGRIND_MAKE_MEM_DEFINED(, sizeof(msg));


Do we need both?



diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index d26991e..46ab8a2 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -850,7 +850,7 @@ AllocSetAlloc(MemoryContext context, Size size)
blksize <<= 1;

/* Try to allocate it */
-   block = (AllocBlock) malloc(blksize);
+   block = (AllocBlock) calloc(1, blksize);

/*
 * We could be asking for pretty big blocks here, so cope if 
malloc
@@ -861,7 +861,7 @@ AllocSetAlloc(MemoryContext context, Size size)
blksize >>= 1;
if (blksize < required_size)
break;
-   block = (AllocBlock) malloc(blksize);
+   block = (AllocBlock) calloc(1, blksize);
}

if (block == NULL)


I think this goes too far. You're zeroing all palloc'd memory, even if 
it's going to be passed to palloc0(), and zeroed there. It might even 
silence legitimate warnings, if there's code somewhere that does 
palloc(), and accesses some of it before initializing. Plus it's a 
performance hit.



diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index e7826a4..4bbd4d2 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -1022,6 +1022,11 @@ test_atomic_uint64(void)
uint64  expected;
int i;

+   /*
+* Prevents MemorySanitizer's "use-of-uninitialized-value" warning
+*/
+   memset(, 0, sizeof(var));
+
pg_atomic_init_u64(, 0);

if (pg_atomic_read_u64() != 0)


What's going on here? Surely pg_atomic_init_u64() should initialize the 
value?


- 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] WIP: About CMake v2

[Christian Convey]

Hi Yury,

I'm interested in helping with your CMake effort.  I don't have any
experience contributing to PG, but I do have some free time at the
moment.  Please let me know if I can help.

I have an intermediate level of experience with CMake, Python, and
Bash scripting.  My native environment is Mint 17.3 (i.e., Ubuntu
15.10), but I'd be happy to create additional VM's as needed.  I'm
also happy to look into bugs on other systems (VMS, etc.) if I can get
SSH access.

Kind regards,
Christian Convey


On Fri, Jul 1, 2016 at 5:26 AM, Dmitry Maslyuk  wrote:
> Hi!
>
> I think, we need simple configure script generator for backward
> compatibility and easy using this build system.
> Try playing with cmake build system under Win2008+MinGW. I plan to write
> perl script for automatic build this
> with depends.
>
> On 29.06.2016 19:23, Yury Zhuravlev wrote:
>>
>> Hello Hackers.
>>
>> I decided to talk about the current state of the project:
>> 1. Merge with 9.6 master. 2. plpython2, plpython3, plperl, pltcl, plsql
>> all work correctly (all tests pass).
>> 3. Works done for all contrib modules. 4. You can use gettext, .po->.mo
>> will have converted by CMake.  5. All test pass under some Linux, FreeBSD,
>> Solaris10 (on Sparc), Windows MSVC 2015. MacOSX I think not big trouble too.
>> 6. Prototype for PGXS (with MSVC support) done.
>> I think is very big progress but I came across one problem.
>> I not have access to many OS and platforms. For each platform need tests
>> and small fixes. I can't develop and give guarantee without it.
>>
>> I think this is very important work which makes it easier further support
>> Postgres but I can not do everything himself. It's physically impossible.
>>
>> I think without community support I can't do significantly more.
>>
>> Current version you can get here:
>> https://github.com/stalkerg/postgres_cmake
>>
>> Thanks!
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


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

Re: [HACKERS] amcheck (B-Tree integrity checking tool)

[Peter Geoghegan]

On Sat, Mar 12, 2016 at 12:38 PM, Peter Geoghegan  wrote:
> Only insofar as it helps diagnose the underlying issue, when it is a
> more subtle issue. Actually fixing the index is almost certainly a
> REINDEX. Once you're into the messy business of diagnosing a
> problematic opclass, you have to be an expert, and tweaking amcheck
> for your requirements (i.e. rebuilding from source) becomes
> reasonable. Part of the reason that the code is so heavily commented
> is to make it hackable, because I do not feel optimistic that I can
> get an expert-orientated interface right, but I still want to make the
> tool as useful as possible to experts.

Heroku began a selective roll-out of amcheck yesterday. amcheck
already found a bug in the PostGiS Geography B-Tree opclass:

https://github.com/postgis/postgis/blob/svn-trunk/postgis/geography_btree.c#L260

The issue is not that PG_RETURN_BOOL() is used here (that's just a
problem of style). Rather, the issue is that the Geography opclass
support function 1 considers an "empty geometry" equal to all other
possible values (values not limited to other empty geometries). This
breaks the transitive law, which nbtree requires and relies on.

I'll go report this to the PostGiS people.

-- 
Peter Geoghegan


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

[HACKERS] Marginal cleanup in regex code: remove typedef "celt"

[Tom Lane]

The regex library used to have a notion of a "collating element"
that was distinct from a "character", but Henry Spencer never
actually implemented support for multi-character collating elements,
and the Tcl crew ripped out most of the stubs for it years ago.
The only thing left that distinguished the "celt" typedef from the
"chr" typedef was that "celt" was supposed to also be able to hold
the not-a-character "NOCELT" value.  I noticed however that the
NOCELT macro is no longer used anywhere, which means we could remove
it as well as the separate typedef, and just deal in chrs.  This
simplifies matters and also removes a trap for the unwary, in that
celt is signed while chr is not, so comparisons could mean different
things.  There's no bug there today because we restrict CHR_MAX to
be less than INT_MAX, but I think there may have been such bugs before
we did that, and there could be again if anyone ever decides to fool
with the range of chr.

The attached patch removes celt and NOCELT, and also removes unnecessary
casts to "chr" (some of them were already unnecessary, probably as a
result of the earlier celt-support-removal changes).

Comments?  Anyone feel like actually reviewing this, or shall I just
push it?

regards, tom lane

diff --git a/src/backend/regex/regc_lex.c b/src/backend/regex/regc_lex.c
index f62ec7d..cd34c8a 100644
*** a/src/backend/regex/regc_lex.c
--- b/src/backend/regex/regc_lex.c
*** lexescape(struct vars * v)
*** 870,876 
  			if (v->now == save || ((int) c > 0 && (int) c <= v->nsubexp))
  			{
  NOTE(REG_UBACKREF);
! RETV(BACKREF, (chr) c);
  			}
  			/* oops, doesn't look like it's a backref after all... */
  			v->now = save;
--- 870,876 
  			if (v->now == save || ((int) c > 0 && (int) c <= v->nsubexp))
  			{
  NOTE(REG_UBACKREF);
! RETV(BACKREF, c);
  			}
  			/* oops, doesn't look like it's a backref after all... */
  			v->now = save;
*** lexdigits(struct vars * v,
*** 986,995 
   */
  static int		/* 1 normal, 0 failure */
  brenext(struct vars * v,
! 		chr pc)
  {
- 	chr			c = (chr) pc;
- 
  	switch (c)
  	{
  		case CHR('*'):
--- 986,993 
   */
  static int		/* 1 normal, 0 failure */
  brenext(struct vars * v,
! 		chr c)
  {
  	switch (c)
  	{
  		case CHR('*'):
*** chrnamed(struct vars * v,
*** 1153,1159 
  		 const chr *endp,		/* just past end of name */
  		 chr lastresort)		/* what to return if name lookup fails */
  {
! 	celt		c;
  	int			errsave;
  	int			e;
  	struct cvec *cv;
--- 1151,1157 
  		 const chr *endp,		/* just past end of name */
  		 chr lastresort)		/* what to return if name lookup fails */
  {
! 	chr			c;
  	int			errsave;
  	int			e;
  	struct cvec *cv;
*** chrnamed(struct vars * v,
*** 1165,1174 
  	v->err = errsave;
  
  	if (e != 0)
! 		return (chr) lastresort;
  
  	cv = range(v, c, c, 0);
  	if (cv->nchrs == 0)
! 		return (chr) lastresort;
  	return cv->chrs[0];
  }
--- 1163,1172 
  	v->err = errsave;
  
  	if (e != 0)
! 		return lastresort;
  
  	cv = range(v, c, c, 0);
  	if (cv->nchrs == 0)
! 		return lastresort;
  	return cv->chrs[0];
  }
diff --git a/src/backend/regex/regc_locale.c b/src/backend/regex/regc_locale.c
index 4fe6292..399de02 100644
*** a/src/backend/regex/regc_locale.c
--- b/src/backend/regex/regc_locale.c
*** static const struct cname
*** 361,369 
  
  
  /*
!  * element - map collating-element name to celt
   */
! static celt
  element(struct vars * v,		/* context */
  		const chr *startp,		/* points to start of name */
  		const chr *endp)		/* points just past end of name */
--- 361,369 
  
  
  /*
!  * element - map collating-element name to chr
   */
! static chr
  element(struct vars * v,		/* context */
  		const chr *startp,		/* points to start of name */
  		const chr *endp)		/* points just past end of name */
*** element(struct vars * v,		/* context */
*** 401,413 
   */
  static struct cvec *
  range(struct vars * v,			/* context */
! 	  celt a,	/* range start */
! 	  celt b,	/* range end, might equal a */
  	  int cases)/* case-independent? */
  {
  	int			nchrs;
  	struct cvec *cv;
! 	celt		c,
  cc;
  
  	if (a != b && !before(a, b))
--- 401,413 
   */
  static struct cvec *
  range(struct vars * v,			/* context */
! 	  chr a,	/* range start */
! 	  chr b,	/* range end, might equal a */
  	  int cases)/* case-independent? */
  {
  	int			nchrs;
  	struct cvec *cv;
! 	chr			c,
  cc;
  
  	if (a != b && !before(a, b))
*** range(struct vars * v,			/* context */
*** 444,450 
  
  	for (c = a; c <= b; c++)
  	{
! 		cc = pg_wc_tolower((chr) c);
  		if (cc != c &&
  			(before(cc, a) || before(b, cc)))
  		{
--- 444,450 
  
  	for (c = a; c <= b; c++)
  	{
! 		cc = pg_wc_tolower(c);
  		if (cc != c &&
  			(before(cc, a) || before(b, cc)))
  		{
*** range(struct vars * v,			/* context */
*** 

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Heikki Linnakangas]

On 08/18/2016 03:45 PM, Michael Paquier wrote:

On Thu, Aug 18, 2016 at 9:28 PM, Heikki Linnakangas  wrote:

Let's take the opportunity and also move src/backend/libpq/ip.c and md5.c
into src/common. It would be weird to have sha.c in src/common, but md5.c in
src/backend/libpq. Looking at ip.c, it could be split into two: some of the
functions in ip.c are clearly not needed in the client, like enumerating all
interfaces.


It would be definitely better to do all that before even moving sha.c.


Agreed.


For the current ip.c, I don't have a better idea than putting in
src/common/ip.c the set of routines used by both the frontend and
backend, and have fe_ip.c the new file that has the frontend-only
things. Need a patch?


Yes, please. I don't think there's anything there that's needed by only 
the frontend, but some of the functions are needed by only the backend. 
So I think we'll end up with src/common/ip.c, and 
src/backend/libpq/be-ip.c. (Not sure about those names, pick something 
that makes sense, given what's left in the files.)


- Heikki



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

[HACKERS] Improving planner's checks for parallel-unsafety

[Tom Lane]

Attached is a patch I'd fooled around with back in July but not submitted.
The idea is that, if our initial scan of the query tree found only
parallel-safe functions, there is no need to rescan subsets of the tree
looking for parallel-restricted functions.  We can mechanize that by
saving the "maximum unsafety" level in PlannerGlobal and looking aside
at that value before conducting a check of a subset of the tree.

This is not a huge win, but it's measurable.  I see about 3% overall TPS
improvement in pgbench on repeated execution of this test query:

select
 abs(unique1) + abs(unique1),
 abs(unique2) + abs(unique2),
 abs(two) + abs(two),
 abs(four) + abs(four),
 abs(ten) + abs(ten),
 abs(twenty) + abs(twenty),
 abs(hundred) + abs(hundred),
 abs(thousand) + abs(thousand),
 abs(twothousand) + abs(twothousand),
 abs(fivethous) + abs(fivethous),
 abs(tenthous) + abs(tenthous),
 abs(odd) + abs(odd),
 abs(even) + abs(even)
from tenk1 limit 1;

This test case is admittedly a bit contrived, in that the number of
function calls that have to be checked is high relative to both the
planning cost and execution cost of the query.  Still, the fact that
the difference is above the noise floor even in an end-to-end test
says that the current method of checking functions twice is pretty
inefficient.

I'll put this in the commitfest queue.

regards, tom lane

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 1fab807..50019f4 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*** _outPlannerGlobal(StringInfo str, const 
*** 2029,2034 
--- 2029,2035 
  	WRITE_BOOL_FIELD(dependsOnRole);
  	WRITE_BOOL_FIELD(parallelModeOK);
  	WRITE_BOOL_FIELD(parallelModeNeeded);
+ 	WRITE_CHAR_FIELD(maxParallelHazard);
  }
  
  static void
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 88d833a..365161c 100644
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
*** static void set_plain_rel_size(PlannerIn
*** 78,84 
  static void create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel);
  static void set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
  		  RangeTblEntry *rte);
! static bool function_rte_parallel_ok(RangeTblEntry *rte);
  static void set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
  	   RangeTblEntry *rte);
  static void set_tablesample_rel_size(PlannerInfo *root, RelOptInfo *rel,
--- 78,84 
  static void create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel);
  static void set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
  		  RangeTblEntry *rte);
! static bool function_rte_parallel_ok(PlannerInfo *root, RangeTblEntry *rte);
  static void set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
  	   RangeTblEntry *rte);
  static void set_tablesample_rel_size(PlannerInfo *root, RelOptInfo *rel,
*** set_rel_consider_parallel(PlannerInfo *r
*** 542,549 
  
  if (proparallel != PROPARALLEL_SAFE)
  	return;
! if (has_parallel_hazard((Node *) rte->tablesample->args,
! 		false))
  	return;
  			}
  
--- 542,548 
  
  if (proparallel != PROPARALLEL_SAFE)
  	return;
! if (!is_parallel_safe(root, (Node *) rte->tablesample->args))
  	return;
  			}
  
*** set_rel_consider_parallel(PlannerInfo *r
*** 596,602 
  
  		case RTE_FUNCTION:
  			/* Check for parallel-restricted functions. */
! 			if (!function_rte_parallel_ok(rte))
  return;
  			break;
  
--- 595,601 
  
  		case RTE_FUNCTION:
  			/* Check for parallel-restricted functions. */
! 			if (!function_rte_parallel_ok(root, rte))
  return;
  			break;
  
*** set_rel_consider_parallel(PlannerInfo *r
*** 629,642 
  	 * outer join clauses work correctly.  It would likely break equivalence
  	 * classes, too.
  	 */
! 	if (has_parallel_hazard((Node *) rel->baserestrictinfo, false))
  		return;
  
  	/*
  	 * Likewise, if the relation's outputs are not parallel-safe, give up.
  	 * (Usually, they're just Vars, but sometimes they're not.)
  	 */
! 	if (has_parallel_hazard((Node *) rel->reltarget->exprs, false))
  		return;
  
  	/* We have a winner. */
--- 628,641 
  	 * outer join clauses work correctly.  It would likely break equivalence
  	 * classes, too.
  	 */
! 	if (!is_parallel_safe(root, (Node *) rel->baserestrictinfo))
  		return;
  
  	/*
  	 * Likewise, if the relation's outputs are not parallel-safe, give up.
  	 * (Usually, they're just Vars, but sometimes they're not.)
  	 */
! 	if (!is_parallel_safe(root, (Node *) rel->reltarget->exprs))
  		return;
  
  	/* We have a winner. */
*** set_rel_consider_parallel(PlannerInfo *r
*** 647,653 
   * Check whether a function RTE is scanning something parallel-restricted.
   */
  static bool
! function_rte_parallel_ok(RangeTblEntry *rte)
  {
 

Re: [HACKERS] Pluggable storage

[Andres Freund]

On 2016-08-18 08:58:11 +0100, Simon Riggs wrote:
> On 16 August 2016 at 19:46, Andres Freund  wrote:
> > On 2016-08-15 12:02:18 -0400, Robert Haas wrote:
> >> Thanks for taking a stab at this.  I'd like to throw out a few concerns.
> >>
> >> One, I'm worried that adding an additional layer of pointer-jumping is
> >> going to slow things down and make Andres' work to speed up the
> >> executor more difficult.  I don't know that there is a problem there,
> >> and if there is a problem I don't know what to do about it, but I
> >> think it's something we need to consider.
> >
> > I'm quite concerned about that as well.
> 
> This objection would apply to all other proposals as well, FDW etc..

Not really. The place you draw your boundary significantly influences
where and how much of a price you pay.  Having another indirection
inside HeapTuple - which is accessed in many many places, is something
different from having a seqscan equivalent, which returns you a batch of
already deformed tuples in array form.  In the latter case there's one
additional indirection per batch of tuples, in the former there's many
for each tuple.


> Do you see some way to add flexibility yet without adding a branch
> point in the code?

I'm not even saying that the approach of doing the indirection inside
the HeapTuple replacement is a no-go, just that it concerns me.  I do
think that working on only lugging arround values/isnull arrays is
something that I could see working better, if some problems are
addressed beforehand.

Greetings,

Andres Freund


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