Re: [HACKERS] Proposal for changes to recovery.conf API

[Bruce Momjian]

On Fri, Dec 16, 2016 at 08:52:25PM -0500, Robert Haas wrote:
> On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane  wrote:
> > I'm still not seeing any value in putting this sort of info into
> > a documentation section that's distinct from the release notes.
> > We've used links to wiki pages in the past when the information
> > seemed to be in flux, and that's reasonable.  But what's the point
> > of just linking to somewhere else in the same document?
> 
> If the explanation is just a few sentences long, I see no reason not
> to include it in the release notes.  But if it's comparable in length
> to a moderately-long chapter then why would we not make it its own
> chapter?  I think your argument boils down to "people probably don't
> need very much detail about this".  But I think that's the wrong line
> of thinking.  In my view, we ought to ship just about as much quality
> documentation as people are willing to write.  Saying that we're going
> to reject good-quality documentation because we don't want to have too
> much of it is like saying we want to reject good-quality features
> because we don't want to have too many of them, or good-quality
> regression tests because we don't want to have too much code coverage,

[ I stopped reading after this. ]

The point is that the documentation about the recovery.conf changes in
Postgres are only interesting to people migrating to Postgres 10, i.e.
this is not quality documentation for someone going from Postgres 10 to
Postgres 11.

-- 
  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] Hang in pldebugger after git commit : 98a64d0

[Robert Haas]

On Fri, Dec 16, 2016 at 10:34 PM, Amit Kapila  wrote:
>>  I think it should be the responsibility of
>> WaitEventSetWaitBlock() to reset the event, if needed, before calling
>> WaitForMultipleObjects().
>>
>
> If we want to change WaitEventSetWaitBlock then ideally we need to
> change all other wait API's (WAIT_USE_SELECT,  WAIT_USE_POLL, etc.) as
> well.

Why?  This is only broken on Windows.  It would be nicer not to touch
any of the un-broken implementations.

>>  BTW, I suggest this rewritten comment:
>>
>> /*--
>>  * FD_READ events are edge-triggered on Windows per
>>  * 
>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx
>>
>
> Isn't the statement in above doc. "For FD_READ, FD_OOB, and FD_ACCEPT
> network events, network event recording and event object signaling are
> level-triggered." says that FD_READ events are level-triggered which
> seems to be contradictory with above comment?

Argh.  I see your point.  Maybe we'd better rephrase that.  The
document does say that, but the behavior they described is actually a
weird hybrid of level-triggered and edge-triggered.  We should
probably just avoid that terminology altogether and explain that
read-ready won't necessarily be re-signalled unless there is an
intervening recv().

-- 
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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Sat, Dec 17, 2016 at 10:23 AM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> (Robert you were in this set at this point), and the same thing was
>> concluded during the informal lunch meeting at PGcon. The point is,
>> the existing SCRAM patch set can survive without touching at *all* the
>> format of pg_authid. We could block SCRAM authentication when
>> "password" is used in pg_hba.conf and as well as when "scram" is used
>> with a plain password stored in pg_authid. Or look at the format of
>> the string in the catalog if "password" is defined and decide the
>> authentication protocol to follow based on that.
>
> As I mentioned up-thread, moving forward with minimal changes to get
> SCRAM in certainly makes sense, but I do think we should be open to
> (and, ideally, encouraging people to work towards) having a seperate
> table for verifiers with independent columns for type and verifier.

Definitely, and you know my position on the matter or I would not have
written last year's patch series. Both things are just orthogonal IMO
at this point. And it would be good to focus just on one problem at
the moment to get it out.
-- 
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] Hang in pldebugger after git commit : 98a64d0

[Amit Kapila]

On Fri, Dec 16, 2016 at 9:03 PM, Robert Haas  wrote:
> On Thu, Dec 15, 2016 at 4:17 AM, Amit Kapila  wrote:
>> On Thu, Dec 15, 2016 at 10:04 AM, Andres Freund  wrote:
>>> Hi,
>>>
>>> On 2016-12-12 16:46:38 +0900, Michael Paquier wrote:
 Ashutosh, could you try it and see if it improves things?
>>>
>>> So what's the theory of why this triggers pldebugger hangs, but
>>> apparently causes not many other problems?
>>>
>>
>> The theory is that with pldebugger latch event gets triggered before
>> FD_READ due to which it seems like the second event gets lost and
>> WaitForMultipleObjects() keeps on waiting indefinitely.  The probable
>> reason is that we fail to reset the event due to which we are seeing
>> this behavior. That seems to be required as per msdn as well, as
>> pointed by Robert upthread.
>>
>> Find attached patch to implement the resetting of event only for the
>> required case.  This fixes the reported problem.
>
> After some study I don't think this is quite right yet.  The client
> code is not obliged to call ModifyWaitEvent() before again calling
> WaitEventSetWait().
>

makes sense.

>  I think it should be the responsibility of
> WaitEventSetWaitBlock() to reset the event, if needed, before calling
> WaitForMultipleObjects().
>

If we want to change WaitEventSetWaitBlock then ideally we need to
change all other wait API's (WAIT_USE_SELECT,  WAIT_USE_POLL, etc.) as
well.  I don't see the need for it, can't we do it in
WaitEventSetWait() after processing all the returned events.  It can
be done by enumerating all wait events and reset the events for which
reset flag is true.  If we see code prior to 9.6, this event (socket
event) has been reset only after processing all returned events, not
after every event.

>  BTW, I suggest this rewritten comment:
>
> /*--
>  * FD_READ events are edge-triggered on Windows per
>  * 
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx
>

Isn't the statement in above doc. "For FD_READ, FD_OOB, and FD_ACCEPT
network events, network event recording and event object signaling are
level-triggered." says that FD_READ events are level-triggered which
seems to be contradictory with above comment?

>
> The dashes keep pgindent from inserting a line break into the link.
>

Thanks for the information.


-- 
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] Declarative partitioning - another take

[Amit Langote]

On Sat, Dec 17, 2016 at 1:07 AM, Robert Haas  wrote:
> On Fri, Dec 16, 2016 at 3:02 AM, Amit Langote
>  wrote:
>> Aside from the above, I found few other issues and fixed them in the
>> attached patches.  Descriptions follow:
>
> To avoid any further mistakes on my part, can you please resubmit
> these with each patch file containing a proposed commit message
> including patch authorship information, who reported the issue, links
> to relevant discussion if any, and any other attribution information
> which I should not fail to include when committing?

I think it's a good advice and will keep in mind for any patches I
post henceforth.

In this particular case, I found all the issues myself while working
with some more esoteric test scenarios, except the first patch (1/7),
where I have mentioned in the description of the patch in the email,
that there were independent reports of the issue by Tomas Vondra and
David Fetter.

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] Proposal for changes to recovery.conf API

[Robert Haas]

On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane  wrote:
> I'm still not seeing any value in putting this sort of info into
> a documentation section that's distinct from the release notes.
> We've used links to wiki pages in the past when the information
> seemed to be in flux, and that's reasonable.  But what's the point
> of just linking to somewhere else in the same document?

If the explanation is just a few sentences long, I see no reason not
to include it in the release notes.  But if it's comparable in length
to a moderately-long chapter then why would we not make it its own
chapter?  I think your argument boils down to "people probably don't
need very much detail about this".  But I think that's the wrong line
of thinking.  In my view, we ought to ship just about as much quality
documentation as people are willing to write.  Saying that we're going
to reject good-quality documentation because we don't want to have too
much of it is like saying we want to reject good-quality features
because we don't want to have too many of them, or good-quality
regression tests because we don't want to have too much code coverage,
or good-quality bug fixes because we don't want to have too few bugs.
It is of course true that all of these things can be done in a bad way
or to excess: our documentation could be a terabyte!  our features
could be so numerous as to befuddle even expert users!  our regression
tests could run for a hundred hours on a supercomputer!  we could fix
so many basically-trivial bugs that our committers all fall sobbing to
the floor with crippling repetitive strain injuries!  But let's not
FUD things that are basically positive.  I think it's fantastic that
Josh Berkus wants to write more documentation, and if 10 other people
show up to do similar work, I'll buy them all a beer at the next
conference we're at together.  I almost wrote "I'll buy them all a
bear" but that might be over the top.

-- 
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] Declarative partitioning vs. sql_inheritance

[Robert Haas]

On Fri, Dec 16, 2016 at 7:39 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 12/16/16 11:05 AM, Robert Haas wrote:
>>> If we were going to do anything about this,
>>> my vote would be to remove sql_inheritance.
>
>> Go for it.
>
>> Let's also remove the table* syntax then.
>
> Meh --- that might break existing queries, to what purpose?
>
> We certainly shouldn't remove query syntax without a deprecation period.
> I'm less concerned about that for GUCs.

I agree.  Patch attached, just removing the GUC and a fairly minimal
amount of the supporting infrastructure.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0fc4e57..605910f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7366,36 +7366,6 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
  
 
- 
-  sql_inheritance (boolean)
-  
-   sql_inheritance configuration parameter
-  
-  inheritance
-  
-  
-   
-This setting controls whether undecorated table references are
-considered to include inheritance child tables.  The default is
-on, which means child tables are included (thus,
-a * suffix is assumed by default).  If turned
-off, child tables are not included (thus, an
-ONLY prefix is assumed).  The SQL standard
-requires child tables to be included, so the off setting
-is not spec-compliant, but it is provided for compatibility with
-PostgreSQL releases prior to 7.1.
-See  for more information.
-   
-
-   
-Turning sql_inheritance off is deprecated, because that
-behavior has been found to be error-prone as well as contrary to SQL
-standard.  Discussions of inheritance behavior elsewhere in this
-manual generally assume that it is on.
-   
-  
- 
-
  
   standard_conforming_strings (boolean)
   stringsstandard conforming
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 7e1bc0e..d7117cb 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2529,11 +2529,9 @@ SELECT name, altitude
 WHERE altitude  500;
 
 
-   Writing * is not necessary, since this behavior is
-   the default (unless you have changed the setting of the
-configuration option).
-   However writing * might be useful to emphasize that
-   additional tables will be searched.
+   Writing * is not necessary, since this behavior is always
+   the default.  However, this syntax is still supported for
+   compatibility with older releases where the default could be changed.
   
 
   
diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml
index 5cc6dbc..0f84c12 100644
--- a/doc/src/sgml/queries.sgml
+++ b/doc/src/sgml/queries.sgml
@@ -145,11 +145,9 @@ FROM table_reference , table_r

 Instead of writing ONLY before the table name, you can write
 * after the table name to explicitly specify that descendant
-tables are included.  Writing * is not necessary since that
-behavior is the default (unless you have changed the setting of the  configuration option).  However writing
-* might be useful to emphasize that additional tables will be
-searched.
+tables are included.  There is no real reason to use this syntax any more,
+because searching descendent tables is now always the default behavior.
+However, it is supported for compatibility with older releases.

 

diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 9e62e00..ba1414b 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -54,7 +54,7 @@ LockTableCommand(LockStmt *lockstmt)
 	foreach(p, lockstmt->relations)
 	{
 		RangeVar   *rv = (RangeVar *) lfirst(p);
-		bool		recurse = interpretInhOption(rv->inhOpt);
+		bool		recurse = (rv->inhOpt == INH_YES);
 		Oid			reloid;
 
 		reloid = RangeVarGetRelidExtended(rv, lockstmt->mode, false,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7a574dc..075b68b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1183,7 +1183,7 @@ ExecuteTruncate(TruncateStmt *stmt)
 	{
 		RangeVar   *rv = lfirst(cell);
 		Relation	rel;
-		bool		recurse = interpretInhOption(rv->inhOpt);
+		bool		recurse = (rv->inhOpt == INH_YES);
 		Oid			myrelid;
 
 		rel = heap_openrv(rv, AccessExclusiveLock);
@@ -2654,7 +2654,7 @@ renameatt(RenameStmt *stmt)
 		renameatt_internal(relid,
 		   stmt->subname,		/* old att name */
 		   stmt->newname,		/* new att name */
-		   interpretInhOption(stmt->relation->inhOpt),	/* recursive? */
+		   (stmt->relation->inhOpt == INH_YES),	/* recursive? */
 		   false,		/* recursing? */
 		   0,	/* expected inhcount */
 		   

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost  wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> On 12/15/16 8:40 AM, Stephen Frost wrote:
> >> > I don't follow why we can't change the syntax for CREATE USER to allow
> >> > specifying the verifier type independently.
> >>
> >> That's what the last patch set I looked at actually does.
> >
> > Well, same here, but it was quite a while ago and things have progressed
> > since then wrt SCRAM, as I understand it...
> 
> From the discussions of last year on -hackers, it was decided to *not*
> have an additional column per complains from a couple of hackers

It seems that, at best, we didn't have consensus on it.  Hopefully we
are moving in a direction of consensus.

> (Robert you were in this set at this point), and the same thing was
> concluded during the informal lunch meeting at PGcon. The point is,
> the existing SCRAM patch set can survive without touching at *all* the
> format of pg_authid. We could block SCRAM authentication when
> "password" is used in pg_hba.conf and as well as when "scram" is used
> with a plain password stored in pg_authid. Or look at the format of
> the string in the catalog if "password" is defined and decide the
> authentication protocol to follow based on that.

As I mentioned up-thread, moving forward with minimal changes to get
SCRAM in certainly makes sense, but I do think we should be open to
(and, ideally, encouraging people to work towards) having a seperate
table for verifiers with independent columns for type and verifier.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Proposal for changes to recovery.conf API

[Tom Lane]

Bruce Momjian  writes:
> On Fri, Dec 16, 2016 at 07:19:43PM -0500, Robert Haas wrote:
>> I really don't see why we're resisting Josh's idea of putting a more
>> complex set of migration instructions in the documentation someplace.
>> Seems useful to me.  Sure, we'd have to "carry" it forever, but we
>> could make a policy of removing migration instructions for releases
>> that are now EOL.

> Well, the item has a limited useful lifespan, and we could adjust the
> wiki page between minor releases if we found problems/improvements.  I
> suppose if we create a "Migration" section in the documentation it would
> be harmless enough, but it is unclear what would be in there and what
> would be in the release notes.  What would be the title of the
> subsection for this?   "Migrating recovery.conf to Postgres 10?"  Maybe
> make it a subsection of the release notes in the docs?

I've been thinking for awhile that we need to start retiring
ancient-branch release notes from the active documentation.  I got
pushback on that when I proposed it to the list (too lazy to look up
the thread right now).  But it would certainly be an easier sell
to make the release notes more voluminous if we started cutting off
the long tail of ancient notes.

I'm still not seeing any value in putting this sort of info into
a documentation section that's distinct from the release notes.
We've used links to wiki pages in the past when the information
seemed to be in flux, and that's reasonable.  But what's the point
of just linking to somewhere else in the same document?

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] Declarative partitioning vs. sql_inheritance

[Tom Lane]

Peter Eisentraut  writes:
> On 12/16/16 11:05 AM, Robert Haas wrote:
>> If we were going to do anything about this,
>> my vote would be to remove sql_inheritance.

> Go for it.

> Let's also remove the table* syntax then.

Meh --- that might break existing queries, to what purpose?

We certainly shouldn't remove query syntax without a deprecation period.
I'm less concerned about that for GUCs.

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] Proposal for changes to recovery.conf API

[Bruce Momjian]

On Fri, Dec 16, 2016 at 07:19:43PM -0500, Robert Haas wrote:
> On Fri, Dec 16, 2016 at 5:29 PM, Bruce Momjian  wrote:
> > I am fine with the release note, or the release notes plus a link to a
> > wiki, like we have done in the past with complex fixes in minor
> > releases:
> >
> > https://wiki.postgresql.org/wiki/20110408pg_upgrade_fix
> >
> > I think what we _don't_ want is the information _only_ in the wiki, nor
> > do we want to carry around migration instructions in our docs forever.
> 
> I really don't see why we're resisting Josh's idea of putting a more
> complex set of migration instructions in the documentation someplace.
> Seems useful to me.  Sure, we'd have to "carry" it forever, but we
> could make a policy of removing migration instructions for releases
> that are now EOL.  And even if we didn't, it's not like extra
> documentation comes with some big cost.  I think a lot of users would
> benefit from a substantial expansion of our documentation, not just in
> this area but in many other areas as well.  I bet that we could double
> the size of the documentation and users would love it; the hard part
> would be finding qualified people to write high-quality documentation
> of all the things that aren't well-explained in the current docs.

Well, the item has a limited useful lifespan, and we could adjust the
wiki page between minor releases if we found problems/improvements.  I
suppose if we create a "Migration" section in the documentation it would
be harmless enough, but it is unclear what would be in there and what
would be in the release notes.  What would be the title of the
subsection for this?   "Migrating recovery.conf to Postgres 10?"  Maybe
make it a subsection of the release notes in the docs?

-- 
  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] Proposal for changes to recovery.conf API

[Robert Haas]

On Fri, Dec 16, 2016 at 5:29 PM, Bruce Momjian  wrote:
> I am fine with the release note, or the release notes plus a link to a
> wiki, like we have done in the past with complex fixes in minor
> releases:
>
> https://wiki.postgresql.org/wiki/20110408pg_upgrade_fix
>
> I think what we _don't_ want is the information _only_ in the wiki, nor
> do we want to carry around migration instructions in our docs forever.

I really don't see why we're resisting Josh's idea of putting a more
complex set of migration instructions in the documentation someplace.
Seems useful to me.  Sure, we'd have to "carry" it forever, but we
could make a policy of removing migration instructions for releases
that are now EOL.  And even if we didn't, it's not like extra
documentation comes with some big cost.  I think a lot of users would
benefit from a substantial expansion of our documentation, not just in
this area but in many other areas as well.  I bet that we could double
the size of the documentation and users would love it; the hard part
would be finding qualified people to write high-quality documentation
of all the things that aren't well-explained in the current docs.

-- 
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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost  wrote:
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 12/15/16 8:40 AM, Stephen Frost wrote:
>> > I don't follow why we can't change the syntax for CREATE USER to allow
>> > specifying the verifier type independently.
>>
>> That's what the last patch set I looked at actually does.
>
> Well, same here, but it was quite a while ago and things have progressed
> since then wrt SCRAM, as I understand it...

>From the discussions of last year on -hackers, it was decided to *not*
have an additional column per complains from a couple of hackers
(Robert you were in this set at this point), and the same thing was
concluded during the informal lunch meeting at PGcon. The point is,
the existing SCRAM patch set can survive without touching at *all* the
format of pg_authid. We could block SCRAM authentication when
"password" is used in pg_hba.conf and as well as when "scram" is used
with a plain password stored in pg_authid. Or look at the format of
the string in the catalog if "password" is defined and decide the
authentication protocol to follow based on that.
-- 
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 for changes to recovery.conf API

[Bruce Momjian]

On Thu, Dec 15, 2016 at 04:16:36PM -0800, Josh Berkus wrote:
> On 12/15/2016 12:54 PM, Tom Lane wrote:
> > Magnus Hagander  writes:
> >> On Thu, Dec 15, 2016 at 1:11 AM, Bruce Momjian  wrote:
> >>> You are saying this is more massive than any other change we have made
> >>> in the past?  In general, what need to be documented?
> > 
> >> I don't necessarily think it's because it's more massive than any chance we
> >> have made before. I think it's more that this is something that we probably
> >> should've had before, and just didn't.
> > 
> >> Right now we basically have a bulletpoint list of things that are new, with
> >> a section about things that are incompatible.  Having an actual section
> >> with more detailed descriptions of how to handle these changes would
> >> definitely be a win. it shouldn't *just* be for these changes of course, it
> >> should be for any other changes that are large enough to benefit from more
> >> than a oneliner about the fact that they've changed.
> > 
> > Yeah, it seems to me that where this is ending up is "we may need to
> > write more in the compatibility entries than we have in the past".
> > I don't see any problem with that, particularly if someone other than
> > Bruce or me is volunteering to write it ;-)
> 
> I'm up for writing it (with help from feature owners), provided that I
> don't have to spend time arguing that it's not too long, or that I
> should put it somewhere different.  So can we settle the "where"
> question first?

I am fine with the release note, or the release notes plus a link to a
wiki, like we have done in the past with complex fixes in minor
releases:

https://wiki.postgresql.org/wiki/20110408pg_upgrade_fix

I think what we _don't_ want is the information _only_ in the wiki, nor
do we want to carry around migration instructions in our docs forever.

-- 
  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] Creating a DSA area to provide work space for parallel execution

[Alvaro Herrera]

Robert Haas wrote:

> I am not sure the issue was time so much as the ability to foresee all
> the problems we'd want to solve.

I think all that movement is okay.  It's not like we're breaking things
to no purpose.  The amount of effort that has to go into making
extensions compile with changed APIs is not *that* bad, surely; it's
pretty clear that we need to keep moving forward.  All the changes you
listed that required lwlock changed have clearly been worth the
breakage, IMO.

> I think it's quite surprising how fast the LWLock system has evolved
> over the last few years.  When I first started working on PostgreSQL
> in 2008, there was no LWLockAcquireOrWait, none of the Var stuff, the
> padding was much less sophisticated, no tranches, no atomics, volatile
> qualifiers all over the place...  and all of that has changed in the
> last 5 years.  Pretty amazing, actually, IMHO.

Yes, I agree.

-- 
Á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] Creating a DSA area to provide work space for parallel execution

[Robert Haas]

On Fri, Dec 16, 2016 at 12:36 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
>> > Thoughts?
>>
>> Hearing no objections, I've gone ahead and committed this.  If that
>> makes somebody really unhappy I can revert it, but I am betting that
>> the real story is that nobody cares about preserving T_ID().
>
> AFAICT the comment on LWLockRegisterTranche is confused; it talks about
> an allocated object being passed, but there isn't any.

Oops.  Thanks, will fix.

-- 
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] Creating a DSA area to provide work space for parallel execution

[Robert Haas]

On Fri, Dec 16, 2016 at 12:37 PM, Andres Freund  wrote:
> Yea, I don't think that's good either.  I'm all for evolving APIs when
> necessary, but constantly breaking the same API release after release
> seems indicative of needing to spend a bit more time on it in the first
> round.

I am not sure the issue was time so much as the ability to foresee all
the problems we'd want to solve.  9.4 added tranches and converted
everything to LWLock * instead of LWLockId, but I think all of the old
APIs still worked.  At that point, we didn't have parallel query and
we weren't that close to having it, so I was loathe to do anything too
invasive.  9.5 removed LWLockAcquireWithVar() and added
LWLockReleaseClearVar(), but most of the API was still fine.  9.6
moved almost everything to tranches and removed RequestAddinLWLocks()
and LWLockAssign(), which was a big break for extensions -- but that
wasn't because of parallel query, but rather because we wanted to use
tranches to support the wait_event stuff and we also wanted to be able
to pad different tranches differently.  This latest change is inspired
by the fact that the 9.4-era changes to support parallel query weren't
quite smart enough to be able to cope with the possibility of multiple
tranches with the same tranche ID in a reasonable way.  That last one
is indeed an oversight but in January of 2014 it wasn't very clear
that we were going to have tranche-ified every LWLock in the system,
without which this change wouldn't be possible.  Quite a lot of work
by at least 3 or 4 different people went into that tranche-ification
effort.

I think it's quite surprising how fast the LWLock system has evolved
over the last few years.  When I first started working on PostgreSQL
in 2008, there was no LWLockAcquireOrWait, none of the Var stuff, the
padding was much less sophisticated, no tranches, no atomics, volatile
qualifiers all over the place...  and all of that has changed in the
last 5 years.  Pretty amazing, actually, IMHO.  If our LWLocks improve
as much between now and 2021 as they have between 2011 and now, it'll
be worth almost any amount of API breakage to get there.  I don't
personally have any plans or ideas that would involve breaking things
for extensions again any time soon, but I won't be very surprised if
somebody else comes up with one.

-- 
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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 12/15/16 8:40 AM, Stephen Frost wrote:
> > I don't follow why we can't change the syntax for CREATE USER to allow
> > specifying the verifier type independently.
> 
> That's what the last patch set I looked at actually does.

Well, same here, but it was quite a while ago and things have progressed
since then wrt SCRAM, as I understand it...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Peter Eisentraut]

On 12/15/16 8:40 AM, Stephen Frost wrote:
> I don't follow why we can't change the syntax for CREATE USER to allow
> specifying the verifier type independently.

That's what the last patch set I looked at actually does.

-- 
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] Declarative partitioning vs. sql_inheritance

[David Steele]

On 12/16/16 11:05 AM, Robert Haas wrote:

> If we were going to do anything about this,
> my vote would be to remove sql_inheritance.

+1.  This option is long past the intended shelf life.

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


-- 
Sent 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 vs. TRANSFORMs

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > That's a good point, we might be doing things wrong in other places in
> > the code by using FirstNormalObjectId on pre-8.1 servers.
> 
> > What I suggest then is an independent patch which uses a different
> > variable than FirstNormalObjectId and sets it correctly based on the
> > version of database that we're connecting to,
> 
> +1

Alright, here we go- patches for every currently supported branch, each
tested from that version back to 7.1, back to 7.3 with a user-defined
CAST, and back to 9.5 with a user-defined TRANSFORM.  Also includes
regression tests for the TAP test structure in master and 9.6.

> pg_dump never intended to support pre-7.0 servers.  I do have 7.0-7.3
> servers in captivity and can do testing if you like.

You are certainly welcome to test and make sure I didn't break anything
for 7.0 servers, but I don't *think* I changed any code paths which have
differences between 7.0 and 7.1 (which I did test against).  That said,
I'm honestly not entirely sure what getCasts() is doing querying out the
"casts" from a 7.1 or 7.0 database.  The query does work, but none of
the "casts" returned have an OID beyond datlastsysoid and I'm not really
sure how to create one or if creating one is really a supported
operation.  If someone was able to create something that getCasts()
would try to dump out, and it used only built-in functions, they will
at least get an error now letting them know that it failed, instead of
having that "cast" silently ignored.

Trying to adjust the query in getFuncs() to account for that case makes
me concerned that we'd actually break more than fix things, and I really
don't like the idea of trying to blindly fix it for 7.0.

Thanks!

Stephen
From 74bdd8fb687b6c182d6ce3fcc8b6bba01e7bbbcc Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Fri, 9 Dec 2016 15:28:03 -0500
Subject: [PATCH 1/2] For 8.0 servers, get last built-in oid from pg_database

We didn't start ensuring that all built-in objects had OIDs less than
16384 until 8.1, so for 8.0 servers we still need to query the value out
of pg_database.  We need this, in particular, to distinguish which casts
were built-in and which were user-defined.

For HEAD, we only worry about going back to 8.0, for the back-branches,
we also ensure that 7.0-7.4 work.

Discussion: https://www.postgresql.org/message-id/flat/20160504183952.GE10850%40tamriel.snowman.net
---
 src/bin/pg_dump/pg_dump.c | 60 ++-
 1 file changed, 54 insertions(+), 6 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7949aad..12eb018 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -96,6 +96,12 @@ bool		g_verbose;			/* User wants verbose narration of our
 /* subquery used to convert user ID (eg, datdba) to user name */
 static const char *username_subquery;
 
+/*
+ * For 8.0 and earlier servers, pulled from pg_database, for 8.1+ we use
+ * FirstNormalObjectId - 1.
+ */
+static Oid g_last_builtin_oid; /* value of the last builtin oid */
+
 /* The specified names/patterns should to match at least one entity */
 static int	strict_names = 0;
 
@@ -233,6 +239,7 @@ static char *convertRegProcReference(Archive *fout,
 		const char *proc);
 static char *convertOperatorReference(Archive *fout, const char *opr);
 static char *convertTSFunction(Archive *fout, Oid funcOid);
+static Oid findLastBuiltinOid_V71(Archive *fout, const char *);
 static void selectSourceSchema(Archive *fout, const char *schemaName);
 static char *getFormattedTypeName(Archive *fout, Oid oid, OidOptions opts);
 static void getBlobs(Archive *fout);
@@ -684,6 +691,20 @@ main(int argc, char **argv)
 		exit_horribly(NULL,
 		   "Exported snapshots are not supported by this server version.\n");
 
+	/*
+	 * Find the last built-in OID, if needed (prior to 8.1)
+	 *
+	 * With 8.1 and above, we can just use FirstNormalObjectId - 1.
+	 */
+	if (fout->remoteVersion < 80100)
+		g_last_builtin_oid = findLastBuiltinOid_V71(fout,
+	PQdb(GetConnection(fout)));
+	else
+		g_last_builtin_oid = FirstNormalObjectId - 1;
+
+	if (g_verbose)
+		write_msg(NULL, "last built-in OID is %u\n", g_last_builtin_oid);
+
 	/* Expand schema selection patterns into OID lists */
 	if (schema_include_patterns.head != NULL)
 	{
@@ -1494,7 +1515,7 @@ selectDumpableCast(CastInfo *cast, Archive *fout)
 	 * This would be DUMP_COMPONENT_ACL for from-initdb casts, but they do not
 	 * support ACLs currently.
 	 */
-	if (cast->dobj.catId.oid < (Oid) FirstNormalObjectId)
+	if (cast->dobj.catId.oid <= (Oid) g_last_builtin_oid)
 		cast->dobj.dump = DUMP_COMPONENT_NONE;
 	else
 		cast->dobj.dump = fout->dopt->include_everything ?
@@ -1526,7 +1547,7 @@ selectDumpableProcLang(ProcLangInfo *plang, Archive *fout)
 		plang->dobj.dump = DUMP_COMPONENT_NONE;
 	else
 	{
-		if (plang->dobj.catId.oid < (Oid) 

Re: [HACKERS] Declarative partitioning vs. sql_inheritance

[Peter Eisentraut]

On 12/16/16 11:05 AM, Robert Haas wrote:
> If we were going to do anything about this,
> my vote would be to remove sql_inheritance.

Go for it.

Let's also remove the table* syntax then.

-- 
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] PSQL commands: \quit_if, \quit_unless

[Pavel Stehule]

2016-12-16 21:18 GMT+01:00 Robert Haas :

> On Fri, Dec 16, 2016 at 12:28 PM, Pavel Stehule 
> wrote:
> > why do you need special operator for negation? there is only one use
> case.
> > It can be solved by \if_not
>
> That's exactly the kind of thing I *don't* want to do.  If you
> absolutely must have that and you can't wait until we get a full-blown
> expression evaluator, then just swap the \if side with the \else side
> and call it good.  The whole point here is to avoid introducing weird
> hacks for lack of a full expression evaluator that will just become
> annoyances once we have one.
>

I don't need it. Because we don't expect expression there, then "not" or
"if_not" is not necessary.

Regards

Pavel


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

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Robert Haas]

On Fri, Dec 16, 2016 at 12:28 PM, Pavel Stehule  wrote:
> why do you need special operator for negation? there is only one use case.
> It can be solved by \if_not

That's exactly the kind of thing I *don't* want to do.  If you
absolutely must have that and you can't wait until we get a full-blown
expression evaluator, then just swap the \if side with the \else side
and call it good.  The whole point here is to avoid introducing weird
hacks for lack of a full expression evaluator that will just become
annoyances once we have one.

-- 
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] Declarative partitioning vs. sql_inheritance

[Robert Haas]

On Fri, Dec 16, 2016 at 2:34 PM, David Fetter  wrote:
> It occurs to me this probably isn't the only GUC that's basically just
> a foot gun at this point.
>
> Is 10 a good time to sweep and clear them?

We never make any progress trying to do these things "in bulk".  If
you think there are other GUCs that need to be removed, start a thread
for each one, or closely related group, and let's talk about it on the
merits.  On this thread, let's just decide whether or not to remove
sql_inheritance.

-- 
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] Declarative partitioning vs. sql_inheritance

[David Fetter]

On Fri, Dec 16, 2016 at 11:05:21AM -0500, Robert Haas wrote:
> On Thu, Dec 15, 2016 at 10:40 AM, Dmitry Ivanov  
> wrote:
> > Hi everyone,
> >
> > Looks like "sql_inheritance" GUC is affecting partitioned tables:
> >
> > [breaks literally everything]
> >
> > I might be wrong, but IMO this should not happen. Queries involving update,
> > delete etc on partitioned tables are basically broken. Moreover, there's no
> > point in performing such operations on a parent table that's supposed to be
> > empty at all times.
> 
> An earlier version of Amit's patches tried to handle this by forcing
> sql_inheritance on for partitioned tables, but it wasn't
> well-implemented and I don't see the point anyway.  Sure, turning
> off sql_inheritance off for partitioned tables produces stupid
> results.  But turning off sql_inheritance for inheritance
> hierarchies also produces stupid results.  If we were going to do
> anything about this, my vote would be to remove sql_inheritance.

+1

It occurs to me this probably isn't the only GUC that's basically just
a foot gun at this point.

Is 10 a good time to sweep and clear them?

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


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

[Pavel Stehule]

2016-12-15 15:36 GMT+01:00 Pavel Stehule :

> Hi
>
> Most important features:
>>>
>>> 1. the values are stored in native types
>>> 2. access to content is protected by ACL - like the content of tables
>>> 3. the content is not MVCC based - no any cost of UPDATE
>>> 4. simple API allows access to content of variables from any supported
>>> environment.
>>>
>>
>> next update - setattr, getattr functions are working now
>>
>
> new update - rebased after partitioning patch
>

next update - with some initial doc

Regards

Pavel

>
> Regards
>
> Pavel
>
>
>>
>> notes, comments?
>>
>> Regards
>>
>> Pavel
>>
>>
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>>
 --
  Craig Ringer   http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

>>>
>>>
>>
>
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 11c2019..d80cab0 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1733,7 +1733,8 @@
S = sequence, v = view,
m = materialized view,
c = composite type, t = TOAST table,
-   f = foreign table
+   f = foreign table,
+   V = session variable
   
  
 
diff --git a/doc/src/sgml/ref/allfiles.sgml b/doc/src/sgml/ref/allfiles.sgml
index 77667bd..1af0748 100644
--- a/doc/src/sgml/ref/allfiles.sgml
+++ b/doc/src/sgml/ref/allfiles.sgml
@@ -89,6 +89,7 @@ Complete list of usable sgml source files in this directory.
 
 
 
+
 
 
 
@@ -132,6 +133,7 @@ Complete list of usable sgml source files in this directory.
 
 
 
+
 
 
 
diff --git a/doc/src/sgml/ref/create_variable.sgml b/doc/src/sgml/ref/create_variable.sgml
new file mode 100644
index 000..7ab402d
--- /dev/null
+++ b/doc/src/sgml/ref/create_variable.sgml
@@ -0,0 +1,152 @@
+
+
+
+ 
+  CREATE VARIABLE
+ 
+
+ 
+  CREATE VARIABLE
+  7
+  SQL - Language Statements
+ 
+
+ 
+  CREATE VARIABLE
+  define a new session secure typed variable
+ 
+
+ 
+
+CREATE VARIABLE [ IF NOT EXISTS ] name [ AS ] data_type ]
+
+ 
+
+ 
+  Description
+
+  
+   CREATE VARIABLE creates a new session variable.
+   These variables are memory only non transactional, but typed and
+   secure. The access is controlled by rights defined by command
+   GRANT and command REVOKE.
+  
+
+  
+   The session variable is initialized to NULL value. The value of
+   variable is lost when session is destroyed.
+  
+
+  
+   The session variable can be any scalar type or a array or composite
+   type.
+  
+
+  
+   After a variable is created, you use the special functions
+   getvar, getattr,
+   setattr and 
+   setvar. Functions
+   getattr, setattr
+   are used for access to any field of variable of composite
+   type.
+  
+ 
+
+ 
+  Parameters
+
+  
+   
+IF NOT EXISTS
+
+ 
+  Do not throw an error if a relation with the same name already exists.
+  A notice is issued in this case. Note that there is no guarantee that
+  the existing relation is anything like the variable that would have
+  been created - it might not even be a variable.
+ 
+
+   
+
+   
+name
+
+ 
+  The name (optionally schema-qualified) of the variable to be created.
+ 
+
+   
+
+   
+data_type
+
+ 
+  The name (optionally schema-qualified) of the data type ofvariable to be created.
+ 
+
+   
+  
+ 
+
+ 
+  Notes
+
+  
+   Use DROP VARIABLE to remove a variable.
+  
+ 
+
+ 
+  Examples
+
+  
+   Create an integer variable var1:
+
+CREATE VARIABLE var1 AS integer;
+
+  
+
+  
+   Set a value of this variable:
+
+SELECT setvar('var1', 10);
+
+ var1
+--
+   10
+
+  
+
+  
+   Select the value of this variable:
+
+SELECT getvar('var1');
+
+ var1
+--
+   10
+
+  
+ 
+
+ 
+  Compatibility
+
+  
+   CREATE VARIABLE is PostgreSQL feature
+  
+ 
+
+ 
+  See Also
+
+  
+   
+  
+ 
+
+
diff --git a/doc/src/sgml/ref/drop_variable.sgml b/doc/src/sgml/ref/drop_variable.sgml
new file mode 100644
index 000..ed8bb22
--- /dev/null
+++ b/doc/src/sgml/ref/drop_variable.sgml
@@ -0,0 +1,89 @@
+
+
+
+ 
+  DROP VARIABLE
+ 
+
+ 
+  DROP VARIABLE
+  7
+  SQL - Language Statements
+ 
+
+ 
+  DROP VARIABLE
+  remove a session variable
+ 
+
+ 
+
+DROP VARIABLE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]
+
+ 
+
+ 
+  Description
+
+  
+   DROP VARIABLE removes session variable.
+   A variable can only be dropped by its owner or a superuser.
+  
+ 
+
+ 
+  Parameters
+
+  
+   
+IF EXISTS
+
+ 
+  Do not throw an error if the variable does not exist. A notice is issued
+  in this case.
+ 
+
+   
+
+   
+name
+
+ 
+  The name (optionally schema-qualified) of a session variable.
+ 
+
+   
+  
+ 
+
+ 
+  Examples
+
+  
+   To remove the session variable var1:
+
+
+DROP VARIABLE var1;
+
+ 
+
+ 
+  Compatibility
+
+  
+   DROP VARIABLE is proprietary PostgreSQL command.
+  
+ 
+
+ 
+  See Also
+
+  
+   
+  
+ 
+
+
diff --git a/doc/src/sgml/ref/grant.sgml 

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Pavel Stehule]

2016-12-16 18:33 GMT+01:00 David G. Johnston :

> On Fri, Dec 16, 2016 at 10:28 AM, Pavel Stehule 
> wrote:
>
>> 2016-12-16 18:21 GMT+01:00 David G. Johnston 
>> :
>>
>>> On Fri, Dec 16, 2016 at 9:55 AM, Robert Haas 
>>> wrote:
>>>

 If the expected committed patch set includes #5 then this becomes a
>>> matter for reviewer convenience so never mind.  But if its at all possible
>>> for #5 to be punted down the road incorporating the eventual "not var" and
>>> "not(var)" syntax into #1 as a kind of shim would seem desirable.
>>>
>>
>> why do you need special operator for negation? there is only one use
>> case. It can be solved by \if_not
>>
>
> ​Not following the thread that closely and the section Robert quoted
> didn't include "\if_not" as a syntax option.  I figured the idea was to
> limit the number of backslash commands and leave the power in the
> expression evaluation.
>

without a expression you can store a negation to variable

I can imagine simple functional only expressions evaluated on client side.

\if not(table_exists('table_name'))

full expressions are not easy implemented without bigger changes in psql
parser design - and I don't see any reason why do some too complex there. I
would not to replace bash, perl, python or lua.


>
> David J.
> ​
>

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Tom Lane]

Robert Haas  writes:
> So I think it would be reasonable for somebody to implement \if,
> \elseif, \endif first, with the argument having to be, precisely, a
> single variable and nothing else (not even a negator).  Then a future
> patch could allow an expression there instead of a variable.  I don't
> think that would be any harder to review than going all the way to #5
> in one shot, and actually it might be simpler.

This seems like a reasonable implementation plan to me, not least because
it tackles the hard part first.  There's no doubt that we can build an
expression evaluator, but I'm not entirely sure how we're going to wedge
conditional eval or loops into psql's command reader.

(Or in other words, let's see \while ... \endloop in the minimal proposal
as well, or at least a sketch of how to get there.)

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] Creating a DSA area to provide work space for parallel execution

[Andres Freund]

On 2016-12-16 12:33:11 -0500, Robert Haas wrote:
> On Fri, Dec 16, 2016 at 12:32 PM, Robert Haas  wrote:
> > On Fri, Dec 16, 2016 at 12:28 PM, Andres Freund  wrote:
> >> On 2016-12-16 11:41:49 -0500, Robert Haas wrote:
> >>> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  
> >>> wrote:
> >>> > Thoughts?
> >>>
> >>> Hearing no objections, I've gone ahead and committed this.  If that
> >>> makes somebody really unhappy I can revert it, but I am betting that
> >>> the real story is that nobody cares about preserving T_ID().
> >>
> >> I don't care about T_ID, but I do care about breaking extensions using
> >> lwlocks like for the 3rd release in a row or such.  This is getting a
> >> bit ridiculous.
> >
> > Hmm, I hadn't thought about that.  :-)
> 
> Err, that was supposed to be :-(  As in sad, not happy.

Both work for me ;)


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

Re: [HACKERS] Creating a DSA area to provide work space for parallel execution

[Andres Freund]

On 2016-12-16 12:32:49 -0500, Robert Haas wrote:
> On Fri, Dec 16, 2016 at 12:28 PM, Andres Freund  wrote:
> > On 2016-12-16 11:41:49 -0500, Robert Haas wrote:
> >> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
> >> > Thoughts?
> >>
> >> Hearing no objections, I've gone ahead and committed this.  If that
> >> makes somebody really unhappy I can revert it, but I am betting that
> >> the real story is that nobody cares about preserving T_ID().
> >
> > I don't care about T_ID, but I do care about breaking extensions using
> > lwlocks like for the 3rd release in a row or such.  This is getting a
> > bit ridiculous.
> 
> Hmm, I hadn't thought about that.  :-)
> 
> I guess we could put back array_base/array_stride and just ignore
> them, but that hardly seems better.  Then we're stuck with that wart
> forever.

Yea, I don't think that's good either.  I'm all for evolving APIs when
necessary, but constantly breaking the same API release after release
seems indicative of needing to spend a bit more time on it in the first
round.  I've a few extensions (one of them citus) that work across
versions, and the ifdef-ery is significant.

Andres


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

Re: [HACKERS] Creating a DSA area to provide work space for parallel execution

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
> > Thoughts?
> 
> Hearing no objections, I've gone ahead and committed this.  If that
> makes somebody really unhappy I can revert it, but I am betting that
> the real story is that nobody cares about preserving T_ID().

AFAICT the comment on LWLockRegisterTranche is confused; it talks about
an allocated object being passed, but there isn't any.

-- 
Á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] Make pg_basebackup -x stream the default

[Fujii Masao]

On Fri, Dec 16, 2016 at 11:36 PM, Magnus Hagander  wrote:
> On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov 
> wrote:
>>
>> Usability review
>>
>> 
>>
>>
>> Patch sounds like a good idea and does what it supposed to do. /me in DBA
>> hat will be happy to have it.
>>
>> However, it makes '-x' parameter a bit confusing/surprising: specifying it
>> will be equivalent to '-X fetch' which is surprising regression from the new
>> default.
>
>
> This seems like a good idea, really.
>
> Given that we already break a number of other things around backups and
> replication in this release, it seems like a good time.
>
> I definitely think removing it is what we should do -- let's not redefine it
> to mean streaming, let's just get rid of -x altogether, and have people use
> -X streaming|fetch|none.
>
> What do others feel about this?

+1 to drop -x option. That's less confusing.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

[Andres Freund]

On 2016-12-16 10:12:42 -0500, Robert Haas wrote:
> On Wed, Dec 14, 2016 at 11:20 PM, Robert Haas  wrote:
> > I've got no problem with that at all, but I want to unbreak things
> > more or less immediately and then you/we can further improve it later.
> 
> Committed

Thanks. Although "Fix drastic slowdown when ..." or so might have been a
less confusing commit message ;)

> , although I realize now that doesn't fix Dilip's problem,
> only my (somewhat different) problem.

Indeed. And the fix for Dilip's thing also fixes your issue, albeit what
you committed still helps noticeably (even with the old hashtable).

> To fix his issue, we need something like your 0001.  Are you going to
> polish that up soon here?

Yes.


Andres


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

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[David G. Johnston]

On Fri, Dec 16, 2016 at 10:28 AM, Pavel Stehule 
wrote:

> 2016-12-16 18:21 GMT+01:00 David G. Johnston :
>
>> On Fri, Dec 16, 2016 at 9:55 AM, Robert Haas 
>> wrote:
>>
>>>
>>> If the expected committed patch set includes #5 then this becomes a
>> matter for reviewer convenience so never mind.  But if its at all possible
>> for #5 to be punted down the road incorporating the eventual "not var" and
>> "not(var)" syntax into #1 as a kind of shim would seem desirable.
>>
>
> why do you need special operator for negation? there is only one use case.
> It can be solved by \if_not
>

​Not following the thread that closely and the section Robert quoted didn't
include "\if_not" as a syntax option.  I figured the idea was to limit the
number of backslash commands and leave the power in the expression
evaluation.

David J.
​

Re: [HACKERS] Creating a DSA area to provide work space for parallel execution

[Robert Haas]

On Fri, Dec 16, 2016 at 12:32 PM, Robert Haas  wrote:
> On Fri, Dec 16, 2016 at 12:28 PM, Andres Freund  wrote:
>> On 2016-12-16 11:41:49 -0500, Robert Haas wrote:
>>> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
>>> > Thoughts?
>>>
>>> Hearing no objections, I've gone ahead and committed this.  If that
>>> makes somebody really unhappy I can revert it, but I am betting that
>>> the real story is that nobody cares about preserving T_ID().
>>
>> I don't care about T_ID, but I do care about breaking extensions using
>> lwlocks like for the 3rd release in a row or such.  This is getting a
>> bit ridiculous.
>
> Hmm, I hadn't thought about that.  :-)

Err, that was supposed to be :-(  As in sad, not happy.

> I guess we could put back array_base/array_stride and just ignore
> them, but that hardly seems better.  Then we're stuck with that wart
> forever.

-- 
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] Creating a DSA area to provide work space for parallel execution

[Robert Haas]

On Fri, Dec 16, 2016 at 12:28 PM, Andres Freund  wrote:
> On 2016-12-16 11:41:49 -0500, Robert Haas wrote:
>> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
>> > Thoughts?
>>
>> Hearing no objections, I've gone ahead and committed this.  If that
>> makes somebody really unhappy I can revert it, but I am betting that
>> the real story is that nobody cares about preserving T_ID().
>
> I don't care about T_ID, but I do care about breaking extensions using
> lwlocks like for the 3rd release in a row or such.  This is getting a
> bit ridiculous.

Hmm, I hadn't thought about that.  :-)

I guess we could put back array_base/array_stride and just ignore
them, but that hardly seems better.  Then we're stuck with that wart
forever.

-- 
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] PSQL commands: \quit_if, \quit_unless

[Pavel Stehule]

2016-12-16 18:21 GMT+01:00 David G. Johnston :

> On Fri, Dec 16, 2016 at 9:55 AM, Robert Haas 
> wrote:
>
>> On Mon, Dec 5, 2016 at 12:32 PM, Robert Haas 
>> wrote:
>> >>  - possible incremental implemention steps on this path:
>> >>
>> >>   (1) minimal condition and expression, compatible with
>> >>   a possible future full-blown expression syntax
>> >>
>> >>  \if :variable
>> >>  \if not :variable -- maybe \if ! :variable
>>
>
> We don't presently have a unary boolean operator named "!" so adding this
> variant would create an inconsistency
>

If we allow some complex expressions there, then it should be a SQL
expressions evaluated on server side.

There are two variants - 1. simple client side expression - can be
functional only, 2. complex server side expression.


>
>> So I think it would be reasonable for somebody to implement \if,
>> \elseif, \endif first, with the argument having to be, precisely, a
>> single variable and nothing else (not even a negator).  Then a future
>> patch could allow an expression there instead of a variable.  I don't
>> think that would be any harder to review than going all the way to #5
>> in one shot, and actually it might be simpler.
>
>
> ​I  worry about the case of disallowing negation in #1 and then not
> getting to #5 (in the same version) where the expression "not(var)" becomes
> possible.​
>
> If the expected committed patch set includes #5 then this becomes a matter
> for reviewer convenience so never mind.  But if its at all possible for #5
> to be punted down the road incorporating the eventual "not var" and
> "not(var)" syntax into #1 as a kind of shim would seem desirable.
>

why do you need special operator for negation? there is only one use case.
It can be solved by \if_not


>
> David J.
>
>
>
>

Re: [HACKERS] Creating a DSA area to provide work space for parallel execution

[Andres Freund]

On 2016-12-16 11:41:49 -0500, Robert Haas wrote:
> On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
> > Thoughts?
> 
> Hearing no objections, I've gone ahead and committed this.  If that
> makes somebody really unhappy I can revert it, but I am betting that
> the real story is that nobody cares about preserving T_ID().

I don't care about T_ID, but I do care about breaking extensions using
lwlocks like for the 3rd release in a row or such.  This is getting a
bit ridiculous.


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

Re: [HACKERS] invalid number of sync standbys in synchronous_standby_names

[Fujii Masao]

On Sat, Dec 17, 2016 at 1:00 AM, Tom Lane  wrote:
> Fujii Masao  writes:
>> When the number of sync standbys is set to 0 in s_s_names, the assersion
>> failure happens as follows. This means that current multiple syncrep code
>> assumes that the num of sync standbys must be greater than 0. But we forgot
>> to forbid users from setting that num to 0. This is an oversight in multiple
>> syncrep patch (so my fault...). Attached patch forbits that.
>
> Ooops.  Patch looks fine to me.

Thanks for the review! Pushed.

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] PSQL commands: \quit_if, \quit_unless

[David G. Johnston]

On Fri, Dec 16, 2016 at 9:55 AM, Robert Haas  wrote:

> On Mon, Dec 5, 2016 at 12:32 PM, Robert Haas 
> wrote:
> >>  - possible incremental implemention steps on this path:
> >>
> >>   (1) minimal condition and expression, compatible with
> >>   a possible future full-blown expression syntax
> >>
> >>  \if :variable
> >>  \if not :variable -- maybe \if ! :variable
>

We don't presently have a unary boolean operator named "!" so adding this
variant would create an inconsistency


> So I think it would be reasonable for somebody to implement \if,
> \elseif, \endif first, with the argument having to be, precisely, a
> single variable and nothing else (not even a negator).  Then a future
> patch could allow an expression there instead of a variable.  I don't
> think that would be any harder to review than going all the way to #5
> in one shot, and actually it might be simpler.


​I  worry about the case of disallowing negation in #1 and then not getting
to #5 (in the same version) where the expression "not(var)" becomes
possible.​

If the expected committed patch set includes #5 then this becomes a matter
for reviewer convenience so never mind.  But if its at all possible for #5
to be punted down the road incorporating the eventual "not var" and
"not(var)" syntax into #1 as a kind of shim would seem desirable.

David J.

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Robert Haas]

On Mon, Dec 5, 2016 at 12:32 PM, Robert Haas  wrote:
>>  - possible incremental implemention steps on this path:
>>
>>   (1) minimal condition and expression, compatible with
>>   a possible future full-blown expression syntax
>>
>>  \if :variable
>>  \if not :variable -- maybe \if ! :variable
>>...
>>  \endif
>>
>>   (2) add "\else"
>>
>>   (3) add "\elif ..." (or maybe "\elsif ..."?)
>>
>>   (4) add greater but limited expressions, compatible with a full blown
>>   expression syntax (eg \if :var/const  :var/const)
>>
>>   (5) add full-blown  support for \if, which suggest that
>>   it would also be available for \set
>>
>>
>> Does this looks okay, or does it need to be amended?
>>
>> A few comments:
>>
>> Given the experience with pgbench and the psql context, I do not think that
>> it would really need to go beyond step 2 above, but I agree that I may be
>> wrong and it is best to be prepared for that from the start. Given the
>> complexity and effort involved with (5), it seems wise to wait for a clearer
>> motivation with actual use-cases before going that far.
>
> Well, my vote would be to go all the way to #5 in one commit.
> Stopping short of that doesn't seem to me to save enough work to make
> much sense.  I don't think we're talking about anything all that
> complex, and it will make future improvements a lot simpler.

After having thought about this a little bit further and reread this a
bit more carefully, I would like to revise my position.  Really, what
I don't want to end up with is a hand-coded expression syntax that is
very limited which then has to be replaced with a full-blown lexer and
parser.  That is, I do not want to ever be at step "4" of this
proposal.

So I think it would be reasonable for somebody to implement \if,
\elseif, \endif first, with the argument having to be, precisely, a
single variable and nothing else (not even a negator).  Then a future
patch could allow an expression there instead of a variable.  I don't
think that would be any harder to review than going all the way to #5
in one shot, and actually it might be simpler.

-- 
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] Creating a DSA area to provide work space for parallel execution

[Robert Haas]

On Wed, Dec 14, 2016 at 3:25 PM, Robert Haas  wrote:
> Thoughts?

Hearing no objections, I've gone ahead and committed this.  If that
makes somebody really unhappy I can revert it, but I am betting that
the real story is that nobody cares about preserving T_ID().

-- 
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] Hash Indexes

[Robert Haas]

On Thu, Dec 15, 2016 at 11:33 AM, Amit Kapila  wrote:
> Attached are the two patches on top of remove-hash-wrtbuf.   Patch
> fix_dirty_marking_v1.patch allows to mark the buffer dirty in one of
> the corner cases in _hash_freeovflpage() and avoids to mark dirty
> without need in _hash_squeezebucket().  I think this can be combined
> with remove-hash-wrtbuf patch. fix_lock_chaining_v1.patch fixes the
> chaining behavior (lock next overflow bucket page before releasing the
> previous bucket page) was broken in _hash_freeovflpage().  These
> patches can be applied in series, first remove-hash-wrtbuf, then
> fix_dirst_marking_v1.patch and then fix_lock_chaining_v1.patch.

I committed remove-hash-wrtbuf and fix_dirty_marking_v1 but I've got
some reservations about fix_lock_chaining_v1.  ISTM that the natural
fix here would be to change the API contract for _hash_freeovflpage so
that it doesn't release the lock on the write buffer.  Why does it
even do that?  I think that the only reason why _hash_freeovflpage
should be getting wbuf as an argument is so that it can handle the
case where wbuf happens to be the previous block correctly.  Aside
from that there's no reason for it to touch wbuf.  If you fix it like
that then you should be able to avoid this rather ugly wart:

 * XXX Here, we are moving to next overflow page for writing without
 * ensuring if the previous write page is full.  This is annoying, but
 * should not hurt much in practice as that space will anyway be consumed
 * by future inserts.

-- 
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] Declarative partitioning vs. sql_inheritance

[Tom Lane]

Robert Haas  writes:
> An earlier version of Amit's patches tried to handle this by forcing
> sql_inheritance on for partitioned tables, but it wasn't
> well-implemented and I don't see the point anyway.  Sure, turning off
> sql_inheritance off for partitioned tables produces stupid results.
> But turning off sql_inheritance for inheritance hierarchies also
> produces stupid results.  If we were going to do anything about this,
> my vote would be to remove sql_inheritance.

+1.  If memory serves, we invented that GUC as a backwards-compatibility
hack, because once upon a time the default behavior was equivalent to
sql_inheritance = off.  But that was a long time ago; a bit of digging
in the git history suggests we changed it in 2000.  It's hard to believe
that anybody still relies on being able to turn it off.

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] postgres_fdw bug in 9.6

[Tom Lane]

Etsuro Fujita  writes:
> On 2016/12/16 11:25, Etsuro Fujita wrote:
>> As I said upthread, an alternative I am thinking is (1) to create an
>> equivalent nestloop join path using inner/outer paths of a foreign join
>> path, except when that join path implements a full join, in which case a
>> merge/hash join path is used, (2) store it in fdw_outerpath as-is, and
>> (3) during an EPQ recheck, apply postgresRecheckForeignScan to the outer
>> subplan created from the fdw_outerpath as-is.  What do you think about
>> that?

> Let me explain about #1 and #2 a bit more.  What I have in mind is:

> * modify postgresGetForeignPaths so that a simple foreign table scan 
> path is stored into the base relation's PgFdwRelationInfo.
> * modify postgresGetForeignJoinPaths so that
>  (a) a local join path for a 2-way foreign join is created using 
> simple foreign table scan paths stored in the base relations' 
> PgFdwRelationInfos, and stored into the join relation's PgFdwRelationInfo.
>  (b) a local join path for a 3-way foreign join, whose left input is 
> a 2-way foreign join, is created using a local join path stored in the 
> left input join relation's PgFdwRelationInfo and a simple foreign table 
> scan path stored into the right input base relation's PgFdwRelationInfo.
>  (c) Likewise for higher level foreign joins.
>  (d) local join paths created are passed to create_foreignscan_path 
> and stored into the fdw_outerpaths of the resulting ForeignPahts.

Hm, isn't this overcomplicated?  As you said earlier, we don't really
care all that much whether the fdw_outerpath is free of lower foreign
joins, because EPQ setup will select those lower join's substitute EPQ
plans anyway.  All that we need is that the EPQ tree be a legal
implementation of the join order with join quals applied at the right
places.  So I think the rule could be

"When first asked to produce a path for a given foreign joinrel, collect
the cheapest paths for its left and right inputs, and make a nestloop path
(or hashjoin path, if full join) from those, using the join quals needed
for the current input relation pair.  Use this as the fdw_outerpath for
all foreign paths made for the joinrel."

The important point here is that we avoid using a merge join because that
has assumptions about input ordering that likely won't be satisfied by
the child paths chosen through this method.  (I guess you could fall back
to it for the case of no quals in a fulljoin, because then the ordering
assumptions are vacuous anyway.)

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] bigint vs txid user confusion

[Robert Haas]

On Thu, Dec 15, 2016 at 3:02 AM, Craig Ringer  wrote:
> I really wish we could just change the pg_stat_activity and
> pg_stat_replication xid fields to be epoch qualified in a 64-bit wide
> 'fullxid' type, or similar.

I think that approach is worth considering.

-- 
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] Declarative partitioning - another take

[Robert Haas]

On Fri, Dec 16, 2016 at 3:02 AM, Amit Langote
 wrote:
> Aside from the above, I found few other issues and fixed them in the
> attached patches.  Descriptions follow:

To avoid any further mistakes on my part, can you please resubmit
these with each patch file containing a proposed commit message
including patch authorship information, who reported the issue, links
to relevant discussion if any, and any other attribution information
which I should not fail to include when committing?

-- 
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] Declarative partitioning vs. sql_inheritance

[Robert Haas]

On Thu, Dec 15, 2016 at 10:40 AM, Dmitry Ivanov  wrote:
> Hi everyone,
>
> Looks like "sql_inheritance" GUC is affecting partitioned tables:
>
> explain (costs off) select * from test;
>  QUERY PLAN  --
> Append
>   ->  Seq Scan on test
>   ->  Seq Scan on test_1
>   ->  Seq Scan on test_2
>   ->  Seq Scan on test_1_1
>   ->  Seq Scan on test_1_2
>   ->  Seq Scan on test_1_1_1
>   ->  Seq Scan on test_1_2_1
> (8 rows)
>
>
> set sql_inheritance = off;
>
>
> explain (costs off) select * from test;
>QUERY PLAN--
> Seq Scan on test
> (1 row)
>
>
> I might be wrong, but IMO this should not happen. Queries involving update,
> delete etc on partitioned tables are basically broken. Moreover, there's no
> point in performing such operations on a parent table that's supposed to be
> empty at all times.

An earlier version of Amit's patches tried to handle this by forcing
sql_inheritance on for partitioned tables, but it wasn't
well-implemented and I don't see the point anyway.  Sure, turning off
sql_inheritance off for partitioned tables produces stupid results.
But turning off sql_inheritance for inheritance hierarchies also
produces stupid results.  If we were going to do anything about this,
my vote would be to remove sql_inheritance.

-- 
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] invalid number of sync standbys in synchronous_standby_names

[Tom Lane]

Fujii Masao  writes:
> When the number of sync standbys is set to 0 in s_s_names, the assersion
> failure happens as follows. This means that current multiple syncrep code
> assumes that the num of sync standbys must be greater than 0. But we forgot
> to forbid users from setting that num to 0. This is an oversight in multiple
> syncrep patch (so my fault...). Attached patch forbits that.

Ooops.  Patch looks fine to me.

regards, tom lane


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

Re: [HACKERS] Linear vs. nonlinear planner cost estimates

[Robert Haas]

On Wed, Dec 14, 2016 at 3:46 PM, Tom Lane  wrote:
> The really *critical* aspect of this, which could cost far more than a
> few extra executions of a costing function, comes in if it damages
> add_path's ability to prune inferior paths early.  So we would not want
> to simply drop the startup_cost field; and we certainly don't want
> add_path calling this function during every path comparison, either.

Agreed.

> So my thought is that we'd need to keep startup_cost for add_path to
> use, though we could (and likely should) redefine it as below.  We
> should still be able to assume that if path A dominates path B at
> first-tuple and last-tuple costs, it dominates everywhere in between.

In theory, there's no reason that has to be true.   B could start out
a little more expensive than A, and then grow very slowly thereafter
until suddenly becoming super-expensive for the very last tuple, so
that for limits more than 1 but less than the total number of tuples
produced B actually wins.  This actually isn't a totally unrealistic
case: consider an index scan with a filter condition where all of the
tuples that pass the filter are clustered near the beginning.  Getting
any number of tuples that are actually present is cheap, but the very
last fetch that fails to find any more tuples is crushingly expensive.

That having been said, I doubt it's realistic to make the cost model
good enough to hope we'd get such cases correct, so making the
assumption that you propose is probably the right idea anyway.

> A thought that occurred to me after more study of the problem example
> is that the "good" index is a bit bigger than the "bad" one, meaning
> it will get charged a bit more in index descent costs.  With our current
> definition of startup_cost as being only the index descent cost, that
> means the good index looks worse on startup_cost than the bad one.  In
> this example, the good index should survive the add_path tournament
> anyway because it has better total_cost --- but it's easy to imagine
> cases where that wouldn't be true.  So I'm thinking that redefining
> startup_cost as time to fetch the first tuple would be a good thing
> in terms of making sure that desirable plans don't lose out in add_path.

+1.

-- 
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] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation [and 2 more messages] [and 1 more messages]

[Kevin Grittner]

I guess the preceding posts leave us with these guarantees about
read-only transactions which we might want to make explicit in the
documentation:

(1)  A serialization failure cannot be initially thrown on a COMMIT
attempt for a read-only transaction; however, if a subtransaction
catches a serialization failure exception on a statement within the
transaction, and doesn't re-throw it or throw any other error which
terminates the transaction, the serialization failure can show up
on the COMMIT attempt.  (NOTE:  We may want to check whether the
"doomed" flag is set on a write conflict for a serializable
transaction.  It seems to me that it should be, but that might have
been missed.  If so, that should be treated as a bug and fixed.)

(2)  A read-only transaction cannot show results inconsistent with
already-committed transactions, so there is no chance of needing to
discard results from a read-only transaction due to failure of the
transaction to commit.

Both of these should hold for both explicit read-only transactions
(which are set to READ ONLY after a BEGIN or START, or due to
default_transaction_read_only being set tot true and not
overridden), and implicit read-only transactions.  It is still
worthwhile to explicitly set serializable transactions to read-only
whenever possible, for performance reasons.

The idea that a serialization failure is not possible on the first
(or only) statement o a read-only transaction was in error, and
should be disregarded.

--
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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Robert Haas]

On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost  wrote:
> * Heikki Linnakangas (hlinn...@iki.fi) wrote:
>> On 12/14/2016 04:57 PM, Stephen Frost wrote:
>> >* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> >>On 12/14/16 5:15 AM, Michael Paquier wrote:
>> >>>I would be tempted to suggest adding the verifier type as a new column
>> >>>of pg_authid
>> >>
>> >>Yes please.
>> >
>> >This discussion seems to continue to come up and I don't entirely
>> >understand why we keep trying to shove more things into pg_authid, or
>> >worse, into rolpassword.
>>
>> I understand the relational beauty of having a separate column for
>> the verifier type, but I don't think it would be practical.
>
> I disagree.

Me, too.  I think the idea of moving everything into a separate table
that allows multiple verifiers is probably not a good thing to do just
right now, because that introduces a bunch of additional issues above
and beyond what we need to do to get SCRAM implemented.  There are
administration and policy decisions to be made there that we should
not conflate with SCRAM proper.

However, Heikki's proposal seems to be that it's reasonable to force
rolpassword to be of the form 'type:verifier' in all cases but not
reasonable to have separate columns for type and verifier.  Eh?

>> For
>> starters, we'd still like to have a self-identifying string format
>> like "scram-sha-256:", so that you can conveniently pass the
>> verifier as a string to CREATE USER.
>
> I don't follow why we can't change the syntax for CREATE USER to allow
> specifying the verifier type independently.  Generally speaking, I don't
> expect *users* to be providing actual encoded *verifiers* very often, so
> it seems like a bit of extra syntax that pg_dump has to use isn't that
> big of a deal.

We don't have to change the CREATE USER syntax at all.  It could just
split on the first colon and put the two halves of the string in
different places.  Of course, changing the syntax might be a good idea
anyway -- or not --- but the point is, right now, when you look at
rolpassword, there's not a clear rule for what kind of thing you've
got in there.  That's absolutely terrible design and has got to be
fixed.  Heikki's proposal of prefixing every entry with a type and a
':' will solve that problem and I'm not going to roll over in my grave
if we do it that way, but there is such a thing as normalization and
that technique could be applied here.

-- 
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] background sessions

[Andrew Borodin]

2016-12-16 20:17 GMT+05:00 Peter Eisentraut :
>> And one more thing... Can we have BackgroundSessionExecute() splitted
>> into two parts: start query and wait for results?
>> It would allow pg_background to reuse bgsession's code.
>
> Yes, I will look into that.

Thank you. I'm marking both patches as "Waiting for author", keeping
in mind that pg_background is waiting for bgsessions.
After updates I'll review these patches.

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] Hang in pldebugger after git commit : 98a64d0

[Robert Haas]

On Thu, Dec 15, 2016 at 4:17 AM, Amit Kapila  wrote:
> On Thu, Dec 15, 2016 at 10:04 AM, Andres Freund  wrote:
>> Hi,
>>
>> On 2016-12-12 16:46:38 +0900, Michael Paquier wrote:
>>> Ashutosh, could you try it and see if it improves things?
>>
>> So what's the theory of why this triggers pldebugger hangs, but
>> apparently causes not many other problems?
>>
>
> The theory is that with pldebugger latch event gets triggered before
> FD_READ due to which it seems like the second event gets lost and
> WaitForMultipleObjects() keeps on waiting indefinitely.  The probable
> reason is that we fail to reset the event due to which we are seeing
> this behavior. That seems to be required as per msdn as well, as
> pointed by Robert upthread.
>
> Find attached patch to implement the resetting of event only for the
> required case.  This fixes the reported problem.

After some study I don't think this is quite right yet.  The client
code is not obliged to call ModifyWaitEvent() before again calling
WaitEventSetWait().  I think it should be the responsibility of
WaitEventSetWaitBlock() to reset the event, if needed, before calling
WaitForMultipleObjects().  BTW, I suggest this rewritten comment:

/*--
 * FD_READ events are edge-triggered on Windows per
 * 
https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx
 * but users of the latch facility are entitled to expect
 * level-triggered behavior.  Indeed, this function itself
 * expects level-triggered behavior, since it doesn't guarantee
 * that a read event will be returned if the latch is set at the
 * same time.  Even if it did, the caller might drop that event
 * expecting it to reoccur on next call.  So, we must force
 * the event to be reset if this WaitEventSet is used again in
 * order to avoid an indefinite hang.
 *--
 */

The dashes keep pgindent from inserting a line break into the link.

-- 
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] jsonb problematic operators

[Tom Lane]

Geoff Winkless  writes:
> To look at this from the other angle, is there a reason why the jsonb
> indexes don't work with the jsonb_ functions but only with the
> operators? Is this something that could be changed easily?

Yes.  No.  However, if you're desperate, you could make SQL wrapper
functions:

regression=# create function my_jsonb_exists(jsonb, text) returns bool
regression-# language sql as 'select $1 ? $2';
CREATE FUNCTION
regression=# create table foo(f1 jsonb);
CREATE TABLE
regression=# create index on foo using gin(f1);
CREATE INDEX
regression=# explain select * from foo where my_jsonb_exists(f1, 'bar');
   QUERY PLAN
-
 Bitmap Heap Scan on foo  (cost=8.01..12.02 rows=1 width=32)
   Recheck Cond: (f1 ? 'bar'::text)
   ->  Bitmap Index Scan on foo_f1_idx  (cost=0.00..8.01 rows=1 width=0)
 Index Cond: (f1 ? 'bar'::text)
(4 rows)

This works because a simple SQL function like this will get inlined before
any interesting planning decisions are made.

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] background sessions

[Peter Eisentraut]

On 12/15/16 1:54 AM, Andrew Borodin wrote:
> And one more thing... Can we have BackgroundSessionExecute() splitted
> into two parts: start query and wait for results?
> It would allow pg_background to reuse bgsession's code.

Yes, I will look into that.

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

[Robert Haas]

On Wed, Dec 14, 2016 at 11:20 PM, Robert Haas  wrote:
> I've got no problem with that at all, but I want to unbreak things
> more or less immediately and then you/we can further improve it later.

Committed, although I realize now that doesn't fix Dilip's problem,
only my (somewhat different) problem.  To fix his issue, we need
something like your 0001.  Are you going to polish that up soon here?
I kinda want to get the regressions we've introduced fixed here.

-- 
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] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation [and 2 more messages] [and 1 more messages]

[Robert Haas]

On Fri, Dec 16, 2016 at 9:39 AM, Kevin Grittner  wrote:
> Good catch!

Thanks!

-- 
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] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation [and 2 more messages] [and 1 more messages]

[Kevin Grittner]

On Fri, Dec 16, 2016 at 8:24 AM, Robert Haas  wrote:
> On Thu, Dec 15, 2016 at 9:01 AM, Kevin Grittner  wrote:
>> I also realized some other properties of read-only transactions
>> that might interest you (and that I should probably document).
>> Since the only way for a read-only transaction to be the on
>> experiencing a serialization failure is if Tout commits before the
>> read-only transaction (which is always Tin) acquires its snapshot,
>> Tpivot is still running when Tin acquires its snapshot, Tpivot
>> commits before a serialization failure involving Tin is detected,
>> and *then* Tin reads a data set affected by the writes of Tpivot.
>> Since a snapshot is only acquired when a statement is run which
>> requires a snapshot, that means that a query run in an implicit
>> transaction (i.e., there is no START or BEGIN statement to
>> explicitly start it; the SELECT or other data-accessing statement
>> automatically starts the transaction so it has a valid context in
>> which to run) that does not write data can never return bad results
>> nor receive a serialization failure.  Nor can those things happen
>> on the *first* or *only* non-writing statement in an explicit
>> transaction.
>
> I don't understand this argument.  Every statement in a read-only,
> serializable transaction runs with the same snapshot, so I don't see
> how it can make a difference whether we're in the middle of running
> the first statement or the tenth.  Tpivot might commit in the middle
> of executing the first statement of the transaction, which might then
> -- later on during the execution of that same statement -- do
> something that causes it to acquire a bunch more SIREAD locks.

Good point.  For the read only transaction to be the one to receive
a serialization failure, it must acquire a snapshot while Tpivot is
still running, and read a data set which was affected by Tpivot,
and must do so after Tpivot has successfully committed.  However,
if the commit of Tpivot comes after Tin has parsed the statement,
determined that it is one that requires a snapshot, and acquired
its snapshot and before it reads the modified data set, Tin could
get the serialization failure.  Muddled thinking on my part to
think of acquiring the snapshot to be atomic with running the
statement which caused the snapshot to be acquired.

Good catch!

--
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] Make pg_basebackup -x stream the default

[Magnus Hagander]

On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov 
wrote:

> Usability review
>
> 
>
>
> Patch sounds like a good idea and does what it supposed to do. /me in DBA
> hat will be happy to have it.
>
> However, it makes '-x' parameter a bit confusing/surprising: specifying it
> will be equivalent to '-X fetch' which is surprising regression from the
> new default.
>

This seems like a good idea, really.

Given that we already break a number of other things around backups and
replication in this release, it seems like a good time.

I definitely think removing it is what we should do -- let's not redefine
it to mean streaming, let's just get rid of -x altogether, and have people
use -X streaming|fetch|none.

What do others feel about this?


One comment about docs:
>
>
>  Includes the required transaction log files (WAL files) in the
>
>  backup. This will include all transaction logs generated during
>
> -the backup. If this option is specified, it is possible to start
>
> -a postmaster directly in the extracted directory without the need
>
> -to consult the log archive, thus making this a completely
> standalone
>
> -backup.
>
> +the backup. Unless the option none is
> specified,
>
> +it is possible to start a postmaster directly in the extracted
>
> +directory without the need to consult the log archive, thus
>
> +making this a completely standalone backup.
>
> 
>
> 
>
> I suggest "method none" instead of "option
> none". I found the word "option" confusing in that
> sentence.
>
>
>
Sounds reasonable, will fix.

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

Re: [HACKERS] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation [and 2 more messages] [and 1 more messages]

[Robert Haas]

On Thu, Dec 15, 2016 at 9:01 AM, Kevin Grittner  wrote:
> I also realized some other properties of read-only transactions
> that might interest you (and that I should probably document).
> Since the only way for a read-only transaction to be the on
> experiencing a serialization failure is if Tout commits before the
> read-only transaction (which is always Tin) acquires its snapshot,
> Tpivot is still running when Tin acquires its snapshot, Tpivot
> commits before a serialization failure involving Tin is detected,
> and *then* Tin reads a data set affected by the writes of Tpivot.
> Since a snapshot is only acquired when a statement is run which
> requires a snapshot, that means that a query run in an implicit
> transaction (i.e., there is no START or BEGIN statement to
> explicitly start it; the SELECT or other data-accessing statement
> automatically starts the transaction so it has a valid context in
> which to run) that does not write data can never return bad results
> nor receive a serialization failure.  Nor can those things happen
> on the *first* or *only* non-writing statement in an explicit
> transaction.

I don't understand this argument.  Every statement in a read-only,
serializable transaction runs with the same snapshot, so I don't see
how it can make a difference whether we're in the middle of running
the first statement or the tenth.  Tpivot might commit in the middle
of executing the first statement of the transaction, which might then
-- later on during the execution of that same statement -- do
something that causes it to acquire a bunch more SIREAD locks. For
example, suppose the query involves calling a function which is
defined like this:

create or replace function getval(t text) returns integer as $$declare
q int; begin execute 'select aid from ' || t || ' limit 1;' into q;
return q; end$$ language plpgsql;

Obviously, every call to this function may grab an SIREAD lock on a new object.

Even without recourse to nested queries, I think we don't know which
index or heap pages will be locked at the start of execution.  We
acquire them as we go along.  At any point in that we could acquire
one which creates an rw-conflict with Tpivot, couldn't we?

-- 
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] [PROPOSAL] Temporal query processing with range types

[Peter Moser]

Am 16.12.2016 um 07:17 schrieb David Fetter:

On Wed, Dec 07, 2016 at 03:57:33PM +0100, Peter Moser wrote:

Am 05.12.2016 um 06:11 schrieb Haribabu Kommi:



On Tue, Oct 25, 2016 at 8:44 PM, Peter Moser > wrote:


We decided to follow your recommendation and add the patch to the
commitfest.


Path is not applying properly to HEAD.
Moved to next CF with "waiting on author" status.



We updated our patch. We tested it with the latest
commit dfe530a09226a9de80f2b4c3d5f667bf51481c49.


This looks neat, but it no longer applies to master.  Is a rebase in
the offing?


We rebased our patch on top of HEAD, that is, commit 
93513d1b6559b2d0805f0b02d312ee550e3d010b.



Best regards,
Anton, Johann, Michael, Peter
diff --git src/backend/commands/explain.c src/backend/commands/explain.c
index 0a669d9..09406d4 100644
--- src/backend/commands/explain.c
+++ src/backend/commands/explain.c
@@ -875,6 +875,12 @@ ExplainNode(PlanState *planstate, List *ancestors,
 		case T_SeqScan:
 			pname = sname = "Seq Scan";
 			break;
+		case T_TemporalAdjustment:
+			if(((TemporalAdjustment *) plan)->temporalCl->temporalType == TEMPORAL_TYPE_ALIGNER)
+pname = sname = "Adjustment(for ALIGN)";
+			else
+pname = sname = "Adjustment(for NORMALIZE)";
+			break;
 		case T_SampleScan:
 			pname = sname = "Sample Scan";
 			break;
diff --git src/backend/executor/Makefile src/backend/executor/Makefile
index 51edd4c..42801d3 100644
--- src/backend/executor/Makefile
+++ src/backend/executor/Makefile
@@ -25,6 +25,8 @@ OBJS = execAmi.o execCurrent.o execGrouping.o execIndexing.o execJunk.o \
nodeSamplescan.o nodeSeqscan.o nodeSetOp.o nodeSort.o nodeUnique.o \
nodeValuesscan.o nodeCtescan.o nodeWorktablescan.o \
nodeGroup.o nodeSubplan.o nodeSubqueryscan.o nodeTidscan.o \
-   nodeForeignscan.o nodeWindowAgg.o tstoreReceiver.o tqueue.o spi.o
+   nodeForeignscan.o nodeWindowAgg.o tstoreReceiver.o tqueue.o spi.o \
+   nodeTemporalAdjustment.o
+
 
 include $(top_srcdir)/src/backend/common.mk
diff --git src/backend/executor/execProcnode.c src/backend/executor/execProcnode.c
index 554244f..610d753 100644
--- src/backend/executor/execProcnode.c
+++ src/backend/executor/execProcnode.c
@@ -114,6 +114,7 @@
 #include "executor/nodeValuesscan.h"
 #include "executor/nodeWindowAgg.h"
 #include "executor/nodeWorktablescan.h"
+#include "executor/nodeTemporalAdjustment.h"
 #include "nodes/nodeFuncs.h"
 #include "miscadmin.h"
 
@@ -334,6 +335,11 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
  estate, eflags);
 			break;
 
+		case T_TemporalAdjustment:
+			result = (PlanState *) ExecInitTemporalAdjustment((TemporalAdjustment *) node,
+ estate, eflags);
+			break;
+
 		default:
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
 			result = NULL;		/* keep compiler quiet */
@@ -531,6 +537,10 @@ ExecProcNode(PlanState *node)
 			result = ExecLimit((LimitState *) node);
 			break;
 
+		case T_TemporalAdjustmentState:
+			result = ExecTemporalAdjustment((TemporalAdjustmentState *) node);
+			break;
+
 		default:
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
 			result = NULL;
@@ -779,6 +789,10 @@ ExecEndNode(PlanState *node)
 			ExecEndLimit((LimitState *) node);
 			break;
 
+		case T_TemporalAdjustmentState:
+			ExecEndTemporalAdjustment((TemporalAdjustmentState *) node);
+			break;
+
 		default:
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
 			break;
@@ -812,3 +826,4 @@ ExecShutdownNode(PlanState *node)
 
 	return planstate_tree_walker(node, ExecShutdownNode, NULL);
 }
+
diff --git src/backend/executor/nodeTemporalAdjustment.c src/backend/executor/nodeTemporalAdjustment.c
new file mode 100644
index 000..95d58a3
--- /dev/null
+++ src/backend/executor/nodeTemporalAdjustment.c
@@ -0,0 +1,537 @@
+#include "postgres.h"
+#include "executor/executor.h"
+#include "executor/nodeTemporalAdjustment.h"
+#include "utils/memutils.h"
+#include "access/htup_details.h"/* for heap_getattr */
+#include "utils/lsyscache.h"
+#include "nodes/print.h"		/* for print_slot */
+#include "utils/datum.h"		/* for datumCopy */
+
+/*
+ * #define TEMPORAL_DEBUG
+ * XXX PEMOSER Maybe we should use execdebug.h stuff here?
+ */
+#ifdef TEMPORAL_DEBUG
+static char*
+datumToString(Oid typeinfo, Datum attr)
+{
+	Oid			typoutput;
+	bool		typisvarlena;
+	getTypeOutputInfo(typeinfo, , );
+	return OidOutputFunctionCall(typoutput, attr);
+}
+
+#define TPGdebug(...) 	{ printf(__VA_ARGS__); printf("\n"); fflush(stdout); }
+#define TPGdebugDatum(attr, typeinfo) 	TPGdebug("%s = %s %ld\n", #attr, datumToString(typeinfo, attr), attr)
+#define TPGdebugSlot(slot) { printf("Printing Slot '%s'\n", #slot); print_slot(slot); fflush(stdout); }
+
+#else
+#define datumToString(typeinfo, attr)
+#define TPGdebug(...)
+#define TPGdebugDatum(attr, typeinfo)
+#define TPGdebugSlot(slot)
+#endif
+
+/*
+ * isLessThan

Re: [HACKERS] jsonb problematic operators

[Geoff Winkless]

On 16 December 2016 at 09:35, Craig Ringer  wrote:
> so it would be consistent with that to use ?? as a literal ? in the
> output query.
>
> This is also what PgJDBC does, per
> https://jdbc.postgresql.org/documentation/head/statement.html . So
> it's consistent .

"Me too".

To look at this from the other angle, is there a reason why the jsonb
indexes don't work with the jsonb_ functions but only with the
operators? Is this something that could be changed easily? It seems
like that would workaround this issue without requiring effort or
agreement from the PHP side.

Geoff


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

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Fujii Masao]

On Fri, Dec 16, 2016 at 5:04 PM, Fujii Masao  wrote:
> On Fri, Dec 16, 2016 at 2:38 PM, Michael Paquier
>  wrote:
>> On Thu, Dec 15, 2016 at 6:08 PM, Masahiko Sawada  
>> wrote:
>>> Attached latest v12 patch.
>>> I changed behavior of "N (standby_list)" to use the priority method
>>> and incorporated some review comments so far. Please review it.
>>
>> Some comments...
>>
>> +Another example of synchronous_standby_names for multiple
>> +synchronous standby is:
>> Here standby takes an 's'.
>>
>> +candidates. The master server will wait for at least 2 replies from 
>> them.
>> +s4 is an asynchronous standby since its name is not in the 
>> list.
>> +   
>> "will wait for replies from at least two of them".
>>
>> + * next-highest-priority standby. In quorum method, the all standbys
>> + * appearing in the list are considered as a candidate for quorum commit.
>> "the all" is incorrect. I think you mean "all the" instead.
>>
>> + * NIL if no sync standby is connected. In quorum method, all standby
>> + * priorities are same, that is 1. So this function returns the list of
>> This is not true. Standys have a priority number assigned. Though it does
>> not matter much for quorum groups, it gives an indication of their position
>> in the defined list.
>>
>>  #synchronous_standby_names = ''# standby servers that provide sync rep
>>  -   # number of sync standbys and comma-separated list of 
>> application_name
>>  +   # synchronization method, number of sync standbys
>>  +   # and comma-separated list of application_name
>>  # from standby(s); '*' = all
>> The formulation is funny here: "sync rep synchronization method".
>>
>> I think that Fujii-san has also some doc changes in his box. For anybody
>> picking up this patch next, it would be good to incorporate the things
>> I have noticed here.
>
> Yes, I will. Thanks!

Attached is the modified version of the patch. Barring objections, I will
commit this version.

Even after committing the patch, there will be still many source comments
and documentations that we need to update, for example,
in high-availability.sgml. We need to check and update them throughly later.

Regards,

-- 
Fujii Masao
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 3054,3094  include_dir 'conf.d'
  transactions waiting for commit will be allowed to proceed after
  these standby servers confirm receipt of their data.
  The synchronous standbys will be those whose names appear
! earlier in this list, and
  that are both currently connected and streaming data in real-time
  (as shown by a state of streaming in the
  
  pg_stat_replication view).
! Other standby servers appearing later in this list represent potential
! synchronous standbys. If any of the current synchronous
! standbys disconnects for whatever reason,
! it will be replaced immediately with the next-highest-priority standby.
! Specifying more than one standby name can allow very high availability.
 
 
  This parameter specifies a list of standby servers using
  either of the following syntaxes:
  
! num_sync ( standby_name [, ...] )
  standby_name [, ...]
  
  where num_sync is
  the number of synchronous standbys that transactions need to
  wait for replies from,
  and standby_name
! is the name of a standby server. For example, a setting of
! 3 (s1, s2, s3, s4) makes transaction commits wait
! until their WAL records are received by three higher-priority standbys
! chosen from standby servers s1, s2,
! s3 and s4.
! 
! 
! The second syntax was used before PostgreSQL
  version 9.6 and is still supported. It's the same as the first syntax
! with num_sync equal to 1.
! For example, 1 (s1, s2) and
! s1, s2 have the same meaning: either s1
! or s2 is chosen as a synchronous standby.
 
 
  The name of a standby server for this purpose is the
--- 3054,3124 
  transactions waiting for commit will be allowed to proceed after
  these standby servers confirm receipt of their data.
  The synchronous standbys will be those whose names appear
! in this list, and
  that are both currently connected and streaming data in real-time
  (as shown by a state of streaming in the
  
  pg_stat_replication view).
! Specifying more than one standby names can allow very high availability.
 
 
  This parameter specifies a list of standby servers using
  either of the following syntaxes:
  
! [FIRST] num_sync ( standby_name [, ...] )
! ANY num_sync ( standby_name [, ...] )
  

Re: [HACKERS] postgres_fdw bug in 9.6

[Etsuro Fujita]

On 2016/12/16 11:25, Etsuro Fujita wrote:

As I said upthread, an alternative I am thinking is (1) to create an
equivalent nestloop join path using inner/outer paths of a foreign join
path, except when that join path implements a full join, in which case a
merge/hash join path is used, (2) store it in fdw_outerpath as-is, and
(3) during an EPQ recheck, apply postgresRecheckForeignScan to the outer
subplan created from the fdw_outerpath as-is.  What do you think about
that?


Let me explain about #1 and #2 a bit more.  What I have in mind is:

* modify postgresGetForeignPaths so that a simple foreign table scan 
path is stored into the base relation's PgFdwRelationInfo.

* modify postgresGetForeignJoinPaths so that
(a) a local join path for a 2-way foreign join is created using 
simple foreign table scan paths stored in the base relations' 
PgFdwRelationInfos, and stored into the join relation's PgFdwRelationInfo.
(b) a local join path for a 3-way foreign join, whose left input is 
a 2-way foreign join, is created using a local join path stored in the 
left input join relation's PgFdwRelationInfo and a simple foreign table 
scan path stored into the right input base relation's PgFdwRelationInfo.

(c) Likewise for higher level foreign joins.
(d) local join paths created are passed to create_foreignscan_path 
and stored into the fdw_outerpaths of the resulting ForeignPahts.


I think that that would need special handling for foreign joins with 
sort orders; add an explicit sort to the local join paths, if needed.


I am thinking to provide a helper function that creates a local join 
path for (a), (b), and (c).


Best regards,
Etsuro Fujita




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

Re: [HACKERS] pg_basebackups and slots

[Petr Jelinek]

On 16/12/16 07:32, Magnus Hagander wrote:
> 
> On Dec 16, 2016 07:27, "Michael Paquier"  > wrote:
> 
> 
> 
> On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander
> > wrote:
> > So here's a patch that does this, for discussion. It implements the
> > following behavior for -X:
> >
> > * When used with <10.0 servers, behave just like before.
> > * When -S  is specified, behave just like before (use an
> existing
> > replication slot, fail if it does not exist)
> > * When used on 10.0 with no -S, create and use a temporary
> replication slot
> > while running, with name pg_basebackup_.
> > * When used with 10.0 with no -S but --no-slot specified, run
> without a slot
> > like before.
> 
> There are users using -X stream without a slot now because they
> don't want to
> cause WAL retention in pg_xlog and are ready for retries in taking
> the base
> backup... I am wondering if it is a good idea to change the default
> behavior
> and not introduce a new option like --temporary-slot, or
> --slot-mode=temp|persistent|none with none being the default
> instead. There
> are users caring about pg_xlog not filling up if pg_basebackup hangs
> on write
> for example. And it may be a good idea to be able to use
> --slot-mode=temp
> with -S/--slot actually to allow users to monitor it. If no slot
> name is given
> with --slot generating one looks fine to me.
> 
> 
> I really think the default should be "what most people want", and not
> "whatever is compatible with a mode that was necessary because we lacked
> infrastructure". 

+1 (btw glad you made the patch)

> 
> Yes there are some people who are fine with their backups failing. I'm
> willing to say there are *many* more who benefit from an automatic slot.
> 

I think the issue with slots taking over disk space should be fixed by
making it possible to cut off slots when necessary (ie some GUC that
would say how much behind slot can be at maximum, with something like 0
being unlimited like it's now) instead of trying to work around that in
every client.

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

[HACKERS] invalid number of sync standbys in synchronous_standby_names

[Fujii Masao]

Hi,

When the number of sync standbys is set to 0 in s_s_names, the assersion
failure happens as follows. This means that current multiple syncrep code
assumes that the num of sync standbys must be greater than 0. But we forgot
to forbid users from setting that num to 0. This is an oversight in multiple
syncrep patch (so my fault...). Attached patch forbits that.

TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 711)

Regards,

-- 
Fujii Masao
*** a/src/backend/replication/syncrep.c
--- b/src/backend/replication/syncrep.c
***
*** 924,929  check_synchronous_standby_names(char **newval, void **extra, GucSource source)
--- 924,936 
  			return false;
  		}
  
+ 		if (syncrep_parse_result->num_sync <= 0)
+ 		{
+ 			GUC_check_errmsg("number of synchronous standbys (%d) must be greater than zero",
+ 			 syncrep_parse_result->num_sync);
+ 			return false;
+ 		}
+ 
  		/* GUC extra value must be malloc'd, not palloc'd */
  		pconf = (SyncRepConfigData *)
  			malloc(syncrep_parse_result->config_size);

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

Re: [HACKERS] Speedup twophase transactions

[Stas Kelvich]

On 27 Sep 2016, at 03:30, Michael Paquier  wrote:OK. I am marking this patch as returned with feedback then. Lookingforward to seeing the next investigations.. At least this review hastaught us one thing or two.So, here is brand new implementation of the same thing.Now instead of creating pgproc entry for prepared transaction during recovery,I just store recptr/xid correspondence in separate 2L-list and deleting entries in thatlist if redo process faced commit/abort. In case of checkpoint or end of recoverytransactions remaining in that list are dumped to files in pg_twophase.Seems that current approach is way more simpler and patch has two times lessLOCs then previous one.-- Stas KelvichPostgres Professional: http://www.postgrespro.comThe Russian Postgres Company

twophase_recovery_list.diff
Description: Binary data

Re: [HACKERS] Slow I/O can break throttling of base backup

[Antonin Houska]

Antonin Houska  wrote:

> It seems to be my bug. I'll check tomorrow.

I could reproduce the problem by adding sufficient sleep time to the
loop.

> Magnus Hagander  wrote:
>> I wonder if the else if (sleep > 0) at the bottom of throttle() should just
>> be a simple else and always run, resetting last_throttle?

I agree. In fact, I could simplify the code even more.

Since (elapsed + sleep) almost equals to GetCurrentIntegerTimestamp(), we can
use the following statement unconditionally (I think I tried too hard to avoid
calling GetCurrentIntegerTimestamp too often in the original patch):

throttled_last = GetCurrentIntegerTimestamp();

Thus we can also get rid of the "else" branch that clears both "sleep" and
"wait_result".

(The patch contains minor comment refinement that I found useful when seeing
the code after years.)

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
new file mode 100644
index ffc7e58..40b3c11
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
*** throttle(size_t increment)
*** 1370,1395 
  		if (wait_result & WL_LATCH_SET)
  			CHECK_FOR_INTERRUPTS();
  	}
- 	else
- 	{
- 		/*
- 		 * The actual transfer rate is below the limit.  A negative value
- 		 * would distort the adjustment of throttled_last.
- 		 */
- 		wait_result = 0;
- 		sleep = 0;
- 	}
  
  	/*
! 	 * Only a whole multiple of throttling_sample was processed. The rest will
! 	 * be done during the next call of this function.
  	 */
  	throttling_counter %= throttling_sample;
  
! 	/* Once the (possible) sleep has ended, new period starts. */
! 	if (wait_result & WL_TIMEOUT)
! 		throttled_last += elapsed + sleep;
! 	else if (sleep > 0)
! 		/* Sleep was necessary but might have been interrupted. */
! 		throttled_last = GetCurrentIntegerTimestamp();
  }
--- 1370,1385 
  		if (wait_result & WL_LATCH_SET)
  			CHECK_FOR_INTERRUPTS();
  	}
  
  	/*
! 	 * As we work with integers, only whole multiple of throttling_sample was
! 	 * processed. The rest will be done during the next call of this function.
  	 */
  	throttling_counter %= throttling_sample;
  
! 	/*
! 	 * Time interval for the remaining amount and possible next increments
! 	 * starts now.
! 	 */
! 	throttled_last = GetCurrentIntegerTimestamp();
  }

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

[Mithun Cy]

Thanks Robert, I have tried to address all of the comments,
On Tue, Dec 6, 2016 at 2:20 AM, Robert Haas  wrote:
>
> +bucket = _hash_hashkey2bucket(hashkey, metap->hashm_maxbucket,
>metap->hashm_highmask,
>metap->hashm_lowmask);
>
> This hunk appears useless.
>
> +metap = (HashMetaPage)rel->rd_amcache;
>
> Whitespace.
>

Fixed.

>
> +/*  Cache the metapage data for next time*/
>
> Whitespace.
>

Fixed.

> +/* Check if this bucket is split after we have cached the
> metapage.
>
> Whitespace.
>

Fixed.

>
> Shouldn't _hash_doinsert() be using the cache, too?
>

Yes, we have an opportunity there, added same in code. But one difference
is at the end at-least once we need to read the meta page to split and/or
write. Performance improvement might not be as much as read-only.

I did some pgbench simple-update tests for same, with below changes.

-   "alter table pgbench_branches add primary key (bid)",
-   "alter table pgbench_tellers add primary key (tid)",
-   "alter table pgbench_accounts add primary key (aid)"
+   "create index pgbench_branches_bid on pgbench_branches
using hash (bid)",
+   "create index pgbench_tellers_tid on pgbench_tellers using
hash (tid)",
+   "create index pgbench_accounts_aid on pgbench_accounts
using hash (aid)"

And, removed all reference keys. But I see no improvements; I will further
do benchmarking for copy command and report same.

Clients

  After Meta page cache

Base Code

%imp

1

 2276.151633

2304.253611

-1.2195696631

32

 36816.596513

36439.552652

1.0347104549

64

 50943.763133

51005.236973

-0.120524565

128

 49156.980457

48458.275106

1.4418700407
Above result is median of three runs, and each run is for 30mins.

*Postgres Server settings:*
./postgres -c shared_buffers=8GB -N 200 -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

*pgbench settings:*
scale_factor = 300 (so database fits in shared_buffer)
Mode =  -M prepared -N (prepared simple-update).

*Machine used:*
power2 same as described as above.


> I think it's probably not a good idea to cache the entire metapage.  The
> only things that you are "really" trying to cache, I think, are
> hashm_maxbucket, hashm_lowmask, and hashm_highmask.  The entire
> HashPageMetaData structure is 696 bytes on my machine, and it doesn't
> really make sense to copy the whole thing into memory if you only need 16
> bytes of it.  It could even be dangerous -- if somebody tries to rely on
> the cache for some other bit of data and we're not really guaranteeing that
> it's fresh enough for that.
>
> I'd suggest defining a new structure HashMetaDataCache with members
> hmc_maxbucket, hmc_lowmask, and hmc_highmask.  The structure can have a
> comment explaining that we only care about having the data be fresh enough
> to test whether the bucket mapping we computed for a tuple is still
> correct, and that for that to be the case we only need to know whether a
> bucket has suffered a new split since we last refreshed the cache.
>

It is not only hashm_maxbucket, hashm_lowmask, and hashm_highmask (3
uint32s) but we also need

*uint32  hashm_spares[HASH_MAX_SPLITPOINTS],* for bucket number to
block mapping in "BUCKET_TO_BLKNO(metap, bucket)".

Note : #define HASH_MAX_SPLITPOINTS 32, so it is (3*uint32 + 32*uint32) =
35*4 = 140 bytes.

>
> The comments in this patch need some work, e.g.:
>
> -
> +   oopaque->hasho_prevblkno = maxbucket;
>
> No comment?
>
>
I have tried to improve commenting part in the new patch.

Apart from this, there seems to be some base bug in _hash_doinsert().
+* XXX this is useless code if we are only storing hash keys.

+   */

+if (itemsz > HashMaxItemSize((Page) metap))

+ereport(ERROR,

+(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),

+ errmsg("index row size %zu exceeds hash maximum %zu",

+itemsz, HashMaxItemSize((Page) metap)),

+   errhint("Values larger than a buffer page cannot be
indexed.")));

 "metap" (HashMetaPage) and Page are different data structure their member
types are not in sync, so should not typecast blindly as above. I think we
should remove this part of the code as we only store hash keys. So I have
removed same but kept the assert below as it is.
Also, there was a bug in the previous patch. I was not releasing the bucket
page lock if cached metadata is old, now same is fixed.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_hash_index_meta_page_07.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] jsonb problematic operators

[Craig Ringer]

On 16 December 2016 at 17:08, Matteo Beccati  wrote:
> Hi,
>
> On 12/12/2016 05:09, Craig Ringer wrote:
>> Does PDO let you double question marks to escape them, writing ?? or
>> \? instead of ? or anything like that?
>>
>> If not, I suggest that you (a) submit a postgres patch adding
>> alternative operator names for ? and ?|, and (b) submit a PDO patch to
>> allow ?? or \? as an escape for ? .
>
> For reference, my plan would be to get "\?" into PDO_pgsql for PHP 7.2.
> I've tried to get it into 7.1, but I was a bit too late into the RC
> process to safely do that.
>
> Since PDO itself has no escaping yet, I'm open to suggestions wrt to the
> actual escape method to use.

SQL typically uses doubling, such that the literal


  I'm

becomes

  'I''m'

and the identifier

  Bob "Kaboom" Jones

becomes

  "Bob ""Kaboom"" Jones"

so it would be consistent with that to use ?? as a literal ? in the
output query.

This is also what PgJDBC does, per
https://jdbc.postgresql.org/documentation/head/statement.html . So
it's consistent .

PHP usually uses backslash escapes, C-style. But this is UGLY when
you're escaping something in a string. Since \? already has a defined
meaning in PHP, you have to write \\? so that the first \ is consumed
by string parsing and the resulting \? is sent to PDO, which then
turns it into ? in the output SQL. This will confuse a lot of users.
Using ?? has no such issues.

-- 
 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] jsonb problematic operators

[Matteo Beccati]

Hi,

On 12/12/2016 05:09, Craig Ringer wrote:
> Does PDO let you double question marks to escape them, writing ?? or
> \? instead of ? or anything like that?
> 
> If not, I suggest that you (a) submit a postgres patch adding
> alternative operator names for ? and ?|, and (b) submit a PDO patch to
> allow ?? or \? as an escape for ? .

For reference, my plan would be to get "\?" into PDO_pgsql for PHP 7.2.
I've tried to get it into 7.1, but I was a bit too late into the RC
process to safely do that.

Since PDO itself has no escaping yet, I'm open to suggestions wrt to the
actual escape method to use.


Cheers
-- 
Matteo Beccati

Development & Consulting - http://www.beccati.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] Declarative partitioning - another take

[Amit Langote]

Hi Dmitry,

On 2016/12/16 0:40, Dmitry Ivanov wrote:
> Hi everyone,
> 
> Looks like "sql_inheritance" GUC is affecting partitioned tables:
> 
> explain (costs off) select * from test;
>  QUERY PLAN  --
> Append
>   ->  Seq Scan on test
>   ->  Seq Scan on test_1
>   ->  Seq Scan on test_2
>   ->  Seq Scan on test_1_1
>   ->  Seq Scan on test_1_2
>   ->  Seq Scan on test_1_1_1
>   ->  Seq Scan on test_1_2_1
> (8 rows)
> 
> 
> set sql_inheritance = off;
> 
> 
> explain (costs off) select * from test;
>QUERY PLAN--
> Seq Scan on test
> (1 row)
> 
> 
> I might be wrong, but IMO this should not happen. Queries involving
> update, delete etc on partitioned tables are basically broken. Moreover,
> there's no point in performing such operations on a parent table that's
> supposed to be empty at all times.
> 
> I've come up with a patch which fixes this behavior for UPDATE, DELETE,
> TRUNCATE and also in transformTableEntry(). It might be hacky, but it
> gives an idea.
> 
> I didn't touch RenameConstraint() and renameatt() since this would break
> ALTER TABLE ONLY command.

@@ -1198,6 +1198,12 @@ ExecuteTruncate(TruncateStmt *stmt)
rels = lappend(rels, rel);
relids = lappend_oid(relids, myrelid);

+   /* Use interpretInhOption() unless it's a partitioned table */
+   if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+   recurse = interpretInhOption(rv->inhOpt);
+   else
+   recurse = true;
+
if (recurse)
{
ListCell   *child;

If you see the else block of this if, you'll notice this:

else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("must truncate child tables too")));

So that you get this behavior:

# set sql_inheritance to off;
SET

# truncate p;
ERROR:  must truncate child tables too

# reset sql_inheritance;
RESET

# truncate only p;
ERROR:  must truncate child tables too

# truncate p;
TRUNCATE TABLE

Beside that, I initially had implemented the same thing as what you are
proposing here, but reverted to existing behavior at some point during the
discussion. I think the idea behind was to not *silently* ignore user
specified configuration and instead error out with appropriate message.
While it seems to work reasonably for DDL and maintenance commands (like
TRUNCATE above), things sound strange for SELECT/UPDATE/DELETE as you're
saying.

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] Declarative partitioning - another take

[Amit Langote]

On 2016/12/16 17:38, Greg Stark wrote:
> Just poking around with partitioning. I notice that "\d parent"
> doesn't list all the partitions, suggesting to use \d+ but a plain
> "\d" does indeed list the partitions. That seems a bit strange and
> also probably impractical if you have hundreds or thousands of
> partitions. Has this come up in previous discussions? Unfortunately
> it's proving a bit hard to search for "\d" :/

Do you mean a plain "\d" (without an argument) should not list tables that
are partitions?  I think that might be preferable.  That would mean, we
list only the root partitioned tables with a plain "\d".

Regarding "\d parent", it does the same thing as regular inheritance, but
using the term "partition" instead of "child table".  Without specifying a
+ (\d parent), one gets just "Number of partitions: # (Use \d+ to list
them.)" and with + (\d+ parent), one gets the full list of partitions
showing the partition bound with each.

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] Declarative partitioning - another take

[Amit Langote]

On 2016/12/16 17:02, Amit Langote wrote:
> [PATCH 2/7] Change how RelationGetPartitionQual() and related code works
> 
> Since we always want to recurse, ie, include the parent's partition
> constraint (if any), get rid of the argument recurse.
> 
> Refactor out the code doing the mapping of attnos of Vars in partition
> constraint expressions (parent attnos -> child attnos).  Move it to a
> separate function map_partition_varattnos() and call it from appropriate
> places.  It previously used to be done in get_qual_from_partbound(),
> which would lead to wrong results in certain multi-level partitioning
> cases, as the mapping would be done for immediate parent-partition pairs.
> Now in generate_partition_qual() which is the workhorse of
> RelationGetPartitionQual(), we first generate the full expression
> (considering all levels of partitioning) and then do the mapping from the
> root parent to a leaf partition.  It is also possible to generate
> partition constraint up to certain non-leaf level and then apply the
> same to leaf partitions of that sub-tree after suitable substitution
> of varattnos using the new map_partition_varattnos() directly.
> 
> Bug fix: ATExecAttachPartition() failed to do the mapping when attaching
> a partitioned table as partition. It is possible for the partitions of
> such table to have different attributes from the table being attached
> and/or the target partitioned table.

Oops, PATCH 2/7 attached with the previous email had a bug in it, whereby
map_partition_varattnos() was not applied to the partition constraint
expressions returned directly from the relcache (rd_partcheck) copy.
Attaching just the updated (only) PATCH 2 which fixes that.  Patches
1,3,4,5,6,7/7 from the previous email [1] are fine.  Sorry about that.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/c820c0eb-6935-6f84-8c6a-785fdff13...@lab.ntt.co.jp
>From b10228cfbdefc578138553b29a7658fb78c32de3 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 15 Dec 2016 16:27:04 +0900
Subject: [PATCH 2/7] Change how RelationGetPartitionQual() and related code
 works

Since we always want to recurse, ie, include the parent's partition
constraint (if any), get rid of the argument recurse.

Refactor out the code doing the mapping of attnos of Vars in partition
constraint expressions (parent attnos -> child attnos).  Move it to a
separate function map_partition_varattnos() and call it from appropriate
places.  It previously used to be done in get_qual_from_partbound(),
which would lead to wrong results in certain multi-level partitioning
cases, as the mapping would be done for immediate parent-partition pairs.
Now in generate_partition_qual() which is the workhorse of
RelationGetPartitionQual(), we first generate the full expression
(considering all levels of partitioning) and then do the mapping from the
root parent to a leaf partition.  It is also possible to generate
partition constraint up to certain non-leaf level and then apply the
same to leaf partitions of that sub-tree after suitable substitution
of varattnos using the new map_partition_varattnos() directly.

Bug fix: ATExecAttachPartition() failed to do the mapping when attaching
a partitioned table as partition. It is possible for the partitions of
such table to have different attributes from the table being attached
and/or the target partitioned table.
---
 src/backend/catalog/partition.c  | 97 
 src/backend/commands/tablecmds.c |  9 ++--
 src/backend/executor/execMain.c  |  3 +-
 src/backend/optimizer/util/plancat.c |  2 +-
 src/include/catalog/partition.h  |  3 +-
 5 files changed, 63 insertions(+), 51 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 9980582b77..694cb469e0 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -122,7 +122,7 @@ static List *get_qual_for_list(PartitionKey key, PartitionBoundSpec *spec);
 static List *get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec);
 static Oid get_partition_operator(PartitionKey key, int col,
 	   StrategyNumber strategy, bool *need_relabel);
-static List *generate_partition_qual(Relation rel, bool recurse);
+static List *generate_partition_qual(Relation rel);
 
 static PartitionRangeBound *make_one_range_bound(PartitionKey key, int index,
 	 List *datums, bool lower);
@@ -850,10 +850,6 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
 	PartitionBoundSpec *spec = (PartitionBoundSpec *) bound;
 	PartitionKey key = RelationGetPartitionKey(parent);
 	List	   *my_qual = NIL;
-	TupleDesc	parent_tupdesc = RelationGetDescr(parent);
-	AttrNumber	parent_attno;
-	AttrNumber *partition_attnos;
-	bool		found_whole_row;
 
 	Assert(key != NULL);
 
@@ -874,38 +870,48 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
  (int) key->strategy);
 	}
 
-	/*
-	 * Translate vars in the generated 

Re: [HACKERS] Declarative partitioning - another take

[Greg Stark]

Just poking around with partitioning. I notice that "\d parent"
doesn't list all the partitions, suggesting to use \d+ but a plain
"\d" does indeed list the partitions. That seems a bit strange and
also probably impractical if you have hundreds or thousands of
partitions. Has this come up in previous discussions? Unfortunately
it's proving a bit hard to search for "\d" :/


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

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Fujii Masao]

On Fri, Dec 16, 2016 at 2:38 PM, Michael Paquier
 wrote:
> On Thu, Dec 15, 2016 at 6:08 PM, Masahiko Sawada  
> wrote:
>> Attached latest v12 patch.
>> I changed behavior of "N (standby_list)" to use the priority method
>> and incorporated some review comments so far. Please review it.
>
> Some comments...
>
> +Another example of synchronous_standby_names for multiple
> +synchronous standby is:
> Here standby takes an 's'.
>
> +candidates. The master server will wait for at least 2 replies from them.
> +s4 is an asynchronous standby since its name is not in the 
> list.
> +   
> "will wait for replies from at least two of them".
>
> + * next-highest-priority standby. In quorum method, the all standbys
> + * appearing in the list are considered as a candidate for quorum commit.
> "the all" is incorrect. I think you mean "all the" instead.
>
> + * NIL if no sync standby is connected. In quorum method, all standby
> + * priorities are same, that is 1. So this function returns the list of
> This is not true. Standys have a priority number assigned. Though it does
> not matter much for quorum groups, it gives an indication of their position
> in the defined list.
>
>  #synchronous_standby_names = ''# standby servers that provide sync rep
>  -   # number of sync standbys and comma-separated list of 
> application_name
>  +   # synchronization method, number of sync standbys
>  +   # and comma-separated list of application_name
>  # from standby(s); '*' = all
> The formulation is funny here: "sync rep synchronization method".
>
> I think that Fujii-san has also some doc changes in his box. For anybody
> picking up this patch next, it would be good to incorporate the things
> I have noticed here.

Yes, I will. Thanks!

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] Declarative partitioning - another take

[Amit Langote]

On 2016/12/14 1:32, Robert Haas wrote:
> On Tue, Dec 13, 2016 at 1:58 AM, Amit Langote
>  wrote:
>> Attaching the above patch, along with some other patches posted earlier,
>> and one more patch fixing another bug I found.  Patch descriptions follow:
>>
>> 0001-Miscallaneous-code-and-comment-improvements.patch
>>
>> Fixes some obsolete comments while improving others.  Also, implements
>> some of Tomas Vondra's review comments.
> 
> Committed with some pgindenting.

Thanks!

>> 0002-Miscallaneous-documentation-improvements.patch
>>
>> Fixes inconsistencies and improves some examples in the documentation.
>> Also, mentions the limitation regarding row movement.
> 
> Ignored because I committed what I think is the same or a similar
> patch earlier.  Please resubmit any remaining changes.

It seems this patch is almost the same thing as what you committed.

>> 0003-Invalidate-the-parent-s-relcache-after-partition-cre.patch
>>
>> Fixes a bug reported by Tomas, whereby a parent's relcache was not
>> invalidated after creation of a new partition using CREATE TABLE PARTITION
>> OF.  This resulted in tuple-routing code not being to able to find a
>> partition that was created by the last command in a given transaction.
> 
> Shouldn't StorePartitionBound() be responsible for issuing its own
> invalidations, as StorePartitionKey() already is?  Maybe you'd need to
> pass "parent" as another argument, but that way you know you don't
> have the same bug at some other place where the function is called.

OK, done that way in PATCH 1/7 (of the attached various patches as
described below).

>> 0004-Fix-a-bug-of-insertion-into-an-internal-partition.patch
>>
>> Fixes a bug I found this morning, whereby an internal partition's
>> constraint would not be enforced if it is targeted directly.  See example
>> below:
>>
>> create table p (a int, b char) partition by range (a);
>> create table p1 partition of p for values from (1) to (10) partition by
>> list (b);
>> create table p1a partition of p1 for values in ('a');
>> insert into p1 values (0, 'a');  -- should fail, but doesn't
> 
> I expect I'm missing something here, but why do we need to hoist
> RelationGetPartitionQual() out of InitResultRelInfo() instead of just
> having BeginCopy() pass true instead of false?
> 
> (Also needs a rebase due to the pgindent cleanup.)

In this case, we want to enforce only the main target relation's partition
constraint (only needed if it happens to be an internal node partition),
not leaf partitions', because the latter is unnecessary.

We do InitResultRelInfo() for every leaf partition.  What
RelationGetPartitionQual() would return to InitResultRelInfo() is the
partition constraint of the individual leaf partitions, which as just
mentioned is unnecessary, and also would be inefficient.  With the
proposed patch, we only retrieve the partition constraint for the targeted
table (if there is any) once.  However, when assigning to
ri_PartitionCheck of individual leaf partition's ResultRelInfo, we still
must map any Vars in the expression from the target tables's attnos to the
corresponding leaf partition's attnos (previous version of the patch
failed to do that).

>> 0005-Fix-a-tuple-routing-bug-in-multi-level-partitioned-t.patch
>>
>> Fixes a bug discovered by Dmitry Ivanov, whereby wrong indexes were
>> assigned to the partitions of lower levels (level > 1), causing spurious
>> "partition not found" error as demonstrated in his email [1].
>
> Committed.  It might have been good to include a test case.

Agreed, added tests in the attached patch PATCH 7/7.


Aside from the above, I found few other issues and fixed them in the
attached patches.  Descriptions follow:

[PATCH 1/7] Invalidate the parent's relcache after partition creation.

Invalidate parent's relcache after a partition is created using CREATE
TABLE PARTITION OF.  (Independent reports by Keith Fiske and David Fetter)

[PATCH 2/7] Change how RelationGetPartitionQual() and related code works

Since we always want to recurse, ie, include the parent's partition
constraint (if any), get rid of the argument recurse.

Refactor out the code doing the mapping of attnos of Vars in partition
constraint expressions (parent attnos -> child attnos).  Move it to a
separate function map_partition_varattnos() and call it from appropriate
places.  It previously used to be done in get_qual_from_partbound(),
which would lead to wrong results in certain multi-level partitioning
cases, as the mapping would be done for immediate parent-partition pairs.
Now in generate_partition_qual() which is the workhorse of
RelationGetPartitionQual(), we first generate the full expression
(considering all levels of partitioning) and then do the mapping from the
root parent to a leaf partition.  It is also possible to generate
partition constraint up to certain non-leaf level and then apply the
same to leaf partitions of that sub-tree after suitable substitution
of varattnos using the