[HACKERS] dealing with extension dependencies that aren't quite 'e'

[Abhijit Menon-Sen]

Hi.

I'm looking at an extension that creates some triggers (on user tables)
dynamically (i.e., not during CREATE EXTENSION, but afterwards). The
author has two problems with it:

* «DROP EXTENSION ext» won't work without adding CASCADE, which is an
  (admittedly relatively minor) inconvenience to users.

* More importantly, pg_dump will dump all those trigger definitions,
  which is inappropriate in this case because the extension will try
  to create them.

As an experiment, I implemented "ALTER EXTENSION … ADD TRIGGER … ON …"
(a few-line patch to gram.y) and taught pg_dump.c's getTriggers() to
skip triggers with any 'e' dependency.

That works, in the sense that DROP EXTENSION will drop the triggers (but
the triggers can't be dropped on their own any more), and pg_dump will
ignore them. I'm trying to find a more generally useful mechanism that
addresses this problem and has a chance of being accepted upstream.

Rather than overloading 'e', we could introduce a new dependency type
that references an extension, but means that the dependent object should
be dropped when the extension is, but it can also be dropped on its own,
and pg_dump should ignore it. That would work for this case, and I can
imagine it *may* be useful for other types of objects (e.g., sequences
that depend on a seqam function provided by an extension, indexes that
depend on index functions provided by an extension).

But that leaves open the question of how exactly to record the
dependency: ALTER EXTENSION … ADD … is the easiest way, but it doesn't
feel right to introduce object-type-specific logic there to determine
the dependency type to use. Besides, I'm not entirely comfortable with
tying pg_dump behaviour so closely with the dependency, though there's
some sort of precedent there with deptype = 'i'.

In short, I can see some scope for improvement, but not clearly enough
to make a concrete proposal. I'm hoping for advice or suggestions with
a view towards submitting something to the next commitfest (2016-03).

Thanks.

-- Abhijit


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

Re: [HACKERS] POC: Cache data in GetSnapshotData()

[Mithun Cy]

On Mon, Jan 4, 2016 at 2:28 PM, Andres Freund  wrote:

> I think at the very least the cache should be protected by a separate
> lock, and that lock should be acquired with TryLock. I.e. the cache is
> updated opportunistically. I'd go for an lwlock in the first iteration.

I tried to implement a simple patch which protect the cache. Of all the
backend which
compute the snapshot(when cache is invalid) only one of them will write to
cache.
This is done with one atomic compare and swap operation.

After above fix memory corruption is not visible. But I see some more
failures at higher client sessions(128 it is easily reproducible).

Errors are
DETAIL:  Key (bid)=(24) already exists.
STATEMENT:  UPDATE pgbench_branches SET bbalance = bbalance + $1 WHERE bid
= $2;
client 17 aborted in state 11: ERROR:  duplicate key value violates unique
constraint "pgbench_branches_pkey"
DETAIL:  Key (bid)=(24) already exists.
client 26 aborted in state 11: ERROR:  duplicate key value violates unique
constraint "pgbench_branches_pkey"
DETAIL:  Key (bid)=(87) already exists.
ERROR:  duplicate key value violates unique constraint
"pgbench_branches_pkey"
DETAIL:  Key (bid)=(113) already exists.

After some analysis I think In GetSnapshotData() while computing snapshot.
/*
 * We don't include our own XIDs (if any) in the snapshot,
but we
 * must include them in xmin.
 */
if (NormalTransactionIdPrecedes(xid, xmin))
xmin = xid;
 *** if (pgxact == MyPgXact)   **
continue;

We do not add our own xid to xip array, I am wondering if other backend try
to use
the same snapshot will it be able to see changes made by me(current
backend).
I think since we compute a snapshot which will be used by other backends we
need to
add our xid to xip array to tell transaction is open.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com
*** a/src/backend/commands/cluster.c
--- b/src/backend/commands/cluster.c
***
*** 1559,1564  finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
--- 1559,1565 
  			elog(ERROR, "cache lookup failed for relation %u", OIDOldHeap);
  		relform = (Form_pg_class) GETSTRUCT(reltup);
  
+ 		Assert(TransactionIdIsNormal(frozenXid));
  		relform->relfrozenxid = frozenXid;
  		relform->relminmxid = cutoffMulti;
  
*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
***
*** 410,415  ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
--- 410,417 
  		if (LWLockConditionalAcquire(ProcArrayLock, LW_EXCLUSIVE))
  		{
  			ProcArrayEndTransactionInternal(proc, pgxact, latestXid);
+ pg_atomic_write_u32(&ProcGlobal->snapshot_cached, 0);
+ ProcGlobal->cached_snapshot_valid = false;
  			LWLockRelease(ProcArrayLock);
  		}
  		else
***
*** 557,562  ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
--- 559,568 
  		/* Move to next proc in list. */
  		nextidx = pg_atomic_read_u32(&proc->nextClearXidElem);
  	}
+ 
+ pg_atomic_write_u32(&ProcGlobal->snapshot_cached, 0);
+ 
+ ProcGlobal->cached_snapshot_valid = false;
  
  	/* We're done with the lock now. */
  	LWLockRelease(ProcArrayLock);
***
*** 1543,1548  GetSnapshotData(Snapshot snapshot)
--- 1549,1556 
  	 errmsg("out of memory")));
  	}
  
+ snapshot->takenDuringRecovery = RecoveryInProgress();
+ 
  	/*
  	 * It is sufficient to get shared lock on ProcArrayLock, even if we are
  	 * going to set MyPgXact->xmin.
***
*** 1557,1568  GetSnapshotData(Snapshot snapshot)
  	/* initialize xmin calculation with xmax */
  	globalxmin = xmin = xmax;
  
! 	snapshot->takenDuringRecovery = RecoveryInProgress();
  
! 	if (!snapshot->takenDuringRecovery)
  	{
  		int		   *pgprocnos = arrayP->pgprocnos;
  		int			numProcs;
  
  		/*
  		 * Spin over procArray checking xid, xmin, and subxids.  The goal is
--- 1565,1599 
  	/* initialize xmin calculation with xmax */
  	globalxmin = xmin = xmax;
  
! 	if (!snapshot->takenDuringRecovery && ProcGlobal->cached_snapshot_valid)
! 	{
! 		Snapshot csnap = &ProcGlobal->cached_snapshot;
! 		TransactionId *saved_xip;
! 		TransactionId *saved_subxip;
  
! 		saved_xip = snapshot->xip;
! 		saved_subxip = snapshot->subxip;
! 
! 		memcpy(snapshot, csnap, sizeof(SnapshotData));
! 
! 		snapshot->xip = saved_xip;
! 		snapshot->subxip = saved_subxip;
! 
! 		memcpy(snapshot->xip, csnap->xip,
! 			   sizeof(TransactionId) * csnap->xcnt);
! 		memcpy(snapshot->subxip, csnap->subxip,
! 			   sizeof(TransactionId) * csnap->subxcnt);
! 		globalxmin = ProcGlobal->cached_snapshot_globalxmin;
! 		xmin = csnap->xmin;
! 
! 		Assert(TransactionIdIsValid(globalxmin));
! 		Assert(TransactionIdIsValid(xmin));
! 	}
! 	else if (!snapshot->takenDuringRecovery)
  	{
  		int		   *pgprocnos = arrayP->pgprocnos;
  		int			numProc

Re: [HACKERS] Proposal: BSD Authentication support

[Chapman Flack]

Forgive my late comment ... I haven't used the PAM support in postgresql
either, or I'd know.  PAM (I know for sure), and I suppose similarly BSD
Authentication, models a generalized auth interaction where a given
authentication module can send a number of arbitrary prompts back to the
client (via callbacks so different protocols and UIs can be used), and
demand a number of arbitrary responses, so that a variety of authentication
schemes can easily be supported.

Is the PostgreSQL support (for either PAM or BSD Authentication) able to
handle that in its designed generality, or only for the case (number of
requested items = 1, item 1 = a password)?

Could the general form be handled with the existing fe/be protocol,
or would the protocol have to grow?

-Chap


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

Re: [HACKERS] Combining Aggregates

[Haribabu Kommi]

On Fri, Jan 15, 2016 at 3:34 PM, David Rowley
 wrote:
> On 8 January 2016 at 22:43, David Rowley 
> wrote:
>>
>> I've attached some re-based patched on current master. This is just to fix
>> a duplicate OID problem.
>>
>
> I've attached two updated patched to fix a conflict with a recent change to
> planner.c

I am getting following compilation error and warning with the latest patch,
because of a function prototype mismatch.

aggregatecmds.c: In function ‘DefineAggregate’:
aggregatecmds.c:93:8: warning: variable ‘serialTypeType’ set but not
used [-Wunused-but-set-variable]
  char  serialTypeType = 0;
^
clauses.c:434:1: error: conflicting types for ‘partial_aggregate_walker’
 partial_aggregate_walker(Node *node, partial_agg_context *context)
 ^
clauses.c:100:13: note: previous declaration of
‘partial_aggregate_walker’ was here
 static bool partial_aggregate_walker(Node *node, void *context);
 ^

Regards,
Hari Babu
Fujitsu Australia


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

Re: [HACKERS] Spelling corrections

[David Rowley]

On 15 January 2016 at 17:19, Robert Haas  wrote:

> On Sun, Jan 10, 2016 at 5:17 AM, David Rowley
>  wrote:
> > On 10 January 2016 at 19:34, Peter Geoghegan  wrote:
> >>
> >> Attached patch fixes a couple of misspellings.
> >
> > Snap!
> >
> http://www.postgresql.org/message-id/CAKJS1f-AGgQaurTwhY=wkJjspgDcmDUE8Yx03XTXCDz=kae...@mail.gmail.com
>
> Committed and back-patched.
>

Thanks for committing.

There was one other patch in the thread I started for this. I'm not sure if
you noticed:
http://www.postgresql.org/message-id/cakjs1f9ov2t0xepi2ss1iluqcedg4fpuc7ajy92yluz+c1h...@mail.gmail.com



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

Re: [HACKERS] Proposal: BSD Authentication support

[Robert Haas]

On Tue, Jan 12, 2016 at 2:27 AM, Marisa Emerson  wrote:
> I've attached the latest version of this patch. I've fixed up an issue with
> the configuration scripts that I missed.

Looks reasonable on a quick read-through.  Can anyone with access to a
BSD system review and test?

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


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

Re: [HACKERS] Spelling corrections

[Robert Haas]

On Sun, Jan 10, 2016 at 5:17 AM, David Rowley
 wrote:
> On 10 January 2016 at 19:34, Peter Geoghegan  wrote:
>>
>> Attached patch fixes a couple of misspellings.
>
> Snap!
> http://www.postgresql.org/message-id/CAKJS1f-AGgQaurTwhY=wkJjspgDcmDUE8Yx03XTXCDz=kae...@mail.gmail.com

Committed and back-patched.

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


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

[HACKERS] Typo in sequence.c

[Vinayak Pokale]

Hi,

I found a typo in sequence.c
Please check the attached patch.

Regards,
Vinayak


typo-sequence-c.patch
Description: Binary data

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

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/01/14 21:36, Rushabh Lathia wrote:

On Thu, Jan 14, 2016 at 2:00 PM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>> wrote:



On 2016/01/12 20:31, Rushabh Lathia wrote:



On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>
>> wrote:
 On 2016/01/06 18:58, Rushabh Lathia wrote:
 .) What the need of following change ?

 @@ -833,9 +833,6 @@ appendWhereClause(StringInfo buf,
   int nestlevel;
   ListCell   *lc;

 -   if (params)
 -   *params = NIL;  /* initialize result
list to
 empty */
 -
   /* Set up context struct for recursion */
   context.root = root;
   context.foreignrel = baserel;
 @@ -971,6 +968,63 @@ deparseUpdateSql(StringInfo buf,
 PlannerInfo *root,
}



 It is needed for deparsePushedDownUpdateSql to store params
in both
 WHERE clauses and expressions to assign to the target columns
 into one params_list list.



Hmm sorry but I am still not getting the point, can you provide some
example to explain this ?



Sorry, maybe my explanation was not enough.  Consider:

postgres=# create foreign table ft1 (a int, b int) server myserver
options (table_name 't1');
postgres=# insert into ft1 values (0, 0);
postgres=# prepare mt(int, int) as update ft1 set a = $1 where b = $2;
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);

After the 5 executions of mt we have

postgres=# explain verbose execute mt(1, 0);
  QUERY PLAN


  Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
->  Foreign Update on public.ft1  (cost=100.00..140.35 rows=12
width=10)
  Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b
= $2::integer))
(3 rows)

If we do that initialization in appendWhereClause, we would get a
wrong params_list list and a wrong remote pushed-down query for the
last mt() in deparsePushedDownUpdateSql.



Strange, I am seeing same behaviour with or without that initialization in
appendWhereClause. After the 5 executions of mt I with or without I am
getting following output:

postgres=# explain verbose execute mt(1, 0);
  QUERY PLAN

  Update on public.ft2  (cost=100.00..140.35 rows=12 width=10)
->  Foreign Update on public.ft2  (cost=100.00..140.35 rows=12 width=10)
  Remote SQL: UPDATE public.t2 SET a = $1::integer WHERE ((b =
$2::integer))
(3 rows)


Really?  With that initialization in appendWhereClause, I got the 
following wrong result (note that both parameter numbers are $1):


postgres=# explain verbose execute mt(1, 0);
 QUERY PLAN

 Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
   ->  Foreign Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
 Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b = 
$1::integer))

(3 rows)


BTW, I keep a ForeignScan node pushing down an update to the remote
server, in the updated patches.  I have to admit that that seems
like rather a misnomer.  So, it might be worth adding a new
ForeignUpdate node, but my concern about that is that if doing so,
we would have a lot of duplicate code in ForeignUpdate and
ForeignScan.  What do you think about that?



Yes, I noticed that in the patch and I was about to point that out in my
final review. As first review I was mainly focused on the functionality
testing
and other overview things. Another reason I haven't posted that in my
first review round is, I was not quite sure whether we need the
separate new node ForeignUpdate, ForeignDelete  and want to duplicate
code? Was also not quite sure about the fact that what we will achieve
by doing that.

So I thought, I will have this open question in my final review comment,
and will take committer's opinion on this. Since you already raised this
question lets take others opinion on this.


OK, let's do that.

Best regards,
Etsuro Fujita




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

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 1/11/16 6:50 PM, Alvaro Herrera wrote:

David Steele wrote:

The patch creates a new counter to separate the log filtering from the
authentication functionality.  This makes it possible to get the same
filtering in other parts of the code (or extensions) without abusing the
ClientAuthInProgress variable or using the log hook.


Hmm, okay, but this would need a large blinking comment explaining that
the calling code have better set a PG_TRY block when incrementing, so
that on errors it resets to zero again.  Or maybe some handling in
AbortOutOfAnyTransaction/AbortCurrentTransaction.  or both.


In the case of pgaudit only the ereport call is wrapped in the 
LimitClientLogOutput() calls which I thought was safe - am I wrong about 
that?



I also considered a new function for ereport (like errhidecontext()) but
this mechanism would not have worked for authentication and so would not
have been used anywhere in core.



If the audit code
calls something that throws an error (other than an audit message -- say
"out of memory"), then it would also be hidden, but you may not want it
to be hidden.


This shouldn't happen -- see above.

I think maybe it's better to have each individual error

message be marked as "don't show to client" rather than a global var.


That's easy enough to do and I already have the code for it since that's 
the first thing I tried.  However, there were two reasons I didn't 
submit that version:


1) Unless pgaudit is committed there wouldn't be any code calling the 
errhidefromclient() function and that seemed like a bad plan.


2) There would be two different ways to suppress client messages but I 
was hoping to only have one.


As you say, authentication is a different beast so maybe #2 is not a big 
deal.  I could combine the alternative ereport patch with the pgaudit 
patch to address #1 but I would like to have the capability in core 
whether pgaudit is committed or not, which is why I submitted it separately.


Any advice would be greatly appreciated.

Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Robert Haas]

On Mon, Jan 11, 2016 at 6:50 PM, Alvaro Herrera
 wrote:
> I think maybe it's better to have each individual error
> message be marked as "don't show to client" rather than a global var.

+1.

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


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

[HACKERS] Comment thinko in expand_inherited_rtentry()

[Amit Langote]

Hi,

Attached fixes what looks like a thinko in a comment: It is the child
relations that are "non-local" temp tables that are skipped from being
included the inheritance set. The comment in question as it stands,
doesn't note that.

Thanks,
Amit
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 694e9ed..11a76af 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1415,9 +1415,9 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 	heap_close(oldrelation, NoLock);
 
 	/*
-	 * If all the children were temp tables, pretend it's a non-inheritance
-	 * situation.  The duplicate RTE we added for the parent table is
-	 * harmless, so we don't bother to get rid of it.
+	 * If all the children were non-local temp tables, pretend it's a
+	 * non-inheritance situation.  The duplicate RTE we added for the
+	 * parent table is harmless, so we don't bother to get rid of it.
 	 */
 	if (list_length(appinfos) < 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] SET syntax in INSERT

[Robert Haas]

On Thu, Jan 14, 2016 at 3:25 PM, Tom Lane  wrote:
> "David G. Johnston"  writes:
>> On Thu, Jan 14, 2016 at 1:07 PM, Tom Lane  wrote:
>>> Vitaly Burovoy  writes:
 You can't now do something like
 INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);
>
>>> Hm ... actually, you might want to try that before opining
>
>> So what's the problem, then?  It seems like a decision has already been
>> made.
>
> Yeah, but is it a decision that we might later find to be at odds
> with a future SQL standard?  The more places we embed that behavior,
> the more risk we take.

I don't see it.  If the SQL standard committee defines foo[2] to mean
something other than array access to element 2 of foo, then we've got
a problem, but they're not going to define it different ways for
SELECT, INSERT, and UPDATE.  And even if they did, we're certainly not
going to want those to mean different and incompatible things.  So I
don't think doubling down on the syntax we already support loses
anything, really.

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


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

[HACKERS] Comment typo in port/atomics/generic.h

[Tatsuro Yamada]

Hi,

I found a typo in generic.h
The attached patch fixes it: s/tomic/atomic/g

Best regards,
Tatsuro Yamada
*** a/src/include/port/atomics/generic.h
--- b/src/include/port/atomics/generic.h
***
*** 1,7 
  /*-
   *
   * generic.h
!  *	  Implement higher level operations based on some lower level tomic
   *	  operations.
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
--- 1,7 
  /*-
   *
   * generic.h
!  *	  Implement higher level operations based on some lower level atomic
   *	  operations.
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group

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

Re: [HACKERS] extend pgbench expressions with functions

[Michael Paquier]

On Thu, Jan 14, 2016 at 5:54 PM, Fabien COELHO  wrote:
> I wrote:
>> - fprintf(stderr, "gaussian parameter must be at least
>> %f (not \"%s\")\n", MIN_GAUSSIAN_PARAM, argv[5]);
>> +fprintf(stderr,
>> +   "random gaussian parameter must be greater
>> than %f "
>> +"(got %f)\n", MIN_GAUSSIAN_PARAM, parameter);
>> This looks like useless noise to me. Why changing those messages?
>
>
> Because the message was not saying that it was about random, and I think
> that it is the important.
>>  fprintf(stderr,
>> -   "exponential parameter must be greater than
>> zero (not \"%s\")\n",
>> -argv[5]);
>> +  "random exponential parameter must be greater than
>> 0.0 "
>> +   "(got %f)\n", parameter);
>>  st->ecnt++;
>> -return true;
>> +   return false;
>> This diff is noise as well, and should be removed.
>
> Ok, I can but "zero" and "not" back, but same remark as above, why not tell
> that it is about random? This information is missing.

Those things should be a separate patch then, committed separately as
they provide more verbose messages.

>> +   /*
>> +* Note: this section could be removed, as the same
>> functionnality
>> +* is available through \set xxx random_gaussian(...)
>> +*/
>> I think that you are right to do that. That's no fun to break existing
>> scripts, even if people doing that with pgbench are surely experienced
>> hackers.
>
> Ok, but I would like a clear go or vote before doing this.

For now, I am seeing opinions on those matters from nobody else than
me and you, and we got toward the same direction. If you think that
there is a possibility that somebody has a different opinion on those
matters, and it is likely so let's keep the patch as-is then and wait
for more input: it is easier to remove code than add it back. I am not
sure what a committer would say, and it surely would be a waste of
time to just move back and worth for everybody.

>> int() should be strengthened regarding bounds. For example:
>> \set cid debug(int(int(9223372036854775807) +
>> double(9223372036854775807)))
>> debug(script=0,command=1): int -9223372036854775808
>
>
> Hmmm. You mean just to check the double -> int conversion for overflow,
> as in:
>
>   SELECT (9223372036854775807::INT8 +
>   9223372036854775807::DOUBLE PRECISION)::INT8;
>
> Ok.

Yes, that's what I mean. The job running into that should definitely
fail with a proper out-of-bound error message.
-- 
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] About get_relation_constraints and include_notnull

[Amit Langote]

On 2016/01/14 23:36, Tom Lane wrote:
> Amit Langote  writes:
>> Why does the argument include_notnull argument exist if
>> get_relation_constraints() is being called from only one place? Perhaps we
>> could remove it and add the IS NOT NULL test expression unconditionally if
>> there are any NOT NULL columns.
> 
> Well, you could argue why have a routine at all instead of inlining it
> into the one caller.  IIRC the thought was that other likely uses of
> constraint-fetching might want to see only the actual check constraints.
> Is there some positive benefit from removing the argument?

The function is local to plancat.c and hasn't seen any callers beside
relation_excluded_by_constraints() for a long time (as can be told from
the git history). It's just that initially I thought it's being used
elsewhere which upon inspecting I didn't find any. Not really sure I can
make the analogy but ExecConstraints() performs both NOT NULL and CHECK
constraint checks without any argument.

Though, we could just leave it alone. Sorry for the noise.

Thanks,
Amit




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

Re: [HACKERS] Doubt in 9.5 release note

[Tom Lane]

Tatsuo Ishii  writes:
> I saw following item in release-9.5.sgml:
> 
>
> Support comments on domain
> constraints (Álvaro Herrera)
>
>   

> It seems the release note has nothing to do with the commit. Also,
> commenting on DOMAIN constraints is not new in 9.5, I think.

Yeah, it is.  See 7eca575d1c28f6eee2bf4564f3d458d10c4a8f47, which is
the commit that should have been listed here, likely.

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] Removing Functionally Dependent GROUP BY Columns

[Julien Rouhaud]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 14/01/2016 23:05, David Rowley wrote:
> On 15 January 2016 at 00:19, Julien Rouhaud
> mailto:julien.rouh...@dalibo.com>>
> wrote:
> 
> 
> +* Technically we could look at UNIQUE indexes
> too, however we'd also +* have to ensure that each
> column of the unique index had a NOT NULL
> 
> 
> s/had/has/
> 
> 
> +* constraint, however since NOT NULL constraints 
> currently are don't
> 
> 
> s/are //
> 
> 
> Both fixed. Thanks.
> 
> 
>> +   /* +* If we found any surplus Vars in the GROUP BY
>> clause, then we'll build +* a new GROUP BY clause without
>> these surplus Vars. +*/ +   if (anysurplus) +   { +
>> List *new_groupby = NIL; + +   foreach (lc,
>> root->parse->groupClause) +   { +   SortGroupClause
>> *sgc = (SortGroupClause *) lfirst(lc); +   TargetEntry
>> *tle; +   Var *var; + +   tle =
>> get_sortgroupclause_tle(sgc, root->parse->targetList); +
>> var = (Var *) tle->expr; + +   if (!IsA(var, Var)) +
>> continue; + [...]
>> 
>> if var isn't a Var, it needs to be added in new_groupby.
>> 
>> 
>> Yes you're right, well, at least I've written enough code to
>> ensure that it's not needed.
> 
> Oh yes, I realize that now.
> 
> 
> I meant to say "I've not written enough code" ...
> 

Yes, that makes sense with the explanation you wrote just after :)

> 
>> Technically we could look inside non-Vars and check if the Expr
>> is just made up of Vars from a single relation, and then we could
>> also make that surplus if we find other Vars which make up the
>> table's primary key.  I didn't make these changes as I thought it
>> was a less likely scenario. It wouldn't be too much extra code to
>> add however. I've went and added an XXX comment to explain that
>> there might be future optimisation opportunities in the future
>> around this.
>> 
> 
> Agreed.
> 
>> I've attached an updated patch.
>> 
> 
> +   /* don't try anything unless there's two Vars */ +
> if (varlist == NULL || list_length(varlist) < 2) +
> continue;
> 
> To be perfectly correct, the comment should say "at least two
> Vars".
> 
> 
> Changed per discussion from you and Geoff
> 
> 
> Except the small remaining typos, this patch looks very fine to me.
> I flag it as ready for committer.
> 
> 
> Many thanks for your followup review.
> 
> I've attached an updated patch to address what you highlighted
> above.
> 

Thanks!

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


- -- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.17 (GNU/Linux)

iQEcBAEBAgAGBQJWmCoYAAoJELGaJ8vfEpOqJzIIALajMHHd8aCDnAuH9uNUyevU
EuKyHTRDJkc8KUNbtDeSpVf9UGT3XUaZx4k/o+aKDdRB/RfYK0GKyDv2Owr4Wx3F
5BY9GuEO3vjqaFuDBH5u9EjySal3jQYC57nB3I0hRvpVRQBi0nFyQre/SXplCB6q
X1NqBfICyu6lwwocAMCeW9qN4ZQclLjxoScJpA4ml9Kj6CQvK2dDSyS00gstLFPH
Bj+n20wEcC7ZyxCpxfHmoZW1sjAvZK5mwrEdFG+lvCO8OwN/73YvDFzQHrpvVXWE
ZVoz069kfekZtXQ1OQ5CcvOAJLD9ewbPq+rpKh9YQorZB1R9QEj0qaxqo7LtjhA=
=G/uH
-END PGP SIGNATURE-


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

[HACKERS] Doubt in 9.5 release note

[Tatsuo Ishii]

I saw following item in release-9.5.sgml:


   
Support comments on domain
constraints (Álvaro Herrera)
   
  

It seems the release note has nothing to do with the commit. Also,
commenting on DOMAIN constraints is not new in 9.5, I think.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

Re: [HACKERS] Removing Functionally Dependent GROUP BY Columns

[David Rowley]

On 15 January 2016 at 00:19, Julien Rouhaud 
wrote:

>
> +* Technically we could look at UNIQUE indexes too,
> however we'd also
> +* have to ensure that each column of the unique index had
> a NOT NULL
>
>
> s/had/has/
>
>
> +* constraint, however since NOT NULL constraints
> currently are don't
>
>
> s/are //
>

Both fixed. Thanks.


> > +   /*
> > +* If we found any surplus Vars in the GROUP BY clause, then
> > we'll build
> > +* a new GROUP BY clause without these surplus Vars.
> > +*/
> > +   if (anysurplus)
> > +   {
> > +   List *new_groupby = NIL;
> > +
> > +   foreach (lc, root->parse->groupClause)
> > +   {
> > +   SortGroupClause *sgc = (SortGroupClause *) lfirst(lc);
> > +   TargetEntry *tle;
> > +   Var *var;
> > +
> > +   tle = get_sortgroupclause_tle(sgc,
> root->parse->targetList);
> > +   var = (Var *) tle->expr;
> > +
> > +   if (!IsA(var, Var))
> > +   continue;
> > + [...]
> >
> > if var isn't a Var, it needs to be added in new_groupby.
> >
> >
> > Yes you're right, well, at least I've written enough code to ensure that
> > it's not needed.
>
> Oh yes, I realize that now.
>
>
I meant to say "I've not written enough code" ...


> > Technically we could look inside non-Vars and check if the Expr is just
> > made up of Vars from a single relation, and then we could also make that
> > surplus if we find other Vars which make up the table's primary key.  I
> > didn't make these changes as I thought it was a less likely scenario. It
> > wouldn't be too much extra code to add however. I've went and added an
> > XXX comment to explain that there might be future optimisation
> > opportunities in the future around this.
> >
>
> Agreed.
>
> > I've attached an updated patch.
> >
>
> +   /* don't try anything unless there's two Vars */
> +   if (varlist == NULL || list_length(varlist) < 2)
> +   continue;
>
> To be perfectly correct, the comment should say "at least two Vars".
>
>
Changed per discussion from you and Geoff


> Except the small remaining typos, this patch looks very fine to me. I
> flag it as ready for committer.


Many thanks for your followup review.

I've attached an updated patch to address what you highlighted above.


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


prune_group_by_clause_59f15cf_2016-01-15.patch
Description: Binary data

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

Re: [HACKERS] SET syntax in INSERT

[David G. Johnston]

On Thu, Jan 14, 2016 at 1:25 PM, Tom Lane  wrote:

> "David G. Johnston"  writes:
> > On Thu, Jan 14, 2016 at 1:07 PM, Tom Lane  wrote:
> >> Vitaly Burovoy  writes:
> >>> You can't now do something like
> >>> INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);
>
> >> Hm ... actually, you might want to try that before opining
>
> > So what's the problem, then?  It seems like a decision has already been
> > made.
>
> Yeah, but is it a decision that we might later find to be at odds
> with a future SQL standard?  The more places we embed that behavior,
> the more risk we take.
>

While I agree with the sentiment I'm not seeing the added risk introduced
as being a major blocker if the syntactic sugar is indeed popular and
otherwise desirable from a code maintenance standpoint.  If the standard
introduces a contradictory concept that we need to address we can do so.
As we've already defined this specific behavior any conflict will likely
result in the already defined behavior changing since having the same
overall concept implemented differently for "VALUES" compared to "SET"
would be undesirable​.  If we end up changing that whether we
"doubled-down" by implementing "SET" seems immaterial.

The question, then, is whether there is any behavior that needs to be
uniquely defined for SET that doesn't already come into play when using
VALUES or SELECT?  I cannot think of any but given the somewhat clandestine
nature of your first example maybe you know of others.

David J.


David J.

Re: [HACKERS] SET syntax in INSERT

[Tom Lane]

"David G. Johnston"  writes:
> On Thu, Jan 14, 2016 at 1:07 PM, Tom Lane  wrote:
>> Vitaly Burovoy  writes:
>>> You can't now do something like
>>> INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);

>> Hm ... actually, you might want to try that before opining

> So what's the problem, then?  It seems like a decision has already been
> made.

Yeah, but is it a decision that we might later find to be at odds
with a future SQL standard?  The more places we embed that behavior,
the more risk we take.

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] SET syntax in INSERT

[Vitaly Burovoy]

On 1/14/16, Tom Lane  wrote:
> Vitaly Burovoy  writes:
>> On 1/14/16, Tom Lane  wrote:
>>> It's more than syntactic sugar; you are going to have to invent
>>> semantics,
>>> as well, because it's less than clear what partial-field assignments
>>> should do.
>>>
>>> Assume a table with an int-array column, and consider
>>> INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;
>
>> Right part is a column name, not an expression. Isn't it?
>
> UPDATE takes this just fine.  The difference is that in UPDATE there's
> no question what the starting value of the column is.
>
>> You can't now do something like
>> INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);
>
> Hm ... actually, you might want to try that before opining.

Oops… Thank you, It's a new feature for me.
But since INSERT has that feature there is no question what to do in such case:

postgres=# create table testtable(i int[] default '{1,3,5}'::int[]);
CREATE TABLE
postgres=# insert into testtable (i[5], i[3], i[1]) values (3,5,4);
INSERT 0 1
postgres=# select * from testtable;
 i
---
 {4,NULL,5,NULL,3}
(1 row)

Save current behavior, i.e. if any array subscript is given, don't
evaluate the default!

-- 
Best regards,
Vitaly Burovoy


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

Re: [HACKERS] SET syntax in INSERT

[David G. Johnston]

On Thu, Jan 14, 2016 at 1:07 PM, Tom Lane  wrote:

> Vitaly Burovoy  writes:
> > On 1/14/16, Tom Lane  wrote:
> >> It's more than syntactic sugar; you are going to have to invent
> semantics,
> >> as well, because it's less than clear what partial-field assignments
> >> should do.
> >>
> >> Assume a table with an int-array column, and consider
> >> INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;
>
> > Right part is a column name, not an expression. Isn't it?


> > You can't now do something like
> > INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);
>
> Hm ... actually, you might want to try that before opining
>

​So what's the problem, then?  It seems like a decision has already been
made.

David J.
​

Re: [HACKERS] SET syntax in INSERT

[Andrew Dunstan]

On 01/14/2016 03:00 PM, Marko Tiikkaja wrote:

On 2016-01-14 20:50, Vitaly Burovoy wrote:

On 1/14/16, Tom Lane  wrote:

Assume a table with an int-array column, and consider

INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;


Right part is a column name, not an expression. Isn't it?
So "arraycol[2]" is not possible there.


I think the idea here was that it's allowed in UPDATE.  But I don't 
see the point of allowing that in an INSERT.







Right. Why not just forbid anything other than a plain column name on 
the LHS for INSERT, at least as a first cut.


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] SET syntax in INSERT

[Tom Lane]

Vitaly Burovoy  writes:
> On 1/14/16, Tom Lane  wrote:
>> It's more than syntactic sugar; you are going to have to invent semantics,
>> as well, because it's less than clear what partial-field assignments
>> should do.
>> 
>> Assume a table with an int-array column, and consider
>> INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;

> Right part is a column name, not an expression. Isn't it?

UPDATE takes this just fine.  The difference is that in UPDATE there's
no question what the starting value of the column is.

> You can't now do something like
> INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);

Hm ... actually, you might want to try that before opining.

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] SET syntax in INSERT

[Marko Tiikkaja]

On 2016-01-14 20:50, Vitaly Burovoy wrote:

On 1/14/16, Tom Lane  wrote:

Assume a table with an int-array column, and consider

INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;


Right part is a column name, not an expression. Isn't it?
So "arraycol[2]" is not possible there.


I think the idea here was that it's allowed in UPDATE.  But I don't see 
the point of allowing that in an INSERT.



.m


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

Re: [HACKERS] SET syntax in INSERT

[Vitaly Burovoy]

On 1/14/16, Tom Lane  wrote:
> Pavel Stehule  writes:
 Probably there is less risk than 7 years ago, but still creating own
 syntax isn't the best idea. This is syntactic sugar only and different
 from ANSi SQL or common standard.
>
> It's more than syntactic sugar; you are going to have to invent semantics,
> as well, because it's less than clear what partial-field assignments
> should do.
>
> Assume a table with an int-array column, and consider
>
> INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;

Right part is a column name, not an expression. Isn't it?
So "arraycol[2]" is not possible there.

You can't now do something like
INSERT INTO foo (arraycol[2], arraycol[4]) VALUES(7, 11);

> I wonder what the other elements of the array will be set to, and what
> the array dimensions will end up being.
>
> If there's a default expression for the array column, does that change
> your answer?
>
> If you say "we'll apply the default and then perform the SET assignments",
> what's your criterion for deciding that you *don't* need to evaluate the
> default?  If the default has side effects (think nextval()) this is a
> user-visible choice.

Default values can be explicitly set as they are set in UPDATE:
INSERT INTO foo SET defcol = DEFAULT;

> I don't say that these questions are unresolvable, but there is certainly
> more here than meets the eye; and therefore there's a nonzero chance of
> being blindsided if the SQL committee someday standardizes this syntax
> and makes some different decisions about what it means.
>
>   regards, tom lane

Be honest I've dreamed about that syntax since I started to work with PG, so +1
-- 
Best regards,
Vitaly Burovoy


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

Re: [HACKERS] SET syntax in INSERT

[Marko Tiikkaja]

On 2016-01-14 20:33, Tom Lane wrote:

Pavel Stehule  writes:

Probably there is less risk than 7 years ago, but still creating own
syntax isn't the best idea. This is syntactic sugar only and different
from ANSi SQL or common standard.


It's more than syntactic sugar; you are going to have to invent semantics,
as well, because it's less than clear what partial-field assignments
should do.


I don't really care for such.  In my opinion it would be fine if this 
simply was only "syntactic sugar", and trying to do any tricks like this 
would simply raise an exception.



.m


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

Re: [HACKERS] SET syntax in INSERT

[Tom Lane]

Pavel Stehule  writes:
>>> Probably there is less risk than 7 years ago, but still creating own
>>> syntax isn't the best idea. This is syntactic sugar only and different
>>> from ANSi SQL or common standard.

It's more than syntactic sugar; you are going to have to invent semantics,
as well, because it's less than clear what partial-field assignments
should do.

Assume a table with an int-array column, and consider

INSERT INTO foo SET arraycol[2] = 7, arraycol[4] = 11;

I wonder what the other elements of the array will be set to, and what
the array dimensions will end up being.

If there's a default expression for the array column, does that change
your answer?

If you say "we'll apply the default and then perform the SET assignments",
what's your criterion for deciding that you *don't* need to evaluate the
default?  If the default has side effects (think nextval()) this is a
user-visible choice.

I don't say that these questions are unresolvable, but there is certainly
more here than meets the eye; and therefore there's a nonzero chance of
being blindsided if the SQL committee someday standardizes this syntax
and makes some different decisions about what it means.

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] SET syntax in INSERT

[Pavel Stehule]

2016-01-14 20:09 GMT+01:00 Marko Tiikkaja :

> On 2016-01-14 8:06 PM, Pavel Stehule wrote:
>
>> Probably there is less risk than 7 years ago, but still creating own
>> syntax
>> isn't the best idea. This is syntactic sugar only and different from ANSi
>> SQL or common standard.
>>
>
> So is RETURNING,


is it ANSI SQL redundant?


> UPSERT,


the behave is partially different than MERGE, so different syntax is 100%
valid


> PL/PgSQL and many other useful features.
>

PL/pgSQL is PL/SQL clone, and because the base is Ada, it cannot be
compatible with SQL/PSM.

Regards

Pavel


>
>
> .m
>

Re: [HACKERS] SET syntax in INSERT

[Marc Mamin]

>SET syntax for INSERT was brought up a few years ago here:
>http://www.postgresql.org/message-id/2c5ef4e30908251010s46d9d566m1da21357891ba...@mail.gmail.com

>What do we think?

+1
this would save comments in long queries.
and usindg AS as style helper as suggested in the old post has its caveat: 

create temp table t( a int,b int);

insert into t select
1 as b,
2 as a;

select * from t ...

regards, 
Marc Mamin

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

Re: [HACKERS] SET syntax in INSERT

[Marko Tiikkaja]

On 2016-01-14 8:06 PM, Pavel Stehule wrote:

Probably there is less risk than 7 years ago, but still creating own syntax
isn't the best idea. This is syntactic sugar only and different from ANSi
SQL or common standard.


So is RETURNING, UPSERT, PL/PgSQL and many other useful features.


.m


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

Re: [HACKERS] SET syntax in INSERT

[Pavel Stehule]

2016-01-14 19:51 GMT+01:00 Robert Haas :

> On Thu, Jan 14, 2016 at 12:13 PM, Marko Tiikkaja  wrote:
> > SET syntax for INSERT was brought up a few years ago here:
> >
> http://www.postgresql.org/message-id/2c5ef4e30908251010s46d9d566m1da21357891ba...@mail.gmail.com
> >
> > From the discussion it seems that one committer was against, one
> committer
> > was not against, and one committer saw something good in the proposal.
> > Personally, I believe this would be a huge readability improvement to
> > INSERTs with more than a few columns.  I'm willing to put in some work to
> > make this happen for 9.7, but I'd like to know that I'm not wasting my
> time.
>
> I'm mildly in favor of this proposal.  I think that "-1 PostgreSQL
> isn't MySQL!" is maybe the lamest reason for not supporting useful
> syntax that I can think of.
>

Now I am able to write more correct argument. It is one from basic SQL
statement, and for using proprietary syntax should be pretty strong reason.

Probably there is less risk than 7 years ago, but still creating own syntax
isn't the best idea. This is syntactic sugar only and different from ANSi
SQL or common standard.

Regards

Pavel



>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] SET syntax in INSERT

[Robert Haas]

On Thu, Jan 14, 2016 at 12:13 PM, Marko Tiikkaja  wrote:
> SET syntax for INSERT was brought up a few years ago here:
> http://www.postgresql.org/message-id/2c5ef4e30908251010s46d9d566m1da21357891ba...@mail.gmail.com
>
> From the discussion it seems that one committer was against, one committer
> was not against, and one committer saw something good in the proposal.
> Personally, I believe this would be a huge readability improvement to
> INSERTs with more than a few columns.  I'm willing to put in some work to
> make this happen for 9.7, but I'd like to know that I'm not wasting my time.

I'm mildly in favor of this proposal.  I think that "-1 PostgreSQL
isn't MySQL!" is maybe the lamest reason for not supporting useful
syntax that I can think of.

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Jan 14, 2016 at 12:14 PM, Andres Freund  wrote:
> On 2016-01-14 12:07:23 -0500, Robert Haas wrote:
>> > Do we want to provide a backward compatible API for all this? I'm fine
>> > either way.
>>
>> How would that work?
>
> I'm thinking of something like;
>
> int WaitOnLatchSet(LatchEventSet *set, int wakeEvents, long timeout);
>
> int
> WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,long 
> timeout)
> {
> LatchEventSet set;
>
> LatchEventSetInit(&set, latch);
>
> if (sock != PGINVALID_SOCKET)
>LatchEventSetAddSock(&set, sock);
>
> return WaitOnLatchSet(set, wakeEvents, timeout);
> }
>
> I think we'll need to continue having wakeEvents and timeout parameters
> for WaitOnLatchSet, we quite frequently want to wait socket
> readability/writability, not wait on the socket, or have/not have
> timeouts.

Well, if we ever wanted to support multiple FDs, we'd need the
readability/writeability thing to be per-fd, not per-set.

Overall, if this is what you have in mind for backward compatibility,
I rate it M for Meh.  Let's just break compatibility and people will
have to update their code.  That shouldn't be hard, and if we don't
make people do it when we make the change, then we'll be stuck with
the backward-compatibility interface for a decade.  I doubt it's worth
it.

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-01-14 18:14:21 +0100, Andres Freund wrote:
> On 2016-01-14 12:07:23 -0500, Robert Haas wrote:
> > > Do we want to provide a backward compatible API for all this? I'm fine
> > > either way.
> >
> > How would that work?
> 
> I'm thinking of something like;
> 
> int WaitOnLatchSet(LatchEventSet *set, int wakeEvents, long timeout);
> 
> int
> WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,long 
> timeout)
> {
> LatchEventSet set;
> 
> LatchEventSetInit(&set, latch);
> 
> if (sock != PGINVALID_SOCKET)
>LatchEventSetAddSock(&set, sock);
> 
> return WaitOnLatchSet(set, wakeEvents, timeout);
> }
> 
> I think we'll need to continue having wakeEvents and timeout parameters
> for WaitOnLatchSet, we quite frequently want to wait socket
> readability/writability, not wait on the socket, or have/not have
> timeouts.

Hm. If we really want to support multiple sockets at some point the
above WaitOnLatchSet signature isn't going to fly, because it won't
support figuring out which fd the event this triggered on.

So it seems we'd need to return something like
struct LatchEvent
{
enum LatchEventType {WL_LATCH_EVENT;WL_TIMEOUT;WL_SOCKET_EVENT; 
WL_POSTMASTER_EVENT;...} event_type;
int mask;
pgsocket event_sock;
};

that'd also allow to extend this to return multiple events if we want
that at some point. Alternatively we could add a pgsocket* argument, but
that doesn't really seem much better.

Not super happy about the above proposal.

Andres


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Tom Lane]

Andres Freund  writes:
> This brings me to something related: I'm wondering if we shouldn't merge
> unix/win32_latch.c.

Well, it's duplicated code on the one hand versus maze-of-ifdefs on the
other.  Feel free to try it and see, but I'm unsure it'd be an improvement.

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] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-01-14 18:14:21 +0100, Andres Freund wrote:
> I'm thinking of something like;
> 
> int WaitOnLatchSet(LatchEventSet *set, int wakeEvents, long timeout);
> 
> int
> WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,long 
> timeout)
> {
> LatchEventSet set;
> 
> LatchEventSetInit(&set, latch);
> 
> if (sock != PGINVALID_SOCKET)
>LatchEventSetAddSock(&set, sock);
> 
> return WaitOnLatchSet(set, wakeEvents, timeout);
> }
> 
> I think we'll need to continue having wakeEvents and timeout parameters
> for WaitOnLatchSet, we quite frequently want to wait socket
> readability/writability, not wait on the socket, or have/not have
> timeouts.

This brings me to something related: I'm wondering if we shouldn't merge
unix/win32_latch.c. If we go this route it seems like the amount of
shared infrastructure will further increase. The difference between
win32 and, say, the select code isn't much bigger than the difference
between select/poll. epoll/win32 are probably more similar than that
actually.

Andres


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-01-14 12:07:23 -0500, Robert Haas wrote:
> > Do we want to provide a backward compatible API for all this? I'm fine
> > either way.
>
> How would that work?

I'm thinking of something like;

int WaitOnLatchSet(LatchEventSet *set, int wakeEvents, long timeout);

int
WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,long 
timeout)
{
LatchEventSet set;

LatchEventSetInit(&set, latch);

if (sock != PGINVALID_SOCKET)
   LatchEventSetAddSock(&set, sock);

return WaitOnLatchSet(set, wakeEvents, timeout);
}

I think we'll need to continue having wakeEvents and timeout parameters
for WaitOnLatchSet, we quite frequently want to wait socket
readability/writability, not wait on the socket, or have/not have
timeouts.



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

Re: [HACKERS] SET syntax in INSERT

[Marko Tiikkaja]

Hi,

SET syntax for INSERT was brought up a few years ago here: 
http://www.postgresql.org/message-id/2c5ef4e30908251010s46d9d566m1da21357891ba...@mail.gmail.com


From the discussion it seems that one committer was against, one 
committer was not against, and one committer saw something good in the 
proposal.  Personally, I believe this would be a huge readability 
improvement to INSERTs with more than a few columns.  I'm willing to put 
in some work to make this happen for 9.7, but I'd like to know that I'm 
not wasting my time.


What do we think?


.m


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 14, 2016 at 12:06 PM, Andres Freund  wrote:
>> Do we want to provide a backward compatible API for all this? I'm fine
>> either way.

> How would that work?

I see no great need to be backwards-compatible on this, especially if it
would complicate matters at all.  I doubt there's a lot of third-party
code using WaitLatch right now.  Just make sure there's an obvious
compile failure for anyone who is.

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] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Jan 14, 2016 at 12:06 PM, Andres Freund  wrote:
> On 2016-01-14 11:31:03 -0500, Robert Haas wrote:
>> On Thu, Jan 14, 2016 at 10:56 AM, Andres Freund  wrote:
>> I think your idea of a data structure the encapsulates a set of events
>> for which to wait is probably a good one.  WaitLatch doesn't seem like
>> a great name.  Maybe WaitEventSet, and then we can have
>> WaitLatch(&latch) and WaitEvents(&eventset).
>
> Hm, I'd like to have latch in the name. It seems far from improbably to
> have another wait data structure. LatchEventSet maybe? The wait would be
> implied by WaitLatch.

I can live with that.

> So effectively we'd create a LatchEventSet feLatchSet; somewhere global
> (and update it from a backend local to the proc latch in
> SwitchToSharedLatch/SwitchBackToLocalLatch()). Then change all WaitLatch
> calls to refer to those.

Sure.

> Do we want to provide a backward compatible API for all this? I'm fine
> either way.

How would that work?

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-01-14 11:31:03 -0500, Robert Haas wrote:
> On Thu, Jan 14, 2016 at 10:56 AM, Andres Freund  wrote:
> I think your idea of a data structure the encapsulates a set of events
> for which to wait is probably a good one.  WaitLatch doesn't seem like
> a great name.  Maybe WaitEventSet, and then we can have
> WaitLatch(&latch) and WaitEvents(&eventset).

Hm, I'd like to have latch in the name. It seems far from improbably to
have another wait data structure. LatchEventSet maybe? The wait would be
implied by WaitLatch.

So effectively we'd create a LatchEventSet feLatchSet; somewhere global
(and update it from a backend local to the proc latch in
SwitchToSharedLatch/SwitchBackToLocalLatch()). Then change all WaitLatch
calls to refer to those.

Do we want to provide a backward compatible API for all this? I'm fine
either way.

Andres


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

Re: [HACKERS] tiny doc patch

[Robert Haas]

On Thu, Jan 14, 2016 at 8:40 AM, Filip Rembiałkowski
 wrote:
> (include RLS option in CREATE USER doc)
>
> should go into HEAD and REL9_5_STABLE

Doesn't ALTER USER need the same fix?

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


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

Re: [HACKERS] Python 3.x versus PG 9.1 branch

[Robert Haas]

On Wed, Jan 13, 2016 at 11:46 AM, Tom Lane  wrote:
> Or we could just blow it off on the grounds that 9.1 is not long
> for this world anyhow.

+1 for blowing it off.  I can't see the point in putting effort into
this.  Nobody should be spinning up new PostgreSQL 9.1 deployments at
this point, and whatever PostgreSQL 9.1 deployments already exist are
evidently OK with what we've been doing up until now.  So it seems
unlikely to help anyone.

Also, if it does help someone, it will be helping them to deploy a
nearly-obsolete PostgreSQL version at a time when, really, it would be
much better if they were thinking about how to get off that version.

Moreover, it's not inconceivable that back-porting all of those
commits could break something that works now, in which event we would
end up worse off than we are today.

So I really don't see any upside.

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


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

Re: [HACKERS] pgindent-polluted commits

[Robert Haas]

On Thu, Jan 14, 2016 at 11:25 AM, Andrew Dunstan  wrote:
> I do think it makes life easier when going through the git history if
> semantic changes are separated from formatting changes.

I agree.  And I agree with Mark Dilger's point, too.

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Jan 14, 2016 at 10:56 AM, Andres Freund  wrote:
> So, I'm wondering how we'd exactly use a hyptothetical
> SetSocketToWaitOn, or SetSocketsToWaitOn (or whatever). I mean it can
> make a fair bit of sense to sometimes wait on MyLatch/port->sock and
> sometimes on MyLatch/fdw connections. The simple proposed code would
> change the epoll set whenever switching between both, but with
> SetSocketsToWaitOn you'd probably end up switching this much more often?
>
> One way to address that would be to create a 'latch wait' datastructure,
> that'd then contain the epoll fd/win32 wait events/... That way you
> could have one 'LatchWait' for latch + client socket and one for latch +
> fdw sockets.

I see your point.  As far as I can see, it's currently true that,
right now, the only places where we wait for a socket are places where
the socket will live for the lifetime of the backend, but I think we
should regard it as likely that, in the future, we'll want to use it
anywhere we want to wait for a socket to become ready.  There are
getting to be a lot of places where we need to unstick some loop
whenever the process latch gets set, and it seems likely to me that
needs will only continue to grow.  So the API should probably
contemplate that sort of need.

I think your idea of a data structure the encapsulates a set of events
for which to wait is probably a good one.  WaitLatch doesn't seem like
a great name.  Maybe WaitEventSet, and then we can have
WaitLatch(&latch) and WaitEvents(&eventset).

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On January 14, 2016 5:16:59 PM GMT+01:00, Robert Haas  
wrote:
>Yeah.  Although I think for now it would be fine to just error out if
>somebody tries to add a socket and there already is one.  Then we
>could lift that limitation in a later commit.  Of course if Andres
>wants to do the whole thing now I'm not going to get in the way, but
>since that will require Windows tinkering and so on it may be more
>than he wants to dive into.

Yea, I don't want to do anything really large at the moment. My primary 
interest is fixing the major performance regression.

Andres


--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


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

Re: [HACKERS] pgindent-polluted commits

[Andrew Dunstan]

On 01/13/2016 12:13 PM, Tom Lane wrote:

Simon Riggs  writes:

On 13 January 2016 at 14:48, Noah Misch  wrote:

I've noticed commits, from a few of you, carrying pgindent changes to lines
the patch would not otherwise change.

Could we review again why this matters?

Basically this is trading off convenience of the committer (all of the
alternatives Noah mentions are somewhat annoying) versus the convenience
of post-commit reviewers.  I'm not sure that his recommendation is the
best trade-off, nor that the situation is precisely comparable to
pre-commit review.  There definitely will be pre-commit review, there
may or may not be any post-commit review.

I'm willing to go with the "separate commit to reindent individual files"
approach if there's a consensus that that makes for a cleaner git history.
But I'm not 100% convinced it matters.






I do think it makes life easier when going through the git history if 
semantic changes are separated from formatting changes.


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] proposal: PL/Pythonu - function ereport

[Catalin Iacob]

On Wed, Jan 13, 2016 at 7:40 PM, Jim Nasby  wrote:
> On 1/12/16 11:25 AM, Catalin Iacob wrote:
>> They're similar but not really the same thing. raise Error and
>> plpy.error are both ways to call ereport(ERROR, ...) while SPIError is
>> raised when coming back after calling into Postgres to execute SQL
>> that itself raises an error. Now indeed, that executed SQL raised an
>> error itself via another ereport(ERROR, ...) somewhere but that's a
>> different thing.
>
>
> Why should they be different? An error is an error. You either want to trap
> a specific type of error or you don't. Having two completely different ways
> to do the same thing is just confusing.

With my (indeed limited) understanding, I don't agree they're the same
thing and I tried to explain above why I think they're quite
different. You may not agree. If they are different things, using
different exception types is natural.

Consider this call chain: SQL code 1 -> PL/Python code 1 -> SPI (via
plpy.execute) -> SQL code 2 -> PL/Python code 2

If I'm writing PL/Python code 1 and I want to raise an error toward
SQL code 1 I use raise plpy.Error. plpy.SPIError is what I get if I
call into SQL code 2 and that has an error. That error could indeed
come from a plpy.Error thrown by PL/Python code 2 or it could come
from a SQL syntax error or whatever. But the symmetry holds:
plpy.Error is what you raise to stop the query and return errors to
your SQL caller and plpy.SPIError is what you get back if you use SPI
to execute some other piece of SQL which has an error. I *think* (I'm
an internals newbie though so I could be wrong) that SQL code 1
doesn't call into PL/Python code 1 via SPI so why would the latter use
something called SPIError to inform the former about an error?


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Jan 14, 2016 at 10:46 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> Incidentally, if we're going to whack around the latch API, it would
>> be nice to pick a design which wouldn't be too hard to extend to
>> waiting on multiple sockets.  The application I have in mind is to
>> send of queries to several foreign servers at once and then wait until
>> bytes come back from any of them.  It's mostly pie in the sky at this
>> point, but it seems highly likely to me that we'd want to do such a
>> thing by waiting for bytes from any of the sockets involved OR a latch
>> event.
>
> Instead of SetSocketToWaitOn, maybe AddSocketToWaitSet and
> RemoveSocketFromWaitSet?  And you'd need some way of identifying
> which socket came ready after a wait call...

Yeah.  Although I think for now it would be fine to just error out if
somebody tries to add a socket and there already is one.  Then we
could lift that limitation in a later commit.  Of course if Andres
wants to do the whole thing now I'm not going to get in the way, but
since that will require Windows tinkering and so on it may be more
than he wants to dive into.

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-01-14 10:39:55 -0500, Robert Haas wrote:
> Doesn't this code make it possible for the self-pipe to fill up,
> self-deadlocking the process?  Suppose we repeatedly enter
> WaitLatchOrSocket().  Each time we do, just after waiting = true is
> set, somebody sets the latch.  We handle the signal and put a byte
> into the pipe.  Returning from the signal handler, we then notice that
> is_set is true and return at once, without draining the pipe.  Repeat
> until something bad happens.

Should be fine because the self-pipe is marked as non-blocking
if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) < 0)
elog(FATAL, "fcntl() failed on write-end of self-pipe: %m");
and sendSelfPipeByte accepts the blocking case as success

/*
 * If the pipe is full, we don't need to retry, the data that's 
there
 * already is enough to wake up WaitLatch.
 */
if (errno == EAGAIN || errno == EWOULDBLOCK)
return;


> > 0004: Support using epoll as the polling primitive in unix_latch.c.
> >   ~3% on high qps workloads, massive scalability improvements (x3)
> >   on very large machines.
> >
> > With 0004 obviously being the relevant bit for this thread. I verified
> > that using epoll addresses the performance problem, using the hardware
> > the OP noticed the performance problem on.
> 
> +/*
> + * Unnecessarily pass data for delete due to bug errorneously
> + * requiring it in the past.
> + */
> 
> This is pretty vague.  And it has a spelling mistake.

Will add a reference to the manpage (where that requirement is coming
from).

> Further down, nodified -> notified.
> 
> +if (wakeEvents != latch->lastmask || latch->lastwatchfd != sock)
> 
> I don't like this very much.

Yea, me neither, which is why I called it out... I think it's not too
likely to cause problems in practice though. But I think changing the
API makes sense, so the likelihood shouldn't be a relevant issue.

> Incidentally, if we're going to whack around the latch API, it would
> be nice to pick a design which wouldn't be too hard to extend to
> waiting on multiple sockets.

Hm. That seems likely to make usage harder for users of the API. So it
seems like it'd make sense to provide a simpler version anyway, for the
majority of users.

So, I'm wondering how we'd exactly use a hyptothetical
SetSocketToWaitOn, or SetSocketsToWaitOn (or whatever). I mean it can
make a fair bit of sense to sometimes wait on MyLatch/port->sock and
sometimes on MyLatch/fdw connections. The simple proposed code would
change the epoll set whenever switching between both, but with
SetSocketsToWaitOn you'd probably end up switching this much more often?

One way to address that would be to create a 'latch wait' datastructure,
that'd then contain the epoll fd/win32 wait events/... That way you
could have one 'LatchWait' for latch + client socket and one for latch +
fdw sockets.

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] Performance degradation in commit ac1d794

[Tom Lane]

Robert Haas  writes:
> Incidentally, if we're going to whack around the latch API, it would
> be nice to pick a design which wouldn't be too hard to extend to
> waiting on multiple sockets.  The application I have in mind is to
> send of queries to several foreign servers at once and then wait until
> bytes come back from any of them.  It's mostly pie in the sky at this
> point, but it seems highly likely to me that we'd want to do such a
> thing by waiting for bytes from any of the sockets involved OR a latch
> event.

Instead of SetSocketToWaitOn, maybe AddSocketToWaitSet and
RemoveSocketFromWaitSet?  And you'd need some way of identifying
which socket came ready after a wait call...

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] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Jan 14, 2016 at 9:39 AM, Andres Freund  wrote:
> I finally got back to working on this. Attached is a WIP patch series
> implementing:
> 0001: Allow to easily choose between the readiness primitives in unix_latch.c
>   Pretty helpful for testing, not useful for anything else.

Looks good.

> 0002: Error out if waiting on socket readiness without a specified socket.

Looks good.

> 0003: Only clear unix_latch.c's self-pipe if it actually contains data.
>   ~2% on high qps workloads

everytime -> every time

+if ((wakeEvents & WL_LATCH_SET) && latch->is_set)
+{
+result |= WL_LATCH_SET;
+}

Excess braces.

Doesn't this code make it possible for the self-pipe to fill up,
self-deadlocking the process?  Suppose we repeatedly enter
WaitLatchOrSocket().  Each time we do, just after waiting = true is
set, somebody sets the latch.  We handle the signal and put a byte
into the pipe.  Returning from the signal handler, we then notice that
is_set is true and return at once, without draining the pipe.  Repeat
until something bad happens.

> 0004: Support using epoll as the polling primitive in unix_latch.c.
>   ~3% on high qps workloads, massive scalability improvements (x3)
>   on very large machines.
>
> With 0004 obviously being the relevant bit for this thread. I verified
> that using epoll addresses the performance problem, using the hardware
> the OP noticed the performance problem on.

+/*
+ * Unnecessarily pass data for delete due to bug errorneously
+ * requiring it in the past.
+ */

This is pretty vague.  And it has a spelling mistake.

Further down, nodified -> notified.

+if (wakeEvents != latch->lastmask || latch->lastwatchfd != sock)

I don't like this very much.  I think it's a bad idea to test
latch->lastwatchfd != sock.  That has an excellent change of letting
people write code that appears to work but then doesn't.  I think it
would be better, if we're going to change the API contract, to make it
a hard break, as I see Tom has also suggested while I've been writing
this.

Incidentally, if we're going to whack around the latch API, it would
be nice to pick a design which wouldn't be too hard to extend to
waiting on multiple sockets.  The application I have in mind is to
send of queries to several foreign servers at once and then wait until
bytes come back from any of them.  It's mostly pie in the sky at this
point, but it seems highly likely to me that we'd want to do such a
thing by waiting for bytes from any of the sockets involved OR a latch
event.

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Tom Lane]

Andres Freund  writes:
> 0004 currently contains one debatable optimization, which I'd like to
> discuss: Currently the 'sock' passed to WaitLatchOrSocket is not
> removed/added to the epoll fd, if it's the numerically same as in the
> last call. That's good for performance, but would be wrong if the socket
> were close and a new one with the same value would be waited on.  I
> think a big warning sign somewhere is sufficient to deal with that
> problem - it's not something we're likely to start doing. And even if
> it's done at some point, we can just offer an API to reset the last used
> socket fd.

Perhaps a cleaner API solution would be to remove the socket argument per
se from the function altogether, instead providing a separate
SetSocketToWaitOn() call.

(Also, if there is a need for it, we could provide a function that still
takes a socket argument, with the understanding that it's to be used for
short-lived sockets where you don't want to change the process's main
epoll state.)

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] Performance degradation in commit ac1d794

[Andres Freund]

On 2015-12-26 12:22:48 +0100, Andres Freund wrote:
> On 2015-12-25 16:29:53 -0500, Tom Lane wrote:
> > Andres Freund  writes:
> > > There's a couple solutions I can think of to that problem:
> > > 1) Use epoll()/kqueue, or other similar interfaces that don't require
> > >re-registering fds at every invocation. My guess is that that'd be
> > >desirable for performance anyway.
> >
> > Portability, on the other hand, would be problematic.
>
> Indeed. But we might be able to get away with it because there's
> realistically just one platform on which people run four socket
> servers. Obviously we'd leave poll and select support in place.  It'd be
> a genuine improvement for less extreme loads on linux, too.

I finally got back to working on this. Attached is a WIP patch series
implementing:
0001: Allow to easily choose between the readiness primitives in unix_latch.c
  Pretty helpful for testing, not useful for anything else.
0002: Error out if waiting on socket readiness without a specified socket.
0003: Only clear unix_latch.c's self-pipe if it actually contains data.
  ~2% on high qps workloads
0004: Support using epoll as the polling primitive in unix_latch.c.
  ~3% on high qps workloads, massive scalability improvements (x3)
  on very large machines.

With 0004 obviously being the relevant bit for this thread. I verified
that using epoll addresses the performance problem, using the hardware
the OP noticed the performance problem on.

The reason I went with using epoll over the PR_SET_PDEATHSIG approach is
that it provides semantics that are more similar to the other platforms,
while being just as platform dependant as PR_SET_PDEATHSIG. It also is
actually measurably faster, at least here.

0004 currently contains one debatable optimization, which I'd like to
discuss: Currently the 'sock' passed to WaitLatchOrSocket is not
removed/added to the epoll fd, if it's the numerically same as in the
last call. That's good for performance, but would be wrong if the socket
were close and a new one with the same value would be waited on.  I
think a big warning sign somewhere is sufficient to deal with that
problem - it's not something we're likely to start doing. And even if
it's done at some point, we can just offer an API to reset the last used
socket fd.


Unless somebody comes up with a platform independent way of addressing
this, I'm inclined to press forward using epoll(). Opinions?

Andres
>From fb67ecf2f6f65525af1ed7c5d5e5dd46e8fa6fc4 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 14 Jan 2016 14:17:43 +0100
Subject: [PATCH 1/4] Make it easier to choose the used waiting primitive in
 unix_latch.c.

---
 src/backend/port/unix_latch.c | 50 +--
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/src/backend/port/unix_latch.c b/src/backend/port/unix_latch.c
index 2ad609c..f52704b 100644
--- a/src/backend/port/unix_latch.c
+++ b/src/backend/port/unix_latch.c
@@ -56,6 +56,22 @@
 #include "storage/pmsignal.h"
 #include "storage/shmem.h"
 
+/*
+ * Select the fd readiness primitive to use. Normally the "most modern"
+ * primitive supported by the OS will be used, but for testing it can be
+ * useful to manually specify the used primitive.  If desired, just add a
+ * define somewhere before this block.
+ */
+#if defined(LATCH_USE_POLL) || defined(LATCH_USE_SELECT)
+/* don't overwrite manual choice */
+#elif defined(HAVE_POLL)
+#define LATCH_USE_POLL
+#elif HAVE_SYS_SELECT_H
+#define LATCH_USE_SELECT
+#else
+#error "no latch implementation available"
+#endif
+
 /* Are we currently in WaitLatch? The signal handler would like to know. */
 static volatile sig_atomic_t waiting = false;
 
@@ -215,10 +231,10 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
 cur_time;
 	long		cur_timeout;
 
-#ifdef HAVE_POLL
+#if defined(LATCH_USE_POLL)
 	struct pollfd pfds[3];
 	int			nfds;
-#else
+#elif defined(LATCH_USE_SELECT)
 	struct timeval tv,
 			   *tvp;
 	fd_set		input_mask;
@@ -247,7 +263,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
 		Assert(timeout >= 0 && timeout <= INT_MAX);
 		cur_timeout = timeout;
 
-#ifndef HAVE_POLL
+#ifdef LATCH_USE_SELECT
 		tv.tv_sec = cur_timeout / 1000L;
 		tv.tv_usec = (cur_timeout % 1000L) * 1000L;
 		tvp = &tv;
@@ -257,7 +273,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
 	{
 		cur_timeout = -1;
 
-#ifndef HAVE_POLL
+#ifdef LATCH_USE_SELECT
 		tvp = NULL;
 #endif
 	}
@@ -291,16 +307,10 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
 		}
 
 		/*
-		 * Must wait ... we use poll(2) if available, otherwise select(2).
-		 *
-		 * On at least older linux kernels select(), in violation of POSIX,
-		 * doesn't reliably return a socket as writable if closed - but we
-		 * rely on that. So far all the known cases of this problem are on
-		 * platforms that also provide a poll() implementation without that

Re: [HACKERS] About get_relation_constraints and include_notnull

[Tom Lane]

Amit Langote  writes:
> Why does the argument include_notnull argument exist if
> get_relation_constraints() is being called from only one place? Perhaps we
> could remove it and add the IS NOT NULL test expression unconditionally if
> there are any NOT NULL columns.

Well, you could argue why have a routine at all instead of inlining it
into the one caller.  IIRC the thought was that other likely uses of
constraint-fetching might want to see only the actual check constraints.
Is there some positive benefit from removing the argument?

regards, tom lane


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

Re: [HACKERS] PATCH: Extending the HyperLogLog API a bit

[Robert Haas]

On Mon, Jan 11, 2016 at 2:22 PM, Alvaro Herrera
 wrote:
> Tomas Vondra wrote:
>> Attached is v2 of the patch, adding the comments.
>
> Looks pretty reasonable to me.  I'm not sure we want to push this ahead
> of the bloom filter stuff, but it looks ready to commit otherwise.

I'd say go ahead and push it.

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


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

Re: [HACKERS] Removing Functionally Dependent GROUP BY Columns

[Julien Rouhaud]

On 14/01/2016 14:29, Geoff Winkless wrote:
> On 14 January 2016 at 13:16, Julien Rouhaud  wrote:
>> You're absolutely right, but in this case the comment is more like a
>> reminder of a bigger comment few lines before that wasn't quoted in my mail
> 
> Fair enough, although I have two niggles with that:
> 
> a) the second comment could become physically separated from the first
> by later additions of extra code, or by refactoring;
> b) if you don't need the comment because the explanation for it is
> local anyway and the comment tells you nothing that the code doesn't,
> why have it at all?
> 

I agree. If I had to choose, I'd vote for removing it.

>> so I assume it's ok to keep it this way.
> 
> Of course it's ok to do whatever you decide is best: as I said
> previously, I fully appreciate that I have no ownership over any of
> the code.
> 

Neither do I, I'm just reviewer here just as you :)

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

[HACKERS] tiny doc patch

[Filip Rembiałkowski]

(include RLS option in CREATE USER doc)

should go into HEAD and REL9_5_STABLE
diff --git a/doc/src/sgml/ref/create_user.sgml b/doc/src/sgml/ref/create_user.sgml
index 6c690b3..574604f 100644
--- a/doc/src/sgml/ref/create_user.sgml
+++ b/doc/src/sgml/ref/create_user.sgml
@@ -31,6 +31,7 @@ CREATE USER name [ [ WITH ] connlimit
 | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password'
 | VALID UNTIL 'timestamp'

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

Re: [HACKERS] Removing Functionally Dependent GROUP BY Columns

[Geoff Winkless]

On 14 January 2016 at 13:16, Julien Rouhaud  wrote:
> You're absolutely right, but in this case the comment is more like a
> reminder of a bigger comment few lines before that wasn't quoted in my mail

Fair enough, although I have two niggles with that:

a) the second comment could become physically separated from the first
by later additions of extra code, or by refactoring;
b) if you don't need the comment because the explanation for it is
local anyway and the comment tells you nothing that the code doesn't,
why have it at all?

> so I assume it's ok to keep it this way.

Of course it's ok to do whatever you decide is best: as I said
previously, I fully appreciate that I have no ownership over any of
the code.

Geoff


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

Re: [HACKERS] Removing Functionally Dependent GROUP BY Columns

[Julien Rouhaud]

On 14/01/2016 14:04, Geoff Winkless wrote:
> On 14 January 2016 at 11:19, Julien Rouhaud  wrote:
>> +   /* don't try anything unless there's two Vars */
>> +   if (varlist == NULL || list_length(varlist) < 2)
>> +   continue;
>>
>> To be perfectly correct, the comment should say "at least two Vars".
> 
> Apologies for butting in and I appreciate I don't have any ownership
> over this codebase or right to suggest any changes, but this just
> caught my eye before I could hit "delete".
> 
> My mantra tends to be "why, not what" for inline comments; in this
> case you can get the same information from the next line of code as
> you get from the comment.
> 

You're absolutely right, but in this case the comment is more like a
reminder of a bigger comment few lines before that wasn't quoted in my mail:

+*[...] If there are no Vars then
+* nothing need be done for this rel. If there's less than two Vars then
+* there's no room to remove any, as the PRIMARY KEY must have at
least one
+* Var, so we can safely also do nothing if there's less than two Vars.


so I assume it's ok to keep it this way.


> Perhaps something like
> 
> /* it's clearly impossible to remove duplicates if there are fewer
> than two GROUPBY columns */
> 
> might be more helpful?
> 
> (also sorry if I've misunderstood what it _actually_ does, I just made
> an assumption based on reading this thread!)
> 


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

Re: [HACKERS] Removing Functionally Dependent GROUP BY Columns

[Geoff Winkless]

On 14 January 2016 at 11:19, Julien Rouhaud  wrote:
> +   /* don't try anything unless there's two Vars */
> +   if (varlist == NULL || list_length(varlist) < 2)
> +   continue;
>
> To be perfectly correct, the comment should say "at least two Vars".

Apologies for butting in and I appreciate I don't have any ownership
over this codebase or right to suggest any changes, but this just
caught my eye before I could hit "delete".

My mantra tends to be "why, not what" for inline comments; in this
case you can get the same information from the next line of code as
you get from the comment.

Perhaps something like

/* it's clearly impossible to remove duplicates if there are fewer
than two GROUPBY columns */

might be more helpful?

(also sorry if I've misunderstood what it _actually_ does, I just made
an assumption based on reading this thread!)

Geoff


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

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Rushabh Lathia]

On Thu, Jan 14, 2016 at 2:00 PM, Etsuro Fujita 
wrote:

> On 2016/01/12 20:31, Rushabh Lathia wrote:
>
>> On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita
>> mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
>> On 2016/01/06 18:58, Rushabh Lathia wrote:
>> .) What the need of following change ?
>>
>> @@ -833,9 +833,6 @@ appendWhereClause(StringInfo buf,
>>   int nestlevel;
>>   ListCell   *lc;
>>
>> -   if (params)
>> -   *params = NIL;  /* initialize result list to
>> empty */
>> -
>>   /* Set up context struct for recursion */
>>   context.root = root;
>>   context.foreignrel = baserel;
>> @@ -971,6 +968,63 @@ deparseUpdateSql(StringInfo buf,
>> PlannerInfo *root,
>>}
>>
>
> It is needed for deparsePushedDownUpdateSql to store params in both
>> WHERE clauses and expressions to assign to the target columns
>> into one params_list list.
>>
>
> Hmm sorry but I am still not getting the point, can you provide some
>> example to explain this ?
>>
>
> Sorry, maybe my explanation was not enough.  Consider:
>
> postgres=# create foreign table ft1 (a int, b int) server myserver options
> (table_name 't1');
> postgres=# insert into ft1 values (0, 0);
> postgres=# prepare mt(int, int) as update ft1 set a = $1 where b = $2;
> postgres=# explain verbose execute mt(1, 0);
> postgres=# explain verbose execute mt(1, 0);
> postgres=# explain verbose execute mt(1, 0);
> postgres=# explain verbose execute mt(1, 0);
> postgres=# explain verbose execute mt(1, 0);
>
> After the 5 executions of mt we have
>
> postgres=# explain verbose execute mt(1, 0);
>  QUERY PLAN
>
> 
>  Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
>->  Foreign Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
>  Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b =
> $2::integer))
> (3 rows)
>
> If we do that initialization in appendWhereClause, we would get a wrong
> params_list list and a wrong remote pushed-down query for the last mt() in
> deparsePushedDownUpdateSql.
>

Strange, I am seeing same behaviour with or without that initialization in
appendWhereClause. After the 5 executions of mt I with or without I am
getting following output:

postgres=# explain verbose execute mt(1, 0);
 QUERY
PLAN

 Update on public.ft2  (cost=100.00..140.35 rows=12 width=10)
   ->  Foreign Update on public.ft2  (cost=100.00..140.35 rows=12 width=10)
 Remote SQL: UPDATE public.t2 SET a = $1::integer WHERE ((b =
$2::integer))
(3 rows)



>
> .) When Tom Lane and Stephen Frost suggested getting the core
>> code involved,
>> I thought that we can do the mandatory checks into core it self
>> and making
>> completely out of dml_is_pushdown_safe(). Please correct me
>>
>
> The reason why I put that function in postgres_fdw.c is Check point 4:
>>
>> +  * 4. We can't push an UPDATE down, if any expressions to assign
>> to the target
>> +  * columns are unsafe to evaluate on the remote server.
>>
>
> Here I was talking about checks related to triggers, or to LIMIT. I think
>> earlier thread talked about those mandatory check to the core. So may
>> be we can move those checks into make_modifytable() before calling
>> the PlanDMLPushdown.
>>
>
> Noticed that.  Will do.
>
> BTW, I keep a ForeignScan node pushing down an update to the remote
> server, in the updated patches.  I have to admit that that seems like
> rather a misnomer.  So, it might be worth adding a new ForeignUpdate node,
> but my concern about that is that if doing so, we would have a lot of
> duplicate code in ForeignUpdate and ForeignScan.  What do you think about
> that?
>
>
Yes, I noticed that in the patch and I was about to point that out in my
final review. As first review I was mainly focused on the functionality
testing
and other overview things. Another reason I haven't posted that in my
first review round is, I was not quite sure whether we need the
separate new node ForeignUpdate, ForeignDelete  and want to duplicate
code? Was also not quite sure about the fact that what we will achieve
by doing that.

So I thought, I will have this open question in my final review comment,
and will take committer's opinion on this. Since you already raised this
question lets take others opinion on this.

Regards,



-- 
Rushabh Lathia
www.EnterpriseDB.come

Re: [HACKERS] Close handle leak in SSPI auth

[Magnus Hagander]

On Sun, Jan 10, 2016 at 8:58 PM, Christian Ullrich 
wrote:

> Hello,
>
> here's a one-line patch to close a handle leak in pg_SSPI_recvauth().
>
> According to the docs, the token retrieved with
> QuerySecurityContextToken() must be closed.


We still leak it in a number of the error paths in this case, but I don't
think we need to care about those -- since they are hard errors, the
process exits shortly thereafter anyway.

Applied and backpatched, thanks.

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

Re: [HACKERS] Removing Functionally Dependent GROUP BY Columns

[Julien Rouhaud]

On 14/01/2016 01:30, David Rowley wrote:
> Many thanks for the thorough review!
> 

And thanks for the patch and fast answer!

> On 12 January 2016 at 23:37, Julien Rouhaud  > wrote:
> 
> 
> In identify_key_vars()
> 
> +   /* Only PK constraints are of interest for now, see comment
> above */
> +   if (con->contype != CONSTRAINT_PRIMARY)
> +   continue;
> 
> I think the comment should better refer to
> remove_useless_groupby_columns() (don't optimize further than what
> check_functional_grouping() does).
> 
> 
> I've improved this by explaining it more clearly.
>  

Very nice.

Small typo though:


+* Technically we could look at UNIQUE indexes too, however 
we'd also
+* have to ensure that each column of the unique index had a 
NOT NULL


s/had/has/


+* constraint, however since NOT NULL constraints currently are 
don't


s/are //

> 
> [...]
> 
> 
> +   {
> +   varlistmatches = bms_add_member(varlistmatches,
> varlistidx);
> +   found_col = true;
> +   /* don't break, there might be dupicates */
> +   }
> 
> small typo on "dupicates". Also, I thought transformGroupClauseExpr()
> called in the parse analysis make sure that there isn't any duplicate in
> the GROUP BY clause. Do I miss something?
> 
> 
> No I missed something :) You are right. I've put a break; here and a
> comment to explain why.
> 

ok :)


> [...]
> 
> 
> +   /*
> +* If we found any surplus Vars in the GROUP BY clause, then
> we'll build
> +* a new GROUP BY clause without these surplus Vars.
> +*/
> +   if (anysurplus)
> +   {
> +   List *new_groupby = NIL;
> +
> +   foreach (lc, root->parse->groupClause)
> +   {
> +   SortGroupClause *sgc = (SortGroupClause *) lfirst(lc);
> +   TargetEntry *tle;
> +   Var *var;
> +
> +   tle = get_sortgroupclause_tle(sgc, root->parse->targetList);
> +   var = (Var *) tle->expr;
> +
> +   if (!IsA(var, Var))
> +   continue;
> + [...]
> 
> if var isn't a Var, it needs to be added in new_groupby.
> 
> 
> Yes you're right, well, at least I've written enough code to ensure that
> it's not needed.

Oh yes, I realize that now.

> Technically we could look inside non-Vars and check if the Expr is just
> made up of Vars from a single relation, and then we could also make that
> surplus if we find other Vars which make up the table's primary key.  I
> didn't make these changes as I thought it was a less likely scenario. It
> wouldn't be too much extra code to add however. I've went and added an
> XXX comment to explain that there might be future optimisation
> opportunities in the future around this. 
>  

Agreed.

> I've attached an updated patch.
> 

+   /* don't try anything unless there's two Vars */
+   if (varlist == NULL || list_length(varlist) < 2)
+   continue;

To be perfectly correct, the comment should say "at least two Vars".

Except the small remaining typos, this patch looks very fine to me. I
flag it as ready for committer.

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

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Marco Atzeri]

On 14/01/2016 06:38, Michael Paquier wrote:

Hi all,

Beginning a new thread seems more adapted regarding $subject but
that's mentioned here as well:
http://www.postgresql.org/message-id/CAB7nPqQXghm_SdB5iniupz1atzMxk=95gv9a8ocdo83sxcn...@mail.gmail.com

It happens based on some investigation from Andrew that cygwin does
not need to use the service-related code, aka register/unregister
options or similar that are proper to WIN32 and rely instead on
cygrunsrv with a SYSV-like init file to manage the service. Based on
my tests with cygwin, I am able to see as well that a server started
within a cygwin session is able to persist even after it is closed,
which is kind of nice and I think that it is a additional reason to
not use the service-related code paths. Hence what about the following
patch, that makes cygwin behave like any *nix OS when using pg_ctl?
This simplifies a bit the code.

Marco, as I think you do some packaging for Postgres in Cygwin, and
others, does that sound acceptable?

Regards,



In general cygwin to behave like any *nix OS is the preferred solution.


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

Re: [HACKERS] checkpointer continuous flushing

[Fabien COELHO]

Hello Andres,


Argh! This is a key point: the sort/flush is designed to help HDDs, and
would have limited effect on SSDs, and it seems that you are showing that
the effect is in fact negative on SSDs, too bad:-(


As you quoted, I could reproduce the slowdown both with SSDs *and* with
rotating disks.


Ok, once again I misunderstood. So you have a regression on HDD with the 
settings you pointed out, I can try that.



On SSDs, the linux IO scheduler works quite well, so this is a place where I
would consider simply disactivating flushing and/or sorting.


Not my experience. In different scenarios, primarily with a large
shared_buffers fitting the whole hot working set, the patch
significantly improves performance.


Good! That would be what I expected, but I have no way to test that.


postgres-ckpt14 \
  -D /srv/temp/pgdev-dev-800/ \
  -c maintenance_work_mem=2GB \
  -c fsync=on \
  -c synchronous_commit=off \


I'm not sure I like this one. I guess the intention is to focus on
checkpointer writes and reduce the impact of WAL writes. Why not.


Now sure what you mean? s_c = off is *very* frequent in the field.


Too bad, because for me it is really disactivating the D of ACID...

I think that this setting would not issue the "sync" calls on the WAL 
file, which means that the impact of WAL writing is somehow reduced and 
random writes (more or less for each transaction) is switched to 
sequential writes by the IO scheduler.



My laptop 1 EVO 840, 1 i7-4800MQ, 16GB ram:
master:
scaling factor: 800


The DB is probably about 12GB, so it fits in memory in the end, meaning that
there should be only write activity after some time? So this is not really
the case where it does not fit in memory, but it is large enough to get
mostly random IOs both in read & write, so why not.


Doesn't really fit into ram - shared buffers uses some space (which will
be double buffered) and the xlog will use some more.


Hmmm. My understanding is that you are really using about 6GB of shared 
buffer data in a run, plus some write only stuff...


xlog is flush/synced constantly and never read again, I would be surprise 
that it has a significant memory impact.



ckpt-14 (flushing by backends disabled):


Is this comment refering to "synchronous_commit = off"?
I guess this is the same on master above, even if not written?


No, what I mean by that is that I didn't active flushing writes in
backends -


I'm not sure that I understand. What is the actual corresponding directive 
in the configuration file?



As you can see there's roughly a 30% performance regression on the
slower SSD and a ~9% on the faster one. HDD results are similar (but I
can't repeat on the laptop right now since the 2nd hdd is now an SSD).


Ok, that is what I would have expected, the larger the database, the smaller
the impact of sorting & flushin on SSDs.


Again: "HDD results are similar". I primarily tested on a 4 disk raid10
of 4 disks, and a raid0 of 20 disks.


I guess similar but with a much lower tps. Anyway I can try that.

--
Fabien.


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

Re: [HACKERS] extend pgbench expressions with functions

[Fabien COELHO]

Hello Michaël,


Hmmm, this is subjective:-)


And dependent on personal opinions and views.


I've decided to stay with the current behavior (\setrandom), that is to
abort the current transaction on errors but not to abort pgbench itself. The
check is done before calling the functions, and I let an assert in the
functions just to be sure. It is going to loop on errors anyway, but this is
what it already does anyway.


OK, yes I see now I missed that during my last review. This has the
disadvantage to double the amount of code dedicated to parameter
checks though :(


Yep, I noticed that obviously, but I envision that "\setrandom" pretty 
unelegant code could go away soon, so this is really just temporary.



But based on the feedback perhaps other folks would feel that it would
be actually worth simply dropping the existing \setrandom command.


Yep, exactly my thoughts. I did not do it because there are two ways: 
actually remove it and be done, or build an equivalent \set at parse time, 
so that would just remove the execution part, but keep some ugly stuff in 
parsing.


I would be in favor of just dropping it.

I won't object to that personally, such pgbench features are something 
for hackers and devs mainly, so I guess that we could survive without a 
deprecation period here.


Yep. I can remove it, but I would like a clear go/vote before doing that.


I can understand that, things like that happen all the time here and
that's not a straight-forward patch that we have here. I am sure that
additional opinions here would be good to have before taking one
decision or another. With the current statu-quo, let's just do what
you think is best.



I let the operators alone and just adds functions management next to it.
I'll merge operators as functions only if it is a blocker.


I think that's a blocker, but I am not the only one here and not a committer.


Ok, I can remove that easily anyway.


- fprintf(stderr, "gaussian parameter must be at least
%f (not \"%s\")\n", MIN_GAUSSIAN_PARAM, argv[5]);
+fprintf(stderr,
+   "random gaussian parameter must be greater than %f "
+"(got %f)\n", MIN_GAUSSIAN_PARAM, parameter);
This looks like useless noise to me. Why changing those messages?


Because the message was not saying that it was about random, and I think
that it is the important.


- if (parameter <= 0.0)
+if (parameter < 0.0)
 {

This bit is false, and triggers an assertion failure when the
exponential parameter is 0.


Oops:-( Sorry.


 fprintf(stderr,
-   "exponential parameter must be greater than
zero (not \"%s\")\n",
-argv[5]);
+  "random exponential parameter must be greater than 0.0 "
+   "(got %f)\n", parameter);
 st->ecnt++;
-return true;
+   return false;
This diff is noise as well, and should be removed.


Ok, I can but "zero" and "not" back, but same remark as above, why not 
tell that it is about random? This information is missing.



+   /*
+* Note: this section could be removed, as the same
functionnality
+* is available through \set xxx random_gaussian(...)
+*/
I think that you are right to do that. That's no fun to break existing
scripts, even if people doing that with pgbench are surely experienced
hackers.


Ok, but I would like a clear go or vote before doing this.


-
   case ENODE_VARIABLE:
Such diffs are noise as well.


Yep.


int() should be strengthened regarding bounds. For example:
\set cid debug(int(int(9223372036854775807) +
double(9223372036854775807)))
debug(script=0,command=1): int -9223372036854775808


Hmmm. You mean just to check the double -> int conversion for overflow,
as in:

  SELECT (9223372036854775807::INT8 +
  9223372036854775807::DOUBLE PRECISION)::INT8;

Ok.

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

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/01/12 20:31, Rushabh Lathia wrote:

On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
On 2016/01/06 18:58, Rushabh Lathia wrote:
.) What the need of following change ?

@@ -833,9 +833,6 @@ appendWhereClause(StringInfo buf,
  int nestlevel;
  ListCell   *lc;

-   if (params)
-   *params = NIL;  /* initialize result list to
empty */
-
  /* Set up context struct for recursion */
  context.root = root;
  context.foreignrel = baserel;
@@ -971,6 +968,63 @@ deparseUpdateSql(StringInfo buf,
PlannerInfo *root,
   }



It is needed for deparsePushedDownUpdateSql to store params in both
WHERE clauses and expressions to assign to the target columns
into one params_list list.



Hmm sorry but I am still not getting the point, can you provide some
example to explain this ?


Sorry, maybe my explanation was not enough.  Consider:

postgres=# create foreign table ft1 (a int, b int) server myserver 
options (table_name 't1');

postgres=# insert into ft1 values (0, 0);
postgres=# prepare mt(int, int) as update ft1 set a = $1 where b = $2;
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);

After the 5 executions of mt we have

postgres=# explain verbose execute mt(1, 0);
 QUERY PLAN

 Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
   ->  Foreign Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
 Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b = 
$2::integer))

(3 rows)

If we do that initialization in appendWhereClause, we would get a wrong 
params_list list and a wrong remote pushed-down query for the last mt() 
in deparsePushedDownUpdateSql.



.) When Tom Lane and Stephen Frost suggested getting the core
code involved,
I thought that we can do the mandatory checks into core it self
and making
completely out of dml_is_pushdown_safe(). Please correct me



The reason why I put that function in postgres_fdw.c is Check point 4:

+  * 4. We can't push an UPDATE down, if any expressions to assign
to the target
+  * columns are unsafe to evaluate on the remote server.



Here I was talking about checks related to triggers, or to LIMIT. I think
earlier thread talked about those mandatory check to the core. So may
be we can move those checks into make_modifytable() before calling
the PlanDMLPushdown.


Noticed that.  Will do.

BTW, I keep a ForeignScan node pushing down an update to the remote 
server, in the updated patches.  I have to admit that that seems like 
rather a misnomer.  So, it might be worth adding a new ForeignUpdate 
node, but my concern about that is that if doing so, we would have a lot 
of duplicate code in ForeignUpdate and ForeignScan.  What do you think 
about that?


Best regards,
Etsuro Fujita




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