Re: Fixes for missing schema qualifications

[Michael Paquier]

On Thu, Nov 29, 2018 at 10:29:04PM -0800, Noah Misch wrote:
> This patch provides no meaningful increment in security or reliability, but it
> does improve stylistic consistency.  Fine to proceed on those grounds, but
> this description doesn't fit.

Indeed, you are right.  I agree.

> > --- a/src/test/modules/worker_spi/worker_spi.c
> > +++ b/src/test/modules/worker_spi/worker_spi.c
> > @@ -115,7 +115,9 @@ initialize_worker_spi(worktable *table)
> >  
> > /* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
> > initStringInfo(&buf);
> > -   appendStringInfo(&buf, "select count(*) from pg_namespace where nspname 
> > = '%s'",
> > +   appendStringInfo(&buf,
> > +"select pg_catalog.count(*) "
> > +"from pg_catalog.pg_namespace where 
> > nspname = '%s'",
> >  table->schema);
> 
> Remove this change.  The rest of the file doesn't schema-qualify, which is
> appropriate for code implementing a test case.

No problem with that either.  Thanks Noah for the lookup.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)

[Kyotaro HORIGUCHI]

Hello.

At Fri, 30 Nov 2018 07:48:26 +0100, Pavel Stehule  
wrote in 
> Hi
> 
> čt 29. 11. 2018 v 14:44 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
> 
> > > On Fri, Sep 21, 2018 at 1:30 PM Pavel Stehule 
> > wrote:
> > >
> > > Thank you for comments
> > >
> > > Attached updated patch
> >
> > Unfortunately, current version of the patch doesn't pass make check,
> > something
> > is missing for xml tests. Could you please rebase it?
> >
> > After that I hope someone from reviewers (Kyotaro?) can probably confirm if
> > it's still in a good shape. For now I'm moving it to the next CF.

Sure. Sorry for coming late. I reconfirmed this.

The most significant change in this version is namespace name
generaton.

- We can remove strlen from mutate_name by mutating the string in
  reverse order. We don't need to mutate it in a human-affinity
  order. The name would be 1-letter in almost all cases.

  Concretely, the order in my mind is the follows:

  "" "a" "b" ..."z" "aa" "ba" "ca"... "za" "ab"..

- Might the 'propriety' correctly be 'properties'?

 + /* register namespace if all propriety are available */


- Is the "if" a mistake of "in"?

 + * collect ns names if ResTarget format for possible usage
 + * in getUniqNames function.


- I suppose the following should be like "register default
  namespace definition if any".

 +/* get default namespace name when it is required */


Maybe the followings are not new. (Note that I'm not a naitive speaker.)

- I cannot read this. (I might be to blame..)

  + * default namespace for XPath expressions. Because there are not any API
  + * how to transform or access to parsed XPath expression we have to parse
  + * XPath here.

- This might need to explain "by what".

 + * Those functionalities are implemented with a simple XPath parser/
 + * preprocessor. This XPath parser transforms a XPath expression to another
 + * XPath expression that can be used by libxml2 XPath evaluation. It doesn't
 + * replace libxml2 XPath parser or libxml2 XPath expression evaluation.

- "add" -> "adds", "def_namespace_name" seems to need to be
  replaced with something else.

 + * This transformation add def_namespace_name to any unqualified node name
 + * or attribute name of xpath expression.

(Sorry, I'll look further later.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: pg_config wrongly marked as not parallel safe?

[Kyotaro HORIGUCHI]

At Thu, 29 Nov 2018 15:03:00 -0800, Andres Freund  wrote in 
<20181129230300.vkj3csjwk7jt2...@alap3.anarazel.de>
> Hi,
> 
> On 2018-11-29 16:23:42 -0500, Robert Haas wrote:
> > Generally, I think Andres is wrong to argue that immutability
> > shouldn't mean *anything* across major versions.  If we can readily
> > foresee that something is going to change in the future, then we
> > shouldn't mark it immutable. However:
> 
> I was too glib/brief. All I meant is that we shouldn't take immutable to
> *guarantee* anything across major versions. We, of course, shouldn't
> break things willy-nilly, and consider the consequences of such
> potential breaking changes. Including having to reindex. It's not like
> that's only the case for changing immutable functions, the index storage
> itself etc also matter.

FWIW, I agree to this.

# And returning to the topic, I vote for pg_config should be "stable".

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: New GUC to sample log queries

[Adrien NAYRAT]

On 11/29/18 10:48 PM, Alvaro Herrera wrote:

Thanks!  I pushed this with two changes -- one was to reword the docs a
bit more, and the other was to compute in_sample only if it's going to
be used (when exceeded is true).  I hope this won't upset any compilers ...


Thanks!



I wonder if there's any sensible way to verify the behavior in an
automated fashion.  Who know what marvels we'd discover about the
reliability/uniformity of random() across platforms.



Yes, I also wondered how to add tests.

Re: New GUC to sample log queries

[Adrien NAYRAT]

On 11/30/18 7:42 AM, Nikolay Samokhvalov wrote:
On Thu, Nov 29, 2018 at 1:49 PM Alvaro Herrera > wrote:


Thanks!  I pushed this with two changes -- one was to reword the docs a
bit more, and the other was to compute in_sample only if it's going to
be used (when exceeded is true).  I hope this won't upset any
compilers ...


This is a great news – I can imaging how helpful this feature will be 
for query analysis and

troubleshooting.

At the same time, there is an approach, when we use application (client) 
code or pgbouncer's
connect_query parameter to perform sampled logging (with 
log_min_duration_statement = 0)

of n% of all *sessions* or *transactions*.


Good idead! Unfortunately it require pgbouncer, but I keep this trick in 
mind.




If you use single-query transactions only, new parameter will do 
equivalent job for you, while
significantly simplifying you life (pgbouncer is not required and you 
don't need to patch application
code). However, if you have multi-statement transaction, 
log_statement_sample_rate will not
be enough for troubleshooting – logging just a single statement of a 
multi-statement transaction

won't really help to troubleshoot in many cases.

That being said, I wonder, does it make sense to think about extending 
the functionality
just committed, with some options to to log all statements of n% of 
transactions (or sessions)?
In other words, allowing to choose, at which level perform sampling – 
statement, transaction, or

session?



+1, I like this idea.

Re: [Todo item] Add entry creation timestamp column to pg_stat_replication

[Michael Paquier]

On Thu, Nov 29, 2018 at 05:43:26PM +0900, myungkyu.lim wrote:
> Changed field name 'last_msg_send_time' to 'reply_time'.

Looks pretty to me at quick glance, unfortunately I have not spent much
time on it, particularly testing it.

+
+ reply_time
+ timestamp with time zone
+ Send time of last message received from WAL
receiver
+
This is a bit confusing as this counts for the last *standby* message
('r'), and not the last feedback message ('h').

+   /*
+* timestamp of the latest message, reported by standby server
+   */
+   TimestampTz replyTime;

A small indentation problem, not a big deal.

Sawada-san, perhaps you would like to look at it?
--
Michael


signature.asc
Description: PGP signature

Re: Changing the autovacuum launcher scheduling; oldest table first algorithm

[Dmitry Dolgov]

> On Fri, Nov 30, 2018 at 3:05 AM Masahiko Sawada  wrote:
>
> On Fri, Nov 30, 2018 at 10:48 AM Michael Paquier  wrote:
> >
> > On Thu, Nov 29, 2018 at 06:21:34PM +0100, Dmitry Dolgov wrote:
> > > Nothing changed since then, but also the patch got not enough review to 
> > > say
> > > that there was substantial feedback. I'll move it to the next CF.
> >
> > I would have suggested to mark the patch as returned with feedback
> > instead as the thing does not apply for some time now, and the author
> > has been notified about a rebase.
>
> Agreed and sorry for the late reply.
>
> The design of this patch would need to be reconsidered based on
> suggestions and discussions we had before. I'll propose it again after
> considerations.

Well, taking into account this information, then yes, it makes sense and I'll
mark it as "Returned with feedback", thanks!

Re: postgres_fdw: oddity in costing aggregate pushdown paths

[Etsuro Fujita]

(2018/11/28 13:38), Etsuro Fujita wrote:
> BTW another thing I noticed is this comment on costing aggregate
> pushdown paths using local statistics in estimate_path_cost_size:
> 
>   * Also, core does not care about costing HAVING expressions and
>   * adding that to the costs.  So similarly, here too we are not
>   * considering remote and local conditions for costing.
> 
> I think this was true when aggregate pushdown went in, but isn't anymore
> because of commit 7b6c07547190f056b0464098bb5a2247129d7aa2.  So we
> should update estimate_path_cost_size so that it accounts for the
> selectivity and cost of the HAVING expressions as well?

There seems to be no objections, I updated the patch as such.  Attached
is an updated version of the patch.

Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index e653c30..dfa6201 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -3209,6 +3209,8 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
  Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE (("C 1" < 100)) AND ((c2 = 6))
 (6 rows)
 
+-- Update local stats on ft2
+ANALYZE ft2;
 -- Add into extension
 alter extension postgres_fdw add operator class my_op_class using btree;
 alter extension postgres_fdw add function my_op_cmp(a int, b int);
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d22c974..d9b26b5 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2844,10 +2844,6 @@ estimate_path_cost_size(PlannerInfo *root,
 			 * strategy will be considered at remote side, thus for
 			 * simplicity, we put all startup related costs in startup_cost
 			 * and all finalization and run cost are added in total_cost.
-			 *
-			 * Also, core does not care about costing HAVING expressions and
-			 * adding that to the costs.  So similarly, here too we are not
-			 * considering remote and local conditions for costing.
 			 */
 
 			ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;
@@ -2880,10 +2876,25 @@ estimate_path_cost_size(PlannerInfo *root,
 			input_rows, NULL);
 
 			/*
-			 * Number of rows expected from foreign server will be same as
-			 * that of number of groups.
+			 * Get the retrieved-rows and rows estimates.  If there are HAVING
+			 * quals, account for the selectivities of the remotely-checked
+			 * and locally-checked quals.
 			 */
-			rows = retrieved_rows = numGroups;
+			if (root->parse->havingQual)
+			{
+retrieved_rows =
+	clamp_row_est(numGroups *
+  clauselist_selectivity(root,
+		 fpinfo->remote_conds,
+		 0,
+		 JOIN_INNER,
+		 NULL));
+rows = clamp_row_est(retrieved_rows * fpinfo->local_conds_sel);
+			}
+			else
+			{
+rows = retrieved_rows = numGroups;
+			}
 
 			/*-
 			 * Startup cost includes:
@@ -2909,6 +2920,20 @@ estimate_path_cost_size(PlannerInfo *root,
 			run_cost += aggcosts.finalCost * numGroups;
 			run_cost += cpu_tuple_cost * numGroups;
 			run_cost += ptarget->cost.per_tuple * numGroups;
+
+			/* Accout for the eval cost of HAVING quals, if any */
+			if (root->parse->havingQual)
+			{
+QualCost	remote_cost;
+
+/* Add in the eval cost of the remotely-checked quals */
+cost_qual_eval(&remote_cost, fpinfo->remote_conds, root);
+startup_cost += remote_cost.startup;
+run_cost += remote_cost.per_tuple * numGroups;
+/* Add in the eval cost of the locally-checked quals */
+startup_cost += fpinfo->local_conds_cost.startup;
+run_cost += fpinfo->local_conds_cost.per_tuple * retrieved_rows;
+			}
 		}
 		else
 		{
@@ -5496,6 +5521,19 @@ add_foreign_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
 	if (!foreign_grouping_ok(root, grouped_rel, extra->havingQual))
 		return;
 
+	/*
+	 * Compute the selectivity and cost of the local_conds, so we don't have
+	 * to do it over again for each path.  The best we can do for these
+	 * conditions is to estimate selectivity on the basis of local statistics.
+	 */
+	fpinfo->local_conds_sel = clauselist_selectivity(root,
+	 fpinfo->local_conds,
+	 0,
+	 JOIN_INNER,
+	 NULL);
+
+	cost_qual_eval(&fpinfo->local_conds_cost, fpinfo->local_conds, root);
+
 	/* Estimate the cost of push down */
 	estimate_path_cost_size(root, grouped_rel, NIL, NIL, &rows,
 			&width, &startup_cost, &total_cost);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 6aa9a7f..f963e99 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -807,6 +807,9 @@ create operator class my_op_class for type int using btree family my_op_family a
 explain (verbose, costs off)
 select array_a

Re: [HACKERS] Cached plans and statement generalization

[Dmitry Dolgov]

> On Fri, Nov 30, 2018 at 3:06 AM Yamaji, Ryo  wrote:
>
> On Fri, Nov 30, 2018 at 3:48 AM, Dmitry Dolgov wrote:
>
> > Hi,
> >
> > Thanks for reviewing. Since another CF is about to end, maybe you can
> > post the full review feedback?
>
> Since I had been busy with my private work, I couldn't review.
> I want to review next commit fest. I am sorry I postponed many times.

No worries, just don't forget to post a review when it will be ready.

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

[Matsumura, Ryo]

Meskes-san

Sorry to bother you, but I hope any comment of yours.

Regards
Ryo Matsumura

> Subject: RE: [PROPOSAL]a new data type 'bytea' for ECPG
> 
> > From: Tsunakawa, Takayuki [mailto:tsunakawa.ta...@jp.fujitsu.com]
> >
> > I think the host variable data type that corresponds to the server-side 
> > bytea
> > should be bytea.  As the following pages state or imply, it would be better
> > to create standard-compliant LOB types someday, and use the keyword BLOB
> in
> > ECPG for that type.  The server-side data types should have the names BLOB,
> > CLOB and NCLOB.  Those types should handle data larget than 1 GB and have
> the
> > locator feature defined in the SQL standard.  Maybe we should also advanced
> > LOB features like Oracle's SecureFiles LOB and SQL Server's FileTables.
> 
> Tsunakawa-san, thanks for your advice.
> I understand that C type definition of client-side bytea is not constrained
> by the standard BLOB.
> 
> What should I do next?
> For now, I attach a patch that is removed noise(pgindent/typedef.list).
> 
> P.S.
> The patch does not support ECPG.bytea in sqltype of "struct sqlvar_struct"
> because of compatibility.
> 
> Regards
> Ryo Matsumura

Re: Reduce amount of WAL generated by CREATE INDEX for gist, gin and sp-gist

[Dmitry Dolgov]

> On Tue, Jul 31, 2018 at 6:36 AM Andrey Lepikhov  
> wrote:
>
> With the consent of Anastasia I will improving this patch further.
> Attachment contains next version of the patch set.

Thank you. I played a bit with this patch, and can confirm visible WAL size
reduction (it's rather obvious, but still). Although, probably due to lack of
my knowledge here, I wonder how does it work when an index is build
concurrently?

> Benchmarks:
> ---
>
> Test: pgbench -f gin-WAL-test.sql -t 5:
> ---
> master:
> Latency average: 27696.299 ms
> WAL size: 2.66 GB

Does it makes sense to measure latency of the entire script, since it contains
also some preparation work? Of course it still shows the difference between
patched version and master, but probably in a more noisy way.

I'm moving this patch to the next CF.

Re: Speeding up text_position_next with multibyte encodings

[Dmitry Dolgov]

> At Fri, 19 Oct 2018 15:52:59 +0300, Heikki Linnakangas  wrote
> Attached is a patch to speed up text_position_setup/next(), in some
> common cases with multibyte encodings.

Hi,

Unfortunately, patch doesn't compile anymore due:

varlena.c: In function ‘text_position_next_internal’:
varlena.c:1337:13: error: ‘start_ptr’ undeclared (first use in this function)
  Assert(start_ptr >= haystack && start_ptr <= haystack_end);

Could you please send an updated version? For now I'm moving it to the next CF.

Re: FETCH FIRST clause PERCENT option

[Surafel Temesgen]

On Fri, Nov 30, 2018 at 10:23 AM Surafel Temesgen 
wrote:


> Attach is a patch that include a fix for it
>
>
By mistake the patch coerce OFFSET clause to float8 too which is not
necessary .
The attached patch correct it
regards
Surafel
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 4db8142afa..8491b7831a 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
 [ LIMIT { count | ALL } ]
 [ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY ]
 [ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
 
 where from_item can be one of:
@@ -1397,7 +1397,7 @@ OFFSET start
 which PostgreSQL also supports.  It is:
 
 OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY
 
 In this syntax, the start
 or count value is required by
@@ -1407,7 +1407,8 @@ FETCH { FIRST | NEXT } [ count ] {
 ambiguity.
 If count is
 omitted in a FETCH clause, it defaults to 1.
-ROW
+with PERCENT count specifies the maximum number of rows to return 
+in percentage.ROW
 and ROWS as well as FIRST
 and NEXT are noise words that don't influence
 the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index 6792f9e86c..ddc11bf6ec 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -43,6 +43,7 @@ ExecLimit(PlanState *pstate)
 	LimitState *node = castNode(LimitState, pstate);
 	ScanDirection direction;
 	TupleTableSlot *slot;
+	TupleDesc   tupleDescriptor;
 	PlanState  *outerPlan;
 
 	CHECK_FOR_INTERRUPTS();
@@ -52,6 +53,8 @@ ExecLimit(PlanState *pstate)
 	 */
 	direction = node->ps.state->es_direction;
 	outerPlan = outerPlanState(node);
+	slot = node->subSlot;
+	tupleDescriptor = node->ps.ps_ResultTupleDesc;
 
 	/*
 	 * The main logic is a simple state machine.
@@ -60,6 +63,23 @@ ExecLimit(PlanState *pstate)
 	{
 		case LIMIT_INITIAL:
 
+			if (node->limitOption == PERCENTAGE)
+			{
+
+			/*
+			 * Find all rows the plan will return.
+			 */
+for (;;)
+{
+	slot = ExecProcNode(outerPlan);
+	if (TupIsNull(slot))
+	{
+		break;
+	}
+	tuplestore_puttupleslot(node->totalTuple, slot);
+}
+			}
+
 			/*
 			 * First call for this node, so compute limit/offset. (We can't do
 			 * this any earlier, because parameters from upper nodes will not
@@ -87,24 +107,46 @@ ExecLimit(PlanState *pstate)
 return NULL;
 			}
 
-			/*
-			 * Fetch rows from subplan until we reach position > offset.
-			 */
-			for (;;)
+			if (node->limitOption == PERCENTAGE)
 			{
-slot = ExecProcNode(outerPlan);
-if (TupIsNull(slot))
+for (;;)
 {
-	/*
-	 * The subplan returns too few tuples for us to produce
-	 * any output at all.
-	 */
-	node->lstate = LIMIT_EMPTY;
-	return NULL;
+	slot = MakeSingleTupleTableSlot(tupleDescriptor, &TTSOpsMinimalTuple);
+	if (!tuplestore_gettupleslot(node->totalTuple, true, true, slot))
+	{
+		node->lstate = LIMIT_EMPTY;
+		return NULL;
+	}
+	else
+	{
+		node->subSlot = slot;
+		if (++node->position > node->offset)
+		break;
+	}
+}
+			}
+			else if (node->limitOption == EXACT_NUMBER)
+			{
+
+/*
+ * Fetch rows from subplan until we reach position > offset.
+ */
+for (;;)
+{
+	slot = ExecProcNode(outerPlan);
+	if (TupIsNull(slot))
+	{
+		/*
+		 * The subplan returns too few tuples for us to produce
+		 * any output at all.
+		 */
+		node->lstate = LIMIT_EMPTY;
+		return NULL;
+	}
+	node->subSlot = slot;
+	if (++node->position > node->offset)
+		break;
 }
-node->subSlot = slot;
-if (++node->position > node->offset)
-	break;
 			}
 
 			/*
@@ -144,18 +186,34 @@ ExecLimit(PlanState *pstate)
 
 	return NULL;
 }
+if (node->limitOption == PERCENTAGE)
+{
+	if (tuplestore_gettupleslot(node->totalTuple, true, false, slot))
+	{
+		node->subSlot = slot;
+		node->position++;
+	}
+	else
+	{
+		node->lstate = LIMIT_SUBPLANEOF;
+		return NULL;
+	}
+}
+else if (node->limitOption == EXACT_NUMBER)
+{
 
-/*
- * Get next tuple from subplan, if any.
- */
-slot = ExecProcNode(outerPlan);
-if (TupIsNull(slot))
-{
-	node->lstate = LIMIT_SUBPLANEOF;
-	return NULL;
+	 /*
+	 * Get next tuple from subplan, if any.
+	 */
+	slot = ExecProcNode(outerPlan);
+	if (TupIsNull(slot))
+	{
+		node->lstate = LIMIT_SUBPLANEOF;
+		return NULL;
+

Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full

[Marius Timmer]

Hello Thomas,

thank you for reviewing our patch.

> Why did you put "trust" there instead of "$authmethod" like the previous 
> lines?
That is a good question in deed. We changed that accordingly.

> The tests pass and show the feature working correctly.  I think this
> is getting close to committable. 

We implemented all other suggestions in the attached patch version...

> I see that Magnus has signed up a committer.
... and changed status to "Ready for Committer" :-).


Kind regards,

Marius Timmer




-- 
Westfälische Wilhelms-Universität Münster (WWU)
Zentrum für Informationsverarbeitung (ZIV)
Röntgenstraße 7-13
Besucheradresse: Einsteinstraße 60 - Raum 101
48149 Münster
+49 251 83 31158
marius.tim...@uni-muenster.de
https://www.uni-muenster.de/ZIV
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c2114021c3..411f1e1679 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   cn (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behavior is similar to the cert authentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
@@ -1865,11 +1872,11 @@ host ... ldap ldapserver=ldap.example.net ldapbasedn="dc=example, dc=net" ldapse

 In a pg_hba.conf record specifying certificate
 authentication, the authentication option clientcert is
-assumed to be 1, and it cannot be turned off since a client
-certificate is necessary for this method.  What the cert
-method adds to the basic clientcert certificate validity test
-is a check that the cn attribute matches the database
-user name.
+assumed to be verify-ca or verify-full,
+and it cannot be turned off since a client certificate is necessary for this
+method. What the cert method adds to the basic
+clientcert certificate validity test is a check that the
+cn attribute matches the database user name.

   
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 8d9d40664b..ef515535a8 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2287,13 +2287,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or its mapping
+   matches the cn (Common Name) of the provided certificate.
+   Note that certificate chain validation is always ensured when the
+   cert authentication method is used
+   (see ).
   
 
   
@@ -2312,18 +2324,34 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
The clientcert authentication option is available for
all authentication methods, but only in pg_hba.conf lines
specified as hostssl.  When clientcert is
-   not specified or is set to 0, the server will still verify any presented
-   client certificates against its CA file, if one is configured — but
-   it will not insist that a client certificate be presented.
+   not specified or is set to no-verify, the server will still
+   verify any presented client certificates against its CA file, if one is
+   configured — but it will not insist that a client certificate be presented.
+  
+
+  
+   There are two approaches to enforce that users provide a certificate during login.
+  
+
+  
+   The firs

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

[Etsuro Fujita]

(2018/11/30 2:58), Dmitry Dolgov wrote:

On Mon, Oct 1, 2018 at 2:55 PM Etsuro Fujita  
wrote:

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

On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
Fujita-san, you are registered as a reviewer of this patch.  Are you
planning to look at it soon?


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


Yep, it would be nice to have a full review here. But as far as I see the issue
you were talking about is still in progress, right?


That's right.


Also looks like someone
needs to take over this rather small patch.


I changed my mind; I thought it would be better to work on that issue 
before this, but Tom raised concerns about the patch I proposed for that 
issue, and I think it'll take much longer time to address that, so I'd 
like to get this done first.


One thing we would need to discuss more about this is the name of a new 
function added by the patch.  IIRC, we have three options so far [1]:


1) perform_one_foreign_dml proposed by Ashutosh
2) execute_dml_single_row proposed by Michael
3) execute_parameterized_dml_stmt proposed by me

I'll withdraw my proposal; I think #1 and #2 would describe the 
functionality better than #3.  My vote would go to #2 or 
execute_dml_stmt_single_row, which moves the function much more closer 
to execute_dml_stmt.  I'd like to hear the opinions of others.


Thanks!

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/CAFjFpRc%2BMdhD2W%3Dofc06n3NriqPCy6ztNBehXTc_g_y9%3DP5k1w%40mail.gmail.com

Re: New GUC to sample log queries

[Sergei Kornilov]

Hello

I can not build current HEAD cleanly. I got warning:

> postgres.c: In function ‘check_log_duration’:
> postgres.c:2254:17: warning: ‘in_sample’ may be used uninitialized in this 
> function [-Wmaybe-uninitialized]
>   if ((exceeded && in_sample) || log_duration)

Should not we have such change?

> -boolin_sample;
> +boolin_sample = false;

thank you!

regards, Sergei

Re: Add extension options to control TAP and isolation tests

[Arthur Zakirov]

On 29.11.2018 05:00, Michael Paquier wrote:

The main patch had to be reverted a couple of days ago via 1d7dd18, and
I have been digging and fixing the set of issues the buildfarm has been
complaining about since.  There were two problems:


Thank you for working on the patch.

I run world-check. It works perfectly. I think the patch is in good shape.


I am attaching the patch I would like to commit to close this thread,
which has as single change TAP_TESTS commented out in contrib/bloom/ to
not explode the buildfarm.  Any objections to that?


As far as I understand bloom TAP tests are stable on some platforms, 
such as linux. Can be TAP test disabled only for some specific 
platforms? For example (as far as I know 'darwin' refers to MacOS):


ifeq (,$(filter win32 darwin,$(PORTNAME)))
TAP_TESTS = 1
endif

PS. But after rereading the Makefile I see that the problem here is to 
get $(PORTNAME). It is defined in Makefile.global, which is included 
below defining TAP_TESTS variable.


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

Re: "SELECT ... FROM DUAL" is not quite as silly as it appears

[Alexander Kuzmenkov]

On 11/29/18 22:13, Tom Lane wrote:

Ooops, I had not seen this before sending v4 patch.  Doesn't seem worth
posting a v5 for, but I'll be sure to fix it.



Thanks for updating, v4 looks good to me.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: libpq host/hostaddr/conninfo inconsistencies

[Dmitry Dolgov]

> On Fri, Oct 26, 2018 at 9:22 AM Fabien COELHO  wrote:
>
> To sum up:
>
> (1) you are somehow against changing the current implementation, eg
> erroring out on possibly misleading configurations, because you do not
> think it is really useful to help users in those cases.
>
> (2) you are not against improving the documentation, although you find it
> clear enough already, but you agree that some people could get confused.
>
> The attached patch v4 only improves the documentation so that it reflects
> what the implementation really does.

Thanks, it's definitely makes sense to propose documentation patch if there are
any concerns about how clear it is. For now I'm moving patch to the next CF.

> I think it too bad to leave out the user-friendly aspects of the patch,
> though.

Why then not split the original proposal into two patches, one to improve the
documentation, and another to make it more user friendly?

Re: GiST VACUUM

[Dmitry Dolgov]

> On Sun, Oct 28, 2018 at 6:32 PM Andrey Borodin  wrote:
>
> Hi everyone!
>
> > 2 окт. 2018 г., в 6:14, Michael Paquier  написал(а):
> > Andrey, your latest patch does not apply.  I am moving this to the next
> > CF, waiting for your input.
>
> I'm doing preps for CF.
> Here's rebased version.

Looks like this patch is waiting for an input since August. Could any of the
reviewers (Heikki?) please take a look at the latest version? In the meantime
I'm moving it to the next CF.

Re: New GUC to sample log queries

[Alvaro Herrera]

On 2018-Nov-30, Sergei Kornilov wrote:

> Hello
> 
> I can not build current HEAD cleanly. I got warning:
> 
> > postgres.c: In function ‘check_log_duration’:
> > postgres.c:2254:17: warning: ‘in_sample’ may be used uninitialized in this 
> > function [-Wmaybe-uninitialized]
> >   if ((exceeded && in_sample) || log_duration)

Ah, I feared that some compiler would not be smart enough to be silent
about that.  I hope it does not emit a warning with this patch?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 9a948f825d..5ab7d3cd8d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2244,12 +2244,13 @@ check_log_duration(char *msec_str, bool was_logged)
 		/*
 		 * Do not log if log_statement_sample_rate = 0. Log a sample if
 		 * log_statement_sample_rate <= 1 and avoid unecessary random() call
-		 * if log_statement_sample_rate = 1.
+		 * if log_statement_sample_rate = 1.  But don't compute any of this
+		 * unless needed.
 		 */
-		if (exceeded)
-			in_sample = log_statement_sample_rate != 0 &&
-(log_statement_sample_rate == 1 ||
- random() <= log_statement_sample_rate * MAX_RANDOM_VALUE);
+		in_sample = exceeded &&
+			log_statement_sample_rate != 0 &&
+			(log_statement_sample_rate == 1 ||
+			 random() <= log_statement_sample_rate * MAX_RANDOM_VALUE);
 
 		if ((exceeded && in_sample) || log_duration)
 		{

Re: New GUC to sample log queries

[Sergei Kornilov]

Hi

> Ah, I feared that some compiler would not be smart enough to be silent
> about that. I hope it does not emit a warning with this patch?
Yes, with this change build is clean. Thank you

PS: currently i use old gcc (Debian 4.9.2-10+deb8u1) 4.9.2

regards, Sergei

Re: [PATCH] Incremental sort (was: PoC: Partial sort)

[Dmitry Dolgov]

> On Mon, Mar 20, 2017 at 10:34 AM Alexander Korotkov 
>  wrote:
>
> Please, find rebased patch in the attachment.

It's been a while since this patch was posted. There is already some amount of
feedback in this thread, and the patch itself unfortunately has some conflicts
with the current master. Alexander, do you have any plans about this feature?
For now probably I'll mark it as "Returned with feedback".

Re: Connection slots reserved for replication

[Alexander Kukushkin]

Hi,

attaching the new version of the patch.
Now it simply reserves max_wal_senders slots in the ProcGlobal, what
guarantees that only walsender process could use them.

Regards,
--
Alexander Kukushkin
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a33a131182..9a23699fbc 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -895,11 +895,11 @@ PostmasterMain(int argc, char *argv[])
 	/*
 	 * Check for invalid combinations of GUC settings.
 	 */
-	if (ReservedBackends + max_wal_senders >= MaxConnections)
+	if (ReservedBackends >= MaxConnections)
 	{
-		write_stderr("%s: superuser_reserved_connections (%d) plus max_wal_senders (%d) must be less than max_connections (%d)\n",
+		write_stderr("%s: superuser_reserved_connections (%d) must be less than max_connections (%d)\n",
 	 progname,
-	 ReservedBackends, max_wal_senders, MaxConnections);
+	 ReservedBackends, MaxConnections);
 		ExitPostmaster(1);
 	}
 	if (XLogArchiveMode > ARCHIVE_MODE_OFF && wal_level == WAL_LEVEL_MINIMAL)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 33387fb71b..dbf307221c 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -43,6 +43,7 @@
 #include "postmaster/autovacuum.h"
 #include "replication/slot.h"
 #include "replication/syncrep.h"
+#include "replication/walsender.h"
 #include "storage/condition_variable.h"
 #include "storage/standby.h"
 #include "storage/ipc.h"
@@ -180,6 +181,7 @@ InitProcGlobal(void)
 	ProcGlobal->freeProcs = NULL;
 	ProcGlobal->autovacFreeProcs = NULL;
 	ProcGlobal->bgworkerFreeProcs = NULL;
+	ProcGlobal->walsenderFreeProcs = NULL;
 	ProcGlobal->startupProc = NULL;
 	ProcGlobal->startupProcPid = 0;
 	ProcGlobal->startupBufferPinWaitBufId = -1;
@@ -253,13 +255,20 @@ InitProcGlobal(void)
 			ProcGlobal->autovacFreeProcs = &procs[i];
 			procs[i].procgloballist = &ProcGlobal->autovacFreeProcs;
 		}
-		else if (i < MaxBackends)
+		else if (i < MaxBackends - max_wal_senders)
 		{
 			/* PGPROC for bgworker, add to bgworkerFreeProcs list */
 			procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
 			ProcGlobal->bgworkerFreeProcs = &procs[i];
 			procs[i].procgloballist = &ProcGlobal->bgworkerFreeProcs;
 		}
+		else if (i < MaxBackends)
+		{
+			/* PGPROC for walsender, add to walsenderFreeProcs list */
+			procs[i].links.next = (SHM_QUEUE *) ProcGlobal->walsenderFreeProcs;
+			ProcGlobal->walsenderFreeProcs = &procs[i];
+			procs[i].procgloballist = &ProcGlobal->walsenderFreeProcs;
+		}
 
 		/* Initialize myProcLocks[] shared memory queues. */
 		for (j = 0; j < NUM_LOCK_PARTITIONS; j++)
@@ -311,6 +320,8 @@ InitProcess(void)
 		procgloballist = &ProcGlobal->autovacFreeProcs;
 	else if (IsBackgroundWorker)
 		procgloballist = &ProcGlobal->bgworkerFreeProcs;
+	else if (am_walsender)
+		procgloballist = &ProcGlobal->walsenderFreeProcs;
 	else
 		procgloballist = &ProcGlobal->freeProcs;
 
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index b636b1e262..44e27866df 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -527,7 +527,7 @@ InitializeMaxBackends(void)
 
 	/* the extra unit accounts for the autovacuum launcher */
 	MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
-		max_worker_processes;
+		max_worker_processes + max_wal_senders;
 
 	/* internal error because the values were all checked previously */
 	if (MaxBackends > MAX_BACKENDS)
@@ -811,12 +811,9 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	}
 
 	/*
-	 * The last few connection slots are reserved for superusers.  Although
-	 * replication connections currently require superuser privileges, we
-	 * don't allow them to consume the reserved slots, which are intended for
-	 * interactive use.
+	 * The last few connection slots are reserved for superusers.
 	 */
-	if ((!am_superuser || am_walsender) &&
+	if ((!am_superuser && !am_walsender) &&
 		ReservedBackends > 0 &&
 		!HaveNFreeProcs(ReservedBackends))
 		ereport(FATAL,
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 03594e77fe..9d763b49dc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2054,7 +2054,7 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		/* see max_connections and max_wal_senders */
+		/* see max_connections */
 		{"superuser_reserved_connections", PGC_POSTMASTER, CONN_AUTH_SETTINGS,
 			gettext_noop("Sets the number of connection slots reserved for superusers."),
 			NULL
@@ -2572,7 +2572,6 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		/* see max_connections and superuser_reserved_connections */
 		{"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
 			gettext_noop("Sets the maximum number of simultaneously running WAL sender processes."),
 			NULL
diff --git a/src/include/storage/proc.h b/src/include/st

Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative

[Dmitry Dolgov]

> On Sun, Oct 28, 2018 at 11:42 PM Daniel Gustafsson  wrote:
>
> > On 26 Sep 2018, at 23:19, Daniel Gustafsson  wrote:
>
> > I’ve rebased these changes on top of your v9 patch as the attached v10.
>
> Attached is a v11 rebased on top of todays HEAD, which had minor conflicts due
> to the recent snprintf work. No functional changes are introduced over v10.

Thank you,

Unfortunately, patch needs to be fixed - it doesn't pass "make -C ssl check"

t/001_ssltests.pl .. 1/65 Bailout called.  Further testing stopped:
system pg_ctl failed
FAILED--Further testing stopped: system pg_ctl failed

Could you post an updated version?

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Thu, Nov 29, 2018 at 2:17 AM Tomas Vondra 
wrote:

(c) allow VOLATILE functions in the FILTER clause, but change the
> behavior to make the behavior sane
>

 Did changing the behavior means getting new snapshot before evaluating a
tuple to ensure the function sees results of any previously executed
queries or there are   other mechanism that can make the behavior sane?

Which leaves us with (b) and (c). Clearly, (b) is simpler to implement,
> because it (c) needs to do the detection too, and then some additional
> stuff. I'm not sure how much more complex (c) is, compared to (b).
>
> The attache patch implement option b prohibit VOLATILE functions but i am
open to change
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 411941ed31..15b6ddebed 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ FILTER ( condition ) ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -353,6 +354,30 @@ COPY { table_name [ ( 

 
+   
+FILTER
+
+   
+The optional FILTER clause has the general form
+
+FILTER condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, FILTER expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4aa8890fe8..b17d84e3fc 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -40,7 +40,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -150,6 +154,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *filterClause;	/* FILTER condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -180,6 +185,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -801,6 +807,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*filterClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -854,6 +861,30 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->filterClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, true);
+
+			/* Transform the raw expression tree */
+			filterClause = transformExpr(pstate, stmt->filterClause, EXPR_KIND_COPY_FILTER);
+
+			if (contain_volatile_functions(filterClause))
+ereport(ERROR,
+		(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+		 errmsg("function in FILTER clause should not be volatile")));
+
+			/*  Make sure it yields a boolean result. */
+			filterClause = coerce_to_boolean(pstate, filterClause, "FILTER");
+			/* we have to fix its collations too */
+			assign_expr_collations(pstate, filterClause);
+
+			filterClause = eval_const_expressions(NULL, filterClause);
+
+			filterClause = (Node *) canonicalize_qual((Expr *) filterClause, false);
+			filterClause = (Node *) make_ands_implicit((Expr *) filterClause);
+		}
+
 		tupDesc = RelationGetDescr(rel);
 		attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
 		foreach(cur, attnums)
@@ -1002,6 +1033,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
+		cstate->filterClause = filterClause;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
@@ -2531,6 +2563,10 @@ CopyFrom(CopyState cstate)
 	if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		proute = ExecSetupPartitionTupleRouting(NULL, cstate->rel);
 
+	if (cstate->filterClause)
+		cstate->qualexpr = ExecInitQual(ca

Re: New GUC to sample log queries

[Alvaro Herrera]

On 2018-Nov-30, Sergei Kornilov wrote:

> > Ah, I feared that some compiler would not be smart enough to be silent
> > about that. I hope it does not emit a warning with this patch?
> Yes, with this change build is clean. Thank you

Thanks, pushed.

> PS: currently i use old gcc (Debian 4.9.2-10+deb8u1) 4.9.2

Only five years old ... not that bad.

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

Re: pg_config wrongly marked as not parallel safe?

[Joe Conway]

On 11/30/18 3:30 AM, Kyotaro HORIGUCHI wrote:
> # And returning to the topic, I vote for pg_config should be "stable".

And on that note, Does this change does warrant backpatching, or should
be applied to master only?

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 11/30/18 2:00 PM, Surafel Temesgen wrote:



On Thu, Nov 29, 2018 at 2:17 AM Tomas Vondra 
mailto:tomas.von...@2ndquadrant.com>> wrote:


(c) allow VOLATILE functions in the FILTER clause, but change the
behavior to make the behavior sane

  Did changing the behavior means getting new snapshot before evaluating 
a tuple to ensure the function sees results of any previously executed 
queries or there are   other mechanism that can make the behavior sane?




I think it should be enough just to switch to CIM_SINGLE and increment 
the command counter after each inserted row.



Which leaves us with (b) and (c). Clearly, (b) is simpler to implement,
because it (c) needs to do the detection too, and then some additional
stuff. I'm not sure how much more complex (c) is, compared to (b).

The attache patch implement option b prohibit VOLATILE functions but i 
am open to change


OK. I'll take a look.


regards

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

Re: [HACKERS] Can ICU be used for a database's default sort order?

[Dmitry Dolgov]

> On Tue, Oct 30, 2018 at 9:07 AM Andrey Borodin  wrote:
>
> Hi!
>
> > 2 окт. 2018 г., в 11:37, Michael Paquier  написал(а):
> >
> > Please note that the latest patch set does not apply, so this has been
> > switched to commit fest 2018-11, waiting on author for a rebase.
>
> PFA rebased version. I've added LDFLAGS_INTERNAL += $(ICU_LIBS) in libpq,

Thanks for providing the rebased version.

> but I'm not entirely sure this is correct way to deal with complaints on ICU
> functions from libpq linking.

Well, it was enough on my own Gentoo, where patch actually compiles without
errors and pass "make check". But for cfbot it doesn't compile, I'm not sure
why.

../../../src/interfaces/libpq/libpq.so: undefined reference to `ucol_open_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to
`uloc_toLanguageTag_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to `u_errorName_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to
`ucol_getVersion_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to
`u_versionToString_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to
`uloc_setDefault_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to
`uloc_getDefault_52'
../../../src/interfaces/libpq/libpq.so: undefined reference to `ucol_close_52'

As a side note, I'm a bit confused, who is the original author of the proposed
patch? If it's Marina, why she isn't involved in the discussion or even
mentioned in the patch itself?

Re: pgsql: Avoid duplicate XIDs at recovery when building initial snapshot

[Dmitry Dolgov]

> On Thu, Nov 1, 2018 at 7:09 AM Michael Paquier  wrote:
>
> On Tue, Oct 23, 2018 at 10:43:38AM +0900, Michael Paquier wrote:
> > Well, following the same kind of thoughts, txid_current_snapshot() uses
> > sort_snapshot() to remove all the duplicates after fetching its data
> > from GetSnapshotData(), so wouldn't we want to do something about
> > removal of duplicates if dummy PGXACT entries are found while scanning
> > the ProcArray also in this case?  What I would think we should do is not
> > only to patch GetRunningTransactionData() but also GetSnapshotData() so
> > as we don't have duplicates also in this case, and do things in such a
> > way that both code paths use the same logic, and that we don't need to
> > have sort_snapshot() anymore.  That would be more costly though...
>
> My apologies it took a bit longer than I thought.  I got a patch on my
> desk for a couple of days, and finally took the time to finish something
> which would address the concerns raised here.  As long as we don't reach
> more than hundreds of thousands of entries, there is not going to be any
> performance impact.  So what I do in the attached is to revert 1df21ddb,
> and then have GetRunningTransactionData() sort the XIDs in the snapshot
> and remove duplicates only if at least one dummy proc entry is found
> while scanning, for xids and subxids.  This way, there is no need to
> impact most of the instance deployments with the extra sort/removal
> phase as most don't use two-phase transactions.  The sorting done at
> recovery when initializing the standby snapshot still needs to happen of
> course.
>
> The patch is added to the upcoming CF for review.

Unfortunately, patch has some conflict with the current master. Could you
please post a rebased version?

Re: [HACKERS] Custom compression methods

[Dmitry Dolgov]

> On Thu, Sep 6, 2018 at 5:27 PM Ildus Kurbangaliev 
>  wrote:
>
> Hi, attached latest set of patches. Rebased and fixed pg_upgrade errors
> related with zlib support.

Thank you for working on this patch, I believe the ideas mentioned in this
thread are quite important for Postgres improvement. Unfortunately, patch has
some conflicts now, could you post a rebased version one more time?

> On Mon, 23 Jul 2018 16:16:19 +0300
> Alexander Korotkov  wrote:
>
> > I'm going to review this patch.  Could you please rebase it?  It
> > doesn't apply for me due to changes made in src/bin/psql/describe.c.

Is there any review underway, could you share the results?

Re: pgbench doc fix

[Dmitry Dolgov]

> On Sat, Nov 3, 2018 at 1:08 AM Tatsuo Ishii  wrote:
>
> > So I do not think a more precise wording harms. Maybe: "prepared: use
> > extended query protocol with REUSED named prepared statements" would
> > be even less slightly ambiguous.
>
> I like this. But maybe we can remove "named"?

I also think it makes sense to adjust wording a bit here, and this version
sounds good (taking into account the commentary about "named"). I'm moving this
to the next CF, where the question would be if anyone from commiters can agree
with this point.

Re: Markdown format output for psql, design notes

[Lætitia Avrot]

Hi,

No I meant independently of the screen, if there's an LF character
> in a cell. Or a '|' character, since that's the same problem: an
> element of structure happening to be in the contents.
> The specs mentioned upthread don't seem to give any indication
> about that being supported.
>
> i've given a list of characters that needs escaping as stated in the
Markdown Extra doc and `|` is certainly one of this.

For LF caracter, I'm totally ok with the fact that it will break the
markdown output and my answer to that is "KISS". I don't want to handle
that case. Markdown Extra obviously decided that there was no such thing as
a multiline row.


> Say we have:
>  SELECT E'foo\nbar' as "Header1", 'foo|bar' as "Header2"
>
> If the markdown output was produced for the sole purpose of being
> converted to HTML in the end, which is often the case, it would work
> to use HTML entities in the output
>
> I don't use Markdown to create a HTML output. I use it to generate pdf for
my customers.

But as Vik said earlier, maybe it's not worth it to provide a markdown
output as pandoc can generate the markdown from the HTML output.
And if you need the markdown output to generate HTML why don't you use the
HTML output ?

Cheers,

Lætitia
-- 
*Think! Do you really need to print this email ? *
*There is no Planet B.*

Re: pgsql: Avoid duplicate XIDs at recovery when building initial snapshot

[Simon Riggs]

On Thu, 1 Nov 2018 at 06:09, Michael Paquier  wrote:

> On Tue, Oct 23, 2018 at 10:43:38AM +0900, Michael Paquier wrote:
> > Well, following the same kind of thoughts, txid_current_snapshot() uses
> > sort_snapshot() to remove all the duplicates after fetching its data
> > from GetSnapshotData(), so wouldn't we want to do something about
> > removal of duplicates if dummy PGXACT entries are found while scanning
> > the ProcArray also in this case?  What I would think we should do is not
> > only to patch GetRunningTransactionData() but also GetSnapshotData() so
> > as we don't have duplicates also in this case, and do things in such a
> > way that both code paths use the same logic, and that we don't need to
> > have sort_snapshot() anymore.  That would be more costly though...
>
> My apologies it took a bit longer than I thought.  I got a patch on my
> desk for a couple of days, and finally took the time to finish something
> which would address the concerns raised here.  As long as we don't reach
> more than hundreds of thousands of entries, there is not going to be any
> performance impact.  So what I do in the attached is to revert 1df21ddb,
> and then have GetRunningTransactionData() sort the XIDs in the snapshot
> and remove duplicates only if at least one dummy proc entry is found
> while scanning, for xids and subxids.  This way, there is no need to
> impact most of the instance deployments with the extra sort/removal
> phase as most don't use two-phase transactions.  The sorting done at
> recovery when initializing the standby snapshot still needs to happen of
> course.
>
> The patch is added to the upcoming CF for review.
>

1df21ddb looks OK to me and was simple enough to backpatch safely.

Seems excessive to say that the WAL record is corrupt, it just contains
duplicates, just as exported snapshots do. There's a few other imprecise
things around in WAL, that is why we need the RunningXact data in the first
place. So we have a choice of whether to remove the duplicates eagerly or
lazily.

For GetRunningTransactionData(), we can do eager or lazy, since its not a
foreground process. I don't object to changing it to be eager in this path,
but this patch is more complex than 1df21ddb and I don't think we should
backpatch this change, assuming it is acceptable.

This patch doesn't do it, but the suggestion that we touch
GetSnapshotData() in the same way so we de-duplicate eagerly is a different
matter and would need careful performance testing to ensure we don't slow
down 2PC users.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit)

[Dmitry Dolgov]

>On Sun, Nov 4, 2018 at 1:27 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Sun, 1 Apr 2018 at 19:58, Yura Sokolov  wrote:
> >
> > I didn't change serialized format. Therefore is no need to change
> > SerializeSnapshot.
> > But in-memory representation were changed, so RestoreSnapshot is changed.
>
> This patch went through the last tree commit fests without any noticeable
> activity, but cfbot says it still applies and doesn't break any tests. Taking
> into account potential performance improvements, I believe it would be a pity
> to stop at this point.
>
> Yura, what're your plans about it? If I understand correctly, there are still
> some commentaries, that were not answered from the last few messages. At the
> same time can anyone from active reviewers (Tomas, Amit) look at it to agree 
> on
> what should be done to push it forward?

Due to lack of response I'm marking it as "Returned with feedback". Feel free
to resubmit a new version though.

Re: Undo worker and transaction rollback

[Dmitry Dolgov]

> On Mon, Nov 5, 2018 at 1:32 PM Dilip Kumar  wrote:
>
> Updated patch, include defect fix from Kuntal posted on [1].
>
> [1] 
> https://www.postgresql.org/message-id/CAGz5QCKpCG6egFAdazC%2BJgyk7YSE1OBN9h-QpwCkg-NnSWN5AQ%40mail.gmail.com

Thanks for working on this feature. Unfortunately, looks like the current
version of patch has some conflicts with the current master, could you please
post an updated version again?

Re: pg_config wrongly marked as not parallel safe?

[Stephen Frost]

Greetings,

* Joe Conway (m...@joeconway.com) wrote:
> On 11/30/18 3:30 AM, Kyotaro HORIGUCHI wrote:
> > # And returning to the topic, I vote for pg_config should be "stable".
> 
> And on that note, Does this change does warrant backpatching, or should
> be applied to master only?

Given that it's a catalog change, I would think just master..

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Flexible permissions for REFRESH MATERIALIZED VIEW

[Dmitry Dolgov]

On Mon, Nov 5, 2018 at 4:19 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> This patch went through the last two commit fests without any noticeable
> activity. As far as I can see, judging from the discussion, there isn't a
> single opinion everyone would agree with, except that simply introducing a new
> permission is probably not enough and we need to address how to do this
> in an extendable way.
>
> > On Sun, 18 Mar 2018 at 22:05, Isaac Morland  wrote:
> >
> > Right now I'm really looking for whether anybody observes any problems with
> > the basic idea. If it's considered to be at least in principle a good idea
> > then I'll go and make a more complete patch.
>
> There were at least two options suggested about how to address the questions
> from the discussion (widening AclMode and introducing another type similar to
> aclitem, but more flexible, to store an "extended" permissions). Maybe the
> constructive approach here would be to try them out and propose a draft
> implementation for one of them?

Due to lack of response I'm marking it as "Returned with feedback". Feel free
to resubmit a new version though.

Re: pg_config wrongly marked as not parallel safe?

[Tom Lane]

Joe Conway  writes:
> On 11/30/18 3:30 AM, Kyotaro HORIGUCHI wrote:
>> # And returning to the topic, I vote for pg_config should be "stable".

> And on that note, Does this change does warrant backpatching, or should
> be applied to master only?

I don't think back-patching the catalog change is really a good idea.
The amount of work involved (e.g. release-noting how to perform the
update on existing databases) is way out of proportion to the benefit
for this particular case.

regards, tom lane

Re: Hash Joins vs. Bloom Filters / take 2

[Dmitry Dolgov]

On Thu, Nov 1, 2018 at 10:17 PM Tomas Vondra
 wrote:
>
> I haven't really planned to work on this anytime soon, unfortunately,
> which is why I proposed to mark it as RwF at the end of the last CF. I
> already have a couple other patches there, and (more importantly) I
> don't have a very clear idea how to implement the filter pushdown.
>
> That being said I still think it'd be an interesting improvement, and if
> someone wants to take over I'm available as a reviewer / tester ...

Since no one expressed any particular desire to take over the patch, I'm
marking it as "Returned with feedback". Although I really like the discussion
that happened here, maybe it worth to move a part of it to the wiki under the
"possible improvements" flag?

Re: [HACKERS] Two pass CheckDeadlock in contentent case

[Dmitry Dolgov]

> On Tue, Nov 6, 2018 at 5:19 AM Masahiko Sawada  wrote:
>
> The idea of this patch seems reasonable.
>
> I've looked at this patch. This patch still can be applied cleanly to
> the current HEAD and passed regression tests.
>
> Here is review comments.

Thanks for the review. Just for the records, patch still has no conflicts and
pass all the tests. Yura, do you have any plans about this patch, could you
respond to the feedback? In the meantime I'm moving it to the next CF.

Re: [PATCH] Improve tab completion for CREATE TABLE

[Dagfinn Ilmari Mannsåker]

ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

> Hi hackers,
>
> Please find attached a patch that adds the following tab completions for
> CREATE TABLE:

Added to the 2019-01 commitfest: https://commitfest.postgresql.org/21/1895/

- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

[Dmitry Dolgov]

> On Wed, Nov 7, 2018 at 10:58 AM Alexey Kondratov  
> wrote:
>
> On 30.10.2018 06:01, Michael Paquier wrote:
>
> > On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote:
> >> Currently in the patch, with dry-run option (-n) pg_rewind only fetches
> >> missing WALs to be able to build file map, while doesn't touch any data
> >> files. So I guess it behaves exactly as you described and we do not need a
> >> separate tool.
> > Makes sense perhaps.  Fetching only WAL segments which are needed for
> > the file map is critical, as you don't want to spend bandwidth for
> > nothing.  Now, I look at your patch, and I can see things to complain
> > about, at least three at short glance:
> > - The TAP test added will fail on Windows.
>
> Thank you for this. Build on Windows has been broken as well. I fixed it
> in the new version of patch, please find attached.

Just to confirm, patch still can be applied without conflicts, and pass all the
tests. Also I like the original motivation for the feature, sounds pretty
useful. For now I'm moving it to the next CF.

> > - Simply copy-pasting RestoreArchivedWAL() from the backend code to
> > pg_rewind is not an acceptable option.  You don't care about %r either
> > in this case.
>
> According to the docs [1] %r is a valid alias and may be used in
> restore_command too, so if we take restore_command from recovery.conf it
> might be there. If we just drop it, then restore_command may stop
> working. Though I do not know real life examples of restore_command with
> %r, we should treat it in expected way (as backend does), of course if
> we want an option to take it from recovery.conf.
>
> > - Reusing the GUC parser is something I would avoid as well.  Not worth
> > the complexity.
>
> Yes, I don't like it either. I will try to make guc-file.l frontend safe.

Any success with that?

Re: POC: Cleaning up orphaned files using undo logs

[Dmitry Dolgov]

> On Thu, Nov 8, 2018 at 4:03 AM Thomas Munro  
> wrote:
>
> On Tue, Nov 6, 2018 at 12:42 AM Kuntal Ghosh  
> wrote:
> > On Thu, Nov 1, 2018 at 8:53 AM Thomas Munro
> >  wrote:
> > > It passes make check on Unix and Windows, though currently it's
> > > failing some of the TAP tests for reasons I'm looking into (possibly
> > > due to bugs in the lower level patches, not sure).
> > >
> > I looked into the regression failures when the tap-tests are enabled.
> > It seems that we're not estimating and allocating the shared memory
> > for rollback-hash tables correctly. I've added a patch to fix the
> > same.
>
> Thanks Kuntal.

Thanks for the patch,

Unfortunately, cfbot complains about these patches and can't apply them for
some reason, so I did this manually to check it out. All of them (including the
fix from Kuntal) were applied without conflicts, but compilation stopped here

undoinsert.c: In function ‘UndoRecordAllocateMulti’:
undoinsert.c:547:18: error: ‘urec’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
   urec->uur_info = 0;  /* force recomputation of info bits */
   ~~~^~~

Could you please post a fixed version of the patch?

Re: pg_dump multi VALUES INSERT

[Dmitry Dolgov]

> On Thu, Nov 8, 2018 at 2:03 PM Surafel Temesgen  wrote:
>
> yes its not much line of code. Attach is a patch that optionally accept the 
> number of row in a single insert statement and if it is not specified one row 
> per statement used

Hi,

Unfortunately, patch needs to be rebased, could you please post an updated
version?

Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

[Dmitry Dolgov]

> On Sat, Nov 10, 2018 at 8:37 PM Andres Freund  wrote:
>
> On 2018-11-10 20:18:33 +0100, Dmitry Dolgov wrote:
> > > On Mon, 2 Jul 2018 at 15:54, Jesper Pedersen  
> > > wrote:
> > >
> > > The patch from November 27, 2017 still applies (with hunks),
> > >
> > >   https://commitfest.postgresql.org/18/1166/
> > >
> > > passes "make check-world" and shows performance improvements.
> > >
> > > Keeping it in "Ready for Committer".
> >
> > Looks like for some reason this patch is failing to attract committers, any
> > idea why? One of the plausible explanations for me is that the patch 
> > requires
> > some more intensive benchmarking of different workloads and types of lock
> > contention to make everyone more confident about it.
>
> Personally it's twofold:
>
> 1) It changes a lot of things, more than I think are strictly
>necessary to achieve the goal.
>
> 2) While clearly the retry logic is not necessary anymore (it was
>introduced when wait-queue was protected by a separate spinlock, which
>could not atomically manipulated together with the lock's state),
>there's reasons why it would be advantageous to keep:  My original
>patch for changing lwlocks to atomics, used lock xadd / fetch_add
>to acquire shared locks (just incrementing the #shared bits after an
>unlocked check) - obviously that can cause superfluous failures for
>concurrent lock releases. Having the retry logic around can make
>that safe.
>
>Using lock xadd to acquire shared locks turns out to be considerably
>more efficient - most locks where the lock state is contended (rather
>than just having processes wait), tend to have a very large fraction
>of shared lockers. And being able to do such a lock acquisition on a
>conteded cacheline with just a single locked operation, commonly
>without retries, is quite beneficial.

Due to lack of response and taking into account this commentary, I'm marking
this patch as "Returned with feedback", but hopefully I can pick it up later to
improve.

Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only" GUC pseudo-variable.

[Dmitry Dolgov]

> On Mon, Nov 12, 2018 at 8:30 PM Elvis Pranskevichus  wrote:
>
> On Monday, November 12, 2018 1:08:37 PM EST Tom Lane wrote:
> > Looking through the thread, it seems like there's a pretty fundamental
> > design issue that hasn't been resolved, namely whether and how this
> > ought to interact with default_transaction_read_only.  The patch as
> > it stands seems to be trying to implement the definition that the
> > transmittable variable session_read_only is equal to
> > "!(DefaultXactReadOnly || RecoveryInProgress())".  I doubt that
> > that's a good idea.  In the first place, it's not at all clear that
> > such a flag is sufficient for all use-cases.  In the second, it's
> > somewhere between difficult and impossible to correctly implement
> > GUCs that are interdependent in that way; the current patch certainly
> > fails to do so.  (It will not correctly track intra-session changes
> > to DefaultXactReadOnly, for instance.)
> >
> > I think that we'd be better off maintaining a strict separation
> > between "in hot standby" and default_transaction_read_only.  If there
> > are use cases in which people would like to reject connections based
> > on either one being true, let's handle that by marking them both
> > GUC_REPORT, and having libpq check both.  (So there need to be two
> > flavors of target-session-attrs libpq parameter, depending on whether
> > you want "in hot standby" or "currently read only" to be checked.)
>
> I agree that the original design with the separate "in_hot_standby" GUC
> is better.  Hot standby and read-only session are distinct modes, not
> all commands that are OK in a read-only session will succeed on a hot-
> standby backend.
>
> > I'm also not terribly happy with the patch adding a whole new
> > cross-backend signaling mechanism for this; surely we don't really
> > need that.  Perhaps it'd be sufficient to transmit SIGHUP and embed
> > the check for "just exited hot standby" in the handling for that?
>
> That seems doable.  Is there an existing check that is a good candidate
> for "just exited hot standby"?

Apparently due the minor conflict, mentioned above, the patch was in "Waiting
on author" state, which probably is not exactly correct. I'm moving it to the
next CF, but still, it would be nice if you post an updated version without
conflicts.

Re: test_pg_dump missing cleanup actions

[Stephen Frost]

Michael,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Sep 04, 2018 at 04:14:15PM -0700, Michael Paquier wrote:
> > On Tue, Sep 04, 2018 at 06:02:51PM -0400, Stephen Frost wrote:
> >> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >>> I'm confused.  Isn't the point of that script exactly to create a modified
> >>> extension for testing pg_dump with?
> > 
> > Not really, the regression tests run for pg_regress and the TAP test
> > suite are two completely isolated things and share no dependencies.
> > e54f757 has actually changed test_pg_dump.sql so as it adds regression
> > tests for pg_init_privs for ALTER EXTENSION ADD/DROP, and visibly those
> > have been added to test_pg_dump because they were easier to add there,
> > and this has no interactions with pg_dump.  What I think should have
> > been done initially is to add those new tests in test_extensions
> > instead.
> 
> I am able to come back to this thread, and I still don't grep from where
> sql/test_pg_dump.sql is called.  Stephen, if the test suite is aiming at
> tracking that pg_init_privs is correctly set up with ALTER EXTENSION
> ADD/DROP, shouldn't it also query the catalog to make sure that the
> correct entries are added and removed after running the so-said command?
> At least that's one way I could see to perform the necessary sanity
> checks without running directly pg_dump.  Perhaps I am missing
> something?

What is perhaps not clear is that the pg_init_privs work was all
entirely, specifically, for pg_dump to then work with and leverage to
only export out the correct set of privileges.  Testing *just*
pg_init_privs without also seeing what pg_dump does with values that end
up in that table wouldn't really be a complete test that things are
actually working.

Now, that test_pg_dump script with all of the various ALTER EXTENSION
ADD things that it's doing with access methods and aggregates and such,
which were quite intentionally left behind, were for testing
*pg_upgrade* as well and that happens to also test pg_dump since
pg_upgrade mostly just wraps pg_dump.  Note that the pg_upgrade test
runs installcheck which is what's installing all of those various things
into the database to test pg_upgrade/pg_dump with.

> >>> What I'd do is leave the final state as-is and add a "drop role if exists"
> >>> at the start, similar to what some of the core regression tests do.
> >> 
> >> I've not followed this thread but based on Tom's response, I agree with
> >> his suggestion of what to do here.
> > 
> > Not as far as I can see..  Please note that using installcheck on the
> > main regression test suite does not leave around any extra roles.  I can
> > understand the role of having a DROP ROLE IF EXISTS though: if you get a
> > crash while testing, then the beginning of the tests are repeatable, so
> > independently of the root issue Tom's suggestion makes sense to me.
> 
> Attached is a patch with more comments about the intents of the test
> suite, and the separate issue pointed out by Tom fixed.  It seems to me
> that actually checking the contents of pg_init_privs would improve the
> reason why the test exists..  I would welcome input about this last
> point.

Now, all that said, the TAP tests for test_pg_dump also create the
extension and then perform further GRANTs and such there and then check
the results of doing a pg_dump.

Hopefully the above helps explain where these are used and why we do
*not* want to have all of those DROP commands at the end to 'clean up'-
we'd be removing a bunch of important testing that we're getting thanks
to pg_upgrade.

In short, let's please not apply this patch.  To make it repeatable, we
could have DROP EXTENSION (et al) IF EXISTS at the top of the script,
similar to the DROP ROLE IF EXISTS that was suggested.  I'm not sure if
we have all the necessary DROP .. IF EXISTS options though today.. :/

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Support custom socket directory in pg_upgrade

[Tom Lane]

I wrote:
> Alvaro Herrera  writes:
>> On 12/11/2018 20:00, Tom Lane wrote:
>>> Also, even if we had an arguably-better idea, I suspect that there would
>>> always be cases where it didn't work.

>> Surely they can just set TMPDIR if /tmp is not writable?  If TMPDIR is
>> set and not writable, bark at the user for it.

> (1) There was nothing about TMPDIR in Peter's proposal, nor would an
> implementation based on mkdtemp(3) automatically do that for us.
> (2) If you accept the proposition that we must provide a user knob of
> some sort, why shouldn't it be a command line switch rather than an
> environment variable?  The former are much easier to document and to
> discover.

So we seem to be at an impasse here.  By my count, three people have
expressed support for the patch's approach of adding a socket-directory
option, while two people seem to prefer the idea of putting pg_upgrade's
socket under /tmp (possibly with a way to override that).  That's not
enough of a consensus to proceed with either approach, really, but
we ought to do something because the problem is real.

Given that we have a patch for this approach, and no patch has been
offered for the /tmp approach, I'm kind of inclined to exercise
committer's discretion and proceed with this patch.  Will anybody
be seriously annoyed if I do?

regards, tom lane

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

[Dmitry Dolgov]

On Mon, Nov 12, 2018 at 8:36 PM Tom Lane  wrote:
>
> Daniel Gustafsson  writes:
> > I’ve split the patch into two logical parts, the signalling functionality 
> > and
> > the userfacing terminate/cancel part.  For extra clarity I’ve also included 
> > the
> > full v19 patch, in case you prefer that instead when seeing the two.

Just for the records, cfbot complains during the compilation:

option.c: In function ‘parseCommandLine’:
option.c:265:8: error: ignoring return value of ‘getcwd’, declared
with attribute warn_unused_result [-Werror=unused-result]
  getcwd(default_sockdir, MAXPGPATH);
^

Re: [HACKERS] WAL logging problem in 9.4.3?

[Dmitry Dolgov]

> On Wed, Nov 14, 2018 at 4:48 AM Kyotaro HORIGUCHI 
>  wrote:
>
> 0004 was shot by e9edc1ba0b. Rebased to the current HEAD.
> Successfully built and passeed all regression/recovery tests
> including additional recovery/t/016_wal_optimize.pl.

Thank you for working on this patch. Unfortunately, cfbot complains that
v4-0004-Fix-WAL-skipping-feature.patch could not be applied without conflicts.
Could you please post a rebased version one more time?

> On Fri, Jul 27, 2018 at 9:26 PM Andrew Dunstan 
>  wrote:
>
> On 07/18/2018 10:58 AM, Heikki Linnakangas wrote:
> > On 18/07/18 16:29, Robert Haas wrote:
> >> On Wed, Jul 18, 2018 at 9:06 AM, Michael Paquier
> >>  wrote:
>  What's wrong with the approach proposed in
>  http://postgr.es/m/55afc302.1060...@iki.fi ?
> >>>
> >>> For back-branches that's very invasive so that seems risky to me
> >>> particularly seeing the low number of complaints on the matter.
> >>
> >> Hmm. I think that if you disable the optimization, you're betting that
> >> people won't mind losing performance in this case in a maintenance
> >> release.  If you back-patch Heikki's approach, you're betting that the
> >> committed version doesn't have any bugs that are worse than the status
> >> quo.  Personally, I'd rather take the latter bet.  Maybe the patch
> >> isn't all there yet, but that seems like something we can work
> >> towards.  If we just give up and disable the optimization, we won't
> >> know how many people we ticked off or how badly until after we've done
> >> it.
> >
> > Yeah. I'm not happy about backpatching a big patch like what I
> > proposed, and Kyotaro developed further. But I think it's the least
> > bad option we have, the other options discussed seem even worse.
> >
> > One way to review the patch is to look at what it changes, when
> > wal_level is *not* set to minimal, i.e. what risk or overhead does it
> > pose to users who are not affected by this bug? It seems pretty safe
> > to me.
> >
> > The other aspect is, how confident are we that this actually fixes the
> > bug, with least impact to users using wal_level='minimal'? I think
> > it's the best shot we have so far. All the other proposals either
> > don't fully fix the bug, or hurt performance in some legit cases.
> >
> > I'd suggest that we continue based on the patch that Kyotaro posted at
> > https://www.postgresql.org/message-id/20180330.100646.86008470.horiguchi.kyotaro%40lab.ntt.co.jp.
> >
> I have just spent some time reviewing Kyatoro's patch. I'm a bit
> nervous, too, given the size. But I'm also nervous about leaving things
> as they are. I suspect the reason we haven't heard more about this is
> that these days use of "wal_level = minimal" is relatively rare.

I'm totally out of context of this patch, but reading this makes me nervous
too. Taking into account that the problem now is lack of review, do you have
plans to spend more time reviewing this patch?

Re: Support custom socket directory in pg_upgrade

[Peter Eisentraut]

On 30/11/2018 17:58, Tom Lane wrote:
> So we seem to be at an impasse here.  By my count, three people have
> expressed support for the patch's approach of adding a socket-directory
> option, while two people seem to prefer the idea of putting pg_upgrade's
> socket under /tmp (possibly with a way to override that).  That's not
> enough of a consensus to proceed with either approach, really, but
> we ought to do something because the problem is real.
> 
> Given that we have a patch for this approach, and no patch has been
> offered for the /tmp approach, I'm kind of inclined to exercise
> committer's discretion and proceed with this patch.  Will anybody
> be seriously annoyed if I do?

I think it's fair to proceed and leave open that someone submits a
(possibly) better patch for a different approach in the future.

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

Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)

[Pavel Stehule]

pá 30. 11. 2018 v 9:26 odesílatel Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> napsal:

> Hello.
>
> At Fri, 30 Nov 2018 07:48:26 +0100, Pavel Stehule 
> wrote in <
> cafj8prd7zg07t4npzu09t4rgxz0btvyyg2emvoh+o_drnoi...@mail.gmail.com>
> > Hi
> >
> > čt 29. 11. 2018 v 14:44 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> > napsal:
> >
> > > > On Fri, Sep 21, 2018 at 1:30 PM Pavel Stehule <
> pavel.steh...@gmail.com>
> > > wrote:
> > > >
> > > > Thank you for comments
> > > >
> > > > Attached updated patch
> > >
> > > Unfortunately, current version of the patch doesn't pass make check,
> > > something
> > > is missing for xml tests. Could you please rebase it?
> > >
> > > After that I hope someone from reviewers (Kyotaro?) can probably
> confirm if
> > > it's still in a good shape. For now I'm moving it to the next CF.
>
> Sure. Sorry for coming late. I reconfirmed this.
>
> The most significant change in this version is namespace name
> generaton.
>
> - We can remove strlen from mutate_name by mutating the string in
>   reverse order. We don't need to mutate it in a human-affinity
>   order. The name would be 1-letter in almost all cases.
>
>   Concretely, the order in my mind is the follows:
>
>   "" "a" "b" ..."z" "aa" "ba" "ca"... "za" "ab"..
>
>
done


> - Might the 'propriety' correctly be 'properties'?
>
>  + /* register namespace if all propriety are available */
>
>
fixed


> - Is the "if" a mistake of "in"?
>
>  + * collect ns names if ResTarget format for possible usage
>  + * in getUniqNames function.
>
>
yup, fixed


> - I suppose the following should be like "register default
>   namespace definition if any".
>
>  +/* get default namespace name when it is required */
>
>
fixed


>
> Maybe the followings are not new. (Note that I'm not a naitive speaker.)
>



>
> - I cannot read this. (I might be to blame..)
>
>   + * default namespace for XPath expressions. Because there are not any
> API
>   + * how to transform or access to parsed XPath expression we have to
> parse
>   + * XPath here.
>
> - This might need to explain "by what".
>
>  + * Those functionalities are implemented with a simple XPath parser/
>  + * preprocessor. This XPath parser transforms a XPath expression to
> another
>  + * XPath expression that can be used by libxml2 XPath evaluation. It
> doesn't
>  + * replace libxml2 XPath parser or libxml2 XPath expression evaluation.
>
> - "add" -> "adds", "def_namespace_name" seems to need to be
>   replaced with something else.
>
>  + * This transformation add def_namespace_name to any unqualified node
> name
>  + * or attribute name of xpath expression.
>

I tried to formulate it better, but I am sorry, my English is not good.

>
> (Sorry, I'll look further later.)
>

I am sending a updated patch

Regards

Pavel


> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>


default-namespaces-20181130-2.patch.gz
Description: application/gzip

Re: pgbench doc fix

[Peter Eisentraut]

On 30/11/2018 15:42, Dmitry Dolgov wrote:
>> On Sat, Nov 3, 2018 at 1:08 AM Tatsuo Ishii  wrote:
>>
>>> So I do not think a more precise wording harms. Maybe: "prepared: use
>>> extended query protocol with REUSED named prepared statements" would
>>> be even less slightly ambiguous.
>>
>> I like this. But maybe we can remove "named"?
> 
> I also think it makes sense to adjust wording a bit here, and this version
> sounds good (taking into account the commentary about "named"). I'm moving 
> this
> to the next CF, where the question would be if anyone from commiters can agree
> with this point.

I don't see a concrete proposed patch here after the discussion.

Reading the documentation again, we could go for much more detail here.
For example, what's the point of having -M simple vs -M extended?

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

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Peter Eisentraut]

On 28/11/2018 14:43, Alvaro Herrera wrote:
> On 2018-Nov-28, Amit Kapila wrote:
> 
>> The problem with this idea is that if someone specifies a particular
>> parameter using query and the query doesn't return any parameters,
>> then it can lead to inadvertent behavior.  For example, if user uses
>> something like pg_stat_statements_reset(,
>> , SELECT s.queryid FROM pg_stat_statements AS s WHERE
>> s.query = 'SELECT $1 AS "ONE"');  now, if the query doesn't return any
>> row, we will remove the stats for all queries that belong to
>> (userid,dbid).  It could be surprising for some users, that's why we
>> came up with option-4 where we keep the default value of parameters as
>> 0.
> 
> Right, I think option 4 is a clear improvement over option 1.  I can get
> behind that one.  Since not many people care to vote, I think this tips
> the scales enough to that side.

I think option 4 is the right choice, and with named parameters and
default values it will also be very convenient.

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

Re: ToDo: show size of partitioned table

[Pavel Stehule]

čt 29. 11. 2018 v 7:58 odesílatel Michael Paquier 
napsal:

> On Thu, Nov 22, 2018 at 03:47:11PM +0100, Pavel Stehule wrote:
> > I have not feel well when I see in one report numbers 40 and 16, I see
> much
> > more comfortable when I see 24 and 16, but for this I need a different
> > perspective
> >
> > What do you think about it?
>
> Maybe, my thought is that this would be a separate patch, and that we
> had better keep it simple for now because that's complicated enough :)
>

any time, the behave should be consistent.

I agree, so there can be another patch, that reports more about partitioned
table for \d+


> So I would rather have a wrapper on top of pg_partition_tree which is
> useful for the user with matching patterns, which shows roughly:
> - the size of the whole partition tree from the root.
> - its direct parent if any.  Potentially in the verbose output.  This
> depends on how much users have multi-level partitions.  My bet would be
> not that much, but more input from others is welcome.
> - perhaps its level in the hierarchy, if integrated most likely as part
> of the verbose output.
>
> Then with the set of commands, have a mapping close to how \d treats
> indexes and relations:
> - \dp shows information about partitioned tables or indexes, if no pattern
> is defined tables show up.  If a partitioned index pattern shows up,
> then index information is displayed.
>

ok

- \dpi and \dpt for respectively partitioned indexes and tables.
>
--
> Michael
>

Re: partitioned tables referenced by FKs

[Alvaro Herrera]

Here's a more credible version of this patch series.

0001 refactors some duplicative code that interprets a pg_constraint row
for a foreign key back into a Constraint node.  Only what is necessary
for current features is read.

0002 moves some code that I added in 3de241dba86f to
src/backend/catalog/pg_constraint.c so that it appears in tablecmds.c
instead.  After developing the current patch I realized that the latter
is its natural home.

0003 changes the shape of a loop in deleteObjectsInList, so that I can
add object type-specific functions to be called before deleting an
object.  This is needed by 0004 and makes no sense on its own; I would
probably push both together, but splitting it like this makes it very
obvious what it is I'm doing.

0004 adds the feature itself

In 0004 there's also a new function "index_get_partition" which allows
to simplify some code I added in 8b08f7d4820f.  I will probably split it
up and commit separately because it's an obvious code beautify.

0005 modifies psql to show the constraint in a different way, which
makes more sense overall (rather than show the "internal" constraint
that concerns the partition, show the top-level contraint that concerns
the ancestor table on which it is defined.  This includes naming the
table in which the constraint is defined).

There's one half of 0005 that I think should be applied to pg11, because
when partitions have foreign keys, the display looks a bit odd
currently.  The other half I would probably merge into 0004 instead of
committing separately.  Even if not backpatched, it makes sense to apply
ahead of the rest because it changes expected output for a regression
test.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 563447095c3884a00ecfd933027524e252cf35f7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Mon, 12 Nov 2018 16:05:16 -0300
Subject: [PATCH 1/5] Refactor DeconstructConstraintRow

---
 src/backend/catalog/pg_constraint.c | 200 
 src/backend/utils/adt/ri_triggers.c |  89 ++--
 src/backend/utils/cache/relcache.c  |  61 +--
 src/include/catalog/pg_constraint.h |   3 +
 4 files changed, 125 insertions(+), 228 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index a8194b02fa..f71d89ff17 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -446,14 +446,11 @@ static void
 clone_fk_constraints(Relation pg_constraint, Relation parentRel,
 	 Relation partRel, List *clone, List **cloned)
 {
-	TupleDesc	tupdesc;
 	AttrNumber *attmap;
 	List	   *partFKs;
 	List	   *subclone = NIL;
 	ListCell   *cell;
 
-	tupdesc = RelationGetDescr(pg_constraint);
-
 	/*
 	 * The constraint key may differ, if the columns in the partition are
 	 * different.  This map is used to convert them.
@@ -483,9 +480,6 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
 		int			nelem;
 		ListCell   *cell;
 		int			i;
-		ArrayType  *arr;
-		Datum		datum;
-		bool		isnull;
 
 		tuple = SearchSysCache1(CONSTROID, parentConstrOid);
 		if (!tuple)
@@ -502,93 +496,11 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
 
 		ObjectAddressSet(parentAddr, ConstraintRelationId, parentConstrOid);
 
-		datum = fastgetattr(tuple, Anum_pg_constraint_conkey,
-			tupdesc, &isnull);
-		if (isnull)
-			elog(ERROR, "null conkey");
-		arr = DatumGetArrayTypeP(datum);
-		nelem = ARR_DIMS(arr)[0];
-		if (ARR_NDIM(arr) != 1 ||
-			nelem < 1 ||
-			nelem > INDEX_MAX_KEYS ||
-			ARR_HASNULL(arr) ||
-			ARR_ELEMTYPE(arr) != INT2OID)
-			elog(ERROR, "conkey is not a 1-D smallint array");
-		memcpy(conkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber));
-
+		DeconstructConstraintRow(tuple, &nelem, conkey, confkey,
+ conpfeqop, conppeqop, conffeqop);
 		for (i = 0; i < nelem; i++)
 			mapped_conkey[i] = attmap[conkey[i] - 1];
 
-		datum = fastgetattr(tuple, Anum_pg_constraint_confkey,
-			tupdesc, &isnull);
-		if (isnull)
-			elog(ERROR, "null confkey");
-		arr = DatumGetArrayTypeP(datum);
-		nelem = ARR_DIMS(arr)[0];
-		if (ARR_NDIM(arr) != 1 ||
-			nelem < 1 ||
-			nelem > INDEX_MAX_KEYS ||
-			ARR_HASNULL(arr) ||
-			ARR_ELEMTYPE(arr) != INT2OID)
-			elog(ERROR, "confkey is not a 1-D smallint array");
-		memcpy(confkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber));
-
-		datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop,
-			tupdesc, &isnull);
-		if (isnull)
-			elog(ERROR, "null conpfeqop");
-		arr = DatumGetArrayTypeP(datum);
-		nelem = ARR_DIMS(arr)[0];
-		if (ARR_NDIM(arr) != 1 ||
-			nelem < 1 ||
-			nelem > INDEX_MAX_KEYS ||
-			ARR_HASNULL(arr) ||
-			ARR_ELEMTYPE(arr) != OIDOID)
-			elog(ERROR, "conpfeqop is not a 1-D OID array");
-		memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));
-
-		datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop,
-			tupdesc, &isnull);

Re: pgsql: Switch pg_verify_checksums back to a blacklist

[Andrew Dunstan]

On 11/30/18 12:35 AM, Michael Paquier wrote:

On Fri, Nov 30, 2018 at 01:36:43AM +, Michael Paquier wrote:

Switch pg_verify_checksums back to a blacklist

This basically reverts commit d55241af705667d4503638e3f77d3689fd6be31,
leaving around a portion of the regression tests still adapted with
empty relation files, and corrupted cases.  This is also proving to be
failing to check properly relation files located in a non-default
tablespace path.

So, jacana is not in love with this commit for the tests:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2018-11-30%2003%3A34%3A23

The error is here:
server started
# Postmaster PID for node "node_checksum" is 14820
error running SQL: 'psql::1: ERROR:  directory
"/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_verify_checksums/tmp_check/t_002_actions_node_checksum_data/ts_corrupt_dir"
does not exist'

What visibly happens is that we create a path for a tablespace which
will include a corrupted table.  However psql tries to run CREATE
TABLESPACE but gets back a complain about the path not existing.  The
thing is that mkdir has been used beforehand so the path should be
here.

I have tested this set of TAP tests on my Windows 10 VM with MSVC 2015
before commit and those tests passed.  Could it be that jacana is using
msys, so a trick similar to what is used in 014_unlogged_reinit.pl is
necessary?

Andrew, could you confirm if the patch attached works?  I would prefer
not doing a blind fix if possible..




All it actually needs is this additional line after the mkdir:


   $tablespace_dir = TestLib::real_dir($tablespace_dir);


Explanation: TAP tests on msys need to run with the DTK perl, which 
understands msys virtualized paths. Postgres, however, does not 
understand such paths, so before you use such a value in, say, CREATE 
TABLESPACE, you need to translate it into a path on the underlying file 
system.



cheers


andrew


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

Re: dsa_allocate() faliure

[Jakub Glapa]

Hi, just a small update.
I've configured the OS for taking crash dumps on Ubuntu 16.04 with the
following (maybe somebody will find it helpful):
I've added LimitCORE=infinity to /lib/systemd/system/postgresql@.service
under [Service] section
I've reloaded the service config with sudo systemctl daemon-reload
Changed the core pattern to: sudo echo
/var/lib/postgresql/core.%p.sig%s.%ts | tee -a /proc/sys/kernel/core_pattern
I had tested it with kill -ABRT pidofbackend and it behaved correctly. A
crash dump was written.

In the last days I've been monitoring no segfault occurred but the
das_allocation did.
I'm starting to doubt if the segfault I've found in dmesg was actually
related.

I've grepped the postgres log for dsa_allocated:
Why do the messages occur sometimes as FATAL and sometimes as ERROR?

2018-11-29 07:59:06 CET::@:[20584]: FATAL:  dsa_allocate could not find 7
free pages
2018-11-29 07:59:06 CET:127.0.0.1(40846):user@db:[19507]: ERROR:
dsa_allocate could not find 7 free pages
2018-11-30 09:04:13 CET::@:[27341]: FATAL:  dsa_allocate could not find 13
free pages
2018-11-30 09:04:13 CET:127.0.0.1(41782):user@db:[25417]: ERROR:
dsa_allocate could not find 13 free pages
2018-11-30 09:28:38 CET::@:[30215]: FATAL:  dsa_allocate could not find 4
free pages
2018-11-30 09:28:38 CET:127.0.0.1(45980):user@db:[29924]: ERROR:
dsa_allocate could not find 4 free pages
2018-11-30 16:37:16 CET::@:[14385]: FATAL:  dsa_allocate could not find 7
free pages
2018-11-30 16:37:16 CET::@:[14375]: FATAL:  dsa_allocate could not find 7
free pages
2018-11-30 16:37:16 CET:212.186.105.45(55004):user@db:[14386]: FATAL:
dsa_allocate could not find 7 free pages
2018-11-30 16:37:16 CET:212.186.105.45(54964):user@db:[14379]: ERROR:
dsa_allocate could not find 7 free pages
2018-11-30 16:37:16 CET:212.186.105.45(54916):user@db:[14370]: ERROR:
dsa_allocate could not find 7 free pages
2018-11-30 16:45:11 CET:212.186.105.45(55356):user@db:[14555]: FATAL:
dsa_allocate could not find 7 free pages
2018-11-30 16:49:13 CET::@:[15359]: FATAL:  dsa_allocate could not find 7
free pages
2018-11-30 16:49:13 CET::@:[15363]: FATAL:  dsa_allocate could not find 7
free pages
2018-11-30 16:49:13 CET:212.186.105.45(54964):user@db:[14379]: FATAL:
dsa_allocate could not find 7 free pages
2018-11-30 16:49:13 CET:212.186.105.45(54916):user@db:[14370]: ERROR:
dsa_allocate could not find 7 free pages
2018-11-30 16:49:13 CET:212.186.105.45(55842):user@db:[14815]: ERROR:
dsa_allocate could not find 7 free pages
2018-11-30 16:56:11 CET:212.186.105.45(57076):user@db:[15638]: FATAL:
dsa_allocate could not find 7 free pages


There's quite a bit errors from today but I was launching the problematic
query in parallel from 2-3 sessions.
Sometimes it was breaking sometimes not.
Couldn't find any pattern.
The workload on this db is not really constant, rather bursting.

--
regards,
Jakub Glapa


On Tue, Nov 27, 2018 at 9:03 AM Thomas Munro 
wrote:

> On Tue, Nov 27, 2018 at 4:00 PM Thomas Munro
>  wrote:
> > Hmm.  I will see if I can come up with a many-partition torture test
> > reproducer for this.
>
> No luck.  I suppose one theory that could link both failure modes
> would a buffer overrun, where in the non-shared case it trashes a
> pointer that is later dereferenced, and in the shared case it writes
> past the end of allocated 4KB pages and corrupts the intrusive btree
> that lives in spare pages to track available space.
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>

Re: Add function to release an allocated SQLDA

[Dmitry Dolgov]

> On Wed, Nov 14, 2018 at 6:01 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Wed, 13 Jun 2018 at 06:30, Kato, Sho  wrote:
> >
> > I add a function called ECPGfreeSQLDA() becasue there is no API for 
> > releasing the SQLDA stored the result set.
> >
> > On Mon, 18 Jun 2018 at 07:42, Thomas Munro  
> > wrote:
> >
> > I wonder how other ESQL/C implementations deal with this stuff (...)
> > To allow that type of usage, we would need two new functions: (...)
>
> Thank you for the patch. Unfortunately, judging from the cfbot.cputube.org it
> can't be applied anymore to the current master. Could you please rebase the
> code and address the reviewer feedback?

Due to lack of response I'm marking this as returned with feedback.

Re: pgbench doc fix

[Fabien COELHO]

So I do not think a more precise wording harms. Maybe: "prepared: use
extended query protocol with REUSED named prepared statements" would
be even less slightly ambiguous.


I like this. But maybe we can remove "named"?


I also think it makes sense to adjust wording a bit here, and this version
sounds good (taking into account the commentary about "named"). I'm moving this
to the next CF, where the question would be if anyone from commiters can agree
with this point.


I don't see a concrete proposed patch here after the discussion.

Reading the documentation again, we could go for much more detail here.
For example, what's the point of having -M simple vs -M extended?


They do not use the same libpq-level approach (PQsendQuery vs 
PQsendQueryParams), so they are not exercising the same type of client? 
Pgbench is also about testing libpq performance.


--
Fabien.

Re: Synchronous replay take III

[Dmitry Dolgov]

> On Thu, Nov 15, 2018 at 6:34 AM Masahiko Sawada  wrote:
> > On Thu, Mar 1, 2018 at 10:40 AM Thomas Munro 
> >  wrote:
> >
> > In previous threads[1][2][3] I called this feature proposal "causal
> > reads".  That was a terrible name, borrowed from MySQL.  While it is
> > probably a useful term of art, for one thing people kept reading it as
> > "casual"

Yeah, that was rather annoying that I couldn't get rid of this while playing
with the "take II" version :)

> To be clear what did you mean read-mostly workloads?
>
> I think there are two kind of reads on standbys: a read happend after
> writes and a directly read (e.g. reporting). The former usually
> requires the causal reads as you mentioned in order to read its own
> writes but the latter might be different: it often wants to read the
> latest data on the master at the time. IIUC even if we send a
> read-only query directly to a synchronous replay server we could get a
> stale result if the standby delayed for less than
> synchronous_replay_max_lag. So this synchronous replay feature would
> be helpful for the former case(i.e. a few writes and many reads wants
> to see them) whereas for the latter case perhaps the keeping the reads
> waiting on standby seems a reasonable solution.
>
> Also I think it's worth to consider the cost both causal reads *and*
> non-causal reads.
>
> I've considered a mixed workload (transactions requiring causal reads
> and transactions not requiring it) on the current design. IIUC the
> current design seems like that we create something like
> consistent-reads group by specifying servers. For example, if a
> transaction doesn't want to causality read it can send query any
> server with synchronous_replay = off but if it wants, it should select
> a synchronous replay server. It also means that client applications or
> routing middlewares such as pgpool is required to be aware of
> available synchronous replay standbys. That is, this design would cost
> the read-only transactions requiring causal reads. On the other hand,
> in token-based causal reads we can send read-only query any standbys
> if we can wait for the change to be replayed. Of course if we don't
> wait forever we can timeout and switch to either another standby or
> the master to execute query but we don't need to choose a server of
> standby servers.

Unfortunately, cfbot says that patch can't be applied without conflicts, could
you please post a rebased version and address commentaries from Masahiko?

Re: Range phrase operator in tsquery

[Dmitry Dolgov]

> On Thu, Nov 15, 2018 at 11:15 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Fri, 27 Apr 2018 at 13:03, Aleksandr Parfenov 
> >  wrote:
> >
> > Nowadays, phrase operator in Postgres FTS supports only exact match of
> > the distance between two words. It is sufficient for a search of
> > simple/exact phrases, but in some cases exact distance is unknown and we
> > want to words be close enough. E.g. it may help to search phrases with
> > additional words in the middle of the phrase
>
> Hi,
>
> Thank you for the patch, it looks like a nice feature. Few questions:
>
> + if (!distance_from_set)
> + {
> + distance_from = distance_to < 0 ? MINENTRYPOS : 0;
> + }
> + if (!distance_to_set)
> + {
> + distance_to = distance_from < 0 ? 0 : MAXENTRYPOS;
> + }
>
> Why use 0 here instead of MAXENTRYPOS/MINENTRYPOS ? It looks a bit strange:
>
> SELECT 'a <,-1000> b'::tsquery;
> tsquery
> 
>  'a' <-16384,-1000> 'b'
> (1 row)
>
> SELECT 'a <,1000> b'::tsquery;
>  tsquery
> --
>  'a' <0,1000> 'b'
> (1 row)
>
> Also I wonder why after introducing MINENTRYPOS the LIMITPOS wasn't changed?
>
> #define LIMITPOS(x) ( ( (x) >= MAXENTRYPOS ) ? (MAXENTRYPOS-1) : (x) )

Due to lack of response I'm marking this as returned with feedback.

Re: [PATCH] Log CSV by default

[Andres Freund]

Hi,

On 2018-11-30 19:53:18 +0100, David Fetter wrote:
> This makes it much simpler for computers to use the logs while not
> making it excessively difficult for humans to use them.

While perhaps not excessively so, I think it increases the difficulty
sufficiently that I'm against such a proposal.  Yes, a conversion into
something more readable can be done programatically, but that's another
step before analyzing a problem. And besides, a lot of log fragments are
embedded in email, screenshotted etc.

Secondarily, csvlog doesn't contain all the interesting information.

Thirdly, it often increases the log volume noticably.


I think having a bin/pg_logparse tool that can parse postgres' config
file and attempt to parse the log contents in whatever format they are
would be much much more useful. Obviously not every log_line_prefix can
be parsed unambiguously, but a lot of formats can, and a lot more
formats can be made unambiguous (e.g. adding escape logic to application
name logging would be very useful).

- Andres

Re: pg_dumpall --exclude-database option

[Dmitry Dolgov]

> On Sun, Nov 18, 2018 at 7:41 PM Andrew Dunstan 
>  wrote:
>
> On 11/17/18 9:55 AM, Alvaro Herrera wrote:
> > The comment in expand_dbname_patterns is ungrammatical and mentions
> > "OID" rather than "name", so I suggest
>
> Will fix.
>
> > Other than that, the patch seems fine to me -- I tested and it works as
> > intended.
> >
> > Personally I would say "See also expand_table_name_patterns" instead of
> > "This is similar to code in pg_dump.c for handling matching table names.",
> > as well as mention this function in expand_table_name_patterns' comment.
> > (No need to mention expand_schema_name_patterns, since these are
> > adjacent.)  But this is mostly stylistic and left to your own judgement.
> >
> > In the long run, I think we should add an option to processSQLNamePattern
> > to use OR instead of AND, which would fix both this problem as well as
> > pg_dump's.  I don't think that's important enough to stall this patch.
>
> Thanks for the review.

Unfortunately judging from cfbot output patch needs to be rebased, could you
please post an updated version with those fixes mentioned above?

Re: dsa_allocate() faliure

[Justin Pryzby]

On Fri, Nov 30, 2018 at 08:20:49PM +0100, Jakub Glapa wrote:
> In the last days I've been monitoring no segfault occurred but the
> das_allocation did.
> I'm starting to doubt if the segfault I've found in dmesg was actually
> related.

The dmesg looks like a real crash, not just OOM.  You can hopefully find the
timestamp of the segfaults in /var/log/syslog, and compare with postgres logs
if they go back far enough.  All the postgres processes except the parent
would've been restarted at that time.

> I've grepped the postgres log for dsa_allocated:
> Why do the messages occur sometimes as FATAL and sometimes as ERROR?

I believe it may depend if it happens in a parallel worker or the leader.

You may get more log detail if you enable CSV logging (although unfortunately
as I recall it doesn't indicate it's a parallel worker).

You could force it to dump core if you recompile postgres with an assert() (see
patch below).

You could build an .deb by running dpkg-buildpackage -rfakeroot or similar (i
haven't done this in awhile), or you could compile, install, and launch
debugging binaries from your homedir (or similar)

You'd want to compile the same version (git checkout REL_10_6) and with the
proper configure flags..perhaps starting with:
./configure --with-libxml --with-libxslt --enable-debug 
--prefix=$HOME/src/postgresql.bin --enable-cassert && time make && make install

Be careful if you have extensions installed that they still work.

Justin

--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -727,4 +727,7 @@ dsa_allocate_extended(dsa_area *area, size_t size, int 
flags)
if (!FreePageManagerGet(segment_map->fpm, npages, &first_page))
+   {
elog(FATAL,
 "dsa_allocate could not find %zu free pages", 
npages);
+   abort()
+   }

Re: make installcheck-world in a clean environment

[Dmitry Dolgov]

> On Sun, Nov 18, 2018 at 8:31 AM Alexander Lakhin  wrote:
>
> I've modified the patch to use the installed version of pg_regress. It
> simplifies a lot. The main idea of the change is to not build pg_regress.

Hi,

I've noticed that for this patch cfbot show strange error

USE_INSTALLED_ASSETS=1 make all
make[2]: Entering directory
`/home/travis/build/postgresql-cfbot/postgresql/src/test/regress'
make -C ../../../contrib/spi
make[3]: Entering directory
`/home/travis/build/postgresql-cfbot/postgresql/contrib/spi'
make[3]: Nothing to be done for `all'.
make[3]: Leaving directory
`/home/travis/build/postgresql-cfbot/postgresql/contrib/spi'
make[2]: Leaving directory
`/home/travis/build/postgresql-cfbot/postgresql/src/test/regress'
rm -rf ./testtablespace
mkdir ./testtablespace
'/usr/local/pgsql/lib/pgxs/src/test/regress/pg_regress' --inputdir=.
--bindir='/usr/local/pgsql/bin'
--bindir='/home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_upgrade/tmp_check/install//usr/local/pgsql/bin'
--port=54464 --dlpath=. --max-concurrent-tests=20
--bindir='/home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_upgrade/tmp_check/install//usr/local/pgsql/bin'
--port=54464 --schedule=./parallel_schedule
make[1]: /usr/local/pgsql/lib/pgxs/src/test/regress/pg_regress:
Command not found
make[1]: *** [installcheck-parallel] Error 127
make[1]: Leaving directory
`/home/travis/build/postgresql-cfbot/postgresql/src/test/regress'
make: *** [installcheck-parallel] Error 2
make: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql'

The same error I've got when tried to run it on my laptop, is it something
expected?

[PATCH] Fix Proposal - Deadlock Issue in Single User Mode When IO Failure Occurs

[Chengchao Yu]

Greetings,



Recently, we hit a few occurrences of deadlock when IO failure (including disk 
full, random remote disk IO failures) happens in single user mode. We found the 
issue exists on both Linux and Windows in multiple postgres versions.



Here are the steps to repro on Linux (as Windows repro is similar):


1.   Get latest PostgreSQL code, build and install the executables.



$ git clone https://git.postgresql.org/git/postgresql.git

$ cd postgresql

$ PGROOT=$(pwd)

$ git checkout REL_11_STABLE

$ mkdir build

$ cd build

$ ../configure --prefix=/path/to/postgres

$ make && make install


2.   Run initdb to initialize a PG database folder.



$ /path/to/postgres/bin/initdb -D /path/to/data


3.   Because the unable to write relation data scenario is difficult to hit 
naturally even reserved space is turned off, I have prepared a small patch (see 
attachment "emulate-error.patch") to force an error when PG tries to write data 
to relation files. We can just apply the patch and there is no need to put 
efforts flooding data to disk any more.



$ cd $PGROOT

$ git apply /path/to/emulate-error.patch

$ cd build

$ make && make install


4.   Connect to the newly initialized database cluster with single user 
mode, create a table, and insert some data to the table, do a checkpoint or 
directly give EOF. Then we hit the deadlock issue and the process will not exit 
until we kill it.



Do a checkpoint explicitly:



$ /path/to/postgres/bin/postgres --single -D /path/to/data/ postgres -c 
exit_on_error=true < create table t1(a int);

> insert into t1 values (1), (2), (3);

> checkpoint;

> EOF



PostgreSQL stand-alone backend 11.1

backend> backend> backend> 2018-11-29 02:45:27.891 UTC [18806] FATAL:  Emulate 
exception in mdwrite() when writing to disk

2018-11-29 02:55:27.891 UTC [18806] CONTEXT:  writing block 8 of relation 
base/12368/1247

2018-11-29 02:55:27.891 UTC [18806] STATEMENT:  checkpoint;



2018-11-29 02:55:27.900 UTC [18806] FATAL:  Emulate exception in mdwrite() when 
writing to disk

2018-11-29 02:55:27.900 UTC [18806] CONTEXT:  writing block 8 of relation 
base/12368/1247



Or directly give an EOF:



$ /path/to/postgres/bin/postgres --single -D /path/to/data/ postgres -c 
exit_on_error=true < create table t1(a int);

> insert into t1 values (1), (2), (3);

> EOF



PostgreSQL stand-alone backend 11.1

backend> backend> backend> 2018-11-29 02:55:24.438 UTC [18149] FATAL:  Emulate 
exception in mdwrite() when writing to disk

2018-11-29 02:45:24.438 UTC [18149] CONTEXT:  writing block 8 of relation 
base/12368/1247


5.   Moreover, when we try to recover the database with single user mode, 
we hit the issue again, and the process does not bring up the database nor exit.



$ /path/to/postgres/bin/postgres --single -D /path/to/data/ postgres -c 
exit_on_error=true

2018-11-29 02:59:33.257 UTC [19058] LOG:  database system shutdown was 
interrupted; last known up at 2018-11-29 02:58:49 UTC

2018-11-29 02:59:33.485 UTC [19058] LOG:  database system was not properly shut 
down; automatic recovery in progress

2018-11-29 02:59:33.500 UTC [19058] LOG:  redo starts at 0/1672E40

2018-11-29 02:59:33.500 UTC [19058] LOG:  invalid record length at 0/1684B90: 
wanted 24, got 0

2018-11-29 02:59:33.500 UTC [19058] LOG:  redo done at 0/1684B68

2018-11-29 02:59:33.500 UTC [19058] LOG:  last completed transaction was at log 
time 2018-11-29 02:58:49.856663+00

2018-11-29 02:59:33.547 UTC [19058] FATAL:  Emulate exception in mdwrite() when 
writing to disk

2018-11-29 02:59:33.547 UTC [19058] CONTEXT:  writing block 8 of relation 
base/12368/1247



Analyses:



So, what happened? Actually, there are 2 types of the deadlock due to the same 
root cause. Let's first take a look at the scenario in step #5. In this 
scenario, the deadlock happens when disk IO failure occurs inside 
StartupXLOG(). If we attach debugger to PG process, we will see the process is 
stuck acquiring the buffer's lw-lock in AbortBufferIO().



void

AbortBufferIO(void)

{

BufferDesc *buf = InProgressBuf;



if (buf)

{

uint32  buf_state;



/*

 * Since LWLockReleaseAll has already been called, we're not holding

 * the buffer's io_in_progress_lock. We have to re-acquire it so that

 * we can use TerminateBufferIO. Anyone who's executing WaitIO on the

 * buffer will be in a busy spin until we succeed in doing this.

 */

LWLockAcquire(BufferDescriptorGetIOLock(buf), LW_EXCLUSIVE);



This is because the same lock has been acquired before buffer manager attempts 
to flush the buffer page, which happens in StartBufferIO().



static bool

StartBufferIO(BufferDesc *buf, bool forInput)

{

uint32  buf_state;



Assert(!InProgressBuf);



for (;;)

{

/*

 * Grab the io_in_progress lock so that other processes can wait for

 * me to finish the I/O.

 */

LWLockAcquire(

Re: [PATCH] Log CSV by default

[Isaac Morland]

> I think having a bin/pg_logparse tool that can parse postgres' config
> file and attempt to parse the log contents in whatever format they are
> would be much much more useful. Obviously not every log_line_prefix can
> be parsed unambiguously, but a lot of formats can, and a lot more
> formats can be made unambiguous (e.g. adding escape logic to application
> name logging would be very useful).
>

Aren't application names set by the client? If they are currently logged
without escaping, couldn't a client insert arbitrary binary content such as
terminal control sequences into log files, with potentially unpleasant
results? Or is there some sort of filter already in place somewhere? Should
I investigate?

Re: pg_dumpall --exclude-database option

[Andrew Dunstan]

On 11/18/18 1:41 PM, Andrew Dunstan wrote:


On 11/17/18 9:55 AM, Alvaro Herrera wrote:

The comment in expand_dbname_patterns is ungrammatical and mentions
"OID" rather than "name", so I suggest

/*
 * The loop below might sometimes result in duplicate entries in the
 * output name list, but we don't care.
 */



Will fix.




I'm not sure this is grammatical either:
    exclude databases whose name matches PATTERN
I would have written it like this:
    exclude databases whose names match PATTERN
but I'm not sure (each database has only one name, of course, but aren't
you talking about multiple databases there?)




I think the original is grammatical.




Other than that, the patch seems fine to me -- I tested and it works as
intended.

Personally I would say "See also expand_table_name_patterns" instead of
"This is similar to code in pg_dump.c for handling matching table 
names.",

as well as mention this function in expand_table_name_patterns' comment.
(No need to mention expand_schema_name_patterns, since these are
adjacent.)  But this is mostly stylistic and left to your own judgement.

In the long run, I think we should add an option to 
processSQLNamePattern

to use OR instead of AND, which would fix both this problem as well as
pg_dump's.  I don't think that's important enough to stall this patch.



Thanks for the review.






Rebased and updated patch attached.


cheers


andrew


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

diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index c51a130f43..973b9955bf 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -300,6 +300,27 @@ PostgreSQL documentation
   
  
 
+
+ 
+  --exclude-database=pattern
+  
+   
+Do not dump databases whose name matches
+pattern.
+Multiple patterns can be excluded by writing multiple
+--exclude-database switches.  The
+pattern parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d
+commands (see ),
+so multiple databases can also be excluded by writing wildcard
+characters in the pattern.  When using wildcards, be careful to
+quote the pattern if needed to prevent shell wildcard expansion.
+   
+  
+ 
+
  
   --if-exists
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d583154fba..3aa0f5f1fa 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1276,7 +1276,8 @@ expand_schema_name_patterns(Archive *fout,
 
 /*
  * Find the OIDs of all tables matching the given list of patterns,
- * and append them to the given OID list.
+ * and append them to the given OID list. See also expand_dbname_patterns()
+ * in pg_dumpall.c
  */
 static void
 expand_table_name_patterns(Archive *fout,
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 5176626476..f57ff2fe3f 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -52,6 +52,8 @@ static PGconn *connectDatabase(const char *dbname, const char *connstr, const ch
 static char *constructConnStr(const char **keywords, const char **values);
 static PGresult *executeQuery(PGconn *conn, const char *query);
 static void executeCommand(PGconn *conn, const char *query);
+static void expand_dbname_patterns(PGconn *conn, SimpleStringList *patterns,
+   SimpleStringList *names);
 
 static char pg_dump_bin[MAXPGPATH];
 static const char *progname;
@@ -87,6 +89,9 @@ static char role_catalog[10];
 static FILE *OPF;
 static char *filename = NULL;
 
+static SimpleStringList database_exclude_patterns = {NULL, NULL};
+static SimpleStringList database_exclude_names = {NULL, NULL};
+
 #define exit_nicely(code) exit(code)
 
 int
@@ -123,6 +128,7 @@ main(int argc, char *argv[])
 		{"column-inserts", no_argument, &column_inserts, 1},
 		{"disable-dollar-quoting", no_argument, &disable_dollar_quoting, 1},
 		{"disable-triggers", no_argument, &disable_triggers, 1},
+		{"exclude-database", required_argument, NULL, 5},
 		{"if-exists", no_argument, &if_exists, 1},
 		{"inserts", no_argument, &inserts, 1},
 		{"lock-wait-timeout", required_argument, NULL, 2},
@@ -318,6 +324,10 @@ main(int argc, char *argv[])
 appendPQExpBufferStr(pgdumpopts, " --no-sync");
 break;
 
+			case 5:
+simple_string_list_append(&database_exclude_patterns, optarg);
+break;
+
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit_nicely(1);
@@ -334,6 +344,16 @@ main(int argc, char *argv[])
 		exit_nicely(1);
 	}
 
+	if (database_exclude_patterns.head != NULL &&
+		(globals_only || roles_only || tablespaces_only))
+	{
+		fprintf(stderr, _("%s: option --exclude-database cannot be used together with -g/--globals-only, -r/--roles-only or -t/--tabl

Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

[Юрий Соколов]

пт, 30 нояб. 2018 г., 19:21 Dmitry Dolgov 9erthali...@gmail.com:

> > On Sat, Nov 10, 2018 at 8:37 PM Andres Freund 
> wrote:
> >
> > On 2018-11-10 20:18:33 +0100, Dmitry Dolgov wrote:
> > > > On Mon, 2 Jul 2018 at 15:54, Jesper Pedersen <
> jesper.peder...@redhat.com> wrote:
> > > >
> > > > The patch from November 27, 2017 still applies (with hunks),
> > > >
> > > >   https://commitfest.postgresql.org/18/1166/
> > > >
> > > > passes "make check-world" and shows performance improvements.
> > > >
> > > > Keeping it in "Ready for Committer".
> > >
> > > Looks like for some reason this patch is failing to attract
> committers, any
> > > idea why? One of the plausible explanations for me is that the patch
> requires
> > > some more intensive benchmarking of different workloads and types of
> lock
> > > contention to make everyone more confident about it.
> >
> > Personally it's twofold:
> >
> > 1) It changes a lot of things, more than I think are strictly
> >necessary to achieve the goal.
> >
> > 2) While clearly the retry logic is not necessary anymore (it was
> >introduced when wait-queue was protected by a separate spinlock, which
> >could not atomically manipulated together with the lock's state),
> >there's reasons why it would be advantageous to keep:  My original
> >patch for changing lwlocks to atomics, used lock xadd / fetch_add
> >to acquire shared locks (just incrementing the #shared bits after an
> >unlocked check) - obviously that can cause superfluous failures for
> >concurrent lock releases. Having the retry logic around can make
> >that safe.
> >
> >Using lock xadd to acquire shared locks turns out to be considerably
> >more efficient - most locks where the lock state is contended (rather
> >than just having processes wait), tend to have a very large fraction
> >of shared lockers. And being able to do such a lock acquisition on a
> >conteded cacheline with just a single locked operation, commonly
> >without retries, is quite beneficial.
>
> Due to lack of response and taking into account this commentary, I'm
> marking
> this patch as "Returned with feedback", but hopefully I can pick it up
> later to
> improve.
>

Good luck.

>

Re: doc - improve description of default privileges

[Tom Lane]

Fabien COELHO  writes:
> Attached v4:
>   - moves the table to the privileges section
>   - updates the table column headers
>   - adds a privilege/aclitem letter mapping table
>   - adds some appropriate links towards psql & aclitem

TBH, I don't think this goes nearly far enough.  It seems like it
is making the fragmentation of aclitem information worse not better.
I feel if we're going to do anything, we should put a unified description
of privileges and aclitem-reading into section 5.6, and take that material
out of the various places where it lives now.  Like the attached, in which
I failed to resist the temptation to wordsmith some stuff as well as move
it around.

regards, tom lane

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index c134bca..18c38e4 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
*** SCRAM-SHA-256$

Re: [PATCH] Log CSV by default

[Tom Lane]

Andres Freund  writes:
> On 2018-11-30 19:53:18 +0100, David Fetter wrote:
>> This makes it much simpler for computers to use the logs while not
>> making it excessively difficult for humans to use them.

> While perhaps not excessively so, I think it increases the difficulty
> sufficiently that I'm against such a proposal.

I don't like this either.  People who prefer CSV format can select it
trivially.  Those who don't are going to be annoyed at us for changing
behavior that's stood for many years.

Also, in addition to the objections you noted, there's the problem that
this change requires changing logging_collector to default to "on".
That's an *enormous* compatibility break, because of the effects on
where the log output goes.  Not to mention having performance impacts
that can be significant.

I think we should reject this out of hand.

> I think having a bin/pg_logparse tool that can parse postgres' config
> file and attempt to parse the log contents in whatever format they are
> would be much much more useful. Obviously not every log_line_prefix can
> be parsed unambiguously, but a lot of formats can, and a lot more
> formats can be made unambiguous (e.g. adding escape logic to application
> name logging would be very useful).

Yeah, it might be possible to make some progress in those directions.

regards, tom lane

Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

[Dmitry Dolgov]

> On Fri, Nov 30, 2018 at 10:31 PM Юрий Соколов  wrote:
>
>> Due to lack of response and taking into account this commentary, I'm marking
>> this patch as "Returned with feedback", but hopefully I can pick it up later 
>> to
>> improve.
>
> Good luck.

Unexpected for me, but promising reaction - regardless of the meaning of it
maybe there are chances that you'll be here to help with the future versions,
thanks.

Re: doc - improve description of default privileges

[Alvaro Herrera]

On 2018-Nov-30, Tom Lane wrote:

> I feel if we're going to do anything, we should put a unified description
> of privileges and aclitem-reading into section 5.6, and take that material
> out of the various places where it lives now.  Like the attached, in which
> I failed to resist the temptation to wordsmith some stuff as well as move
> it around.

I looked at the psql manpage and the HTML rendering of section 5.6 and
it all looks good to me.

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

Re: NOTIFY and pg_notify performance when deduplicating notifications

[Dmitry Dolgov]

> On Mon, Nov 19, 2018 at 8:30 AM Julien Demoor  wrote:
>
> Thank you for the review. I've addressed all your points in the attached
> patch. The patch was made against release 11.1.

I've noticed, that cfbot complains about this patch [1], since:

Duplicate OIDs detected:
3423
found 1 duplicate OID(s) in catalog data

At the same time it's quite minor problem, so I'm moving it to the nexc CF as
"Needs review".

Also since it's performance related patch, and the latest tests I see in the
history of this topic were posted around the time of the initial thread (which
was quite some time ago), could we expect to see some new benchmarks with this
patch and without? Probably the positive difference would be obvious, but
still.

[1]: https://travis-ci.org/postgresql-cfbot/postgresql/builds/461617098

Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

[Dmitry Dolgov]

> On Mon, Nov 19, 2018 at 2:30 PM Nikolay Shaplov  wrote:
>
> В письме от 2 октября 2018 13:46:13 пользователь Michael Paquier написал:
> > On Fri, Sep 14, 2018 at 09:30:25PM +0300, Nikolay Shaplov wrote:
> > > BTW this commit shows why do this patch is important: 857f9c36 adds new
> > > option for b-tree indexes. But thanks to the StdRdOptions this option
> > > will exist for no practical use in all heaps that has just any option set
> > > to non-default value, and in indexes that use StdRdOptions (and also has
> > > any option set) And there will be more. StdRdOptions is long outdated
> > > solution and it needs to be replaced.
> >
> > This patch does not apply anymore, so this is moved to next CF, waiting
> > for author.
> Oup...It seems to me that I've fogot to rebase it when it is needed...
> Did it now

Looks like there are some problems with this patch on windows:

src/backend/access/common/reloptions.c(1469): error C2059: syntax error : '}'

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.22359

> On Fri, Mar 2, 2018 at 8:28 PM Andres Freund  wrote:
>
> On 2018-03-02 20:22:21 +0300, Nikolay Shaplov wrote:
> > Since I get a really big patch as a result, it was decided to commit it in
> > parts.
>
> I get that, but I strongly suggest not creating 10 loosely related
> threads, but keeping it as a patch series in one thread. It's really
> hard to follow for people not continually paying otherwise.

Totally agree. Probably this also makes it harder to see the big picture behind
this patch - which is in turn probably preventing it from getting some more
review. I hope it doesn't sounds ridiculous, taking into account your efforts
by splitting the patch, but maybe it makes sense to gather these pieces
together (as a separate commits, of course) in one thread?

Re: pgsql: Switch pg_verify_checksums back to a blacklist

[Michael Paquier]

On Fri, Nov 30, 2018 at 02:18:18PM -0500, Andrew Dunstan wrote:
> All it actually needs is this additional line after the mkdir:
> 
>$tablespace_dir = TestLib::real_dir($tablespace_dir);
> 
> Explanation: TAP tests on msys need to run with the DTK perl, which
> understands msys virtualized paths. Postgres, however, does not understand
> such paths, so before you use such a value in, say, CREATE TABLESPACE, you
> need to translate it into a path on the underlying file system.

Thanks Andrew for confirming.  I have committed your suggestion, which
should calm down jacana.
--
Michael


signature.asc
Description: PGP signature

Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

[Dmitry Dolgov]

> On Mon, Nov 19, 2018 at 4:58 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Tue, Jul 24, 2018 at 5:52 PM Dave Cramer  wrote:
> >
> > Back in 2016 a patch was proposed that seems to have died on the vine. See 
> > https://www.postgresql.org/message-id/flat/cafgjrd3hdyoa33m69tbeofnner2bzbwa8ffjt2v5vfztbvu...@mail.gmail.com
> > for the history and https://commitfest.postgresql.org/10/621/ for the 
> > commitfest entry.
> > I've rebased the patches and attached them for consideration.
> > JDBC tests here 
> > https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/test/java/org/postgresql/replication/LogicalReplicationTest.java
> >  all pass
>
> Unfortunately the second patch from the series can't be applied anymore, could
> you please rebase it one more time? Other than that it looks strange for me
> that the corresponding discussion stopped when it was quite close to be in a
> good shape, bouncing from "rejected with feedback" to "needs review". Can
> someone from involved people clarify what's the current status of this patch?

Looks like I'm a failure as a neuromancer, since revival didn't happen. I'm
marking it as returned with feedback.

Re: Add extension options to control TAP and isolation tests

[Michael Paquier]

On Fri, Nov 30, 2018 at 02:18:04PM +0300, Arthur Zakirov wrote:
> As far as I understand bloom TAP tests are stable on some platforms, such as
> linux. Can be TAP test disabled only for some specific platforms?

That can be done with PORTNAME.  However I am doubting that those tests
are even reliable on Linux..  So it would be much better to understand
the root issue before enabling them.
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Avoid duplicate XIDs at recovery when building initial snapshot

[Michael Paquier]

On Fri, Nov 30, 2018 at 02:55:47PM +, Simon Riggs wrote:
> 1df21ddb looks OK to me and was simple enough to backpatch safely.

Thanks for the feedback!

> Seems excessive to say that the WAL record is corrupt, it just contains
> duplicates, just as exported snapshots do. There's a few other imprecise
> things around in WAL, that is why we need the RunningXact data in the first
> place. So we have a choice of whether to remove the duplicates eagerly or
> lazily.
> 
> For GetRunningTransactionData(), we can do eager or lazy, since its not a
> foreground process. I don't object to changing it to be eager in this path,
> but this patch is more complex than 1df21ddb and I don't think we should
> backpatch this change, assuming it is acceptable.

Yes, I would avoid a backpatch for this more complicated one, and
we need a solution for already-generated WAL.  It is not complicated to
handle duplicates for xacts and subxacts however holding ProcArrayLock
for a longer time stresses me as it is already a bottleneck.

> This patch doesn't do it, but the suggestion that we touch
> GetSnapshotData() in the same way so we de-duplicate eagerly is a different
> matter and would need careful performance testing to ensure we don't slow
> down 2PC users.

Definitely.  That's a much hotter code path.  I am not completely sure
if that's an effort worth pursuing either..
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Log CSV by default

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Andres Freund  writes:
> > On 2018-11-30 19:53:18 +0100, David Fetter wrote:
> >> This makes it much simpler for computers to use the logs while not
> >> making it excessively difficult for humans to use them.
> 
> > While perhaps not excessively so, I think it increases the difficulty
> > sufficiently that I'm against such a proposal.
> 
> I don't like this either.  People who prefer CSV format can select it
> trivially.  Those who don't are going to be annoyed at us for changing
> behavior that's stood for many years.

Agreed.  Frankly, even if we made this the default, I doubt packagers
would decide to go with it because of the issues raised here.

> I think we should reject this out of hand.

Agreed.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] proposal: schema variables

[Dmitry Dolgov]

> On Wed, Nov 21, 2018 at 8:25 AM Pavel Stehule  wrote:
>
> just rebase

Thanks for working on this patch.

I'm a bit confused, but cfbot again says that there are some conflicts.
Probably they are the minor one, from src/bin/psql/help.c

For now I'm moving it to the next CF.

Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

[Dave Cramer]

Why is this being closed? I did not see the first email looking for
clarification.

The history is the original author dropped off the planet (no idea where he
is)

I can certainly rebase it.

Dave Cramer


On Fri, 30 Nov 2018 at 18:00, Dmitry Dolgov <9erthali...@gmail.com> wrote:

> > On Mon, Nov 19, 2018 at 4:58 PM Dmitry Dolgov <9erthali...@gmail.com>
> wrote:
> >
> > > On Tue, Jul 24, 2018 at 5:52 PM Dave Cramer 
> wrote:
> > >
> > > Back in 2016 a patch was proposed that seems to have died on the vine.
> See
> https://www.postgresql.org/message-id/flat/cafgjrd3hdyoa33m69tbeofnner2bzbwa8ffjt2v5vfztbvu...@mail.gmail.com
> > > for the history and https://commitfest.postgresql.org/10/621/ for the
> commitfest entry.
> > > I've rebased the patches and attached them for consideration.
> > > JDBC tests here
> https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/test/java/org/postgresql/replication/LogicalReplicationTest.java
> all pass
> >
> > Unfortunately the second patch from the series can't be applied anymore,
> could
> > you please rebase it one more time? Other than that it looks strange for
> me
> > that the corresponding discussion stopped when it was quite close to be
> in a
> > good shape, bouncing from "rejected with feedback" to "needs review". Can
> > someone from involved people clarify what's the current status of this
> patch?
>
> Looks like I'm a failure as a neuromancer, since revival didn't happen. I'm
> marking it as returned with feedback.
>

Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

[Dmitry Dolgov]

On Sat, Dec 1, 2018 at 12:17 AM Dave Cramer  wrote:
>
> Why is this being closed? I did not see the first email looking for 
> clarification.

Well, mostly due total absence of response and broken mind reading crystal ball.

> I can certainly rebase it.

Yes, please do. I'll change the CF item status back.

Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

[Dave Cramer]

Dmitry,

Thanks, I have done a preliminary check and it seems pretty straightforward.

I will clean it up for Monday

Thanks for your patience!

Dave Cramer


On Fri, 30 Nov 2018 at 18:22, Dmitry Dolgov <9erthali...@gmail.com> wrote:

> On Sat, Dec 1, 2018 at 12:17 AM Dave Cramer  wrote:
> >
> > Why is this being closed? I did not see the first email looking for
> clarification.
>
> Well, mostly due total absence of response and broken mind reading crystal
> ball.
>
> > I can certainly rebase it.
>
> Yes, please do. I'll change the CF item status back.
>

Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

[Dmitry Dolgov]

>On Sat, Dec 1, 2018 at 12:49 AM Dave Cramer  wrote:
>
> Thanks, I have done a preliminary check and it seems pretty straightforward.
>
> I will clean it up for Monday

Great, thank you!

Frontends including fd.h

[Michael Paquier]

Hi all,

We have a couple of frontend binaries (pg_rewind, and now
pg_verify_checksums) including directly fd.h to get the definitions of
PG_TEMP_FILES_DIR and PG_TEMP_FILE_PREFIX.  This is a poor choice I
think, because fd.h is aimed at being used by the backend, and there 
are some routines declared there which could conflict with their
frontend equivalents, like the various wrappers for fsync.

I have suggested a couple of months ago to have a specific header for
path-related variables, which I called src/include/pg_paths.h, but not
many people liked that.  Perhaps it would be time to bite the bullet, I
think that we are on a path where problems are going to show up for
frontends.  At least PG_TEMP_FILES_DIR and PG_TEMP_FILE_PREFIX could be
moved to it to remove all the existing problems.

Thoughts or opinions?
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Log CSV by default

[Michael Paquier]

On Fri, Nov 30, 2018 at 06:11:03PM -0500, Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> I think we should reject this out of hand.
> 
> Agreed.

+1.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Log CSV by default

[David Fetter]

On Fri, Nov 30, 2018 at 04:55:30PM -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-11-30 19:53:18 +0100, David Fetter wrote:
> >> This makes it much simpler for computers to use the logs while not
> >> making it excessively difficult for humans to use them.
> 
> > While perhaps not excessively so, I think it increases the difficulty
> > sufficiently that I'm against such a proposal.
> 
> I don't like this either.  People who prefer CSV format can select it
> trivially.  Those who don't are going to be annoyed at us for changing
> behavior that's stood for many years.
> 
> Also, in addition to the objections you noted, there's the problem that
> this change requires changing logging_collector to default to "on".
> That's an *enormous* compatibility break, because of the effects on
> where the log output goes.  Not to mention having performance impacts
> that can be significant.
> 
> I think we should reject this out of hand.

It's been far too long since I got one of these!

> > I think having a bin/pg_logparse tool that can parse postgres' config
> > file and attempt to parse the log contents in whatever format they are
> > would be much much more useful. Obviously not every log_line_prefix can
> > be parsed unambiguously, but a lot of formats can, and a lot more
> > formats can be made unambiguous (e.g. adding escape logic to application
> > name logging would be very useful).
> 
> Yeah, it might be possible to make some progress in those directions.

So application names need better handling, and possibly reviews for
security considerations, and pg_logparse ?

OK.

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: Synchronous replay take III

[Thomas Munro]

On Sat, Dec 1, 2018 at 9:06 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:
> Unfortunately, cfbot says that patch can't be applied without conflicts, could
> you please post a rebased version and address commentaries from Masahiko?

Right, it conflicted with 4c703369 and cfdf4dc4.  While rebasing on
top of those, I found myself wondering why syncrep.c thinks it needs
special treatment for postmaster death.  I don't see any reason why we
shouldn't just use WL_EXIT_ON_PM_DEATH, so I've done it like that in
this new version.  If you kill -9 the postmaster, I don't see any
reason to think that the existing coding is more correct than simply
exiting immediately.

On Thu, Nov 15, 2018 at 6:34 PM Masahiko Sawada  wrote:
> On Thu, Mar 1, 2018 at 10:40 AM Thomas Munro
>  wrote:
> > I was pinged off-list by a fellow -hackers denizen interested in the
> > synchronous replay feature and wanting a rebased patch to test.  Here
> > it goes, just in time for a Commitfest.  Please skip to the bottom of
> > this message for testing notes.
>
> Thank you for working on this. The overview and your summary was
> helpful for me to understand this feature, thank you. I've started to
> review this patch for PostgreSQL 12. I've tested this patch and found
> some issue but let me ask you questions about the high-level design
> first. Sorry if these have been already discussed.

Thanks for your interest in this work!

> > This is a design choice favouring read-mostly workloads at the expense
> > of write transactions.  Hot standbys' whole raison for existing is to
> > move *some* read-only workloads off the primary server.  This proposal
> > is for users who are prepared to trade increased primary commit
> > latency for a guarantee about visibility on the standbys, so that
> > *all* read-only work could be moved to hot standbys.
>
> To be clear what did you mean read-mostly workloads?

I mean workloads where only a small percentage of transactions perform
a write.  If you need write-scalability, then hot_standby is not the
solution for you (with or without this patch).

The kind of user who would be interested in this feature is someone
who already uses some kind of heuristics to move some queries to
read-only standbys.  For example, some people send transaction for
logged-in users to the primary database (because only logged-in users
generate write queries), and all the rest to standby servers (for
example "public" users who can only read content).  Another technique
I have seen is to keep user sessions "pinned" on the primary server
for N minutes after they perform a write transaction.  These types of
load balancing policies are primitive ways of achieving
read-your-writes consistency, but they are conservative and
pessimistic: they probably send too many queries to the primary node.

This proposal is much more precise, allowing you to run the minimum
number of transactions on the primary node (ie transactions that
actually need to perform a write), and the maximum number of
transactions on the hot standbys.

As discussed, making reads wait for a token would be a useful
alternative (and I am willing to help make that work too), but:

1.  For users that do more many more reads than writes, would you
rather make (say) 80% of transactions slower or 20%?  (Or 99% vs 1% as
the case may be, depending on your application.)

2.  If you are also using synchronous_commit = on for increased
durability, then you are already making writers wait, and you might be
able to tolerate a small increase.

Peter Eisentraut expressed an interesting point of view against this
general line of thinking:

https://www.postgresql.org/message-id/5643933F.4010701%40gmx.net

My questions are:  Why do we have hot_standby mode?  Is load balancing
a style of usage we want to support?  Do we want a technology that
lets you do more of it?

> I think there are two kind of reads on standbys: a read happend after
> writes and a directly read (e.g. reporting). The former usually
> requires the causal reads as you mentioned in order to read its own
> writes but the latter might be different: it often wants to read the
> latest data on the master at the time. IIUC even if we send a
> read-only query directly to a synchronous replay server we could get a
> stale result if the standby delayed for less than
> synchronous_replay_max_lag. So this synchronous replay feature would
> be helpful for the former case(i.e. a few writes and many reads wants
> to see them) whereas for the latter case perhaps the keeping the reads
> waiting on standby seems a reasonable solution.

I agree 100% that this is not a solution for all users.  But I also
suspect a token system would be quite complicated, and can't be done
in a way that is transparent to applications without giving up
performance advantages.  I wrote about my understanding of the
trade-offs here:

https://www.postgresql.org/message-id/CAEepm%3D0W9GmX5uSJMRXkpNEdNpc09a_OMt18XFhf8527EuGGUQ%40mail.gmail.com

> Also I think it's wor

Re: Synchronous replay take III

[Michael Paquier]

On Sat, Dec 01, 2018 at 02:48:29PM +1300, Thomas Munro wrote:
> On Sat, Dec 1, 2018 at 9:06 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> Unfortunately, cfbot says that patch can't be applied without conflicts, 
>> could
>> you please post a rebased version and address commentaries from Masahiko?
> 
> Right, it conflicted with 4c703369 and cfdf4dc4.  While rebasing on
> top of those, I found myself wondering why syncrep.c thinks it needs
> special treatment for postmaster death.  I don't see any reason why we
> shouldn't just use WL_EXIT_ON_PM_DEATH, so I've done it like that in
> this new version.  If you kill -9 the postmaster, I don't see any
> reason to think that the existing coding is more correct than simply
> exiting immediately.

Hm.  This stuff runs under many assumptions, so I think that we should
be careful here with any changes as the very recent history has proved
(4c70336).  If we were to switch WAL senders on postmaster death, I
think that this could be a change independent of what is proposed here.
--
Michael


signature.asc
Description: PGP signature

Re: make installcheck-world in a clean environment

[Alexander Lakhin]

Hello,

30.11.2018 23:59, Dmitry Dolgov wrote:
>> On Sun, Nov 18, 2018 at 8:31 AM Alexander Lakhin  wrote:
>>
>> I've modified the patch to use the installed version of pg_regress. It
>> simplifies a lot. The main idea of the change is to not build pg_regress.
> Hi,
>
> I've noticed that for this patch cfbot show strange error
>
> USE_INSTALLED_ASSETS=1 make all
> ...
> make[1]: Leaving directory
> `/home/travis/build/postgresql-cfbot/postgresql/src/test/regress'
> make: *** [installcheck-parallel] Error 2
> make: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql'
>
> The same error I've got when tried to run it on my laptop, is it something
> expected?
Thank you for bringing this to my attention.
I've fixed the patch.
(pg_upgrade/test.sh passes custom DESTDIR to `make` but the same DESTDIR
is needed for `make install`)

Best regards,
Alexander
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index afcab930f7..fd0e0eedb0 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -46,7 +46,7 @@ regresscheck: | submake-regress submake-test_decoding temp-install
 	--temp-config $(top_srcdir)/contrib/test_decoding/logical.conf \
 	$(REGRESSCHECKS)
 
-regresscheck-install-force: | submake-regress submake-test_decoding temp-install
+regresscheck-install-force: | temp-install
 	$(pg_regress_installcheck) \
 	$(REGRESSCHECKS)
 
@@ -58,7 +58,7 @@ isolationcheck: | submake-isolation submake-test_decoding temp-install
 	--temp-config $(top_srcdir)/contrib/test_decoding/logical.conf \
 	$(ISOLATIONCHECKS)
 
-isolationcheck-install-force: all | submake-isolation submake-test_decoding temp-install
+isolationcheck-install-force: | submake-isolation temp-install
 	$(pg_isolation_regress_installcheck) \
 	$(ISOLATIONCHECKS)
 
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 673a8c2164..9444dee351 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -111,7 +111,12 @@ make installcheck-parallel
default port number, unless directed otherwise by PGHOST and
PGPORT environment variables.  The tests will be run in a
database named regression; any existing database by this name
-   will be dropped.
+   will be dropped.  The tests in the installcheck mode will use the executables,
+   libraries, and headers, which are already present in the installation tree,
+   not their copies in the source tree (if any).  This mode is suitable for
+   testing distribution packages of PostgreSQL installed on user systems as
+   it allows to check the installation in its entirety without rebuilding
+   any parts of it.
   
 
   
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 956fd274cd..3f5b779823 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -245,14 +245,16 @@ PG_SYSROOT = @PG_SYSROOT@
 
 override CPPFLAGS := $(ICU_CFLAGS) $(CPPFLAGS)
 
-ifdef PGXS
+# For PGXS and in the installcheck mode (when we use the installed assets)
+# we need to target already installed headers and libraries
+ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS))
 override CPPFLAGS := -I$(includedir_server) -I$(includedir_internal) $(CPPFLAGS)
-else # not PGXS
+else # not PGXS and not USE_INSTALLED_ASSETS
 override CPPFLAGS := -I$(top_srcdir)/src/include $(CPPFLAGS)
 ifdef VPATH
 override CPPFLAGS := -I$(top_builddir)/src/include $(CPPFLAGS)
 endif
-endif # not PGXS
+endif # not PGXS and USE_INSTALLED_ASSETS
 
 CC = @CC@
 GCC = @GCC@
@@ -306,7 +308,7 @@ with_gnu_ld = @with_gnu_ld@
 # "LDFLAGS := something" anywhere, ditto for LDFLAGS_INTERNAL.
 # These initial assignments must be "=" type, and elsewhere we must only do
 # "LDFLAGS += something" or "LDFLAGS_INTERNAL += something".
-ifdef PGXS
+ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS))
   LDFLAGS_INTERNAL = -L$(libdir)
 else
   LDFLAGS_INTERNAL = -L$(top_builddir)/src/port -L$(top_builddir)/src/common
@@ -379,6 +381,9 @@ endif
 # install tree just once in any recursive "make check".  The additional test
 # on abs_top_builddir prevents doing anything foolish to the root directory.
 
+# In the installcheck mode we should use what is already installed
+# in the DESTDIR (namely, pg_regress).
+
 check: temp-install
 
 .PHONY: temp-install
@@ -421,7 +426,7 @@ ifeq ($(enable_tap_tests),yes)
 define prove_installcheck
 rm -rf '$(CURDIR)'/tmp_check
 $(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(DESTDIR)$(pgxsdir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
 endef
 
 define prove_check
@@ -564,6

Re: [HACKERS] proposal: schema variables

[Pavel Stehule]

so 1. 12. 2018 v 0:16 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
napsal:

> > On Wed, Nov 21, 2018 at 8:25 AM Pavel Stehule 
> wrote:
> >
> > just rebase
>
> Thanks for working on this patch.
>
> I'm a bit confused, but cfbot again says that there are some conflicts.
> Probably they are the minor one, from src/bin/psql/help.c
>

rebased again

Regards

Pavel


> For now I'm moving it to the next CF.
>


schema-variables-20181201-01.patch.gz
Description: application/gzip

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On 11/29/18, Amit Kapila  wrote:
> On Thu, Nov 29, 2018 at 3:07 PM John Naylor  wrote:
>> Done. I tried adding it to several schedules, but for some reason
>> vacuuming an empty table failed to truncate the heap to 0 blocks.
>> Putting the test in its own group fixed the problem, but that doesn't
>> seem ideal.
>>
>
> It might be because it fails the should_attempt_truncation() check.
> See below code:
>
> if (should_attempt_truncation(vacrelstats))
> lazy_truncate_heap(onerel, vacrelstats, vac_strategy);

I see. I think truncating the FSM is not essential to show either the
old or new behavior -- I could skip that portion to enable running the
test in a parallel group.

>> Can you please repeat the copy test you have done above with
>> fillfactor as 20 and 30?
>
> I will send the results in a separate email soon.

I ran the attached scripts which populates 100 tables with either 4 or
8 blocks. The test tables were pre-filled with one tuple and vacuumed
so that the FSMs were already created when testing the master branch.
The patch branch was compiled with a threshold of 8, but testing
inserts of 4 pages effectively simulates a threshold of 4. Config was
stock, except for fsync = off. I took the average of 40 runs (2
complete tests of 20 runs each) after removing the 10% highest and
lowest:

fillfactor=20
# blocksmaster  patch
4   19.1ms  17.5ms
8   33.4ms  30.9ms

fillfactor=30
# blocksmaster  patch
4   20.1ms  19.7ms
8   34.7ms  34.9ms

It seems the patch might be a bit faster with fillfactor=20, but I'm
at a loss as to why that would be. Previous testing with a higher
threshold showed a significant performance penalty starting around 10
blocks [1], but that used truncation rather than deletion, and had a
fill-factor of 10.

--
[1] 
https://www.postgresql.org/message-id/CAJVSVGWCRMyi8sSqguf6PfFcpM3hwNY5YhPZTt-8Q3ZGv0UGYw%40mail.gmail.com

-John Naylor


fsm-copy-setup.sql
Description: application/sql


fsm-copy-test-v3.sql
Description: application/sql

Re: pgsql: Switch pg_verify_checksums back to a blacklist

[Michael Paquier]

On Sat, Dec 01, 2018 at 08:00:16AM +0900, Michael Paquier wrote:
> Thanks Andrew for confirming.  I have committed your suggestion, which
> should calm down jacana.

And it is now green:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2018-12-01%2004%3A02%3A53
--
Michael


signature.asc
Description: PGP signature

Re: Undo logs

[Amit Kapila]

On Thu, Nov 29, 2018 at 6:27 PM Dilip Kumar  wrote:
>
> On Mon, Nov 26, 2018 at 2:13 PM Amit Kapila  wrote:
> >
> > 10.
> > + if (!UndoRecPtrIsValid(multi_prep_urp))
> > + urecptr = UndoRecordAllocateMulti(urec, 1, upersistence, txid);
> > + else
> > + urecptr = multi_prep_urp;
> > +
> > + size = UndoRecordExpectedSize(urec);
> > ..
> > ..
> > + if (UndoRecPtrIsValid(multi_prep_urp))
> > + {
> > + UndoLogOffset insert = UndoRecPtrGetOffset(multi_prep_urp);
> > + insert = UndoLogOffsetPlusUsableBytes(insert, size);
> > + multi_prep_urp = MakeUndoRecPtr(UndoRecPtrGetLogNo(urecptr), insert);
> > + }
> >
> > Can't we use urecptr instead of multi_prep_urp in above code after
> > urecptr is initialized?
>
> I think we can't because urecptr is just the current pointer we are
> going to return but multi_prep_urp is the static variable we need to
> update so that
> the next prepare can calculate urecptr from this location.
> >

Okay, but that was not apparent from the code, so added a comment in
the attached delta patch.  BTW, wouldn't it be better to move this
code to the end of function once prepare for current record is
complete.

More comments

1.
* We can consider that the log as switched if

/that/ needs to be removed.

2.
+ if (prepare_idx == max_prepared_undo)
+ elog(ERROR, "Already reached the maximum prepared limit.");

a. /Already/already
b. we don't use full-stop (.) in error

3.
+ * also stores the dbid and the  progress of the undo apply during rollback.

/the  progress/  extra space.

4.
+UndoSetPrepareSize(int nrecords, UnpackedUndoRecord *undorecords,
+TransactionId xid, UndoPersistence upersistence)
+{

nrecords should be a second parameter.

5.
+UndoRecPtr
+PrepareUndoInsert(UnpackedUndoRecord *urec, UndoPersistence upersistence,
+   TransactionId xid)

It seems better to have xid parameter before UndoPersistence.

6.
+ /* FIXME: Should we just report error ? */
+ Assert(index < MAX_BUFFER_PER_UNDO);

No need of this Fixme.

7.
PrepareUndoInsert()
{
..
do
{
..
+ /* Undo record can not fit into this block so go to the next block. */
+ cur_blk++;
..
} while (cur_size < size);
..
}

This comment was making me uneasy, so slightly adjusted the code.
Basically, by that time it was not decided whether undo record can fit
in current buffer or not.

8.
+ /*
+ * Save referenced of undo record pointer as well as undo record.
+ * InsertPreparedUndo will use these to insert the prepared record.
+ */
+ prepared_undo[prepare_idx].urec = urec;
+ prepared_undo[prepare_idx].urp = urecptr;

Slightly adjust the above comment.

9.
+InsertPreparedUndo(void)
{
..
+ Assert(prepare_idx > 0);
+
+ /* This must be called under a critical section. */
+ Assert(InRecovery || CritSectionCount > 0);
..
}

I have added a few more comments for above assertions, see if those are correct.

10.
+InsertPreparedUndo(void)
{
..
+ prev_undolen = undo_len;
+
+ UndoLogSetPrevLen(UndoRecPtrGetLogNo(urp), prev_undolen);
..
}

There is no need to use an additional variable prev_undolen in the
above code. I have modified the code to remove it's usage, check if
that is correct.

11.
+ * Fetch the next undo record for given blkno, offset and transaction id (if
+ * valid).  We need to match transaction id along with block number and offset
+ * because in some cases (like reuse of slot for committed transaction), we
+ * need to skip the record if it is modified by a transaction later than the
+ * transaction indicated by previous undo record.  For example, consider a
+ * case where tuple (ctid - 0,1) is modified by transaction id 500 which
+ * belongs to transaction slot 0. Then, the same tuple is modified by
+ * transaction id 501 which belongs to transaction slot 1.  Then, both the
+ * transaction slots are marked for reuse. Then, again the same tuple is
+ * modified by transaction id 502 which has used slot 0.  Now, some
+ * transaction which has started before transaction 500 wants to traverse the
+ * chain to find visible tuple will keep on rotating infinitely between undo
+ * tuple written by 502 and 501.  In such a case, we need to skip the undo
+ * tuple written by transaction 502 when we want to find the undo record
+ * indicated by the previous pointer of undo tuple written by transaction 501.
+ * Start the search from urp.  Caller need to call UndoRecordRelease
to release the
+ * resources allocated by this function.
+ *
+ * urec_ptr_out is undo record pointer of the qualified undo record if valid
+ * pointer is passed.
+ */
+UnpackedUndoRecord *
+UndoFetchRecord(UndoRecPtr urp, BlockNumber blkno, OffsetNumber offset,
+ TransactionId xid, UndoRecPtr * urec_ptr_out,
+ SatisfyUndoRecordCallback callback)


The comment above UndoFetchRecord is very zheap specific, so I have
tried to simplify it.  I think we can give so much detailed examples
only when we introduce zheap code.

Apart from above, there are miscellaneous comments and minor code
edits in the attached delta patch.

12.
PrepareUndoInsert()
{
..
+