Re: [HACKERS] Logical decoding on standby

[Craig Ringer]

On 27 June 2017 at 13:24, Craig Ringer  wrote:

> On 21 June 2017 at 17:30, sanyam jain  wrote:
> > Hi,
> > After changing
> > sendTimeLineIsHistoric = state->currTLI == ThisTimeLineID;
> > to
> > sendTimeLineIsHistoric = state->currTLI != ThisTimeLineID;
> >
> > I was facing another issue.
> > On promotion of a cascaded server ThisTimeLineID in the standby server
> > having logical slot becomes 0.
> > Then i added a function call to GetStandbyFlushRecPtr in
> > StartLogicalReplication which updates ThisTimeLineID.
> >
> > After the above two changes timeline following is working.But i'm not
> sure
> > whether this is correct or not.In any case please someone clarify.
>
> That's a reasonable thing to do, and again, I thought I did it in a
> later revision, but apparently not (?). I've been working on other
> things and have lost track of progress here a bit.
>
> I'll check more closely.
>
>
Hi all.

I've had to backburner this due to other work. In the process of looking
into an unrelated bug recently though, I noticed that the way we handle
snapshots may not be safe for historic snaphots on a standby. Historic
snapshots don't ever set takenDuringRecovery, which allows heapgetpage to
trust PD_IS_VISIBLE on a page. According to comments on heapgetpage that
could be an issue.

Minor compared to some of the other things that'll come up when finishing
this off, but worth remembering.


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

Re: [HACKERS] Proposal for CSN based snapshots

[Alexander Korotkov]

On Mon, Dec 4, 2017 at 6:07 PM, Alexander Kuzmenkov <
a.kuzmen...@postgrespro.ru> wrote:

> Performance on pgbench tpcb with subtransactions is now slightly better
> than master. See the picture 'savepoints2'. This was achieved by removing
> unnecessary exclusive locking on CSNLogControlLock in SubTransSetParent.
> After that change, both versions are mostly waiting on XidGenLock in
> GetNewTransactionId.
>
> Performance on default pgbench tpcb is also improved. At scale 500, csn is
> at best 30% faster than master, see the picture 'tpcb500'. These
> improvements are due to slight optimizations of GetSnapshotData and
> refreshing RecentGlobalXmin less often. At scale 1500, csn is slightly
> faster at up to 200 clients, but then degrades steadily: see the picture
> 'tpcb1500'. Nevertheless, CSN-related code paths do not show up in perf
> profiles or LWLock wait statistics [1]. I think what we are seeing here is
> again that when some bottlenecks are removed, the fast degradation of
> LWLocks under contention leads to net drop in performance. With this in
> mind, I tried running the same benchmarks with patch from Yura Sokolov [2],
> which should improve LWLock performance on NUMA machines. Indeed, with this
> patch csn starts outperforming master on all numbers of clients measured,
> as you can see in the picture 'tpcb1500'. This LWLock change influences the
> csn a lot more than master, which also suggests that we are observing a
> superlinear degradation of LWLocks under increasing contention.
>

These results look promising for me.  Could you try benchmarking using more
workloads including read-only and mixed mostly-read workloads?
You can try same benchmarks I used in my talk about CSN in pgconf.eu [1]
slides 19-25 (and you're welcome to invent more benchmakrs yourself)

Also, I wonder how current version of CSN patch behaves in worst case when
we have to scan the table with a lot of unique xid (and correspondingly
have to do a lot of csnlog lookups)?  See [1] slide 18.  This worst case
was significant part of my motivation to try "rewrite xid with csn"
approach.  Please, find simple extension I used to fill table with random
xmins in the attachment.

1. https://www.slideshare.net/AlexanderKorotkov/the-future-is-csn

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


randomize_xmin.tar.gz
Description: GNU Zip compressed data

Re: proposal: alternative psql commands quit and exit

[Daniel Vérité]

Everaldo Canuto wrote:

> Oracle's sqlplus uses "quit" or "exit" and MySQL client can be exited using
> "quit" and "exit" but for compatibility with psql, it also supports "\q"
> and "\quit".

When looking at the most popular postgres questions on stackoverflow:

https://stackoverflow.com/questions/tagged/postgresql?sort=votes

the first one (most up-voted) happens to be:

"How to exit from PostgreSQL command line utility: psql"

now at 430k views and 1368 upvotes.

Independently of the syntax-compatibility with other tools,
it can be a miserable experience for people who almost
never use psql to type "quit" or "exit" after they're done
with whatever they wanted to do, and nothing
happens except a tiny change in the prompt.

Implementing it is easy, but it might be a hard sell for the project
because it creates a precedent.
The next question in that list is "PostgreSQL DESCRIBE TABLE", so
why not implement "desc tablename;" as a synonym for
"\d tablename"?

The question after that is "Show tables in PostgreSQL", so
why not implement "show tablename;"? Well, this one is
impossible because SHOW is already a SQL command that does
something else, so that's already the end of that road to
compatibility.

Personally I'm +1 on accepting the additional "exit" and "quit"
as synonyms for \q, to comply with the "Do What I Mean" UX rule,
rather than just compatibility with other tools.
I guess this is why "help" is already a synonym of \h.


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

Re: proposal: alternative psql commands quit and exit

[Everaldo Canuto]

Hello,

On Fri, Dec 8, 2017 at 11:57 AM, Daniel Vérité" 
wrote:

> Implementing it is easy, but it might be a hard sell for the project
> because it creates a precedent.
>

 We already have "help" (without slash) so I don't think it is a precedent.


> The next question in that list is "PostgreSQL DESCRIBE TABLE", so
> why not implement "desc tablename;" as a synonym for
> "\d tablename"?
>

I must be honest to you, I create this small patch to see how is contribute
do Postgres and to check how is reception on Postgres community.
That said, DESC[RIBE] is on my list  for next patches but it is not a
synonym to \d.

DESC[RIBE] must be a SQL command, very useful when you are not running
psql. But let us discuss this on another thread ;-)



> The question after that is "Show tables in PostgreSQL", so
> why not implement "show tablename;"? Well, this one is
> impossible because SHOW is already a SQL command that does
> something else, so that's already the end of that road to
> compatibility.
>

Didn't know about SHOW command. Are TABLES a reserved word?

Regards.

Re: Fwd: [BUGS] pg_trgm word_similarity inconsistencies or bug

[Alexander Korotkov]

On Thu, Dec 7, 2017 at 8:59 PM, Robert Haas  wrote:

> On Tue, Nov 7, 2017 at 7:51 AM, Jan Przemysław Wójcik
>  wrote:
> > I'm afraid that creating a function that implements quite different
> > algorithms depending on a global parameter seems very hacky and would
> lead
> > to misunderstandings. I do understand the need of backward compatibility,
> > but I'd opt for the lesser evil. Perhaps a good idea would be to change
> the
> > name to 'substring_similarity()' and introduce the new function
> > 'word_similarity()' later, for example in the next major version release.
>
> That breaks things for everybody using word_similarity() currently.
> If the previous discussion of this topic concluded that
> word_similarity() was an OK name despite being a slight misnomer, I
> don't think we should change our mind now.  Instead the new function
> can be called something which makes the difference clear, e.g.
> strict_word_similarity(), and the old function can remain as it is.


+1
Thank you for pointing this.  Yes, it would be better not to change
existing names and behavior, but adjust documentation and add alternative
behavior with another name.
Therefore, I'm going to provide patchset of two patches:
1) Improve word_similarity() documentation.
2) Add new function strict_word_similarity() (or whatever better name we
invent).

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

Re: [HACKERS] [PATCH] Incremental sort

[Alexander Korotkov]

On Wed, Nov 22, 2017 at 12:01 AM, Thomas Munro <
thomas.mu...@enterprisedb.com> wrote:

> >> I gather that you have
> >> determined empirically that it's better to be able to sort groups of
> >> at least MIN_GROUP_SIZE than to be able to skip the comparisons on the
> >> leading attributes, but why is that the case?
> >
> > Right.  The issue that not only case of one tuple per group could cause
> > overhead, but few tuples (like 2 or 3) is also case of overhead.  Also,
> > overhead is related not only to sorting.  While investigate of regression
> > case provided by Heikki [1], I've seen extra time spent mostly in extra
> > copying of sample tuple and comparison with that.  In order to cope this
> > overhead I've introduced MIN_GROUP_SIZE which allows to skip copying
> sample
> > tuples too frequently.
>
> I see.  I wonder if there could ever be a function like
> ExecMoveTuple(dst, src).  Given the polymorphism involved it'd be
> slightly complicated and you'd probably have a general case that just
> copies the tuple to dst and clears src, but there might be a bunch of
> cases where you can do something more efficient like moving a pointer
> and pin ownership.  I haven't really thought that through and
> there may be fundamental problems with it...
>

ExecMoveTuple(dst, src) would be good.  But, it would be hard to implement
"moving a pointer and pin ownership" principle in our current
infrastructure.  It's because source and destination can have different
memory contexts.  AFAICS, we can't just move memory area between memory
contexts: we have to allocate new area, then memcpy, and then deallocate
old area.


> If you're going to push the tuples into the sorter every time, then I
> guess there are some special cases that could allow future
> optimisations: (1) if you noticed that every prefix was different, you
> can skip the sort operation (that is, you can use the sorter as a dumb
> tuplestore and just get the tuples out in the same order you put them
> in; not sure if Tuplesort supports that but it presumably could),


In order to notice that every prefix is different, I have to compare every
prefix.  But that may introduce an overhead.  So, there reason why I
introduced MIN_GROUP_SIZE is exactly to not compare every prefix...


> (2)
> if you noticed that every prefix was the same (that is, you have only
> one prefix/group in the sorter) then you could sort only on the suffix
> (that is, you could somehow tell Tuplesort to ignore the leading
> columns),


Yes, I did so before.  But again, after introducing MIN_GROUP_SIZE, I
missed knowledge whether all the prefixes were the same or different.  This
is why, I've to sort by full column list for now...

(3) as a more complicated optimisation for intermediate
> group sizes 1 < n < MIN_GROUP_SIZE, you could somehow number the
> groups with an integer that increments whenever you see the prefix
> change, and somehow tell tuplesort.c to use that instead of the
> leading columns.


That is interesting idea.  The reason we have an overhead in comparison
with plain sort is that we do extra comparison (and copying), but knowledge
of this comparison result is lost for sorting itself.  Thus, sorting can
"reuse" prefix comparison, and overhead would be lower.  But the problem is
that we have to reformat tuples before putting them into tuplesort.  I
wonder if tuple reformatting could eat potential performance win...

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

Re: [HACKERS] [PATCH] Incremental sort

[Alexander Korotkov]

Hi!

On Fri, Dec 1, 2017 at 11:39 AM, Antonin Houska  wrote:

> I expected the number of groups actually that actually appear in the
> output,
> you consider it the number of groups started. I can't find similar case
> elsewhere in the code (e.g. Agg node does not report this kind of
> information), so I have no clue. Someone else will have to decide.
>

OK.

> But there is IncrementalSort node on the remote side.
> > Let's see what happens. Idea of "CROSS JOIN, not pushed down" test is
> that cross join with ORDER BY LIMIT is not beneficial to push down, because
> LIMIT is not pushed down and remote side wouldn't be able to use top-N
> heapsort. But if remote side has incremental sort then it can be
> > used, and fetching first 110 rows is cheap. Let's see plan of original
> "CROSS JOIN, not pushed down" test with incremental sort.
> >
> > # EXPLAIN (ANALYZE, VERBOSE) SELECT t1.c3, t2.c3 FROM ft1 t1 CROSS JOIN
> ft2 t2 ORDER BY t1.c1, t2.c1 OFFSET 100 LIMIT 10;
>
> ok, understood, thanks. Perhaps it's worth a comment in the test script.
>
> I'm afraid I still see a problem. The diff removes a query that (although a
> bit different from the one above) lets the CROSS JOIN to be pushed down and
> does introduce the IncrementalSort in the remote database. This query is
> replaced with one that does not allow for the join push down.
>
> *** a/contrib/postgres_fdw/sql/postgres_fdw.sql
> --- b/contrib/postgres_fdw/sql/postgres_fdw.sql
> *** SELECT t1.c1 FROM ft1 t1 WHERE NOT EXIST
> *** 510,517 
>   SELECT t1.c1 FROM ft1 t1 WHERE NOT EXISTS (SELECT 1 FROM ft2 t2 WHERE
> t1.c1 = t2.c2) ORDER BY t1.c1 OFFSET 100 LIMIT 10;
>   -- CROSS JOIN, not pushed down
>   EXPLAIN (VERBOSE, COSTS OFF)
> ! SELECT t1.c1, t2.c1 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c1, t2.c1
> OFFSET 100 LIMIT 10;
> ! SELECT t1.c1, t2.c1 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c1, t2.c1
> OFFSET 100 LIMIT 10;
>   -- different server, not pushed down. No result expected.
>   EXPLAIN (VERBOSE, COSTS OFF)
>   SELECT t1.c1, t2.c1 FROM ft5 t1 JOIN ft6 t2 ON (t1.c1 = t2.c1) ORDER BY
> t1.c1, t2.c1 OFFSET 100 LIMIT 10;
> --- 510,517 
>   SELECT t1.c1 FROM ft1 t1 WHERE NOT EXISTS (SELECT 1 FROM ft2 t2 WHERE
> t1.c1 = t2.c2) ORDER BY t1.c1 OFFSET 100 LIMIT 10;
>   -- CROSS JOIN, not pushed down
>   EXPLAIN (VERBOSE, COSTS OFF)
> ! SELECT t1.c3, t2.c3 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c3, t2.c3
> OFFSET 100 LIMIT 10;
> ! SELECT t1.c3, t2.c3 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c3, t2.c3
> OFFSET 100 LIMIT 10;
>   -- different server, not pushed down. No result expected.
>   EXPLAIN (VERBOSE, COSTS OFF)
>   SELECT t1.c1, t2.c1 FROM ft5 t1 JOIN ft6 t2 ON (t1.c1 = t2.c1) ORDER BY
> t1.c1, t2.c1 OFFSET 100 LIMIT 10;
>
> Shouldn't the test contain *both* cases?


Thank you for pointing that.  Sure, both cases are better.  I've added
second case as well as comments.  Patch is attached.

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


incremental-sort-12.patch
Description: Binary data

Re: proposal: alternative psql commands quit and exit

[Nikolay Samokhvalov]

On Fri, Dec 8, 2017 at 2:10 AM, Michael Paquier 
wrote:
>
> I think that you are going to need better reasons than just being more
> friendly with other database clients ...
>

This is not about other database clients. This is about users considering
migration to Postgres.
People!

I'm definitely +1 for this, I've seen many people struggling because of
this. Being more friendly to new users = more users in future.

At least "quit" and "exit" might be aliases for "help".

Re: proposal: alternative psql commands quit and exit

[Ryan Murphy]

I am +1 on doing this too, unless we can imagine it clashing with any SQL
queries or breaking scripts (which I can't).

Helping people migrate to postgres w minimal frustration is important
enough imho to be worth breaking this \ rule unless we can forsee real
actual compatibility problems.  We already have "help", so it's not like we
have literally 0 commands that don't start with \.

Best,
Ryan

Re: explain analyze output with parallel workers - question about meaning of information for explain.depesz.com

[Amit Kapila]

On Fri, Dec 8, 2017 at 9:33 AM, Amit Kapila  wrote:
> On Fri, Dec 8, 2017 at 12:06 AM, Robert Haas  wrote:
>> On Wed, Dec 6, 2017 at 1:05 AM, Amit Kapila  wrote:
>>> Right and seeing that I have prepared the patch (posted above [1])
>>> which fixes it such that it will resemble the non-parallel case.
>>> Ideally, it would have obviated the need for my previous patch which
>>> got committed as 778e78ae.  However, now that is committed, I could
>>> think of below options:
>>>
>>> 1. I shall rebase it atop what is committed and actually, I have done
>>> that in the attached patch.  I have also prepared a regression test
>>> case patch just to show the output with and without the patch.
>>> 2. For sort node, we can fix it by having some local_info same as
>>> shared_info in sort node and copy the shared_info in that or we could
>>> reinstate the pointer to the DSM in ExecSortReInitializeDSM() by
>>> looking it up in the TOC as suggested by Thomas. If we go this way,
>>> then we need a similar fix for hash node as well.
>>
>> Well, the patch you've actually attached makes the bug go away by
>> removing a net of 53 lines of code.  The other approach would probably
>> add code.  So I am tempted to go with the approach you have here.  I
>> would probably change the T_HashState and T_SortState cases in
>> ExecParallelReInitializeDSM so that they still exist, but just do
>> something like this:
>>
>> case T_HashState:
>> case T_SortState:
>> /* these nodes have DSM state, but no reinitialization is required */
>> break;
>>
>> That way, it will be more clear to future readers of this code that
>> the lack of a reinitialize function is not an oversight, and the
>> compiler should optimize these cases away, merging them with the
>> default case.
>>
>
> Okay, I have adjusted the patch accordingly.  I have also added a
> regression test which should produce the same result across different
> runs, see if that looks okay to you, then it is better to add such a
> test as well.
>

The regression test added by patch needs cleanup at the end which I
have added in the attached patch.


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


fix_accum_instr_parallel_workers_v7.patch
Description: Binary data

Re: proposal: alternative psql commands quit and exit

[Michael Paquier]

On Fri, Dec 8, 2017 at 7:06 PM, Everaldo Canuto
 wrote:
> For people already know psql nothing is changed, we will just have a hidden
> way to exit.
>
> I just think that if it don't change or break anything for usual psql user
> but also help some new comers, then is good.
>
> And remember, mysql client implement "\q" and "\quit" just to be more
> friendly to psql users. Lets us be friendly too ;-)

I think that you are going to need better reasons than just being more
friendly with other database clients by breaking a rule which is
around for 20 years. For one, it seems to me that your patch is
breaking multi-line SQL support with psql, as "quit" or "exit" are
legit SQL object names.
-- 
Michael

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Masahiko Sawada]

On Fri, Dec 1, 2017 at 11:51 AM, Michael Paquier
 wrote:
> On Wed, Nov 29, 2017 at 7:36 AM, Michael Paquier
>  wrote:
>> On Wed, Nov 29, 2017 at 6:44 AM, Robert Haas  wrote:
>>> On Tue, Nov 21, 2017 at 4:32 AM, Masahiko Sawada  
>>> wrote:
 Thank you for comments. Attached updated patch.
>>>
>>> I see that Michael has marked this Ready for Committer, but also that
>>> he didn't update the thread, so perhaps some interested committer
>>> (Fujii Masao?) might have missed the fact that Michael thinks it's
>>> ready to go.
>>
>> Sorry for not mentioning that directly on the thread.
>>
>>> Anyone interested in taking a look at this one for commit?
>>
>> I would assume that Fujii-san would be the chosen one here as he has
>> worked already on the thread.
>
> No replies yet. So I am moving this patch to next CF.

After off-discussion with Fujii-san, I've updated the comment of why
we should disallow interrupts before setting/cleanup the session-level
lock. Please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


fix_do_pg_abort_backup_v10.patch
Description: Binary data

Re: [HACKERS] postgres_fdw bug in 9.6

[Etsuro Fujita]

(2017/11/30 23:22), Tom Lane wrote:

Etsuro Fujita  writes:

(2017/11/30 7:32), Tom Lane wrote:

the output of the foreign join cannot change during EPQ, since the remote
server already locked the rows before returning them.  The only thing that
can change is the output of the local scan on public.tab.  Yes, we then
need to re-verify that foo.a = tab.a ... but in this example, that's the
responsibility of the NestLoop plan node, not the foreign join.



That's right, but is that true when the FDW uses late row locking?


An FDW using late row locking would need to work harder, yes.  But
that's true at the scan level as well as the join level.  We have
already committed to using early locking in postgres_fdw, for the
network-round-trip-cost reasons I mentioned before, and I can't see
why we'd change that decision at the join level.


My concern is FDWs that support join pushdown in combination with late 
row locking.  I don't know whether such FDWs really exist, but if so, an 
output of a foreign join computed from re-fetched tuples might change.



Right now we've got the worst of both worlds, in that we're actually
doing early row locking on the remote, but we're paying (some of) the
code complexity costs required for late locking.


Because we have provided the late row locking API, I think we should pay 
a certain degree of consideration for such FDWs.


Best regards,
Etsuro Fujita

Re: proposal: alternative psql commands quit and exit

[Everaldo Canuto]

On Fri, Dec 8, 2017 at 5:22 AM, Sergei Kornilov  wrote:

> Why not just use ctrl+D shortcut? This EOF signal works both in bash,
> mysql, psql, any CLI tool which I remember
>

Because like I wrote, other sql clients also support "exit" and "quit".

Re: proposal: alternative psql commands quit and exit

[Everaldo Canuto]

On Fri, Dec 8, 2017 at 4:00 AM, Laurenz Albe 
wrote:

> I am -1 on that, because I think that it is not good to break the simple
> rule
> that everything that is a psql command starts with a backslash.
>

We already have "help" command without slash and that is why I implement in
the same way help is implemented.
Also, I don't really break the rules, usual way still works, I just added
an alternative way.

It might reach out to newcomers, but it will confuse people who know psql.
>

For people already know psql nothing is changed, we will just have a hidden
way to exit.

I just think that if it don't change or break anything for usual psql user
but also help some new comers, then is good.

And remember, mysql client implement "\q" and "\quit" just to be more
friendly to psql users. Lets us be friendly too ;-)

Regards

no partition pruning when partitioning using array type

[Amit Langote]

Hi.

I noticed that if you partition using a array type column, partition
pruning using constraint exclusion fails to work due to a minor problem.

Example:

create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');
create table p1 partition of p for values in ('{2, 3}', '{4, 5}');

explain select a from p where a = '{1}';
QUERY PLAN
|-
 Append  (cost=0.00..54.00 rows=14 width=32)
   ->  Seq Scan on p1  (cost=0.00..27.00 rows=7 width=32)
 Filter: (a = '{1}'::integer[])
   ->  Seq Scan on p2  (cost=0.00..27.00 rows=7 width=32)
 Filter: (a = '{1}'::integer[])

explain select a from p where a = '{2, 3}';
QUERY PLAN
|-
 Append  (cost=0.00..54.00 rows=14 width=32)
   ->  Seq Scan on p1  (cost=0.00..27.00 rows=7 width=32)
 Filter: (a = '{2,3}'::integer[])
   ->  Seq Scan on p2  (cost=0.00..27.00 rows=7 width=32)
 Filter: (a = '{2,3}'::integer[])
(5 rows)


In the case of array type partition key, make_partition_op_expr() will
have to put a RelabelType node on top of the partition key Var, after
having selected an = operator from the array_ops family.  The RelabelType
causes operator_predicate_proof() to fail to consider predicate leftop and
clause leftop as equal, because only one of them ends up having the
RelabelType attached to it.

As a simple measure, the attached patch teaches operator_predicate_proof()
to strip RelabelType nodes from all the nodes it compares using equal().
I also added a relevant test in partition_prune.sql.

Thoughts?

Thanks,
Amit
From 2a25202f2f0a415c6884c28fd5bc76243507d5c4 Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 8 Dec 2017 19:09:31 +0900
Subject: [PATCH] Teach operator_predicate_proof() to strip RelabelType

---
 src/backend/optimizer/util/predtest.c | 13 
 src/test/regress/expected/partition_prune.out | 29 +++
 src/test/regress/sql/partition_prune.sql  |  8 
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/src/backend/optimizer/util/predtest.c 
b/src/backend/optimizer/util/predtest.c
index 134460cc13..d0f6278984 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -1407,6 +1407,11 @@ static const StrategyNumber BT_refute_table[6][6] = {
{none, none, BTEQ, none, none, none}/* NE */
 };
 
+/* Strip expr of the surrounding RelabelType, if any. */
+#define strip_relabel(expr) \
+   ((Node *) (IsA((expr), RelabelType) \
+   ? ((RelabelType *) (expr))->arg \
+   : (expr)))
 
 /*
  * operator_predicate_proof
@@ -1503,10 +1508,10 @@ operator_predicate_proof(Expr *predicate, Node *clause, 
bool refute_it)
/*
 * We have to match up at least one pair of input expressions.
 */
-   pred_leftop = (Node *) linitial(pred_opexpr->args);
-   pred_rightop = (Node *) lsecond(pred_opexpr->args);
-   clause_leftop = (Node *) linitial(clause_opexpr->args);
-   clause_rightop = (Node *) lsecond(clause_opexpr->args);
+   pred_leftop = strip_relabel(linitial(pred_opexpr->args));
+   pred_rightop = strip_relabel(lsecond(pred_opexpr->args));
+   clause_leftop = strip_relabel(linitial(clause_opexpr->args));
+   clause_rightop = strip_relabel(lsecond(clause_opexpr->args));
 
if (equal(pred_leftop, clause_leftop))
{
diff --git a/src/test/regress/expected/partition_prune.out 
b/src/test/regress/expected/partition_prune.out
index aabb0240a9..7c78f11d55 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1092,4 +1092,33 @@ explain (costs off) select * from boolpart where a is 
not unknown;
  Filter: (a IS NOT UNKNOWN)
 (7 rows)
 
+-- array type list partition key
+create table arrpart (a int[]) partition by list (a);
+create table arrpart1 partition of arrpart for values in ('{1}');
+create table arrpart2 partition of arrpart for values in ('{2, 3}', '{4, 5}');
+explain (costs off) select * from arrpart where a = '{1}';
+   QUERY PLAN   
+
+ Append
+   ->  Seq Scan on arrpart1
+ Filter: (a = '{1}'::integer[])
+(3 rows)
+
+explain (costs off) select * from arrpart where a = '{1, 2}';
+QUERY PLAN
+--
+ Result
+   One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from arrpart where a in ('{4, 5}', '{1}');
+  QUERY PLAN  
+--
+ Append
+   ->  Seq Scan on arrpart1
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+   ->  Seq Scan on 

ScalarArrayOpExpr and multi-dimensional arrays

[Amit Langote]

Hi.

I wonder if ScalarArrayOpExpr is not really meant for multi-dimensional
arrays appearing on the right hand side?  Because:

# select array[1] = any (array[array[1], array[2]]);

ERROR:  operator does not exist: integer[] = integer
LINE 1: select array[1] = any (array[array[1], array[2]]);
^
HINT:  No operator matches the given name and argument types. You might
need to add explicit type casts.


I noticed this when looking at the constraint of a list partitioned table
on a int[] column.

create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');

\d+ p1
...
Partition of: p FOR VALUES IN ('{1}')
Partition constraint: ((a IS NOT NULL) AND ((a)::anyarray
OPERATOR(pg_catalog.=) ANY (ARRAY['{1}'::integer[]])))

I got the same error as above when I try to put that ANY expression in a
query:

select (a)::anyarray OPERATOR(pg_catalog.=) ANY (ARRAY['{1}'::integer[]])
from p;
ERROR:  operator does not exist: integer[] pg_catalog.= integer

I guess we shouldn't be generating such a constraint expression if backend
is not going to accept the same.  Or should ScalarArrayOpExpr be made to
sanely process multi-dimensional arrays appearing on the right hand side?

Thanks,
Amit

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Michael Paquier]

On Fri, Dec 8, 2017 at 5:38 PM, Masahiko Sawada  wrote:
> After off-discussion with Fujii-san, I've updated the comment of why
> we should disallow interrupts before setting/cleanup the session-level
> lock. Please review it.

+   /*
+* Set session-level lock. If we allow interrupts before setting
+* session-level lock, we could call callbacks with an inconsistent
+* state. To avoid calling CHECK_FOR_INTERRUPTS by LWLockReleaseClearVar
+* which is called by WALInsertLockRelease before changing the backup
+* state we change it while holding the WAL insert lock.
+*/
So you are just adding the reference to WALInsertLockRelease.. Instead
of writing the function names for LWLocks, I would just write "To
avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a
LWLock" and be done with it. There is no point to list a full function
dependency list, which could change in the future with static routines
of lwlock.c.
-- 
Michael

Re: proposal: alternative psql commands quit and exit

[Everaldo Canuto]

On Fri, Dec 8, 2017 at 8:10 AM, Michael Paquier 
wrote:
>
> I think that you are going to need better reasons than just being more
> friendly with other database clients by breaking a rule which is
> around for 20 years.


Well, that reason was enough for other sql clients to implement "\q" and
"\quit" even if they don't use "\" commands.
Note that we are not breaking a rule, we already have "help" without slash
and as I said, "slash way" still supported and still oficial way to exit.


> For one, it seems to me that your patch is
> breaking multi-line SQL support with psql, as "quit" or "exit" are
> legit SQL object names.
>

No, it is not breaking. Did you test it? My patch takes care of multi-line
commands. Look at second line of patch:

pset.cur_cmd_interactive && query_buf->len == 0

Line 4 also take care about other possible problems:

(line[4] == '\0' || line[4] == ';' || isspace((unsigned char) line[4])))


I know that this "feature" don't make any difference for most of you guys
but also it will not hurt anyone and will help a little for newcomers and
people that uses different sql clients. To be honest. I could understand
that is not a good idea to accept patches like this if it is too big since
it could make things hard to maintainers but honestly, this patch is very
small and trivial.


Regards.

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Tels]

Hello Rushabh,

On Fri, December 8, 2017 2:28 am, Rushabh Lathia wrote:
> Thanks for review.
>
> On Fri, Dec 8, 2017 at 6:27 AM, Peter Geoghegan  wrote:
>
>> On Thu, Dec 7, 2017 at 12:25 AM, Rushabh Lathia
>>  wrote:
>> > 0001-Add-parallel-B-tree-index-build-sorting_v14.patch

I've looked only at patch 0002, here are some comments.

> + * leaderasworker indicates whether leader going to participate as
worker  or
> + * not.

The grammar is a bit off, and the "or not" seems obvious. IMHO this could be:

+ * leaderasworker indicates whether the leader is going to participate as
worker

The argument leaderasworker is only used once and for one temp. variable
that is only used once, too. So the temp. variable could maybe go.

And not sure what the verdict was from the const-discussion threads, I did
not follow it through. If "const" is what should be done generally, then
the argument could be consted, as to not create more "unconsted" code.

E.g. so:

+plan_create_index_workers(Oid tableOid, Oid indexOid, const bool
leaderasworker)

and later:

-   sort_mem_blocks / (parallel_workers + 1) <
min_sort_mem_blocks)
+   sort_mem_blocks / (parallel_workers + (leaderasworker
? 1 : 0)) < min_sort_mem_blocks)

Thank you for working on this patch!

All the best,

Tels

Re: proposal: alternative psql commands quit and exit

[Everaldo Canuto]

On Fri, Dec 8, 2017 at 2:26 PM, Vladimir Svedov  wrote:

> I wonder if *exit;* to terminate loop be confused with exit psql in case
> of bad syntax. then instead of reporting error in plpgsql it would just
> silently exit?..
>

I just tested it and works as expected. Second line of patch takes care of
it:

   if (pset.cur_cmd_interactive && query_buf->len == 0 &&

Re: proposal: alternative psql commands quit and exit

[Oliver Ford]

On Thu, Dec 7, 2017 at 11:47 PM, Everaldo Canuto
 wrote:
> Some of us unfortunately have to work with multiple databases like Oracle or
> MySQL. Their respective clients mysql and sqlplus uses "quit" or "exit" to
> exit sql client.
>
> Oracle's sqlplus uses "quit" or "exit" and MySQL client can be exited using
> "quit" and "exit" but for compatibility with psql, it also supports "\q" and
> "\quit".
>
> Postgres psql already support "\q" and "\quit" but I think that could be
> cool if it supports "exit" and "quit", talking to friends I saw that I am
> the only that sometimes try to exit psql with "exit'.

+1 from me. When I first used Postgres I struggled with how to quit
psql. I felt that making people look up how to quit the program is bad
UI design. I admired Postgres as a database, but had the impression
that it was harder to use than MySQL. Not being able to quit or
describe a table in the way I was used to was frustrating.

If anyone is unsure on this point, I'd recommend reading Joel
Spolsky's articles on UI design. He clearly explains how a program
model should match a user model. The fact that it's always been done
this way is irrelevant to new users, who want a db that is intuitive.

Re: ScalarArrayOpExpr and multi-dimensional arrays

[Tom Lane]

Amit Langote  writes:
> I wonder if ScalarArrayOpExpr is not really meant for multi-dimensional
> arrays appearing on the right hand side?  Because:
> # select array[1] = any (array[array[1], array[2]]);

> ERROR:  operator does not exist: integer[] = integer

You are falling into the misimpression that a 2-D array is an array of
1-D arrays.  It is not, even if the syntax makes it look like that.

ScalarArrayOpExpr just iterates over the array elements without regard
to dimensionality; so the LHS must be of the element type.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[Ryan Murphy]

>
> ​There isn't going to be that much appetite for this just so that people
> can use PostgreSQL without reading our documentation: or the output of
> typing "help" at the psql prompt that says "type \q to quit".​
>
>
Agree with this.  The whole reason people here are reluctant to add "quit"
in the first place is that it could become a slippery slope where people
now want every popular MySQL/Oracle command cloned, just so they can avoid
reading docs and never understand the design/power/elegance of Postgres.

For example, while I think "quit" is a good idea, it'd be a shame if it
caused people to never find out about \ commands.

> I'll agree that exiting the program is a special case that merits
consideration

Yes, I think we can find the right balance here.  We don't need to suddenly
start implementing SHOW TABLES, as attractive as that is to people who know
MySQL.

Re: proposal: alternative psql commands quit and exit

[David G. Johnston]

On Fri, Dec 8, 2017 at 8:09 AM, Ryan Murphy  wrote:

> ​There isn't going to be that much appetite for this just so that people
>> can use PostgreSQL without reading our documentation: or the output of
>> typing "help" at the psql prompt that says "type \q to quit".​
>>
>> For example, while I think "quit" is a good idea, it'd be a shame if it
> caused people to never find out about \ commands.
>

​I suppose the only bad thing about introducing "exit"​, which itself seems
like the more common choice for program stopping command, is that someone
might end up inferring that "\e" should work to exit the program instead of
opening an editor...

David J.

Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding

[Peter Eisentraut]

On 12/1/17 18:11, Michael Paquier wrote:
> Cool. Thanks. For REL_10_STABLE, I would suggest the attached patch
> then. This ensures that eSws is checked in the final message and that
> the cbind-flag sent in the first message maps with the data of the
> final message in the backend. I have checked with the following
> configurations with a v10 backend:
> - v11 libpq with SSL
> - v11 libpq without SSL
> - v10 libpq with SSL
> - v10 libpq without SSL
> And in all cases the connection is accepted as it should.

Committed.

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

Re: [HACKERS] parallel.c oblivion of worker-startup failures

[Robert Haas]

On Fri, Dec 8, 2017 at 4:56 AM, Amit Kapila  wrote:
> I think the optimization you are suggesting has a merit over what I
> have done in the patch.  However, one point to note is that we are
> anyway going to call that function down in the path(
> WaitForParallelWorkersToExit->WaitForBackgroundWorkerShutdown->GetBackgroundWorkerPid),
> so we might want to take the advantage of calling it first time as
> well.  We can actually cache the status of workers that have returned
> BGWH_STOPPED and use it later so that we don't need to make this
> function call again for workers which are already stopped.  We can
> even do what Tom is suggesting to avoid calling it for workers which
> are known to be launched, but I am really not sure if that function
> call is costly enough that we need to maintain one extra state to
> avoid that.

Well, let's do what optimization we can without making it too complicated.

> While looking into this code path, I wonder how much we are gaining by
> having two separate calls (WaitForParallelWorkersToFinish and
> WaitForParallelWorkersToExit) to check the status of workers after
> finishing the parallel execution?  They are used together in rescan
> code path, so apparently there is no benefit calling them separately
> there.  OTOH, during shutdown of nodes, it will often be the case that
> they will be called in a short duration after each other except for
> the case where we need to shut down the node for the early exit like
> when there is a limit clause or such.

I'm not 100% sure either, but if we're going to do anything about
that, it seems like a topic for another patch.  I don't think it's
completely without value because there is some time after we
WaitForParallelWorkersToFinish and before we
WaitForParallelWorkersToExit during which we can do things like
retrieve instrumentation data and shut down other nodes, but whether
it pulls it weight in code I don't know.  This kind of grew up
gradually: originally I/we didn't think of all of the cases where we
needed the workers to actually exit rather than just indicating that
they were done generating tuples, and the current situation is the
result of a series of bug-fixes related to that oversight, so it's
quite possible that a redesign would make sense.

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

Re: [HACKERS] What does it mean by XLOG_BACKUP_RECORD?

[Peter Eisentraut]

On 6/29/17 06:09, Masahiko Sawada wrote:
> Thanks, I agree to use XLOG_BACKUP_END instead.
> 
>> Worse, the current comment implies that
>> minRecoveryPoint is incorrectly set if it is true. Bleh.
> 
> Looking at the condition, we use minRecoveryPoint only when
> ControlFile->backupEndRequired is *false*. So I guess that it means
> that minRecoveryPoint is incorrectly set if
> ControlFile->backupEndReuired is true. Am I missing something?

I agree with you that the logic in the comment is correct.  I've
committed just the symbol change.

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

Re: proposal: alternative psql commands quit and exit

[Vladimir Svedov]

I wonder if *exit;* to terminate loop be confused with exit psql in case of
bad syntax. then instead of reporting error in plpgsql it would just
silently exit?..

2017-12-08 15:19 GMT+00:00 Tom Lane :

> "David G. Johnston"  writes:
> > I'll agree that exiting the program is a special case that merits
> > consideration - and it has been given that in the form of the concession
> to
> > the word "help" in order to get the reader unfamiliar with our backslash
> > prefix a non-backslash way to obtain that knowledge apart from reading
> our
> > docs.  Under that premise I would accept (lacking compelling arguments
> for
> > why its a bad idea) this proposal for quit/exit but am against anything
> > beyond that.
>
> Meh.  I never thought that the "help" business was particularly well
> thought out, and this proposal isn't better.  The reason is that to
> avoid breaking multi-line SQL command entry, we can only accept such
> a command when the input buffer is empty.  A psql novice is unlikely
> to be familiar with that concept, let alone know that \r or ^C is the
> way to get there.  There's a weak argument that "help" is of some
> value because it's likely to be the first thing a novice types, but
> that doesn't apply for quit/exit.  The typical interaction I'd foresee
> is more like
>
> postgres=> select 2+2  (user forgets semicolon)
> postgres-> help
> postgres-> quit
> postgres-> exit
>
> with nothing accomplished except to increase the user's frustration
> each time.  Eventually she'll hit on ^D and get out of it, but none
> of these allegedly novice-friendly "features" helped at all.
>
> regards, tom lane
>
>

Re: proposal: alternative psql commands quit and exit

[Oliver Ford]

On Fri, Dec 8, 2017 at 3:10 PM, David G. Johnston
 wrote:
> On Fri, Dec 8, 2017 at 7:34 AM, Oliver Ford  wrote:
>>
>> On Thu, Dec 7, 2017 at 11:47 PM, Everaldo Canuto
>>  wrote:
>>
>> +1 from me. When I first used Postgres I struggled with how to quit
>> psql. I felt that making people look up how to quit the program is bad
>> UI design. I admired Postgres as a database, but had the impression
>> that it was harder to use than MySQL.
>
>
>>
>> Not being able to quit or
>> describe a table in the way I was used to was frustrating.
>
>
> Whomever comes second is almost always going to have that problem.

And for most people, Postgres is their second or third database.

>> who want a db that is intuitive.
>
>
> Intuitive and "works like xyz" are not the same thing ...


Most people will have worked with Bash, Powershell, MySQL, etc and for
them intuitive means typing exit or quit. It would be strange to them
that psql behaves differently than what they're used to. My guess
would be that this quitting difficulty put more people off Postgres
than anything to do with the backend. First impressions count when
retaining users.

Re: Speeding up pg_upgrade

[Mark Dilger]

> On Dec 8, 2017, at 9:21 AM, Stephen Frost  wrote:
> 
> Mark,
> 
> * Mark Dilger (hornschnor...@gmail.com) wrote:
>>> On Dec 7, 2017, at 10:24 PM, Bruce Momjian  wrote:
>>> I think the big problem with two-stage pg_upgrade is that the user steps
>>> are more complex, so what percentage of users are going use the
>>> two-stage method.  The bad news is that only a small percentage of users
>>> who will benefit from it will use it, and some who will not benefit it
>>> will use it.  Also, this is going to require significant server changes,
>>> which have to be maintained.
>> 
>> In my fork of the project, back when I was tracking 9.5, I added an option
>> to vacuum/analyze to make it behave a bit more like autovac, so that I could
>> run
>> 
>>  ANALYZE CONDITIONALLY;
>> 
>> and it would only analyze those tables in the system which autovac would
>> analyze.  In the grammar, CONDITIONALLY gets translated into a
>> VacuumOption flag.  In vacuum (in src/backend/commands/vacuum.c), inside
>> the "Loop to process each selected relation", if this flag is set, it checks 
>> the
>> PgStat_StatTabEntry for the table to determine whether to vacuum or analyze
>> the table.
>> 
>> I think this extension would be helpful in the context of the current 
>> conversation.
>> In those cases where pg_upgrade was able to migrate the statistics to the
>> new database, as long as it set the PgStat_StatTabEntry for each table where
>> statistics were migrated, then the user would just have to execute a
>> "VACUUM CONDITIONALLY" after upgrade, and the database would either
>> do a lot of analyze work, a little analyze work, or no analyze work depending
>> on which tables needed analyzing.
>> 
>> The main advantage here is that the user would always run this command
>> after pg_upgrade, without having to think about whether pg_upgrade had
>> migrated statistics or not.
>> 
>> If the community thinks this is useful, I could put together a patch.
> 
> This certainly sounds nice though I have to admit to being a bit
> skeptical on the keyword selection, but perhaps blue really is the right
> color for that bike shed.
> 
> One thing I'd wonder about is if that makes 'CONDITIONALLY' into a
> reserved keyword, which wouldn't be ideal.  Perhaps a bit of a stretch
> but 'ANALYZE ALL NEEDED' might avoid that?

Yeah, I expected some complaint about CONDITIONALLY, and I don't have
any personal feelings about the choice of terms.  I'm happy to go with
your choice, or whatever the community decides.

mark

Re: Speeding up pg_upgrade

[Stephen Frost]

Bruce,

* Bruce Momjian (br...@momjian.us) wrote:
> I think the big problem with two-stage pg_upgrade is that the user steps
> are more complex, so what percentage of users are going use the
> two-stage method.  The bad news is that only a small percentage of users
> who will benefit from it will use it, and some who will not benefit it
> will use it.  Also, this is going to require significant server changes,
> which have to be maintained.

This is where I think we need to be considering a higher-level tool that
makes it easier to run pg_upgrade and which could handle these multiple
stages.

> I think we need some statistics on how many users are going to benefit
> from this, and how are users suppose to figure out if they will benefit
> from it?

If the complexity issue is addressed, then wouldn't all users who use
pg_upgrade in link mode benefit from this..?  Or are you thinking we
need to figure out which users really need to have pg_upgrade be faster
from those who don't?  The former would be 100% and the latter seems
extremely difficult to determine and not all that useful in the end- not
every user needs SELECT to be faster, but we sure want to make it faster
if we can do so reasonably.

I agree that we need to really consider if the additional complexity is
worth the performance improvement, but I don't think we really have a
gauge as to the complexity level or the ongoing maintenance effort
required, and without that we can't really say if it's too much or not.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Robert Haas]

On Fri, Dec 8, 2017 at 3:20 AM, Masahiko Sawada  wrote:
>> The first hunk in monitoring.sgml looks unnecessary.
>
> You meant the following hunk?
>
> diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
> index 8d461c8..7aa7981 100644
> --- a/doc/src/sgml/monitoring.sgml
> +++ b/doc/src/sgml/monitoring.sgml
> @@ -669,8 +669,8 @@ postgres   27093  0.0  0.0  30096  2752 ?
> Ss   11:34   0:00 postgres: ser
>Heavyweight locks, also known as lock manager locks or simply 
> locks,
>primarily protect SQL-visible objects such as tables.  However,
>they are also used to ensure mutual exclusion for certain internal
> -  operations such as relation extension.
> wait_event will
> -  identify the type of lock awaited.
> +  operations such as waiting for a transaction to finish.
> +  wait_event will identify the type of lock 
> awaited.
>   
>  
>  
>
> I think that since the extension locks are no longer a part of
> heavyweight locks we should change the explanation.

Yes, you are right.

>> +RelationExtensionLockWaiterCount(Relation relation)
>>
>> Hmm.  This is sort of problematic, because with then new design we
>> have no guarantee that the return value is actually accurate.  I don't
>> think that's a functional problem, but the optics aren't great.
>
> Yeah, with this patch we could overestimate it and then add extra
> blocks to the relation. Since the number of extra blocks is capped at
> 512 I think it would not become serious problem.

How about renaming it EstimateNumberOfExtensionLockWaiters?

>> +/* If force releasing, release all locks we're holding */
>> +if (force)
>> +held_relextlock.nLocks = 0;
>> +else
>> +held_relextlock.nLocks--;
>> +
>> +Assert(held_relextlock.nLocks >= 0);
>> +
>> +/* Return if we're still holding the lock even after computation */
>> +if (held_relextlock.nLocks > 0)
>> +return;
>>
>> I thought you were going to have the caller adjust nLocks?
>
> Yeah, I was supposed to change so but since we always release either
> one lock or all relext locks I thought it'd better to pass a bool
> rather than an int.

I don't see why you need to pass either one.  The caller can set
held_relextlock.nLocks either with -- or = 0, and then call
RelExtLockRelease() only if the resulting value is 0.

>> When I took a break from sitting at the computer, I realized that I
>> think this has a more serious problem: won't it permanently leak
>> reference counts if someone hits ^C or an error occurs while the lock
>> is held?  I think it will -- it probably needs to do cleanup at the
>> places where we do LWLockReleaseAll() that includes decrementing the
>> shared refcount if necessary, rather than doing cleanup at the places
>> we release heavyweight locks.
>> I might be wrong about the details here -- this is off the top of my head.
>
> Good catch. It can leak reference counts if someone hits ^C or an
> error occurs while waiting. Fixed in the latest patch. But since
> RelExtLockReleaseAll() is called even when such situations I think we
> don't need to change the place where releasing the all relext lock. We
> just moved it from heavyweight locks. Am I missing something?

Hmm, that might be an OK way to handle it.  I don't see a problem off
the top of my head.  It might be clearer to rename it to
RelExtLockCleanup() though, since it is not just releasing the lock
but also any wait count we hold.

+/* Must be greater than MAX_BACKENDS - which is 2^23-1, so we're fine. */
+#define RELEXT_WAIT_COUNT_MASK((uint32) ((1 << 24) - 1))

Let's drop the comment here and instead add a StaticAssertStmt() that
checks this.

I am slightly puzzled, though.  If I read this correctly, bits 0-23
are used for the waiter count, bit 24 is always 0, bit 25 indicates
the presence or absence of an exclusive lock, and bits 26+ are always
0.  That seems slightly odd.  Shouldn't we either use the highest
available bit for the locker (bit 31) or the lowest one (bit 24)?  The
former seems better, in case MAX_BACKENDS changes later.  We could
make RELEXT_WAIT_COUNT_MASK bigger too, just in case.

+/* Make a lock tag */
+tag.dbid = MyDatabaseId;
+tag.relid = relid;

What about shared relations?  I bet we need to use 0 in that case.
Otherwise, if backends in two different databases try to extend the
same shared relation at the same time, we'll (probably) fail to notice
that they conflict.

+ * To avoid unnecessary recomputations of the hash code, we try to do this
+ * just once per function, and then pass it around as needed.  we can
+ * extract the index number of RelExtLockArray.

This is just a copy-and-paste from lock.c, but actually we have a more
sophisticated scheme here.  I think you can just drop this comment
altogether, really.

+return (tag_hash((const void *) locktag, sizeof(RelExtLockTag))
+% 

Re: Speeding up pg_upgrade

[Stephen Frost]

Mark,

* Mark Dilger (hornschnor...@gmail.com) wrote:
> > On Dec 7, 2017, at 10:24 PM, Bruce Momjian  wrote:
> > I think the big problem with two-stage pg_upgrade is that the user steps
> > are more complex, so what percentage of users are going use the
> > two-stage method.  The bad news is that only a small percentage of users
> > who will benefit from it will use it, and some who will not benefit it
> > will use it.  Also, this is going to require significant server changes,
> > which have to be maintained.
> 
> In my fork of the project, back when I was tracking 9.5, I added an option
> to vacuum/analyze to make it behave a bit more like autovac, so that I could
> run
> 
>   ANALYZE CONDITIONALLY;
> 
> and it would only analyze those tables in the system which autovac would
> analyze.  In the grammar, CONDITIONALLY gets translated into a
> VacuumOption flag.  In vacuum (in src/backend/commands/vacuum.c), inside
> the "Loop to process each selected relation", if this flag is set, it checks 
> the
> PgStat_StatTabEntry for the table to determine whether to vacuum or analyze
> the table.
> 
> I think this extension would be helpful in the context of the current 
> conversation.
> In those cases where pg_upgrade was able to migrate the statistics to the
> new database, as long as it set the PgStat_StatTabEntry for each table where
> statistics were migrated, then the user would just have to execute a
> "VACUUM CONDITIONALLY" after upgrade, and the database would either
> do a lot of analyze work, a little analyze work, or no analyze work depending
> on which tables needed analyzing.
> 
> The main advantage here is that the user would always run this command
> after pg_upgrade, without having to think about whether pg_upgrade had
> migrated statistics or not.
> 
> If the community thinks this is useful, I could put together a patch.

This certainly sounds nice though I have to admit to being a bit
skeptical on the keyword selection, but perhaps blue really is the right
color for that bike shed.

One thing I'd wonder about is if that makes 'CONDITIONALLY' into a
reserved keyword, which wouldn't be ideal.  Perhaps a bit of a stretch
but 'ANALYZE ALL NEEDED' might avoid that?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Robert Haas]

On Fri, Dec 8, 2017 at 4:13 AM, Michael Paquier
 wrote:
> On Fri, Dec 8, 2017 at 5:38 PM, Masahiko Sawada  wrote:
>> After off-discussion with Fujii-san, I've updated the comment of why
>> we should disallow interrupts before setting/cleanup the session-level
>> lock. Please review it.
>
> +   /*
> +* Set session-level lock. If we allow interrupts before setting
> +* session-level lock, we could call callbacks with an inconsistent
> +* state. To avoid calling CHECK_FOR_INTERRUPTS by 
> LWLockReleaseClearVar
> +* which is called by WALInsertLockRelease before changing the backup
> +* state we change it while holding the WAL insert lock.
> +*/
> So you are just adding the reference to WALInsertLockRelease.. Instead
> of writing the function names for LWLocks, I would just write "To
> avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a
> LWLock" and be done with it. There is no point to list a full function
> dependency list, which could change in the future with static routines
> of lwlock.c.

I think it's actually good to be explicit here.  I looked at this
patch a bit last week and had great difficulty understanding how the
CHECK_FOR_INTERRUPTS() could happen.

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

Re: explain analyze output with parallel workers - question about meaning of information for explain.depesz.com

[Robert Haas]

On Fri, Dec 8, 2017 at 5:11 AM, Amit Kapila  wrote:
> The regression test added by patch needs cleanup at the end which I
> have added in the attached patch.

OK, so you've got a test case now, but Thomas independently submitted
a test case patch.  Which one is more awesome?  :-)

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

Re: no partition pruning when partitioning using array type

[Robert Haas]

On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote
 wrote:
> I noticed that if you partition using a array type column, partition
> pruning using constraint exclusion fails to work due to a minor problem.
>
> Example:
>
> create table p (a int[]) partition by list (a);
> create table p1 partition of p for values in ('{1}');
> create table p1 partition of p for values in ('{2, 3}', '{4, 5}');
>
> explain select a from p where a = '{1}';
> QUERY PLAN
> |-
>  Append  (cost=0.00..54.00 rows=14 width=32)
>->  Seq Scan on p1  (cost=0.00..27.00 rows=7 width=32)
>  Filter: (a = '{1}'::integer[])
>->  Seq Scan on p2  (cost=0.00..27.00 rows=7 width=32)
>  Filter: (a = '{1}'::integer[])
>
> explain select a from p where a = '{2, 3}';
> QUERY PLAN
> |-
>  Append  (cost=0.00..54.00 rows=14 width=32)
>->  Seq Scan on p1  (cost=0.00..27.00 rows=7 width=32)
>  Filter: (a = '{2,3}'::integer[])
>->  Seq Scan on p2  (cost=0.00..27.00 rows=7 width=32)
>  Filter: (a = '{2,3}'::integer[])
> (5 rows)
>
> In the case of array type partition key, make_partition_op_expr() will
> have to put a RelabelType node on top of the partition key Var, after
> having selected an = operator from the array_ops family.  The RelabelType
> causes operator_predicate_proof() to fail to consider predicate leftop and
> clause leftop as equal, because only one of them ends up having the
> RelabelType attached to it.
>
> As a simple measure, the attached patch teaches operator_predicate_proof()
> to strip RelabelType nodes from all the nodes it compares using equal().
> I also added a relevant test in partition_prune.sql.

I guess the question is whether that's guaranteed to be safe.  I spent
a little bit of time thinking about it and I don't see a problem.  The
function is careful to check that the opclasses and collations of the
OpExprs are compatible, and it is the behavior of the operator that is
in question here, not the column type, so your change seems OK to me.
But I hope somebody else will also study this, because this stuff is
fairly subtle and I would not like to be the one who breaks it.

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

Re: proposal: alternative psql commands quit and exit

[Robert Haas]

On Fri, Dec 8, 2017 at 8:57 AM, Daniel Vérité"  wrote:
> When looking at the most popular postgres questions on stackoverflow:
>
> https://stackoverflow.com/questions/tagged/postgresql?sort=votes
>
> the first one (most up-voted) happens to be:
>
> "How to exit from PostgreSQL command line utility: psql"
>
> now at 430k views and 1368 upvotes.

Wow, that's pretty crazy.  I was going to vote against this proposal,
but I think I might change my mind.  How can we say that this isn't a
problem for users given that data?  It's evidently not only *a*
problem, but arguably the biggest one.

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

Re: proposal: alternative psql commands quit and exit

[Robert Haas]

On Fri, Dec 8, 2017 at 2:07 PM, Tom Lane  wrote:
>> Wow, that's pretty crazy.  I was going to vote against this proposal,
>> but I think I might change my mind.  How can we say that this isn't a
>> problem for users given that data?  It's evidently not only *a*
>> problem, but arguably the biggest one.
>
> I actually agree that there's a problem there.  What I find pretty dubious
> is the claim that this patch will fix it.

There's no silver bullet for usability issues.

> If we could see our way to recognizing help/quit/exit on a line by
> themselves even when there's data in the query buffer, the argument
> that we've improved matters for novices would be *far* stronger.
> However, given that this is legal, fully-spec-compliant SQL:
>
> select a, b
> exit
> from mytable;
>
> I'm not sure how we could get away with that.  Would it pass muster to do
> that only when isatty(stdin)?  Other ideas?

What if we made it so that "exit" or "quit" bails out entirely when
not in a continuation line, and when entered on a continuation line,
provided isatty(stdin), prints some kind of message telling you that
you're in a continuation line?  For example, if you type "help" on a
continuation line by itself it says something like "use \? for help or
press control-C to abandon the command currently in the input buffer"
and if you type "exit" or "quit" on a a continuation line by itself it
says something like "use \q to quit or press control-C to abandon the
command currently in the input buffer".  Either thing will happen in
addition to, not instead of, adding the text to the current input
buffer.

That way, your valid SQL command will still work, but you'll get a
hint that you may want to choose a less-confusing place to put your
line breaks.  And if, as is more likely, you are a confused new user,
you will get a clue.

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

Typo in recent commit

[Robins Tharakan]

Hi,

Looks like a minor typo in the recent commit.

s/identify/identity/

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a2c6cf36608e10aa223fef49323b5feba344bfcf

-
robins | mobile

Re: Is it possible and worthy to optimize scanRTEForColumn()?

[Andres Freund]

On 2017-12-08 10:05:17 -0500, Tom Lane wrote:
> I'm not particularly concerned about it --- I've not seen profiles
> suggesting that that function is a big time sink.  Tables with very
> many columns tend to be inefficient for lots of reasons, and I rather
> doubt that this particular place is the first thing to hit if you
> want to make that better.

FWIW, I've definitely seen scanRTEForColumn() show up in profiles.  In
some of those cases the issue could be worked around by mostly using
prepared statements.  But it definitely can be painful, not that
surprising given the the complexity is essentially
O(#available-columns * #looked-up-columns).

However I don't think a microoptimization, even if it were correct, as
breaking earlier out of the loop would help meaningfully. We'd need a
different datastructure.

Greetings,

Andres Freund

Re: proposal: alternative psql commands quit and exit

[Robert Haas]

On Fri, Dec 8, 2017 at 10:28 AM, Szymon Lipiński  wrote:
> Well, if I have a new program I usually read some documentation. It really
> helps people to exit vim as well :)

Look, I love vim and use it constantly, but no reasonable person is
going to hold it up as a good example of user-friendly software.
According to Wikipedia, it was written in 1976 by Bill Joy, and
computers have come a long way in the last 40 years.  They are,
broadly, easier to use now.  For example, current versions of vim let
you move around using new-fangled arrow keys, as if computers were
supposed to cater to the lowest common denominator!  Real men (like
me) use hjkl to get around the screen, and look upon those who resort
to the arrow keys as Johnny-come-latelys.  Nevertheless, I can hardly
fault vim/vi's concession to modernity in this regard.

> Thinking this way for me psql's way is the intuitive one, because I know it.
> Should I kindly ask Oracle to change their programs because I rather want to
> use their software than reading their documentation?

If you can convince Oracle to add support for \q to sqlplus, I say go
for it.  Actually, what I'd like even better is if you could convince
them to add curses support, but I guess they would have done that 30
years ago if they were inclined to do it.

Really good software -- which sqlplus is not -- doesn't make it
necessary to read the documentation.  It helps you figure out how to
use it as you go.  When I fire up a new app on my iPhone, it generally
gives me a clue how to use it.  Sure, there's probably an app in the
Apple Store someplace that is absolutely unusable without reading the
documentation, but that's a bug, not a feature.   It's expected that
you'll have to read the documentation to figure out how to use the
advanced features of any complicated program, but that doesn't mean we
should make simple things complicated just to scare off users that
aren't sufficiently committed to the Cause.

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

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Robert Haas  writes:
> On Fri, Dec 8, 2017 at 2:07 PM, Tom Lane  wrote:
>> I'm not sure how we could get away with that.  Would it pass muster to do
>> that only when isatty(stdin)?  Other ideas?

> What if we made it so that "exit" or "quit" bails out entirely when
> not in a continuation line, and when entered on a continuation line,
> provided isatty(stdin), prints some kind of message telling you that
> you're in a continuation line?

Yeah, that might work.  As you say, if it only prints some chatter
(to stderr, I guess) without changing the semantics of the input,
it gets easier to defend.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[hvjunk]

On 08 Des. 2017, at 20:56 , Robert Haas  wrote:
> 
> On Fri, Dec 8, 2017 at 8:57 AM, Daniel Vérité"  
> wrote:
>> When looking at the most popular postgres questions on stackoverflow:
>> 
>> https://stackoverflow.com/questions/tagged/postgresql?sort=votes
>> 
>> the first one (most up-voted) happens to be:
>> 
>> "How to exit from PostgreSQL command line utility: psql"
>> 
>> now at 430k views and 1368 upvotes.
> 
> Wow, that's pretty crazy.  I was going to vote against this proposal,
> but I think I might change my mind.  How can we say that this isn't a
> problem for users given that data?  It's evidently not only *a*
> problem, but arguably the biggest one.

Sounds like the VI exit issue?
https://stackoverflow.com/questions/11828270/how-to-exit-the-vim-editor 


Having “made that ‘mistake’” once, by lots of users, does that imply it is a 
constant problem for those users?

Will, having the psql exit the same as msql/etc., make people rather use 
PostgreSQL than MySQL?

Is it really such a huge barrier to entry for users/sysadmins/developers? 
Most users/developers I coming from the MSSQL type world is looking for a GUI 
to manage their DB, and in that regard MySQL has argue-ably a “better” tool 
with the name Workbench compared to PGAdmin.

Re: Is it possible and worthy to optimize scanRTEForColumn()?

[Andres Freund]

Hi,

On 2017-12-08 14:41:14 -0500, Tom Lane wrote:
> Yeah, if someone were holding a gun on me and saying "make that particular
> function faster", I'd think about a hash table rather than scanning a
> list.  Perhaps a hash table with all the column names exposed by FROM,
> not one hash per RTE.

That sounds right.


> However, if you have a FROM that exposes a lot of column names, and
> then the query only looks up a few of them, you might come out behind
> by building a hash table :-(

Hm, I don't think that's that big of a deal - you don't need many
lookups to make a hashtable worthwhile if the alternative is exhaustive
scans through linked lists.  I'd be more concerned about the pretty
common case we're most of the time hitting now, where there's just a
handfull of columns selected from about as many available columns, the
additional allocations and such might show up.


> I'm still unconvinced that this is the first place to improve for
> wide tables, anyway.

I've run a few profiles with wide columns lately, and on the read-mostly
side without prepared statements this is very commonly the biggest entry
by a lot. Over 90% of the time in one of them.

If the queries select a large number of the underlying rows tupledesc
computations, and their syscache lookups, become the bottleneck. That's
partially what lead me to microoptimize syscache ~two months back.  The
real solution there is going to be to move tupledesc computations to the
planner, but that's a bigger piece of work than I can take on right now.

Greetings,

Andres Freund

Re: Partition pruning for Star Schema

[Mark Kirkwood]

On 04/12/17 17:20, Mark Kirkwood wrote:


On 04/12/17 16:08, Ashutosh Bapat wrote:


On Sun, Dec 3, 2017 at 5:56 AM, legrand legrand
 wrote:

Hello,

I have a typical star schema, having dimension tables "product", 
"calendar"

and "country" and a fact table "sales".
This fact table is partitionned by time (range by month) and country 
(list).


Will query like:

select product.name, calendar.month, sum(sales.net_price)
from sales
  inner join product on (product.id = sales.cust_id)
  inner join country on (country.id = sales.country_id)
  inner join calendar on (calendar.id = sales.calendar_id)
where
  country.name = 'HERE'
  and calendar.year = '2017'
group by product.name,calendar.month

be able to identify needed partitions ?


AFAIU partition pruning, it works only with the partition key columns.
So, if country.name and calendar.year are the partition keys partition
pruning would identify the needed partitions from those tables. But
planner doesn't know that calendar.year is somehow related to
calendar.id and then transfer that knowledge so that partitions of
sales can be identified.



If you can get your code to perform a star transformation on this type 
of query, then you might see some partition pruning.




Actually it won't - sorry. To get that to work, you would need to 
evaluate the additional subqueries to produce fixed values! The patch 
for 'runtime partition pruning' might be what you want tho.


Cheers

Mark

Re: [HACKERS] Re: Is anything preventing us from allowing write to foreign tables from standby?

[Alexander Korotkov]

On Wed, Nov 29, 2017 at 5:07 AM, Michael Paquier 
wrote:

> On Fri, Oct 27, 2017 at 4:44 PM, Robert Haas 
> wrote:
> > However, as Michael also points out, it's arguably wrong to allow a
> > nominally read-only transaction to write data regardless of whether it
> > works.  In the case of a standby it could be argued that your
> > transaction is only read-only because you had no other choice, but
> > nonetheless that's how it is marked.  I have a feeling that if we
> > extend the definition of "read only" to mean "sometimes allow writes",
> > we may regret it.
>
> I still have the same feeling. What I am sure of is that this patch is
> not the correct way to do things. So I am marking it as returned with
> feedback. This is not a rejection from my side, as I think that this
> feature could be useful in some cases, but its design needs way more
> thoughts.
>

Righ, my initial proposal was naive.  This discussion raised at least two
design issues.

1) It doesn't seem OK from the design point of view to allow read-only
transaction to write, even if it's write to foreign server.  One possible
solution is to distinguish three transaction types: real-only, write only
to foreign servers, read-write.
2) We have upcoming patch implementing atomic commit to multiple foreign
servers.  This atomic commit feature wouldn't work on standby, since it
writes to local tables.  We should detect foreign server need in real
read-write transaction for writing to foreign server, and prevent such
writes on standby.

I'll try to propose patch addressing these two issues to the next
commitfest.

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

Faster str to int conversion (was Table with large number of int columns, very slow COPY FROM)

[Andres Freund]

Hi,


On 2017-12-08 10:17:34 -0800, Andres Freund wrote:
> the strtoll is libc functionality triggered by pg_atoi(), something I've
> seen show up in numerous profiles. I think it's probably time to have
> our own optimized version of it rather than relying on libcs.

Attached is a hand-rolled version. After quickly hacking up one from
scratch, I noticed we already kind of have one for int64 (scanint8), so
I changed the structure of this one to be relatively similar.

It's currently using the overflow logic from [1], but that's not
fundamentally required, we could rely on fwrapv for this one too.

This one improves performance of the submitted workload from 1223.950ms
to 1020.640ms (best of three). The profile's shape changes quite
noticeably:

master:
+   15.38%  postgres  libc-2.25.so  [.] __GI_strtoll_l_internal
+   11.79%  postgres  postgres  [.] heap_fill_tuple
+8.00%  postgres  postgres  [.] CopyFrom
+7.40%  postgres  postgres  [.] CopyReadLine
+6.79%  postgres  postgres  [.] ExecConstraints
+6.68%  postgres  postgres  [.] NextCopyFromRawFields
+6.36%  postgres  postgres  [.] heap_compute_data_size
+6.02%  postgres  postgres  [.] pg_atoi
patch:
+   13.70%  postgres  postgres  [.] heap_fill_tuple
+   10.46%  postgres  postgres  [.] CopyFrom
+9.31%  postgres  postgres  [.] pg_strto32
+8.39%  postgres  postgres  [.] CopyReadLine
+7.88%  postgres  postgres  [.] ExecConstraints
+7.63%  postgres  postgres  [.] InputFunctionCall
+7.41%  postgres  postgres  [.] heap_compute_data_size
+7.21%  postgres  postgres  [.] pg_verify_mbstr
+5.49%  postgres  postgres  [.] NextCopyFromRawFields


This probably isn't going to resolve Alex's performance concerns
meaningfully, but seems quite worthwhile to do anyway.

We probably should have int8/16/64 version coded just as use the 32bit
version, but I decided to leave that out for now. Primarily interested
in comments.  Wonder a bit whether it's worth providing an 'errorOk'
mode like scanint8 does, but surveying its callers suggests we should
rather change them to not need it...

Greetings,

Andres Freund

[1] 
http://archives.postgresql.org/message-id/20171030112751.mukkriz2rur2qkxc%40alap3.anarazel.de
>From 98fbe53be0a3046f8ace687f846f91a0043deee8 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sun, 29 Oct 2017 22:13:54 -0700
Subject: [PATCH 1/3] Provide overflow safe integer math inline functions.

Author: Andres Freund, with some code stolen from Greg Stark
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 config/c-compiler.m4  |  22 
 configure |  33 ++
 configure.in  |   4 +
 src/include/common/int.h  | 229 ++
 src/include/pg_config.h.in|   3 +
 src/include/pg_config.h.win32 |   3 +
 6 files changed, 294 insertions(+)
 create mode 100644 src/include/common/int.h

diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 6dcc7906491..0d91e52a28f 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -296,6 +296,28 @@ fi])# PGAC_C_BUILTIN_CONSTANT_P
 
 
 
+# PGAC_C_BUILTIN_OP_OVERFLOW
+# -
+# Check if the C compiler understands __builtin_$op_overflow(),
+# and define HAVE__BUILTIN_OP_OVERFLOW if so.
+#
+# Check for the most complicated case, 64 bit multiplication, as a
+# proxy for all of the operations.
+AC_DEFUN([PGAC_C_BUILTIN_OP_OVERFLOW],
+[AC_CACHE_CHECK(for __builtin_mul_overflow, pgac_cv__builtin_op_overflow,
+[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],
+[PG_INT64_TYPE result;
+__builtin_mul_overflow((PG_INT64_TYPE) 1, (PG_INT64_TYPE) 2, );]
+)],
+[pgac_cv__builtin_op_overflow=yes],
+[pgac_cv__builtin_op_overflow=no])])
+if test x"$pgac_cv__builtin_op_overflow" = xyes ; then
+AC_DEFINE(HAVE__BUILTIN_OP_OVERFLOW, 1,
+  [Define to 1 if your compiler understands __builtin_$op_overflow.])
+fi])# PGAC_C_BUILTIN_OP_OVERFLOW
+
+
+
 # PGAC_C_BUILTIN_UNREACHABLE
 # --
 # Check if the C compiler understands __builtin_unreachable(),
diff --git a/configure b/configure
index 4ecd2e19224..f66899488cc 100755
--- a/configure
+++ b/configure
@@ -14467,6 +14467,39 @@ esac
 
 fi
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_mul_overflow" >&5
+$as_echo_n "checking for __builtin_mul_overflow... " >&6; }
+if ${pgac_cv__builtin_op_overflow+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+PG_INT64_TYPE result;
+__builtin_mul_overflow((PG_INT64_TYPE) 1, (PG_INT64_TYPE) 2, );
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv__builtin_op_overflow=yes
+else
+  pgac_cv__builtin_op_overflow=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext

Re: Partition-wise aggregation/grouping

[legrand legrand]

Thank you for the answer
This is a miss understanding of hash join behaviour on my side.

That means that there is at less on line read in facts_p2 part even if the
second table partition of the hash join operation is empty.

I will remember it now ;o)

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: pgsql: Prohibit identity columns on typed tables and partitions

[Michael Paquier]

On Sat, Dec 9, 2017 at 2:26 AM, Peter Eisentraut  wrote:
> Prohibit identity columns on typed tables and partitions
>
> Those cases currently crash and supporting them is more work then
> originally thought, so we'll just prohibit these scenarios for now.

+   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("identify columns are not
supported on partitions")));
Er... s/identify/identity/. My fault I guess.
-- 
Michael


identity-errmsg-typo.patch
Description: Binary data

Re: plpgsql fails to reinitialize record variables at block re-entry

[Pavel Stehule]

2017-12-09 7:24 GMT+01:00 Tom Lane :

> Consider
>
> regression=# do $$
> regression$# declare r record;
> regression$# begin
> regression$#   raise notice '%', r;
> regression$# end$$;
> ERROR:  record "r" is not assigned yet
> DETAIL:  The tuple structure of a not-yet-assigned record is indeterminate.
> CONTEXT:  SQL statement "SELECT r"
> PL/pgSQL function inline_code_block line 4 at RAISE
>
> Fine, you're supposed to assign something to the record before you use it.
> But look at this:
>
> regression=# do $$
> regression$# begin
> regression$# for i in 1..3 loop
> regression$#   declare r record;
> regression$#   begin
> regression$# if i = 1 then
> regression$#   r := row(i);
> regression$# end if;
> regression$# raise notice '%', r;
> regression$#   end;
> regression$# end loop;
> regression$# end$$;
> NOTICE:  (1)
> NOTICE:  (1)
> NOTICE:  (1)
> DO
>
> Surely that ought to have failed at the i=2 iteration.  There is
> code in plpgsql's exec_stmt_block() that tries to reinitialize
> PLPGSQL_DTYPE_REC variables, but it's never reached (as a quick
> look at coverage.postgresql.org will confirm), because what it
> scans is only the variables attached to the block by
> plpgsql_add_initdatums() --- and that function thinks it should
> only pay attention to PLPGSQL_DTYPE_VAR variables.
>
> The attached patch fixes this by teaching plpgsql_add_initdatums()
> to also list PLPGSQL_DTYPE_REC variables, though I failed to resist
> the temptation to make a couple of nearby cosmetic improvements.
>
> What I'm not sure about is whether to back-patch this.  The current
> behavior is indubitably not right, but we've had no field complaints,
> and it's not entirely far-fetched that some poor sod might have written
> code that depends on it working this way.  So maybe we should leave
> it alone in the back branches.  Thoughts?
>

you are correct

+1

Pavel


> regards, tom lane
>
>

Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding

[Michael Paquier]

On Sat, Dec 9, 2017 at 12:23 AM, Peter Eisentraut
 wrote:
> On 12/1/17 18:11, Michael Paquier wrote:
>> Cool. Thanks. For REL_10_STABLE, I would suggest the attached patch
>> then. This ensures that eSws is checked in the final message and that
>> the cbind-flag sent in the first message maps with the data of the
>> final message in the backend. I have checked with the following
>> configurations with a v10 backend:
>> - v11 libpq with SSL
>> - v11 libpq without SSL
>> - v10 libpq with SSL
>> - v10 libpq without SSL
>> And in all cases the connection is accepted as it should.
>
> Committed.

Thanks.
-- 
Michael

plpgsql fails to reinitialize record variables at block re-entry

[Tom Lane]

Consider

regression=# do $$
regression$# declare r record;
regression$# begin
regression$#   raise notice '%', r;
regression$# end$$;
ERROR:  record "r" is not assigned yet
DETAIL:  The tuple structure of a not-yet-assigned record is indeterminate.
CONTEXT:  SQL statement "SELECT r"
PL/pgSQL function inline_code_block line 4 at RAISE

Fine, you're supposed to assign something to the record before you use it.
But look at this:

regression=# do $$
regression$# begin
regression$# for i in 1..3 loop
regression$#   declare r record;
regression$#   begin
regression$# if i = 1 then
regression$#   r := row(i);
regression$# end if;
regression$# raise notice '%', r;
regression$#   end;
regression$# end loop;
regression$# end$$;
NOTICE:  (1)
NOTICE:  (1)
NOTICE:  (1)
DO

Surely that ought to have failed at the i=2 iteration.  There is
code in plpgsql's exec_stmt_block() that tries to reinitialize
PLPGSQL_DTYPE_REC variables, but it's never reached (as a quick
look at coverage.postgresql.org will confirm), because what it
scans is only the variables attached to the block by
plpgsql_add_initdatums() --- and that function thinks it should
only pay attention to PLPGSQL_DTYPE_VAR variables.

The attached patch fixes this by teaching plpgsql_add_initdatums()
to also list PLPGSQL_DTYPE_REC variables, though I failed to resist
the temptation to make a couple of nearby cosmetic improvements.

What I'm not sure about is whether to back-patch this.  The current
behavior is indubitably not right, but we've had no field complaints,
and it's not entirely far-fetched that some poor sod might have written
code that depends on it working this way.  So maybe we should leave
it alone in the back branches.  Thoughts?

regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 1300ea6..2d7844b 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -2384,14 +2384,14 @@ plpgsql_finish_datums(PLpgSQL_function *function)
 
 /* --
  * plpgsql_add_initdatums		Make an array of the datum numbers of
- *	all the simple VAR datums created since the last call
+ *	all the initializable datums created since the last call
  *	to this function.
  *
  * If varnos is NULL, we just forget any datum entries created since the
  * last call.
  *
- * This is used around a DECLARE section to create a list of the VARs
- * that have to be initialized at block entry.  Note that VARs can also
+ * This is used around a DECLARE section to create a list of the datums
+ * that have to be initialized at block entry.  Note that datums can also
  * be created elsewhere than DECLARE, eg by a FOR-loop, but it is then
  * the responsibility of special-purpose code to initialize them.
  * --
@@ -2402,11 +2402,16 @@ plpgsql_add_initdatums(int **varnos)
 	int			i;
 	int			n = 0;
 
+	/*
+	 * The set of dtypes recognized here must match what exec_stmt_block()
+	 * cares about (re)initializing at block entry.
+	 */
 	for (i = datums_last; i < plpgsql_nDatums; i++)
 	{
 		switch (plpgsql_Datums[i]->dtype)
 		{
 			case PLPGSQL_DTYPE_VAR:
+			case PLPGSQL_DTYPE_REC:
 n++;
 break;
 
@@ -2427,6 +2432,7 @@ plpgsql_add_initdatums(int **varnos)
 switch (plpgsql_Datums[i]->dtype)
 {
 	case PLPGSQL_DTYPE_VAR:
+	case PLPGSQL_DTYPE_REC:
 		(*varnos)[n++] = plpgsql_Datums[i]->dno;
 
 	default:
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 1959d6d..fa4d573 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1184,7 +1184,6 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 {
 	volatile int rc = -1;
 	int			i;
-	int			n;
 
 	/*
 	 * First initialize all variables declared in this block
@@ -1193,13 +1192,17 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 
 	for (i = 0; i < block->n_initvars; i++)
 	{
-		n = block->initvarnos[i];
+		int			n = block->initvarnos[i];
+		PLpgSQL_datum *datum = estate->datums[n];
 
-		switch (estate->datums[n]->dtype)
+		/*
+		 * The set of dtypes handled here must match plpgsql_add_initdatums().
+		 */
+		switch (datum->dtype)
 		{
 			case PLPGSQL_DTYPE_VAR:
 {
-	PLpgSQL_var *var = (PLpgSQL_var *) (estate->datums[n]);
+	PLpgSQL_var *var = (PLpgSQL_var *) datum;
 
 	/*
 	 * Free any old value, in case re-entering block, and
@@ -1241,7 +1244,7 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 
 			case PLPGSQL_DTYPE_REC:
 {
-	PLpgSQL_rec *rec = (PLpgSQL_rec *) (estate->datums[n]);
+	PLpgSQL_rec *rec = (PLpgSQL_rec *) datum;
 
 	if (rec->freetup)
 	{
@@ -1258,13 +1261,8 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 }
 break;
 
-			case PLPGSQL_DTYPE_RECFIELD:
-			case PLPGSQL_DTYPE_ARRAYELEM:
-break;
-
 			default:
-elog(ERROR, "unrecognized dtype: %d",
-	 estate->datums[n]->dtype);
+

Re: proposal: alternative psql commands quit and exit

[Rok Kralj]

Most popular questions on every topic are the simplest ones. Imagine a
parallel universe where this patch is contained in the codebase - I argue
this question about exiting the repl still is the top one, albeit with less
votes.

On Sat, Dec 9, 2017, 11:03 Michael Paquier 
wrote:

> On Sat, Dec 9, 2017 at 3:56 AM, Robert Haas  wrote:
> > On Fri, Dec 8, 2017 at 8:57 AM, Daniel Vérité" 
> wrote:
> >> When looking at the most popular postgres questions on stackoverflow:
> >>
> >> https://stackoverflow.com/questions/tagged/postgresql?sort=votes
> >>
> >> the first one (most up-voted) happens to be:
> >>
> >> "How to exit from PostgreSQL command line utility: psql"
> >>
> >> now at 430k views and 1368 upvotes.
> >
> > Wow, that's pretty crazy.  I was going to vote against this proposal,
> > but I think I might change my mind.  How can we say that this isn't a
> > problem for users given that data?  It's evidently not only *a*
> > problem, but arguably the biggest one.
>
> That's an impressive number, indeed. And an argument about potentially
> doing something.
> --
> Michael
>
>

Re: proposal: alternative psql commands quit and exit

[Michael Paquier]

On Sat, Dec 9, 2017 at 3:56 AM, Robert Haas  wrote:
> On Fri, Dec 8, 2017 at 8:57 AM, Daniel Vérité"  
> wrote:
>> When looking at the most popular postgres questions on stackoverflow:
>>
>> https://stackoverflow.com/questions/tagged/postgresql?sort=votes
>>
>> the first one (most up-voted) happens to be:
>>
>> "How to exit from PostgreSQL command line utility: psql"
>>
>> now at 430k views and 1368 upvotes.
>
> Wow, that's pretty crazy.  I was going to vote against this proposal,
> but I think I might change my mind.  How can we say that this isn't a
> problem for users given that data?  It's evidently not only *a*
> problem, but arguably the biggest one.

That's an impressive number, indeed. And an argument about potentially
doing something.
-- 
Michael

Re: Speeding up pg_upgrade

[Bruce Momjian]

On Fri, Dec  8, 2017 at 12:26:55PM -0500, Stephen Frost wrote:
> Bruce,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > I think the big problem with two-stage pg_upgrade is that the user steps
> > are more complex, so what percentage of users are going use the
> > two-stage method.  The bad news is that only a small percentage of users
> > who will benefit from it will use it, and some who will not benefit it
> > will use it.  Also, this is going to require significant server changes,
> > which have to be maintained.
> 
> This is where I think we need to be considering a higher-level tool that
> makes it easier to run pg_upgrade and which could handle these multiple
> stages.
> 
> > I think we need some statistics on how many users are going to benefit
> > from this, and how are users suppose to figure out if they will benefit
> > from it?
> 
> If the complexity issue is addressed, then wouldn't all users who use
> pg_upgrade in link mode benefit from this..?  Or are you thinking we

The instructions in the docs are going to be more complex.  We don't
have any planned way to make the two-stage approach the same complexity
as the single-stage approach.

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

Re: [HACKERS] Re: proposal - psql: possibility to specify sort for describe commands, when size is printed

[Pavel Stehule]

2017-12-08 22:49 GMT+01:00 Alexander Korotkov :

> Hi!
>
> On Mon, Nov 27, 2017 at 10:18 PM, Pavel Stehule 
> wrote:
>
>> I though about this design more time. I see following disadvantages
>>
>> 1. we are not able to check all possible variants of extended query. If
>> there will be some custom error, then we will raise pretty ugly error
>> messages,
>>
>
> Yes, that's an inevitable shortcoming.  psql is not backend and can't
> perform all the required checks on its side...
>
> 2. I don't thing so using "Size" as table size in human readable format
>> and "size" as table size in raw format is intuitive, but any change of
>> "Size" can introduce some (less probability compatibility issues),
>>
>
> Oh, this is surprisingly hard problem which probably have only imperative
> solution...
>
> 3. What queries will contains size calculations? It is not cheap -
>> requires AccessShareLock
>>
>
> Sorry, I didn't understand this point.  Yes, size calculation requires
> locking, but that is already true for \dt+ and \l+.  Why this is
> disadvantage of proposed approach?
>

Because you don't know the filter and sort clause (it can be generic), you
don't know, if you should to calculate or not the size. Or there should be
rule, so filter, order must be limited to displayed columns.

Regards

Pavel




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

Re: Add %r substitution for psql prompts to show recovery status

[Alexander Korotkov]

On Fri, Dec 8, 2017 at 3:54 AM, Tatsuo Ishii  wrote:

> > On Wed, Dec 6, 2017 at 9:19 PM, Ian Barwick 
> wrote:
> >> Note this substitution sends a "pg_is_in_recovery()" query to the server
> >> each time it's encountered; unless there's something I'm overlooking I
> >> think that's the only reliable way to determine current recovery status.
> >
> > That seems kinda painful.
> >
> > And what happens in an aborted transaction?
>
> Yeah. I think we need some from help backend for this. For example, a
> parameter status message can be used here.  If PostgreSQL moves to the
> recovery state or vice versa, PostgreSQL sends the parameter status
> message to frontend.
>

If we would use parameter status messages, then can we handle compatibility
correctly?  So that new psql can work with old backend without errors (and
without recovery status display, for sure), and old psql can work with new
backend without errors.

Because seeing recovery status in psql prompt is very neat.  But execution
of extra query each time doesn't seem like reasonable price for it...

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

Re: Postgres with pthread

[konstantin knizhnik]

On Dec 7, 2017, at 10:41 AM, Simon Riggs wrote:

>> But it is a theory. The main idea of this prototype was to prove or disprove
>> this expectation at practice.
> 
>> But please notice that it is very raw prototype. A lot of stuff is not
>> working yet.
> 
>> And supporting all of exited Postgres functionality requires
>> much more efforts (and even more efforts are needed for optimizing Postgres
>> for this architecture).
>> 
>> I just want to receive some feedback and know if community is interested in
>> any further work in this direction.
> 
> Looks good. You are right, it is a theory. If your prototype does
> actually show what we think it does then it is a good and interesting
> result.
> 
> I think we need careful analysis to show where these exact gains come
> from. The actual benefit is likely not evenly distributed across the
> list of possible benefits. Did they arise because you produced a
> stripped down version of Postgres? Or did they arise from using
> threads?
> 
> It would not be the first time a result shown in protoype did not show
> real gains on a completed project.
> 
> I might also read your results to show that connection concentrators
> would be a better area of work, since 100 connections perform better
> than 1000 in both cases, so why bother optimising for 1000 connections
> at all? In which case we should read the benefit at the 100
> connections line, where it shows the lower 28% gain, closer to the
> gain your colleague reported.
> 
> So I think we don't yet have enough to make a decision.


Concerning optimal number of connection: one of my intentions was to eliminate 
meed in external connection pool (pgbouncer).
In this case applications can use prepared statements which itself provides two 
times increase of performance.
I  believe that threads have smaller footprint than processes, to it is 
possible to spawn more threads and directly access them without intermediate 
layer with connection pooling.


I have performed experiments at more power server: 
144 virtual cores Intel(R) Xeon(R) CPU E7-8890 v3 @ 2.50GHz.

Here results of read-only queries are different: both pthreads and vanilla 
version shows almost the same speed both for 100 and 1000 connections: about 
1300k TPS
with prepared statement. So there is no performance degradation with increased 
number of connections and no larger difference between processes and threads.

But at read-write workload (pgbench -N) there is still significant advantage of 
pthreads version (kTPS):


Connections
Vanilla
pthreads
100
165
154
1000
85
118


For some reasons (which I do not know yet) multiprocess version of postgres is 
slightly faster for 100 connections, 
but degrades almost twice for 1000 connections, while degradation of 
multithreads version is not so large.

By the way, pthreads version make it possible to much easily check whats going 
on using gdb (manual "profiling") :


thread apply all bt
Thread 997 (Thread 0x7f6e08810700 (LWP 61345)):
#0  0x7f7e03263576 in do_futex_wait.constprop () from /lib64/libpthread.so.0
#1  0x7f7e03263668 in __new_sem_wait_slow.constprop.0 () from 
/lib64/libpthread.so.0
#2  0x00698552 in PGSemaphoreLock ()
#3  0x00702804 in LWLockAcquire ()
#4  0x004f9ac4 in XLogInsertRecord ()
#5  0x00503b97 in XLogInsert ()
#6  0x004bb0d1 in log_heap_clean ()
#7  0x004bd7c8 in heap_page_prune ()
#8  0x004bd9c1 in heap_page_prune_opt ()
---Type  to continue, or q  to quit---
#9  0x004c43d4 in index_fetch_heap ()
#10 0x004c4410 in index_getnext ()
#11 0x006037d2 in IndexNext ()
#12 0x005f3a80 in ExecScan ()
#13 0x00609eba in ExecModifyTable ()
#14 0x005ed6fa in standard_ExecutorRun ()
#15 0x00713622 in ProcessQuery ()
#16 0x00713885 in PortalRunMulti ()
#17 0x007143a5 in PortalRun ()
#18 0x00711cf1 in PostgresMain ()
#19 0x006a708b in backend_main_proc ()
#20 0x7f7e0325a36d in start_thread () from /lib64/libpthread.so.0
#21 0x7f7e02870b8f in clone () from /lib64/libc.so.6

Thread 996 (Thread 0x7f6e08891700 (LWP 61344)):
#0  0x7f7e03263576 in do_futex_wait.constprop () from /lib64/libpthread.so.0
#1  0x7f7e03263668 in __new_sem_wait_slow.constprop.0 () from 
/lib64/libpthread.so.0
#2  0x00698552 in PGSemaphoreLock ()
#3  0x00702804 in LWLockAcquire ()
#4  0x004bc862 in RelationGetBufferForTuple ()
#5  0x004b60db in heap_insert ()
#6  0x0060ad3b in ExecModifyTable ()
#7  0x005ed6fa in standard_ExecutorRun ()
#8  0x00713622 in ProcessQuery ()
#9  0x00713885 in PortalRunMulti ()
#10 0x007143a5 in PortalRun ()
#11 0x00711cf1 in PostgresMain ()
#12 0x006a708b in backend_main_proc ()
#13 0x7f7e0325a36d in start_thread () from /lib64/libpthread.so.0
#14 0x7f7e02870b8f in clone () from /lib64/libc.so.6

Thread 995 (Thread 0x7f6e08912700 (LWP 61343)):

Re: Partition pruning for Star Schema

[legrand legrand]

Thank You !

I will monitor this 'runtime partition pruning' patch.

This will be better than using Partitioned DIM tables "Partion wise joined"
with a multi level partitioned FACT table ;o)

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Postgres with pthread

[Alexander Korotkov]

On Sat, Dec 9, 2017 at 1:09 AM, konstantin knizhnik <
k.knizh...@postgrespro.ru> wrote:

> I am not going to show stack traces of all 1000 threads.
> But you may notice that proc array lock really seems be be a bottleneck.
>

Yes, proc array lock easily becomes a bottleneck on multicore machine with
large number of connections.  Related to this, another patch helping to
large number of connections is CSN.  When our snapshot model was invented,
xip was just array of few elements, and that cause no problem.  Now, we're
considering threads to help us handling thousands of connections.  Snapshot
with thousands of xips looks ridiculous.  Collecting such a large snapshot
could be more expensive than single index lookup.

These two patches threads and CSN are both complicated and require hard
work during multiple release cycles to get committed.  But I really hope
that their cumulative effect can dramatically improve situation on high
number of connections.  There are already some promising benchmarks in CSN
thread.  I wonder if we already can do some cumulative benchmarks of
threads + CSN?

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

Re: CUBE seems a bit confused about ORDER BY

[Alexander Korotkov]

Hi!

On Fri, Dec 8, 2017 at 11:21 AM, Andrey Borodin 
wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
>
> Hi! I've reviewed the patch.
> Patch works as expected, documented and tested.
>

Thank you for reviewing this.


> The code does not use LL_COORD and UR_COORD used all around. But the code
> still is easily readable.
>
> LL_COORD and UR_COORD are always combined with Min or Max function, so LL
> (Lower Left) and UR (Upper Right) are somewhat irrelevant names...
> BTW if we swap implementation of these macro-funcs and fix cube_out,
> almost all tests pass again. This is evidence of good compatibility and
> shows that compatibility overhead is added everywhere.
>

Yes, the thing is that we change behavior of existing ~> operator.  In
general, this is not good idea because it could affect existing users whose
already use this operator.  Typically in such situation, we could leave
existing operator as is, and invent new operator with new behavior.
However, in this particular case, I think there are reasons to make an
exception to the rules.  The reasons are following:
1) The ~> operator was designed especially for knn gist.
2) Knn gist support for current behavior is broken by design and can't be
fixed.  Most we can do to fix existing ~> operator behavior as is to drop
knn gist support.  But then ~> operator would be left useless.
3) It doesn't seems that ~> operator have many users yet, because an error
wasn't reported during whole release cycle.

I hope these reasons justify altering behavior of existing operator as an
exception to the rules.  Another question is whether we should backpatch
it.  But I think we could leave this decision to committer.

I think that this patch is ready for committer.
>

Good.

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

Re: explain analyze output with parallel workers - question about meaning of information for explain.depesz.com

[Robert Haas]

On Fri, Dec 8, 2017 at 5:11 AM, Amit Kapila  wrote:
>> Okay, I have adjusted the patch accordingly.  I have also added a
>> regression test which should produce the same result across different
>> runs, see if that looks okay to you, then it is better to add such a
>> test as well.
>
> The regression test added by patch needs cleanup at the end which I
> have added in the attached patch.

Hmm.  If we're going this way, then shouldn't we revert the changes
commit 2c09a5c12a66087218c7f8cba269cd3de51b9b82 made to
ExecParallelRetrieveInstrumentation?  If that function is only ever
called once, then there's no point doing InstrInit + InstrAgg node, or
checking whether worker_instrument is already initialized.  We can
just palloc + memcpy as the code did previously.

I think.

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

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Dec 6, 2017 at 10:07 AM, Ildus Kurbangaliev
 wrote:
> On Fri, 1 Dec 2017 21:47:43 +0100
> Tomas Vondra  wrote:
>> +1 to do the rewrite, just like for other similar ALTER TABLE commands
>
> Ok. What about the following syntax:
>
> ALTER COLUMN DROP COMPRESSION - removes compression from the column
> with the rewrite and removes related compression options, so the user
> can drop compression method.
>
> ALTER COLUMN SET COMPRESSION NONE for the cases when
> the users want to just disable compression for future tuples. After
> that they can keep compressed tuples, or in the case when they have a
> large table they can decompress tuples partially using e.g. UPDATE,
> and then use ALTER COLUMN DROP COMPRESSION which will be much faster
> then.
>
> ALTER COLUMN SET COMPRESSION  WITH  will change
> compression for new tuples but will not touch old ones. If the users
> want the recompression they can use DROP/SET COMPRESSION combination.
>
> I don't think that SET COMPRESSION with the rewrite of the whole table
> will be useful enough on any somewhat big tables and same time big
> tables is where the user needs compression the most.
>
> I understand that ALTER with the rewrite sounds logical and much easier
> to implement (and it doesn't require Oids in tuples), but it could be
> unusable.

The problem with this is that old compression methods can still be
floating around in the table even after you have done SET COMPRESSION
to something else.  The table still needs to have a dependency on the
old compression method, because otherwise you might think it's safe to
drop the old one when it really is not.  Furthermore, if you do a
pg_upgrade, you've got to preserve that dependency, which means it
would have to show up in a pg_dump --binary-upgrade someplace.  It's
not obvious how any of that would work with this syntax.

Maybe a better idea is ALTER COLUMN SET COMPRESSION x1, x2, x3 ...
meaning that x1 is the default for new tuples but x2, x3, etc. are
still allowed if present.  If you issue a command that only adds
things to the list, no table rewrite happens, but if you remove
anything, then it does.

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

Re: [HACKERS] postgres_fdw bug in 9.6

[Robert Haas]

On Sun, Dec 3, 2017 at 2:56 PM, Tom Lane  wrote:
> Not sure where that leaves us for the patch at hand.  It feels to me
> like it's a temporary band-aid for code that we want to get rid of
> in the not-too-long run.  As such, it'd be okay if it were smaller,
> but it seems bigger and more invasive than I could wish for a band-aid.

Well, the bug report is a year old today, so we should try to do
something.  The patch needs a rebase, but reading through it, I'm not
convinced that it's particularly risky.  I mean, it's going to break
FDWs that are calling GetExistingLocalJoinPath(), but there are
probably very few third-party FDWs that do that.  Any that do are
precisely the ones that need this fix, so I think there's a good
chance they'll forgive of us for requiring them to make a code change.
I think we can contain the risk of anything else getting broken to an
acceptable level because there's not really very much other code
changing.  The changes to joinpath.c need to be carefully audited to
make sure that they are not changing the semantics, but that seems
like it shouldn't take more than an hour of code-reading to check.
The new fields in JoinPathExtraData can be moved to the end of the
struct to minimize ABI breakage.  I don't see what else there is that
could break apart from the EPQ rechecks themselves, and if that
weren't broken already, this patch wouldn't exist.

Now, if you're still super-concerned about this breaking something, we
could commit it only to master, where it will have 9 months to bake
before it gets released.  I think that's overly conservative, but I
think it's still better than waiting for the rewrite you'd like to see
happen.  We don't know when or if anyone is going to undertake that,
and if we wait, we may easing release a v11 that's got the same defect
as v9.6 and now v10.  And I don't see that we lose much by committing
this now even if that rewrite does happen in time for v11.  Ripping
out CreateLocalJoinPath() won't be any harder than ripping out
GetExistingLocalJoinPath().

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