Re: hostorder and failover_timeout for libpq

[Michael Paquier]

On Wed, Sep 19, 2018 at 02:26:53PM +0200, Ildar Musin wrote:
> Another approach would be to leave `pg_conn->connhost` as it is now (i.e.
> not to create global addresses array) and just apply random permutations to
> it if `hostorder=random` is specified. And probably apply permutations to
> addresses list within each individual host.
> 
> At this point I'd like to ask community what in your opinion would be the
> best course of action and whether this feature should be implemented within
> libpq at all? Because from my POV there are factors that really depend on
> network architecture and there is probably no single right solution.

As things stand now, when multiple hosts are defined in a connection
string the order specified in the string is used until a successful
connection is done.  When working on Postgres-XC, we have implemented
similar capability at application-level.  However, now that libpq also
supports multi-host capabilities, I could see a point in having
something within libpq.  What could we get though except a random mode
for read-only or read-write load balancing?  This only use case looks a
bit limited to me to rework again the code paths discarding the
connection failures for that though, as there is as well the argument to
tell the application to generate its own connection string based on
libpq properties.  So my take would be to just do that at
application-level and not bother.

By the way, I can see that the latest patch available does not apply at
tries to juggle with multiple concepts.  I can see at least two of them:
failover_timeout and hostorder.  You should split things.  I have moved
the patch to next CF, waiting on author.
--
Michael


signature.asc
Description: PGP signature

Re: Hash Joins vs. Bloom Filters / take 2

[Michael Paquier]

On Thu, Mar 01, 2018 at 07:04:41PM -0500, David Steele wrote:
> After reviewing the thread I also agree that this should be pushed to
> 2018-09, so I have done so.
> 
> I'm very excited by this patch, though.  In general I agree with Peter that
> a higher rate of false positives is acceptable to save memory.  I also don't
> see any reason why this can't be tuned with a parameter. Start with a
> conservative default and allow the user to adjust as desired.

Not much has happened since last March.  The patch has conflicts in
regression tests.  Thomas, you are registered as a reviewer of this
patch.  Are you planning to look at it?

This is moved to next CF, waiting on author per the rotten bits.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] SERIALIZABLE with parallel query

[Michael Paquier]

On Wed, Sep 19, 2018 at 04:50:40PM -0500, Kevin Grittner wrote:
> I will spend a few more days in testing and review, but figured I
> should pass along "first impressions" now.

Kevin, it seems that this patch is pending on your input.  I have moved
this patch to next CF for now.
--
Michael


signature.asc
Description: PGP signature

Re: Relations being opened without any lock whatever

[Amit Langote]

On 2018/10/01 4:20, Tom Lane wrote:
> Running the regression tests with the patch I showed in
> https://www.postgresql.org/message-id/16565.1538327...@sss.pgh.pa.us
> exposes two places where HEAD is opening relations without having
> any lock at all on them:

Maybe you've noticed but the relation_open calls coming from bootstrap.c
all pass NoLock which trigger the WARNING:

$ initdb -D /tmp/foo

WARNING:  relation_open: no lock held on pg_type
WARNING:  relation_open: no lock held on pg_attrdef
WARNING:  relation_open: no lock held on pg_constraint
WARNING:  relation_open: no lock held on pg_inherits
WARNING:  relation_open: no lock held on pg_index
WARNING:  relation_open: no lock held on pg_operator
WARNING:  relation_open: no lock held on pg_opfamily


Do we need to do something about that, like teaching boot_openrel() and
gettype() in bootstrap.c to pass AccessShareLock instead of NoLock?

Thanks,
Amit

Re: partition tree inspection functions

[Amit Langote]

On 2018/10/01 15:27, Michael Paquier wrote:
> On Mon, Oct 01, 2018 at 03:16:32PM +0900, Amit Langote wrote:
>> I wasn't able to respond to some of issues that Jesper brought up with the
>> approach taken by the latest patch whereby there is no separate
>> pg_partition_level function.  He said that such a function would be useful
>> to get the information about the individual leaf partitions, but I was no
>> longer sure of providing such a function separately.
> 
> Perhaps that could be debated separately as well?  From what I can see
> what's available would unlock the psql patch which would like to add
> support for \dP, or show the size of partitions more easily.

Yeah, maybe there is no reason to delay proceeding with
pg_partition_children which provides a useful functionality.

> I am also
> not completely sure that I see the use-case for pg_partition_level or
> even pg_partition_root_parent as usually in their schemas users append
> rather similar relation names to the parent and the children.  Or
> perhaps not?

We can continue discussing that once we're done dealing with
pg_partition_children and then some other patches that are pending due to
it such as Pavel's.

Thanks,
Amit

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

[Lukas Fittl]

On Tue, Sep 25, 2018 at 1:17 PM, Andres Freund  wrote:
>
> I've pushed the change without that bit - it's just a few additional
> lines if we want to change that.
>

It seems that since this commit, JIT statistics are now only being printed
for parallel query plans. This is due to ExplainPrintJIT being called
before ExecutorEnd, so in a non-parallel query,
queryDesc->estate->es_jit_combined_instr will never get set.

Attached a patch that instead introduces a new ExplainPrintJITSummary
method that summarizes the statistics before they get printed.

I've also removed an (I believe) unnecessary "if (estate->es_instrument)"
check that prevented EXPLAIN without ANALYZE from showing whether JIT would
be used or not.

In addition this also updates a missed section in the documentation with
the new stats output format.

Best,
Lukas

-- 
Lukas Fittl


fix-jit-statistics-in-EXPLAIN-for-non-parallel-queries-v1.patch
Description: Binary data

[PROPOSAL]a new data type 'bytea' for ECPG

[Matsumura, Ryo]

Hi, Hackers

# This is my first post.

I will try to implement a new data type 'bytea' for ECPG.
I think that the implementation is not complicated.
Does anyone need it ?


* Why do I need bytea ?

Currently, ECPG program can treat binary data for bytea column with 'char' type
of C language, but it must convert from/to escaped format with PQunescapeBytea/
PQescapeBytea(). It forces users to add an unnecessary code and to pay cost for
the conversion in runtime.
# My PoC will not be able to solve output conversion cost.

I think that set/put data for host variable should be more simple.
The following is an example of Oracle Pro *C program for RAW type column.

  VARCHAR   raw_data[20];

  /* preprocessed to the following
   * struct 
   * { 
   *unsigned short  len; 
   *unsigned char   arr[20]; 
   * } raw_data;
   */

  raw_data.len = 10;
  memcpy(raw_data.arr, data, 10);

  see also:
  https://docs.oracle.com/cd/E11882_01/appdev.112/e10825/pc_04dat.htm#i23305

In ECPG, varchar host variable cannot be used for bytea because it cannot treat
'\0' as part of data. If the length is set to 10 and there is '\0' at 3rd byte,
ecpglib truncates 3rd byte and later at the following:

  [src/interfaces/ecpg/ecpglib/execute.c]
  ecpg_store_input(const int lineno, const bool force_indicator, const struct
  :
  switch (var->type)
  :
case ECPGt_varchar:
  if (!(newcopy = (char *) ecpg_alloc(variable->len + 1, lineno)))
return false;
  !!  strncpy(newcopy, variable->arr, variable->len);
  newcopy[variable->len] = '\0';

I also think that the behavior of varchar host variable should not be changed
because of compatibility.
Therefore, a new type of host variable 'bytea' is needed.

Since ecpglib can distinguish between C string and binary data, it can send
binary data to backend directly by using 'paramFormats' argument of 
PQexecParams().
Unfortunately, the conversion of output data cannot be omitted in ecpglib 
because
libpq doesn't provide like 'paramFormats'.
 ('resultFormat' means that *all* data from backend is formatted by binary or 
not.)

  PQexecParams(PGconn *conn,
 const char *command,
 int nParams,
 const Oid *paramTypes,
 const char *const *paramValues,
 const int *paramLengths,
  !! const int *paramFormats,
 int resultFormat)



* How to use new 'bytea' ?

ECPG programmers can use almost same as 'varchar' but cannot use as names.
(e.g. connection name, prepared statement name, cursor name and so on)

 - Can use in Declare Section.

  exec sql begin declare section;
bytea data1[512];
bytea data2[DATA_SIZE];   /* can use macro */
bytea send_data[DATA_NUM][DATA_SIZE];  /* can use two dimensional array */
bytea recv_data[][DATA_SIZE]; /* can use flexible array */
  exec sql end declare section;

 - Can *not* use for name.

  exec sql begin declare section;
bytea conn_name[DATA_SIZE];
  exec sql end declare section;

  exec sql connect to :conn_name;   !! error

 - Conversion is not needed in user program.

  exec sql begin declare section;
  bytea send_buf[DATA_SIZE];
  bytea recv_buf[DATA_SIZE - 13];
  int ind_recv;
  exec sql end declare section;

  exec sql create table test (data1 bytea);
  exec sql truncate test;
  exec sql insert into test (data1) values (:send_buf);
  exec sql select data1 into :recv_buf:ind_recv from test;
  /* ind_recv is set to 13. */



* How to preprocess 'bytea' ?

  'bytea' is preprocessed almost same as varchar.
  The following is preprocessed to the next.

exec sql begin declare section;
  bytea data[DATA_SIZE];
  bytea send_data[DATA_NUM][DATA_SIZE];
  bytea recv_data[][DATA_SIZE];
exec sql end declare section;

struct bytea_1 {int len; char arr[DATA_SIZE]} data; 
struct bytea_2 {int len; char arr[DATA_SIZE]} send_data[DATA_NUM]; 
struct bytea_3 {int len; char arr[DATA_SIZE]} *recv_data;


Thank you for your consideration.


Regards
Ryo Matsumura

Re: [PROPOSAL] Shared Ispell dictionaries

[Arthur Zakirov]

On Thu, Jun 14, 2018 at 11:40:17AM +0300, Arthur Zakirov wrote:
> I attached new version of the patch.

The patch still applies to HEAD. I moved it to the next commitfest.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: executor relation handling

[Amit Langote]

On 2018/10/01 2:18, Tom Lane wrote:
> I wrote:
>> 1. You set up transformRuleStmt to insert AccessExclusiveLock into
>> the "OLD" and "NEW" RTEs for a view.  This is surely wrong; we do
>> not want to take exclusive lock on a view just to run a query using
>> the view.  It should (usually, anyway) just be AccessShareLock.
>> However, because addRangeTableEntryForRelation insists that you
>> hold the requested lock type *now*, just changing the parameter
>> to AccessShareLock doesn't work.
>> I hacked around this for the moment by passing NoLock to
>> addRangeTableEntryForRelation and then changing rte->lockmode
>> after it returns, but man that's ugly.  It makes me wonder whether
>> addRangeTableEntryForRelation should be checking the lockmode at all.
> 
> It occurred to me that it'd be reasonable to insist that the caller
> holds a lock *at least as strong* as the one being recorded in the RTE,
> and that there's also been discussions about verifying that some lock
> is held when something like heap_open(foo, NoLock) is attempted.
> So I dusted off the part of 0001 that did that, producing the
> attached delta patch.
> 
> Unfortunately, I can't commit this, because it exposes at least two
> pre-existing bugs :-(.  So we'll need to fix those first, which seems
> like it should be a separate thread.  I'm just parking this here for
> the moment.
> 
> I think that the call sites should ultimately look like
> 
>   Assert(CheckRelationLockedByMe(...));
> 
> but for hunting down the places where the assertion currently fails,
> it's more convenient if it's just an elog(WARNING).

Should this check that we're not in a parallel worker process?

Thanks,
Amit

New vacuum option to do only freezing

[Masahiko Sawada]

Hi,

Attached patch adds a new option FREEZE_ONLY to VACUUM command. This
option is same as FREEZE option except for it disables reclaiming dead
tuples. That is, with this option vacuum does pruning HOT chain,
freezing live tuples and maintaining both visibility map and freespace
map but does not collect dead tuples and invoke neither heap vacuum
nor index vacuum. This option will be useful if user wants to prevent
XID wraparound a table as quick as possible, especially when table is
quite large and is about to XID wraparound. I think this usecase was
mentioned in some threads but I couldn't find them.

Currently this patch just adds the new option to VACUUM command but it
might be good to make autovacuum use it when emergency vacuum is
required.

This is a performance-test result for FREEZE option and FREEZE_ONLY
option. I've tested them on the table which is about 3.8GB table
without indexes and randomly modified.

* FREEZE
INFO:  aggressively vacuuming "public.pgbench_accounts"
INFO:  "pgbench_accounts": removed 5 row versions in 8 pages
INFO:  "pgbench_accounts": found 5 removable, 3000 nonremovable
row versions in 491804 out of 491804 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 722
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
CPU: user: 4.20 s, system: 16.47 s, elapsed: 50.28 s.
VACUUM
Time: 50301.262 ms (00:50.301)

* FREEZE_ONLY
INFO:  aggressively vacuuming "public.pgbench_accounts"
INFO:  "pgbench_accounts": found 4 removable, 3000 nonremovable
row versions in 491804 out of 491804 pages
DETAIL:  freeze 3000 rows
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
CPU: user: 3.10 s, system: 14.85 s, elapsed: 44.56 s.
VACUUM
Time: 44589.794 ms (00:44.590)

Feedback is very welcome.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
From 14c870c263a58cb7b39151e618e1729395e8e354 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada 
Date: Fri, 28 Sep 2018 15:38:05 +0900
Subject: [PATCH v1] Add FREEZE_ONLY option to VACUUM command.

---
 doc/src/sgml/ref/vacuum.sgml | 19 
 src/backend/access/heap/pruneheap.c  | 49 +
 src/backend/commands/vacuum.c|  9 ++--
 src/backend/commands/vacuumlazy.c| 84 +++-
 src/backend/parser/gram.y|  2 +
 src/include/access/heapam.h  |  3 +-
 src/include/nodes/parsenodes.h   |  3 +-
 src/test/regress/expected/vacuum.out |  1 +
 src/test/regress/sql/vacuum.sql  |  1 +
 9 files changed, 137 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index b760e8e..c5560df 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -28,6 +28,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ 
 

+FREEZE_ONLY
+
+ 
+  VACUUM removes dead tuples and prunes HOT-updated
+  tuples chain for live tuples. If the table has any dead tuple it removes
+  them on both table and its indexes.
+  This option is same as FREEZE except for it disables
+  removing dead tuples, so it does only freezing tuples and pruning
+  HOT-updated tuple chains. This is intended to be used when necessary to
+  prevent transaction ID wraparound as quick as possible.
+  Aggressive freezing is always performed when the
+  table is rewritten, so this option is redundant when FULL
+  is specified.
+ 
+
+   
+
+   
 VERBOSE
 
  
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index c2f5343..867d840 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -35,6 +35,7 @@ typedef struct
 	int			nredirected;	/* numbers of entries in arrays below */
 	int			ndead;
 	int			nunused;
+	bool		prune_root;		/* do we prune whole chain or root item? */
 	/* arrays that accumulate indexes of items to be changed */
 	OffsetNumber redirected[MaxHeapTuplesPerPage * 2];
 	OffsetNumber nowdead[MaxHeapTuplesPerPage];
@@ -53,6 +54,8 @@ static void heap_prune_record_redirect(PruneState *prstate,
 		   OffsetNumber offnum, OffsetNumber rdoffnum);
 static void heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum);
 static void heap_prune_record_unused(PruneState *prstate, OffsetNumber offnum);
+static void heap_prune_advance_latest_removed_xid(PruneState *prstate, Page page,
+  OffsetNumber offnum);
 
 
 /*
@@ -152,7 +155,8 @@ heap_page_prune_opt(Relation relation, Buffer buffer)
 			 * needed */
 
 			/* OK to prune */
-			(void) heap_page_prune(relation, buffer, OldestXmin, true, &ignore);
+			(void) heap_page_prune(relation, buffer, OldestXmin, true, &ignore,
+   true);
 		}
 
 		/* And release buffer lock */
@@ -179,7 +183,8 @@ heap_page_prune_opt(Rel

Re: de-deduplicate code in DML execution hooks in postgres_fdw

[Michael Paquier]

On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
> I used perform instead of execute since the later is usually
> associated with local operation. I added "foreign" in the name of the
> function to indicate that it's executed on foreign server. I am happy
> with "remote" as well. I don't think "one" and "single" make any
> difference. I don't like "parameterized" since that gets too tied to
> the method we are using rather than what's actually being done. In
> short I don't find any of the suggestions to be significantly better
> or worse than the name I have chosen. Said that, I am not wedded to
> any of those. A committer is free to choose anything s/he likes.

Fujita-san, you are registered as a reviewer of this patch.  Are you
planning to look at it soon?  It looks useful to me to rework this code
anyway to help with future maintenance, so in the worst case I could
always look at it.  For now I have moved it to the next commit fest.
--
Michael


signature.asc
Description: PGP signature

Re: Hash Joins vs. Bloom Filters / take 2

[Tomas Vondra]

On 10/01/2018 09:15 AM, Michael Paquier wrote:

On Thu, Mar 01, 2018 at 07:04:41PM -0500, David Steele wrote:

After reviewing the thread I also agree that this should be pushed to
2018-09, so I have done so.

I'm very excited by this patch, though.  In general I agree with Peter that
a higher rate of false positives is acceptable to save memory.  I also don't
see any reason why this can't be tuned with a parameter. Start with a
conservative default and allow the user to adjust as desired.


Not much has happened since last March.  The patch has conflicts in
regression tests.  Thomas, you are registered as a reviewer of this
patch.  Are you planning to look at it?

This is moved to next CF, waiting on author per the rotten bits.
--


I don't expect to work on this anytime soon, so maybe mark it as 
returned with feedback instead of moving it to the next CF.


I think the idea to push down the bloom filter to the other side of the 
hash join is what would make it much more interesting than the simple 
approach in this patch - but it's also much more work to make it work.


regards

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

Re: pgbench - add pseudo-random permutation function

[Fabien COELHO]

PostgreSQL Hackers 
Subject: Re: pgbench - add pseudo-random permutation function

On Wed, Sep 26, 2018 at 01:20:49PM +0200, Fabien COELHO wrote:

So, am I right to deducing that you are satisfied with the current status of
the patch, with the nbits implementation either based on popcount (v4) or
clz (v5) compiler intrinsics? I think that the clz option is better.


Fabien, please note that v5 does not apply,


Indeed, tests interact with 92a0342 committed on Thursday.

Here is a rebase of the latest version based on CLZ. According to basic 
test I made, the underlying hardware instruction seems to be more often 
available.



so I moved this patch to next CF, waiting on author.


I'm going to change its state to "Needs review".

--
Fabien.diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index fb58c94d4b..e3cff88bc2 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -312,6 +312,26 @@ fi])# PGAC_C_BUILTIN_BSWAP64
 
 
 
+# PGAC_C_BUILTIN_CLZLL
+# -
+# Check if the C compiler understands __builtin_clzll(),
+# and define HAVE__BUILTIN_CLZLL if so.
+# Both GCC & CLANG seem to have one.
+AC_DEFUN([PGAC_C_BUILTIN_CLZLL],
+[AC_CACHE_CHECK(for __builtin_clzll, pgac_cv__builtin_clzll,
+[AC_COMPILE_IFELSE([AC_LANG_SOURCE(
+[static unsigned long int x = __builtin_clzll(0xaabbccddeeff0011);]
+)],
+[pgac_cv__builtin_clzll=yes],
+[pgac_cv__builtin_clzll=no])])
+if test x"$pgac_cv__builtin_clzll" = xyes ; then
+AC_DEFINE(HAVE__BUILTIN_CLZLL, 1,
+  [Define to 1 if your compiler understands __builtin_clzll.])
+fi])# PGAC_C_BUILTIN_CLZLL
+
+
+
+
 # PGAC_C_BUILTIN_CONSTANT_P
 # -
 # Check if the C compiler understands __builtin_constant_p(),
diff --git a/configure b/configure
index 6414ec1ea6..0f6a549a41 100755
--- a/configure
+++ b/configure
@@ -13921,6 +13921,30 @@ if test x"$pgac_cv__builtin_bswap64" = xyes ; then
 
 $as_echo "#define HAVE__BUILTIN_BSWAP64 1" >>confdefs.h
 
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_clzll" >&5
+$as_echo_n "checking for __builtin_clzll... " >&6; }
+if ${pgac_cv__builtin_clzll+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+static unsigned long int x = __builtin_clzll(0xaabbccddeeff0011);
+
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv__builtin_clzll=yes
+else
+  pgac_cv__builtin_clzll=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv__builtin_clzll" >&5
+$as_echo "$pgac_cv__builtin_clzll" >&6; }
+if test x"$pgac_cv__builtin_clzll" = xyes ; then
+
+$as_echo "#define HAVE__BUILTIN_CLZLL 1" >>confdefs.h
+
 fi
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_constant_p" >&5
 $as_echo_n "checking for __builtin_constant_p... " >&6; }
diff --git a/configure.in b/configure.in
index 158d5a1ac8..231cf27cfb 100644
--- a/configure.in
+++ b/configure.in
@@ -1458,6 +1458,7 @@ PGAC_C_TYPES_COMPATIBLE
 PGAC_C_BUILTIN_BSWAP16
 PGAC_C_BUILTIN_BSWAP32
 PGAC_C_BUILTIN_BSWAP64
+PGAC_C_BUILTIN_CLZLL
 PGAC_C_BUILTIN_CONSTANT_P
 PGAC_C_BUILTIN_UNREACHABLE
 PGAC_C_COMPUTED_GOTO
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 8c464c9d42..265a91df1d 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -917,7 +917,7 @@ pgbench  options  d
 
   
 default_seed 
-   seed used in hash functions by default
+   seed used in hash and pseudo-random permutation functions by default
   
 
   
@@ -1377,6 +1377,13 @@ pgbench  options  d
pow(2.0, 10), power(2.0, 10)
1024.0
   
+  
+   pr_perm(i, size [, seed ] )
+   integer
+   pseudo-random permutation in [0,size)
+   pr_perm(0, 4)
+   0, 1, 2 or 3
+  
   
random(lb, ub)
integer
@@ -1538,6 +1545,24 @@ f(x) = PHI(2.0 * parameter * (x - mu) / (max - min + 1)) /
 
   
 
+  
+Function pr_perm implements a pseudo-random permutation.
+It allows to mix the output of non uniform random functions so that
+values drawn more often are not trivially correlated.
+It permutes integers in [0, size) using a seed by applying rounds of
+simple invertible functions, similarly to an encryption function,
+although beware that it is not at all cryptographically secure.
+Compared to hash functions discussed above, the function
+ensures that a perfect permutation is applied: there are no collisions
+nor holes in the output values.
+Values outside the interval are interpreted modulo the size.
+The function errors if size is not positive.
+If no seed is provided, :default_seed is used.
+For a given size and seed, the function is fully deterministic: if two
+permutations on the same size must not be correlated, use distinct seeds
+as outlined in the previous example about hash functions.
+  
+
   
  

Re: Function for listing archive_status directory

[Christoph Moench-Tegeder]

## Michael Paquier (mich...@paquier.xyz):

> Okay, could you add this patch to the next commit fest?  Here it is:
> https://commitfest.postgresql.org/20/

And here's the patch: https://commitfest.postgresql.org/20/1813/

Regards,
Christoph

-- 
Spare Space

Re: Proposal for Signal Detection Refactoring

[Chris Travers]

On Wed, Sep 26, 2018 at 9:54 AM Chris Travers 
wrote:

>
>
> On Tue, Sep 25, 2018 at 3:23 PM Tom Lane  wrote:
>
>> Chris Travers  writes:
>> > However,  what I think one could do is use a struct of volatile
>> > sig_atomic_t members and macros for checking/setting.  Simply writing a
>> > value is safe in C89 and higher.
>>
>> Yeah, we could group those flags in a struct, but what's the point?
>>
>
> This was one of two things I noticed in my previous patch on interrupts
> and loops where I wasn't sure what the best practice in our code is.
>
> If we don't want to make this change, then would there be any objection to
> me writing up a README describing the flags, and best practices in terms of
> checking them in our code based on the current places we use them?  If the
> current approach will be unlikely to change in the future, then at least we
> can document that the way I went about things is consistent with current
> best practices so next time someone doesn't really wonder.
>

Attaching a first draft of a readme.  Feedback welcome.  I noticed further
that we used to document signals and what they did with carious processes
but that this was removed after 7.0, probably due to complexity reasons.

>
>
>>
>> regards, tom lane
>>
>
>
> --
> Best Regards,
> Chris Travers
> Head of Database
>
> Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
> Saarbrücker Straße 37a, 10405 Berlin
>
>

-- 
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin


sig_doc_patch.patch
Description: Binary data

Re: Hint to set owner for tablespace directory

[Peter Eisentraut]

On 24/09/2018 14:50, Peter Eisentraut wrote:
> On 11/09/2018 17:10, Peter Eisentraut wrote:
>> On 07/09/2018 17:59, Maksim Milyutin wrote:
>>> those directories was that user). The error message "could not set 
>>> permissions on directory ..." disoriented that user. The need to change 
>>> the owner of those directories came after careful reading of 
>>> documentation. I think it would be helpful to show the proposed hint to 
>>> more operatively resolve the problem.
>>
>> I think it might be worth clarifying the documentation instead.  I'm
>> looking at the CREATE TABLESPACE reference page and it's not super clear
>> on first reading.
> 
> How about the attached patch?

I have committed this patch and will close this commitfest item.  I
don't think the idea with the hints in the code was going anywhere.

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

Re: SSL tests failing with "ee key too small" error on Debian SID

[Kyotaro HORIGUCHI]

At Tue, 25 Sep 2018 14:26:42 +0900, Michael Paquier  wrote 
in <20180925052642.gj1...@paquier.xyz>
> On Tue, Sep 25, 2018 at 12:48:57PM +0900, Kyotaro HORIGUCHI wrote:
> > Do you mean that cert/key files are generated on-the-fly while
> > running 'make check'?  It sounds reasonable as long as just
> > replaceing existing files with those with longer (2048bits?) keys
> > doesn't work for all supported platforms.
> 
> The files are present by default in the tree, but can be regenerated
> easily by using the makefile rule "sslfiles".  From what I can see, this
> is caused by OpenSSL 1.1.1 which Debian SID has visibly upgraded to
> recently.  That's the version I have on my system.  I have not dug much
> into the Makefile to see if things could get done right and change the
> openssl commands though..

# I have no experience in Debian..

In Debian /etc/ssl/openssl.cnf has been changed to
"CiperString=DEFAULT@SECLEVEL=2", which implies that "RSA and DHE
keys need to be at least 2048 bit long" according to the
following page.

https://wiki.debian.org/ContinuousIntegration/TriagingTips/openssl-1.1.1

It seems to be Debian's special feature and I suppose
(differently from the previous mail..) it won't happen on other
platforms.

Instead, I managed to cause "ee key too smal" by setting
ssl_ciphers in postgresql.conf as the follows with openssl
1.1.1. With the first attached it happens during
001_ssltests_master.

ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL@SECLEVEL=2' # allowed SSL ciphers

The attached second patch just changes key size to 2048 bits and
"ee key too small" are eliminated in 001_ssltests_master, but
instead I got "ca md too weak" error. This is eliminated by using
sha256 instead of sha1 in cas.config. (third attached)


By the way I got (with both 1.0.2k and 1.1.1) a "tlsv1 alert
unknown ca" error from 002_scram.pl. It is fixed for me by the
forth attached, but I'm not sure why we haven't have such a
complain. (It happens only for me?)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 2b875a3c95..6d267f994e 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -43,6 +43,10 @@ chmod 0644, "ssl/client_wrongperms_tmp.key";
 note "setting up data directory";
 my $node = get_new_node('master');
 $node->init;
+ # restrict cipher suites
+$node->append_conf("postgresql.conf", <<'EOF');
+ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL@SECLEVEL=2' # allowed SSL ciphers
+EOF
 
 # PGHOST is enforced here to set up the node, subsequent connections
 # will use a dedicated connection string.
diff --git a/src/test/ssl/Makefile b/src/test/ssl/Makefile
index 97389c90f8..4b621e18b6 100644
--- a/src/test/ssl/Makefile
+++ b/src/test/ssl/Makefile
@@ -39,7 +39,7 @@ ssl/new_certs_dir:
 
 # Rule for creating private/public key pairs.
 ssl/%.key:
-	openssl genrsa -out $@ 1024
+	openssl genrsa -out $@ 2048
 	chmod 0600 $@
 
 # Root CA certificate
diff --git a/src/test/ssl/cas.config b/src/test/ssl/cas.config
index 013cebae16..8c0ef6d82b 100644
--- a/src/test/ssl/cas.config
+++ b/src/test/ssl/cas.config
@@ -13,7 +13,7 @@ basicConstraints = CA:true
 dir = ./ssl/
 database = ./ssl/root_ca-certindex
 serial = ./ssl/root_ca.srl
-default_md = sha1
+default_md = sha256
 default_days= 1
 default_crl_days= 1
 certificate = ./ssl/root_ca.crt
@@ -26,7 +26,7 @@ email_in_dn= no
 [ server_ca ]
 dir = ./ssl/
 database = ./ssl/server_ca-certindex
-default_md = sha1
+default_md = sha256
 default_days= 1
 default_crl_days= 1
 certificate = ./ssl/server_ca.crt
@@ -42,7 +42,7 @@ crl = ./ssl/server.crl
 [ client_ca ]
 dir = ./ssl/
 database = ./ssl/client_ca-certindex
-default_md = sha1
+default_md = sha256
 default_days= 1
 default_crl_days= 1
 certificate = ./ssl/client_ca.crt
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index b460a7fa8a..147f51783d 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -39,7 +39,7 @@ configure_test_server_for_ssl($node, $SERVERHOSTADDR, "scram-sha-256",
 switch_server_cert($node, 'server-cn-only');
 $ENV{PGPASSWORD} = "pass";
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslmode=require hostaddr=$SERVERHOSTADDR";
+  "user=ssltestuser dbname=trustdb sslmode=require sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
 
 # Default settings
 test_connect_ok($common_connstr, '',

Re: SQL/JSON: documentation

[Liudmila Mantrova]

On 09/28/2018 08:29 PM, Peter Eisentraut wrote:

On 28/06/2018 01:36, Nikita Glukhov wrote:

Attached patch with draft of SQL/JSON documentation written by
Liudmila Mantrova, Oleg Bartunov and me.

Also it can be found in our sqljson repository on sqljson_doc branch:
https://github.com/postgrespro/sqljson/tree/sqljson_doc

We continue to work on it.

Some structural comments:

- I don't think this should be moved to a separate file.  Yes, func.sgml
is pretty big, but if we're going to split it up, we should do it in a
systematic way, not just one section.

- The refentries are not a bad idea, but again, if we just used them for
this one section, the navigation will behave weirdly.  So I'd do it
without them, just using normal subsections.

- Stick to one-space indentation in XML.


Hi Peter,

Thanks for your comments! I'm OK with keeping all reference information 
in func.sgml and will rework it as you suggest. While refentries are 
dear to my heart, let's use subsections for now for the sake of 
consistency. We'll continue working with Nikita and Oleg to improve the 
content before we resend an updated patch; I believe we might still need 
a separate source file if we end up having a separate chapter with usage 
examples and implementation details.


--
Liudmila Mantrova
Technical writer at Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 1 Oct 2018, at 01:19, Michael Paquier  wrote:
> 
> On Sun, Sep 30, 2018 at 10:51:44PM +0900, Michael Paquier wrote:
>> You could have chosen something less complicated, like "ホゲ", which is
>> the equivalent of "foo" in English.  Anyway, non-ASCII characters should
>> not be included in the final patch.

Fixed in the attached v16 revision.

> Looking at the refactoring patch 0001, wouldn't signalfuncs.c make a
> better name for the new file?  There are already multiple examples of
> this type, like logicalfuncs.c, slotfuncs.c, etc.

I have no strong feelings on this, I was merely using the name that Alvaro
suggested when he brought up the refactoring as an extension of this patch.
signalfuncs.c is fine by me, so I did this rename in the attached revision.

> I have moved this patch set to the next commit fest for now.

Thanks.

cheers ./daniel



0001-Refactor-backend-signalling-code-v16.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v16.patch
Description: Binary data

Re: Hint to set owner for tablespace directory

[Maksim Milyutin]

01.10.2018 15:15, Peter Eisentraut wrote:


On 24/09/2018 14:50, Peter Eisentraut wrote:

On 11/09/2018 17:10, Peter Eisentraut wrote:

On 07/09/2018 17:59, Maksim Milyutin wrote:

those directories was that user). The error message "could not set
permissions on directory ..." disoriented that user. The need to change
the owner of those directories came after careful reading of
documentation. I think it would be helpful to show the proposed hint to
more operatively resolve the problem.

I think it might be worth clarifying the documentation instead.  I'm
looking at the CREATE TABLESPACE reference page and it's not super clear
on first reading.

How about the attached patch?

I have committed this patch and will close this commitfest item.  I
don't think the idea with the hints in the code was going anywhere.



Yes, thanks. Sorry for so delayed answer, I was busy. You have clearly 
described the workflow to create tablespace (including *chown* command) 
and in general it is enough to resolve the issue with  wrong owner of 
tablespace directory.


--
Regards,
Maksim Milyutin

Re: de-deduplicate code in DML execution hooks in postgres_fdw

[Etsuro Fujita]

(2018/10/01 19:42), Michael Paquier wrote:

On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:

I used perform instead of execute since the later is usually
associated with local operation. I added "foreign" in the name of the
function to indicate that it's executed on foreign server. I am happy
with "remote" as well. I don't think "one" and "single" make any
difference. I don't like "parameterized" since that gets too tied to
the method we are using rather than what's actually being done. In
short I don't find any of the suggestions to be significantly better
or worse than the name I have chosen. Said that, I am not wedded to
any of those. A committer is free to choose anything s/he likes.


Fujita-san, you are registered as a reviewer of this patch.  Are you
planning to look at it soon?


Yeah, I'm planning to work on this immediately after fixing the issue 
[1], because it still seems to me wise to work on it after addressing 
that issue.  (I'll post an updated version of the patch for that tomorrow.)



It looks useful to me to rework this code
anyway to help with future maintenance,


Agreed.


so in the worst case I could
always look at it.  For now I have moved it to the next commit fest.


Thanks for taking care of this!

Best regards,
Etsuro Fujita

Re: Early WIP/PoC for inlining CTEs

[Andreas Karlsson]

On 07/27/2018 10:10 AM, David Fetter wrote:

On Fri, Jul 27, 2018 at 02:55:26PM +1200, Thomas Munro wrote:

On Thu, Jul 26, 2018 at 7:14 AM, David Fetter  wrote:

Please find attached the next version, which passes 'make check'.


... but not 'make check-world' (contrib/postgres_fdw's EXPLAIN is different).


Please find attached a patch that does.

It doesn't always pass make installcheck-world, but I need to sleep
rather than investigate that at the moment.


I took a quick look at this patch and I have a couple of comments.

1) Is it really safe, for backwards compatibility reasons, to inline 
when there are volatile functions? I imagine that it is possible that 
there are people who rely on that volatile functions inside a CTE are 
always run.


Imagine this case:

WITH cte AS (
  SELECT x, volatile_f(x) FROM tab ORDER BY x
)
SELECT * FROM cte LIMIT 10;

It could change behavior if volatile_f() has side effects and we inline 
the CTE. Is backwards compatibility for cases like this worth 
preserving? People can get the old behavior by adding OFFSET 0 or 
MATERIALIZED, but existing code would break.


2) The code in inline_cte_walker() is quite finicky. It looks correct to 
me but it is hard to follow and some simple bug could easily be hiding 
in there. I wonder if this code could be rewritten in some way to make 
it easier to follow.


3) Can you recall what the failing test was because I have so far not 
managed to reproduce it?


Andreas

Re: Sync ECPG scanner with core

[John Naylor]

On 9/30/18, Michael Paquier  wrote:
> It would be nice to add this patch to the next commit fest:
> https://commitfest.postgresql.org/20/

Done, thanks.

-John Naylor

Re: Oid returned from AddSubscriptionRelState/UpdateSubscriptionRelState

[Petr Jelinek]

Hi,

On 29/09/18 03:35, Andres Freund wrote:
> 
> How come those two functions return oids, even though, as far as I
> understand, the underlying relation is BKI_WITHOUT_OIDS.  I assume
> that's just an oversight?
> 

Yeah, it's copy-pasto that managed to get through, those functions
should return void.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 6ca65097d08244308c264975db4b7b843751d40f Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Mon, 1 Oct 2018 15:19:26 +0200
Subject: [PATCH] Fix return value of subscription-relation mapping
 manipulation functions

These functions manipulate catalog which does not have oids so don't try
to return them from those functions.
---
 src/backend/catalog/pg_subscription.c | 14 +++---
 src/include/catalog/pg_subscription_rel.h |  4 ++--
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index f891ff8054..077701fde0 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -236,13 +236,12 @@ textarray_to_stringlist(ArrayType *textarray)
 /*
  * Add new state record for a subscription table.
  */
-Oid
+void
 AddSubscriptionRelState(Oid subid, Oid relid, char state,
 		XLogRecPtr sublsn)
 {
 	Relation	rel;
 	HeapTuple	tup;
-	Oid			subrelid;
 	bool		nulls[Natts_pg_subscription_rel];
 	Datum		values[Natts_pg_subscription_rel];
 
@@ -272,26 +271,23 @@ AddSubscriptionRelState(Oid subid, Oid relid, char state,
 	tup = heap_form_tuple(RelationGetDescr(rel), values, nulls);
 
 	/* Insert tuple into catalog. */
-	subrelid = CatalogTupleInsert(rel, tup);
+	CatalogTupleInsert(rel, tup);
 
 	heap_freetuple(tup);
 
 	/* Cleanup. */
 	heap_close(rel, NoLock);
-
-	return subrelid;
 }
 
 /*
  * Update the state of a subscription table.
  */
-Oid
+void
 UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
 		   XLogRecPtr sublsn)
 {
 	Relation	rel;
 	HeapTuple	tup;
-	Oid			subrelid;
 	bool		nulls[Natts_pg_subscription_rel];
 	Datum		values[Natts_pg_subscription_rel];
 	bool		replaces[Natts_pg_subscription_rel];
@@ -328,12 +324,8 @@ UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
 	/* Update the catalog. */
 	CatalogTupleUpdate(rel, &tup->t_self, tup);
 
-	subrelid = HeapTupleGetOid(tup);
-
 	/* Cleanup. */
 	heap_close(rel, NoLock);
-
-	return subrelid;
 }
 
 /*
diff --git a/src/include/catalog/pg_subscription_rel.h b/src/include/catalog/pg_subscription_rel.h
index 556cb94841..88f004a8e9 100644
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -67,9 +67,9 @@ typedef struct SubscriptionRelState
 	char		state;
 } SubscriptionRelState;
 
-extern Oid AddSubscriptionRelState(Oid subid, Oid relid, char state,
+extern void AddSubscriptionRelState(Oid subid, Oid relid, char state,
 		XLogRecPtr sublsn);
-extern Oid UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
+extern void UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
 		   XLogRecPtr sublsn);
 extern char GetSubscriptionRelState(Oid subid, Oid relid,
 		XLogRecPtr *sublsn, bool missing_ok);
-- 
2.17.1

Re: Relations being opened without any lock whatever

[Tom Lane]

Amit Langote  writes:
> On 2018/10/01 4:20, Tom Lane wrote:
>> Running the regression tests with the patch I showed in
>> https://www.postgresql.org/message-id/16565.1538327...@sss.pgh.pa.us

> Maybe you've noticed but the relation_open calls coming from bootstrap.c
> all pass NoLock which trigger the WARNING:

Yeah, I'd missed noticing that at the time I posted that patch, but I
sure noticed after changing the WARNING to an Assert ;-)

> Do we need to do something about that, like teaching boot_openrel() and
> gettype() in bootstrap.c to pass AccessShareLock instead of NoLock?

No, bootstrap mode has no need for locking.  I think the right fix is
just to skip the check:
 
+   /*
+* If we didn't get the lock ourselves, assert that caller holds one,
+* except in bootstrap mode where no locks are used.
+*/
+   Assert(lockmode != NoLock ||
+  IsBootstrapProcessingMode() ||
+  CheckRelationLockedByMe(r, AccessShareLock, true));

It's possible that at some point we'd decide to make bootstrap mode
do locking the same as normal mode, but that's not a change I want
to make as part of this patch.

regards, tom lane

Re: executor relation handling

[Tom Lane]

Amit Langote  writes:
> On 2018/09/30 5:04, Tom Lane wrote:
>> 3. There remain some cases where the RTE says RowExclusiveLock but
>> the executor calculation indicates we only need AccessShareLock.
>> AFAICT, this happens only when we have a DO ALSO rule that results
>> in an added query that merely scans the target table.

> I've seen something like that happen for ON CONFLICT's excluded
> pseudo-relation RTE too, because the executor deems only the RTE fetched
> with result relation RT index to require RowExclusiveLock.

OK, I had not carefully inspected every case.

> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
> based on inspecting the query tree to cross-check with rellockmode?  Why
> not just use rellockmode for locking?

Right, that's exactly where we want to end up.  This intermediate state of
the patch is just an attempt to verify that we understand when and how
relying on rellockmode will change the behavior.

> Also, are the discrepancies like this to be
> considered bugs of the existing logic?

Mmm ... hard to say.  In the DO ALSO case, it's possible that query
execution would take AccessShareLock and then RowExclusiveLock, which
at least in principle creates a lock-upgrade deadlock hazard.  So I
think standardizing on taking the rellockmode will be an improvement,
but I don't know that I'd call the existing behavior a bug; I certainly
wouldn't risk trying to back-patch a change for it.

In the ROW_MARK_COPY case, it's conceivable that with the existing code
query re-execution would take only AccessShareLock on a FOR UPDATE target
table, where the parser had originally taken RowShareLock.  Again, it
seems like making the behavior more consistent is an improvement, but
not something we'd try to back-patch.

regards, tom lane

Re: executor relation handling

[Tom Lane]

Amit Langote  writes:
> On 2018/10/01 2:18, Tom Lane wrote:
>> I think that the call sites should ultimately look like
>> Assert(CheckRelationLockedByMe(...));
>> but for hunting down the places where the assertion currently fails,
>> it's more convenient if it's just an elog(WARNING).

> Should this check that we're not in a parallel worker process?

Hmm.  I've not seen any failures in the parallel parts of the regular
regression tests, but maybe I'd better do a force_parallel_mode
run before committing.

In general, I'm not on board with the idea that parallel workers don't
need to get their own locks, so I don't really want to exclude parallel
workers from this check.  But if it's not safe for that today, fixing it
is beyond the scope of this particular patch.

regards, tom lane

Re: automatic restore point

[Peter Eisentraut]

On 01/10/2018 05:34, Alvaro Herrera wrote:
> I don't see it as clear cut as all that ... particularly considering
> that a useful event trigger runs *after* the DDL command in question has
> already written all its WAL, so such a restore point would be completely
> useless.  (Or are ddl_command_start event triggers useful enough?)

The following appears to work:

CREATE FUNCTION evt_automatic_restart_point() RETURNS event_trigger
LANGUAGE plpgsql
AS $$
BEGIN
  PERFORM pg_create_restore_point(tg_tag);
END
$$;

CREATE EVENT TRIGGER automatic_restart_point ON ddl_command_start
EXECUTE PROCEDURE evt_automatic_restart_point();

Are there any concerns about this?

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

Re: Assert failed in snprintf.c

[Tom Lane]

Jaime Casanova  writes:
> sqlsmith made it again, attached is the query (run against regression
> database) that makes the assert fail and the backtrace.
> this happens in head only (or at least 11 is fine).

Ah.  Looks like the has_column_privilege stuff is incautious about whether
it's handed a valid table OID:

regression=# select has_column_privilege(42::oid, 'z'::text, 'q'::text);
server closed the connection unexpectedly

In older branches I get

regression=# select has_column_privilege(42::oid, 'z'::text, 'q'::text);
ERROR:  column "z" of relation "(null)" does not exist

but that's only because glibc's snprintf is forgiving about getting a
NULL pointer for %s.  On some other platforms it'd end in SIGSEGV.

Will fix, thanks for report!

regards, tom lane

Re: Add SKIP LOCKED to VACUUM and ANALYZE

[Bossart, Nathan]

On 9/27/18, 2:52 AM, "Michael Paquier"  wrote:
> Thanks for the new patches.  So I have begun looking at the full set,
> beginning by 0001 which introduces a new common routine to get the
> elevel associated to a skipped relation.  I have been chewing on this
> one, and I think that the patch could do more in terms of refactoring by
> introducing one single routine able to open a relation which is going to
> be vacuumed or analyzed.  This removes quite a lot of duplication
> between analyze_rel() and vacuum_rel() when it comes to using
> try_relation_open().  The result is attached, and that makes the code
> closer to what the recently-added vacuum_is_relation_owner does.
> Nathan, what do you think?

Good idea.  This patch looks good to me.

> Please note that I am on the edge of discarding the stuff in
> find_all_inheritors and such, as well as not worrying about the case of
> analyze which could block for some index columns, but I have not spent
> much time studying this part yet.  Still the potential issues around
> complicating this code, particularly when things come to having a
> partial analyze or vacuum done rather scares me.

Without the find_all_inheritors() stuff, I think we would just need to
modify the ANALYZE documentation patch to say something like

Specifies that ANALYZE should not wait for any conflicting locks
to be released: if a relation cannot be locked immediately without
waiting, the relation is skipped.  Note that even with this
option, ANALYZE can still block when opening the relation's
indexes or when acquiring sample rows to prepare the statistics.
Also, while ANALYZE ordinarily processes all leaf partitions of
partitioned tables, this option will cause ANALYZE to skip all
leaf partitions if there is a conflicting lock on the partitioned
table.

Nathan

Re: pg_ls_tmpdir(); AND, Function for listing archive_status directory

[Justin Pryzby]

On Wed, Sep 26, 2018 at 10:36:03PM +0200, Laurenz Albe wrote:
> Bossart, Nathan wrote:
> > Attached is a patch to add a pg_ls_tmpdir() function that lists the
> > contents of a specified tablespace's pgsql_tmp directory.  This is
> > very similar to the existing pg_ls_logdir() and pg_ls_waldir()
> > functions.

On Sun, Sep 30, 2018 at 10:59:20PM +0200, Christoph Moench-Tegeder wrote:
> while setting up monitoring for a new PostgreSQL instance, I noticed that
> there's no build-in way for a pg_monitor role to check the contents of
> the archive_status directory. We got pg_ls_waldir() in 10, but that
> only lists pg_wal - not it's (sic) subdirectory. 

>It may make sense to have a generic function like
>   pg_ls_dir(dirname text, tablespace oid)
>instead.  But maybe that's taking it too far...

I see Cristoph has another pg_ls_* function, which suggests that Laurenz idea
is good, and a generic pg_ls function is desirable.

Justin

Re: pg_ls_tmpdir()

[Andres Freund]

Hi,

On 2018-09-26 22:36:03 +0200, Laurenz Albe wrote:
> 2. There already are functions "pg_ls_logdir" and "pg_ls_waldir",
>and I can imagine new requests, e.g. pg_ls_datafiles() to list the
>data files in a database directory.
> 
>It may make sense to have a generic function like
> 
>   pg_ls_dir(dirname text, tablespace oid)
> 
>instead.  But maybe that's taking it too far...

There's a generic pg_ls_dir() already - I'm now sure why a generic
function would take the tablespace oid however?

Greetings,

Andres Freund

Re: buildfarm and git pull

[Justin Pryzby]

On Thu, Sep 27, 2018 at 06:32:59PM +0300, Alexander Kuzmenkov wrote:
> It just has to checkout the remote branch as-is.

It doesn't clean files, but I would suggest:
git checkout -B branch remote/branch

Justin

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Mark Wong]

On Sun, Sep 30, 2018 at 12:38:46AM +0100, Andrew Gierth wrote:
> > "Andrew" == Andrew Dunstan  writes:
> 
>  >> What is the size of a C "int" on this platform?
> 
>  Andrew> 4.
> 
> Hmm.
> 
> Because int being more than 32 bits is the simplest explanation for this
> difference.
> 
> How about the output of this query:
> 
> with d(a) as (values ('----'::uuid),
>  ('----'::uuid),
>  ('3f3e3c3b-3a30-3938-3736-353433a2313e'::uuid))
>   select d1.a, d2.a, uuid_cmp(d1.a,d2.a) from d d1, d d2
>order by d1.a asc, d2.a desc;

That also appears to produce the same results:

With 9.4:

postgres=# select version();
  version
---
 PostgreSQL 9.4.19 on s390x-ibm-linux-gnu, compiled by clang version 5.0.1 
(tags/RELEASE_501/final 312548), 64-bit
(1 row)

...

  a   |  a   |  
uuid_cmp
--+--+-
 ---- | ---- |  
 0
 ---- | ---- | 
-2147483648
 ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
-2147483648
 ---- | ---- |  
 1
 ---- | ---- |  
 0
 ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
-2147483648
 3f3e3c3b-3a30-3938-3736-353433a2313e | ---- |  
 1
 3f3e3c3b-3a30-3938-3736-353433a2313e | ---- |  
 1
 3f3e3c3b-3a30-3938-3736-353433a2313e | 3f3e3c3b-3a30-3938-3736-353433a2313e |  
 0
(9 rows)


Then with HEAD:

postgres=# select version();
  version

 PostgreSQL 12devel on s390x-ibm-linux-gnu, compiled by clang version 5.0.1 
(tags/RELEASE_501/final 312548), 64-bit
(1 row)

...

  a   |  a   |  
uuid_cmp
--+--+-
 ---- | ---- |  
 0
 ---- | ---- | 
-2147483648
 ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
-2147483648
 ---- | ---- |  
 1
 ---- | ---- |  
 0
 ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
-2147483648
 3f3e3c3b-3a30-3938-3736-353433a2313e | ---- |  
 1
 3f3e3c3b-3a30-3938-3736-353433a2313e | ---- |  
 1
 3f3e3c3b-3a30-3938-3736-353433a2313e | 3f3e3c3b-3a30-3938-3736-353433a2313e |  
 0
(9 rows)


Regards,
Mark

-- 
Mark Wonghttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Tom Lane]

Mark Wong  writes:
>   a   |  a   
> |  uuid_cmp
> --+--+-
>  ---- | ---- 
> |   0
>  ---- | ---- 
> | -2147483648
>  ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e 
> | -2147483648
>  ---- | ---- 
> |   1
>  ---- | ---- 
> |   0
>  ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e 
> | -2147483648
>  3f3e3c3b-3a30-3938-3736-353433a2313e | ---- 
> |   1
>  3f3e3c3b-3a30-3938-3736-353433a2313e | ---- 
> |   1
>  3f3e3c3b-3a30-3938-3736-353433a2313e | 3f3e3c3b-3a30-3938-3736-353433a2313e 
> |   0
> (9 rows)


Oooh ... apparently, on that platform, memcmp() is willing to produce
INT_MIN in some cases.  That's not a safe value for a sort comparator
to produce --- we explicitly say that somewhere, IIRC.  I think we
implement DESC by negating the comparator's result, which explains
why only the DESC case fails.

regards, tom lane

Re: Proposal for Signal Detection Refactoring

[Chris Travers]

Moving this to documentation due to a general consensus that abstracting this 
is not necessarily worth it.

If we don't want to refactor and abstract this, it is worth documenting the 
design as to how things work now so that others who face bugs can consult docs 
instead of trying to determine acceptable or recommended practices by looking 
at the code alone.

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Andres Freund]

On 2018-10-01 11:58:51 -0400, Tom Lane wrote:
> Mark Wong  writes:
> >   a   |  a  
> >  |  uuid_cmp
> > --+--+-
> >  ---- | 
> > ---- |   0
> >  ---- | 
> > ---- | -2147483648
> >  ---- | 
> > 3f3e3c3b-3a30-3938-3736-353433a2313e | -2147483648
> >  ---- | 
> > ---- |   1
> >  ---- | 
> > ---- |   0
> >  ---- | 
> > 3f3e3c3b-3a30-3938-3736-353433a2313e | -2147483648
> >  3f3e3c3b-3a30-3938-3736-353433a2313e | 
> > ---- |   1
> >  3f3e3c3b-3a30-3938-3736-353433a2313e | 
> > ---- |   1
> >  3f3e3c3b-3a30-3938-3736-353433a2313e | 
> > 3f3e3c3b-3a30-3938-3736-353433a2313e |   0
> > (9 rows)
> 
> 
> Oooh ... apparently, on that platform, memcmp() is willing to produce
> INT_MIN in some cases.  That's not a safe value for a sort comparator
> to produce --- we explicitly say that somewhere, IIRC.

Hm, that'd be pretty painful - memcmp() isn't guaranteed to return
anything smaller. And we use memcmp in a fair number of comparators.


> I think we implement DESC by negating the comparator's result, which
> explains why only the DESC case fails.

That makes sense.

Greetings,

Andres Freund

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Tom Lane]

Andres Freund  writes:
> On 2018-10-01 11:58:51 -0400, Tom Lane wrote:
>> Oooh ... apparently, on that platform, memcmp() is willing to produce
>> INT_MIN in some cases.  That's not a safe value for a sort comparator
>> to produce --- we explicitly say that somewhere, IIRC.

> Hm, that'd be pretty painful - memcmp() isn't guaranteed to return
> anything smaller. And we use memcmp in a fair number of comparators.

Yeah.  So our choices are

(1) Retain the current restriction on what sort comparators can
produce.  Find all the places where memcmp's result is returned
directly, and fix them.  (I wonder if strcmp has same issue.)

(2) Drop the restriction.  This'd require at least changing the
DESC correction, and maybe other things.  I'm not sure what the
odds would be of finding everyplace we need to check.

Neither one is sounding very pleasant, or maintainable.

regards, tom lane

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Andres Freund]

On 2018-10-01 12:13:57 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-10-01 11:58:51 -0400, Tom Lane wrote:
> >> Oooh ... apparently, on that platform, memcmp() is willing to produce
> >> INT_MIN in some cases.  That's not a safe value for a sort comparator
> >> to produce --- we explicitly say that somewhere, IIRC.
> 
> > Hm, that'd be pretty painful - memcmp() isn't guaranteed to return
> > anything smaller. And we use memcmp in a fair number of comparators.
> 
> Yeah.  So our choices are
> 
> (1) Retain the current restriction on what sort comparators can
> produce.  Find all the places where memcmp's result is returned
> directly, and fix them.  (I wonder if strcmp has same issue.)
> 
> (2) Drop the restriction.  This'd require at least changing the
> DESC correction, and maybe other things.  I'm not sure what the
> odds would be of finding everyplace we need to check.
> 
> Neither one is sounding very pleasant, or maintainable.

(2) seems more maintainable to me (or perhaps less unmaintainable). It's
infrastructure, rather than every datatype + support out there...

Greetings,

Andres Freund

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Tom Lane]

Andres Freund  writes:
> On 2018-10-01 12:13:57 -0400, Tom Lane wrote:
>> Yeah.  So our choices are
>> 
>> (1) Retain the current restriction on what sort comparators can
>> produce.  Find all the places where memcmp's result is returned
>> directly, and fix them.  (I wonder if strcmp has same issue.)
>> 
>> (2) Drop the restriction.  This'd require at least changing the
>> DESC correction, and maybe other things.  I'm not sure what the
>> odds would be of finding everyplace we need to check.
>> 
>> Neither one is sounding very pleasant, or maintainable.

> (2) seems more maintainable to me (or perhaps less unmaintainable). It's
> infrastructure, rather than every datatype + support out there...

I guess we could set up some testing infrastructure: hack int4cmp
and/or a couple other popular comparators so that they *always*
return INT_MIN, 0, or INT_MAX, and then see what falls over.

I'm fairly sure that btree, as well as the sort code proper,
has got an issue here.

regards, tom lane

Re: [HACKERS] kqueue

[Matteo Beccati]

Hi Thomas,

On 01/10/2018 01:09, Thomas Munro wrote:

I don't know why the existence of the kqueue should make recvfrom()
slower on the pgbench side.  That's probably something to look into
off-line with some FreeBSD guru help.  Degraded performance for
clients on the same machine does seem to be a show stopper for this
patch for now.  Thanks for testing!


Glad to be helpful!

I've tried running pgbench from a separate VM and in fact kqueue 
consistently takes the lead with 5-10% more tps on select/prepared 
pgbench on NetBSD too.


What I have observed is that sys cpu usage is ~65% (35% idle) with 
kqueue, while unpatched master averages at 55% (45% idle): relatively 
speaking that's almost 25% less idle cpu available for a local pgbench 
to do its own stuff.


Running pgbench locally shows an average 47% usr / 53% sys cpu 
distribution w/ kqueue vs more like 50-50 w/ vanilla, so I'm inclined to 
think that's the reason why we see a performance drop instead. Thoguhts?



Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Re: [HACKERS] kqueue

[Andres Freund]

On 2018-10-01 19:25:45 +0200, Matteo Beccati wrote:
> On 01/10/2018 01:09, Thomas Munro wrote:
> > I don't know why the existence of the kqueue should make recvfrom()
> > slower on the pgbench side.  That's probably something to look into
> > off-line with some FreeBSD guru help.  Degraded performance for
> > clients on the same machine does seem to be a show stopper for this
> > patch for now.  Thanks for testing!
> 
> Glad to be helpful!
> 
> I've tried running pgbench from a separate VM and in fact kqueue
> consistently takes the lead with 5-10% more tps on select/prepared pgbench
> on NetBSD too.
> 
> What I have observed is that sys cpu usage is ~65% (35% idle) with kqueue,
> while unpatched master averages at 55% (45% idle): relatively speaking
> that's almost 25% less idle cpu available for a local pgbench to do its own
> stuff.

This suggest that either the the wakeup logic between kqueue and poll,
or the internal locking could be at issue.  Is it possible that poll
triggers a directed wakeup path, but kqueue doesn't?

Greetings,

Andres Freund

has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Tom Lane]

I wrote:
> Jaime Casanova  writes:
>> sqlsmith made it again, attached is the query (run against regression
>> database) that makes the assert fail and the backtrace.
>> this happens in head only (or at least 11 is fine).

> Ah.  Looks like the has_column_privilege stuff is incautious about whether
> it's handed a valid table OID:
> regression=# select has_column_privilege(42::oid, 'z'::text, 'q'::text);
> server closed the connection unexpectedly

So my first thought was to just throw an error for bad table OID,
as per the attached quick-hack patch.

However, on closer inspection, I wonder whether that's what we really
want.  The rest of the OID-taking have_foo_privilege functions are
designed to return null, not fail, if handed a bad OID.  This is meant to
allow queries scanning system catalogs to not die if an object is
concurrently dropped.  So I think this should do likewise.

But it's not quite clear to me what we want the behavior for bad column
name to be.  A case could be made for either of:

* If either the table OID is bad, or the OID is OK but there's no such
column, return null.

* Return null for bad OID, but if it's OK, continue to throw error
for bad column name.

The second case seems weirdly inconsistent, but it might actually
be the most useful definition.  Not detecting a misspelled column
name is likely to draw complaints.

Thoughts?

regards, tom lane

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index d5285e2..3b963a0 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -2837,10 +2837,20 @@ convert_column_name(Oid tableoid, text *column)
 	colname = text_to_cstring(column);
 	attnum = get_attnum(tableoid, colname);
 	if (attnum == InvalidAttrNumber)
+	{
+		char	   *tablename = get_rel_name(tableoid);
+
+		/* Paranoia is necessary since tableoid could be anything */
+		if (tablename == NULL)
+			ereport(ERROR,
+	(errcode(ERRCODE_UNDEFINED_TABLE),
+	 errmsg("relation with OID %u does not exist",
+			tableoid)));
 		ereport(ERROR,
 (errcode(ERRCODE_UNDEFINED_COLUMN),
  errmsg("column \"%s\" of relation \"%s\" does not exist",
-		colname, get_rel_name(tableoid;
+		colname, tablename)));
+	}
 	pfree(colname);
 	return attnum;
 }

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> I wrote:
> > Jaime Casanova  writes:
> >> sqlsmith made it again, attached is the query (run against regression
> >> database) that makes the assert fail and the backtrace.
> >> this happens in head only (or at least 11 is fine).
> 
> > Ah.  Looks like the has_column_privilege stuff is incautious about whether
> > it's handed a valid table OID:
> > regression=# select has_column_privilege(42::oid, 'z'::text, 'q'::text);
> > server closed the connection unexpectedly
> 
> So my first thought was to just throw an error for bad table OID,
> as per the attached quick-hack patch.
> 
> However, on closer inspection, I wonder whether that's what we really
> want.  The rest of the OID-taking have_foo_privilege functions are
> designed to return null, not fail, if handed a bad OID.  This is meant to
> allow queries scanning system catalogs to not die if an object is
> concurrently dropped.  So I think this should do likewise.

Agreed.

> But it's not quite clear to me what we want the behavior for bad column
> name to be.  A case could be made for either of:
> 
> * If either the table OID is bad, or the OID is OK but there's no such
> column, return null.
> 
> * Return null for bad OID, but if it's OK, continue to throw error
> for bad column name.
> 
> The second case seems weirdly inconsistent, but it might actually
> be the most useful definition.  Not detecting a misspelled column
> name is likely to draw complaints.
> 
> Thoughts?

What are we going to do for dropped columns..?  Seems like with what
you're suggesting we'd throw an error, but that'd make querying with
this function similairly annoying at times.

My inclination would be to make the function return NULL in any case
where we can't find what the user is asking for (and to not throw an
error in general).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> But it's not quite clear to me what we want the behavior for bad column
>> name to be.  A case could be made for either of:
>> 
>> * If either the table OID is bad, or the OID is OK but there's no such
>> column, return null.
>> 
>> * Return null for bad OID, but if it's OK, continue to throw error
>> for bad column name.
>> 
>> The second case seems weirdly inconsistent, but it might actually
>> be the most useful definition.  Not detecting a misspelled column
>> name is likely to draw complaints.
>> 
>> Thoughts?

> What are we going to do for dropped columns..?  Seems like with what
> you're suggesting we'd throw an error, but that'd make querying with
> this function similairly annoying at times.

True, but I think dropping individual columns is much less common
than dropping whole tables.

The general plan in the has_foo_privilege functions is to throw errors for
failing name-based lookups, but return null for failing numerically-based
lookups (object OID or column number).  I'm inclined to think we should
stick to that.  In the case at hand, we'd be supporting queries that
iterate over pg_attribute, but they'd have to pass attnum not attname
to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
error for a typo'ed name is annoying to a different and probably larger
set of users.

regards, tom lane

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Joe Conway]

On 10/01/2018 02:41 PM, Stephen Frost wrote:
> Tom Lane (t...@sss.pgh.pa.us) wrote:
>> But it's not quite clear to me what we want the behavior for bad column
>> name to be.  A case could be made for either of:
>> 
>> * If either the table OID is bad, or the OID is OK but there's no such
>> column, return null.
>> 
>> * Return null for bad OID, but if it's OK, continue to throw error
>> for bad column name.
>> 
>> The second case seems weirdly inconsistent, but it might actually
>> be the most useful definition.  Not detecting a misspelled column
>> name is likely to draw complaints.

Seems you could make the same argument for not detecting a misspelled
table name for this and has_table_privilege...


> My inclination would be to make the function return NULL in any case
> where we can't find what the user is asking for (and to not throw an
> error in general).

+1

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: executor relation handling

[David Rowley]

On 1 October 2018 at 19:39, Amit Langote  wrote:
> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
> based on inspecting the query tree to cross-check with rellockmode?  Why
> not just use rellockmode for locking?  Maybe, okay to keep doing that in
> debug builds though.  Also, are the discrepancies like this to be
> considered bugs of the existing logic?

I got the impression Tom was just leaving that in for a while to let
the buildfarm verify the new code is getting the same lock level as
the old code. Of course, I might be wrong.

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

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> But it's not quite clear to me what we want the behavior for bad column
> >> name to be.  A case could be made for either of:
> >> 
> >> * If either the table OID is bad, or the OID is OK but there's no such
> >> column, return null.
> >> 
> >> * Return null for bad OID, but if it's OK, continue to throw error
> >> for bad column name.
> >> 
> >> The second case seems weirdly inconsistent, but it might actually
> >> be the most useful definition.  Not detecting a misspelled column
> >> name is likely to draw complaints.
> >> 
> >> Thoughts?
> 
> > What are we going to do for dropped columns..?  Seems like with what
> > you're suggesting we'd throw an error, but that'd make querying with
> > this function similairly annoying at times.
> 
> True, but I think dropping individual columns is much less common
> than dropping whole tables.

That certainly doesn't mean that it doesn't happen though, nor does it
mean we don't have to come up with an answer to the question.

> The general plan in the has_foo_privilege functions is to throw errors for
> failing name-based lookups, but return null for failing numerically-based
> lookups (object OID or column number).  I'm inclined to think we should
> stick to that.  In the case at hand, we'd be supporting queries that
> iterate over pg_attribute, but they'd have to pass attnum not attname
> to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
> error for a typo'ed name is annoying to a different and probably larger
> set of users.

... and what's going to happen when they pass in a dropped column,
either via number or name?

I don't have an issue with throwing a failure for name-based lookups but
returning null for failing numerically-based lookups, but I don't really
want us throwing errors on dropped columns.  I would think we'd return
null in that case.  In particular, I can see this function being used in
a where clause across pg_attribute.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> The general plan in the has_foo_privilege functions is to throw errors for
>> failing name-based lookups, but return null for failing numerically-based
>> lookups (object OID or column number).  I'm inclined to think we should
>> stick to that.  In the case at hand, we'd be supporting queries that
>> iterate over pg_attribute, but they'd have to pass attnum not attname
>> to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
>> error for a typo'ed name is annoying to a different and probably larger
>> set of users.

> ... and what's going to happen when they pass in a dropped column,
> either via number or name?

Well, it'll have to fail for the name case, but not for the attnum case.

> I don't have an issue with throwing a failure for name-based lookups but
> returning null for failing numerically-based lookups, but I don't really
> want us throwing errors on dropped columns.  I would think we'd return
> null in that case.

You can't have it both ways.  Either you throw an error if the name's
not there, or you don't.

> In particular, I can see this function being used in
> a where clause across pg_attribute.

As said above, it can work as long as you use attnum not attname.
I don't think this is really so different from iterating across
pg_class (or any other catalog) and passing relname instead of OID.

regards, tom lane

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> The general plan in the has_foo_privilege functions is to throw errors for
> >> failing name-based lookups, but return null for failing numerically-based
> >> lookups (object OID or column number).  I'm inclined to think we should
> >> stick to that.  In the case at hand, we'd be supporting queries that
> >> iterate over pg_attribute, but they'd have to pass attnum not attname
> >> to avoid snapshot-skew failures.  That's a bit annoying, but not throwing
> >> error for a typo'ed name is annoying to a different and probably larger
> >> set of users.
> 
> > ... and what's going to happen when they pass in a dropped column,
> > either via number or name?
> 
> Well, it'll have to fail for the name case, but not for the attnum case.

Why?

> > I don't have an issue with throwing a failure for name-based lookups but
> > returning null for failing numerically-based lookups, but I don't really
> > want us throwing errors on dropped columns.  I would think we'd return
> > null in that case.
> 
> You can't have it both ways.  Either you throw an error if the name's
> not there, or you don't.

I'm not following why we couldn't handle a dropped column differently.

Dropped tables don't hang around in the catalog long after they've been
dropped.

> > In particular, I can see this function being used in
> > a where clause across pg_attribute.
> 
> As said above, it can work as long as you use attnum not attname.
> I don't think this is really so different from iterating across
> pg_class (or any other catalog) and passing relname instead of OID.

If you really insist that we not do something better when it comes to
dropped columns then I'll drop my argument, but I do see dropped columns
as a materially different thing from dropped tables because of how we
keep them around in the catalog.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> You can't have it both ways.  Either you throw an error if the name's
>> not there, or you don't.

> I'm not following why we couldn't handle a dropped column differently.

Different from what?  A name-based lookup isn't going to find a dropped
column, because its attname has been replaced with
"pg.dropped.N"

> Dropped tables don't hang around in the catalog long after they've been
> dropped.

If you are talking about the case where a lookup by attnum finds a dropped
column, that does return null already, cf column_privilege_check().
But I don't see a way for a name-based lookup to do the same without
losing all semblance of error detection.

regards, tom lane

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> You can't have it both ways.  Either you throw an error if the name's
> >> not there, or you don't.
> 
> > I'm not following why we couldn't handle a dropped column differently.
> 
> Different from what?  A name-based lookup isn't going to find a dropped
> column, because its attname has been replaced with
> "pg.dropped.N"

My complaint is specifically trying to do something like:

=*# select *  
from
  pg_class
  join pg_attribute on (pg_class.oid = pg_attribute.attrelid)
  join pg_namespace on (pg_class.relnamespace = pg_namespace.oid)   
   
where   
 
  has_column_privilege(quote_ident(nspname) || '.' || 
quote_ident(relname),attname,'SELECT');

and getting this:

ERROR:  column "pg.dropped.2" of relation "t1" does not exist

> > Dropped tables don't hang around in the catalog long after they've been
> > dropped.
> 
> If you are talking about the case where a lookup by attnum finds a dropped
> column, that does return null already, cf column_privilege_check().
> But I don't see a way for a name-based lookup to do the same without
> losing all semblance of error detection.

Perhaps it's not worth it then, though I still contend that it's pretty
annoying that we throw an error in that case.

Thanks!

Stephen


signature.asc
Description: PGP signature

settings to control SSL/TLS protocol version

[Peter Eisentraut]

There have been some requests to be able to select the TLS versions
PostgreSQL is using.  We currently only hardcode that SSLv2 and SSLv3
are disabled, but there is also some interest now in disabling TLSv1.0
and TLSv1.1.  Also, I've had some issues in some combinations with the
new TLSv1.3, so there is perhaps also some use for disabling at the top end.

Attached is a patch that implements this.  For example:

ssl_min_protocol_version = 'TLSv1'
ssl_max_protocol_version = 'any'

For reference, here is similar functionality implemented elsewhere:

https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslprotocol

Unlike those two, which offer a list of protocols to use, I have gone
with min and max settings.  I think that is easier to use, and it also
maps better to the newer OpenSSL API (SSL_CTX_set_min_proto_version()
etc.).  The older SSL_CTX_set_options()-based approach is deprecated and
has some very weird behaviors that would make it complicated to use for
anything more than a min/max.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 768ccf861b6904baa25a601a09c5e440f3f62cca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 1 Oct 2018 21:43:30 +0200
Subject: [PATCH] Add settings to control SSL/TLS protocol version

For example:

ssl_min_protocol_version = 'TLSv1'
ssl_max_protocol_version = 'any'
---
 doc/src/sgml/config.sgml  |  44 +++
 src/backend/libpq/be-secure-openssl.c | 123 +-
 src/backend/libpq/be-secure.c |   3 +
 src/backend/utils/misc/guc.c  |  33 +
 src/backend/utils/misc/postgresql.conf.sample |   2 +
 src/include/libpq/libpq.h |  11 ++
 6 files changed, 214 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f11b8f724c..efdf8a2849 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1290,6 +1290,50 @@ SSL
   
  
 
+ 
+  ssl_min_protocol_version (enum)
+  
+   ssl_min_protocol_version configuration 
parameter
+  
+  
+  
+   
+Sets the minimum SSL/TLS protocol version to use.  Valid values are
+currently: TLSv1, TLSv1.1,
+TLSv1.2, TLSv1.3.  Older
+versions of the OpenSSL library do not
+support all values.  An error will be raised if an unsupported setting
+is chosen.  Protocol versions before TLS 1.0, namely SSL version 2 and
+3, are always disabled.
+   
+
+   
+The default is TLSv1, mainly to support older
+versions of the OpenSSL library.  You might
+want to set this to a higher value if all software components can
+support the newer protocol versions.
+   
+  
+ 
+
+ 
+  ssl_max_protocol_version (enum)
+  
+   ssl_max_protocol_version configuration 
parameter
+  
+  
+  
+   
+Sets the maximum SSL/TLS protocol version to use.  Valid values are as
+for , with addition of
+any which allows any protocol version.  The default 
is
+any.  Setting the maximum protocol version is
+mainly useful for testing or if some component has issues working with
+a newer protocol.
+   
+  
+ 
+
  
   ssl_dh_params_file (string)
   
diff --git a/src/backend/libpq/be-secure-openssl.c 
b/src/backend/libpq/be-secure-openssl.c
index 6a576572bb..b2b0cccdae 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -67,6 +67,12 @@ static bool SSL_initialized = false;
 static bool dummy_ssl_passwd_cb_called = false;
 static bool ssl_is_server_start;
 
+static int ssl_protocol_version_to_openssl(int v, const char *guc_name);
+#if (OPENSSL_VERSION_NUMBER < 0x1010L)
+static int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
+static int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
+#endif
+
 
 /*  */
 /*  Public interface   
*/
@@ -183,8 +189,14 @@ be_tls_init(bool isServerStart)
goto error;
}
 
-   /* disallow SSL v2/v3 */
-   SSL_CTX_set_options(context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
+   if (ssl_min_protocol_version)
+   SSL_CTX_set_min_proto_version(context,
+ 
ssl_protocol_version_to_openssl(ssl_min_protocol_version,
+   
  
"ssl_min_protocol_version"));
+   if (ssl_max_protocol_version)
+   SSL_CTX_set_max_proto_version(context,
+  

Re: SerializeParamList vs machines with strict alignment

[Tom Lane]

Amit Kapila  writes:
> On Sun, Sep 30, 2018 at 10:44 AM Amit Kapila  wrote:
>> Attached is a patch along these lines, let me know if you have
>> something else in mind?  I have manually verified this with
>> force_parallel_mode=regress configuration in my development
>> environment.  I don't have access to alignment-sensitive hardware, so
>> can't test in such an environment.  I will see if I can write a test
>> as well.

> Attached patch contains a test case as well to cover this code.

This test case doesn't actually trigger the problem.  There are two
reasons: (1) it never invokes datumSerialize with an odd starting
address, and (2) the expanded array produced by the plpgsql function
contains a "flat" array, allowing EA_flatten_into to go through its
"easy" case, which just does a memcpy and so is not sensitive to
misalignment.  To fix (1) we need a previous parameter of odd length,
and to fix (2) we need the plpgsql function to build the array from
parts.  The cast to a domain is unnecessary and might indeed be a
third reason why it doesn't work --- I'd not be very surprised if
that results in array flattening.

The attached revised patch contains a test case that demonstrably triggers
the problem on gaur's host.  Oddly, I do not get a crash either on a PPC
Mac or a Raspberry Pi 3 running Raspbian.  I'm not very sure why; I traced
through things with gdb and it's definitely calling EA_flatten_into with
an odd address and a non-flattened input.  I guess both of those platforms
have kernel handlers for misaligned accesses?  But the Raspbian box ought
to be nearly the same as chipmunk, which is where we saw the problem to
begin with, so I'm a bit confused.

> Kindly suggest what
> is the best path forward, is it a good idea to commit this on HEAD
> first and see if the test passes on all machines in buildfarm and then
> back-patch it?

No, I think you should just commit and back-patch at the same time.
I don't have any doubt about the code patch being good, it's just
a question of whether the test case reliably triggers the problem.
And in the end that's not that important as long as it does so on
at least one buildfarm member.

regards, tom lane

diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c
index f02a5e7..4957682 100644
*** a/src/backend/utils/adt/datum.c
--- b/src/backend/utils/adt/datum.c
*** datumSerialize(Datum value, bool isnull,
*** 338,345 
  		}
  		else if (eoh)
  		{
! 			EOH_flatten_into(eoh, (void *) *start_address, header);
  			*start_address += header;
  		}
  		else
  		{
--- 338,356 
  		}
  		else if (eoh)
  		{
! 			char	   *tmp;
! 
! 			/*
! 			 * EOH_flatten_into expects the target address to be maxaligned,
! 			 * so we can't store directly to *start_address.
! 			 */
! 			tmp = (char *) palloc(header);
! 			EOH_flatten_into(eoh, (void *) tmp, header);
! 			memcpy(*start_address, tmp, header);
  			*start_address += header;
+ 
+ 			/* be tidy. */
+ 			pfree(tmp);
  		}
  		else
  		{
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 26409d3..d1f6510 100644
*** a/src/test/regress/expected/select_parallel.out
--- b/src/test/regress/expected/select_parallel.out
*** ORDER BY 1, 2, 3;
*** 1088,1094 
  --+---+-+--
  (0 rows)
  
! -- test interation between subquery and partial_paths
  SET LOCAL min_parallel_table_scan_size TO 0;
  CREATE VIEW tenk1_vw_sec WITH (security_barrier) AS SELECT * FROM tenk1;
  EXPLAIN (COSTS OFF)
--- 1088,1124 
  --+---+-+--
  (0 rows)
  
! -- test passing expanded-value representations to workers
! SAVEPOINT settings;
! SET force_parallel_mode = 1;
! CREATE FUNCTION make_some_array(int,int) returns int[] as
! $$declare x int[];
!   begin
! x[1] := $1;
! x[2] := $2;
! return x;
!   end$$ language plpgsql parallel safe;
! CREATE TABLE foo(f1 text, f2 int[], f3 text);
! INSERT INTO foo VALUES('1', ARRAY[1,2], 'one');
! PREPARE pstmt(text, int[]) AS SELECT * FROM foo WHERE f1 = $1 AND f2 = $2;
! EXPLAIN (COSTS OFF) EXECUTE pstmt('1', make_some_array(1,2));
! QUERY PLAN
! --
!  Gather
!Workers Planned: 3
!->  Parallel Seq Scan on foo
!  Filter: ((f1 = '1'::text) AND (f2 = '{1,2}'::integer[]))
! (4 rows)
! 
! EXECUTE pstmt('1', make_some_array(1,2));
!  f1 |  f2   | f3  
! +---+-
!  1  | {1,2} | one
! (1 row)
! 
! DEALLOCATE pstmt;
! ROLLBACK TO SAVEPOINT settings;
! -- test interaction between subquery and partial_paths
  SET LOCAL min_parallel_table_scan_size TO 0;
  CREATE VIEW tenk1_vw_sec WITH (security_barrier) AS SELECT * FROM tenk1;
  EXPLAIN (COSTS OFF)
diff --git a/src/test/regress/sql/select_par

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Tom Lane]

Stephen Frost  writes:
> My complaint is specifically trying to do something like:

> =*# select *  
> from
>   pg_class
>   join pg_attribute on (pg_class.oid = pg_attribute.attrelid)
>   join pg_namespace on (pg_class.relnamespace = pg_namespace.oid) 
>  
> where 
>
>   has_column_privilege(quote_ident(nspname) || '.' || 
> quote_ident(relname),attname,'SELECT');

> and getting this:

> ERROR:  column "pg.dropped.2" of relation "t1" does not exist

That code is kinda broken anyway, because it won't survive relation drops
either.  What you *should* be writing is

has_column_privilege(pg_class.oid, attnum, 'SELECT');

which is not only not sensitive to these problems but significantly more
efficient.

Having said that, I'm fine with having it return NULL if the given
attname matches an attisdropped column.  What I was on about is what
happens when you write

has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');

and sometab exists but somecol doesn't.

regards, tom lane

Re: buildfarm and git pull

[Andrew Dunstan]

On 10/01/2018 11:45 AM, Justin Pryzby wrote:

On Thu, Sep 27, 2018 at 06:32:59PM +0300, Alexander Kuzmenkov wrote:

It just has to checkout the remote branch as-is.

It doesn't clean files, but I would suggest:
git checkout -B branch remote/branch



You can see what was done in yesterday's three commits: 



This has been working in a couple of installations, and will be in a 
release soon.


BTW, -hackers isn't the right forum for this. Please take any further 
discussion to buildfarm-members.


cheers

andrew

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

Re: SerializeParamList vs machines with strict alignment

[Tom Lane]

I wrote:
> The attached revised patch contains a test case that demonstrably triggers
> the problem on gaur's host.  Oddly, I do not get a crash either on a PPC
> Mac or a Raspberry Pi 3 running Raspbian.  I'm not very sure why; I traced
> through things with gdb and it's definitely calling EA_flatten_into with
> an odd address and a non-flattened input.  I guess both of those platforms
> have kernel handlers for misaligned accesses?  But the Raspbian box ought
> to be nearly the same as chipmunk, which is where we saw the problem to
> begin with, so I'm a bit confused.

Ah: a bit of googling later, the mystery is solved.  PPC does have support
for unaligned 32-bit accesses, which is as much as EA_flatten_into needs.
(It's 64-bit operations where you might have a problem.)  My info was
also out of date about ARM: more recent processors, at least, can also
do unaligned 32-bit accesses.  chipmunk either has a pretty old processor
or it is configured to disable unaligned access.

Apparently the only somewhat-modern architecture that is resolutely
unaligned-unfriendly is MIPS.

regards, tom lane

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Andrew Dunstan]

On 10/01/2018 11:58 AM, Tom Lane wrote:

Mark Wong  writes:

   a   |  a   | 
 uuid_cmp
--+--+-
  ---- | ---- | 
  0
  ---- | ---- | 
-2147483648
  ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
-2147483648
  ---- | ---- | 
  1
  ---- | ---- | 
  0
  ---- | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
-2147483648
  3f3e3c3b-3a30-3938-3736-353433a2313e | ---- | 
  1
  3f3e3c3b-3a30-3938-3736-353433a2313e | ---- | 
  1
  3f3e3c3b-3a30-3938-3736-353433a2313e | 3f3e3c3b-3a30-3938-3736-353433a2313e | 
  0
(9 rows)


Oooh ... apparently, on that platform, memcmp() is willing to produce
INT_MIN in some cases.  That's not a safe value for a sort comparator
to produce --- we explicitly say that somewhere, IIRC.  I think we
implement DESC by negating the comparator's result, which explains
why only the DESC case fails.





Is there a standard that forbids this, or have we just been lucky up to now?

cheers

andrew

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

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Tom Lane]

Andrew Dunstan  writes:
> On 10/01/2018 11:58 AM, Tom Lane wrote:
>> Oooh ... apparently, on that platform, memcmp() is willing to produce
>> INT_MIN in some cases.  That's not a safe value for a sort comparator
>> to produce --- we explicitly say that somewhere, IIRC.  I think we
>> implement DESC by negating the comparator's result, which explains
>> why only the DESC case fails.

> Is there a standard that forbids this, or have we just been lucky up to now?

We've been lucky; POSIX just says the value is less than, equal to,
or greater than zero.

In practice, a memcmp that operates byte-at-a-time would not likely
return anything outside +-255.  But on a big-endian machine you could
easily optimize to use word-wide operations to compare 4 bytes at a
time, and I suspect that's what's happening here.  Or maybe there's
just some weird architecture-specific reason that makes it cheap
for them to return INT_MIN rather than some other value?

regards, tom lane

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > My complaint is specifically trying to do something like:
[...]
> That code is kinda broken anyway, because it won't survive relation drops
> either.  What you *should* be writing is

Yes, I agree, but it still seems like we're throwing an ERROR
unnecessairly.

> Having said that, I'm fine with having it return NULL if the given
> attname matches an attisdropped column.

Ok, that's really all I was asking about.

> ... What I was on about is what
> happens when you write
> 
>   has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');
> 
> and sometab exists but somecol doesn't.

Yeah, having that throw an error seems reasonable to me.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Larry Rosenman]

On Mon, Oct 01, 2018 at 05:11:02PM -0400, Tom Lane wrote:
> Andrew Dunstan  writes:
> > On 10/01/2018 11:58 AM, Tom Lane wrote:
> >> Oooh ... apparently, on that platform, memcmp() is willing to produce
> >> INT_MIN in some cases.  That's not a safe value for a sort comparator
> >> to produce --- we explicitly say that somewhere, IIRC.  I think we
> >> implement DESC by negating the comparator's result, which explains
> >> why only the DESC case fails.
> 
> > Is there a standard that forbids this, or have we just been lucky up to now?
> 
> We've been lucky; POSIX just says the value is less than, equal to,
> or greater than zero.
> 
> In practice, a memcmp that operates byte-at-a-time would not likely
> return anything outside +-255.  But on a big-endian machine you could
> easily optimize to use word-wide operations to compare 4 bytes at a
> time, and I suspect that's what's happening here.  Or maybe there's
> just some weird architecture-specific reason that makes it cheap
> for them to return INT_MIN rather than some other value?
> 
as a former S3[79]x assembler programmer, they probably do it in
registers or using TRT.  All of which could be word wise. 


>   regards, tom lane
> 

-- 
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 214-642-9640 E-Mail: l...@lerctr.org
US Mail: 5708 Sabbia Drive, Round Rock, TX 78665-2106


signature.asc
Description: PGP signature

Re: executor relation handling

[Tom Lane]

David Rowley  writes:
> On 1 October 2018 at 19:39, Amit Langote  
> wrote:
>> For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
>> etc.), I wonder whether we couldn't just *not* recalculate the lock mode
>> based on inspecting the query tree to cross-check with rellockmode?  Why
>> not just use rellockmode for locking?  Maybe, okay to keep doing that in
>> debug builds though.  Also, are the discrepancies like this to be
>> considered bugs of the existing logic?

> I got the impression Tom was just leaving that in for a while to let
> the buildfarm verify the new code is getting the same lock level as
> the old code. Of course, I might be wrong.

Yeah, exactly.  My plan is to next switch to taking the locks based on
rellockmode, and then if that doesn't show any problems to switch the
executor to just Assert that there's already a suitable lock, and then
lastly to proceed with ripping out the no-longer-needed logic that
supports the downstream calculations of lockmode.  So a bit more granular
than what Amit submitted, but we'll get to the same place in the end,
with more confidence that we didn't break anything.

regards, tom lane

Re: settings to control SSL/TLS protocol version

[Daniel Gustafsson]

> On 1 Oct 2018, at 22:21, Peter Eisentraut  
> wrote:
> 
> There have been some requests to be able to select the TLS versions
> PostgreSQL is using.  We currently only hardcode that SSLv2 and SSLv3
> are disabled, but there is also some interest now in disabling TLSv1.0
> and TLSv1.1.  Also, I've had some issues in some combinations with the
> new TLSv1.3, so there is perhaps also some use for disabling at the top end.
> 
> Attached is a patch that implements this.  For example:
> 
>ssl_min_protocol_version = 'TLSv1'
>ssl_max_protocol_version = ‘any'

I don’t think ‘any’ is a clear name for a setting which means “the highest
supported version”.  How about ‘max_supported’ or something similar?

> For reference, here is similar functionality implemented elsewhere:
> 
> https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
> https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslprotocol
> 
> Unlike those two, which offer a list of protocols to use, I have gone
> with min and max settings.

FWIW, libcurl also supports a min/max approach with CURLOPT_SSLVERSION:

https://curl.haxx.se/libcurl/c/CURLOPT_SSLVERSION.html

+1 for using a min/max approach for setting the version, and it should be
trivial to add support for in the pending GnuTLS and Secure Transport patches.

cheers ./daniel

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Andrew Dunstan]

On 10/01/2018 12:50 PM, Tom Lane wrote:

Andres Freund  writes:

On 2018-10-01 12:13:57 -0400, Tom Lane wrote:

Yeah.  So our choices are

(1) Retain the current restriction on what sort comparators can
produce.  Find all the places where memcmp's result is returned
directly, and fix them.  (I wonder if strcmp has same issue.)

(2) Drop the restriction.  This'd require at least changing the
DESC correction, and maybe other things.  I'm not sure what the
odds would be of finding everyplace we need to check.

Neither one is sounding very pleasant, or maintainable.

(2) seems more maintainable to me (or perhaps less unmaintainable). It's
infrastructure, rather than every datatype + support out there...

I guess we could set up some testing infrastructure: hack int4cmp
and/or a couple other popular comparators so that they *always*
return INT_MIN, 0, or INT_MAX, and then see what falls over.

I'm fairly sure that btree, as well as the sort code proper,
has got an issue here.





I agree option 2 seems less unmaintainable. (Nice use of litotes there?)

cheers

andrew

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

Re: automatic restore point

[Alvaro Herrera]

On 2018-Oct-01, Peter Eisentraut wrote:

> The following appears to work:
> 
> CREATE FUNCTION evt_automatic_restart_point() RETURNS event_trigger
> LANGUAGE plpgsql
> AS $$
> BEGIN
>   PERFORM pg_create_restore_point(tg_tag);
> END
> $$;
> 
> CREATE EVENT TRIGGER automatic_restart_point ON ddl_command_start
> EXECUTE PROCEDURE evt_automatic_restart_point();
> 
> Are there any concerns about this?

Mumble.

Re-reading the implementation in standard_ProcessUtility, I wonder what
is PROCESS_UTILITY_QUERY_NONATOMIC -- there seems to be a maze through
SPI that determines whether this flag is set or not, which could affect
whether the event trigger is useful.  Are utilities executed through a
procedure detected by event triggers?  If so, then this mechanism seems
good enough to me.  But if there's a way to sneak utility commands (DROP
TABLE) without the event trigger being invoked, then no (and in that
case maybe it's just a bug in procedures and we can still not include
this patch).

(Grepping for "atomic" is unsurprisingly unhelpful.  I really wish we
didn't use plain words as struct member names ...)

On the TRUNCATE case it's a bit annoying that you can't do it with event
triggers -- you have to create individual regular triggers on truncate
for each table (so you probably need yet another event trigger that
creates such triggers).

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

Re: Odd 9.4, 9.3 buildfarm failure on s390x

[Thomas Munro]

On Tue, Oct 2, 2018 at 10:55 AM Andrew Dunstan
 wrote:
> On 10/01/2018 12:50 PM, Tom Lane wrote:
> > Andres Freund  writes:
> >> On 2018-10-01 12:13:57 -0400, Tom Lane wrote:
> >>> Yeah.  So our choices are
> >>>
> >>> (1) Retain the current restriction on what sort comparators can
> >>> produce.  Find all the places where memcmp's result is returned
> >>> directly, and fix them.  (I wonder if strcmp has same issue.)
> >>>
> >>> (2) Drop the restriction.  This'd require at least changing the
> >>> DESC correction, and maybe other things.  I'm not sure what the
> >>> odds would be of finding everyplace we need to check.
> >>>
> >>> Neither one is sounding very pleasant, or maintainable.
> >> (2) seems more maintainable to me (or perhaps less unmaintainable). It's
> >> infrastructure, rather than every datatype + support out there...
> > I guess we could set up some testing infrastructure: hack int4cmp
> > and/or a couple other popular comparators so that they *always*
> > return INT_MIN, 0, or INT_MAX, and then see what falls over.
> >
> > I'm fairly sure that btree, as well as the sort code proper,
> > has got an issue here.
> >
> >
>
>
> I agree option 2 seems less unmaintainable. (Nice use of litotes there?)

+1 for option 2.  It seems to me that it should ideally be the job of
the code that is negating the value to deal with this edge case.  I
see that the restriction is clearly documented at the top of
src/backend/access/nbtree/nbtcompare.c even though it directly returns
strncmp() results.  This was quite a surprising thread.

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

Re: SerializeParamList vs machines with strict alignment

[Lou Picciano]

Wow, Tom. This is great stuff. Thanks for it.

Lou Picciano

> On Sep 9, 2018, at 11:27 PM, Tom Lane  wrote:
> 
> I wondered why buildfarm member chipmunk has been failing hard
> for the last little while.  Fortunately, it's supplying us with
> a handy backtrace:
> 
> Program terminated with signal 7, Bus error.
> #0  EA_flatten_into (allocated_size=, result=0xb55ff30e, 
> eohptr=0x188f440) at array_expanded.c:329
> 329   aresult->dataoffset = dataoffset;
> #0  EA_flatten_into (allocated_size=, result=0xb55ff30e, 
> eohptr=0x188f440) at array_expanded.c:329
> #1  EA_flatten_into (eohptr=0x188f440, result=0xb55ff30e, 
> allocated_size=) at array_expanded.c:293
> #2  0x003c3dfc in EOH_flatten_into (eohptr=, result= out>, allocated_size=) at expandeddatum.c:84
> #3  0x003c076c in datumSerialize (value=3934060, isnull=, 
> typByVal=, typLen=, start_address=0xbea3bd54) 
> at datum.c:341
> #4  0x002a8510 in SerializeParamList (paramLI=0x1889f18, 
> start_address=0xbea3bd54) at params.c:195
> #5  0x002342cc in ExecInitParallelPlan (planstate=0x, 
> estate=0x18863e0, sendParams=0x46e, nworkers=1, tuples_needed=-1) at 
> execParallel.c:700
> #6  0x002461dc in ExecGather (pstate=0x18864f0) at nodeGather.c:151
> #7  0x00236b20 in ExecProcNodeFirst (node=0x18864f0) at execProcnode.c:445
> #8  0x0022fc2c in ExecProcNode (node=0x18864f0) at 
> ../../../src/include/executor/executor.h:237
> #9  ExecutePlan (execute_once=, dest=0x188a108, 
> direction=, numberTuples=0, sendTuples=, 
> operation=CMD_SELECT, use_parallel_mode=, planstate=0x18864f0, 
> estate=0x18863e0) at execMain.c:1721
> #10 standard_ExecutorRun (queryDesc=0x188a138, direction=, 
> count=0, execute_once=true) at execMain.c:362
> #11 0x0023d630 in postquel_getnext (fcache=0x1888408, es=0x1889d68) at 
> functions.c:867
> #12 fmgr_sql (fcinfo=0x701c7c) at functions.c:1164
> 
> This is remarkably hard to replicate on other machines, but I eventually
> managed to duplicate it on gaur's host, after which it became really
> obvious that the parallel-query data transfer logic has never been
> stressed very hard on machines with strict data alignment rules.
> 
> In particular, SerializeParamList does this:
> 
>/* Write flags. */
>memcpy(*start_address, &prm->pflags, sizeof(uint16));
>*start_address += sizeof(uint16);
> 
> immediately followed by this:
> 
>datumSerialize(prm->value, prm->isnull, typByVal, typLen,
>   start_address);
> 
> and datumSerialize might do this:
> 
>EOH_flatten_into(eoh, (void *) *start_address, header);
> 
> Now, I will plead mea culpa that the expanded-object API doesn't
> say in large red letters that the target address for EOH_flatten_into
> is supposed to be maxaligned.  It only says
> 
> * The flattened representation must be a valid in-line, non-compressed,
> * 4-byte-header varlena object.
> 
> Still, one might reasonably suspect from that that *at least* 4-byte
> alignment is expected.  This code path isn't providing such alignment,
> and machines that require it will crash.  The only reason we've not
> noticed, AFAICS, is that nobody has been running with
> force_parallel_mode = regress on alignment-picky hardware.
> 
>   regards, tom lane
> 

Re: SerializeParamList vs machines with strict alignment

[Thomas Munro]

On Tue, Oct 2, 2018 at 9:49 AM Tom Lane  wrote:
> Apparently the only somewhat-modern architecture that is resolutely
> unaligned-unfriendly is MIPS.

It's been a few years now since I worked on that architecture, but
Sparc is somewhat-modern and resolutely unaligned-unfriendly.  It's
just that you can optionally install a trap handler that will do super
slow non-atomic misaligned access in software instead of blowing up
with SIGBUS.  With the Sun toolchain you did that explicitly by
building with -misalign (though it's possible that more recent
compilers might be doing that without being asked?).

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

snprintf.c hammering memset()

[Thomas Munro]

Hello hackers,

Mateusz Guzik was benchmarking PostgreSQL on FreeBSD investigating the
kqueue thread and complained off-list about a lot of calls to memset()
of size 256KB from dopr() in our snprintf.c code.

Yeah, that says:

PrintfArgType argtypes[NL_ARGMAX + 2];
...
MemSet(argtypes, 0, sizeof(argtypes));

PrintfArgType is an enum, and we define NL_ARGMAX as 16 if the OS
didn't already define it.  On FreeBSD 11, NL_ARGMAX was defined as 99
in .  On FreeBSD 12, it is defined as 65536... ouch.  On a
Debian box I see it is 4096.

Is there any reason to use the OS definition here?

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

Re: has_column_privilege behavior (was Re: Assert failed in snprintf.c)

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Having said that, I'm fine with having it return NULL if the given
>> attname matches an attisdropped column.

> Ok, that's really all I was asking about.

Ah, we were just talking past each other then :-(.  That behavior existed
already, it wasn't something my draft patch introduced, so I was confused
what you were talking about.

>> ... What I was on about is what
>> happens when you write
>> has_column_privilege('sometab'::regclass, 'somecol', 'SELECT');
>> and sometab exists but somecol doesn't.

> Yeah, having that throw an error seems reasonable to me.

OK, so here's a patch that I think does the right things.

I noticed that has_foreign_data_wrapper_privilege() and some other
recently-added members of the has_foo_privilege family had not gotten
the word about not failing on bogus OIDs, so I also fixed those.

regards, tom lane

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index d5285e2..c5f7918 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -2447,8 +2447,12 @@ has_any_column_privilege_id_id(PG_FUNCTION_ARGS)
  *
  *		The result is a boolean value: true if user has the indicated
  *		privilege, false if not.  The variants that take a relation OID
- *		and an integer attnum return NULL (rather than throwing an error)
- *		if the column doesn't exist or is dropped.
+ *		return NULL (rather than throwing an error) if that relation OID
+ *		doesn't exist.  Likewise, the variants that take an integer attnum
+ *		return NULL (rather than throwing an error) if there is no such
+ *		pg_attribute entry.  All variants return NULL if an attisdropped
+ *		column is selected.  These rules are meant to avoid unnecessary
+ *		failures in queries that scan pg_attribute.
  */
 
 /*
@@ -2466,6 +2470,12 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
 	Form_pg_attribute attributeForm;
 
 	/*
+	 * If convert_column_name failed, we can just return -1 immediately.
+	 */
+	if (attnum == InvalidAttrNumber)
+		return -1;
+
+	/*
 	 * First check if we have the privilege at the table level.  We check
 	 * existence of the pg_class row before risking calling pg_class_aclcheck.
 	 * Note: it might seem there's a race condition against concurrent DROP,
@@ -2826,21 +2836,59 @@ has_column_privilege_id_attnum(PG_FUNCTION_ARGS)
 
 /*
  * Given a table OID and a column name expressed as a string, look it up
- * and return the column number
+ * and return the column number.  Returns InvalidAttrNumber in cases
+ * where caller should return NULL instead of failing.
  */
 static AttrNumber
 convert_column_name(Oid tableoid, text *column)
 {
-	AttrNumber	attnum;
 	char	   *colname;
+	HeapTuple	attTuple;
+	AttrNumber	attnum;
 
 	colname = text_to_cstring(column);
-	attnum = get_attnum(tableoid, colname);
-	if (attnum == InvalidAttrNumber)
-		ereport(ERROR,
-(errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" does not exist",
-		colname, get_rel_name(tableoid;
+
+	/*
+	 * We don't use get_attnum() here because it will report that dropped
+	 * columns don't exist.  We need to treat dropped columns differently from
+	 * nonexistent columns.
+	 */
+	attTuple = SearchSysCache2(ATTNAME,
+			   ObjectIdGetDatum(tableoid),
+			   CStringGetDatum(colname));
+	if (HeapTupleIsValid(attTuple))
+	{
+		Form_pg_attribute attributeForm;
+
+		attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
+		/* We want to return NULL for dropped columns */
+		if (attributeForm->attisdropped)
+			attnum = InvalidAttrNumber;
+		else
+			attnum = attributeForm->attnum;
+		ReleaseSysCache(attTuple);
+	}
+	else
+	{
+		char	   *tablename = get_rel_name(tableoid);
+
+		/*
+		 * If the table OID is bogus, or it's just been dropped, we'll get
+		 * NULL back.  In such cases we want has_column_privilege to return
+		 * NULL too, so just return InvalidAttrNumber.
+		 */
+		if (tablename != NULL)
+		{
+			/* tableoid exists, colname does not, so throw error */
+			ereport(ERROR,
+	(errcode(ERRCODE_UNDEFINED_COLUMN),
+	 errmsg("column \"%s\" of relation \"%s\" does not exist",
+			colname, tablename)));
+		}
+		/* tableoid doesn't exist, so act like attisdropped case */
+		attnum = InvalidAttrNumber;
+	}
+
 	pfree(colname);
 	return attnum;
 }
@@ -3144,6 +3192,9 @@ has_foreign_data_wrapper_privilege_name_id(PG_FUNCTION_ARGS)
 	roleid = get_role_oid_or_public(NameStr(*username));
 	mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
 
+	if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+		PG_RETURN_NULL();
+
 	aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
 
 	PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3167,6 +3218,9 @@ has_foreign_data_wrapper_privilege_id(PG_FUNCTION_ARGS)
 	roleid = GetUserId();
 	mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
 
+	if (!SearchSysCacheExists1(FOR

Re: snprintf.c hammering memset()

[Tom Lane]

Thomas Munro  writes:
> Mateusz Guzik was benchmarking PostgreSQL on FreeBSD investigating the
> kqueue thread and complained off-list about a lot of calls to memset()
> of size 256KB from dopr() in our snprintf.c code.

> Yeah, that says:
> PrintfArgType argtypes[NL_ARGMAX + 2];
> ...
> MemSet(argtypes, 0, sizeof(argtypes));

> PrintfArgType is an enum, and we define NL_ARGMAX as 16 if the OS
> didn't already define it.  On FreeBSD 11, NL_ARGMAX was defined as 99
> in .  On FreeBSD 12, it is defined as 65536... ouch.  On a
> Debian box I see it is 4096.

> Is there any reason to use the OS definition here?

Ouch indeed.  Quite aside from cycles wasted, that's way more stack than
we want this to consume.  I'm good with forcing this to 16 or so ...
any objections?

(For reference, POSIX doesn't require NL_ARGMAX to be more than 9.)

(I wonder if this has anything to do with Andres' performance gripes.)

regards, tom lane

Re: snprintf.c hammering memset()

[Andres Freund]

Hi,

On 2018-10-01 19:52:40 -0400, Tom Lane wrote:
> Thomas Munro  writes:
> > Mateusz Guzik was benchmarking PostgreSQL on FreeBSD investigating the
> > kqueue thread and complained off-list about a lot of calls to memset()
> > of size 256KB from dopr() in our snprintf.c code.
> 
> > Yeah, that says:
> > PrintfArgType argtypes[NL_ARGMAX + 2];
> > ...
> > MemSet(argtypes, 0, sizeof(argtypes));
> 
> > PrintfArgType is an enum, and we define NL_ARGMAX as 16 if the OS
> > didn't already define it.  On FreeBSD 11, NL_ARGMAX was defined as 99
> > in .  On FreeBSD 12, it is defined as 65536... ouch.  On a
> > Debian box I see it is 4096.
> 
> > Is there any reason to use the OS definition here?
> 
> Ouch indeed.  Quite aside from cycles wasted, that's way more stack than
> we want this to consume.  I'm good with forcing this to 16 or so ...
> any objections?

Especially after your performance patch, shouldn't we actually be able
to get rid of that memset entirely?

And if not, shouldn't we be able to reduce the per-element size of
argtypes considerably, by using a uint8 as the base, rather than 4 byte
per element?


> (I wonder if this has anything to do with Andres' performance gripes.)

It probably plays some role, but the profile didn't show it as *too* large
a part.

Greetings,

Andres Freund

Re: SerializeParamList vs machines with strict alignment

[Tom Lane]

Thomas Munro  writes:
> On Tue, Oct 2, 2018 at 9:49 AM Tom Lane  wrote:
>> Apparently the only somewhat-modern architecture that is resolutely
>> unaligned-unfriendly is MIPS.

> It's been a few years now since I worked on that architecture, but
> Sparc is somewhat-modern and resolutely unaligned-unfriendly.  It's
> just that you can optionally install a trap handler that will do super
> slow non-atomic misaligned access in software instead of blowing up
> with SIGBUS.  With the Sun toolchain you did that explicitly by
> building with -misalign (though it's possible that more recent
> compilers might be doing that without being asked?).

Interesting ... I suppose we'd have seen that on the Sparc critters,
except that they weren't running with force_parallel_mode = regress
like chipmunk is.

Now I'm tempted to propose that Amit commit *just* the test case
and not the fix, and wait a day to see which buildfarm critters fail.

regards, tom lane

Re: Add SKIP LOCKED to VACUUM and ANALYZE

[Michael Paquier]

On Mon, Oct 01, 2018 at 03:37:01PM +, Bossart, Nathan wrote:
> Good idea.  This patch looks good to me.

Thanks, I have pushed this one now.

> Without the find_all_inheritors() stuff, I think we would just need to
> modify the ANALYZE documentation patch to say something like
> 
> Specifies that ANALYZE should not wait for any conflicting locks
> to be released: if a relation cannot be locked immediately without
> waiting, the relation is skipped.  Note that even with this
> option, ANALYZE can still block when opening the relation's
> indexes or when acquiring sample rows to prepare the statistics.
> Also, while ANALYZE ordinarily processes all leaf partitions of
> partitioned tables, this option will cause ANALYZE to skip all
> leaf partitions if there is a conflicting lock on the partitioned
> table.

Yes, I think that we could live with something like that.  I could give
this whole thing a shot, but this will have to wait until the commit
fest is closed as there are still many patches to classify.  If you can
send a patch that's of course always helpful..
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Improve autovacuum logging for aggressive and anti-wraparound ru

[Michael Paquier]

On Fri, Sep 28, 2018 at 01:53:14PM +0900, Masahiko Sawada wrote:
> I agree. Can we fix this simply by the attached patch?

Thanks for sending a patch.

+/* autovacuum cannot be anti-wraparound and not aggressive vacuum */
+Assert(aggressive);
+msgfmt = _("automatic aggressive vacuum to prevent wraparound of table 
\"%s.%s.%s\": index scans: %d\n");

While adding this comment in lazy_vacuum_rel() is adapted, I think that
we ought to make the assertion check more aggressive by not having it
depend on if log_min_duration is set or not.  So why not moving that to
a place a bit higher, where aggressive gets defined?
--
Michael


signature.asc
Description: PGP signature

Re: snprintf.c hammering memset()

[Tom Lane]

Andres Freund  writes:
> On 2018-10-01 19:52:40 -0400, Tom Lane wrote:
>> Ouch indeed.  Quite aside from cycles wasted, that's way more stack than
>> we want this to consume.  I'm good with forcing this to 16 or so ...
>> any objections?

> Especially after your performance patch, shouldn't we actually be able
> to get rid of that memset entirely?

That patch takes the memset out of the main line, but it'd still be
a performance problem for formats using argument reordering; and the
stack-space concern would remain the same.

> And if not, shouldn't we be able to reduce the per-element size of
> argtypes considerably, by using a uint8 as the base, rather than 4 byte
> per element?

argtypes is only a small part of the stack-space issue, there's also
argvalues which is (at least) twice as big.  I don't think second-guessing
the compiler about the most efficient representation for an enum is
going to buy us much here.

regards, tom lane

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Michael Paquier]

On Mon, Oct 01, 2018 at 02:37:42PM +0200, Daniel Gustafsson wrote:
>> On 1 Oct 2018, at 01:19, Michael Paquier  wrote:
>> Looking at the refactoring patch 0001, wouldn't signalfuncs.c make a
>> better name for the new file?  There are already multiple examples of
>> this type, like logicalfuncs.c, slotfuncs.c, etc.
> 
> I have no strong feelings on this, I was merely using the name that Alvaro
> suggested when he brought up the refactoring as an extension of this patch.
> signalfuncs.c is fine by me, so I did this rename in the attached revision.

Indeed, I missed the previous part posted here:
https://www.postgresql.org/message-id/20180124154548.gmpyvkzlsijren7u@alvherre.pgsql

Alvaro, do you have a strong preference over one or the other?
--
Michael


signature.asc
Description: PGP signature

Re: snprintf.c hammering memset()

[Tom Lane]

Thomas Munro  writes:
> PrintfArgType is an enum, and we define NL_ARGMAX as 16 if the OS
> didn't already define it.  On FreeBSD 11, NL_ARGMAX was defined as 99
> in .  On FreeBSD 12, it is defined as 65536... ouch.  On a
> Debian box I see it is 4096.

Some further research:

* My Red Hat boxes also think it's 4096.

* macOS thinks it's just 9.

* Assuming I've grepped the .po files correctly, we have no translatable
messages today that use more than 9 %'s.  That's not a totally accurate
result because I didn't try to count "*" precision/width specs, which'd
also count against ARGMAX.  Still, we couldn't be needing much more than
9 slots.

* It's completely silly to imagine that anybody would write a printf call
with more than, perhaps, a couple dozen arguments.  So these OS values
must be getting set with an eye to some other use-case for NL_ARGMAX
besides printf field order control.

Setting snprintf's limit to 16 might be a bit tight based on the observed
results for translatable messages, but I'd be entirely comfortable with
32.  The values we're getting from the OS are just silly.

regards, tom lane

Re: snprintf.c hammering memset()

[Andres Freund]

Hi,

On 2018-10-01 20:19:16 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-10-01 19:52:40 -0400, Tom Lane wrote:
> >> Ouch indeed.  Quite aside from cycles wasted, that's way more stack than
> >> we want this to consume.  I'm good with forcing this to 16 or so ...
> >> any objections?
> 
> > Especially after your performance patch, shouldn't we actually be able
> > to get rid of that memset entirely?
> 
> That patch takes the memset out of the main line, but it'd still be
> a performance problem for formats using argument reordering; and the
> stack-space concern would remain the same.

What I mean is that it shouldn't be that hard to only zero out the
portions of the array that are actually used, and thus could refrain
from introducing the limit.


> > And if not, shouldn't we be able to reduce the per-element size of
> > argtypes considerably, by using a uint8 as the base, rather than 4 byte
> > per element?
> 
> argtypes is only a small part of the stack-space issue, there's also
> argvalues which is (at least) twice as big.  I don't think second-guessing
> the compiler about the most efficient representation for an enum is
> going to buy us much here.

Sure, but argvalues isn't zeroed out.  As long as the memory on the
stack isn't used (as is the case for most of arvalues elements), the
size of the stack allocation doesn't have a significant efficiency
impact - it's still just an %rsp adjustment.  If we're going to continue
to zero out argtypes, it's sure going to be better to zero out 16 rather
than 64bytes (after limiting to 16 args).

Greetings,

Andres Freund

Re: snprintf.c hammering memset()

[Andres Freund]

Hi,

On 2018-10-01 17:46:55 -0700, Andres Freund wrote:
> On 2018-10-01 20:19:16 -0400, Tom Lane wrote:
> > argtypes is only a small part of the stack-space issue, there's also
> > argvalues which is (at least) twice as big.  I don't think second-guessing
> > the compiler about the most efficient representation for an enum is
> > going to buy us much here.
> 
> Sure, but argvalues isn't zeroed out.  As long as the memory on the
> stack isn't used (as is the case for most of arvalues elements), the
> size of the stack allocation doesn't have a significant efficiency
> impact - it's still just an %rsp adjustment.  If we're going to continue
> to zero out argtypes, it's sure going to be better to zero out 16 rather
> than 64bytes (after limiting to 16 args).

Btw, I don't think any common compiler optimizes common enum sizes for
efficiency.  Their size is part of the struct layout, so doing so would
not generally be trivial (although you get to larger enums if you go
above either INT32_MAX or UINT32_MAX element values, but ...).

Greetings,

Andres Freund

Re: snprintf.c hammering memset()

[Tom Lane]

Andres Freund  writes:
> On 2018-10-01 20:19:16 -0400, Tom Lane wrote:
>> That patch takes the memset out of the main line, but it'd still be
>> a performance problem for formats using argument reordering; and the
>> stack-space concern would remain the same.

> What I mean is that it shouldn't be that hard to only zero out the
> portions of the array that are actually used, and thus could refrain
> from introducing the limit.

Well, we use the zeroing exactly to detect which entries have been used.
Probably there's another way, but I doubt it'd be faster.

In any case, the stack-space-consumption problem remains, and IMO that
is a *far* greater concern than the cycles.  Keep in mind that we'll be
calling this code when we have already hit our stack space consumption
limit.  I'm a bit surprised that we've not seen any buildfarm members
fall over in the error recursion test.  We have not designed the system
on the assumption that ereport() could eat half a meg of stack.

regards, tom lane

Re: [HACKERS] Re: Improve OR conditions on joined columns (common star schema problem)

[Michael Paquier]

On Mon, Sep 03, 2018 at 06:59:10PM -0700, Noah Misch wrote:
> If you're going to keep this highly-simplified estimate, please expand the
> comment to say why it doesn't matter or what makes it hard to do better.  The
> non-planunionor.c path for the same query computes its own estimate of the
> same underlying quantity.  Though it may be too difficult in today's system,
> one could copy the competing path's row count estimate here.  Perhaps it
> doesn't matter because higher-level processing already assumes equal row count
> among competitors?

As there has been no replies to Noah's review for one month, I am
marking this patch as returned with feedback for now.
--
Michael


signature.asc
Description: PGP signature

Re: GiST VACUUM

[Michael Paquier]

On Mon, Aug 06, 2018 at 11:12:00PM +0500, Andrey Borodin wrote:
> Done. Added function bms_make_empty(int size)

Andrey, your latest patch does not apply.  I am moving this to the next
CF, waiting for your input.
--
Michael


signature.asc
Description: PGP signature

Re: patch to allow disable of WAL recycling

[Michael Paquier]

On Thu, Sep 13, 2018 at 02:56:42PM -0600, Jerry Jelinek wrote:
> I'll take a look at that. I had been trying to keep the patch as minimal as
> possible, but I'm happy to work through this.

(Please be careful with top-posting)

Jerry, the last status was from three weeks ago with the patch waiting
on the author, so I am marking it as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: speeding up planning with partitions

[Michael Paquier]

On Sat, Sep 29, 2018 at 07:00:02PM +0530, Dilip Kumar wrote:
> I think we can delay allocating memory for rel->part_rels?  And we can
> allocate in add_rel_partitions_to_query only
> for those partitions which survive pruning.

This last review set has not been answered, and as it is recent I am
moving the patch to next CF, waiting on author.
--
Michael


signature.asc
Description: PGP signature

Re: speeding up planning with partitions

[Amit Langote]

On 2018/10/02 10:20, Michael Paquier wrote:
> On Sat, Sep 29, 2018 at 07:00:02PM +0530, Dilip Kumar wrote:
>> I think we can delay allocating memory for rel->part_rels?  And we can
>> allocate in add_rel_partitions_to_query only
>> for those partitions which survive pruning.
> 
> This last review set has not been answered, and as it is recent I am
> moving the patch to next CF, waiting on author.

I was thinking of doing that myself today, thanks for taking care of that.

Will get back to this by the end of this week.

Thanks,
Amit

Re: SerializeParamList vs machines with strict alignment

[Amit Kapila]

On Tue, Oct 2, 2018 at 5:31 AM Tom Lane  wrote:
>
> Thomas Munro  writes:
> > On Tue, Oct 2, 2018 at 9:49 AM Tom Lane  wrote:
> >> Apparently the only somewhat-modern architecture that is resolutely
> >> unaligned-unfriendly is MIPS.
>
> > It's been a few years now since I worked on that architecture, but
> > Sparc is somewhat-modern and resolutely unaligned-unfriendly.  It's
> > just that you can optionally install a trap handler that will do super
> > slow non-atomic misaligned access in software instead of blowing up
> > with SIGBUS.  With the Sun toolchain you did that explicitly by
> > building with -misalign (though it's possible that more recent
> > compilers might be doing that without being asked?).
>
> Interesting ... I suppose we'd have seen that on the Sparc critters,
> except that they weren't running with force_parallel_mode = regress
> like chipmunk is.
>
> Now I'm tempted to propose that Amit commit *just* the test case
> and not the fix, and wait a day to see which buildfarm critters fail.
>

Okay, I will take care of that.  Are you proposing to commit the test
only on HEAD or in back-branches as well?  Ideally committing on HEAD
would serve our purpose.
There is one difference in our tests which I would like to highlight:
fix_datum_serialization_v2:
+EXPLAIN (COSTS OFF) EXECUTE stmt_sel_domain(make_psafe_ad(1,2));
+   QUERY PLAN
+
+ Gather
+   Workers Planned: 1
+   Single Copy: true
+   ->  Seq Scan on tarrdomain
+ Filter: ((c2)::integer[] = '{1,2}'::integer[])
+(5 rows)

fix_datum_serialization_v3
! EXPLAIN (COSTS OFF) EXECUTE pstmt('1', make_some_array(1,2));
! QUERY PLAN
! --
!  Gather
!Workers Planned: 3
!->  Parallel Seq Scan on foo
!  Filter: ((f1 = '1'::text) AND (f2 = '{1,2}'::integer[]))
! (4 rows)

I think if we do Analyze on the table after populating rows, it should
use just one worker and that should be sufficient to hit the case
being discussed.  I would like to change the test so that it uses just
one worker.

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

Re: [FEATURE PATCH] pg_stat_statements with plans (v02)

[Michael Paquier]

On Fri, Apr 06, 2018 at 03:03:30PM +0200, Julian Markwort wrote:
> As I see it, planning duration, first date, and last update date would
> be columns added to the pg_stat_statements view, i.e. they are tracked
> for each kind of a (jumbled) query -- just as the good and bad plans,
> their associated execution times and timestamps are.

The latest patch set does not apply and has been waiting on input from
author for more than a couple of weeks, so please note that I have
marked it as returned with feedback in commit fest 2018-09.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Re: Improve OR conditions on joined columns (common star schema problem)

[Tom Lane]

Michael Paquier  writes:
> On Mon, Sep 03, 2018 at 06:59:10PM -0700, Noah Misch wrote:
>> If you're going to keep this highly-simplified estimate, please expand the
>> comment to say why it doesn't matter or what makes it hard to do better.  The
>> non-planunionor.c path for the same query computes its own estimate of the
>> same underlying quantity.  Though it may be too difficult in today's system,
>> one could copy the competing path's row count estimate here.  Perhaps it
>> doesn't matter because higher-level processing already assumes equal row 
>> count
>> among competitors?

> As there has been no replies to Noah's review for one month, I am
> marking this patch as returned with feedback for now.

FWIW, my problem with this patch is that I remain unconvinced of the basic
correctness of the transform (specifically the unique-ification approach).
Noah's points would be important to address if we were moving the patch
towards commit, but I don't see much reason to put effort into it until
we can think of a way to prove whether that works.

regards, tom lane

Re: [PATCH v18] GSSAPI encryption support

[Michael Paquier]

On Mon, Aug 06, 2018 at 05:23:28PM -0400, Robbie Harwood wrote:
> If you're in a position where you're using Kerberos (or most other
> things from the GSSAPI) for authentication, the encryption comes at
> little to no additional setup cost.  And then you get all the security
> benefits outlined in the docs changes.

Please note that the last patch set does not apply anymore, so I have
moved it to CF 2018-11 and marked it as waiting on author.
--
Michael


signature.asc
Description: PGP signature

Re: SerializeParamList vs machines with strict alignment

[Tom Lane]

Amit Kapila  writes:
> I think if we do Analyze on the table after populating rows, it should
> use just one worker and that should be sufficient to hit the case
> being discussed.  I would like to change the test so that it uses just
> one worker.

I thought that adding an ANALYZE would make the test be net slower, not
faster; ANALYZE isn't free, even on just a row or so.  Also, I believe
that coding the test this way makes the leader send the param values to
multiple workers, which would flush out any problems with serializing a
value multiple times.  As against that, there's a hazard that the number
of workers might not be stable ... but it seems like we have lots of
other occurrences of that same hazard elsewhere in this test script.

regards, tom lane

Re: TupleTableSlot abstraction

[Kyotaro HORIGUCHI]

At Tue, 25 Sep 2018 16:45:09 -0700, Andres Freund  wrote in 
<20180925234509.3hrrf6tmvy5tf...@alap3.anarazel.de>
> Hi,
> 
> On 2018-09-04 18:35:34 +0530, Amit Khandekar wrote:
> > Subject: [PATCH 05/14] Use tts_flags instead of previous bool members
> > 
> > Pack the boolean members in TupleTableSlot into a 16 bit tts_flags.
> > This reduces the size of TupleTableSlot since each bool member takes
> > at least a byte on the platforms where bool is defined as a char.
> > 
> > Ashutosh Bapat and Andres Freund
> 
> > +
> > +/* true = slot is empty */
> > +#defineTTS_ISEMPTY (1 << 1)
> > +#define IS_TTS_EMPTY(slot) ((slot)->tts_flags & TTS_ISEMPTY)
> > +#define SET_TTS_EMPTY(slot) ((slot)->tts_flags |= TTS_ISEMPTY)
> > +#define RESET_TTS_EMPTY(slot) ((slot)->tts_flags &= ~TTS_ISEMPTY)
> 
> The flag stuff is the right way, but I'm a bit dubious about the
> accessor functions.  I can see open-coding the accesses, using the
> macros, or potentially going towards using bitfields.
> 
> Any comments?

Currently we have few setter/resetter function(macro)s for such
simple operations. FWIW open-coding in the following two looks
rather easier to me.

> if (IS_TTS_EMPTY(slot))
> if (slot->tts_flags & TTS_ISEMPTY)


About bitfields, an advantage of it is debugger awareness. We
don't need to look aside to the definitions of bitwise macros
while using debugger. And the current code is preserved in
appearance by using it.

> if (slot->tts_isempty)
> slot->tts_isempty = true;

In short, +1 from me to use bitfields. Coulnd't we use bitfield
here, possiblly in other places then?


=
Not related to tuple slots, in other places, like infomask, we
handle a set of bitmap values altogether.


> infomask = tuple->t_data->t_infomask;

Bare bitfields are a bit inconvenient for the use. It gets
simpler using C11 anonymous member but not so bothersome even in
C99.  Anyway I don't think we jump into that immediately.

infomask.all = tuple->t_data->t_infomask.all;

- if (!HeapTupleHeaderXminCommitted(tuple))
C99> if (tuple->t_infomask.b.xmin_committed)
C11> if (tuple->t_infomask.xmin_committed)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Vacuum: allow usage of more than 1GB of work mem

[Michael Paquier]

On Mon, Jul 16, 2018 at 02:33:17PM -0400, Andrew Dunstan wrote:
> Ah, ok. Thanks. ignore the email I just sent about that.

So...  This thread has basically died of inactivity, while there have
been a couple of interesting things discussed, like the version from
Heikki here:
https://www.postgresql.org/message-id/cd8f7b62-17e1-4307-9f81-427922e5a...@iki.fi

I am marking the patches as returned with feedback for now.
--
Michael


signature.asc
Description: PGP signature

Re: pg_dumpall --exclude-database option

[Michael Paquier]

On Fri, Aug 03, 2018 at 11:08:57PM +0200, Fabien COELHO wrote:
> Patch applies cleanly, compiles, and works for me.

Last review has not been addressed, so please note that this has been
marked as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Removing [Merge]Append nodes which contain a single subpath

[Michael Paquier]

On Mon, Jul 02, 2018 at 09:37:31AM +1200, David Rowley wrote:
> Now that the September 'fest is open for new patches I'm going to move
> this patch over there.  This patch has become slightly less important
> than some other stuff, but I'd still like to come back to it.

Please note that the latest patch does not apply anymore, so I moved it
to CF 2018-11 (already this time of the year!), waiting for your input.
--
Michael


signature.asc
Description: PGP signature

Re: cost_sort() improvements

[Michael Paquier]

On Sun, Jul 22, 2018 at 10:22:10PM +0200, Tomas Vondra wrote:
> Could you perhaps summarize the reasoning or at least point me to the
> relevant parts of the sources, so that I know which parts to focus on?

Teodor, this patch is waiting for some input from you.  I have moved it
to next CF for now, 2018-11.
--
Michael


signature.asc
Description: PGP signature

Re: Push down Aggregates below joins

[Michael Paquier]

On Fri, Aug 03, 2018 at 04:50:11PM +0200, Antonin Houska wrote:
> Antonin Houska  wrote:
> 
> > I didn't have enough time to separate "your functionality" and can do it 
> > when
> > I'm back from vacation.
> 
> So I've separated the code that does not use the 2-stage replication (and
> therefore the feature is not involved in parallel queries).

This patch has been waiting for input from its author for a couple of
months now, so I am switching it to "Returned with Feedback".  If a new
version can be provided, please feel free to send a new one.
--
Michael


signature.asc
Description: PGP signature

Re: Subplan result caching

[Michael Paquier]

On Mon, Jul 09, 2018 at 09:08:07PM +1200, David Rowley wrote:
> I'll mark as waiting on author in the meantime.
> 
> It's great to see someone working on this.

Not that actively unfortunately.  The patch status has remained
unchanged since, so I am marking it as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: POC: GROUP BY optimization

[Michael Paquier]

On Sat, Jun 30, 2018 at 09:19:13AM +1200, Gavin Flower wrote:
> Additionally put an upper limit threshold on the number of combinations to
> check, fairly large by default?
> 
> If first threshold is exceeded, could consider checking out a few more
> selected at random from paths not yet checked, to avoid any bias caused by
> stopping a systematic search.  This might prove important when N! is fairly
> large.

Please note that the latest patch available does not apply, so this has
been moved to next CF 2018-11, waiting for input from its author.
--
Michael


signature.asc
Description: PGP signature

Re: Subplan result caching

[David Rowley]

On 19 July 2018 at 06:27, Robert Haas  wrote:
> On Mon, Jul 9, 2018 at 5:08 AM, David Rowley
>> "LazyMaterialize" seems like a good option for a name. It seems better
>> than "LazyHash" since you may not want to restrict it to a hash table
>> based cache in the future.  A binary search tree may be a good option
>> for types that cannot be hashed.
>
> I think that's not too clear, actually.  The difference between a
> Materialize and a LazyMaterialize is not that this is lazy and that's
> not.  It's that this can cache multiple result sets for various
> parameter values and that can only cache one result set.

Okay. I'm not going to argue with the name of a node type that does
not exist yet. I was more suggesting that it should be a modular
component that we can plug into whatever plan types suit it. My
suggestions for naming was admittedly more of a sales tactic to gain
support for the idea, which perhaps failed.

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