Re: [HACKERS] Hash Indexes

[Amit Kapila]

On Fri, Jun 24, 2016 at 2:38 PM, Mithun Cy  wrote:
> On Thu, Jun 16, 2016 at 12:58 PM, Amit Kapila 
> wrote:
>
> I have a question regarding code changes in _hash_first.
>
> +/*
> + * Conditionally get the lock on primary bucket page for search
> while
> +* holding lock on meta page. If we have to wait, then release the
> meta
> + * page lock and retry it in a hard way.
> + */
> +bucket = _hash_hashkey2bucket(hashkey,
> +
> metap->hashm_maxbucket,
> +
> metap->hashm_highmask,
> +
> metap->hashm_lowmask);
> +
> +blkno = BUCKET_TO_BLKNO(metap, bucket);
> +
> +/* Fetch the primary bucket page for the bucket */
> +buf = ReadBuffer(rel, blkno);
> +if (!ConditionalLockBufferShared(buf))
>
> Here we try to take lock on bucket page but I think if successful we do not
> recheck whether any split happened before taking lock. Is this not necessary
> now?
>

Yes, now that is not needed, because we are doing that by holding the
read lock on metapage.   Split happens by having a write lock on
metapage. The basic idea of this optimization is that if we get the
lock immediately, then do so by holding metapage lock, else if we
decide to wait for getting a lock on bucket page then we still
fallback to previous kind of mechanism.

> Also  below "if" is always true as we enter here only when outer "if
> (retry)" is true.
> +if (retry)
> +{
> +if (oldblkno == blkno)
> +break;
> +_hash_relbuf(rel, buf);
> +}
>

Good catch, I think we don't need this retry check now.  We do need
similar change in _hash_doinsert().



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


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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 21:34, Tom Lane  wrote:


>
> TBH, this looks more like a compiler bug than anything else.


I tend to agree. Especially since valgrind has no complaints on x64 linux,
and neither does DrMemory for 32-bit builds with the same toolchain on the
same Windows and same SDK.

I don't see any particular reason we can't proceed with 9.6beta2 and build
x64 Pg with MS VS 2015. There's no evidence turning up of a Pg bug here,
and compiling with a different toolchain gets us working binaries for the
target platform in question.


> It would be worth recompiling at -O0, or whatever the local equivalent
> of that is, to see if (1) the crash goes away or (2) the debugger's
> printouts get any more reliable


Yeah, it probably is. I'll see if I can find time this w/e.


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

Re: [HACKERS] Memory leak in Pl/Python

[Andrey Zhidenkov]

For test I wrote script in Python, which calls a test function via psycopg2:

#!/usr/bin/env python2

import psycopg2

conn = psycopg2.connect('xxx')

cursor = conn.cursor()

cursor.execute('set application_name to \'TEST\'')

for i in range(1, 100):
cursor.execute('select test()')
conn.commit()


I see memory consumption in htop and pg_activity tools.

On Sat, Jun 25, 2016 at 2:00 AM, David G. Johnston
 wrote:
> On Fri, Jun 24, 2016 at 6:41 PM, Andrey Zhidenkov
>  wrote:
>>
>> For example, when I call this procedure
>> many times,
>
>
> Call how?  Specifically, how are you handling transactions in the calling
> client?  And what/how are you measuring memory consumption?
>
> David J.
>



-- 
Andrey Zhidenkov / Database developer
+79265617190/ andrey.zhiden...@gmail.com




This e-mail message may contain confidential or legally privileged
information and is intended only for the use of the intended
recipient(s). Any unauthorized disclosure, dissemination,
distribution, copying or the taking of any action in reliance on the
information herein is prohibited. E-mails are not secure and cannot be
guaranteed to be error free as they can be intercepted, amended, or
contain viruses. Anyone who communicates with us by e-mail is deemed
to have accepted these risks. Company Name is not responsible for
errors or omissions in this message and denies any responsibility for
any damage arising from the use of e-mail. Any opinion and other
statement contained in this message and any attachment are solely
those of the author and do not necessarily represent those of the
company.


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

Re: [HACKERS] Improving executor performance

[Peter Geoghegan]

On Fri, Jun 24, 2016 at 4:29 PM, Andres Freund  wrote:
> As a motivation, here's a somewhat juicy example of the benefits that
> can be gained (disabled parallelism, results vary too much):
> SELECT l_linestatus, SUM(l_quantity) FROM lineitem GROUP BY 1 ORDER BY 2 DESC 
> LIMIT 10 ;
> non-optimized: 9075.835 ms
> optimized: 5194.024 ms
>
> and that's far from doing all the possible work for that query;
> especially the batching is far from optimal.

That's pretty great. Let me first just say that I think that your
broadly have the right idea here, and that it will likely be a big
area to work on in the years ahead. This may become a big, general
area with many sub-projects, kind of like parallelism. Also, I think
it's very much complementary to parallelism.

> So far I've mostly looked at performance for tpc-h, not because it's a
> particularly good workload, but because it's available.  If somebody has
> good suggestions for an analytics heavy workload...

I think that it's the only thing available that isn't a pain to setup.

> My observations about the performance bottlenecks I found, and partially
> addressed, are in rough order of importance (there's interdependence
> between most of them):
>
> (List of various bottlenecks)

> I'm planning to start individual subthreads for some of these, with
> in-detail discussions and/or patches. But I wanted to present this on a
> higher level once, before spending even more time.

Good call.

> Have I missed concrete issues others noticed? Missed significant
> improvements (please, please, only realistic stuff)?

I'll add one: We should have a way to make routines like
heap_copytuple() "allocate" into the caller's own batch memory. We may
be able to come up with a reasonably generic abstraction that
minimizes the special case handling of things like exhausting caller's
batch allocation. Failure to do something like this wastes an awful
lot of memory, but more importantly causes a lot of unnecessary cache
misses in tuplestore.c, and even with the 9.6 work in tuplesort.c.

Someone should also work on palloc() fragmentation, but I tend to
think that that's a less efficient way of improving matters. I'd leave
that until later. Having an excessive number of retail palloc() calls
for cases where the caller really does process tuples in a batch
fashion is inherently inefficient. I'm all for the simplicity of the
memory context abstraction, but ISTM that a little specialization gets
you very far.

> Unfortunately these items are heavily interdependent. For some queries
> you'll need issue n) addressed before n+2) shows a benefit, for some
> it's the other way, and such.  Given the size of the overall task, the
> only way I can see this being reasonably addressable, is trying to get
> smaller steps in individually, even if not a huge win on their own. And
> then focus on the the larger architectural changes (primarily 3))
> issues.

I could not agree more. This should be strongly emphasized.

I have parallel CREATE INDEX working pretty well now. I don't think I
could have done that without the 9.6 work on external sorting's cache
efficiency, so in some sense that earlier work has enabled parallelism
in a non-obvious way. I think that abbreviated keys will prove really
important for getting the best out of parallel sort (and that a cost
model will have to consider stuff like leading attribute cardinality
-- in other words, it must consider CPU cache efficiency). I also
suspect that solving the problems with heap_deform_tuple() first would
make my pending parallel CREATE INDEX patch look a lot better relative
to a serial CREATE INDEX. That's the kind of intuitive speculation
that's really hard to prove. It may be that it doesn't matter there,
because working on fixing that yields obvious benefits. But, as you
suggest, we should consider that that might not always be true. That's
really tricky, and has historically been the kind of thing we've
managed very badly.

-- 
Peter Geoghegan


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

Re: [HACKERS] Protocol buffer support for Postgres

[Flavius Anton]

On Fri, Jun 24, 2016 at 11:35 AM, Álvaro Hernández Tortosa
 wrote:
>
>
> On 24/06/16 14:23, Flavius Anton wrote:
>>
>> On Thu, Jun 23, 2016 at 2:54 PM, Flavius Anton 
>> wrote:
>>>
>>> On Thu, Jun 23, 2016 at 1:50 PM, Greg Stark  wrote:

 On Thu, Jun 23, 2016 at 8:15 PM, Flavius Anton 
 wrote:
>
> I'd love to talk more about this.

 I thought quite a bit about this a few years ago but never really
 picked up it to work on.
>>
>> Any other thoughts on this? My guess is that it might be an important
>> addition to Postgres that can attract even more users, but I am not
>> sure if there's enough interest from the community. If I want to pick
>> this task, how should I move forward? Do I need to write a design
>> document or similar or should I come up with a patch that implements a
>> draft prototype? I am new to this community and don't know the code
>> yet, so I'd appreciate some guidance from an older, more experienced
>> member.
>>
>>
>
> Other than protobuf, there are also other serialization formats that
> might be worth considering. Not too long ago I suggested working
> specifically on serialization formas for the json/jsonb types:
> https://www.postgresql.org/message-id/56CB8A62.40100%408kdata.com I believe
> this effort is on the same boat.

Sure, there are a bunch of these, some of the most popular being:
* Cap'n Proto
* Flatbuffers
* Thrift

A longer list is already made here[1].

Meanwhile, I came across another interesting idea. What if, for
starters, we don't introduce a completely new serialization format,
like Protocol Buffers, but rather make the JSON support more stronger
and interesting. What I am thinking is to have a JSON schema
(optionally) associated with a JSON column. In this way, you don't
have to store the keys on disk anymore and also you'd have your
database check for JSON validity at INSERT time. I think there are two
big advantages here. What do you think about this one?

--
Flavius

[1] https://en.wikipedia.org/wiki/Comparison_of_data_serialization_formats


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

Re: [HACKERS] Improving executor performance

[Andres Freund]

Hi,


On 2016-06-24 16:29:53 -0700, Andres Freund wrote:
> Comments so far?  Different suggestions on how to get improvements
> around this merged?

Oh, and if somebody is interested on collaborating on some of
these... This is far too big for me to tackle alone.

Andres


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

[HACKERS] Improving executor performance

[Andres Freund]

Hi,

at pgcon, in [1], and various other threads and conferences we talked
about our executor performance needing improvements to perform well in
!OLTP workloads, and how we can get there.

I've played with various techniques, on and off over the last few
weeks. Including trying to make slot_deform_tuple et al. faster, batched
execution for a few node types (SeqScan, hashed Agg), avoiding linked
lists in executor datastructures, and smaller micro optimizations.

As a motivation, here's a somewhat juicy example of the benefits that
can be gained (disabled parallelism, results vary too much):
SELECT l_linestatus, SUM(l_quantity) FROM lineitem GROUP BY 1 ORDER BY 2 DESC 
LIMIT 10 ;
non-optimized: 9075.835 ms
optimized: 5194.024 ms

and that's far from doing all the possible work for that query;
especially the batching is far from optimal.


So far I've mostly looked at performance for tpc-h, not because it's a
particularly good workload, but because it's available.  If somebody has
good suggestions for an analytics heavy workload...


My observations about the performance bottlenecks I found, and partially
addressed, are in rough order of importance (there's interdependence
between most of them):

1) Cache misses cost us a lot, doing more predictable accesses can make
   a huge difference. Without addressing that, many other bottlenecks
   don't matter all that much.  I see *significant* performance
   improvements for large seqscans (when the results are used) simply
   memcpy()'ing the current target block.

   This partially is an intrinsic problem of analyzing a lot of data,
   and partially because our access patterns are bad.


2) Baring 1) tuple deforming is the biggest bottleneck in nearly all
   queries I looked at. There's various places we trigger deforming,
   most ending in either slot_deform_tuple(), heap_getattr(),
   heap_deform_tuple().

   This can be optimized significantly, but even after that it's still a
   major issue.

   Part of the problem is that we sometimes don't know how many elements
   to access (especially in index and hashing related code), the other
   part that we're deforming a lot of columns that we'll never need,
   just because we need a subsequent one.

   The other part is that our tuple format requires a number of
   relatively expensive operations to access data (e.g. alignment
   computations, checking the null bitmap).


3) Our 1-by-1 tuple flow in the executor has two major issues:

   The biggest is that we perform a *lot* of repetitive work for each
   processed tuple. E.g. looking at nodeAgg.c's agg_fill_hash_table(),
   for each single tuple we execute ExecProcNode(), ExecSeqScan(),
   heap_getnext(), lookup_hash_entry(), LookupTupleHashEntry(), ... and
   many more. Each of these has a noticeable per-call state setup.
   When executing on batches of tuples, a lot of that setup work can be
   done once per batch, instead of once per tuple. Part of that the
   compiler can do automatically, part of that has to be done by hand.

   Also very significantly, due to the amount of code executed, there's
   barely any working branch prediction, leading to massive amounts of
   pipeline stalls and instruction cache misses.

   There's also the fact that some future optimizations around using
   SIMD are easier when looking at batches of tuples, but I'm not
   planning to work on that.


4) Various missing micro optimizations have to be performed, for more
   architectural issues to become visible. E.g. [2] causes such bad
   slowdowns in hash-agg workloads, that other bottlenecks are hidden.


5) Per-tuple heap_getnext() makes it hard to fix 3), and has similar
   issues. Using a per-block heap_getmany() that fills a batch at once
   is noticeably faster (and more convenient in a batched world anyway)


6) The use of linked lists adds noticeable #instructions overhead and
   branch misses. Just replacing {FuncExprState,BoolExprState}->args
   with an array gives a ~10%ish boost for queries with a bunch of
   quals.  Another example that appears to hurts noticeably, but which
   I've not experimentally fixed, is AggState->hash_needed.

   To a large degree these seem fairly easily fixable; it's kinda boring
   work, but not really hard.


I'm planning to start individual subthreads for some of these, with
in-detail discussions and/or patches. But I wanted to present this on a
higher level once, before spending even more time.


Have I missed concrete issues others noticed? Missed significant
improvements (please, please, only realistic stuff)?


Unfortunately these items are heavily interdependent. For some queries
you'll need issue n) addressed before n+2) shows a benefit, for some
it's the other way, and such.  Given the size of the overall task, the
only way I can see this being reasonably addressable, is trying to get
smaller steps in individually, even if not a huge win on their own. And
then focus on the the larger architectural changes (primarily 3))

Re: [HACKERS] Memory leak in Pl/Python

[David G. Johnston]

On Fri, Jun 24, 2016 at 6:41 PM, Andrey Zhidenkov <
andrey.zhiden...@gmail.com> wrote:

> For example, when I call this procedure
> many times,


​Call how?  Specifically, how are you handling transactions in the calling
client?  And what/how are you measuring memory consumption?

​David J.
​

Re: [HACKERS] MultiXactId error after upgrade to 9.3.4

[Alvaro Herrera]

After some further testing, I noticed a case that wasn't handled in
heap_update, which I also fixed.  I reworded some comments here and
there, and pushed to all branches.

Further testing and analysis is welcome.

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


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

Re: [HACKERS] Bug in to_timestamp().

[Joshua D. Drake]

On 06/24/2016 02:16 PM, Tom Lane wrote:

Robert Haas  writes:

On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford
 wrote:

To me, 2016-02-30 is an invalid date that should generate an error.



I don't particularly disagree with that, but on the other hand, as
mentioned earlier, to_timestamp() is here for Oracle compatibility,
and if it doesn't do what Oracle's function does, then (1) it's not
useful for people migrating from Oracle and (2) we're making up the
behavior out of whole cloth.  I think things that we invent ourselves
should reject stuff like this, but in a compatibility function we
might want to, say, have compatibility.


Agreed, mostly, but ... how far are we prepared to go on that?


We don't at all. Our goal has never been Oracle compatibility. Yes, we 
have "made allowances" but we aren't in a position that requires that 
anymore.


Let's just do it right.

Sincerely,

JD

/me speaking as someone who handles many, many migrations, none of which 
have ever said, "do we have Oracle compatibility available".


--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

[HACKERS] Memory leak in Pl/Python

[Andrey Zhidenkov]

I have postgresql 9.4.8 on my server and I've noticed always growing
memory when I use plpython. I've made some tests and find a few
situations, when memory leaks. For example, when I call this procedure
many times, I can see an always growing memory:

create or replace
function test() returns bigint as $$

plpy.execute("insert into test(test) values ('test')")

return 1

$$ language plpythonu;

Interestingly, when I use not-modifying data query ('select 1') as
argument of execute(), a leak stops.

How can I fix/avoid this?


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

Re: [HACKERS] Bug in to_timestamp().

[Gavin Flower]

On 25/06/16 08:33, Robert Haas wrote:

On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford
 wrote:

My observation has been that the PostgreSQL development group aims for
correctness and the elimination of surprising results. This was part of the
reason to eliminate a number of automatic casts to dates in earlier
versions.

To me, 2016-02-30 is an invalid date that should generate an error.
Automatically and silently changing it to be 2016-03-01 strikes me as a
behavior I'd expect from a certain other open-source database, not
PostgreSQL.

I don't particularly disagree with that, but on the other hand, as
mentioned earlier, to_timestamp() is here for Oracle compatibility,
and if it doesn't do what Oracle's function does, then (1) it's not
useful for people migrating from Oracle and (2) we're making up the
behavior out of whole cloth.  I think things that we invent ourselves
should reject stuff like this, but in a compatibility function we
might want to, say, have compatibility.


How about a to_timestamp_strict(), in addition?

Its very existence will help people (who bother to read the 
documentation) to look more closely at the differences between the 
definitions, and allow them to choose the most appropriate for their use 
case.



Cheers,
Gavin



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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Fri, Jun 24, 2016 at 2:18 PM, Tom Lane  wrote:
> Uh, why?  It's not a large amount of code and it seems like removing
> it puts a fair-size hole in the symmetry of tuplesort's capabilities.

It's not a small amount of code either.

Removing the code clarifies the division of labor between COPYTUP()
routines in general, their callers (tuplesort_putheaptuple() and
tuplesort_puttupleslot() -- which are also puttuple_common() callers),
and routines that are similar to those caller routines (in that they
at least call puttuple_common()) that do not call COPYTUP()
(tuplesort_putdatum(), and now tuplesort_putindextuplevalues()).

I believe that this has value. All the extra boilerplate code misleads.

-- 
Peter Geoghegan


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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Tom Lane]

Peter Geoghegan  writes:
> On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane  wrote:
>> I think this may be premature in view of bug #14210.  Even if we
>> don't reinstate use of this function to fix that, I'm not really
>> convinced we want to get rid of it; it seems likely to me that
>> we might want it again.

> You pushed a fix for bug #14210 that seems to not weaken the case for
> this at all. Where do you stand on this now? I think that leaving
> things as-is is confusing.

Uh, why?  It's not a large amount of code and it seems like removing
it puts a fair-size hole in the symmetry of tuplesort's capabilities.

regards, tom lane


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

Re: [HACKERS] Bug in to_timestamp().

[Tom Lane]

Robert Haas  writes:
> On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford
>  wrote:
>> To me, 2016-02-30 is an invalid date that should generate an error.

> I don't particularly disagree with that, but on the other hand, as
> mentioned earlier, to_timestamp() is here for Oracle compatibility,
> and if it doesn't do what Oracle's function does, then (1) it's not
> useful for people migrating from Oracle and (2) we're making up the
> behavior out of whole cloth.  I think things that we invent ourselves
> should reject stuff like this, but in a compatibility function we
> might want to, say, have compatibility.

Agreed, mostly, but ... how far are we prepared to go on that?  The one
thing I know about that is different from Oracle and is not something that
most people would consider clearly wrong is the behavior of the FM prefix.
We think it's a prefix that modifies only the next format code; they think
it's a toggle.  If we make that act like Oracle, we will silently break an
awful lot of applications, and there will be *no way* to write code that
is correct under both interpretations.  (And no, I do not want to hear
"let's fix it with a GUC".)  So I'm afraid we're between a rock and a hard
place on that one --- but if we let that stand, the argument that Oracle's
to_timestamp should be treated as right by definition loses a lot of air.

regards, tom lane


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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane  wrote:
> I think this may be premature in view of bug #14210.  Even if we
> don't reinstate use of this function to fix that, I'm not really
> convinced we want to get rid of it; it seems likely to me that
> we might want it again.

You pushed a fix for bug #14210 that seems to not weaken the case for
this at all. Where do you stand on this now? I think that leaving
things as-is is confusing.

Maybe the new copytup_index() comments should indicate why only a
defensive stub implementation is needed in practice. I'm certainly not
opposed to that.

-- 
Peter Geoghegan


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

Re: [HACKERS] Bug in to_timestamp().

[Robert Haas]

On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford
 wrote:
> My observation has been that the PostgreSQL development group aims for
> correctness and the elimination of surprising results. This was part of the
> reason to eliminate a number of automatic casts to dates in earlier
> versions.
>
> To me, 2016-02-30 is an invalid date that should generate an error.
> Automatically and silently changing it to be 2016-03-01 strikes me as a
> behavior I'd expect from a certain other open-source database, not
> PostgreSQL.

I don't particularly disagree with that, but on the other hand, as
mentioned earlier, to_timestamp() is here for Oracle compatibility,
and if it doesn't do what Oracle's function does, then (1) it's not
useful for people migrating from Oracle and (2) we're making up the
behavior out of whole cloth.  I think things that we invent ourselves
should reject stuff like this, but in a compatibility function we
might want to, say, have compatibility.

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


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

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Robert Haas]

On Wed, Jun 22, 2016 at 5:16 AM, Ashutosh Bapat
 wrote:
>> However, if we support deparsing subqueries, the remote query in the above
>> example could be rewritten into something like this:
>>
>> SELECT ss.c2 FROM t1 LEFT JOIN (SELECT t2.a, ROW(a, b) FROM t2) ss(c1, c2)
>> ON (t1.a = ss.c1);
>>
>> So we would no longer need "CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a,
>> r2.b) END" in the target list in the remote query.
>
> Right. Although, it means that the query processor at the other end has to
> do extra work for pulling up the subqueries.

I would be inclined to pick the method that generates cleaner SQL.  I
doubt that difference in optimization speed matters here - it's
presumably very small, especially when compared to the cost of the
network round-trip.

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


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

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Robert Haas]

On Wed, Jun 22, 2016 at 4:11 AM, Amit Langote
 wrote:
>> In an outer join we have to differentiate between a row being null (because
>> there was no joining row on nullable side) and a non-null row with all
>> column values being null. If we cast the whole-row expression to a text
>> e.g. r.*::text and test the resultant value for nullness, it gives us what
>> we want. A null row casted to text is null and a row with all null values
>> casted to text is not null.
>
> You are right.  There may be non-null rows with all columns null which are
> handled wrongly (as Rushabh reports) and the hack I proposed is not right
> for.  Especially if from non-nullable side as in the reported case, NULL
> test for such a whole-row-var would produce the wrong result.  Casting to
> text as your patch does produces the correct behavior.

I agree, but I think we'd better cast to pg_catalog.text instead, just
to be safe.  Committed that way.

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


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

Re: [HACKERS] Odd behavior with domains

[Robert Haas]

On Thu, Jun 23, 2016 at 11:00 PM, Alvaro Herrera
 wrote:
> Well, it's not specifically related to domains -- it's related to the
> fact that pg_catalog objects mask the domain you created in the public
> schema, because pg_catalog is by default in front of all other schemas
> unless you explicitely put it elsewhere.

Well, what's causing the apparent weirdness here is the fact that
pg_catalog, despite being implicitly at the front of the namespath
path, doesn't become the default creation schema as an
explicitly-named schema would.  So you don't try to create things
there but anything that already exists there masks the stuff you do
create.  And I think it's fair to say that's pretty weird to someone
who is unfamiliar with the way the system works.

We could do something like this:

NOTICE: existing type "pg_catalog"."text" will mask new type "public"."text"

We could even make that an ERROR by default, as long as we had some
GUC to disable the behavior for pg_dump.  How often do you really
intentionally create an object that shadows an existing system object?

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


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

Re: [HACKERS] Cleanup in contrib/postgres_fdw/deparse.c

[Robert Haas]

On Mon, Jun 20, 2016 at 4:47 AM, Etsuro Fujita
 wrote:
> Here is a small patch to remove an unnecessary return from
> deparseFromExprForRel in contrib/postgres_fdw/deparse.c.

Seems like a good cleanup.  Committed.

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


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

Re: [HACKERS] Protocol buffer support for Postgres

[Álvaro Hernández Tortosa]

On 24/06/16 14:23, Flavius Anton wrote:

On Thu, Jun 23, 2016 at 2:54 PM, Flavius Anton  wrote:

On Thu, Jun 23, 2016 at 1:50 PM, Greg Stark  wrote:

On Thu, Jun 23, 2016 at 8:15 PM, Flavius Anton  wrote:

I'd love to talk more about this.

I thought quite a bit about this a few years ago but never really
picked up it to work on.

Any other thoughts on this? My guess is that it might be an important
addition to Postgres that can attract even more users, but I am not
sure if there's enough interest from the community. If I want to pick
this task, how should I move forward? Do I need to write a design
document or similar or should I come up with a patch that implements a
draft prototype? I am new to this community and don't know the code
yet, so I'd appreciate some guidance from an older, more experienced
member.




Other than protobuf, there are also other serialization formats 
that might be worth considering. Not too long ago I suggested working 
specifically on serialization formas for the json/jsonb types: 
https://www.postgresql.org/message-id/56CB8A62.40100%408kdata.com I 
believe this effort is on the same boat.


Álvaro

--

Álvaro Hernández Tortosa


---
8Kdata



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

Re: [HACKERS] Protocol buffer support for Postgres

[Flavius Anton]

On Thu, Jun 23, 2016 at 2:54 PM, Flavius Anton  wrote:
> On Thu, Jun 23, 2016 at 1:50 PM, Greg Stark  wrote:
>> On Thu, Jun 23, 2016 at 8:15 PM, Flavius Anton  wrote:
>>>
>>> I'd love to talk more about this.
>>
>> I thought quite a bit about this a few years ago but never really
>> picked up it to work on.

Any other thoughts on this? My guess is that it might be an important
addition to Postgres that can attract even more users, but I am not
sure if there's enough interest from the community. If I want to pick
this task, how should I move forward? Do I need to write a design
document or similar or should I come up with a patch that implements a
draft prototype? I am new to this community and don't know the code
yet, so I'd appreciate some guidance from an older, more experienced
member.

Thanks.

--
Flavius


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

Re: [HACKERS] Rethinking behavior of force_parallel_mode = regress

[Robert Haas]

On Tue, Jun 21, 2016 at 4:41 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane  wrote:
>>> With that thought in mind, I propose that the behavior of
>>> force_parallel_mode = regress is ill-designed so far as EXPLAIN is
>>> concerned.  What it ought to do is suppress *all* Gathers from the output,
>>> not just ones that were added in response to force_parallel_mode itself.
>
>> No, that doesn't sound like a very good idea.  If you do that, then
>> you have no hope of the differences being *zero*, because any place
>> that the regression tests are intended to produce a parallel plan is
>> going to look different.
>
> Well, sure, but in those areas you just set force_parallel_mode to on.

Well, I don't see how that gets you anywhere.  Now every regression
test that generates a parallel plan needs a decoration to set
force_parallel_mode=on temporarily and then change it back to regress
afterwards.  And once you've done that, you no longer get any benefit
out of having changed the behavior of force_parallel_mode=regress.
Either I need more caffeine, or this is a bad plan.  Possibly both,
because I definitely need more caffeine.

>> The charter of force_parallel_mode=regress
>> is that any regression test that passes normally should still pass
>> with that setting.
>
> I would like that charter to include scenarios with other nondefault GUC
> settings, to the extent we can reasonably make it work, because setting
> *only* force_parallel_mode is pretty sad in terms of test coverage.
> Or hadn't you noticed all the bugs we flushed from cover as soon as we
> tried changing the cost values?

Well, I did send a WIP patch to set consider_parallel correctly for
upper rels, which helps a lot, but you seem not to have looked at it.
I think we should fix the bugs in the current approach before deciding
that it doesn't work.  That having been said, I can't disagree with
the principle you're setting forth here.

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


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

Re: [HACKERS] Odd behavior with domains

[Alvaro Herrera]

Joshua D. Drake wrote:

> Yes but what makes it weird is this:
> 
> postgres=# create domain text char(3);
> CREATE DOMAIN
> 
> -- cool, no problem
> 
> postgres=# create domain text char(2);
> ERROR:  type "text" already exists
> 
> -- as expected
> 
> postgres=# \dD
>  List of domains
>  Schema | Name | Type | Modifier | Check
> +--+--+--+---
> (0 rows)
> 
> -- wait what? I just created this.

The unadorned name "text" doesn't refer to the domain at this point,
since it's masked by the system type pg_catalog.text.

If you do "\dD public.*" you will see your "text" domain listed as well.

> postgres=# create domain textd char(2);
> CREATE DOMAIN
> postgres=# \dD
>  List of domains
>  Schema | Name  | Type | Modifier | Check
> +---+--+--+---
>  public | textd | character(2) |  |
> (1 row)
> 
> -- why would this show up without changing the search path if the
> -- previous one didn't?

Because there is no system object named textd.

> postgres=# drop domain text;
> ERROR:  "text" is not a domain

Right -- "text" is not a domain, it is pg_catalog.text.

> postgres=# set search_path to 'public';
> SET
> postgres=# drop domain text;
> ERROR:  "text" is not a domain
> postgres=#

Here you're still referring to pg_catalog.text, since as I said above
pg_catalog is put in front of the search path if you don't specify it
anywhere.  You need to add pg_catalog to search_path *after* public.
So you can do either
set search_path to 'public', 'pg_catalog'
drop domain text;

or
drop domain public.text;


> Note: If this is literally just the way it is, cool. It was just as I was
> exploring this all seemed odd.

Yes, this is the way it is, and yes it is odd -- but as I said it's not
specific to domains:

alvherre=# create table pg_class (a int, b text);
CREATE TABLE
alvherre=# \d
No se encontraron relaciones.


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


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

Re: [HACKERS] Odd behavior with domains

[David G. Johnston]

On Fri, Jun 24, 2016 at 1:08 PM, Joshua D. Drake 
wrote:

> On 06/23/2016 08:00 PM, Alvaro Herrera wrote:
>
>> Joshua D. Drake wrote:
>>
>>> Hey,
>>>
>>> So this came across my twitter feed:
>>>
>>> https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png
>>>
>>> I have verified the oddness with a newer version:
>>>
>>
>> Well, it's not specifically related to domains -- it's related to the
>> fact that pg_catalog objects mask the domain you created in the public
>> schema, because pg_catalog is by default in front of all other schemas
>> unless you explicitely put it elsewhere.
>>
>
> Yes but what makes it weird is this:
>
> postgres=# create domain text char(3);
> CREATE DOMAIN
>
> -- cool, no problem
>
> postgres=# create domain text char(2);
> ERROR:  type "text" already exists
>
> -- as expected
>
> postgres=# \dD
>  List of domains
>  Schema | Name | Type | Modifier | Check
> +--+--+--+---
> (0 rows)
>
> -- wait what? I just created this.
> -- I understand the search_path issue but:
>
>
​The fundamental problem is that for purposes of meta-command \d a domain
and a type are distinct object types.  But as far as the type system goes
the distinction is lost.  What \dD is telling us is that our newborn text
domain type is not visible to us​ - without telling us why (i.e., because
it is being shadowed by the text type).

Why do we even have "\dD"?  "\dT" displays domains.  Based upon that I'd
say \dD should display types regardless of search_path precedence and leave
\dT to display both domains and types with search_path considered.


> postgres=# create domain textd char(2);
> CREATE DOMAIN
> postgres=# \dD
>  List of domains
>  Schema | Name  | Type | Modifier | Check
> +---+--+--+---
>  public | textd | character(2) |  |
> (1 row)
>
> -- why would this show up without changing the search path if the
> -- previous one didn't?
>

Because this isn't being overshadowed by another non-domain type in the
system.


>
>
> postgres=# drop domain text;
> ERROR:  "text" is not a domain
> postgres=# set search_path to 'public';
> SET
> postgres=# drop domain text;
> ERROR:  "text" is not a domain
> postgres=#
>
> -- Now what?
>
> Note: If this is literally just the way it is, cool. It was just as I was
> exploring this all seemed odd.
>

You didn't specify pg_catalog explicitly and it is invalid to have a
search_path that doesn't include pg_catalog so PostgreSQL helps you out by
putting it in front of the one you specify.

SET search_path TO public, pg_catalog;
DROP DOMAIN text;

-- all good

Or just:

DROP DOMAIN public.text;

David J.

Re: [HACKERS] Bug in to_timestamp().

[Joshua D. Drake]

On 06/24/2016 09:26 AM, Steve Crawford wrote:

My observation has been that the PostgreSQL development group aims for
correctness and the elimination of surprising results. This was part of
the reason to eliminate a number of automatic casts to dates in earlier
versions.

To me, 2016-02-30 is an invalid date that should generate an error.
Automatically and silently changing it to be 2016-03-01 strikes me as a
behavior I'd expect from a certain other open-source database, not
PostgreSQL.


I don't think anybody could argue with that in good faith.

Sincerely,

JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

Re: [HACKERS] Odd behavior with domains

[Joshua D. Drake]

On 06/23/2016 08:00 PM, Alvaro Herrera wrote:

Joshua D. Drake wrote:

Hey,

So this came across my twitter feed:

https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png

I have verified the oddness with a newer version:


Well, it's not specifically related to domains -- it's related to the
fact that pg_catalog objects mask the domain you created in the public
schema, because pg_catalog is by default in front of all other schemas
unless you explicitely put it elsewhere.


Yes but what makes it weird is this:

postgres=# create domain text char(3);
CREATE DOMAIN

-- cool, no problem

postgres=# create domain text char(2);
ERROR:  type "text" already exists

-- as expected

postgres=# \dD
 List of domains
 Schema | Name | Type | Modifier | Check
+--+--+--+---
(0 rows)

-- wait what? I just created this.
-- I understand the search_path issue but:


postgres=# create domain textd char(2);
CREATE DOMAIN
postgres=# \dD
 List of domains
 Schema | Name  | Type | Modifier | Check
+---+--+--+---
 public | textd | character(2) |  |
(1 row)

-- why would this show up without changing the search path if the
-- previous one didn't?


postgres=# drop domain text;
ERROR:  "text" is not a domain
postgres=# set search_path to 'public';
SET
postgres=# drop domain text;
ERROR:  "text" is not a domain
postgres=#

-- Now what?

Note: If this is literally just the way it is, cool. It was just as I 
was exploring this all seemed odd.


Sincerely,

JD


--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

Re: [HACKERS] Bug in to_timestamp().

[Steve Crawford]

My observation has been that the PostgreSQL development group aims for
correctness and the elimination of surprising results. This was part of the
reason to eliminate a number of automatic casts to dates in earlier
versions.

To me, 2016-02-30 is an invalid date that should generate an error.
Automatically and silently changing it to be 2016-03-01 strikes me as a
behavior I'd expect from a certain other open-source database, not
PostgreSQL.

Cheers,
Steve

On Fri, Jun 24, 2016 at 8:52 AM, Alex Ignatov 
wrote:

>
> Alex Ignatov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
> On 20.06.2016 17:09, Albe Laurenz wrote:
>
>> Tom Lane wrote:
>>
>>> I don't necessarily have an opinion yet.  I would like to see more than
>>> just an unsupported assertion about what Oracle's behavior is.  Also,
>>> how should FM mode affect this?
>>>
>> I can supply what Oracle 12.1 does:
>>
>> SQL> SELECT to_timestamp('2016-06-13 15:43:36', ' /MM/DD HH24:MI:SS')
>> AS ts FROM dual;
>>
>> TS
>> 
>> 2016-06-13 15:43:36.0 AD
>>
>> SQL> SELECT to_timestamp('2016-06-13 15:43:36', '/MM/DD  HH24:MI:SS')
>> AS ts FROM dual;
>>
>> TS
>> 
>> 2016-06-13 15:43:36.0 AD
>>
>> SQL> SELECT to_timestamp('2016-06-1315:43:36', '/MM/DD
>> HH24:MI:SS') AS ts FROM dual;
>>
>> TS
>> 
>> 2016-06-13 15:43:36.0 AD
>>
>> (to_timestamp_tz behaves the same way.)
>>
>> So Oracle seems to make no difference between one or more spaces.
>>
>> Yours,
>> Laurenz Albe
>>
>> Guys, do we need to change this behavior or may be you can tell me that
> is normal because this and this:
>
> postgres=# SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD
> HH24:MI:SS');
>   to_timestamp
> 
>  2016-03-01 15:43:36+03
> (1 row)
>
> but on the other side we have :
>
> postgres=# select '2016-02-30 15:43:36'::timestamp;
> ERROR:  date/time field value out of range: "2016-02-30 15:43:36"
> LINE 1: select '2016-02-30 15:43:36'::timestamp;
>
> Another bug in to_timestamp/date()?
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] Bug in to_timestamp().

[Alex Ignatov]

Alex Ignatov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 20.06.2016 17:09, Albe Laurenz wrote:

Tom Lane wrote:

I don't necessarily have an opinion yet.  I would like to see more than
just an unsupported assertion about what Oracle's behavior is.  Also,
how should FM mode affect this?

I can supply what Oracle 12.1 does:

SQL> SELECT to_timestamp('2016-06-13 15:43:36', ' /MM/DD HH24:MI:SS') AS ts 
FROM dual;

TS

2016-06-13 15:43:36.0 AD

SQL> SELECT to_timestamp('2016-06-13 15:43:36', '/MM/DD  HH24:MI:SS') AS ts 
FROM dual;

TS

2016-06-13 15:43:36.0 AD

SQL> SELECT to_timestamp('2016-06-1315:43:36', '/MM/DD  HH24:MI:SS') AS 
ts FROM dual;

TS

2016-06-13 15:43:36.0 AD

(to_timestamp_tz behaves the same way.)

So Oracle seems to make no difference between one or more spaces.

Yours,
Laurenz Albe

Guys, do we need to change this behavior or may be you can tell me that 
is normal because this and this:


postgres=# SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD 
HH24:MI:SS');

  to_timestamp

 2016-03-01 15:43:36+03
(1 row)

but on the other side we have :

postgres=# select '2016-02-30 15:43:36'::timestamp;
ERROR:  date/time field value out of range: "2016-02-30 15:43:36"
LINE 1: select '2016-02-30 15:43:36'::timestamp;

Another bug in to_timestamp/date()?


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

Re: [HACKERS] Bug in to_timestamp().

[Alex Ignatov]

On 23.06.2016 20:40, Tom Lane wrote:

Robert Haas  writes:

On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston
 wrote:

My understanding is that is not going to change for 9.6.

That's exactly what is under discussion here.

I would definitely agree with David on that point.  Making to_timestamp
noticeably better on this score seems like a nontrivial project, and
post-beta is not the time for that sort of thing, even if we had full
consensus on what to do.  I'd suggest somebody work on a patch and put
it up for review in the next cycle.

Now, if you were to narrowly define the problem as "whether to skip
non-spaces for a space in the format", maybe that could be fixed
post-beta, but I think that's a wrongheaded approach.  to_timestamp's
issues with input that doesn't match the format are far wider than that.
IMO we should try to resolve the whole problem with one coherent change,
not make incremental incompatible changes at the margins.

At the very least I'd want to see a thought-through proposal that
addresses all three of these interrelated points:

* what should a space in the format match
* what should a non-space, non-format-code character in the format match
* how should we handle fields that are not exactly the width suggested
by the format

regards, tom lane


Totally agree that we need more discussion about error handling in this 
function!


Also this behavior is observed in to_date() and to_number() function:

postgres=# SELECT 
TO_DATE('2!0!1!6!0!6-/-/-/-/-/-/-1!/-/-/-/-/-/-/-3!', '-MM-DD');

  to_date

 0002-01-01
(1 row)

postgres=# postgres=# select to_number('1$#@!!,2,%,%4,5,@%5@4..8-', 
'999G999D9S');

 to_number
---
12
(1 row)

On the our side we have some discussions about to write a patch that 
will change this incorrect  behavior. So stay tuned.


--

Alex Ignatov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Haroon .]

> On Fri, Jun 24, 2016 at 1:28 PM, Craig Ringer
 wrote:

> > I'd like more details from those whose installs are crashing. What exact
> > vcvars env did you run under, with which exact cl.exe version?

This is a Windows server 2012 R2 Standard.
Devenv: Microsoft Visual Studio 2013 Community Version 12.0.31101.0.

 Env:

%comspec% /k ""C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\vcvarsall.bat"" x86_amd64

 'where cl.exe'

C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\bin\x86_amd64\cl.exe
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\bin\cl.exe


I have been able to reproduce it on Windows 7 Professional (Service Pack 1
) also with Microsoft Visual Studio 2013 Community Version 12.0.40629.0.

   Env:
 %comspec% /k ""C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\vcvarsall.bat"" x86_amd64

   'Where cl.exe'
  C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\bin\x86_amd64\cl.exe
  C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\bin\cl.exe



I started with bisect activity between beta2 (bad) and beta1(good) given
that beta1 works fine. Crash occurs at following commit.

commit 100340e2dcd05d6505082a8fe343fb2ef2fa5b2a
Author: Tom Lane 
Date:   Sat Jun 18 15:22:34 2016 -0400


Restore foreign-key-aware estimation of join relation sizes.

This patch provides a new implementation of the logic added by commit
137805f89 and later removed by 77ba61080.  It differs from the original
primarily in expending much less effort per joinrel in large queries,
which it accomplishes by doing most of the matching work once per query
not
once per joinrel.  Hopefully, it's also less buggy and better commented.
The never-documented enable_fkey_estimates GUC remains gone.

There remains work to be done to make the selectivity estimates account
for nulls in FK referencing columns; but that was true of the original
patch as well.  We may be able to address this point later in beta.
In the meantime, any error should be in the direction of overestimating
rather than underestimating joinrel sizes, which seems like the
direction
we want to err in.

Tomas Vondra and Tom Lane
Discussion: <31041.1465069...@sss.pgh.pa.us>

This appears consistent with the crash in planner suggested by crash dump
Craig shared.

Tom any ideas on what could be going wrong here ?

Given that it fails on 'setup_description', I tried bypassing that by
commenting it out, it again crashes on 'setup_privileges' and
'setup_schema'.

debug_query_string for setup_privileges:

*INSERT INTO pg_init_privs   (objoid, classoid, objsubid, initprivs,
privtype)SELECToid,(SELECT oid FROM pg_class WHERE
relname = 'pg_class'),0,relacl,'i'FROM
 pg_classWHERErelacl IS NOT NULLAND relkind IN ('r',
'v', 'm', 'S');INSERT INTO pg_init_privs   (objoid, classoid, objsubid,
initprivs, privtype)SELECTpg_class.oid,(SELECT oid FROM
pg_class WHERE relname = 'pg_class'),pg_attribute.attnum,
 pg_attribute.attacl,'i'FROMpg_classJOIN
pg_attribute ON (pg_class.oid = pg_attribute.attrelid)WHERE
 pg_attribute.attacl IS NOT NULLAND pg_class.relkind IN ('r', 'v',
'm', 'S');INSERT INTO pg_init_privs   (objoid, classoid, objsubid,
initprivs, privtype)SELECToid,(SELECT oid FROM pg_class
WHERE relname = 'pg_proc'),0,proacl,'i'FROM
   pg_procWHEREproacl IS NOT NULL;INSERT INTO pg_init_privs
(objoid, classoid, objsubid, initprivs, privtype)SELECToid,
   (SELECT oid FROM pg_class WHERE relname = 'pg_type'),0,
 typacl,'i'FROMpg_typeWHEREtypacl IS NOT
NULL;INSERT INTO pg_init_privs   (objoid, classoid, objsubid, initprivs,
privtype)SELECToid,(SELECT oid FROM pg_class WHERE
relname = 'pg_language'),0,lanacl,'i'FROM
 pg_languageWHERElanacl IS NOT NULL;INSERT INTO pg_init_privs
(objoid, classoid, objsubid, initprivs, privtype)SELECToid,
   (SELECT oid FROM pg_class WHERE  relname = 'pg_largeobject_metadata'),
 0,lomacl,'i'FROMpg_largeobject_metadata
 WHERElomacl IS NOT NULL;INSERT INTO pg_init_privs   (objoid,
classoid, objsubid, initprivs, privtype)SELECToid,
 (SELECT oid FROM pg_class WHERE relname = 'pg_namespace'),0,
 nspacl,'i'FROMpg_namespaceWHEREnspacl IS
NOT NULL;INSERT INTO pg_init_privs   (objoid, classoid, objsubid,
initprivs, privtype)SELECToid,(SELECT oid FROM pg_class
WHERE relname = 'pg_database'),0,datacl,'i'FROM
   pg_databaseWHEREdatacl IS NOT NULL;INSERT INTO
pg_init_privs   (objoid, 

Re: [HACKERS] Declarative partitioning

[Ashutosh Bapat]

Hi Amit,
I tried creating 2-level partitioned table and tried to create simple table
using CTAS from the partitioned table. It gives a cache lookup error.
Here's the test
CREATE TABLE pt1_l (a int, b varchar, c int) PARTITION BY RANGE(a);
CREATE TABLE pt1_l_p1 PARTITION OF pt1_l FOR VALUES START (1) END (250)
INCLUSIVE PARTITION BY RANGE(b);
CREATE TABLE pt1_l_p2 PARTITION OF pt1_l FOR VALUES START (251) END (500)
INCLUSIVE PARTITION BY RANGE(((a+c)/2));
CREATE TABLE pt1_l_p3 PARTITION OF pt1_l FOR VALUES START (501) END (600)
INCLUSIVE PARTITION BY RANGE(c);
CREATE TABLE pt1_l_p1_p1 PARTITION OF pt1_l_p1 FOR VALUES START ('01')
END ('000125') INCLUSIVE;
CREATE TABLE pt1_l_p1_p2 PARTITION OF pt1_l_p1 FOR VALUES START ('000126')
END ('000250') INCLUSIVE;
CREATE TABLE pt1_l_p2_p1 PARTITION OF pt1_l_p2 FOR VALUES START (251) END
(375) INCLUSIVE;
CREATE TABLE pt1_l_p2_p2 PARTITION OF pt1_l_p2 FOR VALUES START (376) END
(500) INCLUSIVE;
CREATE TABLE pt1_l_p3_p1 PARTITION OF pt1_l_p3 FOR VALUES START (501) END
(550) INCLUSIVE;
CREATE TABLE pt1_l_p3_p2 PARTITION OF pt1_l_p3 FOR VALUES START (551) END
(600) INCLUSIVE;
INSERT INTO pt1_l SELECT i, to_char(i, 'FM00'), i FROM
generate_series(1, 600, 2) i;
CREATE TABLE upt1_l AS SELECT * FROM pt1_l;

The last statement gives error "ERROR:  cache lookup failed for function
0". Let me know if this problem is reproducible.


On Thu, Jun 9, 2016 at 7:20 AM, Amit Langote 
wrote:

> On 2016/06/08 22:22, Ashutosh Bapat wrote:
> > On Mon, May 23, 2016 at 3:35 PM, Amit Langote wrote
> >>
> >> [...]
> >>
> >> I made a mistake in the last version of the patch which caused a
> relcache
> >> field to be pfree'd unexpectedly.  Attached updated patches.
> >
> > 0003-... patch does not apply cleanly. It has some conflicts in
> pg_dump.c.
> > I have tried fixing the conflict in attached patch.
>
> Thanks.  See attached rebased patches.
>
> Regards,
> Amit
>



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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Tom Lane]

Craig Ringer  writes:
> I have absolutely no idea why it's trying to access memory at what looks
> like   (uint64)(-1) though.  Nothing in the auto vars list:

> +  0x0043f7b0 {0x09e32600 {type=T_List (656)
> length=1 head=0x09e325e0 {data={ptr_value=...} ...} ...}} List * *
> + inner_rel 0x09e7ad68 {type=T_EquivalenceClass (537)
> reloptkind=RELOPT_BASEREL (0) relids=0x09e30520 {...} ...} RelOptInfo
> *
> + inner_rel->relids 0x09e30520 {nwords=658 words=0x09e30524
> {...} } Bitmapset *
> + outer_rel 0x0001401dec98
> {postgres.exe!build_joinrel_tlist(PlannerInfo * root, RelOptInfo * joinrel,
> RelOptInfo * input_rel), Line 646} {...} RelOptInfo *
> + outer_rel->relids 0xe808498b48d78b48 {nwords=??? words=0xe808498b48d78b4c
> {...} } Bitmapset *
> + sjinfo 0x0043f870 {type=T_SpecialJoinInfo (543)
> min_lefthand=0x09e7abd0 {nwords=1 words=0x09e7abd4 {...} }
> ...} SpecialJoinInfo *


inner_rel seems to be pointing at garbage, or at least why is the
referenced object tag T_EquivalenceClass not T_RelOptInfo?  And
why aren't we being given anything for outer_rel?  The value for
outer_rel->relids isn't inspiring any confidence either, and
for that matter inner_rel->relids couldn't possibly have more than
nwords==1 given how simple the query is.  In short, either the
debugger is totally confused or the code is, because most of these
pointers aren't pointing at anything sane.

TBH, this looks more like a compiler bug than anything else.  I wonder
whether it's getting confused by taking the address of a parameter
(although surely we do that elsewhere).

It would be worth recompiling at -O0, or whatever the local equivalent
of that is, to see if (1) the crash goes away or (2) the debugger's
printouts get any more reliable.

regards, tom lane


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

[HACKERS] Odd behavior with domains

[Joshua D. Drake]

Hey,

So this came across my twitter feed:

https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png

I have verified the oddness with a newer version:

psql -U postgres
psql (9.5.3)
Type "help" for help.

postgres=# create domain text char(3);
CREATE DOMAIN
postgres=# create domain text char(2);
ERROR:  type "text" already exists
postgres=# \dD
 List of domains
 Schema | Name | Type | Modifier | Check
+--+--+--+---
(0 rows)

postgres=# create domain textd char(2);
CREATE DOMAIN
postgres=# \dD
 List of domains
 Schema | Name  | Type | Modifier | Check
+---+--+--+---
 public | textd | character(2) |  |
(1 row)



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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Haroon]

On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer
 wrote:

>> I was helping Haroon with this last night. I don't have access to the
>> original thread and he's not around so I don't know how much he said.
I'll
>> repeat our findings here.

Craig, I am around now looking into this. I'll update the list as I get
more info.

- Haroon

On 24 June 2016 at 11:27, Michael Paquier  wrote:

> On Fri, Jun 24, 2016 at 3:22 PM, Craig Ringer 
> wrote:
> >
> >
> > On 24 June 2016 at 10:28, Michael Paquier 
> wrote:
> >>
> >> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer 
> >> wrote:
> >> >   * Launch a VS x86 command prompt
> >> >   * devenv /debugexe bin\initdb.exe -D test
> >> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
> >> >   * Run
> >> >   * When it traps at get_restricted_token(), manually move the
> execution
> >> > pointer over the setup of the restricted execution token by dragging &
> >> > dropping the yellow instruction pointer arrow. Yes, really. Or,
> y'know,
> >> > comment it out and rebuild, but I was working with a supplied binary.
> >> >   * Continue until next breakpoint
> >> >   * Launch process explorer and find the pid of the postgres child
> >> > process
> >> >   * Debug->attach to process, attach to the child postgres. This
> doesn't
> >> > detach the parent, VS does multiprocess debugging.
> >> >   * Continue execution
> >> >   * vs will trap on the child when it crashes
> >>
> >> Do you think a crash dump could have been created by creating
> >> crashdumps/ in PGDATA as part of initdb before this query is run?
> >
> >
> >
> > The answer is "yes" btw. Add "crashdumps" to the static array of
> directories
> > created by initdb and it works great.
>
> As simple as attached..
>
> > Sigh. It'd be less annoying if I hadn't written most of the original
> patch.
>
> You mean the patch that created the crashdumps/ trick? This has saved
> me a couple of months back to analyze a problem TBH.
> --
> Michael
>



-- 
Haroonhttp://www.2ndQuadrant.com/

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

[HACKERS] bug in citext's upgrade script for parallel aggregates

[David Rowley]

Seems there's a small error in the upgrade script for citext for 1.1
to 1.2 which will cause min(citext) not to be parallel enabled.

max(citext)'s combinefunc is first set incorrectly, but then updated
to the correct value. I assume it was meant to set the combine
function for min(citext) instead.

Fix attached. I've assumed that because we're still in beta that we
can get away with this fix rather than making a 1.3 version to fix the
issue.

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


citext_1.1_to_1.2_upgrade_fix.patch
Description: Binary data

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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Haroon Muhammad]

I have been running bisect, it breaks at this commit:

*commit 100340e2dcd05d6505082a8fe343fb2ef2fa5b2a*
*Author: Tom Lane >*
*Date:   Sat Jun 18 15:22:34 2016 -0400*
*Restore foreign-key-aware estimation of join relation sizes.*

*This patch provides a new implementation of the logic added by commit*
*137805f89 and later removed by 77ba61080.  It differs from the
original*
*primarily in expending much less effort per joinrel in large queries,*
*which it accomplishes by doing most of the matching work once per
query not*
*once per joinrel.  Hopefully, it's also less buggy and better
commented.*
*The never-documented enable_fkey_estimates GUC remains gone.*

*There remains work to be done to make the selectivity estimates
account*
*for nulls in FK referencing columns; but that was true of the original*
*patch as well.  We may be able to address this point later in beta.*
*In the meantime, any error should be in the direction of
overestimating*
*rather than underestimating joinrel sizes, which seems like the
direction*

*we want to err in.*

*Tomas Vondra and Tom Lane*Discussion: <
31041.1465069...@sss.pgh.pa.us>

-- 
Haroonhttp://www.2ndQuadrant.com/

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



On Fri, Jun 24, 2016 at 12:19 PM, Haroon Muhammad  wrote:

> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer
>  wrote:
>
> >> I was helping Haroon with this last night. I don't have access to the
> >> original thread and he's not around so I don't know how much he said.
> I'll
> >> repeat our findings here.
>
> Craig, I am around now looking into this. I'll update the list as I get
> more info.
>
> Apparently my previous message (this same text ) didn't make it through ...
>
>
> -- Haroon
>
>

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Amit Langote]

On 2016/06/24 17:38, Ashutosh Bapat wrote:
> On Fri, Jun 24, 2016 at 1:59 PM, Amit Langote wrote:
>> I'm now starting to wonder if it would be outright wrong to just use the
>> alias names of corresponding foreign tables directly for whole-row
>> references?  So, instead of these in target lists of remote queries:
>>
>> SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.*)* END, ...
>
> This is wrong. The deparsed query looks like
> SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.col1, r1.col2, ...)*
> END,

Yeah, I had noticed that in explain outputs (should have written like
that).  My point though is why we don't consider dropping the CASE WHEN
... END target list item solution in favor of simply using the alias name
for a whole-row reference without affecting the correctness behavior wrt
outer joins.  Using CASE WHEN to emit the correct result has revealed its
downside (this thread) although a simple whole-row reference would just
work without any special consideration.

> The reason for this is that the foreign table definition may not match the
> target table definition. This has been explained in the comments that you
> have deleted in your patch. Am I missing something?

What may go wrong if we requested r1 (an alias name) in target list of the
sent query instead of ROW(r1.col1, ...) for a whole-row reference in the
original query?  Fear of wrong set of data arriving or in wrong order or
something like that?  This part, I'm not quite sure about.

Thanks,
Amit




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

Re: [HACKERS] Hash Indexes

[Mithun Cy]

On Thu, Jun 16, 2016 at 12:58 PM, Amit Kapila 
 wrote:

I have a question regarding code changes in *_hash_first*.

+/*
+ * Conditionally get the lock on primary bucket page for search
while
+* holding lock on meta page. If we have to wait, then release the
meta
+ * page lock and retry it in a hard way.
+ */
+bucket = _hash_hashkey2bucket(hashkey,
+
 metap->hashm_maxbucket,
+
 metap->hashm_highmask,
+
 metap->hashm_lowmask);
+
+blkno = BUCKET_TO_BLKNO(metap, bucket);
+
+/* Fetch the primary bucket page for the bucket */
+buf = ReadBuffer(rel, blkno);
+if (!ConditionalLockBufferShared(buf))

Here we try to take lock on bucket page but I think if successful we do not
recheck whether any split happened before taking lock. Is this not
necessary now?

Also  below "if" is always true as we enter here only when outer "if
(retry)" is true.
+if (retry)
+{
+if (oldblkno == blkno)
+break;
+_hash_relbuf(rel, buf);
+}

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

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Wed, Jun 22, 2016 at 10:51 AM, Michael Paquier
 wrote:
> This connection string is stored in shared memory in WalRcvData, and
> that's the case for a couple of releases now, so it has already a high
> footprint, though I agree that making that available at SQL level
> makes it even worse. Looking at the code, as libpq does not link
> directly to libpqcommon, I think that the cleanest solution is to do
> something similar to wchar.c, aka split the parsing, deparsing
> routines that we are going to use in a file located in src/backend/,
> and then build libpq using it. Writing a patch for that would not be
> that complicated. What is stored in WalRcvData is then the connection
> string filtered of its password field, or we could just obfuscate it
> with ***. It may still be useful to the user to know that a password
> has been used.

I have been thinking more about that, and came up with the following
idea... We do not want to link libpq directly to the server, so let's
add a new routine to libpqwalreceiver that builds an obfuscated
connection string and let's have walreceiver.c save it in shared
memory. Then pg_stat_wal_receiver just makes use of this string. This
results in a rather light patch, proposed as attached. Connection URIs
get as well translated as connection strings via PQconninfo(), then
the new interface routine of libpqwalreceiver looks at dispchar to
determine if it should dump a field or not and obfuscates it with more
or less ''.

Thoughts?
--
Michael


wal-receiver-conninfo-v3.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Ashutosh Bapat]

On Fri, Jun 24, 2016 at 1:59 PM, Amit Langote  wrote:

> On 2016/06/24 15:44, Ashutosh Bapat wrote:
> >>
> >> I think the proposed idea of applying record::text explicit coercion to
> a
> >> whole-row reference in the IS NOT NULL condition in the CASE WHEN
> >> conversion would work as expected as you explained, but I'm concerned
> that
> >> the cost wouldn't be negligible when the foreign table has a lot of
> columns.
> >
> > That's right, if the foreign server doesn't optimize the case for IS NOT
> > NULL, which it doesn't :)
> >
> > I am happy to use any cheaper means e.g a function which counts number of
> > columns in a record. All we need here is a way to correctly identify
> when a
> > record is null and not null in the way we want (as described upthread). I
> > didn't find any quickly. Do you have any suggestions?
>
> I'm now starting to wonder if it would be outright wrong to just use the
> alias names of corresponding foreign tables directly for whole-row
> references?  So, instead of these in target lists of remote queries:
>
> SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.*)* END, ...
>
>
This is wrong. The deparsed query looks like
SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.col1, r1.col2, ...)*
END,

The reason for this is that the foreign table definition may not match the
target table definition. This has been explained in the comments that you
have deleted in your patch. Am I missing something?

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

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Amit Langote]

On 2016/06/24 15:44, Ashutosh Bapat wrote:
>>
>> I think the proposed idea of applying record::text explicit coercion to a
>> whole-row reference in the IS NOT NULL condition in the CASE WHEN
>> conversion would work as expected as you explained, but I'm concerned that
>> the cost wouldn't be negligible when the foreign table has a lot of columns.
> 
> That's right, if the foreign server doesn't optimize the case for IS NOT
> NULL, which it doesn't :)
> 
> I am happy to use any cheaper means e.g a function which counts number of
> columns in a record. All we need here is a way to correctly identify when a
> record is null and not null in the way we want (as described upthread). I
> didn't find any quickly. Do you have any suggestions?

I'm now starting to wonder if it would be outright wrong to just use the
alias names of corresponding foreign tables directly for whole-row
references?  So, instead of these in target lists of remote queries:

SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN ROW (r1.*) END, ...

Just:

SELECT r1, ...

It seems to produce the correct result.  Although, I may be missing
something because CASE WHEN solution seems to me to be deliberately chosen.

In any case, attached patch doing the above did not change the results of
related regression tests (plans obviously did change since they don't
output the CASE WHENs in target lists anymore).

Also see the example below:

create extension postgres_fdw;
create server myserver foreign data wrapper postgres_fdw options (dbname
'postgres', use_remote_estimate 'true');
create user mapping for CURRENT_USER server myserver;

create table t1(a int, b int);
create table t2(a int, b int);

create foreign table ft1(a int, b int) server myserver options (table_name
't1');
create foreign table ft2(a int, b int) server myserver options (table_name
't2');

insert into t1 values (1), (2);
insert into t1 values (null, null);

insert into t2 values (1);
insert into t2 values (1, 2);

explain (costs off, verbose) select t1, t1 is null, t2, t2 is null from
ft1 t1 left join ft2 t2 on (t1.a = t2.a);
 QUERY PLAN

-
 Foreign Scan
   Output: t1.*, (t1.* IS NULL), t2.*, (t2.* IS NULL)
   Relations: (public.ft1 t1) LEFT JOIN (public.ft2 t2)
   Remote SQL: SELECT r1, r2 FROM (public.t1 r1 LEFT JOIN public.t2 r2 ON
(((r1.a = r2.a
(4 rows)

select t1, t1 is null as t1null, t2, t2 is null as t2null from ft1 t1 left
join ft2 t2 on (t1.a = t2.a);
  t1  | t1null |  t2   | t2null
--++---+
 (1,) | f  | (1,)  | f
 (1,) | f  | (1,2) | f
 (2,) | f  |   | t
 (,)  | t  |   | t
(4 rows))

alter server myserver options (set use_remote_estimate 'false');
analyze;

explain (costs off, verbose) select t1, t1 is null, t2, t2 is null from
ft1 t1 left join ft2 t2 on (t1.a = t2.a);
  QUERY PLAN
--
 Merge Left Join
   Output: t1.*, (t1.* IS NULL), t2.*, (t2.* IS NULL)
   Merge Cond: (t1.a = t2.a)
   ->  Sort
 Output: t1.*, t1.a
 Sort Key: t1.a
 ->  Foreign Scan on public.ft1 t1
   Output: t1.*, t1.a
   Remote SQL: SELECT a, b FROM public.t1
   ->  Sort
 Output: t2.*, t2.a
 Sort Key: t2.a
 ->  Foreign Scan on public.ft2 t2
   Output: t2.*, t2.a
   Remote SQL: SELECT a, b FROM public.t2
(15 rows)

select t1, t1 is null as t1null, t2, t2 is null as t2null from ft1 t1 left
join ft2 t2 on (t1.a = t2.a);
  t1  | t1null |  t2   | t2null
--++---+
 (1,) | f  | (1,)  | f
 (1,) | f  | (1,2) | f
 (2,) | f  |   | t
 (,)  | t  |   | t
(4 rows)

And produces the correct result for Rushabh's case.

Thoughts?

Thanks,
Amit
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index c91f3a5..d742693 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1609,57 +1609,18 @@ deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
 	}
 	else if (varattno == 0)
 	{
-		/* Whole row reference */
-		Relation	rel;
-		Bitmapset  *attrs_used;
-
-		/* Required only to be passed down to deparseTargetList(). */
-		List	   *retrieved_attrs;
-
-		/* Get RangeTblEntry from array in PlannerInfo. */
-		rte = planner_rt_fetch(varno, root);
-
-		/*
-		 * The lock on the relation will be held by upper callers, so it's
-		 * fine to open it with no lock here.
-		 */
-		rel = heap_open(rte->relid, NoLock);
-
-		/*
-		 * The local name of the foreign table can not be recognized by the
-		 * foreign server and the table it references on foreign server might
-		 * have different column ordering or different columns than those
-		 * declared locally. Hence we have to deparse whole-row reference as
-		 * ROW(columns referenced locally). Construct this by deparsing a

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Haroon Muhammad]

On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer
 wrote:

>> I was helping Haroon with this last night. I don't have access to the
>> original thread and he's not around so I don't know how much he said.
I'll
>> repeat our findings here.

Craig, I am around now looking into this. I'll update the list as I get
more info.

Apparently my previous message (this same text ) didn't make it through ...


-- Haroon

Re: [HACKERS] PQconnectdbParams vs PQconninfoParse

[Craig Ringer]

On 23 June 2016 at 21:59, Tom Lane  wrote:

> Craig Ringer  writes:
> > While writing some code that takes a connstring and adds some
> parameters, I
> > noticed that PQconninfoParse doesn't play well with PQconnectdbParams.
>
> > PQconnectdbParams takes a pair of equal-length arrays, one for keys and
> one
> > for values, each terminated by null elements.  But PQconninfoParse
> returns
> > a an array of PQconninfoOption .
>
> > This means the client has to do a bunch of fiddling to turn a parsed
> > conninfo into something that can be passed to PQconnectdbParams .  This
> > seems bizarre. Am I missing something obvious?
>
> Um, I don't see the connection.  Under what circumstances would you want
> to pass the result of PQconninfoParse directly to PQconnectdbParams?
>

If you were passing it directly you wouldn't, but it can make sense to
parse it, modify the results and then pass that to a connect function. To
do so now you have to allocate a couple of new arrays and do a bunch of
copying. It's not hard, just a bit ugly and shouldn't be necessary.


> PQconninfoOption is intended to provide a lot of secondary information
> about the available options, so it's more in the nature of an
> informational record than something you would feed back into the library.
>

That makes sense.


> In particular, I object to using PQconninfoOption as an input data
> structure to the library, because then you'd have a bunch of definitional
> questions about which fields of that struct the client app is expected to
> make valid, plus the potential for well-hidden bugs if some part of the
> library unintentionally relies on a field it shouldn't when looking at a
> PQconninfoOption that came from outside the library rather than inside.
>

OK, good point.

Given those points the current situation is not ugly enough to be worth
pursuing when it's not exactly hard to copy the results into key and value
arrays. I was looking at this as a low-hanging API usability wart, and it
isn't.

Thanks for pointing out the issues.


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

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Ashutosh Bapat]

>
> I think the proposed idea of applying record::text explicit coercion to a
> whole-row reference in the IS NOT NULL condition in the CASE WHEN
> conversion would work as expected as you explained, but I'm concerned that
> the cost wouldn't be negligible when the foreign table has a lot of columns.
>

That's right, if the foreign server doesn't optimize the case for IS NOT
NULL, which it doesn't :)

I am happy to use any cheaper means e.g a function which counts number of
columns in a record. All we need here is a way to correctly identify when a
record is null and not null in the way we want (as described upthread). I
didn't find any quickly. Do you have any suggestions?


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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jun 24, 2016 at 3:22 PM, Craig Ringer  wrote:
>
>
> On 24 June 2016 at 10:28, Michael Paquier  wrote:
>>
>> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer 
>> wrote:
>> >   * Launch a VS x86 command prompt
>> >   * devenv /debugexe bin\initdb.exe -D test
>> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
>> >   * Run
>> >   * When it traps at get_restricted_token(), manually move the execution
>> > pointer over the setup of the restricted execution token by dragging &
>> > dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
>> > comment it out and rebuild, but I was working with a supplied binary.
>> >   * Continue until next breakpoint
>> >   * Launch process explorer and find the pid of the postgres child
>> > process
>> >   * Debug->attach to process, attach to the child postgres. This doesn't
>> > detach the parent, VS does multiprocess debugging.
>> >   * Continue execution
>> >   * vs will trap on the child when it crashes
>>
>> Do you think a crash dump could have been created by creating
>> crashdumps/ in PGDATA as part of initdb before this query is run?
>
>
>
> The answer is "yes" btw. Add "crashdumps" to the static array of directories
> created by initdb and it works great.

As simple as attached..

> Sigh. It'd be less annoying if I hadn't written most of the original patch.

You mean the patch that created the crashdumps/ trick? This has saved
me a couple of months back to analyze a problem TBH.
-- 
Michael


dbg-initdb.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Etsuro Fujita]

On 2016/06/22 19:37, Ashutosh Bapat wrote:

On Wed, Jun 22, 2016 at 3:57 PM, Etsuro Fujita
Maybe I'm confused, but I think that in the system-column case it's
the user's responsibility to specify system columns for foreign
tables in a local query only when that makes sense on the remote
end, as shown in the below counter example:

postgres=# create foreign table fv1 (a int, b int) server myserver
options (table_name 'v1');
CREATE FOREIGN TABLE
postgres=# select ctid, * from fv1;
ERROR:  column "ctid" does not exist
CONTEXT:  Remote SQL command: SELECT a, b, ctid FROM public.v1



But a ctid, when available, would rightly get nullified when referenced
as a column of table.


Really?


What happens with the other system columns is we 0
them out locally, whether they are available at the foreign server or
not. We never try to check whether they are available at the foreign
server or not. If we use such column's column name to decide whether to
report 0 or null and if that column is not available at the foreign
table, we will get error. I think we should avoid that. Those column
anyway do not make any sense.


Agreed except for oid.  I think we should handle oid in the same way as 
ctid, which I'll work on in the next CF.


I think the proposed idea of applying record::text explicit coercion to 
a whole-row reference in the IS NOT NULL condition in the CASE WHEN 
conversion would work as expected as you explained, but I'm concerned 
that the cost wouldn't be negligible when the foreign table has a lot of 
columns.


Best regards,
Etsuro Fujita




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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 10:28, Michael Paquier  wrote:

> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer 
> wrote:
> >   * Launch a VS x86 command prompt
> >   * devenv /debugexe bin\initdb.exe -D test
> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
> >   * Run
> >   * When it traps at get_restricted_token(), manually move the execution
> > pointer over the setup of the restricted execution token by dragging &
> > dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
> > comment it out and rebuild, but I was working with a supplied binary.
> >   * Continue until next breakpoint
> >   * Launch process explorer and find the pid of the postgres child
> process
> >   * Debug->attach to process, attach to the child postgres. This doesn't
> > detach the parent, VS does multiprocess debugging.
> >   * Continue execution
> >   * vs will trap on the child when it crashes
>
> Do you think a crash dump could have been created by creating
> crashdumps/ in PGDATA as part of initdb before this query is run?
>


The answer is "yes" btw. Add "crashdumps" to the static array of
directories created by initdb and it works great.

Sigh. It'd be less annoying if I hadn't written most of the original patch.

For convenience I also commented out the check_root call in
src/backend/main.c and the get_restricted_token(progname) call in initdb.c,
so I could run it easily under an admin account where I can also install
tools etc without hassle. Not recommended on a non-throwaway machine of
course.

The generated crashdump shows the same crash in the same location.

I have absolutely no idea why it's trying to access memory at what looks
like   (uint64)(-1) though.  Nothing in the auto vars list:

+  0x0043f7b0 {0x09e32600 {type=T_List (656)
length=1 head=0x09e325e0 {data={ptr_value=...} ...} ...}} List * *
+ inner_rel 0x09e7ad68 {type=T_EquivalenceClass (537)
reloptkind=RELOPT_BASEREL (0) relids=0x09e30520 {...} ...} RelOptInfo
*
+ inner_rel->relids 0x09e30520 {nwords=658 words=0x09e30524
{...} } Bitmapset *
+ outer_rel 0x0001401dec98
{postgres.exe!build_joinrel_tlist(PlannerInfo * root, RelOptInfo * joinrel,
RelOptInfo * input_rel), Line 646} {...} RelOptInfo *
+ outer_rel->relids 0xe808498b48d78b48 {nwords=??? words=0xe808498b48d78b4c
{...} } Bitmapset *
+ sjinfo 0x0043f870 {type=T_SpecialJoinInfo (543)
min_lefthand=0x09e7abd0 {nwords=1 words=0x09e7abd4 {...} }
...} SpecialJoinInfo *

or locals:

+ inner_rel 0x09e7ad68 {type=T_EquivalenceClass (537)
reloptkind=RELOPT_BASEREL (0) relids=0x09e30520 {...} ...} RelOptInfo
*
inner_rows 270.00 double
+ outer_rel 0x0001401dec98
{postgres.exe!build_joinrel_tlist(PlannerInfo * root, RelOptInfo * joinrel,
RelOptInfo * input_rel), Line 646} {...} RelOptInfo *
outer_rows 2.653351978175e-314#DEN double
+ restrictlist 0x09e32600 {type=T_List (656) length=1
head=0x09e325e0 {data={ptr_value=0x09e31788 ...} ...} ...} List
*
+ root 0x09e7b3f8 {type=1 parse=0x00504ad0
{type=T_AllocSetContext (601) commandType=CMD_UNKNOWN (0) ...} ...} PlannerInfo
*
+ sjinfo 0x0043f870 {type=T_SpecialJoinInfo (543)
min_lefthand=0x09e7abd0 {nwords=1 words=0x09e7abd4 {...} }
...} SpecialJoinInfo *

seems to fit. Though outer_rel->relids is a pretty weird address -
0xe808498b48d78b48? Really?

I'd point DrMemory at it, but unfortunately it only supports 32-bit
applications so far. I don't have access to any of the commerical tools
like Purify. Maybe someone at EDB can help out with that, if you guys do?

Register states are:

RAX = 0043F7B0 RBX = 09E32218 RCX = 09E78510 RDX =
09E7ABD0 RSI = 09E78510 RDI = 09E32218 R8  =
09E7B3F8 R9  = 09E7B1E8 R10 = 09E7A9C0 R11 =
0001 R12 = 09E32200 R13 =  R14 =
09E7B1E8 R15 =  RIP = 0001401A59D1 RSP =
0043F6E0 RBP = 09E7A9C0 EFL = 00010202

and the exact crash site is

fkselec = get_foreign_key_join_selectivity(root,
  outer_rel->relids,
  inner_rel->relids,
  sjinfo,
  );
0001401A59AB  mov r8,qword ptr [r8+8]
0001401A59AF  mov rdx,qword ptr [rdx+8]
0001401A59B3  movaps  xmmword ptr [rax-28h],xmm6
0001401A59B7  movaps  xmmword ptr [rax-38h],xmm7
0001401A59BB  movaps  xmmword ptr [rax-48h],xmm8
0001401A59C0  movaps  xmmword ptr [rax-58h],xmm9
0001401A59C5  lea rax,[rax+38h]
0001401A59C9  movaps  xmm7,xmm3
0001401A59CC  mov qword ptr [rsp+20h],rax
0001401A59D1  movaps  xmmword ptr [rax-68h],xmm10 < here
0001401A59D6  mov qword ptr [rax-48h],r14
0001401A59DA  mov r14,qword ptr [sjinfo]
0001401A59E2  mov ebp,dword ptr [r14+28h]
0001401A59E6  mov