Re: [HACKERS] Perf Benchmarking and regression.

[Andres Freund]

Hi,

On May 26, 2016 9:29:51 PM PDT, Ashutosh Sharma  wrote:
>Hi All,
>
>As we have seen the regression of more than 45% with
>"*backend_flush_after*"
>enabled and set to its default value i.e. 128KB or even when it is set
>to
>some higher value like 2MB, i think we should disable it such that it
>does
>not impact the read write performance and here is the attached patch
>for
>the same.  Please have a look and let me know your thoughts on this.
>Thanks!

I don't think the situation is quite that simple. By *disabling* backend 
flushing it's also easy to see massive performance regressions.  In situations 
where shared buffers was configured appropriately for the workload (not the 
case here IIRC).

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


-- 
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] Perf Benchmarking and regression.

[Ashutosh Sharma]

Hi All,

As we have seen the regression of more than 45% with "*backend_flush_after*"
enabled and set to its default value i.e. 128KB or even when it is set to
some higher value like 2MB, i think we should disable it such that it does
not impact the read write performance and here is the attached patch for
the same.  Please have a look and let me know your thoughts on this. Thanks!

With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com

On Sun, May 15, 2016 at 1:26 AM, Fabien COELHO  wrote:

>
> These raw tps suggest that {backend,bgwriter}_flush_after should better be
>>> zero for this kind of load.Whether it should be the default is unclear
>>> yet,
>>> because as Andres pointed out this is one kind of load.
>>>
>>
>> FWIW, I don't think {backend,bgwriter} are the same here. It's primarily
>> backend that matters.
>>
>
> Indeed, I was a little hasty to put bgwriter together based on this report.
>
> I'm a little wary of "bgwriter_flush_after" though, I would not be
> surprised if someone reports some regressions, although probably not with a
> pgbench tpcb kind of load.
>
> --
> Fabien.
>
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 38b6027..fff7104 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -60,7 +60,7 @@ extern PGDLLIMPORT int NBuffers;
 /* FIXME: Also default to on for mmap && msync(MS_ASYNC)? */
 #ifdef HAVE_SYNC_FILE_RANGE
 #define DEFAULT_CHECKPOINT_FLUSH_AFTER 32
-#define DEFAULT_BACKEND_FLUSH_AFTER 16
+#define DEFAULT_BACKEND_FLUSH_AFTER 0
 #define DEFAULT_BGWRITER_FLUSH_AFTER 64
 #else
 #define DEFAULT_CHECKPOINT_FLUSH_AFTER 0

-- 
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.6 Open Item Ownership

[Noah Misch]

On Wed, Mar 30, 2016 at 08:20:02PM -0400, Noah Misch wrote:
> The release management team has determined the following:
> 
>   From time to time, individual members of the release management team (RMT)
>   may attribute a PostgreSQL 9.6 open item to a particular git commit and
>   determine whether or not it is a beta1 blocker.  The RMT member will send a
>   notification email, which will request a plan for resolving the issue, To:
>   the associated committer and Cc: pgsql-hackers@.  For beta1 blockers, the
>   notification will specify delivery of a plan within 48 calendar hours and
>   resolution within 120 calendar hours.  For other issues, the notification
>   will specify delivery of a plan within 72 calendar hours and resolution
>   within 168 calendar hours.

The release management team has determined the following:

  An open item "owner" is a person taking overall responsibility for the work
  required to close a particular PostgreSQL 9.6 open item.  Tasks required to
  close an open item may include performing tests, persuading issue reporters to
  provide more information, writing patches, reviewing patches, committing
  patches, and providing status updates to the community.  For many complex
  issues, it will be impractical for the owner to perform all work personally.
  For example, a cautious owner may decline to both write and commit a tricky
  patch.  We encourage owners to petition other community members for aid.  At
  all times, the owner retains full responsibility for achieving progress.

  Release dates will be at risk if individual open items stay open for many
  weeks.  If owners manage their items well, the RMT will have minimal
  involvement.  So that the RMT can determine when to intervene, owners shall
  mail status updates to the issue thread.  Each update shall state a date when
  the community will receive another update and what, if anything, is happening
  in the intervening time.  Here are examples of status updates meeting that
  specification:

  I will start reviewing the proposed patch on {now() + $X days} and make it
  my top priority for $Y days after that.  By the end of $Y days, I will
  either have committed some patch or mailed a review of the proposed patch.

  I will test hypotheses $A and $B in my spare moments over the next $X
  days, then report back about what comes next based on those findings.

  I will not work on this before October, so I need the RMT to own it
  however it sees fit.

  I can make time to review fixes or to revert the patch, but I will not do
  most of the fix development.  $original_author, can you write the fix?
  Failing that, can anyone else?  I will follow up in 72 hours based on the
  responses I get.

  I would like to continue owning this $simple_cosmetic_item, but I want to
  help with $scary_item first.  I will send my plans for this item within
  five days of $scary_item being resolved by its owner.

  The RMT will treat the self-selected next update date as a deadline and
  anticipate another status update on or before that date.  Also, the RMT may
  intervene when status updates seem not to be swiftly converging toward a fix
  _and_ the current owner has held the item for at least one week.  Consuming
  more than two weeks in total will often attract RMT intervention.

  The default owner for an open item is the committer of the patch that caused
  the item.  (If a 9.6 commit made an old defect much easier to encounter,
  proceed as though the 9.6 patch caused the problem.)  We encourage committers
  acquiring ownership this way to reply to the open item thread acknowledging
  ownership and giving an initial status update.  Lacking such a message, the
  RMT will mail a notification To: the owner and Cc: pgsql-hackers@.  This
  notification will specify an initial status update within 72 calendar hours.

  The RMT encourages the patch author, if different from the committer, to
  vigorously help the item owner by maximizing the testing, patch writing, and
  other resolution work you do yourself.  This is an excellent way to
  demonstrate your active involvement in the community.

  Owners may transfer ownership to any other willing person.  (Non-committers,
  before accepting transfers, consider that your success will depend crucially
  on your ability to recruit a volunteer committer.)  The RMT is the item owner
  of last resort.  The RMT implicitly owns items not yet attributed to a commit;
  in that capacity, it will often solicit volunteers to research the causative
  commit.  When an owner proposes to transfer ownership to the RMT, the RMT will
  always accept.  However, the RMT will usually resolve the item by reverting
  patches or by a similarly low-cost, risk-averse method.

  Summary:
  - Committers own their commits' open items by default.
  - The owner always has a status update due at a known future date.
  - Items taking longer than 1-2 weeks are a 

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
I'll follow this mood. Yeha.


BTW, I've publushed the HTML-ified SGML docs to 
http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a preview.


Sorry for my late reply.  Fantastic performance improvement, nice 
documentation, and amazing rapid development!  I think I’ll join the review & 
testing in 2016/9 CF.

Regards
Takayuki Tsunakawa

Re: [HACKERS] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Tom Lane]

Tomas Vondra  writes:
> On 05/26/2016 10:10 PM, Tom Lane wrote:
>> In view of 52e8fc3e2, there's more or less no case in which we'd be
>> writing stats without writing stats for the shared catalogs. So I'm
>> tempted to propose that we try to reduce the overhead by merging the
>> shared-catalog stats back into the global-stats file, thereby
>> halving the filesystem metadata traffic for updating those.

> I find this a bit contradictory with the previous paragraph. If you 
> believe that reducing the filesystem metadata traffic will have a 
> measurable benefit, then surely merging writes for multiple dbs (thus 
> not writing the global/shared files multiple times) will have even 
> higher impact, no?

Well, my thinking is that this is something we could get "for free"
without any penalty in response time.  Going further will require
some sort of tradeoff.

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] Further stabilization of isolationtester's timeouts test

[Tom Lane]

I've been noticing recently that certain buildfarm members sometimes fail
the "timeouts" isolation test with this symptom:

*** 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/isolation/expected/timeouts.out  
Mon May 16 23:45:12 2016
--- 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/isolation/results/timeouts.out   
Mon May 16 23:53:08 2016
***
*** 70,73 
  step slto: SET lock_timeout = 6000; SET statement_timeout = 5000;
  step update: DELETE FROM accounts WHERE accountid = 'checking'; 
  step update: <... completed>
! ERROR:  canceling statement due to statement timeout
--- 70,73 
  step slto: SET lock_timeout = 6000; SET statement_timeout = 5000;
  step update: DELETE FROM accounts WHERE accountid = 'checking'; 
  step update: <... completed>
! ERROR:  canceling statement due to lock timeout

as for example here and here:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder=2016-05-16%2021%3A45%3A06
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder=2016-05-26%2016%3A45%3A03

A look at the postmaster log is informative:

2016-05-16 23:52:12.612 CEST [573a40e9.24a8:26] LOG:  statement: BEGIN 
ISOLATION LEVEL READ COMMITTED;
2016-05-16 23:52:12.612 CEST [573a40e9.10ef:50] LOG:  statement: BEGIN 
ISOLATION LEVEL READ COMMITTED;
2016-05-16 23:52:12.613 CEST [573a40e9.24a8:27] LOG:  statement: UPDATE 
accounts SET balance = balance + 100;
2016-05-16 23:52:12.613 CEST [573a40e9.10ef:51] LOG:  statement: SET 
lock_timeout = 6000; SET statement_timeout = 5000;
2016-05-16 23:52:12.614 CEST [573a40e9.10ef:52] LOG:  statement: DELETE FROM 
accounts WHERE accountid = 'checking';
2016-05-16 23:52:12.631 CEST [573a40e9.5afe:34] LOG:  execute 
isolationtester_waiting: SELECT pg_catalog.pg_blocking_pids($1) && 
'{9384,4335}'::integer[]
2016-05-16 23:52:12.631 CEST [573a40e9.5afe:35] DETAIL:  parameters: $1 = '4335'
2016-05-16 23:53:08.658 CEST [573a40e9.10ef:53] ERROR:  canceling statement due 
to lock timeout
2016-05-16 23:53:08.658 CEST [573a40e9.10ef:54] CONTEXT:  while deleting tuple 
(0,1) in relation "accounts"
2016-05-16 23:53:08.658 CEST [573a40e9.10ef:55] STATEMENT:  DELETE FROM 
accounts WHERE accountid = 'checking';
2016-05-16 23:53:08.659 CEST [573a40e9.24a8:28] LOG:  statement: ABORT;
2016-05-16 23:53:08.659 CEST [573a40e9.10ef:56] LOG:  statement: ABORT;

In this case the process seems to have gone to sleep for just short of a
minute rather than the expected 5 seconds.  Presumably that just reflects
overload on the buildfarm member rather than anything really exciting,
but it explains the failure nicely: by the time we got to postgres.c's
ProcessInterrupts(), both the lock and statement timeouts had expired,
and the code there preferentially reports "lock timeout" in that case.

What I propose we do about this is make ProcessInterrupts() compare
the scheduled timeout times if both timeouts have triggered, and report
whichever event was intended to happen first.  We should still apply
the "lock error first" tiebreaking rule if there's an exact tie, but
that's likely to be rare.

The attached patch does that, and also reduces the longer of the two
timeouts in these test cases to 5010 ms.  That should give us good
coverage in the buildfarm of both the case where both timeouts have
fired before reaching ProcessInterrupts(), and the case where they
have not.  We should get bulletproof reproducibility of which timeout
is reported, as long as the system clock doesn't go backwards during
the test.

Unfortunately this doesn't do anything to let us reduce the base 5000-ms
timeout :-(.  That's driven by the need to be sure that isolationtester
has detected that the processes are waiting, which is independent of this
problem.

I'd like to put this into all branches back to 9.3, since we've seen
this type of failure in all of them.  Any objections?

regards, tom lane

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 68811f1..b185c1b 100644
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*** ProcessInterrupts(void)
*** 2909,2914 
--- 2909,2917 
  
  	if (QueryCancelPending)
  	{
+ 		bool		lock_timeout_occurred;
+ 		bool		stmt_timeout_occurred;
+ 
  		/*
  		 * Don't allow query cancel interrupts while reading input from the
  		 * client, because we might lose sync in the FE/BE protocol.  (Die
*** ProcessInterrupts(void)
*** 2929,2945 
  
  		/*
  		 * If LOCK_TIMEOUT and STATEMENT_TIMEOUT indicators are both set, we
! 		 * prefer to report the former; but be sure to clear both.
  		 */
! 		if (get_timeout_indicator(LOCK_TIMEOUT, true))
  		{
- 			(void) get_timeout_indicator(STATEMENT_TIMEOUT, true);
  			LockErrorCleanup();
  			ereport(ERROR,
  	(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
  	 errmsg("canceling statement due to lock timeout")));
  		}
! 		if (get_timeout_indicator(STATEMENT_TIMEOUT, true))
  		{
  			LockErrorCleanup();
  			

Re: [HACKERS] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Michael Paquier]

On Thu, May 26, 2016 at 6:43 PM, Tomas Vondra
 wrote:
> On 05/26/2016 10:10 PM, Tom Lane wrote:
>> Tomas Vondra  writes:
>> In view of 52e8fc3e2, there's more or less no case in which we'd be
>> writing stats without writing stats for the shared catalogs. So I'm
>> tempted to propose that we try to reduce the overhead by merging the
>> shared-catalog stats back into the global-stats file, thereby
>> halving the filesystem metadata traffic for updating those.
>
> [...]
>
> That being said, I'm not opposed to merging the shared catalog into the
> global-stats file - it's not really a separate database so having it in a
> separate file is a bit weird.

While looking at this stuff, to be honest I got surprised that shared
relation stats are in located in a file whose name depends on
InvalidOid, so +1 from here as well to merge that into the global
stats file.
-- 
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] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Tomas Vondra]

Hi,

On 05/26/2016 10:10 PM, Tom Lane wrote:

Tomas Vondra  writes:

Attached is a patch that should fix the coalescing, including the clock
skew detection. In the end I reorganized the code a bit, moving the
check at the end, after the clock skew detection. Otherwise I'd have to
do the clock skew detection on multiple places, and that seemed ugly.


I hadn't been paying any attention to this thread, I must confess.
But I rediscovered this no-coalescing problem while pursuing the poor
behavior for shared catalogs that Peter complained of in
https://www.postgresql.org/message-id/56ad41ac.1030...@gmx.net

I posted a patch at
https://www.postgresql.org/message-id/13023.1464213...@sss.pgh.pa.us
which I think is functionally equivalent to what you have here, but
it goes to some lengths to make the code more readable, whereas this
is just adding another layer of complication to something that's
already a mess (eg, the request_time field is quite useless as-is).
So I'd like to propose pushing that in place of this patch ... do
you care to review it first?


I do care and I'll look at it over the next few days. FWIW when writing 
that patch I intentionally refrained from major changes, as I think the 
plan was to backpatch it. But +1 for more readable code from me.




Reacting to the thread overall:

I see Noah's concern about wanting to merge the write work for
requests about different databases. I've got mixed feelings about
that: it's arguable that any such change would make things worse not
better. In particular, it's inevitable that trying to merge writes
will result in delaying the response to the first request, whether
or not we are able to merge anything. That's not good in itself, and
it means that we can't hope to merge requests over any very long
interval, which very possibly will prevent any merging from
happening in real situations. Also, considering that we know the
stats collector can be pretty slow to respond at all under load, I'm
 worried that this would result in more outright failures.

Moreover, what we'd hope to gain from merging is fewer writes of the
global stats file and the shared-catalog stats file; but neither of
those are very big, so I'm skeptical of what we'd win.


Yep. Clearly there's a trade-off between slowing down response to the 
first request vs. speeding-up the whole process, but as you point out we 
probably can't gain enough to justify that.


I wonder if that's still true on clusters with many databases (say, 
shared systems with thousands of dbs). Perhaps walking the list just 
once would save enough CPU to make this a win.


In any case, if we decide to abandon the idea of merging requests for 
multiple databases, that probably means we can further simplify the 
code. last_statrequests is a list but it actually never contains more 
than just a single request. We kept it that way because of the plan to 
add the merging. But if that's not worth it ...




In view of 52e8fc3e2, there's more or less no case in which we'd be
writing stats without writing stats for the shared catalogs. So I'm
tempted to propose that we try to reduce the overhead by merging the
shared-catalog stats back into the global-stats file, thereby
halving the filesystem metadata traffic for updating those.


I find this a bit contradictory with the previous paragraph. If you 
believe that reducing the filesystem metadata traffic will have a 
measurable benefit, then surely merging writes for multiple dbs (thus 
not writing the global/shared files multiple times) will have even 
higher impact, no?


E.g. let's assume we're still writing the global+shared+db files for 
each database. If there are requests for 10 databases, we'll write 30 
files. If we merge those requests first, we're writing only 12 files.


So I'm not sure about the metadata traffic argument, we'd need to see 
some numbers showing it really makes a difference.


That being said, I'm not opposed to merging the shared catalog into the 
global-stats file - it's not really a separate database so having it in 
a separate file is a bit weird.


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] foreign table batch inserts

[Michael Paquier]

On Thu, May 26, 2016 at 4:25 AM, Etsuro Fujita
 wrote:
> On 2016/05/18 7:08, Michael Paquier wrote:
>>
>> On Wed, May 18, 2016 at 6:00 AM, Manuel Kniep  wrote:
>>>
>>> I realized that inserts into foreign tables are only done row by row.
>>> Consider copying data from one local table to a foreign table with
>>>
>>> INSERT INTO foreign_table(a,b,c) SELECT a,b,c FROM local_table;
>>>
>>> When the foreign  server is for example in another datacenter with long
>>> latency,
>>> this as an enormous performance trade off.
>
>
>> I am adding Fujita-san in the loop here, he is
>> quite involved with postgres_fdw these days so perhaps he has some
>> input to offer.
>
>
> Honestly, I didn't have any idea for executing such an insert efficiently,
> but I was thinking to execute an insert into a foreign table efficiently, by
> sending the whole insert to the remote server, if possible.  For example, if
> the insert is of the form:
>
> INSERT INTO foreign_table(a,b,c) VALUES (1, 2, 3), (4, 5, 6) or
> INSERT INTO foreign_table(a,b,c) SELECT a,b,c FROM foreign_table2
>
> where foreign_table and foreign_table2 belong to the same foreign server,
> then we could send the whole insert to the remote server.
>
> Wouldn't that make sense?

Query strings have a limited length, and this assumption is true for
many code paths in the backend code, so doing that with a long string
would introduce more pain in the logic than anything else, as this
would become more data type sensitive.
-- 
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] Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618

[Michael Paquier]

On Thu, May 26, 2016 at 3:01 AM, Amit Kapila  wrote:
> On Wed, May 25, 2016 at 9:44 PM, Michael Paquier 
> wrote:
>>
>> On Wed, May 25, 2016 at 12:11 AM, Amit Kapila 
>> wrote:
>> >
>> > Okay, attached patch just does that and I have verified that it allows
>> > to
>> > start multiple services in windows.  In off list discussion with Robert,
>> > he
>> > suggested not to complicate the patch by retrying for fixed number of
>> > times
>> > as there is no proof that ERROR_ACCESS_DENIED can occur due to any other
>> > reason in this code path.  This patch is based on Kyotaro san's patch
>> > posted
>> > upthread with just minor changes in comments and indentation.
>>
>> Thanks for catching Robert and getting confirmation on the matter. In
>> the same line of thoughts, I think as well that it is definitely not
>> worth complicating the retry logic in dsm.c, but you knew that already
>> per last week's discussion.
>>
>> Regarding the patch, this looks good to me.
>>
>
> Thanks for reviewing the patch.  I have added the entry for this patch in
> next CF (https://commitfest.postgresql.org/10/636/),  feel free to mark it
> as Ready for committer if you think patch is good.

Yeah, it is definitely better to register it. And I have switched the
patch as ready for committer just now.
-- 
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] Adding an alternate syntax for Phrase Search

[Oleg Bartunov]

On Thu, May 26, 2016 at 3:00 PM, Josh berkus  wrote:
> On 05/22/2016 06:53 PM, Teodor Sigaev wrote:
>>
>>> to_tsquery(' Berkus & "PostgreSQL Version 10.0" ')
>>>
>>> ... would be equivalent to:
>>>
>>> to_tsquery(' Berkus & ( PostgreSQL <-> version <-> 10.0 )')
>>
>> select to_tsquery('Berkus') && phraseto_tsquery('PostgreSQL Version 10.0');
>> does it as you wish
>
> Aha, you didn't mention this in your presentation.  That seems plenty
> good enough for 9.6.

Will add this to the slides.


>
> --
> --
> Josh Berkus
> Red Hat OSAS
> (any opinions are my own)
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


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

[Tom Lane]

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.

regards, tom lane


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

Re: [HACKERS] pg_dump -j against standbys

[Magnus Hagander]

On Thu, May 26, 2016 at 5:57 PM, Tom Lane  wrote:

> Magnus Hagander  writes:
> > Here's an updated patch based on this,and the other feedback.
>
> Looks sane in a quick once-over, but I haven't tested it.
>
>
I've run some tests and it looks good. Will apply. Thanks for the quick
look!



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

Re: [HACKERS] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Tom Lane]

Tomas Vondra  writes:
> Attached is a patch that should fix the coalescing, including the clock 
> skew detection. In the end I reorganized the code a bit, moving the 
> check at the end, after the clock skew detection. Otherwise I'd have to 
> do the clock skew detection on multiple places, and that seemed ugly.

I hadn't been paying any attention to this thread, I must confess.
But I rediscovered this no-coalescing problem while pursuing the poor
behavior for shared catalogs that Peter complained of in
https://www.postgresql.org/message-id/56ad41ac.1030...@gmx.net

I posted a patch at
https://www.postgresql.org/message-id/13023.1464213...@sss.pgh.pa.us
which I think is functionally equivalent to what you have here, but
it goes to some lengths to make the code more readable, whereas this
is just adding another layer of complication to something that's
already a mess (eg, the request_time field is quite useless as-is).
So I'd like to propose pushing that in place of this patch ... do you
care to review it first?

Reacting to the thread overall:

I see Noah's concern about wanting to merge the write work for requests
about different databases.  I've got mixed feelings about that: it's
arguable that any such change would make things worse not better.
In particular, it's inevitable that trying to merge writes will result
in delaying the response to the first request, whether or not we are
able to merge anything.  That's not good in itself, and it means that
we can't hope to merge requests over any very long interval, which very
possibly will prevent any merging from happening in real situations.
Also, considering that we know the stats collector can be pretty slow
to respond at all under load, I'm worried that this would result in
more outright failures.

Moreover, what we'd hope to gain from merging is fewer writes of the
global stats file and the shared-catalog stats file; but neither of
those are very big, so I'm skeptical of what we'd win.

In view of 52e8fc3e2, there's more or less no case in which we'd be
writing stats without writing stats for the shared catalogs.  So I'm
tempted to propose that we try to reduce the overhead by merging the
shared-catalog stats back into the global-stats file, thereby halving
the filesystem metadata traffic for updating those.

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] statistics for shared catalogs not updated when autovacuum is off

[Tom Lane]

Noah Misch  writes:
> You may want to compare your patch to this pending patch for the same bug:
> http://www.postgresql.org/message-id/flat/24f09c2d-e5bf-1f73-db54-8255c1280...@2ndquadrant.com

Oh, interesting.  I had not been paying any attention to that thread.
I'll go respond over there.

regards, tom lane


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

Re: [HACKERS] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Tom Lane]

Nikolay Shaplov  writes:
> Actually I did not expected any discussion for this case. Documentations 
> missed an optional keyword, documentation should be fixed.

99% of the time, you'd be right.  But this is an unusual case, for the
reasons I mentioned 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] Adding an alternate syntax for Phrase Search

[Josh berkus]

On 05/22/2016 06:53 PM, Teodor Sigaev wrote:
> 
>> to_tsquery(' Berkus & "PostgreSQL Version 10.0" ')
>>
>> ... would be equivalent to:
>>
>> to_tsquery(' Berkus & ( PostgreSQL <-> version <-> 10.0 )')
> 
> select to_tsquery('Berkus') && phraseto_tsquery('PostgreSQL Version 10.0');
> does it as you wish

Aha, you didn't mention this in your presentation.  That seems plenty
good enough for 9.6.

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)


-- 
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] ORDER/GROUP BY expression not found in targetlist

[Tom Lane]

Andres Freund  writes:
> trying to reproduce a performance problem I just found:

> =# CREATE TABLE twocol(col01 int, col02 int);
> =# SELECT DISTINCT col01, col02, col01 FROM twocol ;
> ERROR:  XX000: ORDER/GROUP BY expression not found in targetlist
> LOCATION:  get_sortgroupref_tle, tlist.c:341

> which appears to be a 9.6 regression, presumable fallout from the path
> restructuring.

Huh.  The problem is that createplan.c is trying to apply the
physical-tlist optimization to the seqscan underneath the aggregate
node.  That means that the output from the seqscan is just
"col01, col02", which means that col01 can only be decorated with
a single ressortgroupref ... but there are two ressortgrouprefs
for it as far as the groupClause is concerned.  Only one gets applied
to the seqscan's tlist, and then later we fail because we don't find
the other one there.  Conclusions:

* we need to back off the physical-tlist optimization in this case

* the code that transfers sortgroupref labels onto a tlist probably
ought to notice and complain if it's asked to put inconsistent labels
onto the same column.

I'm a little surprised that it's not discarding the third grouping
item as redundant ... but that's probably not something to mess with
right now.  Prior versions don't appear to do that either.

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

[Jim Nasby]

On 5/25/16 8:19 PM, Craig Ringer wrote:

Postgres is well past the point where our relational features are
the big selling point. It's now about scale, an incredibly robust
storage engine, and all the extensiblity opportunities. We've moved
from being an RDBMS to being a "Data Platform". Improving our OO
capabilities just continues that.


Right. I'm just not convinced table inheritance is a useful part of that
picture.


In it's current state it's certainly not the best tool in the toolbox, 
but IMHO there's still plenty of room for the ability to support 
restricted polymorphism, because of all the benefits of not allowing 
willy-nilly stuff in the schema. There's certainly other cases (ie: 
per-customer custom fields) where willy-nilly is what you actually need.


And certainly things that reduce table modification pain for *any* 
operations are most welcome! I think allowing queued backgrounded 
processes would be a huge win there. If we had real stored procedures 
(IE: ability to control transaction state) and a modest background 
scheduler then it wouldn't be hard to build that.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

[ redirecting to -hackers ]

 writes:
> When performing a vanilla database restore using either the 9.2.16 or 9.2.17 
> executables (i.e. just restoring the database files from a 'tar' backup and 
> reading the WAL files created during the 'tar' backup - no specific PIT given 
> in recovery.conf) the database server will abort with a permission denied 
> error on the pg_xlog/RECOVERYXLOG file.  The error occurred restoring both 
> backups that were made under the current version (9.2.16 and 9.2.17) as well 
> as backups made under prior versions (9.2.15 at least).  The exact same 
> restore process/backup files can be used to successfully restore the database 
> using the 9.2.15 executables, but fail when using either 9.2.16 or 9.2.17 
> with the permission denied error.

There were not very many changes between 9.2.15 and 9.2.16.  Between that
and the location of the error:

> 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
> 2016-04-27 17:02:06 EDT 572128cd.1811 [8-1] user=,db=,remote= LOCATION:
> fsync_fname_ext, fd.c:2654

I feel pretty confident in blaming this on the "durable_rename" patch.

The proximate cause of this might just be that the "ignore_perm" exception
is only for EACCES and not EPERM (why?).  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.
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.

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

[Tom Lane]

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

regards, tom lane


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

Re: [HACKERS] pg_dump -j against standbys

[Tom Lane]

Magnus Hagander  writes:
> Here's an updated patch based on this,and the other feedback.

Looks sane in a quick once-over, but I haven't tested 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] Fix a failure of pg_dump with !HAVE_LIBZ

[Tom Lane]

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.

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] ORDER/GROUP BY expression not found in targetlist

[Tom Lane]

Andreas Seltenreich  writes:
> Tom Lane writes:
>> It's looking for an operator that is known to be semantically equality,
>> by virtue of being the equality member of a btree or hash opclass.
>> Type path has no such opclass unfortunately.

> As do lots of data types in the regression db while still having an
> operator providing semantic equivalence.  I was hoping for someone to
> question that status quo.  Naively I'd say an equivalence flag is
> missing in the catalog that is independent of opclasses.

[ shrug... ]  I see little wrong with that status quo.  For this
particular use-case, there are two ways we could implement DISTINCT: one
of them requires sorting, and the other requires hashing.  So you would
need to provide that opclass infrastructure even if there were some other
way of identifying the operator that means equality.

Type path and the other geometric types lack any natural sort order so
it's hard to imagine making a default btree opclass for them.  But a
default hash opclass might not be out of reach, given an exact equality
operator.

Another problem with the geometric types is that long ago somebody
invented "=" operators for most of them that have little to do with what
anyone would consider equality.  The "path = path" operator just compares
whether the paths have the same number of points.  A lot of the other ones
compare areas.  It'd be hard to justify marking any of them as default
equality for the type.

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

[Tom Lane]

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.

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] ORDER/GROUP BY expression not found in targetlist

[Andreas Seltenreich]

Tom Lane writes:

> Andreas Seltenreich  writes:
>> Peter Geoghegan writes:
>>> It's surprising that SQL Smith didn't catch something with such simple
>>> steps to reproduce.
>
>> I removed distinct relatively early because it causes a large part of
>> queries to fail due to it not finding an equality operator it likes.  It
>> seems to be more picky about the equality operator than, say, joins.
>> I'm sure it has a good reason to do so?
>
> It's looking for an operator that is known to be semantically equality,
> by virtue of being the equality member of a btree or hash opclass.
> Type path has no such opclass unfortunately.

As do lots of data types in the regression db while still having an
operator providing semantic equivalence.  I was hoping for someone to
question that status quo.  Naively I'd say an equivalence flag is
missing in the catalog that is independent of opclasses.

regards
Andreas


-- 
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][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Nikolay Shaplov]

В письме от 26 мая 2016 10:05:56 пользователь Tom Lane написал:

> > 2. I think expression with USING in it is more human readable:
> > CREATE INDEX (xxx op_yyy);
> > is less sensible then
> > CREATE INDEX (xxx USING op_yyy);
> 
> Yes.  If we were working in a green field, it would have been better to
> make USING (or some other reserved word) required, not optional, there.
> That would have been better for readability and would have avoided some
> syntactic headaches, such as the need to parenthesize the expressions in
> expression indexes.  However, we're about 18 years too late to make that
> decision.  Opclass with no introductory keyword is the entrenched standard
> at this point, and we're never going to be able to remove it.
No, but we cat keep "USING" as an optional keyword there as it were, just 
mention it in the docs. It seems logical to me. 

// Actually I did not expected any discussion for this case. Documentations 
missed an optional keyword, documentation should be fixed. I am surprised that 
there can be any other opinions ;-)

-- 
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] ORDER/GROUP BY expression not found in targetlist

[Tom Lane]

Andreas Seltenreich  writes:
> Peter Geoghegan writes:
>> It's surprising that SQL Smith didn't catch something with such simple
>> steps to reproduce.

> I removed distinct relatively early because it causes a large part of
> queries to fail due to it not finding an equality operator it likes.  It
> seems to be more picky about the equality operator than, say, joins.
> I'm sure it has a good reason to do so?

It's looking for an operator that is known to be semantically equality,
by virtue of being the equality member of a btree or hash opclass.
Type path has no such opclass unfortunately.  But when you write "a = b"
that just looks for an operator named "=".

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] ORDER/GROUP BY expression not found in targetlist

[Andreas Seltenreich]

Peter Geoghegan writes:

> On Wed, May 25, 2016 at 7:12 PM, Andres Freund  wrote:
>>
>> =# CREATE TABLE twocol(col01 int, col02 int);
>> =# SELECT DISTINCT col01, col02, col01 FROM twocol ;
>> ERROR:  XX000: ORDER/GROUP BY expression not found in targetlist
>> LOCATION:  get_sortgroupref_tle, tlist.c:341
>>
>> which appears to be a 9.6 regression, presumable fallout from the path
>> restructuring.
>
> It's surprising that SQL Smith didn't catch something with such simple
> steps to reproduce.

I removed distinct relatively early because it causes a large part of
queries to fail due to it not finding an equality operator it likes.  It
seems to be more picky about the equality operator than, say, joins.
I'm sure it has a good reason to do so?

regression=> select distinct f1 from path_tbl;
ERROR:  could not identify an equality operator for type path
LINE 1: select distinct f1 from path_tbl;

regression=> \do =
-[ RECORD 38 ]-+
Schema | pg_catalog
Name   | =
Left arg type  | path
Right arg type | path
Result type| boolean
Description| equal



-- 
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][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Tom Lane]

Nikolay Shaplov  writes:
> В письме от 24 мая 2016 12:47:20 пользователь Tom 
> Lane написал:
>> I think we should seriously consider fixing this code/docs discrepancy
>> by making the code match the docs, not vice versa.  That is, let's just
>> remove the USING alternative here entirely.

> I have two arguments for not removing USING there. 

> 1. Backward compatibility. Are you sure, that nobody ever wrote a code using 
> this "USING" keyword? I would say it is unlikely, but will not be sure 100%.

I would say the same.  However, we make incompatible changes in every
major release, and many of them are way harder to deal with than this.

> 2. I think expression with USING in it is more human readable:
> CREATE INDEX (xxx op_yyy);
> is less sensible then 
> CREATE INDEX (xxx USING op_yyy);

Yes.  If we were working in a green field, it would have been better to
make USING (or some other reserved word) required, not optional, there.
That would have been better for readability and would have avoided some
syntactic headaches, such as the need to parenthesize the expressions in
expression indexes.  However, we're about 18 years too late to make that
decision.  Opclass with no introductory keyword is the entrenched standard
at this point, and we're never going to be able to remove 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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[David G. Johnston]

On Thu, May 26, 2016 at 8:09 AM, Nikolay Shaplov 
wrote:

> В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
> > Nikolay Shaplov  writes:
> > > If I read gram.y code for insert statement, I see that there is an
> > > optional
> > > USING keyword before opclass name
> > >
> > > opt_class:  any_name{ $$ = $1; }
> > >
> > > | USING any_name{ $$ = $2; }
> > > | /*EMPTY*/ { $$ = NIL; }
> > >
> > > ;
> > >
> > > but it the documentation this keyword is omitted.
> >
> > I think we should seriously consider fixing this code/docs discrepancy
> > by making the code match the docs, not vice versa.  That is, let's just
> > remove the USING alternative here entirely.
> >
> > If we wanted to make the docs match the code, it would not only be
> > CREATE INDEX that would have to be patched, because that's not the
> > only place that index_elem can appear.  As far as I can find in a
> > quick search, none of the relevant statements have ever documented
> > that USING is allowed here; nor does it appear that any client-side
> > code of ours makes use of the keyword.
> >
> > Also, because USING is already used elsewhere in CREATE INDEX (to
> > introduce the optional index AM name), I think that documenting its
> > use in this clause would add confusion not subtract it.  References
> > to "the USING clause in CREATE INDEX" would become ambiguous.
> >
> > This wouldn't be something to back-patch, of course, but I think it's
> > an entirely reasonable change to make in HEAD.
> >
> > Comments?
>
> I have two arguments for not removing USING there.
>
> 1. Backward compatibility. Are you sure, that nobody ever wrote a code
> using
> this "USING" keyword? I would say it is unlikely, but will not be sure
> 100%.
> Dropping this backward compatibility (even with small chance that it was
> ever
> used) for no practical reason is not wise at all. If it might bring some
> pain
> to somebody, then why do it after all...
>

IIUC, since pg_dump/pg_restore never includes this there is not hazard on
upgrading or restoration.  Furthermore, CREATE INDEX is an administrative
function and thus not likely to cause applications to become inoperative.​

The surface area here is small enough that the decision to disallow should
not be taken off the table.


> 2. I think expression with USING in it is more human readable:
>
> CREATE INDEX (xxx op_yyy);
>
> is less sensible then
>
> CREATE INDEX (xxx USING op_yyy);
>
> in my opinion. In second example person that does not know SQL at all, will
> understand that xxx is main object or action, and op_yyy is about how this
> xxx
> will be done or used.
>
> If somebody would like to write more readable code, why we should forbid
> it to
> him?
>

​I agree.​

​The argument that having a second portion of the query utilizing the USING
keyword would make explanation and comprehension more difficult is also one
I agree with.​

That said we apparently used to interject the word WITH in between but that
ended up generating a potential ambiguity so WITH was changed to USING.
This was circa 1997...

The authors of the docs for the past nearly 20 years have assumed that
there is no USING clause in that location.

If someone was willing to write a doc patch that passed muster before 10.0
goes live its possible that we'd revert the change and commit the doc
patch.  The cost/benefit of that effort is not particularly appealing and
the project seems content to take the more expedient (and now without its
own merits) path forward.


> 2.1. As far as I can get the general idea of SQL, there is a tendency to
> put
> keywords (optional or not) between to object names. Like this
>
> SELECT a _AS_ b from 
>
> I like this tendency
>

Not germane to this discussion.​


> 2.2. I am not familiar with SQL ISO standart, and I suppose there is no
> USING
> at all in that case, but I think it would be good to look there to check
> for
> it or for something similar
>

​Indexes are outside the purview of the ISO SQL Standard.​


> 2.3. And the last, when I found out about this keyword, I started to use
> it in
> my SQL statements that I use while development, and I just liked it. I will
> miss it if you remove it ;-)
>

​Thank you for your patronage and your sacrifice.​

Is there an address where we can send your purple heart :)

While not a great policy to adhere to, a reversion in a 10.x patch release
wouldn't be particularly difficult should people choose to complain rather
than adapt.

​David J.

Re: [HACKERS] Login into PostgreSQL without password

[Christoph Berg]

Re: Murtuza Zabuawala 2016-05-26 

> Hi,
> 
> I have created a role using below sql, then I disconnected & try to login
> into postgres db with newly created user "test_role", It prompt for
> password and I pressed Enter key because I did not provided any password
> when I created role so it throw me an error as below *Error: fe_sendauth:
> no password supplied.*
> 
> Can someone please explain this behaviour of postgreSQL database, where I'm
> not allowed to login without password even if I do not have password set
> for "test_role" user?

"Without password" doesn't mean "can log in without a password", but
rather "doesn't have a valid password". You will have to configure
pg_hba.conf to let you in by other means. (peer, trust, or the
oh-so-deprecated "ident".)

> The work around is I had to manually edit pg_hba conf and change
> authentication method to trust for this user so that i can login without
> password.
> 
> And If that's how postgreSQL authentication works, then can we add a
> mechanism to disallow user to create role without password if running with
> md5 authentication mode?

No. There's legitimate uses for roles without passwords, e.g. roles
that act as user groups. (And there's no such thing as "running with
md5", as there's usually various authentication methods configured in
pg_hba.conf.)

Christoph


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

[HACKERS] Login into PostgreSQL without password

[Murtuza Zabuawala]

Hi,

I have created a role using below sql, then I disconnected & try to login
into postgres db with newly created user "test_role", It prompt for
password and I pressed Enter key because I did not provided any password
when I created role so it throw me an error as below *Error: fe_sendauth:
no password supplied.*

Can someone please explain this behaviour of postgreSQL database, where I'm
not allowed to login without password even if I do not have password set
for "test_role" user?

The work around is I had to manually edit pg_hba conf and change
authentication method to trust for this user so that i can login without
password.

And If that's how postgreSQL authentication works, then can we add a
mechanism to disallow user to create role without password if running with
md5 authentication mode?


*SQL Query used to create role:*

CREATE USER test_role WITH
LOGIN
SUPERUSER
CREATEDB
CREATEROLE
INHERIT
REPLICATION
CONNECTION LIMIT -1;


--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

[Nikolay Shaplov]

В письме от 25 мая 2016 14:03:17 Вы написали:

> > > > >This all should me moved behind "access method" abstraction...
> > > > 
> > > > +1 relopt_kind should be moved in am, at least. Or removed.
> > > 
> > > Hm, but we have tablespace options too, so I'm not sure that using AM as
> > > abstraction level is correct.
> > 
> > We will use am for all indexes, and keep all the rest in relopotion.c for
> > non-indexes. May be divided options catalog into sections one section for
> > each kind.
> That makes sense.  I can review the patch later.
That would be great! ;-)


> 
> > And as I also would like to use this code for dynamic attoptions later, I
> > would like to remove relopt_kind at all. Because it spoils live in that
> > case.
> As I remember, Fabrízio (in CC) had a patch for dynamic reloptions, but
> there was some problem with it and we dumped it; see
> https://www.postgresql.org/message-id/CAFcNs+rqCq1H5eXW-cvdti6T-xo2STEkhjREx
> =odmakk5ti...@mail.gmail.com for previous discussion.

I've read the start of that thread. In my opinion Fabrízio offering quite 
different thing. I am speaking about adding options to a new object (access 
method or later operator class). You add these options when you create this 
object and you have a monopoly of defining options for it.

Fabrízio offers adding options for relkind that already exists. This seems to 
be a complex thing. You should resolve conflicts between two extensions that 
want to define same option for example. Somehow deal with the situation when 
the extension is dropped. Should we remove reloptions created by that 
extension from pg_class then?

And moreover, as far as I understand reloptions is about how does relation 
operates. And the external extension would not affect this at all. So we are 
speaking not about options of a relation, but about extension that want to 
store some info about some relation. I think in general this extension should 
store it's info into it's own data structures. 

May be there is cases when you will really need such behavior. But it is quite 
different task, have almost nothing in common with things I am going to do :-)

-- 
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][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Nikolay Shaplov]

В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
> Nikolay Shaplov  writes:
> > If I read gram.y code for insert statement, I see that there is an
> > optional
> > USING keyword before opclass name
> > 
> > opt_class:  any_name{ $$ = $1; }
> > 
> > | USING any_name{ $$ = $2; }
> > | /*EMPTY*/ { $$ = NIL; }
> > 
> > ;
> > 
> > but it the documentation this keyword is omitted.
> 
> I think we should seriously consider fixing this code/docs discrepancy
> by making the code match the docs, not vice versa.  That is, let's just
> remove the USING alternative here entirely.
> 
> If we wanted to make the docs match the code, it would not only be
> CREATE INDEX that would have to be patched, because that's not the
> only place that index_elem can appear.  As far as I can find in a
> quick search, none of the relevant statements have ever documented
> that USING is allowed here; nor does it appear that any client-side
> code of ours makes use of the keyword.
> 
> Also, because USING is already used elsewhere in CREATE INDEX (to
> introduce the optional index AM name), I think that documenting its
> use in this clause would add confusion not subtract it.  References
> to "the USING clause in CREATE INDEX" would become ambiguous.
> 
> This wouldn't be something to back-patch, of course, but I think it's
> an entirely reasonable change to make in HEAD.
> 
> Comments?

I have two arguments for not removing USING there. 

1. Backward compatibility. Are you sure, that nobody ever wrote a code using 
this "USING" keyword? I would say it is unlikely, but will not be sure 100%. 
Dropping this backward compatibility (even with small chance that it was ever 
used) for no practical reason is not wise at all. If it might bring some pain 
to somebody, then why do it after all...

2. I think expression with USING in it is more human readable:

CREATE INDEX (xxx op_yyy);

is less sensible then 

CREATE INDEX (xxx USING op_yyy);

in my opinion. In second example person that does not know SQL at all, will 
understand that xxx is main object or action, and op_yyy is about how this xxx 
will be done or used.

If somebody would like to write more readable code, why we should forbid it to 
him?

2.1. As far as I can get the general idea of SQL, there is a tendency to put 
keywords (optional or not) between to object names. Like this

SELECT a _AS_ b from 

I like this tendency

2.2. I am not familiar with SQL ISO standart, and I suppose there is no USING 
at all in that case, but I think it would be good to look there to check for 
it or for something similar

2.3. And the last, when I found out about this keyword, I started to use it in 
my SQL statements that I use while development, and I just liked it. I will 
miss it if you remove it ;-) 


-- 
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] [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan)

[Amit Kapila]

On Thu, May 12, 2016 at 11:37 PM, Tom Lane  wrote:
>
> Robert Haas  writes:
> >> Target list for a relation, you mean?  See relation.h:
> >>
> >> *  reltarget - Default Path output tlist for this rel; normally
contains
> >> *  Var and PlaceHolderVar nodes for the values we need
to
> >> *  output from this relation.
> >> *  List is in no particular order, but all rels of an
> >> *  appendrel set must use corresponding orders.
> >> *  NOTE: in an appendrel child relation, may contain
> >> *  arbitrary expressions pulled up from a subquery!
>
> > Err, wow.  That makes my head hurt.  Can you explain why this case
> > only arises for appendrel children, and not for plain rels?
>
> Well, plain rels only output Vars ;-)
>
> But consider an appendrel representing
>
> (SELECT x+1 FROM t1 UNION ALL SELECT y+2 FROM t2) ss(a)
>
> The RTE for ss will have a reltarget list containing just "a".
> Once we pull up the subqueries, the reltarget lists for the two child
> appendrel members will need to contain "x+1" and "y+2" in order to be
> equivalent to the parent's reltarget list.  See set_append_rel_size(),
> which does that transformation.
>

I think this can also lead to an issue, as currently for child relations,
we just copy consider_parallel flag of parent.  Now, if the child rel
target list is adjusted such that it contains parallel restricted
expression, then we are bound to fail.  I think to handle append rel case
appropriately, we should compute the consider_parallel flag for each child
relation in set_append_rel_size() and then later when computing the flag
for parent rel, consider the flag for child rels (basically if any child
rel has consider_parallel as false, then set consider_parallel for parent
as false).  To achieve this, we need to call  set_rel_consider_parallel()
after calling set_rel_size().

Thoughts?

Just to summarize, apart from above issue, we have discussed two different
issues related to parallel query in this thread.
a. Push down of parallel restricted clauses to nodes below gather.  Patch
to fix same is posted upthread [1].
b. Wrong assumption that target list can only contain Vars.  Patch to fix
same is posted upthread [2].  Test which prove our assumption is wrong is
also posted upthread [3].

I will add this issue in list of open issues for 9.6 @
https://wiki.postgresql.org/wiki/PostgreSQL_9.6_Open_Items

[1] -
https://www.postgresql.org/message-id/CAA4eK1Ky2=HsTsT4hmfL=eal5rv0_t59tvwzvk9hqkvn6do...@mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CAA4eK1L-Uo=s4=0jvvva51pj06u5wddvsqg673yuxj_ja+x...@mail.gmail.com
[3] -
https://www.postgresql.org/message-id/CAFiTN-vzg5BkK6kAh3OMhvgRu-uJvkjz47ybtopMAfGJp=z...@mail.gmail.com


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

Re: [HACKERS] foreign table batch inserts

[Etsuro Fujita]

On 2016/05/18 7:08, Michael Paquier wrote:

On Wed, May 18, 2016 at 6:00 AM, Manuel Kniep  wrote:

I realized that inserts into foreign tables are only done row by row.
Consider copying data from one local table to a foreign table with

INSERT INTO foreign_table(a,b,c) SELECT a,b,c FROM local_table;

When the foreign  server is for example in another datacenter with long latency,
this as an enormous performance trade off.



I am adding Fujita-san in the loop here, he is
quite involved with postgres_fdw these days so perhaps he has some
input to offer.


Honestly, I didn't have any idea for executing such an insert 
efficiently, but I was thinking to execute an insert into a foreign 
table efficiently, by sending the whole insert to the remote server, if 
possible.  For example, if the insert is of the form:


INSERT INTO foreign_table(a,b,c) VALUES (1, 2, 3), (4, 5, 6) or
INSERT INTO foreign_table(a,b,c) SELECT a,b,c FROM foreign_table2

where foreign_table and foreign_table2 belong to the same foreign 
server, then we could send the whole insert to the remote server.


Wouldn't that make sense?

Best regards,
Etsuro Fujita




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

[HACKERS] Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618

[Amit Kapila]

On Wed, May 25, 2016 at 9:44 PM, Michael Paquier 
wrote:
>
> On Wed, May 25, 2016 at 12:11 AM, Amit Kapila 
wrote:
> >
> > Okay, attached patch just does that and I have verified that it allows
to
> > start multiple services in windows.  In off list discussion with
Robert, he
> > suggested not to complicate the patch by retrying for fixed number of
times
> > as there is no proof that ERROR_ACCESS_DENIED can occur due to any other
> > reason in this code path.  This patch is based on Kyotaro san's patch
posted
> > upthread with just minor changes in comments and indentation.
>
> Thanks for catching Robert and getting confirmation on the matter. In
> the same line of thoughts, I think as well that it is definitely not
> worth complicating the retry logic in dsm.c, but you knew that already
> per last week's discussion.
>
> Regarding the patch, this looks good to me.
>

Thanks for reviewing the patch.  I have added the entry for this patch in
next CF (https://commitfest.postgresql.org/10/636/),  feel free to mark it
as Ready for committer if you think patch is good.


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

Re: [HACKERS] [sqlsmith] Failed assertions on parallel worker shutdown

[Amit Kapila]

On Tue, May 24, 2016 at 6:36 PM, Andreas Seltenreich 
wrote:
>
>
> Each of the sent plans was collected when a worker dumped core due to
> the failed assertion.  More core dumps than plans were actually
> observed, since with this failed assertion, multiple workers usually
> trip on and dump core simultaneously.
>
> The following query corresponds to plan2:
>
> --8<---cut here---start->8---
> select
>   pg_catalog.pg_stat_get_bgwriter_requested_checkpoints() as c0,
>   subq_0.c3 as c1, subq_0.c1 as c2, 31 as c3, 18 as c4,
>   (select unique1 from public.bprime limit 1 offset 9) as c5,
>   subq_0.c2 as c6
> from
> (select ref_0.tablename as c0, ref_0.inherited as c1,
> ref_0.histogram_bounds as c2, 100 as c3
>   from
> pg_catalog.pg_stats as ref_0
>   where 49 is not NULL limit 55) as subq_0
> where true
> limit 58;
> --8<---cut here---end--->8---
>
>
I am able to reproduce the assertion (it occurs once in two to three times,
but always at same place) you have reported upthread with the above query.
It seems to me, issue here is that while workers are writing tuples in the
tuple queue, the master backend has detached from the queues.  The reason
why master backend has detached from tuple queues is because of Limit
clause, basically after processing required tuples as specified by Limit
clause, it calls shutdown of nodes in below part of code:

ExecutePlan()
{
..
if (TupIsNull(slot))
{
/* Allow nodes to release or shut down resources. */
(void) ExecShutdownNode(planstate);
break;
}
..
}

Now, it is quite possible that the worker has written part of it's data,
after which the queue got detached.  The callers of shm_mq
(tqueueReceiveSlot/shm_mq_send) doesn't handle SHM_MQ_DETACHED due to which
it keeps on sending more data (next tuple) which leads to the assertion in
below code:
shm_mq_sendv()
{
..
/* Write the actual data bytes into the buffer. */
Assert(mqh->mqh_partial_bytes <= nbytes);
..
}

I think the workers should stop processing tuples after the tuple queues
got detached.  This will not only handle above situation gracefully, but
will allow to speed up the queries where Limit clause is present on top of
Gather node.  Patch for the same is attached with mail (this was part of
original parallel seq scan patch, but was not applied and the reason as far
as I remember was we thought such an optimization might not be required for
initial version).

Another approach to fix this issue could be to reset mqh_partial_bytes and
mqh_length_word_complete in shm_mq_sendv  in case of SHM_MQ_DETACHED.
These are currently reset only incase of success.

I have added this issue in list of PostgreSQL 9.6 open items @
https://wiki.postgresql.org/wiki/PostgreSQL_9.6_Open_Items

The steps to reproduce it on regression database is:
1. We need to create enough rows in pg_statistic, so that parallel plan can
be selected.
2. Used below procedure to ensure sufficient rows gets created in
pg_statistic
CREATE OR REPLACE FUNCTION populate_pg_stat() RETURNS int AS
$BODY$
DECLARE
count int;
BEGIN
FOR count IN 1 .. 5
LOOP
Execute 'Create table t' || count || '(c1 int)';
Execute 'insert into t' || count || ' values (generate_series(1,10))';
Execute 'Analyze t' || count;
End Loop;
Return 1;
END
$BODY$
LANGUAGE plpgsql;
3.
set parallel_tuple_cost = 0
set parallel_setup_cost = 0
execute query -
Explain Analyze select
  pg_catalog.pg_stat_get_bgwriter_requested_checkpoints() as c0,
  subq_0.c3 as c1, subq_0.c1 as c2, 31 as c3, 18 as c4,
  (select unique1 from public.bprime limit 1 offset 9) as c5,
  subq_0.c2 as c6
from
(select ref_0.tablename as c0, ref_0.inherited as c1,
ref_0.histogram_bounds as c2, 100 as c3
  from
pg_catalog.pg_stats as ref_0
  where 49 is not NULL limit 55) as subq_0
where true
limit 58;


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


stop_processing_tuples_detached_queue_v1.patch
Description: Binary data

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

[HACKERS] Fix a failure of pg_dump with !HAVE_LIBZ

[Kyotaro HORIGUCHI]

Hello.

I got the following messages during investigating some other bug,
from pg_dump compiled with --without-zlib.

> $ rm -rf testdump; pg_dump "postgres://horiguti:hoge@localhost/postgres" 
> --jobs=9 -Fd -f testdump; echo $?
> pg_dump: [archiver] WARNING: requested compression not available in this 
> installation -- archive will be uncompressed
> pg_dump: [parallel archiver] not built with zlib support
> pg_dump: [archiver (db)] query failed: ERROR:  could not import the requested 
> snapshot
> DETAIL:  The source transaction 10116 is not running anymore.
> pg_dump: [archiver (db)] query failed: ERROR:  invalid snapshot identifier: 
> "2784-1"
> pg_dump: [archiver (db)] query failed: ERROR:  invalid snapshot identifier: 
> "2784-1"
> 1

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(). (The following errors mentioning
snapshot id are the consequences of unexpected sudden death of
the first worker from this bug, which causes expiration of the
snapshot.)

It works correctly with the patch attached, both on Linux and on
Windows.

> $ rm -rf testdump; pg_dump "postgres://horiguti:hoge@localhost/postgres" 
> --jobs=9 -Fd -f testdump; echo $?
> pg_dump: WARNING: requested compression (-1) not available in this 
> installation -- archive will be uncompressed
> 0

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

>From ceb471eea3e688be51e7f167ff7b223072790050 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 26 May 2016 18:28:14 +0900
Subject: [PATCH] Fix compressLevel correction for parallel mode

Being compiled with --without-zlib, pg_dump unexpectedly fails to
create an archive in parallel mode. This is because compressLevel is
checked too late to be propagated to child workers. This patch moves
it to earlier enough.
---
 src/bin/pg_dump/pg_backup_archiver.c | 8 
 src/bin/pg_dump/pg_dump.c| 8 
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 9390a6b..5026027 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3476,14 +3476,6 @@ WriteHead(ArchiveHandle *AH)
 	(*AH->WriteBytePtr) (AH, AH->offSize);
 	(*AH->WriteBytePtr) (AH, AH->format);
 
-#ifndef HAVE_LIBZ
-	if (AH->compression != 0)
-		write_msg(modulename, "WARNING: requested compression not available in this "
-  "installation -- archive will be uncompressed\n");
-
-	AH->compression = 0;
-#endif
-
 	WriteInt(AH, AH->compression);
 
 	crtm = *localtime(>createDate);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1267afb..45a62bd 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -594,6 +594,14 @@ main(int argc, char **argv)
 		else
 			compressLevel = 0;
 	}
+#ifndef HAVE_LIBZ
+	if (compressLevel != 0)
+		write_msg(NULL, "WARNING: requested compression (%d) not "
+  "available in this installation -- "
+  "archive will be uncompressed\n",
+			compressLevel);
+	compressLevel = 0;
+#endif
 
 	/*
 	 * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
-- 
1.8.3.1


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

Re: [HACKERS] pg_dump -j against standbys

[Magnus Hagander]

On Wed, May 25, 2016 at 4:21 PM, Tom Lane  wrote:

> Magnus Hagander  writes:
> >> Also, why didn't you keep using ExecuteSqlQueryForSingleRow()?
>
> > The reason I did that is that ExecuteSqlQueryForSingleRow() is a static
> > method in pg_dump.c. I was planning to go back and review that, and
> > consider moving it, but I forgot it :S
>
> > I think the clean thing is probably to use that one, and also move it
> over
> > to not be a static method in pg_dump.c, but instead sit next to
> > ExecuteSqlQuery in pg_backup_db.c. Do you agree that's reasonable, and
> > something that's OK to backpatch?
>
> No objection here, since it wouldn't be exposed outside pg_dump in any
> case.
>

Here's an updated patch based on this,and the other feedback.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 83f6029..f94caa3 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -173,6 +173,7 @@ typedef struct Archive
 	int			verbose;
 	char	   *remoteVersionStr;		/* server's version string */
 	int			remoteVersion;	/* same in numeric form */
+	bool		isStandby;		/* is server a standby node */
 
 	int			minRemoteVersion;		/* allowable range */
 	int			maxRemoteVersion;
diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
index 35ce945..818bc9e 100644
--- a/src/bin/pg_dump/pg_backup_db.c
+++ b/src/bin/pg_dump/pg_backup_db.c
@@ -37,6 +37,7 @@ _check_database_version(ArchiveHandle *AH)
 {
 	const char *remoteversion_str;
 	int			remoteversion;
+	PGresult   *res;
 
 	remoteversion_str = PQparameterStatus(AH->connection, "server_version");
 	remoteversion = PQserverVersion(AH->connection);
@@ -56,6 +57,20 @@ _check_database_version(ArchiveHandle *AH)
   remoteversion_str, progname, PG_VERSION);
 		exit_horribly(NULL, "aborting because of server version mismatch\n");
 	}
+
+	/*
+	 * When running against 9.0 or later, check if we are in recovery mode,
+	 * which means we are on a hot standby.
+	 */
+	if (remoteversion >= 9)
+	{
+		res = ExecuteSqlQueryForSingleRow((Archive *) AH, "SELECT pg_catalog.pg_is_in_recovery()");
+
+		AH->public.isStandby = (strcmp(PQgetvalue(res, 0, 0), "t") == 0);
+		PQclear(res);
+	}
+	else
+		AH->public.isStandby = false;
 }
 
 /*
@@ -389,6 +404,29 @@ ExecuteSqlQuery(Archive *AHX, const char *query, ExecStatusType status)
 }
 
 /*
+ * Execute an SQL query and verify that we got exactly one row back.
+ */
+PGresult *
+ExecuteSqlQueryForSingleRow(Archive *fout, char *query)
+{
+	PGresult   *res;
+	int			ntups;
+
+	res = ExecuteSqlQuery(fout, query, PGRES_TUPLES_OK);
+
+	/* Expecting a single result only */
+	ntups = PQntuples(res);
+	if (ntups != 1)
+		exit_horribly(NULL,
+	  ngettext("query returned %d row instead of one: %s\n",
+			   "query returned %d rows instead of one: %s\n",
+			   ntups),
+	  ntups, query);
+
+	return res;
+}
+
+/*
  * Convenience function to send a query.
  * Monitors result to detect COPY statements
  */
diff --git a/src/bin/pg_dump/pg_backup_db.h b/src/bin/pg_dump/pg_backup_db.h
index 6408f14..527449e 100644
--- a/src/bin/pg_dump/pg_backup_db.h
+++ b/src/bin/pg_dump/pg_backup_db.h
@@ -16,6 +16,7 @@ extern int	ExecuteSqlCommandBuf(Archive *AHX, const char *buf, size_t bufLen);
 extern void ExecuteSqlStatement(Archive *AHX, const char *query);
 extern PGresult *ExecuteSqlQuery(Archive *AHX, const char *query,
 ExecStatusType status);
+extern PGresult *ExecuteSqlQueryForSingleRow(Archive *fout, char *query);
 
 extern void EndDBCopyMode(Archive *AHX, const char *tocEntryTag);
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1267afb..a6550ed 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -264,7 +264,6 @@ static bool nonemptyReloptions(const char *reloptions);
 static void appendReloptionsArrayAH(PQExpBuffer buffer, const char *reloptions,
 		const char *prefix, Archive *fout);
 static char *get_synchronized_snapshot(Archive *fout);
-static PGresult *ExecuteSqlQueryForSingleRow(Archive *fout, char *query);
 static void setupDumpWorker(Archive *AHX);
 
 
@@ -648,23 +647,11 @@ main(int argc, char **argv)
 		dopt.no_security_labels = 1;
 
 	/*
-	 * When running against 9.0 or later, check if we are in recovery mode,
-	 * which means we are on a hot standby.
+	 * On hot standby slaves, never try to dump unlogged table data, since it
+	 * will just throw an error.
 	 */
-	if (fout->remoteVersion >= 9)
-	{
-		PGresult   *res = ExecuteSqlQueryForSingleRow(fout, "SELECT pg_catalog.pg_is_in_recovery()");
-
-		if (strcmp(PQgetvalue(res, 0, 0), "t") == 0)
-		{
-			/*
-			 * On hot standby slaves, never try to dump unlogged table data,
-			 * since it will just throw an error.
-			 */
-			dopt.no_unlogged_table_data = true;
-		}
-		PQclear(res);
-	}
+	if (fout->isStandby)
+		dopt.no_unlogged_table_data = 

Re: [HACKERS] RLS related docs

[Dean Rasheed]

On 25 May 2016 at 02:04, Joe Conway  wrote:
> Please see attached two proposed patches for the docs related to RLS:
>
> 1) Correction to pg_restore
> 2) Additional mentions that "COPY FROM" does not allow RLS to be enabled
>
> Comments?
>

The pg_restore change looks good -- that was clearly wrong.

Also, +1 for the new note in pg_dump.

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.


> Related question: I believe
>
>   COPY tbl TO ...
>
> is internally converted to
>
>   COPY (select * FROM tbl) TO ...
>
> when RLS is involved. Do we want to document that?
>

I think so, yes, because that makes it clearer what policies will be applied.

Regards,
Dean


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

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

[Etsuro Fujita]

On 2016/05/17 0:25, Robert Haas wrote:

On Wed, May 11, 2016 at 3:20 AM, Etsuro Fujita
 wrote:

Thanks for the review!

I'll add this to the next CF.  I think this should be addressed in advance
of the release of 9.6, though.



I agree.  Committed.


Thanks!

Best regards,
Etsuro Fujita




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

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

[Kyotaro HORIGUCHI]

> Sounds reasonable. I look into this further.

I looked into that and found one problem in the patch.

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

This occurs in the following steps.

1. One of the workers dies from some reason.
   (InitCompressorZlib immediately goes into exit_horribly in this case)

2. The main thread detects in ListenToWorkers that the worker is dead.

3. ListenToWorkers calls exit_horribly then exit_nicely

4. exit_nicely calls archive_close_connection as a callback then
   the callback calls ShutdownWorkersHard

5. ShutdownWorkersHard should close the write side of the pipe
   but the patch skips it for WIN32.

So, the attached patch on top the patch fixes that, that is,
pg_dump returns to command prompt even for the case.

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. Some of the workers succesfully did
before the connection closing and remained listening to their
master to inhibit termination of the entire process.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index f650d3f..6c08426 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -308,7 +308,6 @@ checkAborting(ArchiveHandle *AH)
 static void
 ShutdownWorkersHard(ParallelState *pstate)
 {
-#ifndef WIN32
 	int			i;
 
 	/*
@@ -318,6 +317,7 @@ ShutdownWorkersHard(ParallelState *pstate)
 	for (i = 0; i < pstate->numWorkers; i++)
 		closesocket(pstate->parallelSlot[i].pipeWrite);
 
+#ifndef WIN32
 	for (i = 0; i < pstate->numWorkers; i++)
 		kill(pstate->parallelSlot[i].pid, SIGTERM);
 #else

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