Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

[Peter Geoghegan]

On Fri, Jul 1, 2016 at 10:01 PM, Noah Misch  wrote:
>> I'm happy that I can do at least that much, but I see no reason to not
>> go significantly further.
>
> Don't risk bundling tests for other sorting scenarios.  A minimal test for the
> bug in question helps to qualify your patch as an exemplary bug fix.  Broader
> test coverage evicts your patch from that irreproachable category, inviting
> reviewers to determine that you went too far.

I have no intention of attempting to shoehorn tests for other sorting
scenarios into this bugfix. But, I will wait for Robert to comment on
what I've said before acting.

We should be more ambitious about adding test coverage to tuplesort.c.

-- 
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 batch tuplesort memory CLUSTER case (9.6 only)

[Noah Misch]

On Fri, Jul 01, 2016 at 09:12:42PM -0700, Peter Geoghegan wrote:
> On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch  wrote:
> > PostgreSQL is open to moving features from zero test coverage to nonzero 
> > test
> > coverage.  The last several releases have each done so.  Does that
> > sufficiently clarify the policy landscape?
> 
> Not quite, no. There are costs associated with adding regression tests
> that exercise external sorting. What are the costs, and how are they
> felt? How can we add more extensive test coverage without burdening
> those that run extremely slow buildfarm animals, for example?

Project policy is silent on those matters.  If your test performs one sort on
a table <3 MiB, I expect no trouble.

> > I'll guess that Robert would prefer a minimal test in cluster.sql over
> > starting a new file.  If you want to be sure, wait for his input.
> 
> There are significant advantages to a tuplesort.sql file. It gives us
> an easy way to avoid running the tests where they are inappropriate.
> Also, there are reasons to prefer a datum or heap tuplesort for
> testing certain facets of external sorting: setting work_mem to 64KiB
> can allow a test that exercises multiple polyphase merge passes and is
> relative fast (fast compared to a CLUSTER-based test, with 1MiB of
> maintenance_work_mem, say).
> 
> It's also true that there are special considerations for both hash
> tuplesorts and datum tuplesorts. As recently as a week ago, hash
> tuplesorts were fixed by Tom, having been *completely* broken for
> about one week shy of an entire year. I plan to address that
> separately, per discussion with Tom.
> 
> > I don't know, either.  You read both Robert and me indicating that this bug
> > fix will be better with a test, and nobody has said that a test-free fix is
> > optimal.  Here's your chance to obliterate that no-tests precedent.
> 
> I'm happy that I can do at least that much, but I see no reason to not
> go significantly further.

Don't risk bundling tests for other sorting scenarios.  A minimal test for the
bug in question helps to qualify your patch as an exemplary bug fix.  Broader
test coverage evicts your patch from that irreproachable category, inviting
reviewers to determine that you went too far.

nm


-- 
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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Pavel Stehule]

Hi

2016-07-01 20:46 GMT+02:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 7/1/16 7:06 AM, Craig Ringer wrote:
>
>> Yeah, but since when has the SQL standard adopted any existing
>> implementation's spelling of a feature? It seems to be politically
>> impossible.
>>
>
> The SQL/JSON thing is pretty much straight from Oracle and Microsoft (and
> notably completely different from DB2).
>

I checked standard, and it looks like not a significant problem to
implement it in Postgres. The implementation should be similar to XML -
requires parser support. We doesn't use a identifier of important SQL/JSON
functions: JSON_EXISTS, JSON_VALUE, JSON_QUERY, JSON_TABLE, JSON_ARRAY. The
problem should be with function JSON_OBJECT - standard is based on variadic
function of pairs (name, value), but it should be solvable, because our
first argument is a array, what is not possible in standard. So ANSI SQL
conform implementation of JSON support is still possible in Postgres.

Regards

Pavel





>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] fixing subplan/subquery confusion

[Amit Kapila]

On Sat, Jul 2, 2016 at 12:29 AM, Robert Haas  wrote:
> On Thu, Jun 30, 2016 at 6:49 PM, Robert Haas  wrote:
>> I haven't had a chance to do this yet, so I'm going to do it tomorrow 
>> instead.
>
> I dug into this a bit and found more problems.  I wondered why Tom's
> patch did this:
>
> !   if (has_parallel_hazard((Node *) rte->subquery, 
> false))
> !   return;
> !   break;
>
> Instead of just doing this:
>
> -return;
> +break;
>
> After all, the code that built subquery paths ought to be sufficient
> to find any parallel hazards during subquery planning; there seems to
> be no especially-good reason why we should walk the whole query tree
> searching for the again.  So I changed that and ran the regression
> tests.  With force_parallel_mode=on, things got unhappy on exactly one
> query:
>
>   SELECT * FROM
>   (SELECT a || b AS ab FROM t1
>UNION ALL
>SELECT ab FROM t2) t
>   ORDER BY 1 LIMIT 8;
>
> This failed with a complaint about parallel workers trying to touch
> temporary relations, which turns out to be pretty valid since t1 and
> t2 are BOTH temporary relations.  This turns out to be another facet
> of my ignorance about how subqueries can be pulled up to create
> appendrels with crazy things in them.  set_rel_consider_parallel()
> looks at the appendrel and thinks everything is fine, because the
> reference to temporary tables are buried inside the appendrel members,
> which are note examined.
>
> I think it's hard to avoid the conclusion that
> set_rel_consider_parallel() needs to run on other member rels as well
> as baserels.  We might be able to do that only for cases where the
> parent is a subquery RTE, since if the parent is a baserel then I
> think we must have just a standard inheritance hierarchy and things
> might be OK, but even then, I fear there might be problems with RLS.
> Anyway, the attached patch solves the problem in a fairly "brute
> force" fashion.  We loop over all basrels and other member rels and
> set consider_parallel according to their properties.  Then, when
> setting base rel sizes, we clear consider_parallel for any parents if
> it's clear for any of the children.  Finally, before setting base rel
> pathlists for appendrel children, we clear the flag for the child if
> it's meanwhile been cleared for the parent.  Thus, the parents and
> children always agree and only consider parallel paths for any of the
> rels if they're all OK.  This seems a bit grotty to me so suggestions
> are welcome.
>

@@ -966,8 +985,9 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  continue;
  }
- /* Copy consider_parallel flag from parent. */
- childrel->consider_parallel = rel->consider_parallel;
+ /* If any childrel is not parallel-safe, neither is parent. */
+ if (!childrel->consider_parallel)
+ rel->consider_parallel = false;


Doing this way misses the point that adjust_appendrel_attrs() can
change the targetlist. We should consider it for parallel-safety after
it gets modified.  I think that point can be addressed if try to set
consider_parallel for child rels as I have done in patch [1].


  /*
+ * If the parent isn't eligible for parallelism, there's no point
+ * in considering it for the children.  (This might change someday
+ * if we have a way to build an Append plan where some of the child
+ * plans are forced to run in the parent and others can run in any
+ * process, but for now there's no point in expending cycles building
+ * childrel paths we can't use.)
+ */

+ if (!rel->consider_parallel)
+ childrel->consider_parallel = false;
+

What exactly makes Append plan to not able to run some of the child
nodes is other process?


[1] - 
https://www.postgresql.org/message-id/CAA4eK1Jg_GvaTEjJSaV5vZY6acDmi-B3iXWvdiXa%2BpUFbnkyTg%40mail.gmail.com

-- 
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] Bug in batch tuplesort memory CLUSTER case (9.6 only)

[Peter Geoghegan]

On Fri, Jul 1, 2016 at 8:37 PM, Noah Misch  wrote:
> Yes, please produce a patch version bearing a regression test that exercises
> the bug.  I feel it counts even if the you need Valgrind Memcheck to see that
> it exercises the bug, but I won't interfere if Robert disagrees.  The test
> should take moderate steps to minimize costs.  For example, it should decrease
> maintenance_work_mem, not sort >16 MiB of data.

I agree with both points. I think that we can do just as well with a
1MiB maintenance_work_mem setting.

> PostgreSQL is open to moving features from zero test coverage to nonzero test
> coverage.  The last several releases have each done so.  Does that
> sufficiently clarify the policy landscape?

Not quite, no. There are costs associated with adding regression tests
that exercise external sorting. What are the costs, and how are they
felt? How can we add more extensive test coverage without burdening
those that run extremely slow buildfarm animals, for example?

>> I think that a new tuplesort.sql test file is where a test like this
>> belongs (not in the existing cluster.sql test file). If I write a
>> patch along those lines, is it ever going to be accepted? I would be
>> happy to work on this, but we need to have a sensible conversation on
>> what's acceptable to everyone in this area first. I've screamed bloody
>> murder about the test coverage in this area before, and nothing
>> happened.
>
> I'll guess that Robert would prefer a minimal test in cluster.sql over
> starting a new file.  If you want to be sure, wait for his input.

There are significant advantages to a tuplesort.sql file. It gives us
an easy way to avoid running the tests where they are inappropriate.
Also, there are reasons to prefer a datum or heap tuplesort for
testing certain facets of external sorting: setting work_mem to 64KiB
can allow a test that exercises multiple polyphase merge passes and is
relative fast (fast compared to a CLUSTER-based test, with 1MiB of
maintenance_work_mem, say).

It's also true that there are special considerations for both hash
tuplesorts and datum tuplesorts. As recently as a week ago, hash
tuplesorts were fixed by Tom, having been *completely* broken for
about one week shy of an entire year. I plan to address that
separately, per discussion with Tom.

> I don't know, either.  You read both Robert and me indicating that this bug
> fix will be better with a test, and nobody has said that a test-free fix is
> optimal.  Here's your chance to obliterate that no-tests precedent.

I'm happy that I can do at least that much, but I see no reason to not
go significantly further.

-- 
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 batch tuplesort memory CLUSTER case (9.6 only)

[Noah Misch]

On Fri, Jul 01, 2016 at 08:09:07PM -0700, Peter Geoghegan wrote:
> On Fri, Jul 1, 2016 at 7:05 PM, Noah Misch  wrote:
> > I think such a test would suffice to cover this bug if Valgrind Memcheck 
> > does
> > detect the problem in it.
> 
> Are you asking me to produce a regression test that exercises the bug?

Yes, please produce a patch version bearing a regression test that exercises
the bug.  I feel it counts even if the you need Valgrind Memcheck to see that
it exercises the bug, but I won't interfere if Robert disagrees.  The test
should take moderate steps to minimize costs.  For example, it should decrease
maintenance_work_mem, not sort >16 MiB of data.

> I would be happy to do so, but I require some clarification on project
> policy first. As I already mentioned, we currently have *precisely*
> zero test coverage of external sorting. Apparently, avoiding external
> sort operations in the regression tests has value.

PostgreSQL is open to moving features from zero test coverage to nonzero test
coverage.  The last several releases have each done so.  Does that
sufficiently clarify the policy landscape?

> I think that a new tuplesort.sql test file is where a test like this
> belongs (not in the existing cluster.sql test file). If I write a
> patch along those lines, is it ever going to be accepted? I would be
> happy to work on this, but we need to have a sensible conversation on
> what's acceptable to everyone in this area first. I've screamed bloody
> murder about the test coverage in this area before, and nothing
> happened.

I'll guess that Robert would prefer a minimal test in cluster.sql over
starting a new file.  If you want to be sure, wait for his input.

> If you think I'm overstating things, then consider how many commits
> that touched tuplesort.c added any tests. Even the commit "Fix
> inadequately-tested code path in tuplesort_skiptuples()." didn't add
> tests! There is scarcely any example of anyone deliberately adding
> test coverage to that file. I don't understand why it is so.

I don't know, either.  You read both Robert and me indicating that this bug
fix will be better with a test, and nobody has said that a test-free fix is
optimal.  Here's your chance to obliterate that no-tests precedent.


-- 
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] Reviewing freeze map code

[Amit Kapila]

On Fri, Jul 1, 2016 at 7:52 PM, Masahiko Sawada  wrote:
> On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila  wrote:
>>
>> Why do you think IndexOnlyScan will return wrong result?  If the
>> server crash in the way as you described, the transaction that has
>> made modifications will anyway be considered aborted, so the result of
>> IndexOnlyScan should not be wrong.
>>
>
> Ah, you're right, I misunderstood.
>
> Attached updated patch incorporating your comments.
> I've changed it so that heap_xlog_lock clears vm flags if page is
> marked all frozen.
>

I think we should make a similar change in heap_lock_tuple API as
well.  Also, currently by default heap_xlog_lock tuple tries to clear
the visibility flags, isn't it better to handle it as we do at all
other places (ex. see log_heap_update), by logging the information
about same.   I think it is always advisable to log every action we
want replay to perform.  That way, it is always easy to extend it
based on if some change is required only in certain cases, but not in
others.

Though, it is important to get the patch right, but I feel in the
meantime, it might be better to start benchmarking.  AFAIU, even if
change some part of information while WAL logging it, the benchmark
results won't be much different.


-- 
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] Reviewing freeze map code

[Amit Kapila]

On Sat, Jul 2, 2016 at 12:53 AM, Andres Freund  wrote:
> On 2016-07-01 15:18:39 -0400, Robert Haas wrote:
>> On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada  
>> wrote:
>> > Ah, you're right, I misunderstood.
>> >
>> > Attached updated patch incorporating your comments.
>> > I've changed it so that heap_xlog_lock clears vm flags if page is
>> > marked all frozen.
>>
>> I believe that this should be separated into two patches, since there
>> are two issues here:
>>
>> 1. Locking a tuple doesn't clear the all-frozen bit, but needs to do so.
>> 2. heap_update releases the buffer content lock without logging the
>> changes it has made.
>>
>> With respect to #1, there is no need to clear the all-visible bit,
>> only the all-frozen bit.  However, that's a bit tricky given that we
>> removed PD_ALL_FROZEN.  Should we think about putting that back again?
>
> I think it's fine to just do the vm lookup.
>
>> Should we just clear all-visible and call it good enough?
>
> Given that we need to do that in heap_lock_tuple, which entirely
> preserves all-visible (but shouldn't preserve all-frozen), ISTM we
> better find something that doesn't invalidate all-visible.
>

Sounds logical, considering that we have a way to set all-frozen and
vacuum does that as well.  So probably either we need to have a new
API or add a new parameter to visibilitymap_clear() to indicate the
same.  If we want to go that route, isn't it better to have
PD_ALL_FROZEN as well?


-- 
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] Bug in batch tuplesort memory CLUSTER case (9.6 only)

[Peter Geoghegan]

On Fri, Jul 1, 2016 at 7:05 PM, Noah Misch  wrote:
> I think such a test would suffice to cover this bug if Valgrind Memcheck does
> detect the problem in it.

Are you asking me to produce a regression test that exercises the bug?

I would be happy to do so, but I require some clarification on project
policy first. As I already mentioned, we currently have *precisely*
zero test coverage of external sorting. Apparently, avoiding external
sort operations in the regression tests has value. I attempted to
address this with amcheck, which had extensive tests for B-Tree index
builds in its standard regression tests (including coverage of
collatable types; testing using amcheck conveniently made the tests
portable). The theory there was that as a contrib module, amcheck's
regression tests could be run less frequently by hackers, possibly
very infrequently by having a special test_decoding style target.

Unfortunately, no committer took an interest in amcheck at the time,
so that has yet to go anywhere. If things had gone differently there,
it would be pretty straightforward to add a few CLUSTER commands to
those tests. Still, it probably makes sense to have a dedicated
tuplesort regression test suite, that is (through whatever mechanism)
only run selectively on certain buildfarm animals that don't have slow
I/O subsystems. The tests must also be easily avoidable by hackers
when running tests on their development machines.

I think that a new tuplesort.sql test file is where a test like this
belongs (not in the existing cluster.sql test file). If I write a
patch along those lines, is it ever going to be accepted? I would be
happy to work on this, but we need to have a sensible conversation on
what's acceptable to everyone in this area first. I've screamed bloody
murder about the test coverage in this area before, and nothing
happened.

If you think I'm overstating things, then consider how many commits
that touched tuplesort.c added any tests. Even the commit "Fix
inadequately-tested code path in tuplesort_skiptuples()." didn't add
tests! There is scarcely any example of anyone deliberately adding
test coverage to that file. I don't understand why it is so.

-- 
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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Andreas Karlsson]

On 07/01/2016 07:31 PM, Andres Freund wrote:

Yeah, but since when has the SQL standard adopted any existing
implementation's spelling of a feature? It seems to be politically
impossible.


Especially if nobody really lobbies on part of postgresql.


Has any of the major PostgreSQL companies looked into sending members to 
the ISO committee? It would be nice if we could be represented.


Andreas


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

Re: [HACKERS] Documentation fixes for pg_visibility

[Amit Kapila]

On Fri, Jul 1, 2016 at 9:30 PM, Robert Haas  wrote:
> On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila  wrote:
 Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
 when scanning each block, and then to issue a WARNING and clear the
 visibility map. Indeed that's better. I guess I need to take a closer
 look at vacuumlazy.c. See attached for example, but that's perhaps not
 something to have in 9.6 as that's more a micro-optimization than
 anything else.
>>>
>>> Right, something like that.  I think Andres actually wants something
>>> like this in 9.6, and I'm inclined to think it might be a good idea,
>>> too.  I think there should probably be a test for
>>> all_visible_according_to_vm at the beginning of that so that we don't
>>> add more visibility map checks for pages where we already know the VM
>>> bit can't possibly be set.
>>>
>>> Other opinions on the concept or the patch?
>>
>> +1 on the idea.
>>
>> + PageClearAllVisible(page);
>> + MarkBufferDirty(buf);
>>
>> What is the need to clear the Page level bit, if it is already
>> cleared, doesn't '!all_frozen' indicate that?
>
> No, I don't think so.  I think all_frozen indicates whether we think
> that all tuples on the page qualify as fully frozen.  I don't think it
> tells us anything about whether PD_ALL_VISIBLE is set on the page.
>

Then, can we decide to clear it on that basis?  Isn't it possible that
page is marked as all_visible, even if it contains frozen tuples?  In
the other nearby code (refer below part of code), we only clear the
page level bit after ensuring it is set.  Am I missing something?

else if (PageIsAllVisible(page) && has_dead_tuples)
{
elog(WARNING, "page containing dead tuples is marked as all-visible in
relation \"%s\" page %u",
relname, blkno);
PageClearAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_clear(onerel, blkno, vmbuffer);
}



-- 
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] Comment and function argument names are mismatched in bugmgr.c.

[Masahiko Sawada]

On Sat, Jul 2, 2016 at 6:34 AM, Andres Freund  wrote:
> Hi,
>
> On 2016-06-23 16:24:12 +0900, Masahiko Sawada wrote:
>> By commit 428b1d6b, function WritebackContextInit is added but the
>> comment of this function seems to be incorrect.
>> *max_coalesce variable doesn't exist at anywhere.
>> Also, I think it should be fixed that the argument name of this
>> function does not match function declare in buf_internal.h.
>> Patch for that attached.
>
> Fix looks good to me, and your 'bugmgr.c' typo in $subject made me laugh
> ;). Pushed.

Oops, I realized it now :)
Thanks!

Regards,

--
Masahiko Sawada


-- 
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 batch tuplesort memory CLUSTER case (9.6 only)

[Noah Misch]

On Fri, Jul 01, 2016 at 12:37:23PM -0700, Peter Geoghegan wrote:
> On Fri, Jul 1, 2016 at 12:30 PM, Peter Geoghegan  wrote:
> > Checkout my gensort tool from github. Build the C tool with "make".
> > Then, from the working directory:
> >
> > ./postgres_load.py -m 250 --skew --logged
> > psql -c "CREATE INDEX segfaults on sort_test_skew(sortkey);"
> > psql -c "CLUSTER sort_test_skew USING segfaults;"
> 
> I think that almost any CLUSTER based on an external sort would show
> problems with valgrind memcheck, though. That's probably far easier.

I think such a test would suffice to cover this bug if Valgrind Memcheck does
detect the problem in it.


-- 
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] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 07/02/2016 02:45 AM, Andreas Karlsson wrote:

On 07/02/2016 02:28 AM, Alvaro Herrera wrote:

Generally, version number tests sprinkled all over the place are not
terribly nice.  I think it would be better to get configure to define a
symbol like HAVE_BIO_METH_NEW.  Not sure about the other hunks in this
patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.


Agreed, that it is not nice. I followed what the previous code did, but
I do not like the inflation of this kind of #ifs with my OpenSSL 1.1
patches. I will try to see if I can figure out some good symbols.

Essentially the API changes which require ifdefs are:

- Opaque struts (we see an example above with the BIO struct)
- Renaming of RAND_SSLeay()
- Deprecation of DH_generate_parameters()
- Automatic initialization
- Automatic handling of threading

I do not like the idea of having a define per struct they have made
opaque in 1.1, but I think one define for all structs could be fine
(something like HAVE_OPENSSL_OPAQUE_STRUCTS). What do you think?


Looking at my code again I noticed it is just the BIO and BIO_METHOD 
structs which needed #ifs. The rest could be handled with changing the 
code to work in both old and new versions. If it is just two structs it 
might be fine to have two symbols, hm ..


Andreas


--
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] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 07/02/2016 02:28 AM, Alvaro Herrera wrote:

 static BIO_METHOD *
 my_BIO_s_socket(void)
 {
-   if (!my_bio_initialized)
+   if (!my_bio_methods)
{
-   memcpy(_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
-   my_bio_methods.bread = my_sock_read;
-   my_bio_methods.bwrite = my_sock_write;
-   my_bio_initialized = true;
+   BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket();
+#if SSLEAY_VERSION_NUMBER >= 0x1010L
+   my_bio_methods = BIO_meth_new(BIO_TYPE_SOCKET, "pgsocket");
+   BIO_meth_set_write(my_bio_methods, my_sock_write);
+   BIO_meth_set_read(my_bio_methods, my_sock_read);
+   BIO_meth_set_gets(my_bio_methods, BIO_meth_get_gets(biom));
+   BIO_meth_set_ctrl(my_bio_methods, BIO_meth_get_ctrl(biom));
+   BIO_meth_set_create(my_bio_methods, BIO_meth_get_create(biom));
+   BIO_meth_set_destroy(my_bio_methods, 
BIO_meth_get_destroy(biom));
+   BIO_meth_set_callback_ctrl(my_bio_methods, 
BIO_meth_get_callback_ctrl(biom));
+#else
+   my_bio_methods = malloc(sizeof(BIO_METHOD));
+   memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
+   my_bio_methods->bread = my_sock_read;
+   my_bio_methods->bwrite = my_sock_write;
+#endif


Generally, version number tests sprinkled all over the place are not
terribly nice.  I think it would be better to get configure to define a
symbol like HAVE_BIO_METH_NEW.  Not sure about the other hunks in this
patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.


Agreed, that it is not nice. I followed what the previous code did, but 
I do not like the inflation of this kind of #ifs with my OpenSSL 1.1 
patches. I will try to see if I can figure out some good symbols.


Essentially the API changes which require ifdefs are:

- Opaque struts (we see an example above with the BIO struct)
- Renaming of RAND_SSLeay()
- Deprecation of DH_generate_parameters()
- Automatic initialization
- Automatic handling of threading

I do not like the idea of having a define per struct they have made 
opaque in 1.1, but I think one define for all structs could be fine 
(something like HAVE_OPENSSL_OPAQUE_STRUCTS). What do you think?


Andreas


--
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] OpenSSL 1.1 breaks configure and more

[Alvaro Herrera]

Thanks for this effort.

>  static BIO_METHOD *
>  my_BIO_s_socket(void)
>  {
> - if (!my_bio_initialized)
> + if (!my_bio_methods)
>   {
> - memcpy(_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
> - my_bio_methods.bread = my_sock_read;
> - my_bio_methods.bwrite = my_sock_write;
> - my_bio_initialized = true;
> + BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket();
> +#if SSLEAY_VERSION_NUMBER >= 0x1010L
> + my_bio_methods = BIO_meth_new(BIO_TYPE_SOCKET, "pgsocket");
> + BIO_meth_set_write(my_bio_methods, my_sock_write);
> + BIO_meth_set_read(my_bio_methods, my_sock_read);
> + BIO_meth_set_gets(my_bio_methods, BIO_meth_get_gets(biom));
> + BIO_meth_set_ctrl(my_bio_methods, BIO_meth_get_ctrl(biom));
> + BIO_meth_set_create(my_bio_methods, BIO_meth_get_create(biom));
> + BIO_meth_set_destroy(my_bio_methods, 
> BIO_meth_get_destroy(biom));
> + BIO_meth_set_callback_ctrl(my_bio_methods, 
> BIO_meth_get_callback_ctrl(biom));
> +#else
> + my_bio_methods = malloc(sizeof(BIO_METHOD));
> + memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
> + my_bio_methods->bread = my_sock_read;
> + my_bio_methods->bwrite = my_sock_write;
> +#endif

Generally, version number tests sprinkled all over the place are not
terribly nice.  I think it would be better to get configure to define a
symbol like HAVE_BIO_METH_NEW.  Not sure about the other hunks in this
patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.

-- 
Á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] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 07/01/2016 11:41 AM, Christoph Berg wrote:

thanks for the patches. I applied all there patches on top of HEAD
(10c0558f). The server builds and passes "make check", pgcrypto still
needs work, though:


Thanks, I had forgotten pgcrypto.

When fixing pgcrypto I noticed that the OpenSSL team has deprecated 
RAND_pseudo_bytes() and recommend using RAND_bytes() instead (see 
302d38e3f73d5fd2ba2fd30bb7798778cb9f18dd).


As far as I can tell the only difference is that RAND_bytes() adds an 
error to the error queue if there is not enough entropy for generating 
secure data. And since we already always use strong random with the 
Fortuna algorithm, why not just drop px_get_pseudo_random_bytes()? It 
feels like a potential security problem with to me unclear benefit.


I also found that client CA loading is broken in OpenSSL 1.1-pre5 
(reported as https://github.com/openssl/openssl/pull/1279). This might 
be good to be aware of when testing my patches.


Attached a new set of patches:

0001-Fixes-for-compiling-with-OpenSSL-1.1-v2.patch

The fixes necessary to build with OpenSSL 1.1. Mostly fixes surrounding 
direct access to struct fields.


0002-Remove-OpenSSL-1.1-deprecation-warnings-v2.patch

Fix deprecation warnings. Mostly trusting OpenSSL 1.1 to handle 
threading and initialization automatically.


0003-Remove-px_get_pseudo_random_bytes-v2.patch

Remove the px_get_pseudo_random_bytes() from pgcrypto. Also silcences 
deprecation warning about RAND_pseudo_bytes().


0004-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat-v2.patch

Useful if you want to play around with 
0001-Fixes-for-compiling-with-OpenSSL-1.1-v2.patch before they release a 
new version where CRYPTO_LOCK is added back. See 
https://github.com/openssl/openssl/issues/1260


Andreas
>From 59cb028a38c18cb2f8fff077d355a8a26ecce641 Mon Sep 17 00:00:00 2001
From: Andreas Karlsson 
Date: Tue, 28 Jun 2016 05:55:03 +0200
Subject: [PATCH 1/4] Fixes for compiling with OpenSSL 1.1

- Check for SSL_new now that SSL_library_init is a macro
- Do not access struct members directly
- RAND_SSLeay was renamed to RAND_OpenSSL()
---
 configure| 44 
 configure.in |  4 +--
 contrib/pgcrypto/openssl.c   | 28 
 contrib/sslinfo/sslinfo.c| 14 ++
 src/backend/libpq/be-secure-openssl.c| 39 +++-
 src/interfaces/libpq/fe-secure-openssl.c | 39 +++-
 6 files changed, 98 insertions(+), 70 deletions(-)

diff --git a/configure b/configure
index 1f42c8a..ca83738 100755
--- a/configure
+++ b/configure
@@ -9538,9 +9538,9 @@ else
   as_fn_error $? "library 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_library_init in -lssl" >&5
-$as_echo_n "checking for SSL_library_init in -lssl... " >&6; }
-if ${ac_cv_lib_ssl_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_new in -lssl" >&5
+$as_echo_n "checking for SSL_new in -lssl... " >&6; }
+if ${ac_cv_lib_ssl_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_check_lib_save_LIBS=$LIBS
@@ -9554,27 +9554,27 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   return 0;
 }
 _ACEOF
 if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_lib_ssl_SSL_library_init=yes
+  ac_cv_lib_ssl_SSL_new=yes
 else
-  ac_cv_lib_ssl_SSL_library_init=no
+  ac_cv_lib_ssl_SSL_new=no
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext conftest.$ac_ext
 LIBS=$ac_check_lib_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_library_init" >&5
-$as_echo "$ac_cv_lib_ssl_SSL_library_init" >&6; }
-if test "x$ac_cv_lib_ssl_SSL_library_init" = xyes; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_new" >&5
+$as_echo "$ac_cv_lib_ssl_SSL_new" >&6; }
+if test "x$ac_cv_lib_ssl_SSL_new" = xyes; then :
   cat >>confdefs.h <<_ACEOF
 #define HAVE_LIBSSL 1
 _ACEOF
@@ -9644,9 +9644,9 @@ else
   as_fn_error $? "library 'eay32' or 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_library_init" >&5
-$as_echo_n "checking for library containing SSL_library_init... " >&6; }
-if ${ac_cv_search_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_new" >&5
+$as_echo_n "checking for library containing SSL_new... " >&6; }
+if ${ac_cv_search_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_func_search_save_LIBS=$LIBS
@@ -9659,11 +9659,11 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 28 Jun 2016 11:06:24 +0200
Gilles Darold  wrote:

> Thank you very much for the patch review and please apologies this too
> long response delay. I was traveling since end of April and totally
> forgotten this patch. I have applied all your useful feedbacks on the
> patch and attached a new one (v4) to this email :

Hi Gilles,

My schedule is really full at the moment.  I'll get to this
when I have a bit of time.  If you get anxious please write
and I'll see about making faster progress.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


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

Re: [HACKERS] [sqlsmith] ERROR: plan should not reference subplan's variable

[Tom Lane]

Robert Haas  writes:
> On Fri, Jul 1, 2016 at 5:29 PM, Tom Lane  wrote:
>> ... but if I try to actually execute the query, it crashes at runtime,
>> apparently because the CTE has not been passed over to the parallel
>> worker.  Robert, is it expected that CTEs should be parallel-safe?
>> I'd have thought not.

> Not.  See the RTE_CTE case in set_rel_consider_parallel.

OK; but this is crashing anyway, because after const-simplification the
CTE isn't actually referenced anywhere.  But it still ends up as an
initplan attached to the top plan node.  This may have accidentally
failed to fail before you removed this bit in 5ce5e4a12:

if (glob->subplans != NULL)
glob->wholePlanParallelSafe = false;

I think in the current code, the equivalent thing is that we need to
consider that a plan node is parallel-unsafe once we've stuck some
initPlans on it (which happens only in createplan.c and so doesn't affect
the parallel_safe state of the source Path).  Will see if that helps.

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] [sqlsmith] ERROR: plan should not reference subplan's variable

[Robert Haas]

On Fri, Jul 1, 2016 at 5:29 PM, Tom Lane  wrote:
> Andreas Seltenreich  writes:
>> Updating master from f8c5855..1bdae16, sqlsmith triggers "failed to
>> generate plan" errors again.  Below is the smallest query logged so far.
>
> Hmm, interesting.  This can be reduced to
>
> set force_parallel_mode = on;
>
> explain
> with j1 as (select * from int8_tbl)
> select * from int4_tbl
>   where false and EXISTS (select 1 as c0 from j1);
>
> The "plan should not reference subplan's variable" fail seems to be due
> to ancient fuzzy thinking in SS_finalize_plan.  When I fix that, I get
> a plan like so:
>
>  Gather  (cost=0.00..0.00 rows=0 width=0)
>Workers Planned: 1
>Single Copy: true
>->  Result  (cost=1.05..1.05 rows=0 width=4)
>  One-Time Filter: false
>  CTE j1
>->  Seq Scan on int8_tbl  (cost=0.00..1.05 rows=5 width=16)
>
> but if I try to actually execute the query, it crashes at runtime,
> apparently because the CTE has not been passed over to the parallel
> worker.  Robert, is it expected that CTEs should be parallel-safe?
> I'd have thought not.

Not.  See the RTE_CTE case in set_rel_consider_parallel.

-- 
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] Broken handling of lwlocknames.h

[Michael Paquier]

On Sat, Jul 2, 2016 at 4:13 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Yes that would be indeed cleaner this way. I have poked a bit at that
>> and finished with the attached that defines some rules to generate all
>> the files needed.
>
> I made some mostly-cosmetic changes to this and pushed it.  One thing
> to note is that it seemed to me you'd broken the rule for schemapg.h:
> by removing the phony target, I think you removed the behavior that
> we'd always go and recheck schemapg.h's dependencies.  To do it correctly
> without that target, we'd need src/backend/Makefile to know all of those
> dependencies, duplicating the rather long list in catalog/Makefile.

Thanks. Yes the bug that I was mentioning yesterday was that I missed
one place where the target of schemapg.h was referenced, and I didn't
take the time to fully cover this set of dependencies..
-- 
Michael


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

Re: [HACKERS] pg_replication_origin_xact_reset() and its argument variables

[Tom Lane]

Andres Freund  writes:
> On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
>> Fujii Masao  writes:
>>> As far as I read the code of the function, those arguments don't seem to
>>> be necessary. So I'm afraid that the pg_proc entry for the function might
>>> be incorrect and those two arguments should be removed from the definition.

>> Sure looks that way from here.  Copy-and-paste from the previous
>> line in pg_proc.h, perhaps?

> Yes, that's clearly wrong. Damn.  Can't fix that for 9.5 anymore. The
> function isn't all that important (especially not from SQL), but still,
> that's annoying.  I'm inclined to just remove the args in 9.6. We could
> also add a note to the 9.5 docs, adding that the arguments are there by
> error?

Yeah, seems like the best thing to do.

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] pg_replication_origin_xact_reset() and its argument variables

[Andres Freund]

On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
> Fujii Masao  writes:
> > The document explains that pg_replication_origin_xact_reset() doesn't have
> > any argument variables. But it's been actually defined so as to have two
> > argument variables with pg_lsn and timestamptz data types, as follows.
> 
> > =# \df pg_replication_origin_xact_reset
> >   List of functions
> >Schema   |   Name   | Result data type |
> >Argument data types|  Type
> > +--+--+--+
> >  pg_catalog | pg_replication_origin_xact_reset | void |
> > pg_lsn, timestamp with time zone | normal
> 
> > As far as I read the code of the function, those arguments don't seem to
> > be necessary. So I'm afraid that the pg_proc entry for the function might
> > be incorrect and those two arguments should be removed from the definition.
> > Is this analysis right?
> 
> Sure looks that way from here.  Copy-and-paste from the previous
> line in pg_proc.h, perhaps?

Yes, that's clearly wrong. Damn.  Can't fix that for 9.5 anymore. The
function isn't all that important (especially not from SQL), but still,
that's annoying.  I'm inclined to just remove the args in 9.6. We could
also add a note to the 9.5 docs, adding that the arguments are there by
error?

Andres


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

Re: [HACKERS] Comment and function argument names are mismatched in bugmgr.c.

[Andres Freund]

Hi,

On 2016-06-23 16:24:12 +0900, Masahiko Sawada wrote:
> By commit 428b1d6b, function WritebackContextInit is added but the
> comment of this function seems to be incorrect.
> *max_coalesce variable doesn't exist at anywhere.
> Also, I think it should be fixed that the argument name of this
> function does not match function declare in buf_internal.h.
> Patch for that attached.

Fix looks good to me, and your 'bugmgr.c' typo in $subject made me laugh
;). Pushed.

Thanks Robert, for pointing me to the thread, I had indeed missed it.

Greetings,

Andres Freund


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

Re: [HACKERS] [sqlsmith] ERROR: plan should not reference subplan's variable

[Tom Lane]

Andreas Seltenreich  writes:
> Updating master from f8c5855..1bdae16, sqlsmith triggers "failed to
> generate plan" errors again.  Below is the smallest query logged so far.

Hmm, interesting.  This can be reduced to

set force_parallel_mode = on;

explain
with j1 as (select * from int8_tbl)
select * from int4_tbl
  where false and EXISTS (select 1 as c0 from j1);

The "plan should not reference subplan's variable" fail seems to be due
to ancient fuzzy thinking in SS_finalize_plan.  When I fix that, I get
a plan like so:

 Gather  (cost=0.00..0.00 rows=0 width=0)
   Workers Planned: 1
   Single Copy: true
   ->  Result  (cost=1.05..1.05 rows=0 width=4)
 One-Time Filter: false
 CTE j1
   ->  Seq Scan on int8_tbl  (cost=0.00..1.05 rows=5 width=16)

but if I try to actually execute the query, it crashes at runtime,
apparently because the CTE has not been passed over to the parallel
worker.  Robert, is it expected that CTEs should be parallel-safe?
I'd have thought not.

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] _mdfd_getseg can be expensive

[Peter Geoghegan]

On Thu, Jun 30, 2016 at 7:08 PM, Andres Freund  wrote:
> If you have a big enough index (maybe ~150GB+), sure. Before that,
> probably not.
>
> It's usually pretty easy to see in cpu profiles whether this issue
> exists.

I think that this is a contributing factor to why merging in parallel
CREATE INDEX becomes much more CPU bound when building such very large
indexes, which Corey Huinker has benchmarked using an advanced copy of
the patch. He has shown cases that are sped up by 3.6x when 8 parallel
workers are used (compared to a serial CREATE INDEX), but a several
hundred gigabyte index case only sees a speedup of about 1.5x. (This
bottleneck affects serial CREATE INDEX merging just as much as
parallel, since that part isn't parallelized, but it's far more
noticeable with parallel CREATE INDEX simply because merging in the
leader becomes a huge bottleneck).

Those two cases were not exactly comparable in perhaps several other
ways, but even still my sense is that that this can be at least
partially explained by md.c bottlenecks. This is something that we'll
need to confirm through profiling. Hopefully it's just this one
bottleneck.

--
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] Reviewing freeze map code

[Jim Nasby]

On 7/1/16 3:43 PM, Andres Freund wrote:

On 2016-07-01 15:42:22 -0500, Jim Nasby wrote:

On 7/1/16 2:23 PM, Andres Freund wrote:

The only
cost of that is that vacuum will come along and mark the page
all-visible again instead of skipping it, but that's probably not an
enormous expense in most cases.

I think the main cost is not having the page marked as all-visible for
index-only purposes. If it's an insert mostly table, it can be a long
while till vacuum comes around.


ISTM that's something that should be addressed anyway (and separately), no?


Huh? That's the current behaviour in heap_lock_tuple.


Oh, I was referring to autovac not being aggressive enough on 
insert-mostly tables. Certainly if there's a reasonable way to avoid 
invalidating the VM when locking a tuple that'd be good.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] Reviewing freeze map code

[Andres Freund]

On 2016-07-01 15:42:22 -0500, Jim Nasby wrote:
> On 7/1/16 2:23 PM, Andres Freund wrote:
> > > > The only
> > > > cost of that is that vacuum will come along and mark the page
> > > > all-visible again instead of skipping it, but that's probably not an
> > > > enormous expense in most cases.
> > I think the main cost is not having the page marked as all-visible for
> > index-only purposes. If it's an insert mostly table, it can be a long
> > while till vacuum comes around.
> 
> ISTM that's something that should be addressed anyway (and separately), no?

Huh? That's the current behaviour in heap_lock_tuple.


-- 
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] Reviewing freeze map code

[Jim Nasby]

On 7/1/16 2:23 PM, Andres Freund wrote:

> The only
> cost of that is that vacuum will come along and mark the page
> all-visible again instead of skipping it, but that's probably not an
> enormous expense in most cases.

I think the main cost is not having the page marked as all-visible for
index-only purposes. If it's an insert mostly table, it can be a long
while till vacuum comes around.


ISTM that's something that should be addressed anyway (and separately), no?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] Bad behavior from plpython 'return []'

[Jim Nasby]

On 7/1/16 2:52 PM, Tom Lane wrote:

+   /* if caller tries to specify zero-length array, make it empty */
+   if (nelems <= 0)
+   return construct_empty_array(elmtype);
+
/* compute required space */
nbytes = 0;
hasnulls = false;

But that might introduce new problems too, if any callers expect the
array dimensions to be exactly what they asked for.


You mean ndims? What if instead of an empty array it returned an array 
where *dims was just all zeros (and correctly set *lbs)? array_eq would 
still need to account for that, but I think we don't have a choice about 
that unless we expressly forbid arrays where any of the elements of 
*dims were 0 (which I suspect we should probably do anyway... I don't 
see how you can do anything with a 2x0x3 array...)

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] Bad behavior from plpython 'return []'

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 30, 2016 at 9:25 PM, Jim Nasby  wrote:
>> SELECT array_dims(pg_temp.bad()), array_dims('{}'::text[]);
>> array_dims | array_dims
>> +
>> [1:0]  |
>> (1 row)

> Yeah, that's a bug.

It looks like this is because PLySequence_ToArray neglects to special-case
zero-element arrays.  We could fix it there, but this is not the first
such bug.  I wonder if we should change construct_md_array to force
zero-element arrays to be converted to empty arrays, rather than assuming
callers will have short-circuited the case earlier.  Something like

/* fast track for empty array */
if (ndims == 0)
return construct_empty_array(elmtype);

nelems = ArrayGetNItems(ndims, dims);

+   /* if caller tries to specify zero-length array, make it empty */
+   if (nelems <= 0)
+   return construct_empty_array(elmtype);
+
/* compute required space */
nbytes = 0;
hasnulls = false;

But that might introduce new problems too, if any callers expect the
array dimensions to be exactly what they asked for.

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 batch tuplesort memory CLUSTER case (9.6 only)

[Peter Geoghegan]

On Fri, Jul 1, 2016 at 12:30 PM, Peter Geoghegan  wrote:
> Checkout my gensort tool from github. Build the C tool with "make".
> Then, from the working directory:
>
> ./postgres_load.py -m 250 --skew --logged
> psql -c "CREATE INDEX segfaults on sort_test_skew(sortkey);"
> psql -c "CLUSTER sort_test_skew USING segfaults;"

I think that almost any CLUSTER based on an external sort would show
problems with valgrind memcheck, though. That's probably far easier.

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

[HACKERS] [sqlsmith] ERROR: plan should not reference subplan's variable

[Andreas Seltenreich]

Updating master from f8c5855..1bdae16, sqlsmith triggers "failed to
generate plan" errors again.  Below is the smallest query logged so far.

regards,
Andreas

-- ERROR:  plan should not reference subplan's variable
set force_parallel_mode = 'on';
set max_parallel_workers_per_gather = '1';

explain WITH
jennifer_0 AS (select
(select b from public.rtest_v1 limit 1 offset 5)
   as c0,
pg_catalog.pg_current_xlog_location() as c1,
sample_0.a as c2,
sample_0.a as c3
  from
public.rtest_view4 as sample_0 tablesample system (5.9)
  where cast(null as bigint) = pg_catalog.hashinet(
  cast((select client_addr from pg_catalog.pg_stat_activity limit 1 offset 
35)
 as inet))
  limit 76)
select
ref_0.sl_name as c0
  from
public.shoelace as ref_0
  where (cast(null as anyrange) < cast(null as anyrange))
and (EXISTS (
  select
  39 as c0
from
  jennifer_0 as ref_1
where cast(null as real) = cast(null as real)
limit 81))
  limit 96;


-- 
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 batch tuplesort memory CLUSTER case (9.6 only)

[Peter Geoghegan]

On Fri, Jul 1, 2016 at 12:10 PM, Robert Haas  wrote:
>> I could give you steps to reproduce the bug, but they involve creating
>> a large table using my gensort tool [1]. It isn't trivial. Are you
>> interested?
>
> The bug can't very well be so simple that you need not include a set
> of steps to reproduce it and, at the same time, so complex that even
> so much as reading the list of steps to reproduce it might be more
> than I want to do.

Reading and following are two different things. I don't think reading
alone will do much good with the following steps, but here they are:

Checkout my gensort tool from github. Build the C tool with "make".
Then, from the working directory:

./postgres_load.py -m 250 --skew --logged
psql -c "CREATE INDEX segfaults on sort_test_skew(sortkey);"
psql -c "CLUSTER sort_test_skew USING segfaults;"

That test case isn't at all minimal, but that's how I happened upon
the bug. I didn't tell you this before now because I assumed that
you'd just accept that there was an omission made based on a quick
reading of the code. This is not a complicated bug; the pointer
HeapTuple.t_data needs to be updated when tuples are moved around in
memory, but isn't. readtup_cluster() initializes that field like this:

/* Reconstruct the HeapTupleData header */
tuple->t_data = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);

More or less the same process needs to occur following any movement of
the tuple. Whereas, in all other cases there is no need to do
something similar, as it happens.

-- 
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] Reviewing freeze map code

[Andres Freund]

On 2016-07-01 15:18:39 -0400, Robert Haas wrote:
> On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada  
> wrote:
> > Ah, you're right, I misunderstood.
> >
> > Attached updated patch incorporating your comments.
> > I've changed it so that heap_xlog_lock clears vm flags if page is
> > marked all frozen.
> 
> I believe that this should be separated into two patches, since there
> are two issues here:
> 
> 1. Locking a tuple doesn't clear the all-frozen bit, but needs to do so.
> 2. heap_update releases the buffer content lock without logging the
> changes it has made.
> 
> With respect to #1, there is no need to clear the all-visible bit,
> only the all-frozen bit.  However, that's a bit tricky given that we
> removed PD_ALL_FROZEN.  Should we think about putting that back again?

I think it's fine to just do the vm lookup.

> Should we just clear all-visible and call it good enough?

Given that we need to do that in heap_lock_tuple, which entirely
preserves all-visible (but shouldn't preserve all-frozen), ISTM we
better find something that doesn't invalidate all-visible.


> The only
> cost of that is that vacuum will come along and mark the page
> all-visible again instead of skipping it, but that's probably not an
> enormous expense in most cases.

I think the main cost is not having the page marked as all-visible for
index-only purposes. If it's an insert mostly table, it can be a long
while till vacuum comes around.

Andres


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

Re: [HACKERS] Reviewing freeze map code

[Robert Haas]

On Fri, Jul 1, 2016 at 10:22 AM, Masahiko Sawada  wrote:
> Ah, you're right, I misunderstood.
>
> Attached updated patch incorporating your comments.
> I've changed it so that heap_xlog_lock clears vm flags if page is
> marked all frozen.

I believe that this should be separated into two patches, since there
are two issues here:

1. Locking a tuple doesn't clear the all-frozen bit, but needs to do so.
2. heap_update releases the buffer content lock without logging the
changes it has made.

With respect to #1, there is no need to clear the all-visible bit,
only the all-frozen bit.  However, that's a bit tricky given that we
removed PD_ALL_FROZEN.  Should we think about putting that back again?
 Should we just clear all-visible and call it good enough?  The only
cost of that is that vacuum will come along and mark the page
all-visible again instead of skipping it, but that's probably not an
enormous expense in most cases.

-- 
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] Broken handling of lwlocknames.h

[Tom Lane]

Michael Paquier  writes:
> Yes that would be indeed cleaner this way. I have poked a bit at that
> and finished with the attached that defines some rules to generate all
> the files needed.

I made some mostly-cosmetic changes to this and pushed it.  One thing
to note is that it seemed to me you'd broken the rule for schemapg.h:
by removing the phony target, I think you removed the behavior that
we'd always go and recheck schemapg.h's dependencies.  To do it correctly
without that target, we'd need src/backend/Makefile to know all of those
dependencies, duplicating the rather long list in catalog/Makefile.

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 batch tuplesort memory CLUSTER case (9.6 only)

[Robert Haas]

On Fri, Jul 1, 2016 at 2:23 PM, Peter Geoghegan  wrote:
> On Fri, Jul 1, 2016 at 6:23 AM, Robert Haas  wrote:
>> The proposed patch contains no test case and no description of how to
>> reproduce the problem.  I am not very keen on the idea of trying to
>> puzzle that out from first principles.
>
> I thought that the bug was simple enough that it didn't require a
> testcase. Besides, as I've often complained about there are no tests
> of external  sorting in the regression test suite whatsoever. I don't
> think you'd just accept it now if I tried to add some.
>
> I could give you steps to reproduce the bug, but they involve creating
> a large table using my gensort tool [1]. It isn't trivial. Are you
> interested?

The bug can't very well be so simple that you need not include a set
of steps to reproduce it and, at the same time, so complex that even
so much as reading the list of steps to reproduce it might be more
than I want to do.

-- 
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] fixing subplan/subquery confusion

[Robert Haas]

On Thu, Jun 30, 2016 at 6:49 PM, Robert Haas  wrote:
> I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.

I dug into this a bit and found more problems.  I wondered why Tom's
patch did this:

!   if (has_parallel_hazard((Node *) rte->subquery, false))
!   return;
!   break;

Instead of just doing this:

-return;
+break;

After all, the code that built subquery paths ought to be sufficient
to find any parallel hazards during subquery planning; there seems to
be no especially-good reason why we should walk the whole query tree
searching for the again.  So I changed that and ran the regression
tests.  With force_parallel_mode=on, things got unhappy on exactly one
query:

  SELECT * FROM
  (SELECT a || b AS ab FROM t1
   UNION ALL
   SELECT ab FROM t2) t
  ORDER BY 1 LIMIT 8;

This failed with a complaint about parallel workers trying to touch
temporary relations, which turns out to be pretty valid since t1 and
t2 are BOTH temporary relations.  This turns out to be another facet
of my ignorance about how subqueries can be pulled up to create
appendrels with crazy things in them.  set_rel_consider_parallel()
looks at the appendrel and thinks everything is fine, because the
reference to temporary tables are buried inside the appendrel members,
which are note examined.

I think it's hard to avoid the conclusion that
set_rel_consider_parallel() needs to run on other member rels as well
as baserels.  We might be able to do that only for cases where the
parent is a subquery RTE, since if the parent is a baserel then I
think we must have just a standard inheritance hierarchy and things
might be OK, but even then, I fear there might be problems with RLS.
Anyway, the attached patch solves the problem in a fairly "brute
force" fashion.  We loop over all basrels and other member rels and
set consider_parallel according to their properties.  Then, when
setting base rel sizes, we clear consider_parallel for any parents if
it's clear for any of the children.  Finally, before setting base rel
pathlists for appendrel children, we clear the flag for the child if
it's meanwhile been cleared for the parent.  Thus, the parents and
children always agree and only consider parallel paths for any of the
rels if they're all OK.  This seems a bit grotty to me so suggestions
are welcome.

(Official status update: I'm not prepared to commit this without some
review.  So I intend to wait for a while and see whether I get some
review.  I realize these status updates are supposed to contain a date
by which further action will be taken, but I don't know how to
meaningfully do that in this type of case.)

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


more-allpaths-fixes-v1.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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Peter Eisentraut]

On 7/1/16 7:06 AM, Craig Ringer wrote:

Yeah, but since when has the SQL standard adopted any existing
implementation's spelling of a feature? It seems to be politically
impossible.


The SQL/JSON thing is pretty much straight from Oracle and Microsoft 
(and notably completely different from DB2).


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


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

Re: [HACKERS] Bug in batch tuplesort memory CLUSTER case (9.6 only)

[Peter Geoghegan]

On Fri, Jul 1, 2016 at 6:23 AM, Robert Haas  wrote:
> The proposed patch contains no test case and no description of how to
> reproduce the problem.  I am not very keen on the idea of trying to
> puzzle that out from first principles.

I thought that the bug was simple enough that it didn't require a
testcase. Besides, as I've often complained about there are no tests
of external  sorting in the regression test suite whatsoever. I don't
think you'd just accept it now if I tried to add some.

I could give you steps to reproduce the bug, but they involve creating
a large table using my gensort tool [1]. It isn't trivial. Are you
interested?

> Also, I would appreciate a
> clearer explanation of why this only affects CLUSTER tuplesorts.

As I said, it has the only type of caller tuple that happens to itself
contain a pointer to palloc()'d memory. Clearly that needs to be
updated if the tuple is moved, since it always points to an offset
into the same tuple. I thought that that explanation was simple.

[1] https://github.com/petergeoghegan/gensort
-- 
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] Broken handling of lwlocknames.h

[Tom Lane]

Michael Paquier  writes:
> Yes that would be indeed cleaner this way. I have poked a bit at that
> and finished with the attached that defines some rules to generate all
> the files needed. But actually it does not seem to be enough, for
> example on OSX this would fail to compile because it cannot find the
> postgres binary in src/backend/postgres. Does somebody have an idea
> what this is about? It seems that we have two problems here.

Yeah, on OS X, building regress.so (or any other loadable extension)
will fail unless you built the backend first:

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -O2  -bundle 
-multiply_defined suppress -Wl,-undefined,dynamic_lookup -o regress.so  
regress.o -L../../../src/port -L../../../src/common -Wl,-dead_strip_dylibs   
-bundle_loader ../../../src/backend/postgres
ld: file not found: ../../../src/backend/postgres

The reason is you need the "-bundle_loader postgres" option, because
OS X's linker is pickier about unresolved symbols than Linux's.

However, that is not a 9.6 regression: that's never worked on OS X, or at
least not since we started using -bundle_loader.

I do not think that we want the makefiles to enforce this build
dependency, as that would completely destroy any speed advantage of trying
to build just the particular .so.  Maybe we could enforce the dependency
just on OS X, but since we haven't gotten complaints from people trying
to build like this on OS X, I doubt it's worth the trouble.

So my inclination is to fix the include-file issue and call it good.
In any case, if someone did want to deal with making the -bundle_loader
calls safer, it would be material for a separate patch IMO.

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] Bad behavior from plpython 'return []'

[Robert Haas]

On Thu, Jun 30, 2016 at 9:25 PM, Jim Nasby  wrote:
> CREATE FUNCTION pg_temp.bad() RETURNS text[] LANGUAGE plpythonu AS $$return
> []$$;
> SELECT pg_temp.bad();
>  bad
> -
>  {}
> (1 row)
>
> SELECT pg_temp.bad() = '{}'::text[];
>  ?column?
> --
>  f
> (1 row)
>
> Erm?? Turns out this is because
>
> SELECT array_dims(pg_temp.bad()), array_dims('{}'::text[]);
>  array_dims | array_dims
> +
>  [1:0]  |
> (1 row)

Yeah, that's a bug.

-- 
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] primary_conninfo missing from pg_stat_wal_receiver

[Alvaro Herrera]

Michael Paquier wrote:
> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
>  wrote:

> >> Okay, that argument I buy.
> >>
> >> I suppose this function/view should report no row at all if there is no
> >> wal receiver connected, rather than a view with nulls.
> >
> > The function returns PG_RETURN_NULL() so as we don't have to use a
> > SRF, and the view checks for IS NOT NULL, so there would be no rows
> > popping up.
> 
> In short, I would just go with the attached and call it a day.

Done, thanks.

-- 
Á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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Andres Freund]

On 2016-07-01 19:06:06 +0800, Craig Ringer wrote:
> On 1 July 2016 at 17:52, Oleg Bartunov  wrote:
> 
> > On Wed, Jun 29, 2016 at 2:51 AM, Stefan Keller  wrote:
> > > Hi,
> > >
> > > FYI: I'd just like to point you to following two forthcoming standard
> > > parts from "ISO/IEC JTS 1/SC 32" comittee: one on JSON, and one on
> > > "Multi-Dimensional Arrays" (SQL/MDA).
> > >
> > > They define there some things different as already in PG. See also
> > > Peter Baumann's slides [1] and e.g. [2]
> >
> > I' very dissapointed with this.
> >
> 
> 
> Yeah, but since when has the SQL standard adopted any existing
> implementation's spelling of a feature? It seems to be politically
> impossible.

Especially if nobody really lobbies on part of postgresql.


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

[HACKERS] TLC for EXPLAIN ANALYZE (parallel query and loops)

[David G. Johnston]

https://www.postgresql.org/docs/9.6/static/using-explain.html

Existing...

14.1.2 Explain Analyze
[...]
"""
In some query plans, it is possible for a subplan node to be executed more
than once. For example, the inner index scan will be executed once per
outer row in the above nested-loop plan. In such cases, the loops value
reports the total number of executions of the node, and the actual time and
rows values shown are averages per-execution. This is done to make the
numbers comparable with the way that the cost estimates are shown. Multiply
by the loops value to get the total time actually spent in the node. In the
above example, we spent a total of 0.220 milliseconds executing the index
scans on tenk2.
"""

Additional topics to cover here (somewhere in this region)...

Parallel-executed queries executed in workers also result in an increase in
the number of loops.  The total loop count will be equal to the number of
workers used, plus 1 if the leader contributed to retrieving rows.  Note
that, presently, the leader's contributions are not detailed and can only
be imputed from the total for the node and the detail of the workers.
[other detail goes here - the whole block could be placed subsequent to the
inheritance example].


[Possibly make a shorter form of this into a note...]
A nested-loop execution will often result in exactly 1 row being returned
per loop.  In the parallel case, however, and especially when performing
parallel sequential scans with a highly-restrictive filter, it is possible
that few rows are returned.  For instance, a parallel sequential scan on a
unique value will return a single row but might, including the leader, use
3 scans/loops to perform the work.  In this case the average value per loop
would be 0.333+ - which is rounded down to zero since rows is expressed as
an integer.  In any case when loops > 1 it can be necessary (though not
always sufficient) to examine the parent node to discover the total number
of records returned by the child node.

I'm sure I have some level of imprecision here but hopefully this is enough
to start.

David J.

Re: [HACKERS] ToDo: API for SQL statement execution other than SPI

[Vibhor Kumar]

> On Jul 1, 2016, at 11:39 AM, Robert Haas  wrote:
> 
> On Wed, Jun 29, 2016 at 1:55 AM, Pavel Stehule  > wrote:
>> I am writing two background workers - autoreindex and scheduler. In Both I
>> need to execute queries from top level. I had to wrote redundant code
>> https://github.com/okbob/autoreindex/blob/master/utils.c 
>> 
>> autoreindex_execute_sql_command .Same code is in pglogical. Some statements
>> - like VACUUM or REINDEX CONCURRENTLY is not possible call from SPI.
>> 
>> Can be nice to have this function in core.
> 
> pg_background handles this.  Somebody might want to resurrect that patch.


Here is the link for that patch, which works with 9.5
https://github.com/vibhorkum/pg_background 


Thanks & Regards,
Vibhor Kumar
(EDB) EnterpriseDB Corporation
EnterpriseDB
Blog:http://vibhork.blogspot.com

Re: [HACKERS] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Alvaro Herrera]

Merlin Moncure wrote:

> It's pretty easy to craft a query where you're on the winning side,
> but what's the worst case of doing two pass...is constant folding a
> non trivial fraction of planning time?

One thing that has been suggested is to re-examine the plan after
planning is done, and if execution time is estimated to be large (FSVO),
then run a second planning pass with more expensive optimizations
enabled to try and find better plans.  The guiding principle would be to
continue to very quickly find good enough plans for
frequent/small/simple queries, but spend more planning effort on more
complex ones where execution is likely to take much longer than planning
time.

So doing constant-folding twice would be enabled for the second planning
pass.

-- 
Á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] The link to download PostgreSQL 9.6 Beta 2 for Windows X64 is broken (The link downloads Beta 1)

[Bruce Momjian]

On Fri, Jul  1, 2016 at 05:12:40PM +0100, Dave Page wrote:
> On Fri, Jul 1, 2016 at 4:07 PM, Jean-Pierre Pelletier
>  wrote:
> > The link to download PostgreSQL 9.6 Beta 2 for Windows X64
> > is not working.
> > The link does download something, but it's Beta 1.
> >
> > http://www.enterprisedb.com/products-services-training/pgdownload#windows
> 
> There is an issue with the Windows x64 build with the version of VC++
> that's used for the installers. I believe there was a fix committed
> for this yesterday by Tom.
> 
> The button was supposed to be removed until we get an updated build -
> apologies for the inconvenience.

Yes, and I reported it to EDB a few days ago too.

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

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


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

Re: [HACKERS] The link to download PostgreSQL 9.6 Beta 2 for Windows X64 is broken (The link downloads Beta 1)

[Dave Page]

On Fri, Jul 1, 2016 at 4:07 PM, Jean-Pierre Pelletier
 wrote:
> The link to download PostgreSQL 9.6 Beta 2 for Windows X64
> is not working.
> The link does download something, but it's Beta 1.
>
> http://www.enterprisedb.com/products-services-training/pgdownload#windows

There is an issue with the Windows x64 build with the version of VC++
that's used for the installers. I believe there was a fix committed
for this yesterday by Tom.

The button was supposed to be removed until we get an updated build -
apologies for the inconvenience.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: 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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Robert Haas]

On Fri, Jul 1, 2016 at 12:00 PM, Merlin Moncure  wrote:
> Sure (I didn't put you on that position, just thinking out loud).  The
> problem with UNION ALL is that it's only safe to do so when you know
> for sure the both sides of the partition are non-overlapping.  The
> author of the query often knows this going in but for the planner it's
> not so simple to figure out in many cases.  If there's a subset of
> cases.   UNION sans ALL is probably a dead end on performance grounds.

I'm not sure about that.  It's certainly true that things are much
more likely to work out when you can prove that UNION ALL is
sufficient, because now you avoid de-duplication.  But if the number
of output rows is really small, it might work out anyway.  I mean,
consider this:

SELECT * FROM enormous WHERE rarely_one = 1 OR EXISTS (SELECT 1 FROM
tiny WHERE tiny.x = enormous.x)

As written, you're not going to be able to answer this query without
scanning a full scan of the enormous table.  If you rewrite it to use
UNION, then the first half can be implemented with an index scan or a
bitmap index scan, and the second half can be implemented with a
nested loop over the tiny table with an inner index scan on the
enormous table.  The fact that you have to deduplicate the results may
be a small price to pay for avoiding an enormous scan.

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


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

[HACKERS] The link to download PostgreSQL 9.6 Beta 2 for Windows X64 is broken (The link downloads Beta 1)

[Jean-Pierre Pelletier]

The link to download PostgreSQL 9.6 Beta 2 for Windows X64
is not working.
The link does download something, but it's Beta 1.

http://www.enterprisedb.com/products-services-training/pgdownload#windows

Thanks,
Jean-Pierre Pelletier

Re: [HACKERS] Actuall row count of Parallel Seq Scan in EXPLAIN ANALYZE .

[David G. Johnston]

On Mon, Jun 20, 2016 at 2:54 AM, Masahiko Sawada 
wrote:

> >> QUERY PLAN
> >>
> >>
> -
> >>  Finalize Aggregate  (cost=217018.55..217018.56 rows=1 width=8)
> >> (actual time=2640.015..2640.015 rows=1 loops=1)
> >>Output: count(*)
> >>->  Gather  (cost=217018.33..217018.54 rows=2 width=8) (actual
> >> time=2639.064..2640.002 rows=3 loops=1)
> >>  Output: (PARTIAL count(*))
> >>  Workers Planned: 2
> >>  Workers Launched: 2
> >>  ->  Partial Aggregate  (cost=216018.33..216018.34 rows=1
> >> width=8) (actual time=2632.714..2632.715 rows=1 loops=3)
> >>Output: PARTIAL count(*)
> >>Worker 0: actual time=2632.583..2632.584 rows=1 loops=1
> >>Worker 1: actual time=2627.517..2627.517 rows=1 loops=1
> >>->  Parallel Seq Scan on public.pgbench_accounts
> >> (cost=0.00..205601.67 rows=417 width=0) (actual
> >> time=0.042..1685.542 rows=333 loops=3)
> >>  Worker 0: actual time=0.033..1657.486 rows=3457968
> >> loops=1
> >>  Worker 1: actual time=0.039..1702.979 rows=3741069
> >> loops=1
>


> postgres(1)=# explain analyze verbose select * from pgbench_accounts
> where aid = 10;
>  QUERY PLAN
>
> 
>  Gather  (cost=1000.00..217018.43 rows=1 width=97) (actual
> time=0.541..2094.773 rows=1 loops=1)
>Output: aid, bid, abalance, filler
>Workers Planned: 2
>Workers Launched: 2
>->  Parallel Seq Scan on public.pgbench_accounts
> (cost=0.00..216018.34 rows=0 width=97) (actual time=1390.109..2088.103
> rows=0 loops=3)
>  Output: aid, bid, abalance, filler
>  Filter: (pgbench_accounts.aid = 10)
>  Rows Removed by Filter: 333
>  Worker 0: actual time=2082.681..2082.681 rows=0 loops=1
>  Worker 1: actual time=2087.532..2087.532 rows=0 loops=1
>  Planning time: 0.126 ms
>  Execution time: 2095.564 ms
> (12 rows)
>
>
The below discrepancy bothered me but it finally dawned on me that this is
exactly what Robert's complaint is about.  The 1 row returned by the leader
is averaged across three parallel workers and shows as zero in the "actual"
count for the PSS in the second plan.

​Actual rows in parens:

First Plan:

Finalize Agg. (1) <- Gather (3) <- ​Partial Agg (1x3 = 3) { "Leader" (1) +
Worker0 (1) + Worker1 (1) } <- ParallelSS (3.33Mx3) { "Leader" (~2M) +
 Worker0 (~4M) + Worker1 (~4M) }

Second Plan:

Gather (1) <- ParallelSS (0.33, rounded down :( x 3) { "Leader" (0) +
Worker0 (0) + Worker1 (0) }

(rhetorical question, see my first paragraph)
Why, in the first plan, does the PSS node have a total count of 10M which
are fed to and aggregated by the Partial Agg node parent, while in the
second plan the PSS node shows zero actual rows yet its parent has a row
count of 1?

I'm inclined to go with Robert on this - reporting averages seems
counter-productive.  Similar information can be had by reporting totals and
loop and letting the do the division if needed.

As a middle position if the resultant integer average rows value is 0 can
we output 

Re: [HACKERS] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Tue, Jun 21, 2016 at 4:18 PM, Merlin Moncure  wrote:
> >> explain analyze select * from foo where false or exists (select 1 from
> >> bar where good and foo.id = bar.id);  -- A
> >> explain analyze select * from foo where exists (select 1 from bar
> >> where good and foo.id = bar.id);  -- B
> >> 
> >> These queries are trivially verified as identical but give very different 
> >> plans.
> 
> > Right.  I suspect wouldn't be very hard to notice the special case of
> > FALSE OR (SOMETHING THAT MIGHT NOT BE FALSE) but I'm not sure that's
> > worth optimizing by itself.
> 
> Constant-folding will get rid of the OR FALSE (as well as actually-useful
> variants of this example).  The problem is that that doesn't happen till
> after we identify semijoins.  So the second one gives you a semijoin plan
> and the first doesn't.  This isn't especially easy to improve.  Much of
> the value of doing constant-folding would disappear if we ran it before
> subquery pullup + join simplification, because in non-stupidly-written
> queries those are what expose the expression simplification opportunities.
> We could run it twice but that seems certain to be a dead loser most of
> the time.

While it might be a loser most of the time to run it twice, I have to
agree that it's pretty unfortunate that we don't handle this case in a
more sane way.  I looked a bit into pull_up_sublinks() and it doens't
look like there's an easy way to realize this case there without going
through the full effort of constant-folding.

One approach that I'm wondering about is to do constant folding first
and then track if we introduce a case where additional constant folding
might help and only perform it again in those cases.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila  wrote:
>>> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
>>> when scanning each block, and then to issue a WARNING and clear the
>>> visibility map. Indeed that's better. I guess I need to take a closer
>>> look at vacuumlazy.c. See attached for example, but that's perhaps not
>>> something to have in 9.6 as that's more a micro-optimization than
>>> anything else.
>>
>> Right, something like that.  I think Andres actually wants something
>> like this in 9.6, and I'm inclined to think it might be a good idea,
>> too.  I think there should probably be a test for
>> all_visible_according_to_vm at the beginning of that so that we don't
>> add more visibility map checks for pages where we already know the VM
>> bit can't possibly be set.
>>
>> Other opinions on the concept or the patch?
>
> +1 on the idea.
>
> + PageClearAllVisible(page);
> + MarkBufferDirty(buf);
>
> What is the need to clear the Page level bit, if it is already
> cleared, doesn't '!all_frozen' indicate that?

No, I don't think so.  I think all_frozen indicates whether we think
that all tuples on the page qualify as fully frozen.  I don't think it
tells us anything about whether PD_ALL_VISIBLE is set on the page.

-- 
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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Merlin Moncure]

On Fri, Jul 1, 2016 at 10:27 AM, Robert Haas  wrote:
> On Fri, Jul 1, 2016 at 10:20 AM, Merlin Moncure  wrote:
>> Yeah.  Also, even if you could parse out those cases, it's major
>> optimization fence.  Consider if you have an ORDER BY clause here:
>>
>> SELECT FROM foo WHERE a OR b ORDER BY c;
>>
>> ... by pushing inside a union, you're going to be in trouble in real
>> world cases.  That's just a mess and it would add a lot of runtime
>> analysis of the alternative paths.  It's hard for me to believe
>> rewriting is easier and simpler than rewriting 'false OR x' to 'x'.  I
>> also thing that constant folding strategies are going to render much
>> more sensible output to EXPLAIN.
>
> I don't think that it's easier and simpler and didn't intend to say
> otherwise.  I do think that I've run across LOTS of queries over the
> years where rewriting OR using UNION ALL was a lot faster, and I think
> that case is more likely to occur in practice than FALSE OR WHATEVER.
> But, I'm just throwing out opinions to see what sticks here; I'm not
> deeply invested in this.

Sure (I didn't put you on that position, just thinking out loud).  The
problem with UNION ALL is that it's only safe to do so when you know
for sure the both sides of the partition are non-overlapping.  The
author of the query often knows this going in but for the planner it's
not so simple to figure out in many cases.  If there's a subset of
cases.   UNION sans ALL is probably a dead end on performance grounds.

This hinges on Tom's earlier statements, "Much of
the value of doing constant-folding would disappear if we ran it before
subquery pullup + join simplification, because in non-stupidly-written
queries those are what expose the expression simplification opportunities."

and, especially, "We could run it twice but that seems certain to be a
dead loser most of
the time."

It's pretty easy to craft a query where you're on the winning side,
but what's the worst case of doing two pass...is constant folding a
non trivial fraction of planning time?

merlin


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

Re: [HACKERS] fixing consider_parallel for upper planner rels

[Robert Haas]

On Fri, Jul 1, 2016 at 10:52 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane  wrote:
>>> And the point of that is what, exactly?  If the only change is that
>>> "some restrictions get enforced", I am not clear on why we need such
>>> a test mode in cases where the planner is afraid to put a top Gather on
>>> the plan.  In particular, given the coding as you now have it, it seems
>>> like the only case where there's any difference is where we set
>>> glob->parallelModeOK but nonetheless end up with a not-parallel-safe
>>> topmost path (that doesn't have a Gather within it).  It's not clear
>>> to me why having the executor switch into parallel mode makes sense at
>>> all with such a plan.
>
>> Suppose you create a PL/pgsql function that does an UPDATE and mark it
>> PARALLEL RESTRICTED.  You wonder whether you've marked it correctly.
>> You can set force_parallel_mode=on and SELECT myfunc().  The
>> subsequent ERROR tells you that you've mismarked it.
>
> Right, but that statement is still true with the logic I'm imagining.
> I would also argue that the existing text in config.sgml explaining
> what this parameter does corresponds much more nearly to what I'm
> suggesting than to what you say the semantics are.

I just went and reread that description and it looks to me like it
matches what I said.  I guess I don't really understand what exactly
you want to change.

-- 
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] fixing consider_parallel for upper planner rels

[Robert Haas]

On Fri, Jul 1, 2016 at 11:09 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane  wrote:
>>> Don't have time to re-read this right now, but maybe tomorrow or
>>> Saturday.
>
>> OK, thanks.
>
> There's still the extra-word problem here:
>
> +* If the input rel is marked consider_parallel and there's nothing
> +* that's not parallel-safe in the LIMIT clause, then the final_rel is
> +* can be marked consider_parallel as well.
>
> Other than that, and the quibble over initialization of
> parallelModeNeeded, I'm good with this.

OK, committed.  I think we can argue about parallelModeNeeded as a
separate matter.  That's merely a sideshow as far as this patch is
concerned.

-- 
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] ToDo: API for SQL statement execution other than SPI

[Robert Haas]

On Wed, Jun 29, 2016 at 1:55 AM, Pavel Stehule  wrote:
> I am writing two background workers - autoreindex and scheduler. In Both I
> need to execute queries from top level. I had to wrote redundant code
> https://github.com/okbob/autoreindex/blob/master/utils.c
> autoreindex_execute_sql_command .Same code is in pglogical. Some statements
> - like VACUUM or REINDEX CONCURRENTLY is not possible call from SPI.
>
> Can be nice to have this function in core.

pg_background handles this.  Somebody might want to resurrect that patch.

-- 
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 29, 2016 at 1:38 AM, Ashutosh Bapat
 wrote:
> On Tue, Jun 28, 2016 at 12:52 PM, Etsuro Fujita
>  wrote:
>> On 2016/06/28 15:23, Ashutosh Bapat wrote:
>>>
>>> The wording "column "whole-row reference ..." doesn't look good.
>>> Whole-row reference is not a column. The error context itself should be
>>> "whole row reference for foreign table ft1".
>>
>> Ah, you are right.  Please find attached an updated version.
>
> This looks good to me. Regression tests pass.

Committed.  Thanks to Etsuro Fujita for the patch and to Ashutosh for
the review.

-- 
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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Robert Haas]

On Fri, Jul 1, 2016 at 10:11 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Jul 1, 2016 at 9:52 AM, Tom Lane  wrote:
>>> Maybe, but neither UNION nor UNION ALL would duplicate the semantics
>>> of OR, so there's some handwaving here that I missed.
>
>> SELECT * FROM foo WHERE a = 5 OR a = 4
>> isn't equivalent to
>> SELECT * FROM foo WHERE a = 5
>> UNION
>> SELECT * FROM foo WHERE a = 4
>> ?
>
> It probably is, but you're assuming that "a" appears in the list of
> columns being unioned.  If you make that just "SELECT b FROM ..."
> then the latter form gets rid of duplicate b values where the first
> doesn't.  On the other hand, UNION ALL might introduce duplicates
> not present in the OR query's result.

Right, so, significant query transformations are non-trivial.  But the
point is that with the upper planification stuff, I think it is
possible, at least in some cases, that we could consider reordering
set operations with scan/join planning, just as we've previously
talked about reordering grouping stages relative to scan/join
planning.

The details are undeniably hard to get right.

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


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

Re: [HACKERS] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Robert Haas]

On Fri, Jul 1, 2016 at 10:20 AM, Merlin Moncure  wrote:
> Yeah.  Also, even if you could parse out those cases, it's major
> optimization fence.  Consider if you have an ORDER BY clause here:
>
> SELECT FROM foo WHERE a OR b ORDER BY c;
>
> ... by pushing inside a union, you're going to be in trouble in real
> world cases.  That's just a mess and it would add a lot of runtime
> analysis of the alternative paths.  It's hard for me to believe
> rewriting is easier and simpler than rewriting 'false OR x' to 'x'.  I
> also thing that constant folding strategies are going to render much
> more sensible output to EXPLAIN.

I don't think that it's easier and simpler and didn't intend to say
otherwise.  I do think that I've run across LOTS of queries over the
years where rewriting OR using UNION ALL was a lot faster, and I think
that case is more likely to occur in practice than FALSE OR WHATEVER.
But, I'm just throwing out opinions to see what sticks here; I'm not
deeply invested in this.

-- 
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] fixing consider_parallel for upper planner rels

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane  wrote:
>> Don't have time to re-read this right now, but maybe tomorrow or
>> Saturday.

> OK, thanks.

There's still the extra-word problem here:

+* If the input rel is marked consider_parallel and there's nothing
+* that's not parallel-safe in the LIMIT clause, then the final_rel is
+* can be marked consider_parallel as well.

Other than that, and the quibble over initialization of
parallelModeNeeded, I'm good with this.

regards, tom lane


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

Re: [HACKERS] fixing consider_parallel for upper planner rels

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane  wrote:
>> And the point of that is what, exactly?  If the only change is that
>> "some restrictions get enforced", I am not clear on why we need such
>> a test mode in cases where the planner is afraid to put a top Gather on
>> the plan.  In particular, given the coding as you now have it, it seems
>> like the only case where there's any difference is where we set
>> glob->parallelModeOK but nonetheless end up with a not-parallel-safe
>> topmost path (that doesn't have a Gather within it).  It's not clear
>> to me why having the executor switch into parallel mode makes sense at
>> all with such a plan.

> Suppose you create a PL/pgsql function that does an UPDATE and mark it
> PARALLEL RESTRICTED.  You wonder whether you've marked it correctly.
> You can set force_parallel_mode=on and SELECT myfunc().  The
> subsequent ERROR tells you that you've mismarked it.

Right, but that statement is still true with the logic I'm imagining.
I would also argue that the existing text in config.sgml explaining
what this parameter does corresponds much more nearly to what I'm
suggesting than to what you say the semantics are.

>> What I'm not happy about is that as you've got things constituted,
>> the GetForeignUpperPaths hook is broken so far as injecting parallel paths
>> is concerned, because the consider_parallel flags for the upperrels
>> aren't set yet when it gets called.  If we keep consider_parallel with
>> its present usage, we're going to have to refactor things to fix that.

> I see.  It seems to me, and I may be failing to understand something,
> that the placement of create_upper_paths_hook is substantially better
> than the placement of GetForeignUpperPaths.  If the latter were moved
> to where the former now is, then consider_parallel would be set
> sufficiently early and everything would be fine.

Yeah, I came to more or less the same conclusion last night.  Will see
to it after you commit this.

regards, tom lane


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

Re: [HACKERS] Reviewing freeze map code

[Masahiko Sawada]

On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila  wrote:
> On Thu, Jun 30, 2016 at 8:10 PM, Masahiko Sawada  
> wrote:
>> On Thu, Jun 30, 2016 at 3:12 PM, Amit Kapila  wrote:
>>> On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund  wrote:
 On 2016-06-30 08:59:16 +0530, Amit Kapila wrote:
> On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund  
> wrote:
> > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote:
> >> There is nothing in this record which recorded the information about
> >> visibility clear flag.
> >
> > I think we can actually defer the clearing to the lock release?
>
> How about the case if after we release the lock on page, the heap page
> gets flushed, but not vm and then server crashes?

 In the released branches there's no need to clear all visible at that
 point. Note how heap_lock_tuple doesn't clear it at all. So we should be
 fine there, and that's the part where reusing an existing record is
 important (for compatibility).

>>>
>>> For back branches, I agree that heap_lock_tuple is sufficient,
>>
>> Even if we use heap_lock_tuple, If server crashed after flushed heap
>> but not vm, after crash recovery the heap is still marked all-visible
>> on vm.
>
> So, in this case both vm and page will be marked as all_visible.
>
>> This case could be happen even on released branched, and could make
>> IndexOnlyScan returns wrong result?
>>
>
> Why do you think IndexOnlyScan will return wrong result?  If the
> server crash in the way as you described, the transaction that has
> made modifications will anyway be considered aborted, so the result of
> IndexOnlyScan should not be wrong.
>

Ah, you're right, I misunderstood.

Attached updated patch incorporating your comments.
I've changed it so that heap_xlog_lock clears vm flags if page is
marked all frozen.

Regards,

--
Masahiko Sawada


emit_wal_already_marked_true_case_v2.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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Merlin Moncure]

On Fri, Jul 1, 2016 at 9:11 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Jul 1, 2016 at 9:52 AM, Tom Lane  wrote:
>>> Maybe, but neither UNION nor UNION ALL would duplicate the semantics
>>> of OR, so there's some handwaving here that I missed.
>
>> SELECT * FROM foo WHERE a = 5 OR a = 4
>> isn't equivalent to
>> SELECT * FROM foo WHERE a = 5
>> UNION
>> SELECT * FROM foo WHERE a = 4
>> ?
>
> It probably is, but you're assuming that "a" appears in the list of
> columns being unioned.  If you make that just "SELECT b FROM ..."
> then the latter form gets rid of duplicate b values where the first
> doesn't.  On the other hand, UNION ALL might introduce duplicates
> not present in the OR query's result.

Yeah.  Also, even if you could parse out those cases, it's major
optimization fence.  Consider if you have an ORDER BY clause here:

SELECT FROM foo WHERE a OR b ORDER BY c;

... by pushing inside a union, you're going to be in trouble in real
world cases.  That's just a mess and it would add a lot of runtime
analysis of the alternative paths.  It's hard for me to believe
rewriting is easier and simpler than rewriting 'false OR x' to 'x'.  I
also thing that constant folding strategies are going to render much
more sensible output to EXPLAIN.

FYI, The query is something along the lines of
SELECT * FROM foo
WHERE
  ('a' = 'a' AND EXISTS ...)
  OR ('a' = 'b' AND EXISTS ...)
  OR ('a' = 'c' AND EXISTS ...)

...where the left side of the equality is a parameterized 'filter
mode' flag.  That way the query can introduce filtering behaviors
without doing dynamic acrobatics.

merlin


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

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

[Ashutosh Bapat]

On Fri, Jul 1, 2016 at 7:45 PM, Robert Haas  wrote:

> On Tue, Jun 28, 2016 at 8:20 AM, Ashutosh Bapat
>  wrote:
> >> > postgres_fdw resets the search path to pg_catalog while opening
> >> > connection
> >> > to the server. The reason behind this is explained in deparse.c
> >> >
> >> >  * We assume that the remote session's search_path is exactly
> >> > "pg_catalog",
> >> >  * and thus we need schema-qualify all and only names outside
> >> > pg_catalog.
> >>
> >> Hmm.  OK, should we revert the schema-qualification part of that
> >> commit, or just leave it alone?
> >
> > If we leave that code as is, someone who wants to add similar code later
> > would get confused or will be tempted to create more instances of
> > schema-qualification. I think we should revert the schema qualification.
>
> OK, done.
>

Thanks.

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

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

[Robert Haas]

On Tue, Jun 28, 2016 at 8:20 AM, Ashutosh Bapat
 wrote:
>> > postgres_fdw resets the search path to pg_catalog while opening
>> > connection
>> > to the server. The reason behind this is explained in deparse.c
>> >
>> >  * We assume that the remote session's search_path is exactly
>> > "pg_catalog",
>> >  * and thus we need schema-qualify all and only names outside
>> > pg_catalog.
>>
>> Hmm.  OK, should we revert the schema-qualification part of that
>> commit, or just leave it alone?
>
> If we leave that code as is, someone who wants to add similar code later
> would get confused or will be tempted to create more instances of
> schema-qualification. I think we should revert the schema qualification.

OK, done.

-- 
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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Tom Lane]

Robert Haas  writes:
> On Fri, Jul 1, 2016 at 9:52 AM, Tom Lane  wrote:
>> Maybe, but neither UNION nor UNION ALL would duplicate the semantics
>> of OR, so there's some handwaving here that I missed.

> SELECT * FROM foo WHERE a = 5 OR a = 4
> isn't equivalent to
> SELECT * FROM foo WHERE a = 5
> UNION
> SELECT * FROM foo WHERE a = 4
> ?

It probably is, but you're assuming that "a" appears in the list of
columns being unioned.  If you make that just "SELECT b FROM ..."
then the latter form gets rid of duplicate b values where the first
doesn't.  On the other hand, UNION ALL might introduce duplicates
not present in the OR query's result.

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] Documentation fixes for pg_visibility

[Amit Kapila]

On Fri, Jul 1, 2016 at 6:58 PM, Robert Haas  wrote:
> On Wed, Jun 29, 2016 at 1:42 AM, Michael Paquier
>  wrote:
>> On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas  wrote:
>>> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier
>>>  wrote:
> Under what circumstances would you wish to check only one page of a 
> relation?

 What I'd like to be able to do is to stop scanning the relation once
 one defective tuple has been found: if there is at least one problem,
 the whole vm needs to be rebuilt anyway. So this function could be
 wrapped in a plpgsql function for example. It is more flexible than
 directly modifying this function so as it stops at the first problem
 stopped.
>>>
>>> I think most likely the best way to handle this is teach VACUUM to do
>>> PageClearAllVisible() and visibilitymap_clear() on any page where
>>> VM_ALL_FROZEN(onerel, blkno, ) && !all_frozen.  This would go
>>> well with the existing code to clear incorrectly-set visibility map
>>> bits, and it would allow VACUUM (DISABLE_PAGE_SKIPPING) to serve the
>>> purpose you're talking about here, but more efficiently.
>>
>> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
>> when scanning each block, and then to issue a WARNING and clear the
>> visibility map. Indeed that's better. I guess I need to take a closer
>> look at vacuumlazy.c. See attached for example, but that's perhaps not
>> something to have in 9.6 as that's more a micro-optimization than
>> anything else.
>
> Right, something like that.  I think Andres actually wants something
> like this in 9.6, and I'm inclined to think it might be a good idea,
> too.  I think there should probably be a test for
> all_visible_according_to_vm at the beginning of that so that we don't
> add more visibility map checks for pages where we already know the VM
> bit can't possibly be set.
>
> Other opinions on the concept or the patch?
>

+1 on the idea.

+ PageClearAllVisible(page);
+ MarkBufferDirty(buf);

What is the need to clear the Page level bit, if it is already
cleared, doesn't '!all_frozen' indicate that?

-- 
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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Robert Haas]

On Fri, Jul 1, 2016 at 9:52 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Jun 21, 2016 at 4:18 PM, Merlin Moncure  wrote:
>>> explain analyze select * from foo where false or exists (select 1 from
>>> bar where good and foo.id = bar.id);  -- A
>>> explain analyze select * from foo where exists (select 1 from bar
>>> where good and foo.id = bar.id);  -- B
>>>
>>> These queries are trivially verified as identical but give very different 
>>> plans.
>
>> Right.  I suspect wouldn't be very hard to notice the special case of
>> FALSE OR (SOMETHING THAT MIGHT NOT BE FALSE) but I'm not sure that's
>> worth optimizing by itself.
>
> Constant-folding will get rid of the OR FALSE (as well as actually-useful
> variants of this example).  The problem is that that doesn't happen till
> after we identify semijoins.  So the second one gives you a semijoin plan
> and the first doesn't.  This isn't especially easy to improve.  Much of
> the value of doing constant-folding would disappear if we ran it before
> subquery pullup + join simplification, because in non-stupidly-written
> queries those are what expose the expression simplification opportunities.
> We could run it twice but that seems certain to be a dead loser most of
> the time.
>
>> A more promising line of attack as it
>> seems to me is to let the planner transform back and forth between
>> this form for the query and the UNION form.
>
> Maybe, but neither UNION nor UNION ALL would duplicate the semantics
> of OR, so there's some handwaving here that I missed.

SELECT * FROM foo WHERE a = 5 OR a = 4

isn't equivalent to

SELECT * FROM foo WHERE a = 5
UNION
SELECT * FROM foo WHERE a = 4

?

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


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

[HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Fri, Jul 1, 2016 at 7:17 AM, Robert Haas  wrote:
> On Fri, Jul 1, 2016 at 2:48 AM, Andres Freund  wrote:
>>> This PostgreSQL 9.6 open item is past due for your status update.  Kindly 
>>> send
>>> a status update within 24 hours, and include a date for your subsequent 
>>> status
>>> update.  Refer to the policy on open item ownership:
>>> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
>>
>> IIRC Kevin is out of the office this week, so this'll have to wait till
>> next week.
>
> No, he's back since Tuesday - it was last week that he was out.  I
> spoke with him yesterday about this and he indicated that he had been
> thinking about it and had several ideas about how to fix it.  I'm not
> sure why he hasn't posted here yet.

I have been looking at several possible fixes, and weighing the
pros and cons of each.  I expect to post a patch later today.

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


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

Re: [HACKERS] how is the WAL receiver process stopped and restarted when the network connection is broken and then restored?

[Robert Haas]

On Thu, Jun 23, 2016 at 10:56 AM, Rui Hai Jiang  wrote:
> Please let me know if my understanding is incorrect.

I think you've got it about right.

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


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

Re: [HACKERS] Extract Jsonb key and values

[Robert Haas]

On Thu, Jun 23, 2016 at 8:52 AM, hari.prasath  wrote:
> Hi all,
> I am having jsonb as C character string received by WAL decoding and
> want to extract all its key and value pairs.
>
>
>Which is the best approach for extracting keys and its values?
>
> i)  Converting the C string to a PostgreSQL jsonb object
> ii) Using open-source json-c library

I'm pretty sure there's some built-in function which can do this, but
this question is off-topic for this mailing list, which is about
feature development for PostgreSQL.  You probably want to ask
questions like this on pgsql-general.

-- 
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] Comment and function argument names are mismatched in bugmgr.c.

[Robert Haas]

On Thu, Jun 23, 2016 at 3:24 AM, Masahiko Sawada  wrote:
> By commit 428b1d6b, function WritebackContextInit is added but the
> comment of this function seems to be incorrect.
> *max_coalesce variable doesn't exist at anywhere.
> Also, I think it should be fixed that the argument name of this
> function does not match function declare in buf_internal.h.
> Patch for that attached.

Andres, this looks like one for you to handle.

-- 
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] Actuall row count of Parallel Seq Scan in EXPLAIN ANALYZE .

[Robert Haas]

On Mon, Jun 20, 2016 at 3:06 AM, Satoshi Nagayasu  wrote:
> IMHO, "actual rows" of "Parallel Seq Scan" should not be divided by the loops,
> because the total rows processed here is 1000, not 333 * 3.
> I think the actual row value shown here "333 " is a bit confusing and 
> tricky
> for users.

I don't think the total number of rows for a node should EVER be
divided by the number of loops.  We've had that behavior for a long
time and I find it to be mind-numbingly stupid.  Whenever I'm reading
an EXPLAIN plan, I end up trying to figure out what's really going on
by multiplying the number of rows shown by the loop count, but often
the row count is very small, like 0 or 1 or 2, so the round-off error
is large and you can't really tell what's happening.  Nobody wants to
know the average number of rows returned per node execution: they
want, as you want here, the total number of rows that node ever
processed.  I doubt we can convince Tom Lane to let us change it, but
feel free to post a patch.

One thing I don't think we can do here is have some weird exception
where parallel query works differently from everything else.  "loops"
just counts the number of times that the node was executed.  It most
often ends up >1 when the plan node is on the inner side of a nested
loop, but parallel query ends up creating that scenario also.  There's
no real way to separate those things out, though.  If a node executes
3 times in one worker, 4 times in another, and once in the leader,
what value are you going to display for loops other than 8?  And if
you accept that's the right answer in that case, then you pretty much
need the answer when it executes once in one worker, once in another
worker, and once in the leader to be 3.  I agree that this is very
confusing - and you're not the first person to complain about it - but
I think that parallel query is merely throwing light on the fact that
the pre-existing behavior of EXPLAIN is poorly chosen, not creating
any fundamentally new issue.

-- 
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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Tom Lane]

Robert Haas  writes:
> On Tue, Jun 21, 2016 at 4:18 PM, Merlin Moncure  wrote:
>> explain analyze select * from foo where false or exists (select 1 from
>> bar where good and foo.id = bar.id);  -- A
>> explain analyze select * from foo where exists (select 1 from bar
>> where good and foo.id = bar.id);  -- B
>> 
>> These queries are trivially verified as identical but give very different 
>> plans.

> Right.  I suspect wouldn't be very hard to notice the special case of
> FALSE OR (SOMETHING THAT MIGHT NOT BE FALSE) but I'm not sure that's
> worth optimizing by itself.

Constant-folding will get rid of the OR FALSE (as well as actually-useful
variants of this example).  The problem is that that doesn't happen till
after we identify semijoins.  So the second one gives you a semijoin plan
and the first doesn't.  This isn't especially easy to improve.  Much of
the value of doing constant-folding would disappear if we ran it before
subquery pullup + join simplification, because in non-stupidly-written
queries those are what expose the expression simplification opportunities.
We could run it twice but that seems certain to be a dead loser most of
the time.

> A more promising line of attack as it
> seems to me is to let the planner transform back and forth between
> this form for the query and the UNION form.

Maybe, but neither UNION nor UNION ALL would duplicate the semantics
of OR, so there's some handwaving here that I missed.

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] Truncating/vacuuming relations on full tablespaces

[Robert Haas]

On Mon, Jun 20, 2016 at 7:28 AM, Asif Naeem  wrote:
> Thank you for useful suggestions. PFA patch, I have tried to cover all the
> points mentioned.

Thanks for the new patch.  I think that you have failed to address
this point from my previous review:

# I see why you changed the calling convention for visibilitymap_pin()
# and RecordPageWithFreeSpace(), but that's awfully invasive.  I wonder
# if there's a better way to do that, like maybe having vacuumlazy.c ask
# the VM and FSM for their length in pages and then not trying to use
# those functions for block numbers that are too large.

The patch has gotten a lot smaller, and that's clearly good, but
introducing extended versions of those functions still seems like a
thing we should try to avoid. In particular, there's no way this hunk
is going to be acceptable:

@@ -286,6 +299,10 @@ visibilitymap_set(Relation rel, BlockNumber
heapBlk, Buffer heapBuf,
 if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
 elog(ERROR, "wrong heap buffer passed to visibilitymap_set");

+/* In case of invalid buffer just return */
+if(vmBuf == InvalidBuffer)
+return;
+
 /* Check that we have the right VM page pinned */
 if (!BufferIsValid(vmBuf) || BufferGetBlockNumber(vmBuf) != mapBlock)
 elog(ERROR, "wrong VM buffer passed to visibilitymap_set");

You're going to have to find a different approach there.

-- 
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] EXISTS clauses not being optimized in the face of 'one time pass' optimizable expressions

[Robert Haas]

On Tue, Jun 21, 2016 at 4:18 PM, Merlin Moncure  wrote:
> Observe the following test case (apologies if this is a well
> understood problem):
>
> create temp table foo as select generate_series(1,100) id;
> create index on foo(id);
>
> create temp table bar as select id, id % 10 = 0 as good from
> generate_series(1,100) id;
> create index on bar(good);
>
> analyze foo;
> analyze bar;
>
> explain analyze select * from foo where false or exists (select 1 from
> bar where good and foo.id = bar.id);  -- A
> explain analyze select * from foo where exists (select 1 from bar
> where good and foo.id = bar.id);  -- B
>
> These queries are trivially verified as identical but give very different 
> plans.

Right.  I suspect wouldn't be very hard to notice the special case of
FALSE OR (SOMETHING THAT MIGHT NOT BE FALSE) but I'm not sure that's
worth optimizing by itself.  A more promising line of attack as it
seems to me is to let the planner transform back and forth between
this form for the query and the UNION form.  Obviously, in this case,
the WHERE false branch could then be pruned altogether, but there are
lots of other cases where both branches survived.  Tom's upper planner
pathification stuff makes it much easier to think about how such an
optimization might work, but I think it's still not particularly
simple to get right.

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


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

Re: [HACKERS] Broken handling of lwlocknames.h

[Michael Paquier]

On Fri, Jul 1, 2016 at 10:29 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Since Tom proposed the approach which Michael's patch takes, I'm
>> hoping he will review and commit this.  If it is left to me to fix it,
>> I may just adopt a minimal fix.
>
> I'll take a look at it.

Note: the patch is not complete yet. I found some bugs in it.
-- 
Michael


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

Re: [HACKERS] Broken handling of lwlocknames.h

[Tom Lane]

Robert Haas  writes:
> Since Tom proposed the approach which Michael's patch takes, I'm
> hoping he will review and commit this.  If it is left to me to fix it,
> I may just adopt a minimal fix.

I'll take a look at it.

regards, tom lane


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

Re: [HACKERS] Documentation fixes for pg_visibility

[Robert Haas]

On Wed, Jun 29, 2016 at 1:42 AM, Michael Paquier
 wrote:
> On Tue, Jun 28, 2016 at 7:05 AM, Robert Haas  wrote:
>> On Mon, Jun 27, 2016 at 5:56 PM, Michael Paquier
>>  wrote:
 Under what circumstances would you wish to check only one page of a 
 relation?
>>>
>>> What I'd like to be able to do is to stop scanning the relation once
>>> one defective tuple has been found: if there is at least one problem,
>>> the whole vm needs to be rebuilt anyway. So this function could be
>>> wrapped in a plpgsql function for example. It is more flexible than
>>> directly modifying this function so as it stops at the first problem
>>> stopped.
>>
>> I think most likely the best way to handle this is teach VACUUM to do
>> PageClearAllVisible() and visibilitymap_clear() on any page where
>> VM_ALL_FROZEN(onerel, blkno, ) && !all_frozen.  This would go
>> well with the existing code to clear incorrectly-set visibility map
>> bits, and it would allow VACUUM (DISABLE_PAGE_SKIPPING) to serve the
>> purpose you're talking about here, but more efficiently.
>
> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
> when scanning each block, and then to issue a WARNING and clear the
> visibility map. Indeed that's better. I guess I need to take a closer
> look at vacuumlazy.c. See attached for example, but that's perhaps not
> something to have in 9.6 as that's more a micro-optimization than
> anything else.

Right, something like that.  I think Andres actually wants something
like this in 9.6, and I'm inclined to think it might be a good idea,
too.  I think there should probably be a test for
all_visible_according_to_vm at the beginning of that so that we don't
add more visibility map checks for pages where we already know the VM
bit can't possibly be set.

Other opinions on the concept or the patch?

-- 
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] Bug in batch tuplesort memory CLUSTER case (9.6 only)

[Robert Haas]

On Fri, Jul 1, 2016 at 12:06 AM, Noah Misch  wrote:
> On Sun, Jun 26, 2016 at 09:14:05PM -0700, Peter Geoghegan wrote:
>> In general, moving tuplesort.c batch memory caller tuples around
>> happens when batch memory needs to be recycled, or freed outright with
>> pfree().
>>
>> I failed to take into account that CLUSTER tuplesorts need an extra
>> step when moving caller tuples to a new location (i.e. when moving
>> HeapTuple caller tuples using memmove()), because their particular
>> variety of caller tuple happens to itself contain a pointer to
>> palloc()'d memory. Attached patch fixes this use-after-free bug.
>
> [Action required within 72 hours.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
> efforts toward speedy resolution.  Thanks.
>
> [1] 
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com

The proposed patch contains no test case and no description of how to
reproduce the problem.  I am not very keen on the idea of trying to
puzzle that out from first principles.  Also, I would appreciate a
clearer explanation of why this only affects CLUSTER tuplesorts.

-- 
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] Broken handling of lwlocknames.h

[Robert Haas]

On Fri, Jul 1, 2016 at 12:09 AM, Noah Misch  wrote:
> [Action required within 72 hours.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
> efforts toward speedy resolution.  Thanks.
>
> [1] 
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com

Since Tom proposed the approach which Michael's patch takes, I'm
hoping he will review and commit this.  If it is left to me to fix it,
I may just adopt a minimal fix.

-- 
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] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Robert Haas]

On Fri, Jul 1, 2016 at 1:41 AM, Andreas Seltenreich  wrote:
> Amit Kapila writes:
>> On Fri, Jul 1, 2016 at 9:38 AM, Thomas Munro  
>> wrote:
>>> Or maybe just like this?
>>>
>>> -   snapshot->subxip = snapshot->xip + 
>>> serialized_snapshot->xcnt;
>>> +   snapshot->subxip = ((TransactionId *) (snapshot + 1)) +
>>> +   serialized_snapshot->xcnt;
>>>
>>
>> This way it looks better to me.  Thanks for the patch.
>
> I no longer see these crashes when testing with the patch applied.

Committed and back-patched to 9.5 where this code was added.  Thanks
to all for the report, patch, review, and testing.

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


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

[HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Robert Haas]

On Fri, Jul 1, 2016 at 2:48 AM, Andres Freund  wrote:
>> This PostgreSQL 9.6 open item is past due for your status update.  Kindly 
>> send
>> a status update within 24 hours, and include a date for your subsequent 
>> status
>> update.  Refer to the policy on open item ownership:
>> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com
>
> IIRC Kevin is out of the office this week, so this'll have to wait till
> next week.

No, he's back since Tuesday - it was last week that he was out.  I
spoke with him yesterday about this and he indicated that he had been
thinking about it and had several ideas about how to fix it.  I'm not
sure why he hasn't posted here yet.

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


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

[HACKERS] Re: [ADMIN] problems using pg_start_backup/pg_stop_backup and pg_basebackup at same time

[Magnus Hagander]

On Mon, Jun 27, 2016 at 8:27 PM, Alex Malek  wrote:

> Should I file a bug?
>

Oh, sorry. I did not spot that this was posted to pgsql-admin, which has a
lot fewer readers. I've moved it over to -hackers where more people will
see it.

(And also, please don't top-post on these lists, as it makes the discussion
much harder to follow)



>
> Does anyone know of a workaround?
>
> I could try running pg_basebackup w/ --xlog-method=fetch and see if that
> helps but I do like knowing that the base backup will have all needed WAL
> files regardless of WAL retention policy on master.
>
> Thanks.
> Alex
>
> Recap of what I *think* is going on:
>
> 1) pg_basebackup w/ --xlog-method=stream is started
> 2) archive_command is not executed while pg_basebackup() is running
>

There is no reason why archive_command should not run during a
pg_basebackup run. Do you get anything at all in the logfile indicating
that this would be the case? If you look at the ps output, does the
archiver process indicate what it's doing, and does it have a sub-process
for your rsync command?

Is there any chance that for example the network or other part of the
system gets saturated and the archive_command is simply runniung too slow
to keep up?


3) while pg_basebackup() is running pg_start_backup() is executed
> 4) Later pg_stop_backup() is executed.
> 5) pg_stop_backup() hangs related to the archive_command not running
> 6) hours later when pg_basebackup finishes, back logged archive commands
> get executed then pg_stop_backup() finishes
>

This would be consistent with (2), but there is no reason why (2) would
happen because of pg_basebackup unless there's an actual bug there.

//Magnus



> On Mon, Jun 20, 2016 at 10:15 AM, Alex Malek  wrote:
>
>>
>> Thanks for the response Magnus.
>> I've replied inline below.
>>
>> On Mon, Jun 20, 2016 at 4:17 AM, Magnus Hagander 
>> wrote:
>>
>>> On Mon, Jun 13, 2016 at 6:59 PM, Alex Malek 
>>> wrote:
>>>

 I am experiencing two problems
 1) pg_stop_backup hangs until pg_basebackup finishes
 2) WAL contains references to invalid pages

 (I suspect when 1. is fixed I won't experience 2. )

 Full description below:


 postgresql: 9.3.13

 My situation is I create a writable report database every day using
 pg_start_backup() / rsync / pg_stop_backup()
 (whole process takes 1-2 hours, the rsync actually copies the data dir
 of a  slave/warm spare DB)

>>>
>>> Taking what is called an exclusive backup, which is what
>>> pg_start_backup() does, is not supported off a standby node. Only
>>> pg_basebackup is supported against a standby node.
>>>
>>> Can you reproduce your errors if you make this backup from the master?
>>>
>>
>>
>> pg_start_backup() is run on the master.
>> The rsync copies the data dir of the standby b/c it is local.
>> I have tried rsyncing from the master w/ the same results.
>>
>>
>>>
>>>
>>>
 Also once a week I create a database for backup/archive purposes using
 pg_basebackup
 (whole process takes about 13 hours)


 These two processes used to be able to coincide until recently.
 Recent changes include a major debian upgrade and a minor version of
 postgres upgrade from 9.3.10.

 Now when the report sync occurs during the pg_basebackup (w/
 --xlog-method=stream option) the pg_stop_backup() hangs until
 the the pg_basebackup has completed (4 hours later).

>>>
>>> Do you also have an archive_command, and what is it? Normally,
>>> pg_stop_backup() blocks on the archive command - so perhaps you have ended
>>> up with a dependency between that one and the base backup command somehow?
>>>
>>>
>> The archive command is:
>>
>> archive_command = 'rsync -a %p postgres@archivedb
>> :/storage/postgres/9.3/main-archive/pg_xlog/%f'
>>
>> I thought the archive command was working properly while pg_stop_backup
>> was hanging based on the timestamps of the files, but upon closer
>> inspection using stat I see that the files actually were not copied over
>> until pg_stop_backup stopped hanging.
>>
>> Actually it appears that the archive_command is not executed the entire
>> time pg_basebackup is running.
>> pg_basebackup is started about 5 hours before pg_start_backup is executed.
>> Based on stat it looks like the wal files get written locally but do not
>> get rsynced until pg_basebackup is done.
>>
>>
>>
>>>
>>>
 A labeled WAL backup file is created when the pg_stop_backup() is first
 executed and another
 is created when the pg_basebackup completes.



 While the pg_stop_backup() hangs the following appears in the logs:

 2016-06-11 07:50:45 EDT: WARNING:  pg_stop_backup still waiting for all
 required WAL segments to be archived (7680 seconds elapsed)
 2016-06-11 07:50:45 EDT: HINT:  Check that your archive_command is
 executing properly.  

Re: [HACKERS] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Craig Ringer]

On 1 July 2016 at 17:52, Oleg Bartunov  wrote:

> On Wed, Jun 29, 2016 at 2:51 AM, Stefan Keller  wrote:
> > Hi,
> >
> > FYI: I'd just like to point you to following two forthcoming standard
> > parts from "ISO/IEC JTS 1/SC 32" comittee: one on JSON, and one on
> > "Multi-Dimensional Arrays" (SQL/MDA).
> >
> > They define there some things different as already in PG. See also
> > Peter Baumann's slides [1] and e.g. [2]
>
> I' very dissapointed with this.
>


Yeah, but since when has the SQL standard adopted any existing
implementation's spelling of a feature? It seems to be politically
impossible.


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

Re: [HACKERS] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Oleg Bartunov]

On Wed, Jun 29, 2016 at 2:51 AM, Stefan Keller  wrote:
> Hi,
>
> FYI: I'd just like to point you to following two forthcoming standard
> parts from "ISO/IEC JTS 1/SC 32" comittee: one on JSON, and one on
> "Multi-Dimensional Arrays" (SQL/MDA).
>
> They define there some things different as already in PG. See also
> Peter Baumann's slides [1] and e.g. [2]

I' very dissapointed with this.


>
> :Stefan
>
> [1] 
> https://www.unibw.de/inf4/professors/geoinformatics/agile-2016-workshop-gis-with-nosql
> [2] 
> http://jtc1sc32.org/doc/N2501-2550/32N2528-WG3-Tutorial-Opening-Plenary.pdf
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
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] OpenSSL 1.1 breaks configure and more

[Christoph Berg]

Re: Andreas Karlsson 2016-07-01 <688a438c-ccc2-0431-7100-26e418fc3...@proxel.se>
> Hi,
> 
> Here is an initial set of patches related to OpenSSL 1.1. Everything should
> still build fine on older OpenSSL versions (and did when I tested with
> 1.0.2h).

Hi Andreas,

thanks for the patches. I applied all there patches on top of HEAD
(10c0558f). The server builds and passes "make check", pgcrypto still
needs work, though:

./configure --with-openssl
make world

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 -fpic -I. -I. 
-I../../src/include -D_GNU_SOURCE   -c -o openssl.o openssl.c
openssl.c:205:13: error: field ‘ctx’ has incomplete type
  EVP_MD_CTX ctx;
 ^
openssl.c: In function ‘digest_free’:
openssl.c:253:2: warning: implicit declaration of function ‘EVP_MD_CTX_cleanup’ 
[-Wimplicit-function-declaration]
  EVP_MD_CTX_cleanup(>ctx);
  ^
openssl.c: In function ‘init_openssl_rand’:
openssl.c:990:24: warning: implicit declaration of function ‘RAND_SSLeay’ 
[-Wimplicit-function-declaration]
   RAND_set_rand_method(RAND_SSLeay());
^
openssl.c:990:24: warning: passing argument 1 of ‘RAND_set_rand_method’ makes 
pointer from integer without a cast [-Wint-conversion]
In file included from openssl.c:40:0:
/usr/include/openssl/rand.h:41:5: note: expected ‘const RAND_METHOD * {aka 
const struct rand_meth_st *}’ but argument is of type ‘int’
 int RAND_set_rand_method(const RAND_METHOD *meth);
 ^
openssl.c: In function ‘px_get_pseudo_random_bytes’:
openssl.c:1017:2: warning: ‘RAND_pseudo_bytes’ is deprecated 
[-Wdeprecated-declarations]
  res = RAND_pseudo_bytes(dst, count);
  ^
In file included from openssl.c:40:0:
/usr/include/openssl/rand.h:51:5: note: declared here
 DEPRECATEDIN_1_1_0(int RAND_pseudo_bytes(unsigned char *buf, int num))
 ^
openssl.c: In function ‘digest_block_size’:
openssl.c:222:1: warning: control reaches end of non-void function 
[-Wreturn-type]
 }
 ^
openssl.c: In function ‘digest_result_size’:
openssl.c:214:1: warning: control reaches end of non-void function 
[-Wreturn-type]
 }
 ^
: die Regel für Ziel „openssl.o“ scheiterte
make[2]: *** [openssl.o] Fehler 1
make[2]: Verzeichnis „/home/cbe/projects/postgresql/pg/master/contrib/pgcrypto“ 
wird verlassen

ii  libssl-dev:amd641.1.0~pre5-4 amd64Secure Sockets 
Layer toolkit - development files
ii  libssl1.0.0:amd64   1.0.2d-1 amd64Secure Sockets 
Layer toolkit - shared libraries
ii  libssl1.0.2:amd64   1.0.2h-1 amd64Secure Sockets 
Layer toolkit - shared libraries
ii  libssl1.1:amd64 1.1.0~pre5-4 amd64Secure Sockets 
Layer toolkit - shared libraries
ii  openssl 1.0.2h-1 amd64Secure Sockets 
Layer toolkit - cryptographic utility

Christoph


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

Re: [HACKERS] WIP: About CMake v2

[Dmitry Maslyuk]

Hi!

I think, we need simple configure script generator for backward 
compatibility and easy using this build system.
Try playing with cmake build system under Win2008+MinGW. I plan to write 
perl script for automatic build this

with depends.

On 29.06.2016 19:23, Yury Zhuravlev wrote:

Hello Hackers.

I decided to talk about the current state of the project:
1. Merge with 9.6 master. 2. plpython2, plpython3, plperl, pltcl, 
plsql all work correctly (all tests pass).
3. Works done for all contrib modules. 4. You can use gettext, 
.po->.mo will have converted by CMake.  5. All test pass under some 
Linux, FreeBSD, Solaris10 (on Sparc), Windows MSVC 2015. MacOSX I 
think not big trouble too.  6. Prototype for PGXS (with MSVC support) 
done.

I think is very big progress but I came across one problem.
I not have access to many OS and platforms. For each platform need 
tests and small fixes. I can't develop and give guarantee without it.


I think this is very important work which makes it easier further 
support Postgres but I can not do everything himself. It's physically 
impossible.


I think without community support I can't do significantly more.

Current version you can get here: 
https://github.com/stalkerg/postgres_cmake


Thanks!




--
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] OpenSSL 1.1 breaks configure and more

[Magnus Hagander]

On Fri, Jul 1, 2016 at 10:10 AM, Michael Paquier 
wrote:

> On Fri, Jul 1, 2016 at 5:02 PM, Magnus Hagander 
> wrote:
> > Debian testing is still on 1.0.2h.
> > Debian experimental is on 1.1.0pre5.
> >
> > Not sure here beta2 enters the discussion, it's not mentioned anywhere on
> > their site?
>
> Thanks. From the main page of openssl.org, pre5 is beta2.
>
>
Hah. And it's not mentioned on their download page. I see they continue
down their path of confusing version numbering.



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

Re: [HACKERS] WIP: About CMake v2

[Yury Zhuravlev]

Michael Paquier wrote:

Personally I am allergic to any kind of UIs for
development, and I am sure not to be the only one on this list.


Andrew Dunstan:

We need this to be scriptable, not using a GUI.


GUI is strong optional feature. Helpful for some tasks. 


--
Yury Zhuravlev
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] OpenSSL 1.1 breaks configure and more

[Michael Paquier]

On Fri, Jul 1, 2016 at 5:02 PM, Magnus Hagander  wrote:
> Debian testing is still on 1.0.2h.
> Debian experimental is on 1.1.0pre5.
>
> Not sure here beta2 enters the discussion, it's not mentioned anywhere on
> their site?

Thanks. From the main page of openssl.org, pre5 is beta2.
-- 
Michael


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

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Magnus Hagander]

On Fri, Jul 1, 2016 at 4:08 AM, Michael Paquier 
wrote:

> On Fri, Jul 1, 2016 at 9:27 AM, Andreas Karlsson 
> wrote:
> > Hi,
> >
> > Here is an initial set of patches related to OpenSSL 1.1. Everything
> should
> > still build fine on older OpenSSL versions (and did when I tested with
> > 1.0.2h).
> >
> > 0001-Fixes-for-compiling-with-OpenSSL-1.1.patch
> >
> > This patch fixes the code so it builds with OpenSSL 1.1 (except the
> > CRYPTO_LOCK issue I have reported to the OpenSSL team).
> >
> > - Makes our configure script check for SSL_new instead
> > - Uses functions instead of direct access to struct members
> >
> > 0002-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat.patch
> >
> > Fix for the removal of the CRYPTO_LOCK define. I am trying to convince
> them
> > to add the define back. :)
> >
> > 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
> >
> > Silence all warnings. This commit changes more things and is not
> necessary
> > for getting PostgreSQL to build against 1.1.
> >
> > - Silences deprecation other warnings related to that OpenSSL 1.1 now 1)
> > automatically initializes the library and 2) no longer uses the locking
> > callback.
> > - Silences deprecation warning when generating DH parameters.
>
> Those patches are going to need a careful review by looking at the
> areas they are changing, and a backpatch. On Arch there is no test
> package available except in AUR. And that's the pre3 release, OpenSSL
> folks are on pre5 now with their beta2. It would be annoying to
> compile it manually, but if there is no other way... Is Debian up to
> date with 1.1.0 beta2 in its snapshot packages?
>

Debian testing is still on 1.0.2h.
Debian experimental is on 1.1.0pre5.

Not sure here beta2 enters the discussion, it's not mentioned anywhere on
their site?

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

Re: [HACKERS] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
There's no formal extension API. So there's no boundary between "internal stuff 
we might have to change to fix a problem" and "things extensions can rely on 
not changing under them". In theory anything that changed behaviour or changed 
a header file in almost any way could break an extension.

There's no deliberate breakage and some awareness of possible consequences to 
extensions, but no formal process. I would prefer that the manual explicitly 
recommend recompiling extensions against each minor update (or updating them 
along with the packages), and advise that packagers make their extensions 
depend on an = minor version in their package specifications, not a >= .


Yes, I think such recommendation in the manual is the best.


However, in practice it's fine almost all the time.

Maybe most extensions don’t use sensitive parts of the server…


I think making this more formal would require, as Tom noted, a formal extension 
API we can promise to maintain, likely incorporating:
- ... endlessly more

Endless (^^;)

The main thing is that it's a great deal of work for limited benefit. I don't 
know about you, but I'm not keen.

I’m not keen, either… I don’t think I can form the API that advanced extension 
developers will be satisfied with.  I’ll just document the compabibility 
article in the upgrade section.

Regards
Takayuki Tsunakawa

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

[Etsuro Fujita]

On 2016/04/14 4:57, Robert Haas wrote:

1. For a regular FDW scan, zero the xmin, xmax, and cid of the tuple
before returning it from postgres_fdw, so that we don't expose the
datum-tuple fields.   I can't see any reason this isn't safe, but I
might be missing something.


I noticed odd behavior of this in EvalPlanQual.  Consider:

-- session 1
postgres=# create extension postgres_fdw;
CREATE EXTENSION
postgres=# create server fs foreign data wrapper postgres_fdw options 
(dbname 'postgres');

CREATE SERVER
postgres=# create user mapping for public server fs;
CREATE USER MAPPING
postgres=# create table t1 (a int, b int);
CREATE TABLE
postgres=# create table t2 (a int, b int);
CREATE TABLE
postgres=# insert into t1 values (1, 1);
INSERT 0 1
postgres=# insert into t2 values (1, 1);
INSERT 0 1
postgres=# create foreign table ft1 (a int, b int) server fs options 
(table_name 't1');

CREATE FOREIGN TABLE
postgres=# select xmin, xmax, cmin, * from ft1;
  xmin | xmax | cmin | a | b
--+--+--+---+---
 0 |0 |0 | 1 | 1
(1 row)

-- session 2
postgres=# begin;
BEGIN
postgres=# update t2 set a = a;
UPDATE 1

-- session 1
postgres=# select ft1.xmin, ft1.xmax, ft1.cmin, ft1.* from ft1, t2 for 
update;


-- session 2
postgres=# commit;
COMMIT

-- session 1 (will show the following)
  xmin |xmax| cmin  | a | b
--++---+---+---
   128 | 4294967295 | 16398 | 1 | 1
(1 row)

The values of xmin, xmax, and cmin are not 0!  The reason for that is 
that we don't zero these values in a test tuple stored by 
EvalPlanQualFetchRowMarks for EvalPlanQual re-evaluations.


That cleanup applies to the file_fdw foreign table case as well, so I 
think xmin, xmax, and cid in tuples from such tables should be set to 0, 
too.  And ISTM that it's better that the core (ie, ForeignNext) supports 
doing that, rather than each FDW does that work.  That would also 
minimize the overhead because ForeignNext does that if needed.  Please 
find attached a patch.


Best regards,
Etsuro Fujita


postgres-fdw-syscol-cleanup.patch
Description: binary/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

[HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Andres Freund]

On 2016-06-30 23:51:18 -0400, Noah Misch wrote:
> On Thu, Jun 16, 2016 at 01:56:44PM -0500, Kevin Grittner wrote:
> > On Thu, Jun 16, 2016 at 1:32 PM, Andres Freund  wrote:
> > 
> > > With old_snapshot_threshold=1 I indeed can reproduce the issue. I
> > > disabled autovacuum, to make the scheduling more predictable. But it
> > > should "work" just as well with autovacuum.
> > >
> > > S1: CREATE TABLE toasted(largecol text);
> > > INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM 
> > > generate_series(1, 1000);
> > > BEGIN;
> > > DELETE FROM toasted;
> > > S2: BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> > > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > >> ...
> > > S1: COMMIT;
> > > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > >> ...
> > > S1: /* wait for snapshot threshold to be passed */
> > > S1: VACUUM pg_toast.pg_toast_16437;
> > >> INFO:  0: "pg_toast_16437": found 61942 removable, 0 nonremovable 
> > >> row versions in 15486 out of 15486 pages
> > >> DETAIL:  0 dead row versions cannot be removed yet.
> > > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > > ERROR:  XX000: missing chunk number 0 for toast value 16455 in 
> > > pg_toast_16437
> > > LOCATION:  toast_fetch_datum, tuptoaster.c:1945
> > 
> > Thanks!  That's something I should be able to work with.
> > Unfortunately, I am going to be on vacation, so I won't have any
> > results until sometime after 28 June.
> 
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com

IIRC Kevin is out of the office this week, so this'll have to wait till
next week.

Andres


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

Re: [HACKERS] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Craig Ringer]

On 1 July 2016 at 08:33, Tsunakawa, Takayuki  wrote:

> Hello,
>
> While I was thinking of application binary compatibility between
> PostgreSQL releases, some questions arose about C language user-defined
> functions (UDFs) and extensions that depend on them.
>
> [Q1]
> Can the same UDF binary be used with different PostgreSQL minor releases?
> If it is, is it a defined policy (e.g. written somewhere in the manual,
> wiki, documentation in the source code)?
>
> For example, suppose you build a UDF X (some_extension.so/dll) with
> PostgreSQL 9.5.0.  Can I use the binary with PostgreSQL 9.5.x without
> rebuilding?
>

Probably - but we don't guarantee it.

There's no formal extension API. So there's no boundary between "internal
stuff we might have to change to fix a problem" and "things extensions can
rely on not changing under them". In theory anything that changed behaviour
or changed a header file in almost any way could break an extension.

There's no deliberate breakage and some awareness of possible consequences
to extensions, but no formal process. I would prefer that the manual
explicitly recommend recompiling extensions against each minor update (or
updating them along with the packages), and advise that packagers make
their extensions depend on an = minor version in their package
specifications, not a >= .

However, in practice it's fine almost all the time.

I think making this more formal would require, as Tom noted, a formal
extension API we can promise to maintain, likely incorporating:

- fmgr
- datatype functions and macros
- elog and other core infrastructure
- major shmem structures
- GUC variables
- plan nodes and command structs
- SPI
- replication origins
- bgworkers
- catalog definitions
- ... endlessly more

To actually ensure extensions conform to the API we'd probably have to
build with -fvisibility=hidden (gcc) and on Windows change our .def
generation, so we don't expose anything that's not part of the formal API.
That's a very strict boundary though; there's no practical way an extension
can say "I know what I'm doing, gimme the internals anyway" and reach
through it. I'd prefer a soft boundary that spat warnings when you touch
stuff you're not allowed to, but I don't know of any good way to do that
that works across multiple compilers and toolchains.

We'd almost certainly have to allow ourselves to _expand_ the API in minor
releases since otherwise the early introduction of the formal API would be
a nightmare. That's fine on pretty much every platform though.

The main thing is that it's a great deal of work for limited benefit. I
don't know about you, but I'm not keen.


> Can the same UDF binary be used with different PostgreSQL distributions
> (EnterpriseDB, OpenSCG, RHEL packages, etc.)?  Or should the UDF be built
> with the target distribution?
>

Not especially safely.

If you verified that all the compiler flags were the same and your
extension doesn't transitively bundled reference libraries that might be
different and incompatible versions (notably gettext, which Pg exposes in
its own headers) ... you're probably OK.

Again, in practice it generally works, but I wouldn't recommend it. Nor is
this something we can easily address with an extension API policy.


> How about other distributions which probably don't modify the source
> code?  Should the UDF be built with the target PostgreSQL because configure
> options may differ, which affects data structures


Yeah. And exposed ABI. I don't recommend it.

It's probably safe-ish on MS Windows, which is designed to allow greater
compatibility between executables built with differing toolchains and
options. I wouldn't do it on any unix.

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