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

[Ashutosh Bapat]

On Tue, Mar 22, 2016 at 8:03 AM, Etsuro Fujita 
wrote:

> On 2016/03/19 4:51, Robert Haas wrote:
>
>> On Thu, Mar 17, 2016 at 7:00 AM, Etsuro Fujita
>>  wrote:
>>
>>> So, I'd like to propose: (1) when tableoids are
>>> requested from the remote server, postgres_fdw sets valid values for
>>> them locally, instead (core should support that?)
>>>
>>
> Sure.
>>
>
> and (2) when any of
>>> xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
>>> pushing down foreign joins.  (We might be able to set appropriate values
>>> for them locally the same way as for tableoids, but I'm not sure it's
>>> worth complicating the code.)  I think that would be probably OK,
>>> because users wouldn't retrieve any such columns in practice.
>>>
>>
> Now that seems like the wrong reaction.  I mean, aren't these just
>> going to be 0 or something?  Refusing to push the join down seems
>> strange.
>>
>
> OK, I'll modify the patch so that the join is pushed down even if any of
> xmins, xmaxs, cmins, and cmaxs are requested.  Do you think which one
> should set values for these as well as tableoids, postgres_fdw or core?
>

Earlier in this mail chain, I suggested that the core should take care of
storing the values for these columns. But instead, I think, core should
provide functions which can be used by FDWs, if they want, to return values
for those columns. Something like Datum
get_syscol_value(RelOptInfo/Relation, attno). The function will return
Datum 0 for most of the columns and table's OID for tableoid. My 0.02.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] multivariate statistics v14

[Jeff Janes]

On Sun, Mar 20, 2016 at 4:34 PM, Tomas Vondra
 wrote:
>
>
> D'oh. Thanks for reporting. Attached is v16, hopefully fixing the few
> remaining whitespace issues.

Hi Tomas,

I'm trying out v16 against a common problem, where postgresql thinks
it is likely top stop early during a "order by (index express) limit
1" but it doesn't actually stop early due to cross-column
correlations.  But the multivariate statistics don't seem to help.  Am
I doing this wrong, or just expecting too much?


jjanes=# create table foo as select x, floor(x/(1000/500))::int as
y  from generate_series(1,1000) f(x);
jjanes=# create index on foo (x,y);
jjanes=# create index on foo (y,x);
jjanes=# create statistics jjj on foo (x,y) with (dependencies,histogram);
jjanes=# vacuum analyze ;


jjanes=# explain (analyze, timing off)  select x from foo where y
between 478 and 480 order by x limit 1;
QUERY PLAN
---
 Limit  (cost=0.43..4.92 rows=1 width=4) (actual rows=1 loops=1)
   ->  Index Only Scan using foo_x_y_idx on foo  (cost=0.43..210156.55
rows=46812 width=4) (actual rows=1 loops=1)
 Index Cond: ((y >= 478) AND (y <= 480))
 Heap Fetches: 0
 Planning time: 0.311 ms
 Execution time: 478.917 ms

Here is walks up the index on x, until it meets the first row meeting
the qualification on y. It thinks it will get to stop early and be
very fast, but it doesn't.

If I add an dummy addition to the ORDER BY, to force it not to talk
the index, I get a plan which uses the other index and is actually
much faster, but is planned to be several hundred times slower:


jjanes=# explain (analyze, timing off)  select x from foo where y
between 478 and 480 order by x+0 limit 1;
QUERY PLAN
---
 Limit  (cost=1803.77..1803.77 rows=1 width=8) (actual rows=1 loops=1)
   ->  Sort  (cost=1803.77..1920.80 rows=46812 width=8) (actual rows=1 loops=1)
 Sort Key: ((x + 0))
 Sort Method: top-N heapsort  Memory: 25kB
 ->  Index Only Scan using foo_y_x_idx on foo
(cost=0.43..1569.70 rows=46812 width=8) (actual rows=6 loops=1)
   Index Cond: ((y >= 478) AND (y <= 480))
   Heap Fetches: 0
 Planning time: 0.175 ms
 Execution time: 20.264 ms

(I use the "timing off" option, because without it the second plan
spends most of its time calling "gettimeofday")

Cheers,

Jeff


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

Re: [HACKERS] Parallel Aggregate

[James Sewell]

Good news!

On Tuesday, 22 March 2016, David Rowley 
wrote:

> On 22 March 2016 at 02:35, Robert Haas  > wrote:
> > I have committed this after changing some of the comments.
> >
> > There might still be bugs ... but I don't see them.  And the speedups
> > look very impressive.
> >
> > Really nice work, David.
>
> Thanks for that, and thank you for taking the time to carefully review
> it and commit it.
>
> --
>  David Rowley   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
>


-- 

James Sewell,
PostgreSQL Team Lead / Solutions Architect
__


Level 2, 50 Queen St, Melbourne VIC 3000

*P *(+61) 3 8370 8000  *W* www.lisasoft.com  *F *(+61) 3 8370 8099

-- 


--
The contents of this email are confidential and may be subject to legal or 
professional privilege and copyright. No representation is made that this 
email is free of viruses or other defects. If you have received this 
communication in error, you may not copy or distribute any part of it or 
otherwise disclose its contents to anyone. Please advise the sender of your 
incorrect receipt of this correspondence.

Re: [HACKERS] parallel aggregation - Numeric is unsupported?

[Pavel Stehule]

2016-03-22 6:28 GMT+01:00 David Rowley :

>
> On 22/03/2016 5:24 pm, "Pavel Stehule"  wrote:
> >
> > Hi
> >
> > is it expected in this moment?
> >
> >
> >   Table "public.foo"
> >  Column │  Type   │ Modifiers
> > ╪═╪═══
> >  a  │ integer │
> >
> > postgres=# \d foo2
> >  Table "public.foo2"
> >  Column │  Type   │ Modifiers
> > ╪═╪═══
> >  a  │ numeric │
> >
> > postgres=# explain select sum(a) from foo;
> >QUERY
> PLAN
> >
> 
> >  Finalize Aggregate  (cost=76498.35..76498.36 rows=1 width=8)
> >->  Gather  (cost=76497.93..76498.34 rows=4 width=8)
> >  Number of Workers: 4
> >  ->  Partial Aggregate  (cost=75497.93..75497.94 rows=1 width=8)
> >->  Parallel Seq Scan on foo  (cost=0.00..69247.94
> rows=244 width=4)
> > (5 rows)
> >
> > postgres=# explain select sum(a) from foo2;
> >   QUERY PLAN
> > ══
> >  Aggregate  (cost=179053.25..179053.26 rows=1 width=32)
> >->  Seq Scan on foo2  (cost=0.00..154053.60 rows=860 width=12)
> > (2 rows)
>
> Yes. There's still a few outstanding patches to add support to serialize
> internal states. See the combine aggregates thread.
>
I'll wait :), thank you for info

Regards

Pavel

Re: [HACKERS] parallel aggregation - Numeric is unsupported?

[David Rowley]

On 22/03/2016 5:24 pm, "Pavel Stehule"  wrote:
>
> Hi
>
> is it expected in this moment?
>
>
>   Table "public.foo"
>  Column │  Type   │ Modifiers
> ╪═╪═══
>  a  │ integer │
>
> postgres=# \d foo2
>  Table "public.foo2"
>  Column │  Type   │ Modifiers
> ╪═╪═══
>  a  │ numeric │
>
> postgres=# explain select sum(a) from foo;
>QUERY
PLAN
>

>  Finalize Aggregate  (cost=76498.35..76498.36 rows=1 width=8)
>->  Gather  (cost=76497.93..76498.34 rows=4 width=8)
>  Number of Workers: 4
>  ->  Partial Aggregate  (cost=75497.93..75497.94 rows=1 width=8)
>->  Parallel Seq Scan on foo  (cost=0.00..69247.94
rows=244 width=4)
> (5 rows)
>
> postgres=# explain select sum(a) from foo2;
>   QUERY PLAN
> ══
>  Aggregate  (cost=179053.25..179053.26 rows=1 width=32)
>->  Seq Scan on foo2  (cost=0.00..154053.60 rows=860 width=12)
> (2 rows)

Yes. There's still a few outstanding patches to add support to serialize
internal states. See the combine aggregates thread.

Re: [HACKERS] parallel aggregation - Numeric is unsupported?

[David Rowley]

On 22/03/2016 5:24 pm, "Pavel Stehule"  wrote:
>
> Hi
>
> is it expected in this moment?
>
>
>   Table "public.foo"
>  Column │  Type   │ Modifiers
> ╪═╪═══
>  a  │ integer │
>
> postgres=# \d foo2
>  Table "public.foo2"
>  Column │  Type   │ Modifiers
> ╪═╪═══
>  a  │ numeric │
>
> postgres=# explain select sum(a) from foo;
>QUERY
PLAN
>

>  Finalize Aggregate  (cost=76498.35..76498.36 rows=1 width=8)
>->  Gather  (cost=76497.93..76498.34 rows=4 width=8)
>  Number of Workers: 4
>  ->  Partial Aggregate  (cost=75497.93..75497.94 rows=1 width=8)
>->  Parallel Seq Scan on foo  (cost=0.00..69247.94
rows=244 width=4)
> (5 rows)
>
> postgres=# explain select sum(a) from foo2;
>   QUERY PLAN
> ══
>  Aggregate  (cost=179053.25..179053.26 rows=1 width=32)
>->  Seq Scan on foo2  (cost=0.00..154053.60 rows=860 width=12)
> (2 rows)

Yes. There's still a few outstanding patches to add support to serialize
internal states. See the combine aggregates thread.

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

[Pavel Stehule]

2016-03-22 6:06 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > I can live with SELECT fx(x). It is little bit dangerous, but this risk
> can
> > be easy detected by plpgsql_check.
>
> Dangerous how?
>

I afraid of useless and forgotten call of functions. But the risk is same
like PERFORM - so this is valid from one half. The PERFORM statement holds
special semantic, and it is interesting.

But I don't see any risk if we allow SELECT fx(x) without INTO when fx is
void function. It is absolutely correct.



>
> >> So, I'm -1 on not having any keyword at all.  I have no objection
> >> to Merlin's proposal though.  I agree that PERFORM is starting to
> >> look a bit silly, since it doesn't play with WITH for instance.
>
> > Isn't time to fix PERFORM instead?
>
> I do not think it can be fixed without embedding knowledge of PERFORM into
> the core parser, which I doubt anybody would consider a good idea.
>

I don't see, why PERFORM should be in core parser? What use case should be
fixed?

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] OOM in libpq and infinite loop with getCopyStart()

[Amit Kapila]

On Tue, Mar 22, 2016 at 9:46 AM, Tom Lane  wrote:
>
> Amit Kapila  writes:
> > On Mon, Mar 21, 2016 at 10:13 PM, Robert Haas 
wrote:
> >> It is very difficult to believe that this is a good idea:
> >>
> >> --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
> >> +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
> >> @@ -445,6 +445,7 @@ libpqrcv_PQexec(const char *query)
> >>  if (PQresultStatus(lastResult) == PGRES_COPY_IN ||
> >>  PQresultStatus(lastResult) == PGRES_COPY_OUT ||
> >>  PQresultStatus(lastResult) == PGRES_COPY_BOTH ||
> >> +PQresultStatus(lastResult) == PGRES_FATAL_ERROR ||
> >>  PQstatus(streamConn) == CONNECTION_BAD)
> >> break;
> >>
> >> I mean, why would it be a good idea to blindly skip over fatal errors?
>
> > I think it is not about skipping the FATAL error, rather to stop trying
to
> > get further results on FATAL error.
>
> If the code already includes "lost the connection" as a case to break on,
> I'm not quite sure why "got a query error" is not.
>

This error check is exactly same as PQexecFinish() and there some
explanation is given in comments which hints towards the reason for
continuing on error, basically both the functions PQexecFinish()
and libpqrcv_PQexec() returns the last result if there are many
and PQexecFinish() concatenates the errors as well in some cases.  Do we
see any use in continuing to get result after getting PGRES_FATAL_ERROR
error?


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

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

[Tom Lane]

Pavel Stehule  writes:
> I can live with SELECT fx(x). It is little bit dangerous, but this risk can
> be easy detected by plpgsql_check.

Dangerous how?

>> So, I'm -1 on not having any keyword at all.  I have no objection
>> to Merlin's proposal though.  I agree that PERFORM is starting to
>> look a bit silly, since it doesn't play with WITH for instance.

> Isn't time to fix PERFORM instead?

I do not think it can be fixed without embedding knowledge of PERFORM into
the core parser, which I doubt anybody would consider a good idea.

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] Identifying a message in emit_log_hook.

[Kyotaro HORIGUCHI]

I found that this has been commited.
Thank you for committing this, Simon.

regards,

At Tue, 15 Mar 2016 12:22:05 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160315.122205.08265186.horiguchi.kyot...@lab.ntt.co.jp>
> Thnak you for scooping up this.
> 
> At Thu, 10 Mar 2016 08:14:09 -0500, David Steele  wrote 
> in <56e17321.5050...@pgmasters.net>
> > Hi Simon,
> > 
> > On 3/10/16 7:26 AM, Simon Riggs wrote:
> > >
> > > Can you add this to the CF? It was submitted before deadline.
> > > I presume you have access to do that?
> > 
> > No problem - here it is:
> > 
> > https://commitfest.postgresql.org/9/576/
> 
> Some kind of hash code can be added for shotrcut, but this is
> usable even after it is added.
> 
> One arguable point I see now on this is only ids for the message
> type "message" would be enough, or needed for some other types
> such as "details". This is quite straightforward so I see no
> other arguable point other than the code itself.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Dilip Kumar]

On Tue, Mar 22, 2016 at 12:31 PM, Dilip Kumar  wrote:

> ! pg_atomic_write_u32(>state, state);
>   } while (!StartBufferIO(bufHdr, true));
>
> Better Write some comment, about we clearing the BM_LOCKED from stage
> directly and need not to call UnlockBufHdr explicitly.
> otherwise its confusing.
>

Few more comments..

*** 828,837 
*/
   do
   {
!  LockBufHdr(bufHdr);
*!  Assert(bufHdr->flags & BM_VALID);*
!  bufHdr->flags &= ~BM_VALID;
!  UnlockBufHdr(bufHdr);
   } while (!StartBufferIO(bufHdr, true));
   }
   }
--- 826,834 
*/
   do
   {
!  uint32 state = LockBufHdr(bufHdr);
!  state &= ~(BM_VALID | BM_LOCKED);
!  pg_atomic_write_u32(>state, state);
   } while (!StartBufferIO(bufHdr, true));

1. Previously there was a Assert, any reason why we removed it ?
 Assert(bufHdr->flags & BM_VALID);

2. And also if we don't need assert then can't we directly clear BM_VALID
flag from state variable (if not locked) like pin/unpin buffer does,
without taking buffer header lock?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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

[Amit Kapila]

On Tue, Mar 22, 2016 at 9:13 AM, Haribabu Kommi 
wrote:
>
> On Tue, Mar 22, 2016 at 2:19 PM, Amit Kapila 
wrote:
> > On Mon, Mar 21, 2016 at 6:16 PM, Haribabu Kommi <
kommi.harib...@gmail.com>
> > wrote:
> >>
> >> On Mon, Mar 14, 2016 at 4:51 PM, Amit Kapila 
> >> wrote:
> >> >> Operating system - windows 7
> >> >> Binary - PostgreSQL 9.5 (This doesn't matter, 9.4+ can produce the
> >> >> problem)
> >> >>
> >> >> 1. Create two standard users in the system (test_user1 and
test_user2)
> >> >> 2. Create two databases belongs each user listed above.
> >> >> 3. Now using pg_ctl register the services for the two users.
> >> >> 4. Provide logon permissions to these users to run the services by
> >> >> changing
> >> >> service properties.
> >> >
> >> > Did you mean to say that you changed Log on as: Local System Account
in
> >> > service properties or something else?
> >>
> >> No. Not as local service. The user should be the new standard user
> >> that is created
> >> in the system.
> >>
> >
> > So what do you exactly mean by "Provide logon permissions to these
users",
> > can you describe in detail what exactly you have done to give those
> > permissions.  If I try to do with a new user, it gives me error "could
not
> > open service manager"  at start of service.
>
> 1. Start the cmd with administrator user and add the new postgresql
service
> with a standard user that is created.
> 2. Start the services window with the user having administrator
privileges and
> go to the corresponding added service.
> 3. Right click on the service provides an properties option.
> 4. In the properties, there is an logon tab. Click it
> 5. Provide the password for the new user that is used for creating the
service.
> 6. This adds the user to log on permissions.
>

I am also able to reproduce the issue with these steps.

> >>
> >> Yes, same random number generation is not the problem. In windows apart
> >> from EEXIST error, EACCES also needs to be validated and returned for
> >> new random number generation, instead of throwing an error.
> >>
> >
> > Doing the same handling for EACCES doesn't seem to be sane because if
EACCES
> > came for reason other than duplicate dsm name, then we want to report
the
> > error instead of trying to regenerate the name.  I think here fix
should be
> > to append data_dir path as we do for main shared memory.
>
> Yes, EACCES may be possible other than duplicate dsm name.
>

So as far as I can see there are two ways to resolve this issue, one is to
retry generation of dsm name if CreateFileMapping returns EACCES and second
is to append data_dir name to dsm name as the same is done for main shared
memory, that will avoid the error to occur.  First approach has minor flaw
that if CreateFileMapping returns EACCES due to reason other then duplicate
dsm name which I am not sure is possible to identify, then we should report
error instead try to regenerate the name

Robert and or others, can you share your opinion on what is the best way to
proceed for this issue.

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

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

[Craig Ringer]

On 21 March 2016 at 20:46, Haribabu Kommi  wrote:


>
> No. Not as local service. The user should be the new standard user
> that is created
> in the system.
>

Which was done how, exactly?

Commands run? Steps taken?

PostgreSQL drops privileges once it starts, so it's actually pretty OK to
run it as an admin user, NetworkService, etc.

Otherwise you should really make a service user, not a regular user
account. Don't allow the account to log in interactively, do allow it to
log in as a service. Don't make it a domain account unless you need domain
integration for SSPI etc.

The best option on newer Windows should be to use a managed service account
(https://technet.microsoft.com/en-us/library/ff641731(v=ws.10).aspx) or
virtual account. Eventually the installer should switch to doing that
automatically instead of using NetworkService.


> >> 5. Now try to start the services, the second service fails with the
> >> error message.
> >> 6. Error details can be found out in Event log viewer.
>

Can you get a Process Monitor trace of startup and check exactly where it's
getting access denied, doing what?

You may have to dig through a *lot* of output to find it.

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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Dilip Kumar]

On Sun, Mar 20, 2016 at 4:10 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> Actually, we behave like old code and do such modifications without
> increasing number of atomic operations.  We can just calculate new value of
> state (including unset of BM_LOCKED flag) and write it to the buffer.  In
> this case we don't require more atomic operations.  However, it becomes not
> safe to use atomic decrement in UnpinBuffer().  We can use there loop of
> CAS which wait buffer header to be unlocked like PinBuffer() does.  I'm not
> sure if this affects multicore scalability, but I think it worth trying.
>
> So, this idea is implemented in attached patch.  Please, try it for both
> regression on lower number of clients and scalability on large number of
> clients.
>
>
Good, seems like we have reduced some instructions, I will test it soon.


> Other changes in this patch:
> 1) PinBuffer() and UnpinBuffer() use exponential backoff in spindealy
> like LockBufHdr() does.
> 2) Previous patch contained revert
> of ac1d7945f866b1928c2554c0f80fd52d7f92.  This was not intentional.
> No, it doesn't contains this revert.
>


Some other comments..

*** ReadBuffer_common(SMgrRelation smgr, cha
*** 828,837 
  */
  do
  {
! LockBufHdr(bufHdr);
! Assert(bufHdr->flags & BM_VALID);
! bufHdr->flags &= ~BM_VALID;
! UnlockBufHdr(bufHdr);
  } while (!StartBufferIO(bufHdr, true));
  }
  }
--- 826,834 
  */
  do
  {
! uint32 state = LockBufHdr(bufHdr);
! state &= ~(BM_VALID | BM_LOCKED);
! pg_atomic_write_u32(>state, state);
  } while (!StartBufferIO(bufHdr, true));

Better Write some comment, about we clearing the BM_LOCKED from stage
directly and need not to call UnlockBufHdr explicitly.
otherwise its confusing.




-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

[HACKERS] parallel aggregation - Numeric is unsupported?

[Pavel Stehule]

Hi

is it expected in this moment?


  Table "public.foo"
 Column │  Type   │ Modifiers
╪═╪═══
 a  │ integer │

postgres=# \d foo2
 Table "public.foo2"
 Column │  Type   │ Modifiers
╪═╪═══
 a  │ numeric │

postgres=# explain select sum(a) from foo;
   QUERY
PLAN

 Finalize Aggregate  (cost=76498.35..76498.36 rows=1 width=8)
   ->  Gather  (cost=76497.93..76498.34 rows=4 width=8)
 Number of Workers: 4
 ->  Partial Aggregate  (cost=75497.93..75497.94 rows=1 width=8)
   ->  Parallel Seq Scan on foo  (cost=0.00..69247.94
rows=244 width=4)
(5 rows)

postgres=# explain select sum(a) from foo2;
  QUERY PLAN
══
 Aggregate  (cost=179053.25..179053.26 rows=1 width=32)
   ->  Seq Scan on foo2  (cost=0.00..154053.60 rows=860 width=12)
(2 rows)

Regards

Pavel

p.s. It great step forward - it looks pretty well

Re: [HACKERS] OOM in libpq and infinite loop with getCopyStart()

[Tom Lane]

Amit Kapila  writes:
> On Mon, Mar 21, 2016 at 10:13 PM, Robert Haas  wrote:
>> It is very difficult to believe that this is a good idea:
>> 
>> --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>> +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>> @@ -445,6 +445,7 @@ libpqrcv_PQexec(const char *query)
>>  if (PQresultStatus(lastResult) == PGRES_COPY_IN ||
>>  PQresultStatus(lastResult) == PGRES_COPY_OUT ||
>>  PQresultStatus(lastResult) == PGRES_COPY_BOTH ||
>> +PQresultStatus(lastResult) == PGRES_FATAL_ERROR ||
>>  PQstatus(streamConn) == CONNECTION_BAD)
>> break;
>> 
>> I mean, why would it be a good idea to blindly skip over fatal errors?

> I think it is not about skipping the FATAL error, rather to stop trying to
> get further results on FATAL error.

If the code already includes "lost the connection" as a case to break on,
I'm not quite sure why "got a query error" is not.  Maybe that PQstatus
check is broken too, but it doesn't seem like this patch makes it more so.

regards, tom lane


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

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

[Pavel Stehule]

2016-03-21 23:49 GMT+01:00 Tom Lane :

> Jim Nasby  writes:
> > On 3/21/16 5:03 PM, Merlin Moncure wrote:
> >> in Oracle, you'd simply do:
> >> LogIt('I did something');
>
> > It would be *great* if we could support that in plpgsql.
>
> FWIW, I'm hesitant to just start accepting that syntax as if it were an
> equivalent to "SELECT f(x)" or "PERFORM f(x)".  I think that someday
> we will want to have the ability to have pass-by-reference parameters
> to functions, and that's a fundamentally incompatible behavior so it
> ought to be invoked by a new syntax.  I'd like to save "f(x)" as a
> bare statement for that purpose.  We could also consider inventing
> "CALL f(x)"; but supposing that we already had both "CALL f(x)" (which
> would allow x to be pass-by-ref and possibly modified) and "SELECT f(x)"
> (which would not), which one would you assign bare "f(x)" to mean?  It
> should be "CALL f(x)", not least because that would be the semantics most
> comparable to Oracle's behavior (since they have pass-by-ref parameters
> already).
>

I can live with SELECT fx(x). It is little bit dangerous, but this risk can
be easy detected by plpgsql_check.


>
> So, I'm -1 on not having any keyword at all.  I have no objection
> to Merlin's proposal though.  I agree that PERFORM is starting to
> look a bit silly, since it doesn't play with WITH for instance.
>

Isn't time to fix PERFORM instead?



>
> > While we're on the subject, it'd be great if variable := SELECT ...
> > worked too.
>
> It does, though you might need parens around the sub-select.
>
> regards, tom lane
>

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

[Pavel Stehule]

2016-03-21 23:26 GMT+01:00 Jim Nasby :

> On 3/21/16 5:03 PM, Merlin Moncure wrote:
>
>> in Oracle, you'd simply do:
>> LogIt('I did something');
>>
>
> It would be *great* if we could support that in plpgsql.
>
> I'm not sure what Oracle does for SELECT statements without INTO/BULK
>> UPDATE.  I'm not really inclined to care -- I'm really curious to see
>> an argument where usage of PERFORM actually helps in some meaningful
>> way.  Notably, SELECT without INTO is accepted syntax, but fails only
>> after running the query.  I think that's pretty much stupid but it's
>> fair to say I'm not inventing syntax, only disabling the error.
>>
>
> I don't think it buys much at all.
>
> While we're on the subject, it'd be great if variable := SELECT ... worked
> too.
>

We are support

var := (query expression)

and this syntax is required and supported by ANSI/SQL - there are no any
reason to support other proprietary extension.

Regards

Pavel

>
> I'm not sure what other databases do is relevant.   They use other
>> procedure languages than pl//sql (the biggest players are pl/psm and
>> t-sql) which have a different set of rules in terms of passing
>> variables in and out of queries.
>>
>
> +1
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

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

[Pavel Stehule]

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

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

PERFORM is not interesting - it is proprietary extension.


>
> > Can you start with analyze what is possible, and what semantic is
> allowed in
> > standard and other well known SQL databases?
>
> Typical use of PERFORM is void returning function.  Oracle allows use
> of those functions without any decoration at all.   For example, in
> postgres we might do:
> PERFORM LogIt('I did something');
>
> in Oracle, you'd simply do:
> LogIt('I did something');
>

It is procedure call - it is not SELECT fx(), but CALL fx(), when CALL
statement is implicit.


>
> I'm not sure what Oracle does for SELECT statements without INTO/BULK
> UPDATE.  I'm not really inclined to care -- I'm really curious to see
> an argument where usage of PERFORM actually helps in some meaningful
> way.  Notably, SELECT without INTO is accepted syntax, but fails only
> after running the query.  I think that's pretty much stupid but it's
> fair to say I'm not inventing syntax, only disabling the error.


> I'm not sure what other databases do is relevant.   They use other
> procedure languages than pl//sql (the biggest players are pl/psm and
> t-sql) which have a different set of rules in terms of passing
> variables in and out of queries.
>

But this is important, and you are ignoring this case. If we allow "SELECT
expr;" when result will be ignored once, we cannot to revert it back ever.
So this can be really issue, when you will port applications between
Postgres and other, or if you will switch between Postgres and other db.

Regards

Pavel


>
> merlin
>

Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages

[Peter Geoghegan]

On Mon, Mar 21, 2016 at 3:38 PM, Stephen Frost  wrote:
>> Basically, unlike with the similar nbtinsert.c code, we're checking
>> someone else's tuple in the speculative insertion
>> check_exclusion_or_unique_constraint() case that was changed (or it's
>> an exclusion constraint, where even the check for our own tuple
>> happens only after insertion; no change there in any case). Whereas,
>> in the nbtinsert.c case that I incorrectly duplicated, we're
>> specifically indicating that we're waiting on *our own* already
>> physically inserted heap tuple, and say as much in the
>> XLTW_InsertIndex message that makes it into the log.
>
> I don't quite follow how we're indicating that we're waiting on our own
> already physically inserted heap tuple in the log?

Because it's XLTW_InsertIndex in
check_exclusion_or_unique_constraint() now, this is the message:

case XLTW_InsertIndex:
cxt = gettext_noop("while inserting index tuple (%u,%u) in
relation \"%s\"");
break;

I guess what's at issue is whether or not it's okay that the (heap)
tuple TID shown (by this message) when waiting in
check_exclusion_or_unique_constraint() during ON CONFLICT is *not* our
own -- rather, it's the other guy's for this new use of
XLTW_InsertIndex. Does the message itself convey a contrary meaning,
or is it ambiguous in a way that supports either interpretation (i.e.
does that ambiguity allow us to leave things as they are)?

Certainly, this is unlike the nbtinsert.c XLTW_InsertIndex case (the
only other instance of XLTW_InsertIndex), where we must have
physically inserted already, and report specifically on our own
physically inserted TID when this message is shown. Does that
inconsistency alone make this wrong?

I think that XLTW_InsertIndex is intended to mean our own TID, based
on .1) the only precedent we have, and .2) based on the fact that
there is a distinct XLTW_InsertIndexUnique case at all.

BTW, just so the difference is clear, I point out that
XLTW_InsertIndexUnique (which I now propose to change
check_exclusion_or_unique_constraint() to use) says:

case XLTW_InsertIndexUnique:
cxt = gettext_noop("while checking uniqueness of tuple (%u,%u) in
relation \"%s\"");
break;

> One of the reasons I figured the XLTW_InsertIndex message made sense in
> this case is that it'd provide a consistent result for users.
> Specifically, I don't think it makes sense to produce different messages
> based only on who got there first.  I don't mind having a different
> message when there's something different happening (there's exclusion
> constraints involved, or you're building an index, etc).

I see your point. That's what I thought, initially.

>> It seems fine to characterize a wait here as "checking the uniqueness
>> of [somebody else's] tuple", even though technically we're checking
>> the would-be uniqueness were we to (speculatively, or actually)
>> insert. However, it does not seem fine to claim ctid_wait as a tuple
>> we ourselves inserted.
>
> I don't think either message really fits here, unfortunately.  We're not
> actually checking the uniqueness of someone else's tuple here either,
> after all, we're waiting to see what happens with their tuple because
> ours won't be unique if it goes in with that other tuple in place, if
> I'm following along correctly.

Well, we're determining if there was a conflict or not, in the first
phase of speculative insertion. That means that we need to see if an
update is required (for example). But that needs to behave just like
checking the uniqueness of an existing thing. It's just that our own
not-yet-physically-inserted "conceptual" tuple needs to be the thing
that makes this existing tuple (from some other xact) non-unique.

If that sounds weird, consider that in the standard, non-speculative
exclusion constraint case, the physical insertion actually has already
occurred, but even still we actually are waiting on the other guy's
tuple/xact (and report the other guy's TID, not our own, unlike with
nbtinsert.c). Notice that we make sure that it's another XID towards
the top of the loop within check_exclusion_or_unique_constraint()).

And so, its message says:

case XLTW_RecheckExclusionConstr:
cxt = gettext_noop("while checking exclusion constraint on tuple
(%u,%u) in relation \"%s\"");
break;

Of course, that this appeared for ON CONFLICT DO NOTHING with a B-Tree
index in 9.5.1 was wrong, but only because it said "exclusion
constraint" rather than "unique index". That's about what
XLTW_InsertIndexUnique says already, which is why I now think we
should just use XLTW_InsertIndexUnique.

> One thing I can say is that the XLTW_InsertIndex at least matches the
> *action* we're taking, which is that we're trying to INSERT.

Right, but I don't think that XLTW_InsertIndexUnique specifically
implies that we're not inserting, just as XLTW_RecheckExclusionConstr
does not specifically imply that we're not inserting (actually, we're
usually or always inserting 

Re: [HACKERS] OOM in libpq and infinite loop with getCopyStart()

[Amit Kapila]

On Mon, Mar 21, 2016 at 10:13 PM, Robert Haas  wrote:
>
> On Tue, Mar 1, 2016 at 12:38 AM, Michael Paquier
>  wrote:
> > Thoughts? I have registered that in the CF app, and a patch is attached.
>
> It is very difficult to believe that this is a good idea:
>
> --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
> +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
> @@ -445,6 +445,7 @@ libpqrcv_PQexec(const char *query)
>  if (PQresultStatus(lastResult) == PGRES_COPY_IN ||
>  PQresultStatus(lastResult) == PGRES_COPY_OUT ||
>  PQresultStatus(lastResult) == PGRES_COPY_BOTH ||
> +PQresultStatus(lastResult) == PGRES_FATAL_ERROR ||
>  PQstatus(streamConn) == CONNECTION_BAD)
>  break;
>  }
>
> I mean, why would it be a good idea to blindly skip over fatal errors?
>

I think it is not about skipping the FATAL error, rather to stop trying to
get further results on FATAL error.  This is to ensure that OOM error is
reported rather that ignored.  There has been discussion about this
previously as well [1].


[1] -
http://www.postgresql.org/message-id/CAB7nPqT6gKj6iS9VTPth_h6Sz5Jo-177s6QJN_jrW66wyCjJ=w...@mail.gmail.com

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

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

[Haribabu Kommi]

On Tue, Mar 22, 2016 at 2:19 PM, Amit Kapila  wrote:
> On Mon, Mar 21, 2016 at 6:16 PM, Haribabu Kommi 
> wrote:
>>
>> On Mon, Mar 14, 2016 at 4:51 PM, Amit Kapila 
>> wrote:
>> >> Operating system - windows 7
>> >> Binary - PostgreSQL 9.5 (This doesn't matter, 9.4+ can produce the
>> >> problem)
>> >>
>> >> 1. Create two standard users in the system (test_user1 and test_user2)
>> >> 2. Create two databases belongs each user listed above.
>> >> 3. Now using pg_ctl register the services for the two users.
>> >> 4. Provide logon permissions to these users to run the services by
>> >> changing
>> >> service properties.
>> >
>> > Did you mean to say that you changed Log on as: Local System Account in
>> > service properties or something else?
>>
>> No. Not as local service. The user should be the new standard user
>> that is created
>> in the system.
>>
>
> So what do you exactly mean by "Provide logon permissions to these users",
> can you describe in detail what exactly you have done to give those
> permissions.  If I try to do with a new user, it gives me error "could not
> open service manager"  at start of service.

1. Start the cmd with administrator user and add the new postgresql service
with a standard user that is created.
2. Start the services window with the user having administrator privileges and
go to the corresponding added service.
3. Right click on the service provides an properties option.
4. In the properties, there is an logon tab. Click it
5. Provide the password for the new user that is used for creating the service.
6. This adds the user to log on permissions.


>>
>> >> 5. Now try to start the services, the second service fails with the
>> >> error message.
>> >> 6. Error details can be found out in Event log viewer.
>> >>
>> >
>> > If I follow above steps and do as I mentioned for step-4, I am not able
>> > to
>> > reproduce the issue on Windows-7 m/c using code of HEAD.
>>
>> I am not able to start a service with HEAD code in the same machine, where
>> as it is working for 9.5. I will look into it later and update it.
>>
>
> Okay.  But it is confusing for me because you told earlier that you are able
> to reproduce problem in 9.5.

I am able to reproduce the problem with 9.5 binary. I am getting Access Denied
problem when i try to start the 9.6 binary service with the local user.


>> >> Yes, it is working as same user services. The main problem is,
>> >> PostgreSQL
>> >> as a service for two different users in the same system is not working
>> >> because
>> >> of same random getting generated for two services.
>> >>
>> >
>> > I am not sure why you think same random number is problem, as mentioned
>> > above, even if the dsm name is same due to same random number, the code
>> > has
>> > logic to process it appropriately (regenerate the name of dsm).  Having
>> > said
>> > that, I don't mean to say that we shouldn't have logic to generate
>> > unique
>> > name and I think we might want to add data dir path to name generation
>> > as we
>> > do for main shared memory, however it is better to first completely
>> > understand the underneath issue.
>>
>> Yes, same random number generation is not the problem. In windows apart
>> from EEXIST error, EACCES also needs to be validated and returned for
>> new random number generation, instead of throwing an error.
>>
>
> Doing the same handling for EACCES doesn't seem to be sane because if EACCES
> came for reason other than duplicate dsm name, then we want to report the
> error instead of trying to regenerate the name.  I think here fix should be
> to append data_dir path as we do for main shared memory.

Yes, EACCES may be possible other than duplicate dsm name.


Regards,
Hari Babu
Fujitsu Australia


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

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

[Amit Kapila]

On Mon, Mar 21, 2016 at 6:16 PM, Haribabu Kommi 
wrote:
>
> On Mon, Mar 14, 2016 at 4:51 PM, Amit Kapila 
wrote:
> >> Operating system - windows 7
> >> Binary - PostgreSQL 9.5 (This doesn't matter, 9.4+ can produce the
> >> problem)
> >>
> >> 1. Create two standard users in the system (test_user1 and test_user2)
> >> 2. Create two databases belongs each user listed above.
> >> 3. Now using pg_ctl register the services for the two users.
> >> 4. Provide logon permissions to these users to run the services by
> >> changing
> >> service properties.
> >
> > Did you mean to say that you changed Log on as: Local System Account in
> > service properties or something else?
>
> No. Not as local service. The user should be the new standard user
> that is created
> in the system.
>

So what do you exactly mean by "Provide logon permissions to these users",
can you describe in detail what exactly you have done to give those
permissions.  If I try to do with a new user, it gives me error "could not
open service manager"  at start of service.

>
> >> 5. Now try to start the services, the second service fails with the
> >> error message.
> >> 6. Error details can be found out in Event log viewer.
> >>
> >
> > If I follow above steps and do as I mentioned for step-4, I am not able
to
> > reproduce the issue on Windows-7 m/c using code of HEAD.
>
> I am not able to start a service with HEAD code in the same machine, where
> as it is working for 9.5. I will look into it later and update it.
>

Okay.  But it is confusing for me because you told earlier that you are
able to reproduce problem in 9.5.

> >> Yes, it is working as same user services. The main problem is,
PostgreSQL
> >> as a service for two different users in the same system is not working
> >> because
> >> of same random getting generated for two services.
> >>
> >
> > I am not sure why you think same random number is problem, as mentioned
> > above, even if the dsm name is same due to same random number, the code
has
> > logic to process it appropriately (regenerate the name of dsm).  Having
said
> > that, I don't mean to say that we shouldn't have logic to generate
unique
> > name and I think we might want to add data dir path to name generation
as we
> > do for main shared memory, however it is better to first completely
> > understand the underneath issue.
>
> Yes, same random number generation is not the problem. In windows apart
> from EEXIST error, EACCES also needs to be validated and returned for
> new random number generation, instead of throwing an error.
>

Doing the same handling for EACCES doesn't seem to be sane because
if EACCES came for reason other than duplicate dsm name, then we want to
report the error instead of trying to regenerate the name.  I think here
fix should be to append data_dir path as we do for main shared memory.

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

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

[Etsuro Fujita]

On 2016/03/19 4:51, Robert Haas wrote:

On Thu, Mar 17, 2016 at 7:00 AM, Etsuro Fujita
 wrote:

So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?)



Sure.



and (2) when any of
xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins.  (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.)  I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.



Now that seems like the wrong reaction.  I mean, aren't these just
going to be 0 or something?  Refusing to push the join down seems
strange.


OK, I'll modify the patch so that the join is pushed down even if any of 
xmins, xmaxs, cmins, and cmaxs are requested.  Do you think which one 
should set values for these as well as tableoids, postgres_fdw or core?


Best regards,
Etsuro Fujita




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

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

[Etsuro Fujita]

On 2016/03/19 3:30, Robert Haas wrote:

On Fri, Mar 18, 2016 at 5:15 AM, Etsuro Fujita
 wrote:

Attached is the updated version of the patch.



Committed.


Thank you.

Best regards,
Etsuro Fujita




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

Re: [HACKERS] Declarative partitioning

[Amit Langote]

On 2016/03/22 4:55, Robert Haas wrote:
> So, the last patch on this thread was posted on February 17th, and the
> CF entry was marked Waiting on Author on March 2nd.  Even if we had a
> new patch in hand at this point, I don't think there's any real chance
> of being able to get this done for 9.6; there are too many things left
> to do here in terms of figuring out syntax and scope, and of course
> performance testing.  Moreover, when this goes in, it's going to open
> up lots of opportunities for follow-up optimizations that we surely do
> not have time to follow up on for 9.6.  And, as it is, the patch
> hasn't been updated in over a month and is clearly not in final form
> as it exists today.
> 
> Therefore, I have marked this Returned with Feedback.  I look forward
> to returning to this topic for 9.7, and I'm willing to step up to the
> plate and review this more aggressively at that time, with an eye
> toward committing it when we've got it in good shape.  But I don't
> think there's any way to proceed with it for 9.6.

OK. I agree with the decision.

Actually, I was just about to post a patch set today, but I figure it's
too late for that.  Anyway, I look forward to working on this for 9.7.

Thanks,
Amit




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

Re: [HACKERS] Request - repeat value of \pset title during \watch interations

[David G. Johnston]

On Monday, March 21, 2016, Tom Lane  wrote:

> "David G. Johnston" > writes:
> > On Monday, March 21, 2016, Tom Lane >
> wrote:
> >> What about just discarding the old format entirely, and printing one of
> >> these two things:
> >>
> >> Timestamp (every Ns)
> >>
> >> User Given Title  Timestamp (every Ns)
>
> > This works for me.
>
> If I don't hear objections PDQ, I'm going to update the docs and commit
> it like that.
>
>
Saw it go in.  Thank You.

David J.

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Michael Paquier]

On Tue, Mar 22, 2016 at 6:25 AM, Tom Lane  wrote:
> If I don't hear objections PDQ, I'm going to update the docs and commit
> it like that.

Thanks!
-- 
Michael


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

Re: [HACKERS] [PATCH] we have added support for box type in SP-GiST index

[Stas Kelvich]

> On 22 Mar 2016, at 01:47, Jim Nasby  wrote:
> 
> On 3/21/16 11:57 AM, Teodor Sigaev wrote:
>> A and B are points of intersection of lines. So, box PBCAis a bounding
>> box for points contained in 3-rd (see labeling above). For example X
>> labeled point is not a descendace of child node with centroid  C because
>> it must be in branch of 1-st quad of parent node. So, each node (except
>> root) will have a limitation in its quadrant. To transfer that
>> limitation the traversalValue is used.
> 
> Isn't this basically the same thing that the cube contrib module does? (Which 
> has the added benefit of kNN-capable operators).

Cube and postgres own geometric types are indexed by building R-tree. While 
this is one of the most popular solutions it
degrades almost to linear search when bounding boxes for each index node 
overlaps a lot. This problem will arise when one will
try to index streets in the city with grid system — a lot of street's bounding 
boxes will just coincide with bounding box for whole city.

But that patch use totally different strategy for building index and do not 
suffer from above problem.

> 
> If that's true then ISTM it'd be better to work on pulling cube's features 
> into box?
> 
> If it's not true, I'm still wondering if there's enough commonality here that 
> we should pull cube into core…

There is also intarray module with very common functionality, but it also 
addressing different data pattern.

Optimal indexing and kNN strategy are very sensible to the data itself and if 
we want some general multidimensional search routines,
then I think none of the mentioned extensions is general enough. Cube barely 
applicable when dimensions number is higher than 10-20,
intarray performs bad on data with big sets of possible coordinates, this patch 
is also intended to help with specific, niche problem.

While people tends to use machine learning and regressions models more and more 
it is interesting to have some general n-dim indexing with kNN,
but I think it is different problem and should be solved in a different way.

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




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

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

[Abhijit Menon-Sen]

At 2016-03-21 15:43:09 -0400, robertmh...@gmail.com wrote:
>
> I also think we should allow a function to depend on multiple
> extensions, as Alvaro mentions downthread.

I'm working on an updated patch, will post shortly.

-- Abhijit


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

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

[Robert Haas]

On Mon, Mar 21, 2016 at 6:49 PM, Tom Lane  wrote:
> So, I'm -1 on not having any keyword at all.  I have no objection
> to Merlin's proposal though.  I agree that PERFORM is starting to
> look a bit silly, since it doesn't play with WITH for instance.

Yeah, I think requiring PERFORM is stupid and annoying.  +1 for
letting people write a SELECT with no target.

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

[Merlin Moncure]

On Mon, Mar 21, 2016 at 5:49 PM, Tom Lane  wrote:
> So, I'm -1 on not having any keyword at all.  I have no objection
> to Merlin's proposal though.  I agree that PERFORM is starting to
> look a bit silly, since it doesn't play with WITH for instance.

All right -- I'll submit a revised patch with documentation
adjustments.  "Basic Statements" needs to be revised, in particular
the section, "Executing a Command With No Result".  I'm inclined to
remove that section completely, and rename the next section from
"Executing a Query with a Single-row Result"  to simply, "Executing a
Query" (or perhaps "Non-Dynamic Query").

I'd like to remove documentation and usage of PERFORM except for a
note mentioning the old syntax...this would replace the current note
that existentially questions the necessity of PERFORM in the first
place.  Thanks.

merlin


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

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

[Tom Lane]

Jim Nasby  writes:
> On 3/21/16 5:03 PM, Merlin Moncure wrote:
>> in Oracle, you'd simply do:
>> LogIt('I did something');

> It would be *great* if we could support that in plpgsql.

FWIW, I'm hesitant to just start accepting that syntax as if it were an
equivalent to "SELECT f(x)" or "PERFORM f(x)".  I think that someday
we will want to have the ability to have pass-by-reference parameters
to functions, and that's a fundamentally incompatible behavior so it
ought to be invoked by a new syntax.  I'd like to save "f(x)" as a
bare statement for that purpose.  We could also consider inventing
"CALL f(x)"; but supposing that we already had both "CALL f(x)" (which
would allow x to be pass-by-ref and possibly modified) and "SELECT f(x)"
(which would not), which one would you assign bare "f(x)" to mean?  It
should be "CALL f(x)", not least because that would be the semantics most
comparable to Oracle's behavior (since they have pass-by-ref parameters
already).

So, I'm -1 on not having any keyword at all.  I have no objection
to Merlin's proposal though.  I agree that PERFORM is starting to
look a bit silly, since it doesn't play with WITH for instance.

> While we're on the subject, it'd be great if variable := SELECT ... 
> worked too.

It does, though you might need parens around the sub-select.

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] we have added support for box type in SP-GiST index

[Jim Nasby]

On 3/21/16 11:57 AM, Teodor Sigaev wrote:

A and B are points of intersection of lines. So, box PBCAis a bounding
box for points contained in 3-rd (see labeling above). For example X
labeled point is not a descendace of child node with centroid  C because
it must be in branch of 1-st quad of parent node. So, each node (except
root) will have a limitation in its quadrant. To transfer that
limitation the traversalValue is used.


Isn't this basically the same thing that the cube contrib module does? 
(Which has the added benefit of kNN-capable operators).


If that's true then ISTM it'd be better to work on pulling cube's 
features into box?


If it's not true, I'm still wondering if there's enough commonality here 
that we should pull cube into core...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] Minor bug affecting ON CONFLICT lock wait log messages

[Stephen Frost]

Peter,

* Peter Geoghegan (p...@heroku.com) wrote:
> Thinking about this again, I think we should use
> XLTW_InsertIndexUnique after all. The resemblance of the
> check_exclusion_or_unique_constraint() code to the nbtinsert.c code
> seems only superficial on second thought. So, I propose fixing the fix
> by changing XLTW_InsertIndex to XLTW_InsertIndexUnique.

I'm not against changing it, but...

> Basically, unlike with the similar nbtinsert.c code, we're checking
> someone else's tuple in the speculative insertion
> check_exclusion_or_unique_constraint() case that was changed (or it's
> an exclusion constraint, where even the check for our own tuple
> happens only after insertion; no change there in any case). Whereas,
> in the nbtinsert.c case that I incorrectly duplicated, we're
> specifically indicating that we're waiting on *our own* already
> physically inserted heap tuple, and say as much in the
> XLTW_InsertIndex message that makes it into the log. 

I don't quite follow how we're indicating that we're waiting on our own
already physically inserted heap tuple in the log?

> So, the
> fd658dbb300456b393536802d1145a9cea7b25d6 fix is wrong in that we now
> indicate that the wait was on our own already-inserted tuple, and not
> *someone else's* already-inserted tuple, as it should (we haven't
> inserting anything in the first phase of speculative insertion, this
> precheck). This code is not a do-over of the check in nbtinsert.c --
> rather, the code in nbtinsert.c is a second phase do-over of this code
> (where we've physically inserted a heap tuple + index tuple --
> "speculative" though that is).

One of the reasons I figured the XLTW_InsertIndex message made sense in
this case is that it'd provide a consistent result for users.
Specifically, I don't think it makes sense to produce different messages
based only on who got there first.  I don't mind having a different
message when there's something different happening (there's exclusion
constraints involved, or you're building an index, etc).

> It seems fine to characterize a wait here as "checking the uniqueness
> of [somebody else's] tuple", even though technically we're checking
> the would-be uniqueness were we to (speculatively, or actually)
> insert. However, it does not seem fine to claim ctid_wait as a tuple
> we ourselves inserted.

I don't think either message really fits here, unfortunately.  We're not
actually checking the uniqueness of someone else's tuple here either,
after all, we're waiting to see what happens with their tuple because
ours won't be unique if it goes in with that other tuple in place, if
I'm following along correctly.

One thing I can say is that the XLTW_InsertIndex at least matches the
*action* we're taking, which is that we're trying to INSERT.  While
having the ctid included in the message may be useful for debugging,
I've got doubts about including it in regular messages like this; we
certainly don't do that for the 'normal' INSERT case when we discover
a duplicate key, but that ship has sailed.

Ultimately, I guess I'm inclined to leave it as-committed.  If you
understand enough to realize what that pair of numbers after 'tuple'
means, you've probably found this thread and followed it enough to
understand what's happening.

I don't feel terribly strongly about that position and so if others
feel the XLTW_InsertIndexUnique message really would be better, I'd be
happy to commit the change.

Thanks!

Stephen


signature.asc
Description: Digital signature

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

[Jim Nasby]

On 3/21/16 5:03 PM, Merlin Moncure wrote:

in Oracle, you'd simply do:
LogIt('I did something');


It would be *great* if we could support that in plpgsql.


I'm not sure what Oracle does for SELECT statements without INTO/BULK
UPDATE.  I'm not really inclined to care -- I'm really curious to see
an argument where usage of PERFORM actually helps in some meaningful
way.  Notably, SELECT without INTO is accepted syntax, but fails only
after running the query.  I think that's pretty much stupid but it's
fair to say I'm not inventing syntax, only disabling the error.


I don't think it buys much at all.

While we're on the subject, it'd be great if variable := SELECT ... 
worked too.



I'm not sure what other databases do is relevant.   They use other
procedure languages than pl//sql (the biggest players are pl/psm and
t-sql) which have a different set of rules in terms of passing
variables in and out of queries.


+1
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] WIP: Access method extendability

[Alvaro Herrera]

Alexander Korotkov wrote:
> Hi!
> 
> Thank you for review!

So.  Before this version of the patch was posted in Nov 4th 2015, both
Tom and Heikki had said essentially "CREATE ACCESS METHOD is worthless,
let's pursue this stuff without those commands".
http://www.postgresql.org/message-id/54730dfd.2060...@vmware.com (Nov 2014)
http://www.postgresql.org/message-id/26822.1414516...@sss.pgh.pa.us (Oct 2014)

And then Alexander posted this version, without any discussion that
evidenced that those old objections were overridden.  What happened
here?  Did somebody discuss this in unarchived meetings?  If so, it
would be good to know about that in this thread.

I noticed this state of affairs because I started reading the complete
thread in order to verify whether a consensus had been reached regarding
the move of IndexQualInfo and GenericCosts to selfuncs.h.  But I only
see that Jim Nasby mentioned it and Alexander said "yes they move";
nothing else.  The reason for raising this is that, according to
Alexander, new index AMs will want to use those structs; but current
index AMs only include index_selfuncs.h, and none of them includes
selfuncs.h, so it seems completely wrong to be importing all the planner
knowledge embodied in selfuncs.h into extension index AMs.

One observation is that the bloom AM patch (0003 of this series) uses
GenericCosts but not IndexQualInfo.  But btcostestimate() and
gincostestimate() both use IndexQualInfo, so other AMs might well want
to use it too.

We could move GenericCosts and the prototype for genericcostestimate()
to index_selfuncs.h; that much is pretty reasonable.  But I'm not sure
what to do about IndexQualInfo.  We could just add forward struct
declarations for RestrictInfo and Node to index_selfuncs.h.  But then
the extension code is going to have to import the includes were those
are defined anyway.  Certainly it seems that deconstruct_indexquals()
(which returns a list of IndexQualInfo) is going to be needed by
extension index AMs, at least ...

I've made a few edits to the patch already, but I need to do some more
testing.  Particularly I would like to understand the relcache issues to
figure out whether the current one is right.

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


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

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

[Merlin Moncure]

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

Well, pl/pgsql is based on oracle pl/sql so I don't see how the
standard is involved.  FWICT, "PERFORM" is a postgres extension to
pl/pgsql.  I don't see how the standard plays at all.

> Can you start with analyze what is possible, and what semantic is allowed in
> standard and other well known SQL databases?

Typical use of PERFORM is void returning function.  Oracle allows use
of those functions without any decoration at all.   For example, in
postgres we might do:
PERFORM LogIt('I did something');

in Oracle, you'd simply do:
LogIt('I did something');

I'm not sure what Oracle does for SELECT statements without INTO/BULK
UPDATE.  I'm not really inclined to care -- I'm really curious to see
an argument where usage of PERFORM actually helps in some meaningful
way.  Notably, SELECT without INTO is accepted syntax, but fails only
after running the query.  I think that's pretty much stupid but it's
fair to say I'm not inventing syntax, only disabling the error.

I'm not sure what other databases do is relevant.   They use other
procedure languages than pl//sql (the biggest players are pl/psm and
t-sql) which have a different set of rules in terms of passing
variables in and out of queries.

merlin


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

Re: [HACKERS] [PATCH] we have added support for box type in SP-GiST index

[Stas Kelvich]

> On 21 Mar 2016, at 22:38, Kevin Grittner  wrote:
> 
> On Mon, Mar 21, 2016 at 11:57 AM, Teodor Sigaev  wrote:
> 
>>> I couldn't get the term 4D point.  Maybe it means that we are
>>> using box datatype as the prefix, but we are not treating them
>>> as boxes.
>> 
>> exactly, we treat boxes as 4-dimentional points.
> 
> I'm not entirely sure I understand the terminology either, since I
> tend to think of a *point* as having *zero* dimensions.  Would it
> perhaps be more accurate to say we are treating a 2-dimensional box
> as a point in 4-dimensional space?

Or just say 4-d vector instead of 4-d point. Look like it will be enough 
rigorous.

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




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

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Alvaro Herrera]

Tom Lane wrote:
> "David G. Johnston"  writes:
> > On Monday, March 21, 2016, Tom Lane  wrote:
> >> What about just discarding the old format entirely, and printing one of
> >> these two things:
> >> 
> >> Timestamp (every Ns)
> >> 
> >> User Given Title  Timestamp (every Ns)
> 
> > This works for me.
> 
> If I don't hear objections PDQ, I'm going to update the docs and commit
> it like that.

It works for me too.

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


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

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Tom Lane]

"David G. Johnston"  writes:
> On Monday, March 21, 2016, Tom Lane  wrote:
>> What about just discarding the old format entirely, and printing one of
>> these two things:
>> 
>> Timestamp (every Ns)
>> 
>> User Given Title  Timestamp (every Ns)

> This works for me.

If I don't hear objections PDQ, I'm going to update the docs and commit
it like that.

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] pgbench - allow backslash-continuations in custom scripts

[Tom Lane]

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

However, since pgbench is already set up to slurp the entire file and
lex it in one go, it is just a trivial adjustment to the lexer rules in
that program.  The only thing I found that made it complicated is that
syntax_error() had too simplistic an understanding of how to position
the error cursor usefully, so that needed a bit of work.

I think it'd be okay to commit this without fixing psql at the same time;
if you try it in psql you get an error, so on that side it's unimplemented
behavior rather than an actual incompatibility.  Perhaps somebody will be
motivated to fix it later, but I'm not going to spend that kind of time
on it right now.

I've not written a docs update, but otherwise I think this is committable.
Comments?

regards, tom lane

diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l
index 825dacc..12b5f7e 100644
*** a/src/bin/pgbench/exprscan.l
--- b/src/bin/pgbench/exprscan.l
***
*** 6,17 
   *
   * This lexer supports two operating modes:
   *
!  * In INITIAL state, just parse off whitespace-separated words (this mode
!  * is basically equivalent to strtok(), which is what we used to use).
   *
   * In EXPR state, lex for the simple expression syntax of exprparse.y.
   *
!  * In either mode, stop upon hitting newline or end of string.
   *
   * Note that this lexer operates within the framework created by psqlscan.l,
   *
--- 6,18 
   *
   * This lexer supports two operating modes:
   *
!  * In INITIAL state, just parse off whitespace-separated words.  (This mode
!  * is basically equivalent to strtok(), which is what we used to use.)
   *
   * In EXPR state, lex for the simple expression syntax of exprparse.y.
   *
!  * In either mode, stop upon hitting newline, end of string, or unquoted
!  * backslash (except that backslash-newline is silently swallowed).
   *
   * Note that this lexer operates within the framework created by psqlscan.l,
   *
*** extern void expr_yyset_column(int column
*** 61,69 
  alpha			[a-zA-Z_]
  digit			[0-9]
  alnum			[a-zA-Z0-9_]
! /* {space} + {nonspace} + {newline} should cover all characters */
  space			[ \t\r\f\v]
! nonspace		[^ \t\r\f\v\n]
  newline			[\n]
  
  /* Exclusive states */
--- 62,71 
  alpha			[a-zA-Z_]
  digit			[0-9]
  alnum			[a-zA-Z0-9_]
! /* {space} + {nonspace} + {backslash} + {newline} should cover all characters */
  space			[ \t\r\f\v]
! nonspace		[^ \t\r\f\v\\\n]
! backslash		[\\]
  newline			[\n]
  
  /* Exclusive states */
*** newline			[\n]
*** 98,103 
--- 100,113 
  
  {space}+		{ /* ignore */ }
  
+ {backslash}{newline}	{ /* ignore */ }
+ 
+ {backslash}		{
+ 	/* do not eat, and report end of command */
+ 	yyless(0);
+ 	return 0;
+ }
+ 
  {newline}		{
  	/* report end of command */
  	last_was_newline = true;
*** newline			[\n]
*** 130,143 
  	return FUNCTION;
  }
  
  {newline}		{
  	/* report end of command */
  	last_was_newline = true;
  	return 0;
  }
  
- {space}+		{ /* ignore */ }
- 
  .{
  	/*
  	 * must strdup yytext so that expr_yyerror_more doesn't
--- 140,161 
  	return FUNCTION;
  }
  
+ {space}+		{ /* ignore */ }
+ 
+ {backslash}{newline}	{ /* ignore */ }
+ 
+ {backslash}		{
+ 	/* do not eat, and report end of command */
+ 	yyless(0);
+ 	return 0;
+ }
+ 
  {newline}		{
  	/* report end of command */
  	last_was_newline = true;
  	return 0;
  }
  
  .{
  	/*
  	 * must strdup yytext so that expr_yyerror_more doesn't
*** expr_yyerror_more(yyscan_t yyscanner, co
*** 177,183 
  	/*
  	 * While parsing an expression, we may not have collected the whole line
  	 * yet from the input source.  Lex till EOL so we can report whole line.
! 	 * (If we're at EOF, it's okay to call yylex() an extra time.)
  	 */
  	if (!last_was_newline)
  	{
--- 195,201 
  	/*
  	 * While parsing an expression, we may not have collected the whole line
  	 * yet from the input source.  Lex till EOL so we can report whole line.
! 	 * (If we're at backslash/EOF, it's okay to call yylex() an extra time.)
  	 */
  	if (!last_was_newline)
  	{
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4196b0e..e947f77 100644
*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
*** syntax_error(const char *source, int lin
*** 2413,2426 

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

[Pavel Stehule]

Hi

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

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

My negative opinion is known. The PERFORM statement is much more workaround
than well designed statement, but I would to see ANSI/SQL based fix. I try
to compare benefits and loss.

Can you start with analyze what is possible, and what semantic is allowed
in standard and other well known SQL databases?

Regards

Pavel


>
> merlin
>
>
>
> diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
> index b7f44ca..a860066 100644
> --- a/src/pl/plpgsql/src/pl_exec.c
> +++ b/src/pl/plpgsql/src/pl_exec.c
> @@ -3457,12 +3457,9 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
> }
> else
> {
> -   /* If the statement returned a tuple table, complain */
> +   /* If the statement returned a tuple table, free it. */
> if (SPI_tuptable != NULL)
> -   ereport(ERROR,
> -   (errcode(ERRCODE_SYNTAX_ERROR),
> -errmsg("query has no destination for result data"),
> -(rc == SPI_OK_SELECT) ? errhint("If you want to
> discard the results of a SELECT, use PERFORM instead.") : 0));
> +   SPI_freetuptable(SPI_tuptable);
> }
>
> if (paramLI)
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

[Merlin Moncure]

Patch is trivial (see below), discussion is not :-).

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

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

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

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

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

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

merlin



diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b7f44ca..a860066 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3457,12 +3457,9 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
}
else
{
-   /* If the statement returned a tuple table, complain */
+   /* If the statement returned a tuple table, free it. */
if (SPI_tuptable != NULL)
-   ereport(ERROR,
-   (errcode(ERRCODE_SYNTAX_ERROR),
-errmsg("query has no destination for result data"),
-(rc == SPI_OK_SELECT) ? errhint("If you want to
discard the results of a SELECT, use PERFORM instead.") : 0));
+   SPI_freetuptable(SPI_tuptable);
}

if (paramLI)


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

Re: [HACKERS] snapshot too old, configured by time

[Kevin Grittner]

Thanks to all for the feedback; I will try to respond later this
week.  First I'm trying to get my reviews for other patches posted.

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


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

Re: [HACKERS] Declarative partitioning

[Simon Riggs]

On 21 March 2016 at 19:55, Robert Haas  wrote:

> On Wed, Mar 16, 2016 at 10:49 AM, Alexander Korotkov
>  wrote:
> >> > I'd like to validate that this development plan doesn't overlaps with
> >> > your
> >> > plans.  If out plans are not overlapping then let's accept this plan
> of
> >> > work
> >> > for 9.7.
> >>
> >> It looks OK to me.  Thanks for sharing it.
> >
> >
> > Great! Let's work together.
>
> So, the last patch on this thread was posted on February 17th, and the
> CF entry was marked Waiting on Author on March 2nd.  Even if we had a
> new patch in hand at this point, I don't think there's any real chance
> of being able to get this done for 9.6; there are too many things left
> to do here in terms of figuring out syntax and scope, and of course
> performance testing.  Moreover, when this goes in, it's going to open
> up lots of opportunities for follow-up optimizations that we surely do
> not have time to follow up on for 9.6.  And, as it is, the patch
> hasn't been updated in over a month and is clearly not in final form
> as it exists today.
>
> Therefore, I have marked this Returned with Feedback.  I look forward
> to returning to this topic for 9.7, and I'm willing to step up to the
> plate and review this more aggressively at that time, with an eye
> toward committing it when we've got it in good shape.  But I don't
> think there's any way to proceed with it for 9.6.
>

Good decision.

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

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

Re: [HACKERS] Declarative partitioning

[Robert Haas]

On Wed, Mar 16, 2016 at 10:49 AM, Alexander Korotkov
 wrote:
>> > I'd like to validate that this development plan doesn't overlaps with
>> > your
>> > plans.  If out plans are not overlapping then let's accept this plan of
>> > work
>> > for 9.7.
>>
>> It looks OK to me.  Thanks for sharing it.
>
>
> Great! Let's work together.

So, the last patch on this thread was posted on February 17th, and the
CF entry was marked Waiting on Author on March 2nd.  Even if we had a
new patch in hand at this point, I don't think there's any real chance
of being able to get this done for 9.6; there are too many things left
to do here in terms of figuring out syntax and scope, and of course
performance testing.  Moreover, when this goes in, it's going to open
up lots of opportunities for follow-up optimizations that we surely do
not have time to follow up on for 9.6.  And, as it is, the patch
hasn't been updated in over a month and is clearly not in final form
as it exists today.

Therefore, I have marked this Returned with Feedback.  I look forward
to returning to this topic for 9.7, and I'm willing to step up to the
plate and review this more aggressively at that time, with an eye
toward committing it when we've got it in good shape.  But I don't
think there's any way to proceed with it for 9.6.

-- 
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] pgbench - allow backslash-continuations in custom scripts

[Tom Lane]

Robert Haas  writes:
> Mmph.  I just don't see any benefit in being able to start a command
> in the middle of a line.

I agree that there's no functional benefit; it's a matter of consistency.
In particular, psql has always allowed you to write multiple SQL commands
per line:

SELECT 2+2; SELECT x FROM tab; SELECT y FROM othertab;

and as of yesterday pgbench supports that as well.  So allowing multiple
backslash commands on a line improves consistency both with psql and with
pgbench's own behavior, IMV.

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] pgbench - allow backslash-continuations in custom scripts

[Robert Haas]

On Mon, Mar 21, 2016 at 3:42 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Mon, Mar 21, 2016 at 3:01 PM, Tom Lane  wrote:
>>> Um, why exactly?  That psql behavior is of really ancient standing, and
>>> we have not had complaints about it.
>
>> I think that's mostly because the psql metacommands are ridiculously
>> impoverished.  I'm guessing that pgbench's expression language is
>> eventually going to support strings as a data type, for example, and
>> those strings might want to contain backlashes.
>
> Sure, but once we define strings, they'll be quoted, and the behavior
> can/should/will be different for a backslash inside quotes than one
> outside them --- as it is already in psql.  Moreover, if you're on
> board with the backslash-newline proposal, you've already bought into
> the idea that backslashes outside quotes will behave differently from
> those inside.

Mmph.  I just don't see any benefit in being able to start a command
in the middle of a line.  Even if it's not dangerous to future things
we might want o implement, I'd argue it's still poor style, and of no
practical utility.  But if I lose that argument, then I do.

>>> Shall I make a patch that allows backslash-newline to be handled this way
>>> in both psql and pgbench backslash commands?
>
>> I certainly don't object to such a patch, although if it's between you
>> writing that patch and you getting Tomas Vondra's multivariate
>> statistics stuff committed, I'll take the latter.  :-)
>
> I'll get to that, but I'd like to get this area fully dealt with before
> context-swapping to that one.

Understood.

-- 
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] dealing with extension dependencies that aren't quite 'e'

[Robert Haas]

On Mon, Mar 21, 2016 at 7:19 AM, Abhijit Menon-Sen  wrote:
> At 2016-03-21 13:04:33 +0300, a.korot...@postgrespro.ru wrote:
>>
>> I'm not sure why we want to make new dependency type by ALTER FUNCTION
>> command, not ALTER EXTENSION?
>
> It's a matter of semantics. It means something very different than what
> an 'e' dependency means. The extension doesn't own the function (and so
> pg_dump shouldn't ignore it), but the function depends on the extension
> (and so dropping the extension should drop it).

Yeah, I think this is definitely an ALTER FUNCTION kind of thing, not
an ALTER EXTENSION kind of thing.

I also think we should allow a function to depend on multiple
extensions, as Alvaro mentions downthread.

-- 
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] pgbench - allow backslash-continuations in custom scripts

[Tom Lane]

Robert Haas  writes:
> On Mon, Mar 21, 2016 at 3:01 PM, Tom Lane  wrote:
>> Um, why exactly?  That psql behavior is of really ancient standing, and
>> we have not had complaints about it.

> I think that's mostly because the psql metacommands are ridiculously
> impoverished.  I'm guessing that pgbench's expression language is
> eventually going to support strings as a data type, for example, and
> those strings might want to contain backlashes.

Sure, but once we define strings, they'll be quoted, and the behavior
can/should/will be different for a backslash inside quotes than one
outside them --- as it is already in psql.  Moreover, if you're on
board with the backslash-newline proposal, you've already bought into
the idea that backslashes outside quotes will behave differently from
those inside.

>> Shall I make a patch that allows backslash-newline to be handled this way
>> in both psql and pgbench backslash commands?

> I certainly don't object to such a patch, although if it's between you
> writing that patch and you getting Tomas Vondra's multivariate
> statistics stuff committed, I'll take the latter.  :-)

I'll get to that, but I'd like to get this area fully dealt with before
context-swapping to that one.

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

[Julien Rouhaud]

On 21/03/2016 05:18, James Sewell wrote:
> OK cool, thanks.
> 
> Can we remove the minimum size limit when the per table degree setting
> is applied?
> 
> This would help for tables with 2  - 1000 pages combined with a high CPU
> cost aggregate.
> 

Attached v4 implements that. It also makes sure that the chosen
parallel_degree won't be more than the relation's estimated number of pages.

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index cd234db..80e1f09 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -909,6 +909,17 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | 
UNLOGGED ] TABLE [ IF NOT EXI

 

+parallel_degree (integer)
+
+ 
+  Number of workers wanted for this table. The number of worker will be
+  limited according to the 
+  parameter.
+ 
+
+   
+
+   
 autovacuum_enabled, 
toast.autovacuum_enabled (boolean)
 
  
diff --git a/src/backend/access/common/reloptions.c 
b/src/backend/access/common/reloptions.c
index ea0755a..68bb133 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -267,6 +267,15 @@ static relopt_int intRelOpts[] =
0, 0, 0
 #endif
},
+   {
+   {
+   "parallel_degree",
+   "Number of parallel processes per executor node wanted 
for this relation.",
+   RELOPT_KIND_HEAP,
+   AccessExclusiveLock
+   },
+   -1, 1, INT_MAX
+   },
 
/* list terminator */
{{NULL}}
@@ -1291,7 +1300,9 @@ default_reloptions(Datum reloptions, bool validate, 
relopt_kind kind)
{"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts, 
analyze_scale_factor)},
{"user_catalog_table", RELOPT_TYPE_BOOL,
-   offsetof(StdRdOptions, user_catalog_table)}
+   offsetof(StdRdOptions, user_catalog_table)},
+   {"parallel_degree", RELOPT_TYPE_INT,
+   offsetof(StdRdOptions, parallel_degree)}
};
 
options = parseRelOptions(reloptions, validate, kind, );
diff --git a/src/backend/optimizer/path/allpaths.c 
b/src/backend/optimizer/path/allpaths.c
index e1a5d33..8a871f7 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -669,21 +669,32 @@ create_parallel_paths(PlannerInfo *root, RelOptInfo *rel)
 * just for this relation, but when combined with all of its 
inheritance siblings
 * it may well pay off.
 */
-   if (rel->pages < parallel_threshold && rel->reloptkind == 
RELOPT_BASEREL)
+   if (rel->pages < parallel_threshold && rel->rel_parallel_degree == -1 &&
+   rel->reloptkind == RELOPT_BASEREL)
return;
 
/*
-* Limit the degree of parallelism logarithmically based on the size of 
the
-* relation.  This probably needs to be a good deal more sophisticated, 
but we
-* need something here for now.
+* Use the table parallel_degree if specified, but don't go further than
+* relation's number of pages or max_parallel_degree
 */
-   while (rel->pages > parallel_threshold * 3 &&
-  parallel_degree < max_parallel_degree)
+   if (rel->rel_parallel_degree > 0)
+   parallel_degree = Min(rel->pages, Min(rel->rel_parallel_degree,
+   max_parallel_degree));
+   else
{
-   parallel_degree++;
-   parallel_threshold *= 3;
-   if (parallel_threshold >= PG_INT32_MAX / 3)
-   break;
+   /*
+* Limit the degree of parallelism logarithmically based on the 
size of the
+* relation.  This probably needs to be a good deal more 
sophisticated, but we
+* need something here for now.
+*/
+   while (rel->pages > parallel_threshold * 3 &&
+  parallel_degree < max_parallel_degree)
+   {
+   parallel_degree++;
+   parallel_threshold *= 3;
+   if (parallel_threshold >= PG_INT32_MAX / 3)
+   break;
+   }
}
 
/* Add an unordered partial path based on a parallel sequential scan. */
diff --git a/src/backend/optimizer/util/plancat.c 
b/src/backend/optimizer/util/plancat.c
index 546067b..92feefc 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -128,6 +128,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, 
bool inhparent,
estimate_rel_size(relation, rel->attr_widths - rel->min_attr,

Re: [HACKERS] [PATCH] we have added support for box type in SP-GiST index

[Kevin Grittner]

On Mon, Mar 21, 2016 at 11:57 AM, Teodor Sigaev  wrote:

>> I couldn't get the term 4D point.  Maybe it means that we are
>> using box datatype as the prefix, but we are not treating them
>> as boxes.
>
> exactly, we treat boxes as 4-dimentional points.

I'm not entirely sure I understand the terminology either, since I
tend to think of a *point* as having *zero* dimensions.  Would it
perhaps be more accurate to say we are treating a 2-dimensional box
as a point in 4-dimensional space?

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


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

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

[Robert Haas]

On Mon, Mar 21, 2016 at 3:01 PM, Tom Lane  wrote:
>> Wait, was it really?  I'd been thinking it was mostly to continue
>> queries, not metacommands, but maybe I missed the boat.
>
> Nah, you're right, it was about continuing queries.  Still, we've had
> complaints about the other thing too, and I think if we're going to
> change anything here, we should change it all in the same release.

Fair enough.

>>> * Allow multiple backslash commands on one line, eg
>>> \set foo 5 \set bar 6
>>> The main reason for that is that psql allows it, and one of the things
>>> we're supposedly trying to do here is reduce the behavioral distance
>>> between psql and pgbench parsing rules.
>
>> This seems to me to be going in the wrong direction.
>
> Um, why exactly?  That psql behavior is of really ancient standing, and
> we have not had complaints about it.

I think that's mostly because the psql metacommands are ridiculously
impoverished.  I'm guessing that pgbench's expression language is
eventually going to support strings as a data type, for example, and
those strings might want to contain backlashes.  There's basically no
value in cramming multiple metacommands onto a single line, but there
is the risk of creating unnecessary lexing or parsing difficulties in
the future.

>>> * Allow backslash commands to span lines, probably by adopting the
>>> rule that backslash immediately followed by newline is to be ignored
>>> within a backslash command.  This would not be compatible with psql,
>>> though, at least not unless we wanted to change psql too.
>
>> This might have some point to it, though, if you want to say \set i
>> 
>
> Shall I make a patch that allows backslash-newline to be handled this way
> in both psql and pgbench backslash commands?  At least in psql, there
> would be no backwards compatibility problem, since right now the case
> just fails:
>
> regression=# \set x y \
> Invalid command \. Try \? for help.

I certainly don't object to such a patch, although if it's between you
writing that patch and you getting Tomas Vondra's multivariate
statistics stuff committed, I'll take the latter.  :-)

-- 
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] Request - repeat value of \pset title during \watch interations

[David G. Johnston]

On Monday, March 21, 2016, Tom Lane  wrote:

> "David G. Johnston" > writes:
> > I'll admit it's awkward because it's abbreviated but if someone enters
> > \watch 5 and then sees (5s) in the title I think they can put two and two
> > together.
>
> Where I find this to be awkward is that the format is randomly different
> between the user-title and no-user-title cases.
>
> What about just discarding the old format entirely, and printing one of
> these two things:
>
> Timestamp (every Ns)
>
> User Given Title  Timestamp (every Ns)
>
>
This works for me.

David J.

Re: [HACKERS] snapshot too old, configured by time

[Thom Brown]

On 17 March 2016 at 21:15, Kevin Grittner  wrote:
> New patch just to merge in recent commits -- it was starting to
> show some bit-rot.  Tests folded in with main patch.

In session 1, I've run:

# begin transaction isolation level repeatable read ;
BEGIN

*# declare stuff scroll cursor for select * from test where num between 5 and 9;
DECLARE CURSOR

*# fetch forward 5 from stuff;
 id  | num |   thing
-+-+
   2 |   8 | hellofji djf odsjfiojdsif ojdsiof
   3 |   7 | hellofji djf odsjfiojdsif ojdsiof
 112 |   9 | hellofji djf odsjfiojdsif ojdsiof
 115 |   6 | hellofji djf odsjfiojdsif ojdsiof
 119 |   8 | hellofji djf odsjfiojdsif ojdsiof
(5 rows)


In session 2, over a min later:

# update test set num = 12 where num between 5 and 9 and id between 120 and 250;
ERROR:  snapshot too old


Then back to session 1:

*# fetch forward 5 from stuff;
ERROR:  snapshot too old


Should session 2 be getting that error?

Thom


-- 
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] Request - repeat value of \pset title during \watch interations

[Tom Lane]

Alvaro Herrera  writes:
> (I'll also use this opportunity to complain again about not being able
> to use floating point sleep time.)

That's not unreasonable either, though it seems like material for a
separate patch.

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] Request - repeat value of \pset title during \watch interations

[Tom Lane]

"David G. Johnston"  writes:
> I'll admit it's awkward because it's abbreviated but if someone enters
> \watch 5 and then sees (5s) in the title I think they can put two and two
> together.

Where I find this to be awkward is that the format is randomly different
between the user-title and no-user-title cases.

What about just discarding the old format entirely, and printing one of
these two things:

Timestamp (every Ns)

User Given Title  Timestamp (every Ns)

> If the watched query takes a long to run, or there is a disruption, knowing
> when the last one ran and how often it is supposed to run is useful info to
> have at ones fingertips.

That's not unreasonable.  I just want it to look less weirdly different
between the two cases.

regards, tom lane


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

Re: [HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Tom Lane]

Chapman Flack  writes:
> On 03/21/2016 10:21 AM, Aleksander Alekseev wrote:
>> Well in this case here is a patch that fixes "use of uninitialized
>> value" reports by MemorySanitizer I managed to catch so far.

> I'm new here so someone more experienced would have to weigh in,
> but I would wonder a couple of things:

> a. whether a braced struct assignment is supported in every
>C compiler that PostgreSQL still intends to support

We rely on struct assignment to work already; although I'm not sure
we should expect it to be efficient, so we might not want to use it
in performance-critical places.

> b. whether such a struct assignment is guaranteed to initialize
>padding spaces as well as declared fields (in all supported
>C versions/compilers).

I think this is a valid concern; my recollection is that the C standard
defines struct assignment as "assign each member".

> It's possible that memset() would be more convincing.

+1

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] pgbench - allow backslash-continuations in custom scripts

[Tom Lane]

Robert Haas  writes:
> On Sun, Mar 20, 2016 at 1:07 PM, Tom Lane  wrote:
>> This solves the problem of allowing SQL commands in scripts to span
>> lines, ...

> Excellent.

>> but it doesn't do anything about backslash commands, which was
>> the original point according to the thread title ;-).

> Wait, was it really?  I'd been thinking it was mostly to continue
> queries, not metacommands, but maybe I missed the boat.

Nah, you're right, it was about continuing queries.  Still, we've had
complaints about the other thing too, and I think if we're going to
change anything here, we should change it all in the same release.

>> I can think of
>> two somewhat-independent changes we might want to make at this point,
>> since we're breaking exact script compatibility for 9.6 anyway:
>> 
>> * Allow multiple backslash commands on one line, eg
>> \set foo 5 \set bar 6
>> The main reason for that is that psql allows it, and one of the things
>> we're supposedly trying to do here is reduce the behavioral distance
>> between psql and pgbench parsing rules.

> This seems to me to be going in the wrong direction.

Um, why exactly?  That psql behavior is of really ancient standing, and
we have not had complaints about it.

>> * Allow backslash commands to span lines, probably by adopting the
>> rule that backslash immediately followed by newline is to be ignored
>> within a backslash command.  This would not be compatible with psql,
>> though, at least not unless we wanted to change psql too.

> This might have some point to it, though, if you want to say \set i
> 

Shall I make a patch that allows backslash-newline to be handled this way
in both psql and pgbench backslash commands?  At least in psql, there
would be no backwards compatibility problem, since right now the case
just fails:

regression=# \set x y \
Invalid command \. Try \? for help.

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] Request - repeat value of \pset title during \watch interations

[Alvaro Herrera]

David G. Johnston wrote:

> Tom doesn't care enough to veto and you don't really care...
> 
> I'll admit it's awkward because it's abbreviated but if someone enters
> \watch 5 and then sees (5s) in the title I think they can put two and two
> together.
> 
> If the watched query takes a long to run, or there is a disruption, knowing
> when the last one ran and how often it is supposed to run is useful info to
> have at ones fingertips.  I have done this myself occasionally so I'm not
> speaking from theory.  But I won't complain if its removed.

I like David's UI better FWIW.

(I'll also use this opportunity to complain again about not being able
to use floating point sleep time.)

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


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

Re: [HACKERS] Request - repeat value of \pset title during \watch interations

[David G. Johnston]

On Monday, March 21, 2016, Robert Haas  wrote:

> On Mon, Mar 21, 2016 at 2:09 PM, David G. Johnston
> > wrote:
> > On Monday, March 21, 2016, Tom Lane >
> wrote:
> >> "David G. Johnston" > writes:
> >> > I'd rather not omit sleep but removing "Watch every" is fine
> (preferred
> >> > actually), so:
> >> > Title Is Here Mon Mar 21 15:05:06 2016 (5s)
> >>
> >> Meh ... seems a bit awkward to me.  Couldn't you include " (5s)" in the
> >> title, if you want that info?  If it's variable, you could still
> >> accommodate that:
> >
> > Actually, only if it's a variable that you setup and repeat and you
> show.  A
> > bit cumbersome and mixes the parts that are title and those that are
> present
> > only because you are watching.
>
> Ah, come on.  This doesn't really seem like an issue we should spend
> more time quibbling about.  I think Tom's version is fine.
>
>
Tom doesn't care enough to veto and you don't really care...

I'll admit it's awkward because it's abbreviated but if someone enters
\watch 5 and then sees (5s) in the title I think they can put two and two
together.

If the watched query takes a long to run, or there is a disruption, knowing
when the last one ran and how often it is supposed to run is useful info to
have at ones fingertips.  I have done this myself occasionally so I'm not
speaking from theory.  But I won't complain if its removed.

David J.

Re: [HACKERS] Performance degradation in commit ac1d794

[Thomas Munro]

On Mon, Mar 21, 2016 at 6:09 PM, Andres Freund  wrote:
> On 2016-03-21 11:52:43 +1300, Thomas Munro wrote:
>> * I would be interested in writing a kqueue implementation of this for
>> *BSD (and MacOSX?) at some point if someone doesn't beat me to it.
>
> I hoped that somebody would do that - that'd afaics be the only major
> API missing.

Here's a first swing at it, though I need to find some spare time to
test and figure out if some details like error conditions and EOF are
handled correctly.  It could in theory minimise the number of syscalls
it makes by buffering changes (additions and hopefully one day
removals) and then use a single kevent syscall to apply all
modifications to the set and begin waiting, but for now it mirrors the
epoll code.  One user-visible difference compared to epoll/poll/select
is that it delivers readable and writable events separately if both
conditions are true for a single fd.  It builds and appears to work
correctly on FreeBSD 10.2 and MacOSX 10.10.2.  Sharing this early
version in case any BSD users have any feedback.  I hope to do some
testing this weekend.

-- 
Thomas Munro
http://www.enterprisedb.com


kqueue.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] incorrect docs for pgbench / skipped transactions

[Fabien COELHO]

Ok, I added a reference to the commitfest entry from this wiki page, and a
note about partial 9.5 backporting.


Please split the patch into one part for backporting and one part for
master-only and post both patches, clearly indicating which is which.


Attached are the full patch for head and the backport part (the patch name 
ends with "backport") separated.


--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index cc80b3f..1a4c1f4 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1075,12 +1075,12 @@ END;
The format of the log is:
 
 
-client_id transaction_no time file_no time_epoch time_us schedule_lag
+client_id transaction_no time script_no time_epoch time_us schedule_lag
 
 
where time is the total elapsed transaction time in microseconds,
-   file_no identifies which script file was used
-   (useful when multiple scripts were specified with -f),
+   script_no identifies which script file was used (useful when
+   multiple scripts were specified with -f or -b),
and time_epoch/time_us are a
Unix epoch format time stamp and an offset
in microseconds (suitable for creating an ISO 8601
@@ -1089,10 +1089,8 @@ END;
Field schedule_lag is the difference between the
transaction's scheduled start time, and the time it actually started, in
microseconds. It is only present when the --rate option is used.
-   The last field skipped_transactions reports the number of
-   transactions skipped because they were too far behind schedule. It is only
-   present when both options --rate and --latency-limit
-   are used.
+   When both options --rate and --latency-limit are used,
+   the skipped transactions will use skipped in place of latency.
   
 
   
@@ -1117,7 +1115,8 @@ END;
 
In this example, transaction 82 was late, because it's latency (6.173 ms) was
over the 5 ms limit. The next two transactions were skipped, because they
-   were already late before they were even started.
+   were already late before they were even started (5217 and 5099
+   microsecond schedule lags on the last column are above the 5 ms limit).
   
 
   
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index c6d1454..e82db70 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1112,10 +1112,8 @@ END;
Field schedule_lag is the difference between the
transaction's scheduled start time, and the time it actually started, in
microseconds. It is only present when the --rate option is used.
-   The last field skipped_transactions reports the number of
-   transactions skipped because they were too far behind schedule. It is only
-   present when both options --rate and --latency-limit
-   are used.
+   When both options --rate and --latency-limit are used,
+   the skipped transactions will use skipped in place of latency.
   
 
   

-- 
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] Request - repeat value of \pset title during \watch interations

[Robert Haas]

On Mon, Mar 21, 2016 at 2:09 PM, David G. Johnston
 wrote:
> On Monday, March 21, 2016, Tom Lane  wrote:
>> "David G. Johnston"  writes:
>> > I'd rather not omit sleep but removing "Watch every" is fine (preferred
>> > actually), so:
>> > Title Is Here Mon Mar 21 15:05:06 2016 (5s)
>>
>> Meh ... seems a bit awkward to me.  Couldn't you include " (5s)" in the
>> title, if you want that info?  If it's variable, you could still
>> accommodate that:
>
> Actually, only if it's a variable that you setup and repeat and you show.  A
> bit cumbersome and mixes the parts that are title and those that are present
> only because you are watching.

Ah, come on.  This doesn't really seem like an issue we should spend
more time quibbling about.  I think Tom's version is 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

Re: [HACKERS] Parallel Aggregate

[David Rowley]

On 22 March 2016 at 02:35, Robert Haas  wrote:
> I have committed this after changing some of the comments.
>
> There might still be bugs ... but I don't see them.  And the speedups
> look very impressive.
>
> Really nice work, David.

Thanks for that, and thank you for taking the time to carefully review
it and commit it.

-- 
 David Rowley   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] Request - repeat value of \pset title during \watch interations

[David G. Johnston]

On Monday, March 21, 2016, Tom Lane  wrote:

> "David G. Johnston" > writes:
> > I'd rather not omit sleep but removing "Watch every" is fine (preferred
> > actually), so:
> > Title Is Here Mon Mar 21 15:05:06 2016 (5s)
>
> Meh ... seems a bit awkward to me.  Couldn't you include " (5s)" in the
> title, if you want that info?  If it's variable, you could still
> accommodate that:


Actually, only if it's a variable that you setup and repeat and you show.
A bit cumbersome and mixes the parts that are title and those that are
present only because you are watching.


> regression=# \set delay 5
> regression=# \pset title 'My Title (':delay' s)'
> Title is "My Title (5 s)".
> regression=# select repeat('xyzzy',12) \watch :delay
>

David J.

Re: [HACKERS] checkpointer continuous flushing

[Tomas Vondra]

Hi,

I've repeated the tests, but this time logged details for 5% of the 
transaction (instead of aggregating the data for each second). I've also 
made the tests shorter - just 12 hours instead of 24, to reduce the time 
needed to complete the benchmark.


Overall, this means ~300M transactions in total for the un-throttled 
case, so sample with ~15M transactions available when computing the 
following charts.


I've used the same commits as during the previous testing, i.e. a298a1e0 
(before patches) and 23a27b03 (with patches).


One interesting difference is that while the "patched" version resulted 
in slightly better performance (8122 vs. 8000 tps), the "unpatched" 
version got considerably slower (6790 vs. 7725 tps) - that's ~13% 
difference, so not negligible. Not sure what's the cause - the 
configuration was exactly the same, there's nothing in the log and the 
machine was dedicated to the testing. The only explanation I have is 
that the unpatched code is a bit more unstable when it comes to this 
type of stress testing.


There results (including scripts for generating the charts) are here:

https://github.com/tvondra/flushing-benchmark-2

Attached are three charts - again, those are using CDF to illustrate the 
distributions and compare them easily:


1) regular-latency.png

The two curves intersect at ~4ms, where both CDF reach ~85%. For the 
shorter transactions, the old code is slightly faster (i.e. apparently 
there's some per-transaction overhead). For higher latencies though, the 
patched code is clearly winning - there are far fewer transactions over 
6ms, which makes a huge difference. (Notice the x-axis is actually 
log-scale, so the tail on the old code is actually much longer than it 
might appear.)


2) throttled-latency.png

In the throttled case (i.e. when the system is not 100% utilized, so 
it's more representative of actual production use), the difference is 
quite clearly in favor of the new code.


3) throttled-schedule-lag.png

Mostly just an alternative view on the previous chart, showing how much 
later the transactions were scheduled. Again, the new code is winning.



regards

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

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

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Tom Lane]

"David G. Johnston"  writes:
> I'd rather not omit sleep but removing "Watch every" is fine (preferred
> actually), so:
> Title Is Here Mon Mar 21 15:05:06 2016 (5s)

Meh ... seems a bit awkward to me.  Couldn't you include " (5s)" in the
title, if you want that info?  If it's variable, you could still
accommodate that:

regression=# \set delay 5
regression=# \pset title 'My Title (':delay' s)'
Title is "My Title (5 s)".
regression=# select repeat('xyzzy',12) \watch :delay
   My Title (5 s)   Mon Mar 21 13:39:25 2016

repeat
--
 xyzzyxyzzyxyzzyxyzzyxyzzyxyzzyxyzzyxyzzyxyzzyxyzzyxyzzyxyzzy
(1 row)

But I don't care enough to veto it.
Anyone else have an opinion?

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] jsonb array-style subscription

[David Steele]

On 3/20/16 2:29 PM, Dmitry Dolgov wrote:

 > Do you have an updated patch ready?

No, I'm afraid it will not be ready for Monday.


I have marked this "returned with feedback".  Please feel free to submit 
a reworked patch for 9.7!


--
-David
da...@pgmasters.net


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

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[David G. Johnston]

On Mon, Mar 21, 2016 at 10:14 AM, Tom Lane  wrote:

> Robert Haas  writes:
> > Well, the title isn't normally centered, but yeah, that is odd.  Yeah,
> > that is odd.  Come to think of it, I think I might have expected the
> > title to appear *above* "Watch every %s", not below it.  That might
> > decrease the oddness.
>
> AFAICS, it appears *beside* it with this patch.  It's only below if the
> terminal is narrow enough that it wraps to there.
>
> > As for letting the committer decide, I don't care about this
> > personally at all, so I'm only looking at it to be nice to the people
> > who do.  Whatever is the consensus is OK with me.  I just don't want
> > to get yelled at later for committing something here, so it would be
> > nice to see a few votes for whatever we're gonna do here.
>
> I'm still of the opinion that what would make the most sense is to replace
> the "Watch every Ns" text with the user-given title, if there is one.
> I ran that up the flagpole already and didn't get a lot of salutes, but
> it seems to respond to your concern that the user title ought to be first.
>
> Regardless of that, I concur with your complaints about coding style, in
> particular with the need to repeat the magic constant 50 in several
> places.  Also, I think the patch makes do_watch return the wrong result
> code for the (typical) case where we exit because of query cancel not
> PSQLexecWatch failure.
>
> So on the whole, I'd do it as attached.
>
​
I'd rather not omit sleep but removing "Watch every" is fine (preferred
actually), so:

if (user_title)​
​snprintf(title, title_len, "%s\t%s (%ld​s)", user_title,
asctime(localtime()), sleep)

"""
Title Is Here Mon Mar 21 15:05:06 2016 (5s)

col1
-
1
"""

David J.

Re: [HACKERS] Proposal: Generic WAL logical messages

[Petr Jelinek]

Just noticed there is missing symlink in the pg_xlogdump.

--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
>From f47730e5e8ef5797c7595aafcbf8cff3b375d0ad Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Wed, 24 Feb 2016 17:02:36 +0100
Subject: [PATCH] Logical Decoding Messages

---
 contrib/test_decoding/Makefile  |  2 +-
 contrib/test_decoding/expected/ddl.out  | 21 --
 contrib/test_decoding/expected/messages.out | 56 
 contrib/test_decoding/sql/ddl.sql   |  3 +-
 contrib/test_decoding/sql/messages.sql  | 17 +
 contrib/test_decoding/test_decoding.c   | 17 +
 doc/src/sgml/func.sgml  | 45 +
 doc/src/sgml/logicaldecoding.sgml   | 34 ++
 src/backend/access/rmgrdesc/Makefile|  4 +-
 src/backend/access/rmgrdesc/logicalmsgdesc.c| 41 
 src/backend/access/transam/rmgr.c   |  1 +
 src/backend/replication/logical/Makefile|  2 +-
 src/backend/replication/logical/decode.c| 49 ++
 src/backend/replication/logical/logical.c   | 37 +++
 src/backend/replication/logical/logicalfuncs.c  | 27 
 src/backend/replication/logical/message.c   | 87 +
 src/backend/replication/logical/reorderbuffer.c | 68 +++
 src/backend/replication/logical/snapbuild.c | 19 ++
 src/bin/pg_xlogdump/logicalmsgdesc.c|  1 +
 src/bin/pg_xlogdump/rmgrdesc.c  |  1 +
 src/include/access/rmgrlist.h   |  1 +
 src/include/catalog/pg_proc.h   |  4 ++
 src/include/replication/logicalfuncs.h  |  2 +
 src/include/replication/message.h   | 41 
 src/include/replication/output_plugin.h | 12 
 src/include/replication/reorderbuffer.h | 20 ++
 src/include/replication/snapbuild.h |  2 +
 27 files changed, 602 insertions(+), 12 deletions(-)
 create mode 100644 contrib/test_decoding/expected/messages.out
 create mode 100644 contrib/test_decoding/sql/messages.sql
 create mode 100644 src/backend/access/rmgrdesc/logicalmsgdesc.c
 create mode 100644 src/backend/replication/logical/message.c
 create mode 12 src/bin/pg_xlogdump/logicalmsgdesc.c
 create mode 100644 src/include/replication/message.h

diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 06c9546..309cb0b 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -38,7 +38,7 @@ submake-test_decoding:
 	$(MAKE) -C $(top_builddir)/contrib/test_decoding
 
 REGRESSCHECKS=ddl xact rewrite toast permissions decoding_in_xact \
-	decoding_into_rel binary prepared replorigin time
+	decoding_into_rel binary prepared replorigin time messages
 
 regresscheck: | submake-regress submake-test_decoding temp-install
 	$(MKDIR_P) regression_output
diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out
index 77719e8..32cd24d 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -220,11 +220,17 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
 (7 rows)
 
 /*
- * check that disk spooling works
+ * check that disk spooling works (also for logical messages)
  */
 BEGIN;
 CREATE TABLE tr_etoomuch (id serial primary key, data int);
 INSERT INTO tr_etoomuch(data) SELECT g.i FROM generate_series(1, 10234) g(i);
+SELECT 'tx logical msg' FROM pg_logical_emit_message(true, 'test', 'tx logical msg');
+?column?
+
+ tx logical msg
+(1 row)
+
 DELETE FROM tr_etoomuch WHERE id < 5000;
 UPDATE tr_etoomuch SET data = - data WHERE id > 5000;
 COMMIT;
@@ -233,12 +239,13 @@ SELECT count(*), min(data), max(data)
 FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1')
 GROUP BY substring(data, 1, 24)
 ORDER BY 1,2;
- count |   min   |  max   
+-+
- 1 | BEGIN   | BEGIN
- 1 | COMMIT  | COMMIT
- 20467 | table public.tr_etoomuch: DELETE: id[integer]:1 | table public.tr_etoomuch: UPDATE: id[integer]: data[integer]:-
-(3 rows)
+ count |  min  |  max   
+---+---+
+ 1 | BEGIN  

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Tom Lane]

Robert Haas  writes:
> Well, the title isn't normally centered, but yeah, that is odd.  Yeah,
> that is odd.  Come to think of it, I think I might have expected the
> title to appear *above* "Watch every %s", not below it.  That might
> decrease the oddness.

AFAICS, it appears *beside* it with this patch.  It's only below if the
terminal is narrow enough that it wraps to there.

> As for letting the committer decide, I don't care about this
> personally at all, so I'm only looking at it to be nice to the people
> who do.  Whatever is the consensus is OK with me.  I just don't want
> to get yelled at later for committing something here, so it would be
> nice to see a few votes for whatever we're gonna do here.

I'm still of the opinion that what would make the most sense is to replace
the "Watch every Ns" text with the user-given title, if there is one.
I ran that up the flagpole already and didn't get a lot of salutes, but
it seems to respond to your concern that the user title ought to be first.

Regardless of that, I concur with your complaints about coding style, in
particular with the need to repeat the magic constant 50 in several
places.  Also, I think the patch makes do_watch return the wrong result
code for the (typical) case where we exit because of query cancel not
PSQLexecWatch failure.

So on the whole, I'd do it as attached.

regards, tom lane

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index eef6e4b..a309109 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** static bool
*** 3020,3026 
  do_watch(PQExpBuffer query_buf, long sleep)
  {
  	printQueryOpt myopt = pset.popt;
! 	char		title[50];
  
  	if (!query_buf || query_buf->len <= 0)
  	{
--- 3020,3029 
  do_watch(PQExpBuffer query_buf, long sleep)
  {
  	printQueryOpt myopt = pset.popt;
! 	const char	 *user_title;
! 	char		 *title;
! 	int			  title_len;
! 	int			  res = 0;
  
  	if (!query_buf || query_buf->len <= 0)
  	{
*** do_watch(PQExpBuffer query_buf, long sle
*** 3034,3042 
  	 */
  	myopt.topt.pager = 0;
  
  	for (;;)
  	{
- 		int			res;
  		time_t		timer;
  		long		i;
  
--- 3037,3055 
  	 */
  	myopt.topt.pager = 0;
  
+ 	/*
+ 	 * If there's a title in the user configuration, make sure we have room
+ 	 * for it in the title buffer.
+ 	 */
+ 	user_title = myopt.title;
+ 	if (user_title)
+ 		title_len = strlen(user_title) + 50;
+ 	else
+ 		title_len = 50;
+ 	title = pg_malloc(title_len);
+ 
  	for (;;)
  	{
  		time_t		timer;
  		long		i;
  
*** do_watch(PQExpBuffer query_buf, long sle
*** 3045,3052 
  		 * of completion of the command?
  		 */
  		timer = time(NULL);
! 		snprintf(title, sizeof(title), _("Watch every %lds\t%s"),
!  sleep, asctime(localtime()));
  		myopt.title = title;
  
  		/* Run the query and print out the results */
--- 3058,3071 
  		 * of completion of the command?
  		 */
  		timer = time(NULL);
! 		if (user_title)
! 			snprintf(title, title_len,
! 	 "%s\t%s",
! 	 user_title, asctime(localtime()));
! 		else
! 			snprintf(title, title_len,
! 	 _("Watch every %lds\t%s"),
! 	 sleep, asctime(localtime()));
  		myopt.title = title;
  
  		/* Run the query and print out the results */
*** do_watch(PQExpBuffer query_buf, long sle
*** 3056,3065 
  		 * PSQLexecWatch handles the case where we can no longer repeat the
  		 * query, and returns 0 or -1.
  		 */
! 		if (res == 0)
  			break;
- 		if (res == -1)
- 			return false;
  
  		/*
  		 * Set up cancellation of 'watch' via SIGINT.  We redo this each time
--- 3075,3082 
  		 * PSQLexecWatch handles the case where we can no longer repeat the
  		 * query, and returns 0 or -1.
  		 */
! 		if (res == 0 || res == -1)
  			break;
  
  		/*
  		 * Set up cancellation of 'watch' via SIGINT.  We redo this each time
*** do_watch(PQExpBuffer query_buf, long sle
*** 3084,3090 
  		sigint_interrupt_enabled = false;
  	}
  
! 	return true;
  }
  
  /*
--- 3101,3108 
  		sigint_interrupt_enabled = false;
  	}
  
! 	pg_free(title);
! 	return (res >= 0);
  }
  
  /*

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

Re: [HACKERS] Speedup twophase transactions

[Jesper Pedersen]

On 03/18/2016 12:50 PM, Stas Kelvich wrote:

On 11 Mar 2016, at 19:41, Jesper Pedersen  wrote:



Thanks for review, Jesper.


Some comments:

* The patch needs a rebase against the latest TwoPhaseFileHeader change


Done.


* Rework the check.sh script into a TAP test case (src/test/recovery), as 
suggested by Alvaro and Michael down thread


Done. Originally I thought about reducing number of tests (11 right now), but 
now, after some debugging, I’m more convinced that it is better to include them 
all, as they are really testing different code paths.


* Add documentation for RecoverPreparedFromXLOG


Done.


Thanks, Stas.

I have gone over this version, and tested with --enable-tap-tests + make 
check in src/test/recovery, which passes.


Simon, do you want to move this entry to "Ready for Committer" and take 
the 2REVIEWER section as part of that, or leave it in "Needs Review" 
and update the thread ?


Best regards,
 Jesper



--
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] we have added support for box type in SP-GiST index

[Teodor Sigaev]

+ *implementation of quad-4d tree over boxes for SP-GiST.

Isn't the whole thing actually 3D?
No. The idea if this work is a representation of 2d box as 4d point. Quad means 
quadrant of 2d plane. Originally such kind of tree was developed for 2d point, 
and each node of tree splits plane on 4 quadrant. For 3d tree each node splits 
space for 8 "quadrants", 4d - 16 ones.



+  * For example consider the case of intersection.

No need for a new line after this.

fixed


+  * A quadrant has bounds, but sp-gist keeps only 4-d point (box) in inner 
nodes.

I couldn't get the term 4D point.  Maybe it means that we are using
box datatype as the prefix, but we are not treating them as boxes.

exactly, we treat boxes as 4-dimentional points.




+ * We use traversalValue to calculate quadrant bounds from parent's quadrant
+ * bounds.

I couldn't understand this sentence.  We are using the parent's prefix
and the quadrant to generate the traversalValue.  I think this is the
crucial part of the opclass.  It would be nice to explain it more
clearly.


I'll try to explain with two-dimensional example over points. ASCII-art:
   |
   |
1  |  2
   |
---+-
   |P
 3 |  4
   |
   |

'+' with 'A' represents a centroid or, other words, point which splits plane for 
4 quadrants and it strorend in parent node. 1,2,3,4 are labels of quadrants, 
each labeling will be the same for all pictures and all centriods, and i will 
not show them in pictures later to prevent too complicated images. Let we add C 
- child node (and again, it will split plane for 4 quads):



   | |
   +-+---
 X |B|C
   | |
---+-+---
   |P|A
   | |
   |
   |

A and B are points of intersection of lines. So, box PBCAis a bounding box for 
points contained in 3-rd (see labeling above). For example X labeled point is 
not a descendace of child node with centroid  C because it must be in branch of 
1-st quad of parent node. So, each node (except root) will have a limitation in 
its quadrant. To transfer that limitation the traversalValue is used.


To keep formatting I attached a txt file with this fragment.




+  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group

2016.

fixed




+ *  src/backend/utils/adt/boxtype_spgist.c

Maybe we should name this file as geo_spgist.c to support other types
in the future.

done


+ #include "utils/builtins.h";

Extra ;.

fixed




+ #include "utils/datum.h"

I think this is unnecessary.

removed




+ /* InfR type implements doubles and +- infinity */
+ typedef struct
+ {
+int infFlag;
+double  val;
+ }   InfR;

Why do we need this?  Can't we store infinity in "double"?

Hmm, you are right. fixed.


This isn't returning the value.

removed




+ typedef struct
+ {
+Range   range_x;
+Range   range_y;
+ }   Rectangle;


Wouldn't it be simpler to just using BOX instead of this?

Too much the same names in RectBox




+ static void
+ evalIRangeBox(const IRangeBox *range_box, const Range *range, const int half1,
+  const int half2, IRangeBox *new_range_box)

+ static void
+ evalIRectBox(const IRectBox *rect_box, const Rectangle *centroid,
+ const uint8 quadrant, IRectBox * new_rect_box)

+ inline static void
+ initializeUnboundedBox(IRectBox * rect_box)


Wouldn't it be simpler to palloc and return the value on those functions?

evalRangeBox() initializes part of RectBox, so, it could not palloc its result.
Other methods use the same signature just for consistency.




+ static int
+ intersect2D(const Range * range, const IRangeBox * range_box)


Wouldn't it be better to return "bool" on those functions.

done




+return ((p1 >= 0) && (p2 <= 0));

Extra parentheses.

removed




+ iconst spgChooseIn *in = (spgChooseIn *) PG_GETARG_POINTER(0);
+ ispgChooseOut *out = (spgChooseOut *) PG_GETARG_POINTER(1);


Many variables are defined with "const".  I am not sure they are all
that doesn't change.  If it applies to the pointer, "out" should also
not change in here.  Am I wrong?

No, changes




+/*
+ * Begin. This block evaluates the median of coordinates of boxes
+ */

I would rather explain what the function does on the function header.

fixed




+ memcpy(new_rect_box, rect_box, sizeof(IRectBox));

Do we really need to copy the traversalValues on allTheSame case.
Wouldn't it work if just the same value is passed for all of them.
The search shouldn't continue after allTheSame case.

Seems, yes. spgist tree could contain a locng branches with allTheSame.




+ for (i = 0; flag && i < in->nkeys && flag; i++)

It checks flag two times.

fixed




+boxPointerToRectangle(
+DatumGetBoxP(in->scankeys[i].sk_argument),
+p_query_rect
+   

Re: [HACKERS] WIP: Covering + unique indexes.

[Anastasia Lubennikova]

19.03.2016 08:00, Peter Geoghegan:

On Fri, Mar 18, 2016 at 5:15 AM, David Steele  wrote:

It looks like this patch should be marked "needs review" and I have done so.

Uh, no it shouldn't. I've posted an extensive review on the original
design thread. See CF entry:

https://commitfest.postgresql.org/9/433/

Marked "Waiting on Author".

Thanks to David,
I've missed these letters at first.
I'll answer here.


* You truncate (remove suffix attributes -- the "included" attributes)
within _bt_insertonpg():

-   right_item = CopyIndexTuple(item);
+   indnatts = IndexRelationGetNumberOfAttributes(rel);
+   indnkeyatts = IndexRelationGetNumberOfKeyAttributes(rel);
+
+   if (indnatts != indnkeyatts)
+   {
+   right_item = index_reform_tuple(rel, item, indnatts, indnkeyatts);
+   right_item_sz = IndexTupleDSize(*right_item);
+   right_item_sz = MAXALIGN(right_item_sz);
+   }
+   else
+   right_item = CopyIndexTuple(item);
 ItemPointerSet(&(right_item->t_tid), rbkno, P_HIKEY);

I suggest that you do this within _bt_insert_parent(), instead, iff
the original target page is know to be a leaf page. That's where it
needs to happen for conventional suffix truncation, which has special
considerations when determining which attributes are safe to truncate
(or even which byte in the first distinguishing attribute it is okay
to truncate past)


I agree that _bt_insertonpg() is not right for truncation.
Furthermore, I've noticed that all internal keys are solely the copies 
of "High keys" from the leaf pages. Which is pretty logical.
Therefore, if we have already truncated the tuple, when it became a High 
key, we do not need the same truncation within _bt_insert_parent() or 
any other function.
So the only thing to worry about is the HighKey truncation. I rewrote 
the code. Now only _bt_split cares about truncation.


It's a bit more complicated to add it into index creation algorithm.
There's a trick with a "high key".
/*
 * We copy the last item on the page into the new page, and then
 * rearrange the old page so that the 'last item' becomes its 
high key

 * rather than a true data item.  There had better be at least two
 * items on the page already, else the page would be empty of 
useful

 * data.
 */
/*
 * Move 'last' into the high key position on opage
 */

To be consistent with other steps of algorithm ( all high keys must be 
truncated tuples), I had to update this high key on place:

delete the old one, and insert truncated high key.
The very same logic I use to truncate posting list of a compressed tuple 
in the "btree_compression" patch. [1]

I hope, both patches will be accepted, and then I'll thoroughly merge them .


* I think the comparison logic may have a bug.

Does this work with amcheck? Maybe it works with bt_index_check(), but
not bt_index_parent_check()? I think that you need to make sure that
_bt_compare() knows about this, too. That's because it isn't good
enough to let a truncated internal IndexTuple compare equal to a
scankey when non-truncated attributes are equal.


It is a very important issue. But I don't think it's a bug there.
I've read amcheck sources thoroughly and found that the problem appears at
"invariant_key_less_than_equal_nontarget_offset()


static bool
invariant_key_less_than_equal_nontarget_offset(BtreeCheckState *state,
   Page nontarget, ScanKey key,
   OffsetNumber upperbound)
{
int16natts = state->rel->rd_rel->relnatts;
int32cmp;

cmp = _bt_compare(state->rel, natts, key, nontarget, upperbound);

return cmp <= 0;
}

It uses scankey, made with _bt_mkscankey() which uses only key 
attributes, but calls _bt_compare with wrong keysz.
If we wiil use nkeyatts = state->rel->rd_index->relnatts; instead of 
natts, all the checks would be passed successfully.


Same for invariant_key_greater_than_equal_offset() and 
invariant_key_less_than_equal_nontarget_offset().


In my view, it's the correct way to fix this problem, because the caller 
is responsible for passing proper arguments to the function.
Of course I will add a check into bt_compare, but I'd rather make it an 
assertion (see the patch attached).


I'll add a flag to distinguish regular and truncated tuples, but it will 
not be used in this patch. Please, comment, if I've missed something.
As you've already mentioned, neither high keys, nor tuples on internal 
pages are using  "itup->t_tid.ip_posid", so I'll take one bit of it.


It will definitely require changes in the future works on suffix 
truncation or something like that, but IMHO for now it's enough.


Do you have any objections or comments?

[1] https://commitfest.postgresql.org/9/494/

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/contrib/dblink/dblink.c 

Re: [HACKERS] OOM in libpq and infinite loop with getCopyStart()

[Robert Haas]

On Tue, Mar 1, 2016 at 12:38 AM, Michael Paquier
 wrote:
> Thoughts? I have registered that in the CF app, and a patch is attached.

It is very difficult to believe that this is a good idea:

--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -445,6 +445,7 @@ libpqrcv_PQexec(const char *query)
 if (PQresultStatus(lastResult) == PGRES_COPY_IN ||
 PQresultStatus(lastResult) == PGRES_COPY_OUT ||
 PQresultStatus(lastResult) == PGRES_COPY_BOTH ||
+PQresultStatus(lastResult) == PGRES_FATAL_ERROR ||
 PQstatus(streamConn) == CONNECTION_BAD)
 break;
 }

I mean, why would it be a good idea to blindly skip over fatal errors?

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

[David Rowley]

On 20 March 2016 at 16:48, David Rowley  wrote:
> I've attached another series of patches:
>
> 0001: This is the latest Parallel Aggregate Patch, not intended for
> review here, but is required for the remaining patches. This patch has
> changed quite a bit from the previous one that I posted here, and the
> remaining patches needed re-based due to those changes.
>
> 0002: Adds serial/de-serial function support to CREATE AGGREGATE,
> contains minor fix-ups from last version.
>
> 0003: Adds various combine/serial/de-serial functions for the standard
> set of aggregates, apart from most float8 aggregates.
>
> 0004: Adds regression tests for 0003 pg_aggregate.h changes.
>
> 0005: Documents to mention which aggregate functions support partial mode.
>
> 0006: Re-based version of Haribabu's floating point aggregate support,
> containing some fixes by me.

Now that parallel aggregate has been committed, 0002 no longer applies.
Please find attached a new 0001 patch, intended to replace the
previous 0001 and 0002.

The previous 0003, 0004, 0005 and 0006 should still apply onto the new 0001.

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


0001-Allow-INTERNAL-state-aggregates-to-participate-in-pa_2016-03-22.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] proposal: PL/Pythonu - function ereport

[Robert Haas]

On Wed, Mar 2, 2016 at 12:01 AM, Pavel Stehule  wrote:
>> I see the pfree you added isn't allowed on a NULL pointer but as far
>> as I see message is guaranteed not to be NULL as dgettext never
>> returns NULL.
>>
>> I'll mark this Ready for Committer.
>
> Thank you very much

This patch needs a committer.  Any volunteers?

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

[Robert Haas]

On Sat, Mar 19, 2016 at 11:48 PM, David Rowley
 wrote:
> 0002: Adds serial/de-serial function support to CREATE AGGREGATE,
> contains minor fix-ups from last version.

This looks pretty good, but don't build_aggregate_serialfn_expr and
build_aggregate_deserialfn_expr compile down to identical machine
code?  Keeping two copies of the same code with different parameter
names is a degree of neatnik-ism I'm not sure I can swallow.

The only caller to make_partialgroup_input_target() passes true for
the additional argument.  That doesn't seem right.

Maybe error messages should refer to "aggregate serialization
function" and "aggregate deserialization function" instead of
"aggregate serial function" and "aggregate de-serial function".

- *   Other behavior is also supported and is controlled by the
'combineStates'
- *   and 'finalizeAggs'. 'combineStates' controls whether the trans func or
- *   the combine func is used during aggregation.  When 'combineStates' is
- *   true we expect other (previously) aggregated states as input
rather than
- *   input tuples. This mode facilitates multiple aggregate stages which
- *   allows us to support pushing aggregation down deeper into
the plan rather
- *   than leaving it for the final stage. For example with a query such as:
+ *   Other behavior is also supported and is controlled by the
'combineStates',
+ *   'finalizeAggs' and 'serialStates' parameters. 'combineStates' controls
+ *   whether the trans func or the combine func is used during aggregation.
+ *   When 'combineStates' is true we expect other (previously) aggregated
+ *   states as input rather than input tuples. This mode
facilitates multiple
+ *   aggregate stages which allows us to support pushing aggregation down
+ *   deeper into the plan rather than leaving it for the final stage. For
+ *   example with a query such as:

I'd omit this hunk.  The serialStates thing is really separate from
what's being talked about here, and you discuss it further down.

-- 
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] BRIN is missing in multicolumn indexes documentation

[Petr Jediný]

Hello,

the http://www.postgresql.org/docs/9.5/static/indexes-multicolumn.html
page doesn't mention BRIN support, but according to the
http://www.postgresql.org/docs/9.5/static/sql-createindex.html it is
supported in multicolumn setup.

The attached patch (git diff against master branch) fixes the omission
and adds "Oxford comma" before "and" to unify the writing style with
the style used in the "create index" documentation.

Regards,

Petr Jediny


brin-multicolumn-doc-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] [patch] Proposal for \crosstabview in psql

[Alvaro Herrera]

Robert Haas wrote:
> On Sun, Mar 20, 2016 at 5:27 PM, Pavel Stehule  
> wrote:
> > From my perspective, it is ready for commiter. Daniel solved the most big
> > issues.
> 
> OK, so that brings us back to: is there any committer who likes this
> enough to want to look at committing it?  My view hasn't changed much
> since 
> http://www.postgresql.org/message-id/ca+tgmoz4yaduq9j8xtgrbh868jh2nj_nw_qgkxb32cedsvt...@mail.gmail.com

I volunteer for that, but it'll be a few days so if anyone else is
interested, feel free.

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


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

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

[Alvaro Herrera]

Abhijit Menon-Sen wrote:

> + else if (strcmp(defel->defname, "extdepend") == 0)
> + {
> + if (*extdepend_item)
> + goto duplicate_error;
> +
> + *extdepend_item = defel;
> + }
>   else
>   return false;
>  

I'm not sure I agree with this implementation.  I mentioned ALTER ..
SET SCHEMA and ALTER .. OWNER TO as examples because, since other object
types were mentioned as possible targets for this command, then this
should presumably object-type-agnostic, like those ALTER forms are.  So
IMO we shouldn't shoehorn this into AlterFunctionStmt but rather have
its own node AlterObjectDepends or similar.

The other point is that if we're doing it in ALTER FUNCTION which allows
multiple subcommands in one go, why do we not allow to run this command
for multiple extensions?  After all, it's not completely stupid to think
that one function could depend on multiple extensions, and so if you
agree with that then it's not completely stupid that it should be
possible to declare such in one command, i.e.

ALTER FUNCTION .. DEPENDS ON EXTENSION one, two;
  or perhaps
ALTER FUNCTION .. DEPENDS ON EXTENSION one, DEPENDS ON EXTENSION two;

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


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

Re: [HACKERS] [patch] Proposal for \crosstabview in psql

[Robert Haas]

On Sun, Mar 20, 2016 at 5:27 PM, Pavel Stehule  wrote:
> From my perspective, it is ready for commiter. Daniel solved the most big
> issues.

OK, so that brings us back to: is there any committer who likes this
enough to want to look at committing it?  My view hasn't changed much
since 
http://www.postgresql.org/message-id/ca+tgmoz4yaduq9j8xtgrbh868jh2nj_nw_qgkxb32cedsvt...@mail.gmail.com

-- 
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] [GENERAL] Request - repeat value of \pset title during \watch interations

[Robert Haas]

On Mon, Mar 21, 2016 at 11:17 AM, David G. Johnston
 wrote:
>> And does everybody agree that this is a desirable change?
>
> Adding the title is desirable.  While I'm inclined to bike-shed this
> anything that gets it in I can live with and so I'm content letting the
> author/committer decide where exactly things (including whitespace) appear.
>
> It is a bit odd that the "Watch every %s" gets centered if the result is
> wide but that the title remains left-aligned.

Well, the title isn't normally centered, but yeah, that is odd.  Yeah,
that is odd.  Come to think of it, I think I might have expected the
title to appear *above* "Watch every %s", not below it.  That might
decrease the oddness.

As for letting the committer decide, I don't care about this
personally at all, so I'm only looking at it to be nice to the people
who do.  Whatever is the consensus is OK with me.  I just don't want
to get yelled at later for committing something here, so it would be
nice to see a few votes for whatever we're gonna do here.

-- 
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] [GENERAL] Request - repeat value of \pset title during \watch interations

[David G. Johnston]

On Mon, Mar 21, 2016 at 8:03 AM, Robert Haas  wrote:

> On Sun, Mar 20, 2016 at 9:31 AM, Michael Paquier
>  wrote:
> > And the patch attached gives the following output:
> > With title:
> > =# \watch 1
> > Watch every 1sSun Mar 20 22:28:38 2016
> > popo
> >  a
> > ---
> >  1
> > (1 row)
>

​This doesn't show the blank line above "popo" that the prior example
had...​

>
> > And without title:
> > Watch every 1sSun Mar 20 22:29:31 2016
> >
> >  a
> > ---
> >  1
> > (1 row)
>
>
​Unchanged from present behavior - but its not obvious that the watch line
is center-aligned​

And does everybody agree that this is a desirable change?
>

​Adding the title is desirable.  While I'm inclined to bike-shed this
anything that gets it in I can live with and so I'm content letting the
author/committer decide where exactly things (including whitespace) appear.

​It is a bit odd that the "Watch every %s" gets centered if the result
is wide but that the title remains left-aligned.

The minimally invasive change would be the following:

>optional title<
>watch<
>(blank line)
>headers
>head-body divider
>body
>optional footer

Though I like the idea of:

>optional title
>(blank line - if Title present)
>headers
>head-body divider
>body
>optional footer
>watch

​David J.​

Re: [HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Aleksander Alekseev]

> I'm new here so someone more experienced would have to weigh in,
> but I would wonder a couple of things:
> 
> a. whether a braced struct assignment is supported in every
>C compiler that PostgreSQL still intends to support
> 
> b. whether such a struct assignment is guaranteed to initialize
>padding spaces as well as declared fields (in all supported
>C versions/compilers).
> 
> It's possible that memset() would be more convincing.

Frankly I'm not sure regarding all supported C versions/compilers. But
it seems to be a valid ANSI C. Here is a test program:

```
#include 

typedef struct {
  int i;
  char c;
  long l;
  short s;
} MyStruct;

int main()
{
  int i, sum = 0;
  char *c;
  MyStruct s = {0};

  s.i = 11;
  s.c = 22;
  s.l = 33;
  s.s = 44;

  c = (char*)
  for(i = 0; i < sizeof(s); i++) {
sum += *c;
c++;
  }

  printf("Sum: %d\n", sum);

  return 0;
}
```

I compiled it with various versions of GCC and CLang with different
optimization flags:

clang38 -O3 -ansi -g t.c -o t
gcc -O0 -ansi -g t.c -o t

In all cases running a program under debugger shows that structure is
properly initialized:

(gdb) b main
Breakpoint 1 at 0x4007ae: file t.c, line 12.
(gdb) r
Starting program: /usr/home/eax/temp/t 

Breakpoint 1, main () at t.c:12
12   int i, sum = 0;
(gdb) p memset(, 0xEA, sizeof(MyStruct))
$1 = -5376
(gdb) x/24xb 
0x7fffeb00: 0xea 0xea 0xea 0xea 0xea 0xea 0xea 0xea
0x7fffeb08: 0xea 0xea 0xea 0xea 0xea 0xea 0xea 0xea
0x7fffeb10: 0xea 0xea 0xea 0xea 0xea 0xea 0xea 0xea
(gdb) n
14   MyStruct s = {0};
(gdb) 
16   s.i = 11;
(gdb) x/24xb 
0x7fffeb00: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x7fffeb08: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x7fffeb10: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
(gdb) quit

Naturally we could use memset() as well. But I personally find it a bit
less readable. And in theory it doesn't prevent some _very_ "smart" C
compiler from not cleaning the whole structure anyway.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


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

Re: [HACKERS] Proposal: Generic WAL logical messages

[Craig Ringer]

On 18 March 2016 at 20:36, Artur Zakirov  wrote:

> On 17.03.2016 15:42, Craig Ringer wrote:
>
>>
>>
>> Would you mind sharing the plugin here? I could add it to
>> src/test/modules and add some t/ tests so it runs under the TAP test
>> framework.
>>
>>
>> --
>>   Craig Ringer http://www.2ndQuadrant.com/
>>   PostgreSQL Development, 24x7 Support, Training & Services
>>
>
> Of course. I attached it.
>

Thanks for that.

Since it incorporates fairly significant chunks of code from a number of
places and is really a proof of concept rather than demo/example I don't
think it can really be included as a TAP test module, not without more
rewriting than I currently have time for anyway.



> But it shows concept of DDL replication.
>

Yeah, a part of it anyway. As I outlined in another thread some time ago,
getting DDL replication right is fairly tricky and requires more changes
than just capturing the raw DDL (or even deparsed DDL) and sending it over
the wire.

It's a useful proof of concept, but way too big to use as an in-tree
regression for logical WAL messages as I was hoping.

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

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Robert Haas]

On Sun, Mar 20, 2016 at 9:31 AM, Michael Paquier
 wrote:
> And the patch attached gives the following output:
> With title:
> =# \watch 1
> Watch every 1sSun Mar 20 22:28:38 2016
> popo
>  a
> ---
>  1
> (1 row)
>
> And without title:
> Watch every 1sSun Mar 20 22:29:31 2016
>
>  a
> ---
>  1
> (1 row)

And does everybody agree that this is a desirable change?

As for the patch itself, you could replace all this:

+   /*
+* Take into account any title present in the user setup as a part of
+* what is printed for each iteration by using it as a header.
+*/
+   if (myopt.title)
+   {
+   title_len = strlen(myopt.title);
+   title = pg_malloc(title_len + 50);
+   head_title = pg_strdup(myopt.title);
+   }
+   else
+   {
+   title_len = 0;
+   title = pg_malloc(50);
+   head_title = pg_strdup("");
+   }

...with:

head_title = pg_strdup(myopt.title != NULL ? myopt.title : "");
title_len = strlen(head_title);
title = pg_malloc(title_len + 50);

Better yet, include the + 50 in title_len, and then you don't need to
reference the number 50 again further down.

-- 
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] [COMMITTERS] pgsql: Support parallel aggregation.

[Bert]

#woopwoop! :-D great work, all!

On Mon, Mar 21, 2016 at 3:43 PM, Simon Riggs  wrote:

> On 21 March 2016 at 14:35, David Fetter  wrote:
>
>> On Mon, Mar 21, 2016 at 01:33:28PM +, Robert Haas wrote:
>> > Support parallel aggregation.
>>
>> ...and there was much rejoicing!
>>
>
> +1
>
> Well done all.
>
> --
> Simon Riggshttp://www.2ndQuadrant.com/
> 
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>



-- 
Bert Desmet
0477/305361

Re: [HACKERS] max_parallel_degree context level

[Robert Haas]

On Sun, Mar 20, 2016 at 3:01 AM, David Rowley
 wrote:
> On 12 February 2016 at 04:55, Robert Haas  wrote:
>> On Thu, Feb 11, 2016 at 10:32 AM, Simon Riggs  wrote:
>>> Is it slower if you request N workers, yet only 1 is available?
>>
>> I sure hope so.  There may be some cases where more workers are slower
>> than fewer workers, but those cases are defects that we should try to
>> fix.
>
> It would only take anything but the CPU to be a bottleneck for this to
> be highly likely the case.
> If a non-parallel query is bound on I/O, then adding workers is most
> likely going to slow it down further. I've seen this when testing
> parallel aggregates.

Yeah.  If you're bottlenecked on I/O, having more workers fighting
over the limited amount of CPU work available just adds context
switching and communication overhead.  That's not a particularly easy
problem to solve.  One can imagine a system where the workers exit if
they turn out not be needed, but then of course you might end up
needing them later if the situation shifts.  I think eventually we
should have the ability for workers to both dynamically leave queries
that are I/O bound and dynamically join queries that become CPU bound,
but that is going to be a bit more than we can fit into 9.6.

Meanwhile, I made the change that was the original purpose of this thread.

-- 
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] [COMMITTERS] pgsql: Support parallel aggregation.

[Simon Riggs]

On 21 March 2016 at 14:35, David Fetter  wrote:

> On Mon, Mar 21, 2016 at 01:33:28PM +, Robert Haas wrote:
> > Support parallel aggregation.
>
> ...and there was much rejoicing!
>

+1

Well done all.

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

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

Re: [HACKERS] incorrect docs for pgbench / skipped transactions

[Robert Haas]

On Sat, Mar 19, 2016 at 3:28 PM, Fabien COELHO  wrote:
>>> Thanks for the pointer. However, I do not have "editor priviledge" on
>>> this
>>> wiki, maybe Tomas has?
>>
>> I gave you editor privs now, but since it's in 9.5 I guess it needs to
>> be on the bug tracker (Except, of course, we don't have one.)
>
> Ok, I added a reference to the commitfest entry from this wiki page, and a
> note about partial 9.5 backporting.

Please split the patch into one part for backporting and one part for
master-only and post both patches, clearly indicating which is which.

-- 
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] [COMMITTERS] pgsql: Support parallel aggregation.

[Robert Haas]

On Mon, Mar 21, 2016 at 10:35 AM, David Fetter  wrote:
> On Mon, Mar 21, 2016 at 01:33:28PM +, Robert Haas wrote:
>> Support parallel aggregation.
>
> ...and there was much rejoicing!

I know *I* am!

-- 
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] [COMMITTERS] pgsql: Support parallel aggregation.

[David Fetter]

On Mon, Mar 21, 2016 at 01:33:28PM +, Robert Haas wrote:
> Support parallel aggregation.

...and there was much rejoicing!

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Chapman Flack]

On 03/21/2016 10:21 AM, Aleksander Alekseev wrote:

> Well in this case here is a patch that fixes "use of uninitialized
> value" reports by MemorySanitizer I managed to catch so far.

I'm new here so someone more experienced would have to weigh in,
but I would wonder a couple of things:

a. whether a braced struct assignment is supported in every
   C compiler that PostgreSQL still intends to support

b. whether such a struct assignment is guaranteed to initialize
   padding spaces as well as declared fields (in all supported
   C versions/compilers).

It's possible that memset() would be more convincing.

-Chap



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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Ashutosh Bapat]

Thanks Michael for looking into this.



> In get_useful_ecs_for_relation, it seems to me that this assertion
> should be removed and replaces by an actual check because even if
> right_ec and left_ec are initialized, we cannot be sure that ec_relids
> contains the relations specified:
> /*
>  * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
>  * that left_ec and right_ec will be initialized, per comments in
>  * distribute_qual_to_rels, and rel->joininfo should only contain
> ECs
>  * where this relation appears on one side or the other.
>  */
> if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
> useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
>
>  restrictinfo->right_ec);
> else
> {
> Assert(bms_is_subset(relids,
> restrictinfo->left_ec->ec_relids));
> useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
>
> restrictinfo->left_ec);
> }
>

An EC covers all the relations covered by all the equivalence members it
contains. In case of mergejoinable clause for outer join, EC may cover just
a single relation whose column appears on either side of the clause. In
this case, bms_is_subset() for a given join relation covering single
relation in EC will be false. So, we have to use bms_overlap() instead of
bms_is_subset(). The caller get_useful_pathkeys_for_rel() extracts the
equivalence member (if any), which is entirely covered by the given
relation. Otherwise, you are correct that we have to convert the assertion
into a condition. I have added comments in get_useful_ecs_for_relation()
explaining, why.

See for example the attached (with more tests including combinations
> of joins, and three-table joins). I have added an open item for 9.6 on
> the wiki.
>

Thanks for those tests. Actually, that code is relevant for joins which can
not be pushed down to the foreign server. For such joins we try to add
pathkeys which will help merge joins. I have included the relevant tests
rewriting them to use local tables, so that the entire join is not pushed
down to the foreign server.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index a7f32f3..d38ff86 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -471,20 +471,88 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.
  103
  104
  105
  106
  107
  108
  109
  110
 (10 rows)
 
+-- Test similar to above, except that the full join prevents any equivalence
+-- classes from being merged. This produces single relation equivalence classes
+-- included in join restrictions.
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+QUERY PLAN
+--
+ Limit
+   Output: t1."C 1", t2.c1, t3.c1
+   ->  Merge Right Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: (t3.c1 = t1."C 1")
+ ->  Foreign Scan
+   Output: t3.c1, t2.c1
+   Relations: (public.ft2 t3) LEFT JOIN (public.ft1 t2)
+   Remote SQL: SELECT r3."C 1", r2."C 1" FROM ("S 1"."T 1" r3 LEFT JOIN "S 1"."T 1" r2 ON (((r2."C 1" = r3."C 1" ORDER BY r3."C 1" ASC NULLS LAST
+ ->  Index Only Scan using t1_pkey on "S 1"."T 1" t1
+   Output: t1."C 1"
+(11 rows)
+
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ C 1 | c1  | c1  
+-+-+-
+ 101 | 101 | 101
+ 102 | 102 | 102
+ 103 | 103 | 103
+ 104 | 104 | 104
+ 105 | 105 | 105
+ 106 | 106 | 106
+ 107 | 107 | 107
+ 108 | 108 | 108
+ 109 | 109 | 109
+ 110 | 110 | 110
+(10 rows)
+
+-- Test similar to above with all full outer joins
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+QUERY PLAN
+--
+ Limit
+   Output: t1."C 1", t2.c1, t3.c1
+   ->  Merge Full Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: 

Re: [HACKERS] Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

[Aleksander Alekseev]

> Well, the documentation already says to avoid it:
> 
> http://www.postgresql.org/docs/current/static/xfunc-c.html
> 
>Another important point is to avoid leaving any uninitialized
>bits within data type values; for example, take care to zero out
>any alignment padding bytes that might be present in structs.
> 
> so I don't think what you're suggesting would be controversial
> at all; it looks like what you've done is found a(t least one)
> bug where the documented practice wasn't followed, and it's good
> to find any such places.

Well in this case here is a patch that fixes "use of uninitialized
value" reports by MemorySanitizer I managed to catch so far.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index b48e97c..3e81865 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -328,7 +328,7 @@ gistXLogSplit(RelFileNode node, BlockNumber blkno, bool page_is_leaf,
 			  BlockNumber origrlink, GistNSN orignsn,
 			  Buffer leftchildbuf, bool markfollowright)
 {
-	gistxlogPageSplit xlrec;
+	gistxlogPageSplit xlrec = {0};
 	SplitedPageLayout *ptr;
 	int			npage = 0;
 	XLogRecPtr	recptr;
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index f090ca5..7bab290 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -202,7 +202,7 @@ static void
 addLeafTuple(Relation index, SpGistState *state, SpGistLeafTuple leafTuple,
 		   SPPageDesc *current, SPPageDesc *parent, bool isNulls, bool isNew)
 {
-	spgxlogAddLeaf xlrec;
+	spgxlogAddLeaf xlrec = {0};
 
 	xlrec.newPage = isNew;
 	xlrec.storesNulls = isNulls;
@@ -400,7 +400,7 @@ moveLeafs(Relation index, SpGistState *state,
 	OffsetNumber *toDelete;
 	OffsetNumber *toInsert;
 	BlockNumber nblkno;
-	spgxlogMoveLeafs xlrec;
+	spgxlogMoveLeafs xlrec = {0};
 	char	   *leafdata,
 			   *leafptr;
 
@@ -701,7 +701,7 @@ doPickSplit(Relation index, SpGistState *state,
 	int			currentFreeSpace;
 	int			totalLeafSizes;
 	bool		allTheSame;
-	spgxlogPickSplit xlrec;
+	spgxlogPickSplit xlrec = {0};
 	char	   *leafdata,
 			   *leafptr;
 	SPPageDesc	saveCurrent;
@@ -1492,7 +1492,7 @@ spgAddNodeAction(Relation index, SpGistState *state,
  int nodeN, Datum nodeLabel)
 {
 	SpGistInnerTuple newInnerTuple;
-	spgxlogAddNode xlrec;
+	spgxlogAddNode xlrec = {0};
 
 	/* Should not be applied to nulls */
 	Assert(!SpGistPageStoresNulls(current->page));
@@ -1699,7 +1699,7 @@ spgSplitNodeAction(Relation index, SpGistState *state,
 	BlockNumber postfixBlkno;
 	OffsetNumber postfixOffset;
 	int			i;
-	spgxlogSplitTuple xlrec;
+	spgxlogSplitTuple xlrec = {0};
 	Buffer		newBuffer = InvalidBuffer;
 
 	/* Should not be applied to nulls */
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b119a47..50d3123 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4755,7 +4755,7 @@ XLOGShmemInit(void)
 void
 BootStrapXLOG(void)
 {
-	CheckPoint	checkPoint;
+	CheckPoint	checkPoint = {0};
 	char	   *buffer;
 	XLogPageHeader page;
 	XLogLongPageHeader longpage;
@@ -4802,11 +4802,9 @@ BootStrapXLOG(void)
 	checkPoint.ThisTimeLineID = ThisTimeLineID;
 	checkPoint.PrevTimeLineID = ThisTimeLineID;
 	checkPoint.fullPageWrites = fullPageWrites;
-	checkPoint.nextXidEpoch = 0;
 	checkPoint.nextXid = FirstNormalTransactionId;
 	checkPoint.nextOid = FirstBootstrapObjectId;
 	checkPoint.nextMulti = FirstMultiXactId;
-	checkPoint.nextMultiOffset = 0;
 	checkPoint.oldestXid = FirstNormalTransactionId;
 	checkPoint.oldestXidDB = TemplateDbOid;
 	checkPoint.oldestMulti = FirstMultiXactId;
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 1ff5728..e27a18f 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -667,7 +667,7 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
 	DIR		   *dirdesc;
 	struct dirent *de;
 	char	   *subfile;
-	struct stat st;
+	struct stat st = {0};
 
 	linkloc_with_version_dir = psprintf("pg_tblspc/%u/%s", tablespaceoid,
 		TABLESPACE_VERSION_DIRECTORY);
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 28f9fb5..123f068 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -823,7 +823,7 @@ parse_hba_line(List *line, int line_num, char *raw_line)
 {
 	char	   *str;
 	struct addrinfo *gai_result;
-	struct addrinfo hints;
+	struct addrinfo hints = {0};
 	int			ret;
 	char	   *cidr_slash;
 	char	   *unsupauth;
@@ -1010,12 +1010,6 @@ parse_hba_line(List *line, int line_num, char *raw_line)
 			/* Get the IP address either way */
 			hints.ai_flags = AI_NUMERICHOST;
 			hints.ai_family = AF_UNSPEC;
-			hints.ai_socktype = 0;
-			hints.ai_protocol = 0;
-			hints.ai_addrlen = 0;
-			hints.ai_canonname = NULL;
-			hints.ai_addr = NULL;
-			hints.ai_next = NULL;
 
 			ret = 

Re: [HACKERS] Proposal: Generic WAL logical messages

[Petr Jelinek]

Hi,

thanks for review.

On 17/03/16 13:36, Tomas Vondra wrote:

Hi,

a few comments about the last version of the patch:


1) LogicalDecodeMessageCB

Do we actually need the 'transactional' parameter here? I mean, having
the 'txn' should be enough, as

 transactional = (txt != NULL)



Agreed. Same goes for the ReoderBufferChange struct btw, only 
transactional messages go there so no point in marking them as such.





2) pg_logical_emit_message_bytea / pg_logical_emit_message_text

The comment before _bytea is wrong - it's just a copy'n'paste from the
preceding function (pg_logical_slot_peek_binary_changes). _text has no
comment at all, but it's true it's  just a simple _bytea wrapper.



Heh, blind.


The main issue here however is that the functions are not defined as
strict, but ignore the possibility that the parameters might be NULL. So
for example this crashes the backend

SELECT pg_logical_emit_message(NULL::boolean, NULL::text, NULL::text);



Good point.



3) ReorderBufferQueueMessage

No comment. Not a big deal I guess, the method is simple enough, but why
to break the rule when all the other methods around have at least a
short one?



Yeah I sometimes am not sure if there is a point to put comment to tiny 
straightforward functions that do more or less same as the one above. 
But it's public API so probably better to have one.




4) ReorderBufferChange

The new struct in the 'union' would probably deserve at least a short
comment explaining the purpose (just like the other structs around).




Okay.

Updated version attached.

(BTW please try to CC author of the patch when reviewing)


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
>From 4515b5b3e8f5ddb96124e859b968c43c27f79be3 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Wed, 24 Feb 2016 17:02:36 +0100
Subject: [PATCH] Logical Decoding Messages

---
 contrib/test_decoding/Makefile  |  2 +-
 contrib/test_decoding/expected/ddl.out  | 21 --
 contrib/test_decoding/expected/messages.out | 56 
 contrib/test_decoding/sql/ddl.sql   |  3 +-
 contrib/test_decoding/sql/messages.sql  | 17 +
 contrib/test_decoding/test_decoding.c   | 17 +
 doc/src/sgml/func.sgml  | 45 +
 doc/src/sgml/logicaldecoding.sgml   | 34 ++
 src/backend/access/rmgrdesc/Makefile|  4 +-
 src/backend/access/rmgrdesc/logicalmsgdesc.c| 41 
 src/backend/access/transam/rmgr.c   |  1 +
 src/backend/replication/logical/Makefile|  2 +-
 src/backend/replication/logical/decode.c| 49 ++
 src/backend/replication/logical/logical.c   | 37 +++
 src/backend/replication/logical/logicalfuncs.c  | 27 
 src/backend/replication/logical/message.c   | 87 +
 src/backend/replication/logical/reorderbuffer.c | 68 +++
 src/backend/replication/logical/snapbuild.c | 19 ++
 src/bin/pg_xlogdump/rmgrdesc.c  |  1 +
 src/include/access/rmgrlist.h   |  1 +
 src/include/catalog/pg_proc.h   |  4 ++
 src/include/replication/logicalfuncs.h  |  2 +
 src/include/replication/message.h   | 41 
 src/include/replication/output_plugin.h | 12 
 src/include/replication/reorderbuffer.h | 20 ++
 src/include/replication/snapbuild.h |  2 +
 26 files changed, 601 insertions(+), 12 deletions(-)
 create mode 100644 contrib/test_decoding/expected/messages.out
 create mode 100644 contrib/test_decoding/sql/messages.sql
 create mode 100644 src/backend/access/rmgrdesc/logicalmsgdesc.c
 create mode 100644 src/backend/replication/logical/message.c
 create mode 100644 src/include/replication/message.h

diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 06c9546..309cb0b 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -38,7 +38,7 @@ submake-test_decoding:
 	$(MAKE) -C $(top_builddir)/contrib/test_decoding
 
 REGRESSCHECKS=ddl xact rewrite toast permissions decoding_in_xact \
-	decoding_into_rel binary prepared replorigin time
+	decoding_into_rel binary prepared replorigin time messages
 
 regresscheck: | submake-regress submake-test_decoding temp-install
 	$(MKDIR_P) regression_output
diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out
index 77719e8..32cd24d 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -220,11 +220,17 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
 (7 rows)
 
 /*
- * check that disk spooling works
+ * check that disk spooling works (also for logical messages)
  */
 BEGIN;
 CREATE TABLE tr_etoomuch (id serial primary key, data int);
 

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Robert Haas]

On Sat, Mar 19, 2016 at 8:30 AM, Michael Paquier
 wrote:
>> Doing that with the
>> level of detail and care that it seems to me to require seems like an
>> almost-impossible task.  Most of the major features I've committed
>> this CommitFest are patches where I've personally done multiple rounds
>> of review on over the last several months, and in many cases, other
>> people have been doing code reviews for months before that.  I'm not
>> denying that this patch has prompted a good deal of discussion and
>> what I would call design review, but detailed code review?  I just
>> haven't seen much of that.
>
> There has been none, as well as no real discussion regarding what we
> want to do. The current result, particularly for the management of
> protocol aging, is based on things I wrote by myself which negate the
> many negative opinions received up to now for the past patches (mainly
> the feedback was "I don't like that", without real output or fresh
> ideas during discussion to explain why that's the case).

Well, I said before and I'll say again that I don't like the idea of
multiple password verifiers.  I think that's an accident waiting to
happen, and I'm not prepared to put in the amount of time and energy
that it would take to get that feature committed despite not wanting
it myself, or for being responsible for it afterwards.  I'd prefer we
didn't do it at all, although I'm not going to dig in my heels.  I
might be willing to deal with SCRAM itself, but this whole area is not
my strongest suit.  So ideally some other committer would be willing
to pick this up.

But the problem isn't even just that somebody has to hit the final
commit button - as we've both said, there's a woeful lack of any
meaningful review on this thread, and this sort of change really needs
quite a lot of review.  This has implications for
backward-compatibility, for connectors that don't use libpq, etc.
Really, I'm not even sure we have consensus on the direction.  I mean,
Heikki's proposal to adopt SCRAM sounds good enough at a broad level,
but I don't really know what the alternatives are, I'm mostly just
taking his word for it, and like you say, there's been a fair amount
of miscellaneous negativity floating around.

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