Re: Problem while setting the fpw with SIGHUP

[Michael Paquier]

On Wed, Mar 28, 2018 at 03:40:59PM +0900, Kyotaro HORIGUCHI wrote:
> The attached does that. I don't like that it uses ControlFileLock
> to exlucde concurrent UpdateFullPageWrites and StartupXLOG but
> WALInsertLock cannot be used since UpdateFullPageWrites may take
> the same lock.

You visibly forgot your patch.
--
Michael


signature.asc
Description: PGP signature

Re: ALTER TABLE ADD COLUMN fast default

[Andres Freund]

Hi,

On 2018-03-26 09:02:10 +1030, Andrew Dunstan wrote:
> Thanks for this, all looks good. Here is the consolidate patch
> rebased. If there are no further comments I propose to commit this in
> a few days time.

Here's a bit of post commit review:

@@ -1310,13 +1679,11 @@ slot_getsomeattrs(TupleTableSlot *slot, int attnum)
 
/*
 * If tuple doesn't have all the atts indicated by tupleDesc, read the
-* rest as null
+* rest as NULLs or missing values
 */
-   for (; attno < attnum; attno++)
-   {
-   slot->tts_values[attno] = (Datum) 0;
-   slot->tts_isnull[attno] = true;
-   }
+   if (attno < attnum)
+   slot_getmissingattrs(slot, attno, attnum);
+
slot->tts_nvalid = attnum;
 }

It's worthwhile to note that this'll re-process *all* missing values,
even if they've already been deformed. I.e. if
slot_getmissingattrs(.., first-missing)
slot_getmissingattrs(.., first-missing + 1)
slot_getmissingattrs(.., first-missing + 2)
is called, all three missing values will be copied every time. That's
because tts_nvalid isn't taken into account.  I wonder if slot_getmissingattrs
could take tts_nvalid into account?

I also wonder if this doesn't deserve an unlikely(), to avoid the cost
of spilling registers in the hot branch of not missing any values.


+   else
+   {
+   /* if there is a missing values array we must process them one 
by one */
+   for (missattnum = lastAttNum - 1;
+missattnum >= startAttNum;
+missattnum--)
+   {
+   slot->tts_values[missattnum] = 
attrmiss[missattnum].ammissing;
+   slot->tts_isnull[missattnum] =
+   !attrmiss[missattnum].ammissingPresent;
+   }
+   }
+}

Why is this done backwards? It's noticeably slower to walk arrays
backwards on some CPU microarchitectures.



@@ -176,6 +179,23 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
}
}
 


@@ -469,6 +503,29 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
if (strcmp(defval1->adbin, defval2->adbin) != 0)
return false;
}
+   if (constr1->missing)
+   {
+   if (!constr2->missing)
+   return false;
+   for (i = 0; i < tupdesc1->natts; i++)
+   {
+   AttrMissing *missval1 = constr1->missing + i;
+   AttrMissing *missval2 = constr2->missing + i;

It's a bit odd to not use array indexing here?


@@ -625,7 +625,15 @@ ExecCopySlotMinimalTuple(TupleTableSlot *slot)
if (slot->tts_mintuple)
return heap_copy_minimal_tuple(slot->tts_mintuple);
if (slot->tts_tuple)
-   return minimal_tuple_from_heap_tuple(slot->tts_tuple);
+   {
+   if (TTS_HAS_PHYSICAL_TUPLE(slot) &&
+   HeapTupleHeaderGetNatts(slot->tts_tuple->t_data)
+   < slot->tts_tupleDescriptor->natts)
+   return minimal_expand_tuple(slot->tts_tuple,
+   
slot->tts_tupleDescriptor);
+   else
+   return minimal_tuple_from_heap_tuple(slot->tts_tuple);
+   }
 

What's the TTS_HAS_PHYSICAL_TUPLE doing here? How can this be relevant
given the previous tts_mintuple check? Am I missing something?



@@ -563,10 +569,63 @@ RelationBuildTupleDesc(Relation relation)

MemoryContextAllocZero(CacheMemoryContext,

   relation->rd_rel->relnatts *

   sizeof(AttrDefault));
-   attrdef[ndef].adnum = attp->attnum;
+   attrdef[ndef].adnum = attnum;
attrdef[ndef].adbin = NULL;
+
ndef++;
}
+
+   /* Likewise for a missing value */
+   if (attp->atthasmissing)
+   {
+   Datum   missingval;
+   boolmissingNull;
+
+   /* Do we have a missing value? */
+   missingval = heap_getattr(pg_attribute_tuple,
+ 
Anum_pg_attribute_attmissingval,
+ 
pg_attribute_desc->rd_att,
+ 
&missingNull);
+   if (!missingNull)
+   {
+   /* Yes, fetch fro

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

[Amit Kapila]

On Tue, Mar 27, 2018 at 10:59 PM, Robert Haas  wrote:
> On Tue, Mar 27, 2018 at 7:42 AM, Robert Haas  wrote:
>> On Tue, Mar 27, 2018 at 1:45 AM, Amit Kapila  wrote:
>>> If we don't want to go with the upperrel logic, then maybe we should
>>> consider just merging some of the other changes from my previous patch
>>> in 0003* patch you have posted and then see if it gets rid of all the
>>> cases where we are seeing a regression with this new approach.
>>
>> Which changes are you talking about?
>
> I realized that this version could be optimized in the case where the
> scanjoin_target and the topmost scan/join rel have the same
> expressions in the target list.
>

Good idea, such an optimization will ensure that the cases reported
above will not have regression.  However isn't it somewhat beating the
idea you are using in the patch which is to avoid modifying the path
in-place?  In any case, I think one will still see regression in cases
where this optimization doesn't apply.  For example,

DO $$
DECLARE count integer;
BEGIN
For count In 1..100 Loop
Execute 'explain Select sum(thousand)from tenk1 group by ten';
END LOOP;
END;
$$;

The above block takes 43700.0289 ms on Head and 45025.3779 ms with the
patch which is approximately 3% regression.  In this case, the
regression is lesser than the previously reported cases, but I guess
we might see bigger regression if the number of columns is more or
with a different set of queries.   I think the cases which can
slowdown are where we need to use physical tlist in
create_projection_plan and the caller has requested CP_LABEL_TLIST.  I
have checked in regression tests and there seem to be more cases which
will be impacted.  Another such example from regression tests is:
select count(*) >= 0 as ok from pg_available_extension_versions;


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

Re: [HACKERS] Surjective functional indexes

[Konstantin Knizhnik]

On 27.03.2018 22:08, Simon Riggs wrote:

On 23 March 2018 at 15:54, Simon Riggs  wrote:


So please could you make the change?

Committed, but I still think that change would be good.


Thank you.
But I still not sure whether replacement of bitmap with List or array of 
Oids is really good idea.


There are two aspects: size and speed. It seems to me that memory 
overhead is most important.
At least you rejected your own idea about using autotune for this 
parameters because of presence of extra 16 bytes in statistic.
But RelationData structure is even more space critical: its size is 
multiplied by number of relations and backends.
Bitmap seems to provide the most compact representation of the 
projective index list.


Concerning speed aspect, certainly iteration through the list of all 
indexes with checking presence of index in the bitmap is more expensive 
than just direct iteration through Oid
list or array. But since check of bitmap can be done in constant time, 
both approaches have the same complexity. Also for typical case (< 5 
normal indexes for a table and 1-2 functional indexes)

difference in performance can not be measured.

Also bitmap provides convenient interface for adding new members. To 
construct array of Oids I need first to determine number of indexes, so 
I have to perform two loops.


So if you and Alvaro insist on this change, then I will definitely do 
it. But from my point of view, using bitmap here is acceptable solution.




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

Re: Proposal: http2 wire format

[Andres Freund]

On 2018-03-26 22:44:09 +0200, Damir Simunic wrote:
> > *NONE* of the interesting problems are solved by HTTP2. You *still*
> > need a full blown protocol ontop of it. So no, this doesn't change that.
>
> If you had to nominate only one of those problems, which one would you 
> consider the most interesting?

A few random, very tired, points:

- consolidated message for common tasks:
  - (bind, [describe?,] execute) to reduce overhead of prepared
statement execution (both in messages, as well as branches)
  - (anonymous parse, bind, describe, execute) to make it cheaper to
send statements with out-of-line parameters
- get rid of length limits of individual fields, probably w/ some variable
  length encoding (simple 7 bit?)
- allow *streaming* of large datums
- type-level decisions about binary type transport, right now it's a lot
  of effort (including potentially additional roundtrips), to get the
  few important types to be transported in binary fashion. E.g. floating
  points are really expensive to stringify, bytea as text gets a lot
  bigger etc, but a lot of other types don't benefit a lot
- annotate COMMIT, PREPARE TRANSACTION, COMMIT PREPARED with LSN of
  associated WAL record
- have a less insane cancellation handling
- nested table support

Greetings,

Andres Freund

new buildfarm with gcc & clang "trunk" -Werror?

[Fabien COELHO]

Dear devs,

I've been running buildfarm members moonjelly and seawasp which use gcc & 
clang weekly recompiled trunk versions to build postgres branch HEAD for 
about 6 months


These helped uncover bugs in both gcc & clang, which were reported and 
fixed, so there is a benefit on this side.


These compilations currently fail with -Werror. "gcc trunk" generates 44 
warnings about snprintf-related possible truncations, anc "clang trunk" 
generates 68 warnings about undeclared strl* functions.


Would the project feel appropriate to:

 - fix these warnings (other than putting -Wno-format-truncation or
   similar workarounds...).

 - add permanent gcc/clang trunk beasts with -Werror
   (if so, I'd suggest cannonball & seanettle for the names).

The rational is that postgres will need to be compilable with future 
versions of these compilers, eventually, so the earlier issues are 
detected the better. Also, warnings help detect latent bugs.


However these beasts would be more unstable than others, eg start failing 
when a new warning is added. They are somehow already unstable though, as 
every few month postgres compilation would fail because of a new bug in 
the compilers.


--
Fabien.

Re: new buildfarm with gcc & clang "trunk" -Werror?

[Andres Freund]

Hi,

On 2018-03-28 10:06:31 +0200, Fabien COELHO wrote:
> Would the project feel appropriate to:
> 
>  - fix these warnings (other than putting -Wno-format-truncation or
>similar workarounds...).

Yes, at least for warnings that aren't entirely bogus (see clang's
-Weverything for an extreme example of a lot of bogus stuff).


>  - add permanent gcc/clang trunk beasts with -Werror
>(if so, I'd suggest cannonball & seanettle for the names).

I'm inclined to think that's too noisy.

Greetings,

Andres Freund

Re: Proposal: http2 wire format

[Craig Ringer]

On 28 March 2018 at 00:42, Damir Simunic 
wrote:

>
>
> I'm rapidly losing interest. Unless this goes back toward the concrete and
> practical I think it's going nowhere.
>
>
>
> Your message is exactly what I was hoping for. Thanks for your guidance
> and support, really appreciate you.
>
> Let me now get busy and earn your continued interest and support.
>
>
I spent a lot of time reviewing what you wrote and proposed, looked over
your proof of concept, and offered feedback. Much of which you ignored.
I've been trying to help and took a fair bit of time to do so.

I've outlined what I think needs to happen to push this in a practical
direction.

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

Re: Re: csv format for psql

[Fabien COELHO]

Hello Pavel,


I'd like to convince you to compromise, because otherwise I'm afraid we
will not get this feature.


[...]


The only no-surprise, no-behavioral-change, alternative I see is to have a
csv-specific fieldsep. I'm not keen on that one because it is yet another
variable, one has to remember it, and then it asks the question about
recordsep... and I think there are already too many parameters, and more is
not desirable, although I probably could live with that if I can live with
"fieldsep_zero":-)



I don't share your opinion so introduction csv-specific fieldsep is
surprise less. Current design is wrong - this thread is a evidence.


Wrong is too strong a word. Current design is not perfect, sure. Proposed 
alternatives are somehow worse, at least to some people mind, which 
explains why we arrived on this version after a few iterations.


And if we introduce csv-specific fieldsep, then we multiply this wrong 
design. The fix in this direction is renaming fieldsep to 
fieldsep-unaliagn - but it is probably too big change too. So this 
design is nothing what I can mark as good solution.


Good, we somehow agree on something!

I can live with because it is much better than using pipe as separator 
for csv, and because real impact is small - for almost people it will be 
invisible - but have not good feeling from it.



Are there some possible alternatives?


Given the date and the fact that the cf end is 3 days away, the proposed 
short term alternative is Daniel's version, that I feel is reasonable. Ok, 
people have to do two pset to get comma-separated csv, otherwise they get 
pipe-separated csv in one pset.


You could compromise on that for now, and submit an improvement patch for 
a later version if you wish.


Otherwise, ISTM that the current result of this thread is that there will 
be no simple CSV in pg11:-(


Or maybe I can mark Daniel's latest version as "ready" and point out that 
there is some disagreement on the thread, so it is not a full consensus. 
Then a committer can decide whether it is better in like that or that it 
should be put back in some design stage, possibly with their preference 
wrt to the kind of solution they think best.


--
Fabien.

Re: Proposal: http2 wire format

[Craig Ringer]

On 28 March 2018 at 16:02, Andres Freund  wrote:

> On 2018-03-26 22:44:09 +0200, Damir Simunic wrote:
> > > *NONE* of the interesting problems are solved by HTTP2. You *still*
> > > need a full blown protocol ontop of it. So no, this doesn't change
> that.
> >
> > If you had to nominate only one of those problems, which one would you
> consider the most interesting?
>
> A few random, very tired, points:
>
> - consolidated message for common tasks:
>   - (bind, [describe?,] execute) to reduce overhead of prepared
> statement execution (both in messages, as well as branches)
>   - (anonymous parse, bind, describe, execute) to make it cheaper to
> send statements with out-of-line parameters
> - get rid of length limits of individual fields, probably w/ some variable
>   length encoding (simple 7 bit?)
>

In preparation for the eventually-inevitable 64-bit field sizes, yes.

This should be on the protocol todo wiki.


> - allow *streaming* of large datums


Yes, very much +1 there. That's already on the wiki. Yeah:

* Permit lazy fetches of large values, at least out-of-line TOASTED values
http://www.postgresql.org/message-id/53ff0ef8@2ndquadrant.com


- type-level decisions about binary type transport, right now it's a lot
>   of effort (including potentially additional roundtrips), to get the
>   few important types to be transported in binary fashion. E.g. floating
>   points are really expensive to stringify, bytea as text gets a lot
>   bigger etc, but a lot of other types don't benefit a lot
>

Yeah, as distinct from now, where the client has specify param-by-param,
and where libpq doesn't support mixing text and binary formats in result
sets at all.

Again, needs wiki. I'll add.

- annotate COMMIT, PREPARE TRANSACTION, COMMIT PREPARED with LSN of
>   associated WAL record
>

Already on the wiki, as is the related job of sending the xid of a txn to
the client when one is assigned.


> - have a less insane cancellation handling
>

+100

- nested table support
>
>
Can you elaborate on that one?


A few other points that come to mind for me are:

* labeled result sets (useful for stored procs, etc, as came up recently
with trying to figure out how to let stored procs have OUT params and
multiple result sets)

* room for other resultset formats later. Like Damir, I really want to add
protobuf or json serializations of result sets at some point, mainly so we
can return "entity graphs" in graph representation rather than left-join
projection.

* Robert Haas was talking about some issues relating to sync and the COPY
BOTH protocol a while ago, which we'd want to address.

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

Re: Cast jsonb to numeric, int, float, bool

[Teodor Sigaev]

I think, it should support from/to numeric/bool/text only. If we want to have 
casts to from numeric to other numeric types then it should be full set (int2, 
int4, int8, float4, float8).


I was too optimistic about casting to/from text. It already exists and it works 
by differ way from suggested  casts:


1) select '"foo"'::jsonb::text;  -> "foo"  // with ""
2) select '123'::jsonb::text;-> 123
3) select '123'::jsonb::int4;-> 123

Seems, sometime it would be desirable result in 1) as just
'foo' without "" decoration, but we cannot have 2 different explicit casts for 
the same types. So, I withdraw objections about text casting, but I still 
believe that we need one of two variants:

1) numeric/bool
2) bool, numeric/all variants of numeric types


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: Proposal: http2 wire format

[Tatsuo Ishii]

> A few random, very tired, points:
> 
> - consolidated message for common tasks:
>   - (bind, [describe?,] execute) to reduce overhead of prepared
> statement execution (both in messages, as well as branches)
>   - (anonymous parse, bind, describe, execute) to make it cheaper to
> send statements with out-of-line parameters
> - get rid of length limits of individual fields, probably w/ some variable
>   length encoding (simple 7 bit?)
> - allow *streaming* of large datums
> - type-level decisions about binary type transport, right now it's a lot
>   of effort (including potentially additional roundtrips), to get the
>   few important types to be transported in binary fashion. E.g. floating
>   points are really expensive to stringify, bytea as text gets a lot
>   bigger etc, but a lot of other types don't benefit a lot
> - annotate COMMIT, PREPARE TRANSACTION, COMMIT PREPARED with LSN of
>   associated WAL record
> - have a less insane cancellation handling
> - nested table support

I would like to have portal/statement name to be added to response
messages (i.e. parse complete, bind complete, close complete, and
command complete.). Currently it's not easy to recognize which
response corresponds to which message, which makes certain
applications such as Pgpool-II hard to implement and inefficient.

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

Re: new function for tsquery creartion

[Aleksander Alekseev]

Hello Dmitry,

> A few gotchas:
> 
> I haven't touched gettoken_tsvector() yet. As a result, the following
> queries produce errors:
> 
> select websearch_to_tsquery('simple', );
> ERROR:  syntax error in tsquery: "'"
> 
> select websearch_to_tsquery('simple', '\');
> ERROR:  there is no escaped character: "\"
> 
> Maybe there's more. The question is: should we fix those, or it's fine as it
> is? I don't have a strong opinion about this.

It doesn't sound right to me to accept any input as a general rule but
sometimes return errors nevertheless. That API would be complicated for
the users. Thus I suggest to accept any garbage and try our best to
interpret it.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: Oddity in COPY FROM handling of check constraints on partition tables

[Ashutosh Bapat]

On Wed, Mar 28, 2018 at 6:58 AM, Amit Langote
 wrote:
>> which violates the constraint on the column b (ie, b > 0), so this
>> should abort.  The reason for that is because CopyFrom looks at the
>> parent relation's constraints, not the partition's constraints, when
>> checking the constraint against the input row.
>
> Good catch, thanks!
>

+1

>> Attached is a patch for fixing this issue.
>
> That looks good to me.  This one would need to be back-patched to v10.
Thanks. Please add to the next commitfest so that it doesn't get lost.
We can not add this to v11 open items since it isn't a v11 bug
exactly.


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

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 28 March 2018 at 22:29, Amit Langote  wrote:
> Also, I have redesigned how we derive partition indexes after running
> pruning steps.  Previously, for each step we'd determine the indexes of
> "partitions" that are not pruned leading to a list partition not being
> pruned sometimes, as shown in the two recent examples.  Instead, in the
> new approach, we only keep track of the indexes of the "datums" that
> satisfy individual pruning steps (both base pruning steps and combine
> steps) and only figure out the partition indexes after we've determined
> set of datums that survive all pruning steps.  That is, after we're done
> executing all pruning steps.  Whether we need to scan special partitions
> like null-only and default partition is tracked along with datum indexes
> for each step.  With this change, pruning works as expected in both examples:

Smart thinking! Good to see that solved.

I'll try to look at v43 during my working day tomorrow, in around 9 hours time.

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

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Wed, 28 Mar 2018 15:45:09 +0900 (JST)
Tatsuo Ishii  wrote:

> >> I found the previous patch was broken and this can't handle
> >> views that has subqueries as bellow;
> >> 
> >> 　CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub;
> >> 
> >> I fixed this and attached the updated version including additional tests.
> > 
> > This patch gives a warning while compiling:
> > 
> > lockcmds.c:186:1: warning: no semicolon at end of struct or union
> >  } LockViewRecurse_context;
> >  ^
> 
> Also I get a regression test failure:

Thank you for your reviewing my patch.
I attached the updated patch, v10.

Regards,

> 
> *** 
> /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/lock.out
> 2018-03-28 15:24:13.805314577 +0900
> --- /usr/local/src/pgsql/current/postgresql/src/test/regress/results/lock.out 
> 2018-03-28 15:42:07.975592594 +0900
> ***
> *** 118,124 
>   
>lock_tbl1
>lock_view6
> ! (2 rows)
>   
>   ROLLBACK;
>   -- Verify that we can lock a table with inheritance children.
> --- 118,125 
>   
>lock_tbl1
>lock_view6
> !  mvtest_tm
> ! (3 rows)
> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..96d477c 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   When a view is specified to be locked, all relations appearing in the view
+   definition query are also locked recursively with the same lock mode. 
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..e8a8877 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -23,11 +23,15 @@
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
+#include "nodes/nodeFuncs.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
 
 /*
  * LOCK TABLE
@@ -62,8 +66,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) &lockstmt->mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 }
@@ -107,7 +115,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
  * multiply-inheriting children more than once, but that's no problem.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 {
 	List	   *children;
 	ListCell   *lc;
@@ -120,7 +128,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 		AclResult	aclresult;
 
 		/* Check permissions before acquiring the lock. */
-		aclresult = LockTableAclCheck(childreloid, lockmode);
+		aclresult = LockTableAclCheck(childreloid, lockmode, userid);
 		if (aclresult != ACLCHECK_OK)
 		{
 			char	   *relname = get_rel_name(childreloid);
@@ -157,15 +165,120 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 			continue;
 		}
 
-	

Re: [HACKERS] A design for amcheck heapam verification

[Pavan Deolasee]

On Wed, Mar 28, 2018 at 2:48 AM, Peter Geoghegan  wrote:

> On Tue, Mar 27, 2018 at 6:48 AM, Pavan Deolasee
>  wrote:
> > + * When index-to-heap verification is requested, a Bloom filter is used
> to
> > + * fingerprint all tuples in the target index, as the index is
> traversed to
> > + * verify its structure.  A heap scan later verifies the presence in the
> > heap
> > + * of all index tuples fingerprinted within the Bloom filter.
> > + *
> >
> > Is that correct? Aren't we actually verifying the presence in the index
> of
> > all
> > heap tuples?
>
> I think that you could describe it either way. We're verifying the
> presence of heap tuples in the heap that ought to have been in the
> index (that is, most of those that were fingerprinted).
>

Hmm Ok. It still sounds backwards to me, but then English is not my first
or even second language. "A heap scan later verifies the presence in the
heap of all index tuples fingerprinted" sounds as if we expect to find all
fingerprinted index tuples in the heap. But in reality, we check if all
heap tuples have a fingerprinted index tuple.


>
> You're right - there is a narrow window for REPEATABLE READ and
> SERIALIZABLE transactions. This is a regression in v6, the version
> removed the TransactionXmin test.
>
> I am tempted to fix this by calling GetLatestSnapshot() instead of
> GetTransactionSnapshot(). However, that has a problem of its own -- it
> won't work in parallel mode, and we're dealing with parallel
> restricted functions, not parallel unsafe functions. I don't want to
> change that to fix such a narrow issue. IMV, a better fix is to treat
> this as a serialization failure. Attached patch, which applies on top
> of v7, shows what I mean.
>

Ok. I am happy with that.


>
> I think that this bug is practically impossible to hit, because we use
> the xmin from the pg_index tuple during "is index safe to use?"
> indcheckxmin/TransactionXmin checks (the patch that I attached adds a
> similar though distinct check), which raises another question for a
> REPEATABLE READ xact. That question is: How is a REPEATABLE READ
> transaction supposed to see the pg_index entry to get the index
> relation's oid to call a verification function in the first place?


Well pg_index entry will be visible and should be visible. Otherwise how do
you even maintain a newly created index. I am not sure, but I guess we take
fresh MVCC snapshots for catalog searches, even in RR transactions.


> My
> point is that there is no need for a more complicated solution than
> what I propose.
>

I agree on that.


>
> I don't think so. The way we compute OldestXmin for
> IndexBuildHeapRangeScan() is rather like a snapshot acquisition --
> GetOldestXmin() locks the proc array in shared mode. As you pointed
> out, the fact that it comes after everything else (fingerprinting)
> means that it must be equal to or later than what index scans saw,
> that allowed them to do the kill_prior_tuple() stuff (set LP_DEAD
> bits).
>
>
That's probably true.


>
> > Are there any interesting cases around INSERT_IN_PROGRESS/DELETE_IN_
> PROGRESS
> > tuples, especially if those tuples were inserted/deleted by our own
> > transaction? It probably worth thinking.
>

Anything here that you would like to check? I understand that you may see
such tuples only for catalog tables.


>
> IndexBuildHeapRangeScan() doesn't mention anything about CIC's heap
> ShareUpdateExclusiveLock (it just says SharedLock), because that lock
> strength doesn't have anything to do with IndexBuildHeapRangeScan()
> when it operates with an MVCC snapshot. I think that this means that
> this patch doesn't need to update comments within
> IndexBuildHeapRangeScan(). Perhaps that's a good idea, but it seems
> independent.
>

Ok, I agree. But note that we are now invoking that code
with AccessShareLock() whereas the existing callers either use ShareLock or
ShareUpdateExclusiveLock. That's probably does not matter, but it's a
change worth noting.


>
> > Is
> > there anything we can do to lessen that burden like telling other
> backends
> > to
> > ignore our xmin while computing OldestXmin (like vacuum does)?
>
> I don't think so. The transaction involved is still an ordinary user
> transaction.
>

While that's true, I am thinking if there is anything we can do to stop
this a consistency-checking utility to create other non-trivial side
effects on the state of the database, even if that means we can not check
all heap tuples. For example, can there be a way by which we allow
concurrent vacuum or HOT prune to continue to prune away dead tuples, even
if that means running bt_check_index() is some specialised way (such as not
allowing in a transaction block) and the heap scan  might miss out some
tuples? I don't know answer to that, but given that sometimes bt_check_index()
may take hours if not days to finish, it seems worth thinking or at least
documenting the side-effects somewhere.

Thanks,
Pavan

-- 
 Pavan Deolasee   http://w

Re: Oddity in COPY FROM handling of check constraints on partition tables

[Etsuro Fujita]

(2018/03/28 10:28), Amit Langote wrote:
>> Attached is a patch for fixing this issue.
> 
> That looks good to me.  This one would need to be back-patched to v10.

Thanks for the review!

Best regards,
Etsuro Fujita

Re: Oddity in COPY FROM handling of check constraints on partition tables

[Etsuro Fujita]

(2018/03/28 18:51), Ashutosh Bapat wrote:

On Wed, Mar 28, 2018 at 6:58 AM, Amit Langote
  wrote:



Attached is a patch for fixing this issue.


That looks good to me.  This one would need to be back-patched to v10.

Thanks. Please add to the next commitfest so that it doesn't get lost.
We can not add this to v11 open items since it isn't a v11 bug
exactly.


OK, done.

Best regards,
Etsuro Fujita

Re: Parallel Aggregates for string_agg and array_agg

[Tomas Vondra]

On 03/28/2018 05:28 AM, Tom Lane wrote:
> Tomas Vondra  writes:
>> On 03/27/2018 04:51 AM, David Rowley wrote:
>>> Seems I didn't mean "trans types". I should have said aggregate
>>> function argument types.
> 
>> I'm not sure that's better than the check proposed by Tom. An argument
>> type without send/receive function does not necessarily mean we can't
>> serialize/deserialize the trans value. Because who says the argument
>> value will be embedded in the trans value?
> 
> In general we would not know that, but *for these specific serial/
> deserial functions*, we know exactly what they will do.  Also, IIRC,
> the trans type is declared as INTERNAL, so we don't really have any
> hope of identifying the behavior by inspecting that type declaration.
> 

Sure, which is why I thought your solution was better, as it was
targeted at those particular functions.

> Getting a solution that would work for other polymorphic serialization
> functions seems like a bit of a research project to me.  In the meantime,
> I think David's right that what we need to look at is the actual input
> type of the aggregate, and then assume that what's to be serialized is
> an array of that.  Conceivably an aggregate could be built that uses
> these serial/deserial functions and yet its input type is something else
> than what it constructs an array of ... but I find it a bit hard to
> wrap my brain around what that would be exactly.
> 

But David's fix doesn't check the aggregate to produce an array of the
input type (or anyarray). It could easily be an aggregate computing a
bloom filter or something like that, which has no such issues in the
serial/deserial functions.

Also, if it's checking aggref->aggargtypes, it'll reject anyelement
parameters, no?

regards

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

Re: Jsonb transform for pl/python

[Peter Eisentraut]

On 3/21/18 18:50, Peter Eisentraut wrote:
> On 3/12/18 11:26, Nikita Glukhov wrote:
>> I have reviewed this patch.  Attached new 6th version of the patch with
>> v5-v6 delta-patch.
> 
> Thanks for the update.  I'm working on committing this.

Committed.

Everything seemed to work well.  I just did a bit of cosmetic cleanups.
I did a fair amount of tweaking on the naming of functions, the
extensions and library names, etc., to make it look like the existing
plpython transforms.  I also changed it so that the transform would
support mappings and sequences other than dict and list.  I removed the
non-ASCII test and the test with the huge numbers.

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

Re: [HACKERS] A design for amcheck heapam verification

[Pavan Deolasee]

On Wed, Mar 28, 2018 at 2:48 AM, Peter Geoghegan  wrote:

>
> I don't think so. The transaction involved is still an ordinary user
> transaction.
>
>
Mostly a nitpick, but I guess we should leave a comment
after IndexBuildHeapScan() saying heap_endscan() is not necessary
since IndexBuildHeapScan()
does that internally. I stumbled upon that while looking for any potential
leaks. I know at least one other caller of IndexBuildHeapScan() doesn't
bother to say anything either, but it's helpful.

FWIW I also looked at the 0001 patch and it looks fine to me.

Thanks,
Pavan
-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 3/27/18 20:43, Tomas Vondra wrote:
>>> 3) utility.c
>>>
>>> I find this condition rather confusing:
>>>
>>> (!(context == PROCESS_UTILITY_TOPLEVEL ||
>>>context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
>>>IsTransactionBlock())
>>>
>>> I mean, negated || with another || - it took me a while to parse what
>>> that means. I suggest doing this instead:
>>>
>>> #define ProcessUtilityIsAtomic(context)\
>>>(!(context == PROCESS_UTILITY_TOPLEVEL ||
>>>   context == PROCESS_UTILITY_QUERY_NONATOMIC))
>>>
>>> (ProcessUtilityIsAtomic(context) || IsTransactionBlock())
>> fixed
>>
> Ummm, I still see the original code here.

I put the formula into a separate variable isAtomicContext instead of
repeating it twice.  I think that makes it clearer.  I'm not sure
splitting it up like above makes it better, because the
IsTransactionBlock() is part of the "is atomic".  Maybe adding a comment
would make it clearer.

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

Re: PL/pgSQL nested CALL with transactions

[Tomas Vondra]

On 03/28/2018 02:54 PM, Peter Eisentraut wrote:
> On 3/27/18 20:43, Tomas Vondra wrote:
 3) utility.c

 I find this condition rather confusing:

 (!(context == PROCESS_UTILITY_TOPLEVEL ||
context == PROCESS_UTILITY_QUERY_NONATOMIC) ||
IsTransactionBlock())

 I mean, negated || with another || - it took me a while to parse what
 that means. I suggest doing this instead:

 #define ProcessUtilityIsAtomic(context)\
(!(context == PROCESS_UTILITY_TOPLEVEL ||
   context == PROCESS_UTILITY_QUERY_NONATOMIC))

 (ProcessUtilityIsAtomic(context) || IsTransactionBlock())
>>> fixed
>>>
>> Ummm, I still see the original code here.
> 
> I put the formula into a separate variable isAtomicContext instead of
> repeating it twice.  I think that makes it clearer.  I'm not sure
> splitting it up like above makes it better, because the
> IsTransactionBlock() is part of the "is atomic".  Maybe adding a comment
> would make it clearer.
> 

I see. I thought "fixed" means you adopted the #define, but that's not
the case. I don't think an extra comment is needed - the variable name
is descriptive enough IMHO.

regards

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

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

> On Wed, 28 Mar 2018 15:45:09 +0900 (JST)
> Tatsuo Ishii  wrote:
> 
>> >> I found the previous patch was broken and this can't handle
>> >> views that has subqueries as bellow;
>> >> 
>> >> 　CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub;
>> >> 
>> >> I fixed this and attached the updated version including additional tests.
>> > 
>> > This patch gives a warning while compiling:
>> > 
>> > lockcmds.c:186:1: warning: no semicolon at end of struct or union
>> >  } LockViewRecurse_context;
>> >  ^
>> 
>> Also I get a regression test failure:
> 
> Thank you for your reviewing my patch.
> I attached the updated patch, v10.

Thanks. Looks good to me. I marked the patch as "Ready for Committer".
Unless there's an objection, especially from Robert or Thomas Munro, I
am going to commit/push it.

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

Re: [HACKERS] [PATCH] Incremental sort

[Teodor Sigaev]

BTW, patch had conflicts with master.  Please, find rebased version attached.


Sorry, but patch conflicts with master again.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: WIP: Covering + unique indexes.

[Andrey Borodin]

Hi!

> 21 марта 2018 г., в 21:51, Alexander Korotkov  
> написал(а):
> 
> 
> I took a look at this patchset.  I have some notes about it.
> 
> * I see patch changes dblink, amcheck and tcl contribs.  It would be nice to 
> add corresponding
> check to dblink and amcheck regression tests.  It would be good to do the 
> same with tcn contrib.
> But tcn doesn't have regression tests at all.  And it's out of scope of this 
> patch to add regression
> tests to tcn.  So, it's OK to just check that it's working correctly with 
> covering indexes (I hope it's
> already done by other reviewers).
> 
I propose attached tests to amcheck and dblink. Not very extensive tests 
though, but enough to keep things working.
> * I think that subscription regression tests in src/test/subscription should 
> have some use
> of covering indexes.  Logical decoding and subscription are heavily using 
> primary keys.
> So they need to be tested to work correctly with covering indexes.
I've attached subscription tests. Unfortunately, they crash publisher with
2018-03-28 15:09:05.953 +05 [81805] 001_rep_changes.pl LOG:  statement: DELETE 
FROM tab_cov WHERE a > 20
2018-03-28 15:09:05.954 +05 [81691] LOG:  server process (PID 81805) was 
terminated by signal 11: Segmentation fault
Any of this commands lead to this
$node_publisher->safe_psql('postgres', "DELETE FROM tab_cov WHERE a > 20");
$node_publisher->safe_psql('postgres', "UPDATE tab_cov SET a = -a");


I didn't succeed in debugging. Maybe Anastasia can comment on is it bug or is 
it something wrong with tests?
> 
> * I also think some isolation tests in src/test/isolation need to check 
> covering indexes too.
> In particular insert-conflict-*.spec and lock-*.spec and probably more.
Currently, I couldn't compose good test scenarios, but I will think a bit about 
it more.

Best regards, Andrey Borodin.


0001-Tests-of-covering-indexes-in-amcheck.patch
Description: Binary data


0002-Tests-for-dblink-with-covering-indexes.patch
Description: Binary data


0003-Tests-for-subsciptions-with-sovering-indexes.patch
Description: Binary data

Re: [HACKERS] [PATCH] Incremental sort

[Teodor Sigaev]

BTW, patch had conflicts with master.  Please, find rebased version attached.


Despite by patch conflist patch looks commitable, has anybody objections to 
commit it?


Patch recieved several rounds of review during 2 years, and seems to me, keeping 
it out from sources may cause a lost it. Although it suggests performance 
improvement in rather wide usecases.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: [HACKERS] [PATCH] Incremental sort

[Tomas Vondra]

On 03/28/2018 03:28 PM, Teodor Sigaev wrote:
>> BTW, patch had conflicts with master.  Please, find rebased version
>> attached.
> 
> Despite by patch conflist patch looks commitable, has anybody objections
> to commit it?
> 
> Patch recieved several rounds of review during 2 years, and seems to me,
> keeping it out from sources may cause a lost it. Although it suggests
> performance improvement in rather wide usecases.
> 

No objections from me - if you want me to do one final round of review
after the rebase (not sure how invasive it'll turn out), let me know.

BTW one detail I'd change is name of the GUC variable. enable_incsort
seems unnecessarily terse - let's go for enable_incremental_sort or
something like that.

regards


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

Re: [HACKERS] Partition-wise aggregation/grouping

[Ashutosh Bapat]

On Tue, Mar 27, 2018 at 2:43 PM, Jeevan Chalke
 wrote:

>>  else if (IS_UPPER_REL(foreignrel))
>>  {
>>  PgFdwRelationInfo *ofpinfo;
>> -PathTarget *ptarget =
>> root->upper_targets[UPPERREL_GROUP_AGG];
>> +PathTarget *ptarget = fpinfo->grouped_target;
>>
>> I think we need an assert there to make sure that the upper relation is a
>> grouping relation. That way any future push down will notice it.
>
>
> I am not sure on what we should Assetrt here. Note that we end-up here only
> when doing grouping, and thus I don't think we need any Assert here.
> Let me know if I missed anything.

Since we are just checking whether it's an upper relation and directly
using root->upper_targets[UPPERREL_GROUP_AGG], I thought we could add
an assert to verify that it's really the grouping rel we are dealing
with. But I guess, we can't really check that from given relation. But
then for a grouped rel we can get its target from RelOptInfo. So, we
shouldn't need to root->upper_targets[UPPERREL_GROUP_AGG]. Am I
missing something? For other upper relations we do not set the target
yet but then we could assert that there exists one in the grouped
relation.

>
>>
>>
>> -get_agg_clause_costs(root, (Node *)
>> root->parse->havingQual,
>> +get_agg_clause_costs(root, fpinfo->havingQual,
>>   AGGSPLIT_SIMPLE, &aggcosts);
>>  }
>> Should we pass agg costs as well through GroupPathExtraData to avoid
>> calculating it again in this function?
>
>
> Adding an extra member in GroupPathExtraData just for FDW does not look good
> to me.
> But yes, if we do that, then we can save this calculation.
> Let me know if its OK to have an extra member for just FDW use, will prepare
> a separate patch for that.

I think that should be fine. A separate patch would be good, so that a
committer can decide whether or not to include it.



>>
>> +Node   *havingQual;
>> I am wondering whether we could use remote_conds member for storing this.
>
>
> This havingQual is later checked for shippability and classified into
> pushable and non-pushable quals and stored in remote_conds and local_conds
> respectively.
> Storing it directly in remote_conds and then splitting it does not look good
> to me.
> Also, remote_conds is list of RestrictInfo nodes whereas havingQual is not.
> So using that for storing havingQual does not make sense. So better to have
> a separate member in PgFdwRelationInfo.

Ah sorry, I was wrong about remote_conds. remote_conds and local_conds
are basically the conditions on the relation being pushed down.
havingQuals are conditions on a grouped relation so treating them like
baserestrictinfo or join conditions looks more straight forward,
rather than having a separate member in PgFdwRelationInfo. So, remote
havingQuals go into remote_conds and local havingQuals go to
local_conds.

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

Re: [PROPOSAL] Shared Ispell dictionaries

[Arthur Zakirov]

Please find the attached new version of the patch.

I got rid of 0005 and 0006 parts. There are no
max_shared_dictionaries_size variable, Shareable option,
pg_ts_shared_dictionaries view anymore.

On Sat, Mar 24, 2018 at 04:56:36PM -0400, Tom Lane wrote:
> I do think it's required that changing the dictionary's options with
> ALTER TEXT SEARCH DICTIONARY automatically cause a reload; but if that's
> happening with this patch, I don't see where.  (It might work to use
> the combination of dictionary OID and TID of the dictionary's pg_ts_dict
> tuple as the lookup key for shared dictionaries.  Oh, and have you
> thought about the possibility of conflicting OIDs in different DBs?
> Probably the database OID has to be part of the key, as well.)

The database OID, the dictionary OID, TID and XMIN are used now as
lookup key.

> Also, the scheme for releasing the dictionary DSM during
> RemoveTSDictionaryById is uncertain and full of race conditions:
> the DROP might roll back later, or someone might come along and
> start using the dictionary (causing a fresh DSM load) before the
> DROP commits and makes the dictionary invisible to other sessions.
> I don't think that either of those are necessarily fatal objections,
> but there needs to be some commentary there explaining what happens.

The dictionary's DSM segment is alive till postmaster terminates now.
But when the dictionary is dropped or altered then the previous
(invalid) segment is unpinned. The segment itself is released when all
backends unpins mapping in lookup_ts_parser_cache() or by disconnecting.

The problem here comes when the dictionary was used before dropping or
altering by some process, isn't used after and the process lives a very
long time. In this situation the mapping isn't unpinned and the segment
isn't released. The other problem is that TsearchDictEntry isn't removed
if ts_dict_shmem_release() wasn't called. It may happen after dropping
the dictionary.

> BTW, I was going to complain that this patch alters the API for
> dictionary template init functions without any documentation updates;
> but then I realized that there isn't any documentation to update.
> That pretty well sucks, but I suppose it's not the job of this patch
> to improve that situation.  Still, you could spend a bit more effort on
> the commentary in ts_public.h in 0002, because that commentary is as
> close to an API spec as we've got.

I improved a little bit the commentary in ts_public.h.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index b9fdd77e19..e071994523 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -78,6 +78,8 @@
 #define tmpalloc(sz)  MemoryContextAlloc(Conf->buildCxt, (sz))
 #define tmpalloc0(sz)  MemoryContextAllocZero(Conf->buildCxt, (sz))
 
+#define tmpstrdup(str) MemoryContextStrdup(Conf->buildCxt, (str))
+
 /*
  * Prepare for constructing an ISpell dictionary.
  *
@@ -498,7 +500,7 @@ NIAddSpell(IspellDict *Conf, const char *word, const char 
*flag)
Conf->Spell[Conf->nspell] = (SPELL *) tmpalloc(SPELLHDRSZ + 
strlen(word) + 1);
strcpy(Conf->Spell[Conf->nspell]->word, word);
Conf->Spell[Conf->nspell]->p.flag = (*flag != '\0')
-   ? cpstrdup(Conf, flag) : VoidString;
+   ? tmpstrdup(flag) : VoidString;
Conf->nspell++;
 }
 
@@ -1040,7 +1042,7 @@ setCompoundAffixFlagValue(IspellDict *Conf, 
CompoundAffixFlag *entry,
entry->flag.i = i;
}
else
-   entry->flag.s = cpstrdup(Conf, s);
+   entry->flag.s = tmpstrdup(s);
 
entry->flagMode = Conf->flagMode;
entry->value = val;
@@ -1536,6 +1538,9 @@ nextline:
return;
 
 isnewformat:
+   pfree(recoded);
+   pfree(pstr);
+
if (oldformat)
ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
diff --git a/contrib/dict_int/dict_int.c b/contrib/dict_int/dict_int.c
index 56ede37089..8dd4959028 100644
--- a/contrib/dict_int/dict_int.c
+++ b/contrib/dict_int/dict_int.c
@@ -30,7 +30,7 @@ PG_FUNCTION_INFO_V1(dintdict_lexize);
 Datum
 dintdict_init(PG_FUNCTION_ARGS)
 {
-   List   *dictoptions = (List *) PG_GETARG_POINTER(0);
+   DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictInt*d;
ListCell   *l;
 
@@ -38,7 +38,7 @@ dintdict_init(PG_FUNCTION_ARGS)
d->maxlen = 6;
d->rejectlong = false;
 
-   foreach(l, dictoptions)
+   foreach(l, init_data->dict_options)
{
DefElem*defel = (DefElem *) lfirst(l);
 
diff --git a/contrib/dict_xsyn/dict_xsyn.c b/contrib/dict_xsyn/dict_xsyn.c
index a79ece240c..0b8a32d459 100644
--- a/contrib/dict_xsyn/dict_xsyn.c
+++ b/contrib/dict_xsyn/dict_xsyn.c
@@ -140,7 +140,7 @@ read_dictionary(DictSyn *d, const char *filename)
 Datum
 dxsyn_init(PG_FUNCTION_ARGS)

segfault due to invalid cached plan

[Nicolas Thauvin]

Hello,

A customer sent us a core dump of the crash of the background worker of
the powa extension, running on 9.6.8 along side with cstore_fdw.

The background worker loops on a plpgsql function, which then execute
another plpgsql function that issues the query "TRUNCATE
powa_statements_history_current".

Here is the backtrace :

#0  0x00603147 in list_copy (oldlist=0x2f106a8) at list.c:1176
#1  0x00603380 in list_difference (list1=list1@entry=0x2f106a8, 
list2=list2@entry=0x0) at list.c:867
#2  0x7f3b1545db1a in CStoreProcessUtility (parseTree=0x2f18108, 
queryString=0x2f17698 "TRUNCATE powa_statements_history_current", 
context=PROCESS_UTILITY_QUERY, paramListInfo=0x0, destReceiver=0xc19fe0 
, 
completionTag=0x7ffe2f207430 "3\001") at cstore_fdw.c:353
#3  0x005eb84e in _SPI_execute_plan (plan=plan@entry=0x2f17ca8, 
paramLI=paramLI@entry=0x0, snapshot=snapshot@entry=0x0, 
crosscheck_snapshot=crosscheck_snapshot@entry=0x0, read_only=read_only@entry=0 
'\000', 
fire_triggers=fire_triggers@entry=1 '\001', tcount=tcount@entry=0) at 
spi.c:2202
#4  0x005ebf58 in SPI_execute_plan_with_paramlist (plan=0x2f17ca8, 
params=0x0, read_only=, tcount=0) at spi.c:452
#5  0x7f3ad716b6e5 in exec_stmt_execsql 
(estate=estate@entry=0x7ffe2f2077f0, stmt=stmt@entry=0x2d99ea8) at 
pl_exec.c:3515
#6  0x7f3ad716ce57 in exec_stmt (stmt=0x2d99ea8, estate=0x7ffe2f2077f0) at 
pl_exec.c:1503
#7  exec_stmts (estate=0x7ffe2f2077f0, stmts=) at pl_exec.c:1398
#8  0x7f3ad716ec8b in exec_stmt_block (estate=estate@entry=0x7ffe2f2077f0, 
block=0x2f091b8) at pl_exec.c:1336
#9  0x7f3ad716eeab in plpgsql_exec_function (func=func@entry=0x2d8b738, 
fcinfo=fcinfo@entry=0x2f25628, simple_eval_estate=simple_eval_estate@entry=0x0) 
at pl_exec.c:434
#10 0x7f3ad7163db7 in plpgsql_call_handler (fcinfo=0x2f25628) at 
pl_handler.c:255
#11 0x005c55a2 in ExecMakeFunctionResultNoSets (fcache=0x2f255b8, 
econtext=0x2f253c8, isNull=0x2f25f40 "", isDone=) at 
execQual.c:2041
#12 0x005cb0ce in ExecTargetList (tupdesc=, 
isDone=0x7ffe2f207b34, itemIsDone=0x2f26058, isnull=0x2f25f40 "", 
values=0x2f25f28, econtext=0x2f253c8, targetlist=0x2f26028) at execQual.c:5423
#13 ExecProject (projInfo=, isDone=isDone@entry=0x7ffe2f207b34) 
at execQual.c:5647
#14 0x005e0162 in ExecResult (node=node@entry=0x2f252b8) at 
nodeResult.c:155
#15 0x005c4678 in ExecProcNode (node=node@entry=0x2f252b8) at 
execProcnode.c:392
#16 0x005c09d7 in ExecutePlan (dest=0xc19fe0 , 
direction=, numberTuples=0, sendTuples=1 '\001', 
operation=CMD_SELECT, use_parallel_mode=, planstate=0x2f252b8, 
estate=0x2f251a8)
at execMain.c:1566
#17 standard_ExecutorRun (queryDesc=0x2f04528, direction=, 
count=0) at execMain.c:338
#18 0x7f3b15c7c0a5 in pgss_ExecutorRun (queryDesc=0x2f04528, 
direction=ForwardScanDirection, count=0) at pg_stat_statements.c:877
#19 0x005ebbdf in _SPI_pquery (tcount=0, fire_triggers=1 '\001', 
queryDesc=0x2f04528) at spi.c:2418
#20 _SPI_execute_plan (plan=plan@entry=0x7ffe2f207e40, 
paramLI=paramLI@entry=0x0, snapshot=snapshot@entry=0x0, 
crosscheck_snapshot=crosscheck_snapshot@entry=0x0, read_only=read_only@entry=0 
'\000', 
fire_triggers=fire_triggers@entry=1 '\001', tcount=tcount@entry=0) at 
spi.c:2194
#21 0x005ebe01 in SPI_execute (src=src@entry=0x2da7818 "SELECT 
powa_statements_aggregate()", read_only=, tcount=tcount@entry=0) 
at spi.c:389
#22 0x7f3ad716d9aa in exec_stmt_dynexecute (stmt=0x2cfc520, 
estate=0x7ffe2f208660) at pl_exec.c:3707
#23 exec_stmt (stmt=0x2cfc520, estate=0x7ffe2f208660) at pl_exec.c:1507
#24 exec_stmts (estate=0x7ffe2f208660, stmts=) at pl_exec.c:1398
#25 0x7f3ad716eb29 in exec_stmt_block (estate=estate@entry=0x7ffe2f208660, 
block=block@entry=0x2cfd450) at pl_exec.c:1197
#26 0x7f3ad716ca21 in exec_stmt (stmt=0x2cfd450, estate=0x7ffe2f208660) at 
pl_exec.c:1431
#27 exec_stmts (estate=0x7ffe2f208660, stmts=) at pl_exec.c:1398
#28 0x7f3ad716fda1 in exec_for_query (estate=estate@entry=0x7ffe2f208660, 
stmt=stmt@entry=0x2cfbe60, portal=0x2d59e08, prefetch_ok=prefetch_ok@entry=1 
'\001') at pl_exec.c:5209
#29 0x7f3ad716c5ea in exec_stmt_fors (stmt=0x2cfbe60, 
estate=0x7ffe2f208660) at pl_exec.c:2116
#30 exec_stmt (stmt=0x2cfbe60, estate=0x7ffe2f208660) at pl_exec.c:1467
#31 exec_stmts (estate=0x7ffe2f208660, stmts=) at pl_exec.c:1398
#32 0x7f3ad716dac8 in exec_stmt_if (stmt=0x2cfd6f0, estate=0x7ffe2f208660) 
at pl_exec.c:1699
#33 exec_stmt (stmt=0x2cfd6f0, estate=0x7ffe2f208660) at pl_exec.c:1447
#34 exec_stmts (estate=0x7ffe2f208660, stmts=) at pl_exec.c:1398
#35 0x7f3ad716ec8b in exec_stmt_block (estate=estate@entry=0x7ffe2f208660, 
block=0x2cffce0) at pl_exec.c:1336
#36 0x7f3ad716eeab in plpgsql_exec_function (func=func@entry=0x2cbf0b8, 
fcinfo=fcinfo@entry=0x31a7c88, simple_eval_estate=simple_eval_estate@entry=0x0) 
at pl_exec.c:434
#37 0x7f3ad7163db7 in plpgsql_call_handler (fcinfo=0x

Re: Parallel Aggregates for string_agg and array_agg

[Tom Lane]

Tomas Vondra  writes:
> On 03/28/2018 05:28 AM, Tom Lane wrote:
>> Getting a solution that would work for other polymorphic serialization
>> functions seems like a bit of a research project to me.  In the meantime,
>> I think David's right that what we need to look at is the actual input
>> type of the aggregate, and then assume that what's to be serialized is
>> an array of that.  Conceivably an aggregate could be built that uses
>> these serial/deserial functions and yet its input type is something else
>> than what it constructs an array of ... but I find it a bit hard to
>> wrap my brain around what that would be exactly.

> But David's fix doesn't check the aggregate to produce an array of the
> input type (or anyarray). It could easily be an aggregate computing a
> bloom filter or something like that, which has no such issues in the
> serial/deserial functions.

Oh, if he's not restricting it to these serialization functions, I agree
that seems wrong.  I thought the discussion was about what to do after
checking the functions.

> Also, if it's checking aggref->aggargtypes, it'll reject anyelement
> parameters, no?

I had in mind to look at exprType() of the argument.

regards, tom lane

Re: Parallel Aggregates for string_agg and array_agg

[David Rowley]

On 28 March 2018 at 03:58, Tom Lane  wrote:
> David Rowley  writes:
>> On 27 March 2018 at 13:26, Alvaro Herrera  wrote:
>>> synchronized_seqscans is another piece of precedent in the area, FWIW.
>
>> This is true. I guess the order of aggregation could be made more
>> certain if we remove the cost based optimiser completely, and just
>> rely on a syntax based optimiser.
>
> None of this is responding to my point.  I think the number of people
> who actually don't care about aggregation order for these aggregates
> is negligible, and none of you have argued against that; you've instead
> selected straw men to attack.

I think what everyone else is getting at is that we're not offering to
improve the performance of the people who care about the order which
the values are aggregated. This patch only offers a possible
performance benefit to those who don't. I mentioned and linked to a
thread about someone from PostGIS asking for this, which as far as I
understand has quite a large user base. You've either ignored that or
think the number of people using PostGIS is negligible.

As far as I understand your argument, it's about there being a
possibility a group of people existing who rely on the aggregation
order being defined without an ORDER BY in the aggregate function.
Unfortunately, It appears from the responses from many of the other's
who voiced an opinion about this is that there is no shortage of other
reasons why relying on values being aggregated in a defined order
without an ORDER BY in the aggregate function arguments is a dangerous
assumption to make.  Several reasons were listed why this is undefined
and I mentioned the great lengths we'd need to go to do make the order
more defined without an explicit ORDER BY, and I still imagine I'd
have missed some of the reasons.  I imagine the number of people which
rely on the order being defined without an ORDER BY is diminishing
each release as we add parallelism support for more node types.  Both
Andres and I agree that it's a shame to block useful optimisations due
to the needs of a small diminishing group of people who are not very
good at reading our carefully crafted documentation... for the past 8
years [1].

I imagine this small group of people, if they do exist, must slowly be
waking up to the fact that we've been devising new ways to ruin their
query results for many years now.  It seems pretty strange for us to
call a truce now after we've been wreaking havoc on this group for so
many releases... I really do hope this part is not true, but if such a
person appeared in -novice or -general asking for help, we'd be
telling them to add an ORDER BY, and we'd be quoting the 8-year-old
line in the documents which states that we make no guarantees in this
area in the absence of an ORDER BY.  If they truly have no other
choice then we might consider suggesting they may get what they want
if they disable parallel query, and we'd probably rhyme off a few
other reasons why it might suddenly break on them again.

[1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=doc/src/sgml/func.sgml;h=7d6125c97e5203c9d092ceec3aaf351c1a5fcf1b;hp=f2906cc82230150f72353609e9c831e90dcc10ca;hb=34d26872ed816b299eef2fa4240d55316697f42d;hpb=6a6efb964092902bf53965649c3ed78b1868b37e


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

Re: Proposal: http2 wire format

[Tom Lane]

Andres Freund  writes:
> A few random, very tired, points:

> - consolidated message for common tasks:
>   - (bind, [describe?,] execute) to reduce overhead of prepared
> statement execution (both in messages, as well as branches)
>   - (anonymous parse, bind, describe, execute) to make it cheaper to
> send statements with out-of-line parameters

I do not see a need for this; you can already send those combinations of
messages in a single network packet if you have a mind to.  Tatsuo-san's
point about making it easier to identify which response goes with which
message would improve life for people trying to send multiple messages
in advance of a response, though.

Your other points are sound, except I have no idea what this means:

> - nested table support

regards, tom lane

Re: Parallel Aggregates for string_agg and array_agg

[Tomas Vondra]

On 03/28/2018 03:54 PM, Tom Lane wrote:
> Tomas Vondra  writes:
>> On 03/28/2018 05:28 AM, Tom Lane wrote:
>>> Getting a solution that would work for other polymorphic serialization
>>> functions seems like a bit of a research project to me.  In the meantime,
>>> I think David's right that what we need to look at is the actual input
>>> type of the aggregate, and then assume that what's to be serialized is
>>> an array of that.  Conceivably an aggregate could be built that uses
>>> these serial/deserial functions and yet its input type is something else
>>> than what it constructs an array of ... but I find it a bit hard to
>>> wrap my brain around what that would be exactly.
> 
>> But David's fix doesn't check the aggregate to produce an array of the
>> input type (or anyarray). It could easily be an aggregate computing a
>> bloom filter or something like that, which has no such issues in the
>> serial/deserial functions.
> 
> Oh, if he's not restricting it to these serialization functions, I 
> agree that seems wrong. I thought the discussion was about what to
> do after checking the functions.
> 

Nope, David's patch does not check what the serial/deserial functions
are (and checks aggref->aggargtypes directly, not the exprType thing).

>> Also, if it's checking aggref->aggargtypes, it'll reject anyelement
>> parameters, no?
> 
> I had in mind to look at exprType() of the argument.
> 

Right, I'm fine with that.

regards

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

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Dean Rasheed]

On 28 March 2018 at 01:34, Tomas Vondra  wrote:
> Attached is a patch fixing this. In the end I've decided to keep both
> branches - one handling boolean Vars and one for NOT clauses. I think
> you're right we can only see (NOT var) cases, but I'm not sure about that.
>
> For example, what if an operator does not have a negator? Then we can't
> transform NOT (a AND b) => (NOT a OR NOT b), I guess. So I kept this for
> now, and we can remove this later.
>

OK, but it's going to have to work harder to set "fullmatch"
correctly. If you have a boolean Var clause, which is identical to
"bool_var = true", it ought to add to "eqmatches" if true is found in
the MCV list. Likewise a boolean Var under a NOT clause is identical
to "bool_var = false", so it ought to add to "eqmatches" if false is
found in the MCV list. Both those cases would be easy to handle, if
general NOT support wasn't required, and you just special-cased "NOT
bool_var".

If you're going to handle the general case of an arbitrary clause
under a NOT, then the recursive call to mcv_update_match_bitmap()
would seem to need to know that it's under a NOT (a new "is_not"
parameter?), to invert the logic around adding to "eqmatches". That
applies to other general OpExpr's too -- for example, "NOT (box_var =
?)" won't be rewritten because there is no box_ne operator, but when
mcv_update_match_bitmap() is called recursively with the "box_var =
?", it shouldn't add to "eqmatches", despite this being an EQSEL
operator.

As mentioned before, I think this whole thing only works if
mcv_update_match_bitmap() returns the "eqmatches" list, so that if it
is called recursively, it can be merged with the caller's list. What
isn't immediately obvious to me is what happens to a NOT clause under
another NOT clause, possibly with an AND or OR in-between. Would the
"is_not" parameter just flip back to false again?

There's also an interesting question around the NullTest clause. Since
NULLs are being recorded in the MCV list, shouldn't "IS NULL" be
treated as semantically like an equality clause, and cause that
attribute to be added to "eqmatches" if NULL is found in the MCV list?


> I've also realized that the "fullmatch" flag is somewhat confused,
> because some places interpreted it as "there is equality on each
> attribute" but in fact it also required an actual MCV match.

Yes, I was having similar thoughts. I think "eqmatches" / "fullmatch"
probably just wants to track whether there was an exact comparison on
all the attributes, not whether or not the value was in the MCV list,
because the latter is already available in the "matches" bitmap.
Knowing that complete, exact comparisons happened, and it wasn't in
the MCV list, makes the "(1 - mcv_totalsel)) / otherdistinct" estimate
reasonable.

However, I don't think that tracking "eqmatches" or "fullmatch" is
sufficient for the general case. For example, for other operators like
"!=", "<", "<=", all (or maybe half) the "1 - mcv_totalsel" ought to
count towards the selectivity, plus possibly part of the MCV list
(e.g., for "<=", using the sum of the matching MCV frequencies plus
half the sum of the non-MCV frequencies might be reasonable -- c.f.
scalarineqsel()). For an OR clause, you might want to count the number
of non-MCV matches, because logically each one adds another "(1 -
mcv_totalsel)) / otherdistinct" to the total selectivity. It's not
immediately obvious how that can be made to fit into the current code
structure. Perhaps it could be made to work by tracking the overall
selectivity as it goes along. Or perhaps it could track the
count/proportion of non-MCV matches.

Regards,
Dean

Re: segfault due to invalid cached plan

[Tomas Vondra]

On 03/28/2018 03:54 PM, Nicolas Thauvin wrote:
> Hello,
> 
> A customer sent us a core dump of the crash of the background worker of
> the powa extension, running on 9.6.8 along side with cstore_fdw.
> 
> The background worker loops on a plpgsql function, which then execute
> another plpgsql function that issues the query "TRUNCATE
> powa_statements_history_current".
> 
> Here is the backtrace :
> 
> ...
>
> The core shows that ProcessUtility hook of cstore_fdw produces a
> segfault when trying to copy the relation list of the truncate
> statement plan.
> 
> In frame 2:
> (gdb) p *((TruncateStmt *)parseTree)
> $26 = {type = T_TruncateStmt, relations = 0x2f106a8, restart_seqs = 0 '\000', 
> behavior = DROP_RESTRICT}
> (gdb) p *(((TruncateStmt *)parseTree)->relations)
> $27 = {type = 49574520, length = 0, head = 0x0, tail = 0x2faaab8}
> 
> With this invalid list, standard_ProcessUtility would not have failed
> but the relation would not have been truncated.
> 
> ...
> 
> Could you please give me some pointers to further investigate this
> crash?
> 

Sounds like a bug in CStoreProcessUtility, which is part of cstore_fdw,
not PostgreSQL. So I guess the right place to report the issue is

   https://github.com/citusdata/cstore_fdw


regards

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

Re: [HACKERS] GSoC 2017: weekly progress reports (week 6)

[Dmitry Ivanov]

I'd like to see fastupdate=on in test too, now tests cover only case
without fastupdate. Please, add them.


Here's a couple of tests for pending list (fastupdate = on).

--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 37070b3b72..095b1192cb 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -17,6 +17,7 @@
 #include "access/gin_private.h"
 #include "access/ginxlog.h"
 #include "access/xloginsert.h"
+#include "storage/predicate.h"
 #include "miscadmin.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
@@ -515,6 +516,19 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
 			btree->fillRoot(btree, newrootpg,
 			BufferGetBlockNumber(lbuffer), newlpage,
 			BufferGetBlockNumber(rbuffer), newrpage);
+
+			if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
+			{
+
+PredicateLockPageSplit(btree->index,
+		BufferGetBlockNumber(stack->buffer),
+		BufferGetBlockNumber(lbuffer));
+
+PredicateLockPageSplit(btree->index,
+		BufferGetBlockNumber(stack->buffer),
+		BufferGetBlockNumber(rbuffer));
+			}
+
 		}
 		else
 		{
@@ -524,6 +538,14 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
 			GinPageGetOpaque(newrpage)->rightlink = savedRightLink;
 			GinPageGetOpaque(newlpage)->flags |= GIN_INCOMPLETE_SPLIT;
 			GinPageGetOpaque(newlpage)->rightlink = BufferGetBlockNumber(rbuffer);
+
+			if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
+			{
+
+PredicateLockPageSplit(btree->index,
+		BufferGetBlockNumber(stack->buffer),
+		BufferGetBlockNumber(rbuffer));
+			}
 		}
 
 		/*
diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index f9daaba52e..3fb4fc8264 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -19,6 +19,7 @@
 #include "access/xloginsert.h"
 #include "lib/ilist.h"
 #include "miscadmin.h"
+#include "storage/predicate.h"
 #include "utils/rel.h"
 
 /*
@@ -1423,7 +1424,7 @@ disassembleLeaf(Page page)
  * Any segments that acquire new items are decoded, and the new items are
  * merged with the old items.
  *
- * Returns true if any new items were added. False means they were all
+ * Returns true if any new items were added. false means they were all
  * duplicates of existing items on the page.
  */
 static bool
@@ -1759,7 +1760,7 @@ leafRepackItems(disassembledLeaf *leaf, ItemPointer remaining)
  */
 BlockNumber
 createPostingTree(Relation index, ItemPointerData *items, uint32 nitems,
-  GinStatsData *buildStats)
+  GinStatsData *buildStats, Buffer entrybuffer)
 {
 	BlockNumber blkno;
 	Buffer		buffer;
@@ -1810,6 +1811,12 @@ createPostingTree(Relation index, ItemPointerData *items, uint32 nitems,
 	page = BufferGetPage(buffer);
 	blkno = BufferGetBlockNumber(buffer);
 
+	/*
+	 * Copy a predicate lock from entry tree leaf (containing posting list)
+	 * to  posting tree.
+	 */
+	PredicateLockPageSplit(index, BufferGetBlockNumber(entrybuffer), blkno);
+
 	START_CRIT_SECTION();
 
 	PageRestoreTempPage(tmppage, page);
@@ -1904,6 +1911,7 @@ ginInsertItemPointers(Relation index, BlockNumber rootBlkno,
 		btree.itemptr = insertdata.items[insertdata.curitem];
 		stack = ginFindLeafPage(&btree, false, NULL);
 
+		GinCheckForSerializableConflictIn(btree.index, NULL, stack->buffer);
 		ginInsertValue(&btree, stack, &insertdata, buildStats);
 	}
 }
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 6fe67f346d..63603859bc 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -17,8 +17,10 @@
 #include "access/gin_private.h"
 #include "access/relscan.h"
 #include "miscadmin.h"
+#include "storage/predicate.h"
 #include "utils/datum.h"
 #include "utils/memutils.h"
+#include "utils/rel.h"
 
 /* GUC parameter */
 int			GinFuzzySearchLimit = 0;
@@ -33,11 +35,18 @@ typedef struct pendingPosition
 } pendingPosition;
 
 
+static void
+GinPredicateLockPage(Relation index, BlockNumber blkno, Snapshot snapshot)
+{
+	if (!GinGetUseFastUpdate(index))
+			PredicateLockPage(index, blkno, snapshot);
+}
+
 /*
  * Goes to the next page if current offset is outside of bounds
  */
 static bool
-moveRightIfItNeeded(GinBtreeData *btree, GinBtreeStack *stack)
+moveRightIfItNeeded(GinBtreeData *btree, GinBtreeStack *stack, Snapshot snapshot)
 {
 	Page		page = BufferGetPage(stack->buffer);
 
@@ -73,6 +82,9 @@ scanPostingTree(Relation index, GinScanEntry scanEntry,
 	/* Descend to the leftmost leaf page */
 	stack = ginScanBeginPostingTree(&btree, index, rootPostingTree, snapshot);
 	buffer = stack->buffer;
+
+	GinPredicateLockPage(index, BufferGetBlockNumber(buffer), snapshot);
+
 	IncrBufferRefCount(buffer); /* prevent unpin in freeGinBtreeStack */
 
 	freeGinBtreeStack(stack);
@@ -94,6 +106,8 @@ scanPostingTree(Relation index, GinScanEntry scanEntry,
 			break;/* no 

Re: segfault due to invalid cached plan

[Tom Lane]

Nicolas Thauvin  writes:
> A customer sent us a core dump of the crash of the background worker of
> the powa extension, running on 9.6.8 along side with cstore_fdw.
> The background worker loops on a plpgsql function, which then execute
> another plpgsql function that issues the query "TRUNCATE
> powa_statements_history_current".

I think the odds are very high that this is a cstore_fdw bug.  The
core backend simply doesn't do anything very interesting with TRUNCATE.

regards, tom lane

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Tomas Vondra]

On 03/28/2018 04:12 PM, Dean Rasheed wrote:
> On 28 March 2018 at 01:34, Tomas Vondra  wrote:
>> Attached is a patch fixing this. In the end I've decided to keep both
>> branches - one handling boolean Vars and one for NOT clauses. I think
>> you're right we can only see (NOT var) cases, but I'm not sure about that.
>>
>> For example, what if an operator does not have a negator? Then we can't
>> transform NOT (a AND b) => (NOT a OR NOT b), I guess. So I kept this for
>> now, and we can remove this later.
>>
> 
> OK, but it's going to have to work harder to set "fullmatch"
> correctly. If you have a boolean Var clause, which is identical to
> "bool_var = true", it ought to add to "eqmatches" if true is found in
> the MCV list. Likewise a boolean Var under a NOT clause is identical
> to "bool_var = false", so it ought to add to "eqmatches" if false is
> found in the MCV list. Both those cases would be easy to handle, if
> general NOT support wasn't required, and you just special-cased "NOT
> bool_var".
> 
> If you're going to handle the general case of an arbitrary clause
> under a NOT, then the recursive call to mcv_update_match_bitmap()
> would seem to need to know that it's under a NOT (a new "is_not"
> parameter?), to invert the logic around adding to "eqmatches". That
> applies to other general OpExpr's too -- for example, "NOT (box_var =
> ?)" won't be rewritten because there is no box_ne operator, but when
> mcv_update_match_bitmap() is called recursively with the "box_var =
> ?", it shouldn't add to "eqmatches", despite this being an EQSEL
> operator.
> 
> As mentioned before, I think this whole thing only works if
> mcv_update_match_bitmap() returns the "eqmatches" list, so that if it
> is called recursively, it can be merged with the caller's list. What
> isn't immediately obvious to me is what happens to a NOT clause under
> another NOT clause, possibly with an AND or OR in-between. Would the
> "is_not" parameter just flip back to false again?
> 

After thinking about this a bit more, I'm not sure if updating the info
based on recursive calls makes sense. The fullmatch flag was supposed to
answer a simple question - can there be just a single matching item?

If there are equality conditions on all columns, there can be just a
single matching item - if we have found it in the MCV (i.e. s1 > 0.0),
then we don't need to inspect the non-MCV part.

But handling this in recursive manner breaks this entirely, because with
something like

   (a=1) AND (b=1 OR b=2)

you suddenly can have multiple matching items. Which makes the fullmatch
flag somewhat useless.

So I think we should be looking at top-level equality clauses only, just
like number_of_groups() does.


> There's also an interesting question around the NullTest clause. Since
> NULLs are being recorded in the MCV list, shouldn't "IS NULL" be
> treated as semantically like an equality clause, and cause that
> attribute to be added to "eqmatches" if NULL is found in the MCV list?
> 
> 
>> I've also realized that the "fullmatch" flag is somewhat confused,
>> because some places interpreted it as "there is equality on each
>> attribute" but in fact it also required an actual MCV match.
> 
> Yes, I was having similar thoughts. I think "eqmatches" / "fullmatch"
> probably just wants to track whether there was an exact comparison on
> all the attributes, not whether or not the value was in the MCV list,
> because the latter is already available in the "matches" bitmap.
> Knowing that complete, exact comparisons happened, and it wasn't in
> the MCV list, makes the "(1 - mcv_totalsel)) / otherdistinct" estimate
> reasonable.
> 

I think we can remove the fullmatch flag from mcv_update_bitmap
entirely. All we need to know is the presence of equality clauses and
whether there was a match in MCV (which we know from s1 > 0.0).

> However, I don't think that tracking "eqmatches" or "fullmatch" is
> sufficient for the general case. For example, for other operators like
> "!=", "<", "<=", all (or maybe half) the "1 - mcv_totalsel" ought to
> count towards the selectivity, plus possibly part of the MCV list
> (e.g., for "<=", using the sum of the matching MCV frequencies plus
> half the sum of the non-MCV frequencies might be reasonable -- c.f.
> scalarineqsel()). For an OR clause, you might want to count the number
> of non-MCV matches, because logically each one adds another "(1 -
> mcv_totalsel)) / otherdistinct" to the total selectivity. It's not
> immediately obvious how that can be made to fit into the current code
> structure. Perhaps it could be made to work by tracking the overall
> selectivity as it goes along. Or perhaps it could track the
> count/proportion of non-MCV matches.
> 

Yes, ignoring the non-equality clauses in 0002 is wrong - that's pretty
much why it's WIP and not merged into 0001.

thanks for the feedback

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

Re: segfault due to invalid cached plan

[Nicolas Thauvin]

On Wed, 28 Mar 2018 16:14:04 +0200
Tomas Vondra  wrote:

> On 03/28/2018 03:54 PM, Nicolas Thauvin wrote:
> > Hello,
> > 
> > A customer sent us a core dump of the crash of the background
> > worker of the powa extension, running on 9.6.8 along side with
> > cstore_fdw.
> > 
> > The background worker loops on a plpgsql function, which then
> > execute another plpgsql function that issues the query "TRUNCATE
> > powa_statements_history_current".
> > 
> > Here is the backtrace :
> > 
> > ...
> >
> > The core shows that ProcessUtility hook of cstore_fdw produces a
> > segfault when trying to copy the relation list of the truncate
> > statement plan.
> > 
> > In frame 2:
> > (gdb) p *((TruncateStmt *)parseTree)
> > $26 = {type = T_TruncateStmt, relations = 0x2f106a8, restart_seqs =
> > 0 '\000', behavior = DROP_RESTRICT} (gdb) p *(((TruncateStmt
> > *)parseTree)->relations) $27 = {type = 49574520, length = 0, head =
> > 0x0, tail = 0x2faaab8}
> > 
> > With this invalid list, standard_ProcessUtility would not have
> > failed but the relation would not have been truncated.
> > 
> > ...
> > 
> > Could you please give me some pointers to further investigate this
> > crash?
> >   
> 
> Sounds like a bug in CStoreProcessUtility, which is part of
> cstore_fdw, not PostgreSQL. So I guess the right place to report the
> issue is
> 
>https://github.com/citusdata/cstore_fdw
> 
> 

Having a second look at the code of cstore_fdw, it modifies the data.
I'll investigate more on this code.

Thank you
-- 
Nicolas Thauvin
+33 (0)1 84 16 92 09
http://dalibo.com - http://dalibo.org

Re: [HACKERS] GSoC 2017: weekly progress reports (week 6)

[Dmitry Ivanov]

I'd like to see fastupdate=on in test too, now tests cover only case
without fastupdate. Please, add them.


Here's a couple of tests for pending list (fastupdate = on).


By the way, isn't it strange that permutation "fu1" "rxy3" "wx3" "rxy4" 
"c1" "wy4" "c2" doesn't produce an ERROR?


--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Implementing SQL ASSERTION

[David Fetter]

On Sun, Mar 18, 2018 at 12:29:50PM +, Joe Wildish wrote:
> > 
> >> 
> >> This patch no longer applies.  Any chance of a rebase?
> 
> Attached is a rebased version of this patch. It takes into account
> the ACL checking changes and a few other minor amendments.

Sorry to bother you again, but this now doesn't compile atop master.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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

Re: [HACKERS] [PATCH] Incremental sort

[Tomas Vondra]

On 03/28/2018 05:12 PM, Alexander Korotkov wrote:
> On Wed, Mar 28, 2018 at 4:44 PM, Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> On 03/28/2018 03:28 PM, Teodor Sigaev wrote:
> >> BTW, patch had conflicts with master.  Please, find rebased version
> >> attached.
> >
> > Despite by patch conflist patch looks commitable, has anybody objections
> > to commit it?
> >
> > Patch recieved several rounds of review during 2 years, and seems to me,
> > keeping it out from sources may cause a lost it. Although it suggests
> > performance improvement in rather wide usecases.
> >
> 
> No objections from me - if you want me to do one final round of review
> after the rebase (not sure how invasive it'll turn out), let me know.
> 
> 
> Rebased patch is attached.  Incremental sort get used in multiple places
> of partition_aggregate regression test.  I've checked those cases, and
> it seems that incremental sort was selected right.
> 

OK, I'll take a look.

> BTW one detail I'd change is name of the GUC variable. enable_incsort
> seems unnecessarily terse - let's go for enable_incremental_sort or
> something like that.
> 
> 
> Already enable_incsort was already renamed to enable_incrementalsort
> since [1].  
> 
> 1.
> https://www.postgresql.org/message-id/CAPpHfduAVmiGDZC%2BdfNL1rEGu0mt45Rd_mxwjY57uqwWhrvQzg%40mail.gmail.com
>  
> 

Ah, apologies. I've been looking at the wrong version.

regards

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

Re: WIP: Covering + unique indexes.

[Erik Rijkers]

On 2018-03-28 16:59, Anastasia Lubennikova wrote:

Here is the new version of the patch set.


I can't get these to apply:

patch -b -l -F 25 -p 1 < 
/home/aardvark/download/pgpatches/0110/covering_indexes/20180328/0001-Covering-core-v8.patch



1 out of 19 hunks FAILED -- saving rejects to file 
src/backend/utils/cache/relcache.c.rej



$ cat src/backend/utils/cache/relcache.c.rej
--- src/backend/utils/cache/relcache.c
+++ src/backend/utils/cache/relcache.c
@@ -542,7 +542,7 @@
attp = (Form_pg_attribute) 
GETSTRUCT(pg_attribute_tuple);


if (attp->attnum <= 0 ||
-   attp->attnum > relation->rd_rel->relnatts)
+   attp->attnum > 
RelationGetNumberOfAttributes(relation))
elog(ERROR, "invalid attribute number %d for 
%s",
 attp->attnum, 
RelationGetRelationName(relation));






Erik Rijkers

Re: committing inside cursor loop

[Ildus Kurbangaliev]

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

I have checked new version. Although I can miss something,  the patch looks 
good to me.

The new status of this patch is: Ready for Committer

Re: Parallel Aggregates for string_agg and array_agg

[David Rowley]

On 27 March 2018 at 09:27, Tom Lane  wrote:
> The main thing that remains undone is to get some test coverage ---
> AFAICS, none of these new functions get exercised in the standard
> regression tests.

I've attached a patch which implements some tests for the new functions.

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


parallel_string_agg_array_agg_tests.patch
Description: Binary data

Re: [HACKERS] [PATCH] Incremental sort

[Alvaro Herrera]

Teodor Sigaev wrote:
> > BTW, patch had conflicts with master.  Please, find rebased version 
> > attached.
> 
> Despite by patch conflist patch looks commitable, has anybody objections to
> commit it?
> 
> Patch recieved several rounds of review during 2 years, and seems to me,
> keeping it out from sources may cause a lost it. Although it suggests
> performance improvement in rather wide usecases.

Can we have a recap on what the patch *does*?  I see there's a
description in Alexander's first email
https://postgr.es/m/CAPpHfdscOX5an71nHd8WSUH6GNOCf=V7wgDaTXdDd9=gon-...@mail.gmail.com
but that was a long time ago, and the patch has likely changed in the
meantime ...

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

Re: Proposal: http2 wire format

[Andres Freund]

Hi,

On 2018-03-28 16:29:37 +0800, Craig Ringer wrote:
> > - allow *streaming* of large datums
> 
> Yes, very much +1 there. That's already on the wiki. Yeah:
> 
> * Permit lazy fetches of large values, at least out-of-line TOASTED values
> http://www.postgresql.org/message-id/53ff0ef8@2ndquadrant.com

That's not necessarily the same though. What I think we need is the
ability to have "chunked" encoding with *optional* length for the
overall datum. And then the backend infrastructure to be able to send
*to the wire* partial datums.  Probably with some callback based
StringInfo like buffer.

> - nested table support
> >
> >
> Can you elaborate on that one?

Nested recordsets. E.g. a SRF or procedure returning multiple query results.


> * room for other resultset formats later. Like Damir, I really want to add
> protobuf or json serializations of result sets at some point, mainly so we
> can return "entity graphs" in graph representation rather than left-join
> projection.

-1. I don't think this belongs in postgres.

Greetings,

Andres Freund

Re: Proposal: http2 wire format

[Andres Freund]

Hi,

On 2018-03-28 09:59:34 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > A few random, very tired, points:
> 
> > - consolidated message for common tasks:
> >   - (bind, [describe?,] execute) to reduce overhead of prepared
> > statement execution (both in messages, as well as branches)
> >   - (anonymous parse, bind, describe, execute) to make it cheaper to
> > send statements with out-of-line parameters
> 
> I do not see a need for this; you can already send those combinations of
> messages in a single network packet if you have a mind to.

The simple protocol right now is *considerably* faster than the extended
protocol. The extended protocol sends more data overall, we do more
memory context resets, there's more switches between protocol messages
in both backend and client. All of those aren't free.

https://www.postgresql.org/message-id/12500.1470002232%40sss.pgh.pa.us

I've previously wondered whether we can peek ahead in the stream and
recognize that we got a bind/describe/execute or
parse/bind/describe/execute and execute them all together if all the
necessary data is there. To avoid new protocol messages.


> Your other points are sound, except I have no idea what this means:
> 
> > - nested table support

Yea, not the most descriptive... Returning multiple different resultsets
from a function / procedure. Inability to do so is a serious limitation
of postgres in comparison to some other language with procedures.


Greetings,

Andres Freund

Re: [HACKERS] [PATCH] Incremental sort

[Andres Freund]

Hi,

On 2018-03-28 16:28:01 +0300, Teodor Sigaev wrote:
> > BTW, patch had conflicts with master.  Please, find rebased version 
> > attached.
> 
> Despite by patch conflist patch looks commitable, has anybody objections to
> commit it?

> Patch recieved several rounds of review during 2 years, and seems to me,
> keeping it out from sources may cause a lost it. Although it suggests
> performance improvement in rather wide usecases.

My impression it has *NOT* received enough review to be RFC. Not saying
it's impossible to get there this release, but that just committing it
doesn't seem wise.

Greetings,

Andres Freund

Re: [HACKERS] Pluggable storage

[David Steele]

On 2/26/18 3:19 AM, Alexander Korotkov wrote:
> On Fri, Feb 23, 2018 at 2:20 AM, Robert Haas  > wrote:
> 
> On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
> mailto:a.korot...@postgrespro.ru>> wrote:
> > BTW, EnterpriseDB announces zheap table access method (heap with undo 
> log)
> > [2].  I think this is great, and I'm looking forward for publishing 
> zheap in
> > mailing lists.  But I'm concerning about its compatibility with 
> pluggable
> > table access methods API.  Does zheap use table AM API from this 
> thread?  Or
> > does it just override current heap and needs to be adopted to use table 
> AM
> > API?  Or does it implements own API?
> 
> Right now it just hacks the code.  The plan is to adapt it to whatever
> API we settle on in this thread.
> 
> 
> Great, thank you for clarification.  I'm looking forward reviewing zheap :)
I think this entry should be moved the the next CF.  I'll do that
tomorrow unless there are objections.

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

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

[Andres Freund]

Hi,

On 2018-03-08 13:46:53 -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Mar 8, 2018 at 12:03 PM, Tom Lane  wrote:
> >> FWIW, I would also vote for (1), especially if the only way to do (2)
> >> is stuff as outright scary as this.  I would far rather have (3) than
> >> this, because IMO, what we are looking at right now is going to make
> >> the fallout from multixacts look like a pleasant day at the beach.
> 
> > Whoa.  Well, that would clearly be bad, but I don't understand why you
> > find this so scary.  Can you explain further?
> 
> Possibly I'm crying wolf; it's hard to be sure.  But I recall that nobody
> was particularly afraid of multixacts when that went in, and look at all
> the trouble we've had with that.  Breaking fundamental invariants like
> "ctid points to this tuple or its update successor" is going to cause
> trouble.  There's a lot of code that knows that; more than knows the
> details of what's in xmax, I believe.

Given, as explained nearby, we already do store transient data in the
ctid for speculative insertions (i.e. ON CONFLICT), and it hasn't caused
even a whiff of trouble, I'm currently not inclined to see a huge issue
here.  It'd be great if you could expand on your concerns here a bit, we
gotta figure out a way forward.

I think the proposed patch needs some polish (I'm e.g. unhappy with the
naming of the new macros etc), but I think otherwise it's a reasonable
attempt at solving the problem.


> I would've been happier about expending an infomask bit towards this
> purpose.  Just eyeing what we've got, I can't help noticing that
> HEAP_MOVED_OFF/HEAP_MOVED_IN couldn't possibly be set in any tuple
> in a partitioned table.  Perhaps making these tests depend on
> partitioned-ness would be unworkably messy, but it's worth thinking
> about.

They previously couldn't be set together IIRC, so we could just (mask &
(HEAP_MOVED_OFF |HEAP_MOVED_IN)) == (HEAP_MOVED_OFF |HEAP_MOVED_IN) but
that'd be permanently eating two infomask bits. For something that
doesn't in general have to be able to live on tuples, just on (at?) the
"deleted tuple at end of a chain".

Greetings,

Andres Freund

Re: [HACKERS] path toward faster partition pruning

[Jesper Pedersen]

Hi,

On 03/28/2018 06:30 AM, Amit Langote wrote:

On 2018/03/28 18:29, Amit Langote wrote:

Attached is the updated set of patches, which contains other miscellaneous
changes such as updated comments, beside the main changes described above.


Sorry, one of those miscellaneous changes was a typo that would cause
compilation to fail... Sigh.   Fixed in the updated version.



Just some trivial changes.

However,

explain (costs off) select * from mc2p where a = 2 and b < 1;

is picking up

   ->  Seq Scan on mc2p2
 Filter: ((b < 1) AND (a = 2))

which doesn't seem right, as its definition is

create table mc2p2 partition of mc2p for values from (1, 1) to (2, 
minvalue);


Best regards,
 Jesper
>From 8bb5f25d31471910db2e7907b4c13029edd7bbdf Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Wed, 28 Mar 2018 12:39:42 -0400
Subject: [PATCH] Trivial changes

---
 src/backend/catalog/partition.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 3c26daa098..2ee5ce605c 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1774,7 +1774,7 @@ perform_pruning_base_step(PartitionPruneContext *context,
 	Datum		values[PARTITION_MAX_KEYS];
 	FmgrInfo	partsupfunc[PARTITION_MAX_KEYS];
 
-	/* There better be same number of expressions and compare functions. */
+	/* There better be the same number of expressions and compare functions. */
 	Assert(list_length(opstep->exprs) == list_length(opstep->cmpfns));
 
 	nvalues = 0;
@@ -1783,7 +1783,7 @@ perform_pruning_base_step(PartitionPruneContext *context,
 
 	/*
 	 * Generate the partition look-up key that will be used by one of
-	 * get_partitions_from_keys_* functions called below.
+	 * the get_partitions_from_keys_* functions called below.
 	 */
 	for (keyno = 0; keyno < context->partnatts; keyno++)
 	{
@@ -1969,7 +1969,7 @@ perform_pruning_combine_step(PartitionPruneContext *context,
 	PruneStepResult *result = NULL;
 
 	/*
-	 * In some cases, planner generates a combine step that doesn't contain
+	 * In some cases, the planner generates a combine step that doesn't contain
 	 * any argument steps, to signal us to not prune any partitions.  So,
 	 * return indexes of all datums in that case, including null and/or
 	 * default partition, if any.
@@ -1994,7 +1994,7 @@ perform_pruning_combine_step(PartitionPruneContext *context,
 			PruneStepResult *step_result;
 
 			/*
-			 * step_results[step_id] must contain valid result, which is
+			 * step_results[step_id] must contain a valid result, which is
 			 * confirmed by the fact that cstep's ID is greater than
 			 * step_id.
 			 */
@@ -2147,10 +2147,10 @@ get_partitions_for_keys_hash(PartitionPruneContext *context,
  * If special partitions (null and default) need to be scanned for given
  * values, set scan_null and scan_default in result if present.
  *
- * 'nvalues', if non-zero, should be exactly 1, because list partitioning.
+ * 'nvalues', if non-zero, should be exactly 1, because of list partitioning.
  * 'value' contains the value to use for pruning
  * 'opstrategy' if non-zero must be a btree strategy number
- * 'partsupfunc' contains list partitioning comparison function to be used to
+ * 'partsupfunc' contains the list partitioning comparison function to be used to
  * perform partition_list_bsearch
  * 'nullkeys' is the set of partition keys that are null.
  */
@@ -2161,7 +2161,6 @@ get_partitions_for_keys_list(PartitionPruneContext *context,
 {
 	PruneStepResult *result;
 	PartitionBoundInfo boundinfo = context->boundinfo;
-	int		   *partindices = boundinfo->indexes;
 	int			off,
 minoff,
 maxoff;
@@ -2272,7 +2271,7 @@ get_partitions_for_keys_list(PartitionPruneContext *context,
 			{
 /*
  * This case means all partition bounds are greater, which in
- * turn means that all partition satisfy this key.
+ * turn means that all partitions satisfy this key.
  */
 off = 0;
 			}
@@ -2333,7 +2332,7 @@ get_partitions_for_keys_list(PartitionPruneContext *context,
  * 'values' contains values for partition keys (or a prefix) to be used for
  * pruning
  * 'opstrategy' if non-zero must be a btree strategy number
- * 'partsupfunc' contains range partitioning comparison function to be used to
+ * 'partsupfunc' contains the range partitioning comparison function to be used to
  * perform partition_range_datum_bsearch or partition_rbound_datum_cmp
  * 'nullkeys' is the set of partition keys that are null.
  */
@@ -2404,7 +2403,7 @@ get_partitions_for_keys_range(PartitionPruneContext *context,
 			{
 if (nvalues == partnatts)
 {
-	/* There can only be zero or one matching partitions. */
+	/* There can only be zero or one matching partition. */
 	if (partindices[off + 1] >= 0)
 	{
 		result->datum_offsets = bms_make_singleton(off + 1);
@@ -2532,7 +2531,7 @@ get_partitions_for_keys_range(PartitionPruneContext *context,

Re: Re: csv format for psql

[Pavel Stehule]

2018-03-28 10:24 GMT+02:00 Fabien COELHO :

>
> Hello Pavel,
>
> I'd like to convince you to compromise, because otherwise I'm afraid we
>>> will not get this feature.
>>>
>>
>> [...]
>>
>>>
>>> The only no-surprise, no-behavioral-change, alternative I see is to have
>>> a
>>> csv-specific fieldsep. I'm not keen on that one because it is yet another
>>> variable, one has to remember it, and then it asks the question about
>>> recordsep... and I think there are already too many parameters, and more
>>> is
>>> not desirable, although I probably could live with that if I can live
>>> with
>>> "fieldsep_zero":-)
>>>
>>>
>>> I don't share your opinion so introduction csv-specific fieldsep is
>> surprise less. Current design is wrong - this thread is a evidence.
>>
>
> Wrong is too strong a word. Current design is not perfect, sure. Proposed
> alternatives are somehow worse, at least to some people mind, which
> explains why we arrived on this version after a few iterations.
>
> And if we introduce csv-specific fieldsep, then we multiply this wrong
>> design. The fix in this direction is renaming fieldsep to fieldsep-unaliagn
>> - but it is probably too big change too. So this design is nothing what I
>> can mark as good solution.
>>
>
> Good, we somehow agree on something!
>
> I can live with because it is much better than using pipe as separator for
>> csv, and because real impact is small - for almost people it will be
>> invisible - but have not good feeling from it.
>>
>
> Are there some possible alternatives?
>>
>
> Given the date and the fact that the cf end is 3 days away, the proposed
> short term alternative is Daniel's version, that I feel is reasonable. Ok,
> people have to do two pset to get comma-separated csv, otherwise they get
> pipe-separated csv in one pset.
>

I dislike the last Daniel's design. I am sorry.


> You could compromise on that for now, and submit an improvement patch for
> a later version if you wish.
>

I am able to accept csv specific field sep independent on unaligned field
sep.

I have not any other idea. And there is not any good (acceptable) ideas. It
is hard to expect so there will be change next year. This space is small,
and there are not too much possible variants. We checked all possibility
without any agreement.


>
> Otherwise, ISTM that the current result of this thread is that there will
> be no simple CSV in pg11:-(
>

Can be. If there is not good enough solution now. If we merge it now, then
will be hard to change it due possible compatibility issues.


>
> Or maybe I can mark Daniel's latest version as "ready" and point out that
> there is some disagreement on the thread, so it is not a full consensus.
> Then a committer can decide whether it is better in like that or that it
> should be put back in some design stage, possibly with their preference wrt
> to the kind of solution they think best.


You can do it. But from my view the Daniel's latest version (with respect
to his work) is the worst variant :(. So I am against to commit this
version.

Regards

Pavel



>
> --
> Fabien.
>

Re: [HACKERS] GSoC 2017: weekly progress reports (week 6)

[Teodor Sigaev]

Hi!

I slightly modified test for clean demonstration of difference between 
fastupdate on and off. Also I added CheckForSerializableConflictIn() to 
fastupdate off codepath, but only in case of non-empty pending list.


The next question what I see: why do not we lock entry leaf pages in some cases? 
As I understand, scan should lock any visited page, but now it's true only for 
posting tree. Seems, it also should lock pages in entry tree because concurrent 
procesess could add new entries which could be matched by partial search, for 
example. BTW, partial search (see collectMatchBitmap()) locks correctly entry 
tree, but regular startScanEntry() doesn't lock entry page in case of posting 
tree, only in case of posting list.



Dmitry Ivanov wrote:

I'd like to see fastupdate=on in test too, now tests cover only case
without fastupdate. Please, add them.


Here's a couple of tests for pending list (fastupdate = on).



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 37070b3b72..095b1192cb 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -17,6 +17,7 @@
 #include "access/gin_private.h"
 #include "access/ginxlog.h"
 #include "access/xloginsert.h"
+#include "storage/predicate.h"
 #include "miscadmin.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
@@ -515,6 +516,19 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
 			btree->fillRoot(btree, newrootpg,
 			BufferGetBlockNumber(lbuffer), newlpage,
 			BufferGetBlockNumber(rbuffer), newrpage);
+
+			if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
+			{
+
+PredicateLockPageSplit(btree->index,
+		BufferGetBlockNumber(stack->buffer),
+		BufferGetBlockNumber(lbuffer));
+
+PredicateLockPageSplit(btree->index,
+		BufferGetBlockNumber(stack->buffer),
+		BufferGetBlockNumber(rbuffer));
+			}
+
 		}
 		else
 		{
@@ -524,6 +538,14 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
 			GinPageGetOpaque(newrpage)->rightlink = savedRightLink;
 			GinPageGetOpaque(newlpage)->flags |= GIN_INCOMPLETE_SPLIT;
 			GinPageGetOpaque(newlpage)->rightlink = BufferGetBlockNumber(rbuffer);
+
+			if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
+			{
+
+PredicateLockPageSplit(btree->index,
+		BufferGetBlockNumber(stack->buffer),
+		BufferGetBlockNumber(rbuffer));
+			}
 		}
 
 		/*
diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index f9daaba52e..3fb4fc8264 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -19,6 +19,7 @@
 #include "access/xloginsert.h"
 #include "lib/ilist.h"
 #include "miscadmin.h"
+#include "storage/predicate.h"
 #include "utils/rel.h"
 
 /*
@@ -1423,7 +1424,7 @@ disassembleLeaf(Page page)
  * Any segments that acquire new items are decoded, and the new items are
  * merged with the old items.
  *
- * Returns true if any new items were added. False means they were all
+ * Returns true if any new items were added. false means they were all
  * duplicates of existing items on the page.
  */
 static bool
@@ -1759,7 +1760,7 @@ leafRepackItems(disassembledLeaf *leaf, ItemPointer remaining)
  */
 BlockNumber
 createPostingTree(Relation index, ItemPointerData *items, uint32 nitems,
-  GinStatsData *buildStats)
+  GinStatsData *buildStats, Buffer entrybuffer)
 {
 	BlockNumber blkno;
 	Buffer		buffer;
@@ -1810,6 +1811,12 @@ createPostingTree(Relation index, ItemPointerData *items, uint32 nitems,
 	page = BufferGetPage(buffer);
 	blkno = BufferGetBlockNumber(buffer);
 
+	/*
+	 * Copy a predicate lock from entry tree leaf (containing posting list)
+	 * to  posting tree.
+	 */
+	PredicateLockPageSplit(index, BufferGetBlockNumber(entrybuffer), blkno);
+
 	START_CRIT_SECTION();
 
 	PageRestoreTempPage(tmppage, page);
@@ -1904,6 +1911,7 @@ ginInsertItemPointers(Relation index, BlockNumber rootBlkno,
 		btree.itemptr = insertdata.items[insertdata.curitem];
 		stack = ginFindLeafPage(&btree, false, NULL);
 
+		GinCheckForSerializableConflictIn(btree.index, NULL, stack->buffer);
 		ginInsertValue(&btree, stack, &insertdata, buildStats);
 	}
 }
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 6fe67f346d..63603859bc 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -17,8 +17,10 @@
 #include "access/gin_private.h"
 #include "access/relscan.h"
 #include "miscadmin.h"
+#include "storage/predicate.h"
 #include "utils/datum.h"
 #include "utils/memutils.h"
+#include "utils/rel.h"
 
 /* GUC parameter */
 int			GinFuzzySearchLimit = 0;
@@ -33,11 +35,18 @@ typedef struct pendingPosition
 } pendingPosition;
 
 
+static void
+GinPredicateLockPage(Relation index, BlockNumber blkno, Snapshot snapshot)
+{
+	if (!GinGetUseFastUpdate(index))
+		

Re: [HACKERS] logical decoding of two-phase transactions

[Simon Riggs]

On 28 March 2018 at 16:28, Nikhil Sontakke  wrote:

> Simon, 0003-Add-GID-and-replica-origin-to-two-phase-commit-abort.patch
> is the exact patch that you had posted for an earlier commit.

0003 Pushed

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

Re: disable SSL compression?

[Konstantin Knizhnik]

On 17.03.2018 17:18, Peter Eisentraut wrote:

On 3/11/18 13:28, Tom Lane wrote:

My proposal is the attached patch that sets the default in libpq to off
and adjusts the documentation a bit so it doesn't sound like we have
missed the news altogether.

Seems reasonable as far as it goes, but do we need to make corresponding
server-side changes?

We could add a setting to disable SSL compression on the server, as some
web servers have been doing, but the niche of version combinations where
that would be applicable seems pretty low.




One of our customers was managed to improve speed about 10 times by 
using SSL compression for the system where client and servers are 
located in different geographical regions
and query results are very large because of JSON columns. Them actually 
do not need encryption, just compression.
I expect that it is not the only case where compression of libpq 
protocol can be useful. Please notice that logical replication is also 
using libpq protocol.


If SSL compression is deprecated, should we provide own compression?
I have implemented some prototype implementation of it (patch is attached).
I have added compression=on/off parameter to connection string and -Z 
option to psql and pgbench utilities.

Below are some results:

Compression ratio (raw->compressed):


libz (level=1)
libzstd (level=1)
pgbench -i -s 10
16997209->2536330
16997209->268077
pgbench -t 10 -S
6289036->1523862
6600338<-900293
6288933->1777400
6600338<-1000318


There is no mistyping: libzstd compress COPY data about 10 times better 
than libz, with wonderful compression ratio 63.


Influence on execution time is minimal (I have tested local 
configuration when client and server are at the same host):



no compression
libz (level=1)
libzstd (level=1)
pgbench -i -s 10
1.552
1.572
1.611
pgbench -t 10 -S
4.482
4.926
4.877






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

diff --git a/configure b/configure
index 6f659a5..f2ef62d 100755
--- a/configure
+++ b/configure
@@ -698,6 +698,7 @@ ELF_SYS
 EGREP
 GREP
 with_zlib
+with_zstd
 with_system_tzdata
 with_libxslt
 with_libxml
@@ -862,6 +863,7 @@ with_libxml
 with_libxslt
 with_system_tzdata
 with_zlib
+with_zstd
 with_gnu_ld
 enable_largefile
 enable_float4_byval
@@ -8016,6 +8018,86 @@ fi
 
 
 #
+# ZStd
+#
+
+
+
+# Check whether --with-zstd was given.
+if test "${with_zstd+set}" = set; then :
+  withval=$with_zstd;
+  case $withval in
+yes)
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-zstd option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_zstd=no
+
+fi
+
+
+
+
+if test "$with_zstd" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for ZSTD_compress in -lzstd" >&5
+$as_echo_n "checking for ZSTD_compress in -lzstd... " >&6; }
+if ${ac_cv_lib_zstd_ZSTD_compress+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lzstd  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char ZSTD_compress ();
+int
+main ()
+{
+return ZSTD_compress ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_zstd_ZSTD_compress=yes
+else
+  ac_cv_lib_zstd_ZSTD_compress=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_zstd_ZSTD_compress" >&5
+$as_echo "$ac_cv_lib_zstd_ZSTD_compress" >&6; }
+if test "x$ac_cv_lib_zstd_ZSTD_compress" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBZSTD 1
+_ACEOF
+
+  LIBS="-lzstd $LIBS"
+
+else
+  as_fn_error $? "library 'zstd' is required for ZSTD support" "$LINENO" 5
+fi
+
+fi
+
+
+
+#
 # Elf
 #
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 3bbdf17..08b8ab2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -195,6 +195,7 @@ with_llvm	= @with_llvm@
 with_system_tzdata = @with_system_tzdata@
 with_uuid	= @with_uuid@
 with_zlib	= @with_zlib@
+with_zstd   = @with_zstd@
 enable_rpath	= @enable_rpath@
 enable_nls	= @enable_nls@
 enable_debug	= @enable_debug@
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 6450c5a..1522c70 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -50,6 +50,14 @@ ifeq ($(with_systemd),yes)
 LIBS += -lsystemd
 endif
 
+ifeq ($(with_zstd),yes)
+LIBS += -lzstd
+endif
+
+ifeq ($(with_zlib),yes)
+LIBS += -lz
+endif
+
 ##
 
 all: submake-libpgport submake-schemapg postgres 

Re: [HACKERS] Transactions involving multiple postgres foreign servers

[David Steele]

On 2/27/18 2:21 AM, Masahiko Sawada wrote:
> 
> Hmm, although I've thought concern in case where we don't consider
> local xids of un-resolved fdwxact in GetOldestXmin, I could not find
> problem. Could you share your concern if you have? I'll try to find a
> possibility based on it.

It appears that this entry should be marked Waiting on Author so I have
done that.

I also think it may be time to move this patch to the next CF.

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

Re: PostgreSQL crashes with SIGSEGV

[Tom Lane]

Peter Geoghegan  writes:
> On Mon, Mar 26, 2018 at 5:14 AM, Kyotaro HORIGUCHI
>  wrote:
>> If no one objects, I'll mark this as Ready for Commit in a couple
>> of days.

> Thank you for the review, Horiguchi-san. It's hard to decide how
> important each goal is when coming up with a back-patchable fix like
> this. When the goals are somewhat in competition with each other, a
> second or a third opinion is particularly appreciated.

It looks good to me.  The only real objection would be if someone came
up with a test case proving that there's a significant performance
degradation from the extra copies.  But given that these are back
branches, it would take a pretty steep penalty for me to want to take
the risk of refactoring to avoid that.

I've pushed it with some cosmetic adjustments.

regards, tom lane

Re: PostgreSQL crashes with SIGSEGV

[Peter Geoghegan]

On Wed, Mar 28, 2018 at 10:30 AM, Tom Lane  wrote:
> It looks good to me.  The only real objection would be if someone came
> up with a test case proving that there's a significant performance
> degradation from the extra copies.  But given that these are back
> branches, it would take a pretty steep penalty for me to want to take
> the risk of refactoring to avoid that.
>
> I've pushed it with some cosmetic adjustments.

Thank you, Tom.

-- 
Peter Geoghegan

Re: disable SSL compression?

[Konstantin Knizhnik]

On 28.03.2018 20:26, Konstantin Knizhnik wrote:



On 17.03.2018 17:18, Peter Eisentraut wrote:

On 3/11/18 13:28, Tom Lane wrote:

My proposal is the attached patch that sets the default in libpq to off
and adjusts the documentation a bit so it doesn't sound like we have
missed the news altogether.

Seems reasonable as far as it goes, but do we need to make corresponding
server-side changes?

We could add a setting to disable SSL compression on the server, as some
web servers have been doing, but the niche of version combinations where
that would be applicable seems pretty low.




One of our customers was managed to improve speed about 10 times by 
using SSL compression for the system where client and servers are 
located in different geographical regions
and query results are very large because of JSON columns. Them 
actually do not need encryption, just compression.
I expect that it is not the only case where compression of libpq 
protocol can be useful. Please notice that logical replication is also 
using libpq protocol.


If SSL compression is deprecated, should we provide own compression?
I have implemented some prototype implementation of it (patch is 
attached).
I have added compression=on/off parameter to connection string and -Z 
option to psql and pgbench utilities.

Below are some results:

Compression ratio (raw->compressed):


libz (level=1)
libzstd (level=1)
pgbench -i -s 10
16997209->2536330
16997209->268077
pgbench -t 10 -S
6289036->1523862
6600338<-900293
6288933->1777400
6600338<-1000318


There is no mistyping: libzstd compress COPY data about 10 times 
better than libz, with wonderful compression ratio 63.


Influence on execution time is minimal (I have tested local 
configuration when client and server are at the same host):



no compression
libz (level=1)
libzstd (level=1)
pgbench -i -s 10
1.552
1.572
1.611
pgbench -t 10 -S
4.482
4.926
4.877





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


Sorry, I have attached wrong patch.
To use zstd compression, Postgres should be configured with --with-zstd. 
Otherwise compression will use zlib unless it is disabled by 
--without-zlib option.



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

diff --git a/configure b/configure
index 6f659a5..f2ef62d 100755
--- a/configure
+++ b/configure
@@ -698,6 +698,7 @@ ELF_SYS
 EGREP
 GREP
 with_zlib
+with_zstd
 with_system_tzdata
 with_libxslt
 with_libxml
@@ -862,6 +863,7 @@ with_libxml
 with_libxslt
 with_system_tzdata
 with_zlib
+with_zstd
 with_gnu_ld
 enable_largefile
 enable_float4_byval
@@ -8016,6 +8018,86 @@ fi
 
 
 #
+# ZStd
+#
+
+
+
+# Check whether --with-zstd was given.
+if test "${with_zstd+set}" = set; then :
+  withval=$with_zstd;
+  case $withval in
+yes)
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-zstd option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_zstd=no
+
+fi
+
+
+
+
+if test "$with_zstd" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for ZSTD_compress in -lzstd" >&5
+$as_echo_n "checking for ZSTD_compress in -lzstd... " >&6; }
+if ${ac_cv_lib_zstd_ZSTD_compress+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lzstd  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char ZSTD_compress ();
+int
+main ()
+{
+return ZSTD_compress ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_zstd_ZSTD_compress=yes
+else
+  ac_cv_lib_zstd_ZSTD_compress=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_zstd_ZSTD_compress" >&5
+$as_echo "$ac_cv_lib_zstd_ZSTD_compress" >&6; }
+if test "x$ac_cv_lib_zstd_ZSTD_compress" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBZSTD 1
+_ACEOF
+
+  LIBS="-lzstd $LIBS"
+
+else
+  as_fn_error $? "library 'zstd' is required for ZSTD support" "$LINENO" 5
+fi
+
+fi
+
+
+
+#
 # Elf
 #
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 3bbdf17..08b8ab2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -195,6 +195,7 @@ with_llvm	= @with_llvm@
 with_system_tzdata = @with_system_tzdata@
 with_uuid	= @with_uuid@
 with_zlib	= @with_zlib@
+with_zstd   = @with_zstd@
 enable_rpath	= @enable_rpath@
 enable_nls	= @enable_nls@
 enable_debug	= @enable_debug@
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 6450c5a..1522c70 100644
-

Re: Reopen logfile on SIGHUP

[David Steele]

On 2/27/18 8:54 PM, Michael Paquier wrote:
> On Tue, Feb 27, 2018 at 05:52:20PM -0500, Tom Lane wrote:
>> It already does treat SIGUSR1 as a log rotation request.  Apparently
>> the point of this patch is that some people don't find that easy enough
>> to use, which is fair, because finding out the collector's PID from
>> outside isn't very easy.
> 
> True enough.  The syslogger does not show up in pg_stat_activity either,
> so I think that being able to do so would help for this case.

There does not seem to be any consensus on this patch so I'm marking it
Waiting on Author for the time being.  At the end of the CF I'll mark it
Returned with Feedback if there is no further activity.

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

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

[Tom Lane]

Andres Freund  writes:
> On 2018-03-08 13:46:53 -0500, Tom Lane wrote:
>> ... Breaking fundamental invariants like
>> "ctid points to this tuple or its update successor" is going to cause
>> trouble.  There's a lot of code that knows that; more than knows the
>> details of what's in xmax, I believe.

> Given, as explained nearby, we already do store transient data in the
> ctid for speculative insertions (i.e. ON CONFLICT), and it hasn't caused
> even a whiff of trouble, I'm currently not inclined to see a huge issue
> here.  It'd be great if you could expand on your concerns here a bit, we
> gotta figure out a way forward.

Just what I said.  There's a lot of code that knows how to follow tuple
update chains, probably not all of it in core, and this will break it.
But only in seldom-exercised corner cases, which is the worst of all
possible worlds from a reliability standpoint.  (I don't think ON CONFLICT
is a counterexample because, IIUC, it's not a persistent state.)

Given that there are other ways we could attack it, I think throwing away
this particular invariant is an unnecessarily risky solution.

>> I would've been happier about expending an infomask bit towards this
>> purpose.  Just eyeing what we've got, I can't help noticing that
>> HEAP_MOVED_OFF/HEAP_MOVED_IN couldn't possibly be set in any tuple
>> in a partitioned table.  Perhaps making these tests depend on
>> partitioned-ness would be unworkably messy, but it's worth thinking
>> about.

> They previously couldn't be set together IIRC, so we could just (mask &
> (HEAP_MOVED_OFF |HEAP_MOVED_IN)) == (HEAP_MOVED_OFF |HEAP_MOVED_IN) but
> that'd be permanently eating two infomask bits.

Hmm.  That objection only matters if we have realistic intentions of
reclaiming those bits in future, which I've not heard anyone making
serious effort towards.  Rather than messing with the definition of ctid,
I'd be happier with saying that they're never going to be reclaimed, but
at least we're getting one bit's worth of real use out of them.

regards, tom lane

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

[Andres Freund]

Hi,

On 2018-03-28 13:52:24 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Given, as explained nearby, we already do store transient data in the
> > ctid for speculative insertions (i.e. ON CONFLICT), and it hasn't caused
> > even a whiff of trouble, I'm currently not inclined to see a huge issue
> > here.  It'd be great if you could expand on your concerns here a bit, we
> > gotta figure out a way forward.
> 
> Just what I said.  There's a lot of code that knows how to follow tuple
> update chains, probably not all of it in core, and this will break it.
> But only in seldom-exercised corner cases, which is the worst of all
> possible worlds from a reliability standpoint.

How will it break it? They'll see an invalid ctid and conclude that the
tuple is dead? Without any changes that's already something that can
happen if a later tuple in the chain has been pruned away.  Sure, that
code won't realize it should error out because the tuple is now in a
different partition, but neither would a infomask bit.

I think my big problem is that I just don't see what the worst that can
happen is. We'd potentially see a broken ctid chain, something that very
commonly happens, and consider the tuple to be invisible.  That seems
pretty sane behaviour for unadapted code, and not any worse than other
potential solutions.


> (I don't think ON CONFLICT is a counterexample because, IIUC, it's not
> a persistent state.)

Hm, it can be persistent if we error out in the right moment. But it's
nots super common to encounter that over a long time, I grant you
that. Not that this'd be super persistent either, vacuum/pruning would
normally remove the tuple as well, it's dead after all.


> >> I would've been happier about expending an infomask bit towards this
> >> purpose.  Just eyeing what we've got, I can't help noticing that
> >> HEAP_MOVED_OFF/HEAP_MOVED_IN couldn't possibly be set in any tuple
> >> in a partitioned table.  Perhaps making these tests depend on
> >> partitioned-ness would be unworkably messy, but it's worth thinking
> >> about.
> 
> > They previously couldn't be set together IIRC, so we could just (mask &
> > (HEAP_MOVED_OFF |HEAP_MOVED_IN)) == (HEAP_MOVED_OFF |HEAP_MOVED_IN) but
> > that'd be permanently eating two infomask bits.
> 
> Hmm.  That objection only matters if we have realistic intentions of
> reclaiming those bits in future, which I've not heard anyone making
> serious effort towards.

I plan to submit a patch early v12 that keeps track of the last time a
table has been fully scanned (and when it was created). With part of the
goal being debuggability and part being able to reclaim things like
these bits.


Greetings,

Andres Freund

Re: Rangejoin rebased

[David Steele]

On 3/2/18 11:44 AM, Robert Haas wrote:
> On Fri, Mar 2, 2018 at 11:12 AM, Alexander Kuzmenkov
>  wrote:
>> On 16.01.2018 10:49, Jeff Davis wrote:
>>> My proposed fix is to make an internal opfamily identical to the
>>> external one, such that it's not recognized as part of the same EC,
>>> and the planner won't try to eliminate it. It loses out on potential
>>> optimizations, but those are mostly theoretical since the btree
>>> opclass ordering for ranges is not very interesting to a user.
>>
>> I think I figured out what to do with missing sort directions. We can change
>> select_outer_pathkeys_for_merge() to generate the pathkeys we need. Also,
>> find_mergeclauses_for_outer_pathkeys() has to be changed too, so that it
>> knows which pathkeys are compatible to which range join clauses.
>>
>> About the patch, do I understand it right that you are working on the next
>> version now?
> 
> I think we are quite clearly past the deadline to submit a new patch
> for inclusion in v11 at this point.

It seems that a new patch is needed here but none has been presented.
I've marked this Waiting on Author for the moment, but I really think it
should be marked Returned with Feedback and submitted to the next CF
when a new patch is ready.

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

Re: Rangejoin rebased

[Andres Freund]

Hi,

On 2018-03-28 14:18:42 -0400, David Steele wrote:
> It seems that a new patch is needed here but none has been presented.
> I've marked this Waiting on Author for the moment, but I really think it
> should be marked Returned with Feedback and submitted to the next CF
> when a new patch is ready.

I'd just do so now. There's not been any progress for months, and
there's been an update request weeks ago...

Greetings,

Andres Freund

Re: Rangejoin rebased

[David Steele]

On 3/28/18 2:23 PM, Andres Freund wrote:
> On 2018-03-28 14:18:42 -0400, David Steele wrote:
>> It seems that a new patch is needed here but none has been presented.
>> I've marked this Waiting on Author for the moment, but I really think it
>> should be marked Returned with Feedback and submitted to the next CF
>> when a new patch is ready.
> 
> I'd just do so now. There's not been any progress for months, and
> there's been an update request weeks ago...
Done.

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

Re: PL/pgSQL nested CALL with transactions

[Peter Eisentraut]

On 3/28/18 09:00, Tomas Vondra wrote:
> I see. I thought "fixed" means you adopted the #define, but that's not
> the case. I don't think an extra comment is needed - the variable name
> is descriptive enough IMHO.

Committed as presented then.  Thanks.

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

Re: Function to track shmem reinit time

[David Steele]

On 3/4/18 11:17 AM, Tomas Vondra wrote:
> 
> Furthermore, the patch is yet another victim of fd1a421fe - fixing the
> pg_proc entries is trivial, but a new version is needed.
> 
> I'd also like to see an example/explanation how this improves this
> situation compared to only having pg_postmaster_start_time.
> 
> So I'm setting this as waiting on author for now.

I'm not sure why this was set back to Needs Review since it neither
applies cleanly nor builds.

I'm setting this entry to Waiting on Author, but based on the discussion
I think it should be Returned with Feedback.

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

Re: [bug fix] pg_rewind creates corrupt WAL files, and the standby cannot catch up the primary

[Fujii Masao]

On Wed, Mar 28, 2018 at 11:24 AM, Michael Paquier  wrote:
> On Wed, Mar 28, 2018 at 04:45:49AM +0900, Fujii Masao wrote:
>> The patch looks good to me! Barring objection, I will commit it
>> and back-patch to 9.5 where pg_rewind was added.
>
> Thanks in advance!  I just eyeballed the patch again and I don't see
> issues with what's used here.  The thing should apply smoothly back to
> 9.5, of course let me know if you need help or an extra pair of eyes.

Pushed. Thanks!

Regards,

-- 
Fujii Masao

Re: csv format for psql

[David G. Johnston]

On Wednesday, March 28, 2018, Pavel Stehule  wrote:

>
> Are there some possible alternatives?
>>>
>>
>> Given the date and the fact that the cf end is 3 days away, the proposed
>> short term alternative is Daniel's version, that I feel is reasonable. Ok,
>> people have to do two pset to get comma-separated csv, otherwise they get
>> pipe-separated csv in one pset.
>>
>
>
Could someone post how captions, rows-only, and footer pset settings factor
into this?  Specifically are they fixed to on/off or will they hide/show if
users request them explicitly?

My take on this is that --csv mode is/should be an alternate output mode
from the existing pset controlled one, and functions basically like "\copy
to stdout" and all echoing and metadata outputs are disabled and only query
results, with header and the user specified delimiter, are output.  No
control other than the delimiter seems to be provided in the current design
but that could be expanded upon.  In that specification the existing
fieldsep argument that is tied to pset should not be used and something
like --csv-fieldsep should be provided (I like the prefixing to tie the
option lexically to the master --csv option).

David J.

Re: Trigger file behavior with the standby

[Keiko Oda]

Thanks a lot for the answer, Michael (and sorry for the slow response)!

So, if I understand what you're saying correctly, I'm seeing this behavior
because wal-e keeps fetching wal files from s3 regardless of this
trigger_file, and these fetched wal files are in pg_wal (or pg_xlog),
therefore Postgres just tries to restore whatever available in pg_wal
before the failover. Or, even if there is no file in pg_wal, it still tries
to fetch from the "archive" (s3).
In other words, if I would like to do "immediate failover" (and do not care
about WAL files available in archive or in pg_wal), I should be tweaking
restore_command so that no further fetching/restoring happens.
Is it... accurate?

Thanks,
Keiko

On Mon, Mar 19, 2018 at 9:28 PM, Michael Paquier 
wrote:

> On Mon, Mar 19, 2018 at 01:27:21PM -0700, Keiko Oda wrote:
> > I'm seeing the following behavior with a trigger file which is very
> > confusing to me, I'd like to get some advice of what is the expected
> > behavior of the trigger file with the standby.
>
> This portion from the docs includes your answer:
> https://www.postgresql.org/docs/devel/static/warm-
> standby.html#STANDBY-SERVER-OPERATION
> "Standby mode is exited and the server switches to normal operation when
> pg_ctl promote is run or a trigger file is found (trigger_file). Before
> failover, any WAL immediately available in the archive or in pg_wal will
> be restored, but no attempt is made to connect to the master.
>
> So when creating a trigger file or signaling for promotion, any WAL
> files available are first fetched, and then promotion happens.  In your
> case all the WAL segments from the archives are retrieved first.
> --
> Michael
>

Re: csv format for psql

[David G. Johnston]

On Wednesday, March 28, 2018, David G. Johnston 
wrote:

> On Wednesday, March 28, 2018, Pavel Stehule 
> wrote:
>
>>
>> Are there some possible alternatives?

>>>
>>> Given the date and the fact that the cf end is 3 days away, the proposed
>>> short term alternative is Daniel's version, that I feel is reasonable. Ok,
>>> people have to do two pset to get comma-separated csv, otherwise they get
>>> pipe-separated csv in one pset.
>>>
>>
>>
> Could someone post how captions, rows-only, and footer pset settings
> factor into this?  Specifically are they fixed to on/off or will they
> hide/show if users request them explicitly?
>
>
I found the original post that covers that indeed we simply fix these
values, which is why I was thinking.

 and something like --csv-fieldsep should be provided (I like the prefixing
> to tie the option lexically to the master --csv option).
>

Or, really, just make --csv take an optional argument which, if present, is
the delimiter.  No separate argument needed, and we ignore the pset
fieldsep argument like we ignore everything else.

Need to decide whether to "not ignore" --tuples-only, which doesn't seem
controversial aside from being a pset argument that isn't being ignored in
this design...

David J.

Re: csv format for psql

[David G. Johnston]

On Monday, March 26, 2018, Daniel Verite  wrote:

>
> We could even support only the comma and make it non-configurable
> based on the fact it's Comma-Separated-Values, not
> Whatever-Separated-Values, except that won't do much
> to serve the users interests, as the reality is that
> people use various separators.
>

I like to call it "Character Separated Values" now for just that reason.

David J.

Re: Sample values for pg_stat_statements

[David Steele]

On 3/21/18 1:31 PM, David Steele wrote:
> 
> On 3/10/18 9:02 AM, Tomas Vondra wrote:
>>
>> I've looked at this patch today. I like the idea / intent in general, as
>> it helps with some investigation tasks. That being said, I have a couple
>> of questions/comments based on read through the patch:
> 
> It looks like there are some privacy concerns with this patch as well as
> the suggestions in the review from Tomas.
> 
> Will you be providing an updated patch for this CF?  If not, I think we
> should mark the entry Returned with Feedback for now and let you
> resubmit when you have an updated patch.

Since it doesn't appear that a new patch will be ready for this CF I
have marked this entry Returned with Feedback.

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

Re: csv format for psql

[Joshua D. Drake]

On 03/28/2018 12:35 PM, David G. Johnston wrote:
On Monday, March 26, 2018, Daniel Verite > wrote:



We could even support only the comma and make it non-configurable
based on the fact it's Comma-Separated-Values, not
Whatever-Separated-Values, except that won't do much
to serve the users interests, as the reality is that
people use various separators.


I like to call it "Character Separated Values" now for just that reason.


Isn't the actual wording Character Delimited Values? I may be picking at 
hairs here but every single time I use anything to import a CSV or other 
delimited file (TAB or | usually) that is what the import screen says.


JD



David J.



--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
* Unless otherwise stated, opinions are my own.   *

Re: Using base backup exclusion filters to reduce data transferred with pg_rewind

[Fujii Masao]

On Wed, Mar 28, 2018 at 7:54 AM, Michael Paquier  wrote:
> On Wed, Mar 28, 2018 at 04:13:25AM +0900, Fujii Masao wrote:
>> This code is almost ok in practice, but using the check of
>> "strstr(path, localpath) == path" is more robust here?
>
> No problems with that either.
>
>> Using the following code instead is more robust?
>> This original code is almost ok in practice, though.
>>
>> filename = last_dir_separator(path);
>> if (filename == NULL)
>> filename = path;
>> else
>> filename++;
>> if (strcmp(filename, excludeFiles[excludeIdx]) == 0)
>
> Indeed, using last_dir_separator is a better idea for files.  I was
> looking for something like that actually.
>
>> +  (everything except the relation files). Similarly to base backups,
>> +  the contents of the directories pg_dynshmem/,
>> +  pg_notify/, pg_replslot/,
>> +  pg_serial/, pg_snapshots/,
>> +  pg_stat_tmp/, and
>> +  pg_subtrans/ are omitted from the data copied
>> +  from the source cluster. Any file or directory beginning with
>> +  pgsql_tmp is omitted, as well as are
>> +  backup_label,
>> +  tablespace_map,
>> +  pg_internal.init,
>> +  postmaster.opts and
>> +  postmaster.pid.
>>
>> I don't think this description is necessary. The doc for pg_basebackup
>> also doesn't seem to have this kind of description.
>
> Those are listed in backup.sgml.  And I really think that we should at
> least document that the same type of exclusion filters as base backups
> are used in pg_rewind.

Okay, I revived that change in the doc.

>> So attached is the patch that I updated based on your patch and
>> am thinking to apply.
>
> Thanks.  Except for the documentation part, I am OK for the changes
> proposed.

Committed. Thanks!

Regards,

-- 
Fujii Masao

Re: csv format for psql

[David G. Johnston]

On Wednesday, March 28, 2018, Fabien COELHO  wrote:

>
>
> And if we introduce csv-specific fieldsep, then we multiply this wrong
>> design. The fix in this direction is renaming fieldsep to fieldsep-unaliagn
>> - but it is probably too big change too. So this design is nothing what I
>> can mark as good solution.
>>
>
> Good, we somehow agree on something!
>
> I can live with because it is much better than using pipe as separator for
>> csv, and because real impact is small - for almost people it will be
>> invisible - but have not good feeling from it.
>
>
Concretely...I'm in favor of the "\pset fieldsep_csv ," solution and csv
format should always use its existing value.  Teach \pset fieldsep to fail
if the current format is csv.  Being able to specify the csv fieldsep like
 "\pset format csv ;" would be a plus.

Unaligned format could grow its own fieldsep if it wanted to but it can
also just use the default provided fieldsep var whose default value is
pipe.  If it did grow one it would need to understand "not set" in order to
preserve existing behavior.  We don't have that problem with csv.

I don't believe we can modify fieldsep without causing unwanted grief.

David J.

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

[Robert Haas]

On Wed, Mar 28, 2018 at 3:06 AM, Amit Kapila  wrote:
> Good idea, such an optimization will ensure that the cases reported
> above will not have regression.  However isn't it somewhat beating the
> idea you are using in the patch which is to avoid modifying the path
> in-place?

Sure, but you can't have everything.  I don't think modifying the
sortgroupref data in place is really quite the same thing as changing
the pathtarget in place; the sortgroupref stuff is some extra
information about the targets being computed, not really a change in
targets per se.  But in any case if you want to eliminate extra work
then we've gotta eliminate it...

> In any case, I think one will still see regression in cases
> where this optimization doesn't apply.  For example,
>
> DO $$
> DECLARE count integer;
> BEGIN
> For count In 1..100 Loop
> Execute 'explain Select sum(thousand)from tenk1 group by ten';
> END LOOP;
> END;
> $$;
>
> The above block takes 43700.0289 ms on Head and 45025.3779 ms with the
> patch which is approximately 3% regression.

Thanks for the analysis -- the observation that this seemed to affect
cases where CP_LABEL_TLIST gets passed to create_projection_plan
allowed me to recognize that I was doing an unnecessary copyObject()
call.  Removing that seems to have reduced this regression below 1% in
my testing.

Also, keep in mind that we're talking about extremely small amounts of
time here.  On a trivial query that you're not even executing, you're
seeing a difference of (45025.3779 - 43700.0289)/100 = 0.00132 ms
per execution.  Sure, it's still 3%, but it's 3% of the time in an
artificial case where you don't actually run the query.  In real life,
nobody runs EXPLAIN in a tight loop a million times without ever
running the query, because that's not a useful thing to do.  The
overhead on a realistic test case will be smaller.  Furthermore, at
least in my testing, there are now cases where this is faster than
master.  Now, I welcome further ideas for optimization, but a patch
that makes some cases slightly slower while making others slightly
faster, and also improving the quality of plans in some cases, is not
to my mind an unreasonable thing.

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


0003-Rewrite-the-code-that-applies-scan-join-targets-to-p.patch
Description: Binary data


0002-Postpone-generate_gather_paths-for-topmost-scan-join.patch
Description: Binary data


0001-Teach-create_projection_plan-to-omit-projection-wher.patch
Description: Binary data

Re: reorganizing partitioning code

[Robert Haas]

On Wed, Mar 28, 2018 at 12:07 AM, Amit Langote
 wrote:
> On 2018/03/22 11:45, Amit Langote wrote:
>> FWIW, I did manage to rebase it this morning and posting it here.
>
> Rebased again.
>
> I started wondering if we should separate out stuff related to partition
> bounds.  That is create a utils/partbound.h and put PartitionBoundInfo and
> related comparison and search functions into a utils/adt/partbound.c.  I
> had started thinking about that when I looked at the code added by the
> patch submitted on the "advanced partition matching algorithm for
> partition-wise join" thread [1].  I haven't done anything about that though.

adt = Abstract Data Type, which I think we've interpreted up until now
to mean an SQL-visible data type, so that seems like an odd choice.

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

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

[Dmitry Dolgov]

> On 22 March 2018 at 14:18, Ashutosh Bapat  
> wrote:
> On Thu, Mar 22, 2018 at 4:32 AM, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>>> On 12 March 2018 at 06:00, Ashutosh Bapat  
>>> wrote:
>>> Thanks for the note. Here are rebased patches.
>>
>> Since I started to look at this patch, I can share few random notes (although
>> it's not a complete review, I'm in the progress now), maybe it can be 
>> helpful.
>>
>> In `partition_range_bounds_merge`
>>
>> + if (!merged)
>> + break;
>>
>> is a bit redundant I think, because every time `merged` set to false it
>> followed by break.
>
> Yes, right now. May be I should turn it into Assert(merged); What do you 
> think?

Thank you for reply. Yes, that sounds good. But actually you also mentioned
another topic that bothers me about this patch. Different parts of the
algorithm implementation (at least for functions that build maps of matching
partitions) are quite dependent on each other in terms of shared state. At
first glance in `partition_range_bounds_merge` we have about a dozen of
variables of different mutability level, that affect the control flow:

outer_lb_index
inner_lb_index
merged
merged_index
overlap
merged_lb
merged_ub
finished_outer
finished_inner
ub_cmpval
lb_cmpval
inner_has_default
outer_has_default
jointype

It looks a bit too much for me, and would require commentaries like "if you
changed the logic here, also take a look there". But I'm not saying that I have
any specific suggestions how to change it (although I'll definitely try to do
so, at least to get some better understanding of the underlying algorithm).

>>
>> I've noticed that in some places `IS_PARTITIONED_REL` was replaced
>>
>> - if (!IS_PARTITIONED_REL(joinrel))
>> + if (joinrel->part_scheme == NULL)
>>
>> but I'm not quite follow why? Is it because `boundinfo` is not available
>> anymore at this point? If so, maybe it makes sense to update the commentary 
>> for
>> this macro and mention to not use for joinrel.
>
>
> This is done in try_partitionwise_join(). As explained in the comment
>
>  * Get the list of matching partitions to be joined along with the
>  * partition bounds of the join relation. Because of the restrictions
>  * imposed by partition matching algorithm, not every pair of joining
>  * relations for this join will be able to use partition-wise join. But 
> all
>  * those pairs which can use partition-wise join will produce the same
>  * partition bounds for the join relation.
>
> boundinfo for the join relation is built in this function. So, we
> don't have join relation's partitioning information fully set up yet.
> So we can't use IS_PARTITIONED_REL() there. joinrel->part_scheme if
> set tells that the joining relations have matching partition schemes
> and thus the join relation can possibly use partition-wise join
> technique. If it's not set, then we can't use partition-wise join.
>
> But IS_PARTITIONED_REL() is still useful at a number of other places,
> where it's known to encounter a RelOptInfo whose partitioning
> properties are fully setup. So, I don't think we should change the
> macro or the comments above it.

Just to make myself clear, I wanted to suggest not to change the commentary for
`IS_PARTITIONED_REL` significantly, but just add a sentence that you need to
check if given relation is fully set up.

Also, few more random notes (mostly related to readability, since I found some
parts of the patch hard to read, but of course it's arguable).

```
PartitionRangeBound outer_lb;
PartitionRangeBound outer_ub;
PartitionRangeBound inner_lb;
PartitionRangeBound inner_ub;
PartitionRangeBound *merged_lb = NULL;
PartitionRangeBound *merged_ub = NULL;
```

Maybe it would be better to not repeat the type here? Let's say:

```
PartitionRangeBound outer_lb,
outer_ub,
...
```

It's just too long and distracting.

```
partition_range_bounds_merge(int partnatts, FmgrInfo *partsupfuncs,
 Oid *partcollations, PartitionBoundInfo outer_bi,
 int outer_nparts, PartitionBoundInfo inner_bi,
 int inner_nparts, JoinType jointype,
 List **outer_parts, List **inner_parts)
```

>From what I see in `partition.c` there are a lot functions that accept
`partnatts`, `partcollations` only to pass it down to, e.g.
`partition_rbound_cmp`.
What do you think about introducing a data structure to keep these arguments,
and pass only an instance of this structure instead?

Re: JIT compiling with LLVM v12

[Andres Freund]

On 2018-03-27 10:34:26 -0700, Andres Freund wrote:
> On 2018-03-27 10:05:47 -0400, Peter Eisentraut wrote:
> > On 3/13/18 19:40, Andres Freund wrote:
> > > I've pushed a revised and rebased version of my JIT patchset.
> > 
> > What is the status of this item as far as the commitfest is concerned?
> 
> 7/10 committed. Inlining, Explain, Docs remain.

I've pushed these three.

As explained in the inline commit, I've found an edge case where I could
hit an assert in LLVM when using a more efficient interaction with
on-disk files.  That appears to be a spurious assert, but I don't want
to ignore it until that's confirmed from the LLVM side of things.

For now LLVM is enabled by default when compiled --with-llvm. I'm mildly
inclined to leave it like that until shortly before the release, and
then disable it by default (i.e. change the default of jit=off). But I
think we can make that decision based on experience during the testing
window. I'm opening an open items entry for that.

Yay. Also: Tired.

Greetings,

Andres Freund

Re: JIT compiling with LLVM v12

[Andres Freund]

Hi,

On 2018-03-28 14:27:51 -0700, Andres Freund wrote:
> > 7/10 committed. Inlining, Explain, Docs remain.
> 
> I've pushed these three.

One tiny pending commit I have is to add a few pg_noinline annotations
to slow-path functions, to avoid very common spurious inlines. I'll play
a littlebit more with the set that I think make sense there, and will
send a separate email about that.

Greetings,

Andres Freund

Re: new buildfarm with gcc & clang "trunk" -Werror?

[Peter Eisentraut]

On 3/28/18 04:06, Fabien COELHO wrote:
> Would the project feel appropriate to:
> 
>   - fix these warnings (other than putting -Wno-format-truncation or
> similar workarounds...).

I've been tracking gcc-8, and I have a patch ready, but these things
change a bit with every compiler snapshot, so I'm waiting until things
settle down.

>   - add permanent gcc/clang trunk beasts with -Werror
> (if so, I'd suggest cannonball & seanettle for the names).

I would not like that.  The build farm success should ideally be a
function of correct PostgreSQL code, not external factors.  If an
in-progress compiler does funny things, what are we supposed to do about
that?

Generally, fixing up PostgreSQL for a new compiler release isn't a major
effort and can be done briefly before the release of the compiler.

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

Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

[Tom Lane]

Claudio Freire  writes:
> Attached patches, rebased and modified as discussed:
> 1 no longer does tree pruning, it just vacuums a range of the FSM
> 2 reintroduces tree pruning for the initial FSM vacuum
> 3 and 4 remain as they were, but rebased

I reviewed and cleaned up 0001.  The API for FreeSpaceMapVacuumRange was
underspecified, and the use of it seemed to have off-by-one errors.  Also,
you still had vacuum doing a full FreeSpaceMapVacuum call at the end;
I thought the point was to get rid of that.  We then need to make sure
we clean up after a truncation, but we can do that by introducing a
FreeSpaceMapVacuumRange call into FreeSpaceMapTruncateRel.  I think the
attached 0001 is committable, but feel free to take another look.

I still don't like 0002.  It's adding a lot of complexity, and
not-negligible overhead, to solve yesterday's problem.  After 0001,
there's no reason to assume that vacuum is particularly likely to get
cancelled between having made cleanups and having updated the upper FSM
levels.  (Maybe the odds are a bit more for the no-indexes case, but
that doesn't seem like it's common enough to justify a special mechanism
either.)

Not sure what to think about 0003.  At this point I'd be inclined
to flush UpdateFreeSpaceMap altogether and use FreeSpaceMapVacuumRange
in its place.  I don't think the design of that function is any better
chosen than its name, and possible bugs in its subroutines don't make
it more attractive.

Not sure about 0004 either.  The fact that we can't localize what part of
the index needs to be updated means that repeated IndexFreeSpaceMapVacuum
calls are a roughly quadratic cost.  Maybe in proportion to the other work
we have to do, they're not much, but on the other hand how much benefit is
there?  Should we make the call conditional on how many index pages got
updated?  Also, I wonder why you only touched nbtree and spgist.  (For
that matter, I wonder why BRIN doesn't go through IndexFreeSpaceMapVacuum
like the rest of the index AMs.  Or perhaps it has the right idea and we
should get rid of IndexFreeSpaceMapVacuum as a useless layer.)

regards, tom lane

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index f9da24c..d2a0066 100644
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
***
*** 85,90 
--- 85,99 
  #define VACUUM_TRUNCATE_LOCK_TIMEOUT			5000	/* ms */
  
  /*
+  * When a table has no indexes, vacuum the FSM after every 8GB, approximately
+  * (it won't be exact because we only vacuum FSM after processing a heap page
+  * that has some removable tuples).  When there are indexes, this is ignored,
+  * and we vacuum FSM after each index/heap cleaning pass.
+  */
+ #define VACUUM_FSM_EVERY_PAGES \
+ 	((BlockNumber) (((uint64) 8 * 1024 * 1024 * 1024) / BLCKSZ))
+ 
+ /*
   * Guesstimation of number of dead tuples per page.  This is used to
   * provide an upper limit to memory allocated when vacuuming small
   * tables.
*** lazy_vacuum_rel(Relation onerel, int opt
*** 285,293 
  	pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
   PROGRESS_VACUUM_PHASE_FINAL_CLEANUP);
  
- 	/* Vacuum the Free Space Map */
- 	FreeSpaceMapVacuum(onerel);
- 
  	/*
  	 * Update statistics in pg_class.
  	 *
--- 294,299 
*** lazy_scan_heap(Relation onerel, int opti
*** 465,471 
  	TransactionId relfrozenxid = onerel->rd_rel->relfrozenxid;
  	TransactionId relminmxid = onerel->rd_rel->relminmxid;
  	BlockNumber empty_pages,
! vacuumed_pages;
  	double		num_tuples,		/* total number of nonremovable tuples */
  live_tuples,	/* live tuples (reltuples estimate) */
  tups_vacuumed,	/* tuples cleaned up by vacuum */
--- 471,478 
  	TransactionId relfrozenxid = onerel->rd_rel->relfrozenxid;
  	TransactionId relminmxid = onerel->rd_rel->relminmxid;
  	BlockNumber empty_pages,
! vacuumed_pages,
! next_fsm_block_to_vacuum;
  	double		num_tuples,		/* total number of nonremovable tuples */
  live_tuples,	/* live tuples (reltuples estimate) */
  tups_vacuumed,	/* tuples cleaned up by vacuum */
*** lazy_scan_heap(Relation onerel, int opti
*** 501,506 
--- 508,514 
  		relname)));
  
  	empty_pages = vacuumed_pages = 0;
+ 	next_fsm_block_to_vacuum = (BlockNumber) 0;
  	num_tuples = live_tuples = tups_vacuumed = nkeep = nunused = 0;
  
  	indstats = (IndexBulkDeleteResult **)
*** lazy_scan_heap(Relation onerel, int opti
*** 752,757 
--- 760,772 
  			vacrelstats->num_dead_tuples = 0;
  			vacrelstats->num_index_scans++;
  
+ 			/*
+ 			 * Vacuum the Free Space Map to make newly-freed space visible on
+ 			 * upper-level FSM pages.  Note we have not yet processed blkno.
+ 			 */
+ 			FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum, blkno);
+ 			next_fsm_block_to_vacuum = blkno;
+ 
  			/* Report that we are once again scanning the heap */
  			pgstat_progress_u

Re: JIT compiling with LLVM v12

[Peter Eisentraut]

On 3/28/18 17:27, Andres Freund wrote:
> I've pushed these three.

Great, now the only thing remaining is to prepare an unconference
session explaining all this to the rest of us. ;-)

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

Re: WIP: Covering + unique indexes.

[Peter Eisentraut]

On 1/25/18 23:19, Thomas Munro wrote:
> +  PRIMARY KEY (  class="parameter">column_name [, ... ] )  class="parameter">index_parameters INCLUDE
> (column_name [,
> ...]) |
> 
> I hadn't seen that use of "" before.  Almost everywhere else
> we use explicit [ and ] characters, but I see that there are other
> examples, and it is rendered as [ and ] in the output.

I think this will probably not come out right in the generated psql
help.  Check that please.

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

Re: JIT compiling with LLVM v12

[Andres Freund]

Hi,

On 2018-03-28 18:06:24 -0400, Peter Eisentraut wrote:
> On 3/28/18 17:27, Andres Freund wrote:
> > I've pushed these three.
> 
> Great, now the only thing remaining is to prepare an unconference
> session explaining all this to the rest of us. ;-)

Hah! Happy to, if there's enough people interested.  I've a talk about
it too (state of jit, 2018 edition), but I wasn't planning to go into
too low level details. More about what is good, what is bad, and how we
make it better ;)

Greetings,

Andres Freund

Re: Fix a typo in walsender.c

[Bruce Momjian]

On Tue, Feb 27, 2018 at 07:22:20PM +0900, atorikoshi wrote:
> Hi,
> 
> Attached a minor patch for a typo: s/log/lag
> 
> Regards,
> 
> -- 
> Atsushi Torikoshi
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center

> diff --git a/src/backend/replication/walsender.c 
> b/src/backend/replication/walsender.c
> index d46374d..8bb1919 100644
> --- a/src/backend/replication/walsender.c
> +++ b/src/backend/replication/walsender.c
> @@ -1245,7 +1245,7 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr 
> lsn, TransactionId xid,
>  /*
>   * LogicalDecodingContext 'update_progress' callback.
>   *
> - * Write the current position to the log tracker (see XLogSendPhysical).
> + * Write the current position to the lag tracker (see XLogSendPhysical).
>   */
>  static void
>  WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn, 
> TransactionId xid)

Thanks, patch applied.

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

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

Re: JIT compiling with LLVM v12

[David Steele]

On 3/28/18 6:09 PM, Andres Freund wrote:


On 2018-03-28 18:06:24 -0400, Peter Eisentraut wrote:

On 3/28/18 17:27, Andres Freund wrote:

I've pushed these three.


Great, now the only thing remaining is to prepare an unconference
session explaining all this to the rest of us. ;-)


Hah! Happy to, if there's enough people interested.  I've a talk about
it too (state of jit, 2018 edition), but I wasn't planning to go into
too low level details. More about what is good, what is bad, and how we
make it better ;)


+1 for an unconference session.  This is some seriously cool stuff.

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

Re: Fix for pg_stat_activity putting client hostaddr into appname field

[Edmund Horner]

I sent the original in haste, and now I need to make some corrections... sigh.

> Subject: Fix for pg_stat_activity putting client hostaddr into appname field

Actually, it's the hostname appears in the appname field.

> I noticed when querying pg_stat_activity (in 10.1):

10.1 was where I first noticed the bug, but it's still present in master.

> I've tracked this down to bootstrap/pgstat.c.

Should be postmaster/pgstat.c.

> In the case of my query, the pointers for st_appname in the aux processes 
> happen to point into the BackendClientHostnameBuffer segment.

To be clear, I think this is a memory error.  These rogue pointers
could do a lot more damage than merely pointing to the wrong strings.

> It's an extra 3 kB ...

A rough estimate, that was also wrong.  7 aux processes * (1024 bytes
activity + 64 for hostname + 64 for appname) = about 8 kB.

I do apologise for the confusion!

Edmund

Re: Updating parallel.sgml's treatment of parallel joins

[Thomas Munro]

On Fri, Mar 23, 2018 at 6:26 AM, Robert Haas  wrote:
> On Fri, Feb 23, 2018 at 10:30 PM, Thomas Munro
>  wrote:
>> Here is an attempt at updating parallel.sgml to cover Parallel Hash.
>> I will be neither surprised nor offended if Robert would like to put
>> it differently!
>
> Looks nice.  Committed.

Thanks.

One thing that is slightly odd about that page[1] is that some places
use the style "parallel sequential scan" (lower
case, emphasis on first mention, head noun = "scan" or "join") and
other places use the style "Partial Aggregate node"
(title case, fixed width typeface, head noun = "node").

I think that page also needs a new  for "Parallel Append".
Perhaps the authors of commit ab727167 would like to write a patch for
that?  I'll be interested to see which style they go for...

[1] https://www.postgresql.org/docs/devel/static/parallel-plans.html

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

Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

[Claudio Freire]

On Wed, Mar 28, 2018 at 6:54 PM, Tom Lane  wrote:
> Claudio Freire  writes:
>> Attached patches, rebased and modified as discussed:
>> 1 no longer does tree pruning, it just vacuums a range of the FSM
>> 2 reintroduces tree pruning for the initial FSM vacuum
>> 3 and 4 remain as they were, but rebased
>
> I reviewed and cleaned up 0001.  The API for FreeSpaceMapVacuumRange was
> underspecified, and the use of it seemed to have off-by-one errors.  Also,
> you still had vacuum doing a full FreeSpaceMapVacuum call at the end;
> I thought the point was to get rid of that.  We then need to make sure
> we clean up after a truncation, but we can do that by introducing a
> FreeSpaceMapVacuumRange call into FreeSpaceMapTruncateRel.  I think the
> attached 0001 is committable, but feel free to take another look.

+
+ /*
+  * Periodically do incremental FSM vacuuming to make newly-freed
+  * space visible on upper FSM pages.  Note: although we've cleaned
+  * the current block, we haven't yet updated its FSM entry (that
+  * happens further down), so passing end == blkno is correct.
+  */
+ if (blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
+ {
+ FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum,
+ blkno);
+ next_fsm_block_to_vacuum = blkno;
+ }
  }

Using next_fsm_block_to_vacuum there seems strange.

v10 counted the number of blocks with updated free space to vacuum the
FSM only after a lot of changes to it were made. This will vacuum the
FSM after *scanning* a lot of pages, even if little modifications were
made to them. Not sure which is better, but the logic in v10 sounded
right to me.

! if (fsm_end.logpageno == addr.logpageno)
! end_slot = fsm_end_slot;
! else if (fsm_end.logpageno > addr.logpageno)
! end_slot = SlotsPerFSMPage;
! else
! end_slot = -1;
!
! for (slot = start_slot; slot < end_slot; slot++)

This doesn't seem correct.

The +1 in v10 was intentional. Suppose a leaf node of the FSM has a
parent in slot 3, and that one has a parent at slot 10.

This would vacuum slots 0-9 on the upper level, but not 10. That would
leave a whole branch (the one where the end block belongs) unvisited.

That's why the end_slot has to be inclusive of the end block. We have
work to do recursing the end_slot.


> I still don't like 0002.  It's adding a lot of complexity, and
> not-negligible overhead, to solve yesterday's problem.

Are you sure it's non-negligible?

Most of my benchmarks couldn't measure any change whatsoever after
this patch in regards to run/cpu time.

The size of the FSM is so much smaller than the table, that the cost
of vacuuming it is drowned by all the other work done and buried under
the noise.

Maybe under very special cases where vacuum does nothing, skipping all
rows, it might be measurable. A heavily bloated-then-cleaned table
with few live rows per page, but all frozen, that would be a total
worst-case. But reading the visibility map to skip rows is comparable
work to vacuuming the FSM. There's no reason to think it would worsen
by *that* much. I might try to benchmark that a bit after the long
weekend.

Anyway, it's a separate patch to be independently committed/vetted.

> After 0001,
> there's no reason to assume that vacuum is particularly likely to get
> cancelled between having made cleanups and having updated the upper FSM
> levels.  (Maybe the odds are a bit more for the no-indexes case, but
> that doesn't seem like it's common enough to justify a special mechanism
> either.)

Why not?

Any kind of DDL (even those that don't rewrite the heap) would cancel
autovacuum.

You might think DDL isn't common enough to worry about, but I've seen
cases where regular reindex were required to keep index bloat in check
(and were cron'd), and those cancel autovacuum all the time.

> Not sure about 0004 either.  The fact that we can't localize what part of
> the index needs to be updated means that repeated IndexFreeSpaceMapVacuum
> calls are a roughly quadratic cost.

Well, the index bulk delete itself already is a huge elephant-sized
quadratic cost.

The FSM is only the cherry on top.

The updated part can't be localize because it isn't. All blocks could
potentially be changed. Even in correlated indexes, upper levels need
not be physically correlated and would screw with the "vacuum block
range" optimization.

I could try to optimize the case where it's possible, by recording the
first and last blocks modified, but that would be a hugely invasive
patch (it would have to touch a lot of places in btree code).

And index bloat is a very real issue, as bad as heap bloat is.

>  Maybe in proportion to the other work
> we have to do, they're not much, but on the other hand how much benefit is
> there?

A week-long multi-pass vacuum 

Re: [PROPOSAL] Shared Ispell dictionaries

[Arthur Zakirov]

Here is the new version of the patch.

Now RemoveTSDictionaryById() and AlterTSDictionary() unpin the
dictionary DSM segment. So if all attached backends disconnect allocated
DSM segments will be released.

lookup_ts_dictionary_cache() may unping DSM mapping for all invalid
dictionary cache entries.

I added xmax in DictPointerData. It is used as a lookup key now too. It
helps to reload a dictionary after roll back DROP command.

There was a bug in ts_dict_shmem_location(), I fixed it.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index b9fdd77e19..e071994523 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -78,6 +78,8 @@
 #define tmpalloc(sz)  MemoryContextAlloc(Conf->buildCxt, (sz))
 #define tmpalloc0(sz)  MemoryContextAllocZero(Conf->buildCxt, (sz))
 
+#define tmpstrdup(str) MemoryContextStrdup(Conf->buildCxt, (str))
+
 /*
  * Prepare for constructing an ISpell dictionary.
  *
@@ -498,7 +500,7 @@ NIAddSpell(IspellDict *Conf, const char *word, const char 
*flag)
Conf->Spell[Conf->nspell] = (SPELL *) tmpalloc(SPELLHDRSZ + 
strlen(word) + 1);
strcpy(Conf->Spell[Conf->nspell]->word, word);
Conf->Spell[Conf->nspell]->p.flag = (*flag != '\0')
-   ? cpstrdup(Conf, flag) : VoidString;
+   ? tmpstrdup(flag) : VoidString;
Conf->nspell++;
 }
 
@@ -1040,7 +1042,7 @@ setCompoundAffixFlagValue(IspellDict *Conf, 
CompoundAffixFlag *entry,
entry->flag.i = i;
}
else
-   entry->flag.s = cpstrdup(Conf, s);
+   entry->flag.s = tmpstrdup(s);
 
entry->flagMode = Conf->flagMode;
entry->value = val;
@@ -1536,6 +1538,9 @@ nextline:
return;
 
 isnewformat:
+   pfree(recoded);
+   pfree(pstr);
+
if (oldformat)
ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
diff --git a/contrib/dict_int/dict_int.c b/contrib/dict_int/dict_int.c
index 56ede37089..8dd4959028 100644
--- a/contrib/dict_int/dict_int.c
+++ b/contrib/dict_int/dict_int.c
@@ -30,7 +30,7 @@ PG_FUNCTION_INFO_V1(dintdict_lexize);
 Datum
 dintdict_init(PG_FUNCTION_ARGS)
 {
-   List   *dictoptions = (List *) PG_GETARG_POINTER(0);
+   DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictInt*d;
ListCell   *l;
 
@@ -38,7 +38,7 @@ dintdict_init(PG_FUNCTION_ARGS)
d->maxlen = 6;
d->rejectlong = false;
 
-   foreach(l, dictoptions)
+   foreach(l, init_data->dict_options)
{
DefElem*defel = (DefElem *) lfirst(l);
 
diff --git a/contrib/dict_xsyn/dict_xsyn.c b/contrib/dict_xsyn/dict_xsyn.c
index a79ece240c..0b8a32d459 100644
--- a/contrib/dict_xsyn/dict_xsyn.c
+++ b/contrib/dict_xsyn/dict_xsyn.c
@@ -140,7 +140,7 @@ read_dictionary(DictSyn *d, const char *filename)
 Datum
 dxsyn_init(PG_FUNCTION_ARGS)
 {
-   List   *dictoptions = (List *) PG_GETARG_POINTER(0);
+   DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSyn*d;
ListCell   *l;
char   *filename = NULL;
@@ -153,7 +153,7 @@ dxsyn_init(PG_FUNCTION_ARGS)
d->matchsynonyms = false;
d->keepsynonyms = true;
 
-   foreach(l, dictoptions)
+   foreach(l, init_data->dict_options)
{
DefElem*defel = (DefElem *) lfirst(l);
 
diff --git a/contrib/unaccent/unaccent.c b/contrib/unaccent/unaccent.c
index 247c202755..2a2fbee5fa 100644
--- a/contrib/unaccent/unaccent.c
+++ b/contrib/unaccent/unaccent.c
@@ -267,12 +267,12 @@ PG_FUNCTION_INFO_V1(unaccent_init);
 Datum
 unaccent_init(PG_FUNCTION_ARGS)
 {
-   List   *dictoptions = (List *) PG_GETARG_POINTER(0);
+   DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
TrieChar   *rootTrie = NULL;
boolfileloaded = false;
ListCell   *l;
 
-   foreach(l, dictoptions)
+   foreach(l, init_data->dict_options)
{
DefElem*defel = (DefElem *) lfirst(l);
 
diff --git a/src/backend/commands/tsearchcmds.c 
b/src/backend/commands/tsearchcmds.c
index 3a843512d1..3753e32b2c 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -386,17 +386,25 @@ verify_dictoptions(Oid tmplId, List *dictoptions)
}
else
{
+   DictInitData init_data;
+
/*
 * Copy the options just in case init method thinks it can 
scribble on
 * them ...
 */
dictoptions = copyObject(dictoptions);
 
+   init_data.dict_options = dictoptions;
+   init_data.dict.id = InvalidOid;
+   init_data.dict.xmin = InvalidTransactionId;
+   init_data.dict.xmax = InvalidTransactionId;
+   ItemPointerSetInvalid(&in

Re: disable SSL compression?

[Peter Eisentraut]

On 3/28/18 13:26, Konstantin Knizhnik wrote:
> If SSL compression is deprecated, should we provide own compression?
> I have implemented some prototype implementation of it (patch is attached).
> I have added compression=on/off parameter to connection string and -Z
> option to psql and pgbench utilities.

What I'd like to see here is extensive protocol documentation that
describes the compression method negotiation, and the interaction with
SSL, and a test suite to support that.

Maybe start a new thread.

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

Re: committing inside cursor loop

[Peter Eisentraut]

On 3/28/18 11:34, Ildus Kurbangaliev wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
> 
> I have checked new version. Although I can miss something,  the patch looks 
> good to me.
> 
> The new status of this patch is: Ready for Committer

Committed, thanks!

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

Re: disable SSL compression?

[Andres Freund]

On March 28, 2018 4:15:02 PM PDT, Peter Eisentraut 
 wrote:
>On 3/28/18 13:26, Konstantin Knizhnik wrote:
>> If SSL compression is deprecated, should we provide own compression?
>> I have implemented some prototype implementation of it (patch is
>attached).
>> I have added compression=on/off parameter to connection string and -Z
>> option to psql and pgbench utilities.
>
>What I'd like to see here is extensive protocol documentation that
>describes the compression method negotiation, and the interaction with
>SSL, and a test suite to support that.
>
>Maybe start a new thread.

+analysis of whether that's safe to do from a cryptographic POV. There's a 
reason compression has been disabled for just about all SSL/TLS libraries.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: csv format for psql

[Isaac Morland]

On 28 March 2018 at 15:43, Joshua D. Drake  wrote:

> On 03/28/2018 12:35 PM, David G. Johnston wrote:
>
> I like to call it "Character Separated Values" now for just that reason.
>
>
> Isn't the actual wording Character Delimited Values? I may be picking at
> hairs here but every single time I use anything to import a CSV or other
> delimited file (TAB or | usually) that is what the import screen says.
>

CSV stands for Comma Separated Values, and not anything else common as far
as I can tell. A Google search for "csv" turns up the Wikipedia page
describing the file format as the first hit, followed by the Wikipedia
disambiguation page for CSV, which links to the aforementioned Wikipedia
page as the only file-format-related link.

The actual reason I'm posting this is because some of the discussion has
made me a bit confused: there is already a CSV format defined for the COPY
command and used by the psql \copy. I just want to check that what is being
discussed here would use the exact same format as the existing CSV COPY
format; and the configurability of them should be the exact same options
(which already includes being able to change the delimiter). I think it's
important that Postgres not have two CSVs with slightly different
behaviours. Indeed, psql could use COPY behind the scenes to generate the
CSV output, which would guarantee no nasty surprises.

If this is already widely accepted or if I'm horribly misunderstanding the
discussion then I'm sorry for the noise.