Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Tue, Sep 27, 2016 at 7:16 PM, Kyotaro HORIGUCHI
 wrote:
> I apologize in advance that the comments in this message might
> one of the ideas discarded in the past thread.. I might not grasp
> the discussion completely X(

No problem.

> At Wed, 18 May 2016 17:57:49 -0400, Michael Paquier 
>  wrote in 
> 
>
>> A couple of months back is has been reported to pgsql-bugs that WAL
>> segments were always switched with a low value of archive_timeout even
>> if a system is completely idle:
>> http://www.postgresql.org/message-id/20151016203031.3019.72...@wrigleys.postgresql.org
>> In short, a closer look at the problem has showed up that the logic in
>> charge of checking if a checkpoint should be skipped or not is
>> currently broken, because it completely ignores standby snapshots in
>> its calculation of the WAL activity. So a checkpoint always occurs
>> after checkpoint_timeout on an idle system since hot_standby has been
>> introduced as wal_level. This did not get better from 9.4, since
>> standby snapshots are logged every 15s by the background writer
>> process. In 9.6, since wal_level = 'archive' and 'hot_standby'
>> actually has the same meaning, the skip logic that worked with
>> wal_level = 'archive' does not do its job anymore.
>>
>> One solution that has been discussed is to track the progress of WAL
>> activity when doing record insertion by being able to mark some
>> records as not updating the progress of WAL. Standby snapshot records
>> enter in this category, making the checkpoint skip logic more robust.
>> Attached is a patch implementing a solution for it, by adding in
>
> If I understand the old thread correctly, this still doesn't
> solve the main issue, excessive checkpoint and segment
> switching. The reason is the interaction between XLOG_SWITCH and
> checkpoint as mentioned there. I think we may treat XLOG_SWITCH
> as NO_PROGRESS, since we can recover to the lastest state without
> it. If it is not wrong, the second patch attached (v12-2) inserts
> XLOG_SWITCH as NO_PROGRESS and skips segment switching when no
> progress took place for the round.

Possibly. That's a second problem I did not want to tackle now. I was
going to study that more precisely after this set of patches gets
done. There is already enough complication in them, and they solve a
large portion of the problem.

>> WALInsertLock a new field that gets updated for each record to track
>> the LSN progress. This allows to reliably skip the generation of
>> standby snapshots in the bgwriter or checkpoints on an idle system.
>
> WALInsertLock doesn't seem to me to be a good place for
> progressAt even considering the discussion about adding few
> instructions (containing a branch) in the
> hot-section. BackgroundWriterMain blocks all running
> XLogInsertRecord every 200 ms, not 15 or 30 seconds (only for
> replica, though). If this is correct, the Amit's suggestion below
> will have more significance, and I rather agree with it. XLogCtl
> is more suitable place for progressAt for the case.

Based on my past look at the problem and memories, having a variable
in WALInsertLock allows use to not have to touch the hottest spinlock
code path in WAL insertion and PG: ReserveXLogInsertLocation(). I'd
rather still avoid that.

>> > Also, I think it is worth to once take the performance data for
>> > write tests (using pgbench 30 minute run or some other way) with
>> > minimum checkpoint_timeout (i.e 30s) to see if the additional locking
>> > has any impact on performance.  I think taking locks at intervals
>> > of 15s or 30s should not matter much, but it is better to be safe.
>>
>> I don't think performance will be impacted, but there are no reasons
>> to not do any measurements either. I'll try to get some graphs
>> tomorrow with runs on my laptop, mainly looking for any effects of
>> this patch on TPS when checkpoints show up.
>
> I don't think the impact is measurable on a laptop, where 2 to 4
> cores available at most.

Yeah, I couldn't either.. Still I would like to keep the hot spinlock
section as small as possible if we can.

>> Per discussion with Andres at PGcon, we decided that this is an
>> optimization, only for 9.7~ because this has been broken for a long
>> time. I have also changed XLogIncludeOrigin() to use a more generic
>> routine to set of status flags for a record being inserted:
>> XLogSetFlags(). This routine can use two flags:
>> - INCLUDE_ORIGIN to decide if the origin should be logged or not
>> - NO_PROGRESS to decide at insertion if a record should update the LSN
>> progress or not.
>> Andres mentioned me that we'd want to have something similar to
>> XLogIncludeOrigin, but while hacking I noticed that grouping both
>> things under the same umbrella made more sense.
>
> This looks reasonable.

Thanks. That would be fine as a first, independent patch, but that
would mean that XLogSetFlags has only only value, which is a bit
pointless so I grouped them. And this makes the exiting interface
cleaner as well f

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Wed, Sep 28, 2016 at 6:12 AM, David Steele  wrote:
> I tried the attached patch set and noticed an interesting behavior. With
> archive_timeout=5 whenever I made a change I would get a WAL segment within
> a few seconds as expected then another one would follow a few minutes later.

That's intentional. We may be able to make XLOG_SWITCH records as not
updating the progress LSN, but I wanted to tackle that as a separate
patch once we got the basics done correctly, which is still what I
think this patch is doing. I should have been more precise upthread:
this patch makes the handling of checkpoint skip logic correct for
only standby snapshots, not segment switches, and puts the infra to
handle other things.
-- 
Michael


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

Re: [HACKERS] Showing parallel status in \df+

[Rushabh Lathia]

On Mon, Sep 26, 2016 at 3:06 PM, Stephen Frost  wrote:
> I feel like we're getting wrapped around the axle as it regards who is
> perceived to be voting for what.

Thanks Stephen Frost for listing down all the concerns from the people
on the different approaches.

On Tue, Sep 27, 2016 at 7:56 PM, Stephen Frost  wrote:

> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > I think the debate is more about whether moving the source display
> > functionality over to \sf is a better solution than rearranging \df+
> > output.  (If we had consensus to do that, I'd be happy to go code it,
> > but I'm not going to invest the effort when it seems like we don't.)
>
> Right, that's the main question.
>
> > If we'd had \sf all along, I think it's likely that we would never
> > have put source-code display into \df.  But of course we didn't,
>
> Indeed.
>
> > and what would have been best in a green field is not necessarily
> > what's best or achievable given existing reality.  Both Robert and
> > Peter have put forward the argument that people are used to finding
> > this info in \df+ output, and I think that deserves a whole lot of
> > weight.  The \sf solution might be cleaner, but it's not so much
> > better that it can justify forcing people to relearn their habits.
> >
> > So I think that rearranging \df+ output is really what we ought to
> > be doing here.
>
> Alright, given that Robert's made it clear what his preference is and
> you're in agreement with that, I'll remove my objection to moving down
> that path.  I agree that it's better than the current situation.  If we
> do end up improving \sf (which seems like a good idea, in general), then
> we may wish to consider a display option to control if the source is
> included in \df+ or not, but that doesn't need to bar this patch from
> going in.
>
> The earlier comments on the thread hadn't been as clear with regard to
> who held what opinions regarding the options and I'm glad that we were
> able to reach a point where it was much clearer that there was strong
> support for keeping the source in \df+.
>
> > I'm not necessarily wedded to any of the precise details of what I did
> > in my patch --- for instance, maybe function bodies ought to be indented
> > one tab stop?  But we've not gotten to the merits of such points, for
> > lack of agreement about whether this is the basic approach to take.
>
> As for this, I wouldn't indent or change the source at all.  For
> starters, indentation actually matters for some PLs, and I can certainly
> see people wanting to be able to copy/paste from the output, now that
> it'll be possible to reasonably do from the \df+ output.
>


Yes, it seems like "source code" as part of \df+ output (irrespective of
language) is still very much useful for the people - it make sense
not to change it at all. Also agree with Stephen view that once we do
end up improving \sf - we may be re-consider removing source code
from the \df+ output. For now we should stick with the goal for a thread
that started out being about showing parallel status in \df+ output.



> Thanks!
>
> Stephen
>



-- 
Rushabh Lathia
www.EnterpriseDB.com

Re: [HACKERS] Floating point comparison inconsistencies of the geometric types

[Emre Hasegeli]

> Emre, are you going to address the above?  It would have to be Real
> Soon Now.

Yes, I am working on it.  I found more problems, replaced more
algorithms.  That took a lot of time.  I will post the new version
really soon.  I wouldn't feel bad, if you wouldn't have enough time to
review it in this commitfest.


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

Re: [HACKERS] Bug in to_timestamp().

[amul sul]

On Fri, Sep 16, 2016 at 10:01 PM, Artur Zakirov
 wrote:
> On 25.08.2016 13:26, amul sul wrote:
>>>
>>> Thanks. I've created the entry in
>>> https://commitfest.postgresql.org/10/713/
>>> . You can add yourself as a reviewer.
>>>
>>
>> Done, added myself as reviewer & changed status to "Ready for
>> Committer". Thanks !
>>
>> Regards,
>> Amul Sul
>>
>>
>
> It seems there is no need to rebase patches. But I will not be able to fix
> patches for two weeks because of travel if someone will have a note or
> remark.
>
> So it would be nice if someone will be able to fix possible issues for above
> reasone.

Sure, Ill do that, if required.

Regards,
Amul


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

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Michael Paquier]

On Sat, Sep 24, 2016 at 5:37 PM, Masahiko Sawada  wrote:
> I still vote for changing behaviour of existing syntax 'k (n1, n2)' to
> quorum commit.
> That is,
> 1.  'First k (n1, n2, n3)' means that the master server waits for ACKs
> from k standby servers whose name appear earlier in the list.
> 2.  'Any k (n1, n2, n3)' means that the master server waits for ACKs
> from any k listed standby servers.
> 3.  'n1, n2, n3' is the same as #1 with k=1.
> 4.  '(n1, n2, n3)' is the same as #2 with k=1.

OK, so I have done a review of this patch keeping that in mind as
that's the consensus. I am still getting familiar with the code...

-transactions will wait until all the standby servers which are considered
+transactions will wait until the multiple standby servers which
are considered
There is no real need to update this sentence.

+FIRST means to control the standby servers with
+different priorities. The synchronous standbys will be those
+whose name appear earlier in this list, and that are both
+currently connected and streaming data in real-time(as shown
+by a state of streaming in the
+
+pg_stat_replication view). Other standby
+servers appearing later in this list represent potential
+synchronous standbys. If any of the current synchronous
+standbys disconnects for whatever reason, it will be replaced
+immediately with the next-highest-priority standby.
+For example, a setting of FIRST 3 (s1, s2, s3, s4)
+makes transaction commits wait until their WAL records are received
+by three higher-priority standbys chosen from standby servers
+s1, s2, s3 and s4.
It does not seem necessary to me to enter in this level of details:
The keyword FIRST, coupled with an integer number N, chooses the first
N higher-priority standbys and makes transaction commit when their WAL
records are received. For example FIRST 3 (s1, s2, s3, s4)
makes transaction commits wait until their WAL records are received by
the three high-priority standbys chosen from standby servers s1, s2,
s3 and s4.

+ANY means to control all of standby servers with
+same priority. The master sever will wait for receipt from
+at least num_sync
+standbys, which is quorum commit in the literature. The all of
+listed standbys are considered as candidate of quorum commit.
+For example, a setting of  ANY 3 (s1, s2, s3, s4) makes
+transaction commits wait until receiving receipts from at least
+any three standbys of four listed servers s1,
+s2, s3, s4.

Similarly, something like that...
The keyword ANY, coupled with an integer number N, chooses N standbys
in a set of standbys with the same, lowest, priority and makes
transaction commit when WAL records are received those N standbys. For
example ANY 3(s1, s2, s3, s4) makes transaction commits wait until WAL
records have been received from 3 servers in the set s1, s2, s3 and
s4.

It could be good also to mention that no keyword specified means ANY,
which is incompatible with 9.6. The docs also miss the fact that if a
simple list of servers is given, without parenthesis and keywords,
this is equivalent to FIRST 1.

-synchronous_standby_names = '2 (s1, s2, s3)'
+synchronous_standby_names = 'First 2 (s1, s2, s3)'
Nit here. It may be a good idea to just use upper-case characters in
the docs, or just lower-case for consistency, but not mix both.
Usually GUCs use lower-case characters.

+  when standby is considered as a condidate of quorum commit.
s/condidate/candidate/

-syncrep_scanner.c: FLEXFLAGS = -CF -p
+syncrep_scanner.c: FLEXFLAGS = -CF -p -i
Hm... Is that actually a good idea? Now "NODE" and "node" are two
different things for application_name, but with this patch both would
have the same meaning. I am getting to think that we could just use
the lower-case characters for the keywords any/first. Is this -i
switch a problem for elements in standby_list?

+ * Calculate the 'pos' newest Write, Flush and Apply positions among
sync standbys.
I don't understand this comment.

+   if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY)
+   got_recptr = SyncRepGetOldestSyncRecPtr(&writePtr, &flushPtr,
+&applyPtr, &am_sync);
+   else /* SYNC_REP_QUORUM */
+   got_recptr = SyncRepGetNNewestSyncRecPtr(&writePtr, &flushPtr,
+ &applyPtr,
SyncRepConfig->num_sync,
+ &am_sync);
Those could be grouped together, there is no need to have pos as an argument.

+   /* In quroum method, all sync standby priorities are always 1 */
+   if (found && SyncRepConfig->sync_method == SYNC_REP_QUORUM)
+   priority = 1;
This is dead code, SyncRepGetSyncStandbysPriority is not called for
QUORUM. You may want to add an assert in
SyncRepGetSyncStandbysPriority and SyncRepGetSyncStandbysQuorum to be
sure that 

Re: [HACKERS] Push down more full joins in postgres_fdw

[Etsuro Fujita]

On 2016/09/27 13:33, Ashutosh Bapat wrote:

I wrote:

ISTM that the use of the same RTI for subqueries in multi-levels in a
remote
SQL makes the SQL a bit difficult to read.  How about using the
position
of
the join rel in join_rel_list, (more precisely, the position plus
list_length(root->parse->rtable)), instead?


I wrote:

The join rel is appended to the end of the list, so I was thinking to get
the position info by list_length during postgresGetForeignJoinPaths.



That's true only when the paths are being added to a newly created
joinrel. But that's not true always. We may add paths with different
joining order to an existing joinrel, in which case list_length would
not give its position. Am I missing something?


I think you are right, but postgresGetForeignJoinPaths only allows us to 
add a foreign join path to a newly created joinrel.  The reason is 
because that routine skips all its work after the first call for that 
joinrel, by checking to see if joinrel->fdw_private is not NULL.  So, I 
think it's reasonable to get the position by list_length in that routine.


Best regards,
Etsuro Fujita




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

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

[Heikki Linnakangas]

On 09/26/2016 09:02 AM, Michael Paquier wrote:

On Mon, Sep 26, 2016 at 2:15 AM, David Steele  wrote:

* [PATCH 3/8] Switch password_encryption to a enum

Does not apply on HEAD (98c2d3332):


Interesting, it works for me on da6c4f6.


For here on I used 39b691f251 for review and testing.
I seems you are keeping on/off for backwards compatibility, shouldn't
the default now be "md5"?

-#password_encryption = on
+#password_encryption = on  # on, off, md5 or plain


That sounds like a good idea, so switched this way.


Committed this patch in the series, to turn password_encryption GUC into 
an enum.


There was one bug in the patch: if a plaintext password was given with 
CREATE/ALTER USER foo PASSWORD 'bar', but password_encryption was 'md5', 
it would incorrectly pass PASSWORD_TYPE_MD5 to the check-password hook. 
That would limit the amount of checking that the hook can do. Fixed 
that. Also edited the docs and comments a little bit, hopefully for the 
better.


Once we get the main SCRAM patch in, we may want to remove the "on" 
alias altogether. We don't promise backwards-compatibility of config 
files or GUC values, and not many people set password_encryption=on 
explicitly anyway, since it's the default. But I kept it now, as there's 
no ambiguity on what "on" means, yet.


- Heikki



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

Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

[Greg Stark]

On Wed, Sep 28, 2016 at 7:40 AM, Piotr Stefaniak
 wrote:
> Not remembering the context, I was initially confused about what exactly
> supposedly needs to be done in order to have ASan support, especially
> since I've been using it for a couple of years without any kind of
> modifications. Having read the whole thread now, I assume the discussion
> is now about getting MSan support, since apparently it's been already
> concluded that not much is needed for getting ASan support:


The differnce between msan and asan is only related to whether
uninitialized reads are tracked. All other memory errors such as
reading past the end of an allocation or reading after a free are
tracked by both.

Without asan support in Postgres's memdebug.h asan will just track
whether you're using memory that is outside the memory that malloc has
handed Postgres. It doesn't know anything about whether that memory
has been returned by palloc or has since been pfree'd. Even the bounds
checking is not great since you could be reading from palloc's header
or from the bytes in the next palloc block that happened to be
returned by the same malloc (or another malloc if you're unlucky).

The support I added to memdebug.h called macros which call llvm
intrinsics to mark the memory malloc'd by Postgres as unusuable until
it's returned by palloc. Once it's returned by palloc it's marked
usable except for a "guard" byte at the end. Then pfree marks the
memory unusable again. This basically mimics the behaviour you would
get from asan if you were using malloc directly.

I added support for msan as well which is basically just one more
macro to make the distinction between usable but uninitialized memory
and usable and initialized memory. But I was unable to test it because
msan didn't work for me at all. This seems to be the way of things
with llvm. It's great stuff but there's always 10% that is broken
because there's some cool new thing that's better.

-- 
greg


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

Re: [HACKERS] Push down more full joins in postgres_fdw

[Etsuro Fujita]

On 2016/09/26 16:30, Etsuro Fujita wrote:

On 2016/09/13 14:17, Ashutosh Bapat wrote:



It won't remain minimal as the number of paths created increases,
increasing the number of times a query is deparsed. We deparse query
every time time we cost a path for a relation with use_remote_estimates
true. As we try to push down more and more stuff, we will create more
paths and deparse the query more time.



Also, that makes the interface better. Right now, in your patch, you
have changed the order of deparsing in the existing code, so that the
aliases are registered while deparsing FROM clause and before any Var
nodes are deparsed. If we create aliases at the time of path creation,
only once in GetForeignJoinPaths or GetForeignPaths as appropriate, that
would require less code churn and would save some CPU cycles as well.



Agreed.  Will fix.


Done.  Attached is an updated version of the patch.

I didn't create aliases at anytime.  Instead, I added a logic to get 
info about the alias to a given expression from reltarget->exprs for 
relations in a given join tree.  See isSubqueryExpr and 
getSubselectAliasInfo.


As proposed by you, the patch differentiates between a base relation 
alias and a subquery alias by using different prefixes "r" and "s", 
respectively.  Also, subquery aliases are indexed by RTI for baserels 
and the position in join_rel_list + the length of rtable for joinrels, 
as proposed upthread.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 102,107  typedef struct deparse_expr_cxt
--- 102,109 
  /* Handy macro to add relation name qualification */
  #define ADD_REL_QUALIFIER(buf, varno)	\
  		appendStringInfo((buf), "%s%d.", REL_ALIAS_PREFIX, (varno))
+ #define SS_TAB_ALIAS_PREFIX	"s"
+ #define SS_COL_ALIAS_PREFIX	"c"
  
  /*
   * Functions to determine whether an expression can be evaluated safely on
***
*** 152,164  static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod,
   deparse_expr_cxt *context);
  static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
  	   deparse_expr_cxt *context);
! static void deparseSelectSql(List *tlist, List **retrieved_attrs,
   deparse_expr_cxt *context);
  static void deparseLockingClause(deparse_expr_cxt *context);
  static void appendOrderByClause(List *pathkeys, deparse_expr_cxt *context);
  static void appendConditions(List *exprs, deparse_expr_cxt *context);
  static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
! 	RelOptInfo *joinrel, bool use_alias, List **params_list);
  
  
  /*
--- 154,175 
   deparse_expr_cxt *context);
  static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
  	   deparse_expr_cxt *context);
! static void deparseSelectSql(List *tlist,
!  List *remote_conds,
!  List **retrieved_attrs,
   deparse_expr_cxt *context);
  static void deparseLockingClause(deparse_expr_cxt *context);
  static void appendOrderByClause(List *pathkeys, deparse_expr_cxt *context);
  static void appendConditions(List *exprs, deparse_expr_cxt *context);
  static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
! 	  RelOptInfo *foreignrel, bool add_rel_alias,
! 	  List **params_list);
! static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
!    bool is_subquery, List **params_list);
! static void appendSubselectAlias(deparse_expr_cxt *context);
! static void getSubselectAliasInfo(Expr *node, int *tabno, int *colno,
! 	  RelOptInfo *foreignrel);
! static bool isSubqueryExpr(Expr *node, int *tabno, int *colno, RelOptInfo *foreignrel);
  
  
  /*
***
*** 780,796  deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *rel,
  	context.foreignrel = rel;
  	context.params_list = params_list;
  
! 	/* Construct SELECT clause and FROM clause */
! 	deparseSelectSql(tlist, retrieved_attrs, &context);
! 
! 	/*
! 	 * Construct WHERE clause
! 	 */
! 	if (remote_conds)
! 	{
! 		appendStringInfo(buf, " WHERE ");
! 		appendConditions(remote_conds, &context);
! 	}
  
  	/* Add ORDER BY clause if we found any useful pathkeys */
  	if (pathkeys)
--- 791,798 
  	context.foreignrel = rel;
  	context.params_list = params_list;
  
! 	/* Construct SELECT clause, FROM clause, and WHERE clause */
! 	deparseSelectSql(tlist, remote_conds, retrieved_attrs, &context);
  
  	/* Add ORDER BY clause if we found any useful pathkeys */
  	if (pathkeys)
***
*** 803,809  deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *rel,
  /*
   * Construct a simple SELECT statement that retrieves desired columns
   * of the specified foreign table, and append it to "buf".  The output
!  * contains just "SELECT ... FROM ".
   *
   * We also create an integer List of the columns being retrieved, which is
   * returned to *retrieved_attrs.
--- 805,811 
  /*
   *

[HACKERS] Transaction user id through logical decoding

[valeriof]

Hi all,
I'm developing a custom plugin to stream Postgres CDC changes to my client
application. One of the info the application needs is the user id of the
user who executed a certain transaction. I can see we have access to other
transaction info (xid, lsn, changed data) but apparently the user id is not
available.
Does anyone know if it is possible to extract this info in any way?

Thanks,
Valerio



--
View this message in context: 
http://postgresql.nabble.com/Transaction-user-id-through-logical-decoding-tp5923261.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] Password identifiers, protocol aging and SCRAM protocol

[Heikki Linnakangas]

On 09/26/2016 09:02 AM, Michael Paquier wrote:

* [PATCH 2/8] Move encoding routines to src/common/
>
> I wonder if it is confusing to have two of encode.h/encode.c.  Perhaps
> they should be renamed to make them distinct?

Yes it may be a good idea to rename that, like encode_utils.[c|h] for
the new files.


Looking at these encoding functions, the SCRAM protocol actually uses 
base64 for everything. The hex encoding is only used in the server, to 
encode the StoredKey and ServerKey in pg_authid. So we don't need that 
in the client. It would actually make sense to use base64 for the fields 
in pg_authid, too. Takes less space, and seems more natural for SCRAM 
anyway.


libpq actually has its own implementation of hex encoding and decoding 
already, in fe-exec.c. So if we wanted to use hex-encoding for 
something, we could use that, or if we moved the routines from 
src/backend/utils/encode.c, then we should try to reuse them for the 
purposes of fe-exec.c, too. And libpq already has an implementation of 
the 'escape' encoding, too, in fe-exec.c.  But as I said above, I don't 
think we need to touch any of that.


In summary, I think we only need to move the base64 routines to 
src/common. I'd prefer to be quite surgical in what we put in 
src/common, and avoid moving stuff that's not strictly required by both 
the server and the client.


- Heikki



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

[Heikki Linnakangas]

On 09/28/2016 12:53 PM, Heikki Linnakangas wrote:

On 09/26/2016 09:02 AM, Michael Paquier wrote:

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


I wonder if it is confusing to have two of encode.h/encode.c.  Perhaps
they should be renamed to make them distinct?

Yes it may be a good idea to rename that, like encode_utils.[c|h] for
the new files.


Looking at these encoding functions, the SCRAM protocol actually uses
base64 for everything.


Oh, one more thing. The SCRAM spec says:


The use of base64 in SCRAM is restricted to the canonical form with
no whitespace.


Our b64_encode routine does use whitespace, so we can't use it as is for 
SCRAM. As the patch stands, we might never output anything long enough 
to create linefeeds, but let's be tidy. The base64 implementation is 
about 100 lines of code, so perhaps we should just leave 
src/backend/utils/encode.c alone, and make a new copy of the base64 
routines in src/common.


- Heikki



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

[Heikki Linnakangas]

On 09/28/2016 12:53 PM, Heikki Linnakangas wrote:

On 09/26/2016 09:02 AM, Michael Paquier wrote:

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


I wonder if it is confusing to have two of encode.h/encode.c.  Perhaps
they should be renamed to make them distinct?

Yes it may be a good idea to rename that, like encode_utils.[c|h] for
the new files.


Looking at these encoding functions, the SCRAM protocol actually uses
base64 for everything.


Oh, one more thing. The SCRAM spec says:


The use of base64 in SCRAM is restricted to the canonical form with
no whitespace.


Our b64_encode routine does use whitespace, so we can't use it as is for 
SCRAM. As the patch stands, we might never output anything long enough 
to create linefeeds, but let's be tidy. The base64 implementation is 
about 100 lines of code, so perhaps we should just leave 
src/backend/utils/encode.c alone, and make a new copy of the base64 
routines in src/common.


- Heikki



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

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

[Daniel Verite]

Tomas Vondra wrote:

> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log 
> message, but with '%d' and not '%ld' (as needed after changing the type 
> to Size).

This could be %zu for the Size type. It's supported by elog().

However, it occurs to me that if we don't claim the 2GB..4GB range for
the CopyData message, because its Int32 length is not explicitly
unsigned as mentioned upthread, then it should follow that we don't
need to change StringInfo.{len,maxlen} from int type to Size type.

We should just set the upper limit for StringInfo.maxlen to
(0x7fff-1) instead of MaxAllocHugeSize for stringinfos with the
allow_long flag set, and leave the len and maxlen fields to their
original, int type.

Does that make sense?


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


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

[Michael Paquier]

On Wed, Sep 28, 2016 at 7:03 PM, Heikki Linnakangas  wrote:
> On 09/28/2016 12:53 PM, Heikki Linnakangas wrote:
>>
>> On 09/26/2016 09:02 AM, Michael Paquier wrote:

 * [PATCH 2/8] Move encoding routines to src/common/
>
>
> I wonder if it is confusing to have two of encode.h/encode.c.  Perhaps
> they should be renamed to make them distinct?
>>>
>>> Yes it may be a good idea to rename that, like encode_utils.[c|h] for
>>> the new files.
>>
>>
>> Looking at these encoding functions, the SCRAM protocol actually uses
>> base64 for everything.

OK, I thought that moving everything made more sense for consistency
but let's keep src/common/ as small as possible.

> Oh, one more thing. The SCRAM spec says:
>
>> The use of base64 in SCRAM is restricted to the canonical form with
>> no whitespace.
>
> Our b64_encode routine does use whitespace, so we can't use it as is for
> SCRAM. As the patch stands, we might never output anything long enough to
> create linefeeds, but let's be tidy. The base64 implementation is about 100
> lines of code, so perhaps we should just leave src/backend/utils/encode.c
> alone, and make a new copy of the base64 routines in src/common.

OK, I'll refresh that tomorrow with the rest. Thanks for the commit to
extend password_encryption.
-- 
Michael


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

[HACKERS] psql casts aspersions on server reliability

[Robert Haas]

psql tends to do things like this:

rhaas=# select * from pg_stat_activity;
FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

Basically everything psql has to say about this is a lie:

1. It says the server closed the connection unexpectedly, but it just
finished processing a FATAL message from the server.  So how
unexpected is it that the connection would thereafter be closed?

2.  It says the server probably terminated abnormally, but PostgreSQL
is rarely so unreliable as to just give up the ghost and die.  The
ErrorResponse it just processed has an errcode of
ERRCODE_ADMIN_SHUTDOWN, so probably what happened is somebody either
shut down the server or killed the session.

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


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

Re: [HACKERS] Tracking wait event for latches

[Robert Haas]

On Tue, Sep 27, 2016 at 8:39 PM, Thomas Munro
 wrote:
> Ok, if they really are independent then shouldn't we take advantage of
> that at call sites where we might be idle but we might also be waiting
> for the network?

I certainly didn't intend for them to be independent, and I don't
think they should be.  I think it should be a hierarchy - as it is
currently.  I think it's a bad idea to introduce the notational
overhead of having to pass through two integers rather than one
everywhere, and a worse idea to encourage people to think of the
wait_event_type and wait_event are related any way other than
hierarchically.

> Actually, I'm still not sold on "Activity" and "Client".  I think
> "Idle" and "Network" would be better.  Everybody knows intuitively
> what "Idle" means.  "Network" is better than "Client" because it
> avoids confusion about user applications vs replication connections or
> clients vs servers.

Hmm, I could live with that, if other people like it.

> s/auxilliary/auxiliary/, but I wouldn't it be better to say something
> more general like "from another process in the cluster"?  Background
> workers are not generally called auxiliary processes, and some of
> these wait points are waiting for those.

Agreed; or perhaps it could even be waiting for another regular backend.

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


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

Re: [HACKERS] Tracking wait event for latches

[Michael Paquier]

On Wed, Sep 28, 2016 at 9:35 PM, Robert Haas  wrote:
> On Tue, Sep 27, 2016 at 8:39 PM, Thomas Munro
>  wrote:
>> Ok, if they really are independent then shouldn't we take advantage of
>> that at call sites where we might be idle but we might also be waiting
>> for the network?
>
> I certainly didn't intend for them to be independent, and I don't
> think they should be.  I think it should be a hierarchy - as it is
> currently.  I think it's a bad idea to introduce the notational
> overhead of having to pass through two integers rather than one
> everywhere, and a worse idea to encourage people to think of the
> wait_event_type and wait_event are related any way other than
> hierarchically.

So should I change back the patch to have only one argument for the
eventId, and guess classId from it?
-- 
Michael


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

Re: [HACKERS] Tracking wait event for latches

[Robert Haas]

On Wed, Sep 28, 2016 at 8:38 AM, Michael Paquier
 wrote:
> On Wed, Sep 28, 2016 at 9:35 PM, Robert Haas  wrote:
>> On Tue, Sep 27, 2016 at 8:39 PM, Thomas Munro
>>  wrote:
>>> Ok, if they really are independent then shouldn't we take advantage of
>>> that at call sites where we might be idle but we might also be waiting
>>> for the network?
>>
>> I certainly didn't intend for them to be independent, and I don't
>> think they should be.  I think it should be a hierarchy - as it is
>> currently.  I think it's a bad idea to introduce the notational
>> overhead of having to pass through two integers rather than one
>> everywhere, and a worse idea to encourage people to think of the
>> wait_event_type and wait_event are related any way other than
>> hierarchically.
>
> So should I change back the patch to have only one argument for the
> eventId, and guess classId from it?

Why would you need to guess?  But, yes, I think one argument is much preferable.

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


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

[HACKERS] Python3.4 detection on 9.6 configuration

[Lou Picciano]

PostgreSQL Friends: 

Trying to build 9.6RC1, with Python3.4, on OpenIndiana (Illumos). It seems the 
detection of shared library status of the .so has changed. This appears to be 
related to a different(?) elucidation of python configuration. 

A 'hardwired' change to the configure script to trap platform 'solaris' will 
work, but this seems the inappropriate approach. 

Would be happy to work through this here - I'd like to make it a small 
'contribution'. 

Clipped from configure script: 



1. 
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking how to link an embedded 
Python application" >&5 

2. 
$as_echo_n "checking how to link an embedded Python application... " >&6; } 

3. 

4. 
python_libdir=`${PYTHON} -c "import distutils.sysconfig; print(' 
'.join(filter(None,distutils.sysconfig.get_config_vars('LIBDIR'"` 

5. 
python_ldlibrary=`${PYTHON} -c "import distutils.sysconfig; print(' 
'.join(filter(None,distutils.sysconfig.get_config_vars('LDLIBRARY'"` 

6. 
python_so=`${PYTHON} -c "import distutils.sysconfig; print(' 
'.join(filter(None,distutils.sysconfig.get_config_vars('SO'"` 

7. 
echo "-" 

8. 
echo "- LOU MOD: python_so: $python_so" 

9. 
echo "-" 

10. 

11. 
configure finds '.so' on Python2.7 

12. 
configure finds '.cpython-34m.so' on Python3.4 

13. 

14. 
- LATER in the config script, the following 'hardwired' change will 
'fix' this, but is likely not the right approach: 

15. 
(our Python _is_ built as a shared lib. So, this is wonky, but it will work: ) 

16. 

17. 
# We need libpython as a shared library. With Python >=2.5, we 

18. 
# check the Py_ENABLE_SHARED setting. On Debian, the setting is not 

19. 
# correct before the jessie release (http://bugs.debian.org/695979). 

20. 
# We also want to support older Python versions. So as a fallback 

21. 
# we see if there is a file that is named like a shared library. 

22. 

23. 
if test "$python_enable_shared" != 1; then 

24. 
if test "$PORTNAME" = darwin; then 

25. 
# macOS does supply a .dylib even though Py_ENABLE_SHARED does 

26. 
# not get set. The file detection logic below doesn't succeed 

27. 
# on older macOS versions, so make it explicit. 

28. 
python_enable_shared=1 

29. 
elif test "$PORTNAME" = win32; then 

30. 
# Windows also needs an explicit override. 

31. 
python_enable_shared=1 

32. 
# - MOD BY LOU:  

33. 
elif test "$PORTNAME" = solaris; then 

34. 
# Solaris explicit override. 

35. 
python_enable_shared=1 

36. 
else 

37. 
# We don't know the platform shared library extension here yet, 

38. 
# so we try some candidates. 

39. 
for dlsuffix in .so .sl; do 

40. 
if ls "$python_libdir"/libpython*${dlsuffix}* >/dev/null 2>&1; then 

41. 
python_enable_shared=1 

42. 
break 

43. 
fi 

44. 
done 

45. 
fi 

46. 
fi 

Re: [HACKERS] Logical Replication WIP

[Peter Eisentraut]

On 9/23/16 9:28 PM, Petr Jelinek wrote:
>> Document to what extent other relation types are supported (e.g.,
>> > materialized views as source, view or foreign table or temp table as
>> > target).  Suggest an updatable view as target if user wants to have
>> > different table names or write into a different table structure.
>> > 
> I don't think that's good suggestion, for one it won't work for UPDATEs
> as we have completely different path for finding the tuple to update
> which only works on real data, not on view. I am thinking of even just
> allowing table to table replication in v1 tbh, but yes it should be
> documented what target relation types can be.

I'll generalize this then to: Determine which relation types should be
supported at either end, document that, and then make sure it works that
way.  A restrictive implementation is OK for the first version, as long
as it keeps options open.

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



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

Re: [HACKERS] psql casts aspersions on server reliability

[Tom Lane]

Robert Haas  writes:
> psql tends to do things like this:
> rhaas=# select * from pg_stat_activity;
> FATAL:  terminating connection due to administrator command
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.

> Basically everything psql has to say about this is a lie:

I cannot get terribly excited about this.  What you seem to be proposing
is that psql try to intuit the reason for connection closure from the
last error message it got, but that seems likely to lead to worse lies
than printing a boilerplate message.

I could go along with just dropping the last sentence ("This probably...")
if the last error we got was FATAL level.  I don't find "unexpectedly"
to be problematic here: from the point of view of psql, and probably
of its user, the shutdown *was* unexpected.

regards, tom lane


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

Re: [HACKERS] Python3.4 detection on 9.6 configuration

[Tom Lane]

Lou Picciano  writes:
> Trying to build 9.6RC1, with Python3.4, on OpenIndiana (Illumos). It
> seems the detection of shared library status of the .so has changed.

Changed from what?  I don't recall that we've touched that code in quite
some time.  What was the last version that worked for you?  What
exactly is failing?

I'm having a hard time following your not-really-a-patch, but it looks
like you're proposing forcing python_enable_shared=1 on Solaris, which
sounds like a pretty bad idea.  AFAIK the shared-ness of libpython is
up to whoever built it.

regards, tom lane


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

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

[David Steele]

On 9/28/16 5:25 AM, Heikki Linnakangas wrote:
> 
> Once we get the main SCRAM patch in, we may want to remove the "on"
> alias altogether. We don't promise backwards-compatibility of config
> files or GUC values, and not many people set password_encryption=on
> explicitly anyway, since it's the default.

+1.

-- 
-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] Showing parallel status in \df+

[Tom Lane]

Rushabh Lathia  writes:
> On Mon, Sep 26, 2016 at 3:06 PM, Stephen Frost  wrote:
>> I feel like we're getting wrapped around the axle as it regards who is
>> perceived to be voting for what.

> Thanks Stephen Frost for listing down all the concerns from the people
> on the different approaches.

I'm not sure if we've arrived at a consensus or not, but here's my
current thinking: it's very early in the v10 cycle, so we have time
to experiment.  I propose to push my current patch (ie, move PL function
source code to \df+ footers), and we can use it in HEAD for awhile
and see what we think.  We can alway improve or revert it later.

regards, tom lane


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

Re: [HACKERS] psql casts aspersions on server reliability

[Robert Haas]

On Wed, Sep 28, 2016 at 9:14 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> psql tends to do things like this:
>> rhaas=# select * from pg_stat_activity;
>> FATAL:  terminating connection due to administrator command
>> server closed the connection unexpectedly
>> This probably means the server terminated abnormally
>> before or while processing the request.
>
>> Basically everything psql has to say about this is a lie:
>
> I cannot get terribly excited about this.  What you seem to be proposing
> is that psql try to intuit the reason for connection closure from the
> last error message it got, but that seems likely to lead to worse lies
> than printing a boilerplate message.
>
> I could go along with just dropping the last sentence ("This probably...")
> if the last error we got was FATAL level.  I don't find "unexpectedly"
> to be problematic here: from the point of view of psql, and probably
> of its user, the shutdown *was* unexpected.

I don't care very much whether we try to intuit the reason for
connection closure or not; it could be done, but I don't feel that it
has to be done.  My bigger point is that currently psql speculates
that the reason for *every* connection closure is abnormal server
termination, which is actually a very rare event.

It may have been common when that message was added.
1a17447be1186fdd36391c58a2a0209f613d89c4 changed the wording this
message in 2001, and the original message seems to date to
011ee13131f6fa2f6dbafd3827b70d051cb28f64 in 1996.  And my guess is at
that time the server probably did just roll over and die with some
regularity.  But today it usually doesn't.  It's neither helpful nor
good PR for libpq to guess that the most likely cause of a server
disconnection is server unreliability.

I have seen actual instances of customers getting upset by this
message even though the server had been shut down quite cleanly.  The
message got into a logfile and induced minor panic.  Fortunately, I
have not seen this happen lately.

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


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

Re: [HACKERS] Showing parallel status in \df+

[Pavel Stehule]

2016-09-28 16:03 GMT+02:00 Tom Lane :

> Rushabh Lathia  writes:
> > On Mon, Sep 26, 2016 at 3:06 PM, Stephen Frost 
> wrote:
> >> I feel like we're getting wrapped around the axle as it regards who is
> >> perceived to be voting for what.
>
> > Thanks Stephen Frost for listing down all the concerns from the people
> > on the different approaches.
>
> I'm not sure if we've arrived at a consensus or not, but here's my
> current thinking: it's very early in the v10 cycle, so we have time
> to experiment.  I propose to push my current patch (ie, move PL function
> source code to \df+ footers), and we can use it in HEAD for awhile
> and see what we think.  We can alway improve or revert it later.
>

I had some objection to format of source code - it should be full source
code, not just header and body.

Regards

Pavel

>
> regards, tom lane
>

Re: [HACKERS] Python3.4 detection on 9.6 configuration

[Lou Picciano]

- Original Message -

> From: "Tom Lane"  
> To: "Lou Picciano"  
> Cc: pgsql-hackers@postgresql.org 
> Sent: Wednesday, September 28, 2016 9:33:06 AM 
> Subject: Re: [HACKERS] Python3.4 detection on 9.6 configuration 

> Lou Picciano  writes: 
> > Trying to build 9.6RC1, with Python3.4, on OpenIndiana (Illumos). It 
> > seems the detection of shared library status of the .so has changed. 

Specifically, configure is not finding the .so. It's not that the so isn't 
built 'shared'; it is. 

> Changed from what? I don't recall that we've touched that code in quite 
> some time. What was the last version that worked for you? What 
> exactly is failing? 

Core bit seems to be the python3.4-config behavior: 


/usr/bin/python3.4-config --extension-suffix 

.cpython-34m.so 
I don't know if this is designed behavior for Python3.4 - or if it's a bug 
there? I'm working this with the Python gang as well. 

Of course, this option doesn't exist under Python2.7. 

> I'm having a hard time following your not-really-a-patch, but it looks 
> like you're proposing forcing python_enable_shared=1 on Solaris, 

Certainly not! I was simply offering this as evidence that PostgreSQL will 
build just fine, against Python3.4, using this hack. (It's useful in getting us 
a working build in situ o continue other testing - even before the more elegant 
fix - whatever that turns out to be!) 

> which sounds like a pretty bad idea. AFAIK the shared-ness of libpython is 
> up to whoever built it. 

Indeed. As I mentioned, our Python3.4 is built shared. 

> regards, tom lane 

[HACKERS] Handling dropped attributes in pglogical_proto

[Konstantin Knizhnik]

Hi,

pglogical_read_tuple from pglogical_proto.c contains the following code:

natts = pq_getmsgint(in, 2);
if (rel->natts != natts)
elog(ERROR, "tuple natts mismatch, %u vs %u", rel->natts, 
natts);



But if table was just altered and some attribute was removed from the 
table, then rel->natts can be greater than natts.

The problem can be fixed by the following patch:

--- a/pglogical_proto.c
+++ b/pglogical_proto.c
@@ -263,10 +263,15 @@ pglogical_read_tuple(StringInfo in, PGLogicalRelation 
*rel,
{
int attid = rel->attmap[i];
Form_pg_attribute att = desc->attrs[attid];
-   charkind = pq_getmsgbyte(in);
+   charkind;
const char *data;
int len;
 
+   if (att->atttypid == InvalidOid) {

+   continue;
+   }
+   kind = pq_getmsgbyte(in);
+
switch (kind)
{
case 'n': /* null */


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



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

Re: [HACKERS] psql casts aspersions on server reliability

[David Steele]

On 9/28/16 10:22 AM, Robert Haas wrote:
> On Wed, Sep 28, 2016 at 9:14 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> psql tends to do things like this:
>>> rhaas=# select * from pg_stat_activity;
>>> FATAL:  terminating connection due to administrator command
>>> server closed the connection unexpectedly
>>> This probably means the server terminated abnormally
>>> before or while processing the request.
>>
>>> Basically everything psql has to say about this is a lie:
>>
>> I cannot get terribly excited about this.  What you seem to be proposing
>> is that psql try to intuit the reason for connection closure from the
>> last error message it got, but that seems likely to lead to worse lies
>> than printing a boilerplate message.
>>
>> I could go along with just dropping the last sentence ("This probably...")
>> if the last error we got was FATAL level.  I don't find "unexpectedly"
>> to be problematic here: from the point of view of psql, and probably
>> of its user, the shutdown *was* unexpected.
> 
> I don't care very much whether we try to intuit the reason for
> connection closure or not; it could be done, but I don't feel that it
> has to be done.  My bigger point is that currently psql speculates
> that the reason for *every* connection closure is abnormal server
> termination, which is actually a very rare event.
> 
> It may have been common when that message was added.
> 1a17447be1186fdd36391c58a2a0209f613d89c4 changed the wording this
> message in 2001, and the original message seems to date to
> 011ee13131f6fa2f6dbafd3827b70d051cb28f64 in 1996.  And my guess is at
> that time the server probably did just roll over and die with some
> regularity.  But today it usually doesn't.  It's neither helpful nor
> good PR for libpq to guess that the most likely cause of a server
> disconnection is server unreliability.
> 
> I have seen actual instances of customers getting upset by this
> message even though the server had been shut down quite cleanly.  The
> message got into a logfile and induced minor panic.  Fortunately, I
> have not seen this happen lately.

+1 for making this error message less frightening.  I have also had to
explain it away on occasion.

-- 
-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] Tuplesort merge pre-reading

[Peter Geoghegan]

On Thu, Sep 15, 2016 at 9:51 PM, Heikki Linnakangas  wrote:
>> I still don't get why you're doing all of this within mergeruns() (the
>> beginning of when we start merging -- we merge all quicksorted runs),
>> rather than within beginmerge() (the beginning of one particular merge
>> pass, of which there are potentially more than one). As runs are
>> merged in a non-final merge pass, fewer tapes will remain active for
>> the next merge pass. It doesn't do to do all that up-front when we
>> have multiple merge passes, which will happen from time to time.
>
>
> Now that the pre-reading is done in logtape.c, it doesn't stop at a run
> boundary. For example, when we read the last 1 MB of the first run on a
> tape, and we're using a 10 MB read buffer, we will merrily also read the
> first 9 MB from the next run. You cannot un-read that data, even if the tape
> is inactive in the next merge pass.

I've had a chance to think about this some more. I did a flying review
of the same revision that I talk about here, but realized some more
things. First, I will do a recap.

> I don't think it makes much difference in practice, because most merge
> passes use all, or almost all, of the available tapes. BTW, I think the
> polyphase algorithm prefers to do all the merges that don't use all tapes
> upfront, so that the last final merge always uses all the tapes. I'm not
> 100% sure about that, but that's my understanding of the algorithm, and
> that's what I've seen in my testing.

Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.

FWIW, polyphase merge actually doesn't distribute runs based on the
Fibonacci sequence (at least in Postgres). It uses a generalization of
the Fibonacci numbers [1] -- *which* generalization varies according
to merge order/maxTapes. IIRC, with Knuth's P == 6 (i.e. merge order
== 6), it's the "hexanacci" sequence.

The following code is from your latest version, posted on 2016-09-14,
within the high-level mergeruns() function (the function that takes
quicksorted runs, and produces final output following 1 or more merge
passes):

> +   usedBlocks = 0;
> +   for (tapenum = 0; tapenum < state->maxTapes; tapenum++)
> +   {
> +   int64   numBlocks = blocksPerTape + (tapenum < remainder ? 1 : 0);
> +
> +   if (numBlocks > MaxAllocSize / BLCKSZ)
> +   numBlocks = MaxAllocSize / BLCKSZ;
> +   LogicalTapeAssignReadBufferSize(state->tapeset, tapenum,
> +   numBlocks * BLCKSZ);
> +   usedBlocks += numBlocks;
> +   }
> +   USEMEM(state, usedBlocks * BLCKSZ);

I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).

It's weird that you change the meaning of maxTapes by reassigning in
advance of iterating through maxTapes like this. I should point out
again that the master branch mergebatch() function does roughly the
same thing as you're doing here as follows:

static void
mergebatch(Tuplesortstate *state, int64 spacePerTape)
{
int srcTape;

Assert(state->activeTapes > 0);
Assert(state->tuples);

/*
 * For the purposes of tuplesort's memory accounting, the batch allocation
 * is special, and regular memory accounting through USEMEM() calls is
 * abandoned (see mergeprereadone()).
 */
for (srcTape = 0; srcTape < state->maxTapes; srcTape++)
{
char   *mergetuples;

if (!state->mergeactive[srcTape])
continue;

/* Allocate buffer for each active tape */
mergetuples = MemoryContextAllocHuge(state->tuplecontext,
 spacePerTape);

/* Initialize state for tape */
state->mergetuples[srcTape] = mergetuples;
state->mergecurrent[srcTape] = mergetuples;
state->mergetail[srcTape] = mergetuples;
state->mergeoverflow[srcTape] = NULL;
}

state->batchUsed = true;
state->spacePerTape = spacePerTape;
}

Notably, this function:

* Works without altering the meaning of maxTapes. state->maxTapes is
Knuth's T, which is established very early and doesn't change with
polyphase merge (same applies to state->tapeRange). What does change
across merge passes is the number of *active* tapes. I don't think
it's necessary to change that in any way. I find it very confusing.
(Also, that you're using state->currentRun here at all seems bad, for
more or less 

Re: [HACKERS] PATCH: Exclude additional directories in pg_basebackup

[David Steele]

On 9/28/16 2:45 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 11:27 PM, David Steele  wrote:
>> On 9/26/16 2:36 AM, Michael Paquier wrote:
>>
>>> Just a nit:
>>>  
>>> - postmaster.pid
>>> + postmaster.pid and postmaster.opts
>>>  
>>> Having one  block for each file would be better.
>>
>> OK, changed.
> 
> You did not actually change it :)

Oops, this was intended to mean that I changed:

>>> +const char *excludeFile[] =
>>> excludeFiles[]?

> +
> + backup_label and tablespace_map.  If these
> + files exist they belong to an exclusive backup and are not 
> applicable
> + to the base backup.
> +
> This is a bit confusing. When using pg_basebackup the files are
> ignored, right, but they are included in the tar stream so they are
> not excluded at the end. So we had better remove purely this
> paragraph. Similarly, postgresql.auto.conf.tmp is something that
> exists only for a short time frame. Not listing it directly is fine
> IMO.

OK, I agree that's confusing and probably not helpful to the user.

> +   /* If symlink, write it as a directory anyway */
> +#ifndef WIN32
> +   if (S_ISLNK(statbuf->st_mode))
> +#else
> +   if (pgwin32_is_junction(pathbuf))
> +#endif
> +
> +   statbuf->st_mode = S_IFDIR | S_IRWXU;
> Indentation here is confusing. Coverity would complain.

OK.

> +   /* Stat the file */
> +   snprintf(pathbuf, MAXPGPATH, "%s/%s", path, de->d_name);
> There is no need to put the stat() call this early in the loop as this
> is just used for re-creating empty directories.

OK.

> +if (strcmp(pathbuf + basepathlen + 1,
> +   excludeFiles[excludeIdx]) == 0)
> There is no need for complicated maths, you can just use de->d_name.

OK.

> pg_xlog has somewhat a similar treatment, but per the exception
> handling of archive_status it is better to leave its check as-is.

OK.

> The DEBUG1 entries are really useful for debugging, it would be nice
> to keep them in the final patch.

That was my thinking as well.  It's up to Peter, though.

> Thinking harder, your logic can be simplified. You could just do the 
> following:
> - Check for interrupts first
> - Look at the list of excluded files
> - Call lstat()
> - Check for excluded directories

I think setting excludeFound = false the second time is redundant since
it must be false at that point.  Am I missing something or are you just
being cautious in case code changes above?

> After all that fixed, I have moved the patch to "Ready for Committer".
> Please use the updated patch though.

The v6 patch looks good to me.

-- 
-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] assert violation in logical messages serialization

[Robert Haas]

On Tue, Sep 27, 2016 at 10:45 AM, Stas Kelvich  wrote:
> During processing of logical messages in ReorderBufferSerializeChange()
> pointer to ondisk structure isn’t updated after possible reorder buffer 
> realloc by
> ReorderBufferSerializeReserve().
>
> Actual reason spotted by Konstantin Knizhnik.

Thanks for the patch, and congratulations to Konstantin on the spot.
Committed and back-patched to 96.

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


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost  wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> This is now being obsoleted by the later idea of allowing base installs
> >> from a chain of upgrade scripts.  But if your upgrade scripts contain
> >> ALTER TABLE commands, you will probably still want to write base install
> >> scripts that do a fresh CREATE TABLE instead.
> >
> > I've updated the patch to remove the new base version script and to rely
> > on the changes made by Tom to install the 1.4 version and then upgrade
> > to 1.5.
> >
> > Based on my testing, it appears to all work correctly.
> 
> Same conclusion from here.

Fantastic, thanks for the testing!

> > Updated (much smaller) patch attached.
> 
> I had a look at it, and it is doing the work it claims to do. So I am
> marking it as "Ready for Committer".

Great.  I'm going to go over the whole patch closely again and will push
it soon.

Thanks again!

Stephen


signature.asc
Description: Digital signature

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

[Robert Haas]

On Tue, Sep 27, 2016 at 5:15 PM, Tomas Vondra
 wrote:
> So, I got the results from 3.10.101 (only the pgbench data), and it looks
> like this:
>
>  3.10.101   1  8 16 32 64128192
> 
>  granular-locking2582  18492  33416  49583  53759  53572  51295
>  no-content-lock 2580  18666  33860  49976  54382  54012  51549
>  group-update2635  18877  33806  49525  54787  54117  51718
>  master  2630  18783  33630  49451  54104  53199  50497
>
> So 3.10.101 performs even better tnan 3.2.80 (and much better than 4.5.5),
> and there's no sign any of the patches making a difference.

I'm sure that you mentioned this upthread somewhere, but I can't
immediately find it.  What scale factor are you testing here?

It strikes me that the larger the scale factor, the more
CLogControlLock contention we expect to have.  We'll pretty much do
one CLOG access per update, and the more rows there are, the more
chance there is that the next update hits an "old" row that hasn't
been updated in a long time.  So a larger scale factor also increases
the number of active CLOG pages and, presumably therefore, the amount
of CLOG paging activity.

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


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

[HACKERS] compiler warning read_objtype_from_string()

[Peter Eisentraut]

I'm getting the following compiler warning (using nondefault
optimization options):

objectaddress.c: In function 'read_objtype_from_string':
objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
  return type;

The comment for the function says

 * Return ObjectType for the given object type as given by
 * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
 * possible output type from getObjectTypeDescription, return -1.

But the claim that it can return -1 does not seem supported by the code.

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


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

Re: [HACKERS] Cache Hash Index meta page.

[Mithun Cy]

On Tue, Sep 27, 2016 at 1:53 AM, Jeff Janes  wrote:
 > I think that this needs to be updated again for v8 of concurrent and v5
of wal

Adding the rebased patch over [1] + [2]

[1] Concurrent Hash index.

[2] Wal for hash index.


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


cache_hash_index_metapage_onAmit_05_03_with_wall.patch
Description: Binary data

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

Re: [HACKERS] Tuplesort merge pre-reading

[Heikki Linnakangas]

On 09/28/2016 06:05 PM, Peter Geoghegan wrote:

On Thu, Sep 15, 2016 at 9:51 PM, Heikki Linnakangas  wrote:

I don't think it makes much difference in practice, because most merge
passes use all, or almost all, of the available tapes. BTW, I think the
polyphase algorithm prefers to do all the merges that don't use all tapes
upfront, so that the last final merge always uses all the tapes. I'm not
100% sure about that, but that's my understanding of the algorithm, and
that's what I've seen in my testing.


Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.


How does the distribution of the runs on the tapes matter?


+   usedBlocks = 0;
+   for (tapenum = 0; tapenum < state->maxTapes; tapenum++)
+   {
+   int64   numBlocks = blocksPerTape + (tapenum < remainder ? 1 : 0);
+
+   if (numBlocks > MaxAllocSize / BLCKSZ)
+   numBlocks = MaxAllocSize / BLCKSZ;
+   LogicalTapeAssignReadBufferSize(state->tapeset, tapenum,
+   numBlocks * BLCKSZ);
+   usedBlocks += numBlocks;
+   }
+   USEMEM(state, usedBlocks * BLCKSZ);


I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).


You can't release the tape buffer at the end of a pass, because the 
buffer of a tape will already be filled with data from the next run on 
the same tape.


- Heikki



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

Re: [HACKERS] Tuplesort merge pre-reading

[Peter Geoghegan]

On Wed, Sep 28, 2016 at 5:04 PM, Heikki Linnakangas  wrote:
>> Not sure that I understand. I agree that each merge pass tends to use
>> roughly the same number of tapes, but the distribution of real runs on
>> tapes is quite unbalanced in earlier merge passes (due to dummy runs).
>> It looks like you're always using batch memory, even for non-final
>> merges. Won't that fail to be in balance much of the time because of
>> the lopsided distribution of runs? Tapes have an uneven amount of real
>> data in earlier merge passes.
>
>
> How does the distribution of the runs on the tapes matter?

The exact details are not really relevant to this discussion (I think
it's confusing that we simply say "Target Fibonacci run counts",
FWIW), but the simple fact that it can be quite uneven is.

This is why I never pursued batch memory for non-final merges. Isn't
that what you're doing here? You're pretty much always setting
"state->batchUsed = true".

>> I'm basically repeating myself here, but: I think it's incorrect that
>> LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
>> generally, it is questionable that it is called in such a high level
>> routine, rather than the start of a specific merge pass -- I said so a
>> couple of times already).
>
>
> You can't release the tape buffer at the end of a pass, because the buffer
> of a tape will already be filled with data from the next run on the same
> tape.

Okay, but can't you just not use batch memory for non-final merges,
per my initial approach? That seems far cleaner.

-- 
Peter Geoghegan


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

Re: [HACKERS] Tuplesort merge pre-reading

[Peter Geoghegan]

On Wed, Sep 28, 2016 at 5:11 PM, Peter Geoghegan  wrote:
> This is why I never pursued batch memory for non-final merges. Isn't
> that what you're doing here? You're pretty much always setting
> "state->batchUsed = true".

Wait. I guess you feel you have to, since it wouldn't be okay to use
almost no memory per tape on non-final merges.

You're able to throw out so much code here in large part because you
give almost all memory over to logtape.c (e.g., you don't manage each
tape's share of "slots" anymore -- better to give everything to
logtape.c). So, with your patch, you would actually only have one
caller tuple in memory at once per tape for a merge that doesn't use
batch memory (if you made it so that a merge *could* avoid the use of
batch memory, as I suggest).

In summary, under your scheme, the "batchUsed" variable contains a
tautological value, since you cannot sensibly not use batch memory.
(This is even true with !state->tuples callers).

Do I have that right? If so, this seems rather awkward. Hmm.

-- 
Peter Geoghegan


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

Re: [HACKERS] "Some tests to cover hash_index"

[Robert Haas]

On Tue, Sep 20, 2016 at 2:26 PM, Alvaro Herrera
 wrote:
> Why not use generate_series() queries to insert the appropriate number
> of tuples, instead of a handful of INSERT lines each time?  Since each
> insert is a separate transaction, that would probably be faster.
>
> Why do you have a plpgsql function just to create a cursor?  Wouldn't it
> be simpler to create the cursor in an SQL statement?

This patch hasn't been updated in over a week, so I'm marking it
Returned with Feedback.  I think this is a good effort and I hope
something committable will come from it, but with 2 days left it's not
going to happen this CF.

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


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

Re: [HACKERS] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Peter Eisentraut wrote:
> I'm getting the following compiler warning (using nondefault
> optimization options):
> 
> objectaddress.c: In function 'read_objtype_from_string':
> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>   return type;

Umm.  I think it can only be uninitialized if we fall out of the end of
the array, in which case we're supposed to throw the ERROR and never
return.  Is that not working?

> The comment for the function says
> 
>  * Return ObjectType for the given object type as given by
>  * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
>  * possible output type from getObjectTypeDescription, return -1.
> 
> But the claim that it can return -1 does not seem supported by the code.

Actually, it is -- but the -1 value comes from the ObjectType array.
Perhaps the comment should state that explicitely.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: pg_ctl: Detect current standby state from pg_control

[Peter Eisentraut]

On 9/28/16 12:44 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
>  wrote:
>> > Seems overcomplicated to me. How about returning the control file all
>> > the time and let the caller pfree the result? You could then use
>> > crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.
> In short I would just go with the attached and call it a day.

Pushed that way.  Thanks!

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


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

Re: [HACKERS] Issue with bgworker, SPI and pgstat_report_stat

[Robert Haas]

On Sat, Sep 3, 2016 at 12:29 AM, Michael Paquier
 wrote:
> On Sat, Sep 3, 2016 at 10:02 AM, Tomas Vondra
>  wrote:
>> In any case, I think adding the pgstat_report_stat() into worker_spi seems
>> like a reasonable (and backpatchable) fix.
>
> Doing just that sounds reasonable seen from here. I am wondering also
> if it would not be worth mentioning in the documentation of the
> bgworkers that users trying to emulate somewhat the behavior of a
> backend should look at PostgresMain(). The code in itself is full of
> hints as well.

Everybody seems happy with this fix for a first step, so I've
committed it and back-patched it to 9.3.

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


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

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

[Robert Haas]

On Tue, Sep 20, 2016 at 3:50 AM, Michael Paquier
 wrote:
> On Mon, Sep 19, 2016 at 6:11 PM, Pavel Stehule  
> wrote:
>> I am thinking so commit's description should be inside README
>
> Horiguchi-san, your patch has some whitespace issues, you may want to
> get a run with git diff --check. Here are some things I have spotted:
> src/bin/psql/tab-complete.c:1074: trailing whitespace.
> +"MATERIALIZED VIEW",
> src/bin/psql/tab-complete.c:2621: trailing whitespace.
> +   COMPLETE_WITH_QUERY(Query_for_list_of_roles,
>
> This set of patches is making psql tab completion move into a better
> shape, particularly with 0001 that removes the legendary huge if-elif
> and just the routine return immediately in case of a keyword match.
> Things could be a little bit more shortened by for example not doing
> the refactoring of the tab macros because they are just needed in
> tab-complete.c. The other patches introduce further improvements for
> the existing infrastructure, but that's a lot of things just for
> adding IF [NOT] EXISTS to be honest.
>
> Testing a bit, I have noticed that for example trying to after typing
> "create table if", if I attempt to do a tab completion "not exists"
> does not show up. I suspect that the other commands are failing at
> that as well.

This patch hasn't been updated in over a week and we're just about out
of time for this CommitFest, so I've marked it "Returned with
Feedback" for now.  If it gets updated, it can be resubmitted for the
next CommitFest.

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


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

Re: [HACKERS] Showing parallel status in \df+

[Tom Lane]

Pavel Stehule  writes:
> 2016-09-28 16:03 GMT+02:00 Tom Lane :
>> I propose to push my current patch (ie, move PL function
>> source code to \df+ footers), and we can use it in HEAD for awhile
>> and see what we think.  We can alway improve or revert it later.

> I had some objection to format of source code - it should be full source
> code, not just header and body.

That would be redundant with stuff that's in the main part of the \df
display.  I really don't need to see the argument types twice, for instance.

regards, tom lane


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

Re: [HACKERS] hstore: add hstore_length function

[Robert Haas]

On Wed, Jun 8, 2016 at 10:44 AM, Robert Haas  wrote:
> On Mon, Jun 6, 2016 at 7:57 PM, Korbin Hoffman  wrote:
>> With regards to your second point- I've been maintaining consistency
>> with the rest of the hstore module. Hstore's _size is internally
>> stored as a uint, but all uses of HS_COUNT across the feature end up
>> stored in a signed int. I could only find (grep) a few occurrences of
>> PG_RETURN_UINT32 across the entire codebase, and none in the hstore
>> module. If there's strong consensus for change, though, I'm happy to
>> do so.
>
> The PG_RETURN_BLAH macro chosen should match the declared return type
> of that function.  So if your function, for example, returns int4 (or
> integer, which is the same thing), PG_RETURN_INT32 is correct.
>
> There are no built-in SQL datatypes for unsigned integers, which is
> why you did not find many uses of PG_RETURN_UINT32 in the code base.

Since this patch was never updated in response to this review, I am
marking it "Returned with Feedback" in this CommitFest.  If it is
updated, it can be resubmitted to a future CommitFest.

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


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

Re: [HACKERS] [PATCH] SortSupport for macaddr type

[Robert Haas]

On Wed, Sep 14, 2016 at 6:14 AM, Julien Rouhaud
 wrote:
> On 26/08/2016 19:44, Brandur wrote:
>> Hello,
> Hello,
>
>> I've attached a patch to add SortSupport for Postgres' macaddr which has the
>> effect of improving the performance of sorting operations for the type. The
>> strategy that I employ is very similar to that for UUID, which is to create
>> abbreviated keys by packing as many bytes from the MAC address as possible 
>> into
>> Datums, and then performing fast unsigned integer comparisons while sorting.
>>
>> I ran some informal local benchmarks, and for cardinality greater than 100k
>> rows, I see a speed up on `CREATE INDEX` of roughly 25% to 65%. (For those
>> interested, I put a few more numbers into a small report here [2].)
>>
>
> That's a nice improvement!
>
>> Admittedly, this is not quite as useful as speeding up sorting on a more 
>> common
>> data type like TEXT or UUID, but the change still seems like a useful
>> performance improvement. I largely wrote it as an exercise to familiarize
>> myself with the Postgres codebase.
>>
>> I'll add an entry into the current commitfest as suggested by the Postgres 
>> Wiki
>> and follow up here with a link.
>>
>> Thanks, and if anyone has feedback or other thoughts, let me know!
>>
>
> I just reviewed your patch.  It applies and compiles cleanly, and the
> abbrev feature works as intended.  There's not much to say since this is
> heavily inspired on the uuid SortSupport. The only really specific part
> is in the abbrev_converter function, and I don't see any issue with it.
>
> I have a few trivial comments:
>
> * you used macaddr_cmp_internal() function name, for uuid the same
> function is named uuid_internal_cmp().  Using the same naming pattern is
> probably better.
>
> * the function comment on macaddr_abbrev_convert() doesn't mention
> specific little-endian handling
>
> * "There will be two bytes of zero padding on the least significant end"
>
> "least significant bits" would be better
>
> * This patch will trigger quite a lot modifications after a pgindent
> run.  Could you try to run pgindent on mac.c before sending an updated
> patch?

Since it's been two weeks and this patch hasn't been updated in
response to this review, I have marked it "Returned with Feedback" in
the CommitFest.  If it is updated, it can be resubmitted for the next
CommitFest.

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


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

Re: [HACKERS] Binary I/O for isn extension

[Robert Haas]

On Mon, Aug 22, 2016 at 8:14 AM, Fabien COELHO  wrote:
> Hello Shay,
>> Attached is a new version of the patch, adding an upgrade script and the
>> rest of it. Note that because, as Fabien noted, there's doesn't seem to be
>> a way to add send/receive functions with ALTER TYPE, I did that by
>> updating
>> pg_type directly - hope that's OK.
>
> This patch does not apply anymore, because there as been an update in
> between to mark relevant contrib functions as "parallel".
>
> Could you update the patch?

So, it's been over a month since this request, and there doesn't seem
to be an update to this patch.  The CommitFest is over in 2 days, so
I've marked this "Returned with Feedback".  Shay, please feel free to
resubmit for the next CommitFest.

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


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

Re: [HACKERS] Change error code for hstore syntax error

[Robert Haas]

On Sun, Sep 4, 2016 at 7:15 PM, Marko Tiikkaja  wrote:
> On 2016-05-09 19:42, Sherrylyn Branchaw wrote:
>>
>> I'm attaching a revised patch; please let me know if there are any other
>> issues before I submit to the commitfest.
>
> I think this is mostly good, but these two should be changed:
>
>   errmsg("unexpected end of string: \"%s\"", state->begin)
>   errmsg("syntax error at position %d: \"%s\"", ...)
>
> Right now, aside from the error code, these two look like they're reporting
> about an error in the SQL statement itself, and not in an input value for a
> type.  I think they should look more like this:
>
>   errmsg("invalid input syntax for type hstore: \"%s\"", string),
>   errdetail("Unexpected end of input.")
>
> If possible, it might also make sense to provide more information than
> "unexpected end of string".  For example: what character were you expecting
> to find, or what were you scanning?  I didn't look too closely what exactly
> could be done here.  I'll leave that part to you.

Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback".  Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.

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


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

Re: [HACKERS] pg_basebackup stream xlog to tar

[Robert Haas]

On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier
 wrote:
> [ review comments ]

This thread has been sitting idle for more than 3 weeks, so I'm
marking it "Returned with Feedback" in the CommitFest application.
Magnus, Michael's latest round of comments seem pretty trivial, so
perhaps you want to just fix whichever of them seem to you to have
merit and commit without waiting for the next CommitFest.  Or, you can
resubmit for the next CommitFest if you think it needs more review.
But the CommitFest is just about over so it's time to clean out old
entries, one way or the other.

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


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

Re: [HACKERS] Proposal for changes to recovery.conf API

[Robert Haas]

On Tue, Sep 6, 2016 at 10:11 AM, David Steele  wrote:
> On 9/6/16 8:07 AM, Robert Haas wrote:
>> On Wed, Aug 31, 2016 at 9:45 PM, Simon Riggs 
>> wrote:
>>> Related cleanup
>>> * Promotion signal file is now called "promote.trigger" rather than
>>> just "promote"
>>> * Remove user configurable "trigger_file" mechanism - use
>>> "promote.trigger" for all cases
>>
>>
>> I'm in favor of this.  I don't think that it's very hard for authors
>> of backup tools to adapt to this new world, and I don't see that
>> allowing configurability here does anything other than create more
>> cases to worry about.
>
> +1 from a backup tool author.

It's time to wrap up this CommitFest, and this thread doesn't seem to
contain anything that looks like a committable patch.  So, I'm marking
this "Returned with Feedback".  I hope that the fact that there's been
no discussion for the last three weeks doesn't mean this effort is
dead; I would like very much to see it move forward.

Thanks,

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


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

Re: [HACKERS] pg_basebackup stream xlog to tar

[Magnus Hagander]

On Sep 28, 2016 19:11, "Robert Haas"  wrote:
>
> On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier
>  wrote:
> > [ review comments ]
>
> This thread has been sitting idle for more than 3 weeks, so I'm
> marking it "Returned with Feedback" in the CommitFest application.
> Magnus, Michael's latest round of comments seem pretty trivial, so
> perhaps you want to just fix whichever of them seem to you to have
> merit and commit without waiting for the next CommitFest.  Or, you can
> resubmit for the next CommitFest if you think it needs more review.
> But the CommitFest is just about over so it's time to clean out old
> entries, one way or the other.

Yeah, understood. I was planning to get back to it this week, but failed to
find the time. I'll still have some hope about later this week, but most
likely not until the next.

/Magnus

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

[Robert Haas]

On Tue, Sep 6, 2016 at 9:37 PM, Gerdan Rezende dos Santos
 wrote:
> After review, I realized that there is a call to the function:
> doShellQuoting (pgdumpopts, OPTARG), which no longer seems to exist ...
> After understand the code, I saw that the call is appendShellString
> (pgdumpopts, OPTARG).
>
> Follow the patches already with the necessary corrections.

This doesn't seem to take into account the discussion between Tom Lane
and Jim Nasby about how this feature should work.

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


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

Re: [HACKERS] Truncating/vacuuming relations on full tablespaces

[Robert Haas]

On Thu, Sep 8, 2016 at 2:46 AM, Haribabu Kommi  wrote:
> Patch needs rebase, it is failing to apply on latest master.
> And also there are some pending comment fix from Robert.

It's been almost three weeks and this hasn't been updated, so I think
it's pretty clear that it should be marked "Returned with Feedback" at
this point.  I'll go do that.  Asif, if you update the patch, you can
resubmit for the next CommitFest.  Please make sure that all review
comments already given are addressed in your next revision so that
reviewers don't waste time giving you the same comments again.

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


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

Re: [HACKERS] Sample configuration files

[Robert Haas]

On Thu, Sep 8, 2016 at 8:52 AM, Tom Lane  wrote:
> Vik Fearing  writes:
>> I noticed that this patch has been marked Waiting on Author with no
>> comment.  Peter, what more should I be doing right now while waiting for
>> Martín's review?
>
> FWIW, I agree with the upthread misgivings about whether this is actually
> a useful effort.  Even if we installed the sample config files somewhere
> (something there is not consensus for AFAICT), they would not actually
> *do* anything useful as standalone files.  I suppose you are imagining
> that people would either manually concatenate them onto postgresql.conf
> or insert an include directive for them into postgresql.conf, but neither
> of those things sound pleasant or maintainable.
>
> Moreover, it's not clear why anyone would do that at all in the age of
> ALTER SYSTEM SET.
>
> I suggest that it'd be more fruitful to view this as a documentation
> effort; that is, in each contrib module's SGML documentation file provide
> a standardized section listing all its parameters and their default
> settings.  That would be something that could be copied-and-pasted from
> into either an editor window on postgresql.conf for the old guard, or
> an ALTER SYSTEM SET command for the new.

So, tallying up the votes, one person has spoken in favor of this
(Martín Marqués) and two against it (Tom Lane and Robert Haas).  One
presumes the author is also in favor, so that's a 2-2 tie.  That's not
exactly a consensus against this effort, but it's not a ringing
endorsement, either.  It's hard for me to imagine anything getting
committed here unless some more people think it's a good idea.

So, anyone else have an opinion, pro or con?

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


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

Re: [HACKERS] Change error code for hstore syntax error

[Sherrylyn Branchaw]

Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback".  Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.

Will do, Robert, and many thanks to Marko for the feedback. I apologize for
the delay; I had surgery two days ago and will get back to this as soon as
possible.

Sherrylyn

Re: [HACKERS] Index Onlys Scan for expressions

[Robert Haas]

On Thu, Sep 8, 2016 at 2:58 PM, Vladimir Sitnikov
 wrote:
> Ildar> Could you please try the patch and tell if it works for you?
>
> I've tested patch6 against recent head. The patch applies with no problems.
>
> The previous case (filter on top of i-o-s) is fixed. Great work.
>
> Here are the test cases and results:
> https://gist.github.com/vlsi/008e18e18b609fcaaec53d9cc210b7e2
>
> However, it looks there are issues when accessing non-indexed columns.
> The error is "ERROR: variable not found in subplan target list"
> The case is 02_case2_fails.sql (see the gist link above)
>
> The essence of the case is "create index on substr(vc, 1, 128)"
> and assume that majority of the rows have length(vc)<128.
> Under that conditions, it would be nice to do index-only-scan
> and filter (like in my previous case), but detect "long" rows
> and do additional recheck for them.

Based on this report, this patch appears to have bugs that would
preclude committing it, so I'm marking it "Returned with Feedback" for
this CommitFest, which is due to end shortly.  Ildar, please feel free
to resubmit once you've updated the patch.

FWIW, I think this is a good effort and hope to see it move forward.

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


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

Re: [HACKERS] pgbench more operators & functions

[Jeevan Ladhe]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

The patch looks good to me now.
Passing this to committer.

The new status of this patch is: Ready for Committer

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

Re: [HACKERS] identity columns

[Robert Haas]

On Mon, Sep 12, 2016 at 5:02 PM, Peter Eisentraut
 wrote:
> Thank you for this extensive testing.  I will work on getting the bugs
> fixed.

It looks like the patch has not been updated; since the CommitFest is
(hopefully) wrapping up, I am marking this "Returned with Feedback"
for now.

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


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

Re: [HACKERS] PassDownLimitBound for ForeignScan/CustomScan

[Robert Haas]

On Tue, Sep 13, 2016 at 9:07 PM, Kouhei Kaigai  wrote:
> It is because of just my time pressure around the patch submission days.
> I'll try to enhance postgres_fdw as a usage of this run-time optimization.

Time has (pretty much) expired for this CommitFest.  In any case, this
will amount to a whole new patch, not just a rework of the current
one.  So I'm going to mark this "Rejected" in the CommitFest, and I
suggest you start a new thread for the proposed approach if you get a
chance to work on it.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Robert Haas]

On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
>> Christoph Berg  writes:
>>> I've always been wondering why we don't set a log_line_prefix by
>>> default.
>>
>> I think the odds of getting to something that everyone would agree on
>> are nil, so I'm not excited about getting into that particular
>> bikeshed-painting discussion.  Look at the amount of trouble we're
>> having converging on a default for the regression tests, which are
>> a far narrower use-case than "everybody".
>
> Well, practically anything that includes a PID and the timestamp is
> going to be an improvement over the status quo.  Just because we can't
> all agree on what would be perfect does not mean that we can't do
> better than what we've got now.  +1 for trying.

Is there any chance we can move forward here, or is this effort doomed for now?

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


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

Re: [HACKERS] less expensive pg_buffercache on big shmem

[Robert Haas]

On Tue, Sep 20, 2016 at 7:43 PM, Tomas Vondra
 wrote:
> On 09/02/2016 11:01 AM, Robert Haas wrote:
>>
>> On Fri, Sep 2, 2016 at 8:49 AM, Andres Freund  wrote:
>>>
>>> On 2016-09-02 08:31:42 +0530, Robert Haas wrote:

 I wonder whether we ought to just switch from the consistent method to
 the semiconsistent method and call it good.
>>>
>>>
>>> +1. I think, before long, we're going to have to switch away from having
>>> locks & partitions in the first place. So I don't see a problem relaxing
>>> this. It's not like that consistency really buys you anything...  I'd
>>> even consider not using any locks.
>>
>> I think we certainly want to lock the buffer header, because otherwise
>> we might get a torn read of the buffer tag, which doesn't seem good.
>> But it's not obvious to me that there's any point in taking the lock
>> on the buffer mapping partition; I'm thinking that doesn't really do
>> anything unless we lock them all, and we all seem to agree that's
>> going too far.
>
> +1 from me to only locking the buffer headers. IMHO that's perfectly fine
> for the purpose of this extension.

So, I think we have agreement on the way forward here, but what we
don't have is a committable patch.  I'm willing to commit one before
the end of this CommitFest if somebody produces one RSN; otherwise,
this is going to have to go into the "Returned with Feedback" bucket.

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


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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Sun, Sep 25, 2016 at 3:28 PM, Tomas Vondra
 wrote:
> Sure, that would be useful.
>
> I think it would be useful to make repository of such data sets, so that
> patch authors & reviewers can get a reasonable collection of data sets if
> needed, instead of scrambling every time. Opinions?

In theory, great idea.  In practice, I suspect the problem will be
that nobody will know what the use case for a particular data set was
supposed to be, and therefore it'll become a collection of files
nobody knows what to do with.

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


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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
> OK, I'll think about how to do that more efficiently.  The smaller
> incremental improvement isn't surprising, because in this example the
> index would still be 90-something MB if it had no free space at all,
> so there's going to be decreasing returns from any additional work
> to avoid wasted free space.  But if we can do it cheaply, this does
> suggest that using pages in order by free space is of value.

Tom, are you planning to do something about this patch yet this
CommitFest, or leave it until later?

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


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

[HACKERS] CommitFest wrap-up

[Robert Haas]

As some of you probably noticed, I just made a sweep through
everything that was marked "Waiting on Author" in the CommitFest and
hadn't been updated in the last couple of days.  Most of those I
marked as "Returned with Feedback", but some of them got some other
status, one I committed, and a few I just sent a ping of some sort to
the thread.

With that cleanup, things now look like this:

Needs review: 46. Waiting on Author: 21. Ready for Committer: 18.
Committed: 94. Moved to next CF: 1. Rejected: 12. Returned with
Feedback: 27. Total: 219.

There is obviously a good bit of stuff that has been marked "Ready for
Committer"; it would be good if committers could take a look at those
and see if they agree that a commit might be possible without undue
effort.

There is also a lot of stuff that is still in a "Needs Review" state.
I suspect a good amount of that stuff has actually had some review,
and if somebody wants to help, it would be great to go through those
entries and change the status of any of them that are not actually
waiting for review - i.e. if they have been reviewed and are awaiting
an update, mark them as "Waiting on Author".  This will help us
separate the things that still really deserve a look from the stuff
that has already had one.

The things that haven't had any review yet should get a review if
that's at all possible.

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


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

[HACKERS] Batches, error handling and transaction in the protocol

[Shay Rojansky]

Hi everyone, I'd appreciate some guidance on an issue that's been raised
with Npgsql, input from other driver writers would be especially helpful.

Npgsql currently supports batching (or pipelining) to avoid roundtrips, and
sends a Sync message only at the end of the batch (so
Parse1/Bind1/Describe1/Execute1/Parse2/Bind2/Describe2/Execute2/Sync). The
reasoning is that if the first statement in the batch fails, the others
shouldn't be processed. This seems to be the standard approach (the
proposed patch for libpq seems to do the same).

At the same time, if the batch doesn't occur within an explicit transaction
(i.e. after BEGIN), it is automatically wrapped in an implicit transaction,
with Sync committing it. This can, for example, provoke deadlocks if two
batches try to update the same rows in reverse order. The problem is that
the user didn't request a transaction in any way - they're just using
batching to avoid roundtrips and their intention is to be in autocommit
mode.

One possible solution for this would be to insert a Sync after every
execute in the batch, rather than a single Sync at the very end. This would
make batches work the same as unbatched statements, and would resolve the
deadlocks. However, behavior in case of error would be problematic:
PostgreSQL would continue executing later messages if earlier ones failed,
Npgsql would have to deal with multiple errors, etc.

More generally speaking, the protocol appears to couple two different
things which may be unrelated. On the one hand, we have a protocol sync
mechanism for error recovery (skip until Sync). One the other hand, we have
an implicit transaction for extended query messages until that same Sync.
It seems valid to want to have error recovery without an implicit
transaction, but this doesn't seem supported by the current protocol (I
could add a note for v4).

Finally, to give more context, a Microsoft developer ran into this while
running ASP.NET benchmarks over Npgsql and its Entity Framework Core ORM
provider. One of EFCore's great new features is that it batches database
updates into a single roundtrip, but this triggered deadlocks. Whereas in
many cases it's OK to tell users to solve the deadlocks by properly
ordering their statements, when an ORM is creating the batch it's a more
difficult proposition.

Thanks for any thoughts or guidance!

Shay

Re: [HACKERS] Binary I/O for isn extension

[Shay Rojansky]

Sorry about this, I just haven't had a free moment (and it's definitely not
very high priority...)

On Wed, Sep 28, 2016 at 5:04 PM, Robert Haas  wrote:

> On Mon, Aug 22, 2016 at 8:14 AM, Fabien COELHO 
> wrote:
> > Hello Shay,
> >> Attached is a new version of the patch, adding an upgrade script and the
> >> rest of it. Note that because, as Fabien noted, there's doesn't seem to
> be
> >> a way to add send/receive functions with ALTER TYPE, I did that by
> >> updating
> >> pg_type directly - hope that's OK.
> >
> > This patch does not apply anymore, because there as been an update in
> > between to mark relevant contrib functions as "parallel".
> >
> > Could you update the patch?
>
> So, it's been over a month since this request, and there doesn't seem
> to be an update to this patch.  The CommitFest is over in 2 days, so
> I've marked this "Returned with Feedback".  Shay, please feel free to
> resubmit for the next CommitFest.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
>> OK, I'll think about how to do that more efficiently.  The smaller
>> incremental improvement isn't surprising, because in this example the
>> index would still be 90-something MB if it had no free space at all,
>> so there's going to be decreasing returns from any additional work
>> to avoid wasted free space.  But if we can do it cheaply, this does
>> suggest that using pages in order by free space is of value.

> Tom, are you planning to do something about this patch yet this
> CommitFest, or leave it until later?

I doubt I will get to it this week, so let's mark it RWF for this fest.

regards, tom lane


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

Re: [HACKERS] Tuplesort merge pre-reading

[Heikki Linnakangas]

On 09/28/2016 07:11 PM, Peter Geoghegan wrote:

On Wed, Sep 28, 2016 at 5:04 PM, Heikki Linnakangas  wrote:

Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.



How does the distribution of the runs on the tapes matter?


The exact details are not really relevant to this discussion (I think
it's confusing that we simply say "Target Fibonacci run counts",
FWIW), but the simple fact that it can be quite uneven is.


Well, I claim that the fact that the distribution of runs is uneven, 
does not matter. Can you explain why you think it does?



This is why I never pursued batch memory for non-final merges. Isn't
that what you're doing here? You're pretty much always setting
"state->batchUsed = true".


Yep. As the patch stands, we wouldn't really need batchUsed, as we know 
that it's always true when merging, and false otherwise. But I kept it, 
as it seems like that might not always be true - we might use batch 
memory when building the initial runs, for example - and because it 
seems nice to have an explicit flag for it, for readability and 
debugging purposes.



I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).



You can't release the tape buffer at the end of a pass, because the buffer
of a tape will already be filled with data from the next run on the same
tape.


Okay, but can't you just not use batch memory for non-final merges,
per my initial approach? That seems far cleaner.


Why? I don't see why the final merge should behave differently from the 
non-final ones.


- Heikki



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

Re: [HACKERS] psql casts aspersions on server reliability

[Petr Jelinek]

On 28/09/16 17:13, David Steele wrote:
> On 9/28/16 10:22 AM, Robert Haas wrote:
>> On Wed, Sep 28, 2016 at 9:14 AM, Tom Lane  wrote:
>>> Robert Haas  writes:
 psql tends to do things like this:
 rhaas=# select * from pg_stat_activity;
 FATAL:  terminating connection due to administrator command
 server closed the connection unexpectedly
 This probably means the server terminated abnormally
 before or while processing the request.
>>>
 Basically everything psql has to say about this is a lie:
>>>
>>> I cannot get terribly excited about this.  What you seem to be proposing
>>> is that psql try to intuit the reason for connection closure from the
>>> last error message it got, but that seems likely to lead to worse lies
>>> than printing a boilerplate message.
>>>
>>> I could go along with just dropping the last sentence ("This probably...")
>>> if the last error we got was FATAL level.  I don't find "unexpectedly"
>>> to be problematic here: from the point of view of psql, and probably
>>> of its user, the shutdown *was* unexpected.
>>
>> I don't care very much whether we try to intuit the reason for
>> connection closure or not; it could be done, but I don't feel that it
>> has to be done.  My bigger point is that currently psql speculates
>> that the reason for *every* connection closure is abnormal server
>> termination, which is actually a very rare event.
>>
>> It may have been common when that message was added.
>> 1a17447be1186fdd36391c58a2a0209f613d89c4 changed the wording this
>> message in 2001, and the original message seems to date to
>> 011ee13131f6fa2f6dbafd3827b70d051cb28f64 in 1996.  And my guess is at
>> that time the server probably did just roll over and die with some
>> regularity.  But today it usually doesn't.  It's neither helpful nor
>> good PR for libpq to guess that the most likely cause of a server
>> disconnection is server unreliability.
>>
>> I have seen actual instances of customers getting upset by this
>> message even though the server had been shut down quite cleanly.  The
>> message got into a logfile and induced minor panic.  Fortunately, I
>> have not seen this happen lately.
> 
> +1 for making this error message less frightening.  I have also had to
> explain it away on occasion.
> 

+1 I've seen this being misleading way too often.

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


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

Re: [HACKERS] should xlog_outdesc modify its argument?

[Mark Dilger]

> On Sep 27, 2016, at 11:25 PM, Heikki Linnakangas  wrote:
> 
> On 09/28/2016 02:35 AM, Mark Dilger wrote:
>> The function
>> 
>>  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
>> 
>> in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
>> and after returning a string describing an XLogRecord, it clears the
>> state data in its XLogReaderState argument.  That mixes the read-only
>> semantics of "give me a string that describes this argument" and the
>> read-write semantics of "clear out the value in this argument".
> 
> I don't see where the "clears the state data" is happening. Can you elaborate?

My apologies.  At the bottom of the function, it calls through the function 
pointer

RmgrTable[rmid].rm_desc(buf, record);

which is set up to call various *_desc functions.  I must have chased through
those function pointers incorrectly, as I can't find the problem now that I am
reviewing all those functions.

Sorry for the noise,

Mark Dilger



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

Re: [HACKERS] Showing parallel status in \df+

[Pavel Stehule]

Hi

2016-09-28 18:57 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > 2016-09-28 16:03 GMT+02:00 Tom Lane :
> >> I propose to push my current patch (ie, move PL function
> >> source code to \df+ footers), and we can use it in HEAD for awhile
> >> and see what we think.  We can alway improve or revert it later.
>
> > I had some objection to format of source code - it should be full source
> > code, not just header and body.
>
> That would be redundant with stuff that's in the main part of the \df
> display.  I really don't need to see the argument types twice, for
> instance.
>

I am sorry, I disagree. Proposed form is hard readable. Is not possible to
simply copy/paste.

I cannot to imagine any use case for proposed format.

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Christoph Berg]

Re: Robert Haas 2016-09-28 

> > Well, practically anything that includes a PID and the timestamp is
> > going to be an improvement over the status quo.  Just because we can't
> > all agree on what would be perfect does not mean that we can't do
> > better than what we've got now.  +1 for trying.
> 
> Is there any chance we can move forward here, or is this effort doomed for 
> now?

IMHO it would make sense. Maybe we should collect a few suggestions,
and then take a poll?

Christoph


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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Wed, Sep 28, 2016 at 2:11 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
>>> OK, I'll think about how to do that more efficiently.  The smaller
>>> incremental improvement isn't surprising, because in this example the
>>> index would still be 90-something MB if it had no free space at all,
>>> so there's going to be decreasing returns from any additional work
>>> to avoid wasted free space.  But if we can do it cheaply, this does
>>> suggest that using pages in order by free space is of value.
>
>> Tom, are you planning to do something about this patch yet this
>> CommitFest, or leave it until later?
>
> I doubt I will get to it this week, so let's mark it RWF for this fest.

OK, done.  Thanks for the reply.

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


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

Re: [HACKERS] Binary I/O for isn extension

[Robert Haas]

On Wed, Sep 28, 2016 at 2:05 PM, Shay Rojansky  wrote:
> Sorry about this, I just haven't had a free moment (and it's definitely not
> very high priority...)

No issues, just cleaning house.

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


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

Re: [HACKERS] Floating point comparison inconsistencies of the geometric types

[Kevin Grittner]

On Wed, Sep 28, 2016 at 12:02 PM, Emre Hasegeli  wrote:

>> `make check` finds differences per the attached.  Please
>> investigate why the regression tests are failing and what the
>> appropriate response is.
>
> I fixed the first one and workaround the second with COLLATE "C".  I
> have how my changes caused this regression.
>
> "select_views" test runs "SELECT name, #thepath FROM iexit ORDER BY 1,
> 2" and expects to get rows in this order:
>
>>  I- 580Ramp |8
>>  I- 580/I-680  Ramp |2
>
> With the collation on my laptop, this is actually true:
>
>> regression=# select 'I- 580/I-680  Ramp' < 'I- 580   
>>  Ramp';
>>  ?column?
>> --
>>  t
>> (1 row)
>
> However, on the Linux server, I am testing it is not:
>
>> regression=# select 'I- 580Ramp' < 'I- 580/I-680 
>>  Ramp';
>>  ?column?
>> --
>>  f
>> (1 row)
>
> Do you know how it is not failing on the master?

Well, those two results are not contradictory -- notice that you
switched the order of the values in the comparison.  I don't think
you've really found the explanation yet.

>> [discussing inline static functions compared to macros for min()/max(), etc.]
>> I suspect that they will be as fast or faster, and they eliminate
>> the hazard of multiple evaluation, where a programmer might not be
>> aware of the multiple evaluation or of some side-effect of an
>> argument.
>
> I reworked the the patches to use inline functions and fixed the
> problems I found.  The new versions are attached.

Will take a look and post again.

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


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

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Tue, Sep 27, 2016 at 3:06 PM, Jesper Pedersen
 wrote:
> I have been running various tests, and applications with this patch together
> with the WAL v5 patch [1].
>
> As I havn't seen any failures and doesn't currently have additional feedback
> I'm moving this patch to "Ready for Committer" for their feedback.

Cool!  Thanks for reviewing.

Amit, can you please split the buffer manager changes in this patch
into a separate patch?  I think those changes can be committed first
and then we can try to deal with the rest of it.  Instead of adding
ConditionalLockBufferShared, I think we should add an "int mode"
argument to the existing ConditionalLockBuffer() function.  That way
is more consistent with LockBuffer().  It means an API break for any
third-party code that's calling this function, but that doesn't seem
like a big problem.  There are only 10 callers of
ConditionalLockBuffer() in our source tree and only one of those is in
contrib, so probably there isn't much third-party code that will be
affected by this, and I think it's worth it for the long-term
cleanliness.

As for CheckBufferForCleanup, I think that looks OK, but: (1) please
add an Assert() that we hold an exclusive lock on the buffer, using
LWLockHeldByMeInMode; and (2) I think we should rename it to something
like IsBufferCleanupOK.  Then, when it's used, it reads like English:
if (IsBufferCleanupOK(buf)) { /* clean up the buffer */ }.

I'll write another email with my thoughts about the rest of the patch.
For the record, Amit and I have had extensive discussions about this
effort off-list, and as Amit noted in his original post, the design is
based on suggestions which I previously posted to the list suggesting
how the issues with hash indexes might get fixed.  Therefore, I don't
expect to have too many basic disagreements regarding the design of
the patch; if anyone else does, please speak up.  Andres already
stated that he things working on btree-over-hash would be more
beneficial than fixing hash, but at this point it seems like he's the
only one who takes that position.  Even if we accept that working on
the hash AM is a reasonable thing to do, it doesn't follow that the
design Amit has adopted here is ideal.  I think it's reasonably good,
but that's only to be expected considering that I drafted the original
version of it and have been involved in subsequent discussions;
someone else might dislike something that I thought was OK, and any
such opinions certainly deserve a fair hearing.  To be clear, It's
been a long time since I've looked at any of the actual code in this
patch and I have at no point studied it deeply, so I expect that I may
find a fair number of things that I'm not happy with in detail, and
I'll write those up along with any design-level concerns that I do
have.  This should in no way forestall review from anyone else who
wants to get involved.

Thanks,

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


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

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

[Stephen Frost]

Heikki, Michael, Magnus,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Sep 27, 2016 at 10:42 PM, Heikki Linnakangas  wrote:
> > The libpq-side is not. Just calling random() won't do. We haven't needed for
> > random numbers in libpq before, but now we do. Is the pgcrypto solution
> > portable enough that we can use it in libpq?
> 
> Do you think that urandom would be enough then? The last time I took a
> look at that, I saw urandom on all modern platforms even those ones:
> OpenBSD, NetBSD, Solaris, SunOS. For Windows the CryptGen stuff would
> be nice enough I guess..

Magnus had been working on a patch that, as I recall, he thought was
portable and I believe could be used on both sides.

Magnus, would what you were working on be helpful here...?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Hash Indexes

[Andres Freund]

On 2016-09-28 15:04:30 -0400, Robert Haas wrote:
> Andres already
> stated that he things working on btree-over-hash would be more
> beneficial than fixing hash, but at this point it seems like he's the
> only one who takes that position.

Note that I did *NOT* take that position. I was saying that I think we
should evaluate whether that's not a better approach, doing some simple
performance comparisons.

Greetings,

Andres Freund


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

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Wed, Sep 28, 2016 at 3:06 PM, Andres Freund  wrote:
> On 2016-09-28 15:04:30 -0400, Robert Haas wrote:
>> Andres already
>> stated that he things working on btree-over-hash would be more
>> beneficial than fixing hash, but at this point it seems like he's the
>> only one who takes that position.
>
> Note that I did *NOT* take that position. I was saying that I think we
> should evaluate whether that's not a better approach, doing some simple
> performance comparisons.

OK, sorry.  I evidently misunderstood your position, for which I apologize.

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


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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Done.

> I think you should use braces here, not parens:

Fixed.

> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Done.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

Reworded to not attempt to use AND and OR as verbs.  Additionally, a
patch is also included to remove those from the comments in
rowsecurity.c.  There are a few other places where we have "OR'd" in the
code base, but I didn't think it made sense to change those as part of
this effort.

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> With this patch, pg_policy catalog now has seven columns, however
> Natts_pg_policy is still set to 6. It should be updated to 7 now.
> Doing this regression seems OK.

Ah, certainly interesting that it only caused incorrect behavior and not
a crash (and no incorrect behavior even on my system, at least with the
regression tests and other testing I've done).

Fixed.

> 1. In documentation, we should put both permissive as well as restrictive in
> the header like permissive|restrictive. 

I'm not sure which place in the documentation you are referring to
here..?  [ AS { PERMISSIVE | RESTRICTIVE } ] was added to the CREATE
POLICY synopsis documentation.

> 2. "If the policy is a "permissive" or "restrictive" policy." seems broken
> as
> sentence starts with "If" and there is no other part to it. Will it be
> better
> to say "Specifies whether the policy is a "permissive" or "restrictive"
> policy."?

Rewrote this to be clearer, I hope.

> 3. " .. a policy can instead by "restrictive""
> Do you mean "instead be" here?

This was also rewritten.

> 4. It will be good if we have an example for this in section
> "5.7. Row Security Policies"

I haven't added one yet, but will plan to do so.

> 5. AS ( PERMISSIVE | RESTRICTIVE )
> should be '{' and '}' instead of parenthesis.

Fixed.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

As mentioned, this is tab-completion for the new options which this
patch introduces.

> 7. Natts_pg_policy should be updated to 7 now.

Fixed.

> 8. In following error, $2 and @2 should be used to correctly display the
> option and location.

Fixed.

> I think adding negative test to test this error should be added in
> regression.

Done.

> 9. Need to update following comments in gram.y to reflect new changes.

Done.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

As mentioned, I don't see a use-case for it currently.

> 11. Will it be better to use boolean for polpermissive in _policyInfo?
> And then set that appropriately while getting the policies. So that later we
> only need to test the boolean avoiding string comparison.

Done.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.

Done, for this and the other defaults.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -\QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +\QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E

Updated to reflect what pg_dump now produces.

> 14. While displaying policy details in permissionsList, per syntax, we
> should
> display (RESTRICT) before the command option. Also will it be better to use
> (RESTRICTIVE) instead of (RESTRICT)?

Fixed.

> 15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
> after
> policy name and before command option ?
> If we do that then changes related to adding "POLICY" followed by
> "RESTRICTIVE"
> will be straight forward.

Fixed.

> 16. It be good to have test-coverage for permissionsList,
> describeOneTableDetails and dump-restore changes. Please add those.

Done.

> 17. In pg_policies view, we need to add details related to PERMISSIVE and
> RESTRICTIVE. Please do so. Also add test for it.

Done.

> 18. Fix typos pointed earlier by Alvera.

Done.

Updated patch attached.

Thanks!

Stephen
From 020871cddd3c7187bd55a52673cae0af17a95246 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH 1/2] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 doc/src/sgml/ref/create_policy.sgml   |  28 
 src/backend/catalog/system_views.

Re: [HACKERS] Showing parallel status in \df+

[Alvaro Herrera]

Pavel Stehule wrote:

> I am sorry, I disagree. Proposed form is hard readable. Is not possible to
> simply copy/paste.

Why do you care?  You can use \sf if you want to copy&paste the
function code.

> I cannot to imagine any use case for proposed format.

My vote (which was not counted by Stephen) was to remove it from \df+
altogether.  I stand by that.  People who are used to seeing the output
in \df+ will wonder "where the heck did it go" and eventually figure it
out, at which point it's no longer a problem.  We're not breaking
anyone's scripts, that's for sure.

If we're not removing it, I +0 support the option of moving it to
footers.  I'm -1 on doing nothing.

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


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

Re: [HACKERS] Showing parallel status in \df+

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Pavel Stehule wrote:
> > I cannot to imagine any use case for proposed format.
> 
> My vote (which was not counted by Stephen) was to remove it from \df+

Oh, sorry about that, not sure how I missed it. :/

> altogether.  I stand by that.  People who are used to seeing the output
> in \df+ will wonder "where the heck did it go" and eventually figure it
> out, at which point it's no longer a problem.  We're not breaking
> anyone's scripts, that's for sure.
> 
> If we're not removing it, I +0 support the option of moving it to
> footers.  I'm -1 on doing nothing.

This is more-or-less the same position that I have.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ICU integration

[Thomas Munro]

On Fri, Sep 23, 2016 at 6:27 PM, Thomas Munro
 wrote:
> On Wed, Aug 31, 2016 at 2:46 PM, Peter Eisentraut
>  wrote:
>> Here is a patch I've been working on to allow the use of ICU for sorting
>> and other locale things.
>
> This is very interesting work, and it's great to see some development
> in this area.  I've been peripherally involved in more than one
> collation-change-broke-my-data incident over the years.  I took the
> patch for a quick spin today.  Here are a couple of initial
> observations.

This seems like a solid start, but there are unresolved questions
about both high level goals (versioning strategy etc) and also some
technical details with this WIP patch.  It looks like several people
have an interest and ideas in this area, but clearly there isn't going
to be a committable patch in the next 48 hours.  So I will set this to
'Returned with Feedback' for now.  If you think you'll have a new
patch for the next CF then it looks like you can still 'Move to Next
CF' from 'Returned with Feedback' state if appropriate.  Thanks!

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


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

Re: [HACKERS] Showing parallel status in \df+

[Pavel Stehule]

2016-09-28 21:59 GMT+02:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > I am sorry, I disagree. Proposed form is hard readable. Is not possible
> to
> > simply copy/paste.
>
> Why do you care?  You can use \sf if you want to copy&paste the
> function code.
>

I know so I can use \sf. But I don't see any sense to have less readable
output of any psql command.


>
> > I cannot to imagine any use case for proposed format.
>
> My vote (which was not counted by Stephen) was to remove it from \df+
> altogether.  I stand by that.  People who are used to seeing the output
> in \df+ will wonder "where the heck did it go" and eventually figure it
> out, at which point it's no longer a problem.  We're not breaking
> anyone's scripts, that's for sure.
>

I prefer removing before proposed solution with proposed format.

We are in cycle because prosrc field is used for two independent features -
and then it can be hard to find a agreement.

Name of function in dll is some different than PL function body. But it is
stored and displayed in one field - and it is impossible do it well.

Regards

Pavel


>
> If we're not removing it, I +0 support the option of moving it to
> footers.  I'm -1 on doing nothing.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] kqueue

[Keith Fiske]

On Thu, Sep 15, 2016 at 11:11 PM, Thomas Munro <
thomas.mu...@enterprisedb.com> wrote:

> On Thu, Sep 15, 2016 at 11:04 AM, Thomas Munro
>  wrote:
> > On Thu, Sep 15, 2016 at 10:48 AM, Keith Fiske  wrote:
> >> Thomas Munro brought up in #postgresql on freenode needing someone to
> test a
> >> patch on a larger FreeBSD server. I've got a pretty decent machine
> (3.1Ghz
> >> Quad Core Xeon E3-1220V3, 16GB ECC RAM, ZFS mirror on WD Red HDD) so
> offered
> >> to give it a try.
> >>
> >> Bench setup was:
> >> pgbench -i -s 100 -d postgres
> >>
> >> I ran this against 96rc1 instead of HEAD like most of the others in this
> >> thread seem to have done. Not sure if that makes a difference and can
> re-run
> >> if needed.
> >> With higher concurrency, this seems to cause decreased performance. You
> can
> >> tell which of the runs is the kqueue patch by looking at the path to
> >> pgbench.
> >
> > Thanks Keith.  So to summarise, you saw no change with 1 client, but
> > with 4 clients you saw a significant drop in performance (~93K TPS ->
> > ~80K TPS), and a smaller drop for 64 clients (~72 TPS -> ~68K TPS).
> > These results seem to be a nail in the coffin for this patch for now.
> >
> > Thanks to everyone who tested.  I might be back in a later commitfest
> > if I can figure out why and how to fix it.
>
> Ok, here's a version tweaked to use EVFILT_PROC for postmaster death
> detection instead of the pipe, as Tom Lane suggested in another
> thread[1].
>
> The pipe still exists and is used for PostmasterIsAlive(), and also
> for the race case where kevent discovers that the PID doesn't exist
> when you try to add it (presumably it died already, but we want to
> defer the report of that until you call EventSetWait, so in that case
> we stick the traditional pipe into the kqueue set as before so that
> it'll fire a readable-because-EOF event then).
>
> Still no change measurable on my laptop.  Keith, would you be able to
> test this on your rig and see if it sucks any less than the last one?
>
> [1] https://www.postgresql.org/message-id/13774.1473972000%40sss.pgh.pa.us
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>


Ran benchmarks on unaltered 96rc1 again just to be safe. Those are first.
Decided to throw a 32 process test in there as well to see if there's
anything going on between 4 and 64

~/pgsql96rc1/bin/pgbench -i -s 100 -d pgbench -p 5496

[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1543809
latency average: 0.039 ms
tps = 25729.749474 (including connections establishing)
tps = 25731.006414 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1548340
latency average: 0.039 ms
tps = 25796.928387 (including connections establishing)
tps = 25798.275891 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1535072
latency average: 0.039 ms
tps = 25584.182830 (including connections establishing)
tps = 25585.487246 (excluding connections establishing)

[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5621013
latency average: 0.043 ms
tps = 93668.594248 (including connections establishing)
tps = 93674.730914 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5659929
latency average: 0.042 ms
tps = 94293.572928 (including connections establishing)
tps = 94300.500395 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5649572
latency average: 0.042 ms
tps = 94115.854165 (including connections establishing)
tps = 94123.436211 (excluding connections establishing)

[keith@corpus ~]$ /home/keith/pgsql96

Re: [HACKERS] Floating point comparison inconsistencies of the geometric types

[Kevin Grittner]

On Wed, Sep 28, 2016 at 2:04 PM, Kevin Grittner  wrote:

> Will take a look and post again.

I am moving this patch to the next CF.  You'll be hearing from me
sometime after this CF is closed.

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


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

Re: [HACKERS] Bug in to_timestamp().

[Tom Lane]

Artur Zakirov  writes:
> - now DCH_cache_getnew() is called after parse_format(). Because now 
> parse_format() can raise an error and in the next attempt 
> DCH_cache_search() could return broken cache entry.

I started looking at your 0001-to-timestamp-format-checking-v4.patch
and this point immediately jumped out at me.  Currently the code relies
... without any documentation ... on no elog being thrown out of
parse_format().  That's at the very least trouble waiting to happen.
There's a hack to deal with errors from within the NUMDesc_prepare
subroutine, but it's a pretty ugly and underdocumented hack.  And what
you had here was randomly different from that solution, too.

After a bit of thought it seemed to me that a much cleaner fix is to add
a "valid" flag to the cache entries, which we can leave clear until we
have finished parsing the new format string.  That avoids adding extra
data copying as you suggested, removes the need for PG_TRY, and just
generally seems cleaner and more bulletproof.

I've pushed a patch that does it that way.  The 0001 patch will need
to be rebased over that (might just require removal of some hunks,
not sure).

I also pushed 0002-to-timestamp-validation-v2.patch with some revisions
(it'd broken acceptance of BC dates, among other things, but I think
I fixed everything).

Since you told us earlier that you'd be on vacation through the end of
September, I'm assuming that nothing more will happen on this patch during
this commitfest, so I will mark the CF entry Returned With Feedback.

regards, tom lane


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
>> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
>>> I think the odds of getting to something that everyone would agree on
>>> are nil, so I'm not excited about getting into that particular
>>> bikeshed-painting discussion.  Look at the amount of trouble we're
>>> having converging on a default for the regression tests, which are
>>> a far narrower use-case than "everybody".

>> Well, practically anything that includes a PID and the timestamp is
>> going to be an improvement over the status quo.  Just because we can't
>> all agree on what would be perfect does not mean that we can't do
>> better than what we've got now.  +1 for trying.

> Is there any chance we can move forward here, or is this effort doomed for 
> now?

It seemed like nobody wanted to try to push this forward, and it will take
somebody actively pushing, IMO, for something to happen.

Perhaps we should first try to get a consensus on the regression test
use-case.

regards, tom lane


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

Re: [HACKERS] kqueue

[Thomas Munro]

On Thu, Sep 29, 2016 at 9:09 AM, Keith Fiske  wrote:
> On Thu, Sep 15, 2016 at 11:11 PM, Thomas Munro
>  wrote:
>> Ok, here's a version tweaked to use EVFILT_PROC for postmaster death
>> detection instead of the pipe, as Tom Lane suggested in another
>> thread[1].
>>
>> [...]
>
> Ran benchmarks on unaltered 96rc1 again just to be safe. Those are first.
> Decided to throw a 32 process test in there as well to see if there's
> anything going on between 4 and 64

Thanks!  A summary:

┌──┬─┬───┬┬───┐
│   code   │ clients │  average  │ standard_deviation │  median   │
├──┼─┼───┼┼───┤
│ 9.6rc1   │   1 │ 25704.923 │108.766 │ 25731.006 │
│ 9.6rc1   │   4 │ 94032.889 │322.562 │ 94123.436 │
│ 9.6rc1   │  32 │ 86647.401 │ 33.616 │ 86664.849 │
│ 9.6rc1   │  64 │ 79360.680 │   1217.453 │ 79941.243 │
│ 9.6rc1/kqueue-v6 │   1 │ 24569.683 │   1433.339 │ 25146.434 │
│ 9.6rc1/kqueue-v6 │   4 │ 93435.450 │ 50.214 │ 93442.716 │
│ 9.6rc1/kqueue-v6 │  32 │ 88000.328 │135.143 │ 87891.856 │
│ 9.6rc1/kqueue-v6 │  64 │ 71726.034 │   4784.794 │ 72271.146 │
└──┴─┴───┴┴───┘

┌─┬───┬───┬──┐
│ clients │ unpatched │  patched  │  percent_change  │
├─┼───┼───┼──┤
│   1 │ 25731.006 │ 25146.434 │ -2.271858317548874692000 │
│   4 │ 94123.436 │ 93442.716 │ -0.72322051651408051 │
│  32 │ 86664.849 │ 87891.856 │  1.415807001521458833000 │
│  64 │ 79941.243 │ 72271.146 │ -9.594668173973727179000 │
└─┴───┴───┴──┘

The variation in the patched 64 client numbers is quite large, ranging
from ~66.5k to ~79.5k.  The highest number matched the unpatched
numbers which ranged 77.9k to 80k.  I wonder if that is noise and we
need to run longer (in which case the best outcome might be 'this
patch is neutral on FreeBSD'), or if something the patch does is doing
is causing that (for example maybe EVFILT_PROC proc filters causes
contention on the process table lock).

Matteo's results with the v6 patch on a low end NetBSD machine were
not good.  But the report at [1] implies that larger NetBSD and
OpenBSD systems have terrible problems with the
poll-postmaster-alive-pipe approach, which this EVFILT_PROC approach
would seem to address pretty well.

It's difficult to draw any conclusions at this point.

[1] https://www.postgresql.org/message-id/flat/20160915135755.GC19008%40genua.de

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

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

Re: [HACKERS] Showing parallel status in \df+

[Tom Lane]

Pavel Stehule  writes:
> We are in cycle because prosrc field is used for two independent features -
> and then it can be hard to find a agreement.

I thought pretty much everyone was on board with the idea of keeping
prosrc in \df+ for internal/C-language functions (and then probably
renaming the column, since it isn't actually source code in that case).
The argument is over what to do for PL functions, which is only one use
case not two.

regards, tom lane


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

Re: [HACKERS] compiler warning read_objtype_from_string()

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> I'm getting the following compiler warning (using nondefault
>> optimization options):
>> objectaddress.c: In function 'read_objtype_from_string':
>> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> return type;

> Umm.  I think it can only be uninitialized if we fall out of the end of
> the array, in which case we're supposed to throw the ERROR and never
> return.  Is that not working?

I do not think you should assume that the compiler is smart enough to
deduce that, nor that all compilers even know ereport(ERROR) doesn't
return.  Personally I don't see the point of the "type" variable at
all, anyway.  I would have written this as

inti;

for (i = 0; i < lengthof(ObjectTypeMap); i++)
{
if (strcmp(ObjectTypeMap[i].tm_name, objtype) == 0)
return ObjectTypeMap[i].tm_type;
}
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 errmsg("unrecognized object type \"%s\"", objtype)));
return -1;/* keep compiler quiet */

regards, tom lane


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
> >> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
> >>> I think the odds of getting to something that everyone would agree on
> >>> are nil, so I'm not excited about getting into that particular
> >>> bikeshed-painting discussion.  Look at the amount of trouble we're
> >>> having converging on a default for the regression tests, which are
> >>> a far narrower use-case than "everybody".
> 
> >> Well, practically anything that includes a PID and the timestamp is
> >> going to be an improvement over the status quo.  Just because we can't
> >> all agree on what would be perfect does not mean that we can't do
> >> better than what we've got now.  +1 for trying.
> 
> > Is there any chance we can move forward here, or is this effort doomed for 
> > now?
> 
> It seemed like nobody wanted to try to push this forward, and it will take
> somebody actively pushing, IMO, for something to happen.
> 
> Perhaps we should first try to get a consensus on the regression test
> use-case.

I thought Peter's suggestion for regression test drivers was a good one
and I see no reason to block that.  Why do you (Tom) object so strongly
against having a different one on buildfarm than elsewhere?  I'd rather
have buildfarm adopt the new suggestion than having buildfarm drive the
new stuff.

Adopting a default prefix is a different question.  For one thing IMHO
it should not have %a (application name).  Christoph's suggestion
(Debian's default) seemed good.

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


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

Re: [HACKERS] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Tom Lane wrote:

> I do not think you should assume that the compiler is smart enough to
> deduce that, nor that all compilers even know ereport(ERROR) doesn't
> return.  Personally I don't see the point of the "type" variable at
> all, anyway.  I would have written this as
> 
> [code]

Makes sense.  I will patch it that way.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Robert Haas]

On Wed, Sep 28, 2016 at 6:07 PM, Alvaro Herrera
 wrote:
> I thought Peter's suggestion for regression test drivers was a good one
> and I see no reason to block that.  Why do you (Tom) object so strongly
> against having a different one on buildfarm than elsewhere?  I'd rather
> have buildfarm adopt the new suggestion than having buildfarm drive the
> new stuff.
>
> Adopting a default prefix is a different question.  For one thing IMHO
> it should not have %a (application name).  Christoph's suggestion
> (Debian's default) seemed good.

Yeah, I like Cristoph's suggestion fine.  It meets my criteria of
"includes timestamp and PID" and overall seems reasonable.   If we
adopted that across the board, it wouldn't be too much different from
what Peter proposed for the regression test.  Just to compare.

Christoph/Debian:
log_line_prefix = '%t [%p-%l] %q%u@%d '
Peter:
log_line_prefix = '%t [%p]: [%l] %qapp=%a '

So Peter's got %p and %l separated by "]: [" whereas Christoph has
them separated only by a dash.  Presumably that's minor.  Then they've
both got %q.  After that, Christoph has %u@%d, which seems reasonable
for an actual system, and Peter's got app=%a, which is better for the
regression tests because the user name will depend on the UNIX
username of the person running the tests.

So how about we adopt both suggestions, except changing Peter's to '%t
[%p-%l] %qapp=%a ' so that they are a bit more similar?  I bet that
would make more people happier than it would make less happy.

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


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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Thomas Munro]

On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
 wrote:
> On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
>  wrote:
>>
>> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
>> >
>> > [training_wheels_004.patch]
>>
>> [review]

Ping.

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


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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote:
> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
>  wrote:
> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
> >  wrote:
> >>
> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
> >> >
> >> > [training_wheels_004.patch]
> >>
> >> [review]
> 
> Ping.

I'll have another revision out as soon as I get some more test cases.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Perhaps we should first try to get a consensus on the regression test
>> use-case.

> I thought Peter's suggestion for regression test drivers was a good one
> and I see no reason to block that.  Why do you (Tom) object so strongly
> against having a different one on buildfarm than elsewhere?  I'd rather
> have buildfarm adopt the new suggestion than having buildfarm drive the
> new stuff.

Well, my point is only that if you can't convince Andrew to sync the
buildfarm's choices with whatever your proposal is, then you haven't
got consensus.

regards, tom lane


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