Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat

[Amit Kapila]

On Sat, May 28, 2016 at 5:06 AM, Tom Lane  wrote:
>
> Alvaro Herrera  writes:
> > Regarding this:
> >> + * XXX this certainly looks useless, why not just wait
indefinitely?
>
> > There's another select_loop() in vacuumdb.c suggesting that the timeout
> > is used to check for cancel requests; as I understood while working on
> > the vacuumdb changes, select() is not interrupted in that case.  I
> > suppose that means it's necessary here also.  But on the other hand it's
> > quite possible that the original developer just copied what was in
> > pg_dump and that it's not actually needed; if that's the case, perhaps
> > it's better to rip it out from both places.
>
> Ah, interesting.  That ties into something else I was wondering about,
> which is how we could get useful control-C cancellation on Windows.
> It looks like the vacuumdb.c version of this code actually is tied
> into an interrupt handler, but whoever copied it for parallel.c just
> ripped out the CancelRequested checks, making the looping behavior
> pretty useless.
>

It seems to me that CancelRequested checks were introduced in vacuumdb.c as
part of commit a1792320 and select_loop for parallel.c version exists from
commit 9e257a18 which got committed earlier.  I think control-C handling
for Windows in parallel.c is missing or if there is some way to deal with
it, clearly it is not same as what we do in vacuumdb.c.

> For pg_restore (parallel or not) it would be useful if the program
> didn't just fall over at control-C but actually sent cancel requests
> to the backend(s).  It's not such a problem if we're transferring
> data, but if we're waiting for some slow operation like CREATE INDEX,
> the current behavior isn't very good.  On the Unix side we have some
> SIGINT infrastructure there already, but I don't see any for Windows.
>
> So now I'm thinking we should leave that alone, with the expectation
> that we'll be putting CancelRequested checks back in at some point.
>
> >
https://www.postgresql.org/message-id/20150122174601.gb1...@alvh.no-ip.org
>
> Hmm, did the patch you're discussing there get committed?
>

Yes, it was committed - a1792320



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

Re: [HACKERS] Hard to maintain duplication in contain_volatile_functions_not_nextval_walker

[Amit Kapila]

On Sat, May 28, 2016 at 12:28 AM, Andres Freund  wrote:
>
> Hi,
>
> contain_volatile_functions_walker is duplicated, near entirely, in
> contain_volatile_functions_not_nextval_walker.
>

Previously, I also had same observation.

> Wouldn't it have been better not to duplicate, and keep a flag about
> ignoring nextval in the context variable?
>

makes sense.  +1 for doing it in the way as you are suggesting.


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

Re: [HACKERS] [GENERAL] Permission Denied Error on pg_xlog/RECOVERYXLOG file

[Andres Freund]

On 2016-05-27 20:54:43 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-05-26 12:44:51 -0400, Tom Lane wrote:
> > 2016-04-27 17:02:06 EDT 572128cd.1811 [7-1] user=,db=,remote= FATAL:  42501:
> > could not open file "pg_xlog/RECOVERYXLOG": Permission denied
> 
> > So, what's the permission of RECOVERYXLOG at that point?  It's pretty
> > weird that directly after running reason_command it's not readable.
> 
> s/not readable/not writable/.  I doubt that it's a good idea for that
> code to think that it can fail hard on non-writable files.

But we actually sometimes write to files we've recovered; if they're the
end of the WAL after archive recovery and/or promotion. If a
restore_command restores files in a non-writable way it's buggy; I don't
see why it's worthwhile to work around that.

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] [GENERAL] Permission Denied Error on pg_xlog/RECOVERYXLOG file

[Tom Lane]

Andres Freund  writes:
> On 2016-05-26 12:44:51 -0400, Tom Lane wrote:
> 2016-04-27 17:02:06 EDT 572128cd.1811 [7-1] user=,db=,remote= FATAL:  42501:
> could not open file "pg_xlog/RECOVERYXLOG": Permission denied

> So, what's the permission of RECOVERYXLOG at that point?  It's pretty
> weird that directly after running reason_command it's not readable.

s/not readable/not writable/.  I doubt that it's a good idea for that
code to think that it can fail hard on non-writable files.

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] [GENERAL] Permission Denied Error on pg_xlog/RECOVERYXLOG file

[Andres Freund]

On 2016-05-26 12:44:51 -0400, Tom Lane wrote:
> > 2016-04-27 17:02:06 EDT 572128cd.1811 [7-1] user=,db=,remote= FATAL:  42501:
> > could not open file "pg_xlog/RECOVERYXLOG": Permission denied

So, what's the permission of RECOVERYXLOG at that point?  It's pretty
weird that directly after running reason_command it's not readable. Are
you doing something involving sudo or such in restore_command?


> The proximate cause of this might just be that the "ignore_perm" exception
> is only for EACCES and not EPERM (why?).

I essentially just copied your logic from d8179b001ae574da00c8 ff.


> In general, though, it seems to
> me that the durable_rename patch has introduced a whole lot of new failure
> conditions that were not there before, for IMO very little reason.

Uh. Like provably loosing data after crashes?


> I think we would be better off fixing those functions so that there is
> *no* case other than failure of the rename() or link() call itself that
> will be treated as a hard error.  Blowing up completely is not an
> improvement over not fsyncing.

I'm not convinced of that.  Hiding unexpected issues for longer, just to
continue kind-of-operating, can make the impact of problems a lot worse,
and it makes it very hard to actually learn about the issues.

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] Parallel pg_dump's error reporting doesn't work worth squat

[Tom Lane]

Alvaro Herrera  writes:
> Regarding this:
>> + * XXX this certainly looks useless, why not just wait 
>> indefinitely?

> There's another select_loop() in vacuumdb.c suggesting that the timeout
> is used to check for cancel requests; as I understood while working on
> the vacuumdb changes, select() is not interrupted in that case.  I
> suppose that means it's necessary here also.  But on the other hand it's
> quite possible that the original developer just copied what was in
> pg_dump and that it's not actually needed; if that's the case, perhaps
> it's better to rip it out from both places.

Ah, interesting.  That ties into something else I was wondering about,
which is how we could get useful control-C cancellation on Windows.
It looks like the vacuumdb.c version of this code actually is tied
into an interrupt handler, but whoever copied it for parallel.c just
ripped out the CancelRequested checks, making the looping behavior
pretty useless.

For pg_restore (parallel or not) it would be useful if the program
didn't just fall over at control-C but actually sent cancel requests
to the backend(s).  It's not such a problem if we're transferring
data, but if we're waiting for some slow operation like CREATE INDEX,
the current behavior isn't very good.  On the Unix side we have some
SIGINT infrastructure there already, but I don't see any for Windows.

So now I'm thinking we should leave that alone, with the expectation
that we'll be putting CancelRequested checks back in at some point.

> https://www.postgresql.org/message-id/20150122174601.gb1...@alvh.no-ip.org

Hmm, did the patch you're discussing there get committed?

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] Parallel pg_dump's error reporting doesn't work worth squat

[Alvaro Herrera]

Regarding this:

> *** select_loop(int maxFd, fd_set *workerset
> *** 1092,1104 
> --- 1150,1160 
>   fd_set  saveSet = *workerset;
>   
>   #ifdef WIN32
>   for (;;)
>   {
>   /*
>* sleep a quarter of a second before checking if we should 
> terminate.
> +  * XXX this certainly looks useless, why not just wait 
> indefinitely?
>*/
>   struct timeval tv = {0, 25};
>   

There's another select_loop() in vacuumdb.c suggesting that the timeout
is used to check for cancel requests; as I understood while working on
the vacuumdb changes, select() is not interrupted in that case.  I
suppose that means it's necessary here also.  But on the other hand it's
quite possible that the original developer just copied what was in
pg_dump and that it's not actually needed; if that's the case, perhaps
it's better to rip it out from both places.

https://www.postgresql.org/message-id/20150122174601.gb1...@alvh.no-ip.org

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

[HACKERS] new PageAddItemFlags()

[Alvaro Herrera]

In the course of fixing a BRIN bug, I'm adding a new function to
bufpage.c called PageAddItemFlags, which is much like PageAddItem()
except that the "ovewrite" and "is_heap" boolean flags are gone and
replaced by an integer bitmap and some #defines.  If anybody would like
to bikeshed, please do so in that thread.  I intend to back-patch this
to 9.5.

https://www.postgresql.org/message-id/20160527223008.GA735540@alvherre.pgsql

-- 
Á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] [sqlsmith] PANIC: failed to add BRIN tuple

[Alvaro Herrera]

Andreas Seltenreich wrote:
> I wrote:
> 
> > Re-fuzzing now with your patch applied.
> 
> This so far yielded three BRIN core dumps on different machines with the
> same backtraces.  Crash recovery fails in these cases.
> 
> I've put the data directory here (before recovery):
> 
> http://ansel.ydns.eu/~andreas/brincrash2-spidew.tar.xz (9.1M)
> 
> Corresponding backtraces of the backend and startup core files below.

Ah, of course.  These two crashes are two different bugs: the xlog one
is because I forgot to attach the new PageAddItem() flag to the
corresponding replay code; and the one in regular running is because I
neglected to have the patched PageAddItem() compute pd_lower correctly,
which in effect left pages as if empty.

I came up with a simple-minded test program (attached) which reproduces
the reported crashes in a couple of seconds; it can be improved further
for stronger BRIN testing, but at least with this I am confident that
the crashes at hand are gone.  If you can re-run sqlsmith and see if you
can find different bugs, I'd appreciate it.

I'm not going to push this just yet, in order to give others time to
comment on the new PageAddItemFlags API I'm adding.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 6fa8e8ce3812dd6be2ae962ffe076a06482906c9
Author: Alvaro Herrera 
AuthorDate: Fri May 27 18:14:01 2016 -0400
CommitDate: Fri May 27 18:14:01 2016 -0400

Fix PageAddItem BRIN bug

BRIN was relying on the ability of removing a tuple from an index page,
then putting it back again later; but the PageAddItem interface doesn't
actually allow this to happen for tuples beyond the first free one past
the last used item.  To fix, create a new function PageAddItemFlags
which is like PageAddItem except that it has a flags bitmap; the
"overwrite" and "is_heap" boolean flags in the original become
PAI_OVERWITE and PAI_IS_HEAP flags, and a new flag
PAI_ALLOW_LARGE_OFFSET enables the behavior required by BRIN.
PageAddItem() remains with the original signature, for compatibility
with third-party modules (and other callers in core code are not
modified, either).

I also added a new error check in brinGetTupleForHeapBlock to raise an
error if an offset found in the revmap is not actually marked as live by
the page header, which would have detected this sooner (and, in any
case, react with a "corrupted BRIN index" error, rather than a hard
crash, suggesting a REINDEX.)

Reported by Andreas Seltenreich as detected by his handy sqlsmith
fuzzer.

diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
index d0ca485..4b1afce 100644
--- a/src/backend/access/brin/brin_pageops.c
+++ b/src/backend/access/brin/brin_pageops.c
@@ -179,8 +179,8 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
 
 		START_CRIT_SECTION();
 		PageIndexDeleteNoCompact(oldpage, , 1);
-		if (PageAddItem(oldpage, (Item) newtup, newsz, oldoff, true,
-		false) == InvalidOffsetNumber)
+		if (PageAddItemFlags(oldpage, (Item) newtup, newsz, oldoff,
+			 PAI_OVERWRITE | PAI_ALLOW_LARGE_OFFSET) == InvalidOffsetNumber)
 			elog(ERROR, "failed to add BRIN tuple");
 		MarkBufferDirty(oldbuf);
 
diff --git a/src/backend/access/brin/brin_revmap.c b/src/backend/access/brin/brin_revmap.c
index 812f76c..853181b 100644
--- a/src/backend/access/brin/brin_revmap.c
+++ b/src/backend/access/brin/brin_revmap.c
@@ -272,6 +272,10 @@ brinGetTupleForHeapBlock(BrinRevmap *revmap, BlockNumber heapBlk,
 		/* If we land on a revmap page, start over */
 		if (BRIN_IS_REGULAR_PAGE(page))
 		{
+			if (*off > PageGetMaxOffsetNumber(page))
+ereport(ERROR,
+		(errcode(ERRCODE_INDEX_CORRUPTED),
+		 errmsg_internal("corrupted BRIN index: inconsistent range map")));
 			lp = PageGetItemId(page, *off);
 			if (ItemIdIsUsed(lp))
 			{
diff --git a/src/backend/access/brin/brin_xlog.c b/src/backend/access/brin/brin_xlog.c
index deb7af4..3103775 100644
--- a/src/backend/access/brin/brin_xlog.c
+++ b/src/backend/access/brin/brin_xlog.c
@@ -193,7 +193,8 @@ brin_xlog_samepage_update(XLogReaderState *record)
 			elog(PANIC, "brin_xlog_samepage_update: invalid max offset number");
 
 		PageIndexDeleteNoCompact(page, , 1);
-		offnum = PageAddItem(page, (Item) brintuple, tuplen, offnum, true, false);
+		offnum = PageAddItemFlags(page, (Item) brintuple, tuplen, offnum,
+  PAI_OVERWRITE | PAI_ALLOW_LARGE_OFFSET);
 		if (offnum == InvalidOffsetNumber)
 			elog(PANIC, "brin_xlog_samepage_update: failed to add tuple");
 
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 0189bc9..d227d4b 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -153,12 +153,12 @@ PageIsVerified(Page page, BlockNumber blkno)
 
 
 /*
- *	PageAddItem
+ *	

[HACKERS] copyParamList

[Andrew Gierth]

copyParamList does not respect from->paramMask, in what looks to me like
an obvious oversight:

retval->paramMask = NULL;
[...]
/* Ignore parameters we don't need, to save cycles and space. */
if (retval->paramMask != NULL &&
!bms_is_member(i, retval->paramMask))

retval->paramMask is never set to anything not NULL in this function,
so surely that should either be initializing it to from->paramMask, or
checking from->paramMask in the conditional?

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] 9.4 -> 9.5 regression with queries through pgbouncer on RHEL 6

[Andres Freund]

Hi,


On 2016-05-27 19:57:34 +0300, Vladimir Borodin wrote:
> -performance
> > Here is how the results look like for 9.4, 9.5 and 9.6. All are built from 
> > latest commits on yesterday in
> > * REL9_4_STABLE (a0cc89a28141595d888d8aba43163d58a1578bfb),
> > * REL9_5_STABLE (e504d915bbf352ecfc4ed335af934e799bf01053),
> > * master (6ee7fb8244560b7a3f224784b8ad2351107fa55d).
> > 
> > All of them are build on the host where testing is done (with stock gcc 
> > versions). Sysctls, pgbouncer config and everything we found are the same, 
> > postgres configs are default, PGDATA is in tmpfs. All numbers are 
> > reproducible, they are stable between runs.
> > 
> > Shortly:
> > 
> > OS  PostgreSQL version  TPS Avg. 
> > latency
> > RHEL 6  9.4 44898   
> > 1.425 ms
> > RHEL 6  9.5 26199   
> > 2.443 ms
> > RHEL 6  9.5 43027   
> > 1.487 ms
> > Ubuntu 14.049.4 67458   
> > 0.949 ms
> > Ubuntu 14.049.5 64065   
> > 0.999 ms
> > Ubuntu 14.049.6 64350   
> > 0.995 ms
> 
> The results above are not really fair, pgbouncer.ini was a bit different on 
> Ubuntu host (application_name_add_host was disabled). Here are the right 
> results with exactly the same configuration:
> 
> OSPostgreSQL version  TPS Avg. 
> latency
> RHEL 69.4 44898   
> 1.425 ms
> RHEL 69.5 26199   
> 2.443 ms
> RHEL 69.5 43027   
> 1.487 ms
> Ubuntu 14.04  9.4 45971   1.392 ms
> Ubuntu 14.04  9.5 40282   1.589 ms
> Ubuntu 14.04  9.6 45410   1.409 ms

Hm. I'm a bit confused. You show one result for 9.5 with bad and one
with good performance. I suspect the second one is supposed to be a 9.6?

Am I understanding correctly that the performance near entirely
recovered with 9.6? If so, I suspect we might be dealing with a memory
alignment issue. Do the 9.5 results change if you increase
max_connections by one or two (without changing anything else)?

What's the actual hardware?

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] Parallel pg_dump's error reporting doesn't work worth squat

[Tom Lane]

I wrote:
>> BTW, after having spent the afternoon staring at it, I will assert with
>> confidence that pg_dump/parallel.c is an embarrassment to the project.

> I've done a bit of work on a cosmetic cleanup patch, but don't have
> anything to show yet.

Attached is the threatened cosmetic cleanup of parallel.c.  As I went
through it, I found quite a few things not to like, but in this patch
I've mostly resisted the temptation to fix them immediately, and have
just tried to document what's there more accurately.

Aside from a major amount of comment-rewriting and a very small amount of
cosmetic code adjustment (mostly moving code for more clarity), this
patch changes these things:

* Rename SetupWorker() to RunWorker() to reflect what it actually does,
and remove its unused "worker" argument.

* Rename lockTableNoWait() to lockTableForWorker() for clarity, and move
the test for BLOBS into it instead of having an Assert that the caller
checked that.

* Don't bother with getThreadLocalPQExpBuffer() at all in non-Windows
builds; it was identical to getLocalPQExpBuffer() anyway, except for
being misleadingly named.

* Remove some completely-redundant or otherwise ill-considered Asserts.

* Fix incorrect "Assert(msgsize <= bufsize)" --- should be < bufsize.

* Fix missing socket close in one error exit from pgpipe().  This isn't
too exciting at present since we'll just curl up and die if it fails,
but might as well get it right.

I have some other, less-cosmetic, things I want to do here, but first
does anyone care to review this?

regards, tom lane

diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index c656ba5..1a52fae 100644
*** a/src/bin/pg_dump/parallel.c
--- b/src/bin/pg_dump/parallel.c
***
*** 2,21 
   *
   * parallel.c
   *
!  *	Parallel support for the pg_dump archiver
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
   *
-  *	The author is not responsible for loss or damages that may
-  *	result from its use.
-  *
   * IDENTIFICATION
   *		src/bin/pg_dump/parallel.c
   *
   *-
   */
  
  #include "postgres_fe.h"
  
  #include "parallel.h"
--- 2,62 
   *
   * parallel.c
   *
!  *	Parallel support for pg_dump and pg_restore
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
   *
   * IDENTIFICATION
   *		src/bin/pg_dump/parallel.c
   *
   *-
   */
  
+ /*
+  * Parallel operation works like this:
+  *
+  * The original, master process calls ParallelBackupStart(), which forks off
+  * the desired number of worker processes, which each enter WaitForCommands().
+  *
+  * The master process dispatches an individual work item to one of the worker
+  * processes in DispatchJobForTocEntry().  That calls
+  * AH->MasterStartParallelItemPtr, a routine of the output format.  This
+  * function's arguments are the parents archive handle AH (containing the full
+  * catalog information), the TocEntry that the worker should work on and a
+  * T_Action value indicating whether this is a backup or a restore task.  The
+  * function simply converts the TocEntry assignment into a command string that
+  * is then sent over to the worker process. In the simplest case that would be
+  * something like "DUMP 1234", with 1234 being the TocEntry id.
+  *
+  * The worker process receives and decodes the command and passes it to the
+  * routine pointed to by AH->WorkerJobDumpPtr or AH->WorkerJobRestorePtr,
+  * which are routines of the current archive format.  That routine performs
+  * the required action (dump or restore) and returns a malloc'd status string.
+  * The status string is passed back to the master where it is interpreted by
+  * AH->MasterEndParallelItemPtr, another format-specific routine.  That
+  * function can update state or catalog information on the master's side,
+  * depending on the reply from the worker process.  In the end it returns a
+  * status code, which is 0 for successful execution.
+  *
+  * Remember that we have forked off the workers only after we have read in
+  * the catalog.  That's why our worker processes can also access the catalog
+  * information.  (In the Windows case, the workers are threads in the same
+  * process.  To avoid problems, they work with cloned copies of the Archive
+  * data structure; see init_spawned_worker_win32().)
+  *
+  * In the master process, the workerStatus field for each worker has one of
+  * the following values:
+  *		WRKR_IDLE: it's waiting for a command
+  *		WRKR_WORKING: it's been sent a command
+  *		WRKR_FINISHED: it's returned a result
+  *		WRKR_TERMINATED: process ended
+  * The FINISHED state indicates that the worker is idle, but 

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Nikolay Shaplov wrote:
> >> Story start from the point that I found out that a.m. can not forbid 
> >> changing 
> >> some of it's reloptions with ALTER INDEX command.
> 
> > Hmm, this sounds like a bug to me.  In BRIN, if you change the
> > pages_per_range option for an existing index, the current index
> > continues to work because the value used during the last index build is
> > stored in the metapage.  Only when you reindex after changing the option
> > the new value takes effect.
> 
> > I think Bloom should do likewise.
> 
> AFAICT, Bloom *does* do that.  The reloptions are only consulted directly
> while initializing the metapage.

Ah, clearly I misunderstood what he was saying.

> I think Nikolay's complaint is essentially that there should be a way
> for an AM to forbid ALTER INDEX if it's not going to support on-the-fly
> changes of a given reloption.  That might be a helpful usability
> improvement, but it's only a usability improvement not a bug fix.

I can get behind that idea, but this also says I should get away from
this thread until 10dev opens.

-- 
Á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] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Tom Lane]

Alvaro Herrera  writes:
> Nikolay Shaplov wrote:
>> Story start from the point that I found out that a.m. can not forbid 
>> changing 
>> some of it's reloptions with ALTER INDEX command.

> Hmm, this sounds like a bug to me.  In BRIN, if you change the
> pages_per_range option for an existing index, the current index
> continues to work because the value used during the last index build is
> stored in the metapage.  Only when you reindex after changing the option
> the new value takes effect.

> I think Bloom should do likewise.

AFAICT, Bloom *does* do that.  The reloptions are only consulted directly
while initializing the metapage.

I think Nikolay's complaint is essentially that there should be a way
for an AM to forbid ALTER INDEX if it's not going to support on-the-fly
changes of a given reloption.  That might be a helpful usability
improvement, but it's only a usability improvement not a bug fix.

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] Status of 64 bit atomics

[Andres Freund]

Hi,

On 2016-05-27 11:00:42 -0400, John Gorman wrote:
> Someone recently told me that the postgresql atomics library was incomplete
> for 64 bit operations such as pg_atomic_fetch_add_u64() and should not be
> used.

There's currently no fallback for 32bit platforms without 64bit atomics
support.  I posted a patch adding that fallback, which I plan to commit
soon after the start of the 9.7 development window opens.


> Can someone definitively confirm whether it is okay to rely on the 64
> bit atomics or whether it is better to protect 64 bit operations with
> a spinlock?

For current versions 64bit atomics are working, but you'll get
compilation errors if the platform doesn't have 64bit atomics
support. That's not actually many CPUs these days; most prominent are
probably older arm CPUs.

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] Status of 64 bit atomics

[Arthur Silva]

On May 27, 2016 5:01 PM, "John Gorman"  wrote:
>
> Hi All
>
> Someone recently told me that the postgresql atomics library was
incomplete
> for 64 bit operations such as pg_atomic_fetch_add_u64() and should not be
used.
>
> Can someone definitively confirm whether it is okay to rely on the 64 bit
atomics
> or whether it is better to protect 64 bit operations with a spinlock?
>
> Thanks!
> John

Golang has asm implementations for these even on 32bit platforms (see
https://github.com/golang/go/tree/master/src/sync/atomic).

Couldn't we borrow them? Or even better, fall back to spin lock on these,
but transparently.

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Nikolay Shaplov wrote:
> В письме от 27 мая 2016 15:05:58 Вы написали:
> > Nikolay Shaplov wrote:
> > > Story start from the point that I found out that a.m. can not forbid
> > > changing some of it's reloptions with ALTER INDEX command. That was not
> > > necessary before, because all reloptions at that existed at that time can
> > > be changed on fly. But now for bloom index it is unacceptable, because
> > > for changing bloom's reloptions for existing index will lead to index
> > > malfunction.
> > 
> > Hmm, this sounds like a bug to me.  In BRIN, if you change the
> > pages_per_range option for an existing index, the current index
> > continues to work because the value used during the last index build is
> > stored in the metapage.  Only when you reindex after changing the option
> > the new value takes effect.
> > 
> > I think Bloom should do likewise.
> 
> I do not think that it is the best behavior. Because if we came to this 
> situation, the current value of pages_per_range that index actually using is 
> not available for user, because he is not able to look into meta page.

You're right in that, but this is a much less serious behavior than
causing the index to fail to work altogether just because the option was
changed.  Since we're certainly not going to rework reloptions in 9.6,
IMO the bloom behavior should be changed to match BRIN's.

> In this case it would be better either forbid changing the options, so it 
> would be consistent, or force index rebuild, then it would be consistent too. 
> I would vote for first behavior as this is less work to do for me, and can be 
> changed later, if it is really needed for some case.

I think forcing index rebuild is not a great idea.  That turns a simple
ALTER INDEX command which doesn't block the index heavily nor for a long
time, into a much more serious deal.

-- 
Á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] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

В письме от 27 мая 2016 15:05:58 Вы написали:
> Nikolay Shaplov wrote:
> > Story start from the point that I found out that a.m. can not forbid
> > changing some of it's reloptions with ALTER INDEX command. That was not
> > necessary before, because all reloptions at that existed at that time can
> > be changed on fly. But now for bloom index it is unacceptable, because
> > for changing bloom's reloptions for existing index will lead to index
> > malfunction.
> 
> Hmm, this sounds like a bug to me.  In BRIN, if you change the
> pages_per_range option for an existing index, the current index
> continues to work because the value used during the last index build is
> stored in the metapage.  Only when you reindex after changing the option
> the new value takes effect.
> 
> I think Bloom should do likewise.

I do not think that it is the best behavior. Because if we came to this 
situation, the current value of pages_per_range that index actually using is 
not available for user, because he is not able to look into meta page.

In this case it would be better either forbid changing the options, so it 
would be consistent, or force index rebuild, then it would be consistent too. 
I would vote for first behavior as this is less work to do for me, and can be 
changed later, if it is really needed for some case.

PS sorry I did not add mail list to the CC, so I send it second time...


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

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Nikolay Shaplov wrote:

> Story start from the point that I found out that a.m. can not forbid changing 
> some of it's reloptions with ALTER INDEX command. That was not necessary  
> before, because all reloptions at that existed at that time can be changed on 
> fly. But now for bloom index it is unacceptable, because for changing bloom's 
> reloptions for existing index will lead to index malfunction.

Hmm, this sounds like a bug to me.  In BRIN, if you change the
pages_per_range option for an existing index, the current index
continues to work because the value used during the last index build is
stored in the metapage.  Only when you reindex after changing the option
the new value takes effect.

I think Bloom should do likewise.

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

[HACKERS] Hard to maintain duplication in contain_volatile_functions_not_nextval_walker

[Andres Freund]

Hi,

contain_volatile_functions_walker is duplicated, near entirely, in
contain_volatile_functions_not_nextval_walker.

Wouldn't it have been better not to duplicate, and keep a flag about
ignoring nextval in the context variable?

While at it, couldn't we also fold contain_mutable_functions_walker()
together using a similar technique?

Regards,

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] Allow COPY to use parameters

[Merlin Moncure]

On Fri, May 27, 2016 at 11:36 AM, Corey Huinker  wrote:
>>
>> For the following pretend that "STRING" has the same behavior as the
>> "format(...)" function.
>>
>> EXECUTE STRING('COPY %I TO %L', 'testtable', 'testfile.txt');
>
> +1

-1

Why use syntax to do this?  If we need a function with special
behaviors, let's make a function with special behaviors, hopefully not
called 'string()'...for example, format_safe().  Furthermore, we
already have functions to deal with safe string injection,
quote_ident/literal, and I'm not sure it's a good divert users away
from using those functions.

Either way, I don't think we should mix improvements to EXECUTE with
this patch, which I think addresses the problems I have with COPY
nicely (at least as it pertains to my pain points)...so +1 to Andrew's
proposal.  I would rarely have to use dynamic SQL for COPY with that
in place.  Other utility commands (off the top of my head) tend not to
be a problem for me (NOTIFY would be, but pg_notify handles that
case).

merlin


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

Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> By the way, the reason of the "invalid snapshot identifier" is
> that some worker threads try to use it after the connection on
> the first worker closed.

... BTW, I don't quite see what the issue is there.  The snapshot is
exported from the master session, so errors in worker sessions should not
cause such failures in other workers.  And I don't see any such failure
when setting up a scenario that will cause a worker to fail on Linux.
The "invalid snapshot identifier" bleats would make sense if you had
gotten a server-side error (and transaction abort) in the master session,
but I don't see any evidence that that happened in that example.  Might be
worth seeing if that's reproducible.

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] 9.4 -> 9.5 regression with queries through pgbouncer on RHEL 6

[Vladimir Borodin]

-performance
+hackers

> 25 мая 2016 г., в 17:33, Vladimir Borodin  написал(а):
> 
> Hi all.
> 
> We have found that queries through PgBouncer 1.7.2 (with transaction pooling) 
> to local PostgreSQL are almost two times slower in 9.5.3 than in 9.4.8 on 
> RHEL 6 hosts (all packages are updated to last versions). Meanwhile the 
> problem can’t be reproduced i.e. on Ubuntu 14.04 (also fully-updated).
> 
> Here is how the results look like for 9.4, 9.5 and 9.6. All are built from 
> latest commits on yesterday in
>   * REL9_4_STABLE (a0cc89a28141595d888d8aba43163d58a1578bfb),
>   * REL9_5_STABLE (e504d915bbf352ecfc4ed335af934e799bf01053),
>   * master (6ee7fb8244560b7a3f224784b8ad2351107fa55d).
> 
> All of them are build on the host where testing is done (with stock gcc 
> versions). Sysctls, pgbouncer config and everything we found are the same, 
> postgres configs are default, PGDATA is in tmpfs. All numbers are 
> reproducible, they are stable between runs.
> 
> Shortly:
> 
> OSPostgreSQL version  TPS Avg. 
> latency
> RHEL 69.4 44898   
> 1.425 ms
> RHEL 69.5 26199   
> 2.443 ms
> RHEL 69.5 43027   
> 1.487 ms
> Ubuntu 14.04  9.4 67458   0.949 ms
> Ubuntu 14.04  9.5 64065   0.999 ms
> Ubuntu 14.04  9.6 64350   0.995 ms

The results above are not really fair, pgbouncer.ini was a bit different on 
Ubuntu host (application_name_add_host was disabled). Here are the right 
results with exactly the same configuration:

OS  PostgreSQL version  TPS Avg. 
latency
RHEL 6  9.4 44898   1.425 ms
RHEL 6  9.5 26199   2.443 ms
RHEL 6  9.5 43027   1.487 ms
Ubuntu 14.049.4 45971   1.392 ms
Ubuntu 14.049.5 40282   1.589 ms
Ubuntu 14.049.6 45410   1.409 ms

It can be seen that there is a regression for 9.5 in Ubuntu also, but not so 
significant. We first thought that the reason is 
38628db8d8caff21eb6cf8d775c0b2d04cf07b9b (Add memory barriers for 
PgBackendStatus.st_changecount protocol), but in that case the regression 
should also be seen in 9.6 also.

There also was a bunch of changes in FE/BE communication (like 
387da18874afa17156ee3af63766f17efb53c4b9 or 
98a64d0bd713cb89e61bef6432befc4b7b5da59e) and that may answer the question of 
regression in 9.5 and normal results in 9.6. Probably the right way to find the 
answer is to do bisect. I’ll do it but if some more diagnostics information can 
help, feel free to ask about it.

> 
> You could see that the difference between major versions on Ubuntu is not 
> significant, but on RHEL 9.5 is 70% slower than 9.4 and 9.6.
> 
> Below are more details.
> 
> RHEL 6:
> 
> postgres@pgload05g ~ $ /usr/lib/postgresql/9.4/bin/pgbench -U postgres -T 60 
> -j 64 -c 64 -S -n 'host=localhost port=6432 dbname=pg94'
> transaction type: SELECT only
> scaling factor: 100
> query mode: simple
> number of clients: 64
> number of threads: 64
> duration: 60 s
> number of transactions actually processed: 2693962
> latency average: 1.425 ms
> tps = 44897.461518 (including connections establishing)
> tps = 44898.763258 (excluding connections establishing)
> postgres@pgload05g ~ $ /usr/lib/postgresql/9.4/bin/pgbench -U postgres -T 60 
> -j 64 -c 64 -S -n 'host=localhost port=6432 dbname=pg95'
> transaction type: SELECT only
> scaling factor: 100
> query mode: simple
> number of clients: 64
> number of threads: 64
> duration: 60 s
> number of transactions actually processed: 1572014
> latency average: 2.443 ms
> tps = 26198.928627 (including connections establishing)
> tps = 26199.803363 (excluding connections establishing)
> postgres@pgload05g ~ $ /usr/lib/postgresql/9.4/bin/pgbench -U postgres -T 60 
> -j 64 -c 64 -S -n 'host=localhost port=6432 dbname=pg96'
> transaction type: SELECT only
> scaling factor: 100
> query mode: simple
> number of clients: 64
> number of threads: 64
> duration: 60 s
> number of transactions actually processed: 2581645
> latency average: 1.487 ms
> tps = 43025.676995 (including connections establishing)
> tps = 43027.038275 (excluding connections establishing)
> postgres@pgload05g ~ $
> 
> Ubuntu 14.04 (the same hardware):
> 
> postgres@pgloadpublic02:~$ /usr/lib/postgresql/9.4/bin/pgbench -U postgres -T 
> 60 -j 64 -c 64 -S -n 'host=localhost port=6432 dbname=pg94'
> transaction type: SELECT only
> scaling factor: 100
> query mode: simple
> number of 

Re: [HACKERS] Allow COPY to use parameters

[Corey Huinker]

>
>
> For the following pretend that "STRING" has the same behavior as the
> "format(...)" function.
>
> EXECUTE STRING('COPY %I TO %L', 'testtable', 'testfile.txt');​
>

+1
We should make string sanitization easy so that people use it by default.

In the mean time, if you're just using psql, the new \gexec command will
cover that

select format('COPY %I TO %L', 'testtable', 'testfile.txt')
\gexec


but it won't help with any \-commands. And it won't work for
schema-qualified table names, and if you're using COPY tab FROM PROGRAM,
you're going to have cases where %L finds an escape-y character in the
command string (like using head -n 1 and sed to unpivot a header row) which
results in an E'...' string that COPY can't handle.

For \copy, I end up doing something like

select format('\\copy %I from program %L',:'table_name','pigz -cd ' ||
:'file_name') as copy_command
\gset
:copy_command


Which won't win any beauty contests, and suffers from all the limitations I
listed earlier, but works for me.

I'm indifferent to whether these commands need to be PREPARE-able so long
as sanitization becomes a solved problem.

Re: [HACKERS] PATCH: pg_restore parallel-execution-deadlock issue

[Tom Lane]

Michael Paquier  writes:
> On Fri, May 27, 2016 at 4:07 PM, Amit Kapila  wrote:
>> On Fri, May 27, 2016 at 3:05 AM, Tom Lane  wrote:
>>> 2. Armin proposes that WaitForTerminatingWorkers needs to do CloseHandle()
>>> on the various thread handles.  That sounds plausible but I don't know
>>> enough Windows programming to know if it really matters.
>>> 
>>> 3. Should we replace ExitThread() with _endthreadex()?  Again, it
>>> seems plausible but I'm not the person to ask.

>> I think point (2) and (3) are related because using _endthreadex won't close
>> the thread handle explicitly [1].

> Yep.

OK, I pushed something based on that.  It's untested by me but the
buildfarm should tell us if I broke anything too badly.

I believe we've now dealt with all the issues originally raised by
Armin, so I've marked this patch committed in the CF app.

regards, tom lane


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Tue, May 24, 2016 at 4:10 PM, Robert Haas  wrote:
> On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner  wrote:
>> On Tue, May 24, 2016 at 12:00 PM, Andres Freund  wrote:
>>> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
 On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner  wrote:
> On Fri, May 6, 2016 at 7:48 PM, Andres Freund  wrote:

>> That comment reminds me of a question I had: Did you consider the effect
>> of this patch on analyze? It uses a snapshot, and by memory you've not
>> built in a defense against analyze being cancelled.
>>
>> The primary defense is not considering a cancellation except in 30
>> of the 500 places a page is used.  None of those 30 are, as far as
>> I can see (upon review in response to your question), used in the
>> analyze process.
>
> It's not obvious to me how this is supposed to work.  If ANALYZE's
> snapshot is subject to being ignored for xmin purposes because of
> snapshot_too_old, then I would think it would need to consider
> cancelling itself if it reads a page with possibly-removed data, just
> like in any other case.  It seems that we might not actually need a
> snapshot set for ANALYZE in all cases, because the comments say:
>
> /* functions in indexes may want a snapshot set */
> PushActiveSnapshot(GetTransactionSnapshot());
>
> If we can get away with it, it would be a rather large win to only set
> a snapshot when the table has an expression index.  For purposes of
> "snapshot too old", though, it will be important that a function in an
> index which tries to read data from some other table which has been
> pruned cancels itself when necessary.

I will make this my top priority after resolving the question of whether
there is an issue with CREATE INDEX.  I expect to have a resolution,
probably involving some patch, by 3 June.

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


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Fri, May 27, 2016 at 9:58 AM, Kevin Grittner  wrote:
> On Tue, May 24, 2016 at 4:56 PM, Andres Freund  wrote:

>> If an old session with >= repeatable read accesses a clustered
>> table (after the cluster committed), they'll now not see any
>> errors, because all the LSNs look new.
>
> Again, it is new LSNs that trigger errors; if the page has not been
> written recently the LSN is old and there is no error.  I think you
> may be seeing problems based on getting the basics of this
> backwards.

I am reviewing the suggestion of a possible bug now, and will make
it my top priority until resolved.  By the end of 1 June I will
either have committed a fix or posted an explanation of why the
concern is mistaken, with test results to demonstrate correct
behavior.

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


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

Re: [HACKERS] Fix a typo in 9.6 release notes

[Tom Lane]

=?UTF-8?Q?L=c3=a9onard_Benedetti?=  writes:
> Patch to fix a typo in 9.6 release notes. It is especially ironic that
> this is a mistake of diacritical on my name, on a patch improving the
> support of diacritics^^.

Pushed, thanks!

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] Status of 64 bit atomics

[John Gorman]

Hi All

Someone recently told me that the postgresql atomics library was incomplete
for 64 bit operations such as pg_atomic_fetch_add_u64() and should not be
used.

Can someone definitively confirm whether it is okay to rely on the 64 bit
atomics
or whether it is better to protect 64 bit operations with a spinlock?

Thanks!
John

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Tue, May 24, 2016 at 4:56 PM, Andres Freund  wrote:

> The LSNs of the created index pages will be new, and we'll thus
> not necessarily error out when requried.

It is *old* LSNs that are *safe* -- *new* LSNs are what trigger the
"snapshot too old" error.  So your observation may be a reason that
there is not, in fact, any bug here.  That said, even if there is
no chance that using the index could generate incorrect results, it
may be worth looking at whether avoiding index use (as mentioned
below) might avoid false positives, and have enough value as an
optimization to add.  Of course, if that is the case, there is the
question of whether that is appropriate for 9.6 or material for the
first version 10 CF.

> At the very least we'd have to set indexInfo->ii_BrokenHotChain
> in that case.

If there is a bug, this is what I will look at first -- having
queries avoid the index if they are using an old snapshot, rather
than throwing an error.

> As far as I can see normal index builds will allow concurrent hot
> prunes and everything; since those only require page-level
> exclusive locks.
>
> So for !concurrent builds we might end up with a corrupt index.

... by which you mean an index that omits certainly-dead heap
tuples which have been the subject of early pruning or vacuum, even
if there are still registered snapshots that include those tuples?
Or do you see something else?

Again, since both the heap pages involved and all the new index
pages would have LSNs recent enough to trigger the old snapshot
check, I'm not sure where the problem is, but will review closely
to see what I might have missed.

> I think many of the places relying on heap scans with !mvcc
> snapshots are problematic in that way. Outdated reads will not be
> detected by TestForOldSnapshot() (given the (snapshot)->satisfies
> == HeapTupleSatisfiesMVCC condition therein), and rely on the
> snapshot to be actually working.  E.g. CLUSTER/ VACUUM FULL rely
> on accurate HEAPTUPLE_RECENTLY_DEAD

Don't the "RECENTLY" values imply that the transaction is still
running which cause the tuple to be dead?  Since tuples are not
subject to early pruning or vacuum when that is the case, where do
you see a problem?  The snapshot itself has the usual xmin et al.,
so I'm not sure what you might mean by "the snapshot to be actually
working" if not the override at the time of pruning/vacuuming.

> If an old session with >= repeatable read accesses a clustered
> table (after the cluster committed), they'll now not see any
> errors, because all the LSNs look new.

Again, it is new LSNs that trigger errors; if the page has not been
written recently the LSN is old and there is no error.  I think you
may be seeing problems based on getting the basics of this
backwards.

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


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

Re: [HACKERS] Allow COPY to use parameters

[David G. Johnston]

On Fri, May 27, 2016 at 6:02 AM, Craig Ringer  wrote:

>
> COPY FROM $1 TO 'myfilename'
>
>
​Random thought - how about at least making the following work:

For the following pretend that "STRING" has the same behavior as the
"format(...)" function.

EXECUTE STRING('COPY %I TO %L', 'testtable', 'testfile.txt');​

<(conceptually similar to: EXECUTE format(​'COPY %I TO %L', 'testtable',
'testfile.txt')>

​This doesn't solve the knowledge problem ​but at least provides an
idiomatic way to execute dynamic SQL without pl/pgsql and without forcing
the client library to take responsibility for proper data massaging in
order to eliminate sql injection.

As an extension making:

PREPARE name STRING('COPY %I TO %L', ?, ?);​

EXECUTE name STRING USING ('testtable', 'testfile.txt');

David J.

Re: [HACKERS] COMMENT ON, psql and access methods

[Tom Lane]

Michael Paquier  writes:
> As far as I can see, COMMENT ON has no support for access methods.
> Wouldn't we want to add it as it is created by a command? On top of
> that, perhaps we could have a backslash command in psql to list the
> supported access methods, like \dam[S]? The system methods would be in
> this case all the in-core ones.

I'm a bit skeptical that we need this yet.  Maybe if custom access methods
become so popular that everybody's got one, it'd be worth the trouble.

regards, tom lane


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

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

В письме от 24 мая 2016 17:12:16 пользователь Nikolay Shaplov написал:

While working on this patch I met some difficulties that makes me to completely 
rewrite a code that is responsible for interacting reloptions.c with access 
methods.

Story start from the point that I found out that a.m. can not forbid changing 
some of it's reloptions with ALTER INDEX command. That was not necessary  
before, because all reloptions at that existed at that time can be changed on 
fly. But now for bloom index it is unacceptable, because for changing bloom's 
reloptions for existing index will lead to index malfunction.

I was thinking about adding for_alter flag argument for parseRelOptions funtion 
and pass it all the way from indexcmds.c & tablecmds.c, and add some flag in 
the definition of reloptions to mark those reloptions that should not be used 
in ALTER INDEX clause. 

This theoretically this will give us a way to throw error when user tries to 
change an reloption that should not be changed.

But unfortunately it turned out that in ATExecSetRelOptions new reloptions is 
merged with existing reloptions values using transformRelOptions, and only 
after that the result is sent for validation. 

So on the step of the validation we can not distinguish old options from a new 
one. 

I've been thinking how to work this around, but found out that there is no 
simple way. I can pass another array of flags through the whole call stack. But 
this make all thing uglier. 

I can create another function that do pre-validation for new reloptions, 
before doing final validation of the whole set. But this will make me to have  
to entities available from the a.m.: the descriptor of  reloptions and 
function for custom validation of the results (i.e. like adjustBloomOptions 
for Bloom).

But it would be not good if we dedicate two entires of a.m. to reoptions 
definition. And there can be even more... 

So I came to a conclusion: gather all the staff that defines the all the 
behavior of reloption parsing and validation into one structure, both array of 
relopt_value, custom validation function, and all the other staff.

And this structure is the only thing that a.m. gives to reloptions.c code for 
parsing and validation purposes. And that should be enough.

I hope this make code more flexible and more easy to understand.



-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


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

Re: [HACKERS] PATCH: pg_restore parallel-execution-deadlock issue

[Michael Paquier]

On Fri, May 27, 2016 at 4:07 PM, Amit Kapila  wrote:
> On Fri, May 27, 2016 at 3:05 AM, Tom Lane  wrote:
>>
>> Michael Paquier  writes:
>> > ea274b2 has changed the way disconnection is done is is now closing
>> > both the read and write pipes. So you may want to retry if things get
>> > better with the next round of minor releases.
>>
>> Hadn't paid attention to this thread before ...
>>
>> 1. Armin proposes using "shutdown(pipeWrite, SD_BOTH)" where the code
>> committed this morning (df8d2d8c4) has "closesocket(pipeWrite)".
>> I'd prefer to leave it that way since it's the same as for the Unix case,
>> and Kyotaro-san says it works for him.  Is there a reason we'd need
>> shutdown() instead?

Hm, OK.

>> 2. Armin proposes that WaitForTerminatingWorkers needs to do CloseHandle()
>> on the various thread handles.  That sounds plausible but I don't know
>> enough Windows programming to know if it really matters.
>>
>> 3. Should we replace ExitThread() with _endthreadex()?  Again, it
>> seems plausible but I'm not the person to ask.
>>
>
> I think point (2) and (3) are related because using _endthreadex won't close
> the thread handle explicitly [1].

Yep.

> [1] - https://msdn.microsoft.com/en-us/library/kdzttdcb.aspx
> Refer line "_endthread automatically closes the thread handle, whereas
> _endthreadex does not."

And the rest of the sentence:
Therefore, when you use _beginthread and _endthread, do not explicitly
close the thread handle by calling the Win32 CloseHandle API. This
behavior differs from the Win32 ExitThread API.

Personally I understand that as well as for the first part: when using
_beginthreadex and _endthreadex, be sure to call CloseHandle() to
explicitely close the thread handle.

And based on the second part if we use ExitThread after beginning a
thread with _beginthreadex we would get unreliable behavior. I guess
you don't need a patch? Because by looking again at this thread and
the windows docs what we have now is unpredictable.
-- 
Michael


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

[HACKERS] Fix a typo in 9.6 release notes

[Léonard Benedetti]

Patch to fix a typo in 9.6 release notes. It is especially ironic that
this is a mistake of diacritical on my name, on a patch improving the
support of diacritics^^.

Léonard Benedetti

diff --git a/doc/src/sgml/release-9.6.sgml b/doc/src/sgml/release-9.6.sgml
index fed1199..38e1967 100644
--- a/doc/src/sgml/release-9.6.sgml
+++ b/doc/src/sgml/release-9.6.sgml
@@ -3165,7 +3165,7 @@ This commit is also listed under libpq and PL/pgSQL
 Extend contrib/unaccent's
 standard unaccent.rules file to handle all diacritics
 known to Unicode, and expand ligatures correctly
-(Thomas Munro, Leonard Benedetti)
+(Thomas Munro, Lonard Benedetti)

 

-- 
2.8.2


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

[HACKERS] COMMENT ON, psql and access methods

[Michael Paquier]

Hi all,

As far as I can see, COMMENT ON has no support for access methods.
Wouldn't we want to add it as it is created by a command? On top of
that, perhaps we could have a backslash command in psql to list the
supported access methods, like \dam[S]? The system methods would be in
this case all the in-core ones.

Regards,
-- 
Michael


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

[HACKERS] Floating point comparison inconsistencies of the geometric types

[Emre Hasegeli]

There are those macros defined for the built-in geometric types:

> #define EPSILON 1.0E-06

> #define FPzero(A)   (fabs(A) <= EPSILON)
> #define FPeq(A,B)   (fabs((A) - (B)) <= EPSILON)
> #define FPne(A,B)   (fabs((A) - (B)) > EPSILON)
> #define FPlt(A,B)   ((B) - (A) > EPSILON)
> #define FPle(A,B)   ((A) - (B) <= EPSILON)
> #define FPgt(A,B)   ((A) - (B) > EPSILON)
> #define FPge(A,B)   ((B) - (A) <= EPSILON)

with this warning:

>  *XXX These routines were not written by a numerical analyst.

Most of the geometric operators use those macros for comparison, but
those  do not:

* polygon << polygon
* polygon &< polygon
* polygon &> polygon
* polygon >> polygon
* polygon <<| polygon
* polygon &<| polygon
* polygon |&> polygon
* polygon |>> polygon
* box @> point
* point <@ box
* lseg <@ box
* circle @> point
* point <@ circle

This is really a bug that needs to be fixed one way or another.  I think
that it is better to fix it by removing the macros all together.  I
am not sure how useful they are in practice.  I haven't seen anyone
complaining about the above operators not using the macros.  Though,
people often complain about the ones using the macros and the problems
caused by them.

The hackers evidently don't like the macros, either.  That should be
why they are not used on the new operators.  What annoys me most about
this situation is the inconsistency blocks demonstrating our indexes.
Because of this, we had to rip out point type support from BRIN
inclusion operator class which could be useful to PostGIS.

Fixing it has been discussed many times before [1][2][3][4] with
no result.  Here is my plan to fix the situation covering the problems
around it:

1) Reimplement some operators to avoid divisions

The attached patch does it on some of the line operators.

2) Use exact comparison on everywhere except the operators fuzzy
   comparison is certainly required

The attach patch changes all operators except some "lseg" operators.
"lseg" stores two points on a line.  Most of the operations done on it
are lossy.  I don't see a problem treating them differently, those
operators are very unlikely to be index supported.

3) Check numbers for underflow and overflow

I am thinking to use CHECKFLOATVAL on utils/adt/float.c, but it is not
exposed outside at the moment.  Would it be okay to create a new header
file utils/float.h to increase code sharing between float and geometric
types?

4) Implement relative fuzzy comparison for the remaining operators

It is better to completely get rid of those macros while we are on it.
I think we can get away by implementing just a single function for fuzzy
equality, not for other comparisons.  I am inclined to put it to
utils/adt/float.c.  Is this a good idea?

Tom Lane commented on the function posted to the list [1] on 2002:

> Not like that. Perhaps use a fraction of the absolute value of the
> one with larger absolute value.  As-is it's hard to tell how FLT_EPSILON
> is measured.

I cannot image how the function would look like.  I would appreciate
any guidance.

5) Implement default hash operator class for all geometric types

This would solve most complained problem of those types allowing them
to used with DISTINCT and GROUP BY.

6) Implement default btree operator class at least for the point type

This would let the point type to be used with ORDER BY.  It is
relatively straight forward to implement it for the point type.  Is it
a good idea to somehow implement it for other types?

7) Add GiST index support for missing cross type operators

Currently only contained by operators are supported by an out-of-range
strategy number.  I think we can make the operator classes much nicer
by allowing really cross type operator families.

Comments?

[1] 
https://www.postgresql.org/message-id/flat/d90a5a6c612a39408103e6ecdd77b8290fd...@voyager.corporate.connx.com
[2] https://www.postgresql.org/message-id/flat/4a7c2c4b.5020...@netspace.net.au
[3] https://www.postgresql.org/message-id/flat/12549.1346111...@sss.pgh.pa.us
[4] 
https://www.postgresql.org/message-id/flat/20150512181307.gj2...@alvh.no-ip.org
From aa79e331595860489cdbbdce2d5f35a7d1f33783 Mon Sep 17 00:00:00 2001
From: Emre Hasegeli 
Date: Wed, 25 May 2016 17:53:19 +0200
Subject: [PATCH] Stop using FP macros on geo_ops.c

---
 src/backend/access/gist/gistproc.c|  17 +-
 src/backend/access/spgist/spgkdtreeproc.c |  24 +--
 src/backend/utils/adt/geo_ops.c   | 312 +++---
 src/backend/utils/adt/geo_spgist.c|  24 +--
 src/include/utils/geo_decls.h |  16 --
 src/test/regress/expected/point.out   |   8 +-
 6 files changed, 193 insertions(+), 208 deletions(-)

diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index e8213e2..7fb4437 100644
--- a/src/backend/access/gist/gistproc.c
+++ 

Re: [HACKERS] Allow COPY to use parameters

[Craig Ringer]

On 27 May 2016 at 15:17, Andrew Gierth  wrote:

> > "Merlin" == Merlin Moncure  writes:
>
>  Merlin> Note, the biggest pain point I have with COPY is not being able
>  Merlin> to parameterize the filename argument.
>
> Second proof of concept attached. This goes so far as to allow
> statements like:
>
> do $$
>   declare t text := 'bar'; f text := '/tmp/copytest.dat';
>   begin copy (select t, now()) to (f) csv header; end;
> $$;
>
> Also "copy foo to $1" or "copy (select * from foo where x=$1) to $2" and
> so on should work from PQexecParams or in a plpgsql EXECUTE.
>
> (I haven't tried to parameterize anything other than the filename and
> query. Also, it does not accept arbitrary expressions - only $n, '...'
> or a columnref. $n and '...' can have parens or not, but the columnref
> must have them due to conflicts with unreserved keywords PROGRAM, STDIN,
> STDOUT. This could be hacked around in other ways, I guess, if the
> parens are too ugly.)
>
>
In addition to it being generally nice to be able to send parameters to
COPY (SELECT ...), I'd personally like very basic and limited parameter
support for all utility statements so clients can use the v3 protocol and
parameter binding without having to figure out the type of statement.

Currently users have to know "Oh, this is a utility statement and can't be
parameterised, so instead of using my client driver's parameter binding I
have to do string interpolation".

SQL injection detection static analysis and trace tools might complain, and
it means we have to tell users to do exactly what they should otherwise
never do, but there's not really a way around it right now.

To make things more complicated, some client drivers use the simple-query
protocol and do client-side in-driver parameter interpolation. Like
psycopg2. Such drivers cannot easily enable use of server-side binding and
extended query protocol because right now they have to look at the SQL and
figure out which statements can be bound server-side and which require
client-side interpolation.

For drivers like PgJDBC that do support parameter binding the application
programmer has to know they can't use it for some kinds of statement, and
have to do string interpolation on the SQL string. Carefully, if they're
doing anything with client supplied data.

IMO this is a bit of a wart in Pg, and it'd be nice to get rid of it... but
I'm aware it might not be worth the server-side complexity of handling
parameter binding in utility statements.

But.

People will want to be able to parameterise identifiers too. There's just
no way to do that. For plannable or utility statements. You can't write


SELECT ... FROM $1;

or

COPY FROM $1 TO 'myfilename'


... and users have to know that and deal with it. By string interpolation.
So even if parameter binding for literals in utility statements becomes
possible it doesn't mean users never have to interpolate stuff.

I don't think it's much worse to say "you can't use parameter binding in
this statement type" than it is to say "you can't use paremeter binding for
identifiers". So I'm not really that excited to solve this unless there's a
way to solve it _just_ for SQL expressions within utility statements like
COPY (SELECT ...).

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

Re: [HACKERS] Fix a failure of pg_dump with !HAVE_LIBZ

[Kyotaro HORIGUCHI]

At Thu, 26 May 2016 11:54:35 -0400, Tom Lane  wrote in 
<7601.1464278...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > The warning says that it makes uncompressed archive but it really
> > doesn't since workers die unexpectedly from the succeeding
> > errors. This is because that compressLevel is corrected in
> > ReadHead(), where too late for it to be propagated to workers.
> > One reasonable place would be where the default value of
> > compressLevel is set in main().
> 
> Yeah, agreed.  I also noticed that you get the WARNING even when you
> did not ask for compression, which seems rather unhelpful and annoying.
> So I suppressed it by making the default be 0 not Z_DEFAULT_COMPRESSION
> when we don't HAVE_LIBZ.

Yeah, that's what I thought but didn't. Thanks for adding it and
committing this.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat

[Kyotaro HORIGUCHI]

At Thu, 26 May 2016 10:53:47 -0400, Tom Lane  wrote in 
<5237.1464274...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> >> Next, I got the following behavior for the following command,
> >> then freeze. Maybe stopping at the same point with the next
> >> paragraph but I'm not sure. The same thing occurs this patch on
> >> top of the current master but doesn't on Linux.
> 
> > [ need to close command sockets in ShutdownWorkersHard ]
> 
> Hah.  I had made that same change in the cosmetic-cleanups patch I was
> working on ... but I assumed it wasn't a live bug or we'd have heard
> about it.
> 
> Pushed along with the comment improvements I'd made to that function.

Thank you!

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat

[Kyotaro HORIGUCHI]

At Thu, 26 May 2016 12:15:29 -0400, Tom Lane  wrote in 
<8273.1464279...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > At Wed, 25 May 2016 10:11:28 -0400, Tom Lane  wrote in 
> > <24577.1464185...@sss.pgh.pa.us>
> >> The only case
> >> that is certain to work is switches before non-switch arguments, and so
> >> we should not give any documentation examples in the other order.  On a
> >> platform using GNU getopt(), getopt() will rearrange the argv[] array to
> >> make such cases work, but non-GNU getopt() doesn't do that (and I don't
> >> really trust the GNU version not to get confused, either).
> 
> > Yeah, I knew it after reading port/getopt_long.c. But the error
> > message seems saying nothing about that (at least to me). And
> > those accumstomed to the GNU getopt's behavior will fail to get
> > the point of the message. The documentation also doesn't
> > explicitly saying about the restriction; it is only implicitly
> > shown in synopsis. How about something like the following
> > message? (The patch attached looks too dependent on the specific
> > behavior of our getopt_long.c, but doing it in getopt_long.c also
> > seems to be wrong..)
> 
> It's not a bad idea to try to improve our response to this situation,
> but I think this patch needs more work.  I wonder in particular why
> you're not basing the variant error message on the first unprocessed
> argument starting with '-', rather than looking at the word before it.

Sorry, it just comes from my carelessness. It is correct to do
what you wrote as above. And it is also the cause of my obscure
error message.

> Another thought is that the message is fairly unhelpful because it does
> not show where in the arguments things went wrong.  Maybe something
> more like
> 
>   if (argv[optind][0] == '-')
>   fprintf(stderr, _("%s: misplaced option \"%s\": options must 
> appear before non-option arguments\n"),
>   progname, argv[optind]);
>   else
>   // existing message
> 
> In any case, if we wanted to do something about this scenario, we should
> do it consistently across all our programs, not just pg_dump.  I count
> 25 appearances of that "too many command-line arguments" message...

Although I suppose no one has ever complained before about that,
and the same thing will occur on the other new programs even if
the programs are fixed now..

I'll consider more on it for some time..

Thank you for the suggestion.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Allow COPY to use parameters

[Andrew Gierth]

> "Merlin" == Merlin Moncure  writes:

 Merlin> Note, the biggest pain point I have with COPY is not being able
 Merlin> to parameterize the filename argument.

Second proof of concept attached. This goes so far as to allow
statements like:

do $$
  declare t text := 'bar'; f text := '/tmp/copytest.dat';
  begin copy (select t, now()) to (f) csv header; end;
$$;

Also "copy foo to $1" or "copy (select * from foo where x=$1) to $2" and
so on should work from PQexecParams or in a plpgsql EXECUTE.

(I haven't tried to parameterize anything other than the filename and
query. Also, it does not accept arbitrary expressions - only $n, '...'
or a columnref. $n and '...' can have parens or not, but the columnref
must have them due to conflicts with unreserved keywords PROGRAM, STDIN,
STDOUT. This could be hacked around in other ways, I guess, if the
parens are too ugly.)

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3201476..97debb7 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -37,6 +37,8 @@
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
 #include "nodes/makefuncs.h"
+#include "nodes/nodeFuncs.h"
+#include "parser/analyze.h"
 #include "rewrite/rewriteHandler.h"
 #include "storage/fd.h"
 #include "tcop/tcopprot.h"
@@ -279,13 +281,13 @@ static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
 
 
 /* non-export function prototypes */
-static CopyState BeginCopy(bool is_from, Relation rel, Node *raw_query,
+static CopyState BeginCopy(bool is_from, Relation rel, Node *raw_query, ParamListInfo params,
 		  const char *queryString, const Oid queryRelId, List *attnamelist,
 		  List *options);
 static void EndCopy(CopyState cstate);
 static void ClosePipeToProgram(CopyState cstate);
-static CopyState BeginCopyTo(Relation rel, Node *query, const char *queryString,
-			const Oid queryRelId, const char *filename, bool is_program,
+static CopyState BeginCopyTo(Relation rel, Node *query, ParamListInfo params, const char *queryString,
+			const Oid queryRelId, Node *filename_expr, bool is_program,
 			List *attnamelist, List *options);
 static void EndCopyTo(CopyState cstate);
 static uint64 DoCopyTo(CopyState cstate);
@@ -767,6 +769,43 @@ CopyLoadRawBuf(CopyState cstate)
 }
 
 
+static char *
+CopyEvalFilename(QueryDesc *qd, Node *expr, ParamListInfo params)
+{
+	char *filename = NULL;
+
+	if (expr)
+	{
+		EState *estate = qd ? qd->estate : CreateExecutorState();
+		MemoryContext oldcontext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+		Assert(exprType(expr) == CSTRINGOID);
+
+		if (qd == NULL)
+			estate->es_param_list_info = params;
+
+		{
+			ExprContext *ecxt = CreateExprContext(estate);
+			ExprState *exprstate = ExecPrepareExpr(copyObject(expr), estate);
+			bool isnull;
+			Datum result = ExecEvalExprSwitchContext(exprstate, ecxt, , NULL);
+			if (!isnull)
+filename = MemoryContextStrdup(oldcontext, DatumGetCString(result));
+			FreeExprContext(ecxt, true);
+		}
+
+		MemoryContextSwitchTo(oldcontext);
+
+		if (qd == NULL)
+			FreeExecutorState(estate);
+
+		if (!filename)
+			elog(ERROR, "COPY filename expression must not return NULL");
+	}
+
+	return filename;
+}
+
 /*
  *	 DoCopy executes the SQL COPY statement
  *
@@ -787,7 +826,7 @@ CopyLoadRawBuf(CopyState cstate)
  * the table or the specifically requested columns.
  */
 Oid
-DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
+DoCopy(const CopyStmt *stmt, const char *queryString, ParamListInfo params, uint64 *processed)
 {
 	CopyState	cstate;
 	bool		is_from = stmt->is_from;
@@ -906,7 +945,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
 			select->targetList = list_make1(target);
 			select->fromClause = list_make1(from);
 
-			query = (Node *) select;
+			query = (Node *) parse_analyze((Node *) select, queryString, NULL, 0);
 
 			/*
 			 * Close the relation for now, but keep the lock on it to prevent
@@ -929,6 +968,8 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
 
 	if (is_from)
 	{
+		char *filename;
+
 		Assert(rel);
 
 		/* check read-only transaction and parallel mode */
@@ -936,15 +977,20 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
 			PreventCommandIfReadOnly("COPY FROM");
 		PreventCommandIfParallelMode("COPY FROM");
 
-		cstate = BeginCopyFrom(rel, stmt->filename, stmt->is_program,
+		filename = CopyEvalFilename(NULL, stmt->filename, params);
+
+		cstate = BeginCopyFrom(rel, filename, stmt->is_program,
 			   stmt->attlist, stmt->options);
 		cstate->range_table = range_table;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
+
+		if (filename)
+			pfree(filename);
 	}
 	else
 	{
-		cstate = BeginCopyTo(rel, query, queryString, relid,
+		cstate = BeginCopyTo(rel, query, params, queryString, relid,
 			 stmt->filename, stmt->is_program,
 			 

Re: [HACKERS] PATCH: pg_restore parallel-execution-deadlock issue

[Amit Kapila]

On Fri, May 27, 2016 at 3:05 AM, Tom Lane  wrote:
>
> Michael Paquier  writes:
> > ea274b2 has changed the way disconnection is done is is now closing
> > both the read and write pipes. So you may want to retry if things get
> > better with the next round of minor releases.
>
> Hadn't paid attention to this thread before ...
>
> It looks like there are still a few things we need to deal with before
> considering Armin's submission resolved:
>
> 1. Armin proposes using "shutdown(pipeWrite, SD_BOTH)" where the code
> committed this morning (df8d2d8c4) has "closesocket(pipeWrite)".
> I'd prefer to leave it that way since it's the same as for the Unix case,
> and Kyotaro-san says it works for him.  Is there a reason we'd need
> shutdown() instead?
>
> 2. Armin proposes that WaitForTerminatingWorkers needs to do CloseHandle()
> on the various thread handles.  That sounds plausible but I don't know
> enough Windows programming to know if it really matters.
>
> 3. Should we replace ExitThread() with _endthreadex()?  Again, it
> seems plausible but I'm not the person to ask.
>

I think point (2) and (3) are related because using _endthreadex won't
close the thread handle explicitly [1].


[1] - https://msdn.microsoft.com/en-us/library/kdzttdcb.aspx
Refer line "*_endthread* automatically closes the thread handle, whereas
*_endthreadex* does not."

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

Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof

[Piotr Stefaniak]

On 2016-05-25 21:13, Tom Lane wrote:

Assuming this patch withstands more careful review, we will need to think
about project policy for how/when to apply such fixes.


I discovered yesterday that Bruce Evans had done the fix for sizeof in 
their fork of indent(1) in 2004 (r125623 [1]). The core fix is exactly 
the same as mine, he just did more fixes around it, which I haven't 
analyzed.


I'm trying to see if FreeBSD indent can successfully do pg_bsd_indent's 
job. So far I had to fix one thing, which is not adding a space after a 
cast operator, for which they added no option to turn it off. Currently 
I'm fighting one other bug, but I think that'll be it.


I took interest in FreeBSD's fork of indent(1) because they've fixed 
more things than NetBSD people have in their fork, it seems. I'm also 
hoping it'll be easier to reinvent GNU indent's -tsn ("set tabsize to n 
spaces") option for FreeBSD indent than it would be for any other of the 
forks that aren't GNU. I envision that to be the first step to getting 
rid of some of the work-arounds pgindent does, mainly running entab and 
detab as pre- and post-processing steps.


If porting FreeBSD indent to PostgreSQL's sources turns out to be 
successful, there will be a choice between rebasing pg_bsd_indent on 
that and picking specific patches and applying it on PG's fork of indent(1).


[1] 
https://svnweb.freebsd.org/base/head/usr.bin/indent/lexi.c?r1=125619=125623



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