Re: [HACKERS] Lower msvc build verbosity level

[Michael Paquier]

On Sat, Apr 9, 2016 at 1:47 AM, Andrew Dunstan  wrote:
>
>
> On 04/08/2016 12:24 PM, Christian Ullrich wrote:
>>
>> * Tom Lane wrote:
>>
>>> +several.  Grepping for compiler warnings, for example, is really painful
>>> right now on any MSVC critter.  I've resorted to grepping for "warning
>>> C",
>>> which skips the noise messages, but I'm never sure if I'm missing
>>> something.
>>
>>
>> You miss all diagnostics from other tools than the compiler, for one
>> thing.
>>
>> There is a simple solution to that, however. MSBuild and VCBuild repeat
>> all warnings and errors produced during the build at the bottom of the log.
>> I just checked on mastodon (VS 2005, the oldest), and it does that already.
>>
>> This depends on the whole build being done using a single solution file
>> that contains all the individual projects, but there is no reason to assume
>> we will start building individual projects instead, I assume.
>>
>
> Yeah, what is more on the whole this is going to be far more beneficial,
> because stuff just gets lost badly in the noise. I have committed the
> change.

Thanks a lot! This is making my day, and I suspect many others are to come.
-- 
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] Move PinBuffer and UnpinBuffer to atomics

[Andres Freund]

On 2016-04-10 09:03:37 +0300, Alexander Korotkov wrote:
> On Sun, Apr 10, 2016 at 8:36 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
> 
> > On Sat, Apr 9, 2016 at 10:49 PM, Andres Freund  wrote:
> >
> >>
> >>
> >> On April 9, 2016 12:43:03 PM PDT, Andres Freund 
> >> wrote:
> >> >On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
> >> >> There are results with 5364b357 reverted.
> >> >
> >> >Crazy that this has such a negative impact. Amit, can you reproduce
> >> >that? Alexander, I guess for r/w workload 5364b357 is a benefit on that
> >> >machine as well?
> >>
> >> How sure are you about these measurements?
> >
> >
> > I'm pretty sure.  I've retried it multiple times by hand before re-run the
> > script.
> >
> >
> >> Because there really shouldn't be clog lookups one a steady state is
> >> reached...
> >>
> >
> > Hm... I'm also surprised. There shouldn't be clog lookups once hint bits
> > are set.
> >
> 
> I also tried to run perf top during pgbench and get some interesting
> results.
> 
> Without 5364b357:
>5,69%  postgres [.] GetSnapshotData
>4,47%  postgres [.] LWLockAttemptLock
>3,81%  postgres [.] _bt_compare
>3,42%  postgres [.] hash_search_with_hash_value
>3,08%  postgres [.] LWLockRelease
>2,49%  postgres [.] PinBuffer.isra.3
>1,58%  postgres [.] AllocSetAlloc
>1,17%  [kernel] [k] __schedule
>1,15%  postgres [.] PostgresMain
>1,13%  libc-2.17.so [.] vfprintf
>1,01%  libc-2.17.so [.] __memcpy_ssse3_back
> 
> With 5364b357:
>   18,54%  postgres [.] GetSnapshotData
>3,45%  postgres [.] LWLockRelease
>3,27%  postgres [.] LWLockAttemptLock
>3,21%  postgres [.] _bt_compare
>2,93%  postgres [.] hash_search_with_hash_value
>2,00%  postgres [.] PinBuffer.isra.3
>1,32%  postgres [.] AllocSetAlloc
>1,10%  libc-2.17.so [.] vfprintf
> 
> Very surprising.  It appears that after 5364b357, GetSnapshotData consumes
> more time.  But I can't see anything depending on clog buffers
> in GetSnapshotData code...

Could you retry after applying the attached series of patches?

- Andres
>From e8ad791c97004c64a1f27a500ba100b69fdc8d87 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sun, 10 Apr 2016 21:47:04 -0700
Subject: [PATCH 1/3] Finish 09adc9a8c09c9640de05c7023b27fb83c761e91c.

---
 src/backend/access/transam/parallel.c |  2 +-
 src/backend/port/sysv_shmem.c |  2 +-
 src/backend/port/win32_shmem.c|  2 +-
 src/backend/storage/buffer/buf_init.c | 19 ++-
 src/backend/storage/ipc/shm_toc.c |  6 +++---
 src/backend/storage/ipc/shmem.c   | 17 ++---
 src/include/c.h   |  2 --
 src/include/pg_config_manual.h|  8 
 src/include/storage/shm_toc.h |  2 +-
 9 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 0bba9a7..f8ea89c 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -237,7 +237,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
 		shm_toc_estimate_keys(>estimator, 6);
 
 		/* Estimate space need for error queues. */
-		StaticAssertStmt(BUFFERALIGN(PARALLEL_ERROR_QUEUE_SIZE) ==
+		StaticAssertStmt(CACHELINEALIGN(PARALLEL_ERROR_QUEUE_SIZE) ==
 		 PARALLEL_ERROR_QUEUE_SIZE,
 		 "parallel error queue size not buffer-aligned");
 		shm_toc_estimate_chunk(>estimator,
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 6c442b9..084bc31 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -559,7 +559,7 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
 	 * Initialize space allocation status for segment.
 	 */
 	hdr->totalsize = size;
-	hdr->freeoffset = MAXALIGN(sizeof(PGShmemHeader));
+	hdr->freeoffset = CACHELINEALIGN(sizeof(PGShmemHeader));
 	*shim = hdr;
 
 	/* Save info for possible future use */
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 0ff2c7e..81705fc 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -241,7 +241,7 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
 	 * Initialize space allocation status for segment.
 	 */
 	hdr->totalsize = size;
-	hdr->freeoffset = MAXALIGN(sizeof(PGShmemHeader));
+	hdr->freeoffset = CACHELINEALIGN(sizeof(PGShmemHeader));
 	hdr->dsm_control = 0;
 
 	/* Save info for possible future use */
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index a5cffc7..61f9c34 100644
--- a/src/backend/storage/buffer/buf_init.c

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  wrote:
> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
>> Jeff Janes  writes:
>>> When I compile now without cassert, I get the compiler warning:
>>
>>> syncrep.c: In function 'SyncRepUpdateConfig':
>>> syncrep.c:878:6: warning: variable 'parse_rc' set but not used
>>> [-Wunused-but-set-variable]
>>
>> If there's a good reason for that to be an Assert, I don't see it.
>> There are no callers of SyncRepUpdateConfig that look like they
>> need to, or should expect not to have to, tolerate errors.
>> I think the way to fix this is to turn the Assert into a plain
>> old test-and-ereport-ERROR.
>>
>
> I've changed the draft patch Amit implemented so that it doesn't parse
> twice(check_hook and assign_hook).
> So assertion that was in assign_hook is no longer necessary.
>
> Please find attached.

Thanks for the patch!

When I emptied s_s_names, reloaded the configration file, set it to 'standby1'
and reloaded the configuration file again, the master crashed with
the following error.

*** glibc detected *** postgres: wal sender process postgres [local]
streaming 0/3015F18: munmap_chunk(): invalid pointer:
0x024d9a40 ***
=== Backtrace: =
*** glibc detected *** postgres: wal sender process postgres [local]
streaming 0/3015F18: munmap_chunk(): invalid pointer:
0x024d9a40 ***
/lib64/libc.so.6[0x3be8e75f4e]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
=== Backtrace: =
/lib64/libc.so.6[0x3be8e75f4e]
postgres: wal sender process postgres [local] streaming
0/3015F18(set_config_option+0x12cb)[0x982242]
postgres: wal sender process postgres [local] streaming
0/3015F18(SetConfigOption+0x4b)[0x9827ff]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
postgres: wal sender process postgres [local] streaming
0/3015F18(set_config_option+0x12cb)[0x982242]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
postgres: wal sender process postgres [local] streaming
0/3015F18(SetConfigOption+0x4b)[0x9827ff]
postgres: wal sender process postgres [local] streaming
0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
postgres: wal sender process postgres [local] streaming
0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
postgres: wal sender process postgres [local] streaming
0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
postgres: wal sender process postgres [local] streaming
0/3015F18(PostgresMain+0x772)[0x8141b6]
postgres: wal sender process postgres [local] streaming
0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
postgres: wal sender process postgres [local] streaming
0/3015F18(PostgresMain+0x772)[0x8141b6]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
postgres: wal sender process postgres [local] streaming
0/3015F18(PostmasterMain+0x1134)[0x784c08]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x6ce12e]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x3be8e1ed5d]
postgres: wal sender process postgres [local] streaming
0/3015F18(PostmasterMain+0x1134)[0x784c08]
postgres: wal sender process postgres [local] streaming 0/3015F18[0x467e99]

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] [COMMITTERS] pgsql: Move each SLRU's lwlocks to a separate tranche.

[Andres Freund]

On 2016-04-10 16:08:56 -0700, Andres Freund wrote:
> On 2016-04-05 15:48:22 -0400, Robert Haas wrote:
> > On Fri, Mar 25, 2016 at 12:47 PM, Tom Lane  wrote:
> > > Robert Haas  writes:
> > >> On Fri, Mar 25, 2016 at 9:48 AM, Tom Lane  wrote:
> > >>> Robert Haas  writes:
> >  It's stupid that we keep spending time and energy figuring out which
> >  shared memory data structures require alignment and which ones don't.
> >  Let's just align them *all* and be done with it.  The memory cost
> >  shouldn't be more than a few kB.
> > >
> > >>> I think such a proposal should come with a measurement, not just
> > >>> speculation about what it costs.
> > >
> > >> About 6kB with default settings.  See below.
> > >
> > > Sold, then.
> > 
> > Excellent.  Done.
> 
> InitBufferPool() manually fixes up alignment; that should probably be
> removed now.
> 
> I also wonder if it doesn't make sense to fix PG_CACHE_LINE_SIZE to
> 64byte on x86. I personally think a manual ifdef in pg_config_manual.h
> is ok for that.

Also, this doesn't seem to be complete. This now aligns sizes to
cacheline boundaries, but it doesn't actually align the returned values
afaics? That might kind of work sometimes, if freeoffset is initially
aligned to PG_CACHE_LINE_SIZE, but that's not guaranteed, it's just
shmhdr->freeoffset += MAXALIGN(sizeof(slock_t));

Additionally, doesn't this obsolete

/*
 * Preferred alignment for disk I/O buffers.  On some CPUs, copies between
 * user space and kernel space are significantly faster if the user buffer
 * is aligned on a larger-than-MAXALIGN boundary.  Ideally this should be
 * a platform-dependent value, but for now we just hard-wire it.
 */
#define ALIGNOF_BUFFER  32

and

/* extra alignment for large requests, since they are probably buffers 
*/
if (size >= BLCKSZ)
newStart = BUFFERALIGN(newStart);

- 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] Optimization for updating foreign tables in Postgres FDW

[Michael Paquier]

On Mon, Apr 11, 2016 at 11:30 AM, Etsuro Fujita
 wrote:
> I agree with Rushabh.  Thanks for updating the patch!

Yes, not using a PG_TRY/CATCH block is better. We are not doing
anything that need to clean up PGresult in case of an unplanned
failure.

> Another thing I'd like to propose to revise the patch is to call
> pgfdw_report_error in the newly added hunk, with the clear argument set to
> *false*.  The PGresult argument is NULL there, so no need to release the
> PGresult.

Sure, this saves a couple of cycles. PQclear is anyway smart enough to
handle NULL results correctly, but this way is better.

> Attached is an updated patch.

+   if (wc & WL_SOCKET_READABLE)
+   {
+   if (!PQconsumeInput(dmstate->conn))
+   pgfdw_report_error(ERROR, NULL, dmstate->conn, false,
+  dmstate->query);
+   }
OK, so here we would fail with ERRCODE_CONNECTION_FAILURE in the case
where the socket is readable but no data can be consumed. I guess it
makes sense.

+   if ((cancel = PQgetCancel(entry->conn)))
+   {
+   PQcancel(cancel, errbuf, sizeof(errbuf));
+   PQfreeCancel(cancel);
+   }
Wouldn't it be better to issue a WARNING here if PQcancel does not succeed?
-- 
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] 2016-03 Commitfest

[Andres Freund]

On 2016-04-08 11:52:45 -0400, Robert Haas wrote:
> On Fri, Apr 8, 2016 at 11:00 AM, Andres Freund  wrote:
> > I've finished polishing the Pin/Unpin patch. But the final polishing
> > happened on an intercontential flight, after days spent preparing my
> > move to SF. I'd be glad if you would allow me to look over the patch
> > again, before pushing it sometime this weekend; this stuff is subtle,
> > and I'm not exactly my best right now.
> 
> In view of these circumstances, the RMT has voted to extend the
> deadline for this particular patch by 2.5 days; that is, this patch
> may be committed with RMT approval no later than 2016-04-11 12:00:00
> GMT, which I believe is approximately 4am Monday morning where you
> are.

I've pushed this now. Didn't find anything really grave; fixed some easy
to misunderstand variable naming, and some variables declared again in a
nested scope (correct, but confusing).  Thanks for the extension.

I guess we can release 9.6 now.

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] Weird irreproducible behaviors in plpython tests

[Tom Lane]

Andres Freund  writes:
> I downloaded the RHEL6 srpm, and it appears to be patched to
> automatically disable pymalloc when running under valgrind (via
> disable-pymalloc-on-valgrind-py26.patch).  That explains why you're
> seing the problem, but skink isn't (it's running debian).

Hah.  I suspected Red Hat had done something to hide the issue, but
didn't get around to investigating yet.  Anyway, it's clearly a real
bug, even if skink is failing to find it.

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] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/04/08 22:21, Rushabh Lathia wrote:

On Fri, Apr 8, 2016 at 6:28 PM, Robert Haas > wrote:



The comment just before the second hunk in the patch says:

* We don't use a PG_TRY block here, so be careful not to
throw error
* without releasing the PGresult.

But the patch adds a whole bunch of new things there that seem like
they can error out, like CHECK_FOR_INTERRUPTS(), for example.  Isn't
that a problem?



Basically we fetching the PGresult, after the newly added hunk, so there
should not be any problem.

But yes comment is definitely at wrong place.

PFA patch with correction.


I agree with Rushabh.  Thanks for updating the patch!

Another thing I'd like to propose to revise the patch is to call 
pgfdw_report_error in the newly added hunk, with the clear argument set 
to *false*.  The PGresult argument is NULL there, so no need to release 
the PGresult.


Attached is an updated patch.

Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 189f290..8820597 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -598,6 +598,26 @@ pgfdw_xact_callback(XactEvent event, void *arg)
 case XACT_EVENT_ABORT:
 	/* Assume we might have lost track of prepared statements */
 	entry->have_error = true;
+	/*
+	 * If we had submitted a command to the remote server using
+	 * an asynchronous execution function, the command might
+	 * have not yet completed.  Check to see if a command is
+	 * still being processed by the remote server, and if so,
+	 * request cancellation of the command; if not, abort
+	 * gracefully.
+	 */
+	if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
+	{
+		PGcancel   *cancel;
+		char		errbuf[256];
+
+		if ((cancel = PQgetCancel(entry->conn)))
+		{
+			PQcancel(cancel, errbuf, sizeof(errbuf));
+			PQfreeCancel(cancel);
+		}
+		break;
+	}
 	/* If we're aborting, abort all remote transactions too */
 	res = PQexec(entry->conn, "ABORT TRANSACTION");
 	/* Note: can't throw ERROR, it would be infinite loop */
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index ee0220a..0378f1d 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -32,6 +32,7 @@
 #include "optimizer/var.h"
 #include "optimizer/tlist.h"
 #include "parser/parsetree.h"
+#include "storage/latch.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
 #include "utils/lsyscache.h"
@@ -3131,6 +3132,7 @@ execute_dml_stmt(ForeignScanState *node)
 	ExprContext *econtext = node->ss.ps.ps_ExprContext;
 	int			numParams = dmstate->numParams;
 	const char **values = dmstate->param_values;
+	int			wc;
 
 	/*
 	 * Construct array of query parameter values in text format.
@@ -3147,12 +3149,42 @@ execute_dml_stmt(ForeignScanState *node)
 	 * parameter (see deparse.c), the "inference" is trivial and will produce
 	 * the desired result.  This allows us to avoid assuming that the remote
 	 * server has the same OIDs we do for the parameters' types.
+	 */
+	if (!PQsendQueryParams(dmstate->conn, dmstate->query, numParams,
+		   NULL, values, NULL, NULL, 0))
+		pgfdw_report_error(ERROR, NULL, dmstate->conn, false, dmstate->query);
+
+	/*
+	 * Receive data until PQgetResult is ready to get the result without
+	 * blocking.
+	 */
+	while (PQisBusy(dmstate->conn))
+	{
+		/* Sleep until there's something to do */
+		wc = WaitLatchOrSocket(MyLatch,
+			   WL_LATCH_SET | WL_SOCKET_READABLE,
+			   PQsocket(dmstate->conn),
+			   -1L);
+		ResetLatch(MyLatch);
+
+		CHECK_FOR_INTERRUPTS();
+
+		/* Data available in socket */
+		if (wc & WL_SOCKET_READABLE)
+		{
+			if (!PQconsumeInput(dmstate->conn))
+pgfdw_report_error(ERROR, NULL, dmstate->conn, false,
+   dmstate->query);
+		}
+	}
+
+	/*
+	 * Get the result
 	 *
 	 * We don't use a PG_TRY block here, so be careful not to throw error
 	 * without releasing the PGresult.
 	 */
-	dmstate->result = PQexecParams(dmstate->conn, dmstate->query,
-   numParams, NULL, values, NULL, NULL, 0);
+	dmstate->result = PQgetResult(dmstate->conn);
 	if (PQresultStatus(dmstate->result) !=
 		(dmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
 		pgfdw_report_error(ERROR, dmstate->result, dmstate->conn, true,

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

Re: [HACKERS] Weird irreproducible behaviors in plpython tests

[Andres Freund]

On 2016-04-10 22:03:53 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-04-10 17:55:25 -0400, Tom Lane wrote:
> >> Hmm.  It's true that I don't have the python debuginfo RPM installed.
> >> But this is a live bug, so I suspect you were too generous about
> >> those suppressions.
> 
> > Could be - I just used the ones (after adapting for 32 vs. 64 bit
> > issues) provided by upstream.
> 
> I still see the same failure after installing python-debuginfo:
> 
> ==00:00:00:55.235 18250== Invalid read of size 1
> ==00:00:00:55.235 18250==at 0x4A07F92: strlen (mc_replace_strmem.c:403)
> ==00:00:00:55.235 18250==by 0x826860: MemoryContextStrdup (mcxt.c:1158)
> ==00:00:00:55.235 18250==by 0x800441: set_errdata_field (elog.c:1230)
> ==00:00:00:55.235 18250==by 0x803EE8: err_generic_string (elog.c:1210)
> ==00:00:00:55.235 18250==by 0xDFEF2DD: PLy_elog (plpy_elog.c:117)
> ==00:00:00:55.235 18250==by 0xDFF018D: PLy_procedure_call 
> (plpy_exec.c:1084)
> ==00:00:00:55.235 18250==by 0xDFF14C6: PLy_exec_function (plpy_exec.c:105)
> ==00:00:00:55.235 18250==by 0xDFF2103: plpython_inline_handler 
> (plpy_main.c:345)
> ==00:00:00:55.235 18250==by 0x809737: OidFunctionCall1Coll (fmgr.c:1596)
> ==00:00:00:55.235 18250==by 0x70E97F: standard_ProcessUtility 
> (utility.c:515)
> ==00:00:00:55.235 18250==by 0x70A856: PortalRunUtility (pquery.c:1175)
> ==00:00:00:55.235 18250==by 0x70B8FF: PortalRunMulti (pquery.c:1306)
> ==00:00:00:55.235 18250==  Address 0xefe1954 is 36 bytes inside a block of 
> size 49 free'd
> ==00:00:00:55.235 18250==at 0x4A06430: free (vg_replace_malloc.c:446)
> ==00:00:00:55.235 18250==by 0x398AE90D5C: tupledealloc (tupleobject.c:170)
> ==00:00:00:55.235 18250==by 0x398AE79E3A: dict_dealloc (dictobject.c:911)
> ==00:00:00:55.235 18250==by 0x398AE5C586: BaseException_clear 
> (exceptions.c:77)
> ==00:00:00:55.235 18250==by 0x398AE5C5BF: BaseException_dealloc 
> (exceptions.c:87)
> ==00:00:00:55.235 18250==by 0x398AE9A704: subtype_dealloc 
> (typeobject.c:1019)
> ==00:00:00:55.236 18250==by 0xDFEEDBC: PLy_traceback (plpy_elog.c:358)
> ==00:00:00:55.236 18250==by 0xDFEF154: PLy_elog (plpy_elog.c:83)
> ==00:00:00:55.236 18250==by 0xDFF018D: PLy_procedure_call 
> (plpy_exec.c:1084)
> ==00:00:00:55.236 18250==by 0xDFF14C6: PLy_exec_function (plpy_exec.c:105)
> ==00:00:00:55.236 18250==by 0xDFF2103: plpython_inline_handler 
> (plpy_main.c:345)
> ==00:00:00:55.236 18250==by 0x809737: OidFunctionCall1Coll (fmgr.c:1596)
> 
> 
> With the patch I'm working on, no error, not even with the python-specific
> suppressions all removed from valgrind.supp.  Not sure what to make of
> that.  RHEL6's version of python is 2.6.6, which is not real new, but
> the documentation that comes with it indicates that the false-valgrind-
> warnings problem exists.

I downloaded the RHEL6 srpm, and it appears to be patched to
automatically disable pymalloc when running under valgrind (via
disable-pymalloc-on-valgrind-py26.patch).  That explains why you're
seing the problem, but skink isn't (it's running debian).

Greetings,

Andres Freund
diff -up Python-2.6.6/configure.in.disable-pymalloc-on-valgrind Python-2.6.6/configure.in
--- Python-2.6.6/configure.in.disable-pymalloc-on-valgrind	2010-11-29 15:45:07.199350502 -0500
+++ Python-2.6.6/configure.in	2010-11-29 15:45:07.208351260 -0500
@@ -2538,6 +2538,19 @@ then
 fi
 AC_MSG_RESULT($with_pymalloc)
 
+# Check for Valgrind support
+AC_MSG_CHECKING([for --with-valgrind])
+AC_ARG_WITH([valgrind],
+  AC_HELP_STRING([--with-valgrind], [Enable Valgrind support]),,
+  with_valgrind=no)
+AC_MSG_RESULT([$with_valgrind])
+if test "$with_valgrind" != no; then
+AC_CHECK_HEADER([valgrind/valgrind.h],
+  [AC_DEFINE([WITH_VALGRIND], 1, [Define if you want pymalloc to be disabled when running under valgrind])],
+  [AC_MSG_ERROR([Valgrind support requested but headers not available])]
+)
+fi
+
 # Check for --with-wctype-functions
 AC_MSG_CHECKING(for --with-wctype-functions)
 AC_ARG_WITH(wctype-functions, 
diff -up Python-2.6.6/Misc/NEWS.disable-pymalloc-on-valgrind Python-2.6.6/Misc/NEWS
--- Python-2.6.6/Misc/NEWS.disable-pymalloc-on-valgrind	2010-08-23 19:37:56.0 -0400
+++ Python-2.6.6/Misc/NEWS	2010-11-29 15:45:07.209350567 -0500
@@ -21,6 +21,11 @@ What's New in Python 2.6.6 rc 2?
 
 *Release date: 2010-08-16*
 
+- Issue #2422: When compiled with the ``--with-valgrind`` option, the
+  pymalloc allocator will be automatically disabled when running under
+  Valgrind.  This gives improved memory leak detection when running
+  under Valgrind, while taking advantage of pymalloc at other times.
+
 Library
 ---
 
diff -up Python-2.6.6/Objects/obmalloc.c.disable-pymalloc-on-valgrind Python-2.6.6/Objects/obmalloc.c
--- Python-2.6.6/Objects/obmalloc.c.disable-pymalloc-on-valgrind	2010-05-09 11:15:40.0 -0400
+++ Python-2.6.6/Objects/obmalloc.c	2010-11-29 

Re: [HACKERS] Weird irreproducible behaviors in plpython tests

[Tom Lane]

Andres Freund  writes:
> On 2016-04-10 17:55:25 -0400, Tom Lane wrote:
>> Hmm.  It's true that I don't have the python debuginfo RPM installed.
>> But this is a live bug, so I suspect you were too generous about
>> those suppressions.

> Could be - I just used the ones (after adapting for 32 vs. 64 bit
> issues) provided by upstream.

I still see the same failure after installing python-debuginfo:

==00:00:00:55.235 18250== Invalid read of size 1
==00:00:00:55.235 18250==at 0x4A07F92: strlen (mc_replace_strmem.c:403)
==00:00:00:55.235 18250==by 0x826860: MemoryContextStrdup (mcxt.c:1158)
==00:00:00:55.235 18250==by 0x800441: set_errdata_field (elog.c:1230)
==00:00:00:55.235 18250==by 0x803EE8: err_generic_string (elog.c:1210)
==00:00:00:55.235 18250==by 0xDFEF2DD: PLy_elog (plpy_elog.c:117)
==00:00:00:55.235 18250==by 0xDFF018D: PLy_procedure_call (plpy_exec.c:1084)
==00:00:00:55.235 18250==by 0xDFF14C6: PLy_exec_function (plpy_exec.c:105)
==00:00:00:55.235 18250==by 0xDFF2103: plpython_inline_handler 
(plpy_main.c:345)
==00:00:00:55.235 18250==by 0x809737: OidFunctionCall1Coll (fmgr.c:1596)
==00:00:00:55.235 18250==by 0x70E97F: standard_ProcessUtility 
(utility.c:515)
==00:00:00:55.235 18250==by 0x70A856: PortalRunUtility (pquery.c:1175)
==00:00:00:55.235 18250==by 0x70B8FF: PortalRunMulti (pquery.c:1306)
==00:00:00:55.235 18250==  Address 0xefe1954 is 36 bytes inside a block of size 
49 free'd
==00:00:00:55.235 18250==at 0x4A06430: free (vg_replace_malloc.c:446)
==00:00:00:55.235 18250==by 0x398AE90D5C: tupledealloc (tupleobject.c:170)
==00:00:00:55.235 18250==by 0x398AE79E3A: dict_dealloc (dictobject.c:911)
==00:00:00:55.235 18250==by 0x398AE5C586: BaseException_clear 
(exceptions.c:77)
==00:00:00:55.235 18250==by 0x398AE5C5BF: BaseException_dealloc 
(exceptions.c:87)
==00:00:00:55.235 18250==by 0x398AE9A704: subtype_dealloc 
(typeobject.c:1019)
==00:00:00:55.236 18250==by 0xDFEEDBC: PLy_traceback (plpy_elog.c:358)
==00:00:00:55.236 18250==by 0xDFEF154: PLy_elog (plpy_elog.c:83)
==00:00:00:55.236 18250==by 0xDFF018D: PLy_procedure_call (plpy_exec.c:1084)
==00:00:00:55.236 18250==by 0xDFF14C6: PLy_exec_function (plpy_exec.c:105)
==00:00:00:55.236 18250==by 0xDFF2103: plpython_inline_handler 
(plpy_main.c:345)
==00:00:00:55.236 18250==by 0x809737: OidFunctionCall1Coll (fmgr.c:1596)


With the patch I'm working on, no error, not even with the python-specific
suppressions all removed from valgrind.supp.  Not sure what to make of
that.  RHEL6's version of python is 2.6.6, which is not real new, but
the documentation that comes with it indicates that the false-valgrind-
warnings problem exists.

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] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
> Jeff Janes  writes:
>> When I compile now without cassert, I get the compiler warning:
>
>> syncrep.c: In function 'SyncRepUpdateConfig':
>> syncrep.c:878:6: warning: variable 'parse_rc' set but not used
>> [-Wunused-but-set-variable]
>
> If there's a good reason for that to be an Assert, I don't see it.
> There are no callers of SyncRepUpdateConfig that look like they
> need to, or should expect not to have to, tolerate errors.
> I think the way to fix this is to turn the Assert into a plain
> old test-and-ereport-ERROR.
>

I've changed the draft patch Amit implemented so that it doesn't parse
twice(check_hook and assign_hook).
So assertion that was in assign_hook is no longer necessary.

Please find attached.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index cb6b5e5..4733dc1 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -68,6 +68,7 @@
 #include "storage/proc.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
+#include "utils/memutils.h"
 #include "utils/ps_status.h"
 
 /* User-settable parameters for sync rep */
@@ -362,7 +363,7 @@ SyncRepInitConfig(void)
 	int			priority;
 
 	/* Update the config data of synchronous replication */
-	SyncRepFreeConfig(SyncRepConfig);
+	SyncRepFreeConfig(SyncRepConfig, true);
 	SyncRepConfig = NULL;
 	SyncRepUpdateConfig();
 
@@ -897,13 +898,16 @@ SyncRepUpdateConfig(void)
  * Free a previously-allocated config data of synchronous replication.
  */
 void
-SyncRepFreeConfig(SyncRepConfigData *config)
+SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
 {
 	if (!config)
 		return;
 
 	list_free_deep(config->members);
-	pfree(config);
+
+	if (itself)
+		free(config);
+
 }
 
 #ifdef USE_ASSERT_CHECKING
@@ -954,6 +958,10 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)
 
 	if (*newval != NULL && (*newval)[0] != '\0')
 	{
+		MemoryContext oldcontext;
+
+		oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
 		syncrep_scanner_init(*newval);
 		parse_rc = syncrep_yyparse();
 		syncrep_scanner_finish();
@@ -1012,17 +1020,36 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)
 		}
 
 		/*
-		 * syncrep_yyparse sets the global syncrep_parse_result as side effect.
-		 * But this function is required to just check, so frees it
-		 * after parsing the parameter.
+		 * syncrep_yyparse sets the global syncrep_parse_result.
+		 * Save syncrep_parse_result to extra in order to use it in
+		 * assign_synchronous_standby_names hook as well.
 		 */
-		SyncRepFreeConfig(syncrep_parse_result);
+		*extra = (void *)syncrep_parse_result;
+
+		MemoryContextSwitchTo(oldcontext);
 	}
 
 	return true;
 }
 
 void
+assign_synchronous_standby_names(const char *newval, void *extra)
+{
+	SyncRepConfigData *myextra = extra;
+
+	/*
+	 * Free members of SyncRepConfig if it already refers somewhere,
+	 * but SyncRepConfig itself is freed by set_extra_field.
+	 */
+	if (SyncRepConfig)
+		SyncRepFreeConfig(SyncRepConfig, false);
+
+	SyncRepConfig = myextra;
+
+	return;
+}
+
+void
 assign_synchronous_commit(int newval, void *extra)
 {
 	switch (newval)
diff --git a/src/backend/replication/syncrep_gram.y b/src/backend/replication/syncrep_gram.y
index 380fedc..de25a40 100644
--- a/src/backend/replication/syncrep_gram.y
+++ b/src/backend/replication/syncrep_gram.y
@@ -76,7 +76,7 @@ static SyncRepConfigData *
 create_syncrep_config(char *num_sync, List *members)
 {
 	SyncRepConfigData *config =
-		(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
+		(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
 
 	config->num_sync = atoi(num_sync);
 	config->members = members;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e4a0119..a9c982b 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2780,23 +2780,12 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
 	MemoryContextSwitchTo(oldcontext);
 
 	/*
-	 * Allocate and update the config data of synchronous replication,
-	 * and then get the currently active synchronous standbys.
+	 * Get the currently active synchronous standbys.
 	 */
-	SyncRepUpdateConfig();
 	LWLockAcquire(SyncRepLock, LW_SHARED);
 	sync_standbys = SyncRepGetSyncStandbys(NULL);
 	LWLockRelease(SyncRepLock);
 
-	/*
-	 * Free the previously-allocated config data because a backend
-	 * no longer needs it. The next call of this function needs to
-	 * allocate and update the config data newly because the setting
-	 * of sync replication might be changed between the calls.
-	 */
-	SyncRepFreeConfig(SyncRepConfig);
-	SyncRepConfig = NULL;
-
 	for (i = 0; i < max_wal_senders; i++)
 	{
 		WalSnd *walsnd = >walsnds[i];
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fb091bc..3ce83bf 100644
--- 

Re: [HACKERS] [COMMITTERS] pgsql: Move each SLRU's lwlocks to a separate tranche.

[Andres Freund]

On 2016-04-05 15:48:22 -0400, Robert Haas wrote:
> On Fri, Mar 25, 2016 at 12:47 PM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> On Fri, Mar 25, 2016 at 9:48 AM, Tom Lane  wrote:
> >>> Robert Haas  writes:
>  It's stupid that we keep spending time and energy figuring out which
>  shared memory data structures require alignment and which ones don't.
>  Let's just align them *all* and be done with it.  The memory cost
>  shouldn't be more than a few kB.
> >
> >>> I think such a proposal should come with a measurement, not just
> >>> speculation about what it costs.
> >
> >> About 6kB with default settings.  See below.
> >
> > Sold, then.
> 
> Excellent.  Done.

InitBufferPool() manually fixes up alignment; that should probably be
removed now.

I also wonder if it doesn't make sense to fix PG_CACHE_LINE_SIZE to
64byte on x86. I personally think a manual ifdef in pg_config_manual.h
is ok for that.

Greetings,

Andres Freund


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

Re: [HACKERS] Relax requirement for INTO with SELECT in pl/pgsql

[David G. Johnston]

On Sun, Apr 10, 2016 at 10:01 AM, Pavel Stehule 
wrote:

>
>
> 2016-04-10 18:49 GMT+02:00 David G. Johnston :
>
>> On Sun, Apr 10, 2016 at 9:16 AM, Pavel Stehule 
>> wrote:
>>
>>>
>>>
>>> 2016-04-10 17:49 GMT+02:00 David G. Johnston >> >:
>>>
 On Sun, Apr 10, 2016 at 1:13 AM, Pavel Stehule  wrote:

> Hi
>
> 2016-03-21 22:13 GMT+01:00 Pavel Stehule :
>
>> Hi
>>
>> 2016-03-21 21:24 GMT+01:00 Merlin Moncure :
>>
>>> Patch is trivial (see below), discussion is not :-).
>>>
>>> I see no useful reason to require INTO when returning data with
>>> SELECT.  However, requiring queries to indicate not needing data via
>>> PERFORM causes some annoyances:
>>>
>>> *) converting routines back and forth between pl/pgsql and pl/sql
>>> requires needless busywork and tends to cause errors to be thrown at
>>> runtime
>>>
>>> *) as much as possible, (keywords begin/end remain a problem),
>>> pl/pgsql should be a superset of sql
>>>
>>> *) it's much more likely to be burned by accidentally forgetting to
>>> swap in PERFORM than to accidentally leave in a statement with no
>>> actionable target.  Even if you did so in the latter case, it stands
>>> to reason you'd accidentally leave in the target variable, too.
>>>
>>> *) the PERFORM requirement hails from the days when only statements
>>> starting with SELECT return data.  There is no PERFORM equivalent for
>>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where
>>> you
>>> might have a RETURNING clause that does something but not necessarily
>>> want to place the result in a variable (for example passing to
>>> volatile function).  Take a look at the errhint() clause below -- we
>>> don't even have a suggestion in that case.
>>>
>>> This has come up before, and there was a fair amount of sympathy for
>>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>>> get a hearing on the issue -- thanks.  If we decide to move forward,
>>> this would effectively deprecate PERFORM and the documentation will
>>> be
>>> suitably modified as well.
>>>
>>
>>
> here is another argument why this idea is not good.
>
>
> http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function
>
> Now, when people coming from T-SQL world use some T-SQL constructs,
> then usually the code should not work with the error "query has not
> destination for data ... "
>
> When PLpgSQL will be more tolerant, then their code will be executed
> without any error, but will not work.
>
>
 I would be inclined to require that DML returning tuples requires INTO
 while a SELECT does not.  Adding RETURNING is a deliberate user action that
 we can and probably should be conservative for.  Writing SELECT is default
 user behavior and is quite often used only for its side-effects.  Since SQL
 proper doesn't offer a means to distinguish between the two modes adding
 that distinction to pl/pgSQL, while useful, doesn't seem like something
 that has to be forced upon the user.

>>>
>>> It doesn't help - SELECT is most often used construct.
>>>
>>> We can be less strict for SELECT expr, but SELECT FROM should not be
>>> allowed without INTO clause.
>>>
>>>
>> ​SELECT perform_this_action_for_every_user(user_id) FROM usertable;
>>
>> I still only care about side-effects.
>>
>> The rule remains (becomes?) simple:  Use INTO if you need to capture the
>> SQL value into a pl/pgSQL variable - otherwise don't.  WRT your prior post
>> I'd tell the user they are doing something really unusual if they write
>> INSERT RETURNING without INTO - which I have no problem doing.
>>
>
> This is the problem. Any other databases doesn't allow it - or it has
> pretty different semantic (in T-SQL)
>
> I am skeptical if benefit is higher than costs.
>

​This isn't T-SQL, and if you are not going to explain how it works and why
its behavior is desirable I'm not going to be convinced that it matters.



>
>
>>
>> We don't need to force the user to tell us they intentionally omitted the
>> INTO clause.  The omission itself is sufficient.  Using select without a
>> target pl/pgSQL variable is a valid and probably quite common construct and
>> hindering it because it might make debugging a function a bit harder (wrong
>> data instead of an error) doesn't seem worthwhile.  You are making
>> accommodations for exceptional situations.  I'm not convinced that it will
>> be significantly harder to spot a missing INTO in a world where one is
>> allowed to write such a statement without PERFORM.  Yes, it will not be as
>> 

Re: [HACKERS] Weird irreproducible behaviors in plpython tests

[Andres Freund]

On 2016-04-10 17:55:25 -0400, Tom Lane wrote:
> Hmm.  It's true that I don't have the python debuginfo RPM installed.
> But this is a live bug, so I suspect you were too generous about
> those suppressions.

Could be - I just used the ones (after adapting for 32 vs. 64 bit
issues) provided by upstream.

> FWIW, HEAD passes cleanly under valgrind for me after fixing this one
> problem.  I have to leave shortly but will work on the back branches
> later.

Looking through them again:
# Python's allocator does some low-level tricks for efficiency. Those
# can be disabled for better instrumentation; but few people testing
# postgres will have such a build of python. So add broad
# suppressions of the resulting errors.
# See also https://svn.python.org/projects/python/trunk/Misc/README.valgrind
{
   python_clever_allocator
   Memcheck:Addr4
   fun:PyObject_Free
}

{
   python_clever_allocator
   Memcheck:Addr8
   fun:PyObject_Free
}

{
   python_clever_allocator
   Memcheck:Value4
   fun:PyObject_Free
}

{
   python_clever_allocator
   Memcheck:Value8
   fun:PyObject_Free
}

{
   python_clever_allocator
   Memcheck:Cond
   fun:PyObject_Free
}

{
   python_clever_allocator
   Memcheck:Addr4
   fun:PyObject_Realloc
}

{
   python_clever_allocator
   Memcheck:Addr8
   fun:PyObject_Realloc
}

{
   python_clever_allocator
   Memcheck:Value4
   fun:PyObject_Realloc
}

{
   python_clever_allocator
   Memcheck:Value8
   fun:PyObject_Realloc
}

{
   python_clever_allocator
   Memcheck:Cond
   fun:PyObject_Realloc
}

I can't actually see any triggering invalidly with the backtrace you
provided :(

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] Weird irreproducible behaviors in plpython tests

[Tom Lane]

Andres Freund  writes:
> On 2016-04-10 17:10:57 -0400, Tom Lane wrote:
>> ... I'll be fixing this one
>> shortly, but now we have another puzzle: why isn't buildfarm member
>> skink seeing the same failure?  It is running the plpython tests.

> I possible that it's hidden by the broad python error suppressions I
> added to valgrind.supp, and the reason they're showing for you is that
> you don't appear to have python debugging symbols. But I don't exactly
> see the error matching up...

Hmm.  It's true that I don't have the python debuginfo RPM installed.
But this is a live bug, so I suspect you were too generous about
those suppressions.

FWIW, HEAD passes cleanly under valgrind for me after fixing this one
problem.  I have to leave shortly but will work on the back branches
later.

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] Weird irreproducible behaviors in plpython tests

[Andres Freund]

On 2016-04-10 17:10:57 -0400, Tom Lane wrote:
> After failing at that, it occurred to me to try it under valgrind,
> and kaboom!  I found a *different* bug, which has apparently been
> there a long time.  (I say different because I don't see how this
> one could produce tick's symptoms; it's a reference to already-freed
> strings, but not an attempt to pfree one.)  I'll be fixing this one
> shortly, but now we have another puzzle: why isn't buildfarm member
> skink seeing the same failure?  It is running the plpython tests.
> Can anyone else reproduce a failure by valgrind'ing the plpython
> tests?  It looks here like

I possible that it's hidden by the broad python error suppressions I
added to valgrind.supp, and the reason they're showing for you is that
you don't appear to have python debugging symbols. But I don't exactly
see the error matching up...

- Andres


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

[HACKERS] Weird irreproducible behaviors in plpython tests

[Tom Lane]

Buildfarm member tick failed today with what appears to be a bug
introduced by (or at least exposed by) the recent plpython ereport
patch.  Presumably the fact that it's failing and no other critters are
has to do with its use of -DCLOBBER_CACHE_ALWAYS and/or
-DRANDOMIZE_ALLOCATED_MEMORY.  However, I cannot reproduce the failure
by using those switches, even though tick's CentOS platform should be
pretty much identical to my RHEL6 machine.  Can anyone else reproduce it?

After failing at that, it occurred to me to try it under valgrind,
and kaboom!  I found a *different* bug, which has apparently been
there a long time.  (I say different because I don't see how this
one could produce tick's symptoms; it's a reference to already-freed
strings, but not an attempt to pfree one.)  I'll be fixing this one
shortly, but now we have another puzzle: why isn't buildfarm member
skink seeing the same failure?  It is running the plpython tests.
Can anyone else reproduce a failure by valgrind'ing the plpython
tests?  It looks here like

==00:00:00:29.670 24996== Invalid read of size 1
==00:00:00:29.670 24996==at 0x4A07F92: strlen (mc_replace_strmem.c:403)
==00:00:00:29.670 24996==by 0x826860: MemoryContextStrdup (mcxt.c:1158)
==00:00:00:29.670 24996==by 0x800441: set_errdata_field (elog.c:1230)
==00:00:00:29.670 24996==by 0x803EE8: err_generic_string (elog.c:1210)
==00:00:00:29.670 24996==by 0xDFEF382: PLy_elog (plpy_elog.c:117)
==00:00:00:29.670 24996==by 0xDFF049D: PLy_procedure_call (plpy_exec.c:1084)
==00:00:00:29.670 24996==by 0xDFF1BBF: PLy_exec_function (plpy_exec.c:105)
==00:00:00:29.670 24996==by 0xDFF25C6: plpython_inline_handler 
(plpy_main.c:345)
==00:00:00:29.670 24996==by 0x809737: OidFunctionCall1Coll (fmgr.c:1596)
==00:00:00:29.670 24996==by 0x70E97F: standard_ProcessUtility 
(utility.c:515)
==00:00:00:29.670 24996==by 0x70A856: PortalRunUtility (pquery.c:1175)
==00:00:00:29.670 24996==by 0x70B8FF: PortalRunMulti (pquery.c:1306)
==00:00:00:29.670 24996==  Address 0xefe2894 is 36 bytes inside a block of size 
49 free'd
==00:00:00:29.670 24996==at 0x4A06430: free (vg_replace_malloc.c:446)
==00:00:00:29.670 24996==by 0x398AE90D5C: ??? (in 
/usr/lib64/libpython2.6.so.1.0)
==00:00:00:29.670 24996==by 0x398AE79E3A: ??? (in 
/usr/lib64/libpython2.6.so.1.0)
==00:00:00:29.670 24996==by 0x398AE5C586: ??? (in 
/usr/lib64/libpython2.6.so.1.0)
==00:00:00:29.670 24996==by 0x398AE5C5BF: ??? (in 
/usr/lib64/libpython2.6.so.1.0)
==00:00:00:29.670 24996==by 0x398AE9A704: ??? (in 
/usr/lib64/libpython2.6.so.1.0)
==00:00:00:29.670 24996==by 0xDFEECFE: PLy_traceback (plpy_elog.c:358)
==00:00:00:29.670 24996==by 0xDFEF21E: PLy_elog (plpy_elog.c:83)
==00:00:00:29.670 24996==by 0xDFF049D: PLy_procedure_call (plpy_exec.c:1084)
==00:00:00:29.670 24996==by 0xDFF1BBF: PLy_exec_function (plpy_exec.c:105)
==00:00:00:29.670 24996==by 0xDFF25C6: plpython_inline_handler 
(plpy_main.c:345)
==00:00:00:29.670 24996==by 0x809737: OidFunctionCall1Coll (fmgr.c:1596)

the core of the problem being that PLy_get_spi_error_data returns a
pointer to a string that is pointing into the "val" (a/k/a "v") Python
object, which PLy_traceback has carefully released the last refcount on.
So this coding should have failed immediately under any valgrind
testing.  The ereport patch perhaps gave us more scope for the error,
but for me, valgrind'ing the plpython tests fails similarly in 9.5.
So we should have noticed this long since.

I dislike bugs that are platform-dependent with no obvious
reason for it :-(

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] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 10/04/16 20:47, Christian Ullrich wrote:

* Andrew Dunstan:


On 04/09/2016 08:43 AM, Christian Ullrich wrote:



Michael Paquier wrote:



I don't think that's good to use malloc in those code paths, and I


Oh?

+#if (_MSC_VER >= 1900)
+uint32cp;
+
+if (GetLocaleInfoEx(ctype,
+LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
+(LPWSTR) , sizeof(cp) / sizeof(WCHAR)) > 0)
+{
+r = malloc(16);/* excess */
+if (r != NULL)
+sprintf(r, "CP%u", cp);
+}
+else
+{
+#endif



But r is return value so it has to be allocated. The intermediate 
variables are function local.



don't think we need to be too precious about saving a byte or two
here. Can one or other of you please prepare a replacement patch based
in this discussion?


Sorry, I don't think I'm up to that (at least not for another week or
so). I have to read up on the issue first; right now I'm not sure what
exactly the problem is.

What I can say is that the existing patch needs more work, because
GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument,
but the patched win32_langinfo() is giving it a locale identifier
("German_Germany.1252"). At least it does for me.


That really depends on what you set in config/commandline. The current 
code supports both (that's why there is the else fallback to old code 
which handles the "German_Germany.1252" format).



As for missing code page information in the _locale_t type, ISTM it
isn't hidden any better in UCRT than it was before:

int main()
{
 /* Set locale with nonstandard code page */
 _locale_t loc = _create_locale(LC_ALL, "German_Germany.850");

 __crt_locale_data_public* pub =
(__crt_locale_data_public*)(loc->locinfo);
 printf("CP: %d\n", pub->_locale_lc_codepage);// "CP: 850"
 return 0;
}



This is basically same as the approach of fixing _locale_t that was also 
considered upthread but nobody was too happy with it. I guess the worry 
is that given that the header file is obviously unfinished in the area 
where this is defined and also the fact that this looks like something 
that's not meant to be used outside of that header makes people worry 
that it might change between point releases of the SDK.


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


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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Amit Kapila]

On Sun, Apr 10, 2016 at 6:15 PM, Amit Kapila 
wrote:

> On Sun, Apr 10, 2016 at 11:10 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Sun, Apr 10, 2016 at 7:26 AM, Amit Kapila 
>> wrote:
>>
>>> On Sun, Apr 10, 2016 at 1:13 AM, Andres Freund 
>>> wrote:
>>>
 On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
 > There are results with 5364b357 reverted.


>>> What exactly is this test?
>>> I think assuming it is a read-only -M prepared pgbench run where data
>>> fits in shared buffers.  However if you can share exact details, then I can
>>> try the similar test.
>>>
>>
>> Yes, the test is:
>>
>> pgbench -s 1000 -c $clients -j 100 -M prepared -S -T 300
>> (shared_buffers=24GB)
>>
>>
>>>
 Crazy that this has such a negative impact. Amit, can you reproduce
 that?
>>>
>>>
>>> I will try it.
>>>
>>
>> Good.
>>
>
> Okay, I have done some performance testing of read-only tests with
> configuration suggested by you to see the impact
>
> pin_unpin - latest version of pin unpin patch on top of HEAD.
> pin_unpin_clog_32 - pin_unpin + change clog buffers to 32
>
> Client_Count/Patch_ver 64 128
> pin_unpin 330280 133586
> pin_unpin_clog_32 388244 132388
>
>
> This shows that at 64 client count, the performance is better with 32 clog
> buffers.  However, I think this is more attributed towards the fact that
> contention seems to shifted to procarraylock as to an extent indicated in
> Alexandar's mail.  I will try once with cache the snapshot patch as well
> and with clog buffers as 64.
>
>
I went ahead and tried with Cache the snapshot patch and with clog buffers
as 64 and below is performance data:

Description of patches

pin_unpin - latest version of pin unpin patch on top of HEAD.
pin_unpin_clog_32 - pin_unpin + change clog buffers to 32
pin_unpin_cache_snapshot - pin_unpin + Cache the snapshot
pin_unpin_clog_64 - pin_unpin + change clog buffers to 64


Client_Count/Patch_ver 64 128
pin_unpin 330280 133586
pin_unpin_clog_32 388244 132388
pin_unpin_cache_snapshot 412149 144799
pin_unpin_clog_64 391472 132951


Above data seems to indicate that cache the snapshot patch will make
performance go further up with clog buffers as 128 (HEAD).  I will take the
performance data with pin_unpin + clog buffers as 32 + cache the snapshot,
but above seems a good enough indication that making clog buffers as 128 is
a good move considering we will one day improve GetSnapshotData either by
Cache the snapshot technique or some other way.   Also making clog buffers
as 64 instead of 128 seems to address the regression (at the very least in
above tests), but for read-write performance, clog buffers as 128 has
better numbers, though the difference between 64 and 128 is not very high.


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan:


On 04/09/2016 08:43 AM, Christian Ullrich wrote:



Michael Paquier wrote:



I don't think that's good to use malloc in those code paths, and I


Oh?

+#if (_MSC_VER >= 1900)
+   uint32  cp;
+
+   if (GetLocaleInfoEx(ctype,
+   LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
+   (LPWSTR) , sizeof(cp) / sizeof(WCHAR)) > 0)
+   {
+   r = malloc(16); /* excess */
+   if (r != NULL)
+   sprintf(r, "CP%u", cp);
+   }
+   else
+   {
+#endif


I don't think we need to be too precious about saving a byte or two
here. Can one or other of you please prepare a replacement patch based
in this discussion?


Sorry, I don't think I'm up to that (at least not for another week or 
so). I have to read up on the issue first; right now I'm not sure what 
exactly the problem is.


What I can say is that the existing patch needs more work, because 
GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument, 
but the patched win32_langinfo() is giving it a locale identifier 
("German_Germany.1252"). At least it does for me.


I have not gone through the code sufficiently closely to find out where 
the argument to win32_langinfo() originates, but I will when I can.


As for missing code page information in the _locale_t type, ISTM it 
isn't hidden any better in UCRT than it was before:


int main()
{
/* Set locale with nonstandard code page */
_locale_t loc = _create_locale(LC_ALL, "German_Germany.850");

__crt_locale_data_public* pub = 
(__crt_locale_data_public*)(loc->locinfo);

printf("CP: %d\n", pub->_locale_lc_codepage);  // "CP: 850"
return 0;
}

--
Christian



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

Re: [HACKERS] multivariate statistics v14

[Tomas Vondra]

On 04/10/2016 10:25 AM, Simon Riggs wrote:

On 9 April 2016 at 18:37, Tatsuo Ishii > wrote:

> But I still think it wouldn't move the patch any closer to committable
> state, because what it really needs is review whether the catalog
> definition makes sense, whether it should be more like pg_statistic,
> and so on. Only then it makes sense to describe the catalog structure
> in the SGML docs, I think. That's why I added some basic SGML docs for
> CREATE/DROP/ALTER STATISTICS, which I expect to be rather stable, and
> not the catalog and other low-level stuff (which is commented heavily
> in the code anyway).

Without "user-level docs" (now I understand that the term means all
SGML docs for you), it is very hard to find a visible
characteristics/behavior of the patch. CREATE/DROP/ALTER STATISTICS
just defines a user interface, and does not help how it affects to the
planning. The READMEs do not help either.

In this case reviewing your code is something like reviewing a program
which has no specification.

That's the reason why I said before below, but it was never seriously
considered.


I would likely have said this myself but didn't even get that far.

Your contribution was useful and went further than anybody else's
review, so thank you.


100% agreed. Thanks for the useful feedback.

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


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

Re: [HACKERS] multivariate statistics v14

[Tomas Vondra]

Hello,

On 04/09/2016 07:37 PM, Tatsuo Ishii wrote:

But I still think it wouldn't move the patch any closer to committable
state, because what it really needs is review whether the catalog
definition makes sense, whether it should be more like pg_statistic,
and so on. Only then it makes sense to describe the catalog structure
in the SGML docs, I think. That's why I added some basic SGML docs for
CREATE/DROP/ALTER STATISTICS, which I expect to be rather stable, and
not the catalog and other low-level stuff (which is commented heavily
in the code anyway).


Without "user-level docs" (now I understand that the term means all
SGML docs for you), it is very hard to find a visible
characteristics/behavior of the patch. CREATE/DROP/ALTER STATISTICS
just defines a user interface, and does not help how it affects to
the planning. The READMEs do not help either.

In this case reviewing your code is something like reviewing a
program which has no specification.


I certainly agree that reviewing a patch without the context is hard. My 
intent was to provide such context / explanation in the READMEs, but 
perhaps I failed to do so with enough detail.


BTW when you say that READMEs do not help either, does that mean you 
consider READMEs unsuitable for this type of information in general, or 
that the current READMEs lack important information?




That's the reason why I said before below, but it was never
seriously considered.

>

I've considered it, but my plan was to have detailed READMEs, and then 
eventually distill that into something suitable for the SGML (perhaps 
without discussion of some implementation details). Maybe that's not the 
right approach.


FWIW providing the context is why I started working on a "paper" 
explaining both the motivation and implementation, including a bit of 
math and figures (which is what we don't have in READMEs or SGML). I 
haven't updated it recently, and it probably got buried in the thread, 
but perhaps this would be a better way to provide the context?


regards

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


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

Re: [HACKERS] Regression test CREATE USER/ROLE usage

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> broken that way as rowsecurity.sql, which is (still) creating roles
> >> named "alice" and "bob", but it's not acceptable like this.
> 
> > Attached is a patch to fix all of the role usage in rowsecurity.sql
> > (I believe, feel free to let me know if there's anything else).  In
> > particular, all of the roles are changed to begin with 'regress_', and
> > they are all now created with NOLOGIN.
> 
> +1 for the general idea, although there's something to be said for
> using names like 'regress_alice' and 'regress_bob' in this context.
> 'regress_user1' and 'regress_user2' are awfully gray and same-y,
> and therefore a bit error-prone IMO.

Fair enough.

I'll adjust the ending '_userX' to be actual names along the lines of
'alice', 'bob', 'carol', 'dave', 'eve', 'frank', etc.

(List pulled from https://en.wikipedia.org/wiki/Alice_and_Bob).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Regression test CREATE USER/ROLE usage

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> broken that way as rowsecurity.sql, which is (still) creating roles
>> named "alice" and "bob", but it's not acceptable like this.

> Attached is a patch to fix all of the role usage in rowsecurity.sql
> (I believe, feel free to let me know if there's anything else).  In
> particular, all of the roles are changed to begin with 'regress_', and
> they are all now created with NOLOGIN.

+1 for the general idea, although there's something to be said for
using names like 'regress_alice' and 'regress_bob' in this context.
'regress_user1' and 'regress_user2' are awfully gray and same-y,
and therefore a bit error-prone IMO.

regards, tom lane


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

[HACKERS] Regression test CREATE USER/ROLE usage

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> broken that way as rowsecurity.sql, which is (still) creating roles
> named "alice" and "bob", but it's not acceptable like this.

Attached is a patch to fix all of the role usage in rowsecurity.sql
(I believe, feel free to let me know if there's anything else).  In
particular, all of the roles are changed to begin with 'regress_', and
they are all now created with NOLOGIN.

I'll plan to push this in the next day or so.

I'll also work up a patch for the rest of the CREATE USER/ROLE usage in
the core regression tests.

This will result in a bit of not-strictly-necessary code churn, but
having consistency in the code base really is valuable where we have an
agreed upon policy as to what all the tests should be doing, for new
(and even old) developers to follow.

Comments and concerns welcome, of course.

Thanks!

Stephen
From 1c09f28d20582f62b386004b67a415f170af5fe6 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Sun, 10 Apr 2016 13:34:19 -0400
Subject: [PATCH] Prefix RLS regression test roles with 'regress_'

To avoid any possible overlap with existing roles on a system when
doing a 'make installcheck', use role names which start with
'regress_'.

Pointed out by Tom.
---
 src/test/regress/expected/rowsecurity.out | 752 +++---
 src/test/regress/sql/rowsecurity.sql  | 452 +-
 2 files changed, 602 insertions(+), 602 deletions(-)

diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 067aa8d..c76f6b7 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -4,43 +4,43 @@
 -- Clean up in case a prior regression run failed
 -- Suppress NOTICE messages when users/groups don't exist
 SET client_min_messages TO 'warning';
-DROP USER IF EXISTS rls_regress_user0;
-DROP USER IF EXISTS rls_regress_user1;
-DROP USER IF EXISTS rls_regress_user2;
-DROP USER IF EXISTS rls_regress_exempt_user;
-DROP ROLE IF EXISTS rls_regress_group1;
-DROP ROLE IF EXISTS rls_regress_group2;
-DROP SCHEMA IF EXISTS rls_regress_schema CASCADE;
+DROP USER IF EXISTS regress_rls_user0;
+DROP USER IF EXISTS regress_rls_user1;
+DROP USER IF EXISTS regress_rls_user2;
+DROP USER IF EXISTS regress_rls_exempt_user;
+DROP ROLE IF EXISTS regress_rls_group1;
+DROP ROLE IF EXISTS regress_rls_group2;
+DROP SCHEMA IF EXISTS regress_rls_schema CASCADE;
 RESET client_min_messages;
 -- initial setup
-CREATE USER rls_regress_user0;
-CREATE USER rls_regress_user1;
-CREATE USER rls_regress_user2;
-CREATE USER rls_regress_exempt_user BYPASSRLS;
-CREATE ROLE rls_regress_group1 NOLOGIN;
-CREATE ROLE rls_regress_group2 NOLOGIN;
-GRANT rls_regress_group1 TO rls_regress_user1;
-GRANT rls_regress_group2 TO rls_regress_user2;
-CREATE SCHEMA rls_regress_schema;
-GRANT ALL ON SCHEMA rls_regress_schema to public;
-SET search_path = rls_regress_schema;
+CREATE USER regress_rls_user0 NOLOGIN;
+CREATE USER regress_rls_user1 NOLOGIN;
+CREATE USER regress_rls_user2 NOLOGIN;
+CREATE USER regress_rls_exempt_user BYPASSRLS NOLOGIN;
+CREATE ROLE regress_rls_group1 NOLOGIN;
+CREATE ROLE regress_rls_group2 NOLOGIN;
+GRANT regress_rls_group1 TO regress_rls_user1;
+GRANT regress_rls_group2 TO regress_rls_user2;
+CREATE SCHEMA regress_rls_schema;
+GRANT ALL ON SCHEMA regress_rls_schema to public;
+SET search_path = regress_rls_schema;
 -- setup of malicious function
 CREATE OR REPLACE FUNCTION f_leak(text) RETURNS bool
 COST 0.001 LANGUAGE plpgsql
 AS 'BEGIN RAISE NOTICE ''f_leak => %'', $1; RETURN true; END';
 GRANT EXECUTE ON FUNCTION f_leak(text) TO public;
 -- BASIC Row-Level Security Scenario
-SET SESSION AUTHORIZATION rls_regress_user0;
+SET SESSION AUTHORIZATION regress_rls_user0;
 CREATE TABLE uaccount (
 pguser  name primary key,
 seclv   int
 );
 GRANT SELECT ON uaccount TO public;
 INSERT INTO uaccount VALUES
-('rls_regress_user0', 99),
-('rls_regress_user1', 1),
-('rls_regress_user2', 2),
-('rls_regress_user3', 3);
+('regress_rls_user0', 99),
+('regress_rls_user1', 1),
+('regress_rls_user2', 2),
+('regress_rls_user3', 3);
 CREATE TABLE category (
 cidint primary key,
 cname  text
@@ -60,20 +60,20 @@ CREATE TABLE document (
 );
 GRANT ALL ON document TO public;
 INSERT INTO document VALUES
-( 1, 11, 1, 'rls_regress_user1', 'my first novel'),
-( 2, 11, 2, 'rls_regress_user1', 'my second novel'),
-( 3, 22, 2, 'rls_regress_user1', 'my science fiction'),
-( 4, 44, 1, 'rls_regress_user1', 'my first manga'),
-( 5, 44, 2, 'rls_regress_user1', 'my second manga'),
-( 6, 22, 1, 'rls_regress_user2', 'great science fiction'),
-( 7, 33, 2, 'rls_regress_user2', 'great technology book'),
-( 8, 44, 1, 'rls_regress_user2', 'great manga');
+( 1, 11, 1, 'regress_rls_user1', 'my first novel'),
+( 2, 11, 2, 'regress_rls_user1', 'my second novel'),
+( 3, 22, 2, 

Re: [HACKERS] [COMMITTERS] pgsql: Bloom index contrib module

[Tom Lane]

Noah Misch  writes:
> On Sat, Apr 09, 2016 at 10:08:01PM -0400, Tom Lane wrote:
>> Still, we've reached the most coverage this test can give us at 1000
>> rows, which still means it's wasting the last 99% of its runtime.

> If dropping the row count to 1000 shaves >500ms on your primary machine, +1
> for committing such a row count change.  This is exactly what I meant by
> "someone identifies a way to realize similar coverage with lower duration."
> Thanks for contributing this study.

Further testing with gcov showed that the max coverage point was reached
somewhere between 500 and 750 rows (I didn't bother to pin it down
exactly).  So I thought setting the table size to 1000 rows was probably
shaving things a bit too close; I made it 2000 rows instead.

I also added a few more test cases to improve the coverage beyond what
it was to start with, using a white-box approach of modifying the test
script until gcov said that it was hitting the areas it wasn't before.

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] Relax requirement for INTO with SELECT in pl/pgsql

[Pavel Stehule]

2016-04-10 18:49 GMT+02:00 David G. Johnston :

> On Sun, Apr 10, 2016 at 9:16 AM, Pavel Stehule 
> wrote:
>
>>
>>
>> 2016-04-10 17:49 GMT+02:00 David G. Johnston 
>> :
>>
>>> On Sun, Apr 10, 2016 at 1:13 AM, Pavel Stehule 
>>> wrote:
>>>
 Hi

 2016-03-21 22:13 GMT+01:00 Pavel Stehule :

> Hi
>
> 2016-03-21 21:24 GMT+01:00 Merlin Moncure :
>
>> Patch is trivial (see below), discussion is not :-).
>>
>> I see no useful reason to require INTO when returning data with
>> SELECT.  However, requiring queries to indicate not needing data via
>> PERFORM causes some annoyances:
>>
>> *) converting routines back and forth between pl/pgsql and pl/sql
>> requires needless busywork and tends to cause errors to be thrown at
>> runtime
>>
>> *) as much as possible, (keywords begin/end remain a problem),
>> pl/pgsql should be a superset of sql
>>
>> *) it's much more likely to be burned by accidentally forgetting to
>> swap in PERFORM than to accidentally leave in a statement with no
>> actionable target.  Even if you did so in the latter case, it stands
>> to reason you'd accidentally leave in the target variable, too.
>>
>> *) the PERFORM requirement hails from the days when only statements
>> starting with SELECT return data.  There is no PERFORM equivalent for
>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
>> might have a RETURNING clause that does something but not necessarily
>> want to place the result in a variable (for example passing to
>> volatile function).  Take a look at the errhint() clause below -- we
>> don't even have a suggestion in that case.
>>
>> This has come up before, and there was a fair amount of sympathy for
>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>> get a hearing on the issue -- thanks.  If we decide to move forward,
>> this would effectively deprecate PERFORM and the documentation will be
>> suitably modified as well.
>>
>
>
 here is another argument why this idea is not good.


 http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function

 Now, when people coming from T-SQL world use some T-SQL constructs,
 then usually the code should not work with the error "query has not
 destination for data ... "

 When PLpgSQL will be more tolerant, then their code will be executed
 without any error, but will not work.


>>> I would be inclined to require that DML returning tuples requires INTO
>>> while a SELECT does not.  Adding RETURNING is a deliberate user action that
>>> we can and probably should be conservative for.  Writing SELECT is default
>>> user behavior and is quite often used only for its side-effects.  Since SQL
>>> proper doesn't offer a means to distinguish between the two modes adding
>>> that distinction to pl/pgSQL, while useful, doesn't seem like something
>>> that has to be forced upon the user.
>>>
>>
>> It doesn't help - SELECT is most often used construct.
>>
>> We can be less strict for SELECT expr, but SELECT FROM should not be
>> allowed without INTO clause.
>>
>>
> ​SELECT perform_this_action_for_every_user(user_id) FROM usertable;
>
> I still only care about side-effects.
>
> The rule remains (becomes?) simple:  Use INTO if you need to capture the
> SQL value into a pl/pgSQL variable - otherwise don't.  WRT your prior post
> I'd tell the user they are doing something really unusual if they write
> INSERT RETURNING without INTO - which I have no problem doing.
>

This is the problem. Any other databases doesn't allow it - or it has
pretty different semantic (in T-SQL)

I am skeptical if benefit is higher than costs.


>
> We don't need to force the user to tell us they intentionally omitted the
> INTO clause.  The omission itself is sufficient.  Using select without a
> target pl/pgSQL variable is a valid and probably quite common construct and
> hindering it because it might make debugging a function a bit harder (wrong
> data instead of an error) doesn't seem worthwhile.  You are making
> accommodations for exceptional situations.  I'm not convinced that it will
> be significantly harder to spot a missing INTO in a world where one is
> allowed to write such a statement without PERFORM.  Yes, it will not be as
> convenient.  Its a usability trade-off.
>
> ​There is value in having the non-procedural aspects of pl/pgSQL be as
> close to pure SQL as possible.​
>

It is not valid (semantically)  - you cannot throw result in pure SQL


>
> ​I am not in a position to realistically judge the trade-offs involved
> here as it pertains to something learning the language.  I personally
> 

Re: [HACKERS] Relax requirement for INTO with SELECT in pl/pgsql

[David G. Johnston]

On Sun, Apr 10, 2016 at 9:16 AM, Pavel Stehule 
wrote:

>
>
> 2016-04-10 17:49 GMT+02:00 David G. Johnston :
>
>> On Sun, Apr 10, 2016 at 1:13 AM, Pavel Stehule 
>> wrote:
>>
>>> Hi
>>>
>>> 2016-03-21 22:13 GMT+01:00 Pavel Stehule :
>>>
 Hi

 2016-03-21 21:24 GMT+01:00 Merlin Moncure :

> Patch is trivial (see below), discussion is not :-).
>
> I see no useful reason to require INTO when returning data with
> SELECT.  However, requiring queries to indicate not needing data via
> PERFORM causes some annoyances:
>
> *) converting routines back and forth between pl/pgsql and pl/sql
> requires needless busywork and tends to cause errors to be thrown at
> runtime
>
> *) as much as possible, (keywords begin/end remain a problem),
> pl/pgsql should be a superset of sql
>
> *) it's much more likely to be burned by accidentally forgetting to
> swap in PERFORM than to accidentally leave in a statement with no
> actionable target.  Even if you did so in the latter case, it stands
> to reason you'd accidentally leave in the target variable, too.
>
> *) the PERFORM requirement hails from the days when only statements
> starting with SELECT return data.  There is no PERFORM equivalent for
> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
> might have a RETURNING clause that does something but not necessarily
> want to place the result in a variable (for example passing to
> volatile function).  Take a look at the errhint() clause below -- we
> don't even have a suggestion in that case.
>
> This has come up before, and there was a fair amount of sympathy for
> this argument albeit with some dissent -- notably Pavel.  I'd like to
> get a hearing on the issue -- thanks.  If we decide to move forward,
> this would effectively deprecate PERFORM and the documentation will be
> suitably modified as well.
>


>>> here is another argument why this idea is not good.
>>>
>>>
>>> http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function
>>>
>>> Now, when people coming from T-SQL world use some T-SQL constructs, then
>>> usually the code should not work with the error "query has not destination
>>> for data ... "
>>>
>>> When PLpgSQL will be more tolerant, then their code will be executed
>>> without any error, but will not work.
>>>
>>>
>> I would be inclined to require that DML returning tuples requires INTO
>> while a SELECT does not.  Adding RETURNING is a deliberate user action that
>> we can and probably should be conservative for.  Writing SELECT is default
>> user behavior and is quite often used only for its side-effects.  Since SQL
>> proper doesn't offer a means to distinguish between the two modes adding
>> that distinction to pl/pgSQL, while useful, doesn't seem like something
>> that has to be forced upon the user.
>>
>
> It doesn't help - SELECT is most often used construct.
>
> We can be less strict for SELECT expr, but SELECT FROM should not be
> allowed without INTO clause.
>
>
​SELECT perform_this_action_for_every_user(user_id) FROM usertable;

I still only care about side-effects.

The rule remains (becomes?) simple:  Use INTO if you need to capture the
SQL value into a pl/pgSQL variable - otherwise don't.  WRT your prior post
I'd tell the user they are doing something really unusual if they write
INSERT RETURNING without INTO - which I have no problem doing.

We don't need to force the user to tell us they intentionally omitted the
INTO clause.  The omission itself is sufficient.  Using select without a
target pl/pgSQL variable is a valid and probably quite common construct and
hindering it because it might make debugging a function a bit harder (wrong
data instead of an error) doesn't seem worthwhile.  You are making
accommodations for exceptional situations.  I'm not convinced that it will
be significantly harder to spot a missing INTO in a world where one is
allowed to write such a statement without PERFORM.  Yes, it will not be as
convenient.  Its a usability trade-off.

​There is value in having the non-procedural aspects of pl/pgSQL be as
close to pure SQL as possible.​

​I am not in a position to realistically judge the trade-offs involved here
as it pertains to something learning the language.  I personally haven't
found the need to specify PERFORM particularly problematic but I've also
never been bit by the inverse - specifying PERFORM when in fact I needed to
assign to a variable.  I guess my main point is I see no fundamental reason
to require a user to explicitly inform that they are omitting the INTO
clause but don't see that changing the status-quo will affect a significant
change in my quality of life.  My experiences are quite limited 

Re: [HACKERS] Relax requirement for INTO with SELECT in pl/pgsql

[Pavel Stehule]

2016-04-10 17:49 GMT+02:00 David G. Johnston :

> On Sun, Apr 10, 2016 at 1:13 AM, Pavel Stehule 
> wrote:
>
>> Hi
>>
>> 2016-03-21 22:13 GMT+01:00 Pavel Stehule :
>>
>>> Hi
>>>
>>> 2016-03-21 21:24 GMT+01:00 Merlin Moncure :
>>>
 Patch is trivial (see below), discussion is not :-).

 I see no useful reason to require INTO when returning data with
 SELECT.  However, requiring queries to indicate not needing data via
 PERFORM causes some annoyances:

 *) converting routines back and forth between pl/pgsql and pl/sql
 requires needless busywork and tends to cause errors to be thrown at
 runtime

 *) as much as possible, (keywords begin/end remain a problem),
 pl/pgsql should be a superset of sql

 *) it's much more likely to be burned by accidentally forgetting to
 swap in PERFORM than to accidentally leave in a statement with no
 actionable target.  Even if you did so in the latter case, it stands
 to reason you'd accidentally leave in the target variable, too.

 *) the PERFORM requirement hails from the days when only statements
 starting with SELECT return data.  There is no PERFORM equivalent for
 WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
 might have a RETURNING clause that does something but not necessarily
 want to place the result in a variable (for example passing to
 volatile function).  Take a look at the errhint() clause below -- we
 don't even have a suggestion in that case.

 This has come up before, and there was a fair amount of sympathy for
 this argument albeit with some dissent -- notably Pavel.  I'd like to
 get a hearing on the issue -- thanks.  If we decide to move forward,
 this would effectively deprecate PERFORM and the documentation will be
 suitably modified as well.

>>>
>>>
>> here is another argument why this idea is not good.
>>
>>
>> http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function
>>
>> Now, when people coming from T-SQL world use some T-SQL constructs, then
>> usually the code should not work with the error "query has not destination
>> for data ... "
>>
>> When PLpgSQL will be more tolerant, then their code will be executed
>> without any error, but will not work.
>>
>>
> I would be inclined to require that DML returning tuples requires INTO
> while a SELECT does not.  Adding RETURNING is a deliberate user action that
> we can and probably should be conservative for.  Writing SELECT is default
> user behavior and is quite often used only for its side-effects.  Since SQL
> proper doesn't offer a means to distinguish between the two modes adding
> that distinction to pl/pgSQL, while useful, doesn't seem like something
> that has to be forced upon the user.
>

It doesn't help - SELECT is most often used construct.

We can be less strict for SELECT expr, but SELECT FROM should not be
allowed without INTO clause.

It is reason, why I dislike this proposal, because the rules when INTO is
allowed, required will be more complex. Now, the rules are pretty simple -
and it is safe for beginners. I accept so current behave should be limiting
for experts.

Regards

Pavel


>
> On the last point I probably wouldn't bother to deprecate PERFORM for that
> reason, let alone the fact we likely would never choose to actually remove
> the capability.
>
> ​I'm not convinced that allowing RETURNING to be target-less is needed.
> With writable CTEs you can now get that capability by wrapping the DML in a
> target-less SELECT.  Again, coming back to "typical usage", I'd have no
> problem making something like "RETURNING func(col) INTO /dev/null" ​work
> for the exceptional cases that need returning but don't have any good
> variables to assign the values to and don't want to make some up just to
> ignore them.
>
> David J.
>
>

Re: [HACKERS] Relax requirement for INTO with SELECT in pl/pgsql

[David G. Johnston]

On Sun, Apr 10, 2016 at 1:13 AM, Pavel Stehule 
wrote:

> Hi
>
> 2016-03-21 22:13 GMT+01:00 Pavel Stehule :
>
>> Hi
>>
>> 2016-03-21 21:24 GMT+01:00 Merlin Moncure :
>>
>>> Patch is trivial (see below), discussion is not :-).
>>>
>>> I see no useful reason to require INTO when returning data with
>>> SELECT.  However, requiring queries to indicate not needing data via
>>> PERFORM causes some annoyances:
>>>
>>> *) converting routines back and forth between pl/pgsql and pl/sql
>>> requires needless busywork and tends to cause errors to be thrown at
>>> runtime
>>>
>>> *) as much as possible, (keywords begin/end remain a problem),
>>> pl/pgsql should be a superset of sql
>>>
>>> *) it's much more likely to be burned by accidentally forgetting to
>>> swap in PERFORM than to accidentally leave in a statement with no
>>> actionable target.  Even if you did so in the latter case, it stands
>>> to reason you'd accidentally leave in the target variable, too.
>>>
>>> *) the PERFORM requirement hails from the days when only statements
>>> starting with SELECT return data.  There is no PERFORM equivalent for
>>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
>>> might have a RETURNING clause that does something but not necessarily
>>> want to place the result in a variable (for example passing to
>>> volatile function).  Take a look at the errhint() clause below -- we
>>> don't even have a suggestion in that case.
>>>
>>> This has come up before, and there was a fair amount of sympathy for
>>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>>> get a hearing on the issue -- thanks.  If we decide to move forward,
>>> this would effectively deprecate PERFORM and the documentation will be
>>> suitably modified as well.
>>>
>>
>>
> here is another argument why this idea is not good.
>
>
> http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function
>
> Now, when people coming from T-SQL world use some T-SQL constructs, then
> usually the code should not work with the error "query has not destination
> for data ... "
>
> When PLpgSQL will be more tolerant, then their code will be executed
> without any error, but will not work.
>
>
I would be inclined to require that DML returning tuples requires INTO
while a SELECT does not.  Adding RETURNING is a deliberate user action that
we can and probably should be conservative for.  Writing SELECT is default
user behavior and is quite often used only for its side-effects.  Since SQL
proper doesn't offer a means to distinguish between the two modes adding
that distinction to pl/pgSQL, while useful, doesn't seem like something
that has to be forced upon the user.

On the last point I probably wouldn't bother to deprecate PERFORM for that
reason, let alone the fact we likely would never choose to actually remove
the capability.

​I'm not convinced that allowing RETURNING to be target-less is needed.
With writable CTEs you can now get that capability by wrapping the DML in a
target-less SELECT.  Again, coming back to "typical usage", I'd have no
problem making something like "RETURNING func(col) INTO /dev/null" ​work
for the exceptional cases that need returning but don't have any good
variables to assign the values to and don't want to make some up just to
ignore them.

David J.

Re: [HACKERS] pgbench - allow backslash-continuations in custom scripts

[Alvaro Herrera]

Tom Lane wrote:
> So I looked into this, and found that persuading psql to let backslash
> commands cross line boundaries is a much bigger deal than just fixing the
> lexer.  The problem is that MainLoop would need to grow an understanding
> of having received only a partial backslash command and needing to go back
> to readline() for another line.  And probably HandleSlashCmds would need
> to be changed to stop parsing and back out without doing anything when it
> hits backslash-newline.  It's do-able no doubt, but it's not going to be a
> small and simple patch.

FWIW, I would love to see this in some future release: particularly for
\copy lines with large queries, the limitation that only single-line
input is accepted is very annoying -- much more so when the query comes
pasted from some other input, or when you have a file with a query and
just want to add a quick \copy prefix.

(Hmm, a much simpler alternative would be to allow \g-like syntax, i.e.
the query is already in the query buffer and the \copy line just
specifies the output file.  In fact, for queries in history this is much
more convenient than the current syntax ...)

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/09/2016 08:43 AM, Christian Ullrich wrote:

Michael Paquier wrote:

On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
 wrote:

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich  wrote:

* Andrew Dunstan wrote:

On 04/08/2016 11:02 AM, Christian Ullrich wrote:
  src/port/chklocale.c(233): warning C4133: 'function': incompatible
  types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]

Do you have a fix for the LPCWSTR parameter issue?

As long as the locale short name cannot contain characters outside of ASCII,
and I don't see how it could, just the typical measure-allocate-convert
dance, add error handling to taste:

int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
WCHAR *wctype = malloc(res * sizeof(WCHAR));
memset(wctype, 0, res * sizeof(WCHAR));
res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);

I don't think that's good to use malloc in those code paths, and I
think that we cannot use palloc as well for a buffer passed directly
into this function, so it looks that we had better use a fix-sized
buffer and allocate the output into that. What would be a a correct
estimation of the maximum size we should allow? 80 (similar to
pg_locale.c)?

I think it should not take more than 16 characters, but if you want to make 
sure the code can deal with long names as well, MSDN gives an upper limit of 85 
for those somewhere.




I don't think we need to be too precious about saving a byte or two 
here. Can one or other of you please prepare a replacement patch based 
in this discussion?


cheers

andrew



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

[HACKERS] Parallel Aggregate costs don't consider combine/serial/deserial funcs

[David Rowley]

Hi,

I realised a few days ago that the parallel aggregate code does not
cost for the combine, serialisation and deserialisation functions at
all.

I've attached a patch which fixes this.

One small point which I was a little unsure of in the attached is,
should the "if (aggref->aggdirectargs)" part of
count_agg_clauses_walker() be within the "if
(!context->combineStates)". I simply couldn't decide. We currently
have no aggregates which this affects anyway, per; select * from
pg_aggregate where aggcombinefn <> 0 and aggkind <> 'n'; so for now
I've left it outwith.

Another thing I thought of is that it's not too nice that I have to
pass 3 bools to count_agg_clauses() in order to tell it what to do. I
was tempted to invent some bitmask flags for this, then modify
create_agg_path() to use the same flags, but I thought I'd better not
cause too much churn with this patch.

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


parallel_aggs_costs_fix.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] Move PinBuffer and UnpinBuffer to atomics

[Amit Kapila]

On Sun, Apr 10, 2016 at 11:10 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Sun, Apr 10, 2016 at 7:26 AM, Amit Kapila 
> wrote:
>
>> On Sun, Apr 10, 2016 at 1:13 AM, Andres Freund 
>> wrote:
>>
>>> On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
>>> > There are results with 5364b357 reverted.
>>>
>>>
>> What exactly is this test?
>> I think assuming it is a read-only -M prepared pgbench run where data
>> fits in shared buffers.  However if you can share exact details, then I can
>> try the similar test.
>>
>
> Yes, the test is:
>
> pgbench -s 1000 -c $clients -j 100 -M prepared -S -T 300
> (shared_buffers=24GB)
>
>
>>
>>> Crazy that this has such a negative impact. Amit, can you reproduce
>>> that?
>>
>>
>> I will try it.
>>
>
> Good.
>

Okay, I have done some performance testing of read-only tests with
configuration suggested by you to see the impact

pin_unpin - latest version of pin unpin patch on top of HEAD.
pin_unpin_clog_32 - pin_unpin + change clog buffers to 32

Client_Count/Patch_ver 64 128
pin_unpin 330280 133586
pin_unpin_clog_32 388244 132388


This shows that at 64 client count, the performance is better with 32 clog
buffers.  However, I think this is more attributed towards the fact that
contention seems to shifted to procarraylock as to an extent indicated in
Alexandar's mail.  I will try once with cache the snapshot patch as well
and with clog buffers as 64.


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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Amit Kapila]

On Sun, Apr 10, 2016 at 11:33 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Sun, Apr 10, 2016 at 8:36 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Sat, Apr 9, 2016 at 10:49 PM, Andres Freund 
>> wrote:
>>
>>>
>>>
>>> On April 9, 2016 12:43:03 PM PDT, Andres Freund 
>>> wrote:
>>> >On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
>>> >> There are results with 5364b357 reverted.
>>> >
>>> >Crazy that this has such a negative impact. Amit, can you reproduce
>>> >that? Alexander, I guess for r/w workload 5364b357 is a benefit on that
>>> >machine as well?
>>>
>>> How sure are you about these measurements?
>>
>>
>> I'm pretty sure.  I've retried it multiple times by hand before re-run
>> the script.
>>
>>
>>> Because there really shouldn't be clog lookups one a steady state is
>>> reached...
>>>
>>
>> Hm... I'm also surprised. There shouldn't be clog lookups once hint bits
>> are set.
>>
>
> I also tried to run perf top during pgbench and get some interesting
> results.
>
> Without 5364b357:
>5,69%  postgres [.] GetSnapshotData
>4,47%  postgres [.] LWLockAttemptLock
>3,81%  postgres [.] _bt_compare
>3,42%  postgres [.] hash_search_with_hash_value
>3,08%  postgres [.] LWLockRelease
>2,49%  postgres [.] PinBuffer.isra.3
>1,58%  postgres [.] AllocSetAlloc
>1,17%  [kernel] [k] __schedule
>1,15%  postgres [.] PostgresMain
>1,13%  libc-2.17.so [.] vfprintf
>1,01%  libc-2.17.so [.] __memcpy_ssse3_back
>
> With 5364b357:
>   18,54%  postgres [.] GetSnapshotData
>3,45%  postgres [.] LWLockRelease
>3,27%  postgres [.] LWLockAttemptLock
>3,21%  postgres [.] _bt_compare
>2,93%  postgres [.] hash_search_with_hash_value
>2,00%  postgres [.] PinBuffer.isra.3
>1,32%  postgres [.] AllocSetAlloc
>1,10%  libc-2.17.so [.] vfprintf
>
> Very surprising.  It appears that after 5364b357, GetSnapshotData consumes
> more time.  But I can't see anything depending on clog buffers
> in GetSnapshotData code...
>

There is a related fact presented by Mithun C Y as well [1] which suggests
that Andres's idea of reducing the cost of snapshot shows noticeable gain
after increasing the clog buffers.  If you read that thread you will notice
that initially we didn't notice much gain by that idea, but with increased
clog buffers, it started showing noticeable gain.  If by any chance, you
can apply that patch and see the results (latest patch is at [2]).


[1] -
http://www.postgresql.org/message-id/CAD__Ouic1Tvnwqm6Wf6j7Cz1Kk1DQgmy0isC7=ogx+3jtfg...@mail.gmail.com

[2] -
http://www.postgresql.org/message-id/cad__ouiwei5she2wwqck36ac9qmhvjuqg3cfpn+ofcmb7rd...@mail.gmail.com


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

Re: [HACKERS] multivariate statistics v14

[Simon Riggs]

On 9 April 2016 at 18:37, Tatsuo Ishii  wrote:

> > But I still think it wouldn't move the patch any closer to committable
> > state, because what it really needs is review whether the catalog
> > definition makes sense, whether it should be more like pg_statistic,
> > and so on. Only then it makes sense to describe the catalog structure
> > in the SGML docs, I think. That's why I added some basic SGML docs for
> > CREATE/DROP/ALTER STATISTICS, which I expect to be rather stable, and
> > not the catalog and other low-level stuff (which is commented heavily
> > in the code anyway).
>
> Without "user-level docs" (now I understand that the term means all
> SGML docs for you), it is very hard to find a visible
> characteristics/behavior of the patch. CREATE/DROP/ALTER STATISTICS
> just defines a user interface, and does not help how it affects to the
> planning. The READMEs do not help either.
>
> In this case reviewing your code is something like reviewing a program
> which has no specification.
>
> That's the reason why I said before below, but it was never seriously
> considered.
>

I would likely have said this myself but didn't even get that far.

Your contribution was useful and went further than anybody else's review,
so thank you.

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

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

Re: [HACKERS] Relax requirement for INTO with SELECT in pl/pgsql

[Pavel Stehule]

Hi

2016-03-21 22:13 GMT+01:00 Pavel Stehule :

> Hi
>
> 2016-03-21 21:24 GMT+01:00 Merlin Moncure :
>
>> Patch is trivial (see below), discussion is not :-).
>>
>> I see no useful reason to require INTO when returning data with
>> SELECT.  However, requiring queries to indicate not needing data via
>> PERFORM causes some annoyances:
>>
>> *) converting routines back and forth between pl/pgsql and pl/sql
>> requires needless busywork and tends to cause errors to be thrown at
>> runtime
>>
>> *) as much as possible, (keywords begin/end remain a problem),
>> pl/pgsql should be a superset of sql
>>
>> *) it's much more likely to be burned by accidentally forgetting to
>> swap in PERFORM than to accidentally leave in a statement with no
>> actionable target.  Even if you did so in the latter case, it stands
>> to reason you'd accidentally leave in the target variable, too.
>>
>> *) the PERFORM requirement hails from the days when only statements
>> starting with SELECT return data.  There is no PERFORM equivalent for
>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
>> might have a RETURNING clause that does something but not necessarily
>> want to place the result in a variable (for example passing to
>> volatile function).  Take a look at the errhint() clause below -- we
>> don't even have a suggestion in that case.
>>
>> This has come up before, and there was a fair amount of sympathy for
>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>> get a hearing on the issue -- thanks.  If we decide to move forward,
>> this would effectively deprecate PERFORM and the documentation will be
>> suitably modified as well.
>>
>
>
here is another argument why this idea is not good.

http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function

Now, when people coming from T-SQL world use some T-SQL constructs, then
usually the code should not work with the error "query has not destination
for data ... "

When PLpgSQL will be more tolerant, then their code will be executed
without any error, but will not work.

Regards

Pavel



> My negative opinion is known. The PERFORM statement is much more
> workaround than well designed statement, but I would to see ANSI/SQL based
> fix. I try to compare benefits and loss.
>
> Can you start with analyze what is possible, and what semantic is allowed
> in standard and other well known SQL databases?
>
> Regards
>
> Pavel
>
>
>>
>> merlin
>>
>>
>>
>> diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
>> index b7f44ca..a860066 100644
>> --- a/src/pl/plpgsql/src/pl_exec.c
>> +++ b/src/pl/plpgsql/src/pl_exec.c
>> @@ -3457,12 +3457,9 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
>> }
>> else
>> {
>> -   /* If the statement returned a tuple table, complain */
>> +   /* If the statement returned a tuple table, free it. */
>> if (SPI_tuptable != NULL)
>> -   ereport(ERROR,
>> -   (errcode(ERRCODE_SYNTAX_ERROR),
>> -errmsg("query has no destination for result data"),
>> -(rc == SPI_OK_SELECT) ? errhint("If you want to
>> discard the results of a SELECT, use PERFORM instead.") : 0));
>> +   SPI_freetuptable(SPI_tuptable);
>> }
>>
>> if (paramLI)
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

Re: [HACKERS] [COMMITTERS] pgsql: Bloom index contrib module

[Alexander Korotkov]

On Sun, Apr 10, 2016 at 9:01 AM, Noah Misch  wrote:

> On Sat, Apr 09, 2016 at 10:08:01PM -0400, Tom Lane wrote:
> > I wrote:
> > > I was depressed, though not entirely surprised, to find that you get
> > > exactly that same line-count coverage if the table size is cut back
> > > to ONE row.
> >
> > Oh, I found the flaw in my testing: there are two INSERTs in the test
> > script and I was changing only one of them.  After correcting that,
> > the results behave a little more sanely:
> >
> >Line Coverage   Functions
> > 1 row: 70.4 % 349 / 496   93.1 %  27 / 29
> > 10 row:73.6 % 365 / 496   93.1 %  27 / 29
> > 100 rows:  73.6 % 365 / 496   93.1 %  27 / 29
> > 1000 rows: 75.4 % 374 / 496   93.1 %  27 / 29
> >
> > Still, we've reached the most coverage this test can give us at 1000
> > rows, which still means it's wasting the last 99% of its runtime.
>
> If dropping the row count to 1000 shaves >500ms on your primary machine, +1
> for committing such a row count change.  This is exactly what I meant by
> "someone identifies a way to realize similar coverage with lower duration."
> Thanks for contributing this study.


+1, row count reduction is a good to reduce regression test time.

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

Re: [HACKERS] Odd system-column handling in postgres_fdw join pushdown patch

[Noah Misch]

On Wed, Apr 06, 2016 at 01:14:34AM -0400, Noah Misch wrote:
> On Tue, Apr 05, 2016 at 03:41:00PM +0900, Etsuro Fujita wrote:
> > On 2016/03/29 15:37, Etsuro Fujita wrote:
> > >I added two helper functions: GetFdwScanTupleExtraData and
> > >FillFdwScanTupleSysAttrs.  The FDW author could use the former to get
> > >info about system attributes other than ctids and oids in fdw_scan_tlist
> > >during BeginForeignScan, and the latter to set values for these system
> > >attributes during IterateForeignScan (InvalidTransactionId for
> > >xmins/xmaxs, InvalidCommandId for cmins/cmaxs, and valid values for
> > >tableoids).  Attached is a proposed patch for that.  I also slightly
> > >simplified the changes to make_tuple_from_result_row and
> > >conversion_error_callback made by the postgres_fdw join pushdown patch.
> > >  What do you think about that?
> > 
> > I revised comments a little bit.  Attached is an updated version of the
> > patch.  I think this issue should be fixed in advance of the PostgreSQL
> > 9.6beta1 release.
> 
> Of the foreign table columns affected here, I bet only tableoid sees
> non-negligible use.  Even tableoid is not very prominent, so I would not have
> thought of this as a beta1 blocker.  What about this bug makes pre-beta1
> resolution especially valuable?

This will probably get resolved earlier if it enters the process now as a non
beta blocker, compared to entering the process later as a beta blocker.  I'm
taking the liberty of doing that:

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this.  Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution.  Please
present, within 72 hours, a plan to fix the defect within seven days of this
message.  Thanks.


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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Alexander Korotkov]

On Sun, Apr 10, 2016 at 8:36 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Sat, Apr 9, 2016 at 10:49 PM, Andres Freund  wrote:
>
>>
>>
>> On April 9, 2016 12:43:03 PM PDT, Andres Freund 
>> wrote:
>> >On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
>> >> There are results with 5364b357 reverted.
>> >
>> >Crazy that this has such a negative impact. Amit, can you reproduce
>> >that? Alexander, I guess for r/w workload 5364b357 is a benefit on that
>> >machine as well?
>>
>> How sure are you about these measurements?
>
>
> I'm pretty sure.  I've retried it multiple times by hand before re-run the
> script.
>
>
>> Because there really shouldn't be clog lookups one a steady state is
>> reached...
>>
>
> Hm... I'm also surprised. There shouldn't be clog lookups once hint bits
> are set.
>

I also tried to run perf top during pgbench and get some interesting
results.

Without 5364b357:
   5,69%  postgres [.] GetSnapshotData
   4,47%  postgres [.] LWLockAttemptLock
   3,81%  postgres [.] _bt_compare
   3,42%  postgres [.] hash_search_with_hash_value
   3,08%  postgres [.] LWLockRelease
   2,49%  postgres [.] PinBuffer.isra.3
   1,58%  postgres [.] AllocSetAlloc
   1,17%  [kernel] [k] __schedule
   1,15%  postgres [.] PostgresMain
   1,13%  libc-2.17.so [.] vfprintf
   1,01%  libc-2.17.so [.] __memcpy_ssse3_back

With 5364b357:
  18,54%  postgres [.] GetSnapshotData
   3,45%  postgres [.] LWLockRelease
   3,27%  postgres [.] LWLockAttemptLock
   3,21%  postgres [.] _bt_compare
   2,93%  postgres [.] hash_search_with_hash_value
   2,00%  postgres [.] PinBuffer.isra.3
   1,32%  postgres [.] AllocSetAlloc
   1,10%  libc-2.17.so [.] vfprintf

Very surprising.  It appears that after 5364b357, GetSnapshotData consumes
more time.  But I can't see anything depending on clog buffers
in GetSnapshotData code...

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

Re: [HACKERS] [COMMITTERS] pgsql: Bloom index contrib module

[Noah Misch]

On Sat, Apr 09, 2016 at 10:08:01PM -0400, Tom Lane wrote:
> I wrote:
> > I was depressed, though not entirely surprised, to find that you get
> > exactly that same line-count coverage if the table size is cut back
> > to ONE row.
> 
> Oh, I found the flaw in my testing: there are two INSERTs in the test
> script and I was changing only one of them.  After correcting that,
> the results behave a little more sanely:
> 
>Line Coverage   Functions
> 1 row: 70.4 % 349 / 496   93.1 %  27 / 29
> 10 row:73.6 % 365 / 496   93.1 %  27 / 29
> 100 rows:  73.6 % 365 / 496   93.1 %  27 / 29
> 1000 rows: 75.4 % 374 / 496   93.1 %  27 / 29
> 
> Still, we've reached the most coverage this test can give us at 1000
> rows, which still means it's wasting the last 99% of its runtime.

If dropping the row count to 1000 shaves >500ms on your primary machine, +1
for committing such a row count change.  This is exactly what I meant by
"someone identifies a way to realize similar coverage with lower duration."
Thanks for contributing this study.


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