Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 1:53 AM, Robert Haas  wrote:
> On Mon, Aug 21, 2017 at 5:30 PM, Simon Riggs  wrote:
>> Thinking ahead, are we going to add a new --no-objecttype switch every
>> time someone wants it?
>
> I'd personally be fine with --no-whatever for any whatever that might
> be a subsidiary property of database objects.  We've got
> --no-security-labels, --no-tablespaces, --no-owner, and
> --no-privileges already, so what's wrong with --no-comments?
>
> (We've also got --no-publications; I think it's arguable whether that
> is the same kind of thing.)

And --no-subscriptions in the same bucket.
-- 
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] Function to move the position of a replication slot

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 12:37 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 8/31/17 08:19, Magnus Hagander wrote:
>>> I think that, in the end, covered all the comments?
>
>> I didn't see any explanation of what this would actually be useful for.
>> I suppose you could skip over some changes you don't want replicated,
>> but how do you find to what position to skip?
>
> Um ... I can see how you might expect to skip some events in a logical
> replication stream and have a chance of things not being utterly broken.
> But how can that work for physical replication?  Missed updates are
> normally spelled "unrecoverable data corruption" at that level.

One use-case possible, even if it is easy to counter it by dropping
and recreating a slot, is to give up with what has been retained and
allow another client to reuse the same slot for a new standby.
-- 
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] configure issue - warnings sort: No such file or directory

2017-09-01 Thread Pavel Stehule
2017-09-02 6:27 GMT+02:00 Pavel Stehule :

>
>
> 2017-09-02 6:15 GMT+02:00 Tom Lane :
>
>> Pavel Stehule  writes:
>> > ./configure: line 15762: sort: No such file or directory
>> > ./configure: line 15759: sed: No such file or directory
>>
>> /bin not in your PATH, perhaps?
>>
>
> I have not, but sed/sort I can use and
>
>  export PATH="/bin:$PATH"
>
> doesn't help
>

but looks so it is Fedora26 issue - I see these lines elsewhere too.

Regards

Pavel



>
>> regards, tom lane
>>
>
>


Re: [HACKERS] configure issue - warnings sort: No such file or directory

2017-09-01 Thread Pavel Stehule
2017-09-02 6:15 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > ./configure: line 15762: sort: No such file or directory
> > ./configure: line 15759: sed: No such file or directory
>
> /bin not in your PATH, perhaps?
>

I have not, but sed/sort I can use and

 export PATH="/bin:$PATH"

doesn't help

>
> regards, tom lane
>


Re: [HACKERS] configure issue - warnings sort: No such file or directory

2017-09-01 Thread Tom Lane
Pavel Stehule  writes:
> ./configure: line 15762: sort: No such file or directory
> ./configure: line 15759: sed: No such file or directory

/bin not in your PATH, perhaps?

regards, tom lane


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


[HACKERS] configure issue - warnings sort: No such file or directory

2017-09-01 Thread Pavel Stehule
configure: using compiler=gcc (GCC) 7.1.1 20170622 (Red Hat 7.1.1-3)
configure: using CFLAGS=-Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -O2
configure: using CPPFLAGS= -D_GNU_SOURCE -I/usr/include/libxml2
configure: using LDFLAGS=  -Wl,--as-needed
./configure: line 15762: sort: No such file or directory
./configure: line 15759: sed: No such file or directory
configure: creating ./config.status
config.status: creating GNUmakefile
config.status: creating src/Makefile.global
config.status: creating src/include/pg_config.h
config.status: creating src/include/pg_config_ext.h
config.status: src/include/pg_config_ext.h is unchanged
config.status: creating src/interfaces/ecpg/include/ecpg_config.h
config.status: linking src/backend/port/tas/dummy.s to
src/backend/port/tas.s
config.status: linking src/backend/port/dynloader/linux.c to
src/backend/port/dynloader.c
config.status: linking src/backend/port/sysv_sema.c to
src/backend/port/pg_sema.c
config.status: linking src/backend/port/sysv_shmem.c to
src/backend/port/pg_shmem.c
config.status: linking src/backend/port/unix_latch.c to
src/backend/port/pg_latch.c
config.status: linking src/backend/port/dynloader/linux.h to
src/include/dynloader.h
config.status: linking src/include/port/linux.h to
src/include/pg_config_os.h
config.status: linking src/makefiles/Makefile.linux to src/Makefile.port
./configure: line 38: sort: No such file or directory
./configure: line 35: sed: No such file or directory

It is related to:

# The following way of writing the cache mishandles newlines in values,
# but we know of no workaround that is simple, portable, and efficient.
# So, we kill variables containing newlines.
# Ultrix sh set writes to stderr and can't be redirected directly,
# and sets the high bit in the cache file unless we assign to the vars.
(
  for ac_var in `(set) 2>&1 | sed -n
's/^\([a-zA-Z_][a-zA-Z0-9_]*\)=.*/\1/p'`; do
eval ac_val=\$$ac_var
case $ac_val in #(
*${as_nl}*)
  case $ac_var in #(
  *_cv_*) { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: cache
variable $ac_var contains a newline" >&5
$as_echo "$as_me: WARNING: cache variable $ac_var contains a newline" >&2;}
;;
  esac
  case $ac_var in #(
  _ | IFS | as_nl) ;; #(
  BASH_ARGV | BASH_SOURCE) eval $ac_var= ;; #(
  *) { eval $ac_var=; unset $ac_var;} ;;
  esac ;;
esac
  done

  (set) 2>&1 |
case $as_nl`(ac_space=' '; set) 2>&1` in #(
*${as_nl}ac_space=\ *)
  # `set' does not quote correctly, so add quotes: double-quote
  # substitution turns  into \\, and sed turns \\ into \.
  sed -n \
<-->"s/'/'''/g;
<-->  s/^\\([_$as_cr_alnum]*_cv_[_$as_cr_alnum]*\\)=\\(.*\\)/\\1='\\2'/p"
  ;; #(
*)
  # `set' quotes correctly as required by POSIX, so do not add quotes.
  sed -n "/^[_$as_cr_alnum]*_cv_[_$as_cr_alnum]*=/p"
  ;;
esac |
sort
) |
  sed '
 /^ac_cv_env_/b end
 t clear
 :clear
 s/^\([^=]*\)=\(.*[{}].*\)$/test "${\1+set}" = set || &/
 t end
 s/^\([^=]*\)=\(.*\)$/\1=${\1=\2}/
 :end' >>confcache

Regards

Pavel


Re: [HACKERS] Restricting maximum keep segments by repslots

2017-09-01 Thread Peter Eisentraut
I'm still concerned about how the critical situation is handled.  Your
patch just prints a warning to the log and then goes on -- doing what?
The warning rolls off the log, and then you have no idea what happened,
or how to recover.

I would like a flag in pg_replication_slots, and possibly also a
numerical column that indicates how far away from the critical point
each slot is.  That would be great for a monitoring system.

-- 
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] Secondary index access optimizations

2017-09-01 Thread Thomas Munro
On Wed, Aug 16, 2017 at 9:23 PM, Konstantin Knizhnik
 wrote:
> postgres=# explain select * from bt where k between 1 and 2 and v = 100;
>   QUERY PLAN
> --
>  Append  (cost=0.29..15.63 rows=2 width=8)
>->  Index Scan using dti1 on dt1  (cost=0.29..8.30 rows=1 width=8)
>  Index Cond: (v = 100)
>->  Index Scan using dti2 on dt2  (cost=0.29..7.33 rows=1 width=8)
>  Index Cond: (v = 100)
>  Filter: (k <= 2)
> (6 rows)

+1

This seems like a good feature to me: filtering stuff that is
obviously true is a waste of CPU cycles and may even require people to
add redundant stuff to indexes.  I was pondering something related to
this over in the partition-wise join thread (join quals that are
implied by partition constraints and should be discarded).

It'd be interesting to get Amit Langote's feedback, so I CC'd him.
I'd be surprised if he and others haven't got a plan or a patch for
this down the back of the sofa.

I might be missing some higher level architectural problems with the
patch, but for what it's worth here's some feedback after a first read
through:

--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1441,6 +1441,20 @@ relation_excluded_by_constraints(PlannerInfo *root,
 if (predicate_refuted_by(safe_constraints, rel->baserestrictinfo, false))
 return true;

+/*
+ * Remove from restrictions list items implied by table constraints
+ */
+safe_restrictions = NULL;
+foreach(lc, rel->baserestrictinfo)
+{
+RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);

I think the new way to write this is "RestrictInfo *rinfo =
lfirst_node(RestrictInfo, lc)".

+if (!predicate_implied_by(list_make1(rinfo->clause),
safe_constraints, false)) {
+safe_restrictions = lappend(safe_restrictions, rinfo);
+}
+}
+rel->baserestrictinfo = safe_restrictions;

It feels wrong to modify rel->baserestrictinfo in
relation_excluded_by_constraints().  I think there should probably be
a function with a name that more clearly indicates that it mutates its
input, and you should call that separately after
relation_excluded_by_constraints().  Something like
remove_restrictions_implied_by_constraints()?

> It is because operator_predicate_proof is not able to understand that k <
> 20001 and k <= 2 are equivalent for integer type.
>
> [...]
>
>  /*
>   * operator_predicate_proof
>  if (clause_const->constisnull)
>  return false;
>
> +if (!refute_it
> +&& ((pred_op == Int4LessOrEqualOperator && clause_op ==
> Int4LessOperator)
> +|| (pred_op == Int8LessOrEqualOperator && clause_op ==
> Int8LessOperator)
> +|| (pred_op == Int2LessOrEqualOperator && clause_op ==
> Int2LessOperator))
> +&& pred_const->constbyval && clause_const->constbyval
> +&& pred_const->constvalue + 1 == clause_const->constvalue)
> +{
> +return true;
> +}
> +

I'm less sure about this part.  It seems like a slippery slope.

A couple of regression test failures:

 inherit  ... FAILED
 rowsecurity  ... FAILED

 2 of 179 tests failed.


I didn't try to understand the rowsecurity one, but at first glance
all the differences reported in the inherit test are in fact cases
where your patch is doing the right thing and removing redundant
filters from scans.  Nice!

-- 
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] Function to move the position of a replication slot

2017-09-01 Thread Tom Lane
Peter Eisentraut  writes:
> On 8/31/17 08:19, Magnus Hagander wrote:
>> I think that, in the end, covered all the comments?

> I didn't see any explanation of what this would actually be useful for.
> I suppose you could skip over some changes you don't want replicated,
> but how do you find to what position to skip?

Um ... I can see how you might expect to skip some events in a logical
replication stream and have a chance of things not being utterly broken.
But how can that work for physical replication?  Missed updates are
normally spelled "unrecoverable data corruption" at that level.

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] Function to move the position of a replication slot

2017-09-01 Thread Peter Eisentraut
On 8/31/17 08:19, Magnus Hagander wrote:
> Rebased. Now named pg_advance_replication_slot. ERROR on logical slots.
> Forward only. 
> 
> I think that, in the end, covered all the comments?

I didn't see any explanation of what this would actually be useful for.
I suppose you could skip over some changes you don't want replicated,
but how do you find to what position to skip?

Logical replication has a similar mechanism, using the function
pg_replication_origin_advance().  Any overlap there?  (Maybe the names
could be aligned.)
(https://www.postgresql.org/docs/devel/static/logical-replication-conflicts.html)

-- 
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] memory fields from getrusage()

2017-09-01 Thread Justin Pryzby
On Sat, Sep 02, 2017 at 02:00:44PM +1200, Thomas Munro wrote:
> On Sat, Sep 2, 2017 at 7:46 AM, Peter Eisentraut
>  wrote:
> > On 6/15/17 10:58, Justin Pryzby wrote:
> >> On Thu, Jun 15, 2017 at 10:29:21AM -0400, Robert Haas wrote:
> >>> On Wed, Jun 14, 2017 at 6:28 PM, Justin Pryzby  
> >>> wrote:
>  On Tue, Jun 13, 2017 at 12:16:00PM -0400, Robert Haas wrote:
> > It might be worth adding platform-specific code for common platforms.
> 
>  All I care (which linux happily/happens to support) is maxrss; I was 
>  probably
>  originally interested in this while digging into an issue with hash agg.
> >>>
> >>> I don't think it needs to go in a separate file.  I'd just patch 
> >>> ShowUsage().
> >
> > I have committed a patch that shows maxrss, with /1024 adjustment for
> > macOS.  That should cover all platforms that I could find.(*)
> 
> Apparently ru_maxrss is in *pages* on Solaris-derived systems:
> 
> https://illumos.org/man/3c/getrusage

..but note that that:
"The ru_maxrss, ru_ixrss, ru_idrss, and ru_isrss members of the rusage
structure are set to 0 in this implementation."

Same here:
https://docs.oracle.com/cd/E23823_01/html/816-5168/getrusage-3c.html

..and earlier solaris docs don't seem to mention getrusage at all (?)

posix docs say:
http://pubs.opengroup.org/onlinepubs/009695399/functions/getrusage.html
|CHANGE HISTORY
|First released in Issue 4, Version 2.

..which I gather is SUSv1 c. 1995

Curiously, GNU time reported maxrss too high by a factor of 4 (and that was
present in early centos6.X)
https://bugzilla.redhat.com/show_bug.cgi?id=702826

Justin


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


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

2017-09-01 Thread Amit Kapila
On Fri, Sep 1, 2017 at 9:17 PM, Robert Haas  wrote:
> On Fri, Sep 1, 2017 at 10:03 AM, Dilip Kumar  wrote:
>>> Sure will do so.  In the meantime, I have rebased the patch.
>>
>> I have repeated some of the tests we have performed earlier.
>

Thanks for repeating the performance tests.

> OK, these tests seem to show that this is still working.  Committed,
> again.  Let's hope this attempt goes better than the last one.
>

Thanks for committing.


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


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


Re: [HACKERS] memory fields from getrusage()

2017-09-01 Thread Thomas Munro
On Sat, Sep 2, 2017 at 7:46 AM, Peter Eisentraut
 wrote:
> On 6/15/17 10:58, Justin Pryzby wrote:
>> On Thu, Jun 15, 2017 at 10:29:21AM -0400, Robert Haas wrote:
>>> On Wed, Jun 14, 2017 at 6:28 PM, Justin Pryzby  wrote:
 On Tue, Jun 13, 2017 at 12:16:00PM -0400, Robert Haas wrote:
> It might be worth adding platform-specific code for common platforms.

 All I care (which linux happily/happens to support) is maxrss; I was 
 probably
 originally interested in this while digging into an issue with hash agg.
>>>
>>> I don't think it needs to go in a separate file.  I'd just patch 
>>> ShowUsage().
>
> I have committed a patch that shows maxrss, with /1024 adjustment for
> macOS.  That should cover all platforms that I could find.(*)

Apparently ru_maxrss is in *pages* on Solaris-derived systems:

https://illumos.org/man/3c/getrusage

AIX seems to be like Linux and FreeBSD (kilobytes):

https://www.ibm.com/support/knowledgecenter/en/ssw_aix_61/com.ibm.aix.basetrf1/getrusage_64.htm

-- 
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] Adding support for Default partition in partitioning

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas  wrote:
> On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
>  wrote:
>> 0001:
>> This patch refactors RelationBuildPartitionDesc(), basically this is patch
>> 0001 of default range partition[1].
>
> I spent a while studying this; it seems to be simpler and there's no
> real downside.  So, committed.

BTW, the rest of this series seems to need a rebase.  The changes to
insert.sql conflicted with 30833ba154e0c1106d61e3270242dc5999a3e4f3.

-- 
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] Parallel Hash take II

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 7:42 PM, Thomas Munro
 wrote:
>> I'm thinking about something like this:
>>
>> Gather
>> -> Nested Loop
>>   -> Parallel Seq Scan
>>   -> Hash Join
>> -> Seq Scan
>> -> Parallel Hash
>>   -> Parallel Seq Scan
>>
>> The hash join has to be rescanned for every iteration of the nested loop.
>
> I think you mean:
>
>  Gather
>  -> Nested Loop
>-> Parallel Seq Scan
>-> Parallel Hash Join
>  -> Parallel Seq Scan
>  -> Parallel Hash
>-> Parallel Seq Scan

I don't, though, because that's nonsense.  Maybe what I wrote is also
nonsense, but it is at least different nonsense.

Let's try it again with some table names:

Gather
-> Nested Loop
  -> Parallel Seq Scan on a
  -> (Parallel?) Hash Join
-> Seq Scan on b (NOT A PARALLEL SEQ SCAN)
-> Parallel Hash
  -> Parallel Seq Scan on c

I argue that this is a potentially valid plan.  b, of course, has to
be scanned in its entirety by every worker every time through, which
is why it's not a Parallel Seq Scan, but that requirement does not
apply to c.  If we take all the rows in c and stick them into a
DSM-based hash table, we can reuse them every time the hash join is
rescanned and, AFAICS, that should work just fine, and it's probably a
win over letting each worker build a separate copy of the hash table
on c, too.

Of course, there's the "small" problem that I have no idea what to do
if the b-c join is (or becomes) multi-batch.  When I was thinking
about this before, I was imagining that this case might Just Work with
your patch provided that you could generate a plan shaped like this,
but now I see that that's not actually true, because of multiple
batches.

-- 
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] Optional message to user when terminating/cancelling backend

2017-09-01 Thread Thomas Munro
On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson  wrote:
> Good point.  I’ve attached a new version which issues a NOTICE on truncation
> and also addresses all other comments so far in this thread.  Thanks a lot for
> the early patch reviews!

FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:

make[3]: Entering directory
`/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
772
972
make[3]: *** [postgres.bki] Error 1

-- 
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] Protect syscache from bloating with negative cache entries

2017-09-01 Thread Thomas Munro
On Mon, Aug 28, 2017 at 9:24 PM, Kyotaro HORIGUCHI
 wrote:
> This patch have had interferences from several commits after the
> last submission. I amended this patch to follow them (up to
> f97c55c), removed an unnecessary branch and edited some comments.

Hi Kyotaro-san,

This applies but several regression tests fail for me.  Here is a
sample backtrace:

frame #3: 0x00010f0614c0
postgres`ExceptionalCondition(conditionName="!(attnum < 0 ? attnum ==
(-2) : cache->cc_tupdesc->attrs[attnum].atttypid == 26)",
errorType="FailedAssertion", fileName="catcache.c", lineNumber=1384) +
128 at assert.c:54
frame #4: 0x00010f03b5fd
postgres`CollectOIDsForHashValue(cache=0x7fe273821268,
hashValue=994410284, attnum=0) + 253 at catcache.c:1383
frame #5: 0x00010f055e8e
postgres`SysCacheSysCacheInvalCallback(arg=140610577303984, cacheid=0,
hashValue=994410284) + 94 at syscache.c:1692
frame #6: 0x00010f03fbbb
postgres`CallSyscacheCallbacks(cacheid=0, hashvalue=994410284) + 219
at inval.c:1468
frame #7: 0x00010f03f878
postgres`LocalExecuteInvalidationMessage(msg=0x7fff51213ff8) + 88
at inval.c:566
frame #8: 0x00010ee7a3f2
postgres`ReceiveSharedInvalidMessages(invalFunction=(postgres`LocalExecuteInvalidationMessage
at inval.c:555), resetFunction=(postgres`InvalidateSystemCaches at
inval.c:647)) + 354 at sinval.c:121
frame #9: 0x00010f03fcb7 postgres`AcceptInvalidationMessages +
23 at inval.c:686
frame #10: 0x00010eade609 postgres`AtStart_Cache + 9 at xact.c:987
frame #11: 0x00010ead8c2f postgres`StartTransaction + 655 at xact.c:1921
frame #12: 0x00010ead8896 postgres`StartTransactionCommand +
70 at xact.c:2691
frame #13: 0x00010eea9746 postgres`start_xact_command + 22 at
postgres.c:2438
frame #14: 0x00010eea722e
postgres`exec_simple_query(query_string="RESET SESSION
AUTHORIZATION;") + 126 at postgres.c:913
frame #15: 0x00010eea68d7 postgres`PostgresMain(argc=1,
argv=0x7fe2738036a8, dbname="regression", username="munro") + 2375
at postgres.c:4090
frame #16: 0x00010eded40e
postgres`BackendRun(port=0x7fe2716001a0) + 654 at
postmaster.c:4357
frame #17: 0x00010edec793
postgres`BackendStartup(port=0x7fe2716001a0) + 483 at
postmaster.c:4029
frame #18: 0x00010edeb785 postgres`ServerLoop + 597 at postmaster.c:1753
frame #19: 0x00010ede8f71 postgres`PostmasterMain(argc=8,
argv=0x7fe271403860) + 5553 at postmaster.c:1361
frame #20: 0x00010ed0ccd9 postgres`main(argc=8,
argv=0x7fe271403860) + 761 at main.c:228
frame #21: 0x7fff8333a5ad libdyld.dylib`start + 1

-- 
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] More replication race conditions

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 12:03 AM, Alvaro Herrera  wrote:
> Michael Paquier wrote:
>> On Mon, Aug 28, 2017 at 8:25 AM, Michael Paquier
>>  wrote:
>> > Today's run has finished with the same failure:
>> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
>> > Attached is a patch to make this code path wait that the transaction
>> > has been replayed. We could use as well synchronous_commit = apply,
>> > but I prefer the solution of this patch with a wait query.
>>
>> I have added an open item for this issue for PG10. The 2PC tests in
>> src/test/recovery are new in PG10, introduced by 3082098.
>
> Thanks, pushed.

Thanks, the error has now not happened for 5 runs. Let's see if it
ever shows up again.
-- 
Michael


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


Re: [HACKERS] Rename RECOVERYXLOG to RECOVERYWAL?

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 3:06 AM, Robert Haas  wrote:
> I don't think this really buys us anything.  If we'd applied it to v10
> maybe, but what do we get out of whacking it around now?
>
> "Consistency", I hear you cry!  Fair point.  But we never had a goal
> of eliminating all internal references to "xlog", just the user-facing
> ones.  And since RECOVERYXLOG is not documented, I think there's a
> good argument that it's not user-facing.  You could argue that since
> it shows up in the file system it's implicitly user-facing, and maybe
> you're right; if some other committer really wants to make this
> change, I won't grouse much.  But personally I'd favor leaving it
> alone to avoid having the behavior change a little bit in every new
> release.

I may be wrong, but I would suspect that some backup tools doing
FS-level backup are checking on the existence of this file and skip
it. From the point of view of operations, it does not matter much as
any existing RECOVERYXLOG is unlinked before being replaced by a new
one, but that would not be nice to add silently 16MB in each backup.
-- 
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] Upcoming commit fest will begin soon

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 1:46 AM, Andres Freund  wrote:
> On 2017-09-01 16:14:25 +0200, Daniel Gustafsson wrote:
>> > On 01 Sep 2017, at 05:33, Michael Paquier  
>> > wrote:
>> >
>> > Another question is: who would like to become the CF manager for this
>> > time? This commit fest is very large, so it is going to be difficult
>> > for one person to do the whole thing, hence at least 2 people would be
>> > welcome. Please note that I am not directly volunteering for this one,
>> > but per my number of patches I need to look at many things, so I may
>> > be considered as a sort of co-manager ;)
>> > New heads for this exercise would be nice though.
>>
>> I volunteer to be co-managing this CF.  Since I’ve never done it before, and
>> the CF in question is rather large I wouldn’t mind it co-managing it with
>> someone as you mention.
>
> Where do I send the booze / xanax / ...?  Thanks for doing this!

Thanks for volunteering, Daniel. This is a difficult task. As you are
yourself familiar with the community process in reviewing patches, I
think you'll do fine.
-- 
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] Adding -E switch to pg_dumpall

2017-09-01 Thread Michael Paquier
On Sat, Sep 2, 2017 at 1:32 AM, Robert Haas  wrote:
> On Fri, Jul 21, 2017 at 9:21 AM, Michael Paquier
>  wrote:
>> On Thu, Jul 20, 2017 at 11:17 AM, Fabien COELHO  wrote:
>>> Ok for me. I switched the status to "Ready for committers".
>>
>> Thanks for the review, Fabien.
>
> LGTM.  Committed.

Thanks for the commit.
-- 
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] Parallel Hash take II

2017-09-01 Thread Thomas Munro
On Sat, Sep 2, 2017 at 10:45 AM, Robert Haas  wrote:
> On Fri, Sep 1, 2017 at 6:32 PM, Thomas Munro
>  wrote:
>> On Sat, Sep 2, 2017 at 5:13 AM, Robert Haas  wrote:
>>> On Thu, Aug 31, 2017 at 8:53 AM, Thomas Munro
>>>  wrote:
 Check out ExecReScanGather(): it shuts down and waits for all workers
 to complete, which makes the assumptions in ExecReScanHashJoin() true.
 If a node below Gather but above Hash Join could initiate a rescan
 then the assumptions would not hold.  I am not sure what it would mean
 though and we don't generate any such plans today to my knowledge.  It
 doesn't seem to make sense for the inner side of Nested Loop to be
 partial.  Have I missed something here?
>>>
>>> I bet this could happen, although recent commits have demonstrated
>>> that my knowledge of how PostgreSQL handles rescans is less than
>>> compendious.  Suppose there's a Nested Loop below the Gather and above
>>> the Hash Join, implementing a join condition that can't give rise to a
>>> parameterized path, like a.x + b.x = 0.
>>
>> Hmm.  I still don't see how that could produce a rescan of a partial
>> path without an intervening Gather, and I would really like to get to
>> the bottom of this.
>
> I'm thinking about something like this:
>
> Gather
> -> Nested Loop
>   -> Parallel Seq Scan
>   -> Hash Join
> -> Seq Scan
> -> Parallel Hash
>   -> Parallel Seq Scan
>
> The hash join has to be rescanned for every iteration of the nested loop.

I think you mean:

 Gather
 -> Nested Loop
   -> Parallel Seq Scan
   -> Parallel Hash Join
 -> Parallel Seq Scan
 -> Parallel Hash
   -> Parallel Seq Scan

... but we can't make plans like that and they would produce nonsense
output.  The Nested Loop's inner plan is partial, but
consider_parallel_nestloop only makes plans with parallel-safe but
non-partial ("complete") inner paths.

/*
 * consider_parallel_nestloop
 *Try to build partial paths for a joinrel by joining a
partial path for the
 *outer relation to a complete path for the inner relation.
 *

-- 
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] Parallel Hash take II

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 6:32 PM, Thomas Munro
 wrote:
> On Sat, Sep 2, 2017 at 5:13 AM, Robert Haas  wrote:
>> On Thu, Aug 31, 2017 at 8:53 AM, Thomas Munro
>>  wrote:
>>> Check out ExecReScanGather(): it shuts down and waits for all workers
>>> to complete, which makes the assumptions in ExecReScanHashJoin() true.
>>> If a node below Gather but above Hash Join could initiate a rescan
>>> then the assumptions would not hold.  I am not sure what it would mean
>>> though and we don't generate any such plans today to my knowledge.  It
>>> doesn't seem to make sense for the inner side of Nested Loop to be
>>> partial.  Have I missed something here?
>>
>> I bet this could happen, although recent commits have demonstrated
>> that my knowledge of how PostgreSQL handles rescans is less than
>> compendious.  Suppose there's a Nested Loop below the Gather and above
>> the Hash Join, implementing a join condition that can't give rise to a
>> parameterized path, like a.x + b.x = 0.
>
> Hmm.  I still don't see how that could produce a rescan of a partial
> path without an intervening Gather, and I would really like to get to
> the bottom of this.

I'm thinking about something like this:

Gather
-> Nested Loop
  -> Parallel Seq Scan
  -> Hash Join
-> Seq Scan
-> Parallel Hash
  -> Parallel Seq Scan

The hash join has to be rescanned for every iteration of the nested loop.

Maybe I'm confused.

-- 
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] Parallel Hash take II

2017-09-01 Thread Thomas Munro
On Sat, Sep 2, 2017 at 5:13 AM, Robert Haas  wrote:
> On Thu, Aug 31, 2017 at 8:53 AM, Thomas Munro
>  wrote:
>> Check out ExecReScanGather(): it shuts down and waits for all workers
>> to complete, which makes the assumptions in ExecReScanHashJoin() true.
>> If a node below Gather but above Hash Join could initiate a rescan
>> then the assumptions would not hold.  I am not sure what it would mean
>> though and we don't generate any such plans today to my knowledge.  It
>> doesn't seem to make sense for the inner side of Nested Loop to be
>> partial.  Have I missed something here?
>
> I bet this could happen, although recent commits have demonstrated
> that my knowledge of how PostgreSQL handles rescans is less than
> compendious.  Suppose there's a Nested Loop below the Gather and above
> the Hash Join, implementing a join condition that can't give rise to a
> parameterized path, like a.x + b.x = 0.

Hmm.  I still don't see how that could produce a rescan of a partial
path without an intervening Gather, and I would really like to get to
the bottom of this.

At the risk of mansplaining the code that you wrote and turning out to
be wrong:  A Nested Loop can't ever have a partial path on the inner
side.  Under certain circumstances it can have a partial path on the
outer side, because its own results are partial, but for each outer
row it needs to do a total (non-partial) scan of the inner side so
that it can reliably find or not find matches.  Therefore we'll never
rescan partial paths directly, we'll only ever rescan partial paths
indirectly via a Gatheroid node that will synchronise the rescan of
all children to produce a non-partial result.

There may be more reasons to rescan that I'm not thinking of.  But the
whole idea of a rescan seems to make sense only for non-partial paths.
What would it even mean for a worker process to decide to rescan (say)
a Seq Scan without any kind of consensus?

Thought experiment: I suppose we could consider replacing Gather's
clunky shut-down-and-relaunch-workers synchronisation technique with a
new protocol where the Gather node sends a 'rescan!' message to each
worker and then discards their tuples until it receives 'OK, rescan
starts here', and then each parallel-aware node type supplies its own
rescan synchronisation logic as appropriate.  For example, Seq Scan
would somehow need to elect one participant to run
heap_parallelscan_reinitialize and others would wait until it has
done.  This might not be worth the effort, but thinking about this
problem helped me see that rescan of a partial plan without a Gather
node to coordinate doesn't make any sense.

Am I wrong?

-- 
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] pg_basebackup throttling doesn't throttle as promised

2017-09-01 Thread Jeff Janes
On Fri, Sep 1, 2017 at 1:32 PM, Jeff Janes  wrote:

> The "-r" option to pg_basebackup is supposed to throttle the rate of the
> backup.  But it only works properly if the server is mostly idle.
>
> Every non-trivial call to XLogFlush or XLogBackgroundFlush will wake up
> the wal sender (the one which is not really sending wal, but base files),
> and the throttling routine doesn't go back to sleep after being awoke
> early.  Rather, it releases another 32kb of data.
>
>
> Should the basebackup.c throttle sleep in a loop until its time has
> expired?
>
> Or should walsender.c WalSndWakeup not wake a wal sender whose status
> is WALSNDSTATE_BACKUP?
>
> Or both?
>


I'm attaching a patch for each option.  Each one independently solves the
problem.  But I think we should do both.  There is no point in issuing
unnecessary kill system calls, and there may also be more spurious wake-ups
than just these ones.

Cheers,

Jeff


pg_basebackup_throttle_1_v1.patch
Description: Binary data


pg_basebackup_throttle_2_v1.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] tupconvert.c API change in v10 release notes

2017-09-01 Thread Bruce Momjian
On Wed, Jul 19, 2017 at 12:39:07PM -0400, Tom Lane wrote:
> Justin Pryzby  writes:
> > FYI, I happened across this commit comment:
> > 3f902354b08ac788600f0ae54fcbfc1d4e3ea765
> > |   So, let's accept the removal of the guarantee about
> > |   the output tuple's rowtype marking, recognizing that this is a API 
> > change
> > |   that could conceivably break third-party callers of tupconvert.c.  (So,
> > |   let's remember to mention it in the v10 release notes.)
> 
> > ..but couldn't see that the commit or change is so referenced.
> 
> Yeah, I see nothing about 3f902354b in release-10.sgml either.
> We've had varying policies over the years about whether to mention
> internal API changes in the release notes or not, but this one
> I think does belong there, since it's a silent API break rather
> than one that would easily be caught due to compiler errors.
> Bruce, did you have any specific reasoning for leaving it out?

I doubt I saw that sentence in the paragraph.  For long text like that,
I am usually looking for "BACKWARDS INCOMPATIBLE CHANGE" or something
like that.  Sorry I missed it.

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

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


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


[HACKERS] pg_basebackup throttling doesn't throttle as promised

2017-09-01 Thread Jeff Janes
The "-r" option to pg_basebackup is supposed to throttle the rate of the
backup.  But it only works properly if the server is mostly idle.

Every non-trivial call to XLogFlush or XLogBackgroundFlush will wake up the
wal sender (the one which is not really sending wal, but base files), and
the throttling routine doesn't go back to sleep after being awoke early.
Rather, it releases another 32kb of data.


Should the basebackup.c throttle sleep in a loop until its time has
expired?

Or should walsender.c WalSndWakeup not wake a wal sender whose status
is WALSNDSTATE_BACKUP?

Or both?

Cheers,

Jeff


Re: [HACKERS] memory fields from getrusage()

2017-09-01 Thread Peter Eisentraut
On 6/15/17 10:58, Justin Pryzby wrote:
> On Thu, Jun 15, 2017 at 10:29:21AM -0400, Robert Haas wrote:
>> On Wed, Jun 14, 2017 at 6:28 PM, Justin Pryzby  wrote:
>>> On Tue, Jun 13, 2017 at 12:16:00PM -0400, Robert Haas wrote:
 It might be worth adding platform-specific code for common platforms.
>>>
>>> All I care (which linux happily/happens to support) is maxrss; I was 
>>> probably
>>> originally interested in this while digging into an issue with hash agg.
>>
>> I don't think it needs to go in a separate file.  I'd just patch ShowUsage().

I have committed a patch that shows maxrss, with /1024 adjustment for
macOS.  That should cover all platforms that I could find.(*)

I omitted the i{x,d,s}rss stuff.  My understanding is that to show them
you should divide the numbers by the elapsed time in some specific way.
Seeing that neither Linux or macOS fill these numbers in, I lost
interest.  Someone could produce a separate patch to address this.

Since you were only interested in maxrss to begin with, I think this
thread is concluded.(*)

(*) modulo buildfarm wrath

-- 
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] Adding support for Default partition in partitioning

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
 wrote:
> 0001:
> This patch refactors RelationBuildPartitionDesc(), basically this is patch
> 0001 of default range partition[1].

I spent a while studying this; it seems to be simpler and there's no
real downside.  So, committed.

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


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


Re: [HACKERS] advanced partition matching algorithm for partition-wise join

2017-09-01 Thread Ashutosh Bapat
PFA the patches rebased on the latest sources. There are also fixes
for some of the crashes and bugs reported. I haven't yet included the
testcase patch in the main patchset.

On Mon, Aug 28, 2017 at 12:44 PM, Rajkumar Raghuwanshi
 wrote:
> On Mon, Aug 21, 2017 at 12:43 PM, Ashutosh Bapat
>  wrote:
>>
>> TODOs
>> ---
>> 1. Add tests for advanced partition matching algorithm
>
>
> Hi Ashutosh,
>
> I have applied all partition-wise-join patches (v26) and tested feature. I
> have modified partition_join.sql file and added extra test cases to test
> partition matching.
>
> Attaching WIP test case patch which as of now have some server crashes and a
> data corruptions issue which is commented in the file itself and need to be
> removed once issue got solved. Also some of queries is not picking or
> picking partition-wise-join as per expectation which may need some
> adjustment.
>
>
>
>
>
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
From 865242c79b56f021dc619bc028480097d11bb69a Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Thu, 6 Jul 2017 14:15:22 +0530
Subject: [PATCH 11/12] Modify bound comparision functions to accept members
 of PartitionKey

Functions partition_bound_cmp(), partition_rbound_cmp() and
partition_rbound_datum_cmp() are required to merge partition bounds
from joining relations. While doing so, we do not have access to the
PartitionKey of either relations. So, modify these functions to accept
only required members of PartitionKey so that the functions can be
reused for merging bounds.

Ashutosh Bapat.
---
 src/backend/catalog/partition.c |   76 ++-
 1 file changed, 44 insertions(+), 32 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 96a64ce..d42e1b5 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -126,15 +126,17 @@ static List *generate_partition_qual(Relation rel);
 
 static PartitionRangeBound *make_one_range_bound(PartitionKey key, int index,
 	 List *datums, bool lower);
-static int32 partition_rbound_cmp(PartitionKey key,
-	 Datum *datums1, PartitionRangeDatumKind *kind1,
-	 bool lower1, PartitionRangeBound *b2);
-static int32 partition_rbound_datum_cmp(PartitionKey key,
-		   Datum *rb_datums, PartitionRangeDatumKind *rb_kind,
+static int32 partition_rbound_cmp(int partnatts, FmgrInfo *partsupfunc,
+	 Oid *partcollation, Datum *datums1,
+	 PartitionRangeDatumKind *kind1, bool lower1,
+	 PartitionRangeBound *b2);
+static int32 partition_rbound_datum_cmp(int partnatts, FmgrInfo *partsupfunc,
+		   Oid *partcollation, Datum *rb_datums,
+		   PartitionRangeDatumKind *rb_kind,
 		   Datum *tuple_datums);
 
-static int32 partition_bound_cmp(PartitionKey key,
-	PartitionBoundInfo boundinfo,
+static int32 partition_bound_cmp(int partnatts, FmgrInfo *partsupfunc,
+	Oid *partcollation, PartitionBoundInfo boundinfo,
 	int offset, void *probe, bool probe_is_bound);
 static int partition_bound_bsearch(PartitionKey key,
 		PartitionBoundInfo boundinfo,
@@ -719,8 +721,9 @@ check_new_partition_bound(char *relname, Relation parent,
  * First check if the resulting range would be empty with
  * specified lower and upper bounds
  */
-if (partition_rbound_cmp(key, lower->datums, lower->kind, true,
-		 upper) >= 0)
+if (partition_rbound_cmp(key->partnatts, key->partsupfunc,
+		 key->partcollation, lower->datums,
+		 lower->kind, true, upper) >= 0)
 {
 	ereport(ERROR,
 			(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
@@ -771,9 +774,11 @@ check_new_partition_bound(char *relname, Relation parent,
 		{
 			int32		cmpval;
 
-			cmpval = partition_bound_cmp(key, boundinfo,
-		 offset + 1, upper,
-		 true);
+			cmpval = partition_bound_cmp(key->partnatts,
+		 key->partsupfunc,
+		 key->partcollation,
+		 boundinfo, offset + 1,
+		 upper, true);
 			if (cmpval < 0)
 			{
 /*
@@ -2138,7 +2143,9 @@ qsort_partition_rbound_cmp(const void *a, const void *b, void *arg)
 	PartitionRangeBound *b2 = (*(PartitionRangeBound *const *) b);
 	PartitionKey key = (PartitionKey) arg;
 
-	return partition_rbound_cmp(key, b1->datums, b1->kind, b1->lower, b2);
+	return partition_rbound_cmp(key->partnatts, key->partsupfunc,
+key->partcollation, b1->datums, b1->kind,
+b1->lower, b2);
 }
 
 /*
@@ -2155,7 +2162,7 @@ qsort_partition_rbound_cmp(const void *a, const void *b, void *arg)
  * two contiguous partitions.
  */
 static int32
-partition_rbound_cmp(PartitionKey key,
+partition_rbound_cmp(int partnatts, FmgrInfo *partsupfunc, Oid *partcollation,
 	 Datum *datums1, PartitionRangeDatumKind *kind1,
 	 bool lower1, 

Re: [HACKERS] Rename RECOVERYXLOG to RECOVERYWAL?

2017-09-01 Thread David Steele
On 9/1/17 2:06 PM, Robert Haas wrote:
> On Fri, Sep 1, 2017 at 12:57 PM, David Steele  wrote:
>> I searched the various threads on the xlog -> wal rename and I couldn't
>> find any specific mention of why this was not renamed.
>>
>> I have attached a patch in case it was an oversight rather than left
>> as-is on purpose.
> 
> I don't think this really buys us anything.  If we'd applied it to v10
> maybe, but what do we get out of whacking it around now?

I was thinking it would be applied to v10.

> "Consistency", I hear you cry!  Fair point.  But we never had a goal
> of eliminating all internal references to "xlog", just the user-facing
> ones.  And since RECOVERYXLOG is not documented, I think there's a
> good argument that it's not user-facing.  You could argue that since
> it shows up in the file system it's implicitly user-facing, and maybe
> you're right; 

That's exactly my argument, in fact!

> if some other committer really wants to make this
> change, I won't grouse much.  But personally I'd favor leaving it
> alone to avoid having the behavior change a little bit in every new
> release.

Seems like since v10 is still beta and this is not really documented it
wouldn't be that big a deal to make the change.  If nothing else it
might keep the question from coming up in the future.

I'm not going to make a big fuss about it, though.  I noticed it while
testing the v10 support in pgbackRest and thought it was worth bringing up.

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


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


Re: [HACKERS] Missing SIZE_MAX

2017-09-01 Thread Tom Lane
I wrote:
> Robert Haas  writes:
>> It might be worth the effort to clean all of this up, just because the
>> next person who gets bitten by it may not be as smart as you are.

> Yeah.  I was just thinking that maybe the appropriate investment of
> effort is to make [U]INT64CONST smarter, so that it results in a
> properly-suffixed constant and doesn't need a cast.  Then it'd be a
> lot easier to make these other macros be #if-safe.

Actually, that looks easier than I thought.  The current approach to
[U]INT64CONST dates to before we were willing to require the compiler
to have working 64-bit support.  I think that now we can just assume
that either an L/UL or LL/ULL suffix will work, as in the attached
draft.  (This'd allow dropping configure's HAVE_LL_CONSTANTS probe
entirely, but I didn't do that yet.)

regards, tom lane

diff --git a/src/include/c.h b/src/include/c.h
index 4f8bbfc..4fb8ef0 100644
*** a/src/include/c.h
--- b/src/include/c.h
*** typedef long int int64;
*** 288,293 
--- 288,295 
  #ifndef HAVE_UINT64
  typedef unsigned long int uint64;
  #endif
+ #define INT64CONST(x)  (x##L)
+ #define UINT64CONST(x) (x##UL)
  #elif defined(HAVE_LONG_LONG_INT_64)
  /* We have working support for "long long int", use that */
  
*** typedef long long int int64;
*** 297,316 
  #ifndef HAVE_UINT64
  typedef unsigned long long int uint64;
  #endif
  #else
  /* neither HAVE_LONG_INT_64 nor HAVE_LONG_LONG_INT_64 */
  #error must have a working 64-bit integer datatype
  #endif
  
- /* Decide if we need to decorate 64-bit constants */
- #ifdef HAVE_LL_CONSTANTS
- #define INT64CONST(x)  ((int64) x##LL)
- #define UINT64CONST(x) ((uint64) x##ULL)
- #else
- #define INT64CONST(x)  ((int64) x)
- #define UINT64CONST(x) ((uint64) x)
- #endif
- 
  /* snprintf format strings to use for 64-bit integers */
  #define INT64_FORMAT "%" INT64_MODIFIER "d"
  #define UINT64_FORMAT "%" INT64_MODIFIER "u"
--- 299,311 
  #ifndef HAVE_UINT64
  typedef unsigned long long int uint64;
  #endif
+ #define INT64CONST(x)  (x##LL)
+ #define UINT64CONST(x) (x##ULL)
  #else
  /* neither HAVE_LONG_INT_64 nor HAVE_LONG_LONG_INT_64 */
  #error must have a working 64-bit integer datatype
  #endif
  
  /* snprintf format strings to use for 64-bit integers */
  #define INT64_FORMAT "%" INT64_MODIFIER "d"
  #define UINT64_FORMAT "%" INT64_MODIFIER "u"
*** typedef unsigned PG_INT128_TYPE uint128;
*** 338,351 
  #define PG_UINT16_MAX	(0x)
  #define PG_INT32_MIN	(-0x7FFF-1)
  #define PG_INT32_MAX	(0x7FFF)
! #define PG_UINT32_MAX	(0x)
  #define PG_INT64_MIN	(-INT64CONST(0x7FFF) - 1)
  #define PG_INT64_MAX	INT64CONST(0x7FFF)
  #define PG_UINT64_MAX	UINT64CONST(0x)
  
  /* Max value of size_t might also be missing if we don't have stdint.h */
  #ifndef SIZE_MAX
! #define SIZE_MAX ((size_t) -1)
  #endif
  
  /*
--- 333,350 
  #define PG_UINT16_MAX	(0x)
  #define PG_INT32_MIN	(-0x7FFF-1)
  #define PG_INT32_MAX	(0x7FFF)
! #define PG_UINT32_MAX	(0xU)
  #define PG_INT64_MIN	(-INT64CONST(0x7FFF) - 1)
  #define PG_INT64_MAX	INT64CONST(0x7FFF)
  #define PG_UINT64_MAX	UINT64CONST(0x)
  
  /* Max value of size_t might also be missing if we don't have stdint.h */
  #ifndef SIZE_MAX
! #if SIZEOF_SIZE_T == 8
! #define SIZE_MAX PG_UINT64_MAX
! #else
! #define SIZE_MAX PG_UINT32_MAX
! #endif
  #endif
  
  /*

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


Re: [HACKERS] GnuTLS support

2017-09-01 Thread Daniel Gustafsson
> On 01 Sep 2017, at 20:00, Robert Haas  wrote:
> 
> On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
 I have seen discussions from time to time about OpenSSL and its licensing
 issues so I decided to see how much work it would be to add support for
 another TLS library, and  I went with GnuTLS since it is the library I know
 best after OpenSSL and it is also a reasonably popular library.
>> 
>>> Thanks for working on this.  I think it's good for PostgreSQL to have
>>> more options in this area.
>> 
>> +1.  We also have a patch in the queue to support macOS' TLS library,
>> and I suppose that's going to be facing similar issues.  It would be
>> a good plan, probably, to try to push both of these to conclusion in
>> the same development cycle.
> 
> The thing which I think would save the most aggravation - at least for
> my employer - is a Windows SSL implementation.

In 53ea546e.6020...@vmware.com, an early version of SChannel support was posted
by Heikki.  If anyone is keen to pick up the effort that would most likely be a
good starting point.

> Relying on OpenSSL
> means that every time OpenSSL puts out a critical security fix, we've
> got to rewrap all the Windows installers to pick up the new version.
> If we were relying on what's built into Windows, it would be
> Microsoft's problem.  Granted, it's not anybody's job to solve
> EnterpriseDB's problems except EnterpriseDB, but users might like it
> too -- and anyone else who is building Windows installers for
> PostgreSQL.
> 
> Depending on macOS TLS instead of OpenSSL has similar advantages, of
> course, just for a somewhat less common platform.

I think providing alternatives to OpenSSL on platforms where OpenSSL can’t be
relied on to be already available (Windows and macOS come to mind) would be a
great thing for many users and app developers.

cheers ./daniel

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


Re: [HACKERS] Rename RECOVERYXLOG to RECOVERYWAL?

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 12:57 PM, David Steele  wrote:
> I searched the various threads on the xlog -> wal rename and I couldn't
> find any specific mention of why this was not renamed.
>
> I have attached a patch in case it was an oversight rather than left
> as-is on purpose.

I don't think this really buys us anything.  If we'd applied it to v10
maybe, but what do we get out of whacking it around now?

"Consistency", I hear you cry!  Fair point.  But we never had a goal
of eliminating all internal references to "xlog", just the user-facing
ones.  And since RECOVERYXLOG is not documented, I think there's a
good argument that it's not user-facing.  You could argue that since
it shows up in the file system it's implicitly user-facing, and maybe
you're right; if some other committer really wants to make this
change, I won't grouse much.  But personally I'd favor leaving it
alone to avoid having the behavior change a little bit in every new
release.

-- 
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] OpenFile() Permissions Refactor

2017-09-01 Thread David Steele
On 9/1/17 1:15 PM, Peter Eisentraut wrote:
> On 8/29/17 12:15, David Steele wrote:
> 
> I wonder whether we even need that much flexibility.  We already set a
> global umask, so we could just open all files with 0666 and let the
> umask sort it out.  Then we don't need all the *Perm() variants.

Well, there's one instance where the *Perm is used:

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
-   fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | 
PG_BINARY,
-  S_IRUSR | S_IWUSR | S_IRGRP 
| S_IROTH);
+   fd = OpenTransientFilePerm(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC |
PG_BINARY,
+  S_IRUSR | S_IWUSR | 
S_IRGRP | S_IROTH);

I also think it's worth leaving the variants for extensions to use.
Even though there are no calls in the core extensions it's hard to say
what might be out there in the field.

> I don't like the function-like macros in fd.h.  We can use proper functions.

I had them as functions originally but thought macros might be
friendlier with compilers that don't inline.  I'm happy to change them back.

> I also wonder whether the umask save-and-restore code in copy.c and
> be-fsstubs.c is sound.  If the AllocateFile() call in between errors
> out, then there is nothing that restores the original umask.  This might
> need a TRY/CATCH block, or maybe just a chmod() afterwards.

Unless I'm mistaken this is a preexisting issue.  Would you prefer I
submit a different patch for that or combine it into this patch?

The chmod() implementation seems the safer option to me and produces
fewer code paths.  It also prevents partially-written files from being
accessible to any user but postgres.

> The mkdir() calls could perhaps use some further refactoring so you
> don't have to specify the mode everywhere.

I thought about that but feared it would be considered an overreach.
Does fd.c seem like a good place for the new function?

> This kind of code:
> 
> -   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
> +   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
> ereport(FATAL,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>  errmsg("data directory \"%s\" has group or world access",
> DataDir),
> -errdetail("Permissions should be u=rwx (0700).")));
> +errdetail("Permissions should be (%04o).",
> PG_DIR_MODE_DEFAULT)));
> 
> can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
> PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
> consistent.

Well, the eventual goal is to make the mask/mode configurable - at least
to the extent that group access is allowed.  However, I'm happy to leave
that discussion for another day.

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


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


Re: [HACKERS] GnuTLS support

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
>>> I have seen discussions from time to time about OpenSSL and its licensing
>>> issues so I decided to see how much work it would be to add support for
>>> another TLS library, and  I went with GnuTLS since it is the library I know
>>> best after OpenSSL and it is also a reasonably popular library.
>
>> Thanks for working on this.  I think it's good for PostgreSQL to have
>> more options in this area.
>
> +1.  We also have a patch in the queue to support macOS' TLS library,
> and I suppose that's going to be facing similar issues.  It would be
> a good plan, probably, to try to push both of these to conclusion in
> the same development cycle.

The thing which I think would save the most aggravation - at least for
my employer - is a Windows SSL implementation.  Relying on OpenSSL
means that every time OpenSSL puts out a critical security fix, we've
got to rewrap all the Windows installers to pick up the new version.
If we were relying on what's built into Windows, it would be
Microsoft's problem.  Granted, it's not anybody's job to solve
EnterpriseDB's problems except EnterpriseDB, but users might like it
too -- and anyone else who is building Windows installers for
PostgreSQL.

Depending on macOS TLS instead of OpenSSL has similar advantages, of
course, just for a somewhat less common platform.

-- 
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] path toward faster partition pruning

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 2:02 AM, Amit Langote
 wrote:
> Attached is now also the set of patches that implement the actual
> partition-pruning logic, viz. the last 3 patches (0004, 0005, and 0006) of
> the attached.

It strikes me that this patch set is doing two things but maybe in the
opposite order that I would have chosen to attack them.  First,
there's getting partition pruning to use something other than
constraint exclusion.  Second, there's deferring work that is
currently done at an early stage of the process until later, so that
we waste less effort on partitions that are ultimately going to be
pruned.

The second one is certainly a worthwhile goal, but there are fairly
firm interdependencies between the first one and some other things
that are in progress.  For example, the first one probably ought to be
done before hash partitioning gets committed, because
constraint-exclusion based partitioning pruning won't work with
partitioning pruning, but some mechanism based on asking the
partitioning code which partitions might match will.  Such a mechanism
is more efficient for list and range partitions, but it's the only
thing that will work for hash partitions.  Also, Beena Emerson is
working on run-time partition pruning, and the more I think about it,
the more I think that overlaps with this first part.  Both patches
need a mechanism to identify, given a btree-indexable comparison
operator (< > <= >= =) and a set of values, which partitions might
contain matching values.  Run-time partition pruning will call that at
execution time, and this patch will call it at plan time, but it's the
same logic; it's just a question of the point at which the values are
known.  And of course we don't want to end up with two copies of the
logic.

Therefore, IMHO, it would be best to focus first on how we're going to
identify the partitions that survive pruning, and then afterwards work
on transposing that logic to happen before partitions are opened and
locked.  That way, we get some incremental benefit sooner, and also
unblock some other development 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] CurrentUserId may be invalid during the rest of a session

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 6:02 AM, Richard Guo  wrote:
> Is this expected behavior?

I don't think so.  My impression after brief research is that this is
a bug introduced here:

Author: Tom Lane 
Branch: master Release: REL8_3_0 [eedb068c0] 2008-01-03 21:23:15 +
Branch: REL8_2_STABLE Release: REL8_2_6 [3af35f8d4] 2008-01-03 21:23:45 +
Branch: REL8_1_STABLE Release: REL8_1_11 [46cf9c260] 2008-01-03 21:24:26 +
Branch: REL8_0_STABLE Release: REL8_0_15 [108b19d86] 2008-01-03 21:25:00 +
Branch: REL7_4_STABLE Release: REL7_4_19 [230d5cfc4] 2008-01-03 21:25:34 +
Branch: REL7_3_STABLE Release: REL7_3_21 [218cf59b6] 2008-01-03 21:25:58 +

Make standard maintenance operations (including VACUUM, ANALYZE, REINDEX,
and CLUSTER) execute as the table owner rather than the calling user, using
the same privilege-switching mechanism already used for SECURITY DEFINER
functions.  The purpose of this change is to ensure that user-defined
functions used in index definitions cannot acquire the privileges of a
superuser account that is performing routine maintenance.  While a function
used in an index is supposed to be IMMUTABLE and thus not able to
do anything
very interesting, there are several easy ways around that restriction; and
even if we could plug them all, there would remain a risk of
reading sensitive
information and broadcasting it through a covert channel such as CPU usage.

To prevent bypassing this security measure, execution of SET SESSION
AUTHORIZATION and SET ROLE is now forbidden within a SECURITY
DEFINER context.

Thanks to Itagaki Takahiro for reporting this vulnerability.

Security: CVE-2007-6600

-- 
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] OpenFile() Permissions Refactor

2017-09-01 Thread Tom Lane
Peter Eisentraut  writes:
> I wonder whether we even need that much flexibility.  We already set a
> global umask, so we could just open all files with 0666 and let the
> umask sort it out.  Then we don't need all the *Perm() variants.

-1.  What that would mean is that if somebody had a need for a nonstandard
file mask, the path of least resistance would be to bypass fd.c and open
the file directly.  We do *not* want to encourage that.

I think David's design with macros inserting the default permission
choices looks fine (but I haven't read the patch completely).

> I also wonder whether the umask save-and-restore code in copy.c and
> be-fsstubs.c is sound.  If the AllocateFile() call in between errors
> out, then there is nothing that restores the original umask.  This might
> need a TRY/CATCH block, or maybe just a chmod() afterwards.

Yeah, this seems like a problem.

> This kind of code:

> -   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
> +   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
> ereport(FATAL,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>  errmsg("data directory \"%s\" has group or world access",
> DataDir),
> -errdetail("Permissions should be u=rwx (0700).")));
> +errdetail("Permissions should be (%04o).",
> PG_DIR_MODE_DEFAULT)));

I think this hunk should be left entirely alone.  The goal of this
patch should not be "eliminate every reference to S_IRWXO", anyway.
Trying to replace this one seems like it can have no good effect:
it would mean that if anyone ever changes PG_MODE_MASK_DEFAULT,
they've silently introduced either a security hole or a overly
restrictive check.

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] Patch: add --if-exists to pg_recvlogical

2017-09-01 Thread Peter Eisentraut
On 8/26/17 15:40, Rosser Schwarz wrote:
> On Fri, Aug 25, 2017 at 12:34 PM, Robert Haas  > wrote:
> 
> On Sun, May 21, 2017 at 3:04 PM, Rosser Schwarz
> > wrote:
> > While doing some scripting around pg_recvlogical at $work, I found a 
> need
> > for $subject. Attached, find a patch to that effect... 
> 
> Please add this to commitfest.postgresql.org
> 
> 
> 
> Done, thanks!
> 
> https://commitfest.postgresql.org/14/1256/ 

  
+  --if-exists
+  
+   
+Do not error out when --drop-slot or
--start are
+specified and a slot with the specified name does not exist.
+   
+  
+ 

I understand the --drop-slot part.  But I don't understand what it means
to ignore a missing replication slot when running --start.

The same option should be added to pg_receivewal as well.

-- 
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] GnuTLS support

2017-09-01 Thread Daniel Gustafsson

> On 01 Sep 2017, at 19:10, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
> 
>>> There are currently two failing SSL tests which at least to me seems more
>>> like they test specific OpenSSL behaviors rather than something which need
>>> to be true for all SSL libraries.
> 
>> I don't know what we should do about these issues.
> 
> Maybe the SSL test suite needs to be implementation-specific as well.

To properly test the macOS Secure Transport support we will need to use
Keychain files on top of plain PEM files, so I think we have to.  That being
said, we should probably define a (as large possible) minimum set which applies
to all to ensure compatability between different frontends and backends.

cheers ./daniel

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


Re: [HACKERS] OpenFile() Permissions Refactor

2017-09-01 Thread Peter Eisentraut
On 8/29/17 12:15, David Steele wrote:
> While working on the patch to allow group reads of $PGDATA I refactored
> the various OpenFile() functions to use default/global permissions
> rather than permissions defined in each call.
> 
> I think the patch stands on its own regardless of whether we accept the
> patch to allow group permissions (which won't make this CF).  We had a
> couple different ways of defining permissions (e.g. 0600 vs S_IRUSR |
> S_IWUSR) and in any case it represented quite a bit of duplication.
> This way seems simpler and less error-prone.

Yeah, it would be good to clean some of that up.

I wonder whether we even need that much flexibility.  We already set a
global umask, so we could just open all files with 0666 and let the
umask sort it out.  Then we don't need all the *Perm() variants.

I don't like the function-like macros in fd.h.  We can use proper functions.

I also wonder whether the umask save-and-restore code in copy.c and
be-fsstubs.c is sound.  If the AllocateFile() call in between errors
out, then there is nothing that restores the original umask.  This might
need a TRY/CATCH block, or maybe just a chmod() afterwards.

The mkdir() calls could perhaps use some further refactoring so you
don't have to specify the mode everywhere.

This kind of code:

-   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
ereport(FATAL,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 errmsg("data directory \"%s\" has group or world access",
DataDir),
-errdetail("Permissions should be u=rwx (0700).")));
+errdetail("Permissions should be (%04o).",
PG_DIR_MODE_DEFAULT)));

can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
consistent.

-- 
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] Parallel Hash take II

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 8:53 AM, Thomas Munro
 wrote:
> Solution 2:  Teach tuple queues to spill to disk instead of blocking
> when full.  I think this behaviour should probably only be activated
> while the leader is running the plan rather than draining tuple
> queues; the current block-when-full behaviour would still be
> appropriate if the leader is simply unable to drain queues fast
> enough.  Then the deadlock risk would go away.
>
> When I wrote it, I figured that leader_gate.c was cheap and would do
> for now, but I have to admit that it's quite confusing and it sucks
> that later batches lose a core.  I'm now thinking that 2 may be a
> better idea.  My first thought is that Gather needs a way to advertise
> that it's busy while running the plan, shm_mq needs a slightly
> different all-or-nothing nowait mode, and TupleQueue needs to write to
> a shared tuplestore or other temp file-backed mechanism when
> appropriate.  Thoughts?

The problem with solution 2 is that it might lead to either (a)
unbounded amounts of stuff getting spooled to files or (b) small spool
files being repeatedly created and deleted depending on how the leader
is spending its time.  If you could spill only when the leader is
actually waiting for the worker, that might be OK.

> Check out ExecReScanGather(): it shuts down and waits for all workers
> to complete, which makes the assumptions in ExecReScanHashJoin() true.
> If a node below Gather but above Hash Join could initiate a rescan
> then the assumptions would not hold.  I am not sure what it would mean
> though and we don't generate any such plans today to my knowledge.  It
> doesn't seem to make sense for the inner side of Nested Loop to be
> partial.  Have I missed something here?

I bet this could happen, although recent commits have demonstrated
that my knowledge of how PostgreSQL handles rescans is less than
compendious.  Suppose there's a Nested Loop below the Gather and above
the Hash Join, implementing a join condition that can't give rise to a
parameterized path, like a.x + b.x = 0.

-- 
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] GnuTLS support

2017-09-01 Thread Tom Lane
Robert Haas  writes:
> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
>> I have seen discussions from time to time about OpenSSL and its licensing
>> issues so I decided to see how much work it would be to add support for
>> another TLS library, and  I went with GnuTLS since it is the library I know
>> best after OpenSSL and it is also a reasonably popular library.

> Thanks for working on this.  I think it's good for PostgreSQL to have
> more options in this area.

+1.  We also have a patch in the queue to support macOS' TLS library,
and I suppose that's going to be facing similar issues.  It would be
a good plan, probably, to try to push both of these to conclusion in
the same development cycle.

> I think that what this shows is that the current set of GUCs is overly
> OpenSSL-centric.  We created a set of GUCs that are actually specific
> to one particular implementation but named them as if they were
> generic.  My idea about this would be to actually rename the existing
> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> as needed for other SSL implementations.

Works for me.

>> There are currently two failing SSL tests which at least to me seems more
>> like they test specific OpenSSL behaviors rather than something which need
>> to be true for all SSL libraries.

> I don't know what we should do about these issues.

Maybe the SSL test suite needs to be implementation-specific as well.

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] signed logging format for pid in log_line_prefix?

2017-09-01 Thread Tom Lane
Alvaro Herrera  writes:
> I wonder if we could just adopt pid_t for PIDs.

We could (if somebody is willing to find and change all the relevant
declarations).  But that doesn't do anything at all to clarify which
printf format code to use for them.

Note that the POSIX snippet you quote doesn't actually guarantee that
pid_t is not wider than "long".  So while we could convert all these
places to
printf("...%ld...", (long) pid_t_variable_here);
that's still not formally correct.

Since we have yet to see a platform where our current habit of casting
pids to int doesn't work just as well, I'm inclined not to bother
changing anything here.

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] [PATCH] Document the order of changing certain settings when using hot-standby servers

2017-09-01 Thread Robert Haas
On Thu, Apr 13, 2017 at 8:29 AM, Yorick Peterse  wrote:
> Good point, right now it can give you the idea that applying it to just
> 1 standby (instead of all of them) is good enough, when instead you
> need to apply it to all of them.
>
> Attached is an adjusted version of my changes to better reflect this.

To me, this just seems redundant.  The existing documentation already says:

 For these parameters,
 the value on the standby must
 be equal to or greater than the value on the primary. If these parameters
 are not set high enough then the standby will refuse to start.

Now you're proposing to add:

If you want to increase these values you
should do so on all standby servers first, before applying the changes to
the primary. If you instead want to decrease these values you should do so
on the primary first, before applying the changes to all standby servers.

But that's just the obvious logical consequence of the existing statement.

If we're going to add this text, I'd move it one sentence earlier and
stick "Therefore, " at the beginning.  But it strikes me that it's
just a bet that if we say things twice instead of once, people will
pay more attention -- which is maybe true, but if that's done on a
widespread basis, it will cause the documentation to become bloated.

-- 
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] Rename RECOVERYXLOG to RECOVERYWAL?

2017-09-01 Thread David Steele
I searched the various threads on the xlog -> wal rename and I couldn't
find any specific mention of why this was not renamed.

I have attached a patch in case it was an oversight rather than left
as-is on purpose.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index df4843f409..fd7bfa11d1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3562,7 +3562,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
set_ps_display(activitymsg, false);
 
restoredFromArchive = RestoreArchivedFile(path, 
xlogfname,
-   
  "RECOVERYXLOG",
+   
  "RECOVERYWAL",

  XLogSegSize,

  InRedo);
if (!restoredFromArchive)
@@ -5550,10 +5550,10 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr 
endOfLog)
XLogArchiveCleanup(xlogfname);
 
/*
-* Since there might be a partial WAL segment named RECOVERYXLOG, get 
rid
+* Since there might be a partial WAL segment named RECOVERYWAL, get rid
 * of it.
 */
-   snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYXLOG");
+   snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYWAL");
unlink(recoveryPath);   /* ignore any error */
 
/* Get rid of any remaining recovered timeline-history file, too */

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


Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-01 Thread Robert Haas
On Mon, Aug 21, 2017 at 5:30 PM, Simon Riggs  wrote:
> Thinking ahead, are we going to add a new --no-objecttype switch every
> time someone wants it?

I'd personally be fine with --no-whatever for any whatever that might
be a subsidiary property of database objects.  We've got
--no-security-labels, --no-tablespaces, --no-owner, and
--no-privileges already, so what's wrong with --no-comments?

(We've also got --no-publications; I think it's arguable whether that
is the same kind of thing.)

-- 
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] Missing SIZE_MAX

2017-09-01 Thread Tom Lane
Robert Haas  writes:
> On Fri, Sep 1, 2017 at 12:22 PM, Tom Lane  wrote:
>> [ warning: more than you really wanted to know ahead ]

> It might be worth the effort to clean all of this up, just because the
> next person who gets bitten by it may not be as smart as you are.

Yeah.  I was just thinking that maybe the appropriate investment of
effort is to make [U]INT64CONST smarter, so that it results in a
properly-suffixed constant and doesn't need a cast.  Then it'd be a
lot easier to make these other macros be #if-safe.

Or we could just recast the test in fe-exec.c to not use SIZE_MAX.
Checking whether "SIZEOF_SIZE_T == 4" would really have the same
effect, though it's uglier.

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] log_destination=file

2017-09-01 Thread Magnus Hagander
On Fri, Sep 1, 2017 at 6:44 PM, Tom Lane  wrote:

> Magnus Hagander  writes:
> > On Fri, Sep 1, 2017 at 6:00 PM, Greg Stark  wrote:
> >> So what happens now with these messages? My understanding is that
> >> they're missing from the CSV logs and are simply inserted into the
> >> text logs without any log_line_prefix? The logging collector doesn't
> >> recognize these messages and reformat them for the CSV logs does it?
>
> > Yeah, that's pretty much it.
>
> > Fixing that is also on my plan, but for later :)
>
> Keep in mind that you've got to be really, really conservative about
> adding functionality in the logging collector process.  If it ever
> crashes, we have problems.  If memory serves, the postmaster is able
> to restart it, but we cannot restore the original stdout/stderr
> destination, which is problematic if that's where its output had
> been going.  So it's pretty close to being as mission-critical as
> the postmaster itself.
>

Yeah. That's one of the reasons I'm not trying to do it all in one batch.

But yeah, the postmaster restarts it just fine. And with the WIP patch I
posted earlier, the message from the postmaster that it did gets lost if
you are logging to stderr. It does end up in the CSV file though. And I'm
sure there are plenty of other corner cases around that one to consider.

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


Re: [HACKERS] Upcoming commit fest will begin soon

2017-09-01 Thread Andres Freund
On 2017-09-01 16:14:25 +0200, Daniel Gustafsson wrote:
> > On 01 Sep 2017, at 05:33, Michael Paquier  wrote:
> > 
> > Another question is: who would like to become the CF manager for this
> > time? This commit fest is very large, so it is going to be difficult
> > for one person to do the whole thing, hence at least 2 people would be
> > welcome. Please note that I am not directly volunteering for this one,
> > but per my number of patches I need to look at many things, so I may
> > be considered as a sort of co-manager ;)
> > New heads for this exercise would be nice though.
> 
> I volunteer to be co-managing this CF.  Since I’ve never done it before, and
> the CF in question is rather large I wouldn’t mind it co-managing it with
> someone as you mention.

Where do I send the booze / xanax / ...?  Thanks for doing this!

Andres


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


Re: [HACKERS] log_destination=file

2017-09-01 Thread Tom Lane
Magnus Hagander  writes:
> On Fri, Sep 1, 2017 at 6:00 PM, Greg Stark  wrote:
>> So what happens now with these messages? My understanding is that
>> they're missing from the CSV logs and are simply inserted into the
>> text logs without any log_line_prefix? The logging collector doesn't
>> recognize these messages and reformat them for the CSV logs does it?

> Yeah, that's pretty much it.

> Fixing that is also on my plan, but for later :)

Keep in mind that you've got to be really, really conservative about
adding functionality in the logging collector process.  If it ever
crashes, we have problems.  If memory serves, the postmaster is able
to restart it, but we cannot restore the original stdout/stderr
destination, which is problematic if that's where its output had
been going.  So it's pretty close to being as mission-critical as
the postmaster itself.

regards, tom lane


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


Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

2017-09-01 Thread Tom Lane
Simon Riggs  writes:
> On 1 September 2017 at 15:19, Tom Lane  wrote:
>> This patch makes me itch.  Why is it correct for these three checks,
>> and only these three checks out of the couple dozen uses of isTopLevel
>> in standard_ProcessUtility, to instead do something else?

> No problem, it was a quick fix, not a deep one.

My thought is that what we need to do is find a way for isTopLevel
to be false if we're processing a multi-command string.  It looks
like exec_simple_query is already doing the right thing in terms
of what it tells PortalRun; why is that not propagating down to
ProcessUtility?

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] Missing SIZE_MAX

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 12:22 PM, Tom Lane  wrote:
> [ warning: more than you really wanted to know ahead ]

It might be worth the effort to clean all of this up, just because the
next person who gets bitten by it may not be as smart as you are.

-- 
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] Assert that the correct locks are held when calling PageGetLSN()

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 12:24 PM, Jacob Champion  wrote:
> On Fri, Sep 1, 2017 at 9:12 AM, Robert Haas  wrote:
>> It's a good idea to add patches to commitfest.postgresql.org
>
> Hi Robert, I added it yesterday as
> https://commitfest.postgresql.org/14/1279/ . Let me know if I need to
> touch anything up there.

Nah, looks fine, I just somehow missed it when I looked the first time.

-- 
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] Adding -E switch to pg_dumpall

2017-09-01 Thread Robert Haas
On Fri, Jul 21, 2017 at 9:21 AM, Michael Paquier
 wrote:
> On Thu, Jul 20, 2017 at 11:17 AM, Fabien COELHO  wrote:
>> Ok for me. I switched the status to "Ready for committers".
>
> Thanks for the review, Fabien.

LGTM.  Committed.

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


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


Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-01 Thread Jacob Champion
On Fri, Sep 1, 2017 at 9:12 AM, Robert Haas  wrote:
> It's a good idea to add patches to commitfest.postgresql.org

Hi Robert, I added it yesterday as
https://commitfest.postgresql.org/14/1279/ . Let me know if I need to
touch anything up there.

Thanks!
--Jacob


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


Re: [HACKERS] log_destination=file

2017-09-01 Thread Magnus Hagander
On Fri, Sep 1, 2017 at 6:00 PM, Greg Stark  wrote:

> On 31 August 2017 at 13:49, Tom Lane  wrote:
> > Magnus Hagander  writes:
> >> On Thu, Aug 31, 2017 at 2:34 PM, Tom Lane  wrote:
> > Yes, it's pretty important, because of assorted stuff not-under-our-
> > control that doesn't know about ereport and will just print to stderr
> > anyway.  Some examples: dynamic linker can't-resolve-symbol failure
> > messages, glibc malloc corruption error messages, just about any external
> > module in plperl or plpython.  I don't find this to be negotiable.
>
> So what happens now with these messages? My understanding is that
> they're missing from the CSV logs and are simply inserted into the
> text logs without any log_line_prefix? The logging collector doesn't
> recognize these messages and reformat them for the CSV logs does it?
>

Yeah, that's pretty much it.

Fixing that is also on my plan, but for later :)



> I'm actually asking because I'm more concerned with JSON logs or
> msgpack logs. Currently these are supported with an emit_log_hook but
> they can't capture these non-ereport logs either.
>
> Also the CSV and emit_log_hook based logs don't have any convenient
> way to turn them on and off and control the location and filename of
> the logs. It would be nice if we could have something like
>
> log_destinations='stderr=text,syslog=json,postgresql-%Y-%m-%
> d_%H%M%S.csv=csv'
>

That could definitely be an interesting improvement as well. Decoupling the
format from the destination would make a lot of sense. (Arguments can
certainly be made for example for csv-to-syslog, when you have a custom
syslog server)



> >> Are you actually asking for a benchmark of if logging gets slower?
> >
> > Yes.
>
> Personally I don't think it's "performance" so much as operational
> issues that are more concerning. For all we know there are people out
> there who tried to use the logging collector and found it didn't work
> well on some system -- perhaps it interacted with systemd or something
> else on the system -- and they switched back to just using stderr. I
> don't know how to flush these users out though if there are any. Just
> making this change early in a release cycle is the best we can do.
>

The most common use of stderr I think is debian/ubuntu which uses that and
then pg_ctl based redirect. Or at least used to (I remember being annoyed
by that at least once..)

AFAIK all other major platforms already use the logging collector in the
standard packages.

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


Re: [HACKERS] Missing SIZE_MAX

2017-09-01 Thread Tom Lane
[ warning: more than you really wanted to know ahead ]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> We have a workaround for that symbol in timezone/private.h:
>> #ifndef SIZE_MAX
>> #define SIZE_MAX ((size_t) -1)
>> #endif
>> and a bit of grepping finds other places that are using the (size_t) -1
>> trick explicitly.  So what I'm tempted to do is move the above stanza
>> into c.h.

> Sounds good to me.

On closer inspection, C99 requires SIZE_MAX and related macros to be a
"constant expression suitable for use in #if preprocessing directives",
which we need for the fe-exec.c usage because it does

#if INT_MAX >= (SIZE_MAX / 2)
if (newSize > SIZE_MAX / sizeof(PGresAttValue *))

(We could maybe dispense with this #if check, but I feared that doing
so would result in nanny-ish "expression is constant false" warnings
from some compilers on 64-bit platforms.)

Now, that cast doesn't really work in an #if expression.  Some language
lawyering leads me to conclude that in #if, a C compiler will interpret
the above value of SIZE_MAX as "((0) -1)", or just signed -1.  So
fe-exec.c's test will surely evaluate to true, which seems like a safe
outcome.  But you could certainly imagine other cases where you get
incorrect results if SIZE_MAX looks like a signed -1 to an #if-test.

When I look into /usr/include/stdint.h on my Linux box, I find

# if __WORDSIZE == 64
#  define SIZE_MAX  (18446744073709551615UL)
# else
#  define SIZE_MAX  (4294967295U)
# endif

so I thought about trying to duplicate that logic.  We can certainly test
SIZEOF_SIZE_T == 8 as a substitute for the #if condition.  The hard part
is arriving at a portable spelling of "UL", since it would need to be
"ULL" instead on some platforms.  We can't make use of our UINT64CONST
macro because that includes a cast.  So it seems like if we want to be
100% correct it would need to be something like

#ifndef SIZE_MAX
#if SIZEOF_SIZE_T == 8
#if SIZEOF_LONG == 8
#define SIZE_MAX (18446744073709551615UL)
#else /* assume unsigned long long is what we need */
#define SIZE_MAX (18446744073709551615ULL)
#endif
#else /* 32-bit */
#define SIZE_MAX (4294967295U)
#endif
#endif

That's mighty ugly.  Is it worth the trouble, rather than trusting
that the "(size_t) -1" trick will work?  Given that we have so few
needs for SIZE_MAX, I'm kind of inclined just to stick with the cast.

I notice BTW that PG_INT64_MIN, PG_INT64_MAX, and PG_UINT64_MAX
all contain casts and thus are equally risky to test in #if-tests.
I see no at-risk code in our tree right now, but someday we might
need to make those look something like the above, too.

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] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 2:53 PM, Jacob Champion  wrote:
> While working on checksum support for GPDB, we noticed that several
> callers of PageGetLSN() didn't follow the correct locking procedure.
> To try to help ferret out present and future mistakes, we added an
> assertion to PageGetLSN() that checks whether those locks were being
> held correctly. This patch is a first-draft attempt to port that
> assertion back up to postgres master, based on work by Asim Praveen,
> Ashwin Agrawal, and myself.
>
> The patch is really two pieces: add the assertion, and fix the callers
> that would trip it. The latter part is still in progress, because I'm
> running into some places where I'm not sure what the correct way
> forward is.
>
> (I'm a newbie to this list and this code base, so please don't
> hesitate to correct me on anything, whether that's code- or
> culture-related!)

It's a good idea to add patches to commitfest.postgresql.org

-- 
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] GnuTLS support

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
> I have seen discussions from time to time about OpenSSL and its licensing
> issues so I decided to see how much work it would be to add support for
> another TLS library, and  I went with GnuTLS since it is the library I know
> best after OpenSSL and it is also a reasonably popular library.

Thanks for working on this.  I think it's good for PostgreSQL to have
more options in this area.

> = Design questions
>
> == GnuTLS priority strings vs OpenSSL cipher lists
>
> GnuTLS uses a different format for specifying ciphers. Instead of setting
> ciphers, protocol versions, and ECDH curves separately GnuTLS instead uses a
> single priority string[1].
>
> For example the default settings of PostgreSQL (which include disabling
> SSLv3)
>
> ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
> ssl_prefer_server_ciphers = on
> ssl_ecdh_curve = 'prime256v1'
>
> is represented with a string similar to
>
> SECURE128:+3DES-CBC:+GROUP-SECP256R1:%SERVER_PRECEDENCE
>
> So the two libraries have decided on different ways to specify things.

I think that what this shows is that the current set of GUCs is overly
OpenSSL-centric.  We created a set of GUCs that are actually specific
to one particular implementation but named them as if they were
generic.  My idea about this would be to actually rename the existing
GUCs to start with "openssl" rather than "ssl", and then add new GUCs
as needed for other SSL implementations.

Depending on what we think is best, GUCs for an SSL implementation
other than the one against which we compiled can either not exist at
all, or can exist but be limited to a single value (e.g. "none", as we
currently do when the compile has no SSL support at all).  Also, we
could add a read-only GUC with a name like ssl_library that exposes
the name of the underlying SSL implementation - none, openssl, gnutls,
or whatever.

I think if we go the route of insisting that every SSL implementation
has to use the existing GUCs, we're just trying to shove a square peg
into a round hole, and there's no real benefit for users.  If the
string that has to be stuffed into ssl_ciphers differs based on which
library was chosen at compile time, then you can't have a uniform
default configuration for all libraries anyway.  I think it'll be
easier to explain and document this if there's separate documentation
for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
documentation section that tries to explain every implementation
separately.

> There are currently two failing SSL tests which at least to me seems more
> like they test specific OpenSSL behaviors rather than something which need
> to be true for all SSL libraries.
>
> The two tests:
>
> # Try with just the server CA's cert. This fails because the root file
> # must contain the whole chain up to the root CA.
> note "connect with server CA cert, without root CA";
> test_connect_fails("sslrootcert=ssl/server_ca.crt sslmode=verify-ca");
>
> # A CRL belonging to a different CA is not accepted, fails
> test_connect_fails(
> "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca
> sslcrl=ssl/client.crl");
>
> For the missing root CA case GnuTLS seems to be happy enough with just an
> intermediate CA and as far as I can tell this behavior is not easy to
> configure.
>
> And for the CRL belonging to a different CA case GnuTLS can be configured to
> either just store such a non-validating CRL or to ignore it, but not to
> return an error.
>
> Personally I think thee two tests should just be removed but maybe I am
> missing something.

I don't know what we should do about these issues.

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


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


Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

2017-09-01 Thread Simon Riggs
On 1 September 2017 at 15:19, Tom Lane  wrote:
> Simon Riggs  writes:
>> I've added tests to the recent patch to show it works.
>
> I don't think those test cases prove anything (ie, they work fine
> on an unpatched server).  With a backslash maybe they would.
>
>> Any objection to me backpatching this, please say.
>
> This patch makes me itch.  Why is it correct for these three checks,
> and only these three checks out of the couple dozen uses of isTopLevel
> in standard_ProcessUtility, to instead do something else?

No problem, it was a quick fix, not a deep one.

-- 
Simon Riggshttp://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] log_destination=file

2017-09-01 Thread Greg Stark
On 31 August 2017 at 13:49, Tom Lane  wrote:
> Magnus Hagander  writes:
>> On Thu, Aug 31, 2017 at 2:34 PM, Tom Lane  wrote:
> Yes, it's pretty important, because of assorted stuff not-under-our-
> control that doesn't know about ereport and will just print to stderr
> anyway.  Some examples: dynamic linker can't-resolve-symbol failure
> messages, glibc malloc corruption error messages, just about any external
> module in plperl or plpython.  I don't find this to be negotiable.

So what happens now with these messages? My understanding is that
they're missing from the CSV logs and are simply inserted into the
text logs without any log_line_prefix? The logging collector doesn't
recognize these messages and reformat them for the CSV logs does it?

I'm actually asking because I'm more concerned with JSON logs or
msgpack logs. Currently these are supported with an emit_log_hook but
they can't capture these non-ereport logs either.

Also the CSV and emit_log_hook based logs don't have any convenient
way to turn them on and off and control the location and filename of
the logs. It would be nice if we could have something like

log_destinations='stderr=text,syslog=json,postgresql-%Y-%m-%d_%H%M%S.csv=csv'

>> Are you actually asking for a benchmark of if logging gets slower?
>
> Yes.

Personally I don't think it's "performance" so much as operational
issues that are more concerning. For all we know there are people out
there who tried to use the logging collector and found it didn't work
well on some system -- perhaps it interacted with systemd or something
else on the system -- and they switched back to just using stderr. I
don't know how to flush these users out though if there are any. Just
making this change early in a release cycle is the best we can do.


-- 
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] Missing SIZE_MAX

2017-09-01 Thread Alvaro Herrera
Tom Lane wrote:

> We have a workaround for that symbol in timezone/private.h:
> 
> #ifndef SIZE_MAX
> #define SIZE_MAX ((size_t) -1)
> #endif
> 
> and a bit of grepping finds other places that are using the (size_t) -1
> trick explicitly.  So what I'm tempted to do is move the above stanza
> into c.h.

Sounds good to me.

-- 
Á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] signed logging format for pid in log_line_prefix?

2017-09-01 Thread Alvaro Herrera
Greg Stark wrote:
> Both the text and csv logging seem to use %d on for logging the server pid:
> 
> appendStringInfo(buf, "%d", MyProcPid);
> 
> Am I missing something or wouldn't this mean we print pids with large
> values as negative numbers? Isn't that strange? Wouldn't we rather use
> %u here?

MyProcPid is an int, so %d is the natural choice.  the sys_types.h
manpage says:

*  blksize_t, pid_t, and ssize_t shall be signed integer types.

and also:
   The implementation shall support one or more  programming  environments
   in  which  the  widths of blksize_t, pid_t, size_t, ssize_t, and susec‐
   onds_t are no greater than the width of type long.

I wonder if we could just adopt pid_t for PIDs.

-- 
Á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] Missing SIZE_MAX

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 11:09 AM, Tom Lane  wrote:
> Commit 2e70d6b5e added a dependency on SIZE_MAX to libpq/fe_exec.c.
> According to C99 and recent POSIX, that symbol should be provided
> by , but SUS v2 (POSIX 2001) doesn't require 
> to exist at all ... and I now notice that gaur/pademelon doesn't
> have it, and unsurprisingly is failing to compile fe_exec.c.
>
> We have a workaround for that symbol in timezone/private.h:
>
> #ifndef SIZE_MAX
> #define SIZE_MAX ((size_t) -1)
> #endif
>
> and a bit of grepping finds other places that are using the (size_t) -1
> trick explicitly.  So what I'm tempted to do is move the above stanza
> into c.h.  Any objections?

Not from me.

-- 
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] Speed up Clog Access by increasing CLOG buffers

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 10:03 AM, Dilip Kumar  wrote:
>> Sure will do so.  In the meantime, I have rebased the patch.
>
> I have repeated some of the tests we have performed earlier.

OK, these tests seem to show that this is still working.  Committed,
again.  Let's hope this attempt goes better than the last one.

-- 
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] signed logging format for pid in log_line_prefix?

2017-09-01 Thread Tom Lane
Greg Stark  writes:
> Both the text and csv logging seem to use %d on for logging the server pid:
> appendStringInfo(buf, "%d", MyProcPid);

> Am I missing something or wouldn't this mean we print pids with large
> values as negative numbers? Isn't that strange? Wouldn't we rather use
> %u here?

pid_t is a signed type; see for example waitpid(2):

   The value of pid can be:

   < -1   meaning  wait  for  any  child process whose process group ID is
  equal to the absolute value of pid.

   -1 meaning wait for any child process.

   0  meaning wait for any child process whose  process  group  ID  is
  equal to that of the calling process.

   > 0meaning  wait  for  the  child  whose process ID is equal to the
  value of pid.

So I think using %d is fine.  Someday we might wish that we were using
a wider type than int to hold PIDs, but I don't think it'll ever be
unsigned.

regards, tom lane


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


[HACKERS] Missing SIZE_MAX

2017-09-01 Thread Tom Lane
Commit 2e70d6b5e added a dependency on SIZE_MAX to libpq/fe_exec.c.
According to C99 and recent POSIX, that symbol should be provided
by , but SUS v2 (POSIX 2001) doesn't require 
to exist at all ... and I now notice that gaur/pademelon doesn't
have it, and unsurprisingly is failing to compile fe_exec.c.

We have a workaround for that symbol in timezone/private.h:

#ifndef SIZE_MAX
#define SIZE_MAX ((size_t) -1)
#endif

and a bit of grepping finds other places that are using the (size_t) -1
trick explicitly.  So what I'm tempted to do is move the above stanza
into c.h.  Any objections?

regards, tom lane


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


[HACKERS] signed logging format for pid in log_line_prefix?

2017-09-01 Thread Greg Stark
Both the text and csv logging seem to use %d on for logging the server pid:

appendStringInfo(buf, "%d", MyProcPid);

Am I missing something or wouldn't this mean we print pids with large
values as negative numbers? Isn't that strange? Wouldn't we rather use
%u here?


-- 
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] More replication race conditions

2017-09-01 Thread Alvaro Herrera
Michael Paquier wrote:
> On Mon, Aug 28, 2017 at 8:25 AM, Michael Paquier
>  wrote:
> > Today's run has finished with the same failure:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
> > Attached is a patch to make this code path wait that the transaction
> > has been replayed. We could use as well synchronous_commit = apply,
> > but I prefer the solution of this patch with a wait query.
> 
> I have added an open item for this issue for PG10. The 2PC tests in
> src/test/recovery are new in PG10, introduced by 3082098.

Thanks, pushed.

-- 
Á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] [PATCH] Fix drop replication slot blocking instead of returning error

2017-09-01 Thread Simone Gotti
On Fri, Sep 1, 2017 at 4:31 PM, Alvaro Herrera  wrote:
>
> Both patches pushed, which should close the open item.

Hi Alvaro,

thanks a lot!

Cheers,
Simone.


>
> --
> Á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] [PATCH] Fix drop replication slot blocking instead of returning error

2017-09-01 Thread Alvaro Herrera

Both patches pushed, which should close the open item.

-- 
Á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] pgbench: Skipping the creating primary keys after initialization

2017-09-01 Thread Fabien COELHO



I'm wondering whether this truncation should be yet another available
command? Hmmm... maybe not.


Currently TRUNCATE pgbench_accounts command is executed within a
transaction started immediately before it. If we move it out of the
transaction, the table data will be truncated even if the copying data
failed. Maybe we can do TRUNCATE pgbench_accounts, pgbench_history
instead. Thought?


Keep the truncate in the transaction, and truncate both (or all?) tables 
together.


--
Fabien.


--
Sent 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 fix] Savepoint-related statements terminates connection

2017-09-01 Thread Tom Lane
Simon Riggs  writes:
> I've added tests to the recent patch to show it works.

I don't think those test cases prove anything (ie, they work fine
on an unpatched server).  With a backslash maybe they would.

> Any objection to me backpatching this, please say.

This patch makes me itch.  Why is it correct for these three checks,
and only these three checks out of the couple dozen uses of isTopLevel
in standard_ProcessUtility, to instead do something else?

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] Upcoming commit fest will begin soon

2017-09-01 Thread Daniel Gustafsson
> On 01 Sep 2017, at 05:33, Michael Paquier  wrote:
> 
> Another question is: who would like to become the CF manager for this
> time? This commit fest is very large, so it is going to be difficult
> for one person to do the whole thing, hence at least 2 people would be
> welcome. Please note that I am not directly volunteering for this one,
> but per my number of patches I need to look at many things, so I may
> be considered as a sort of co-manager ;)
> New heads for this exercise would be nice though.

I volunteer to be co-managing this CF.  Since I’ve never done it before, and
the CF in question is rather large I wouldn’t mind it co-managing it with
someone as you mention.

cheers ./daniel

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


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

2017-09-01 Thread Dilip Kumar
On Wed, Aug 30, 2017 at 12:54 PM, Amit Kapila  wrote:
>
> That would have been better. In any case, will do the tests on some
> higher end machine and will share the results.
>
>> Given that we've changed the approach here somewhat, I think we need
>> to validate that we're still seeing a substantial reduction in
>> CLogControlLock contention on big machines.
>>
>
> Sure will do so.  In the meantime, I have rebased the patch.

I have repeated some of the tests we have performed earlier.

Machine:
Intel 8 socket machine with 128 core.

Configuration:

shared_buffers=8GB
checkpoint_timeout=40min
max_wal_size=20GB
max_connections=300
maintenance_work_mem=4GB
synchronous_commit=off
checkpoint_completion_target=0.9

I have run taken one reading for each test to measure the wait event.
Observation is same that at higher client count there is a significant
reduction in the contention on ClogControlLock.

Benchmark:  Pgbench simple_update, 30 mins run:

Head: (64 client) : (TPS 60720)
53808  Client  | ClientRead
  26147  IPC | ProcArrayGroupUpdate
   7866  LWLock  | CLogControlLock
   3705  Activity| LogicalLauncherMain
   3699  Activity| AutoVacuumMain
   3353  LWLock  | ProcArrayLoc
   3099  LWLock  | wal_insert
   2825  Activity| BgWriterMain
   2688  Lock| extend
   1436  Activity| WalWriterMain

Patch: (64 client) : (TPS 67207)
 53235  Client  | ClientRead
  29470  IPC | ProcArrayGroupUpdate
   4302  LWLock  | wal_insert
   3717  Activity| LogicalLauncherMain
   3715  Activity| AutoVacuumMain
   3463  LWLock  | ProcArrayLock
   3140  Lock| extend
   2934  Activity| BgWriterMain
   1434  Activity| WalWriterMain
   1198  Activity| CheckpointerMain
   1073  LWLock  | XidGenLock
869  IPC | ClogGroupUpdate

Head:(72 Client): (TPS 57856)

 55820  Client  | ClientRead
  34318  IPC | ProcArrayGroupUpdate
  15392  LWLock  | CLogControlLock
   3708  Activity| LogicalLauncherMain
   3705  Activity| AutoVacuumMain
   3436  LWLock  | ProcArrayLock

Patch:(72 Client): (TPS 65740)

  60356  Client  | ClientRead
  38545  IPC | ProcArrayGroupUpdate
   4573  LWLock  | wal_insert
   3708  Activity| LogicalLauncherMain
   3705  Activity| AutoVacuumMain
   3508  LWLock  | ProcArrayLock
   3492  Lock| extend
   2903  Activity| BgWriterMain
   1903  LWLock  | XidGenLock
   1383  Activity| WalWriterMain
   1212  Activity| CheckpointerMain
   1056  IPC | ClogGroupUpdate


Head:(96 Client): (TPS 52170)

  62841  LWLock  | CLogControlLock
  56150  IPC | ProcArrayGroupUpdate
  54761  Client  | ClientRead
   7037  LWLock  | wal_insert
   4077  Lock| extend
   3727  Activity| LogicalLauncherMain
   3727  Activity| AutoVacuumMain
   3027  LWLock  | ProcArrayLock

Patch:(96 Client): (TPS 67932)

  87378  IPC | ProcArrayGroupUpdate
  80201  Client  | ClientRead
  11511  LWLock  | wal_insert
   4102  Lock| extend
   3971  LWLock  | ProcArrayLock
   3731  Activity| LogicalLauncherMain
   3731  Activity| AutoVacuumMain
   2948  Activity| BgWriterMain
   1763  LWLock  | XidGenLock
   1736  IPC | ClogGroupUpdate

Head:(128 Client): (TPS 40820)

 182569  LWLock  | CLogControlLock
  61484  IPC | ProcArrayGroupUpdate
  37969  Client  | ClientRead
   5135  LWLock  | wal_insert
   3699  Activity| LogicalLauncherMain
   3699  Activity| AutoVacuumMain

Patch:(128 Client): (TPS 67054)

 174583  IPC | ProcArrayGroupUpdate
  66084  Client  | ClientRead
  16738  LWLock  | wal_insert
   4993  IPC | ClogGroupUpdate
   4893  LWLock  | ProcArrayLock
   4839  Lock| extend

Benchmark: select for update with 3 save points, 10 mins run

Script:
\set aid random (1,3000)
\set tid random (1,3000)

BEGIN;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
SAVEPOINT s1;
SELECT tbalance FROM pgbench_tellers WHERE tid = :tid for UPDATE;
SAVEPOINT s2;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
SAVEPOINT s3;
SELECT tbalance FROM pgbench_tellers WHERE tid = :tid for UPDATE;
END;

Head:(64 Client): (TPS 44577.1802)

  53808  Client  | ClientRead
  26147  IPC | ProcArrayGroupUpdate
   7866  LWLock  | CLogControlLock
   3705  Activity| LogicalLauncherMain
   3699  Activity| AutoVacuumMain
   3353  LWLock  | ProcArrayLock
   3099  LWLock  | wal_insert

Patch:(64 Client): (TPS 46156.245)

 53235  Client  | ClientRead
  

Re: [HACKERS] More replication race conditions

2017-09-01 Thread Simon Riggs
On 31 August 2017 at 12:54, Simon Riggs  wrote:

>> The above-described topic is currently a PostgreSQL 10 open item.  Simon,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> v10 open item, please let us know.  Otherwise, please observe the policy on
>> open item ownership[1] and send a status update within three calendar days of
>> this message.  Include a date for your subsequent status update.  Testers may
>> discover new open items at any time, and I want to plan to get them all fixed
>> well in advance of shipping v10.  Consequently, I will appreciate your 
>> efforts
>> toward speedy resolution.  Thanks.
>>
>> [1] 
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>
> I acknowledge receipt of the reminder and will fix by end of day tomorrow.

Applied. Thanks for the patch, Petr.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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


Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

2017-09-01 Thread Simon Riggs
On 1 September 2017 at 08:09, Michael Paquier  wrote:
> On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs  wrote:
>> I'm not sure I see the use case for anyone using SAVEPOINTs in this
>> context, so simply throwing a good error message is enough.
>>
>> Clearly nobody is using this, so lets just lock the door. I don't
>> think fiddling with the transaction block state machine is anything
>> anybody wants to do in back branches, at least without a better reason
>> than this.
>
> I don't think you can say that, per se the following recent report:
> https://www.postgresql.org/message-id/cah2-v61vxnentfj2v-zd+ma-g6kqmjgd5svxou3jbvdzqh0...@mail.gmail.com

AIUI, nobody is saying this should work, we're just discussing how to
produce an error message. We should fix it, but not spend loads of
time on it.

I've added tests to the recent patch to show it works.

Any objection to me backpatching this, please say.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


prevent_multistatement_savepoints.v2.patch
Description: Binary data

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


Re: [HACKERS] adding the commit to a patch's thread

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 4:26 AM, Alvaro Herrera  wrote:
> Erik Rijkers wrote:
>> Would it be possible to change the commitfest a bit and make it possible to
>> add the commit (or commit-message, or hash) to the thread in the
>> commitfest-app.
>
> +1 to add one or more commit hashes to CF entry metadata.
>
> (Back-filling for old entries welcome)

Couldn't the CF app scrape the commit messages for references to
threads, and if the commit message points to a thread with exactly 1
patch record, associate the commit to that patch?

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


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-01 Thread Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi Emre,

I'm afraid these patches conflict with current master branch.

The new status of this patch is: Waiting on Author

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


Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

2017-09-01 Thread Antonin Houska
Ashutosh Bapat  wrote:

> I originally thought to provide it along-with the changes to
> expand_inherited_rtentry(), but that thread is taking longer. Jeevan
> Chalke needs rebased patches for his work on aggregate pushdown and
> Thomas might need them for further review. So, here they are.

Since I have related patch in the current commitfest
(https://commitfest.postgresql.org/14/1247/), I spent some time reviewing your
patch:

* generate_partition_wise_join_paths()

Right parenthesis is missing in the prologue.


* get_partitioned_child_rels_for_join()

I think the Assert() statement is easier to understand inside the loop, see
the assert.diff attachment.


* have_partkey_equi_join()

As the function handles generic join, this comment doesn't seem to me
relevant:

/*
 * The equi-join between partition keys is strict if equi-join between
 * at least one partition key is using a strict operator. See
 * explanation about outer join reordering identity 3 in
 * optimizer/README
 */
strict_op = op_strict(opexpr->opno);

And I think the function can return true even if strict_op is false for all
the operators evaluated in the loop.


* match_expr_to_partition_keys()

I'm not sure this comment is clear enough:

/*
 * If it's a strict equi-join a NULL partition key on one side will
 * not join a NULL partition key on the other side. So, rows with NULL
 * partition key from a partition on one side can not join with those
 * from a non-matching partition on the other side. So, search the
 * nullable partition keys as well.
 */
if (!strict_op)
continue;

My understanding of the problem of NULL values generated by outer join is:
these NULL values --- if evaluated by non-strict expression --- can make row
of N-th partition on one side of the join match row(s) of *other than* N-th
partition(s) on the other side. Thus the nullable input expressions may only
be evaluated by strict operators. I think it'd be clearer if you stressed that
(undesired) *match* of partition keys can be a problem, rather than mismatch.

If you insist on your wording, then I think you should at least move the
comment below to the part that only deals with strict operators.


* There are several places where lfirst_node() macro should be used. For
  example

rel = lfirst_node(RelOptInfo, lc);

instead of

rel = (RelOptInfo *) lfirst(lc);


* map_and_merge_partitions()

Besides a few changes proposed in map_and_merge_partitions.diff (a few of them
to suppress compiler warnings) I think that this part needs more thought:

{
Assert(mergemap1[index1] != mergemap2[index2] &&
   mergemap1[index1] >= 0 && mergemap2[index2] >= 0);

/*
 * Both the partitions map to different merged partitions. This
 * means that multiple partitions from one relation matches to one
 * partition from the other relation. Partition-wise join does not
 * handle this case right now, since it requires ganging multiple
 * partitions together (into one RelOptInfo).
 */
merged_index = -1;
}

I could hit this path with the following test:

CREATE TABLE a(i int) PARTITION BY LIST(i);
CREATE TABLE a_0 PARTITION OF a FOR VALUES IN (0, 2);
CREATE TABLE b(j int) PARTITION BY LIST(j);
CREATE TABLE b_0 PARTITION OF b FOR VALUES IN (1, 2);

SET enable_partition_wise_join TO on;

SELECT  *
FROMa
FULL JOIN
b ON i = j;

I don't think there's a reason not to join a_0 partition to b_0, is there?

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index a75b1a3..3094b56
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*** get_partitioned_child_rels_for_join(Plan
*** 6160,6170 
  		PartitionedChildRelInfo *pc = lfirst(l);
  
  		if (bms_is_member(pc->parent_relid, join_relids))
  			result = list_concat(result, list_copy(pc->child_rels));
  	}
  
- 	/* The root partitioned table is included as a child rel */
- 	Assert(list_length(result) >= bms_num_members(join_relids));
- 
  	return result;
  }
--- 6160,6172 
  		PartitionedChildRelInfo *pc = lfirst(l);
  
  		if (bms_is_member(pc->parent_relid, join_relids))
+ 		{
+ 			/* The root partitioned table is included as a child rel */
+ 			Assert(list_length(pc->child_rels) >= 1);
+ 
  			result = list_concat(result, list_copy(pc->child_rels));
+ 		}
  	}
  
  	return result;
  }
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
new file mode 100644
index eb35fab..aa9c70d
*** a/src/backend/catalog/partition.c
--- b/src/backend/catalog/partition.c
*** partition_list_bounds_merge(int 

Re: [HACKERS] [PATCH] Off-by-one error in logical slot resource retention

2017-09-01 Thread Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi Craig,

I'm afraid patches 0002 and 0003 don't apply anymore. Could you please resolve 
the conflicts?

The new status of this patch is: Waiting on Author

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


Re: [HACKERS] Visual Studio 2017 Build Support

2017-09-01 Thread Haribabu Kommi
On Fri, Sep 1, 2017 at 11:06 AM, Tanay Varma 
wrote:

>
>
> Hello,
>
>
>
> This is with respect to the original thread on “visual studio 2017 build
> support” created by Haribabu Kommi (kommi.harib...@gmail.com).
>
>
> 
>
> https://www.postgresql.org/message-id/CAJrrPGcZpraBCe6fJ963kVzKdM7AW
> PTYmXJ=8neap87wed9...@mail.gmail.com
>
>
>
> Firstly, I would like to thank Haribabu Kommi for authoring the patch.
>
>
>
> I am posting a small update to the final patch submitted by Haribabu Kommi
> to also support the recent* v15.3 Release of Visual Stuido 2017 which
> upgrades the VC tools to version 14.11.*
>
>
>
> It would be great if this patch could be accepted so that Postgres could
> be built using the latest VS tools.
>
>
Hi,

Thanks for the review and change the patch to support the latest VS 2017.
In [1],  I already posted an updated patch that provides the support for
latest VS 2017,
may be you could have missed to check it. And also that patch includes the
fix for any
future version number updates from VS 2017.

[1] -
https://www.postgresql.org/message-id/CAJrrPGe%3D4jMkREOffnaDU93OerJwkVborPGE5O1Z1h1Jj-hUrg%40mail.gmail.com

Regards,
Hari Babu
Fujitsu Australia


Re: [HACKERS] WIP Patch: Precalculate stable functions, infrastructure v1

2017-09-01 Thread Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi Marina,

I'm sorry to inform you that the v5 path set become a little outdated:

```
$ git apply v5-0002-Precalculate-stable-functions-planning-and-execut.patch
error: patch failed: src/pl/plpgsql/src/pl_exec.c:6471
error: src/pl/plpgsql/src/pl_exec.c: patch does not apply
```

If it's not too much trouble could you please fix the conflicts with the 
current master branch?

The new status of this patch is: Waiting on Author

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


Re: [HACKERS] Upcoming commit fest will begin soon

2017-09-01 Thread Michael Paquier
On Fri, Sep 1, 2017 at 12:33 PM, Michael Paquier
 wrote:
> Another question is: who would like to become the CF manager for this
> time? This commit fest is very large, so it is going to be difficult
> for one person to do the whole thing, hence at least 2 people would be
> welcome. Please note that I am not directly volunteering for this one,
> but per my number of patches I need to look at many things, so I may
> be considered as a sort of co-manager ;)
> New heads for this exercise would be nice though.

The first commit fest is now officially "In Progress". Here is the
current score:
Needs review: 158.
Waiting on Author: 22.
Ready for Committer: 32.
-- 
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] Statement-level rollback

2017-09-01 Thread Simon Riggs
On 14 August 2017 at 23:58, Peter Eisentraut
 wrote:
> On 2/28/17 02:39, Tsunakawa, Takayuki wrote:
>> The code for stored functions is not written yet, but I'd like your feedback 
>> for the specification and design based on the current patch.  I'll add this 
>> patch to CommitFest 2017-3.
>
> This patch needs to be rebased for the upcoming commit fest.

I'm willing to review this if the patch is going to be actively worked on.

-- 
Simon Riggshttp://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] pg_serial early wraparound

2017-09-01 Thread Thomas Munro
On Wed, Jun 28, 2017 at 1:11 PM, Thomas Munro
 wrote:
> On Sat, Mar 25, 2017 at 7:27 AM, Thomas Munro
>  wrote:
>> On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
>>  wrote:
>>> You claim that SLRUs now support five digit segment name, while in slru.h
>>> at current master I see the following:
>>>
>>>  * Note: slru.c currently assumes that segment file names will be four hex
>>>  * digits.  This sets a lower bound on the segment size (64K transactions
>>>  * for 32-bit TransactionIds).
>>>  */
>
> I've now complained about that comment in a separate thread.
>
>> It's not urgent, it's just cleanup work, so I've now moved it to the
>> next commitfest.  I will try to figure out a new way to demonstrate
>> that it works correctly without having to ask a review[er] to disable
>> any assertions.  Thanks again.

Rebased again, now with a commit message.  That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.

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


ssi-slru-wraparound-v3.patch
Description: Binary data


ssi-slru-wraparound-test.sh
Description: Bourne shell script

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


Re: [HACKERS] Crash on promotion when recovery.conf is renamed

2017-09-01 Thread Thomas Munro
On Sat, Aug 26, 2017 at 7:32 AM, Robert Haas  wrote:
> On Sat, Apr 8, 2017 at 10:05 AM, David Steele  wrote:
>> On 3/28/17 1:21 AM, Tsunakawa, Takayuki wrote:
>>> Thanks, marked as ready for committer, as the code is good and Alexander 
>>> reported the test success.
>>
>> This bug has been moved to CF 2017-07.
>
> This bug fix has been pending in "Ready for Committer" state for about
> 4.5 months.  Three committers (Magnus, Heikki, Tom) have contributed
> to the thread to date.  Maybe one of them would like to commit this?

In the meantime its bits have begun to rot.  Michael, could you please rebase?

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] Minor code improvement to postgresGetForeignPlan

2017-09-01 Thread Etsuro Fujita
On 2017/04/07 13:12, Tatsuro Yamada wrote:> The declaration of  
postgresGetForeignPlan uses baserel, but

the actual definition uses foreignrel. It would be better to sync.


Agreed.


Please find attached a patch.


The patch looks good to me, so I'll mark this as Ready for Committer.

(I'm not sure we should do the same thing to the function declaration in  
other places such as fdwapi.h and the documentation for consistency, but  
if so, I'd vote for leaving that for another patch.)


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] Update comment in ExecPartitionCheck

2017-09-01 Thread Etsuro Fujita

On 2017/08/26 2:28, Robert Haas wrote:

On Tue, Jul 4, 2017 at 4:55 AM, Etsuro Fujita
 wrote:

This comment in an error handling in ExecPartitionCheck():

 if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
 {
 char   *val_desc;
 Relationorig_rel = rel;

 /* See the comment above. */
 if (resultRelInfo->ri_PartitionRoot)

should be updated because we don't have any comment on that above in the
code.  Since we have a comment on that in ExecConstraints() defined just
below that function, I think the comment should be something like this: "See
the comment in ExecConstraints().".  Attached is a patch for that.


Hrm.  I'm not sure I understand which comment in ExecConstraints()
this is supposed to refer to.  Maybe we need to think a bit harder
about how to make this clear.


The comment in ExecConstraints is this:

/*
 * If the tuple has been routed, it's been converted to the
 * partition's rowtype, which might differ from the root
 * table's.  We must convert it back to the root table's
 * rowtype so that val_desc shown error message matches the
 * input tuple.
 */
if (resultRelInfo->ri_PartitionRoot)

How about replacing the comment "See the comment above." in 
ExecPartitionCheck with something like this: "If the tuple has been 
routed, convert it from the partition's rowtype to the root table's. See 
the comment in ExecConstraints().".  I think that would make it easy to 
specify that comment in ExecConstrains.  I'd like to propose to update 
the same comments in other places as well, just for consistency.


PFA an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 1884,1890  ExecPartitionCheck(ResultRelInfo *resultRelInfo, 
TupleTableSlot *slot,
char   *val_desc;
Relationorig_rel = rel;
  
!   /* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = ExecFetchSlotTuple(slot);
--- 1884,1893 
char   *val_desc;
Relationorig_rel = rel;
  
!   /*
!* If the tuple has been routed, convert it from the partition's
!* rowtype to the root table's.  See the comment in 
ExecConstraints().
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = ExecFetchSlotTuple(slot);
***
*** 2011,2017  ExecConstraints(ResultRelInfo *resultRelInfo,
char   *val_desc;
Relationorig_rel = rel;
  
!   /* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
--- 2014,2023 
char   *val_desc;
Relationorig_rel = rel;
  
!   /*
!* If the tuple has been routed, convert it from the 
partition's
!* rowtype to the root table's.  See the comment above.
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
***
*** 2121,2127  ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
 * USING policy.
 */
case WCO_VIEW_CHECK:
!   /* See the comment in 
ExecConstraints(). */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
--- 2127,2137 
 * USING policy.
 */
case WCO_VIEW_CHECK:
!   /*
!* If the tuple has been routed, 
convert it from the
!* partition's rowtype to the root 
table's.  See the
!* comment in ExecConstraints().
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);

-- 
Sent via pgsql-hackers 

Re: [HACKERS] CLUSTER command progress monitor

2017-09-01 Thread Masahiko Sawada
On Fri, Sep 1, 2017 at 3:38 PM, Tatsuro Yamada
 wrote:
> Hi Thomas,
>
>>> Any comments or suggestion are welcome.
>>
>>
>> Although this patch updates src/test/regress/expected/rules.out I
>> think perhaps you included the wrong version?  That regression test
>> fails for me
>
>
> Thanks for the comment.
>
> I use the patch on 7b69b6ce and it's fine.
> Did you use "initdb" command after "make install"?
> The pg_stat_progress_cluster view is created in initdb, probably.
>

I also got a regression test error (applied to abe85ef). Here is
regression.diff file.

*** /home/masahiko/source/postgresql/src/test/regress/expected/rules.out
   2017-09-01 17:27:33.680055612 -0700
--- /home/masahiko/source/postgresql/src/test/regress/results/rules.out
2017-09-01 17:28:10.410055596 -0700
***
*** 1819,1824 
--- 1819,1849 
  pg_stat_get_db_conflict_bufferpin(d.oid) AS confl_bufferpin,
  pg_stat_get_db_conflict_startup_deadlock(d.oid) AS confl_deadlock
 FROM pg_database d;
+ pg_stat_progress_cluster| SELECT s.pid,
+ s.datid,
+ d.datname,
+ s.relid,
+ CASE s.param1
+ WHEN 0 THEN 'initializing'::text
+ WHEN 1 THEN 'scanning heap'::text
+ WHEN 2 THEN 'sorting tuples'::text
+ WHEN 3 THEN 'writing new heap'::text
+ WHEN 4 THEN 'scan heap and write new heap'::text
+ WHEN 5 THEN 'swapping relation files'::text
+ WHEN 6 THEN 'rebuilding index'::text
+ WHEN 7 THEN 'performing final cleanup'::text
+ ELSE NULL::text
+ END AS phase,
+ CASE s.param2
+ WHEN 0 THEN 'index scan'::text
+ WHEN 1 THEN 'seq scan'::text
+ ELSE NULL::text
+ END AS scan_method,
+ s.param3 AS scan_index_relid,
+ s.param4 AS heap_tuples_total,
+ s.param5 AS heap_tuples_scanned
+FROM (pg_stat_get_progress_info('CLUSTER'::text) s(pid, datid,
relid, param1, param2, param3, param4, param5, param6, param7, param8,
param9, param10)
+  LEFT JOIN pg_database d ON ((s.datid = d.oid)));
  pg_stat_progress_vacuum| SELECT s.pid,
  s.datid,
  d.datname,
***
*** 1841,1871 
  s.param7 AS num_dead_tuples
 FROM (pg_stat_get_progress_info('VACUUM'::text) s(pid, datid,
relid, param1, param2, param3, param4, param5, param6, param7, param8,
param9, param10)
   LEFT JOIN pg_database d ON ((s.datid = d.oid)));
- pg_stat_progress_cluster| SELECT
- s.pid,
- s.datid,
- d.datname,
- s.relid,
- CASE s.param1
- WHEN 0 THEN 'initializing'::text
- WHEN 1 THEN 'scanning heap'::text
- WHEN 2 THEN 'sorting tuples'::text
- WHEN 3 THEN 'writing new heap'::text
- WHEN 4 THEN 'scan heap and write new heap'::text
- WHEN 5 THEN 'swapping relation files'::text
- WHEN 6 THEN 'rebuilding index'::text
- WHEN 7 THEN 'performing final cleanup'::text
- ELSE NULL::text
- END AS phase,
- CASE S.param2
- WHEN 0 THEN 'index scan'
- WHEN 1 THEN 'seq scan'
- END AS scan_method,
- s.param3 AS index_relid,
- s.param4 AS heap_blks_total,
- s.param5 AS heap_blks_scanned
-FROM (pg_stat_get_progress_info('CLUSTER'::text) s(pid, datid,
relid, param1, param2, param3, param4, param5)
-  LEFT JOIN pg_database d ON ((s.datid = d.oid)));
  pg_stat_replication| SELECT s.pid,
  s.usesysid,
  u.rolname AS usename,
--- 1866,1871 

==

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


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


Re: [HACKERS] Surjective functional indexes

2017-09-01 Thread Konstantin Knizhnik


On 01.09.2017 09:25, Simon Riggs wrote:

On 1 September 2017 at 05:40, Thomas Munro
 wrote:

On Fri, Jun 9, 2017 at 8:08 PM, Konstantin Knizhnik
 wrote:

Attached please find rebased version of the patch.
Now "projection" attribute is used instead of surjective/injective.

Hi Konstantin,

This still applies but it doesn't compile after commits 2cd70845 and
c6293249.  You need to change this:

   Form_pg_attribute att = RelationGetDescr(indexDesc)->attrs[i];

... to this:

   Form_pg_attribute att = TupleDescAttr(RelationGetDescr(indexDesc), i);

Thanks!

Does the patch work fully with that change? If so, I will review.


Attached please find rebased version of the patch.
Yes, I checked that it works after this fix.
Thank you in advance for review.

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

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 83ee7d3..52189ac 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -294,8 +294,33 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] 
 The optional WITH clause specifies storage
 parameters for the index.  Each index method has its own set of allowed
-storage parameters.  The B-tree, hash, GiST and SP-GiST index methods all
-accept this parameter:
+storage parameters. All indexes accept the following parameter:
+   
+
+   
+   
+projection
+
+ 
+   Functional index is based on on projection function: function which extract subset of its argument.
+   In mathematic such functions are called non-injective. For injective function if any attribute used in the indexed
+   expression is changed, then value of index expression is also changed. So to check that index is affected by the
+   update, it is enough to check the set of changed fields. By default this parameters is assigned true value and function is considered
+   as non-injective.
+   In this case change of any of indexed key doesn't mean that value of the function is changed. For example, for
+   the expression expression(bookinfo-'isbn') defined
+   for column of JSON type is changed only when ISBN is changed, which rarely happen. The same is true for most
+   functional indexes. For non-injective functions, Postgres compares values of indexed expression for old and updated tuple and updates
+   index only when function results are different. It allows to eliminate index update and use HOT update.
+   But there are extra evaluations of the functions. So if function is expensive or probability that change of indexed column will not effect
+   the function value is small, then marking index as projection may increase update speed.
+
+
+   
+   
+
+   
+ The B-tree, hash, GiST and SP-GiST index methods all accept this parameter:

 

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index ec10762..b73165f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -130,6 +130,15 @@ static relopt_bool boolRelOpts[] =
 	},
 	{
 		{
+			"projection",
+			"Evaluate functional index expression on update to check if its values is changed",
+			RELOPT_KIND_INDEX,
+			AccessExclusiveLock
+		},
+		true
+	},
+	{
+		{
 			"security_barrier",
 			"View acts as a row security barrier",
 			RELOPT_KIND_VIEW,
@@ -1301,7 +1310,7 @@ fillRelOptions(void *rdopts, Size basesize,
 break;
 			}
 		}
-		if (validate && !found)
+		if (validate && !found && options[i].gen->kinds != RELOPT_KIND_INDEX)
 			elog(ERROR, "reloption \"%s\" not found in parse table",
  options[i].gen->name);
 	}
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e29c5ad..05e372f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -56,6 +56,7 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
+#include "catalog/index.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "port/atomics.h"
@@ -74,7 +75,9 @@
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
-
+#include "utils/memutils.h"
+#include "nodes/execnodes.h"
+#include "executor/executor.h"
 
 /* GUC variable */
 bool		synchronize_seqscans = true;
@@ -126,6 +129,7 @@ static bool ConditionalMultiXactIdWait(MultiXactId multi, MultiXactStatus status
 static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup);
 static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_modified,
 	   bool *copy);
+static bool ProjectionIsNotChanged(Relation relation, HeapTuple oldtup, HeapTuple newtup);
 
 
 /*
@@ -3547,8 +3551,6 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
 	key_attrs = 

Re: [HACKERS] pgjdbc logical replication client throwing exception

2017-09-01 Thread dipesh
Hi,

I am also facing the same issue. Have you solved the problem?
Could you please guide me how to deal with it.

Thanks,
Dipesh



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html


-- 
Sent 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 10 release notes

2017-09-01 Thread Masahiko Sawada
Hi all,

On Tue, Aug 1, 2017 at 5:53 AM, Thomas Munro
 wrote:
> On Tue, Apr 25, 2017 at 1:31 PM, Bruce Momjian  wrote:
>> I have committed the first draft of the Postgres 10 release notes.  They
>> are current as of two days ago, and I will keep them current.  Please
>> give me any feedback you have.
>
> Hi Bruce,
>
> "Add AFTER trigger transition tables to record changed rows (Kevin Grittner)"
>
> Any chance I could ask for a secondary author credit here?  While I
> started out as a reviewer and I understand that we don't list those, I
> finished up writing quite a lot of lines of committed code for this
> (see commits 1add0b15, 8c55244a, c46c0e52, 501ed02c, f32d57fd,
> 9e6104c6, 29fd3d9d, 304007d9, 5ebeb579) and was already listed as a
> co-author by Kevin in the original commits (59702716, 18ce3a4a).  Of
> course I wish I'd identified and fixed all of those things *before*
> the original commits, but that's how it played out...
>

It might be too late but I found that the following entry is
categorized in Monitoring. But in PostgreSQL 9.6 release note, the
feature related to default role is categorized in Permissions
Management. I think the adding new default roles can be categorized in
the same category to not confuse users and personally it's more
suitable.

"Add default monitoring roles (Dave Page)"

Thought?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


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


Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization

2017-09-01 Thread Masahiko Sawada
On Fri, Sep 1, 2017 at 4:42 PM, Fabien COELHO  wrote:
>
> Hello Masahiko-san,
>
> Patch applies and compiles.
>
> One bug found, and some minor points again. Sorry for this hopefully last
> iteration... I'm kind of an iterative person...
>
> I've generated the doc to look a it.
>
> Short option "-I" does not use a "=", it should be "-I
> custom_init_commands".
>
> Also maybe it would look nicer and clearer if the short mnemonic was outside
> the literal, that is with:
>
>   c (cleanup)
>
> instead of:
>
>   c (cleanup)
>
> But this is debatable. Do it the way you think is best.
>
> Command "g" does not work after "f", something I had not tested before:
>
>  ./pgbench -i -I ctvpfg
>  cleaning up...
>  creating tables...
>  vacuum...
>  set primary keys...
>  set foreign keys...
>  ERROR:  cannot truncate a table referenced in a foreign key constraint
>  DETAIL:  Table "pgbench_history" references "pgbench_accounts".
>  HINT:  Truncate table "pgbench_history" at the same time, or use TRUNCATE
> ... CASCADE.
>
> I think it should work. It probably just mean to TRUNCATE all tables as one
> command, or add the suggested CASCADE. I would favor the first option.
>
> I'm wondering whether this truncation should be yet another available
> command? Hmmm... maybe not.

Currently TRUNCATE pgbench_accounts command is executed within a
transaction started immediately before it. If we move it out of the
transaction, the table data will be truncated even if the copying data
failed. Maybe we can do TRUNCATE pgbench_accounts, pgbench_history
instead. Thought?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


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


Re: [HACKERS] adding the commit to a patch's thread

2017-09-01 Thread Alvaro Herrera
Erik Rijkers wrote:

> Would it be possible to change the commitfest a bit and make it possible to
> add the commit (or commit-message, or hash) to the thread in the
> commitfest-app.

+1 to add one or more commit hashes to CF entry metadata.

(Back-filling for old entries welcome)

-- 
Á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] pgbench: Skipping the creating primary keys after initialization

2017-09-01 Thread Fabien COELHO


Hello Masahiko-san,

Patch applies and compiles.

One bug found, and some minor points again. Sorry for this hopefully last 
iteration... I'm kind of an iterative person...


I've generated the doc to look a it.

Short option "-I" does not use a "=", it should be "-I 
custom_init_commands".


Also maybe it would look nicer and clearer if the short mnemonic was 
outside the literal, that is with:


  c (cleanup)

instead of:

  c (cleanup)

But this is debatable. Do it the way you think is best.

Command "g" does not work after "f", something I had not tested before:

 ./pgbench -i -I ctvpfg
 cleaning up...
 creating tables...
 vacuum...
 set primary keys...
 set foreign keys...
 ERROR:  cannot truncate a table referenced in a foreign key constraint
 DETAIL:  Table "pgbench_history" references "pgbench_accounts".
 HINT:  Truncate table "pgbench_history" at the same time, or use TRUNCATE ... 
CASCADE.

I think it should work. It probably just mean to TRUNCATE all tables as 
one command, or add the suggested CASCADE. I would favor the first option.


I'm wondering whether this truncation should be yet another available 
command? Hmmm... maybe not.


--
Fabien.


--
Sent 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 fix] Savepoint-related statements terminates connection

2017-09-01 Thread Michael Paquier
On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs  wrote:
> I'm not sure I see the use case for anyone using SAVEPOINTs in this
> context, so simply throwing a good error message is enough.
>
> Clearly nobody is using this, so lets just lock the door. I don't
> think fiddling with the transaction block state machine is anything
> anybody wants to do in back branches, at least without a better reason
> than this.

I don't think you can say that, per se the following recent report:
https://www.postgresql.org/message-id/cah2-v61vxnentfj2v-zd+ma-g6kqmjgd5svxou3jbvdzqh0...@mail.gmail.com
-- 
Michael


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


Re: [HACKERS] utility commands benefiting from parallel plan

2017-09-01 Thread Haribabu Kommi
Hi All,

Attached a rebased patch that supports parallelism for the queries
that are underneath of some utility commands such as CREATE TABLE AS
and CREATE MATERIALIZED VIEW.

Note: This patch doesn't make the utility statement (insert operation)
to run in parallel. It only allows the select query to be parallel if the
query
is eligible for parallel.

Regards,
Hari Babu
Fujitsu Australia


0001-Make-parallel-eligible-for-utility-commands-undernea_V3.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


  1   2   >