Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

2017-03-18 Thread Rafia Sabih
On Sat, Mar 18, 2017 at 5:40 AM, Robert Haas  wrote:
> On Fri, Mar 17, 2017 at 9:15 AM, Ashutosh Bapat
>  wrote:
>> This set of patches fixes both of those things.
>
> 0001 changes the purpose of a function and then 0007 renames it.  It
> would be better to include the renaming in 0001 so that you're not
> taking multiple whacks at the same function in the same patch series.
> I believe it would also be best to include 0011's changes to
> adjust_appendrel_attrs_multilevel in 0001.
>
> 0002 should either add find_param_path_info() to the relevant header
> file as extern from the beginning, or it should declare and define it
> as static and then 0007 can remove those markings.  It makes no sense
> to declare it as extern but put the prototype in the .c file.
>
> 0004 still needs to be pared down.  If you want to get something
> committed this release cycle, you have to get these details taken care
> of, uh, more or less immediately.  Actually, preferably, several weeks
> ago.  You're welcome to maintain your own test suite locally but what
> you submit should be what you are proposing for commit -- or if not,
> then you should separate the part proposed for commit and the part
> included for dev testing into two different patches.
>
> In 0005's README, the part about planning partition-wise joins in two
> phases needs to be removed.  This patch also contains a small change
> to partition_join.sql that belongs in 0004.
>
> 0008 removes direct tests against RELOPT_JOINREL almost everywhere,
> but it overlooks the new ones added to postgres_fdw.c by
> b30fb56b07a885f3476fe05920249f4832ca8da5.  It should be updated to
> cover those as well, I suspect.  The commit message claims that it
> will "Similarly replace RELOPT_OTHER_MEMBER_REL test with
> IS_OTHER_REL() where we want to test for child relations of all kinds,
> but in fact it makes exactly zero such substitutions.
>
> While I was studying what you did with reparameterize_path_by_child(),
> I started to wonder whether reparameterize_path() doesn't need to
> start handling join paths.  I think it only handles scan paths right
> now because that's the only thing that can appear under an appendrel
> created by inheritance expansion, but you're changing that.  Maybe
> it's not critical -- I think the worst consequences of missing some
> handling there is that we won't consider a parameterized path in some
> case where it would be advantageous to do so.  Still, you might want
> to investigate a bit.
>
I was trying to play around with this patch and came across following
case when without the patch query completes in 9 secs and with it in
15 secs. Theoretically, I tried to capture the case when each
partition is having good amount of rows in output and each has to
build their own hash, in that case the cost of building so many hashes
comes to be more costly than having an append and then join. Thought
it might be helpful to consider this case in better designing of the
algorithm. Please feel free to point out if I missed something.

Test details:
commit: b4ff8609dbad541d287b332846442b076a25a6df
Please find the attached .sql file for the complete schema and data
and .out file for the result of explain analyse with and without
patch.

-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


pwj_regress_test.out
Description: Binary data


test_case_pwj.sql
Description: Binary data

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


Re: [HACKERS] PinBuffer() no longer makes use of strategy

2017-03-18 Thread Jim Nasby

On 3/16/17 12:48 PM, David Steele wrote:

This patch looks pretty straight forward and applies cleanly and
compiles at cccbdde.

It's not a straight revert, though, so still seems to need review.

Jim, do you know when you'll have a chance to look at that?


Yes. Compiles and passes for me as well.

One minor point: previously the code did

  if (buf->usage_count < BM_MAX_USAGE_COUNT)

but now it does

  if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)

being prone to paranoia, I prefer the first, but I've seen both styles 
in the code so I don't know if it's worth futzing with.


Marked as RFC.
--
Jim Nasby, Chief Data Architect, Austin TX
OpenSCG http://OpenSCG.com


--
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: Improve OR conditions on joined columns (common star schema problem)

2017-03-18 Thread Jim Nasby

On 3/16/17 11:54 AM, David Steele wrote:

On 2/14/17 4:03 PM, Tom Lane wrote:

Jim Nasby  writes:

On 2/14/17 1:18 PM, Tom Lane wrote:

One point that could use further review is whether the de-duplication
algorithm is actually correct.  I'm only about 95% convinced by the
argument I wrote in planunionor.c's header comment.



I'll put some thought into it and see if I can find any holes. Are you
only worried about the removal of "useless" rels or is there more?


Well, the key point is whether it's really OK to de-dup on the basis
of only the CTIDs that are not eliminated in any UNION arm.  I was
feeling fairly good about that until I thought of the full-join-to-
left-join-to-no-join conversion issue mentioned in the comment.
Now I'm wondering if there are other holes; or maybe I'm wrong about
that one and it's not necessary to be afraid of full joins.


This patch applies cleanly (with offsets) and compiles at cccbdde.

Jim, have you had time to think about this?  Any insights?


Having thought about it, I share Tom's concerns. And now I'm worried 
about what happens if there are multiple separate OR clauses. I guess 
those would be handled by separate UNIONs?


I'm also finding it a bit hard to follow the comment Tom mentioned. I'm 
pretty sure I understand what's going on until this part:



The identical proof can be expected to apply
+  * in other arms, except in an arm that references that rel in its version
+  * of the OR clause.  But in such an arm, we have effectively added a
+  * restriction clause to what is known in other arms, which means that the
+  * set of rows output by that rel can't increase compared to other arms.


AIUI, this is describing a case something like this:

SELECT child.blah FROM child LEFT JOIN parent USING(parent_id)
  WHERE child.foo AND (child.baz=1 or child.baz=2)

given that parent.parent_id is unique. Except for these concerns, there 
would need to be a complex OR somewhere in here that sometimes 
referenced parent and sometimes didn't, such as


  WHERE child.foo AND (child.baz=1 OR parent.foo=3)

But I'm not following the logic here (very possibly because I'm wrong 
about what I said above):



+  * Therefore the situation in such an arm must be that including the rel
+  * could result in either zero or one output row, rather than exactly one
+  * output row as in other arms.  So we still don't need to consider it for
+  * de-duplication.


I'm definitely not certain about the rest of it.

Tom, could you expand the description some, especially with some examples?
--
Jim Nasby, Chief Data Architect, Austin TX
OpenSCG http://OpenSCG.com


--
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] Composite IS NULL again, this time with plpgsql

2017-03-18 Thread Tom Lane
Andrew Gierth  writes:
> This obviously happens because plpgsql is storing the variable as an
> expanded list of fields rather than as a single composite datum, and
> rebuilding the datum value when it needs to be passed to SQL for
> evaluation.  Without an "isnull" flag for each composite subvalue, this
> can't regenerate the original datum closely enough to give the same
> value on an isnull test.

> What to do about this?

Sooner or later, I think we're going to have to bite the bullet and
switch over to using regular composite datums for plpgsql composite
variables --- that is, use the REC not ROW code paths for that case.

The ROW code paths should only get used for cases like "SELECT INTO a,b,c"
--- where there's no way to name the row-as-a-whole so no question
arises of whether it's null or not.

People have pushed back on that idea for various reasons, but we're
never going to have consistent behavior at this level of detail
until we do 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


[HACKERS] Composite IS NULL again, this time with plpgsql

2017-03-18 Thread Andrew Gierth
This came up recently on irc:

create type t1 as (a integer, b integer);
create type t2 as (p t1, q t1);
create function null_t2() returns t2 language sql
  as $f$ select null::t2; $f$;

Now consider the following plpgsql:

declare
  v t2;
begin
  v := null_t2();
  raise info 'v is null = %', v is null;  -- shows 'f'
end;

The value of the plpgsql variable v always behaves as if it had been
assigned row(row(null,null),row(null,null)) -- which, as we thrashed out
in detail sometime last year, isn't the same as row(null,null) or just
null and isn't considered null by IS NULL.

So there's no non-ugly way to preserve the nullity or otherwise of the
function result. The best I could come up with in answer to the original
problem (how to detect in plpgsql whether a composite value returned by
a function was actually null) was this:

declare
  v t2;
  v_null boolean;
begin
  select x into v from null_t2() as x where not (x is null);
  v_null := not found;
  raise info 'v is null = %', v_null;
end;

which lacks a certain obviousness (and you have to remember to use not
(x is null) rather than x is not null).

This obviously happens because plpgsql is storing the variable as an
expanded list of fields rather than as a single composite datum, and
rebuilding the datum value when it needs to be passed to SQL for
evaluation.  Without an "isnull" flag for each composite subvalue, this
can't regenerate the original datum closely enough to give the same
value on an isnull test.

What to do about this?

-- 
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] [PATCH] SortSupport for macaddr type

2017-03-18 Thread Peter Geoghegan
On Sat, Mar 18, 2017 at 2:54 PM, Peter Geoghegan  wrote:
> This seems fine to me, especially
> because it lets us compare macaddr using simple 3-way unsigned int
> comparisons, which isn't otherwise the case.

Out of idle curiosity, I decided to generate disassembly of both
macaddr_cmp_internal(), and the patch's abbreviated comparator. The
former consists of 49 x86-64 instructions at -02 on my machine,
totaling 135 bytes of object code. The latter consists of only 10
instructions, or 24 bytes of object code.

-- 
Peter Geoghegan


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


Re: [HACKERS] mat views stats

2017-03-18 Thread Tom Lane
Jim Mlodgenski  writes:
> After digging into things further, just making refresh report the stats
> for what is it basically doing simplifies and solves it and it is
> something we can back patch if that the consensus. See the attached
> patch.

I've pushed this into HEAD with one non-cosmetic change: the patch tried
to pass a uint64 tuple count to pgstat_count_heap_insert(), whose argument
was only declared as "int".  This would go seriously wrong with matviews
having more than INT_MAX rows, which hardly seems out of the question,
so I changed pgstat_count_heap_insert() to take int64 instead.

I don't think we can make that change in the back branches though.
It seems too likely that third-party code might be calling 
pgstat_count_heap_insert().

We could possibly kluge around this to produce a safe-to-back-patch
fix by doing something like

pgstat_count_heap_insert(matviewRel, (int) Min(processed, INT_MAX));

But that seems pretty ugly.  Given the lack of previous reports, I'm
personally content to leave this unfixed in the back branches.

Comments?

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] SortSupport for macaddr type

2017-03-18 Thread Peter Geoghegan
On Tue, Mar 14, 2017 at 3:25 AM, Heikki Linnakangas  wrote:
> Looks straightforward at a quick read-through. I have just a couple of
> questions. How much of the performance gain comes from avoiding the
> FunctionCallInvoke overhead, by simply having SortSupport with a comparison
> function, and how much comes from the "abbreviation"?

I assume it's very similar to the existing performance characteristics
that UUID SortSupport has.

> Also, is it worth the cycles and code to have the hyperloglog estimator, and
> aborting the abbreviation if there are only a few distinct values. Or put
> another way, how much does the abbreviation slow things down in the worst
> case?

Postgres doesn't support passing 6 byte types by-value due to a lack
of support from macros in places like postgres.h. There is a
GET_4_BYTES(), and a GET_8_BYTES(), but no GET_6_BYTES(). It doesn't
really seem worth adding those, just like it didn't seem worth it back
when I pointed out that a TID sort is very slow in the context of
CREATE INDEX CONCURRENTLY. (We ended up coming up with an ad-hoc int8
encoding scheme there, which allowed us to use the int8 default
opclass rather than the TID default opclass, you'll recall.)

It's a bit odd that macaddr abbreviated keys are actually always
smaller (not including padding) than sizeof(Datum) on 64-bit machines,
but this guarantees that abbreviated comparisons will reliably
indicate what an authoritative comparison would indicate, since of
course all relevant information is right there in the abbreviated key.
This will be the only time where that's generally true with
abbreviated keys for any type. This seems fine to me, especially
because it lets us compare macaddr using simple 3-way unsigned int
comparisons, which isn't otherwise the case.

I doubt that you'll ever see a real need to abort abbreviation.
Arguably it shouldn't be considered, but maybe it's worth being
consistent. It might matter on 32-bit platforms, where there is only a
single NIC-specific byte to differentiate MAC addresses, but even that
seems like a stretch. With a type like macaddr, where there is no
major cost like strxfrm() to worry about when abbreviating, the only
cost of abbreviating is that tie-breakers will have an extra
index_getattr() or similar. If there was no abbreviation, or if it
aborted, then the index_getattr() can happen once per SortTuple,
up-front.

Nitpick: the patch should probably not refer to 32-bit or 64-bit
systems. Rather, it should refer to Datum size only.

-- 
Peter Geoghegan


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


Re: [HACKERS] free space map and visibility map

2017-03-18 Thread Jeff Janes
On Sat, Mar 18, 2017 at 2:09 PM, Masahiko Sawada 
wrote:

> On Fri, Mar 17, 2017 at 9:37 PM, Jeff Janes  wrote:
> > With some intensive crash-recovery testing, I've run into a situation
> where
> > I get some bad table bloat.  There will be large swaths of the table
> which
> > are empty (all results from heap_page_items other than lp are either
> zero or
> > NULL), but have zero available space in the fsm, and are marked as
> > all-visible and all-frozen in the vm.
> >
> > I guess it is a result of a crash causing updates to the fsm to be lost.
> > Then due to the (crash-recovered) visibility map showing them as all
> visible
> > and all frozen, vacuum never touches the pages again, so the fsm never
> gets
> > corrected.
>
> I guess that this happens only if heap_xlog_clean applies FPI. Right?
> Updating fsm can be lost but fsm is updated by replaying HEAP2_CLEAN
> record during crash recovery.
>

Isn't HEAP2_CLEAN only issued before an intended HOT update?  (Which then
can't leave the block as all visible or all frozen).  I think the issue is
here is HEAP2_VISIBLE or HEAP2_FREEZE_PAGE.  Am I reading this correctly,
that neither of those ever update the FSM, regardless of FPI?

I don't know how to test the issue of which record is most responsible.  I
could turn off FPW globally and see what happens, with some tweaking to my
testing harness.

Cheers,

Jeff


Re: [HACKERS] free space map and visibility map

2017-03-18 Thread Masahiko Sawada
On Fri, Mar 17, 2017 at 9:37 PM, Jeff Janes  wrote:
> With some intensive crash-recovery testing, I've run into a situation where
> I get some bad table bloat.  There will be large swaths of the table which
> are empty (all results from heap_page_items other than lp are either zero or
> NULL), but have zero available space in the fsm, and are marked as
> all-visible and all-frozen in the vm.
>
> I guess it is a result of a crash causing updates to the fsm to be lost.
> Then due to the (crash-recovered) visibility map showing them as all visible
> and all frozen, vacuum never touches the pages again, so the fsm never gets
> corrected.

I guess that this happens only if heap_xlog_clean applies FPI. Right?
Updating fsm can be lost but fsm is updated by replaying HEAP2_CLEAN
record during crash recovery.

>
> 'VACUUM (DISABLE_PAGE_SKIPPING) foo;'   does fix it, but that seems to be
> the only thing that will.

If the above is correct, another one option is to allow
heap_xlog_clean to update fsm even when appling FPI.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
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] Removing binaries (was: createlang/droplang deprecated)

2017-03-18 Thread Tom Lane
Magnus Hagander  writes:
> createuser, dropuser - definitely pollutes the namespace, people do
> sometimes try them for the wrong thing. Unlike the db ones they do add
> value though -- I don't think we have a psql way of in a single command
> doing what --pwprompt on createuser does, do we? But given that we are in
> the process of breaking a lot of other scripts for 10, perhaps we should
> rename it to pg_createuser?

I'm not particularly on board with arguments like "we already broke a ton
of stuff so let's break some more".  Eventually you've managed to create
a daunting barrier to upgrading at all.

I think a more reasonable way to proceed is to install symlinks
pg_createuser -> createuser (or the other direction), mark the older names
as deprecated, and announce that we'll remove the old names a few releases
from now.  That gives people time to adjust.

Maybe we should handle createdb likewise, rather than just kicking it to
the curb.  I know I use it quite often; it's less typing than psql -c
'create database ...' postgres, and still would be with a pg_ prefix.

regards, tom lane


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


Re: [HACKERS] Allow interrupts on waiting standby

2017-03-18 Thread Tom Lane
Michael Paquier  writes:
> Both things are fixed in the new version attached. I have added as
> well this patch to the next commit fest:
> https://commitfest.postgresql.org/13/977/

Couple of thoughts on this patch ---

1. Shouldn't WaitExceedsMaxStandbyDelay's CHECK_FOR_INTERRUPTS be moved to
after the WaitLatch call?  Not much point in being woken immediately by
an interrupt if you're not going to respond.

2. Is it OK to ResetLatch here?  If the only possible latch event in this
process is interrupt requests, then I think WaitLatch, then ResetLatch,
then CHECK_FOR_INTERRUPTS is OK; but otherwise it seems like you risk
discarding events that need to be serviced later.

3. In the same vein, if we're going to check WL_POSTMASTER_DEATH, should
there be a test for that and immediate exit(1) here?

4. I'd be inclined to increase the sleep interval only if we did time out,
not if we were awakened by some other event.

5. The comment about maximum sleep length needs some work.  At first
glance you might think that without the motivation of preventing long
uninterruptible sleeps, we might as well allow the sleep length to grow
well past 1s.  I think that'd be bad, because we want to wake up
reasonably soon after the xact(s) we're waiting for commit.  But neither
the original text nor the proposed replacement mention this.

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] Removing binaries (was: createlang/droplang deprecated)

2017-03-18 Thread Magnus Hagander
Magnus Hagander  writes:
>> 2017-03-18 14:00 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:
>>> I just noticed that createlang and droplang have been listed as
>>> deprecated since PG 9.1.
>>> Do we dare remove them?

> (I'd extend it to all the non-prefixed pg binaries, but let's open that
can
> of worms right now, one thing at a time)

> To my mind, these two and createuser/dropuser are the only really serious
> namespacing problems among our standard binaries.  The ones with names
> ending in "db" don't seem likely to cause huge confusion.  I suppose that
> if we were naming it today, "psql" wouldn't get that name; but the chances
> of renaming that one are certainly zero, namespace conflict or no.

> But createuser/dropuser are a real problem, because they certainly could
> be mistaken for system-level utilities.

Yeah, I've seen people use those and get quite confused. Luckily they don't
tend to *break* things, but it's definitely namespace pollution.

If we look through the binaries we have now that are not prefixed with pg_:

clusterdb, vacuumdb, reindexdb - while not clear that they are about
*postgres*, it's pretty clear they're about a database. I wouldn't name
them without pg_ today, but I doubt it's worth chaning. They also add
actual value (being able to process multiple things)

createdb, dropdb - also not clear they're about postgres, more likely to be
used by mistake but not that bad. That said, do they add any *value* beyond
what you can do with psql -c "CREATE DATABASE"? I don't really see one, so
I'd suggest dropping these too.

createuser, dropuser - definitely pollutes the namespace, people do
sometimes try them for the wrong thing. Unlike the db ones they do add
value though -- I don't think we have a psql way of in a single command
doing what --pwprompt on createuser does, do we? But given that we are in
the process of breaking a lot of other scripts for 10, perhaps we should
rename it to pg_createuser?

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


Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki

2017-03-18 Thread Jan Michálek
2017-03-12 19:21 GMT+01:00 Jan Michálek :

>
>
> 2017-03-10 9:43 GMT+01:00 Jan Michálek :
>
>>
>>
>> 2017-03-09 20:10 GMT+01:00 Peter Eisentraut <
>> peter.eisentr...@2ndquadrant.com>:
>>
>>> This is looking pretty neat.  I played around with it a bit.  There are
>>> a couple of edge cases that you need to address, I think.
>>>
>>
>> Thanks, original code is very synoptical and and well prepared for adding
>> new formats.
>>
>>
>>>
>>> - Does not support \x
>>>
>>
>> I know, i dnot`t know, if \x make sense in this case. I will look, how it
>> is done in other formats like html. I think, that it should work in sense,
>> that table generated to rst should give similar output after processing
>> like output of html format.
>>
>>
> I prepared something like this (i have no prepared diff, i need do some
> another changes)
> There a few things I need to do. First problem is bold column names, i
> should do it in sme fashin as "RECORD", but i need to do some research
> about length of column.
> Bigger problem is with tab indent, rst processor doesn`t work with this in
> this case.
>

In new diff is added \x for rst and skipping leading spaces in rst in both.
make check passed

Jan


>
> jelen=# execute q \g | xclip
> +-+-
> ---+
> | **RECORD 1**
> |
> +-+-
> ---+
> | column1 | Elephant, kangaroo,
> |
> | | squirrel, gorilla
> |
> +-+-
> ---+
> | column2 | 121
> |
> +-+-
> ---+
> | column3 | 1.0035971223021583
> |
> +-+-
> ---+
> | column4 | 0.
> |
> +-+-
> ---+
> | column5 | Hello Hello Hello Hello Hello Hello Hello Hello Hello
> Hello|
> +-+-
> ---+
> | **RECORD 2**
> |
> +-+-
> ---+
> | column1 | goat, rhinoceros,
>|
> | | monkey, ape
>  |
> +-+-
> ---+
> | column2 | 11121
> |
> +-+-
> ---+
> | column3 | 1.0007824726134585
> |
> +-+-
> ---+
> | column4 | 5.
> |
> +-+-
> ---+
> | column5 | xx xx xx xx xx xx xx xx xx
> xx  |
> +-+-
> ---+
> | **RECORD 3**
> |
> +-+-
> ---+
> | column1 | donkey, cow, horse, tit,
> |
> | | eagle, whale,
>   |
> | | aligator,
> |
> | |pelican,
>   |
> | | grasshoper
> |
> | | pig
> |
> | | bat
>  |
> +-+-
> ---+
> | column2 | 14351
> |
> +-+-
> ---+
> | column3 | 50.3877551020408163
> |
> +-+-
> ---+
> | column4 | 345.11
> |
> +-+-
> ---+
> | column5 | yy yy yy yy yy yy yy yy yy
> yy  |
> +-+-
> ---+
>
>
>
>
>
>>
>>> - When \pset format is rst, then \pset linestyle also shows up as
>>>   "rst".  That is wrong.  Same for markdown.
>>>
>>
>> I will look on this.
>>
>>
>>>
>>> - Broken output in tuples_only (\t) mode. (rst and markdown)
>>>
>>
>> Similar to \x, im not certain, what it should return. I will look, what
>> returns html format. Or i can use it in markdown for nice vs expanded
>> format.
>>
>>
>>>
>>> - rst: Do something about \pset title; the way it currently shows up
>>>   appears to be invalid; could use ".. table:: title" directive
>>>
>>
>> OK, it shouldn`t be problem alter this.
>>
>>
>>>
>>> - markdown: Extra blank line between table and footer.
>>>
>>
>> It is because markdown needs empty line after table, if is row count
>> presented.
>>
>>
>>>
>>> - markdown: We should document or comment somewhere exactly which of the
>>>   various markdown table formats this 

Re: [HACKERS] createlang/droplang deprecated

2017-03-18 Thread Magnus Hagander
On Sat, Mar 18, 2017 at 6:13 PM, Robert Haas  wrote:

> On Sat, Mar 18, 2017 at 11:29 AM, Tom Lane  wrote:
> > Magnus Hagander  writes:
> >>> 2017-03-18 14:00 GMT+01:00 Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com>:
>  I just noticed that createlang and droplang have been listed as
>  deprecated since PG 9.1.
>  Do we dare remove them?
> >
> >> (I'd extend it to all the non-prefixed pg binaries, but let's open that
> can
> >> of worms right now, one thing at a time)
> >
> > To my mind, these two and createuser/dropuser are the only really serious
> > namespacing problems among our standard binaries.  The ones with names
> > ending in "db" don't seem likely to cause huge confusion.  I suppose that
> > if we were naming it today, "psql" wouldn't get that name; but the
> chances
> > of renaming that one are certainly zero, namespace conflict or no.
> >
> > But createuser/dropuser are a real problem, because they certainly could
> > be mistaken for system-level utilities.
>
> Well, let's do one thing at a time.  I think it'd be fine to drop
> createlang and droplang; we can discuss other things on other threads.
>

+1. I see no issues at all dropping createlang/droplang, so let's do that.
Some of the others can be worth discussing, so let's not wait for that to
pan out before these are removed.

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


Re: [HACKERS] PATCH: Configurable file mode mask

2017-03-18 Thread Robert Haas
On Wed, Mar 15, 2017 at 3:00 AM, Tsunakawa, Takayuki
 wrote:
> I'm on David's side, too.  I don't postmaster to always scan all files at 
> startup.

+1.  Even just doing it during crash recovery, it can take a
regrettably long time on machines with tons of relations and a very
slow disk.  I've been sort of thinking that we should add some logging
there so that users know what's happening when that code goes into the
tank - I've seen that come up 3 or 4 times now, and I'm getting tired
of telling people to run strace to find out.

I think Tom's concerns about people doing insecure stuff are
excessive.  People can do insecure stuff no matter what we do, and
trying to prevent them often leads to them doing even-more-insecure
stuff.  That having been aid, I do wonder whether the idea of allowing
group read privileges specifically might be a better concept than a
umask, though, because (1) it's not obvious that there's a real use
case for anything other than group read privileges, so why not support
exactly that to avoid user confusion and (2) umask is a pretty
specific concept that may not apply on every platform.  For example,
AFS has an ACL list instead of using the traditional UNIX permission
bits, and I'm not sure Windows has the umask concept exactly either.
So wording what we're trying to do a bit more generically might be
smart.

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


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


Re: [HACKERS] [PATCH] Use $ parameters as replacement characters for pg_stat_statements

2017-03-18 Thread Tom Lane
Robert Haas  writes:
> On Mon, Mar 13, 2017 at 6:05 PM, Tom Lane  wrote:
>> I wonder if it would improve matters to use $n, but starting with the
>> first number after the actual external Param symbols in the query.

> That sounds pretty sensible, although I guess it's got the weakness
> that you might get confused about which kind of $n you are looking at.
> However, I'd be inclined to deem that a fairly minor weakness and go
> with it: just because somebody could hypothetically get confused
> doesn't mean that real people will.

So it turns out this discussion just reinvented the alternative that
Lukas had in his 0002 proposal.  Are there any remaining objections
to proceeding with that approach?


In a quick read of the patch, I note that it falsifies and fails to
update the header comment for generate_normalized_query:

 * *query_len_p contains the input string length, and is updated with
 * the result string length (which cannot be longer) on exit.

It doesn't look like the calling code depends (any more?) on the
assumption that the string doesn't get longer, but it would be good
to double-check that.

This bit seems much more expensive and complicated than necessary:

+   /* Accomodate the additional query replacement characters */
+   norm_query_buflen = query_len;
+   for (i = 0; i < jstate->clocations_count; i++)
+   {
+   norm_query_buflen += floor(log10(abs(i + 1 + 
jstate->highest_extern_param_id))) + 1;
+   }

I'd just add, say, "10 * clocations_count" to the allocation length.
We can afford to waste a few bytes on this transient copy of the query
text.

The documentation is overly vague about what the parameter symbols are,
in particular failing to explain that their numbers start from after
the last $n occurring in the original query text.

It seems like a good idea to have a regression test case demonstrating
that, too.

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 v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

2017-03-18 Thread Elvis Pranskevichus
On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote:
> On Sat, Mar 18, 2017 at 2:56 AM, Elvis Pranskevichus 
 wrote:
> > Currently, clients wishing to know when the server exits hot standby
> > have to resort to polling, which is often suboptimal.
> > 
> > This adds the new "in_hot_standby" GUC variable that is reported via
> > a> 
> > ParameterStatus message.  This allows the clients to:
> >(a) know right away that they are connected to a server in hot
> >
> >standby; and
> >
> >(b) know immediately when a server exits hot standby.
> > 
> > This change will be most beneficial to various connection pooling
> > systems (pgpool etc.)
> 
> Why adding a good chunk of code instead of using pg_is_in_recovery(),
> which switches to false once a server exits recovery?

That requires polling the database continuously, which may not be 
possible or desirable.  

My main motivation here is to gain the ability to manage a pool of 
connections in asyncpg efficiently.  A part of the connection release 
protocol is "UNLISTEN *;", which the server in Hot Standby would fail to 
process.  Polling the database for pg_is_in_recovery() is not feasible 
in this case, unfortunately.

 Elvis  


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


Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

2017-03-18 Thread Petr Jelinek
On 16/03/17 17:15, David Steele wrote:
> On 2/1/17 3:59 PM, Pavel Stehule wrote:
>> Hi
>>
>> 2017-01-24 21:33 GMT+01:00 Pavel Stehule > >:
>>
>> Perhaps that's as simple as renaming all the existing _ns_*
>> functions to _block_ and then adding support for pragmas...
>>
>> Since you're adding cursor_options to PLpgSQL_expr it should
>> probably be removed as an option to exec_*.
>>
>> I have to recheck it. Some cursor options going from dynamic
>> cursor variables and are related to dynamic query - not query
>> that creates query string.  
>>
>> hmm .. so current state is better due using options like
>> CURSOR_OPT_PARALLEL_OK
>>
>>  if (expr->plan == NULL)
>> exec_prepare_plan(estate, expr, (parallelOK ?
>>   CURSOR_OPT_PARALLEL_OK : 0) |
>> expr->cursor_options);
>>
>> This options is not permanent feature of expression - and then I
>> cannot to remove cursor_option argument from exec_*
>>
>> I did minor cleaning - remove cursor_options from plpgsql_var
>>
>> + basic doc
> 
> This patch still applies cleanly and compiles at cccbdde.
> 
> Any reviewers want to have a look?
> 

I'll bite.

I agree with Jim that it's not very nice to add yet another
block/ns-like layer. I don't see why pragma could not be added to either
PLpgSQL_stmt_block (yes pragma can be for whole function but function
body is represented by PLpgSQL_stmt_block as well so no issue there), or
to namespace code. In namespace since they are used for other thing
there would be bit of unnecessary propagation but it's 8bytes per
namespace, does not seem all that much.

My preference would be to add it to PLpgSQL_stmt_block (unless we plan
to add posibility to add pragmas for other loops and other things) but I
am not sure if current block is easily (and in a fast way) accessible
from all places where it's needed. Maybe the needed info could be pushed
to estate from PLpgSQL_stmt_block during the execution.

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


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


Re: [HACKERS] New CORRESPONDING clause design

2017-03-18 Thread Pavel Stehule
2017-03-18 19:12 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > 2017-03-18 18:32 GMT+01:00 Tom Lane :
> >> I definitely don't see a reason for CORRESPONDING to track locations of
> >> name list elements when no other name list productions do.  It might be
> >> worth proposing a followon patch to change all of them (perhaps by
> adding
> >> a location field to struct "Value") and then make use of the locations
> in
> >> error messages more widely.
>
> > I had a idea use own node for  CORRESPONDING with location - and using
> this
> > location in related error messages.
>
> I think using a private node type for CORRESPONDING is exactly the wrong
> thing.  It's a columnList and it should be like other columnLists.  If
> there's an argument for providing a location for "no such column" errors
> for CORRESPONDING, then surely there's also an argument for providing
> a location for "no such column" errors for FOREIGN KEY and the other
> places where we have lists of column names.
>

The corresponding clause is used in UNION queries - these queries can be
pretty long, so marking wrong corresponding clause can be helpful.

Probably there are not any other argument for special node,

Regards

Pavel


>
> regards, tom lane
>


Re: [HACKERS] New CORRESPONDING clause design

2017-03-18 Thread Tom Lane
Pavel Stehule  writes:
> 2017-03-18 18:32 GMT+01:00 Tom Lane :
>> I definitely don't see a reason for CORRESPONDING to track locations of
>> name list elements when no other name list productions do.  It might be
>> worth proposing a followon patch to change all of them (perhaps by adding
>> a location field to struct "Value") and then make use of the locations in
>> error messages more widely.

> I had a idea use own node for  CORRESPONDING with location - and using this
> location in related error messages.

I think using a private node type for CORRESPONDING is exactly the wrong
thing.  It's a columnList and it should be like other columnLists.  If
there's an argument for providing a location for "no such column" errors
for CORRESPONDING, then surely there's also an argument for providing
a location for "no such column" errors for FOREIGN KEY and the other
places where we have lists of column names.

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] Renaming of pg_xlog and pg_clog

2017-03-18 Thread Fujii Masao
On Fri, Mar 17, 2017 at 11:03 PM, Michael Paquier
 wrote:
> On Fri, Mar 17, 2017 at 10:58 PM, Robert Haas  wrote:
>> Fine!  I've committed the pg_clog renaming, but I'd really like to
>> draw the line here.  I'm not going to commit the pg_subtrans ->
>> pg_subxact naming and am -1 on anyone else doing so.  I think that
>> having the names of things in the code match what shows up in the file
>> system is an awfully desirable property which we should preserve
>> insofar as we can do so without compromising other goals.  Not
>> invalidating what users and DBAs think they know about how PostgreSQL
>> works is a good thing, too; there are a lot more people who know
>> something about the directory layout than will read this thread.
>
> I'm cool with that, thanks for the commit.

monitoring.sgml still has some references to "clog". We should update them?

Regards,

-- 
Fujii Masao


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


Re: [HACKERS] New CORRESPONDING clause design

2017-03-18 Thread Pavel Stehule
2017-03-18 18:32 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > 2017-03-18 17:50 GMT+01:00 Tom Lane :
> >> I'm not impressed by using A_Const for the members of the CORRESPONDING
> >> name list.  That's not a clever solution, that's a confusing kluge,
> >> because it's a complete violation of the meaning of A_Const.  Elsewhere
> >> we just use lists of String for name lists, and that seems sufficient
> >> here.  Personally I'd just use the existing columnList production rather
> >> than rolling your own.
>
> > The reason was attach a location to name for more descriptive error
> > message.
>
> [ shrug... ] The patch fails to actually use the location anywhere.
> If it had, you might have noticed that it's attaching the wrong location
> to all elements except the first :-(.  So I'm not very excited about that.
> I definitely don't see a reason for CORRESPONDING to track locations of
> name list elements when no other name list productions do.  It might be
> worth proposing a followon patch to change all of them (perhaps by adding
> a location field to struct "Value") and then make use of the locations in
> error messages more widely.
>

I had a idea use own node for  CORRESPONDING with location - and using this
location in related error messages.

What do you think about it?

Regards

Pavel

>
> regards, tom lane
>


Re: [HACKERS] New CORRESPONDING clause design

2017-03-18 Thread Tom Lane
Pavel Stehule  writes:
> 2017-03-18 17:50 GMT+01:00 Tom Lane :
>> I'm not impressed by using A_Const for the members of the CORRESPONDING
>> name list.  That's not a clever solution, that's a confusing kluge,
>> because it's a complete violation of the meaning of A_Const.  Elsewhere
>> we just use lists of String for name lists, and that seems sufficient
>> here.  Personally I'd just use the existing columnList production rather
>> than rolling your own.

> The reason was attach a location to name for more descriptive error
> message.

[ shrug... ] The patch fails to actually use the location anywhere.
If it had, you might have noticed that it's attaching the wrong location
to all elements except the first :-(.  So I'm not very excited about that.
I definitely don't see a reason for CORRESPONDING to track locations of
name list elements when no other name list productions do.  It might be
worth proposing a followon patch to change all of them (perhaps by adding
a location field to struct "Value") and then make use of the locations in
error messages more widely.

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] [POC] A better way to expand hash indexes.

2017-03-18 Thread Mithun Cy
On Thu, Mar 16, 2017 at 10:55 PM, David Steele  wrote:
> It does apply with fuzz on 2b32ac2, so it looks like c11453c and
> subsequent commits are the cause.  They represent a fairly substantial
> change to hash indexes by introducing WAL logging so I think you should
> reevaluate your patches to be sure they still function as expected.

Thanks, David here is the new improved patch I have also corrected the
pageinspect's test output. Also, added notes in README regarding the
new way of adding bucket pages efficiently in hash index. I also did
some more tests pgbench read only and read write;
There is no performance impact due to the patch. The growth of index
size has become much efficient as the numbers posted in the initial
proposal mail.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


expand_hashbucket_efficiently_03.patch
Description: Binary data

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


Re: [HACKERS] wait events for disk I/O

2017-03-18 Thread Rushabh Lathia
On Sat, Mar 18, 2017 at 5:15 PM, Robert Haas  wrote:

> On Fri, Mar 17, 2017 at 10:01 AM, Rushabh Lathia
>  wrote:
> > I tried to cover all the suggestion in the attached latest patch.
>
> Committed.  I reworded the documentation entries, renamed a few of the
> wait events to make things more consistent, put all three lists in
> rigorous alphabetical order, and I fixed a couple of places where an
> error return from a system call could lead to returning without
> clearing the wait event.
>
>
Thanks Robert.


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



-- 
Rushabh Lathia


Re: [HACKERS] New CORRESPONDING clause design

2017-03-18 Thread Pavel Stehule
2017-03-18 17:50 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > I have not any objection - I'll mark this patch as ready for commiter
>
> I took a quick look through this and noted that it fails to touch
> ruleutils.c, which means that dumping of views containing CORRESPONDING
> certainly doesn't work.
>

it my mistake - this bug I should to see :(

>
> Also, the changes in parser/analyze.c seem rather massive and
> correspondingly hard to review.  Is it possible to rearrange the
> patch to reduce the size of that diff?  If you can avoid moving
> or reindenting existing code, that'd help.
>
> The code in that area seems rather confused, too.  For instance,
> I'm not sure exactly what orderCorrespondingList() is good for,
> but it certainly doesn't look to be doing anything that either its
> name or its header comment (or the comments at the call sites) would
> suggest.  Its input and output tlists are always in the same order.
>
> I'm a little disturbed by the fact that determineMatchingColumns()
> is called twice, and more disturbed by the fact that it looks to be
> O(N^2).  This could be really slow with a lot of columns, couldn't it?
>
> I also think there should be some comments about exactly what matching
> semantics we're enforcing.   The SQL standard says
>
> a) If CORRESPONDING is specified, then:
>   i) Within the columns of T1, equivalent s shall
>  not be specified more than once and within the columns of
>  T2, equivalent s shall not be specified more
>  than once.
>
> That seems unreasonably stringent to me; it ought to be sufficient to
> forbid duplicates of the names listed in CORRESPONDING, or the common
> column names if there's no BY list.  But whichever restriction you prefer,
> this code seems to be failing to check it --- I certainly don't see any
> new error message about "column name "foo" appears more than once".
>
> I don't think you want to be leaving behind debugging cruft like this:
> +   elog(DEBUG4, "%s", ltle->resname);
>
> I'm not impressed by using A_Const for the members of the CORRESPONDING
> name list.  That's not a clever solution, that's a confusing kluge,
> because it's a complete violation of the meaning of A_Const.  Elsewhere
> we just use lists of String for name lists, and that seems sufficient
> here.  Personally I'd just use the existing columnList production rather
> than rolling your own.
>

The reason was attach a location to name for more descriptive error
message.


> The comments in parsenodes.h about the new fields seem rather confused,
> in fact I think they're probably backwards.
>
> Also, I thought the test cases were excessively uninspired and repetitive.
> This, for example, seems like about two tests too many:
>
> +SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(a) SELECT 4 a, 5 b, 6 c;
> + a
> +---
> + 1
> + 4
> +(2 rows)
> +
> +SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(b) SELECT 4 a, 5 b, 6 c;
> + b
> +---
> + 2
> + 5
> +(2 rows)
> +
> +SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(c) SELECT 4 a, 5 b, 6 c;
> + c
> +---
> + 3
> + 6
> +(2 rows)
>
> without even considering the fact that these are pretty duplicative of
> tests around them.  And some of the added tests seem to be just testing
> basic UNION functionality, which we already have tests for.  I fail to
> see the point of adding any of these:
>
> +SELECT 1 AS two UNION CORRESPONDING SELECT 2 two;
> + two
> +-
> +   1
> +   2
> +(2 rows)
> +
> +SELECT 1 AS one UNION CORRESPONDING SELECT 1 one;
> + one
> +-
> +   1
> +(1 row)
> +
> +SELECT 1 AS two UNION ALL CORRESPONDING SELECT 2 two;
> + two
> +-
> +   1
> +   2
> +(2 rows)
> +
> +SELECT 1 AS two UNION ALL CORRESPONDING SELECT 1 two;
> + two
> +-
> +   1
> +   1
> +(2 rows)
>
> What I think actually is needed is some targeted testing showing
> non-interference with nearby features.  As an example, CORRESPONDING
> can't safely be implemented by just replacing the tlists of the
> input sub-selects with shortened versions, because that would break
> situations such as DISTINCT in the sub-selects.  I think it'd be a
> good idea to have a test along the lines of
>
> SELECT DISTINCT x, y FROM ...
> UNION ALL CORRESPONDING
> SELECT DISTINCT x, z FROM ...
>
> with values chosen to show that the DISTINCT operators did operate
> on x/y and x/z, not just x alone.
>
> regards, tom lane
>


Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

2017-03-18 Thread Corey Huinker
On Fri, Mar 17, 2017 at 2:18 PM, Corey Huinker 
wrote:

>
>> > \set x 'arg1 arg2'
>>
>> > \if false
>> > \cmd_that_takes_exactly_two_args :x
>> > \endif
>>
>> Yeah, throwing errors for bad arguments would also need to be suppressed.
>>
>> regards, tom lane
>>
>
> Ok, barring other feedback, I'm going to take my marching orders as "make
> each slash command active-aware". To keep that sane, I'm probably going to
> break out each slash command family into it's own static function.
>

...and here it is.

v24 highlights:

- finally using git format-patch
- all conditional slash commands broken out into their own functions
(exec_command_$NAME) , each one tests if it's in an active branch, and if
it's not it consumes the same number of parameters, but discards them.
comments for each slash-command family were left as-is, may need to be
bulked up.
- TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif
placement
- documentation recommending ON_ERROR_STOP removed, because invalid
expressions no longer throw off if-endif balance
- documentation updated to reflex that contextually-correct-but-invalid
boolean expressions are treated as false
- psql_get_variable has a passthrough void pointer now, but I ended up not
needing it. Instead, all slash commands in false blocks either fetch
OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know.
From da8306acab0f0304d62f966c5217139bacfe722e Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Sat, 18 Mar 2017 12:47:25 -0400
Subject: [PATCH] Add \if...\endif blocks

---
 doc/src/sgml/ref/psql-ref.sgml |   90 +-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1513 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 ++
 src/bin/psql/conditional.h |   62 +
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql-on-error-stop.out   |  506 +++
 src/test/regress/expected/psql.out |  100 ++
 .../regress/expected/psql_if_on_error_stop.out |   14 +
 src/test/regress/parallel_schedule |2 +-
 src/test/regress/serial_schedule   |1 +
 src/test/regress/sql/psql.sql  |  100 ++
 src/test/regress/sql/psql_if_on_error_stop.sql |   17 +
 20 files changed, 2316 insertions(+), 284 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h
 create mode 100644 src/test/regress/expected/psql-on-error-stop.out
 create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out
 create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+The expression argument
+of \if or 

Re: [HACKERS] createlang/droplang deprecated

2017-03-18 Thread Robert Haas
On Sat, Mar 18, 2017 at 11:29 AM, Tom Lane  wrote:
> Magnus Hagander  writes:
>>> 2017-03-18 14:00 GMT+01:00 Peter Eisentraut 
>>> :
 I just noticed that createlang and droplang have been listed as
 deprecated since PG 9.1.
 Do we dare remove them?
>
>> (I'd extend it to all the non-prefixed pg binaries, but let's open that can
>> of worms right now, one thing at a time)
>
> To my mind, these two and createuser/dropuser are the only really serious
> namespacing problems among our standard binaries.  The ones with names
> ending in "db" don't seem likely to cause huge confusion.  I suppose that
> if we were naming it today, "psql" wouldn't get that name; but the chances
> of renaming that one are certainly zero, namespace conflict or no.
>
> But createuser/dropuser are a real problem, because they certainly could
> be mistaken for system-level utilities.

Well, let's do one thing at a time.  I think it'd be fine to drop
createlang and droplang; we can discuss other things on other threads.

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


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


Re: [HACKERS] background sessions

2017-03-18 Thread Robert Haas
On Sat, Mar 18, 2017 at 10:59 AM, Petr Jelinek
 wrote:
>> shm_redirect_to_shm_mq() wasn't really designed to be used this way;
>> it's designed for use by the worker, not the process that launched it.
>> If an error occurs while output is redirected, bad things will happen.
>> I think it would be better to find a way of sending that message to
>> the queue without doing this.
>
> Couldn't we just create special version of pq_endmessage that sends to
> shm_mq?

Yes, I think that sounds good.

>> Also, I suspect this is deadlock-prone.  If we get stuck trying to
>> send a message to the background session while the queue is full, and
>> at the same time the session is stuck trying to send us a long error
>> message, we will have an undetected deadlock.  That's why
>> pg_background() puts the string being passed to the worker into the
>> DSM segment in its entirety, rather than sending it through a shm_mq.
>
> Yeah I think this will need to use the nowait = true when sending to
> shm_mq and chunk the message if necessary...

Hmm, yeah.  If you take breaks while sending to check for data that
you need to receive, then you should be fine.

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


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


[HACKERS] Re: new set of psql patches for loading (saving) data from (to) text, binary files

2017-03-18 Thread Pavel Stehule
Hi


> > > There's a whitespace-only hunk that shouldn't be included.
> > >
> > > I don't agree with the single-column/single-row restriction on these.
> I
> > > can certainly see a case where someone might want to, say, dump out a
> > > bunch of binary integers into a file for later processing.
> > >
> > > The tab-completion for 'gstore' wasn't correct (you didn't include the
> > > double-backslash).  The patch also has conflicts against current master
> > > now.
> > >
> > > I guess my thinking about moving this forward would be to simplify it
> to
> > > just '\gb' which will pull the data from the server side in binary
> > > format and dump it out to the filename or command given.  If there's a
> > > new patch with those changes, I'll try to find time to look at it.
> >
> > ok I'll prepare patch
>
> Great, thanks!
>

I rewrote these patches - it allows binary export/import from psql and the
code is very simple. The size of the patch is bigger due including 4KB
binary file (in hex format 8KB).

What is done:

create table foo foo(a bytea);

-- import
insert into foo values($1)
\gloadfrom ~/xxx.jpg bytea

-- export
\pset format binary
select a from foo
\g ~/xxx2.jpg

tested on import 55MB binary file

Comments, notes?

Available import formats are limited to text, bytea, xml - these formats
are safe for receiving data via recv function.

Regards

Pavel



>
> Stephen
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412020..f26d406f89 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1945,6 +1945,38 @@ CREATE INDEX
 
 
   
+\gloadfrom filename  format  
+
+
+
+ Sends the current query input buffer to the server. The current
+ query should to have one query parameter $1.
+ The content of filename file will be used as
+ the value of this parameter.
+
+
+
+ When format is not specified, then data are
+ passed in text format. When format is specified, then data are
+ passed in binary format. The available formats are:
+ text, bytea
+ or xml type. In the example the XML document is 
+ imported to table my_table:
+
+= CREATE TABLE my_table(id serial, doc xml);
+= INSERT INTO my_table(doc) VALUES($1) RETURNING id
+- \gloadfrom ~/Documents/data.xml xml
+ id 
+
+  1
+(1 row)
+
+
+
+  
+
+
+  
 \gset [ prefix ]
 
 
@@ -2366,8 +2398,8 @@ lo_import 152801
   aligned, wrapped,
   html, asciidoc,
   latex (uses tabular),
-  latex-longtable, or
-  troff-ms.
+  latex-longtable, troff-ms or
+  binary.
   Unique abbreviations are allowed.  (That would mean one letter
   is enough.)
   
@@ -2404,6 +2436,12 @@ lo_import 152801
   also requires the LaTeX
   longtable and booktabs packages.
   
+
+  
+  The binary format is simply output values in
+  binary format.
+  
+
   
   
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 4f4a0aa9bd..51ed3df58c 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -28,6 +28,7 @@
 #endif
 
 #include "catalog/pg_class.h"
+#include "catalog/pg_type.h"
 #include "portability/instr_time.h"
 
 #include "libpq-fe.h"
@@ -936,6 +937,50 @@ exec_command(const char *cmd,
 		status = PSQL_CMD_SEND;
 	}
 
+	/*
+	 * \gloadfrom filename -- send query and use content of file as parameter
+	 */
+	else if (strcmp(cmd, "gloadfrom") == 0)
+	{
+		char	   *fmt = NULL;
+		char	   *fname = psql_scan_slash_option(scan_state,
+   OT_FILEPIPE, NULL, false);
+
+		if (!fname)
+		{
+			psql_error("\\%s: missing required argument\n", cmd);
+			success = false;
+		}
+		else
+		{
+			/* try to get separate format arg */
+			fmt = psql_scan_slash_option(scan_state,
+		OT_NORMAL, NULL, true);
+
+			if (fmt)
+			{
+if (strcmp(fmt, "text") == 0)
+	pset.gloadfrom_fmt = TEXTOID;
+else if (strcmp(fmt, "bytea") == 0)
+	pset.gloadfrom_fmt = BYTEAOID;
+else if (strcmp(fmt, "xml") == 0)
+	pset.gloadfrom_fmt = XMLOID;
+else
+{
+	psql_error("\\%s: only [text, bytea, xml] format can be specified\n", cmd);
+	success = false;
+}
+			}
+			else
+pset.gloadfrom_fmt = 0;		/* UNKNOWNOID */
+
+			expand_tilde();
+			pset.gloadfrom = pg_strdup(fname);
+		}
+		free(fname);
+		status = PSQL_CMD_SEND;
+	}
+
 	/* \gset [prefix] -- send query and store result into variables */
 	else if (strcmp(cmd, "gset") == 0)
 	{
@@ -2518,6 +2563,9 @@ _align2string(enum printFormat in)
 		case PRINT_TROFF_MS:
 			return "troff-ms";
 			break;
+		case PRINT_BINARY:
+			return "binary";
+			break;
 	}
 	return "unknown";
 }
@@ -2589,9 +2637,11 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			popt->topt.format = PRINT_LATEX_LONGTABLE;
 		

Re: [HACKERS] New CORRESPONDING clause design

2017-03-18 Thread Tom Lane
Pavel Stehule  writes:
> I have not any objection - I'll mark this patch as ready for commiter

I took a quick look through this and noted that it fails to touch
ruleutils.c, which means that dumping of views containing CORRESPONDING
certainly doesn't work.

Also, the changes in parser/analyze.c seem rather massive and
correspondingly hard to review.  Is it possible to rearrange the
patch to reduce the size of that diff?  If you can avoid moving
or reindenting existing code, that'd help.

The code in that area seems rather confused, too.  For instance,
I'm not sure exactly what orderCorrespondingList() is good for,
but it certainly doesn't look to be doing anything that either its
name or its header comment (or the comments at the call sites) would
suggest.  Its input and output tlists are always in the same order.

I'm a little disturbed by the fact that determineMatchingColumns()
is called twice, and more disturbed by the fact that it looks to be
O(N^2).  This could be really slow with a lot of columns, couldn't it?

I also think there should be some comments about exactly what matching
semantics we're enforcing.   The SQL standard says

a) If CORRESPONDING is specified, then:
  i) Within the columns of T1, equivalent s shall
 not be specified more than once and within the columns of
 T2, equivalent s shall not be specified more
 than once.

That seems unreasonably stringent to me; it ought to be sufficient to
forbid duplicates of the names listed in CORRESPONDING, or the common
column names if there's no BY list.  But whichever restriction you prefer,
this code seems to be failing to check it --- I certainly don't see any
new error message about "column name "foo" appears more than once".

I don't think you want to be leaving behind debugging cruft like this:
+   elog(DEBUG4, "%s", ltle->resname);

I'm not impressed by using A_Const for the members of the CORRESPONDING
name list.  That's not a clever solution, that's a confusing kluge,
because it's a complete violation of the meaning of A_Const.  Elsewhere
we just use lists of String for name lists, and that seems sufficient
here.  Personally I'd just use the existing columnList production rather
than rolling your own.

The comments in parsenodes.h about the new fields seem rather confused,
in fact I think they're probably backwards.

Also, I thought the test cases were excessively uninspired and repetitive.
This, for example, seems like about two tests too many:

+SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(a) SELECT 4 a, 5 b, 6 c;
+ a 
+---
+ 1
+ 4
+(2 rows)
+
+SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(b) SELECT 4 a, 5 b, 6 c;
+ b 
+---
+ 2
+ 5
+(2 rows)
+
+SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(c) SELECT 4 a, 5 b, 6 c;
+ c 
+---
+ 3
+ 6
+(2 rows)

without even considering the fact that these are pretty duplicative of
tests around them.  And some of the added tests seem to be just testing
basic UNION functionality, which we already have tests for.  I fail to
see the point of adding any of these:

+SELECT 1 AS two UNION CORRESPONDING SELECT 2 two;
+ two 
+-
+   1
+   2
+(2 rows)
+
+SELECT 1 AS one UNION CORRESPONDING SELECT 1 one;
+ one 
+-
+   1
+(1 row)
+
+SELECT 1 AS two UNION ALL CORRESPONDING SELECT 2 two;
+ two 
+-
+   1
+   2
+(2 rows)
+
+SELECT 1 AS two UNION ALL CORRESPONDING SELECT 1 two;
+ two 
+-
+   1
+   1
+(2 rows)

What I think actually is needed is some targeted testing showing
non-interference with nearby features.  As an example, CORRESPONDING
can't safely be implemented by just replacing the tlists of the
input sub-selects with shortened versions, because that would break
situations such as DISTINCT in the sub-selects.  I think it'd be a
good idea to have a test along the lines of

SELECT DISTINCT x, y FROM ...
UNION ALL CORRESPONDING
SELECT DISTINCT x, z FROM ...

with values chosen to show that the DISTINCT operators did operate
on x/y and x/z, not just x alone.

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] createlang/droplang deprecated

2017-03-18 Thread Tom Lane
Magnus Hagander  writes:
>> 2017-03-18 14:00 GMT+01:00 Peter Eisentraut 
>> :
>>> I just noticed that createlang and droplang have been listed as
>>> deprecated since PG 9.1.
>>> Do we dare remove them?

> (I'd extend it to all the non-prefixed pg binaries, but let's open that can
> of worms right now, one thing at a time)

To my mind, these two and createuser/dropuser are the only really serious
namespacing problems among our standard binaries.  The ones with names
ending in "db" don't seem likely to cause huge confusion.  I suppose that
if we were naming it today, "psql" wouldn't get that name; but the chances
of renaming that one are certainly zero, namespace conflict or no.

But createuser/dropuser are a real problem, because they certainly could
be mistaken for system-level utilities.

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

2017-03-18 Thread Petr Jelinek
On 15/03/17 17:58, Robert Haas wrote:
> On Wed, Mar 15, 2017 at 6:43 AM, Pavel Stehule  
> wrote:
>> I don't understand - CHECK_FOR_INTERRUPTS called from executor implicitly.
> 
> True.  So there shouldn't be any problem here.  I'm confused as can be
> about what you want changed.
> 
> Some review of the patch itself:
> 
> +pq_redirect_to_shm_mq(session->seg, session->command_qh);
> +pq_beginmessage(, 'X');
> +pq_endmessage();
> +pq_stop_redirect_to_shm_mq();
> 
> shm_redirect_to_shm_mq() wasn't really designed to be used this way;
> it's designed for use by the worker, not the process that launched it.
> If an error occurs while output is redirected, bad things will happen.
> I think it would be better to find a way of sending that message to
> the queue without doing this.

Couldn't we just create special version of pq_endmessage that sends to
shm_mq?

> 
> Also, I suspect this is deadlock-prone.  If we get stuck trying to
> send a message to the background session while the queue is full, and
> at the same time the session is stuck trying to send us a long error
> message, we will have an undetected deadlock.  That's why
> pg_background() puts the string being passed to the worker into the
> DSM segment in its entirety, rather than sending it through a shm_mq.
> 

Yeah I think this will need to use the nowait = true when sending to
shm_mq and chunk the message if necessary...

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


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


Re: [HACKERS] createlang/droplang deprecated

2017-03-18 Thread Magnus Hagander
On Sat, Mar 18, 2017 at 3:09 PM, Pavel Stehule 
wrote:

>
>
> 2017-03-18 14:00 GMT+01:00 Peter Eisentraut  com>:
>
>> I just noticed that createlang and droplang have been listed as
>> deprecated since PG 9.1.
>>
>> Do we dare remove them?
>>
>
> +1
>
>
+1.

(I'd extend it to all the non-prefixed pg binaries, but let's open that can
of worms right now, one thing at a time)

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


Re: [HACKERS] createlang/droplang deprecated

2017-03-18 Thread Pavel Stehule
2017-03-18 14:00 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> I just noticed that createlang and droplang have been listed as
> deprecated since PG 9.1.
>
> Do we dare remove them?
>

+1

Pavel


>
> --
> Peter Eisentraut  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] logical replication access control patches

2017-03-18 Thread Petr Jelinek
Hi,

I went over this patch set, don't really have all that much to say
except it looks good for the most part (details inline).


On 16/03/17 02:54, Peter Eisentraut wrote:
> New patch set based on the discussions.  I have dropped the PUBLICATION
> privilege patch.  The patches are also reordered a bit in approximate
> decreasing priority order.
> 
> 0001 Refine rules for altering publication owner
> 
> kind of a bug fix

Agreed, this can be committed as is.

> 
> 0002 Change logical replication pg_hba.conf use
> 
> This was touched upon in the discussion at
> 
> and seems to have been viewed favorably there.

Seems like a good idea and I think can be committed as well.

> 
> 0003 Add USAGE privilege for publications
> 
> a way to control who can subscribe to a publication
> 

Hmm IIUC this removes ability of REPLICATION role to subscribe to
publications. I am not quite sure I like that.

> 0004 Add subscription apply worker privilege checks
> 
> This is a prerequisite for the next one (or one like it).
> 
> 0005 Add CREATE SUBSCRIPTION privilege on databases
> 
> Need a way to determine which user can create subscriptions.  The
> presented approach made sense to me, but maybe there are other ideas.
> 

The CREATE SUBSCRIPTION as name of privilege is bit weird but something
like SUBSCRIBE would be more fitting for publish side (to which you
subscriber) so don't really have a better name. I like that the patches
cache the acl result so performance impact should be negligible.

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


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


[HACKERS] createlang/droplang deprecated

2017-03-18 Thread Peter Eisentraut
I just noticed that createlang and droplang have been listed as
deprecated since PG 9.1.

Do we dare remove them?

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


[HACKERS] Re: [COMMITTERS] pgsql: Add TAP tests for password-based authentication methods.

2017-03-18 Thread Michael Paquier
On Fri, Mar 17, 2017 at 6:37 PM, Heikki Linnakangas
 wrote:
> Add TAP tests for password-based authentication methods.
>
> Tests all combinations of users with MD5, plaintext and SCRAM verifiers
> stored in pg_authid, with plain 'password', 'md5' and 'scram'
> authentication methods.

This patch has forgotten two things:
1) src/test/authentication/.gitignore.
2) A refresh of vcregress.pl to run this test suite on Windows.
Please find attached a patch to address both issues.
-- 
Michael
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index ecec0a60c7..4ef8d31f8e 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -451,6 +451,7 @@ $ENV{CONFIG}="Debug";
 vcregress ecpgcheck
 vcregress isolationcheck
 vcregress bincheck
+vcregress authcheck
 vcregress recoverycheck
 vcregress upgradecheck
 
@@ -467,8 +468,9 @@ $ENV{CONFIG}="Debug";
 
   
Running the regression tests on client programs, with
-   vcregress bincheck, or on recovery tests, with
-   vcregress recoverycheck, requires an additional Perl module
+   vcregress bincheck, on recovery tests, with
+   vcregress recoverycheck, or on authentication tests with
+   vcregress authcheck requires an additional Perl module
to be installed:

 
diff --git a/src/test/authentication/.gitignore b/src/test/authentication/.gitignore
new file mode 100644
index 00..871e943d50
--- /dev/null
+++ b/src/test/authentication/.gitignore
@@ -0,0 +1,2 @@
+# Generated by test suite
+/tmp_check/
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index f1b9819cd2..c3cb4d0c74 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -34,7 +34,7 @@ if (-e "src/tools/msvc/buildenv.pl")
 
 my $what = shift || "";
 if ($what =~
-/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck|recoverycheck)$/i
+/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck|recoverycheck|authcheck)$/i
   )
 {
 	$what = uc $what;
@@ -90,6 +90,7 @@ my %command = (
 	ISOLATIONCHECK => \,
 	BINCHECK   => \,
 	RECOVERYCHECK  => \,
+	AUTHCHECK  => \,
 	UPGRADECHECK   => \,);
 
 my $proc = $command{$what};
@@ -373,6 +374,16 @@ sub recoverycheck
 	exit $status if $status;
 }
 
+sub authcheck
+{
+	InstallTemp();
+
+	my $mstat  = 0;
+	my $dir= "$topdir/src/test/authentication";
+	my $status = tap_check($dir);
+	exit $status if $status;
+}
+
 # Run "initdb", then reconfigure authentication.
 sub standard_initdb
 {
@@ -599,6 +610,7 @@ sub usage
 	print STDERR
 	  "Usage: vcregress.pl  [  ]\n\n",
 	  "Options for :\n",
+	  "  authcheck  run authentication test suite\n",
 	  "  bincheck   run tests of utilities in src/bin/\n",
 	  "  check  deploy instance and run regression tests on it\n",
 	  "  contribcheck   run tests of modules in contrib/\n",

-- 
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] logical replication apply to run with sync commit off by default

2017-03-18 Thread Petr Jelinek
On 07/03/17 06:23, Petr Jelinek wrote:
> Hi,
> 
> there has been discussion at the logical replication initial copy thread
> [1] about making apply work with sync commit off by default for
> performance reasons and adding option to change that per subscription.
> 
> Here I propose patch to implement this - it adds boolean column
> subssynccommit to pg_subscription catalog which decides if
> synchronous_commit should be off or local for apply. And it adds
> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and
> ALTER SUBSCRIPTION. When nothing is specified it will set it to false.
> 
> The patch is built on top of copy patch currently as there are conflicts
> between the two and this helps a bit with testing of copy patch.
> 
> [1]
> https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xoor8j71mad1oteojawmuje3...@mail.gmail.com
> 

I rebased this patch against recent changes and the latest version of
copy patch.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
>From 0876a23ddc385a6c4dc8dc26abcd1e82c9ab2482 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription

This also changes default behaviour of subscription workers to
synchronous_commit = off
---
 doc/src/sgml/catalogs.sgml | 11 ++
 doc/src/sgml/ref/alter_subscription.sgml   |  1 +
 doc/src/sgml/ref/create_subscription.sgml  | 15 
 src/backend/catalog/pg_subscription.c  |  1 +
 src/backend/commands/subscriptioncmds.c| 59 +-
 src/backend/replication/logical/launcher.c |  2 +-
 src/backend/replication/logical/worker.c   | 28 +-
 src/bin/pg_dump/pg_dump.c  | 12 +-
 src/bin/pg_dump/pg_dump.h  |  1 +
 src/bin/psql/describe.c|  5 ++-
 src/include/catalog/pg_subscription.h  | 11 --
 src/test/regress/expected/subscription.out | 27 +++---
 src/test/regress/sql/subscription.sql  |  3 +-
 13 files changed, 143 insertions(+), 33 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 228ec78..f71d9c9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6395,6 +6395,17 @@
  
 
  
+  subsynccommit
+  bool
+  
+  
+   If true, the apply for the subscription will run with
+   synchronous_commit set to local.
+   Otherwise it will have it set to false.
+  
+ 
+
+ 
   subconninfo
   text
   
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6335e17..712de98 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,7 @@ ALTER SUBSCRIPTION name WITH ( where suboption can be:
 
 SLOT NAME = slot_name
+| SYNCHRONOUS_COMMIT = boolean
 
 ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] { REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }
 ALTER SUBSCRIPTION name REFRESH PUBLICATION WITH ( puboption [, ... ] )
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 6468470..6baff2f 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -29,6 +29,7 @@ CREATE SUBSCRIPTION subscription_name
  
@@ -145,6 +146,20 @@ CREATE SUBSCRIPTION subscription_name
 

+SYNCHRONOUS_COMMIT = boolean
+
+ 
+  Modifies the synchronous_commit setting of the
+  subscription workers. When set to true, the
+  synchronous_commit for the worker will be set to
+  local otherwise to false. The
+  default value is false independently of the default
+  synchronous_commit setting for the instance.
+ 
+
+   
+
+   
 NOCONNECT
 
  
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9b74892..26921aa 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -68,6 +68,7 @@ GetSubscription(Oid subid, bool missing_ok)
 	sub->name = pstrdup(NameStr(subform->subname));
 	sub->owner = subform->subowner;
 	sub->enabled = subform->subenabled;
+	sub->synccommit = subform->subsynccommit;
 
 	/* Get conninfo */
 	datum = SysCacheGetAttr(SUBSCRIPTIONOID,
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index cba2d5c..402f682 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -60,12 +60,13 @@ static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
 static void
 parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 		   bool *enabled, bool *create_slot, char **slot_name,
-		   bool *copy_data)
+		   bool *copy_data, bool 

Re: [HACKERS] ANALYZE command progress checker

2017-03-18 Thread Ashutosh Sharma
Hi Vinayak,

Here are couple of review comments that may need your attention.

1. Firstly, I am seeing some trailing whitespace errors when trying to
apply your v3 patch using git apply command.

[ashu@localhost postgresql]$ git apply pg_stat_progress_analyze_v3.patch
pg_stat_progress_analyze_v3.patch:155: trailing whitespace.
CREATE VIEW pg_stat_progress_analyze AS
pg_stat_progress_analyze_v3.patch:156: trailing whitespace.
SELECT
pg_stat_progress_analyze_v3.patch:157: trailing whitespace.
S.pid AS pid, S.datid AS datid, D.datname AS datname,
pg_stat_progress_analyze_v3.patch:158: trailing whitespace.
S.relid AS relid,
pg_stat_progress_analyze_v3.patch:159: trailing whitespace.
CASE S.param1 WHEN 0 THEN 'initializing'
error: patch failed: doc/src/sgml/monitoring.sgml:521

2) The test-case 'rules' is failing.  I think it's failing because in
rules.sql 'ORDERBY viewname' is used which will put
'pg_stat_progress_analyze' ahead of 'pg_stat_progress_vacuum' in the
list of catalog views. You may need to correct rules.out file
accordingly. This is what i could see in rules.sql,

SELECT viewname, definition FROM pg_views WHERE schemaname <>
'information_schema' ORDER BY viewname;

I am still reviewing your patch and doing some testing. Will update if
i find any issues. Thanks.

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

On Fri, Mar 17, 2017 at 3:16 PM, vinayak
 wrote:
>
> On 2017/03/17 10:38, Robert Haas wrote:
>>
>> On Fri, Mar 10, 2017 at 2:46 AM, vinayak
>>  wrote:
>>>
>>> Thank you for reviewing the patch.
>>>
>>> The attached patch incorporated Michael and Amit comments also.
>>
>> I reviewed this tonight.
>
> Thank you for reviewing the patch.
>>
>> +/* Report compute index stats phase */
>> +pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
>> +
>> PROGRESS_ANALYZE_PHASE_COMPUTE_INDEX_STATS);
>>
>> Hmm, is there really any point to this?  And is it even accurate?  It
>> doesn't look to me like we are computing any index statistics here;
>> we're only allocating a few in-memory data structures here, I think,
>> which is not a statistics computation and probably doesn't take any
>> significant time.
>>
>> +/* Report compute heap stats phase */
>> +pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
>> +
>> PROGRESS_ANALYZE_PHASE_COMPUTE_HEAP_STATS);
>>
>> The phase you label as computing heap statistics also includes the
>> computation of index statistics.  I wouldn't bother separating the
>> computation of heap and index statistics; I'd just have a "compute
>> statistics" phase that begins right after the comment that starts
>> "Compute the statistics."
>
> Understood. Fixed in the attached patch.
>>
>>
>> +/* Report that we are now doing index cleanup */
>> +pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
>> +PROGRESS_ANALYZE_PHASE_INDEX_CLEANUP);
>>
>> OK, this doesn't make any sense either.  We are not cleaning up the
>> indexes here.  We are just closing them and releasing resources.  I
>> don't see why you need this phase at all; it can't last more than some
>> tiny fraction of a second.  It seems like you're trying to copy the
>> exact same phases that exist for vacuum instead of thinking about what
>> makes sense for ANALYZE.
>
> Understood. I have removed this phase.
>>
>>
>> +/* Report number of heap blocks scaned so far*/
>> +pgstat_progress_update_param(PROGRESS_ANALYZE_HEAP_BLKS_SCANNED,
>> targblock);
>>
>> While I don't think this is necessarily a bad thing to report, I find
>> it very surprising that you're not reporting what seems to me like the
>> most useful thing here - namely, the number of blocks that will be
>> sampled in total and the number of those that you've selected.
>> Instead, you're just reporting the length of the relation and the
>> last-sampled block, which is a less-accurate guide to total progress.
>> There are enough counters to report both things, so maybe we should.
>>
>> +/* Report total number of sample rows*/
>> +pgstat_progress_update_param(PROGRESS_ANALYZE_TOTAL_SAMPLE_ROWS,
>> numrows);
>>
>> As an alternative to the previous paragraph, yet another thing you
>> could report is number of rows sampled out of total rows to be
>> sampled.  But this isn't the way to do it: you are reporting the
>> number of rows you sampled only once you've finished sampling.  The
>> point of progress reporting is to report progress -- that is, to
>> report this value as it's updated, not just to report it when ANALYZE
>> is practically done and the value has reached its maximum.
>
> Understood.
> I have reported number of rows sampled out of total rows to be sampled.
> I have not reported the number of blocks that will be sampled in total.
> So currect pg_stat_progress_analyze view looks like:
>
> =# \d+ pg_stat_progress_analyze
>  

Re: [HACKERS] wait events for disk I/O

2017-03-18 Thread Robert Haas
On Fri, Mar 17, 2017 at 10:01 AM, Rushabh Lathia
 wrote:
> I tried to cover all the suggestion in the attached latest patch.

Committed.  I reworded the documentation entries, renamed a few of the
wait events to make things more consistent, put all three lists in
rigorous alphabetical order, and I fixed a couple of places where an
error return from a system call could lead to returning without
clearing the wait event.

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


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


Re: [HACKERS] pageinspect and hash indexes

2017-03-18 Thread Ashutosh Sharma
On Sat, Mar 18, 2017 at 1:34 PM, Amit Kapila  wrote:
> On Sat, Mar 18, 2017 at 12:12 AM, Ashutosh Sharma  
> wrote:
>> On Fri, Mar 17, 2017 at 10:54 PM, Jeff Janes  wrote:
>>> While trying to figure out some bloating in the newly logged hash indexes,
>>> I'm looking into the type of each page in the index.  But I get an error:
>>>
>>> psql -p 9876 -c "select hash_page_type(get_raw_page('foo_index_idx',x)) from
>>> generate_series(1650,1650) f(x)"
>>>
>>> ERROR:  page is not a hash page
>>> DETAIL:  Expected ff80, got .
>>>
>>> The contents of the page are:
>>>
>>> \xa400d8f203bf65c91800f01ff01f0420...
>>>
>>> (where the elided characters at the end are all zero)
>>>
>>> What kind of page is that actually?
>>
>> it is basically either a newly allocated bucket page or a freed overflow 
>> page.
>>
>
> What makes you think that it can be a newly allocated page?
> Basically, we always initialize the special space of newly allocated
> page, so not sure what makes you deduce that it can be newly allocated
> page.

I came to know this from the following experiment.

I  created a hash index and kept on inserting data in it till the split happens.

When split happened, I could see following values for firstblock and
lastblock in _hash_alloc_buckets()

Breakpoint 1, _hash_alloc_buckets (rel=0x7f6ac951ee30, firstblock=34,
nblocks=32) at hashpage.c:993
(gdb) n
(gdb) pfirstblock
$15 = 34
(gdb) pnblocks
$16 = 32
(gdb) n
(gdb) plastblock
$17 = 65

AFAIU, this bucket split resulted in creation of new bucket pages from
block number 34 to 65.

The contents for metap are as follows,

(gdb) p*metap
$18 = {hashm_magic = 105121344,hashm_version = 2, hashm_ntuples =
2593, hashm_ffactor = 81, hashm_bsize = 8152, hashm_bmsize = 4096,
hashm_bmshift = 15,
  hashm_maxbucket = 32,hashm_highmask = 63, hashm_lowmask = 31,
hashm_ovflpoint = 6, hashm_firstfree = 0, hashm_nmaps = 1,
hashm_procid = 450,
  hashm_spares = {0, 0,0, 0, 0, 1, 1, 0 },
hashm_mapp = {33,0 }}

Now, if i try to check the page type for block number 65, this is what i see,

test=# select * from hash_page_type(get_raw_page('con_hash_index', 65));
ERROR:  page is not a hash page
DETAIL:  Expected ff80, got .
test=#

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


-- 
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] Two phase commit in ECPG

2017-03-18 Thread Michael Meskes
> Yes. I added two-phase commit test to "make check" test schedule
> while
> adding new two type of test.

Thank you, committed.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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 to improve performance of replay of AccessExclusiveLock

2017-03-18 Thread Simon Riggs
On 16 March 2017 at 09:52, David Rowley  wrote:

>> Seemed easier to write it than explain further. Please see what you think.
>
>
> Thanks. I had been looking for some struct to store the flag in. I'd not
> considered just adding a new global variable.

As Amit says, I don't see the gain from adding that to each xact state.

I'd suggest refactoring my patch so that the existign
MyXactAccessedTempRel becomes MyXactFlags and we can just set a flag
in the two cases (temp rels and has-aels). That way we still have one
Global but it doesn't get any uglier than it already is.

-- 
Simon Riggshttp://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] Patch to improve performance of replay of AccessExclusiveLock

2017-03-18 Thread Simon Riggs
On 16 March 2017 at 19:08, Amit Kapila  wrote:
> On Thu, Mar 16, 2017 at 6:01 AM, Simon Riggs  wrote:
>> On 8 March 2017 at 10:02, David Rowley  wrote:
>>> On 8 March 2017 at 01:13, Simon Riggs  wrote:
 Don't understand this. I'm talking about setting a flag on
 commit/abort WAL records, like the attached.
>>>
>>> There's nothing setting a flag in the attached. I only see the
>>> checking of the flag.
>>
>> Yeh, it wasn't a full patch, just a way marker to the summit for you.
>>
 We just need to track info so we can set the flag at EOXact and we're done.
>>>
>>> Do you have an idea where to store that information? I'd assume we'd
>>> want to store something during LogAccessExclusiveLocks() and check
>>> that in XactLogCommitRecord() and XactLogAbortRecord(). I don't see
>>> anything existing which might help with that today. Do you think I
>>> should introduce some new global variable for that? Or do you propose
>>> calling GetRunningTransactionLocks() again while generating the
>>> Commit/Abort record?
>>
>> Seemed easier to write it than explain further. Please see what you think.
>>
>
> @@ -5563,7 +5575,8 @@ xact_redo_abort(xl_xact_parsed_abort *parsed,
> TransactionId xid)
>   /*
>   * Release locks, if any. There are no invalidations to send.
>   */
> - StandbyReleaseLockTree(xid, parsed->nsubxacts, parsed->subxacts);
> + if (parsed->xinfo & XACT_XINFO_HAS_AE_LOCKS)
> + StandbyReleaseLockTree(xid, parsed->nsubxacts, parsed->subxacts);
>
>
> The patch uses XACT_XINFO_HAS_AE_LOCKS during abort replay, but during
> normal operation (XactLogAbortRecord()), it doesn't seem to log the
> information.  Is this an oversight or do you have some reason for
> doing so?

Good catch, thanks. Yes, I changed my mind on whether it was needed
during implementation.

-- 
Simon Riggshttp://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] Patch to improve performance of replay of AccessExclusiveLock

2017-03-18 Thread Simon Riggs
On 16 March 2017 at 09:52, David Rowley  wrote:

> I was just trying to verify if it was going to do the correct thing in
> regards to subtransactions and I got a little confused at the comments at
> the top of StandbyAcquireAccessExclusiveLock():
>
>  * We record the lock against the top-level xid, rather than individual
>  * subtransaction xids. This means AccessExclusiveLocks held by aborted
>  * subtransactions are not released as early as possible on standbys.
>
> if this is true, and it seems to be per LogAccessExclusiveLock(), does
> StandbyReleaseLockTree() need to release the locks for sub transactions too?
>
> This code:
>
> for (i = 0; i < nsubxids; i++)
> StandbyReleaseLocks(subxids[i]);

Yes, you are correct, good catch. It's not broken, but the code does
nothing, slowly.

We have two choices... remove this code or assign locks against subxids.

1. In xact_redo_abort() we can just pass NULL to
StandbyReleaseLockTree(), keeping the other code as is for later
fixing by another approach.

2. In LogAccessExclusiveLock() we can use GetCurrentTransactionId()
rather than GetTopTransactionId(), so that we assign the lock to the
subxid rather than the top xid. That could increase lock traffic, but
less likely. It also solves the problem of early release when AELs
held by subxids.

(2) looks safe enough, so patch attached.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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


Re: [HACKERS] Microvacuum support for Hash Index

2017-03-18 Thread Amit Kapila
On Fri, Mar 17, 2017 at 8:34 PM, Ashutosh Sharma  wrote:
> On Fri, Mar 17, 2017 at 6:13 PM, Amit Kapila  wrote:
>> On Fri, Mar 17, 2017 at 12:27 PM, Ashutosh Sharma  
>> wrote:
>>> On Fri, Mar 17, 2017 at 8:20 AM, Amit Kapila  
>>> wrote:
>>>
>>> As I said in my previous e-mail, I think you need
 to record clearing of this flag in WAL record XLOG_HASH_DELETE as you
 are not doing this unconditionally and then during replay clear it
 only when the WAL record indicates the same.
>>>
>>> Thank you so much for putting that point. I too think that we should
>>> record the flag status in the WAL record and clear it only when
>>> required during replay.
>>>
>>
>> I think hashdesc.c needs an update (refer case XLOG_HASH_DELETE:).
>
> Done. Thanks!
>

This version looks good to me.

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


-- 
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] pageinspect and hash indexes

2017-03-18 Thread Amit Kapila
On Sat, Mar 18, 2017 at 12:12 AM, Ashutosh Sharma  wrote:
> On Fri, Mar 17, 2017 at 10:54 PM, Jeff Janes  wrote:
>> While trying to figure out some bloating in the newly logged hash indexes,
>> I'm looking into the type of each page in the index.  But I get an error:
>>
>> psql -p 9876 -c "select hash_page_type(get_raw_page('foo_index_idx',x)) from
>> generate_series(1650,1650) f(x)"
>>
>> ERROR:  page is not a hash page
>> DETAIL:  Expected ff80, got .
>>
>> The contents of the page are:
>>
>> \xa400d8f203bf65c91800f01ff01f0420...
>>
>> (where the elided characters at the end are all zero)
>>
>> What kind of page is that actually?
>
> it is basically either a newly allocated bucket page or a freed overflow page.
>

What makes you think that it can be a newly allocated page?
Basically, we always initialize the special space of newly allocated
page, so not sure what makes you deduce that it can be newly allocated
page.


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


-- 
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] pageinspect and hash indexes

2017-03-18 Thread Amit Kapila
On Sat, Mar 18, 2017 at 1:42 AM, Tom Lane  wrote:
> Ashutosh Sharma  writes:
>> Basically, when we started working on WAL for hash index, we found
>> that WAL routine 'XLogReadBufferExtended' does not expect a page to be
>> completely zero page else it returns Invalid Buffer. To fix this, we
>> started initializing freed overflow page or new bucket pages using
>> _hash_pageinit() which basically initialises page header portion but
>> not it's special area where page type information is present. That's
>> why you are seeing an ERROR saying 'page is not a hash page'. Actually
>> pageinspect module needs to handle this type of page. Currently it is
>> just handling zero pages but not an empty pages. I will submit a patch
>> for this.
>
> That seems like entirely the wrong approach.  You should make the special
> space valid, instead, so that tools like pg_filedump can make sense of
> the page.
>

We were not aware that external tools like pg_filedump are dependent
on special space of index, but after looking at the code of
pg_filedump, I agree with you that we need to initialize the special
space in this case (free overflow page).  I think we can mark such a
page type as LH_UNUSED_PAGE  and then initialize the other fields of
special space.  Nonetheless, I think we still need modifications in
hashfuncs.c so that it can understand this type of page.


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


-- 
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 v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

2017-03-18 Thread Michael Paquier
On Sat, Mar 18, 2017 at 2:56 AM, Elvis Pranskevichus  wrote:
> Currently, clients wishing to know when the server exits hot standby
> have to resort to polling, which is often suboptimal.
>
> This adds the new "in_hot_standby" GUC variable that is reported via a
> ParameterStatus message.  This allows the clients to:
>
>(a) know right away that they are connected to a server in hot
>standby; and
>
>(b) know immediately when a server exits hot standby.
>
> This change will be most beneficial to various connection pooling
> systems (pgpool etc.)

Why adding a good chunk of code instead of using pg_is_in_recovery(),
which switches to false once a server exits recovery?
-- 
Michael


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