Re: [HACKERS] proposal: schema variables

[Pavel Stehule]

2018-08-22 9:00 GMT+02:00 Fabien COELHO :

>
> Hello Pavel,
>
> AFAICR, I had an objection on such new objects when you first proposed
>>> something similar in October 2016.
>>>
>>> Namely, if session variables are not transactional, they cannot be used
>>> to
>>> implement security related auditing features which were advertised as the
>>> motivating use case: an the audit check may fail on a commit because of a
>>> differed constraint, but the variable would keep its "okay" value unduly,
>>> which would create a latent security issue, the audit check having failed
>>> but the variable saying the opposite.
>>>
>>> So my point was that they should be transactional by default, although I
>>> would be ok with an option for having a voluntary non transactional
>>> version.
>>>
>>> Is this issue addressed somehow with this ?
>>>
>>
>>
>> 1. I respect your opinion, but I dont agree with it. Oracle, db2 has
>> similar or very similar feature non transactional, and I didnt find any
>> requests to change it.
>>
>
> The argument of authority that "X does it like that" is not a valid answer
> to my technical objection about security implications of this feature.
>
> 2. the prototype implementation was based on relclass items, and some
>> transactional behave was possible. Peter E. had objections to this design
>> and proposed own catalog table. I did it. Now, the transactional behave is
>> harder to implement, although it is not impossible. This patch is not
>> small
>> now, so I didnt implement it.
>>
>
> "It is harder to implement" does not look like a valid answer either.
>
> I have a strong opinion so default behave have to be non transactional.
>>
>
> The fact that you have a "strong opinion" does not really answer my
> objection. Moreover, I said that I would be ok with a non transactional
> option, provided that a default transactional is available.
>
> Transactional variables significantly increases complexity of this patch,
>> now is simple, because we can reset variable on drop variable command.
>> Maybe I miss some simply implementation, but I spent on it more than few
>> days. Still, any cooperation are welcome.
>>
>
> "It is simpler to implement this way" is not an answer either, especially
> as you said that it could have been on point 2.
>
>
> As I do not see any clear answer to my objection about security
> implications, I understand that it is not addressed by this patch.
>
>
> At the bare minimum, if this feature ever made it as is, I think that a
> clear caveat must be included in the documentation about not using it for
> any security-related purpose.
>
> Also, I'm not really sure how useful such a non-transactional object can
> be for other purposes: the user should take into account that the
> transaction may fail and the value of the session variable be inconsistent
> as a result. Sometimes it may not matter, but if it matters there is no
> easy way around the fact.
>

I agree, so it should be well documented to be clear, what is possible,
what not, and to be correct expectations.

This feature has two (three) purposes

1. global variables for PL language
2. holding some session based informations, that can be used in security
definer functions.
3. Because it is not transactional, then it allows write operation on read
only hot stand by instances.

It is not transactional safe, but it is secure in sense a possibility to
set a access rights. I understand, so some patterns are not possible, but
when you need hold some keys per session, then this simply solution can be
good enough. The variables are clean after session end.

I think it is possible for some more complex patterns, but then developer
should be smarter, and should to enocode state result to content of
variable. There is strong benefit - read write access to variables is very
cheap and fast.

I invite any patch to doc (or everywhere) with explanation and about
possible risks.

Regards

Pavel





> --
> Fabien.
>

Re: plan_cache_mode and postgresql.conf.sample

[Michael Paquier]

On Wed, Aug 22, 2018 at 03:41:26PM +0900, Michael Paquier wrote:
> This line now spawns at 87 characters for me, so that's not quite it.  I
> think that you should just have put the list into two lines.

Thanks Thomas for af63926, this addresses the issue.
--
Michael


signature.asc
Description: PGP signature

Re: Improve behavior of concurrent ANALYZE/VACUUM

[Michael Paquier]

On Wed, Aug 22, 2018 at 03:49:16PM +, Bossart, Nathan wrote:
> I think so, since this is the only ownership checks we do on
> individual partitions.  Another simple way to test this would be to
> create a partitioned table with a different owner than the partitions
> and to run VACUUM as the partitioned table owner.  In this case, we
> would still rely on the checks in vacuum_rel() and analyze_rel().  IMO
> this is a reason to avoid skipping gathering the individual partitions
> based upon the ownership of the partitioned table.  It's true that
> this wouldn't fix the locking issue for partitions, but the
> aforementioned edge case is still present with 0002 anyway.  Plus, it
> would add a bit more consistency to partition handling in VACUUM.

Normal regression tests are less costly than isolation tests, so let's
use them as possible.  What you attached is covering only a portion of
all the scenarios though, as it is as well interesting to see what
happens if another user owns only the partitioned table, only one
partition, and the partitioned as well as at least one partition.  I
have extended your patch as attached.  It applies on top of HEAD.  Once
applied with the other patch one can easily stop the difference in
behavior, and this stresses the ownership checks in vacuum_rel() and
analyze_rel() as well.  Perhaps we could begin by that?

> We should probably return false here.

Oh, my compiler complained here as well.  Fixed it on my branch.
--
Michael
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c9be71ef60..57e451738c 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -124,6 +124,9 @@ DROP TABLE vactst;
 DROP TABLE vacparted;
 -- relation ownership, WARNING logs generated as all are skipped.
 CREATE TABLE vacowned (a int);
+CREATE TABLE vacowned_parted (a int) PARTITION BY LIST (a);
+CREATE TABLE vacowned_part1 PARTITION OF vacowned_parted FOR VALUES IN (1);
+CREATE TABLE vacowned_part2 PARTITION OF vacowned_parted FOR VALUES IN (2);
 CREATE ROLE regress_vacuum;
 SET ROLE regress_vacuum;
 -- Simple table
@@ -147,6 +150,102 @@ ANALYZE pg_catalog.pg_authid;
 WARNING:  skipping "pg_authid" --- only superuser can analyze it
 VACUUM (ANALYZE) pg_catalog.pg_authid;
 WARNING:  skipping "pg_authid" --- only superuser can vacuum it
+-- partitioned table and its partitions, no ownership.
+-- Relations are not listed in a single command to test ownership
+-- independently.
+VACUUM vacowned_parted;
+WARNING:  skipping "vacowned_parted" --- only table or database owner can vacuum it
+WARNING:  skipping "vacowned_part1" --- only table or database owner can vacuum it
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+VACUUM vacowned_part1;
+WARNING:  skipping "vacowned_part1" --- only table or database owner can vacuum it
+VACUUM vacowned_part2;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+ANALYZE vacowned_parted;
+WARNING:  skipping "vacowned_parted" --- only table or database owner can analyze it
+WARNING:  skipping "vacowned_part1" --- only table or database owner can analyze it
+WARNING:  skipping "vacowned_part2" --- only table or database owner can analyze it
+ANALYZE vacowned_part1;
+WARNING:  skipping "vacowned_part1" --- only table or database owner can analyze it
+ANALYZE vacowned_part2;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can analyze it
+VACUUM (ANALYZE) vacowned_parted;
+WARNING:  skipping "vacowned_parted" --- only table or database owner can vacuum it
+WARNING:  skipping "vacowned_part1" --- only table or database owner can vacuum it
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+VACUUM (ANALYZE) vacowned_part1;
+WARNING:  skipping "vacowned_part1" --- only table or database owner can vacuum it
+VACUUM (ANALYZE) vacowned_part2;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+RESET ROLE;
+-- Partitioned table and one partition owned by other user.
+ALTER TABLE vacowned_parted OWNER TO regress_vacuum;
+ALTER TABLE vacowned_part1 OWNER TO regress_vacuum;
+SET ROLE regress_vacuum;
+VACUUM vacowned_parted;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+VACUUM vacowned_part1;
+VACUUM vacowned_part2;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+ANALYZE vacowned_parted;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can analyze it
+ANALYZE vacowned_part1;
+ANALYZE vacowned_part2;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can analyze it
+VACUUM (ANALYZE) vacowned_parted;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it
+VACUUM (ANALYZE) vacowned_part1;
+VACUUM (ANALYZE) vacowned_part2;
+WARNING:  skipping "vacowned_part2" --- only table or database owner can vacuum it

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Andres Freund]

On 2018-08-22 17:09:05 -0700, Andres Freund wrote:
> Attached is a version doing so.

Mildly updated version attached. This adds an explanatory commit
message, removes one stray docs C89 reference, and fixes a mis-squashing
of a patch.

Greetings,

Andres Freund
>From 4b63f3307180a778018439ad3dee9a89aa4f4352 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 22 Aug 2018 15:10:23 -0700
Subject: [PATCH v2 1/4] Require C99 (and thus MSCV 2013 upwards).

In 86d78ef50e01 I enabled configure to check for C99 support, with the
goal of checking which platforms support C99.  While there are a few
machines without C99 support, de-supporting them for v12 was deemed
acceptable.

While not tested in aforementioned commit, the biggest increase in
minimum version comes from MSVC, which gained C99 support fairly
late. The subset in MSVC 2013 is sufficient for our needs, at this
point.  While that is a significant increase in minimum version, the
existing windows binaries are already built with a new enough version.

Make configure error out if C99 support could not be detected. For
MSVC builds, increase the minimum version to 2013.

The increase to MSVC 2013 allows us to get rid of VCBuildProject.pm,
as that was only required for MSVC 2005/2008.

Author: Andres Freund
Discussion: https://postgr.es/m/97d4b165-192d-3605-749c-f614a0c4e...@2ndquadrant.com
---
 configure |  49 +
 configure.in  |  10 +
 doc/src/sgml/install-windows.sgml |  26 +--
 doc/src/sgml/installation.sgml|   2 +-
 doc/src/sgml/sources.sgml |  33 ++--
 src/tools/msvc/MSBuildProject.pm  |  78 +---
 src/tools/msvc/README |  18 +-
 src/tools/msvc/Solution.pm| 102 --
 src/tools/msvc/VCBuildProject.pm  | 309 --
 src/tools/msvc/VSObjectFactory.pm |  37 +---
 src/tools/msvc/build.pl   |   6 +-
 11 files changed, 112 insertions(+), 558 deletions(-)
 delete mode 100644 src/tools/msvc/VCBuildProject.pm

diff --git a/configure b/configure
index 836d68dad37..dd439ddd2f6 100755
--- a/configure
+++ b/configure
@@ -4602,6 +4602,13 @@ if test "x$ac_cv_prog_cc_c99" != xno; then :
 fi
 
 
+
+# Error out if the compiler does not support C99, as the codebase
+# relies on that.
+if test "$ac_cv_prog_cc_c99" = no; then
+as_fn_error $? "C compiler \"$CC\" does not support C99" "$LINENO" 5
+fi
+
 ac_ext=cpp
 ac_cpp='$CXXCPP $CPPFLAGS'
 ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
@@ -5361,6 +5368,48 @@ fi
 
 
   # -Wdeclaration-after-statement isn't applicable for C++
+  # Really don't want VLAs to be used in our dialect of C
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Werror=vla, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Werror=vla, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Werror_vla+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Werror=vla"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__Werror_vla=yes
+else
+  pgac_cv_prog_CC_cflags__Werror_vla=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Werror_vla" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Werror_vla" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Werror_vla" = x"yes"; then
+  CFLAGS="${CFLAGS} -Werror=vla"
+fi
+
+
+  # -Wvla is not applicable for C++
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wendif-labels, for CFLAGS" >&5
 $as_echo_n "checking whether ${CC} supports -Wendif-labels, for CFLAGS... " >&6; }
diff --git a/configure.in b/configure.in
index 6e141064e5c..5869ab7c5bc 100644
--- a/configure.in
+++ b/configure.in
@@ -359,6 +359,13 @@ esac
 
 AC_PROG_CC([$pgac_cc_list])
 AC_PROG_CC_C99()
+
+# Error out if the compiler does not support C99, as the codebase
+# relies on that.
+if test "$ac_cv_prog_cc_c99" = no; then
+AC_MSG_ERROR([C compiler "$CC" does not support C99])
+fi
+
 AC_PROG_CXX([$pgac_cxx_list])
 
 # Check if it's Intel's compiler, which (usually) pretends to be gcc,
@@ -477,6 +484,9 @@ if test "$GCC" = yes -a "$ICC" = no; then
   # These work in some but not all gcc versions
   PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement])
   # -Wdeclaration-after-statement isn't applicable for C++
+  # Really don't want VLAs to be used in our dialect of C
+  PGAC_PROG_CC_CFLAGS_OPT([-Werror=vla])
+  # -Wvla is not applicable for C++
   PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
   PGAC_PROG_CXX_CFLAGS_OPT([-Wendif-labels])
   PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-format-attribute])
diff --git 

Re: Query is over 2x slower with jit=on

[Jonathan S. Katz]

> On Aug 22, 2018, at 7:13 PM, Andres Freund  wrote:
> 
> On 2018-08-22 18:29:58 -0400, Jonathan S. Katz wrote:
>> 
>>> On Aug 22, 2018, at 2:58 PM, Andreas Joseph Krogh  
>>> wrote:
>>> 
>>> På onsdag 22. august 2018 kl. 20:52:05, skrev Andres Freund 
>>> mailto:and...@anarazel.de>>:
>>> On 2018-08-22 19:51:12 +0200, Andreas Joseph Krogh wrote:
 I thought JITing of prepared queries happended once (in "prepare")
>>> 
>>> No, it happens when the first JITed function is executed.
>>> 
>>> 
 so it didn't have to do the JITing every time the query is
 executed. Isn't the previously generated bytecode usable for
 subsequent queries?
>>> 
>>> No, not currently. There's some reasons preventing that (primarily that
>>> we currently rely on addresses of certain things not to change during
>>> execution). There's ongoing work to change that, but that's certainly
>>> not going to be ready for v11.
>>> 
>>> Greetings,
>>> 
>>> Andres Freund
>>> 
>>> 
>>> Ok, thanks for clarifying.
>> 
>> Per earlier note[1] I was able to reproduce this issue with similar results 
>> to
>> Andreas while running 11 Beta 3.
>> 
>> jit = on
>> https://explain.depesz.com/s/vgzD 
>> 
>> Planning Time: 0.921 ms
>> JIT:
>>  Functions: 193
>>  Generation Time: 121.595 ms
>>  Inlining: false
>>  Inlining Time: 0.000 ms
>>  Optimization: false
>>  Optimization Time: 58.045 ms
>>  Emission Time: 1201.100 ms
>> Execution Time: 1628.017 ms
>> 
>> jit = off
>> https://explain.depesz.com/s/AvZM 
>> 
>> Planning Time: 1.398 ms
>> Execution Time: 256.473 ms
>> 
>> I increased the the search range I used in the query by 3x, and got these 
>> numbers:
>> 
>> jit=on
>> Planning Time: 0.931 ms
>> JIT:
>>  Functions: 184
>>  Generation Time: 126.587 ms
>>  Inlining: true
>>  Inlining Time: 98.865 ms
>>  Optimization: true
>>  Optimization Time: 20518.982 ms
>>  Emission Time: 7270.963 ms
>> Execution Time: 28772.576 ms
>> 
>> jit=off
>> Planning Time: 1.527 ms
>> Execution Time: 959.160 ms
> 
> For the archives sake: This likely largely is the consequence of
> building with LLVM's expensive assertions enabled, as confirmed by
> Jonathan over IM.

I recompiled with the release version of LLVM. jit=on was still slower,
but the discrepancy was not as bad as the previously reported result:

jit = off
Planning Time: 0.938 ms
Execution Time: 935.599 ms

jit = on
Planning Time: 0.951 ms
JIT:
  Functions: 184
  Generation Time: 17.605 ms
  Inlining: true
  Inlining Time: 20.522 ms
  Optimization: true
  Optimization Time: 1001.034 ms
  Emission Time: 665.319 ms
Execution Time: 2491.560 ms

However, it was still 2x+ slower, so still +1ing for open items.

Jonathan


signature.asc
Description: Message signed with OpenPGP

Re: Speeding up INSERTs and UPDATEs to partitioned tables

[Amit Langote]

On 2018/08/22 21:30, David Rowley wrote:
> On 22 August 2018 at 19:08, Amit Langote  
> wrote:
>> +#define PartitionTupRoutingGetToParentMap(p, i) \
>> +#define PartitionTupRoutingGetToChildMap(p, i) \
>>
>> If the "Get" could be replaced by "Child" and "Parent", respectively,
>> they'd sound more meaningful, imho.
> 
> I did that to save 3 chars.  I think putting the additional
> Child/Parent in the name is not really required. It's not as if we're
> going to have a ParentToParent or a ChildToChild map, so I thought it
> might be okay to assume that if it's "ToParent", that it's being
> converted from the child and "ToChild" seems safe to assume it's being
> converted from the parent. I can change it though if you feel very
> strongly that what I've got is no good.

No strong preference as such. Maybe, let's defer to committer.

>> I've looked at v6 and spotted some minor typos.
>>
>> + * ResultRelInfo for, before we go making one, we check for a
>> pre-made one
>>
>> s/making/make/g
> 
> I disagree, but perhaps we can just reword it a bit. I've now got:
> 
> + * Every time a tuple is routed to a partition that we've yet to set the
> + * ResultRelInfo for, before we go to the trouble of making one, we check
> + * for a pre-made one in the hash table.

Sure.  I guess "to the trouble of" was missing then. :)

>> +/* If nobody else set the per-subplan array of maps, do so ouselves. */
>>
>> I guess I'm the one to blame here for misspelling "ourselves".
> 
> Thanks for noticing.
> 
>> Since the above two are minor issues, fixed them myself in the attached
>> updated version; didn't touch the macro though.
> 
> I've attached a v8. The only change from your v7 is in the "go making" 
> comment.

Thanks.

>> Do you agree to setting this patch to "Ready for Committer" in the
>> September CF?
> 
> I read through the entire patch a couple of times yesterday and saw
> nothing else, so yeah, I think now is a good time for someone with
> more authority to have a look at it.

Okay, doing it now.

Thanks,
Amit

Re: Removing useless DISTINCT clauses

[David Rowley]

On 23 August 2018 at 08:11, Jim Finnerty  wrote:
> Here is an update to this thread, for potential inclusion in v12.  I
> couldn't get the most recent 'v5' patch to apply cleanly, so I recreated a
> v6 patch on PG10.5 by hand, and made a few changes and improvements:

Well, the patch I wrote was never intended for v10. Why did you go to
the effort of doing that when you're proposing the patch for v12? It
seems my v5 patch and Tom's v6 version apply fine to today's master
without any rejected hunks.

> The pg_rewrite catalog contains a serialized representation of the Query
> node in its ev_action column.  If there is a way to recreate the contents of
> the pg_rewrite relation without bumping the catversion, can someone please
> explain how?  If not, then this change is incomplete and would require a new
> catalog version (catversion.h) too.

If you're proposing for v12, why do you care about that? catversion
changes are commonplace during major version development.  Or are you
proposing this gets committed to PostgreSQL 10? ... That's not going
to happen... Plus, it does not seem well aligned with the intent of
this thread to discuss ways to do that either.

> Additional work on this patch would be desirable.  It should check for
> unique + not null, in addition to just the pk constraint.  The DISTINCT
> could be eliminated in cases with multiple relations if all the joins are
> 1:1, although that would arguably belong in a different patch.

Certainly. Patches are welcome, but I think I've mentioned to you
previously that I'm not planning that for this patch.

> p.s. the v6 patch works for the problem case that Tom Lane reported with the
> v5 patch

That would be less ambiguous if there wasn't now two v6 version of the patch.

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

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Andres Freund]

Hi,

On 2018-08-22 20:16:24 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > There's a few further potential cleanups due to relying on c99:
> > - Use __func__ unconditionally, rather than having configure test for it
> > - Use inline unconditionally, rather than having configure test for it
> > - Remove tests for AC_TYPE_INTPTR_T, AC_TYPE_UINTPTR_T,
> >   AC_TYPE_LONG_LONG_INT, we can rely on them being present.
> > - probably more in that vein
> 
> I wouldn't be in too much of a hurry to do that, particularly not the
> third item.  You are confusing "compiler is c99" with "system headers
> are c99".  Moreover, I don't see that we're buying much with such
> changes.

Yea, I am not in much of a hurry on any of them.  I think the only
argument for them is that it'd buy us a littlebit of a reduction in
configure runtime...

Greetings,

Andres Freund

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Tom Lane]

Andres Freund  writes:
> There's a few further potential cleanups due to relying on c99:
> - Use __func__ unconditionally, rather than having configure test for it
> - Use inline unconditionally, rather than having configure test for it
> - Remove tests for AC_TYPE_INTPTR_T, AC_TYPE_UINTPTR_T,
>   AC_TYPE_LONG_LONG_INT, we can rely on them being present.
> - probably more in that vein

I wouldn't be in too much of a hurry to do that, particularly not the
third item.  You are confusing "compiler is c99" with "system headers
are c99".  Moreover, I don't see that we're buying much with such
changes.

regards, tom lane

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Andres Freund]

Hi,

On 2018-08-22 05:02:11 -0700, Andres Freund wrote:
> If we agree on that, I'm going to propose a patch that includes:
> - relevant cleanups to configure
> - adapts sources.sgml to refer to C99 instead of C89
> - add some trivial conversions to for(int i;;) and struct initializers,
>   so the relevant old animals fail
> - adds a configure check to enable errors with vla usage (-Werror=vla)
>
> Questions:
>
> - do we want to make declarations at arbitrary points errors? It's
>   already a warning currently.
> - other new restrictions that we want to introduce at the same time?

Attached is a version doing so. Turns out ripping < msvc 2010 support
allows us to get rid of a fair bit of code.  Using appveyor I tested
that I didn't bungle things too badly.  But I don't really know what I'm
doing in that area, Andrew or Craig, your input would be appreciated.

I tried to update sources.sgml to reflect my understanding of the subset
of C99 we're going to use.  I'd rather start more restrictive and then
argue about relaxing things separately.


There's a few further potential cleanups due to relying on c99:
- Use __func__ unconditionally, rather than having configure test for it
- Use inline unconditionally, rather than having configure test for it
- Remove tests for AC_TYPE_INTPTR_T, AC_TYPE_UINTPTR_T,
  AC_TYPE_LONG_LONG_INT, we can rely on them being present.
- probably more in that vein


I'd rather do these separately lateron, in case one of them causes
trouble on some animals.


There's some potential ones I think we should *not* do:
- remove AC_C_FLEXIBLE_ARRAY_MEMBER, and rely on it unconditionally.
  I'm disinclined to do this, because C++ IIRC doesn't support it in any
  version, and I don't want to make Peter's life unnecessarily hard.
- remove AC_C_RESTRICT check, rely on it unconditionally: MSVC still
  spells this differently.


Greetings,

Andres Freund
>From c579be2ae7a020d6005d84a5b6f24872ba8b1b05 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 22 Aug 2018 15:10:23 -0700
Subject: [PATCH v1 1/4] Require C99 (and thus MSCV 2013 upwards).

Author: Andres Freund
Discussion: https://postgr.es/m/97d4b165-192d-3605-749c-f614a0c4e...@2ndquadrant.com
---
 configure| 49 
 configure.in | 10 +++
 doc/src/sgml/sources.sgml| 33 ++---
 src/tools/msvc/MSBuildProject.pm |  2 +-
 src/tools/msvc/README|  6 ++--
 src/tools/msvc/build.pl  |  6 +---
 6 files changed, 86 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index 836d68dad37..dd439ddd2f6 100755
--- a/configure
+++ b/configure
@@ -4602,6 +4602,13 @@ if test "x$ac_cv_prog_cc_c99" != xno; then :
 fi
 
 
+
+# Error out if the compiler does not support C99, as the codebase
+# relies on that.
+if test "$ac_cv_prog_cc_c99" = no; then
+as_fn_error $? "C compiler \"$CC\" does not support C99" "$LINENO" 5
+fi
+
 ac_ext=cpp
 ac_cpp='$CXXCPP $CPPFLAGS'
 ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
@@ -5361,6 +5368,48 @@ fi
 
 
   # -Wdeclaration-after-statement isn't applicable for C++
+  # Really don't want VLAs to be used in our dialect of C
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Werror=vla, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Werror=vla, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Werror_vla+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Werror=vla"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__Werror_vla=yes
+else
+  pgac_cv_prog_CC_cflags__Werror_vla=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Werror_vla" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Werror_vla" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Werror_vla" = x"yes"; then
+  CFLAGS="${CFLAGS} -Werror=vla"
+fi
+
+
+  # -Wvla is not applicable for C++
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wendif-labels, for CFLAGS" >&5
 $as_echo_n "checking whether ${CC} supports -Wendif-labels, for CFLAGS... " >&6; }
diff --git a/configure.in b/configure.in
index 6e141064e5c..5869ab7c5bc 100644
--- a/configure.in
+++ b/configure.in
@@ -359,6 +359,13 @@ esac
 
 AC_PROG_CC([$pgac_cc_list])
 AC_PROG_CC_C99()
+
+# Error out if the compiler does not support C99, as the codebase
+# relies on that.
+if test "$ac_cv_prog_cc_c99" = no; then
+AC_MSG_ERROR([C compiler "$CC" does not support C99])
+fi
+
 AC_PROG_CXX([$pgac_cxx_list])
 
 # Check if it's Intel's compiler, which 

Re: SPI_cursor_fetch Memory overrun

[Tom Lane]

Wu Ivy  writes:
>  fprintf(fp, (i == tupdesc->natts) ? "%s\n" : "%s," , 
> SPI_getvalue(tuple, tupdesc, i)); 

That (specifically the SPI_getvalue call) is what's leaking memory.
You could improve matters by pfree'ing the result string after each
such call.

Just to add insult to injury, it's also looking up the column datatype's
output function afresh on every call.  There could perhaps be some leakage
involved in those lookups too, though I'd bet the main problem is the
result strings.

regards, tom lane

Re: Query is over 2x slower with jit=on

[Andres Freund]

On 2018-08-22 18:29:58 -0400, Jonathan S. Katz wrote:
> 
> > On Aug 22, 2018, at 2:58 PM, Andreas Joseph Krogh  
> > wrote:
> > 
> > På onsdag 22. august 2018 kl. 20:52:05, skrev Andres Freund 
> > mailto:and...@anarazel.de>>:
> > On 2018-08-22 19:51:12 +0200, Andreas Joseph Krogh wrote:
> > > I thought JITing of prepared queries happended once (in "prepare")
> > 
> > No, it happens when the first JITed function is executed.
> > 
> > 
> > >  so it didn't have to do the JITing every time the query is
> > > executed. Isn't the previously generated bytecode usable for
> > > subsequent queries?
> > 
> > No, not currently. There's some reasons preventing that (primarily that
> > we currently rely on addresses of certain things not to change during
> > execution). There's ongoing work to change that, but that's certainly
> > not going to be ready for v11.
> > 
> > Greetings,
> > 
> > Andres Freund
> > 
> > 
> > Ok, thanks for clarifying.
> 
> Per earlier note[1] I was able to reproduce this issue with similar results to
> Andreas while running 11 Beta 3.
> 
> jit = on
> https://explain.depesz.com/s/vgzD 
> 
> Planning Time: 0.921 ms
> JIT:
>   Functions: 193
>   Generation Time: 121.595 ms
>   Inlining: false
>   Inlining Time: 0.000 ms
>   Optimization: false
>   Optimization Time: 58.045 ms
>   Emission Time: 1201.100 ms
> Execution Time: 1628.017 ms
> 
> jit = off
> https://explain.depesz.com/s/AvZM 
> 
> Planning Time: 1.398 ms
> Execution Time: 256.473 ms
> 
> I increased the the search range I used in the query by 3x, and got these 
> numbers:
> 
> jit=on
> Planning Time: 0.931 ms
> JIT:
>   Functions: 184
>   Generation Time: 126.587 ms
>   Inlining: true
>   Inlining Time: 98.865 ms
>   Optimization: true
>   Optimization Time: 20518.982 ms
>   Emission Time: 7270.963 ms
> Execution Time: 28772.576 ms
> 
> jit=off
> Planning Time: 1.527 ms
> Execution Time: 959.160 ms

For the archives sake: This likely largely is the consequence of
building with LLVM's expensive assertions enabled, as confirmed by
Jonathan over IM.

Greetings,

Andres Freund

SPI_cursor_fetch Memory overrun

[Wu Ivy]

Hi all,

I’m creating a C extension to write data from the Postgres  to a text file . 
Since the table can be very large, I only want to process one chunk of data per 
time to keep the memory stable. To achieve this, I use SPI  
cursor(https://www.postgresql.org/docs/current/static/spi-spi-cursor-fetch.html 
) to 
fetch row chunks from the table. Following is my program:

char* command;
uint64 proc = 1;

// Prepare a statement without executing it yet
SPI_connect();
command = psprintf("SELECT * FROM %s", tableName);
SPIPlanPtr SPIplan = SPI_prepare_cursor(command, 0, NULL, 0);
pfree(command);

// Set up a cursor using a statement created with SPI_prepare_cursor
Portal cursor= SPI_cursor_open(NULL, SPIplan, NULL, NULL, true);
if (SPIplan == NULL){
elog(ERROR,"couldn't create cursor via SPI");
}

// Writing to local Desktop
clock_t begin = clock();
int count = 0;
FILE *fp = fopen("/home/ubuntu/test.txt", "w");
if(fp == NULL){
return psprintf("Error when open file");
}

while(proc > 0){
// Fetch 20 rows from a cursor
SPI_cursor_fetch(cursor, true, 20);
proc = SPI_processed; // number of row returned

if (proc != 0){
TupleDesc tupdesc = SPI_tuptable->tupdesc;
uint64 j;

for (j = 0; j < proc; j++){
HeapTuple tuple = tuptable->vals[j];
int i;
count++;
for (i = 1; i <= tupdesc->natts; i++){ //natts -> number of 
columns
 fprintf(fp, (i == tupdesc->natts) ? "%s\n" : "%s," , 
SPI_getvalue(tuple, tupdesc, i)); 
}
}
}else{
break;
}

SPI_freetuptable(SPI_tuptable);
elog(INFO, “freed tuptable”);
sleep(5);
}


From my understanding, cursor points at the entire result rows on heap. After 
fetching certain number of rows, SPI_tuptable points to the allocated row set. 
After finishing writing the row set to text file,  I freed the memory of it by 
calling SPI_freetuptable( ) before next fetch.  I expected the memory to stay 
around a constant value through out the program, However, the actual memory 
kept increasing when I run the program.  Also, I only observed memory drop 
after SPI_freetuptable() of first chunk. After that memory kept going up even  
SPI_freetuptable()  is executed.  Any clue of why is it happening?  

Thanks in advance!

Best,
Ivy

Re: JIT compiling with LLVM v12

[Andres Freund]

Hi,

On 2018-08-22 18:15:29 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-08-22 06:20:21 +, Noah Misch wrote:
> >> Regardless of the choice of jit={on|off} default, these numbers tell me 
> >> that
> >> some or all of jit_*_cost defaults are too low.
>
> > I don't think it really shows that. The reason that JITing gets started
> > there is that the tables aren't analyzed and we end up with crazy ass
> > estimates about the cost of the queries. No useful setting of the cost
> > limits will protect against that... :(
>
> I don't buy that line of argument one bit.  No, we generally don't
> analyze most of the regression test tables, but the planner still
> knows that they're not very large.  If JIT is kicking in for those
> queries, the defaults are set wrong.

I looked at the queries that get JITed, I didn't just make that claim up
out of thin air. The first query that's JITed e.g. is:

+explain analyze SELECT '' AS tf_12, BOOLTBL1.*, BOOLTBL2.*
+   FROM BOOLTBL1, BOOLTBL2
+   WHERE BOOLTBL2.f1 <> BOOLTBL1.f1;
+QUERY PLAN
+--
+ Nested Loop  (cost=0.00..118524.73 rows=3948050 width=34) (actual 
time=8.376..8.390 rows=12 loops=1)
+   Join Filter: (booltbl2.f1 <> booltbl1.f1)
+   Rows Removed by Join Filter: 4
+   ->  Seq Scan on booltbl1  (cost=0.00..38.10 rows=2810 width=1) (actual 
time=0.018..0.019 rows=4 loops=1)
+   ->  Materialize  (cost=0.00..52.15 rows=2810 width=1) (actual 
time=0.004..0.005 rows=4 loops=4)
+ ->  Seq Scan on booltbl2  (cost=0.00..38.10 rows=2810 width=1) 
(actual time=0.007..0.009 rows=4 loops=1)
+ Planning Time: 0.074 ms
+ JIT:
+   Functions: 6
+   Generation Time: 0.935 ms
+   Inlining: false
+   Inlining Time: 0.000 ms
+   Optimization: false
+   Optimization Time: 0.451 ms
+   Emission Time: 7.716 ms
+ Execution Time: 43.466 ms
+(16 rows)

Now you can say that'd be solved by bumping the cost up, sure. But
obviously the row / cost model is pretty much out of whack here, I don't
see how we can make reasonable decisions in a trivial query that has a
misestimation by five orders of magnitude.

Another subsequent case is:
 set enable_sort = off;  -- try to make it pick a hash setop implementation
 select '(2,5)'::cashrange except select '(5,6)'::cashrange;
which is expensive because a sort is chosen even though sort is disabled
(yes, this might be a bug in the test):
 EXPLAIN  select '(2,5)'::cashrange except select '(5,6)'::cashrange;
┌┐
│ QUERY PLAN
 │
├┤
│ SetOp Except  (cost=100.06..100.07 rows=1 width=36)   
 │
│   ->  Sort  (cost=100.06..100.06 rows=2 width=36) 
 │
│ Sort Key: ('($2.00,$5.00)'::cashrange)
 │
│ ->  Append  (cost=0.00..0.05 rows=2 width=36) 
 │
│   ->  Subquery Scan on "*SELECT* 1"  (cost=0.00..0.02 rows=1 
width=36) │
│ ->  Result  (cost=0.00..0.01 rows=1 width=32) 
 │
│   ->  Subquery Scan on "*SELECT* 2"  (cost=0.00..0.02 rows=1 
width=36) │
│ ->  Result  (cost=0.00..0.01 rows=1 width=32) 
 │
│ JIT:  
 │
│   Functions: 7
 │
│   Inlining: true  
 │
│   Optimization: true  
 │
└┘
(12 rows)

Obviously the high costing here distorts things.  Many of the other
cases here are along similar lines as the two cases before.


> Additional evidence for the
> defaults being wrong is the number of reports we've had of JIT making
> things slower.

Maybe.

Greetings,

Andres Freund

Re: Query is over 2x slower with jit=on

[Jonathan S. Katz]

> On Aug 22, 2018, at 2:58 PM, Andreas Joseph Krogh  wrote:
> 
> På onsdag 22. august 2018 kl. 20:52:05, skrev Andres Freund 
> mailto:and...@anarazel.de>>:
> On 2018-08-22 19:51:12 +0200, Andreas Joseph Krogh wrote:
> > I thought JITing of prepared queries happended once (in "prepare")
> 
> No, it happens when the first JITed function is executed.
> 
> 
> >  so it didn't have to do the JITing every time the query is
> > executed. Isn't the previously generated bytecode usable for
> > subsequent queries?
> 
> No, not currently. There's some reasons preventing that (primarily that
> we currently rely on addresses of certain things not to change during
> execution). There's ongoing work to change that, but that's certainly
> not going to be ready for v11.
> 
> Greetings,
> 
> Andres Freund
> 
> 
> Ok, thanks for clarifying.

Per earlier note[1] I was able to reproduce this issue with similar results to
Andreas while running 11 Beta 3.

jit = on
https://explain.depesz.com/s/vgzD 

Planning Time: 0.921 ms
JIT:
  Functions: 193
  Generation Time: 121.595 ms
  Inlining: false
  Inlining Time: 0.000 ms
  Optimization: false
  Optimization Time: 58.045 ms
  Emission Time: 1201.100 ms
Execution Time: 1628.017 ms

jit = off
https://explain.depesz.com/s/AvZM 

Planning Time: 1.398 ms
Execution Time: 256.473 ms

I increased the the search range I used in the query by 3x, and got these 
numbers:

jit=on
Planning Time: 0.931 ms
JIT:
  Functions: 184
  Generation Time: 126.587 ms
  Inlining: true
  Inlining Time: 98.865 ms
  Optimization: true
  Optimization Time: 20518.982 ms
  Emission Time: 7270.963 ms
Execution Time: 28772.576 ms

jit=off
Planning Time: 1.527 ms
Execution Time: 959.160 ms

So, I would +1 this for open items.

Jonathan

[1] 
https://www.postgresql.org/message-id/7F838324-064B-4A24-952C-2800CFBD39D6%40postgresql.org
 



signature.asc
Description: Message signed with OpenPGP

Re: JIT compiling with LLVM v12

[Tom Lane]

Andres Freund  writes:
> On 2018-08-22 06:20:21 +, Noah Misch wrote:
>> Regardless of the choice of jit={on|off} default, these numbers tell me that
>> some or all of jit_*_cost defaults are too low.

> I don't think it really shows that. The reason that JITing gets started
> there is that the tables aren't analyzed and we end up with crazy ass
> estimates about the cost of the queries. No useful setting of the cost
> limits will protect against that... :(

I don't buy that line of argument one bit.  No, we generally don't
analyze most of the regression test tables, but the planner still
knows that they're not very large.  If JIT is kicking in for those
queries, the defaults are set wrong.  Additional evidence for the
defaults being wrong is the number of reports we've had of JIT making
things slower.

I was OK with that happening during early beta, on the grounds of getting
more testing for the JIT code; but it's time to fix the numbers.

regards, tom lane

Re: jsonpath

[Nikita Glukhov]

Attached 17th version of the patches rebased onto the current master.

Nothing significant has changed.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


sqljson_jsonpath_v17.tar.gz
Description: application/gzip

Re: csv format for psql

[Daniel Verite]

Fabien COELHO wrote:

> Doc: "according to the csv rules" -> "according to csv rules."?

Fixed.

> Doc: "RFC-4180" -> "RFC 4180"?

Fixed. The other references to RFCs use this syntax indeed.

> The previous RFC specifies CRLF as eol, but '\n' is hardcoded in the 
> source. I'm fine with that, but I'd suggest that the documentation should 
> said which EOL is used.

'\n' gets translated by libc when the output is in text mode.
We discussed this upthread, but maybe it should be a code comment:
added now.

> ISTM that "--csv" & "-C" are not documented, neither in sgml nor under
> --help.
> 
> "fieldsep_csv" does not show on the list of output options under "\?".

Oops, fixed.

> There seems to be a test in the code to set an empty string "" by default,
> but it is unclear to me when this is triggered.

Where is that code?

> I'd tend to use "CSV" instead of "csv" everywhere it makes sense, eg in 
> the doc (CSV rules) and in variable names in the code (FooCsv -> FooCSV?), 
> but that is pretty debatable.

I've changed to upper case in a couple places and added  tags,
but depending on the context sometimes lower case feels more consistent.
This is the same as, for instance, ASCII. We display \pset linestyle as
ascii, not ASCII, presumably because everything else in the \pset area
is lower case. But both cases are accepted in input.

Also added a CSV entry in the doc index per Alvaro's suggestion in
https://www.postgresql.org/message-id/20180809202125.r4mdx2jzm7hytetz@alvherre.pgsql
with pointers to psql and COPY.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 13a8b68..98147ef 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -672,6 +672,10 @@ COPY count
 
   
CSV Format
+   
+CSV
+in COPY
+   
 

 This format option is used for importing and exporting the Comma
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index eb9d93a..80cb843 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -151,7 +151,21 @@ EOF
   
 
 
-
+ 
+  --csv
+  
+  
+  
+   CSV
+   in psql
+  
+  Switches to CSV output mode. This is equivalent
+  to \pset format csv.
+  
+  
+ 
+
+ 
   -d dbname
   --dbname=dbname
   
@@ -2557,6 +2571,19 @@ lo_import 152801
   
 
   
+  fieldsep_csv
+  
+  
+  Specifies the field separator to be used in the
+  CSV format. When the separator appears in a field
+  value, that field is output inside double quotes according to
+  CSV rules. To set a tab as field separator, type
+  \pset fieldsep_csv '\t'. The default is a comma.
+  
+  
+  
+
+  
   fieldsep_zero
   
   
@@ -2584,12 +2611,11 @@ lo_import 152801
   format
   
   
-  Sets the output format to one of unaligned,
-  aligned, wrapped,
-  html, asciidoc,
+  Sets the output format to one of aligned,
+  asciidoc, csv, 
html,
   latex (uses tabular),
-  latex-longtable, or
-  troff-ms.
+  latex-longtable, troff-ms,
+  unaligned, or wrapped.
   Unique abbreviations are allowed.  (That would mean one letter
   is enough.)
   
@@ -2597,14 +2623,23 @@ lo_import 152801
   unaligned format writes all columns of a 
row on one
   line, separated by the currently active field separator. This
   is useful for creating output that might be intended to be read
-  in by other programs (for example, tab-separated or comma-separated
-  format).
+  in by other programs.
   
 
   aligned format is the standard, 
human-readable,
   nicely formatted text output;  this is the default.
   
 
+ csv format writes columns separated by
+ commas, applying the quoting rules described in RFC 4180.
+ Alternative separators can be selected with
+ \pset fieldsep_csv.
+ The output is compatible with the CSV format of the
+ COPY command. The header with column names
+ is output unless the tuples_only parameter is
+ on. Title and footers are not printed.
+ 
+
   wrapped format is like 
aligned but wraps
   wide data values across lines to make the output fit in the target
   column width.  The target width is determined as described under
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5b4d54a..3b47b38 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1941,8 +1941,8 @@ exec_command_pset(PsqlScanState scan_state, bool 
active_branch)
 
int i;

Re: Query is over 2x slower with jit=on

[Andres Freund]

On 2018-08-22 20:48:48 +0200, Pierre Ducroquet wrote:
> On Wednesday, August 22, 2018 6:51:55 PM CEST Andres Freund wrote:
> > On 2018-08-22 18:39:18 +0200, Andreas Joseph Krogh wrote:
> > > Just to be clear; The query really runs slower (wall-clock time), it's not
> > > just the timing.
> > 
> > I bet it's not actually running slower, it "just" takes longer to start
> > up due to the JITing in each worker. I suspect what we should do is to
> > multiple the cost limits by the number of workers, to model that.  But
> > without the fixed instrumentation that's harder to see...
> 
> It depends on the query. It has been shown in other threads that query can 
> indeed take longer to run because of JITing : if the cost is too low to fire 
> LLVM optimizer, the generated code can be so bad it will be slower than the 
> non-JIT executor.

This largely seems to be orthogonal to what I'm talking about.


> Cf for instance a previous discussion here : 
> http://www.postgresql-archive.org/PATCH-LLVM-tuple-deforming-improvements-td6029385.html

I'd wish people stopped using www.postgresql-archive.org. It's *NOT*
postgresql.org maintained, in fact I do not know who does. It does shows
ads when downloading links, which I'm personally not ok with.


Greetings,

Andres Freund

Re: Query is over 2x slower with jit=on

[Andreas Joseph Krogh]

På onsdag 22. august 2018 kl. 20:52:05, skrev Andres Freund mailto:and...@anarazel.de>>:
On 2018-08-22 19:51:12 +0200, Andreas Joseph Krogh wrote:
 > I thought JITing of prepared queries happended once (in "prepare")

 No, it happens when the first JITed function is executed.


 >  so it didn't have to do the JITing every time the query is
 > executed. Isn't the previously generated bytecode usable for
 > subsequent queries?

 No, not currently. There's some reasons preventing that (primarily that
 we currently rely on addresses of certain things not to change during
 execution). There's ongoing work to change that, but that's certainly
 not going to be ready for v11.

 Greetings,

 Andres Freund
 
 
Ok, thanks for clarifying.
 
--
 Andreas Joseph Krogh

Re: Query is over 2x slower with jit=on

[Andres Freund]

On 2018-08-22 19:51:12 +0200, Andreas Joseph Krogh wrote:
> I thought JITing of prepared queries happended once (in "prepare")

No, it happens when the first JITed function is executed.


>  so it didn't have to do the JITing every time the query is
> executed. Isn't the previously generated bytecode usable for
> subsequent queries?

No, not currently. There's some reasons preventing that (primarily that
we currently rely on addresses of certain things not to change during
execution). There's ongoing work to change that, but that's certainly
not going to be ready for v11.

Greetings,

Andres Freund

Re: Query is over 2x slower with jit=on

[Pierre Ducroquet]

On Wednesday, August 22, 2018 6:51:55 PM CEST Andres Freund wrote:
> On 2018-08-22 18:39:18 +0200, Andreas Joseph Krogh wrote:
> > Just to be clear; The query really runs slower (wall-clock time), it's not
> > just the timing.
> 
> I bet it's not actually running slower, it "just" takes longer to start
> up due to the JITing in each worker. I suspect what we should do is to
> multiple the cost limits by the number of workers, to model that.  But
> without the fixed instrumentation that's harder to see...

It depends on the query. It has been shown in other threads that query can 
indeed take longer to run because of JITing : if the cost is too low to fire 
LLVM optimizer, the generated code can be so bad it will be slower than the 
non-JIT executor.
Cf for instance a previous discussion here : 
http://www.postgresql-archive.org/PATCH-LLVM-tuple-deforming-improvements-td6029385.html

I think it would be interesting to try the query from this thread with a patch 
forcing the LLVM codegen to O1 (I found no PassManager there to play with, it 
seems to be an off/on/extreme switch ; patch 0001-LLVM-Use-the-O1-CodeGen-
level.patch from thread mentioned above).

Re: Stored procedures and out parameters

[Dave Cramer]

On Wed, 22 Aug 2018 at 12:58, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 22/08/2018 18:49, David G. Johnston wrote:
> > What others have done doesn't change the situation that has arisen for
> > PostgreSQL due to its implementation history.
>
> What others have done seems relevant, because the whole reason these
> questionable interfaces exist is to achieve compatibility across SQL
> implementations.  Otherwise you can just make a native SQL call directly.
>

It seems to me that if we don't make it possible to call a function or a
procedure using
the same mechanism the drivers will have to make a choice which one to
implement.
That said the path of least resistance and regression for the drivers would
be to not implement
calling procedures through each respective drivers mechanism. I would think
given the importance of
this work it would be a shame not to make it easy to use.

I also agree with David that driver writers made the best out of the
situation with functions and we are now asking for the server to dual
purpose the call command.

Is there a technical reason why this is not possible ?


Dave Cramer

da...@postgresintl.com
www.postgresintl.com

>
>

Sv: Re: Query is over 2x slower with jit=on

[Andreas Joseph Krogh]

På onsdag 22. august 2018 kl. 18:51:55, skrev Andres Freund mailto:and...@anarazel.de>>:
On 2018-08-22 18:39:18 +0200, Andreas Joseph Krogh wrote:
 > Just to be clear; The query really runs slower (wall-clock time), it's not
 > just the timing.

 I bet it's not actually running slower, it "just" takes longer to start
 up due to the JITing in each worker. I suspect what we should do is to
 multiple the cost limits by the number of workers, to model that.  But
 without the fixed instrumentation that's harder to see...
 
Well, yes, that might be. By "runs" I meant from me hitting ENTER in psql to 
the time the query finishes...
 
I thought JITing of prepared queries happended once (in "prepare") so it 
didn't have to do the JITing every time the query is executed. Isn't 
the previously generated bytecode usable for subsequent queries?
 
--
 Andreas Joseph Krogh

Re: Stored procedures and out parameters

[Vladimir Sitnikov]

Peter>AFAICT in no case does it involve allowing functions to be called as
procedures or vice versa.

Oracle DB uses the same way to execute both procedures and functions:
pl/sql block.

For instance:
procedure) begin my_proc(); end;
function) begin :result := my_fun(); end;

Call like begin my_fun(); end; would fail.
However there's no dedicated command to call procedures or a command to
call functions.

Vladimir

Re: Stored procedures and out parameters

[Peter Eisentraut]

On 22/08/2018 18:49, David G. Johnston wrote:
> What others have done doesn't change the situation that has arisen for
> PostgreSQL due to its implementation history.

What others have done seems relevant, because the whole reason these
questionable interfaces exist is to achieve compatibility across SQL
implementations.  Otherwise you can just make a native SQL call directly.

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

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[David Steele]

On 8/22/18 10:56 AM, Peter Eisentraut wrote:
> On 22/08/2018 14:02, Andres Freund wrote:
>> If we agree on that, I'm going to propose a patch that includes:
>> - relevant cleanups to configure
>> - adapts sources.sgml to refer to C99 instead of C89
>> - add some trivial conversions to for(int i;;) and struct initializers,
>>   so the relevant old animals fail
>> - adds a configure check to enable errors with vla usage (-Werror=vla)
> 
> sounds good

Sounds good to me.

> 
>> - do we want to make declarations at arbitrary points errors? It's
>>   already a warning currently.
> 
> While there are legitimate criticisms, it's a standard feature in C,
> C++, and many other languages, so I don't see what we'd gain by fighting it.

+1.=

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

Re: Query is over 2x slower with jit=on

[Andres Freund]

On 2018-08-22 18:39:18 +0200, Andreas Joseph Krogh wrote:
> Just to be clear; The query really runs slower (wall-clock time), it's not 
> just the timing.

I bet it's not actually running slower, it "just" takes longer to start
up due to the JITing in each worker. I suspect what we should do is to
multiple the cost limits by the number of workers, to model that.  But
without the fixed instrumentation that's harder to see...

Greetings,

Andres Freund

Re: Stored procedures and out parameters

[David G. Johnston]

On Wed, Aug 22, 2018 at 9:39 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 16/08/2018 19:54, Shay Rojansky wrote:
> > I don't think there's anything really Microsoft-specific about any of
> > this (except maybe in the history) - just like JDBC and psycopg, there's
> > simply a single standard way in the database API for invoking
> > server-side things, and not two ways.
>
> Have you looked what those standard interfaces do in other SQL
> implementations (e.g., Oracle, DB2, Derby, MySQL)?  AFAICT in no case
> does it involve allowing functions to be called as procedures or vice
> versa.


That is all well and good except PostgreSQL took its sweet time
implementing procedures and so effectively encouraged its community to
dual-purpose functions.  Now the community is simply asking core to
recognize that history and dual-purpose the "CALL" command.

What others have done doesn't change the situation that has arisen for
PostgreSQL due to its implementation history.

David J.

Re: Stored procedures and out parameters

[Peter Eisentraut]

On 16/08/2018 19:54, Shay Rojansky wrote:
> I don't think there's anything really Microsoft-specific about any of
> this (except maybe in the history) - just like JDBC and psycopg, there's
> simply a single standard way in the database API for invoking
> server-side things, and not two ways.

Have you looked what those standard interfaces do in other SQL
implementations (e.g., Oracle, DB2, Derby, MySQL)?  AFAICT in no case
does it involve allowing functions to be called as procedures or vice versa.

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

Sv: Re: Query is over 2x slower with jit=on

[Andreas Joseph Krogh]

På onsdag 22. august 2018 kl. 18:12:41, skrev Andres Freund mailto:and...@anarazel.de>>:
Hi,

 On 2018-04-18 18:37:30 -0400, Robert Haas wrote:
 > On Wed, Apr 18, 2018 at 3:29 PM, Andres Freund  wrote:
 > > Not convinced that that is true - the issue is more likely that JIT work 
in workers is counted as execute time... Gotta add that somehow, not sure what 
the best way would be.
 >
 > Oh, that does seem like something that should be fixed.  If that's
 > what is happening here, it's bound to confuse a lot of people.
 > Probably you need to add some code to
 > ExecParallelRetrieveInstrumentation.

 I had lost track of this, and we unfortunately hadn't added an open item
 back then.  I think we should add it now?

 RMT (with me recused), do you think we should accept the new code fixing
 this would entail? And thus that this should be an open item? It's
 arguably a new feature, although I don't find that a terribly convincing
 position.

 Greetings,

 Andres Freund
 
Just to be clear; The query really runs slower (wall-clock time), it's not 
just the timing.
 
--
 Andreas Joseph Krogh

Re: Query is over 2x slower with jit=on

[Jonathan S. Katz]

> On Aug 22, 2018, at 12:12 PM, Andres Freund  wrote:
> 
> Hi,
> 
> On 2018-04-18 18:37:30 -0400, Robert Haas wrote:
>> On Wed, Apr 18, 2018 at 3:29 PM, Andres Freund  wrote:
>>> Not convinced that that is true - the issue is more likely that JIT work in 
>>> workers is counted as execute time... Gotta add that somehow, not sure what 
>>> the best way would be.
>> 
>> Oh, that does seem like something that should be fixed.  If that's
>> what is happening here, it's bound to confuse a lot of people.
>> Probably you need to add some code to
>> ExecParallelRetrieveInstrumentation.
> 
> I had lost track of this, and we unfortunately hadn't added an open item
> back then.  I think we should add it now?
> 
> RMT (with me recused), do you think we should accept the new code fixing
> this would entail? And thus that this should be an open item? It's
> arguably a new feature, although I don't find that a terribly convincing
> position.

Reviewed the earlier discussion. I can argue this both ways.

As it stands right now it seems like we will be defaulting “jit = off” which
means there will only be so many people who will be using jit in PG11,
and as such we could provide the patch for 12.

However, for the people who do enable JIT in PG11, this is a scenario
that would cause the query to perform in an unexpected way that is no
fault of their own, which one would argue is a bug. I have not tried to
reproduce it myself, but from looking at the explain plans from Andreas,
I’m confident I could craft a moderately complex query that could
demonstrate the performance degradation with jit = on.

While adding the code may constitute being a new feature, it is a feature
that would prevent user frustration on something that we are highlighting as a
“major feature” of PG11, even if it’s not enabled by default.

What I would you Andres is how complex will the patch be and how much time
will it take? As such I would +1 it for open items right now unless it seems 
like
this could take a significant of time to come up with a proper patch, in which
case I would reconsider, but at least with it on open items we would be able
to continuously review.

Thanks,

Jonathan




signature.asc
Description: Message signed with OpenPGP

Re: JIT compiling with LLVM v12

[Peter Eisentraut]

On 22/08/2018 16:54, Andres Freund wrote:
> I don't see particularly much benefit in deciding before beta,
> personally.  What's making you think it'd be important to decide before?
> Pretty fundamentally, it'll be a setting you don't know is effectively
> on, for the forseeable future anyway?

Users are evaluating PostgreSQL 11 beta in their environments, including
its performance.  I have to tell them, whatever performance test results
you get now might not be what you'll get with the final 11.0.

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

Re: Query is over 2x slower with jit=on

[Andres Freund]

Hi,

On 2018-04-18 18:37:30 -0400, Robert Haas wrote:
> On Wed, Apr 18, 2018 at 3:29 PM, Andres Freund  wrote:
> > Not convinced that that is true - the issue is more likely that JIT work in 
> > workers is counted as execute time... Gotta add that somehow, not sure what 
> > the best way would be.
> 
> Oh, that does seem like something that should be fixed.  If that's
> what is happening here, it's bound to confuse a lot of people.
> Probably you need to add some code to
> ExecParallelRetrieveInstrumentation.

I had lost track of this, and we unfortunately hadn't added an open item
back then.  I think we should add it now?

RMT (with me recused), do you think we should accept the new code fixing
this would entail? And thus that this should be an open item? It's
arguably a new feature, although I don't find that a terribly convincing
position.

Greetings,

Andres Freund

Sv: Re: JIT compiling with LLVM v12

[Andreas Joseph Krogh]

På onsdag 22. august 2018 kl. 16:36:00, skrev Peter Eisentraut <
peter.eisentr...@2ndquadrant.com >:
On 22/08/2018 08:20, Noah Misch wrote:
 > Regardless of the choice of jit={on|off} default, these numbers tell me that
 > some or all of jit_*_cost defaults are too low.

 That was also my earlier analysis.

 I'm suspicious that we haven't had much feedback about this.  We've
 heard of one or two cases where LLVM broke a query outright, and that
 was fixed and that was a good result.  But we haven't heard anything
 about performance regressions.  Surely there must be some.  There hasn't
 been any discussion or further analysis of the default cost settings
 either.  I feel that we don't have enough information.

 Another problem is that LLVM is only enabled in some versions of
 packages.  For example, in the PGDG RPMs, it's enabled for RHEL 7 but
 not RHEL 6.  So you could be in for a surprise if you upgrade your
 operating system at some point.

 I would like, however, that we make a decision one way or the other
 before the next beta.  I've been handwaving a bit to users not to rely
 on the current betas for performance testing because the defaults might
 change later.  That's bad either way.
 
FWIW; Our largest report-queries perform worse (then v10) with 
jit=on; 
https://www.postgresql.org/message-id/VisenaEmail.24.e60072a07f006130.162d95c3e17%40tc7-visena
 
Disabling JIT makes them perform slightly better than v10.
 
--
 Andreas Joseph Krogh

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Andres Freund]

Hi,

On 2018-08-22 16:56:15 +0200, Peter Eisentraut wrote:
> On 22/08/2018 14:02, Andres Freund wrote:
> > - do we want to make declarations at arbitrary points errors? It's
> >   already a warning currently.
> 
> While there are legitimate criticisms, it's a standard feature in C,
> C++, and many other languages, so I don't see what we'd gain by fighting it.

I personally don't really care - for C there's really not much of a
difference (contrast to C++ with RAII). But Robert and Tom both said
this would be an issue with moving to C99 for them. I don't want to hold
up making progress here by fighting over this issue.

Greetings,

Andres Freund

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Peter Eisentraut]

On 22/08/2018 14:02, Andres Freund wrote:
> If we agree on that, I'm going to propose a patch that includes:
> - relevant cleanups to configure
> - adapts sources.sgml to refer to C99 instead of C89
> - add some trivial conversions to for(int i;;) and struct initializers,
>   so the relevant old animals fail
> - adds a configure check to enable errors with vla usage (-Werror=vla)

sounds good

> - do we want to make declarations at arbitrary points errors? It's
>   already a warning currently.

While there are legitimate criticisms, it's a standard feature in C,
C++, and many other languages, so I don't see what we'd gain by fighting it.

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

Re: JIT compiling with LLVM v12

[Andres Freund]

Hi,

On 2018-08-22 16:36:00 +0200, Peter Eisentraut wrote:
> I'm suspicious that we haven't had much feedback about this.  We've
> heard of one or two cases where LLVM broke a query outright, and that
> was fixed and that was a good result.  But we haven't heard anything
> about performance regressions.  Surely there must be some.  There hasn't
> been any discussion or further analysis of the default cost settings
> either.  I feel that we don't have enough information.

Yea. I don't think we'll get really good feedback before production
unfortunately :(


> I would like, however, that we make a decision one way or the other
> before the next beta.  I've been handwaving a bit to users not to rely
> on the current betas for performance testing because the defaults might
> change later.  That's bad either way.

I don't see particularly much benefit in deciding before beta,
personally.  What's making you think it'd be important to decide before?
Pretty fundamentally, it'll be a setting you don't know is effectively
on, for the forseeable future anyway?

Greetings,

Andres Freund

Re: patch to allow disable of WAL recycling

[Alvaro Herrera]

On 2018-Aug-22, Andres Freund wrote:

> On 2018-08-22 11:06:17 -0300, Alvaro Herrera wrote:

> > I suppose that the use case that was initially proposed (ZFS) has not
> > yet been tested so we shouldn't reject this patch immediately, but
> > perhaps what Joyent people should be doing now is running Tomas' test
> > script on ZFS and see what the results look like.
> 
> IDK, I would see it less negatively. Yes, we should put a BIG FAT
> warning to never use this on non COW filesystems. And IMO ZFS (and also
> btrfs) sucks badly here, even though they really shouldn't. But given
> the positive impact for zfs & btrfs, and the low code complexity, I
> think it's not insane to provide this tunable.

Yeah, but let's see some ZFS numbers first :-)

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

Re: JIT compiling with LLVM v12

[Peter Eisentraut]

On 22/08/2018 08:20, Noah Misch wrote:
> Regardless of the choice of jit={on|off} default, these numbers tell me that
> some or all of jit_*_cost defaults are too low.

That was also my earlier analysis.

I'm suspicious that we haven't had much feedback about this.  We've
heard of one or two cases where LLVM broke a query outright, and that
was fixed and that was a good result.  But we haven't heard anything
about performance regressions.  Surely there must be some.  There hasn't
been any discussion or further analysis of the default cost settings
either.  I feel that we don't have enough information.

Another problem is that LLVM is only enabled in some versions of
packages.  For example, in the PGDG RPMs, it's enabled for RHEL 7 but
not RHEL 6.  So you could be in for a surprise if you upgrade your
operating system at some point.

I would like, however, that we make a decision one way or the other
before the next beta.  I've been handwaving a bit to users not to rely
on the current betas for performance testing because the defaults might
change later.  That's bad either way.

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

Re: patch to allow disable of WAL recycling

[Andres Freund]

On 2018-08-22 11:06:17 -0300, Alvaro Herrera wrote:
> On 2018-Aug-21, Jerry Jelinek wrote:
> 
> > Tomas,
> > 
> > Thanks for doing all of this testing. Your testing and results are much
> > more detailed than anything I did. Please let me know if there is any
> > follow-up that I should attempt.
> 
> Either I completely misread these charts, or there is practically no
> point to disabling WAL recycling (except on btrfs, but then nobody in
> their right minds would use it for Postgres given these numbers anyway).
> I suppose that the use case that was initially proposed (ZFS) has not
> yet been tested so we shouldn't reject this patch immediately, but
> perhaps what Joyent people should be doing now is running Tomas' test
> script on ZFS and see what the results look like.

IDK, I would see it less negatively. Yes, we should put a BIG FAT
warning to never use this on non COW filesystems. And IMO ZFS (and also
btrfs) sucks badly here, even though they really shouldn't. But given
the positive impact for zfs & btrfs, and the low code complexity, I
think it's not insane to provide this tunable.

Greetings,

Andres Freund

Re: patch to allow disable of WAL recycling

[Alvaro Herrera]

On 2018-Aug-21, Jerry Jelinek wrote:

> Tomas,
> 
> Thanks for doing all of this testing. Your testing and results are much
> more detailed than anything I did. Please let me know if there is any
> follow-up that I should attempt.

Either I completely misread these charts, or there is practically no
point to disabling WAL recycling (except on btrfs, but then nobody in
their right minds would use it for Postgres given these numbers anyway).
I suppose that the use case that was initially proposed (ZFS) has not
yet been tested so we shouldn't reject this patch immediately, but
perhaps what Joyent people should be doing now is running Tomas' test
script on ZFS and see what the results look like.

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

Re: Pre-v11 appearances of the word "procedure" in v11 docs

[Jonathan S. Katz]

> On Aug 22, 2018, at 9:15 AM, Peter Eisentraut 
>  wrote:
> 
> On 18/08/2018 19:37, Jonathan S. Katz wrote:
>> I reviewed the patches, here are my comments.
> 
> Committed all three with some adjustments.  Thanks.

Awesome, thanks! I removed the open item.

Jonathan


signature.asc
Description: Message signed with OpenPGP

Re: Pre-v11 appearances of the word "procedure" in v11 docs

[Peter Eisentraut]

On 18/08/2018 19:37, Jonathan S. Katz wrote:
> I reviewed the patches, here are my comments.

Committed all three with some adjustments.  Thanks.

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

Re: plan_cache_mode and postgresql.conf.sample

[David Rowley]

On 22 August 2018 at 18:41, Michael Paquier  wrote:
> On Wed, Aug 22, 2018 at 06:26:52PM +1200, Thomas Munro wrote:
>> Thanks, pushed.  I removed one tab because it looks like the comments
>> in that file are supposed to line up with tabstop=8 (unlike our source
>> files which use 4), and this one didn't.  I hope I got that right!

Thanks for pushing.

I'd failed to notice the 8 char tab stop alignment. Thanks for
removing the additional tab.

> This line now spawns at 87 characters for me, so that's not quite it.  I
> think that you should just have put the list into two lines.

Thomas has now fixed this, but it appears it wasn't the first line to
be guilty of being too long in that file.

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

Re: Speeding up INSERTs and UPDATEs to partitioned tables

[David Rowley]

On 22 August 2018 at 19:08, Amit Langote  wrote:
> +#define PartitionTupRoutingGetToParentMap(p, i) \
> +#define PartitionTupRoutingGetToChildMap(p, i) \
>
> If the "Get" could be replaced by "Child" and "Parent", respectively,
> they'd sound more meaningful, imho.

I did that to save 3 chars.  I think putting the additional
Child/Parent in the name is not really required. It's not as if we're
going to have a ParentToParent or a ChildToChild map, so I thought it
might be okay to assume that if it's "ToParent", that it's being
converted from the child and "ToChild" seems safe to assume it's being
converted from the parent. I can change it though if you feel very
strongly that what I've got is no good.

>> I also fixed a bug where
>> the child to parent map was not being initialised when on conflict
>> transition capture was required. I added a test which was crashing the
>> backend but fixed the code so it works correctly.
>
> Oops, I guess you mean my omission of checking if
> mtstate->mt_oc_transition_capture is non-NULL in ExecInitRoutingInfo.

Yeah.

> I've looked at v6 and spotted some minor typos.
>
> + * ResultRelInfo for, before we go making one, we check for a
> pre-made one
>
> s/making/make/g

I disagree, but perhaps we can just reword it a bit. I've now got:

+ * Every time a tuple is routed to a partition that we've yet to set the
+ * ResultRelInfo for, before we go to the trouble of making one, we check
+ * for a pre-made one in the hash table.

> +/* If nobody else set the per-subplan array of maps, do so ouselves. */
>
> I guess I'm the one to blame here for misspelling "ourselves".

Thanks for noticing.

> Since the above two are minor issues, fixed them myself in the attached
> updated version; didn't touch the macro though.

I've attached a v8. The only change from your v7 is in the "go making" comment.

> Do you agree to setting this patch to "Ready for Committer" in the
> September CF?

I read through the entire patch a couple of times yesterday and saw
nothing else, so yeah, I think now is a good time for someone with
more authority to have a look at it.

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


v8-0001-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch
Description: Binary data

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Sandeep Thakkar]

On Wed, Aug 22, 2018 at 5:32 PM, Andres Freund  wrote:

> Hi,
>
> On 2018-08-22 17:17:27 +0530, Sandeep Thakkar wrote:
> > > We build windows binaries (>=9.3) on Windows 7 and Windows Server 2012
> R2.
> > For 9.3, the Visual Studio version is 2010 and for 9.4 and v10, we use
> > 2013. For v11, we use 2017.
>
> Sndeep: Thanks for the information.  Did you ever encounter problems (at
> build or during runtime) with using those binaries on older platforms?
>
> IIRC when the binaries were built with VC++ 2013 on 9.4, we had problems
running them on XP and hence we had used "/p:PlatformToolset=v120_xp"
option to msbuild during build time. From v10, we stopped using that
toolset and instead used the default one i.e v120


> Everyone: Given the fact that all the people building windows packages
> currently use a new enough stack by a fair margin, I think we should
> conclude that there's no obstacle on the windows side of things.
>
>
> If we agree on that, I'm going to propose a patch that includes:
> - relevant cleanups to configure
> - adapts sources.sgml to refer to C99 instead of C89
> - add some trivial conversions to for(int i;;) and struct initializers,
>   so the relevant old animals fail
> - adds a configure check to enable errors with vla usage (-Werror=vla)
>
> Questions:
>
> - do we want to make declarations at arbitrary points errors? It's
>   already a warning currently.
> - other new restrictions that we want to introduce at the same time?
>
> Greetings,
>
> Andres Freund
>



-- 
Sandeep Thakkar

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Andres Freund]

Hi,

On 2018-08-22 17:17:27 +0530, Sandeep Thakkar wrote:
> > We build windows binaries (>=9.3) on Windows 7 and Windows Server 2012 R2.
> For 9.3, the Visual Studio version is 2010 and for 9.4 and v10, we use
> 2013. For v11, we use 2017.

Sndeep: Thanks for the information.  Did you ever encounter problems (at
build or during runtime) with using those binaries on older platforms?

Everyone: Given the fact that all the people building windows packages
currently use a new enough stack by a fair margin, I think we should
conclude that there's no obstacle on the windows side of things.


If we agree on that, I'm going to propose a patch that includes:
- relevant cleanups to configure
- adapts sources.sgml to refer to C99 instead of C89
- add some trivial conversions to for(int i;;) and struct initializers,
  so the relevant old animals fail
- adds a configure check to enable errors with vla usage (-Werror=vla)

Questions:

- do we want to make declarations at arbitrary points errors? It's
  already a warning currently.
- other new restrictions that we want to introduce at the same time?

Greetings,

Andres Freund

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Sandeep Thakkar]

On Wed, Aug 22, 2018 at 4:59 AM, Andres Freund  wrote:

> On 2018-08-21 17:58:00 -0400, Andrew Dunstan wrote:
> >
> >
> > On 08/21/2018 04:49 PM, Andres Freund wrote:
> > > On 2018-08-21 11:09:15 -0700, Joshua D. Drake wrote:
> > > > On 08/21/2018 11:06 AM, Andrew Dunstan wrote:
> > > > >
> > > > >
> > > > > XP at least is essentially a dead platform for us. My animals are
> not
> > > > > able to build anything after release 10.
> > > > I wouldn't think XP should even be on our list anymore. Microsoft
> hasn't
> > > > supported it in 4 years.
> > > XP isn't the only thing relevant here, vista and 2008 R1 are in the
> same
> > > class.
> > >
> >
> >
> > I do have a machine in my laptop graveyard with Vista. The only WS2008
> > instace I have available is R2 and AWS doesn't seem to have any AMIs for
> R1.
> >
> > Honestly, I don't think these matter terribly much. Anyone building now
> is
> > not likely to be targeting them.
>
> I agree, I think we should just decree that the minimum is MSVC 2013 and
> that people building 12 need to deal with that.  I would personally
> *additionally* would say that we officially don't support *running* (not
> compiling) on XP, 2003, 2008R1 and Vista (all unsupported by MS) - but
> that's a somewhat orthogonal decision.
>
> We build windows binaries (>=9.3) on Windows 7 and Windows Server 2012 R2.
For 9.3, the Visual Studio version is 2010 and for 9.4 and v10, we use
2013. For v11, we use 2017.


> Greetings,
>
> Andres Freund
>



-- 
Sandeep Thakkar

Re: C99 compliance for src/port/snprintf.c

[Sandeep Thakkar]

Hi

On Thu, Aug 16, 2018 at 6:30 PM, Andres Freund  wrote:

> Hi,
>
> On 2018-08-16 14:28:25 +0200, Peter Eisentraut wrote:
> > On 16/08/2018 01:06, Andres Freund wrote:
> > > So it looks like msvc 2013 might be the relevant requirement.
> >
> > According to my research (completely untested in practice), you need
> > 2010 for mixed code and declarations and 2013 for named initialization
> > of structs.
> >
> > I wonder what raising the msvc requirement would imply for supporting
> > older Windows versions.
>
> One relevant tidbit is that afaict 2013 still allows *targeting* older
> versions of windows, down to XP and 2003, while requiring a newer
> platforms to run. See:
> https://docs.microsoft.com/en-us/visualstudio/productinfo/
> vs2013-compatibility-vs
> I don't know if that's hard to do, but I strongly suspect that the
> existing installers already do that (otherwise supporting newer versions
> would likely require separate builds).
>
> 2013 still runs on Windows 7, should you want that:
> https://docs.microsoft.com/en-us/visualstudio/productinfo/
> vs2013-sysrequirements-vs
>
> According to https://www.postgresql.org/download/windows/
> the available binaries already effectively restrict windows support:
>
> EDB installers, for 10, restrict to:
> 64 Bit Windows: 2016, 2012 R2 & R1, 2008 R2, 7, 8, 10
> 32 Bit Windows: 2008 R1, 7, 8, 10
>
> BIGSQL to: Windows 10 and Windows Server 2012.
>
> Of those 2013 only doesn't run on 2008 R1 anymore. Which still can be
> targeted from the newer windows versions.
>
>
> It'd be good to get confirmation that the windows binaries / installers
> are indeed built on newer platforms than the oldest supported version.
>
> We build windows binaries (>=9.3) on Windows 7 and Windows Server 2012 R2.
For 9.3, the Visual Studio version is 2010 and for 9.4 and v10, we use
2013. For v11, we use 2017.


> Random observation: http://www.openscg.com/bigsql/postgresql/installers/
> seems to indicate that packages aren't updated anymore. While it says
> "(09-Aug-18)" besides the major versions, it does not actually in fact
> have the last set of minor releases.  I suspect that's related to
> openscg's acquisition by amazon?  Either they need to catch up, or we
> need to take down the page and probably alert people about that fact.
>
> Greetings,
>
> Andres Freund
>



-- 
Sandeep Thakkar

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

[Etsuro Fujita]

(2018/08/16 12:00), Etsuro Fujita wrote:

(2018/08/15 23:40), Robert Haas wrote:

Given the comments from the RMT, and also on general principle, it
seems like we really need to get on with committing something here.
It's my understanding you plan to do that, since it's your patch.
When?


I plan to do that late next week as I'll go on leave until next Tuesday.


I added the commit message.  Please find attached an updated version of 
the patch.  Does that make sense?  If there are not objections, I'll 
push this tomorrow.


Best regards,
Etsuro Fujita
>From e8e5fa32984a51e73831529b055bddbed03feaa1 Mon Sep 17 00:00:00 2001
From: Etsuro Fujita 
Date: Wed, 22 Aug 2018 19:59:01 +0900
Subject: [PATCH] Disable support for partitionwise joins in problematic
 cases.

Commit f49842d1ee31b976c681322f76025d7732e860f3, which added support for
partitionwise joins, built the child's tlist by applying
adjust_appendrel_attrs() to the parent's.  So in the case where the parent
tlist included a whole-row Var for the parent, the child tlist contained a
ConverRowtypeExpr.  That commit added code to handle that to the planner,
but some code paths still made the old assumption that a scan or join
rel's would only include Vars and PlaceHolderVars, causing the following
errors:

* When creating an explicit sort node for an input path for building a
  mergejoin plan for a child join, prepare_sort_from_pathkeys() throws the
  'could not find pathkey item to sort' error.
* When deparsing a relation participating a pushed down foreign child join
  as a subquery in contrib/postgres_fdw, get_relation_column_alias_ids()
  throws the 'unexpected expression in subquery output' error.
* When performing set_plan_references() on a local join plan created by
  contrib/postgres_fdw for EvalPlanQual support, fix_join_expr() throws
  the 'variable not found in subplan target lists' error.

To fix these, two approaches have been proposed: one by Ashutosh Bapat and
one by me.  While the former keeps building the child's tlist with a
ConvertRowtypeExpr, the latter builds it with a whole-row Var for the
child and tries to fix it up later.  But both approaches need more work,
so refuse to generate partitionwise join paths when whole-row Vars are
involved, instead.  We don't need to handle ConvertRowtypeExprs in the
child's tlists for now, so this commit also removes the changes to the
planner, such as setrefs.c.

Previously, partitionwise join computed attr_needed data for each child
separately, and built the child join's tlists using that data, which also
required an extra step for adding PlaceHolderVars to the child join's
tlists, but it would be more efficient to build the child join's tlist by
applying adjust_appendrel_attrs() to the parent join's.  So this commit
also changes it that way, and simplifies build_joinrel_tlist() and
placeholder.c as well as part of set_append_rel_size() to basically what
they were before partitionwise join went in.

Back-patch to PG11 where partitionwise join was introduced.

Report by Rajkumar Raghuwanshi.  Analysis by Ashutosh Bapat, who also
provided some of regression tests.  Patch by me, reviewed by Robert Haas.

Discussion: https://postgr.es/m/cakcux6ktu-8teflwtquuzbyfaza83vuzurd7c1yhc-yewyy...@mail.gmail.com
---
 contrib/postgres_fdw/expected/postgres_fdw.out|   71 ++---
 contrib/postgres_fdw/sql/postgres_fdw.sql |   14 ++-
 src/backend/nodes/outfuncs.c  |1 +
 src/backend/optimizer/path/allpaths.c |   78 +
 src/backend/optimizer/path/joinrels.c |7 +
 src/backend/optimizer/plan/setrefs.c  |   58 +-
 src/backend/optimizer/util/placeholder.c  |   58 --
 src/backend/optimizer/util/relnode.c  |  125 +++--
 src/include/nodes/relation.h  |8 +-
 src/test/regress/expected/partition_aggregate.out |   84 ++
 src/test/regress/expected/partition_join.out  |   93 +---
 src/test/regress/sql/partition_aggregate.sql  |   10 ++
 src/test/regress/sql/partition_join.sql   |   14 ++-
 13 files changed, 365 insertions(+), 256 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d912bd9..21a2ef5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8337,8 +8337,9 @@ ALTER TABLE fprt2_p1 SET (autovacuum_enabled = 'false');
 ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
 INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM') FROM generate_series(0, 249, 3) i;
 INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM') FROM generate_series(250, 499, 3) i;
-CREATE FOREIGN TABLE ftprt2_p1 PARTITION OF fprt2 FOR VALUES FROM (0) TO (250)
+CREATE FOREIGN TABLE ftprt2_p1 (b int, c varchar, a int)
 	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
+ALTER 

Re: Proposal: SLRU to Buffer Cache

[Andres Freund]

Hi,

On 2018-08-22 13:35:47 +0500, Andrey Borodin wrote:
> > 15 авг. 2018 г., в 2:35, Shawn Debnath  написал(а):
> > 
> > At the Unconference in Ottawa this year, I pitched the idea of moving
> > components off of SLRU and on to the buffer cache. The motivation
> > behind the idea was three fold:
> > 
> >  * Improve performance by eliminating fixed sized caches, simplistic
> >scan and eviction algorithms.
> >  * Ensuring durability and consistency by tracking LSNs and checksums
> >per block.
> +1, I like this idea more than current patch on CF with checksums for SLRU 
> pages.

Yea, I don't think it really makes sense to reimplement this logic for
SLRUs (and then UNDO) separately.


> >  1. Implement a generic block storage manager that parameterizes
> > several options like segment sizes, fork and segment naming and
> > path schemes, concepts entrenched in md.c that are strongly tied to
> > relations. To mitigate risk, I am planning on not modifying md.c
> > for the time being.
> Probably I'm missing something, but why this should not be in access
> methods?

I think it's not an absurd idea to put the reserved oid into pg_am
(under a separate amtype). Although the fact that shared entries would
be in database local tables is a bit weird. But I'm fairly certain that
we'd not put any actual data into it, not the least because we need to
be able to acess clo etc from connections that cannot attach to a
database (say the startup process, which will never ever start reading
from a catalog table).  So I don't really see what you mean with:

> You can extend AM to control it's segment size and ability to
> truncate unneeded pages. This may to be useful, for example, in LSM
> tree implementation or something similar.

that doesn't really seem like it could work. Nor am I even clear what
the above points really have to do with the AM layer.

Greetings,

Andres Freund

Re: Windows vs C99 (was Re: C99 compliance for src/port/snprintf.c)

[Amit Kapila]

On Tue, Aug 21, 2018 at 11:16 PM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2018-08-21 13:29:20 -0400, Tom Lane wrote:
>>> We've got a buildfarm handy that could answer the question.
>>> Let's just stick a test function in there for a day and see
>>> which animals fail.
>
>> I think we pretty much know the answer already, anything before 2013
>> will fail.
>
> Do we know that for sure?  I thought it was theoretical.
>

I have MSVC 2010 on my Windows 7 VM where I have tried the C99
constructs provided by Peter E. and below are my findings:

Tried compiling below function in one of the .c files in the src/backend
---
int
bar()
{
int x;
x = 1;
int y;
y = 2;

for (int i = 0; i < 5; i++)
;

return 0;
}

error C2143: syntax error : missing ';' before 'type'
error C2065: 'y' : undeclared identifier
error C2143: syntax error : missing ';' before 'type'
error C2143: syntax error : missing ';' before 'type'
error C2143: syntax error : missing ')' before 'type'
error C2143: syntax error : missing ';' before 'type'
error C2065: 'i' : undeclared identifier
warning C4552: '<' : operator has no effect; expected operator with side-effect
error C2065: 'i' : undeclared identifier
error C2059: syntax error : ')'

Tried compiling below struct in one of the .c files in the src/backend
---
struct foo
{
int a;
int b;
};

struct foo f = {
.a = 1,
.b = 2
};

error C2059: syntax error : '.'

I have also tried compiling above in standalone console application
project in MSVC 2010 and able to compile the function successfully,
but struct is giving the same error as above.  So, it seems to me that
it won't work on <= MSVC 2010.  I am personally okay with upgrading my
Windows VM.

I have found a couple of links which suggest that MSVC 2015 has a good
amount of conformance with C99 [1].  It seems MSVC 2013 also has
decent support for C99 [2], but I am not able to verify if the
constructs shared by Peter E can be compiled on it.

[1] 
https://social.msdn.microsoft.com/Forums/en-US/fa17bcdd-7165-4645-a676-ef3797b95918/details-on-c99-support-in-msvc?forum=vcgeneral
[2] 
https://blogs.msdn.microsoft.com/vcblog/2013/07/19/c99-library-support-in-visual-studio-2013/

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

Re: Two proposed modifications to the PostgreSQL FDW

[Masahiko Sawada]

On Wed, Aug 22, 2018 at 5:13 PM Chris Travers  wrote:
>
>
>
> On Wed, Aug 22, 2018 at 9:09 AM Masahiko Sawada  wrote:
>>
>> On Wed, Aug 22, 2018 at 1:20 PM Chris Travers  
>> wrote:
>> > The two things I would suggest is that rather than auto-detecting (if I 
>> > understand your patch correctly) whether prepared transactions are 
>> > possible on the other system, making it  an option to the foreign server 
>> > or foreign table.  Otherwise one might enable prepared transactions for 
>> > one set of operations on one database and have it automatically cause 
>> > headaches in another context.
>>
>> Yeah, currently it's an option for foreign servers. The patch adds a
>> new option "two_phase_commit" to postgres_fdw.
>
>
> Seems sane.
>>
>>
>> >
>> > The other thing I wasn't quite sure about on your patch was what happens 
>> > if, say, someone trips over a power cord while the background worker is 
>> > trying to commit things, whether the information is available on the 
>> > initiating server when it comes back. whether a DBA has to go out and try 
>> > to figure out what needs to be committed remotely, and how this would be 
>> > done.  If you could explain that process, that would be helpful to me.
>> >
>> > (In my approach these would be recorded on the master and an SQL function 
>> > could re-initiate the background worker.)
>>
>> My approach is almost the same as yours. For details, in the
>> pre-commit we do WAL-logging for each participants server before
>> preparing transactions on the remote sides. The WAL has information of
>> participants foreign servers(foreign server oid, database oid etc) and
>> its global transaction identifier. Even if plug-pulled during trying
>> to commit we can recover the global transactions that are not
>> completed yet and its participants information from WAL. After the
>> recovery users needs to execute the SQL function to fix the
>> in-completed global transactions. Since the function can find out
>> whether the remote transaction should be committed or rollback-ed by
>> checking CLOG. Does my answer make sense?
>
>
> Yeah.  That is probably more elegant than my solution.  I do wonder though if 
> the next phase would not be to add some sort of hook to automatically start 
> the background worker in this case.
>>
>>
>> >>
>> >>
>> >> >
>> >> > Moreover since COMMIT PREPARED occurs during the commit hook, not the 
>> >> > precommit hook, it is too late to roll back the local transaction.  We 
>> >> > cannot raise errors since this causes a conflict in the commit status 
>> >> > of the local transaction.  So when we commit the local transaction we 
>> >> > commit to committing all prepared transactions as soon as possible.  
>> >> > Note some changes need to be made to make this usable in the FDW 
>> >> > context, so what I am hoping is that the dialog helps impact the 
>> >> > discussion and options going forward.
>> >> >
>> >> >>
>> >> >> Also
>> >> >> since we don't want to wait for COMMIT PREPARED to complete we need to
>> >> >> consider that users could cancel the query anytime. To not break the
>> >> >> current semantics we cannot raise error during 2nd phase of two-phase
>> >> >> commit but it's not realistic because even the palloc() can raise an
>> >> >> error.
>> >> >
>> >> >
>> >> > We don't palloc.  All memory used here is on the stack.  I do allow for 
>> >> > dramatic precondition checks to cause errors but those should never 
>> >> > happen absent some other programmer doing something dramatically unsafe 
>> >> > anyway.  For example if you try to double-commit a transaction set.
>> >>
>> >> Sorry, palloc() is just an example. I'm not sure all FDWs can
>> >> implement all callbacks for two-phase commit without codes that could
>> >> emit errors.
>> >
>> >
>> > Yeah, but if you are in the commit hook and someone emits an error, that's 
>> > wrong because that then tries to rollback an already committed transaction 
>> > and the backend rightfully panics.  In fact I should probably strip out 
>> > the precondition check errors there and issue  a warning.  It might 
>> > sometimes happen when something goes seriously wrong on a system level, 
>> > but
>>
>> In my patch since the commit hook is performed by the background
>> worker not by the backends it's no problem if someone emits an error
>> in the commit hook. After the backend prepared transactions on the all
>> remote side, it enqueue itself to the wait queue. The background
>> worker gets the global transaction waiting to be completed and commit
>> prepared transaction on all remote side. After completed the global
>> transaction the background worker dequeue it.
>
>
> Seems sane.  I was firing off one background worker per global transaction 
> that needed cleanup.  This might be an area to think about in terms of 
> questions of larger parallelism in remote writes.
>>
>>
>> >>
>> >>
>> >> >
>> >> > There is a possible of system errors if one can no longer write to the 
>> >> > file 

Re: Proposal: SLRU to Buffer Cache

[Andrey Borodin]

Hi!

> 15 авг. 2018 г., в 2:35, Shawn Debnath  написал(а):
> 
> At the Unconference in Ottawa this year, I pitched the idea of moving
> components off of SLRU and on to the buffer cache. The motivation
> behind the idea was three fold:
> 
>  * Improve performance by eliminating fixed sized caches, simplistic
>scan and eviction algorithms.
>  * Ensuring durability and consistency by tracking LSNs and checksums
>per block.
+1, I like this idea more than current patch on CF with checksums for SLRU 
pages.

>  1. Implement a generic block storage manager that parameterizes
> several options like segment sizes, fork and segment naming and
> path schemes, concepts entrenched in md.c that are strongly tied to
> relations. To mitigate risk, I am planning on not modifying md.c
> for the time being.
Probably I'm missing something, but why this should not be in access methods? 
You can extend AM to control it's segment size and ability to truncate unneeded 
pages. This may to be useful, for example, in LSM tree implementation or 
something similar.

Best regards, Andrey Borodin.

Re: JIT compiling with LLVM v12

[Andres Freund]

On 2018-08-22 06:20:21 +, Noah Misch wrote:
> On Wed, Mar 28, 2018 at 02:27:51PM -0700, Andres Freund wrote:
> > For now LLVM is enabled by default when compiled --with-llvm. I'm mildly
> > inclined to leave it like that until shortly before the release, and
> > then disable it by default (i.e. change the default of jit=off). But I
> > think we can make that decision based on experience during the testing
> > window. I'm opening an open items entry for that.
> 
> I'll vote for jit=on and letting any bugs shake out earlier, but it's not a
> strong preference.

Similar.


> I see jit slows the regression tests considerably:
> 
> # x86_64, non-assert, w/o llvm
> $ for n in 1 2 3; do env time make -C src/bin/pg_upgrade check; done 2>&1 | 
> grep elapsed
> 7.64user 4.24system 0:36.40elapsed 32%CPU (0avgtext+0avgdata 
> 36712maxresident)k
> 8.09user 4.50system 0:37.71elapsed 33%CPU (0avgtext+0avgdata 
> 36712maxresident)k
> 7.53user 4.18system 0:36.54elapsed 32%CPU (0avgtext+0avgdata 
> 36712maxresident)k
> 
> # x86_64, non-assert, w/  llvm trunk
> $ for n in 1 2 3; do env time make -C src/bin/pg_upgrade check; done 2>&1 | 
> grep elapsed
> 9.58user 5.79system 0:49.61elapsed 30%CPU (0avgtext+0avgdata 
> 36712maxresident)k
> 9.47user 5.92system 0:47.84elapsed 32%CPU (0avgtext+0avgdata 
> 36712maxresident)k
> 9.09user 5.51system 0:47.94elapsed 30%CPU (0avgtext+0avgdata 
> 36712maxresident)k
> 
> # mips32el, assert, w/o llvm (buildfarm member topminnow) [1]
> 28min install-check-*
> 35min check-pg_upgrade
> 
> # mips32el, assert, w/  llvm 6.0.1 [1]
>  63min install-check-*
> 166min check-pg_upgrade
> 
> Regardless of the choice of jit={on|off} default, these numbers tell me that
> some or all of jit_*_cost defaults are too low.

I don't think it really shows that. The reason that JITing gets started
there is that the tables aren't analyzed and we end up with crazy ass
estimates about the cost of the queries. No useful setting of the cost
limits will protect against that... :(

Greetings,

Andres Freund

Re: Two proposed modifications to the PostgreSQL FDW

[Chris Travers]

On Wed, Aug 22, 2018 at 9:09 AM Masahiko Sawada 
wrote:

> On Wed, Aug 22, 2018 at 1:20 PM Chris Travers 
> wrote:
> > The two things I would suggest is that rather than auto-detecting (if I
> understand your patch correctly) whether prepared transactions are possible
> on the other system, making it  an option to the foreign server or foreign
> table.  Otherwise one might enable prepared transactions for one set of
> operations on one database and have it automatically cause headaches in
> another context.
>
> Yeah, currently it's an option for foreign servers. The patch adds a
> new option "two_phase_commit" to postgres_fdw.
>

Seems sane.

>
> >
> > The other thing I wasn't quite sure about on your patch was what happens
> if, say, someone trips over a power cord while the background worker is
> trying to commit things, whether the information is available on the
> initiating server when it comes back. whether a DBA has to go out and try
> to figure out what needs to be committed remotely, and how this would be
> done.  If you could explain that process, that would be helpful to me.
> >
> > (In my approach these would be recorded on the master and an SQL
> function could re-initiate the background worker.)
>
> My approach is almost the same as yours. For details, in the
> pre-commit we do WAL-logging for each participants server before
> preparing transactions on the remote sides. The WAL has information of
> participants foreign servers(foreign server oid, database oid etc) and
> its global transaction identifier. Even if plug-pulled during trying
> to commit we can recover the global transactions that are not
> completed yet and its participants information from WAL. After the
> recovery users needs to execute the SQL function to fix the
> in-completed global transactions. Since the function can find out
> whether the remote transaction should be committed or rollback-ed by
> checking CLOG. Does my answer make sense?
>

Yeah.  That is probably more elegant than my solution.  I do wonder though
if the next phase would not be to add some sort of hook to automatically
start the background worker in this case.

>
> >>
> >>
> >> >
> >> > Moreover since COMMIT PREPARED occurs during the commit hook, not the
> precommit hook, it is too late to roll back the local transaction.  We
> cannot raise errors since this causes a conflict in the commit status of
> the local transaction.  So when we commit the local transaction we commit
> to committing all prepared transactions as soon as possible.  Note some
> changes need to be made to make this usable in the FDW context, so what I
> am hoping is that the dialog helps impact the discussion and options going
> forward.
> >> >
> >> >>
> >> >> Also
> >> >> since we don't want to wait for COMMIT PREPARED to complete we need
> to
> >> >> consider that users could cancel the query anytime. To not break the
> >> >> current semantics we cannot raise error during 2nd phase of two-phase
> >> >> commit but it's not realistic because even the palloc() can raise an
> >> >> error.
> >> >
> >> >
> >> > We don't palloc.  All memory used here is on the stack.  I do allow
> for dramatic precondition checks to cause errors but those should never
> happen absent some other programmer doing something dramatically unsafe
> anyway.  For example if you try to double-commit a transaction set.
> >>
> >> Sorry, palloc() is just an example. I'm not sure all FDWs can
> >> implement all callbacks for two-phase commit without codes that could
> >> emit errors.
> >
> >
> > Yeah, but if you are in the commit hook and someone emits an error,
> that's wrong because that then tries to rollback an already committed
> transaction and the backend rightfully panics.  In fact I should probably
> strip out the precondition check errors there and issue  a warning.  It
> might sometimes happen when something goes seriously wrong on a system
> level, but
>
> In my patch since the commit hook is performed by the background
> worker not by the backends it's no problem if someone emits an error
> in the commit hook. After the backend prepared transactions on the all
> remote side, it enqueue itself to the wait queue. The background
> worker gets the global transaction waiting to be completed and commit
> prepared transaction on all remote side. After completed the global
> transaction the background worker dequeue it.
>

Seems sane.  I was firing off one background worker per global transaction
that needed cleanup.  This might be an area to think about in terms of
questions of larger parallelism in remote writes.

>
> >>
> >>
> >> >
> >> > There is a possible of system errors if one can no longer write to
> the file log but at this point as long as we have logged the phase change
> to commit we are able to recover later.
> >> >
> >> > So in the event where one sees an error here one continues on to the
> next transaction in the global transaction set and tries to commit it, etc.
> until it runs through 

Re: Two proposed modifications to the PostgreSQL FDW

[Masahiko Sawada]

On Wed, Aug 22, 2018 at 1:20 PM Chris Travers  wrote:
>
>
>
> On Wed, Aug 22, 2018 at 3:12 AM Masahiko Sawada  wrote:
>>
>> On Tue, Aug 21, 2018 at 5:36 PM Chris Travers  
>> wrote:
>> >
>> >
>> >
>> > On Tue, Aug 21, 2018 at 8:42 AM Masahiko Sawada  
>> > wrote:
>> >>
>> >> On Tue, Aug 21, 2018 at 1:47 AM Chris Travers  
>> >> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Aug 20, 2018 at 4:41 PM Andres Freund  
>> >> > wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> On 2018-08-20 16:28:01 +0200, Chris Travers wrote:
>> >> >> > 2.  TWOPHASECOMMIT=[off|on] option
>> >> >>
>> >> >> > The second major issue that I see with PostgreSQL's foreign database
>> >> >> > wrappers is the fact that there is no two phase commit which means 
>> >> >> > that a
>> >> >> > single transaction writing to a group of tables has no expectation 
>> >> >> > that all
>> >> >> > backends will commit or rollback together.  With this patch an 
>> >> >> > option would
>> >> >> > be applied to foreign tables such that they could be set to use two 
>> >> >> > phase
>> >> >> > commit  When this is done, the first write to each backend would 
>> >> >> > register a
>> >> >> > connection with a global transaction handler and a pre-commit and 
>> >> >> > commit
>> >> >> > hooks would be set up to properly process these.
>> >> >> >
>> >> >> > On recommit a per-global-transaction file would be opened in the data
>> >> >> > directory and prepare statements logged to the file.  On error, we 
>> >> >> > simply
>> >> >> > roll back our local transaction.
>> >> >> >
>> >> >> > On commit hook , we go through and start to commit the remote global
>> >> >> > transactions.  At this point we make a best effort but track whether 
>> >> >> > or not
>> >> >> > we were successfully on all.  If successful on all, we delete the 
>> >> >> > file.  If
>> >> >> > unsuccessful we fire a background worker which re-reads the file and 
>> >> >> > is
>> >> >> > responsible for cleanup.  If global transactions persist, a SQL
>> >> >> > administration function will be made available to restart the cleanup
>> >> >> > process.  On rollback, we do like commit but we roll back all 
>> >> >> > transactions
>> >> >> > in the set.  The file has enough information to determine whether we 
>> >> >> > should
>> >> >> > be committing or rolling back on cleanup.
>> >> >> >
>> >> >> > I would like to push these both for Pg 12.  Is there any feedback on 
>> >> >> > the
>> >> >> > concepts and the problems first
>> >> >>
>> >>
>> >> Thank you for the proposal. I agree that it's a major problem that
>> >> postgres_fdw (or PostgreSQL core API) doesn't support two-phase
>> >> commit.
>> >>
>> >> >> There's been *substantial* work on this. You should at least read the
>> >> >> discussion & coordinate with the relevant developers.
>> >> >
>> >> >
>> >> > I suppose I should forward this to them directly also.
>> >> >
>> >> > Yeah.   Also the transaction manager code for this I wrote while 
>> >> > helping with a proof of concept for this copy-to-remote extension.
>> >> >
>> >> > There are a few big differences in implementation with the patches you 
>> >> > mention and the disagreement was part of why I thought about going this 
>> >> > direction.
>> >> >
>> >> > First, discussion of differences in implementation:
>> >> >
>> >> > 1.  I treat the local and remote transactions symmetrically and I make 
>> >> > no assumptions about what might happen between prepare and an attempted 
>> >> > local commit.
>> >> >prepare goes into the precommit hook
>> >> >commit goes into the commit hook and we never raise errors if it 
>> >> > fails (because you cannot rollback at that point).  Instead a warning 
>> >> > is raised and cleanup commences.
>> >> >rollback goes into the rollback hook and we never raise errors if it 
>> >> > fails (because you are already rolling back).
>> >> >
>> >> > 2.  By treating this as a property of a table rather than a property of 
>> >> > a foreign data wrapper or a server, we can better prevent prepared 
>> >> > transactions where they have not been enabled.
>> >> >This also ensures that we know whether we are guaranteeing two phase 
>> >> > commit or not by looking at the table.
>> >> >
>> >> > 3.  By making this opt-in it avoids a lot of problems with regards to 
>> >> > incorrect configuration etc since if the DBA says "use two phase 
>> >> > commit" and failed to enable prepared transactions on the other side...
>> >> >
>> >> > On to failure modes:
>> >> >  1.  Its possible that under high load too many foreign transactions 
>> >> > are prepared and things start rolling back instead of committing.  Oh 
>> >> > well
>> >> >  2.  In the event that a foreign server goes away between prepare and 
>> >> > commit, we continue to retry via the background worker.  The background 
>> >> > worker is very pessimistic and checks every remote system for the named 
>> >> > transaction.
>> >>
>> >> If some participant servers fail during COMMIT PREPARED, will 

Re: Speeding up INSERTs and UPDATEs to partitioned tables

[Amit Langote]

On 2018/08/21 14:44, David Rowley wrote:
> On 3 August 2018 at 17:58, Amit Langote  wrote:
>> On 2018/07/31 16:03, David Rowley wrote:
>>> Maybe we can do that as a follow-on patch.
>>
>> We probably could, but I think it would be a good idea get rid of *all*
>> redundant allocations due to tuple routing in one patch, if that's the
>> mission of this thread and the patch anyway.
> 
> I started looking at this patch today and I now agree that it should
> be included in the main patch.

Great, thanks.

> I changed a few things with the patch. For example, the map access
> macros you'd defined were not in CamelCase.

In the updated patch:

+#define PartitionTupRoutingGetToParentMap(p, i) \
+#define PartitionTupRoutingGetToChildMap(p, i) \

If the "Get" could be replaced by "Child" and "Parent", respectively,
they'd sound more meaningful, imho.

> I also fixed a bug where
> the child to parent map was not being initialised when on conflict
> transition capture was required. I added a test which was crashing the
> backend but fixed the code so it works correctly.

Oops, I guess you mean my omission of checking if
mtstate->mt_oc_transition_capture is non-NULL in ExecInitRoutingInfo.

Thanks for fixing it and adding the test case.

> I also got rid of
> the child_parent_map_not_required array since we now no longer need
> it.  The code now always initialises the maps in cases where they're
> going to be required.

Yes, thought I had removed the field in my patch, but looks like I had
just removed the comment about it.

> I've attached a v3 version of your patch and also v6 of the main patch
> which includes the v3 patch.

I've looked at v6 and spotted some minor typos.

+ * ResultRelInfo for, before we go making one, we check for a
pre-made one

s/making/make/g

+/* If nobody else set the per-subplan array of maps, do so ouselves. */

I guess I'm the one to blame here for misspelling "ourselves".


Since the above two are minor issues, fixed them myself in the attached
updated version; didn't touch the macro though.

Do you agree to setting this patch to "Ready for Committer" in the
September CF?

Thanks,
Amit

From 79c906997d80dc426530dea0b75363ef20286001 Mon Sep 17 00:00:00 2001
From: "dgrow...@gmail.com" 
Date: Thu, 26 Jul 2018 19:54:55 +1200
Subject: [PATCH v7] Speed up INSERT and UPDATE on partitioned tables

This is more or less a complete redesign of PartitionTupleRouting. The
aim here is to get rid of all the possibly large arrays that were being
allocated during ExecSetupPartitionTupleRouting().  We now allocate
small arrays to store the partition's ResultRelInfo and only enlarge
these when we run out of space.  The partitions array is now ordered
by the order in which the partition's ResultRelInfos are initialized
rather than in same order as partdesc.

The slowest part of ExecSetupPartitionTupleRouting still remains.  The
find_all_inheritors call still remains by far the slowest part of the
function. This patch just removes the other slow parts.

Initialization of the parent to child and child to parent translation maps
arrays are now only performed when we need to store the first translation
map.  If the column order between the parent and its child are the same,
then no map ever needs to be stored, these (possibly large) arrays did
nothing.  The fact that we now always initialize the child to parent map
whenever transition capture is required, we no longer need the
child_parent_map_not_required array.  Previously this was only required
so we could determine if no map was required or if the map had not yet
been initialized.

In simple INSERTs hitting a single partition to a partitioned table with
many partitions, the shutdown of the executor was also slow in comparison
to the actual execution. This was down to the loop which cleans up each
ResultRelInfo having to loop over an array which contained mostly NULLs
which had to be skipped.  Performance of this has now improved as the array
we loop over now no longer has to skip possibly many NULL values.

David Rowley and Amit Langote
---
 src/backend/commands/copy.c   |  48 +-
 src/backend/executor/execPartition.c  | 798 ++
 src/backend/executor/nodeModifyTable.c| 109 +---
 src/backend/utils/cache/partcache.c   |  11 +-
 src/include/catalog/partition.h   |   6 +-
 src/include/executor/execPartition.h  | 171 --
 src/test/regress/expected/insert_conflict.out |  22 +
 src/test/regress/sql/insert_conflict.sql  |  26 +
 8 files changed, 626 insertions(+), 565 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 9bc67ce60f..0dfb9e2e95 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2510,8 +2510,12 @@ CopyFrom(CopyState cstate)
/*
 * If there are any triggers with transition tables on the named 
relation,
 * we need to be prepared to capture transition tuples.
+*

Re: [HACKERS] proposal: schema variables

[Fabien COELHO]

Hello Pavel,


AFAICR, I had an objection on such new objects when you first proposed
something similar in October 2016.

Namely, if session variables are not transactional, they cannot be used to
implement security related auditing features which were advertised as the
motivating use case: an the audit check may fail on a commit because of a
differed constraint, but the variable would keep its "okay" value unduly,
which would create a latent security issue, the audit check having failed
but the variable saying the opposite.

So my point was that they should be transactional by default, although I
would be ok with an option for having a voluntary non transactional
version.

Is this issue addressed somehow with this ?



1. I respect your opinion, but I dont agree with it. Oracle, db2 has
similar or very similar feature non transactional, and I didnt find any
requests to change it.


The argument of authority that "X does it like that" is not a valid answer 
to my technical objection about security implications of this feature.



2. the prototype implementation was based on relclass items, and some
transactional behave was possible. Peter E. had objections to this design
and proposed own catalog table. I did it. Now, the transactional behave is
harder to implement, although it is not impossible. This patch is not small
now, so I didnt implement it.


"It is harder to implement" does not look like a valid answer either.


I have a strong opinion so default behave have to be non transactional.


The fact that you have a "strong opinion" does not really answer my 
objection. Moreover, I said that I would be ok with a non transactional 
option, provided that a default transactional is available.



Transactional variables significantly increases complexity of this patch,
now is simple, because we can reset variable on drop variable command.
Maybe I miss some simply implementation, but I spent on it more than few
days. Still, any cooperation are welcome.


"It is simpler to implement this way" is not an answer either, especially 
as you said that it could have been on point 2.



As I do not see any clear answer to my objection about security 
implications, I understand that it is not addressed by this patch.



At the bare minimum, if this feature ever made it as is, I think that a 
clear caveat must be included in the documentation about not using it for 
any security-related purpose.


Also, I'm not really sure how useful such a non-transactional object can 
be for other purposes: the user should take into account that the 
transaction may fail and the value of the session variable be inconsistent 
as a result. Sometimes it may not matter, but if it matters there is no 
easy way around the fact.


--
Fabien.

Re: plan_cache_mode and postgresql.conf.sample

[Michael Paquier]

On Wed, Aug 22, 2018 at 06:26:52PM +1200, Thomas Munro wrote:
> Thanks, pushed.  I removed one tab because it looks like the comments
> in that file are supposed to line up with tabstop=8 (unlike our source
> files which use 4), and this one didn't.  I hope I got that right!

This line now spawns at 87 characters for me, so that's not quite it.  I
think that you should just have put the list into two lines.
--
Michael


signature.asc
Description: PGP signature

Re: plan_cache_mode and postgresql.conf.sample

[Thomas Munro]

On Wed, Aug 22, 2018 at 5:45 PM, David Rowley
 wrote:
> While testing something that I needed to ensure a generic plan was
> being used, during editing postgresql.conf I couldn't quite remember
> the exact spelling of the option to do that.  I think the valid
> options for the setting should be listed in the sample config file.
>
> Patch attached.

Thanks, pushed.  I removed one tab because it looks like the comments
in that file are supposed to line up with tabstop=8 (unlike our source
files which use 4), and this one didn't.  I hope I got that right!

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

Re: JIT compiling with LLVM v12

[Noah Misch]

On Wed, Mar 28, 2018 at 02:27:51PM -0700, Andres Freund wrote:
> For now LLVM is enabled by default when compiled --with-llvm. I'm mildly
> inclined to leave it like that until shortly before the release, and
> then disable it by default (i.e. change the default of jit=off). But I
> think we can make that decision based on experience during the testing
> window. I'm opening an open items entry for that.

I'll vote for jit=on and letting any bugs shake out earlier, but it's not a
strong preference.

I see jit slows the regression tests considerably:

# x86_64, non-assert, w/o llvm
$ for n in 1 2 3; do env time make -C src/bin/pg_upgrade check; done 2>&1 | 
grep elapsed
7.64user 4.24system 0:36.40elapsed 32%CPU (0avgtext+0avgdata 36712maxresident)k
8.09user 4.50system 0:37.71elapsed 33%CPU (0avgtext+0avgdata 36712maxresident)k
7.53user 4.18system 0:36.54elapsed 32%CPU (0avgtext+0avgdata 36712maxresident)k

# x86_64, non-assert, w/  llvm trunk
$ for n in 1 2 3; do env time make -C src/bin/pg_upgrade check; done 2>&1 | 
grep elapsed
9.58user 5.79system 0:49.61elapsed 30%CPU (0avgtext+0avgdata 36712maxresident)k
9.47user 5.92system 0:47.84elapsed 32%CPU (0avgtext+0avgdata 36712maxresident)k
9.09user 5.51system 0:47.94elapsed 30%CPU (0avgtext+0avgdata 36712maxresident)k

# mips32el, assert, w/o llvm (buildfarm member topminnow) [1]
28min install-check-*
35min check-pg_upgrade

# mips32el, assert, w/  llvm 6.0.1 [1]
 63min install-check-*
166min check-pg_upgrade

Regardless of the choice of jit={on|off} default, these numbers tell me that
some or all of jit_*_cost defaults are too low.


[1] The mips32el runs used "nice -+20" and ran on a shared machine.  I include
them to show the trend, but exact figures may be non-reproducible.