Re: [HACKERS] parallel mode and parallel contexts

[Amit Kapila]

On Tue, Apr 21, 2015 at 11:38 PM, Robert Haas  wrote:
>
>
> After thinking about it a bit more, I realized that even if we settle
> on some solution to that problem, there's another issues: the
> wait-edges created by this system don't really have the same semantics
> as regular lock waits.  Suppose A is waiting on a lock held by B and B
> is waiting for a lock held by A; that's a deadlock.  But if A is
> waiting for B to write to a tuple queue and B is waiting for A to read
> from a tuple queue, that's not a deadlock if the queues in question
> are the same.  If they are different queues, it might be a deadlock,
> but then again maybe not.  It may be that A is prepared to accept B's
> message from one queue, and that upon fully receiving it, it will do
> some further work that will lead it to write a tuple into the other
> queue.  If so, we're OK; if not, it's a deadlock.

I agree that the way deadlock detector works for locks, it might not
be same as it needs to work for communication buffers (tuple queues).
Here I think we also need to devise some different way to remove resources
from wait/resource queue, it might not be a good idea to do it similar to
locks
as locks are released at transaction end whereas this new resources
(communication buffers) don't operate at transaction boundary.

> I'm not sure
> whether you'll want to argue that that is an implausible scenario, but
> I'm not too sure it is.  The worker could be saying "hey, I need some
> additional piece of your backend-local state in order to finish this
> computation", and the master could then provide it.  I don't have any
> plans like that, but it's been suggested previously by others, so it's
> not an obviously nonsensical thing to want to do.
>

If such a thing is not possible today and also it seems that is a not a
good design to solve any problem, then why to spend too much effort
in trying to find ways to detect deadlocks for the same.

> A further difference in the semantics of these wait-edges is that if
> process A is awaiting AccessExclusiveLock on a resource held by B, C,
> and D at AccessShareLock, it needs to wait for all of those processes
> to release their locks before it can do anything else.  On the other
> hand, if process A is awaiting tuples from B, C, and D, it just needs
> ONE of those processes to emit tuples in order to make progress.  Now
> maybe that doesn't make any difference in practice, because even if
> two of those processes are making lively progress and A is receiving
> tuples from them and processing them like gangbusters, that's probably
> not going to help the third one get unstuck.  If we adopt the approach
> of discounting that possibility, then as long as the parallel leader
> can generate tuples locally (that is, local_scan_done = false) we
> don't report the deadlock, but as soon as it can no longer do that
> (local_scan_done = true) then we do, even though we could still
> theoretically read more tuples from the non-stuck workers.  So then
> you have to wonder why we're not solving problem #1, because the
> deadlock was just as certain before we generated the maximum possible
> number of tuples locally as it was afterwards.
>

I think the deadlock detection code should run if anytime we have to
wait for more than deadlock timeout.  Now I think the important point
here is when to actually start waiting, as per current parallel_seqscan
patch we wait only after checking the tuples from all queues, we could
have done it other way (wait if we can't fetch from one queue) as well.
It seems both has pros and cons, so we can proceed assuming current
way is okay and we can consider to change it in future once we find some
reason for the same.

Having said above, I feel this is not the problem that we should try to
solve at this point unless there is any scenario where we could hit
deadlock due to communication buffers.  I think such a solution would
be required for advanced form of parallelism (like intra-query parallelism).
By the way, why are we trying to solve this problem, is there any way
with which we can hit it for parallel sequential scan?


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

Re: [HACKERS] a fast bloat measurement tool (was Re: Measuring relation free space)

[Amit Kapila]

On Fri, Apr 24, 2015 at 8:05 PM, Tomas Vondra 
wrote:
>
>
>
> On 04/24/15 14:58, Amit Kapila wrote:
>>
>> On Fri, Apr 24, 2015 at 8:46 AM, Abhijit Menon-Sen > > wrote:
>>  >
>>  > At 2015-04-24 08:35:40 +0530, amit.kapil...@gmail.com
>>  wrote:
>>  > >
>>  > > > Just stick a PG_RETURN_NULL() at the end?
>>  > >
>>  > > That should also work.
>>  >
>>  > OK, updated patch attached with just that one change.
>>  >
>>
>> Patch looks good to me, will mark it as Ready for Committer if Tomas
>> or anyone else doesn't have more to add in terms of review.
>
>
> FWIW, I'm OK with the patch as is. Your reviews were spot on.
>

Thanks, I have marked this patch as Ready For Committer.


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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

"David G. Johnston"  writes:
> On Saturday, April 25, 2015, Tom Lane  wrote:
>> Still another case that needs to be thought about is "create table likeit
>> (like base) without oids" where base does have OIDs.  Probably the right
>> thing here is to let the WITHOUT OIDS spec override what we see in base.

> Why are oids special in this manner?  No other inherited column can be
> omitted from the child table.

Hm, good point; INHERITS will silently override such a specification:

regression=# create table base1 (f1 int) with oids;
CREATE TABLE
regression=# create table c2 () inherits (base1) without oids;
CREATE TABLE
regression=# \d+ c2
  Table "public.c2"
 Column |  Type   | Modifiers | Storage | Stats target | Description 
+-+---+-+--+-
 f1 | integer |   | plain   |  | 
Inherits: base1
Has OIDs: yes

> Though I guess unlike inherits there is no
> reason to mandate the final result be identical to the base table - though
> here is something to be said for pointing out the inconsistency and
> requiring the user to alter table if indeed they want to have the oid-ness
> changed.

Yeah, LIKE doesn't necessarily have to behave the same as INHERITS;
but probably we should follow that precedent unless we have a specific
argument not to.  Which I don't.

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] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[David G. Johnston]

On Saturday, April 25, 2015, Tom Lane  wrote:

>
> It's perhaps debatable whether it should act that way, but in the absence
> of complaints from the field, I'm hesitant to change these cases.  It
> might be better if the effective behavior were "table gets OIDs if
> default_with_oids = true or WITH OIDS is given or base table has OIDs".


+1


>
> Still another case that needs to be thought about is "create table likeit
> (like base) without oids" where base does have OIDs.  Probably the right
> thing here is to let the WITHOUT OIDS spec override what we see in base.
>
>
Why are oids special in this manner?  No other inherited column can be
omitted from the child table.  Though I guess unlike inherits there is no
reason to mandate the final result be identical to the base table - though
here is something to be said for pointing out the inconsistency and
requiring the user to alter table if indeed they want to have the oid-ness
changed.

David J.

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

Bruce Momjian  writes:
> I have changed the default value back to the function call as it should
> have been all along;  patch attached.  I will revisit this for 9.6
> unless I hear otherwise.

I still don't like this patch one bit.  I don't think that this code
should be modifying stmt->options that way.  Also, you have not addressed
whether this is even the right semantics.  In particular, currently
default_with_oids will force an OID column to exist regardless of whether
the LIKE-referenced table has them:

regression=# create table base (f1 int);
CREATE TABLE
regression=# set default_with_oids = true;
SET
regression=# create table likeit (like base);
CREATE TABLE
regression=# \d+ base
 Table "public.base"
 Column |  Type   | Modifiers | Storage | Stats target | Description 
+-+---+-+--+-
 f1 | integer |   | plain   |  | 

regression=# \d+ likeit
Table "public.likeit"
 Column |  Type   | Modifiers | Storage | Stats target | Description 
+-+---+-+--+-
 f1 | integer |   | plain   |  | 
Has OIDs: yes

Another variant is "create table likeit (like base) with oids".

It's perhaps debatable whether it should act that way, but in the absence
of complaints from the field, I'm hesitant to change these cases.  It
might be better if the effective behavior were "table gets OIDs if
default_with_oids = true or WITH OIDS is given or base table has OIDs".

Still another case that needs to be thought about is "create table likeit
(like base) without oids" where base does have OIDs.  Probably the right
thing here is to let the WITHOUT OIDS spec override what we see in base.

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] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

On Sat, Apr 25, 2015 at 06:15:25PM -0400, Tom Lane wrote:
> Robert Haas  writes:
> > No, I just misread your email.  I thought you said you had attached
> > the patch; rereading it, I see that you said you had applied the
> > patch.  Silly me.
> 
> The real problem with this patch is it's wrong.  Specifically, it broke
> the other case I mentioned in my original email:
> 
> regression=# create table src2 (f1 int, primary key(oid)) with oids;
> ERROR:  column "oid" named in key does not exist
> LINE 1: create table src2 (f1 int, primary key(oid)) with oids;
>^

Wow, thanks for seeing that mistake.  I had things just fine, but then I
decided to optimize it and forgot that this code is used in non-LIKE
situations.  Reverted.

> That works in 9.4, and was still working in HEAD as of my original email.
> I think the patch's logic for attaching made-up OIDS options is actually
> backwards (it's adding TRUE where it should add FALSE and vice versa),
> but in any case I do not like the dependence on default_with_oids that
> was introduced by the patch.  I am not sure there's any guarantee that
> default_with_oids can't change between parsing and execution of a CREATE
> TABLE command.

I have changed the default value back to the function call as it should
have been all along;  patch attached.  I will revisit this for 9.6
unless I hear otherwise.

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

  + Everyone has their own god. +
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 1fc8c2c..40fa9d6
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
***
*** 56,61 
--- 56,62 
  #include "rewrite/rewriteManip.h"
  #include "utils/acl.h"
  #include "utils/builtins.h"
+ #include "utils/guc.h"
  #include "utils/lsyscache.h"
  #include "utils/rel.h"
  #include "utils/syscache.h"
*** transformCreateStmt(CreateStmt *stmt, co
*** 281,286 
--- 282,298 
  	 * Output results.
  	 */
  	stmt->tableElts = cxt.columns;
+ 	/*
+ 	 * Add WITH/WITHOUT OIDS, if necessary.  A literal statement-specified
+ 	 * WITH/WITHOUT OIDS will still take precedence because the first
+ 	 * matching "oids" in "options" is used.
+ 	 */
+ 	if (!interpretOidsOption(stmt->options, true) && cxt.hasoids)
+ 		stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(TRUE)));
+ 	else if (interpretOidsOption(stmt->options, true) && !cxt.hasoids)
+ 		stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(FALSE)));
  	stmt->constraints = cxt.ckconstraints;
  
  	result = lappend(cxt.blist, stmt);
*** transformTableLikeClause(CreateStmtConte
*** 849,854 
--- 861,868 
  		}
  	}
  
+ 	cxt->hasoids = relation->rd_rel->relhasoids;
+ 
  	/*
  	 * Copy CHECK constraints if requested, being careful to adjust attribute
  	 * numbers so they match the child.

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

Re: [HACKERS] forward vs backward slashes in msvc build code

[Andrew Dunstan]

On 04/25/2015 08:01 PM, Michael Paquier wrote:

On Sun, Apr 26, 2015 at 2:19 AM, Andrew Dunstan  wrote:

On 04/25/2015 10:53 AM, Michael Paquier wrote:

On Sat, Apr 25, 2015 at 9:58 PM, Peter Eisentraut wrote:

On 4/24/15 12:22 AM, Michael Paquier wrote:

Now that the stuff related to the move from contrib/ to src/bin/,
modulescheck and tmp_install has been committed, shouldn't we give a
new shot at this patch? Attached is a rebased version.

done

Thanks, bowerbird is running fine:

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2015-04-25%2013%3A31%3A06



But currawong is not - it runs an older version of the MS build tools. See
:

Bad format filename 'src/backend/access/brin/brin.c'
  at src/tools/msvc/VCBuildProject.pm line 73.
 VCBuildProject::WriteFiles(VC2008Project=HASH(0x9a7274),
"*Project::F") called at src/tools/msvc/Project.pm line 363
 Project::Save(VC2008Project=HASH(0x9a7274)) called at
src/tools/msvc/Solution.pm line 539
 Solution::Save(VS2008Solution=HASH(0xc00b7c)) called at
src/tools/msvc/Mkvcbuild.pm line 656
 Mkvcbuild::mkvcbuild(HASH(0xbed464)) called at build.pl line 36

Oops, attached is a patch that should make currawong happier..


OK, pushed.

cheers

andrew


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

[HACKERS] Temporal extensions

[Dave Jones]

Hi all,

My apologies I couldn't directly respond to the earlier thread on this
subject
(http://www.postgresql.org/message-id/50d99278.3030...@dc.baikal.ru) but
I wasn't subscribed to the list at that point.

I've been working on a conversion of several utilities I wrote for
another engine, and was wondering if there was any interest in seeing
any of them added to contrib/ at some point in the vague undefined future?

The github repo for the source is here:

  https://github.com/waveform80/oliphant

The documentation for the extensions (currently) resides here:

  http://oliphant.readthedocs.org/en/latest/

(Obviously the docs would need conversion to docbook for any official
sort of patch; I've used docbook in the past, I mainly wrote the docs in
rst because that's what I'm most used to at the moment and it's awfully
convenient :)

The major extension is "history" which is intended for the tracking of
temporal data. This comprises various functions for the creation of
history tables, their associated triggers, and utility views for
providing alternate transformations of the historical data.

The other extensions ("assert", "auth", "merge") all really exist in
support of "history" (to some greater or lesser degree) but I'd split
them out previously as on the original engine (DB2) they served some
purpose and perhaps they can serve some here. However, I'd have no issue
removing such dependence and simply merging the relevant code into the
"history" extension if that was deemed appropriate (as you can guess,
it's "history" I'm really interested in.).

The extensions are all written in plpgsql (no C), and there's some
rudimentary tests of their functionality under the tests/ dir (which
again would need converting/expanding in the event they were to become
"official"). That said, having read through the former thread
(referenced above) I get the impression there's still plenty I need to
think about.

I predict some questions are bound to arise, so I'll provide some brief
answers introductory below:

Q. Why not build on the existing work?

Honestly, I didn't think to go looking for it until I considered posting
to this list, and it's been enlightening seeing what others have done in
this space (doh!). Personally, I started tinkering with this sort of
stuff long ago, in a database engine far far away* and the stuff I had,
had fulfilled all my needs in this space. So, when it came time to move
onto postgres that's where I started (for better or worse).

* http://groups.google.com/group/comp.databases.ibm-db2/msg/e84aeb1f6ac87e6c

Q. Why are you using two timestamp fields instead of a range?

Short answer: performance. I did some tests with a relatively large
history data set using the two-field timestamp method, and a range
method before starting work on the "history" extension. These tests
seemed to indicate that using ranges significantly impacted performance
(both writes, and queries). It didn't look terribly difficult to convert
the code between the two systems so for the time being I pressed ahead
with the two-field method, but I'd love to find out if I was doing
something stupid with ranges. Further discussion definitely wanted!

Q. Why AFTER triggers instead of BEFORE?

Largely because on the original engine BEFORE triggers were limited in
functionality, so they had to be AFTER triggers. After reading some of
the discussion on the linked thread, this may be a decision I have to
re-visit.

Q. What about official SQL:2011 syntax? Application time, etc.?

Sure - all stuff I'd love to see in Postgres at some point, but not
stuff I'm qualified to even begin looking at (given it requires engine
alterations).


Anyway, that's probably enough for now. Questions, suggestions,
criticisms all gratefully received!

Dave.


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

Re: [HACKERS] inherit support for foreign tables

[Tom Lane]

Etsuro Fujita  writes:
> On 2015/04/16 16:05, Etsuro Fujita wrote:
>> I agree with you on this point.  However, ISTM there is a bug in
>> handling OIDs on foreign tables; while we now allow for ALTER SET
>> WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter
>> for foreign tables.  I think that since CREATE FOREIGN TABLE should be
>> consistent with ALTER FOREIGN TABLE, we should also allow the parameter
>> for foreign tables.  Attached is a patch for that.

> I also updated docs.  Attached is an updated version of the patch.

I believe that we intentionally did not do this, and here is why not:
existing pg_dump files assume that default_with_oids doesn't affect any
relation type except plain tables.  pg_backup_archiver.c only bothers
to change the GUC when about to dump a plain table, and otherwise leaves
it at its previous value.  That means if we apply a patch like this, it's
entirely possible that pg_dump/pg_restore will result in foreign tables
accidentally acquiring OID columns.

Since default_with_oids is really only meant as a backwards-compatibility
hack, I don't personally have a problem with restricting it to act only on
plain tables.  However, it might be a good idea to explicitly document
this interaction in a code comment to prevent anyone from re-inventing
this idea...  I'll go do that.

regards, tom lane


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

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Joel Jacobson]

+1


On Sat, Apr 25, 2015 at 10:23 PM, Pavel Stehule 
wrote:

> Hi
>
> 2015-04-24 19:16 GMT+02:00 Joel Jacobson :
>
>> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule 
>> wrote:
>> >> Example:
>> >>
>> >> context_messages = -warning, -error, +notice
>> >
>> >
>> > I prefer your first proposal - and there is a precedent for plpgsql -
>> > plpgsql_extra_checks
>> >
>> > It is clean for anybody. +-identifiers looks like horrible httpd
>> config. :)
>>
>> I have to agree on that :) Just thought this is the best we can do if
>> we want to reduce the number of GUCs to a minimum.
>>
>
> I played with some prototype and I am thinking so we need only one GUC
>
> plpgsql.display_context_messages = 'none'; -- compatible with current
> plpgsql.display_context_messages = 'all';
> plpgsql.display_context_messages = 'exception, log'; -- what I prefer
>
> I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement
>
> RAISE NOTICE WITH CONTEXT 'some message';
> RAISE NOTICE WITH CONTEXT USING message = 'some message';
> RAISE EXCEPTION WITHOUT CONTEXT 'other message';
>
> The patch is very small with full functionality (without documentation) -
> I am thinking so it can work. This patch is back compatible - and allow to
> change default behave simply.
>
> plpgsql.display_context_messages can be simplified to some like
> plpgsql.display_context_min_messages
>
> What do you think about it?
>
> Regards
>
> Pavel
>
>

Re: [HACKERS] forward vs backward slashes in msvc build code

[Michael Paquier]

On Sun, Apr 26, 2015 at 2:19 AM, Andrew Dunstan  wrote:
>
> On 04/25/2015 10:53 AM, Michael Paquier wrote:
>>
>> On Sat, Apr 25, 2015 at 9:58 PM, Peter Eisentraut wrote:
>>>
>>> On 4/24/15 12:22 AM, Michael Paquier wrote:

 Now that the stuff related to the move from contrib/ to src/bin/,
 modulescheck and tmp_install has been committed, shouldn't we give a
 new shot at this patch? Attached is a rebased version.
>>>
>>> done
>>
>> Thanks, bowerbird is running fine:
>>
>> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2015-04-25%2013%3A31%3A06
>
>
>
> But currawong is not - it runs an older version of the MS build tools. See
> :
>
>Bad format filename 'src/backend/access/brin/brin.c'
>  at src/tools/msvc/VCBuildProject.pm line 73.
> VCBuildProject::WriteFiles(VC2008Project=HASH(0x9a7274),
> "*Project::F") called at src/tools/msvc/Project.pm line 363
> Project::Save(VC2008Project=HASH(0x9a7274)) called at
> src/tools/msvc/Solution.pm line 539
> Solution::Save(VS2008Solution=HASH(0xc00b7c)) called at
> src/tools/msvc/Mkvcbuild.pm line 656
> Mkvcbuild::mkvcbuild(HASH(0xbed464)) called at build.pl line 36

Oops, attached is a patch that should make currawong happier..
-- 
Michael
diff --git a/src/tools/msvc/VCBuildProject.pm b/src/tools/msvc/VCBuildProject.pm
index 513cfb5..dda662f 100644
--- a/src/tools/msvc/VCBuildProject.pm
+++ b/src/tools/msvc/VCBuildProject.pm
@@ -71,7 +71,7 @@ EOF
 	foreach my $fileNameWithPath (sort keys %{ $self->{files} })
 	{
 		confess "Bad format filename '$fileNameWithPath'\n"
-		  unless ($fileNameWithPath =~ /^(.*)\\([^\\]+)\.(c|cpp|y|l|rc)$/);
+		  unless ($fileNameWithPath =~ m!^(.*)/([^/]+)\.(c|cpp|y|l|rc)$!);
 		my $dir  = $1;
 		my $file = $2;
 

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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

Robert Haas  writes:
> No, I just misread your email.  I thought you said you had attached
> the patch; rereading it, I see that you said you had applied the
> patch.  Silly me.

The real problem with this patch is it's wrong.  Specifically, it broke
the other case I mentioned in my original email:

regression=# create table src2 (f1 int, primary key(oid)) with oids;
ERROR:  column "oid" named in key does not exist
LINE 1: create table src2 (f1 int, primary key(oid)) with oids;
   ^

That works in 9.4, and was still working in HEAD as of my original email.
I think the patch's logic for attaching made-up OIDS options is actually
backwards (it's adding TRUE where it should add FALSE and vice versa),
but in any case I do not like the dependence on default_with_oids that
was introduced by the patch.  I am not sure there's any guarantee that
default_with_oids can't change between parsing and execution of a CREATE
TABLE command.

Apparently we need a few more regression tests in this area.  In the
meantime I suggest reverting and rethinking the patch.

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] Allow SQL/plpgsql functions to accept record

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 22, 2015 at 6:12 PM, Jim Nasby  wrote:
>> As for allowing SQL and plpgsql functions to accept a record, I think our
>> JSON functionality just provided plenty of reason we should allow accepting
>> them, even if you can't do much with it: you *can* hand it to row_to_json(),
>> which does allow you to do something useful with it. So it seems reasonable
>> to me that we should at least accept it as a function argument.

> I agree that that would be useful.  I think the problem with an
> expression like rowvar.something is that PL/pgsql cannot infer the
> type of the result, and nothing else works without that.  I doubt that
> it's practical to lift that restriction.

Well, we already support local variables of type RECORD in plpgsql, so
it's not immediately clear to me that function arguments would be much
worse.  There are a lot of deficiencies with the RECORD-local-variable
implementation: if you try to change the actual RECORD type from one call
to the next you'll probably have problems.  But it seems like we could
avoid that for function arguments by treating RECORD as a polymorphic
argument type, and thereby generating a separate set of plan trees for
each actual record type passed to the function within a given session.
So in principle it ought to work better than the local-variable case does
today.

In short I suspect that Jim is right and this has more to do with a
shortage of round tuits than any fundamental problem.

Not sure about the SQL-function case.  That might be even easier because
functions.c doesn't try to cache plans across queries; or maybe not.

regards, tom lane


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

Re: [HACKERS] PL/pgSQL, RAISE and error context

[Pavel Stehule]

Hi

2015-04-24 19:16 GMT+02:00 Joel Jacobson :

> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule 
> wrote:
> >> Example:
> >>
> >> context_messages = -warning, -error, +notice
> >
> >
> > I prefer your first proposal - and there is a precedent for plpgsql -
> > plpgsql_extra_checks
> >
> > It is clean for anybody. +-identifiers looks like horrible httpd config.
> :)
>
> I have to agree on that :) Just thought this is the best we can do if
> we want to reduce the number of GUCs to a minimum.
>

I played with some prototype and I am thinking so we need only one GUC

plpgsql.display_context_messages = 'none'; -- compatible with current
plpgsql.display_context_messages = 'all';
plpgsql.display_context_messages = 'exception, log'; -- what I prefer

I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

RAISE NOTICE WITH CONTEXT 'some message';
RAISE NOTICE WITH CONTEXT USING message = 'some message';
RAISE EXCEPTION WITHOUT CONTEXT 'other message';

The patch is very small with full functionality (without documentation) - I
am thinking so it can work. This patch is back compatible - and allow to
change default behave simply.

plpgsql.display_context_messages can be simplified to some like
plpgsql.display_context_min_messages

What do you think about it?

Regards

Pavel
commit cf9e23a29162ac55fcab1ac4d9e7a24492de0736
Author: Pavel Stehule 
Date:   Sat Apr 25 22:09:28 2015 +0200

initial implementation of (WITH|WITHOUT) CONTEXT clause to plpgsql RAISE statement.

initial implementation of plpgsql GUC plpgsql.display_context_messages

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index deefb1f..ea0dac5 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2921,6 +2921,7 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 	char	   *err_table = NULL;
 	char	   *err_schema = NULL;
 	ListCell   *lc;
+	bool			hide_ctx;
 
 	/* RAISE with no parameters: re-throw current exception */
 	if (stmt->condname == NULL && stmt->message == NULL &&
@@ -3080,10 +3081,42 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 			err_message = pstrdup(unpack_sql_state(err_code));
 	}
 
-	/*
-	 * Throw the error (may or may not come back)
-	 */
-	estate->err_text = raise_skip_msg;	/* suppress traceback of raise */
+	if (stmt->context_info == PLPGSQL_CONTEXT_DISPLAY)
+		hide_ctx = false;
+	else if (stmt->context_info == PLPGSQL_CONTEXT_SUPPRESS)
+	{
+		hide_ctx = true;
+	}
+	else
+	{
+		/* we display a messages via plpgsql_display_context_messages */
+		switch (stmt->elog_level)
+		{
+			case ERROR:
+hide_ctx = !(plpgsql_display_context_messages & PLPGSQL_DISPLAY_CONTEXT_ERROR);
+break;
+			case WARNING:
+hide_ctx = !(plpgsql_display_context_messages & PLPGSQL_DISPLAY_CONTEXT_WARNING);
+break;
+			case NOTICE:
+hide_ctx = !(plpgsql_display_context_messages & PLPGSQL_DISPLAY_CONTEXT_NOTICE);
+break;
+			case INFO:
+hide_ctx = !(plpgsql_display_context_messages & PLPGSQL_DISPLAY_CONTEXT_INFO);
+break;
+			case LOG:
+hide_ctx = !(plpgsql_display_context_messages & PLPGSQL_DISPLAY_CONTEXT_LOG);
+break;
+			case DEBUG1:
+hide_ctx = !(plpgsql_display_context_messages & PLPGSQL_DISPLAY_CONTEXT_DEBUG);
+break;
+			default:
+elog(ERROR, "unexpected RAISE statement level");
+		}
+	}
+
+	if (hide_ctx)
+		estate->err_text = raise_skip_msg;
 
 	ereport(stmt->elog_level,
 			(err_code ? errcode(err_code) : 0,
@@ -3099,7 +3132,8 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
 			 (err_table != NULL) ?
 			 err_generic_string(PG_DIAG_TABLE_NAME, err_table) : 0,
 			 (err_schema != NULL) ?
-			 err_generic_string(PG_DIAG_SCHEMA_NAME, err_schema) : 0));
+			 err_generic_string(PG_DIAG_SCHEMA_NAME, err_schema) : 0,
+			 errhidecontext(hide_ctx)));
 
 	estate->err_text = NULL;	/* un-suppress... */
 
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 4026e41..48914a7 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -259,6 +259,7 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token 	K_CONSTANT
 %token 	K_CONSTRAINT
 %token 	K_CONSTRAINT_NAME
+%token 	K_CONTEXT
 %token 	K_CONTINUE
 %token 	K_CURRENT
 %token 	K_CURSOR
@@ -341,6 +342,8 @@ static	void			check_raise_parameters(PLpgSQL_stmt_raise *stmt);
 %token 	K_WARNING
 %token 	K_WHEN
 %token 	K_WHILE
+%token 	K_WITH
+%token 	K_WITHOUT
 
 %%
 
@@ -1716,6 +1719,7 @@ stmt_raise		: K_RAISE
 		new->cmd_type	= PLPGSQL_STMT_RAISE;
 		new->lineno		= plpgsql_location_to_lineno(@1);
 		new->elog_level = ERROR;	/* default */
+		new->context_info = PLPGSQL_CONTEXT_DEFAULT;
 		new->condname	= NULL;
 		new->message	= NULL;
 		new->params		= NIL;
@@ -1773,6 +1777,21 @@ stmt_raise		: K_RAISE
 			if (tok == 0)
 yyerror("unexpected end of function definition");
 
+			/* Optional choose about including context */

Re: [HACKERS] Bug in planner

[Tom Lane]

David Rowley  writes:
> On 24 April 2015 at 21:43, Teodor Sigaev  wrote:
>> I faced with planner error:
>> ERROR:  could not find RelOptInfo for given relids

> I've done a little debugging on this too and I get the idea that in
> eqjoinsel() that min_righthand incorrectly does not have a bit set for "t3"

Yeah.  The short of it seems to be that initsplan.c is too optimistic
about whether antijoins can be reordered against outer joins in their RHS.
The discussion in optimizer/README says pretty clearly that they can't
(and eqjoinsel is relying on that, per the comment therein), so I think
this is basically brain fade in translating that logic to code.

regards, tom lane

diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index a7655e4..00b2625 100644
*** a/src/backend/optimizer/plan/initsplan.c
--- b/src/backend/optimizer/plan/initsplan.c
*** make_outerjoininfo(PlannerInfo *root,
*** 1165,1173 
  		 * For a lower OJ in our RHS, if our join condition does not use the
  		 * lower join's RHS and the lower OJ's join condition is strict, we
  		 * can interchange the ordering of the two OJs; otherwise we must add
! 		 * lower OJ's full syntactic relset to min_righthand.  Here, we must
! 		 * preserve ordering anyway if either the current join is a semijoin,
! 		 * or the lower OJ is either a semijoin or an antijoin.
  		 *
  		 * Here, we have to consider that "our join condition" includes any
  		 * clauses that syntactically appeared above the lower OJ and below
--- 1165,1173 
  		 * For a lower OJ in our RHS, if our join condition does not use the
  		 * lower join's RHS and the lower OJ's join condition is strict, we
  		 * can interchange the ordering of the two OJs; otherwise we must add
! 		 * the lower OJ's full syntactic relset to min_righthand.  Also, we
! 		 * must preserve ordering anyway if either the current join or the
! 		 * lower OJ is either a semijoin or an antijoin.
  		 *
  		 * Here, we have to consider that "our join condition" includes any
  		 * clauses that syntactically appeared above the lower OJ and below
*** make_outerjoininfo(PlannerInfo *root,
*** 1184,1189 
--- 1184,1190 
  		{
  			if (bms_overlap(clause_relids, otherinfo->syn_righthand) ||
  jointype == JOIN_SEMI ||
+ jointype == JOIN_ANTI ||
  otherinfo->jointype == JOIN_SEMI ||
  otherinfo->jointype == JOIN_ANTI ||
  !otherinfo->lhs_strict || otherinfo->delay_upper_joins)
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index e4f3f22..ed9ad0e 100644
*** a/src/test/regress/expected/join.out
--- b/src/test/regress/expected/join.out
*** WHERE d.f1 IS NULL;
*** 2284,2289 
--- 2284,2331 
  (3 rows)
  
  --
+ -- regression test for proper handling of outer joins within antijoins
+ --
+ create temp table tt4x(c1 int, c2 int, c3 int);
+ explain (costs off)
+ select * from tt4x t1
+ where not exists (
+   select 1 from tt4x t2
+ left join tt4x t3 on t2.c3 = t3.c1
+ left join ( select t5.c1 as c1
+ from tt4x t4 left join tt4x t5 on t4.c2 = t5.c1
+   ) a1 on t3.c2 = a1.c1
+   where t1.c1 = t2.c2
+ );
+QUERY PLAN
+ -
+  Hash Anti Join
+Hash Cond: (t1.c1 = t2.c2)
+->  Seq Scan on tt4x t1
+->  Hash
+  ->  Merge Right Join
+Merge Cond: (t5.c1 = t3.c2)
+->  Merge Join
+  Merge Cond: (t4.c2 = t5.c1)
+  ->  Sort
+Sort Key: t4.c2
+->  Seq Scan on tt4x t4
+  ->  Sort
+Sort Key: t5.c1
+->  Seq Scan on tt4x t5
+->  Sort
+  Sort Key: t3.c2
+  ->  Merge Left Join
+Merge Cond: (t2.c3 = t3.c1)
+->  Sort
+  Sort Key: t2.c3
+  ->  Seq Scan on tt4x t2
+->  Sort
+  Sort Key: t3.c1
+  ->  Seq Scan on tt4x t3
+ (24 rows)
+ 
+ --
  -- regression test for problems of the sort depicted in bug #3494
  --
  create temp table tt5(f1 int, f2 int);
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index d0cf0a0..5b65ea8 100644
*** a/src/test/regress/sql/join.sql
--- b/src/test/regress/sql/join.sql
*** LEFT JOIN (
*** 448,453 
--- 448,470 
  WHERE d.f1 IS NULL;
  
  --
+ -- regression test for proper handling of outer joins within antijoins
+ --
+ 
+ create temp table tt4x(c1 int, c2 int, c3 int);
+ 
+ explain (costs off)
+ select * from tt4x t1
+ where not exists (
+   select 1 from tt4x t2
+ left join tt4x t3 on t2.

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Peter Geoghegan]

On Sat, Apr 25, 2015 at 12:35 PM, Peter Geoghegan  wrote:
>>> > That
>>> > it has 'morphing' characteristics imo just makes it worse, rather than
>>> > better. Besides being confusing that it has different meanings, it's far
>>> > from inconceivable that somebody wants to return values from the
>>> > preexisting, new, and merged rows.
>>>
>>> This is how RETURNING works from UPDATEs in general.
>>
>> And there's been a patch (which unfortunately died because it's
>> implementation wasn't good), to allow referring to the other versions of
>> the tuple. It has been wished for numerous times.
>
> Well, if that patch is ever committed, then it won't be hard to get
> the behavior here too, since it is literally exactly the same code. I
> don't change anything about it, and that seems to be your problem.


I withdraw this remark. Even in a world where this patch is committed,
it still makes sense for the INSERT returning behavior to not be
altered (and to project only TARGET tuples even if they come from the
auxiliary UPDATE). The "join" is within the auxiliary UPDATE, not the
INSERT, and it should be no more possible to project intermediate
tuples (like EXCLUDED.*) from the INSERT's RETURNING than it is to
project CTE scan tuples from an INSERT ... RETURNING with a CTE.

-- 
Peter Geoghegan


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Peter Geoghegan]

On Sat, Apr 25, 2015 at 12:23 PM, Andres Freund  wrote:
> 95% of all users will know NEW/OLD from triggers, not rules. Where NEW
> is used in a quite comparable way.

I don't think it's comparable.

>> >> Seems pretty descriptive of the situation to me - I actually put a lot
>> >> of thought into this. Additionally, the word is widely understood by
>> >> non-native speakers. TARGET is also very descriptive, because it
>> >> situationally describes either the existing tuple actually present in
>> >> the table, or (from a RETURNING clause) the final tuple present in the
>> >> table post-UPDATE. We use the term "target" for that pervasively (in
>> >> the docs and in the code).
>> >
>> > Sorry, I don't buy either argument. EXISTING and NEW would surely at
>> > least as widely understood than EXCLUDE and TARGET. The latter does just
>> > about no sense to me; especially from a user POV. I don't think the
>> > existing usage of the term has much to do what it's used for here.
>>
>> Yes it does. The UPDATE docs refer to the target table in a way
>> intended to distinguish it from any joined-to table (FROM table). It's
>> clear as day.
>
> Which means the term is used in a different way for INSERTs and UPDATEs
> already.

No, it isn't. It both cases the table is the one involved in the parse
analysis setTargetTable() call.

>> Maybe EXISTING is equally well understood as a word in general, but
>> it's way more ambiguous than EXCLUDED is here.
>
> What? I'm not suggesting to replace EXCLUDED by EXISTING - that'd make
> absolutely no sense. My suggesting is to have NEW refer to the tuple
> specified in the INSERT and EXISTING to the, well, pre existing tuple
> that the conflict is with.

Okay, but that doesn't change my opinion.

>> > That
>> > it has 'morphing' characteristics imo just makes it worse, rather than
>> > better. Besides being confusing that it has different meanings, it's far
>> > from inconceivable that somebody wants to return values from the
>> > preexisting, new, and merged rows.
>>
>> This is how RETURNING works from UPDATEs in general.
>
> And there's been a patch (which unfortunately died because it's
> implementation wasn't good), to allow referring to the other versions of
> the tuple. It has been wished for numerous times.

Well, if that patch is ever committed, then it won't be hard to get
the behavior here too, since it is literally exactly the same code. I
don't change anything about it, and that seems to be your problem.

> the docs say:
>Since
>RETURNING is not part of the UPDATE
>auxiliary query, the special ON CONFLICT UPDATE aliases
>(TARGET and EXCLUDED) may not be
>referenced;  only the row as it exists after updating (or
>inserting) is returned.
>
> So I don't understand that whole chain of argument. There's no such
> morphing behaviour, unless I miss something?

That's a documentation bug (a remnant of an earlier version). Sorry
about that. You can reference TARGET from returning. It's directly
contradicted by this much earlier statement on the INSERT doc page:

"Both aliases can be used in the auxiliary query targetlist and WHERE
clause, while the TARGET alias can be used anywhere within the entire
statement (e.g., within the RETURNING clause)"

I'll go fix that.

> 2a5d80b27d2c5832ad26dde4651c64dd2004f401:
>> The problem with this seems to be that it more or less
>> necessitates making both IGNORE and UPDATE fully reserved keywords in
>> order to avoid an ambiguity, which we prefer not to do
>
> It does not. As mentioned in the thread DO UPDATE/NOTHING work without
> anything like that.

I just mean that it couldn't work as-was in the repo at that time.
This commit message was written before your proposal of 8 hours ago.

We're going to have to agree to disagree here. I've given you my
opinion. I'm burnt out on this patch, and whatever the path of least
resistance is is the path I'll take. Frankly, the only reason that I'm
putting up any kind of argument is because I don't think that your
proposal is the path of least resistance.

-- 
Peter Geoghegan


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Andres Freund]

On 2015-04-25 11:50:59 -0700, Peter Geoghegan wrote:
> On Sat, Apr 25, 2015 at 11:24 AM, Andres Freund  wrote:
> >> > c) Right now the UPDATE can refer to pseudo relations 'TARGET' and
> >> >'EXCLUDED'. I think especially the latter doesn't fit anymore at
> >> >all. How about 'CONFLICTING' and 'EXISTING'? Or even NEW and OLD?
> >>
> >> NEW and OLD are terribly misleading, since surely the NEW tuple is the
> >> one actually appended to the relation by the UPDATE, and the OLD one
> >> is the existing one (not the excluded one). Plus they have all that
> >> intellectual baggage from rules.
> >
> > What 'intellectual baggage' would that be? That they're already known to
> > have been used in another place? I don't see the problem.
> 
> The problem is that they make you think of rules, and they don't
> describe what's going on at all.

95% of all users will know NEW/OLD from triggers, not rules. Where NEW
is used in a quite comparable way.

> >> Seems pretty descriptive of the situation to me - I actually put a lot
> >> of thought into this. Additionally, the word is widely understood by
> >> non-native speakers. TARGET is also very descriptive, because it
> >> situationally describes either the existing tuple actually present in
> >> the table, or (from a RETURNING clause) the final tuple present in the
> >> table post-UPDATE. We use the term "target" for that pervasively (in
> >> the docs and in the code).
> >
> > Sorry, I don't buy either argument. EXISTING and NEW would surely at
> > least as widely understood than EXCLUDE and TARGET. The latter does just
> > about no sense to me; especially from a user POV. I don't think the
> > existing usage of the term has much to do what it's used for here.
> 
> Yes it does. The UPDATE docs refer to the target table in a way
> intended to distinguish it from any joined-to table (FROM table). It's
> clear as day.

Which means the term is used in a different way for INSERTs and UPDATEs
already.  To me it sounds like it's a remnant of your earlier syntax
proposal for UPSERT.

> Maybe EXISTING is equally well understood as a word in general, but
> it's way more ambiguous than EXCLUDED is here.

What? I'm not suggesting to replace EXCLUDED by EXISTING - that'd make
absolutely no sense. My suggesting is to have NEW refer to the tuple
specified in the INSERT and EXISTING to the, well, pre existing tuple
that the conflict is with.

> > That
> > it has 'morphing' characteristics imo just makes it worse, rather than
> > better. Besides being confusing that it has different meanings, it's far
> > from inconceivable that somebody wants to return values from the
> > preexisting, new, and merged rows.
> 
> This is how RETURNING works from UPDATEs in general.

And there's been a patch (which unfortunately died because it's
implementation wasn't good), to allow referring to the other versions of
the tuple. It has been wished for numerous times.


> IOW, if you do an UPDATE FROM (which is pretty similar to ON CONFLICT
> UPDATE, syntax-wise), then you can only refer to the joined table's
> tuple and the final post-update tuple from within RETURNING.

> You cannot refer to the pre-UPDATE target tuple there either -- it's
> *exactly* the same situation. Why should it be any different here? The
> situational/morphing characteristic of the alias name TARGET is
> therefore absolutely appropriate, in that it follows UPDATE.

Contrasting
> TARGET is also very descriptive, because it
> situationally describes either the existing tuple actually present in
> the table, or (from a RETURNING clause) the final tuple present in the
> table post-UPDATE. We use the term "target" for that pervasively (in
> the docs and in the code).

the docs say:
   Since
   RETURNING is not part of the UPDATE
   auxiliary query, the special ON CONFLICT UPDATE aliases
   (TARGET and EXCLUDED) may not be
   referenced;  only the row as it exists after updating (or
   inserting) is returned.

So I don't understand that whole chain of argument. There's no such
morphing behaviour, unless I miss something?

2a5d80b27d2c5832ad26dde4651c64dd2004f401:
> The problem with this seems to be that it more or less
> necessitates making both IGNORE and UPDATE fully reserved keywords in
> order to avoid an ambiguity, which we prefer not to do

It does not. As mentioned in the thread DO UPDATE/NOTHING work without
anything like that.

Greetings,

Andres Freund


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Peter Geoghegan]

On Sat, Apr 25, 2015 at 11:50 AM, Peter Geoghegan  wrote:
> To be fair, there is one unrelated slight difference with RETURNING
> and conventional UPDATEs: You cannot return the EXCLUDED tuple (in the
> same way that you can reference the joined-FROM tuple within
> conventional UPDATEs). This is because the pertinent information is
> likely to be in the target tuple (after all, the DML statement names
> the proposed-for-insertion tuples itself, directly), but more
> importantly because projecting both would necessitate *always*
> qualifying the RETURNING column names to resolve which tuple is
> intended (UPDATE FROM will seldom be a self-join, but this will always
> be like a self-join).


It also makes sense because this is the RETURNING clause of an INSERT,
not an UPDATE. So the general INSERT behavior is what is expected. It
ought to be irrelevant if tuples were projected by actually inserting
or updating. Otherwise, your UPSERT is probably misconceived.

-- 
Peter Geoghegan


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Peter Geoghegan]

On Sat, Apr 25, 2015 at 11:24 AM, Andres Freund  wrote:
>> > b) unclear whether the WHERE belongs to colb or the whole index
>> >expression. The equivalent for aggregates, which I bet is going to be
>> >used less often, caused a fair amount of confusing.
>>
>> I don't see those two situations as being comparable. The inference
>> specification does not accept aggregates.
>
> Huh? It's pretty much entirely besides the point that inference doesn't
> accept aggregates. The point is that ORDER BY for aggregates has
> confused users because it's inside the parens.

Would any alternative cause less confusion? That's the real issue. And
I'm unconvinced that your alternative would.

>> > a) Why is is 'CONFLICT"? We're talking about a uniquness violation. What
>> >if we, at some later point, also want to handle other kind of
>> >violations? Shouldn't it be ON UNIQUE CONFLICT/ERROR/VIOLATION ...
>>
>> I think that naming unique violations alone would be wrong (not to
>> mention ludicrously verbose).
>
> Why?

Because, as I said, it might not be a unique violation at all. It
could be an exclusion violation.

>> > b) For me there's a WITH before the index inference clause missing, to
>> >have it read in 'SQL' style.
>>
>> I'm not seeing it. BTW, Robert was the one who initially proposed that
>> the unique index inference clause follow this exact style (albeit
>> before it accepted a WHERE clause to infer partial indexes, which was
>> only added a couple of months ago).
>
> So?

So, his opinion matters if it comes down to a vote. The inference
specification syntax as implemented is exactly what he suggested (plus
I've added a predicate).

> I guess I can live with that uglyness; but I'd like somebody else to
> chime in.

Agreed.

>> > c) Right now the UPDATE can refer to pseudo relations 'TARGET' and
>> >'EXCLUDED'. I think especially the latter doesn't fit anymore at
>> >all. How about 'CONFLICTING' and 'EXISTING'? Or even NEW and OLD?
>>
>> NEW and OLD are terribly misleading, since surely the NEW tuple is the
>> one actually appended to the relation by the UPDATE, and the OLD one
>> is the existing one (not the excluded one). Plus they have all that
>> intellectual baggage from rules.
>
> What 'intellectual baggage' would that be? That they're already known to
> have been used in another place? I don't see the problem.

The problem is that they make you think of rules, and they don't
describe what's going on at all.

>> Seems pretty descriptive of the situation to me - I actually put a lot
>> of thought into this. Additionally, the word is widely understood by
>> non-native speakers. TARGET is also very descriptive, because it
>> situationally describes either the existing tuple actually present in
>> the table, or (from a RETURNING clause) the final tuple present in the
>> table post-UPDATE. We use the term "target" for that pervasively (in
>> the docs and in the code).
>
> Sorry, I don't buy either argument. EXISTING and NEW would surely at
> least as widely understood than EXCLUDE and TARGET. The latter does just
> about no sense to me; especially from a user POV. I don't think the
> existing usage of the term has much to do what it's used for here.

Yes it does. The UPDATE docs refer to the target table in a way
intended to distinguish it from any joined-to table (FROM table). It's
clear as day. Maybe EXISTING is equally well understood as a word in
general, but it's way more ambiguous than EXCLUDED is here.

> That
> it has 'morphing' characteristics imo just makes it worse, rather than
> better. Besides being confusing that it has different meanings, it's far
> from inconceivable that somebody wants to return values from the
> preexisting, new, and merged rows.

This is how RETURNING works from UPDATEs in general. IOW, if you do an
UPDATE FROM (which is pretty similar to ON CONFLICT UPDATE,
syntax-wise), then you can only refer to the joined table's tuple and
the final post-update tuple from within RETURNING. You cannot refer to
the pre-UPDATE target tuple there either -- it's *exactly* the same
situation. Why should it be any different here? The
situational/morphing characteristic of the alias name TARGET is
therefore absolutely appropriate, in that it follows UPDATE.

To be fair, there is one unrelated slight difference with RETURNING
and conventional UPDATEs: You cannot return the EXCLUDED tuple (in the
same way that you can reference the joined-FROM tuple within
conventional UPDATEs). This is because the pertinent information is
likely to be in the target tuple (after all, the DML statement names
the proposed-for-insertion tuples itself, directly), but more
importantly because projecting both would necessitate *always*
qualifying the RETURNING column names to resolve which tuple is
intended (UPDATE FROM will seldom be a self-join, but this will always
be like a self-join).

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make chang

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Andres Freund]

On 2015-04-25 11:05:49 -0700, Peter Geoghegan wrote:
> On Sat, Apr 25, 2015 at 2:01 AM, Andres Freund  wrote:
> > My problem with the WHERE being inside the parens in the above is that
> > it's
> > a) different from CREATE INDEX
> 
> I don't think that that's an important goal.

Given that it's used to 'match' to indexes, I can't agree.

> > b) unclear whether the WHERE belongs to colb or the whole index
> >expression. The equivalent for aggregates, which I bet is going to be
> >used less often, caused a fair amount of confusing.
> 
> I don't see those two situations as being comparable. The inference
> specification does not accept aggregates.

Huh? It's pretty much entirely besides the point that inference doesn't
accept aggregates. The point is that ORDER BY for aggregates has
confused users because it's inside the parens.

> > a) Why is is 'CONFLICT"? We're talking about a uniquness violation. What
> >if we, at some later point, also want to handle other kind of
> >violations? Shouldn't it be ON UNIQUE CONFLICT/ERROR/VIOLATION ...
> 
> I think that naming unique violations alone would be wrong (not to
> mention ludicrously verbose).

Why?

> The syntax has been like this for some time, and
> hasn't been a point of contention for a long time, so I thought this
> was settled.

I really don't care if it's been that for a long while. This is a not
yet commited feature.

> > b) For me there's a WITH before the index inference clause missing, to
> >have it read in 'SQL' style.
> 
> I'm not seeing it. BTW, Robert was the one who initially proposed that
> the unique index inference clause follow this exact style (albeit
> before it accepted a WHERE clause to infer partial indexes, which was
> only added a couple of months ago).

So?

I guess I can live with that uglyness; but I'd like somebody else to
chime in.

> > c) Right now the UPDATE can refer to pseudo relations 'TARGET' and
> >'EXCLUDED'. I think especially the latter doesn't fit anymore at
> >all. How about 'CONFLICTING' and 'EXISTING'? Or even NEW and OLD?
> 
> NEW and OLD are terribly misleading, since surely the NEW tuple is the
> one actually appended to the relation by the UPDATE, and the OLD one
> is the existing one (not the excluded one). Plus they have all that
> intellectual baggage from rules.

What 'intellectual baggage' would that be? That they're already known to
have been used in another place? I don't see the problem.

How about EXISTING and NEW?

> """
> verb (used with object), excluded, excluding.
> 1.
> to shut or keep out; prevent the entrance of.
> 2.
> to shut out from consideration, privilege, etc.:
> Employees and their relatives were excluded from participation in the contest.
> 3.
> to expel and keep out; thrust out; eject:
> He was excluded from the club for infractions of the rules.
> """
> 
> Seems pretty descriptive of the situation to me - I actually put a lot
> of thought into this. Additionally, the word is widely understood by
> non-native speakers. TARGET is also very descriptive, because it
> situationally describes either the existing tuple actually present in
> the table, or (from a RETURNING clause) the final tuple present in the
> table post-UPDATE. We use the term "target" for that pervasively (in
> the docs and in the code).

Sorry, I don't buy either argument. EXISTING and NEW would surely at
least as widely understood than EXCLUDE and TARGET. The latter does just
about no sense to me; especially from a user POV. I don't think the
existing usage of the term has much to do what it's used for here. That
it has 'morphing' characteristics imo just makes it worse, rather than
better. Besides being confusing that it has different meanings, it's far
from inconceivable that somebody wants to return values from the
preexisting, new, and merged rows.

Greetings,

Andres Freund


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

Re: [HACKERS] Fwd: [GENERAL] 4B row limit for CLOB tables

[Bruce Momjian]

On Fri, Apr 24, 2015 at 11:39:04PM -0500, Jim Nasby wrote:
> On 4/24/15 7:11 PM, Álvaro Hernández Tortosa wrote:
> >On 24/04/15 05:24, Tom Lane wrote:
> ...
> >>TBH, I've got very little enthusiasm for fixing this given the number
> >>of reports of trouble from the field, which so far as I recall is zero.
> >>Álvaro's case came up through intentionally trying to create an
> >>unreasonable number of tables, not from real usage.  This thread likewise
> >>appears to contain lots of speculation and no reports of anyone hitting
> >>a problem in practice.
> >
> > It is certainly true that this was a very synthetic case. I
> >envision, however, certain use cases where we may hit a very large
> >number of tables:
> 
> The original case has NOTHING to do with the number of tables and
> everything to do with the number of toasted values a table can have.
> If you have to toast 4B attributes in a single relation it will
> fail. In reality, if you get anywhere close to that things will fall
> apart due to OID conflicts.
> 
> This case isn't nearly as insane as 4B tables. A table storing 10
> text fields each of which is 2K would hit this limit with only 400M
> rows. If my math is right that's only 8TB; certainly not anything
> insane space-wise or rowcount-wise.
> 
> Perhaps it's still not fixing, but I think it's definitely worth
> documenting.

And it is now documented in the Postgres FAQ thanks to 'Rogerdpack',
which is where that "maximum" table came from:


https://wiki.postgresql.org/wiki/FAQ#What_is_the_maximum_size_for_a_row.2C_a_table.2C_and_a_database.3F

Note if you are storing a table with rows that exceed 2KB in size
(aggregate size of each row) then the "Maximum number of rows in a
table" may be limited to 4 Billion, see TOAST. 

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

  + Everyone has their own god. +


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

Re: [HACKERS] INSERT ... ON CONFLICT syntax issues

[Peter Geoghegan]

On Sat, Apr 25, 2015 at 2:01 AM, Andres Freund  wrote:
> My problem with the WHERE being inside the parens in the above is that
> it's
> a) different from CREATE INDEX

I don't think that that's an important goal.

> b) unclear whether the WHERE belongs to colb or the whole index
>expression. The equivalent for aggregates, which I bet is going to be
>used less often, caused a fair amount of confusing.

I don't see those two situations as being comparable. The inference
specification does not accept aggregates. It seems obvious to me that
the predicate only ever applies to the entire table. And it's obvious
that it's part of the inference specification because it appears in
parentheses with everything else - otherwise, *users* might find this
phantom WHERE clause ambiguous/confusing.

> But I'm generally having some doubts about the syntax.
>
> Right now it's
> INSERT ... ON CONFLICT opt_on_conf_clause UPDATE|IGNORE.
>
> A couple things:
>
> a) Why is is 'CONFLICT"? We're talking about a uniquness violation. What
>if we, at some later point, also want to handle other kind of
>violations? Shouldn't it be ON UNIQUE CONFLICT/ERROR/VIOLATION ...

I think that naming unique violations alone would be wrong (not to
mention ludicrously verbose). Heikki and I both feel that the CONFLICT
keyword captures the fact that this could be a dup violation, or an
exclusion violation. The syntax has been like this for some time, and
hasn't been a point of contention for a long time, so I thought this
was settled. Note that the syntax is quite similar to the SQLite
syntax of the same feature, that has ON CONFLICT IGNORE (it also has
ON CONFLICT REPLACE, but not ON CONFLICT UPDATE).

> b) For me there's a WITH before the index inference clause missing, to
>have it read in 'SQL' style.

I'm not seeing it. BTW, Robert was the one who initially proposed that
the unique index inference clause follow this exact style (albeit
before it accepted a WHERE clause to infer partial indexes, which was
only added a couple of months ago).

> c) Right now the UPDATE can refer to pseudo relations 'TARGET' and
>'EXCLUDED'. I think especially the latter doesn't fit anymore at
>all. How about 'CONFLICTING' and 'EXISTING'? Or even NEW and OLD?

NEW and OLD are terribly misleading, since surely the NEW tuple is the
one actually appended to the relation by the UPDATE, and the OLD one
is the existing one (not the excluded one). Plus they have all that
intellectual baggage from rules.

CONFLICTING, as Greg Stark pointed out many months ago, is something
that applies to both tuples that can be referenced, which is why I
*stopped* using it months ago. They conflict with *each other*. Any
conflict must pertain to both.

Dictionary.com defines "exclude" as:

"""
verb (used with object), excluded, excluding.
1.
to shut or keep out; prevent the entrance of.
2.
to shut out from consideration, privilege, etc.:
Employees and their relatives were excluded from participation in the contest.
3.
to expel and keep out; thrust out; eject:
He was excluded from the club for infractions of the rules.
"""

Seems pretty descriptive of the situation to me - I actually put a lot
of thought into this. Additionally, the word is widely understood by
non-native speakers. TARGET is also very descriptive, because it
situationally describes either the existing tuple actually present in
the table, or (from a RETURNING clause) the final tuple present in the
table post-UPDATE. We use the term "target" for that pervasively (in
the docs and in the code).

> So I guess it boils down to that I think we should switch the syntax to
> be:
>
> INSERT ... ON UNIQUE VIOLATION [WITH (cola, colb) WHERE ...] DO 
> {NOTHING|UPDATE}

Beauty is in the eye of the beholder and all, but that seems pretty
ugly to me. Honestly, I think we should just accept that the predicate
appears in the parentheses on the odd occasion that it appears at all
- partial unique indexes are not all that common.

-- 
Peter Geoghegan


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

Re: [HACKERS] forward vs backward slashes in msvc build code

[Andrew Dunstan]

On 04/25/2015 10:53 AM, Michael Paquier wrote:

On Sat, Apr 25, 2015 at 9:58 PM, Peter Eisentraut wrote:

On 4/24/15 12:22 AM, Michael Paquier wrote:

Now that the stuff related to the move from contrib/ to src/bin/,
modulescheck and tmp_install has been committed, shouldn't we give a
new shot at this patch? Attached is a rebased version.

done

Thanks, bowerbird is running fine:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2015-04-25%2013%3A31%3A06



But currawong is not - it runs an older version of the MS build tools. 
See 
:


   Bad format filename 'src/backend/access/brin/brin.c'
 at src/tools/msvc/VCBuildProject.pm line 73.
VCBuildProject::WriteFiles(VC2008Project=HASH(0x9a7274), "*Project::F") 
called at src/tools/msvc/Project.pm line 363
Project::Save(VC2008Project=HASH(0x9a7274)) called at 
src/tools/msvc/Solution.pm line 539
Solution::Save(VS2008Solution=HASH(0xc00b7c)) called at 
src/tools/msvc/Mkvcbuild.pm line 656
Mkvcbuild::mkvcbuild(HASH(0xbed464)) called at build.pl line 36

cheers

andrew



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

Re: [HACKERS] pg_dump: largeobject behavior issues (possible bug)

[Andrew Dunstan]

On 04/25/2015 12:32 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 04/24/2015 06:41 PM, Tom Lane wrote:

Yeah, this was brought up when we added per-large-object metadata; it was
obvious that that patch would cause pg_dump to choke on large numbers of
large objects.  The (perhaps rather lame) argument was that you wouldn't
have that many of them.
Given that large objects don't have any individual dependencies,
one could envision fixing this by replacing the individual large-object
DumpableObjects by a single placeholder to participate in the sort phase,
and then when it's time to dump that, scan the large objects using a
cursor and create/print/delete the information separately for each one.
This would likely involve some rather painful refactoring in pg_dump
however.

I think we need to think about this some more, TBH, I'm not convinced
that the changes made back in 9.0 were well conceived. Having separate
TOC entries for each LO seems wrong in principle, although I understand
why it was done.

Perhaps.  One advantage of doing it this way is that you can get
pg_restore to extract a single LO from an archive file; though it's
debatable whether that's worth the potential resource-consumption hazards.



In my view it isn't worth it.



Another issue is that restore options such as --no-owner and
--no-privileges would not work for LOs (at least not without messy hacks)
if we go back to a scheme where all the LO information is just SQL
commands inside a single TOC object.

After further thought I realized that if we simply hack pg_dump to emit
the LOs in a streaming fashion, but keep the archive-file representation
the same as it is now, then we haven't really fixed the problem because
pg_restore is still likely to choke when it tries to read the archive's
TOC.  So my proposal above isn't enough either.



Yep, that's certainly true.



Perhaps what we need is some sort of "second-level TOC" which is only ever
processed in a streaming fashion, by both pg_dump and pg_restore.  This
would not support dependency resolution or re-ordering, but we don't need
those abilities for LOs.





+1, I had a similar thought, half-formed, but you've expressed it better 
than I could have.


cheers

andrew


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

Re: [HACKERS] pg_dump: largeobject behavior issues (possible bug)

[Tom Lane]

Andrew Dunstan  writes:
> On 04/24/2015 06:41 PM, Tom Lane wrote:
>> Yeah, this was brought up when we added per-large-object metadata; it was
>> obvious that that patch would cause pg_dump to choke on large numbers of
>> large objects.  The (perhaps rather lame) argument was that you wouldn't
>> have that many of them.

>> Given that large objects don't have any individual dependencies,
>> one could envision fixing this by replacing the individual large-object
>> DumpableObjects by a single placeholder to participate in the sort phase,
>> and then when it's time to dump that, scan the large objects using a
>> cursor and create/print/delete the information separately for each one.
>> This would likely involve some rather painful refactoring in pg_dump
>> however.

> I think we need to think about this some more, TBH, I'm not convinced 
> that the changes made back in 9.0 were well conceived. Having separate 
> TOC entries for each LO seems wrong in principle, although I understand 
> why it was done.

Perhaps.  One advantage of doing it this way is that you can get
pg_restore to extract a single LO from an archive file; though it's
debatable whether that's worth the potential resource-consumption hazards.
Another issue is that restore options such as --no-owner and
--no-privileges would not work for LOs (at least not without messy hacks)
if we go back to a scheme where all the LO information is just SQL
commands inside a single TOC object.

After further thought I realized that if we simply hack pg_dump to emit
the LOs in a streaming fashion, but keep the archive-file representation
the same as it is now, then we haven't really fixed the problem because
pg_restore is still likely to choke when it tries to read the archive's
TOC.  So my proposal above isn't enough either.

Perhaps what we need is some sort of "second-level TOC" which is only ever
processed in a streaming fashion, by both pg_dump and pg_restore.  This
would not support dependency resolution or re-ordering, but we don't need
those abilities for LOs.

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] pg_dump: largeobject behavior issues (possible bug)

[Andrew Dunstan]

On 04/24/2015 06:41 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 04/23/2015 04:04 PM, Andrew Gierth wrote:

The relevant code is getBlobs in pg_dump.c, which queries the whole of
pg_largeobject_metadata without using a cursor (so the PGresult is
already huge thanks to having >100 million rows), and then mallocs a
BlobInfo array and populates it from the PGresult, also using pg_strdup
for the oid string, owner name, and ACL if any.

I'm surprised this hasn't come up before. I have a client that I
persuaded to convert all their LOs to bytea fields because of problems
with pg_dump handling millions of LOs, and kept them on an older
postgres version until they made that change.

Yeah, this was brought up when we added per-large-object metadata; it was
obvious that that patch would cause pg_dump to choke on large numbers of
large objects.  The (perhaps rather lame) argument was that you wouldn't
have that many of them.

Given that large objects don't have any individual dependencies,
one could envision fixing this by replacing the individual large-object
DumpableObjects by a single placeholder to participate in the sort phase,
and then when it's time to dump that, scan the large objects using a
cursor and create/print/delete the information separately for each one.
This would likely involve some rather painful refactoring in pg_dump
however.



I think we need to think about this some more, TBH, I'm not convinced 
that the changes made back in 9.0 were well conceived. Having separate 
TOC entries for each LO seems wrong in principle, although I understand 
why it was done. For now, my advice would be to avoid use of 
pg_dump/pg_restore if you have large numbers of LOs. The good news is 
that these days there are alternative methods of doing backup / restore, 
albeit not 100% equivalent with pg_dump / pg_restore.


One useful thing might be to provide pg_dump with 
--no-blobs/--blobs-only switches so you could at least easily segregate 
the blobs into their own dump file. That would be in addition to dealing 
with the memory problems pg_dump has with millions of LOs, of course.



cheers

andrew


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

Re: [HACKERS] forward vs backward slashes in msvc build code

[Michael Paquier]

On Sat, Apr 25, 2015 at 9:58 PM, Peter Eisentraut wrote:
> On 4/24/15 12:22 AM, Michael Paquier wrote:
>> Now that the stuff related to the move from contrib/ to src/bin/,
>> modulescheck and tmp_install has been committed, shouldn't we give a
>> new shot at this patch? Attached is a rebased version.
>
> done

Thanks, bowerbird is running fine:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2015-04-25%2013%3A31%3A06
-- 
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] Ignoring some binaries generated in src/test

[Michael Paquier]

On Sat, Apr 25, 2015 at 6:14 AM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Tom Lane wrote:
>>> The current logic in src/test/Makefile, particularly the way that
>>> the modules subdirectory is handled, seems pretty ugly/convoluted
>>> anyway.  I wonder why it was done that way rather than just ensuring
>>> that modules/ doesn't do anything for "make install"?
>
>> Because we do want to have the Makefile in src/test/modules to install
>> the modules if "make install" is invoked there.  That way, you can run
>> "make -C src/test/modules install installcheck", and it works.
>
> OK.  I still wonder if there isn't a better way to get that effect, but
> I left it alone for now.  I committed Michael's new .gitignore files and
> fixed the Makefiles so that "make clean" and friends clean up properly.

Thanks.
-- 
Michael


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

Re: [HACKERS] collate.linux.utf8 test coverage

[Michael Paquier]

On Sat, Apr 25, 2015 at 4:51 AM, Andrew Dunstan  wrote:
> The optional buildfarm module that runs this test was broken by commit
> dcae5faccab64776376d354decda0017c648bb53
>
> Since nobody has responded to my complaint about this, I have disabled it on
> crake, the only buildfarm machine that has actually been running the test,
> so we now have no buildfarm coverage for it.

Commit dcae5fac has switched in pg_regress --temp-install to
--temp-instance, --psqldir to --bindir, and has removed --top-builddir
and --extra-install. After looking at TestCollateLinuxUTF8.pm, I think
that you need to use the one-liner attached.
Regards,
-- 
Michael
diff --git a/PGBuild/Modules/TestCollateLinuxUTF8.pm b/PGBuild/Modules/TestCollateLinuxUTF8.pm
index 465a1cd..19333f1 100644
--- a/PGBuild/Modules/TestCollateLinuxUTF8.pm
+++ b/PGBuild/Modules/TestCollateLinuxUTF8.pm
@@ -82,7 +82,7 @@ sub installcheck
 my $logpos = -s "$installdir/logfile" || 0;
 
 my @checklog;
-my $cmd ="./pg_regress --psqldir=$installdir/bin --dlpath=. "
+my $cmd ="./pg_regress --bindir=$installdir/bin --dlpath=. "
   ."$inputdir --port=$buildport collate.linux.utf8";
 @checklog = `cd $pgsql/src/test/regress && $cmd 2>&1`;
 

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

Re: [HACKERS] improving speed of make check-world

[Michael Paquier]

On Sat, Apr 25, 2015 at 7:59 AM, Peter Eisentraut  wrote:
> On 4/23/15 1:22 PM, Jeff Janes wrote:
>> Something about this commit (dcae5faccab64776376d354d) broke "make
>> check" in parallel conditions when started from a clean directory.  It
>> fails with a different error each time, one example:
>>
>> make -j4 check > /dev/null
>>
>> In file included from gram.y:14515:
>> scan.c: In function 'yy_try_NUL_trans':
>> scan.c:10307: warning: unused variable 'yyg'
>> /usr/bin/ld: tab-complete.o: No such file: No such file or directory
>> collect2: ld returned 1 exit status
>> make[3]: *** [psql] Error 1
>> make[2]: *** [all-psql-recurse] Error 2
>> make[2]: *** Waiting for unfinished jobs
>> make[1]: *** [all-bin-recurse] Error 2
>> make: *** [all-src-recurse] Error 2
>> make: *** Waiting for unfinished jobs
>
> I think the problem is that "check" depends on "all", but now also
> depends on temp-install, which in turn runs install and all.  With a
> sufficient amount of parallelism, you end up running two "all"s on top
> of each other.
>
> It seems this can be fixed by removing the check: all dependency.  Try
> removing that in the top-level GNUmakefile.in and see if the problem
> goes away.  For completeness, we should then also remove it in the other
> makefiles.

Yep. I spent some time yesterday and today poking at that, but I
clearly missed that dependency.. Attached is a patch fixing the
problem. I tested check and check-world with some parallel jobs and
both worked. In the case of check, the amount of logs is very reduced
because all the install process is done by temp-install which
redirects everything into tmp_install/log/install.log.
-- 
Michael
diff --git a/GNUmakefile.in b/GNUmakefile.in
index 361897a..42aeaa6 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -62,7 +62,7 @@ distclean maintainer-clean:
 # Garbage from autoconf:
 	@rm -rf autom4te.cache/
 
-check check-tests: all
+check check-tests:
 
 check check-tests installcheck installcheck-parallel installcheck-tests:
 	$(MAKE) -C src/test/regress $@
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 613e9c3..e47ebab 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -39,7 +39,7 @@ submake-test_decoding:
 
 REGRESSCHECKS=ddl rewrite toast permissions decoding_in_xact decoding_into_rel binary prepared
 
-regresscheck: all | submake-regress submake-test_decoding temp-install
+regresscheck: | submake-regress submake-test_decoding temp-install
 	$(MKDIR_P) regression_output
 	$(pg_regress_check) \
 	--temp-config $(top_srcdir)/contrib/test_decoding/logical.conf \
@@ -53,7 +53,7 @@ regresscheck-install-force: | submake-regress submake-test_decoding temp-install
 
 ISOLATIONCHECKS=mxact delayed_startup ondisk_startup concurrent_ddl_dml
 
-isolationcheck: all | submake-isolation submake-test_decoding temp-install
+isolationcheck: | submake-isolation submake-test_decoding temp-install
 	$(MKDIR_P) isolation_output
 	$(pg_isolation_regress_check) \
 	--temp-config $(top_srcdir)/contrib/test_decoding/logical.conf \
diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index fc809a0..d479788 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -58,7 +58,7 @@ clean distclean maintainer-clean:
 # ensure that changes in datadir propagate into object file
 initdb.o: initdb.c $(top_builddir)/src/Makefile.global
 
-check: all
+check:
 	$(prove_check)
 
 installcheck:
diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile
index 58f8b66..0d8421a 100644
--- a/src/bin/pg_basebackup/Makefile
+++ b/src/bin/pg_basebackup/Makefile
@@ -50,7 +50,7 @@ clean distclean maintainer-clean:
 		$(OBJS)
 	rm -rf tmp_check
 
-check: all
+check:
 	$(prove_check)
 
 installcheck:
diff --git a/src/bin/pg_config/Makefile b/src/bin/pg_config/Makefile
index 71ce236..dbc9899 100644
--- a/src/bin/pg_config/Makefile
+++ b/src/bin/pg_config/Makefile
@@ -49,7 +49,7 @@ clean distclean maintainer-clean:
 	rm -f pg_config$(X) $(OBJS)
 	rm -rf tmp_check
 
-check: all
+check:
 	$(prove_check)
 
 installcheck:
diff --git a/src/bin/pg_controldata/Makefile b/src/bin/pg_controldata/Makefile
index f7a4010..fd7399b 100644
--- a/src/bin/pg_controldata/Makefile
+++ b/src/bin/pg_controldata/Makefile
@@ -35,7 +35,7 @@ clean distclean maintainer-clean:
 	rm -f pg_controldata$(X) $(OBJS)
 	rm -rf tmp_check
 
-check: all
+check:
 	$(prove_check)
 
 installcheck:
diff --git a/src/bin/pg_ctl/Makefile b/src/bin/pg_ctl/Makefile
index 525e1d4..37eb482 100644
--- a/src/bin/pg_ctl/Makefile
+++ b/src/bin/pg_ctl/Makefile
@@ -38,7 +38,7 @@ clean distclean maintainer-clean:
 	rm -f pg_ctl$(X) $(OBJS)
 	rm -rf tmp_check
 
-check: all
+check:
 	$(prove_check)
 
 installcheck:
diff --git a/src/bin/scripts/Makefile b/src/bin/scripts/Makefile
index 8b6f54c..c831716 100644
--- a/src/bin/scripts/Makefile
+++ b/src/bin/scripts/Makefile
@@ -69,7 +69,7 @@ clean distclean maintainer-clean:
 	rm -f dumput

Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"

[Jan de Visser]

On April 22, 2015 06:04:42 PM Payal Singh wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   tested, failed
> Spec compliant:   not tested
> Documentation:tested, failed
> 
> Error in postgresql.conf gives the expected result on pg_ctl reload,
> although errors in pg_hba.conf file don't. Like before, reload completes
> fine without any information that pg_hba failed to load and only
> information is present in the log file. I'm assuming pg_ctl reload should
> prompt user if file fails to load irrespective of which file it is -
> postgresql.conf or pg_hba.conf.

Will fix. Not hard, just move the code that writes the .pid file to after the 
pg_hba reload.

> 
> There is no documentation change so far, but I guess that's not yet
> necessary.

I will update the page for pg_ctl. At least the behaviour of -w/-W in relation 
to the reload command needs explained.

> 
> gmake check passed all tests.
> 
> The new status of this patch is: Waiting on Author

jan



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

Re: [HACKERS] adding more information about process(es) cpu and memory usage

[Michael Paquier]

On Sat, Apr 25, 2015 at 4:23 PM, Julien Rouhaud wrote:
> There are at least two projects that provides this kind of statistics
> for backends: pg_proctab (https://github.com/markwkm/pg_proctab) and
> pg_stat_kcache (https://github.com/dalibo/pg_stat_kcache).  Michael also
> wrote an article on this topic some weeks ago
> (http://michael.otacoo.com/postgresql-2/postgres-calculate-cpu-usage-process/).

Don't use that hack except for development purposes ;) pg_stat_kcache
gives shows that core has enough facility for an extension doing
query-base CPU statistics by using the two hooks at executor init and
end with two calls to getrusage to calculate diffs for the stats it
needs. pg_proctab is useful as well to get live data.
-- 
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] forward vs backward slashes in msvc build code

[Peter Eisentraut]

On 4/24/15 12:22 AM, Michael Paquier wrote:
> On Fri, Mar 13, 2015 at 6:04 PM, Michael Paquier
>  wrote:
>> On Fri, Mar 13, 2015 at 6:20 AM, Alvaro Herrera wrote:
>>> Peter Eisentraut wrote:
 This is contrib/chkpass not finding the crypt symbol, which is
 presumably in some library.  But I can't see how it would normally find
 it, without my patch.
>>>
>>> It seems crypt is provided by libpgport.  So chkpass should be mentioned
>>> in @contrib_uselibpgport, but isn't.  Maybe the fix is just to add it
>>> there?
>>
>> I had a closer look at this patch, and yes indeed, the problem was
>> exactly that. Now honestly I cannot understand why this dependency
>> with libpgport was not necessary before... In any case, attached is a
>> patch rebased on HEAD that builds correctly with MSVC.
> 
> Now that the stuff related to the move from contrib/ to src/bin/,
> modulescheck and tmp_install has been committed, shouldn't we give a
> new shot at this patch? Attached is a rebased version.

done



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

Re: [HACKERS] Can pg_dump make use of CURRENT/SESSION_USER

[Fabrízio de Royes Mello]

On Wed, Mar 18, 2015 at 2:24 PM, Robert Haas  wrote:
>
> On Wed, Mar 18, 2015 at 1:23 PM, Fabrízio de Royes Mello
>  wrote:
> >> > If we ever implement something like
> >> >
> >> > COMMENT ON CURRENT_DATABASE IS ...
> >> >
> >> > it will be useful, because you will be able to restore a dump into
> >> > another database and have the comment apply to the target database.
> >
> > I think it's simple to implement, but how about pg_dump... we need to
add
> > new option (like --use-current-database) or am I missing something ?
>
> I think we'd just change it to use the new syntax, full stop.  I see
> no need for an option.
>

I'm returning on this...

What's the reasonable syntaxes?

COMMENT ON CURRENT DATABASE IS 'text';

or

COMMENT ON DATABASE { CURRENT_DATABASE | object_name } IS 'text';


> >> > (Also, I wonder about
> >> > ALTER USER foo IN DATABASE current_database ...
> >> > because that will let us dump per-database user options too.)
> >>
> >> +1 for both of those ideas.
> >
>

ALTER ROLE { role_specification | ALL } [ IN CURRENT DATABASE ] SET/RESET
...

or

ALTER ROLE { role_specification | ALL } [ IN { CURRENT DATABASE | DATABASE
database_name } ] SET/RESET ...


Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Feedback on getting rid of VACUUM FULL

[Simon Riggs]

On 24 April 2015 at 22:36, Jim Nasby  wrote:


> Instead of adding forcefsm, I think it would be more useful to accept a
> target block number. That way we can actually control where the new tuple
> goes. For this particular case we'd presumably go with normal FSM page
> selection logic, but someone could chose to to do something more
> sophisticated if they wanted.
>
> [1] http://postgresql.org/message-id/3409.1253147...@sss.pgh.pa.us
> [2] http://postgresql.org/message-id/3631.1253149...@sss.pgh.pa.us


I don't think specifying exact blocks will help, it will get us in more
trouble in the long run.

I think we need to be able to specify these update placement strategies

* TARGBLOCK_SAME - try to put the update on the same block if possible -
default
* TARGBLOCK_NEW - always force the update to go on a new block, to shrink
table rapidly

and these new block selection strategies

* FSM_ANY - Any block from FSM - default, as now
* FSM_NEAR - A block near the current one to maintain clustering as much as
possible - set automatically if table is clustered
* FSM_SHRINK - A block as near to block 0 as possible, while still handing
out different blocks to each backend by reselecting a block if we
experience write contention

I would suggest that if VACUUM finds the table is bloated beyond a specific
threshold it automatically puts it in FSM_SHRINK mode, and resets it back
to FSM_ANY once the bloat has reduced. That will naturally avoid bloat.

fsm modes can also be set manually to enforce bloat minimization.

We can also design a utility to actively use TARGBLOCK_NEW and FSM_SHRINK
to reduce table size without blocking writes.

But this is all stuff for 9.6...

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Bug in planner

[David Rowley]

On 24 April 2015 at 21:43, Teodor Sigaev  wrote:

> Hi!
>
> I faced with planner error:
> ERROR:  could not find RelOptInfo for given relids
>
>
Good find!

I've simplified your query a bit, the following still shows the issue
(using your schema):

SELECT *
FROM t1
WHERE NOT EXISTS (SELECT t2.c2 AS c1
  FROM t2
  LEFT OUTER JOIN t3 ON t2.c3 = t3.c1
  LEFT OUTER JOIN (SELECT t5.c1 AS c1
   FROM t4
   LEFT OUTER JOIN t5 ON t4.c2 = t5.c1 --offset 0
  ) a1 ON a1.c1 = t3.c2
  WHERE t1.c1 = t2.c2
);

I've done a little debugging on this too and I get the idea that in
eqjoinsel() that min_righthand incorrectly does not have a bit set for "t3"

When the failing call is made to find_join_rel() with the above query
relids being searched for has a decimal value of 388 (binary 11100 i.e
t5, t4, t2)

find_join_rel makes a pass over join_rel_list to search for the 388 valued
relids.

 join_rel_list contains the following:

1 -> 396  (110001100) t5, t4, t3, t2
2 -> 384  (11000) t5, t4
3 -> 392  (110001000) t5, t4, t3
4 -> 396  (110001100) t5, t4, t3, t2

I looked up simple_rte_array to determine which bits are for which relation.

simple_rte_array:
1 -> t1
2 -> t2
3 -> t3
4 -> join
5 -> a1
6 -> join
7 -> t4
8 -> t5


I'd imagine that the find_join_input_rel() search should actually be for
relids 396. I need to spend more time in this area to get a better grasp of
what's actually meant to be happening, but I think the problem lies
in make_outerjoininfo() when it determines what min_righthand should be set
to with the following:

/*
 * Similarly for required RHS.  But here, we must also include any lower
 * inner joins, to ensure we don't try to commute with any of them.
 */
min_righthand = bms_int_members(bms_union(clause_relids, inner_join_rels),
right_rels);

I think the problem seems to be down to the fact that inner_join_rels and
clause_relids are built from deconstruct_jointree() which I'd imagine does
not get modified when the subquery for t4 and t5 is pulled up, therefore is
out-of-date. 

I've attached a patch which appears to fix the problem, but this is more
for the purposes of a demonstration of where the problem lies. I don't have
enough knowledge of what's meant to be happening here, I'll need to spend
more time reading code and debugging.

On a side note, I just discovered another join removal opportunity:

explain select * from t1 where not exists(select 1 from t2 left join t3 on
t2.c1 = t3.c1 where t1.c1=t2.c1);

The join to t3 here is useless, as since it's a left join, the join could
only cause duplication of t2 rows, and this does not matter as we're
performing an anti join anyway (same applies for semi join).

Regards

David Rowley


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

[HACKERS] INSERT ... ON CONFLICT syntax issues

[Andres Freund]

I'm separating this discussion out of the  thread because I think this
needs wider input.

On 2015-04-24 19:21:37 -0700, Peter Geoghegan wrote:
> I've *provisionally* pushed code that goes back to the old way,
> Andres: 
> https://github.com/petergeoghegan/postgres/commit/2a5d80b27d2c5832ad26dde4651c64dd2004f401
>
> Perhaps this is the least worst way, after all.

I still think it's a bad idea. To recap, the old and current way is:

INSERT ... ON CONFLICT (cola, colb [WHERE predicate_for_partial]) UPDATE|IGNORE

My problem with the WHERE being inside the parens in the above is that
it's
a) different from CREATE INDEX
b) unclear whether the WHERE belongs to colb or the whole index
   expression. The equivalent for aggregates, which I bet is going to be
   used less often, caused a fair amount of confusing.

That's why I wanted the WHERE outside the (), which requires either
adding DO between the index inference clause, and the action, to avoid
ambiguities in the grammar.


But I'm generally having some doubts about the syntax.

Right now it's
INSERT ... ON CONFLICT opt_on_conf_clause UPDATE|IGNORE.

A couple things:

a) Why is is 'CONFLICT"? We're talking about a uniquness violation. What
   if we, at some later point, also want to handle other kind of
   violations? Shouldn't it be ON UNIQUE CONFLICT/ERROR/VIOLATION ...
b) For me there's a WITH before the index inference clause missing, to
   have it read in 'SQL' style.
c) Right now the UPDATE can refer to pseudo relations 'TARGET' and
   'EXCLUDED'. I think especially the latter doesn't fit anymore at
   all. How about 'CONFLICTING' and 'EXISTING'? Or even NEW and OLD?

So I guess it boils down to that I think we should switch the syntax to
be:

INSERT ... ON UNIQUE VIOLATION [WITH (cola, colb) WHERE ...] DO {NOTHING|UPDATE}

Greetings,

Andres Freund


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

Re: [HACKERS] Row security violation error is misleading

[Dean Rasheed]

On 25 April 2015 at 01:52, Stephen Frost  wrote:
> * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
>> The second patch [2] is the one that is actually relevant to this
>> thread. This patch is primarily to apply the RLS checks earlier,
>> before an update is attempted, more like a regular permissions check.
>> This adds a new enum to classify the kinds of WCO, a side benefit of
>> which is that it allows different error messages when RLS checks are
>> violated, as opposed to WITH CHECK OPTIONs on views.
>
> I've gone ahead and pushed this, please take a look and test it and let
> me know if you see any issues or have any questions or concerns.
>

Brilliant, thanks! I gave it a quick read-through and all the changes
look good, so thanks for all your work on this.

Regards,
Dean


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

Re: [HACKERS] adding more information about process(es) cpu and memory usage

[Julien Rouhaud]

Le 24/04/2015 21:11, Jim Nasby a écrit :
> On 4/24/15 6:29 AM, Robert Haas wrote:
>> On Thu, Apr 23, 2015 at 9:28 PM, Tom Lane  wrote:
>>> The reason nobody's gotten around to that in the last fifteen years is
>>> that per-process rusage isn't actually all that interesting; there's
>>> too much that happens in background daemons, for instance.
>>
>> There's *some* stuff that happens in background daemons, but if you
>> want to measure user and system time consume by a particularly query,
>> this would actually be a pretty handy way to do that, I think.
> 
> I more often am wondering what a running backend is doing OS-wise, but
> being able to see what happened when it finished would definitely be
> better than what's available now.

There are at least two projects that provides this kind of statistics
for backends: pg_proctab (https://github.com/markwkm/pg_proctab) and
pg_stat_kcache (https://github.com/dalibo/pg_stat_kcache).  Michael also
wrote an article on this topic some weeks ago
(http://michael.otacoo.com/postgresql-2/postgres-calculate-cpu-usage-process/).

Regards
-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


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