[HACKERS] 2016-03 Commitfest Manager

[David Steele]

I volunteered a while back to be the CFM and I haven't seen any other
volunteers or objections to my offer.

I am still ready, eager, and willing!

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Addition of extra commit fest entry to park future patches

[David Steele]

On 3/1/16 3:28 PM, Magnus Hagander wrote:
> On Tue, Mar 1, 2016 at 10:12 AM, Alvaro Herrera
> Magnus Hagander wrote:
>
> > Yeah, we can do that. I'd suggest we either name it based on the current
> > tentative date for CF1 (september), or name it specificaly "9.7-first" 
> or
> > something like that rather than just plain "future", to make it more 
> clear.
> 
> +1 to both names suggested by Magnus.
> 
> 
> 
> We do need to pick one of them :)

I'm good with 9.7-first.  I presume it can be renamed later to fit the
standard scheme?

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Addition of extra commit fest entry to park future patches

[David Steele]

On 3/1/16 3:35 PM, Kevin Grittner wrote:
> On Tue, Mar 1, 2016 at 2:28 PM, Magnus Hagander  wrote:
>> On Tue, Mar 1, 2016 at 10:12 AM, Alvaro Herrera  
>> wrote:
>>> Magnus Hagander wrote:
> 
 I'd suggest we either name it based on the current tentative
 date for CF1 (september), or name it specificaly "9.7-first" or
 something like that rather than just plain "future", to make it
 more clear.
>>>
>>> +1 to both names suggested by Magnus.
>>
>> We do need to pick one of them :)
>>
>> Anybody else with preferences?
> 
> I would prefer to see a consistent namimg pattern (i.e., 2016-09)
> and rename it if we reschedule.

I'm fine with that - it does help set expectations.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] 2016-03 Commitfest Manager

[David Steele]

On 3/1/16 3:04 PM, Tom Lane wrote:
> David Steele  writes:
>> I volunteered a while back to be the CFM and I haven't seen any other
>> volunteers or objections to my offer.
> 
>> I am still ready, eager, and willing!
> 
> I haven't heard any other volunteers either.  You have the conn, sir.

Aye aye!  Once the 2016-03 CF has been closed I'll send out a kickoff
report.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] 2016-03 Commitfest Manager

[David Steele]

On 3/1/16 8:49 PM, Michael Paquier wrote:
> On Wed, Mar 2, 2016 at 7:22 AM, Michael Paquier
>  wrote:
>> On Wed, Mar 2, 2016 at 6:15 AM, David Steele  wrote:
>>> On 3/1/16 3:04 PM, Tom Lane wrote:
>>>> David Steele  writes:
>>>>> I volunteered a while back to be the CFM and I haven't seen any other
>>>>> volunteers or objections to my offer.
>>>>
>>>>> I am still ready, eager, and willing!
>>>>
>>>> I haven't heard any other volunteers either.  You have the conn, sir.
>>>
>>> Aye aye!  Once the 2016-03 CF has been closed I'll send out a kickoff
>>> report.
>>
>> Magnus, should David have administration rights in the CF app? I
>> believe that he cannot change the CF status now. We should switch to
>> "In Progress". I can do that at least.
> 
> I am planning to switch 2016-03 from "Open" to "Processing" in a
> couple of hours, at which stage nobody will be able to register new
> patches to it. I guess it's more than time.

Agreed.  I see you created the new CF so no reason to keep it open.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

[HACKERS] 2016-03 Commitfest

[David Steele]

The 2016-03 commitfest is officially in progress!

There are currently a lot of patches waiting for review but with no
reviewers:

Needs review: 97
Needs *reviewer*: 58

Please check the "needs reviewer" list
(https://commitfest.postgresql.org/9/?reviewer=-2) for patches to
review.  The committers need our help to work through this enormous load
of patches.

If this is you:

Waiting on Author: 16

Then please post an updated patch as soon as possible so it can be
reviewed.  Some of these patches have not seen any activity from the
author in a long time.

The good news is we are already 14% done with the CF:

Committed: 17
Rejected: 2
Returned with Feedback: 1

I'll post a status update on this thread at least once a week and more
often as needed.  Going forward there will be more detail on individual
patches that are not making progress for whatever reason.

Let's get reviewing!

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] More stable query plans via more predictable column statistics

[David Steele]

On 3/2/16 11:10 AM, Shulgin, Oleksandr wrote:
> On Wed, Feb 24, 2016 at 12:30 AM, Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> I think it'd be useful not to have all the changes in one lump, but
> structure this as a patch series with related changes in separate
> chunks. I doubt we'd like to mix the changes in a single commit, and
> it makes the reviews and reasoning easier. So those NULL-handling
> fixes should be in one patch, the MCV patches in another one.
> 
> 
> OK, such a split would make sense to me.  Though, I'm a bit late as the
> commitfest is already closed to new patches, I guess asking the CF
> manager to split this might work (assuming I produce the patch files)?

If the patch is broken into two files that gives the review/committer
more options but I don't think it requires another CF entry.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Re: Add generate_series(date, date) and generate_series(date, date, integer)

[David Steele]

On 2/21/16 2:24 PM, Vik Fearing wrote:
> On 02/21/2016 07:56 PM, Corey Huinker wrote:
>>
>> Other than that, the only difference is the ::date part. Is it
>> really worth adding extra code just for that? I would say not.
>>
>>
>> I would argue it belongs for the sake of completeness. 
> 
> So would I.
> 
> +1 for adding this missing function.

+1. FWIW, a sample query I wrote for a customer yesterday would have
looked nicer with this function. Here's how the generate_series looked:

generate_series('2016-03-01'::date, '2016-03-31'::date, interval '1
day')::date

But it would have been cleaner to write:

generate_series('2016-03-01'::date, '2016-03-31'::date)

More importantly, though, I don't like that the timestamp version of the
function happily takes date parameters but returns timestamps. I feel
this could lead to some subtle bugs for the unwary.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] (pgaudit) Audit log is not output after the SET ROLE.

[David Steele]

On 3/7/16 4:39 AM, Toshi Harada wrote:
> 
> I am testing the pgaudit(https://commitfest.postgresql.org/9/463/).
> (use "http://www.postgresql.org/message-id/56b0101b.6070...@pgmasters.net"; 
> attached patch on 9.6-devel)
> 
> I found strange thing.
> 
> - After SET ROLE, part of the SQL is not the audit log output.
> - SQL comprising a relation is not output to the audit log.
> 
> * Reproduce:
> ** prepare
> 
> createuser test_user -U postgres
> createdb test -U postgres -O test_user
> psql test -U test_user -c "CREATE TABLE team(id int, name text)"
> 
> ** pgaudit settings
> 
> shared_preload_libraries = 'pgaudit'
> pgaudit.log = 'all'

Both of these are in postgresql.conf?

> ** test sql script (test.sql)
> 
> SELECT 1;
> SELECT * FROM team; -- output audit log
> SET ROLE test_user;
> SELECT 2;
> SELECT * FROM team; -- no output audit log
> SELECT 3;
> RESET ROLE;
> SELECT * FROM team; -- output audit log
> 
> ** run script
> 
> psql test -U postgres -f test.sql
> 
> ** audit log
> 
> LOG:  AUDIT: SESSION,1,1,READ,SELECT,,,SELECT 1;,
> LOG:  AUDIT: SESSION,2,1,READ,SELECT,,,SELECT * FROM team;,
> LOG:  AUDIT: SESSION,3,1,MISC,SET,,,SET ROLE test_user;,
> LOG:  AUDIT: SESSION,4,1,READ,SELECT,,,SELECT 2;,
> LOG:  AUDIT: SESSION,5,1,READ,SELECT,,,SELECT 3;,
> LOG:  AUDIT: SESSION,6,1,MISC,RESET,,,RESET ROLE;,
> LOG:  AUDIT: SESSION,7,1,READ,SELECT,,,SELECT * FROM team;,

Well, that definitely doesn't look right.

You may have noticed that the pgaudit patch is marked as "returned with
feedback" so it is closed for the current commitfest and will not be
included in 9.6.

I'll definitely look at this bug but I would ask that you resubmit it at
https://github.com/pgaudit/pgaudit/issues so we can continue the
conversation there.

Thanks!
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[David Steele]

Hi Robbie,

On 3/8/16 5:44 PM, Robbie Harwood wrote:
> Hello friends,
> 
> Here's yet another version of GSSAPI encryption support.  It's also
> available for viewing on my github:

I got this warning when applying the first patch in the set:

../other/v6-0001-Move-common-GSSAPI-code-into-its-own-files.patch:245:
new blank line at EOF.
+
warning: 1 line adds whitespace errors.

I know it's minor but I'm always happier when patches apply cleanly.

The build went fine but when testing I was unable to logon at all.  I'm
using the same methodology as in
http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except
that I'm running against 51c0f63 and using the v6 patch set.

psql simply hangs and never returns.  I have attached a pcap of the
psql/postgres session generated with:

tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap

If you would like me to capture more information please let me know
specifically how you would like me to capture it.

I reverted to v5 and got the same behavior I was seeing with v4 and v5,
namely that I can only logon occasionally and usually get this error:

psql: expected authentication request from server, but received

Using a fresh build from 51c0f63 I can logon reliably every time so I
don't think there's an issue in my environment.

-- 
-David
da...@pgmasters.net


gssapi.pcap
Description: Binary data


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;

[David Steele]

On 12/23/15 9:15 PM, Michael Paquier wrote:

On Mon, Dec 7, 2015 at 9:14 PM, Michael Paquier
 wrote:

On Wed, Sep 2, 2015 at 6:19 AM, Marko Tiikkaja  wrote:

Hopefully nobody minds if I slip this to the commit fest that started
today?  The attached patch should address all the comments from the 9.5
cycle.

All the previous comments have been addressed. Perhaps some regression tests
would have some value?

This thread has been stalling for a couple of weeks now. I have marked
it as "returned with feedback". Marko, if you are still working on
this patch, could you add some regression tests and then move it to
the next CF?
This was submitted to the 2016-03 CF but no new patch was supplied and 
there's been no activity on the thread for months.  I'm marking it as 
"returned with feedback."


Marko, if can address Michael's concerns or supply a new patch I'll be 
happy to open the CF entry again.


Thanks,

--
-David
da...@pgmasters.net



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

[David Steele]

Hi Victor,

On 2/1/16 5:05 PM, Alvaro Herrera wrote:

Victor Wagner wrote:

On Fri, 22 Jan 2016 16:36:15 -0300
Alvaro Herrera  wrote:


You're editing the expected file for the libpq-regress thingy, but you
haven't added any new lines to test the new capability.  I think it'd
be good to add some there.  (I already said this earlier in the
thread; is there any reason you ignored it the first time?)

I seriously doubt that this program can be used to test new
capabilities.

All it does, it calls PQconninfoParse and than examines some fields of
PGconn structure.

Ah, you're right, I didn't remember that.


If I add some new uris, than only thing I can test is that comma is
properly copied from the URI to this field. And may be that some syntax
errors are properly detected.

Yeah, we should do that.


So, I think that new functionality need other approach for testing.
There should be test of real connection to real temporary cluster.
Probably, based on Perl TAP framework which is actively used in the
Postgres recently.

Yes, agreed.  So please have a look at that one and share your opinion
about it.  It'd be useful.

Meanwhile I'm moving the patch to the next commitfest.


There hasn't been any movement on this patch in a while.  Will you have 
a new tests ready for review soon?


Thanks,

--
-David
da...@pgmasters.net

Re: [HACKERS] Improving replay of XLOG_BTREE_VACUUM records

[David Steele]

On 1/8/16 9:34 AM, Alvaro Herrera wrote:

Simon Riggs wrote:

On 8 January 2016 at 13:36, Alvaro Herrera  wrote:

I would agree except for the observation on toast indexes.  I think
that's an important enough use case that perhaps we should have both.

The exclusion of toast indexes is something we can remove also, I have
recently discovered. When we access toast data we ignore MVCC, but we still
have the toast pointer and chunkid to use for rechecking our scan results.
So a later patch will add some rechecks.

Ah, interesting, glad to hear.  I take it you're pushing your patch
soon, then?


ISTM that this patch should be "returned with feedback" or "rejected" 
based on the thread.  I'm marking it "waiting for author" for the time 
being.


Thanks,

--
-David
da...@pgmasters.net



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

Re: [HACKERS] statistics for array types

[David Steele]

On 1/18/16 11:24 AM, Alvaro Herrera wrote:

Alexander Korotkov wrote:


The patch implementing my idea above is attached.

What's the status here?  Jeff, did you have a look at Alexander's
version of your patch?  Tomas, does this patch satisfy your concerns?


This was marked as "needs review" but I have changed it to "waiting for 
author".


Thanks,
-David


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

Re: [HACKERS] Identifying a message in emit_log_hook.

[David Steele]

Hi Simon,

On 3/10/16 7:26 AM, Simon Riggs wrote:


Can you add this to the CF? It was submitted before deadline.
I presume you have access to do that?


No problem - here it is:

https://commitfest.postgresql.org/9/576/

--
-David
da...@pgmasters.net


--
Sent 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] Client Log Output Filtering

[David Steele]

On 3/9/16 7:37 PM, Petr Jelinek wrote:

On 03/02/16 05:02, Robert Haas wrote:

On Mon, Feb 1, 2016 at 7:24 PM, David Steele  wrote:


I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3).  I'll also
move it to the next CF.


I don't see any reason not to accept this.



Yes, the idea seems sane.

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.


I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false.  Am I missing something?

--
-David
da...@pgmasters.net


--
Sent 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] Client Log Output Filtering

[David Steele]

On 3/10/16 11:00 AM, Petr Jelinek wrote:

> The comment above errhidefromclient says "Only log levels below ERROR
> can be hidden from the client." but use of the errhidefromclient(true)
> actually does hide the error message from client, client just gets
> failed query without any message when used with ERROR level.

Right you are.  The v3 patch adds this check.

I also added the documentation to sources.sgml that Tom pointed out was
missing.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index fcb3e40..4ad15d0 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -353,6 +353,14 @@ ereport(ERROR,
  includes the current statement already.
 

+   
+
+ errhidefromclient(bool hide_from_client) can be
+ called to suppress message output to the client when the error severity
+ level is lower than ERROR.  It is useful for hiding sensitive
+ messages from the client while still logging them on the server.
+
+   
   

 
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5b7554b..5e5035d 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1094,6 +1094,22 @@ errhidecontext(bool hide_ctx)
return 0;   /* return value does 
not matter */
 }
 
+/*
+ * errhidefromclient --- optionally suppress output of message to client
+ * if error severity level < ERROR.
+ */
+int
+errhidefromclient(bool hide_from_client)
+{
+   ErrorData  *edata = &errordata[errordata_stack_depth];
+
+   /* we don't bother incrementing recursion_depth */
+   CHECK_STACK_DEPTH();
+
+   edata->hide_from_client = hide_from_client && edata->elevel < ERROR;
+
+   return 0;   /* return value does 
not matter */
+}
 
 /*
  * errfunction --- add reporting function name to the current error
@@ -1477,7 +1493,7 @@ EmitErrorReport(void)
send_message_to_server_log(edata);
 
/* Send to client, if enabled */
-   if (edata->output_to_client)
+   if (edata->output_to_client && !edata->hide_from_client)
send_message_to_frontend(edata);
 
MemoryContextSwitchTo(oldcontext);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 7d338dd..05dfe2b 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -179,6 +179,7 @@ extern int  errcontext_msg(const char *fmt,...) 
pg_attribute_printf(1, 2);
 
 extern int errhidestmt(bool hide_stmt);
 extern int errhidecontext(bool hide_ctx);
+extern int errhidefromclient(bool hide_from_client);
 
 extern int errfunction(const char *funcname);
 extern int errposition(int cursorpos);
@@ -343,6 +344,7 @@ typedef struct ErrorData
boolshow_funcname;  /* true to force funcname inclusion */
boolhide_stmt;  /* true to prevent STATEMENT: 
inclusion */
boolhide_ctx;   /* true to prevent CONTEXT: 
inclusion */
+   boolhide_from_client;   /* true to prevent client 
output */
const char *filename;   /* __FILE__ of ereport() call */
int lineno; /* __LINE__ of 
ereport() call */
const char *funcname;   /* __func__ of ereport() call */


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/10/16 11:07 AM, Tom Lane wrote:
> Petr Jelinek  writes:
>> The comment above errhidefromclient says "Only log levels below ERROR 
>> can be hidden from the client." but use of the errhidefromclient(true) 
>> actually does hide the error message from client, client just gets 
>> failed query without any message when used with ERROR level.
> 
> Um.  That seems pretty broken --- I think it's a violation of the wire
> protocol spec.
> 
> I notice though that we allow client_min_messages to be set to FATAL,
> which would be a different way of violating the protocol.  Maybe we
> should reduce the max setting of that to ERROR?

This was the same conclusion I came to for the log_level setting in pgaudit.

I'll submit a proposal to hackers after 9.6 to make this change.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/10/16 9:51 AM, Tom Lane wrote:

> The patch is evidently modeled on errhidestmt and errhidectx, which are
> making the same assumption for their fields.
> 
> I wonder whether, instead of continuing to proliferate random bool fields
> in struct ErrorData, we oughta replace them all with an "int flags" field.
> That's probably material for a separate patch though.

There are currently six boolean flags (if you include my new one) that
all relate to visibility in one way or another.  Combining them into a
"flags" field makes sense to me.

I'll submit a proposal to hackers after 9.6 to make this change.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Proposal: BSD Authentication support

[David Steele]

On 1/14/16 11:22 PM, Robert Haas wrote:
> On Tue, Jan 12, 2016 at 2:27 AM, Marisa Emerson  wrote:
>> I've attached the latest version of this patch. I've fixed up an issue with
>> the configuration scripts that I missed.
> Looks reasonable on a quick read-through.  Can anyone with access to a
> BSD system review and test?

Is anyone with access to/experience with BSD able to review and test
this patch?  Seems like it would make a great addition to 9.6.

Thanks,

-- 
-David
da...@pgmasters.net




signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Inconsistent error handling in START_REPLICATION command

[David Steele]

On 1/21/16 9:53 AM, Shulgin, Oleksandr wrote:
> On Thu, Jan 21, 2016 at 3:25 PM, Robert Haas  > wrote:
>
>
> So it's true that the client can't unilaterally terminate COPY BOTH
> mode; it can only send CopyDone.  But an error on the server side
> should do so.
>
>
> Hm, you're right.  Even though the server sends COPY_BOTH message
> before the plugin startup, an attempt by the client to actually read
> the data messages results in ErrorResponse handled on the client, then
> the client can re-submit the corrected START_REPLICATION command and
> continue without the need to reconnect.  This cannot be actually
> tested in psql, but I can verify the behavior with my python scripts.
>
> Then I don't have a preference for the early error reporting in this
> case.  If the current behavior potentially allows for more flexible
> error reporting, I'm for keeping it.

It looks like a decision needs to be made here whether to apply this
patch, send it back to the author, or reject it so I'm marking it "Ready
for Committer".

Robert, since you were participating in this conversation can you have a
look?

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Combining Aggregates

[David Steele]

On 1/20/16 11:42 PM, David Rowley wrote:
> On 21 January 2016 at 08:06, Robert Haas  > wrote:
> 
> On Wed, Jan 20, 2016 at 7:38 AM, David Rowley
> mailto:david.row...@2ndquadrant.com>>
> wrote:
> > Agreed. So I've attached a version of the patch which does not have any 
> of
> > the serialise/deserialise stuff in it.
> 
> I re-reviewed this and have committed most of it with only minor
> kibitizing.  A few notes:
> 
> 
> I've attached the re-based remainder, which includes the serial/deserial
> again.
> 
> I'll submit this part to March 'fest, where hopefully we'll also have
> something to utilise it.

As far as I can tell this is the most recent version of the patch but it
doesn't apply on master.  I'm guessing that's mostly because of Tom's
UPP patch.

This is a long and confusing thread and I should know since I just read
through the entire thing.  I'm setting this back to "waiting for author"
and I'd like see a rebased patch and a summary of what's in the patch
before setting it back to "needs review".

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] eXtensible Transaction Manager API (v2)

[David Steele]

On 2/10/16 12:50 PM, Konstantin Knizhnik wrote:

> PostgresProffesional cluster teams wants to propose new version of
> eXtensible Transaction Manager API.
> Previous discussion concerning this patch can be found here:
> 
> http://www.postgresql.org/message-id/f2766b97-555d-424f-b29f-e0ca0f6d1...@postgrespro.ru

I see a lot of discussion on this thread but little in the way of consensus.

> The API patch itself is small enough, but we think that it will be
> strange to provide just API without examples of its usage.

It's not all that small, though it does apply cleanly even after a few
months.  At least that indicates there is not a lot of churn in this area.

I'm concerned about the lack of response or reviewers for this patch.
It may be because everyone believes they had their say on the original
thread, or because it seems like a big change to go into the last CF, or
for other reasons altogether.

I think you should try to make it clear why this patch would be a win
for 9.6.

Is anyone willing to volunteer a review or make an argument for the
importance of this patch?

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Inconsistent error handling in START_REPLICATION command

[David Steele]

On 3/11/16 1:11 PM, Tom Lane wrote:
> Robert Haas  writes:
>> On Fri, Mar 11, 2016 at 11:36 AM, David Steele  wrote:
>>> It looks like a decision needs to be made here whether to apply this patch,
>>> send it back to the author, or reject it so I'm marking it "Ready for
>>> Committer".
>>>
>>> Robert, since you were participating in this conversation can you have a
>>> look?
> 
>> Who, me?  *looks around*
>>
>> OK, I had a look.  I don't think this is a bug.
> 
> I agree, and Craig's complaint that this change reduces flexibility
> for plugins seems to tilt the balance against it.  I see that Alex
> himself accepted that argument in his last message in the thread
> ().
>
> So let's mark it rejected and move on.

+1 and done.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] remove wal_level archive

[David Steele]

On 2/8/16 2:34 PM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 1/26/16 10:56 AM, Simon Riggs wrote:
>>> Removing one of "archive" or "hot standby" will just cause confusion and
>>> breakage, so neither is a good choice for removal.
>>>
>>> What we should do is 
>>> 1. Map "archive" and "hot_standby" to one level with a new name that
>>> indicates that it can be used for both/either backup or replication.
>>>   (My suggested name for the new level is "replica"...)
>>> 2. Deprecate "archive" and "hot_standby" so that those will be removed
>>> in a later release.
>>
>> Updated patch to reflect these suggestions.
> 
> I wonder if the "keep one / keep both" argument is running in circles as
> new reviewers arrive at the thread.  Perhaps somebody could read the
> whole thread(s) and figure out a way to find consensus so that we move
> forward on this.

There was a lot of argument upstream about whether to keep 'hot_standby'
or 'archive' but after the proposal to change it to 'replica' came up
everybody seemed to fall in line with that.

+1 from me for using 'replica' as the WAL level to replace 'hot_standby'
and 'archive'.

+1 from me for removing the 'hot_standby' and 'archive' options entirely
in 9.6 rather than deprecating.

Unless anyone has objections I would like to mark this 'ready for
committer'.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] eXtensible Transaction Manager API (v2)

[David Steele]

On 3/11/16 1:30 PM, Robert Haas wrote:

> There's been a lot of discussion on another thread about this patch.
> The subject is "The plan for FDW-based sharding", but the thread kind
> of got partially hijacked by this issue.  The net-net of that is that
> I don't think we have a clear enough idea about where we're going with
> global transaction management to make it a good idea to adopt an API
> like this.  For example, if we later decide we want to put the
> functionality in core, will we keep the hooks around for the sake of
> alternative non-core implementations?  I just don't believe this
> technology is nearly mature enough to commit to at this point.

Ah yes, I forgot about the related discussion on that thread.  Pasting
here for reference:

http://www.postgresql.org/message-id/20160223164335.ga11...@momjian.us

> Konstantin does not agree with my assessment, perhaps unsurprisingly.

I'm certainly no stranger to feeling strongly about a patch!

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[David Steele]

Hi Filip,

On 2/20/16 8:00 AM, Filip Rembiałkowski wrote:
> On Fri, Feb 19, 2016 at 10:09 PM, Catalin Iacob  On 2/9/16, Tom Lane mailto:t...@sss.pgh.pa.us>>
> wrote:
> > FWIW, I think it would be a good thing if the NOTIFY statement syntax 
> were
> > not remarkably different from the syntax used in the pg_notify() 
> function
> > call.  To do otherwise would certainly be confusing.  So on the whole
> > I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.
> 
> Filip, do you agree with Tom's proposal? Do you plan to rework the
> patch on these lines? If you are I'll try to review it, if not I could
> give it a shot as I'm interested in having this in 9.6.
> 
> I see that Tom's remarks give more flexibility, and your refinement
> makes sense.

It looks like we are waiting on a new patch from you before this can be
reviewed.  Are you close to having that done?

Meanwhile, I have marked it "Waiting on Author".

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] eXtensible Transaction Manager API (v2)

[David Steele]

On 3/11/16 2:00 PM, Oleg Bartunov wrote:
> On Fri, Mar 11, 2016 at 7:11 PM, David Steele  I'm concerned about the lack of response or reviewers for this patch.
> It may be because everyone believes they had their say on the original
> thread, or because it seems like a big change to go into the last CF, or
> for other reasons altogether.
> 
> 
> We'll prepare easy setup to play with our solutions, so any developers
> could see how it works.  Hope this weekend we'll post something about this.

OK, then for now I'm marking this "waiting for author."  You can switch
it back to "needs review" once you have posted additional material.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] insufficient qualification of some objects in dump files

[David Steele]

Hi Peter,

On 2/26/16 1:30 AM, Tom Lane wrote:
> Michael Paquier  writes:
>> On Fri, Feb 26, 2016 at 9:47 AM, Peter Eisentraut  wrote:
>>> Tom thought this might require an archive version dump, but I'm not
>>> sure.  The tags are more of an informational string for human
>>> consumption, not strictly part of the archive format.
> 
>> Hm, the TOC entry, with its tag changed, is part of the dump, and this
>> is written in the archive, but the shape of TocEntry does not change
>> so this is really debatable.
> 
> I had in mind that we would add a separate field for tag's schema name to
> TocEntry, which surely would require an archive format number bump.
> As the patch is presented, I agree with Peter that it does not really
> need a format number bump.  The question that has to be answered is
> whether this solution is good enough?  You could not trust it for
> automated processing of tags --- it's easy to think of cases in which the
> schema/object name separation would be ambiguous.  So is the tag really
> "strictly for human consumption"?  I'm not sure about that.

It looks like there is still some discussion to be had here about
whether a "human readable" solution is enough.

Until that's resolved I've marked this patch "Waiting on Author".

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] 2016-03 Commitfest

[David Steele]

We're are now one third of the way through the 2016-03 Commitfest.

There are still some patches left that need review but have no reviewer
(https://commitfest.postgresql.org/9/?reviewer=-2) though a lot have
been picked up in the last week.

Needs review: 56
Needs *reviewer*: 15 (was 58 last week)

There were a number of patches that were marked as "needs review" but
were actually "waiting for author" as far as I could see.  If your patch
is in this state it would be good if you could give an idea when you
will be able to supply a new patch or otherwise address concerns raised
in the thread.

Waiting on Author: 29

The closed patches are up significantly since last week and the good
news is that most of them were committed.  36% of the patches are now
closed.

Committed: 46
Rejected: 4
Returned with Feedback: 4

I will continue to respond individually to threads that seem idle or
have issues that need to be resolved.  Please do not hesitate to contact
me about about any concerns that you might have regarding the Commitfest
process.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] remove wal_level archive

[David Steele]

On 3/11/16 1:29 PM, David Steele wrote:


Unless anyone has objections I would like to mark this 'ready for
committer'.


This patch is now ready for committer.

--
-David
da...@pgmasters.net


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

Re: [HACKERS] Batch update of indexes

[David Steele]

Hi Konstantin,

On 2/3/16 11:47 AM, Konstantin Knizhnik wrote:

Attached please find patch for "ALTER INDEX ... WHERE ..." clause.
It is now able to handle all three possible situations:
1. Making index partial (add WHERE condition to the ordinary index)
2. Extend partial index range (less restricted index predicate)
3. Arbitrary change of partial index predicate

In case 2) new records are added to the index.
In other two cases index is completely reconstructed.

This patch includes src/bin/insbench utility for testing insert
performance. It can be easily excluded from the patch to reduce it size.
Also it is better to apply this patch together with "index-only scans
with partial indexes" patch:


This patch no longer applies on master:

$ git apply ../other/alter-index.patch
../other/alter-index.patch:27: trailing whitespace.
static void
../other/alter-index.patch:62: indent with spaces.
SPIPlanPtr plan;
../other/alter-index.patch:63: indent with spaces.
Portal portal;
../other/alter-index.patch:90: trailing whitespace.

../other/alter-index.patch:99: trailing whitespace.

error: patch failed: src/test/regress/expected/aggregates.out:831
error: src/test/regress/expected/aggregates.out: patch does not apply

Please provide a new patch for review.  Meanwhile I am marking this 
"waiting on author".


Thanks,
--
-David
da...@pgmasters.net


--
Sent 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 backslash-continuations in custom scripts

[David Steele]

On 2/18/16 6:54 AM, Kyotaro HORIGUCHI wrote:


First, I rebased the previous patch set and merged three of
them. Now they are of three patches.


1. Making SQL parser part of psqlscan independent from psql.

Moved psql's baskslsh command stuff out of original psqlscan.l
and some psql stuff the parser directly looked are used via a
set of callback functions, which can be all NULL for usages
from other than psql.

2. Making pgbench to use the new psqlscan parser.

3. Changing the way to hold SQL/META commands from array to
 linked list.

 The #2 introduced linked list to store SQL multistatement but
 immediately the caller moves the elements into an array. This
 patch totally changes the way to linked list.


Any takers to review this updated patch?

--
-David
da...@pgmasters.net


--
Sent 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] speeding up GIN build with parallel workers

[David Steele]

On 2/18/16 10:10 AM, Constantin S. Pan wrote:

On Wed, 17 Feb 2016 23:01:47 +0300
Oleg Bartunov  wrote:


My feedback is (Mac OS X 10.11.3)

set gin_parallel_workers=2;
create index message_body_idx on messages using gin(body_tsvector);
LOG:  worker process: parallel worker for PID 5689 (PID 6906) was
terminated by signal 11: Segmentation fault


Fixed this, try the new patch. The bug was in incorrect handling
of some GIN categories.


Oleg, it looks like Constantin has updated to patch to address the issue 
you were seeing.  Do you have time to retest and review?


Thanks,
--
-David
da...@pgmasters.net


--
Sent 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] Effective storage of duplicates in B-tree index.

[David Steele]

Hi Anastasia,

On 2/18/16 12:29 PM, Anastasia Lubennikova wrote:

18.02.2016 20:18, Anastasia Lubennikova:

04.02.2016 20:16, Peter Geoghegan:

On Fri, Jan 29, 2016 at 8:50 AM, Anastasia Lubennikova
  wrote:

I fixed it in the new version (attached).


Thank you for the review.
At last, there is a new patch version 3.0. After some refactoring it
looks much better.
I described all details of the compression in this document
https://goo.gl/50O8Q0 (the same text without pictures is attached in
btc_readme_1.0.txt).
Consider it as a rough copy of readme. It contains some notes about
tricky moments of implementation and questions about future work.
Please don't hesitate to comment it.


Sorry, previous patch was dirty. Hotfix is attached.


This looks like an extremely valuable optimization for btree indexes but 
unfortunately it is not getting a lot of attention.  It still applies 
cleanly for anyone interested in reviewing.


It's not clear to me that you answered all of Peter's questions in [1]. 
 I understand that you've provided a README but it may not be clear if 
the answers are in there (and where).


Also, at the end of the README it says:

13. Xlog. TODO.

Does that mean the patch is not yet complete?

Thanks,
--
-David
da...@pgmasters.net

[1] 
http://www.postgresql.org/message-id/cam3swzq3_plqch4w7uq8q_f2t4hesektr2n0rq5pxa18oer...@mail.gmail.com



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

Re: [HACKERS] PATCH: use foreign keys to improve join estimates v1

[David Steele]

Hi Thomas,

On 2/24/16 11:21 AM, Tomas Vondra wrote:


Overall, I still believe the FK patch is a clear improvement of the
current status - while it's true it does not improve all possible cases
and there's a room for additional improvements (like handling multiple
candidate FK constraints), it should not make existing estimates worse.


The latest version of this patch does not apply:

$ git apply ../other/0001-estimation-with-fkeys-v2.patch
../other/0001-estimation-with-fkeys-v2.patch:748: trailing whitespace.

error: patch failed: src/backend/optimizer/util/plancat.c:27
error: src/backend/optimizer/util/plancat.c: patch does not apply
error: patch failed: src/include/nodes/relation.h:468
error: src/include/nodes/relation.h: patch does not apply

David's most recent version also does not apply:

$ git apply ../other/estimation-with-fkeys-v2_davidv4.patch
../other/estimation-with-fkeys-v2_davidv4.patch:517: trailing whitespace.

error: patch failed: src/backend/optimizer/util/plancat.c:27
error: src/backend/optimizer/util/plancat.c: patch does not apply
error: patch failed: src/include/nodes/relation.h:472
error: src/include/nodes/relation.h: patch does not apply

I don't think it would be clear to any reviewer which patch to apply 
even if they were working.  I'm marking this "waiting for author".


Thanks,
--
-David
da...@pgmasters.net


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

[HACKERS] Re: [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

[David Steele]

On 2/25/16 4:44 PM, Vitaly Burovoy wrote:


Added to the commitfest 2016-03.

[CF] https://commitfest.postgresql.org/9/540/


This looks like a fairly straight-forward bug fix (the size of the patch 
is deceptive because there a lot of new tests included).  It applies 
cleanly.


Anastasia, I see you have signed up to review.  Do you have an idea when 
you will get the chance to do that?


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] Batch update of indexes

[David Steele]

On 3/14/16 10:28 AM, Konstantin Knizhnik wrote:


Rebased patch is attached.


Thanks for the quick turnaround!

Marko, you are signed up to review this patch.  Do you have an idea of 
when you'll be able to do that?


--
-David
da...@pgmasters.net


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

On 2/23/16 2:17 AM, Michael Paquier wrote:


As a continuation of the thread firstly dedicated to SCRAM:
http://www.postgresql.org/message-id/55192afe.6080...@iki.fi
Here is a new thread aimed at gathering all the ideas of this previous
thread and aimed at clarifying a bit what has been discussed until now
regarding password protocols, verifiers, and SCRAM itself.


It looks like this patch set is a bit out of date.

When applying 0004:

$ git apply 
../other/0004-Remove-password-verifiers-for-unsupported-protocols-.patch

error: patch failed: src/bin/pg_upgrade/pg_upgrade.c:262
error: src/bin/pg_upgrade/pg_upgrade.c: patch does not apply

Then I tried to build with just 0001-0003:

cd /postgres/src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3318
3319
3320
3321
3322
make[3]: *** [postgres.bki] Error 1

Could you provide an updated set of patches for review?  Meanwhile I am 
marking this as "waiting for author".


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

[David Steele]

Hi Jeff,

On 2/25/16 5:00 PM, Jeff Janes wrote:


But, It doesn't sound like I am going to win that debate.  Given that,
I don't think we need a different name for the function. I'm fine with
explaining the word-boundary subtlety in the documentation, and
keeping the function name itself simple.


It's not clear to me if you are requesting more documentation here or 
stating that you are happy with it as-is.  Care to elaborate?


Other than that I think this patch looks to be ready for committer. Any 
objections?


--
-David
da...@pgmasters.net


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

[HACKERS] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

[David Steele]

On 2/24/16 12:40 AM, Michael Paquier wrote:


This has the merit to be clear, thanks for the input. Whatever the
approach taken at the end we have two candidates:
- Extend XLogInsert() with an extra argument for flags (Andres)
- Introduce XLogInsertExtended with this extra argument and let
XLogInsert() in peace (Robert and I).
Actually, I lied, there was still something I could do for this
thread: attached are two patches implementing both approaches as
respectively a-1 and a-2. Patch b is the set of logs I used for the
tests to show with a low checkpoint_timeout that checkpoints are
getting correctly skipped on an idle system.


Unfortunately neither A nor B apply anymore.

However, since the patches can still be read through I wonder if Robert 
or Andres would care to opine on whether A or B is better now that they 
can see the full implementation?


For my 2c I'm happy with XLogInsertExtended() since it seems to be a 
rare use case where flags are required.  This can always be refactored 
in the future if/when the use of flags spreads.


I think it would be good to make a decision on this before asking 
Michael to rebase.


Thanks,
--
-David
da...@pgmasters.net


--
Sent 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] we have added support for box type in SP-GiST index

[David Steele]

On 2/15/16 10:29 AM, Teodor Sigaev wrote:


It's very pity but author is not able to continue work on this patch,
and I would like to raise this flag.

I'd like to add some comments about patches:

traversalValue patch adds arbitrary value assoсiated with branch in
SP-GiST tree walk. Unlike to recostructedValue it could be just pointer,
it havn't to be a regular pgsql type. Also, it could be used independly
from reconstructedValue. This patch is used both following attached
patches.

range patch just switchs using reconstructedValue to traversalValue in
range opclasses. reconstructedValue was used just because it was an
acceptable workaround in case of range type. Range opclase stores a full
value in leafs and doesn't need to use reconstructedValue to return
tuple in index only scan.
See http://www.postgresql.org/message-id/5399.1343512...@sss.pgh.pa.us

q4d patch implements index over boxes using SP-GiST. Basic idea was an
observation, range opclass thinks about one-dimensional ranges as 2D
points.
Following this idea, we can think that 2D box (what is 2 points or 4
numbers) could represent a 4D point. We hoped that this approach will be
much more effective than traditional R-Tree in case of many overlaps in
box's collection.
Performance results are coming shortly.


It appears that the issues raised in this thread have been addressed but 
the patch still has not gone though a real review.


Anybody out there willing to take a crack at a review?  All three 
patches apply (with whitespace issues).


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] dealing with extension dependencies that aren't quite 'e'

[David Steele]

Hi Abhijit,

On 3/1/16 8:36 AM, Jim Nasby wrote:

On 2/29/16 10:33 PM, Abhijit Menon-Sen wrote:

>Given the audience for this, I think it'd probably be OK to just
>provide a function that does this, instead of DDL.

That seems like a promising idea. Can you suggest some possible usage?


pg_extension_dependency( regextension, any )

where "any" would be all the other reg* types. That should be a lot less
work to code up than messing with the grammar.


So where are we on this now?  Were you going to implement this as a 
function the way Jim suggested?


Alexander, you are signed up to review.  Any opinion on which course is 
best?


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[David Steele]

On 2/26/16 11:37 PM, Amit Kapila wrote:


On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila 

This patch no longer applies cleanly:

$ git apply ../other/group_update_clog_v6.patch
error: patch failed: src/backend/storage/lmgr/proc.c:404
error: src/backend/storage/lmgr/proc.c: patch does not apply
error: patch failed: src/include/storage/proc.h:152
error: src/include/storage/proc.h: patch does not apply

It's not clear to me whether Robert has completed a review of this code 
or it still needs to be reviewed more comprehensively.


Other than a comment that needs to be fixed it seems that all questions 
have been answered by Amit.


Is this "ready for committer" or still in need of further review?

--
-David
da...@pgmasters.net


--
Sent 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 backslash-continuations in custom scripts

[David Steele]

Hi Fabien,

On 3/14/16 3:27 PM, Fabien COELHO wrote:

>> Any takers to review this updated patch?
> 
> I intend to have a look at it, I had a look at a previous instance, but
> I'm ok if someone wants to proceed.

There's not exactly a long line of reviewers at the moment so if you
could do a followup review that would be great.

Thanks,
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[David Steele]

On 3/14/16 4:10 PM, Robbie Harwood wrote:
> David Steele  writes:
> 
>> Hi Robbie,
>>
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Hello friends,
>>>
>>> Here's yet another version of GSSAPI encryption support.  It's also
>>> available for viewing on my github:
>>
>> The build went fine but when testing I was unable to logon at all.  I'm
>> using the same methodology as in
>> http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except
>> that I'm running against 51c0f63 and using the v6 patch set.
>>
>> psql simply hangs and never returns.  I have attached a pcap of the
>> psql/postgres session generated with:
>>
>> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>>
>> If you would like me to capture more information please let me know
>> specifically how you would like me to capture it.
> 
> Please disregard my other email.  I think I've found the issue; will
> post a new version in a moment.

Strange timing since I was just testing this.  Here's what I got:

$ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres
conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
psql (9.6devel)
Type "help" for help.

postgres=>

This was after commenting out:

// appendBinaryPQExpBuffer(&conn->gwritebuf,
//  conn->inBuffer + conn->inStart,
//  conn->inEnd - conn->inStart);
// conn->inEnd = conn->inStart;

The good news I can log on every time now!

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[David Steele]

On 3/14/16 7:20 PM, Robbie Harwood wrote:

> David Steele  writes:
>
>>
>> Strange timing since I was just testing this.  Here's what I got:
>>
>> $ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres
>> conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
>> psql (9.6devel)
>> Type "help" for help.
>>
>> postgres=>
> 
> Thanks, that certainly is interesting!  I did finally manage to
> reproduce the issue on my end, but the rate of incidence is much lower
> than what you and Michael were seeing: I have to run connections in a
> loop for about 10-20 minutes before it makes itself apparent (and no,
> it's not due to entropy).  Apparently I just wasn't patient enough.

I'm running client and server on the same VM - perhaps that led to less
latency than your setup?

> All that is to say: thank you very much for investigating that!

My pleasure!

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[David Steele]

On 3/8/16 5:44 PM, Robbie Harwood wrote:

> Here's yet another version of GSSAPI encryption support.

OK, everything seems to be working fine with a 9.6 client and server so
next I tested older clients and I got this error:

$ /usr/lib/postgresql/9.1/bin/psql -h localhost \
  -U vagr...@pgmasters.net postgres
psql: FATAL:  GSSAPI did no provide required flags

There's a small typo in the error message, BTW.

Here's the output from the server logs:

DEBUG:  forked new backend, pid=7746 socket=9
DEBUG:  postgres child[7746]: starting with (
DEBUG:  postgres
DEBUG:  )
DEBUG:  InitPostgres
DEBUG:  my backend ID is 2
DEBUG:  StartTransaction
DEBUG:  name: unnamed; blockState:   DEFAULT; state: INPROGR,
xid/subid/cid: 0/1/0, nestlvl: 1, children:
DEBUG:  Processing received GSS token of length 667
DEBUG:  gss_accept_sec_context major: 0, minor: 0, outlen: 161,
outflags: 1b2
DEBUG:  sending GSS response token of length 161
DEBUG:  sending GSS token of length 161
FATAL:  GSSAPI did no provide required flags
DEBUG:  shmem_exit(1): 1 before_shmem_exit callbacks to make
DEBUG:  shmem_exit(1): 6 on_shmem_exit callbacks to make
DEBUG:  proc_exit(1): 3 callbacks to make
DEBUG:  exit(1)
DEBUG:  shmem_exit(-1): 0 before_shmem_exit callbacks to make
DEBUG:  shmem_exit(-1): 0 on_shmem_exit callbacks to make
DEBUG:  proc_exit(-1): 0 callbacks to make
DEBUG:  reaping dead processes
DEBUG:  server process (PID 7746) exited with exit code 1

I got the same result with a 9.4 client so didn't try any others.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[David Steele]

On 3/15/16 1:17 AM, Amit Kapila wrote:

> On Tue, Mar 15, 2016 at 12:00 AM, David Steele 
>> This patch no longer applies cleanly:
>>
>> $ git apply ../other/group_update_clog_v6.patch
>> error: patch failed: src/backend/storage/lmgr/proc.c:404
>> error: src/backend/storage/lmgr/proc.c: patch does not apply
>> error: patch failed: src/include/storage/proc.h:152
>> error: src/include/storage/proc.h: patch does not apply
> 
> For me, with patch -p1 <  it works, but any how I have
> updated the patch based on recent commit.  Can you please check the
> latest patch and see if it applies cleanly for you now.

Yes, it now applies cleanly (101fd93).

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

[David Steele]

On 3/14/16 12:27 PM, Artur Zakirov wrote:
> On 14.03.2016 18:48, David Steele wrote:
>> Hi Jeff,
>>
>> On 2/25/16 5:00 PM, Jeff Janes wrote:
>>
>>> But, It doesn't sound like I am going to win that debate.  Given that,
>>> I don't think we need a different name for the function. I'm fine with
>>> explaining the word-boundary subtlety in the documentation, and
>>> keeping the function name itself simple.
>>
>> It's not clear to me if you are requesting more documentation here or
>> stating that you are happy with it as-is.  Care to elaborate?
>>
>> Other than that I think this patch looks to be ready for committer. Any
>> objections?
>>
> 
> There was some comments about the word-boundary subtlety. But I think it
> was not enough.
> 
> I rephrased the explanation of word_similarity() and %>. It is better now.
> 
> But if it is not correct I can change the explanation.

Since to only change in the latest patch is to documentation I have
marked this "ready for committer".

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:

> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier  
> wrote:
>
>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele  wrote:
>>
>>> Could you provide an updated set of patches for review?  Meanwhile I am
>>> marking this as "waiting for author".
>>
>> Sure. I'll provide them shortly with all the comments addressed. Up to
>> now I just had a couple of comments about docs and whitespaces, so I
>> didn't really bother sending a new set, but this meritates a rebase.
> 
> And here they are. I have addressed the documentation and the
> whitespaces reported up to now at the same time.

For this first review I would like to focus on the user visible changes
introduced in 0001-0002.

First I created two new users with each type of supported verifier:

postgres=# create user test with password 'test';
CREATE ROLE
postgres=# create user testu with unencrypted password 'testu'
   valid until '2017-01-01';
CREATE ROLE

1) I see that rolvaliduntil is still in pg_authid:

postgres=# select oid, rolname, rolvaliduntil from pg_authid;

  oid  | rolname | rolvaliduntil
---+-+
10 | vagrant |
 16387 | test|
 16388 | testu   | 2017-01-01 00:00:00+00

I think that's OK if we now define it to be "role validity" (it's still
password validity in the patched docs).  I would also like to see a
validuntil column in pg_auth_verifiers so we can track password
expiration for each verifier separately.  For now I think it's enough to
copy the same validity both places since there can only be one verifier.

2) I don't think the column naming in pg_auth_verifiers is consistent
with other catalogs:

postgres=# select * from pg_auth_verifiers;

 roleid | verimet |   verival
+-+-
  16387 | m   | md505a671c66aefea124cc08b76ea6d30bb
  16388 | p   | testu

System catalogs generally use a 3 character prefix so I would expect the
columns to be (if we pick avr as a prefix):

avrrole
avrmethod
avrverifier
avrvaliduntil

I'm not a big fan in abbreviating too much so you can see I've expanded
the names a bit.

3) rolpassword is still in pg_shadow even though it is not useful anymore:

postgres=# select usename, passwd, valuntil from pg_shadow;

 usename |  passwd  |valuntil
-+--+
 vagrant |  |
 test|  |
 testu   |  | 2017-01-01 00:00:00+00

If anyone is actually using this column in a meaningful way they are in
for a nasty surprise when trying use the value in passwd as a verifier.
 I would prefer to drop the column entirely and produce a clear error.

Perhaps a better option would be to drop pg_shadow entirely since it
seems to have no further purpose in life.

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] silent data loss with ext4 / all current versions

[David Steele]

On 3/15/16 10:39 AM, Michael Paquier wrote:

> On Thu, Mar 10, 2016 at 4:25 AM, Andres Freund wrote:
> 
>> Note that we currently have some frontend programs with the equivalent
>> problem. Most importantly receivelog.c (pg_basebackup/pg_recveivexlog)
>> are missing pretty much the same directory fsyncs.  And at least for
>> pg_recvxlog it's critical, especially now that receivexlog support
>> syncrep.  I've not done anything about that; there's pretty much no
>> chance to share backend code with the frontend in the back-branches.
> 
> Yeah, true. We definitely need to do something for that, even for HEAD
> it seems like an overkill to have something in for example src/common
> to allow frontends to have something if the fix is localized
> (pg_rewind may use something else), and it would be nice to finish
> wrapping that for the next minor release, so I propose the attached
> patches.

I noticed this when reviewing the pg_receive_xlog refactor and was going
to submit a patch after the CF.  It didn't occur to me to piggyback on
this work but I think it makes sense.

+1 from me for fixing this in pg_receivexlog and back-patching.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] ALTER TABLE behind-the-scenes effects' CONTEXT

[David Steele]

On 1/30/16 10:52 AM, Marko Tiikkaja wrote:
> On 2016-01-21 04:17, Simon Riggs wrote:
>> Marko, I was/am waiting for an updated patch. Could you comment please?
> 
> Sorry, I've not found time to work on this recently.
> 
> Thanks for everyone's comments so far.  I'll move this to the next CF
> and try and get an updated patch done in time for that one.

There's been no activity on this thread for while and no new patch was
submitted for the CF.

I have marked it "returned with feedback".

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[David Steele]

On 3/15/16 1:42 PM, Robert Haas wrote:
> On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
>  wrote:
>> Hello, this is the new version of this patch.
> 
> The CommitFest entry for this patch is messed up.  It shows no author,
> even though I'm pretty sure that a patch has to have one by
> definition.
> 
> https://commitfest.postgresql.org/9/518/
> 
> Also, this patch was listed as Waiting on Author, but it's been
> updated since it was last reviewed, so I switched it back to Needs
> Review.  Can someone please review it and, if appropriate, mark it
> Ready for Committer?

Author has been set to Kyotaro Horiguchi.

-- 
-David
da...@pgmasters.net


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

[HACKERS] Re: [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator

[David Steele]

Hi Kevin,

On 3/1/16 11:08 AM, Roma Sokolov wrote:
>> On 27 Feb 2016, at 03:46, Euler Taveira  wrote:
>> Because it is not a common practice to test catalog dependency on
>> separate tests (AFAICS initial catalogs are tested with oidjoins.sql).
>> Also, your test case is too huge for such a small use case. 
> 
> It seems oidjoins.sql is automatically generated and contains tests only for
> initial database state. On the other hand, there are tests for CREATE OPERATOR
> and ALTER OPERATOR, so it seems reasonable to me to have separate DROP 
> OPERATOR
> test, or to move all operator related testing to one file. This is however
> clearly outside of the scope of this patch, so in v3 I've simplified tests 
> using
> queries from oidjoins.sql.

You've signed up to review this patch, do you have an idea of when you
might be able to do the review?

Thanks,
-- 
-David
da...@pgmasters.net


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

[HACKERS] Re: pg_hba_lookup function to get all matching pg_hba.conf entries

[David Steele]

On 3/3/16 12:16 AM, Haribabu Kommi wrote:
> On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi  
> wrote:
>>
>> This patch needs to be applied on top discard_hba_and_ident_cxt patch
>> that is posted earlier.
> 
> Here I attached a re-based patch to the latest head with inclusion of
> discard_hba_ident_cxt patch for easier review as a single patch.

Alex, Scott, do you have an idea of when you'll be able to review this
new version?

It applies with a minor conflict (caused by pg_control_* commit):

$ git apply -3 ../other/pg_hba_lookup_poc_v13.patch
error: patch failed: src/include/utils/builtins.h:1151
Falling back to three-way merge...
Applied patch to 'src/include/utils/builtins.h' with conflicts.
U src/include/utils/builtins.h

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent 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 stats per script & other stuff

[David Steele]

On 3/4/16 1:53 PM, Fabien COELHO wrote:

>>> That is why the "fs" variable in process_file is declared "static",
>>> and why
>>> I wrote "some hidden awkwarness".
>>>
>>> I did want to avoid a malloc because then who would free the struct?
>>> addScript cannot to it systematically because builtins are static. Or it
>>> would have to create an on purpose struct, but I then that would be more
>>> awkwarness, and malloc/free to pass arguments between functions is not
>>> efficient nor very elegant.
>>>
>>> So the "static" option looked like the simplest & most elegant version.
>>
>> Surely that trick breaks if you have more than one -f switch, no?  Oh, I
>> see what you're doing: you only use the command list, which is
>> allocated, so it doesn't matter that the rest of the struct changes
>> later.
> 
> The two fields that matter (desc and commands) are really copied into
> sql_scripts, so what stays in the is overriden if used another time.
> 
>> I'm not concerned about freeing the struct; what's the problem with it
>> surviving until the program terminates?
> 
> It is not referenced anywhere so it is a memory leak.
> 
>> If somebody specifies thousands of -f switches, they will waste a few
>> bytes with each, but I'm hardly concerned about a few dozen kilobytes
>> there ...
> 
> Ok, so you prefer a memory leak. I hate it on principle.
> 
> Here is a v23 with a memory leak anyway.

Álvaro, it looks like you've been both reviewer and committer on this
work for some time.

The latest patch seems to address you final concern.  Can I mark it
"ready for committer"?

-- 
-David
da...@pgmasters.net


-- 
Sent 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] Supporting +-Infinity values by to_timestamp(float8)

[David Steele]

On 3/4/16 2:56 PM, Vitaly Burovoy wrote:

> On 3/4/16, Anastasia Lubennikova  wrote:
>
>> I think that you should update documentation. At least description of
>> epoch on this page:
>> http://www.postgresql.org/docs/devel/static/functions-datetime.html
> 
> Thank you very much for pointing where it is located (I saw only
> "to_timestamp(TEXT, TEXT)").
> I'll think how to update it.

Vitaly, have you decided how to update this yet?

>> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
>> abbreviation, but it seems slightly confusing to me.
> 
> It doesn't matter for me what it is called, it is short enough and
> reflects a type on which it is applied.
> What would the best name be for it?

Anastasia, any suggestions for a better name, or just leave it as is?

I'm not in favor of the "4", either.  I think I would prefer
JULIAN_MAXYEAR_STAMP.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] statistics for array types

[David Steele]

On 3/9/16 7:42 PM, David Steele wrote:
> On 1/18/16 11:24 AM, Alvaro Herrera wrote:
>> Alexander Korotkov wrote:
>>
>>> The patch implementing my idea above is attached.
>> What's the status here?  Jeff, did you have a look at Alexander's
>> version of your patch?  Tomas, does this patch satisfy your concerns?
> 
> This was marked as "needs review" but I have changed it to "waiting for
> author".

There has been no activity on this thread in quite a while and no
updated patch during this CF.  It has been marked "returned with
feedback". Please feel free to resubmit for 9.7!

-- 
-David
da...@pgmasters.net


-- 
Sent 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: Generic WAL logical messages

[David Steele]

On 3/9/16 6:58 PM, Petr Jelinek wrote:
> On 08/03/16 21:21, Artur Zakirov wrote:
>> I think here
>>
>>> +const char *
>>> +logicalmsg_identify(uint8 info)
>>> +{
>>> +if (info & ~XLR_INFO_MASK == XLOG_LOGICAL_MESSAGE)
>>> +return "MESSAGE";
>>> +
>>> +return NULL;
>>> +}
>>
>> we should use brackets
>>
>> const char *
>> logicalmsg_identify(uint8 info)
>> {
>>  if ((info & ~XLR_INFO_MASK) == XLOG_LOGICAL_MESSAGE)
>>  return "MESSAGE";
>>
>>  return NULL;
>> }
>>
> 
> Correct, fixed, thanks.
> 
> I also rebased this as there was conflict after the fixes to logical
> decoding by Andres.

This patch applies cleanly and is ready for review with no outstanding
issues that I can see.  Simon and Artur, you are both signed up as
reviewers.  Care to take a crack at it?

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

On 3/16/16 9:00 AM, Michael Paquier wrote:

> On Tue, Mar 15, 2016 at 6:38 PM, David Steele  wrote:
>
>> 1) I see that rolvaliduntil is still in pg_authid:
>> I think that's OK if we now define it to be "role validity" (it's still
>> password validity in the patched docs).  I would also like to see a
>> validuntil column in pg_auth_verifiers so we can track password
>> expiration for each verifier separately.  For now I think it's enough to
>> copy the same validity both places since there can only be one verifier.
> 
> FWIW, this is an intentional change, and my goal is to focus on only
> the protocol aging for now. We will need to move rolvaliduntil to
> pg_auth_verifiers if we want to allow rolling updates of password
> verifiers for a given role, but that's a different patch, and we need
> to think about the SQL interface carefully. This infrastructure makes
> the move easier by the way to do that, and honestly I don't really see
> what we gain now by copying the same value to two different system
> catalogs.

Here's my thinking.  If validuntil is moved to pg_auth_verifiers now
then people can start using it there.  That will make it less traumatic
when/if validuntil in pg_authid is removed later.  The field in
pg_authid could be deprecated in this release to let people know not to
use it.

Or, as I suggested it could be recast as role validity, which right now
happens to be the same as password validity.

>> 2) I don't think the column naming in pg_auth_verifiers is consistent
>> with other catalogs:
>> postgres=# select * from pg_auth_verifiers;
>>  roleid | verimet |   verival
>> +-+-
>>   16387 | m   | md505a671c66aefea124cc08b76ea6d30bb
>>   16388 | p   | testu
>>
>> System catalogs generally use a 3 character prefix so I would expect the
>> columns to be (if we pick avr as a prefix):
> 
> OK, this makes sense.
> 
>> avrrole
>> avrmethod
>> avrverifier
> 
> Assuming "ver" is the prefix, we get: verroleid, vermethod, vervalue.
> I kind of like those ones, more than with "avr" as prefix actually.
> Other ideas are of course welcome.

ver is fine as a prefix.

>> 3) rolpassword is still in pg_shadow even though it is not useful anymore:
>> postgres=# select usename, passwd, valuntil from pg_shadow;
>>
>>  usename |  passwd  |valuntil
>> -+--+
>>  vagrant |  |
>>  test|  |
>>  testu   |  | 2017-01-01 00:00:00+00
>>
>> If anyone is actually using this column in a meaningful way they are in
>> for a nasty surprise when trying use the value in passwd as a verifier.
>>  I would prefer to drop the column entirely and produce a clear error.
>>
>> Perhaps a better option would be to drop pg_shadow entirely since it
>> seems to have no further purpose in life.
> 
> We discussed that on the previous thread and the conclusion was to
> keep pg_shadow, but to clobber the password value with "***",
> explaining this choice:
> http://www.postgresql.org/message-id/6174.1455501...@sss.pgh.pa.us

Ah, I missed that one.

-- 
-David
da...@pgmasters.net


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

[David Steele]

On 3/10/16 7:34 AM, Simon Riggs wrote:

On 9 March 2016 at 23:11, David Steele mailto:da...@pgmasters.net>> wrote:

There hasn't been any movement on this patch in a while.  Will you
have a new tests ready for review soon?


I see the value in this feature, but the patch itself needs work and
probably some slimming down/reality and a good spellcheck.

If someone takes this on soon it can go into 9.6, otherwise I vote to
reject this early to avoid wasting review time.


Since there has been no response from the author I have marked this 
patch "returned with feedback".  Please feel free to resubmit for 9.7!


--
-David
da...@pgmasters.net


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

[HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

[David Steele]

On 3/10/16 1:24 PM, Corey Huinker wrote:


New patch for Alvaro's consideration.

Very minor changes since the last time, the explanations below are
literally longer than the changes:
- Rebased, though I don't think any of the files had changed in the
mean time
- Removed infinity checks/errors and the test cases to match
- Amended documentation to add 'days' after 'step' as suggested
- Did not add a period as suggested, to remain consistent with other
descriptions in the same sgml table
- Altered test case and documentation of 7 day step example so that
the generated dates do not land evenly on the end date. A reader
might incorrectly infer that the end date must be in the result set,
when it doesn't have to be.
- Removed unnecessary indentation that existed purely due to
following of other generate_series implementations


As far as I can see overall support is in favor of this patch although 
it is not overwhelming by any means.


I think in this case it comes down to a committer's judgement so I have 
marked this "ready for committer" and passed the buck on to Álvaro.


--
-David
da...@pgmasters.net


--
Sent 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: Add generate_series(date,date) and generate_series(date,date,integer)

[David Steele]

On 3/17/16 1:17 PM, Robert Haas wrote:
> On Thu, Mar 17, 2016 at 12:59 PM, Tom Lane  wrote:
>> One idea that might be worth considering is to define the function
>> as generate_series(date,date,interval) returns timestamp (without
>> time zone).  The point here would be only to move the behavior for
>> date inputs as far as getting timestamp without tz rather than
>> timestamp with tz; which would at least save some timezone rotations
>> in typical use, as well as rather debatable semantics.  (The fact
>> that timestamptz is the preferred type in this hierarchy isn't
>> really doing us any favors here.)
> 
> That's a fairly tenuous benefit, though, and a substantially different
> patch.  I think it's time to give up here and move on.  We can revisit
> this for another release after we've had more time to think about it,
> if that seems like a smart thing to do when the time comes.

This has been marked "returned with feedback".

-- 
-David
da...@pgmasters.net


-- 
Sent 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] Request - repeat value of \pset title during \watch interations

[David Steele]

On 3/17/16 5:07 PM, David G. Johnston wrote:

> Figured out it had to be added to 2016-09...done

Hmm ... this patch is currently marked "needs review" in CF 2016-03.  Am
I missing something, should this have been closed?

-- 
-David
da...@pgmasters.net


-- 
Sent 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: SET ROLE hook

[David Steele]

On 3/6/16 1:17 AM, Pavel Stehule wrote:

> 2016-03-05 21:49 GMT+01:00 Joe Conway  >:
> 
> I still don't see any point in trying to pass data back from the hooks
> as the extension can maintain that state just fine, although it looks
> like it would be pretty trivial to do using a new void* member added to
> role_auth_extra. Tom (or anyone else), any comment on that?
> 
> see Tom's comment, I share his opinion.
> 
> I do however find myself wishing I could pass the action down from
> set_config_option() to at least the assign_role() hook, but that seems
> more invasive than I'd like.
> 
> describe this use case, please.

Joe, it looks there are some unresolved questions from Pavel and Craig
on this thread and probably a new patch is required.  Any idea when you
can get to that?

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] jsonb array-style subscription

[David Steele]

Hi Dmitry,

On 3/3/16 5:31 AM, Dmitry Dolgov wrote:
>> If the patch were proposing a similar amount of new infrastructure to
>> support some datatype-extensible concept of subscripting, I'd be much
>> happier about it.
> 
> Well, actually, I agree with that. I can try to rework the patch to
> achieve this goal.

Do you have an updated patch ready?  If so please post it by Monday or
this patch will be marked "returned with feedback".

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent 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: Add generate_series(date,date) and generate_series(date,date,integer)

[David Steele]

On 3/17/16 11:30 AM, David G. Johnston wrote:
> On Thu, Mar 17, 2016 at 7:57 AM, Corey Huinker  <mailto:corey.huin...@gmail.com>>wrote:
> 
> On Thu, Mar 17, 2016 at 10:00 AM, David Steele  <mailto:da...@pgmasters.net>> wrote:
> 
> On 3/17/16 4:49 AM, Dean Rasheed wrote:
> 
> > On 16 March 2016 at 23:32, David Steele  <mailto:da...@pgmasters.net>> wrote:
> >
> >>
> >> I think in this case it comes down to a committer's judgement so I 
> have
> >> marked this "ready for committer" and passed the buck on to Álvaro.
> >
> > So I was pretty much "meh" on this patch too, because I'm not
> > convinced it actually saves much typing, if any.
> >
> > However, I now realise that it introduces a backwards-compatibility
> > breakage. Today it is possible to type
> >
> > SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7 
> days');
> 
> It can also be broken as below and this is even scarier to me:
> 
> 
> Above and below are the same query​...

Not sure I agree.  My point was that even if developers were pretty
careful with their casting (or are using two actual dates) then there's
still possibility for breakage.

> postgres=# SELECT * FROM generate_series('01-01-2000'::date,
> '01-04-2000'::date, '7 days');
> ERROR:  invalid input syntax for integer: "7 days"
> LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7
> days');
> <...>
> 
> I see two ways around this: 
> 
> 1. Drop the step parameter entirely. My own use cases only ever
> require the step values 1 and -1, and which one the user wants can
> be inferred from (start < end). This would still change the output
> type where a person wanted timestamps, but instead input two dates.
> 
> ​Undesirable.​

Very undesirable.  Week intervals are a very valid use case and I don't
like the automagic interval idea.

> 
> 2. Rename the function date_series() or generate_series_date()
> 
> I still think this is an important function because at the last
> several places I've worked, I've found myself manufacturing a query
> where some event data is likely to have date gaps, but we want to
> see the date gaps or at least have the 0 values contribute to a
> later average aggregate.
> 
> 
> ​I'd call it "generate_dates(...)" and be done with it.
> 
> We would then have:
> 
> generate_series()
> generate_subscripts()
> generate_dates()

To me this completely negates the idea of this "just working" which is
why it got a +1 from me in the first place.  If I have to remember to
use a different function name then I'd prefer to just cast on the
timestamp version of generate_series().

Sorry, but this is now -1 from me, at least for this commitfest.  While
I like the idea I think it is far too late to be redesigning such a
minor feature.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

Hi Magnus,

On 3/2/16 12:49 PM, Marco Nenciarini wrote:

> I've finally found some time to take a look to the patch.
> 
> It applies with some fuzziness on master, but the result looks correct.
> Unfortunately the OID of the new pg_stop_backup function conflicts with
> "pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).
>
> After changing it the patch does not compile:

It's been two weeks since Marco found these issues in the patch.  Please
provide an updated patch soon or I will mark this "returned with feedback".

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent 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] Request - repeat value of \pset title during \watch interations

[David Steele]

On 3/17/16 7:00 PM, Tom Lane wrote:
> David Steele  writes:
>> On 3/17/16 5:07 PM, David G. Johnston wrote:
>>> Figured out it had to be added to 2016-09...done
> 
>> Hmm ... this patch is currently marked "needs review" in CF 2016-03.  Am
>> I missing something, should this have been closed?
> 
> The message I saw was post-1-March.  If it was in fact submitted in
> time for 2016-03, then we owe it a review.

I meant to add the CF record and forgot:

https://commitfest.postgresql.org/9/480

It was added 2016-01-13 by Michael Paquier.

-- 
-David
da...@pgmasters.net


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

[HACKERS] Re: [PATH] Jsonb, insert a new value into an array at arbitrary position

[David Steele]

Hi Dmitry,

On 3/7/16 8:50 PM, Vitaly Burovoy wrote:

> On 2016-02-29 17:19+00, Dmitry Dolgov <9erthali...@gmail.com> wrote:
> 
> Hello! The first pass of a review is below.

It's be over a week since Vitaly posted this review.  Please respond
and/or provide a new patch as soon as you can.

Thanks,
-- 
-David
da...@pgmasters.net


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

[David Steele]

On 3/14/16 9:57 AM, Anastasia Lubennikova wrote:

> New version of the patch implements pg_dump well.
> Documentation related to constraints is updated.
> 
> I hope, that patch is in a good shape now.

It looks like this patch should be marked "needs review" and I have done so.

-- 
-David
da...@pgmasters.net


-- 
Sent 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: Add generate_series(date,date) and generate_series(date,date,integer)

[David Steele]

On 3/17/16 4:49 AM, Dean Rasheed wrote:

> On 16 March 2016 at 23:32, David Steele  wrote:
>
>>
>> I think in this case it comes down to a committer's judgement so I have
>> marked this "ready for committer" and passed the buck on to Álvaro.
> 
> So I was pretty much "meh" on this patch too, because I'm not
> convinced it actually saves much typing, if any.
> 
> However, I now realise that it introduces a backwards-compatibility
> breakage. Today it is possible to type
> 
> SELECT * FROM generate_series('01-01-2000'::date, '01-04-2000', '7 days');

It can also be broken as below and this is even scarier to me:

postgres=# SELECT * FROM generate_series('01-01-2000'::date,
'01-04-2000'::date, '7 days');
ERROR:  invalid input syntax for integer: "7 days"
LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7 days');

And only works when:

postgres=# SELECT * FROM generate_series('01-01-2000'::date,
'01-04-2000'::date, '7 days'::interval);
generate_series

 2000-01-01 00:00:00+00
(1 row)

One might argue that string constants for dates in actual code might be
rare but I think it's safe to say that string constants for intervals
are pretty common.  I also think it unlikely that they are all
explicitly cast.

I marked this "waiting for author" so Corey can respond.  I actually
tested with the v3 patch since the v4 patch seems to be broken with git
apply or patch:

$ patch -p1 < ../other/generate_series_date.v4.diff
patching file doc/src/sgml/func.sgml
Hunk #1 succeeded at 14787 (offset 87 lines).
Hunk #2 succeeded at 14871 (offset 87 lines).
patching file src/backend/utils/adt/date.c
patch:  malformed patch at line 163: diff --git
a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h

$ git apply -3 ../other/generate_series_date.v4.diff
fatal: corrupt patch at line 163

-- 
-David
da...@pgmasters.net


-- 
Sent 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: make NOTIFY list de-duplication optional

[David Steele]

On 3/11/16 1:46 PM, David Steele wrote:

Hi Filip,

On 2/20/16 8:00 AM, Filip Rembiałkowski wrote:

On Fri, Feb 19, 2016 at 10:09 PM, Catalin Iacob mailto:t...@sss.pgh.pa.us>>
 wrote:
 > FWIW, I think it would be a good thing if the NOTIFY statement syntax 
were
 > not remarkably different from the syntax used in the pg_notify() function
 > call.  To do otherwise would certainly be confusing.  So on the whole
 > I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.

 Filip, do you agree with Tom's proposal? Do you plan to rework the
 patch on these lines? If you are I'll try to review it, if not I could
 give it a shot as I'm interested in having this in 9.6.

I see that Tom's remarks give more flexibility, and your refinement
makes sense.


It looks like we are waiting on a new patch from you before this can be
reviewed.  Are you close to having that done?

Meanwhile, I have marked it "Waiting on Author".


Since there has been no activity on this thread since before the CF and 
no response from the author I have marked this "returned with feedback". 
Please feel free to resubmit for 9.7!


--
-David
da...@pgmasters.net


--
Sent 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_ctl promote wait

[David Steele]

Hi Peter,

On 3/9/16 3:08 PM, Michael Paquier wrote:

> Here are some comments about 0002
<...>
> I think that we had better do something like the attached first.
> Thoughts?

It's been a week since Michael reviewed this patch.  Could you please
comment on his proposed changes as soon as possible?

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Speedup twophase transactions

[David Steele]

On 1/26/16 3:39 PM, Stas Kelvich wrote:

Agree, I had the same idea in my mind when was writing that script.

I will migrate it to TAP suite and write a review for Michael Paquier's patch.

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



On 26 Jan 2016, at 20:20, Alvaro Herrera  wrote:

Stas Kelvich wrote:


While this patch touches quite sensible part of postgres replay and there is 
some rarely used code paths, I wrote shell script to setup master/slave 
replication and test different failure scenarios that can happened with 
instances. Attaching this file to show test scenarios that I have tested and 
more importantly to show what I didn’t tested. Particularly I failed to 
reproduce situation where StandbyTransactionIdIsPrepared() is called, may be 
somebody can suggest way how to force it’s usage. Also I’m not too sure about 
necessity of calling cache invalidation callbacks during 
XlogRedoFinishPrepared(), I’ve marked this place in patch with 2REVIEWER 
comment.


I think this is the third thread in which I say this: We need to push
Michael Paquier's recovery test framework, then convert your test script
to that.  That way we can put your tests in core.


It seems this thread has been waiting quite a while on a new patch.  If 
one doesn't appear by Monday I will mark this "returned with feedback".


--
-David
da...@pgmasters.net


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

Re: [HACKERS] eXtensible Transaction Manager API (v2)

[David Steele]

On 3/16/16 7:59 AM, Stas Kelvich wrote:
> On 12 Mar 2016, at 13:19, Michael Paquier  wrote:
>>
>> On Fri, Mar 11, 2016 at 9:35 PM, Tom Lane  wrote:
>>> IMO this is not committable as-is, and I don't think that it's something
>>> that will become committable during this 'fest.  I think we'd be well
>>> advised to boot it to the 2016-09 CF and focus our efforts on other stuff
>>> that has a better chance of getting finished this month.
>>
>> Yeah, I would believe that a good first step would be to discuss
>> deeply about that directly at PGCon for folks that will be there and
>> interested in the subject. It seems like a good timing to brainstorm
>> things F2F at the developer unconference for example, a couple of
>> months before the 1st CF of 9.7. We may perhaps (or not) get to
>> cleaner picture of what kind of things are wanted in this area.
> 
> To give overview of xtm coupled with postgres_fdw from users perspective i’ve 
> packed patched postgres with docker
> and provided test case when it is easy to spot violation of READ COMMITTED 
> isolation level without XTM.
> 
> This test fills database with users across two shards connected by 
> postgres_fdw and inherits the same table. Then 
> starts to concurrently transfers money between users in different shards:
> 
> begin;
> update t set v = v - 1 where u=%d; -- this is user from t_fdw1, first shard
> update t set v = v + 1 where u=%d; -- this is user from t_fdw2, second shard
> commit;
> 
> Also test simultaneously runs reader thread that counts all money in system:
> 
> select sum(v) from t;
> 
> So in transactional system we expect that sum should be always constant (zero 
> in our case, as we initialize user with zero balance).
> But we can see that without tsdtm total amount of money fluctuates around 
> zero.
> 
> https://github.com/kelvich/postgres_xtm_docker

This is an interesting example but I don't believe it does much to
address the concerns that were raised in this thread.

As far as I can see the consensus is that this patch should not be
considered for the current CF so I have marked it "returned with feedback".

If possible please follow Michael's advice and create a session at the
PGCon unconference in May.  I'm certain there will be a lot of interest.

-- 
-David
da...@pgmasters.net


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

[David Steele]

On 3/12/16 1:24 AM, Pavel Stehule wrote:


Great, thank you very much. I hope so this feature really useful. It
allow to simple export/import XML doc in different encodings, JSONs and
can be enhanced future via options. The nice feature  (but not for this
release) can be additional cast info for import -- like "COPY
table(jsonb_column) FROM stdin (FORMAT RAW, CAST json_2_jsonb). Because
there are the options, there are big space for other enhancing.


Andres Karlsson pointed out that this patch has two CF entries:

https://commitfest.postgresql.org/9/223/
https://commitfest.postgresql.org/9/547/

I closed the one that was in the "needs review" (547) state and kept the 
one that is "ready for committer" (223).


--
-David
da...@pgmasters.net


--
Sent 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: Add generate_series(date,date) and generate_series(date,date,integer)

[David Steele]

On 3/17/16 11:55 AM, David G. Johnston wrote:

> On Thu, Mar 17, 2016 at 8:41 AM, David Steele  <mailto:da...@pgmasters.net>>wrote:
> 
>> Not sure I agree.  My point was that even if developers were pretty
>> careful with their casting (or are using two actual dates) then there's
>> still possibility for breakage.
> 
> With the first argument casted to date it doesn't matter whether you
> cast the second argument as the pseudo-type anyelement will take its
> value from the first argument and force the second to date.

Ah, I see.

> The main problem is that the system tries to cast unknown to integer
> based upon finding gs(date, date, integer) and fails without ever
> considering that gs(ts, ts, interval) would succeed.

I'm tempted to say, "why don't we just fix that?" but I'm sure any
changes in that area would introduce a variety of new and interesting
behaviors.

> ​So let the user decide whether to trade-off learning a new function
> name but getting dates instead of timestamps or going through the hassle
> of casting.​
> 
> For me, it would have been a nice bonus if the generate_series() could
> be used directly but I feel that the desired functionality is desirable
> and the name itself is of only minor consideration.
> 
> I can see that newbies might ponder why two functions exist but they
> should understand "backward compatibility".
> 
> I suspect that most people will simply think: "use generate_series for
> numbers and use generate_dates for, well, dates".  The ones left out in
> the cold are those wondering why they use "generate_series" for
> timestamp series with a finer precision than date.  I'd be willing to
> offer a "generate_timestamps" to placate them.  And might as well toss
> in "generate_numbers" for completeness - though now I likely doom the
> proposal...so I'll stick with just add generate_dates so the behavior is
> possible without superfluous casting or the need to write a custom
> function.  Dates are too common a unit of measure to leave working with
> them this cumbersome.

I'm not finding this argument persuasive.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] 2016-03 Commitfest

[David Steele]

We are now nearly two thirds of the way through the 2016-03 Commitfest.

There are still some patches left that need review but have no reviewer
(https://commitfest.postgresql.org/9/?status=1&reviewer=-2) and this 
hasn't changed much since last week.


Needs review: 55 (was 56 last week)
Needs *reviewer*: 12 (was 15 last week)

There are still a lot of patches that need review but the good news is 
that nearly all of them have seen some level of review.


Waiting on Author: 11 (was 29 last week)

The number of patches waiting on author has gone down quite a bit but 
that was somewhat on account of idle patches being closed and a few that 
needed to be marked for review.


49% of the patches are now closed and I suspect that the low hanging 
fruit has been cleared away and we are left with more complicated patches.


Committed: 56 (was 46 last week)
Rejected: 5 (was 4 last week)
Returned with Feedback: 11 (was 4 last week)

** PATCH AUTHORS PLEASE READ **

If you have been pinged on a thread that is "waiting for author" and do 
not respond by next Tuesday your patch will likely be closed.  We are 
now in crunch time with two weeks until the end of the CF and three 
weeks until feature freeze.  If you have extenuating circumstances 
please make them clear on the thread so everyone knows the status.


Thanks,
--
-David
da...@pgmasters.net


--
Sent 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 Access Method WIP

[David Steele]

On 3/4/16 11:09 PM, Petr Jelinek wrote:

> But first here is updated patch for sequence access methods. I went with
> the previously discussed custom type as this gives us proper control
> over the width of the state and making sure that it's not gonna be
> toastable, etc and we need this as sequence is limited to single page.
> 
> Attached are 3 separate files. The first one (0001-create-am) is mainly
> separate for the reason that there is some overlap with Alexander's
> index access methods patch, so I extracted common code into separate
> patch as it will make it easier to merge in case we are lucky enough to
> get these patches in (but it's still based on Alexander's code). It
> provides infra for defining new access methods from SQL, although
> without the parser and without any actual access methods.
> 
> The 0002-seqam provides the SQL interface and support for sequence
> access methods on top of the infra patch, and also provides all the
> sequence access methods infrastructure and abstraction and documentation.
> 
> And finally the 0003-gapless-seq is example contrib module that
> implements dependably and transitionally safe gapless sequence access
> method. It's obviously slow as it has to do locking and basically
> serialize all the changes to sequence so only one transaction may use it
> at a time but it's truly gapless. It also serves as an example of use of
> the api and as a test.

As far as I can see Petr has addressed all the outstanding issues in
this patch and it's ready for a review.  The patch applies with some
easily-fixed conflicts:

$ git apply -3 ../other/0002-seqam-2015-03-05.patch
error: patch failed: src/include/catalog/pg_proc.h:5225
Falling back to three-way merge...
Applied patch to 'src/include/catalog/pg_proc.h' with conflicts.

Simon, you are signed up to review.  Do you have an idea of when you'll
be doing that?

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] 2016-03 Commitfest

[David Steele]

On 3/18/16 11:25 AM, Andreas Karlsson wrote:


The COPY RAW patch seems to have two entries in the commitfest.

https://commitfest.postgresql.org/9/223/ and
https://commitfest.postgresql.org/9/547/

Are those about the same patch?


Indeed they are - good catch!  I've responded on the thread and closed #547.

--
-David
da...@pgmasters.net


--
Sent 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: index-only scans with partial indexes

[David Steele]

On 3/9/16 3:29 AM, Kyotaro HORIGUCHI wrote:

> Hello, thank you for the comments. The new v8 patch is attched.

As far as I can see this patch should be marked "ready for review" so
now it is.

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:
> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier
>  wrote:
>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele  wrote:
>>> Could you provide an updated set of patches for review?  Meanwhile I am
>>> marking this as "waiting for author".
>>
>> Sure. I'll provide them shortly with all the comments addressed. Up to
>> now I just had a couple of comments about docs and whitespaces, so I
>> didn't really bother sending a new set, but this meritates a rebase.
> 
> And here they are. I have addressed the documentation and the
> whitespaces reported up to now at the same time.

Here's my full review of this patch set.

First let me thank you for submitting this patch for the current CF.  I
feel a bit guilty that I requested it and am only now posting a full
review.  In my defense I can only say that being CFM has been rather
more work than I was expecting, but I'm sure you know the feeling.

* [PATCH 1/9] Add facility to store multiple password verifiers

This is a pretty big patch but I went through it carefully and found
nothing to complain about.  Your attention to detail is impressive as
always.

Be sure to update the column names for pg_auth_verifiers as we discussed
in [1].

* [PATCH 2/9] Introduce password_protocols

diff --git a/src/test/regress/expected/password.out
b/src/test/regress/expected/password.out
+SET password_protocols = 'plain';
+ALTER ROLE role_passwd5 PASSWORD VERIFIERS (plain = 'foo'); -- ok
+ALTER ROLE role_passwd5 PASSWORD VERIFIERS (md5 = 'foo'); -- error
+ERROR:  specified password protocol not allowed
+DETAIL:  List of authorized protocols is specified by password_protocols.

So that makes sense but you get the same result if you do:

postgres=# alter user role_passwd5 password 'foo';
ERROR:  specified password protocol not allowed
DETAIL:  List of authorized protocols is specified by password_protocols.

I don't think this makes sense - if I have explicitly set
password_protocols to 'plain' and I don't specify a verifier for alter
user then it seems like it should work.  If nothing else the error
message lacks information needed to identify the problem.

* [PATCH 3/9] Add pg_auth_verifiers_sanitize

This function is just a little scary but since password_protocols
defaults to 'plain,md5' I can live with it.

* [PATCH 4/9] Remove password verifiers for unsupported protocols in
pg_upgrade

Same as above - it will always be important for password_protocols to
default to *all* protocols to avoid data being dropped during the
pg_upgrade by accident.  You've done that here (and later in the SCRAM
patch) so I'm satisfied but it bears watching.

What I would do is add some extra comments in the GUC code to make it
clear to always update the default when adding new verifiers.

* [PATCH 5/9] Move sha1.c to src/common

This looks fine to me and is a good reuse of code.

* [PATCH 6/9] Refactor sendAuthRequest

I tested this across different client versions and it seems to work fine.

* [PATCH 7/9] Refactor RandomSalt to handle salts of different lengths

A simple enough refactor.

* [PATCH 8/9] Move encoding routines to src/common/

A bit surprising that these functions were never used by any front end code.

* Subject: [PATCH 9/9] SCRAM authentication

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
@@ -1616,18 +1619,34 @@ FlattenPasswordIdentifiers(List *verifiers, char
*rolname)
 * instances of Postgres, an md5 hash passed as a plain verifier
 * should still be treated as an MD5 entry.
 */
-   if (spec->veriftype == AUTH_VERIFIER_MD5 &&
-   !isMD5(spec->value))
+   switch (spec->veriftype)
{
-   char encrypted_passwd[MD5_PASSWD_LEN + 1];
-   if (!pg_md5_encrypt(spec->value, rolname, 
strlen(rolname),
-   
encrypted_passwd))
-   elog(ERROR, "password encryption failed");
-   spec->value = pstrdup(encrypted_passwd);
+   case AUTH_VERIFIER_MD5:

It seems like this case statement should have been introduced in patch
0001.  Were you just trying to avoid churn in the code unless SCRAM is
committed?

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
+
+static char *
+read_attr_value(char **input, char attr)
+{

Numerous functions like the above in auth-scram.c do not have comments.

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
+   else if (strcmp(token->string, "scram") == 0)
+   {
+   if (Db_user_namespace)
+   {
+   

Re: [HACKERS] [PATCH] Phrase search ported to 9.6

[David Steele]

On 2/29/16 10:33 AM, Artur Zakirov wrote:

> Conclusion
> --
> 
> This patch is large and it needs more research. I will be reviewing it
> and will give another notes later.

This thread has not had a response from the authors or new patch in more
than two weeks.  Please post something soon or it will be marked
"returned with feedback".

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] jsonb array-style subscription

[David Steele]

On 3/20/16 2:29 PM, Dmitry Dolgov wrote:

 > Do you have an updated patch ready?

No, I'm afraid it will not be ready for Monday.


I have marked this "returned with feedback".  Please feel free to submit 
a reworked patch for 9.7!


--
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

Hi Magnus,

On 3/19/16 8:15 AM, Magnus Hagander wrote:

> I've attached an updated patch, which is rebased on current master and
> includes the oid fix.

Before doing a thorough review of this patch there are a few points I
would like to consider:

* I think it's really important to provide the stop time in some fashion
when using this new technique.  I would prefer a new column to be
returned from pg_stop_backup() but I could live with STOP TIME being
recorded in the label file.  STOP TIME should probably be included in
the label file anyway.

* It seems like STOP WAL LOCATION should now also be recorded in the
label file.  Preferably this would used by recovery to determine when
the database has reach consistency but that could be a future patch.
I'm not very happy with the current method of using pg_control to get
this information as it assumes that pg_control is copied last (at least
based on the code comments).

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 3/22/16 12:14 PM, Magnus Hagander wrote:
> On Tue, Mar 22, 2016 at 5:08 PM, David Steele  <mailto:da...@pgmasters.net>> wrote:
> 
> On 3/19/16 8:15 AM, Magnus Hagander wrote:
> 
> > I've attached an updated patch, which is rebased on current master and
> > includes the oid fix.
> 
> Before doing a thorough review of this patch there are a few points I
> would like to consider:
> 
> * I think it's really important to provide the stop time in some fashion
> when using this new technique.  I would prefer a new column to be
> returned from pg_stop_backup() but I could live with STOP TIME being
> recorded in the label file.  STOP TIME should probably be included in
> the label file anyway.
> 
> Adding the stop time column should be a simple addition and I don't see
> a problem with that. I think I misunderstood your original request on
> that. Because you are just talking about returning a timestamptz with
> the "right now" value for when you called pg_stop_backup()? Or to be
> specific, just before pg_Stop_backup *finished*. Or do you mean when
> pg_stop_backup() started?

What would be ideal is the minimum time that could be used for PITR.  In
an exclusive backup that's the time the end-of-backup record is written
to WAL.  In a non-exlusive backup I'm not quite sure how that works.

> Doing it in the backup label file is obviously a different target, where
> we might need to consider backwards compatibility, Should we?

Physical backups can only be restored in the same version so I'm not
sure why it would be a problem?  Do you mean for programs outside of
Postgres that are parsing this file?

> * It seems like STOP WAL LOCATION should now also be recorded in the
> label file.  Preferably this would used by recovery to determine when
> the database has reach consistency but that could be a future patch.
> I'm not very happy with the current method of using pg_control to get
> this information as it assumes that pg_control is copied last (at least
> based on the code comments).
> 
> That seems entirely out of scope for this patch, though. Doesn't mean it
> shouldn't be done, but that's a separate thing.

Fair enough.

-- 
-David
da...@pgmasters.net


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

[David Steele]

On 3/15/16 3:42 AM, Peter Geoghegan wrote:
> On Tue, Mar 15, 2016 at 12:31 AM, Amit Langote
>  wrote:
>> Ah, I see the nuance.  Thanks for the explanation.  Maybe,
>> bt_index_check() and bt_index_parent_child_check() /
>> bt_index_check_parent_child().  IMHO, the latter more clearly highlights
>> the fact that parent/child relationships in the form of down-links are
>> checked.
> 
> Well, the downlink is in the parent, because there is no such thing as
> an "uplink". So I prefer bt_index_parent_check(), since it usefully
> hints at starting from the parent. It's also more concise.
> 
>> By the way, one request (as a non-native speaker of English language, who
>> ends up looking up quite a few words regularly) -
>>
>> Could we use "conform" or "correspond" instead of "comport" in the
>> following error message:
>>
>> "left link/right link pair in index \"%s\" don't comport"
> 
> OK. I'll do something about that.

It looks like an updated patch is expected here, though it seems that
the only requests are for updates to comments.

-- 
-David
da...@pgmasters.net


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

[David Steele]

On 3/15/16 10:01 PM, Kyotaro HORIGUCHI wrote:

> Ok, I understand that this is not an issue in a hurry. I'll go to
> another patch that needs review.

Since we're getting towards the end of the CF is it time to pick this up
again?

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent 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_ctl promote wait

[David Steele]

On 3/16/16 12:19 PM, David Steele wrote:
> Hi Peter,
> 
> On 3/9/16 3:08 PM, Michael Paquier wrote:
> 
>> Here are some comments about 0002
> <...>
>> I think that we had better do something like the attached first.
>> Thoughts?
> 
> It's been a week since Michael reviewed this patch.  Could you please
> comment on his proposed changes as soon as possible?

Bump.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[David Steele]

Hi Kyotaro,

On 3/18/16 3:22 AM, Pavel Stehule wrote:

> I am looking this patch. It looks well, but this feature doesn't
> respect upper or lower chars. It enforce upper chars. This is not
> consistent with any other autocomplete.
> 
> 
> I checked it against sql help and these statements doesn't work
> 
> alter foreign table  drop column
> alter text search configuration jjj drop mapping
> alter type hhh drop attribute
> drop cast
> drop extension
> drop operator
> drop text search
> drop transform -- missing autocomplete completely
> drop user mapping
> alter table jjj add column
> create temp sequence
> create sequence

Do you have an idea of when you will have a new patch ready?

Thanks,
-- 
-David
da...@pgmasters.net


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

[David Steele]

On 3/22/16 8:54 PM, Michael Paquier wrote:
> On Wed, Mar 23, 2016 at 9:52 AM, Michael Paquier
>  wrote:
>> On Wed, Mar 23, 2016 at 1:38 AM, David Steele  wrote:
>>> On 3/15/16 10:01 PM, Kyotaro HORIGUCHI wrote:
>>>
>>>> Ok, I understand that this is not an issue in a hurry. I'll go to
>>>> another patch that needs review.
>>>
>>> Since we're getting towards the end of the CF is it time to pick this up
>>> again?
>>
>> Perhaps not. This is a legit bug with an unfinished patch (see index
>> relation truncation) that is going to need a careful review. I don't
>> think that this should be impacted by the 4/8 feature freeze, so we
>> could still work on that after the embargo and we've had this bug for
>> months actually. FWIW, I am still planning to work on it once the CF
>> is done, in order to keep my manpower focused on actual patch reviews
>> as much as possible...
> 
> In short, we may want to bump that to next CF... I have already marked
> this ticket as something to work on soonish on my side, so it does not
> change much seen from here if it's part of the next CF. What we should
> just be sure is not to lose track of its existence.

I would prefer not to bump it to the next CF unless we decide this will
not get fixed for 9.6.

-- 
-David
da...@pgmasters.net


-- 
Sent 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 v8] GSSAPI encryption support

[David Steele]

On 3/20/16 12:09 AM, Robbie Harwood wrote:
> Hello friends,
> 
> A new version of my GSSAPI encryption patchset is available, both in
> this email and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt8

Excellent, Robbie!  I've run this patch through my test cases and
everything works.

Now that it's working I'll be writing up an actual review so expect that
by Monday.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] POC, WIP: OR-clause support for indexes

[David Steele]

Hi Teador,

On 3/19/16 8:44 PM, Tomas Vondra wrote:


Sadly the v4 does not work for me - I do get assertion failures.


Time is growing short and there seem to be some serious concerns with 
this patch.  Can you provide a new patch soon?  If not, I think it might 
be be time to mark this "returned with feedback".


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] unexpected result from to_tsvector

[David Steele]

Hi Artur,

On 3/20/16 10:42 AM, Tom Lane wrote:

"Shulgin, Oleksandr"  writes:

On Mar 20, 2016 01:09, "Dmitrii Golub"  wrote:

Alex, actually subdomain can start with digit,



Not according to the RFC you have linked to.


The powers-that-be relaxed that some time ago; I assume there's a newer
RFC.  For instance, "163.com" is a real domain:


You marked this patch "needs review" and then a few minutes later 
changed it to "waiting on author".


If this was a mistake please change it back to "needs review".  If you 
really are working on a new patch when can we expect that?


Thanks,
--
-David
da...@pgmasters.net


--
Sent 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] Phrase search ported to 9.6

[David Steele]

Hi Dmitry,

On 3/16/16 12:38 PM, Dmitry Ivanov wrote:


I've made an attempt to fix some of the issues you've listed, although there's
still much work to be done. I'll add some comments later.


Do you know when you'll have a chance to respond to reviews and provide 
a new patch?


Time is short and it's not encouraging that you say there is "still much 
work to be done".  Perhaps it would be best to mark this "returned with 
feedback"?


Thanks,
--
-David
da...@pgmasters.net


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

[HACKERS] Re: WIP: Detecting SSI conflicts before reporting constraint violations

[David Steele]

Hi Thomas,

On 3/13/16 8:20 PM, Thomas Munro wrote:


<...>  I will have another look at this in
a few days but for now I need to do some other things, so I'm posting
these observations in case they are in some way helpful...


It's not clear to me what state this patch should be in but the thread 
has been idle for over a week.


Have you had the chance to take a look as you indicated above?

Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] Batch update of indexes

[David Steele]

On 3/14/16 10:37 AM, David Steele wrote:

On 3/14/16 10:28 AM, Konstantin Knizhnik wrote:


Rebased patch is attached.


Thanks for the quick turnaround!

Marko, you are signed up to review this patch.  Do you have an idea of
when you'll be able to do that?


Bump.

Since it looks like Marko has not had time to look at this would anyone 
else care to do a review?


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] Improving replay of XLOG_BTREE_VACUUM records

[David Steele]

Hi Vladimir,

On 3/14/16 2:15 PM, Vladimir Borodin wrote:


JFYI, I’m preparing the stand to reproduce the initial problem and I
hope to finish testing this week.


Do you know when you'll have the results from the testing you were going 
to do?  It seems this patch is currently waiting on that to be finished.


Thanks,
--
-David
da...@pgmasters.net


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