Re: [HACKERS] postgres_fdw bug in 9.6

[Ashutosh Bapat]

Probably we should use "could not be created" instead of "was not created"
in "... a local path suitable for EPQ checks was not created".

"outer_path should not require relations from inner_path" may be reworded
as "outer paths should not be parameterized by the inner relations".

"neither path should require relations from the other path" may be reworded
as "neither path should be parameterized by the the other joining relation".


> While the comment below mentions ON true, the testcase you have added is
>> for ON
>> false. Either the testcase should change or this comment. That raises
>> another
>> question, what happens when somebody does FULL JOIN ON false?
>> + * If special case: for "x FULL JOIN y ON true",
>> there
>>
>
> FULL JOIN ON FALSE would be handled the same way as FULL JOIN ON TRUE, so
> I think we should rewrite that comment into something like this: If special
> case: for "x FULL JOIN y ON true" or "x FULL JOIN y ON false"...
>

Ok.


>
> Why JOIN_RIGHT is being treated differently from JOIN_LEFT? We should be
>> able
>> to create a nested loop join for JOIN_RIGHT?
>> +case JOIN_RIGHT:
>> +case JOIN_FULL:
>>
>
> I don't think so, because nestloop joins aren't supported for JOIN_RIGHT.
> See ExecInitNestLoop().
>

Hmm, I see in match_unsorted_outer()
1254 case JOIN_RIGHT:
1255 case JOIN_FULL:
1256 nestjoinOK = false;
1257 useallclauses = true;
1258 break;

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> The patch doesn't seem to behave like that.  The Parse message calls 
> start_xact_command() -> enable_statement_timeout() -> enable_timeout(), and 
> set stmt_timer_started to true.  Subsequent Bind and Execute messages call 
> enable_statement_timeout(), but enable_statement_timeout() doesn't call 
> enable_timeout() because stmt_timer_started is already true.
> 
> 
>> > It looks like the patch does the following.  I think this is desirable,
>> because starting and stopping the timer for each message may be costly as
>> Tom said.
>> > Parse(statement1)
>> >   start timer
>> > Bind(statement1, portal1)
>> > Execute(portal1)
>> >   stop timer
>> > Sync
> 
> I ran one INSERT statement using JDBC with log_min_messages = DEBUG3, and the 
> server log shows what I said.
> 
> DEBUG:  parse : insert into a values(2)
> DEBUG:  Set statement timeout
> LOG:  duration: 0.431 ms  parse : insert into a values(2)
> DEBUG:  bind  to 
> LOG:  duration: 0.127 ms  bind : insert into a values(2)
> DEBUG:  Disable statement timeout
> LOG:  duration: 0.184 ms  execute : insert into a values(2)
> DEBUG:  snapshot of 1+0 running transaction ids (lsn 0/163BF28 oldest xid 561 
> latest complete 560 next xid 562)

Check.

>> This doesn't work in general use cases. Following pattern appears frequently
>> in applications.
>> 
>> Parse(statement1)
>> Bind(statement1, portal1)
>> Execute(portal1)
>> Bind(statement1, portal1)
>> Execute(portal1)
>> :
>> :
>> Sync
> 
> It works.  The first Parse-Bind-Execute is measured as one unit, then 
> subsequent Bind-Execute pairs are measured as other units.  That's because 
> each Execute ends the statement_timeout timer and the Bind message starts it 
> again.  I think this is desirable, so the current patch looks good.  May I 
> mark this as ready for committer?  FYI, make check-world passed successfully.

It's too late. Someone has already moved the patch to the next CF (for
PostgreSQL 11).

>> Also what would happen if client just send a parse message and does nothing
>> after that?
> 
> It's correct to trigger the statement timeout in this case, because the first 
> SQL statement started (with the Parse message) and its execution is not 
> finished (with Execute message.)
> 
> 
> Regards
> Takayuki Tsunakawa
> 
> 
> 


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

[Amit Khandekar]

Thanks Andres for your review comments. Will get back with the other
comments, but meanwhile some queries about the below particular
comment ...

On 4 April 2017 at 10:17, Andres Freund  wrote:
> On 2017-04-03 22:13:18 -0400, Robert Haas wrote:
>> On Mon, Apr 3, 2017 at 4:17 PM, Andres Freund  wrote:
>> > Hm.  I'm not really convinced by the logic here.  Wouldn't it be better
>> > to try to compute the minimum total cost across all workers for
>> > 1..#max_workers for the plans in an iterative manner?  I.e. try to map
>> > each of the subplans to 1 (if non-partial) or N workers (partial) using
>> > some fitting algorith (e.g. always choosing the worker(s) that currently
>> > have the least work assigned).  I think the current algorithm doesn't
>> > lead to useful #workers for e.g. cases with a lot of non-partial,
>> > high-startup plans - imo a quite reasonable scenario.

I think I might have not understood this part exactly. Are you saying
we need to consider per-subplan parallel_workers to calculate total
number of workers for Append ? I also didn't get about non-partial
subplans. Can you please explain how many workers you think should be
expected with , say , 7 subplans out of which 3 are non-partial
subplans ?

>>
>> Well, that'd be totally unlike what we do in any other case.  We only
>> generate a Parallel Seq Scan plan for a given table with one # of
>> workers, and we cost it based on that.  We have no way to re-cost it
>> if we changed our mind later about how many workers to use.
>> Eventually, we should probably have something like what you're
>> describing here, but in general, not just for this specific case.  One
>> problem, of course, is to avoid having a larger number of workers
>> always look better than a smaller number, which with the current
>> costing model would probably happen a lot.
>
> I don't think the parallel seqscan is comparable in complexity with the
> parallel append case.  Each worker there does the same kind of work, and
> if one of them is behind, it'll just do less.  But correct sizing will
> be more important with parallel-append, because with non-partial
> subplans the work is absolutely *not* uniform.
>
> Greetings,
>
> Andres Freund



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database 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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Actually the statement timer is replaced with new statement timer value
> in enable_statement_timeout().

The patch doesn't seem to behave like that.  The Parse message calls 
start_xact_command() -> enable_statement_timeout() -> enable_timeout(), and set 
stmt_timer_started to true.  Subsequent Bind and Execute messages call 
enable_statement_timeout(), but enable_statement_timeout() doesn't call 
enable_timeout() because stmt_timer_started is already true.


> > It looks like the patch does the following.  I think this is desirable,
> because starting and stopping the timer for each message may be costly as
> Tom said.
> > Parse(statement1)
> >   start timer
> > Bind(statement1, portal1)
> > Execute(portal1)
> >   stop timer
> > Sync

I ran one INSERT statement using JDBC with log_min_messages = DEBUG3, and the 
server log shows what I said.

DEBUG:  parse : insert into a values(2)
DEBUG:  Set statement timeout
LOG:  duration: 0.431 ms  parse : insert into a values(2)
DEBUG:  bind  to 
LOG:  duration: 0.127 ms  bind : insert into a values(2)
DEBUG:  Disable statement timeout
LOG:  duration: 0.184 ms  execute : insert into a values(2)
DEBUG:  snapshot of 1+0 running transaction ids (lsn 0/163BF28 oldest xid 561 
latest complete 560 next xid 562)


> This doesn't work in general use cases. Following pattern appears frequently
> in applications.
> 
> Parse(statement1)
> Bind(statement1, portal1)
> Execute(portal1)
> Bind(statement1, portal1)
> Execute(portal1)
> :
> :
> Sync

It works.  The first Parse-Bind-Execute is measured as one unit, then 
subsequent Bind-Execute pairs are measured as other units.  That's because each 
Execute ends the statement_timeout timer and the Bind message starts it again.  
I think this is desirable, so the current patch looks good.  May I mark this as 
ready for committer?  FYI, make check-world passed successfully.


> Also what would happen if client just send a parse message and does nothing
> after that?

It's correct to trigger the statement timeout in this case, because the first 
SQL statement started (with the Parse message) and its execution is not 
finished (with Execute message.)


Regards
Takayuki Tsunakawa





-- 
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] ANALYZE command progress checker

[Amit Langote]

On 2017/04/04 14:15, Amit Langote wrote:
> On 2017/04/04 14:12, Amit Langote wrote:
>> Two kinds of statistics are collected if the table is a inheritance parent.
>>
>> First kind considers the table by itself and calculates regular
>> single-table statistics using rows sampled from the only available heap
>> (by calling do_analyze_rel() with inh=false).
> 
> Oops, should have also mentioned that this part is not true for the new
> partitioned tables.  There are single-table statistics for partitioned
> tables, because it contains no data.

Sorry again, I meant *no* single-table statistics partitioned tables...

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] [PATCH] Incremental sort

[Andres Freund]

On 2017-04-03 22:18:21 -0400, Robert Haas wrote:
> On Mon, Apr 3, 2017 at 5:09 PM, Andres Freund  wrote:
> > To me this hasn't gotten even remotely enough performance evaluation.
> > And I don't think it's fair to characterize it as pending since 2013,
> > given it was essentially "waiting on author" for most of that.
> 
> This is undeniably a patch which has been kicking around for a lot of
> time without getting a lot of attention, and if it just keeps getting
> punted down the road, it's never going to become committable.

Indeed, it's old.  And it hasn't gotten enough timely feedback.

But I don't think the wait time can meaningfully be measured by
subtracting two dates:
The first version of the patch, as a PoC, has been posted 2013-12-14,
which then got a good amount of feedback & revisions, and then stalled
till 2014-07-12.  There a few back-and forths yielded a new version.
>From 2014-09-15 till 2015-10-16 the patch stalled, waiting on its
author.  That version had open todos ([1]), as had the version from
2016-03-13 [2], which weren't addressed 2016-03-30 - unfortunately that
was pretty much when the tree was frozen.  2016-09-13 a rebased patch
was sent, some minor points were raised 2016-10-02 (unaddressed), a
larger review was done 2016-12-01 ([5]), unaddressed till 2017-02-18.
At that point we're in this thread.

There's obviously some long waiting-on-author periods in there.  And
some long needs-review periods.


> Alexander's questions upthread about what decisions the committer who
> took an interest (Heikki) would prefer never really got an answer, for
> example.  I don't deny that there may be some work left to do here,
> but I think blaming the author for a week's delay when this has been
> ignored so often for so long is unfair.

I'm not trying to blame Alexander for a week's worth of delay, at all.
It's just that, well, we're past the original code-freeze date, three
days before the "final" code freeze. I don't think fairness is something
we can achieve at this point :(.  Given the risk of regressions -
demonstrated in this thread although partially adressed - and the very
limited amount of benchmarking done, it seems unlikely that this is
going to be merged.

Regards,

Andres


[1] 
http://archives.postgresql.org/message-id/CAPpHfdvhwMsG69exCRUGK3ms-ng0PSPcucH5FU6tAaM-qL-1%2Bw%40mail.gmail.com
[2] 
http://archives.postgresql.org/message-id/CAPpHfdvzjYGLTyA-8ib8UYnKLPrewd9Z%3DT4YJNCRWiHWHHweWw%40mail.gmail.com
[3] 
http://archives.postgresql.org/message-id/CAPpHfdtCcHZ-mLWzsFrRCvHpV1LPSaOGooMZ3sa40AkwR=7...@mail.gmail.com
[4] 
http://archives.postgresql.org/message-id/capphfdvj1tdi2wa64zbbp5-yg-uzarxzk3k7j7zt-crx6ys...@mail.gmail.com
[5] 
http://archives.postgresql.org/message-id/CA+TgmoZapyHRm7NVyuyZ+yAV=u1a070bogre7pkgyraegr4...@mail.gmail.com
[6] 
http://archives.postgresql.org/message-id/CAPpHfds1waRZ=NOmueYq0sx1ZSCnt+5QJvizT8ndT2=etze...@mail.gmail.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] ANALYZE command progress checker

[Amit Langote]

On 2017/04/04 14:12, Amit Langote wrote:
> Two kinds of statistics are collected if the table is a inheritance parent.
> 
> First kind considers the table by itself and calculates regular
> single-table statistics using rows sampled from the only available heap
> (by calling do_analyze_rel() with inh=false).

Oops, should have also mentioned that this part is not true for the new
partitioned tables.  There are single-table statistics for partitioned
tables, because it contains no data.

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] ANALYZE command progress checker

[Amit Langote]

On 2017/03/30 17:39, Masahiko Sawada wrote:
> On Wed, Mar 29, 2017 at 5:38 PM, vinayak wrote:
 I have updated the patch.
> 
> I reviewed v7 patch.
> 
> When I ran ANALYZE command to the table having 5 millions rows with 3
> child tables, the progress report I got shows the strange result. The
> following result was output after sampled all target rows from child
> tables (i.g, after finished acquire_inherited_sample_rows). I think
> the progress information should report 100% complete at this point.
> 

[...]

> 
> I guess the cause of this is that you always count the number of
> sampled rows in acquire_sample_rows staring from 0 for each child
> table. num_rows_sampled is reset whenever starting collecting sample
> rows.
> Also, even if table has child table, the total number of sampling row
> is not changed. I think that main differences between analyzing on
> normal table and on partitioned table is where we fetch the sample row
> from. So I'm not sure if adding "computing inherited statistics" phase
> is desirable. If you want to report progress of collecting sample rows
> on child table I think that you might want to add the information
> showing which child table is being analyzed.

Two kinds of statistics are collected if the table is a inheritance parent.

First kind considers the table by itself and calculates regular
single-table statistics using rows sampled from the only available heap
(by calling do_analyze_rel() with inh=false).

The second kind are called inheritance statistics, which consider rows
sampled from the whole inheritance hierarchy (by calling do_analyze_rel()
with inh=true).  Note that we are still collecting statistics for the
parent table, so we not really "analyzing" child tables; we just collect
sample rows from them to calculate whole-tree statistics for the root
parent table.  It might still be possible to show the child table being
sampled though.

> --
> pg_stat_progress_analyze.num_target_sample_rows is set while ignoring
> the number of rows the target table has actually. So If the table has
> rows less than target number of rows, the num_rows_sampled never reach
> to num_target_sample_rows.
> 
> --
> @@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel,
> }
> 
> samplerows += 1;
> +
> +   /* Report number of rows sampled so far */
> +
> pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED,
> numrows);
> }
> }
> 
> I think that it's wrong to use numrows variable to report the progress
> of the number of rows sampled. As the following comment mentioned, we
> first save row into rows[] and increment numrows until numrows <
> targrows. And then we could replace stored sample row with new sampled
> row. That is, after collecting "numrows" rows, analyze can still take
> a long time to get and replace the sample row. To computing progress
> of collecting sample row, IMO reporting the number of blocks we
> scanned so far is better than number of sample rows. Thought?

We can report progress in terms of individual blocks only inside
acquire_sample_rows(), which seems undesirable when one thinks that we
will be resetting the target for every child table.  We should have a
global target that considers all child tables in the inheritance
hierarchy, which maybe is possible if we count them beforehand in
acquire_inheritance_sample_rows().  But why not use target sample rows,
which remains the same for both when we're collecting sample rows from one
table and from the whole inheritance hierarchy.  We can keep the count of
already collected rows in a struct that is used across calls for all the
child tables and increment upward from that count when we start collecting
from a new child table.

>> /*
>>  * The first targrows sample rows are simply copied into the
>>  * reservoir. Then we start replacing tuples in the sample
>>  * until we reach the end of the relation.  This algorithm is
>>  * from Jeff Vitter's paper (see full citation below). It
>>  * works by repeatedly computing the number of tuples to skip
>>  * before selecting a tuple, which replaces a randomly chosen
>>  * element of the reservoir (current set of tuples).  At all
>>  * times the reservoir is a true random sample of the tuples
>>  * we've passed over so far, so when we fall off the end of
>>  * the relation we're done.
>>  */

It seems that we could use samplerows instead of numrows to count the
progress (if we choose to count progress in terms of sample rows collected).

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] Refactor handling of database attributes between pg_dump and pg_dumpall

[Haribabu Kommi]

On Thu, Mar 30, 2017 at 12:00 PM, Haribabu Kommi 
wrote:

>
>
> On Wed, Mar 29, 2017 at 11:04 PM, Andreas Karlsson 
> wrote:
>
>> On 03/29/2017 05:43 AM, Haribabu Kommi wrote:
>> > Updated patch attached.
>>
>> I get a test failure in the pg_upgrade tests, but I do not have time
>> right now to investigate.
>>
>> The failing test is "Restoring database schemas in the new cluster".
>>
>
> Thanks for test.
>
> I found the reason for failure.
>
> Before this refactor patch, in case of --binary-upgrade, the pg_dumpall
> dumps all the global objects and also the database objects. These objects
> will be restored first during the preparation of the new cluster and later
> each individual database is restored.
>
> Because of the refactoring of the database objects, currently as part of
> globals dump with --binary-upgrade, no database objects gets dumped.
> During restore no databases are created. so while restoring individual
> database, it leads to failure as it not able to connect to the target
> database.
>

I modified the pg_upgrade code to use template1 database as a connecting
database while restoring the dump along with --create option to pg_restore
to create the database objects instead of connecting to the each individual
database.

And also while dumping the database objects, passed the new option of
--enable-pgdumpall-behaviour to pg_dump to dump the database objects
as it expected dump during pg_dumpall --binary-upgrade.

Both pg_dump and pg_upgrade tests are passed. Updated patch attached
I will add this patch to the next commitfest.

Regards,
Hari Babu
Fujitsu Australia


pg_dump_changes_5.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: Batch/pipelining support for libpq

[Andres Freund]

On 2017-04-04 08:57:33 +0900, Michael Paquier wrote:
> On Tue, Apr 4, 2017 at 8:26 AM, Andres Freund  wrote:
> > On 2017-04-04 09:24:23 +1000, Vaishnavi Prabakaran wrote:
> >> Just quickly, Is it not ok to consider only the code patch for this CF
> >> without test patch?
> >
> > I'd say no, it's not acceptable.  This is too much new code for it not
> > to be tested.
> 
> Doesn't it depend actually?

Well, I didn't make a general statement, I made one about this patch.
And this would add a significant bunch of untested code, and it'll likely
take years till it gets decent coverage outside.


> In the case of this patch, it seems to me that we would have a far
> better portable set of tests if we had a dedicated set of subcommands
> available at psql level, particularly for Windows/MSVC.

That's a really large scope creep imo.  Adding a bunch of user-facing
psql stuff doesn't compare in complexity to running a test across
platforms.  We can just do that from regess.c or such, if that ends up
being a problem..

> If that's a  requirement for this patch so let it be. I am not saying that 
> tests
> are not necessary. They are of course, but in this case having a bit
> more infrastructure would be more be more helpful for users and the
> tests themselves.

I'm not following.


Greetings,

Andres Freund


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

Re: [HACKERS] Parallel Append implementation

[Andres Freund]

On 2017-04-03 22:13:18 -0400, Robert Haas wrote:
> On Mon, Apr 3, 2017 at 4:17 PM, Andres Freund  wrote:
> > Hm.  I'm not really convinced by the logic here.  Wouldn't it be better
> > to try to compute the minimum total cost across all workers for
> > 1..#max_workers for the plans in an iterative manner?  I.e. try to map
> > each of the subplans to 1 (if non-partial) or N workers (partial) using
> > some fitting algorith (e.g. always choosing the worker(s) that currently
> > have the least work assigned).  I think the current algorithm doesn't
> > lead to useful #workers for e.g. cases with a lot of non-partial,
> > high-startup plans - imo a quite reasonable scenario.
> 
> Well, that'd be totally unlike what we do in any other case.  We only
> generate a Parallel Seq Scan plan for a given table with one # of
> workers, and we cost it based on that.  We have no way to re-cost it
> if we changed our mind later about how many workers to use.
> Eventually, we should probably have something like what you're
> describing here, but in general, not just for this specific case.  One
> problem, of course, is to avoid having a larger number of workers
> always look better than a smaller number, which with the current
> costing model would probably happen a lot.

I don't think the parallel seqscan is comparable in complexity with the
parallel append case.  Each worker there does the same kind of work, and
if one of them is behind, it'll just do less.  But correct sizing will
be more important with parallel-append, because with non-partial
subplans the work is absolutely *not* uniform.

Greetings,

Andres Freund


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Thu, Mar 30, 2017 at 11:17 PM, Bruce Momjian  wrote:

> On Tue, Mar 21, 2017 at 04:04:58PM -0400, Bruce Momjian wrote:
> > On Tue, Mar 21, 2017 at 04:56:16PM -0300, Alvaro Herrera wrote:
> > > Bruce Momjian wrote:
> > > > On Tue, Mar 21, 2017 at 04:43:58PM -0300, Alvaro Herrera wrote:
> > > > > Bruce Momjian wrote:
> > > > >
> > > > > > I don't think it makes sense to try and save bits and add
> complexity
> > > > > > when we have no idea if we will ever use them,
> > > > >
> > > > > If we find ourselves in dire need of additional bits, there is a
> known
> > > > > mechanism to get back 2 bits from old-style VACUUM FULL.  I assume
> that
> > > > > the reason nobody has bothered to write the code for that is that
> > > > > there's no *that* much interest.
> > > >
> > > > We have no way of tracking if users still have pages that used the
> bits
> > > > via pg_upgrade before they were removed.
> > >
> > > Yes, that's exactly the code that needs to be written.
> >
> > Yes, but once it is written it will take years before those bits can be
> > used on most installations.
>
> Actually, the 2 bits from old-style VACUUM FULL bits could be reused if
> one of the WARM bits would be set  when it is checked.  The WARM bits
> will all be zero on pre-9.0.  The check would have to be checking the
> old-style VACUUM FULL bit and checking that a WARM bit is set.
>
>
We're already doing that in the submitted patch.

Thanks,
Pavan

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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Pavel Stehule]

2017-04-03 21:24 GMT+02:00 Fabien COELHO :

>
> Hello Tom,
>
>   \if [select current_setting('server_version_num')::int < 11]
>>>
>>
>> I really dislike this syntax proposal.
>>
>
> It would get us into having to count nested brackets, and distinguish
>> quoted from unquoted brackets, and so on ... and for what?  It's not really
>> better than
>>
>>   \if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'
>>
>
> Hmmm. On the positive side, it looks more expression-like, and it allows
> to think of handling a multi-line expression on day.
>
> ISTM that the lexer already counts parentheses, so maybe using external
> parentheses might work without much effort?
>
> (ie, "\if sql ...text to send to server...").
>>
>> If you're worried about shaving keystrokes, make the "select" be implicit.
>> That would be particularly convenient for the common case where you're
>> just trying to evaluate a boolean expression with no table reference.
>>
>
> I'm looking for a solution to avoid the suggested "sql" prefix, because "I
> really dislike this proposal", as you put it.
>
>
The expression evaluation is interesting question, but there is a
workaround - we can use \gset already.

What is more important, because there is not any workaround, is detection
if some variable exists or not.

So possibilities

1. \if defined varname
2. \ifdefined or \ifdef varname
3. \if :?varname

I like first two, and I can live with @3 - although it is not intuitive

Regards

Pavel

Re: [HACKERS] Instead of DROP function use UPDATE pg_proc in an upgrade extension script

[Andrew Gierth]

> "Vicky" == Vicky Vergara  writes:

 Vicky> UPDATE pg_proc SET [...]

 Vicky> So, I want to know how "safe" can you consider the second
 Vicky> method, and what kind of other objects do I need to test besides
 Vicky> views.

Speaking from personal experience (I did this in the upgrade script for
ip4r, in a much simpler case than yours, and broke it badly), it's not
at all safe.

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Fri, Mar 31, 2017 at 12:31 PM, Dilip Kumar  wrote:

> On Thu, Mar 30, 2017 at 5:27 PM, Amit Kapila 
> wrote:
> > I am not sure if we can consider it as completely synthetic because we
> > might see some similar cases for json datatypes.  Can we once try to
> > see the impact when the same test runs from multiple clients?  For
> > your information, I am also trying to setup some tests along with one
> > of my colleague and we will report the results once the tests are
> > complete.
> We have done some testing and below is the test details and results.
>
> Test:
> I have derived this test from above test given by pavan[1] except
> below difference.
>
> - I have reduced the fill factor to 40 to ensure that multiple there
> is scope in the page to store multiple WARM chains.
> - WARM updated all the tuples.
> - Executed a large select to enforce lot of recheck tuple within single
> query.
> - Smaller tuple size (aid field is around ~100 bytes) just to ensure
> tuple have sufficient space on a page to get WARM updated.
>
> Results:
> ---
> * I can see more than 15% of regression in this case. This regression
> is repeatable.
> * If I increase the fill factor to 90 than regression reduced to 7%,
> may be only fewer tuples are getting WARM updated and others are not
> because of no space left on page after few WARM update.
>

Thanks for doing the tests. The tests show us that if the table gets filled
up with WARM chains, and they are not cleaned up and the table is subjected
to read-only workload, we will see regression. Obviously, the test is
completely CPU bound, something WARM is not meant to address.I am not yet
certain if recheck is causing the problem. Yesterday I ran the test where I
was seeing regression with recheck completely turned off and still saw
regression. So there is something else that's going on with this kind of
workload. Will check.

Having said that, I think there are some other ways to fix some of the
common problems with repeated rechecks. One thing that we can do it rely on
the index pointer flags to decide whether recheck is necessary or not. For
example, a WARM pointer to a WARM tuple does not require recheck.
Similarly, a CLEAR pointer to a CLEAR tuple does not require recheck. A
WARM pointer to a CLEAR tuple can be discarded immediately because the only
situation where it can occur is in the case of aborted WARM updates. The
only troublesome situation is a CLEAR pointer to a WARM tuple. That
entirely depends on whether the index had received a WARM insert or not.
What we can do though, if recheck succeeds for the first time and if the
chain has only WARM tuples, we set the WARM bit on the index pointer. We
can use the same hint mechanism as used for marking index pointers dead to
minimise overhead.

Obviously this will only handle the case when the same tuple is rechecked
often. But if a tuple is rechecked only once then may be other overheads
will kick-in, thus reducing the regression significantly.

Thanks,
Pavan

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

Re: [HACKERS] Instead of DROP function use UPDATE pg_proc in an upgrade extension script

[Pavel Stehule]

2017-04-04 6:17 GMT+02:00 Vicky Vergara :

>
> Hello,
>
>
> When creating an extension upgrade sql script, because the function does
> not have the same parameter names and/or parameters type and/or the result
> types changes, there is the need to drop the function because otherwise the
> CREATE OR REPLACE of the new signature will fail.
>
>
> So for example:
>
> having the following function:
>
>
> SELECT proallargtypes, proargmodes, proargnames FROM pg_proc WHERE
> proallargtypes = '{25,20,20,16,23,23,20,20}'   AND proname =
> 'pgr_edgedisjointpaths';
> -[ RECORD 1 ]--+
> -
> proallargtypes | {25,20,20,16,23,23,20,20}
> proargmodes| {i,i,i,i,o,o,o,o}
> proargnames| {"","","",directed,seq,path_seq,node,edge}
>
>
> When adding extra OUT parameters, because the result types ()
> change, the function needs a DROP:
>
> -- Row type defined by OUT parameters is different
>
>  ALTER EXTENSION pgrouting DROP FUNCTION pgr_edgedisjointpaths(text,
> bigint,bigint,boolean);
>
>  DROP FUNCTION IF EXISTS pgr_edgedisjointpaths(text,
> bigint,bigint,boolean);
>
>
> but doing that, objects that depend on the function. like a view, get
> dropped when using CASCADE in the ALTER extension, and functions that use
> the pgr_edgedisjointpaths internally don't get dropped.
>
>
> So, I must say that I experimented: instead of doing the drop, I made:
>
>
> UPDATE pg_proc SET
>
>   proallargtypes = '{25,20,20,16,23,23,23,20,20,
> 701,701}',
>
>   proargmodes = '{i,i,i,i,o,o,o,o,o,o,o}',
>
>proargnames = '{"","","","directed","seq","
> path_id","path_seq","node","edge","cost","agg_cost"}'
>
>  WHERE proallargtypes = '{25,20,20,16,23,23,20,20}'   AND proname =
> 'pgr_edgedisjointpaths';
>
>
> And CASCADE was not needed, and the view remained intact.
>
>
> So, I want to know how "safe" can you consider the second method, and what
> kind of other objects do I need to test besides views.
>

It is not safe due views - that are saved in post analyze form.

Regards

Pavel

> My plan, is to use the second method:
>
> - when the current names of the OUT parameters don't change, and there is
> an additional OUT parameter
>
> - when the current names of the IN parameters don't change, and there is
> an additional IN parameter with a default value
>
>
> Thanks
>
>
> Vicky Vergara
>
>
>
>
>
>
>
>

[HACKERS] Compiler warning in costsize.c

[Michael Paquier]

Hi all,

In builds where USE_ASSERT_CHECKING is not enabled, costsize.c can
generate warnings. Here is for example with MSVC:
  src/backend/optimizer/path/costsize.c(4520): warning C4101: 'rte' : unreferen
ced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
  src/backend/optimizer/path/costsize.c(4640): warning C4101: 'rte' : unreferen
ced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]

a9c074ba7 has done an effort, but a bit more is needed as the attached.

Thanks,
-- 
Michael


costsize-fix-warning.patch
Description: Binary data

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

[HACKERS] Instead of DROP function use UPDATE pg_proc in an upgrade extension script

[Vicky Vergara]

Hello,


When creating an extension upgrade sql script, because the function does not 
have the same parameter names and/or parameters type and/or the result types 
changes, there is the need to drop the function because otherwise the CREATE OR 
REPLACE of the new signature will fail.


So for example:

having the following function:


SELECT proallargtypes, proargmodes, proargnames FROM pg_proc WHERE
proallargtypes = '{25,20,20,16,23,23,20,20}'   AND proname = 
'pgr_edgedisjointpaths';
-[ RECORD 1 
]--+-
proallargtypes | {25,20,20,16,23,23,20,20}
proargmodes| {i,i,i,i,o,o,o,o}
proargnames| {"","","",directed,seq,path_seq,node,edge}


When adding extra OUT parameters, because the result types () change, the 
function needs a DROP:


-- Row type defined by OUT parameters is different

 ALTER EXTENSION pgrouting DROP FUNCTION 
pgr_edgedisjointpaths(text,bigint,bigint,boolean);

 DROP FUNCTION IF EXISTS pgr_edgedisjointpaths(text,bigint,bigint,boolean);


but doing that, objects that depend on the function. like a view, get dropped 
when using CASCADE in the ALTER extension, and functions that use the 
pgr_edgedisjointpaths internally don't get dropped.


So, I must say that I experimented: instead of doing the drop, I made:


UPDATE pg_proc SET

  proallargtypes = 
'{25,20,20,16,23,23,23,20,20,701,701}',

  proargmodes = '{i,i,i,i,o,o,o,o,o,o,o}',

   proargnames = 
'{"","","","directed","seq","path_id","path_seq","node","edge","cost","agg_cost"}'

 WHERE proallargtypes = '{25,20,20,16,23,23,20,20}'   AND proname = 
'pgr_edgedisjointpaths';


And CASCADE was not needed, and the view remained intact.


So, I want to know how "safe" can you consider the second method, and what kind 
of other objects do I need to test besides views.

My plan, is to use the second method:

- when the current names of the OUT parameters don't change, and there is an 
additional OUT parameter

- when the current names of the IN parameters don't change, and there is an 
additional IN parameter with a default value


Thanks


Vicky Vergara

Re: [HACKERS] wait event documentation

[Amit Langote]

On 2017/04/03 22:32, Robert Haas wrote:
> Right now, the information on wait events is organized into one giant
> table inside 
> https://www.postgresql.org/docs/devel/static/monitoring-stats.html#monitoring-stats-views
> -- the wait event type is inserted into the lefthand column of the
> table using moreRows="...", which is awkward to maintain and has
> already provoked several fixup commits after people (including me)
> messed it up.  And indeed it seems to be slightly messed up at the
> moment, too; the LWLock section needs moreRows++.
> 
> Instead of continuing to repair this every time it gets broken, I
> propose that we break this into one table that lists all the
> wait_event_type values -- LWLock, Lock, BufferPin, Activity, Client,
> Extension, IPC, Timeout, and IO -- and then a second table for each
> type that has multiple wait events listing all of the wait events for
> that type.
> 
> I also propose hoisting this out of section 28.2.3 - Viewing
> Statistics - and making it a new toplevel section of chapter 28.  So
> between the current "28.3 Viewing Locks" and the current "28.4
> Progress Reporting" we'd add a new section "Wait Events" and link to
> that from 28.2.3.  That would also give us a place to include any
> general text that we want to have regarding wait events, apart from
> the tables.
> 
> Thoughts?

+1.

By the way, wonder if it wouldn't make sense to take the whole Table 28.1.
Dynamic Statistics Views into a new section (perhaps before 28.2 Viewing
Locks or after), since those views display information different from what
the statistics collector component collects and publishes (those in the
Table 28.2. Collected Statistics Views).

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] Refactoring identifier checks to consistently use strcmp

[Alvaro Herrera]

Daniel Gustafsson wrote:
> Testing DefElem options is done with both strcmp() and pg_strcasecmp() a bit
> mixed.  Since the option defnames are all lowercased, either via IDENT, 
> keyword
> rules or “by hand” with makeString(), using strcmp() is safe (and assumed to 
> be
> so in quite a lot of places).
> 
> While it’s not incorrect per se to use pg_strcasecmp(), it has the potential 
> to
> hide a DefElem created with a mixed-case defname where it in other places is
> expected to be in lowercase, which may lead to subtle bugs.
> 
> The attached patch refactors to use strcmp() consistently for option 
> processing
> in the command code as a pre-emptive belts+suspenders move against such subtle
> bugs and to make the code more consistent.  Also reorders a few checks to have
> all in the same “format” and removes a comment related to the above.
> 
> Tested with randomizing case on options in make check (not included in patch).

Does it work correctly in the Turkish locale?

-- 
Álvaro Herrerahttps://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] [POC] A better way to expand hash indexes.

[Robert Haas]

On Sat, Apr 1, 2017 at 3:29 AM, Mithun Cy  wrote:
> On Sat, Apr 1, 2017 at 12:31 PM, Mithun Cy  wrote:
>> Also adding a patch which implements the 2nd way.
> Sorry, I forgot to add sortbuild_hash patch, which also needs similar
> changes for the hash_mask.

Committed.

-- 
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] Unable to build doc on latest head

[Ashutosh Bapat]

On Tue, Apr 4, 2017 at 2:01 AM, Andrew Gierth 
wrote:

> > "Peter" == Peter Eisentraut 
> writes:
>
>  > On 4/3/17 02:44, Ashutosh Bapat wrote:
>  >> [1] says that id.attribute is supported in stylesheets version
>  >> 1.77.1.  Do I need to update stylesheets version? How do I do it?
>  >> Any help will be appreciated.
>
>  Peter> The oldest version among the ones listed at
>  Peter> http://docbook.sourceforge.net/release/xsl/ that works for me is
>  Peter> 1.77.0.  Which one do you have?
>

1.76.1, docbook-xsl 1.76.1+dfsg-1ubuntu1


>
> BTW, the version in freebsd ports is 1.76.1, and the download stats on
> docbook.sourceforge.net suggests that this one is still pretty widely
> used despite its age. I filed a PR on the port, but who knows when
> anything will happen.
>

:(

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Unable to build doc on latest head

[Ashutosh Bapat]

On Mon, Apr 3, 2017 at 8:36 PM, Tom Lane  wrote:

> Ashutosh Bapat  writes:
> > Recently my doc build has started failing with errors
> > runtime error: file stylesheet.xsl line 57 element call-template
> > The called template 'id.attribute' was not found.
>
> > [1] says that id.attribute is supported in stylesheets version 1.77.1.
> Do I
> > need to update stylesheets version?
>
> FWIW, it's working for me with docbook-style-xsl-1.79.1.
>
> Appendix J claims our minimum supported version is 1.74.0, but since
> none of that page seems to have been updated for the new docs tooling,
> I'm not surprised if it's wrong.
>
> > How do I do it? Any help will be appreciated.
>
> That would depend on your platform.  For me, the stylesheets shipped with
> RHEL6 were too old, but I grabbed a more recent SRPM out of the Fedora
> repos and was able to build/install that with little trouble.
>
>
I forgot to specify the platform in my mail. It's Ubuntu 12.04 64bit. I
have docbook-xsl 1.76.1+dfsg-1ubuntu1. Looks like I need to update it to
1.77.1, but 12.04 doesn't support it readily, will need to force an upgrade.

Amit Langote, on other thread, said that the html docs compile even after
those errors. I didn't notice that. I thought, the docs didn't get built
because of those errors. So, probably I am good right now.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] pg_partman 3.0.0 - real-world usage of native partitioning and a case for native default

[Ashutosh Bapat]

On Mon, Apr 3, 2017 at 10:45 PM, Keith Fiske  wrote:

>
> On Mon, Apr 3, 2017 at 5:13 AM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>>
>>
>> On Fri, Mar 31, 2017 at 9:00 PM, Keith Fiske  wrote:
>>
>>> I've gotten pg_partman working with native partitioning already so I can
>>> hopefully have things ready to work when 10 is released. I've got a branch
>>> on github with this version for anyone to test and I'll hopefully have this
>>> released in the next few weeks after I finish some more testing myself. Any
>>> feedback would be appreciated!
>>>
>>> https://github.com/keithf4/pg_partman/tree/v3.0.0
>>>
>>
>> There's already a proposal to support default partition as [1]. That
>> proposal talks about default partition in list partitioned tables. For
>> range partitioned tables, we expect that a single partition with unbounded
>> bounds would serve as default partition.
>>
>
> This would not work. The completely unbounded partition would overlap all
> other possible partitions. How would it decide which child table to put
> data in? Looks like this is stopped right from the start anyway.
>
> keith@keith=# create table testing_range (id int, created_at timestamptz)
> partition by range (created_at);
> CREATE TABLE
> Time: 41.987 ms
>
> keith@keith=# create table testing_range_default partition of
> testing_range for values from (unbounded) to (unbounded);
> CREATE TABLE
> Time: 8.625 ms
>
> keith@keith=# create table testing_range_p2017_04 partition of
> testing_range for values from ('2017-04-01 00:00:00') to ('2017-05-01
> 00:00:00');
> ERROR:  partition "testing_range_p2017_04" would overlap partition
> "testing_range_default"
> Time: 4.516 ms
>
>

Hmm, looks like default partition for range would be helpful 1. in these
case and 2. to hold data in the holes in the existing partitioning scheme.


>
>>
>>>
>>>
>>> Thankfully since native partitioning still uses inheritance internally
>>> for the most part, pg_partman works pretty well without nearly as much
>>> change as I thought I would need. The biggest deficiency I'm seeing has to
>>> do with not having a "default" partition to put data that doesn't match any
>>> children. The fact that it throws an error is a concern, but it's not where
>>> I see the main problem. Where this really comes into play is when someone
>>> wants to make an existing table into a partitioned table. There's really no
>>> easy way to do this outside of making a completely brand new partition set
>>> and copying/moving the data from the old to the new.
>>>
>>
>> If there are multiple partitions, there is likely to be more data that
>> needs to be moved that is retained in the old table. So, creating complete
>> brand new partitioning and copying/moving data is annoying but not as much
>> as it sounds. Obviously, if we could avoid it, we should try to.
>>
>
> Not sure I follow what you're saying here. With pg_partman, making the old
> table the parent and still containing all the data has caused no issues
> when I've migrated clients to it, nor has anyone reported any issues to me
> with this system. New data goes to the child tables as they should and old
> data is then moved when convenient. It makes things work very smoothly and
> the only outage encountered is a brief lock at creation time.
>

In partitioning, partitioned table doesn't have any storage. Data that
belongs to a given partition is expected to be there from day one.


>
> I've been watching that thread as well and as soon as a fix is posted
> about the latest concerns, I'll gladly look into reviewing it.
>

Thanks.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> Where is the statement_timeout timer stopped when processing Parse and Bind 
> messages?

Actually the statement timer is replaced with new statement timer
value in enable_statement_timeout().

> Do you mean the following sequence of operations are performed in this patch?
> 
> Parse(statement1)
>   start timer
>   stop timer
> Bind(statement1, portal1)
>   start timer
>   stop timer
> Execute(portal1)
>   start timer
>   stop timer
> Sync

Yes.

> It looks like the patch does the following.  I think this is desirable, 
> because starting and stopping the timer for each message may be costly as Tom 
> said.
> Parse(statement1)
>   start timer
> Bind(statement1, portal1)
> Execute(portal1)
>   stop timer
> Sync

This doesn't work in general use cases. Following pattern appears
frequently in applications.

Parse(statement1)
Bind(statement1, portal1)
Execute(portal1)
Bind(statement1, portal1)
Execute(portal1)
:
:
Sync

Also what would happen if client just send a parse message and does
nothing after that?

So I think following is better:

Parse(statement1)
Bind(statement1, portal1)
Execute(portal1)
  start timer
  stop timer
Bind(statement1, portal1)
Execute(portal1)
  start timer
  stop timer
:
:
Sync

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] Documentation improvements for partitioning

[Robert Haas]

On Mon, Apr 3, 2017 at 12:52 AM, Amit Langote
 wrote:
> I noticed what looks like a redundant line (or an incomplete sentence) in
> the paragraph about multi-column partition keys.  In the following text:
>
> +   partitioning, if desired.  Of course, this will often result in a
> larger
> +   number of partitions, each of which is individually smaller.
> +   criteria.  Using fewer columns may lead to coarser-grained
> +   A query accessing the partitioned table will have
> +   to scan fewer partitions if the conditions involve some or all of
> these
>
> This:
>
> +   criteria.  Using fewer columns may lead to coarser-grained
>
> looks redundant.  But maybe we can keep that sentence by completing it,
> which the attached patch does as follows:
>
> +   number of partitions, each of which is individually smaller.  On the
> +   other hand, using fewer columns may lead to a coarser-grained
> +   partitioning criteria with smaller number of partitions.
>
> The patch also fixes some typos I noticed.

Thanks for the corrections.  Committed.

-- 
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] Fix obsolete comment in GetSnapshotData

[Robert Haas]

On Sun, Apr 2, 2017 at 9:29 PM, Craig Ringer  wrote:
> You're right, I muddled it with PROCARRAY_VACUUM_FLAG.
>
> PROCARRAY_FLAGS_VACUUM is sufficient.

Committed that way.

-- 
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] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> No. Parse, bind and Execute message indivually stops and starts the
> statement_timeout timer with the patch. Unless there's a lock conflict,
> parse and bind will take very short time. So actually users could only
> observe the timeout in execute message though.

Where is the statement_timeout timer stopped when processing Parse and Bind 
messages?  Do you mean the following sequence of operations are performed in 
this patch?

Parse(statement1)
  start timer
  stop timer
Bind(statement1, portal1)
  start timer
  stop timer
Execute(portal1)
  start timer
  stop timer
Sync

It looks like the patch does the following.  I think this is desirable, because 
starting and stopping the timer for each message may be costly as Tom said.

Parse(statement1)
  start timer
Bind(statement1, portal1)
Execute(portal1)
  stop timer
Sync


> > (3)
> > +   /*
> > +* Sanity check
> > +*/
> > +   if (!xact_started)
> > +   {
> > +   ereport(ERROR,
> > +
>   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +errmsg("Transaction
> must have been already started to set statement timeout")));
> > +   }
> >
> > I think this fragment can be deleted, because enable/disable_timeout()
> is used only at limited places in postgres.c, so I don't see the chance
> of misuse.
> 
> I'd suggest leave it as it is. Because it might be possible that the function
> is used in different place in the future. Or at least we should document
> the pre-condition as a comment.

OK, I favor documenting to reduce code, but I don't have a strong opinion.  I'd 
like to leave this to the committer.  One thing to note is that you can remove 
{ and } in the following code like other places.

+   if (!xact_started)
+   {
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("Transaction must have 
been already started to set statement timeout")));
+   }

Regards
Takayuki Tsunakawa



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

Re: [HACKERS] [sqlsmith] Failed assertion in _hash_kill_items/MarkBufferDirtyHint

[Robert Haas]

On Fri, Mar 31, 2017 at 10:02 PM, Ashutosh Sharma  wrote:
> As suggested, I am now acquiring lock inside the caller function.
> Attached is the patch having this changes. Thanks.

Committed.

-- 
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] If an extension library is loaded during pg_upgrade, can it tell?

[Chapman Flack]

On 04/03/17 22:08, Bruce Momjian wrote:
> On Mon, Apr  3, 2017 at 09:53:34PM -0400, Chapman Flack wrote:
>> Hi,
>>
>> Is there any distinctive state that could be checked by extension code
>> to determine that it has been loaded incidentally during the operation
>> of pg_upgrade rather than under normal conditions?
>> ... 
> You can check the backend global variable IsBinaryUpgrade to check if
> binary upgrade is being performed.  Does that help you?

That sounds like exactly what I'd hoped for. Thanks!

-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] wait event documentation

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> Instead of continuing to repair this every time it gets broken, I propose
> that we break this into one table that lists all the wait_event_type values
> -- LWLock, Lock, BufferPin, Activity, Client, Extension, IPC, Timeout, and
> IO -- and then a second table for each type that has multiple wait events
> listing all of the wait events for that type.
> 
> I also propose hoisting this out of section 28.2.3 - Viewing Statistics
> - and making it a new toplevel section of chapter 28.  So between the current
> "28.3 Viewing Locks" and the current "28.4 Progress Reporting" we'd add
> a new section "Wait Events" and link to that from 28.2.3.  That would also
> give us a place to include any general text that we want to have regarding
> wait events, apart from the tables.

+1
I'm visually impaired and using screen reader software which reads text by 
Synthetic speech.  It was not easy for me to navigate a large table to find the 
first row of a particular wait event type.  With your proposal, I'll be able to 
move among tables with a single key press ("T" and "Shift + T".)  I'd also be 
happy about separating the section for wait events as you propose, because 
navigating in a long section is cumbersome.

Regards
Takayuki Tsunakawa




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

[Robert Haas]

On Mon, Apr 3, 2017 at 5:09 PM, Andres Freund  wrote:
> To me this hasn't gotten even remotely enough performance evaluation.
> And I don't think it's fair to characterize it as pending since 2013,
> given it was essentially "waiting on author" for most of that.

This is undeniably a patch which has been kicking around for a lot of
time without getting a lot of attention, and if it just keeps getting
punted down the road, it's never going to become committable.
Alexander's questions upthread about what decisions the committer who
took an interest (Heikki) would prefer never really got an answer, for
example.  I don't deny that there may be some work left to do here,
but I think blaming the author for a week's delay when this has been
ignored so often for so long is unfair.

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

[Robert Haas]

On Mon, Apr 3, 2017 at 4:17 PM, Andres Freund  wrote:
> Hm.  I'm not really convinced by the logic here.  Wouldn't it be better
> to try to compute the minimum total cost across all workers for
> 1..#max_workers for the plans in an iterative manner?  I.e. try to map
> each of the subplans to 1 (if non-partial) or N workers (partial) using
> some fitting algorith (e.g. always choosing the worker(s) that currently
> have the least work assigned).  I think the current algorithm doesn't
> lead to useful #workers for e.g. cases with a lot of non-partial,
> high-startup plans - imo a quite reasonable scenario.

Well, that'd be totally unlike what we do in any other case.  We only
generate a Parallel Seq Scan plan for a given table with one # of
workers, and we cost it based on that.  We have no way to re-cost it
if we changed our mind later about how many workers to use.
Eventually, we should probably have something like what you're
describing here, but in general, not just for this specific case.  One
problem, of course, is to avoid having a larger number of workers
always look better than a smaller number, which with the current
costing model would probably happen a lot.

-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Etsuro Fujita]

On 2017/04/04 3:21, Andres Freund wrote:

On 2017-02-28 21:45:22 +0900, Etsuro Fujita wrote:

Here is a patch for $subject.


This is a nontrivial patch, submitted just before the start of the last
CF for postgres 10.  Therefore I think we should move this to the next
CF.


Honestly, I'm not satisfied with this patch and I think it would need 
more work.  Moved to the next CF.


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] If an extension library is loaded during pg_upgrade, can it tell?

[Bruce Momjian]

On Mon, Apr  3, 2017 at 09:53:34PM -0400, Chapman Flack wrote:
> Hi,
> 
> Is there any distinctive state that could be checked by extension code
> to determine that it has been loaded incidentally during the operation
> of pg_upgrade rather than under normal conditions?
> 
> PL/Java ordinarily checks what version of its schema is around, but
> that may be premature while pg_upgrade is doing its thing and the schema
> might not be all there yet.

You can check the backend global variable IsBinaryUpgrade to check if
binary upgrade is being performed.  Does that help you?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

[HACKERS] If an extension library is loaded during pg_upgrade, can it tell?

[Chapman Flack]

Hi,

Is there any distinctive state that could be checked by extension code
to determine that it has been loaded incidentally during the operation
of pg_upgrade rather than under normal conditions?

PL/Java ordinarily checks what version of its schema is around, but
that may be premature while pg_upgrade is doing its thing and the schema
might not be all there yet.

-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] Making clausesel.c Smarter

[Claudio Freire]

On Mon, Apr 3, 2017 at 9:19 PM, Claudio Freire  wrote:
> On Mon, Apr 3, 2017 at 8:52 PM, David Rowley
>  wrote:
>>> One last observation:
>>>
>>> +/*
>>> + * An IS NOT NULL test is a no-op if there's any other strict 
>>> quals,
>>> + * so if that's the case, then we'll only apply this, otherwise 
>>> we'll
>>> + * ignore it.
>>> + */
>>> +else if (cslist->selmask == CACHESEL_NOTNULLTEST)
>>> +s1 *= cslist->notnullsel;
>>>
>>> In some paths, the range selectivity estimation code punts and returns
>>> DEFAULT_RANGE_INEQ_SEL. In those cases, if a not null test was
>>> present, it should still be applied. It could make a big difference if
>>> the selectivity for the nulltest is high.
>>
>> I'd say that the location that applies the DEFAULT_RANGE_INEQ_SEL
>> should apply the nullfrac. Seems wrong to rely on a IS NOT NULL test
>> to exists to estimate that properly. I don't think that needs done as
>> part of this patch. I'd rather limit the scope since "returned with
>> feedback" is already knocking at the door of this patch.
>
> I agree, but this patch regresses for those cases if the user
> compensated with a not null test, leaving little recourse, so I'd fix
> it in this patch to avoid the regression.
>
> Maybe you're right that the null fraction should be applied regardless
> of whether there's an explicit null check, though.

The more I read that code, the more I'm convinced there's no way to
get a DEFAULT_RANGE_INEQ_SEL if (rqlist->hibound != DEFAULT_INEQ_SEL
&&
 rqlist->lobound != DEFAULT_INEQ_SEL) other than from contradictory
clauses, a case the current code handles very poorly, returning
DEFAULT_RANGE_INEQ_SEL and ignoring nullfrac, something that could
give way-off estimates if the table had lots of nulls.

Say, consider if "value" was mostly null and you write:

SELECT * FROM testdata WHERE value BETWEEN 10 AND 20 AND value BETWEEN
50 AND 60;

All other cases I can think of would end with either hibound or
lobound being equal to DEFAULT_INEQ_SEL.

It seems to me, doing a git blame, that the checks in the else branch
were left only as a precaution, one that seems unnecessary.

If you ask me, I'd just leave:

+ if (rqlist->hibound == DEFAULT_INEQ_SEL || rqlist->lobound ==
DEFAULT_INEQ_SEL)
+ {
+ /* No point in checking null selectivity, DEFAULT_INEQ_SEL
implies we have no stats */
+ s2 = DEFAULT_RANGE_INEQ_SEL;
+ }
+ else
+ {
...
+   s2 = rqlist->hibound + rqlist->lobound - 1.0;
+   nullsel = nulltestsel(root, IS_NULL, rqlist->var, varRelid);
+   notnullsel = 1.0 - nullsel;
+
+   /* Adjust for double-exclusion of NULLs */
+   s2 += nullsel;
+   if (s2 <= 0.0) {
+  if (s2 <= (-1.0e-4 * notnullsel - 1.0e-6))
+  {
+   /* Most likely contradictory clauses found */
+   s2 = (1.0e-10 < notnullsel) ? 1.0e-10 : notnullsel;
+  }
+  else
+  {
+   /* Could be a rounding error */
+   s2 = DEFAULT_RANGE_INEQ_SEL * notnullsel;
+  }
+   }
+ }

Where (-1.0e-4 * notnullsel - 1.0e-6) is just a very rough (and overly
cautious) estimation of the amount of rounding error that could be
there with 32-bit floats.

The above assumes a non-DEFAULT_INEQ_SEL value in lobound/hibound is
an estimation based on stats, maybe approximate, but one that makes
sense (ie: preserves the monotonicity of the CPF), and as such
negative values are either sign of a contradiction or rounding error.
The previous code left the possibility of negatives coming out of
default selectivities creeping up on non-DEFAULT_INEQ_SEL estimates,
but that doesn't seem possible coming out of scalarineqsel.

But I'd like it if Tom could comment on that, since he's the one that
did that in commit 547bb4a7f2bdccad9253a99211ce84b3f9de485a (way back
in 2004).

Barring that, I'd just change the

s2 = DEFAULT_RANGE_INEQ_SEL;

To

s2 = DEFAULT_RANGE_INEQ_SEL * notnullsel;

Which makes a lot more sense.


-- 
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] ANALYZE command progress checker

[vinayak]

On 2017/03/30 17:39, Masahiko Sawada wrote:

On Wed, Mar 29, 2017 at 5:38 PM, vinayak
 wrote:

On 2017/03/25 4:30, Robert Haas wrote:

On Fri, Mar 24, 2017 at 3:41 AM, vinayak
 wrote:

I have updated the patch.

You can't change the definition of AcquireSampleRowsFunc without
updating the documentation in fdwhandler.sgml, but I think I don't
immediately understand why that's a thing we want to do at all if
neither file_fdw nor postgres_fdw are going to use the additional
argument.  It seems to be entirely pointless because the new value
being passed down is always zero and even if the callee were to update
it, do_analyze_rel() would just ignore the change on return.  Am I
missing something here?  Also, the whitespace-only changes to fdwapi.h
related to AcquireSampleRowsFunc going to get undone by pgindent, so
even if there's some reason to change that you should leave the lines
that don't need changing untouched.

I reviewed v7 patch.

When I ran ANALYZE command to the table having 5 millions rows with 3
child tables, the progress report I got shows the strange result. The
following result was output after sampled all target rows from child
tables (i.g, after finished acquire_inherited_sample_rows). I think
the progress information should report 100% complete at this point.

#= select * from pg_stat_progress_analyze ;
   pid  | datid | datname  | relid |  phase   |
num_target_sample_rows | num_rows_sampled
---+---+--+---+--++--
  81719 | 13215 | postgres | 16440 | collecting inherited sample rows |
300 |  180
(1 row)

I guess the cause of this is that you always count the number of
sampled rows in acquire_sample_rows staring from 0 for each child
table. num_rows_sampled is reset whenever starting collecting sample
rows.
Also, even if table has child table, the total number of sampling row
is not changed. I think that main differences between analyzing on
normal table and on partitioned table is where we fetch the sample row
from. So I'm not sure if adding "computing inherited statistics" phase
is desirable. If you want to report progress of collecting sample rows
on child table I think that you might want to add the information
showing which child table is being analyzed.

--
pg_stat_progress_analyze.num_target_sample_rows is set while ignoring
the number of rows the target table has actually. So If the table has
rows less than target number of rows, the num_rows_sampled never reach
to num_target_sample_rows.

--
@@ -1180,6 +1213,9 @@ acquire_sample_rows(Relation onerel, int elevel,
 }

 samplerows += 1;
+
+   /* Report number of rows sampled so far */
+
pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_SAMPLED,
numrows);
 }
 }

I think that it's wrong to use numrows variable to report the progress
of the number of rows sampled. As the following comment mentioned, we
first save row into rows[] and increment numrows until numrows <
targrows. And then we could replace stored sample row with new sampled
row. That is, after collecting "numrows" rows, analyze can still take
a long time to get and replace the sample row. To computing progress
of collecting sample row, IMO reporting the number of blocks we
scanned so far is better than number of sample rows. Thought?


 /*
  * The first targrows sample rows are simply copied into the
  * reservoir. Then we start replacing tuples in the sample
  * until we reach the end of the relation.  This algorithm is
  * from Jeff Vitter's paper (see full citation below). It
  * works by repeatedly computing the number of tuples to skip
  * before selecting a tuple, which replaces a randomly chosen
  * element of the reservoir (current set of tuples).  At all
  * times the reservoir is a true random sample of the tuples
  * we've passed over so far, so when we fall off the end of
  * the relation we're done.
  */

Looks good to me.
In the attached patch I have reported number of blocks scanned so far 
instead of number of sample rows.


In the 'collecting inherited sample rows' phase, child_relid is reported 
as a separate column.
The child_relid is reported its value only in 'collecting inherited 
sample rows' phase. For single relation it return 0.

I am not sure whether this column would helpful or not.
Any suggestions are welcome.

+/* Reset rows processed to zero for the next column */
+
pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED,
0);

This seems like a bad design.  If you see a value other than zero in
this field, you still won't know how much progress analyze has made,
because you don't know which column you're processing.  I also feel
like 

Re: [HACKERS] logical decoding of two-phase transactions

[Masahiko Sawada]

On Thu, Mar 30, 2017 at 12:55 AM, Stas Kelvich  wrote:
>
>> On 28 Mar 2017, at 18:08, Andres Freund  wrote:
>>
>> On 2017-03-28 15:55:15 +0100, Simon Riggs wrote:
>>>
>>>
>>> That assertion is obviously false... the plugin can resolve this in
>>> various ways, if we allow it.
>>
>> Handling it by breaking replication isn't handling it (e.g. timeouts in
>> decoding etc).  Handling it by rolling back *prepared* transactions
>> (which are supposed to be guaranteed to succeed!), isn't either.
>>
>>
>>> You can say that in your opinion you prefer to see this handled in
>>> some higher level way, though it would be good to hear why and how.
>>
>> It's pretty obvious why: A bit of DDL by the user shouldn't lead to the
>> issues mentioned above.
>>
>>
>>> Bottom line here is we shouldn't reject this patch on this point,
>>
>> I think it definitely has to be rejected because of that.  And I didn't
>> bring this up at the last minute, I repeatedly brought it up before.
>> Both to Craig and Stas.
>
> Okay. In order to find more realistic cases that blocks replication
> i’ve created following setup:
>
> * in backend: tests_decoding plugins hooks on xact events and utility
> statement hooks and transform each commit into prepare, then sleeps
> on latch. If transaction contains DDL that whole statement pushed in
> wal as transactional message. If DDL can not be prepared or disallows
> execution in transaction block than it goes as nontransactional logical
> message without prepare/decode injection. If transaction didn’t issued any
> DDL and didn’t write anything to wal, then it skips 2pc too.
>
> * after prepare is decoded, output plugin in walsender unlocks backend
> allowing to proceed with commit prepared. So in case when decoding
> tries to access blocked catalog everything should stop.
>
> * small python script that consumes decoded wal from walsender (thanks
> Craig and Petr)
>
> After small acrobatics with that hooks I’ve managed to run whole
> regression suite in parallel mode through such setup of test_decoding
> without any deadlocks. I’ve added two xact_events to postgres and
> allowedn prepare of transactions that touched temp tables since
> they are heavily used in tests and creates a lot of noise in diffs.
>
> So it boils down to 3 failed regression tests out of 177, namely:
>
> * transactions.sql — here commit of tx stucks with obtaining SafeSnapshot().
> I didn’t look what is happening there specifically, but just checked that
> walsender isn’t blocked. I’m going to look more closely at this.
>
> * prepared_xacts.sql — here select prepared_xacts() sees our prepared
> tx. It is possible to filter them out, but obviously it works as expected.
>
> * guc.sql — here pendingActions arrives on 'DISCARD ALL’ preventing tx
> from being prepared. I didn’t found the way to check presence of
> pendingActions outside of async.c so decided to leave it as is.
>
> It seems that at least in regression tests nothing can block twophase
> logical decoding. Is that strong enough argument to hypothesis that current
> approach doesn’t creates deadlock except locks on catalog which should be
> disallowed anyway?
>
> Patches attached. logical_twophase_v5 is slightly modified version of previous
> patch merged with Craig’s changes. Second file is set of patches over previous
> one, that implements logic i’ve just described. There is runtest.sh script 
> that
> setups postgres, runs python logical consumer in background and starts
> regression test.
>
>

I reviewed this patch but when I tried to build contrib/test_decoding
I got the following error.

$ make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE   -c -o
test_decoding.o test_decoding.c -MMD -MP -MF .deps/test_decoding.Po
test_decoding.c: In function '_PG_init':
test_decoding.c:126: warning: assignment from incompatible pointer type
test_decoding.c: In function 'test_decoding_process_utility':
test_decoding.c:271: warning: passing argument 5 of
'PreviousProcessUtilityHook' from incompatible pointer type
test_decoding.c:271: note: expected 'struct QueryEnvironment *' but
argument is of type 'struct DestReceiver *'
test_decoding.c:271: warning: passing argument 6 of
'PreviousProcessUtilityHook' from incompatible pointer type
test_decoding.c:271: note: expected 'struct DestReceiver *' but
argument is of type 'char *'
test_decoding.c:271: error: too few arguments to function
'PreviousProcessUtilityHook'
test_decoding.c:276: warning: passing argument 5 of
'standard_ProcessUtility' from incompatible pointer type
../../src/include/tcop/utility.h:38: note: expected 'struct
QueryEnvironment *' but argument is of type 'struct DestReceiver *'
test_decoding.c:276: warning: passing argument 6 of
'standard_ProcessUtility' from incompatible pointer type

[HACKERS] Remove pg_stat_progress_vacuum from Table 28.2

[Amit Langote]

It seems pg_stat_progress_vacuum is not supposed to appear in the table
titled "Collected Statistics Views".  It was added by c16dc1aca.  Attached
patch fixes that.

Thanks,
Amit
>From 779fd54f0e30455e0393252a4ba4514d23e0af15 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 4 Apr 2017 10:16:53 +0900
Subject: [PATCH] Remove pg_stat_progress_vacuum from Table 28.2

---
 doc/src/sgml/monitoring.sgml | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 9856968997..dbd4d54120 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -514,13 +514,6 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
   calls during the current transaction (which are not
   yet included in pg_stat_user_functions).
  
-
- 
-  pg_stat_progress_vacuumpg_stat_progress_vacuum
-  One row for each backend (including autovacuum worker processes) running
-  VACUUM, showing current progress.
-  See .
- 
 

   
-- 
2.11.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] show "aggressive" or not in autovacuum logs

[Kyotaro HORIGUCHI]

Hello,

At Fri, 31 Mar 2017 18:20:23 +0900, Masahiko Sawada  
wrote in 
> On Wed, Mar 29, 2017 at 12:46 PM, Kyotaro HORIGUCHI
>  wrote:
> > Hello, it would be too late but I'd like to propose this because
> > this cannot be back-patched.
> >
> >
> > In autovacuum logs, "%u skipped frozen" shows the number of pages
> > skipped by ALL_FROZEN only in aggressive vacuum.
> >
> > So users cannot tell whether '0 skipped-frozen' means a
> > non-agressive vacuum or no frozen-pages in an agressive vacuum.
> >
> > I think it is nice to have an indication whether the scan was
> > "agressive" or not in log output.
> 
> Good idea. I also was thinking about this.

Thanks. Currently we cannot use "skipped-frozen" to see the
effect of ALL_FROZEN.

> > Like this,
> >
> >> LOG:  automatic aggressive vacuum of table 
> >> "template1.pg_catalog.pg_statistic": index scans: 0
> >
> > "0 skipped frozen" is uesless in non-aggressive vacuum but
> > removing it would be too-much.  Inserting "aggressive" reduces
> > machine-readability so it might be better in another place. The
> > attached patch does the following.
> >
> >>  LOG:  automatic vacuum of table "postgres.public.pgbench_branches": mode: 
> >> normal, index scans: 0
> >>  LOG:  automatic vacuum of table "postgres.public.pgbench_branches": mode: 
> >> aggressive, index scans: 0
> >
> 
> Should we add this even to the manual vacuum verbose message?

I forgot that. The patch adds the mode indication in the first
message of VACUUM VERBOSE.

| =# vacuum freeze verbose it;
| INFO:  vacuuming "public.it" in aggressive mode
| INFO:  "it": found 0 removable, 0 nonremovable row versions in 0 out of 0 
pages
...
| Skipped 0 pages due to buffer pins, 0 frozen pages.

I still feel a bit uneasy about the word "aggressive" here. Is it
better to be "freezing" or something?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 1027e08d096068f4841343a1adf6e4db61c0d218 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Tue, 4 Apr 2017 09:56:11 +0900
Subject: [PATCH] Show "aggressive" or not in vacuum messages

VACUUM VERBOSE or autovacuum emits log message with "n skipped-frozen"
but we cannot tell whether the vacuum was non-freezing (or not
aggressive) vacuum or freezing (or aggressive) vacuum with no tuple to
freeze. This patch adds indication of "aggressive" or "normal" in
autovacuum logs and VACUUM VERBOSE messages.
---
 src/backend/commands/vacuumlazy.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 5b43a66..9d02327 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -374,10 +374,11 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 			 * emitting individual parts of the message when not applicable.
 			 */
 			initStringInfo();
-			appendStringInfo(, _("automatic vacuum of table \"%s.%s.%s\": index scans: %d\n"),
+			appendStringInfo(, _("automatic vacuum of table \"%s.%s.%s\": mode: %s, index scans: %d\n"),
 			 get_database_name(MyDatabaseId),
 			 get_namespace_name(RelationGetNamespace(onerel)),
 			 RelationGetRelationName(onerel),
+			 aggressive ? "aggressive" : "normal",
 			 vacrelstats->num_index_scans);
 			appendStringInfo(, _("pages: %u removed, %u remain, %u skipped due to pins, %u skipped frozen\n"),
 			 vacrelstats->pages_removed,
@@ -488,9 +489,9 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 
 	relname = RelationGetRelationName(onerel);
 	ereport(elevel,
-			(errmsg("vacuuming \"%s.%s\"",
+			(errmsg("vacuuming \"%s.%s\" in %s mode",
 	get_namespace_name(RelationGetNamespace(onerel)),
-	relname)));
+	relname, aggressive ? "aggressive" : "normal")));
 
 	empty_pages = vacuumed_pages = 0;
 	num_tuples = tups_vacuumed = nkeep = nunused = 0;
-- 
2.9.2


-- 
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 bitmapscan isn't exercised in regression tests

[Dilip Kumar]

On Mon, Apr 3, 2017 at 11:22 PM, Andres Freund  wrote:
> That's better than before, but I'd appreciate working on a bit more
> coverage. E.g. rescans probably aren't exercised in that test, right?
>
> If you have time & energy, it'd also be good to expand the tests to
> cover the prefetching logic - it's quite bad that it's currently not
> tested at all :(

Sure I can do that, In attached patch, I only fixed the problem of not
executing the bitmap test.  Now, I will add few cases to cover other
parts especially rescan and prefetching logic.


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


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

Re: [HACKERS] Making clausesel.c Smarter

[Claudio Freire]

On Mon, Apr 3, 2017 at 8:52 PM, David Rowley
 wrote:
>> One last observation:
>>
>> +/*
>> + * An IS NOT NULL test is a no-op if there's any other strict quals,
>> + * so if that's the case, then we'll only apply this, otherwise 
>> we'll
>> + * ignore it.
>> + */
>> +else if (cslist->selmask == CACHESEL_NOTNULLTEST)
>> +s1 *= cslist->notnullsel;
>>
>> In some paths, the range selectivity estimation code punts and returns
>> DEFAULT_RANGE_INEQ_SEL. In those cases, if a not null test was
>> present, it should still be applied. It could make a big difference if
>> the selectivity for the nulltest is high.
>
> I'd say that the location that applies the DEFAULT_RANGE_INEQ_SEL
> should apply the nullfrac. Seems wrong to rely on a IS NOT NULL test
> to exists to estimate that properly. I don't think that needs done as
> part of this patch. I'd rather limit the scope since "returned with
> feedback" is already knocking at the door of this patch.

I agree, but this patch regresses for those cases if the user
compensated with a not null test, leaving little recourse, so I'd fix
it in this patch to avoid the regression.

Maybe you're right that the null fraction should be applied regardless
of whether there's an explicit null check, though.


-- 
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] delta relations in AFTER triggers

[Thomas Munro]

On Tue, Apr 4, 2017 at 3:41 AM, Kevin Grittner  wrote:
> On Mon, Apr 3, 2017 at 8:59 AM, Tom Lane  wrote:
>> Thomas Munro  writes:
>>> Or perhaps the code to inject trigger data transition tables into SPI
>>> (a near identical code block these three patches) should be somewhere
>>> common so that each PLs would only need to call a function.  If so,
>>> where should that go?
>>
>> spi.c?
>
> Until now, trigger.c didn't know about SPI, and spi.c didn't know
> about triggers.  The intersection was left to referencing code, like
> PLs.  Is there any other common code among the PLs dealing with this
> intersection?  If so, maybe a new triggerspi.c file (or
> spitrigger.c?) would make sense.  Possibly it could make sense from
> a code structure PoV even for a single function, but it seems kinda
> iffy for just this function.  As far as I can see it comes down to
> adding it to spi.c or creating a new file -- or just duplicating
> these 30-some lines of code to every PL.

Ok, how about SPI_register_trigger_data(TriggerData *)?  Or any name
you prefer...  I didn't suggest something as specific as
SPI_register_transition_tables because think it's plausible that
someone might want to implement SQL standard REFERENCING OLD/NEW ROW
AS and make that work in all PLs too one day, and this would be the
place to do that.

See attached, which add adds the call to all four built-in PLs.  Thoughts?

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


transition-tables-for-all.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: Batch/pipelining support for libpq

[Michael Paquier]

On Tue, Apr 4, 2017 at 8:26 AM, Andres Freund  wrote:
> On 2017-04-04 09:24:23 +1000, Vaishnavi Prabakaran wrote:
>> Just quickly, Is it not ok to consider only the code patch for this CF
>> without test patch?
>
> I'd say no, it's not acceptable.  This is too much new code for it not
> to be tested.

Doesn't it depend actually? I would think that the final patch may not
include all the tests implemented if:
- The thread on which a patch has been developed had a set of tests
done and posted with it.
- Including them does not make sense if we have a way to run those
tests more efficiently. Sometimes a bunch of benchmarks or tests are
run on a patch bu for the final result keeping them around does not
make much sense.

In the case of this patch, it seems to me that we would have a far
better portable set of tests if we had a dedicated set of subcommands
available at psql level, particularly for Windows/MSVC. If that's a
requirement for this patch so let it be. I am not saying that tests
are not necessary. They are of course, but in this case having a bit
more infrastructure would be more be more helpful for users and the
tests themselves.

Note that I won't complain either if this set of C tests are included
at the end.
-- 
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] make check-world output

[Noah Misch]

On Mon, Apr 03, 2017 at 03:03:44PM +0900, Michael Paquier wrote:
> On Sat, Apr 1, 2017 at 4:28 PM, Noah Misch  wrote:
> > The pg_upgrade test suite originated in an age when "make check-world" was
> > forbidden to depend on Perl; the choice was a shell script or a C program.  
> > We
> > do maintain vcregress.pl:upgradecheck(), a Windows-specific Perl port of the
> > suite.  Maintaining both versions has been tedious.  I'd welcome a
> > high-quality rewrite using src/test/perl facilities.
> 
> I need to check in my archives, but I recall having a WIP patch doing that:
> - remove the shell scripts in src/bin/pg_upgrade.
> - remove upgradecheck in vcregress.pl.
> - add a TAP test in src/bin/pg_upgrade to run the upgrade tests.
> So you are interested by that?

No, not really.  A WIP patch is the easy part.  The bulk of the work is giving
it all the coverage, debuggability, and robustness of the current harness.


-- 
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] Making clausesel.c Smarter

[David Rowley]

On 4 April 2017 at 11:35, Claudio Freire  wrote:
> I'd prefer it if all occurrences of the concept were changed, to
> maintain consistency.
> That would include variables used to hold expressions that refer to
> these as well, as in the case of:
>
> +Node   *var;
> +
> +if (varonleft)
> +var = leftexpr;
> +else
> +var = rightexpr;
> +

Thanks for looking again.
I didn't change that one as there's already a variable named "expr" in
the scope. I thought changing that would mean code churn that I don't
really want to add to the patch. Someone else might come along and ask
me why I'm renaming this unrelated variable.  I kinda of think that if
it was var before when it meant expr, then it's not the business of
this patch to clean that up. I didn't rename the struct member, for
example, as the meaning is no different than before.

> One last observation:
>
> +/*
> + * An IS NOT NULL test is a no-op if there's any other strict quals,
> + * so if that's the case, then we'll only apply this, otherwise we'll
> + * ignore it.
> + */
> +else if (cslist->selmask == CACHESEL_NOTNULLTEST)
> +s1 *= cslist->notnullsel;
>
> In some paths, the range selectivity estimation code punts and returns
> DEFAULT_RANGE_INEQ_SEL. In those cases, if a not null test was
> present, it should still be applied. It could make a big difference if
> the selectivity for the nulltest is high.

I'd say that the location that applies the DEFAULT_RANGE_INEQ_SEL
should apply the nullfrac. Seems wrong to rely on a IS NOT NULL test
to exists to estimate that properly. I don't think that needs done as
part of this patch. I'd rather limit the scope since "returned with
feedback" is already knocking at the door of this patch.



-- 
 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] Rewriting the test of pg_upgrade as a TAP test

[Michael Paquier]

On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost  wrote:
> Not good if it lowers the coverage, but hopefully that's fixable. Have you
> analyzed where we're reducing coverage..?

The current set of tests is just running pg_upgrade using the same
version for the source and target instances. Based on that I am not
lowering what is happening in this set of tests. Just doing some
cleanup.

> As for what I'm remembering, there's this:
> https://www.postgresql.org/message-id/5669acd9-efdc-2a0f-afea-10ba6003a...@dunslane.net
>
> Of course, it's possible I misunderstood..

This invokes directly pg_upgrade, so that's actually a third,
different way to test pg_upgrade on top of the two existing methods
that are used in vcregress.pl and pg_upgrade's test.sh

> That seems focused on upgrading and I'd really like to see a general way to
> do this with the TAP structure, specifically so we can test pg_dump and psql
> against older versions.  Having the ability to then be run under the
> coverage testing would be fantastic and would help a great deal with the
> coverage report.

I don't disagree with that. What we need first is some logic to store
in a temporary directory the installation of all the previous major
versions that we have. For example use a subfolder in tmp_install
tagged with the major version number, and then when the TAP test
starts we scan for all the versions present in tmp_install and test
the upgrade with a full grid. One issue though is that we add
$(bindir) in PATH and that there is currently no logic to change PATH
automatically depending on the major/minor versions you are working
on.

So in short I don't think that this lack of infrastructure should be a
barrier for what is basically a cleanup but... I just work here.
-- 
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] Making clausesel.c Smarter

[Claudio Freire]

On Mon, Apr 3, 2017 at 5:59 AM, David Rowley
 wrote:
>> +static void addCachedSelectivityRangeVar(CachedSelectivityClause
>> **cslist, Node *var,
>> bool varonleft, bool isLTsel, Selectivity s2);
>>
>> You're changing clause -> var throughout the code when dealing with
>> nodes, but looking at their use, they hold neither. All those
>> addCachedSelectivity functions are usually passed expression nodes. If
>> you're renaming, perhaps you should rename to "expr".
>
> hmm, this is true. I kind of followed the lead of the name of the
> variable in the old RangeQueryClause struct. I have changed the names
> of these to be expr in the attached, but I didn't change the name of
> the Var in the new CachedSelectivityClause struct. It seems like a
> change not related to this patch.
>
> Do you think I should change that too?

I'd prefer it if all occurrences of the concept were changed, to
maintain consistency.
That would include variables used to hold expressions that refer to
these as well, as in the case of:

+Node   *var;
+
+if (varonleft)
+var = leftexpr;
+else
+var = rightexpr;
+


>> I would suggest avoiding making the assumption that it doesn't unless
>> the operator is strict.
>>
>> One could argue that such an operator should provide its own
>> selectivity estimator, but the strictness check shouldn't be too
>> difficult to add, and I think it's a better choice.
>>
>> So you'd have to check that:
>>
>> default:
>> if (op_strict(expr->opno) && func_strict(expr->opfuncid))
>> addCachedSelectivityOtherClause(, var, s2);
>> else
>> s1 = s1 * s2;
>> break;
>>
>
> I've changed it to something along those lines. I don't think the
> func_strict here is required, though, so I've gone with just
> op_strict().

Indeed, I realized right after typing it, I thought I had corrected it
before I hit send. Seems I hadn't. Good thing you noticed.

One last observation:

+/*
+ * An IS NOT NULL test is a no-op if there's any other strict quals,
+ * so if that's the case, then we'll only apply this, otherwise we'll
+ * ignore it.
+ */
+else if (cslist->selmask == CACHESEL_NOTNULLTEST)
+s1 *= cslist->notnullsel;

In some paths, the range selectivity estimation code punts and returns
DEFAULT_RANGE_INEQ_SEL. In those cases, if a not null test was
present, it should still be applied. It could make a big difference if
the selectivity for the nulltest is high.


-- 
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: Batch/pipelining support for libpq

[Andres Freund]

On 2017-04-04 09:24:23 +1000, Vaishnavi Prabakaran wrote:
> Hi,
> On Tue, Apr 4, 2017 at 7:05 AM, Andres Freund  wrote:
> 
> > On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > > > The CF has been extended until April 7 but time is still growing short.
> > > > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or
> > this
> > > > submission will be marked "Returned with Feedback".
> > > >
> > > >
> > > Thanks for the information, attached the latest patch resolving one
> > > compilation warning. And, please discard the test patch as it will be
> > > re-implemented later separately.
> >
> > Hm.  If the tests aren't ready yet, it seems we'll have to move this to
> > the next CF.
> >
> >
> Thanks for your review and I will address your review comments and send the
> newer version of patch shortly.

Cool.

> Just quickly, Is it not ok to consider only the code patch for this CF
> without test patch?

I'd say no, it's not acceptable.  This is too much new code for it not
to be tested.

Andres


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

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

[Vaishnavi Prabakaran]

Hi,
On Tue, Apr 4, 2017 at 7:05 AM, Andres Freund  wrote:

> On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > > The CF has been extended until April 7 but time is still growing short.
> > > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or
> this
> > > submission will be marked "Returned with Feedback".
> > >
> > >
> > Thanks for the information, attached the latest patch resolving one
> > compilation warning. And, please discard the test patch as it will be
> > re-implemented later separately.
>
> Hm.  If the tests aren't ready yet, it seems we'll have to move this to
> the next CF.
>
>
Thanks for your review and I will address your review comments and send the
newer version of patch shortly.
Just quickly, Is it not ok to consider only the code patch for this CF
without test patch?

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

Re: [HACKERS] Should we cacheline align PGXACT?

[Jim Van Fleet]

pgsql-hackers-ow...@postgresql.org wrote on 04/03/2017 01:58:03 PM:

> From: Andres Freund 
> To: Alexander Korotkov 
> Cc: David Steele , Ashutosh Sharma 
> , Simon Riggs , Alvaro
> Herrera , Robert Haas 
> , Bernd Helmle , Tomas 
> Vondra , pgsql-hackers  hack...@postgresql.org>
> Date: 04/03/2017 01:59 PM
> Subject: Re: [HACKERS] Should we cacheline align PGXACT?
> Sent by: pgsql-hackers-ow...@postgresql.org
> 
> On 2017-03-25 19:35:35 +0300, Alexander Korotkov wrote:
> > On Wed, Mar 22, 2017 at 12:23 AM, David Steele  
wrote:
> > 
> > > Hi Alexander
> > >
> > > On 3/10/17 8:08 AM, Alexander Korotkov wrote:
> > >
> > > Results look good for me.  Idea of committing both of patches looks
> > >> attractive.
> > >> We have pretty much acceleration for read-only case and small
> > >> acceleration for read-write case.
> > >> I'll run benchmark on 72-cores machine as well.
> > >>
> > >
> > > Have you had a chance to run those tests yet?
> > >
> > 
> > I discovered an interesting issue.
> > I found that ccce90b3 (which was reverted) gives almost same effect as
> > PGXACT alignment on read-only test on 72-cores machine.
> 
> That's possibly because it changes alignment?
> 
> 
> > That shouldn't be related to the functionality of ccce90b3 itself, 
because
> > read-only test don't do anything with clog.  And that appears to be 
true.
> > Padding of PGPROC gives same positive effect as ccce90b3.  Padding 
patch
> > (pgproc-pad.patch) is attached.  It's curious that padding changes 
size of
> > PGPROC from 816 bytes to 848 bytes.  So, size of PGPROC remains 
16-byte
> > aligned.  So, probably effect is related to distance between PGPROC
> > members...
> > 
> > See comparison of 16-bytes alignment of PGXACT + reduce PGXACT access 
vs.
> > padding of PGPROC.
> 
> My earlier testing had showed that padding everything is the best
> approach :/
>
My approach has been to, generally, pad "everything" as well.  In my 
testing on power, I padded  PGXACT to 16 bytes.  To my surprise, with the 
padding in isolation, the performance (on hammerdb) was slightly degraded.

Jim Van Fleet 
> 
> I'm inclined to push this to the next CF, it seems we need a lot more
> benchmarking here.
> 
> Greetings,
> 
> Andres Freund
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
> 

Re: [HACKERS] Variable substitution in psql backtick expansion

[David G. Johnston]

On Mon, Apr 3, 2017 at 5:12 AM, Daniel Verite 
wrote:

> Queries can be as complex as necessary, they just have to fit in one line.


​Line continuation in general is missed though I thought something already
when in for 10.0 that improves upon this...​


> In no way at all,in the sense that, either you choose to use an SQL
> evaluator, or a client-side evaluator (if it exists), or a backtick
> expression.
> They are mutually exclusive for a single \if invocation.
>
> Client-side evaluation would have to be called with a syntax
> that is unambiguously different.


​Is that the universe: server, client, shell?

Shell already has backticks required
​Server, being the most common, ideally wouldn't need demarcation
Client thus would want its own symbol pairing to distinguish it from server.

Server doesn't need a leading marker but do we want to limit it to single
statements only and allow an optional trailing semi-colon (or backslash
command) which, if present, would end the "server" portion of the string
and allow for treatment of additional terms on the same line to be parsed
in a different context?

I'm conceptually for the implied "SELECT" idea.  It overlaps with plpgsql
syntax.

David J.

Re: [HACKERS] Rewriting the test of pg_upgrade as a TAP test

[Stephen Frost]

Michael,

On Mon, Apr 3, 2017 at 18:29 Michael Paquier 
wrote:

> On Tue, Apr 4, 2017 at 12:12 AM, Stephen Frost  wrote:
> > I'm very curious what you're thinking here?  IIRC, Andrew had some ideas
> > for how to do true cross-version testing with TAP in the buildfarm, but
> > I don't think we actually have that yet..?
>
> I heard about nothing in this area. Cross-branch tests may be an
> interesting challenge as tests written in branch X may not be in Y.
> The patch presented here does lower the coverage we have now.


Not good if it lowers the coverage, but hopefully that's fixable. Have you
analyzed where we're reducing coverage..?

As for what I'm remembering, there's this:
https://www.postgresql.org/message-id/5669acd9-efdc-2a0f-afea-10ba6003a...@dunslane.net

Of course, it's possible I misunderstood..

That seems focused on upgrading and I'd really like to see a general way to
do this with the TAP structure, specifically so we can test pg_dump and
psql against older versions.  Having the ability to then be run under the
coverage testing would be fantastic and would help a great deal with the
coverage report.

Thanks!

Stephen

Re: [HACKERS] Candidate for local inline function?

[Andres Freund]

On 2017-03-17 15:29:27 -0500, Kevin Grittner wrote:
> On Fri, Mar 17, 2017 at 3:23 PM, Andres Freund  wrote:
> > On 2017-03-17 15:17:33 -0500, Kevin Grittner wrote:
> >> Why do we warn of a hazard here instead of eliminating said hazard
> >> with a static inline function declaration in executor.h?
> >
> > Presumably because it was written long before we started relying on
> > inline functions :/
> 
> Right.  git blame says it was changed in 2004.
> 
> >> /*
> >>  * ExecEvalExpr was formerly a function containing a switch statement;
> >>  * now it's just a macro invoking the function pointed to by an ExprState
> >>  * node.  Beware of double evaluation of the ExprState argument!
> >>  */
> >> #define ExecEvalExpr(expr, econtext, isNull) \
> >> ((*(expr)->evalfunc) (expr, econtext, isNull))
> >>
> >> Should I change that to a static inline function doing exactly what
> >> the macro does?  In the absence of multiple evaluations of a
> >> parameter with side effects, modern versions of gcc have generated
> >> the same code for a macro versus a static inline function, at least
> >> in the cases I checked.
> >
> > I'm absolutely not against changing this to an inline function, but I'd
> > prefer if that code weren't touched quite right now, there's a large
> > pending patch of mine in the area.  If you don't mind, I'll just include
> > the change there, rather than have a conflict?
> 
> Fine with me.

For posterities sake: I've indeed done so.

- Andres


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

Re: [HACKERS] Rewriting the test of pg_upgrade as a TAP test

[Michael Paquier]

On Tue, Apr 4, 2017 at 12:12 AM, Stephen Frost  wrote:
> I'm very curious what you're thinking here?  IIRC, Andrew had some ideas
> for how to do true cross-version testing with TAP in the buildfarm, but
> I don't think we actually have that yet..?

I heard about nothing in this area. Cross-branch tests may be an
interesting challenge as tests written in branch X may not be in Y.
The patch presented here does lower the coverage we have now.
-- 
Michael


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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Fabien COELHO]

\set d sqrt(1 + random(1000) * 17)
\echo :d
sqrt(1+random(1000)*17)

I assume we want to keep that pre-existing behavior of \set in
psql,


Ok. So no interpreted expression ever in psql's \set for backward 
compatibility.


that is, making a copy of that string and associating a name to it, 
whereas I guess pgbench computes a value from it and stores that value.


Actually it stores the parsed tree, ready to be executed.

--
Fabien.


--
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] Supporting huge pages on Windows

[Andres Freund]

On 2017-04-03 04:56:45 +, Tsunakawa, Takayuki wrote:
> +/*
> + * EnableLockPagesPrivilege
> + *
> + * Try to acquire SeLockMemoryPrivilege so we can use large pages.
> + */
> +static bool
> +EnableLockPagesPrivilege(int elevel)
> +{
> + HANDLE hToken;
> + TOKEN_PRIVILEGES tp;
> + LUID luid;
> +
> + if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | 
> TOKEN_QUERY, ))
> + {
> + ereport(elevel,
> + (errmsg("could not enable Lock Pages in Memory 
> user right: error code %lu", GetLastError()),
> +  errdetail("Failed system call was %s.", 
> "OpenProcessToken")));
> + return FALSE;
> + }

I don't think the errdetail is quite right - OpenProcessToken isn't
really a syscall, is it? But then it's a common pattern already in
wind32_shmem.c...


> + if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, ))
> + {
> + ereport(elevel,
> + (errmsg("could not enable Lock Pages in Memory 
> user right: error code %lu", GetLastError()),
> +  errdetail("Failed system call was %s.", 
> "LookupPrivilegeValue")));

Other places in the file actually log the arguments to the function...

Wonder if we should quote "Lock Pages in Memory" or add dashes, to make
sure it's clear that that's the right?


> + if (huge_pages == HUGE_PAGES_ON || huge_pages == HUGE_PAGES_TRY)
> + {
> + /* Does the processor support large pages? */
> + largePageSize = GetLargePageMinimum();
> + if (largePageSize == 0)
> + {
> + ereport(huge_pages == HUGE_PAGES_ON ? FATAL : DEBUG1,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +  errmsg("the processor does not support 
> large pages")));
> + ereport(DEBUG1,
> + (errmsg("disabling huge pages")));
> + }
> + else if (!EnableLockPagesPrivilege(huge_pages == HUGE_PAGES_ON 
> ? FATAL : DEBUG1))
> + {
> + ereport(DEBUG1,
> + (errmsg("disabling huge pages")));
> + }
> + else
> + {
> + /* Huge pages available and privilege enabled, so turn 
> on */
> + flProtect = PAGE_READWRITE | SEC_COMMIT | 
> SEC_LARGE_PAGES;

Why don't we just add the relevant flag, instead of overwriting the
previous contents?




> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
> index c63819b..2358ed0 100644
> --- a/src/bin/pg_ctl/pg_ctl.c
> +++ b/src/bin/pg_ctl/pg_ctl.c
> @@ -1708,11 +1708,6 @@ typedef BOOL (WINAPI * __SetInformationJobObject) 
> (HANDLE, JOBOBJECTINFOCLASS, L
>  typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE);
>  typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, 
> JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD);
>  
> -/* Windows API define missing from some versions of MingW headers */
> -#ifndef  DISABLE_MAX_PRIVILEGE
> -#define DISABLE_MAX_PRIVILEGE0x1
> -#endif
> -

>  /*
>   * Create a restricted token, a job object sandbox, and execute the specified
>   * process with it.
> @@ -1794,7 +1789,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION 
> *processInfo, bool as_ser
>   }
>  
>   b = _CreateRestrictedToken(origToken,
> -
> DISABLE_MAX_PRIVILEGE,
> +0,
>  sizeof(dropSids) / 
> sizeof(dropSids[0]),
>  dropSids,
>  0, NULL,

Uh - isn't that a behavioural change?  Was this discussed?

Greetings,

Andres Freund


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

Re: [HACKERS] Making clausesel.c Smarter

[Claudio Freire]

On Mon, Apr 3, 2017 at 6:22 PM, David Rowley
 wrote:
> On 4 April 2017 at 08:24, Andres Freund  wrote:
>> On 2017-04-03 20:59:42 +1200, David Rowley wrote:
>>> Updated patch attached.
>>>
>>> Thanks for reviewing it.
>>
>> Given the time in the release cycle I'm afraid that this it's too late
>> to get this into v10.  Does anybody disagree?  If not, it should be
>> moved to the next CF.
>
> I strongly disagree. The time in the release cycle is the final
> commitfest. This is when patches are commited to the repository. The
> exception to this is that no large invasive patches should arrive new
> in the final commitfest. This is not one of those.

Selectivity misestimations can have a huge impact on query
performance, and that could be a big deal if there's a big regression
on some queries.

But, the beta period could be enough to detect any issues there. I'll
leave that decision to committers or commitfest managers I guess.

>
> Tom has already mentioned he'd like to look at this. If you're not
> willing, then please just don't look at it, but please also don't
> remove other peoples opportunity for doing so.
>
> Please explain your logic for thinking otherwise. Have I somehow
> misunderstood what the 1 week extension means?

But Tom hasn't yet, and even if he does, we'd have to be done with the
review within this week.

In any case, I intend to go on with the review later today, and at
first look the patch seems in good shape (though it'll take a deeper
look before I make that my official review). Most other patches in
need of review are a bit complex for my knowledge of the code base,
and I already started reviewing this one. We could perhaps be done by
the end of the week.

We could defer that decision to within 4 days. If we're done, we're
done. If we're not, move to next CF.


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

[Alexander Korotkov]

On Tue, Apr 4, 2017 at 12:09 AM, Andres Freund  wrote:

> On 2017-04-04 00:04:09 +0300, Alexander Korotkov wrote:
> > > >Thank you!
> > > >I already sent version of patch after David's reminder.
> > > >Please find rebased patch in the attachment.
> > >
> > > Cool. I think that's still a bit late for v10?
> > >
> >
> > I don't know.  ISTM, that I addressed all the issues raised by reviewers.
> > Also, this patch is pending since late 2013.  It would be very nice to
> > finally get it in...
>
> To me this hasn't gotten even remotely enough performance evaluation.
>

I'm ready to put my efforts on that.


> And I don't think it's fair to characterize it as pending since 2013,
>

Probably, this duration isn't good characteristic at all.


> given it was essentially "waiting on author" for most of that.
>

What makes you think so?  Do you have some statistics?  Or is it just
random assumption?

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

Re: [HACKERS] Making clausesel.c Smarter

[Andres Freund]

On 2017-04-04 09:22:07 +1200, David Rowley wrote:
> On 4 April 2017 at 08:24, Andres Freund  wrote:
> > On 2017-04-03 20:59:42 +1200, David Rowley wrote:
> >> Updated patch attached.
> >>
> >> Thanks for reviewing it.
> >
> > Given the time in the release cycle I'm afraid that this it's too late
> > to get this into v10.  Does anybody disagree?  If not, it should be
> > moved to the next CF.
> 
> I strongly disagree. The time in the release cycle is the final
> commitfest. This is when patches are commited to the repository. The
> exception to this is that no large invasive patches should arrive new
> in the final commitfest. This is not one of those.

Right.


> Tom has already mentioned he'd like to look at this.

He also said that it'll be a while, and he hadn't yet by the time of the
original freeze date.


> Please explain your logic for thinking otherwise. Have I somehow
> misunderstood what the 1 week extension means?

Well, we have a lot of pending work, some of that has been pending for
years essentially.  And Tom hadn't looked at it yet, and he wasn't at
pgconf.us, so he wasn't actually delayed due to that...

I'm fine with not punting right now, but we need to reduce the number of
patches and focus on stuff that's realistic to get in. There's 4 days
and ~60 open CF entries.  Please help whip other patches into shape...

- Andres


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

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Thomas Munro]

On Tue, Apr 4, 2017 at 9:11 AM, Andres Freund  wrote:
> Hi,
>
> On 2017-03-31 17:53:12 +1300, Thomas Munro wrote:
>> Thanks very much to Rafia for testing, and to Andres for his copious
>> review feedback.  Here's a new version.  Changes:
>
> I unfortunately think it's too late to get this into v10.  There's still
> heavy development going on, several pieces changed quite noticeably
> since the start of the CF and there's still features missing.  Hence I
> think this unfortunately has to be pushed - as much as I'd have liked to
> have this in 10.
>
> Do you agree?

Agreed.

Thank you very much Andres, Ashutosh, Peter, Rafia and Robert for all
the review, testing and discussion so far.

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


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

Re: [HACKERS] Making clausesel.c Smarter

[David Rowley]

On 4 April 2017 at 08:24, Andres Freund  wrote:
> On 2017-04-03 20:59:42 +1200, David Rowley wrote:
>> Updated patch attached.
>>
>> Thanks for reviewing it.
>
> Given the time in the release cycle I'm afraid that this it's too late
> to get this into v10.  Does anybody disagree?  If not, it should be
> moved to the next CF.

I strongly disagree. The time in the release cycle is the final
commitfest. This is when patches are commited to the repository. The
exception to this is that no large invasive patches should arrive new
in the final commitfest. This is not one of those.

Tom has already mentioned he'd like to look at this. If you're not
willing, then please just don't look at it, but please also don't
remove other peoples opportunity for doing so.

Please explain your logic for thinking otherwise. Have I somehow
misunderstood what the 1 week extension means?


-- 
 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] PATCH: recursive json_populate_record()

[Andres Freund]

On 2017-03-21 14:31:08 -0400, Andrew Dunstan wrote:
>
>
> On 03/21/2017 01:37 PM, David Steele wrote:
> > On 3/16/17 11:54 AM, David Steele wrote:
> >> On 2/1/17 12:53 AM, Michael Paquier wrote:
> >>> On Thu, Jan 26, 2017 at 6:49 AM, Tom Lane  wrote:
>  Nikita Glukhov  writes:
> > On 25.01.2017 23:58, Tom Lane wrote:
> >> I think you need to take a second look at the code you're producing
> >> and realize that it's not so clean either.  This extract from
> >> populate_record_field, for example, is pretty horrid:
> 
> > But what if we introduce some helper macros like this:
> 
> > #define JsValueIsNull(jsv) \
> >  ((jsv)->is_json ? !(jsv)->val.json.str \
> >  : !(jsv)->val.jsonb || (jsv)->val.jsonb->type == jbvNull)
> 
> > #define JsValueIsString(jsv) \
> >  ((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \
> >  : (jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString)
> 
>  Yeah, I was wondering about that too.  I'm not sure that you can make
>  a reasonable set of helper macros that will fix this, but if you want
>  to try, go for it.
> 
>  BTW, just as a stylistic thing, I find "a?b:c||d" unreadable: I have
>  to go back to the manual every darn time to convince myself whether
>  that means (a?b:c)||d or a?b:(c||d).  It's better not to rely on
>  the reader (... or the author) having memorized C's precedence rules
>  in quite that much detail.  Extra parens help.
> >>>
> >>> Moved to CF 2017-03 as discussion is going on and more review is
> >>> needed on the last set of patches.
> >>>
> >>
> >> The current patches do not apply cleanly at cccbdde:
> >>
> >> $ git apply ../other/0001-introduce-JsonContainerXxx-macros-v04.patch
> >> error: patch failed: src/backend/utils/adt/jsonb_util.c:328
> >> error: src/backend/utils/adt/jsonb_util.c: patch does not apply
> >> error: patch failed: src/backend/utils/adt/jsonfuncs.c:1266
> >> error: src/backend/utils/adt/jsonfuncs.c: patch does not apply
> >> error: patch failed: src/include/utils/jsonb.h:218
> >> error: src/include/utils/jsonb.h: patch does not apply
> >>
> >> In addition, it appears a number of suggestions have been made by Tom
> >> that warrant new patches.  Marked "Waiting on Author".
> >
> > This thread has been idle for months since Tom's review.
> >
> > The submission has been marked "Returned with Feedback".  Please feel
> > free to resubmit to a future commitfest.
> >
> >
>
> Please revive. I am planning to look at this later this week.

Since that was 10 days ago - you're still planning to get this in before
the freeze?

- Andres


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

Re: [HACKERS] Can't compile with profiling after BRIN autosummarization

[Alvaro Herrera]

Jeff Janes wrote:

> A newer gcc gave a better error message which lead me to add the line:
> 
> #include "storage/block.h" /* for typedef BlockNumber */
> 
> into autovacuum.h, which fixes the problem.

Ah, Peter already fixed it.

-- 
Álvaro Herrerahttps://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] Can't compile with profiling after BRIN autosummarization

[Alvaro Herrera]

Jeff Janes wrote:

> This git bisects down to:
> 
> commit 7526e10224f0792201e99631567bbe44492bbde4
> Author: Alvaro Herrera 
> Date:   Sat Apr 1 14:00:53 2017 -0300
> 
> BRIN auto-summarization
> 
> The lines are:
> 
> extern void AutoVacuumRequestWork(AutoVacuumWorkItemType type,
>   Oid relationId, BlockNumber blkno);

Oh, of course: the BlockNumber typedef is not #included by autovacuum.h.
Mea culpa -- will fix.

-- 
Álvaro Herrerahttps://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] WIP: [[Parallel] Shared] Hash

[Andres Freund]

Hi,

On 2017-03-31 17:53:12 +1300, Thomas Munro wrote:
> Thanks very much to Rafia for testing, and to Andres for his copious
> review feedback.  Here's a new version.  Changes:

I unfortunately think it's too late to get this into v10.  There's still
heavy development going on, several pieces changed quite noticeably
since the start of the CF and there's still features missing.  Hence I
think this unfortunately has to be pushed - as much as I'd have liked to
have this in 10.

Do you agree?

Regards,

Andres


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

Re: [HACKERS] parallel explain analyze support not exercised

[Robert Haas]

On Mon, Apr 3, 2017 at 3:31 PM, Andres Freund  wrote:
>> If this is 'make check', then we should have 8 parallel workers
>> allowed, so if we only do one of these at a time, then I think we're
>> OK.  But if somebody changes that configuration setting or if it's
>> 'make installcheck', then the configuration could be anything.
>
> Hm - we already rely on max_parallel_workers_per_gather being set with
> some of the explains in the test.  So I guess we're ok also relying on
> actual workers being present?

I'm not really sure about that one way or the other.  Our policy on
which configurations are supported vis-a-vis 'make installcheck' seems
to be, essentially, that if a sufficiently-prominent community member
cares about it, then it ends up getting made to work, unless an
even-more-prominent community member objects.  That's why, for
example, our regression tests pass in Czech.  I can't begin to guess
whether breaking installcheck against configurations with low values
of max_parallel_workers or max_worker_processes will bother anybody.

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


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

Re: [HACKERS] [PATCH] Incremental sort

[Andres Freund]

Hi,

On 2017-04-04 00:04:09 +0300, Alexander Korotkov wrote:
> > >Thank you!
> > >I already sent version of patch after David's reminder.
> > >Please find rebased patch in the attachment.
> >
> > Cool. I think that's still a bit late for v10?
> >
> 
> I don't know.  ISTM, that I addressed all the issues raised by reviewers.
> Also, this patch is pending since late 2013.  It would be very nice to
> finally get it in...

To me this hasn't gotten even remotely enough performance evaluation.
And I don't think it's fair to characterize it as pending since 2013,
given it was essentially "waiting on author" for most of that.

Greetings,

Andres Freund


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

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

[Robert Haas]

On Thu, Mar 30, 2017 at 1:14 AM, Ashutosh Bapat
 wrote:
> Done.

Ashutosh and I spent several hours discussing this patch set today.
I'm starting to become concerned about the fact that 0004 makes the
partition bounds part of the PartitionScheme, because that means you
can't do a partition-wise join between two tables that have any
difference at all in the partition bounds.  It might be possible in
the future to introduce a notion of a compatible partition scheme, so
that you could say, OK, well, these two partition schemes are not
quite the same, but they are compatible, and we'll make a new
partition scheme for whatever results from reconciling them.

What I think *may* be better is to consider the partition bound
information as a property of the RelOptInfo rather than the
PartitionScheme.  For example, suppose we're joining A with partitions
A1, A2, and A4 against B with partitions B1, B2, and B3 and C with
partitions C1, C2, and C5.  With the current approach, we end up with
a PartitionScheme for each baserel and, not in this patch set but
maybe eventually, a separate PartitionScheme for each of (A B), (A C),
(B C), and (A B C).  That seems pretty unsatisfying.  If we consider
the PartitionScheme to only include the question of whether we're
doing a join on the partition keys, then if the join includes WHERE
a.key = b.key AND b.key = c.key, we can say that they all have the
same PartitionScheme up front.  Then, each RelOptInfo can have a
separate list of bounds, like this:

A: 1, 2, 4
B: 1, 2, 3
C: 1, 2, 5
A B: 1, 2, 3, 4
A C: 1, 2, 4, 5
B C: 1, 2, 3, 5
A B C: 1, 2, 3, 4, 5

Or if it's an inner join, then instead of taking the union at each
level, we can take the intersection, because any partition without a
match on the other side of the join, then that partition can't produce
any rows and doesn't need to be scanned.  In that case, the
RelOptInfos for (A B), (A C), (B, C), and (A, B, C) will all end up
with a bound list of 1, 2.

A related question (that I did not discuss with Ashutosh, but occurred
to me later) is whether the PartitionScheme ought to worry about
cross-type joins.  For instance, if A is partitioned on an int4 column
and B is partitioned on an int8 column, and they are joined on their
respective partitioning columns, can't we still do a partition-wise
join?  We do need to require that the operator family of the operator
actually used in the query, the operator family used to partition the
inner table, and the operator family used to partition the other table
all match; and the collation used for the comparison in the query, the
collation used to partition the outer table, and the collation used to
partition the inner table must all match.  But it doesn't seem
necessary to require an exact type or typmod match.  In many ways this
seems a whole lot like the what we test when building equivalence
classes (cf. process_equivalence) although I'm not sure that we can
leverage that in any useful way.

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


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

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

[Andres Freund]

On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > The CF has been extended until April 7 but time is still growing short.
> > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or this
> > submission will be marked "Returned with Feedback".
> >
> >
> Thanks for the information, attached the latest patch resolving one
> compilation warning. And, please discard the test patch as it will be
> re-implemented later separately.

Hm.  If the tests aren't ready yet, it seems we'll have to move this to
the next CF.


> + 
> +  Batch mode and query pipelining
> +
> +  
> +   libpq
> +   batch mode
> +  
> +
> +  
> +   libpq
> +   pipelining
> +  
> +
> +  
> +   libpq supports queueing up multiple queries 
> into
> +   a pipeline to be executed as a batch on the server. Batching queries 
> allows
> +   applications to avoid a client/server round-trip after each query to get
> +   the results before issuing the next query.
> +  

"queueing .. into a pipeline" sounds weird to me - but I'm not a native
speaker.  Also batching != pipelining.


> +  
> +   When to use batching
> +
> +   
> +Much like asynchronous query mode, there is no performance disadvantage 
> to
> +using batching and pipelining. It increases client application complexity
> +and extra caution is required to prevent client/server deadlocks but
> +offers considerable performance improvements.
> +   

s/offers/can sometimes offer/


> +  
> +   Using batch mode
> +
> +   
> +To issue batches the application must switch
> +libpq into batch mode.

s/libpq/a connection/?


> Enter batch mode with  +
> linkend="libpq-PQbatchBegin">PQbatchBegin(conn) 
> or test
> +whether batch mode is active with  +
> linkend="libpq-PQbatchStatus">PQbatchStatus(conn).
>  In batch mode only  +linkend="libpq-async">asynchronous operations are permitted, and
> +COPY is not recommended as it most likely will 
> trigger failure in batch processing. 
> +(The restriction on COPY is an implementation
> +limit; the PostgreSQL protocol and server can support batched
> COPY).

Hm, I'm unconvinced that that's a useful parenthetical in the libpq
docs.


> +   
> +The client uses libpq's asynchronous query functions to dispatch work,
> +marking the end of each batch with PQbatchQueueSync.
> +Concurrently, it uses PQgetResult and
> +PQbatchQueueProcess to get results.

"Concurrently" imo is a dangerous word, somewhat implying multi-threading.


> +   
> +
> + It is best to use batch mode with libpq in
> + non-blocking mode. If 
> used in
> + blocking mode it is possible for a client/server deadlock to occur. The
> + client will block trying to send queries to the server, but the server 
> will
> + block trying to send results from queries it has already processed to 
> the
> + client. This only occurs when the client sends enough queries to fill 
> its
> + output buffer and the server's receive buffer before switching to
> + processing input from the server, but it's hard to predict exactly when
> + that'll happen so it's best to always use non-blocking mode.
> +
> +   

Such deadlocks are possible just as well with non-blocking mode, unless
one can avoid sending queries and switching to receiving results anytime
in the application code.


> +
> + Batched operations will be executed by the server in the order the 
> client
> + sends them. The server will send the results in the order the statements
> + executed. The server may begin executing the batch before all commands
> + in the batch are queued and the end of batch command is sent. If any
> + statement encounters an error the server aborts the current transaction 
> and
> + skips processing the rest of the batch. Query processing resumes after 
> the
> + end of the failed batch.
> +

What if a batch contains transaction boundaries?


> +   
> +Processing results
> +
> +
> + The client interleaves result
> + processing with sending batch queries, or for small batches may
> + process all results after sending the whole batch.
> +

That's a very long  text, is it not?


> +
> + To get the result of the first batch entry the client must call  + 
> linkend="libpq-PQbatchQueueProcess">PQbatchQueueProcess.
>  It must then call

What does 'QueueProcess' mean? Shouldn't it be 'ProcessQueue'? You're
not enquing a process or processing, right?


> + PQgetResult and handle the results until
> + PQgetResult returns null (or would return null if
> + called).

What is that parenthetical referring to?  IIRC we don't provide any
external way to determine PQgetResult would return NULL.


Have you checked how this API works with PQsetSingleRowMode()?

> +
> + To enter single-row mode, call PQsetSingleRowMode 
> immediately after a
> + successful call of PQbatchQueueProcess. This mode 
> selection is effective 
> + 

Re: [HACKERS] [PATCH] Incremental sort

[Alexander Korotkov]

On Mon, Apr 3, 2017 at 10:05 PM, Andres Freund  wrote:

> On April 3, 2017 12:03:56 PM PDT, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
> >On Mon, Apr 3, 2017 at 9:34 PM, Andres Freund 
> >wrote:
> >
> >> On 2017-03-29 00:17:02 +0300, Alexander Korotkov wrote:
> >> > On Tue, Mar 28, 2017 at 5:27 PM, David Steele 
> >> wrote:
> >> > > On 3/20/17 10:19 AM, Heikki Linnakangas wrote:
> >> > >
> >> > >> On 03/20/2017 11:33 AM, Alexander Korotkov wrote:
> >> > >>
> >> > >>> Please, find rebased patch in the attachment.
> >> > >>>
> >> > >>
> >> > >> I had a quick look at this.
> >> > >>
> >> > >
> >> > > <...>
> >> > >
> >> > > According to 'perf', 85% of the CPU time is spent in
> >ExecCopySlot(). To
> >> > >> alleviate that, it might be worthwhile to add a special case for
> >when
> >> > >> the group contains exactly one group, and not put the tuple to
> >the
> >> > >> tuplesort in that case. Or if we cannot ensure that the
> >Incremental
> >> Sort
> >> > >> is actually faster, the cost model should probably be smarter,
> >to
> >> avoid
> >> > >> picking an incremental sort when it's not a win.
> >> > >>
> >> > >
> >> > > This thread has been idle for over a week.  Please respond with a
> >new
> >> > > patch by 2017-03-30 00:00 AoE (UTC-12) or this submission will be
> >> marked
> >> > > "Returned with Feedback".
> >>
> >> > Thank you for reminder!
> >>
> >> I've just done so.  Please resubmit once updated, it's a cool
> >feature.
> >>
> >
> >Thank you!
> >I already sent version of patch after David's reminder.
> >Please find rebased patch in the attachment.
>
> Cool. I think that's still a bit late for v10?
>

I don't know.  ISTM, that I addressed all the issues raised by reviewers.
Also, this patch is pending since late 2013.  It would be very nice to
finally get it in...

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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Corey Huinker]

>
> I assume we want to keep that pre-existing behavior of \set in
> psql, that is, making a copy of that string and associating a
> name to it, whereas I guess pgbench computes a value from it and
> stores that value.
>

Maybe a \set-variant called \expr?

Re: [HACKERS] SERIALIZABLE with parallel query

[Kevin Grittner]

On Fri, Mar 10, 2017 at 8:19 PM, Thomas Munro
 wrote:
> On Wed, Feb 22, 2017 at 2:01 PM, Robert Haas  wrote:
>> I don't think I know enough about the serializable code to review
>> this, or at least not quickly, but it seems very cool if it works.
>> Have you checked what effect it has on shared memory consumption?
>
> I'm not sure how to test that.  Kevin, can you provide any pointers to
> the test workloads you guys used when developing SSI?

During development there was first and foremost the set of tests
which wound up implemented in the isolation testing environment
developed by Heikki, although for most of the development cycle
these tests were run by a python tool written by Markus Wanner
(which was not as fast as Heikki's C-based tool, but provided a lot
more detail in case of failure -- so it was very nice to have until
we got down to polishing things).

The final stress test to chase out race conditions and the
performance benchmarks were all run by Dan Ports on big test
machines at MIT.  I'm not sure how much I can say to elaborate on
what is in section 8 of this paper:

http://vldb.org/pvldb/vol5/p1850_danrkports_vldb2012.pdf

I seem to remember that he had some saturation run versions of the
"DBT-2++" tests, modified to monitor for serialization anomalies
missed by the implementation, which he sometimes ran for days at a
time on MIT testing machines.  There were some very narrow race
conditions uncovered by this testing, which at high concurrency on a
16 core machine might hit a particular problem less often than once
per day.

I also remember that both Anssi Kääriäinen and Yamamoto Takahashi
did a lot of valuable testing of the patch and found problems that
we had missed.  Perhaps they can describe their testing or make
other suggestions.

--
Kevin Grittner


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

[Andres Freund]

On 2017-04-03 16:31:27 -0400, Peter Eisentraut wrote:
> > Is it guaranteed that the caller expects an int64?  I saw that
> > nextvalueexpr's have a typeid field.
> 
> It expects one of the integer types.  We could cast the result of
> Int64GetDatum() to the appropriate type, but that wouldn't actually do
> anything.

Uh.  What about 32bit systems (and 64bit systems without
float8passbyval)?

Greetings,

Andres Freund


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

Re: [HACKERS] Unable to build doc on latest head

[Andrew Gierth]

> "Peter" == Peter Eisentraut  writes:

 > On 4/3/17 02:44, Ashutosh Bapat wrote:
 >> [1] says that id.attribute is supported in stylesheets version
 >> 1.77.1.  Do I need to update stylesheets version? How do I do it?
 >> Any help will be appreciated.

 Peter> The oldest version among the ones listed at
 Peter> http://docbook.sourceforge.net/release/xsl/ that works for me is
 Peter> 1.77.0.  Which one do you have?

BTW, the version in freebsd ports is 1.76.1, and the download stats on
docbook.sourceforge.net suggests that this one is still pretty widely
used despite its age. I filed a PR on the port, but who knows when
anything will happen.

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] Making clausesel.c Smarter

[Claudio Freire]

On Mon, Apr 3, 2017 at 5:24 PM, Andres Freund  wrote:
> On 2017-04-03 20:59:42 +1200, David Rowley wrote:
>> Updated patch attached.
>>
>> Thanks for reviewing it.
>
> Given the time in the release cycle I'm afraid that this it's too late
> to get this into v10.  Does anybody disagree?  If not, it should be
> moved to the next CF.
>
> Greetings,
>
> Andres Freund

While personally I'd like to see this patch make it (I got bitten by
this more than once), I can't argue with that logic with the feature
freeze almost upon us, especially given the stage the review is at
(ie: just beginning).

I'm still going to review it though.


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

[Peter Eisentraut]

On 4/3/17 14:19, Andres Freund wrote:
> Are you going to try to merge this soon, or are you pushing this to 11?

I plan to commit it this week.  It was basically ready weeks ago, but
had to be adjusted after the executor changes.

>> +case T_NextValueExpr:
>> +{
>> +NextValueExpr *nve = (NextValueExpr *) node;
>> +
>> +scratch.opcode = EEOP_NEXTVALUEEXPR;
>> +scratch.d.nextvalueexpr.seqid = nve->seqid;
>> +
>> +ExprEvalPushStep(state, );
>> +break;
>> +}
> 
> Hm - that's probably been answered somewhere, but why do we need a
> special expression type for this?

Because that's the way to evade the separate permission check that
nextval the function would otherwise do.

>> diff --git a/src/backend/executor/execExprInterp.c 
>> b/src/backend/executor/execExprInterp.c
>> index 4fbb5c1e74..5935b9ef75 100644
>> --- a/src/backend/executor/execExprInterp.c
>> +++ b/src/backend/executor/execExprInterp.c
>> @@ -60,6 +60,7 @@
>>  
>>  #include "access/tuptoaster.h"
>>  #include "catalog/pg_type.h"
>> +#include "commands/sequence.h"
>>  #include "executor/execExpr.h"
>>  #include "executor/nodeSubplan.h"
>>  #include "funcapi.h"
>> @@ -337,6 +338,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, 
>> bool *isnull)
>>  &_EEOP_NULLIF,
>>  &_EEOP_SQLVALUEFUNCTION,
>>  &_EEOP_CURRENTOFEXPR,
>> +&_EEOP_NEXTVALUEEXPR,
>>  &_EEOP_ARRAYEXPR,
>>  &_EEOP_ARRAYCOERCE,
>>  &_EEOP_ROW,
>> @@ -1228,6 +1230,14 @@ ExecInterpExpr(ExprState *state, ExprContext 
>> *econtext, bool *isnull)
>>  EEO_NEXT();
>>  }
>>  
>> +EEO_CASE(EEOP_NEXTVALUEEXPR)
>> +{
>> +*op->resvalue = 
>> Int64GetDatum(nextval_internal(op->d.nextvalueexpr.seqid, false));
>> +*op->resnull = false;
>> +
>> +EEO_NEXT();
>> +}
> 
> Is it guaranteed that the caller expects an int64?  I saw that
> nextvalueexpr's have a typeid field.

It expects one of the integer types.  We could cast the result of
Int64GetDatum() to the appropriate type, but that wouldn't actually do
anything.

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


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

Re: [HACKERS] identity columns

[Peter Eisentraut]

On 3/30/17 22:57, Vitaly Burovoy wrote:
> Why do you still want to leave "ADD IF NOT EXISTS" instead of using
> "SET IF NOT EXISTS"?
> If someone wants to follow the standard he can simply not to use "IF
> NOT EXISTS" at all. Without it error should be raised.

As I tried to mention earlier, it is very difficult to implement the IF
NOT EXISTS behavior here, because we need to run the commands the create
the sequence before we know whether we will need it.  So this is a bit
different from many other ALTER TABLE commands.  It could be done, but
it would require some major reworking of things or a new idea of some
kind.  It could be done later, but I think it's not worth holding things
up for that.

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


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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Daniel Verite]

Fabien COELHO wrote:

> Now it could be decided that \set in psql stays simplistic because it is 
> not needed as much as it is with pgbench. Fine with me.

It's not just that. It's that currently, if we do in psql:

\set d sqrt(1 + random(1000) * 17)

then we get that:

\echo :d
sqrt(1+random(1000)*17)

I assume we want to keep that pre-existing behavior of \set in
psql, that is, making a copy of that string and associating a
name to it, whereas I guess pgbench computes a value from it and
stores that value.

Certainly if we want the same sort of evaluator in pgbench and psql
we'd better share the code between them, but I don't think it will be
exposed by the same backslash commands in both programs,
if only for that backward compatibility concern.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] SERIALIZABLE with parallel query

[Thomas Munro]

On Tue, Apr 4, 2017 at 6:41 AM, Andres Freund  wrote:
> Hi,
>
> On 2017-03-11 15:19:23 +1300, Thomas Munro wrote:
>> Here is a rebased patch.
>
> It seems that this patch is still undergoing development, review and
> performance evaluation.  Therefore it seems like it'd be a bad idea to
> try to get this into v10.  Any arguments against moving this to the next
> CF?

None, and done.

It would be good to get some feedback from Kevin on whether this is a
reasonable approach, but considering that these data structures may
finish up being redesigned as part of the GSoC project[1], it may be
best to wait and see where that goes before doing anything.  I'll
follow developments there, and if this patch remains relevant I'll
plan to do some more work on it including testing (possibly with the
RUBiS benchmark from Kevin and Dan's paper since it seems the most
likely to be able to really use parallelism) for PG11 CF1.

[1] 
https://wiki.postgresql.org/wiki/GSoC_2017#Eliminate_O.28N.5E2.29_scaling_from_rw-conflict_tracking_in_serializable_transactions

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


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

Re: [HACKERS] Making clausesel.c Smarter

[Andres Freund]

On 2017-04-03 20:59:42 +1200, David Rowley wrote:
> Updated patch attached.
> 
> Thanks for reviewing it.

Given the time in the release cycle I'm afraid that this it's too late
to get this into v10.  Does anybody disagree?  If not, it should be
moved to the next CF.

Greetings,

Andres Freund


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

Re: [HACKERS] Parallel Append implementation

[Andres Freund]

Hi,


On 2017-03-24 21:32:57 +0530, Amit Khandekar wrote:
> diff --git a/src/backend/executor/nodeAppend.c 
> b/src/backend/executor/nodeAppend.c
> index a107545..e9e8676 100644
> --- a/src/backend/executor/nodeAppend.c
> +++ b/src/backend/executor/nodeAppend.c
> @@ -59,9 +59,47 @@
>  
>  #include "executor/execdebug.h"
>  #include "executor/nodeAppend.h"
> +#include "miscadmin.h"
> +#include "optimizer/cost.h"
> +#include "storage/spin.h"
> +
> +/*
> + * Shared state for Parallel Append.
> + *
> + * Each backend participating in a Parallel Append has its own
> + * descriptor in backend-private memory, and those objects all contain
> + * a pointer to this structure.
> + */
> +typedef struct ParallelAppendDescData
> +{
> + LWLock  pa_lock;/* mutual exclusion to choose 
> next subplan */
> + int pa_first_plan;  /* plan to choose while 
> wrapping around plans */
> + int pa_next_plan;   /* next plan to choose by any 
> worker */
> +
> + /*
> +  * pa_finished : workers currently executing the subplan. A worker which
> +  * finishes a subplan should set pa_finished to true, so that no new
> +  * worker picks this subplan. For non-partial subplan, a worker which 
> picks
> +  * up that subplan should immediately set to true, so as to make sure
> +  * there are no more than 1 worker assigned to this subplan.
> +  */
> + boolpa_finished[FLEXIBLE_ARRAY_MEMBER];
> +} ParallelAppendDescData;


> +typedef ParallelAppendDescData *ParallelAppendDesc;

Pointer hiding typedefs make this Andres sad.



> @@ -291,6 +362,276 @@ ExecReScanAppend(AppendState *node)
>   if (subnode->chgParam == NULL)
>   ExecReScan(subnode);
>   }
> +
> + if (padesc)
> + {
> + padesc->pa_first_plan = padesc->pa_next_plan = 0;
> + memset(padesc->pa_finished, 0, sizeof(bool) * node->as_nplans);
> + }
> +

Is it actually guaranteed that none of the parallel workers are doing
something at that point?


> +/* 
> + *   exec_append_parallel_next
> + *
> + *   Determine the next subplan that should be executed. Each worker 
> uses a
> + *   shared next_subplan counter index to start looking for 
> unfinished plan,
> + *   executes the subplan, then shifts ahead this counter to the next
> + *   subplan, so that other workers know which next plan to choose. 
> This
> + *   way, workers choose the subplans in round robin order, and thus 
> they
> + *   get evenly distributed among the subplans.
> + *
> + *   Returns false if and only if all subplans are already finished
> + *   processing.
> + * 
> + */
> +static bool
> +exec_append_parallel_next(AppendState *state)
> +{
> + ParallelAppendDesc padesc = state->as_padesc;
> + int whichplan;
> + int initial_plan;
> + int first_partial_plan = ((Append 
> *)state->ps.plan)->first_partial_plan;
> + boolfound;
> +
> + Assert(padesc != NULL);
> +
> + /* Backward scan is not supported by parallel-aware plans */
> + Assert(ScanDirectionIsForward(state->ps.state->es_direction));
> +
> + /* The parallel leader chooses its next subplan differently */
> + if (!IsParallelWorker())
> + return exec_append_leader_next(state);

It's a bit weird that the leader's case does is so separate, and does
it's own lock acquisition.


> + found = false;
> + for (whichplan = initial_plan; whichplan != PA_INVALID_PLAN;)
> + {
> + /*
> +  * Ignore plans that are already done processing. These also 
> include
> +  * non-partial subplans which have already been taken by a 
> worker.
> +  */
> + if (!padesc->pa_finished[whichplan])
> + {
> + found = true;
> + break;
> + }
> +
> + /*
> +  * Note: There is a chance that just after the child plan node 
> is
> +  * chosen above, some other worker finishes this node and sets
> +  * pa_finished to true. In that case, this worker will go ahead 
> and
> +  * call ExecProcNode(child_node), which will return NULL tuple 
> since it
> +  * is already finished, and then once again this worker will 
> try to
> +  * choose next subplan; but this is ok : it's just an extra
> +  * "choose_next_subplan" operation.
> +  */

IIRC not all node types are safe against being executed again when
they've previously returned NULL.  That's why e.g. nodeMaterial.c
contains the following blurb:
/*
 * If necessary, try to fetch another row from the subplan.
 *
 * Note: the 

Re: [HACKERS] [PATCH] few fts functions for jsonb

[Andrew Dunstan]

On 04/03/2017 03:41 PM, Sven R. Kunze wrote:
> On 03.04.2017 21:30, Andrew Dunstan wrote:
>> On 04/03/2017 02:44 PM, Sven R. Kunze wrote:
>>> On 01.04.2017 22:20, Andrew Dunstan wrote:
 I added documentation when I committed it for the new functions, in
 the
 FTS section. I'm not sure what we need to add to the JSON section if
 anything.
>>> Not sure, if this is related but the formatting of
>>> https://www.postgresql.org/docs/devel/static/functions-textsearch.html
>>> looks a bit strange.
>>>
>>> Just 2 questions/notes:
>>> 1) in what order are the values of the JSON extracted?
>> In the order they exist in the underlying document.
>
> Just asking as the order can have implications for fulltext searches.
> So, might be valuable for the docs.
>
>
> Are these documents equally ordered in this sense?
>
> srkunze=# select '{"a": "abc", "b": "def"}'::jsonb;
>   jsonb
> --
>  {"a": "abc", "b": "def"}
> (1 row)
>
> srkunze=# select '{"b": "def", "a": "abc"}'::jsonb;
>   jsonb
> --
>  {"a": "abc", "b": "def"}
> (1 row)
>


Yes, when converted to jsonb these two documents are identical.


>
> Also what about non-ascii keys? Are they ordered by the default locale
> of the PostgreSQL cluster (say de_DE.utf-8)?


Yes, I believe so.

cheers

andrew

-- 
Andrew Dunstanhttps://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] few fts functions for jsonb

[Sven R. Kunze]

On 03.04.2017 21:30, Andrew Dunstan wrote:

On 04/03/2017 02:44 PM, Sven R. Kunze wrote:

On 01.04.2017 22:20, Andrew Dunstan wrote:

I added documentation when I committed it for the new functions, in the
FTS section. I'm not sure what we need to add to the JSON section if
anything.

Not sure, if this is related but the formatting of
https://www.postgresql.org/docs/devel/static/functions-textsearch.html
looks a bit strange.

Just 2 questions/notes:
1) in what order are the values of the JSON extracted?

In the order they exist in the underlying document.


Just asking as the order can have implications for fulltext searches. 
So, might be valuable for the docs.



Are these documents equally ordered in this sense?

srkunze=# select '{"a": "abc", "b": "def"}'::jsonb;
  jsonb
--
 {"a": "abc", "b": "def"}
(1 row)

srkunze=# select '{"b": "def", "a": "abc"}'::jsonb;
  jsonb
--
 {"a": "abc", "b": "def"}
(1 row)


Also what about non-ascii keys? Are they ordered by the default locale 
of the PostgreSQL cluster (say de_DE.utf-8)?



2) Regarding the additional line:
to_tsvector([ config regconfig , ] document json(b))tsvector
reduce document text to tsvectorto_tsvector('english', '{"a": "The
Fat Rats"}'::json)'fat':2 'rat':3

Maybe change "reduce document text to tsvector" to "extracting JSON
values  and reduce to tsvector"?




OK, I will do something along those lines.

cheers

andrew





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

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

[Daniel Verite]

  Hi,

In interactive mode, the warning in untaken branches is misleading
when \endif is on the same line as the commands that
are skipped. For instance:

  postgres=# \if false \echo NOK \endif
  \echo command ignored; use \endif or Ctrl-C to exit current \if block
  postgres=# 

From the point of view of the user, the execution flow has exited
the branch already when this warning is displayed.
Of course issuing the recommended \endif at this point doesn't work:

  postgres=# \endif
  \endif: no matching \if

Maybe that part of the message: 
"use \endif or Ctrl-C to exit current \if block"
should be displayed only when coming back at the prompt,
and if the flow is still in an untaken branch at this point?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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 explain analyze support not exercised

[Andres Freund]

On 2017-04-03 15:13:13 -0400, Robert Haas wrote:
> On Mon, Apr 3, 2017 at 1:53 PM, Andres Freund  wrote:
> >> Please find the attached for the same.
> >
> >> +-- to increase the parallel query test coverage
> >> +EXPLAIN (analyze, timing off, summary off, costs off) SELECT * FROM tenk1;
> >> + QUERY PLAN
> >> +-
> >> + Gather (actual rows=1 loops=1)
> >> +   Workers Planned: 4
> >> +   Workers Launched: 4
> >> +   ->  Parallel Seq Scan on tenk1 (actual rows=2000 loops=5)
> >> +(4 rows)
> >
> > Is there an issue that we might not actually be able to start all four
> > workers?  Serious question, not rhetorical.
> 
> If this is 'make check', then we should have 8 parallel workers
> allowed, so if we only do one of these at a time, then I think we're
> OK.  But if somebody changes that configuration setting or if it's
> 'make installcheck', then the configuration could be anything.

Hm - we already rely on max_parallel_workers_per_gather being set with
some of the explains in the test.  So I guess we're ok also relying on
actual workers being present?

- Andres


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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Fabien COELHO]

[...] but OTOH "\if sql 1 from table where expr" looks awkward. Given an 
implicit select, I would prefer "\if exists (select 1 from table where 
expr)" but now it's not shorter.


Possibly, but it is just an SQL expression, which looks good in the middle 
of an sql script.



An advantage of prepending the SELECT automatically, is that it
would prevent people from abusing this syntax by putting
update/insert/delete or even DDL in there, imagining that this would
be a success/failure test for these operations.



Having these fail to execute in the first place, when called by \if,
seems like a sane failure mode that we would gain incidentally.


Yes, it should be avoided.

--
Fabien.


--
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] few fts functions for jsonb

[Andrew Dunstan]

On 04/03/2017 02:44 PM, Sven R. Kunze wrote:
> On 01.04.2017 22:20, Andrew Dunstan wrote:
>> I added documentation when I committed it for the new functions, in the
>> FTS section. I'm not sure what we need to add to the JSON section if
>> anything.
>
> Not sure, if this is related but the formatting of
> https://www.postgresql.org/docs/devel/static/functions-textsearch.html
> looks a bit strange.
>
> Just 2 questions/notes:
> 1) in what order are the values of the JSON extracted?

In the order they exist in the underlying document.

>
> 2) Regarding the additional line:
> to_tsvector([ config regconfig , ] document json(b))tsvector
> reduce document text to tsvectorto_tsvector('english', '{"a": "The
> Fat Rats"}'::json)'fat':2 'rat':3
>
> Maybe change "reduce document text to tsvector" to "extracting JSON
> values  and reduce to tsvector"?
>
>


OK, I will do something along those lines.

cheers

andrew

-- 
Andrew Dunstanhttps://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] few fts functions for jsonb

[Andrew Dunstan]

On 04/03/2017 02:22 PM, Andres Freund wrote:
> On 2017-04-01 16:20:46 -0400, Andrew Dunstan wrote:
>>
>> On 03/31/2017 03:17 PM, Oleg Bartunov wrote:
>>>
>>> On 30 Mar 2017 23:43, "Dmitry Dolgov" <9erthali...@gmail.com
>>> > wrote:
>>>
>>> On 31 March 2017 at 00:01, Andrew Dunstan
>>> >> > wrote:
>>> >
>>> > I have just noticed as I was writing/testing the non-existent
>>> docs for
>>> > this patch that it doesn't supply variants of to_tsvector that
>>> take a
>>> > regconfig as the first argument. Is there a reason for that? Why
>>> > should the json(b) versions be different from the text versions?
>>>
>>> No, there is no reason, I just missed that. Here is a new version
>>> of the patch (only the functions part)
>>> to add those variants.
>>>
>>>
>>> Congratulations with patch committed, who will write an addition
>>> documentation? I think we need to touch  FTS and JSON parts.
>> I added documentation when I committed it for the new functions, in the
>> FTS section. I'm not sure what we need to add to the JSON section if
>> anything.
> I see that the CF entry for this hasn't been marked as committed:
> https://commitfest.postgresql.org/13/1054/
> Is there anything left here?
>


Says "Status committed" for me. I fixed this in Sunday after Tom prodded me.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Variable substitution in psql backtick expansion

[Fabien COELHO]

Hello Tom,


  \if [select current_setting('server_version_num')::int < 11]


I really dislike this syntax proposal.


It would get us into having to count nested brackets, and distinguish 
quoted from unquoted brackets, and so on ... and for what?  It's not 
really better than


  \if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'


Hmmm. On the positive side, it looks more expression-like, and it allows 
to think of handling a multi-line expression on day.


ISTM that the lexer already counts parentheses, so maybe using external 
parentheses might work without much effort?



(ie, "\if sql ...text to send to server...").

If you're worried about shaving keystrokes, make the "select" be implicit.
That would be particularly convenient for the common case where you're
just trying to evaluate a boolean expression with no table reference.


I'm looking for a solution to avoid the suggested "sql" prefix, because "I 
really dislike this proposal", as you put it.


--
Fabien.


--
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] Variable substitution in psql backtick expansion

[Fabien COELHO]

Hello Daniel,


  - how would it work, both with \set ... and \if ...?


The idea is that the need to have two command (a SELECT .. \gset
followed by an \if) and a temporary variable in-between would be
lifted by implementing a close equivalent in one command.
It would behave essentially the same as the two commands.

I don't see that \set must have to be involved in that improvement,
although it could be indirectly, depending on what exactly we
implement.


My point is that there was some idea expressed by Tom or Robert (?) at 
some point that pgbench & psql should implement the same backslash 
commands when appropriate.


Currently pgbench allows client-side expressions in \set, eg

  \set d sqrt(1 + random(1000) * 17)

There is a patch pending to allow pgbench's \set a more developed 
syntactic subset of pg SQL, including booleans, logical operator and such. 
There is another pending patch which implements \gset and variant in 
pgbench. If these patches get it, I would probably also implement \if 
because it makes sense for benchmarking.


If psql's \if accepts expressions in psql, then it seems logical at some 
level that this syntax would be more or less compatible with pgbench 
expressions, somehow, and vice-versa. Hence my question. If I implement 
\if in pgbench, I will trivially reuse the \set expression parser 
developed by Robert, i.e. have "\if expression".


Now it could be decided that \set in psql stays simplistic because it is 
not needed as much as it is with pgbench. Fine with me.



\set differs in that it already exists in released versions,
so we have the backward compatibility to consider.


Sure.


With \if we are not bound by that, but what \if will be at feature
freeze needs to be as convenient as we can make it in this release,
and not block progress in v11 and later, as these future improvements
will have to be backward-compatible against v10.


Sure.


  - should it be just simple expressions or may it allow complex
queries?


Let's imagine that psql would support a syntax like this:
 \if [select current_setting('server_version_num')::int < 11]
or
 \if [select 1 from pg_catalog.pg_extension where extname='pgcrypto']

where by convention [ and ] enclose an SQL query that's assumed to
return a single-row, single-column bool-ish value, and in which
psql variables would be expanded, like they are now in
backtick expressions.


Hmmm. Why not. or maybe a parenthesis? At least it looks less terrible 
than a prefix thing like "\if sql".



Queries can be as complex as necessary, they just have to fit in one line.


Hmmm. I'm not sure that the one-line constraint is desirable.


  - how would error detection and handling work from a script?


The same as SELECT..\gset followed by \if, when the SELECT fails.


There is a problem: AFAICS currently there is no way to test whether
something failed. When there was no \if, there was not way to test 
anything, so no need to report issues. Now that there is a if, I think
that having some variable reporting would make sense, eg whether an error 
occured, how many rows were affected, things like that.



  - should it have some kind of continuation, as expressions are
likely to be longer than a constant?


No, to me that falls into the issue of continuation of backslash
commands in general, which is discussed separately.


Hmmm. If there is a begin/end syntactic marker, probably psql lexer could 
handle waiting for the end of the expression.



  - how would they interact with possible client-side expressions?


In no way at all,in the sense that, either you choose to use an SQL
evaluator, or a client-side evaluator (if it exists), or a backtick
expression.


My strong desire is to avoid an explicit client vs server side evaluator 
choice in the form of something like "\if sql ...". Maybe I could buy 
brackets or parentheses, though.



They are mutually exclusive for a single \if invocation.


Sure.


Client-side evaluation would have to be called with a syntax
that is unambiguously different. For example it could be
\if (:SQLSTATE = '23505')
 \echo A record with the unique key :key_id already exists
 rollback
\endif

AFAIK we don't have :SQLSTATE yet, but we should :)


Yes, that is one of my points, error (and success) reporting through 
variables become useful once you have a test available to do something 
about it.



Maybe the parentheses are not required, or we could require a different set
of brackets to enclose an expression to evaluate internally, like {}, or
whatever provided it's not ambiguous.


Hmmm. Yep, not ambiguous, and if possible transparent:-)

Another idea I'm toying with is that by default \if whatever... would
be an SQL server side expression, but for some client-side expressions 
which could be filtered out by regex. It would be enough to catch

define and simple comparisons:

  ((not)? defined \w+|\d+ (=|<|>|<=|<>|!=|...) \d+)

That could be interpreted client side easily enough.


(on this point, I 

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

[Robert Haas]

On Thu, Mar 30, 2017 at 6:32 AM, Amit Langote
 wrote:
> So, because we want to create an Append path for each partitioned table in
> a tree separately, we'll need RelOptInfo for each one, which in turn
> requires an AppendRelInfo.

Hmm.  It's no more desirable to have an Append inside of another
Append with partitionwise join than it is in general.  If we've got A
partitioned into A1, A2, A3 and similarly B partitioned into B1, B2,
and B3, and then A1 and B1 are further partitioned into A1a, A1b, B1a,
B1b, then a partitionwise join between the tables ought to end up
looking like this:

Append
-> Join (A1a, B1a)
-> Join (A1b, B1b)
-> Join (A2, B2)
-> Join (A3, B3)

So here we don't actually end up with an append-path for A1-B1 here
anywhere.  But you might need that in more complex cases, I guess,
because suppose you now join this to C with partitions C1, C2, C3; but
C1 is not sub-partitioned.  Then you might end up with a plan like:

Append
-> Join
  -> Append
-> Join (A1a, B1a)
-> Join (A1b, B1b)
  -> Scan C1
-> Join ((A2, B2), C2)
-> Join ((A3, B3), C3)

So maybe you're right.

-- 
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 explain analyze support not exercised

[Robert Haas]

On Mon, Apr 3, 2017 at 1:53 PM, Andres Freund  wrote:
>> Please find the attached for the same.
>
>> +-- to increase the parallel query test coverage
>> +EXPLAIN (analyze, timing off, summary off, costs off) SELECT * FROM tenk1;
>> + QUERY PLAN
>> +-
>> + Gather (actual rows=1 loops=1)
>> +   Workers Planned: 4
>> +   Workers Launched: 4
>> +   ->  Parallel Seq Scan on tenk1 (actual rows=2000 loops=5)
>> +(4 rows)
>
> Is there an issue that we might not actually be able to start all four
> workers?  Serious question, not rhetorical.

If this is 'make check', then we should have 8 parallel workers
allowed, so if we only do one of these at a time, then I think we're
OK.  But if somebody changes that configuration setting or if it's
'make installcheck', then the configuration could be anything.

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


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

Re: [HACKERS] [PATCH] Incremental sort

[Andres Freund]

On April 3, 2017 12:03:56 PM PDT, Alexander Korotkov 
 wrote:
>On Mon, Apr 3, 2017 at 9:34 PM, Andres Freund 
>wrote:
>
>> On 2017-03-29 00:17:02 +0300, Alexander Korotkov wrote:
>> > On Tue, Mar 28, 2017 at 5:27 PM, David Steele 
>> wrote:
>> > > On 3/20/17 10:19 AM, Heikki Linnakangas wrote:
>> > >
>> > >> On 03/20/2017 11:33 AM, Alexander Korotkov wrote:
>> > >>
>> > >>> Please, find rebased patch in the attachment.
>> > >>>
>> > >>
>> > >> I had a quick look at this.
>> > >>
>> > >
>> > > <...>
>> > >
>> > > According to 'perf', 85% of the CPU time is spent in
>ExecCopySlot(). To
>> > >> alleviate that, it might be worthwhile to add a special case for
>when
>> > >> the group contains exactly one group, and not put the tuple to
>the
>> > >> tuplesort in that case. Or if we cannot ensure that the
>Incremental
>> Sort
>> > >> is actually faster, the cost model should probably be smarter,
>to
>> avoid
>> > >> picking an incremental sort when it's not a win.
>> > >>
>> > >
>> > > This thread has been idle for over a week.  Please respond with a
>new
>> > > patch by 2017-03-30 00:00 AoE (UTC-12) or this submission will be
>> marked
>> > > "Returned with Feedback".
>>
>> > Thank you for reminder!
>>
>> I've just done so.  Please resubmit once updated, it's a cool
>feature.
>>
>
>Thank you!
>I already sent version of patch after David's reminder.
>Please find rebased patch in the attachment.

Cool. I think that's still a bit late for v10?

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


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

Re: [HACKERS] Should we cacheline align PGXACT?

[Andres Freund]

On 2017-03-25 19:35:35 +0300, Alexander Korotkov wrote:
> On Wed, Mar 22, 2017 at 12:23 AM, David Steele  wrote:
> 
> > Hi Alexander
> >
> > On 3/10/17 8:08 AM, Alexander Korotkov wrote:
> >
> > Results look good for me.  Idea of committing both of patches looks
> >> attractive.
> >> We have pretty much acceleration for read-only case and small
> >> acceleration for read-write case.
> >> I'll run benchmark on 72-cores machine as well.
> >>
> >
> > Have you had a chance to run those tests yet?
> >
> 
> I discovered an interesting issue.
> I found that ccce90b3 (which was reverted) gives almost same effect as
> PGXACT alignment on read-only test on 72-cores machine.

That's possibly because it changes alignment?


> That shouldn't be related to the functionality of ccce90b3 itself, because
> read-only test don't do anything with clog.  And that appears to be true.
> Padding of PGPROC gives same positive effect as ccce90b3.  Padding patch
> (pgproc-pad.patch) is attached.  It's curious that padding changes size of
> PGPROC from 816 bytes to 848 bytes.  So, size of PGPROC remains 16-byte
> aligned.  So, probably effect is related to distance between PGPROC
> members...
> 
> See comparison of 16-bytes alignment of PGXACT + reduce PGXACT access vs.
> padding of PGPROC.

My earlier testing had showed that padding everything is the best
approach :/


I'm inclined to push this to the next CF, it seems we need a lot more
benchmarking here.

Greetings,

Andres Freund


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

Re: [HACKERS] LWLock optimization for multicore Power machines

[Andres Freund]

Hi,

On 2017-03-31 13:38:31 +0300, Alexander Korotkov wrote:
> > It seems that on this platform definition of atomics should be provided by
> > fallback.h.  But it doesn't because I already defined 
> > PG_HAVE_ATOMIC_U32_SUPPORT
> > in arch-ppc.h.  I think in this case we shouldn't provide ppc-specific
> > implementation of pg_atomic_fetch_mask_add_u32().  However, I don't know
> > how to do this assuming arch-ppc.h is included before compiler-specific
> > headers.  Thus, in arch-ppc.h we don't know yet if we would find
> > implementation of atomics for this platform.  One possible solution is to
> > provide assembly implementation for all atomics in arch-ppc.h.

That'd probably not be good use of effort.


> BTW, implementation for all atomics in arch-ppc.h would be too invasive and
> shouldn't be considered for v10.
> However, I made following workaround: declare pg_atomic_uint32 and
> pg_atomic_fetch_mask_add_u32_impl() only when we know that generic-gcc.h
> would declare gcc-based atomics.
> Could you, please, check it on Apple PPC?

That doesn't sound crazy to me.


> +/*
> + * pg_atomic_fetch_mask_add_u32 - atomically check that masked bits in 
> variable
> + * and if they are clear then add to variable.

Hm, expanding on this wouldn't hurt.


> + * Returns the value of ptr before the atomic operation.

Sounds a bit like it'd return the pointer itself, not what it points to...


> + * Full barrier semantics.

Does it actually have full barrier semantics?  Based on a quick skim I
don't think the ppc implementation doesn't?



> +#if defined(HAVE_ATOMICS) \
> + && (defined(HAVE_GCC__ATOMIC_INT32_CAS) || 
> defined(HAVE_GCC__SYNC_INT32_CAS))
> +
> +/*
> + * Declare pg_atomic_uint32 structure before generic-gcc.h does it in order 
> to
> + * use it in function arguments.
> + */

If we do this, then the other side needs a pointer to keep this up2date.

> +#define PG_HAVE_ATOMIC_U32_SUPPORT
> +typedef struct pg_atomic_uint32
> +{
> + volatile uint32 value;
> +} pg_atomic_uint32;
> +
> +/*
> + * Optimized implementation of pg_atomic_fetch_mask_add_u32() for Power
> + * processors.  Atomic operations on Power processors are implemented using
> + * optimistic locking.  'lwarx' instruction 'reserves index', but that
> + * reservation could be broken on 'stwcx.' and then we have to retry.  Thus,
> + * each CAS operation is a loop.  But loop of CAS operation is two level 
> nested
> + * loop.  Experiments on multicore Power machines shows that we can have huge
> + * benefit from having this operation done using single loop in assembly.
> + */
> +#define PG_HAVE_ATOMIC_FETCH_MASK_ADD_U32
> +static inline uint32
> +pg_atomic_fetch_mask_add_u32_impl(volatile pg_atomic_uint32 *ptr,
> +   uint32 mask, 
> uint32 increment)
> +{
> + uint32  result,
> + tmp;
> +
> + __asm__ __volatile__(
> + /* read *ptr and reserve index */
> +#ifdef USE_PPC_LWARX_MUTEX_HINT
> + "   lwarx   %0,0,%5,1   \n"
> +#else
> + "   lwarx   %0,0,%5 \n"
> +#endif

Hm, we should remove that hint bit stuff at some point...


> + "   and %1,%0,%3\n" /* calculate '*ptr & mask" 
> */
> + "   cmpwi   %1,0\n" /* compare '*ptr & mark' vs 0 */
> + "   bne-$+16\n" /* exit on '*ptr & mark != 0' */
> + "   add %1,%0,%4\n" /* calculate '*ptr + 
> increment' */
> + "   stwcx.  %1,0,%5 \n" /* try to store '*ptr + increment' 
> into *ptr */
> + "   bne-$-24\n" /* retry if index reservation is 
> broken */
> +#ifdef USE_PPC_LWSYNC
> + "   lwsync  \n"
> +#else
> + "   isync   \n"
> +#endif
> + : "="(result), "="(tmp), "+m"(*ptr)
> + : "r"(mask), "r"(increment), "r"(ptr)
> + : "memory", "cc");
> + return result;
> +}

I'm not sure that the barrier semantics here are sufficient.


> +/*
> + * Generic implementation of pg_atomic_fetch_mask_add_u32() via loop
> + * of compare & exchange.
> + */
> +static inline uint32
> +pg_atomic_fetch_mask_add_u32_impl(volatile pg_atomic_uint32 *ptr,
> +   uint32 mask_, 
> uint32 add_)
> +{
> + uint32  old_value;
> +
> + /*
> +  * Read once outside the loop, later iterations will get the newer value
> +  * via compare & exchange.
> +  */
> + old_value = pg_atomic_read_u32_impl(ptr);
> +
> + /* loop until we've determined whether we could make an increment or 
> not */
> + while (true)
> + {
> + uint32  desired_value;
> + boolfree;
> +
> + desired_value = old_value;
> + free = (old_value & mask_) == 0;
> + if (free)
> + desired_value += add_;
> +
> + /*
> +   

Re: [HACKERS] Improve OR conditions on joined columns (common star schema problem)

[Tom Lane]

Andres Freund  writes:
> On 2017-02-14 14:18:54 -0500, Tom Lane wrote:
>> One point that could use further review is whether the de-duplication
>> algorithm is actually correct.  I'm only about 95% convinced by the
>> argument I wrote in planunionor.c's header comment.

> Are you planning to push this into v10? Given the looming freeze I'm
> inclined to bump this to the next CF.

I'm still not fully convinced the patch is correct, so I have no problem
with bumping it out.

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] few fts functions for jsonb

[Sven R. Kunze]

On 01.04.2017 22:20, Andrew Dunstan wrote:

I added documentation when I committed it for the new functions, in the
FTS section. I'm not sure what we need to add to the JSON section if
anything.


Not sure, if this is related but the formatting of 
https://www.postgresql.org/docs/devel/static/functions-textsearch.html 
looks a bit strange.


Just 2 questions/notes:
1) in what order are the values of the JSON extracted?

2) Regarding the additional line:
to_tsvector([ config regconfig , ] document json(b))tsvector reduce 
document text to tsvectorto_tsvector('english', '{"a": "The Fat 
Rats"}'::json)'fat':2 'rat':3


Maybe change "reduce document text to tsvector" to "extracting JSON 
values  and reduce to tsvector"?



Sven


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

[Andres Freund]

Hi,

On 2017-02-14 14:18:54 -0500, Tom Lane wrote:
> I think this might be code-complete, modulo the question of whether we
> want an enabling GUC for it.  I'm still concerned about the question
> of whether it adds more planning time than it's worth for most users.
> (Obviously it needs some regression test cases too, and a lot more
> real-world testing than I've given it.)

> One point that could use further review is whether the de-duplication
> algorithm is actually correct.  I'm only about 95% convinced by the
> argument I wrote in planunionor.c's header comment.
> 
> Potential future work includes finding join ORs in upper-level INNER JOIN
> ON clauses, and improving matters so we can do the unique-ification with
> hashing as well as sorting.  I don't think either of those things has to
> be in the first commit, though.

Are you planning to push this into v10? Given the looming freeze I'm
inclined to bump this to the next CF.

- Andres


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