Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/02/15 6:26, Alvaro Herrera wrote:
> Another option is to rethink this feature from the ground up: instead of
> cloning catalog rows for each children, maybe we should have the trigger
> lookup code, when running DML on the child relation (the partition),
> obtain trigger entries not only for the child relation itself but also
> for its parents recursively -- so triggers defined in the parent are
> fired for the partitions, too.  I'm not sure what implications this has
> for constraint triggers.
>
> The behavior should be the same, except that you cannot modify the
> trigger (firing conditions, etc) on the partition individually -- it
> works at the level of the whole partitioned table instead.

Do you mean to fire these triggers only if the parent table (not a child
table/partition) is addressed in the DML, right?  If the table directly
addressed in the DML is a partition whose parent has a row-level trigger,
then that trigger should not get fired I suppose.

Thanks,
Amit

Re: autovacuum: change priority of the vacuumed tables

[Masahiko Sawada]

Hi,

On Thu, Feb 8, 2018 at 11:01 PM, Ildus Kurbangaliev
 wrote:
> Hi,
>
> Attached patch adds 'autovacuum_table_priority' to the current list of
> automatic vacuuming settings. It's used in sorting of vacuumed tables in
> autovacuum worker before actual vacuum.
>
> The idea is to give possibility to the users to prioritize their tables
> in autovacuum process.
>

Hmm, I couldn't understand the benefit of this patch. Would you
elaborate it a little more?

Multiple autovacuum worker can work on one database. So even if a
table that you want to vacuum first is the back of the list and there
other worker would pick up it. If the vacuuming the table gets delayed
due to some big tables are in front of that table I think you can deal
with it by increasing the number of autovacuum workers.

Regards,

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

Re: Cached/global query plans, autopreparation

[he...@visionlink.org]

Any idea on how feasible it would be as an extention or is the work too
central to abstract that way?

 Chet Henry
Senior Software Developer - Dev Ops Liaison
VisionLink, Inc.
3101 Iris Ave, Ste 240
Boulder, CO 80301
  he...@visionlink.org


Site  | Blog  | Join
Our Team  | Try a
Demo 
[image: Twitter]    [image: Pinterest]
   [image: Facebook]
   [image:
LinkedIn]

   [image: YouTube]


On Wed, Feb 14, 2018 at 2:19 PM, Tom Lane  wrote:

> Andres Freund  writes:
> > On 2018-02-13 09:13:09 -0800, Shay Rojansky wrote:
> >> Was wondering if anyone has a reaction to my email below about statement
> >> preparation, was it too long? :)
>
> > Well, the issue is that implementing this is a major piece of work. This
> > post doesn't offer either resources nor a simpler way to do so. There's
> > no huge debate about the benefit of having a global plan cache, so I'm
> > not that surprised there's not a huge debate about a post arguing that
> > we should have one.
>
> Actually, I'm pretty darn skeptical about the value of such a cache for
> most use-cases.  But as long as it can be turned off and doesn't leave
> residual overhead nor massive added code cruft, I won't stand in the way
> of someone else investing their time in it.
>
> In any case, as you say, it's moot until somebody steps up to do it.
>
> regards, tom lane
>
>

Re: Python 3.7 support

[Peter Eisentraut]

On 2/13/18 21:45, Michael Paquier wrote:
> On Tue, Feb 13, 2018 at 04:17:13PM -0500, Peter Eisentraut wrote:
>> A small patch to tweak the tests to support output differences with
>> Python 3.7 (currently in beta).
> 
> Wouldn't it be better to wait for the version to be released before
> pushing anything in the tree?  If there are again changes when this
> comes out as GA then an extra patch would be needed.

Sure, we can just leave it here for the record until the release comes
closer.

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

Re: reorganizing partitioning code

[Amit Langote]

On 2018/02/15 5:30, Alvaro Herrera wrote:
> BTW may I suggest that
> 
>   git config diff.algorithm=histogram
> 
> results in better diffs?

Aha!  That's much better.

Thanks,
Amit

Re: JIT compiling with LLVM v10.1

[Andres Freund]

Hi,

On 2018-02-14 23:32:17 +0100, Pierre Ducroquet wrote:
> Here are the LLVM4 and LLVM3.9 compatibility patches.
> Successfully built, and executed some silly queries with JIT forced to make 
> sure it worked.

Thanks!

I'm going to integrate them into my series in the next few days.

Regards,

Andres

Re: JIT compiling with LLVM v10.1

[Pierre Ducroquet]

On Wednesday, February 14, 2018 7:17:10 PM CET Andres Freund wrote:
> Hi,
> 
> On 2018-02-07 06:54:05 -0800, Andres Freund wrote:
> > I've pushed v10.0. The big (and pretty painful to make) change is that
> > now all the LLVM specific code lives in src/backend/jit/llvm, which is
> > built as a shared library which is loaded on demand.
> > 
> > The layout is now as follows:
> > 
> > src/backend/jit/jit.c:
> > Part of JITing always linked into the server. Supports loading the
> > LLVM using JIT library.
> > 
> > src/backend/jit/llvm/
> > 
> > Infrastructure:
> >  llvmjit.c:
> > General code generation and optimization infrastructure
> >  
> >  llvmjit_error.cpp, llvmjit_wrap.cpp:
> > Error / backward compat wrappers
> >  
> >  llvmjit_inline.cpp:
> > Cross module inlining support
> > 
> > Code-Gen:
> >   llvmjit_expr.c
> >   
> > Expression compilation
> >   
> >   llvmjit_deform.c
> >   
> > Deform compilation
> 
> I've pushed a revised version that hopefully should address Jeff's
> wish/need of being able to experiment with this out of core. There's now
> a "jit_provider" PGC_POSTMASTER GUC that's by default set to
> "llvmjit". llvmjit.so is the .so implementing JIT using LLVM. It fills a
> set of callbacks via
> extern void _PG_jit_provider_init(JitProviderCallbacks *cb);
> which can also be implemented by any other potential provider.
> 
> The other two biggest changes are that I've added a README
> https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=blob;
> f=src/backend/jit/README;hb=jit and that I've revised the configure support
> so it does more error
> checks, and moved it into config/llvm.m4.
> 
> There's a larger smattering of small changes too.
> 
> I'm pretty happy with how the separation of core / shlib looks now. I'm
> planning to work on cleaning and then pushing some of the preliminary
> patches (fixed tupledesc, grouping) over the next few days.
> 
> Greetings,
> 
> Andres Freund

Hi

Here are the LLVM4 and LLVM3.9 compatibility patches.
Successfully built, and executed some silly queries with JIT forced to make 
sure it worked.

 Pierre>From c856a5db2f0ba34ba7c230a65f60277ae0e7347f Mon Sep 17 00:00:00 2001
From: Pierre 
Date: Fri, 2 Feb 2018 09:11:55 +0100
Subject: [PATCH 1/8] Add support for LLVM4 in llvmjit.c

Signed-off-by: Pierre Ducroquet 
---
 src/backend/jit/llvm/llvmjit.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 7a96ece0f7..7557dc9a19 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -222,13 +222,20 @@ llvm_get_function(LLVMJitContext *context, const char *funcname)
 
 		addr = 0;
 		if (LLVMOrcGetSymbolAddressIn(handle->stack, , handle->orc_handle, mangled))
-			elog(ERROR, "failed to lookup symbol");
+			elog(ERROR, "failed to lookup symbol %s", mangled);
 		if (addr)
 			return (void *) addr;
 	}
 
 #else
 
+#if LLVM_VERSION_MAJOR < 5
+	if ((addr = LLVMOrcGetSymbolAddress(llvm_opt0_orc, mangled)))
+		return (void *) addr;
+	if ((addr = LLVMOrcGetSymbolAddress(llvm_opt3_orc, mangled)))
+		return (void *) addr;
+	elog(ERROR, "failed to lookup symbol %s for %s", mangled, funcname);
+#else
 	if (LLVMOrcGetSymbolAddress(llvm_opt0_orc, , mangled))
 		elog(ERROR, "failed to lookup symbol");
 	if (addr)
@@ -237,6 +244,8 @@ llvm_get_function(LLVMJitContext *context, const char *funcname)
 		elog(ERROR, "failed to lookup symbol");
 	if (addr)
 		return (void *) addr;
+#endif // LLVM_VERSION_MAJOR
+
 #endif
 
 	elog(ERROR, "failed to JIT: %s", funcname);
@@ -374,11 +383,18 @@ llvm_compile_module(LLVMJitContext *context)
 	 * faster instruction selection mechanism is used.
 	 */
 	{
-		LLVMSharedModuleRef smod;
 		instr_time tb, ta;
 
 		/* emit the code */
 		INSTR_TIME_SET_CURRENT(ta);
+#if LLVM_VERSION_MAJOR < 5
+		orc_handle = LLVMOrcAddEagerlyCompiledIR(compile_orc, context->module,
+			 llvm_resolve_symbol, NULL);
+		// It seems there is no error return from that function in LLVM < 5.
+#else
+		LLVMSharedModuleRef smod;
+
+		LLVMSharedModuleRef smod;
 		smod = LLVMOrcMakeSharedModule(context->module);
 		if (LLVMOrcAddEagerlyCompiledIR(compile_orc, _handle, smod,
 		llvm_resolve_symbol, NULL))
@@ -386,6 +402,7 @@ llvm_compile_module(LLVMJitContext *context)
 			elog(ERROR, "failed to jit module");
 		}
 		LLVMOrcDisposeSharedModuleRef(smod);
+#endif
 		INSTR_TIME_SET_CURRENT(tb);
 		INSTR_TIME_SUBTRACT(tb, ta);
 		ereport(DEBUG1, (errmsg("time to emit: %.3fs",
-- 
2.16.1

>From a44378f05c33a40c485f26e5f007614100c70fe7 Mon Sep 17 00:00:00 2001
From: Pierre 
Date: Fri, 2 Feb 2018 09:13:40 +0100
Subject: [PATCH 2/8] Add LLVM4 support in llvmjit_error.cpp

Signed-off-by: Pierre Ducroquet 
---
 src/backend/jit/llvm/llvmjit_error.cpp | 6 ++
 1 file changed, 6 

Re: Is this a bug?

[Tatsuo Ishii]

>> "User was holding shared buffer pin for too long" sounds unusual to
>> me. Is this a bug? PostgreSQL version is 10.1 according to the user.
> 
> Sounds normal to me.  Maybe the message could use some work, but this
> is how hot standby works: if max_standby_archive_delay or
> max_standby_streaming_delay expires, whatever is blocking recovery
> gets blasted out of the way.

I didn't know that recovery conflict could happen even with buffer
pin. I should have examined the source code first. Sorry for noise.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: tapeblocks is uninitialized in logtape.c

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Actually, on looking closer, it's more likely got something to do with
>> whether or not you used --enable-cassert.  If the Assert at the top of
>> the function is disabled, then IMO a compiler that failed to complain
>> about this coding would be pretty damn broken.

> I tried removing the Assert, and my compiler doesn't complain.  I
> noticed that the function is static and the only caller has its own
> assert about ntapes; yet removing that one doesn't cause a warning
> either.  I made the function non-static -- still no warning.

If I make the function non-static, I get a warning (note you can't
just remove the "static" on the function itself, you also have to
change the forward declaration earlier in the file).  Marking the
function pg_noinline also results in a warning.

It is darn weird that there's no warning after inlining --- the caller
is only asserting ntapes > 0, which is not strong enough to prove
that the loop must be iterated.  Moreover, I see the warning even
if I leave in the "Assert(lts->nTapes >= 2)" ... so my compiler, at
least, is not using those assertions to prove the loop must iterate.

I notice that, because "lt" is initialized to NULL, the compiler
might feel it can assume that the loop iterated at least once
if control gets to the use of tapeblocks.  But the whole thing
still seems weird.  I think we must be looking at some squishy
spots in gcc's rules about when to issue a warning.

regards, tom lane

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Robert Haas wrote:
> On Tue, Jan 23, 2018 at 5:10 PM, Alvaro Herrera  
> wrote:

> >> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
> >> partitioned tables?
> >
> > Haven't done this yet, either.  I like Simon's suggestion of outright
> > disallowing this.
> 
> Why not just make it work?

I haven't had as much time to work on this as I wished, so progress has
been a bit slow.  That is to say, this version is almost identical to
the one I last posted.  I added a test for enable/disable trigger, which
currently fails because the code to support it is not implemented.  I
added a report of the trigger name to the relevant test, for improved
visibility of what is happening.  (I intend to push that one, since it's
a trivial improvement.)

Now one way to fix that would be to do as Amit suggests elsewhere, ie.,
to add a link to parent trigger from child trigger, so we can search for
children whenever the parent is disabled.  We'd also need a new index on
that column so that the searches are fast, and perhaps a boolean flag
("trghaschildren") to indicate that searches must be done.
(We could add an array of children OID instead, but designwise that
seems much worse.)

Another option is to rethink this feature from the ground up: instead of
cloning catalog rows for each children, maybe we should have the trigger
lookup code, when running DML on the child relation (the partition),
obtain trigger entries not only for the child relation itself but also
for its parents recursively -- so triggers defined in the parent are
fired for the partitions, too.  I'm not sure what implications this has
for constraint triggers.

The behavior should be the same, except that you cannot modify the
trigger (firing conditions, etc) on the partition individually -- it
works at the level of the whole partitioned table instead.

For foreign key triggers to work properly, I think I'd propose that this
occurs only for non-internal triggers.  For internal triggers,
particularly FK triggers, we continue with the current approach in that
patch which is to create trigger clones.

This seems more promising to me.

Thoughts?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 6f9dd70fd0690294d8cf27de40830421a2340a1f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 13 Feb 2018 19:47:16 -0300
Subject: [PATCH v3 1/3] Mention trigger name in test

---
 src/test/regress/expected/triggers.out | 42 +-
 src/test/regress/sql/triggers.sql  |  2 +-
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/test/regress/expected/triggers.out 
b/src/test/regress/expected/triggers.out
index 98db323337..e7b4b31afc 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1865,7 +1865,7 @@ create table parted2_stmt_trig1 partition of 
parted2_stmt_trig for values in (1)
 create table parted2_stmt_trig2 partition of parted2_stmt_trig for values in 
(2);
 create or replace function trigger_notice() returns trigger as $$
   begin
-raise notice 'trigger on % % % for %', TG_TABLE_NAME, TG_WHEN, TG_OP, 
TG_LEVEL;
+raise notice 'trigger % on % % % for %', TG_NAME, TG_TABLE_NAME, TG_WHEN, 
TG_OP, TG_LEVEL;
 if TG_LEVEL = 'ROW' then
return NEW;
 end if;
@@ -1910,12 +1910,12 @@ create trigger trig_del_after after delete on 
parted2_stmt_trig
 with ins (a) as (
   insert into parted2_stmt_trig values (1), (2) returning a
 ) insert into parted_stmt_trig select a from ins returning tableoid::regclass, 
a;
-NOTICE:  trigger on parted_stmt_trig BEFORE INSERT for STATEMENT
-NOTICE:  trigger on parted2_stmt_trig BEFORE INSERT for STATEMENT
-NOTICE:  trigger on parted_stmt_trig1 BEFORE INSERT for ROW
-NOTICE:  trigger on parted_stmt_trig1 AFTER INSERT for ROW
-NOTICE:  trigger on parted2_stmt_trig AFTER INSERT for STATEMENT
-NOTICE:  trigger on parted_stmt_trig AFTER INSERT for STATEMENT
+NOTICE:  trigger trig_ins_before on parted_stmt_trig BEFORE INSERT for 
STATEMENT
+NOTICE:  trigger trig_ins_before on parted2_stmt_trig BEFORE INSERT for 
STATEMENT
+NOTICE:  trigger trig_ins_before on parted_stmt_trig1 BEFORE INSERT for ROW
+NOTICE:  trigger trig_ins_after on parted_stmt_trig1 AFTER INSERT for ROW
+NOTICE:  trigger trig_ins_after on parted2_stmt_trig AFTER INSERT for STATEMENT
+NOTICE:  trigger trig_ins_after on parted_stmt_trig AFTER INSERT for STATEMENT
  tableoid  | a 
 ---+---
  parted_stmt_trig1 | 1
@@ -1925,25 +1925,25 @@ NOTICE:  trigger on parted_stmt_trig AFTER INSERT for 
STATEMENT
 with upd as (
   update parted2_stmt_trig set a = a
 ) update parted_stmt_trig  set a = a;
-NOTICE:  trigger on parted_stmt_trig BEFORE UPDATE for STATEMENT
-NOTICE:  trigger on parted_stmt_trig1 BEFORE UPDATE for ROW
-NOTICE:  trigger on parted2_stmt_trig BEFORE UPDATE for 

Re: Cached/global query plans, autopreparation

[Tom Lane]

Andres Freund  writes:
> On 2018-02-13 09:13:09 -0800, Shay Rojansky wrote:
>> Was wondering if anyone has a reaction to my email below about statement
>> preparation, was it too long? :)

> Well, the issue is that implementing this is a major piece of work. This
> post doesn't offer either resources nor a simpler way to do so. There's
> no huge debate about the benefit of having a global plan cache, so I'm
> not that surprised there's not a huge debate about a post arguing that
> we should have one.

Actually, I'm pretty darn skeptical about the value of such a cache for
most use-cases.  But as long as it can be turned off and doesn't leave
residual overhead nor massive added code cruft, I won't stand in the way
of someone else investing their time in it.

In any case, as you say, it's moot until somebody steps up to do it.

regards, tom lane

Re: tapeblocks is uninitialized in logtape.c

[Tom Lane]

Jaime Casanova  writes:
> i tried to fix the ones in your 2nd attachment, but i'm not real sure
> about what value initialize the typename array in objectaddress.c and
> the bool values in pgbench.c (both of them i initialized to NULL)

Pushed with minor adjustments.  Notably, I didn't like the way you'd
fixed the issues in pgbench.c.  The issue seems to be that those compilers
note that coerceToBool doesn't initialize its output argument in all
code paths, and don't note that the callers only examine the output
if it returns true.  So I thought it was better to make coerceToBool
always set the output to something.  That way we aren't totally giving
up error detection in the callers: if there were a code path that used
the output variable without even having called coerceToBool, we'd still
hear about it.

regards, tom lane

Re: tapeblocks is uninitialized in logtape.c

[Alvaro Herrera]

Tom Lane wrote:

> Actually, on looking closer, it's more likely got something to do with
> whether or not you used --enable-cassert.  If the Assert at the top of
> the function is disabled, then IMO a compiler that failed to complain
> about this coding would be pretty damn broken.

I tried removing the Assert, and my compiler doesn't complain.  I
noticed that the function is static and the only caller has its own
assert about ntapes; yet removing that one doesn't cause a warning
either.  I made the function non-static -- still no warning.

gcc (Debian 6.3.0-18) 6.3.0 20170516

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

Re: [PATCH][PROPOSAL] Add enum releation option type

[Nikolay Shaplov]

В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал:

> > 1. I've changed error message from 'Valid values are "XXX", "YYY" and
> > "ZZZ".' to 'Valid values are "XXX", "YYY", "ZZZ".' to make a code a bit
> > simpler. If it is not acceptable, please let me know, I will add "and" to
> > the string.
> I don't think we care about this, but is this still the case if you use
> a stringinfo?
>
> > 2. Also about the string with the list of acceptable values: the code that
> > creates this string is inside parse_one_reloption function now.
>
> I think you could save most of that mess by using appendStringInfo and
> friends.

Here is the second verion of the patch where I use appendStringInfo to prepare
error message. The code is much more simple here now, and it now create value
list with "and" at the end: '"xxx", "yyy" and "zzz"'


--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..82a0492 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,9 @@ static relopt_real realRelOpts[]  	{{NULL}}
 };

-static relopt_string stringRelOpts[] +static const char *gist_buffering_enum_names[] = GIST_OPTION_BUFFERING_VALUE_NAMES;
+static const char *view_check_option_names[] = VIEW_OPTION_CHECK_OPTION_VALUE_NAMES;
+static relopt_enum enumRelOpts[]  {
 	{
 		{
@@ -413,10 +415,8 @@ static relopt_string stringRelOpts[]  			RELOPT_KIND_GIST,
 			AccessExclusiveLock
 		},
-		4,
-		false,
-		gistValidateBufferingOption,
-		"auto"
+		gist_buffering_enum_names,
+		GIST_OPTION_BUFFERING_AUTO
 	},
 	{
 		{
@@ -425,11 +425,14 @@ static relopt_string stringRelOpts[]  			RELOPT_KIND_VIEW,
 			AccessExclusiveLock
 		},
-		0,
-		true,
-		validateWithCheckOption,
-		NULL
+		view_check_option_names,
+		VIEW_OPTION_CHECK_OPTION_NOT_SET
 	},
+	{{NULL}}
+};
+
+static relopt_string stringRelOpts[] +{
 	/* list terminator */
 	{{NULL}}
 };
@@ -476,6 +479,12 @@ initialize_reloptions(void)
    realRelOpts[i].gen.lockmode));
 		j++;
 	}
+	for (i = 0; enumRelOpts[i].gen.name; i++)
+	{
+		Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+   enumRelOpts[i].gen.lockmode));
+		j++;
+	}
 	for (i = 0; stringRelOpts[i].gen.name; i++)
 	{
 		Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -514,6 +523,14 @@ initialize_reloptions(void)
 		j++;
 	}

+	for (i = 0; enumRelOpts[i].gen.name; i++)
+	{
+		relOpts[j] = [i].gen;
+		relOpts[j]->type = RELOPT_TYPE_ENUM;
+		relOpts[j]->namelen = strlen(relOpts[j]->name);
+		j++;
+	}
+
 	for (i = 0; stringRelOpts[i].gen.name; i++)
 	{
 		relOpts[j] = [i].gen;
@@ -611,6 +628,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
 		case RELOPT_TYPE_REAL:
 			size = sizeof(relopt_real);
 			break;
+		case RELOPT_TYPE_ENUM:
+			size = sizeof(relopt_enum);
+			break;
 		case RELOPT_TYPE_STRING:
 			size = sizeof(relopt_string);
 			break;
@@ -690,6 +710,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
 }

 /*
+ * add_enuum_reloption
+ *		Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+	 const char **allowed_values, int default_val)
+{
+	relopt_enum *newoption;
+
+	newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+   name, desc);
+	newoption->allowed_values = allowed_values;
+	newoption->default_val = default_val;
+
+	add_reloption((relopt_gen *) newoption);
+}
+
+/*
  * add_string_reloption
  *		Add a new string reloption
  *
@@ -1190,6 +1228,50 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
 	   optreal->min, optreal->max)));
 			}
 			break;
+		case RELOPT_TYPE_ENUM:
+			{
+relopt_enum *opt_enum = (relopt_enum *) option->gen;
+int			i = 0;
+
+parsed = false;
+while (opt_enum->allowed_values[i])
+{
+	if (pg_strcasecmp(value, opt_enum->allowed_values[i]) == 0)
+	{
+		option->values.enum_val = i;
+		parsed = true;
+		break;
+	}
+	i++;
+}
+if (!parsed)
+{
+	StringInfoData buf;
+
+	initStringInfo();
+	i = 0;
+	while (opt_enum->allowed_values[i])
+	{
+		appendStringInfo(,"\"%s\"",
+		 opt_enum->allowed_values[i]);
+		if (opt_enum->allowed_values[i + 1])
+		{
+			if (opt_enum->allowed_values[i + 2])
+appendStringInfo(,", ");
+			else
+appendStringInfo(," and ");
+		}
+		i++;
+	}
+	ereport(ERROR,
+			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+			 errmsg("invalid value for \"%s\" option",
+	option->gen->name),
+			 errdetail("Valid values are %s.", /*str*/ buf.data)));
+	pfree(buf.data);
+}
+			}
+			break;
 		case RELOPT_TYPE_STRING:
 			{
 relopt_string *optstring = (relopt_string *) option->gen;
@@ -1283,6 +1365,11 @@ fillRelOptions(void *rdopts, Size basesize,
 			

Re: tapeblocks is uninitialized in logtape.c

[Tom Lane]

Peter Eisentraut  writes:
> On 2/13/18 23:10, Tom Lane wrote:
>> Jaime Casanova  writes:
>>> My compiler gives me this message
>>> logtape.c: In function ‘ltsConcatWorkerTapes’:
>>> logtape.c:462:48: warning: ‘tapeblocks’ may be used uninitialized in
>>> this function [-Wmaybe-uninitialized]
>>> lts->nBlocksAllocated = lt->offsetBlockNumber + tapeblocks;

>> FWIW, I'm not seeing that.  What compiler are you using exactly?

> This warning comes from using -Og instead of -O2.

Actually, on looking closer, it's more likely got something to do with
whether or not you used --enable-cassert.  If the Assert at the top of
the function is disabled, then IMO a compiler that failed to complain
about this coding would be pretty damn broken.

regards, tom lane

Re: tapeblocks is uninitialized in logtape.c

[Tom Lane]

Peter Eisentraut  writes:
> On 2/13/18 23:10, Tom Lane wrote:
>> FWIW, I'm not seeing that.  What compiler are you using exactly?

> This warning comes from using -Og instead of -O2.
> It's the only such warning, so it would be nice to silence it, because
> using -Og is somewhat useful.

There's a different one of the eight (I forget which exactly) that is
the only warning I see when building on some BSD platforms I have laying
about.  So I think they're probably all annoying, just to different
people.  I'm inclined to pick up Jaime's patch to silence all of them.

regards, tom lane

Re: reorganizing partitioning code (was: Re: [HACKERS] path toward faster partition pruning)

[Alvaro Herrera]

This is looking attractive.

Please don't #include postgres.h in partcache.h.  That's per policy.

Why do you need to #include parsenodes.h in partcache.h?

I think rewriteManip.h can do with just relcache.h rather than rel.h
(probably partition.h can do likewise)

thanks

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

[HACKERS] Inserting data into a new catalog table via source code

[rqtx]

The company I'm working is currently trying to give some external users access 
to our database. Said users are partners of ours (company partners).

I was tasked to create a new catalog table to log every user activity (already 
done) and now I'm trying to insert some information in this table using the 
CatalogTupleInsert function. The data in question is the query the user issues 
to psql and the payload of the answer.

My thought process was that I would be able to alter the source code so I would 
insert this kind of data for every user, him being superuser or not...

Instead, I noticed the data is only  inserted in the table if the forked 
process (the transaction) is from a user which has the superuser flag...

Is there a way to make this function work with any user?

Re: Cancelling parallel query leads to segfault

[Andres Freund]

On 2018-02-12 15:43:49 -0500, Peter Eisentraut wrote:
> On 2/6/18 12:06, Andres Freund wrote:
> > On 2018-02-06 12:01:08 -0500, Peter Eisentraut wrote:
> >> On 2/1/18 20:35, Andres Freund wrote:
> >>> On February 1, 2018 11:13:06 PM GMT+01:00, Peter Eisentraut
> >>>  wrote:
>  Here is a patch to implement that idea. Do you have a way to test it
>  repeatedly, or do you just randomly cancel queries?
> >>>
> >>> For me cancelling the long running parallel queries I tried reliably
> >>> triggers the issue. I encountered it while cancelling tpch q1 during JIT
> >>> work.
> >>
> >> Why does canceling a query result in elog(FATAL)?  It should just be
> >> elog(ERROR), which wouldn't trigger this issue.
> > 
> > The workers are shut down.
> 
> I have used the setup mentioned in
> 
> to reproduce this, without success.  I have tried statement_timeout and
> manual cancels.  Any other ideas?
> 
> I don't doubt that the issue exists, but it would be nice to be able to
> reproduce it.

With your example I can reliably trigger the issue if I shut down the
server while the query is running:

^C2018-02-14 10:54:06.786 PST [22261][] LOG:  received fast shutdown request
2018-02-14 10:54:06.786 PST [22261][] LOG:  aborting any active transactions
2018-02-14 10:54:06.786 PST [22275][4/3] FATAL:  terminating connection due to 
administrator command
2018-02-14 10:54:06.786 PST [22275][4/3] STATEMENT:  select from t1 where a = 
55;
2018-02-14 10:54:06.786 PST [22274][5/3] FATAL:  terminating connection due to 
administrator command
2018-02-14 10:54:06.786 PST [22274][5/3] STATEMENT:  select from t1 where a = 
55;
2018-02-14 10:54:06.786 PST [22271][3/2] FATAL:  terminating connection due to 
administrator command
2018-02-14 10:54:06.786 PST [22271][3/2] STATEMENT:  select from t1 where a = 
55;
2018-02-14 10:54:06.787 PST [22261][] LOG:  background worker "logical 
replication launcher" (PID 22268) exited with exit code 1
2018-02-14 10:54:06.787 PST [22261][] LOG:  background worker "parallel worker" 
(PID 22274) exited with exit code 1
2018-02-14 10:54:06.787 PST [22261][] LOG:  background worker "parallel worker" 
(PID 22275) exited with exit code 1
2018-02-14 10:54:06.788 PST [22261][] LOG:  server process (PID 22271) was 
terminated by signal 11: Segmentation fault
2018-02-14 10:54:06.788 PST [22261][] DETAIL:  Failed process was running: 
select from t1 where a = 55;
2018-02-14 10:54:06.788 PST [22261][] LOG:  terminating any other active server 
processes
2018-02-14 10:54:06.789 PST [22285][] FATAL:  the database system is shutting 
down
2018-02-14 10:54:06.789 PST [22261][] LOG:  abnormal database system shutdown
2018-02-14 10:54:06.790 PST [22261][] LOG:  database system is shut down

but only if I don't use EXPLAIN ANALYZE. Not quite sure what that is
about.

Your patch appears to fix the issue.

Greetings,

Andres Freund

Re: [COMMITTERS] pgsql: Rearm statement_timeout after each executed query.

[Andres Freund]

On 2018-02-09 15:50:44 -0500, Robert Haas wrote:
> On Wed, Feb 7, 2018 at 7:04 PM, Thomas Munro
>  wrote:
> > On Tue, Feb 6, 2018 at 5:21 PM, Peter Eisentraut
> >  wrote:
> >> On 9/18/17 22:41, Andres Freund wrote:
> >>> Rearm statement_timeout after each executed query.
> >>
> >> This appears to have broken statement_timeout behavior in master such
> >> that only every second query is affected by it.
> >
> > Yeah, I also just ran into this while testing a nearby complaint about
> > statement timeouts vs parallel query.  In the error path
> > stmt_timeout_active remains true, so the next statement does nothing
> > in enable_statement_timeout().  I think we just need to clear that
> > flag in the error path, right where we call disable_all_timeouts().
> > See attached.
> 
> Looks right.  Committed, but I thought the comment was strange (forget
> about?) so I just left that out.

Thanks Peter, Thomas, Robert!

- Andres

Re: JIT compiling with LLVM v10.1

[Andres Freund]

Hi,

On 2018-02-07 06:54:05 -0800, Andres Freund wrote:
> I've pushed v10.0. The big (and pretty painful to make) change is that
> now all the LLVM specific code lives in src/backend/jit/llvm, which is
> built as a shared library which is loaded on demand.
> 
> The layout is now as follows:
> 
> src/backend/jit/jit.c:
> Part of JITing always linked into the server. Supports loading the
> LLVM using JIT library.
> 
> src/backend/jit/llvm/
> Infrastructure:
>  llvmjit.c:
> General code generation and optimization infrastructure
>  llvmjit_error.cpp, llvmjit_wrap.cpp:
> Error / backward compat wrappers
>  llvmjit_inline.cpp:
> Cross module inlining support
> Code-Gen:
>   llvmjit_expr.c
> Expression compilation
>   llvmjit_deform.c
> Deform compilation

I've pushed a revised version that hopefully should address Jeff's
wish/need of being able to experiment with this out of core. There's now
a "jit_provider" PGC_POSTMASTER GUC that's by default set to
"llvmjit". llvmjit.so is the .so implementing JIT using LLVM. It fills a
set of callbacks via
extern void _PG_jit_provider_init(JitProviderCallbacks *cb);
which can also be implemented by any other potential provider.

The other two biggest changes are that I've added a README
https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=blob;f=src/backend/jit/README;hb=jit
and that I've revised the configure support so it does more error
checks, and moved it into config/llvm.m4.

There's a larger smattering of small changes too.

I'm pretty happy with how the separation of core / shlib looks now. I'm
planning to work on cleaning and then pushing some of the preliminary
patches (fixed tupledesc, grouping) over the next few days.

Greetings,

Andres Freund

Re: Google Summer of Code 2018

[Stephen Frost]

Greetings,

* Ankit Raj (rajankitn...@gmail.com) wrote:
> I want to participate in GSOC2018 in the PostgreSQl organization, so can
> you help me out as to what I have to preapre?

The best thing to do is to review the Student Guide, available here:

https://g.co/gsoc/studentguide

As discussed there, you'll be writing a proposal for the specific
project that you want to work on over the summer.  Included in the
Student Guide is a list of the elements which make up a qualify
proposal.

To develop the proposal, you'll want to discuss your project idea on
this list and/or with the prospective mentors.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Cached/global query plans, autopreparation

[he...@visionlink.org]

​Coming from a PHP application I have several of the same concerns and
wishes​.  Given that php can not share any (resources) between requests it
would be impossible to accomplish what you have in .NET.  We still prepare
statements though for use in result sets and other loops (ORM driven).  I'm
wondering if it is possible to solve this as an extension to postgres?  If
Citus could refactor their code into an extension surly this is simple in
comparison.  I would be willing to help but PHP has made me soft, so it
will take a bit longer.

Thanks,

Chet Henry

On Tue, Feb 13, 2018 at 10:13 AM, Shay Rojansky  wrote:

> Hi all,
>
> Was wondering if anyone has a reaction to my email below about statement
> preparation, was it too long? :)
>
> (and sorry for top-posting)
>
> On Tue, Feb 6, 2018 at 9:27 PM, Shay Rojansky  wrote:
>
>> Hi all.
>>
>> Various versions of having PostgreSQL caching and/or autopreparing
>> statement plans have been discussed (https://www.postgresql.org/me
>> ssage-id/op.t9ggb3wacigqcu%40apollo13.peufeu.com, https://
>> www.postgresql.org/message-id/8e76d8fc-8b8c-14bd-d4d1-e9cf19
>> 3a74f5%40postgrespro.ru), without clear conclusions or even an agreement
>> on what might be worthwhile to implement. I wanted to bring this up again
>> from a PostgreSQL driver maintainer's perspective (I'm the owner of Npgsql,
>> the open source .NET driver), apologies in advance if I'm repeating things
>> or I've missed crucial information. Below I'll describe three relevant
>> issues and what I've done to deal with them.
>>
>> When the same statement is rerun, preparing it has a very significant
>> performance boost. However, in short-lived connection scenarios it's
>> frequently not possible to benefit from this - think of a typical webapp
>> which allocates a connection from a pool, run a query and then return the
>> connection. To make sure prepared statements are used, Npgsql's connection
>> pool doesn't send DISCARD ALL when a connection is returned (to avoid
>> wiping out the connections), and maintains an internal table mapping SQL
>> (and parameter types) to a PostgreSQL statement name. The next time the
>> application attempts to prepare the same SQL, the prepared statement is
>> found in the table and no preparation needs to occur. This means that
>> prepared statements persist across pooled connection open/close, and are
>> never discarded unless the user uses a specific API. While this works, the
>> disadvantages are that:
>> 1. This kind of mechanism needs to be implemented again and again, in
>> each driver:
>> 2. It relies on Npgsql's internal pooling, which can track persistent
>> prepared statements on physical connections. If an external pool is used
>> (e.g. pgpool), this isn't really possible.
>> 1. It complicates resetting the session state (instead of DISCARD ALL, a
>> combination of all other reset commands except DEALLOCATE ALL needs be
>> sent). This is minor.
>>
>> The second issue is that many applications don't work directly against
>> the database API (ADO.NET in .NET, JDBC in Java). If any sort of O/RM or
>> additional layer is used, there's a good chance that that layer doesn't
>> prepare in any way, and indeed hide your access to the database API's
>> preparation method. Two examples from the .NET world is dapper (a very
>> popular micro-O/RM) and Entity Framework. In order to provide the best
>> possible performance in these scenarios, Npgsql has an opt-in feature
>> whereby it tracks how many times a given statement was executed, and once
>> it passes a certain threshold automatically prepares it. An LRU cache is
>> then used to determine which prepared statements to discard, to avoid
>> explosion. In effect, statement auto-preparation is implemented in the
>> driver. I know that the JDBC driver also implements such a mechanism (it
>> was actually the inspiration for the Npgsql feature). The issues with this
>> are:
>>
>> 1. As above, this has to be implemented by every driver (and is quite
>> complex to do well)
>> 2. There's a possible missed opportunity in having a single plan on the
>> server, as each connection has its own (the "global plan" option). Many
>> apps out there send the same statements across many connections so this
>> seems relevant - but I don't know if the gains outweigh the contention
>> impact in PostgreSQL.
>>
>> Finally, since quite a few (most?) other databases include
>> autopreparation (SQL Server, Oracle...), users porting their applications -
>> which don't explicitly prepare - experience a big performance drop. It can
>> rightly be said that porting an application across databases isn't a
>> trivial task and that adjustments need to be made, but from experience I
>> can say that PostgreSQL is losing quite a few users to this.
>>
>> The above issues could be helped by having PostgreSQL cache on its side
>> (especially the second issue, which is the most important). Ideally, any
>> adopted solution would be 

Re: tapeblocks is uninitialized in logtape.c

[Jaime Casanova]

On 13 February 2018 at 23:10, Tom Lane  wrote:
> Jaime Casanova  writes:
>>> My compiler gives me this message
>>> logtape.c: In function ‘ltsConcatWorkerTapes’:
>>> logtape.c:462:48: warning: ‘tapeblocks’ may be used uninitialized in
>>> this function [-Wmaybe-uninitialized]
>>> lts->nBlocksAllocated = lt->offsetBlockNumber + tapeblocks;
>
> FWIW, I'm not seeing that.  What compiler are you using exactly?
>

gcc version 6.3.0 20170516 (Debian 6.3.0-18)

> (There are one or two other places where I see "may be used uninitialized"
> complaints from certain older gcc versions.  Not sure how excited we
> should be about removing such warnings.)
>

i tried to fix the ones in your 2nd attachment, but i'm not real sure
about what value initialize the typename array in objectaddress.c and
the bool values in pgbench.c (both of them i initialized to NULL)

-- 
Jaime Casanova  www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index de869e00ff..5bea073a2b 100644
*** a/src/backend/access/transam/xloginsert.c
--- b/src/backend/access/transam/xloginsert.c
***
*** 584,590  XLogRecordAssemble(RmgrId rmid, uint8 info,
  		if (include_image)
  		{
  			Page		page = regbuf->page;
! 			uint16		compressed_len;
  
  			/*
  			 * The page needs to be backed up, so calculate its hole length
--- 584,590 
  		if (include_image)
  		{
  			Page		page = regbuf->page;
! 			uint16		compressed_len = 0;
  
  			/*
  			 * The page needs to be backed up, so calculate its hole length
diff --git a/src/backend/catalog/objectaddressindex b4c2467710..3fa5b6e1db 100644
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
***
*** 1576,1583  get_object_address_opf_member(ObjectType objtype,
  	ObjectAddress address;
  	ListCell   *cell;
  	List	   *copy;
! 	TypeName   *typenames[2];
! 	Oid			typeoids[2];
  	int			membernum;
  	int			i;
  
--- 1576,1583 
  	ObjectAddress address;
  	ListCell   *cell;
  	List	   *copy;
! 	TypeName   *typenames[2] = { NULL };
! 	Oid			typeoids[2] = { InvalidOid };
  	int			membernum;
  	int			i;
  
diff --git a/src/backend/utils/sort/logtapindex 66bfcced8d..d6794bf3de 100644
*** a/src/backend/utils/sort/logtape.c
--- b/src/backend/utils/sort/logtape.c
***
*** 411,417  ltsConcatWorkerTapes(LogicalTapeSet *lts, TapeShare *shared,
  	 SharedFileSet *fileset)
  {
  	LogicalTape *lt = NULL;
! 	long		tapeblocks;
  	long		nphysicalblocks = 0L;
  	int			i;
  
--- 411,417 
  	 SharedFileSet *fileset)
  {
  	LogicalTape *lt = NULL;
! 	long		tapeblocks = 0L;
  	long		nphysicalblocks = 0L;
  	int			i;
  
diff --git a/src/bin/pgbench/pgbench.c index 31ea6ca06e..8b0c8bcaa8 100644
*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
***
*** 1614,1620  evalLazyFunc(TState *thread, CState *st,
  			 PgBenchFunction func, PgBenchExprLink *args, PgBenchValue *retval)
  {
  	PgBenchValue a1, a2;
! 	bool ba1, ba2;
  
  	Assert(isLazyFunc(func) && args != NULL && args->next != NULL);
  
--- 1614,1620 
  			 PgBenchFunction func, PgBenchExprLink *args, PgBenchValue *retval)
  {
  	PgBenchValue a1, a2;
! 	bool ba1 = NULL, ba2 = NULL;
  
  	Assert(isLazyFunc(func) && args != NULL && args->next != NULL);
  
***
*** 1935,1941  evalStandardFunc(
  			/* logical operators */
  		case PGBENCH_NOT:
  			{
! bool b;
  if (!coerceToBool([0], ))
  	return false;
  
--- 1935,1941 
  			/* logical operators */
  		case PGBENCH_NOT:
  			{
! bool b = NULL;
  if (!coerceToBool([0], ))
  	return false;
  

Re: master plpython check fails on Solaris 10

[Tom Lane]

Marina Polyakova  writes:
> On 14-02-2018 17:54, Tom Lane wrote:
>> I think we need to fix the error callback code so that it
>> uses the "arg" field to find the relevant procedure, and that
>> that's a back-patchable fix, because nested plpython functions
>> would show this up as wrong in any case.  That would also let us
>> undo the not-terribly-safe practice of having code between the
>> PLy_push_execution_context call and the PG_TRY that is supposed
>> to ensure the context gets popped again.

> Thank you very much! I'll try to implement this.

I hsve a patch mostly done already.

regards, tom lane

Re: [HACKERS] MERGE SQL Statement for PG11

[Stephen Frost]

Greetings Pavan,

* Pavan Deolasee (pavan.deola...@gmail.com) wrote:
> On Tue, Feb 6, 2018 at 3:37 PM, Stephen Frost  wrote:
> > Coming out of that, my understanding is that Simon is planning to have a
> > patch which implements RLS and partitioning (though the query plans for
> > partitioning may be sub-par and not ideal) as part of MERGE and I've
> > agreed to review at least the RLS bits (though my intention is to at
> > least go through the rest of the patch as well, though likely in less
> > detail).  Of course, I encourage others to review it as well.
>
> Thanks for volunteering to review the RLS bits. I am planning to send a
> revised version soon. As I work through it, I am faced with some semantic
> questions again. Would appreciate if you or anyone have answers to those.

Thanks for working on this.

> While executing MERGE, for existing tuples in the target table, we may end
> up doing an UPDATE or DELETE, depending on the WHEN MATCHED AND conditions.
> So it seems unlikely that we would be able to push down USING security
> quals down to the scan. For example, if the target row is set for deletion,
> it seems wrong to exclude the row from the join based on UPDATE policy's
> USING quals. So I am thinking that we should apply the respective USING
> quals *after* the decision to either update, delete or do nothing for the
> given target tuple is made.
> 
> The question I have is, if the USING qual evaluates to false or NULL,
> should we silently ignore the tuple (like regular UPDATE does) or throw an
> error (like INSERT ON CONFLICT DO UPDATE)? ISTM that we might have decided
> to throw an error in case of INSERT ON CONFLICT to avoid any confusion
> where the tuple is neither inserted nor updated. Similar situation may
> arise with MERGE because for a source row, we may neither do UPDATE
> (because of RLS) nor INSERT because a matching tuple already exists. But
> someone may argue that we should stay closer to regular UPDATE/DELETE.

The reasoning behind the INSERT ON CONFLICT DO UPDATE approach when it
comes to RLS is that we specifically didn't want to end up "losing"
data- an INSERT which doesn't actually INSERT a row (or take the UPDATE
action if the row already exists) ends up throwing that data away even
though clearly the user expected us to do something with it, which is
why we throw an error in that case instead.

For my part, at least, it seems like MERGE would likewise possibly be
throwing away data that the user is expecting to be incorporated if no
action is taken due to RLS and that then argues that we should be
throwing an error in such a case, similar to the INSERT ON CONFLICT DO
UPDATE case.

> Apart from that, are there any security angles that we need to be mindful
> of and would those impact the choice?

Regarding the above, no security issues come to mind with either
approach.  The security concerns are primairly not allowing rows to be
directly seen which the user does not have access to, and not allowing
the action to result in rows being added, modified, or removed in a way
which would violate the policies defined.

> SELECT policies will be applied to the target table during the scan and
> rows which do not pass SELECT quals will not be processed at all. If there
> are NOT MATCHED actions, we might end up inserting duplicate rows in that
> case or throw errors, but I don't see anything wrong with that. Similar
> things would have happened if someone tried to insert rows into the table
> using regular INSERT.

I agree, this seems like the right approach.

> Similarly, INSERT policies will be applied when MERGE attempts to INSERT a
> row into the table and error will be thrown if the row does not satisfy
> INSERT policies.

That sounds correct to me.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: master plpython check fails on Solaris 10

[Marina Polyakova]

On 14-02-2018 17:54, Tom Lane wrote:

Marina Polyakova  writes:

On 14-02-2018 3:43, Peter Eisentraut wrote:

OK, can you get some kind of stack trace or other debugging
information?



I got this backtrace from gdb:


Hmm, so the only question in my mind is how did this ever work for 
anyone?


The basic problem here is that, instead of using the
ErrorContextCallback.arg field provided for the purpose,
plpython_error_callback is using PLy_current_execution_context()
to try to identify the context it's supposed to report on.
In the example, that points to the context associated with the
inner DO block, not with the outer procedure.  That context
looks like it should reliably have curr_proc->proname == NULL,
so how come this test case doesn't crash for everybody?

In any case the expected output for the transaction_test4
case is obviously wrong.  Rather than reporting the transaction_test4
function and then the inner DO block, it's reporting 2 levels
of transaction_test4.  That seems to trace directly to both levels
of error context callback looking at the same execution context.

I think we need to fix the error callback code so that it
uses the "arg" field to find the relevant procedure, and that
that's a back-patchable fix, because nested plpython functions
would show this up as wrong in any case.  That would also let us
undo the not-terribly-safe practice of having code between the
PLy_push_execution_context call and the PG_TRY that is supposed
to ensure the context gets popped again.


Thank you very much! I'll try to implement this.


While I'm bitching ... I wonder how long it's been since the
comment for PLy_procedure_name() had anything to do with its
actual behavior.


AFAIU this has always been like that, since the commit 1ca717f3...

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Bug in to_timestamp().

[Dmitry Dolgov]

> On 12 February 2018 at 12:49, Arthur Zakirov 
wrote:
>
> Yes, I somehow missed it. I changed the behaviour of separator
> characters in format string. They are greedy now and eat whitespaces
> before a separator character in an input string. I suppose that there
> should be no such problems as in [1].

Thanks. I have a few minor complains about some noise:

* On line 52 there is a removed empty line, without any other changes around

* On line 177 there is a new commented out line of code. I assume it's not
an
  explanation or something and we don't need it, am I right?

Also, I spotted one more difference between this patch and Oracle. In the
situation, when a format string doesn't have anything meaningful, with the
patch we've got:

SELECT to_timestamp('2000 + JUN', '/');
  to_timestamp
-
0001-01-01 00:00:00+00:53:28 BC
(1 row)

SELECT to_timestamp('2000 + JUN', ' ');
  to_timestamp
-
0001-01-01 00:00:00+00:53:28 BC
(1 row)

And Oracle complains about this:

SELECT to_timestamp('2000 + JUN', ' /') FROM dual
ORA-01830: date format picture ends before converting entire input string

SELECT to_timestamp('2000 + JUN', ' ') FROM dual
ORA-01830: date format picture ends before converting entire input string

It's sort of corner case, but anyway maybe you would be interested to handle
it.

>> About usage of locale dependent functions e.g. `isalpha`. Yes, looks like
>> it's better to have locale-agnostic implementation, but then I'm
confused - all
>> functions except `isdigit`/`isxdigit` are locale-dependent, including
>> `isspace`, which is also in use.
>
> I returned is_separator_char() function for now.

Thanks. Answering my own question about `isspace`, I finally noticed, that
this
function was used before the patch, and there were no complains - so
probably
it's fine.

Re: [HACKERS] Client Connection redirection support for PostgreSQL

[Robert Haas]

On Tue, Feb 13, 2018 at 5:23 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> -- might need some defense against the redirected-to server getting
>> the same password as was sent to the original server.  Is that a
>> security risk?  Does HTTP have a rule about this?
>
> Without having read any of the previous discussion ... I'd say that if the
> redirect info is placed in pg_hba.conf then I would expect a redirect to
> happen before any authentication exchange, so that this is not an issue.
> Perhaps it would be a good security measure for clients to refuse a
> redirect once they've sent any auth-related messages.
>
> But ... pg_hba.conf?  Really?  Surely that is a completely random and
> inappropriate place to control redirection?

Where would you suggest?

My thought was that if, for example, you migrated one database off of
a multiple database cluster to a new location, you'd want to redirect
anyone trying to connect to that database to the new server, so you
need to put the redirection facility someplace where we can make
decisions about whether or not to redirect based on rules involving
database names.  The other things we expose in pg_hba.conf seem like
they could potentially be useful, too, although maybe less so.  For
instance, if you've got several standbys (or several masters connected
via some MMR solution) you could redirect connections which come from
the "wrong" IP block to the server to which they are local.  I think
of pg_hba.conf as a place where we decide what to do with connections,
and redirecting them seems like one thing we might decide to do.

I hadn't really thought deeply about whether redirection should happen
before or after authentication.  For the most part, before seems
better, because it seems a bit silly to force people to authenticate
just so that you can tell them to go someplace else.  Also, that would
lead to double authentication, which might for example result in
multiple password prompts, which users might either dislike or find
confusing.  The only contrary argument that comes to mind is that
someone could argue that there's a security leakage --- if someone has
a redirect rule that only engages for a particular user or database
name, then you can perhaps guess that the user or database name exists
on the target system, or that in general it's one that they care
about.  However, reject rules already have the same exposure.
Similarly, you might also be able to infer something based on the type
of authentication request that you get from the server.  So I don't
see this argument as compelling.

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

Re: master plpython check fails on Solaris 10

[Tom Lane]

I wrote:
> so how come this test case doesn't crash for everybody?

I traced through this, and what I see is that the error information
constructed at the time of the inner ereport includes

0x1f98528 "invalid transaction termination", detail = 0x0, detail_log = 
0x0, hint = 0x0, context = 
0x1f98598 "PL/Python anonymous code block\nSQL statement \"DO LANGUAGE 
plpythonu $x$ plpy.commit() $x$\"\nPL/Python function \"(null)\"", message_id = 
0xa0fc50 "invalid transaction termination", schema_name = 0x0, 

So, in fact, we *are* passing a null pointer to sprintf here.
Apparently, none of the machines in the buildfarm crash on that,
or at least none of the ones building plpython do, which is
pretty troubling from a test coverage standpoint.  I suggest that
it might be a good idea to insert an "Assert(strvalue != NULL)"
in src/port/snprintf.c, approx. line 748, so that at least the
machines using that fallback implementation are guaranteed to
whine about this type of mistake.

The reason we don't see this obviously-bogus output in the test
results is that when control exits the outer Python function,
the inner error result is thrown away during
PLy_abort_open_subtransactions, and then we generate an all-new error
report with "PLy_elog(ERROR, NULL)".  That contains only Python's
own traceback plus the output from a (duplicate) call of
plpython_error_callback, which now gets the right answer because
PLy_execution_contexts is now pointing at the outer function's
context.

So I'm still thinking I can construct a test case in which plpython
outputs a clearly-wrong context stack, but it will have to involve
elog(NOTICE) rather than ERROR.  Will work on that.

regards, tom lane

Re: [HACKERS] Partition-wise aggregation/grouping

[Jeevan Chalke]

On Wed, Feb 14, 2018 at 12:17 PM, Rafia Sabih 
wrote:

> On Tue, Feb 13, 2018 at 6:21 PM, Jeevan Chalke <
> jeevan.cha...@enterprisedb.com> wrote:
>
>>
>> I see that partition-wise aggregate plan too uses parallel index, am I
>> missing something?
>>
>>
> You're right, I missed that, oops.
>
>>
>>> Q18 takes some 390 secs with patch and some 147 secs without it.
>>>
>>
>> This looks strange. This patch set does not touch parallel or seq scan as
>> such. I am not sure why this is happening. All these three queries explain
>> plan shows much higher execution time for parallel/seq scan.
>>
>> Yeah strange it is.
>

Off-list I have asked Rafia to provide me the perf machine access where she
is doing this bench-marking to see what's going wrong.
Thanks Rafia for the details.

What I have observed that, there are two sources, one with HEAD and other
with HEAD+PWA. However the configuration switches were different. Sources
with HEAD+PWA has CFLAGS="-ggdb3 -O0" CXXFLAGS="-ggdb3 -O0" flags in
addition with other sources. i.e. HEAD+PWA is configured with
debugging/optimization enabled which account for the slowness.

I have run EXPLAIN for these three queries on both the sources having
exactly same configuration switches and I don't find any slowness with PWA
patch-set.

Thus, it will be good if you re-run the benchmark by keeping configuration
switches same on both the sources and share the results.

Thanks



> However, do you see similar behaviour with patches applied,
>> "enable_partition_wise_agg = on" and "enable_partition_wise_agg = off" ?
>>
>
> I tried that for query 18, with patch and  enable_partition_wise_agg =
> off, query completes in some 270 secs. You may find the explain analyse
> output for it in the attached file. I noticed that on head the query plan
> had parallel hash join however with patch and no partition-wise agg it is
> using nested loop joins. This might be the issue.
>
>>
>> Also, does rest of the queries perform better with partition-wise
>> aggregates?
>>
>>
> As far as this setting goes, there wasn't any other query using
> partition-wise-agg, so, no.
>
> BTW, just an FYI, this experiment is on scale factor 20.
>
> --
> Regards,
> Rafia Sabih
> EnterpriseDB: http://www.enterprisedb.com/
>


-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: master plpython check fails on Solaris 10

[Tom Lane]

Marina Polyakova  writes:
> On 14-02-2018 3:43, Peter Eisentraut wrote:
>> OK, can you get some kind of stack trace or other debugging 
>> information?

> I got this backtrace from gdb:

Hmm, so the only question in my mind is how did this ever work for anyone?

The basic problem here is that, instead of using the
ErrorContextCallback.arg field provided for the purpose,
plpython_error_callback is using PLy_current_execution_context()
to try to identify the context it's supposed to report on.
In the example, that points to the context associated with the
inner DO block, not with the outer procedure.  That context
looks like it should reliably have curr_proc->proname == NULL,
so how come this test case doesn't crash for everybody?

In any case the expected output for the transaction_test4
case is obviously wrong.  Rather than reporting the transaction_test4
function and then the inner DO block, it's reporting 2 levels
of transaction_test4.  That seems to trace directly to both levels
of error context callback looking at the same execution context.

I think we need to fix the error callback code so that it
uses the "arg" field to find the relevant procedure, and that
that's a back-patchable fix, because nested plpython functions
would show this up as wrong in any case.  That would also let us
undo the not-terribly-safe practice of having code between the
PLy_push_execution_context call and the PG_TRY that is supposed
to ensure the context gets popped again.

While I'm bitching ... I wonder how long it's been since the
comment for PLy_procedure_name() had anything to do with its
actual behavior.

regards, tom lane

Kerberos test suite

[Peter Eisentraut]

Here is a patch with a test suite for the Kerberos/GSSAPI authentication
functionality.  It's very similar in principle to the recently added
LDAP tests, and similar caveats apply.

You will need the client and server parts of a krb5 package
installation, possibly named krb5-workstation and krb5-server, or
perhaps krb5-user and krb5-kdc.

(If it appears to hang for you in the "setting up Kerberos" step, you
might need more entropy/wait a while.  That problem appears to be
limited to some virtual machine setups, but the specifics are not clear.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 108530921d7a3780cd5d1a31aebadd29621de429 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 14 Feb 2018 09:10:50 -0500
Subject: [PATCH] Tests for Kerberos/GSSAPI authentication

---
 configure   |   2 +
 configure.in|   1 +
 src/Makefile.global.in  |   1 +
 src/test/kerberos/.gitignore|   2 +
 src/test/kerberos/Makefile  |  25 +++
 src/test/kerberos/README|  25 +++
 src/test/kerberos/t/001_auth.pl | 143 
 src/test/perl/PostgresNode.pm   |   8 ++-
 8 files changed, 205 insertions(+), 2 deletions(-)
 create mode 100644 src/test/kerberos/.gitignore
 create mode 100644 src/test/kerberos/Makefile
 create mode 100644 src/test/kerberos/README
 create mode 100644 src/test/kerberos/t/001_auth.pl

diff --git a/configure b/configure
index 7dcca506f8..84b547be79 100755
--- a/configure
+++ b/configure
@@ -708,6 +708,7 @@ with_uuid
 with_systemd
 with_selinux
 with_openssl
+with_krb_srvnam
 krb_srvtab
 with_python
 with_perl
@@ -5814,6 +5815,7 @@ fi
 
 
 
+
 cat >>confdefs.h <<_ACEOF
 #define PG_KRB_SRVNAM "$with_krb_srvnam"
 _ACEOF
diff --git a/configure.in b/configure.in
index 4d26034579..dbd7fa2c48 100644
--- a/configure.in
+++ b/configure.in
@@ -650,6 +650,7 @@ PGAC_ARG_REQ(with, krb-srvnam,
  [NAME], [default service principal name in Kerberos (GSSAPI) 
[postgres]],
  [],
  [with_krb_srvnam="postgres"])
+AC_SUBST(with_krb_srvnam)
 AC_DEFINE_UNQUOTED([PG_KRB_SRVNAM], ["$with_krb_srvnam"],
[Define to the name of the default PostgreSQL service 
principal in Kerberos (GSSAPI). (--with-krb-srvnam=NAME)])
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d980f81046..1c6c0d3eea 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -188,6 +188,7 @@ with_selinux= @with_selinux@
 with_systemd   = @with_systemd@
 with_libxml= @with_libxml@
 with_libxslt   = @with_libxslt@
+with_krb_srvnam= @with_krb_srvnam@
 with_system_tzdata = @with_system_tzdata@
 with_uuid  = @with_uuid@
 with_zlib  = @with_zlib@
diff --git a/src/test/kerberos/.gitignore b/src/test/kerberos/.gitignore
new file mode 100644
index 00..871e943d50
--- /dev/null
+++ b/src/test/kerberos/.gitignore
@@ -0,0 +1,2 @@
+# Generated by test suite
+/tmp_check/
diff --git a/src/test/kerberos/Makefile b/src/test/kerberos/Makefile
new file mode 100644
index 00..1363529498
--- /dev/null
+++ b/src/test/kerberos/Makefile
@@ -0,0 +1,25 @@
+#-
+#
+# Makefile for src/test/kerberos
+#
+# Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/kerberos/Makefile
+#
+#-
+
+subdir = src/test/kerberos
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+export with_krb_srvnam
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
+
+clean distclean maintainer-clean:
+   rm -rf tmp_check
diff --git a/src/test/kerberos/README b/src/test/kerberos/README
new file mode 100644
index 00..94cf7ff42e
--- /dev/null
+++ b/src/test/kerberos/README
@@ -0,0 +1,25 @@
+src/test/kerberos/README
+
+Tests for Kerberos/GSSAPI functionality
+===
+
+This directory contains a test suite for Kerberos/GSSAPI
+functionality.  This requires a full MIT Kerberos installation,
+including server and client tools, and is therefore kept separate and
+not run by default.  You might need to adjust some paths in the test
+file to have it find MIT Kerberos in a place that hadn't been thought
+of yet.
+
+Also, this test suite creates a KDC server that listens for TCP/IP
+connections on localhost without any real access control, so it is not
+safe to run this on a system where there might be untrusted local
+users.
+
+Running the tests
+=
+
+make check
+
+or
+
+make installcheck
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
new file mode 100644
index 00..5d6f3df8bf
--- /dev/null
+++ 

Re: Is this a bug?

[Robert Haas]

On Wed, Feb 14, 2018 at 7:35 AM, Tatsuo Ishii  wrote:
> Today one of Pgpool-II users reported an interesting fatal error
> message from PostgreSQL:
>
> FATAL: terminating connection due to conflict with recovery
> DETAIL: User was holding shared buffer pin for too long.
> HINT: In a moment you should be able to reconnect to the database and repeat 
> your command.
>
> "User was holding shared buffer pin for too long" sounds unusual to
> me. Is this a bug? PostgreSQL version is 10.1 according to the user.

Sounds normal to me.  Maybe the message could use some work, but this
is how hot standby works: if max_standby_archive_delay or
max_standby_streaming_delay expires, whatever is blocking recovery
gets blasted out of the way.

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

Re: Is this a bug?

[Sergei Kornilov]

Hello
No, this is not bug. 
https://www.postgresql.org/docs/10/static/hot-standby.html#HOT-STANDBY-CONFLICT
Message in DETAIL describe conflict reason.

> Application of a vacuum cleanup record from WAL conflicts with queries 
> accessing the target page on the standby, whether or not the data to be 
> removed is visible.
this is your case

regards, Sergei

Is this a bug?

[Tatsuo Ishii]

Today one of Pgpool-II users reported an interesting fatal error
message from PostgreSQL:

FATAL: terminating connection due to conflict with recovery
DETAIL: User was holding shared buffer pin for too long.
HINT: In a moment you should be able to reconnect to the database and repeat 
your command.

"User was holding shared buffer pin for too long" sounds unusual to
me. Is this a bug? PostgreSQL version is 10.1 according to the user.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key

[Amit Kapila]

On Tue, Feb 13, 2018 at 12:41 PM, amul sul  wrote:
> On Tue, Feb 13, 2018 at 11:32 AM, Amit Kapila  wrote:
>>
>> Your change appears fine to me.  I think one can set both block number
>> and offset as we do for  HeapTupleHeaderIsSpeculative, but the way you
>> have done it looks good to me.  Kindly include it in the next version
>> of your patch by adding the missing comment.
>>
>
> Thanks for the confirmation, updated patch attached.
>

+# Concurrency error from GetTupleForTrigger
+# Concurrency error from ExecLockRows

I think you don't need to mention above sentences in spec files.
Apart from that, your patch looks good to me.  I have marked it as
Ready For Committer.

Notes for Committer -
1. We might need some changes in update-tuple-routing mechanism if we
decide to do anything for the bug [1] discussed in the nearby thread,
but as that is not directly related to this patch, we can move ahead.
2. I think it is better to document that for update tuple routing the
delete+insert will be replayed separately on replicas.  I leave this
to the discretion of the committer.

[1] - 
https://www.postgresql.org/message-id/CAAJ_b94bYxLsX0erZXVH-anQPbWqcYUPWX4xVRa1YJY%3DPh60ZQ%40mail.gmail.com

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

Re: master plpython check fails on Solaris 10

[Marina Polyakova]

On 14-02-2018 3:43, Peter Eisentraut wrote:

On 2/13/18 05:40, Marina Polyakova wrote:

Binary search has shown that this failure begins with commit
8561e4840c81f7e345be2df170839846814fa004 (Transaction control in PL
procedures.). On the previous commit
(b9ff79b8f17697f3df492017d454caa9920a7183) there's no
plpython_transaction test and plpython check passes.


OK, can you get some kind of stack trace or other debugging 
information?


I got this backtrace from gdb:

Program received signal SIGSEGV, Segmentation fault.
0x7d13bb50 in strlen () from /lib/64/libc.so.1
(gdb) bt
#0  0x7d13bb50 in strlen () from /lib/64/libc.so.1
#1  0x7d1aabf8 in _ndoprnt () from /lib/64/libc.so.1
#2  0x7d1ad3d4 in vsnprintf () from /lib/64/libc.so.1
#3  0x0001009ab174 in pvsnprintf (
buf=0x100d680e3 "PL/Python function \"", '\177' times>..., len=933,
fmt=0x100d684a0 "PL/Python function \"%s\"", 
args=0x7fff77f8) at psprintf.c:121

#4  0x000100492bd0 in appendStringInfoVA (str=0x7fff7738,
fmt=0x100d684a0 "PL/Python function \"%s\"", 
args=0x7fff77f8) at stringinfo.c:130
#5  0x000100911038 in errcontext_msg (fmt=0x7aa1eb98 
"PL/Python function \"%s\"")

at elog.c:1021
#6  0x7aa11ea8 in plpython_error_callback (arg=0x100e7d7e8) at 
plpy_main.c:402

#7  0x00010090e9f8 in errfinish (dummy=0) at elog.c:438
#8  0x000100482e78 in SPI_commit () at spi.c:211
#9  0x7aa13e90 in PLy_commit (self=0x0, args=0x0) at 
plpy_plpymodule.c:602
#10 0x71a1ca40 in PyEval_EvalFrameEx (f=0x70619548, 
throwflag=2147449896)

at Python/ceval.c:4004
#11 0x71a1d8a0 in PyEval_EvalFrameEx (f=0x706193a0, 
throwflag=2147450456)

at Python/ceval.c:4106
#12 0x71a1ed20 in PyEval_EvalCodeEx (co=0x7060e6b0, 
globals=0x705236b4,
locals=0x71bf9940 <_PyThreadState_Current>, args=0x0, 
argcount=0, kws=0x0, kwcount=0,

defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3252
#13 0x71a1ef64 in PyEval_EvalCode (co=0x7060e6b0, 
globals=0x70525c58,

locals=0x70525c58) at Python/ceval.c:667
#14 0x7aa10938 in PLy_procedure_call (proc=0x7fff8580,
kargs=0x7aa1e128 "args", vargs=0x7060d290) at 
plpy_exec.c:1031
#15 0x7aa0cf08 in PLy_exec_function (fcinfo=0x7fff86f0, 
proc=0x7fff8580)

at plpy_exec.c:107
#16 0x7aa11c64 in plpython_inline_handler 
(fcinfo=0x7fff8be0) at plpy_main.c:353
#17 0x00010091e44c in OidFunctionCall1Coll (functionId=16385, 
collation=0, arg1=4311287992)

at fmgr.c:1327
#18 0x0001003746ec in ExecuteDoStmt (stmt=0x100f8fa70, atomic=1 
'\001') at functioncmds.c:2183

#19 0x0001006f420c in standard_ProcessUtility (pstmt=0x100f90768,
queryString=0x70528054 "DO LANGUAGE plpythonu $x$ 
plpy.commit() $x$",
context=PROCESS_UTILITY_QUERY, params=0x0, queryEnv=0x0, 
dest=0x100cd6708 ,

completionTag=0x7fff93f8 "") at utility.c:533
#20 0x0001006f3b04 in ProcessUtility (pstmt=0x100f90768,
queryString=0x70528054 "DO LANGUAGE plpythonu $x$ 
plpy.commit() $x$",
context=PROCESS_UTILITY_QUERY, params=0x0, queryEnv=0x0, 
dest=0x100cd6708 ,

completionTag=0x7fff93f8 "") at utility.c:358
#21 0x0001004886e0 in _SPI_execute_plan (plan=0x7fff95b0, 
paramLI=0x0, snapshot=0x0,
crosscheck_snapshot=0x0, read_only=0 '\000', fire_triggers=1 '\001', 
tcount=0) at spi.c:2207

#22 0x000100483980 in SPI_execute (
src=0x70528054 "DO LANGUAGE plpythonu $x$ plpy.commit() 
$x$", read_only=0 '\000',

tcount=0) at spi.c:416
#23 0x7aa17698 in PLy_spi_execute_query (
query=0x70528054 "DO LANGUAGE plpythonu $x$ plpy.commit() 
$x$", limit=0)

at plpy_spi.c:331
#24 0x7aa16c3c in PLy_spi_execute (self=0x0, 
args=0x7051fb50) at plpy_spi.c:168
#25 0x719af824 in PyCFunction_Call (func=0x7062a098, 
arg=0x7051fb50, kw=0x0)

at Objects/methodobject.c:116
#26 0x71a1cfb0 in PyEval_EvalFrameEx (f=0x7af29c20, 
throwflag=2147457704)

at Python/ceval.c:4020
#27 0x71a1d8a0 in PyEval_EvalFrameEx (f=0x706191f8, 
throwflag=2147458264)

at Python/ceval.c:4106
#28 0x71a1ed20 in PyEval_EvalCodeEx (co=0x7072cf30, 
globals=0x70523624,
locals=0x71bf9940 <_PyThreadState_Current>, args=0x0, 
argcount=0, kws=0x0, kwcount=0,

defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3252
#29 0x71a1ef64 in PyEval_EvalCode (co=0x7072cf30, 
globals=0x70525910,

locals=0x70525910) at Python/ceval.c:667
---Type  to continue, or q  to quit---
#30 0x7aa10938 in PLy_procedure_call (proc=0x100f91278, 
kargs=0x7aa1e128 "args",

vargs=0x7060d0e0) at plpy_exec.c:1031
#31 0x7aa0cf08 in PLy_exec_function 

Re: [HACKERS] why not parallel seq scan for slow functions

[Amit Kapila]

On Fri, Feb 2, 2018 at 12:15 AM, Robert Haas  wrote:
> On Wed, Jan 31, 2018 at 11:48 PM, Amit Kapila  wrote:
>
> The other cases aren't so clear.  In the case of the first call within
> create_ordered_paths, there's no loss in the !is_sorted case because
> apply_projection_to_path will be getting called on a Sort path.  But
> when is_sorted is true, the current code can push a target list into a
> Gather or Gather Merge that was created with some other target list,
> and with the patch it can't.  I'm not quite sure what sort of query
> would trigger that problem, but it seems like there's something to
> worry about there.
>

I think the query plans which involve Gather Merge -> Parallel Index
Scan can be impacted.  For ex. cosider below case:

With parallel-paths-tlist-cost-rmh-v2.patch
postgres=# set cpu_operator_cost=0;
SET
postgres=# set parallel_setup_cost=0;set parallel_tuple_cost=0;set
min_parallel_table_scan_size=0;set
max_parallel_workers_per_gather=4;
SET
SET
SET
SET
postgres=# explain (costs off, verbose) select simple_func(aid) from
pgbench_accounts where aid > 1000 order by aid;
  QUERY PLAN
---
 Gather Merge
   Output: simple_func(aid), aid
   Workers Planned: 4
   ->  Parallel Index Only Scan using pgbench_accounts_pkey on
public.pgbench_accounts
 Output: aid
 Index Cond: (pgbench_accounts.aid > 1000)
(6 rows)

HEAD and parallel_paths_include_tlist_cost_v7
postgres=# explain (costs off, verbose) select simple_func(aid) from
pgbench_accounts where aid > 1000 order by aid;
  QUERY PLAN
---
 Gather Merge
   Output: (simple_func(aid)), aid
   Workers Planned: 4
   ->  Parallel Index Only Scan using pgbench_accounts_pkey on
public.pgbench_accounts
 Output: simple_func(aid), aid
 Index Cond: (pgbench_accounts.aid > 1000)
(6 rows)

For the above test, I have initialized pgbench with 100 scale factor.

It shows that with patch parallel-paths-tlist-cost-rmh-v2.patch, we
will lose the capability to push target list in some cases.  One lame
idea could be that at the call location, we detect if it is a Gather
(Merge) and then push the target list, but doing so at all the
locations doesn't sound to be a good alternative as compared to
another approach where we push target list in
apply_projection_to_path.

>  Similarly I can't see any obvious reason why this
> isn't a problem for adjust_path_for_srfs and build_minmax_path as
> well, although I haven't tried to construct queries that hit those
> cases, either.
>

Neither do I, but I can give it a try if we expect something different
than the results of above example.

> Now, we could just give up on this approach and leave that code in
> apply_projection_to_path, but what's bothering me is that, presumably,
> any place where that code is actually getting used has the same
> problem that we're trying to fix in grouping_planner: the tlist evals
> costs are not being factored into the decision as to which path we
> should choose, which might make a good parallel path lose to an
> inferior non-parallel path.
>

Your concern is valid, but isn't the same problem exists in another
approach as well, because in that also we can call
adjust_paths_for_srfs after generating gather path which means that we
might lose some opportunity to reduce the relative cost of parallel
paths due to tlists having SRFs.  Also, a similar problem can happen
in create_order_paths  for the cases as described in the example
above.

>  It would be best to fix that throughout
> the code base rather than only fixing the more common paths -- if we
> can do so with a reasonable amount of work.
>

Agreed, I think one way to achieve that is instead of discarding
parallel paths based on cost, we retain them till the later phase of
planning, something like what we do for ordered paths.  In that case,
the way changes have been done in the patch
parallel_paths_include_tlist_cost_v7 will work.  I think it will be
helpful for other cases as well if we keep the parallel paths as
alternative paths till later stage of planning (we have discussed it
during parallelizing subplans as well), however, that is a bigger
project on its own.  I don't think it will be too bad to go with what
we have in parallel_paths_include_tlist_cost_v7 with the intent that
in future when we do the other project, the other cases will be
automatically dealt.

I have updated the patch parallel_paths_include_tlist_cost_v7 by
changing some of the comments based on
parallel-paths-tlist-cost-rmh-v2.patch.

I have one additional comment on parallel-paths-tlist-cost-rmh-v2.patch
@@ -374,6 +374,14 @@ cost_gather(GatherPath *path, PlannerInfo *root,
  startup_cost += parallel_setup_cost;
  run_cost += parallel_tuple_cost * 

Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

[Ashutosh Bapat]

On Wed, Feb 14, 2018 at 2:50 PM, Etsuro Fujita
 wrote:
>
> I'd vote for #1, but ISTM that that's more like a feature, not a fix.
> Pushing down ConvertRowtypeExprs to the remote seems to me to be the same
> problem as pushing down PHVs to the remote, which is definitely a feature
> development.  I think a fix for this would be to just give up pushing down a
> child join in foreign_join_ok or somewhere else if the reltarget of the
> child-join relation contains any ConvertRowtypeExprs.

That means we won't be able to push down any scans under a DML on a
partitioned table. That will be too restrictive.

I think the case of PHV and ConvertRowtypeExprs are different. The the
former we need a subquery to push PHVs, whereas to deparse
ConvertRowtypeExpr on the nullable side we can use the same strategy
as whole-row expression deparsing.

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

Re: spelling of enable_partition_wise_join

[Ashutosh Bapat]

On Wed, Feb 14, 2018 at 2:15 AM, Alvaro Herrera  wrote:
> Peter Eisentraut wrote:
>> I wonder how others feel about this, but the spelling of
>> enable_partition_wise_join feels funny to me every time I look at it.  I
>> would write it enable_partitionwise_join.
>
> +1
>
> See also: https://postgr.es/m/20171005134847.shzldz2ublrb3ny2@alvherre.pgsql

To that I replied with
https://www.postgresql.org/message-id/CAFjFpRcsZnxCen88a-16R5EYqPCwFYnFThM%2Bmjagu%3DB1QvxPVA%40mail.gmail.com.
I didn't get any further response, so nothing was changed that time.
Alvaro, Peter, Gavin have voted for partitionwise in this thread and
Robert had similar objections earlier. Looks like we should change it.

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