Re: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Robert Haas 
On Fri, Jan 6, 2017 at 10:18 AM, Tom Lane  wrote:
>> I don't think there are a lto of people who use dead tree editions anymore,
>> but they certainly do exist. A lot of people use the PDFs though,
>> particularly for offline reading or loading them in ebook readers. So it
>> still has to be workable there.
>
> PDFs do have hyperlinks, so that in itself isn't an argument for keeping
> the dead-tree-friendly approach.  However, I've noticed some variation
> among tools in whether a PDF hyperlink is visibly distinct, or whether
> you have to mouse over it to find out that it would take you somewhere.
> Not sure if that's enough of a usability fail to argue for keeping the
> old way.

Personally, I wouldn't sweat it.

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

2017-01-10 Thread Tom Lane 
Robert Haas  writes:
> I'm not meaning to be funny or sarcastic or disrespectful when I say
> that I think C is the best possible language for PostgreSQL.  It works
> great, and we've got a ton of investment in making it work.

Yeah.  There's certainly a whole lot of path dependency in that statement
--- if you were starting to write Postgres from scratch today, you would
very likely choose some other language.  But given where we are, there's
just not a lot of attraction in trying to convert to another language.

As other people noted, the one path that might possibly make sense is
a gradual upgrade to C++.  But getting past the exceptions issue is a
pretty high bar that we'd have to clear before we could do much in
that direction; and it's not obvious that C++ would offer enough benefit
to be worth it.  Most of us would rather spend our time on new features
or performance improvements, not fighting with a language changeover.

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] Off-by-one oddity in minval for decreasing sequences

2017-01-10 Thread Robert Haas 
On Fri, Jan 6, 2017 at 2:15 PM, Daniel Verite  wrote:
> When testing the patch at https://commitfest.postgresql.org/12/768/
> ("sequence data type" by Peter E.), I notice that there's a preexisting
> oddity in the fact that sequences created with a negative increment
> in current releases initialize the minval to -(2^63)+1 instead of -2^63,
> the actual lowest value for a bigint.
>
> postgres=# CREATE SEQUENCE s INCREMENT BY -1;
> CREATE SEQUENCE
>
> postgres=# SELECT seqmin,seqmin+pow(2::numeric,63)
>FROM pg_sequence where seqrelid='s'::regclass;
> seqmin|  ?column?
> --+
>  -9223372036854775807 | 1.
>
> But it's still possible to set it to -2^63 manually either by
> altering the sequence or by specifying it explicitly at CREATE time
> with CREATE SEQUENCE s MINVALUE -9223372036854775808
> so it's inconsistent with the potential argument that we couldn't
> store this value for some reason.
>
> postgres=# ALTER SEQUENCE s minvalue -9223372036854775808;
> ALTER SEQUENCE
> postgres=# select seqmin,seqmin+pow(2::numeric,63)
>from pg_sequence where seqrelid='s'::regclass;
> seqmin|  ?column?
> --+
>  -9223372036854775808 | 0.
>
>
> The defaults comes from these definitions, in include/pg_config_manual.h
>
> /*
>  * Set the upper and lower bounds of sequence values.
>  */
> #define SEQ_MAXVALUEPG_INT64_MAX
> #define SEQ_MINVALUE(-SEQ_MAXVALUE)
>
> with no comment as to why SEQ_MINVALUE is not PG_INT64_MIN.
>
> When using other types than bigint, Peter's patch fixes the inconsistency
> but also makes it worse by ISTM applying the rule that the lowest value
> is forbidden for int2 and int4 in addition to int8.
>
> I'd like to suggest that we don't do that starting with HEAD, by
> setting seqmin to the real minimum of the supported range, because
> wasting that particular value seems silly and a hazard if
> someone wants to use a sequence to store any integer
> as opposed to just calling nextval().

This seems like a sensible argument to me, but maybe somebody's got a
contrary viewpoint?

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

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 10:55 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I'm not meaning to be funny or sarcastic or disrespectful when I say
>> that I think C is the best possible language for PostgreSQL.  It works
>> great, and we've got a ton of investment in making it work.
>
> Yeah.  There's certainly a whole lot of path dependency in that statement
> --- if you were starting to write Postgres from scratch today, you would
> very likely choose some other language.  But given where we are, there's
> just not a lot of attraction in trying to convert to another language.

Really?  What language would you pick in a vacuum?  The Linux kernel
is written in C, too, for pretty much the same reasons: it's the
canonical language for system software.  I don't deny that there may
be some newer languages out which could theoretically be used and work
well, but do any of them really have a development community and user
base around them that is robust enough that we'd want to be downstream
of it?  C has its annoyances, but its sheer pervasiveness is an
extremely appealing feature.

-- 
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: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Kevin Grittner 
On Tue, Jan 10, 2017 at 9:58 AM, Tom Lane  wrote:

> whether to continue using "see section m.n"-type cross-references

For my part, I have a preference for including the section name
with the link text, although if it took much work to add it (rather
than being the new default) I might question whether the benefit
was worth the effort of adding it.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Block level parallel vacuum WIP

2017-01-10 Thread Claudio Freire 
On Tue, Jan 10, 2017 at 6:42 AM, Masahiko Sawada  wrote:
> Attached result of performance test with scale factor = 500 and the
> test script I used. I measured each test at four times and plot
> average of last three execution times to sf_500.png file. When table
> has index, vacuum execution time is smallest when number of index and
> parallel degree is same.

It does seem from those results that parallel heap scans aren't paying
off, and in fact are hurting.

It could be your I/O that's at odds with the parallel degree settings
rather than the approach (ie: your I/O system can't handle that many
parallel scans), but in any case it does warrant a few more tests.

I'd suggest you try to:

1. Disable parallel lazy vacuum, leave parallel index scans
2. Limit parallel degree to number of indexes, leaving parallel lazy
vacuum enabled
3. Cap lazy vacuum parallel degree by effective_io_concurrency, and
index scan parallel degree to number of indexes

And compare against your earlier test results.

I suspect 1 could be the winner, but 3 has a chance too (if e_i_c is
properly set up for your I/O system).


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

Re: [HACKERS] Block level parallel vacuum WIP

2017-01-10 Thread Claudio Freire 
On Tue, Jan 10, 2017 at 6:42 AM, Masahiko Sawada  wrote:
>> Does this work negate the other work to allow VACUUM to use >
>> 1GB memory?
>
> Partly yes. Because memory space for dead TIDs needs to be allocated
> in DSM before vacuum worker launches, parallel lazy vacuum cannot use
> such a variable amount of memory as that work does. But in
> non-parallel lazy vacuum, that work would be effective. We might be
> able to do similar thing using DSA but I'm not sure that is better.

I think it would work well with DSA as well.

Just instead of having a single segment list, you'd have one per worker.

Since workers work on disjoint tid sets, that shouldn't pose a problem.

The segment list can be joined together later rather efficiently
(simple logical joining of the segment pointer arrays) for the index
scan phases.


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

Re: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 10:58 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Jan 6, 2017 at 10:18 AM, Tom Lane  wrote:
 I don't think there are a lto of people who use dead tree editions anymore,
 but they certainly do exist. A lot of people use the PDFs though,
 particularly for offline reading or loading them in ebook readers. So it
 still has to be workable there.
>
>>> PDFs do have hyperlinks, so that in itself isn't an argument for keeping
>>> the dead-tree-friendly approach.  However, I've noticed some variation
>>> among tools in whether a PDF hyperlink is visibly distinct, or whether
>>> you have to mouse over it to find out that it would take you somewhere.
>>> Not sure if that's enough of a usability fail to argue for keeping the
>>> old way.
>
>> Personally, I wouldn't sweat it.
>
> Um ... are you expressing an opinion on the question at hand (ie, whether
> to continue using "see section m.n"-type cross-references), and if so
> in which direction?

Not exactly.  I'm saying that, in deciding that underlying question,
we should assume that PDF readers will do something sensible with
links.  I think most do, and those that don't will presumably
eventually be fixed so that they do.  I might revise that opinion if
several people show up and say "I use PDF reader X and it displays
links in a dumb way and always will but I love it anyway", though.

Personally, I think that if the doc toolchain changeover changed the
way xrefs render - and it seems that it did - that's a bug that ought
to be fixed, or the whole thing should be reverted.  We have no
agreement on that change, and a lot of existing markup that was
written with the old way in mind.  Or at least Peter ought to put some
work into reviewing existing usages and cleaning up things that don't
look right any more.  I don't think "(see Section 10.20, Blah Blah)"
is entirely horrible compared to "(see Section 10.20)" but there are
lots of place that use other phrasing, like "This is further described
in Section 10.20, which also explains how to frob your wug." and if
those places now read "This is further described in Section 10.20,
Using Wug Frobbing, which also explains how to frob your wug.", that's
not so good.

-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

2017-01-10 Thread David Fetter 
On Mon, Jan 09, 2017 at 07:52:11PM -0300, Alvaro Herrera wrote:
> David Fetter wrote:
> 
> > +   if (query->commandType == CMD_UPDATE || query->commandType == 
> > CMD_DELETE)
> > +   {
> > +   /* Make sure there's something to look at. */
> > +   Assert(query->jointree != NULL);
> > +   if (query->jointree->quals == NULL)
> > +   ereport(ERROR,
> > +   (errcode(ERRCODE_SYNTAX_ERROR),
> > +errmsg("%s requires a WHERE clause 
> > when the require_where hook is enabled.",
> > +query->commandType == 
> > CMD_UPDATE ? "UPDATE" : "DELETE"),
> > +errhint("To %s all rows, use \"WHERE 
> > true\" or similar.",
> > +query->commandType == 
> > CMD_UPDATE ? "update" : "delete")));
> > +   }
> 
> Per my earlier comment, I think this should use
> ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED instead.

Fixed.

> I think this should say "the \"require_hook\" extension" rather than
> use the term "hook".

Fixed.

> (There are two or three translatability rules violations in this
> snippet,

Based on the hints in the docs docs around translation, I've
refactored this a bit.

> but since this is an extension and those are not translatable, I
> won't say elaborate further.)

"Not translatable," or "not currently translated?"

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
commit 42f50cb8fa9848bbbc6776bcea03293a6b28b2d4
Author: Alvaro Herrera 
Date:   Tue Jan 10 11:41:13 2017 -0300

Fix overflow check in StringInfo; add missing casts

A few thinkos I introduced in fa2fa9955280.  Also, amend a similarly
broken comment.

Report by Daniel Vérité.
Authors: Daniel Vérité, Álvaro Herrera
Discussion: 
https://postgr.es/m/1706e85e-60d2-494e-8a64-9af1e1b21...@manitou-mail.org

diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index bdc204e..3eee49b 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -313,19 +313,20 @@ enlargeStringInfo(StringInfo str, int needed)
 * for efficiency, double the buffer size each time it overflows.
 * Actually, we might need to more than double it if 'needed' is big...
 */
-   newlen = 2 * str->maxlen;
-   while (needed > newlen)
+   newlen = 2 * (Size) str->maxlen;
+   while ((Size) needed > newlen)
newlen = 2 * newlen;
 
/*
-* Clamp to the limit in case we went past it.  Note we are assuming 
here
-* that limit <= INT_MAX/2, else the above loop could overflow.  We will
-* still have newlen >= needed.
+* Clamp to the limit in case we went past it.  (We used to depend on
+* limit <= INT32_MAX/2, to avoid overflow in the loop above; we no 
longer
+* depend on that, but if "needed" and str->maxlen ever become wider, we
+* will need similar caution here.)  We will still have newlen >= 
needed.
 */
if (newlen > limit)
newlen = limit;
 
-   str->data = (char *) repalloc_huge(str->data, (Size) newlen);
+   str->data = (char *) repalloc_huge(str->data, newlen);
 
str->maxlen = newlen;
 }

-- 
Sent 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] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS

2017-01-10 Thread Matheus de Oliveira 
On Mon, Jan 9, 2017 at 10:58 AM, Ashutosh Sharma 
wrote:

> > Also, should I add translations for that error message in other
> languages (I
> > can do that without help of tools for pt_BR) or is that a latter process
> in
> > the releasing?
> >
>
> I think you should add it but i am not sure when it is done.
>
>
I'll ask one of the guys who work with pt_BR translations (I know him in
person).



> > Other than that, I added a few regression tests (similar to others used
> for
> > ADP), and patched the documentation (my English is not that good, so I'm
> > open to suggestions). Anything else I forgot?
>
> You have forgot to change the description section of "ADP". In the
> description section you need to mention that privileges for schemas
> too can be altered along with other database objects.


Oh... Indeed an oversight, thanks for pointing that out.



> Other than that,
> I feel the patch looks good and has no bug.


Attached a rebased version and with the docs update pointed by Ashutosh
Sharma.

Best regards,
-- 
Matheus de Oliveira
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 04064d3..e3363f8 100644
*** a/doc/src/sgml/ref/alter_default_privileges.sgml
--- b/doc/src/sgml/ref/alter_default_privileges.sgml
***
*** 46,51  GRANT { USAGE | ALL [ PRIVILEGES ] }
--- 46,55 
  ON TYPES
  TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
  
+ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ 
  REVOKE [ GRANT OPTION FOR ]
  { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
  [, ...] | ALL [ PRIVILEGES ] }
***
*** 71,76  REVOKE [ GRANT OPTION FOR ]
--- 75,86 
  ON TYPES
  FROM { [ GROUP ] role_name | PUBLIC } [, ...]
  [ CASCADE | RESTRICT ]
+ 
+ REVOKE [ GRANT OPTION FOR ]
+ { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
  
   
  
***
*** 81,88  REVOKE [ GRANT OPTION FOR ]
 ALTER DEFAULT PRIVILEGES allows you to set the privileges
 that will be applied to objects created in the future.  (It does not
 affect privileges assigned to already-existing objects.)  Currently,
!only the privileges for tables (including views and foreign tables),
!sequences, functions, and types (including domains) can be altered.

  

--- 91,99 
 ALTER DEFAULT PRIVILEGES allows you to set the privileges
 that will be applied to objects created in the future.  (It does not
 affect privileges assigned to already-existing objects.)  Currently,
!only the privileges for schemas, tables (including views and foreign
!tables), sequences, functions, and types (including domains) can be
!altered.

  

***
*** 125,130  REVOKE [ GRANT OPTION FOR ]
--- 136,143 
are altered for objects later created in that schema.
If IN SCHEMA is omitted, the global default privileges
are altered.
+   IN SCHEMA is not allowed when using ON SCHEMAS
+   as schemas can't be nested.
   
  
 
diff --git a/src/backend/catalog/aclchk.c b/src/backeindex 7803d0d..9d64ab6 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 950,955  ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
--- 950,959 
  			all_privileges = ACL_ALL_RIGHTS_TYPE;
  			errormsg = gettext_noop("invalid privilege type %s for type");
  			break;
+ 		case ACL_OBJECT_NAMESPACE:
+ 			all_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ 			errormsg = gettext_noop("invalid privilege type %s for schema");
+ 			break;
  		default:
  			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
   (int) action->objtype);
***
*** 1137,1142  SetDefaultACL(InternalDefaultACL *iacls)
--- 1141,1156 
  this_privileges = ACL_ALL_RIGHTS_TYPE;
  			break;
  
+ 		case ACL_OBJECT_NAMESPACE:
+ 			if (OidIsValid(iacls->nspid))
+ ereport(ERROR,
+ 		(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ 		 errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS")));
+ 			objtype = DEFACLOBJ_NAMESPACE;
+ 			if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+ this_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ 			break;
+ 
  		default:
  			elog(ERROR, "unrecognized objtype: %d",
   (int) iacls->objtype);
***
*** 1363,1368  RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
--- 1377,1385 
  			case DEFACLOBJ_TYPE:
  iacls.objtype = ACL_OBJECT_TYPE;
  break;
+ 			case DEFACLOBJ_NAMESPACE:
+ iacls.objtype = ACL_OBJECT_NAMESPACE;
+ break;
  			default:
  /* Shouldn't get here */
  elog(ERROR, "unexpected default ACL type: %d",
***

Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

2017-01-10 Thread Alvaro Herrera 
Alvaro Herrera wrote:
> Daniel Verite wrote:
> 
> > My tests are OK too but I see an issue with the code in
> > enlargeStringInfo(), regarding integer overflow.
> > The bit of comment that says:
> > 
> >   Note we are assuming here that limit <= INT_MAX/2, else the above
> >   loop could overflow.
> > 
> > is obsolete, it's now INT_MAX instead of INT_MAX/2.
> 
> I would keep this comment but use UINT_MAX/2 instead.

I rephrased the comment instead.  As you say, the current code no longer
depends on that, but we will depend on something similar when we enlarge
the other variables.

With that, pushed and I hope this is closed for good.

If you or anyone else want to revisit things so that pg10 can load
even larger tuples, be my guest.  There are quite a few places that will
need to be touched, though (in particular, as I recall, the FE/BE
protocol.)

-- 
Álvaro Herrerahttps://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] background sessions

2017-01-10 Thread Robert Haas 
On Thu, Dec 29, 2016 at 5:18 PM, Peter Eisentraut
 wrote:
> For additional entertainment, I include patches that integrate
> background sessions into dblink.  So dblink can open a connection to a
> background session, and then you can use the existing dblink functions
> to send queries, read results, etc.  People use dblink to make
> self-connections to get autonomous subsessions, so this would directly
> address that use case.  The 0001 patch is some prerequisite refactoring
> to remove an ugly macro mess, which is useful independent of this.  0002
> is the actual patch.

Would that constitute a complete replacement for pg_background?

-- 
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: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Tom Lane 
Robert Haas  writes:
> On Fri, Jan 6, 2017 at 10:18 AM, Tom Lane  wrote:
>>> I don't think there are a lto of people who use dead tree editions anymore,
>>> but they certainly do exist. A lot of people use the PDFs though,
>>> particularly for offline reading or loading them in ebook readers. So it
>>> still has to be workable there.

>> PDFs do have hyperlinks, so that in itself isn't an argument for keeping
>> the dead-tree-friendly approach.  However, I've noticed some variation
>> among tools in whether a PDF hyperlink is visibly distinct, or whether
>> you have to mouse over it to find out that it would take you somewhere.
>> Not sure if that's enough of a usability fail to argue for keeping the
>> old way.

> Personally, I wouldn't sweat it.

Um ... are you expressing an opinion on the question at hand (ie, whether
to continue using "see section m.n"-type cross-references), and if so
in which direction?

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

2017-01-10 Thread Kevin Grittner 
On Tue, Jan 10, 2017 at 7:48 AM, Robert Haas  wrote:

> I'm not meaning to be funny or sarcastic or disrespectful when I say
> that I think C is the best possible language for PostgreSQL.  It works
> great, and we've got a ton of investment in making it work.  I can't
> see why we'd want to start converting even a part of the code to
> something else.  Perhaps it seems like a good idea from 10,000 feet,
> but in practice I believe it would be fraught with difficulties - and
> if it injected even a few additional instructions into hot code paths,
> it would be a performance loser.

It strikes me that exactly the set of functions that Joel is
suggesting could be written in another language is the set where
the declarations in the .h files could be considered for
replacement with static inline functions, potentially giving a
significant performance boost which would not be available if they
were instead converted to another language.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Placement of InvokeObjectPostAlterHook calls

2017-01-10 Thread Robert Haas 
On Fri, Jan 6, 2017 at 12:53 PM, Tom Lane  wrote:
> While reviewing Etsuro-san's patch to force replanning after FDW option
> changes, I noticed that there is a great lack of consistency about where
> InvokeObjectPostAlterHook calls have been placed relative to other actions
> such as forced relcache invals.  I wonder exactly what expectations a
> post-alter hook function could have about cache coherency, or indeed if
> there's any clarity at all about what such a hook might do.  For instance,
> it looks to me like the hook would generally need to do a
> CommandCounterIncrement in order to be able to "see" the update it's being
> called for, and I'm unsure that that would be safe at every call site.
> Is that supposed to be allowed, or are we expecting that object access
> hooks are only going to do open-loop actions that don't rely on the
> details of the change?

I remember working pretty hard to make this consistent when that code
went in.  In particular, I think the rule I tried to follow was to
place the hooks just after the code that injects dependencies.  I
don't know whether the inconsistencies you're seeing are due to (1)
that being a poor rule of thumb, (2) that rule of thumb not being
consistently followed at the time the original patch was committed, or
(3) subsequent drift.

> I suppose this fits in well with our grand tradition of not documenting
> hooks at all, but for a set of hooks as invasive as these are, I think
> we ought to do better.

I would personally be in favor of documenting all of our hooks,
including these.  That's a lot of work and I don't have time to do it
real soon, but I think it's a worthwhile goal and I'd be willing to
contribute to the effort.  I think they ought to be documented in the
SGML documentation near other things that pertain to server
extensibility.  However, I'm pretty sure you've shot down these kinds
of ideas in the past.  If you've revised your opinion, swell.

-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

2017-01-10 Thread David Fetter 
On Tue, Jan 10, 2017 at 08:31:47AM -0800, David Fetter wrote:
> On Mon, Jan 09, 2017 at 07:52:11PM -0300, Alvaro Herrera wrote:
> > David Fetter wrote:
> > 
> > > + if (query->commandType == CMD_UPDATE || query->commandType == 
> > > CMD_DELETE)
> > > + {
> > > + /* Make sure there's something to look at. */
> > > + Assert(query->jointree != NULL);
> > > + if (query->jointree->quals == NULL)
> > > + ereport(ERROR,
> > > + (errcode(ERRCODE_SYNTAX_ERROR),
> > > +  errmsg("%s requires a WHERE clause 
> > > when the require_where hook is enabled.",
> > > +  query->commandType == 
> > > CMD_UPDATE ? "UPDATE" : "DELETE"),
> > > +  errhint("To %s all rows, use \"WHERE 
> > > true\" or similar.",
> > > +  query->commandType == 
> > > CMD_UPDATE ? "update" : "delete")));
> > > + }
> > 
> > Per my earlier comment, I think this should use
> > ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED instead.
> 
> Fixed.
> 
> > I think this should say "the \"require_hook\" extension" rather than
> > use the term "hook".
> 
> Fixed.
> 
> > (There are two or three translatability rules violations in this
> > snippet,
> 
> Based on the hints in the docs docs around translation, I've
> refactored this a bit.
> 
> > but since this is an extension and those are not translatable, I
> > won't say elaborate further.)
> 
> "Not translatable," or "not currently translated?"

Oops^2.  Correct patch attached and sent to correct list. :P

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
commit 9e65a67434a553b717ba735472cf3108f4ee0e23
Author: David Fetter 
Date:   Thu Jul 21 23:34:21 2016 -0700

require_where: a contrib hook

This adds a process utility hook which makes simple UPDATE and DELETE
statements require a WHERE clause when loaded.

It is not intended to provide a general capability.  Instead, its job is to
prevent common human errors made by people who only rarely use SQL.  The 
hook
is small enough to be usable as part of a short lesson on hooks.

diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..4bd456f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -40,6 +40,7 @@ SUBDIRS = \
pgstattuple \
pg_visibility   \
postgres_fdw\
+   require_where   \
seg \
spi \
tablefunc   \
diff --git a/contrib/require_where/Makefile b/contrib/require_where/Makefile
new file mode 100644
index 000..933eb00
--- /dev/null
+++ b/contrib/require_where/Makefile
@@ -0,0 +1,19 @@
+# contrib/require_where/Makefile
+
+MODULE_big = require_where
+OBJS = require_where.o
+
+PGFILEDESC = 'require_where - require simple DELETEs and UPDATEs to have a 
WHERE clause'
+
+REGRESS = require_where
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS = $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/require_where
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_builddir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/require_where/expected/require_where.out 
b/contrib/require_where/expected/require_where.out
new file mode 100644
index 000..3fe28ec
--- /dev/null
+++ b/contrib/require_where/expected/require_where.out
@@ -0,0 +1,16 @@
+--
+--   Test require_where
+--
+\set echo all
+CREATE TABLE test_require_where(t TEXT);
+UPDATE test_require_where SET t=t; -- succeeds
+DELETE FROM test_require_where; -- succeeds
+LOAD 'require_where';
+UPDATE test_require_where SET t=t; -- fails
+ERROR:  UPDATE requires a WHERE clause when the "require_where" extension is 
loaded.
+HINT:  To update all rows, use "WHERE true" or similar.
+UPDATE test_require_where SET t=t WHERE true; -- succeeds
+DELETE FROM test_require_where; -- fails
+ERROR:  DELETE requires a WHERE clause when the "require_where" extension is 
loaded.
+HINT:  To delete all rows, use "WHERE true" or similar.
+DELETE FROM test_require_where WHERE true; -- succeeds
diff --git a/contrib/require_where/require_where.c 
b/contrib/require_where/require_where.c
new file mode 100644
index 000..0152a25
--- /dev/null
+++ b/contrib/require_where/require_where.c
@@ -0,0 +1,68 @@
+/*
+ * --
+ *
+ * require_where.c
+ *
+ * Copyright (C) 2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/require_where/require_where.c
+ *
+ * --
+

Re: [HACKERS] GSoC 2017

2017-01-10 Thread Andrew Borodin 
2017-01-10 14:53 GMT+05:00 Alexander Korotkov :
> 1. What project ideas we have?

I have one more project of interest which I can mentor.

Topic. GiST API advancement

===Idea===
GiST API was designed at the beginning of 90th to reduce boilerplate
code around data access methods over balanced tree. Now, after 30
years, there are some ideas on improving this API.

===Project details===
Opclass developer must specify 4 core operations to make a type GiST-indexable:
1. Split: a function to split set of datatype instances into two parts.
2. Penalty calculation: a function to measure penalty for unification
of two keys.
3. Collision check: a function which determines whether two keys may
have overlap or are not intersecting.
4. Unification: a function to combine two keys into one so that
combined key collides with both input keys.

Functions 2 and 3 can be improved.
For example, Revised R*-tree[1] algorithm of insertion cannot be
expressed in terms of penalty-based algorithms. There was some
attempts to bring parts of RR*-tree insertion, but they come down to
ugly hacks [2]. Current GiST API, due to penalty-based insertion
algorithm, does not allow to implement important feature of RR*-tree:
overlap optimization. As Norbert Beckman, author of RR*-tree, put it
in discussion: “Overlap optimization is one of the main elements, if
not the main query performance tuning element of the RR*-tree. You
would fall back to old R-Tree times if that would be left off.”

Collision check currently returns binary result:
1.   Query may be collides with subtree MBR
2.   Query do not collides with subtree
This result may be augmented with a third state: subtree is totally
within query. In this case GiST scan can scan down subtree without key
checks.

Potential effect of these improvements must be benchmarked. Probably,
implementation of these two will spawn more ideas on GiST performance
improvements.

Finally, GiST do not provide API for bulk loading. Alexander Korotkov
during GSoC 2011 implemented buffered GiST build. This index
construction is faster, but yields the index tree with virtually same
querying performance. There are different algorithms aiming to provide
better indexing tree due to some knowledge of data, e.g. [3]


[1] Beckmann, Norbert, and Bernhard Seeger. "A revised r*-tree in
comparison with related index structures." Proceedings of the 2009 ACM
SIGMOD International Conference on Management of data. ACM, 2009.
[2] 
https://www.postgresql.org/message-id/flat/CAJEAwVFMo-FXaJ6Lkj8Wtb1br0MtBY48EGMVEJBOodROEGykKg%40mail.gmail.com#cajeawvfmo-fxaj6lkj8wtb1br0mtby48egmvejboodroegy...@mail.gmail.com
[3] Achakeev, Daniar, Bernhard Seeger, and Peter Widmayer. "Sort-based
query-adaptive loading of r-trees." Proceedings of the 21st ACM
international conference on Information and knowledge management. ACM,
2012.

Best regards, Andrey Borodin.


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

Re: [HACKERS] pageinspect: Hash index support

2017-01-10 Thread Jesper Pedersen 

On 01/10/2017 10:39 AM, Tom Lane wrote:

Robert Haas  writes:

No, you're missing the point.  The patch doesn't need to add
pageinspect--1.6.sql at all, nor does it remove pageinspect--1.5.sql.
It only needs to add pageinspect--1.5--1.6.sql and change the default
version to 1.6.  No other changes are required.


Right, this is a new recommended process since commit 40b449ae8,
which see for rationale.

Use, eg, commit 11da83a0e as a model for extension update patches.



Thanks for the commit ids !

Revised patched attached.

Best regards,
 Jesper


>From 2c75e4af17903c22961f23ed5455614340d0dd4e Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Tue, 3 Jan 2017 07:49:42 -0500
Subject: [PATCH] Add support for hash index in pageinspect contrib module v11

Authors: Ashutosh Sharma and Jesper Pedersen.

---
 contrib/pageinspect/Makefile  |  10 +-
 contrib/pageinspect/hashfuncs.c   | 558 ++
 contrib/pageinspect/pageinspect--1.5--1.6.sql |  76 
 contrib/pageinspect/pageinspect.control   |   2 +-
 doc/src/sgml/pageinspect.sgml | 149 +++
 src/backend/access/hash/hashovfl.c|  52 +--
 src/include/access/hash.h |   1 +
 7 files changed, 817 insertions(+), 31 deletions(-)
 create mode 100644 contrib/pageinspect/hashfuncs.c
 create mode 100644 contrib/pageinspect/pageinspect--1.5--1.6.sql

diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 87a28e9..d7f3645 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -2,13 +2,13 @@
 
 MODULE_big	= pageinspect
 OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o \
-		  brinfuncs.o ginfuncs.o $(WIN32RES)
+		  brinfuncs.o ginfuncs.o hashfuncs.o $(WIN32RES)
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.5.sql pageinspect--1.4--1.5.sql \
-	pageinspect--1.3--1.4.sql pageinspect--1.2--1.3.sql \
-	pageinspect--1.1--1.2.sql pageinspect--1.0--1.1.sql \
-	pageinspect--unpackaged--1.0.sql
+DATA = pageinspect--1.5.sql pageinspect--1.5--1.6.sql \
+	pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
+	pageinspect--1.2--1.3.sql pageinspect--1.1--1.2.sql \
+	pageinspect--1.0--1.1.sql pageinspect--unpackaged--1.0.sql
 PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
 
 REGRESS = page btree brin gin
diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c
new file mode 100644
index 000..525e780
--- /dev/null
+++ b/contrib/pageinspect/hashfuncs.c
@@ -0,0 +1,558 @@
+/*
+ * hashfuncs.c
+ *		Functions to investigate the content of HASH indexes
+ *
+ * Copyright (c) 2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		contrib/pageinspect/hashfuncs.c
+ */
+
+#include "postgres.h"
+
+#include "access/hash.h"
+#include "access/htup_details.h"
+#include "catalog/pg_am.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "utils/builtins.h"
+
+PG_FUNCTION_INFO_V1(hash_page_type);
+PG_FUNCTION_INFO_V1(hash_page_stats);
+PG_FUNCTION_INFO_V1(hash_page_items);
+PG_FUNCTION_INFO_V1(hash_bitmap_info);
+PG_FUNCTION_INFO_V1(hash_metapage_info);
+
+#define IS_HASH(r) ((r)->rd_rel->relam == HASH_AM_OID)
+
+/* 
+ * structure for single hash page statistics
+ * 
+ */
+typedef struct HashPageStat
+{
+	uint32		live_items;
+	uint32		dead_items;
+	uint32		page_size;
+	uint32		free_size;
+
+	/* opaque data */
+	BlockNumber hasho_prevblkno;
+	BlockNumber hasho_nextblkno;
+	Bucket		hasho_bucket;
+	uint16		hasho_flag;
+	uint16		hasho_page_id;
+} HashPageStat;
+
+
+/*
+ * Verify that the given bytea contains a HASH page, or die in the attempt.
+ * A pointer to the page is returned.
+ */
+static Page
+verify_hash_page(bytea *raw_page, int flags)
+{
+	Page		page;
+	int			raw_page_size;
+	HashPageOpaque pageopaque;
+
+	raw_page_size = VARSIZE(raw_page) - VARHDRSZ;
+
+	if (raw_page_size != BLCKSZ)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("input page too small"),
+ errdetail("Expected size %d, got %d",
+		   BLCKSZ, raw_page_size)));
+
+	page = VARDATA(raw_page);
+
+	if (PageIsNew(page))
+		ereport(ERROR,
+(errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("index table contains unexpected zero page")));
+
+	if (PageGetSpecialSize(page) != MAXALIGN(sizeof(HashPageOpaqueData)))
+		ereport(ERROR,
+(errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("index table contains corrupted page")));
+
+	pageopaque = (HashPageOpaque) PageGetSpecialPointer(page);
+	if (pageopaque->hasho_page_id != HASHO_PAGE_ID)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("page is not a HASH page"),
+ errdetail("Expected %08x, got %08x.",
+		   HASHO_PAGE_ID, pageopaque->hasho_page_id)));
+
+	if (!(pageopaque->hasho_flag & flags))
+		ereport(ERROR,
+

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Fabien COELHO 


Hello Craig,


I have submitted a proof of this fact in the form of a counter example which
(1) (pseudo) implements the use-case by logging into an audit table the fact
a user accesses the secure level (2) shows that the status of a non
transactional session variable used for keeping this status is wrong for the
use case in some cases (it says that all is well while appending to the
audit table failed).


You've been assuming everyone else cares about auditing such access
into a table.


No, I have not.

For the PosgreSQL product, I'm really assuming that a security feature 
should work in all cases, not just some cases with implicit uncheckable 
restrictions, especially restrictions related to transactions which is all 
a database is about. I think that providing a misleading feature is a bad 
idea.


Note that my blessing is not required. If a committer wants to add this 
then they can do it.


But you're fixated on the idea that without that use case satisfied the 
rest is useless, and that's simply not the case. Transactional vars are 
only needed if you make _write_ changes to the DB that must be committed 
atomically with the var change. If you're only doing (maybe expensive) 
lookups, it doesn't matter.


It does not matter if and only if the transaction does not fail, not 
because the variable is not transactional. Basically, if it is 
untransactional, then it works only if it behaves exactly like a 
transaction...




Again ... I think you've assumed everyone else is talking about the
same security-related case you are.


I'm looking forward to see any use case which requires untransactional 
variables with permissions and works correctly without adding un-database 
constraints such as "please do not use in transactions that change any 
data because then it may or may not work".




It's kind of like someone coming to you and saying they want to add an
engine to their glider, and you explaining that it's totally useless
to add an engine unless it can also function as a submarine. Um,
that's nice, but not what they asked for.


Hmmm... I think that it is really like adding an engine on a glider which 
does not work if the glider flies under a cloud. You just have to recall 
that you should not fly under a cloud when the engine is turned on.


--
Fabien.


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

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Fabien COELHO 


Hello Robert,


You're just ignoring explanations from other people - Craig in
particular - about why it DOES satisfy their use case.


I'm not so sure about Craig precise opinion, but I cannot talk in his 
name. I think that I understood that he points out that there exists a 
situation where the use case is okay despite an untransactional variable: 
if the containing transaction is warranted not to fail, and probably 
(provably?) a read-only transaction is enough for that. Okay, sure...


This falls under "the feature works sometime", which I think is not 
acceptable for a security thing in pg core.


And the reason his argument is valid is because he is questioning your 
premise. [...]


Yes.

I made the assumption that PostgreSQL is about keeping data safe and 
secure, and that misleading features which do not comply with this goal 
should be kept out.


This is indeed a subjective opinion, not provable truth.

I only assumed that this opinion was implicitely shared, so that providing 
a counter example with the feature where data is not safe or secure was 
enough to dismiss the proposal.


I'm clearly wrong: some people are okay with a security feature proven not 
to work in some case, if it works for their particular (read-only) case.



I do not like Pavel's feature, this is a subjective opinion. This feature
does not provide a correct solution for the use case, this is an objective
fact. The presented feature does not have a real use case, this is too bad.


If the presented feature had no use case, I don't think there would be
3 or 4 people arguing for it.  Those people aren't stupid.


I have not said that, nor thought that.

I pointed out my arguments, basically I answer "security must always work" 
to "the feature can work sometimes". Then it cycles. As I can offer 
limited time for reviewing features, at some point I do not have any more 
time to argue constructively and convince people, that is life. That is 
when I tried to conclude my contribution by sending my review.


[..] Are you also willing to accept other people's differing 
conclusions?


I do not have to "accept", or not, differing conclusions. The committer 
decides in the end, because they have the power, I just have words.


All I can say is that as a committer I would not commit such a feature.

As a basic contributor, I can hope that the best decision is made in the 
end, and for that I try to express arguments precisely and objectively, 
that is the point of reviewing a proposal and give advice about how it 
should be amended if I think it should.


I believe that the words "silly" and "academic" were used about certain 
proposals that you made, [..] it does necessarily imply personal 
disrespect.


Sure. "Silly academic" suits me though, I'm fine with it:-)

--
Fabien.


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

[HACKERS] Couple of issues with prepared FETCH commands

2017-01-10 Thread Andrew Gierth 
(This came up on IRC, but I'm not sure to what extent it should be
considered a "bug")

If you do  PQprepare(conn, "myfetch", "FETCH ALL FROM mycursor", ...);
then the results are unpredictable in two ways:

Firstly, nothing causes the plancache entry to be revalidated just
because "mycursor" got opened with a different query, so the result type
can change between uses.  This could be considered a "caveat user" case,
though, and I can't find anything that actually breaks.

But the problem that actually came up is this: if you do the PQprepare
before the named cursor has actually been opened, then everything works
_up until_ the first event, such as a change to search_path, that forces
a revalidation; and at that point it fails with the "must not change
result type" error _even if_ the cursor always has exactly the same
result type.  This happens because the initial prepare actually stored
NULL for plansource->resultDesc, since the cursor name wasn't found,
while on the revalidate, when the cursor obviously does exist, it gets
the actual result type.

It seems a bit of a "gotcha" to have it fail in this case when the
result type isn't actually being checked in other cases.

(In the reported case, search_path was actually changing due to the
creation of a temp table, so there was a certain amount of
spooky-action-at-a-distance to figure out in order to locate the
problem.)

-- 
Andrew (irc:RhodiumToad)


-- 
Sent 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_dump / copy bugs with "big lines" ?

2017-01-10 Thread Daniel Verite 
Alvaro Herrera wrote:

> With that, pushed and I hope this is closed for good.

Great!
I understand the fix couldn't be backpatched because
we are not allowed to change the StringInfo struct in
existing releases. But it occurs to me that the new "long_ok"
flag might not be necessary after all now that it's settled that
the length remains an int32.
The flag is used to enforce a rule that the string can't normally grow
past 1GB, and can reach 2GB only as an exception that we choose to
exercise for COPY starting with v10.
But that 1GB rule is never mentioned in stringinfo.[ch] AFAICS.
I know that 1GB is the varlena size and is a limit for various things,
but I don't know to what extent StringInfo is concerned by that.

Is it the case that users of StringInfo in existing releases
and extensions are counting on its allocator to fail and abort
if the buffer grows over the varlena range?

If it's true, then we're stuck with the current situation
for existing releases.
OTOH if this seems like a nonlegit expectation, couldn't we say that
the size limit is 2^31 for all, and suppress the flag introduced by
the fix?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Fabien COELHO 



I do not like Pavel's feature, this is a subjective opinion. This feature
does not provide a correct solution for the use case, this is an objective
fact. The presented feature does not have a real use case, this is too bad.


Oh, also, you might want to tell Oracle and the many people who use 
package variables that.


As it can be used safely with nested transaction, I have no doubt that 
they do that, and that auditors check that carefully when auditing code:-)



[...] Your unwillingness to listen to anyone else isn't doing your 
argument any favours though.


Hmmm. I'm far from perfect and I have a limited supply of patience when 
logic does not always apply in a long discussion.


However I think that my review of Pavel proposal is fair, with a clear 
separation of objective (proven) facts and subjective but argumented 
opinions. I do not think that I can contribute anything more by continuing 
argumenting, so I wish I would not have been dragged back into this 
thread:-(


Despite a lot of effort, Pavel proposal is still about a untransactional 
(by default) session variables. Too bad. Time out for me. I'm deeply 
against that, I have said it: I think it would harm PostgreSQL to provide 
such a misleading security feature. Then I'm done. If a committer wants to 
add untransactional session variables with permissions, it is their 
priviledge, and my blessing is not needed anyway.


--
Fabien.


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

Re: [HACKERS] Replication/backup defaults

2017-01-10 Thread Stephen Frost 
Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Tue, Jan 10, 2017 at 8:03 PM, Robert Haas  wrote:
> 
> > On Mon, Jan 9, 2017 at 11:02 AM, Peter Eisentraut
> >  wrote:
> > > On 1/9/17 7:44 AM, Magnus Hagander wrote:
> > >> So based on that, I suggest we go ahead and make the change to make both
> > >> the values 10 by default. And that we do that now, because that lets us
> > >> get it out through more testing on different platforms, so that we catch
> > >> issues earlier on if they do arise.
> > >
> > > Sounds good.
> >
> > I may be outvoted, but I'm still not in favor of changing the default
> > wal_level.  That caters only to people who lack sufficient foresight
> > to know that they need a replica before the system becomes so critical
> > that they can't bounce it to update the configuration.
> 
> True. But the current level only caters to those people who run large ETL
> jobs without doing any tuning on their system (at least none that would
> require a restart), or another one of the fairly specific workloads.
> 
> And as I keep re-iterating, it's not just about replicas, it's also about
> the ability to make proper backups. Which is a pretty fundamental feature.
> 
> I do think you are outvoted, yes :) At least that's the result of my
> tallying up the people who have spoken out on the thread.

I tend to agree with Magnus on this, being able to perform an online
backup is pretty darn important.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] CONNECTION LIMIT and Parallel Query don't play well together

2017-01-10 Thread David Rowley 
It has come to my attention that when a user has a CONNECTION LIMIT
set, and they make use of parallel query, that their queries can fail
due to the connection limit being exceeded.

Simple test case:

postgres=# CREATE USER user1 LOGIN CONNECTION LIMIT 2;
CREATE ROLE
postgres=# \c postgres user1
You are now connected TO DATABASE "postgres" AS USER "user1".
postgres=> CREATE TABLE t1 AS (SELECT i FROM GENERATE_SERIES(1,600) s(i));
SELECT 600
postgres=> SET max_parallel_workers_per_gather = 2;
SET
postgres=> SELECT COUNT(*) FROM t1;
ERROR:  too many connections FOR ROLE "user1"
CONTEXT:  parallel worker

Now, as I understand it, during the design of parallel query, it was
designed in such a way that nodeGather could perform all of the work
in the main process in the event that no workers were available, and
that the only user visible evidence of this would be the query would
be slower than it would otherwise be.

After a little bit of looking around I see that CountUserBackends()
does not ignore the parallel workers, and counts these as
"CONNECTIONS". It's probably debatable to weather these are
connections or not, but I do see that max_connections is separate from
max_worker_processes, per:

/* the extra unit accounts for the autovacuum launcher */
MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
max_worker_processes;

so the two don't stomp on each other's feet, which makes me think that
a parallel worker should not consume a user connection, since it's not
eating into max_connections. Also this is convenient fix for this
would be to have CountUserBackends() ignore parallel workers
completely.

The alternatives I've thought of are would be to make some additional
checks in RegisterDynamicBackgroundWorker() to make sure we don't get
more workers than the user would be allowed, but that would add more
code between the lock and increase contention, and we'd also somehow
need to find a way to reserve the connections until the parallel
workers started, so they were not taken by another concurrent
connection in the meantime. This all sounds pretty horrid.

Perhaps we can provide greater control of parallel workers per user in
a future release to allow admins who are concerned about users hogging
all of the parallel workers. Yet that's likely premature, as we don't
have a per query nob for that yet.

Thoughts?

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


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

Re: [HACKERS] sequence data type

2017-01-10 Thread Daniel Verite 
Peter Eisentraut wrote:

> This could probably be sorted out somehow, but I don't want
> to be too lax now and cause problems for later features.  There is a
> similar case, namely changing the return type of a function, which we
> also prohibit.

Consider the case of a table with a SERIAL column which later
has to become a BIGINT due to growth.
Currently a user would just alter the column's type and does
need to do anything with the sequence.

With the patch, it becomes a problem because

- ALTER SEQUENCE seqname MAXVALUE new_value
will fail because new_value is beyond the range of INT4.

- ALTER SEQUENCE seqname TYPE BIGINT
does not exist (yet?)

- DROP SEQUENCE seqname  (with the idea of recreating the
sequence immediately after) will be rejected because the table
depends on the sequence.

What should a user do to upgrade the SERIAL column?

BTW, I notice that a direct UPDATE of pg_sequence happens
to work (now that we have pg_sequence thanks to your other
recent contributions on sequences), but I guess it falls under the
rule mentioned in 
https://www.postgresql.org/docs/devel/static/catalogs.html

"You can drop and recreate the tables, add columns, insert and update values,
and severely mess up your system that way. Normally, one should not change
the system catalogs by hand, there are normally SQL commands to do that"

Previously, UPDATE seqname SET property=value was rejected with
a specific error "cannot change sequence".

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

Re: [HACKERS] Replication/backup defaults

2017-01-10 Thread David Steele 
On 1/10/17 3:06 PM, Stephen Frost wrote:

> * Magnus Hagander (mag...@hagander.net) wrote:
>> On Tue, Jan 10, 2017 at 8:03 PM, Robert Haas  wrote:

>>> I may be outvoted, but I'm still not in favor of changing the default
>>> wal_level.  That caters only to people who lack sufficient foresight
>>> to know that they need a replica before the system becomes so critical
>>> that they can't bounce it to update the configuration.
>>
>> True. But the current level only caters to those people who run large ETL
>> jobs without doing any tuning on their system (at least none that would
>> require a restart), or another one of the fairly specific workloads.
>>
>> And as I keep re-iterating, it's not just about replicas, it's also about
>> the ability to make proper backups. Which is a pretty fundamental feature.
>>
>> I do think you are outvoted, yes :) At least that's the result of my
>> tallying up the people who have spoken out on the thread.
> 
> I tend to agree with Magnus on this, being able to perform an online
> backup is pretty darn important.

Agreed and +1.

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] RustgreSQL

2017-01-10 Thread Josh Berkus 
On 01/09/2017 05:54 PM, Joel Jacobson wrote:
> On Mon, Jan 9, 2017 at 3:22 PM, Jim Nasby  wrote:
>> I do wonder if there are parts of the codebase that would be much better
>> suited to a language other than C, and could reasonably be ported.
>> Especially if that could be done in such a way that the net result is still
>> C code so we're not adding dependencies to non developers (similar to
>> bison).
>>
>> Extensions are a step in that direction, but they're ultimately not core
>> Postgres (which is a different issue).
> 
> I think this is a great idea!
> 
> That way the amount of C code could be reduced over time,
> while safely extending the official version with new functionality on
> the surface,
> without increasing the amount of C code.

Even if you don't ever end up touching core Postgres, being able to
write extensions in languages other than C (like, full-featured
extensions) would be its own benefit.

Why not start there?  That is, assuming that Joel has gobs of time to
work on this?  For that matter, I know that Jeff Davis is quite fond of
Rust.

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)


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

Re: [HACKERS] sequence data type

2017-01-10 Thread Andrew Gierth 
> "Daniel" == Daniel Verite  writes:

 Daniel> Consider the case of a table with a SERIAL column which later
 Daniel> has to become a BIGINT due to growth.  Currently a user would
 Daniel> just alter the column's type and does need to do anything with
 Daniel> the sequence.

 Daniel> With the patch, it becomes a problem because

 Daniel> - ALTER SEQUENCE seqname MAXVALUE new_value
 Daniel> will fail because new_value is beyond the range of INT4.

 Daniel> - ALTER SEQUENCE seqname TYPE BIGINT
 Daniel> does not exist (yet?)

 Daniel> - DROP SEQUENCE seqname  (with the idea of recreating the
 Daniel> sequence immediately after) will be rejected because the table
 Daniel> depends on the sequence.

 Daniel> What should a user do to upgrade the SERIAL column?

Something along the lines of:

begin;
alter table tablename alter column id drop default;
alter sequence tablename_id_seq owned by none;
create sequence tablename_id_seq2 as bigint owned by tablename.id;
select setval('tablename_id_seq2', last_value, is_called) from tablename_id_seq;
drop sequence tablename_id_seq;
alter table tablename alter column id type bigint;
alter table tablename alter column id set default nextval('tablename_id_seq2');
commit;

Not impossible, but not at all obvious and quite involved. (And -1 for
this feature unless this issue is addressed.)

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] RustgreSQL

2017-01-10 Thread Craig Ringer 
On 10 January 2017 at 23:10, otheus uibk  wrote:

> Craig, isn't it the case that C++ exceptions still cause tremendous
> slow-downs of the entire code-base?

No, and it hasn't been so for a long time even for gcc.

See e.g. 
http://stackoverflow.com/questions/13835817/are-exceptions-in-c-really-slow

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


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

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Craig Ringer 
On 11 January 2017 at 06:09, Fabien COELHO  wrote:

> I'm not so sure about Craig precise opinion, but I cannot talk in his name.
> I think that I understood that he points out that there exists a situation
> where the use case is okay despite an untransactional variable: if the
> containing transaction is warranted not to fail, and probably (provably?) a
> read-only transaction is enough for that. Okay, sure...

No.

I'm saying that if you do a series of checks, then set the variable
last, it does not matter if the xact somehow aborts after setting the
variable. You have already done all your checks and are satisfied that
the user has the appropriate access level.

This does not cover all use cases. It's not suitable if the checks
involve writes to the database since you can then have a situation
where the writes are lost but the variable setting retained.

However, it does cover some common and useful ones like looking up
external services, possibly expensive queries, etc, and setting a
variable to cache "yup, access approved".

> I made the assumption that PostgreSQL is about keeping data safe and secure,
> and that misleading features which do not comply with this goal should be
> kept out.

This is hyperbolic. Every system has rules and restrictions.
Appropriately documented, I don't see a problem.

Should we also remove sequences because they violate transactional
boundaries and users get confused by them? You won't mind that you can
only use synthetic keys completely serially, right?

Sometimes compromises are a thing.

Now, I have already said that I actually agree that transactional vars
are probably a good default and something we should have if we do
this. But they are not the One True And Only Way.

> I'm clearly wrong: some people are okay with a security feature proven not
> to work in some case, if it works for their particular (read-only) case.

Many normal things work only in some cases.

COMMIT can be indeterminate if you lose your connection after sending
COMMIT and before getting a reply. Did my transaction commit? Um, I
dunno. (Yes, I know we have 2PC). That's pretty fundmental...

So. I would like transactional variables and think you have made a
good case for them. If they prove impractical, I think variables with
access controls that are not transactional are also OK though less
useful, so long as they are clearly documented.

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


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

Re: [HACKERS] Replication/backup defaults

2017-01-10 Thread Michael Paquier 
On Wed, Jan 11, 2017 at 10:06 AM, David Steele  wrote:
> On 1/10/17 3:06 PM, Stephen Frost wrote:
>> * Magnus Hagander (mag...@hagander.net) wrote:
>>> On Tue, Jan 10, 2017 at 8:03 PM, Robert Haas  wrote:
>
 I may be outvoted, but I'm still not in favor of changing the default
 wal_level.  That caters only to people who lack sufficient foresight
 to know that they need a replica before the system becomes so critical
 that they can't bounce it to update the configuration.
>>>
>>> True. But the current level only caters to those people who run large ETL
>>> jobs without doing any tuning on their system (at least none that would
>>> require a restart), or another one of the fairly specific workloads.
>>>
>>> And as I keep re-iterating, it's not just about replicas, it's also about
>>> the ability to make proper backups. Which is a pretty fundamental feature.
>>>
>>> I do think you are outvoted, yes :) At least that's the result of my
>>> tallying up the people who have spoken out on the thread.
>>
>> I tend to agree with Magnus on this, being able to perform an online
>> backup is pretty darn important.
>
> Agreed and +1.

+1'ing.
-- 
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] CONNECTION LIMIT and Parallel Query don't play well together

2017-01-10 Thread Amit Kapila 
On Wed, Jan 11, 2017 at 2:44 AM, David Rowley
 wrote:
> It has come to my attention that when a user has a CONNECTION LIMIT
> set, and they make use of parallel query, that their queries can fail
> due to the connection limit being exceeded.
>
> Simple test case:
>
> postgres=# CREATE USER user1 LOGIN CONNECTION LIMIT 2;
> CREATE ROLE
> postgres=# \c postgres user1
> You are now connected TO DATABASE "postgres" AS USER "user1".
> postgres=> CREATE TABLE t1 AS (SELECT i FROM GENERATE_SERIES(1,600) s(i));
> SELECT 600
> postgres=> SET max_parallel_workers_per_gather = 2;
> SET
> postgres=> SELECT COUNT(*) FROM t1;
> ERROR:  too many connections FOR ROLE "user1"
> CONTEXT:  parallel worker
>
> Now, as I understand it, during the design of parallel query, it was
> designed in such a way that nodeGather could perform all of the work
> in the main process in the event that no workers were available, and
> that the only user visible evidence of this would be the query would
> be slower than it would otherwise be.
>

This has been reported previously [1] and I have explained the reason
why such a behaviour is possible and why this can't be handled in
Gather node.

> After a little bit of looking around I see that CountUserBackends()
> does not ignore the parallel workers, and counts these as
> "CONNECTIONS". It's probably debatable to weather these are
> connections or not,

I think this is not only for parallel workers, rather any background
worker that uses database connection
(BGWORKER_BACKEND_DATABASE_CONNECTION) will be counted in a similar
way.  I am not sure if it is worth inventing something to consider
such background worker connections different from backend connections.
However, I think we should document it either in parallel query or in
background worker or in Create User .. Connection section.


[1] - 
https://www.postgresql.org/message-id/20161222111345.25620.8603%40wrigleys.postgresql.org


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


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

Re: [HACKERS] WARM and indirect indexes

2017-01-10 Thread Amit Kapila 
On Wed, Jan 11, 2017 at 12:54 AM, Alvaro Herrera
 wrote:
> Two options are on the table to attack the problem of updates causing
> write amplification: WARM and indirect indexes.  They are completely
> different approaches but have overlapping effects on what scenarios are
> improved.  Here's a recap of both features, with the intent that we make
> a well-considered decision about each.
>
> The main effect of both features is that an updated tuple doesn't
> require updating indexes that are on unmodified columns.  Indirect
> indexes are a completely new server feature which may enable other
> refinements later on; WARM is a targeted optimization on top of the HOT
> optimization.
>
> The big advantage of WARM is that it works automatically, like HOT: the
> user doesn't need to do anything different than today to get the
> benefit.  With indirect indexes, the user needs to create the index as
> indirect explicitely.
>
> There are two big disadvantages to WARM (as to HOT): it cannot be
> applied when the heap page is full; and concurrent long-running
> transactions can spoil reclaimability of recently dead tuples in heap
> pages.  There's a further disadvantage: currently, there can be only one
> WARM update in an update chain.  (Pavan believes it is possible to allow
> multiple ones.)  All those cases can benefit from indirect indexes.
>
> Another interesting case is a table with a primary key and a JSON
> object, on which you have a GIN index (or an int[] array, or text
> search).  What happens if you modify the JSON?  With WARM, this is just
> a normal index update.  With indirect indexes, you may be able to skip
> inserting index entries for all the JSON elements except those which
> changed.  (However, this is not implemented yet.)
>

I think both are of use in somewhat different scenarios, probably WARM
will be more effective and can be used in more number of cases to
reduce write amplification.  It seems to me that indirect indexes are
generally useful with clustered index where data is stored in the leaf
node of the tree, that doesn't mean it can't be used in other cases as
mentioned by you.  If you see the work required by indirect indexes is
justified with respect to its usecase, then I think it will be okay to
get both features as the indirect index can be used in future if
somebody implements clustered index.

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


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

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

2017-01-10 Thread Dilip Kumar 
On Sat, Dec 31, 2016 at 9:01 AM, Amit Kapila  wrote:
> Agreed and changed accordingly.
>
>> 2. It seems that we have missed one unlock in case of absorbed
>> wakeups. You have initialised extraWaits with -1 and if there is one
>> extra wake up then extraWaits will become 0 (it means we have made one
>> extra call to PGSemaphoreLock and it's our responsibility to fix it as
>> the leader will Unlock only once). But it appear in such case we will
>> not make any call to PGSemaphoreUnlock.
>>
>
> Good catch!  I have fixed it by initialising extraWaits to 0.  This
> same issue exists from Group clear xid for which I will send a patch
> separately.
>
> Apart from above, the patch needs to be adjusted for commit be7b2848
> which has changed the definition of PGSemaphore.

I have reviewed the latest patch and I don't have any more comments.
So if there is no objection from other reviewers I can move it to
"Ready For Committer"?


I have performed one more test, with 3000 scale factor because
previously I tested only up to 1000 scale factor. The purpose of this
test is to check whether there is any regression at higher scale
factor.

Machine: Intel 8 socket machine.
Scale Factor: 3000
Shared Buffer: 8GB
Test: Pgbench RW test.
Run: 30 mins median of 3

Other modified GUC:
-N 300 -c min_wal_size=15GB -c max_wal_size=20GB -c
checkpoint_timeout=900 -c maintenance_work_mem=1GB -c
checkpoint_completion_target=0.9

Summary:
- Did not observed any regression.
- The performance gain is in sync with what we have observed with
other tests at lower scale factors.


Sync_Commit_Off:
client  Head Patch

8 10065   10009
16   18487   18826
32   28167   28057
64   26655   28712
128 20152   24917
256 16740   22891

Sync_Commit_On:

Client   Head Patch

8 5102   5110
16   8087   8282
32 12523 12548
64 14701 15112
128   14656  15238
256   13421  16424

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

2017-01-10 Thread Peter Geoghegan 
On Fri, Jan 6, 2017 at 12:01 PM, Thomas Munro
 wrote:
> Here is a new WIP patch.  I have plenty of things to tidy up (see note
> at end), but the main ideas are now pretty clear and I'd appreciate
> some feedback.

I have some review feedback for your V3. I've chosen to start with the
buffile.c stuff, since of course it might share something with my
parallel tuplesort patch. This isn't comprehensive, but I will have
more comprehensive feedback soon.

I'm not surprised that you've generally chosen to make shared BufFile
management as simple as possible, with no special infrastructure other
than the ability to hold open other backend temp files concurrently
within a worker, and no writing to another worker's temp file, or
shared read pointer. As you put it, everything is immutable. I
couldn't see much opportunity for adding a lot of infrastructure that
wasn't written explicitly as parallel hash join code/infrastructure.
My sense is that that was a good decision. I doubted that you'd ever
want some advanced, generic shared BufFile thing with multiple read
pointers, built-in cache coherency, etc. (Robert seemed to think that
you'd go that way, though.)

Anyway, some more specific observations:

* ISTM that this is the wrong thing for shared BufFiles:

> +BufFile *
> +BufFileImport(BufFileDescriptor *descriptor)
> +{
...
> +   file->isInterXact = true; /* prevent cleanup by this backend */

There is only one user of isInterXact = true BufFiles at present,
tuplestore.c. It, in turn, only does so for cases that require
persistent tuple stores. A quick audit of these tuplestore.c callers
show this to just be cursor support code within portalmem.c. Here is
the relevant tuplestore_begin_heap() rule that that code adheres to,
unlike the code I've quoted above:

 * interXact: if true, the files used for on-disk storage persist beyond the
 * end of the current transaction.  NOTE: It's the caller's responsibility to
 * create such a tuplestore in a memory context and resource owner that will
 * also survive transaction boundaries, and to ensure the tuplestore is closed
 * when it's no longer wanted.

I don't think it's right for buffile.c to know anything about file
paths directly -- I'd say that that's a modularity violation.
PathNameOpenFile() is called by very few callers at the moment, all of
them very low level (e.g. md.c), but you're using it within buffile.c
to open a path to the file that you obtain from shared memory
directly. This is buggy because the following code won't be reached in
workers that call your BufFileImport() function:

/* Mark it for deletion at close */
VfdCache[file].fdstate |= FD_TEMPORARY;

/* Register it with the current resource owner */
if (!interXact)
{
VfdCache[file].fdstate |= FD_XACT_TEMPORARY;

ResourceOwnerEnlargeFiles(CurrentResourceOwner);
ResourceOwnerRememberFile(CurrentResourceOwner, file);
VfdCache[file].resowner = CurrentResourceOwner;

/* ensure cleanup happens at eoxact */
have_xact_temporary_files = true;
}

Certainly, you don't want the "Mark it for deletion at close" bit.
Deletion should not happen at eoxact for non-owners-but-sharers
(within FileClose()), but you *do* want CleanupTempFiles() to call
FileClose() for the virtual file descriptors you've opened in the
backend, to do some other cleanup. In general, you want to buy into
resource ownership for workers. As things stand, I think that this
will leak virtual file descriptors. That's really well hidden because
there is a similar CleanupTempFiles() call at proc exit, I think.
(Didn't take the time to make sure that that's what masked problems.
I'm sure that you want minimal divergence with serial cases,
resource-ownership-wise, in any case.)

Instead of all this, I suggest copying some of my changes to fd.c, so
that resource ownership within fd.c differentiates between a vfd that
is owned by the backend in the conventional sense, including having a
need to delete at eoxact, as well as a lesser form of ownership where
deletion should not happen. Maybe you'll end up using my BufFileUnify
interface [1] within workers (instead of just within the leader, as
with parallel tuplesort), and have it handle all of that for you.
Currently, that would mean that there'd be an unused/0 sized "local"
segment for the unified BufFile, but I was thinking of making that not
happen unless and until a new segment is actually needed, so even that
minor wart wouldn't necessarily affect you.

> Some assorted notes on the status:  I need to do some thinking about
> the file cleanup logic: both explicit deletes at the earliest possible
> time, and failure/error paths.  Currently the creator of each file is
> responsible for cleaning it up, but I guess if the creator aborts
> early the file disappears underneath the others' feet, and then I
> guess they might raise a confusing error report that races against the
> root cause error report;

Re: [HACKERS] WARM and indirect indexes

2017-01-10 Thread Alvaro Herrera 
Bruce Momjian wrote:

> 1.  What percentage speedup is the _average_ user going to get?  You
> have to consider people who will use indirect indexes who get no benefit
> or a net slowdown, and users who will get a benefit.
> 
> 2.  What percentage of users are going to use indirect indexes?
> 
> So, for #1 you might have users who are getting +1%, +50%, and -20%, so
> maybe +10% average, and for #2 you might have 0.1%.  When you multiply
> them out, you get 0.01% average improvement per installation, which is
> very small.  Obviously, these are just wild guesses, but this is just to
> make a point.

Perhaps not many users will require indirect indexes; but for those that
do, the feature might be invaluable.  We don't do only things that
benefit everybody -- some features are there to satisfy small
populations ("snapshot too old" is a recent example).  We should of
course do, and perhaps even favor doing things that benefit everybody,
but should also do the other things.

-- 
Álvaro Herrerahttps://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

[HACKERS] Passing query string to workers

2017-01-10 Thread Rafia Sabih 
Hello everybody,

Currently, query string is not passed to the workers and only master has
it. In the events, when multiple queries are running on a server and for
one query some worker crashes then it becomes quite burdensome to find the
query with the crashed worker, since on the worker crash no query is
displayed.

To fix this, I propose a patch wherein query string is passed to the
workers as well, hence, displayed when worker crashes.

Approach:
A token for query string is created in the shared memory, this token is
populated with the query string using the global string --
debug_query_string. Now, for each of the worker when
ExecGetParallelQueryDesc is called, we retrieve the query text from shared
memory and pass it to CreateQueryDesc.

Next, to ensure that query gets displayed at the time of crash,
BackendStatusArray needs to be populated correctly, specifically for our
purpose, activity needs to be filled with current query. For this I called
pgstat_report_activity in ParallelWorkerMain, with the query string, this
populates workers' tuples in system table -- pgstat_activity as well.
Previously, pgstat_report_activity was only called for master in
exec_simple_query, hence, for workers pgstat_activity remained null.

Results:
Here is an output for artificially created worker crash with and without
the patch.

Without the patch error report on worker crash:
 LOG:  worker process: parallel worker for PID 49739 (PID 49741) was
terminated by signal 11: Segmentation fault

Error report with the patch:
 LOG:  worker process: parallel worker for PID 51757 (PID 51758) was
terminated by signal 11: Segmentation fault
2017-01-11 11:10:27.630 IST [51742] DETAIL:  Failed process was running:
explain analyse select
l_returnflag,
l_linestatus,
sum(l_quantity) as sum_qty,
sum(l_extendedprice) as sum_base_price,
sum(l_extendedprice * (1 - l_discount)) as sum_disc_price,
sum(l_extendedprice * (1 - l_discount) * (1 + l_tax)) as sum_charge,
avg(l_quantity) as avg_qty,
avg(l_extendedprice) as avg_price,
avg(l_discount) as avg_disc,
count(*) as count_order
from
lineitem
where
l_shipdate <= date '1998-12-01' - interval '119' day
group by
l_returnflag,
l_linestatus
order by
l_returnflag,
l_linestatus
LIMIT 1;

Inputs of all sorts are encouraged.
-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


pass_queryText_to_workers_v1.patch
Description: Binary data

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

Re: [HACKERS] RustgreSQL

2017-01-10 Thread Amit Langote 
On 2017/01/11 8:02, Josh Berkus wrote:
> On 01/09/2017 05:54 PM, Joel Jacobson wrote:
>> On Mon, Jan 9, 2017 at 3:22 PM, Jim Nasby  wrote:
>>> I do wonder if there are parts of the codebase that would be much better
>>> suited to a language other than C, and could reasonably be ported.
>>> Especially if that could be done in such a way that the net result is still
>>> C code so we're not adding dependencies to non developers (similar to
>>> bison).
>>>
>>> Extensions are a step in that direction, but they're ultimately not core
>>> Postgres (which is a different issue).
>>
>> I think this is a great idea!
>>
>> That way the amount of C code could be reduced over time,
>> while safely extending the official version with new functionality on
>> the surface,
>> without increasing the amount of C code.
> 
> Even if you don't ever end up touching core Postgres, being able to
> write extensions in languages other than C (like, full-featured
> extensions) would be its own benefit.
> 
> Why not start there?  That is, assuming that Joel has gobs of time to
> work on this?  For that matter, I know that Jeff Davis is quite fond of
> Rust.

It seems someone tried (perhaps ran out of steam, unfortunately):

* Postgres extensions written in Rust *
https://github.com/thehydroimpulse/postgres-extension.rs

Thanks,
Amit




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

Re: [HACKERS] WARM and indirect indexes

2017-01-10 Thread Bruce Momjian 
On Tue, Jan 10, 2017 at 04:24:42PM -0300, Alvaro Herrera wrote:
> Two options are on the table to attack the problem of updates causing
> write amplification: WARM and indirect indexes.  They are completely
> different approaches but have overlapping effects on what scenarios are
> improved.  Here's a recap of both features, with the intent that we make
> a well-considered decision about each.
> 
> The main effect of both features is that an updated tuple doesn't
> require updating indexes that are on unmodified columns.  Indirect
> indexes are a completely new server feature which may enable other
> refinements later on; WARM is a targeted optimization on top of the HOT
> optimization.  
> 
> The big advantage of WARM is that it works automatically, like HOT: the
> user doesn't need to do anything different than today to get the
> benefit.  With indirect indexes, the user needs to create the index as
> indirect explicitely.

Thank you for the summary.  I think we have to consider two things with
indirect indexes:

1.  What percentage speedup is the _average_ user going to get?  You
have to consider people who will use indirect indexes who get no benefit
or a net slowdown, and users who will get a benefit.

2.  What percentage of users are going to use indirect indexes?

So, for #1 you might have users who are getting +1%, +50%, and -20%, so
maybe +10% average, and for #2 you might have 0.1%.  When you multiply
them out, you get 0.01% average improvement per installation, which is
very small.  Obviously, these are just wild guesses, but this is just to
make a point.  If you assume WARM has been optimized, #1 gets even
lower.

I am not saying we shouldn't do it, but I am afraid that the complexity
in figuring out when to use indirect indexes, combined with the number
of users who will try them, really hurts its inclusion.

FYI, we have a similar issue in adding GUC variables, which I outlined
in this blog post:

http://momjian.us/main/blogs/pgblog/2009.html#January_10_2009

-- 
  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] pgbench - allow backslash continuations in \set expressions

2017-01-10 Thread Rafia Sabih 
On Sat, Dec 3, 2016 at 1:52 PM, Fabien COELHO  wrote:
>
>> FWIW, I looked a bit further and concluded that probably psqlscan.l
>> doesn't need to be modified; so likely you could do this across all of
>> pgbench's commands without touching psql.  That might be an acceptable
>> compromise for now, though I still think that as soon as we have this
>> for pgbench, users will start wanting it in psql.
>
>
> The attached patch adds backslash-return (well newline really) continuations
> to all pgbench backslash-commands.
>
> The attached test uses continuations on all such commands (sleep set
> setshell and shell).
>
> I think that adding continuations to psql should be a distinct patch.
>
> --
> Fabien.

Hello Fabien,
The patch does not apply on the latest head, I guess this requires
rebasing since yours is posted in December. Again, it is giving
trailing whitespace errors (as I reported for the earlier version),
plus it does not apply with git apply, hopefully that would be fixed
once rebased.
Other than that, I observed that if after backslash space is there,
then the command fails. I think it should be something like if after
backslash some spaces are there, followed by end-of-line then it
should ignore these spaces and read next line, atleast with this new
meaning of backslash. Otherwise, it should be mentioned in the docs
that backslash should not be followed by space.

-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


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

Re: [HACKERS] An isolation test for SERIALIZABLE READ ONLY DEFERRABLE

2017-01-10 Thread Michael Paquier 
On Thu, Jan 5, 2017 at 6:41 AM, Thomas Munro
 wrote:
> It's a bit of a strange case: we're indirectly waiting for other
> backends' transactions to end, but it's not exactly a lock or even a
> predicate lock: it's waiting for a time when we could run safely with
> predicate locking disabled.  So it's not at all clear that
> pg_blocking_pids should try to get its hands on the appropriate pids
> (or if it even could).  Hence my attempt to do this using our
> wonderful wait introspection.
>
> I don't think putting the particular wait_event name into the test
> spec would be very useful.  It's really there as a whitelist to be
> conservative about excluding random waits caused by concurrent
> autovacuum etc.  It just happens to have only one thing in the
> whitelist for now, and I'm not sure what else would ever go in it...

Do you think that expanding the wait query by default could be
intrusive for the other tests? I am wondering about such a white list
to generate false positives for the existing tests, including
out-of-core extensions, as all the tests now rely only on
pg_blocking_pids().
-- 
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] postgres_fdw bug in 9.6

2017-01-10 Thread Ashutosh Bapat 
On Fri, Jan 6, 2017 at 6:31 PM, Etsuro Fujita
 wrote:
> On 2017/01/05 12:10, Etsuro Fujita wrote:
>>
>> On 2016/12/28 17:34, Ashutosh Bapat wrote:
>>>
>>> Hmm. If I understand the patch correctly, it does not return any path
>>> when merge join is allowed and there are merge clauses but no hash
>>> clauses. In this case we will not create a foreign join path, loosing
>>> some optimization. If we remove GetExistingLocalJoinPath, which
>>> returns a path in those cases as well, we have a regression in
>>> performance.
>
>
>> Ok, will revise, but as I mentioned upthread, I'm not sure it's a good
>> idea to search the pathlist to get a merge join even in this case.  I'd
>> vote for creating a merge join path from the inner/outer paths in this
>> case as well.
>
>
> Done.  Attached is the new version of the patch.
>
> * I'm still not sure the search approach is the right way to go, so I
> modified CreateLocalJoinPath so that it creates a mergejoin path that
> explicitly sorts both the outer and inner relations as in
> sort_inner_and_outer, by using the information saved in that function. I
> think we could try to create a sort-free mergejoin as in
> match_unsorted_outer, but I'm not sure it's worth complicating the code.
> * I modified CreateLocalJoinPath so that it handles the cheapest-total paths
> for the outer/inner relations that are parameterized if possible.
> * I adjusted the code and revised some comments.
>
> As I said upthread, we could skip costing for a local join path in
> CreateLocalJoinPath, for efficiency, but I'm not sure we should do that.

CreateLocalJoinPath tries to construct a nestloop path for the given
join relation because it wants to avoid merge/hash join paths which
require expensive setup not required for EPQ. But it chooses cheap
paths for joining relations which may not be nestloop paths. So,
effectively it could happen that the whole alternate local plan would
be filled with hash/merge joins except the uppermost join. Or it can
have foreign paths in there, which will need redirection. That's not
very good. Neither it's good to start constructing a nestloop join
tree top to bottom every time. We have to choose one of the following
alternatives

1. develop a mechanism to remember epq path for joining relations so
that it's available to CreateLocalJoinPath
2.let the caller pass epq local paths for joining relations to
CreateLocalJoinPath. How it remembers those, is FDW's problem.
2. Fix existing code by applying patch from [1]

[1] 
https://www.postgresql.org/message-id/cafjfprd9ozekur9aormh2toxq4nyuusw2kb9s%2bo%3duvz4xcr%3...@mail.gmail.com
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Floating point comparison inconsistencies of the geometric types

2017-01-10 Thread Kyotaro HORIGUCHI 
Hello, 

At Fri, 18 Nov 2016 10:58:27 +0100, Emre Hasegeli  wrote in 

> > To keep such kind of integrity, we should deeply consider about
> > errors.
> 
> My point is that I don't think we can keep integrity together with the
> fuzzy behaviour, or at least I don't have the skills to do that.  I
> can just leave the more complicated operators like "is a
> point on a line" as it is, and only change the basic ones.  Do you
> think a smaller patch like this would be acceptable?

The size of the patch is not a problem. I regret that I haven't
made your requirement clear. So as the startpoint of the new
discussion, I briefly summarize the current implement of
geometric comparisons.

- Floating point comparisons for gemetric types

  Comparison related to geometric types is performed by FPeq
  macro and similars defined in geo_decls.h. This intends to give
  tolerance to the comparisons.

 A
  FPeq: |<=e-|-e=>|(<= means inclusive, e = epsilon = tolerance)
  FPne:   ->|  e | e  |<-  (<- means exclusive)
  FPlt:  | e  |<-  
  FPle: |<=e |
  FPgt:   ->|  e |
  FPge:  | e=>|

  These seems reasonable ignoring the tolerance amount issue.


- Consistency between index and non-index scans.

 GIST supports geometric types.

 =# create table tpnt1(id int, p point);
 =# insert into tpnt1 (select i + 200, point(i*1.0e-6 / 100.0, i * 1.0e-6 / 
100.0) from generate_series(-200, 200) as i);
 =# create index on tpnt1 using gist (p);
 =# set enable_seqscan to false;
 =# set enable_bitmapscan to true;
 =# select count(*) from tpnt1 where p ~= point(0, 0);
201
 =# select count(*) from tpnt1 where p << point(0, 0);
100
 =# set enable_seqscan to true;
 =# set enable_bitmapscan to false;
 =# select count(*) from tpnt1 where p ~= point(0, 0);
201
 =# select count(*) from tpnt1 where p << point(0, 0);
100

At least for the point type, (bitmap) index scan is consistent
with sequential scan.  I remember that the issue was
"inconsistency between indexed and non-indexed scans over
geometric types" but they seem consistent with each other.

You mentioned b-tree, which don't have predefined opclass for
geometric types. So the "inconsistency" may be mentioning the
difference between geometric values and combinations of plain
(first-class) values. But the two are different things and
apparently using different operators (~= and = for equality) so
the issue is not fit for this. More specifically, "p ~= p0" and
"x = x0 and y = y0" are completely different.


Could you let me (or any other guys on this ml) have more precise
description on the problem and/or what you want to do with this
patch?

regards,
 
-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] sequence data type

2017-01-10 Thread Michael Paquier 
On Tue, Jan 10, 2017 at 5:03 AM, Peter Eisentraut
 wrote:
> On 1/8/17 2:39 PM, Steve Singer wrote:
>> The only concern I have with the functionality is that there isn't a way
>> to change the type of a sequence.
>
> If we implement the NEXT VALUE FOR expression (or anything similar that
> returns a value from the sequence), then the return type of that
> expression would be the type of the sequence.  Then, changing the type
> of the sequence would require reparsing all expressions involving the
> sequence.  This could probably be sorted out somehow, but I don't want
> to be too lax now and cause problems for later features.  There is a
> similar case, namely changing the return type of a function, which we
> also prohibit.
>
>> @@ -1236,7 +1239,15 @@ init_params(ParseState *pstate, List *options,
>> bool isInit,
>>  {
>>  DefElem*defel = (DefElem *) lfirst(option);
>>
>> -   if (strcmp(defel->defname, "increment") == 0)
>> +   if (strcmp(defel->defname, "as") == 0)
>> +   {
>> +   if (as_type)
>> +   ereport(ERROR,
>> + (errcode(ERRCODE_SYNTAX_ERROR),
>> +errmsg("conflicting or
>> redundant options")));
>> +   as_type = defel;
>> +   }
>> +   else if (strcmp(defel->defname, "increment") == 0)
>>
>> Should we including parser_errposition(pstate, defel->location)));  like
>> for the other errors below this?
>
> Yes, good catch.

+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("MINVALUE (%s) is too large for sequence data type %s",
+   bufm, format_type_be(seqform->seqtypid;
"too large" for a minimum value, really? You may want to add a comment
to say that the _MAX values are used for symmetry.

+   /* We use the _MAX constants for symmetry. */
+   if (seqform->seqtypid == INT2OID)
+   seqform->seqmin = -PG_INT16_MAX;
+   else if (seqform->seqtypid == INT4OID)
+   seqform->seqmin = -PG_INT32_MAX;
+   else
+   seqform->seqmin = -PG_INT64_MAX;
Hm. Is symmetry an important properly for sequences? It seems to me
that if we map with the data types we had better use the MIN values
instead for consistency. So the concept of this patch is rather weird
and would introduce an exception with the rest of the system just for
sequences.

(There are no tests for cycles).
-- 
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] proposal: session server side variables

2017-01-10 Thread Pavel Stehule 
2017-01-10 7:31 GMT+01:00 Fabien COELHO :

>
> Hello Robert,
>
> Half-persistence (in definition, not in value) is not a key feature needed
>>> by the use-case.
>>>
>>
>> Well, you don't get to decide that.
>>
>
> I do not think that your reprimand is deserved about this point: I did not
> decide a subjective opinion, I noted an objective fact.
>
> You've been told by at least three or four people that they don't want
>> variables to be transactional, you've been pointed to documentation links
>> showing that in other database systems including Oracle variables are not
>> transactional, and you still insist that this proposal is senseless unless
>> variables are transactional.
>>
>
> Indeed.
>
> I have submitted a proof of this fact in the form of a counter example
> which (1) (pseudo) implements the use-case by logging into an audit table
> the fact a user accesses the secure level (2) shows that the status of a
> non transactional session variable used for keeping this status is wrong
> for the use case in some cases (it says that all is well while appending to
> the audit table failed).
>
> I have also recognized that the use-case could be implemented safely,
> although not correctly, if pg provides nested/autonomous transactions like
> Oracle, DB2 or MS SQL does, but I think that having such a useful feature
> is quite far away...
>
> You have every right to decide what you think is useful, but you don't
>> have a right to decide what other people think is useful.
>>
>
> Hmmm.
>
> I feel entitled to point out to other people that their belief that a
> feature as described provides a correct solution to a particular use case
> is wrong, if it is factually the case. If they persist in this belief
> despite the submitted proof, I can only be sad about it, because if pg
> provides a feature for a security-relared use case which does not work
> correctly it is just shooting one's foot.
>
> I do not like Pavel's feature, this is a subjective opinion. This feature
> does not provide a correct solution for the use case, this is an objective
> fact. The presented feature does not have a real use case, this is too bad.
>

I wrote more time, so transactional and temporal support can be optional
feature. The people who uses package or module variables in other RDBMS
probably doesn't agree with you, so there are not real use case.

The transaction support is not main point in my proposal - the main points
are:

1. catalog based - created only once time, persistent metadata
2. allows be schema organized - like our PL functions
3. disallow identifier conflict - the name is unique in catalogue
4. allows to use declared secure access

After this discussion I append points

5. can be temporal - metadata are cleaned after end of life scope
6. can be transactional where content should be sensitive on possible
rollback

Regards

Pavel


>
> Finally, I did not "veto" this feature, I reviewed it in depth and
> concluded negatively. You are a committer and I'm just a "silly academic",
> you do not have to listen to anything I say and can take majority votes
> against proofs if you want.
>
> --
> Fabien.
>

Re: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Bruce Momjian 
On Tue, Jan 10, 2017 at 12:39:57PM -0500, Robert Haas wrote:
> Since I've spent a fair amount of brainpower trying to use 
> rather than  where possible, I'm not innately enthusiastic about
> a project whose end is to get rid of .  I won't lose a lot of
> sleep over it if we decide to go that direction, though.

FYI, doc/src/sgml/README.links has instructions on how these markup
elements behave, or at least used to behave.  We need to update that
file if it changed.

-- 
  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] WARM and indirect indexes

2017-01-10 Thread Pavan Deolasee 
On Wed, Jan 11, 2017 at 7:55 AM, Bruce Momjian  wrote:

> On Tue, Jan 10, 2017 at 04:24:42PM -0300, Alvaro Herrera wrote:
> > Two options are on the table to attack the problem of updates causing
> > write amplification: WARM and indirect indexes.  They are completely
> > different approaches but have overlapping effects on what scenarios are
> > improved.  Here's a recap of both features, with the intent that we make
> > a well-considered decision about each.
> >
> > The main effect of both features is that an updated tuple doesn't
> > require updating indexes that are on unmodified columns.  Indirect
> > indexes are a completely new server feature which may enable other
> > refinements later on; WARM is a targeted optimization on top of the HOT
> > optimization.
> >
> > The big advantage of WARM is that it works automatically, like HOT: the
> > user doesn't need to do anything different than today to get the
> > benefit.  With indirect indexes, the user needs to create the index as
> > indirect explicitely.
>
> Thank you for the summary.  I think we have to consider two things with
> indirect indexes:
>
> 1.  What percentage speedup is the _average_ user going to get?  You
> have to consider people who will use indirect indexes who get no benefit
> or a net slowdown, and users who will get a benefit.
>
> 2.  What percentage of users are going to use indirect indexes?
>

That could also be seen as an advantage to indirect indexes. While I
haven't seen the code, I believe indirect index code will only be hit if
someone actually uses them. So there won't be any overhead for other users
who do not wish to use the feature. WARM on the other hand will be "always
on" feature, even for system tables. That clearly has advantages, both from
usability perspective as well as the fact that the code will be heavily
tested. But if there are cases which get adversely affected by WARM, they
will have to pay the price for larger benefit.

To me, a better strategy is probably to focus on one of the patches, get
that in and then evaluate the second patch, both from complexity as well as
performance given that the first patch may have narrowed the gaps.

I was going to ask if we could implement indirect indexes as a separate
IndexAM. But I re-read this thread and found that you'd in fact done it
that way in the first version but then discarded it for performance
reasons. Is there a merit in evaluating that path for indirect indexes
again?

Thanks,
Pavan

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

[HACKERS] Do we support using agg or window functions in delete statement?

2017-01-10 Thread 高增琦 
Hi,

In transformDeleteStmt:

qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
qry->hasAggs = pstate->p_hasAggs;
if (pstate->p_hasAggs)
parseCheckAggregates(pstate, qry);

These code set agg and window function status for delete query,
but there is no similar code in transformInsertStmt and
transformUpdateStmt.

Do we support using agg or window function in delete statement?
Or, this code should be removed?

Some history of these code:
1. 1996-7-9 "Postgres95 1.01 Distribution - Virgin Sources":
Introduce agg check for insert/update/delete.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/parser/analyze.c;h=504e557abee8651596a9219dca069fcd20ecdaac;hb=d31084e9d1118b25fd16580d9d8c2924b5740dff

in transformDeleteStmt:
/* make sure we don't have aggregates in the where clause */
if (pstate->p_numAgg > 0)
parseCheckAggregates(pstate, qry);
in transformInsertStmt:
if (pstate->p_numAgg > 0)
finalizeAggregates(pstate, qry);
in transformUpdateStmt:
/* make sure we don't have aggregates in the where clause */
if (pstate->p_numAgg > 0)
parseCheckAggregates(pstate, qry);

2. 2006-6-21 "Disallow aggregate functions in UPDATE commands (unless
within a sub-SELECT)":
Change parseCheckAggregates to ereport in transformUpdateStmt.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/parser/analyze.c;h=4e30d2b96f3f58176f74aa0061660f47ca0b6426;hp=566c9a0488df68c94effea9e3f59d82da930eb18;hb=1f5ca045a435bc6aa9c98d7296973925c5208add;hpb=e256bafaa2aec06dd9dc9493c4e600319ab11525

in transformUpdateStmt:
if (pstate->p_hasAggs)
ereport(ERROR,
(errcode(ERRCODE_GROUPING_ERROR),
 errmsg("cannot use aggregate function in UPDATE")));

3. 2006-8-2 "Add support for multi-row VALUES clauses as part of INSERT
statements":
Change parseCheckAggregates in insert to ereport.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/parser/analyze.c;h=4f7001b6f1ac4fdd9fe65cba835eeb2513f8ad06;hp=b086afe8ca25a8d91314d352b847c47f3a05d32e;hb=9caafda579f699b43fa4c89bf13a2331ef00611e;hpb=d307c428cbb7c426e40163d234d993e644bbcc6b

in transformInsertStmt:
if (pstate->p_hasAggs)
ereport(ERROR,
(errcode(ERRCODE_GROUPING_ERROR),
 errmsg("cannot use aggregate function in VALUES")));

4. 2008-12-28 "Support window functions a la SQL:2008.":
Add window function related check for insert/update/delete.
(use the same style as agg)

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/parser/analyze.c;h=70688655cce18ac317faeafa2b51225f320fe493;hp=cdac02b71db69399e00b4a63eefe0d2f9f481ad0;hb=95b07bc7f5010233f52f9d11da74e2e5b653b0a7;hpb=38e9348282e9d078487147ba8a85aebec54e3a08

in transformDeleteStmt:
qry->hasAggs = pstate->p_hasAggs;
if (pstate->p_hasAggs)
parseCheckAggregates(pstate, qry);
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
if (pstate->p_hasWindowFuncs)
parseCheckWindowFuncs(pstate, qry);
in transformInsertStmt:
if (pstate->p_hasAggs)
ereport(ERROR,
(errcode(ERRCODE_GROUPING_ERROR),
 errmsg("cannot use aggregate function in VALUES"),
 parser_errposition(pstate,
locate_agg_of_level((Node *) qry, 0;
if (pstate->p_hasWindowFuncs)
ereport(ERROR,
(errcode(ERRCODE_WINDOWING_ERROR),
 errmsg("cannot use window function in VALUES"),
 parser_errposition(pstate,
locate_windowfunc((Node *) qry;
in transformUpdateStmt:
if (pstate->p_hasAggs)
ereport(ERROR,
(errcode(ERRCODE_GROUPING_ERROR),
 errmsg("cannot use aggregate function in UPDATE"),
 parser_errposition(pstate,
locate_agg_of_level((Node *) qry, 0;
if (pstate->p_hasWindowFuncs)
ereport(ERROR,
(errcode(ERRCODE_WINDOWING_ERROR),
 errmsg("cannot use window function in UPDATE"),
 parser_errposition(pstate,
locate_windowfunc((Node *) qry;

5. 2012-8-10 "Centralize the logic for detecting misplaced aggregates,
window funcs, etc.":
Remove ereport in update/insert.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/parser/analyze.c;h=6c3d89a14f6b1f19176864af4a0ea18eebd9f4bd;hp=93ef7246c51850aca1292e9d6388a0f97a0b;hb=eaccfded98a9c677d3a2e849c1747ec90e8596a6;hpb=b3055ab4fb5839a872bfe354b2b5ac31e6903ed6

Thanks


-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-10 Thread Etsuro Fujita 

On 2017/01/11 13:40, Ashutosh Bapat wrote:

CreateLocalJoinPath tries to construct a nestloop path for the given
join relation because it wants to avoid merge/hash join paths which
require expensive setup not required for EPQ. But it chooses cheap
paths for joining relations which may not be nestloop paths. So,
effectively it could happen that the whole alternate local plan would
be filled with hash/merge joins except the uppermost join.


In many cases the cheapest-total-cost outer and inner paths for a higher 
foreign-join relation would be foreign join paths, which would have 
nestloop paths as their fdw_outerpaths if not full joins.  So by 
redirection, the plan tree for EPQ would be mostly constructed by 
nestloop joins.  No?



Or it can
have foreign paths in there, which will need redirection. That's not
very good.


Maybe I'm missing something, but redirection isn't a problem.


We have to choose one of the following
alternatives

1. develop a mechanism to remember epq path for joining relations so
that it's available to CreateLocalJoinPath
2.let the caller pass epq local paths for joining relations to
CreateLocalJoinPath. How it remembers those, is FDW's problem.


I thought such a thing, but I think that would be overcomplicated, as 
pointed out by Tom [2].



2. Fix existing code by applying patch from [1]


As I said before, that might be fine for 9.6, but I don't think it's a 
good idea to search the pathlist because once we support parameterized 
foreign join paths, which is on my TODOs, we would have to traverse 
through the possibly-lengthy pathlist to find a local-join path, as 
mentioned in [3].


Best regards,
Etsuro Fujita

[2] https://www.postgresql.org/message-id/12565.1481904788%40sss.pgh.pa.us
[3] 
https://www.postgresql.org/message-id/c1075e4e-8297-5cf6-3f30-cb21266aa2ee%40lab.ntt.co.jp





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

[HACKERS] Obsolete reference to ParallelMain() in a comment

2017-01-10 Thread Amit Langote 
The comment above ParallelQueryMain() still refers to ParallelMain() as
its caller which is no longer the case.  Attached fixes that.

Thanks,
Amit
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index 86d9fb59ba..6cf62daab8 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -742,10 +742,11 @@ ExecParallelInitializeWorker(PlanState *planstate, shm_toc *toc)
 /*
  * Main entrypoint for parallel query worker processes.
  *
- * We reach this function from ParallelMain, so the setup necessary to create
- * a sensible parallel environment has already been done; ParallelMain worries
- * about stuff like the transaction state, combo CID mappings, and GUC values,
- * so we don't need to deal with any of that here.
+ * We reach this function from ParallelWorkerMain, so the setup necessary to
+ * create a sensible parallel environment has already been done;
+ * ParallelWorkerMain worries about stuff like the transaction state, combo
+ * CID mappings, and GUC values, so we don't need to deal with any of that
+ * here.
  *
  * Our job is to deal with concerns specific to the executor.  The parallel
  * group leader will have stored a serialized PlannedStmt, and it's our job

-- 
Sent 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 to implement pg_current_logfile() function

2017-01-10 Thread Michael Paquier 
On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas  wrote:
> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  
> wrote:
>> Applied in last version of the patch v18 as well as removing of an
>> unused variable.
>
> OK, this looks a lot closer to being committable.  But:
>
> [long review]

Gilles, could you update the patch based on this review from Robert? I
am marking the patch as "waiting on author" for the time being. I
looked a bit at the patch but did not notice anything wrong with it,
but let's see with a fresh version...
-- 
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] parallelize queries containing subplans

2017-01-10 Thread Robert Haas 
On Wed, Dec 28, 2016 at 1:17 AM, Amit Kapila  wrote:
> Currently, queries that have references to SubPlans or
> AlternativeSubPlans are considered parallel-restricted.  I think we
> can lift this restriction in many cases especially when SubPlans are
> parallel-safe.  To make this work, we need to propagate the
> parallel-safety information from path node to plan node and the same
> could be easily done while creating a plan.  Another option could be
> that instead of propagating parallel-safety information from path to
> plan, we can find out from the plan if it is parallel-safe (doesn't
> contain any parallel-aware node) by traversing whole plan tree, but I
> think it is a waste of cycles.  Once we have parallel-safety
> information in the plan, we can use that for detection of
> parallel-safe expressions in max_parallel_hazard_walker().  Finally,
> we can pass all the subplans to workers during plan serialization in
> ExecSerializePlan().  This will enable workers to execute subplans
> that are referred in parallel part of the plan.  Now, we might be able
> to optimize it such that we pass only subplans that are referred in
> parallel portion of plan, but I am not sure if it is worth the trouble
> because it is one-time cost and much lesser than other things we do
> (like creating
> dsm, launching workers).

It seems unfortunate to have to add a parallel_safe flag to the
finished plan; the whole reason we have the Path-Plan distinction is
so that we can throw away information that won't be needed at
execution time.  The parallel_safe flag is, in fact, not needed at
execution time, but just for further planning.  Isn't there some way
that we can remember, at the time when a sublink is converted to a
subplan, whether or not the subplan was created from a parallel-safe
path?  That seems like it would be cleaner than maintaining this flag
for all plans.

-- 
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: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 12:22 PM, Tom Lane  wrote:
> However, that complaint was already lodged in another thread.  What I
> think *this* thread is about is whether we ought to switch from the
> up-to-now-project-standard style
>
> ... how to frob your wug (see ) ...
>
> to
>
> ... how to frob your wug ...

I'm not sure if what you describe as existing style is in fact
standard practice.  I say that because it's not really my practice.  I
tend to use  if there is a way that I can incorporate the link
text into the sentence without contortions or parentheses; if not, I
use .  That's not exactly either of the styles you are
mentioning here.  It also means that it's quite likely that there are
places where changing what xref produces will make the markup I
committed produce not-so-nice output.  I'd be included to write the
above as something like:

...how to frob your wug, as further discussed in .

> The second way is better adapted to modern doc-reading environments, IMO,
> because it doesn't distract you with a parenthetical remark.  But it loses
> in output formats that don't have hyperlinks, or at least so I'd expect.

I agree, but I think "working well in environments without hyperlinks"
should be something close to a non-goal, perhaps even an anti-goal.
If there's no loss from working well in such a format, fine; if there
is, I'd rather cater to the 99.99% of people whose reading environment
includes links rather than the other 0.01% of people.

> (Possibly an output format like that would insert footnotes, but I've
> always found that a footnote marker every few words is really distracting
> too.)

+1.

> If we did start doing things this way, we wouldn't care so much what
>  produces because we wouldn't be using it anymore anyway.
> Not that that's a good reason to accept the inconsistency.

Since I've spent a fair amount of brainpower trying to use 
rather than  where possible, I'm not innately enthusiastic about
a project whose end is to get rid of .  I won't lose a lot of
sleep over it if we decide to go that direction, though.

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

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 12:24 PM, Joshua D. Drake  
wrote:
> In human terms, C is the only one of these that has been around long enough
> to realize it isn't a teenager (or child really), and although you may still
> be able to do the things you could in your 20s, you are going to pay for
> them the next day.

On aging, tell me about it.

On language selection, if I were working at Google, I'd be fine with
doing my next project in Go, because if Go goes away, then either
Google will pay me or someone else to rewrite all of my code, or
they'll be the ones to suffer the fallout of telling me to write in Go
in the first place.  Either way, no problem.  If I were working at a a
startup that will either fail or go public within 5 years, Go would be
fine for that, too.  If Google stops supporting it, by the time they'd
be likely to make any decisions that would adversely affect the
company, I'd be either rich or employed elsewhere.  But if I were
starting a database project that I hoped or expected to last another
20 years, I'm not sure I'd want to be tied to a language with less
than 10 years of history.

We've often talked about the value of having a PostgreSQL community
which is not controlled by any one company.  We'd lose that at least
some of that value if our entire code base were written in a language
controlled by one company.  C is boring, but it's not going away.
It's also extremely fast and resource-efficient, and it's got a big,
rich ecosystem of tools and libraries around it.  There are all sorts
of things that are best written in some other language, but for system
software it's hard to beat.

-- 
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: [DOCS] [HACKERS] Questionable tag usage

2017-01-10 Thread Tom Lane 
Robert Haas  writes:
> Personally, I think that if the doc toolchain changeover changed the
> way xrefs render - and it seems that it did - that's a bug that ought
> to be fixed,

I quite agree.  We'll have enough to do with the toolchain changeover;
we don't need random changes in what common markup produces.

However, that complaint was already lodged in another thread.  What I
think *this* thread is about is whether we ought to switch from the
up-to-now-project-standard style

... how to frob your wug (see ) ...

to

... how to frob your wug ...

The second way is better adapted to modern doc-reading environments, IMO,
because it doesn't distract you with a parenthetical remark.  But it loses
in output formats that don't have hyperlinks, or at least so I'd expect.
(Possibly an output format like that would insert footnotes, but I've
always found that a footnote marker every few words is really distracting
too.)

If we did start doing things this way, we wouldn't care so much what
 produces because we wouldn't be using it anymore anyway.
Not that that's a good reason to accept the inconsistency.

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] Placement of InvokeObjectPostAlterHook calls

2017-01-10 Thread Tom Lane 
Robert Haas  writes:
> On Fri, Jan 6, 2017 at 12:53 PM, Tom Lane  wrote:
>> While reviewing Etsuro-san's patch to force replanning after FDW option
>> changes, I noticed that there is a great lack of consistency about where
>> InvokeObjectPostAlterHook calls have been placed relative to other actions
>> such as forced relcache invals.

> I remember working pretty hard to make this consistent when that code
> went in.  In particular, I think the rule I tried to follow was to
> place the hooks just after the code that injects dependencies.  I
> don't know whether the inconsistencies you're seeing are due to (1)
> that being a poor rule of thumb, (2) that rule of thumb not being
> consistently followed at the time the original patch was committed, or
> (3) subsequent drift.

Well, what I was concerned about was specifically placement relative
to cache-invalidation calls.  But on reflection it may not matter, since
those really don't do anything except queue up actions to be taken at the
next CommandCounterIncrement boundary.  If we allowed the PostAlterHook
functions to do a CommandCounterIncrement then it would be problematic,
but I since found some comments indicating that they shouldn't do that.

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

2017-01-10 Thread Joshua D. Drake 

On 01/10/2017 08:12 AM, Robert Haas wrote:


Really?  What language would you pick in a vacuum?  The Linux kernel
is written in C, too, for pretty much the same reasons: it's the
canonical language for system software.  I don't deny that there may
be some newer languages out which could theoretically be used and work
well, but do any of them really have a development community and user
base around them that is robust enough that we'd want to be downstream
of it?  C has its annoyances, but its sheer pervasiveness is an
extremely appealing feature.


If we boil this down, I don't think any of this idea has to do with the 
fact that our database is written in C. I think it has to do with C is 
no longer "hip". We don't want to be hip. We are database people. Leave 
hip to MongoDB.


We want performance, stability, maturity and portability. (Not 
necessarily in that order).


There is not a single above hardware language (E.g; let's not rewrite in 
assembly) that provides those four requirements.


Rust is awesome. It is also 5 years old.
Go is awesome. It is also 8 years old.

C is awesome. It is 39 years old.

In human terms, C is the only one of these that has been around long 
enough to realize it isn't a teenager (or child really), and although 
you may still be able to do the things you could in your 20s, you are 
going to pay for them the next day.


JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
Sent 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_restore accepts -j -1

2017-01-10 Thread Ashutosh Bapat 
On Tue, Jan 10, 2017 at 10:18 AM, Stephen Frost  wrote:
> Greetings,
>
> For reasons which seem likely to be entirely unintentional, pg_restore
> will accept a '-1' for -j:
>
> pg_restore -j -1
>
> This seems to result in the parallel state being NULL and so things
> don't outright break, but it hardly seems likely to be what the user was
> asking for- my guess is that they actually wanted "parallel, single
> transaction", which we don't actually support:
>
> -> pg_restore -j 2 -1
> pg_restore: cannot specify both --single-transaction and multiple jobs
>
> We also don't accept -1 for pg_dump:
>
> -> pg_dump -j -1
> pg_dump: invalid number of parallel jobs
>
> If I'm missing something, please let me know, otherwise I'll plan to put
> the same check into pg_restore which exists in pg_dump.

Both the code blocks were added by 9e257a18, but I don't see any
description of why they are different in pg_dump.c and pg_restore.c.
In fact per comments in pg_restore.c, that condition should be same as
pg_dump.c. I am not sure whether it's just for windows specific
condition or the whole block. But I don't see any reason not to
replicate the same conditions in pg_restore.c

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Typo in dsa.c

2017-01-10 Thread Masahiko Sawada 
Hi,

Attached fixes comment typos in dsa.c.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


fix_typo_in_dsa_c.patch
Description: binary/octet-stream

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

Re: [HACKERS] [PATCH] Refactor "if(strspn(str, ...) == strlen(str)" code

2017-01-10 Thread Michael Paquier 
On Fri, Dec 9, 2016 at 3:23 AM, Aleksander Alekseev
 wrote:
>> You could just change it to
>> if (str[strspn(str, " \t\n\r\f")] == '\0')
>> to mitigate calling strlen. It's safe to do so because strspn will
>> only return values from 0 to strlen(str).
>
>> [...] I have serious doubts that the "optimized" implementation
>> you propose is actually faster than a naive one; it may be slower, and
>> it's certainly longer and harder to understand/test.
>
> I would like to point out that I never said it's optimized. However I
> like the code Geoff proposed. It definitely doesn't make anything worse.
> I decided to keep pg_str_contansonly procedure (i.e. not to inline this
> code) for now. Code with strspn looks OK in a simple example. However in
> a concrete context it looks like a bad Perl code in ROT13 to me:

Looking at this patch, I am not sure that it is worth worrying about.
This is a receipt to make back-patching a bit more complicated, and it
makes the code more complicated to understand. So I would vote for
rejecting it and move on.

By the way, as you are placing this routine in src/common/, you may
want to consider updating the code in src/bin/ that use libpqcommon.
-- 
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] WARM and indirect indexes

2017-01-10 Thread Bruce Momjian 
On Tue, Jan 10, 2017 at 11:36:24PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> 
> > 1.  What percentage speedup is the _average_ user going to get?  You
> > have to consider people who will use indirect indexes who get no benefit
> > or a net slowdown, and users who will get a benefit.
> > 
> > 2.  What percentage of users are going to use indirect indexes?
> > 
> > So, for #1 you might have users who are getting +1%, +50%, and -20%, so
> > maybe +10% average, and for #2 you might have 0.1%.  When you multiply
> > them out, you get 0.01% average improvement per installation, which is
> > very small.  Obviously, these are just wild guesses, but this is just to
> > make a point.
> 
> Perhaps not many users will require indirect indexes; but for those that
> do, the feature might be invaluable.  We don't do only things that
> benefit everybody -- some features are there to satisfy small
> populations ("snapshot too old" is a recent example).  We should of
> course do, and perhaps even favor doing things that benefit everybody,
> but should also do the other things.

I never said "We should do only things that benefit everybody," so why
are you saying that?  You are arguing against something I didn't say. I
am trying to make a balanced analysis, and you arguing against an
extreme position.

My point is that anything you add must be weighed against the value it
gives to users who use it, and the percentage of users who will use it.
Against that benefit, you have to look at the cost of exposing that API
to users, code complexity, maintenance, etc.

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

2017-01-10 Thread otheus uibk 
Joel Jacobson j...@trustly.com wrote:
> My motivation is primarily I don't want to learn all the over-complicated
details of C,

That's rich, mate. C is one of the simplest languages. It's simplicity is
its main benefit and its biggest drawback: it shields very little from the
actual underlying hardware and system. C++ is incredibly complex, while
still granting one access to the underlying system.  Every other high-level
language I've seen that shields its users from the "details", ultimately
suffers for it: either you the programmer must accept the limitations of
what the language provides at the cost of flexibility and power, or you
can't do what you really want to do, or you have to use lower-level
primitives to accomplish what you want.

Craig Ringer said:
> This is only partly a deficiency of C. Lots of it is down to low level
systems being complex, hard and varied. Weak vs strong memory ordering,
LP64 vs ILP64, etc etc etc.

Well-said. Adding to that: interprocess management and communication.

>  I suspect we'd land up having to move to C++ exceptions

Craig, isn't it the case that C++ exceptions still cause tremendous
slow-downs of the entire code-base?

Re: [HACKERS] Hash support for grouping sets

2017-01-10 Thread Robert Haas 
On Thu, Jan 5, 2017 at 10:48 PM, Andrew Gierth
 wrote:
> Herewith a patch for doing grouping sets via hashing or mixed hashing
> and sorting.

Cool.

-- 
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] pageinspect: Hash index support

2017-01-10 Thread Tom Lane 
Robert Haas  writes:
> No, you're missing the point.  The patch doesn't need to add
> pageinspect--1.6.sql at all, nor does it remove pageinspect--1.5.sql.
> It only needs to add pageinspect--1.5--1.6.sql and change the default
> version to 1.6.  No other changes are required.

Right, this is a new recommended process since commit 40b449ae8,
which see for rationale.

Use, eg, commit 11da83a0e as a model for extension update patches.

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] Re: new set of psql patches for loading (saving) data from (to) text, binary files

2017-01-10 Thread Pavel Stehule 
Hi

Thank you for review

2017-01-09 17:24 GMT+01:00 Jason O'Donnell :

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   not tested
> Documentation:tested, failed
>
> Pavel,
>
> gstore/gbstore:
>
> The functionality worked as expected - one row, one column results of
> queries can be sent to a file or shell.  It would be nice if a test case
> was included that proves results more than one row, one column wide will
> fail.
>

fixed


>
> The documentation included is awkward to read.  How about:
>
> "Sends the current query input buffer to the server and stores
> the result to an output file specified in the query or pipes the output
> to a shell command.  The file or command are written to only if the query
> successfully returns exactly one, non-null row and column.  If the
> query fails or does not return data, an error is raised. "
>

super

>
>
> Parameterized Queries:
>
> The functionality proposed works as expected.  Throughout the
> documentation, code and test cases the word "Parameterized" is spelled
> incorrectly: "PARAMETRIZED_QUERIES"
>

fixed

>
>
> set_from_file/set_from_bfile:
>
> The functionality proposed worked fine, I was able to set variables in sql
> from files.  Minor typo in the documentation:
> "The content is escapeaed as bytea value."
>

fixed


>
> Hope this helps!
>
> Jason O'Donnell
> Crunchy Data
>
> The new status of this patch is: Waiting on Author
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 9915731..4f95f86 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1944,6 +1944,28 @@ hello 10
 
   
 
+
+  
+\gstore [ filename ]
+\gstore [ |command ]
+\gbstore [ filename ]
+\gbstore [ |command ]
+
+
+ Sends the current query input buffer to the server and stores
+ the result to an output file specified in the query or pipes the output
+ to a shell command.  The file or command are written to only if the query
+ successfully returns exactly one, non-null row and column.  If the
+ query fails or does not return data, an error is raised.
+
+= SELECT avatar FROM users WHERE id = 123
+- \gbstore ~/avatar.png
+
+
+
+  
+
+
   
 \h or \help [ command ]
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 4139b77..33f4559 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -929,6 +929,27 @@ exec_command(const char *cmd,
 		status = PSQL_CMD_SEND;
 	}
 
+	/* \gstore [filename], \gbstore [filename] -- send query and store result in (binary) file */
+	else if (strcmp(cmd, "gstore") == 0 ||
+			  (strcmp(cmd, "gbstore") == 0))
+	{
+		char	   *fname = psql_scan_slash_option(scan_state,
+   OT_FILEPIPE, NULL, false);
+
+		if (!fname)
+			pset.gfname = pg_strdup("");
+		else
+		{
+			expand_tilde();
+			pset.gfname = pg_strdup(fname);
+		}
+
+		pset.raw_flag = true;
+		pset.binres_flag = (strcmp(cmd, "gbstore") == 0);
+		free(fname);
+		status = PSQL_CMD_SEND;
+	}
+
 	/* help */
 	else if (strcmp(cmd, "h") == 0 || strcmp(cmd, "help") == 0)
 	{
@@ -1064,7 +1085,6 @@ exec_command(const char *cmd,
 		free(opt2);
 	}
 
-
 	/* \o -- set query output */
 	else if (strcmp(cmd, "o") == 0 || strcmp(cmd, "out") == 0)
 	{
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index e1b04de..a6aaebe 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -854,6 +854,85 @@ StoreQueryTuple(const PGresult *result)
 	return success;
 }
 
+/*
+ * StoreRawResult: the returned value (possibly binary) is displayed
+ * or stored in file. The result should be exactly one row, one column.
+ */
+static bool
+StoreRawResult(const PGresult *result)
+{
+	bool		success = true;
+
+	if (PQntuples(result) < 1)
+	{
+		psql_error("no rows returned for \\gstore or \\gbstore\n");
+		success = false;
+	}
+	else if (PQntuples(result) > 1)
+	{
+		psql_error("more than one row returned for \\gstore or \\gbstore\n");
+		success = false;
+	}
+	else if (PQnfields(result) < 1)
+	{
+		psql_error("no columns returned for \\gstore or \\gbstore\n");
+		success = false;
+	}
+	else if (PQnfields(result) > 1)
+	{
+		psql_error("more than one column returned for \\gstore or \\gbstore\n");
+		success = false;
+	}
+	else if (PQgetisnull(result, 0, 0))
+	{
+		psql_error("returned value is null for \\gstore or \\gbstore\n");
+		success = false;
+	}
+	else
+	{
+		char	   *value;
+		int			length;
+		FILE	   *fout = NULL;
+		bool		is_pipe = false;
+
+		value = PQgetvalue(result, 0, 0);
+		length = PQgetlength(result, 0, 0);
+
+		if (pset.gfname

Re: [HACKERS] pageinspect: Hash index support

2017-01-10 Thread Robert Haas 
On Fri, Jan 6, 2017 at 1:14 AM, Ashutosh Sharma  wrote:
>> The previous patch was using pageinspect--1.5.sql as a base, and then uses
>> pageinspect--1.5-1.6.sql to upgrade to version 1.6.
>>
>> Removing pageinspect--1.5.sql, and adding pageinspect--1.6.sql with the
>> current interface will use pageinspect--1.6.sql directly where as existing
>> installations will use the upgrade scripts.
>>
>> Hence I don't see a reason why we should keep pageinspect--1.5.sql around
>> when we can provide a clear interface description in a pageinspect--1.6.sql.
>
> okay, agreed.

No, you're missing the point.  The patch doesn't need to add
pageinspect--1.6.sql at all, nor does it remove pageinspect--1.5.sql.
It only needs to add pageinspect--1.5--1.6.sql and change the default
version to 1.6.  No other changes are required.

-- 
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] pageinspect: Hash index support

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 12:19 PM, Jesper Pedersen
 wrote:
> Revised patched attached.

+itup = (IndexTuple) PageGetItem(uargs->page, id);
+
+MemSet(nulls, 0, sizeof(nulls));
+
+j = 0;
+values[j++] = UInt16GetDatum(uargs->offset);
+values[j++] = CStringGetTextDatum(psprintf("(%u,%u)",
+
BlockIdGetBlockNumber(&(itup->t_tid.ip_blkid)),
+itup->t_tid.ip_posid));
+
+ptr = (char *) itup + IndexInfoFindDataOffset(itup->t_info);
+dlen = IndexTupleSize(itup) - IndexInfoFindDataOffset(itup->t_info);

It seems like this could be used to index off the end of the page, if
you feed it invalid data.

+dump = palloc0(dlen * 3 + 1);

This is wasteful.  Just use palloc and install a terminating NUL byte instead.

+sprintf(dump, "%02x", *(ptr + off) & 0xff);

*(ptr + off) is normally written ptr[off].

+if (pageopaque->hasho_flag != LH_OVERFLOW_PAGE)
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("page is not an overflow page"),
+ errdetail("Expected %08x, got %08x.",
+LH_OVERFLOW_PAGE, pageopaque->hasho_flag)));

I think this is an unnecessary test given that you've already called
verify_hash_page().

+if (bitmappage >= metap->hashm_nmaps)
+elog(ERROR, "invalid overflow bit number %u", ovflbitno);

I think this should be an ereport(), because it's reachable given a
bogus page which a user might construct (or a corrupted page).

+test=# SELECT * FROM hash_page_items(get_raw_page('con_hash_index', 1));
+ itemoffset |  ctid   |  data
++-+-
+  1 | (3145728,14376) | 00 c0 ca 3e 00 00 00 00
+  2 | (3145728,14376) | 00 c0 ca 3e 00 00 00 00
+  3 | (3407872,14376) | 00 c0 ca 3e 00 00 00 00

Won't the first 4 bytes always be a hash code and the second 4 bytes
always 0?  Should we just return the hash code as an int4 or int8
instead of pretending it's a bunch of uninterpretable binary data?

+  hash_bitmap_info returns information about
+  the status of a bit for an overflow page in bitmap page of a
HASH
+  index. For example:
+
+test=# SELECT * FROM hash_bitmap_info('con_hash_index', 2050);
+ bitmapblkno | bitmapbit
+-+---
+  65 | 1
+

I find this hard to understand.  This says that it returns
information, but the nature of the returned information is unspecified
and in my opinion unclear.

-- 
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] PoC: Grouped base relation

2017-01-10 Thread Robert Haas 
On Mon, Jan 9, 2017 at 12:56 PM, Antonin Houska  wrote:
> Attached is a draft patch that lets partial aggregation happen at base
> relation level. If the relations contain relatively small number of groups,
> the number of input rows of the aggregation at the query level can be reduced
> this way.  Also, if append relation and postgres_fdw planning is enhanced
> accordingly, patch like this can let us aggregate individual tables on remote
> servers (e.g. shard nodes) and thus reduce the amount of rows subject to the
> final aggregation.

Very interesting.  I don't have time to study this in detail right
now, but as a concept it seems worthwhile.  I think the trick is
figuring out at which levels of the path tree it makes sense to
consider partial aggregation.

-- 
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] PoC: Grouped base relation

2017-01-10 Thread Pantelis Theodosiou 
On Mon, Jan 9, 2017 at 5:56 PM, Antonin Houska  wrote:

> Attached is a draft patch that lets partial aggregation happen at base
> relation level. If the relations contain relatively small number of groups,
> the number of input rows of the aggregation at the query level can be
> reduced
> this way.  Also, if append relation and postgres_fdw planning is enhanced
> accordingly, patch like this can let us aggregate individual tables on
> remote
> servers (e.g. shard nodes) and thus reduce the amount of rows subject to
> the
> final aggregation.
>
> For example, consider query
>
> SELECT b.j, sum(a.x) FROM a, b WHERE a.i = b.j GROUP BY b.j;
>
> and tables "a"
>
>  i | x
> ---
>  1 | 3
>  1 | 4
>
> and "b"
>
>  j
> ---
>  1
>  1
>

The example should have j= 1,2 , right?

and "b"

 j
---
 1
 2



> The base relations grouped look like
>
>  i | sum(a.x)| count(*)
> ---
>  1 |   7 |   2
>


Otherwise, the sum and count would be 14 and 4.


>
> and
>
>  j | count(*)
> -
>  1 |   2
>
>
>
>

Pantelis

Re: [HACKERS] PoC: Grouped base relation

2017-01-10 Thread Pantelis Theodosiou 
On Tue, Jan 10, 2017 at 6:52 PM, Pantelis Theodosiou 
wrote:

>
>
> On Mon, Jan 9, 2017 at 5:56 PM, Antonin Houska  wrote:
>
>> Attached is a draft patch that lets partial aggregation happen at base
>> relation level. If the relations contain relatively small number of
>> groups,
>> the number of input rows of the aggregation at the query level can be
>> reduced
>> this way.  Also, if append relation and postgres_fdw planning is enhanced
>> accordingly, patch like this can let us aggregate individual tables on
>> remote
>> servers (e.g. shard nodes) and thus reduce the amount of rows subject to
>> the
>> final aggregation.
>>
>> For example, consider query
>>
>> SELECT b.j, sum(a.x) FROM a, b WHERE a.i = b.j GROUP BY b.j;
>>
>> and tables "a"
>>
>>  i | x
>> ---
>>  1 | 3
>>  1 | 4
>>
>> and "b"
>>
>>  j
>> ---
>>  1
>>  1
>>
>
> The example should have j= 1,2 , right?
>
> and "b"
>
>  j
> ---
>  1
>  2
>
>
>
>> The base relations grouped look like
>>
>>  i | sum(a.x)| count(*)
>> ---
>>  1 |   7 |   2
>>
>
>
> Otherwise, the sum and count would be 14 and 4.
>
>
>>
>> and
>>
>>  j | count(*)
>> -
>>  1 |   2
>>
>>
>>
>>
>
> Pantelis
>

Or perhaps I should be reading more carefully the whole mail before
posting. Ignore the previous.

Re: [HACKERS] Replication/backup defaults

2017-01-10 Thread Robert Haas 
On Mon, Jan 9, 2017 at 11:02 AM, Peter Eisentraut
 wrote:
> On 1/9/17 7:44 AM, Magnus Hagander wrote:
>> So based on that, I suggest we go ahead and make the change to make both
>> the values 10 by default. And that we do that now, because that lets us
>> get it out through more testing on different platforms, so that we catch
>> issues earlier on if they do arise.
>
> Sounds good.

I may be outvoted, but I'm still not in favor of changing the default
wal_level.  That caters only to people who lack sufficient foresight
to know that they need a replica before the system becomes so critical
that they can't bounce it to update the configuration.

-- 
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] Add support to COMMENT ON CURRENT DATABASE

2017-01-10 Thread Robert Haas 
On Mon, Jan 9, 2017 at 3:14 PM, Peter Eisentraut
 wrote:
> To achieve consistent support for specifying the current database, we
> would need to change the grammar for every command involving databases.

I wouldn't have thought there would be all that many of those, though.

> And it would also set a precedent for similar commands, such as current
> user/role.

True.  Maybe it's a GOOD precedent, though.

> Plus support in psql, pg_dump, etc. would get more complicated.

I'm not totally convinced...

> Instead, it would be simpler to define a grammar symbol like
>
> database_name: ColId | CURRENT_DATABASE
>
> and make a small analogous change in objectaddress.c and you're done.
>
> Compare rolespec in gram.y.

...but that's certainly an existing precedent for your proposal.

-- 
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] Cache Hash Index meta page.

2017-01-10 Thread Robert Haas 
On Tue, Dec 27, 2016 at 3:06 AM, Mithun Cy  wrote:
> Thanks, just like _bt_getroot I am introducing a new function
> _hash_getbucketbuf_from_hashkey() which will give the target bucket
> buffer for the given hashkey. This will use the cached metapage
> contents instead of reading meta page buffer if cached data is valid.
> There are 2 places which can use this service 1. _hash_first and 2.
> _hash_doinsert.

Can we adapt the ad-hoc caching logic in hashbulkdelete() to work with
this new logic?  Or at least update the comments?

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

[HACKERS] WARM and indirect indexes

2017-01-10 Thread Alvaro Herrera 
Two options are on the table to attack the problem of updates causing
write amplification: WARM and indirect indexes.  They are completely
different approaches but have overlapping effects on what scenarios are
improved.  Here's a recap of both features, with the intent that we make
a well-considered decision about each.

The main effect of both features is that an updated tuple doesn't
require updating indexes that are on unmodified columns.  Indirect
indexes are a completely new server feature which may enable other
refinements later on; WARM is a targeted optimization on top of the HOT
optimization.  

The big advantage of WARM is that it works automatically, like HOT: the
user doesn't need to do anything different than today to get the
benefit.  With indirect indexes, the user needs to create the index as
indirect explicitely.

There are two big disadvantages to WARM (as to HOT): it cannot be
applied when the heap page is full; and concurrent long-running
transactions can spoil reclaimability of recently dead tuples in heap
pages.  There's a further disadvantage: currently, there can be only one
WARM update in an update chain.  (Pavan believes it is possible to allow
multiple ones.)  All those cases can benefit from indirect indexes.

Another interesting case is a table with a primary key and a JSON
object, on which you have a GIN index (or an int[] array, or text
search).  What happens if you modify the JSON?  With WARM, this is just
a normal index update.  With indirect indexes, you may be able to skip
inserting index entries for all the JSON elements except those which
changed.  (However, this is not implemented yet.)

- When scanning a WARM-updated block starting from an index, you may
need to do more work to walk the update chain until you find the visible
tuple.  Most of the time, HOT/WARM chains are very short thanks to HOT
pruning, so this shouldn't be a problem.

- Indirect indexes require a primary key to be present.  If the PK is
dropped, the IndIx must be dropped too.

- Indirect indexes become larger if the primary key is wide.

- Indirect indexes are not fully implemented yet (need to remove
restriction of PK value being 6 bytes; also wholesale vacuuming, though
there's no universal agreement that this is strictly necessary.)

- An indirect index takes longer to read, since it needs to descend both
the IndIx itself and the primary key index.

-- 
Álvaro Herrera


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

Re: [HACKERS] Replication/backup defaults

2017-01-10 Thread Magnus Hagander 
On Tue, Jan 10, 2017 at 8:03 PM, Robert Haas  wrote:

> On Mon, Jan 9, 2017 at 11:02 AM, Peter Eisentraut
>  wrote:
> > On 1/9/17 7:44 AM, Magnus Hagander wrote:
> >> So based on that, I suggest we go ahead and make the change to make both
> >> the values 10 by default. And that we do that now, because that lets us
> >> get it out through more testing on different platforms, so that we catch
> >> issues earlier on if they do arise.
> >
> > Sounds good.
>
> I may be outvoted, but I'm still not in favor of changing the default
> wal_level.  That caters only to people who lack sufficient foresight
> to know that they need a replica before the system becomes so critical
> that they can't bounce it to update the configuration.
>

True. But the current level only caters to those people who run large ETL
jobs without doing any tuning on their system (at least none that would
require a restart), or another one of the fairly specific workloads.

And as I keep re-iterating, it's not just about replicas, it's also about
the ability to make proper backups. Which is a pretty fundamental feature.

I do think you are outvoted, yes :) At least that's the result of my
tallying up the people who have spoken out on the thread.

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

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 1:31 AM, Fabien COELHO  wrote:
> I have submitted a proof of this fact in the form of a counter example which
> (1) (pseudo) implements the use-case by logging into an audit table the fact
> a user accesses the secure level (2) shows that the status of a non
> transactional session variable used for keeping this status is wrong for the
> use case in some cases (it says that all is well while appending to the
> audit table failed).
>
> I feel entitled to point out to other people that their belief that a
> feature as described provides a correct solution to a particular use case is
> wrong, if it is factually the case. If they persist in this belief despite
> the submitted proof, I can only be sad about it, because if pg provides a
> feature for a security-relared use case which does not work correctly it is
> just shooting one's foot.

You're just ignoring explanations from other people - Craig in
particular - about why it DOES satisfy their use case.  And the reason
his argument is valid is because he is questioning your premise.  You
are proving "if A, then B" and he's saying, "yes, but not A".  That's
not a logical fallacy on his part.  That's you proving something that
is in his view irrelevant to the desirability of the feature.

> I do not like Pavel's feature, this is a subjective opinion. This feature
> does not provide a correct solution for the use case, this is an objective
> fact. The presented feature does not have a real use case, this is too bad.

If the presented feature had no use case, I don't think there would be
3 or 4 people arguing for it.  Those people aren't stupid.

> Finally, I did not "veto" this feature, I reviewed it in depth and concluded
> negatively.

Sure, that's pretty fair.  Are you also willing to accept other
people's differing conclusions?

> You are a committer and I'm just a "silly academic", you do not
> have to listen to anything I say and can take majority votes against proofs
> if you want.

I believe that the words "silly" and "academic" were used about
certain proposals that you made, and you have here pulled them out of
the context in which they were written and recast them as general
judgements on you rather than statements about certain ideas which you
proposed or certain arguments which you made.  I think most people on
this mailing list, including me, are very careful to avoid "ad
hominum" arguments, and I believe that is also the case in the
arguments made to you.  Everybody's ideas on this mailing list,
including mine, come in for criticism from time to time.  That doesn't
necessarily imply personal disrespect.

-- 
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] merging some features from plpgsql2 project

2017-01-10 Thread Peter Eisentraut 
On 1/10/17 12:06 AM, Pavel Stehule wrote:
> A check how much rows was impacted by query is relative often task. So
> we can do this task more user friendly.
> 
> Second motivation - ROW_COUNT is working for static and for dynamic SQL
> - it can be partial replace of FOUND variable.

What is stopping anyone from claiming that their favorite diagnostic
item is also a relatively often task and request it to become an
automatic variable?  Where does it stop?

It's not like PL/pgSQL is the king of brevity.  Creating inconsistent
and arbitrary warts to save a few characters does not appear appealing.

-- 
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] _hash_addovflpage has a bug

2017-01-10 Thread Robert Haas 
On Mon, Jan 9, 2017 at 10:46 PM, Amit Kapila  wrote:
> Yeah, we can write code that way, but then it is better to rely just
> on retain_pin variable in the function and add an Assert for bucket
> page whenever we are retaining pin.  How about doing something like
> attached patch?

Committed.

>>  Not sure exactly how that
>> works out in terms of locking.
>
> We have to change the locking order as mentioned above by me.  This
> change is already present in that patch, so maybe we add the check as
> suggested by you along with that patch.  Now, another thing we could
> do is to extract those changes from WAL patch, but I am not sure if it
> is worth the effort.

I'm not sure at this point, either.

-- 
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] Microvacuum support for Hash Index

2017-01-10 Thread Jesper Pedersen 

Hi Ashutosh,

On 01/10/2017 05:24 AM, Ashutosh Sharma wrote:

Thanks for reporting this problem. It is basically coming because i
forgot to unpin the bucketbuf in hash_xlog_vacuum_one_page(). Please
find the attached v5 patch that fixes the issue.



The crash is now fixed, but the

--- test.sql ---
\set id random(1,10)
\set val random(0,10)
BEGIN;
UPDATE test SET val = :val WHERE id = :id;
COMMIT;
--- test.sql ---

case gives

client 6 aborted in command 3 of script 0; ERROR:  deadlock detected
DETAIL:  Process 14608 waits for ShareLock on transaction 1444620; 
blocked by process 14610.
Process 14610 waits for ShareLock on transaction 1444616; blocked by 
process 14608.

HINT:  See server log for query details.
CONTEXT:  while rechecking updated tuple (12,3) in relation "test"
...

using pgbench -M prepared -c 10 -j 10 -T 300 -f test.sql test

Best regards,
 Jesper



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

Re: [HACKERS] merging some features from plpgsql2 project

2017-01-10 Thread Pavel Stehule 
2017-01-10 14:26 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 1/10/17 12:06 AM, Pavel Stehule wrote:
> > A check how much rows was impacted by query is relative often task. So
> > we can do this task more user friendly.
> >
> > Second motivation - ROW_COUNT is working for static and for dynamic SQL
> > - it can be partial replace of FOUND variable.
>
> What is stopping anyone from claiming that their favorite diagnostic
> item is also a relatively often task and request it to become an
> automatic variable?  Where does it stop?
>

There is only two possible fields - ROW_COUNT and RESULT_OID. Result Oid is
not almost unused today. So stop is ROW_COUNT


> It's not like PL/pgSQL is the king of brevity.  Creating inconsistent
> and arbitrary warts to save a few characters does not appear appealing.
>

yes

Regards

Pavel


>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] RustgreSQL

2017-01-10 Thread Jan de Visser 
On Monday, January 9, 2017 7:39:49 PM EST Joel Jacobson wrote:
> On Mon, Jan 9, 2017 at 6:03 PM, Craig Ringer  wrote:
> > Immutable functions can and do use functionality from all over the
> > server. They just don't depend on user-visible mutable _state_
> > elsewhere in the server.
> 
> OK, fair point, but at least the functionality *could* be written without
> using existing C functions, since its only the input that determine what
> output will be returned. The dependencies used by the immutable
> functions can also be ported, function by function, until there are
> no dependencies.

Be that as it may, I don't think you have convinced anybody that that is 
something worth doing. The fact it *could* be done doesn't mean it *should* be 
done.

You're proposing to introduce a metric eff-ton of instability in a project 
that routinely spends ten-message email threads discussing changing an elog to 
ereport.

To give you some perspective: *everybody* agrees autotools (the mechanism used 
to generate makefiles) is aweful. Everybody. About a year ago somebody showed 
saying "Hey, I have a draft patch replacing autotools with cmake". Cmake is 
infinitely better (mostly because it was developed in this century as opposed 
to the early 80s, and so is more in tune with current toolchains). Yury has 
been working on it for a year now, and I personally don't think it's going to 
land in version 10. And this is "just" the make infrastructure.

What you are proposing is not going to happen unless you get some serious buy-
in from a significant number of veteran contributors. And those are exactly the 
people that say "C? What's the problem?"



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

Re: [HACKERS] merging some features from plpgsql2 project

2017-01-10 Thread Marko Tiikkaja 
On Tue, Jan 10, 2017 at 2:26 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> It's not like PL/pgSQL is the king of brevity.


This is essentially saying "PL/PgSQL isn't perfect, so we shouldn't try and
make it better".  I hear this argument a lot, and as long as people keep
rejecting improvements for this reason they can keep saying it.  It's a
self-fulfilling prophecy.


.m

Re: [HACKERS] RustgreSQL

2017-01-10 Thread Robert Haas 
On Tue, Jan 10, 2017 at 8:42 AM, Jan de Visser  wrote:
> Be that as it may, I don't think you have convinced anybody that that is
> something worth doing. The fact it *could* be done doesn't mean it *should* be
> done.

+1.

> What you are proposing is not going to happen unless you get some serious buy-
> in from a significant number of veteran contributors. And those are exactly 
> the
> people that say "C? What's the problem?"

+1.

I'm not meaning to be funny or sarcastic or disrespectful when I say
that I think C is the best possible language for PostgreSQL.  It works
great, and we've got a ton of investment in making it work.  I can't
see why we'd want to start converting even a part of the code to
something else.  Perhaps it seems like a good idea from 10,000 feet,
but in practice I believe it would be fraught with difficulties - and
if it injected even a few additional instructions into hot code paths,
it would be a performance loser.

-- 
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] pg_stat_lwlock wait time view

2017-01-10 Thread Robert Haas 
On Mon, Jan 9, 2017 at 12:13 AM, Haribabu Kommi
 wrote:
> Whenever the Backend is waiting for an LWLock, it sends the message to
> "stats collector" with PID and wait_event_info of the lock. Once the stats
> collector receives the message, Adds that Backend entry to Hash table after
> getting the start time. Once the Backend ends the waiting for the Lock, it
> sends the signal to the "stats collector" and it gets the entry from Hash
> table
> and finds out the wait time and update this time to the corresponding LWLock
> entry in another Hash table.

I will be extremely surprised if this doesn't have a severe negative
impact on performance when LWLock contention is high (e.g. a pgbench
read-only test using a scale factor that fits in the OS cache but not
shared_buffers).

-- 
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] Logical Replication WIP

2017-01-10 Thread Peter Eisentraut 
On 1/2/17 8:32 AM, Petr Jelinek wrote:
> On 02/01/17 05:23, Steve Singer wrote:
>> but I can't drop the subscription either
>>
>>
>> test_b=# drop subscription mysub;
>> ERROR:  could not drop replication origin with OID 1, in use by PID 24996
>>
>>  alter subscription mysub disable;
>> ALTER SUBSCRIPTION
>> drop subscription mysub;
>> ERROR:  could not drop replication origin with OID 1, in use by PID 24996
>>
>> drop subscription mysub nodrop slot;
>>
>> doesn't work either.  If I first drop the working/active subscription on
>> the original 'test' database it works but I can't seem to drop the
>> subscription record on test_b

I can't reproduce this exactly, but I notice that CREATE SUBSCRIPTION
NOCREATE SLOT does not create a replication origin, but DROP
SUBSCRIPTION NODROP SLOT does attempt to drop the origin.  If the origin
is not in use, it will just go away, but if it is in use, it might lead
to the situation described above, where the second subscription cannot
be removed.

> I guess this is because replication origins are pg instance global and
> we use subscription name for origin name internally. Maybe we need to
> prefix/suffix it with db oid or something like that, but that's
> problematic a bit as well as they both have same length limit. I guess
> we could use subscription OID as replication origin name which is
> somewhat less user friendly in terms of debugging but would be unique.

I think the most robust way would be to associate origins to
subscriptions using the object dependency mechanism, and just pick an
internal name like we do for automatically created indexes or sequences,
for example.

-- 
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] GSoC 2017

2017-01-10 Thread Atri Sharma 
Count me in as a mentor

On 10-Jan-2017 3:24 PM, "Alexander Korotkov" 
wrote:

> Hi all!
>
> In 2016 PostgreSQL project didn't pass to GSoC program.  In my
> understanding the reasons for that are following.
>
> 1. We did last-minute submission of our application to GSoC.
> 2. In 2016 GSoC application form for mentoring organizations has been
> changed.  In particular, it required more detailed information about
> possible project.
>
> As result we didn't manage to make a good enough application that time.
> Thus, our application was declined. See [1] and [2] for details.
>
> I think that the right way to manage this in 2017 would be to start
> collecting required information in advance.  According to GSoC 2017
> timeline [3] mentoring organization can submit their applications from
> January 19 to February 9.  Thus, now it's a good time to start collecting
> project ideas and make call for mentors.  Also, we need to decide who would
> be our admin this year.
>
> In sum, we have following questions:
> 1. What project ideas we have?
> 2. Who are going to be mentors this year?
> 3. Who is going to be project admin this year?
>
> BTW, I'm ready to be mentor this year.  I'm also open to be an admin if
> needed.
>
> [1] https://www.postgresql.org/message-id/flat/CAA-
> aLv4p1jfuMpsRaY2jDUQqypkEXUxeb7z8Mp-0mW6M03St7A%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/flat/CALxAEPuGpAjBSN-
> PTuxHfuLLqDS47BEbO_ZYxUYQR3ud1nwbww%40mail.gmail.com
> [3] https://developers.google.com/open-source/gsoc/timeline
>
> --
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Re: [HACKERS] GSoC 2017

2017-01-10 Thread Andrew Borodin 
2017-01-10 14:53 GMT+05:00 Alexander Korotkov :
> 1. What project ideas we have?

Hi!
I'd like to propose project on sorting algorithm research. I’m ready
to be a mentor on this project.

===Topic===
Sorting algorithms benchmark and implementation.

===Idea===
Currently the PostgreSQL uses Hoare’s Quicksort implementation based
on work of Bentley and McIlroy [1] from 1993, while there exist some
more novel algorithms [2], [3], and [4] which are actively used by
highly optimized code like Java and .NET. Probably, use of optimized
sorting algorithm could yield general system performance improvement.
Also, use of non-comparison based algorithms deserves attention and
benchmarking [5].

===Project details===
The project has four essential parts:
1.   Implementation of benchmark for sorting. Making sure that
operations using sorting are represented proportionally to some
“average” use cases.
2.   Selection of benchmark algorithms. Selection can be based,
for example, on scientific papers or community opinions.
3.   Benchmark implementation of selected algorithms. Analysis of
results, picking of winner.
4.   Industrial implementation for pg_qsort(), pg_qsort_args() and
gen_qsort_tuple.pl. Implemented patch is submitted to commitfest,
other patch is reviewed by the student.

[1] Bentley, Jon L., and M. Douglas McIlroy. "Engineering a sort
function." Software: Practice and Experience 23.11 (1993): 1249-1265.
[2] Musser, David R. "Introspective sorting and selection algorithms."
Softw., Pract. Exper. 27.8 (1997): 983-993.
[3] Auger, Nicolas, Cyril Nicaud, and Carine Pivoteau. "Merge
Strategies: from Merge Sort to TimSort." (2015).
[4] Beniwal, Sonal, and Deepti Grover. "Comparison of various sorting
algorithms: A review." International Journal of Emerging Research in
Management  2 (2013).
[5] Mcllroy, Peter M., Keith Bostic, and M. Douglas Mcllroy.
"Engineering radix sort." Computing systems 6.1 (1993): 5-27.

Best regards, Andrey Borodin.


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

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Craig Ringer 
On 10 January 2017 at 14:31, Fabien COELHO  wrote:
> I do not like Pavel's feature, this is a subjective opinion. This feature
> does not provide a correct solution for the use case, this is an objective
> fact. The presented feature does not have a real use case, this is too bad.

Oh, also, you might want to tell Oracle and the many people who use
package variables that.

Now, that said, huge numbers of people blindly do all sorts of unsafe
things and mostly get away with it. Using MERGE in concurrent OLTP
workloads. Racey upserts. Blindly assuming xacts will succeed and not
keeping the info around to retry them until confirmation of commit is
received. That sort of business.

Nonetheless, it's pretty clear they're far from having a "real use case".

I'd like to see transactional vars. I think it's worthwhile and you've
made a reasonable argument that they're useful, and should probably
even be the default. Your unwillingness to listen to anyone else isn't
doing your argument any favours though.

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


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

Re: [HACKERS] Declarative partitioning - another take

2017-01-10 Thread Amit Langote 
On 2017/01/05 5:50, Robert Haas wrote:
> On Tue, Dec 27, 2016 at 3:59 AM, Amit Langote
>  wrote:
>> Patches 0001 to 0006 unchanged.
>
> Committed 0001 earlier, as mentioned in a separate email.  Committed
> 0002 and part of 0003.

Thanks!  I realized however that the approach I used in 0002 of passing
the original slot to ExecConstraints() fails in certain situations.  For
example, if a BR trigger changes the tuple, the original slot would not
receive those changes, so it will be wrong to use such a tuple anymore.
In attached 0001, I switched back to the approach of converting the
partition-tupdesc-based tuple back to the root partitioned table's format.
 The converted tuple contains the changes by BR triggers, if any.  Sorry
about some unnecessary work.

> But I'm skeptical that the as-patched-by-0003
> logic in generate_partition_qual() makes sense.  You do this:
>
> result = list_concat(generate_partition_qual(parent),
>  copyObject(rel->rd_partcheck));
>
> /* Mark Vars with correct attnos */
> result = map_partition_varattnos(result, rel, parent);
>
> But that has the effect of applying map_partition_varattnos to
> everything in rel->rd_partcheck in addition to applying it to
> everything returned by generate_partition_qual() on the parent, which
> doesn't seem right.

I've replaced this portion of the code with (as also mentioned below):

/* Quick copy */
if (rel->rd_partcheck != NIL)
return copyObject(rel->rd_partcheck);

Down below (for the case when the partition qual is not cached, we now do
this:

my_qual = get_qual_from_partbound(rel, parent, bound);

/* Add the parent's quals to the list (if any) */
if (parent->rd_rel->relispartition)
result = list_concat(generate_partition_qual(parent), my_qual);
else
result = my_qual;

/*
 * Change Vars to have partition's attnos instead of the parent's.
 * We do this after we concatenate the parent's quals, because
 * we want every Var in it to bear this relation's attnos.
 */
result = map_partition_varattnos(result, rel, parent);

Which is then cached wholly in rd_partcheck.

As for your concern whether it's correct to do so, consider that doing
generate_partition_qual() on the parent returns qual with Vars that bear
the parent's attnos (which is OK as far parent as partition is concerned).
 To apply the qual to the current relation as partition, we must change
the Vars to have this relation's attnos.

> Also, don't we want to do map_partition_varattnos() just ONCE, rather
> than on every call to this function?  I think maybe your concern is
> that the parent might be changed without a relcache flush on the
> child, but I don't quite see how that could happen.  If the parent's
> tuple descriptor changes, surely the child's tuple descriptor would
> have to be altered at the same time...

Makes sense.  I fixed so that we return copyObject(rel->rd_partcheck), if
it's non-NIL, instead of generating parent's qual and doing the mapping
again.  For some reason, I thought we couldn't save the mapped version in
the relcache.

By the way, in addition to the previously mentioned bug of RETURNING, I
found that WITH CHECK OPTION didn't work correctly as well.  In fact
automatically updatable views failed to consider partitioned tables at
all.  Patch 0007 is addressed towards fixing that.

Thanks,
Amit
>From e408234633c01817d6a2313fdbdccdb4f0057c1e Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 6 Jan 2017 15:53:10 +0900
Subject: [PATCH 1/7] Fix reporting of violation in ExecConstraints, again

We decided in f1b4c771ea74f42447dccaed42ffcdcccf3aa694 that passing
the original slot (one containing the tuple formatted per root
partitioned table's tupdesc) to ExecConstraints(), but that breaks
certain cases.  Imagine what would happen if a BR trigger changed the
tuple - the original slot would not contain those changes.
So, it seems better to convert (if necessary) the tuple formatted
per partition tupdesc after tuple-routing back to the root table's
format and use the converted tuple to make val_desc shown in the
message if an error occurs.
---
 src/backend/commands/copy.c|  6 ++--
 src/backend/executor/execMain.c| 53 +-
 src/backend/executor/nodeModifyTable.c |  5 ++--
 src/include/executor/executor.h|  3 +-
 src/test/regress/expected/insert.out   | 18 ++--
 src/test/regress/sql/insert.sql| 17 ++-
 6 files changed, 82 insertions(+), 20 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f56b2ac49b..65eb167087 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2491,8 +2491,7 @@ CopyFrom(CopyState cstate)
 
 	for (;;)
 	{
-		TupleTableSlot *slot,
-	   *oldslot;
+		TupleTableSlot *slot;
 		bool		skip_tuple;
 		Oid			loaded_oid = InvalidOid;
 
@@

Re: [HACKERS] Microvacuum support for Hash Index

2017-01-10 Thread Ashutosh Sharma 
Hi Jesper,

> However (master / WAL v7 / MV v4),
>
> --- ddl.sql ---
> CREATE TABLE test AS SELECT generate_series(1, 10) AS id, 0 AS val;
> CREATE INDEX IF NOT EXISTS idx_id ON test USING hash (id);
> CREATE INDEX IF NOT EXISTS idx_val ON test USING hash (val);
> ANALYZE;
> --- ddl.sql ---
>
> --- test.sql ---
> \set id random(1,10)
> \set val random(0,10)
> BEGIN;
> DELETE FROM test WHERE id = :id;
> INSERT INTO test VALUES (:id, :val);
> COMMIT;
> --- test.sql ---
>
> gives
>
> #9  0x0098a83e in elog_finish (elevel=20, fmt=0xb6ea92 "incorrect
> local pin count: %d") at elog.c:1378
> #10 0x007f0b33 in LockBufferForCleanup (buffer=1677) at
> bufmgr.c:3605
> #11 0x00549390 in XLogReadBufferForRedoExtended (record=0x2afced8,
> block_id=1 '\001', mode=RBM_NORMAL, get_cleanup_lock=1 '\001',
> buf=0x7ffe3ee27c8c) at xlogutils.c:394
> #12 0x004c5026 in hash_xlog_vacuum_one_page (record=0x2afced8) at
> hash_xlog.c:1109
> #13 0x004c5547 in hash_redo (record=0x2afced8) at hash_xlog.c:1214
> #14 0x0053a361 in StartupXLOG () at xlog.c:6975
> #15 0x007a4ca0 in StartupProcessMain () at startup.c:216
>
> on the slave instance in a master-slave setup.

Thanks for reporting this problem. It is basically coming because i
forgot to unpin the bucketbuf in hash_xlog_vacuum_one_page(). Please
find the attached v5 patch that fixes the issue.

>
> Also, the src/backend/access/README file should be updated with a
> description of the changes which this patch provides.

okay, I have updated the insertion algorithm in the README file.

--
With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com


microvacuum_hash_index_v5.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Parallel bitmap heap scan

2017-01-10 Thread tushar 

On 01/10/2017 11:29 AM, tushar wrote:

On 01/09/2017 07:22 PM, Dilip Kumar wrote:

Thanks, Tushar. I have fixed it. The defect was in 0002. I have also
observed another issue related to code refactoring, Actually, there
was some code present in 0001 which supposed to be in 0003.

Thanks, I have checked at my end and it is fixed now.

We found a regression , earlier the testcase was working fine (against 
the older patches of Parallel bitmap heap scan)  but now getting a 
server crash

against v8 patches.

Testcase - (one of the table of TPC-H )

postgres=#explain analyze verbose
SELECT SUM(l_extendedprice) FROM lineitem
WHERE (l_shipdate >= '1995-01-01'::date)
AND (l_shipdate <='1996-03-31'::date);
WARNING:  terminating connection because of crash of another server process
DETAIL:  The postmaster has commanded this server process to roll back 
the current transaction and exit, because another server process exited 
abnormally and possibly corrupted shared memory.
HINT:  In a moment you should be able to reconnect to the database and 
repeat your command.

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost

Here is the stack trace - ( Two core dump file generated)

[centos@centos-cpula bin]$  gdb -q -c data/core.25434 
/home/centos/PG10_10jan/postgresql/edbpsql/bin/postgres
Reading symbols from 
/home/centos/PG10_10jan/postgresql/edbpsql/bin/postgres...done.

[New Thread 25434]
Missing separate debuginfo for
Try: yum --enablerepo='*-debug*' install 
/usr/lib/debug/.build-id/7f/719af91ee951b4fcb6647e7868f95f766a616b
Reading symbols from /usr/lib64/libssl.so.10...(no debugging symbols 
found)...done.

Loaded symbols for /usr/lib64/libssl.so.10
Reading symbols from /usr/lib64/libcrypto.so.10...(no debugging symbols 
found)...done.

Loaded symbols for /usr/lib64/libcrypto.so.10
Reading symbols from /lib64/librt.so.1...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/librt.so.1
Reading symbols from /lib64/libdl.so.2...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libdl.so.2
Reading symbols from /lib64/libm.so.6...(no debugging symbols found)...done.
Loaded symbols for /lib64/libm.so.6
Reading symbols from /lib64/libc.so.6...(no debugging symbols found)...done.
Loaded symbols for /lib64/libc.so.6
Reading symbols from /lib64/libpthread.so.0...(no debugging symbols 
found)...done.

[Thread debugging using libthread_db enabled]
Loaded symbols for /lib64/libpthread.so.0
Reading symbols from /lib64/libgssapi_krb5.so.2...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libgssapi_krb5.so.2
Reading symbols from /lib64/libkrb5.so.3...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libkrb5.so.3
Reading symbols from /lib64/libcom_err.so.2...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libcom_err.so.2
Reading symbols from /lib64/libk5crypto.so.3...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libk5crypto.so.3
Reading symbols from /lib64/libz.so.1...(no debugging symbols found)...done.
Loaded symbols for /lib64/libz.so.1
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/ld-linux-x86-64.so.2
Reading symbols from /lib64/libkrb5support.so.0...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libkrb5support.so.0
Reading symbols from /lib64/libkeyutils.so.1...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libkeyutils.so.1
Reading symbols from /lib64/libresolv.so.2...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libresolv.so.2
Reading symbols from /lib64/libselinux.so.1...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libselinux.so.1
Reading symbols from /lib64/libnss_files.so.2...(no debugging symbols 
found)...done.

Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `postgres: bgworker: parallel worker for PID 
25433  '.

Program terminated with signal 11, Segmentation fault.
#0  0x006f2fa6 in pagetable_destroy (tb=0x2079bf0) at 
../../../src/include/lib/simplehash.h:361

361tb->alloc->HashFree(tb->data, tb->alloc->args);
Missing separate debuginfos, use: debuginfo-install 
glibc-2.12-1.192.el6.x86_64 keyutils-libs-1.4-5.el6.x86_64 
krb5-libs-1.10.3-57.el6.x86_64 libcom_err-1.41.12-22.el6.x86_64 
libselinux-2.0.94-7.el6.x86_64 openssl-1.0.1e-48.el6_8.1.x86_64 
zlib-1.2.3-29.el6.x86_64

(gdb) bt
#0  0x006f2fa6 in pagetable_destroy (tb=0x2079bf0) at 
../../../src/include/lib/simplehash.h:361

#1  0x006f3b52 in tbm_free (tbm=0x2077fe0) at tidbitmap.c:296
#2  0x006ab29b in ExecEndBitmapHeapScan (node=0x207e760) at 
nodeBitmapHeapscan.c:717

#3  0x00691701 in ExecEndNode (node=0x207e760) at execProcnode.c:689
#4  0x006a8f86 in ExecEndAgg (node=0x207e878) at nodeAgg.c:3563
#5

Re: [HACKERS] Block level parallel vacuum WIP

2017-01-10 Thread Masahiko Sawada 
On Mon, Jan 9, 2017 at 6:01 PM, Simon Riggs  wrote:
> On 9 January 2017 at 08:48, Masahiko Sawada  wrote:
>
>> I had not considered necessity of dead lock detection support.
>
> It seems like a big potential win to scan multiple indexes in parallel.
>
> Does the design for collecting dead TIDs use a variable amount of
> memory?

No. Collecting dead TIDs and calculation for max dead tuples are same
as current lazy vacuum. That is, the memory space for dead TIDs is
allocated with fixed size. If parallel lazy vacuum that memory space
is allocated in dynamic shared memory, else is allocated in local
memory.

> Does this work negate the other work to allow VACUUM to use >
> 1GB memory?

Partly yes. Because memory space for dead TIDs needs to be allocated
in DSM before vacuum worker launches, parallel lazy vacuum cannot use
such a variable amount of memory as that work does. But in
non-parallel lazy vacuum, that work would be effective. We might be
able to do similar thing using DSA but I'm not sure that is better.

Attached result of performance test with scale factor = 500 and the
test script I used. I measured each test at four times and plot
average of last three execution times to sf_500.png file. When table
has index, vacuum execution time is smallest when number of index and
parallel degree is same.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


parallel_vacuum.sh
Description: Bourne shell script

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

[HACKERS] GSoC 2017

2017-01-10 Thread Alexander Korotkov 
Hi all!

In 2016 PostgreSQL project didn't pass to GSoC program.  In my
understanding the reasons for that are following.

1. We did last-minute submission of our application to GSoC.
2. In 2016 GSoC application form for mentoring organizations has been
changed.  In particular, it required more detailed information about
possible project.

As result we didn't manage to make a good enough application that time.
Thus, our application was declined. See [1] and [2] for details.

I think that the right way to manage this in 2017 would be to start
collecting required information in advance.  According to GSoC 2017
timeline [3] mentoring organization can submit their applications from
January 19 to February 9.  Thus, now it's a good time to start collecting
project ideas and make call for mentors.  Also, we need to decide who would
be our admin this year.

In sum, we have following questions:
1. What project ideas we have?
2. Who are going to be mentors this year?
3. Who is going to be project admin this year?

BTW, I'm ready to be mentor this year.  I'm also open to be an admin if
needed.

[1]
https://www.postgresql.org/message-id/flat/CAA-aLv4p1jfuMpsRaY2jDUQqypkEXUxeb7z8Mp-0mW6M03St7A%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/flat/CALxAEPuGpAjBSN-PTuxHfuLLqDS47BEbO_ZYxUYQR3ud1nwbww%40mail.gmail.com
[3] https://developers.google.com/open-source/gsoc/timeline

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Declarative partitioning - another take

2017-01-10 Thread Amit Langote 

Hi Amul,

On 2017/01/09 17:29, amul sul wrote:
> I got server crash due to assert failure at ATTACHing overlap rang
> partition, here is test case to reproduce this:
> 
> CREATE TABLE test_parent(a int) PARTITION BY RANGE (a);
> CREATE TABLE test_parent_part2 PARTITION OF test_parent FOR VALUES
> FROM(100) TO(200);
> CREATE TABLE test_parent_part1(a int NOT NULL);
> ALTER TABLE test_parent ATTACH PARTITION test_parent_part1 FOR VALUES
> FROM(1) TO(200);
> 
> I think, this bug exists in the following code of check_new_partition_bound():
> 
>  767 if (equal || off1 != off2)
>  768 {
>  769 overlap = true;
>  770 with = boundinfo->indexes[off2 + 1];
>  771 }
> 
> When equal is true array index should not be 'off2 + 1'.

Good catch.  Attached patch should fix that.  I observed crash with the
following command as well:

ALTER TABLE test_parent ATTACH PARTITION test_parent_part1 FOR VALUES FROM
(1) TO (300);

That's because there is one more case when the array index shouldn't be
off2 + 1 - the case where the bound at off2 is an upper bound (I'd wrongly
assumed that it's always a lower bound).  Anyway, I rewrote the
surrounding comments to clarify the logic a bit.

> While reading code related to this, I wondered why
> partition_bound_bsearch is not immediately returns when cmpval==0?

partition_bound_bsearch() is meant to return the *greatest* index of the
bound less than or equal to the input bound ("probe").  But it seems to me
now that we would always return the first index at which we get 0 for
cmpval, albeit after wasting cycles to try to find even greater index.
Because we don't have duplicates in the datums array, once we encounter a
bound that is equal to probe, we are only going to find bounds that are
*greater than* probe if we continue looking right, only to turn back again
to return the equal index (which is wasted cycles in invoking the
partition key comparison function(s)).  So, it perhaps makes sense to do
this per your suggestion:

@@ -1988,8 +2018,11 @@ partition_bound_bsearch(PartitionKey key,
PartitionBoundInfo boundinfo,
 if (cmpval <= 0)
 {
 lo = mid;
 *is_equal = (cmpval == 0);
+
+if (*is_equal)
+break;
 }

Thanks,
Amit
>From 7fe537f8e8efeac51c3b0cc91ac51a1aa39399cd Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 10 Jan 2017 17:43:36 +0900
Subject: [PATCH] Fix some wrong thinking in check_new_partition_bound()

Because a given range bound in the PartitionBoundInfo.datums array
is sometimes a lower range bound and at other times an upper range
bound, we must be careful when assuming which, especially when
interpreting the result of partition_bound_bsearch which returns
the index of the greatest bound that is less than or equal to probe.
Due to an error in thinking about the same, the relevant code in
check_new_partition_bound() caused invalid partition (index == -1)
to be chosen as the partition being overlapped.

Also, we need not continue searching for even greater bound in
partition_bound_bsearch() once we find the first bound that is *equal*
to the probe, because we don't have duplicate datums.  That spends
cycles needlessly.  Per suggestion from Amul Sul.

Reported by: Amul Sul
Patch by: Amit Langote
Reports: https://www.postgresql.org/message-id/CAAJ_b94XgbqVoXMyxxs63CaqWoMS1o2gpHiU0F7yGnJBnvDc_A%40mail.gmail.com
---
 src/backend/catalog/partition.c| 62 ++
 src/test/regress/expected/create_table.out | 10 -
 src/test/regress/sql/create_table.sql  |  4 ++
 3 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index f54e1bdf3f..df5652de4c 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -741,35 +741,64 @@ check_new_partition_bound(char *relname, Relation parent, Node *bound)
 		   boundinfo->strategy == PARTITION_STRATEGY_RANGE);
 
 	/*
-	 * Find the greatest index of a range bound that is less
-	 * than or equal with the new lower bound.
+	 * Firstly, find the greatest range bound that is less
+	 * than or equal to the new lower bound.
 	 */
 	off1 = partition_bound_bsearch(key, boundinfo, lower, true,
    );
 
 	/*
-	 * If equal has been set to true, that means the new lower
-	 * bound is found to be equal with the bound at off1,
-	 * which clearly means an overlap with the partition at
-	 * index off1+1).
-	 *
-	 * Otherwise, check if there is a "gap" that could be
-	 * occupied by the new partition.  In case of a gap, the
-	 * new upper bound should not cross past the upper
-	 * boundary of the gap, that is, off2 == off1 should be
-	 * true.
+	 * off1 == -1 means that all existing bounds are greater
+	 *

Re: [HACKERS] Declarative partitioning - another take

2017-01-10 Thread Amit Langote 

Hi Kieth,

On 2017/01/10 14:44, Keith Fiske wrote:
> Is there any reason for the exclusion of parent tables from the pg_tables
> system catalog view? They do not show up in information_schema.tables as
> well. I believe I found where to make the changes and I tested to make sure
> it works for my simple case. Attached is my first attempt at patching
> anything in core. Not sure if there's anywhere else this would need to be
> fixed.

That's an oversight.  The original partitioning patch didn't touch
information_schema.sql and system_views.sql at all.  I added the relkind =
'P' check in some other views as well, including what your patch considered.

Thanks,
Amit
>From 9eef3e87b9a025d233aa4b935b50bb0c7633efbb Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 10 Jan 2017 18:12:25 +0900
Subject: [PATCH] information_schema/system_views.sql and relkind 'P'

Currently, partitioned table are not taken into account in various
information_schema and system views.

Reported by: Keith Fiske
Patch by: Kieth Fiske, Amit Langote
Reports: https://www.postgresql.org/message-id/CAG1_KcDJiZB=l6youo_bvufj2q2851_xdkfhw0jdcd_2vtk...@mail.gmail.com
---
 src/backend/catalog/information_schema.sql | 37 +++---
 src/backend/catalog/system_views.sql   |  3 ++-
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 4df390a763..318f195b81 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -453,7 +453,7 @@ CREATE VIEW check_constraints AS
   AND a.attnum > 0
   AND NOT a.attisdropped
   AND a.attnotnull
-  AND r.relkind = 'r'
+  AND r.relkind IN ('r', 'P')
   AND pg_has_role(r.relowner, 'USAGE');
 
 GRANT SELECT ON check_constraints TO PUBLIC;
@@ -525,7 +525,7 @@ CREATE VIEW column_domain_usage AS
   AND a.attrelid = c.oid
   AND a.atttypid = t.oid
   AND t.typtype = 'd'
-  AND c.relkind IN ('r', 'v', 'f')
+  AND c.relkind IN ('r', 'v', 'f', 'P')
   AND a.attnum > 0
   AND NOT a.attisdropped
   AND pg_has_role(t.typowner, 'USAGE');
@@ -564,7 +564,7 @@ CREATE VIEW column_privileges AS
   pr_c.relowner
FROM (SELECT oid, relname, relnamespace, relowner, (aclexplode(coalesce(relacl, acldefault('r', relowner.*
  FROM pg_class
- WHERE relkind IN ('r', 'v', 'f')
+ WHERE relkind IN ('r', 'v', 'f', 'P')
 ) pr_c (oid, relname, relnamespace, relowner, grantor, grantee, prtype, grantable),
 pg_attribute a
WHERE a.attrelid = pr_c.oid
@@ -586,7 +586,7 @@ CREATE VIEW column_privileges AS
 ) pr_a (attrelid, attname, grantor, grantee, prtype, grantable),
 pg_class c
WHERE pr_a.attrelid = c.oid
- AND relkind IN ('r', 'v', 'f')
+ AND relkind IN ('r', 'v', 'f', 'P')
  ) x,
  pg_namespace nc,
  pg_authid u_grantor,
@@ -629,7 +629,7 @@ CREATE VIEW column_udt_usage AS
 WHERE a.attrelid = c.oid
   AND a.atttypid = t.oid
   AND nc.oid = c.relnamespace
-  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f')
+  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f', 'P')
   AND pg_has_role(coalesce(bt.typowner, t.typowner), 'USAGE');
 
 GRANT SELECT ON column_udt_usage TO PUBLIC;
@@ -738,7 +738,7 @@ CREATE VIEW columns AS
CAST('NEVER' AS character_data) AS is_generated,
CAST(null AS character_data) AS generation_expression,
 
-   CAST(CASE WHEN c.relkind = 'r' OR
+   CAST(CASE WHEN c.relkind IN ('r', 'P') OR
   (c.relkind IN ('v', 'f') AND
pg_column_is_updatable(c.oid, a.attnum, false))
 THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_updatable
@@ -753,7 +753,7 @@ CREATE VIEW columns AS
 
 WHERE (NOT pg_is_other_temp_schema(nc.oid))
 
-  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f')
+  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f', 'P')
 
   AND (pg_has_role(c.relowner, 'USAGE')
OR has_column_privilege(c.oid, a.attnum,
@@ -789,7 +789,7 @@ CREATE VIEW constraint_column_usage AS
 AND d.objid = c.oid
 AND c.connamespace = nc.oid
 AND c.contype = 'c'
-AND r.relkind = 'r'
+AND r.relkind IN ('r', 'P')
 AND NOT a.attisdropped
 
 UNION ALL
@@ -841,7 +841,7 @@ CREATE VIEW constraint_table_usage AS
 WHERE c.connamespace = nc.oid AND r.relnamespace = nr.oid
   AND ( (c.contype = 'f' AND c.confrelid = r.oid)
  OR (c.contype IN ('p', 'u') AND c.conrelid = r.oid) )
-  AND

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

2017-01-10 Thread Fujii Masao 
On Wed, Jan 4, 2017 at 4:32 PM, Andres Freund  wrote:
> Hi,
>
> On 2017-01-03 10:37:08 -0500, Stephen Frost wrote:
>> * Vladimir Rusinov (vrusi...@google.com) wrote:
>> > I think I +1 on this.
>> > I've did a github search on these function names and there is a lot of code
>> > that use them. E.g. there is 8.5k hits for pg_last_xlog_location
>> > ;
>> > a lot of them a forks and copy-pastes, but still, that's quite a lot. Let's
>> > keep the aliases around for couple of versions after which hopefully a lot
>> > of the code will be updated.
>>
>> And there's 12k hits for pg_xlog.
>
>> If we do that, we'll just end up with exactly the same question about
>> removing them and the same amount of code breakage in a few years.  I
>> don't see how that is really helping anyone.
>
> Meh^2.  The cost of having pg_xlog was that people lost their
> data. Hence their was motivation of changing things. The cost of having
> some function aliases is, what, a pg_proc line?  If we end up carrying
> them forever, so what?
>
>
>> If we really feel that this is the only thing between 9.6 and 10 that'll
>> cause problems for some serious amount of code and we don't expect to
>> change the function APIs anytime in the near future then perhaps we
>> could keep aliases, *document* them, and treat them as full functions
>> just like the regular ones.
>
> I think we've been far to cavalier lately about unnecessarily breaking
> admin and monitoring tools. There's been pg_stat_activity backward
> incompat changes in most of the last releases. It's a *PAIN* to develop
> monitoring / admin tools that work with a number of releases.  It's fine
> to cause that pain if there's some considerable benefit (e.g. not
> triggering data loss seems like a case for that, as imo is unifying
> configuration), but I don't see how that justifying breaking things
> gratuitously.  Just renaming well known functions for a minor bit of
> cleanliness seems not to survive a cost/benefit analysis.

+1

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] proposal: session server side variables

2017-01-10 Thread Craig Ringer 
On 10 January 2017 at 14:31, Fabien COELHO  wrote:

> I have submitted a proof of this fact in the form of a counter example which
> (1) (pseudo) implements the use-case by logging into an audit table the fact
> a user accesses the secure level (2) shows that the status of a non
> transactional session variable used for keeping this status is wrong for the
> use case in some cases (it says that all is well while appending to the
> audit table failed).

You've been assuming everyone else cares about auditing such access
into a table.

Personally I tend to agree with you that it's useful enough to justify
transactional vars. But you're fixated on the idea that without that
use case satisfied the rest is useless, and that's simply not the
case. Transactional vars are only needed if you make _write_ changes
to the DB that must be committed atomically with the var change. If
you're only doing (maybe expensive) lookups, it doesn't matter.

> I feel entitled to point out to other people that their belief that a
> feature as described provides a correct solution to a particular use case is
> wrong, if it is factually the case. If they persist in this belief despite
> the submitted proof, I can only be sad about it, because if pg provides a
> feature for a security-relared use case which does not work correctly it is
> just shooting one's foot.

Again ... I think you've assumed everyone else is talking about the
same security-related case you are.

I haven't seen Pavel talking about access audit logging. If he has
been, then my mistake and you're quite correct. But my reading is that
you've been _assuming_ that.

> I do not like Pavel's feature, this is a subjective opinion. This feature
> does not provide a correct solution for the use case, this is an objective
> fact.

For _your_ use case.

> The presented feature does not have a real use case, this is too bad.

No, it just doesn't match your idea of what the use case is.

It does have other uses that are perfectly valid. Look up access
rights, set var. It's fine.

Now, I think we might as well do it transactionally, but it's not some
kind of absolute requirement; if you're not transactional, it just
means you can't do reliable audit logging of access into tables. Of
course, we can't do that already for anything else.

It's kind of like someone coming to you and saying they want to add an
engine to their glider, and you explaining that it's totally useless
to add an engine unless it can also function as a submarine. Um,
that's nice, but not what they asked for.

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


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

Re: [HACKERS] Parallel bitmap heap scan

2017-01-10 Thread Dilip Kumar 
On Tue, Jan 10, 2017 at 2:19 PM, tushar  wrote:
> We found a regression , earlier the testcase was working fine (against the
> older patches of Parallel bitmap heap scan)  but now getting a server crash
> against v8 patches.
>
> Testcase - (one of the table of TPC-H )
>
> postgres=#explain analyze verbose
> SELECT SUM(l_extendedprice) FROM lineitem
> WHERE (l_shipdate >= '1995-01-01'::date)
> AND (l_shipdate <='1996-03-31'::date);

While fixing some of the review comments in v7 and v8, I had allocated
new memory for pagetable, and missed to initialize it.

Thanks for reporting, I have fixed it in v9. After fix query is
running fine for me. Please try attached patch and confirm from your
side.

postgres=# explain analyze verbose
SELECT SUM(l_extendedprice) FROM lineitem
WHERE (l_shipdate >= '1995-01-01'::date)
AND (l_shipdate <='1996-03-31'::date);

QUERY PLAN

--
---
 Finalize Aggregate  (cost=798002.46..798002.47 rows=1 width=32)
(actual time=15501.245..15501.245 rows=1 loops=1)
   Output: sum(l_extendedprice)
   ->  Gather  (cost=798002.24..798002.45 rows=2 width=32) (actual
time=15494.358..15498.919 rows=3 loops=1)
 Output: (PARTIAL sum(l_extendedprice))
 Workers Planned: 2
 Workers Launched: 2
 ->  Partial Aggregate  (cost=797002.24..797002.25 rows=1
width=32) (actual time=15492.937..15492.937 rows=1 loops=3)
   Output: PARTIAL sum(l_extendedprice)
   Worker 0: actual time=15491.218..15491.219 rows=1 loops=1
   Worker 1: actual time=15493.514..15493.514 rows=1 loops=1
   ->  Parallel Bitmap Heap Scan on public.lineitem
(cost=147461.75..791014.31 rows=2395170 width=8) (actual
time=8553.301..15061.333 rows=189294
7 loops=3)
 Output: l_extendedprice
 Recheck Cond: ((lineitem.l_shipdate >=
'1995-01-01'::date) AND (lineitem.l_shipdate <= '1996-03-31'::date))
 Rows Removed by Index Recheck: 6451177
 Heap Blocks: exact=27963 lossy=164938
 Worker 0: actual time=8548.957..15054.511
rows=1887239 loops=1
 Worker 1: actual time=8554.817..15050.317
rows=1902477 loops=1
 ->  Bitmap Index Scan on idx_lineitem_shipdate
(cost=0.00..146024.65 rows=5748409 width=0) (actual
time=8533.701..8533.701 rows=5678841
loops=1)
   Index Cond: ((lineitem.l_shipdate >=
'1995-01-01'::date) AND (lineitem.l_shipdate <= '1996-03-31'::date))
 Planning time: 2.742 ms
 Execution time: 15509.696 ms
(21 rows)



-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0001-opt-parallelcost-refactoring-v9.patch
Description: Binary data


0002-hash-support-alloc-free-v9.patch
Description: Binary data


0003-parallel-bitmap-heap-scan-v9.patch
Description: Binary data

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

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-10 Thread Ashutosh Bapat 
>
>> If the inner and/or outer paths are not ordered as required, we will need
>> to
>> order them. Code below doesn't seem to handle that case.
>> /*
>>  * If the paths are already well enough ordered, we
>> can
>>  * skip doing an explicit sort.
>>  */
>> if (outerkeys &&
>> pathkeys_contained_in(outerkeys,
>> outer_path->pathkeys))
>> outerkeys = NIL;
>> if (innerkeys &&
>> pathkeys_contained_in(innerkeys,
>> inner_path->pathkeys))
>> innerkeys = NIL;
>
>
> I might be missing something, but if the outer/inner paths are already well
> sorted, we wouldn't need an explicit sort, so we can set the
> outerkeys/innerkeys to NIL safely.  (You can find the same optimization in
> try_mergejoin_path.)

Actually I didn't notice that create_mergejoin_path saves those keys
in the MergePath and then adds sorting steps during planning. Sorry
for the trouble.

Another concern here is that we are copying pieces of logic in
add_paths_to_joinrel() without arranging it as neatly as in that
function.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

  1   2   >