Re: [HACKERS] pg_hba_file_settings view patch

2016-10-02 Thread Michael Paquier
On Mon, Oct 3, 2016 at 3:25 PM, Vitaly Burovoy  wrote:
> I guess for ability to use filtering like:
>
> SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE '%.example.com';
>
> I think it would be harder if options is an array of strings...

With unnest() and a matching pattern, not that hard but..
-- 
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_hba_file_settings view patch

2016-10-02 Thread Vitaly Burovoy
On 10/2/16, Michael Paquier  wrote:
> +   push_jsonb_string_key(&parseState, "map");
> +   push_jsonb_string_value(&parseState, hba->usermap);
> [...]
> +
> + options
> + jsonb
> + Configuration options set for authentication method
> +
> Why is it an advantage to use jsonb here instead of a simple array
> made of name=value? If they were nested I'd see a case for it but it
> seems to me that as presented this is just an overkill.

I guess for ability to use filtering like:

SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE '%.example.com';

I think it would be harder if options is an array of strings...

-- 
Best regards,
Vitaly Burovoy


-- 
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] GiST penalty functions [PoC]

2016-10-02 Thread Andrew Borodin
> This thread has basically died, so marking as returned with feedback.
Well, not exactly died, but it's kind of in blind lead.

I'll summarize a bit for future references:
1. cube extension for indexing lowered dimensionality data (2d in 3d)
is broken. Queries effectively will degrade to full index scan.
2. In existing GiST API this can be fixed only with technique,
implemented in this patch. But this technique seems controversial.
3. This patch also brings 30%-40% speedup, but expected speedup of
advanced GIST API techniques is much higher.
As Norbert Beckmann put it in other discussion:
> Overlap optimization [requires API advancement] is one of the main elements, 
> if not the main query performance tuning element of the RR*-tree. You would 
> fall back to old R-Tree times if that would be left off.

Outcome: I'm planning to rollout another patch for both GiST and cube,
probably to next CF, which will render this patch unnecessary.
Thanks to everyone for discussion.

Best regards, Andrey Borodin.


-- 
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] Commit fest 2016-09 is now closed

2016-10-02 Thread Michael Paquier
On Mon, Oct 3, 2016 at 2:01 PM, Michael Paquier
 wrote:
> There was something like 60~70 patches still listed as active in the
> CF app on Friday my time, so doing the vacuum cleanup has taken some
> time by marking patches as returned with feedback, rejected (few) and
> moved to next CF. A couple of things I noted when going through the
> remaining entries:
> - a lot of folks have registered to review a patch and did not post a
> review. Please avoid putting your name on a patch if you can't afford
> the time to look at a patch.
> - The rule one patch sent = one patch review of equal difficulty still works.
> - Be careful of the status of the patch, and update it correctly. I
> have noticed a lot of improvements in this area actually!

Forgot something: if you feel that your patch did not receive the
correct treatment, of course feel free to update its status in the CF
app, feel free as well to complain at me and/or on this thread if
necessary. I am sure we will be able to sort things out :)
-- 
Michael


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


[HACKERS] Commit fest 2016-09 is now closed

2016-10-02 Thread Michael Paquier
Hi all,

Per $subject, I would like to thank on behalf of Fabrizio everybody
who has taken the time to send patches, review them and argue about
them. This has been by experience the largest commit fest ever, with
more than 200 entries in total. The delay in stabilizing 9.6 is likely
the explanation behind such a high number, a lot of patches have been
parked there for a couple of months.

There was something like 60~70 patches still listed as active in the
CF app on Friday my time, so doing the vacuum cleanup has taken some
time by marking patches as returned with feedback, rejected (few) and
moved to next CF. A couple of things I noted when going through the
remaining entries:
- a lot of folks have registered to review a patch and did not post a
review. Please avoid putting your name on a patch if you can't afford
the time to look at a patch.
- The rule one patch sent = one patch review of equal difficulty still works.
- Be careful of the status of the patch, and update it correctly. I
have noticed a lot of improvements in this area actually!

Here is the final score of this session:
- Committed: 103.
- Moved to next CF: 51.
- Rejected: 12.
- Returned with Feedback: 53. Total: 219.

Thanks,
-- 
Michael


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


Re: [HACKERS] pg_hba_file_settings view patch

2016-10-02 Thread Michael Paquier
On Mon, Sep 5, 2016 at 4:09 PM, Haribabu Kommi  wrote:
> On Sun, Sep 4, 2016 at 1:44 AM, Simon Riggs  wrote:
>> On 15 August 2016 at 12:17, Haribabu Kommi 
>> wrote:
>>
>> > comments?
>>
>> This looks like a good feature contribution, thanks.
>>
>> At present the patch doesn't apply cleanly, please rebase.
>
>
> Rebased patch is attached.

Moved to next CF as there is a patch and no reviews.

+   push_jsonb_string_key(&parseState, "map");
+   push_jsonb_string_value(&parseState, hba->usermap);
[...]
+
+ options
+ jsonb
+ Configuration options set for authentication method
+
Why is it an advantage to use jsonb here instead of a simple array
made of name=value? If they were nested I'd see a case for it but it
seems to me that as presented this is just an overkill. In short, I
think that this patch needs a bit of rework, so I am marking it as
returned with feedback.
-- 
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] Declarative partitioning - another take

2016-10-02 Thread Amit Langote
On 2016/10/03 13:26, Michael Paquier wrote:
> On Fri, Sep 30, 2016 at 9:10 AM, Robert Haas  wrote:
>> On Thu, Sep 29, 2016 at 8:09 AM, Amit Langote
>>  wrote:
>>> I removed DEPENDENCY_IGNORE.  Does the following look good or am I still
>>> missing something?
>>
>> You missed your commit message, but otherwise looks fine.
> 
> I have moved this patch to next CF because that's fresh, but switched
> the patch as "waiting on author". Be careful, the patch was in "needs
> review".

Thanks Michael.  I was going to post the patch addressing Robert's
comments today, which I will anyway.  Then I will switch the status back
to "Needs Review", albeit as part of the next CF.

Thanks,
Amit




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


Re: [HACKERS] pg_xlogdump follow into the future

2016-10-02 Thread Michael Paquier
On Mon, Jul 18, 2016 at 8:10 PM, Magnus Hagander  wrote:
>
>
> On Thu, Jul 14, 2016 at 8:27 PM, Andres Freund  wrote:
>>
>> On 2016-07-14 13:46:23 +0200, Magnus Hagander wrote:
>> > Currently, if you run pg_xlogdump with -f, you have to specify an end
>> > position in an existing file, or if you don't it will only follow until
>> > the
>> > end of the current file.
>>
>> That's because specifying a file explicitly says that you only want to
>> look at that file, specifying two files that you want the range
>> inclusively between the two files.  -f works if you just use -s.
>
>
> Hmm. It does now. I'm *sure* it didn't when I was testing it. It must've
> been something else that was broken at that point :)

Same as Andres here, my understanding is that one file means that you
just want to look at this file, and two files permits to look at a
range of changes. But I have no argument against changing the current
behavior either.

>> > I'd appreciate a review of that by someone who's done more work on the
>> > xlog
>> > stuff, but it seems trivial to me. Not sure I can argue it's a bugfix
>> > though, since the usecase simply did not work...
>>
>> I'd say it's working as intended, and you want to change that
>> intent. That's fair, but I'd not call it a bug, and I'd say it's not
>> really 9.6 material.
>
> Based on that, I agree that it's working as intended.
>
> And definitely that it's not 9.6 material.
>
> I'll stick it on the CF page so I don't forget about it.

Moved to next CF. Magnus, you may want to finish wrapping that if you
still intend to change the current behavior.
-- 
Michael


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


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

2016-10-02 Thread Michael Paquier
On Sat, Jul 2, 2016 at 8:56 AM, Karl O. Pinc  wrote:
> 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 :
>
> 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.

Moved to next CF, the patch still applies. Karl, you have registered
to review this patch a couple of months back but nothing happened. I
have removed your name for now. If you have time, don't hesitate to
come back to 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] sequence data type

2016-10-02 Thread Michael Paquier
On Sun, Sep 11, 2016 at 12:38 PM, Vitaly Burovoy
 wrote:
> On 9/10/16, Jim Nasby  wrote:
>> On 9/3/16 6:01 PM, Gavin Flower wrote:
>>> I am curious as to the use cases for other possibilities.
>>
>> A hex or base64 type might be interesting, should those ever get created...
>> --
>> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
>
> Hex or base64 are not data types. They are just different
> representation types of binary sequences.
> Even for bigints these representations are done after writing numbers
> as byte sequences.

Moved to next CF. The patch did not actually receive that much reviews.
-- 
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] raw output from copy

2016-10-02 Thread Michael Paquier
On Sat, Jul 16, 2016 at 5:55 PM, Pavel Stehule  wrote:
> I am sending fresh version of COPY RAW patch.

Moved to next CF per this status.

+++ b/src/interfaces/libpq/test/copy-raw-regress.pl
@@ -0,0 +1,48 @@
+#!/usr/bin/perl -w
+
+use strict;
I don't understand why this is shaped this way, I mean the perl part
if we have the TAP infra in place. MSVC is not testing it as well.
-- 
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] Declarative partitioning - another take

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 9:10 AM, Robert Haas  wrote:
> On Thu, Sep 29, 2016 at 8:09 AM, Amit Langote
>  wrote:
>> I removed DEPENDENCY_IGNORE.  Does the following look good or am I still
>> missing something?
>
> You missed your commit message, but otherwise looks fine.

I have moved this patch to next CF because that's fresh, but switched
the patch as "waiting on author". Be careful, the patch was in "needs
review".
-- 
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] NOT EXIST for PREPARE

2016-10-02 Thread Michael Paquier
On Sun, Jun 5, 2016 at 11:33 AM, David G. Johnston
 wrote:
> As an aside; most (all?) of our INEs apply to persistent schema objects.
> Extending that to session objects is a conceptual leap.

There is close to no activity here, so I marked the patch as returned
with feedback. I am doubtful about the performance btw..
-- 
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] asynchronous and vectorized execution

2016-10-02 Thread Kyotaro HORIGUCHI
At Mon, 3 Oct 2016 13:14:23 +0900, Michael Paquier  
wrote in 
> On Thu, Sep 29, 2016 at 5:50 PM, Kyotaro HORIGUCHI
>  wrote:
> > Sorry for no response, but, the answer is yes. We could be able
> > to avoid the problem by managing execution state for every
> > node. But it needs an additional flag in *State structs and
> > manipulating on the way shuttling up and down around the
> > execution tree.
> 
> Moved to next CF.

Thank you.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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


Re: [HACKERS] delta relations in AFTER triggers

2016-10-02 Thread Michael Paquier
On Sat, Sep 10, 2016 at 7:28 AM, Kevin Grittner  wrote:
> v6 fixes recently-introduced bit-rot.

Not as big as I thought, only 2k when both patches are combined... The
patch without noapi in its name needs to be applied first, and after
the patch with noapi can be applied.
 60 files changed, 2073 insertions(+), 63 deletions(-)
Moved to next CF.
-- 
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] sequences and pg_upgrade

2016-10-02 Thread Michael Paquier
On Sat, Oct 1, 2016 at 1:50 AM, Anastasia Lubennikova
 wrote:
> 23.09.2016 21:06, Peter Eisentraut:
>>
>> Here is an updated patch set.  Compared to the initial set, I have
>> changed pg_dump's sorting priorities so that sequence data is always
>> after table data.  This would otherwise have introduced a problem
>> because sortDataAndIndexObjectsBySize() only considers consecutive
>> DO_TABLE_DATA entries.  Also, I have removed the separate
>> --sequence-data switch from pg_dump and made it implicit in
>> --binary-upgrade.  (So the previous patches 0002 and 0003 have been
>> combined, because it's no longer a separate feature.)
>>
>
> The patches are good, no complaints.
> But again, I have the same question.
> I was confused, why do we always dump sequence data,
> because I'd overlooked the --sequence-data key. I'd rather leave this
> option,
> because it's quite non intuitive behaviour...
>  /* dump sequence data even in schema-only mode */

Moved to next CF. This is fresh.
-- 
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] asynchronous and vectorized execution

2016-10-02 Thread Michael Paquier
On Thu, Sep 29, 2016 at 5:50 PM, Kyotaro HORIGUCHI
 wrote:
> Sorry for no response, but, the answer is yes. We could be able
> to avoid the problem by managing execution state for every
> node. But it needs an additional flag in *State structs and
> manipulating on the way shuttling up and down around the
> execution tree.

Moved to next CF.
-- 
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] Aggregate Push Down - Performing aggregation on foreign server

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 8:58 PM, Jeevan Chalke
 wrote:
> On Mon, Sep 26, 2016 at 6:15 PM, Ashutosh Bapat
>  wrote:
>>
>> This patch will need some changes to conversion_error_callback(). That
>> function reports an error in case there was an error converting the
>> result obtained from the foreign server into an internal datum e.g.
>> when the string returned by the foreign server is not acceptable by
>> local input function for the expected datatype. In such cases, the
>> input function will throw error and conversion_error_callback() will
>> provide appropriate context for that error. postgres_fdw.sql has tests
>> to test proper context
>> We need to fix the error context to provide meaningful information or
>> at least not crash. This has been discussed briefly in [1].
>
> Oops. I had that in mind when working on this. Somehow skipped checking
> for conversion error context. I have fixed that in v3 patch.
> Removed assert and for non Var expressions, printing generic context.
> This context is almost in-line with the discussion you referred here.

Moved to next CF.
-- 
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] Macro customizable hashtable / bitmapscan & aggregation perf

2016-10-02 Thread Andres Freund
On 2016-10-02 02:59:04 +0200, Tomas Vondra wrote:
> On 10/02/2016 02:17 AM, Andres Freund wrote:
> > Hi,
> > ...
> > 
> > > > > I think a crucial part of the benchmarking will be identifying and
> > > > > measuring corner cases, e.g. a lot of rows with the same key, etc.
> > > > > Although that probably is not a major issue for the two places
> > > > > switched to the new implementation (e.g. execGrouping merges the
> > > > > duplicates to a single group, by definition).
> > > > 
> > > > Hm. I don't really see a case where that's going to cause issues - all
> > > > the execGrouping.c users only store one key, and either ignore later
> > > > ones, or add the data to the initial tuple in the group.  I don't really
> > > > see any case where repeated keys should cause an issue for a hash table?
> > > > 
> > > 
> > > Yep, that's pretty much what I suggested. I was wondering more about the
> > > other places where this hash table might be used - I've been thinking 
> > > about
> > > hashjoins, but now that I think of it - that's a fairly specialized and
> > > tuned code. In any case, let's not complicate the discussion for now.
> > 
> > My point is that I don't really see any potential usecase where
> > that's an issue.
> > 
> 
> OK, I think we agree the two places modified by the submitted patch series
> are fine. Let's leave discussion about places modified by future patches for
> the future ;-)

I think my problem here is that I just can't see a potential use-case
for hashtables where that's an issue ;)


> > > 5) SH_RESIZE
> > > 
> > > Do we expect to shrink hash tables?
> > 
> > Not at the moment. Should be doable, but I'm not sure it's worth
> > developing and testing - I don't see any usecases in pg atm.
> > 
> 
> OK, sure. Then what about adding this to SH_RESIZE?
> 
> Assert(oldsize <= newsize);

Yes. And potentially rename it to SH_GROW.


> I assumed we might shrink the catcache after invalidation, for example (but
> maybe I don't really know how that works).

They don't shrink so far, and in most invalidation cases we don't delete
entries, just mark them as invalid (as they might be referenced on the
outside at that moment).


> > > It's not entirely clear why is it guaranteed that there's always
> > > an element with optimal position (when load factor < 1)? Adding an
> > >  explanation or a link to a paper would be helpful, I guess.
> > 
> > As the load factor always has to be below 1, there always has to be
> > an empty bucket. Every bucket after an empty bucket has to be at
> > it's optimal position.

> Hmmm, thanks to moving the entries after delete?

Pretty much, yes.


> Adding an assert() enforcing this seems like a good idea.

Probably requires some additional state to be meaningfully enforced. Not
sure that's worth the effort. If that invariant is broken, not a lot of
stuff works (believe me, I have broken it ;)). Otherwise searches won't
necessarily work anymore, if they didn't end up in their original
position (as the cell would now be empty, a lookup/insert/delete would
not find the existing element).


> > > 7) Should hash agg size estimation for the planner consider the 
> > > fillfactor?
> > > 
> > > I think we should account for fillfactor - we should account for the
> > > allocated memory, even if there's free space in the hash table. After all,
> > > that's what hashjoins do.
> > 
> > We didn't really for dynahash (as it basically assumed a fillfactor of 1
> > all the time), not sure if changing it won't also cause issues.
> > 
> 
> That's a case of glass half-full vs. half-empty, I guess. If we assumed load
> factor 1.0, then I see it as accounting for load factor (while you see it as
> not accounting of it).

Well, even before we grow by factors of two, so 1 wasn't accurate for
most of the time.  My issue isn't really that I don't want to do it, but
that I'm not entirely sure that there's a good way. TupleHashEntryData
itself isn't exactly large, and we'd have to multiply it by the load
factor. At the same time, after the patch, AggStatePerGroupData isn't
allocated for empty elements anymore, and that's the largest part of the
memory.  So I'm kind of inclined to just disregard the fillfactor (and
document that).

> Also, with load factor 0.8 the table is only ~20% larger, so even if we
> don't include that into work_mem it's a reasonably small error (easily
> smaller than errors in the pre-9.5 HashJoin accounting, which did not
> include chunk headers etc.).

I think in either cases the entries themselves are smaller after the
patch by enough that the overhead doesn't matter once you crossed a few
dozen entries.

Regards,

Andres


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


Re: [HACKERS] Multi-tenancy with RLS

2016-10-02 Thread Michael Paquier
On Tue, Jul 19, 2016 at 3:42 PM, Haribabu Kommi
 wrote:
> The above changes are based on my understanding to the discussion occurred in
> this mail. In case if I miss anything, please let me know, i will
> correct the same.

The patch series still apply.

+   " ((classid = (select oid from pg_class where
relname = 'pg_aggregate'))"
+   " OR (classid = (select oid from pg_class where
relname = 'pg_cast') AND has_cast_privilege(objid, 'any'))"
+   " OR (classid = (select oid from pg_class where
relname = 'pg_collation'))"
[... long list ...]
That's quite hard to digest...

+static bool
+get_catalog_policy_string(Oid relationid, Form_pg_class
pg_class_tuple, char *buf)
This is an exceptionally weak interface at quick glance. This is using
SQL strings, and nothing is actually done regarding potentially
conflicting name types...

The number of new files included in policy.c is impressive as well..

This does not count as a full review though, so I am moving it to next
CF. Perhaps it will find its audience.
-- 
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_sequence catalog

2016-10-02 Thread Michael Paquier
On Sun, Sep 11, 2016 at 2:15 PM, Amit Kapila  wrote:
> On Sun, Sep 11, 2016 at 12:39 AM, Andres Freund  wrote:
>> On 2016-09-10 17:23:21 +0530, Amit Kapila wrote:
>>> >
>>>
>>> I may be missing something here, but why would it contend on a lock,
>>> as per locking scheme proposed by Alvaro, access to sequence object
>>> will need a share lock on buffer page.
>>
>> To make checkpointing/bgwriter work correctly we need exclusive locks on
>> pages while writing..., or some new lock level preventing the page from
>> being written out, while "shared dirtying" locks are being held.
>>
>
> Right and I think you have a very valid concern, but if we think that
> storing multiple sequences on a same page is a reasonable approach,
> then we can invent some locking mechanism as indicated by you such
> that two writes on same page won't block each other, but they will be
> blocked with bgwriter/checkpointer.

This thread has died a couple of weeks back, so I am marking it as
returned with feedback by seeing the discussion that has been done.
Feel free to update the patch if you think that's not adapted.
-- 
Michael


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


Re: [HACKERS] [PATCH] Logical decoding timeline following take II

2016-10-02 Thread Michael Paquier
On Thu, Sep 1, 2016 at 1:08 PM, Craig Ringer  wrote:
> Attached is a rebased and updated logical decoding timeline following
> patch for 10.0.

Moved to next CF, nothing has happened since submission.
-- 
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] Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

2016-10-02 Thread Michael Paquier
On Fri, Sep 16, 2016 at 6:12 AM, Tom Lane  wrote:
> I'm happy to get rid of the LCM behavior, I just want to have some wiggle
> room to be able to get it back if somebody really needs it.

Er, actually no that's this thread for this CF entry:
https://commitfest.postgresql.org/10/759/
Still there has not been much activity.
-- 
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] Changed SRF in targetlist handling

2016-10-02 Thread Michael Paquier
On Wed, Aug 24, 2016 at 3:55 AM, Andres Freund  wrote:
> Comments?

This thread has no activity for some time now and it is linked to this CF entry:
https://commitfest.postgresql.org/10/759/
I am marking it as returned with feedback..
-- 
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] Stopping logical replication protocol

2016-10-02 Thread Craig Ringer
On 3 Oct. 2016 10:15, "Michael Paquier"  wrote:
>
> On Tue, Sep 27, 2016 at 11:05 AM, Craig Ringer 
wrote:
> > On 26 September 2016 at 21:52, Vladimir Gordiychuk 
wrote:
> >>>You should rely on time I tests as little as possible. Some of the test
> >>> hosts are VERY slow. If possible something deterministic should be
used.
> >>
> >> That's why this changes difficult to verify. Maybe in that case we
should
> >> write benchmark but not integration test?
> >> In that case we can say, before this changes stopping logical
replication
> >> gets N ms but after apply changes it gets NN ms where NN ms less than
N ms.
> >> Is it available add this kind of benchmark to postgresql? I will be
grateful
> >> for links.
> >
> > It's for that reason that I added a message printed only in verbose
> > mode that pg_recvlogical emits when it's exiting after a
> > client-initiated copydone.
> >
> > You can use the TAP tests, print diag messages, etc. But we generally
> > want them to run fairly quickly, so big benchmark runs aren't
> > desirable. You'll notice that I left diag messages in to report the
> > timing for the results in your tests, I just changed the tests so they
> > didn't depend on very tight timing for success/failure anymore.
> >
> > We don't currently have any automated benchmarking infrastructure.
>
> Which seems like this patch is not complete yet.

Personally I think it is. I'm just explaining why I adjusted Vladimir's
tests to be less timing sensitive and rely on a qualitative property
instead.

That said, it's had recent change and it isn't a big intrusive change that
really need attention this cf.

>  I am marking it as
> returned with feedback, but it may be a better idea to move it to next
> CF once a new version with updated tests shows up.

I'll move it now. I think the tests are fine, if Vladimir agrees, so IMO
it's ready to go. More eyes wouldn't hurt though.

If Vladimir wants benchmarking based tests that's a whole separate project
IMO. Something that will work robustly on the weird slow machines we have
isn't simple. Probably a new buildfarm option etc since we won't want to
clutter the really slow old niche boxes with it.

Vladimir, can I have your opinion on the latest patch or if you want to
make changes, an updated patch?


Re: [HACKERS] Pinning a buffer in TupleTableSlot is unnecessary

2016-10-02 Thread Michael Paquier
On Wed, Aug 31, 2016 at 6:03 AM, Andres Freund  wrote:
> On 2016-08-30 21:59:44 +0100, Greg Stark wrote:
>> On Tue, Aug 30, 2016 at 11:12 AM, Heikki Linnakangas  wrote:
>> > While profiling some queries and looking at executor overhead, I realized
>> > that we're not making much use of TupleTableSlot's ability to hold a buffer
>> > pin. In a SeqScan, the buffer is held pinned by the underlying heap-scan
>> > anyway. Same with an IndexScan, and the SampleScan. The only thing that
>> > relies on that feature is TidScan, but we could easily teach TidScan to 
>> > hold
>> > the buffer pin directly.
>> >
>> > So, how about we remove the ability of a TupleTableSlot to hold a buffer
>> > pin, per the attached patch? It shaves a few cycles from a ExecStoreTuple()
>> > and ExecClearTuple(), which get called a lot. I couldn't measure any actual
>> > difference from that, though, but it seems like a good idea from a
>> > readability point of view, anyway.
>>
>> Out of curiosity why go in this direction and not the other? Why not
>> move those other scans to use the TupleTableSlot API to manage the
>> pins. Offhand it sounds more readable not less to have the
>> TupleTableSlot be a self contained data structure that guarantees the
>> lifetime of the pins matches the slots rather than have a dependency
>> on the code structure in some far-away scan.
>
> At least for heap scans the pins are page level, and thus longer lived
> than the data in a slot. It's important that a scan holds a pin, because
> it needs to rely on the page not being hot pruned etc..

I have moved this patch to next CF. Heikki, are you still planning to
work on 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] PATCH: Batch/pipelining support for libpq

2016-10-02 Thread Craig Ringer
On 3 October 2016 at 10:10, Michael Paquier  wrote:
> On Tue, Sep 6, 2016 at 8:01 PM, Craig Ringer  wrote:
>> On 6 September 2016 at 16:10, Daniel Verite  wrote:
>>> Craig Ringer wrote:
>>>
 Updated patch attached.
>>>
>>> Please find attached a couple fixes for typos I've came across in
>>> the doc part.
>>
>> Thanks, will apply and post a rebased patch soon, or if someone picks
>> this up in the mean time they can apply your diff on top of the patch.
>
> Could you send an updated patch then? At the same time I am noticing
> that git --check is complaining... This patch has tests and a
> well-documented feature, so I'll take a look at it soon at the top of
> my list. Moved to next CF for now.

Thanks.

I'd really like to teach psql in non-interactive mode to use it, but
(a) I'm concerned about possible subtle behaviour differences arising
if we do that and (b) I won't have the time. I think it's mostly of
interest to app authors and driver developers and that's what it's
aimed at. pg_restore could benefit a lot too.

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


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


Re: [HACKERS] GiST penalty functions [PoC]

2016-10-02 Thread Michael Paquier
On Thu, Sep 22, 2016 at 3:45 AM, Andrew Borodin  wrote:
> [blah]
>
> Practically, this algorithm cannot be implemented in current GiST API
> (only if we provide non-penalty-based choose subtree function,
> optional for GiST extension), but it certainly has scientific value.

This thread has basically died, so marking as returned with feedback.
If you feel that's not adapted, feel free to move it to next CF.
-- 
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] Cache Hash Index meta page.

2016-10-02 Thread Michael Paquier
On Thu, Sep 29, 2016 at 12:55 AM, Mithun Cy  wrote:
> On Tue, Sep 27, 2016 at 1:53 AM, Jeff Janes  wrote:
>  > I think that this needs to be updated again for v8 of concurrent and v5
> of wal
>
> Adding the rebased patch over [1] + [2]
>
> [1] Concurrent Hash index.
> [2] Wal for hash index.

Moved to next CF.
-- 
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] Proposal: speeding up GIN build with parallel workers

2016-10-02 Thread Michael Paquier
On Wed, Sep 14, 2016 at 3:48 PM, Heikki Linnakangas  wrote:
> If we flushed the tree to a tape instead, then we could perhaps use the
> machinery that Peter's parallel B-tree patch is adding to tuplesort.c, to
> merge the tapes. I'm not sure if that works out, but I think it's worth some
> experimentation.

Marking as returned with feedback per mainly this comment.
-- 
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] Tracking wait event for latches

2016-10-02 Thread Thomas Munro
On Fri, Sep 30, 2016 at 5:47 PM, Michael Paquier
 wrote:
> On Fri, Sep 30, 2016 at 1:48 AM, Robert Haas  wrote:
>> The way that we're constructing the wait event ID that ultimately gets
>> advertised in pg_stat_activity is a bit silly in this patch.  We start
>> with an event ID (which is an integer) and we then do an array lookup
>> (via GetWaitEventClass) to get a second integer which is then XOR'd
>> back into the first integer (via pgstat_report_wait_start) before
>> storing it in the PGPROC.  New idea: let's change
>> pgstat_report_wait_start() to take a single integer argument which it
>> stores without interpretation.  Let's separate the construction of the
>> wait event into a separate macro, say make_wait_event().  Then
>> existing callers of pgstat_report_wait_start(a, b) will instead do
>> pgstat_report_wait_start(make_wait_event(a, b)), but callers that want
>> to construct the wait event and push it through a few levels of the
>> call stack before advertising it only need to pass one integer.  If
>> done correctly, this should be cleaner and faster than what you've got
>> right now.
>
> Hm, OK So ProcSleep() and ProcWaitForSignal() need an additional
> argument in the shape of a uint32 wait_event. So we need to change the
> shape of WaitLatch&friends to have also just a uint32 as an extra
> argument. This has as result to force all the callers of WaitLatch to
> use the new routine pgstat_make_wait_event() (renamed it), so pgstat.h
> needs to be included where WaitLatch() is called.

Hmm.  I like the use of pgstat in that name.  It helps with the
confusion created by the overloading of the term 'wait event' in the
pg_stat_activity view and the WaitEventSet API, by putting it into a
different pseudo-namespace.

+ uint32 wait_event;

How about a typedef for that instead of using raw uint32 everywhere?
Something like pgstat_wait_descriptor?  Then a variable name like
pgstat_desc, since this is most definitely not just a wait_event
anymore.

+ /* Define event to wait for */

It's not defining the event to wait for at all, it's building a
description for pgstat.

+ wait_event = pgstat_make_wait_event(WAIT_EXTENSION,
+ WAIT_EVENT_EXTENSION);

It's not making a wait event, it's combining a class and an event.
How about something like this:

pgstat_desc = pgstat_make_wait_descriptor(WAIT_CLASS_EXTENSION,
WAIT_EVENT_EXTENSION)?

  /* Sleep until there's something to do */
  wc = WaitLatchOrSocket(MyLatch,
WL_LATCH_SET | WL_SOCKET_READABLE,
PQsocket(conn),
-   -1L);
+   -1L,
+   wait_event);

... then use 'pgstat_desc' here.

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


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


Re: [HACKERS] [COMMITTERS] pgsql: Copy-editing for contrib/pg_visibility documentation.

2016-10-02 Thread Michael Paquier
On Mon, Oct 3, 2016 at 11:06 AM, Robert Haas  wrote:
> On Sun, Oct 2, 2016 at 9:00 PM, Thomas Munro
>  wrote:
>> On Mon, Oct 3, 2016 at 1:36 PM, Robert Haas  wrote:
>>> On Sat, Oct 1, 2016 at 3:33 PM, Tom Lane  wrote:
 Copy-editing for contrib/pg_visibility documentation.

 Add omitted names for some function parameters.
 Fix some minor grammatical issues.
>>>
>>> Why do you keep insisting on changing case where I've written "which"
>>> to instead say "that" in situations where AFAIK either is perfectly
>>> correct?  I find such changes at best neutral, and in some cases
>>> worse.
>>
>> 'Which' looks OK to me too here, but I speak some kind of British
>> English, and it looks like at least some writers of formal American
>> English prefer 'that' here:
>>
>> * 
>> http://www.chicagomanualofstyle.org/qanda/data/faq/topics/Whichvs.That.html?old=Whichvs.That01.html
>> * https://en.oxforddictionaries.com/usage/that-or-which
>
> Gosh, I think "she held out the hand that was hurt" is clearly worse
> than "she held out the hand which was hurt", but even if someone
> prefers the opposite, correcting it as a supposed grammatical error
> strikes me as arrant pedantry.  You know, the kind up with which I
> don't particularly wish to put.

Interesting, "which" seems more natural to me, perhaps because of
classes at school based on "Britain English". My teachers sometimes
referred to "American English" as something to run away from as far as
possible.
-- 
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] Notice lock waits

2016-10-02 Thread Michael Paquier
On Mon, Oct 3, 2016 at 11:40 AM, Michael Paquier
 wrote:
> On Fri, Sep 30, 2016 at 2:00 AM, Jeff Janes  wrote:
>> What do you think of Jim Nasby's idea of making a settable level, rather
>> just on or off?
>
> [reading the code]
> That would be a better idea. The interface proposed, aka 2 GUCs doing
> basically the same thing is quite confusing I think. I am marking the
> patch as returned with feedback for now.

Forgot to mention that I also found myself enforcing
client_min_messages to warning to avoid annoying NOTICE messages.
-- 
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] Logical Replication WIP

2016-10-02 Thread Michael Paquier
On Wed, Sep 28, 2016 at 10:12 PM, Peter Eisentraut
 wrote:
> On 9/23/16 9:28 PM, Petr Jelinek wrote:
>>> Document to what extent other relation types are supported (e.g.,
>>> > materialized views as source, view or foreign table or temp table as
>>> > target).  Suggest an updatable view as target if user wants to have
>>> > different table names or write into a different table structure.
>>> >
>> I don't think that's good suggestion, for one it won't work for UPDATEs
>> as we have completely different path for finding the tuple to update
>> which only works on real data, not on view. I am thinking of even just
>> allowing table to table replication in v1 tbh, but yes it should be
>> documented what target relation types can be.
>
> I'll generalize this then to: Determine which relation types should be
> supported at either end, document that, and then make sure it works that
> way.  A restrictive implementation is OK for the first version, as long
> as it keeps options open.

The newest patch is 3-week old, so marking this entry as returned with feedback.
-- 
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] Add support for restrictive RLS policies

2016-10-02 Thread Michael Paquier
On Thu, Sep 29, 2016 at 7:18 PM, Jeevan Chalke
 wrote:
> Hi Stephen,
>
>
>> > 4. It will be good if we have an example for this in section
>> > "5.7. Row Security Policies"
>>
>> I haven't added one yet, but will plan to do so.
>>
> I think you are going to add this in this patch itself, right?
>
> I have reviewed your latest patch and it fixes almost all my review
> comments.
> Also I am agree with your responses for couple of comments like response on
> ALTER POLICY and tab completion. No issues with that.
>
> However in documentation, PERMISSIVE and RESTRICTIVE are actually literals
> and not parameters as such.  Also can we combine these two options into one
> like below (similar to how we document CASCADE and RESTRICT for DROP
> POLICY):
>
>
> PERMISSIVE
> RESTRICTIVE
>
> 
>  
> ... explain PERMISSIVE ...
>  
>  
> ... explain RESTRICTIVE ...
>  
> 
>
>
> Apart from this changes look excellent to me.

I have moved that to next CF, my guess is that Stephen is going to
update soon and the activity is fresh.
-- 
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] Notice lock waits

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 2:00 AM, Jeff Janes  wrote:
> What do you think of Jim Nasby's idea of making a settable level, rather
> just on or off?

[reading the code]
That would be a better idea. The interface proposed, aka 2 GUCs doing
basically the same thing is quite confusing I think. I am marking the
patch as returned with feedback for now.
-- 
Michael


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


Re: [HACKERS] Bug in to_timestamp().

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 12:34 PM, Amul Sul  wrote:
> The new status of this patch is: Ready for Committer

And moved to next CF with same status.
-- 
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] WIP: Covering + unique indexes.

2016-10-02 Thread Michael Paquier
On Tue, Sep 27, 2016 at 12:17 AM, Anastasia Lubennikova
 wrote:
> Ok, I'll write it in a few days.

Marked as returned with feedback per last emails exchanged.
-- 
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] Rename max_parallel_degree?

2016-10-02 Thread Michael Paquier
On Sat, Oct 1, 2016 at 1:23 AM, Julien Rouhaud
 wrote:
> On 23/09/2016 21:10, Robert Haas wrote:
>> On Fri, Sep 23, 2016 at 8:54 AM, Peter Eisentraut
>>  wrote:
>>> On 9/20/16 4:07 PM, Robert Haas wrote:
 No, I'm assuming that the classes would be built-in.  A string tag
 seems like over-engineering to me, particularly because the postmaster
 needs to switch on the tag, and we need to be very careful about the
 degree to which the postmaster trusts the contents of shared memory.
>>>
>>> I'm hoping that we can come up with something that extensions can
>>> participate in, without the core having to know ahead of time what those
>>> extensions are or how they would be categorized.
>>>
>>> My vision is something like
>>>
>>> max_processes = 512  # requires restart
>>>
>>> process_allowances = 'connection:300 superuser:10 autovacuum:10
>>> parallel:30 replication:10 someextension:20 someotherextension:20'
>>> # does not require restart
>>
>> I don't think it's going to be very practical to allow extensions to
>> participate in the mechanism because there have to be a finite number
>> of slots that is known at the time we create the main shared memory
>> segment.
>>
>> Also, it's really important that we don't add lots more surface area
>> for the postmaster to crash and burn.
>>
>
> It seems that there's no objection on Robert's initial proposal, so I'll
> try to implement it.
>
> I've already fixed every other issues mentioned upthread, but I'm facing
> a problem for this one.  Assuming that the bgworker classes are supposed
> to be mutually exclusive, I don't see a simple and clean way to add such
> a check in SanityCheckBackgroundWorker().  Am I missing something
> obvious, or can someone give me some advice for this?

Okay, so marking it as returned with feedback is adapted? I have done
so but feel free to contradict me.
-- 
Michael


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


Re: [HACKERS] patch: function xmltable

2016-10-02 Thread Michael Paquier
On Tue, Sep 27, 2016 at 2:29 PM, Pavel Stehule  wrote:
> 2016-09-27 5:53 GMT+02:00 Craig Ringer :
>>
>> [...]
>> ... so please delete that text. I thought I'd tested it but the state
>> of my tests dir says I just got distracted by another task at the
>> wrong time.

Moved patch to next CF with same status: ready for committer.
-- 
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] Transactions involving multiple postgres foreign servers

2016-10-02 Thread Michael Paquier
On Wed, Sep 28, 2016 at 3:30 PM, Ashutosh Bapat
 wrote:
> I agree, but we need to cope with above two problems.

I have marked the patch as returned with feedback per the last output
Ashutosh has provided.
-- 
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] Stopping logical replication protocol

2016-10-02 Thread Michael Paquier
On Tue, Sep 27, 2016 at 11:05 AM, Craig Ringer  wrote:
> On 26 September 2016 at 21:52, Vladimir Gordiychuk  wrote:
>>>You should rely on time I tests as little as possible. Some of the test
>>> hosts are VERY slow. If possible something deterministic should be used.
>>
>> That's why this changes difficult to verify. Maybe in that case we should
>> write benchmark but not integration test?
>> In that case we can say, before this changes stopping logical replication
>> gets N ms but after apply changes it gets NN ms where NN ms less than N ms.
>> Is it available add this kind of benchmark to postgresql? I will be grateful
>> for links.
>
> It's for that reason that I added a message printed only in verbose
> mode that pg_recvlogical emits when it's exiting after a
> client-initiated copydone.
>
> You can use the TAP tests, print diag messages, etc. But we generally
> want them to run fairly quickly, so big benchmark runs aren't
> desirable. You'll notice that I left diag messages in to report the
> timing for the results in your tests, I just changed the tests so they
> didn't depend on very tight timing for success/failure anymore.
>
> We don't currently have any automated benchmarking infrastructure.

Which seems like this patch is not complete yet. I am marking it as
returned with feedback, but it may be a better idea to move it to next
CF once a new version with updated tests shows up.
-- 
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] PL/Python adding support for multi-dimensional arrays

2016-10-02 Thread Michael Paquier
On Sat, Oct 1, 2016 at 8:45 AM, Jim Nasby  wrote:
> On 9/29/16 1:51 PM, Heikki Linnakangas wrote:
>>
>> Jim, I was confused, but you agreed with me. Were you also confused, or
>> am I missing something?
>
>
> I was confused by inputs:

I have marked the patch as returned with feedback. Or Heikki, do you
plan on looking at it more and commit soon?
-- 
Michael


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


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

2016-10-02 Thread Michael Paquier
On Tue, Sep 6, 2016 at 8:01 PM, Craig Ringer  wrote:
> On 6 September 2016 at 16:10, Daniel Verite  wrote:
>> Craig Ringer wrote:
>>
>>> Updated patch attached.
>>
>> Please find attached a couple fixes for typos I've came across in
>> the doc part.
>
> Thanks, will apply and post a rebased patch soon, or if someone picks
> this up in the mean time they can apply your diff on top of the patch.

Could you send an updated patch then? At the same time I am noticing
that git --check is complaining... This patch has tests and a
well-documented feature, so I'll take a look at it soon at the top of
my list. Moved to next CF for now.
-- 
Michael


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


Re: [HACKERS] [COMMITTERS] pgsql: Copy-editing for contrib/pg_visibility documentation.

2016-10-02 Thread Robert Haas
On Sun, Oct 2, 2016 at 9:00 PM, Thomas Munro
 wrote:
> On Mon, Oct 3, 2016 at 1:36 PM, Robert Haas  wrote:
>> On Sat, Oct 1, 2016 at 3:33 PM, Tom Lane  wrote:
>>> Copy-editing for contrib/pg_visibility documentation.
>>>
>>> Add omitted names for some function parameters.
>>> Fix some minor grammatical issues.
>>
>> Why do you keep insisting on changing case where I've written "which"
>> to instead say "that" in situations where AFAIK either is perfectly
>> correct?  I find such changes at best neutral, and in some cases
>> worse.
>
> 'Which' looks OK to me too here, but I speak some kind of British
> English, and it looks like at least some writers of formal American
> English prefer 'that' here:
>
> * 
> http://www.chicagomanualofstyle.org/qanda/data/faq/topics/Whichvs.That.html?old=Whichvs.That01.html
> * https://en.oxforddictionaries.com/usage/that-or-which

Gosh, I think "she held out the hand that was hurt" is clearly worse
than "she held out the hand which was hurt", but even if someone
prefers the opposite, correcting it as a supposed grammatical error
strikes me as arrant pedantry.  You know, the kind up with which I
don't particularly wish to put.

-- 
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] Block level parallel vacuum WIP

2016-10-02 Thread Michael Paquier
On Fri, Sep 16, 2016 at 6:56 PM, Masahiko Sawada  wrote:
> Yeah, I don't have a good solution for this problem so far.
> We might need to improve group locking mechanism for the updating
> operation or came up with another approach to resolve this problem.
> For example, one possible idea is that the launcher process allocates
> vm and fsm enough in advance in order to avoid extending fork relation
> by parallel workers, but it's not resolve fundamental problem.

Marked as returned with feedback because of lack of activity and...
Feedback provided.
-- 
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_upgade vs config

2016-10-02 Thread Andrew Dunstan



On 10/02/2016 07:21 PM, Tom Lane wrote:

I wrote:

It occurs to me that a back-patchable workaround for this would be to
make get_loadable_libraries sort the library names in order by length
(and I guess we might as well sort same-length names alphabetically).
This would for example guarantee that hstore_plpython is probed after
both hstore and plpython.  Admittedly, this is a kluge of the first
water.  But I see no prospect of back-patching any real fix, and it
would definitely be better if pg_upgrade didn't fail on these modules.

I've tested the attached and verified that it allows pg_upgrade'ing
of the hstore_plpython regression DB --- or, if I reverse the sort
order, that it reproducibly fails.  I propose back-patching this
at least as far as 9.5, where the transform modules came in.  It might
be a good idea to go all the way back, just so that the behavior is
predictable.




Yeah, it's a really ugly kluge, but I don't have a better idea.

cheers

andrew


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


Re: [HACKERS] Optimization for lazy_scan_heap

2016-10-02 Thread Michael Paquier
On Mon, Sep 26, 2016 at 5:26 AM, Rahila Syed  wrote:
> Some initial comments on optimize_lazy_scan_heap_v2.patch.

Seems worth pursuing. Marking as returned with feedback because of
lack of activity and some basic reviews sent.
-- 
Michael


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


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

2016-10-02 Thread Michael Paquier
On Mon, Sep 5, 2016 at 1:53 PM, Pavan Deolasee  wrote:
> 0001_track_root_lp_v4.patch: This patch uses a free t_infomask2 bit to track
> latest tuple in an update chain. The t_ctid.ip_posid is used to track the
> root line pointer of the update chain. We do this only in the latest tuple
> in the chain because most often that tuple will be updated and we need to
> quickly find the root only during update.
>
> 0002_warm_updates_v4.patch: This patch implements the core of WARM logic.
> During WARM update, we only insert new entries in the indexes whose key has
> changed. But instead of indexing the real TID of the new tuple, we index the
> root line pointer and then use additional recheck logic to ensure only
> correct tuples are returned from such potentially broken HOT chains. Each
> index AM must implement a amrecheck method to support WARM. The patch
> currently implements this for hash and btree indexes.

Moved to next CF, I was surprised to see that it is not *that* large:
 43 files changed, 1539 insertions(+), 199 deletions(-)
-- 
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] Hash Indexes

2016-10-02 Thread Michael Paquier
On Mon, Oct 3, 2016 at 12:42 AM, Pavel Stehule  wrote:
>
>
> 2016-10-02 12:40 GMT+02:00 Michael Paquier :
>>
>> On Sun, Oct 2, 2016 at 3:31 AM, Greg Stark  wrote:
>> > On Fri, Sep 30, 2016 at 2:11 AM, Robert Haas 
>> > wrote:
>> >> For one thing, we can stop shipping a totally broken feature in release
>> >> after release
>> >
>> > For what it's worth I'm for any patch that can accomplish that.
>> >
>> > In retrospect I think we should have done the hash-over-btree thing
>> > ten years ago but we didn't and if Amit's patch makes hash indexes
>> > recoverable today then go for it.
>>
>> +1.
>
> +1

And moved to next CF to make it breath.
-- 
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] PoC: Partial sort

2016-10-02 Thread Michael Paquier
On Tue, Sep 13, 2016 at 5:32 PM, Alexander Korotkov
 wrote:
> On Fri, Apr 8, 2016 at 10:09 PM, Peter Geoghegan  wrote:
>>
>> On Wed, Mar 30, 2016 at 8:02 AM, Alexander Korotkov
>>  wrote:
>> > Hmm... I'm not completely agree with that. In typical usage partial sort
>> > should definitely use quicksort.  However, fallback to other sort
>> > methods is
>> > very useful.  Decision of partial sort usage is made by planner.  But
>> > planner makes mistakes.  For example, our HashAggregate is purely
>> > in-memory.
>> > In the case of planner mistake it causes OOM.  I met such situation in
>> > production and not once.  This is why I'd like partial sort to have
>> > graceful
>> > degradation for such cases.
>>
>> I think that this should be moved to the next CF, unless a committer
>> wants to pick it up today.
>
>
> Patch was rebased to current master.

Applies on HEAD at e8bdee27 and passes make-check, now I am seeing
zero documentation so it is a bit hard to see what this patch is
achieving without reading the thread.

$ git diff master --check
src/backend/optimizer/prep/prepunion.c:967: trailing whitespace.
+   cost_sort(&sorted_p, root, NIL, 0,
src/backend/utils/sort/tuplesort.c:1244: trailing whitespace.
+ * tuplesort_updatemax

+ * Returns true if the plan node isautomatically materializes its output
Typo here.

Still, this has received to reviews, so moved to next CF.
-- 
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] amcheck (B-Tree integrity checking tool)

2016-10-02 Thread Michael Paquier
On Thu, Sep 8, 2016 at 3:44 AM, Peter Geoghegan  wrote:
> On Fri, Sep 2, 2016 at 11:19 AM, Kevin Grittner  wrote:
>> IMV the process is to post a patch to this list to certify that it
>> is yours to contribute and free of IP encumbrances that would
>> prevent us from using it.  I will wait for that post.
>
> I attach my V3. There are only very minor code changes here, such as
> breaking out a separate elevel constant for DEBUG2 traces that show
> information of how the B-Tree is accessed (e.g. current level, block
> number). Tiny tweaks to about 3 comments were also performed. These
> changes are trivial.
>
> I've also removed the tests added, since external sort regression test
> coverage is in a much better place now.

Okay, moved to next CF. I may look at it finally I got some use-cases
for it, similar to yours I guess..
-- 
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] multivariate statistics (v19)

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 8:10 PM, Heikki Linnakangas  wrote:
> This patch set is in pretty good shape, the only problem is that it's so big
> that no-one seems to have the time or courage to do the final touches and
> commit it.

Did you see my suggestions about simplifying its SQL structure? You
could shave some code without impacting the base set of features.

> I fear that using "statistics" as the name of the new object might get a bit
> awkward. "statistics" is a plural, but we use it as the name of a single
> object, like "pants" or "scissors". Not sure I have any better ideas though.
> "estimator"? "statistics collection"? Or perhaps it should be singular,
> "statistic". I note that you actually called the system table
> "pg_mv_statistic", in singular.
>
> I'm not a big fan of storing the stats as just a bytea blob, and having to
> use special functions to interpret it. By looking at the patch, it's not
> clear to me what we actually store for functional dependencies. A list of
> attribute numbers? Could we store them simply as an int[]? (I'm not a big
> fan of the hack in pg_statistic, that allows storing arrays of any data type
> in the same column, though. But for functional dependencies, I don't think
> we need that.)

I am marking this patch as returned with feedback for now.

> Overall, this is going to be a great feature!

+1.
-- 
Michael


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


Re: [HACKERS] PATCH: two slab-like memory allocators

2016-10-02 Thread Michael Paquier
On Sun, Oct 2, 2016 at 10:15 AM, Tomas Vondra
 wrote:
> One more comment about GenSlab, particularly about unpredictability of the
> repalloc() behavior. It works for "large" chunks allocated in the AllocSet
> part, and mostly does not work for "small" chunks allocated in the
> SlabContext. Moreover, the chunkSize changes over time, so for two chunks of
> the same size, repalloc() may work on one of them and fail on the other,
> depending on time of allocation.
>
> With SlabContext it's perfectly predictable - repalloc() call fails unless
> the chunk size is exactly the same as before (which is perhaps a bit
> pointless, but if we decide to fail even in this case it'll work 100% time).
>
> But with GenSlabContext it's unclear whether the call fails or not.
>
> I don't like this unpredictability - I'd much rather have consistent
> failures (making sure people don't do repalloc() on with GenSlab). But I
> don't see a nice way to achieve that - the repalloc() call does not go
> through GenSlabRealloc() at all, but directly to SlabRealloc() or
> AllocSetRealloc().
>
> The best solution I can think of is adding an alternate version of
> AllocSetMethods, pointing to a different AllocSetReset implementation.

You guys are still playing with this patch, so moved to next CF with
"waiting on author".
-- 
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] GiST support for UUIDs

2016-10-02 Thread Michael Paquier
On Wed, Aug 19, 2015 at 1:25 PM, Paul A Jungwirth
 wrote:
> This is my first patch, so my apologies if anything is missing. I went
> the guidelines and I think I have everything covered. :-)

I am moving this patch to next CF, removing Julien Rouhaud and Teodor
Sigaev as reviewers because they have not showed up once on this
thread for reviews.
-- 
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_upgrade documentation improvement patch

2016-10-02 Thread Michael Paquier
On Tue, Apr 19, 2016 at 3:42 AM, Yuri Niyazov  wrote:
> Should I update the documentation patch to instruct the use of
> pg_controldata exclusively?

I  guess so. Marked as returned with feedback because the thread has died.
-- 
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] "Re: Question about grant create on database and pg_dump/pg_dumpall

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 12:29 AM, Tom Lane  wrote:
> The fundamental thing we have to do in order to move forward on this is
> to rethink what's the division of labor between pg_dump and pg_dumpall.
> I find the patch as presented quite unacceptable because it's made no
> effort to do that (or even to touch the documentation).

Patch marked as returned with feedback for now.

> 1. pg_dump without --create continues to do what it does today, ie it just
> dumps objects within the database, assuming that database-level properties
> will already be set correctly for the target database.
>
> 2. pg_dump with --create creates the target database and also sets all
> database-level properties (ownership, ACLs, ALTER DATABASE SET, etc etc).
>
> 3. pg_dumpall loses all code relating to individual-database creation
> and property setting and instead relies on pg_dump --create to do that.
> This would leave only the code relating to "pg_dumpall -g" (ie, dump roles
> and tablespaces) within pg_dumpall itself.

I saw a couple of people willing to have that on those mailling lists
over the years... So +1 for the idea.

> One thing that would still be messy is that presumably "pg_dumpall -g"
> would issue ALTER ROLE SET commands, but it's unclear what to do with
> ALTER ROLE IN DATABASE SET commands.  Should those become part of
> "pg_dump --create"'s charter?  It seems like not, but I'm not certain.

I guess that we need to think about simplifying the restore flow if we
have the occasion:
1) First restore the result of pg_dumpall -g
2) Restore the state of each database's dump --create
pg_dump --create assigns the created database to its rightful owner,
so it assumes that the role has been already created. If we do not
include those commands in pg_dump --create we make the whole restore
scenario more complicated.

> Another thing that requires some thought is that pg_dumpall is currently
> willing to dump ACLs and other properties for template1/template0, though
> it does not invoke pg_dump on them.  If we wanted to preserve that
> behavior while still moving the code that does those things to pg_dump,
> pg_dump would have to grow an option that would let it do that.  But
> I'm not sure how much of that behavior is actually sensible.

An extra option looks like the way to go. There are people that
willingly set up ACLs and we could just document that something like
pg_dump --template needs to be run to generate the template's dump
values.

> This would probably take a pg_dump archive version bump, since I think
> we don't currently record enough information for --create to do this
> (and we can't just cram the extra commands into the DATABASE entry,
> since we don't know whether --create will be specified to pg_restore).
> But we've done those before.

Yep.

(By the way, last time I saw dump/restore integration into an internal
system we have finished by using pg_dumpall -g and then pg_dump on a
given DB for simplicity)
-- 
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] [COMMITTERS] pgsql: Copy-editing for contrib/pg_visibility documentation.

2016-10-02 Thread Thomas Munro
On Mon, Oct 3, 2016 at 1:36 PM, Robert Haas  wrote:
> On Sat, Oct 1, 2016 at 3:33 PM, Tom Lane  wrote:
>> Copy-editing for contrib/pg_visibility documentation.
>>
>> Add omitted names for some function parameters.
>> Fix some minor grammatical issues.
>
> Why do you keep insisting on changing case where I've written "which"
> to instead say "that" in situations where AFAIK either is perfectly
> correct?  I find such changes at best neutral, and in some cases
> worse.

'Which' looks OK to me too here, but I speak some kind of British
English, and it looks like at least some writers of formal American
English prefer 'that' here:

* 
http://www.chicagomanualofstyle.org/qanda/data/faq/topics/Whichvs.That.html?old=Whichvs.That01.html
* https://en.oxforddictionaries.com/usage/that-or-which

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


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


Re: [HACKERS] [PATCH] add option to pg_dumpall to exclude tables from the dump

2016-10-02 Thread Michael Paquier
On Thu, Sep 29, 2016 at 2:16 AM, Robert Haas  wrote:
> On Tue, Sep 6, 2016 at 9:37 PM, Gerdan Rezende dos Santos
>  wrote:
>> After review, I realized that there is a call to the function:
>> doShellQuoting (pgdumpopts, OPTARG), which no longer seems to exist ...
>> After understand the code, I saw that the call is appendShellString
>> (pgdumpopts, OPTARG).
>>
>> Follow the patches already with the necessary corrections.
>
> This doesn't seem to take into account the discussion between Tom Lane
> and Jim Nasby about how this feature should work.

So, Juergen, it would be nice if you could participate in the
discussion and get a consensus on the patch. Until then, I am marking
this patch as returned with feedback.
-- 
Michael


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


Re: [HACKERS] Patch: Implement failover on libpq connect level.

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 5:44 PM, Victor Wagner  wrote:
> But backward compatibility is more
> important thing, so I now assume, that user tries to connect just one
> node, and this node is read only, user knows what he is doing.

Moved this patch to next CF.

(Note that I am in CF-vacuuming mode this morning, I'll try to close it asap.)
-- 
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] Forbid use of LF and CR characters in database and role names

2016-10-02 Thread Michael Paquier
On Sun, Oct 2, 2016 at 10:47 PM, Michael Paquier
 wrote:
> And seeing nothing happening here, I still don't know what to do with
> this patch. Thoughts? If we are going to do nothing I would suggest to
> just remove the comment in string_utils.c saying that such LF and CR
> will be unsupported in a future release and move on.

And so I am just marking this patch as returned with feedback.
-- 
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] [COMMITTERS] pgsql: Copy-editing for contrib/pg_visibility documentation.

2016-10-02 Thread Robert Haas
On Sat, Oct 1, 2016 at 3:33 PM, Tom Lane  wrote:
> Copy-editing for contrib/pg_visibility documentation.
>
> Add omitted names for some function parameters.
> Fix some minor grammatical issues.

Why do you keep insisting on changing case where I've written "which"
to instead say "that" in situations where AFAIK either is perfectly
correct?  I find such changes at best neutral, and in some cases
worse.

-- 
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] pg_upgade vs config

2016-10-02 Thread Tom Lane
I wrote:
> It occurs to me that a back-patchable workaround for this would be to
> make get_loadable_libraries sort the library names in order by length
> (and I guess we might as well sort same-length names alphabetically).
> This would for example guarantee that hstore_plpython is probed after
> both hstore and plpython.  Admittedly, this is a kluge of the first
> water.  But I see no prospect of back-patching any real fix, and it
> would definitely be better if pg_upgrade didn't fail on these modules.

I've tested the attached and verified that it allows pg_upgrade'ing
of the hstore_plpython regression DB --- or, if I reverse the sort
order, that it reproducibly fails.  I propose back-patching this
at least as far as 9.5, where the transform modules came in.  It might
be a good idea to go all the way back, just so that the behavior is
predictable.

regards, tom lane

diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index b432b54..5435bff 100644
*** a/src/bin/pg_upgrade/function.c
--- b/src/bin/pg_upgrade/function.c
***
*** 15,20 
--- 15,44 
  
  
  /*
+  * qsort comparator for pointers to library names
+  *
+  * We sort first by name length, then alphabetically for names of the same
+  * length.  This is to ensure that, eg, "hstore_plpython" sorts after both
+  * "hstore" and "plpython"; otherwise transform modules will probably fail
+  * their LOAD tests.  (The backend ought to cope with that consideration,
+  * but it doesn't yet, and even when it does it'd be a good idea to have
+  * a predictable order of probing here.)
+  */
+ static int
+ library_name_compare(const void *p1, const void *p2)
+ {
+ 	const char *str1 = *(const char *const *) p1;
+ 	const char *str2 = *(const char *const *) p2;
+ 	int			slen1 = strlen(str1);
+ 	int			slen2 = strlen(str2);
+ 
+ 	if (slen1 != slen2)
+ 		return slen1 - slen2;
+ 	return strcmp(str1, str2);
+ }
+ 
+ 
+ /*
   * get_loadable_libraries()
   *
   *	Fetch the names of all old libraries containing C-language functions.
*** get_loadable_libraries(void)
*** 38,47 
  		PGconn	   *conn = connectToServer(&old_cluster, active_db->db_name);
  
  		/*
! 		 * Fetch all libraries referenced in this DB.  We can't exclude the
! 		 * "pg_catalog" schema because, while such functions are not
! 		 * explicitly dumped by pg_dump, they do reference implicit objects
! 		 * that pg_dump does dump, e.g. CREATE LANGUAGE plperl.
  		 */
  		ress[dbnum] = executeQueryOrDie(conn,
  		"SELECT DISTINCT probin "
--- 62,68 
  		PGconn	   *conn = connectToServer(&old_cluster, active_db->db_name);
  
  		/*
! 		 * Fetch all libraries referenced in this DB.
  		 */
  		ress[dbnum] = executeQueryOrDie(conn,
  		"SELECT DISTINCT probin "
*** get_loadable_libraries(void)
*** 69,76 
  
  			res = executeQueryOrDie(conn,
  	"SELECT 1 "
! 		   "FROM	pg_catalog.pg_proc JOIN pg_namespace "
! 			 "		ON pronamespace = pg_namespace.oid "
  			   "WHERE proname = 'plpython_call_handler' AND "
  	"nspname = 'public' AND "
  	"prolang = 13 /* C */ AND "
--- 90,98 
  
  			res = executeQueryOrDie(conn,
  	"SELECT 1 "
! 	"FROM pg_catalog.pg_proc p "
! 	"JOIN pg_catalog.pg_namespace n "
! 	"ON pronamespace = n.oid "
  			   "WHERE proname = 'plpython_call_handler' AND "
  	"nspname = 'public' AND "
  	"prolang = 13 /* C */ AND "
*** get_loadable_libraries(void)
*** 112,124 
  	if (found_public_plpython_handler)
  		pg_fatal("Remove the problem functions from the old cluster to continue.\n");
  
- 	/* Allocate what's certainly enough space */
- 	os_info.libraries = (char **) pg_malloc(totaltups * sizeof(char *));
- 
  	/*
! 	 * Now remove duplicates across DBs.  This is pretty inefficient code, but
! 	 * there probably aren't enough entries to matter.
  	 */
  	totaltups = 0;
  
  	for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
--- 134,151 
  	if (found_public_plpython_handler)
  		pg_fatal("Remove the problem functions from the old cluster to continue.\n");
  
  	/*
! 	 * Now we want to remove duplicates across DBs and sort the library names
! 	 * into order.  This avoids multiple probes of the same library, and
! 	 * ensures that libraries are probed in a consistent order, which is
! 	 * important for reproducible behavior if one library depends on another.
! 	 *
! 	 * First transfer all the names into one array, then sort, then remove
! 	 * duplicates.  Note: we strdup each name in the first loop so that we can
! 	 * safely clear the PGresults in the same loop.  This is a bit wasteful
! 	 * but it's unlikely there are enough names to matter.
  	 */
+ 	os_info.libraries = (char **) pg_malloc(totaltups * sizeof(char *));
  	totaltups = 0;
  
  	for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
*** get_loadable_libraries(void)
*** 131,157 

Re: [HACKERS] pg_upgade vs config

2016-10-02 Thread Tom Lane
Andres Freund  writes:
> On 2016-10-02 17:59:47 -0400, Tom Lane wrote:
>> So now I'm thinking you're right, it'd be better to have some solution
>> whereby dfmgr.c knows about cross-module dependencies and loads the
>> dependencies first.  Not sure how to approach that.  The extension
>> "requires" mechanism is tantalizingly close to providing the data
>> we need, but dfmgr.c doesn't know about that, and there's no concept
>> of a reverse mapping from .so names to extensions anyway.

> One, kind of extreme, way to get there would be to resolve the hstore
> symbols hstore_plpython needs with load_external_function, during
> _PG_init().  Most of these files don't actually depend on a large number
> of symbols, so that should actually be doable.

Hm.  That would actually not be a bad idea, perhaps, because the method
we're using right now requires that the linker not bitch about unresolved
symbols at build time, which is a really bad thing that I'd prefer to
turn off.

It's still not a very back-patchable answer, but it's something that
we could get to in HEAD without a huge amount of work.

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_upgade vs config

2016-10-02 Thread Andres Freund
On 2016-10-02 17:59:47 -0400, Tom Lane wrote:
> I've found that the Linux '-l:hstore.so' solution works on HPUX as well,
> at least to the extent of being able to run LOAD.  However, it doesn't
> seem to be possible to make it work on macOS, which has a hard distinction
> between "loadable modules" and "shared libraries".  Our extensions are
> the former, which means that e.g. hstore.so is not a shlib and the linker
> will refuse to consider it as a linkable library.  I tried converting
> them to true shlibs, which is just a matter of changing "-bundle" to
> "-dynamiclib" in the link command and dropping "-bundle_loader postgres"
> because that's not accepted with "-dynamiclib".  That works about 90%,
> but I found that plpython failed its regression test for a very scary
> reason: there's a "hash_search()" somewhere in libc on this platform
> and the linker was preferentially resolving plpython's call to that
> rather than to the one in Postgres.  I don't think we want to risk
> that sort of platform dependency :-(

ISTM that that's a risk independent of this specific issue? On other
platforms, I mean?


> So now I'm thinking you're right, it'd be better to have some solution
> whereby dfmgr.c knows about cross-module dependencies and loads the
> dependencies first.  Not sure how to approach that.  The extension
> "requires" mechanism is tantalizingly close to providing the data
> we need, but dfmgr.c doesn't know about that, and there's no concept
> of a reverse mapping from .so names to extensions anyway.

One, kind of extreme, way to get there would be to resolve the hstore
symbols hstore_plpython needs with load_external_function, during
_PG_init().  Most of these files don't actually depend on a large number
of symbols, so that should actually be doable.

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] pg_upgade vs config

2016-10-02 Thread Tom Lane
I wrote:
> So it doesn't seem that we've broken anything since 9.5 --- it didn't
> work before either.  The seeming successes may have been due to chance,
> i.e. pg_upgrade probing the libraries in an order that happened to work.
> I see no evidence that get_loadable_libraries/check_loadable_libraries
> are paying any attention to what order the libraries are checked in.

It occurs to me that a back-patchable workaround for this would be to
make get_loadable_libraries sort the library names in order by length
(and I guess we might as well sort same-length names alphabetically).
This would for example guarantee that hstore_plpython is probed after
both hstore and plpython.  Admittedly, this is a kluge of the first
water.  But I see no prospect of back-patching any real fix, and it
would definitely be better if pg_upgrade didn't fail on these modules.

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_upgade vs config

2016-10-02 Thread Tom Lane
Andrew Dunstan  writes:
> On 10/02/2016 12:54 PM, Tom Lane wrote:
>> [ digs more deeply ... ]  Oh, weird: it looks like this succeeded in
>> every case except 9.6->HEAD upgrade.  Did we break something recently?

> Yeah, my latest version of the test module (soon to hit githyb) also 
> does a self upgrade, and these modules pass that on 9.5, whereas they 
> fail on 9.6, as well as the 9.6->HEAD and HEAD self-tests failing. So 
> indeed it looks like we've broken something.

I've now checked that simply doing LOAD 'hstore_plpython2.so' in a fresh
session fails, in both HEAD and 9.5, on both Linux and current macOS.
So it doesn't seem that we've broken anything since 9.5 --- it didn't
work before either.  The seeming successes may have been due to chance,
i.e. pg_upgrade probing the libraries in an order that happened to work.
I see no evidence that get_loadable_libraries/check_loadable_libraries
are paying any attention to what order the libraries are checked in.

I've found that the Linux '-l:hstore.so' solution works on HPUX as well,
at least to the extent of being able to run LOAD.  However, it doesn't
seem to be possible to make it work on macOS, which has a hard distinction
between "loadable modules" and "shared libraries".  Our extensions are
the former, which means that e.g. hstore.so is not a shlib and the linker
will refuse to consider it as a linkable library.  I tried converting
them to true shlibs, which is just a matter of changing "-bundle" to
"-dynamiclib" in the link command and dropping "-bundle_loader postgres"
because that's not accepted with "-dynamiclib".  That works about 90%,
but I found that plpython failed its regression test for a very scary
reason: there's a "hash_search()" somewhere in libc on this platform
and the linker was preferentially resolving plpython's call to that
rather than to the one in Postgres.  I don't think we want to risk
that sort of platform dependency :-(

On further reflection, though, this approach is probably a dead end
even without any platform concerns.  The reason is that if we do
'LOAD foo' and the dynamic linker then pulls in 'bar.so', bar.so
will be present in the address space all right, but we will not have
checked for a compatible magic block, nor have called its _PG_init
function if any.  In general, if calls into a module occur without
having called its _PG_init, it might misbehave very badly.

So now I'm thinking you're right, it'd be better to have some solution
whereby dfmgr.c knows about cross-module dependencies and loads the
dependencies first.  Not sure how to approach that.  The extension
"requires" mechanism is tantalizingly close to providing the data
we need, but dfmgr.c doesn't know about that, and there's no concept
of a reverse mapping from .so names to extensions anyway.

regards, tom lane


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


[HACKERS] Non-empty default log_line_prefix

2016-10-02 Thread Christoph Berg
Re: Tom Lane 2016-09-29 <18642.1475159...@sss.pgh.pa.us>
> > Possibly the longer version could be added as an example in the
> > documentation.
> 
> I suspect that simply having a nonempty default in the first place
> is going to do more to raise peoples' awareness than anything we
> could do in the documentation.  But perhaps an example along these
> lines would be useful for showing proper use of %q.

Patch attached. (Still using %t, I don't think %m makes sense for the
default.)

Christoph
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index e826c19..a4d8b74
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** local0.*/var/log/postgresql
*** 5004,5010 
   value will pad on the left. Padding can be useful to aid human
   readability in log files.
   This parameter can only be set in the postgresql.conf
!  file or on the server command line. The default is an empty string.
  
   

--- 5004,5011 
   value will pad on the left. Padding can be useful to aid human
   readability in log files.
   This parameter can only be set in the postgresql.conf
!  file or on the server command line. The default is
!  %t [%p]  which logs a time stamp and the process ID.
  
   

*** FROM pg_stat_activity;
*** 5142,5147 
--- 5143,5159 
   include those escapes if you are logging to syslog.
  
 
+ 
+
+ 
+  The %q escape is useful when including information that 
is
+  only available in session (backend) context like user or database
+  name. An example would be:
+ 
+ log_line_prefix = '%t [%p] %q%u@%d/%a '
+ 
+ 
+

   
  
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index cced814..b71fa93
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** static struct config_string ConfigureNam
*** 3014,3020 
gettext_noop("If blank, no prefix is used.")
},
&Log_line_prefix,
!   "",
NULL, NULL, NULL
},
  
--- 3014,3020 
gettext_noop("If blank, no prefix is used.")
},
&Log_line_prefix,
!   "%t [%p] ",
NULL, NULL, NULL
},
  
diff --git a/src/backend/utils/misc/postgresql.conf.sample 
b/src/backend/utils/misc/postgresql.conf.sample
new file mode 100644
index 05b1373..9eaa23e
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***
*** 430,436 
  #log_duration = off
  #log_error_verbosity = default# terse, default, or verbose 
messages
  #log_hostname = off
! #log_line_prefix = '' # special values:
#   %a = application name
#   %u = user name
#   %d = database name
--- 430,436 
  #log_duration = off
  #log_error_verbosity = default# terse, default, or verbose 
messages
  #log_hostname = off
! #log_line_prefix = '%t [%p] ' # special values:
#   %a = application name
#   %u = user name
#   %d = database name

-- 
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_upgade vs config

2016-10-02 Thread Andrew Dunstan



On 10/02/2016 12:54 PM, Tom Lane wrote:

Andrew Dunstan  writes:

The biggest issue is this: the upgrade fails completely on
ltree-plpython and hstore-plpython, presumably because these modules
rely on the plpython module being loaded first. pg_upgrade rather
simple-mindedly calls LOAD on the object library to test if it's usable.

FWIW, that seems to have worked fine yesterday on prairiedog.

I suspect the explanation is that macOS's dynamic linker is smart enough
to pull in plpython when one of those modules is LOAD'ed.  The ideal fix
would be to make that happen on all platforms.  I'm not actually sure
why it doesn't already; surely every dynamic linker in existence has
such a capability.

[ digs more deeply ... ]  Oh, weird: it looks like this succeeded in
every case except 9.6->HEAD upgrade.  Did we break something recently?



Yeah, my latest version of the test module (soon to hit githyb) also 
does a self upgrade, and these modules pass that on 9.5, whereas they 
fail on 9.6, as well as the 9.6->HEAD and HEAD self-tests failing. So 
indeed it looks like we've broken something. Yet another example of why 
I need to get this test module production ready :-)


cheers

andrew



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


Re: [HACKERS] pg_upgade vs config

2016-10-02 Thread Tom Lane
Andrew Dunstan  writes:
> On 10/02/2016 01:53 PM, Tom Lane wrote:
>> Because pg_dump with --binary-upgrade neglects to emit
>> ALTER EXTENSION bloom ADD ACCESS METHOD bloom;
>> which it would need to do in order to make this work right.  The other
>> small problem is that there is no such ALTER EXTENSION syntax in the
>> backend.  This is a rather major oversight in the patch that added DDL
>> support for access methods, if you ask me.

> I agree.

Remarkably enough, it seems that only a gram.y production need be added
--- the only other code involved is objectaddress.c, which does seem
to have gotten extended sufficiently.

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_upgade vs config

2016-10-02 Thread Andrew Dunstan



On 10/02/2016 01:53 PM, Tom Lane wrote:

So then why are the pre-upgrade and post-upgrade dumps different?

Because pg_dump with --binary-upgrade neglects to emit

ALTER EXTENSION bloom ADD ACCESS METHOD bloom;


That's what I suspected.



which it would need to do in order to make this work right.  The other
small problem is that there is no such ALTER EXTENSION syntax in the
backend.  This is a rather major oversight in the patch that added DDL
support for access methods, if you ask me.



I agree.

cheers

andrew



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


Re: [HACKERS] pg_upgade vs config

2016-10-02 Thread Tom Lane
Andrew Dunstan  writes:
> On 10/02/2016 09:50 AM, Michael Paquier wrote:
>> On Sun, Oct 2, 2016 at 10:40 PM, Andrew Dunstan  wrote:
>>> It looks like we have some work to do to teach pg_dump about handling access
>>> methods in extensions. This doesn't look quite as bad as the first issue,
>>> but it's a pity 9.6 escaped into the wild with this issue.

>> 562f06f3 has addressed this issue 3 months ago, and there is a test in
>> src/test/modules/test_pg_dump.

> So then why are the pre-upgrade and post-upgrade dumps different?

Because pg_dump with --binary-upgrade neglects to emit

ALTER EXTENSION bloom ADD ACCESS METHOD bloom;

which it would need to do in order to make this work right.  The other
small problem is that there is no such ALTER EXTENSION syntax in the
backend.  This is a rather major oversight in the patch that added DDL
support for access methods, if you ask me.

(Also, I didn't look at what test_pg_dump is testing, but I bet it
isn't attempting to cover --binary-upgrade behavior.)

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_upgade vs config

2016-10-02 Thread Tom Lane
I wrote:
> I suspect the explanation is that macOS's dynamic linker is smart enough
> to pull in plpython when one of those modules is LOAD'ed.  The ideal fix
> would be to make that happen on all platforms.  I'm not actually sure
> why it doesn't already; surely every dynamic linker in existence has
> such a capability.

Some experimentation says that this is indeed possible on Linux, at least.
It's a bit of a pain because the .so's we need to reference are not named
"libsomething.so" and thus a straight -l switch doesn't work.  The Linux
ld(1) man page documents that you can write "-l:filename" to override the
addition of "lib", but I have no idea how portable that is to other
toolchains.  (On macOS, and maybe other BSD-derived systems, it looks
like you can do this without the colon, ie -lhstore.so will work.)

Also, it seems that 
-L../hstore -l:hstore$(DLSUFFIX)
which was my first attempt, doesn't work because you end up with a
hard-coded reference to "../hstore/hstore.so", which rpath searching
doesn't cope with.  I was able to make it work by copying hstore.so
and plpython2.so into contrib/hstore_plpython and then just writing

SHLIB_LINK += -L. -l:hstore$(DLSUFFIX) \
-l:plpython$(python_majorversion)$(DLSUFFIX) $(python_libspec)

That results in undecorated references that do work with the rpath.

This all seems depressingly platform-specific, but maybe we can make
it work on enough platforms to be satisfactory.

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_upgade vs config

2016-10-02 Thread Tom Lane
Andrew Dunstan  writes:
> The biggest issue is this: the upgrade fails completely on 
> ltree-plpython and hstore-plpython, presumably because these modules 
> rely on the plpython module being loaded first. pg_upgrade rather 
> simple-mindedly calls LOAD on the object library to test if it's usable. 

FWIW, that seems to have worked fine yesterday on prairiedog.

I suspect the explanation is that macOS's dynamic linker is smart enough
to pull in plpython when one of those modules is LOAD'ed.  The ideal fix
would be to make that happen on all platforms.  I'm not actually sure
why it doesn't already; surely every dynamic linker in existence has
such a capability.

[ digs more deeply ... ]  Oh, weird: it looks like this succeeded in
every case except 9.6->HEAD upgrade.  Did we break something recently?

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] Hash Indexes

2016-10-02 Thread Pavel Stehule
2016-10-02 12:40 GMT+02:00 Michael Paquier :

> On Sun, Oct 2, 2016 at 3:31 AM, Greg Stark  wrote:
> > On Fri, Sep 30, 2016 at 2:11 AM, Robert Haas 
> wrote:
> >> For one thing, we can stop shipping a totally broken feature in release
> after release
> >
> > For what it's worth I'm for any patch that can accomplish that.
> >
> > In retrospect I think we should have done the hash-over-btree thing
> > ten years ago but we didn't and if Amit's patch makes hash indexes
> > recoverable today then go for it.
>
> +1.
>

+1

Pavel


> --
> 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_upgade vs config

2016-10-02 Thread Andrew Dunstan



On 10/02/2016 09:50 AM, Michael Paquier wrote:

On Sun, Oct 2, 2016 at 10:40 PM, Andrew Dunstan  wrote:

It looks like we have some work to do to teach pg_dump about handling access
methods in extensions. This doesn't look quite as bad as the first issue,
but it's a pity 9.6 escaped into the wild with this issue.

562f06f3 has addressed this issue 3 months ago, and there is a test in
src/test/modules/test_pg_dump.



So then why are the pre-upgrade and post-upgrade dumps different?

cheers

andrew


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


Re: [HACKERS] Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

2016-10-02 Thread Michael Paquier
On Thu, Sep 22, 2016 at 7:48 PM, Rushabh Lathia
 wrote:
> I performed basic test with patch,
>
> a) patch get applied cleanly on latest source,
> b) able to build documentation cleanly.
>
> Marking this as ready for committer.

Oops, incorrect patch... I am moving it to next CF. Discussion can
continue there.
-- 
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] Minor improvement to fdwhandler.sgml

2016-10-02 Thread Michael Paquier
On Wed, Jan 27, 2016 at 7:52 PM, Etsuro Fujita
 wrote:
> Here is a small patch to do s/for/For/ to two section titles in
> fdwhandlers.sgml, for consistency.

I am grepping 54 places where "for" is used in a , and none of
them use an upper case for its first letter. I am marking this patch
as rejected.
-- 
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] Quorum commit for multiple synchronous replication.

2016-10-02 Thread Michael Paquier
On Wed, Sep 28, 2016 at 5:14 PM, Michael Paquier
 wrote:
> OK, so I have done a review of this patch keeping that in mind as
> that's the consensus. I am still getting familiar with the code...

Returned with feedback for now. This just needs polishing so feel free
to move it to the next CF once you have a new patch.
-- 
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] Showing parallel status in \df+

2016-10-02 Thread Michael Paquier
On Sun, Oct 2, 2016 at 10:55 PM, Michael Paquier
 wrote:
> Let's remove it and move on then. By looking again at this thread and
> particularly 
> https://www.postgresql.org/message-id/20160926190618.gh5...@tamriel.snowman.net
> (thanks Stephen for the summary) that's where we are heading to.

(Moved to next CF)
-- 
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] Showing parallel status in \df+

2016-10-02 Thread Michael Paquier
On Sat, Oct 1, 2016 at 9:47 AM, Tom Lane  wrote:
> Jim Nasby  writes:
>> On 9/28/16 2:59 PM, Alvaro Herrera wrote:
>>> My vote (which was not counted by Stephen) was to remove it from \df+
>>> altogether.  I stand by that.  People who are used to seeing the output
>>> in \df+ will wonder "where the heck did it go" and eventually figure it
>>> out, at which point it's no longer a problem.  We're not breaking
>>> anyone's scripts, that's for sure.
>>>
>>> If we're not removing it, I +0 support the option of moving it to
>>> footers.  I'm -1 on doing nothing.
>
>> I agree with everything Alvaro just said.
>
> Well, alternatively, can we get a consensus for doing that?  People
> did speak against removing PL source code from \df+ altogether, but
> maybe they're willing to reconsider if the alternative is doing nothing.
>
> Personally I'm on the edge of washing my hands of the whole thing...

Let's remove it and move on then. By looking again at this thread and
particularly 
https://www.postgresql.org/message-id/20160926190618.gh5...@tamriel.snowman.net
(thanks Stephen for the summary) that's where we are heading to.
-- 
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_upgade vs config

2016-10-02 Thread Michael Paquier
On Sun, Oct 2, 2016 at 10:40 PM, Andrew Dunstan  wrote:
> It looks like we have some work to do to teach pg_dump about handling access
> methods in extensions. This doesn't look quite as bad as the first issue,
> but it's a pity 9.6 escaped into the wild with this issue.

562f06f3 has addressed this issue 3 months ago, and there is a test in
src/test/modules/test_pg_dump.
-- 
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] Forbid use of LF and CR characters in database and role names

2016-10-02 Thread Michael Paquier
On Mon, Sep 12, 2016 at 11:38 AM, Michael Paquier
 wrote:
> On Mon, Sep 12, 2016 at 10:01 AM, Noah Misch  wrote:
>> I discourage documenting LF/CR restrictions.  For the epsilon of readers who
>> would benefit from this knowledge, the error message suffices.  For everyone
>> else, it would just dilute the text.  (One could argue against other parts of
>> our documentation on this basis, but I'm not calling for such a study.  I'm
>> just saying that today's lack of documentation on this topic is optimal.)
>
> Okay.
>
>>> > > I think the way forward here, if any, is to work on removing these
>>> > > restrictions, not to keep sprinkling them around.
>>> >
>>> > If we were talking about pathnames containing spaces, I would agree,
>>> > but I've never heard of a legitimate pathname containing CR or LF.  I
>>> > can't see us losing much by refusing to allow such pathnames, except
>>> > for security holes.
>>
>> (In other words, +1 to that.)
>
> Yep. To be honest, I see value in preventing directly the use of
> newlines and carriage returns in database and role names to avoid
> users to be bitten by custom scripts, things for example written in
> bash that scan manually for pg_database, pg_roles, etc. And I have
> seen such things over the years. Now it is true that the safeguards in
> core are proving to be enough, if only the in-core tools are used, but
> that's not necessarily the case with all the things gravitating around
> this community.

And seeing nothing happening here, I still don't know what to do with
this patch. Thoughts? If we are going to do nothing I would suggest to
just remove the comment in string_utils.c saying that such LF and CR
will be unsupported in a future release and move on.
-- 
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] pgbench - allow to store select results into variables

2016-10-02 Thread Michael Paquier
On Tue, Sep 27, 2016 at 10:41 AM, Amit Langote
 wrote:
> On 2016/09/26 20:27, Fabien COELHO wrote:
>>
>> Hello Amit,
>>
 I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as I
 have no further comments at the moment.
>>>
>>> Wait... Heikki's latest commit now requires this patch to be rebased.
>>
>> Indeed. Here is the rebased version, which still get through my various
>> tests.
>
> Thanks, Fabien.  It seems to work here too.

Moved to next CF with same status, ready for committer.
-- 
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] pgbench more operators & functions

2016-10-02 Thread Michael Paquier
On Sat, Oct 1, 2016 at 11:35 PM, Fabien COELHO  wrote:
> Attached version changes:
>  - removes C operators not present in psql
>  - document operators one per line

Moved to next CF with same status: "Ready for committer".
-- 
Michael


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


[HACKERS] pg_upgade vs config

2016-10-02 Thread Andrew Dunstan


I'm working on updating and making production ready my experimental 
cross version pg_upgrade testing module for the buildfarm. A couple of 
things have emerged that are of concern. This module does a much more 
complete test that our normal test for pg_upgrade, which only checks 
upgrading the standard regression database. This tests all the databases 
the buildfarm creates for testing, including those made by modules in 
contrib.


The biggest issue is this: the upgrade fails completely on 
ltree-plpython and hstore-plpython, presumably because these modules 
rely on the plpython module being loaded first. pg_upgrade rather 
simple-mindedly calls LOAD on the object library to test if it's usable. 
It's a bit embarrassing that we can't upgrade a database using one of 
our own modules. At the very least we should hard-code a way around this 
(e.g. have it load the relevant plpython module first), but more 
generally I think we need a way to tell pg_upgrade that module X relies 
on module Y. In the past ISTR we've said we don't support having 
dependencies between loadable modules, but that ship now seems to have 
sailed.


Second, we get an unexpected difference between the pre-upgrade and 
post-upgrade dumps for the bloom module:


   --- /home/bf/bfr/root/upgrade/HEAD/origin-REL9_6_STABLE.sql
   2016-10-02 09:16:03.298341639 -0400
   +++
   /home/bf/bfr/root/upgrade/HEAD/converted-REL9_6_STABLE-to-HEAD.sql
   2016-10-02 09:16:54.889343991 -0400
   @@ -7413,6 +7413,20 @@
 COMMENT ON EXTENSION bloom IS 'bloom access method - signature
   file based index';


   +--
   +-- Name: bloom; Type: ACCESS METHOD; Schema: -; Owner:
   +--
   +
   +CREATE ACCESS METHOD bloom TYPE INDEX HANDLER public.blhandler;
   +
   +
   +--
   +-- Name: ACCESS METHOD bloom; Type: COMMENT; Schema: -; Owner:
   +--
   +
   +COMMENT ON ACCESS METHOD bloom IS 'bloom index access method';
   +
   +
 SET search_path = public, pg_catalog;

 SET default_tablespace = '';



It looks like we have some work to do to teach pg_dump about handling 
access methods in extensions. This doesn't look quite as bad as the 
first issue, but it's a pity 9.6 escaped into the wild with this issue.


cheers

andrew



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


Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 10:12 PM, Daniel Verite  wrote:
> Tomas Vondra wrote:
>
>> A few minor comments regarding the patch:
>>
>> 1) CopyStartSend seems pretty pointless - It only has one function call
>> in it, and is called on exactly one place (and all other places simply
>> call allowLongStringInfo directly). I'd get rid of this function and
>> replace the call in CopyOneRowTo(() with allowLongStringInfo().
>>
>> 2) allowlong seems awkward, allowLong or allow_long would be better
>>
>> 3) Why does resetStringInfo reset the allowLong flag? Are there any
>> cases when we want/need to forget the flag value? I don't think so, so
>> let's simply not reset it and get rid of the allowLongStringInfo()
>> calls. Maybe it'd be better to invent a new makeLongStringInfo() method
>> instead, which would make it clear that the flag value is permanent.
>>
>> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log
>> message, but with '%d' and not '%ld' (as needed after changing the type
>> to Size).
>>
>> 5) The comment at allowLongStringInfo talks about allocLongStringInfo
>> (i.e. wrong function name).
>
> Here's an updated patch. Compared to the previous version:
>
> - removed CopyStartSend (per comment #1 in review)
>
> - renamed flag to allow_long (comment #2)
>
> - resetStringInfo no longer resets the flag (comment #3).
>
> - allowLongStringInfo() is removed (comment #3 and #5),
> makeLongStringInfo() and initLongStringInfo() are provided
> instead, as alternatives to makeStringInfo() and initStringInfo().
>
> - StringInfoData.len is back to int type, 2GB max.
> (addresses comment #4 incidentally).
> This is safer because  many routines peeking
> into StringInfoData use int for offsets into the buffer,
> for instance most of the stuff in backend/libpq/pqformat.c
> Altough these routines are not supposed to be called on long
> buffers, this assertion was not enforced in the code, and
> with a 64-bit length effectively over 2GB, they would misbehave
> pretty badly.

The patch status is "Waiting on Author" in the CF app, but a new patch
has been sent 2 days ago, so this entry has been moved to next CF with
"Needs Review".
-- 
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] [BUG] pg_basebackup from disconnected standby fails

2016-10-02 Thread Michael Paquier
> So, if I understand correctly, then we can mark the version posted by
> you upthread [1] which includes a test along with Kyotaro's fix can be
> marked as Ready for committer.  If so, then please change the status
> of patch accordingly.

Patch moved to next CF 2016-11, still with status "ready for committer".

IMO, this thread has reached as conclusion to use this patch to
address the problem reported:
cab7npqtv5gmkqcndofgtgqoqxz2xlz4rrw247oqojzztvy6...@mail.gmail.com
-- 
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] pgsql: Add putenv support for msvcrt from Visual Studio 2013

2016-10-02 Thread Michael Paquier
On Tue, Sep 6, 2016 at 9:36 PM, Michael Paquier
 wrote:
> OK, let's get to the next step of the game and get a committer to look
> at this patch.

Moved to next CF. It would be good to get a committer on this one. We
have come on a conclusion on what to do. Actually, 0001 can be just
HEAD-only per the lack of complaints.
-- 
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] postgres_fdw : altering foreign table not invalidating prepare statement execution plan.

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 1:09 AM, Ashutosh Bapat
 wrote:
>>
>>> The attached patch adds the
>>> dependencies from create_foreignscan_plan() which is called for any
>>> foreign access. I have also added testcases to test the functionality.
>>> Let me know your comments on the patch.
>>
>>
>> Hmm.  I'm not sure that that's a good idea.
>>
>> I was thinking the changes to setrefs.c proposed by Amit to collect that
>> dependencies would be probably OK, but I wasn't sure that it's a good idea
>> that he used PlanCacheFuncCallback as the syscache inval callback function
>> for the foreign object caches because it invalidates not only generic plans
>> but query trees, as you mentioned downthread.  So, I was thinking to modify
>> his patch so that we add a new syscache inval callback function for the
>> caches that is much like PlanCacheFuncCallback but only invalidates generic
>> plans.
>
> PlanCacheFuncCallback() invalidates the query tree only when
> invalItems are added to the plan source. The patch adds the
> dependencies in root->glob->invalItems, which standard_planner()
> copies into PlannedStmt::invalItems. This is then copied into the
> gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the
> query tree. I have verified this under the debugger. Am I missing
> something?

Moved to next CF. Feel free to continue the work on this item.
-- 
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] WAL logging problem in 9.4.3?

2016-10-02 Thread Michael Paquier
On Thu, Sep 29, 2016 at 10:02 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Thu, 29 Sep 2016 16:59:55 +0900, Michael Paquier 
>  wrote in 
> 
>> On Mon, Sep 26, 2016 at 5:03 PM, Kyotaro HORIGUCHI
>>  wrote:
>> > Hello, I return to this before my things:)
>> >
>> > Though I haven't played with the patch yet..
>>
>> Be sure to run the test cases in the patch or base your tests on them then!
>
> All items of 006_truncate_opt fail on ed0b228 and they are fixed
> with the patch.
>
>> > Though I don't know how it actually impacts the perfomance, it
>> > seems to me that we can live with truncated_to and sync_above in
>> > RelationData and BufferNeedsWAL(rel, buf) instead of
>> > HeapNeedsWAL(rel, buf).  Anyway up to one entry for one relation
>> > seems to exist at once in the hash.
>>
>> TBH, I still think that the design of this patch as proposed is pretty
>> cool and easy to follow.
>
> It is clean from certain viewpoint but additional hash,
> especially hash-searching on every HeapNeedsWAL seems to me to be
> unacceptable. Do you see it accetable?
>
>
> The attached patch is quiiiccck-and-dirty-hack of Michael's patch
> just as a PoC of my proposal quoted above. This also passes the
> 006 test.  The major changes are the following.
>
> - Moved sync_above and truncted_to into  RelationData.
>
> - Cleaning up is done in AtEOXact_cleanup instead of explicit
>   calling to smgrDoPendingSyncs().
>
> * BufferNeedsWAL (replace of HeapNeedsWAL) no longer requires
>   hash_search. It just refers to the additional members in the
>   given Relation.
>
> X I feel that I have dropped one of the features of the origitnal
>   patch during the hack, but I don't recall it clearly now:(
>
> X I haven't consider relfilenode replacement, which didn't matter
>   for the original patch. (but there's few places to consider).
>
> What do you think about this?

I have moved this patch to next CF. (I still need to look at your patch.)
-- 
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] [GENERAL] pg_upgrade from 9.5 to 9.6 fails with "invalid argument"

2016-10-02 Thread Andrew Dunstan



On 10/01/2016 02:01 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 09/30/2016 12:24 PM, Tom Lane wrote:

Seems to be some additional prep work needed somewhere ...
No upgrade_install_root at 
/home/bfarm/bf-scripts/build-farm-4.17/PGBuild/Modules/TestUpgradeXversion.pm 
line 51.

Oh sorry, you also need an entry for that in the config file. Mine has:
 upgrade_install_root => '/home/bf/bfr/root/upgrade',

Yesterday's runs of prairiedog had this turned on.  I'm not sure how
to interpret the output (because there isn't any, ahem), but it does
not appear that the test detected anything wrong.



I'm working on collecting the log files and making the module more 
useful. But you can see pretty much all the logs (lots of them!) after a 
run inside the upgrade directories.


cheers

andrew




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


Re: [HACKERS] Hash Indexes

2016-10-02 Thread Michael Paquier
On Sun, Oct 2, 2016 at 3:31 AM, Greg Stark  wrote:
> On Fri, Sep 30, 2016 at 2:11 AM, Robert Haas  wrote:
>> For one thing, we can stop shipping a totally broken feature in release 
>> after release
>
> For what it's worth I'm for any patch that can accomplish that.
>
> In retrospect I think we should have done the hash-over-btree thing
> ten years ago but we didn't and if Amit's patch makes hash indexes
> recoverable today then go for it.

+1.
-- 
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] On conflict update & hint bits

2016-10-02 Thread Peter Geoghegan
On Sat, Oct 1, 2016 at 1:15 PM, Peter Geoghegan  wrote:
> It also looks like the DO NOTHING variant is similarly affected, even
> when the isolation level is READ COMMITTED.:-(

Actually, the DO NOTHING variant is also only affected in isolation
levels greater than READ COMMITTED. Not sure why I thought otherwise.


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